Repository navigation
docs: document the pull request lifecycle once and remove passages against it - #694
Conversation
TheGreatAxios
left a comment
TheGreatAxios
left a comment
There was a problem hiding this comment.
Independent review of 5a5d065 against CL-11616, its audit document, and origin/main 6f4cf17.
1. Needs a rebase; it conflicts with main. The branch does not contain origin/main. A trial merge conflicts in docs/ARCHITECTURE.md, docs/PRODUCT.md, the user manual and packages/triage-manager/README.md. Since the branch base, main has rewritten the policy file, CI wait, patch-reading, unrouted-mail, liveness and sidecar passages (#688, #690, #695, #702, #703, #704, #706).
2. The new Lifecycle section, and the rules taken from it, contradict main:
- Step 6 says "The manager itself waits for CI". On main (#706), a head that waits only on CI posts that it waits and its manager ends. The reconciler re-reads its checks and queues it again once CI settles, and a fresh manager posts the verdict.
- Step 8 says "The manager ends only after step 7". This is false for the same reason. A manager also stops waiting after three hours from its claim and decides with
noWait. - Step 9 says "One local sidecar per hub holds the listener, the managers and the judges, up to a cap". On main (#702), each deployment runs in a sidecar process of its own, capped by
TRIAGE_MAX_LOCAL_SIDECARS(default 8), with overflow to a sandbox. - The same claim appears in the root AGENTS.md rule "One sidecar per hub" and in
packages/triage-manager/AGENTS.md("waits for CI ... itself, and ends only after its final post"). Both contradict main. The issue's rule text predates #702, so its wording needs the owner's call. - Step 1 says "mails the host's listener. It holds nothing back but ... the pause label". On main the hub mails a live manager directly and the listener only gets pull requests with no manager up. Main also holds heads whose mail reached no listener as
unroutedand resends them (#704). A repository without a file answersneeds-setupand mails nothing. - Step 2 says "It mails the pull request's live manager, or deploys one". On main the listener never mails a live manager. It deploys one, and when the wake mail fails it cancels that manager and deploys one more, once.
- Step 5 says "The judge is ... deployed when the rules need it". On main the judge is also held until the host's checks finish, or until the manager stops waiting for them.
3. Audit item 13 is not fixed. It is an explicit Outcome item: "the superseded-comment exception". docs/PRODUCT.md (Verdict, "When only a policy change flips the verdict ... marks the earlier one superseded") still has it. So does the user manual's authors paragraph ("Only when someone changes the repository's policy ... does Triage post a new comment, which says it supersedes the one above"). Grepping triage-workflows, triage-manager and github-tool for "supersede" finds nothing that posts such a comment.
4. The manual still has a stale code review passage. The Pull request page list, item 4 Code review, still says that "Send to GitHub posts the review to the pull request" with no mention that Triage posts it with the verdict. That contradicts the corrected Code review section above it.
5. Audit item 38 is only partly done. packages/triage-contracts/README.md still describes stored-policy reading: "A version 1 policy still reads, as version 2 ... named in dropped" and "A stored step's for reads as ...". The audit says to delete it.
Checked and fine:
- Audit items 3, 4, 5, 6, 10, 11, 12, 16, 18, 19, 20, 29, 31, 32, 36 and 39 are addressed.
- The "Always, in this repository" automation paragraph matches
policy-grants.ts,do.tsand the Settings label. - Keeping the bridges' own routes is correct: the hub pins
@corbits/webhooks0.3.2. - The manager does post the review (
triage-manager/src/actions/review.ts, effectreview:<head>). - Root AGENTS.md gains all four rules, and the triage-manager and triage-judge AGENTS.md files exist.
- No new hedges, and no mechanics added to the manual.
5a5d065 to
912b6a2
Compare
|
Rebased on main and fixed findings 1, 2, 4 and 5 in 912b6a2; finding 3 is not applied because |
|
Triage: In progress Triage is waiting for CI to finish before it decides. This comment shows the verdict once it has. |
TheGreatAxios
left a comment
TheGreatAxios
left a comment
There was a problem hiding this comment.
Re-review of 912b6a2, based on origin/main cc30552. The branch contains main, and only markdown files changed.
The earlier findings are closed:
- Lifecycle steps 1, 2, 5, 6, 8 and 9 now match main: the hub mails a live manager directly,
unroutedholds,needs-setup, the listener's single retry, the judge held for CI, the manager ending while its head waits only on CI with the reconciler waking it, the 1-hour and 3-hour bounds, and one local sidecar per deployment capped byTRIAGE_MAX_LOCAL_SIDECARS. - The root AGENTS.md and triage-manager AGENTS.md wording matches main.
- Item 13 is correctly kept.
writeHeadedCommentand the supersedes note intriage-contracts/src/triage-comment.tsimplement it; my earlier "no code" claim was wrong. - The manual's Code review item 4 is fixed, and the version 1 policy text is gone from the contracts README.
- No hedges were added, and no mechanics in the manual.
Two low findings remain:
- Lifecycle step 4 says "The manager posts its labels at once and one comment that reads in progress". On main, the in-progress comment is written only by a pass that ends waiting (for CI, the judge or code review), and only where the role holds Comment. A policy of only deterministic rules posts its verdict on the first pass with no in-progress state. Bitbucket Data Center gets a build status, not labels. Suggested wording: "Unless the first pass decides, the manager posts at once that the head is in progress, naming what it waits on."
- The
triage-contractsREADMEactionsparagraph still says "a stored or file policy naming a group ofAUTHOR_GROUPS". The hub stores no policy text since #688, so "stored or" is stale, the same as audit item 38.
…ainst it ARCHITECTURE.md gains a Lifecycle of a pull request section after Components, matching main: the hub mails a live manager directly and holds unrouted heads, the listener deploys only for pull requests with no manager and retries a failed wake once, a head waiting only on CI ends its manager until the reconciler wakes it, and each deployment has a local sidecar of its own up to the cap. The components diagram shows both hosts, the judge and code review in a sandbox, and the triage pipeline names the workflow that runs each step and adds the review post. Code review is described as posted by the manager with the verdict in ARCHITECTURE, PRODUCT and the user manual. Root AGENTS.md gains the writer, isolation, placement and size rules; triage-manager and triage-judge get an AGENTS.md and triage-workflows' is rewritten around the shared steps. References to the deleted pr-triage deployment, hedges, host data wording, the stored version 1 and step `for` reading and the missing package rows are fixed.
912b6a2 to
b164d6e
Compare
TheGreatAxios
left a comment
TheGreatAxios
left a comment
There was a problem hiding this comment.
Both low findings applied: step 4 reworded; 'stored or' removed from the contracts README.
TheGreatAxios
left a comment
TheGreatAxios
left a comment
There was a problem hiding this comment.
No findings.
![corbits-triage[bot]](https://avatars.githubusercontent.com/u/275831056?s=60&v=4){kind=small}
There was a problem hiding this comment.
Consolidates the pull request lifecycle into one section across the docs and AGENTS.md files, and removes passages that contradicted main. Most important defects: a policy key the schema does not have is documented, and the Bitbucket send button can still show after the manager has already posted a review.
P2 · Bitbucket review shows a Send button even after the manager has already posted it · apps/web/src/components/CodeReviewThreads.tsx:82–83
The docs now say the manager posts each review with its verdict, and that Send to Bitbucket is offered only for a finished review that is not on the pull request yet. SendToBitbucket never checks whether the review is already on the pull request. It shows the button until a send in this session succeeds. SendToGithub does check the pull request's reviews, so the two behave differently.
Trigger: a Bitbucket Data Center pull request whose review the manager already posted with the verdict, viewed in the portal before any manual send. The button still appears, which contradicts the new docs.
Fix: check the pull request's existing reviews in SendToBitbucket, as SendToGithub does, and hide the button when the review is already there.
| the rules that fail a pull request without the decision model have passed, or | ||
| whatever they say with `when: { rules: any }`; drafts are not reviewed. |
There was a problem hiding this comment.
P2 · Documents a when: { rules: any } key the policy schema does not have
The triage-contracts README lists the block keys as when: { triage, paths }. There is no rules key, so the documentation tells policy authors to write a key that the file check will reject or silently ignore.
Trigger: a policy author adds when: { rules: any } to the code-review block, as PRODUCT.md suggests.
Fix: replace the example with a key the schema accepts, when: { triage, paths }, and check the example against the contracts README.
| - **API first.** Every custom hub route is described in `/openapi.json`. The portal and the extension wrap those routes and are never the only way to do anything. | ||
| - **Credentials, grants and tenants are Interchange's.** Secrets live in the hub vault and reach sidecars at launch and rotation; never read them anywhere else, never persist them. | ||
| - **Hosts.** GitHub and Bitbucket Data Center. A repository record carries `source` (absent = GitHub), every mail names it as `repoSource`, and each host has its own pr-triage package. A change to host behaviour covers both, or says why not. | ||
| - **Hosts.** GitHub and Bitbucket Data Center. A repository record carries `source` (absent = GitHub), every mail names it as `repoSource`, and each host has its own listener, manager, judge and code review workflows. A change to host behaviour covers both, or says why not. |
There was a problem hiding this comment.
P3 · Claims each host has its own code review workflow
IMPLEMENTATION.md describes pr-code-review as one provider-neutral code review workflow, not one per host. This normative rule in AGENTS.md says each host has its own code review workflow, which conflicts with that.
Trigger: a contributor follows the Hosts rule when changing code review and creates a duplicate workflow for each host.
Fix: change the rule so that code review is a single shared workflow, and host-specific behaviour is limited to the posting adapters.
| limits. The judge is its own workflow with its own budget, deployed when | ||
| the rules need it once the host's checks have finished, or once the | ||
| manager stops waiting for them. Code review is its own workflow in a |
There was a problem hiding this comment.
P3 · Judge deployment 'once the manager stops waiting' contradicts the manager ending on a CI-only wait
Step 5 has the manager wait for the host's checks before it deploys the judge. Step 6 says a head that waits only on CI ends its manager. The two steps do not say which applies when a head is waiting on CI and the judge has not yet been deployed.
Trigger: rules need the judge while CI is still pending.
Fix: state explicitly whether the judge is deployed while the manager is waiting on CI, or whether the manager ends first and the reconciler deploys the judge after CI settles, and make both steps say the same thing.
| That switch is a brake for the whole workspace. With it off, a repository | ||
| earns more one action at a time: **Always, in this repository** on a held |
There was a problem hiding this comment.
P3 · Automation switch described as 'off' though its values are policy or ask
ARCHITECTURE.md describes the switch as taking the values policy or ask. PRODUCT.md says 'with it off', which does not say which value is meant. Readers cannot tell when Always, in this repository is how automation is earned.
Trigger: someone reads PRODUCT.md to configure workspace automation.
Fix: name the value explicitly, for example 'with it set to ask', and say which value lets a repository earn automation through Always, in this repository.
The docs now describe one pull request lifecycle, end to end, in one place, matching main as it is today, and the passages that contradicted it are fixed.
What changed
unroutedand sent again. A repository without a policy file gets no mail. The listener retries a failed wake once with a fresh manager. A head that waits only on CI ends its manager, and the reconciler wakes it once CI settles. The judge is held until the host's checks finish, and a manager stops waiting three hours after its claim. Each deployment has a local sidecar of its own, up toTRIAGE_MAX_LOCAL_SIDECARS, with overflow and code review in a sandbox.packages/triage-managerandpackages/triage-judgeget their own AGENTS.md; the manager's says it ends while its head waits only on CI.packages/triage-workflows/AGENTS.mdis rewritten around the shared steps.fortext.Audit items out of scope here
Each is owned by another issue: 9, 17, 27 and 37 (CL-11618, inert policy keys); 7 and 34 (CL-11612) and 35 (CL-11613), the patch and judge budgets; 24 (CL-11213, CL-11214), starting without a model; 25 (CL-11617), size in the high-risk default; 26 (CL-11614), the status line. Items 1, 2, 14, 15, 23 and 33 (CL-11611) and 8, 16, 21, 22 and 28 (#688) are already on main.
How it was verified
bun run lintexits 0. oxlint does not check markdown, and only markdown changed.apps/hub/src/manager-intake.ts,manager-state.ts,packages/triage-manager/src/actions/listener.ts,manager.ts,judge.ts, and the sidecar cap inapps/hub/src/env.ts.Waiting on you
writeHeadedCommentinpackages/triage-contracts/src/triage-comment.ts, called from github-tool and bitbucket-datacenter-tool, posts the new comment. Deleting the sentence would make the docs describe something the product does not do. Options: (a) keep the sentence as the one exception; (b) open an issue to remove supersession from the code, and delete the sentence in that PR. Recommendation: (b). The sentence stays in this PR.forreading (audit item 38). The README text is deleted as the audit asks, but the reader code is still inpackages/triage-contracts/src/check-pack.ts(fromVersion1,stepCondition). Removing it is a code change for its own issue.Closes CL-11616