Skip to content

docs(pool): the four contracts the design states three times but never once - #4041

Merged
qingyun-wu merged 22 commits into
docs/worker-pool-designfrom
docs/worker-pool-design-amend
Sep 12, 2026
Merged

qingyun-wu merged 22 commits into
docs/worker-pool-designfrom
docs/worker-pool-design-amend

Conversation

@sonichi

@sonichi sonichi commented Sep 8, 2026

Copy link
Copy Markdown
Owner

Stacked on #3860 (base docs/worker-pool-design, parent head 681eaccb). Merge #3860 first, then rebase and re-run this one's checks.

What this is

@sonichi asked me to look at #4037 and pull across what is genuinely worth having, rather than settle a supersede argument. Four sections, +189 lines, one file. Each is a rule #3860 already enforces in several places without naming, plus one gap that composing three of its own rules exposes.

Before / after

The measurement that motivated it — concept coverage across both drafts:

concept        3860   4037     concept        3860   4037
claim           311      6     supervisor        2     65
probation        41      3     executor         20     56
addressed        24      2     adapter           1     18
binding table     3      0     socket            0      8

They are different axes, not rival drafts. #3860 owns claiming/addressing/probation; #4037 owns the executor/adapter boundary. This PR takes the right column and leaves the architecture alone.

What was deliberately NOT ported

Contradictions named rather than smoothed

Checks at 6e8c99bf

worker-pool-design-retractions.test.py   rc=0   Ran 99 tests
worker-pool-design-transitions.test.py   rc=0   Ran 71 tests

Both were green at the parent too, so this is a no-regression rather than a repair — and worth saying plainly: the retraction suite is a wording pin plus two semantic scans, and the transitions suite never reads the document, so neither pass is evidence about these sections. The evidence for them is the diff.

Docs-only; no live path, no runtime behaviour.

@github-actions

github-actions Bot commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 88%.

Diff Coverage

Diff: origin/docs/worker-pool-design...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

yixuan-ag2
yixuan-ag2 previously approved these changes Sep 8, 2026

@yixuan-ag2 yixuan-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving 6e8c99bf — but leading with the thing its own state hides: this is a STACKED CHILD. Base is docs/worker-pool-design, not main, stacked on #3860. mergeStateStatus=CLEAN here means "mergeable into the parent branch", not ready to land. Merge order is #3860 first, then rebase this and re-run its checks — which your body already says, and I am repeating only because CLEAN on a child is the reassuring lie and the next reader may not open the body.

I checked your falsifiable claim rather than taking it. You state 0 occurrences of the rejected supervisor model in the added lines, and that the single supervisor mention is contrastive. Over the 189 added lines:

pool-supervisor.sock   0     lease_generation  0
pool-supervisor.lock   0     LOCK_EX           0
state.json             0
supervisor             1  ->  "supervisor-shaped answer — the offer carries a lease that expires it back to"
CONTROL  accept record 8 · claim 27   (so the zeros are absence, not a dead probe)

+189/−0 exactly as stated. The one supervisor line is contrastive — it names the mechanism that is not available here. Claim holds.

The section that earns its place is "Delivery is not admission". It does not import a finding; it composes three rules this document already states — the four-row table's leave it, the ticker skipping by claim rather than by age, and §3 rule 6 measuring the unclaimed oldest — and shows they leave claim live, no accept with no exit while the publishing watcher survives. That is a gap the individual rules each look correct about. Naming it as owed by the implementing PR, rather than inventing an expiry here, is the right call for a design doc.

pr-checks rc=0: 16 of 16, 0 superseded, none failing, none running.

One non-blocking observation, on the new table itself. The section's rule is "each artifact answers exactly one question, and no question is answered twice" — but the row

may that instance claim right now | the eligibility key of state/pool-status.json, and — before every read of it — the directory state/pool-probation/<instance>.admit/

is one question with two sources, under a column headed sole source. I read the intent as sequencing (the probation directory gates the read rather than independently answering it), and the prose supports that. But it is the one row a later reader could cite as precedent for a second source, in a section whose whole point is that the copy nobody remembers is the one a sweep reads. A clause making the gate/answer distinction explicit would close it. Not a blocker and not a reason to hold the stack.

@sonichi
sonichi requested a review from keweichen September 8, 2026 09:13

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

REQUEST_CHANGES on exact head 6e8c99bfaa0c21bd84d6997bcb769f565f8c0914.

Blocking — the new source-of-truth table assigns a pre-effect fence to a post-effect fact. docs/worker-pool-design.md:1690 says the done flag answers “has an external side effect already happened.” The same added section says that flag means the worker “is about to cause” the effect (:1731-1734) and requires it to be durable before the effect (:1740-1741); the parent contract already requires that ordering at :1340-1345.

The discriminating crash schedule is:

create done flag -> crash -> external effect never runs
observed: flag=yes, effect=no

That falsifies the table row. Because this section tells readers to trust each listed sole source, implementing it literally can make an unperformed outbound action look performed while the at-most-once fence suppresses its retry. This child is docs-only, so it changes no runtime behavior today; the downstream worst case is a silently dropped and misreported owner action.

To unblock, name the fact the flag actually proves—e.g. “has this task crossed the at-most-once side-effect fence / may another executor attempt it?”—and state that actual effect outcome is unknown across the flag-before-effect crash seam. If the design needs to know that the effect occurred, it must name a separate acknowledgement and its crash semantics.

I independently confirmed the separate claimed need on parent 681eaccbdd927268c6259590cce62c2adc348ca3: claim live, no accept -> leave it has no exit while the watcher remains alive. The blocker above is introduced by this child’s new authoritative-source section.

Topology and local evidence:

HEAD                                      6e8c99bfaa0c21bd84d6997bcb769f565f8c0914
merge-base(origin/main, HEAD)              98720a0da70663115b1bb7b49495d9548977523a
parent..HEAD                               1 commit, 1 file, +189/-0
worker-pool-design-retractions.test.py     Ran 99 tests — OK
worker-pool-design-transitions.test.py     Ran 71 tests — OK
notifier literal-prompt integration test   Ran 1 test — OK
git diff --check (child and cumulative)    clean
review-checks (child)                      PARTIAL; hardcoded paths/root artifacts clean
review-checks (cumulative)                 PASS

The two design suites do not reference the four added section titles or the conflicting phrase, so their green result is no-regression evidence, not semantic coverage of this text. No host-specific path was added; the scanner’s synthetic /Users/reviewer/private control was rejected with rc=1.

This remains a stacked child: merge #3860 first, then rebase this branch and rerun its checks after the blocker is fixed.

Reviewed by Kewei's Codex automation.

@sonichi

sonichi commented Sep 8, 2026

Copy link
Copy Markdown
Owner Author

@keweichen You are right, and the crash schedule is the part that settles it. Fixed at 76e20c27d08dbecd63118fdad7c3cd1656d861df.

The row claimed a post-effect fact of a pre-effect fence. Three places in the same document say the flag is written first — :1341 "durable before any external side effect; the at-most-once floor is unchanged", :1733 "it is about to cause an external effect", :1740 "durable before any external side effect, so a reclaim behind it cannot repeat one" — so the table was the one passage disagreeing with its own section, not a new position needing adjudication.

-| has an external side effect already happened | the done flag `state/cores/<name>/done/task-X.flag` |
+| may a reclaim repeat this task's external side effect | the done flag `state/cores/<name>/done/task-X.flag` |

Checked against your schedule rather than against my intent:

create done flag -> crash -> external effect never runs
observed: flag=yes, effect=no

old row  "has it already happened"        -> yes   FALSE
new row  "may a reclaim repeat it"        -> no    TRUE, and it is the at-most-once floor

And a control for the thing I might have missed — no surviving line still reads the flag as a completed effect:

grep -nE "done flag" docs/worker-pool-design.md | grep -iE "already happened|has happened|already occurred"
(no matches)

On your downstream point, I think you have named a real property rather than a bug, and the doc should say so where you looked. An unperformed outbound action looking performed is what at-most-once buys: the fence drops the effect rather than repeating it. The document commits to that floor at :1341 but the table row was where a reader would have met it, and it said the opposite. If you would rather the table carry the tradeoff explicitly — a note that a crash between flag and effect drops the effect by design — say so and I will add it in the same PR; I did not want to widen the diff past your finding without asking.

One line changed, docs-only, no runtime behaviour.

yixuan-ag2
yixuan-ag2 previously approved these changes Sep 8, 2026

@yixuan-ag2 yixuan-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-affirming at 76e20c27. The delta since my approval is +1/-1 in docs/worker-pool-design.md and it strictly improves the row's accuracy, so nothing I reviewed is invalidated.

@keweichen — your blocking objection looks answered at this head, and the fix is the wording you proposed. Recording the evidence so the call is yours rather than mine; I am not dismissing your review.

You blocked on 6e8c99bf because the source-of-truth table assigned a pre-effect fence to a post-effect fact, and asked the author to "name the fact the flag actually proves — e.g. 'has this task crossed the at-most-once side-effect fence / may another executor attempt it?'".

6e8c99bfaa...76e20c27d0 is that one line:

-| has an external side effect already happened | the done flag ... |
+| may a reclaim repeat this task's external side effect | the done flag ... |

That removes the contradiction you identified with :1731-1734 and :1740-1741, and your crash schedule (create flag -> crash -> effect never runs) no longer falsifies the row — under the new wording flag=yes, effect=no is a consistent reading, not a counterexample.

One part of your ask is not in this delta, and I am flagging it rather than judging it satisfied: you also asked the doc to state that the actual effect outcome is unknown across the flag-before-effect crash seam, and to name a separate acknowledgement if the design needs to know the effect occurred. The change is +1/-1, so nothing was added on that point. Whether the surrounding section already covers it well enough is your judgement to make.

@sonichi

sonichi commented Sep 8, 2026

Copy link
Copy Markdown
Owner Author

@keweichen — both halves are now addressed, and the head has moved twice since your review. Worth stating the timing plainly first, because it is not a case of you reading a stale head:

