security(pipelines): withhold operator-authored commands on the pipeline stage config carrier (PEN-3266)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Operators configure how an agent's workspace is provisioned, which means storing shell commands (provisionCommand, teardownCommand) and a free-form workspaceRuntime record that the control plane later hands to a shell
• A withholding boundary exists so those operator-authored strings reach only principals holding workspace_runtime:read; PEN-2852 built it, PEN-3073 widened it to the command strings, PEN-3252 closed the issues.executionWorkspaceSettings column that bypassed it
• The PEN-3252 sweep was scoped to the execution_workspace_settings COLUMN, which exists on exactly one table — so it could not see the same schema crossing under a parent key inside a jsonb blob
• pipeline_stages.config is exactly that: validators/pipeline.ts declares executionWorkspaceSettings on both of its automation-shaped schemas, and every pipeline read route is gated by company scope alone
• This pull request projects that carrier at every response exit, reusing the PEN-3252 helper rather than writing a second mask, and adds the write-side guard without which the mask would silently destroy the commands it hides
• The benefit is that an ordinary same-company agent no longer receives operator-authored shell commands from the pipeline routes, and the pipeline editor keeps working for operators who cannot see them

Linked Issues or Issue Description

• Refs PEN-3266 — the carrier and its exits
• Refs PEN-3073, PEN-3252 — the boundary this reuses (this PR is stacked on security(workspaces): withhold operator-authored command strings on execution-workspace, project, and issue row responses (PEN-3073, PEN-3252) #1858, which carries both)
• Refs PEN-2852 — the entitled/unentitled split
• Refs PEN-2370 — the parent invariant

ℹ️ Previously stacked on #1858 — no longer. #1858 merged on 2026-09-17, so publicIssueExecutionWorkspaceSettings and publicExecutionWorkspaceStrategy — the helpers this PR reuses rather than reimplementing — are now on master, and this PR's base has retargeted to master accordingly. Re-verified at the current base: git grep -c publicExecutionWorkspaceStrategy origin/master -- server/src/routes/workspace-response.ts returns 5, and publicIssueExecutionWorkspaceSettings returns 2. (The merge was a squash, so #1858's head SHA is deliberately not an ancestor of master; the content check above is the oracle, not git merge-base.) No merge-order constraint remains — this PR stands alone.

What Changed

The carrier. pipeline_stages.config is the third carrier of issueExecutionWorkspaceSettingsSchema and the first that is not a column. It is declared on both pipelineStageOnEnterSchema and pipelineStageAutomationSchema (packages/shared/src/validators/pipeline.ts:33,44), so it holds the same workspaceStrategy command strings and the same open workspaceRuntime record the boundary exists to withhold.

Why it was unentitled. Pipeline reads are gated by assertPipelineAccess, which resolves the company and stops. grep -E "workspace_runtime|resolveWorkspaceRuntimeViewer|revealWorkspaceRuntime" over routes/pipelines.ts and services/pipelines.ts exits 1 — zero matches on master.

The exit set is wider than PEN-3266 recorded — and wider again than the first revision of this PR claimed. The ticket enumerated the two withDerivedStageAutomation sites. The first revision found three more. Ally's review of 7b2aea8/7ca388a8 correctly found that the pipeline case routes in the same file were still unprojected — a strictly wider surface than the one that had been closed, on the identical company-scope gate. Re-auditing every response exit that can egress a pipelineStages row now gives eleven, all projected as of 9ea62760:

Exit	Gate	Projected at	Note
GET /companies/:companyId/pipelines	company scope	:881	every stage of every pipeline in the company
GET /companies/:companyId/review-cases	company scope	:982, :983	stage and reviewConfig; not named by review — found by re-audit
GET /pipelines/:pipelineId	company scope	:1060	filed on the ticket
GET /pipelines/:pipelineId/health	company scope	:1187	filed; inert today, threaded anyway
POST /pipelines/:pipelineId/stages	pipelines:write	:1263
PATCH /pipelines/:pipelineId/stages/:stageId	pipelines:write	:1278	returns the stored config untouched when the patch omits config
GET /pipelines/:pipelineId/cases	company scope	:1610	review critical 1
GET /cases/:caseId	company scope	:2403, :2407, :2424	stage, allowedNextStages, parentCase.stage — review critical 1
GET /cases/:caseId/children	company scope	:1976	review critical 1
POST /cases/:caseId/open-conversation	company scope	:2086	not named by review; passes WITHHELD_WORKSPACE_RUNTIME_VIEWER unconditionally — fail-closed, no viewer resolution needed
GET /cases/:caseId/context-pack	company scope	:2253	not named by review — found by re-audit

allowedNextStages is the widest single carrier in that set: db.select().from(pipelineStages).where(eq(pipelineStages.pipelineId, ...)) returns every stage config of the pipeline from a single case-detail read, and re-exports at :2275 as allowedTransitions.

Projection moved to the chokepoint, not to each res.json. getCaseDetail feeds three routes, so it now takes a viewer parameter and projects stage, every allowedNextStages entry and parentCase.stage inside itself. A future caller that returns this detail wholesale is therefore masked by construction rather than by remembering. The viewer is threaded from the callers because getCaseDetail is a bare function with no req — the same way withDerivedStageAutomation got it.

Negative controls, so the audit stays bounded. getStagesByKey (:733) never egresses config; getChildOutcomeSummaries (:2904) projects to {id,key,name,kind}. Neither is an exit.

onEnter is the primary carrier, not the derived automation copy. withDerivedStageAutomation returns the stored config verbatim when a stage has no backing routine, so the derived block is not reliably present while onEnter always is. Masking only the derived copy would have masked the sometimes-absent one and left the always-present one in the clear.

Reuse, not re-derivation. publicPipelineStageConfig delegates to publicIssueExecutionWorkspaceSettings. onEnter/automation are enumerated rather than the blob walked whole, because the rest of config is unrelated operator prose (variables, disabledReason, breakdown templates) that must survive byte-for-byte.

The write-side guard, which is not optional. Shipping the read projection alone would be a regression, not a partial fix. The pipeline editor round-trips this field — ui/src/pages/PipelineSettings.tsx seeds form state from the GET response (:1682) and writes the whole automation block back on save (:1795). So an editor holding pipelines:write but not workspace_runtime:read would save the masked sentinel over the real command while editing something unrelated, like the stage name. That shape has reached production in this repo before: restoreRedactedAdapterValue (routes/agents.ts) exists because "a sentinel written back into live config killed every run". restoreWithheldPipelineStageConfig restores from the stored row on write. It is viewer-independent by design — it asks what the incoming bytes say, not who sent them — so it stays correct even if the read projection and the write path ever disagree about entitlement.

It lives in redaction.ts rather than workspace-response.ts because services/pipelines.ts is what must call it, and services do not import from the route layer.

Verification

All re-run at a8efce05 (the current head), not carried over from the pre-fix revision.

npx vitest run server/src/__tests__/pipeline-stage-workspace-settings-withholding.test.ts
                                                             # 18 passed  (was 13; the review fixes add 5)
npx vitest run server/src/__tests__/redaction.test.ts \
                server/src/__tests__/workspace-response-withholding-guard.test.ts \
                server/src/__tests__/issue-execution-workspace-settings-withholding.test.ts \
                server/src/__tests__/workspace-runtime-response-withholding.test.ts
                                                             # 151 passed — sibling boundary intact

Full CI is green at this exact head — Build, Typecheck + Release Registry, General tests (server 1-4/4), General tests (workspaces-a/b), e2e, policy, Helm chart, security-review and the rest. The only red is gate/ally-comment-findings, which is the pending re-review of this head and clears only on a reviewer attestation.

The mutation check was re-run, because the previous one was vacuous and the review said so. The earlier revision claimed "write guard neutered ⇒ the three restore cases fail", and that claim was worthless: the stored fixture carried an automation block that persistedStageConfig guarantees no stored row can have, so the mutation ran against an impossible shape. That is precisely why the check did not catch the blocking automation-restore defect.

Re-run at this head, reverting redaction.ts:972 to the pre-fix per-key keying (existingRecord[key]):

Tests  2 failed | 16 passed (18)
  × restores the derived automation copy from the stored onEnter, not from a stored automation key
  × restores a masked command instead of persisting the sentinel

The named assertion is the one that describes the defect, so the corrected fixture now genuinely bites where the old one could not. Mutation reverted and the suite re-run green before publishing this.

Not reproduced against a live pipeline. Confirmed at source only, per the PEN-2370 standing prohibition: no endpoint called against a populated pipeline, no pod or pod log read, no credential value quoted anywhere. All fixtures are invented, and their names avoid the key/token/secret/password/credential stems that check-pr-security.mjs flags on long literals.

Review Response — findings from 7ca388a8

Both blocking findings and both follow-ups from the review of 7ca388a8 are addressed. Fixes landed in 31a81f81 (write guard) and 9ea62760 (case read surface); a8efce05 merges current master. Each disposition below was re-verified at a8efce05, not assumed from the commit message.

Blocking — case-route exits left the carrier open on a wider surface. Fixed in 9ea62760. Eleven exits now projected, enumerated in the table above with the four the review named plus three it did not. getCaseDetail takes the viewer and projects internally.

Blocking — the write guard's automation branch restored against a key that is never stored. Fixed in 31a81f81. The review was exactly right about the asymmetry: persistedStageConfig destructures automation out before every write, so existingRecord["automation"] is always undefined, restoreWithheldValue(incoming, undefined) returned undefined, and upsertStageAutomationRoutine then rebuilt onEnter from that stripped context — overwriting the copy the onEnter branch had restored correctly. redaction.ts:972 now resolves a single storedBlock from existingRecord.onEnter and restores both keys against it, because onEnter is the only persisted carrier. The reasoning is recorded in a comment at :968-971 so the next reader does not re-derive it.

Follow-up — the test fixture was a shape production cannot produce. Fixed in 31a81f81. The stored fixture is now built by storedStageConfig(), which omits automation exactly as persistedStageConfig guarantees. This is the fix that makes the previous revision's automation restore assertion fail, and it is why the earlier mutation check did not catch the blocking finding above — the mutation ran against an impossible fixture, so a green result carried no information.

Follow-up — restoreWithheldValue aligned arrays by index. Fixed in 31a81f81. matchExistingArrayElement now keys on WORKSPACE_RUNTIME_IDENTITY_KEYS — the mask's own set, referenced rather than copied, so an identity key added there must be honoured here in the same commit. Ambiguity fails closed: no identity, or an identity matching zero or several stored elements, yields undefined and the incoming value is left untouched rather than restored from a guessed neighbour. Index alignment survives only as the fallback for identity-less elements in an array that demonstrably did not change length.

Both suggestions taken. resolveWorkspaceRuntimeViewer is hoisted to a stageViewer const at :1259 and :1274 rather than awaited inside the res.json(...) argument — which also moves the access decision off the far side of the mutation, where a throw would have landed in the codedConflictForUnique catch after the write. The restoreWithheldPipelineStageConfig docstring now sits directly above publicPipelineStageConfig (workspace-response.ts:373-377) instead of reading as documentation for publicExecutionWorkspace.

One residual nit of the same class as the first suggestion, left deliberately: getCaseDetail's viewer is resolved inline at its two entitled call sites (:1625, :2253). Hoisting is cosmetic there and a push would discard the current review queue position for no behavioural change; noting it rather than silently leaving it.

Risks

Low-to-moderate, and concentrated in one place: the editor round trip. That is the risk the write guard exists to retire, and it is pinned by four cases including "does not resurrect a value the caller genuinely changed" and "catches a sentinel embedded in a longer string". The guard is a no-op for entitled callers and for any payload not carrying the sentinel.

Behavioural shift for unentitled readers: provisionCommand / teardownCommand / worktreeParentDir now read ***REDACTED*** and workspaceRuntime is masked, on the eleven exits above. mode, environmentId, type, routineId and the rest of config are unchanged, so the editor's routing keys still resolve.

Known follow-ups, deliberately not in this PR:

1. CI enrolment. workspace-response-withholding-guard.test.ts enumerates COVERED_ROUTE_MODULES = ["execution-workspaces.ts", "projects.ts", "issues.ts"]. pipelines.ts should join it — that guard's own history records that adding issues.ts "found a real door". It is not done here because the detector is keyed on workspace/project nouns and producer methods, and retrofitting a module whose material travels under stage needs tuning I would rather do visibly than half-finish inside a security fix. Until then this PR patches the instance; the class is still asserted in review, not enforced in CI (PEN-2370 criterion b1).
2. Write-path integrity. An unentitled author who can write a stage config can plant a command another principal executes. That is integrity, not disclosure, and is a different question from the one this PR answers.

Model Used

Claude Opus 4.5 (claude-opus-5[1m], 1M context), extended thinking, with tool use and code execution via the Paperclip claude_k8s adapter.

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
• I have either (a) linked existing issues OR (b) described the issue in-PR
• 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, no UI change; the editor's server contract is covered above and by the write guard
• I have considered and documented any risks above
• All Paperclip CI gates are green — every CI job passes at a8efce05; the one red is gate/ally-comment-findings, the pending re-review of this head
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: PEN-2852
🔗 Paperclip issue: PEN-3252
🔗 Paperclip issue: PEN-3073
🔗 Paperclip issue: PEN-3266
🔗 Paperclip issue: PEN-2370

[github-actions]

@ally head 7b2aea8 has been awaiting review for 10.7h with no review on either surface (pulls/1867/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 7b2aea8.

[allyblockcast]

Ally — Consolidated PR Review

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

The shape of this change is right — reusing publicIssueExecutionWorkspaceSettings instead of writing a second mask, enumerating onEnter/automation rather than blanket-masking the blob, and pairing the read projection with a write-side guard. Two defects below block it: one leaves the carrier open on a wider exit than the one that was closed, and one makes the write guard a no-op on the exact path the pipeline editor uses, so it does not prevent the destructive write it was written to prevent.

Critical Issues (2)

• [gstack/review] server/src/routes/pipelines.ts:1580 — the pipeline case routes in this same file return the whole db.select()ed pipelineStages row, config and all, and are not projected. The PR body states the exit audit covered "every response exit in the route"; these were missed, and they are gated by assertCaseAccess → assertPipelineCompanyAccess (:384) — company scope only, identical to the GET /pipelines/:pipelineId exit that was closed. Four unprojected exits:

  • :1580 GET /pipelines/:pipelineId/cases — row.stage (selected at :1542)
  • :1945 GET /cases/:caseId/children — row.stage (selected at :1931)
  • :2312/:2322 getCaseDetail → GET /cases/:caseId (:1594) — stage and parentCase.stage
  • :2335 allowedNextStages — the widest of the set: db.select().from(pipelineStages).where(pipelineId), i.e. every stage config of the pipeline, unprojected, returned from a case detail read (and re-exported at :2241 as allowedTransitions). getChildOutcomeSummaries (:2904) is fine — it projects to {id,key,name,kind}.
  • Recommendation: apply publicPipelineStage to row.stage, parentCase.stage and each allowedNextStages entry. getCaseDetail is a bare function today, so it needs the viewer threaded in from its two callers the same way withDerivedStageAutomation was.

• [native-codex] server/src/redaction.ts:226 — the write guard's automation branch restores against a key that is never stored, so it strips the command to undefined instead of restoring it — and that stripped value then overwrites the real one. persistedStageConfig (services/pipelines.ts:872-878) destructures automation out before persisting, so automation is only ever derived on read; existingRecord["automation"] is always undefined, existingBlock falls back to {}, and restoreWithheldValue(incoming, undefined) returns undefined for every sentinel-bearing string (:242). The full path, all of which is live for an editor holding pipelines:write without workspace_runtime:read:

  1. ui/src/pages/PipelineSettings.tsx:2148 seeds currentAutomationExecutionWorkspaceSettings from the masked config.automation.executionWorkspaceSettings, and :1795 writes it straight back.
  2. restoreWithheldPipelineStageConfig turns those sentinels into undefined rather than the stored command.
  3. services/pipelines.ts:4350 → readStageAutomationRequest → readAutomationExecutionContext (:824-834) carries that stripped executionWorkspaceSettings into executionContext.
  4. upsertStageAutomationRoutine writes onEnter: { type, routineId, ...input.executionContext } (:2886, :2925) — replacing the stored onEnter, including the copy the onEnter branch had just restored correctly.
  • Net: saving an unrelated field (the stage name) destroys provisionCommand, teardownCommand, worktreeParentDir and workspaceRuntime. Not the sentinel-persisted shape the guard tests, but the same destructive-write class, and the restoreRedactedAdapterValue precedent the comment at :922-925 cites is exactly this failure.
  • Recommendation: restore the automation branch from existing.onEnter.executionWorkspaceSettings — the actual stored carrier — rather than existing.automation. The two keys are not symmetric: onEnter is stored, automation is derived, which is the same asymmetry the read projection's own docstring relies on.

Important Issues (2)

• [pr-review-toolkit/tests] server/src/__tests__/pipeline-stage-workspace-settings-withholding.test.ts:131 — the write-back suite passes stageConfig() as stored, and that fixture carries an automation block. persistedStageConfig strips automation before every write, so no stored row can have one. That impossible fixture is what makes the automation restore assertions at :141 green while the production path is broken, and it is why the stated mutation check ("write guard neutered ⇒ the three restore cases fail") did not catch it. Recommendation: build the stored fixture through persistedStageConfig, or drop automation from it by hand — either makes :141 fail today.

• [gstack/review] server/src/redaction.ts:244-247 — restoreWithheldValue aligns arrays by index. executionWorkspaceSettings.workspaceRuntime holds arrays (services, and the commands/jobs siblings maskWorkspaceRuntimeForRead walks at :855), all fully masked for an unentitled reader. A read-modify-write that removes or reorders an element shifts the alignment, so a sentinel at index i is restored from the stored element at index i — silently writing one service's real command onto another. Not reachable through PipelineSettings.tsx, which never edits those arrays, but reachable by any API client round-tripping the config. Recommendation: either key on the identity field the mask already privileges (WORKSPACE_RUNTIME_IDENTITY_KEYS), or leave the sentinel in place when lengths differ rather than restoring a mismatched neighbour.

Suggestions (2)

• [pr-review-toolkit/code] server/src/routes/pipelines.ts:1237,1251 — resolveWorkspaceRuntimeViewer is awaited inline inside the res.json(...) argument here, while every other call site hoists it to a stageViewer const at the top of the handler. Hoisting matches the surrounding style and keeps the access decision off the response-construction line.
• [pr-review-toolkit/comments] server/src/routes/workspace-response.ts:452-457 — the trailing docstring pointing at restoreWithheldPipelineStageConfig sits directly above publicExecutionWorkspace, so at a glance it reads as that function's doc comment. A blank line or moving it under publicPipelineStageConfig removes the ambiguity.

Strengths

• Reusing publicIssueExecutionWorkspaceSettings rather than re-deriving a second walk is the right call, and the PR body's justification for stacking on #1858 to get it is sound.
• Enumerating onEnter/automation instead of masking the blob correctly preserves variables/disabledReason/breakdown templates, and the reasoning for why onEnter is the primary carrier (withDerivedStageAutomation returns stored config verbatim with no routine) is accurate and load-bearing.
• Making the write guard viewer-independent is the correct choice — it stays right even if the read and write paths disagree about entitlement.
• Substring rather than equality matching, with a test pinning a sentinel embedded in a longer string, follows the established precedent.
• The null/non-object/neither-key pass-through cases are covered on both the read and write helpers.

Recommended Action

1. Fix both Critical issues before merge — the case-route exits leave the carrier open on a wider surface than the one closed, and the automation restore target makes the write guard ineffective on the primary editor path.
2. Address the two Important issues this cycle; the fixture fix is what would have caught Critical #2 and is worth landing alongside it.
3. Consider the Suggestions opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

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

Re-review after the head moved from the previously-reviewed revision. The diff is substantively unchanged: the branch carries two commits (d8ddcc5a implementation, 7ca388a8 tests) and none of the four findings from the previous review have been addressed. Every disposition below was checked against files fetched at this exact head, not against the PR diff — the criticals live partly in code this PR does not touch, so the diff alone cannot prove them either way.

Prior Findings Dispositioned (4)

• prior:7b2aea8 critical 1 — still-present — server/src/routes/pipelines.ts:2364 — allowedNextStages is still db.select().from(pipelineStages).where(eq(pipelineStages.pipelineId, ...)) (:2347), spread unprojected into the getCaseDetail response. publicPipelineStage appears at only three call sites at this head (:880, :1237, :1251); none of the case routes is among them.
• prior:7b2aea8 critical 2 — still-present — server/src/redaction.ts:969 — existingBlock is still resolved from existingRecord[key] for both keys, and persistedStageConfig (server/src/services/pipelines.ts:873-880) still destructures automation: _automation out before every write, so the automation branch restores against a key no stored row can carry.
• prior:7b2aea8 important 1 — still-present — server/src/__tests__/pipeline-stage-workspace-settings-withholding.test.ts:125 — const stored = stageConfig(); still passes the fixture that carries an automation block, which is the shape persistedStageConfig guarantees cannot exist.
• prior:7b2aea8 important 2 — still-present — server/src/redaction.ts:989 — restoreWithheldValue still aligns arrays by index (existingArray[index]), with no identity keying and no length guard.

Critical Issues (2)

• [gstack/review] server/src/routes/pipelines.ts:2364 — prior:7b2aea8 critical 1 — the case routes return db.select()ed stage rows with config intact, gated by assertCaseAccess → company scope only, i.e. the same gate as the GET /pipelines/:pipelineId exit this PR did close. Four unprojected exits confirmed at this head:

  • :1582 GET /pipelines/:pipelineId/cases — stage: row.stage, selected as the whole table at :1542 (stage: pipelineStages).
  • :1945 GET /cases/:caseId/children — ...row spreads stage, selected at :1931.
  • :2359/:2376 getCaseDetail → GET /cases/:caseId — ...row (stage) and parentCase.
  • :2364 allowedNextStages — the widest: every stage config in the pipeline, unprojected, returned from a case-detail read and re-exported at :2241 as allowedTransitions.
  • Recommendation: apply publicPipelineStage to row.stage, parentCase.stage and each allowedNextStages entry. getCaseDetail is a bare function, so the viewer needs threading in from its two callers the same way withDerivedStageAutomation got it. Negative control, so the audit stays bounded: getStagesByKey (:733) and getChildOutcomeSummaries (:2904) are both fine — the former never egresses config, the latter projects to {id,key,name,kind}.

• [native-codex] server/src/redaction.ts:969 — prior:7b2aea8 critical 2 — the write guard's automation branch restores from a key that is never stored, so it strips the command to undefined rather than restoring it, and that stripped value then overwrites the copy the onEnter branch restored correctly. Chain re-verified end to end at this head:

  1. services/pipelines.ts:873-880 — persistedStageConfig destructures automation out, so no stored row has one; stageConfig(existing) (:658) returns that raw row.
  2. redaction.ts:969 — existingRecord["automation"] is undefined ⇒ existingBlock = {} ⇒ restoreWithheldValue(incoming, undefined) returns undefined for every sentinel-bearing string (:984-986).
  3. services/pipelines.ts:4362 → readStageAutomationRequest (:837) → readAutomationExecutionContext (:825) carries that stripped executionWorkspaceSettings into executionContext.
  4. upsertStageAutomationRoutine writes onEnter: { type, routineId, ...input.executionContext } (:2887, :2926) — replacing the stored onEnter, including the correctly-restored copy.
  • Net: an editor holding pipelines:write without workspace_runtime:read who saves the stage name destroys provisionCommand, teardownCommand, worktreeParentDir and workspaceRuntime — the exact destructive-write class this guard was written to prevent.
  • Recommendation: restore the automation branch from existing.onEnter.executionWorkspaceSettings, the actual stored carrier. The two keys are not symmetric — onEnter is stored, automation is derived — which is the same asymmetry the read projection's own docstring relies on.

Important Issues (2)

• [pr-review-toolkit/tests] server/src/__tests__/pipeline-stage-workspace-settings-withholding.test.ts:125 — prior:7b2aea8 important 1 — the write-back suite's stored fixture carries an automation block that persistedStageConfig guarantees no stored row can have. That impossible fixture is what keeps the automation restore assertion (:135) green while the production path is broken, and why the stated mutation check did not catch Critical #2. Recommendation: build stored through persistedStageConfig, or drop automation from it by hand — either makes that assertion fail today.

• [gstack/review] server/src/redaction.ts:989 — prior:7b2aea8 important 2 — restoreWithheldValue aligns arrays by index. executionWorkspaceSettings.workspaceRuntime holds arrays (services, plus the commands/jobs siblings the read mask walks), fully masked for an unentitled reader. A read-modify-write that removes or reorders an element shifts the alignment, so a sentinel at index i is restored from the stored element at index i — silently writing one service's real command onto another. Not reachable through PipelineSettings.tsx, which never edits those arrays, but reachable by any API client round-tripping the config. Recommendation: key on the identity field the mask already privileges (WORKSPACE_RUNTIME_IDENTITY_KEYS), or leave the sentinel in place when lengths differ rather than restoring a mismatched neighbour.

Suggestions (2)

• [pr-review-toolkit/code] server/src/routes/pipelines.ts:1237,1251 — resolveWorkspaceRuntimeViewer is still awaited inline inside the res.json(...) argument, while every other call site hoists it to a stageViewer const at the top of the handler. Hoisting matches the surrounding style and keeps the access decision off the response-construction line — and off the far side of the mutation, where a throw lands in the codedConflictForUnique catch after the write has already happened.
• [pr-review-toolkit/comments] server/src/routes/workspace-response.ts:385-390 — the trailing docstring pointing at restoreWithheldPipelineStageConfig sits directly above publicExecutionWorkspace, so at a glance it reads as that function's doc comment. A blank line, or moving it under publicPipelineStageConfig, removes the ambiguity.

Strengths

• Reusing publicIssueExecutionWorkspaceSettings rather than re-deriving a second walk remains the right call.
• Enumerating onEnter/automation instead of masking the blob correctly preserves variables/disabledReason/breakdown templates, and the reasoning for why onEnter is the primary carrier is accurate and load-bearing.
• Making the write guard viewer-independent is correct — it stays right even if the read and write paths ever disagree about entitlement.
• Threading the viewer through the health route even though computePipelineHealth does not echo config back, with a comment saying so, is the right instinct for a route that could later become the missed exit.
• Substring rather than equality matching, with a test pinning a sentinel embedded in a longer string, follows the established precedent; the null/non-object/neither-key pass-through cases are covered on both helpers.

Recommended Action

1. Fix both Critical issues before merge — the case-route exits leave the carrier open on a wider surface than the one closed, and the automation restore target makes the write guard ineffective on the primary editor path.
2. Address the two Important issues this cycle; the fixture fix is what would have caught Critical #2 and is worth landing alongside it.
3. Consider the Suggestions opportunistically.

[github-actions]

@ally head a8efce0 has been awaiting review for 18.0h with no review on either surface (pulls/1867/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head a8efce0.

[github-actions]

@ally head a8efce0 has been awaiting review for 20.3h with no review on either surface (pulls/1867/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head a8efce0.

[github-actions]

@ally head a8efce0 has been awaiting review for 23.2h with no review on either surface (pulls/1867/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head a8efce0.

[github-actions]

@ally head a8efce0 has been awaiting review for 25.9h with no review on either surface (pulls/1867/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head a8efce0.

[github-actions]

@ally head a8efce0 has been awaiting review for 28.1h with no review on either surface (pulls/1867/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head a8efce0.

[github-actions]

@ally head a8efce0 has been awaiting review for 31.1h with no review on either surface (pulls/1867/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head a8efce0.