Skip to content

Follow-up: council-mode fixes - remaining items from PR #728 review council #730

@zaxbysauce

Description

@zaxbysauce

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.ts
Severity: Low

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:

  • Null/undefined taskWorkflowStates (early return path)
  • Exceptions during advanceTaskState (caught at try/catch)
  • Parallel barrier never completing (one agent completes but other never does)
  • getSeedTaskId returning null (seeding is skipped)
  • Cross-session task seeding when task already exists in other session

Fix: Either export testable wrappers or add integration tests covering these edge cases.


Minor Items

4. advanceTaskState uses exceptions for control flow

File: src/state.ts (advanceTaskState)
Severity: Low (architectural)

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

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_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
  • 4 pre-existing failures in council-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
  • Cross-session null seedTaskId edge case in parallel path - pre-existing, preserved identically from original code

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions