Skip to content

feat(sessions): watch CI on published merge requests and bound the fix attempts - #1529

Open
srtab wants to merge 21 commits into
mainfrom
claude/cool-carson-edd8db
Open

feat(sessions): watch CI on published merge requests and bound the fix attempts#1529
srtab wants to merge 21 commits into
mainfrom
claude/cool-carson-edd8db

Conversation

@srtab

@srtab srtab commented Aug 25, 2026

Copy link
Copy Markdown
Owner

After DAIV publishes a merge request, it now watches that MR's CI and spends a bounded number of agent runs trying to make it green, then hands back to a human with a diagnosis.

Implements docs/superpowers/specs/2026-08-25-pipeline-watch-design.md.

How it works

  • Arm — the post-run block of run_job_task points a watch at the MR the run just published (state lives on Session: watch_state, watch_attempts, watch_pipeline_id, watch_armed_at) and enqueues an immediate evaluation, because the push already happened during publish so the CI event can arrive before the watch exists.
  • React — new CI webhooks on both platforms (GitLab pipeline_events, GitHub workflow_run) enqueue evaluate_pipeline_watch_task on the interactive queue.
  • Judge — a pure function classifies a finished pipeline GREEN / ACTIONABLE / UNCLEAR. Deliberately conservative: only an unambiguous failed is actionable; blocked/manual/skipped/canceled and zero-job pipelines are UNCLEAR, which stops the watch with an explanatory MR note and spends no attempts. The zero-job case is real — a private cross-project include resolves as the pushing identity, and an ephemeral per-project bot can't read it.
  • Fix — an actionable failure dispatches an agent run on the MR's own thread and branch, with a small prompt naming the failed jobs and pipeline URL (no embedded logs; the agent has trace tools). At the cap the watch goes exhausted, comments on the MR, and notifies once.
  • Reconcile — a cron sweep every 10 minutes catches missed events and stuck states; a watch idle past 6h goes unclear.

Loop safety

The feature spends real money per attempt, so the bound is the design's centre of gravity. watch_attempts is incremented at dispatch behind a primary-key compare-and-swap (so a losing racer cannot dispatch), never reset when a fix run re-arms, and the cap is read per dispatch from repo config clamped by site settings. watch_pipeline_id dedupes repeat events on both the webhook and poll paths, and a fix run that produces no diff ends the watch immediately — no diff means no push, so no pipeline and no event would ever arrive.

Configuration

pipeline_watch_enabled (default true) and pipeline_watch_max_attempts (default 3, min 1), site-wide under Configuration → Pipeline Watch, overridable per repository in .daiv.yml:

pipeline_watch:
  enabled: true
  max_attempts: 3

A repository can tighten or disable; the operator holds the master switch (both are clamped at every consumption site).

Rollout — required, and it differs per platform

  • GitLab: python manage.py setup_webhooks --update. setup_webhooks skips hooks that already exist without --update, so without this the feature is inert on every already-onboarded repository.
  • GitHub: subscribe the GitHub App to Workflow run. The management command returns early on GitHub, so it is not the GitHub path.

Both checklists in docs/getting-started/platform-setup.md are updated.

Notes for review

  • accept_callback on both new callbacks deliberately does not reject events attributed to DAIV, unlike every other callback here. On repos using project-scoped ephemeral push tokens the publisher pushes with [skip ci] and heals CI as the service account, so the pipeline most worth watching is attributed to DAIV itself. test_a_pipeline_daiv_triggered_is_still_accepted guards this; loop protection comes from the attempt counter, never from identity.
  • Job.status's Literal gained preparing, waiting_for_resource, canceling and waiting_for_callback — this is the first code to populate Job from the GitLab jobs API, so the narrower vocabulary became reachable.
  • The GitHub Actions API doesn't expose per-job required-ness, so that client treats every job as required: a continue-on-error failure counts as real. Conservative direction — can cost a wasted attempt, never misses a failure.
  • Three migrations: core/0016, sessions/0007, notifications/0008. sessions/0007 rebuilds both check constraints keyed on SessionOrigin.values (session_origin_valid and run_trigger_type_valid).

Known gaps, deliberately not in this PR

  • The watch arms only in run_job_task, so it covers API/MCP/UI jobs, scheduled runs and chat — but not issue-webhook or MR-comment-webhook publishes, which drive their managers directly. The spec chose this arm site deliberately, but its Scope section's caveat about issue-originated MRs implies broader reach. Widening it is new design work.
  • A dead fix run currently ages out silently at 6h rather than being re-judged: the reconciler's un-stick sweep doesn't clear watch_pipeline_id, so the next poll dedupes and returns. One-line fix, follow-up.
  • The enqueue in _adispatch_fix_run is unguarded, unlike asubmit_batch_runs; a broker failure spends the attempt and leaves an unreaped READY Run row.
  • The site master switch doesn't drain already-armed watches — webhooks stop at once, but the poll path can still spend the remaining cap per armed MR until the 6h expiry.

Verification

  • make test — 4699 passed, 90.22% coverage
  • ruff check + format --check clean; mkdocs build --strict clean
  • make lint-typing — 463 diagnostics, all pre-existing Django field-descriptor false positives, no new error class
  • All three migrations apply cleanly to a fresh database

srtab added 17 commits August 25, 2026 16:38
Move sessions.pipeline_watch's acreate_run import into the lazy block in
_adispatch_fix_run that already defers run_job_task for this reason. That
was the edge closing the jobs.tasks -> sessions.pipeline_watch ->
sessions.services -> jobs.tasks cycle.

With the cycle gone, jobs.tasks imports aarm_watch, aexhaust_watch and
evaluate_pipeline_watch_task plainly at module level, replacing a module
__getattr__ hook and three noqa: F821 suppressions that had switched off
undefined-name checking on the call sites.
Add docs/features/pipeline-watch.md covering what the feature does, its
conservative judgment logic (including why zero-job and blocked/manual
pipelines stop the watch), settings, per-repo config, the GitHub
allow_failure caveat, non-goals, and the required rollout step.

Add Pipeline Watch to mkdocs.yml nav and the repository-config reference.

Add the changelog entry under Unreleased → Added with the rollout note.

Fix test_only_short_work_shares_the_queue_the_titler_runs_on: the
evaluate_pipeline_watch_task added by this plan runs on the interactive
queue (short, user-visible) but was missing from INTERACTIVE_TASKS.
…the loop

The arm-time evaluation polls seconds after the publish push, so it read a
pipeline that was still `created`/`pending`/`running` — or absent. `judge_pipeline`
answers UNCLEAR there, which was a terminal branch: it set `watch_state=unclear`
and commented on the MR. Every watch DAIV armed closed itself, on every published
merge request. `aevaluate_watch` now judges only a pipeline that has reached a
verdict, and leaves anything in progress armed for the next event or sweep; the
six-hour expiry is the backstop the design already had for a pipeline that never
starts. `blocked`/`manual` stay judgeable — nothing but a person resolves those.

`_adispatch_fix_run` enqueued `run_job_task` with no `run_id` and created the Run
afterwards, so the task's `trigger_type` read came back empty, `was_fix_run` was
False and every re-arm reset `watch_attempts` to 0. Nothing bounded the loop.
Reordered to the create-then-enqueue the other two dispatchers use, with the row
created READY (QUEUED means "not yet enqueued" to two recovery sweeps, both of
which would have enqueued a second task for one attempt).

