Trial run-3 (2026-05-19) surfaced the first instance of a broader bug
class: V1 contract fields existed and agents emitted them, but no
controller code wired them into state transitions. An adversarial
"walk the happy path" code review found 4 more, all listed below.
The class shape: a V1 field is "Required iff X" by contract docstring,
the worker emits it correctly, but the master reads the wrong field
(or doesn't read it at all), so a critical state transition silently
no-ops or drops to the wrong default.
FIX#0 — outcome-mapper early-return (committed earlier in this
session) — moved role dispatch before the ``outcome is None`` guard
so estimator+reviewer+summarizer (V1 contracts without an ``outcome``
field) are correctly handled. Without this fix, all estimator
attempts in trial run-3 completed successfully then were silently
discarded, stranding all 6 workflows in ANALYZING.
FIX#1 — current_tier never written from estimator's recommended_tier
File: tools/controller/master/tick.py
The ANALYZING→IMPLEMENTING UPDATE wrote only current_state /
last_transition_at / entered_state_at. recommended_tier from the
estimator payload was never extracted, so every PR ran at the
workflow's creation-time tier (typically 0) regardless of what the
estimator recommended — the entire tier-escalation ladder was
informational-only. Fix: per-event ``extra_set`` clauses; on
``estimator_done`` / ``estimator_metadata_only`` events, set
``current_tier = :rec_tier`` from the payload (with 0..2 validation).
Tests: TestEstimatorRecommendedTierWritten (3 cases).
FIX#2 — approved_at_sha never passed to merge callback
File: tools/controller/master/merging.py, forgejo_http.py
ReviewerOutputV1.approved_at_sha is the exact SHA the reviewer
signed off on. Pre-fix the MergeCallback signature was
``(owner, repo, pr_number)`` — Forgejo merged whatever HEAD currently
was. Race condition: a concurrent push (operator or another driver)
between approval and merge would silently merge unapproved code.
Fix: extended signature to ``(owner, repo, pr_number, approved_at_sha)``;
SQL SELECT now pulls the latest reviewer attempt's output_payload as
a subquery; merge_pr forwards it to Forgejo as ``head_commit_id``
(Forgejo refuses with 409 if HEAD has advanced). Defensive: still
merges when approved_at_sha is None but logs a WARNING. Tests:
TestApprovedAtShaPassedToMerge (2 cases).
FIX#3 — tier_last_succeeded column had ZERO writers
File: tools/controller/master/tick.py
The schema column existed; the merging.py 409-conflict path read it
to recover the last-known-good tier; but NOTHING ever wrote to it.
Every workflow's tier_last_succeeded was permanently NULL → the
409-recovery path transitioned to IMPLEMENTING(tier=NULL) → scheduler
silently coerced to tier 0. Fix: on ``implementer_pushed`` event,
``UPDATE workflows SET tier_last_succeeded = current_tier``. Tests:
TestTierLastSucceededWritten.
FIX#4 — outcome column NULL for estimator/reviewer/summarizer
File: tools/controller/worker/runner.py
``workflow_attempts.outcome`` is the operator-facing audit column.
Pre-fix the runner extracted ``output_payload.get("outcome")``
blindly — works for implementer/conflict_resolver but those three
roles have no ``outcome`` field. Result: ``SELECT … WHERE outcome IS
NOT NULL`` audit queries silently missed every estimator/reviewer/
summarizer attempt. Fix: new ``_derive_outcome_for_audit(role, payload)``
helper synthesizes meaningful per-role values:
- implementer/conflict_resolver: payload['outcome'] (unchanged)
- reviewer: payload['verdict']
- estimator: 'metadata-only' OR f'tier-{recommended_tier}'
- summarizer: 'summarized'
Tests: TestOutcomeAuditColumn (parametrized 8 cases).
FIX#5 — conflict_resolver new_head_sha never preferred
File: tools/controller/worker/runner.py
ConflictResolverOutputV1.new_head_sha is "Required iff outcome='resolved'"
(the canonical post-rebase branch tip). Pre-fix runner.py used
``commit_shas[-1]`` for head_sha_after — works for normal git rebase
--continue but wrong for resolvers that did force-pushed merge commits
where the last commit SHA ≠ the branch tip. CI status poll would then
poll the wrong SHA. Fix: when role=='conflict_resolver', prefer
``new_head_sha`` over commits[-1]. Tests:
TestConflictResolverNewHeadShaUsed (2 cases).
ALSO updated existing tests that papered over the original bug:
- test_master_outcomes.py: estimator tests used to inject a fake
``"outcome": "(implicit)"`` field; now use real V1 shape (no outcome).
Reviewer tests now use ``verdict`` (the real V1 field) not ``outcome``.
- test_master_tick.py reviewer tests: same `verdict` switch.
- test_master_merging.py: updated all 13 ``lambda o, r, n: ...``
merge-callback stubs to the new 4-arg signature.
CONFIRMED-CLEAN (no fix needed) by the same code review:
- outcomes.py post-fix-#0
- prefetch.py field reads
- prompts.py field accesses
- ci_status_poll.py role+outcome filter
The above were verified to handle all 5 V1 contract shapes correctly.
Total: 802 → 819 controller tests, 0 regressions.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
drew
3e23853ffa