feat(sessions): watch CI on published merge requests and bound the fix attempts - #1529
Open
srtab wants to merge 21 commits into
Open
feat(sessions): watch CI on published merge requests and bound the fix attempts#1529srtab wants to merge 21 commits into
srtab wants to merge 21 commits into
Conversation
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.
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
run_job_taskpoints a watch at the MR the run just published (state lives onSession: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.pipeline_events, GitHubworkflow_run) enqueueevaluate_pipeline_watch_taskon theinteractivequeue.failedis actionable;blocked/manual/skipped/canceledand 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-projectincluderesolves as the pushing identity, and an ephemeral per-project bot can't read it.exhausted, comments on the MR, and notifies once.unclear.Loop safety
The feature spends real money per attempt, so the bound is the design's centre of gravity.
watch_attemptsis 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_iddedupes 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(defaulttrue) andpipeline_watch_max_attempts(default3, min 1), site-wide under Configuration → Pipeline Watch, overridable per repository in.daiv.yml: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
python manage.py setup_webhooks --update.setup_webhooksskips hooks that already exist without--update, so without this the feature is inert on every already-onboarded repository.Both checklists in
docs/getting-started/platform-setup.mdare updated.Notes for review
accept_callbackon 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_acceptedguards this; loop protection comes from the attempt counter, never from identity.Job.status'sLiteralgainedpreparing,waiting_for_resource,cancelingandwaiting_for_callback— this is the first code to populateJobfrom the GitLab jobs API, so the narrower vocabulary became reachable.continue-on-errorfailure counts as real. Conservative direction — can cost a wasted attempt, never misses a failure.core/0016,sessions/0007,notifications/0008.sessions/0007rebuilds both check constraints keyed onSessionOrigin.values(session_origin_validandrun_trigger_type_valid).Known gaps, deliberately not in this PR
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.watch_pipeline_id, so the next poll dedupes and returns. One-line fix, follow-up.enqueuein_adispatch_fix_runis unguarded, unlikeasubmit_batch_runs; a broker failure spends the attempt and leaves an unreapedREADYRun row.Verification
make test— 4699 passed, 90.22% coverageruff check+format --checkclean;mkdocs build --strictcleanmake lint-typing— 463 diagnostics, all pre-existing Django field-descriptor false positives, no new error class