You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Follow-up items identified by the independent review council after completing PR #728 follow-up fixes (branch fix-pr728-followup). These are non-blocking improvements discovered during holistic assessment of the 6 completed tasks.
Priority Items
1. REJECT verdict evidence file not verified in tests
The REJECT synthesis test verifies parsed.overallVerdict === 'REJECT' but does NOT read the evidence file to confirm it was written with verdict: 'REJECT'. The synthesizePhaseCouncilAdvisory function always writes evidence regardless of verdict, but this is never asserted for the REJECT path.
Fix: Add expect(evidence.verdict).toBe('REJECT') assertion in the REJECT synthesis test.
2. Third copy of Stage B advancement in update-task-status.ts:475-514
File: src/tools/update-task-status.ts (lines 475-514) Severity: Medium
The PR #728 follow-up extracted 3 near-copies of reviewer_run to tests_run advancement logic into shared helpers (advanceStageBForSession/propagateStageBToOtherSessions). However, update-task-status.ts contains an independent Stage B advancement loop (lines 475-514) that duplicates the same coder_delegated to reviewer_run to tests_run pattern. It does NOT use the new helpers and has no recordStageBCompletion/hasBothStageBCompletions in parallel mode.
Fix: Extract this third copy to use the shared helpers, or apply the same refactoring pattern.
3. Helpers lack direct unit tests
Files: src/hooks/delegation-gate.ts (lines 597-734) Severity: Medium
The two new helpers (advanceStageBForSession, propagateStageBToOtherSessions) are closure-private inside createDelegationGateHook and have no direct unit tests. They are only exercised through toolAfter integration tests. Edge cases not directly tested:
advanceTaskState throws INVALID_TASK_STATE_TRANSITION for illegal transitions. The helpers guard against this with eligibleStates filtering, but the underlying API uses exceptions for control flow - an anti-pattern that creates coupling between the helper guards and advanceTaskState internals. If the guard logic drifts, swallowed errors become invisible failures.
Suggestion: Add a canAdvanceTaskState(session, taskId, newState): boolean predicate in state.ts to eliminate exception-based control flow.
5. stageBCompletion Sets are append-only - stale data risk
File: src/hooks/delegation-gate.ts Severity: Low
The stageBCompletion Sets are append-only and never cleared. If task states are ever reset (retry/restart semantics), stale completion bits could cause premature barrier firing.
Suggestion: Clear stageBCompletion for a taskId when advanceTaskState transitions through complete, or when a task is explicitly reset.
6. Council-disabled test doesn't assert taskCouncilApproved remains unset
The "council disabled" fallback test verifies state advances to tests_run but does NOT assert session.taskCouncilApproved is null/undefined. The test's purpose is to verify fallback path runs Stage B when council is off, but the absence of council verdict recording is not confirmed.
Fix: Add assertion to verify taskCouncilApproved is undefined/null.
7. Reviewer gate missed logger env-var guard
Severity: Process lesson
The reviewer gate APPROVED Task 1.2 (console.warn to logger.warn) before the test_engineer caught that logger.warn is gated behind OPENCODE_SWARM_DEBUG=1. The two-gate pipeline correctly self-corrected, but future reviewers should check whether utility functions have side-effect guards (env vars, feature flags) before approving substitutions.
Already Deferred (not actionable)
8 skipped describe.skip tests in architect-council-prompt.test.ts (lines 290-352) - documented as requiring follow-up PR with ARCHITECT_PROMPT Stage B section changes
Summary
Follow-up items identified by the independent review council after completing PR #728 follow-up fixes (branch
fix-pr728-followup). These are non-blocking improvements discovered during holistic assessment of the 6 completed tasks.Priority Items
1. REJECT verdict evidence file not verified in tests
File:
tests/unit/tools/submit-phase-council-verdicts.unit.test.tsSeverity: Low
The REJECT synthesis test verifies
parsed.overallVerdict === 'REJECT'but does NOT read the evidence file to confirm it was written withverdict: 'REJECT'. ThesynthesizePhaseCouncilAdvisoryfunction always writes evidence regardless of verdict, but this is never asserted for the REJECT path.Fix: Add
expect(evidence.verdict).toBe('REJECT')assertion in the REJECT synthesis test.2. Third copy of Stage B advancement in
update-task-status.ts:475-514File:
src/tools/update-task-status.ts(lines 475-514)Severity: Medium
The PR #728 follow-up extracted 3 near-copies of reviewer_run to tests_run advancement logic into shared helpers (
advanceStageBForSession/propagateStageBToOtherSessions). However,update-task-status.tscontains an independent Stage B advancement loop (lines 475-514) that duplicates the samecoder_delegatedtoreviewer_runtotests_runpattern. It does NOT use the new helpers and has norecordStageBCompletion/hasBothStageBCompletionsin parallel mode.Fix: Extract this third copy to use the shared helpers, or apply the same refactoring pattern.
3. Helpers lack direct unit tests
Files:
src/hooks/delegation-gate.ts(lines 597-734)Severity: Medium
The two new helpers (
advanceStageBForSession,propagateStageBToOtherSessions) are closure-private insidecreateDelegationGateHookand have no direct unit tests. They are only exercised throughtoolAfterintegration tests. Edge cases not directly tested:taskWorkflowStates(early return path)advanceTaskState(caught at try/catch)getSeedTaskIdreturning null (seeding is skipped)Fix: Either export testable wrappers or add integration tests covering these edge cases.
Minor Items
4.
advanceTaskStateuses exceptions for control flowFile:
src/state.ts(advanceTaskState)Severity: Low (architectural)
advanceTaskStatethrowsINVALID_TASK_STATE_TRANSITIONfor illegal transitions. The helpers guard against this witheligibleStatesfiltering, but the underlying API uses exceptions for control flow - an anti-pattern that creates coupling between the helper guards andadvanceTaskStateinternals. If the guard logic drifts, swallowed errors become invisible failures.Suggestion: Add a
canAdvanceTaskState(session, taskId, newState): booleanpredicate instate.tsto eliminate exception-based control flow.5.
stageBCompletionSets are append-only - stale data riskFile:
src/hooks/delegation-gate.tsSeverity: Low
The
stageBCompletionSets are append-only and never cleared. If task states are ever reset (retry/restart semantics), stale completion bits could cause premature barrier firing.Suggestion: Clear
stageBCompletionfor a taskId whenadvanceTaskStatetransitions throughcomplete, or when a task is explicitly reset.6. Council-disabled test doesn't assert taskCouncilApproved remains unset
File:
tests/unit/hooks/delegation-gate-council-fallback.test.ts(lines 252-294)Severity: Low
The "council disabled" fallback test verifies state advances to
tests_runbut does NOT assertsession.taskCouncilApprovedis null/undefined. The test's purpose is to verify fallback path runs Stage B when council is off, but the absence of council verdict recording is not confirmed.Fix: Add assertion to verify
taskCouncilApprovedis undefined/null.7. Reviewer gate missed logger env-var guard
Severity: Process lesson
The reviewer gate APPROVED Task 1.2 (console.warn to logger.warn) before the test_engineer caught that
logger.warnis gated behindOPENCODE_SWARM_DEBUG=1. The two-gate pipeline correctly self-corrected, but future reviewers should check whether utility functions have side-effect guards (env vars, feature flags) before approving substitutions.Already Deferred (not actionable)
describe.skiptests inarchitect-council-prompt.test.ts(lines 290-352) - documented as requiring follow-up PR with ARCHITECT_PROMPT Stage B section changescouncil-fast-path-quorum.test.ts- exist in original PR fix(council): make council additive at phase-level, never suppress per-task Stage B gates #728 branch, unrelated to this fix set