feat: enrich Known Issues section with finding context - #179
Open
alansikora wants to merge 2 commits into
Open
alansikora wants to merge 2 commits into
alansikora wants to merge 2 commits into
Conversation
Deploying codecanary with
|
| Latest commit: |
ab18166
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e135ccc0.codecanary.pages.dev |
| Branch Preview URL: | https://feat-known-issues-context.codecanary.pages.dev |
![codecanary-bot[bot]](https://avatars.githubusercontent.com/in/3176464?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐥 CodeCanary — PR #179
Summary
Found 1 issues (1 warning)
💬 See inline comments for details.
🔧 Fix all with AI
Copy the prompt below and paste it into your AI coding tool:
Fix the following code review findings. For each finding, apply the suggested fix or resolve the described issue.
---
## File: internal/review/prompt_test.go, Line: 238
**Issue (warning):** Escape test does not verify `</inject>` is escaped
The forbidden-tags slice in `TestBuildIncrementalPrompt_KnownIssuesEscapesUntrustedBody` checks for `<inject>`, `</inject>`, and `</known-issues>`, but the expected-escaped slice only checks for `<inject>` and `</known-issues>` — it omits `</inject>`. This means the test would pass even if `</inject>` leaked through unescaped, leaving a gap in the regression coverage.
Suggested fix: Add `"</inject>"` to the expected-escaped assertions slice so the round-trip is fully verified.
Status
- New findings: 1
Previously the Known Issues section in BuildIncrementalPrompt emitted only `path:line` for each still-open finding, leaving the LLM without the original concern's text. That made two failure modes likely: 1. Variant-with-different-wording duplicates — the reviewer can't compare its new finding to the original if it doesn't see the original. 2. Stale findings — when the incremental diff invalidates an open finding's premise (e.g. a documentation update or refactor in the diff makes the original concern moot), the reviewer can't surface that conflict because it doesn't know what the open finding said. This PR replaces the `path:line` list with a richer rendering mirroring the Recently Resolved Issues section: title, severity, and description for each open thread, plus instruction text covering the "evidence that an open finding is now wrong" case. The reviewer is told to NOT silently re-emit the original — instead to anchor a NEW finding to the relevant diff line whose description notes the conflict. Untrusted body text is escaped via escapeAllTags, matching the treatment of the resolved-issues section.
The forbidden list checked <inject>, </inject> and </known-issues>, but the escaped-fragment list omitted </inject>. The test would have passed if </inject> leaked through unescaped and was simply dropped rather than escaped, leaving a gap in the regression coverage.
alansikora
force-pushed
the
feat/cross-statement-consistency
branch
from
September 4, 2026 22:39
c94275c to
b067273
Compare
alansikora
force-pushed
the
feat/known-issues-context
branch
from
September 4, 2026 22:39
3ccde41 to
ab18166
Compare
alansikora
added a commit
that referenced
this pull request
Sep 4, 2026
…issues" PR #179 rewrites the same Known Issues section with a strictly richer rendering — full title, severity and description per entry, plus an instruction for spotting when the incremental diff undermines an open finding. Keeping both would mean resolving a conflict in favour of #179 anyway, so drop this commit here and let #179 own the section. This reverts commit 7dd98fc.
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.
Purpose
Stacked on top of #178. Replaces the bare `path:line` list in the incremental prompt's Known Issues section with a richer rendering (title, severity, description) so the reviewer can:
Details
Verify and Review
References