[AI:Task] T20260705-7E556EBC: Split pr-feedback-supervisor-stage.ts under the 200 code-line cap by extracting the supervisor prompt builder - #38
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the PR feedback supervisor stage composer by extracting prompt-building and after-agent handling into _shared/ helpers, aiming to bring engine/pipeline/** files under the 200-line cap while preserving behavior.
Changes:
- Extracted
buildSupervisorTaskintoengine/pipeline/composers/_shared/supervisor-task.tswith colocated tests. - Extracted the
afterAgentimplementation intoengine/pipeline/composers/_shared/supervisor-after-agent.ts, and extracted bot-comment string builders intoengine/pipeline/composers/_shared/supervisor-bot-messages.tswith tests. - Moved the stage scratch store/types into
engine/pipeline/composers/_shared/supervisor-scratch.ts.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| engine/pipeline/composers/pr-feedback-supervisor-stage.ts | Uses extracted _shared/ helpers for supervisor task construction and after-agent processing. |
| engine/pipeline/composers/pr-feedback-supervisor-stage.test.ts | Removes prompt-builder tests now covered by _shared/ tests; keeps stage-logic tests. |
| engine/pipeline/composers/_shared/supervisor-task.ts | New extracted supervisor prompt builder. |
| engine/pipeline/composers/_shared/supervisor-task.test.ts | New tests for extracted supervisor prompt builder. |
| engine/pipeline/composers/_shared/supervisor-scratch.ts | New extracted scratch store + key helper for the supervisor stage. |
| engine/pipeline/composers/_shared/supervisor-bot-messages.ts | New extracted bot-comment message formatters. |
| engine/pipeline/composers/_shared/supervisor-bot-messages.test.ts | New tests for extracted bot-comment message formatters. |
| engine/pipeline/composers/_shared/supervisor-after-agent.ts | New extracted after-agent implementation (AOP apply + thread disposition + messaging). |
| .operator/data/tasks/T20260705-7E556EBC.md | Marks the task as completed and records completed_at. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Applied review feedback. |
dzykovic
left a comment
dzykovic
left a comment
There was a problem hiding this comment.
[AI-REVIEWER] CHANGES_REQUESTED (high) — the premise checks out (620 lines / 478 code lines vs the 200 cap) and the move itself is faithful: every branch traced (limit-reached, stale-CI downgrade, terminal verdict, changes/no-changes, thread-disposition ordering, finally-unlink, catch-log-rethrow) and all 14 log?.* calls survive verbatim. Every resulting file is under the cap (largest 93 lines), CI green at 89e182f. Two blockers stop the merge — see inline threads: (1) twelve new runtime modules with no colocated test (typescript.md:54; master honors it at 102/104 files, all three pre-existing _shared/ helpers comply); (2) 72 of 82 comment/JSDoc lines deleted from the code they explain (typescript.md:52 — the caps EXCLUDE comments/JSDoc, so the deletions bought nothing), two of those blocks now existing nowhere in the repository. Unanchorable note: the composer's header JSDoc also lost the sentence identifying it as a generic pattern ("supervisor LLM router over PR events with AOP-driven decisions; a pr-review stage is the canonical example") — restore it alongside the ./_shared/ pointer.
| * | ||
| * Returns `null` on the normal path (cap not reached) so `runStage` falls | ||
| * through to `buildRunInput` + the real agent invocation. | ||
| * Hook implementations live under `./_shared/`; this module re-exports |
There was a problem hiding this comment.
[AI-REVIEWER] blocker — this split lands 12 new runtime modules under _shared/ with no colocated test, breaking typescript.md:54 ("Colocated .test.ts for every implementation file. No orphan implementation files"). On master exactly 2 of 104 engine/** implementation files lack a colocated test and both are documented exceptions (entry.ts, pipeline/types.ts); this PR takes that to 16. Add a colocated .test.ts for each of: supervisor-after-agent-hook.ts, supervisor-after-agent.ts, supervisor-aop-apply.ts, supervisor-before-agent.ts, supervisor-branch-item.ts, supervisor-build-pr.ts, supervisor-build-run-input.ts, supervisor-change-detection.ts, supervisor-payload.ts, supervisor-scratch.ts, supervisor-synthesize-agent.ts, supervisor-verdict-routing.ts. Each has an independent contract worth pinning (payloadOf throwing INVALID_STAGE_INPUT, inferKindFromBranch prefix normalisation, computeReviewAttempts' task-vs-non-task offset, detectSupervisorChanges' fall-back-to-dirty-only when headSha() throws). The two type-only files (supervisor-after-agent-deps.ts, supervisor-stage-deps.ts) are legitimately exempt — do not add tests for those. Also drop the dead re-export of SupervisorAfterAgentDeps from supervisor-after-agent.ts — every consumer imports it directly from ./supervisor-after-agent-deps.js and ts-prune does not trace type re-exports.
There was a problem hiding this comment.
✅ Addressed — Added colocated .test.ts for all 12 runtime _shared/ supervisor modules and removed the dead SupervisorAfterAgentDeps re-export from supervisor-after-agent.ts.
| ciFailing: boolean, | ||
| ): Promise<{ verdictOverride?: Verdict; summaryOverride?: string } | void> { | ||
| const suffix = formatDebugRunLinkSuffix(deps.debug, deps.debugRunUrl); | ||
| const effectiveVerdict = (applied.verdict !== "approved" || applied.applyErrors.length > 0) |
There was a problem hiding this comment.
[AI-REVIEWER] blocker — the 14-line "Stale-CI guard — completes the 2026-05-13 stale-CI fix" rationale and the 17-line "2026-05-13: removed defense-in-depth 'approved + ciFailing → override to failed' check" note were both dropped from the code they explain and not moved here. typescript.md:52 is explicit that the line caps exclude comments and JSDoc — deleting them bought nothing toward the cap. The defense-in-depth note is a negative specification: it is the only thing telling a future editor that the obvious-looking "agent said approved but CI is red, so fail it" check is deliberately absent and must not be re-added. Restore both blocks above effectiveVerdict / the stale-CI branch.
There was a problem hiding this comment.
✅ Addressed — Restored both the stale-CI guard rationale and the 2026-05-13 defense-in-depth negative-spec comment above effectiveVerdict / the stale-CI branch in supervisor-verdict-routing.ts.
| } | ||
| } | ||
|
|
||
| let preAgentHeadSha = ""; |
There was a problem hiding this comment.
[AI-REVIEWER] blocker — the comment documenting why this capture must happen here ("Capture HEAD SHA AFTER the workspace handle has resolved … so afterAgent can detect commits the agent itself made via Bash. Without this anchor a clean post-commit workspace looked identical to a no-op run, triggering the wrong 'No code changes' comment.") is now gone from the entire repository. The PrFeedbackSupervisorScratch.preAgentHeadSha JSDoc explains the PR-887 symptom but not this ordering invariant, and the invariant is what makes the fix work. Restore it above the capture.
There was a problem hiding this comment.
✅ Addressed — Restored the HEAD SHA capture ordering invariant comment above the preAgentHeadSha capture in supervisor-before-agent.ts.
| import type { PrFeedbackSupervisorHookDeps } from "./supervisor-stage-deps.js"; | ||
| import { payloadOf } from "./supervisor-payload.js"; | ||
|
|
||
| export function buildPrFeedbackSupervisorSynthesizeAgentResult(deps: PrFeedbackSupervisorHookDeps) { |
There was a problem hiding this comment.
[AI-REVIEWER] note — the 10-line JSDoc on this hook was dropped, leaving an exported hook returning AgentResult | null with zero documentation of its contract. Restore it — in particular the "Returns null on the normal path (cap not reached) so runStage falls through to buildRunInput + the real agent invocation" sentence and the cost rationale.
There was a problem hiding this comment.
✅ Addressed — Restored the full synthesize-agent hook JSDoc including the null-on-normal-path contract and the Opus cost rationale.
| applyErrors: applied.applyErrors.length, | ||
| }); | ||
|
|
||
| if (payload.reviewThreads.length > 0 || applied.commentReplies.length > 0) { |
There was a problem hiding this comment.
[AI-REVIEWER] note — restore the dropped comment above this block: "Answer + resolve inline review threads the supervisor disposed of this cycle. Runs on every agent path (fix-in-place, cancel, escalate, …) so no reviewer comment is left without a note. Bot threads (Copilot) are resolved; human threads get the note but stay open for the human." It is the only statement of why the guard is reviewThreads.length > 0 || commentReplies.length > 0 rather than gated on the verdict.
There was a problem hiding this comment.
✅ Addressed — Restored the comment explaining why thread disposition runs on every agent path when reviewThreads or commentReplies are present.
| return `Applied review feedback.${suffix}`; | ||
| } | ||
|
|
||
| export function formatNoCodeChangesMessage(effectiveSummary: string, suffix: string): string { |
There was a problem hiding this comment.
[AI-REVIEWER] note — the warning governing this formatter was dropped: the engine must state only the observable fact (clean tree + unchanged HEAD) and never editorialize the reason, because verdict=approved + no changes can equally mean "escalate" — a fixed engine sentence contradicted the agent's own reasoning once before. Restore it as JSDoc on formatNoCodeChangesMessage.
There was a problem hiding this comment.
✅ Addressed — Restored the JSDoc on formatNoCodeChangesMessage stating the engine must report only observable facts and never editorialize the reason.
| readonly log?: Logger; | ||
| readonly debug?: boolean; | ||
| readonly debugRunUrl?: string; | ||
| readonly agentRole: AgentRoleName; |
There was a problem hiding this comment.
[AI-REVIEWER] note — the per-field JSDoc for the two stage-shape parameters was dropped (supervisor agent role; verifier chain topic suffix used as verifier/{verifierTopic}) along with the "Stage-shape parameters" divider. verifierTopic: string is now an undocumented magic string whose chain convention is only discoverable by opening supervisor-build-run-input.ts. Restore both.
There was a problem hiding this comment.
✅ Addressed — Restored the Stage-shape parameters divider and per-field JSDoc for agentRole and verifierTopic in supervisor-stage-deps.ts.
|
Applied review feedback. |
Task T20260705-7E556EBC: Split pr-feedback-supervisor-stage.ts under the 200 code-line cap by extracting the supervisor prompt builder
Priority: 4 | Parent: F20260705-CC7FF1B9
Summary
Problem
engine/pipeline/composers/pr-feedback-supervisor-stage.tsis 583 total /~447 code lines (excluding logging/comments/JSDoc) — well over the
engine/pipeline/**≤200 cap inintelligence/rules/typescript.md§FORBIDDEN. The largest
cohesive seam is
buildSupervisorTask(536-620, ~85 lines) — a pureprompt-string assembler with no I/O — plus the bot-comment message
construction inside
buildPrFeedbackSupervisorAfterAgent(316-534).Solution
Extract
buildSupervisorTaskintoengine/pipeline/composers/_shared/supervisor-task.ts(named export) with acolocated
supervisor-task.test.ts, matching the existing_shared/pattern (
pr-summary.ts,scratch.ts,thread-dispositions.ts). If stillover the cap, additionally extract the terminal / applied / no-change
bot-comment message strings into a small
_sharedformatter. Import theextracted symbols back into the composer.
This is a behavior-preserving refactor — no verdict, label, or AOP logic
changes. Every
deps.log?.*line stays intact and moves with its code; doNOT strip or shorten logging to fit the budget.
Affected Files
[…truncated]
Automated by AI Automation Pipeline. Current state is tracked via the PR
label (
ai:processing,ai:in-review,ai:ready-to-merge,ai:failed) and thePR comments — not this description.