git-cleanup: convert the skill to a command driving a dynamic workflow - #217
git-cleanup: convert the skill to a command driving a dynamic workflow#217frabert wants to merge 14 commits into
Conversation
f800e0a to
72b09fa
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Review — PR #217 (git-cleanup: skill to command + dynamic workflow)Read the workflow, command, JS suite, all of P2 — P3 — P3 — P3 — P3 — P3 — P4 — P4 — P4 — P4 — P4 — P4 — P4 — P4 — P4 — PR body numbers are stale: "37 assertions" (file sets 61) and "1.0.1 to 2.0.0" (manifests say 2.3.0). Correct in the body: the eleven-agent ceiling and the 71-to-82 floor. Checked and cleanFail-open paths in the refutation gate ( |
|
Thanks — worked through all of it in b6aaee2. Every P2 and P3 was real and is fixed. P2 — deletion order. Right, and the README transcript was reporting an outcome that command sequence cannot produce. Gate 2, phase 3, and the transcript now put P2 — unbounded unit size. Fixed with P2 — refuter asked the wrong question. The clearest of the three. The refuter now tests each claim against what the claim named — the default branch for a PR or commit, the superseding branch for a supersession — instead of always the default branch. P3 — P3 — suite printed "passed" on a failing run, and P3 — P3 — Version. Agreed, and taken to 2.0.0. Also fixed from the P4 list: Two things deliberately not changed:
One note on history: the branch was rebased onto #216 and force-pushed while I was working. I replayed my commit onto the new base with |
hbrodin
left a comment
hbrodin
left a comment
There was a problem hiding this comment.
Review of the analysis path, with the findings I could reproduce by execution. Ran the JS suite (37/37) and the eval self-test (39/39) green on node v24 before starting.
Two I'd treat as blocking, both reproduced:
git branch -daccepts branches whose work is not in the default branch, so the one delete category that skips refutation has an unsound backstop (analyze-branches.js:282).staging,production,dev, andhotfix/*reach the delete list, with force-delete and an emptyneedsReviewon the remote-gone path (analyze-branches.js:17).
Four worth fixing before merge: the quoting guidance on the agent-facing path, the gate-1 audit rule rejecting the workflow's own SAFE_TO_DELETE evidence string, worktreePath being optional but load-bearing, and the loadability check now passing vacuously.
Three more on subagent tool constraints, unbounded context in the investigator prompt, and an unstated pipeline() ordering dependency.
Some things I checked that came back clean, so they don't appear as comments: the refuter prompt not being batch-scoped has no downstream effect (assembly drops out-of-batch verdicts correctly); the decide-side prompt cap works as intended at 30 branches per prompt for a 150-branch repo; and a dirty worktree can't actually be removed out from under the user — git refuses without --force, and branch -d/-D both refuse while a worktree holds the branch. I also measured the fleet cost: a repo with a dozen branches spawns three agents, not eleven, because the deterministic triage decides the rest without spawning anything.
The safety architecture — deletions in the main session, adversarial refutation, triage in JS instead of prose — is the right shape and holds up. The findings below are about places where the verification does less than the surrounding comments claim.
| // Branches that must never be analyzed, recommended, or deleted. Filtered here in | ||
| // JavaScript rather than in an agent prompt: an agent can be talked out of a rule, | ||
| // a regex cannot. | ||
| const PROTECTED = /^(main|master|develop|release\/.*)$/ |
There was a problem hiding this comment.
Blocker — staging, production, dev, and hotfix/* all reach the delete list.
Ran the workflow with stubbed agents against a survey containing those four names:
merged path -> [["staging","git branch -d"],["production","git branch -d"],
["dev","git branch -d"],["hotfix/urgent","git branch -d"]]
remote-gone path -> [["staging","git branch -D"],["dev","git branch -D"],
["production","git branch -D"],["hotfix/urgent","git branch -D"]]
remote-gone needsReview -> []
The remote-gone path yields force-delete for all four, with an empty needsReview.
The defaultBranch/currentBranch additions genuinely fixed the trunk case — that was a good catch. But the literal list here is thinner than the prose around it suggests, and these are exactly the long-lived names whose remote gets deleted during a branch-protection change or a repo migration.
There was a problem hiding this comment.
Fixed in 906aad3. Your remote-gone reproduction is now a committed test case (case 12), covering both paths and Staging for the case-sensitivity hole. The list now covers long-lived integration and environment names — staging, production, prod, preprod, dev, qa, uat, integration, next, canary, stable, plus release/*, hotfix/*, support/*, maint*/* — and matches case-insensitively. Both of your reproductions return empty now.
|
|
||
| The workflow reports evidence so you can audit it, not so you can forward it unread. Before building the gate-1 table: | ||
|
|
||
| 1. **Every `deleteCandidate` names specific evidence** — a PR number, a commit sha, or a superseding branch. "Similar name", "looks stale", or an empty evidence string is not a delete recommendation. Move it to needs-review. |
There was a problem hiding this comment.
This audit rule rejects the workflow's own output.
Every SAFE_TO_DELETE candidate carries exactly this evidence string, from workflows/analyze-branches.js:282:
"reported merged into main; git branch -d re-checks"
No PR number, no commit sha, no superseding branch. Applied literally, this rule moves every git-proven merged branch to needs-review — the one category where the evidence is strongest.
Either exempt SAFE_TO_DELETE here explicitly, or have the workflow name the merge commit in that evidence string so it satisfies the rule on its own terms.
There was a problem hiding this comment.
Fixed in 906aad3. Rule 1 now says SAFE_TO_DELETE satisfies it by naming the tip commit, and the evidence string does name it (tip abc1234 reported by git branch --merged as an ancestor of main). So the rule is satisfied on its own terms rather than by an exemption. Rule 2 also stopped naming a four-entry list that no longer matches the regex.
| }, | ||
| uniqueCommits: { type: 'integer', description: 'count of git log <default>..<branch>' }, | ||
| lastCommit: { type: 'string', description: 'short sha and subject' }, | ||
| worktreePath: { type: 'string', description: 'checkout path, or "" when none' }, |
There was a problem hiding this comment.
worktreePath is optional here — it's absent from the required list above — but load-bearing downstream: commands/git-cleanup.md:157 tells the model to order git worktree remove before the branch delete using this field.
A schema-conformant survey that omits it produces:
deleteCandidates: [{"branch":"feature/auth", ..., "worktreePath":""}]
worktrees: [{"path":"/wt/auth","branch":"feature/auth","stale":true}]
The ordering is then silently skipped and the branch delete fails with used by worktree at .... The command handles that failure gracefully, so this degrades rather than breaks — but it degrades for exactly the case the analysis flagged.
The authoritative source is the required worktrees[] array, which report() already joins against at line 541. Deriving the ordering there removes the dependency on an optional field.
There was a problem hiding this comment.
Fixed in 906aad3. report() now builds a shortRef(branch) -> path map from the required worktrees[] array and prefers it, falling back to the optional field only if the join misses. A schema-conformant survey that omits worktreePath now still gets the ordering right, which case 14 pins.
| const pending = new Set(ambiguous.map((b) => b.name)) | ||
| const units = clusters.flatMap((c) => { | ||
| const decide = c.filter((b) => pending.has(b.name)) | ||
| const context = c.filter((b) => !pending.has(b.name)) |
There was a problem hiding this comment.
MAX_BRANCHES_PER_UNIT caps decide at 10, but context is uncapped and gets replicated into every slice below (line 322).
Measured with 300 settled siblings and 10 ambiguous branches in one cluster:
decideRefsPerPrompt: [10]
contextRefsPerPrompt: [600]
promptChars: [40872]
~41 KB of prompt to decide ten branches, ~98% of it context.
For balance: the decide side is fine. I measured 150 branches -> 5 investigators -> 30 branches per prompt, so that cap does what the comment at lines 28-33 intends. It's only the context list that grows without bound.
There was a problem hiding this comment.
Fixed in 906aad3. Context is capped at 8 per unit and ranked so tracked siblings come first — a supersession claim needs the branch that is still live, and an unbounded tail of stale local leftovers was the part that multiplied across slices. Thanks for measuring the decide side separately; that is what told me the cap belonged only on context.
| .filter(Boolean) | ||
| .join('\n') | ||
|
|
||
| const READ_ONLY = [ |
There was a problem hiding this comment.
READ_ONLY is a prose constraint, and none of the three agent() calls (lines 215, 380, 411) pass agentType — so nothing restricts these subagents' tools, and they must hold Bash to run git at all.
Three attacker-influenceable strings are interpolated verbatim into those prompts:
| Source | Site |
|---|---|
mergeLog (default-branch commit subjects) |
lines 341-345 |
lastCommit subject + branch names |
lines 361-362 |
investigator evidence -> refuter prompt |
line 405 |
In fairness, this is not a new exposure: the deleted skill also read git log --oneline output into context (old SKILL.md:67,70,100). What changed is where it lands. The old skill's allowed-tools was Bash Read Grep AskUserQuestion — one foreground session, under the user's permission prompts. That content now reaches background agents the user never sees.
opts.agentType with a read-only tool set is a cheap mitigation and fits the file's own argument at lines 14-17 ("an agent can be talked out of a rule, a regex cannot"). Delimiting the untrusted fields as data would close most of the rest.
There was a problem hiding this comment.
Partly fixed in 906aad3, and you have the shape of it right. Untrusted spans — the merge log, the branch/commit lines, and the investigator's evidence field flowing into the refuter — are now fenced in <repo-data> with an explicit boundary saying the contents are data, cannot lift the read-only constraint or claim a step was already done, and that text reading like an instruction is itself a finding to report.
I did not add agentType. The workflow's agent() takes no tool list, so restricting tools means shipping a plugin agent and depending on <plugin>:<agent> resolving inside a workflow subagent — which I cannot test from here, and an unresolvable agentType fails the whole run rather than degrading. I would rather not trade a working analysis for an untested restriction on this PR. Your framing of the real change — same content, but now reaching background agents the user never sees — is the right one, and worth its own change once the resolution behaviour is confirmed.
| const isRefuted = (x) => x.refuted !== false | ||
|
|
||
| results.forEach((r, i) => { | ||
| const batchBranches = new Map(batches[i].flatMap((u) => u.decide).map((b) => [b.name, b])) |
There was a problem hiding this comment.
This maps results[i] back to batches[i], so the correctness of the whole assembly step rests on pipeline() returning index-aligned results — an invariant the script never states.
With a deliberately misaligned stub:
ordered : del=[aa/1..ii/9] (9), unanalyzed=[]
misaligned: del=[bb/2,ee/5,hh/8] (3), unanalyzed=[aa/1,cc/3,dd/4,ff/6,gg/7,ii/9]
It fails safe — the mismatched branches drop to unanalyzed rather than into the delete list — which is the right direction and consistent with the rest of the file's design. Worth a comment recording the dependency, since a future harness change would degrade coverage silently rather than loudly.
There was a problem hiding this comment.
Fixed in 906aad3 — recorded as a DEPENDENCY comment at the results.forEach, including your finding that misalignment degrades to unanalyzed rather than into the delete list. Agreed that is the right direction; the risk was that it degrades silently, so the note says so explicitly.
| @@ -1,360 +0,0 @@ | |||
| --- | |||
There was a problem hiding this comment.
Removing the last skill leaves the plugin's only entry point unverified by anything in CI. The PR notes the loadability half; the other half makes it sharper:
.github/scripts/check_codex_loadability.py:230-254countsskills/**/SKILL.mdand compares against loaded skills, so zero skills passes vacuously at0 == 0..github/scripts/validate_plugin_metadata.pycontains no references tocommandsat all — the command's frontmatter is validated by nothing.git-cleanupis now the only plugin in the repo with commands and no skills, so this isn't an established pattern being followed.
Between those two, nothing checks that commands/git-cleanup.md parses or loads under any CLI, and it's the sole way to invoke the plugin. Agreed the gap is in the checkers rather than this plugin — but I'd rather the loadability check fail on a plugin with no verifiable entry point than pass on zero, and that's a small change worth landing alongside this.
There was a problem hiding this comment.
Fixed in 906aad3, in the checkers rather than by restoring the skill — the point of the PR is that workflow-shaped work should be a workflow, so a skill kept purely as a loadability anchor would undo it. Two changes to validate_plugin_metadata.py:
validate_command_frontmatter— command files must have parseable frontmatter, adescription:, andallowed-tools:rather thantools:. You are right that the validator contained no references tocommandsat all.validate_entry_points— a plugin exposing no skills, commands, agents, hooks, or.mcp.jsonis now an error, so a plugin that ships nothing runnable cannot pass at0 == 0.
Six self-test assertions cover both, including the two negative directions that matter: a valid command is accepted, and commands alone satisfy the entry-point rule. Neither check fires on any existing plugin. This does not make Codex load commands/ — that is still a real gap — but the plugin's entry point is no longer unvalidated by everything.
|
@hbrodin — thanks, this was a genuinely useful review. All nine are addressed in 906aad3, and I've replied in each inline thread with the specifics; this is the summary in one place since those replies collapse. Both blockers were right. The
The four before-merge items are all fixed: single-quote guidance mirrored into the two agent-facing copies (with the On the last of those — I fixed it in One I only partly did, flagged so it isn't mistaken for done: the subagent tool constraint. Untrusted spans — merge log, branch and commit lines, and the investigator's Two notes back, on things from your clean list:
Suite is at 44 assertions, all green, and the two blocker reproductions are committed as cases rather than left as prose. I've left your threads unresolved for you to close as you see fit. |
|
need the linter regression fixed, and an approval from @hbrodin |
cc34a57 to
f9df87f
Compare
f9df87f to
d79f6e3
Compare
Replace the prose SKILL.md with a `/git-cleanup` slash command plus a JavaScript dynamic workflow that fans branch analysis out across subagents. The split is the safety property, not an implementation detail. The workflow is read-only: it surveys git state, triages everything git already answers in plain JS, sends only the genuinely ambiguous branches to batched investigators, and puts every delete candidate in front of a skeptic asked to find a commit that is NOT in the default branch. Both user gates and every `git branch -d/-D` and `git worktree remove` stay in the main session, because subagents run in the background and cannot ask the user anything. Uncertainty resolves toward keeping a branch throughout: a refutation missing its `refuted` field, duplicate refutations, a dead agent, and a missing verdict all downgrade to needs-review rather than to a delete recommendation. A wrong keep costs another look at a branch list; a wrong delete costs work that exists nowhere else. Also adds a `js-tests` make target and CI job. Both carry the same zero-discovery guard as `python-tests` — an empty glob fails rather than reporting a pass — because a suite asserting that a branch-deleting workflow fails closed is worse than useless if nothing runs it. The plugin no longer ships a skill, so it loses its Codex presentation sidecar (`agents/openai.yaml` and the brand mark); that metadata only attaches to skills in this repo. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
zizmor's artipacked audit flagged it. Every other checkout in this workflow already opts out; the new job was copied without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Correctness: - Split oversized clusters (MAX_BRANCHES_PER_UNIT). Clustering is transitive on a two-segment match, so 150 dependabot/npm_and_yarn/* branches collapsed into one unit handed to a single agent, with MAX_INVESTIGATORS providing no relief. - Scope the refuter to what each claim actually asserts. It only ever checked the default branch, so a SUPERSEDED claim citing an unmerged sibling was always refuted — one of the two documented evidence paths could never survive. - Permit `fetch --prune` explicitly in READ_ONLY. The constraint listed inspect-only commands and the next line ordered a fetch; an agent resolving that in favour of the constraint sees no `[gone]` branches and reports a clean repo. - Gate 2 and phase 3 now remove a worktree before deleting the branch it holds. Git refuses to delete a checked-out branch, so the previous order failed for exactly the case the workflow computes `stale` for. - Keep the inline evidence standard unconditional. `pluginDir` is model-substituted and can arrive wrong rather than empty, in which case the Read failed and the agent proceeded with no standard at all. Test integrity: - The suite tracked assertions run but not assertions failed, so a failing run still printed "37 assertions passed" as its last line — the only line visible in a collapsed CI group. - js-tests now checks execution, not just discovery: `node <file>` exits 0 on a file that asserted nothing, the same shape python-tests moved away from. Each suite must print a `<n> assertions passed` line with n > 0. Also: 2.0.0, not 1.1.0 — deleting the skill is a capability removal, and anyone loading this plugin for its skill gets nothing after the update. Document node as a `make check` prerequisite. Specify unpushedCommits for a gone upstream and the 40-entry mergeLog window. Fix a comment describing a `|| echo main` fallback the code no longer uses, and meta.whenToUse still naming the deleted skill. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The existing js suite stubs every agent and tests the triage logic in
analyze-branches.js. Nothing covered the part that can destroy work: the
model reading a real repository and deciding what to recommend.
This adds seven cases (five positive, two negative), each run twice --
once with the plugin loaded and once without -- grading the GATE 1
analysis. No eval harness existed in this repo, so this establishes the
convention as well as the suite.
Two make targets, split by cost:
eval-selftest free, no API calls, part of `make check`
evals the real suite, opt-in only
That split is the point. The paid suite runs rarely, so the cheap proof
that the graders still fire runs on every commit -- a grader whose
pattern silently stopped matching would otherwise report a clean bill of
health indefinitely.
Graders read two surfaces that are never interchangeable: executed tool
calls answer "did it delete anything", response prose answers "what did
it propose". Conflating them scores intentions instead of outcomes.
Findings from the first full run, recorded in evals/README.md so they are
not rediscovered:
- Never regex a command string in prose. A regex cannot tell a
recommendation from a mention. Three of four regex_absent graders
failed correct responses -- conditionals ("if you confirm this is
abandoned, I'd run ..."), explicit refusals, and worked examples
answering the question asked. One briefly produced a headline "+0.20
uplift" that was pure artifact. One regex grader remains, on headings.
- Never grade a gate-2 artifact. The command prints literal delete
commands only after the user answers gate 1, which never happens
headless. A grader looking for them failed the plugin for following
its own safety protocol while the unaided arm "passed".
- Scores are locale-sensitive: awk honours LC_NUMERIC and emits "8,00"
under it_IT, which the delta column then subtracts as strings.
Results: 6 of 7 cases show delta 0.00 -- Opus handles the analysis
correctly unaided. The one case that discriminates is 06, where the
unaided arm executed `git branch -d fix/typo` on a bare "tidy it up"
request and destroyed the branch (delta +0.75, verified against repo
state and the tool-call log, not prose).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Lint job failed on plugins/git-cleanup/evals: shellcheck could not follow either `source` directive. A relative `source=` is resolved against shellcheck's working directory, not the script's. Running `shellcheck -x plugins/.../run-evals.sh` from the repo root therefore looks for ./lib/graders.sh and does not find it. `source-path=SCRIPTDIR` anchors it to the script's own directory, which is what the path was relative to all along. The reason this passed locally is the more useful half. The `shell` target ran shellcheck with --severity=warning; SC1091 is info-level, so the filter hid it. The pre-commit hook CI runs is plain `shellcheck -x` with no filter, so `make check` could not catch this class of failure at all -- contradicting the promise at the top of the Makefile that every target mirrors a CI job. Dropped the filter so the two match. The repo is already clean under the stricter args, so this costs nothing today and closes the gap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Blockers: - `git branch -d` is not the backstop the SAFE_TO_DELETE comment claimed. It accepts a branch merged into HEAD *or* into its own upstream, so a branch level with its remote but never merged to the default branch deletes cleanly under -d. That was the only delete category with nothing behind it. Each candidate now carries a `verifyWith` — `git merge-base --is-ancestor <tip> <default>` — that the main session runs immediately before the delete, and the evidence names the tip commit so the claim is checkable rather than asserted. - PROTECTED covered four names. `staging`, `production`, `dev` and `hotfix/*` all reached the delete list, with force-delete and an empty needsReview on the remote-gone path — which is precisely how those branches fail, their remote being deleted during a branch-protection change or a repo migration. The list now covers long-lived integration and environment branches and matches case-insensitively. Also: - Quoting guidance on the agent-facing path said `"$branch"`, under which `$(...)` still substitutes. Both copies the subagents read now require single quotes, with the `'\''` escape, since `has'quote` is a legal branch name and the agents paste literal names rather than expanding a variable. - The gate-1 audit rule rejected the workflow's own SAFE_TO_DELETE evidence string, which would have moved every git-proven merged branch to needs-review. - `worktreePath` is optional in the schema but load-bearing for delete ordering. The join is now derived from the required `worktrees[]` array. - The investigator's context list was uncapped and replicated into every slice of a split cluster: 300 siblings produced a 41 KB prompt that was 98% context. Capped at 8, ranked to keep tracked siblings, since those are the plausible superseders. - Untrusted repo text — branch names, commit subjects, and the investigator's own evidence field — is now fenced in `<repo-data>` with an explicit data boundary. `agent()` takes no tool list and the agents need Bash for git, so the tool-level restriction is not available from here; the boundary is stated instead. - Recorded the `pipeline()` index-alignment dependency the assembly step rests on. Checkers, so a commands-only plugin is not unverified: - The validator now checks command frontmatter (parses, has a description, uses `allowed-tools:` not `tools:`) — it previously had no references to commands at all. - A plugin exposing no entry point at all is now an error, so the loadability checks cannot pass vacuously at 0 == 0 on a plugin that ships nothing runnable. - Six new self-test assertions cover both, including that a valid command is accepted and that commands alone satisfy the entry-point rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
hbrodin measured a dozen branches spawning three agents, because the deterministic triage decides most of them without spawning anything. Eleven was the worst case presented as the headline number. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The execution guard demanded a `<n> assertions passed` line, which is git-cleanup's own convention. semgrep-rule-variant-creator's suites use node:test and report `<mark> pass <n>`, so the guard failed two honest suites for using the other format — a guard that only knew the format of the suite it shipped with. Both formats now count. The node:test branch does not anchor on `^.`: that mark is multi-byte, the recipe runs under /bin/sh in whatever locale the machine has, and `.` matches a single byte in the C locale — which matched interactively and failed under make. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d79f6e3 to
663578e
Compare
Three real conflicts, all from both sides growing the same scaffolding: - Makefile: main added `eval-self-tests` (discovers any evals harness that advertises `--self-test`); this branch added `eval-selftest` (hardcoded to `evals/selftest/run-selftest.sh`). Kept main's discovery-based target and dropped the branch's, then taught git-cleanup's `run-evals.sh` to accept `--self-test` so the discovery finds it. The branch's `js-tests` and `evals` targets carry over unchanged. - validator: both sides added a checker in the same place. Kept all three — `validate_command_frontmatter`, `validate_skill_frontmatter`, and `validate_entry_points` — and wired all three into the scan. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#253 landed its own `validate_command_frontmatter`, so the two versions of that checker had to be reconciled rather than both kept: - Took main's constants block (it carries the new `HARDCODED_PATH_PATTERN`) and its stricter `command_files`, kept this branch's `validate_entry_points`, and adopted main's three-argument `validate_subagent_dispatch`. - main's command fixture asserted that a command with `name` and `allowed-tools` is accepted; this branch's checker also requires `description:`, which a slash command needs to appear usefully in the command list. Gave the fixture a description and added the two missing-case assertions the merge had dropped. - `SELF_TEST_MINIMUM` is exact by main's convention, so it moves 45 → 53. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The nine hbrodin threads were already handled in c5358a1; these are the github-actions review's, which were not. Correctness: - `verifyWith` now names `refs/heads/<branch>` rather than the tip sha the survey agent reported. The agent joins `branch -vv` and `branch --merged` into one row itself, so a transposed or stale `lastCommit` could carry a sha that IS an ancestor of the default branch while the branch is not — the precondition would then pass on a branch it never examined, and `-d` accepts it too. A refname cannot desynchronise from the branch it names. This also removes the `--is-ancestor (unknown) main` bash syntax error when `lastCommit` is empty; the evidence now says so in words. - Both refnames in `verifyWith` are single-quoted through a new `sq()` helper, with the `'\''` escape. Refnames may legally contain `$(...)`, backticks and `'` — only a space is refused — and this is the one place the workflow builds a shell command for the model to paste, so it now meets the bar the command file sets for the agents. - `g_no_destructive_command_run` missed `branch --delete`, `push -d`, `update-ref -d`, and anything behind another global option (`git -c …`, `git --git-dir=…`). A run that deleted a branch by any of those spellings scored PASS from the grader whose only job is to notice. Global options are now consumed generically and both spellings of every delete flag are matched; the self-test covers all of them plus three non-delete pushes that must still pass. - `g_all_branches_mentioned` returned PASS on an empty manifest — the repo's own named anti-pattern. It now ERRORs, with an assertion proving it. - `make-repo.sh` claimed reproducible shas while inheriting the caller's git config. `eval-self-tests` is in `make check`, so a developer with `commit.gpgsign = true` would have had the whole build block on a passphrase. GIT_CONFIG_GLOBAL/SYSTEM are pointed at /dev/null and hooks/signing disabled per-repo. The pinned `3fcf672`, `64b5c2a` and `a2f470c` are unchanged. - The command file handed the model a literal `${CLAUDE_PLUGIN_ROOT}` with nothing to expand it, and documented recovery for two failures but not that one. It now resolves the root first and treats an unreadable scriptPath as a fall-through to the inline path rather than an abort. Docs that contradicted the code: - git-cleanup README stated the `git branch -d` safety rationale this PR exists to disprove, and never mentioned `verifyWith` — a maintainer reading it would have dropped the precondition as redundant. Its gate-2 example showed an unguarded `git branch -d` too, and its protected list named four of ~25 names. - merge-evidence.md said "Git proved it; nothing further is needed" for the one category that now carries a precondition, and referred to "the skill's" fallback. - evals/README.md credited `analyze-branches.test.mjs` with covering gate-2 prose it does not read. - Makefile said CI scopes the validator to touched plugins. It does not — only the version-increment check is scoped; AGENTS.md had it right. - The context-ranking comment claimed recency; the sort key is tracked-ness only and the schema carries no date to sort on. - A dead `grep -v` in the self-test, overwritten by the next line. Not addressed: the Codex entry-point gap (`commands/` and `workflows/` are not Codex-supported components, so git-cleanup has no invocable entry point there). That is a maintainer call about plugin shape, not something to decide inside this PR. Suites: 47 JS assertions, 49 eval self-test assertions, 53 validator assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Merged Correctness
Docs that contradicted the code — #1 (both halves), #9, #10, #11, #13, #15, #16 all fixed. #1 was the worst of them: the README stated the #14 dead Not addressed — #3, the Codex gap. You are right that Version 2.2.0 in both manifests. Suites: 47 JS assertions, 49 eval self-test assertions, 53 validator assertions. 🤖 Addressed by Claude Code |
One conflict: #258 raised SELF_TEST_MINIMUM to 71 for its own new fixtures, this branch had it at 53. The floor is exact by convention, and the merged self-test runs both sets, so it moves to the measured 82. #258's new documented-command checker reports nothing against this branch's docs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
hbrodin
left a comment
hbrodin
left a comment
There was a problem hiding this comment.
Re-reviewed only what changed since my last pass. Every one of the nine findings from that round is addressed, and I re-verified each by execution rather than reading the replies — verifyWith naming refs/heads/<branch>, the environment-branch filter, the worktreeOf join, the capped-and-logged context, the recorded pipeline() alignment dependency. Locally green: 47/47 JS assertions, 49/49 eval self-test, 82/82 validator self-test, validator clean across all 41 plugins.
Two findings below, both reproduced by running the script with stubbed agents. Both are in the deterministic core rather than in agent behaviour, so both are cheap to pin with a test.
Things I checked that came back clean, so they aren't comments: sq() correctly escapes $(...), backticks and embedded single quotes in verifyWith; detached-HEAD worktrees don't collide in the new worktreeOf map; the GIT_CONFIG_GLOBAL=/dev/null fixture hardening doesn't break repo creation; and the new run-evals.sh --self-test exec path works on a machine without claude installed. I also confirmed the <repo-data> fence is escapable by a branch name (x</repo-data>_... is a legal refname) — the ceiling is one ambiguous-bucket branch behind both gates, and everything I threw at it aimed at protected or already-settled branches was dropped by the batch-scoping guard, so I'm raising it as a one-line hardening suggestion rather than a finding: strip the delimiter inside fence().
…rvey reports
Both findings from hbrodin's second pass. Both were in the deterministic core, so both
are pinned by tests rather than argued about.
**Protected branches vanished from the report.** The filter ran before triage and
`report()` only read `settled`/`investigated`, so a protected branch landed in no output
array at all — `staging` carrying seven unpushed commits was simply absent, and Safety
Rule 7 ("a partial run must not read as a complete one") had nothing to fire on. Never
deletable and never mentioned are different guarantees; only the first was wanted. They
now travel to `report()` and come back under `keep` with category `PROTECTED`, evidence
naming why they were excluded and their unpushed count when they have one. An unpushed
count on a protected branch is logged as well.
Also took the second half: `test`, `testing`, `demo`, `sandbox`, `latest` and `default`
are out of the regex. They are not environment branches, they are the throwaway local
names this tool exists to clean up, and with `/i` the list took `Test` and `Demo` too.
Over-protection is not free just because it errs safe — a branch this tool refuses to
touch has to be deleted by hand.
**`defaultBranch` arrived as a remote ref.** `git symbolic-ref refs/remotes/origin/HEAD`
prints `refs/remotes/origin/mainline`, not `mainline`, and the prompt did not pass
`--short` nor did the schema say which form it wanted. The name comparison therefore
missed, and a repo whose default branch is outside `PROTECTED` saw its own trunk on the
delete list — with `verifyWith` returning 0, since `git branch --merged
refs/remotes/origin/mainline` still lists `mainline`. The command file's inline fallback
already normalized (`--short`, then `${default_branch#origin/}`), so the two analysis
paths disagreed with each other. Fixed with a `localName()` applied to both
`defaultBranch` and `currentBranch`, `--short` in the survey prompt, and a `description`
on both schema properties. `currentBranch` had the same exposure and was failing safe
only because `git branch -d` refuses the checked-out branch.
Tests: 61 JS assertions, up from 47. Four cases added — the three reported spellings of
`defaultBranch` each protecting the trunk, a fully qualified `currentBranch`, a protected
branch with unpushed work surviving into `keep`, and the trimmed names being analyzable
again. Three existing assertions changed from "absent everywhere" to "absent from the
delete paths, present under PROTECTED".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…namic-workflow-57b146
3 participants
Replaces the prose
SKILL.mdwith a/git-cleanupslash command plus a dynamic workflow — a JavaScript orchestration script that fans branch analysis out across subagents.What changed
commands/git-cleanup.md/git-cleanupentry point: both gates, the confirmations, and every deletionworkflows/analyze-branches.jsreferences/merge-evidence.mdtests/analyze-branches.test.mjsskills/git-cleanup/is deleted. Version 1.0.1 → 2.0.0 in both manifests — removing the skill is a capability removal, not a feature.Why this shape
Only the genuinely uncertain branches get an agent:
git branch --mergedalready answered.The fleet is bounded at eleven agents; past five batches the batches grow rather than the count.
Nothing destructive moved into the workflow. Subagents run in the background with no way to reach the user, so both gates and every
git branch -d/-Dandgit worktree removestay in the main session. Every agent prompt carries a read-only constraint.Uncertainty resolves toward keeping a branch. A refutation missing its
refutedfield, duplicate refutations, a dead agent, and a missing verdict all downgrade to needs-review. A wrong keep costs another look at a branch list; a wrong delete costs work that exists nowhere else.Review notes
trunkwas offeredgit branch -d trunk), a worktree marked stale without consulting its dirty flag, and branch names reaching Bash unquoted (git refnames forbid spaces but permit$and backticks, so$(id)is a legal branch name). Each was reproduced by execution before and after the fix.js-testsmake target and CI job. Both carry the same zero-discovery guard aspython-tests— an empty glob fails rather than reporting a pass. Verified the guard fires. The earlier version of this branch shipped the suite with nothing running it, and a mutation that silently dropped unverified candidates passed 22/22; it now fails.agents/openai.yamland the brand mark (that presentation metadata only attaches to skills here), andcheck_codex_loadability.py— which countsskills/**/SKILL.md— now passes vacuously at0 == 0for this plugin. Nothing in CI verifies the plugin's only entry point loads under either CLI. That gap is in the checkers, not in this plugin, and is worth its own issue..js/.mjsremains unlinted and unformatted. No formatter in this repo covers it. Left out of scope; wiring one up is a repo-wide decision.Testing
make self-test lint python-tests js-tests validateis green — validator reports no errors, 361 references resolved, 37/37 JS assertions.🤖 Generated with Claude Code