Also on the evaluate path: the poll now correlates the pipeline it read against
the merge request's head sha (a stale terminal pipeline otherwise closed the
watch green or spent an attempt twice) and against `watch_pipeline_id`, which the
`pipeline_id is None` path skipped entirely; the FIXING transition is a
compare-and-swap on WATCHING with `F("watch_attempts") + 1`, so two events
finishing together cannot both dispatch off the same stale read; it restamps
`watch_armed_at`, without which a repo whose CI takes over 30 minutes had every
fix run declared stale before it started; the fix run resolves a sandbox
environment like both MR-comment callbacks do; and the site switch is now a
ceiling over the per-repo one, as the docs already promised.
`Job.status`'s Literal was narrower than the GitLab jobs API, and this branch is
the first code to populate `Job` from it: a pipeline containing a `preparing`,
`canceling`, `waiting_for_resource` or `waiting_for_callback` job raised a
ValidationError out of `get_pipeline` and failed the evaluation task. All four are
documented job statuses; adding values is backward compatible, since `is_failed()`
only tests `== "failed"`.

Both pipeline callbacks now gate on `watch_enabled`, so a repo cannot enable a
watch the operator turned off site-wide. Drops the `project` argument
`_to_pipeline` never used.
…udgment table

`setup_webhooks` returns immediately on GitHub — an App's event subscriptions are
centralized, not per repository — so the page's single "run `setup_webhooks
--update`" instruction was a no-op on every GitHub install, and the feature stayed
inert with nothing to say so. The rollout section and the changelog note now give
GitLab the command and GitHub the App settings change, and both operator event
checklists in platform-setup gain **Workflow run** (the GitLab verification list
was also missing **Pipeline events**).

The judgment table contradicted itself on `success | any | Green`, which the
zero-jobs row overrides, and omitted the surprising `failed`-with-no-failed-job
row. Rewritten to match `judge_pipeline` in order, including the in-progress
pipeline that is now not judged at all. Adds the two platform caveats a reader
cannot infer: a GitHub `workflow_run` event is one workflow rather than the
branch's whole CI, and the reconciler's branch-ref poll does not see GitLab
detached merge-request pipelines.

Also lists the page under the llmstxt Features section, where it was the only
`nav` entry missing.
The watch only armed from run_job_task, so merge requests published by
issue addressing or by a chat turn were never babysat. Move the arm into
sessions.pipeline_watch as aarm_watch_after_run and call it from all
three seams; review addressing stays excluded on purpose, since it
pushes to a merge request someone else may own.

Arming now keys on a new GitState.published rather than code_changes,
which stays true for a clean tree already on its merge request and so
cannot answer "did this turn push?". A fix run that pushed nothing ends
the watch instead of stranding it until it ages out.

Webhooks gate on the judge's own status vocabulary and on an existence
check for an open watch, so an unrelated pipeline costs one indexed
SELECT rather than an interactive-queue round-trip. Pin watch_state at
the DB layer and add the partial index the reconciler's repo-less sweeps
need.

Cleanups found by review of the above:

- Hoist _safe_get_state to BaseManager; the issue addressor's unguarded
  aget_state could discard a run whose merge request had already
  published, and its `if snapshot else` guards were unreachable.
- amark_failed_and_advance takes a log_context, so a pipeline-watch
  enqueue failure no longer logs under submit_batch_runs.
- arequest_watch_evaluation replaces the gate-then-enqueue block that
  was copy-pasted into both platform callbacks.
- judge_pipeline walks the job list once; failed_job_names takes the
  fallback its three callers each spelled out.
- Reuse WatchState.open() for the index condition, DAIV_DIR in tests,
  and a shared amake_watched_session row builder.
- Drop select_related("user") from aevaluate_watch: it is read on two of
  eight branches and both helpers keep their own read.
srtab and others added 4 commits August 26, 2026 16:03
…ost owners

Follow-up pass over the pipeline watch, covering the failure modes the
preceding commits left open:

- Every watch_state write goes through one compare-and-swap helper
  (_atransition), including the reconciler's own transitions and the
  no-diff exhaust. Two events finishing together both pass their checks
  off their own stale read, and only the row count says which one owns
  the transition — without it each posts its own MR comment, which no
  constraint dedupes.
- Correlate webhook pipelines against the MR head too, not just polled
  ones: a webhook names which pipeline it reports, not whether that
  pipeline is still the head, and GitLab auto-cancels redundant
  pipelines so the push after ours makes the older one emit a terminal
  canceled.
- get_pipeline no longer answers None on 401/403/5xx on either platform,
  which made an outage read as "no pipeline yet" — something the watch
  waits out in silence. Only a 404 is absence; anything else propagates,
  leaves the watch armed, and is retried by the next event or sweep.
- Platform failures log at WARNING without a traceback when transient
  (is_transient_platform_error), so an hours-long outage does not mint
  one Sentry error per sweep — the same split is_transient_bus_error and
  _is_transient_mcp_error draw.
- aarm_watch attributes the MR thread it creates to the originating run's
  owner without ever reassigning an existing one: the thread may be a
  human's MR conversation.
- The give-up notification moves to notifications/watch_notifiers.py,
  deduped per (thread, pipeline) rather than per thread — a thread
  outlives the merge request and a watch can be re-armed with a fresh
  budget. Adds the RocketChat renderer the event was missing.
- The reconciler's 6-hour give-up posts a note and logs a warning instead
  of closing the watch silently: it is where every unresolved failure
  lands, and closing it silently made those indistinguishable.
- Site settings coerce every IntegerField width, not a roster of three,
  so PositiveSmallIntegerField no longer returns the raw string.
- One platform client per evaluation, and select_related on the owner the
  fix-run dispatch reads.
fix(sessions): harden the pipeline watch against races, outages and lost owners
Four textual conflicts, all additive on both sides:

- sessions/models.py — keep main's `external_refs` alongside the branch's
  `watch_*` columns. The branch's migration is renumbered 0008 and re-parented
  onto main's `0007_session_external_refs`, so the graph has one leaf again.
- codebase/managers/issue_addressor.py — both sides added a post-run checkpoint
  read. Merged into one `_safe_get_state` (the branch's tolerant reader, which
  `_build_agent_result` already accepts `None` from), feeding main's
  `apersist_session_ref` and then the branch's `aarm_watch_after_run` — the
  order `jobs/tasks.py` auto-merged into.
- codebase/clients/gitlab/client.py — keep both new methods:
  main's `_await_merge_request_head_ref` and the branch's pipeline readers.
- tests/.../test_git.py — keep both new test groups.

Two semantic breaks the textual merge hid:

- sessions/services.py — main's new refs-merge error path called
  `_mark_failed_and_advance`, which this branch renamed to
  `amark_failed_and_advance`.
- codebase/clients/base.py — both sides added an import in the same
  alphabetical run; `Pipeline` was dropped when `TriggeredPipeline` won the
  hunk.

4949 unit tests pass, ruff clean, `makemigrations --check` detects no drift,
mkdocs --strict builds.
The remote branch had moved on while main was being merged in: PR #1534
("harden the pipeline watch against races, outages and lost owners"). Two
textual conflicts, both sides kept:

- notifications/run_notifiers.py — the two sides changed adjacent lines for
  unrelated reasons. Keep the remote's `enabled_channel_types()` helper *and*
  main's `BatchRow.from_values(...)` over `values(*BatchRow.COLUMNS)`, which
  adds the `envelope__summary` column; `from_values` is keyed by column name
  because the positional splat it replaces would silently swap `status` and
  `summary`.
- tests/.../test_rocketchat_renderers.py — two independent test classes
  appended at the same point.

One semantic break the textual merge hid:

- tests/.../test_watch_state.py — the remote side's new index-literals test
  imports the pipeline-watch migration by module name, which the previous
  commit renumbered to 0008 to clear main's own 0007.

5001 unit tests pass, ruff clean, `makemigrations --check` detects no drift
(sessions at 0008, notifications at 0009), mkdocs --strict builds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant