Repository navigation
fix(claude-review): don't let a partial post's landed findings rename the rest - #107
Closed
crowecawcaw wants to merge 1 commit into
Closed
crowecawcaw wants to merge 1 commit into
crowecawcaw wants to merge 1 commit into
Conversation
crowecawcaw
force-pushed
the
fix/claude-review-suffixed-suppress
branch
from
October 7, 2026 19:10
c5f79f6 to
4590d3b
Compare
… the rest Two findings that share a base fingerprint post as base and base-2. If base posted but base-2 failed, a re-run of the post job added the live threads to the suppress list it names findings against. base was now suppressed, so both findings were dropped; the re-run reported nothing unposted and advanced the baseline, losing base-2 for good. Suffixes now skip only the suppress list captured when the review began, which a re-run reads from the same artifact, so each finding gets the same name in every attempt. Threads live on the PR at post time are checked after naming and drop only an exact match. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
crowecawcaw
force-pushed
the
fix/claude-review-suffixed-suppress
branch
from
October 7, 2026 19:33
4590d3b to
e83940f
Compare
crowecawcaw
changed the title
Author
|
Folded into #112, which carries this commit unchanged as its first commit. |
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
1 participant
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 was the problem/requirement? (What/Why)
Review feedback on #100 (comment).
When two findings in one run share a base fingerprint, they post as
baseandbase-2. Ifbaseposted butbase-2failed, a re-run of thepostjob added the PR's live threads to the suppress list it names findings against.basewas now suppressed, soselect_findingsdropped both. The re-run reported nothing unposted and advanced the baseline, andbase-2was lost for good.What was the solution? (How)
Split the two roles the suppress list was playing:
prior-review-state.json). A re-run reads that from the same artifact, so every attempt names each finding the same way.Outside the re-run case, behavior is unchanged.
What is the impact of this change?
A partial post followed by a re-run no longer loses a same-symbol finding.
How was this change tested?
Red-green with unit tests:
test_rerun_after_partial_post_keeps_a_suffixed_finding:post()against the fake GitHub. Two findings share a symbol, only the second fails to post, then the job is re-run. It failed on mainline (base-2never posted) and passes now.test_suffixed_finding_survives_when_only_the_first_already_landed:select_findingswithbaselive on the PR still yieldsbase-2.test_suffix_never_collides_with_a_literal_symbol: a literal symbolfoo-2is not named the same as a suffixedfoo. This held before too; the test pins it.test_same_fp_in_one_run_is_disambiguated, pass unchanged.All 61 tests pass (
python -m unittest discover -s test).Was this change documented?
Yes, in the
select_findingsdocstring.Is this a breaking change?
No. Marker format is unchanged, so existing threads still match.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.