Skip to content

feat(manager): keep the manager alive until its final post - #695

Merged
TheGreatAxios merged 18 commits into
mainfrom
cl-11611-keep-the-manager-alive-until-its-final-post
Oct 28, 2026
Merged

TheGreatAxios merged 18 commits into
mainfrom
cl-11611-keep-the-manager-alive-until-its-final-post

Conversation

@TheGreatAxios

@TheGreatAxios TheGreatAxios commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

A pull request's manager now stays alive until its head has its final verdict, and the hub no longer holds heads for CI. While the manager waits, the one triage comment shows the review as in progress.

Why the second pass on #688 posted nothing

hostPending in render.ts counted every machine rule that could not be checked as waiting on the host. That included code patterns on a patch Triage could not read in full, which never resolves. So on the pass after CI finished, the judge was held back and the verdict came out as waiting. The mirror wrote only the waiting marker, which it skips while Needs Human Review is on the pull request, and a waiting verdict records no row. The manager polled once, found nothing and ended, and no later event arrived. The first post only happened because the reconciler's last retry carried noWait. Prod logs (take, done, take, done with no deployment) and the pull request's timeline show this sequence.

What changed

  • Root cause fix: whether a head waits on the host now comes from its facts, not from rule results. The rule-result inference (hostPending, waitsOnHost) is deleted. A head waits when its CI is still running (checks: "pending") and the policy reads CI anywhere: CI green, or a step's CI condition, approve or merge. It also waits when mergeability is not computed and no-merge-conflicts is on. Otherwise it never waits on CI. A rule Triage could not check itself, such as code patterns on an unread patch, neither waits nor stops the wait. The judge, done and the in-progress comment all read the same facts (hostAwaited).
  • Manager lifetime: done is asked to wait while the head waits for CI, or while a judge or code review it mailed has not reported. The manager ends once the head has its final verdict and the actions ran or were held. A check finishing, a reviewer's result or a new head reaches the live manager, and a new head starts the pass over.
  • CI waiting: the manager reads a waiting head again every 5 minutes without an event. This covers Bitbucket builds, which send no webhook, and a GitHub check_run that never arrives. After 60 minutes it decides with noWait. Its passes run out after 3 hours.
  • CI start grace: where CI green is on, a head with no checks within 2 minutes of its first pass waits for CI to start. The pass at 2 minutes reads it again and decides it as having no CI.
  • In-progress comment: a pass that ends waiting writes the one triage comment as Triage: In progress, naming what it waits for (CI, the decision model, code review). It uses the existing comment writer under an effect id per head and per set of things awaited. The comment is rewritten only when that set changes, and the verdict edits it in place. The in-progress comment keeps the header of the verdict it replaced, so the next verdict still notes what changed since that verdict.
  • Code review timing: a pass that mails the judge also deploys code review when the block runs whatever the rules say (when.rules: any) and its when names every verdict. Under rules: pass the judge's rules could still shut the gate, so review waits for the verdict. A review that starts while CI is awaited reports the CI wait too, so the 60-minute timer keeps running and the comment names both. The review is still posted with the final verdict.
  • Lifetime: a manager lives 3 hours from its claim, measured as a clock rather than a pass count, and a new head does not extend it. The pass after that decides without what it still awaited. Its verdict says what Triage stopped waiting for and holds every Do for a person.
  • Reconciler slots: a manager waiting for CI holds no in-flight slot. Only managers that are judging or reviewing count toward the 5.
  • Comment: the "changed since" note is carried in the verdict header, so a rerun on the same head keeps it, including a rerun through the in-progress comment. The host leaves an in-progress comment unchanged when it already says the same thing.
  • Hub: the hub records on the claim whether each pass waited for CI. Take answers with waitingSince and headSince. The portal's Waiting for CI now comes from the live manager.
  • CI hold removed: the coalescer's hold (triage_waiting_ci, the waiting triage-state status and its events) and the kind: "waiting" mail are removed. Every window goes to the listener when it closes.
  • CI holds as a fact: a Do held or skipped because of CI carries the head's CI state (ci: "pending" | "failure" | "cancelled" | "none") instead of being recognised from its reason text. CI_REASONS, PENDING_CI and CI_NOT_PASSED are deleted. The comment's "Waiting for checks to finish" line groups by ci === "pending". A Do held for a person, or one Triage cannot run itself, carries no ci. The verdict records only ci, and the portal works out "will run when CI finishes" (heldUntil) from ci === "pending".
  • Stored judge verdicts: a stored judge verdict that is posted again keeps the judge's answer, and its Dos are made again from the current pass's facts (verdictOnFacts). So CI that turns green on the same commit lets an approve or merge run without running the judge again. decided() no longer reads actions.
  • Stopped manager: a manager whose 3 hours ran out deploys no judge. It decides on the machine rules, with every Do held and the stopped note.
  • What the host is awaited for: hostAwaited returns a fact (ci-starting, ci-pending, mergeability), and render.ts turns it into text once, from the CI rule's own reasons in checks.ts.
  • Docs: ARCHITECTURE (coalescer, reconciler, manager lifetime, in-progress comment, and that a waiting manager holds its sidecar slot for up to 3 hours), PRODUCT (In progress, Waiting for CI), the user manual, BITBUCKET_DATACENTER and the manager README.

