-
Notifications
You must be signed in to change notification settings - Fork 0
feat: DISMISS F# protocol (v1.2.0) #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| import { describe, it, expect } from "vitest"; | ||
| import { findingKey, numberFindings, parseDismissals, filterSuppressed, DismissTracker } from "./dismiss"; | ||
|
|
||
| describe("findingKey", () => { | ||
| it("extracts severity + location from finding line", () => { | ||
| const line = '- **Medium:** src/gateway/model.ts:97 — chatId extraction uses wrong index'; | ||
| const key = findingKey(line); | ||
| expect(key).toContain("medium:src/gateway/model.ts:97"); | ||
| }); | ||
|
|
||
| it("handles numbered finding format (F# prefix)", () => { | ||
| const line = '- **F1 Medium:** src/foo.ts:10 — something bad'; | ||
| const key = findingKey(line); | ||
| expect(key).toBe("medium:src/foo.ts:10 — something bad"); | ||
| }); | ||
|
|
||
| it("falls back to trimmed line for non-matching format", () => { | ||
| const key = findingKey("some random text"); | ||
| expect(key).toBe("some random text"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("numberFindings", () => { | ||
| it("numbers finding bullets sequentially", () => { | ||
| const text = '- **High:** foo.ts:1 — bug\n- **Low:** bar.ts:2 — nit\nSome other text'; | ||
| const { numbered, findings } = numberFindings(text); | ||
| expect(numbered).toContain("**F1 High:**"); | ||
| expect(numbered).toContain("**F2 Low:**"); | ||
| expect(numbered).toContain("Some other text"); | ||
| expect(findings).toHaveLength(2); | ||
| }); | ||
|
|
||
| it("preserves non-finding lines unchanged", () => { | ||
| const text = 'Header\n\n- **Medium:** x.ts:5 — issue\n\nFooter'; | ||
| const { numbered } = numberFindings(text); | ||
| expect(numbered).toContain("Header"); | ||
| expect(numbered).toContain("Footer"); | ||
| expect(numbered).toContain("**F1 Medium:**"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("parseDismissals", () => { | ||
| it("parses DISMISS F# with colon separator", () => { | ||
| const text = "The chatId extraction is intentional.\nDISMISS F1: intentional design for telegram thread format"; | ||
| const dismissals = parseDismissals(text); | ||
| expect(dismissals.size).toBe(1); | ||
| expect(dismissals.get(1)).toBe("intentional design for telegram thread format"); | ||
| }); | ||
|
|
||
| it("parses multiple dismissals", () => { | ||
| const text = "DISMISS F1: by design\nDISMISS F2: not a real issue"; | ||
| const dismissals = parseDismissals(text); | ||
| expect(dismissals.size).toBe(2); | ||
| expect(dismissals.get(1)).toBe("by design"); | ||
| expect(dismissals.get(2)).toBe("not a real issue"); | ||
| }); | ||
|
|
||
| it("parses dash separator", () => { | ||
| const text = "DISMISS F3 - already reviewed"; | ||
| const dismissals = parseDismissals(text); | ||
| expect(dismissals.get(3)).toBe("already reviewed"); | ||
| }); | ||
|
|
||
| it("returns empty map when no dismissals", () => { | ||
| const text = "I fixed both issues as suggested."; | ||
| expect(parseDismissals(text).size).toBe(0); | ||
| }); | ||
| }); | ||
|
|
||
| describe("filterSuppressed", () => { | ||
| it("removes suppressed findings", () => { | ||
| const text = '- **High:** foo.ts:1 — bug one\n- **Low:** bar.ts:2 — nit two'; | ||
| const suppressed = new Set([findingKey('- **High:** foo.ts:1 — bug one')]); | ||
| const result = filterSuppressed(text, suppressed); | ||
| expect(result).not.toContain("bug one"); | ||
| expect(result).toContain("nit two"); | ||
| }); | ||
|
|
||
| it("returns null when all findings suppressed", () => { | ||
| const text = '- **High:** foo.ts:1 — bug one'; | ||
| const suppressed = new Set([findingKey('- **High:** foo.ts:1 — bug one')]); | ||
| expect(filterSuppressed(text, suppressed)).toBeNull(); | ||
| }); | ||
|
|
||
| it("returns original when no suppressions", () => { | ||
| const text = '- **Low:** x.ts:5 — something'; | ||
| expect(filterSuppressed(text, new Set())).toBe(text); | ||
| }); | ||
| }); | ||
|
|
||
| describe("DismissTracker", () => { | ||
| it("tracks dismissals and suppresses after threshold", () => { | ||
| const tracker = new DismissTracker(); | ||
| const findings = ['- **Medium:** src/foo.ts:10 — bad pattern', '- **Low:** src/bar.ts:5 — nit']; | ||
| tracker.setLastFindings(findings); | ||
|
|
||
| // First dismiss | ||
| tracker.processDismissals("DISMISS F1: intentional"); | ||
| expect(tracker.getSuppressed().size).toBe(0); // threshold is 2 | ||
|
|
||
| // Second dismiss (same finding via new review) | ||
| tracker.setLastFindings(findings); | ||
| tracker.processDismissals("DISMISS F1: still intentional"); | ||
| expect(tracker.getSuppressed().size).toBe(1); | ||
| }); | ||
|
|
||
| it("reset clears all state", () => { | ||
| const tracker = new DismissTracker(); | ||
| tracker.setLastFindings(['- **High:** x.ts:1 — bug']); | ||
| tracker.processDismissals("DISMISS F1: nope"); | ||
| tracker.processDismissals("DISMISS F1: nope again"); | ||
| expect(tracker.getSuppressed().size).toBe(1); | ||
|
|
||
| tracker.reset(); | ||
| expect(tracker.getSuppressed().size).toBe(0); | ||
| }); | ||
| }); |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| /** | ||
| * dismiss.ts — Track and suppress dismissed review findings | ||
| * | ||
| * When the agent responds to a review with "DISMISS F#: reason", | ||
| * that finding is tracked. If the same finding appears in a subsequent | ||
| * review cycle and has been dismissed >= threshold times, it's suppressed. | ||
| * | ||
| * Finding identity: severity + file:line + first 60 chars of problem text. | ||
| */ | ||
|
|
||
| import { log } from "./logger"; | ||
|
|
||
| export interface DismissedFinding { | ||
| key: string; | ||
| reason: string; | ||
| count: number; | ||
| } | ||
|
|
||
| /** How many times a finding must be dismissed before it's auto-suppressed. */ | ||
| const SUPPRESS_THRESHOLD = 2; | ||
|
|
||
| /** Extract a stable key from a finding bullet line. */ | ||
| export function findingKey(line: string): string { | ||
| // Format: - **Severity:** file:line — problem text | ||
| const match = line.match(/^\s*-\s*\*\*(?:F\d+\s+)?(\w+):\*\*\s*(.+)/); | ||
| if (!match) return line.trim().slice(0, 80); | ||
| const severity = match[1].toLowerCase(); | ||
| const rest = match[2].trim().slice(0, 60); | ||
| return `${severity}:${rest}`; | ||
| } | ||
|
|
||
| /** Number findings in review text and return the numbered version + finding list. */ | ||
| export function numberFindings(text: string): { numbered: string; findings: string[] } { | ||
| const lines = text.split("\n"); | ||
| const findings: string[] = []; | ||
| let counter = 0; | ||
|
|
||
| const numbered = lines.map(line => { | ||
| // Match finding bullets: - **Severity:** ... | ||
| const match = line.match(/^(\s*-\s*)\*\*(\w+):\*\*(.*)$/); | ||
| if (match) { | ||
| counter++; | ||
| findings.push(line); | ||
| return `${match[1]}**F${counter} ${match[2]}:**${match[3]}`; | ||
| } | ||
| return line; | ||
| }).join("\n"); | ||
|
|
||
| return { numbered, findings }; | ||
| } | ||
|
|
||
| /** Parse DISMISS markers from agent text. Returns map of F# → reason. */ | ||
| export function parseDismissals(text: string): Map<number, string> { | ||
| const dismissals = new Map<number, string>(); | ||
| // Match: DISMISS F1: reason or DISMISS F1 - reason or DISMISS F1 reason | ||
| const pattern = /DISMISS\s+F(\d+)\s*[:–\-]\s*(.+)/gi; | ||
|
Check failure on line 56 in dismiss.ts
|
||
| let match; | ||
| while ((match = pattern.exec(text)) !== null) { | ||
| dismissals.set(parseInt(match[1], 10), match[2].trim()); | ||
| } | ||
| return dismissals; | ||
| } | ||
|
|
||
| /** Filter suppressed findings from review text. Returns filtered text or null if all suppressed. */ | ||
| export function filterSuppressed(text: string, suppressed: Set<string>): string | null { | ||
| if (suppressed.size === 0) return text; | ||
|
|
||
| const lines = text.split("\n"); | ||
| const filtered = lines.filter(line => { | ||
| const match = line.match(/^\s*-\s*\*\*\w+:\*\*/); | ||
| if (!match) return true; // keep non-finding lines | ||
| const key = findingKey(line); | ||
| return !suppressed.has(key); | ||
| }); | ||
|
|
||
| // If all findings were suppressed, return null (should be LGTM) | ||
| const remaining = filtered.filter(l => l.match(/^\s*-\s*\*\*/)); | ||
| if (remaining.length === 0) return null; | ||
|
|
||
| return filtered.join("\n"); | ||
| } | ||
|
|
||
| /** | ||
| * Dismiss tracker — stores dismissed findings across review loops. | ||
| * Scoped to one orchestrator instance (one session). | ||
| */ | ||
| export class DismissTracker { | ||
| private dismissed = new Map<string, DismissedFinding>(); | ||
| private lastFindings: string[] = []; | ||
|
|
||
| /** Record the findings from the latest review (for F# → finding mapping). */ | ||
| setLastFindings(findings: string[]): void { | ||
| this.lastFindings = findings; | ||
| } | ||
|
|
||
| /** Process agent's response text for DISMISS markers. */ | ||
| processDismissals(agentText: string): number { | ||
| const markers = parseDismissals(agentText); | ||
| if (markers.size === 0) return 0; | ||
|
|
||
| let count = 0; | ||
| for (const [fNum, reason] of markers) { | ||
| const finding = this.lastFindings[fNum - 1]; // F1 = index 0 | ||
| if (!finding) continue; | ||
|
|
||
| const key = findingKey(finding); | ||
| const existing = this.dismissed.get(key); | ||
| if (existing) { | ||
| existing.count++; | ||
| existing.reason = reason; | ||
| } else { | ||
| this.dismissed.set(key, { key, reason, count: 1 }); | ||
| } | ||
| count++; | ||
| log(`dismiss: F${fNum} dismissed (${key}) — "${reason}" [count=${this.dismissed.get(key)!.count}]`); | ||
| } | ||
| return count; | ||
| } | ||
|
|
||
| /** Get the set of finding keys that should be suppressed (dismissed >= threshold). */ | ||
| getSuppressed(): Set<string> { | ||
| const suppressed = new Set<string>(); | ||
| for (const [key, entry] of this.dismissed) { | ||
| if (entry.count >= SUPPRESS_THRESHOLD) { | ||
| suppressed.add(key); | ||
| } | ||
| } | ||
| return suppressed; | ||
| } | ||
|
|
||
| /** Reset all dismissals (e.g. on session end). */ | ||
| reset(): void { | ||
| this.dismissed.clear(); | ||
| this.lastFindings = []; | ||
| } | ||
| } | ||
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
applyDismissFilteringflips a senior result fromISSUES_FOUNDto LGTM, this path sends an LGTM message butoutcome.architectis still absent because architect execution already happened (or was skipped) insidehandleAgentEndbased on the unfiltered result. For multi-file git-based changes, that bypasses the intended architect cross-file check and can tell the agent it's safe to push without ever running it. This occurs whenever all remaining findings are suppressed viaDISMISS F#.Useful? React with 👍 / 👎.