Account for the second review, and stop the evidence trail depending on a branch - #12
Merged
Merged
Conversation
…for reviews REVIEW-RESPONSE.md carried all 21 findings from the first review and none of the 22 from the second, while presenting itself as the place every finding is accounted for. That is precisely the failure it was written to prevent: a response document that quietly drops findings reads as coverage while being a selection. The second review's account goes in whole, including the fact that makes it readable: thirteen of its findings were marked "Reconfirmed" because they were reconfirmed against a tree that had never received the fix. Fourteen were already closed and invisible, six were real and are fixed, two are accepted and cannot be closed by editing this repository. Also recorded, because it is worth more than either statement alone: both reviews independently reached the same strategic correction, and POSITIONING.md was written after the first and before the second arrived. The test now requires both series. It required only R-01..R-21, which is how the gap survived being written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DgUkQPrkaHMg5bMRepNGXu
REVIEW-RESPONSE.md cites the commit that closed each finding, and those commits were squash-merged: they are not ancestors of main, and a clone reaches them only through the pre-merge branch ref that is deliberately left in place. That is a real evidence trail resting on a branch nobody has promised not to tidy up. Each citation is now a link to the commit on GitHub, which keeps a pull request's commits permanently -- so every link resolves whatever happens to any branch. The document says which of the two routes is which rather than leaving a reader to discover the fragile one. The test keeps the local check where it can run and reports which route was unavailable where it cannot. Three cases, and the third is the one worth having: the objects are present and every citation is checked; the clone is shallow and says so; or the objects are absent because this clone has no branch ref, which is reported as "I could not look" rather than as "the citation is false". Those are different answers and collapsing them is how a false negative gets a green tick, which is the objection this whole repository is built around. It also now checks that a citation's text and its link name the same commit, by prefix -- an abbreviated SHA linking the full one is normal, a citation pointing somewhere else is not. Verified against a real single-branch clone with none of the objects, and two mutations: a link redirected to another commit, and a citation for a commit that does not exist. Both fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DgUkQPrkaHMg5bMRepNGXu
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.
What this changes, and why
Documentation and one test. No detector code.
Two problems, both found by asking "is anything left?" after #11 merged rather
than by a check failing.
REVIEW-RESPONSE.mdaccounted for one of two reviews. It carried all 21findings from the review of
f3acbf53and none of the 22 from the review ofmainat21068b6, while presenting itself as the place every finding isaccounted for. That is exactly the failure the document exists to prevent: a
response that quietly drops findings reads as coverage while being a selection.
The second review's full table is now in, including the fact that makes it
readable — thirteen of its findings were marked "Reconfirmed" because they were
reconfirmed against a tree that had never received the fix. Fourteen were
already closed and invisible, six were real and are fixed, two are accepted and
cannot be closed by editing this repository.
The commit citations depended on a branch ref surviving. Each closed finding
cites the commit that closed it, and #11 was squash-merged — so those commits
are not ancestors of
main, and a clone reaches them only throughclaude/cohaera-third-security-review-oaa6dd, left frozen at the pre-mergehead. A real evidence trail resting on a branch nobody had promised not to tidy
up. Citations are now links to the commits on GitHub, which keeps a pull
request's commits permanently, so they resolve whatever happens to any branch.
Evidence
grep -c "F-0[1-9]..."on
REVIEW-RESPONSE.mdreturned 0, and a--single-branchclone ofmainresolves none of the cited commits.whose link points at a different commit than its text; a citation for a
commit that does not exist. Both fail. The accounting test was also
confirmed to fail with any
F-nnremoved.python -m pytest tests/ -qpasses. 851 tests.python tests/test_evasion.pyunchanged — 26/26.ruff checkpasses.mypy src/cohaeraclean.python tools/readme_facts.py --checkpasses.If this changes detection
n/a, because nothing under
src/cohaerais touched. The evaluation card isunchanged and was not regenerated.
If this changes an evasion
n/a, because no evasion is affected.
Anything a reviewer should push back on
The citation test can now skip, and a skip is not a pass. Three cases: the
objects are present and every citation is checked; the clone is shallow and says
so; or the objects are absent because this clone has no branch ref, which is
reported as "I could not look" rather than "the citation is false". I think
that is right — those are different answers, and collapsing them is how a false
negative gets a green tick, which is the objection this repository is built
around. But it does mean a clone without the branch never verifies the
citations, and somebody could put a bogus SHA in and only CI would catch it. The
format and text-versus-link checks still run unconditionally, which is the
mitigation. If you would rather it fail hard, say so.
Where this branch came from. #11's branch is now load-bearing — it is the
only ref from which the cited commits are locally reachable — so this work went
onto a fresh branch off
mainrather than force-pushing that one. Samereasoning as the note at the bottom of #11: please don't delete either branch,
though after this change the links survive it if you do.
Generated by Claude Code