-
Notifications
You must be signed in to change notification settings - Fork 0
Feat/improved agent #31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
8f0293b to
7645efc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a major architectural change to the agent system, replacing the draft-based workflow with a Virtual File System (VFS) that stages changes before applying them. The agent now works with file diffs instead of text drafts, and the reviewer provides feedback through structured comments.
Changes:
- Introduced VFS service for staging file modifications and generating diffs
- Refactored agent workflow to use writer/reviewer tools with VFS integration
- Added theme system with multiple UI themes and diff viewer
- Implemented session resume/retry functionality
- Enhanced web fetch with PDF support and binary content detection
Reviewed changes
Copilot reviewed 54 out of 55 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/vfs.ts | New VFS service for staging file changes and generating diffs |
| src/services/agent.ts | Refactored agent to use VFS-based workflow with session resume |
| src/tui/hooks/useAgent.ts | Changed to global state with useSyncExternalStore |
| src/tui/theme/index.ts | New theme system with GitHub Dark, Monokai, and Dracula themes |
| src/tui/components/* | New DiffReviewModal, DiffView, Sidebar, and updated components |
| src/tools/vfs.ts | New VFS-based file manipulation tools |
| src/tools/review.ts | New reviewer tools for commenting and approving/rejecting |
| src/services/prompts.ts | Updated prompts for VFS-based workflow |
| src/services/web.ts | Added PDF extraction and binary content detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/tui/hooks/useAgent.ts
Outdated
| let globalState: AgentState = { ...initialAgentState }; | ||
| const listeners = new Set<() => void>(); | ||
|
|
||
| let globalSession: { | ||
| submitUserAction: (action: UserAction) => Effect.Effect<void, unknown>; | ||
| cancel: () => Effect.Effect<void>; | ||
| fiber: Fiber.RuntimeFiber<void, unknown> | null; | ||
| } | null = null; | ||
|
|
||
| let globalLastRunOptions: RunOptions | null = null; |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using global mutable state in React hooks creates race conditions when multiple instances exist or components unmount/remount. This violates React's rules of hooks and can lead to memory leaks. Consider using React Context or a proper state management solution instead of module-level globals.
| const step = Effect.fn("step")(function* ( | ||
| currentCycle: number, | ||
| ): Effect.fn.Return< |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The step function is recursive (line 578: return yield* step(cycle) and line 608) without a clear base case at the function signature level. While there is an iteration limit check at line 409, the recursive pattern makes the code harder to follow. Consider refactoring to use an iterative loop with explicit continuation conditions.
| } | ||
|
|
||
| return yield* Effect.fail(new UserDirError({ message: "Could not determine config directory" })); | ||
| return yield* new UserDirError({ message: "Could not determine config directory" }); |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changed from Effect.fail(new UserDirError(...)) to yield* new UserDirError(...). The new syntax is incorrect - you cannot yield from a Data.TaggedError instance. This will cause a runtime error. Should be return yield* Effect.fail(new UserDirError(...)).
| } | ||
|
|
||
| return yield* Effect.fail(new UserDirError({ message: "Could not determine data directory" })); | ||
| return yield* new UserDirError({ message: "Could not determine data directory" }); |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as line 27 - incorrect Effect error handling syntax. Should be return yield* Effect.fail(new UserDirError(...)).
| return yield* new FileReadError({ | ||
| cause: "Access denied", | ||
| message: `Access denied: ${resolved} is outside of ${cwd}`, | ||
| }); |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect Effect error handling syntax - cannot yield from error constructor. Should be return yield* Effect.fail(new FileReadError(...)).
| return yield* new FileReadError({ | |
| cause: "Access denied", | |
| message: `Access denied: ${resolved} is outside of ${cwd}`, | |
| }); | |
| return yield* Effect.fail( | |
| new FileReadError({ | |
| cause: "Access denied", | |
| message: `Access denied: ${resolved} is outside of ${cwd}`, | |
| }), | |
| ); |
| return yield* new AgentStreamError({ | ||
| message: "Prompt is required for new sessions", | ||
| cause: new Error("Missing prompt"), | ||
| }); |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect Effect error handling - yielding from error constructor instead of Effect.fail. Should be return yield* Effect.fail(new AgentStreamError(...)).
| return yield* new AgentStreamError({ | |
| message: "Prompt is required for new sessions", | |
| cause: new Error("Missing prompt"), | |
| }); | |
| return yield* Effect.fail( | |
| new AgentStreamError({ | |
| message: "Prompt is required for new sessions", | |
| cause: new Error("Missing prompt"), | |
| }), | |
| ); |
| export const ANTIGRAVITY_ENDPOINT_SANDBOX = "https://daily-cloudcode-pa.sandbox.googleapis.com"; | ||
|
|
||
| export const ANTIGRAVITY_DEFAULT_ENDPOINT = ANTIGRAVITY_ENDPOINT_DAILY; | ||
| export const ANTIGRAVITY_DEFAULT_ENDPOINT = ANTIGRAVITY_ENDPOINT_SANDBOX; |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed from DAILY to SANDBOX endpoint. This appears to be a configuration change that should not be committed to the main branch as it affects the production behavior. Consider using environment variables for endpoint selection.
| const command = Command.make("jot", {}, () => | ||
| Effect.tryPromise({ | ||
| try: () => startTUI(), | ||
| catch: (error) => new TUIStartupError({ cause: error, message: `Failed to start TUI: ${error}` }), | ||
| }), | ||
| ).pipe( | ||
| Command.withSubcommands([writeCommand, configCommand, antigravityCommand]), |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default command now always starts the TUI. The removed auth command is no longer accessible. If this is intentional, ensure users have an alternative way to authenticate.
This commit fixes a bug where resuming a session would reset the VFS because the agent loop started from cycle 0 instead of the saved cycle count. It also refactors the Session service to reuse logic and adds a regression test.
Better inputs, themes support, global keymap, diff view for changes
This fixes an issue where the DiffReviewModal would appear empty. The root cause was the DiffView component having flexGrow: 1 inside a scrollbox, which caused layout issues. We also ensured the scrollbox is focused to allow scrolling.
1f44609 to
7c21a85
Compare
No description provided.