Skip to content

feat(code-review): hand addressed outdated threads to the maintainer to resolve - #1758

Open
shahar1 wants to merge 1 commit into
apache:mainfrom
shahar1:feat/code-review-outdated-threads
Open

shahar1 wants to merge 1 commit into
apache:mainfrom
shahar1:feat/code-review-outdated-threads

Conversation

@shahar1

@shahar1 shahar1 commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • A review thread GitHub marks outdated blocked APPROVE until someone resolved it, even when a later push had already fixed what it asked for. On a base branch with required_conversation_resolution, the PR then sat blocked and the skill never said why. This came up for real on Add EKS operator for commands in existing Pods airflow#72542: an outdated kubernetes_conn_id thread, fixed and answered "Done", was one of the two threads that held the merge.
  • code-review context now lists unresolved outdated threads (outdated_threads). The agent checks each one against the current diff (new classifications/outdated-thread.md). For each thread the code addresses, disposition --addressed-thread <id> removes it from the thread gate and returns it in resolve_first, and the skill tells the maintainer: "resolve it to unblock the merge".
  • Guard rails: only threads that are both unresolved and outdated can be passed; any other id exits with an error. The author saying "Done" does not count; the current diff has to show the fix. The skill never resolves a thread itself, since that stays the maintainer's sign-off. Addressed threads never appear in the review body. Threads that are not outdated still gate APPROVE as before.

Type of change

  • Skill change (.claude/skills/<name>/) — eval fixtures updated below
  • Python package (tools/*/ with pyproject.toml)
  • Tool / bridge contract (tools/<system>/*.md)
  • Groovy reference impl
  • Cross-cutting (RFC, AGENTS.md, sandbox, privacy-LLM)
  • Documentation (docs/, README.md, CONTRIBUTING.md)
  • Project template (projects/_template/)
  • CI / dev loop (prek, workflows, validators)
  • Other: cr-pr.graphql reads thread id / isOutdated and comment url (vetted-ops query, plugin copy synced)

Test plan

  • prek run --all-files passes
  • For Python packages touched: uv run pytest / ruff check / mypy passes — tools/pr-management/tests/code_review: 141 passed, including 3 new tests: test_step6_addressed_outdated_threads_go_to_the_maintainer, test_outdated_thread_the_code_addresses_unblocks_approve, and test_addressed_thread_must_be_outdated_and_unresolved
  • For Groovy bridges touched: command-line invocation tested end-to-end
  • For skill changes: eval suite passes for the affected skill — new step-6-outdated-thread suite, 3/3 with claude -p
  • For skill behaviour changes: a new or updated eval fixture is included in this PR (step-6-outdated-thread: fixed as asked / code moved but the concern remains / author says "Done" but the diff disagrees)
  • Other: ran the edited cr-pr.graphql live against Add EKS operator for commands in existing Pods airflow#72542. With the thread state from before it was resolved, context listed the outdated kubernetes_conn_id thread, and --addressed-thread dropped the thread gate from 2 to 1 and returned it in resolve_first. The remaining thread is not outdated, so it still blocked APPROVE.

RFC-AI-0004 compliance

  • HITL — no new mutation; resolving a thread is left to the maintainer, the skill only tells them
  • Vendor neutrality — placeholders kept; eval fixtures are synthetic (acme)
  • Write-access discipline — no autonomous outbound messages; resolve_first is maintainer-facing only, never in the review body

Linked issues

Refs #1585 (the single APPROVE definition this narrows).

Notes for reviewers (optional)

  • Scope is outdated threads only. A thread that is not outdated but already answered (for example, a reviewer withdrew their concern in a reply) still gates APPROVE. Extending the same flow to those is a separate decision, because nothing in the diff can show it is addressed.
  • resolve_first is returned even when another gate keeps the result at COMMENT, so the maintainer still learns which threads they can resolve.
  • SKILL.md Golden rule 7 gained one sentence; measured_tokens was restamped (3770 → 3826); surface_hash is unchanged.

Generative AI: drafted with Claude Code (Claude Opus 5.5), reviewed by the author.

🤖 Generated with Claude Code

…to resolve

A review thread GitHub marks outdated stayed an approval gate until someone
resolved it, even when the later push had already fixed what it asked for.
The disposition fell back to COMMENT, and on a base branch that requires
conversation resolution the PR stayed blocked with nobody told why.

`code-review context` now lists unresolved outdated threads (`outdated_threads`).
The agent compares each one against the current diff. For each thread the code
addresses, `disposition --addressed-thread <id>` drops it from the thread gate and
returns it in `resolve_first`, and the skill tells the maintainer to resolve it to
unblock the merge. Threads that are not outdated, or that the code does not
visibly address, still gate APPROVE. The skill never resolves a thread itself.

`cr-pr.graphql` now reads each thread's `id` and `isOutdated` and each comment's
`url`. A new `classifications/outdated-thread.md` holds the judgement.
Golden rule 7, Step 6, the APPROVE gate docs, the tool README and the
pr-management spec name the exception. A new `step-6-outdated-thread` eval
suite covers the judgement.

Generated-by: Claude Code (Opus 5.5)
@github-actions github-actions Bot added family:pr-management pr-management-* skills family:tools tools/* family:docs Docs, MISSION.md, READMEs capability:review Deep per-item code review or contributor mentoring substrate:analytics Tool substrate: read-only metrics / dashboards / renderers substrate:sandbox Tool substrate: agent isolation, egress control, settings audit substrate:framework-dev Tool substrate: build / validate / eval the framework itself labels Oct 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

capability:review Deep per-item code review or contributor mentoring family:docs Docs, MISSION.md, READMEs family:pr-management pr-management-* skills family:tools tools/* substrate:analytics Tool substrate: read-only metrics / dashboards / renderers substrate:framework-dev Tool substrate: build / validate / eval the framework itself substrate:sandbox Tool substrate: agent isolation, egress control, settings audit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant