feat: extend non-finding filter to title + suggestion fields#24
Merged
Conversation
Six confirmation findings escaped on PRs #10/#12 because the model routed the "no real finding" signal into whichever field the filter didn't inspect — the title ("N/A — …", "…is correct"), the suggestion ("No action needed — …"), or a rambling scenario's conclusion ("…No bug here."). The filter tested only failure_scenario prefixes. Filter (deterministic layer): - title, failure_scenario, and suggestion all tested against the existing prefix set plus a new separator-anchored confirmation set (none / no failure / no concrete failure scenario / no bug / no action needed / no change needed). The required separator ([—–:.-] or end-of-text) keeps real scenarios like "No failure occurs until…" and "None of the guards…" safe — and lets "none" return, resolving the PR #9 prompt/filter drift in the filtering direction. - Declarative confirmation titles ("…is correct" / "…is accurate") and leaked scenario conclusions ("…no bug here", "…analysis was wrong") drop via end-anchored suffixes. Prompt (advisory layer): OUTPUT_DISCIPLINE bans declarative confirmation titles + verification-task titles, and gains a suggestion-format line — a suggestion of "no bug"/"no action needed"/"N/A" means no finding. README: new Non-finding filter section documenting the drop contract and the anchoring rationale; What-it-does + How-it-works + Shipped surfaces updated in lockstep. Accepted tradeoffs (user-approved): genuine "Placeholder text …" titles and imperative "Ensure the timeout is correct" titles would drop — zero observed instances in ~70 bot findings vs 6 observed escapes. 17 new tests on the real escaped strings (398 total); 3 mutation checks verified (title-suffix rule, separator anchoring, title field removal each fail exactly their covering tests).
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @aliasunder, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
![umm-actually[bot]](https://avatars.githubusercontent.com/in/4233716?s=60&v=4){kind=small}
|
umm-actually re-reviewed at No new findings (2 tracked finding(s) across all runs). umm-actually · deepseek/deepseek-v4-pro |
The CONFIRMATION_SCENARIO_SUFFIX regex uses `\bno bug(?: here)?` which matches both "...no bug" and "...no bug here", but the README only listed the latter. Add both variants so the filter contract matches the code. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…le-field CONFIRMATION_PREFIX Two coverage gaps found during test audit: - The 'analysis was wrong' alternative in CONFIRMATION_SCENARIO_SUFFIX had no end-of-string test (the existing test matched 'No bug here.' instead) - CONFIRMATION_PREFIX on the title field was untested (only NON_FINDING_PREFIX was exercised on titles) Both mutation-verified: removing each regex alternative fails exactly the new test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The OUTPUT_DISCIPLINE and ANCHORING_CONTRACT assertions used multiple toContain calls with short phrases — violating the convention that deterministic output gets exact assertions. Replaced with single toContain of each full constant text. AGENTS.md test conventions now explicitly address the large-string case: assert the full section in one toContain, not multiple fragments. Co-Authored-By: Claude Opus 4.6 (1M context) <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
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.
Why
Six confirmation findings escaped the non-finding filter on PRs #10 and #12. The filter tested only
failure_scenarioprefixes, so the model routed the "no real finding" signal into whichever field went uninspected:N/A — maintains unposted design (ignore)(#12)N/A — designed behavior…); the scenario read as realREADME doc-mention description is accurate(#10)…is accurateExtensionless path handling in posix.basename is correct(#10)…is correct; scenarioNo failure — …; suggestionNo bug — …Use posix.extname…(#10)No concrete failure scenario — …; suggestionNo change needed — …Verify related file token budget…(#10)No action needed — …Treat dot followed by non-word…(#10)…my initial analysis was wrong. … No bug here.The prompt's HARD PROHIBITION already bans these; the model ignored it six times. The deterministic filter is the enforcement layer, so it now inspects all three prose fields.
What changed
Filter (deterministic) —
src/review/filter-non-findings.tstitle,failure_scenario, andsuggestionare all tested against the existing prefix set plus a new separator-anchored confirmation set (none,no failure,no concrete failure scenario,no bug,no action needed,no change needed). The required separator ([—–:.-]or end-of-text) is the false-positive guard:No failure occurs until the third retry…andNone of the guards catch this input(PR feat: filter non-findings and constrain comment verbosity #9's original FP) never match. It also letsnonereturn safely, resolving the PR feat: filter non-findings and constrain comment verbosity #9 prompt/filter drift in the filtering direction.…is correct/…is accurate) and leaked scenario conclusions (…no bug here,…analysis was wrong).FilterResultshape, pre-cap ordering, and log-only observability are unchanged — no ripple into orchestrate/select/comment-mapping.Prompt (advisory) —
src/review/prompt.tsOUTPUT_DISCIPLINE bans declarative confirmation titles and verification-task titles, and gains a suggestion-format line (a suggestion ofno bug/no action needed/N/Ameans there is no finding). Format enforcement was deliberately kept prompt-side: a deterministic imperative-title allowlist would have dropped 8 observed genuine declarative titles to avoid 2 hypothetical false positives.README — new Non-finding filter section documenting the full drop contract and the anchoring rationale; What-it-does, How-it-works, and Shipped surfaces updated in lockstep.
Accepted tradeoffs (approved)
Placeholder text overflows…title would drop (same tradeoff class PR feat: filter non-findings and constrain comment verbosity #9 accepted for failure_scenario).Ensure the timeout is correcttitle would drop — zero observed instances in ~70 bot findings, vs 6 observed escapes the rule catches.Verification
\b, and removing the title field each fail exactly their covering tests.npm test/npm run lint/npm run build/npm run prettier:check/docker build .all green.