cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.0k Pull Requests 115 Actions Packages Projects Releases Wiki Activity
2,521 Commits 1,418 Branches 10 Tags
d4e1219577e01f270aca90e2706cfdcd523100cd
Commit Graph

73 Commits

Author SHA1 Message Date
drew d4e1219577 fix(controller): scope conflict-marker check to resolved files 
finalize_conflict_resolution grepped every file in the PR's
origin/<base>..HEAD diff for committed conflict markers. On a large
sentinel PR (478-file diff) that includes .opencode/agents/
git-rebase-util.md — an agent def that *documents* conflict markers
with literal "<<<<<<< HEAD" lines — so the check false-positived and
rejected every resolution, looping the conflict-resolver indefinitely
(run-9: 185 attempts in ~4h, 125 on this exact error).

Scope the grep to the files the resolution actually touched
(resolved_files = prep-time conflicted set + the agent's reported
files_modified). A leftover marker can only be in a file the agent
resolved; a clean rebase/merge resolves nothing so the check is
skipped. Adds a regression test with a marker-documenting file in the
PR diff that is not a resolved file.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-21 21:33:44 -04:00
drew 615a05b982 feat(controller): rebase-default conflict resolution with merge fallback 
PR branches 177-180 commits ahead of base cannot be rebased
commit-by-commit by a single-shot resolver agent (too many conflict
stops for one session). Conflict-prep now defaults to rebase (linear
history) and falls back to a single 3-way merge when the branch is too
divergent (commit count over CONTROLLER_CONFLICT_REBASE_MAX_COMMITS,
default 60). The merge pipeline derives the track from branch shape via
a Do:rebase -> Do:merge ladder in _make_merge_pr — no stored flag.

Adds a git_rebase_continue MCP tool plus status rebase/merge-in-progress
fields so the conflict-resolver agent is fully MCP-driven and dual-mode
(mid-rebase or mid-merge). Also routes a green-CI implementer noop
straight to REVIEWING instead of a deadlock-prone AWAITING_CI round
trip. No state-machine change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-21 17:17:16 -04:00
drew 8c9a0c50bb fix(controller): green-CI noop routes forward instead of ABANDONED 
An implementer dispatched against a fully-green CI that emits `noop`
("nothing to fix") was mapped to competence-failure -> ESCALATING ->
ABANDONED at MAX_TIER. PR-39 and PR-40 dead-ended exactly this way:
12/12 CI gates green, workflow ABANDONED, solely because the
implementer said `noop` instead of the synonymous `verified-clean`.

The tick now reads the attempt's input_payload ci_summary; a `noop`
whose attempt saw an unambiguously green CI (overall success, zero
failed, zero pending, >=1 passed) routes via `implementer_verified`
-> AWAITING_CI (-> ci_green -> REVIEWING) -- the same forward path
`verified-clean` already takes. A non-green `noop` (red / pending /
unknown / no CI) keeps the competence-failure -> escalate behavior.

- outcomes.py: `attempt_saw_green_ci` param gates the noop branch.
- tick.py: `_input_ci_summary` + `_ci_summary_is_green` helpers.
- tests: green->AWAITING_CI, red/none->ESCALATING, helper unit tests.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-21 02:44:57 -04:00
drew 14e592ddd5 feat(controller): zombie-CI detection — stop waiting on dead CI runs 
A CI gate stuck `pending` is ambiguous: the job may genuinely be
running, or the run may be dead (a crashed runner, an Actions job whose
terminal commit-status was never posted — `CI / status-check` zombies
routinely here). The "wait for the whole run to finish" fix then waited
forever on the dead case (PR #36: a `status-check` gate pending for 8 h
while the run had actually finished RED 8 h earlier).

New `ci_run_status.classify_ci_run` resolves a still-pending run to
`complete` / `running` / `stale` via two checks, authoritative-first:

  1. ACTIVE-RUN — `get_action_tasks` asks Forgejo's Actions API
     directly whether a task for the commit is still running; catches a
     dead run immediately, regardless of age.
  2. AGE — if no gate has updated in > CONTROLLER_CI_STALE_AFTER_MIN
     (default 90) the run has stopped; the fallback when the Actions
     API is unavailable.

A `stale` run is no longer waited on: the verdict is taken from the
gates that DID finish (`terminal_verdict`) — any failure → red, all
pass → green, fully-dead → red. Applied in both `ci_status_poll` (the
AWAITING_CI verdict) and `ci_summarize` (the implementer's summary —
zombie pending gates drop out of `gates_pending`/`overall_state`) so
the two agree and never ping-pong.

New `get_action_tasks` Forgejo callback wired through forgejo_http →
__main__ → the poll and the prefetch path.

23 new tests; full controller suite (1222) green.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-21 02:21:12 -04:00
drew 2fef8cb9f9 fix(controller): strip Actions log timestamps before CI parsing 
Every implementer was handed `findings=0` for its red gates even when
the parser's own summary line counted real failures ("Pyright: 10
errors", "Behave: 2 scenarios failed"). Root cause: GitHub/Forgejo
Actions prefixes EVERY log line with an ISO-8601 timestamp
("2026-05-20T18:04:41.1555784Z "). Every parser extracts findings with
^-anchored regexes against the raw tool output (pyright
`^file:line:col - error:`, behave `^\s+...feature:N  Scenario:`); the
timestamp prefix pushes that content off the line start so the anchors
never match — while the UN-anchored summary-count regex still matches,
producing the misleading "summary says N, findings=[]" state.

Fix: new `strip_log_timestamps()` in ci_summary_parsers/_base.py,
applied once centrally in `ci_summarize._safe_fetch` — the single
chokepoint feeding every parser and the no-parser raw-log excerpt.
Lines without a timestamp prefix are left untouched, so it is safe
unconditionally; it also trims ~28 chars/line off the implementer's
raw-log excerpt.

5 new tests; full controller suite (1200) green.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-21 01:13:20 -04:00
drew 5f0fff0db2 fix(controller): wait for the whole CI run to finish before acting on it 
Two places acted on a CI run that was still executing — the PR-39/PR-40
"doesn't wait for CI to finish" incident:

1. ci_status_poll — keyed the AWAITING_CI verdict purely off Forgejo's
   combined `state`. That combined state flips to `failure` the instant
   ONE gate fails, even with other gates still running (and can read
   `success` before a late gate reports). So a run at 9-passed /
   1-failed / 2-pending fired `ci_red` and yanked the workflow out of
   AWAITING_CI mid-run. New `_ci_run_incomplete()` scans the per-gate
   `statuses`; while any gate is pending/running the poll waits,
   regardless of the combined state. Only a fully-terminal run yields a
   verdict.

2. implementer prompt routing — `_ci_summary_is_pending` required the
   run to be failure-free to count as "still pending," so a run with an
   early failure + gates still executing fell through to the
   ci-infra-failure block and the implementer acted on a partial
   result. Dropped the no-failures clause: any pending gate → the
   ci-not-ready (wait) block, which now also covers partial results.

Zombie/stale runs (a gate pending for hours) are left to the
controller's CI-freshness gate to re-trigger — a separate follow-up;
ci_poll_exhaustion remains the backstop.

4 new tests; full controller suite (1195) green.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-21 00:55:09 -04:00
drew eb1d81828f feat(controller): implementer_retrigger_ci — agent re-triggers CI on ci-infra-failure 
A CI job hard-killed by OOM / pod eviction produces no verdict — nothing
in the diff to fix. The implementer correctly emits
outcome=ci-infra-failure, but that alone only shuffles workflow state;
CI never re-runs, so every retry reads the same dead run and the
workflow loops until the _MAX_CI_INFRA_FAILURE backstop STUCKs it
(observed live on PR #39 in run-3).

New ``implementer_retrigger_ci(owner, repo, pr_branch)`` MCP tool on the
implementer response-builder lets the agent kick a fresh CI run. Forgejo
15.x has no Actions rerun API, so it reuses the controller's existing
mechanism — ``ci_rerun.trigger_ci_rerun_via_empty_commit`` — an empty
commit that advances the PR head SHA, which is the unambiguous
fresh-state signal for CI (and a stale review).

- ci_rerun.py is loaded standalone (importlib by file path, registered
  in sys.modules before exec so its @dataclass resolves) — the
  response-builder MCP must not pull the heavy tools.controller.master
  package.
- _retrigger_ci resolves the Forgejo base URL + token from env
  (FORGEJO_URL / FORGEJO_API_BASE, FORGEJO_TOKEN / GITEA_TOKEN) and
  never raises.
- Once per session: a second implementer_retrigger_ci call is refused
  benignly so a looping agent cannot pile junk commits on the PR.
- The implementer prompt's ci-infra-failure block now instructs the
  agent to call the tool before emitting the outcome.

10 new tests (test_mcp_builders.py, test_worker_prompts.py); full
controller suite (1192) green.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-21 00:22:46 -04:00
drew 9d20f865d3 feat(controller): pre-review mergeable gate — skip doomed reviews 
When CI goes green, route to REVIEWING only if the PR still merges
cleanly into base. If a cheap Forgejo mergeable check shows the base
advanced while CI ran, route AWAITING_CI → CONFLICT_RESOLVING instead
— handing the conflict to the LLM conflict_resolver BEFORE the
expensive reviewer pass, since code that must be rebased gets re-CI'd
and re-reviewed afterwards anyway.

- new event `pre_review_base_conflict` + transition
  (AWAITING_CI → CONFLICT_RESOLVING)
- ci_status_poll: `_decide_green_event` gate behind a new optional
  `get_pr_details` callback; conservative — only an explicit
  mergeable=false diverts, an unknown/uncomputed bit falls through to
  ci_green so a fresh PR is never false-routed
- gate-fired event rows carry `mergeable` in the payload so the
  false-positive rate is observable from controller_events
- wired through loop.py (5th ci_status_poll_args element) + __main__

One API call, no LLM — cheap+frequent detection gating the
expensive+rare conflict_resolver/CI/reviewer stages.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-21 00:13:03 -04:00
drew 8fe98cb5c0 feat(controller): salvage timed-out implementer + cap unbounded prefetch-retry loop 
Two reliability fixes from the proactive audit of untouched pipeline
modules (AUDIT-1, AUDIT-2 in .drew/PENDING_FIXES.md).

AUDIT-1 — a timed-out implementer's committed work was never salvaged.
opencode_session maps an OpenCode timeout / transport-error to
WorkerError, which propagated out of production_agent_runner BEFORE the
canonical-output wait + _salvage_implementer_commits ran. A timeout is
the case most likely to have a complete committed fix (agent ran out of
wallclock, not correctness). The session-exception handler now runs the
same salvage the canonical-missing path uses before discarding the
attempt as worker-internal-error.

AUDIT-2 — a persistently failing prefetch() retried every master tick
forever (the T4-4 guard covered only the issue-kind sub-case; same
unbounded-retry class as the run-2 174x estimator loop). The scheduler
now journals each prefetch failure as a controller_events row, counts
failures since the workflow's last transition, and routes the workflow
to STUCK after _PREFETCH_FAILURE_LIMIT (10) failures. The STUCK
transition is rowcount-guarded so the journal never records a
transition the current_state guard rejected.

7 new tests; full controller suite (1169) green.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-20 23:28:02 -04:00
drew 58307bbdab refactor(ci-logs): retire the dead legacy {sha}.json cache layer 
Follow-up to the B1 unification: with `fetch_pr_failure_logs` now a
view over the `get_ci_logs` bundle, the legacy `{sha}.json` cache had
no readers left. Remove it wholesale rather than leave it orphaned.

- `_ci_logs.py`: delete `_cache_covers_all_current_failures`,
  `_record_failure`, `_read_cache`, `_write_cache`, `cache_path` — all
  zero-caller after B1. `invalidate` re-pointed onto the bundle cache
  (`bundle_cache_path`) so it stays a working API. Module docstring
  rewritten to describe the bundle-as-single-store reality.
- `local_ci.py`: `_write_ci_logs_cache` no longer writes the legacy
  `{sha}.json` — `put_local_bundle` already populates the bundle that
  `fetch_pr_failure_logs` projects, so the MCP tool still sees local
  CI logs. One write path, not two.
- Tests re-pointed onto `bundle_cache_path`; `ruff format` applied.

No behavior change — only dead code removed and the docstring
brought current.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-20 23:02:07 -04:00
drew f76568a871 feat(controller): ci-infra-failure implementer outcome → bounded rerun 
The implementer-side counterpart to the `indeterminate` verdict. When
the implementer IS dispatched onto a CI failure and finds the log
carries no verdict (a hard-kill / OOM — nothing in the diff to fix),
it can now emit `outcome=ci-infra-failure` instead of being forced to
`blocked` → STUCK.

`ci-infra-failure` forbids commits/files/blockers (no-work invariant,
like `noop`) and fires `implementer_ci_infra_failure`, routing
IMPLEMENTING → DISCOVERED so the CI-freshness gate reruns CI under its
bounded RERUN_BUDGET. Backstopped by `_MAX_CI_INFRA_FAILURE=4` so a
mis-classification cannot loop the gate forever.

Wired through: the V1 contract enum, the implementer MCP builder
(outcome value + no-work invariant), the state machine (event +
IMPLEMENTING→DISCOVERED transition), the outcome mapper (+ per-workflow
cap), tick's `_count_prior_ci_infra_failure`, and the implementer
prompt — which now surfaces the outcome whenever the CI summary shows
a failing gate, with guidance to use it ONLY when the log genuinely
shows no verdict.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-20 22:55:28 -04:00
drew 6ab6df319c feat(controller): indeterminate-CI verdict for no-verdict (OOM) failures 
Diagnosed live on PR 39 (STUCK 2026-05-20): both CI gates ran 17
minutes, the captured log was <3 minutes and ended mid-execution
(`still running` / a just-launched runner) with ZERO error markers.
A hard process kill (OOM-killer / pod eviction) cannot flush a
buffer, print a traceback or emit an exit code — it leaves a *hole*,
not a phrase. The classifier called this `fresh_real`, the implementer
was sent to "fix" a failure with nothing to fix, and the PR
dead-ended at blocked → STUCK.

New `indeterminate` verdict: a failing gate whose FULL log carries no
terminal verdict marker (`##[error]`, test summary, Traceback, exit
code) AND ends mid-execution. Both conditions required — "no marker"
alone over-fires on a real failure whose tool output isn't in the
marker set (e.g. `ruff format`). Routed to a bounded rerun, same as
`infra_broken`/`stale`, via `ci_status_poll` and `ci_gate`.

Also fixes an infra-signature regression exposed by the full-log
change: the bare step-name signatures (`git fetch`, `Set up job`,
`actions/checkout`) matched the checkout/setup preamble of EVERY job
log — against a full untruncated log they turned every failing run
into `infra_broken`. Replaced with genuine error-text signatures
(`could not read from remote`, `download action repository failed`,
`unable to access`, `failed to connect`).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-20 22:55:19 -04:00
drew c4a11901f5 feat(ci): unified CI-log cache — phase 2 (consumer migration + RUN_CI_LOCAL) 
Builds on 27289ea4b. Routes the remaining CI-log consumers through the
get_ci_logs bundle and closes an adversarial-review bug.

- forgejo_http._make_get_failure_logs (the freshness gate +
  ci_status_poll's get_failure_logs callback) now derives failing-job
  logs from get_ci_logs — full logs, not the old 4000-char tail the
  infra-vs-real classifier could miss a signature past.
- prefetch._build_ci_summary: skip the get_ci_logs fetch entirely when
  CI is fully green (no failing gate) — avoids a wasted full all-jobs
  session-fetch on every green PR.
- local_ci: RUN_CI_LOCAL now seeds the unified bundle too
  (_ci_logs.put_local_bundle, source="local") via _per_job_full_logs —
  so the implementer/reviewer ci_summary reads local-CI logs through
  the same get_ci_logs entry point as Forgejo CI.
- collect_all_jobs: order failing jobs first before the max_jobs cap,
  and raise the default cap 10 -> 20. Without this a 12-gate run
  (e.g. PR 39) could drop a failing gate from the bundle.
- 4 new tests; full controller suite green (1173 passed).

Still deferred: (head_sha, run_id, attempt) multi-attempt keying —
needs live Forgejo verification of the attempt API/URL shape.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-20 21:06:08 -04:00
drew 27289ea4b7 feat(ci): unified CI-log cache — get_ci_logs + implementer path (phase 1) 
The implementer's ci_summary carried an empty raw_log_excerpt for every
failed gate (diagnosed via PR 39): prefetch._build_ci_summary passed a
no-op log fetcher, and ci_summarize._gate_to_nox_session never stripped
Forgejo's "(pull_request)" event suffix, so every PR gate fell through
to NoParserAvailable with no log fetched at all.

Phase 1 — the implementer-facing path:

- _ci_logs.get_ci_logs(): unified entry point — every job of a run,
  full untruncated logs, one cache. `partial` marks an in-flight run;
  a terminal + clean bundle is frozen forever. Reuses the existing
  session-cookie login + exponential backoff. Additive —
  fetch_pr_failure_logs and its 9 consumers are untouched.
- ci_summarize: strip the "(pull_request)" event suffix so gates
  resolve to their nox parser; _no_parser_failure now carries the raw
  log instead of hardcoding "".
- prefetch._build_ci_summary + forgejo_http + __main__: wire a real
  get_ci_logs-backed log_fetcher so each failed gate's raw_log_excerpt
  is filled from the cache.
- 9 new tests; full controller suite green (1169 passed).

Deferred to later phases: re-point the freshness gate / ci_status_poll
onto get_ci_logs, RUN_CI_LOCAL into the same cache, and
(head_sha, run_id, attempt) multi-attempt keying.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-20 20:56:52 -04:00
drew 521117882e fix(controller): conflict resolver lands resolutions on the PR 
Run-2 PR #40 exposed that the conflict-resolver stage never resolved
conflicts *on the PR*. The resolver agent owned the whole git flow
(rebase + resolve + push) but was never given the PR head branch name,
so it pushed the resolved branch to a guessed branch ("main"), the push
was lost, and it reported "resolved" anyway — the controller advanced
the workflow on a resolution that never reached the PR.

This splits the work along the controller's principle: deterministic
git mechanics in the worker, only the semantic merge in the agent.

- conflict_rebase.py (new): the worker starts the rebase
  (prepare_conflict_rebase — hands the agent a real mid-rebase
  worktree) and, after the agent's resolution, lands it with a
  *verified* push (finalize_conflict_resolution) — force-with-lease
  pinned to the PR head the resolver started from, then re-reads the
  remote to confirm it moved. A failed/rejected push is never reported
  as resolved. run_conflict_resolver_attempt orchestrates it.
- ConflictResolverInputV1 / prefetch: carry head_ref so the worker
  knows the PR branch (this also makes the worker's fetch_and_validate
  run for the conflict_resolver — it gated on head_sha AND head_ref).
- worker/__main__.py: route conflict_resolver attempts through the new
  orchestration.
- conflict-resolver-worker.md: the agent resolves + continues the
  rebase and never pushes — the controller lands it.
- mcp_git_server.py: add the controller worktree root to the git MCP's
  allowed bases (every git_* MCP call errored for this role before).

The committed-marker check is scoped to the PR's own diff — a
whole-tree grep false-positived on repo files that legitimately
document conflict markers. The worker's rebase/push are lost-lock
guarded so a reaped worker cannot race the shared worktree.

1136 controller tests pass (+11 new in test_conflict_rebase.py).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-20 18:27:19 -04:00
drew 3106230ef8 fix(controller): cap estimator worker-internal-error retries 
An estimator whose session ends without emitting canonical output fails
with worker-internal-error. Unlike the implementer, the estimator has no
escalation path (it runs pre-tier) and no salvage (it produces no git
artifact) — so a flaky estimator session just re-enqueues, with nothing
to stop it. Run-2 burned 174 consecutive estimator worker-internal-error
attempts on one PR.

After _ESTIMATOR_WORKER_ERROR_LIMIT (3) such failures the workflow now
STUCKs for operator attention via estimator_failed_twice — symmetric
with the implementer worker-error escalation cap.

- outcomes.py: the cap + the prior_estimator_worker_errors param.
- tick.py: _count_prior_estimator_worker_errors (per-workflow count;
  the estimator runs pre-tier, so tier is not a meaningful axis).
- state_machine.py: estimator_failed_twice description corrected — the
  event had no emitter before this; strict-parse failures route via
  contract-violation -> pickup_exhausted, not this event.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-20 17:42:41 -04:00
drew a6ff7da63c fix(controller): recover work lost to plumbing + run-25 reliability fixes 
Run-25 surfaced five ways the controller lost or mis-routed correct
implementer work. Each is fixed:

1. Canonical-output salvage (agent_runner.py): when an implementer's
   OpenCode session ends cleanly but emits no V1 output, its commits
   are no longer discarded — they are pushed and a synthetic
   outcome=resolved routes the workflow to AWAITING_CI so CI judges the
   code. Guarded by lost_lock_check so a reaped worker cannot race a
   fresh one on the shared worktree.

2. Preserve committed-but-unpushed work (workspace.py): the pre-reset
   snapshot (auto-scratch/pr-<N>) now captures committed commits ahead
   of the reset target, not just dirty edits — a finished-but-unpushed
   fix survives for the next attempt to adopt.

3. blocked escalates (outcomes.py): an implementer 'blocked' below
   MAX_TIER now escalates to a stronger tier instead of dead-ending at
   STUCK; only 'blocked' AT MAX_TIER STUCKs.

4. Diff-size tier floor (tick.py): the estimator's structured
   recommended_tier can contradict its own reasoning (a 387-file PR
   emitted as tier 0). current_tier is now floored deterministically by
   diff size so a huge PR cannot run on the weakest model.

5. CI log excerpt (prompts.py): the per-gate log excerpt is now
   tail-clipped, not head-clipped — the failure tracebacks live at the
   bottom and were being cut off, leaving the implementer blind.

1121 controller tests pass (+34 new).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-20 15:18:17 -04:00
drew 0db0a15dad feat(controller): RUN_CI_LOCAL verdict source, ci-not-ready outcome, escalation hardening 
Adds RUN_CI_LOCAL — an on-demand local-CI verdict source for when the
cluster's Forgejo CI is broken — plus robustness fixes, the telemetry
Live-tab rewrite, and PR-level cost attribution.

Controller:
- RUN_CI_LOCAL: the master swaps its Forgejo CI callbacks for local
  `forgejo-runner exec` runs (tools/run-ci-full-local.sh + local_ci.py).
  Async per-(owner,repo,SHA) on-disk job cache; preflights the
  forgejo-runner binary + Docker daemon at startup (fail loud, not a
  red verdict on every PR); GCs finished run dirs + per-run actcache.
- ci-not-ready implementer outcome + implementer_ci_not_ready event:
  an implementer that runs before the on-demand verdict exists parks
  in AWAITING_CI instead of dead-ending at STUCK; capped against
  ci_red ping-pong.
- ci_poll_exhaustion skips its sweep while a local CI run is in
  flight, so AWAITING_CI workflows queued behind on-demand CI are not
  STUCK'd by the remote-CI-sized timeout.
- Escalate the workflow after repeated worker-internal-error at a
  tier instead of retrying to pickup-exhaustion -> STUCK.
- forgejo_http: normalise Forgejo's per-gate `status` key to `state`
  so failing gates are actually counted (they previously all read as
  pending).
- Per-tier worker timeouts bumped +15 min; a timed-out attempt's
  dirty-worktree residue is preserved on auto-scratch/pr-<N> before
  the next attempt's reset.
- Implementer agents now verify only the CI-flagged gate(s) via a
  targeted re-run rather than the full local battery before claiming
  resolved/noop. Re-running the whole suite CI will run anyway was the
  #1 cause of implementer timeouts; CI remains the real gate and
  re-dispatches the implementer on red.

Telemetry:
- Live tab rebuilt on /api/live (controller DB run state + the live
  OpenCode session forest) after the live_log_writer sidecar was
  retired with the legacy dispatchers.
- Durable per-attempt input/output payloads surfaced in the Live
  drill-down, archived-session detail, and Workflows timeline.
- PR-level cost attribution: worker session tags carry -pr-<n>;
  backfill_llm_activity_pr.py repairs rows written before the fix.

Shared:
- tools/controller/session_tag.py — one canonical controller-tag
  parser shared by the telemetry server and the backfill.

Tests: new coverage for local_ci (state machine, log parsing,
_summarize_run, GC, in-flight probe, preflight), the ci-not-ready
path, the escalation/ci-not-ready SQL counters, the ci_poll
in-flight skip, and CI-status payload parsing across both sources.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-20 11:49:05 -04:00
drew 0bc734c020 style: ruff format the controller-state-machine branch (288 files) 
Applies `ruff format` to the accumulated formatting debt on this branch.
Formatting-only — no behavioral changes. Required for CI/lint's format
gate (`nox -s format -- --check`), which the branch was failing on 288
tracked files that drifted from ruff's canonical style.

In-progress WIP files are intentionally excluded so this commit stays a
clean formatting-only diff.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-20 00:09:17 -04:00
drew a2fcecb28d feat(controller): CI-freshness gate — re-trigger stale/infra CI instead of dead-ending 
A discovered PR with stale CI (every job failed at the checkout step —
a git-fetch connection reset, pure infra, no code ran) burned an
estimator + a tier-2 implementer and dead-ended at STUCK. The pipeline
had no notion of CI freshness and never triggered CI — only polled.

New early master tick (ci_gate) runs before DISCOVERED->ANALYZING
promotion. For each DISCOVERED pr-kind workflow it classifies the CI
via ci_freshness.classify_ci_result:

  - infra_broken — failed; the failing jobs' LOG content carries a
    checkout/setup signature (curl 56, expected 'packfile', ...).
    Logs are fetched via _ci_logs (session-cookie auth).
  - stale — failed; newest status older than CONTROLLER_CI_MAX_AGE_S
    (default 6h). Timestamp-based, log-independent — catches an old
    failure even when Forgejo has purged its logs.
  - no_ci / pending / fresh_real — handled accordingly.

infra_broken/stale/no_ci -> push an empty commit to the PR branch
(Forgejo 15.0.2 has no rerun API), routing DISCOVERED -> AWAITING_CI
(new event discovery_ci_rerun_triggered). The existing AWAITING_CI
poller then gets a real verdict. A reran CI that is ALSO infra/stale
routes AWAITING_CI -> DISCOVERED (new event ci_infra_recheck) so the
gate re-handles it; bounded by a rerun budget of 3, then STUCK.

Also wires the existing CI summarizer into prefetch so workers stop
receiving ci_summary=null.

1095 controller tests pass.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-19 20:19:55 -04:00
drew f454670a0e chore(controller): launch merge_drive from the pipeline script; rename worker-concurrency env var 
The launch script now starts merge_drive by default (T5-7: it is the
controller's singleton merge stage — APPROVED -> MERGING -> MERGED).
Previously the script killed merge_drive as a "legacy" process but
never started it, so controller workflows dead-ended at APPROVED.
merge_drive moves into the CONTROLLER process group; --no-merge skips
it. Also adds the worker thread-pool concurrency knob (default 2 for
the trial harness).

Renames the misleadingly-named env var
CONTROLLER_MAX_CONCURRENT_WORKERS_PER_MACHINE ->
CONTROLLER_MAX_CONCURRENT_WORKER_THREADS_PER_MACHINE: it sizes a
ThreadPoolExecutor inside one worker process, it does not spawn
worker processes.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-19 19:18:23 -04:00
drew 17cd91d275 fix(controller): post-commit review — close 3 merge-bridge stranding bugs 
The trial-5 merge-pipeline split could strand workflows in MERGING
forever on its main paths. The controller has no MERGING handler
(merge_drive owns it), so any terminal_state that maps to no event
leaves the workflow orphaned. Three such holes, all caught by the
post-commit multi-perspective review:

1. merge_train emits "merge-error-{403,5xx,...}" for any non-2xx/409
   Forgejo merge POST — none were mapped. Added _resolve_bridge_event:
   403 -> branch_protection_blocked, all else -> retry_exhausted (STUCK).

2. run_one_cycle applied one shared outcome.terminal_state to every
   claimed PR. A bisected train returns "bisected" (unmapped) so all
   PRs stranded; a mixed train could tell a merged PR merge_base_conflict.
   CycleOutcome now carries pr_terminal_states and events emit per-PR.

3. Graceful shutdown mid-merge ("stopped") was unmapped. Added the
   merge_interrupted event (MERGING -> APPROVED) so the workflow
   returns to the handoff state for clean re-pickup.

Also fixes a stale comment in _controller_db_bridge.py (merge_base_conflict
routes to CONFLICT_RESOLVING, not IMPLEMENTING) and adds a drift-guard
test cross-checking the bridge's transition map against the canonical
state machine — that drift is what produced the stale comment.

3304 tests pass.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-19 18:05:17 -04:00
drew a6986008ee feat(controller): trial-5 batch — dispute path, merge pipeline split, conflict-resolver hardening 
T5-1   reviewer feedback rendered full-body to the implementer
T5-4/9 implementer dispute path — dispute-at-any-tier with per-tier cap,
       OPERATOR_ATTENTION state on stalemate, pr-review-worker-dispute agent
T5-5   reviewer BLOCKING ISSUE EVIDENCE RULE + 5-step validation
T5-7   merge step split into a singleton process — impl/review masters write
       APPROVED and stop; merge_drive owns APPROVED -> MERGING -> MERGED
T5-10  merge process is fully deterministic; base conflicts bounce to the
       controller's CONFLICT_RESOLVING (LLM); conflict_drive sidecar retired
T5-11  implementer fast success path — verified-clean outcome so a no-op
       after conflict resolution doesn't force busywork
T5-12  conflict-resolver permissions fixed across all paths (/tmp/** glob)
T5-13  conflict-resolver PR-intent prehydration (title/body/comments)

Adds tools/_controller_db_bridge.py so merge_drive reads the controller DB
directly (Option B), plus APPROVED + OPERATOR_ATTENTION states, the
dispute/verified-clean events, and the V1 contract fields backing them.
Reviewer model: baseline -> sonnet, dispute -> opus.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-19 17:54:07 -04:00
drew ab8a5e5bfb fix(controller): T4-4 — route issue workflows to STUCK when role has no prefetch 
Trial-4 observed wf=6 (entity #34, kind='issue') promoted ANALYZING→
IMPLEMENTING by the estimator, but build_implementer_input raises
``ValueError("implementer prefetch needs kind='pr', got 'issue'")``.
The scheduler logged WARNING and retried every ~5s forever — log spam
+ workflow never reached a terminal state. Pre-fix:

  2026-05-18 21:13:43 WARNING ... prefetch failed ...
  2026-05-18 21:13:50 WARNING ... prefetch failed ...
  2026-05-18 21:13:55 WARNING ... prefetch failed ...
  (50+ identical lines over the trial)

Fix: before calling prefetch, scheduler checks the workflow's kind
column. If kind='issue' and role in
{implementer, reviewer, conflict_resolver}, the workflow is routed
to STUCK with reason='issue-not-supported (T4-4)'. Operator-visible
controller_events row tags this as 'issue-not-supported'.

Future work: IssueImplementerOutputV1 contract already exists in
contracts/v1.py:362; a follow-up batch can add ``build_issue_
implementer_input`` + remove this short-circuit. For the trial we
just need issues to stop wedging.

Test: TestIssueWorkflowRoutedToStuck in test_batch_s_fixes.py.

833 controller tests pass (was 832; +1).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 21:20:17 -04:00
drew 46764b841b fix(controller): batch S — 11 fixes from trial-4 finding + 2nd code-review pass 
Trial-4 ran the controller past ANALYZING for the first time. One
critical regression surfaced live (T4-1 — scheduler dup-attempt
enqueue), and a parallel agent-driven code review found 8 more bugs
across "races + error paths + agent quality" classes. Batch S
addresses 11 of those.

LIVE-OBSERVED REGRESSION (trial-4 2026-05-19 00:49):

T4-1 — Scheduler enqueues duplicate estimator after workflow advanced
File: tools/controller/master/scheduler.py
The "already pending" check filtered on
``status IN ('pending', 'in_progress')`` — missing the brief window
where the prior attempt is ``status='complete'`` but tick hasn't yet
processed its outcome. Result: scheduler enqueues a 2nd estimator/etc.;
when its outcome fires from a now-advanced state, IllegalTransition
→ STUCK. Hit wf=1 in trial-4. Fix: also skip when an unprocessed
``complete`` attempt exists (finished_at > w.last_transition_at).

E-5 — IllegalTransition over-aggressive STUCKing
File: tools/controller/master/tick.py
Companion fix to T4-1. Even if T4-1 escapes in some other path (or
a worker delays writing outcome past tick), a stale-outcome
(workflow already advanced via parallel path like ci_status_poll or
reconciliation) shouldn't STUCK. New ``_is_stale_role_outcome``
helper recognizes "this role's outcome arrived after the workflow
moved past its origin state" → consume the attempt, bump
last_transition_at, continue. Only genuine state corruption → STUCK.

E-3 — Corrupted output_payload silently wedges workflow
File: tools/controller/master/tick.py
Pre-fix _decode_output_payload swallowed json.JSONDecodeError →
mapper returned None → tick bumped last_transition_at but workflow
never moved. Operator had no signal. Now raises
``CorruptedOutputPayload`` → tick routes to STUCK with reason.

E-1 — Contract-violation routed to STUCK on first attempt
File: tools/controller/master/outcomes.py + tick.py
v9 spec promised retry-once-with-corrective-prompt for
contract-violation; the column ``strict_parse_retries`` existed but
nothing read/incremented it. _map_failed_outcome now takes
``prior_contract_violations`` count (queried in tick.py); STUCKs
only when count ≥ _CONTRACT_VIOLATION_RETRY_LIMIT (2). First two
violations re-enqueue.

R-1 — Reconciliation flipped workflow state mid-attempt
File: tools/controller/master/reconciliation.py
Worker holding a lock + heartbeating; reconciliation flipped current_state
to MERGED/ABANDONED based on Forgejo; tick.py then skipped the
worker's eventual write (terminal-state exclusion). Worker's output
lost. Fix: _apply_transition first checks for in_progress attempts
on the same workflow and defers if any exist.

R-2 + R-8 — _apply_transition lacks current_state guard
Files: reconciliation.py + merging.py
Same pattern as ci_status_poll's existing TOCTOU defense. UPDATE now
filters ``WHERE current_state = :from_state``; on rowcount=0, skip
the event row. Prevents racing ticks from over-writing each other.

R-4 — Reaper UPDATE didn't re-check heartbeat freshness
File: tools/controller/reaper.py
A worker's healthy heartbeat between reaper's SELECT and UPDATE
would be silently overwritten; the worker's later _write_outcome
(filtered on locked_by_instance) returned rowcount=0 → output lost.
UPDATE now includes the same freshness filter as the SELECT, so
fresh heartbeats protect the row.

E-8 — merging_retry_count not reset on STUCK/abandoned paths
File: tools/controller/master/merging.py
Pre-fix, only 200/409/422 paths reset the counter. 403 (branch
protection), 404 (externally closed → ABANDONED), retry-exhausted
(STUCK) leaked stale counts. If operator unsticks a STUCK workflow
back through MERGING, the stale count made it STUCK again sooner
than expected. All terminal-state-changing paths now reset.

A-1 — commit_shas validation accepted any ≥7-char string
File: tools/controller/mcp/implementer_builder.py
Tightened to ``re.fullmatch(r"[0-9a-f]{7,40}")``. Pre-fix an agent
could pass any 7+ char string; head_sha_advanced accepted the
hallucination; CI poll then 404'd on the fake SHA forever (until
2h ci_poll_exhaustion).

A-2 — merging-409 → IMPLEMENTING(tier=NULL) trap
File: tools/controller/master/merging.py
On 409 the handler routes to IMPLEMENTING(tier=tier_last_succeeded);
if that's NULL (e.g., metadata-only → REVIEWING → approve → 409 path
where implementer_pushed never fired), the MCP rejects tier=NULL →
contract-violation → STUCK. Fix: default to current_tier when
tier_last_succeeded is NULL.

A-9 — Reviewer cross-field check: verdict ↔ suggested_next_action
File: tools/controller/mcp/reviewer_builder.py
Agent could set verdict=approve + suggested_next_action=abandon; the
master fired reviewer_approve regardless. New _VERDICT_ACTION_COMPAT
map enforces compatible pairs at finalize.

TESTS:

- New file ``test_batch_s_fixes.py`` with 13 regression tests, one
  per fix class.
- Updated test_master_outcomes.py for the new contract-violation
  retry behavior.
- Updated test_master_prefetch.py fixture to bump last_transition_at
  past the seeded attempts (the new T4-1 filter would otherwise
  correctly identify the pre-seeded attempts as unprocessed).

Total: 819 → 832 tests, 0 regressions.

DEFERRED to a follow-up batch (per PENDING_FIXES.md):
- 8 MEDIUM items (error UX, dead fields, signal-loss in error paths)
- 5 LOW items (agent quality polish)
- 4 CONFIRMED-CLEAN (no fix needed)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 21:15:26 -04:00
drew 3e23853ffa fix(controller): batch R — wire 5 V1-contract fields the controller silently dropped 
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>
 2026-05-18 20:40:31 -04:00
drew 2ad0958cb7 fix(controller): batch O — trial-2 fix: interpolate real attempt_id into prompts 
Trial run-2 surfaced the actual blocker: the response-builder MCPs
shared their module-global ``_STATE`` across OpenCode sessions (they're
registered as ``type: local`` in opencode.json, so OpenCode spawns one
subprocess per OpenCode-server lifetime, not per session). The CA2/PD5
cross-session reset I added in the M-batch keyed on
``identity.attempt_id`` — but the prompt template literally read:

  1. ``estimator_start(attempt_id=..., workflow_id=..., ...)``

The ``...`` were placeholder ellipses, not interpolated. So agents
guessed ``attempt_id=1`` every single time. The cross-session reset
compared ``1 == 1``, decided "same attempt — no reset", and the prior
attempt's ``finalized=True`` persisted forever. Every estimator
session after the first failed with ``"response already finalized;
no further mutations allowed"`` on every ``_set_*`` call → no
finalize → 30s worker timeout → workflow STUCK.

Observed in trial run-2: 2 successes (attempts 1, 2), then 15
consecutive failures (attempts 3–17 across all 6 workflows) before
the pickup_guard would have STUCK every workflow.

Two fixes (defense in depth):

1. **Unconditional reset on _start** in all 5 builders. We can't
   distinguish "agent retry in same session" from "new session reusing
   this MCP" reliably — the observable signature is identical. Just
   reset whenever ``_STATE.started`` is True; ``reset_for_new_attempt``
   logs a WARN if the prior state was in-flight so abandoned attempts
   are still visible to ops. The agent's last ``_start`` always wins.

2. **Interpolate real attempt_id + workflow_id + pr_number + head_sha
   into all 5 prompts' Output contract sections**. The agent_runner
   now injects ``attempt_id`` into ``input_payload`` (matching the
   existing ``workspace_dir`` injection pattern). Each ``build_*_prompt``
   reads ``input_payload.get("attempt_id")`` and bakes the concrete
   value into the MCP-call signature shown to the agent, plus a
   ``PASS THESE EXACT VALUES`` directive.

Also strengthened each prompt's ``_finalize`` line with
``**You MUST call this tool — without it the controller times out.**``
so the agent understands the contract is hard, not optional.

Updated tests:
- ``test_double_start_refused`` → ``test_double_start_resets_silently``:
  pins the new permissive-reset behavior + asserts the WARN log fires.
- ``test_implementer_rejects_intra_session_double_start`` updated for
  same reason; now asserts the second _start succeeds + last-wins
  semantics (used_tier == new tier).
- NEW ``test_prompt_interpolates_real_attempt_id`` parametrized over
  all 5 roles: asserts ``attempt_id=42`` + ``workflow_id=7`` appear
  literally in the rendered prompt AND that ``attempt_id=...`` /
  ``workflow_id=...`` placeholders DO NOT.

Total: 795 → 800 tests, 0 regressions.

The model-override files (.md + .txt) are still in place from the
prior turn — that change is orthogonal to this bug fix; OpenCode
ignores the dispatcher's pass-through per the README, and the actual
generation has been on haiku the whole time. The .md frontmatter
change to sonnet will only take effect on the next OpenCode restart.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 19:11:17 -04:00
drew a4892f8db9 fix(controller): batch N — second-round adversarial review fixes 
Round-2 adversarial review on commit 84da77421 confirmed 20+ M-batch
fixes were correct, but surfaced 4 real new issues + missing test
coverage for the gnarliest ones. This commit:

BLOCK-ON-TRIAL fixes:
- CA-R2-C: estimator + summarizer have setter-auto-start (for
  backward compat) that sets _STATE.started=True WITHOUT populating
  identity. The M-batch cross-session reset keyed on identity.attempt_id
  → never triggered for setter-first paths. An explicit _start
  afterwards saw started=True, raised "may only be called once",
  permanently wedging the MCP. Added "identity-less-carryover"
  detection to estimator_start + summarizer_start so they also reset
  in that case. Test: TestSetterThenStartDoesNotRaise (2 cases).
- PD-R2-3: empty FORGEJO_URL silently produced "/{owner}/{repo}.git"
  garbage clone URLs that failed cryptically per attempt. Worker
  startup now fails loudly (exit 2) with a clear error pointing at
  the env var. Test: TestEmptyForgejoUrlFailsStartup.
- PD-R2-7: stale-file unlink permission failure was logged as
  WARNING and proceeded — the poller would then read the stale file
  as if it were fresh attempt output (the very bug CA1 was supposed
  to fix). Hard-raises WorkerError now so the underlying perms/FS
  issue surfaces.
- PD-R2-14: legacy_adapter._clamp_tier(None) silently returned 0 for
  the implementer's `used_tier` field — lying about which tier
  actually ran when the runner has a None-tier bug. Added
  allow_none=False mode that emits a loud WARN. Test: TestUsedTierNoneWarn.

TEST COVERAGE for previously-untested fixes (TE-R2-7..16):
- CA10 credential helper: 2 tests verifying clone URL has no token
  embed AND that clone_if_absent installs `credential.helper` with
  the env-sourced FORGEJO_TOKEN reference in local git config.
- CA6 CI poll role union: test seeds implementer attempt_number=1
  then conflict_resolver attempt_number=2, asserts CI poll picks
  the newer conflict_resolver SHA.
- PD9 outcome='resolved' filter: test seeds a `blocked` implementer
  attempt with head_sha_after set, asserts the poll skips it
  (workflows_waiting=1) instead of false-greening on stale SHA.
- PD16 inline-callback exception handling: 2 tests verifying that a
  RuntimeError from the callback re-raises as
  WorkerError(outcome='worker-internal-error') AND that
  WorkerLostLock passes through unchanged.
- PD10 atomic-write .tmp leftover: pre-creates a garbage .tmp from a
  prior crashed write, runs successful finalize, asserts the .tmp is
  gone after os.replace and the final content is correct.
- PD11 V1-passthrough wallclock: explicit assertion that the agent's
  wallclock_seconds survives passthrough even when the runner passes
  a different measured wallclock.
- CA9 dataclasses.replace regression bound: asserts WorkerConfig
  field set is preserved across replace; a future-added field that's
  not handled by the CLI replace call will fail this test loudly.
- CA4 adapter telemetry positive case: paired with the existing
  negative (V1-passthrough doesn't log) test.

POLISH (CA-R2-D): adapter telemetry log promoted from INFO to
WARNING so the migration-progress signal survives production
log-level tuning (default INFO often raised to WARNING).

REGRESSION FIX: 2 existing entry_points tests broke when PD-R2-3
added FORGEJO_URL validation. Added FORGEJO_URL to those fixtures.

Total: 781 → 795 tests, 0 failures.

CONFIRMED-CLEAN by round-2 reviewers (the M-batch fixes that actually
landed correctly): CA1, CA2 (implementer/reviewer/conflict_resolver),
CA3, CA6, CA7, CA8, CA9, CA12, PD1, PD3, PD4, PD7, PD8, PD9, PD10,
PD13, PD14, PD15, PD16, PD22.

DEFERRED (not block-on-trial): test fidelity improvements
(TE-R2-1, -2 specific assertion strengthening), CA-R2-E (reviewer
same-attempt-id reuse), TE-R2-11 (CA7 lost-lock short-circuit
direct test), TE-R2-15 (cross-session concurrent race — probably
unreachable in single-MCP-per-OpenCode-process model).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 18:41:22 -04:00
drew 84da774212 fix(controller): batch M — post-Phase-1m adversarial review fixes 
Three rounds of adversarial review (Chief Architect / Principal Dev /
Senior Test Engineer) on commits 3ca794be7..db12f45ac surfaced ~35
issues. This commit addresses 25+ across criticals, highs, and
mediums, and adds 40 new tests covering the changes plus key gaps the
review identified.

CRITICALS (M1):
- CA1: stale {role}_output.json from a prior attempt on the same
  per-PR workspace was readable as "fresh" output of the new attempt.
  agent_runner now unlinks the MCP-canonical path AND every fallback
  path BEFORE the session runs.
- CA2/PD5: opencode.json-registered MCP subprocesses persist across
  OpenCode sessions, but BuilderState was module-singleton. Added
  reset_for_new_attempt() + cross-session detection (compare
  identity.attempt_id) to every *_start; force-resets with WARN if
  prior attempt was interrupted (timeout / lost lock).
- PD3: inline-JSON callback could overwrite an MCP-written canonical
  V1 file with adapted-from-prose garbage. Callback now inspects
  existing files and skips when V1 is already present.
- PD4: FORGEJO_URL = .rstrip("/api/v1") is a character-set strip —
  catastrophic for hosts whose path contains /v1 in the middle.
  Replaced with explicit endswith()-based suffix strip.
- CA10: clone URL embedded $FORGEJO_TOKEN, persisted into
  .git/config where any agent could cat it. Token now sourced via
  local credential.helper at clone-time, URL kept clean.
- CA12: state.finalized was set BEFORE the file write, so disk-full
  / OSError left the agent unable to retry finalize. Reordered.

HIGHS (M2):
- CA3/PD12: output_path validation (NUL-byte rejection, must be
  absolute, parent-not-file check) in finalize_and_emit.
- CA6: ci_status_poll SELECT only considered implementer attempts;
  conflict_resolver also pushes commits. SQL now unions both roles.
- PD9: ci_status_poll could advance on a stale "resolved" SHA from a
  blocked attempt (whose head_sha_after == head_sha_before). Added
  outcome='resolved' filter.
- CA8: cancelled/stale CI states mapped to ci_red_retry_same_tier,
  burning pickup_count on healthy PRs. Both now wait (treated as
  operator/system action, not failure). timed_out stays red.
- TE9: unknown Forgejo CI states now WARN-log instead of silently
  being treated as pending — operators see new state strings.
- PD8: ci_status_poll event_type strings standardized to match the
  state-machine event names (ci_green / ci_red_retry_same_tier)
  instead of legacy ci-green / ci-red.
- CA7: inline-JSON callback now checks lost_lock_check BEFORE write
  so a file isn't staged after lock loss.
- PD10: atomic .tmp + os.replace writes in both MCP finalize and
  inline callback so the poller never sees a half-written file.
- PD16: inline_output_callback exceptions now re-raise as WorkerError
  instead of being silently logged (root cause was buried 30s later
  in a canonical-output timeout).
- CA9: WorkerConfig manual rebuild on --max-concurrent/--poll-interval
  silently dropped new fields. Use dataclasses.replace, matching
  round-4 P5 fix in master/__main__.py.

MEDIUMS (M3) — legacy_adapter quality upgrades:
- PD1: unrecognized confidence values now WARN instead of silently
  defaulting to "medium" — surfaces agent prompt drift.
- PD2: estimator recommended_tier clamped to {0,1,2} so an out-of-
  range int doesn't bypass the adapter's whole purpose.
- PD7: reviewer blocking_issues list-of-strings coerced into the
  list-of-BlockingIssue-dict shape strict_parse requires.
- PD13: conflict_resolver prompt defaults tier=1 + warns instead of
  raising; the scheduler always sets it but defends against drift.
- PD14: summarizer summary < 50 chars padded with a clear marker so
  strict_parse accepts it (and the truncation is visible).
- PD15: implementer blockers capped at 4096 chars each so a buggy
  agent can't blow up audit log / DB column.
- PD17: launch script accepts either FORGEJO_TOKEN or GITEA_TOKEN
  with a clear error if both are unset.
- PD22: conflict_resolver adapter accepts singular commit_sha
  fallback, matching implementer.
- CA4: every adapter invocation logs role + payload key fingerprint
  so operators can measure agent-migration progress.
- estimator + summarizer now have explicit _start tools (the prompts
  already referenced them; previously absent → first call would fail).

TESTS (M4) — added 40 tests in test_post_review_fixes.py:
- Cross-session MCP state reset (implementer + reviewer + estimator
  + summarizer; intra-session double-start still rejected).
- finalize_and_emit output_path precedence (arg > env > stdout),
  parent-dir creation, rejection of relative/NUL paths, failed-write
  leaves state retryable.
- legacy_adapter quality: tier clamping, blocker cap, non-string
  commit warning, blocking_issues string coercion, conflict_resolver
  full roundtrip + non-resolved head clearing, summarizer padding,
  confidence warning, V1-passthrough no-log.
- opencode.json registration parity: every MCP the prompts name is
  registered with the correct module path.
- Per-role prompts mention {role}_output.json (canonical poller path)
  + the "DO NOT emit chat-JSON" directive.
- FORGEJO_URL suffix-strip parametrized table.
- agent_runner stale-file cleanup: prior-attempt file is unlinked
  before a new session can read it as phantom output.

Also updated 2 pre-existing tests for the CA8 / PD8 / PD13 behavior
changes (cancelled→wait, event_type renaming, conflict_resolver
default-tier warning).

Total: 741 → 781 tests, 0 regressions.

DEFERRED (M5 follow-up — non-trial-blocking):
- CA5: head_sha verification via git cat-file (requires subprocess).
- CA11: discovery_interval_s wall-time cadence (vs iteration count).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 18:26:06 -04:00
drew db12f45acb feat(controller): Phase 1m — wire response-builder MCPs into OpenCode (Option A) 
The trial-path expedient (legacy_adapter harvesting chat-JSON) is now
the SAFETY NET; the architecture-intended path (agents call the
controller's response-builder MCPs, which validate against V1 +
write canonical JSON) is now wired end-to-end.

Four pieces:

1. MCP `finalize` accepts `output_path` argument
   ``mcp/_builder_base.finalize_and_emit`` adds ``output_path: str | None``
   kwarg. Precedence: explicit arg > ``CONTROLLER_CANONICAL_OUTPUT_PATH``
   env > stdout. This solves the per-attempt path problem that
   blocked opencode.json registration (the env var is static; the
   per-attempt path comes from the prompt, the agent passes it as a
   tool arg). Each role's finalize updated:
   - estimator_finalize(output_path)
   - implementer_finalize(output_path)
   - reviewer_finalize(output_path)
   - conflict_finalize(output_path)
   - summarizer_finalize(output_path)
   Creates parent directory if missing (so the controller doesn't
   need to pre-create). Returns ``wrote_to`` in the ok dict so tests
   can pin the path.

2. opencode.json registers the 5 controller MCPs
   ``.opencode/opencode.json`` adds:
   - implementer-response-builder
   - reviewer-response-builder
   - estimator-response-builder
   - conflict-resolver-response-builder
   - summarizer-response-builder
   Each spawned via ``python -m tools.controller.mcp.{role}_builder``
   with PYTHONPATH=/repo-root so the controller imports resolve.

3. Controller prompt builder injects the EXACT tool sequence
   ``worker/prompts.py`` rewrites each role's "Output contract"
   section. The old "PREFERRED MCP / FALLBACK file-write" instruction
   becomes a single REQUIRED contract: numbered tool calls (``X_start``,
   ``X_set_*``, ``X_finalize(output_path=...)``) with the explicit
   per-attempt path baked in. Explicit "DO NOT emit a JSON object in
   your final chat message" instruction to override the legacy
   contract baked into the agent system prompts.

4. Agent permission whitelists include the new MCPs
   - ``.opencode/agents/task-implementor.md`` (source for tier-{0,1,2,min}
     variants — regenerated via tools/sync_tier_models.py)
   - ``.opencode/agents/pr-review-worker.md``
   - ``.opencode/agents/estimator-implementation.md``
   - ``.opencode/agents/conflict-resolver-worker.md``
   Each adds the matching ``"{role}*": allow`` pattern.

Safety net preserved:
The legacy_adapter (commit bcc59d38a) is KEPT as a fallback path.
If an agent ignores the new instruction + emits chat-JSON anyway,
``opencode_session.py:inline_output_callback`` harvests it +
``legacy_adapter`` normalizes to V1 + writes to the canonical out
path. Both paths produce valid V1 → ``strict_parse`` succeeds. The
MCP path is now the WORKING preferred path; the chat-JSON harvest
is the safety net.

Total: 738 controller tests pass (+0 net — wiring change, no new
behavior tests), 0 regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 18:10:48 -04:00
drew bcc59d38af fix(controller): trial-path output channel — harvest inline JSON + legacy-shape adapter 
Trial run-1 surfaced the real failure mode: the existing agent
prompts emit their result as a single JSON object in the FINAL
assistant message (the legacy dispatch_implementer/dispatch_review
contract). They DON'T call the controller's response-builder MCPs
(not registered in opencode.json) and they DON'T write a file. The
worker's canonical-output poller timed out every attempt.

Fix (two pieces):

1. INLINE JSON HARVEST
   ``_opencode_worker.run_session_blocking`` already extracts the
   last JSON object from the agent's final response into
   ``SessionResult.parsed_json``. The controller's opencode_session
   adapter was ignoring it.
   - ``opencode_session.py:run_opencode_session`` now accepts an
     ``inline_output_callback`` kwarg. After the session completes,
     if parsed_json is set, the callback fires.
   - ``agent_runner.py`` provides the callback: writes the parsed
     JSON to the canonical-output path so the existing poller picks
     it up uniformly with the MCP + file-write channels.

2. LEGACY → V1 SHAPE ADAPTER (``worker/legacy_adapter.py``)
   The legacy JSON shape doesn't match V1 contracts:
   - estimator: legacy {recommended_tier, is_confident, reasoning}
     vs V1 {output_version, recommended_tier, is_metadata_only,
     confidence, reasoning, wallclock_seconds}
   - implementer: legacy {outcome: resolved|unresolved, ...} vs V1
     {outcome: resolved|rebase-failed|blocked|noop|competence-failure,
     commit_shas, blockers, used_tier, ...}
   - reviewer: legacy {verdict, ...} vs V1 {output_version, verdict,
     blocking_issues, suggested_next_action, ...}

   ``adapt_to_v1(role, payload, tier, wallclock_seconds)`` normalizes
   each role's legacy shape into the corresponding V1 dict:
   - adds ``output_version="V1"``
   - maps outcome enums (``unresolved`` → ``blocked``)
   - synthesizes missing required fields with sensible defaults
   - coerces singular commit_sha → list[commit_shas]
   - converts legacy is_confident bool → confidence string

   Already-V1 payloads pass through unchanged. The agent_runner
   wraps the inline_output_callback to run the adapter before
   writing to the canonical path.

Tests (+12 in test_legacy_adapter.py):
- Each role's adapt: legacy in, V1-validating-via-strict_parse out
- Already-V1 passthrough
- Outcome enum mappings + invalid-outcome fallback
- Singular commit_sha normalization
- Non-dict / unknown-role passthrough (lets strict_parse raise)

Total: 738 controller tests pass (+12 net), 0 regressions.

Long-term: update agent prompts to emit V1 directly, register the
MCPs in opencode.json. For the trial, this adapter lets the existing
agents flow through the new controller unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 17:56:47 -04:00
drew 3ca794be75 feat(controller): autonomous CI status polling — closes the last trial gap 
The Phase 2 trial previously required operator-intervention SQL to
advance workflows from AWAITING_CI → REVIEWING (no automated CI
status polling). This commit wires the missing tick so the trial
runs end-to-end without manual help.

Components:
- ``master/forgejo_http.py``: new ``get_ci_status`` callback wraps
  Forgejo's ``/commits/{sha}/status`` combined-status endpoint;
  added to ``ForgejoCallbacks``.
- ``master/ci_status_poll.py`` (NEW): ``run_ci_status_poll_tick``
  scans AWAITING_CI workflows, fetches CI status keyed on the
  latest implementer attempt's ``head_sha_after``, and applies
  state transitions via ``apply_event``. TOCTOU-defended UPDATE
  (``WHERE current_state='AWAITING_CI'``) + per-row exception
  isolation.
- ``master/loop.py``: new ``ci_status_poll_args=(owner, repo,
  get_ci_status)`` kwarg + ``ci_status_poll_interval_s`` config
  (default 60s) + ``MasterTickReport.ci_status_poll`` field.
- ``master/__main__.py``: threads ``callbacks.get_ci_status`` into
  the loop.

State mapping (Forgejo combined-status state → event):
- success / neutral / skipped / warning → ci_green → REVIEWING
- failure / error / cancelled / timed_out / stale →
  ci_red_retry_same_tier → IMPLEMENTING
- pending / queued / in_progress / action_required → no-op (wait)
- None / unknown / fetch failure → no-op (transient)

The ``ci_polling_exhausted`` timeout (default 2h) remains as the
safety net for CI that genuinely never reports.

Tests (+14 in test_master_ci_status_poll.py):
- Happy paths (success→green, failure→red, pending→wait)
- Error paths (callback raises; workflow without head_sha)
- Event row shape (event_type='ci-green'/'ci-red', reason payload)
- Extended state mapping (cancelled, neutral, in_progress)
- Other-repo isolation
- LoopIntegration end-to-end via master_main_loop with safety timer

RUNBOOK updated: removed the manual SQL workaround; added the
autonomous CI poll's tunables.

Total: 726 controller tests pass (+14 net), 0 regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 17:37:04 -04:00
drew f93f0d1c53 fix(controller): round-4 P2 — defensive input_payload normalization 
If the DB returned NULL input_payload (legacy/seeded data, schema
bug, hand-edited rows), agent_runner crashed on ``dict(None)`` →
WorkerError → master re-pickups → STUCK after MAX_PICKUPS reaps.
Silent infinite-loop until exhaustion.

Fix: ``worker/runner.py:run_one_attempt`` checks isinstance(dict)
on entry; substitutes empty dict + logs WARNING. Agent still runs
normally with an empty payload.

Test: ``test_none_input_payload_doesnt_crash`` — passes
input_payload=None + verifies the attempt completes normally.

Total: 712 controller tests pass (+1 net).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 17:25:01 -04:00
drew f57d9f9478 fix(controller): batch L — round-4 trial-blockers (A1, P1, P3, P4, P5, T5) 
Round-4 adversarial review found 5 trial-blockers + 1 silent-debt
item the post-round-3 deep pass missed. All fixed.

A1 — pre-clone the workspace so the agent has a worktree to operate on
``worker/__main__.py``: the agent_runner closure now constructs a
``PerPRWorkspace`` from input_payload.owner/repo/pr_number + the
FORGEJO_URL+FORGEJO_TOKEN env vars. Pre-flight:
- ``workspace.ensure_present()`` creates the dir skeleton.
- ``workspace.clone_if_absent()`` clones the repo into
  ``{workspace_dir}/worktree/`` if not already present (idempotent).
- ``workspace.fetch_and_validate(head_sha, head_ref)`` refreshes +
  verifies the workspace is at the expected head. ``StaleInputError``
  → ``WorkerError(outcome='stale-input')`` so the master re-prefetches
  without burning a pickup. ``RuntimeError`` → ``worker-internal-error``.
Previously the agent saw an empty workspace_dir + had no repo.

P1 — partial-write defense in the canonical-output poller
``worker/agent_runner.py:_wait_for_canonical_output`` now polls each
path with a two-pass quiescence check (size stable + content parses
as JSON) before returning. Partial writes (agent crashed mid-flush)
are skipped + the polling loop continues. The previous
``f.read().strip()`` returned partial JSON which then tripped
``ContractValidationError`` → ``worker-internal-error`` with no
record of WHICH path; now logs source path on every read.

P3 — TOCTOU defense in promote_discovered
``master/promote.py``: the UPDATE now filters
``current_state='DISCOVERED'``. If a concurrent reconciliation
moved the row off DISCOVERED between SELECT and UPDATE, rowcount=0
+ we skip the event-row write. No duplicate audit entry; no
overwriting a pause-by-label-removal.

P4 — explicit tuple-length validation in reconciliation_args + discovery_args
``master/loop.py``: previously a 6-tuple silently fell into the
``else`` 4-tuple unpack, raised ValueError("too many values"), got
swallowed by the per-iter ``except Exception``, and reconciliation
silently died forever. Now: ``elif n == 4`` + ``else: raise TypeError``.
The TypeError still hits the per-iter except (so the loop doesn't
crash) but ``logger.exception`` surfaces the actionable message in
journald. Operator sees "reconciliation_args must be a 4- or 5-tuple;
got length 6" instead of zero indication.

P5 — --tick-interval CLI flag preserves other config fields
``master/__main__.py``: replaced the manual ``MasterConfig(...)``
rebuild (which dropped reconciliation/ci_poll/discovery intervals)
with ``dataclasses.replace(cfg_loop, tick_interval_s=args.tick_interval)``.
Operators who pass --tick-interval no longer silently revert the
other intervals to defaults.

T5 — scheduler._commit_escalation uses safe_json_dumps
``master/scheduler.py``: the escalation event row's payload was the
only call site that bypassed safe_json_dumps. Now consistent — a
future contributor adding a datetime/Decimal field won't trip raw
json.dumps at runtime.

Tests (+4 net):
- ``test_worker_agent_runner.py::test_partial_write_not_read``: pins
  P1 (truncated fallback file + valid MCP output → MCP wins).
- ``test_master_promote.py::test_toctou_state_change_between_select_and_update``:
  pins P3 (steal state via monkey-patch → no double-promotion, no
  extra event row).
- ``test_master_loop.py::test_reconciliation_args_wrong_length_logs_not_silent``:
  pins P4 (6-tuple → logged error, not silent forever).
- ``test_entry_points.py::test_tick_interval_flag_preserves_other_cfg_fields``:
  pins P5 (env-set non-default intervals survive --tick-interval).

Total: 711 controller tests pass (+4 net), 0 regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 17:23:35 -04:00
drew b4ebf6a2bd docs(controller): RUNBOOK — Phase 2 trial gaps + manual workarounds 
Document the two known gaps that need operator intervention for an
end-to-end trial:

1. MCP-to-OpenCode transport: response-builder MCPs aren't yet
   registered in opencode.json. Agents use the FALLBACK file-write
   path (``{workspace_dir}/{role}_output.json``) per the prompt
   instructions. The agent_runner polls both channels.

2. CI status polling not wired: AWAITING_CI exits only via
   ci_polling_exhausted timeout (default 2h → STUCK). To advance
   during the trial, the operator runs a SQL UPDATE to manually
   transition AWAITING_CI → REVIEWING (snippet in the RUNBOOK).

Added a step-by-step Trial checklist covering: env vars, label
the PR, expected log timeline, the manual SQL to advance past
AWAITING_CI, and the verification query.

Both gaps have follow-up phases queued (1m for MCP wiring, 1n for
CI poller). The trial as described validates the full controller
state machine + most of the worker substrate; only the CI poll +
the MCP transport are operator-intervention paths.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 16:57:34 -04:00
drew 7bf39a0b51 fix(controller): RB8 — head_sha bookkeeping in _write_outcome 
The worker's _write_outcome never wrote head_sha_before / head_sha_after
on the workflow_attempts row. But tick.py reads both to compute
head_sha_advanced, which the outcome mapper REQUIRES to distinguish
``implementer_pushed`` (true push) from ``implementer_blocked``
(worker said resolved but git didn't move).

Without these columns set, every implementer attempt's "resolved"
outcome mapped to a no-op event, and workflows would stall
permanently after the agent ran successfully.

Fix:
- ``worker/runner.py:_write_outcome`` accepts ``head_sha_before`` +
  ``head_sha_after`` kwargs and writes both columns in the UPDATE.
- ``run_one_attempt`` computes:
  - head_sha_before = input_payload["head_sha"] (what the agent
    was told to start from)
  - head_sha_after = output_payload["commit_shas"][-1] if any
    commits were produced; falls back to head_sha_before otherwise
    so tick.py correctly sees "no advance" when the agent didn't
    commit.

Tests (+2):
- test_head_sha_before_after_recorded — pins the happy path where
  the agent committed and head_sha advanced.
- test_no_commits_means_head_sha_unchanged — pins the no-advance
  case where head_sha_after equals head_sha_before.

Total: 707 controller tests pass (+2 net), 0 regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 16:55:23 -04:00
drew 0c9e8fdd69 fix(controller): trial-path output channel — fallback file-write for unwired MCP layer 
RB7 (newly discovered) — response-builder MCPs are NEVER REACHABLE
from the OpenCode agent:

The controller spawns the per-attempt response-builder MCP via
``python -m tools.controller.mcp.{role}_builder`` with stdin=PIPE
+ CONTROLLER_CANONICAL_OUTPUT_PATH set. But:
- ``.opencode/opencode.json`` does NOT register the controller MCPs.
- OpenCode only knows about MCPs it sees in opencode.json (graphify,
  ci, forgejo, git, handoff).
- The OpenCode agent has NO transport to call
  ``implementer_finalize`` because OpenCode doesn't see the MCP.
- The controller's spawned MCP subprocess sits waiting for JSON-RPC
  on stdin that no one sends.
- The agent does work, the session ends, the canonical-output-file
  is empty, agent_runner times out with WorkerError.

This would have been the single biggest blocker to running an actual
Phase 2 trial. The MCP layer is structurally complete but not
plumbed into OpenCode — pivoting requires either registering them
in opencode.json (which doesn't support per-attempt env injection)
OR adding a fallback channel.

Fix (trial-path):
- ``worker/prompts.py``: each role's output-contract section now
  documents BOTH the PREFERRED MCP call AND a FALLBACK direct
  file-write to ``{workspace_dir}/{role}_output.json``. The agent
  can pick either; once the MCP layer is wired (Phase 1m) it'll
  naturally prefer the MCP.
- ``worker/agent_runner.py:_wait_for_canonical_output``: now polls
  the MCP-canonical path AND a list of ``fallback_paths``. Call
  site adds ``{workspace_dir}/{role}_output.json`` to fallback
  paths. First non-empty wins.

Tests (+2):
- ``test_fallback_file_picked_up_when_mcp_silent``: agent writes
  only the fallback file (no MCP drive); runner reads it.
- ``test_first_nonempty_output_wins``: documents the
  poll-both-channels semantic.

Documented Phase-1m work:
- Register controller MCPs in ``.opencode/opencode.json``
- Solve per-attempt env injection (the canonical_output_path is
  per-attempt; opencode.json env is static — needs a request_id
  arg on finalize() OR a workspace-relative output convention)

Until that's done, the trial uses the fallback file-write path.

Total: 705 controller tests pass (+2 net), 0 regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 16:52:16 -04:00
drew 251eeb21ff fix(controller): more pipeline run-blockers — merging tick, periodic discovery, worker create_all, systemd ordering 
Continuing the round-3 deep-pass cleanup. Three more run-blockers
+ one robustness fix.

RB5 — MERGING handler never invoked from master loop:
``run_merging_tick`` was exported by the master package but no caller
fired it. Workflows that transition to MERGING (via reviewer
approval) would sit there indefinitely with no Forgejo merge call.

Fix:
- ``master/loop.py`` accepts a ``merging_args=(owner, repo,
  merge_callback)`` kwarg. When set, the tick fires every iteration
  (cheap if no workflows in MERGING).
- ``MasterTickReport`` gains ``merging: MergingHandlerReport | None``.
- ``master/__main__.py`` wires it from the Forgejo callback bundle.

RB6 — periodic discovery never fires:
``run_discovery`` was only called at startup via
``run_startup_backfill`` + the ``--discovery-only-once`` smoke flag.
PRs created after master startup would not be discovered until the
master restarted.

Fix:
- ``master/loop.py`` accepts ``discovery_args=(owner, repo, list_prs,
  list_issues)`` or the 5-tuple with kwargs. Periodic tick on its
  own cadence (``CONTROLLER_DISCOVERY_INTERVAL_S``, default 30s).
- ``MasterTickReport`` gains ``discovery: DiscoveryReport | None``.
- ``master/__main__.py`` wires it + threads ``require_opt_in_label``
  through.

RB-robust — worker calls create_all defensively:
Master is normally responsible for schema creation (workers run
After= it via systemd ordering). But if the worker is started in
isolation (test / local dev / unit ordering broken), it'd crash on
the first query against missing tables.

Fix:
- ``worker/__main__.py`` calls ``create_all(engine)`` after
  ``build_engine``. ``create_all`` is idempotent (CREATE TABLE IF
  NOT EXISTS); safe to call from both master + worker.
- ``cleveragents-controller-worker@.service`` adds
  ``After=cleveragents-controller-master.service`` +
  ``Wants=cleveragents-controller-master.service`` so systemd
  enforces the start ordering in production.

Total: 703 controller tests pass (no test changes; all new wiring
is exercised by master_main_loop tests via the new kwargs).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 16:47:10 -04:00
drew febb352618 fix(controller): pipeline run-blockers — promoter, scheduler, owner/repo, workspace_dir patch 
Round-3 deep pass identified four issues that would have prevented an
actual end-to-end pipeline run:

RB1 — DISCOVERED → ANALYZING never fired in production:
The state machine defines (DISCOVERED, discovery_picked_up) →
ANALYZING but NO production code fires the event. Workflows
created by discovery would sit in DISCOVERED forever.

Fix:
- New ``master/promote.py``: ``run_promote_discovered_tick`` scans
  for DISCOVERED workflows + fires ``discovery_picked_up`` via
  apply_event (state-machine invariants stay enforced) + emits a
  ``discovery-promoted`` controller_events row per transition.
- Composes with the master loop's other ticks; runs every iteration
  (cheap — typically 0-1 row).

RB2 — scheduler.schedule_next_attempts never called from master loop:
The scheduler was exported by the master package but never invoked.
It creates the ``workflow_attempts`` rows that workers dequeue —
without it, workers would have nothing to pick up.

Fix:
- ``master/loop.py`` now accepts a ``prefetch: PrefetchCallback``
  kwarg. When provided, the loop runs promote_discovered + scheduler
  every iteration after tick/reaper/reconciliation.
- ``MasterTickReport`` gains ``promote_discovered`` and ``scheduler``
  optional fields so on_iteration callbacks see both.
- ``master/__main__.py`` builds a ``PrefetchDataCallbacks`` from the
  Forgejo callback bundle and constructs the production
  ``make_prefetch_callback(engine, callbacks)`` — wires through to
  the loop's new prefetch kwarg.

RB3 — owner / repo missing from V1 input contracts:
The implementer / reviewer / estimator / conflict-resolver V1 inputs
had pr_number but not owner/repo. The OpenCode agent would have
had no way to know which Forgejo repo to clone — it would have had
to derive owner/repo from process env, coupling the worker to a
single repo.

Fix:
- ``contracts/v1.py``: added ``owner: str`` and ``repo: str``
  (min_length=1) to ImplementerInputV1, ReviewerInputV1,
  EstimatorInputV1, ConflictResolverInputV1.
- ``master/prefetch.py``: builders populate owner/repo from the
  Workflow row (already known at prefetch time).
- Existing test fixtures in ``test_contracts_v1.py`` updated.

RB4 — input_payload.workspace_dir placeholder reached the agent:
Prefetch wrote ``workspace_dir = "<worker-injected>"`` as a
placeholder; the worker never patched it before invoking the
OpenCode session. The prompt builder rendered the literal
placeholder string into the agent's prompt — the agent had no idea
where to clone.

Fix:
- ``worker/agent_runner.py``: patches input_payload.workspace_dir
  with the real path immediately before calling run_opencode_session.
  Uses a shallow copy so the caller's dict isn't side-effected.
- ``worker/__main__.py``: workspace_dir naming convention is now
  ``pr-{owner}-{repo}-{pr_number}`` (matches workspace.py's
  PerPRWorkspace convention) so the janitor's pr-* glob + the
  agent's expected workspace location agree. Falls back to
  ``pr-attempt-{N}`` for legacy input_payloads missing owner/repo.

Tests:
- ``test_master_promote.py`` (NEW, +7 tests):
  - empty DB no-op
  - single workflow promoted
  - multiple promoted in one tick
  - only DISCOVERED targeted (non-DISCOVERED untouched)
  - controller_events row emitted with correct shape
  - idempotent after first promotion
  - LoopIntegration end-to-end: DISCOVERED → ANALYZING → pending
    estimator attempt visible in workflow_attempts (pins the entire
    previously-broken pipeline from discovery to enqueue)

Total: 703 controller tests pass (+7 net), 0 regressions.

Without these four fixes, the pipeline would have looked alive in
unit tests but produced zero work in a real deployment.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 16:43:36 -04:00
drew c8677d985d fix(controller): batch J — round-3 cleanup (R3, R4, R5, R6, R8, R10) 
R3 — delete unused schema columns:
``workflows.ci_flake_retries_remaining`` and
``workflows.awaiting_ci_started_at`` shipped in round-1 batch D as
"staged for future tick handlers" — but no producer or reader ever
used them. ci_poll.py uses ``entered_state_at`` (already populated
on every transition) as the AWAITING_CI start timestamp. Delete
both columns; the flake-retries counter should ship with its
producer, not as speculative schema.

R4 — safe_json_dumps raises on sets:
Previously sets/frozensets were silently coerced to sorted lists.
This contradicted the "raise loudly" design intent (JSON has no
native set; a reader doing ``parsed["tags"]`` would get a list,
losing set algebra). Now raises with an actionable message
pointing the producer at ``sorted(list(...))`` for explicit
conversion.

R5 — TestSQLiteConcurrentDequeue docstring honest:
The test name implied it pinned SQLite's busy_timeout retry. It
doesn't — :memory: + StaticPool means both threads share one
connection (SQLAlchemy serializes per-connection). Updated
docstring to say what the test ACTUALLY pins (Python-level
serialization safety, exactly-one-winner, clean loser-reason) and
explicitly what it doesn't (cross-process SQLITE_BUSY retry,
which would need file-backed SQLite + QueuePool — not shipped
because multi-machine requires Postgres).

R6 — safety timer on test_loop_runs_ci_poll_exhaustion_on_cadence:
Previously the test relied entirely on on_iter setting stop when
workflows_exhausted>0. If the logic regressed (SQL schema drift,
on_iter never seeing the count), the test wedged CI indefinitely.
Now armed with threading.Timer(5.0, stop.set) safety net + an
assertion that surfaces the failure mode if the safety timer
fired first.

R8 — _json_safe scope documented honestly:
The docstring claimed "everywhere the controller serializes" but
the encoder is only wired at runner.py and scheduler.py — the two
sites that serialize WORKER-ORIGINATED payloads. Other json.dumps
call sites (event-row payloads, discovery markers) serialize
fixed-shape dicts of native types and don't need restriction.
Updated docstring to scope the claim accurately.

R10 — missing test assertions added:
- test_strict_parser_coverage_blocks_startup_with_stubs: now
  captures logs + asserts the operator-facing error message is
  emitted (so journald shows the cause; a silent rc=2 would be
  confusing).
- test_externally_merged_takes_priority_over_label_removal: now
  asserts event_type="external-merge" + reason="externally-merged"
  (a regression that transitioned correctly with the wrong reason
  in the audit trail would now be caught).
- test_second_pause_captures_post_resume_state (NEW): pause/resume/
  pause cycle. After RESUME + workflow advances to REVIEWING, a
  second PAUSE must capture REVIEWING as pre_pause_state (not the
  original IMPLEMENTING). Round-2 coverage only exercised first
  pause.

Total: 696 controller tests pass (+3 net from new + updated tests),
0 regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 16:35:17 -04:00
drew 9b6b64f0d3 fix(controller): batch I — round-3 correctness items (R2, R7, R9) 
R2 — STUCK short-circuits the opt-in label gate:
Round-2's batch G shipped reconciliation-ordering with merged/closed
winning over label removal. But _decide_transition returns
("STUCK", "pr-not-found-on-forgejo") for 404s — that STUCK was
falling through to the label gate. Operator removing the label on a
deleted PR could PAUSE it forever.

Fix: extend the short-circuit set in master/reconciliation.py to
include STUCK alongside MERGED/ABANDONED. All Forgejo-terminal
transitions now bypass the label gate.

Test: test_pr_404_takes_priority_over_label_removal pins the
contract end-to-end (404 + no opt-in label → STUCK, not PAUSED).

R7 — defense-in-depth for corrupt workflow state in ci_poll:
ci_poll.py only caught IllegalTransitionError from apply_event, but
apply_event raises ValueError for states not in KNOWN_STATES (DB
row corruption, unknown-state guard miss). A single bad row would
have aborted the whole tick.

Fix: broaden the exception handler to (IllegalTransitionError,
ValueError). One bad row is skipped; valid rows still STUCK.

Test: test_unknown_state_skips_row_doesnt_crash_tick monkey-patches
apply_event to raise ValueError once + verifies the tick processes
the other workflow normally.

R9 — distinct event_types per reconciliation reason:
Pause / resume / external-merge / external-close / external-issue-
close / pr-not-found-on-forgejo all used event_type='reconciliation'.
Operators querying controller_events for "what happened" could
only distinguish via JSON-payload LIKE queries — dialect-specific
(SQLite LIKE vs Postgres ::jsonb->>).

Fix: master/reconciliation.py introduces _REASON_TO_EVENT_TYPE
mapping:
- opt-in-label-removed       → 'label-pause'
- opt-in-label-restored      → 'label-resume'
- externally-merged          → 'external-merge'
- externally-closed-not-merged → 'external-close'
- issue-closed-externally    → 'external-issue-close'
- pr-not-found-on-forgejo    → 'external-pr-deleted'
Unknown reasons fall back to 'reconciliation' so future contributors
adding a new reason still emit a well-formed row.

Both _apply_transition and _apply_transition_with_pre_pause now
derive event_type via _event_type_for(reason).

Tests updated (filter by new event_type per case):
- test_master_reconciliation.py::TestEventRows refactored:
  - test_transition_emits_external_merge_event
  - test_closed_pr_emits_external_close_event (NEW)
  - test_pr_404_emits_external_pr_deleted_event (NEW)
  - test_consistent_workflow_no_event widened to all 7 event_types
- test_label_gate.py: pause/resume tests filter by 'label-pause' /
  'label-resume' respectively + assert the event_type matches.

Total: 693 controller tests pass (+2 net), 0 regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 16:32:23 -04:00
drew c4a7f0027d fix(controller): batch H — round-2 important items (N5-N10) 
Six items from the round-2 adversarial review's "important" list.

N5 — restricted JSON encoder replaces ``default=str``:
``default=str`` silently stringified custom objects, sets, and bytes
to ``"<MyObj at 0x...>"`` — masking worker output bugs. Now uses a
restricted encoder (``tools/controller/_json_safe.safe_json_dumps``)
with an allowlist:
- datetime / date → ISO-8601 string
- Decimal → str (preserves precision)
- UUID → canonical string
- Path → str
- set / frozenset → sorted list (best-effort)
- Everything else → TypeError (a worker output regression surfaces
  loudly instead of writing garbage to the DB)

Replaces ``json.dumps(..., default=str)`` at:
- ``worker/runner.py`` (terminal-state UPDATE write)
- ``master/scheduler.py`` (input_payload INSERT + UPDATE)
Tests: ``test_json_safe.py`` (+15 tests) — every allowlisted type +
rejected types (custom class, bytes, complex) + nested structures
+ kwargs forwarding.

N6 — strict parser check now runs BEFORE backfill + loop:
Previously the strict-parser exit could run AFTER backfill (the test
``test_strict_parser_coverage_blocks_startup_with_stubs`` passed
because backfill's ``fail_if_called`` AssertionError was swallowed
by the bare ``except Exception``, then strict exited 2). The test
asserted the right outcome via the wrong path.

Fix:
- ``master/__main__.py``: parser-coverage check moved to immediately
  after engine creation, BEFORE backfill + loop. Strict-mode failure
  exits 2 without wasting a Forgejo round-trip + without dependent
  code paths firing.
- Test refactored: count-based assertions on backfill and loop call
  counts (0 each) instead of fail_if_called. Catches regressions
  where the strict check moves back below either.

N7 — _to_aware_datetime unit-tested in isolation:
Previously exercised only via end-to-end comment-filter test. New
``TestToAwareDatetime`` (+12 tests) covers: None, empty string, Z
suffix, +00:00 offset, microseconds preserved, naive datetime →
UTC, malformed string → None, partial string → None, unsupported
types → None, timezone abbreviations → None, equality across
Z + offset forms (the regression the helper exists to defend).

N8 — runtime=None lazy-import branch tested:
Previously all 24 HTTP factory tests injected a fake runtime; the
production path (``build_callbacks(cfg=None)`` → sys.path injection
+ lazy import of ``tools._claim_runtime``) was untested.
``TestBuildCallbacksDefaultRuntime`` (+2 tests): verifies the import
succeeds + every ForgejoCallbacks attribute is callable; verifies
the import is idempotent (second call doesn't crash on sys.path
re-insert).

N9 — resume event-row emission asserted:
Round-1's batch B added the PAUSE event-row test
(``test_label_removed_emits_event_with_reason``) but not RESUME.
``test_label_restored_emits_event_with_reason`` pins the resume's
controller_events shape (from_state=PAUSED, to_state=<prior>,
reason="opt-in-label-restored", source="reconciliation") so
operators auditing the timeline see both pause + resume.

N10 — concurrent SQLite dequeue test:
The dequeue docstring claims SQLite BEGIN-DEFERRED concurrent
dequeues "retry via busy_timeout (5s)" — but no test verified.
``TestSQLiteConcurrentDequeue::test_two_threads_racing_one_wins``
spawns two threads, both attempt dequeue simultaneously via a
threading.Barrier. Asserts: neither thread raises SQLITE_BUSY-
without-retry, exactly one acquires the attempt, the loser sees
no_pending_eligible (winner committed first).

Total: 691 controller tests pass (+31 net), 0 regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 16:15:01 -04:00
drew 147e3403c1 fix(controller): batch G — show-stoppers from round-2 review (N1–N4) 
Four items the round-2 adversarial review flagged as ship-blockers.

N1 — PID-reuse defense is now WIRED in production:
Round 1's batch D shipped ``subprocess_starttime`` in the sidecar +
janitor checks against it, but NO production code wrote sidecars.
The defense was unwired; tests passed against a code path that
production never invoked.

Fix:
- ``worker/agent_runner.py`` accepts ``workspace_dir`` and
  ``opencode_server_url`` kwargs. When ``workspace_dir`` is set, it
  writes a sidecar (``{workspace_dir}/worker.session``) immediately
  after MCP spawn capturing the real PID + starttime from
  ``/proc/{pid}/stat`` field 22. Removes it on attempt completion.
- ``worker/__main__.py`` builds the per-attempt workspace dir
  (``{workspace_root}/pr-attempt-{N}/``) and threads it through the
  agent_runner closure with the OpenCode URL. The naming convention
  is picked up by the janitor's ``pr-*`` glob; when per-PR shared
  workspaces ship (Phase 1k++ follow-up), it changes to
  ``pr-{owner}-{repo}-{N}``.
- Test: ``TestSidecarWiring`` (+2 tests) verifies the sidecar appears
  during the attempt, carries the right PID + starttime + instance,
  and is cleaned up post-attempt.

N2 — AWAITING_CI escape event firing is now WIRED in production:
Round 1's batch D shipped ``ci_polling_exhausted`` /
``ci_flake_retries_exhausted`` in TRANSITIONS, but NO production code
emitted them. Workflows could still hang in AWAITING_CI forever.

Fix:
- New ``master/ci_poll.py``: ``run_ci_poll_exhaustion_tick`` scans
  workflows whose ``entered_state_at`` is older than
  ``CONTROLLER_AWAITING_CI_TIMEOUT_S`` (default 7200s) and fires
  ``ci_polling_exhausted`` via ``apply_event`` → STUCK + emits a
  ``ci_poll_exhausted`` controller_events row with the threshold
  payload.
- ``master/loop.py`` integrates the new tick on its own cadence
  (``ci_poll_exhaustion_interval_s`` env, default 300s). Composes
  with the existing master loop. ``MasterTickReport`` gains
  ``ci_poll_exhaustion: CIPollExhaustionReport | None``.
- Tests: ``test_master_ci_poll.py`` (+7 tests) — happy path, fresh
  workflow stays untouched, only AWAITING_CI is targeted (other
  long-lived non-terminal states ignored), event row shape pinned,
  default threshold matches the documented 2h, end-to-end loop
  integration (master_main_loop drives the exhaustion +
  workflow → STUCK without operator intervention).
- Dialect-portable SQL (Postgres interval, SQLite julianday).
- Handles SQLite returning TIMESTAMP as str from text() queries
  (no .isoformat() on str).

N3 — externally-merged/closed PRs now win over label removal:
Round-1's PAUSE-on-label-removed shipped, but reconciliation
checked the label gate BEFORE checking merged/closed. Operators
removing the opt-in label on an already-merged PR would PAUSE the
workflow forever — never transitioning to MERGED.

Fix:
- ``master/reconciliation.py:_reconcile_one`` re-ordered:
  1. Check terminal-state mappings (merged/closed) FIRST — apply
     immediately if they fire.
  2. THEN the opt-in label gate (pause/resume).
  3. Fall through to "consistent" otherwise.
- Tests: ``test_externally_merged_takes_priority_over_label_removal``
  + ``test_externally_closed_takes_priority_over_label_removal``
  pin the contract. Both seed an IMPLEMENTING workflow + Forgejo
  reporting "merged/closed AND no opt-in label" → workflow
  transitions to MERGED/ABANDONED (not PAUSED) + pre_pause_state
  stays None.

N4 — graceful handling of empty env vars:
``int(os.environ.get("CONTROLLER_FORGEJO_REQUEST_TIMEOUT_S", "30"))``
crashes with non-actionable ``int('') ValueError`` if the operator
sets the env to empty/whitespace (common when sourcing a partially-
edited /etc/cleveragents/master.env file).

Fix:
- ``master/forgejo_cfg.py:_env_int(name, default)`` — empty or
  whitespace-only values fall back to the documented default; only
  non-numeric values still raise (with a clear message naming the
  variable).
- Tests: ``test_empty_env_value_falls_back_to_default`` +
  ``test_whitespace_only_env_falls_back`` + updated
  ``test_malformed_env_raises_value_error`` to match the new
  "not a valid integer" wording.

Total: 660 controller tests pass (+13 net), 0 regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 16:11:27 -04:00
drew abff38a274 refactor(controller): batch F — ControllerForgejoConfig + parser strict mode (items 16, 17) 
Final two items from the consolidated adversarial-review punch list.

ITEM 16 — ControllerForgejoConfig:
Previously ``master/__main__.py:build_cfg_stub`` reached into
``tools/_mcp_common.ForgejoCfg`` via sys.path injection and mutated
``cfg.owner`` / ``cfg.repo`` after construction. That inverted the
dependency direction (the controller is the new system; it shouldn't
reach into legacy pipeline modules) and tied controller deployments
to whatever schema ForgejoCfg happened to have.

Replaced with ``master/forgejo_cfg.py``: a dataclass owning exactly
the fields ``_claim_runtime`` reads (token, request_timeout_s,
api_retries, claim_ttl_seconds) plus the controller's own
(owner, repo). ``from_environment(owner, repo)`` reads the same env
vars the legacy ForgejoCfg used (FORGEJO_TOKEN, CONTROLLER_FORGEJO_*
tunables) so operators don't have to relearn anything.

``build_cfg_stub`` is now a 1-line delegate to ``from_environment``;
no sys.path mutation, no cross-package import.

ITEM 17 — parser coverage validator + strict mode:
Master startup now calls ``validate_parser_coverage()`` (already
exposed by Phase 1j's parser registry) and logs the gap loudly:

  WARNING CI parser coverage: 3/10 real (7 stub: ['bandit', 'build',
  'radon', 'robot_framework', 'semgrep', 'slipcover', 'vulture']).
  Stub-parser gates fall back to raw_log_excerpt; implementers see
  the log but not structured findings.

Operators who want the strict plan-v10 "STUCK on unknown tool"
behaviour set ``CONTROLLER_STRICT_PARSER_COVERAGE=1``; master then
refuses to start (exit 2) until every EXPECTED_PARSER has a real
implementation. Default off: ship-stubs is the migration-friendly
path; strict is the after-everything-is-implemented gate.

Tests:
- test_forgejo_cfg.py (+13 tests): dataclass shape, env resolution
  (defaults, FORGEJO_TOKEN preferred over CONTROLLER_FORGEJO_TOKEN,
  malformed env raises), no-sys-path-injection AST audit + returns
  the typed dataclass.
- test_entry_points.py (+2 tests): strict-mode blocks startup +
  default mode only warns.

Total: 647 controller tests pass (+12 net), 0 regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 15:40:45 -04:00
drew d71046b9a0 fix(controller): batch D — PID-reuse, AWAITING_CI escape, flake bound, scheduler skip 
Four safety items from the consolidated adversarial-review punch list.

ITEM 8 — PID-reuse hazard in janitor:
The janitor SIGKILL'd whatever process happened to live at the
sidecar's recorded subprocess_pid. Between sidecar write and janitor
sweep, the OS can reuse the PID for an unrelated process; the janitor
was killing innocents under fork-heavy workloads.

Fix:
- ``session_sidecar.py``: added ``subprocess_starttime`` field
  (Optional[int]) + ``read_proc_starttime(pid)`` helper that reads
  ``/proc/{pid}/stat`` field 22 (clock ticks since boot — monotonic
  for a (boot, pid) pair).
- ``WorkerSession.from_dict`` filters unknown keys so forward + back
  compat with sidecars from earlier/later versions is preserved.
- ``janitor._pid_alive`` and ``_kill_with_grace`` accept
  ``expected_starttime``; on mismatch they short-circuit and DON'T
  signal the impostor.
- ``_kill_with_grace`` return semantics tightened: True iff a signal
  was actually delivered (False for "PID gone" / "PID reused"). The
  ``sessions_killed`` counter now reflects real kills.

ITEM 9 — AWAITING_CI escape from infinite poll:
Previously AWAITING_CI could only exit via ``ci_green`` /
``ci_red_*`` / ``ci_flake_retry`` — if CI hangs forever (runner
outage, broken integration, etc.) the workflow had no controller-
driven STUCK path; only operator_unstick could rescue it.

Fix: new ``ci_polling_exhausted`` event → STUCK. The master's
AWAITING_CI poll handler is the natural place to emit it once a
threshold passes (deferred to a follow-up — Phase 1k+ ships the
event in the table; the timer fires it).

ITEM 10 — ci_flake_retry was unbounded:
The ``ci_flake_retry`` self-loop on AWAITING_CI had no encoded
ceiling. Pathological flaky CI could loop forever (the docstring
said "retry once per gate" but nothing enforced it).

Fix:
- New ``workflows.ci_flake_retries_remaining`` column (server_default
  '1', default 1 — operators tune via ``CONTROLLER_CI_FLAKE_RETRIES``
  at startup or via direct UPDATE).
- New ``ci_flake_retries_exhausted`` event → ESCALATING. Master
  decrements the column on each ci_flake_retry; at 0 the next CI
  failure routes through ci_red_* (regular path) or this new
  event (escalates if the operator wants a hard ceiling).

ITEM 11 — scheduler now skips PAUSED workflows:
Without this, the scheduler could enqueue a fresh attempt for a
PAUSED workflow between two reconciliation ticks (race: label
removed at T+0, reconciliation runs at T+300, scheduler ticks at
T+30 with stale DB state). The window is at most one attempt of
worker work.

Fix: ``schedule_next_attempts`` SQL now lists only
{ANALYZING, IMPLEMENTING, REVIEWING, CONFLICT_RESOLVING, ESCALATING}
explicitly; PAUSED is excluded by absence. Reconciliation owns the
PAUSED → resume transition; scheduler doesn't touch it.

Schema additions:
- ``workflows.ci_flake_retries_remaining`` (INTEGER NOT NULL DEFAULT 1)
- ``workflows.awaiting_ci_started_at`` (TIMESTAMP NULL) — for the
  poll-exhaustion timer (timer impl deferred; column is staged).

Tests:
- TestReadProcStarttime — 3 tests (Linux skip-guard) for the
  /proc/pid/stat parser (self-pid > 0, missing pid is None,
  invalid pid is None).
- TestJanitor::test_pid_reuse_defended_via_starttime — pins the
  contract end-to-end (real subprocess + fabricated wrong starttime
  → janitor doesn't signal).
- TestPhase1kPlusTransitions — 5 tests pinning the new events +
  proving the load-bearing invariants still pass.
- test_scheduler_skips_paused_workflows — pins item 11.

Total: 603 controller tests pass (+10 net), 0 regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 15:30:45 -04:00
drew a1c6646a64 fix(controller): batch C — placeholder patching + pickup_count semantics 
Items 3 + 4 from the consolidated adversarial-review punch list.

ITEM 3 — placeholders no longer poison the audit trail:
The prefetch (master/prefetch.py) writes input_payload with
``attempt_id=0`` and ``attempt_number=1`` as placeholders because
the autoincrement PK isn't known until after INSERT. Previously
those values stayed in the DB forever — post-mortem queries against
``workflow_attempts.input_payload`` would show ``attempt_id=0``
and operators would chase ghosts.

Fix: ``master/scheduler.py:_insert_pending_attempt`` now patches both
fields with their real values:
- attempt_number: patched BEFORE the INSERT (we compute it as MAX+1).
- attempt_id: patched via a follow-up UPDATE after INSERT (we need
  the autoincrement first). One extra UPDATE per attempt; cheap
  compared to forever-incorrect audit trail.

Test: ``test_scheduler_patches_attempt_id_and_number_into_payload``
asserts the stored payload carries the real values, not the
placeholders.

ITEM 4 — pickup_count tracks REAPS, not dequeues:
Previously the dequeue path bumped ``pickup_count = pickup_count + 1``
on every successful pickup. With ``MAX_PICKUPS=3`` (default), 3
crashed-mid-attempt workers would STUCK the workflow — but that's
the wrong semantic. A worker that successfully picks an attempt
and runs it should NOT burn a pickup. Only failures (stale-heartbeat
reset by the reaper) should count toward the exhaustion limit.

Fix:
- ``db/dequeue.py`` (both postgres + sqlite paths): removed the
  ``pickup_count = pickup_count + 1`` UPDATE. Dequeue is a healthy
  pickup; doesn't bump.
- ``reaper.py``: added ``pickup_count = pickup_count + 1`` to the
  reset UPDATE. Each reap = one failed pickup.
- Docstrings updated to reflect the new semantics in both files.

Tests:
- Updated existing assertions in ``test_db_dequeue.py`` and
  ``test_reaper_and_pickup_guard.py`` to reflect: dequeue keeps
  pickup_count; reaper bumps it.
- ``TestPickupCountSemantics``: 2 new tests pin the contract end-to-end
  — N healthy dequeues stay at 0; alternating dequeue→reap→dequeue
  walks pickup_count up by 1 per reap.

Impact: a worker pool that crashes 3 times mid-attempt now needs
3 REAPS (not 3 dequeues) to STUCK the workflow. With default
TTL=600s + reaper_interval=60s, that's 30+ minutes of repeated
mid-attempt failure before STUCK — appropriately conservative.

Total: 593 controller tests pass (+3 new), 0 regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 15:24:42 -04:00
drew feeec3e9c7 fix(controller): batch B — PAUSED state for label-gate pause/resume (item 2) 
Adversarial review flagged: removing the opt-in label transitions a
live workflow to ABANDONED — but ABANDONED is TERMINAL with only
``operator_unstick`` re-entry → DISCOVERED, losing all prior
controller_events continuity. Operators removing the label to "pause"
a long-running PR will be surprised it restarted from scratch.

Fix: introduce a non-terminal ``PAUSED`` state.

State machine changes (``tools/controller/state_machine.py``):
- ``PAUSED`` added to KNOWN_STATES (non-terminal — has exits via
  ``opt_in_label_restored`` and ``operator_unstick``).
- ``opt_in_label_removed`` / ``opt_in_label_restored`` events
  documented in EVENTS but NOT listed per-state in TRANSITIONS —
  they're out-of-band master-driven events written directly by
  reconciliation. Listing them per-state breaks the
  per-state-event-set invariants (ESCALATING / CONFLICT_RESOLVING).
- ``(PAUSED, operator_unstick) → DISCOVERED`` for the escape hatch.

Schema change (``tools/controller/db/models.py``):
- ``workflows.pre_pause_state: Mapped[str | None]`` column captures
  the resume target. Master writes it on pause; clears it on resume.

Reconciliation logic (``tools/controller/master/reconciliation.py``):
- New ``_apply_transition_with_pre_pause`` helper writes both
  ``current_state`` and ``pre_pause_state`` atomically + emits the
  ``reconciliation`` event row.
- On label removal (current != PAUSED): captures pre_pause_state,
  transitions to PAUSED.
- On label restoration (current == PAUSED): reads pre_pause_state
  (fallback DISCOVERED for legacy NULL data), transitions back,
  clears pre_pause_state.
- PAUSED workflows are now SCANNED by reconciliation (not just
  non-terminals) so we can detect label-restored.

Behaviour matrix:
| current | label    | result                                   |
|---------|----------|------------------------------------------|
| any !=  | absent   | → PAUSED, pre_pause_state = current     |
| PAUSED  | present  | → pre_pause_state (or DISCOVERED)       |
| PAUSED  | absent   | stays PAUSED (no transition)            |
| any !=  | present  | regular state checks (no-op for label)  |

Tests (test_label_gate.py refactor + 3 new tests):
- test_label_removed_pauses_workflow (was: abandons)
- test_label_restored_resumes_from_pre_pause_state (new)
- test_paused_workflow_without_label_stays_paused (new)
- test_resume_fallback_when_pre_pause_state_missing (new — legacy
  data without the new column)
- Existing event-reason test still passes (reason string unchanged).

Total: 590 controller tests pass, 0 regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 15:20:38 -04:00
drew 6ba1926c52 fix(controller): batch A — datetime json, Forgejo state map, ISO compare, dequeue docs 
Four narrow bug fixes flagged by adversarial code review (items 1, 5,
6, 7 from the consolidated critique).

ITEM 1 — datetime → json.dumps crash (silent write-after-work failure):
- ``worker/runner.py:274`` and ``master/scheduler.py:283`` now pass
  ``default=str`` to ``json.dumps`` so nested datetime fields (e.g.
  CISummary.observed_at) serialize without raising.
- Before this fix: a worker would do its real work, then crash on
  the terminal-state UPDATE with TypeError, get recorded as
  ``worker-internal-error``, and the output payload would be lost.
- Test: TestDatetimeSerializationSafety in test_master_ci_summarize
  + test_scheduler_handles_datetime_in_input_payload in
  test_master_prefetch (both pin the regression — the with-default
  test passes, the without-default test asserts the TypeError so
  future maintainers see the failure mode).

ITEM 5 — Forgejo state mapping completeness:
- Extended ``_FORGEJO_STATE_TO_GATE_STATUS`` in
  ``master/ci_summarize.py`` to cover ``cancelled``, ``timed_out``,
  ``action_required``, ``queued``, ``in_progress``, ``neutral``,
  ``skipped``, ``stale`` — states observed across Forgejo / Gitea /
  GH-mirror that previously collapsed to ``pending``, telling the
  implementer "CI is still running" when really a job was cancelled.
- ``cancelled`` / ``timed_out`` / ``action_required`` / ``stale``
  now map to ``error`` (the gate failed).
- ``queued`` / ``in_progress`` stay ``pending`` (still running).
- ``neutral`` / ``skipped`` → ``passed``/``skipped`` (informational).
- Test: TestExtendedForgejoStates — 6 tests covering each new state.

ITEM 6 — lexicographic ISO comparison drops/dupes comments:
- ``master/prefetch.py:_comment_bodies_since`` and ``_iso`` replaced
  with ``_to_aware_datetime`` + datetime comparison. Forgejo emits
  ``2026-05-18T12:00:00Z``; Python's ``datetime.isoformat()`` emits
  ``2026-05-18T12:00:00+00:00`` — a string compare gives 'Z' (0x5A)
  vs '+' (0x2B) which silently misorders timestamps.
- Now parses via ``datetime.fromisoformat`` (with Z → +00:00
  rewrite), defaults naive timestamps to UTC, and compares as
  ``datetime``.
- Test: test_comments_filter_handles_z_suffix_vs_offset_form pins
  the regression.

ITEM 7 — false BEGIN IMMEDIATE claim in dequeue docstring:
- The dequeue docstring claimed ``BEGIN IMMEDIATE`` was applied by
  session_scope; it wasn't. Attempted a global ``begin``-event
  listener that conflicted with StaticPool's shared-connection
  model (test_prefetch_callback_works_in_scheduler broke).
- Reverted to a documentation fix: SQLite stays on default
  BEGIN DEFERRED (the SQLITE_BUSY retry via busy_timeout=5s is
  acceptable for single-host dev) + the docs make MULTI-MACHINE
  REQUIRES POSTGRES explicit at three call sites
  (db/session.py, db/dequeue.py, RUNBOOK.md was already updated
  in Phase 1l). Postgres has FOR UPDATE SKIP LOCKED which is what
  production actually uses.

Tests: 586 controller tests pass (+10 new), 0 regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 15:17:38 -04:00
drew 1e27039555 feat(controller): render CI raw_log_excerpt + target_url in worker prompts 
Phase 1j shipped prefetch + parsers; Phase 1i shipped the prompt
templates. But the prompt's CI-summary section only rendered each
failing gate's name, status, and summary_line — the raw_log_excerpt
(up to 16KB captured per gate) and target_url were sitting in the
input_payload dict, never surfaced to the LLM.

Impact: for stub-parser gates (7 of 10 tools — robot_framework,
bandit, semgrep, vulture, radon, slipcover, build), the implementer
saw "parser pending; see raw_log_excerpt" with no log in the prompt.
It would have to shell out via OpenCode to re-fetch the log, which
is wasteful and a regression vs the existing pipeline.

This commit:
- Renders ``target_url`` as ``log: <url>`` for failing gates so the
  LLM can curl it if needed.
- Renders ``raw_log_excerpt`` inside a fenced code block, truncated
  to 3KB per gate (storage cap is 16KB; trim to prompt-friendly
  size with a marker preserving the original byte count).
- Renders ``composite_findings`` as nested sub-findings (security_scan
  → bandit head + semgrep/vulture children, each with their own
  excerpt) so multi-tool gates are first-class in the prompt.
- Skips the fence + URL for passing gates (already filtered) and
  for failures with no excerpt (defensive — keeps clean parsers
  from emitting empty log blocks).

Conversion: the 20% of CI failures handled by stub parsers go from
"broken — implementer doesn't see the log" to "less structured —
implementer sees the raw log and can reason about it." Real parsers
still ship in priority order as a follow-up.

Tests (+7 in TestCIFailureRendering, 0 regressions):
- target_url rendered for failing gate
- target_url skipped for passing gate
- raw_log_excerpt rendered in markdown code fence
- Long log excerpt truncated with marker
- Missing excerpt → no log block (no empty fence)
- composite_findings render as sub-findings with their own excerpts
- Stub-parser-pending failure now surfaces the actual log (the
  whole point of the commit)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 15:00:28 -04:00
drew a19b609554 feat(controller): Phase 1l — systemd units + ops runbook 
Deployment surface for the controller. Operators copy the unit files
+ env examples to /etc/, customize, and `systemctl enable --now`.

Layout (under tools/controller/deploy/):
- systemd/cleveragents-controller-master.service — master singleton.
  One unit per (owner, repo); plan v9 singleton constraint enforced
  by deployment.
- systemd/cleveragents-controller-worker@.service — worker template.
  `systemctl enable cleveragents-controller-worker@implementer-1`
  spins up one instance; scale by adding instances. Per-instance
  env override at /etc/cleveragents/worker.<inst>.env (optional)
  specialises CLEVERAGENTS_WORKER_ROLES per instance.
- systemd/master.env.example — every env var the master reads,
  documented. Copy to /etc/cleveragents/master.env, mode 0640.
- systemd/worker.env.example — every env var the workers read.
- RUNBOOK.md — operator guide:
  * Prereqs (Linux + systemd 245+, Python 3.13 + uv, Postgres 14+,
    OpenCode server, dedicated cleveragents user).
  * First-time setup (7 ordered steps from useradd to first PR).
  * Day-to-day ops (where logs live, healthy queries, role pool
    sizing, label-based pause, clean restart).
  * Incident response (6 named scenarios: STUCK workflows, MERGING
    retry exhaustion, no-OpenCode, dequeue starvation, DB loss,
    operator-unstick procedure).
  * Migration playbook (Phase 2: opt one PR in via label,
    monitor controller_events, escalate to full management).
  * Tunables cheat sheet covering all 8 env vars + their tradeoffs.
- README.md — index linking the above.

Hardening on both units: NoNewPrivileges, PrivateTmp,
ProtectSystem=strict, ProtectHome, narrow ReadWritePaths, kernel
+ control-group protections. RestartPreventExitStatus=2 prevents
systemd loop-restart on misconfiguration (exit 2 = bad env).

No code changes; verified via `systemd-analyze verify` (unit syntax
parses; only "venv path doesn't exist on dev host" warnings, which
are expected). Test suite: 569/569 pass, 0 regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-18 14:49:00 -04:00