fix(workspace): keep per_run workspaces run-scoped (BLO-19063) - #1231
fix(workspace): keep per_run workspaces run-scoped (BLO-19063)#1231kkroo wants to merge 1 commit into
Conversation
Prevent reuse and reuse_existing pinning whenever the effective workspace strategy is per_run, including policy and issue-level overlays. Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
🔗 Paperclip issue: BLO-19063 |
|
@ally Please review this exact independent-successor head: This Local evidence: 226 heartbeat/model-profile tests, 29 policy/isolation/authz tests, and server typecheck passed. Auto-merge is intentionally off pending current-head App and User attestations plus CI. |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 5c41efd
Critical Issues (0)
Important Issues (1)
- [gstack/review]
server/src/services/heartbeat.ts:21100— Per-run workspaces leak asactiveafter every sequential run. With an issue already pointing at an active workspace, this new guard makesreusableExistingExecutionWorkspacenull. The run then creates and points the issue at a new workspace, but the only old-workspace transition atserver/src/services/heartbeat.ts:21759is guarded by that same now-null variable. The old worktree remains active indefinitely, and its desired runtime services are restarted on process startup (workspace-runtime.ts:6116).- Track ownership by run and transition/clean up completed per-run workspaces without retiring one that a concurrent run still owns; add an integration test covering two sequential runs and the old workspace's terminal status.
Suggestions (0)
Strengths
- The policy and issue-level override resolution is deliberately centralized, preventing the reuse predicate from drifting from the realized configuration.
- The regression coverage exercises policy precedence and the previously missed adapter override path.
Recommended Action
- Address the Important workspace lifecycle leak before merge.
|
Closing as stale — this draft can no longer be landed as a diff. Measured against master
At that distance the surrounding code has moved out from under the patch, so "resolve the conflicts" would mean rewriting it against a codebase it was never written for — not merging it. Reopening is cheap if the branch is still wanted; the commits are untouched. Closing the PR does not close the underlying issue. Where this PR referenced a BLO issue, that issue remains the record of intent and the place to re-implement from, against current master. No judgement here on whether the original problem still exists — I did not verify that per-PR, and it should be re-checked before any re-implementation, since some of these may already have been fixed independently. |
Thinking Path
Linked Issues or Issue Description
Refs BLO-19063. Supersedes the App-authored #1154; the original remains open and unchanged.
A GitHub duplicate search for
per_run workspacefound #1154 and its merged prerequisite #1143 as the only overlap.ROADMAP.mdhas no conflicting planned core work.What Changed
reuse_existingworkspace whenever the effective strategy resolves torunScope: "per_run".reuse_existingpin for a per-run workspace, including legacy pins created before the setting changed.Verification
The focused suites emit an expected optional-local-adapter warning for an absent
taalas-chatjimmy-wrapper; all assertions passed.Risks
Low and intentional.
per_issueand shared-workspace restore behavior remain unchanged. Explicitper_runnow always realizes a fresh workspace, trading reuse for the isolation it advertises. Model profiles no longer control workspace placement. No schema, migration, or API change is included.Model Used
OpenAI Codex, GPT-5. Tool-assisted GitHub review-context inspection, source reconstruction, isolated-worktree editing, and local test/typecheck execution; context window is platform-managed.
Checklist