[Benkapner]

Summary

Stamp <!-- finding:f_abc --> on inline review comments when findings[].id is present and matches f_ plus alphanumeric characters. Missing or invalid ids leave the comment unchanged.
This command copies the id from the review JSON. It does not assign ids. Until the agents repo emits findings[].id, posted reviews look the same as today.

Related Issue

Part of AISDLC-122. Related: #7907 already looks for this marker when fetching human-resolved threads.

Changes

• Add ID to ReviewFinding (json:"id,omitempty")
• Prepend <!-- finding:<id> --> in formatFindingComment for valid ids
• Same stamp on file-level comments (they use the same formatter)
• Tests for valid, missing, and invalid ids

Testing

• make lint passes (stage changes first, then run)
• Tests added/updated for new or modified logic

Checklist

• PR title follows Conventional Commits (correct type, ! for breaking changes)
• Commits are signed off (DCO) — human and human-directed agent sessions only

[github-actions]

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer with write access must add the ok-to-test label after the latest push.

See E2E testing guide for details.

[qodo-code-review]

PR Summary by Qodo

Stamp validated finding IDs on review comments

✨ Enhancement 🧪 Tests 🕐 10-20 Minutes

AI Description

• Copy valid upstream finding IDs into HTML markers on inline and file-level review comments.
• Leave comments unchanged when an ID is missing or invalid; test parsing and both comment paths.

Diagram

graph TD
  J["Review JSON"] --> F["Review finding"] --> V{"Valid ID?"} -->|Yes| S["Add marker"] --> C["Review comment"] --> G["Review API"]
  V -->|No| U["Unchanged body"] --> C

Loading

High-Level Assessment

Using the existing shared formatter is appropriate: it covers inline and file-level comments without duplicating stamping logic. Keeping ID assignment upstream and omitting invalid IDs preserves current behavior until the agent emits them.

Files changed (2) +64 / -2

Enhancement (1) +15 / -1

postreview.goCarry and validate finding IDs when formatting review comments +15/-1

Carry and validate finding IDs when formatting review comments

• Adds the optional JSON finding ID and stamps valid IDs as HTML markers in the shared comment formatter. Missing or invalid IDs do not change the formatted body.

internal/cli/postreview.go

Tests (1) +49 / -1

postreview_test.goTest finding ID parsing and comment stamping +49/-1

Test finding ID parsing and comment stamping

• Covers JSON ID parsing, valid and invalid formatter inputs, and marker behavior for inline and file-level comments.

internal/cli/postreview_test.go

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Review stamp tests fail for sample IDs 📘 Rule violation ▣ Testability

Description

TestFindingsToReviewComments_StampsFindingID uses f_inline and f_file as valid IDs, but
findingIDRe rejects underscores after f_. Both comments remain unstamped, so the new inline and
file-level assertions fail when the Go package tests run.

Code

internal/cli/postreview_test.go[R1270-1271]

+		{ID: "f_inline", File: "a.go", Line: 10, Severity: "high", Category: "bug", Description: "In hunk"},
+		{ID: "f_file", File: "a.go", Line: 50, Severity: "low", Category: "style", Description: "Outside hunk"},

Relevance

●●● Strong

Test fixtures violate the documented ID grammar, causing deterministic assertion failures; similar
test-coverage fixes were accepted.

PR-#1010
PR-#2415
PR-#7693

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The checklist calls for tests that verify new Go logic. The new test expects stamps for two IDs
rejected by the new regular expression, so its assertions cannot pass.

Rule 1062049: Require tests for new or modified Go logic
internal/cli/postreview.go[38-41]
internal/cli/postreview_test.go[1268-1284]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new stamp test expects IDs containing underscores to be stamped, but the formatter accepts only alphanumeric characters after `f_`.

## Fix Focus Areas
- internal/cli/postreview_test.go[1270-1284]

## Recommended Fix
Replace the two sample IDs with valid alphanumeric IDs and update their expected markers. Keep the separate invalid-ID case, then run the affected Go package tests.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Dismiss ↗ | View ↗

---

Context sources

✅ Compliance rules (platform): 72 rules

✅ Skills: writing-user-docs, writing-adrs, check-patch-coverage

✅ Cross-repo context — repo relationships

  Explored: repo: fullsend-ai/autonomy-analysis (sha: 46717359) — View relationship

  Explored: repo: fullsend-ai/agents (sha: 3af27dde) — View relationship

Review mode: ⚖️ Balanced: This is a localized behavioral change to review-comment formatting and identifier validation, with contract-facing GitHub comment effects that warrant a complete single-pass review.

Tip of the day

💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[rh-hemartin]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:06 AM UTC · Completed 7:23 AM UTC

Commit: c9c5843 · View workflow run →

Runtime: pi · Model: openai/gpt-6.1-sol → gpt-6.1-sol · Effort: high · Cost: $2.11

[fullsend-ai-review]

Risk Assessment: moderate (2/5)

Details

Small additive change with balanced test coverage and no protected paths, but elevated to moderate risk due to high recent churn, multiple active authors, and frequent regression fixes in internal/cli/postreview.go.

[fullsend-ai-review]

Review

Findings

Low

• [logic-error] internal/cli/postreview.go:211 — A non-string findings[].id invalidates an otherwise usable JSON review. For example, "id":123 causes unmarshalling to fail, after which parseReviewResult silently treats the whole input as plain text with action comment, dropping findings and the JSON head_sha. This affects malformed direct CLI input; it is not evidence of a failure in schema-validated agent output.
  Remediation: Ignore non-string optional IDs without discarding the surrounding review. Add tests preserving body, action, head SHA, and findings for numeric, boolean, object, and array IDs.

• [secret-exposure] internal/cli/postreview.go:550 — The newly published ID is not scanned by sanitizeReviewResult. An ID containing f_ followed by AIza and 35 alphanumeric characters passes the marker regex while matching the existing Google API-key redactor. It can therefore be posted verbatim through direct CLI input, including in raw Markdown despite being visually hidden. Runner-level output scanning mitigates the normal secured agent path; this is a defense-in-depth gap at the standalone command boundary.
  Remediation: Scan the ID before stamping it and omit the stamp if scanning reports a secret or changes the ID. Add a synthetic credential regression test and an ordinary-ID preservation test.

---

Labels: The PR changes Go implementation and tests for review-comment posting.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

[fullsend-ai-review]

See the review comment for full details.

[Benkapner]

/fs-fix

[waynesun09]

LGTM. Both earlier bot findings are fixed in 12e8517: a non-string id is now ignored instead of breaking the review parse, and the id goes through the secret scanner before it is stamped. Nothing changes until the review agent emits findings[].id, and nothing parses inline comment bodies by their first line, so prepending the marker is safe. Tests pass locally.

The bot's CHANGES_REQUESTED is from c9c5843, before the fix; it needs a re-run or a dismissal.

Optional: every other marker uses the <!-- fullsend:… --> prefix. Since #7907 and the agents side will build on this one, <!-- fullsend:finding:f_… --> might be worth adopting now, while nothing emits ids yet.

[Benkapner]

im leaving the marker as to stay aligned with #7907. playback looks like Botics infra (not the stamp), so could you re-queue and dismiss/re-run the bot CHANGES_REQUESTED on the c9c5843 commit? @waynesun09 @rh-hemartin