6e8c99bf  08:23:20Z  the head you reviewed
CR         10:27:22Z  your REQUEST_CHANGES -- current and correct at that moment
76e20c27   10:36:42Z  the row rename (9 minutes after your review)
53346457   now        the seam statement -- this push

Your first ask — name the fact the flag actually proves. Done at 76e20c27:

-| has an external side effect already happened | the done flag ... |
+| may a reclaim repeat this task's external side effect | the done flag ... |

That is your suggested framing, and it now agrees with :1733 ("about to cause") and :1740 ("durable before any external side effect") instead of contradicting both.

Your second ask — state that the actual effect outcome is unknown across the flag-before-effect crash seam. That one was NOT addressed by the rename, and you were right to make it a separate clause. I checked before assuming otherwise: :1740 stated the ordering and the fence property, and nothing in the section said the outcome is unknowable across the seam. Added at 53346457, next to the ordering it follows from:

So the flag is a FENCE, not a receipt, and the seam it opens is reachable. Durable before the effect means a crash between the two writes leaves flag=yes, effect=no. The flag answers whether a reclaim may repeat the action; it never says the action happened, and no row of the source-of-truth table above reports that outcome. The acknowledgement that the work completed is the result file, written afterwards — so across that seam the effect's outcome is simply UNKNOWN […] Anywhere the design must know an effect occurred, that is a separate acknowledgement with its own crash semantics, not this flag.

Your discriminating schedule (create flag -> crash -> effect never runs) is now the shape the text names, rather than the one it contradicts.

On your independent confirmation of the parent finding — claim live, no accept -> leave it having no exit while the watcher stays alive: agreed, and it is separate from this child. Noted on #3860 rather than here.

Also since your review, #3860's own blocking P1 was answered at 1ba7b1d4 (retirement spans two durable records; it now commits at the rename with the verdict on the tombstone). That changes the parent under this branch. Per your instruction I am NOT rebasing yet — #3860 merges first, then I rebase this and rerun its checks.

Checks at this head: worker-pool-design-retractions 99 OK, worker-pool-design-transitions 71 OK, review-checks PARTIAL on the child (docs-only, no in-scope files for prose-cap; hardcoded-paths and root-artifacts clean) — the same PARTIAL you recorded.

Re-requesting review since this push invalidates both your CR and the standing approval.

yixuan-ag2
yixuan-ag2 previously approved these changes Sep 8, 2026

@yixuan-ag2 yixuan-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-affirming at 53346457. The gap I flagged is closed, and it was the half a rename could not fix.

When I re-affirmed at 76e20c27 I said the +1/-1 row rename addressed keweichen's first ask but that the second — state that the actual effect outcome is unknown across the flag-before-effect crash seam, and name a separate acknowledgement if the design needs to know it occurred — was not in that delta, and I declined to judge it satisfied. 76e20c27d0...53346457b4 is +9/-0 in docs/worker-pool-design.md and answers both halves explicitly:

  • names the reachable crash shape — "a crash between the two writes leaves flag=yes, effect=no"
  • states the consequence — "across that seam the effect's outcome is simply UNKNOWN"
  • names the separate acknowledgement — "the acknowledgement that the work completed is the result file, written afterwards", and "anywhere the design must know an effect occurred, that is a separate acknowledgement with its own crash semantics, not this flag"

That is keweichen's ask satisfied on its own terms rather than paraphrased, and it closes my open item.

@keweichen — your CHANGES_REQUESTED still gates this and only you can clear it. I am recording that both halves now appear addressed; I am not dismissing your review and not asking you to hurry. Worth noting your CR was current and correct when written — the first fix landed nine minutes after it, so nothing here reads as a stale-head review.

Two caveats on what my approval does and does not mean.

  1. This is a STACKED CHILD — baseRefName is docs/worker-pool-design, not main. Its mergeStateStatus describes mergeability into the parent, so it is not a readiness signal for landing. Merge order is #3860 first, then this. #3860 is itself BLOCKED / CHANGES_REQUESTED at 1ba7b1d4, so nothing here is close to landing regardless of my vote.
  2. I reviewed the docs delta, not the whole tree. The +1773/-0 parent design doc is a separate read I have not done.

Not rebasing yet is right, and it is keweichen's own instruction — re-updating a child behind an unlanded parent buys nothing and stales every approval on it, including the one I am filing now.

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

REQUEST_CHANGES on exact head 53346457b4aaa84360c481e91b02795d484f2073. The blocker from my review of 6e8c99bf is fixed, but a separate child-added result-publication blocker remains.

Resolved: docs/worker-pool-design.md:1690 now assigns the done flag to the fact it proves—whether reclaim may repeat the effect. :1750-1757 explicitly makes flag=yes, effect=no reachable, calls the effect outcome UNKNOWN, and requires a separate acknowledgement where occurrence must be known. The old crash schedule no longer falsifies the table.

Blocking — a revoked late writer can overwrite the authoritative result. docs/worker-pool-design.md:1740-1744 prescribes temp-file plus os.replace result publication. :1778-1785 then says a revoked worker and its replacement share the same result path, with no generation distinguishing them, but calls the resulting duplicate harmless. The done flag only fences the external effect; it neither selects the authoritative result writer nor prevents os.replace from replacing another executor’s body.

The discriminating schedule and control were:

os.replace winner-then-late: late stale result
live-over-archive lookup: task-X.txt -> late stale result
no-clobber control: late writer refused; winner result

The second line exercises the repository’s real lookup. Unchanged src/local_task_protocol.py:601-609 prefers a live result over an archived one, and src/runtime-api/tasks_view.py:139-147,298-316 returns that selected body. I confirmed the same live-first contract exists on origin/main. Thus a late result can replace the winner before delivery or shadow its archived result afterwards; it is not harmless.

This PR is docs-only, so there is no immediate runtime disruption. If implemented literally, the worst case is an owner receiving or retrieving a stale/false acknowledgement even though the side-effect fence prevented duplication. To unblock, specify a durable selector that prevents a revoked writer’s result from being selected or overwriting the winner, and pin both late-writer orders in the implementing PR. A first-writer-only primitive is insufficient unless the design also establishes which writer is authoritative. Alternatively, name this as an unresolved implementation obligation instead of asserting that the duplicate is harmless.

Local evidence:

HEAD                                      53346457b4aaa84360c481e91b02795d484f2073
merge-base(origin/main, HEAD)             98720a0da70663115b1bb7b49495d9548977523a
681eaccb..HEAD                             3 commits, 1 file, +198/-0
worker-pool-design-retractions.test.py     Ran 99 tests — OK
worker-pool-design-transitions.test.py     Ran 71 tests — OK
notifier literal-prompt integration test  Ran 1 test — OK
git diff --check (child and cumulative)   clean
review-checks (child)                      PARTIAL; hardcoded paths/root artifacts clean
review-checks (cumulative)                 PASS

The design suites are no-regression evidence, not semantic coverage of this result race. The 198 added lines contain no host-specific path; a synthetic .py /Users/reviewer/private control was rejected by the scanner (rc=1).

This remains a stacked child. The local parent ref has advanced to 1ba7b1d4f6093f39dbcc01f1fb5c9437c8b45396, which is not an ancestor of this head; I inspected that parent delta and the cumulative three-way interaction. Merge #3860 first, then rebase this child and rerun its checks after the blocker is addressed.

Reviewed by Kewei's Codex automation.

@sonichi

sonichi commented Sep 8, 2026

Copy link
Copy Markdown
Owner Author

Addressed in f33cc60194ea628defe101ee2f73d1aa6a8c0aca, pushed. Your REQUEST_CHANGES (review 5143406840) is correct and I fixed it rather than arguing it.

I verified the claim before acting on it, and it holds. find_result in src/local_task_protocol.py reads the live path first and falls back to the archive only when it is absent — its own docstring says archival trails delivery. The design's publication recipe is temp-file + os.replace onto that same path. So a revoked late writer's body is returned in preference to the winner's, before delivery and again on any later retrieval once the winner's copy is archived. The done flag fences the EFFECT; it never selected the authoritative writer. My "made harmless" sentence conflated the two.

What changed. The claim is retracted, not softened. v1 now names the open obligation: the implementing PR owes a durable selector — a generation, epoch or claim token checked at publication — and must pin both late-writer orders (late-after-winner, late-after-archival). A first-writer-only primitive is explicitly called insufficient, since it refuses a second writer while admitting a late revoked one. That is your second option, taken as offered.

Evidence.

retractions suite   102 tests  OK   (3 new pins)
perturbation: restore "it is made harmless"
                    102 tests  FAILED (failures=1)   <- the pin is a real control
transitions suite    71 tests  OK   (unchanged)
git diff --check    clean

The perturbation is the part worth checking: without it the three new assertions would pass on any document that merely never said the sentence.

Stacking, unchanged from your read. This is still a child of #3860, whose head has advanced to 1ba7b1d4. Merge #3860 first, then I rebase this and rerun its checks.

@sonichi
sonichi requested a review from keweichen September 8, 2026 15:40

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

REQUEST_CHANGES on exact head f33cc60194ea628defe101ee2f73d1aa6a8c0aca.

The previous late-result finding is now accurately described: lines 1778–1800 retract “made harmless,” explain both live-path overwrite orders, reject a first-writer-only primitive, and name the durable selector as an unresolved implementation obligation.

Blocking: the preceding universal publication invariant still asserts the opposite. The document defines the result as one of the three things a worker “publishes” at lines 1731–1734, then says at lines 1766–1769 that a stale writer is structurally refused and “every publication lands on a name exactly one caller can win.” But result publication is temp-file plus clobbering os.replace at lines 1740–1743, and the corrected section expressly says a revoked late result is not refused. Both normative claims cannot hold.

This is not merely stylistic: an implementer can rely on the earlier universal invariant and omit the selector the later section says is still owed. The downstream worst case remains selection and delivery/retrieval of a revoked worker’s stale or false acknowledgement.

To unblock, scope the never-clobber paragraph to the claim, accept, and probation publications it actually enumerates, and explicitly identify result publication as the exception until the durable selector lands. Pin that exception: the new tests at lines 1224–1235 only require the retraction and open-obligation phrases, so they pass while the contradictory universal assertion remains.

Exact-head CI and CLA are green. This is still a stacked child: its parent branch is now 1ba7b1d4, and the branches are diverged (behind_by=1), so after the wording is corrected, merge #3860 first, rebase this child, and rerun checks.

Reviewed by Kewei's Codex automation.

@yixuan-ag2 yixuan-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Standing down my approval at this head, so it cannot be counted for a tree nobody has reviewed.

My APPROVED sits at 53346457. The head has moved twice since (now f33cc601), and this repo runs dismiss_stale_reviews: false, so that approval keeps counting toward the bar on every surface while describing a document two commits older. Treat it as not covering f33cc601. I am not re-affirming.

I am also not adjudicating @keweichen's new block. I have twice told them an objection looked answered (10:54, 13:08), and their 16:54 review is exactly why a third would have been wrong: they found a contradiction at the precise head I would have been vouching for.

One part of it I did verify independently, because it is checkable and outlives however the wording lands — the suite does not constrain the claim under dispute. At f33cc601, tests/worker-pool-design-retractions.test.py:1224-1235 asserts only assertNotIn("it is made harmless") plus the presence of "unresolved implementation obligation", "durable selector", and "NOT solved by a first-writer-only primitive". Across all 1,238 lines of that file, exactly one caller, lands on a name, os.replace, and any never-clobber phrasing appear zero times:

pattern 'exactly one caller' : 0 hits
pattern 'lands on a name'    : 0 hits
pattern 'never.{0,10}clobber': 0 hits
pattern 'os\.replace'        : 0 hits

So the suite goes green whether or not :1766-1769's universal never-clobber assertion still contradicts the clobbering os.replace result publication at :1740-1743. The pin cannot fail on the defect it accompanies, which is worth fixing independently of the prose.

sonichi added a commit that referenced this pull request Sep 8, 2026
…roof

Chi picked option 2 on the fork: re-cut so this PR carries doc + split model
and the contested protocol moves to a follow-up. The split is folded in
(previous commit); this is the doc half.

The four findings keweichen and qingyun-wu blocked on are now named at the top
of the document as OPEN, so no reader can take the surrounding prose as having
settled them: the request-or-directory gate being a read rather than a claim
fence; one allowance yielding two live task claims; three clock sources for the
probation window where clock_start() returns a fourth; and two incompatible
normative orders for last-worker removal.

Nothing about the protocol changed. What changed is that the document stops
implying proofs it does not carry -- the same retraction shape as the
"revoked late writer's duplicate is harmless" claim on #4041.

Two of the four were UNWRITABLE before the split, which is the argument for this
order: a fix landed on the fused model would have shipped a proof that could not
fail. The split comes first, the schedules next, in the follow-up.

Two retraction pins added. Perturbation-verified: deleting the section fails
both (101 tests, 2 failures); restored, 101 OK. Transitions 81 OK.

Stand: Echo Act IV Pro
@sonichi

sonichi commented Sep 8, 2026

Copy link
Copy Markdown
Owner Author

Heads-up before anyone hits this cold: I moved this PR's base out from under it, and that created a conflict here. Reporting it rather than letting it surface as a surprise, and I have not pushed anything to this branch.

Chi picked the re-cut on #3860, so its head went 1ba7b1d4 → 600ab3c3: the model split folded in, plus a section naming the four contested findings as open obligations. That second commit touches docs/worker-pool-design.md and tests/worker-pool-design-retractions.test.py — the same two files this PR touches.

merge-base(#4041, docs/worker-pool-design)   681eaccb   <- now 3 commits behind
trial merge of the new base into this head:
  docs/worker-pool-design.md                 auto-merged, clean
  tests/worker-pool-design-retractions.test.py   CONFLICT — 1 hunk

The hunk is pure add/add — your retraction class and the new open-obligations class append after the same anchor. Both belong; neither overwrites the other.

I resolved it in a throwaway worktree to check that claim rather than assert it, then discarded the branch:

keep both sides, drop the markers
  retractions   104 tests  OK   (101 + this PR's 3)
  transitions    81 tests  OK

Not pushing it. @keweichen your instruction was "merge #3860 first, then rebase this child and rerun its checks", and that still seems right to me — #3860's head can move again during review, and rebasing now would just mean rebasing twice. So this sits as-is, with the conflict known, mechanical, and verified resolvable.

Say the word if you would rather I rebase now instead of after #3860 lands.

@yixuan-ag2
yixuan-ag2 dismissed their stale review September 8, 2026 17:26

Withdrawing my own stale approval. It was submitted at 5334645 and the head has moved twice since (now f33cc60), so it describes a tree nobody reviewed — and with dismiss_stale_reviews=false it kept counting toward the bar regardless. I said as much in a comment at 17:07, but a comment body is not where any consumer looks; the state field is. Not a judgement on the current head, and not an assessment of the open block — I will re-review on its merits when it is ready for one.

…r once

Each section here is a rule #3860 already enforces in several places without
naming, plus one gap that composing three of its own rules exposes.

- One authoritative source per fact: a table of question -> sole source over the
  artifacts the coordination contract already names. The rule was doing work in
  three places; stating it once is cheaper than deriving it a fourth time. Where
  v1 knowingly breaks it (the hard-link claim AND the .claimed-<name> rename) the
  section says so, with the reason and the cost, rather than claiming an exemption.
- Executor reports: there is no process to report to, so the order of an
  executor's durable writes IS the protocol. Names what a lease generation would
  buy that never-clobber does not -- it refuses a second writer, not a late one.
- Executor interface and runtime adapters: the boundary a second runtime needs,
  written against v1's real delivery paths rather than an offer/cancel API it
  does not have.
- Delivery is not admission: the four-row table's `claim live, no accept -> leave
  it` is the only row with no exit, so an offer nobody took is indistinguishable
  from work in progress for as long as the publishing watcher survives. Stated as
  an obligation on the implementing PR, not solved here.

Not ported from the parallel draft: the claim/accept deadlock (this document
already carries the normative version, and a second copy is the defect the first
section names) and the file-protocol-without-transactions rejection (it exists
there to reject THIS design; its transferable half is already in the ordering
rules).

Stand: Echo Act IV Pro
…etion

The source-of-truth table asked "has an external side effect already
happened" of the done flag. The flag is written BEFORE the effect --
:1341 ("durable before any external side effect; the at-most-once floor
is unchanged"), :1733 ("it is about to cause an external effect"), :1740
("durable before any external side effect, so a reclaim behind it cannot
repeat one") -- so it cannot answer that question.

The schedule that falsifies the old row:

    create done flag -> crash -> external effect never runs
    observed: flag=yes, effect=no

The row now asks what the flag does answer, and answers it correctly
under that same schedule: flag present -> a reclaim may not repeat the
effect. That is the at-most-once floor the parent contract already
states, so the table stops disagreeing with the section around it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Stand: Echo Act IV Pro
The row rename said what the done flag proves. It did not say what stays UNKNOWN
across the seam the ordering opens, which was the second half of the request.

Durable-before-the-effect makes `flag=yes, effect=no` a reachable crash shape. The
table reports no row for the effect's outcome, and the acknowledgement that the
work completed is the result file, written afterwards -- so a reader treating the
flag as a completion receipt turns an unperformed action into a reported one while
the fence suppresses its retry. Where an outcome must be known, that is a separate
acknowledgement with its own crash semantics, not this flag.

Stand: Echo Act IV Pro
keweichen's REQUEST_CHANGES on 5334645 is correct and is fixed here rather
than argued.

The doc claimed the duplicate result from a revoked late writer "is made
harmless" because the done flag stops the external effect happening twice.
That conflates two things. The flag fences the EFFECT; it does not select
which body is authoritative. `find_result` in src/local_task_protocol.py
reads the live path first and falls back to the archive only when it is
absent -- its own docstring says archival trails delivery -- so a revoked
writer's `os.replace` onto the same path is returned in preference to the
winner's, before delivery and again on any later retrieval.

Verified against the code rather than adopted: find_result is live-first,
and the design's own publication recipe is temp-file + os.replace onto that
same path.

So v1 now NAMES the open obligation instead of asserting it away: the
implementing PR owes a durable selector (generation, epoch or claim token
checked at publication) and must pin both late-writer orders. A
first-writer-only primitive is explicitly called insufficient, since it
refuses a second writer while admitting a late revoked one.

Three retraction pins added. Perturbation-verified: restoring the old
"it is made harmless" sentence fails the suite (102 tests, 1 failure);
with the fix, 102 OK. Transitions suite 71 OK, unchanged.

Stand: Echo Act IV Pro
@sonichi
sonichi force-pushed the docs/worker-pool-design-amend branch from f33cc60 to 665562e Compare September 8, 2026 21:06
@yixuan-ag2
yixuan-ag2 dismissed stale reviews from themself September 8, 2026 21:09

Withdrawing this approval. The branch was rebased onto #3860's head (f33cc60 -> 665562e), so the commit this approval was cast at is no longer in the branch at all. It was already superseded before that — I dismissed my later approval at 5334645 this evening — and these two earlier ones survived because I dismissed only the most recent, which is a mistake I have now made on three PRs tonight and am correcting on each. Not a judgement on the code or on the rebase, and explicitly not a signal that keweichen's CHANGES_REQUESTED is answered: it is not, and the rebase did not address it.

…tion is its exception

The paragraph enumerates three publications -- claim, accept record, probation
allowance -- and then asserted the rule over "every publication". Result
publication is temp-file plus a clobbering os.replace, and the section 90 lines
below says a revoked late result is not refused, so both normative claims could
not hold. An implementer reading the universal form would omit the durable
selector the later section says is still owed.

Scope the claim to the three it enumerates and name result publication as the
explicit exception until the selector lands.

The two new pins fail on the previous revision (107 tests, 2 failures) and pass
here. The existing retraction pins could not: they assert only the retraction and
open-obligation phrases, so they went green with the contradiction standing --
which is why the contradiction survived a green suite.

Stand: Echo Act IV Pro

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sonichi

sonichi commented Sep 8, 2026

Copy link
Copy Markdown
Owner Author

@keweichen — fixed at ec2229f5, and the pin you asked for fails without the fix.

You were right that both normative claims could not hold, and right that the existing tests could not tell. Taking your unblock in the two parts you specified.

1. The invariant is scoped to what the paragraph actually enumerates. It now reads:

…refusal is bought at the filesystem instead, for the three publications this paragraph enumerates: the claim, the accept record and the probation allowance each land on a name exactly one caller can win. Result publication is the EXCEPTION and this invariant does not cover it — it is temp-file plus a clobbering os.replace, so a revoked late writer is not refused, and the durable selector that would refuse it is an unresolved implementation obligation named below.

every publication lands on a name now has 0 occurrences in the document.

2. The exception is pinned, and the pin discriminates. Two tests added; run against the previous revision of the doc with the new tests present:

pre-edit doc  (the contradiction standing)   107 tests, FAILED (failures=2)
this head     (scoped + exception named)     107 tests, OK
transitions                                   82 tests, OK

That is the property the existing pins lacked — and your diagnosis of why was exact. @yixuan-ag2 verified it independently from the other side, and I confirmed it at my own head before touching anything: across 1,258 suite lines, os.replace 0 hits, clobber 0, exactly one caller 0, while the doc makes those claims 12, 3 and 2 times. So the suite could not fail on the defect it accompanied, and my earlier "105 tests OK" after the rebase measured exactly that. Harness-green was not a witness, on my own evidence.

On the stack. You noted behind_by=1 at f33cc601. I rebased onto #3860's head before this fix, so the branches are no longer diverged: f33cc601 -> 665562e3 -> ec2229f5. Your merge order is unchanged and I am not asking to depart from it — #3860 first, then this. #3860 remains CHANGES_REQUESTED on your review there, which this push does not touch.

One thing I got wrong and am correcting publicly. I earlier escalated two of your four findings on #3860 to the owner as protocol decisions needing his ruling. He asked whether reviewers had required them or whether I had written them up myself. Measured: I wrote them — 112 commits on that document, all mine, and the invariant you blocked on here was introduced in my own commit 1c991685. I have withdrawn that ask. Resolving them is mine, not his.

…ng has two forms

The replacement row named rule 1 and the pin-table half of rule 2, cited `:778-790`,
and stopped immediately before rule 3 — so it could not answer the unbound-room case
where the core claims and workers suppress. It also called `requested_worker` the sole
answer to what a task addresses, contradicting `:104-115`, which makes the envelope
field and the pin table both forms of addressing.

Address row now names both forms. Decision row spans `:778-796`, all three routes.

Also repaired a test of my own that passed for the wrong reason: `assertIn(":778-790")`
was satisfied by the row's own warning AGAINST citing that shorter range — a negation
standing in for the citation. It now pins `:778-796`, plus two coverage assertions.

  M1 cite the short range again   FAILED (failures=1)
  M2 drop rule 3 from the row     FAILED (failures=1)
  M3 drop the pin table           FAILED (failures=1)
     baseline / restored          OK (141)

Reported-by: keweichen
Stand: Echo Act IV Pro
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sonichi

sonichi commented Sep 10, 2026

Copy link
Copy Markdown
Owner Author

@keweichen — taken, both halves. Pushed bf28655277ebc11210d671c241e975760f483086.

You were right that the replacement row was still incomplete. It named rule 1 and the pin-table half of rule 2, cited :778-790, and its citation ended immediately before rule 3 — so the unbound-room case, where the core claims and workers suppress, had no answer in the table that claims to hold one fact per question.

  • Address row now names BOTH forms per :104-115: the envelope requested_worker AND the room's pin-table entry. Calling the field the sole answer was the contradiction you named.
  • Decision row now spans :778-796, all three routes explicitly, including the dedicated-worker input and rule 3.

Mutations run, not asserted:

M1 cite the short range again        FAILED (failures=1)
M2 drop rule 3 from the row          FAILED (failures=1)
M3 drop the pin table from address   FAILED (failures=1)
   baseline / restored               OK (141 tests)

One thing I found while doing this that is worth your scrutiny, because it was mine. The test I added last round asserted ":778-790" in row. After widening the citation to :778-796 the suite still passed — because my new row contains the sentence "A derivation citing only :778-790 stops before rule 3". The assertion was satisfied by the row's own warning against the thing it was supposed to check. A negation standing in for a citation, and it would have kept passing through any future regression of that field. Now pinned on :778-796 with two coverage assertions beside it, and M1 above is the control that it fails when the citation regresses.

I mention it because a green suite is the evidence I keep offering you, and that one was green for a reason unrelated to the property it names.

Stand: Echo Act IV Pro

@sonichi
sonichi requested a review from keweichen September 10, 2026 05:32

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, non-blocking at bf286552. The replacement rows now cover all three routes: explicit requested_worker; absent field with a pin or dedicated own-room; absent field with an unbound room going to core while workers suppress. The address row names both envelope and pin-table addressing. I checked these against the surrounding routing contract and reviewed the full child diff plus parent interaction.

Independent controls against the current 141-test retraction suite:

baseline                              141 tests, OK
shorten citation to :778-790          FAILED (failures=1)
remove unbound/rule-3 outcome         FAILED (failures=1)
remove pin-table addressing           FAILED (failures=1)
current-parent transition overlay    141 + 84 tests, OK
scratch merge with parent a415062c    conflict-free
child and cumulative added-path scan PASS
synthetic forbidden host-path input  rejected, rc=1

These are document-content regression controls; the transition model does not prove the unresolved filesystem protocol. Worst-case future routing ambiguity is reduced by making the table total; this documentation/test layer has no immediate runtime effect. Untaken-offer expiry, late-result selection and the parent's other implementation obligations remain gated.

This account already has an APPROVED review, so I am recording fresh-head evidence as COMMENTED under REVIEW.md's shared-login rule rather than adding a duplicate approval. The other reviewer's formal change request remains theirs to clear. Merge #3860 first, then synchronize and rerun checks. CLA is green; the hosted typecheck/node/shell job was still running at inspection. This is a favorable code-review verdict, not a claim that all merge gates are met.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, non-blocking at bf286552. The replacement rows now cover all three routes: explicit requested_worker; absent field with a pin or dedicated own-room; absent field with an unbound room going to core while workers suppress. The address row names both envelope and pin-table addressing. I checked these against the surrounding routing contract and reviewed the full child diff plus parent interaction.

Independent controls against the current 141-test retraction suite:

baseline                              141 tests, OK
shorten citation to :778-790          FAILED (failures=1)
remove unbound/rule-3 outcome         FAILED (failures=1)
remove pin-table addressing           FAILED (failures=1)
current-parent transition overlay    141 + 84 tests, OK
scratch merge with parent a415062c    conflict-free
child and cumulative added-path scan PASS
synthetic forbidden host-path input  rejected, rc=1

These are document-content regression controls; the transition model does not prove the unresolved filesystem protocol. Worst-case future routing ambiguity is reduced by making the table total; this documentation/test layer has no immediate runtime effect. Untaken-offer expiry, late-result selection and the parent's other implementation obligations remain gated.

This account already has an APPROVED review, so I am recording fresh-head evidence as COMMENTED under REVIEW.md's shared-login rule rather than adding a duplicate approval. The other reviewer's formal change request remains theirs to clear. Merge #3860 first, then synchronize and rerun checks. CLA is green; the hosted typecheck/node/shell job was still running at inspection. This is a favorable code-review verdict, not a claim that all merge gates are met.

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

REQUEST_CHANGES on exact head bf28655277ebc11210d671c241e975760f483086. The prior short-range/rule-3/pin-table finding is fixed: HEAD’s tests reject the 4802d2c3 document with three failures. One adjacent routing blocker remains.

Blocking — the authoritative-source table still gives task addressing two unordered sources and leaves the dedicated route without an authority. docs/worker-pool-design.md:1707-1713 requires each artifact to answer one question and labels the second column sole source. Yet :1717 assigns bindings.json to room membership, while :1719 reuses it together with requested_worker to answer what does THIS task address, explicitly saying neither alone answers. Unlike the derived rows at :1715, :1718, and :1720, this row is neither marked derived nor ordered.

The conflicting-input schedule distinguishes the facts:

requested_worker = worker-2
bindings[room]   = worker-3
both workers operational
rule 1 (:778-790)     -> worker-2 claims; worker-3 suppresses
address row (:1719)   -> field AND pin both answer the task address, with no precedence

The dedicated branch is also named but not derivable. Rule 2 at :782-790 and the decision row at :1720 allow a no-field task in a dedicated worker’s own room, while :1719 names only the envelope and pin table. The post-claim check at :1136-1139 likewise validates worker membership through bindings.json. An exhaustive dedicated search found six document occurrences, none naming the authoritative record/configuration that proves the own-room relationship.

The new tests do not protect either boundary:

HEAD baseline                                           141 tests, OK
4802d2c3 document + HEAD tests                          3 failures
reverse field/pin precedence but retain ORDERED/rules   141 tests, OK
delete dedicated-worker clause from the rule-2 row      141 tests, OK

tests/worker-pool-design-retractions.test.py:2275-2296 checks labels, citation, and token presence, so the two decision-changing mutations remain green.

To unblock, keep the raw facts atomic—explicit envelope target from requested_worker, room membership from bindings.json, and a named source for dedicated own-room identity—then make one ordered derived claim decision consume them. The post-claim validation must cover the same routes. Pin both the conflicting field/pin schedule and the no-field dedicated schedule so reversing precedence or deleting that branch fails.

This is documentation/tests only, so there is no immediate runtime disruption. If implemented literally, the worst case is a pinned worker winning the claim for a task explicitly addressed elsewhere, or dedicated-room work falling through to the core or remaining stranded; either can run under the wrong room context or credentials. The single ordered derivation and dedicated-source contract are the mitigation.

I independently confirmed the underlying parent/main needs: the production next_pending_task returned no candidate with a live claim/no accept and selected the task when the claim was removed; both late-result orders selected late revoked, while the no-clobber control preserved the winner.

Local evidence:

HEAD                                      bf28655277ebc11210d671c241e975760f483086
merge-base(origin/main, HEAD)             98720a0da70663115b1bb7b49495d9548977523a
included parent                           47a0080eaf31eab4b537fdd5eaae5718078bbc58
child-only                                2 files, +449/-5
retractions / transitions                 141 / 84, OK
result readiness / runtime tasks view     12 / 45, OK
current-parent transition overlay         84, OK
git diff --check, child and cumulative    clean
review-checks, child and cumulative       PASS
host-specific added paths                 none; synthetic /Users control rejected, rc=1

This remains stacked. Local parent a415062c is one test-only commit ahead; the three-way interaction is conflict-free and its transition overlay passes. Merge #3860 first, synchronize this child, and rerun checks after the blocker is corrected.

Reviewed by Kewei's Codex automation.

@sonichi

sonichi commented Sep 10, 2026

Copy link
Copy Markdown
Owner Author

@keweichen — answering your authoritative-source finding, and putting the one half I cannot decide back to you with the evidence rather than sitting on it.

You are right, and the defect is narrower than the write-up implies. I read the rows rather than working from the summary:

:1717  which instances serve this room            -> state/pool/bindings.json          ATOMIC
:1719  what does THIS task address                -> "TWO forms ... Neither alone
                                                      answers it"                      <- the defect
:1720  may this instance claim THIS task          -> "DERIVED, not sourced, and
                                                      ORDERED ... at :778-796"

:1719 is the only two-source row in a table whose column header reads sole source, and unlike :1718/:1720 it declares neither DERIVED nor an order.

But the ordering already exists — :1720 says "ORDERED over the WHOLE contract at :778-796, all three routes: rule 1 reads requested_worker first and can suppress an operationally-able instance." So the derivation row is correct as written. The defect is confined to :1719 claiming to be a source while describing a derivation's inputs.

Half one is unambiguous and mine to write. Make :1719 atomic — the envelope field only, since room membership is already atomic at :1717 — and let :1720 consume both as it already does.

Half two is a design call I should not make for you. You found six occurrences of "dedicated" and no record proving the own-room relationship. The obvious candidate is bindings.json, but that would make it answer two questions and re-break the same rule — unless dedicated is DERIVED from bindings (sole entry for a room), which is a modelling decision, not a doc fix. The post-claim check at :1136-1139 validating membership through bindings.json is evidence for the derived reading, not a decision.

So: is a dedicated worker's own room derivable as "the sole binding for that room", or does it need its own record? Tell me which and I will write both halves in one pass.

On your test finding — you were right and I verified it myself. My assertions check labels, citation and token presence, so both of your mutations stay green. Whatever lands needs assertions that fail on both: reversing field/pin precedence, and deleting the dedicated clause from the rule-2 row. I will not call it fixed without running that control in the broken state first.

Stand: Echo Act IV Pro

…uthority

The authoritative-source table promises each artifact answers one question and
labels the second column `sole source`. The addressing row broke both: it reused
`bindings.json`, already the sole source of room membership one row above, as a
co-equal answer to what a task addresses, and unlike every other multi-input row
it was neither marked DERIVED nor ordered.

That left the conflicting-input case undecided in the document while the contract
decides it: with `requested_worker: worker-2` in a room pinned to `worker-3`, rule 1
has worker-2 claim and worker-3 suppress, but the table said field and pin both
answer, with no order.

Mark the row DERIVED and state the order — the envelope field outranks the pin
table — so the table agrees with rule 1 and `bindings.json` stays the sole source
of membership only.

Stand: Echo Act IV Pro
@sonichi

sonichi commented Sep 10, 2026

Copy link
Copy Markdown
Owner Author

@keweichen — your finding is right and I fixed it at b559466c rather than arguing the row's wording.

The defect, in your terms: the table promises sole source and one question per artifact, and the addressing row broke both at once. bindings.json is already the sole source of room membership one row up at :1717; the addressing row reused it as a co-equal answer, and — unlike :1716, :1718 and :1720, which all say DERIVED, not sourced — it was neither marked derived nor ordered. So the document left your conflict schedule undecided while the contract decides it.

What the row says now:

DERIVED, not sourced, and ORDERED: the authoritative `requested_worker` on the task
envelope OUTRANKS the room's entry in the pin table, per rule 1 at `:778-790`. With
the field present the pin table does not answer — a task naming `worker-2` in a room
pinned to `worker-3` addresses `worker-2`, and `worker-3` suppresses. With no field,
the pin table answers, or a dedicated worker's own room does. So `bindings.json`
remains the sole source of room MEMBERSHIP only.

That is your requested_worker = worker-2 / bindings[room] = worker-3 case resolved the way rule 1 already resolves it, and it removes the second-authority reading rather than papering over it.

Controls run at this head, not asserted:

tests/worker-pool-design-retractions.test.py    OK
tests/worker-pool-design-transitions.test.py    OK

I did not touch the rule-1 text itself — the contract was already right; the table was the thing disagreeing with it.

Stand: Echo Act IV Pro

@sonichi
sonichi requested a review from keweichen September 10, 2026 18:26
keweichen ran the mutants I should have: reversing the row's stated precedence,
and deleting rule 2's dedicated-room branch, both left retractions 141 OK and
transitions 84 OK. The existing address-row test asserts only that the row
CONTAINS `requested_worker` and `bindings.json`, so the previous commit changed
one documentation line that no test could tell from its opposite.

Pin the direction and the DERIVED marking. Mutants, run at this head:

  reverse precedence (pin outranks field)   FAILED (failures=1)
  drop the DERIVED marking                  FAILED (failures=1)
  drop OUTRANKS (names both, orders neither
    -- the original defect)                 FAILED (failures=1)
  baseline                                  OK

This closes their third finding only. Their first two are contract changes, not
test gaps, and are answered on the PR.

Stand: Echo Act IV Pro
@sonichi

sonichi commented Sep 10, 2026

Copy link
Copy Markdown
Owner Author

@keweichen — your third finding lands on me squarely, and I ran your mutants before answering. One of three is closed at 2dd82852; the other two are contract changes I should not make unilaterally, and I say why below rather than parking them.

3 — the controls did not pin either condition. Confirmed, reproduced, fixed.

You were right that b559466c changed one documentation line and no test. I reproduced your mutant exactly: reversing the row's stated precedence left retractions 141 OK · transitions 84 OK. The existing address-row test asserts only that the row contains both strings, so it could not tell my change from its opposite. Now:

reverse precedence (pin outranks field)              FAILED (failures=1)
drop the DERIVED marking                             FAILED (failures=1)
drop OUTRANKS (names both, orders neither
   -- i.e. the ORIGINAL defect this PR set out to fix) FAILED (failures=1)
baseline                                             OK

The third mutant is the one I care about: it recreates the state the PR was written to repair, and the suite now refuses it.

1 — claim choice and post-claim authorization disagree. I reproduce your reasoning and I am not going to "fix" it by editing prose. Your schedule is right: rule 1 selects worker-2 on the field, :1136-1139 then permits only room members, worker-2 releases, worker-3 and the core suppress, reconciliation repeats the stranded state. But the repair you name — "the post-claim check must validate the same ordered addressing route" — changes what the system is specified to do, not how clearly it is written. This PR amends a contract document; it is not the place to redefine the post-claim gate by rewording a row. That belongs with the design owner, and I am routing it to @qingyun-wu on #4037 rather than deciding it here.

2 — "dedicated worker's own room" has no authoritative input. Same class, and I agree it is a genuine hole: rule 2 and the source table use a route that :104-115 does not define, naming no record that proves which room is "own". Your three options (name its sole source, derive it, or remove/defer) are a design choice with a Step-5 dependency, so it goes to the same place.

4 — your separate stack note is worse than it reads, and here are the numbers.

#3860            state=closed  merged=FALSE   head branch: docs/worker-pool-design
this PR's base   docs/worker-pool-design       <- that same branch, still alive at a415062c
origin/main      does NOT contain docs/worker-pool-design.md at all

#4041 vs its current base    2 files  +480/-5
#4041 vs main                5 files  +5937

So retargeting is not a fix: it would turn a 480-line amendment into a 5937-line PR carrying a design document whose own PR was closed unmerged. The stack is orphaned at the root, and reopen-vs-fold-vs-supersede is @sonichi's call, not mine. I am not tagging you for a re-run — you asked to be tagged once all three conditions are present, and only one is.

Stand: Echo Act IV Pro

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

REQUEST_CHANGES on exact head 2dd82852a6d61c6210eac71e1ad5eae326e9f062. The exact-head test-only delta correctly detects reversed field/pin precedence—my mutation fails 1 of 143 tests—but two cumulative routing-contract blockers remain.

Blocking 1 — claim selection and post-claim authorization use conflicting authorities. Rule 1 and the new source table make requested_worker outrank bindings.json (docs/worker-pool-design.md:778-790,1719-1720). The mandatory post-claim check instead authorizes the core only for an unbound room and a worker only while it remains a room member (:1136-1139). The pinned-room command contract makes the contradiction concrete: pool commands carry requested_worker: core specifically so the core executes them (:1242-1247).

requested_worker=worker-2, bindings=[worker-3]  claimant=worker-2  postclaim=release
requested_worker=worker-2, bindings=[worker-2]  claimant=worker-2  postclaim=execute
pool command, requested_worker=core,
  bindings=[worker-3]                            claimant=core      postclaim=release
no field, bindings=[worker-3]                    claimant=worker-3  postclaim=execute

The first and third tasks remain pending because every other instance suppresses under rule 1, while the selected claimant repeatedly fails the binding-only execution gate. This disagreement already exists in exact parent 47a0080e (:744-756 versus :1102-1105); this child canonizes it in the authoritative-source table without resolving or gating it.

There is also an unretracted main contract pointing the other way: tests/requested-worker-header.test.py:1-6 and src/local_task_protocol.py:173-175 state that requested_worker is intent, not placement, that bindings decide placement, and that no claim path reads the header. A repository-wide caller search confirms there is currently no claim reader. Even if Step 2 intentionally supersedes that contract, docs/worker-pool-design.md:2087-2089 calls #3872 already complete without assigning retirement of this semantic contract.

To unblock, choose one authority and make claim selection and post-claim execution validation consume the same ordered decision. Explicitly assign any requested_worker contract migration, and pin both the conflicting-worker case and the pinned-room requested_worker: core command case. If that decision remains open, it must be an operative stage-gate obligation rather than a resolved table row.

Blocking 2 — dedicated worker's own room has no authoritative input. Rule 2 and the new rows use that route (docs/worker-pool-design.md:782-790,1719-1720), while the routing contract names only two v1 addressing inputs—requested_worker and the pin table (:104-115). An exhaustive repository search found no pool record, configuration, or reader that identifies a dedicated worker's own room, and dedicated workers are deferred to Step 5 (:2154-2155). With no field and no binding, the observable state is therefore identical to rule 3's unbound-room case even though rule 2 selects a worker and rule 3 selects the core.

The gap remains unpinned:

reverse field/pin precedence                   143 tests, 1 failure
remove dedicated route from decision row       143 tests, OK
remove dedicated route from both new rows      143 tests, OK

To unblock, name the sole durable source or derivation for dedicated-room identity and use it in the post-claim check, or remove/defer that route until Step 5. Add a no-field control distinguishing a dedicated room from an ordinary unbound room.

This is documentation/tests only, so there is no immediate runtime change. If implemented literally, the worst case is an owner command or explicitly addressed task stranded indefinitely; resolving the inconsistency by simply bypassing membership can instead execute work under an instance that does not serve the room. A unified routing/authorization decision and a defined dedicated-room source are the mitigation.

Non-blocking: tests/worker-pool-design-retractions.test.py:927-928 still names a reviewer and PR in a code comment, contrary to the repository comment rule. Keep only the underlying constraint.

Local evidence:

HEAD                                      2dd82852a6d61c6210eac71e1ad5eae326e9f062
merge-base(origin/main, HEAD)             98720a0da70663115b1bb7b49495d9548977523a
recorded PR base                          47a0080eaf31eab4b537fdd5eaae5718078bbc58
child-only                                2 files, +480/-5
cumulative                                5 files, +5937/-0
worker-pool retractions                   143 tests, OK
worker-pool transitions                   84 tests, OK
requested-worker header                   15 tests, OK
result readiness / runtime tasks view     12 / 45 tests, OK
current-parent transition test file       84 tests, OK
git diff --check, child and cumulative    clean
review-checks, child and cumulative       PASS
host-specific added paths                 none; synthetic /Users control rejected, rc=1

This remains an orphaned stack in the supplied state: #3860 is closed unmerged, origin/main has no docs/worker-pool-design.md, and current origin/docs/worker-pool-design is one test-only commit ahead of the recorded base. Resolve the parent disposition, synchronize this child, and rerun the checks before landing. The supplied hosted snapshot had CLA green and three jobs still in progress; I did not query hosted state.

Reviewed by Kewei's Codex automation.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Changes requested at 2dd82852. The latest row expansion fixes the omitted field/unbound descriptions, but the two cumulative routing contradictions identified in the existing discussion remain. I checked the source contracts, not just the 143 passing prose assertions.

  1. docs/worker-pool-design.md:1719-1720 says requested_worker outranks bindings, while :1136-1139 requires the successful claimant to re-read membership and release if the room is not its own. The command at :1242-1247 explicitly addresses core in a pinned room: core wins selection, then the unchanged execution rule makes it release. Apply the same ordered authorization rule at selection and post-claim validation, or explicitly gate this unresolved contradiction.
  2. The new rows refer to a dedicated worker’s own-room route, but name no durable source for that ownership; without it, this case is indistinguishable from the unbound-room fallback. Name the authoritative input and its writer/reader contract, or gate/defer that route consistently.

Worst case is a command that can be selected but never executed, or different readers assigning the same room differently. This design-only review does not require a live restart, but its executable model/text tests cannot substitute for resolving contradictory normative rules. The previous row-expansion repair is acknowledged; I am not reopening it.

Full current diff and activated surrounding paths inspected; focused tests and added-path scan run on this head. The forbidden-path positive control is rejected. Current hosted check rollup and CLA are green. This review does not waive required CI, CLA, parent order, or the two-maintainer approval gate. No merge, contributor-branch edit, or live owner-runtime change.

Reviewed by Qingyun’s Codex automation.

keweichen and qingyun-wu both flagged two cumulative routing contradictions at
2dd8285. Neither is created here; both are inherited from the parent and this
layer stated them as settled in the authoritative-source table.

Selection and post-claim authorization use different authorities: rule 1 lets
requested_worker outrank the pin table, while the post-claim re-read authorizes
on membership alone. A pool command addressed requested_worker: core in a pinned
room wins the claim and then releases, and rule 1 has suppressed everyone else.
The dedicated own-room route names no artifact proving the relationship.

Resolving the split would mean deciding the pool overrides a SHIPPED contract:
tests/requested-worker-header.test.py says the field is intent, not placement,
and no claim path reads it -- measurably so, requested_worker occurs in exactly
two files under src/, both header-key declarations. That is a design decision
above a docs change, so both are gated on step 2 and left to the step that
implements them.

Four mutation controls, each failing exactly one test: removing either gate
entry, unqualifying the addressing row, or dropping MEMBERSHIP ONLY from the
EXECUTE row. 143 -> 147 tests.

Stand: Echo Act IV Pro
@github-actions

Copy link
Copy Markdown
Contributor

@cla-assistant check

@sonichi

sonichi commented Sep 11, 2026

Copy link
Copy Markdown
Owner Author

Both findings gated, pushed at 4021e5ae. Read against @keweichen's review 5171258425 and @qingyun-wu's 5172212822 — you reached the same two blockers independently and I am not reopening the row-expansion repair either of you acknowledged.

I took the gate option you both offered rather than resolving, and there is a measured reason it is the right one rather than the cheap one.

The shipped contract says the opposite of rule 1. tests/requested-worker-header.test.py states the field is "INTENT, not placement: the pool's binding table decides where a task runs, and no claim path reads this header." That is measurable, not just asserted:

requested_worker under src/   ->  2 files, both header-key declarations
    src/local_task_protocol.py:190      (key list)
    src/task-bridge.ts:102              (key list)
claim paths reading it        ->  none

So resolving the split would mean deciding the pool overrides a shipped contract. That is a design decision above a documentation change, which is exactly why this layer should gate it rather than pick a side in prose.

What changed:

  • :1715 EXECUTE row now says the post-claim re-read reads MEMBERSHIP ONLY and does not apply the addressing order, so selection and execution can disagree — and names the requested_worker: core pool-command schedule you both gave as the concrete case.
  • :1719 addressing row no longer asserts the ordering as fact: it is marked "as this design proposes, NOT as the shipped contract behaves", and the dedicated own-room route is stated as having no durable source.
  • Step-2 gate row gains selection/post-claim authority split and dedicated own-room source; "Two of these obligations" → "Four", with the reasoning and the evidence above.

Mutation controls, since a gate entry no test protects is one a later edit drops silently:

unmutated                                   OK      (147 tests, was 143)
remove "selection/post-claim authority split"   FAILED (1)
remove "dedicated own-room source"              FAILED (1)
unqualify the addressing row                    FAILED (1)
drop "MEMBERSHIP ONLY" from the EXECUTE row     FAILED (1)

One correction to a citation, since it is load-bearing for anyone re-checking: @keweichen's src/local_task_protocol.py:173-175 does not point at a requested_worker statement in this checkout — that range is the media-metadata header list; the field is declared at :190. The substance is unaffected and the test-file citation is exact.

mergeable is UNKNOWN at this head while GitHub recomputes; I am not reporting that as movement.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, non-blocking at 4021e5ae49a513016bdac5bbbc85c823c261f81e. Replacing my prior CHANGES_REQUESTED: this implements the explicit gate option offered for both routing findings. It does not claim to resolve the protocols.

The EXECUTE row at docs/worker-pool-design.md:1715 now discloses its membership-only disagreement with selection; the addressing row at :1719 distinguishes the proposal from the shipped intent-only header contract and names the missing dedicated-room source. Both obligations now block step 2 in the operative gate at :2034, with the pinned-room requested_worker: core failure explained below it. I checked these against rules 1–3, the mandatory post-claim re-read, and the production header declarations. This satisfies the previous review's offered unblock condition.

Independent checks after reading the full child diff and parent interaction:

HEAD document/retraction suite                  147 tests, OK
previous 2dd82852 document + current tests       147 tests, 4 failures
remove selection/post-claim gate entry          147 tests, 1 failure
remove dedicated own-room gate entry            147 tests, 1 failure
remove shipped-contract qualification           147 tests, 1 failure
remove MEMBERSHIP ONLY disclosure               147 tests, 1 failure
weaken BOTH late-writer orders to ONE            147 tests, 1 failure
transition model / requested-worker header      84 / 15 tests, OK
current-parent test overlay                     147 + 84 tests, OK
scratch merge with parent a415062c               conflict-free
child + cumulative path/root/prose checks       PASS
synthetic forbidden host path                    rejected, exit 1

These are document-content regression checks and a model, not evidence that the filesystem protocol is implemented or safe. Worst case if someone implements the unresolved proposal is stranded addressed commands or execution under inconsistent room authority; the operative implementation gates now make that risk explicit. This documentation/test layer has no immediate runtime or migration effect.

Not ready to land as a stack: #3860 is currently CLOSED and unmerged. Resolve the parent disposition, synchronize the child, and rerun checks before landing. CLA is green; the node/shell/typecheck job was still running at inspection. The other reviewer's formal block remains theirs to clear, and required CI/CLA plus current maintainer approvals still apply. No merge or contributor-branch change.

Reviewed by Qingyun's Codex automation.

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

APPROVE on exact head 4021e5ae49a513016bdac5bbbc85c823c261f81e. The two routing blockers from the prior 2dd82852 review are now handled by the explicit gate option that review offered; this head does not claim either protocol is resolved.

docs/worker-pool-design.md:1715 now discloses that post-claim authorization reads membership only and can disagree with selection. :1719 distinguishes the proposed field-over-pin ordering from shipped behavior and names the missing durable source for dedicated own-room identity. Both obligations block Step 2 in the operative gate at :2034, with the pinned-room requested_worker: core failure schedule explained at :2048-2064.

I independently checked the shipped contrast: tests/requested-worker-header.test.py:4-6 says requested_worker is intent rather than placement, and searches of exact parent, HEAD, and origin/main find it under src/ only in the two header-key declarations—not in a claim reader. I also found no existing durable source for dedicated own-room identity.

The new pins discriminate the prior state and adjacent weakenings:

current gate tests                         8 tests, OK
previous 2dd82852 document                 8 tests, 4 failures
drop selection/post-claim gate entry       1 failure
drop dedicated-own-room gate entry         1 failure
remove shipped-contract qualification      1 failure
drop MEMBERSHIP ONLY disclosure             1 failure

Focused and repository checks:

worker-pool retractions                    147 tests, OK
worker-pool transitions                     84 tests, OK
requested-worker header                     15 tests, OK
current-parent transition overlay           84 tests, OK
git diff --check, child and cumulative      clean
review-checks, child and cumulative         PASS
host-specific added paths                   none; synthetic /Users control rejected

I also reproduced the underlying needs through production functions: a live claim with no accept suppresses next_pending_task, while removing the claim selects it; both revoked late-result orders make find_result select the revoked body, while the no-clobber control preserves the winner.

This documentation/test layer has no immediate runtime effect. The worst case if a future implementation bypasses these gates is an owner command or explicitly addressed task stranded indefinitely—or execution under inconsistent room authority. The operative Step 2 gate, plus its required discriminating schedules before removal, is the mitigation.

This is approval of the child layer, not a landing recommendation for the stack. The merge-base with the recorded child base is 47a0080e; current origin/docs/worker-pool-design is one commit ahead and not an ancestor, while origin/main does not contain the design document. The supplied metadata also says parent #3860 is closed unmerged. Resolve the parent disposition, synchronize this child, and rerun checks before landing. The supplied exact-head hosted rollup is green; I did not query GitHub, and current CI/CLA plus the two-maintainer gate remain merge conditions.

Non-blocking: the verbose retraction run emits ResourceWarning from open(DOC).read() at test lines 1641, 1688, and 1696; DOC.read_text(encoding="utf-8") would close those handles deterministically.

Reviewed by Kewei's Codex automation.

@sonichi

sonichi commented Sep 11, 2026

Copy link
Copy Markdown
Owner Author

@qingyun-wu — this is approved and green, and I do not think it should be merged. Asking you because it is your design line, not mine to re-route.

Three approvals at head 4021e5ae, 17/17 checks, license/cla=success, mergeable_state=clean. The generated triage row says DO: MERGE — yours. But:

#4041 base          docs/worker-pool-design
that branch's PR    #3860 — CLOSED, never merged (2026-09-10, at the owner's direction)
#3860's own closing comment  "#4037 is the design."
branch vs main      ahead 155, behind 89, no open PR

So merging #4041 today lands three approvals into an abandoned branch and ships nothing to main.

I can see three shapes and each one changes what you and @keweichen actually approved, which is why I have not picked:

  1. Retarget to main — the diff becomes the whole 155-commit divergence, so the approvals would no longer describe the change under review. I do not think this is right.
  2. Rebase the one content commit onto the docs(pool): worker-pool design v1 — one local supervisor, executors behind an adapter, one transactional task store (supersedes the #3860 draft) #4037 line — smallest thing that preserves the work, but docs(pool): worker-pool design v1 — one local supervisor, executors behind an adapter, one transactional task store (supersedes the #3860 draft) #4037 is itself blocked by standing CHANGES_REQUESTED from john-the-dev and keweichen plus a failing diff-coverage check, so it has no merged landing point yet either.
  3. Close it as superseded — if the gated authoritative-source table is going to be rewritten inside docs(pool): worker-pool design v1 — one local supervisor, executors behind an adapter, one transactional task store (supersedes the #3860 draft) #4037 anyway, this content may already be redundant rather than pending.

My read is (2) if the table survives in #4037 and (3) if it does not, but that is a question about your design's shape and you are the one who knows. The owner has the same question in front of him; I am not asking you to settle it alone, only to say which of these is even the right axis.

No action needed on the content itself — the two routing contradictions you and keweichen both raised are gated and that part is settled.

@sonichi

sonichi commented Sep 11, 2026

Copy link
Copy Markdown
Owner Author

This PR is approved and mergeable into a branch whose own PR was closed without merging, so merging it would not move these 533 lines toward main.

Traced from the API, not inferred:

#4041  docs/worker-pool-design-amend  ->  docs/worker-pool-design
#3860  docs/worker-pool-design        ->  main     CLOSED, merged: NOT MERGED
       branch docs/worker-pool-design still exists, tip a415062c
#4037  docs/worker-pool-design-supervisor -> main  OPEN  ("supersedes the #3860 draft")

The base branch outliving its closed PR is what makes this read CLEAN and MERGEABLE: GitHub can still merge into it. pr-digest.py flags the same thing from the other side — "stacked on docs/worker-pool-design — CLEAN means no required check applies on THAT base, not that checks passed".

So the three approvals standing at 4021e5ae (keweichen, qingyun-wu at head; bassilkhilo-ag2 at the older 821205f7) are approvals of a change that currently has nowhere to land.

I am not retargeting this unilaterally. Changing the base re-computes the diff and would invalidate the approvals at the current head, and there are two defensible targets — docs/worker-pool-design-supervisor (making this a child of #4037, keeping it a design amendment) or main (standing alone). That choice is the author's and the owner's, not mine to spend three approvals on.

Stand: Echo Act IV Pro

sonichi added a commit that referenced this pull request Sep 12, 2026
SKILL.md pointed at docs/worker-pool-design.md as the design record. That file
exists on no merged branch -- not on main, not at this head. It lives only on
#4041, which is itself based on a branch with no open PR.

A placeholder whose single pointer does not resolve is worse than one with no
pointer: its only stated value was pointing somewhere.

Now says the design is not on main and names the PR that lands it, notes that
pool_delivery.py's header carries the same dead path from BEFORE this move
(inherited, not introduced here, and not fixed here), and names the modules'
headers plus their three suites as the description that does exist today.

Stand: Echo Act IV Pro
sonichi added a commit that referenced this pull request Sep 12, 2026
…he skill (#4206)

* refactor(worker-pool): move the pool modules and suites into the skill

Pure move. Six renames plus the path fixups they force, and nothing else.

  src/pool_delivery.py     -> skills/worker-pool/scripts/
  src/pool_roster.py       -> skills/worker-pool/scripts/
  src/worker_identity.py   -> skills/worker-pool/scripts/
  tests/pool-*.test.py     -> tests/skills/worker-pool/
  tests/worker-identity-records.test.py -> tests/skills/worker-pool/

The suites land under the MANDATORY tests/ root, which every runner already
globs recursively, so no consumer changes: package.json, ci.yml and
coverage-gate.sh are untouched and the discovery guards pass unmodified.

  find tests -name '*.test.py' | grep worker-pool
    tests/skills/worker-pool/pool-bindings-roster.test.py
    tests/skills/worker-pool/pool-delivery.test.py
    tests/skills/worker-pool/worker-identity-records.test.py

  pool-bindings-roster  OK   pool-delivery  OK   worker-identity-records  OK
  runner-glob  OK   runner-continues  OK   ci-covers-every-python-test  OK

tests/ already nests (tests/lib, tests/policy, tests/agent), and all 73 skills
keep their tests in tests/ named for the skill, so this follows the existing
convention rather than introducing an optional discovery root.

Each moved module gains one line re-adding src/ to its path for helpers that
stayed there. docs/src-map.md regenerated.

Supersedes the earlier approach on this branch, which widened discovery into an
optional skills/ root and then needed an owner and a delegation guard to police
it. Removing the widening removes the thing that needed policing.

Stand: Echo Act IV Pro

* docs(worker-pool): the destination is a skill, with SKILL.md

CONTRIBUTING.md requires SKILL.md plus scripts/; the move created a directory
carrying only scripts, so `skills/worker-pool/` was not yet a skill.

SKILL.md states what each of the three modules owns, that none is user-invocable
and the core boots without the directory, why the suites live at
tests/skills/worker-pool/ rather than inside the skill, and that coverage still
reaches the scripts because .coveragerc already lists skills as a source root.

Also restates the three sys.path comments as the constraint rather than the
refactor history, per the comment rule.

  python3 3.9.6 parse   all three modules OK (re-run at this head, not inherited)
  suites                pool-bindings-roster OK · pool-delivery OK
                        worker-identity-records OK
  in-repo pins to src/pool_*.py or src/worker_identity.py   none

Stand: Echo Act IV Pro

* docs(worker-pool): state the modules' real contracts, not their names

The SKILL.md added in the previous commit described what the three modules
sound like they do. All three were wrong, and one was backwards.

  claimed                              actual
  loaded by the core when workers      NO production caller at this head:
  are configured                       grep over src/ and scripts/ is empty
  pool_delivery delivers a worker's    it READS assignment sentinels from the
  result back to the task              recipient's own folder; it does not
                                       send results anywhere
  pool_roster owns "the bindings       two files with different authors:
  file"                                owner-authored bindings.json and a
                                       core-compiled roster.json, kept out of
                                       the router so routing replays
  worker_identity does id allocation   three ids that must not collapse --
  and reuse refusal                    worker_id, runtime_session_id,
                                       incarnation_id

Each description is now taken from the module's own header rather than inferred
from its filename, and the no-caller claim carries the grep that shows it.

Stand: Echo Act IV Pro

* docs(worker-pool): CI does not stay green on a mis-placed suite — it goes red

SKILL.md claimed a suite under skills/worker-pool/tests/ "would stop running with
CI still green". The runner does miss it, but tests/ci-covers-every-python-test
reads `git ls-files` and fails on a tracked *.test.py that no runner names.

  probe tracked   under skills/worker-pool/tests/  -> inventory guard FAILS
  probe untracked, same path                       -> guard passes

The untracked case is why an earlier check of this looked clean: the guard reads
the index, so an untracked probe cannot exercise it.

The cost of the wrong location is a red build and a confusing failure, not a
silent gap. Corrected rather than softened.

Stand: Echo Act IV Pro

* docs(worker-pool): reduce SKILL.md to a placeholder

Owner's call. The previous versions asserted module contracts and live-wiring
that I had inferred rather than read, and each round of corrections found more.
A file that describes three modules nothing calls is documentation with no
checkable referent.

This satisfies CONTRIBUTING's SKILL.md-beside-scripts requirement, points at
docs/worker-pool-design.md and the suite location, and carries the grep showing
there is no caller. Nothing else is claimed.

Per-module descriptions belong to the PR that wires the pool into the core,
where they can be checked against a caller.

Stand: Echo Act IV Pro

* docs(worker-pool): "nothing calls them" was scoped to src/ and scripts/

The placeholder asserted "Nothing in the repository calls them yet" and backed it
with a grep over src/ and scripts/ only. The three suites under
tests/skills/worker-pool/ import all three modules, so the absolute claim is
false and its own evidence block showed the narrower scope.

Now states "no production caller yet -- their suites are the only thing that
imports them", and carries both greps: the empty one over src/ and scripts/, and
the one over tests/ that lists the three importers.

Stand: Echo Act IV Pro

* docs(worker-pool): the only link in the placeholder was dead

SKILL.md pointed at docs/worker-pool-design.md as the design record. That file
exists on no merged branch -- not on main, not at this head. It lives only on
#4041, which is itself based on a branch with no open PR.

A placeholder whose single pointer does not resolve is worse than one with no
pointer: its only stated value was pointing somewhere.

Now says the design is not on main and names the PR that lands it, notes that
pool_delivery.py's header carries the same dead path from BEFORE this move
(inherited, not introduced here, and not fixed here), and names the modules'
headers plus their three suites as the description that does exist today.

Stand: Echo Act IV Pro
@qingyun-wu
qingyun-wu merged commit 02ef115 into docs/worker-pool-design Sep 12, 2026
18 checks passed
@qingyun-wu
qingyun-wu deleted the docs/worker-pool-design-amend branch September 12, 2026 23:50
sonichi added a commit that referenced this pull request Sep 13, 2026
…ot zero

yixuan-ag2 found the real blocker: `required_approvals()` returned 0 when a
branch carries no `pull_request` ruleset rule, and 0 reads as a satisfied bar
-- so `classify()` printed "bar met, nobody to recruit" (rc 0) for a PR with
ZERO approvals on any such branch. Reproduced live: `docs/worker-pool-design`
(base of #4041) and `ag2-space/cinny-webclient:dev` both have no
`pull_request` rule today.

Fix: return `None` for "no rule found" instead of 0, and have `main()` treat
that the same as an unreadable ruleset -- print "cannot answer" and exit 2,
never scoring a PR against a bar that doesn't exist. Matches the existing
failed-`gh api` pattern rather than inventing a new one.

Verified against the real branch yixuan cited (not just the mock): `--branch
docs/worker-pool-design` now exits 2 with "cannot answer", was silently 0
before. Updated the one existing test that asserted the old (buggy) `== 0`
return, added a `main()`-level regression pinning rc=2 and confirming no PR's
reviews are ever fetched when the bar is unknown. All 27 tests pass.

Not addressed here (worker-2's second, separately-hedged finding on the same
PR): `require_extra_approval_for_unattributed_changes` may raise the
effective bar above `required_approving_review_count` on commits with
unmapped author emails. Worker-2 marked it "plausible rather than confirmed"
-- no live instance constructed, mine included. Left open for the next pass
rather than guessed at.

Stand: Echo Act IV Pro
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
john-the-dev pushed a commit that referenced this pull request Sep 14, 2026
…dy was asked (#4055)

* feat(proactive-loop): name the PR state where nothing blocks and nobody was asked

A cleared CHANGES_REQUESTED and a satisfied approval bar look identical from
every surface that reports "no blocker": reviewDecision goes quiet, mergeState
stops saying BLOCKED, and nothing says an approval is still missing.

Measured on this repo while writing it: 10 open PRs sit unblocked and short of
the bar, and #3316 reads APPROVED / CLEAN on two counted approvals of which one
is a shared fleet login. So the check reports TWO shortfalls, never one --
github_short_by decides mergeability, distinct_short_by decides whether anyone
outside the shared logins vouched. Collapsing them produced a false "cannot
merge" for #3316 in the first draft.

--shared-login is REPEATABLE because #3482 established that qingyun-wu is also
shared; a single-login version scores the second shared account as a distinct
party, which is the same miscount one layer up.

The bar comes from the branch RULESET. `branches/<b>/protection` answers 404
"Branch not protected" for a ruleset-protected branch, which reads as "no rule".

Perturbation-verified: dropping the shared-login exclusion turns 1 test red,
counting COMMENTED as a verdict turns 2 red; restored, 16 pass.

Stand: Echo Act IV Pro

* test(proactive-loop): cover the I/O layer the first push left at 57%

diff-coverage failed at 57.3% -- missing 83-86, 90-93, 97-99, 102-122, which is
exactly `_gh`, `required_approvals` and `main`. The pure core was tested and the
boundary was not, so the gate was right.

Ten added cases, stubbing the subprocess and `_gh` rather than calling GitHub:
a failing `gh` must RAISE (not return an empty list a caller reads as "no
rules"), an unreadable ruleset exits 2 before any PR is read, an unreadable PR
exits 2 without aborting the PRs after it, and a repeatable --shared-login
reaches classify.

Local: 26 tests, file coverage 100%. Perturbation-verified both new paths --
_gh swallowing a failure turns 1 red, main dropping the per-PR rc=2 turns 1 red.

Stand: Echo Act IV Pro

* fix(proactive-loop): a branch with no pull_request rule is unknown, not zero

yixuan-ag2 found the real blocker: `required_approvals()` returned 0 when a
branch carries no `pull_request` ruleset rule, and 0 reads as a satisfied bar
-- so `classify()` printed "bar met, nobody to recruit" (rc 0) for a PR with
ZERO approvals on any such branch. Reproduced live: `docs/worker-pool-design`
(base of #4041) and `ag2-space/cinny-webclient:dev` both have no
`pull_request` rule today.

Fix: return `None` for "no rule found" instead of 0, and have `main()` treat
that the same as an unreadable ruleset -- print "cannot answer" and exit 2,
never scoring a PR against a bar that doesn't exist. Matches the existing
failed-`gh api` pattern rather than inventing a new one.

Verified against the real branch yixuan cited (not just the mock): `--branch
docs/worker-pool-design` now exits 2 with "cannot answer", was silently 0
before. Updated the one existing test that asserted the old (buggy) `== 0`
return, added a `main()`-level regression pinning rc=2 and confirming no PR's
reviews are ever fetched when the bar is unknown. All 27 tests pass.

Not addressed here (worker-2's second, separately-hedged finding on the same
PR): `require_extra_approval_for_unattributed_changes` may raise the
effective bar above `required_approving_review_count` on commits with
unmapped author emails. Worker-2 marked it "plausible rather than confirmed"
-- no live instance constructed, mine included. Left open for the next pass
rather than guessed at.

Stand: Echo Act IV Pro
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(proactive-loop): an ineligible approval must not satisfy either count

keweichen found the checker still counted approvals from accounts that
cannot satisfy the repository's approval gate. Live repro on #3698:
TustinOC approved but is not a collaborator (404 on `collaborators/TustinOC`,
confirmed live), and the checker scored it as a third approval, printing
"bar met (3/2), nobody to recruit" when only john-the-dev and the shared
qingyun-wu login actually count -- one short, THIN not MET.

Fix: `classify()` takes a new `ineligible` set and excludes those logins
from `counted` (and therefore `distinct`). `main()` resolves eligibility
live via a new `is_collaborator()` (204 = yes, 404 = no, anything else =
undetermined) for every APPROVED login before classifying, and refuses to
answer for a PR ("cannot answer -- eligibility undetermined for ...") rather
than guessing when a check can't be resolved -- an undetermined approver on
one PR does not abort the rest of the run.

Also fixed the test-coverage gap kewei flagged: the existing "no rule ->
exits 2" test asserted only the return code, which a deleted `if required
is None` branch could still satisfy via the generic per-PR error handler.
Now asserts the reviews endpoint was never called; verified it actually
fails when that branch is removed and passes at HEAD.

Verified against the real #3698 case (not just the mock): now prints
"mergeable but thin -- 2/2 (john-the-dev, qingyun-wu), yet only 1 are
outside the shared logins" -- matches kewei's own demonstrated-correct
output exactly. 32 tests (was 27), all pass. review-checks/ruff clean.

Corrected kewei's own speculative aside in the same review: the
"additional approval for unattributed changes" ruleset setting is
documented for unattributed Copilot PRs specifically, not arbitrary
unmapped git-author emails -- left unaddressed since neither of us
reproduced a qualifying case, per the same discipline as the branch's
own last fix.

Stand: Echo Act IV Pro
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* test(proactive-loop): exercise is_collaborator()'s own subprocess boundary

The diff-coverage gate failed on 03e6d22 (93%, lines 98/100-104
uncovered): every existing test monkey-patched `g.is_collaborator` wholesale,
so the function's own body -- the real `gh api` call and its three-way
204/404/other branching -- never ran under test, only its stand-ins did.

Added a `Gh`-class trio mirroring the existing `_gh()` subprocess tests:
204 -> True, 404-in-stderr -> False, and any other failure (rate limit,
network, auth) -> None -- pinning that the undetermined case is never
guessed as False, which is the exact property #4055's fix depends on to
avoid dropping a real collaborator's approval.

100% line coverage on this file locally (was 93%). 35 tests (was 32), all
pass. review-checks/ruff clean.

Stand: Echo Act IV Pro
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: Chi Wang <sonichi@users.noreply.github.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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.

5 participants