fix(hooks): stop stale-team-cleanup deleting a live session's task list - #3305
Merged
Conversation
staleTeamCleanup runs at SessionStart while other sessions are mid-flight,
and cleanupTeam() removes CC's own ~/.claude/teams/<name> AND
~/.claude/tasks/<name>. A false positive there destroys a running session's
task list. On this machine that is 375 task dirs behind 6 team configs.
isStaleTeam had three ways to say "abandoned" about a live team:
1. `if (!existsSync(configPath)) return true` deleted a directory at ANY
age. A session that has just mkdir'd its team and not yet written
config.json was a delete target for the whole window between the two
syscalls.
2. The age came from the team directory's OWN mtime. A directory's mtime
only moves when its direct children change, so writing a nested task
file leaves it untouched and an actively-writing team reads as idle.
3. `catch { return true }` turned an unreadable directory into a delete.
Could-not-observe is not evidence of age.
Now the age gate runs FIRST and applies with or without a config, the age
comes from the newest mtime anywhere in the tree (bounded depth, symlinks
not followed), and every failure path returns false. The gate still fires
on genuinely abandoned teams, config-less or not, so this is a fix and not
a disable.
Evidence, per the v10 definition of done:
against origin/main's implementation 3 failed | 3 passed
against this change 6 passed
full hook suite 307 files, 7565 passed, 2 skipped
Found by the activation audit (2026-08-08), which asked of each mechanism
"does it actually fire" rather than "is it correct". Reproduced
independently by a second agent that built its own control.
Refs #164
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Auto-regenerated by .github/workflows/skill-autobuild.yml after a src-only edit left the committed build output stale (#2528).
…ssion Required by the PR Playground gate (ci.yml:186), which needs docs/<branch-with-slashes-as-dashes>/ to hold at least one .html and the word playground in the PR body. Renders the three independent misfire paths, the before/after flow, the six test cases with their old-code verdicts, and a copy-paste repro that reverts one file and shows 3 failures against origin/main. Refs #164
Contributor
✅ CI Assessment ReportScore: 91/100 (Grade A)
📊 Coverage Details
Generated by OrchestKit CI Report • vitest + @vitest/coverage-v8 |
The hook performs the only irreversible action in the lifecycle set - an
rmSync of CC's ~/.claude/teams/<name> AND ~/.claude/tasks/<name> - and it
logged a bare count: "Cleaned 2 stale team(s)". No name, no age, no path,
no reason. When it deleted a live session's task list there was nothing to
read afterwards, which is exactly why the misfire stayed invisible.
teamStaleness() now returns a typed verdict carrying a greppable reason
(no-home | not-found | unobservable | recent-activity | past-window), the
measured age, and the path whose mtime supplied it. isStaleTeam() stays as
a thin wrapper for callers that do not log.
The hook logs the case for deletion BEFORE the rmSync, so a wrong delete
leaves a record even if the process dies mid-call, and it logs KEEPs too -
a hook that only logs when it acts cannot be told apart from one that never
ran. An unobservable tree gets its own line, because "could not decide" is
not the same as "healthy".
before Cleaned 2 stale team(s)
after DELETE "x": idle 9.3h (window 4h), newest=<path> -> removing ...
KEEP "y": could not observe the tree, refusing to delete
6 team(s) examined, 1 removed, 5 kept: y(recent-activity 0.4h), ...
Refs #164
Auto-regenerated by .github/workflows/skill-autobuild.yml after a src-only edit left the committed build output stale (#2528).
CI shard 1/4 and Hook Coverage both failed on an assertion that reads as identical strings. The two paths differ past the truncation point. newestMtimeMs keeps the FIRST path on an mtime tie. Writing state.json also stamps its parent directory at the same instant, so on ext4's nanosecond granularity the directory tied and won, while on APFS the file did. The test asserted which side of that tie won, so it passed locally and failed on Linux. Backdating every ancestor - not just the team root - leaves exactly one newest path and removes the tie. The second fixture had the same latent hole: it left config.json fresh at the root, so it would have passed for the wrong reason. Refs #164
Verified against the installed binary, not inherited from a report:
grep -c -a -F 'abort-if' ~/.local/share/claude/versions/2.1.226 -> 0
grep -c -a -F 'goal' -> 140
grep -c -a -F 'stop after' -> 2
grep -c -a -F 'permission_mode' -> 53 (control)
abort-if appears in none of the 357 archived changelogs either. Claude Code
takes ONE goal per session, replace-on-set, with the bound written inside that
single condition. So the second line these skills emitted did not add a safety
rail: it parsed as a fresh condition, REPLACED the first, and left the session
goaled on an unsatisfiable budget string with every acceptance assertion
discarded. prd-to-goal/SKILL.md told the user to paste the block straight into
Claude Code, and recipe-library.md:12 hardened the mistake into policy across
8 shipped recipes ("It is the safety rail, not boilerplate").
13 two-line blocks folded into one condition
8 prose sites that still taught the old form
1 skill frontmatter description, which is skill-router trigger text
1 policy line, now saying the opposite
The audit that found this undercounted it twice. It named 4 files; write-prd
carried it too. And it said nothing about a worse variant the fold could never
have reached: 5 /goal lines with NO terminating bound at all, in brainstorm,
explore, audit-full, dev, and the assertion-grader's own worked example. That
is the exact shape prd-to-goal warns about in its own anti-pattern table -
"Unbounded. There is no terminating condition ... this is how runs eat 500K
tokens overnight." The skill documented the hazard and shipped it five times.
Bounds chosen from the skill's own guidance: dev 5 (services come up or they
are broken), brainstorm 8, explore 10, the grader example 15, audit-full 30.
Definition of done, machine-checked:
emitted abort-if lines 0 (was 13)
/goal until WITH a bound 19
/goal until with NO bound 0 (was 5)
Three abort-if mentions survive on purpose: the two lines explaining why it was
removed, and the policy line that now says the opposite of what it said before.
Closes #3312
Refs #164
Auto-regenerated by .github/workflows/skill-autobuild.yml after a src-only edit left the committed build output stale (#2528).
The Build check went red because I edited 9 SKILL.md files for #3312 and never ran the build. plugins/ and docs/site/lib/generated are build outputs; the drift gate (ci.yml:77-82) compares them against a fresh build and fails on any difference. Editing src/ without rebuilding is exactly what CLAUDE.md warns about. 27 files regenerated. Verified the fix propagated into the built artifact: plugins/ork/skills emitted abort-if 0 bounded 19 unbounded 0 Five of the 27 are plugins/ork/commands/*.md — regenerated full-body copies of skills edited once. That is #3314's 499,359 duplicated characters demonstrating itself: one edit, two artifacts, and the wrapper only stays correct because a build step keeps rewriting it. Refs #164
output-guard.mjs held two HAND-TYPED allow-lists of which envelope keys CC consumes per event. Their provenance comment cited "types.ts plus the #1234 audit" — one hand-maintained artifact plus a memory. Both lists were wrong. CC ships its own hooks reference as string literals inside the binary, so this was never a matter of opinion. Verbatim, from 2.1.226: "Hook-specific output for the Stop event. additionalContext is non-error feedback delivered to the model; the conversation continues so the model can act on it." "Hook-specific output for the SubagentStop event. additionalContext is non-error feedback delivered to the subagent..." "Return additionalContext via hookSpecificOutput to inject context once for the whole batch." [PostToolBatch] The guard named none of the three, so it silently DELETED additionalContext from six live hooks: stop/task-completion-check, stop/stop-failure-handler, stop/session-summary, subagent-stop/sync-subagent-stop-dispatcher, subagent-stop/subagent-scope-auditor, subagent-stop/retry-handler. It also omitted WorktreeCreate, whose documented http/callback form returns hookSpecificOutput.worktreePath and therefore needs its hookEventName intact. A guard that wrongly ALLOWS gets caught — something misbehaves. A guard that wrongly STRIPS produces silence. That is the direction nobody checks. WHAT CHANGED spec/cc-output-keys.spec.yml source of truth, every entry carrying the binary string that justifies it bin/cc-output-keys.generated.mjs what the guard imports scripts/derive-cc-output-keys.mjs --check, reads the real binary npm run verify:cc-keys the drift gate Runs locally and on workflow_dispatch, never in ci.yml — ci.yml does not install the CC binary. CI verifies the committed artifact; only this script talks to the binary. It exits 2 = CANNOT-OBSERVE distinct from 1 = drift, and refuses to report a contract if the binary yields no hookSpecificOutput literal. A NEW RULE THE GUARD DID NOT HAVE Allow-listing WorktreeCreate exposed that the old rule caught #1794 by coincidence. It keyed on "does this event consume hookSpecificOutput at all", and WorktreeCreate happened to be off the list. Once it joined, a WorktreeCreate response labelled hookEventName:'UserPromptSubmit' sailed through — which is #1794 itself, the bug where CC created a literal-named directory from the additionalContext string. Rule 0 now checks the mismatch directly: a hookEventName that disagrees with the firing event drops the ENTIRE hookSpecificOutput. Narrower than the old rule and strictly stronger, because it holds regardless of the allow-list. The warning text changed with it — saying "stripped hookEventName" while dropping the whole envelope would be a lying log. UNVERIFIED IS NOT A LICENCE TO STRIP watchPaths, updatedPermissions and sessionTitle are declared by ork and not documented by the binary. They are recorded as UNVERIFIED and the guard leaves them alone. Stripping on not-knowing is exactly how Stop broke. EVIDENCE drift gate, mutation-tested RED exit 1, naming Stop/SubagentStop/ PostToolBatch GREEN exit 0 output-guard-cc-contract 18 passed (fails against the old allow-lists) lifecycle-output-protocol 46 passed (3 message assertions updated) full hook suite 308 files, 7587 passed, 2 skipped dispatcher wiring 8/8, real binary, real stdin typecheck clean TWO BUGS I WROTE IN THE GATE, BOTH CAUGHT BEFORE COMMIT The --check first grepped the file TEXT for 'Stop', which matched inside a comment and inside KEY_EVENTS, so it reported OK against a deliberately broken allow-list. And the red-proof harness edited the wrong Set — 'Stop' appears first in EVENTS_WITH_HOOK_EVENT_NAME — so the mutation never touched the set under test. Both are the same disease as the thing being fixed: a check that cannot fail. The harness now asserts its own removal count. Closes #3307 Refs #164
…ions The mismatched-hookEventName rule added in 19d5c13 broke 9 security tests. Every one of them the same shape: ✗ shell process-sub of curl — expected deny, got abstain ✗ eval of curl substitution — expected deny, got abstain ✗ netcat reverse shell (-e) — expected deny, got abstain lib/output.ts HARDCODES hookEventName:'PreToolUse' inside outputDeny, outputAsk and outputDefer, regardless of which event the hook fires on. So a PermissionRequest hook legitimately emits a 'PreToolUse' label, the rule read that as a mismatch, dropped the whole hookSpecificOutput — and the permission decision went with it. A guard converting a denial into an abstention is the single worst direction this file can fail in, and it is exactly the failure mode the guard exists to prevent. Reverted, with the reasoning left in place at the call site so it is not rediscovered the hard way. Fixing it properly means teaching the output builders their firing event, which is wider than this change. WHAT SURVIVES — the actual finding The allow-list correction stays, because it only ever ADDS permission: additionalContext now preserved on Stop, SubagentStop, PostToolBatch All three are documented verbatim by the CC binary and were absent from the hand-typed list, so the guard had been deleting output from six live hooks: stop/task-completion-check, stop/stop-failure-handler, stop/session-summary, subagent-stop/sync-subagent-stop-dispatcher, subagent-stop/subagent-scope-auditor, subagent-stop/retry-handler. WHAT IS DEFERRED WorktreeCreate, MessageDisplay and Elicitation are back OFF the hookEventName allow-list. Adding them was only safe alongside the mismatch rule; without it they re-open the #1794 class. A test pins the current WorktreeCreate behaviour so the gap stays visible instead of being forgotten. ALSO Added cc-output-keys.generated.d.mts. CI's Static Analysis runs `tsc -p tsconfig.test.json`, a DIFFERENT config from the `npm run typecheck` used locally — the local run was clean while CI failed TS7016 on this import. A .mjs needs a .d.mts, not a .d.ts. EVIDENCE tests/security/test-egress-guard.sh 15 passed, 0 failed (was 6/9) tsc -p tsconfig.test.json clean (was TS7016) npm run verify:cc-keys OK Refs #3307 Refs #164
…ions The mismatched-hookEventName rule added in 19d5c13 broke 9 security tests. Every one of them the same shape: ✗ shell process-sub of curl — expected deny, got abstain ✗ eval of curl substitution — expected deny, got abstain ✗ netcat reverse shell (-e) — expected deny, got abstain lib/output.ts HARDCODES hookEventName:'PreToolUse' inside outputDeny, outputAsk and outputDefer, regardless of which event the hook fires on. So a PermissionRequest hook legitimately emits a 'PreToolUse' label, the rule read that as a mismatch, dropped the whole hookSpecificOutput — and the permission decision went with it. A guard converting a denial into an abstention is the single worst direction this file can fail in, and it is exactly the failure mode the guard exists to prevent. Reverted, with the reasoning left in place at the call site so it is not rediscovered the hard way. Fixing it properly means teaching the output builders their firing event, which is wider than this change. WHAT SURVIVES — the actual finding The allow-list correction stays, because it only ever ADDS permission: additionalContext now preserved on Stop, SubagentStop, PostToolBatch All three are documented verbatim by the CC binary and were absent from the hand-typed list, so the guard had been deleting output from six live hooks: stop/task-completion-check, stop/stop-failure-handler, stop/session-summary, subagent-stop/sync-subagent-stop-dispatcher, subagent-stop/subagent-scope-auditor, subagent-stop/retry-handler. WHAT IS DEFERRED WorktreeCreate, MessageDisplay and Elicitation are back OFF the hookEventName allow-list. Adding them was only safe alongside the mismatch rule; without it they re-open the #1794 class. A test pins the current WorktreeCreate behaviour so the gap stays visible instead of being forgotten. ALSO Added cc-output-keys.generated.d.mts. CI's Static Analysis runs `tsc -p tsconfig.test.json`, a DIFFERENT config from the `npm run typecheck` used locally — the local run was clean while CI failed TS7016 on this import. A .mjs needs a .d.mts, not a .d.ts. EVIDENCE tests/security/test-egress-guard.sh 15 passed, 0 failed (was 6/9) tsc -p tsconfig.test.json clean (was TS7016) npm run verify:cc-keys OK Refs #3307 Refs #164
src/hooks/src/lib/__probe_main_tmp.ts is a 0-byte scratch file a subagent left behind; a broad `git add -A` swept it into 6780f6f. Nothing imports it. This is the second one today. The first, __guards_main_tmp.ts, was an 11,603-byte copy of guards.ts sitting next to the real file — it would have poisoned any `grep permission_mode src/`, which is exactly the check the permission-mode audit depended on. Caught before it was committed; this one was not. Ignoring the shape rather than trusting the next cleanup to notice. Refs #164
Two stale expectations, both mine, both from #3305. 1. Three assertions in lifecycle-output-protocol.test.ts were rewritten in 19d5c13 to expect the mismatched-hookEventName rule's wording ("declared hookEventName=X on a Y event"). That rule was reverted later in the same PR for turning 9 security-test denials into abstentions, but these expectations were not reverted with it. The guard emits "stripped hookEventName=X from Y response", so all three failed. 2. Stop sat in SANITIZE_EVENTS, asserting the guard strips additionalContext from it. That is precisely the defect #3307 reports: CC's shipped binary documents "Hook-specific output for the Stop event. additionalContext is non-error feedback delivered to the model", and the guard was deleting it from three live stop hooks. The test encoded the bug, so fixing the bug broke the test. Stop is removed from the list, with the reason recorded at the site; output-guard-cc-contract.test.ts already asserts Stop, SubagentStop and PostToolBatch keep their output. I reported this file as "46 passed" in the PR body. It was 31 passed, 15 failed. CI caught it on shard 2/4. before 15 failed | 31 passed after 45 passed shard 2/4 77 files, 1943 passed full 308 files, 7586 passed, 2 skipped typecheck clean, incl. tsconfig.test.json Refs #164
The pre-commit gate has not been passing. It has been HANGING, with no output
and no timeout, which is why recent commits carried `--no-verify`. Three
defects, all invisible to CI because CI runs bash 5.2 on Linux and this only
reproduces on macOS bash 5.3.
1. scripts/ci/run-tests.sh:61 — `done <<< "$FOUND"`
bash 5.3 backs a here-string with a pipe rather than a temp file and writes
it from the current shell before the loop starts reading. macOS PIPE_BUF is
512 bytes. $FOUND for tests/indexes is 543 bytes of absolute paths, for six
files, so the write blocks forever with nothing draining it.
The trigger is the total length of the test paths in a directory, so it
arrives when someone adds a test or clones into a deeper directory, and it
never reproduces in CI. Measured:
$FOUND = 543 bytes bash 5.3 -> DEADLOCK
bash 3.2 -> read 6 lines
Swept the same shape across 27 more files. `done < <(printf '%s\n' "$V")`
feeds the identical trailing newline and keeps the loop in the current
shell, so variables set inside it still survive. That is why it is process
substitution and not `printf | while`.
2. tests/ci/lint.sh:185 — `grep -q "^model:" <<< "$agent_head"`
Same deadlock, from the fix for a different bug. The comment above it
records replacing a `head | grep -q` pipeline that reported every agent as
missing `model:`. The replacement traded a race for a hang: head -50 of an
agent is ~1073 bytes, grep -q exits at its match, bash blocks on the rest.
Now a pure-bash [[ =~ ]] helper, which has no subprocess to deadlock on and
no pipeline status to misread.
3. tests/indexes/test-agent-categories.sh:55 — `sed ... | grep -q` under
`set -o pipefail`
grep -q exits at the first match, sed dies on SIGPIPE, the pipeline returns
141, and `! 141` is true, so a field that IS present reads as missing. It
reported 33 of 36 agents missing `category:`. Only README.md actually lacks
it, and the test already skips README. Replaced with one awk pass.
before exit 1, 34 false "missing category" failures
after exit 0, all 36 agents valid
Verified end to end, normal PATH, no bash shim:
./tests/run-all-tests.sh --lint exit=0 3 categories passed, 0 failed
bash -n across all 29 files clean on 3.2 and 5.3, except a
pre-existing 3.2-only parse issue in
test-playground-standard.sh (rc=2 at HEAD
too; its shebang resolves to 5.3)
Refs #164
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
1 participant
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.
First slice of v10 (#164). Four issues, all found by the 2026-08-08
convergence audit — five passes, ~42 agents, every claim through an adversarial
refuter, evidence taken from the shipped CC binary rather than documentation.
They share one root cause: OrchestKit writes to Claude Code contracts nobody
verified against the binary.
1 ·
stale-team-cleanupwas deleting a live session's task listcleanupTeam()removes CC's own~/.claude/teams/<n>and~/.claude/tasks/<n>, and the hook runs at SessionStart — while othersessions are mid-flight. On this machine that is 375 task dirs behind 6 team
configs, with ten concurrent sessions.
isStaleTeamhad three independent ways to call a live team abandoned:!existsSync(config) → truedeleted a directory at any age, includingone created milliseconds earlier by a session that had not yet written
config.json.task file is written — so an actively-writing team read as idle.
catch { return true }turned an unreadable directory into a delete.Now the age gate runs first and applies with or without a config, age comes
from the newest mtime anywhere in the tree, and every failure path returns
false. The gate still fires on genuinely abandoned teams, so this is a fixand not a disable.
2 · That delete was invisible
It logged
Cleaned 2 stale team(s)— a count. No name, no age, no path, noreason. When it deleted a live session's task list there was nothing to read
afterwards, which is exactly why the misfire survived.
teamStaleness()now returns a typed verdict with a greppable reason(
no-home | not-found | unobservable | recent-activity | past-window), themeasured age, and the path whose mtime supplied it. The delete is logged
before the
rmSync, so a wrong delete leaves a record even if the processdies mid-call. Keeps are logged too — a hook that only logs when it acts cannot
be told apart from one that never ran.
3 ·
/goal abort-if— a syntax CC has never supported (#3312)CC takes one goal per session, replace-on-set, with the bound inside the
single condition. So the second line ork emitted did not add a rail: it parsed
as a fresh condition, replaced the first, and left the session goaled on an
unsatisfiable budget string with every acceptance assertion discarded.
prd-to-goal/SKILL.mdtold users to paste it straight into Claude Code, andrecipe-library.md:12hardened the mistake into policy across 8 recipes.The audit undercounted this twice. It named 4 files;
write-prdcarried it too.And it missed a worse variant no fold could reach: 5
/goallines with noterminating bound at all, in
brainstorm,explore,audit-full,devandthe assertion-grader's own worked example — the exact shape
prd-to-goalwarnsabout in its own anti-pattern table.
4 · The contract gate was already half-built, and wrong (#3307)
output-guard.mjsheld two hand-typed allow-lists whose provenance read"types.ts plus the #1234 audit" — one hand-maintained artifact plus a memory.
CC ships its own hooks reference as string literals inside the binary, so this
was never a matter of opinion. Verbatim from 2.1.226:
The guard named none of the three. It had been silently deleting output from
six live hooks:
stop/task-completion-check,stop/stop-failure-handler,stop/session-summary,subagent-stop/sync-subagent-stop-dispatcher,subagent-stop/subagent-scope-auditor,subagent-stop/retry-handler.A guard that wrongly allows gets caught — something misbehaves. A guard that
wrongly strips produces silence.
Now spec-driven:
spec/cc-output-keys.spec.ymlsrc/hooks/bin/cc-output-keys.generated.mjsscripts/derive-cc-output-keys.mjs--check, reads the real binarynpm run verify:cc-keysRuns locally and on
workflow_dispatch, never inci.yml— CI does notinstall the CC binary. Exits 2 = CANNOT-OBSERVE distinct from 1 = drift,
and refuses to report a contract if the binary yields no
hookSpecificOutputliteral at all.
A rule that was tried and reverted, on purpose
Allow-listing
WorktreeCreateexposed that the old rule caught #1794 bycoincidence — it keyed on "does this event consume hookSpecificOutput", and
WorktreeCreate happened to be off the list. So I added a mismatched-label rule.
It broke 9 security tests, turning "expected deny" into "got abstain."
lib/output.tshardcodeshookEventName:'PreToolUse'inoutputDeny/outputAsk/outputDeferregardless of firing event, so a PermissionRequesthook legitimately emits a mismatched label — and dropping the envelope on that
took the permission decision with it. A guard converting a denial into an
abstention is the worst direction this file can fail in.
Reverted, with the reasoning left at the call site.
WorktreeCreate,MessageDisplayandElicitationare back off the allow-list; a test pins thecurrent behaviour so the gap stays visible rather than forgotten. Fixing it
properly means teaching the builders their firing event — wider than this PR.
Evidence
Every change carries a test that fails against the pre-fix code. A green
suite is not evidence on its own — this PR's own #3307 finding is a guard whose
tests passed while it deleted output.
Playground
docs/fix--v10-critical-activation/playground.html— the three misfire paths,the before/after flow, the six test cases with their old-code verdicts, and a
copy-paste repro that reverts one file and shows the failures against
origin/main. Verified in a real browser, zero console errors.Refs #164
Closes #3307
Closes #3312