feat(dismiss): remember dismissed findings across review runs (#59) - #509
Open
nitishagar wants to merge 2 commits into
Open
feat(dismiss): remember dismissed findings across review runs (#59)#509nitishagar wants to merge 2 commits into
nitishagar wants to merge 2 commits into
Conversation
…a#59) Add a per-repo, filesystem-based dismissal store (~/.opencodereview/dismissals/<encoded-repo>.json) that records SHA-256 fingerprints of findings a user dismissed. A review whose store exists automatically suppresses dismissed findings from its output via a read-side filter applied to the returned comment slice; a review with no store behaves byte-identically to today. The fingerprint is SHA256(path + start_line + end_line + normalizedContent), where normalization is TrimSpace + ToLower. Identity is exact-content by design (documented limitation: rephrased findings resurface); semantic identity is deferred as future work. New ocr dismiss subcommand manages the store: - add <session-id> <index|path:line> record a dismissal - list show recorded dismissals - remove <fingerprint|index> remove a dismissal Loading is fail-safe: a missing file is the no-opt-in case (one stat, no read); a corrupt/unreadable file yields an empty store plus a warning and is left untouched. Saving is atomic via temp+rename at mode 0600. Storage lives in internal/session/dismissal.go (DismissalStore, DismissalFingerprint, DismissalFilter, LoadComments). The filter is wired into Agent.Run on the success path only (err == nil), so a failed review's partial output is never masked. No existing session JSONL, resume index, or collector output is mutated. The new session.LoadComments helper walks a session JSONL and returns the flattened comment list (existing LoadDetail exposes only counts), which dismiss add uses to resolve findings by index or path:line.
|
|
Contributor
|
🔍 OpenCodeReview found 4 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Four fixes from the automated review on alibaba#509: 1. runDismissList: guard against a nil store. LoadDismissals returns (nil, err) when the store path cannot be resolved (HOME unset); the previous code printed a warning then dereferenced store.List(), panicking. Return an error on a nil store instead. 2. loadDismissalFilter: drop the explicit os.Stat before LoadDismissals. ReadFile inside LoadDismissals already performs the existence check, so the stat was redundant and opened a stat->read TOCTOU window in which a deleted file returned an empty filter instead of nil. Rely on LoadDismissals and return nil for an empty/missing store to keep the "nil filter == stateless" invariant (D2). 3. resolveRemoveRef: when a short-fingerprint prefix matches more than one entry, report the ambiguity explicitly rather than falling through to a generic "no match" error (consistent with resolveFindingRef). 4. printDismissTable: stop double-truncating ContentPreview, which is already capped at store time. Adds regression tests for the nil-store panic guard and the ambiguous prefix report.
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
2 participants
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.
Summary
Closes #59.
Adds a per-repo, filesystem-based dismissal store that remembers findings a user dismissed, so identical recurring false positives stop resurfacing on every review run. Aligned with the maintainer's stated "filesystem-based, still in design" direction.
internal/session/dismissal.go:DismissalStore(load/save/record/remove/contains),DismissalFingerprint(SHA256 of path + line range + normalized content),DismissalFilter(read-side filter on the returned comment slice), andLoadComments(walks a session JSONL for the flattened comment list).internal/agent/agent.go: one new optionalArgs.Dismissalsfield + one conditionalSuppresscall on the success path ofAgent.Run. Collectors, session JSONL, and resume index are untouched.ocr dismisssubcommand (cmd/opencodereview/dismiss_cmd.go):add <session-id> <index|path:line>,list,remove <fingerprint|index>.Design (key invariants)
ocr reviewflag is added; if~/.opencodereview/dismissals/<encoded-repo>.jsonexists and parses, the review suppresses matches; otherwise behavior is byte-identical to today (one stat, no read).[]LlmCommentafter collection; the collector's raw output and the session JSONL are never mutated.0600.Schema is
version: 1so future semantic-identity changes are additive, not breaking.How to test
Checklist
make buildsucceedsmake testgreen (all packages,-race)make coverage≥ 80% (81.9% total; newdismissal.gocore functions at 87–100%)go vet ./...andgo fmt ./...cleanAgent.Runsuppression test over a real git repoOut of scope (deliberately)
ocr reviewflag (the file's existence is the opt-in).A validated plan bundle (PLAN / IMPLICIT_SPEC / PLAN_VALIDATION / IMPLEMENTATION_VALIDATION) for this change lives under `thoughts/shared/plans/2026-07-25-issue-59-dismissed-suggestion-memory/`.