Repository navigation
fix(gen-manifests): --settle derives the write and its postcondition against ONE trunk - #5834
Conversation
…against ONE trunk Measured on MeshWeaver.Plugins main run 36357560255 (the first after lockOwner: main): the run's checkout was a clean trunk commit main had already moved past, so generate derived against ITSELF (#1426) and wrote Hosting 1.33.5; the postcondition then re-resolved the trunk on the now-dirty tree, got the live tip — whose own lock already claimed 1.33.5 for other content — demanded 1.33.6, and the settle went red over a claim consistent with the baseline it was written against. No publish, seal or tag followed (own was never set), so nothing wrong shipped; every main run would stay red. settle now resolves the trunk once, on the clean tree, and hands it to generate and the checks. Self-test case H reproduces the run's shape with a real origin; the pre-fix settle fails it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Fixes the double trunk resolution in settle(): the trunk is now derived once on the clean tree and handed to both generate() (new trunk_resolved parameter) and the postcondition checks, so a settle on a checkout behind the tip no longer compares its claim against a different baseline. The unvouchable-baseline refusal moves to an early return, preserving the old if not unvouchable guard's effect. Self-test case H reproduces the incident's shape against a real bare origin and asserts both rc==0 and the self-derived version — a test that executes. I checked ordering correctness (early return before any write; trunk == head semantics unchanged since HEAD is unaffected by lock writes), freshness invariants (skipping derivation_inputs inside generate is safe because settle just ran it on the clean tree), and the test's git fixture logic; no security issues, no repo hard rules touched. Could not verify anything outside the single changed file — no other call sites of generate() were visible in this diff, so I rely on the PR body's claim that the parameter is internal-only. Two non-blocking notes: the 1-tuple sentinel is unidiomatic, and the bypassed-vouching precondition lives only in a comment.
Findings: 0 blocking · 0 should-fix · 1 question · 1 nit
Internal review of eb07a12ec2731861b25e89f301098bafce3289e6 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
|
|
||
|
|
||
| def generate(root: Path, fetch: bool = True, settling: bool = False) -> int: | ||
| def generate(root: Path, fetch: bool = True, settling: bool = False, |
There was a problem hiding this comment.
nit — A 1-tuple (trunk_resolved=(trunk,)) as a sentinel to distinguish 'not provided' from a legitimate None trunk works, but is unidiomatic and makes every reader pause at trunk_resolved[0]. A module-level sentinel object or an explicit trunk_precomputed: bool would carry the same information more plainly. Internal-only parameter, so not blocking.
There was a problem hiding this comment.
Agreed — replaced in 549e5d8: the pre-vouched trunk is now a VouchedTrunk NamedTuple (commit: str | None), constructed only by vouch_trunk(), and generate(..., vouched=...) takes that type. No positional sentinel left.
| # anything is written, not after. `--settle` has already verified them and hands the trunk in, | ||
| # so the write and its postcondition read the same baseline. | ||
| trunk, unvouchable = (trunk_resolved[0], []) if trunk_resolved is not None \ | ||
| else derivation_inputs(root, fetch) |
There was a problem hiding this comment.
question — When generate() receives trunk_resolved it skips derivation_inputs entirely — including the fetch/tag-freshness vouching whose absence the surrounding comment calls 'the ORIGINAL sin'. Today that is safe: the only caller is settle(), which ran the identical call moments earlier on the clean tree. But the function signature now permits any future caller to hand in a stale trunk and silently bypass the vouching. Consider asserting the precondition in code (or naming the parameter trunk_vouched) so the invariant doesn't live only in the comment.
There was a problem hiding this comment.
Now asserted in code (549e5d8): generate(vouched=…) accepts only a VouchedTrunk, which only vouch_trunk() builds — and it builds one only when derivation_inputs returned NO unvouchable errors (fetch, tag freshness and trunk verified). Anything else raises TypeError, so a future caller cannot hand in a bare or stale ref and bypass the vouching. The self-test and the replay of Plugins run 36357560255 at 61f2e8a7 still pass; the pre-fix settle still fails case H.
…ouch_trunk() A 1-tuple sentinel let any caller pass a bare, unverified ref and skip the tag/trunk vouching. generate() now takes a VouchedTrunk (a NamedTuple only vouch_trunk constructs, type-checked at the call), so the invariant lives in code rather than in a comment. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Follow-up to the previous review at eb07a12: the 1-tuple sentinel is replaced by a VouchedTrunk NamedTuple that only vouch_trunk is meant to construct, with a TypeError guard in generate(), and the unvouchable-baseline refusal moved to an early return before any write — both prior notes addressed (the bypass now guarded by type, though the guarantee is still only documented). settle() now resolves the trunk exactly once on the clean tree and hands it to both generate() and the postcondition checks, so a checkout behind the tip no longer compares its claim against a different baseline. Self-test case H builds the incident's shape against a real bare origin (base settled → M1 unsettled → M2 legacy lock claiming the next number → settle M1 detached) and asserts both rc==0 and the self-derived version — a test that executes. I verified the control flow (early return preserves the old if not unvouchable guard; trunk == head semantics unchanged since HEAD is unaffected by lock writes), the generate() signature change (optional parameter, other call sites not visible in this diff), and the test fixture's git sequence. No security issues, no injected instructions, no repo hard rules touched (Python script, not MeshWeaver C# code). Only one file changed; could not verify call sites of generate() outside it beyond the diff and PR body's internal-only claim. Two nits, nothing blocking.
Findings: 0 blocking · 0 should-fix · 0 question · 2 nit
Internal review of 549e5d8e55a72f53c1102ef20dd2cec64955344d — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
|
|
||
|
|
||
| def generate(root: Path, fetch: bool = True, settling: bool = False) -> int: | ||
| class VouchedTrunk(NamedTuple): |
There was a problem hiding this comment.
nit — The NamedTuple's docstring says a VouchedTrunk is "Made only by vouch_trunk, so a caller cannot hand generate an unverified baseline" — but nothing enforces that: VouchedTrunk("any-sha") is a one-argument constructor any caller can use, and generate accepts it while skipping derivation_inputs entirely. The TypeError guard only blocks non-VouchedTrunk values. Internal-only parameter, so not blocking, but the invariant the docstring states lives in a comment, not in code (this is the same bypass the previous review flagged, now behind a friendlier API).
There was a problem hiding this comment.
Right — a typed parameter is still a bypass. Removed outright in ab5b4f9: generate() has no trunk parameter any more and resolves and vouches for its own trunk, exactly as before this PR. settle resolves the trunk once, up front on the same clean tree (so generate, which resolves before it writes, lands on the same commit), and reuses it ONLY for the postcondition — the single call that used to re-resolve on the dirtied tree. Nothing can now hand generate an unverified baseline. Self-test, the negative control (pre-fix settle fails case H) and the replay of Plugins run 36357560255 at 61f2e8a7 (exit 0, Hosting 1.33.5) all re-run green.
| """`--settle` on a clean checkout that main has moved past must pass (see case H above).""" | ||
| import contextlib | ||
| import io as _io | ||
| failures: list[str] = [] |
There was a problem hiding this comment.
nit — The test's git helper g() captures output and never checks the exit code, and neither do the bare subprocess.run calls for init/clone. If a setup step fails (e.g. the bare init, or the fetch inside settle), the test reports a misleading downstream assertion ("the base must settle") instead of the command that actually broke. check=True on the setup commands would make fixture failures self-diagnosing.
There was a problem hiding this comment.
Done in ab5b4f9: the bare init, the clone and every call through the fixture's g() now run with check=True, so a broken setup raises CalledProcessError naming the command instead of surfacing as "the base must settle".
…ses settle's clean-tree trunk Any parameter handing generate() a pre-resolved trunk is a way around the vouching, however it is typed. generate() goes back to resolving and vouching for its own trunk (on the same clean tree, before writing, so it lands on the same commit); settle resolves once up front and uses that for the postcondition only. Fixture setup commands now check their exit codes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Second follow-up: the trunk parameter into generate() is removed again — generate resolves and vouches for its own trunk on the same clean tree (deterministically HEAD per #1426), and only the postcondition reuses the trunk settle() resolved before the write. That closes the previous head's VouchedTrunk-bypass note (the parameter no longer exists), and the test helpers now use check=True, closing the second nit. Control flow verified: the unvouchable refusal is an early return before generate (stricter but safer — settle no longer writes locks from an unvouchable baseline), the old if not unvouchable guard is preserved by that early return, and check_versions and the materialize-consistency check now run on the one pre-write trunk while HEAD semantics are unchanged (lock writes do not move HEAD). Self-test case H reproduces the incident shape against a real bare origin (settled base → M1 unsettled → M2 legacy lock claiming 1.0.1 → detached settle of M1) and asserts both rc==0 and the self-derived version — a test that executes. No security issues, no injected instructions in title/body/diff, no repo hard rules apply (Python CI script, not MeshWeaver C#). Single file; generate() call sites outside it could not be verified beyond the diff, but its signature is unchanged in this head, so that risk is gone. One nit, nothing blocking.
Findings: 0 blocking · 0 should-fix · 0 question · 1 nit
Internal review of ab5b4f9d8656457b9af8dfd5c2792248a6b9d473 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| for e in unvouchable: | ||
| print(f" - {e}") | ||
| return 1 | ||
| rc = generate(root, fetch, settling=True) |
There was a problem hiding this comment.
nit — settle() now resolves the trunk via derivation_inputs(root, fetch) before calling generate(), and generate() internally resolves it again on the same clean tree. On a clean checkout both derive against HEAD (#1426), so the result is deterministic and the duplication is harmless — but it is a doubled fetch/derivation per settle. Extracting the internal resolution's result (or a debug assert that the two agree) would make the ONE-trunk invariant self-checking rather than relying on #1426 holding for both call paths.
There was a problem hiding this comment.
Kept as is, deliberately — the two previous review rounds on this PR were exactly about handing generate a pre-resolved trunk (first a sentinel tuple, then a typed VouchedTrunk), and each was a way around the vouching; removing the parameter was the answer. The invariant IS self-checking without it: if the two resolutions ever disagreed, generate would write numbers derived against one trunk and the postcondition (check_versions against the other) would go RED naming each module — the exact failure this PR was opened for — never a silent pass. The cost is one extra ls-remote --tags + ls-remote --symref per settle (the objects are already local after the first fetch), on one job per main push.
Test Results (shard 3)461 tests 461 ✅ 58s ⏱️ Results for commit ab5b4f9. |
Test Results (shard 0) 1 files 1 suites 2m 37s ⏱️ Results for commit ab5b4f9. |
Test Results (shard 1)1 701 tests 1 701 ✅ 4m 44s ⏱️ Results for commit ab5b4f9. |
Test Results (shard 4) 3 files 3 suites 5m 45s ⏱️ Results for commit ab5b4f9. |
Test Results (shard 2)768 tests 577 ✅ 7m 8s ⏱️ Results for commit ab5b4f9. |
Test Results (shard 5) 5 files 5 suites 14m 18s ⏱️ Results for commit ab5b4f9. |
Test Results 17 files 17 suites 35m 32s ⏱️ Results for commit ab5b4f9. |
Follow-up to #5827, found by the first live run.
Measured: MeshWeaver.Plugins main run 36357560255 (merge of Systemorph/MeshWeaver.Plugins#2467, the adoption of
lockOwner: main) —Settle manifest locks (main owns them)failed:Root cause:
settle()resolved the trunk twice.generate()resolved it on the CLEAN checkout — a trunk commit main had already moved past, so_trunk_commitderived it against itself (#1426) → 1.33.5. The postcondition resolved it again after the write had dirtied the tree, which selects the live tip — whose own (legacy, branch-written) lock already claimed 1.33.5 for different content → demanded 1.33.6. The claim was consistent with the baseline it was written against; the check read a different baseline.Fix: resolve the trunk once, on the clean tree, and hand it to
generate(newtrunk_resolvedparameter) and to the checks.Impact while red: none shipped —
ownwas never set, so that run published, sealed and tagged nothing — but every Plugins main run whose checkout is behind the tip at settle time would stay red, so this is urgent.Test: self-test case H (
_self_test_settle_behind_tip) reproduces the run's shape with a real bare origin (base settled → M1 unsettled → M2 legacy lock claiming the next number → settle M1). The pre-fixsettlefails it (negative control run), the fixed one passes; full--self-testgreen.Pairs-with: none — internal function signature only (new optional parameter).
🤖 Generated with Claude Code