compass(rules): holds are the three landing preconditions; other violations are landed and filed (#257)

[jgong5]

Closes #257

These are the four follow-ups from #137's cycle-3 review (issuecomment-5787989146). Two files changed, 13 insertions and 5 deletions. Item (c) is a PR-body edit and is not in the diff.

Supersession check at 6987b0716

#315 and later PRs did not do any of the four. No open PR touches either file.

	State at the tip	Done here
(a)	L155-156 said "Any other hold names the rule in this file behind it; a rule violation seen in an approved PR is filed as an issue, not held". Nothing said which rules are holds.	New clause
(b)	16_execution_plan.md L474: "any surprise stops and is discussed"	Reworded
(c)	#137's #115 row listed three files	Body patched
(d)	L20-23 ran gh auth setup-git with no note about the gh login	Note added, marked unverified

(a) Holds are landing preconditions

The sentence at L155-156 is replaced with this:

Holds are landing preconditions, and there are three: need human on the PR or below it (every escalation rule in this file holds through this label), an APPROVE covering each head, and the tree check below. A hold names the one that is unmet. A violation of any other rule seen in an approved PR is landed and filed as an issue, not held.

The rest of the landing bullet is unchanged. That includes "A PR whose body declares an escalation without the label gets the label". No other rule's text changed.

Every rule in the file that holds a PR

Line numbers are at this head (78acec47f).

Rule	Lines	How it holds	Precondition
Escalations and need human	L102-111	The label stops all agent action, merge included	1, label
Review-loop stop (same finding for two cycles, or more than three cycles)	L112-115	Applies need human to the PR	1, label
Effort over ~2x is an escalation	L148-150	It is an escalation, so L104-105 applies the label	1, label
Stop and diagnose	L89-94	An outcome that needs an owner ruling is an escalation, so it gets the label. A finding does not hold.	1, label
A body that declares an escalation without the label	L158-159	Gets the label	1, label
Landing needs an APPROVE covering the current head, on the PR and on each unlanded PR below it	L153-158	No approval covers the head	2, approval
An approval covers a tree	L174-178	New commits need a delta review before landing	2, approval
Gate 4 and "an inert pin on a required finding blocks APPROVE"	L134-144	Acts before the approval exists	2, approval
Tree check on a moved tip	L165-170	A merged tree that differs from the reviewed head's tree needs the batch gated first	3, tree check

Two rules look like holds but are not landing rules:

• "Only open, non-draft PRs merge" (L191-192) states what gh stack merge accepts. The same line says to apply the landing rule first.
• "Never force-push a branch under review" (L196) holds nothing on its own. An amend moves the head past its approval, so precondition 2 catches it.

What the reviewer should check: the clause makes gates 1-3 (L122-133) non-holds once a PR is approved. Gate 4 is the approval, and gates 1-3 are checked before it. The one landing-time re-check is gate 1 on the combined tree, and that is part of the tree check. So an agent that thinks an approved PR misses gate 2 or 3 files an issue and lands the PR. This is what #257 asked for: "an agent could still block an approved PR by citing a content rule". Before this change, the file allowed such a hold if the agent named the rule. That is the one change in meaning, and it is the intended one.

(b) 16_execution_plan.md L474

"any surprise stops and is discussed" now reads "any surprise stops and is diagnosed, and becomes an escalation only when it needs an owner ruling". This matches AI_DEV_RULES L89-94 and L102-104. The rule was called "the halt rule", which is not a name the file uses. It is now "the stop-and-diagnose rule". The next paragraph is unchanged. It says the five triggers are escalations, and the section header already says each ends in an owner decision.

(c) #137's body

Edited with REST PATCH (-F body=@file) and read back. The original is saved at agent_scratch/compass_dev/issue-257/pr137_body_orig.md. The readback differs from the original on one line only:

-| #115 (stacked on #86) | `89ae4ff40` | plain | rc 1, CONFLICT in `spec/__init__.py`, `merge.py`, `validate.py` | `8e8a8b22e` |
+| #115 (stacked on #86) | `89ae4ff40` | plain | rc 1, CONFLICT in `spec/__init__.py`, `merge.py`, `validate.py`, `tests/compass/test_spec_verbs.py` | `8e8a8b22e` |

The fourth file comes from a local re-run. git merge-tree --write-tree --name-only 89ae4ff40 134fa56bb returns rc 1 and lists CONFLICT (add/add) for tests/compass/test_spec_verbs.py, alongside the three that were already listed. Body length went from 14634 to 14670 characters, which is the 36 characters added.

(d) gh auth setup-git after a teardown

The note is stated as unverified. No container was torn down, rebuilt or restarted. What was checked, read-only, in the running gpu_docker container:

• gh auth status reports the login as /root/.config/gh/hosts.yml.
• /root/.config is not a mount. The only mounts under /root or /workspace are /workspace and /root/.cache/huggingface.
• teardown.sh runs docker rm. setup.sh has no gh step.
• gpu_docker/CLAUDE.md names /workspace/.github-amd-token, which is on the workspace mount.

It is still untested whether setup-git actually fails after a real teardown, and whether that token file is enough to log gh in.

Gates

Gate 1: ATOM's suite, unmodified, measured as a delta. Run on node 18 in xiaobizh_n18_cpu with each tree's own scripts/compass/gate_cpu.sh. The two trees were staged with git archive into /tmp/i257gates/{control,branch}/ATOM. Each tree has a .compass-commit and a .compass-changed stamp, written from the same ref as its archive. The tarball MD5 matched on both ends. The runs went one at a time, each under timeout -k 10 2400, with output written to a file and not piped.

side	commit (gate's commit: line)	atom.__file__	result	GATE_CPU_RC
control (tip)	6987b0716	/tmp/i257gates/control/ATOM/atom/__init__.py	5263 passed, 155 skipped, 3 xfailed	0
branch	78acec47f	/tmp/i257gates/branch/ATOM/atom/__init__.py	5263 passed, 155 skipped, 3 xfailed	0

• Node-id delta from the two --junitxml files: 5421 ids on each side, none only in one side, and 0 outcomes changed. None of the known timing flakes fired on either side.
• Merged tree: git merge-tree --write-tree 6987b0716 78acec47f returns cd04df61c4b994835e9ab5cf806f7a7603c00086 with rc 0. That equals 78acec47f^{tree}, because the tip is the head's parent. The branch stamp therefore names the commit whose tree is the merged tree, and no commit-tree object was needed.
• gpu: not required (.compass-changed stamp). The two changed files match no GPU trigger.

Gate 2 (new tests) does not apply. The diff is prose in two docs. grep -rl over tests/ and scripts/ finds no file that names AI_DEV_RULES.md or 16_execution_plan.md (rc 1).

Gate 3, named result. The clause is present at L159-163. Every hold in the file maps to one of the three preconditions (table above), so an agent can classify a rule without knowing its history: it holds only if it is the label, the approval or the tree check. The (d) note is stated as unverified.

Gate 4 has not been run. The coordinator dispatches the reviewer.

🤖 Generated with Claude Code

[jgong5]

Non-blocking: the closed list scopes need human to the PR, while L107 scopes it to the issue too.

Rule quoted, L107: "The label stops all agent action on that issue or PR (no commit, review, amend or merge, even after a passed review)".

This line says the holds are exactly three, and the first is "need human on the PR or below it". At the tip, an agent seeing the label on a PR's issue only could still hold by naming L107, through "Any other hold names the rule in this file behind it". That sentence is gone. So an approved PR whose issue carries the label, but the PR itself does not, now meets all three preconditions and lands. Its Closes #N then closes the labelled issue, which is an agent action on it.

This is narrow. L106 already says to "label each PR it holds", and today no open, unlabelled PR delivers a labelled issue (I checked every open PR's body against the ten open labelled issues). The tip's own landing sentence was also PR-scoped. But the file is read literally, and "there are three" is new. One way to close it is "need human on the PR, its issue, or any PR below it". Fixing it here or filing it both work (rule: "A finding not fixed in the PR that found it gets an issue").

[jgong5]

Non-blocking: "there are three" sits beside "Four gates land a task, all required" (L121), and the file never connects them.

Rule quoted, L121: "Four gates land a task, all required:". Named result of #257: "an agent reading the file can classify any rule as a hold or a filed violation without history."

Read literally, L121 lists four things required to land, and this line lists three landing preconditions. The literal reading still resolves. Gate 4 is the approval (precondition 2). Gate 1 is re-run by the tree check (precondition 3). Gates 2 and 3 are not in the list, so a miss seen in an approved PR is "landed and filed". The PR body gives the reason that is correct: gates 1-3 are what the reviewer checks before the approval exists. But that reason is only in the PR body, which squashes away. Without it, an agent sees "four ... all required" beside "three" and has to reconstruct why.

One parenthetical would do it, for example: "(gates 1-3 are checked before the approval that is gate 4; gate 1 is re-checked by the tree check)". This does not change meaning, and it is not required to land.

[jgong5]

Non-blocking: the facts cited here check out, and one half of the "unverified" can now be dropped.

Principle 8: "Every claim carries its measurement."

Read-only checks against the running jgong5_vllm container:

• gh auth status gives Logged in to github.com account jgong5 (/root/.config/gh/hosts.yml). No GH_TOKEN or GITHUB_TOKEN is set in the environment, so the login really is that file.
• docker inspect jgong5_vllm mounts only /data, /md1/users/jgong5 -> /workspace, hf_cache -> /root/.cache/huggingface and /mnt. findmnt -T /root/.config resolves to the overlay root, so the file is not on a mount.
• teardown.sh ends in docker rm "${CONTAINER_NAME}". setup.sh contains no gh step.
• /workspace/.github-amd-token exists on the workspace mount.
• New: GH_CONFIG_DIR=<empty scratch dir> GIT_CONFIG_GLOBAL=<scratch file> gh auth setup-git prints You are not logged into any GitHub hosts. Run gh auth login to authenticate. and exits with rc 1 (gh 2.45.0). It wrote nothing, and the real global config is unchanged. So "setup-git needs gh logged in" is measured now. What is still unverified is only the teardown itself, and whether the token file is enough.

ponytail shrink: L23-26, about -1 line: "A teardown also discards the gh login (/root/.config/gh/hosts.yml), and setup-git refuses without one, so gh auth login (or the token file gpu_docker/CLAUDE.md names) first; untested after a real teardown."

[jgong5]

Review cycle 1: PR #362 (issue #257), head 78acec47fbed5c560187b17ccbdfac94923c3f6c

This review was written by an agent. GitHub refuses a formal verdict on a self-authored PR, so the verdict is the line below.

Verdict: APPROVE, at head 78acec47fbed5c560187b17ccbdfac94923c3f6c. There are no blocking issues. No owner ruling is needed. The rules-file change does what #137's approving review asked for and nothing more. There are three non-blocking findings, posted inline: label scope, gates vs three and (d) facts. Per the rules file, a finding not fixed here gets an issue.

I read these first: the eight principles in design/README.md, then all of AI_DEV_RULES.md at the tip d10cb834f and at the head, then #257, then #137's issuecomment-5787989146, then the diff. Between the merge-base 6987b0716 and the tip, neither file changed, so the tip-to-head diff of the two files is exactly this PR.

1. Sentence by sentence (AI_DEV_RULES.md, tip to head)

Head lines	Sentence	Meaning change	Within the brief?
L23-26 (new)	"setup-git needs gh logged in, ... may be needed first (unverified: ...)"	Adds an advisory. It uses "may", so it adds no obligation, and it does not change the recipe's two commands.	Yes, (d)
L26-28	"A pull as root leaves files root-owned ... chown after every pull:"	None. Same words, re-wrapped.	n/a
L159-161	"Holds are landing preconditions, and there are three: need human on the PR or below it (...), an APPROVE covering each head, and the tree check below."	Replaces "Any other hold names the rule in this file behind it", closing the list at three. Each of the three restates the tip's own landing sentence (L153-158) or the tree check (L165-170). One side effect: an issue-only label is no longer holdable by naming L107 (inline, non-blocking).	Yes, (a)
L161	"(every escalation rule in this file holds through this label)"	None. It is a true description: L104-105 applies the label on any escalation, and each escalation rule routes there (review-loop stop L112-115, effort ~2x L148-150, stop-and-diagnose outcome L94, undeclared-label L158-159).	Yes
L161-162	"A hold names the one that is unmet."	Narrows "names the rule in this file behind it" to one of the three.	Yes, (a)
L162-163	"A violation of any other rule seen in an approved PR is landed and filed as an issue, not held."	The tip already said "a rule violation seen in an approved PR is filed as an issue, not held", and L153-154 already landed any approved PR that has no label. "Landed" states what was already implied. "Any other" excludes the three preconditions, which a violation "seen in an approved PR" could not meaningfully hold anyway.	Yes, (a)
L163-164	"Where a handoff note contradicts this file, this file wins."	None	n/a

16_execution_plan.md L474-475: "The stop-and-diagnose rule ... any surprise stops and is diagnosed, and becomes an escalation only when it needs an owner ruling." This is a design doc, so no rule's meaning changes. It now paraphrases AI_DEV_RULES.md L89-94 ("stop and diagnose it ... The outcome is a finding or an escalation") and L102-104 ("An escalation is anything that needs an owner ruling"). The old name, "halt rule", was not one the rules file uses. (b) checked.

Ruling on "gates 1-3 are no longer holds once approved"

It is what the approving review asked for, and it does not weaken a rule the owner wrote.

• docs(compass): correct the stacking rules, and three gates that could not catch what happened #137's cycle-3 review, item (a): "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 ..." It proposes exactly this clause. Gates 2 and 3 are content rules in that sense. Each is a property of the PR or its issue that the reviewer checks before approving.
• The tip, as landed by docs(compass): correct the stacking rules, and three gates that could not catch what happened #137, already says "a rule violation seen in an approved PR is filed as an issue, not held". So at the tip, a gate 2 or 3 miss seen in an approved PR was already filed and not held. docs(compass): correct the stacking rules, and three gates that could not catch what happened #137 carried need human until 2026-09-23T11:52:41Z and merged 40 s later, and L110 says only the owner removes the label. The only thing removed is the competing half-sentence, "Any other hold names the rule in this file behind it". That half-sentence is the ambiguity the review flagged. The PR body calls this "the one change in meaning". It is really the tip's own second half winning over its first. That is slightly smaller than the body says, and it points the same way.
• Gate 1 is still enforced at landing through the tree check (L165-170), which gates the combined tree whenever it differs from the reviewed head's tree. Gate 4 is precondition 2 itself.
• What is left over is the textual "four ... all required" (L121) beside "there are three" (L159). That is a readability gap, not a change in meaning (inline, non-blocking).

2. Hold inventory

I checked the PR body's table against every line of the head file. Every rule that can hold a PR maps to one of the three preconditions, and the line numbers are right at the head. Label: L102-111, L112-115, L148-150, L89-94, L158-159. Approval: L153-158, L174-178, and L134-144 (an inert pin blocks APPROVE). Tree check: L165-170. The two look-alikes are also placed correctly: draft state (L191) is a gh stack merge input, and force-push (L196) is caught by precondition 2.

One gap, non-blocking: L107 scopes the label to "that issue or PR", but precondition 1 reads "on the PR or below it". See the inline comment. No live case exists today: no open, unlabelled PR names one of the ten open labelled issues with a delivering verb.

Nothing else in the file holds a PR. I checked the setup rules, delivery-check, merge-conflict, stacking, link and drift rules, and none holds a PR. The per-wave GPU superset is explicitly not waited on (L155-156).

3. (c) #137's body

diff pr137_body_orig.md <current #137 body> gives exactly one differing line, line 96, the #115 row, with tests/compass/test_spec_verbs.py added. Everything else is byte-identical. The API reports body length 14670, as claimed. I re-ran git merge-tree --write-tree --name-only 89ae4ff40 134fa56bb: rc 1, with four conflicts: spec/__init__.py (content), then merge.py, validate.py and tests/compass/test_spec_verbs.py (all three add/add). Checked.

4. (d) the rebuild note

It is stated as unverified: "(unverified: not yet tested after a real teardown)". Every fact it cites is true, and I added one measurement: gh auth setup-git with an empty GH_CONFIG_DIR exits with rc 1 and "You are not logged into any GitHub hosts". The details are inline. No container was torn down, restarted or modified. That run used a scratch config dir and a scratch GIT_CONFIG_GLOBAL. Checked.

5. ponytail-review

• shrink: AI_DEV_RULES.md:L23-26: fold the four-line note into about three lines. "A teardown also discards the gh login (/root/.config/gh/hosts.yml) and setup-git refuses without one, so log in first (gh auth login or the token file gpu_docker/CLAUDE.md names); untested after a real teardown."

The (a) clause's parenthetical is what makes the escalation rules classifiable, so it stays. The (b) line is one sentence for one sentence.

net: -1 lines possible.

6. Gate: merged tree

The tip was re-read with git ls-remote fork refs/heads/feature/atomcompass_new, which gives d10cb834f787025cf668ba2962cb5e94ab01373b.

git merge-tree --write-tree d10cb834f 78acec47f returns rc 0 and tree 34c82227a9ff88cf12542b808533ac2c41f1d9ff. That is not 78acec47f^{tree} (cd04df61c), because #357 landed on the tip after the branch point, so the merged tree was gated. The stamp is git commit-tree 34c82227a -p d10cb834f -p 78acec47f = 62c2286fc. It is an object only; no ref was created.

item	value
staged	git archive 62c2286fc, tar md5 bf222c98… on both ends, docker exec -i … tar -x into /tmp/pr362r1gates/merged/ATOM in xiaobizh_n18_cpu. The shared mount was not touched.
stamps	.compass-commit = 62c2286fc…; .compass-changed = AI_DEV_RULES.md, 16_execution_plan.md (vs d10cb834f)
atom.__file__	/tmp/pr362r1gates/merged/ATOM/atom/__init__.py
gate's own lines	commit: 62c2286fc (stamp), gpu: not required (.compass-changed stamp)
result	5263 passed, 155 skipped, 3 xfailed, 0 failed, pytest: rc=0, GATE_CPU_RC=0 PASSED. It ran in 187 s under timeout -k 10 2400 with the tree's own scripts/compass/gate_cpu.sh, unpiped.

The counts equal the developer's branch and control runs at 6987b0716 (5263 / 155 / 3). No known timing flake fired, so no re-run was needed. I ran the gate once, as briefed, with no separate tip control. The diff is prose in two docs, and git grep -E "AI_DEV_RULES|16_execution_plan" 78acec47f -- tests scripts finds no reader (rc 1). So the rc and the zero-failure count are the check.

Findings

#	Where	Severity	Principle or rule
1	L159-160: label scope, PR vs issue	non-blocking	rule L107 "stops all agent action on that issue or PR"
2	L159 vs L121: "three" beside "four ... all required"	non-blocking	#257 named result, "classify any rule ... without history"
3	L23-26: (d) facts confirmed, one half now measured, shrink	non-blocking	principle 8, principle 3

None of these needs an owner ruling. None blocks landing.