Skip to content

compass(rules): the reviewer checks gates 1-3 as they apply per task, so approval never waits for the per-wave GPU tier (#369) - #371

Merged
jgong5 merged 1 commit into
feature/atomcompass_newfrom
compass/issue-369-reviewer-gates-per-task
Sep 24, 2026
Merged

jgong5 merged 1 commit into
feature/atomcompass_newfrom
compass/issue-369-reviewer-gates-per-task

Conversation

@jgong5

@jgong5 jgong5 commented Sep 24, 2026 •

Copy link
Copy Markdown
Owner

Closes #369

What changed

atom/compass/AI_DEV_RULES.md: one clause at L161-162, +2/−2, with no other file touched. Five words are inserted: "as they apply per task". git diff --word-diff shows that one insertion and nothing else. No neighbouring line is rewrapped.

-  APPROVE covering each head (the reviewer checks gates 1-3 before approving, and
-  the approval is gate 4), and the tree check below. A
+  APPROVE covering each head (the reviewer checks gates 1-3 as they apply per task
+  before approving, and the approval is gate 4), and the tree check below. A

Sentence-level diff

The changed sentence is the three-precondition sentence at L158-162. It is split below into the clauses #367's review used. Every other sentence in the file is byte-identical.

Lines (head) Sentence or clause Old meaning New meaning
L158 "Holds are landing preconditions, and there are three:" There are three landing preconditions. Unchanged.
L159-160 "need human on the PR or below it (a label on an issue a PR delivers counts as on that PR; every escalation rule in this file holds through this label)" Precondition 1: the label, including one on a delivered issue. Unchanged; byte-identical.
L160-161 "an APPROVE covering each head" Precondition 2 is an APPROVE covering each head. Unchanged.
L161-162 "(the reviewer checks gates 1-3 as they apply per task before approving" Before approving, the reviewer checks gates 1-3. Read literally, that includes both tiers of gate 1, so a reviewer could withhold APPROVE until the per-wave GPU superset had run. Before approving, the reviewer checks gates 1-3 only in their per-task form. For gate 1, that is the GPU-free tier and the "needing to edit an ATOM test" check. For gates 2 and 3, it is all of each, because neither has a per-wave part. The per-wave GPU superset is not a pre-approval check.
L162 "and the approval is gate 4)" The APPROVE is gate 4. Unchanged.
L162 "and the tree check below." Precondition 3 is the tree check. Unchanged.
L162-165 "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. Where a handoff note contradicts this file, this file wins." As written. Unchanged; byte-identical.

Why gates 2 and 3 do not move. Every term in them is already per task:

  • gate 2 covers "what the task added";
  • gate 3 covers a result "stated in the issue body before the task is claimed". Its umbrella case, "names the child that carries it", also sits in one issue body.

So for gates 2 and 3, "as they apply per task" selects everything they contain. The qualifier removes only gate 1's per-wave tier, which is the only per-wave requirement among gates 1-3.

Named result: no reading makes a reviewer wait for the per-wave GPU superset

The three sentences, side by side (head):

Rule Text
Gate 1 (L121-122) "ATOM's test suite passes unmodified, in two tiers: the GPU-free tier per task, the GPU superset per wave as a delta."
Landing rule (L152-155) "An agent lands any PR whose APPROVE covers its current head (below) and with no need human on it or anywhere below it in its stack; it does not wait for the per-wave GPU superset."
New clause (L160-162) "an APPROVE covering each head (the reviewer checks gates 1-3 as they apply per task before approving, and the approval is gate 4)"

Why they now agree:

  1. Gate 1 assigns its two tiers to two scopes, using its own words. The GPU-free tier is "per task", and the GPU superset is "per wave".
  2. The new clause limits the reviewer's pre-approval checks to gates 1-3 "as they apply per task". It reuses gate 1's own scope word. So gate 1 contributes only its GPU-free tier, and the per-wave superset falls outside what the reviewer checks before approving.
  3. Landing needs an APPROVE and no label, and it "does not wait for the per-wave GPU superset". The APPROVE no longer depends on the superset. So no precondition waits on it, directly through landing or indirectly through the reviewer.

The file has no other route to that wait. I read every sentence that names a gate, a review or an approval: grep -n -i -E "gate|review|approv", 36 lines.

  • Gate 4 (L133-143): "looping to APPROVE as above". It names no gate-1 tier.
  • The review loop (L98-100, L111-114, L115-119): these cover when a reviewer reviews, stops and posts. None names a gate-1 tier.
  • Delta review (L175-179): covers which commits get reviewed. It names no tier.
  • The tree check (L166-171): "gate the combined tree once before landing". This is an action for the landing agent, not for the reviewer, and it sits under the landing rule, which already excludes the per-wave superset.

L161-162 was the only sentence that tied the reviewer's approval to gate 1, and it now names the per-task scope.

Ponytail shrink:: not taken

The suggested wording was "(gate 4; the reviewer checks gates 1-3 before it)", plus a reflow. Its −2 lines depend on rewrapping L162-165, which are four byte-identical neighbours. The brief allows the shrink only if it does not rewrap unrelated lines. Without the reflow it saves no line: the qualified short form still spans the same two lines. It would also swap "before approving" for "before it", whose referent is less direct. So I took the minimal insertion, and the diff stays at +2/−2 with no rewrap.

I considered a second wording, from the #367 inline r4088381014: "gate 1's GPU-free tier, 2 and 3". It is explicit, but longer, and it restates gate 1's tier name. "As they apply per task" keys on gate 1's own scope word instead, so the clause keeps following gate 1 if the tiers are ever renamed.

Gate 1: ATOM's suite, unmodified, on node 18

Setup:

  • Where: xiaobizh_n18_cpu, using the tree's own scripts/compass/gate_cpu.sh.
  • Staging: git archive of a git commit-tree stamp commit, with .compass-commit and .compass-changed written from the same ref. The tar md5 matched on both ends. It was piped through docker exec -i … tar -x into /tmp/i369gates/…. The shared mount was not touched.
  • Runs: each ran once, under timeout -k 10 2400 with --junitxml, unpiped.
  • atom.__file__: asserted under the staged root before each run, and printed again by the gate.

The tip moved from aff86031e to 177f65ef5 during the work (#365, which changes only tests/compass/test_spec_verbs.py). So I gated the merged tree against a fresh control at the new tip as well.

tip side gate's commit: line atom.__file__ result GATE_CPU_RC
aff86031e control aff86031e (stamp), .compass-changed empty /tmp/i369gates/control/ATOM/atom/__init__.py 5265 passed, 155 skipped, 3 xfailed, 0 failed (197.6 s) 0 PASSED
aff86031e merged 99f7e411a (stamp), .compass-changed = atom/compass/AI_DEV_RULES.md, gpu: not required /tmp/i369gates/branch/ATOM/atom/__init__.py 5265 / 155 / 3, 0 failed (195.5 s) 0 PASSED
177f65ef5 control 177f65ef5 (stamp), .compass-changed empty /tmp/i369gates/t2/control/ATOM/atom/__init__.py 5265 / 155 / 3, 0 failed (191.2 s) 0 PASSED
177f65ef5 merged 17df04a99 (stamp), .compass-changed = atom/compass/AI_DEV_RULES.md, gpu: not required /tmp/i369gates/t2/branch/ATOM/atom/__init__.py 5265 / 155 / 3, 0 failed (197.8 s) 0 PASSED
  • Count delta: zero in every category, at both tips.
  • Node-id delta: I compared the junit files by classname::name and outcome. Each side has 5423 ids: 5265 passed and 158 skipped (junit records the 3 xfailed as skipped). At both tips: 0 only in the control, 0 only in the merged tree, 0 with a changed outcome.
  • Merged tree:
    • At aff86031e, git merge-tree --write-tree aff86031e 805fb0e82 returns rc 0 and f5297f2051c06845cf95b2ec4e4b0174f0edfcec, which equals 805fb0e82^{tree}.
    • At 177f65ef5, git merge-tree --write-tree 177f65ef5 805fb0e82 returns rc 0 and 5e0a16cc4665b0e57aab42317ccae23a10e7e6d2. That is the tree gated as 17df04a99.
  • Stamp commits: 99f7e411a and 17df04a99 are objects only. No ref was created for them.
  • Timing flakes: none of the named timing tests fired on any of the four runs, so no re-run was needed.
  • Nothing reads the file: git grep AI_DEV_RULES -- tests scripts returns rc 1 at the head.
  • Other open PRs: none touches AI_DEV_RULES.md. I checked the file lists of all open PRs over REST.

Gate 2 does not apply: the change is to a rules document, with no code and no tests.

Dev record

🤖 Generated with Claude Code

…es per task (#369)

The landing precondition said the reviewer checks gates 1-3 before
approving. Gate 1 has two tiers, the GPU-free tier per task and the GPU
superset per wave, so a literal reader could withhold APPROVE until a
per-wave GPU delta existed. The landing rule says landing does not wait
for that superset.

Qualify the clause with "as they apply per task". Gate 1 applies per task
only as its GPU-free tier, so the per-wave superset is no longer a
pre-approval check. Two lines change, and no neighbouring line is
rewrapped.

The optional ponytail shrink is not taken. Its two-line saving needs
four byte-identical neighbour lines rewrapped, and without that reflow it
saves no line.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
on that PR; every escalation rule in this file holds through this label), an
APPROVE covering each head (the reviewer checks gates 1-3 before approving, and
the approval is gate 4), and the tree check below. A
APPROVE covering each head (the reviewer checks gates 1-3 as they apply per task

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking (follow-up issue, not a change to this PR): after this clause is narrowed, no sentence in the file says who runs gate 1's per-wave GPU superset, or when.

Rules quoted:

  • L121-122: "the GPU-free tier per task, the GPU superset per wave as a delta."
  • L154-155: "it does not wait for the per-wave GPU superset."
  • Principle 6: "Refuse rather than fall back. A declined answer with a named reason is a result. A guessed one is a defect."

What I checked. grep -n -i -E "gpu|superset|wave|tier" at the head gives only L121-122 and L155 for the superset. L120 still says "Four gates land a task, all required", and L155 exempts landing from the superset. Before this PR, the only reading that gave the superset an owner was the literal one at L161: the reviewer runs it before approving. That reading contradicted L154-155, and this PR removes it, as #369 asked.

Why it does not block. It drops no duty a reviewer has in practice. That reading could never be followed without breaking L154-155, and the PR's own named result is that the reviewer does not wait. The gap was already in the file. This PR only makes it visible.

Suggested follow-up. File an issue asking the owner or planner who runs the per-wave delta, and when it runs, for example the landing agent after a wave's last landing. Until that is answered, a literal reader will find the tier required at L120 with nobody assigned to run it.

@jgong5

jgong5 commented Sep 24, 2026

Copy link
Copy Markdown
Owner Author

Agent-authored review (cycle 1). I read the eight principles in atom/compass/design/README.md and atom/compass/AI_DEV_RULES.md in full, at tip 8c0a2e6ee and at head 805fb0e82.

Verdict: APPROVE for head 805fb0e822485ba28f85f0054221354eadb19f84. There are no blocking findings, and no owner ruling is needed. One non-blocking finding is posted inline at L161: the per-wave superset has no named owner. It asks for a follow-up issue, not a change to this PR.

1. Word diff and meaning

  • Diff: git diff --word-diff 805fb0e82^ 805fb0e82 has exactly one change, {+as they apply per task+}, in the L161 hunk. --word-diff=porcelain has one + token and no - token. The +2/−2 comes from "before approving, and" wrapping onto L162. No other word moves.
  • Tip against head: diff of the file at 8c0a2e6ee and at 805fb0e82 differs only at L161-162. The tip has not touched the file since the base aff86031e.
  • Meaning: only precondition 2's parenthetical changes. The label precondition (L159-160), gate 4 as the approval, the tree check, and L163-165 are byte-identical and unchanged in meaning. The PR body's sentence table is accurate.

2. Ruling on "as they apply per task" (principle 6)

Principle 6: "Refuse rather than fall back. A declined answer with a named reason is a result. A guessed one is a defect."

  • It is gate 1's exact scope word. L121-122: "the GPU-free tier per task, the GPU superset per wave as a delta." The new clause reuses "per task" word for word. So it selects the GPU-free tier and leaves out the per-wave superset, using gate 1's own split.
  • The "edit an ATOM test" rule survives. L122-124: "Needing to edit an ATOM test means the change altered ATOM's behaviour and must be justified on its own terms." Its subject is "the change", which is the task's diff. It is a property of the diff, not of a suite run, so it is per task whichever tier the edited test belongs to. An edit to a GPU-only test still sits in the task's diff and is still caught.
  • Gate 2 survives in full. L125-128: "New CPU-only tests for what the task added … must exercise something the PR did not itself add." Every term is per task.
  • Gate 3 survives in full. L129-132: "stated in the issue body before the task is claimed … names the child that carries it." Every term is per task.
  • Gate 4 is untouched. The qualifier covers only "gates 1-3". Gate 4's own pre-approval duties stay: ponytail-review, "seen it fire", and "An inert pin on a required finding blocks APPROVE" (L143).
  • A reading I considered and rejected. "As they apply" could be read as "insofar as they apply", which would let a reviewer declare a gate inapplicable. That gives no new discretion. What applies to a task is still fixed by each gate's own text, and gates 2 and 3 are wholly per task. The only thing the qualifier can remove is the per-wave tier.
  • Ruling: the clause drops no duty a reviewer can perform today. The superset could never be a pre-approval check without breaking L154-155. No owner ruling is needed.

3. The three-sentence argument, and a sweep

  • The argument holds. Gate 1 puts its tiers into two scopes. The clause checks only the "per task" scope before approving. L152-155 lands on APPROVE plus no label, "it does not wait for the per-wave GPU superset." So the superset gates neither landing nor approval.
  • Sweep: grep -n -i -E "gate|review|approv" gives 36 lines, which matches the PR body's count. I read each one. No other sentence ties approval to the superset:
    • L100, L111-119 and L133-143 cover the loop, where it stops, where it posts, and gate 4's own duties. They name no gate-1 tier.
    • L175-179 covers delta review and restacks. It names no tier.
    • Two sentences tie landing, not approval, to gates in general:
      • L120: "Four gates land a task, all required."
      • L169-171: "the gate stands … gate the combined tree once before landing."
        Both sit under L154-155's explicit exemption, so neither is a route to the wait.
    • grep -i -E "gpu|superset|wave|tier" finds the superset only at L121-122 and L155.
  • Non-blocking, posted inline at L161 (r4088558296): after this PR, nothing in the file says who runs the per-wave superset delta, or when. The gap was already there. This PR removes the one reading that pointed at the reviewer, and that reading contradicted L154-155. I suggest a follow-up issue.

4. ponytail-review over the diff

I read the raw SKILL.md. The diff adds five words.

Lean already. Ship.

5. Gate: the merged tree at the current tip, gated once

Item Value
Tip, re-read just before gating 8c0a2e6eefc7d75cc84868a209c11eb8044d40de (#355)
git merge-tree --write-tree 8c0a2e6ee 805fb0e82 rc 0, 70ddb5fcc66f1053711742078054793639731419
That tree against the tip's tree atom/compass/AI_DEV_RULES.md only, +2/−2
Stamp git commit-tree → d0ed42e57 (parent 8c0a2e6ee), an object in my scratch repo with no ref. .compass-commit = d0ed42e57; .compass-changed = atom/compass/AI_DEV_RULES.md
Staging git archive → tar md5 d5f55a74bddca60cf5162cc653b44647, the same on both ends. It went through docker exec -i into xiaobizh_n18_cpu:/tmp/pr371r1gates/ATOM. The shared mount was not touched.
atom.__file__ asserted, then printed as /tmp/pr371r1gates/ATOM/atom/__init__.py. The gate printed the same path, plus commit: d0ed42e57 (stamp) and gpu: not required
Run the tree's own scripts/compass/gate_cpu.sh --junitxml, under timeout -k 10 2400, unpiped
Result 5268 passed, 155 skipped, 3 xfailed, 0 failed (197.2 s). GATE_CPU_RC=0 PASSED
junit 5426 testcases (5268 + 155 + 3), 0 <failure>

The +3 against the PR body's 5265 is explained by the moved tip. #355 (177f65ef5..8c0a2e6ee) added eos_token_id, stop_token_ids and pipeline_parallel_size to the fields read with no default. So test_runner_rpc_surface.py::test_each_config_field_read_with_no_default_is_one_atom_declares now has 6 parameters. The junit lists all 6, the three new ones included.

git grep AI_DEV_RULES -- tests scripts on the merged tree returns rc 1, so no test reads the changed file. None of the named timing tests fired, so no re-run was needed.

Blocking: none. Non-blocking: 1, inline at L161 (the per-wave superset has no named owner; needs a follow-up issue). No owner ruling needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant