docs(compass): correct the stacking rules, and three gates that could not catch what happened - #137
Conversation
… not catch what happened The stacking section already recommended `gh stack`; the operating instruction banned it, and the ban's stated reason does not survive measurement. On a throwaway stack, `gh stack merge <pr> --squash --yes` landed only the named PR, produced one commit per PR with its message body intact, and auto-retargeted the child to `mergeable_state=clean` with no rebase and no force-push. The 403 and 422 are what the plain endpoints return on a stacked PR, not a property of stacks. Records the working commands and the four gotchas, and corrects the claim that landing a stack bottom forces a restack: true for a hand-managed base chain, false for a linked stack. The one real cost is that no `--message` flag exists, so a hand-written squash message cannot be supplied at merge time. Three gates could not catch things that happened. Gate 2 now requires tests to exercise something the PR did not itself add: a module plus tests for that module, imported by nothing else, passed all four gates and demonstrated nothing. Gate 3 now covers umbrella briefs, which state no named result and so force the developer into the choice the rule forbids. And two rules are added from failures rather than theory: a finding that outlives its PR needs an issue, because PR bodies are squashed away; and a PR's state is read from the last entry in its thread, because one PR sat recorded as approved through six checks with no reviewer on its head. The effort instrument is deliberately not touched here. It is the open escalation and the evidence is in the PR body. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jgong5
added
the
need human
…ward needs Two additions, both from this session rather than from theory. The owner asked what a capture task had established and got a process report back. The reply that worked opened with the finding -- a real model traces at both widths and its shapes are entirely concrete, 0 symbolic of 13,107 -- and put the method under it. The existing output-shaping line says to lead with the next action, which is right for a status message and wrong for an answer; this adds the answer case. The main worktree was found fourteen commits behind after a full session of landings. The rule to fast-forward it already existed; what it lacked was a reason urgent enough to stop it being skipped. That reason is that the main worktree holds the local integration branch, every linked worktree shares the ref, and the resolver tries the bare name first -- so a stale local branch wins for any command that omits COMPASS_INTEGRATION_REF. The chown back is recorded as two steps because one is a trap that was walked into while writing this. `chown -R` over the main worktree also covers `.git/worktrees/`, which every linked worktree shares, and container git then refuses all of them with dubious ownership. Four worktrees broke at once, mid-session, with agents running. `.git` must go back to root afterwards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two chains were linked with gh stack when they were two PRs each, then grew to five and four. Nobody went back. The result is worse than not linking: gh stack merge auto-retargets the members inside the stack and not the ones outside it, so two disciplines apply to different PRs in one chain and nothing on the PR says which. The owner found it by reading the stacks rather than the reports. The rule now says to re-link the whole chain in the same step a PR joins it -- gh stack link updates an existing stack rather than creating a second, so it is idempotent -- and not to link a chain carrying need human anywhere below it, since the label stops agent action on every member the link would touch. The check is stated as something to build and run rather than as a habit: for each open PR whose base is another open PR's branch, assert both sit in one stack, exit non-zero on drift, and count held chains separately. It is stated rather than shipped as a path, because the working copy lives in agent_scratch, which is git-ignored, and a committed rule must not depend on a file the next session may not have. Also corrects the unstack clause written one commit earlier. It said unstack refuses while a member is queued for merge. Measured again after both probe PRs were closed and their branches deleted, it still refuses -- so a stack object can outlive everything it points at, and linking is not freely reversible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six amendments, each with the measurement that justifies it: - Landing is the agents' job. An approved PR with no need human on it or below it in its stack is landed with no owner approval; the only other holds are reasons written in the rules, named when used. Adds the landing procedure: tree comparison on a moved tip, a trial-merge gate when trees differ, and the post-landing steps. A handoff note does not override the rules file. - A declared escalation carries the label, or it is not a stop. - A pin is inert until someone has seen it fail: reinstate the defect, record both counts and the failing node id; mutations preserve line count. - An approval covers a tree, not a PR: a moved head gets a delta review pinned to <approved sha>..<head>. - Check delivery (issue comments, delivering PRs) before claiming or briefing an issue. - The design-doc rule is checked at the head over the whole file set, with each hit classified as prose or emitted. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| - **If there are no blocking issues, say so explicitly** in every message. | ||
| - **Quote the context; stop only for critical decisions.** | ||
| - **Always communicate PR status.** The owner does not care about local worktree state. | ||
| - **Answer with the conclusion first.** When the owner asks what a task |
There was a problem hiding this comment.
Required: contradicts L13, eight lines below, in the same section.
Here: the first line is "the finding, not the method, the process, or what happens next".
L13: "Output shaping (/i-have-adhd): lead with the next action".
This is scoped to "when the owner asks what a task established", and that may be the intended distinction, but nothing says which rule wins when a status message does both, which is most of them. One clause fixes it, for example "this overrides the next-action lead for that question only". Otherwise the example sentence alone is enough and the last sentence ("What this replaces…") can go.
This amendment, the fast-forward paragraph (L32-38) and the chown recipe (L39-56) all come from 37c9457f2, and none of the three appears in the PR body's list of amendments.
There was a problem hiding this comment.
Done in e8d43f1. The rule now reads: "the first line is the finding, not the method or the process; for that question this overrides the next-action lead below." The "What this replaces" sentence is gone. The three 37c9457f2 additions are now items 12-14 in the body.
| beyond tidiness: the main worktree holds the local `feature/atomcompass_new`, | ||
| **every linked worktree shares that ref**, and `compass_resolve_ref` tries the | ||
| bare name first -- so a stale local branch beats `fork/...` for any command that | ||
| omits `COMPASS_INTEGRATION_REF`. That is the defect the resolver was changed to |
There was a problem hiding this comment.
"That is the defect the resolver was changed to announce rather than resolve silently" is history. A reader who does not know the change can't act on it. The instruction stands without the sentence. compass_resolve_ref and COMPASS_INTEGRATION_REF do exist (scripts/compass/README.md:311-355), so the mechanism claim checks out.
This bullet and L21-31 are both about the fast-forward. Folding this into L21 as one clause, "it is easy to skip, and a stale local branch wins ref resolution for every linked worktree", saves five lines.
There was a problem hiding this comment.
Done in e8d43f1. The history sentence is cut, and the bullet is folded into the fast-forward rule as one reason: "every linked worktree shares the main worktree's local feature/atomcompass_new, and compass_resolve_ref tries that bare name first, so a stale local branch wins for any command that omits COMPASS_INTEGRATION_REF."
| chown -R 0:0 .git # MUST follow, or every linked worktree breaks | ||
| ``` | ||
|
|
||
| Working tree 13797-owned, `.git` root-owned -- the state the repo was already |
There was a problem hiding this comment.
Required: this paragraph contradicts L25-30, which still stands, and its stated premise is not the state of the repo today.
L25-30 (unchanged): chowning to the host user "makes container git refuse the same tree with dubious ownership until a safe.directory entry for that path exists… expect to re-add it after a container rebuild — script it".
Here: "Working tree 13797-owned, .git root-owned -- the state the repo was already in, which is why no safe.directory entry is needed."
Only one of those can be the instruction. Measured just now on the feature/atomcompass_new clone (llm_infer_deploy_study/perf_modeling/ATOM):
.gitis 13797-owned, not root: 321 of 323 entries at depth ≤2, and every one of 3725 entries under.git/objects./root/.gitconfigcarries explicitsafe.directoryentries for that path and its.git, plussafe.directory = *.
So container git works on this tree today because of safe.directory, not because of the ownership split. With * set, the claim "no entry is needed" cannot currently be tested either way. Either replace L25-30 with this recipe and remeasure without the * entry, or keep L25-30 and cut "which is why no safe.directory entry is needed". The two-step chown and the verify-afterwards instruction are worth keeping whichever way this goes.
There was a problem hiding this comment.
Done in e8d43f1. I re-measured: .git is 13797-owned, and /root/.gitconfig has safe.directory = *. With GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_NOSYSTEM=1, container git refuses both the main worktree and a linked one (kv-1) with dubious ownership. The file now has one recipe. Container git works because of safe.directory = *, and after a rebuild you re-add it with git config --global --add safe.directory '*'. After every pull, run chown -R 13797:13797 . (whole tree), then verify with stat and git -C <each worktree> rev-parse HEAD. The old L25-30 paragraph and the root-owned .git step are both gone.
| a citation in the output record. | ||
| - **The design-doc rule is checked at the head, over the PR's whole file set** — | ||
| never over the added lines of a delta, which cannot see a reference that | ||
| arrived before the range. Classify each hit as prose or emitted. A design |
There was a problem hiding this comment.
"Classify each hit as prose or emitted" has no consequence attached. L73-76 already forbids both (code may not cite, and "This extends to runtime data"). If emitted hits must be fixed in this PR and prose hits may be filed as an issue, say that. If both are fixed, the classification step can go.
The test-opens-by-path exemption is useful and should stay.
There was a problem hiding this comment.
Taken in e8d43f1. The classification step had no consequence, so it is cut. The by-path exemption stays.
| whose review already passed. An agent applies the label the moment it | ||
| escalates, so it can stop itself; only the owner removes it, and removal is | ||
| what restarts the work. | ||
| - **A declared escalation carries the label, or it is not a stop.** A halt |
There was a problem hiding this comment.
Required: say where "declared escalation" ends and "actionable review finding" (L146-147) begins.
L146-147: "an actionable review finding is not an escalation, and is not labelled." This rule: every declared escalation is labelled. Neither says what an escalation is, so it falls to the reader to decide whether each of these is one: the effort halt (L213-216, "halt-and-discuss"), the stop-and-discuss list (L133 "When something does not work as expected"), and the review-loop stop (which already says to label).
The line the evidence supports is: anything that needs an owner ruling before work can continue is an escalation and is labelled when it is declared. Anything the developer can resolve without a ruling is a finding and is not labelled. One sentence like that, here or at L146, makes both rules mechanical. As it stands, a stop-and-discuss raised in a PR body is not covered by either rule.
Verified on GitHub: #81, #85 and #95 have never carried a label. #91 was labelled only at 2026-09-22T22:33Z, after its round-2 APPROVE (09-21 20:01Z). All four PR bodies declare an effort halt past ~2x and point to #89, which has carried need human since 09-21 18:12Z. The measurement holds.
There was a problem hiding this comment.
Done in e8d43f1, with your wording: "An escalation is anything that needs an owner ruling before work can continue; it is labelled need human when it is declared. Anything the developer can fix without a ruling is a finding, and is not labelled." The stop-and-discuss list is an escalation until its cause is known. If the cause turns out to be the task's own bug, fixable without a ruling, it becomes a finding. The effort halt is named as an escalation. L146 now reads "The owner is asked only for an escalation, never for a finding."
| module plus tests for that module, imported by nothing else, is | ||
| self-confirming: it passes every gate and demonstrates nothing. Where a | ||
| task is verification rather than implementation, the brief says so and the | ||
| deliverable is evidence, not a package. |
There was a problem hiding this comment.
Required: this sentence decides ruling B, which the PR body says it leaves out.
The body's ruling B asks "If verification tasks should generally leave evidence and no product code, that belongs in the rules as a task type, and I have not assumed it." This line says: "Where a task is verification rather than implementation, the brief says so and the deliverable is evidence, not a package." That is the ruling, written as settled. It arrived in c409d80c7, so it has been in the PR since before the body was written.
The half that measurement supports is the self-confirming-package test (the first two sentences). Keep those. Either drop "and the deliverable is evidence, not a package", or keep only "the brief says so" and leave what the deliverable is to the owner's ruling on B.
There was a problem hiding this comment.
Done in e8d43f1. Gate 2 keeps only the self-confirming-package test. The verification-task sentence is removed entirely, not only its deliverable clause, because whether verification is a separate task type is itself what ruling B asks. B stays in the body, with a note that c409d80c7 had assumed it.
| counts plus the failing node id and assertion. A developer reverts their | ||
| own fix before claiming it; if nothing reddens, they add the pin or state | ||
| why the fix is unobservable. **Mutations preserve line count** — a | ||
| line-drift guard fires on any edit and reads as coverage. Measured: #163 |
There was a problem hiding this comment.
Required: the rule as written was satisfied at #163, so it would not have prevented the failure it cites.
I read #163's cycle-2 APPROVE (2026-09-22 12:34Z). That reviewer did what this rule prescribes: reverted each of the eight fixes independently on git archive trees, recorded the counts and node ids, and found finding 6 "39 passed — inert / does not bite". They approved anyway, treating the inert pin as a non-blocking cleanup. The later delta review's 39 passed → 1 failed / 38 passed confirms the measurement (verified in the thread). What the evidence shows is not missing reinstatement. It is an inert pin on a required finding being accepted as non-blocking.
The missing clause: an inert pin on a required finding is itself a required finding, and APPROVE waits for it. Without that sentence, a reviewer can follow this paragraph to the letter and repeat #163.
Also, "a line-drift guard fires on any edit and reads as coverage" uses a term this file never defines. A future reader can't apply it. Say what it means, for example "a test that asserts a source line number or a file's length fails on any edit, so a mutation that changes the line count looks as if the test caught it", or cut it.
There was a problem hiding this comment.
Done in e8d43f1. Added: "An inert pin on a required finding is itself a required finding: the reviewer does not approve over it." The evidence line now says what happened: "#163's cycle-2 reviewer reinstated the defect, recorded the pin as inert (39 passed), and approved anyway." The line-drift guard is defined in one clause: "a test that asserts a source line number or a file's length fails on any edit and would look as if it caught the mutation."
| `feature/atomcompass_new` — never `main`, never `master`, never a branch on | ||
| upstream `ROCm/ATOM`. | ||
| - **Landing is the agents' job; no owner approval is needed or sought.** An agent | ||
| lands any PR that is approved — the verdict in the last review comment of its |
There was a problem hiding this comment.
Landing condition omits gates 1-3, and defines "approved" differently from L127.
- L178: "Four gates land a task, all required." This bullet lands on "approved" plus "no label". If an APPROVE is meant to certify gates 1-3, say so in one clause. "The only other holds are reasons written in this file" also leaves out
16_execution_plan.md's "A task touching EPLB, CUDA-graph capture bounds, block tables… runs the GPU superset as part of its own gate, not at wave end", which lives in doc 16, not in this file. Separately, the only "wave" left in this file is L182's per-wave GPU superset. One clause saying landing does not wait for it would close the last "wait for a wave" reading. - Here the verdict is "in the last review comment of its thread". L127 says a PR's state is "read from the last entry in its comment thread". When a developer comment follows an APPROVE with no head change, L127 reads the PR as not approved and this bullet reads it as approved. Pick one definition and refer to it.
No leftover text says landing waits for the owner. L2-3, L146 and L150-154 are all consistent with this bullet. Good.
There was a problem hiding this comment.
Done in e8d43f1. (1) "The APPROVE is the reviewer's statement that gates 1-3 hold for that head; landing does not wait for the per-wave GPU superset." (2) One definition, used in both places: "A PR's state is its last thread entry; its verdict is the last comment that carries one." Landing reads the verdict. A developer record after an APPROVE with no head change leaves the verdict standing.
| lands any PR that is approved — the verdict in the last review comment of its | ||
| thread, covering its current head — with no `need human` on it **or anywhere | ||
| below it in its stack**. The only other holds are reasons written in this file | ||
| — a declared escalation such as an effort halt, or a change these rules forbid |
There was a problem hiding this comment.
Required: this hold contradicts the escalation rule at L155-156.
L155-156: "A declared escalation carries the label, or it is not a stop. A halt declared in prose does not stop automation; the label does."
Here: "The only other holds are … a declared escalation such as an effort halt", as a hold separate from need human.
If a labelled escalation is already covered by the need human condition at L224-225, this clause only does anything for an unlabelled escalation, which L155 says is not a stop. An agent reading only this bullet will hold on prose. An agent reading only L155 will land. Suggested fix: drop the escalation from this list. If an agent finds an escalation declared but not labelled at landing time, it applies the label (as L155 already requires) and then holds because of the label. The hold list then has one mechanism.
Separately, "a change these rules forbid such as design-doc references in code" is a review finding and should show up as REQUEST_CHANGES in the last review. As a landing-time hold it lets the landing agent overrule an APPROVE on its own reading. If that is intended, say so. If not, cut it.
There was a problem hiding this comment.
Done in e8d43f1. "The hold is the label: a PR whose body declares an escalation but carries no label gets the label, and is then held by it." I also cut the design-doc-reference hold. A forbidden change is a review finding, and the landing agent does not overrule an APPROVE.
| - **A handoff note is a predecessor's judgement, not a rule.** Where a note | ||
| contradicts this file, this file wins. | ||
| - **Before landing on a moved tip, compare trees.** If | ||
| `<tip after landing>^{tree}` equals `<reviewed head>^{tree}`, the reviewed |
There was a problem hiding this comment.
Required: <tip after landing> is not known before landing, which is when this check is supposed to run.
As written, the check can only be done after the fact, and that is how the #162→#170 example was obtained. The form you can run beforehand is:
git merge-tree --write-tree <current tip> <reviewed head> # == <reviewed head>^{tree} ?
I checked that this reproduces the example. git merge-tree --write-tree 92f1fdafe 5e8d821f6 → fd1492b35…, and so does it against d175b03c6. That equals 5e8d821f6^{tree} and the landed 5a07d4489^{tree}. Measurement verified.
One thing the example should not be read as showing: 92f1fdafe (the tip #162 landed on) was already an ancestor of #170's reviewed head, so tree equality was guaranteed. That is the degenerate case of "a moved tip", and it says nothing about a tip that gained a commit the head lacks. The rule is fine. It is the example that is weaker than it reads.
There was a problem hiding this comment.
Done in e8d43f1. The check now runs before landing: "If git merge-tree --write-tree <current tip> <reviewed head> prints <reviewed head>^{tree}, the reviewed gate result stands." I re-ran it and got fd1492b35. The evidence now says the #170 case was trivial: "only because the tip was already an ancestor". The rule also states that equality is automatic in that case.
| against an older tip: `git merge-tree --write-tree`, bottom-first per chain, | ||
| then gate the combined tree before landing — two green PRs can merge red, and | ||
| package-wide globs are the known mechanism. | ||
| - **After landing:** fast-forward the main worktree (above); close a tracker |
There was a problem hiding this comment.
"After landing: fast-forward the main worktree", but L21 says the main agent does the fast-forward, and this bullet now lets any agent land. Say which of them does it. If it is the landing agent, L21 needs amending. If it is the main agent, add "(the main agent's job, L21)" here.
"Close a tracker issue whose tasks have all landed": L112-113 require every issue to be closed "deliberately, with the handoff comment". Say whether a tracker's close needs one too.
There was a problem hiding this comment.
Done in e8d43f1. The landing agent now does the fast-forward, and L21 says so: "On every landing, the landing agent fast-forwards the main worktree". A tracker issue is now closed "with a handoff comment", consistent with L112-113.
| **Do not link a chain with `need human` anywhere below it.** The label stops | ||
| agent action on that PR, and linking acts on every member. | ||
|
|
||
| **The drift check**, which belongs in the slot check rather than in memory: for |
There was a problem hiding this comment.
This depends on something no reader can check: a script that is not in the tree, run from a "slot check" this file never defines.
The landing amendment rightly says a handoff note never overrides this file. This paragraph points in the opposite direction. It says the check "belongs in the slot check rather than in memory", that its working copy "may sit in agent_scratch/", and to "rebuild it rather than assume it survived". So the rule's instrument exists only in scratch space and in the head of whoever runs the "slot check". "Which this project has now learned twice" is history, not instruction.
Either commit the check under scripts/compass/ and name it here in one line, or state only the invariant: for each open PR whose base is another open PR's branch, both sit in one stack; held chains are counted, not failed. The positive-control sentence repeats the pin rule at L199 ("inert until someone has seen it fail"). One general statement in gate 4 would cover both.
There was a problem hiding this comment.
Done in e8d43f1. I cut this to the invariant. The slot check, the scratch copy and "learned twice" are gone: "Drift check: for each open PR whose base is another open PR's branch, both sit in one stack (gh api ...stacks?pull_request=<n>); held chains are counted, not failed." The positive-control sentence is merged into one general statement in gate 4: "A check counts only once someone has seen it fire — a test, a pin, or any instrument in this file."
Review — PR #137,
|
| # | New text | Stands against | Inline |
|---|---|---|---|
| 1 | L226 landing holds on "a declared escalation such as an effort halt", separately from the label | L155-156 "a halt declared in prose does not stop automation; the label does" | L226 |
| 2 | L53-54 ".git root-owned … no safe.directory entry is needed" |
L25-30 (unchanged) "container git refuse[s] … until a safe.directory entry … exists … script it". Also, measured today: .git is 13797-owned and /root/.gitconfig has safe.directory = *, so the premise is not the current state |
L53 |
| 3 | L5-8 "the finding, not … what happens next" | L13 "lead with the next action" | L5 |
| 4 | L223 "the verdict in the last review comment" | L127 "state is read from the last entry in its comment thread" | L223 |
| 5 | L240 landing agent fast-forwards after landing | L21 "the main agent fast-forwards" | L240 |
Also: L146-147 vs L155. "An actionable review finding is not an escalation, and is not labelled" and "a declared escalation carries the label" do not conflict, but nothing says where the line between them is. Neither rule defines "escalation", so the L133 stop-and-discuss list falls in the gap. Proposed wording is inline at L155.
Checked and clean:
- The old "Landing the bottom of a stack forces one restack of everything stacked above it" (base L151-152) is removed, not left beside the correction. L306-310 replaces it, scoped to hand-managed chains.
- Nothing left in the file says landing waits for the owner. The only remaining "wave" is gate 1's per-wave GPU superset (L182). I suggest one clause saying landing does not wait for it (inline L223).
- The pin rule (L199-207) does not contradict gate 2 or gate 4. Its positive-control idea is repeated in the drift check (L303-305).
Owner rulings
- Ruling B is decided in the diff. L190-192: "Where a task is verification rather than implementation, the brief says so and the deliverable is evidence, not a package." The body's ruling B asks exactly this and says "I have not assumed it". The line dates from
c409d80c7. Inline L192. - Ruling A is untouched. L213 still reads "lines of code" with no instrument named. The landing rule mentions an effort halt only as a kind of hold, which does not choose an instrument.
Rule vs evidence
- Pin rule (L199-207): the evidence does not support the mechanism. compass: the artifact store's identity layer — keys, one naming function, provenance, immutability (ART-1) #163's cycle-2 reviewer reinstated all eight fixes, recorded counts and node ids, measured finding 6 as "39 passed — inert", and approved anyway. The rule as written was followed. The missing clause is an inert pin on a required finding is itself a required finding. Inline L205.
- Landing tree check (L232-234) compares against
<tip after landing>, which is unknowable beforehand. Usegit merge-tree --write-tree <tip> <reviewed head>. I confirmed this reproducesfd1492b35for compass: park a remote fill and hand back what the router relays (#160) #170. Inline L233.
Measurements
Verified:
- compass: park a remote fill and hand back what the router relays (#160) #170's reviewed head
5e8d821f6has^{tree}=fd1492b35= landed tip5a07d4489^{tree}. compass: a simulated KV connector on an injected clock (#156) #162 and compass: park a remote fill and hand back what the router relays (#160) #170 merged 1 s apart, with compass: park a remote fill and hand back what the router relays (#160) #170's basecompass/kv-1. "4601 passed" appears in compass: park a remote fill and hand back what the router relays (#160) #170's thread. Caveat: the prior tip92f1fdafewas already an ancestor of5e8d821f6, so equal trees were guaranteed. The example is the degenerate case (inline L233). gh stack mergeprobe: probe: ghstack A #130 merged as one commit, 1 parent, with message body intact. probe: ghstack B #131 stayed open and its timeline hasautomatic_base_change_succeeded, with base nowprobe/ghstack-base. Stack CI: Collect Accuracy tests summary ROCm/ATOM#132 exists withopen:false.- compass(backends): a step price that reads the batch shape, and the schedule that moves with it (M1-2) #81, compass(design): guard the open-items register's own counts #85 and compass(design): a third document may not state the register's extent #95 have never been labelled. compass: the causality detectors, and three injected violations that fire them (CA-5, #48) #91 was labelled only at 2026-09-22 22:33Z, after its round-2 APPROVE. All four bodies declare a >~2x effort halt and cite The effort rule does not name its instrument, and the candidates disagree by 2-4x on the same diff #89. The effort rule does not name its instrument, and the candidates disagree by 2-4x on the same diff #89 has carried
need humansince 09-21 18:12Z. - compass: the artifact store's identity layer — keys, one naming function, provenance, immutability (ART-1) #163: cycle-2 APPROVE on an inert pin ("39 passed" with the defect reinstated), then
1 failed / 38 passedat065f34f06. Verified, but see above: the evidence does not match the rule. compass_resolve_reftries the bare name first (scripts/compass/README.md:332).- W1.6 — M1 fake model: HF-config geometry plus the shape-analytic cost stub #24 was an umbrella whose brief named no result. The result was recorded afterwards.
- compass: capture the fake-tensor operator inventory at TP1 and TP2 (P0.4 / T5) #10: 931 test lines verified. Source was 618 lines across the PR (the body says 544 in the module, plausibly
capture/alone). compass: withdraw the fake-tensor capture module and its tests (CAP-0) #129's deletions are 1481 against the body's 1,475. Both figures are body-only and not in the diff.
Taken on the document's word: ~42 approved PRs sat for a day, and the owner's quote. 13 heads had moved past their approvals. Two briefs for already-delivered issues. 41 design-doc refs in one PR, and 9 reaching runtime output. The "14 commits behind" reading. The chown breaking four worktrees (the current state contradicts the recipe's premise; see contradiction 2). The #81 "six slot checks". The gh stack 403/422 texts, unstack refusals, the orphan stack, and "only non-draft PRs merge".
Readers: git grep AI_DEV_RULES at head finds nothing under tests/ or scripts/. There are prose mentions in CLAUDE.md and design/16_execution_plan.md, so no gate run is needed.
Rules that lean on this board's history
The PR numbers used as evidence (#81/#85/#91/#95, #162→#170, #163) are fine. In each case the rule stands without them. What does not stand on its own:
- L37 "the defect the resolver was changed to announce"
- L205 "line-drift guard", which is undefined
- L297-305: "the slot check", a working copy in
agent_scratch/, and "learned twice"
The drift check is the one amendment that relies on something not checkable in the tree, a scratch script run from a procedure this file never defines. That is the inverse of the "handoff note never overrides this file" principle the same PR adds. Commit the check or state only the invariant (inline L297).
The PR body's amendment list also omits three additions: L5-12 (conclusion first), L32-38 (the fast-forward is easy to skip) and L39-56 (the chown recipe), all from 37c9457f2.
Proportionality (principle 3)
The file goes from 155 to 313 lines and from 1708 to 3547 words, 2.08x. Each amendment has a real cause, but the file is now long enough that skimming is a real risk. Cutting is better than adding a summary. About 50-60 lines can go without losing force:
- L39-56 replaces L25-30 instead of sitting beside it. This also fixes contradiction 2.
- L32-38 folds into L21 as one clause.
- Drift check L297-305 becomes a committed script plus one line.
- One "a check counts only after it has been seen to fire" sentence in gate 4 replaces the two copies (L199, L303-305).
- The
gh stackgotchas (L267-282): the orphan-stack narrative compresses to about 3 lines. Each "Measured:" tail becomes one clause.
Nothing should be cut outright. The landing, escalation-label, approval-covers-a-tree and check-delivery rules are each short and load-bearing.
Label need human and draft state are unchanged by this review.
Review of 5757068 (REQUEST_CHANGES). Gate 2 keeps the self-confirming package test and drops the sentence that decided what a verification task delivers; that stays an owner ruling in the PR body. An inert pin on a required finding is now itself a required finding. The tree check before landing uses git merge-tree against the current tip, and its #170 example is labelled as the trivial ancestor case. Escalation is defined: anything that needs an owner ruling is labelled; anything fixable without one is a finding. The landing hold is the label alone. State is the last thread entry; verdict is the last comment carrying one. The landing agent does the fast-forward. The conclusion-first rule overrides the next-action lead for that question only. The two ownership recipes become one, matching the measured state: container git works because of safe.directory = *, which a rebuild drops. The drift check is stated as its invariant, and the line-drift guard is defined in one clause. 313 -> 281 lines. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| chown -R 13797:13797 . # whole tree, .git included -- the repo's standing state | ||
| ``` | ||
|
|
||
| Verify with `stat -c "%u %n" . .git` and `git -C <each worktree> rev-parse HEAD` |
There was a problem hiding this comment.
The recipe is true as measured today. Two small gaps for an agent following it cold after a rebuild. Non-blocking.
Measured in the container, 2026-09-23: uid 0. /root/.gitconfig has safe.directory = * (plus ~70 per-path entries). perf_modeling/ATOM and its .git, .git/worktrees, .git/objects are all 13797:13797, and the main tree has 0 root-owned files. With GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_NOSYSTEM=1, container git refuses both the main worktree and the linked worktree compass-worktrees/rules with dubious ownership. The linked worktree's own directory is 0:0, so the refusal comes from the shared .git being 13797's. That confirms L28-33 and the reason the whole-tree chown is safe with * set.
- The verify step names no expected value. Say "both lines print
13797". Otherwisestatshows a number and the agent has nothing to compare it with. /root/.gitconfigalso holds thegh auth git-credentialhelper, so after a rebuild re-addingsafe.directorygetsfetchworking andpushstill fails. Consider "… andgh auth setup-git" on L31.
There was a problem hiding this comment.
Taken in 31b948d, both gaps. The verify step now reads: "Verify afterwards: stat -c "%u %n" . .git prints 13797 on both lines, and git -C <each worktree> rev-parse HEAD succeeds." The rebuild step now adds: "... before any git command, then gh auth setup-git — the same file holds the credential helper push needs."
| workaround improvised under build pressure is exactly the class of decision | ||
| that never gets written down. | ||
| stated range; an interface that cannot be implemented as specified. Each is an | ||
| **escalation** (defined below) until its cause is known; if the cause turns out |
There was a problem hiding this comment.
Required. Read together with L144-148, this sentence stops the investigation it asks for.
L121: every item on the stop-and-discuss list "is an escalation … until its cause is known". L134-135: an escalation "is labelled need human when it is declared". L144-146: the label means "no agent commits to it, reviews it, amends it", and "only the owner removes it".
Follow those three rules literally. An unpredicted test failure gets labelled on the spot. From then on no agent may work on the PR to find the cause, and the finding-not-escalation exit in the rest of this sentence can never be reached without the owner. Every surprising red on the board turns into an owner interrupt, which L133 ("never for a finding") is there to prevent.
The sentence also contradicts the definition at L134. A failure whose cause is unknown needs investigation. It does not need a ruling. Suggested wording:
Each is investigated first. If settling it needs an owner ruling, it is an escalation and is labelled; if the cause is a bug in the task's own change, it is a finding, fixed and recorded in the PR body.
That keeps "do not work around it". The workaround is still forbidden, and the diagnosis is not.
There was a problem hiding this comment.
Done in 31b948d, using your wording. The bullet now opens "When something does not work as expected, stop and diagnose it. Do not work around it." and continues: "Each is investigated first — the workaround is forbidden, the diagnosis is not. If the cause is a bug in the task's own change, it is a finding: fixed, and recorded in the PR body. Only if settling it needs an owner ruling is it an escalation (defined below), labelled and discussed with the owner." That matches the escalation definition two bullets down, so labelling happens only when a ruling is needed. The effort halt stays an escalation, because an overrun past ~2x needs a ruling by definition.
| continue; it is labelled `need human` when it is declared. Anything the | ||
| developer can fix without a ruling is a finding, and is not labelled.** A halt | ||
| declared in prose does not stop automation; the label does. When the ruling | ||
| lives on a separate issue, **label each PR it holds anyway** and name the issue |
There was a problem hiding this comment.
Required (one sentence in the PR body, not the diff). This line answers #250, and the body says it does not.
The body, item 7: "Whether those PRs should now be labelled is #250, not this PR." #250 is labelled need human and gives the owner two options: (1) label #81, #85 and #95 too, or (2) remove the label from #91 and leave the ruling on #89.
This line chooses option 1 in so many words: "When the ruling lives on a separate issue, label each PR it holds anyway". L199-202 makes the >2x overrun "an escalation, labelled as above". Once this lands, an agent has no discretion left: #81, #85 and #95 (open, unlabelled today) must be labelled need human, with #89 named on each. #89 is still open and need human, and the PR body says ruling A "holds three approved PRs".
That is the ruling-B pattern the last round removed: a diff deciding a question that the body presents as the owner's. I am not asking for the line to change. It is the letter of the already-landed AI_DEV_RULES.md:95 too, as #250 itself notes. Change the body instead, e.g.: "This diff takes #250's option 1. Landing it answers #250, and #81/#85/#95 are then labelled. If you want option 2, this line is the one to change."
There was a problem hiding this comment.
Done in the body, as you asked. The line is unchanged. The body now says so at the top ("One exception, stated rather than hidden: the diff decides #250 (option 1)"), in item 7 ("This diff decides #250, as its option 1. ... once this lands, #81, #85 and #95 ... are labelled need human, each naming #89. Landing this PR answers #250. If you want option 2 instead ..., that line is the one to change."), and as ruling C under what the owner is asked to accept. The "Whether those PRs should now be labelled is #250, not this PR" sentence is gone. I re-checked just now: #81, #85 and #95 are open and unlabelled, and #91 and #250 carry need human.
| defect only after reinstating the defect — the pre-fix code via | ||
| `git show`, nothing else changed — re-running, and recording both counts | ||
| plus the failing node id and assertion. A developer reverts their own fix | ||
| before claiming it; if nothing reddens, they add the pin or state why the |
There was a problem hiding this comment.
Confirmed against #163's thread, plus one open door. Non-blocking.
Checked: comment 5776588567 (2026-09-22 12:34Z) is the cycle-2 APPROVE. It reverts all eight cycle-1 findings independently, records finding 6 as "39 passed — inert / does not bite", and says "I am not holding the verdict on it". Finding 6 was one of the eight findings behind the cycle-1 REQUEST_CHANGES (5776227495), so it was a required finding. The delta review 5781783612 then measured 39 passed → 1 failed / 38 passed at 065f34f06. The new L189-190 sentence would have blocked that APPROVE, and the evidence line is accurate.
The open door is "or state why the fix is unobservable". The #163 reviewer's stated reason for approving was that the fix is "correct by construction regardless", which is the same kind of argument. Yet the defect was observable: a one-string change made the pin bite. Suggest: "An 'unobservable' claim is itself checked: if the reviewer can make the reinstated defect fail a test, the claim is wrong and the pin is inert."
There was a problem hiding this comment.
Taken in 31b948d. The open door is closed. The sentence now reads: "if nothing reddens, they add the pin. A claim that a fix is unobservable is checked the same way: if the reviewer can make the reinstated defect fail a test, the claim is wrong." I kept a narrow route for fixes that really are unobservable, such as a pure refactor, but the reviewer's reinstatement now tests that claim instead of accepting it.
| lands any PR whose verdict is APPROVE covering its current head, with no | ||
| `need human` on it **or anywhere below it in its stack**. The APPROVE is the | ||
| reviewer's statement that gates 1-3 hold for that head; landing does not wait | ||
| for the per-wave GPU superset. **The hold is the label**: a PR whose body |
There was a problem hiding this comment.
Ruling on cutting the design-doc-reference hold: the cut is right. "A review must REQUEST_CHANGES it" is not enough on its own, but a landing-time hold would not have fixed that either. Non-blocking.
Why the cut is right: this list now has one mechanism, the label, and every other hold is mechanical (head coverage, tree equality). A hold that depends on the landing agent's own reading of a code rule would let any agent veto an APPROVE. A reviewer that missed 41 references and a landing agent grepping on its way past fail in the same way. Neither is an instrument, and the second one is less careful.
Why review alone is not enough: L66-68 is the evidence. Five cycles, each reporting clean, and nine references reached runtime output. The fix that matches L183 ("a check counts only once someone has seen it fire") is a committed check that runs on the tree in the CPU gate. Today nothing in scripts/compass/ does it (git ls-tree on the tip: no design-ref check). That is code, so it belongs in an issue, not in this PR.
One gap the cut leaves: the file does not say what a landing agent does when it sees such a violation in an approved PR. Under the new L134-136 that is a finding, not an escalation, so it may not label. Suggest one clause: "An agent that sees a rule violation in an approved PR lands it anyway and files the violation as an issue (L104)." Without it, the only move left is to post a comment with a verdict in it, and by L109 that becomes the PR's verdict. That is the landing-time veto again, arriving by a different route.
There was a problem hiding this comment.
Taken in 31b948d. After "an agent that holds a PR names the rule" the landing bullet now says: "An agent that sees a rule violation in an approved PR lands it anyway and files the violation as an issue; it does not post a verdict." The committed whole-tree design-doc check for the CPU gate is not built here. The body names it as a follow-up to be filed as an issue, since scripts/compass/ is held.
| - **A handoff note is a predecessor's judgement, not a rule.** Where a note | ||
| contradicts this file, this file wins. | ||
| - **Before landing on a moved tip, compare trees.** If | ||
| `git merge-tree --write-tree <current tip> <reviewed head>` prints |
There was a problem hiding this comment.
Required. For a stacked child whose parent has squash-landed, this command gives a conflicted, wrong tree. That happened on 5 of the 19 PRs landed today.
I ran this rule's command for every PR in today's batch (5a07d4489..05880556e), with <current tip> set to each landing commit's parent and <reviewed head> set to the PR's head:
| PRs | two-way merge-tree <tip> <head> |
vs the tree that landed |
|---|---|---|
| 14 independent PRs (#86, #142, #181-#211) | clean | equal, 14/14 |
| 5 chain children: #115, #125, #136, #146, #150 | rc=1, conflicts in spec/validate.py, test_spec_verbs.py, test_capture_real_model.py … |
differs, 5/5 |
the same 5, with --merge-base <parent PR's head> |
clean | equal, 5/5 (8e8a8b22e, d621434a0, 813690027, 05b1ebb4b, b22b352d4) |
The cause: a child's head still carries its parent's original commits, while the tip carries the parent's squash. The automatic merge-base is too old, so the parent's changes are applied twice. The landing simulation that produced the gated tree already knew this. agent_scratch/compass_dev/landing-sim/sim2.sh passes --merge-base=<parent PR head> for exactly those five, and says why in its header comment. The rule does not say it.
An agent following L221 and L228 as written gets a conflict on every stacked child. L69 makes merge conflicts "the agent's call", so the agent would then hand-resolve a conflict that does not exist.
Suggested fix, which also merges the two bullets and saves about 4 lines:
Before landing on a moved tip, compute the tree that will land:
git merge-tree --write-tree [--merge-base <parent PR's head>] <current tip> <reviewed head>, with--merge-basefor a PR whose parent has squash-landed. If it equals the tree that was gated, the gate stands. Otherwise apply the batch bottom-first per chain this way and gate the combined tree once, before landing: two green PRs can merge red, and package-wide globs are the known mechanism.
(gh stack merge does this itself on GitHub's side. The rule matters for the trial merge you gate before you call it.)
There was a problem hiding this comment.
Done in 31b948d. I verified one of each form before writing it (git 2.43.0, main clone):
- compass(spec): merge, validate and explain over the machine spec (SPEC-2) #86, independent, tip
5a07d4489(not an ancestor of its head):git merge-tree --write-tree 5a07d4489 1d116c85fgivesd9101913f, rc 0. That equals the landed89ae4ff40^{tree}. - compass(spec): render the difference the check found, and stop claiming a measured origin #115, stacked on compass(spec): merge, validate and explain over the machine spec (SPEC-2) #86, tip
89ae4ff40. The plain form gives rc 1, withCONFLICT (content)inspec/__init__.pyandCONFLICT (add/add)inspec/merge.pyandspec/validate.py. With--merge-base 1d116c85f(compass(spec): merge, validate and explain over the machine spec (SPEC-2) #86's head) it gives8e8a8b22e, rc 0. That equals the landedca39c4ca4^{tree}.
The two bullets are now one: "Before landing on a moved tip, compute the tree that will land. An independent PR: git merge-tree --write-tree <current tip> <reviewed head>. A PR stacked on another adds --merge-base <parent's reviewed head>: its head still carries the parent's original commits while the tip carries the parent's squash, so the plain form reports conflicts that do not exist. If the result is <reviewed head>^{tree}, the gate stands. Otherwise apply the whole batch this way, bottom-first per chain, and gate the combined tree once before landing ..." Net, the section is 2 lines shorter even with the flag and the new evidence.
| `<reviewed head>^{tree}`, the reviewed gate result stands with no re-run. | ||
| The check only tells you something when the tip has commits the head lacks; | ||
| if the tip is an ancestor of the head, the trees are equal by construction. | ||
| Measured: #170's command output and reviewed tree are both `fd1492b35`, but |
There was a problem hiding this comment.
Cite today's batch here instead of #170. It is the non-trivial case, and it shows the rule doing its job.
Measured today on 5a07d4489..05880556e, 19 squash landings:
- All 19 were non-trivial. For every one, the tip it landed on was not an ancestor of its reviewed head.
- All 19 computed trees differed from the reviewed head's tree. So the equality branch of L221 never held once, and each PR went to the trial-merge bullet, which is correct. When the tip has gained commits the head lacks, equality holds only if the head already contains those commits' content. In practice that is a child whose parent squash-landed, and whose parent was itself on the current tip.
- The batch was trial-merged bottom-first per chain (with the
--merge-basefix, see L221) and gated once. The combined tree23b288eegave 4829 passed,GATE_CPU_RC=0, against control5a07d4489at 4601 passed (landing-sim/results/{combined,control}.r1.log). - The landed tip
05880556ehas tree23b288eee0b7663461a469c63ba9fe789134a191, exactly the gated tree.
So the batch demonstrates the rule end to end: the per-PR check says "differs", one combined gate runs, and the tree that lands is the one that was gated. #170 only shows the command's syntax. A one-line replacement:
Measured: 19 PRs landed as one batch; every computed tree differed from its reviewed head, the combined tree
23b288eewas gated once (4829 passed), and the landed tip carries exactly that tree.
There was a problem hiding this comment.
Done in 31b948d. The #170 line is gone and the batch is cited: "Measured: 19 PRs landed as one batch; every computed tree differed from its reviewed head, the plain form falsely conflicted on all 5 stacked children, the combined tree 23b288eee was gated once (4829 passed, against 4601 on the old tip), and the landed tip 05880556e carries exactly that tree." I checked the counts against landing-sim/results/{combined,control}.r1.log (4829 and 4601 passed, both GATE_CPU_RC=0) and checked that 05880556e^{tree} is 23b288eee0b7….
Re-review: PR #137 (
|
| # | Item | Status |
|---|---|---|
| 1 | Ruling B out of the diff | Resolved. No sentence mentions verification tasks, evidence-as-deliverable, or package-vs-evidence. Gate 2 keeps only the self-confirming test. Ruling A is untouched: L199 still says "lines of code" and names no instrument. L201's "an escalation, labelled as above" classifies the overrun and does not measure it. |
| 2 | Inert pin | Resolved, and the evidence is verified in #163's thread. 5776588567 (cycle 2, 09-22 12:34Z) reverted all eight findings, recorded finding 6 as "39 passed — inert", and said "I am not holding the verdict on it". Finding 6 was one of the eight behind cycle 1's REQUEST_CHANGES (5776227495). 5781783612 then measured 1F/38P at 065f34f06. One open door is left: "state why the fix is unobservable" (inline L188, non-blocking). |
| 3 | Tree check before landing | Runnable now, but wrong for stacked PRs. See required item 1 (inline L221) and the evidence swap (inline L225). |
| 4a | Landing hold is the label | Resolved. L212-215 gives one mechanism. My ruling on cutting the design-doc hold is below. |
| 4b | Ownership recipe | True as measured today (inline L43, two small gaps). |
| 4c | conclusion-first vs next-action; state vs verdict; who fast-forwards | Consistent everywhere. L7 scopes the override to one question. L109 defines state and verdict separately, and L130, L208 and L234 all use "verdict". L19 and L231 both say the landing agent fast-forwards, and "main agent" is left only at L130 (orchestration), which does not conflict. |
| 5 | Escalation defined | Defined, but see required item 2 (L121) and required item 3 (#250). |
Required 1: the tree check, run on a non-trivial case (inline L221, L225)
I ran L221's command on every landing in 5a07d4489..05880556e, with <current tip> = the landing commit's parent. All 19 cases are non-trivial: no tip was an ancestor of its reviewed head. For the 14 independent PRs, the two-way merge-tree reproduces the landed tree exactly. For the 5 chain children (#115, #125, #136, #146, #150) it exits 1 with conflicts. Passing --merge-base <parent PR's head>, the form landing-sim/sim2.sh actually used, reproduces all 5 landed trees exactly.
Does today's batch demonstrate the rule? Yes, better than #170 does, and the evidence line should cite it. The per-PR check reported "differs" 19/19, which is correct. The batch was trial-merged and gated once: tree 23b288ee, 4829 passed, GATE_CPU_RC=0, against control 4601 at 5a07d4489. The landed tip 05880556e carries exactly 23b288eee0b7663461a469c63ba9fe789134a191. It also shows that on a non-trivial tip the equality branch is rarely taken, and the rule should not imply otherwise. A merged one-bullet wording is inline, and it is about 4 lines shorter.
Required 2: "an escalation until its cause is known" (inline L121)
L121 makes every unpredicted failure an escalation. Escalations are labelled on declaration (L135). The label stops all agent work and only the owner removes it (L144-148). So the diagnosis that could turn it into a finding can never run. This also contradicts the L134 definition, since an unknown cause needs investigation, not a ruling. Fix: investigate first, and label only if settling it needs a ruling.
Required 3: #81 / #85 / #95 (inline L138)
Under this text, yes, #81, #85 and #95 must be labelled need human, each naming #89. All three are open, unlabelled, past ~2x, and held by #89's open ruling. L138 ("label each PR it holds anyway") and L201 ("an escalation, labelled as above") leave no discretion. That is option 1 of #250, which is itself labelled need human and asks the owner to choose. The body says "whether those PRs should now be labelled is #250, not this PR". Fix the body, not the line: say the diff takes option 1, and that landing it answers #250.
Ruling on cutting the design-doc-reference hold (inline L212)
The cut is right. "A review must REQUEST_CHANGES it" is not sufficient on its own, and a landing-time hold would not have made it sufficient. The 41 references survived five cycles because nobody ran a whole-tree check. A landing agent's discretionary grep is the same non-instrument, run less carefully, and it would let any agent veto an APPROVE. What L183 of this file calls for is a committed check that runs in the CPU gate. Nothing in scripts/compass/ does this today, so that belongs in a follow-up issue. One clause is missing: what a landing agent does when it sees a violation. It should land and file an issue. Otherwise the remaining route is a comment carrying a verdict, which by L109 becomes the verdict, and that is the veto again.
Contradictions introduced by the fix
- L121 vs L134/L144: required item 2 above.
- Body item 7 vs L138: required item 3 above.
- Checked and clean: every reference to a cut sentence. The removed "declared escalation such as an effort halt" hold, "slot check", "main agent fast-forwards", the two-step
chown -R 0:0 .git, and the verification-task sentence have no leftover references.design/16_execution_plan.md:476-480("any surprise stops and is discussed … appliesneed human") agrees with the new definition once L121 is fixed. - Readers:
git grep AI_DEV_RULES e8d43f104 -- tests scriptsreturns nothing, so no gate run is needed. There are prose mentions only inCLAUDE.md:6and16_execution_plan.md(L82, 407, 476-479, 489), and none of them depends on cut text.
Followability
With the three fixes in, an agent reading it cold can apply every rule without this board's history. PR numbers now appear only as evidence tails. Two small gaps are inline: the verify step names no expected value (L43), and gh auth setup-git is needed after a rebuild as well as safe.directory (L31).
Proportionality (principle 3)
281 lines, 3208 words (313/3547 before). Most rules are now one bullet each, and the file reads rather than skims up to about L233. After that, 40 lines of gh stack mechanics (L240-278) are reference material in a rules file. What else could go, about 45 lines, none of it load-bearing as a rule:
- L240-278: move the
gh stackcommands and gotchas toscripts/compass/README.md. Keep 3 lines here: recommended, link whole chains, don't link over a label. - L220-230: the two tree-check bullets become one (inline L221), saving about 4 lines.
- L116: "keep the solution as simple as possible" restates principle 3, which L128 already makes every agent read.
None of these blocks the PR.
Review cycle 2 of e8d43f1, three required items and five non-blocking. - Tree check: an independent PR uses the plain `git merge-tree --write-tree <tip> <head>`; a PR stacked on another adds `--merge-base <parent's reviewed head>`, because its head carries the parent's original commits while the tip carries the parent's squash. Verified: #86 plain gives d910191, its landed tree; #115 plain exits 1 with conflicts, and with --merge-base 1d116c8 gives 8e8a8b2, its landed tree. The two tree-check bullets are one, and the evidence is the 19-PR batch (combined tree 23b288e gated once, 4829 passed, carried by 0588055) instead of the trivial #170 case. - Stop-and-discuss: investigated first; an escalation, and labelled, only if settling it needs an owner ruling. Labelling on sight froze the diagnosis the rule asks for. - A landing agent that sees a rule violation in an approved PR lands it and files an issue; it posts no verdict. - A claim that a fix is unobservable is checked by reinstating the defect. - The ownership recipe names the expected uid (13797) and adds `gh auth setup-git` after a rebuild. - The line restating principle 3 is cut; the gh stack section is tightened in place. 281 -> 277 lines, 3208 -> 3158 words. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Review cycle 3: PR #137 (
|
| # | Item | Status | What I verified |
|---|---|---|---|
| 1 | The tree check was wrong for stacked PRs | Resolved | See below. I re-ran all three claimed commands, and each one reproduced. |
| 2 | Stop-and-discuss defeated itself | Resolved | L117-126 now says to investigate first. A cause in the task's own change is a finding. Only a cause that needs an owner ruling becomes an escalation. That matches L135-137, and the effort halt (L202-205) is still an escalation. |
| 3 | The body contradicted L138 on #250 | Resolved | The body now says, near the top, that the diff decides #250 as option 1. Item 7 says the same, and ruling C asks the owner to accept it. L138-140 ("label each PR it holds anyway") agrees with all three places. |
1. The tree check (L225-237), re-run on node-local git
| case | command | output | rc | landed tree |
|---|---|---|---|---|
| #86, independent | git merge-tree --write-tree 5a07d4489 1d116c85f |
d9101913f |
0 | 89ae4ff40^{tree} = d9101913f |
| #115, stacked on #86, plain form | git merge-tree --write-tree 89ae4ff40 134fa56bb |
CONFLICT in spec/__init__.py, spec/merge.py, spec/validate.py, and tests/compass/test_spec_verbs.py |
1 | — |
#115, --merge-base 1d116c85f |
git merge-tree --write-tree --merge-base 1d116c85f 89ae4ff40 134fa56bb |
8e8a8b22e |
0 | ca39c4ca4^{tree} = 8e8a8b22e |
- Both cases are non-trivial.
5a07d4489is not an ancestor of1d116c85f, and89ae4ff40is not an ancestor of134fa56bb. Both computed trees also differ from their reviewed heads' trees:1d116c85f^{tree}=8fa846500and134fa56bb^{tree}=6aa056ed2. That fits "every computed tree differed from its reviewed head". - The batch evidence line holds.
05880556e^{tree}=23b288eee0b7663461a469c63ba9fe789134a191.landing-sim/seq_final.tsvshows 19 rows, allclean, and the last row (compass(design): partition a breached reply contract instead of calling it a hang (#207) #211) names that same tree.results/combined.r1.logshows4829 passed … GATE_CPU_RC=0.results/control.r1.logshowscommit: 5a07d4489,4601 passed … GATE_CPU_RC=0. - The merged bullet reads as one procedure. It gives the independent form, then the stacked form with its reason, then the equality test, then the batch fallback. An agent can run it without knowing this board's history.
2. Diagnose first, escalate only on a ruling (L117-126)
- It no longer contradicts the definition. L135-137 defines an escalation as anything that needs an owner ruling, and says anything the developer can fix is a finding. L117-126 now applies that definition: a cause that needs a ruling is labelled, and a bug in the task's own change is fixed and recorded. The earlier deadlock is gone. That deadlock was: every unpredicted failure is an escalation, the label freezes the diagnosis, and only the owner can unfreeze it.
- A third case is still covered. A cause outside the task's change that can be fixed without a ruling, such as a design doc that is simply wrong, falls under L137's "anything the developer can fix without a ruling is a finding". It does not fall through a gap.
- The effort halt still reads as an escalation. L204-205 says it is "an escalation, labelled as above". It is defined as one outright, not routed through a diagnosis, so the new wording does not weaken it.
3. #250 (body vs L138)
- The body's second paragraph says: "One exception, stated rather than hidden: the diff decides compass: the >2x effort halt holds five PRs, and only one of them is labelled #250 (option 1). Accepting this PR accepts that — see C."
- Item 7 says: "This diff decides compass: the >2x effort halt holds five PRs, and only one of them is labelled #250, as its option 1." It names compass(backends): a step price that reads the batch shape, and the schedule that moves with it (M1-2) #81, compass(design): guard the open-items register's own counts #85 and compass(design): a third document may not state the register's extent #95, each naming The effort rule does not name its instrument, and the candidates disagree by 2-4x on the same diff #89. It also names the line to change if the owner wants option 2.
- Ruling C asks the owner to accept the decision, and the section header says "A and B are not in the diff; C is". Nothing is hidden, and the body and L138-140 now agree.
Also confirmed
- The landing-agent clause (L218-220) does not contradict the state/verdict rule (L110-116). It forbids posting a verdict, which is exactly how the landing-time veto would come back. See follow-up (a) for one wording gap next to it.
- The inert-pin door is closed (L189-193). "State why the fix is unobservable" is gone. A claim that a fix is unobservable is now tested by reinstatement. This does not conflict with the "inert pin on a required finding" clause that follows it.
- The ownership recipe (L28-45) now says to run
gh auth setup-gitafter a rebuild and to expect13797on bothstatlines. Neither change conflicts with the chown line or with the four setup rules. - Ruling A is still undecided. L202 says "lines of code" and names no instrument. Ruling B is still undecided. Gate 2 (L175-178) keeps only the self-confirming-package test, and no sentence mentions verification tasks or evidence as a deliverable. Ruling C is decided openly and flagged.
- Length:
wc -lwat31b948d10gives 277 lines, 3158 words, as claimed. - Readers:
git grep AI_DEV_RULES 31b948d10 -- tests scriptsreturns nothing (rc 1), so no gate run is needed. - Removed text has no stranded references. "Design and implement solutions … as simple as possible", which restated principle 3, is gone. The
gh stackbullets are shorter but keep every measured fact: the 403, the 422,--baseonlink, non-draft only, no--message,unstackrefusing, and theDELETE404. The.git/gh-stackmetadata line was also dropped. It was not a rule.
For a follow-up issue (not required here)
a. Hold vs violation wording (L217-220). "The only other holds are rules in this file … an agent that holds a PR names the rule" sits right next to "an agent that sees a rule violation in an approved PR lands it anyway". The specific examples make the intent clear. Holds are landing preconditions: the label, an approval that covers the head, and the tree check. Violations are content defects. But the text never states that split, so an agent could still hold a PR by citing a content rule such as the design-doc rule. One clause fixes it: "Holds are landing preconditions; a content violation is landed and filed."
b. design/16_execution_plan.md ~L476 still paraphrases the old rule: "any surprise stops and is discussed." It does not contradict the new text, because this file wins and the five named escalations there are real rulings. But it now describes what used to happen rather than what happens now. Reword it to "stops and is diagnosed".
c. The PR body's #115 row lists three conflicting files; there are four. The plain form also conflicts in tests/compass/test_spec_verbs.py. This is in the body only, the diff does not list files, and the conclusion is unchanged.
d. The rebuild recipe (L30-32). gh auth setup-git assumes gh is still authenticated. /root/.config/gh is under /root too, so after a teardown gh auth login (or the token file named in gpu_docker/CLAUDE.md) may be needed first. This is untested and is only a note.
The need human label and the draft state are unchanged by this review.
| The owner is asked only for an escalation, never for a finding. | ||
| - **An escalation is anything that needs an owner ruling before work can |
There was a problem hiding this comment.
shrink: the escalation/finding split appears three times: L134, the new definition at L135, and the 2x-overrun bullet at L204. Keep L135 and cut L134. At L204, write "an escalation" and stop there.
| **A check counts only once someone has seen it fire** — a test, a pin, or | ||
| any instrument in this file. A reviewer credits a test with holding a | ||
| defect only after reinstating the defect — the pre-fix code via | ||
| `git show`, nothing else changed — re-running, and recording both counts | ||
| plus the failing node id and assertion. A developer reverts their own fix | ||
| before claiming it; if nothing reddens, they add the pin. A claim that a | ||
| fix is unobservable is checked the same way: if the reviewer can make the | ||
| reinstated defect fail a test, the claim is wrong. **An inert pin on a | ||
| required finding is itself a required finding: the reviewer does not | ||
| approve over it.** Mutations | ||
| preserve line count, because a test that asserts a source line number or a | ||
| file's length fails on any edit and would look as if it caught the | ||
| mutation. Measured: #163's cycle-2 reviewer reinstated the defect, recorded | ||
| the pin as inert (`39 passed`), and approved anyway. |
There was a problem hiding this comment.
shrink: 14 lines, and the core fits in two: "A reviewer credits a pin only after reinstating the defect (git show of the pre-fix code, line count preserved) and seeing it red. An inert pin on a required finding blocks APPROVE." The developer-revert sentence and the "unobservable claim" sentence are the same procedure told again.
| - **Before landing on a moved tip, compute the tree that will land.** An | ||
| independent PR: `git merge-tree --write-tree <current tip> <reviewed head>`. | ||
| A PR stacked on another adds `--merge-base <parent's reviewed head>`: its | ||
| head still carries the parent's original commits while the tip carries the | ||
| parent's squash, so the plain form reports conflicts that do not exist. If | ||
| the result is `<reviewed head>^{tree}`, the gate stands. Otherwise apply the | ||
| whole batch this way, bottom-first per chain, and gate the combined tree once | ||
| before landing — two green PRs can merge red, and package-wide globs are the | ||
| known mechanism. Measured: 19 PRs landed as one batch; every computed tree | ||
| differed from its reviewed head, the plain form falsely conflicted on all 5 | ||
| stacked children, the combined tree `23b288eee` was gated once (4829 passed, | ||
| against 4601 on the old tip), and the landed tip `05880556e` carries exactly | ||
| that tree. |
There was a problem hiding this comment.
yagni: this is a runbook of about 14 lines inside a policy file. Move the merge-tree sequence into a script (a new scripts/compass/land_batch.sh, which does not exist yet) and leave one line: "Batch landings on a moved tip go through land_batch.sh."
| - **An unlinked chain pays a restack of everything above on each parent | ||
| landing** — `git rebase --onto <new> <old> <branch>` (a plain rebase | ||
| conflicts) plus a REST base patch, per child. A linked `gh stack` does not. |
There was a problem hiding this comment.
shrink: gh stack now takes four bullets (L252 merge, L256 gotchas, L263 link-whole-chain, L272 unlinked restack) for one tool. Fold them into one bullet of about 8 lines and drop the 403/422 probe detail, which is only a reason. This bullet could also go if linking is mandatory, but L248 still calls stacking "recommended, not required". Decide which it is.
|
Over-engineering review (ponytail-review). This pass looks only for complexity. It does not look for correctness. There are no blocking issues. Every comment below is a suggestion to shorten the file, and the change lands the same with or without them. net: about -70 lines possible. Most of it comes from the "Measured:" stories (L25) and from rules stated more than once (L110, L135). |
| - Output shaping (`/i-have-adhd`): lead with the next action, number multi-step | ||
| tasks, end with one concrete next action, restate state every turn, specific time | ||
| estimates, matter-of-fact error tone, cap lists at 5, no preamble or closing |
There was a problem hiding this comment.
delete: "specific time estimates" contradicts L202-203 ("Effort is estimated in lines of code, not time"). Cut the clause. "restate state every turn" also overlaps L4 ("Always communicate PR status"), so keep one of the two.
| `feature/atomcompass_new` (the integration branch). That updates the main | ||
| worktree; it does not develop there, so the rule above stands. It is easy to | ||
| skip because nothing visibly breaks, but every linked worktree shares the main |
There was a problem hiding this comment.
delete: "That updates the main worktree; it does not develop there, so the rule above stands. It is easy to skip because nothing visibly breaks" argues with the previous bullet instead of stating a rule. Keep the compass_resolve_ref reason, which is the part that matters.
| - **No design-doc references in code.** No `D18`, `P0.4`, `T5`, `W2.5`, backticked | ||
| doc numbers, "principle N", or numbered labels like "Gate 1". No quoting design | ||
| principles as justification. Say what the code does, its functions, how it works. | ||
| Design docs may cite each other freely; code may not cite them at all. This | ||
| extends to **runtime data** — a `(BEYOND-D18)` suffix on an emitted stub name was | ||
| a citation in the output record. | ||
| - **The design-doc rule is checked at the head, over the PR's whole file set** — | ||
| never over the added lines of a delta, which cannot see a reference that | ||
| arrived before the range. A design document a test opens **by path** is a | ||
| functional dependency, not a citation, and stays. Measured: one PR carried | ||
| **41** through cycles that each reported clean; a board-wide census found nine | ||
| reaching runtime output. |
There was a problem hiding this comment.
shrink: two bullets, one rule. Merge them: "No design-doc references in code or runtime output (D18, P0.4, T5, "principle N", "Gate 1"). Checked at the head over the PR's whole file set. A doc a test opens by path is a dependency, not a citation." That's 3 lines instead of 12, and the (BEYOND-D18) example and the census go too.
| - Merge conflicts are the agent's call, not the owner's ("don't bother me on merge | ||
| conflict, it's on you"). Tasks are cut so each touches one module plus its tests, | ||
| which makes most of them disjoint, but they are **not guaranteed disjoint** and no |
There was a problem hiding this comment.
shrink: L70-76 is 7 lines of reasons for having no locking machinery. The rule fits in 2: "Merge conflicts are the agent's call. Frequent conflicts mean re-cut the tasks, not add a scheduler." Nothing currently proposes the file locking this rules out, so the "no allocation-time file locking" sentence can go.
| - **Automation is on by default.** An agent acts on any issue or PR that does | ||
| not carry the `need human` label — no opt-in, no waiting to be told. | ||
| - **`need human` stops all agent action on that issue or PR** — no agent |
There was a problem hiding this comment.
shrink: need human semantics are spread across five bullets: L135-142 (what it is), L143-144 (automation on without it), L145-149 (what it stops), L150-153 (review-loop halt applies it) and L215-217 (landing hold). Make it one bullet: "need human = escalation. Applied by the agent the moment it escalates, removed only by the owner. It stops all agent action on that issue/PR and everything stacked above it. Without it, agents act by default. A halt in prose is not a halt." About 5 lines instead of about 20.
| or the loop passes three cycles, it halts, goes to the owner and applies | ||
| `need human` to the PR: a task that cannot converge is mis-cut, not | ||
| under-worked. | ||
| - Reviewer agents must post their review to the PR; **the verdict goes in the |
There was a problem hiding this comment.
shrink: this bullet and the two after it (L158-166, outside the diff, so they can't take a line comment) say where a review goes, in 13 lines. They fit in 3. The exact text is in the standalone summary comment.
| Baselines are recorded first (the suite's and ruff's pass/fail state, before the | ||
| first Compass commit) — the lint baseline on this repository is already known | ||
| to be dirty, and a pre-existing failure attributed to Compass costs a day. |
There was a problem hiding this comment.
shrink: "and a pre-existing failure attributed to Compass costs a day" is a reason. Keep "Record suite and ruff baselines before the first Compass commit; ruff is already dirty." 1 line.
| - **PRs land squashed onto `feature/atomcompass_new`, the integration branch**, | ||
| one commit per task. GitHub enforces this structurally | ||
| (`allow_merge_commit=false`, `allow_rebase_merge=false`). Base branch is always | ||
| `feature/atomcompass_new` — never `main`, never `master`, never a branch on | ||
| upstream `ROCm/ATOM`. |
There was a problem hiding this comment.
shrink: GitHub already enforces squash-only, so the allow_merge_commit / allow_rebase_merge parenthesis tells an agent nothing it can act on. "never a branch on upstream ROCm/ATOM" repeats L277. Keep: "PRs land squashed onto feature/atomcompass_new, never main/master." 1 line.
|
Over-engineering review, pass 2: the whole file, not only the delta (ponytail-review). There are no blocking issues. Every finding is a suggestion to shorten the file. The 11 inline comments from pass 1 still stand, and pass 2 does not repeat them. Findings on lines inside the diff are posted inline. The three below sit on unchanged lines outside every hunk, so they have no line to anchor to.
Whole file, pass 1 + pass 2: net about -110 of 277 lines possible. Most of the savings come from three places: the 12 "Measured:" stories, the |
Applies the over-engineering review on this PR. Each rule is now stated once: - the need-human/escalation semantics, spread over five bullets, become one - the "approval covers the head" rule, stated three times, is kept once - review placement, the design-doc rule and the gh stack procedure are merged into one bullet each The "Measured:" stories behind the rules are removed. So are the arguments for choices nobody disputes, and the "specific time estimates" clause, which contradicted the lines-of-code effort rule. Kept, despite the review: the merge-tree landing procedure, as a short sub-bullet rather than a new script, and the unlinked-chain restack line, because stacking is still only recommended. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| its local `feature/atomcompass_new`, and `compass_resolve_ref` tries that bare | ||
| name first, so a stale branch wins for any command that omits | ||
| `COMPASS_INTEGRATION_REF`. After a container rebuild (`/root` does not survive |
There was a problem hiding this comment.
What do compass_resolve_ref and COMPASS_INTEGRATION_REF mean?
There was a problem hiding this comment.
compass_resolve_ref is a shell function in scripts/compass/_lib.sh. snapshot.sh and gate_cpu.sh use it to pick the base they diff against. It tries the bare feature/atomcompass_new first and falls back to <remote>/feature/atomcompass_new only when no local branch has that name. COMPASS_INTEGRATION_REF is the environment variable that overrides the ref it is given (default feature/atomcompass_new).
This matters for the rule because if the local branch exists but is stale, the scripts silently diff against an old base. Fast-forwarding the main worktree, or setting COMPASS_INTEGRATION_REF=fork/feature/atomcompass_new, prevents that.
Fixed in 94e28a0: the bullet now says what the scripts do instead of naming the function.
| not chosen afterwards. An umbrella brief whose children carry the work | ||
| states one anyway, or names the child that carries it; a developer choosing | ||
| one afterwards is the case this rule forbids. | ||
| 4. Review by the task's reviewer agent, looping to APPROVE as above. **A check |
There was a problem hiding this comment.
One more rule: The reviewer should utilize the ponytail-review skill (https://github.com/DietrichGebert/ponytail/blob/main/skills/ponytail-review/SKILL.md) to prevent over-engineering. Not sure where is the right place to put this rule in this file, your call.
There was a problem hiding this comment.
Added in 94e28a0 as part of gate 4 (review): the reviewer also runs the linked ponytail-review skill over the diff, and posts its findings like any others. I put it in gate 4 because that is where the file defines what a review consists of. The "Reviews go on the PR" bullet only says where the output goes.
Its findings are not made blocking, so the reviewer judges them like any other finding. Tell me if you want them to block APPROVE.
…l-review rules - Main-worktree bullet: say what the Compass scripts do with the local integration branch, not just a function name, and name the `fork` remote assumption. - Landing: landing a stacked PR lands every unlanded PR below it, so each of those needs its own APPROVE covering its head, and no label. - Tasks: decompose a complex task into sub-tasks, each its own issue. - Gate 4: the reviewer also runs the ponytail-review skill. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… tests Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
jgong5
removed
the
need human
1 participant
Process amendments to
AI_DEV_RULES.md, drawn from what this session measured ratherthan from opinion. Labelled
need humanon open, so nothing proceeds on it.Two kinds of content: the diff carries what measurement settles; this body carries what
needs your ruling and is deliberately not in the diff. One exception, stated rather
than hidden: the diff decides #250 (option 1). Accepting this PR accepts that — see C.
In the diff — settled by measurement
1. The stacking section was right, and the operating instruction contradicted it
AI_DEV_RULES.mdalready recommendsgh stack. The standing slot-check prompt says"never
gh stack", with the reason "it creates a server-side object that blockssquash-merging the parent AND retargeting the child". I followed the prompt for this
whole session and hand-managed every base chain.
Measured on a throwaway stack (
probe/ghstack-{base,a,b}, PRs #130/#131, stack ROCm#132;the integration branch was never touched and everything is cleaned up):
gh stack merge 130 --squash --yeslanded #130 only, #131 stayed openmergeable_state=cleanThe 403 on
PUT /pulls/<n>/mergeand the 422 onPATCH ... -f base=are real, but theyare what you get by reaching for the plain endpoints on a stacked PR. The 403's own
text names the stack merge path. I read an error message as a wall instead of as a
signpost.
The amended section records the working commands, the four gotchas that cost time
(
--baserequired onlink;unstackrefuses while a member is queued and can orphan astack object; only non-draft PRs merge; no
--messageflag), and corrects the oldclaim that landing a stack bottom forces a restack — true for a hand-managed base chain,
false for a linked stack.
The one real cost of stacking is that a hand-written squash message cannot be supplied
at merge time. Since the squash message is where a task's measured result is recorded,
that is worth your judgement, not mine.
2. Gate 2 — tests must exercise something the PR did not itself add
PR #10 landed a 544-line module in
atom/compass/capture/(618 added source lines acrossthe PR, counting its design-doc edits) and 931 lines of tests for that module, with no
other importer in the tree. It passed all four gates and demonstrated nothing about ATOM.
The rollback (#127 / PR #129) removed 1,481 lines.
The diff carries only the self-confirming-package test. An earlier revision also said a
verification task's "deliverable is evidence, not a package"; that decided ruling B, and
this round removed it (see B below).
3. Gate 3 — umbrella briefs
The rule says a named result is "stated in the issue body before the task is claimed and
not chosen afterwards". Umbrella briefs state none — #24 did not — so its developer picked
one and said that it picked. That is the case the rule forbids, and the developer had no
compliant option.
4. A finding that outlives its PR needs an issue
PR bodies are squashed away. Two reviews re-derived findings that had been written down
repeatedly with no issue to point at. This session opened #123, #126 and #128 for exactly
this reason.
5. A PR's state is its last thread entry; its verdict is the last comment carrying one
PR #81 sat recorded as approved through six consecutive slot checks while its last
thread entry was a developer record and no reviewer had seen its head. The rule now
separates the two readings, and the landing rule uses the second: a developer record after
an APPROVE with no head change leaves the verdict standing.
6. Landing is the agents' job — the owner stated it directly
Owner, 2026-09-23: "Since auto mode is on by default, why do you expect my approval to
land approved PRs?" ~42 approved, unlabelled PRs had waited a day because a handoff note
called landing "the owner's call". An agent lands any PR whose verdict is APPROVE covering
its head, with no
need humanon it or below it in its stack. The hold is the label:an escalation declared in prose gets labelled, then holds by the label. The APPROVE
certifies gates 1-3 for that head; landing does not wait for the per-wave GPU superset.
The landing agent fast-forwards the main worktree. An agent that sees a rule violation in
an approved PR lands it and files an issue; it does not post a verdict, which by the
state/verdict rule would become the verdict and bring back a landing-time veto.
On a moved tip, compute the tree that will land before landing. An independent PR uses
git merge-tree --write-tree <current tip> <reviewed head>; a PR stacked on another adds--merge-base <parent's reviewed head>, because its head still carries the parent'soriginal commits while the tip carries the parent's squash. Equal to
<reviewed head>^{tree}means the gate stands; otherwise the batch is trial-merged bottom-first per chain and gated
once. Verified on two of today's landings:
5a07d4489d9101913f, rc 0d9101913f89ae4ff40spec/__init__.py,merge.py,validate.py,tests/compass/test_spec_verbs.py8e8a8b22e89ae4ff40--merge-base 1d116c85f(#86's head)8e8a8b22e, rc 08e8a8b22eThe reviewer ran all 19: plain form 14/14 on the independent PRs, false conflicts 5/5 on
the stacked ones (#115, #125, #136, #146, #150),
--merge-base5/5. The evidence line nowcites that batch rather than #170 (the trivial, ancestor case): every per-PR check said
"differs", the combined tree
23b288eeewas gated once (4829 passed,GATE_CPU_RC=0,against 4601 at
5a07d4489), and the landed tip05880556ecarries exactly that tree.Follow-up, not built here: the design-doc rule needs a committed whole-tree check in
the CPU gate; nothing in
scripts/compass/does that today, andscripts/compass/is heldas the instrument every open PR's gate delta is measured against. It will be filed as an
issue.
7. An escalation is anything that needs an owner ruling; it is labelled
#81, #85, #91 and #95 all declared effort halts and none was labelled — the ruling sat on
#89 — and an agent dispatched work at #91 because it looked unlabelled and approved.
Anything the developer can fix without a ruling is a finding and is not labelled. The
effort halt is an escalation. The stop-and-discuss list is investigated first: a cause
in the task's own change is a finding, and only one that needs an owner ruling becomes an
escalation and is labelled — labelling on sight would stop the diagnosis that decides it.
This diff decides #250, as its option 1. The line "When the ruling lives on a
separate issue, label each PR it holds anyway", with the effort halt defined as a labelled
escalation, leaves no discretion: once this lands, #81, #85 and #95 (open, unlabelled
today, all held by #89) are labelled
need human, each naming #89. Landing this PR answers#250. If you want option 2 instead (unlabel #91 and leave the ruling on #89), that line is
the one to change. See C below.
8. A check counts only once seen to fire; an inert pin on a required finding blocks
#163's cycle-2 reviewer reinstated the defects, recorded one pin as inert (
39 passed),and approved anyway; one commit later the same reinstatement gave
1 failed / 38 passed. Reinstating was not the missing step — treating an inert pin as non-blocking was.The rule now says so, and a developer's claim that a fix is unobservable is checked the
same way: if the reviewer can make the reinstated defect fail a test, the claim is wrong
(#163's reviewer approved on "correct by construction", and a one-string change made the
pin bite). A re-check of ~25 approved PRs found four inert pins, five fixes
nothing holds, and three findings closed on a test that passed on a neighbouring refusal.
9. An approval covers a tree, not a PR
13 PRs had heads that moved past the comment that approved them, one with an unreviewed
commit sitting under two other approved PRs.
10. Check delivery before claiming or briefing
Two briefs in one day were written for issues an open PR already delivered: one missed a
delivery note in the first comment; one asserted "no PR delivers it — checked, not
assumed" with no check run.
11. The design-doc rule is checked against the tree, not the diff
One PR carried 41 design-doc references while every review cycle reported clean, each
having grepped only the added lines of its delta. A board-wide census found nine reaching
runtime output, inside
raisemessages and printed table notes.12. Answer with the conclusion first (
37c9457f2)When the owner asks what a task established, the first line is the finding. It overrides
the next-action lead for that question only.
13. The fast-forward is easy to skip (
37c9457f2)The main worktree was measured 14 commits behind after a session of landings. Every linked
worktree shares its local
feature/atomcompass_new, andcompass_resolve_reftries thatbare name first.
14. Container git works because of
safe.directory = *(37c9457f2, corrected)37c9457f2said.gitstays root-owned so nosafe.directoryentry is needed. Measuredtoday:
.gitis 13797-owned and/root/.gitconfigcarriessafe.directory = *; with theentry removed, container git refuses both the main worktree and a linked one. The file now
has one recipe: re-add
safe.directory = *and rungh auth setup-gitafter a rebuild(
/root/.gitconfigalso holds the credential helper, so without itfetchworks andpushfails),chown -R 13797:13797 .after every pull, and verify thatstatprints13797for both.and.git.Round 3 (review of
5757068df)Removed ruling B from gate 2; added the inert-pin clause; made the tree check runnable
before landing; made five contradicting pairs agree; defined escalation; cut the drift
check to its invariant; defined the line-drift guard in one clause. 313 → 281 lines,
3547 → 3208 words, while adding the definitions above.
Round 4 (review of
e8d43f104)merge-treeforms (plain for an independent PR,--merge-base <parent's reviewed head>for a stacked one), the two tree-check bulletsare one, and the evidence is today's 19-PR batch instead of compass: park a remote fill and hand back what the router relays (#160) #170.
settling it needs an owner ruling.
the "unobservable" claim is checked by the reviewer; the verify step names
13797;gh auth setup-gitafter a rebuild; L116 (a restatement of principle 3) is cut and thegh stacksection is tightened in place. Thegh stackmechanics were not moved toscripts/compass/README.md, becausescripts/compass/is held. The whole-treedesign-doc check is named above as a follow-up.
Your rulings — A and B are not in the diff; C is
A. Which instrument does the effort rule mean?
This is the open escalation and it now holds three approved PRs and six stacked behind
them. The rule says "estimated in lines of code" and names no instrument. The three
candidates disagree structurally, not marginally:
ast.unparsecollapses a ten-name import block and a twenty-entry__all__into oneline each. Import- and export-heavy files are the norm at a package boundary.
157 docstring lines out of 588, 42%.
black.The pattern that should decide it: three consecutive prose-only rounds moved only the
prose-sensitive instrument — 2.80→3.00x, 4.20→6.00x, 2.00→2.60x, each with zero
statement-line change. And on PR #81's final round, production AST went 145 → 146 — one
statement for eighteen physical lines, all of it the explanation a reviewer had asked
for. Eleven-plus PRs have raised a halt rather than trim a comment.
As written the rule pays agents to delete explanation. A reviewer on #67 independently
recommended SLOC with the bands re-based once (that task's 150–250 becomes roughly
258–430 at the 1.72x measured there). I have not put this in the diff because re-basing
every existing estimate is your call.
B. Should a verification task produce a package at all?
Amendment 2 stops a self-confirming package passing the gates. It does not answer whether
atom/compass/capture/should have existed. Your ruling on #10 was that the test mustdrive a real TP2 model; CAP-1 is building that now. If verification tasks should
generally leave evidence and no product code, that belongs in the rules as a task type,
and I have not assumed it. (
c409d80c7did assume it in gate 2; round 3 took that sentence out.)C. #250 — accept that this diff answers it as option 1
Unlike A and B, this one is decided by the diff, and accepting the PR accepts it. The
escalation rule says "When the ruling lives on a separate issue, label each PR it holds
anyway", and a >2x effort overrun is "an escalation, labelled as above". Landed, that
labels #81, #85 and #95
need human, each naming #89 — #250's option 1. It is also theletter of the already-landed rules text, as #250 notes. If you prefer option 2 (remove the
label from #91 and let #89 alone hold them), the "label each PR it holds anyway" sentence
is the one to change before this lands.
Deviation to declare
AI_DEV_RULES.mdsays the main agent orchestrates and does not write the change itself.The main agent wrote amendments 1–5, at your request, with all five task slots busy.
Flagging it rather than letting it pass as normal.
Amendments 6–11 and rounds 3–4 are not a deviation: a developer agent wrote them at your direct request,
which is the rule being followed. Your instruction to update this PR is what authorised
work on it while it carries
need human; the label stays, and removing it is yours.🤖 Generated with Claude Code