v2 hardening: shared-state locking, generated slash commands, name validation, version normalization - #22
Merged
Merged
Conversation
Version numbering had drifted across three files: plugin.json said 0.7.0, README said "v0.6 (current)", and ROADMAP said "Shipping Now - v0.6". None of them agreed, and 0.x undersold a tool with 135 tests, a plugin manifest, and a documented install path people already follow. Normalises on 2.0.0 as current, with everything before it presented as v1. The break lands where the substance breaks: v0.4 through v0.6 built the roster, the coordinator, and branch hygiene, while v0.7 carried a behavioural breaking change (persona slash commands stopped writing ~/.claude/CLAUDE.md) that today's three merges complete. A major bump is the honest home for that. The result reads in one pass: v1 built the team, v2 made the team dependable. No v1.0.0 release is fabricated. The repository has no git tags, so v1 is presented as the first generation of the tool rather than a dated release, and the README states plainly that the 0.x numbers were development numbering. Also corrects two stale claims in ROADMAP.md. The "82-test suite" line had been wrong since early July (the DEVLOG v0.7 entry already recorded 98) and the count is now 135. Two v0.6 checkboxes were unchecked but visibly done: the README refactor, and ROADMAP.md itself. Per-version test counts are removed, since one count against current is the only number that will not rot. Local profile overrides is promoted from "exploring" to top priority. Sync fixed the symptom, an edit landing in one installed copy out of three. It did not fix the cause: the only place to customise a persona is a tracked file the next git pull overwrites. The 2026-04-09 ROADMAP revision-history row still reads "v0.6 shipping" and is left verbatim. A dated changelog entry is not rewritten to match a later renumber; the new row beneath it records the change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019j5DHEZsoeCGRueTbNLuTb
commands/<name>.md was hand-maintained while being about 95 percent derivable from profiles/<name>.md. It drifted twice: commit 6d9bb89 "sync commands/ for six personas", and again during the plain-English work. The drift test already contained the transformation as a checker, so this turns that checker into a generator. scripts/generate-agents.sh now writes both agents/<name>.md and commands/<name>.md. Extending it rather than adding a sibling script is deliberate: bin/claude-team sync and install.sh both invoke this one script, so a sibling would never run on either path and claude-team sync would install a slash command lagging the profile edit that triggered it. Each profile gains a ## Greeting section holding the one line the persona says on switch. That was the only part of a command file not derivable from the profile. The generator aborts with a named error when a profile lacks it, so a new persona cannot ship a command ending in a bare separator. ## Greeting is excised from the generated subagent, which is invoked rather than switched to. Verified byte-identical against the previous hand-maintained files. Eleven of seventeen match exactly. Six differ by exactly one deleted blank line, and those six are precisely the files 6d9bb89 hand-synced, so this normalises that drift. agents/ is unchanged from HEAD, which proves ## Greeting does not leak. The hand-maintained drift checker becomes "generated commands match their profiles", mirroring the agents test, plus a new guard that the greeting stays out of the subagents. The sync test now seeds its marker in the profile rather than the generated command, since regeneration would overwrite it; that proves profile to generated command to installed command end to end. 136 tests, up from 135. README and ROADMAP are corrected where they described commands/ as hand-maintained. The DEVLOG entries that say so are dated records of what was true then and are left as written. Known follow-up, flagged during generation and not yet fixed: cmd_use cats the whole profile into ~/.claude/CLAUDE.md, so a globally pinned persona now also carries its greeting and would greet on every new session. The fix belongs in bin/claude-team, which another change is holding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019j5DHEZsoeCGRueTbNLuTb
~/.claude/branches/INDEX.md and ~/.claude/CLAUDE.md are shared across every Claude Code session on the machine. Parallel sessions are this product's headline feature, so concurrent access is the designed-for case. Nothing serialised it: grep for flock over the CLI returned zero. Measured on the unlocked code with 40-way concurrency, five runs, every run lost data: 23 to 29 of 40 index rows survived and 5 to 8 of 20 status updates applied. An independent 12-way probe reproduced it at 8 of 12 rows. A second failure mode was found that the original report missed. block_install greps for its start marker, misses it against a concurrent write that has not landed, takes the append path, and writes a duplicate block. Two of five runs produced two to four CLAUDE-TEAM blocks in CLAUDE.md, after which get_active reads a corrupted awk range. Appends were never torn. Bash >> opens O_APPEND and POSIX makes the offset update and the write one step, so an 80-byte row is always intact. The appends were discarded: one landing between a rewriter's read and its rename is erased by that rename. Atomicity of an append says nothing about its survival, which is why branch start and session start must take the same lock as the rewriters despite looking safe. Serialisation uses a directory lock via mkdir. It is one filesystem operation on every platform and needs no binary that stock macOS omits, unlike flock. The accepted tradeoff is that the kernel does not release it when the holder dies, so liveness is handled here: EXIT, INT, and TERM traps release on every exit short of SIGKILL; a lock whose owner record names a dead PID on this host is broken; and a lock older than 120 seconds is broken, which covers a reboot where the PID may have been reused. A contender confirms any verdict twice before breaking anything. Rejected: flock with an mkdir fallback, because two schemes do not exclude each other and any host with mixed invocations silently loses exclusion, with the fallback running only on the platform CI does not cover it. Also rejected: noclobber and symlink locks, which cannot hold the owner record as a child and so need a second file and a second race. Verification found two bugs in the first implementation that review had not. Treating "no owner record" as stale broke live locks 73 times per run, because every holder passes through that state between its mkdir and its record write. Treating "no lock directory" as stale made contenders remove directories other contenders had just created. Both now count as live. All five temp files are now created beside their destination and renamed within one filesystem. This is not comment accuracy. A cross-device mv degrades to copy plus unlink, and a reader can observe a half-copied index during it, so this is what makes "readers need no lock" true. Proven with TMPDIR=/nonexistent-tmpdir, under which bare mktemp cannot create a file and every write path still works. Two latent bugs fixed in passing. awk > tmp && mv reported success when awk failed, because set -e exempts a failing command in an && list, so branch close and session done printed "marked as merged" without changing the index; both now die. ensure_branches_index could truncate when two sessions both found the index missing and both wrote the header with >; it now writes under noclobber. Fourteen tests added. Nine of them fail on the pre-fix code, including the duplicate coordinator block. The concurrency pair is timing-dependent by nature: it fails probabilistically on unlocked code, which is the direction that can only ever under-report, and passes deterministically on locked code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019j5DHEZsoeCGRueTbNLuTb
Generating slash commands from profiles added a ## Greeting section to every profile, holding the line a persona says when a session switches to it. cmd_use cats the whole profile into ~/.claude/CLAUDE.md, which is the global pin, so 'claude-team use akira' wrote that greeting into state every future session reads. Pinned, it makes every new session open by greeting the user, forever, rather than once at the switch. profile_without_greeting stops at the heading. The section runs to end of file, so that is the whole excision, and a profile without the heading prints unchanged. cmd_launch is deliberately left alone. It opens one dedicated session with the persona as system prompt, so greeting once on arrival is the intended behaviour there. The difference is between pinning a default and starting a session. Two tests added. The greeting guard fails on the previous code; the companion assertion that the signature question still arrives guards the other direction, so a future change cannot satisfy the first by truncating the profile early. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019j5DHEZsoeCGRueTbNLuTb
Two unrelated things, both in the test suite's blast radius. CI lint was red from commit 00b82aa, which is mine. shellcheck read the bare word 'done' in "$CLI" branch done as a loop terminator (SC1010) at two sites, and flagged an unquoted expansion (SC2086) at a third. The rest of this suite already quotes "done" for exactly that reason, so the new code was the outlier. The unquoted expansion packed a command and its label into one colon-delimited string and relied on word splitting to unpack it. Quoting it would have passed "coordinator on" as a single argument and broken the test, so the packing is gone rather than quoted. Three plain lines beat one clever loop. resolve_name now requires a name to match ^[a-z0-9][a-z0-9_-]*$. It is the single choke point for show, use, and launch, and it stays orthogonal to the $CLAUDE_TEAM_PROFILES override the suite depends on. This is not a vulnerability fix and is not claimed as one. The CLI runs as the invoking user, reads with that user's permissions, never writes to the traversed path, and appends .md, so every file it could reach is one the caller can already cat. It earns its place for two other reasons. It fixes a real correctness bug: 'use ../gtm' previously exited 0, printed a success line with an empty name, and pinned a non-profile into ~/.claude/CLAUDE.md, where every future session reads it. It also stops a leading-dash name reaching 'claude -n' as an option. sanitize_branch_for_path was checked and deliberately left alone. It maps / to -, so a component with no / cannot traverse, and git's own check-ref-format rejects '..' before the path is built. Validation there would reduce no risk. Eleven tests added. Six fail on the previous code, including the one that matters most: a rejected name reaching the global pin. The first of them asserts the traversal target still exists, because an earlier run of this control passed for the wrong reason when gtm.md was absent from a scratch tree and the traversal resolved to nothing either way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019j5DHEZsoeCGRueTbNLuTb
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019j5DHEZsoeCGRueTbNLuTb
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 all five findings from an adversarial review of the codebase. Each was delegated to the persona whose lane it sits in, working from profiles installed through the documented
git clone+bash install.shpath.Tests: 136 → 163. shellcheck clean.
1. The parallel-sessions feature was losing data (the important one)
~/.claude/branches/INDEX.mdand~/.claude/CLAUDE.mdare shared across every session on the machine, and nothing serialized writes to them. Measured at 40-way concurrency, five runs, every run lost data: 23–29 of 40 rows survived, 5–8 of 20 status updates applied. An independent 12-way probe reproduced it at 8/12.Two things the review got wrong, both caught by Akira:
The review reasoned that
>>appends inbranch startwere safe because POSIX makes an O_APPEND write atomic. That reasoning is correct and irrelevant. The appends were never torn — they were erased, by a rewriter's rename landing after their append. Atomicity of an append says nothing about its survival. That's why the appenders need the lock too.The review also never looked at
CLAUDE.md.block_installgreps for its start marker, misses it against an unlanded concurrent write, takes the append path, and writes a duplicate block. Two of five runs produced up to fourCLAUDE-TEAMblocks, after whichget_activereads a corrupted awk range.Approach:
mkdirdirectory lock.flockis absent from stock macOS, and a flock-with-mkdir-fallback design doesn't exclude itself, so a host with mixed invocations silently loses mutual exclusion — with the fallback running only on the platform CI doesn't cover it. Accepted cost is owning liveness: EXIT/INT/TERM traps, a dead-PID rule, and a 120s ceiling for the reboot/PID-reuse case.Verification beat reasoning twice in the same change. The first implementation treated "no owner record" as stale. Every live holder passes through that state between its
mkdirand its record write, so contenders broke live locks 73 times per run. It also treated "no lock directory" as stale, so contenders removed directories other contenders had just created. Review caught neither; the harness caught both.Also fixed in passing:
awk > tmp && mvreported success whenawkfailed (set -eexempts a failing command in an&&list), sobranch doneprinted "marked as merged" without changing the index.2.
commands/is now generatedIt was hand-maintained while ~95% derivable, and the drift test already contained the transformation as a checker. Now a generator. Profiles gained a
## Greetingsection — the only non-derivable part — excised from the subagents.Alex overruled the brief here, correctly. Offered a choice between extending
generate-agents.shand adding a sibling script, he extended it:claude-team syncandinstall.shboth invoke that one script, so a sibling would never run on either path andsyncwould install a slash command lagging the edit that triggered it.Verified byte-identical: 11/17 exact, 6 differing by one deleted blank line — precisely the files commit
6d9bb89hand-synced.3. Name validation (downgraded, then fixed anyway)
Morgan rated the path traversal Low / Informational as a vulnerability and refused to inflate it: the CLI runs as the invoking user, reads with that user's permissions, and appends
.md, so every reachable file is one the caller can alreadycat.He fixed it on correctness grounds instead.
use ../gtmexited 0, printed a success line with an empty name, and pinned a non-profile into~/.claude/CLAUDE.mdwhere every future session reads it. That's the real defect, and it's what the tests assert.He also checked
sanitize_branch_for_path, found git'scheck-ref-formatalready rejects.., and left it alone — a control reducing no risk is a liability.4. Greeting kept out of the global pin
## Greetingin profiles meantclaude-team use akirawrote "Greet the user briefly as Akira" into the global pin, so every new session would greet forever. Excised fromuse, deliberately kept inlaunch:usepins a default,launchstarts a session, and that's the moment a greeting is for.5. Version normalization
plugin.jsonsaid0.7.0, README saidv0.6 (current), ROADMAP saidShipping Now — v0.6. Now2.0.0throughout.Toni put the major bump where the break actually is:
v0.7carried a behavioral breaking change (slash commands stopped writing~/.claude/CLAUDE.md), and today's three merges finish that arc. No v1 release is fabricated — the repo has zero git tags, so v1 is presented as the first generation, not a dated release. The April roadmap row still reads "v0.6 shipping" and stays verbatim.Also corrected: the "82-test suite" claim had been wrong since early July (DEVLOG's own v0.7 entry already said 98).
Not fixed, deliberately
The CLI symlink into the clone. If the clone moves, the CLI and hook break loudly — and loud is correct. The install genuinely is broken,
bash install.shrepairs it, and silencing it would hide real breakage. The symlink buysgit pullupdating the CLI with no reinstall.Mistakes made and recorded
00b82aa— three shellcheck findings intests/run.sh, two a baredoneread as a loop terminator. The rest of the suite already quotes"done". Morgan caught it from an adjacent file.${spec%%:*}would passcoordinator onas one argument and break the test. Right finding, wrong fix, which is the argument for verifying a subagent's patch rather than applying it.gtm.md, so the traversal resolved to nothing and everything passed on unfixed code. The suite now asserts the target exists first.Dogfooding note
Installing the team validated the same-day install fixes end to end. It also surfaced a gap: Claude Code registers subagent types at session start, so seventeen freshly installed agents were invisible to the running session — while the installer prints "Subagents installed (delegate with...)" as though they're live.
install-commandscorrectly says slash commands need no restart. The one that does need a restart is the one that doesn't say so. Not fixed here; worth a one-line installer change.Generated by Claude Code