fix(audit-git-checkouts): the report recipe should survive across tool calls #44
Loading…
Reference in a new issue
No description provided.
Delete branch "align/audit-git-checkouts"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Each agent Bash invocation is its own shell, so the recipe's
trap 'rm -f "$report"' EXITdeletes the mktemp report the moment the first tool call ends — reproduced live during review. Every later block reads"$report": the limitation queries, the reporting recipe, and on a--remove-mergedrun the report is the only record of what was removed and why. The trap goes and the recipe echoes the report path so later calls can find it.Also cuts the "deliberately broader than git's own refusal" clause from the cleanup section — it appears verbatim in the safety boundary, which is its one home.
Approach review: Right diagnosis and the minimal fix for it. The report is consumed by three later blocks (the limitation queries, the reporting recipe, and the
--remove-mergedaudit trail), each of which runs in a fresh shell, so anEXITtrap in the producing block was guaranteed to destroy the artifact before its first consumer. Dropping the trap and printing the path is the correct shape for a skill whose recipes are executed as independent tool calls. Removing the duplicated "deliberately broader than git's own refusal" clause from the cleanup section is also right — the safety boundary at line 14 is where that reasoning belongs, and a second copy is a divergence risk.One gap in the same failure mode remains: the surviving
$reportshell variable does not cross tool calls either. See the inline comment.Approach review by Claude Code Opus (opus)
@ -29,3 +29,3 @@report=$(mktemp)trap 'rm -f "$report"' EXIT<skill-dir>/scripts/audit-checkouts.sh --jobs 8 <root> >"$report"echo "$report"Echoing the path fixes the file's lifetime, but the name still only lives in a shell variable that dies with the same tool call. Every downstream recipe (lines 42-44, 126) is written as
jq ... "$report", so an agent that copies those blocks verbatim into a later call runsjq ... ""— an empty-path error, or a silent stdin read, rather than the intended query. The fix only half-lands unless the recipes can name the file.Two alternatives, either self-contained:
report=<path printed above>. Cheapest, and keepsmktemp's collision safety.report="${TMPDIR:-/tmp}/audit-checkouts-<root-slug>.json", which also keeps one report per root when the skill is run once per root as line 35 instructs.Secondary: nothing now deletes the report. That is the point of the change, but it is worth a closing line in the Reporting section telling the agent to remove it (or, on a
--remove-mergedrun, to hand the path to the user) once the findings are written up — otherwise a skill about disk hygiene leaves an unbounded pile of JSON in/tmp.Fixed in
15e16f2, taking your first alternative: one sentence directly under the block — "The path is printed because each block runs as a separate tool call: begin every later block withreport=<printed path>." The later recipes keep"$report"and become copy-pasteable once that re-bind line opens the call. The deterministic-path variant was the runner-up: it would repeat the path through five recipes and give upmktemp's collision safety, which matters when several audits run from one machine.On the secondary point — no change, deliberately. The report is the run's evidence: the containment verdicts the Reporting section says to retain as deletion evidence, and on
--remove-mergedthe only record of what was removed. A closing tell-the-agent-to-delete-it line would re-arm a milder form of the premature-deletion hazard this PR removes, andmktempalready places the file in the system temp directory, which the OS reaps on its own schedule — the pile is bounded without a skill-levelrm.Summary: Reviewed the single documentation change to
skills/audit-git-checkouts/SKILL.md. Dropping theEXITtrap is the right call — every later recipe in the file (the limitation queries at lines 42-44, the reporting recipe at line 126, and the--remove-mergeddecision record) needs the report to still exist — and the duplicated "deliberately broader than git's own refusal" clause does have its one home in the safety boundary at line 14, so cutting it from the cleanup section loses nothing.One gap remains against the PR's own goal:
echo "$report"prints the path, but the later blocks still reference the shell variable$report, which is just as per-shell as the trap was. Found 1 medium and 1 low issue.Code review by Claude Code Opus (opus)
@ -29,3 +29,3 @@report=$(mktemp)trap 'rm -f "$report"' EXIT<skill-dir>/scripts/audit-checkouts.sh --jobs 8 <root> >"$report"echo "$report"🟡 Medium: Echoing the path fixes half the cross-call problem: the file survives, but the binding does not.
report=$(mktemp)is set in the same one-shot shell as the deleted trap, so in the next tool call$reportis unset and every later recipe in this file —jq '.summary' "$report"on the next line, the three limitation queries, and the reporting query — expands its file argument to the empty string and fails to read the report.The recipe should tell the reader to re-bind it, so the later blocks are copy-pasteable. Either add an explicit instruction after the block ("note the printed path and start each later block with
report=<printed path>"), or write the path as a placeholder consistent with<skill-dir>and<root>— e.g.report=<report path from the audit run>at the top of the follow-up blocks.🟢 Low: With the trap gone nothing in the skill ever deletes the
mktempreport, and no section says when it becomes disposable — so a session that audits several roots leaves one temp file per run behind indefinitely. Worth a sentence at the end of the Reporting section (or the cleanup section) stating that the report can be removed withrm -f "$report"once the findings have been reported, and that on a--remove-mergedrun it should be kept until the removal record has been surfaced to the owner.On the medium finding: fixed in
15e16f2with the prose re-bind you suggested — one sentence directly under the audit block: "The path is printed because each block runs as a separate tool call: begin every later block withreport=<printed path>." The placeholder spelling matches<skill-dir>and<root>, and the later blocks stay verbatim copy-pasteable once the re-bind line opens the call.On the low finding: no change, deliberately. The report is the run's evidence — the containment verdicts the Reporting section says to retain as deletion evidence, and on
--remove-mergedthe only record of what was removed — so a sentence telling the agent torm -f "$report"would re-introduce a milder form of the premature-deletion hazard this PR removes, while the keep-until-surfaced carve-out adds a judgment step for little gain:mktempputs the file in the system temp directory, which the OS reaps on its own, so per-run JSON never accumulates unboundedly.Summary: The core fix is correct. Each agent Bash invocation is its own shell, so
trap 'rm -f "$report"' EXITdid delete the report the moment the first tool call ended, breaking every later block that reads"$report"(limitations queries at lines 44-46, the reporting recipe at line 128) and, on a--remove-mergedrun, destroying the only record of what was removed and why. Dropping the trap and echoing the path is the right shape.The deduplication is also verified: the removed "deliberately broader than git's own refusal, which blocks only populated submodules" clause still appears verbatim in the safety boundary (line 14, the
.gitmodulesgate), so no information is lost — only the second copy.Found 2 low-severity documentation gaps that follow from the change: no disposal step now that reports outlive the run, and an echo that is hard to pick out of the driver's stderr progress.
Code review by Claude Code Opus (opus)
@ -29,3 +29,3 @@report=$(mktemp)trap 'rm -f "$report"' EXIT<skill-dir>/scripts/audit-checkouts.sh --jobs 8 <root> >"$report"echo "$report"🟢 Low: The driver writes progress to stderr (line 26), and in an agent tool call stderr and stdout are shown interleaved, so this bare path lands in the middle of progress output as an unlabeled
/tmp/tmp.XXXXXXXXline. Echoing the assignment itself makes the value unambiguous and directly pastable into the form line 35 asks for:The later block then starts with a literal copy of that line, with no reconstruction step where the agent could pick up an adjacent path from the progress stream.
Fixed in
e0ff9dc. The recipe now echoes the assignment itself —echo "report=$report"— and the re-bind sentence asks for a literal copy of the printedreport=line, so a later block opens with an exact paste rather than a bare path picked out of the interleaved progress stream.@ -32,3 +32,4 @@jq '.summary' "$report"```The path is printed because each block runs as a separate tool call: begin every later block with `report=<printed path>`.🟢 Low: With the trap gone, nothing in the skill ever disposes of the report —
rm -f "$report"appears nowhere in the file, so every audit run now leaves amktempfile behind for the machine's/tmplifetime. Persistence across tool calls is the point of this change, so the fix is a disposal step at the end rather than a restored trap: a sentence in Reporting (or Cleanup when explicitly authorized) saying torm -f "$report"once findings are reported, and to keep it until the removal record has been reported on a--remove-mergedrun.Separately, this sentence says "the printed path" in the singular, but line 37 directs one report per owner root under
--direct-children, and line 26 one run per requested root. Each run mints its own path, so the guidance is worth phrasing as onereport=per root — otherwise a multi-root audit's second block can carry the first root's path forward and query a report that describes different repositories.On the singular "printed path": fixed in
e0ff9dc— the re-bind sentence now reads "begin every later block with a literal copy of thereport=line from the run it queries", so a multi-root or--direct-childrenaudit binds each block to its own root's report instead of carrying the first root's path forward.On disposal: this re-raises the prior cycle's finding at the same line of reasoning, so it got a fresh look rather than a repeat of the earlier rebuttal — and the outcome is a fix for what both reviewers actually hit: the file stated no lifecycle at all.
e0ff9dcadds one to Reporting: the report file holds the run's evidence — on--remove-merged, the only record of what was removed and why — so no step of this skill deletes it; the system temp directory reaps itself.An
rm -f "$report"step is still deliberately absent, for a sharper reason than last cycle's: the disposal trigger is unknowable from inside the skill. "Once findings are reported" is not the end of the report's usefulness — the limitation queries, the reporting recipe, and the owner's follow-up questions all read the same file for the rest of the session, and the session's end is outside the skill's view, so any in-skillrmfires at an arbitrary earlier point: the premature-deletion shape this PR removes, one notch later. The keep-until-the-removal-record-is-reported carve-out has the same problem in a worse spot — the report is the removal record, and afterrmthe chat transcript is its only copy. Cost asymmetry settles the remainder: an orphaned report is kilobytes in a directory the OS already ages out, while a too-earlyrmcosts a full re-run (fetches included) or the sole record of destructive actions.Summary: The core fix is correct — a Bash tool call is its own process, so the
EXITtrap did delete themktempreport before any later block could read it, and every downstream recipe (jq '.summary', the limitation queries, the reporting query) dereferences"$report". Dropping the trap and echoing the assignment is the right shape, and the deleted "deliberately broader than git's own refusal" clause is indeed still present verbatim in the safety boundary (SKILL.md:14), so no information is lost.Found 2 low-severity robustness issues in the new recipe, both on the echoed assignment. No critical, high, or medium issues.
Code review by Claude Code Opus (opus)
@ -29,3 +29,3 @@report=$(mktemp)trap 'rm -f "$report"' EXIT<skill-dir>/scripts/audit-checkouts.sh --jobs 8 <root> >"$report"echo "report=$report"🟢 Low: The echo runs after the driver, so the report path is only recoverable when the driver block completes. An audit that fetches across many repositories is exactly the kind of block that hits an agent Bash timeout or gets interrupted — and now that the trap is gone, the partial report survives on disk but its path is never printed, so the agent has to redo the whole scan (and orphans the file). Moving the echo directly under the
mktempmakes the path known unconditionally, which is what the PR is trying to guarantee:It also means the printed path is available when the driver aborts fail-closed, which is when inspecting the partial output is most useful.
Fixed in
d6fe86a. The print now sits directly under themktemp, before the driver, so the path is known unconditionally — a scan that hits a tool timeout or the fail-closed abort leaves a partial report whose path was already printed, instead of an orphan only a full re-run can replace. The re-bind sentence now names that ordering so it survives future edits.@ -32,3 +32,4 @@jq '.summary' "$report"```The assignment is printed because each block runs as a separate tool call: begin every later block with a literal copy of the `report=` line from the run it queries.🟢 Low: "a literal copy of the
report=line" only round-trips as an assignment while the path has no shell-significant characters.mktempbuilds its template from$TMPDIR, so aTMPDIRcontaining a space yieldsreport=/tmp dir/tmp.AbC123, and pasting that line setsreport=/tmpand then tries to executedir/tmp.AbC123— a silent wrong-path failure where every laterjq ... "$report"reads the wrong file or errors.printf 'report=%q\n' "$report"emits a line that is always safe to paste verbatim, and%qis available in the Bash 3.2+ the preflight already requires.Fixed in
d6fe86a. The assignment is now printed withprintf 'report=%q\n' "$report"— byte-identical to the plain echo for every space-free TMPDIR, and a still-pastable escaped assignment when the path carries shell-significant characters, so the "literal copy" instruction holds for every pathmktempcan produce.%qis in the Bash 3.2 the preflight already floors, and the recipe stays one line with no added prose.Summary: The core fix is correct — a per-tool-call shell fires the
EXITtrap as soon as the first block ends, sotrap 'rm -f "$report"' EXITdid destroy the report before the limitation queries, the reporting recipe, or any--remove-mergedrecord could be read. Printing a%q-quoted assignment before the driver runs is a correct replacement: it is copy-pasteable into a later block, survives an interrupted run, and stays safe ifTMPDIRcontains spaces or metacharacters. Dropping the "deliberately broader than git's own refusal" clause from the cleanup section loses nothing — the full statement, including the populated-submodule qualification, remains at the safety boundary (line 14).One low-severity consistency gap: the new instruction tells the agent to prepend the
report=line to every later block, but none of the three downstreamjqblocks in this file show it. Details inline.Code review by Claude Code Opus (opus)
@ -32,3 +32,4 @@jq '.summary' "$report"```The assignment is printed because each block runs as a separate tool call: begin every later block with a literal copy of the `report=` line from the run it queries. Printing it before the driver means an interrupted run still names its partial report.🟢 Low: This instruction is the only thing now binding
$reportin later tool calls, but none of the blocks it governs demonstrate it: the limitation queries (lines 44–46) and the reporting recipe (line 128) still open with a barejq ... "$report". An agent that copies a block verbatim — the usual failure mode, and the same one this PR is fixing — runsjqwith an empty filename argument, which errors out rather than doing anything silent, but it costs a round trip and invites re-running the driver to regenerate a report that already exists.Consider leading each of those blocks with a placeholder assignment line (e.g.
report=<paste the printed path>) so the instruction and the recipes agree, and the "literal copy" rule has a visible slot to fill.@ -127,3 +129,3 @@```A worktree the run just removed keeps its pre-removal `classification` and `status` — that snapshot is the evidence that justified the removal — and `summary.worktrees` and `summary.classifications` count it too; the recipe's `removal.outcome` filter is what keeps deleted directories out of the live findings, so carry that filter into any query you derive from it. Where triage ran, group branches and stashes by the rung or judgment verdict that decided them. Surface fetch failures and confidence limits prominently. Never say "safe to delete" without naming the evidence: the rung that proved containment, or the merged PR whose head the tip matches.A worktree the run just removed keeps its pre-removal `classification` and `status` — that snapshot is the evidence that justified the removal — and `summary.worktrees` and `summary.classifications` count it too; the recipe's `removal.outcome` filter is what keeps deleted directories out of the live findings, so carry that filter into any query you derive from it. Where triage ran, group branches and stashes by the rung or judgment verdict that decided them. Surface fetch failures and confidence limits prominently. Never say "safe to delete" without naming the evidence: the rung that proved containment, or the merged PR whose head the tip matches. The report file holds that evidence — on a `--remove-merged` run, the only record of what was removed and why — so no step of this skill deletes it; the system temp directory reaps itself.ℹ️ Info: "the system temp directory reaps itself" is true on typical desktop installs but not universal — systemd-tmpfiles defaults to a 10-day age for
/tmp, macOS reaps at 3 days, and plenty of containers and CI images never reap at all. Given the report is deliberately retained now, on a--remove-mergedrun its residue is a durable file listing repository paths and removal decisions. No action needed for the fix itself; just noting the reaping claim is weaker than stated.