fix(guardrail): close the remaining loose ends - #20
Merged
Merged
Conversation
Four things, all previously reported and consciously deferred, now closed. WAIVER LIFECYCLE. An approval the owner actually gave was silently discarded if the confirmation dialog had been open longer than the 600s TTL: the note was consumed and the waiver never written, so CI then failed on a finding they had explicitly allowed. The TTL exists to stop a DENIED note being reused by a later edit — but the content hash already proves the note belongs to this exact edit, and that guard does not expire. Aged notes are now honoured when they carry a content hash, and dropped when they do not. CONCURRENCY. _append_grace was an unlocked read-modify-write of a file the checker also reads, using two SHARED temp filenames, so two edits in flight could interleave or collide. It now takes an exclusive lock and stages through per-process temp files. Locking is best-effort and never blocks an edit. install-hooks.sh HARDCODED .git/hooks, which is wrong in a git worktree where .git is a FILE — the redirect failed with "Not a directory" and no hook was installed, silently, in exactly the checkouts this team works in. It now uses `git rev-parse --git-path hooks`, which also honours core.hooksPath. SYNC CLASSIFIED BY FOLDER NAME. The public/private decision keyed on the destination DIRECTORY name, so a worktree of command_center called anything else classified as public and had its payload replaced with the redacted one. Fail-closed meant nothing leaked, but it is still the wrong content in the wrong repo — and this run reproduced it. Classification now reads the destination git remote, so what a checkout is called no longer matters; a destination whose remote cannot be read is refused rather than guessed. Verified: an approval survives a 30-minute-old dialog; a denial still never becomes a waiver; six concurrent recorders leave the profile parseable with no stray lock or temp files; the installer works in a worktree and is idempotent; a command_center checkout named "totally-unrelated-name" classifies as private; and every repo holds the payload variant it should. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Four things, all previously reported and consciously deferred, now closed.
WAIVER LIFECYCLE. An approval the owner actually gave was silently discarded if
the confirmation dialog had been open longer than the 600s TTL: the note was
consumed and the waiver never written, so CI then failed on a finding they had
explicitly allowed. The TTL exists to stop a DENIED note being reused by a
later edit — but the content hash already proves the note belongs to this exact
edit, and that guard does not expire. Aged notes are now honoured when they
carry a content hash, and dropped when they do not.
CONCURRENCY. _append_grace was an unlocked read-modify-write of a file the
checker also reads, using two SHARED temp filenames, so two edits in flight
could interleave or collide. It now takes an exclusive lock and stages through
per-process temp files. Locking is best-effort and never blocks an edit.
install-hooks.sh HARDCODED .git/hooks, which is wrong in a git worktree where
.git is a FILE — the redirect failed with "Not a directory" and no hook was
installed, silently, in exactly the checkouts this team works in. It now uses
git rev-parse --git-path hooks, which also honours core.hooksPath.SYNC CLASSIFIED BY FOLDER NAME. The public/private decision keyed on the
destination DIRECTORY name, so a worktree of command_center called anything
else classified as public and had its payload replaced with the redacted one.
Fail-closed meant nothing leaked, but it is still the wrong content in the
wrong repo — and this run reproduced it. Classification now reads the
destination git remote, so what a checkout is called no longer matters; a
destination whose remote cannot be read is refused rather than guessed.
Verified: an approval survives a 30-minute-old dialog; a denial still never
becomes a waiver; six concurrent recorders leave the profile parseable with no
stray lock or temp files; the installer works in a worktree and is idempotent;
a command_center checkout named "totally-unrelated-name" classifies as private;
and every repo holds the payload variant it should.
Closes the last items left open after the Product Guardrail merged. Each was reported by review and consciously deferred as low severity; none was a security hole, and each is now closed with a control proving it.
🤖 Generated with Claude Code