Skip to content

fix(claude-review): harden posting and bump claude-code-action for Opus 5.5 - #100

Merged
crowecawcaw merged 3 commits into
aws-deadline:mainlinefrom
crowecawcaw:fix/claude-review-feedback
Oct 7, 2026
Merged

crowecawcaw merged 3 commits into
aws-deadline:mainlinefrom
crowecawcaw:fix/claude-review-feedback

Conversation

@crowecawcaw

@crowecawcaw crowecawcaw commented Oct 6, 2026 •

Copy link
Copy Markdown

What was the problem/requirement? (What/Why)

Review feedback on #99, plus two more issues found while fixing it:

  1. Forged threads. parse_threads trusted any thread whose first comment had an fp marker, whoever wrote it. Anyone can open a thread, and fingerprints are easy to guess. A forged one could hide a real finding, change the open counts, or get a human's thread replied to and resolved.
  2. Partial posts counted as success. When GitHub rejected some comments, those findings were lost. The baseline still advanced and the status could go green.
  3. Deleted files. Findings could only anchor to new-side lines, so a finding on a deleted file was dropped.
  4. Re-runs. The artifact name included run_attempt. Re-running only a failed post job looked for an artifact that was never uploaded. Also, post dedupes against fingerprints captured before anything was posted, so a re-run after a partial post would post duplicates.
  5. Release notes. With tool_choice: auto, the model can answer in plain text. The script made one request and then failed; the "loop below enforces it" comment was wrong.
  6. (Found in my own review) Marker injection. Markers are matched by search, so a marker the model wrote into a body (for example, because the PR told it to) took precedence over the one we append. That could relabel a finding's severity or fingerprint.
  7. (Reported on openjd-rs) Opus 5.5 doesn't work. The pinned claude-code-action v1.0.136 bundles Claude Code CLI 2.1.163, and Bedrock rejects it for us.anthropic.claude-opus-5-5 (the default since feat(claude-review): converge reviews in fewer rounds #99): API Error: 400 Claude Code 2.1.163 does not support this model; version 2.1.280 or newer is required. Every caller on the default model is failing.
  8. (Found in review) v1.0.244 also adds a write-access check for workflow_run events. It checks both github.actor and workflow_run.actor, and on a fork PR that's the fork author, who never has write access. Without an allowlist, every review of an external contributor's PR would fail before Claude ran. v1.0.136 didn't check workflow_run at all.

What was the solution? (How)

  1. Ignore threads whose first comment isn't from the Actions bot. Summary and "addressed" markers already work this way.
  2. post_review returns the findings it couldn't post. If any are left over:
    • the baseline stays put,
    • the status is error,
    • the summary comment lists them.
  3. A finding can set side: LEFT and anchor to a removed line by its old line number. diff_lines now parses both sides and keys deleted files by their old path. In incremental mode, a LEFT should-fix only needs this revision to have removed code from that file: old-side line numbers in the PR diff and the interdiff count from different commits.
  4. The artifact is named claude-review-<run_id> with overwrite: true. post adds the PR's current threads to the suppress list before posting.
  5. Retry up to 3 times. Each retry keeps the model's previous reply in the conversation and adds a reminder to call emit_release_notes. A malformed tool call gets an error tool_result before the retry. A refusal isn't retried. entries: [] is a valid result.
  6. Neutralize <!-- in model text (finding bodies, addressed reasons, and the unposted list in the summary).
  7. Bump claude-code-action to v1.0.244. It bundles CLI 2.1.292 (claude-agent-sdk 0.3.292 in its bun.lock). The inputs we pass (use_bedrock, github_token, prompt, claude_args) are unchanged between the two versions.
  8. Set allowed_non_write_users: "*" on the action. It's the only workable value, since fork authors can't be listed in advance, and it restores v1.0.136's behavior. It's safe here because the review job's token is read-only, Claude only gets read tools, and posting happens in the separate post job. The bypass only applies when github_token is passed, and we pass it.

What is the impact of this change?

  • Comments that GitHub fails to post no longer disappear without trace.
  • Deleted files can now get findings.
  • Re-running only the post job works.
  • Callers need no changes.
  • External contributors' PRs keep getting reviewed after the action bump.

