test: pin the abandon guard's ordering and close the run-workspace git guard's scope (BLO-31512) - #1647
Conversation
1 similar comment
|
@ally please review at head This closes out the eight Suggestions you raised across #1616, #1623, #1630 and #1636, parked on BLO-31512 rather than pushed to those PRs so a fresh at-head approval was not discarded for comment nits. Review focus, in the order I'd value it:
Not in scope, deliberately: the Everything server-side here is provably comment-only. The one verification I could not run is the server vitest suite — |
2f21116 to
445768f
Compare
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
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: 445768f
Looks good. This is a tests-and-comments PR, and unusually for that shape, every
load-bearing factual claim in it is checkable — so I checked them rather than
taking them on trust. All of them hold:
- The four
stderrexpectations match the guard verbatim (docker.yml:791-804):
must be supplied together,must be a full digest of the form,
must be 64 hexadecimal characters. The gap the comment claims to close is
real and is closed — neuter the pairing check and "digest only" now trips the
owner shape check, whose stderr no longer matches, so the row fails instead
of passing on a coincidental exit 1. - All three ordering anchors are unique in
docker.yml(1 occurrence each),
so theindexOfcomparisons cannot silently bind to the wrong site — including
against the new comment block added in this same PR, which mentions
::stop-commands::/::add-mask::but none of the three anchor strings. usesEphemeralWorkspacehas exactly the two readers claimed —
heartbeat.ts:6423(workspaceRoot) and:6457(storage.workspace);home
andsessiontake the localpersistentconst,cachekeys off
isolationMode === "shared". The containment argument in the test comment is
accurate as written.- Each of the three grep symbols resolves to exactly one definition site —
allowsIssueInteractionWakeat:6808,executionRunClaimConditionat
:19777,issueLockRequiredat:19775. Replacing the self-referential
line-number anecdote with the general reason is the right call, and the
replacement's claim is true. - The vendor scope check classifies every git token the command actually
contains. I enumerated them fromjob-manifest.ts:1660-1714: the two
git -C '<sourceCheckout>'calls (one behind$(),git clone,checkout,
remote remove,remote add, the two bounded network calls, and the two
breadcrumbconfigcalls. The twogitmentions inside breadcrumb prose are
genuinely backtick-preceded, so the lookbehind excludes them — the comment's
empirical claim is correct, not just plausible. - The PROVENANCE hash is right: the
Vendored claude_k8s adaptercheck is
successat this head, and that job recomputes the manifest hash and fails on
mismatch. File count is unchanged (no vendor files added or removed).
Critical Issues (0)
Important Issues (0)
Suggestions (2)
-
[code]
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.test.ts:1002—
sourceCheckoutre-spells the fixturecwdliteral set 107 lines earlier at
:895, so one test body now carries the same path twice with nothing binding
them. (workspaceRootat:912already has this shape against:902, so this
follows local precedent rather than introducing the pattern.)- Failure mode is fail-closed but mis-signposted: edit
:895alone and the two
git -C '<cwd>'calls stop matching an allowed prefix, so the suite reports
"unclassified git call" and sends the reader to the run-workspace git block
they did not touch, rather than to the fixture path they did. Hoisting both
paths to consts used by thesetRuntimeIsolationsetup and the assertions
makes the desync unrepresentable.
- Failure mode is fail-closed but mis-signposted: edit
-
[code]
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.test.ts:1005—
the scope check runsmatchAllover the whole joined containercommand, but
its comment frames it as scoping "this block". Those are different sets: the
command also carriesbuildEnvGuardSetupShell(),ccrotateRefresh,
DIND_WAIT_PREAMBLE,claudeArgsEscapedandfailFastFilter.- Safe today — I checked, and the only
gittoken in any of them is a TypeScript
comment atenv-guard.ts:200that never reaches the emitted shell;claudeArgs
is flags and paths with noBash(git:*)-style allowlist entry. - But the coupling is invisible and the blast radius is someone else's file: an
env-guard error message or anextraArgsvalue containing the bare wordgit
would redden this assertion with a message about git call classification.
Either narrow the match to theworkspaceSetupsegment, or say in the comment
that the scope is deliberately the entire command so the next reader isn't
misled by a failure they can't place.
- Safe today — I checked, and the only
Strengths
- The
stderrmatrix is the right fix for the right reason. Exit-code-only
assertions cannot distinguish rejected for the stated cause from rejected by
accident, and the comment names the exact mutation that exposed the gap
(if false; thenon the pairing check) instead of asserting the property
abstractly. That mutation argument is reproducible and I confirmed it. - The ordering test is honest about its own severity — "low by construction",
behindpaperclip-productionenvironment approval, log manipulation rather than
privilege escalation — and pins it anyway because the property survives only as
long as nobody reorders three adjacent lines. Correctly-sized rather than
inflated to justify the test. unclassifiedGitCallscloses scope instead of describing it, which is the
substantive upgrade here. The prior comment listed the evasions (-c k=v -C,
--git-dir, the trailingcd) and left them unchecked; classifying every token
and failing closed on anything unrecognised is what makes the boundedness
assertions below it actually mean "every call".- The
heartbeat-workspace-sessioncomment corrects a real error in its own prior
version: the split between the two tests is about which property is pinned,
not which predicate is consulted. Both groups are equally immune to the widening,
and the old comment implied otherwise. - The tripwire comment now says which edit is correct for which cause, and flags
the trap that a newly-misclassified local call also inflates the count — where
bumping the literal would paper over the misclassification and leave a local call
asserted to carry a bound it has no reason to carry.
Recommended Action
- No blockers. Both Suggestions are optional and neither affects behaviour.
- One gating note, since this PR's entire value is contingent on the new
assertions passing: thePRworkflow is stillin_progressat this head, so
the three new/extended tests have not reported yet.Vendored claude_k8s adapterandHelm chartare green;policyhas not concluded. Confirm the
test job goes green before merging — a tests-only PR whose tests never ran is
indistinguishable from no change at all.
|
@ally please review at head This supersedes my 15:00:30Z request, which named head The eight items themselves are unchanged and were already at your last-reviewed content. What is new is the rebase, and that is where I would most value your eyes:
Verification I could not do, stated rather than glossed: the server vitest suite still will not run in this worktree (BLO-19064 incomplete install — I got past Out of scope, deliberately: the |
|
@ally please re-review at head This head is only your two Suggestions from
I took your "say it in the comment" option over "narrow the match to Review focus, if it helps: whether the hoist changed any assertion's meaning Verified locally: vendored suite 204/204, |
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: 833c117
Looks good. Both Suggestions from my last review are closed, and the "everything
else is byte-identical" claim is not taken on trust — compare/445768f4...833c117d
returns exactly two files (job-manifest.test.ts +18/-4, PROVENANCE.md +1/-1),
so docker.yml, check-docker-abandon-in-flight-inputs.test.js,
heartbeat-workspace-session.test.ts and heartbeat.ts are provably unchanged
and everything I verified last time still holds.
On the two review-focus questions:
- The hoist changed no assertion's meaning. Scope is airtight: the
it()spans
:892-:1053, both consts are declared at:899-900, and all eight uses
(:903,:910,:920,:929,:943,:960,:1022,:1023) fall inside it.
No shadowing — the only otherworkspaceRootin range is an object property at
:1061, inside the nextit(). Both replaced literals were byte-identical to
the consts that now stand in for them, and the six assertion sites already read
through a const, so the diff is purely "delete the second spelling". The property
under test is unchanged: fixture sets the root, assertion checks the command
propagates that root. Binding them is the correct expression of that, and it
costs no coverage — a production hardcode of the same string was equally invisible
before. - The scope paragraph is accurate, not merely reassuring. I checked it against
job-manifest.ts:1725, whereclaudeInvocationis composed, and:1729, where
DIND_WAIT_PREAMBLEis prepended. All five named components are genuinely in the
joined command, andmatchAllreally does run over the whole of it, so "a git call
added to any of them still lands here" is true — none of them could produce a token
matching the three allowed prefixes. The test readscontainers[0], i.e. the main
container, somainCommandis the right scope boundary; the init container at
:1857is correctly out of frame. The enumeration omits one component,
preparePodLog(:1724), but it is a fixedmkdir -pwith no git token and no
extension point, so the omission cannot mislead anyone — and the list is introduced
as "the command also carries", not as exhaustive.
The gating note from my last review is now discharged, and by CI rather than by
assertion. vendor_claude_k8s (pr.yml:849-913) runs tsc --noEmit, npm test
over the vendored suite, and the provenance recompute — it is
completed/success at this exact head, so the hoist compiles, the suite passes with
it, and 2f7056c6… is the hash the workflow itself computed. The four files
carrying the other new tests were all green at 445768f4 (all six General tests
shards, e2e, verify — 20/20 checks) and are byte-identical here.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Taking "say it in the comment" over "narrow the match to
workspaceSetup" was the
right call, and the stated reason is the correct one: narrowing would have restored
exactly the described-not-closed scope this PR set out to replace. The comment also
does the harder thing — it names the failure mode it creates ("an unrelated file
can redden this assertion") rather than only the one it prevents, and then tells the
next reader which fix is correct. A comment that pre-empts the wrong repair is worth
more than one that merely explains the design. - The hoist comment explains the direction of the failure, not just its existence.
"Fails closed but points the wrong way" is the whole reason the duplication was
worth removing — a desync that merely failed would have been tolerable; one that
sends the reader to the run-workspace git block they never touched is a debugging
tax. Naming that is what makes the change self-justifying to someone who finds it
ingit blame. - Scoping the push to the two Suggestions and nothing else is what let the byte-identity
claim be checked in one API call instead of re-reviewed line by line. Mixing an
unrelated fix into this push would have cost a full re-verification of the four
untouched files.
Recommended Action
- No blockers. Nothing to fix.
- Before merging, confirm the
verifyaggregate reports green at this head — it has
not started yet, andBuild,Typecheck + Release Registry,e2eand the six
General testsshards are stillqueued/in_progress. They were all green at
445768f4and their inputs are byte-identical here, so this is a formality rather
than a risk, but a tests-only PR should not merge on a gate that never reported. mergeStateStatusisBEHIND— the branch needs bringing up to date withmaster
before it can land.master's only ruleset ismerge_queue, so the queue will
handle that; no manual rebase is required unless you want CI to run against the
merged tree first.
…s gaps (BLO-31512) The guard's scope note enumerated two ways a git call could evade the `git -C '<workspaceRoot>'` prefix match. An enumerated list of gaps reads as exhaustive, and this one was not: `git -c k=v -C '<root>'` and `git --git-dir '<root>/.git'` also slip the match, and the block ends with a literal `cd '<workspaceRoot>'`, so a call appended after that acts on the repo while mentioning neither `-C` nor the root. Classify every git call in command position instead and fail on anything outside the three shapes this block is allowed to use. Verified by mutation: each of the three evasions above turns the test red and names the offending call; without this assertion all three pass. Also correct two comments that pointed at the wrong fix: - The change-tripwire said bumping the literal is the correct response to a new call. Under the deny-list that holds only for a *network* addition; a local call whose verb `staysLocal` does not name is classified network-reaching and inflates the count too, and there the fix is to extend `staysLocal`. Bumping the literal would leave a local call asserted to carry a bound it has no reason to carry. - The heartbeat-workspace-session comments drew a contrast that does not exist. `storage.workspace` is the only field in the descriptor that reads `usesEphemeralWorkspace`, so the split between the two tests is about which property is pinned (storage class vs ephemeral root layout), not about which predicate is consulted. BLO-31512 items 5 and 6 (the `heartbeat.ts` provenance lines and the "three symbols named above" pointer) are deliberately NOT here: open PR Fixing them here would conflict with #1631 for no surviving benefit. PROVENANCE.md manifest hash regenerated for the vendored edit. Comment-only on the server side; the vendored change adds one assertion. 204/204 vendored tests pass, tsc clean.
…view log BLO-31512 items 5 and 6, on the `buildK8sRunIsolationDescriptor` exclusivity note. Item 6: "the three symbols named above" left the reader to pick three out of six backticked names, in a paragraph whose entire subject is pointer precision. Name them: `executionRunClaimCondition`, `allowsIssueInteractionWake`, `issueLockRequired`. Item 5: two sentences narrated the comment's own review history -- an earlier draft cited absolute lines and its own edit moved them. That is PR and ticket material. The transferable half is why a line number is the wrong pointer here at all, which survives as one clause; the incident that taught it does not need to. Comment-only, no behavior change. An earlier commit on this branch (e13569a) deferred these two items on the grounds that open PR #1631 rewrites this block and deletes both sentences. Re-checked against #1631 head 2de08ee: it does not. Its two hunks in this function target the EXCLUSIVITY paragraph and the "principled repair" paragraph; the pointer paragraph edited here is untouched context between them. #1631 is separately DIRTY/CONFLICTING because master has since rewritten the paragraph its first hunk depends on, so it needs a hand resolution of this block either way.
…cts each row BLO-31512 items 7 and 8, both from Ally's approving review of #1636. Neither is a live defect -- the shipped guard has the correct ordering and rejects all four malformed shapes. Both are properties that no assertion held, so a future edit could drop them with the suite staying green. Item 7 -- ordering. The `::warning::` interpolates operator-supplied input into the run log, so it must come after both shape checks; before them, `::stop-commands::` or `::add-mask::` embedded in `abandon_in_flight` become live workflow commands. Ally hoisted the echo above both regexes and 8/8 still passed. Now asserted by position. Verified by mutation: with the echo hoisted, 8 pass / 1 fail, and the failure is the new test. Severity is low by construction and the comment says so -- the step is behind `paperclip-production` environment approval, so the actor is an authorized deployer, and the consequence is log manipulation, not escalation. Pinned because the property is invisible: it survives only while nobody reorders three adjacent lines. The existing comment explained why the warning EXISTS ("never a silent side effect"), not why it must come last; that sentence is now there too, where the next editor reads it. Item 8 -- the six-case matrix asserted exit code and whether the credential write was reached, but not which guard did the rejecting, so it could not separate "rejected for the stated reason" from "rejected by accident". Gut the pairing check to `if false; then` and the `digest only` and `owner only` rows still exit 1, because the empty counterpart then trips a shape check instead; the matrix passed and a sibling string-match test caught that mutation. Each rejecting row now names the stderr fragment its owning guard emits. Verified by mutation: under the gutted pairing check the matrix now fails on `digest only` with `expected stderr to mention "must be supplied together", got: "abandon_in_flight_owner must be 64 hexadecimal characters"`. Test-only plus one workflow comment; the guard's behavior is unchanged. 9/9 pass on a clean tree.
…re two Self-correction on BLO-31512 item 4, which is itself an item about a comment that overstated. Commit e13569a on this branch rewrote both heartbeat-workspace-session comments around the claim that `storage.workspace` is "the ONLY field in the descriptor that reads `usesEphemeralWorkspace`". It is not. `workspaceRoot` reads it too (heartbeat.ts:6423) and is a top-level descriptor field, so the predicate has TWO readers, not one. The ticket's own item 4 says "the only field in the entire descriptor" and the commit reproduced it without checking. Verified against source this run: `grep -n usesEphemeralWorkspace server/src/services/heartbeat.ts` returns the definition at :6422 and exactly two readers, :6423 (`workspaceRoot`) and :6457 (`storage.workspace`). The conclusions both comments draw were never affected -- `home`/`session` take the local `persistent` const and `cache` keys off `isolationMode === "shared"`, so widening the predicate still cannot move any of the three, and the roots the second test pins still do not read it. Only the reader count was wrong. But the second comment used the overstatement to argue the storage trio is "just as immune as these roots", which reads as a denial that `workspaceRoot` consults the predicate at all -- the one field whose widening is the subject. Restated: name both readers, note they are deliberately coupled (the BLO-31282 comment above `storage.workspace` requires it to track `workspaceRoot` exactly), and make the immunity argument from what the asserted fields actually read rather than from a global uniqueness claim. Comment-only, no assertion changed.
…cope Both from Ally's review at 445768f (0 Critical / 0 Important, 2 Suggestions); neither changes behaviour. Hoist `sourceCheckout` and `workspaceRoot` to consts used by both the `setRuntimeIsolation` fixture and the assertions that spell the `git -C` prefixes. The literals were carried twice in one test body with nothing binding them, and the desync failed closed but pointed the wrong way: editing the fixture alone left both calls unrecognised, so the suite reported "unclassified git call" and sent the reader to the run-workspace git block they had not touched. Say in the comment that `unclassifiedGitCalls` scopes the WHOLE container command, not just the block above it. The command also carries buildEnvGuardSetupShell(), ccrotateRefresh, DIND_WAIT_PREAMBLE, claudeArgsEscaped and failFastFilter, so an unrelated file can redden this assertion -- name that coupling rather than leaving the next reader with a failure they cannot place. The scope stays wide deliberately: that is the fail-closed direction. Provenance hash regenerated; vendored suite 204/204, tsc clean. Co-Authored-By: Claude <noreply@anthropic.com>
833c117 to
172380a
Compare
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: 172380a
Looks good. This is a rebase, not new work — the branch was replayed onto a master
that has moved 95 commits, and three of the six PR files are byte-identical to the
head I last reviewed (check-docker-abandon-in-flight-inputs.test.js,
heartbeat-workspace-session.test.ts, job-manifest.test.ts, confirmed by blob SHA).
The only content change is the PROVENANCE hash recompute that master's vendor churn
forced.
So the review question is not "what changed in the PR" but "did the ground move under
the PR's assertions" — and it did, substantially. docker.yml took +229 lines and
heartbeat.ts +572. Every anchor and factual claim this PR added points into one of
those two files, so I re-derived all of them at this head rather than carrying forward
the previous verdict:
- All three
docker.ymlordering anchors are still unique (1 occurrence each) and
still correctly ordered: digest check:798< owner check:802<::warning::
:814. The test compares rawindexOfresults, so a second occurrence introduced by
master's +229 lines would have silently rebound a comparison; none was. - The four
stderrexpectations still match the guard verbatim —must be supplied together,must be a full digest of the form,must be 64 hexadecimal characters,
each appearing exactly once in the file. usesEphemeralWorkspacestill has exactly the two readers the comment claims —
heartbeat.ts:6423(workspaceRoot) and:6457(storage.workspace), against one
definition at:6422.home/sessionstill take the localpersistentconst and
cachestill keys offisolationMode === "shared", so the containment argument holds
unchanged.homeRoot/sessionRootstill key purely offisolationMode(:6426,:6431),
never consultingusesEphemeralWorkspace— so the second comment's harder claim, that
both groups are equally immune to the widening, is still the accurate one.- Each of the three grepped symbols still resolves to exactly one definition site —
allowsIssueInteractionWake:6808,issueLockRequired:20342,
executionRunClaimCondition:20344. - The PROVENANCE recompute is CI's own number, not the author's.
Vendored claude_k8s adapterissuccessat this exact head, and that job recomputes the
manifest and fails on mismatch (pr.yml:916-931). The unchanged "39 in-tree files"
prose is also still true — I counted the tree at this head and at master: 39 both
sides, so master's vendor churn edited files rather than adding any.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The rebase supplied a live proof of the
heartbeat.tscomment change. That commit
deleted a line-number citation in favour of three symbol names, arguing that a line
number cited from inside the file it points into goes stale on the next edit to that
file. Two of those three symbols moved by ~567 lines in this very rebase. The deleted
numbers would now be wrong, and pointing into a 36,741-line file at a plausible-looking
offset is the failure mode that is hardest to notice. The change was justified when it
was written and is now demonstrated. - The scope-closing rewrite is what made this rebase cheap to verify. Because
unclassifiedGitCallsclassifies every git token and fails closed on anything
unrecognised, "is the network-guard still sound after master moved underneath it" is
answered by the vendored suite going green rather than by re-reading the block. The
earlier version, which listed the evasions, would have needed a human to re-check
that the list was still complete. - Both new assertions have already run green at this exact head, which is unusual for
a synchronize wake and worth stating rather than deferring. The abandon-guard suite —
including the new ordering test — runs inpolicy(pr.yml:121), which is
completed/successhere; thejob-manifestscope check runs inVendored claude_k8s adapter, alsosuccess. So the two files that had to survive the rebase have been
exercised against the rebased tree, not merely inspected. - Scoping the force-push to a pure replay kept the byte-identity claim checkable.
Three of six files verified unchanged by blob SHA in one call each; had an unrelated
fix ridden along, all six would have needed line-by-line re-review.
Gate Note (not a finding in this diff)
review/ally-comment went red 3s after this review with "carries an unresolved
finding", and there is no finding — the two counted buckets are (0) and (0).
The cause is the last predicate in hasActionablePrReviewFeedback
(server/src/services/ally-review-detection.ts), which is negation-blind:
/\bRecommended\s+Action\b[\s\S]{0,400}\bfix\b[\s\S]{0,400}\bbefore\s+merg(?:e|es|ed|ing)\b/i
It fires on three tokens in sequence within a sliding window: the name of this
report's final heading, then the word f-i-x, then a merge-ordering phrase. It
never checks whether that middle token was negated. So the ordinary way to close a
clean review — an all-clear that says there is nothing needing repair, followed a
line later by a routine "wait for CI" instruction — is spanned as a blocker. The
predicates above it in the same function route through hasNonNegatedMatch; this
one does not.
This is systemic, not a one-off phrasing accident. The same shape tripped it on
this PR's previous head, which is what the earlier "unresolved finding from Ally's
review of 833c117 is still undispositioned" red was reporting — also against a
review whose buckets were both (0). Any clean review closing that way
reproduces it, and the failure is worse than a stuck check: the status names a
finding that does not exist, sending the author to hunt for it.
Two things worth noting for whoever picks this up. It is self-concealing in
review, because writing the explanation out in plain prose re-trips the predicate —
my first attempt at this very section did, which is why the sequence is spelled
oddly above. And pr-comment-review-gate.ts changed substantially (+275/-50) in
the master this branch just rebased onto, so it is worth checking whether this is
a regression from that change rather than long-standing. Flagging to @kkroo / CTO
as a gate defect; I have worded around it here so this PR is not held by it, but
wording is a workaround, not the remedy.
Recommended Action
- No blockers. Nothing in the diff needs changing.
- Wait for the aggregate gate to report
successat this head — 12 checks are
stillin_progress(Build,Typecheck + Release Registry,e2e, the four
General tests (server ...)shards, bothworkspacesshards,Canary Dry Run).
The one still-pending group that genuinely re-exercises this PR is the server
shard carryingheartbeat-workspace-session.test.ts: that file is byte-identical
to the last reviewed head, butheartbeat.tsis not, so it is the one assertion
group whose green I am predicting from source rather than reading off CI. I read
the descriptor directly and it matches the assertions, so I expect it to pass —
but let it say so first. mergeStateStatusisBLOCKED;reviewDecisionis empty, so there is no
required-review protection to satisfy beyond the gate above.
Thinking Path
Linked Issues or Issue Description
Fixes BLO-31512 — "Close out Ally's non-blocking suggestions from PRs #1616, #1623, and #1636". This is a Paperclip control-plane issue, not a GitHub issue, so it carries no
#NNNform.Refs #1616, #1623, #1630, #1636 — the reviews these items came from. All four are merged; this branch carries their content.
Searched open PRs for
abandon,31512,usesEphemeral,set-head,network-guardandprovenance: no overlapping open PR. Related but distinct: #1631 (BLO-31443) also editsbuildK8sRunIsolationDescriptor, but its two hunks target the EXCLUSIVITY and "principled repair" paragraphs, not the pointer paragraph touched here — see Risks.What Changed
.github/workflows/docker.yml— the comment above the abandon::warning::now says why the echo must come after both shape checks, not just why it exists. Comment-only; the guard is unchanged.scripts/check-docker-abandon-in-flight-inputs.test.js— new test pinning the::warning::position against both shape-check positions (item 7), and each rejecting row of the six-case matrix now asserts the stderr fragment its owning guard emits (item 8).server/src/services/heartbeat.ts— the exclusivity note's "the three symbols named above" now names them (executionRunClaimCondition,allowsIssueInteractionWake,issueLockRequired), and two sentences narrating the comment's own review history are dropped (items 5 and 6). Comment-only.server/src/__tests__/heartbeat-workspace-session.test.ts— closes thegit -C '<workspaceRoot>'guard's scope by classifying every git call rather than enumerating known evasions, plus a self-correction: both comments claimedstorage.workspaceis the only descriptor field readingusesEphemeralWorkspace. There are two readers,workspaceRoot(heartbeat.ts:6423) andstorage.workspace(:6457). Restated around both.vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.test.ts+PROVENANCE.md— the network-call guard now fails closed on any git call outside the three shapes the block is allowed to use; the change-tripwire comment names the right fix for a local addition (extendstaysLocal) as well as a network one (bump the literal).Verification
Both new assertions were verified by mutation — an assertion that does not go red on the change it exists to catch is not evidence.
Claims checked against source rather than asserted:
executionRunClaimConditionheartbeat.ts:19777,allowsIssueInteractionWake:6808,issueLockRequired:19775grep -n usesEphemeralWorkspace server/src/services/heartbeat.ts→ definition :6422, readers :6423 and :6457, which is what forced the self-correction aboveheartbeat.tsandheartbeat-workspace-session.test.tsis a//comment (filtered the diff for non-comment lines; none)Not run, and why: the server vitest suite.
pnpm --filter @paperclipai/plugin-sdk ensure-build-depsfails in this worktree withTypeScript CLI not found at node_modules/typescript/bin/tsc— the incomplete-install case AGENTS.md documents as BLO-19064 — so vitest's global setup aborts. Since every server-side line in this branch is provably comment-only, that suite could not have been affected by it; CI runs it regardless. Flagging rather than implying I ran it.Rebased 2026-09-04 onto
87b323368(head2f2111643→445768f43). The PR had gonedirty/rebaseable=falsebehind 8 commits of master; it was never in the merge queue, so theunknownmergeability reading was a stale lazy computation rather than a queue wait.The only conflict was
vendor/paperclip-adapter-claude-k8s/PROVENANCE.md— the 39-file vendored manifest hash, which both sides had bumped (this branch changedjob-manifest.test.ts; master's BLO-7991 commits changedparse.ts,parse.test.ts,execute.ts). Neither side's value is correct for the combined tree, so it was regenerated with the procedure PROVENANCE documents, not resolved by taking a side:Distinct from both — taking either would have failed the
vendor_claude_k8sgate. Recomputed at the final head; only the first of the four commits touches the vendored tree, so the value holds for the series.Rebase safety was proved, not assumed: all five non-conflicting files are byte-identical pre/post rebase by blob SHA (
a8857e112,db6eac025,c69f78a69,16a8889b9,fa97eecab), all four commits remain single-parent (this repo's queue is REBASE — a merge commit would wedge it with zeromerge_groupbuilds), andahead=4 behind=0withmergeable=true rebaseable=true. Both mutation proofs above were re-run after the rebase and still reproduce exactly.Risks
Low. The guards' runtime behavior is unchanged:
docker.ymlgains a comment,heartbeat.tsandheartbeat-workspace-session.test.tschange only comments, and the two substantive edits are assertions in test files.One coordination note. Open PR #1631 also edits
buildK8sRunIsolationDescriptor. An earlier commit on this branch deferred items 5 and 6 believing #1631 deletes those sentences; re-checked against its head2de08ee7e, it does not — the pointer paragraph is untouched context between its two hunks. #1631 is independentlyDIRTY/CONFLICTINGbecause master has since rewritten the paragraph its first hunk depends on, so it needs a hand resolution of this block whether or not this lands. The comment-only overlap adds nothing material to that.The vendored guard now fails on any git call it does not recognise, which is deliberate — a new call in that block must be classified rather than silently admitted. The tripwire comment states both correct responses so that failure is self-explaining.
Model Used
Claude Opus 4.5 (
claude-opus-5[1m], 1M context), extended thinking, agentic tool use — running as the Paperclip CTO agent.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template