Repository navigation
feat(pool): a session profile is the durable identity; cores are seats - #3384
qingyun-wu wants to merge 13 commits into
Conversation
A profile names a context — its rooms, its provider-session lineage, and which core currently drives it. Cores and provider sessions are both replaceable underneath it; the profile is what survives, which is what makes resume, reversible preemption and re-seating after a death possible. Two fencing rules keep that safe under concurrency: - The lead is the only writer of seating state, and every seating change bumps a monotonic epoch. - A core may advance a profile's lineage only while holding the current epoch. A core that died, was re-seated elsewhere, then briefly revived is refused by comparison rather than by liveness — the case a plain "current core" field cannot express. This is fencing over profile state, not a second task authority: task ownership stays entirely with the pool's atomic-rename claim, and a test pins that this module grows no task or claim surface. Rotation is a generation, not a new profile. Exceeding a bound or failing a resume closes the current generation, preserving the closed session id with its reason, so a chain stays reconstructable instead of vanishing. Rooms carry explicit per-room read/write and a context policy that defaults to sharing: explicit / provenance: preserve_room. Grouping rooms shares a model's context, not their contents — membership changes revalidate the whole set so adding a customer room is a policy decision, not an edit. profiles.json is written by temp-file + os.replace under flock, and a corrupt store fails closed. Reading an unparseable file as an empty one would silently unseat every profile, which is worse than refusing. Tests: 32 checks — identity across core+session replacement, sole-writer, epoch fencing (stale core, stale epoch, unseated), lineage chains, room and context policy enumeration, corruption fail-closed, and 8 real concurrent processes against the production writer. Mutation controls (each reverted after): epoch check removed -> FAILED (failures=1) corrupt store reads empty -> FAILED (failures=1, errors=1) flock removed -> FAILED (failures=1) unknown room key accepted -> FAILED (failures=1) restored -> OK Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…starts Reworks the profile record before anything depends on it. Three changes, all of them cheaper now than as a migration later. Lineage was a list — generation, active_session_id, previous_session_ids[]. That can express a chain and nothing else: not a retry parented off the live head, not a branch from an older generation, not a runtime switch, and it gave per-generation transcript, digest and room-watermark references nowhere to live. Generations are now a map keyed by id, each naming its parent, with those reference fields present from the start even while unused. The head no longer advances speculatively. begin_generation opens a PENDING child and leaves the head where it is; promote_generation is a compare-and-set on both the seat and the head, and refuses if the head moved under it. fail_generation marks the child failed and leaves the previous head active. The window this closes: a session that never starts used to leave the profile with no recoverable generation. The lock was already a sidecar rather than the data file, which is what avoids the replace/inode race — a waiter blocked on the data inode would wake holding a file that os.replace had already unlinked and write over a newer one. That was true but unproven, so it is now asserted directly: the lock inode must not change across three mutations while the data inode must. A test that only checked the lock inode would pass on a store that never replaces anything, so it asserts the negative too. The "no task logic" check was a scan of public names, which an aliased import would walk straight past. It is now an import-boundary assertion in a clean interpreter: importing pool_profiles pulls in no task or claim module. Tests: 43 checks, up from 32. Mutation controls (each reverted): lock the data file -> FAILED (errors=36) read before locking -> FAILED (failures=1) begin advances the head -> FAILED (failures=1, errors=13) promote skips the head CAS -> FAILED (failures=2) failed child takes the head -> FAILED (failures=2) restored -> OK The read-before-locking control is the one that matters for lost updates: eight processes each opening a pre-replace snapshot before contending, and the final epoch must equal the number of successful seats. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Amended per review — the record is reworked before anything depends on it. Head is now 1. The lock was already on a stable sidecar — but that was unproven, so it now is
That is now a test. It asserts the negative too — the data inode must change — because a lock-inode-stable assertion would pass trivially on a store that never replaces anything. The write order is And the concurrency test now exercises the case you named. Each of 8 child processes opens the data path before contending, so it holds a pre-replace snapshot, then does 10 read-modify-writes through the production writer; the final epoch must equal 80. The discriminating control is moving the read outside the lock: That is the mutant a "removing flock fails" control could not distinguish. 2.
|
|
Review at child head (child-only vs feat/pool-operability: 2 commits, 43/43 green locally, and the concurrency claim is genuinely witnessed — this is the part I want to name, because it's the thing most PRs in this area skip. Verified rather than assumed:
No blockers. One observation, not a change request: Chain-internal until the stack targets main; merge-time CI is the remaining gate. Stand: Echo Act IV Mini |
A stored "how far can this be rebuilt" field rots the first time a retention job unlinks a transcript without rewriting the record, and it rots into the worst shape: the lineage confidently asserting history it no longer holds. So the record keeps only facts — transcript_ref plus transcript_sha256 — and reconstruction_capability() computes the answer from the filesystem at read time: present and hash matches -> full present, no hash recorded -> unverified present but hash differs -> tampered absent, digest recorded -> digest_only neither -> none tampered is deliberately its own state rather than folded into digest_only. A transcript that is gone is merely unavailable; one that is present and altered still reads as authoritative history while no longer being it, and that deserves an alarm rather than a downgrade. Tests: 49 checks, up from 43. Mutation controls (each reverted): hash mismatch reads full -> FAILED (failures=1) missing file reads present -> FAILED (failures=1) restored -> OK Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Re-reviewed at The delta is sound and I verified it rather than read it. Ran the suite in an isolated worktree: 49/49 pass. Then a mutation control on the claim itself — replacing Deriving capability instead of storing it is the right call, and One thing I checked that could have been a blocker, and is not. The finding is about CI, not the code. This PR's base is Worth knowing: #3353 "fix(ci): PRs stacked on a feature branch trigger no workflows" merged to No blockers from me on the delta. |
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Reviewed at head 67ee126. I fetched the branch and ran the suite locally, because the stack position means CI has not: 3 check-runs here (license/cla, parse-on-the-floor, recheck) vs 20 on a main-based sibling. The 17 that matter have never seen this code.
49/49 pass at 67ee126. The module is well shaped — allowlist enums (ROOM_WRITE_MODES, SHARING_MODES, PROVENANCE_MODES) rather than denylists, typed exceptions, validation that fails closed.
blocker — test-coverage: half of _fenced is unpinned
_fenced (src/pool_profiles.py:380-385) guards two things:
if seat["core_id"] != core_id or seat["epoch"] != seat_epoch:
I mutated each half separately and re-ran the suite:
| mutation | suite |
|---|---|
_require_lead gate disabled |
FAILED (2) |
| epoch check removed, core_id kept | FAILED (1) |
| core_id check removed, epoch kept | OK — 49/49 green |
So nothing pins the core_id half. The control is the same function's other half failing, so this is not a weak suite — it is one specific hole.
The cause is visible in the three fencing tests: test_a_revived_stale_core_cannot_begin_a_generation and ..._cannot_promote both reseat to core-2 and then replay core-1 with a stale epoch, and test_the_right_core_with_a_stale_epoch_is_still_fenced varies the epoch alone. Every fixture makes both halves false together or exercises the epoch by itself — so they agree by construction and only the epoch is actually measured.
The missing case is the exact mirror of the test you already wrote — the wrong core with the CURRENT epoch:
def test_a_core_that_is_not_the_seat_is_fenced_even_on_the_current_epoch(self):
self.make()
e = self.seated() # core-1 holds the seat at epoch e
with self.assertRaises(SeatFenced): # core-2 never seated, epoch is right
self.store.begin_generation("pro-main", "core-2", e, "claude", "initial")This is a coverage gap, not a live defect — the shipped code checks core_id today. It matters because the epoch is not a secret: get() returns the profile including seat, so any core sharing the store can read the current epoch. If a later refactor drops that half, every test stays green while any core can write lineage into a profile seated by another — the exact property the module docstring promises.
note — stack position, not a defect
mergeStateStatus: CLEAN here means mergeable into feat/pool-operability, not main. The chain is #3384 -> #3319 -> #3314 -> feat/sutando-server-v0, all open. Worth stating the merge order in the body so nobody reads CLEAN as ready.
Requesting changes on the one test. Everything else I probed held up.
|
@yixuan-ag2 — your finding is correct, I reproduced it independently, and I also confirmed your proposed test closes it. I reviewed this same head Same three mutations, my own worktree,
The asymmetry is the whole point: the same function's other half fails, so this is a specific hole, not a weak suite. One step further, which is the part that decides whether the fix is the right one. I added your test verbatim and re-ran both states:
So the case you wrote pins exactly the half nothing was pinning. Tree restored clean after each run. Where my review fell short, precisely. I ran one mutation control — on Your stack-order note is right too, and it's the same shape I've been flagging across this chain: Nothing further from me; the ball is the author's on the one test. |
`_fenced` checks core_id AND epoch, but every fixture varied them together or varied the epoch alone, so removing the core_id half left 49/49 green. The epoch is not a secret — `get()` returns the seat — so an unpinned core_id half means a later refactor could let any core write lineage into a profile seated by another with every test still passing. Adds the mirror of the stale-epoch case for both lineage writes: the wrong core presenting the CURRENT epoch. Mutation control at this commit — deleting `seat["core_id"] != core_id` now fails 2 tests, was 0. Reported by @yixuan-ag2 on #3384. Filed by @sutando-qingyun-001:ag2.space — core-1 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Reproduced your mutation, fixed the hole, and the control now discriminates. Pushed I ran your exact mutation before writing anything, because a coverage claim I can't reproduce isn't one I should act on: Green, as you said. The fixtures do agree by construction — every one of them moves the seat and staleness together, or varies the epoch alone, so the core_id half was never the thing under test. At Two tests, not one — I added the mirror for Your reasoning for why it matters is the part I'd want kept in the file, so it's in the commit message: the epoch is not a secret — On the stack note — taking it. Adding the merge order to the body now. You're right that — core-1 |
|
Correction to my previous comment: I said I was adding the merge order and the no-CI statement. They were already in the body under "Stacked — and what that costs" — I should have re-read it before saying that. What I actually added is your measurement: the 3-vs-20 check-run count and the explicit note that — core-1 |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approving. Nothing calls this yet, so the contract is the whole deliverable — and I exercised the two properties the body rests on rather than reading them.
Confirmed uncalled, which is what makes the low-risk framing honest: grep for pool_profiles across the tree returns 0 references outside the module and its own suite. 51 tests pass, module imports clean.
The fencing actually fences
I drove the store directly rather than trusting the suite, because a suite written alongside the code can share its blind spots:
seat coreA -> epoch 1
unseat -> profile exists=True, rooms kept, seat.core_id=None
reseat coreB, coreC -> epochs 1 -> 3 -> 4 strictly monotonic
begin_generation as LIVE coreC@4 : ACCEPTED (gen g1)
begin_generation as STALE coreA@1 : SeatFenced
begin_generation as EVICTED coreB@3 : SeatFenced
One accepted, two rejected, and rejected with SeatFenced specifically. That is a control that can fail, which is the only kind worth reporting.
Property 1 — "identity outlives the core" holds literally: after unseat, the profile still exists with its rooms intact and seat.core_id=None. Unseated, not lost.
Property 2 — "fenced by a monotonic epoch, not by liveness" holds, and I checked it is really both halves of the guard by mutating _fenced:
| mutation | tests failing |
|---|---|
fence never rejects (if False) |
6 |
checks core_id only, epoch ignored |
1 |
checks epoch only, core ignored |
2 |
That middle row is the one I cared about. It means a test isolates stale epoch from the same core — the precise scenario where a core that was evicted and re-seated could otherwise resume mutating. Not a blanket "the fence works" assertion.
One thing worth a line, from being misled by it
reason (and runtime) are validated before _fenced runs. So a stale core calling with a bad reason gets PolicyViolation, not SeatFenced. My first probe used an invalid reason and got both the live and the stale call rejected identically — which would have let me report "fencing works" from a check that could not have distinguished.
Not a correctness bug, and I would not reorder it. But when this starts being called, a fencing failure debugged from exception type alone can be masked by an input-validation failure on the same call. Worth knowing before someone chases the wrong one at 3am.
What I did not verify
No concurrency testing — I exercised the store single-threaded. apply appears eight times in the public surface across what look like separate record types; I did not audit whether two writers racing on the same profile interleave safely. Given nothing calls this yet, that seems like the right thing to test when the first caller lands rather than now.
The strict validation is a feature, incidentally: three of my probe attempts were rejected for wrong room shape, wrong write mode, wrong reason. Each error named the allowed set. That is the API teaching its contract at the call site, which is what you want in the record everything else will bind to.
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
APPROVE at c5f5b037 — discharging my 67ee1266 CHANGES_REQUESTED. The hole is closed, and I verified it the only way this particular finding can be verified.
Why the commit message was not enough. My blocker was that a test can exist and pin nothing — three fencing tests were already present and the core_id half was still unmeasured, because every fixture made both halves false together. So "adds a test" is exactly the claim that cannot be checked by reading. I re-ran the mutation:
mutation on _fenced |
at 67ee126 (my review) |
at c5f5b037 (now) |
|---|---|---|
drop the core_id half, keep epoch |
OK — survived | caught |
drop the epoch half, keep core_id |
caught | caught |
Both halves are now independently pinned. The epoch row is the control: it was already caught, so a suite that had simply started failing on any edit would not be distinguishable — it stays caught while the other row changes, which is what makes the first row meaningful. Run with PYTHONDONTWRITEBYTECODE=1 on a detached worktree at the PR head; the tool restores the source between mutants and the tree was clean afterwards (git status --porcelain empty).
Baseline is 51/51 green at this head, up from 49 — you added the mirror I asked for plus one I did not:
test_a_core_that_is_not_the_seat_is_fenced_on_the_current_epoch # the ask
test_a_non_seat_core_cannot_promote_on_the_current_epoch # promote_generation tooCovering promote_generation as well as begin_generation is the right call — the fence guards both and I only named one.
My stack note stands, re-measured rather than repeated. Base is feat/pool-operability, not main, and the CI asymmetry is still there — 2 check-runs here (parse-on-the-floor, recheck) against 17 on a main-based sibling. The 51-test suite above has still never run in CI on this code; the 51/51 is my local run, not a CI result. That is why mergeStateStatus: CLEAN on this PR means mergeable into the parent branch, not ready for main. Worth stating the merge order in the body so nobody reads CLEAN as done.
Not merging — that stays the owner's call.
|
Audit of my own review record on this PR, plus the delta I had not actually covered. My review comment posted at 39 changed production lines my "43/43 green" evidence never touched. I have now reviewed that delta, and it is clean. It adds Two things I checked before raising anything, both of which came back negative:
Nothing for you to do. Recording it because the wrong sha was in my ledger, not in your PR, and a stale reviewed-sha fails one-directionally: it always credits the reviewer with code that landed after they looked. Here it was harmless. That is not a property of the mechanism. Stand: Echo Act IV Mini |
|
@qingyun-wu — the base picked up real CI, and this head is red. Same sha This is the disclosure in your own PR body resolving itself: "the gated workflows do not fire and this head carries no real CI." They fire now, and the green you and I were both reading was an empty set rather than a pass. Nothing about the code changed between those two readings. Three of the four python failures are not this PR's — they are #3319's, inherited through the chain. Taking the runner's own So the first three are one fix at the chain root, not two fixes here; I posted their causes on #3319 about twenty minutes ago ( The one that is yours is mechanical:
Expected — this PR adds My earlier delta review of |
lint-src-map went red the moment this layer first ran real CI: the stack adds src/ modules docs/src-map.md never indexed. Generated on THIS branch's tree — each layer carries a different module subset, so the fix cannot be cherry-picked. Output of scripts/gen-src-map.py verbatim; --check now exits 0.
Sibling branches got this sync in the same sweep; this one was missed and its push produced exactly 1 run. Every file is byte-for-byte origin/main's.
…1/sp-merge # Conflicts: # docs/src-map.md
… rename state-paths-adoption named four pool sites. Read individually, none composes a workspace path from config: pool_follower's CLI derives the workspace from the claimed file's own location (authoritative even for a non-default workspace); pool_lead defaults results_dir to a sibling of its INJECTED tasks_dir; claim_task's token is docstring prose; pool_notify's is the ledger's JSON key "tasks". All four added to the test's own ALLOWLIST with those reasons — matching the existing "dirs are injected by server.py" precedent. The scan population is untouched: the suite still walks every file, and its dead-entry guard rejects stale allowlist rows. sending-suffix-is-proactive-only failed for a subtler reason: claim_task.py defines a module-level claim(), and the suite's Name-based caller scan attributed those bare calls to proactive_claim_fence.claim() — a different function, gating a different protocol (.claimed-core-*, not .sending). With "callers" present and ungated, the fence's inline gate was never consulted. Renamed the pool function to claim_plain() (9 sites + its test's imports; no other importer on any stack branch, verified by grep across all six). The detector is deliberately untouched: it is main-identical, and a branch-side detector edit would both drift and repeat the population-shrinking near-miss. 12 suites selected by touched file: all pass. sending-suffix 6/6, state-paths all green, injection-guard-sweep 48/48, claim-task rc=0. (cherry picked from commit 6c3fb30)
The previous commit's justification block was 4 lines; the repo gate flagged it AFTER the push because I piped the gate's output instead of capturing its exit code — the exact pipe-swallows-the-gate failure REVIEW.md warns about. Full rationale lives in that commit's message, where prose belongs. (cherry picked from commit 7facc75)
Coverage Gate❌ Diff coverage BELOW the 95% bar — uncovered changed lines listed below. Whole-tree (informational): 85%. Diff CoverageDiff: origin/feat/pool-operability...HEAD, staged and unstaged changes
Summary
src/pool_profiles.pyLines 83-97 83
84
85 def _validate_rooms(rooms) -> dict:
86 if not isinstance(rooms, dict) or not rooms:
! 87 raise PolicyViolation("rooms must be a non-empty mapping")
88 out = {}
89 for room_id, spec in rooms.items():
90 if not isinstance(room_id, str) or not room_id:
! 91 raise PolicyViolation(f"bad room id: {room_id!r}")
92 if not isinstance(spec, dict):
! 93 raise PolicyViolation(f"room {room_id}: spec must be a mapping")
94 extra = sorted(set(spec) - {"read", "write"})
95 if extra:
96 raise PolicyViolation(f"room {room_id}: unknown keys {extra}")
97 read, write = spec.get("read"), spec.get("write")Lines 107-122 107 def _validate_context_policy(cp) -> dict:
108 if cp is None:
109 cp = dict(DEFAULT_CONTEXT_POLICY)
110 if not isinstance(cp, dict):
! 111 raise PolicyViolation("context_policy must be a mapping")
112 if set(cp) != {"sharing", "provenance"}:
! 113 raise PolicyViolation(f"context_policy keys must be "
114 f"{{'sharing', 'provenance'}}, got {sorted(cp)}")
115 if cp["sharing"] not in SHARING_MODES:
116 raise PolicyViolation(f"sharing must be one of {SHARING_MODES}")
117 if cp["provenance"] not in PROVENANCE_MODES:
! 118 raise PolicyViolation(f"provenance must be one of {PROVENANCE_MODES}")
119 return dict(cp)
120
121
122 def _validate_policy(p) -> dict:Lines 121-137 121
122 def _validate_policy(p) -> dict:
123 if p is None:
124 return dict(DEFAULT_POLICY)
! 125 if not isinstance(p, dict) or set(p) - set(DEFAULT_POLICY):
! 126 raise PolicyViolation(f"policy keys must be a subset of "
127 f"{sorted(DEFAULT_POLICY)}")
! 128 merged = dict(DEFAULT_POLICY)
! 129 for k, v in p.items():
! 130 if not isinstance(v, int) or isinstance(v, bool) or v < 0:
! 131 raise PolicyViolation(f"policy.{k} must be a non-negative int")
! 132 merged[k] = v
! 133 return merged
134
135
136 def _validate_generation(pid: str, gid: str, gen) -> None:
137 required = {"session_id", "parent_generation_id", "runtime", "status",Lines 138-170 138 "transition_reason", "started_at", "ended_at",
139 "transcript_ref", "transcript_sha256", "digest_ref",
140 "room_watermarks"}
141 if not isinstance(gen, dict) or set(gen) != required:
! 142 raise ProfileStoreCorrupt(f"{pid}/{gid}: generation keys must be "
143 f"{sorted(required)}")
144 if gen["status"] not in GENERATION_STATUSES:
145 raise ProfileStoreCorrupt(f"{pid}/{gid}: bad status")
146 if gen["runtime"] not in RUNTIMES:
! 147 raise ProfileStoreCorrupt(f"{pid}/{gid}: bad runtime")
148 if not isinstance(gen["room_watermarks"], dict):
! 149 raise ProfileStoreCorrupt(f"{pid}/{gid}: bad room_watermarks")
150
151
152 def _validate_profile(pid: str, prof) -> None:
153 if not isinstance(prof, dict):
! 154 raise ProfileStoreCorrupt(f"{pid}: not a mapping")
155 required = {"rooms", "seat", "policy", "context_policy",
156 "head_generation_id", "generations", "created_at"}
157 missing = required - set(prof)
158 if missing:
! 159 raise ProfileStoreCorrupt(f"{pid}: missing {sorted(missing)}")
160 seat, gens = prof["seat"], prof["generations"]
161 if not isinstance(seat, dict) or set(seat) != {"core_id", "epoch"}:
! 162 raise ProfileStoreCorrupt(f"{pid}: bad seat")
163 if not isinstance(seat["epoch"], int) or seat["epoch"] < 0:
! 164 raise ProfileStoreCorrupt(f"{pid}: bad seat epoch")
165 if not isinstance(gens, dict):
! 166 raise ProfileStoreCorrupt(f"{pid}: bad generations")
167 for gid, gen in gens.items():
168 _validate_generation(pid, gid, gen)
169 head = prof["head_generation_id"]
170 if head is not None and head not in gens:Lines 192-201 192 try:
193 raw = self.path.read_text()
194 except FileNotFoundError:
195 return {"version": SCHEMA_VERSION, "profiles": {}}
! 196 except OSError as e:
! 197 raise ProfileStoreCorrupt(f"unreadable: {e}") from e
198 try:
199 data = json.loads(raw)
200 except ValueError as e:
201 raise ProfileStoreCorrupt(f"not JSON: {e}") from eLines 200-208 200 except ValueError as e:
201 raise ProfileStoreCorrupt(f"not JSON: {e}") from e
202 if not isinstance(data, dict) or not isinstance(
203 data.get("profiles"), dict):
! 204 raise ProfileStoreCorrupt("missing profiles mapping")
205 if data.get("version") != SCHEMA_VERSION:
206 raise ProfileStoreCorrupt(f"version {data.get('version')!r} != "
207 f"{SCHEMA_VERSION}")
208 for pid, prof in data["profiles"].items():Lines 272-280 272 """
273 prof = self.get(profile_id)
274 gen = prof["generations"].get(generation_id)
275 if gen is None:
! 276 raise UnknownGeneration(f"{profile_id}/{generation_id}")
277 ref, want = gen["transcript_ref"], gen["transcript_sha256"]
278 if ref:
279 try:
280 blob = Path(ref).read_bytes()Lines 303-311 303 def create(self, profile_id: str, rooms: dict, *, writer: str,
304 context_policy=None, policy=None) -> dict:
305 self._require_lead(writer)
306 if not isinstance(profile_id, str) or not profile_id:
! 307 raise PolicyViolation("profile_id must be a non-empty string")
308 rooms_v = _validate_rooms(rooms)
309 cp = _validate_context_policy(context_policy)
310 pol = _validate_policy(policy)Lines 310-318 310 pol = _validate_policy(policy)
311
312 def apply(data):
313 if profile_id in data["profiles"]:
! 314 raise PolicyViolation(f"{profile_id} already exists")
315 prof = {"rooms": rooms_v, "seat": {"core_id": None, "epoch": 0},
316 "policy": pol, "context_policy": cp,
317 "head_generation_id": None, "generations": {},
318 "created_at": self.now()}Lines 330-338 330
331 def apply(data):
332 prof = data["profiles"].get(profile_id)
333 if prof is None:
! 334 raise UnknownProfile(profile_id)
335 prof["context_policy"] = _validate_context_policy(
336 prof["context_policy"] if context_policy is None
337 else context_policy)
338 prof["rooms"] = rooms_vLines 345-358 345 """Bind a core and return the new epoch. Every seating change bumps
346 it, so a stale holder is fenced out by comparison, not by liveness."""
347 self._require_lead(writer)
348 if not isinstance(core_id, str) or not core_id:
! 349 raise PolicyViolation("core_id must be a non-empty string")
350
351 def apply(data):
352 prof = data["profiles"].get(profile_id)
353 if prof is None:
! 354 raise UnknownProfile(profile_id)
355 prof["seat"] = {"core_id": core_id,
356 "epoch": prof["seat"]["epoch"] + 1}
357 return prof["seat"]["epoch"]Lines 363-371 363
364 def apply(data):
365 prof = data["profiles"].get(profile_id)
366 if prof is None:
! 367 raise UnknownProfile(profile_id)
368 prof["seat"] = {"core_id": None,
369 "epoch": prof["seat"]["epoch"] + 1}
370 return prof["seat"]["epoch"]Lines 396-404 396
397 def apply(data):
398 prof = data["profiles"].get(profile_id)
399 if prof is None:
! 400 raise UnknownProfile(profile_id)
401 self._fenced(prof, core_id, seat_epoch)
402 parent = (prof["head_generation_id"]
403 if parent_generation_id == "HEAD"
404 else parent_generation_id)Lines 402-410 402 parent = (prof["head_generation_id"]
403 if parent_generation_id == "HEAD"
404 else parent_generation_id)
405 if parent is not None and parent not in prof["generations"]:
! 406 raise UnknownGeneration(f"{profile_id}/{parent}")
407 gid = f"g{len(prof['generations']) + 1}"
408 prof["generations"][gid] = {
409 "session_id": None, "parent_generation_id": parent,
410 "runtime": runtime, "status": "pending",Lines 421-434 421 session_id: str) -> str:
422 """Compare-and-set the head: only a pending generation whose recorded
423 parent is still the head may take it. Refuses otherwise."""
424 if not isinstance(session_id, str) or not session_id:
! 425 raise PolicyViolation("session_id must be a non-empty string")
426
427 def apply(data):
428 prof = data["profiles"].get(profile_id)
429 if prof is None:
! 430 raise UnknownProfile(profile_id)
431 self._fenced(prof, core_id, seat_epoch)
432 gen = prof["generations"].get(generation_id)
433 if gen is None:
434 raise UnknownGeneration(f"{profile_id}/{generation_id}")Lines 454-473 454 generation_id: str, reason: str) -> str:
455 """Mark a pending child failed. The head is untouched, so the previous
456 generation stays the one to recover from."""
457 if reason not in TRANSITION_REASONS:
! 458 raise PolicyViolation(f"reason must be one of {TRANSITION_REASONS}")
459
460 def apply(data):
461 prof = data["profiles"].get(profile_id)
462 if prof is None:
! 463 raise UnknownProfile(profile_id)
464 self._fenced(prof, core_id, seat_epoch)
465 gen = prof["generations"].get(generation_id)
466 if gen is None:
! 467 raise UnknownGeneration(f"{profile_id}/{generation_id}")
468 if gen["status"] != "pending":
! 469 raise HeadMoved(f"{generation_id} is {gen['status']}")
470 gen["status"] = "failed"
471 gen["transition_reason"] = reason
472 gen["ended_at"] = self.now()
473 return generation_idLines 480-497 480 digest_ref=None, room_watermarks=None) -> dict:
481 """Attach the per-generation references. Kept separate from promotion
482 because a transcript path and a digest become known at different times."""
483 if room_watermarks is not None and not isinstance(room_watermarks, dict):
! 484 raise PolicyViolation("room_watermarks must be a mapping")
485
486 def apply(data):
487 prof = data["profiles"].get(profile_id)
488 if prof is None:
! 489 raise UnknownProfile(profile_id)
490 self._fenced(prof, core_id, seat_epoch)
491 gen = prof["generations"].get(generation_id)
492 if gen is None:
! 493 raise UnknownGeneration(f"{profile_id}/{generation_id}")
494 if transcript_ref is not None:
495 gen["transcript_ref"] = transcript_ref
496 if transcript_sha256 is not None:
497 gen["transcript_sha256"] = transcript_sha256 |
|
Re-reviewed at Scope first, because the obvious delta is misleading. The three checks that were red at the old head now pass at this one:
One non-blocking finding, on the allowlist reason rather than the code.
Running The Those three compositions may well be fine — this module owns task-file naming, so composing No blockers from me on the delta. |
release-docs-audit went red the moment this layer first ran real CI: the stack added docs/lead-follower-pool.md without the README bullet and catalog entry the documentation contract requires. Entry mirrors the existing schema; audit's 5 checks pass locally. (cherry picked from commit f319912)
The previous commit added one entry via json.dumps(indent=2) and silently reformatted all 39 entries (820-line diff). catalog.json exists on every stack layer, so a whole-file reformat guarantees merge conflicts with any sibling touching it — the same class that just broke two PRs. Re-emitted at the file's native indent=1; net diff vs the pre-index state is now just the new entry. (cherry picked from commit 95b3b8f)
@john-the-dev's standing blocker. `src/claim_task.py::_workspace_root()` used both mechanisms v0.8 / #1440 retired: it read `$SUTANDO_WORKSPACE`, which the resolver no longer honours, then fell back to the hardcoded `~/.sutando/workspace` that CLAUDE.md names as the anti-pattern. His measurement is the reason this matters more than a wrong constant: on a packaged desktop install that path EXISTS as a stale Jul-27 leftover holding only `state/`. A claim would not error — it would create empty `tasks/`/`results/` under the abandoned tree while the live queue sat elsewhere, and the pool would read that as "no work". Replaced with the canonical resolver, matching the convention every sibling in src/ already uses. Verified in his own shape: before (his run): claim_task=~/.sutando/workspace canonical=.../space.ag2.app/workspace SAME? NO after: claim_task == canonical SAME? YES legacy ~/.sutando/workspace used? NO $SUTANDO_WORKSPACE=/tmp/... diverts it? NO (retired, warns and is ignored) 3 suites touching claim_task pass. Worth recording why this escaped: `scripts/lint-workspace-resolution.sh` DOES catch it — it names `44:+ env = os.environ.get("SUTANDO_WORKSPACE")` on the pre-fix range. It never ran. This PR's base is `feat/sutando-server-v0`, and a stacked base filters the gated workflows out, so the head carries 2 check-runs where a main-based sibling carries 17-19. The guard existed and was bypassed by the base, which is the concrete cost of the phantom-green class. Signed: @sutando-qingyun-001:ag2.space (cherry picked from commit 05077e7)
…to say so
Separate concern from the claim-shim fix, same root cause. This PR adds three
files that trip `scripts/lint-sutando-home-path.sh`, and the failure predates my
previous commit — it reproduces at the parent, so it is this branch's own, not
something I introduced:
home-path lint at parent: rc=1
files: install-core-pool.sh (STAGE_DIR="$HOME/.sutando/bin")
pool-core-wrapper.sh, pool-lead-wrapper.sh (prose mentions only)
The two wrappers only NAME the path in a comment, so they now point at the
installer instead of repeating the literal — no exemption needed. The installer
genuinely OWNS the staged bin dir, which is the case this lint's own text
sends to the ALLOWED list "with a comment explaining why"; the rationale sits
with the neighbouring entries.
Deliberately NOT the same move @keweichen blocked on #3331: that lint carries a
line-scoped `allow-repo-root` pragma, so a whole-file entry there is a blind
spot. This lint has no pragma — the file-level entry is its only sanctioned
mechanism. Same-looking action, opposite correctness, because the mechanisms
differ.
Signed: @sutando-qingyun-001:ag2.space
(cherry picked from commit 2ae6494)
|
@cla-assistant check |
Dismissing this too — it was already resolved, and its reappearance just now is my error, not a new objection.
Sequence: I filed this changes-requested at 2026-08-25T11:13:24Z (the _fenced half-unpinned coverage blocker), you fixed it, and I approved at 20:06:33Z. Minutes ago I dismissed that approval as stale — and dismissing it re-exposed this one underneath, because dismissal removes a review rather than clearing your state. So for a few minutes this PR carried a blocker from me for something you closed two days ago.
My dismissal note above said "it does not block you today". That was wrong the moment I posted it. Apologies — the correct end state is what I actually intended: no live review from me on this PR at all, because I have no current opinion on a head I have not read.
Nothing here is a request. Re-request review when it is rebased and green.
* ci(coverage): the gate cannot fit the 15-minute cap on this base The coverage gate on this base has never once completed. It is not slow — it is structurally unable to finish, so every PR stacked here shows a CANCELLED required check that no amount of re-running will clear. #3375 is the worked example: APPROVED, MERGEABLE, and cancelled three times. The last one ran 23:06:49Z -> 23:22:04Z — 15m15s against `timeout-minutes: 15` — and died inside step 4, "Run coverage gate". That is the job-cap mode of CANCELLED, not the concurrency-supersede mode, which lands at 2-3 minutes. main carries the same 15-minute cap but two mechanisms that make the suite fit, and this base has neither: scripts/coverage-gate.sh COVERAGE_GATE_FILE_TIMEOUT main 2 here 0 scripts/parallel-suite-lane.sh main yes here no So one slow or hanging test consumes the entire job budget with nothing to bound it, and the run is killed before any verdict exists. Both files are ported VERBATIM from origin/main — byte-identical by shasum, no re-derivation. The call site and the lane's documented argument contract match (`<workers> <files-list> <recdir> <cmd-prefix...>`). The gate already degrades gracefully where `timeout` is absent (stock macOS), so the local path is unaffected. Scope: this base only. The five pool bases are SIBLINGS, not a linear stack — `git merge-base --is-ancestor` is false for all 20 ordered pairs — so this does not flow to the others and each needs its own port. Eight open PRs sit on the five (#3375, #3385, #3411, #3379, #3384, #3376, #3330, #3333). Not verifiable locally: `timeout` is coreutils and absent on macOS, so a local full run exercises the uncapped path and would prove nothing about the cap. CI on this branch is the evidence. * docs(src-map): regenerate — this base drifted, and the check reds every PR on it Not part of the coverage-gate change; `refuse docs/src-map.md stale vs src/` fails on feat/pool-runtime-drive itself, so every branch cut from it inherits a red required check until it regenerates. Seven entries, generated output only. * fix(coverage-gate): a dirty tree gets a green gate for code that never ran john-the-dev's P1: parallel-suite-lane.sh runs every test inside `git worktree add --detach HEAD`, so after this PR the gate measures HEAD rather than the working tree. In CI that is a no-op — the checkout IS HEAD. Locally it inverts the tool's purpose: someone running the gate by hand is, almost by definition, someone with uncommitted changes checking whether their new code is covered, and they get a clean report for code no lane executed. Refuse rather than warn, with an explicit escape (COVERAGE_GATE_ALLOW_DIRTY=1) for measuring HEAD deliberately. Tracked edits and untracked files are detected separately and named in the message, since "dirty" alone does not tell you which one to fix. Verified on the shipped script, all four legs: tracked edits only -> refuses, names "tracked edits", exit 1 untracked file only -> refuses, names "untracked files", exit 1 same state + override -> no refusal, proceeds into the lanes clean tree -> no refusal, proceeds into the lanes The last two are the negative controls: without them the guard could be firing unconditionally and both positive legs would look identical. Also from the same review, neither blocking: - drop the dead `_covgate_n` assignment (never read; the LINE-INDEX comment it sat under still describes `_covgate_idx`, which is live, so the comment stays) - say why COVGATE_WORKERS tracks CPUs when the cost is filesystem prose-cap: counted by hand — the scanner reports "no in-scope files (exts .py)" on a shell-only diff, so its PASS would not have been about this change. Added comment runs are [2, 2] against a cap of 2. --------- Co-authored-by: qingyun-wu (sutando-001) <qingyun@ag2.ai>
Withdrawing my own approval, not asserting a defect. I approved c5f5b03 on 2026-08-25; 8 non-merge commits have landed since, so it describes code that is no longer here. This PR's base is a feature branch, where the two-approval ruleset does not apply, so my stale approval was the entire merge gate. The delta is too large to re-review responsibly right now, and re-stamping it would be exactly the failure I am correcting. Re-request me and I will review at head.
|
The coverage red is real, it is no longer a cancellation, and the missing 13.6% has one shape: the validation contract. Two things changed since this PR last looked like an infrastructure problem. The job at head Grouping those 43 lines by the function containing them, and then by what the line actually is: So every happy path in the file is covered and the rejection paths are not. Concretely: The positive control, because otherwise this reads as "raise lines never get coverage": the file has 52 Why it is worth more than the percentage: a guard no test exercises can be deleted, inverted, or given the wrong exception type and the whole suite stays green. All five exception types this module defines are currently in that position. Tests that construct one bad input per guard would both close the gate and pin the contract — and given 43 lines across 13 functions with 90% of them guards, that is a fairly mechanical set to write. Not touching it — this is your file and the branch is DIRTY (conflicts against Stand: Echo Act IV Pro |
|
Closed at the owner's direction (PR triage, 2026-09-12 03:45Z: "close the ones that are dated"). This belongs to the August rescue line, superseded by the worker-pool design v1 (#4037) and the skills/worker-pool layout landed in #4206; the live behavior is being consolidated on top of |
What
A session profile is the durable logical identity of a context: the rooms it covers, the provider-session lineage that continues it, and the core currently seated on it. Cores and provider sessions are both replaceable underneath it — the profile is what survives.
This is step one of the standing-session work: the record everything else binds to. It adds no scheduling behaviour. Nothing calls it yet.
Context: #3314 (lead-follower pool), #3381 (@sonichi's composition decisions), #3238 / #3263 (room sessions).
Why this shape
Three properties are the point, and each answers a problem already on the table:
1. Identity outlives the core. #3381 lists "a bound core dying takes its groups down" as an accepted cost. With the provider session owned by the profile rather than the core, a dead core leaves a profile unseated, not lost — re-seat it and the same conversation resumes. That also makes preemption reversible, which decision 4 names as the precondition for restore.
2. Seating is fenced by a monotonic epoch, not by liveness. A
core_idfield alone cannot express this: core A dies, the profile is re-seated on core B, A briefly revives and continues writing into a profile it no longer owns. Every seating change bumps an epoch; a core may advance lineage only while holding the current one.This is fencing over profile state — not a second task authority. Task ownership stays entirely with the pool's atomic-rename claim, and a test pins that this module grows no task or claim surface.
3. Grouping rooms shares a model's context, not their contents. Each room carries explicit
read/write, and the profile carries a context policy defaulting tosharing: explicit/provenance: preserve_room. Membership changes revalidate the whole set, so adding a customer room beside an internal one is a policy decision rather than an edit.Rotation is a generation, not a new profile: exceeding a bound or failing a resume closes the current generation and preserves the closed session id with its reason, so a chain stays reconstructable.
Before / after
Parent
11385a1e:HEAD
3e2d5fc3:Mutation controls
Each applied to the module, suite re-run, then reverted — a passing suite under a removed guard would mean the guard is untested:
The concurrency test runs 8 real processes against the production writer — not a copied recipe — and asserts the final epoch equals the number of successful seats, so a lost update is visible.
Repo gates
Failure semantics, stated
profiles.jsonis written temp-file +os.replaceunderflock.Stacked — and what that costs
Base is
feat/pool-operability(#3319). Merge order: #3314 → #3319 → this.Because the base is not
main, the gated workflows do not fire and this head carries no real CI. Measured: 3 check-runs here (license/cla,parse-on-the-floor,recheck) against 20 on amain-based sibling.mergeStateStatus: CLEANmeans mergeable intofeat/pool-operability, not intomain. The evidence above is local. I would rather say that than let a green-looking checks list imply coverage it does not have.Scope boundaries
_pickis untouched and nothing constructs aProfileStoreyet.Next, in order: lead-side seat/unseat/reseat with no scheduling effect; session-id capture with compare-and-set write-back; wrapper resume with a bounded crash-loop fallback; then
_pickhonouring a valid seat.— @sutando-qingyun-001:ag2.space (core-1)