How it was verified

All results below are on the rebased head.

  • bun run test:unit: 1454 pass, 0 fail. The manager tests cover:
    • a final verdict ends the manager;
    • a head with running CI gets the Waiting label and the in-progress comment, and the manager polls on;
    • the 2-minute CI start grace;
    • the 5-minute re-read;
    • the 60-minute noWait;
    • code review deployed with the judge, with the in-progress comment naming both, then only code review;
    • the in-progress comment keeping the earlier verdict's header;
    • the feat: make the policy file the one source of what Triage may do #688 case, where code patterns on an unread patch no longer hold the judge;
    • a review deployed while CI runs, which keeps the CI wait and names both in the comment;
    • rules: pass review not deploying with the judge;
    • the 3-hour stop, which decides, says what it stopped waiting for and holds every Do;
    • the 3-hour stop while CI runs on a judge policy, which deploys no judge and holds every Do;
    • CI must pass off with an approve step, CI red then green on the same commit: the approve runs on the green pass and the judge is not run again;
    • a take with no head.
  • pr-triage tests:
    • a policy that reads CI waits while CI runs;
    • a policy that reads no CI decides at once, and its Dos wait only for running CI;
    • CI holds carry the ci state;
    • the changed note survives a same-head rerun.
  • Portal test: a held Do with ci: "pending" reads as "will run when CI finishes", and one with ci: "failure" does not.
  • bun run test:postgres: 154 pass, 1 fail. The failure is the email verification test in server.postgres.test.ts, which this PR does not change; that file passes 11 of 11 when run alone. The hub suite on its own passes 443 of 443. Both runs that failed were made while another agent ran Postgres shards on the same local database.
  • bun run test:dom: 11 pass.
  • bun run test:chain: 13 pass, 0 fail, across 11 files. The chain covers:
    • CI finishing after the first post (waiting.test.ts): one manager shows Waiting with the in-progress comment, judges once check_run completes, posts the verdict into the same comment and ends;
    • CI finishing with no event (reread.test.ts): the waiting manager reads the head again within 5 minutes, and the verdict replaces the in-progress comment;
    • a judge reporting back (woken.test.ts, judged.test.ts): the manager waits for its judge, and the verdict replaces the in-progress comment.
  • bun run lint: 22 warnings, the same as on main.
  • tsc --noEmit: 0 errors on triage-workflows, triage-manager, hub, web and tests/chain.

Accepted

  • The manager is held to the local sidecar cap and sandbox overflow (CL-11587). A waiting manager holds its slot for up to 3 hours. ARCHITECTURE says what happens at the cap.

  • Waiting heads are re-read every 5 minutes.

  • Rebased onto feat(web): say in one line what Triage is waiting on or needs from you #693: a manager waiting for CI shows Waiting on CI ahead of awaiting its reviewers.

  • Code review is not covered by the chain test. The chain has no isolated provisioner and no review model, so code review deployed with the judge is covered by the manager tests.

Closes CL-11611

@linear-code

linear-code Bot commented Oct 28, 2026

Copy link
Copy Markdown

CL-11611

@TheGreatAxios

Copy link
Copy Markdown
Contributor Author

Keeps one manager per pull request alive until its final post: it waits for CI, the judge and code review itself, shows the one triage comment as In progress, and the hub's CI hold is gone.

Ran locally on 8bb1357: lint (no new warnings), manager/workflows/contracts tests 615 pass, hub tests with Postgres 440 pass, bun run test:chain 13 pass. CI check still pending.

Findings:

  • packages/triage-workflows/src/logic/render.ts:824: when CI is pending and another rule is unconfirmed (for example code patterns on a patch that was not read in full), a rules-only policy gives a final Unsure at once, posts it, runs its actions and ends the manager. CI is never waited for. The cause is waitsOnHost, which needs every unconfirmed rule to be a host check. Until now the hub held these heads before triage, so this path did not come up. Reproduced with renderVerdict (ci-green and conventions both could-not-check: unsure, waiting undefined; ci-green alone: waiting: true). This is the feat: make the policy file the one source of what Triage may do #688 fix overshooting: an unread patch no longer holds the judge, but it now stops the CI wait as well. It should wait when some unconfirmed rule is the host's.
  • apps/hub/src/github/triage-reconciler.ts:93 with packages/triage-workflows/src/logic/actions.ts:280: removing the hold dropped the dependsOnCi case where CI green is off but a step approves, merges or has a ci condition. On pending CI those Dos are held or skipped and the verdict is final, so the manager ends. On Bitbucket nothing comes back for them: there is no build webhook, and waitsOnCi only polls verdicts that carry a ci-green check. On GitHub they depend on the check_run webhook arriving. The manual still says "They go ahead once the checks are done" (docs/Corbits Triage User Manual/Corbits Triage User Manual.md:370). Found by reading the code, not reproduced end to end. Needs a call: the manager waits for CI whenever the policy reads CI, or the reconciler polls verdicts with Dos held on CI.
  • packages/triage-contracts/src/triage-comment.ts:127: on a same-head rerun through the in-progress comment (Review again, or a judge sent again on the same head), the "Changed since the last verdict" note is lost. withChangedNote copies the note from the current body, which is now the in-progress text. Reproduced with a fake host: H1 verdict, then H2 verdict (note present), then in-progress, then H2 verdict again (note gone).
  • packages/triage-contracts/src/check-pack.ts:1381: dependsOnCi has no callers left. It is a dead export.
  • apps/hub/src/github/reconcile-plan.ts:218 / :64: a manager waiting for CI is observed as running, so its head now counts against maxInFlight (default 5) for up to an hour. Before, a waiting row did not count. Five pull requests waiting for CI stop the reconciler from queuing never-triaged, failed or base-moved heads in that tenant. Found by reading the code.
  • docs/ARCHITECTURE.md:158 with packages/triage-manager/src/triage-manager.ts:16: each waiting manager holds one of the local sidecar's TRIAGE_SIDECAR_MAX_DEPLOYMENTS (16) slots for up to 3 hours, and its judge and code review take two more. A backlog mail (one manager per open head), or a push wave while CI runs, can fill the local cap. Without a sandbox, judge and code-review placements for those same pull requests then fail after about 2.5 minutes and degrade. The PR accepts the 3-hour hold, but it does not say what happens at the cap. Not reproduced.
  • packages/triage-manager/src/triage-manager.ts:60 / packages/triage-manager/src/actions/manager.ts:174: with when.rules: any, code review can deploy while the verdict waits for CI. doneReviewing sends no cleared, so its report is { head } without waiting. That closes the pass as not waiting and resets waitingSince, which restarts the 60-minute decide-anyway timer. The in-progress comment also names only code review, not CI. Found by reading the code.
  • packages/triage-manager/src/actions/review.ts:143: reviewEarly checks the rules gate on the machine rules only. If the judge then fails a model rule, the gate the policy asked for (when.rules: pass) shuts, but the review has already run. awaiting keeps the manager alive until that unposted review reports or hits its deadline. Found by reading the code.
  • packages/triage-manager/src/triage-manager.ts:16: the 3-hour limit is an iteration count (PASSES), not a time. Passes driven by events skip the 30-second poll and still use up iterations, and a new head late in the run gets only what is left. When passes run out, stop ends the manager with the In progress comment still up. Recovery then waits for the next event or for the reconciler's run-lost path. Found by reading the code.
  • packages/triage-manager/src/actions/progress.ts:16, docs/ARCHITECTURE.md:347: "written once per set" / "rewritten only when that set changes" is not true. The stock effect key includes the run, the step and the input, and take output differs on every pass. Every pass that reaches a progress step edits the comment again with the same body. Harmless, but the docs are wrong.
  • Overlap with feat(web): say in one line what Triage is waiting on or needs from you #693: apps/hub/src/github/manager-runs.ts:39, ManagedPull.live in apps/hub/src/manager-state.ts, runOf in apps/hub/src/github/triage-verdicts.ts:117, ARCHITECTURE and the manual all conflict with feat(web): say in one line what Triage is waiting on or needs from you #693's awaiting field and run: awaiting state. Whichever merges second has to combine them, and waiting-ci should win over awaiting.

Not ready: the first two findings change when a verdict is final.

@TheGreatAxios

Copy link
Copy Markdown
Contributor Author

Re-review at a31f711. The manager now reads its wait from the head's facts (hostAwaited), waits whenever the policy reads CI, runs on a 3-hour clock counted from its claim, and holds a Do on CI as a ci fact rather than as reason text.

Ran locally: lint (22 warnings, same as before, none new); typecheck not run (bun check loops on Bun 1.4.2 and the repo has no tsc); manager/workflows/contracts/host-tool tests 681 pass, hub tests with Postgres 441 pass. I did not run the chain test. The builder's one timeout followed by two 13/13 runs is an unverified flake. CI has not run on this head: only cla reported, because the branch conflicts with main.

Most of the earlier findings are addressed. The following remain or are new:

  • Does not merge with main. git merge-tree against origin/main reports 8 conflicts:

    • apps/hub/src/github/manager-runs.ts
    • apps/hub/src/github/reconcile-plan.ts
    • apps/hub/src/github/triage-verdicts.ts
    • apps/hub/src/manager-state.ts
    • packages/triage-contracts/src/hub-responses.ts
    • packages/triage-workflows/src/logic/actions.ts
    • packages/triage-workflows/src/logic/actions.test.ts
    • docs/Corbits Triage User Manual/Corbits Triage User Manual.md

    The actions.ts conflict is a semantic one: feat(web): say in one line what Triage is waiting on or needs from you #693's heldUntil: "ci-finishes" and this PR's ci: <state> are two facts for the same thing. Keep one of them. The ci state is the richer of the two, and the portal's heldUntil can be derived from ci === "pending".

  • packages/triage-workflows/src/logic/settled.ts:76: the re-judge change is not safe when CI later goes green in one case. decided() now treats a judge verdict as lasting when its Do was skipped or held on ci: failure or cancelled.

    • Case: CI green off, a step that approves or merges, CI red, then re-run green on the same head.
    • What happens: the check event reaches the manager, and verdict() (packages/triage-manager/src/actions/judge.ts:33) posts the stored judge verdict as it was made, with the Do still skipped or held on the old CI state. Nothing re-evaluates the Dos against the new facts, so the approve or merge never runs. Before this change, CI_REASONS kept that verdict undecided, so the judge ran again.
    • Verified: a probe gives decided() true for an approve skipped with ci: "failure", and false for one held with ci: "pending". The replay path was found by reading, not reproduced end to end.
    • Fix: either re-judge on any ci other than success, or re-evaluate the Dos from the pass's facts on a stored verdict. The second keeps the judge from running again and fits "facts carried forward".
  • packages/triage-manager/src/actions/manager.ts:270 with packages/triage-manager/src/actions/judge.ts:70: when the 3-hour clock runs out while a head is still waiting for CI on a policy that needs the judge, stopped forces noWait.

    • What happens: judgeScope then sees nothing to wait for and deploys the judge. That pass posts nothing, and awaiting returns false because stopped is set. The manager ends with a judge out and no verdict up. The judge's result later wakes a new manager with a fresh claim, which posts with Dos running on their own. This bypasses the "every Do held for a person" rule for stopped verdicts.
    • When it happens: reachable when a head's CI wait starts more than 2 hours into a claim. Found by reading.
    • Fix: judgeScope should not deploy on a stopped pass, so it decides on the machine rules with the stopped note.
  • packages/triage-workflows/src/logic/render.ts:551 and packages/triage-workflows/src/logic/checks.ts:169: the two "checks have not started yet" strings are duplicate literals, and hostAwaited returns the reason text rather than a fact. Nothing infers from it today. The waiting reason could come from the CI rule's own reason, so there is one source.

  • Inference from reason text: the CI-related cases are gone (CI_REASONS, PENDING_CI, CI_NOT_PASSED, hostPending). Older ones remain outside this PR's scope: settled.ts:32,40,54 (NOT_ASKED_REASON, POLICY_UNREADABLE prefix, NEEDS_JUDGE_REASON, QUALITY_EVALUATION_LIMIT_ERROR prefix), apps/hub/src/github/verdict-row.ts:69, and render.ts:473,487,536,595. Worth their own issue.

Earlier findings now resolved: the mixed unconfirmed rules, CI-reading policies waiting, the changed-since note, the dead dependsOnCi export (it now has a caller), the in-flight slot, the waiting-since reset while a reviewer is out, early review under rules: pass, the pass-count clock, and the progress comment's "written once" claim. The sidecar cap is unchanged and accepted in the PR.

Not ready: it conflicts with main, there is the re-judge gap, and there is the stopped-pass judge deploy.

@TheGreatAxios
TheGreatAxios force-pushed the cl-11611-keep-the-manager-alive-until-its-final-post branch from a31f711 to c6823a6 Compare October 28, 2026 04:03
@TheGreatAxios
TheGreatAxios marked this pull request as ready for review October 28, 2026 04:04
@TheGreatAxios

Copy link
Copy Markdown
Contributor Author

Re-review at c6823a6, rebased on main. The CI state is now the one record of a CI hold, a replayed judge verdict has its Dos made again from the pass's facts, a stopped manager deploys no judge, and hostAwaited returns a fact.

Checks:

  • git merge-tree with origin/main is clean, and main is an ancestor of the head.
  • Local lint: 22 warnings, the same as before, none new.
  • Local tests:
    • manager, workflows, contracts and host tools: 691 pass.
    • hub with Postgres: 443 pass.
    • bun run test:chain: 13 pass.
  • CI: all 17 checks pass or skip, including typecheck and chain 1-5.

All three findings from the last review are closed:

  • Red then green on the same commit: a stored judge verdict is now replayed through verdictOnFacts, so its Dos are made again from the current facts. decided() no longer reads actions, and the manager test at triage-manager.test.ts:850 covers it.
  • Stopped pass: a stopped pass deploys no judge (judgeScope), and verdict() skips the judge on it.
  • One CI record: ci is it. The portal derives heldUntil from ci === "pending", and no heldUntil is left in the workflows or contracts.

There is no new logic that infers from reason text. CI_RUNNING, CI_NOT_STARTED and MERGEABILITY_UNKNOWN are defined once and used only as output. There is no dead code: every new export has a caller, and dependsOnCi is used by hostAwaited.

One finding:

  • packages/triage-manager/src/actions/judge.ts:30-40: a replayed stored judge verdict ignores what the head still awaits from the host.
    • When it happens: CI is re-run on the same commit, so the facts read pending again, and the policy reads CI.
    • What happens: judgeScope sees the stored judgement as settled and posts. verdict() then returns verdictOnFacts(item, stored), which keeps the stored rendering, so it is never waiting, even though waitingOnHost(rules.items) is true. The pass posts it as a final verdict with its Dos held on ci: pending, awaiting sees no wait, and the manager ends.
    • Effect: whether the held Dos ever run depends on the completed check_run webhook, with no 5-minute re-read behind it. The reconciler polls only verdicts that carry a ci-green check, so it does not cover a policy whose actions read CI while CI green is off. This goes against "the manager waits whenever the policy reads CI".
    • Fix: when waitingOnHost(rules.items), render the replay as waiting, as the evaluate path does, so the pass shows Waiting for CI and the manager keeps polling.
    • Found by reading, not reproduced.

Not ready until the replay waits for CI, or the owner accepts relying on the webhook for that case.

@TheGreatAxios

Copy link
Copy Markdown
Contributor Author

Fixed in d59e216: a stored judge verdict is no longer posted while the head waits on the host again; that pass waits like the evaluate path, and a manager test covers it.

@TheGreatAxios

Copy link
Copy Markdown
Contributor Author

Re-review at d59e216. Before replaying a stored judgement, the manager now checks whether the head is waiting on the host. The stopped-waiting branch runs after the replay.

Verified:

  • CI runs again on the same commit with a stored judgement: the pass waits (evaluateRules comes out waiting), deploys no judge, posts nothing and asks done to wait. The new manager test covers this.
  • Stopped pass with a stored judgement: take sets noWait, so hostAwaited returns nothing and the stored answer is replayed. I probed this twice with a temporary test on a pass-with-approve policy, with CI still running past the 3-hour clock:
    • no judge was deployed;
    • verdict pass, posted with the "stopped waiting" note;
    • the comment Do was held and the approve was skipped;
    • done's wait was false, so the manager ends and never waits.
  • Manager and workflows tests: 390 pass. Lint: 22 warnings, the same as before.

One low finding:

  • packages/triage-manager/src/actions/review-clean.ts:128: stoppedWaiting holds every Do but keeps its ci: "pending".
    • Effect: the portal reads that as heldUntil: "ci-finishes", and apps/web/src/lib/pending-dos.ts:104 counts the Do as one that runs on its own once CI finishes. The comment says "no action ran on its own, so you decide".
    • Fix: drop ci from the Dos the stop holds, as holdDos does for Dos held for a person.
    • Seen in the probe output: {"kind":"comment","held":true,"ci":"pending"}.

The manager waits for its head's CI and reviewers itself and ends once its final verdict is up; the hub no longer holds heads for CI. Rules Triage could not check for its own reasons no longer hold the judge.
A head with no checks yet waits as pending while CI green is on, for two minutes from its first pass, then is read again and decided as having no CI.
…opens

A pass that mails the judge also deploys code review when the rules step opens its gate and its when names every verdict; the review is still posted with the final verdict.
…waits

A pass that ends waiting for CI, the judge or code review writes the triage comment as In progress, naming what it waits for, and the verdict edits it in place. The chain covers the five-minute re-read and the in-progress comment.
…omment

The in-progress comment keeps the header of the verdict it replaces, so the next verdict still notes what changed since it.
Any unconfirmed rule of the host's now holds the verdict, whatever else is unconfirmed, and so does CI still running when a step reads it through a CI condition, approve or merge.
A rerun on the same head, including one through the in-progress comment, takes the note from the last verdict's header rather than the comment's text.
The reconciler counts only managers judging or reviewing toward the tenant's in-flight cap; one parked on its head's CI does not.
Whether a head waits on the host is read from its facts (CI pending where the policy reads CI, mergeability not computed where conflicts are checked); the inference over rule results is gone. A review started while CI is awaited keeps the CI wait. Early code review goes out with the judge only under when.rules any. A manager lives three hours from its claim; the pass after that decides, says what it stopped waiting for and holds every Do.
@TheGreatAxios
TheGreatAxios force-pushed the cl-11611-keep-the-manager-alive-until-its-final-post branch from 7082a7d to 6b6ae0a Compare October 28, 2026 06:03
@TheGreatAxios
TheGreatAxios merged commit 6445044 into main Oct 28, 2026
17 checks passed
@TheGreatAxios
TheGreatAxios deleted the cl-11611-keep-the-manager-alive-until-its-final-post branch October 28, 2026 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant