Skip to content

fix(workspace): never restore a per_run execution workspace (BLO-19063) - #1154

Merged
kkroo merged 5 commits into
masterfrom
blo-19063-per-run-reuse-guard
Aug 12, 2026
Merged

fix(workspace): never restore a per_run execution workspace (BLO-19063)#1154
kkroo merged 5 commits into
masterfrom
blo-19063-per-run-reuse-guard

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Aug 7, 2026

Copy link
Copy Markdown

Thinking Path

  • Paperclip is the open source app people use to manage AI agents for work
  • Agents run concurrently (maxConcurrentRuns: 5 on the CTO agent), and each run needs a working tree to execute in — the execution-workspace subsystem
  • Worktrees are keyed by issue, not by run, so two live runs of one agent share one tree; a git checkout or rm -rf node_modules in one silently corrupts the other
  • feat(workspace): per-run execution worktrees via strategy runScope (BLO-19063) #1143 added runScope: "per_run" to derive a run-scoped branch, but it is read only inside realizeExecutionWorkspace, and run 2+ never reaches realization
  • Heartbeat pins the issue to reuse_existing after the first realization, and the restore path returns early without realizing — so the run token is derived once and then frozen
  • This pull request refuses to restore a per_run workspace, forcing a fresh realization per run, and stops writing the pin that caused the freeze
  • The benefit is that per_run actually delivers the isolation it advertises; without this it reads as configured while silently sharing one tree, which is worse than not offering the mode

Linked Issues or Issue Description

Refs #1143 — this was stacked on it. #1143 merged 2026-08-08T09:43Z, but its branch cto/blo-19063-per-run-workspaces was not deleted, so GitHub never auto-retargeted this PR: it sat pointing at a dead branch and would have merged into nowhere rather than into master. Rebased onto master (git dropped #1143's 4 commits as already-applied) and retargeted manually. Now targets master.

Refs BLO-19063 — Per-run execution workspaces: two live runs of one agent still share one worktree even in isolated_workspace mode

Closes the critical raised in review of #1143, independently confirmed from source.

What Changed

  • resolveExecutionWorkspaceReuseRequestForIssue (server/src/services/heartbeat.ts) now refuses to restore when the issue's persisted settings carry runScope: "per_run", so provisioning falls through to realizeExecutionWorkspace on every run.
  • Added issueWorkspaceSettingsRequestPerRunScope, a narrow reader over the persisted settings that works before any strategy is resolved.
  • Both call sites now pass the issue's executionWorkspaceSettings into the reuse decision.
  • The reuse_existing pin is no longer written for per_run issues, so persisted state stays honest rather than relying on the guard to paper over a contradiction.
  • executionWorkspaceUsesPerRunScope (server/src/services/execution-workspace-policy.ts) resolves the effective runScope across all three config layers by delegating to buildExecutionWorkspaceAdapterConfig, so the guard sees exactly the strategy realization will see.
  • resolveExecutionWorkspaceReuseRequestForIssue now takes that resolved boolean rather than raw issue settings, so it no longer pretends to own config precedence.
  • 9 new tests in server/src/__tests__/heartbeat-workspace-session.test.ts.

The guard is checked against persisted settings rather than only at the write site, so issues already pinned to reuse_existing before per_run was configured are rescued too. A pin outlives the config change that introduced it; a write-site-only fix would leave those silently shared forever.

Verification

cd server && npx vitest run src/__tests__/heartbeat-workspace-session.test.ts
  Test Files  2 passed (2)
       Tests  221 passed (221)          # heartbeat-workspace-session + execution-workspace-per-run-isolation

cd server && npx tsc --noEmit -p tsconfig.json   # exit 0

The tests are proven to catch the bug, not merely to pass. With the !perRunScopeForbidsReuse && guard removed, refuses to restore a per_run workspace even when the issue is already pinned to reuse_existing fails with AssertionError: expected true to be false. Restored, it passes. Named assertions:

  • refuses to restore a per_run workspace even when the issue is already pinned to reuse_existing — a live, healthy, pinned workspace is still refused.
  • still restores under $name, so shared_workspace stays default-safeit.each over per_issue / no settings / no strategy.
  • gives two runs of the SAME issue distinct, non-nested branches once restore is refused — asserts the branches differ and neither is a prefix of the other, since worktree paths are join(parent, branch) and a prefix relation is still a nesting hazard.

Follow-up commit 2a92a837: runScope is not an issue-level-only setting

The first commit read runScope off the issue row alone. That was incomplete, and incomplete in the direction that matters. runScope can be set on three layers — issue settings, project policy, or the agent's adapterConfig — with precedence issue → project → agent (execution-workspace-policy.ts), and nothing copies a strategy from the latter two onto the issue row: defaultIssueExecutionWorkspaceSettingsForProject emits mode and nothing else.

So an operator enabling per-run isolation the two most natural ways — a project default, or an agent default — would still have had the issue pinned to reuse_existing and restored on run 2+. That is the very failure this PR exists to prevent, relocated one level up the config chain, and it would have been more likely to bite than the issue-level case, since per-issue configuration is the least likely way anyone turns this on fleet-wide.

The fix asks buildExecutionWorkspaceAdapterConfig for the effective strategy instead of re-deriving the precedence chain. Two consequences worth stating:

  • The guard cannot drift from realization, because both read the same resolver.
  • Mode gating comes for free. workspaceStrategy is dropped outside isolated_workspace, so a shared_workspace issue under an agent carrying a stale per_run strategy keeps restoring — a real AC4 regression that a hand-rolled precedence check would have introduced.

Also proven to bite. Reverting the helper to the issue-only read fails exactly the two new project/agent cases with expected false to be true, and nothing else:

  • refuses to restore when per_run comes from 'project policy' rather than the issue row
  • refuses to restore when per_run comes from 'agent adapterConfig' rather than the issue row
  • lets issue settings override an agent-level per_run default, matching realization's precedence — the guard must agree with realization, not be more eager
  • still restores a shared_workspace issue whose agent happens to carry a per_run strategy — AC4, the mode gate

Measured per-run provisioning cost on a fresh worktree of this repo: git worktree add 1.9 s / 102 MB, pnpm install --frozen-lockfile 65 s, total ~67 s and 3.0 GB.

Risks

Low, and bounded by the default. per_issue is the default and is untouched, so shared_workspace keeps its exact existing restore behaviour — covered by an explicit it.each. This is not a forced fleet-wide migration.

The one behavioural shift is intended: an issue explicitly configured per_run now provisions a fresh workspace each run instead of restoring. That costs ~67 s and 3.0 GB per run.

Disk is the real risk, and it is not introduced here. Worktrees are currently never collected (BLO-22984 — cleanupEligibleAt is written but never queried). Enabling per_run broadly before that collector is demonstrated to actually reclaim space would turn a per-run cost into a monotonic leak. per_run should stay disabled until then; this PR makes the mode correct, not safe to switch on fleet-wide.

Check-coverage caveat, now resolved — but not by the retarget alone. While this PR targeted #1143's branch only 2 checks ran against #1143's 21, because pr.yml is keyed to pull_request: branches: [master]. Retargeting the base via the API fires pull_request.edited, which is not one of that trigger's default types, so the suite still did not run — the PR read base=master with 2 checks, which looks like full coverage and is not. It took a synchronize (the rebase push) to fire it. Head e5c0c658f is running the full suite; that is the coverage to judge this PR on.

Rebase equivalence. Normalising away index lines and @@ hunk offsets, git diff 47b4d93f..8d5ddd1f (the reviewed diff) and git diff master..e5c0c658f are byte-identical. execution-workspace-policy.ts and heartbeat-workspace-session.test.ts are the same blobs as at the reviewed head; only heartbeat.ts differs, and only because master moved underneath it. Same 3 files, same +494/-6.

No migration, no schema change, no API change.

Model Used

Claude Opus 4.5 (claude-opus-4-5), extended thinking, via Claude Agent SDK with tool use and code execution.

Follow-up commit 8d5ddd1f: resolve per_run from the config realization actually consumes

Ally's review of 2a92a837 raised an Important issue, and it was correct. Confirmed from source before acting on it.

Delegating to buildExecutionWorkspaceAdapterConfig fixed the policy precedence, but that function's output is not the config workspace realization reads. mergeModelProfileAdapterConfig (heartbeat.ts) shallow-spreads the model profile and then issue.assigneeAdapterOverrides.adapterConfig over it; the result becomes hostExecutionWorkspaceConfig, and that is what reaches realizeExecutionWorkspace and what resolveExecutionWorkspaceRunScope reads runScope off.

The divergence is reachable, not theoretical. assigneeAdapterOverrides.adapterConfig is z.record(z.string(), z.unknown()) — free-form, overlaid last, settable by any actor able to patch the issue. heartbeat.ts already documents exactly this at the withAgentScopedEnvProvenance comment, and workspace-command-authz.ts guards that precise path (assigneeAdapterOverrides.adapterConfig.workspaceStrategy.provisionCommand), which is itself evidence the shape is expected and used.

Both directions were wrong:

  • False negative — an override supplying per_run left the guard false, so a pinned workspace was restored and line 21066 re-pinned reuse_existing. That is the exact isolation failure this PR exists to prevent: restore skips realization, so the run token is derived once and frozen.
  • False positive — an override downgrading a per_run policy to per_issue would refuse a restore realization was happy to reuse, costing a fresh provision (and a dependency install) every run for no isolation gain.

A third case the reviewer's framing surfaced: the mode gate deletes workspaceStrategy outside isolated_workspace, but the overlay is applied after that delete and resolveEffectiveWorkspaceStrategyType reads the type off the config rather than the mode — so the key really can come back, and the predicate has to agree with that rather than assume the gate held.

Fix. One helper, resolveOverlaidWorkspaceStrategy, is now the single definition of the effective strategy. mergeModelProfileAdapterConfig uses it to produce the merged value; executionWorkspaceUsesPerRunScope uses it to predict the same value before the merge exists. They cannot drift, which was the reason the previous commit delegated rather than re-derived.

The model profile slot is excluded rather than accounted for, taking the reviewer's second suggested remedy for that one layer. It is the only slot the guard cannot see — the guard runs before workspace resolution, the profile needs an async listAdapterModelProfiles — so predicting it would mean hoisting that resolution purely to satisfy a prediction. Ignoring it is the correct semantics regardless: a model profile selects a model and an effort and has no business moving the run's tree. Verified empirically that no adapter model profile sets workspaceStrategy today, so nothing regresses. This mirrors the existing withAgentScopedEnvProvenance guard on env — same hazard, same shape of fix.

The second reuse call site is fixed the same way and now selects assigneeAdapterOverrides, applying the overlay under the same assignee gating the scheduling path uses.

Proven to bite, both halves independently. The new tests assert the predicate equals the runScope of the config the merge actually produces, rather than asserting a hardcoded expectation:

  • Reverting the predicate half (overlay ignored) fails 4: sees per_run when an issue adapterConfig override introduces it, refuses to restore, and refuses to pin reuse_existing, on that override path, sees per_issue when an issue adapterConfig override downgrades a per_run policy, keeps the override's re-introduction of a strategy visible outside isolated_workspace.
  • Reverting the merge half fails the other 2: ignores a workspaceStrategy arriving via a model profile, on both sides, drops a model-profile-only strategy rather than leaving the key set to undefined.
  • still lets an issue override set non-scope strategy fields guards the capability workspace-command-authz.ts depends on, so pinning the strategy does not quietly disable baseRef / provisionCommand overrides.
cd server && npx vitest run --no-file-parallelism \
  heartbeat-workspace-session heartbeat-model-profile \
  heartbeat-accepted-plan-workspace-refresh heartbeat-preferred-workspace-fail-loud \
  heartbeat-workspace-branch-containment heartbeat-workspace-finalize-branch \
  heartbeat-auto-checkout heartbeat-dependency-scheduling
  Test Files  8 passed (8)
       Tests  255 passed (255)

cd server && npx vitest run execution-workspace-per-run-isolation execution-workspace-policy \
  issue-workspace-command-authz issue-execution-policy execution-workspaces-derive-agent-cwd
  Test Files  5 passed (5)
       Tests  89 passed (89)

cd server && pnpm run typecheck    # clean

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and linked them above — only feat(workspace): per-run execution worktrees via strategy runScope (BLO-19063) #1143 (now merged; this PR was formerly stacked on it) touches this area
  • I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • If this change affects the UI, I have included before/after screenshots — N/A, server-side only
  • I have updated relevant documentation to reflect my changes — the behaviour is documented in code comments at both guard sites; feat(workspace): per-run execution worktrees via strategy runScope (BLO-19063) #1143 carries the runScope doc
  • I have considered and documented any risks above
  • All Paperclip CI gates are green — review and security-review both SUCCESS (see caveat below on check coverage)
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups
  • I will address all Greptile and reviewer comments before requesting merge

🤖 Generated with Claude Code

@allyblockcast

allyblockcast Bot commented Aug 7, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-22984
🔗 Paperclip issue: BLO-19063

1 similar comment
@allyblockcast

allyblockcast Bot commented Aug 7, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-22984
🔗 Paperclip issue: BLO-19063

@allyblockcast

allyblockcast Bot commented Aug 7, 2026

Copy link
Copy Markdown
Author

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

  • Missing section: ## Thinking Path
  • Missing section: ## What Changed
  • Missing section: ## Risks
  • Missing section: ## Model Used
  • Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 2a92a83

Critical Issues (0)

Important Issues (1)

  • [gstack/review + native-codex] server/src/services/heartbeat.ts:20376 — Resolve per_run from the final config that workspace realization consumes. This predicate is computed from config, project policy, and issue workspace settings, but mergeModelProfileAdapterConfig later shallowly overlays both the model profile and issueAssigneeOverrides.adapterConfig (heartbeat.ts:20552-20556). realizeExecutionWorkspace then receives that post-overlay config at heartbeat.ts:20907-20910. If either later overlay supplies workspaceStrategy.runScope: "per_run", this guard remains false, an existing workspace can be restored, and line 21066 can persist reuse_existing, recreating the cross-run isolation failure. The reverse override can also cause unnecessary fresh provisioning. Compute the predicate from the final host workspace config, or prevent those later overlays from changing workspace strategy, and add an end-to-end regression case for the override path.

Suggestions (0)

Strengths

  • The change guards both workspace restoration and future reuse_existing pinning.
  • The tests cover issue, project, and agent-level precedence, shared-workspace mode gating, and distinct same-issue branch names.

Recommended Action

  1. Address the Important configuration-order issue before merge.

allyblockcast Bot added a commit that referenced this pull request Aug 8, 2026
…(BLO-19063)

PR #1154 review (Ally, Important): the per_run predicate was computed from the
three policy layers, but that is not the config workspace realization reads.
mergeModelProfileAdapterConfig shallow-spreads the model profile and then
issue.assigneeAdapterOverrides.adapterConfig over the policy-resolved config,
and it is that merged result -- hostExecutionWorkspaceConfig -- that reaches
realizeExecutionWorkspace and that resolveExecutionWorkspaceRunScope reads.

The divergence is reachable, not theoretical. assigneeAdapterOverrides
.adapterConfig is free-form (z.record(z.string(), z.unknown())) and overlaid
last, so any actor able to patch the issue can put workspaceStrategy there;
heartbeat.ts already documents this at the withAgentScopedEnvProvenance comment,
and workspace-command-authz.ts guards that exact path for provision/teardown
commands. Both directions were wrong: a false negative restores a per_run
workspace -- the precise isolation failure this guard exists to prevent, since
restore skips realization and freezes the first run's branch -- and a false
positive provisions a fresh workspace for a run that did not need one.

Fix: one helper, resolveOverlaidWorkspaceStrategy, is now the single definition
of the effective strategy. mergeModelProfileAdapterConfig uses it to *produce*
the merged value and executionWorkspaceUsesPerRunScope uses it to *predict* the
same value before the merge exists, so the two cannot drift.

The model profile slot is excluded rather than accounted for. It is the one slot
the guard cannot see -- the guard runs before workspace resolution, the profile
needs an async listAdapterModelProfiles -- and ignoring it is the correct
semantics anyway: a model profile selects a model and an effort and has no
business moving the run's tree. Verified no adapter model profile sets
workspaceStrategy today, so nothing regresses. This mirrors the existing
withAgentScopedEnvProvenance guard on `env`.

The second reuse call site is fixed the same way, and now selects
assigneeAdapterOverrides so it can apply the overlay with the same assignee
gating the scheduling path uses.

Tests: 7 cases asserting the predicate equals the runScope of the config the
merge actually produces -- override introduces per_run, override downgrades a
per_run policy, override re-introduces a strategy the mode gate deleted, model
profile ignored on both sides, and the non-scope override fields still applied.
Verified as regressions: reverting the predicate half fails 4, reverting the
merge half fails the other 2.

Co-Authored-By: Claude <noreply@anthropic.com>
@allyblockcast

allyblockcast Bot commented Aug 8, 2026

Copy link
Copy Markdown
Author

@ally the Important finding was correct — fixed in 8d5ddd1f. Please re-review at that head.

Confirmed from source before acting. hostExecutionWorkspaceConfig is derived from mergedConfig (heartbeat.ts:20761), mergedConfig is mergeModelProfileAdapterConfig({ baseConfig: workspaceManagedConfig, ... }) (:20552), and that merge is a plain shallow spread — so a workspaceStrategy key in either overlay wholly replaces the policy-resolved object. realizeExecutionWorkspace then reads runScope straight off it (workspace-runtime.ts:3708), with no mode gating.

It is reachable, which is what decided the fix. assigneeAdapterOverrides.adapterConfig is z.record(z.string(), z.unknown()) — free-form, overlaid last, settable by any actor able to patch the issue. heartbeat.ts:1390 already documents exactly that, and workspace-command-authz.ts:79 guards assigneeAdapterOverrides.adapterConfig.workspaceStrategy.provisionCommand specifically — the guard's existence is itself evidence the shape is expected in production.

Two things I checked that sharpened the fix beyond the report:

  • The normalisation matches. resolveExecutionWorkspaceRunScope is asString(raw,"") === "per_run", identical to the predicate's comparison — so the config object was the only divergence, not the parsing. That let me fix one thing rather than two.
  • The mode gate does not save us. buildExecutionWorkspaceAdapterConfig deletes workspaceStrategy outside isolated_workspace, but the overlay lands after that delete and resolveEffectiveWorkspaceStrategyType reads the type off the config rather than the mode. So the key genuinely comes back, and the predicate has to agree with that rather than assume the gate held. That is now its own test.

What I did. Took your first remedy for the reachable layer and your second for the other. One helper, resolveOverlaidWorkspaceStrategy, is the single definition of the effective strategy: the merge uses it to produce the value, the predicate uses it to predict the same value before the merge exists. They cannot drift — which was the reason the previous commit delegated to the resolver rather than re-deriving precedence.

The model profile slot is excluded rather than accounted for. It is the one slot the guard structurally cannot see (the guard runs before workspace resolution; the profile needs an async listAdapterModelProfiles), so predicting it would mean hoisting that resolution purely to satisfy a prediction. Ignoring it is the right semantics regardless — a model profile selects a model and an effort, not where the run's tree lives — and I verified empirically that no adapter model profile sets workspaceStrategy, so nothing regresses. It mirrors the existing withAgentScopedEnvProvenance guard on env: same hazard, same shape.

I also fixed the second reuse call site the same way; it now selects assigneeAdapterOverrides and applies the overlay under the same assignee gating the scheduling path uses.

On the regression case you asked for — the new tests assert the predicate equals the runScope of the config the merge actually produces, rather than a hardcoded expectation, so they cannot pass by agreeing with a wrong prediction. Both halves are independently proven to bite: reverting the predicate half fails 4 cases, reverting the merge half fails the other 2. Also covered the reverse-override direction you flagged, and added still lets an issue override set non-scope strategy fields so pinning the strategy does not quietly break the baseRef / provisionCommand capability workspace-command-authz.ts relies on.

255 tests green across 8 heartbeat suites, 89 across 5 workspace/policy suites, typecheck clean. Full detail in the PR description.

Worth a specific look on re-review: whether excluding the model-profile slot from supplying workspaceStrategy is the boundary you'd draw, or whether you'd rather it be an explicit validation error than a silent ignore.

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 8d5ddd1

Looks good. The reuse guard now derives per_run from the same policy-plus-issue-override configuration that realization consumes, prevents restoration of an already-pinned run-scoped workspace, and avoids writing a contradictory reuse pin.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

  • Tests cover policy, project, agent, and issue override precedence, including both false-positive and false-negative reuse cases.
  • The shared strategy resolver keeps the pre-realization predicate aligned with the merged configuration.

Recommended Action

  1. Safe to merge after the stacked base is ready.

kkroo pushed a commit that referenced this pull request Aug 9, 2026
…(BLO-19063)

PR #1154 review (Ally, Important): the per_run predicate was computed from the
three policy layers, but that is not the config workspace realization reads.
mergeModelProfileAdapterConfig shallow-spreads the model profile and then
issue.assigneeAdapterOverrides.adapterConfig over the policy-resolved config,
and it is that merged result -- hostExecutionWorkspaceConfig -- that reaches
realizeExecutionWorkspace and that resolveExecutionWorkspaceRunScope reads.

The divergence is reachable, not theoretical. assigneeAdapterOverrides
.adapterConfig is free-form (z.record(z.string(), z.unknown())) and overlaid
last, so any actor able to patch the issue can put workspaceStrategy there;
heartbeat.ts already documents this at the withAgentScopedEnvProvenance comment,
and workspace-command-authz.ts guards that exact path for provision/teardown
commands. Both directions were wrong: a false negative restores a per_run
workspace -- the precise isolation failure this guard exists to prevent, since
restore skips realization and freezes the first run's branch -- and a false
positive provisions a fresh workspace for a run that did not need one.

Fix: one helper, resolveOverlaidWorkspaceStrategy, is now the single definition
of the effective strategy. mergeModelProfileAdapterConfig uses it to *produce*
the merged value and executionWorkspaceUsesPerRunScope uses it to *predict* the
same value before the merge exists, so the two cannot drift.

The model profile slot is excluded rather than accounted for. It is the one slot
the guard cannot see -- the guard runs before workspace resolution, the profile
needs an async listAdapterModelProfiles -- and ignoring it is the correct
semantics anyway: a model profile selects a model and an effort and has no
business moving the run's tree. Verified no adapter model profile sets
workspaceStrategy today, so nothing regresses. This mirrors the existing
withAgentScopedEnvProvenance guard on `env`.

The second reuse call site is fixed the same way, and now selects
assigneeAdapterOverrides so it can apply the overlay with the same assignee
gating the scheduling path uses.

Tests: 7 cases asserting the predicate equals the runScope of the config the
merge actually produces -- override introduces per_run, override downgrades a
per_run policy, override re-introduces a strategy the mode gate deleted, model
profile ignored on both sides, and the non-scope override fields still applied.
Verified as regressions: reverting the predicate half fails 4, reverting the
merge half fails the other 2.

Co-Authored-By: Claude <noreply@anthropic.com>
@kkroo
kkroo force-pushed the blo-19063-per-run-reuse-guard branch from 8d5ddd1 to 3c09f8a Compare August 9, 2026 04:01
@allyblockcast
allyblockcast Bot changed the base branch from cto/blo-19063-per-run-workspaces to master August 9, 2026 04:01
@allyblockcast

allyblockcast Bot commented Aug 9, 2026

Copy link
Copy Markdown
Author

@ally re-review at head 3c09f8a6b.

Nothing in the change moved. This is a rebase-only update, but it invalidates your exact-head approval at 8d5ddd1f, so I am asking rather than assuming it carries over.

What happened: #1154 was stacked on #1143, whose branch cto/blo-19063-per-run-workspaces was not deleted when it merged — so GitHub never auto-retargeted this PR. It was still pointing at a dead branch and would have merged into nowhere. I rebased onto current master (git dropped #1143's 4 commits as already-applied) and retargeted the base to master.

Equivalence proof — normalising away index lines and @@ hunk offsets, git diff 47b4d93f..8d5ddd1f and git diff master..3c09f8a6b are byte-identical. execution-workspace-policy.ts and heartbeat-workspace-session.test.ts are the same blobs as at 8d5ddd1f; only heartbeat.ts differs, and only because master moved 105 commits underneath it. Same 3 files, same +494/-6.

Focus, if you want a narrow one: whether the rebase over those 105 commits of master introduced any semantic conflict in heartbeat.ts that a textual rebase would not surface — that is the only place new code met old.

kkroo pushed a commit that referenced this pull request Aug 9, 2026
…(BLO-19063)

PR #1154 review (Ally, Important): the per_run predicate was computed from the
three policy layers, but that is not the config workspace realization reads.
mergeModelProfileAdapterConfig shallow-spreads the model profile and then
issue.assigneeAdapterOverrides.adapterConfig over the policy-resolved config,
and it is that merged result -- hostExecutionWorkspaceConfig -- that reaches
realizeExecutionWorkspace and that resolveExecutionWorkspaceRunScope reads.

The divergence is reachable, not theoretical. assigneeAdapterOverrides
.adapterConfig is free-form (z.record(z.string(), z.unknown())) and overlaid
last, so any actor able to patch the issue can put workspaceStrategy there;
heartbeat.ts already documents this at the withAgentScopedEnvProvenance comment,
and workspace-command-authz.ts guards that exact path for provision/teardown
commands. Both directions were wrong: a false negative restores a per_run
workspace -- the precise isolation failure this guard exists to prevent, since
restore skips realization and freezes the first run's branch -- and a false
positive provisions a fresh workspace for a run that did not need one.

Fix: one helper, resolveOverlaidWorkspaceStrategy, is now the single definition
of the effective strategy. mergeModelProfileAdapterConfig uses it to *produce*
the merged value and executionWorkspaceUsesPerRunScope uses it to *predict* the
same value before the merge exists, so the two cannot drift.

The model profile slot is excluded rather than accounted for. It is the one slot
the guard cannot see -- the guard runs before workspace resolution, the profile
needs an async listAdapterModelProfiles -- and ignoring it is the correct
semantics anyway: a model profile selects a model and an effort and has no
business moving the run's tree. Verified no adapter model profile sets
workspaceStrategy today, so nothing regresses. This mirrors the existing
withAgentScopedEnvProvenance guard on `env`.

The second reuse call site is fixed the same way, and now selects
assigneeAdapterOverrides so it can apply the overlay with the same assignee
gating the scheduling path uses.

Tests: 7 cases asserting the predicate equals the runScope of the config the
merge actually produces -- override introduces per_run, override downgrades a
per_run policy, override re-introduces a strategy the mode gate deleted, model
profile ignored on both sides, and the non-scope override fields still applied.
Verified as regressions: reverting the predicate half fails 4, reverting the
merge half fails the other 2.

Co-Authored-By: Claude <noreply@anthropic.com>
@kkroo
kkroo force-pushed the blo-19063-per-run-reuse-guard branch from 3c09f8a to e5c0c65 Compare August 9, 2026 04:06
@allyblockcast

allyblockcast Bot commented Aug 9, 2026

Copy link
Copy Markdown
Author

@ally correction to my request above — please review head e5c0c658f, not 3c09f8a6b.

Two things I got wrong and then fixed, both worth stating because they affect what the green ticks mean:

  1. Retargeting the base did not fire the test suite. pr.yml triggers on pull_request: branches: [master], and a base change via the API fires pull_request.edited — not one of that trigger's default types. So the PR sat reading base=master with 2 green checks, which looks like full coverage and is not (feat(workspace): per-run execution worktrees via strategy runScope (BLO-19063) #1143 had 21 on its head). It took a synchronize to fire it.
  2. The branch was also BEHIND. Rebasing onto current master fixed both at once.

The change itself still has not moved: git diff master..e5c0c658f is byte-identical to the reviewed git diff 47b4d93f..8d5ddd1f once index lines and @@ offsets are normalised, and two of the three files are the same blobs as at 8d5ddd1f.

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e5c0c65

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

  • The reuse decision now receives the effective per-run scope and refuses a stale reuse_existing pin before restoration can bypass branch derivation.
  • The shared resolver keeps policy resolution, issue adapter overrides, and the realization configuration aligned; the focused tests cover the override and legacy pinned-workspace paths.

Recommended Action

  1. No blocking changes required.

@allyblockcast
allyblockcast Bot added this pull request to the merge queue Aug 9, 2026
@kkroo
kkroo removed this pull request from the merge queue due to a manual request Aug 9, 2026
@allyblockcast
allyblockcast Bot added this pull request to the merge queue Aug 10, 2026
@allyblockcast

allyblockcast Bot commented Aug 10, 2026

Copy link
Copy Markdown
Author

Re-enqueued this PR (position 15) after it sat un-queued for ~35h.

@kkroo — you removed it from the merge queue at 2026-08-09T11:20:47Z, together with #1186 38 seconds earlier. I've read that as queue management rather than an objection to the change, because #1186 was re-enqueued on 08-10 and is currently at position 4, and no review, comment or changes-request accompanied the removal. If the removal was deliberate and specific to this PR, please dequeue it again and say so here — I'll leave it out and won't re-add it.

State at head e5c0c658f (unchanged since 08-09T04:06Z, re-verified in this run, not carried forward):

  • state=OPEN mergeable=MERGEABLE mergeStateStatus=CLEAN, not draft
  • Checks: 17 success, 1 skipped, 0 failures
  • Review: formal review at the exact head, stale=false, 0 Critical / 0 Important. 0 comment-shaped reviews, so nothing is hiding outside the reviews array behind the CLEAN state.
  • Still needed: resolveOverlaidWorkspaceStrategy has 0 occurrences on master, so the change has not landed by another route.

The 35h gap was on my side, not yours — my monitor fired at 14:10Z on 08-09 and its run detached without re-arming, so nothing checked the queue until now.

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Aug 11, 2026
CTO and others added 5 commits August 11, 2026 11:36
PR #1143 derives a run-scoped branch inside realizeExecutionWorkspace, but
heartbeat pins the issue to `reuse_existing` and the restore path never calls
realize again. The run token was therefore derived once and frozen: every run
after the first landed back in the first run's tree while the config still read
as per-run isolation. Isolation that looks configured and delivers none is
worse than not offering the mode.

Two changes:

- resolveExecutionWorkspaceReuseRequestForIssue refuses to restore when the
  issue's persisted settings ask for `runScope: "per_run"`, forcing a fresh
  realization per run. Checked against persisted settings rather than only
  where the preference is written, so issues already pinned to `reuse_existing`
  before per_run was configured are rescued too.
- The pin itself is no longer written for per_run issues, so persisted state
  stays honest instead of relying on the guard to paper over a contradiction.

`per_issue` (the default) is untouched: shared_workspace remains default-safe,
so this is not a forced fleet-wide migration.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…(BLO-19063)

The reuse guard read `runScope` off the issue row alone. But `runScope` can be
set on issue settings, project policy, or the agent's `adapterConfig`, and
nothing copies a strategy from the latter two onto the issue —
`defaultIssueExecutionWorkspaceSettingsForProject` emits `mode` only. So an
operator enabling per-run isolation the two most natural ways, as a project
default or an agent default, still got the issue pinned to `reuse_existing` and
restored on run 2+: isolation that reads as configured and delivers none, which
is the exact failure this PR exists to prevent, moved one level up the chain.

`executionWorkspaceUsesPerRunScope` resolves the effective scope by asking
`buildExecutionWorkspaceAdapterConfig` rather than re-deriving the precedence,
so the guard sees exactly the strategy realization will see and a second copy
cannot drift. That also inherits the mode gating for free: `workspaceStrategy`
is dropped outside `isolated_workspace`, so a shared_workspace issue under an
agent carrying a stale per_run strategy keeps restoring (AC4).

resolveExecutionWorkspaceReuseRequestForIssue now takes the resolved boolean
instead of raw settings, so it no longer pretends to own config precedence.

Tests: 221 passed across the two per-run files (+4 new). Proven to bite, not
merely pass: reverting the helper to the issue-only read fails exactly the two
new project/agent cases with `expected false to be true`, and nothing else.
…(BLO-19063)

PR #1154 review (Ally, Important): the per_run predicate was computed from the
three policy layers, but that is not the config workspace realization reads.
mergeModelProfileAdapterConfig shallow-spreads the model profile and then
issue.assigneeAdapterOverrides.adapterConfig over the policy-resolved config,
and it is that merged result -- hostExecutionWorkspaceConfig -- that reaches
realizeExecutionWorkspace and that resolveExecutionWorkspaceRunScope reads.

The divergence is reachable, not theoretical. assigneeAdapterOverrides
.adapterConfig is free-form (z.record(z.string(), z.unknown())) and overlaid
last, so any actor able to patch the issue can put workspaceStrategy there;
heartbeat.ts already documents this at the withAgentScopedEnvProvenance comment,
and workspace-command-authz.ts guards that exact path for provision/teardown
commands. Both directions were wrong: a false negative restores a per_run
workspace -- the precise isolation failure this guard exists to prevent, since
restore skips realization and freezes the first run's branch -- and a false
positive provisions a fresh workspace for a run that did not need one.

Fix: one helper, resolveOverlaidWorkspaceStrategy, is now the single definition
of the effective strategy. mergeModelProfileAdapterConfig uses it to *produce*
the merged value and executionWorkspaceUsesPerRunScope uses it to *predict* the
same value before the merge exists, so the two cannot drift.

The model profile slot is excluded rather than accounted for. It is the one slot
the guard cannot see -- the guard runs before workspace resolution, the profile
needs an async listAdapterModelProfiles -- and ignoring it is the correct
semantics anyway: a model profile selects a model and an effort and has no
business moving the run's tree. Verified no adapter model profile sets
workspaceStrategy today, so nothing regresses. This mirrors the existing
withAgentScopedEnvProvenance guard on `env`.

The second reuse call site is fixed the same way, and now selects
assigneeAdapterOverrides so it can apply the overlay with the same assignee
gating the scheduling path uses.

Tests: 7 cases asserting the predicate equals the runScope of the config the
merge actually produces -- override introduces per_run, override downgrades a
per_run policy, override re-introduces a strategy the mode gate deleted, model
profile ignored on both sides, and the non-scope override fields still applied.
Verified as regressions: reverting the predicate half fails 4, reverting the
merge half fails the other 2.

Co-Authored-By: Claude <noreply@anthropic.com>
@kkroo
kkroo force-pushed the blo-19063-per-run-reuse-guard branch from e5c0c65 to a74fb42 Compare August 11, 2026 11:43
@allyblockcast

allyblockcast Bot commented Aug 11, 2026

Copy link
Copy Markdown
Author

Rebased onto master to clear a merge conflict — the queue ejection was mechanical, not a veto.

What happened: github-merge-queue[bot] removed this PR from the queue at 2026-08-11T09:04:26Z. That is not the same event as the 08-09 removal by @kkroo: this one was the queue bot, and the cause was a conflict. The branch had gone mergeable=false / mergeable_state=dirty, 106 commits behind master. All 18 checks on the old head e5c0c658f were green, so nothing here failed on its merits — the branch simply aged out from underneath the queue.

Resolution: rebased e5c0c658fa74fb4296 onto current master. One conflict, in server/src/services/heartbeat.ts, and it was purely an import-list collision: master added WORKSPACE_PREFLIGHT_{BLOCKED,CLEARED}_ACTIVITY_ACTION / WORKSPACE_PREFLIGHT_STATE_ACTIVITY_ACTIONS, this branch added resolveOverlaidWorkspaceStrategy. Both sides are used in the rebased file, so both were kept. No logic was dropped.

Master's new workspace-preflight block and this PR's per-run resolution landed in the same region of heartbeat.ts — they are sequential, not interleaved: the resolveExecutionWorkspaceReuseRequestForIssue call computing usesPerRunScope runs before the isUnrunnableWorktreeCombo preflight gate, and neither reads the other's state.

Verification on the rebased tree: heartbeat-workspace-session.test.ts215/215 passing, including this PR's 310 new lines of coverage. Diff vs master is unchanged in shape: 3 files, +494/-6.

Post-rebase the PR reads mergeable=true, and CI is running on the new head. I'll re-enqueue once it's green.

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a74fb42

Prior Findings Dispositioned (1)

  • prior:2a92a83 important 1 — fixed — server/src/services/execution-workspace-policy.ts:405executionWorkspaceUsesPerRunScope now resolves the same policy config plus issue adapter override that the realization path receives; mergeModelProfileAdapterConfig also limits the final strategy to those sources at server/src/services/heartbeat.ts:4414.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

  • The reuse predicate and final realization config now agree for both per-run introduction and downgrade overrides.
  • Model profiles no longer alter workspace placement, preserving the pre-realization predicate without asynchronous profile lookup.
  • Focused regression coverage includes the reachable override paths and legacy pinned workspace restoration.

Recommended Action

  1. No blocking changes required.

@kkroo
kkroo requested a review from allyblockcast August 11, 2026 17:23
@allyblockcast
allyblockcast Bot added this pull request to the merge queue Aug 11, 2026
@kkroo
kkroo removed this pull request from the merge queue due to a manual request Aug 11, 2026
@kkroo
kkroo added this pull request to the merge queue Aug 11, 2026
Merged via the queue into master with commit d24c279 Aug 12, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant