Skip to content

Design review: PR review feedback piped into architect/implementer as structured input #247

Description

@PolyphonyRequiem

Summary

The PR review loop today routes the aggregate vote (approved / changes_requested / pending) back into the architect / implementer agent, but does not pipe per-comment, line-level, or rationale text into the agent''s structured input. The agent is on the honor system to call gh pr view --comments (or the ADO equivalent) and remember to incorporate what it reads. We should review whether to make this an explicit pipe.

This is an architecture-level question, not a bug — filing for design review, not immediate fix.

How it works today

What flows back to the agent (verified — works)

  • polyphony pr poll-status (src/Polyphony/Commands/PrCommands.PollStatus.cs) consumes:
    • GitHub PR reviewDecision (APPROVED / CHANGES_REQUESTED / PENDING)
    • Per-reviewer last-vote-wins (any changes_requested blocks; otherwise any approved satisfies; otherwise pending) — PrCommands.PollStatus.cs:170-194
    • Magic-string fallback on top-level comments: polyphony:approve / polyphony:request-changes regex match — needed because GitHub blocks PR-author self-approval (issue PR self-approval gap on GitHub: magic comment is interim; revisit other options #207). See MagicApproveRegex / MagicRequestChangesRegex at PrCommands.PollStatus.cs:288-294.
    • ADO equivalent reviewer-vote schema in PrCommands.PollStatusAdo.cs.
  • Resulting state == ''changes_requested'' routes:
    • plan-level.yaml:1115-1116revise_counterarchitect
    • Same shape exists for MG / impl / feature PR legs.
  • revise_counter caps at 5 then surfaces revise_cap_gate (operator can re-poll, force one more revision, or abort).

What does NOT flow back (the gap)

  • Inline review comments (line-level / file-level) — invisible to the agent unless it explicitly fetches them.
  • Review-summary text from a CHANGES_REQUESTED review — invisible.
  • Resolved / unresolved thread metadata — invisible.
  • The body of magic-string-bearing comments (e.g. an operator could write polyphony:request-changes — the YAML edit needs to also cover apex-driver.yaml line 1085; only the polyphony:request-changes token is acted on, the rationale is not piped to the architect).
  • The architect prompt at plan-level.yaml:497-498 literally instructs the agent: "On revision loop entry (changes_requested from plan_reviewer/human): must read PR comments via gh and incorporate feedback." — i.e. the agent must remember to go look.
  • The pending_review_gate operator UI (plan-level.yaml:1253-1285) offers continue / abort only — there''s no in-conductor "I left comments, please reroute to the architect with this feedback bound to the prompt" affordance.

Why this is a gap

  • Architect skipping feedback is a silent failure mode. If the agent forgets to call gh pr view --comments (or hits a token / network issue and the call fails non-fatally), it will produce a "revision" that ignores the actual review feedback. The aggregate vote alone is not enough signal to revise meaningfully.
  • No structured place for human rationale. Operators today have to choose between (a) leaving a native PR review (which gets aggregated into a single vote with no rationale flow-through) and (b) commenting polyphony:request-changes (only the token matters; the rationale is dropped on the floor).
  • The gap repeats per PR class. Plan PRs, MG PRs, impl PRs, and feature PRs each have their own poll-status / revise routing copies, so any fix needs to land in a shared subworkflow (or be replicated) — not just plan-level.yaml.
  • Inline-comment ergonomics matter for the implementer especially. A reviewer leaving "this method should be internal sealed" on line 47 is the highest-value signal class; the workflow currently throws it away.

Shape options to chew on (no decision yet)

  1. pr_comments_loader step + structured input binding. Add a script step that runs after poll-status == changes_requested, calls polyphony pr fetch-comments (new verb), emits { summary_text, threads: [{ file, line, body, author, resolved }], magic_comment_rationale }, and the next architect step binds it as workflow.input.review_feedback. Architect prompt is rewritten to read from the bound input rather than "remember to go look".
  2. Comment-driven gate option. Add a third pending_review_gate choice revise_now that ignores the platform vote and forces routing to the architect with the latest comments bound — useful for operators who left advisory comments without a formal review.
  3. Magic-comment rationale capture. Have poll-status capture the full body of any polyphony:request-changes-bearing comment (not just the token) and surface it as output.magic_rationale. Smallest possible improvement; doesn''t need a new verb.
  4. Do nothing — document the contract. Honor-system stays. Make the architect prompt include an explicit mandatory sub-step 1. Run gh pr view --comments and read every thread before drafting revisions. and rely on the model to comply.
  5. Some hybrid — magic-rationale capture (cheap) + loader step for inline threads (more invasive) + gate option to operator-trigger a revision.

Affected locations

  • src/Polyphony/Commands/PrCommands.PollStatus.cs (GitHub leg)
  • src/Polyphony/Commands/PrCommands.PollStatusAdo.cs (ADO leg)
  • src/Polyphony/Models/PrPollStatusResult.cs (output schema would extend)
  • .conductor/registry/workflows/plan-level.yaml (pending_review_gate, poll_statusrevise_counter route, architect prompt + input bindings)
  • .conductor/registry/workflows/implement-mg.yaml and feature-pr.yaml (carry-forward — same shape lives there)
  • .conductor/registry/prompts/architect-plan-level.md (the "must read PR comments via gh" instruction)
  • ADO leg parity (currently lighter; ADO inline comments have a different API surface)

Acceptance for this issue

  • Decision recorded (ADR or PR description) on whether to:
    • extend poll-status schema with comment payload, OR
    • add a separate pr fetch-feedback verb, OR
    • keep the honor-system contract and just tighten the architect prompt.
  • If we go with structured-pipe, dispatch follow-up implementation issues per PR class (plan / MG / feature).
  • If we keep honor-system, document the decision so this gap stops getting re-discovered.

Context

Discovered during AB#3066 dogfood (silent-swallow fix, mid-session 2026-05-09). The plan PR #246 reached pending_review_gate; the operator noted that there''s no way to leave actionable feedback that the architect would necessarily see — only the aggregate vote routes back. Magic-string approval works for self-approve (issue #207); magic-string change-request works as a vote signal but the rationale is dropped.

Companion to #207 (self-approval magic comments).

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

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions