feat(workspace): reuse checkout opens and trim repeated bootstrap - #125
feat(workspace): reuse checkout opens and trim repeated bootstrap#125Waishnav wants to merge 9 commits into
Conversation
Greptile SummaryThe PR separates workspace reuse from bootstrap delivery.
Confidence Score: 4/5The PR should not merge until partial checkpoint restoration preserves unreviewed edits when the baseline ref is missing. When only the open review ref survives a restart, initialization writes a snapshot of the already-edited working tree into the missing baseline ref; the next last-shown comparison therefore reports no pending changes. Files Needing Attention: src/review-checkpoints.ts, src/review-checkpoints.test.ts
|
| Filename | Overview |
|---|---|
| src/workspaces.ts | Adds canonical project keys, persisted checkout reuse, stale-directory replacement, fresh worktree behavior, and project-level bootstrap claims. |
| src/workspace-store.ts | Adds persisted conversation bindings and atomic project-bootstrap claims. |
| src/review-checkpoints.ts | Makes checkpoint initialization restart-aware and deduplicates concurrent initialization. |
| src/server.ts | Integrates conversation-scoped workspace opens, compact model responses, complete card payloads, and awaited checkpoint registration. |
| src/db/migrations.ts | Adds binding and bootstrap-ledger migrations with backfill from existing conversation bindings. |
Sequence Diagram
sequenceDiagram
participant C as ChatGPT
participant S as MCP server
participant W as Workspace registry
participant DB as SQLite store
participant R as Review checkpoints
C->>S: open_workspace(path, mode, session)
S->>W: openWorkspace(input, conversation hash)
W->>DB: lookup binding / claim bootstrap
alt checkout binding is usable
W-->>S: reused workspace
else checkout is new or stale
W->>DB: persist replacement binding
W-->>S: new checkout workspace
else worktree mode
W-->>S: fresh managed worktree
end
S->>R: initializeWorkspace(workspaceId, root)
R-->>S: checkpoint state registered
S-->>C: compact model result + complete hidden card payload
Reviews (5): Last reviewed commit: "fix(server): separate workspace reuse fr..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
2e2c471 to
866d49e
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChatGPT session metadata now scopes repeated ChangesConversation-scoped workspace reuse
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server.ts (1)
755-772: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAdd the
reopeninput that the PR objectives and the docs promise.The PR objectives state that
reopen: truerequests a fresh workspace. The input schema at lines 756-772 accepts onlypath,mode, andbaseRef. Noreopenfield exists, andOpenWorkspaceOptionsin src/workspaces.ts carries onlyconversationScopeHash.Without that input there is no way to force a new workspace inside one ChatGPT conversation. docs/chatgpt-coding-workflow.md line 34 still lists "the user explicitly asks to reopen" as a reopen trigger, but a repeated call now returns the existing
workspaceIdand omits bootstrap details. A user request to reopen therefore cannot be satisfied.Add a
reopenboolean to the input schema, forward it throughOpenWorkspaceOptions, and bypass the binding lookup when it is set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.ts` around lines 755 - 772, Add an optional boolean reopen field to the workspace tool input schema and OpenWorkspaceOptions, then propagate it through the workspace-opening flow. Update the binding lookup logic to bypass the existing workspace when reopen is true, while preserving normal reuse behavior when it is false or omitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/workspaces.ts`:
- Around line 100-121: Make concurrent conversation-scoped opens deterministic
by updating openWorkspace in src/workspaces.ts#L100-L121 to register the
in-flight promise synchronously, keyed by conversationScopeHash, and compute
targetKey inside that promise; retain cleanup of the exact registered promise.
Update src/workspaces.test.ts#L194-L208 to assert order-independent results:
both calls share one workspace.id and workspace.root, and exactly one result has
includeBootstrapContext === true.
---
Outside diff comments:
In `@src/server.ts`:
- Around line 755-772: Add an optional boolean reopen field to the workspace
tool input schema and OpenWorkspaceOptions, then propagate it through the
workspace-opening flow. Update the binding lookup logic to bypass the existing
workspace when reopen is true, while preserving normal reuse behavior when it is
false or omitted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e525385f-701f-49ca-a756-40126f1e95cc
📒 Files selected for processing (13)
docs/chatgpt-coding-workflow.mdpackage.jsonsrc/db/migrations.tssrc/db/schema.tssrc/oauth-store.test.tssrc/request-meta.test.tssrc/request-meta.tssrc/server.tssrc/ui/tool-display.test.tssrc/ui/tool-display.tssrc/workspace-store.tssrc/workspaces.test.tssrc/workspaces.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/workspace-store.ts (1)
144-172: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid the extra round trip and the unchecked non-null assertion in
setConversationBinding.After
.onConflictDoUpdate(...).run(), the method re-queries withthis.getConversationBinding(...)!. This adds an extra database round trip. The!only affects the type; it performs no runtime check. IfgetConversationBindingever returnsundefinedat that point (for example, a delete of the binding races with this call), the method returnsundefinedtyped asWorkspaceConversationBinding, and a caller can later dereference it and crash.drizzle-orm supports
.returning()after.onConflictDoUpdate()for SQLite, including composite-key targets. Use it to fetch the affected row from the same statement.♻️ Proposed fix using `.returning()`
setConversationBinding(input: { conversationScopeHash: string; targetKey: string; workspaceSessionId: string; }): WorkspaceConversationBinding { const now = new Date().toISOString(); - this.database.db + const [row] = this.database.db .insert(workspaceConversationBindings) .values({ conversationScopeHash: input.conversationScopeHash, targetKey: input.targetKey, workspaceSessionId: input.workspaceSessionId, createdAt: now, lastUsedAt: now, }) .onConflictDoUpdate({ target: [ workspaceConversationBindings.conversationScopeHash, workspaceConversationBindings.targetKey, ], set: { workspaceSessionId: input.workspaceSessionId, lastUsedAt: now, }, }) - .run(); - - return this.getConversationBinding(input.conversationScopeHash, input.targetKey)!; + .returning() + .all(); + + return rowToWorkspaceConversationBinding(row); }Please confirm
.returning()behaves as expected with thebetter-sqlite3driver at drizzle-orm 0.45.2 before applying.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workspace-store.ts` around lines 144 - 172, Update setConversationBinding to append Drizzle’s returning() to the insert/onConflictDoUpdate statement and capture the affected row from run(), eliminating the follow-up getConversationBinding query and its non-null assertion. Preserve the composite conflict target and return the returned WorkspaceConversationBinding, with explicit handling if the statement unexpectedly returns no row.src/server.ts (1)
896-902: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueReport
skillDiagnosticsas 0 for reused workspaces.The other summary counts derive from the suppressed collections and become 0 on reuse.
skillDiagnosticsstill reportsworkspace.skillDiagnostics.length, so the card can advertise diagnostics that the response omits.♻️ Proposed summary alignment
- skillDiagnostics: workspace.skillDiagnostics.length, + skillDiagnostics: includeBootstrapContext ? workspace.skillDiagnostics.length : 0,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.ts` around lines 896 - 902, Update the summary construction around the reused workspace handling so the skillDiagnostics count is reported as 0 when reused, matching the suppressed diagnostics collection in the response; retain workspace.skillDiagnostics.length for non-reused workspaces.src/workspaces.ts (1)
139-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClean up the orphaned workspace session when a binding is discarded.
The recovery path removes the in-memory entry and the binding row. It leaves the persisted
workspaceSessionsrow, so stale rows accumulate for every discarded binding. Thecatchblock also hides the recovery reason, which makes allowed-root or filesystem problems hard to diagnose.Consider deleting or marking the session record and logging the discarded binding at debug level. Do you want me to open an issue to track session cleanup for discarded bindings?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workspaces.ts` around lines 139 - 154, Update the discarded-binding recovery path in the workspace binding logic to also remove or mark deleted the persisted workspace session associated with binding.workspaceSessionId, alongside the existing in-memory and binding cleanup. Replace the empty catch in this path with debug-level logging that includes the binding/session identifiers and the failure details, while preserving recovery by discarding the unusable binding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/workspaces.test.ts`:
- Around line 194-208: Update the concurrent open assertions in the
persistentRegistry.openWorkspace test to avoid assuming which result has
includeBootstrapContext true. Assert that both results share the same workspace
identity and that exactly one result includes bootstrap context, while
preserving the existing shared-root and agent-file assertions.
---
Nitpick comments:
In `@src/server.ts`:
- Around line 896-902: Update the summary construction around the reused
workspace handling so the skillDiagnostics count is reported as 0 when reused,
matching the suppressed diagnostics collection in the response; retain
workspace.skillDiagnostics.length for non-reused workspaces.
In `@src/workspace-store.ts`:
- Around line 144-172: Update setConversationBinding to append Drizzle’s
returning() to the insert/onConflictDoUpdate statement and capture the affected
row from run(), eliminating the follow-up getConversationBinding query and its
non-null assertion. Preserve the composite conflict target and return the
returned WorkspaceConversationBinding, with explicit handling if the statement
unexpectedly returns no row.
In `@src/workspaces.ts`:
- Around line 139-154: Update the discarded-binding recovery path in the
workspace binding logic to also remove or mark deleted the persisted workspace
session associated with binding.workspaceSessionId, alongside the existing
in-memory and binding cleanup. Replace the empty catch in this path with
debug-level logging that includes the binding/session identifiers and the
failure details, while preserving recovery by discarding the unusable binding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d5a656d-0a46-4528-bd92-fe0506cbb741
📒 Files selected for processing (13)
docs/chatgpt-coding-workflow.mdpackage.jsonsrc/db/migrations.tssrc/db/schema.tssrc/oauth-store.test.tssrc/request-meta.test.tssrc/request-meta.tssrc/server.tssrc/ui/tool-display.test.tssrc/ui/tool-display.tssrc/workspace-store.tssrc/workspaces.test.tssrc/workspaces.ts
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ui/card-types.ts`:
- Around line 52-53: Update isExpandableCard to return true when either
agentProviders or agents is present and non-empty, alongside the existing
expandable-content checks. Add a regression test covering a card containing only
these metadata arrays and verify both fields expose the expansion affordance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 45a10ac5-c984-44d4-a437-04754c3ff418
📒 Files selected for processing (6)
docs/chatgpt-coding-workflow.mdsrc/server.tssrc/ui/card-types.tssrc/ui/tool-display.test.tssrc/workspaces.test.tssrc/workspaces.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/chatgpt-coding-workflow.md
- src/server.ts
- src/workspaces.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/workspace-store.ts (1)
144-172: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
.returning()to avoid a redundant query and a non-null assertion.
setConversationBindingperformsinsert().onConflictDoUpdate()and then issues a separategetConversationBindingcall with a non-null assertion (!) to obtain the result. Drizzle's SQLite dialect supports.returning()directly after.onConflictDoUpdate(), including with a composite conflict target. Use it to return the upserted row from the same statement, removing the extra round-trip and the assumption that the row still exists when the second query runs.♻️ Proposed refactor using `.returning()`
setConversationBinding(input: { conversationScopeHash: string; targetKey: string; workspaceSessionId: string; }): WorkspaceConversationBinding { const now = new Date().toISOString(); - this.database.db + const [row] = this.database.db .insert(workspaceConversationBindings) .values({ conversationScopeHash: input.conversationScopeHash, targetKey: input.targetKey, workspaceSessionId: input.workspaceSessionId, createdAt: now, lastUsedAt: now, }) .onConflictDoUpdate({ target: [ workspaceConversationBindings.conversationScopeHash, workspaceConversationBindings.targetKey, ], set: { workspaceSessionId: input.workspaceSessionId, lastUsedAt: now, }, }) - .run(); - - return this.getConversationBinding(input.conversationScopeHash, input.targetKey)!; + .returning() + .all(); + + return rowToWorkspaceConversationBinding(row); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workspace-store.ts` around lines 144 - 172, Update setConversationBinding to chain returning() after onConflictDoUpdate(), capture the single returned workspace conversation binding, and return it directly. Remove the separate getConversationBinding call and its non-null assertion while preserving the existing insert values and composite conflict target.src/request-meta.ts (1)
11-20: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider a keyed hash for real anonymization.
openAiConversationScopeHashuses a plain, unkeyed SHA-256 hash. The"openai"prefix is a public constant, so it adds no secrecy. Anyone with a candidate session string can recompute the same hash and confirm a match against the persistedconversationScopeHashcolumn.Use HMAC-SHA256 with a server-local secret instead of a plain hash. This prevents offline correlation of persisted conversation scopes if the database is ever exposed, and keeps hash determinism per install.
Confirm whether
openai/sessionvalues are guaranteed to be high-entropy, opaque identifiers. If they are predictable or low-entropy, the current unkeyed hash provides weaker protection than the "anonymized" claim implies.🔒 Proposed HMAC-based fix
-import { createHash } from "node:crypto"; +import { createHmac } from "node:crypto"; + +const CONVERSATION_SCOPE_SECRET = + process.env.DEVSPACE_CONVERSATION_SCOPE_SECRET ?? "devspace-conversation-scope"; export function openAiConversationScopeHash( meta: Record<string, unknown> | undefined, ): string | undefined { const session = metadataString(meta, "openai/session"); if (!session) return undefined; - return createHash("sha256") - .update(JSON.stringify(["openai", session])) - .digest("hex"); + return createHmac("sha256", CONVERSATION_SCOPE_SECRET) + .update(JSON.stringify(["openai", session])) + .digest("hex"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/request-meta.ts` around lines 11 - 20, Update openAiConversationScopeHash to use deterministic HMAC-SHA256 with a server-local secret instead of the unkeyed createHash flow, preserving undefined behavior when openai/session is absent. Reuse the project’s established secret/configuration source and confirm whether session values are opaque high-entropy identifiers; ensure the resulting digest remains stable per installation.src/ui/tool-display.test.ts (1)
28-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese reuse cases do not exercise any reuse behavior.
src/server.tsLines 910-918 build the card summary frommode,agentsFiles,availableAgentsFiles,skills,agentProviders,agents, andskillDiagnostics. It never setsreused.getToolDisplayignoressummaryentirely, andgetToolHeaderSummaryreads onlymode,agentsFiles, andskills. Both assertions therefore pass for a key that no producer emits, so they pin no reuse contract.Choose one option:
- Add
reusedto the card summary insrc/server.tsand toToolHeaderSummaryhandling if the widget should show reuse.- Remove
reusedfrom these test cards to keep the fixtures aligned with the emitted summary.Also applies to: 102-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/tool-display.test.ts` around lines 28 - 31, Align the reuse test fixtures with the actual card-summary contract: either add and propagate reused through the server card summary and ToolHeaderSummary/getToolDisplay handling if reuse should affect the widget, or remove summary.reused from both assertions if it is not emitted. Keep the chosen behavior consistent across the affected tests and production symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server.ts`:
- Around line 811-816: Update the workspace setup around
reviewCheckpoints.initializeWorkspace so persisted workspaces are initialized
whenever reviewChanges/show_changes may access them, including when
includeBootstrapContext is false. Register or initialize the workspace before
the config.widgets === "changes" and includeBootstrapContext gate, while
preserving the existing initialization inputs and avoiding duplicate
initialization.
In `@src/workspaces.test.ts`:
- Around line 209-210: Update the concurrent open assertions in the workspace
test to avoid assuming which Promise.all result receives bootstrap context.
Assert that both results share the same workspace identity and that exactly one
concurrent open includes bootstrap context, preserving the existing behavior
without checking a fixed result order.
---
Nitpick comments:
In `@src/request-meta.ts`:
- Around line 11-20: Update openAiConversationScopeHash to use deterministic
HMAC-SHA256 with a server-local secret instead of the unkeyed createHash flow,
preserving undefined behavior when openai/session is absent. Reuse the project’s
established secret/configuration source and confirm whether session values are
opaque high-entropy identifiers; ensure the resulting digest remains stable per
installation.
In `@src/ui/tool-display.test.ts`:
- Around line 28-31: Align the reuse test fixtures with the actual card-summary
contract: either add and propagate reused through the server card summary and
ToolHeaderSummary/getToolDisplay handling if reuse should affect the widget, or
remove summary.reused from both assertions if it is not emitted. Keep the chosen
behavior consistent across the affected tests and production symbols.
In `@src/workspace-store.ts`:
- Around line 144-172: Update setConversationBinding to chain returning() after
onConflictDoUpdate(), capture the single returned workspace conversation
binding, and return it directly. Remove the separate getConversationBinding call
and its non-null assertion while preserving the existing insert values and
composite conflict target.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ac3719ff-95f2-4b9e-81c2-7f8d8846f21b
📒 Files selected for processing (13)
docs/chatgpt-coding-workflow.mdpackage.jsonsrc/db/migrations.tssrc/db/schema.tssrc/oauth-store.test.tssrc/request-meta.test.tssrc/request-meta.tssrc/server.tssrc/ui/card-types.tssrc/ui/tool-display.test.tssrc/workspace-store.tssrc/workspaces.test.tssrc/workspaces.ts
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/review-checkpoints.test.ts (1)
43-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the restart test to also cover
workspace_openpreservation.This test confirms that
since: "last_shown"(baselineRef) survives a manager restart.initializeWorkspaceStateinsrc/review-checkpoints.ts(lines 137-149) independently preservesopenRefandbaselineRefbased on separatehasCommitRefchecks. Add an assertion forsince: "workspace_open"after restart, so the test verifies both refs survive restart, not only the default one.🧪 Suggested addition
assert.equal(afterRestart.summary.files, 2); assert.match(afterRestart.patch, /world/); + + const sinceOpen = await restartedManager.reviewChanges({ + workspaceId: "ws_review", + root, + since: "workspace_open", + markReviewed: false, + }); + assert.equal(sinceOpen.summary.files, 2);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/review-checkpoints.test.ts` around lines 43 - 52, Extend the restart scenario in the review checkpoint test after the existing restartedManager review to call reviewChanges with since: "workspace_open" and markReviewed: false, then assert the result matches the expected workspace-open baseline (including the appropriate file count or patch content). Keep the existing last_shown assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/review-checkpoints.ts`:
- Around line 120-163: Update the reviewChanges state lookup to treat a cached
WorkspaceReviewState as ready only when it has either gitRoot or diagnostic;
when both are absent, re-enter initializeWorkspace so the existing
initialization promise is awaited instead of rejecting prematurely. Preserve the
existing deduplication behavior for concurrent workspace initialization and use
the relevant state/cache symbols already in the reviewChanges flow.
---
Nitpick comments:
In `@src/review-checkpoints.test.ts`:
- Around line 43-52: Extend the restart scenario in the review checkpoint test
after the existing restartedManager review to call reviewChanges with since:
"workspace_open" and markReviewed: false, then assert the result matches the
expected workspace-open baseline (including the appropriate file count or patch
content). Keep the existing last_shown assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b4d0541-f32c-4dc9-aeae-39ff81fcfce1
📒 Files selected for processing (3)
src/review-checkpoints.test.tssrc/review-checkpoints.tssrc/server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server.ts
| if (hasOpenRef && hasBaselineRef) return; | ||
|
|
||
| const commit = await createWorkingTreeSnapshot(eligibility.gitRoot); | ||
| if (!hasOpenRef) { | ||
| await git(eligibility.gitRoot, ["update-ref", state.openRef, commit]); | ||
| } | ||
| if (!hasBaselineRef) { | ||
| await git(eligibility.gitRoot, ["update-ref", state.baselineRef, commit]); |
There was a problem hiding this comment.
When DevSpace restarts with the workspace open ref present but the baseline ref missing, initialization preserves the open ref but snapshots the current edited tree into the missing baseline. The next show_changes call compares against that fresh baseline and reports no changes, omitting edits made before the restart.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/review-checkpoints.ts (1)
120-134: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPublish the workspace state only after initialization resolves.
Line 127 inserts an incomplete state into the shared
statesmap beforegetGitEligibilityruns.reviewChangestreats any present state as ready: it checksif (!state)at line 82, so it skipsinitializeWorkspace, then fails at line 87 on!state?.gitRoot. Ashow_changescall that overlaps an in-flightopen_workspaceinitialization therefore throws "show_changes requires a Git workspace in this version." even though initialization is about to succeed.The previous fix hardened the
initializeWorkspacefast path, but the early publication at line 127 is the root cause andreviewChangesstill reads the map directly. Build the state locally and insert it once, aftergitRootordiagnosticis set.🛡️ Proposed fix to remove the incomplete-state window
const refs = reviewRefs(workspaceId); const state: WorkspaceReviewState = { root, ...refs }; - states.set(workspaceId, state); try { const eligibility = await getGitEligibility(root); if (!eligibility.ok || !eligibility.gitRoot) { state.diagnostic = eligibility.message ?? "show_changes requires a Git workspace in this version."; return; } @@ } catch (error) { state.diagnostic = error instanceof Error ? error.message : String(error); + } finally { + states.set(workspaceId, state); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/review-checkpoints.ts` around lines 120 - 134, Update initializeWorkspaceState so the WorkspaceReviewState remains local while getGitEligibility and the subsequent initialization complete; move states.set(workspaceId, state) to the point after either gitRoot or diagnostic has been assigned. Preserve the existing initialized state contents and diagnostic behavior, ensuring reviewChanges never observes an incomplete state.
🧹 Nitpick comments (1)
src/workspace-store.ts (1)
144-202: 🗄️ Data Integrity & Integration | 🔵 TrivialAdd retention for unused
workspace_conversation_bindings.
workspace_conversation_bindingsaccumulates one row per conversation/target unless the same binding is reused or the linkedworkspace_sessionis deleted.last_used_atis updated, but no query or prune deletes stale binding rows, and no index covers it. Add alast_used_at-based cleanup path plus an index if this table is expected to grow long-term.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workspace-store.ts` around lines 144 - 202, The workspace conversation binding store updates lastUsedAt but never removes stale rows or indexes that timestamp. Add a lastUsedAt index to the workspaceConversationBindings schema and implement a cleanup method that deletes bindings older than a supplied retention cutoff, preserving active bindings; expose the cleanup through the store’s existing maintenance path if one exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/workspaces.ts`:
- Around line 137-166: Limit the try/catch in openConversationWorkspace to
validating the persisted binding: resolving the workspace, checking
workspace.root with stat, and determining whether it is a directory. Move
reusedWorkspaceContext(workspace) outside the try so errors from context
initialization, including agent/profile loading, propagate without deleting the
healthy binding or creating a replacement workspace. Preserve
touchConversationBinding and the existing cleanup behavior only when binding
validation fails.
- Around line 179-190: The reusedWorkspaceContext method should skip
loadInitialAgentsFiles and findAvailableAgentsFiles when includeBootstrapContext
is false, while preserving agentProfiles loading. Return empty agentsFiles and
availableAgentsFiles arrays for reused contexts, and update the reboundWorkspace
and concurrent worktree reuse assertions in the workspace tests to expect the
empty results.
---
Duplicate comments:
In `@src/review-checkpoints.ts`:
- Around line 120-134: Update initializeWorkspaceState so the
WorkspaceReviewState remains local while getGitEligibility and the subsequent
initialization complete; move states.set(workspaceId, state) to the point after
either gitRoot or diagnostic has been assigned. Preserve the existing
initialized state contents and diagnostic behavior, ensuring reviewChanges never
observes an incomplete state.
---
Nitpick comments:
In `@src/workspace-store.ts`:
- Around line 144-202: The workspace conversation binding store updates
lastUsedAt but never removes stale rows or indexes that timestamp. Add a
lastUsedAt index to the workspaceConversationBindings schema and implement a
cleanup method that deletes bindings older than a supplied retention cutoff,
preserving active bindings; expose the cleanup through the store’s existing
maintenance path if one exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d2ba1a0-3114-4e2f-b8e1-ce666cadf55f
📒 Files selected for processing (16)
docs/chatgpt-coding-workflow.mdpackage.jsonsrc/db/migrations.tssrc/db/schema.tssrc/oauth-store.test.tssrc/request-meta.test.tssrc/request-meta.tssrc/review-checkpoints.test.tssrc/review-checkpoints.tssrc/server.tssrc/ui/card-types.test.tssrc/ui/card-types.tssrc/ui/workspace-app.tsxsrc/workspace-store.tssrc/workspaces.test.tssrc/workspaces.ts
| private async openConversationWorkspace( | ||
| input: OpenWorkspaceInput, | ||
| conversationScopeHash: string, | ||
| targetKey: string, | ||
| ): Promise<WorkspaceContext> { | ||
| const binding = this.store?.getConversationBinding(conversationScopeHash, targetKey); | ||
| if (binding) { | ||
| try { | ||
| const workspace = this.getWorkspace(binding.workspaceSessionId); | ||
| const workspaceStats = await stat(workspace.root); | ||
| if (workspaceStats.isDirectory()) { | ||
| this.store?.touchConversationBinding(conversationScopeHash, targetKey); | ||
| return await this.reusedWorkspaceContext(workspace); | ||
| } | ||
| } catch { | ||
| // The persisted workspace is no longer usable; replace its binding below. | ||
| } | ||
|
|
||
| this.workspaces.delete(binding.workspaceSessionId); | ||
| this.store?.deleteConversationBinding(conversationScopeHash, targetKey); | ||
| } | ||
|
|
||
| const context = await this.openNewWorkspace(input); | ||
| this.store?.setConversationBinding({ | ||
| conversationScopeHash, | ||
| targetKey, | ||
| workspaceSessionId: context.workspace.id, | ||
| }); | ||
| return context; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Limit the recovery path to binding validation.
The try block covers reusedWorkspaceContext. If that call fails for a reason unrelated to the binding, for example loadInitialAgentsFiles or loadLocalAgentProfiles throws, the catch swallows the error, the code deletes a healthy binding, and openNewWorkspace creates a second workspace for the same target. In worktree mode that also creates a second managed worktree.
Validate the binding inside the try, then build the reused context outside it.
🛡️ Proposed narrower recovery scope
const binding = this.store?.getConversationBinding(conversationScopeHash, targetKey);
if (binding) {
+ let reusable: Workspace | undefined;
try {
const workspace = this.getWorkspace(binding.workspaceSessionId);
const workspaceStats = await stat(workspace.root);
if (workspaceStats.isDirectory()) {
- this.store?.touchConversationBinding(conversationScopeHash, targetKey);
- return await this.reusedWorkspaceContext(workspace);
+ reusable = workspace;
}
} catch {
// The persisted workspace is no longer usable; replace its binding below.
}
+ if (reusable) {
+ this.store?.touchConversationBinding(conversationScopeHash, targetKey);
+ return await this.reusedWorkspaceContext(reusable);
+ }
+
this.workspaces.delete(binding.workspaceSessionId);
this.store?.deleteConversationBinding(conversationScopeHash, targetKey);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async openConversationWorkspace( | |
| input: OpenWorkspaceInput, | |
| conversationScopeHash: string, | |
| targetKey: string, | |
| ): Promise<WorkspaceContext> { | |
| const binding = this.store?.getConversationBinding(conversationScopeHash, targetKey); | |
| if (binding) { | |
| try { | |
| const workspace = this.getWorkspace(binding.workspaceSessionId); | |
| const workspaceStats = await stat(workspace.root); | |
| if (workspaceStats.isDirectory()) { | |
| this.store?.touchConversationBinding(conversationScopeHash, targetKey); | |
| return await this.reusedWorkspaceContext(workspace); | |
| } | |
| } catch { | |
| // The persisted workspace is no longer usable; replace its binding below. | |
| } | |
| this.workspaces.delete(binding.workspaceSessionId); | |
| this.store?.deleteConversationBinding(conversationScopeHash, targetKey); | |
| } | |
| const context = await this.openNewWorkspace(input); | |
| this.store?.setConversationBinding({ | |
| conversationScopeHash, | |
| targetKey, | |
| workspaceSessionId: context.workspace.id, | |
| }); | |
| return context; | |
| } | |
| private async openConversationWorkspace( | |
| input: OpenWorkspaceInput, | |
| conversationScopeHash: string, | |
| targetKey: string, | |
| ): Promise<WorkspaceContext> { | |
| const binding = this.store?.getConversationBinding(conversationScopeHash, targetKey); | |
| if (binding) { | |
| let reusable: Workspace | undefined; | |
| try { | |
| const workspace = this.getWorkspace(binding.workspaceSessionId); | |
| const workspaceStats = await stat(workspace.root); | |
| if (workspaceStats.isDirectory()) { | |
| reusable = workspace; | |
| } | |
| } catch { | |
| // The persisted workspace is no longer usable; replace its binding below. | |
| } | |
| if (reusable) { | |
| this.store?.touchConversationBinding(conversationScopeHash, targetKey); | |
| return await this.reusedWorkspaceContext(reusable); | |
| } | |
| this.workspaces.delete(binding.workspaceSessionId); | |
| this.store?.deleteConversationBinding(conversationScopeHash, targetKey); | |
| } | |
| const context = await this.openNewWorkspace(input); | |
| this.store?.setConversationBinding({ | |
| conversationScopeHash, | |
| targetKey, | |
| workspaceSessionId: context.workspace.id, | |
| }); | |
| return context; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/workspaces.ts` around lines 137 - 166, Limit the try/catch in
openConversationWorkspace to validating the persisted binding: resolving the
workspace, checking workspace.root with stat, and determining whether it is a
directory. Move reusedWorkspaceContext(workspace) outside the try so errors from
context initialization, including agent/profile loading, propagate without
deleting the healthy binding or creating a replacement workspace. Preserve
touchConversationBinding and the existing cleanup behavior only when binding
validation fails.
[GPT-5.6 Thinking] RESPONDING ON BEHALF OF WAISHNAVReworked the workspace contract on head
@coderabbitai full review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/db/migrations.ts (2)
247-255: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the target-key tuple shape that this parser expects.
This helper reads index 1 of a tuple that
conversationCheckoutTargetKeybuilds insrc/workspaces.tsat line 198 as["checkout", projectKey, null]. Nothing connects the two sites. If a later change adds a leading element to that tuple, this parser silently derives wrong project keys and the backfill writes bad rows.This migration must keep parsing the historical shape, so do not import the producer. Add a comment that states the expected shape.
♻️ Proposed comment
+// Historical target keys are JSON tuples of [mode, projectKey, ref]. +// This migration is frozen; keep parsing that shape even if the producer changes. function projectKeyFromConversationTarget(targetKey: string): string | undefined {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/migrations.ts` around lines 247 - 255, Add a concise comment immediately above projectKeyFromConversationTarget documenting that it parses the historical conversation checkout target-key tuple shape ["checkout", projectKey, null] and reads the project key from index 1. Do not import or couple this migration to conversationCheckoutTargetKey.
217-220: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueOrder the backfill select to make the result deterministic.
Several bindings in one conversation can derive the same
projectKey. One example is a checkout target and a worktree target for the same project.insert or ignorekeeps whichever row the scan returns first, and the select has noorder by, so the retainedcreated_atandlast_used_atare not defined.
claimConversationBootstraponly tests row presence, so behavior does not change today. Ordering bycreated_atpreserves the earliest claim and matches the "first open per conversation" semantics.♻️ Proposed deterministic ordering
const bindings = sqlite.prepare(` select conversation_scope_hash, target_key, created_at, last_used_at from workspace_conversation_bindings + order by created_at `).all() as Array<{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/migrations.ts` around lines 217 - 220, Update the bindings query in the migration backfill to order results by created_at ascending before processing them. Keep the existing insert-or-ignore flow unchanged so duplicate project keys retain the earliest claim and its associated last_used_at value.src/db/schema.ts (1)
58-69: 📐 Maintainability & Code Quality | 🔵 TrivialPlan retention for
workspace_conversation_bootstraps.This table has no foreign key and no cascade path, so rows persist after the related workspace sessions disappear. It gains one row for each conversation and project pair and never shrinks.
lastUsedAtis written byclaimConversationBootstrapinsrc/workspace-store.tsbut no code reads it.Row size is small, so this is not urgent. Consider a periodic delete that uses
lastUsedAtolder than a retention window, and add an index onlastUsedAtwhen you add that sweep.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/schema.ts` around lines 58 - 69, Plan retention for the workspaceConversationBootstraps table by adding a periodic cleanup that deletes rows whose lastUsedAt exceeds the chosen retention window, and add an index on lastUsedAt to support the sweep efficiently. Reuse the existing claimConversationBootstrap timestamp updates and integrate the cleanup with the project’s established periodic maintenance mechanism.src/review-checkpoints.test.ts (1)
43-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a concurrency regression test.
This block only exercises sequential restart persistence. Add a test that calls
initializeWorkspaceandreviewChangesconcurrently (e.g., viaPromise.all) for the sameworkspaceIdright after manager creation, before eligibility resolves. This would catch the readiness-race issue flagged insrc/review-checkpoints.ts(Lines 56-59,120-163) if it regresses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/review-checkpoints.test.ts` around lines 43 - 52, Add a concurrency regression test alongside the sequential restart test that invokes initializeWorkspace and reviewChanges concurrently via Promise.all on a newly created manager using the same workspaceId, before eligibility resolution. Assert the concurrent flow completes with the expected persisted review summary and patch content, covering the readiness synchronization in createReviewCheckpointManager and the initializeWorkspace/reviewChanges paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/chatgpt-coding-workflow.md`:
- Around line 36-40: Update the reopening conditions in the workflow
documentation to include an explicit user request to reopen and a model switch
to a different folder or worktree, while retaining the existing unknown
workspaceId condition and new isolated worktree request.
In `@src/review-checkpoints.ts`:
- Around line 56-59: Fix workspace readiness across initializeWorkspaceState and
reviewChanges: defer assigning state.gitRoot until refs are verified or created,
so gitRoot indicates complete initialization and concurrent callers still await
the pending initialization. Update reviewChanges to call initializeWorkspace
whenever state is missing or both gitRoot and diagnostic are undefined, then
refresh state before continuing.
In `@src/ui/workspace-app.tsx`:
- Around line 516-523: Update formatDiagnostic so non-string, non-Error
diagnostics fall back to String(diagnostic) when JSON.stringify returns
undefined, ensuring diagnostics.map(formatDiagnostic).join("; ") never produces
an empty entry.
In `@src/workspaces.ts`:
- Around line 159-177: Limit the try/catch around the persisted binding
validation in the workspace reuse flow: keep getWorkspace and the root stat
directory check inside try, but move reusedWorkspaceContext outside it. Claim
bootstrap only when constructing the reused context after validation succeeds,
so errors from loadLocalAgentProfiles, loadInitialAgentsFiles, or
findAvailableAgentsFiles propagate without deleting the healthy binding or
consuming the claim.
- Around line 206-207: Gate the calls to loadInitialAgentsFiles and
findAvailableAgentsFiles in the workspace-open flow so reused contexts skip this
work when includeBootstrapContext is false, while preserving line 205’s
workspace reuse and ensuring src/server.ts still receives the agent data when
bootstrap context is included. Inspect non-test consumers before changing the
behavior, and update the reused-context assertions in src/workspaces.test.ts to
match the gated results.
---
Nitpick comments:
In `@src/db/migrations.ts`:
- Around line 247-255: Add a concise comment immediately above
projectKeyFromConversationTarget documenting that it parses the historical
conversation checkout target-key tuple shape ["checkout", projectKey, null] and
reads the project key from index 1. Do not import or couple this migration to
conversationCheckoutTargetKey.
- Around line 217-220: Update the bindings query in the migration backfill to
order results by created_at ascending before processing them. Keep the existing
insert-or-ignore flow unchanged so duplicate project keys retain the earliest
claim and its associated last_used_at value.
In `@src/db/schema.ts`:
- Around line 58-69: Plan retention for the workspaceConversationBootstraps
table by adding a periodic cleanup that deletes rows whose lastUsedAt exceeds
the chosen retention window, and add an index on lastUsedAt to support the sweep
efficiently. Reuse the existing claimConversationBootstrap timestamp updates and
integrate the cleanup with the project’s established periodic maintenance
mechanism.
In `@src/review-checkpoints.test.ts`:
- Around line 43-52: Add a concurrency regression test alongside the sequential
restart test that invokes initializeWorkspace and reviewChanges concurrently via
Promise.all on a newly created manager using the same workspaceId, before
eligibility resolution. Assert the concurrent flow completes with the expected
persisted review summary and patch content, covering the readiness
synchronization in createReviewCheckpointManager and the
initializeWorkspace/reviewChanges paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d3b7188-08fa-4480-b1a6-07d41a079898
📒 Files selected for processing (16)
docs/chatgpt-coding-workflow.mdpackage.jsonsrc/db/migrations.tssrc/db/schema.tssrc/oauth-store.test.tssrc/request-meta.test.tssrc/request-meta.tssrc/review-checkpoints.test.tssrc/review-checkpoints.tssrc/server.tssrc/ui/card-types.test.tssrc/ui/card-types.tssrc/ui/workspace-app.tsxsrc/workspace-store.tssrc/workspaces.test.tssrc/workspaces.ts
| Do not reopen the same checkout folder unless: | ||
|
|
||
| - the `workspaceId` is rejected as unknown | ||
| - the user switches to another folder | ||
| - the user switches between checkout and worktree mode | ||
| - the user explicitly asks to reopen | ||
| - the user asks for a new isolated worktree |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the reopening conditions with open_workspace.
src/server.ts allows reopening when the user asks to reopen and when the model switches to a different folder or worktree. This document omits both cases. Restore them so the workflow does not reject valid user requests.
Proposed fix
- the `workspaceId` is rejected as unknown
-- the user switches to another folder
+- the user switches to another folder or worktree
+- the user asks to reopen the checkout
- the user asks for a new isolated worktree📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Do not reopen the same checkout folder unless: | |
| - the `workspaceId` is rejected as unknown | |
| - the user switches to another folder | |
| - the user switches between checkout and worktree mode | |
| - the user explicitly asks to reopen | |
| - the user asks for a new isolated worktree | |
| Do not reopen the same checkout folder unless: | |
| - the `workspaceId` is rejected as unknown | |
| - the user switches to another folder or worktree | |
| - the user asks to reopen the checkout | |
| - the user asks for a new isolated worktree |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/chatgpt-coding-workflow.md` around lines 36 - 40, Update the reopening
conditions in the workflow documentation to include an explicit user request to
reopen and a model switch to a different folder or worktree, while retaining the
existing unknown workspaceId condition and new isolated worktree request.
| if ( | ||
| existingState?.root === root && | ||
| (existingState.gitRoot !== undefined || existingState.diagnostic !== undefined) | ||
| ) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fix the readiness race between initializeWorkspaceState and its consumers.
initializeWorkspaceState publishes state into states synchronously (line 127) before eligibility resolves, then sets state.gitRoot = eligibility.gitRoot (line 136) before the open/baseline refs are verified or created (lines 137-149). This creates two related problems:
initializeWorkspace's own early-return check at Lines 56-59 treatsgitRoot !== undefinedas "fully initialized." A second concurrentinitializeWorkspacecall for the sameworkspaceIdcan seegitRootset and return immediately, bypassing theinitializationspending-map wait, even though ref creation is still in flight.reviewChanges(unchanged in this diff) only checksif (!state)before deciding whether to callinitializeWorkspace. Sincestateis set synchronously and early, a concurrentreviewChangescall sees a non-null but incomplete state, skipsinitializeWorkspaceentirely, and then either throws a generic "requires a Git workspace" error (ifgitRootisn't set yet) or runsgit rev-parse --verify <ref>^{commit}against a ref that doesn't exist yet (ifgitRootis set but refs aren't created).
This is the same issue raised in a previous review round (marked "Addressed"), but the current code still exhibits it. Set state.gitRoot only after refs are confirmed or created, and make reviewChanges re-enter initializeWorkspace whenever gitRoot/diagnostic are both absent, mirroring the readiness condition already used at Lines 56-59.
🔧 Proposed fix
state.gitRoot = eligibility.gitRoot;
const [hasOpenRef, hasBaselineRef] = await Promise.all([
hasCommitRef(eligibility.gitRoot, state.openRef),
hasCommitRef(eligibility.gitRoot, state.baselineRef),
]);
- if (hasOpenRef && hasBaselineRef) return;
-
- const commit = await createWorkingTreeSnapshot(eligibility.gitRoot);
- if (!hasOpenRef) {
- await git(eligibility.gitRoot, ["update-ref", state.openRef, commit]);
- }
- if (!hasBaselineRef) {
- await git(eligibility.gitRoot, ["update-ref", state.baselineRef, commit]);
- }
+ if (!hasOpenRef || !hasBaselineRef) {
+ const commit = await createWorkingTreeSnapshot(eligibility.gitRoot);
+ if (!hasOpenRef) {
+ await git(eligibility.gitRoot, ["update-ref", state.openRef, commit]);
+ }
+ if (!hasBaselineRef) {
+ await git(eligibility.gitRoot, ["update-ref", state.baselineRef, commit]);
+ }
+ }
+ state.gitRoot = eligibility.gitRoot;Also update the unchanged reviewChanges readiness check outside this range:
async reviewChanges({ workspaceId, root, since = "last_shown", markReviewed = true }) {
let state = states.get(workspaceId);
if (!state || (state.gitRoot === undefined && state.diagnostic === undefined)) {
await this.initializeWorkspace({ workspaceId, root });
state = states.get(workspaceId);
}
if (!state?.gitRoot) {
throw new Error(state?.diagnostic ?? "show_changes requires a Git workspace in this version.");
}
// ...Also applies to: 120-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/review-checkpoints.ts` around lines 56 - 59, Fix workspace readiness
across initializeWorkspaceState and reviewChanges: defer assigning state.gitRoot
until refs are verified or created, so gitRoot indicates complete initialization
and concurrent callers still await the pending initialization. Update
reviewChanges to call initializeWorkspace whenever state is missing or both
gitRoot and diagnostic are undefined, then refresh state before continuing.
| function formatDiagnostic(diagnostic: unknown): string { | ||
| if (typeof diagnostic === "string") return diagnostic; | ||
| if (diagnostic instanceof Error) return diagnostic.message; | ||
| try { | ||
| return JSON.stringify(diagnostic); | ||
| } catch { | ||
| return String(diagnostic); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect diagnostic producers and confirm whether undefined values can enter
# the workspace-card payload.
rg -n -C 4 'skillDiagnostics|formatDiagnostic' srcRepository: Waishnav/devspace
Length of output: 7013
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate all diagnostics definitions/usages and inspect the relevant workspace-app section.
rg -n -C 3 'diagnostics|Diagnostics|diagnostic' src/ui workspace src | head -n 200
printf '\n--- workspace-app relevant section ---\n'
sed -n '445,525p' src/ui/workspace-app.tsx
printf '\n--- runtime behavior probe ---\n'
node - <<'JS'
console.log(JSON.stringify({
value: undefined,
stringifyUndefined: JSON.stringify(undefined),
stringifyUndefinedNullified: JSON.stringify(undefined) ?? String(undefined),
stringOfUndefined: String(undefined)
}))
JSRepository: Waishnav/devspace
Length of output: 11182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect LoadedSkills diagnostics type/shape and diagnostic producers.
sed -n '1,130p' src/skills.ts
printf '\n--- diagnostic assignment references ---\n'
rg -n -C 3 'diagnostics\s*:|type\s*: .*diagnostic|collision|LoadSkillsResult' src
printf '\n--- runtime behavior probe ---\n'
node - <<'JS'
const cases = [undefined, {a: undefined}, null];
for (const value of cases) {
const serialized = JSON.stringify(value);
console.log({
input: value,
inputType: typeof value,
stringified: JSON.stringify(value),
stringifiedNullishCoalesced: serialized ?? String(value),
resultOfFormat: (serialized) ? serialized : String(value),
});
}
JSRepository: Waishnav/devspace
Length of output: 7046
Handle object diagnostics consistently.
formatDiagnostic already formats strings and Error objects, but other diagnostic values are stringified directly. When JSON.stringify returns undefined, diagnostics.map(formatDiagnostic).join("; ") inserts an empty entry in the output. Add a string fallback for non-string/non-warning formats.
Proposed fix
try {
- return JSON.stringify(diagnostic);
+ const serialized = JSON.stringify(diagnostic);
+ return serialized ?? String(diagnostic);
} catch {
return String(diagnostic);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function formatDiagnostic(diagnostic: unknown): string { | |
| if (typeof diagnostic === "string") return diagnostic; | |
| if (diagnostic instanceof Error) return diagnostic.message; | |
| try { | |
| return JSON.stringify(diagnostic); | |
| } catch { | |
| return String(diagnostic); | |
| } | |
| function formatDiagnostic(diagnostic: unknown): string { | |
| if (typeof diagnostic === "string") return diagnostic; | |
| if (diagnostic instanceof Error) return diagnostic.message; | |
| try { | |
| const serialized = JSON.stringify(diagnostic); | |
| return serialized ?? String(diagnostic); | |
| } catch { | |
| return String(diagnostic); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/workspace-app.tsx` around lines 516 - 523, Update formatDiagnostic so
non-string, non-Error diagnostics fall back to String(diagnostic) when
JSON.stringify returns undefined, ensuring
diagnostics.map(formatDiagnostic).join("; ") never produces an empty entry.
| const binding = this.store?.getConversationBinding(conversationScopeHash, targetKey); | ||
| if (binding) { | ||
| try { | ||
| const workspace = this.getWorkspace(binding.workspaceSessionId); | ||
| const workspaceStats = await stat(workspace.root); | ||
| if (workspaceStats.isDirectory()) { | ||
| this.store?.touchConversationBinding(conversationScopeHash, targetKey); | ||
| return await this.reusedWorkspaceContext( | ||
| workspace, | ||
| this.store?.claimConversationBootstrap(conversationScopeHash, projectKey) ?? true, | ||
| ); | ||
| } | ||
| } catch { | ||
| // The persisted workspace is no longer usable; replace its binding below. | ||
| } | ||
|
|
||
| this.workspaces.delete(binding.workspaceSessionId); | ||
| this.store?.deleteConversationBinding(conversationScopeHash, targetKey); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Limit the recovery path to binding validation.
The try block still wraps reusedWorkspaceContext at line 166. That call runs loadLocalAgentProfiles, loadInitialAgentsFiles, and findAvailableAgentsFiles. If any of them throws for a reason unrelated to the binding, the catch at line 171 swallows the error. Execution then reaches lines 175-176, deletes a healthy binding, and creates a second workspace for the same target at line 179.
The bootstrap claim makes the outcome worse. Line 168 calls claimConversationBootstrap before the throw, so the claim is already consumed. The replacement path at line 188 claims again and receives false. The caller then gets a new workspace id with no bootstrap context.
Validate the binding inside the try. Build the reused context outside it, after the claim.
🛡️ Proposed narrower recovery scope
const binding = this.store?.getConversationBinding(conversationScopeHash, targetKey);
if (binding) {
+ let reusable: Workspace | undefined;
try {
const workspace = this.getWorkspace(binding.workspaceSessionId);
const workspaceStats = await stat(workspace.root);
if (workspaceStats.isDirectory()) {
- this.store?.touchConversationBinding(conversationScopeHash, targetKey);
- return await this.reusedWorkspaceContext(
- workspace,
- this.store?.claimConversationBootstrap(conversationScopeHash, projectKey) ?? true,
- );
+ reusable = workspace;
}
} catch {
// The persisted workspace is no longer usable; replace its binding below.
}
+ if (reusable) {
+ this.store?.touchConversationBinding(conversationScopeHash, targetKey);
+ return await this.reusedWorkspaceContext(
+ reusable,
+ this.store?.claimConversationBootstrap(conversationScopeHash, projectKey) ?? true,
+ );
+ }
+
this.workspaces.delete(binding.workspaceSessionId);
this.store?.deleteConversationBinding(conversationScopeHash, targetKey);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/workspaces.ts` around lines 159 - 177, Limit the try/catch around the
persisted binding validation in the workspace reuse flow: keep getWorkspace and
the root stat directory check inside try, but move reusedWorkspaceContext
outside it. Claim bootstrap only when constructing the reused context after
validation succeeds, so errors from loadLocalAgentProfiles,
loadInitialAgentsFiles, or findAvailableAgentsFiles propagate without deleting
the healthy binding or consuming the claim.
| const agentsFiles = await this.loadInitialAgentsFiles(workspace.root); | ||
| const availableAgentsFiles = await this.findAvailableAgentsFiles(workspace.root, agentsFiles); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Reused opens still walk the whole workspace tree, and the caller may discard the result.
findAvailableAgentsFiles at line 207 walks the entire workspace root and calls tryRealpath for each candidate file. src/server.ts sets availableAgentsFileOutputs to [] when includeBootstrapContext is false, so on repeat opens that walk runs and the output is dropped. On a large repository this is a full tree walk on a request path for no delivered value.
Keep line 205. The reused workspace can be a restored session with agentProfiles: [] from line 248, and src/server.ts builds cardAgents from that field.
src/workspaces.test.ts lines 288-292 currently assert that reboundWorkspace.agentsFiles and availableAgentsFiles match the first open, so gating this work on includeBootstrapContext needs those assertions updated too. Confirm whether any consumer outside src/server.ts reads these fields on a reused context before deciding.
#!/bin/bash
# Description: Find every non-test consumer of agentsFiles and availableAgentsFiles on a WorkspaceContext.
set -euo pipefail
rg -nP --type=ts --type=tsx -C6 '\b(agentsFiles|availableAgentsFiles)\b' src -g '!**/*.test.ts'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/workspaces.ts` around lines 206 - 207, Gate the calls to
loadInitialAgentsFiles and findAvailableAgentsFiles in the workspace-open flow
so reused contexts skip this work when includeBootstrapContext is false, while
preserving line 205’s workspace reuse and ensuring src/server.ts still receives
the agent data when bootstrap context is included. Inspect non-test consumers
before changing the behavior, and update the reused-context assertions in
src/workspaces.test.ts to match the gated results.
1 participant
ChatGPT can issue
open_workspacemore than once for the same project during one conversation. Returning the full bootstrap every time repeats project instructions, nested instruction paths, skills, subagent metadata, and diagnostics in the model context.DevSpace now treats workspace lifecycle and bootstrap delivery as separate concerns:
show_changes.The implementation canonicalizes project paths, makes concurrent project-bootstrap claims atomic, validates restored checkout roots, and keeps worktree calls independent. Tests cover checkout reuse, fresh sequential and concurrent worktrees, both cross-mode directions, aliases, stale roots, persistence, checkpoint restoration, migration backfill, and UI display behavior.
Summary by CodeRabbit
New Features
Bug Fixes