Skip to content

fix: stop session done from deleting untracked work, plus a test audit - #25

Merged
d6veteran merged 4 commits into
mainfrom
claude/robin-test-audit
Jul 29, 2026
Merged

fix: stop session done from deleting untracked work, plus a test audit#25
d6veteran merged 4 commits into
mainfrom
claude/robin-test-audit

Conversation

@d6veteran

Copy link
Copy Markdown
Collaborator

Robin audited the suite. The most important thing they found is not a test gap.

205 tests (was 175). shellcheck clean.

1. session done destroyed untracked work

cmd_session_done guarded with git diff --quiet and git diff --cached --quiet. Neither sees an untracked file. Two lines later, worktree removal retried with --force on any failure, overriding git's own refusal to remove a worktree holding untracked files.

Isolated three ways — same commands, only the file state differs:

Worktree state Before
Modified tracked file Refused, exit 1, preserved
Staged new file Refused, exit 1, preserved
Untracked new file ✓ Session closed, exit 0, worktree deleted, file gone

It then wrote merged into the branch index. Nothing was merged.

Writing a file and not staging it yet is the most ordinary state in a working session, so the destructive path was the common one.

The guard is now one git status --porcelain call, which sees staged, unstaged, and untracked together, and it lists the offending paths — a refusal you can't act on is only half a fix. The --force fallback is gone: git blocks exactly the states the guard should have caught, so a block now surfaces with the manual command rather than being forced through.

The exact sequence that destroyed the file now prints:

⚠ Worktree still has uncommitted work:
    ?? notes.txt
  Commit it, stash it, or delete the files, then close the session.
  Untracked files count: they are not in any commit and would be lost.

No --force flag added. The stated problem is data loss, and the refusal names the files and the remedy, so there's no dead end. Deliberate discard is a different, unreported need — a small addition later if it turns out to be annoying.

2. The suite rewrote its own tracked files

Found while verifying Robin's drift work. The install.sh end-to-end test ran the real installer from $REPO_DIR. install.sh delegates to claude-team sync, which resolves its repo directory from the CLI's own path — the real clone — and regenerates agents/ and commands/ there.

The consequence is worse than untidiness: the drift check runs before the install test, so a genuine drift failed the first run and was silently repaired later in the same run. Run it again, green, nothing fixed. That's precisely how a real failure gets written off as flaky.

Installer now runs from a throwaway copy. Verified by appending a marker to a committed agent and confirming it survives a full run.

3. The drift check was one-directional

The old check asked whether each profile line appeared in the generated file — a subset test, blind to extra or wrong content. It also predicted expected output by re-implementing the generator's stripping logic by hand in the test file. That second implementation is exactly what let the same check assert the missing Handoff Brief as correct: the hand-written awk mirrored the generator's bug instead of the requirement.

It now regenerates from the committed profiles with the real generator into a scratch tree and diffs. Catches both directions, and can't mirror a bug the generator doesn't have, because it is the generator. Deleted ~26 lines of mirror logic.

Robin flagged the limit honestly: regenerate-and-diff structurally cannot catch a generator whose transform is wrong but self-consistent. That needs content assertions, which is what the Handoff Brief presence checks are. Both kinds are needed.

4. Other coverage added

cmd_launch had no assertion on which profile it attaches — patching it to resolve sasha for every persona left the suite green. launch --worktree had zero coverage despite combining git side effects with process exec. Also: branch guard refusing to clobber a foreign hook, session start reusing a divergent local branch, and every command failing safely when the profiles directory is missing entirely.

Reported, not fixed

  • stderr leak under lock contention. bin/claude-team:95 — a trailing 2>/dev/null never applies, because bash sets up redirections left-to-right and the < fails first. 8 of 15 concurrent calls leak a raw bash error. Functionally harmless, exit code and data correct.
  • Hardcoded persona rosters drifted in 4 places. claude-team help and the installer Quick Start are missing 4 to 9 of the 17. Same hardcoded-list bug class as the persona copies and the handoff brief.

Robin's risk assessment names that list-vs-derived pattern as the project's recurring weak spot, which matches what three of today's six PRs fixed.

Verification

Every finding reproduced independently before acting: the data-loss bug three ways, the repo mutation with a marker file, the drift blindness by injecting content no profile contains. Runtime 7.8s → 9.0s locally.


Generated by Claude Code

claude added 4 commits July 29, 2026 16:16
Covers PRs #23 and #24. Both close the same kind of gap from opposite
directions: documentation describing a product the code did not match. #23
removed a customization promise that should never be kept. #24 found session
handoff briefing already shipped and fixed the one delivery surface where it
never arrived.

Records the third case in two days of a green suite protecting the wrong
behavior, and two of my own mistakes: a control test that passed against a
scratch tree missing the file it was traversing to, and an installer diagnosis
that blamed a message deleted three commits earlier.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019j5DHEZsoeCGRueTbNLuTb
…d a missing profiles dir

Audit of the whole suite, 175 tests to 201. No production code changed.

The drift check was one-directional. It walked profile lines and asked whether
each appeared in the generated file, which is a subset test: extra or wrong
content in a generated file was invisible to it. Worse, it predicted the
expected output by re-implementing the generator's stripping logic a second
time, by hand, in the test file. That second implementation is what let the same
check assert a missing Handoff Brief as correct, because the hand-written awk
mirrored the generator's bug rather than the requirement.

It now regenerates every agent and slash command from the committed profiles
into a scratch tree using the real generator, then diffs against the committed
copies. That catches both directions and cannot mirror a bug the generator does
not have, because it is the generator rather than a copy of it. It also deleted
about 26 lines of mirror logic. Verified by appending a line to a committed
agent that no profile contains: the old check stayed green, the new one fails.

What regenerate-and-diff structurally cannot catch is a generator whose
transform is wrong but self-consistent, since a fresh run reproduces the same
wrong output. That needs content assertions, which is what the Handoff Brief
presence checks are. Drift checks and content checks cover different failure
modes and the suite needs both.

cmd_launch had no assertion on which profile it attaches. Patching it to resolve
sasha for every persona left the suite green, because the tier assertion reads
tiers.conf through a name lookup independent of the attached file, and the only
other check looked for the literal string append-system-prompt-file rather than
the path after it. The four dry-run tests now pin the resolved path.

launch --worktree had zero coverage despite being the most complex path in the
command, combining git side effects with process exec. Now covered: dry-run has
no side effect, a real invocation through a stub claude binary execs with the
correct directory and profile, a second call reuses the worktree instead of
re-registering it, and use outside a git repo dies cleanly.

Also new: session close refuses a worktree with tracked modifications or staged
additions, branch guard refuses to clobber a pre-existing hook it did not write,
session start reuses a genuinely divergent local branch, and every command fails
safely when the profiles directory is missing entirely, which was untested and
is a real state on a fresh machine.

The JSON validity checks silently disappeared without python3, taking the total
from 175 to 173 with no signal. They now print a visible skip.

Runtime 7.8s to 8.8s locally. The added time is the launch --worktree and
session close blocks, which each build a real repo and worktree, and both cover
risk that previously had none.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019j5DHEZsoeCGRueTbNLuTb
cmd_session_done guarded against uncommitted changes with two git diff calls.
Neither sees an untracked file. Two lines later the worktree removal retried
with --force on any failure, which overrode git's own refusal to remove a
worktree holding untracked files. Net effect: closing a session deleted work
that was never in a commit, printed a green checkmark, exited 0, and recorded
the branch as merged in the index. Nothing was merged.

Isolated three ways, same commands, only the file state differing: a modified
tracked file was refused, a staged new file was refused, and an untracked new
file was deleted. Writing a file and not staging it yet is the most ordinary
state in a working session, so the destructive path was the common one.

The guard is now one git status --porcelain call, which reports staged,
unstaged, and untracked together. It lists the offending paths, because a
refusal the user cannot act on is only half a fix. The --force fallback is gone:
git blocks exactly the states the guard should have caught, so a block now
surfaces as an error with the manual command rather than being forced through.

No --force flag was added to session done. The stated problem is data loss, and
the refusal already names the files and the remedy, so there is no dead end.
Discarding scratch files on purpose is a different, unreported need.

Also fixes the suite rewriting its own tracked files. The install.sh end to end
test ran the real installer from REPO_DIR, and install.sh delegates to
claude-team sync, which resolves its repo directory from the CLI's own path and
regenerates agents/ and commands/ there. A genuine drift therefore failed the
regeneration check earlier in the run and was silently repaired later in the
same run, so a second run came back green with nothing fixed, which is how a
real failure gets dismissed as flaky. The installer now runs from a throwaway
copy. Verified: a marker appended to a committed agent survives a full run.

Four tests added for the untracked case, including one asserting the refusal
names the file. 205 tests, up from 201. shellcheck clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019j5DHEZsoeCGRueTbNLuTb
Two findings from the test audit, both reported there and fixed here.

Four hardcoded persona rosters had drifted. 'claude-team help' and the
slash-command listing after install-commands were each missing cornelius,
ernie, piper, and reiner; the installer's Quick Start was missing nine of
seventeen, and its slash-command line listed eight. Nothing was broken, since
every name still resolved, but a new user reading the tool's own output could
not discover a third to a half of their team.

This is the same bug class as the three-copy persona problem and the handoff
brief that reached three surfaces out of four: a list a human has to remember to
update is a list that goes stale. Patching the four copies would leave the fifth
to drift later, so all of them now derive from the profiles on disk. A new
persona appears in every roster the moment its profile lands.

bin/claude-team gains persona_roster, which emits slug, display name, and short
role for each profile, skipping the coordinators. short_role mirrors the rule
already in scripts/generate-agents.sh: drop a trailing Consultant or Manager
unless that leaves a single word, so "Product Manager" survives intact.
cmd_help and cmd_install_commands both read from it. install.sh derives its
slash-command line the same way and no longer enumerates personas in Quick
Start, since 'claude-team list' is the accurate answer and cannot drift.

The lock leaked a raw bash error to the terminal under contention.
_lock_is_stale read the owner record with a trailing 2>/dev/null, which is never
installed: bash applies redirections left to right and aborts on the first
failure, so the '<' fails before the stderr redirect exists. A missing owner
file is the normal transient state between a holder's mkdir and its record
write, which the surrounding comment already documents, so this fired constantly
whenever two sessions overlapped. Measured at 15 concurrent writers: 9 leaked
errors before, 0 after. Exit codes and data were always correct, so this was
noise rather than damage, but it appeared exactly when parallel sessions
overlap, which is the feature the product leads with.

Four tests added, three of which fail against the previous code, naming the four
missing personas exactly. They assert the derivation rather than the current
text, so they keep working as the roster grows. 209 tests, up from 205.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019j5DHEZsoeCGRueTbNLuTb
@d6veteran
d6veteran marked this pull request as ready for review July 29, 2026 17:20
@d6veteran
d6veteran merged commit 2bf5f02 into main Jul 29, 2026
3 checks passed
@d6veteran
d6veteran deleted the claude/robin-test-audit branch July 30, 2026 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants