plan: #3172 - #379
Conversation
PolyphonyRequiem
left a comment
There was a problem hiding this comment.
🤖 Polyphony plan reviewer (advisory)
Summary: The plan is tightly scoped, well-grounded in concrete file paths/line ranges, and the audit-vs-gap-fill posture is appropriate given that most of the work already landed in the tree. However, it bakes in a hard-coded terminal-state set as the source of truth, which is the exact P5 anti-pattern the polyphony reviewer guidance flags as a blocker. There is also at least one real gap in the existing implementation (the polyphony reset remediation hint required by AC #2) that the plan should name explicitly so the audit pass does not rubber-stamp it.
Strengths
- Crisp, falsifiable acceptance criteria — each terminal state, each bypass intent, and the probe-failure non-blocking case are individually testable.
- Honest framing: tells the implementer the work is likely already present and reduces their job to audit + surgical gap-fill rather than re-implementing.
- Concrete pointers (script + Pester file paths, line ranges,
Describeblock name) — a downstream agent can find the code without spelunking. - Probe-failure semantics are spelled out (skip with a warning, never block the happy path), which avoids a common over-fitting failure mode.
Concerns
- Hard-coded state strings (P5 anti-pattern). Both the plan and the existing implementation enshrine
('Done', 'Closed', 'Removed', 'Resolved')and'To Do'as literals in PowerShell. Per repo reviewer guidance, terminal/state names must be sourced frompolyphony validateor.polyphony-config/process-config.yaml, not pasted into scripts. This is the same regression class as commits9f96f8b,03aab89,5ea9929. The plan should either (a) require sourcing the terminal set from config, or (b) explicitly justify why this script is exempt and add a regression test that fails if config and script drift. - AC #2 vs. current message. The throw at
Invoke-PolyphonySdlc.ps1:287-303mentionstwig state … 'To Do'(reopen) but does not mentionpolyphony reset --root-id N, which AC #2 requires as a remediation path. The plan says "if gaps are found, close them with surgical edits" but does not call out this specific gap, so a fast audit could miss it. Recommend listing it explicitly under "known likely gaps to verify." - Bypass coverage gap in AC. AC #4 names
-Intent resume,-Intent replan, and-SkipStateCheckas bypasses, but the implementation guard at line 259 only checks$Intent -eq 'new'. That happens to cover bothresumeandreplan, but the test matrix should assert each named intent individually rather than relying on the inverse — otherwise a future refactor that adds a fourth intent would silently change behaviour. - No mention of
polyphony resetinvocation contract. The plan asks the message to referencepolyphony reset --root-id Nbut does not say whether the implementer should verify that verb actually exists / accepts that flag today. Worth a one-line check so the message does not point operators at a non-existent command.
Suggested changes
- Add an acceptance criterion: "Terminal-state set is sourced from
.polyphony-config/process-config.yaml(orpolyphony validateoutput), not hard-coded in the script." If a hard-coded list is genuinely the right call here, replace this with a justification note plus a drift-detection test. - Add to the Proposed Approach a "known gaps to confirm" sub-list, starting with: message must include
polyphony reset --root-id $ApexId(currently missing). - Strengthen the test-matrix language in AC #3/#4: enumerate
-Intent resume,-Intent replan, and-SkipStateCheckas three distinct bypass test cases, each paired with a terminal state. - Note that the reopen hint
'To Do'is itself a state-name literal and should come from the same config source as the terminal set.
This is an advisory comment from the polyphony plan_reviewer agent. It does not vote — human review is the merge gate.
|
polyphony:approve 6a0ce84 |
requests_parent_change: false
ancestor_plan_generations:
root: 1
Plan for #3172 (root #3165)
Promotes
plan/3165-3172intoplan/3165.AB#3172