fix: close three data-loss paths found by the three-way audit - #27
Merged
Conversation
…ms that were false Three auditors worked independently and all three found the same meta-pattern: a fix applied to one instance of a bug without checking its siblings. Every finding below was reproduced before the fix and is now covered by a test that fails against the previous code. session done deleted gitignored files. The earlier fix replaced two git diff calls with git status --porcelain, which sees untracked files but not ignored ones, and git worktree remove deletes those. A .env holding the only copy of a credential was destroyed at exit 0. The class survived the fix; only the instance closed. The guard now passes --ignored, and exempts .claude-session, which this command created and is meant to remove. Without that exemption the guard blocked every close, which the suite caught immediately. Damaged marker pairs in ~/.claude/CLAUDE.md destroyed user content. Both block_install and block_remove find the block by scanning for START then END, and with END missing they deleted everything from START to end of file. That file is one the tool tells users to hand-edit, so a lost marker is an ordinary accident, and it took a personal runbook with it at exit 0. Both writers now refuse when the markers are unbalanced or duplicated, rather than guessing which of the user's lines belong inside the block. A literal pipe in a branch name or plan slug silently corrupted the index. It is stored as a pipe-delimited table read back with awk -F'|', git accepts a pipe in a ref name, and "sprint-42 | scoped" is an ordinary slug. The tool printed a success line, then status reported none, done could not close the row, and session done deleted the worktree while the row stayed active forever. Both write sites now reject the character, which is cheaper than making every reader robust to a broken row. session start validates before creating the worktree, so a rejection leaves no orphan. Separately, session done now says so when its rewrite matched no row, since silence there is what turned a corrupt row into a success message. Test infrastructure: the branch section ran against the ambient working directory and asserted the repo's own name, so it failed in a differently-named clone and failed ten ways outside a git repo, both of which happen to contributors using this tool's own worktree sessions. It now builds a fixture like every other git-touching section. A mktemp -d nested inside a string leaked one directory on every run. Nothing asserted the generated description: field, which Claude Code reads to choose a subagent, so collapsing all seventeen to one string passed 209 tests; a content assertion now catches it, because regenerate-and-diff structurally cannot. Documentation: gtm.md is a published document that sold "Ten specialists" and "Bash 3.2+" while the product has seventeen and hard-exits below Bash 4, so a macOS reader following it hit a fatal error on first run. plugin.json hardcoded "Seventeen", the same drift just fixed in the tests; the count is now simply gone, since a number that is not written cannot go stale. The test count in README and ROADMAP said 175 against an actual 209. 223 tests, up from 209. shellcheck clean. Suite now passes from a renamed clone and from outside a git repo, and leaks no temp directories. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019j5DHEZsoeCGRueTbNLuTb
The branch section needed a git fixture, and I gave it one by redefining
run_cmd mid-file and restoring the original afterwards. CI's shellcheck raises
SC2218 on that ("this function is only defined later"), because once a name has
several definitions every earlier call becomes ambiguous about which body it
reaches. The same ambiguity applies to a human reader, so the error is fair.
Replaced with a separate run_branch helper defined once, beside run_cmd, at the
top of the file. Nine calls inside the branch section point at it. One
definition per name now.
Worth recording: local shellcheck 0.11.0 reports zero SC2218 on the exact file
CI rejected, verified by linting that committed blob directly. The check is
present in 0.11.0 and fires on a simple synthetic case, so the two versions
disagree specifically about redefinition. Every "lint clean" claim I made today
rested on the local run, and this is the case that proves the local run is not a
proxy for the gate.
223 tests, unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019j5DHEZsoeCGRueTbNLuTb
d6veteran
marked this pull request as ready for review
July 29, 2026 23:18
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.
Akira, Robin, and Toni audited independently in separate lanes. All three found the same meta-pattern: a fix applied to one instance of a bug, with its siblings never checked.
Grades: B− systems, B− tests, C+ product.
223 tests (was 209). shellcheck clean.
Three data-loss paths, all reproduced before fixing
1.
session donestill deleted data — my fix moved the bug rather than closing itThis morning I replaced two
git diffcalls withgit status --porcelainto catch untracked files. Akira checked the next case:--porcelainomits ignored files;git worktree removedeletes them. A file is gitignored because it's generated or because it's secret, and the second kind exists nowhere else.Guard now passes
--ignored, exempting only.claude-session— the marker this command creates and is meant to remove. Without that exemption the guard blocked every session close, which the suite caught on the first run.2. Damaged markers in
CLAUDE.mddeleted user contentblock_installandblock_removelocate the block by scanning for START then END. With END missing, both deleted everything from START to end of file. That's a file the tool tells users to hand-edit, so a lost marker is an ordinary accident.Verified: a
CLAUDE.mdwith a broken block plus a personal deployment runbook.claude-team use akira→ runbook gone, exit 0.Both writers now refuse on unbalanced or duplicated markers rather than guessing which of the user's lines belong inside the block.
3. A pipe silently corrupted the branch index
The index is a pipe-delimited table read with
awk -F'|'. Git accepts|in a ref name, and"sprint-42 | scoped"is an ordinary plan slug. Result:Worse via
session start:session doneprinted success, deleted the worktree, and left the rowactiveforever.Both write sites now reject the character — cheaper than hardening every reader.
session startvalidates before creating the worktree, so a rejection leaves no orphan. Separately,session donenow reports when its rewrite matched no row, since silence there is what turned a corrupt row into a success message.Test infrastructure
$PWDand asserted the repo's own name. Failed in a differently-named clone; failed ten ways outside a git repo. Both happen to contributors using this tool's own worktree sessions. Now has a fixture like every other git-touching section.description:field — the one Claude Code reads to pick a subagent. Collapsing all 17 to one string passed 209 tests. A content assertion catches it now, because regenerate-and-diff structurally cannot: the generator would agree with itself.mktemp -dnested inside a string leaked a directory on every run, green or not.Documentation
gtm.mdis marked Published and sold "Ten specialists" and "Bash 3.2+" while the product has 17 and hard-exits below Bash 4. A macOS reader following it hit a fatal error on first run.plugin.jsonhardcoded "Seventeen" — the same drift just fixed in the tests. The count is now simply gone. A number that isn't written can't go stale.Verification
main, includinguser content below a damaged block survives (file missing: 'step one').description:assertion proven by collapsing the generator's output and watching it go red.Still open
Akira's lock-breaking race under contention (a stale lock plus concurrent writers can discard a successful write), and
_TMP_FILEcleanup being dead code because command substitution runs it in a subshell. Both are real and reported; neither is a silent data-loss path.Generated by Claude Code