fix(web-api-discovery): move the placement guard to the steps that actually write committable output - #473
Merged
Conversation
…annot strand #300 warned whenever a capture started from the primary checkout on the protected branch, and that was true as written: the default output was the work tree root, so every run left artifacts where commits are blocked. #377 moved the default into the run's own session directory under the gitignored captures root. Location stopped being evidence of harm, but the guard kept asking only about location, so it fired on runs that leave nothing behind. A warning that fires without harm is not free -- it is how the warning gets trained out of an operator's attention, and it was: the reported behaviour is answering yes and going on recording from the protected branch, because the advisory was noise at the only step that raised it. The location question now gains the destination question, asked through classifyDestination() and nowhere else -- the same classifier outputDestinationWarning already uses, so there is no second gitignore answer free to disagree. The decision also gets a single owner. The PowerShell front door used to run the check first and hand the recorder HARCAPTURE_PLACEMENT_GUARD_RAN so the operator was told once; it cannot answer "is this destination gitignored" because it never resolves the destination. The recorder does, so the recorder decides, and the handshake goes with the duplicate check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q4oCSX5MzcTAVKFWzFvZ9x
…mittable output The guard was wired into capture and nowhere else. Capture, since #377, writes its default output to the gitignored session directory and so cannot strand anything -- while the reference extract, the api document, the standalone scrub and the standalone catalogue all take an explicit destination, all produce artifacts meant to be committed, and all said nothing when run from the primary checkout on the protected branch. So the coverage was exactly inverted, and "the developer learns not to run on main" could not happen: the one warning that fired was false, and the four steps that would have taught the lesson were silent. The shared guard gains the destination half in both runtimes -- Get-StrandingPlacement alongside strandingPlacement() -- pinned against each other by a new table of destination shapes, because two implementations of one rule is how #300 arrived. The gitignore question is delegated to classifyDestination() rather than asked again, so there is no second answer free to disagree. The message hands over commands rather than describing the problem. A pre-write guard cannot end in `mv` the way the capture epilogue does, so the actionable fix is the pair: the `git worktree add` line and the same command re-run with its output inside the worktree, both filled in. An interactive operator can accept the offer and have the worktree made and the destination retargeted; a non-interactive one gets the commands and proceeds, because ShouldContinue throws under -NonInteractive and an agent must never be prompted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q4oCSX5MzcTAVKFWzFvZ9x
…ion check Findings from an independent review (Claude Sonnet 5) of the first two commits. `catalogue` still asked only where the operator was standing, so it emitted the relocation notice for its own default output -- the capture's session directory under the gitignored captures root. An operator was told to `mv` an artifact that had never been misplaced, which is the same false warning this issue exists to remove, surviving in the one command the first pass did not touch. Both commands now go through placementForOutput, which exempts the captures root structurally and asks the shared guard about everything else. The exemption is deliberate rather than a shortcut: the root IS gitignored, but this tool is what makes it so and that happens after the guard, which must fire before anything is recorded -- so asking git during the window in between answers "not ignored" for a directory about to be ignored. placementForRun, which had been a second answer to the same question, is gone. Also from the review: - The PowerShell ignore probe ran against the ambient environment while classifyDestination() strips the whole GIT_* namespace and pins HOME. A forged core.excludesFile could therefore suppress the PowerShell warning and not the Node one -- two implementations of one rule disagreeing, in the dangerous direction. Both now probe the same stripped environment, and the agreement table has a forged-config case so the drift cannot return silently. - Invoke-HarCatalogue classified the output DIRECTORY, against the rule the rest of the guard follows: a consumer's .gitignore names these artifacts, so the question has to be asked about the file that will be written. - The prefix comparison in New-GuardWorktree and Get-GuardCommandPath required a separator after the base, so a destination that WAS the checkout root compared as outside it. Both now share Get-GuardRelativePath, where the equality case is an empty relative path rather than a miss. One finding was rejected: the review held that a missing git leaves a stale $LASTEXITCODE because the error is non-terminating. Measured, it is terminating -- `& nosuchcmd` raises CommandNotFoundException -- so the try/catch already returns unverifiable, and the comment now records that this was verified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q4oCSX5MzcTAVKFWzFvZ9x
… on a throw Push-GuardProbeEnvironment published its saved copy only after it had finished mutating, and Pop ran from a finally nested INSIDE the try that Push sat in. So a throw part-way through the push -- a provider error on one Remove-Item -- left the process with a stripped GIT_* namespace and a HOME pointing at a directory that does not exist, and nothing to put back: every later git call in the session would read no user configuration. The save is now published before the first mutation and Pop runs from an outer finally, so an interrupted push is still undone. Nesting throws rather than silently overwriting the outer save, which would have made the outer pop restore the placeholder instead of the operator's real environment. Nothing recurses today; the guard is what keeps that true. Also: Get-GuardCommandPath returned '' when the target IS the base -- a correct relative path and an unusable command argument, since it renders as an empty token. It says '.' now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q4oCSX5MzcTAVKFWzFvZ9x
… silencing itself Second pass from the independent reviewer (Claude Sonnet 5), which confirmed the five earlier fixes and raised four smaller things. The bug this branch closes had no regression test at the level it occurred: every placement test exercised `start`, and `catalogue` -- the command that actually carried it -- had none. Two now cover both directions, and they were confirmed RED against the previous commit's capture-har.js before landing, so they pin the behaviour rather than describing it. The re-entrancy check added last commit threw from inside the try whose catch means "git is missing", so the one failure it exists to make loud was reported as git being unavailable and quietly downgraded to unverifiable. Push moves outside that try; it publishes its saved copy before mutating, so the finally still restores an interrupted push. Pop conflated "absent" with "present but empty" and deleted a variable that existed, in the function whose entire job is exact restoration. It compares against $null now. The captures-root exemption comment claimed to be exhaustive. It is, for `start`; `catalogue` can be handed a session found under the legacy root, which falls through to git's own answer -- correct either way, but the comment now says which it is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q4oCSX5MzcTAVKFWzFvZ9x
…re-entry Caught by the independent reviewer (Claude Sonnet 5) on its final pass: the previous commit fixed one fault by reintroducing another. Moving the whole push outside the try did make a re-entrant call throw where the caller can see it, instead of being caught by the block that means "git is missing" and downgraded to unverifiable. But the finally went with the try, so a push that failed AFTER publishing its saved copy propagated with no pop at all -- leaving the environment stripped, HOME pointed at a directory that does not exist, and the slot permanently occupied, so every later probe in the process would report a re-entry that never happened. That is worse than the mislabelled warning it replaced, and the comment claiming the finally still covered it was simply wrong. The two goals are separable, because only one of the two steps mutates anything. The re-entrancy ASSERTION is raised before the try, where nothing can have changed yet and the error reaches the caller. The MUTATION stays inside it, so a push that fails part-way is still popped. Test-GuardProbeActive is the shared predicate, so the condition is not spelled twice. Four tests now cover what nothing exercised: the environment comes back on the success path and on the git-cannot-run path, a variable that was set but empty is restored rather than deleted, and a nested probe refuses without disturbing the outer push. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q4oCSX5MzcTAVKFWzFvZ9x
MarkMichaelis
force-pushed
the
fix/471-relocate-placement-guard
branch
from
September 11, 2026 09:23
5dd37bb to
e3e25b9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #471
The problem
The placement guard warned on the one step that can no longer pollute a work tree, and
was absent from the four that can.
Invoke-HarCapture.ps1warned whenever a capture started from the primary checkout on theprotected branch. That was true as written when #300 added it — the default output was
then the work tree root. Two later changes moved it:
.har-captures/,gitignored by construction (
capture-har.js: with no--output-path,outputPath === sessionDir)..har-captures/to the main working tree, never a linked worktree,because
git worktree removehad destroyed a 71 MB capture.So a default capture from
mainwrites nothing into the work tree — but the guard firedanyway, because
ShouldWarnasked only about location and never about whether the runwould write anything committable. Its sibling
outputDestinationWarninghad already gotthis right: "the default, which is never classified because it cannot be the problem".
Meanwhile the coverage was exactly inverted:
Invoke-HarCapture(default)Invoke-HarCapture(explicit-OutputPath)extract-har-reference.js --outgenerate-api-document.jsInvoke-SanitizeHar.ps1 -OutputHarInvoke-HarCatalogue.ps1 -OutputPathA warning that fires without harm is not free — it is how the warning gets trained out of
an operator's attention, which is what happened: the reported behaviour was answering
yand going on recording from
main.RepoWorkflowGuard.ps1predicted it in its owndocstring: "noise is how a warning gets trained out of an operator's attention."
What changed
1. The guard asks about the destination, not only the location.
placementForOutput()is the single entry point for bothstartandcatalogue. Itexempts the captures root structurally and hands everything else to
classifyDestination()— the same classifieroutputDestinationWarninguses, so there isno second gitignore answer free to disagree.
The exemption is structural rather than a gitignore question for a reason worth stating:
the captures root is gitignored, but this tool is what makes it so, and that happens
after the guard — which must fire before anything is recorded. Asking git in the window
between the two answers "not ignored" for a directory about to be ignored, which is the
false warning this PR exists to remove.
The decision also gets a single owner. The PowerShell front door used to run the check and
hand the recorder
HARCAPTURE_PLACEMENT_GUARD_RANso the operator was told once; it cannotanswer "is this destination gitignored" because it never resolves the destination. The
recorder does, so the recorder decides, and the handshake goes with the duplicate check.
2. The four steps that write committable output are guarded, before they write.
#300's load-bearing invariant is preserved — the guard runs before any work begins, sonothing that cost the operator effort exists when it fires.
3. The message hands over commands rather than describing the problem.
A pre-write guard cannot end in
mvthe way the capture epilogue does, so the actionablefix is the pair — filled in, not templated:
4. Interactive operators can accept the offer.
Accepting creates the worktree and retargets the output into it. Non-interactive callers
get the commands and proceed —
ShouldContinuethrows under-NonInteractive, and anagent must never be prompted.
Not in scope
text, so the Adding Command-Line Options — Prompt First rule is not engaged.
deletion, and no issue number exists at capture time to name a branch with.
concern 4 — "a placement design that is correct by construction may make the guard
unnecessary, or reduce it to a much narrower check" — which fix(web-api-discovery): scrub output lands untracked at the repo root, is unstamped, and leaves no link back to its raw #377 made true. [NEEDS DISCUSSION - do not start] web-api-discovery: capture output placement is decided by git, and raw vs scrubbed follow opposite rules #361 stays
open for the rest.
Independent review
Reviewed by Claude Sonnet 5 (authoring model: Opus 5), two full passes plus a
confirmation pass.
Pass 1 raised five findings; four were accepted and fixed, one rejected with evidence:
cataloguekept the pre-web-api-discovery: the placement guard warns on the one step that cannot pollute, and is absent from the four that can #471 location-only check. Accepted — it emitted therelocation notice for its own gitignored default, telling operators to
mva file thathad never been misplaced. The exact bug this PR closes, surviving in the one command the
first pass missed. Both commands now go through a single
placementForOutput.core.excludesFile. Accepted — the JSclassifier strips the whole
GIT_*namespace and pinsHOME; the PowerShell probe didnot, so an injected
core.excludesFilecould suppress the PowerShell warning and not theNode one. Both now probe the same stripped environment, with a forged-config case in the
cross-runtime agreement table.
Invoke-HarCatalogueclassified the directory, not the file. Accepted.gitleaves a stale$LASTEXITCODE." Rejected — measured:& nosuchcmdraises a terminatingCommandNotFoundException, so the existingtry/catch already returns
unverifiable.Pass 2 confirmed all five and found four Low/Nit items, since fixed: the re-entrancy guard
threw from inside the catch that means "git missing" (silencing the one failure it exists
to make loud); the
cataloguefix had no regression test at the level it occurred;Popconflated "absent" with "present but empty"; and one comment overclaimed.
Pass 3 caught a regression introduced by the fix to the first of those, and it was right
to. Hoisting the environment push out of the
tryto make a re-entrant call throw wherethe caller can see it also hoisted it out of the
finally: a push failing afterpublishing its saved copy would then leave
HOMEpointing at a directory that does notexist and the slot permanently occupied, so every later probe in the process would report
a re-entry that never happened — worse than the mislabelled warning it replaced. The two
goals are separable because only one of the two steps mutates: the assertion is raised
before the
try, the mutation stays inside it. Four tests now cover the paths nothingexercised.
Pass 4 confirmed the fix and reported no Critical or Important issues.
Known gap, accepted: no test forces a failure inside the push itself, as opposed to
in the git call after it. The path is covered structurally and by trace; pinning it would
mean mocking
Remove-Item, which buys less than it costs. Recorded rather than paperedover.
Verification
CI is billing-blocked, so everything was run locally.
Both sets of new tests were confirmed Red against the pre-change code before the fix
landed — the
starttests againstcapture-har.jsatmain, thecataloguetestsagainst the previous commit — so neither is vacuously green.
Functional check against a real throwaway checkout on
main(not a fixture):Accepting the offer was exercised directly:
Environment restoration is pinned by test on both paths: a pre-existing
GIT_CONFIG_COUNTand
HOMEsurvive a successful probe and one where git cannot run at all, anempty-string variable is restored rather than deleted, and a nested probe refuses without
disturbing the outer push.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Q4oCSX5MzcTAVKFWzFvZ9x