fix(runtime-host): separate the persisted grant record from the authority it derives - #4427
Conversation
a8ec928 to
9e575f1
Compare
|
I re-reviewed the latest head (
The control-namespace capture and released credential fixture are valuable and should stay. I would not add generic per-grant semantic probes: once unknown grants fail closed, the real forward roll catches omissions, while focused migration tests remain the right place to verify each explicitly chosen successor. |
9e575f1 to
01d119f
Compare
…rity it derives A credential's grant list served two masters. As a record it should hold what some earlier release wrote; as an authority it may only name operations this build defines and this principal's policy allows. Decoding reconciled them in place, so every vocabulary change had to be patched into the same array — first three hand-written constants, then a fourth for the SessionTodo cutover that stranded workspaces holding a credential issued before it (#4420). The record is now `StoredAccessCredential.grants`, kept as the file states it, and `effectiveOperationGrants` derives the authority on every decode without writing back. A grant this build cannot serve is absent from the authority and present in the record, so an unrelated later mutation no longer erases it — neither a newer build's key seen by an older one, nor a key whose migration entry was forgotten. `PERSISTED_GRANT_MIGRATIONS` is the only thing that rewrites the record, and a replacement naming no successor fails to compile. Deriving per principal also puts a Client Capability provider under its own policy rather than the remote owner's, which decoding never applied, and retires the Session Guest special case: its record was never authoritative, so it is simply one more derivation rule. The published JSON keeps `operationGrants` as its key. An explicit encoder states the on-disk shape once, so a field added to the runtime type cannot reach the file by accident. Refs #4420 Generated-by: Claude Code (claude-opus-5)
The forward roll restored and compared the State Root alone, but the record that decides whether a Host starts at all is the access credential file, and it lives in the account-local control namespace rather than inside the Root. The harness was structurally blind to it: no seeded credential, no golden capture of that directory, so a release could strand every existing workspace and still qualify. The fixture now asks the released build to issue a credential with everything it is able to grant, rather than naming operations here — a fixture that hard-codes today's keys stops covering the next rename the moment it lands. Golden capture and restore span both durable locations. Inspection also asserts that the reading build can account for every stored grant. That question is asked of whichever build is reading rather than compared between them, so a rename shipping without its migration entry fails on the candidate while the released build, which predates the check, skips it. Generated-by: Claude Code (claude-opus-5)
The forward-roll job existed and already ran, but nothing woke it when the code that decodes durable state changed. The trigger now selects it from the decoders and, decisively, from the operation vocabulary they decode against: the SessionTodo cutover that caused #4420 changed `protocol/operations.ts` and no decoder, so a decoder-only trigger stays green on the exact change shape this guard exists to catch. The planner test pins that path. It runs on the heavy lane rather than the CLI packaging lane. Packaging is nine minutes that prove nothing this check needs; the baseline is instead the published predecessor, downloaded and integrity-checked against the registry's own digest, and read by the workspace already built on that lane. That costs about a minute of wall clock on roughly one commit in ten, against a full cross-platform matrix of runner time — and runner time is the scarcer resource here, which is why this lane is one job of serial steps to begin with. Generated-by: Claude Code (claude-opus-5)
01d119f to
eb7a3a1
Compare
|
Both findings were right. Rebased onto current Finding 2. Verified first: replaying the real 85-file change list of #4351 through the planner gave Finding 1. Your diagnosis holds on every point, including the sharpest one: fail-open decode plus a I did not restore fail-closed, because it prices in a cost the prescription does not name. This file sits at Both fail-closed and my fail-open were symptoms of one defect your comment made visible: the array was simultaneously the record and the authority. They are separate now. Against your specific objections:
No per-grant semantic probes were added. The assertion is one question asked of the reading build; the fixture does not know the registry exists. One limit, since my earlier description overreached: this catches a retired key with no entry, not a key that stays valid while its meaning widens. That stays a human declaration, as you said. Separately, found while verifying: the forward-roll baseline is whichever Nightly the registry currently tags, unfenced against the commit under test. The quiet failure is the bad one — if that Nightly already contains the migration under test, both sides speak the same vocabulary and it passes without crossing a version boundary. Pre-existing, exposure widened here, tracked in #4447. |
me2seeks
left a comment
There was a problem hiding this comment.
Approved. Separating the persisted grant record from the current effective authority preserves recoverability across operation vocabulary changes, while explicit replacements and releases keep migration intent auditable. The released-state qualification and planner coverage make this compatibility boundary materially easier to maintain.
M4n5ter
left a comment
There was a problem hiding this comment.
English
Re-review at eb7a3a15: the previous grant-record/effective-authority issue is resolved. One important issue remains.
[Important] Include the leaf operation-spec modules in the State Root compatibility trigger
ci-test-plan.mjs treats protocol/operations.ts as the owner of the operation vocabulary, but that file only aggregates the spec maps. The operation keys are declared across 40 leaf protocol modules, for example session-transcript.ts.
I verified the planner directly:
protocol/operations.ts stateRootCompat=true
protocol/session-transcript.ts stateRootCompat=false
protocol/session-turns.ts stateRootCompat=false
A normal rename or removal in a leaf module can therefore still skip the released State Root qualification and leave an omitted grant migration undetected. Please cover the modules that contribute to HOST_OPERATION_SPECS and add a planner regression test using a leaf protocol file.
Verification: build passed; planner/qualification tests 69/69; grant migration tests 11/11; Runtime Host suite 1550 passed, 12 skipped, 0 failed.
中文
在 eb7a3a15 上重新审查:之前 grant record 与 effective authority 的问题已经解决。目前还剩一个重要问题。
[Important] State Root 兼容性触发器需要覆盖实际声明 operation spec 的子模块
ci-test-plan.mjs 将 protocol/operations.ts 视为 operation vocabulary 的所有者,但该文件实际上只负责聚合;operation key 分散声明在 40 个协议子模块中,例如 session-transcript.ts。
我直接验证了 planner:
protocol/operations.ts stateRootCompat=true
protocol/session-transcript.ts stateRootCompat=false
protocol/session-turns.ts stateRootCompat=false
因此,直接在子模块中重命名或删除 operation key,仍可能跳过 released State Root qualification,使遗漏的 grant migration 无法被发现。请覆盖所有参与组成 HOST_OPERATION_SPECS 的模块,并使用一个协议子模块补充 planner 回归测试。
验证结果:构建通过;planner/qualification tests 69/69;grant migration tests 11/11;Runtime Host suite 1550 passed、12 skipped、0 failed。
Summary
A credential's grant list served two masters. As a record it should hold what some earlier release wrote; as an authority it may only name operations this build defines and this principal's policy allows. Decoding reconciled the two in place, so every vocabulary change had to be patched into the same array — three hand-written constants, then a fourth for the SessionTodo cutover that stranded workspaces holding a credential issued before it (#4420).
This separates them, and guards the seam that let it ship.
StoredAccessCredential.grants, kept as the file states it.effectiveOperationGrantsderives the authority on every decode and never writes back, so a grant this build cannot serve is absent from the authority and still present in the record. An unrelated later mutation no longer erases it — neither a newer build's key read by an older one, nor a key whose migration entry was forgotten.PERSISTED_GRANT_MIGRATIONSis the only thing that rewrites the record, and a replacement naming no successor fails to compile.protocol/operations.tsis what feat(runtime): cut over SessionTodo to Runtime Host authority #4351 changed; it touched no decoder, so a decoder-only trigger would have stayed green on the exact change this guard exists to catch.Deriving per principal also puts a Client Capability provider under its own policy instead of the remote owner's — decoding never applied it — and retires the Session Guest special case, which is now just another derivation rule.
Refs #4420
Where this check runs
It runs on the
heavylane, against the published predecessor downloaded and integrity-checked from the registry, read by the workspace that lane already built. The alternative was the CLI packaging lane, which supplies a built artifact but spends nine minutes of packaging that proves nothing this check needs.heavylane (this PR)Packaging costs nothing in wall clock only because it hides behind a slower lane — a margin nobody promised to keep. Runner time is the scarcer resource here, which is why this lane is one job of serial steps sharing a single checkout, install and build to begin with. Both fire on roughly 1 commit in 10.
Measured:
plan25s andheavy1150s on main; the qualification step 76s; the packaging lane's jobs 1863s in total.Verification
stateRootCompat = truewithfull = false— precise selection, not a full-suite fallback. It yieldedfalsebefore this changeoperations.tsfrom the trigger fails the new planner testbiome checkclean; ASF header check passesKnown limits
session.turns.querygaining a separate landmark query, say. No set comparison derives product semantics; that stays a human declaration in the registry.schemaVersionabove the known maximum stays fail-closed. Structure the build cannot interpret is categorically different from one vocabulary item it does not own, and relaxing it is a separate change.AI use
Tool(s) and scope: Claude Code (Opus 5) investigated the failure, wrote the change and its tests, and ran the verification above. OpenAI Codex and two other models were consulted independently on the design; the record/authority separation replaced an earlier fail-open decode after Codex identified that the earlier shape let a later write erase what decoding had dropped.
Checklist
Does this PR entail a change in behavior?