Skip to content

Review agent should read plan/spec documents linked from PR description and validate implementation completeness #269

Description

@fullsend-ai-retro

What happened

PR #4079 added repos diff and repos sync commands. The PR description explicitly stated "Implements PR 6 from the repos management plan" with a clickable link to the plan document. The review agent ran 9 times before the human review (Jul 10-11) but never read this plan document. The CRITICAL finding — guard-variable FULLSEND_PER_REPO_INSTALL not being reconciled as the plan spec requires — was caught only by waynesun09, who explicitly cross-referenced the implementation against the plan's PR 6 requirements table. The review agent's correctness sub-agent has a general instruction about "design document alignment" and the intent-coherence sub-agent reads "CLAUDE.md, AGENTS.md, and any ADRs referenced by changed files," but neither sub-agent has a systematic process for extracting document links from the PR description and reading them. The agent accepted the code's self-documentation (a code comment saying the guard "can never differ from the desired value in this context") at face value without verifying this claim against the normative spec. Review agent runs: 29125182390 through 29149804471.

What could go better

The review agent should extract internal repo document links from the PR description (markdown links to files in docs/plans/, docs/ADRs/, docs/specs/, etc.) and read them as part of the review context. When the PR claims to implement a specific section or item from a plan, the agent should validate that all requirements from that section are addressed. This would have caught the guard-variable gap because the plan's PR 6 table lists FULLSEND_PER_REPO_INSTALL as a reconciled variable, but managedVariables in the implementation omitted it. Confidence is high that this gap caused the miss — waynesun09's review was explicitly framed as cross-referencing the PR against the plan spec, and this was the method that surfaced the CRITICAL finding. Uncertainty: the plan document is ~1,800 lines; adding it to sub-agent context increases token cost per run. The pr-review skill may need to extract only the relevant section rather than the full document.

Proposed change

Enhance the pr-review orchestrator (SKILL.md, step 2 or step 3) to:

  1. Parse the PR description for internal repo document links — markdown links matching patterns like docs/plans/*.md, docs/ADRs/*.md, docs/specs/*.md, or relative paths to .md files in the repo.
  2. Read linked documents (or the relevant section if the link includes a fragment identifier) and include them in the context package passed to the correctness and intent-coherence sub-agents.
  3. When the PR description uses implementation-claiming language ("Implements X from [plan]", "Closes requirement Y in [spec]"), add a specific check to the correctness sub-agent's prompt: validate that each requirement listed in the referenced section is implemented or explicitly descoped in the PR.

This complements the existing "Design document alignment" instruction in the correctness sub-agent by providing the document to check against, rather than relying on the sub-agent to independently discover and read it.

Files to modify:

  • skills/pr-review/SKILL.md — add document extraction in the context-gathering phase
  • skills/pr-review/sub-agents/correctness.md — strengthen the design-document-alignment instruction with explicit plan-completeness checking
  • Optionally skills/pr-review/sub-agents/intent-coherence.md — add plan-linked document reading alongside the existing ADR reading

Validation criteria

  1. Run the review agent against the PR #4079 diff with the enhanced skill. The agent should read docs/plans/repos-management.md and flag that FULLSEND_PER_REPO_INSTALL is listed as a reconciled variable in the plan's PR 6 table but omitted from the implementation's managedVariables.
  2. On a test PR that links to a plan document and implements 3 of 4 listed requirements, the agent should flag the missing requirement.
  3. On a PR that does NOT link to any plan documents, behavior should be unchanged (no regression in review time or token cost).
  4. Token cost increase for plan-referencing PRs should be bounded — reading the relevant section only, not the full document if it exceeds a size threshold (e.g., 500 lines).

Generated by retro agent from fullsend-ai/fullsend#4079

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions