[AI:Task] T20260705-8B7B4079: Split aop-planner-stage.ts under the 200 code-line cap by extracting PR-body rendering - #39
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the AOP planner stage composer by extracting PR title/body/commit-message rendering (and related helpers) into shared modules, with tests, to reduce the size and responsibilities of aop-planner-stage.ts within the engine’s pipeline composers.
Changes:
- Extracted planner PR content rendering (rejection / catch-up / plan) plus terminal-comment helper into
_shared/planner-pr-body.tswith new Vitest coverage. - Split
buildAopPlannerAfterAgentinto its own module and moved shared scratch/deps types into_shared/. - Updated the task work-item to
completedwith acompleted_attimestamp.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| engine/pipeline/composers/aop-planner-stage.ts | Refactors imports and delegates PR-body construction to shared builders; re-exports moved afterAgent. |
| engine/pipeline/composers/aop-planner-after-agent.ts | New module containing the extracted afterAgent hook logic. |
| engine/pipeline/composers/_shared/planner-pr-body.ts | New shared PR-body/title/commit-message builders + terminal-comment string helper. |
| engine/pipeline/composers/_shared/planner-pr-body.test.ts | Adds unit tests to lock PR-body rendering behavior. |
| engine/pipeline/composers/_shared/aop-planner-scratch.ts | Extracts the scratch-store shape and singleton into a shared module. |
| engine/pipeline/composers/_shared/aop-planner-deps.ts | Extracts the hook dependency interface into a shared module for reuse. |
| .operator/data/tasks/T20260705-8B7B4079.md | Marks the task as completed and records completion time. |
Comments suppressed due to low confidence (2)
engine/pipeline/composers/aop-planner-after-agent.ts:128
- This reviewer-facing message hardcodes “develop” as the merge target. Since the base branch is configurable per repo, this wording can be wrong; prefer a branch-agnostic phrase like “base branch”.
`${deps.displayName} **${itemId}** determined invalid by ${deps.agentRole}: ${reason}${suffix}\n\nThe PR carries the \`status: rejected\` flip ready for review. Merge to propagate the rejection to develop, or close-without-merge if you disagree (the supervisor handles override on the next cycle).`,
engine/pipeline/composers/_shared/planner-pr-body.ts:105
- These terminal bot-comment strings claim the PR is being closed (“Closing PR — …”), but this stage path only posts a comment and flips work-item state (no close call). That can mislead reviewers; consider removing “Closing PR — …” or rephrasing as an instruction to the reviewer (and update planner-pr-body.test.ts).
case "cancelled":
return `${displayName} **${itemId}** cancelled: ${reason}. Closing PR — no retry will be attempted.`;
case "rejected":
return `${displayName} **${itemId}** rejected: ${reason}. Closing PR — the retrospective will regenerate a replacement item with updated scope.`;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Applied review feedback. |
dzykovic
left a comment
There was a problem hiding this comment.
[AI-REVIEWER] PASS (high) — premise verified: aop-planner-stage.ts was 561 lines on master (cap 200), now 111 with every extracted file ≤192. Behaviour preservation checked mechanically: comment-stripped diffs of afterAgent/synthesizeAgentResult/buildRunInput are empty; a normalised literal-by-literal diff of old buildPR vs _shared/planner-pr-body.ts shows exactly one change — the Copilot-raised "propagate the rejection to develop" → "to the base branch" copy fix, already dispositioned. Test file untouched and still exercising all moved code through barrel re-exports, +148 test lines, CI green at 376d824. No collision with PR #38 (disjoint file sets). Non-blocking notes for follow-up: (1) the copy fix means the PR is not literally "behaviour-preserving" as the task text claims — the delta is intended product copy; (2) four new implementation files rely on the neighbouring aggregate test rather than colocated tests — same concern as raised as a blocker on PR #38, settle one convention; (3) _shared/ on master means "imported by 2+ composers" while these new _shared/ modules have a single consumer family — worth settling before the remaining split tasks of F20260705-CC7FF1B9 land.
Task T20260705-8B7B4079: Split aop-planner-stage.ts under the 200 code-line cap by extracting PR-body rendering
Priority: 4 | Parent: F20260705-CC7FF1B9
Summary
Problem
engine/pipeline/composers/aop-planner-stage.tsis 561 total / ~425 codelines (excluding logging/comments/JSDoc) — well over the
engine/pipeline/**≤200 cap.
buildAopPlannerBuildPR(270-354) carries three PR-body renderings(rejection body
288-302, catch-up body315-325, plan-path template call332-342), andbuildTerminalComment(552-561) is a standalone stringhelper.
Solution
Extract the PR-title/body/commit-message construction for the rejection,
catch-up, and plan paths into
engine/pipeline/composers/_shared/planner-pr-body.ts(pure functions takingthe item + rejection/catch-up context, returning
{title, body, commitMessage}), with a colocatedplanner-pr-body.test.ts. Optionally movebuildTerminalCommentthere too.buildAopPlannerBuildPRbecomes a thindispatcher that reads scratch and calls the extracted builder.
Behavior-preserving: scratch handling, verdict routing, and label transitions
are unchanged. Keep all logging lines in place.
Affected Files
engine/pipeline/composers/aop-planner-stage.ts:270-354— reducebuildAopPlannerBuildPRto a dispatcher; import from_shared[…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.