fix: address post-merge review feedback from PRs #164-#167 #169
Description
Post-Merge Review Feedback — PRs #164-#167
Four PRs were merged in quick succession. External reviewers (Copilot, Greptile, CodeRabbit, Gemini) posted comments after merge. All valid findings need to be addressed.
Excluded after validation against DESIGN_SPEC.md and open issues:
- Human escalation queue/persistence → depends on Implement human approval queue API (Litestar controller + guards) #37 (M6), intentional stub
- Per-conflict strategy overrides → spec mentions but not required for M4
detect_conflict/log_dissenton strategy protocol → responsibility correctly separated at service levelTokenTrackermutability → scoped single-execution state, explicitly documented- Meeting protocol interface simplicity → phases are implementation details, not contract
CRITICAL (4)
| # | File | Issue | Reviewer |
|---|---|---|---|
| C1 | communication/meeting/round_robin.py |
decisions and action_items never populated — auto_create_tasks silently non-functional |
greptile, coderabbit |
| C2 | communication/meeting/position_papers.py |
Same: minutes returned with empty decisions/action_items |
greptile, coderabbit |
| C3 | communication/meeting/structured_phases.py |
Same: synthesis prompt asks for decisions but response not parsed into structured fields | greptile, coderabbit |
| C4 | communication/conflict_resolution/_helpers.py |
find_losers() doesn't validate winning_agent_id exists in positions — all positions become losers on data integrity error |
copilot, coderabbit |
MAJOR (17)
| # | File | Issue | Reviewer |
|---|---|---|---|
| M1 | communication/meeting/round_robin.py |
Summary budget (20%) reserved even when leader_summarizes=False — wastes budget |
coderabbit |
| M2 | communication/meeting/structured_phases.py |
Discussion phase can exhaust synthesis reserve — nested sub-reserve needed | greptile |
| M3 | communication/meeting/orchestrator.py |
Duplicate participant_ids not rejected in _validate_inputs() — confusing downstream error |
copilot, coderabbit |
| M4 | communication/meeting/orchestrator.py |
Protocol registry not frozen with MappingProxyType — violates project conventions |
coderabbit |
| M5 | communication/meeting/_token_tracker.py |
record() silently allows exceeding budget — no error, no warning, no log |
coderabbit |
| M6 | communication/conflict_resolution/_helpers.py |
pick_highest_seniority() lacks hierarchy tiebreaker — inconsistent with AuthorityResolver |
copilot, greptile |
| M7 | communication/conflict_resolution/debate_strategy.py |
Authority fallback uses pick_highest_seniority() without hierarchy tiebreaker |
copilot |
| M8 | communication/conflict_resolution/hybrid_strategy.py |
Same: _authority_fallback() doesn't consult hierarchy |
coderabbit |
| M9 | communication/delegation/hierarchy.py |
get_lowest_common_manager(a, a) returns supervisor instead of a — missing fast-path |
copilot, coderabbit |
| M10 | core/enums.py |
_SENIORITY_ORDER silently depends on enum declaration order — no validation guard |
greptile |
| M11 | communication/conflict_resolution/config.py |
max_tokens_per_argument is dead config — never consumed by any strategy or evaluator |
coderabbit |
| M12 | engine/decomposition/models.py |
DecompositionResult validator doesn't verify task IDs match plan subtask IDs — only checks count |
copilot, greptile, coderabbit |
| M13 | engine/decomposition/models.py |
derived_parent_status returns COMPLETED for mixed completed+cancelled states |
greptile |
| M14 | engine/decomposition/rollup.py |
Double-logging DECOMPOSITION_ROLLUP_COMPUTED when total == 0 (warning + debug) |
greptile |
| M15 | engine/decomposition/service.py |
Created Task instances have dependencies=() — subtask dependencies not copied from plan |
coderabbit |
| M16 | engine/routing/models.py |
Duplicate subtask IDs in RoutingResult silently discarded (set conversion) instead of rejected |
coderabbit |
| M17 | communication/bus_memory.py |
Single unsubscribe sentinel only wakes one waiter — multiple pending receive() calls left blocked |
coderabbit |
MINOR (15)
| # | File | Issue | Reviewer |
|---|---|---|---|
| m1 | communication/meeting/structured_phases.py |
MEETING_CONFLICT_DETECTED emitted at both INFO and DEBUG — duplicate event |
coderabbit |
| m2 | communication/meeting/structured_phases.py |
assert used for invariants — skipped with python -O |
copilot |
| m3 | communication/meeting/position_papers.py |
Same assert issue for result_inputs/result_contributions |
copilot |
| m4 | communication/meeting/_prompts.py |
presenter_id dropped from formatted agenda prompt |
coderabbit |
| m5 | communication/meeting/models.py |
Aggregate token fields not validated against sum of contributions | coderabbit |
| m6 | communication/meeting/models.py |
error_message accepts empty string for FAILED/BUDGET_EXHAUSTED status |
coderabbit |
| m7 | communication/conflict_resolution/service.py |
Private _MIN_POSITIONS imported across module boundary |
greptile |
| m8 | core/enums.py |
compare_seniority() uses O(n) .index() — should precompute rank dict |
copilot |
| m9 | communication/bus_memory.py |
asyncio.QueueFull catch is dead code — queue is unbounded |
greptile |
| m10 | communication/bus_memory.py |
_log_receive_null() infers wake-up reason outside lock — racy state check |
copilot, coderabbit |
| m11 | communication/messenger.py |
channel_name parameter typed as str instead of NotBlankStr |
coderabbit |
| m12 | communication/messenger.py |
receive() docstring missing unsubscribe as None return path |
copilot |
| m13 | engine/parallel.py |
Re-raising task_error outside except block can lose traceback context |
copilot |
| m14 | engine/routing/service.py |
No validation that parent_task.id matches plan.parent_task_id |
copilot |
| m15 | engine/routing/models.py |
Routing model validators raise without logging (violates CLAUDE.md convention) | coderabbit |
TRIVIAL (4)
| # | File | Issue | Reviewer |
|---|---|---|---|
| t1 | engine/routing/scorer.py |
Logger uses string literal event name instead of centralized constant | copilot |
| t2 | core/task.py |
Task docstring missing task_structure and coordination_topology fields |
coderabbit |
| t3 | DESIGN_SPEC.md |
Package tree says AgendaItem but actual model is MeetingAgendaItem |
coderabbit |
| t4 | communication/meeting/config.py |
StructuredPhasesConfig docstring lists auto_create_tasks (moved to MeetingProtocolConfig) |
copilot, greptile |
Test Coverage Gaps (6)
| # | File | Gap | Reviewer |
|---|---|---|---|
| T1 | tests/unit/communication/test_enums.py |
MEETING_CONTRIBUTION enum value not asserted in test_values() |
copilot, coderabbit |
| T2 | tests/integration/communication/test_meeting_integration.py |
Missing pytestmark = pytest.mark.timeout(30) |
copilot |
| T3 | All tests/unit/communication/meeting/ modules |
Missing pytestmark = pytest.mark.timeout(30) |
copilot |
| T4 | tests/unit/communication/conflict_resolution/test_authority_strategy.py |
No 3+ participant test cases | coderabbit |
| T5 | tests/unit/communication/conflict_resolution/test_debate_strategy.py |
max_tokens_per_argument never exercised in tests |
coderabbit |
| T6 | DESIGN_SPEC.md |
Spec references find_loser/build_dissent_record but code has find_losers/build_dissent_records (plural) |
copilot |
Total: 40 code issues + 6 test/doc gaps = 46 items