From e49c01262128507131dad8ca892c8c91e4d3b5bf Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 23:48:49 +0000 Subject: [PATCH 1/3] docs: package the game team as a mini team that proves the framework generalises Implements audit finding F6, which said "seventeen specialists" was padded by four personas the positioning does not serve, while the interesting story went untold: the persona framework generalises to any domain, and a WW2 tabletop studio inside a software tool is the proof. The count stays at seventeen. The four stop being a bonus roster and become the argument, which also answers finding F7: there was no response to "why not wire up the two or three specialists I care about myself, in an afternoon?" The honest answer is that you could, because the mechanism was never the hard part. What does not paste in from a system prompt is an interlocking team. That argument is shown rather than asserted, from three checkable facts about the profiles. Each persona is defined as much by the lane it will not cross as the one it owns. The Handoff Briefs name each other in two closed loops, Reiner to Piper and back, Cornelius and Ernie passing a line over what the record supports. And all four were assembled from the same profile structure and the same generator as the other thirteen, for a domain the tool was never built for. The section is written self-contained, with no references to surrounding material, so it survives the planned move of the dossiers to TEAM.md as a unit. gtm.md carried a stale roster throughout: "Ten of them", "one of ten", and "10 named specialist personas" in the flagship post, plus a "call X for Y" list missing Sage, Kai, Iris, and the studio entirely. All corrected to seventeen, with the studio as its own mini-table in the highest-traffic asset and a separate content-calendar thread rather than being forced into the existing series. Deliberately not claimed: that a game shipped. Nothing in the repo supports it, and the verifiable asset is the team's structure, not a product. Also left alone: gtm.md predates the no-emdash house style and is full of them, which is a separate cleanup rather than part of this finding. 223 tests unchanged; nothing here is under test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019j5DHEZsoeCGRueTbNLuTb --- README.md | 8 ++++++-- gtm.md | 29 ++++++++++++++++++++++++----- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 369fdd6..f1507fd 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,7 @@ ### The Game Development Team -Four specialists for card and board game projects. Reiner designs the systems, Cornelius verifies the history, Ernie writes the words, and Piper tries to break it all. +A four-person studio for card and board game projects, and a working demonstration that the persona framework is not just for software. Built from the same profile structure and the same generator as the rest of the team, the four hold their lanes and hand off by name: Reiner designs the systems, Cornelius verifies the history, Ernie writes the words, and Piper tries to break it all. | Name | Role | Ask them about | |---|---|---| @@ -370,7 +370,11 @@ Toni is strategic and audience-obsessed. They think about every decision through ## The Game Development Team -Reiner, Cornelius, Ernie, and Piper work as a unit on card and board game projects: systems design, historical accuracy, narrative, and playtesting. Each keeps a strict lane (Reiner decides what the evidence means, Cornelius owns the facts, Ernie owns the words, Piper owns the table) and they hand off to each other by name, the same way the rest of the team does. +Reiner, Cornelius, Ernie, and Piper are a mini team, and they are also the answer to a fair question: if the tool just injects a persona, why not wire up the two or three specialists you care about yourself, in an afternoon? + +You could. The mechanism was never the hard part. Copying a profile is easy; building specialists that hold a lane and hand off cleanly is the work, and it is what turns a folder of prompts into a team. These four are the proof, because they are a working team for a domain the tool was never built for: a WW2 tabletop-game studio, assembled from the same profile structure and the same generator as every other specialist here. + +Look at how they interlock. Each is defined as much by the lane it will not cross as by the one it owns. Reiner designs the systems but hands the table to Piper. Cornelius establishes the facts and writes no prose. Ernie writes the in-world words and sends positioning to Toni, verification to Cornelius. Piper breaks the game and redesigns nothing, then hands what the table produced back to Reiner. Their Handoff Briefs name each other in a closed loop: Reiner to Piper, Piper to Reiner, Cornelius and Ernie passing a line back and forth over what the record will support. That interlock is the part you cannot paste in from a system prompt. It is the curation the mechanism does not give you, and it is the same discipline the rest of the team runs on. ### Reiner: Tabletop Game Designer diff --git a/gtm.md b/gtm.md index 89b57d1..2f8b0fe 100644 --- a/gtm.md +++ b/gtm.md @@ -19,7 +19,9 @@ These people don't need to be sold on Claude. They need to be sold on **the diff claude-team-cli is not a prompt library. It's not a collection of system prompts you paste into a chat window. -It's a named, opinionated specialist who shows up with domain expertise, asks the questions a senior practitioner would ask, and pushes back when something's off. Ten of them, covering the full product development lifecycle from discovery to launch. +It's a named, opinionated specialist who shows up with domain expertise, asks the questions a senior practitioner would ask, and pushes back when something's off. Seventeen of them. Thirteen cover the full product development lifecycle from discovery to launch. The other four are a tabletop-game studio, and they exist to answer the one objection this category always draws. + +**The objection, and the answer.** "If it just injects a persona, why not wire up the two or three specialists I care about myself, in an afternoon?" You could. The mechanism was never the moat. Copying a profile is easy; curating specialists who hold a lane and hand off to each other cleanly is the work, and that is what a session actually runs on. The proof is the game studio: Reiner (design), Cornelius (history), Ernie (narrative), and Piper (playtesting) form an interlocking four-person team in a domain the tool was never built for, assembled from the same profile structure as the software specialists. Anyone can paste three prompts; a coherent studio that holds its lanes and hands off by name is a different claim, and it is the one worth paying attention to. **One-line positioning:** *Your AI development team. Seventeen specialists, one CLI, zero meetings.* @@ -34,6 +36,7 @@ Generic Claude gives you a checklist. A team member reframes the problem. 1. **You're not getting Claude's best work.** Without a persona, Claude defaults to generic, safe, surface-level responses. With a specialist active, it thinks the way that domain actually thinks. 2. **It's not a gimmick — it's a workflow.** The coordinator suggests who should lead each task. Slash commands let you switch mid-session. The devlog and roadmap skills persist context across sessions. 3. **Ten minutes to install. Immediate difference.** `git clone`, `bash install.sh`, done. No API keys, no configuration, no dependencies beyond Bash and Claude Code. +4. **The team is the moat, not the mechanism.** Injecting a persona is easy to copy; a curated set of specialists who hold their lanes and hand off cleanly is not. The four-person tabletop-game studio (Reiner, Cornelius, Ernie, Piper) is the proof: the same structure produces a working team even in a domain the tool was never built for. --- @@ -58,6 +61,7 @@ The blog series follows a single product (ACME Personal Jet Packs) through its e | 10 | "Who owns this, when is it due, and what's blocking it?" (Quinn) | Blog (Medium) | Written | | 11 | "The tools that make the team remember" (Devlog + Roadmap) | Blog (Medium) | Written | | 12 | "AI Writes Code Fast. Lint Keeps It Honest." | Blog (Medium) | **Published** | +| E | "I pointed a dev-team tool at a WW2 board game" (game studio / extensibility) | Blog + LinkedIn | Planned | | L1 | Post 0 LinkedIn teaser (Robin before/after) | LinkedIn | Written (Section 6) | | L2 | Slash commands overview | LinkedIn | Written (Section 7) | @@ -69,6 +73,10 @@ Post 12 breaks from the ACME series format. It's a standalone thought piece targ **Publishing:** Medium + LinkedIn. Same golden rule (no link in LinkedIn body). LinkedIn teaser leads with the tension: AI writes fast, review doesn't scale. +#### The Extensibility Angle (separate thread) + +The four-person tabletop-game studio (Reiner, Cornelius, Ernie, Piper) does not belong in the ACME jet-pack series: it is not part of that product's lifecycle, and forcing it in would blur the narrative. It earns its own standalone piece, aimed squarely at the "why not just build this myself" skeptic. Working title: *"I pointed a dev-team tool at a WW2 board game. It built a studio."* The argument is that the same profile structure produced a coherent, lane-disciplined team in a domain the tool was never designed for, which is the strongest answer we have to "why not wire up three personas yourself." Same before/after proof structure; the "after" is the studio holding its lanes and handing off by name. One post, not a series. + ### Post Structure — Persona Spotlights (Posts 1-10) Each persona post follows the same template: @@ -226,11 +234,11 @@ So I built a team of them. #### What claude-team-cli actually does -It gives you 10 named specialist personas for Claude Code. Each one is a formal expert consultant with deep domain knowledge, a distinct way of thinking, and real opinions about how work should be done. +It gives you seventeen named specialist personas for Claude Code. Each one is a formal expert consultant with deep domain knowledge, a distinct way of thinking, and real opinions about how work should be done. You pick who's on the task. Claude shows up as that person. -Need to define requirements? Call River. Design an API? Akira. Build a component that needs to be accessible and secure? Sasha. Data pipelines? Jordan. Dashboards and KPIs? Casey. Security review? Morgan. Deployment infrastructure? Alex. Test strategy? Robin. Launch positioning? Toni. Sprint planning? Quinn. +Need to define requirements? Call River. Design an API? Akira. Build a component that needs to be accessible and secure? Sasha. Data pipelines? Jordan. Dashboards and KPIs? Casey. Security review? Morgan. Deployment infrastructure? Alex. Test strategy? Robin. Launch positioning? Toni. Sprint planning? Quinn. Company formation and finances? Sage. A mockup before code? Kai. A logo or icon set? Iris. And when the work isn't software at all, there's a four-person studio for tabletop games: Reiner, Cornelius, Ernie, and Piper. You can switch mid-session with a slash command. The coordinator — an optional behavior layer — suggests who should lead each task and flags when the work drifts into a different domain. @@ -251,6 +259,17 @@ You can switch mid-session with a slash command. The coordinator — an optional | **Toni** | Product Marketing | *"Who specifically benefits from this — and what would make them choose us over doing nothing?"* | | **Quinn** | Project Manager | *"Who owns this, when is it due, and what's blocking it?"* | +And when the project isn't software at all, the same structure holds. Here's the four-person studio for tabletop games: + +| Name | Role | They'll ask you... | +|---|---|---| +| **Reiner** | Tabletop Game Designer | *"What decision is the player actually making here, and is it interesting?"* | +| **Cornelius** | Military Historian | *"Is this what actually happened, and why did it matter to the outcome?"* | +| **Ernie** | WW2 Narrative Author | *"Is this true, and does it make the reader feel why it mattered?"* | +| **Piper** | Tabletop Playtester | *"How do I break this, and is it still fun when I can't?"* | + +They were built from the same profile structure as the specialists above, and they hold their lanes the same way: Reiner designs, Cornelius checks the history, Ernie writes the words, Piper tries to break it. That is the real claim. Not "you can inject a persona" (you always could), but "the same structure produces a working team even in a domain the tool was never built for." + These aren't decorative. Each persona comes with enterprise-grade security instincts — every team member flags secrets in code, PII in logs, and missing access controls as a matter of course. --- @@ -361,7 +380,7 @@ Robin: Before I sketch the strategy, I need to That's not a checklist. That's a senior QA engineer reframing the problem before touching a test file. -Robin is one of ten. Two-minute install. Open source. +Robin is one of seventeen. Two-minute install. Open source. What domain do you wish you had a specialist for? @@ -426,4 +445,4 @@ github.com/code-katz/claude-publish-agent --- -*Document maintained by Toni. Last updated: 2026-03-27.* +*Document maintained by Toni. Last updated: 2026-07-29.* From fb23e4737ac741513419eb88f24d172f6be61bc8 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 01:07:35 +0000 Subject: [PATCH 2/3] fix: take stale locks over in place, and five other robustness findings Six findings from the systems audit. The headline one did not go the way I specified, and the correction matters more than the fix. I accepted the recommendation to make lock breaking single-winner by renaming the stale lock directory aside instead of removing it. Measured, that changes nothing: 5 lost updates in 30 rounds against 6 unfixed. Renaming makes the removal single-winner, but the staleness verdict still gets applied to whatever occupies the path afterwards. An instrumented run caught the real sequence: A renames the stale lock aside, B takes the freed path with a fresh mkdir, C renames away B's live lock, and three processes hold the lock at once. Any design that empties the path frees it for a moment, and a mkdir can land in that moment. So a contender no longer removes the lock directory at all. It takes the lock over in place: it renames the owner record aside, which exactly one contender can do, verifies what it actually claimed, then writes its own record. Only the owner ever removes the directory. Measured 0 lost updates in 40 rounds under the same 12-writer storm with CPU pressure. The directory-rename claim survives only for the ownerless deadline path, where there is no record to take over. Liveness now outranks the age rule. kill -0 decides first, and the 120 second age test is the fallback for cases where liveness cannot be established: a foreign host or a non-numeric PID. A reboot can leave a lock whose PID has been reused by a live process, and that lock now wedges until the timeout rather than being broken. The timeout already prints the rm -rf command, so that is loud and recoverable, which beats silently discarding a completed write. tmp_beside now returns through _TMP_FILE rather than stdout, because every caller wrapped it in a command substitution and the subshell stranded the assignment, so the EXIT trap could never remove a temp file. A SIGINT mid-rewrite left a full partial copy beside CLAUDE.md. tmp_commit clears the registration only after the rename succeeds. A titleless .md in the profiles directory no longer kills the pipeline under set -euo pipefail. list exited 1 printing nothing with empty stderr, and help exited 0 with a silently empty roster. Both now skip the file and name it. install.sh survives stdin at end of file. A bare read returned non-zero and ended the install mid-prompt, so a piped or CI run exited 1 with no summary after every earlier step had already applied. End of file now skips the coordinator and says why, because nobody is present to consent to writing a global file. Enter still means casual. cmd_sync explains a failed copy, naming the three surfaces and that rerunning heals it. My diagnosis here was wrong: set -e already stopped the command, so the defect was that it was unexplained rather than silent. Three things found on the way. lock_acquire could hang forever, because the stale branch skipped the deadline check, so a lock that could never be claimed spun with no timeout and no message; latent before, reachable after, now dies loudly. A single local declaration referenced its own earlier variable, which is not in scope yet. And assert_count could never pass with an expected 0, because grep -c prints 0 and also exits 1, so the || fallback produced two lines and bash raised a syntax error instead of comparing. 268 tests, up from 223. Seventeen fail against main, one per finding, including one that names the eight personas help silently dropped. Suite runtime 9.4s to 15.5s, which is a real cost and is flagged rather than absorbed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019j5DHEZsoeCGRueTbNLuTb --- bin/claude-team | 368 +++++++++++++++++++++++++++++++++++++++----- install.sh | 20 ++- tests/run.sh | 400 +++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 743 insertions(+), 45 deletions(-) diff --git a/bin/claude-team b/bin/claude-team index 37ad4c4..76ff314 100755 --- a/bin/claude-team +++ b/bin/claude-team @@ -62,10 +62,15 @@ capitalize() { printf '%s\n' "${1^}"; } # - the holder records host, PID, and acquisition time in /owner; # - the EXIT, INT, and TERM traps release the lock on every exit except # SIGKILL and power loss; -# - a contender breaks a lock whose holder is a dead process on this host, -# or whose age exceeds LOCK_STALE_SECONDS. The age rule also covers a lock -# left by a reboot, where the recorded PID may since have been reassigned -# to a live process. +# - a contender breaks a lock only when the holder is provably gone: a dead +# PID on this host, or, where liveness cannot be established at all, an +# acquisition older than LOCK_STALE_SECONDS. See _lock_is_stale. +# - breaking has exactly one winner, and the lock directory is removed only by +# the process that owns it. A stale lock is taken over IN PLACE, by renaming +# its owner record aside, which exactly one contender can do. See +# _lock_takeover, which also records why removing the directory instead +# cannot be made correct. The one exception is a lock with no owner record at +# all, which has nothing to take over; see _lock_break. # # The critical sections are a few awk passes over a small file, so a wait of # seconds already means a holder is stuck rather than queued. @@ -74,13 +79,54 @@ LOCK_STALE_SECONDS=120 LOCK_POLL_SECONDS=0.05 LOCK_HOST="${HOSTNAME:-$(uname -n 2>/dev/null || echo unknown)}" -_LOCK_DIR="" # non-empty while this process holds a lock -_TMP_FILE="" # non-empty while a temp file waits for its rename +_LOCK_DIR="" # non-empty while this process holds a lock +_LOCK_DEAD="" # non-empty while a renamed-aside lock waits for removal +_LOCK_CLAIMED="" # non-empty while a claimed owner record waits to be replaced +_LOCK_CLAIMED_DEST="" # where _LOCK_CLAIMED must be put back on an early exit +_TMP_FILE="" # non-empty while a temp file waits for its rename -_lock_owner() { cat "$1/owner" 2>/dev/null || true; } +_LOCK_OWNER_LINE="" # owner record from the last _lock_read_owner call -# True only when the lock holder is provably gone: a dead PID on this host, or -# an acquisition older than LOCK_STALE_SECONDS. +# Read an owner record from $1, which is the path of the record FILE, into +# _LOCK_OWNER_LINE. Takes the file and not its directory, because a claimed +# record is renamed to a sibling name and has to be readable too. +# +# Fork count is load-bearing here, not a micro-optimisation. Every fork between +# the staleness verdict and the claim widens the window in which other contenders +# can act on the lock, and a verdict applied after that window is applied to the +# wrong object. 'owner=$(cat ...)' costs two forks, the subshell and the exec; a +# redirect into 'read' costs none. +_lock_read_owner() { + local line="" + # A missing file leaves $line empty and returns non-zero, and so does a final + # line with no newline, which has still been assigned. Keep what was assigned. + 2>/dev/null read -r line < "$1" || true + _LOCK_OWNER_LINE="$line" + return 0 +} + +# True only when the lock holder is provably gone. +# +# Liveness is checked first and it is decisive: on this host, a live PID is +# never stale, whatever its age. The age test used to run first and +# independently, so a holder that was merely paused lost a lock it still held, +# and both writers then reported success on a write only one of them kept. +# Three ordinary events pause a holder past 120 seconds: SIGSTOP (job control), +# a laptop suspend mid-command, and a clock that steps forward (NTP correction, +# a VM resuming from a snapshot). +# +# The age rule is the fallback for the case where liveness cannot be +# established at all: a lock recorded by a different host, which happens when +# $HOME is shared over NFS, or a PID field that is not a number. +# +# The tradeoff, accepted deliberately. A reboot can leave a lock whose recorded +# PID has since been reused by an unrelated live process. Liveness-first then +# judges that lock live, so instead of being broken at 120 seconds it wedges +# until the 15-second acquire timeout, and the timeout message already prints +# the exact 'rm -rf' that clears it. That is loud, bounded, and recoverable by +# the user. The alternative, which is what the age-first order did, is to +# silently discard a completed write. A wedge the user can see and fix beats a +# lost write nobody ever learns about. # # Two states deliberately count as live. An absent lock directory is not # stale: the next mkdir wins it, and breaking it would delete a directory that @@ -100,11 +146,17 @@ _lock_is_stale() { if ! 2>/dev/null read -r host pid started < "$lock_dir/owner"; then return 1 fi - now=$(date '+%s') - if [[ "$started" =~ ^[0-9]+$ ]] && (( now - started > LOCK_STALE_SECONDS )); then + # An 'if', not 'kill -0 ... && return 1': an && list here returns the failed + # left-hand status, which is the shape that has already killed two functions + # in this file when it landed last in a body. + if [[ "$host" == "$LOCK_HOST" && "$pid" =~ ^[0-9]+$ ]]; then + if kill -0 "$pid" 2>/dev/null; then + return 1 + fi return 0 fi - if [[ "$host" == "$LOCK_HOST" && "$pid" =~ ^[0-9]+$ ]] && ! kill -0 "$pid" 2>/dev/null; then + now=$(date '+%s') + if [[ "$started" =~ ^[0-9]+$ ]] && (( now - started > LOCK_STALE_SECONDS )); then return 0 fi return 1 @@ -121,14 +173,133 @@ _lock_take() { return 0 } +# Remove directories a killed breaker left beside the lock. Only a SIGKILL +# between _lock_break's rename and its removal can leave one, because the traps +# cover every other exit, and nothing reads these paths. An 'if' rather than +# '&& rm', so a false test cannot become the function's exit status. +_lock_sweep_dead() { + local lock_dir="$1" leftover + for leftover in "$lock_dir".dead.*; do + if [[ -d "$leftover" ]]; then rm -rf "$leftover"; fi + done + return 0 +} + +# Take a stale lock over IN PLACE, and let exactly one contender do it. +# Returns 0 only if this process now holds the lock. $2 is the owner record the +# caller judged stale, and it is what makes this safe. +# +# The lock directory is never removed here, and that is the whole design. A +# contender claims the lock by renaming its OWNER RECORD aside, which succeeds +# for exactly one of them because the source file stops existing and every later +# rename of it gets ENOENT. The winner then writes its own record in place. +# +# Removing the directory instead cannot be made correct, and it took two +# measured attempts to establish that. 'rm -rf "$lock_dir"' lost updates in 6 of +# 30 runs at 12 concurrent writers. Claiming the removal with +# 'mv "$lock_dir" "$lock_dir".dead.$$' still lost them in 5 of 30, and an +# instrumented run showed why: contender A renamed the stale lock aside, B took +# the path A had just freed, and C, acting on a verdict computed before any of +# that, renamed away B's LIVE lock. All three reported success. Verifying after +# the claim and rebuilding the victim's lock cut it to 1 in 30, but could not +# reach zero, because every one of those designs frees the lock path for a moment +# and a mkdir can always land in that moment. +# +# Taking over in place has no such moment. While the record is renamed aside the +# directory still exists, so no mkdir can succeed, and a contender that finds no +# readable owner record already treats the lock as live and waits. Two further +# properties follow, and both matter: +# +# - the verification is free of races. Once the record has been renamed aside, +# this process holds the only path to it, so it can be read and compared at +# leisure. A record that is not the one we judged means our verdict expired, +# and we hand it straight back. +# - handing it back cannot fail. The only writers of /owner are a holder +# that has just created the directory, which cannot happen while it exists, +# and a takeover winner, which cannot happen while we hold the record. So +# nothing can occupy the path we are restoring. +_lock_takeover() { + local lock_dir="$1" want="$2" got + local claimed="$lock_dir/owner.dead.$$" + mv "$lock_dir/owner" "$claimed" 2>/dev/null || return 1 + # Registered for the traps: dying here would otherwise leave the lock with no + # owner record at all, which reads as live to every contender and so wedges + # the file until the timeout tells a human to remove it. + _LOCK_CLAIMED="$claimed" + _LOCK_CLAIMED_DEST="$lock_dir/owner" + _lock_read_owner "$claimed" + got="$_LOCK_OWNER_LINE" + if [[ "$got" != "$want" ]]; then + mv "$claimed" "$lock_dir/owner" 2>/dev/null || true + _LOCK_CLAIMED="" + _LOCK_CLAIMED_DEST="" + return 1 + fi + # Ours. _LOCK_DIR is set before the record is written, so the traps release the + # lock if this process dies in between. + _LOCK_DIR="$lock_dir" + printf '%s %s %s\n' "$LOCK_HOST" "$$" "$(date '+%s')" > "$lock_dir/owner" \ + || die "Cannot write the lock owner record: $lock_dir/owner" + rm -f "$claimed" + _LOCK_CLAIMED="" + _LOCK_CLAIMED_DEST="" + _lock_sweep_dead "$lock_dir" + return 0 +} + +# Remove an OWNERLESS lock directory, and let exactly one contender do it. +# Returns 0 only if this process removed it. +# +# This is the deadline path only: a lock with no owner record at all belongs to a +# holder that died between its mkdir and its record write, and there is no record +# to take over, so the directory itself has to go. _lock_takeover handles every +# lock that has a record. +# +# The removal is claimed by the rename for the same reason as above: 'mv' onto a +# name that does not exist succeeds for exactly one contender. $2 is the record +# expected inside, which is the empty string here; a directory that has acquired +# a real owner record since the caller tested it fails that check and is rebuilt +# for its owner rather than deleted. +_lock_break() { + # Two 'local's, not one: within a single 'local', $lock_dir is not yet in + # scope, so 'dead' would be built from whatever the caller's scope held. + local lock_dir="$1" want="$2" got + local dead="$lock_dir.dead.$$" + mv "$lock_dir" "$dead" 2>/dev/null || return 1 + # Registered between the rename and the removal so the traps clear it: a + # renamed-aside directory must not outlive the process that claimed it. + _LOCK_DEAD="$dead" + _lock_read_owner "$dead/owner" + got="$_LOCK_OWNER_LINE" + if [[ "$got" != "$want" ]]; then + # Rebuilt with mkdir rather than renamed back: 'mv "$dead" "$lock_dir"' onto + # a path a third contender has already taken moves the directory INSIDE it, + # leaving debris in a live lock. mkdir simply fails when the path is taken, + # and a lock is addressed by path and not by inode, so a rebuilt directory + # holding the same record is the same lock to every reader. + if mkdir "$lock_dir" 2>/dev/null; then + printf '%s\n' "$got" > "$lock_dir/owner" + fi + rm -rf "$dead" + _LOCK_DEAD="" + return 1 + fi + rm -rf "$dead" + _LOCK_DEAD="" + _lock_sweep_dead "$lock_dir" + return 0 +} + # Take the lock for a shared file. Waits up to LOCK_WAIT_SECONDS, then fails # with the holder's identity and the command that clears the lock by hand. # -# Before it breaks a stale lock, the contender confirms the verdict twice, -# LOCK_POLL_SECONDS apart, with an unchanged owner record, so a lock that -# another contender took in between survives. The residual window is the few -# microseconds between the second check and the removal, and it opens only -# once a holder has already died. +# Before it claims a stale lock, the contender confirms the verdict twice, +# LOCK_POLL_SECONDS apart, with an unchanged owner record, so a lock that another +# contender took in between survives. That narrows the race; it does not close +# it, because the second check and the claim are still two separate steps. +# _lock_takeover is what closes it, by verifying after the claim what it actually +# took, and the confirm-twice pass is kept because it keeps almost every +# contender from reaching that point at all. lock_acquire() { local target="$1" lock_dir="$1.lock" deadline snapshot broke_once=false deadline=$(( SECONDS + LOCK_WAIT_SECONDS )) @@ -138,25 +309,48 @@ lock_acquire() { return 0 fi if _lock_is_stale "$lock_dir"; then - snapshot=$(_lock_owner "$lock_dir") + _lock_read_owner "$lock_dir/owner" + snapshot="$_LOCK_OWNER_LINE" sleep "$LOCK_POLL_SECONDS" - if _lock_is_stale "$lock_dir" && [[ "$(_lock_owner "$lock_dir")" == "$snapshot" ]]; then - rm -rf "$lock_dir" + # A successful takeover means this process now holds the lock, so return + # rather than looping back to mkdir a path that is deliberately still + # occupied. Losing the claim is the normal outcome for every contender but + # one, and is not an error. + if _lock_is_stale "$lock_dir"; then + _lock_read_owner "$lock_dir/owner" + if [[ "$_LOCK_OWNER_LINE" == "$snapshot" ]] && _lock_takeover "$lock_dir" "$snapshot"; then + return 0 + fi fi - continue + # Falls through to the deadline check rather than looping straight back. + # A 'continue' here skipped that check entirely, so a lock that looked + # stale but could never be claimed spun forever with no message and no + # timeout. Timing out and naming the holder is the recoverable failure. fi if (( SECONDS >= deadline )); then # An owner record still missing after the full wait belongs to a holder # that died between its mkdir and its record write: no live holder stays # in that state for seconds. Break it once, then wait again. + # + # This path has no owner record to confirm against, so the single-winner + # rename is the only thing standing between two contenders that reach + # their deadlines together and a double removal. The budget is spent only + # on a claim that succeeded: a contender that loses the race has removed + # nothing, so it keeps its one break for the next deadline. It cannot + # loop, because the winner's own owner record makes '! -s owner' false. + # The expected record is the empty string, which is what makes this path + # safe too: a lock that has acquired a real owner record since the test + # above fails the verification inside _lock_break and is handed back. if [[ "$broke_once" == false && -d "$lock_dir" && ! -s "$lock_dir/owner" ]]; then - broke_once=true - rm -rf "$lock_dir" + if _lock_break "$lock_dir" ""; then + broke_once=true + fi deadline=$(( SECONDS + LOCK_WAIT_SECONDS )) continue fi + _lock_read_owner "$lock_dir/owner" die "Timed out after ${LOCK_WAIT_SECONDS}s waiting for the lock on $target. -Holder (host pid epoch): $(_lock_owner "$lock_dir") +Holder (host pid epoch): $_LOCK_OWNER_LINE If no other claude-team command is running, remove the lock: rm -rf $lock_dir" fi @@ -171,7 +365,13 @@ lock_release() { [[ -n "$_LOCK_DIR" ]] || return 0 local dir="$_LOCK_DIR" owner_pid _LOCK_DIR="" - owner_pid=$(awk '{print $2; exit}' "$dir/owner" 2>/dev/null || true) + # Fork-free, like every other read of this record: 'awk' in a command + # substitution costs two forks between the read and the removal below, and that + # gap is the window in which the directory could stop being ours. The record is + # "host pid epoch", written by this file, so the middle field is the PID. + _lock_read_owner "$dir/owner" + owner_pid="${_LOCK_OWNER_LINE#* }" + owner_pid="${owner_pid%% *}" if [[ "$owner_pid" == "$$" ]]; then rm -rf "$dir" fi @@ -181,7 +381,20 @@ lock_release() { # Runs on a normal exit, on die, on a set -e failure, and on Ctrl-C. Nothing # runs on SIGKILL or power loss; lock_acquire's staleness rule covers those. _cleanup() { + # lock_release first. If this process had already taken the lock over, that + # removes the whole directory and the claimed record goes with it, and the + # restore below then fails harmlessly. Restoring first would instead leave a + # directory whose record names someone else, which lock_release must not touch. lock_release + if [[ -n "$_LOCK_CLAIMED" ]]; then + mv "$_LOCK_CLAIMED" "$_LOCK_CLAIMED_DEST" 2>/dev/null || true + _LOCK_CLAIMED="" + _LOCK_CLAIMED_DEST="" + fi + if [[ -n "$_LOCK_DEAD" ]]; then + rm -rf "$_LOCK_DEAD" + _LOCK_DEAD="" + fi if [[ -n "$_TMP_FILE" ]]; then rm -f "$_TMP_FILE" _TMP_FILE="" @@ -192,21 +405,29 @@ trap _cleanup EXIT trap '_cleanup; exit 130' INT trap '_cleanup; exit 143' TERM -# Create a temp file in the target's own directory and register it for -# cleanup. mktemp otherwise writes to TMPDIR, which is often tmpfs and so a -# different filesystem from $HOME. mv across filesystems degrades to copy plus -# unlink, which is not atomic: a reader can see a partial file, and an -# interrupted copy leaves one behind. +# Create a temp file in the target's own directory and register it in +# _TMP_FILE for the cleanup traps. mktemp otherwise writes to TMPDIR, which is +# often tmpfs and so a different filesystem from $HOME. mv across filesystems +# degrades to copy plus unlink, which is not atomic: a reader can see a partial +# file, and an interrupted copy leaves one behind. +# +# The path is returned in _TMP_FILE and NOT printed. Every caller used to read +# it as tmp=$(tmp_beside ...), and command substitution runs the function in a +# subshell, so the assignment landed in a child that then exited: the parent's +# _TMP_FILE stayed empty and _cleanup had nothing to remove. The registration +# this comment promised never happened, and a SIGINT during a rewrite left a +# full-size orphan beside CLAUDE.md. Callers must read _TMP_FILE directly. tmp_beside() { local target="$1" dir dir=$(dirname "$target") mkdir -p "$dir" _TMP_FILE=$(mktemp "$dir/.claude-team.XXXXXX") - printf '%s\n' "$_TMP_FILE" } # Put a temp file from tmp_beside in place. Source and target share a # filesystem, so this is a rename: readers see the old file or the new one. +# The registration is cleared only after the rename has succeeded, so a failing +# mv leaves the temp file registered and the traps remove it. tmp_commit() { mv "$1" "$2" _TMP_FILE="" @@ -234,8 +455,24 @@ resolve_name() { # Title parsing. Profile titles use the form "# Name — Role"; both parts # split at the FIRST em dash so a role may itself contain one. +# +# Prints nothing when the file has no H1, instead of failing. The old form was +# 'grep -m1 | sed', and under 'set -euo pipefail' the grep's non-zero exit on no +# match killed the whole command: one stray '.md' in PROFILES_DIR, a notes file +# or an editor backup, made 'list' exit 1 having printed zero personas with +# empty stderr, and made 'help' exit 0 with a silently empty roster, because +# persona_roster runs inside process substitution where the failure is +# invisible. Callers must treat an empty title as "not a persona profile". profile_title() { - grep -m1 '^# ' "$1" | sed 's/^# //' + local line + line=$(grep -m1 '^# ' "$1" 2>/dev/null) || return 0 + printf '%s\n' "${line#\# }" +} + +# Name the file that was skipped. The failure this replaces was silent, and the +# user cannot act on a roster that is quietly short unless the cause is named. +warn_not_a_profile() { + echo "$(yellow "!") Skipping $1: no '# Name — Role' heading, so it is not a persona profile." >&2 } title_name() { @@ -267,6 +504,10 @@ persona_roster() { name=$(basename "$profile" .md) case "$name" in coordinator*) continue ;; esac title=$(profile_title "$profile") + if [[ -z "$title" ]]; then + warn_not_a_profile "$profile" + continue + fi printf '%s\t%s\t%s\n' "$name" "$(title_name "$title")" "$(short_role "$(title_role "$title")")" done } @@ -327,7 +568,8 @@ block_install() { touch "$file" block_assert_sane "$file" "$start" "$end" "block" local tmp - tmp=$(tmp_beside "$file") + tmp_beside "$file" + tmp="$_TMP_FILE" if grep -qF "$start" "$file"; then awk -v m="$start" 'index($0, m){exit} {print}' "$file" > "$tmp" printf '%s\n%s\n%s\n' "$start" "$content" "$end" >> "$tmp" @@ -350,7 +592,8 @@ block_remove() { lock_acquire "$file" block_assert_sane "$file" "$start" "$end" "block" local tmp - tmp=$(tmp_beside "$file") + tmp_beside "$file" + tmp="$_TMP_FILE" awk -v s="$start" -v e="$end" ' index($0, s){skip=1} skip && index($0, e){skip=0; next} @@ -380,9 +623,15 @@ cmd_list() { local name name=$(basename "$profile" .md) [[ "$name" == coordinator* ]] && continue # coordinator profiles are not team members - # Extract the title line (first # heading) and the role after the dash + # Extract the title line (first # heading) and the role after the dash. + # A file with no H1 is not a persona profile: name it and move on, rather + # than aborting the whole listing on it. local title role title=$(profile_title "$profile") + if [[ -z "$title" ]]; then + warn_not_a_profile "$profile" + continue + fi role=$(title_role "$title") local name_cap @@ -430,6 +679,10 @@ cmd_use() { local display_name display_name=$(title_name "$(profile_title "$profile")") + # A profile with no '# Name — Role' heading yields an empty title. The block + # was still installed, so name what was activated rather than printing a + # checkmark with a blank name. + [[ -n "$display_name" ]] || display_name=$(capitalize "$name") echo "" echo "$(green "✓") $(bold "$display_name") is now active." @@ -720,7 +973,8 @@ cmd_branch_close() { [[ -n "$active" ]] || die "No active branch found for '$project'. Nothing to close." local tmp - tmp=$(tmp_beside "$BRANCHES_INDEX") + tmp_beside "$BRANCHES_INDEX" + tmp="$_TMP_FILE" awk -F'|' -v proj="$project" -v ns="$new_status" ' { p = $3; st = $6 @@ -806,7 +1060,8 @@ cmd_branch_guard() { || die "A pre-commit hook already exists at $hook_file (not installed by claude-team). Remove it manually first." fi local tmp_hook - tmp_hook=$(tmp_beside "$hook_file") + tmp_beside "$hook_file" + tmp_hook="$_TMP_FILE" cat > "$tmp_hook" << 'HOOK' #!/usr/bin/env bash # Installed by claude-team branch guard @@ -1134,7 +1389,8 @@ Run this from inside a session worktree, or use 'claude-team branch done' for no # row another session appends in between is not discarded. lock_acquire "$BRANCHES_INDEX" local tmp - tmp=$(tmp_beside "$BRANCHES_INDEX") + tmp_beside "$BRANCHES_INDEX" + tmp="$_TMP_FILE" awk -F'|' -v proj="$project" -v br="$branch" ' { p = $3; b = $4; st = $6 @@ -1236,6 +1492,30 @@ cmd_session() { esac } +# Stop a sync that copied only part of a persona, and say what state that left. +# +# The three surfaces are one unit: a persona is a profile, a slash command, and +# a subagent, and the whole point of this command is to move them together. Three +# sequential 'cp' batches used to run unchecked, so a failure between batches +# left the profiles new while agents/ and commands/ still held the old version, +# at exit 0 and with a green checkmark on the batch that did land. That is the +# exact split the command exists to prevent, and it is the one state nothing +# else warns about. +# +# Nothing is rolled back. A partial sync is repaired by finishing it, not by +# reverting to a different partial state, and a rollback that fails part-way +# leaves a third state to reason about. +sync_die() { + die "$1 +The sync stopped part-way, so these three surfaces may now hold different versions: + profiles $PROFILES_DIR + subagents $HOME/.claude/agents + slash commands $HOME/.claude/commands +A persona is all three together, so treat this as one broken install, not two +good surfaces and one stale one. Fix the error above, then rerun +'claude-team sync': it recopies all three and brings them back level." +} + # Regenerate agents from profiles, then copy all three installed surfaces from # the clone into ~/.claude. A persona exists as three self-contained files # (profile, slash command, subagent), so editing one installed copy leaves the @@ -1262,20 +1542,24 @@ Make sure you are running this from the claude-team-cli repo." fi mkdir -p "$PROFILES_DIR" - cp "$repo_dir/profiles"/*.md "$PROFILES_DIR/" + cp "$repo_dir/profiles"/*.md "$PROFILES_DIR/" \ + || sync_die "Failed to copy profiles into $PROFILES_DIR." if [[ -f "$repo_dir/profiles/tiers.conf" ]]; then - cp "$repo_dir/profiles/tiers.conf" "$PROFILES_DIR/" + cp "$repo_dir/profiles/tiers.conf" "$PROFILES_DIR/" \ + || sync_die "Failed to copy tiers.conf into $PROFILES_DIR." fi echo "$(green "✓") Profiles synced to $(dim "$PROFILES_DIR")" if [[ -d "$repo_dir/agents" ]]; then mkdir -p "$agents_dst" - cp "$repo_dir/agents"/*.md "$agents_dst/" + cp "$repo_dir/agents"/*.md "$agents_dst/" \ + || sync_die "Failed to copy subagents into $agents_dst." echo "$(green "✓") Subagents synced to $(dim "$agents_dst")" fi mkdir -p "$commands_dst" - cp "$repo_dir/commands"/*.md "$commands_dst/" + cp "$repo_dir/commands"/*.md "$commands_dst/" \ + || sync_die "Failed to copy slash commands into $commands_dst." echo "$(green "✓") Slash commands synced to $(dim "$commands_dst")" say_session_scope echo "" diff --git a/install.sh b/install.sh index a5fa7cd..077cddb 100755 --- a/install.sh +++ b/install.sh @@ -99,8 +99,24 @@ echo " $(bold "casual") (default): commit directly to main — no branch enforc echo " $(bold "prod"): branch required before any code; worktrees + MR/PR flow." echo "" printf " Enable the coordinator now? [casual/prod/n] (default: casual) " -read -r coord_answer -coord_answer="${coord_answer:-casual}" +# A bare 'read' returns non-zero at end of file, and under 'set -e' that ended +# the install right here: 'bash install.sh < /dev/null', a CI job, or any pipe +# exited 1 mid-prompt with no summary and the coordinator unconfigured, after +# every earlier step had already been applied. +# +# End of file is not the same as pressing Enter. Enter is a person choosing the +# default; end of file means nobody is present to choose, and the casual path +# writes a block into the user's global ~/.claude/CLAUDE.md. So Enter still +# means casual, and end of file skips the coordinator rather than editing a +# global file unattended. The user can enable it later in one command. +coord_answer="" +if read -r coord_answer; then + coord_answer="${coord_answer:-casual}" +else + echo "" + echo " $(yellow "!") No answer on stdin (end of file), so the coordinator stays off." + coord_answer="n" +fi # Delegate to the CLI just installed: it owns the marker-block editing # (atomic replace-or-append), so the logic lives in exactly one place and diff --git a/tests/run.sh b/tests/run.sh index bb6467b..f112891 100644 --- a/tests/run.sh +++ b/tests/run.sh @@ -84,7 +84,14 @@ assert_file_lacks() { assert_count() { local name="$1" file="$2" pattern="$3" expected="$4" local count - count=$(grep -c "$pattern" "$file" 2>/dev/null || echo 0) + # 'grep -c' prints 0 and ALSO exits 1 when nothing matches, so a trailing + # '|| echo 0' appended a second zero and made count the two-line string + # '0\n0'. Bash then failed to evaluate it as arithmetic, so an assert_count + # with an expected 0 could never pass and reported a syntax error instead of a + # comparison. Take grep's own 0 and only default when it printed nothing, + # which is what a missing file does. + count=$(grep -c "$pattern" "$file" 2>/dev/null) + count=${count:-0} if [[ "$count" -eq "$expected" ]]; then ok "$name" else fail "$name (expected $expected x '$pattern', got $count)"; fi } @@ -411,6 +418,58 @@ assert_contains "multi-dash title keeps the full role" "Role One — Extended" " rm -rf "$TITLE_DIR" echo "" +# One stray '.md' with no H1 heading used to take out the whole roster. +# profile_title was 'grep -m1 | sed', and under 'set -euo pipefail' the grep's +# non-zero exit on no match killed the pipeline: 'list' exited 1 having printed +# zero personas, with EMPTY stderr, and 'help' exited 0 with a silently empty +# roster, because persona_roster runs inside process substitution where the +# failure is invisible. A notes file or an editor backup in the profiles +# directory was enough, and neither symptom said why. +echo "a titleless file in the profiles directory" + +STRAY_DIR=$(mktemp -d) +cp "$PROFILES_DIR"/*.md "$STRAY_DIR/" +cp "$PROFILES_DIR/tiers.conf" "$STRAY_DIR/" 2>/dev/null || true +printf -- '- a scratch note with no H1 heading\n- second line\n' > "$STRAY_DIR/notes.md" +STRAY_ERR="$TEST_HOME/stray-list-stderr" +STRAY_HELP_ERR="$TEST_HOME/stray-help-stderr" + +if stray_out=$(CLAUDE_TEAM_PROFILES="$STRAY_DIR" HOME="$TEST_HOME" "$CLI" list 2>"$STRAY_ERR"); then + ok "list still exits 0 with a titleless file present" +else + fail "list still exits 0 with a titleless file present" +fi +stray_missing="" +for profile in "$PROFILES_DIR"/*.md; do + pname=$(basename "$profile" .md) + case "$pname" in coordinator*) continue ;; esac + grep -qi "$pname" <<< "$stray_out" || stray_missing="$stray_missing $pname" +done +if [[ -z "$stray_missing" ]]; then ok "list still shows every real persona" +else fail "list still shows every real persona (missing:$stray_missing)"; fi +assert_file_has "list names the file it skipped" "$STRAY_ERR" "notes.md" +assert_count "list names the skipped file once" "$STRAY_ERR" "notes.md" 1 + +if stray_help=$(CLAUDE_TEAM_PROFILES="$STRAY_DIR" HOME="$TEST_HOME" "$CLI" help 2>"$STRAY_HELP_ERR"); then + ok "help still exits 0 with a titleless file present" +else + fail "help still exits 0 with a titleless file present" +fi +stray_missing="" +for profile in "$PROFILES_DIR"/*.md; do + pname=$(basename "$profile" .md) + case "$pname" in coordinator*) continue ;; esac + grep -q "claude-team use $pname " <<< "$stray_help" || stray_missing="$stray_missing $pname" +done +if [[ -z "$stray_missing" ]]; then ok "help's roster is not silently truncated" +else fail "help's roster is not silently truncated (missing:$stray_missing)"; fi +assert_file_has "help names the file it skipped" "$STRAY_HELP_ERR" "notes.md" +# The skip must not swallow a real persona whose title merely sits lower down. +assert_not_contains "the titleless file is not listed as a persona" "notes" "$stray_out" +rm -rf "$STRAY_DIR" +rm -f "$STRAY_ERR" "$STRAY_HELP_ERR" +echo "" + # branch commands echo "branch commands" @@ -773,6 +832,166 @@ fi rm -rf "$DMG_HOME" echo "" +# Lock breaking. The CLI derives its host field from bash's own HOSTNAME, so +# these fixtures compute it the same way; a mismatch would make every planted +# lock look like it came from another machine and silently change which rule +# applies to it. +LOCK_HOST="${HOSTNAME:-$(uname -n 2>/dev/null || echo unknown)}" + +# A PID that is certainly dead: start a child, reap it, then reuse its number. +# Linux and macOS both allocate PIDs increasing, so the number is not reissued +# within the life of this suite. +dead_pid() { + local p + sh -c 'exit 0' & p=$! + wait "$p" 2>/dev/null + printf '%s\n' "$p" +} + +# Breaking a stale lock must have exactly one winner, and the way that is +# achieved is observable: a stale lock is taken over IN PLACE, by renaming its +# owner record aside, and the directory itself is never removed. +# +# Removing the directory is what could not be made safe, whichever way the +# removal was claimed, because removing it frees the lock path and a mkdir can +# land in that gap. Measured at 12 concurrent writers: 'rm -rf' lost updates in 6 +# of 30 runs, claiming the removal by renaming the directory aside still lost +# them in 5 of 30, and adding a verify-and-rebuild cut it to 1 in 30. Taking over +# in place measured 0 in 40. +# +# A sentinel file inside the planted lock is what separates the two designs. If +# the lock was taken over in place, the sentinel is still there while the new +# owner holds it. If the directory was deleted and recreated, it is gone. +echo "lock breaking has a single winner" + +BREAK_HOME=$(mktemp -d) +mkdir -p "$BREAK_HOME/.claude" +BREAK_MD="$BREAK_HOME/.claude/CLAUDE.md" +BREAK_LOCK="$BREAK_MD.lock" +# About 2 MB, so the new owner holds the lock for long enough to look at. The +# poll below spins in microseconds, so this only has to beat process startup. +break_line=$(printf '%*s' 4096 '' | tr ' ' 'x') +for ((_i = 0; _i < 500; _i++)); do printf '%s\n' "$break_line"; done > "$BREAK_MD" +printf 'keep-this-line\n' >> "$BREAK_MD" +mkdir -p "$BREAK_LOCK" +: > "$BREAK_LOCK/sentinel" +break_dead=$(dead_pid) +printf '%s %s %s\n' "$LOCK_HOST" "$break_dead" "$(date '+%s')" > "$BREAK_LOCK/owner" + +( CLAUDE_TEAM_PROFILES="$PROFILES_DIR" HOME="$BREAK_HOME" "$CLI" use akira >/dev/null 2>&1 ) & +break_pid=$! +break_saw_takeover=false +break_in_place=false +break_deadline=$((SECONDS + 20)) +while (( SECONDS < break_deadline )); do + break_owner="" + 2>/dev/null read -r break_owner < "$BREAK_LOCK/owner" || true + # A record that no longer names the planted dead PID means the takeover is + # done. It is briefly absent mid-claim, which is why emptiness is not the test. + if [[ -n "$break_owner" && "$break_owner" != *" $break_dead "* ]]; then + break_saw_takeover=true + if [[ -e "$BREAK_LOCK/sentinel" ]]; then break_in_place=true; fi + break + fi + # Nothing left to observe once the write has landed. Without this the loop + # would spin to its deadline on a build that never takes the lock over. + if grep -qF "CLAUDE-TEAM:START" "$BREAK_MD" 2>/dev/null; then break; fi +done +wait "$break_pid" 2>/dev/null +if [[ "$break_saw_takeover" == true ]]; then + ok "the takeover was observed, so this test is not vacuous" +else + fail "the takeover was observed, so this test is not vacuous (never saw the record change)" +fi +if [[ "$break_in_place" == true ]]; then + ok "a stale lock is taken over in place, never deleted and recreated" +else + fail "a stale lock is taken over in place, never deleted and recreated" +fi +assert_file_has "the contender still gets the lock and writes" "$BREAK_MD" "CLAUDE-TEAM:START" +assert_file_has "taking over a lock preserves user content" "$BREAK_MD" "keep-this-line" +if [[ ! -d "$BREAK_LOCK" ]]; then ok "no lock directory remains after a takeover" +else fail "no lock directory remains after a takeover"; fi +break_left=$(find "$BREAK_HOME/.claude" -maxdepth 1 -name 'CLAUDE.md.lock*' 2>/dev/null | wc -l | tr -d ' ') +if [[ "$break_left" == "0" ]]; then ok "a takeover leaves no lock litter beside the target" +else fail "a takeover leaves no lock litter beside the target (found $break_left)"; fi +rm -rf "$BREAK_HOME" + +# SIGKILL between the rename and the removal is the one case the traps cannot +# cover, and it leaves an inert '.dead.' directory that nothing +# reads. The next break must sweep it, or a machine that crashes under load +# accumulates them beside the user's CLAUDE.md forever. +SWEEP_HOME=$(mktemp -d) +mkdir -p "$SWEEP_HOME/.claude" +SWEEP_MD="$SWEEP_HOME/.claude/CLAUDE.md" +SWEEP_LOCK="$SWEEP_MD.lock" +printf 'keep-this-line\n' > "$SWEEP_MD" +mkdir -p "$SWEEP_LOCK" "$SWEEP_LOCK.dead.999001" "$SWEEP_LOCK.dead.999002/nested" +printf 'leftover owner record\n' > "$SWEEP_LOCK.dead.999001/owner" +printf '%s %s %s\n' "$LOCK_HOST" "$(dead_pid)" "$(date '+%s')" > "$SWEEP_LOCK/owner" +CLAUDE_TEAM_PROFILES="$PROFILES_DIR" HOME="$SWEEP_HOME" "$CLI" use akira >/dev/null 2>&1 +sweep_left=$(find "$SWEEP_HOME/.claude" -maxdepth 1 -name 'CLAUDE.md.lock.dead.*' 2>/dev/null | wc -l | tr -d ' ') +if [[ "$sweep_left" == "0" ]]; then ok "a break sweeps renamed-aside directories a killed winner left behind" +else fail "a break sweeps renamed-aside directories a killed winner left behind (found $sweep_left)"; fi +assert_file_has "the write still lands alongside the sweep" "$SWEEP_MD" "CLAUDE-TEAM:START" +rm -rf "$SWEEP_HOME" +echo "" + +# Liveness must outrank the age rule. The 120-second age test used to run first +# and independently of kill -0, so a live holder that had been paused lost a lock +# it still held and both writers then reported success on a write only one of +# them kept. SIGSTOP, a laptop suspend mid-command, and a clock stepping forward +# all produce exactly that state. +echo "liveness outranks the lock age rule" + +LIVE_HOME=$(mktemp -d) +mkdir -p "$LIVE_HOME/.claude" +LIVE_MD="$LIVE_HOME/.claude/CLAUDE.md" +LIVE_LOCK="$LIVE_MD.lock" +printf 'keep-this-line\n' > "$LIVE_MD" +mkdir -p "$LIVE_LOCK" +sleep 30 & +live_holder=$! +# Acquired far beyond LOCK_STALE_SECONDS, by a process that is still running. +printf '%s %s %s\n' "$LOCK_HOST" "$live_holder" "$(( $(date '+%s') - 100000 ))" > "$LIVE_LOCK/owner" +( CLAUDE_TEAM_PROFILES="$PROFILES_DIR" HOME="$LIVE_HOME" "$CLI" use akira >/dev/null 2>&1 ) & +live_contender=$! +# One second is twenty poll intervals. The age-first order broke this lock on +# the first one. +sleep 1 +if [[ -d "$LIVE_LOCK" ]] && [[ "$(cat "$LIVE_LOCK/owner" 2>/dev/null)" == *" $live_holder "* ]]; then + ok "an ancient lock held by a live PID is not broken" +else + fail "an ancient lock held by a live PID is not broken (the contender took it)" +fi +assert_file_lacks "the contender writes nothing while a live holder holds the lock" \ + "$LIVE_MD" "CLAUDE-TEAM:START" +kill "$live_contender" 2>/dev/null +wait "$live_contender" 2>/dev/null +kill "$live_holder" 2>/dev/null +wait "$live_holder" 2>/dev/null +rm -rf "$LIVE_HOME" + +# The age rule still has to work where liveness cannot be established, which is +# the case it exists for: a lock recorded by another host, as happens when $HOME +# is shared over NFS and that machine crashed. The PID here is deliberately a +# LIVE one, so this fails if the host check ever stops gating the kill -0. +FOREIGN_HOME=$(mktemp -d) +mkdir -p "$FOREIGN_HOME/.claude" +FOREIGN_MD="$FOREIGN_HOME/.claude/CLAUDE.md" +printf 'keep-this-line\n' > "$FOREIGN_MD" +mkdir -p "$FOREIGN_MD.lock" +printf 'some-other-host %s %s\n' "$$" "$(( $(date '+%s') - 100000 ))" > "$FOREIGN_MD.lock/owner" +if CLAUDE_TEAM_PROFILES="$PROFILES_DIR" HOME="$FOREIGN_HOME" "$CLI" use akira >/dev/null 2>&1; then + ok "an ancient lock from another host is still broken by the age rule" +else + fail "an ancient lock from another host is still broken by the age rule" +fi +assert_file_has "the write lands after breaking another host's ancient lock" \ + "$FOREIGN_MD" "CLAUDE-TEAM:START" +rm -rf "$FOREIGN_HOME" +echo "" + # Shared-state concurrency. ~/.claude/branches/INDEX.md and ~/.claude/CLAUDE.md # are shared across every session on the machine, and parallel sessions are the # product's headline feature, so concurrent access is the designed-for case. @@ -825,6 +1044,64 @@ assert_count "concurrent writers leave one team block" "$CONC_MD" "CLAUDE assert_count "concurrent writers leave one coordinator block" "$CONC_MD" "CLAUDE-COORDINATOR:START" 1 assert_count "concurrent writers keep user content" "$CONC_MD" "keep-this-line" 1 rm -rf "$CONC_HOME" + +# The same storm, but starting from a stale lock, which is the state that made +# lock breaking the dangerous part: every contender arrives, agrees the holder is +# gone, and races to remove. Every writer here is a rewriter with its own +# distinct transition to apply, so any two contenders that end up inside the +# critical section together must lose one of them. +# +# This is a probabilistic detector, and deliberately kept as one. The window +# between a contender's staleness verdict and its removal is microseconds wide, +# so on the unfixed code this storm reproduced the lost update in roughly 1 run +# in 6 at this size, and more often under CPU load. It is here to guard the +# end-to-end invariant that no row is ever lost; the deterministic proofs of the +# single-winner rename are in the 'lock breaking has a single winner' section. +STALE_HOME=$(mktemp -d) +STALE_INDEX="$STALE_HOME/.claude/branches/INDEX.md" +mkdir -p "$STALE_HOME/.claude" +STALE_ROOT=$(mktemp -d) +for i in 1 2 3 4 5 6 7 8 9 10 11 12; do + git init -q "$STALE_ROOT/r$i" + (cd "$STALE_ROOT/r$i" && git commit -q --allow-empty -m init) + (cd "$STALE_ROOT/r$i" && CLAUDE_TEAM_PROFILES="$PROFILES_DIR" HOME="$STALE_HOME" "$CLI" branch start "feat/s$i" >/dev/null 2>&1) +done +mkdir -p "$STALE_INDEX.lock" +printf '%s %s %s\n' "$LOCK_HOST" "$(dead_pid)" "$(date '+%s')" > "$STALE_INDEX.lock/owner" +for i in 1 2 3 4 5 6 7 8 9 10 11 12; do + (cd "$STALE_ROOT/r$i" && CLAUDE_TEAM_PROFILES="$PROFILES_DIR" HOME="$STALE_HOME" "$CLI" branch "done" >/dev/null 2>&1) & +done +wait +# merged == 12 with rows == 12 is what pins it: a discarded rewrite leaves its +# row behind as 'active', so the row count alone would not move. +assert_count "a stale lock plus 12 writers keeps every row" "$STALE_INDEX" '^| 2' 12 +assert_count "a stale lock plus 12 writers applies every rewrite" "$STALE_INDEX" '| merged |' 12 +assert_file_has "a stale lock plus 12 writers keeps the header" "$STALE_INDEX" "Branch Index" +if [[ ! -d "$STALE_INDEX.lock" ]]; then ok "the storm leaves no lock directory behind" +else fail "the storm leaves no lock directory behind"; fi +stale_left=$(find "$STALE_HOME/.claude/branches" -maxdepth 1 -name 'INDEX.md.lock.dead.*' 2>/dev/null | wc -l | tr -d ' ') +if [[ "$stale_left" == "0" ]]; then ok "the storm leaves no renamed-aside lock behind" +else fail "the storm leaves no renamed-aside lock behind (found $stale_left)"; fi +if [[ -z "$(find "$STALE_HOME/.claude" -name '.claude-team.*' 2>/dev/null)" ]]; then + ok "the storm leaves no temp file behind" +else fail "the storm leaves no temp file behind"; fi +rm -rf "$STALE_ROOT" + +# CLAUDE.md under the same conditions: a stale lock, then writers on both of its +# independent marker blocks. +STALE_MD="$STALE_HOME/.claude/CLAUDE.md" +printf 'keep-this-line\n' > "$STALE_MD" +mkdir -p "$STALE_MD.lock" +printf '%s %s %s\n' "$LOCK_HOST" "$(dead_pid)" "$(date '+%s')" > "$STALE_MD.lock/owner" +for _ in 1 2 3 4 5; do + (CLAUDE_TEAM_PROFILES="$PROFILES_DIR" HOME="$STALE_HOME" "$CLI" use akira >/dev/null 2>&1) & + (CLAUDE_TEAM_PROFILES="$PROFILES_DIR" HOME="$STALE_HOME" "$CLI" coordinator on >/dev/null 2>&1) & +done +wait +assert_count "a stale lock plus writers leaves one team block" "$STALE_MD" "CLAUDE-TEAM:START" 1 +assert_count "a stale lock plus writers leaves one coordinator block" "$STALE_MD" "CLAUDE-COORDINATOR:START" 1 +assert_count "a stale lock plus writers keeps user content" "$STALE_MD" "keep-this-line" 1 +rm -rf "$STALE_HOME" echo "" # Temp files must be created beside their destination, not in TMPDIR. A @@ -861,6 +1138,51 @@ else fail "no temp file litter left in ~/.claude"; fi rm -rf "$TMP_HOME" "$TMP_REPO" echo "" +# An interrupted write must not leave its temp file behind. tmp_beside documents +# that it registers the file in _TMP_FILE for the cleanup traps, but every caller +# read it as tmp=$(tmp_beside ...), and command substitution runs the function in +# a subshell: the assignment landed in a child that then exited, the parent's +# _TMP_FILE stayed empty, and _cleanup could never remove anything. Reproduced as +# a 150 MB orphan beside CLAUDE.md after a SIGINT mid-rewrite. +echo "an interrupted write leaves no temp file behind" + +INT_HOME=$(mktemp -d) +mkdir -p "$INT_HOME/.claude" +INT_MD="$INT_HOME/.claude/CLAUDE.md" +# A CLAUDE.md of about 8 MB. Bash defers a trap until the foreground child it is +# waiting on returns, so the signal only has to arrive before the rename, not +# inside a narrow window; the size is what makes the copy into the temp file long +# enough for that to be comfortable rather than tight. +int_line=$(printf '%*s' 4096 '' | tr ' ' 'x') +for ((_i = 0; _i < 2000; _i++)); do printf '%s\n' "$int_line"; done > "$INT_MD" + +( CLAUDE_TEAM_PROFILES="$PROFILES_DIR" HOME="$INT_HOME" "$CLI" use akira >/dev/null 2>&1 ) & +int_pid=$! +int_saw_tmp=false +int_deadline=$((SECONDS + 20)) +while (( SECONDS < int_deadline )); do + for _t in "$INT_HOME/.claude"/.claude-team.*; do + if [[ -f "$_t" ]]; then int_saw_tmp=true; break 2; fi + done + if grep -qF "CLAUDE-TEAM:START" "$INT_MD" 2>/dev/null; then break; fi +done +if [[ "$int_saw_tmp" == true ]]; then + kill -INT "$int_pid" 2>/dev/null + ok "the temp file window was observed, so this test is not vacuous" +else + fail "the temp file window was observed, so this test is not vacuous (the write finished first: enlarge the fixture)" +fi +wait "$int_pid" 2>/dev/null +# Proves the interrupt landed before the rename. Without it, a signal arriving +# after tmp_commit would leave no orphan on any build and the check below would +# pass for the wrong reason. +assert_file_lacks "the interrupted write did not land" "$INT_MD" "CLAUDE-TEAM:START" +int_orphans=$(find "$INT_HOME/.claude" -maxdepth 1 -name '.claude-team.*' 2>/dev/null | wc -l | tr -d ' ') +if [[ "$int_orphans" == "0" ]]; then ok "SIGINT mid-write leaves no temp file beside the target" +else fail "SIGINT mid-write leaves no temp file beside the target (found $int_orphans)"; fi +rm -rf "$INT_HOME" +echo "" + # launch + plugin surfaces echo "launch + plugin surfaces" @@ -1139,6 +1461,44 @@ assert_contains "sync says slash commands are live now" "No restart neede assert_contains "sync says subagents wait for a new session" "Next session" "$out" rm -rf "$SYNC_REPO" "$SYNC_HOME" +# A sync that fails part-way must say so. Measured on the unchecked version: +# 'set -e' already stopped it and already exited 1, so the exit code and the +# stopping were never the problem. What the user got was a raw 'cp: cannot +# overwrite directory' line, a green checkmark on the batch that HAD landed, and +# nothing naming the state that leaves behind: profiles new, subagents and slash +# commands still on the previous version. That divergence is the one thing this +# command exists to prevent and the one thing nothing else warns about. +# +# So of the assertions below, the three message checks are the real regression +# controls; the exit code and the stop-before-the-next-surface checks already +# held and are kept to pin them against a future '|| true'. +# +# The failure is forced with a DIRECTORY sitting where a file has to land, which +# fails for root too. A chmod would not: this suite may run as root, and there +# the permission bits are advisory, so a chmod-based fixture would pass +# vacuously on exactly the machine most likely to run it. +SYNCFAIL_REPO=$(mktemp -d) +SYNCFAIL_HOME=$(mktemp -d) +cp -R "$REPO_DIR"/profiles "$REPO_DIR"/commands "$REPO_DIR"/agents \ + "$REPO_DIR"/scripts "$REPO_DIR"/bin "$SYNCFAIL_REPO/" +mkdir -p "$SYNCFAIL_HOME/.claude/agents/robin.md" +if out=$(HOME="$SYNCFAIL_HOME" bash "$SYNCFAIL_REPO/bin/claude-team" sync 2>&1); then + fail "sync exits nonzero when a surface cannot be copied" +else + ok "sync exits nonzero when a surface cannot be copied" +fi +assert_contains "the sync failure names the divergence" "surfaces may now hold different versions" "$out" +assert_contains "the sync failure says rerunning heals it" "rerun" "$out" +assert_contains "the sync failure names the failing copy" "Failed to copy subagents" "$out" +# Stopping is the point: continuing is what produced the divergence silently. +assert_not_contains "a failed sync stops before the next surface" "Slash commands synced" "$out" +if [[ ! -f "$SYNCFAIL_HOME/.claude/commands/robin.md" ]]; then + ok "a failed sync does not go on to install the later surface" +else + fail "a failed sync does not go on to install the later surface" +fi +rm -rf "$SYNCFAIL_REPO" "$SYNCFAIL_HOME" + echo "" # Messaging accuracy. Nothing asserted any of these strings before, so a @@ -1203,6 +1563,44 @@ if [[ "$hookrefs" == "1" ]]; then else fail "reinstall does not duplicate the SessionStart hook (found $hookrefs)" fi + +# The coordinator prompt used a bare 'read', which returns non-zero at end of +# file, and under 'set -e' that ended the installer right there: exit 1, no +# summary, coordinator unconfigured, and every earlier step already applied. +# 'bash install.sh < /dev/null' is what CI and any pipe look like. +EOF_HOME=$(mktemp -d) +if eof_out=$( (cd "$INSTALL_REPO" && HOME="$EOF_HOME" bash install.sh < /dev/null) 2>&1 ); then + ok "install.sh exits 0 with stdin at end of file" +else + fail "install.sh exits 0 with stdin at end of file" +fi +assert_contains "a non-interactive install still reaches its summary" "Done!" "$eof_out" +if [[ -f "$EOF_HOME/.claude/team/robin.md" ]]; then ok "a non-interactive install still installs profiles" +else fail "a non-interactive install still installs profiles"; fi +# End of file means nobody is present to consent, and the casual path writes a +# block into the user's own global CLAUDE.md. Skipping is the safe reading. +# This one also held before the fix, for the wrong reason: the installer died at +# the prompt and so never reached the coordinator either way. The assertions that +# actually separate the two are the exit code and the summary above. +if ! grep -qF "CLAUDE-COORDINATOR:START" "$EOF_HOME/.claude/CLAUDE.md" 2>/dev/null; then + ok "end of file skips the coordinator instead of editing the global CLAUDE.md" +else + fail "end of file skips the coordinator instead of editing the global CLAUDE.md" +fi +assert_contains "the installer says why it skipped the coordinator" "end of file" "$eof_out" +rm -rf "$EOF_HOME" + +# Pressing Enter is a person choosing the default, and must still mean casual. +# Treating end of file as the default is what this separates it from. +ENTER_HOME=$(mktemp -d) +if (cd "$INSTALL_REPO" && printf '\n' | HOME="$ENTER_HOME" bash install.sh >/dev/null 2>&1); then + ok "install.sh runs clean when the coordinator prompt gets a bare Enter" +else + fail "install.sh runs clean when the coordinator prompt gets a bare Enter" +fi +assert_file_has "a bare Enter still means casual" "$ENTER_HOME/.claude/CLAUDE.md" "CLAUDE-COORD-MODE: casual" +rm -rf "$ENTER_HOME" + rm -rf "$INSTALL_HOME" "$INSTALL_REPO" echo "" From 73619cbd6d65d641faae39a83f780cc88f64efbb Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 21:31:47 +0000 Subject: [PATCH 3/3] test: assert write coherence after SIGINT, not which side won the race The macOS job failed on "the interrupted write did not land". Linux and shellcheck passed. The assertion was testing timing rather than correctness. Its guard waits for the temp file to appear and then sends SIGINT, and a comment claims that proves the interrupt arrived before the rename. It does not: the rename can land in the microseconds between the temp file becoming visible and the signal being delivered. On macOS, where process spawn is slower and the window sits differently, the write completed first and the assertion failed on a build where nothing was wrong. Whether the rename beats the signal is not a property the design promises. What it does promise is that a reader sees the whole old file or the whole new one, never a partial write, because the temp file and its target share a filesystem and the commit is a rename. So the test now asserts coherence: the marker count is zero or one, with START and END balanced, in either outcome. It also asserts all 2000 user lines survive, since preserving the user's own content is the point of writing through a temp file at all. Verified against both outcomes directly: signal first gives 0 start and 0 end, rename first gives 1 and 1, and all 2000 lines and zero orphans in both. The orphan check is untouched and still fails against main, finding the temp file that the unregistered _TMP_FILE left behind. That is the bug this block exists for. The two replaced assertions now pass against main as well, which is correct: main never wrote a partial file, it only failed to clean up after itself. 269 tests. shellcheck clean locally, and CI is the authority on that. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019j5DHEZsoeCGRueTbNLuTb --- tests/run.sh | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/tests/run.sh b/tests/run.sh index f112891..7f900ca 100644 --- a/tests/run.sh +++ b/tests/run.sh @@ -1173,10 +1173,22 @@ else fail "the temp file window was observed, so this test is not vacuous (the write finished first: enlarge the fixture)" fi wait "$int_pid" 2>/dev/null -# Proves the interrupt landed before the rename. Without it, a signal arriving -# after tmp_commit would leave no orphan on any build and the check below would -# pass for the wrong reason. -assert_file_lacks "the interrupted write did not land" "$INT_MD" "CLAUDE-TEAM:START" +# Assert what the design actually guarantees, which is that a reader sees the +# whole old file or the whole new one, never a partial write. Whether the rename +# beat the signal is timing, not correctness: seeing the temp file appear does +# not prove the signal arrived before tmp_commit, because the rename can land in +# the microseconds between the two. An earlier version asserted the write had +# not landed and failed on macOS for exactly that reason, where process spawn is +# slower and the window sits differently. +int_starts=$(grep -cF "CLAUDE-TEAM:START" "$INT_MD" 2>/dev/null || true) +int_ends=$(grep -cF "CLAUDE-TEAM:END" "$INT_MD" 2>/dev/null || true) +if [[ "$int_starts" == "$int_ends" && ( "$int_starts" == "0" || "$int_starts" == "1" ) ]]; then + ok "an interrupted write leaves the file coherent, not half-written" +else + fail "an interrupted write leaves the file coherent, not half-written ($int_starts start, $int_ends end)" +fi +# The user's own content must survive either outcome. +assert_count "an interrupted write preserves every user line" "$INT_MD" "^$int_line$" 2000 int_orphans=$(find "$INT_HOME/.claude" -maxdepth 1 -name '.claude-team.*' 2>/dev/null | wc -l | tr -d ' ') if [[ "$int_orphans" == "0" ]]; then ok "SIGINT mid-write leaves no temp file beside the target" else fail "SIGINT mid-write leaves no temp file beside the target (found $int_orphans)"; fi