Repository navigation
feat: auto-restore sessions opened in provider GUI (Fixes #32) - #33
Conversation
When packing, leave tiny listable stubs so providers can still show sessions. Wrappers now run `watch` with the real binary so opening a stub in the GUI materializes the full archive without an extra command. Fixes #32
|
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:XL
PR Summary by QodoAuto-restore packed sessions opened via provider GUI using stubs + watch
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| const child = spawn(command, commandArgs, { | ||
| stdio: 'inherit', | ||
| env: process.env, | ||
| }); | ||
|
|
||
| child.on('exit', (code) => { | ||
| childExitCode = code ?? 0; | ||
| stop = true; |
There was a problem hiding this comment.
Suggestion: The spawned provider is not cleaned up when watchSessionStubs fails. The provider child can remain running with inherited stdio while the watch command has already failed, leaving an orphaned provider process. Keep the child handle and terminate or await it in a failure cleanup path; also handle the child error event explicitly. [missing cleanup]
Severity Level: Major ⚠️
- ⚠️ Wrapped provider can outlive failed watcher command.
- ⚠️ Orphaned processes retain provider resources and stdio.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/cli/commands/watchCommand.ts
**Line:** 142:149
**Comment:**
*Missing Cleanup: The spawned provider is not cleaned up when `watchSessionStubs` fails. The provider child can remain running with inherited stdio while the watch command has already failed, leaving an orphaned provider process. Keep the child handle and terminate or await it in a failure cleanup path; also handle the child `error` event explicitly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| try { | ||
| const watcher = watch(manifest.originalPath, { persistent: true }, () => { | ||
| // Any FS event on a stub is treated as provider open intent. | ||
| void materializeIfNeeded(manifest, true); |
There was a problem hiding this comment.
Suggestion: The asynchronous restore launched from the filesystem callback is detached with void, so failures from ensureSessionRestored become unhandled promise rejections instead of being returned through the declared Effect error channel. A corrupt archive or filesystem failure can therefore leave the watcher running while the caller receives no failure status. Catch the callback promise and route the failure into the watch lifecycle or a reported error state. [possible bug]
Severity Level: Major ⚠️
- ⚠️ Corrupt archives fail without watcher status reporting.
- ⚠️ GUI opens can leave the stub unmaterialized.
- ⚠️ Detached failures bypass `SessionWatchError` handling.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/sessionWatch.ts
**Line:** 95:95
**Comment:**
*Possible Bug: The asynchronous restore launched from the filesystem callback is detached with `void`, so failures from `ensureSessionRestored` become unhandled promise rejections instead of being returned through the declared Effect error channel. A corrupt archive or filesystem failure can therefore leave the watcher running while the caller receives no failure status. Catch the callback promise and route the failure into the watch lifecycle or a reported error state.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| while (!request.shouldStop()) { | ||
| const manifests = yield* listVaultSessionManifests(request.vaultPath); |
There was a problem hiding this comment.
Suggestion: The watcher handles are closed only after the polling loop exits normally. If listVaultSessionManifests or another polling operation fails, the Effect exits before this cleanup runs, while the persistent: true filesystem watchers remain active and can keep the process alive. Put watcher cleanup in a guaranteed finalizer or try/finally path. [resource leak]
Severity Level: Major ⚠️
- ⚠️ Vault read failures bypass filesystem watcher cleanup.
- ⚠️ Persistent handles can keep failed watch processes alive.
- ⚠️ Later stub events may invoke stale callbacks.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/sessionWatch.ts
**Line:** 154:155
**Comment:**
*Resource Leak: The watcher handles are closed only after the polling loop exits normally. If `listVaultSessionManifests` or another polling operation fails, the Effect exits before this cleanup runs, while the `persistent: true` filesystem watchers remain active and can keep the process alive. Put watcher cleanup in a guaranteed finalizer or `try/finally` path.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
|
||
| # Follow the provider process and materialize GUI-opened stubs (no session id in argv). | ||
| if command -v node >/dev/null 2>&1; then | ||
| exec node "$CLI_ENTRY" watch --provider "$PROVIDER" -- "$REAL_BIN" "$@" |
There was a problem hiding this comment.
Suggestion: The wrapper now starts the real provider only through watch, but unlike the preflight call there is no best-effort fallback. Any watch initialization error, such as an unreadable or malformed setup configuration or vault, causes the wrapper to exit before launching the provider. Preserve the direct provider fallback when watcher startup fails. [error handling]
Severity Level: Major ⚠️
- ❌ Wrapped provider launch fails with invalid setup configuration.
- ⚠️ Codex, Claude, Grok, and other wrappers become unavailable.
- ⚠️ Provider launch depends on watcher configuration availability.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/lifecycleWrappers.ts
**Line:** 165:165
**Comment:**
*Error Handling: The wrapper now starts the real provider only through `watch`, but unlike the preflight call there is no best-effort fallback. Any watch initialization error, such as an unreadable or malformed setup configuration or vault, causes the wrapper to exit before launching the provider. Preserve the direct provider fallback when watcher startup fails.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if (isStub) { | ||
| yield* removePath(request.originalPath); | ||
| return 'continue-restore' as const; | ||
| } |
There was a problem hiding this comment.
Suggestion: The stub is deleted before archive extraction and verification complete. A corrupt, unavailable, or incompatible archive therefore leaves the provider path missing instead of retaining the listable stub for a later retry. Defer deletion until restoration succeeds, or recreate the stub on every restore failure. [state/lifecycle]
Severity Level: Major ⚠️
- ⚠️ Corrupt archives remove GUI-visible session stubs.
- ⚠️ Provider session lists hide sessions during failed restore.
- ⚠️ Users must retry restoration manually.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/sessionArchive.ts
**Line:** 641:644
**Comment:**
*State Lifecycle: The stub is deleted before archive extraction and verification complete. A corrupt, unavailable, or incompatible archive therefore leaves the provider path missing instead of retaining the listable stub for a later retry. Defer deletion until restoration succeeds, or recreate the stub on every restore failure.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Code Review by Qodo
1. Unhandled watch rejections
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| # Follow the provider process and materialize GUI-opened stubs (no session id in argv). | ||
| if command -v node >/dev/null 2>&1; then | ||
| exec node "$CLI_ENTRY" watch --provider "$PROVIDER" -- "$REAL_BIN" "$@" | ||
| fi |
There was a problem hiding this comment.
1. Wrapper invokes watch command 📘 Rule violation ⛨ Security
The provider wrapper now executes agent-session-pack watch, which performs filesystem operations on real local session paths outside the approved CLI entry points. This violates the restriction that only pnpm savings, pnpm evidence:local, or agent-session-pack check may reach real session-directory access code.
Agent Prompt
## Issue description
`watch` is a new CLI entry point that watches/restores sessions by accessing real local session directories, but the compliance policy restricts such access to specific approved entry points only.
## Issue Context
`src/core/lifecycleWrappers.ts` now automatically runs `watch` for provider wrappers, making real session-directory access reachable from many non-approved entrypoints.
## Fix Focus Areas
- src/core/lifecycleWrappers.ts[163-166]
- src/cli/commands/watchCommand.ts[121-129]
Potential approaches:
- Add an explicit runtime guard (similar to an `assertApprovedEntryPoint`) that only permits real session-dir access when invoked via the approved entry points (or a dedicated, explicitly-approved env var).
- If `watch` is intended to be an approved entry point, update the project’s entrypoint/guard conventions so `watch` is treated equivalently (and ensure wrappers invoke only the guarded path).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const isStub = yield* isArchivedStubPath(request.originalPath, request.sourceKind); | ||
| if (isStub) { | ||
| yield* removePath(request.originalPath); | ||
| return 'continue-restore' as const; |
There was a problem hiding this comment.
2. Stub delete lacks byte backup 📘 Rule violation ☼ Reliability
The restore path deletes an archived stub at originalPath via removePath() without first preserving the original bytes to a backup/archive location. This risks data loss on destructive operations and violates the required pre-delete byte preservation step.
Agent Prompt
## Issue description
A destructive deletion (`removePath(request.originalPath)`) happens when the existing path is detected as an archived stub, but no byte-preserving backup is performed immediately before deletion.
## Issue Context
Even if the path is “only a stub”, the compliance rule requires preserving exact bytes before deletion operations.
## Fix Focus Areas
- src/core/sessionArchive.ts[640-643]
Potential approaches:
- Introduce and use a helper like `removePathWithBackup({ path, vaultPath })` that reads raw bytes and writes them to a deterministic backup location before deletion.
- Alternatively, replace deletion with a safe rename/move into the vault (or another configured backup area) before continuing restore.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const parsePollMs = (value: string | undefined): number => { | ||
| if (value === undefined) { | ||
| return 750; | ||
| } | ||
|
|
||
| const parsed = Number(value); | ||
| if (!Number.isFinite(parsed) || parsed < 100) { | ||
| return 750; | ||
| } |
There was a problem hiding this comment.
3. poll-ms parsed without schema 📘 Rule violation ⚙ Maintainability
The watch CLI boundary uses ad-hoc parsing for poll-ms via Number(...) checks rather than Effect Schema validation. This reduces consistency and makes it easier for invalid inputs to bypass structured boundary validation.
Agent Prompt
## Issue description
`poll-ms` is validated with manual numeric parsing instead of Effect Schema at the CLI boundary.
## Issue Context
The compliance rule prefers Effect Schema validation at module boundaries (like CLI parsing) over ad-hoc checks when schemas can express the constraints.
## Fix Focus Areas
- src/cli/commands/watchCommand.ts[210-218]
Potential approaches:
- Define an Effect Schema for `pollMs` (e.g., `Schema.NumberFromString` + bounds/refinement) and decode it, returning a typed error/effect on invalid input.
- Optionally define a full args schema for `WatchArgs` and parse/decode once at the boundary, rather than per-field parsing.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const watcher = watch(manifest.originalPath, { persistent: true }, () => { | ||
| // Any FS event on a stub is treated as provider open intent. | ||
| void materializeIfNeeded(manifest, true); | ||
| }); |
There was a problem hiding this comment.
4. Unhandled watch rejections 🐞 Bug ☼ Reliability
The fs.watch callback fires async materializeIfNeeded() and discards its Promise; if ensureSessionRestored() fails, the rejection is unhandled and can terminate or destabilize the watch process (and wrappers). This can cause missed restores and/or unexpected wrapper failures during normal provider usage.
Agent Prompt
### Issue description
`fs.watch` invokes an async function (`materializeIfNeeded`) but its Promise is not handled. Any rejection (e.g., from `ensureSessionRestored`) becomes an unhandled rejection, which may terminate the process depending on runtime settings.
### Issue Context
This happens in the new `watchSessionStubs` implementation and impacts lifecycle wrappers that now exec `watch`.
### Fix Focus Areas
- src/core/sessionWatch.ts[92-96]
- src/core/sessionWatch.ts[111-152]
### What to change
- In the `fs.watch` callback, explicitly handle the returned Promise:
- `materializeIfNeeded(...).catch(err => onEvent({status:'error', ...}) / log to stderr)`
- or refactor `materializeIfNeeded` into an `Effect` and run it via an Effect runtime/fiber with structured error handling.
- Add basic per-session/path deduping (optional but recommended): avoid concurrent restores for the same `originalPath` when multiple events fire in quick succession.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| while (!request.shouldStop()) { | ||
| const manifests = yield* listVaultSessionManifests(request.vaultPath); | ||
| const scoped = manifests.filter( | ||
| (manifest) => request.provider === undefined || manifest.provider === request.provider, |
There was a problem hiding this comment.
5. Watch loop is i/o heavy 🐞 Bug ➹ Performance
watchSessionStubs re-reads all vault manifests every poll interval and reads/parses each original session file to check for stub markers, even after sessions have been restored. This can create sustained disk I/O and CPU overhead while a provider runs (especially with many manifests or large session files).
Agent Prompt
### Issue description
The watch loop polls by repeatedly:
1) listing+reading all manifests from disk, and
2) reading full contents of `manifest.originalPath` to detect file stubs.
This can be expensive at runtime.
### Issue Context
This affects the new `watch` feature that wrappers run continuously while the provider runs.
### Fix Focus Areas
- src/core/sessionWatch.ts[154-171]
- src/core/sessionWatch.ts[115-132]
- src/core/sessionStub.ts[102-118]
- src/core/manifestStore.ts[79-95]
- src/core/manifestStore.ts[108-118]
### What to change
- Avoid re-reading all manifests every poll:
- cache manifests in memory and refresh less frequently, or
- add a watcher on the vault manifest directory to update the set when manifests change.
- Avoid reading full session files to detect stubs:
- for file stubs, read only a small prefix (or use a lightweight marker strategy),
- short-circuit checks once a path is known to be fully restored (stop checking/stop watching if appropriate).
- Consider making polling only a fallback for watcher registration/missed events, rather than scanning all manifests and paths continuously.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| child.on('exit', (code) => { | ||
| childExitCode = code ?? 0; | ||
| stop = true; | ||
| }); |
There was a problem hiding this comment.
6. Child exit misreported 🐞 Bug ≡ Correctness
watchCommand doesn’t handle child process spawn errors and treats signal-based termination as exit code 0 (code ?? 0), so wrappers can incorrectly report success or crash on failed spawns. This breaks the wrapper contract of returning the provider’s real exit status.
Agent Prompt
### Issue description
When `watch` follows a child process:
- a failed spawn emits an `error` event which is currently unhandled,
- the `exit` handler maps `code === null` (signal termination) to `0`, reporting success.
### Issue Context
Lifecycle wrappers now `exec ... watch -- <real-bin> ...`, so `watch` must accurately propagate provider termination.
### Fix Focus Areas
- src/cli/commands/watchCommand.ts[142-150]
- src/cli/commands/watchCommand.ts[183-185]
### What to change
- Add `child.on('error', err => { childExitCode = 127 (or 1); stop = true; /* log */ })`.
- Update `child.on('exit', (code, signal) => { ... })` to handle `signal`:
- map to a non-zero exit code (commonly `128 + signalNumber` if available), or
- at minimum treat `code === null` as non-zero and include signal name in stderr output.
- Ensure `process.exitCode` reflects the derived exit status.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
User description
Fixes #32
Summary
watchmaterializes a stub when the provider opens/touches it (GUI session pick).watch --provider X -- <real-cli> …so Grok/Codex/Claude launches get this without a second command.Confidence
8/10 — Unit tests for stubs/watch/pack+restore; GUI behavior depends on the provider writing/touching the stub path (Grok multi-file dirs work well; pure read-only opens may need a touch/event).
Test plan
pnpm test128 passpnpm typecheck && pnpm check:ci && pnpm buildMain safety
Summary by cubic
Auto-restore archived sessions when opened from a provider GUI by leaving tiny stubs on pack and running a new watch process to materialize them on open. This keeps sessions listable and removes the need for a second command.
watchCLI to monitor stubs and restore on access; supports provider filter, JSON events, and polling fallback.watch --provider <X> -- <real-bin> …afterpreflightso Grok/Codex/Claude launches auto-restore.Written for commit ec6e528. Summary will update on new commits.
CodeAnt-AI Description
Restore packed sessions automatically when selected in provider apps
What Changed
watchcommand with provider filtering, polling, JSON events, and support for running a provider process.Impact
✅ Packed sessions remain visible in provider GUIs✅ GUI-selected sessions restore without a separate command✅ Fewer accidental overwrites of active sessions💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.