How was this change tested?

  • New unit tests:
    • forged thread marker;
    • marker injection in a body;
    • both-side diff parsing, deleted files, and removed lines that look like ---/+++ headers;
    • LEFT anchors in full and incremental modes;
    • post() against a fake GitHub: all posted, partial, zero posted, and a re-run after a partial post that doesn't duplicate;
    • release notes: text-then-tool, malformed tool call, empty entries, the attempt limit, and refusal.
  • All 58 tests pass (python -m unittest discover -s test).
  • actionlint and zizmor 1.23.1 report the same findings before and after the change; nothing new.
  • End-to-end on the fork harness (TEST: review-feedback fixes (deletion + bugs) crowecawcaw/deadline-cloud#27; Opus 4.8, because the fork's test role doesn't allow 5.5):
    • Deleted file: the first review posted a blocking LEFT-side comment on the deleted _sigint_handler.py ("still imported by 4 modules"), plus 3 RIGHT-side findings.
    • Re-running only post: GitHub failed the first summary write (unexpected end of JSON input). Re-running just the failed job downloaded attempt 1's artifact, skipped the 4 already-posted findings as "already raised", and completed. No duplicates.
    • Forged thread: I opened a thread as a non-bot user carrying <!-- claude-review fp=…::forged sev=blocking -->. The next review ignored it: it counted 4 threads, not 5, and 2 open blocking, not 3. The thread got no reply and wasn't resolved.
    • Incremental review: after a push that fixed one bug, the review resolved that thread with an "Addressed" reply and edited the summary in place.
  • Release notes run against real Bedrock with us.anthropic.claude-opus-5-5: one attempt, 12 entries for 0.60.6..0.60.8.
  • The fork e2e above ran on v1.0.136. CLI 2.1.163 still accepts Opus 4.8, which is why that run didn't catch item 7.
  • End-to-end with the action bump and us.anthropic.claude-opus-5-5 (test: Claude review on Opus 5.5 (aws-deadline/.github#100) crowecawcaw/deadline-cloud#28, after opening the fork's test role to Opus 5.5): v1.0.244 installed CLI 2.1.292, and the run used Opus 5.5 with no error. It took 4 turns and cost $0.16. It posted 5 inline findings plus the summary, covering all 5 planted bugs (3 blocking, 1 should-fix, 1 nit). The log shows the permission check being skipped (Bypassing write permission check ... due to allowed_non_write_users='*').
  • Not exercised end-to-end: a PR from an author without write access. The docs: AE SMF support #28 author has write access, so the run shows that the bypass takes effect but not the fork-author case. In the action's code, the * check returns before any permission lookup, so the actor's access level doesn't matter.
  • Not exercised end-to-end: GitHub rejecting a single inline comment (the partial-post path). It's covered by the post() unit tests.

Was this change documented?

Yes, in the docstrings, the workflow comments, and the agent prompt (the new side field).

Is this a breaking change?

No.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…sts, and re-runs

Follow-ups to aws-deadline#99 from review feedback:

- Only trust a review thread's fp marker when the bot wrote its first
  comment. Anyone can open a thread, and the fp format is predictable, so a
  forged marker could suppress the real finding, skew the open counts, or get
  a human's thread replied to and resolved.
- Neutralize HTML comments in model text before posting, so a marker the PR
  induces the model to write cannot outrank the one we append.
- A partial post no longer counts as a finished review: the baseline stays
  put, the status is an error, and the summary lists the findings that did
  not land.
- Findings can anchor to removed code (side LEFT), so a finding on a deleted
  file is no longer dropped for want of a new-side line.
- Name the review artifact per run rather than per attempt, so re-running
  only a failed post job finds it, and re-read the PR's threads before
  posting so that re-run does not duplicate the comments that landed.
- Release notes: auto tool choice can return plain text; retry up to three
  times with a reminder instead of failing on the first text answer. An
  empty entries list is a valid result.

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
@crowecawcaw
crowecawcaw marked this pull request as ready for review October 6, 2026 20:04
@crowecawcaw
crowecawcaw requested a review from a team as a code owner October 6, 2026 20:04
andychoquette
andychoquette previously approved these changes Oct 6, 2026
v1.0.136 bundles Claude Code CLI 2.1.163 (claude-agent-sdk 0.3.163), which
Bedrock rejects for us.anthropic.claude-opus-5-5 with a 400: version 2.1.280
or newer is required. v1.0.244 bundles CLI 2.1.292. The action's inputs are
unchanged between the two versions.

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
@crowecawcaw crowecawcaw changed the title fix(claude-review): harden posting against forged markers, partial posts, and re-runs fix(claude-review): harden posting and bump claude-code-action for Opus 5.5 Oct 6, 2026
if: steps.meta.outputs.resolved == 'true'
timeout-minutes: ${{ inputs.review_timeout_minutes }}
uses: anthropics/claude-code-action@0b1b62002952733671bde978d429b50b51c51c85 # v1.0.136
uses: anthropics/claude-code-action@58985842b834ed26087302ba27d07bc24ca8697a # v1.0.244

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

claude-code-action v1.0.244 now runs collaborator permission checks for workflow_run. It checks both github.actor and workflow_run.actor, then fails if either lacks repository write access.

This workflow exists specifically to review public fork PRs. Those authors do not have write access, and allowed_non_write_users is unset, so every external-contributor review will fail before Claude runs. Re-running will not help because the upstream run actor remains the fork author.

Add:

allowed_non_write_users: "*"

This is appropriate here because the review job has a read-only token, exposes only read tools, and delegates posting to a separate deterministic job. Test it using a real fork PR before merging.

I verified this directly in the source pinned by 58985842…; v1.0.136 did not perform this check for workflow_run.

claude-code-action v1.0.244 checks write access for workflow_run events, for both github.actor and workflow_run.actor. On fork PRs that's the fork author, who never has write access, so every external review failed before Claude ran. The review job's token is read-only, Claude only gets read tools, and posting happens in the separate post job, so bypassing the check is safe here.

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
@crowecawcaw
crowecawcaw force-pushed the fix/claude-review-feedback branch from 2d2e528 to b0314f5 Compare October 7, 2026 16:02
@crowecawcaw
crowecawcaw merged commit f8e55a0 into aws-deadline:mainline Oct 7, 2026
7 checks passed
pr_lines = diff_right_lines((env.context_dir / "pr.diff").read_text(encoding="utf-8"))
# Also suppress what is on the PR now: a re-run of this job after a
# partial post must not post the findings that did land a second time.
suppress |= suppressed_fps(fetch_threads(env.repo, env.pr))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When two findings share a base fingerprint, they are assigned base and base-2. If base posts but base-2 fails, the re-run adds base to suppress. select_findings checks the unsuffixed fingerprint against suppress before
assigning numeric suffixes, so it drops both records.

The re-run then reports no unposted findings and advances the baseline, permanently losing base-2. I reproduced this locally.

Assign deterministic suffixes before applying suppression, then compare the suffixed fingerprint against live threads. Add a test where two same-symbol findings are generated and only the second fails to post.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants