feat: deliver Phase 1 durable core - #121
Conversation
📝 WalkthroughWalkthroughAdds operation journaling and fenced recovery to the authority kernel, plus scripted runtime contracts for ledgers, artifacts, evidence, mediation, and registries. New fixtures and extensive tests cover validation, replay, witness handling, fault injection, restoration, and lifecycle invariants. ChangesAuthority kernel
Runtime contract fixtures
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)Operation recoverysequenceDiagram
participant RecoveryCaller
participant recoverFencedRun
participant ScriptedLedger
participant OperationJournal
RecoveryCaller->>recoverFencedRun: submit ledger history and generation binding
recoverFencedRun->>ScriptedLedger: verify chain and read back generation claim
ScriptedLedger-->>recoverFencedRun: committed claim or failure
recoverFencedRun->>OperationJournal: restore authorized operation records
OperationJournal-->>recoverFencedRun: reconstructed pending effects
Evidence admissionsequenceDiagram
participant EvidenceClient
participant EvidenceRuntime
participant ScriptedArtifactFixture
EvidenceClient->>EvidenceRuntime: prepare evidence request
EvidenceRuntime->>ScriptedArtifactFixture: verify artifact binding and witness state
ScriptedArtifactFixture-->>EvidenceRuntime: artifact fact
EvidenceClient->>EvidenceRuntime: admit prepared evidence
EvidenceRuntime-->>EvidenceClient: admission result and snapshot state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e63b15be3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (15)
packages/authority-kernel/src/operation.ts (2)
575-599: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
run-less branch inproofValueis tautological dead code.Every call site passes
state.subject.run/subject.run, so therun ?? raw.event.split('/event/')[0]fallback comparesraw.eventagainst a value derived from itself and can never fail. Consider makingrunrequired to keep the validator honest.♻️ Suggested tightening
-function proofValue(value: unknown, run?: string): OperationCommitProof | undefined { +function proofValue(value: unknown, run: string): OperationCommitProof | undefined { @@ - raw.event !== `${run ?? raw.event.split('/event/')[0]}/event/${raw.position + 1}` || - (run !== undefined && - (!raw.event.startsWith(`${run}/event/`) || !raw.transaction.startsWith(`${run}/txn/${raw.position + 1}/`))) + raw.event !== `${run}/event/${raw.position + 1}` || + !raw.transaction.startsWith(`${run}/txn/${raw.position + 1}/`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/authority-kernel/src/operation.ts` around lines 575 - 599, Make the run parameter required in proofValue and remove the run-less fallback logic. Validate raw.event and raw.transaction directly against the supplied run, eliminating the conditional run check while preserving the existing position, identity, and digest validation.
1211-1272: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDuplicate-observation check scans every operation's reconciliations on each call.
Line 1245-1247 is O(operations × reconciliations) per reconciliation. A
Set<string>of consumed observation IDs maintained alongsideoperationskeeps this O(1) and is trivially safe here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/authority-kernel/src/operation.ts` around lines 1211 - 1272, Replace the full scan of operations’ reconciliations in recordReconciliation with a Set<string> of consumed observation operation IDs maintained alongside operations. Check the Set for duplicates before appending, and add observation.value.operation to it only after the reconciliation record is successfully appended, preserving existing validation behavior.packages/authority-kernel/tests/operation-contract.test.mjs (1)
665-675: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis assertion passes for the wrong reason.
recordReconciliationaccepts exactlyoperation,observationOperation,proof; the extraordinal/outcome/observationDigestkeys makefields()reject the container outright, so the "cannot select certainty without a linked observation" path is never exercised. Drop the extra keys (and assert the code) so the test covers the intended invariant.💚 Suggested change
- assert.equal( - journal.recordReconciliation({ - operation: oracle.operation, - ordinal: 1, - observationOperation: oracle.observationOperation, - outcome: 'confirmed-absence', - observationDigest: digest('f'), - proof: proof(4), - }).ok, - false, - ); + assert.deepEqual( + journal.recordReconciliation({ + operation: oracle.operation, + observationOperation: oracle.observationOperation, + proof: proof(4), + }), + { ok: false, error: { family: 'FC-INPUT', code: 'INVALID_RECONCILIATION_RECORD' } }, + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/authority-kernel/tests/operation-contract.test.mjs` around lines 665 - 675, Update the recordReconciliation test case to pass only the accepted operation, observationOperation, and proof fields, removing ordinal, outcome, and observationDigest. Assert the returned failure code as well as ok being false, so the test exercises the missing linked-observation certainty invariant rather than container field validation.packages/authority-kernel/tests/recovery-contract.test.mjs (1)
6-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the runtime-contracts workspace dependency before switching this test to the package export
packages/authority-kernel/package.jsondoes not declare@agentic-workflow-kit/jig-runtime-contracts, so a workspace import would still be outside the package graph unless that dependency is added first. Add it as a devDependency, then import@agentic-workflow-kit/jig-runtime-contractsinstead of reaching into../../runtime-contracts/dist/index.js.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/authority-kernel/tests/recovery-contract.test.mjs` around lines 6 - 8, Add `@agentic-workflow-kit/jig-runtime-contracts` as a devDependency in packages/authority-kernel/package.json, then update the runtime import in the recovery-contract test to use the package export instead of the relative ../../runtime-contracts/dist/index.js path; leave the operation and recovery imports unchanged.packages/runtime-contracts/src/evidence.ts (1)
1104-1122: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winQuarantine/rejection outcomes are not idempotent under retry.
outcomeKeymixesstate.position + 1into the key, so re-preparing the identical hostile or oversize payload appends a fresh journal entry with a different key each time, while clean inputs are deduplicated bystate.intents.has(key)(Line 1258). A caller retrying after a lost response cannot reconcile to the earlier outcome and the journal grows per attempt. If this asymmetry is intentional for the fixture, a short comment recording it would help; otherwise key outcomes off the configuration binding plus content basis like intents do.Also applies to: 1167-1190
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/runtime-contracts/src/evidence.ts` around lines 1104 - 1122, Update outcomeKey and its quarantine/rejection call sites to derive a stable key from the configuration binding and outcome content basis, without including the changing journal position. Ensure retries of identical hostile or oversized payloads resolve to the existing journal outcome, matching the deduplication behavior guarded by state.intents.has(key).packages/runtime-contracts/tests/ledger-contract.test.mjs (1)
57-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
competingreadback outcome is never asserted.The local
competingbinding at Line 89 assertsFC-FENCE/STALE_GENERATION, because the second append promotesgen/2as the current generation andreadbackfences on generation before reaching the competing branch (src/ledger.tsLine 546). So thekind: 'competing'outcome — and the test's "fixed five outcomes" claim — has no coverage. Consider adding a same-generation, different-transaction readback to exercise it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/runtime-contracts/tests/ledger-contract.test.mjs` around lines 57 - 101, Extend the semantic ledger test around the existing competing readback to add a same-generation, different-transaction scenario that reaches the competing branch and asserts the expected kind: 'competing' outcome. Keep the existing FC-FENCE/STALE_GENERATION assertion for the promoted competing generation, and update the fixed-outcomes coverage only through the new readback case.packages/runtime-contracts/src/artifact.ts (1)
74-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo hand-rolled SHA-256 implementations; prefer
node:crypto. Both modules re-implement the full SHA-256 compression loop instead of using a standard digest, duplicating ~60 lines of bit manipulation whose correctness is only implicitly verified by the contract tests (which themselves usecreateHash('sha256')).
packages/runtime-contracts/src/artifact.ts#L74-L131: replacehashwith acreateHash('sha256')call (or a single shared helper module) and drop the constant table and compression loop.packages/runtime-contracts/src/evidence.ts#L257-L316: delete the duplicatedsha256and import the shared helper instead.If the fixtures must stay free of Node built-ins for portability reasons, extracting one internal
digest.tsused by both files still removes the duplication.As per coding guidelines, "Prefer well-known libraries/frameworks over 'rolling your own' for common tasks (cryptography, core data structures, standard algorithms)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/runtime-contracts/src/artifact.ts` around lines 74 - 131, Replace the hand-rolled hash implementation in packages/runtime-contracts/src/artifact.ts:74-131, including its constants and compression loop, with a standard SHA-256 digest using node:crypto or a shared internal helper. In packages/runtime-contracts/src/evidence.ts:257-316, remove the duplicated sha256 implementation and reuse that same helper; if Node built-ins are unsuitable, place the single shared implementation in an internal digest module.Source: Coding guidelines
packages/runtime-contracts/src/mediation.ts (3)
418-418: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the import to the top of the file.
The declaration is hoisted so this works, but a trailing import after 400 lines of code is surprising and most
import/first-style lint rules will flag it.capabilityDigest(Line 153) depends on it.♻️ Proposed relocation
@@ line 1 @@ +import { type CanonicalJson, stageDigest } from '`@agentic-workflow-kit/jig-codec`'; + const OPERATION_STATE_VERSION = 'jig.operation.v1';@@ line 418 @@ -import { type CanonicalJson, stageDigest } from '`@agentic-workflow-kit/jig-codec`';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/runtime-contracts/src/mediation.ts` at line 418, Move the CanonicalJson and stageDigest import to the file’s top-level import section, before all executable declarations. Keep capabilityDigest and all other consumers unchanged.
219-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImplicit catch-all route.
Any future operation prefix that isn't
OPC-ART-silently routes toPORT-ARTIFACT/CB-STORE. Making the last branch explicit (type.startsWith('OPC-ART-')) and failing withUNKNOWN_OPERATION_CLASSotherwise keeps the closed catalog honest as it grows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/runtime-contracts/src/mediation.ts` around lines 219 - 221, Update the operation classification logic near the existing OPC-DEL- branch to make the artifact route conditional on type.startsWith('OPC-ART-') rather than using an implicit fallback. For any unmatched operation prefix, return the established UNKNOWN_OPERATION_CLASS failure instead of routing to PORT-ARTIFACT/CB-STORE.
1-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated operation catalog can drift from the authority kernel.
OPERATION_STATE_VERSIONandOPERATION_TYPESare byte-identical copies ofpackages/authority-kernel/src/operation.ts:1-35. The test at Lines 48-51 only checks that every kernel type routes; a stale/extra entry here would pass. If package isolation is intentional, consider adding an equality assertion againstkernel.OPERATION_TYPESin the contract test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/runtime-contracts/src/mediation.ts` around lines 1 - 34, Add a contract-test assertion comparing the mediation module’s OPERATION_STATE_VERSION and OPERATION_TYPES with the authority kernel’s exported catalog, ensuring both collections match exactly in value and order. Keep the existing routing coverage test, and use the kernel symbols as the authoritative reference so stale or extra mediation entries fail.packages/runtime-contracts/tests/mediation-contract.test.mjs (2)
62-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead branch and redundant index lookup in the case loop.
capabilityKind === 'CB-DELIVERY'(Line 76) never matches —casescontainsCB-REVIEW-PUBLICATION, notCB-DELIVERY— soauthorityis alwaysnullhere. Andcases.findIndex(...)(Line 63) just recomputes the loop index.♻️ Proposed cleanup
- for (const [type, capabilityKind, port] of cases) { - const ordinal = cases.findIndex((entry) => entry[0] === type) + 1; + for (const [index, [type, capabilityKind, port]] of cases.entries()) { + const ordinal = index + 1;Then either drop the
authorityconditional in favour ofconst authority = null;, or add aCB-DELIVERYcase so the branch is actually reached.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/runtime-contracts/tests/mediation-contract.test.mjs` around lines 62 - 82, Clean up the case loop by replacing the unreachable capabilityKind === 'CB-DELIVERY' authority branch with the intended constant null value, and use the loop’s index instead of recomputing it via cases.findIndex. Preserve the existing transaction and operation ID generation for each case.
140-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
kernel.OPERATION_STATE_VERSIONinstead of the literal.Lines 84 and 307 read the version from the kernel; hardcoding
'jig.operation.v1'here (and at Line 227) hides drift between the kernel constant and mediation's local copy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/runtime-contracts/tests/mediation-contract.test.mjs` at line 140, Replace the hardcoded 'jig.operation.v1' value in the mediation contract test, including the occurrences near lines 140 and 227, with kernel.OPERATION_STATE_VERSION. Ensure all operation state version assertions in this test consistently use the kernel constant, matching the existing usage near lines 84 and 307.packages/runtime-contracts/tests/registry-contract.test.mjs (2)
358-373: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCoverage gap: no two simultaneously active waiters for the same story.
The alternate waiter reuses
fixture.story, but the first waiter is already consumed by then, so exactly one same-story waiter is ever active. Theleast-selection path ingrantresolves the winning record by story, which is only unambiguous under that assumption — see the comment onpackages/runtime-contracts/src/registry.tsLines 531-542. Adding a case with two active same-story waiters at different priorities would pin the intended behaviour.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/runtime-contracts/tests/registry-contract.test.mjs` around lines 358 - 373, The registry tests lack coverage for two simultaneously active waiters sharing the same story. Extend the relevant grant scenario around store.waiter and store.grant to create two same-story waiters with different priorities before granting, then assert the least-selection behavior resolves the intended waiter unambiguously according to the registry contract.
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnchecked result dereferencing makes regressions report as
TypeError.
createRegistryBinding(...).valueat Line 14 and the manyx.value.contentDigest/y.error.codereads (Lines 79, 92, 111, 161, 189, 300, 313, 321, 352, ...) assume the preceding operation's outcome. When a contract regresses, the suite fails with "Cannot read properties of undefined" instead of pointing at the broken step. A tinyokValue(result)/errorOf(result)helper that asserts before returning would keep failures diagnosable.Also applies to: 70-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/runtime-contracts/tests/registry-contract.test.mjs` at line 14, Update registry-contract.test.mjs to add assertion helpers such as okValue and errorOf that validate each operation result before returning its value or error. Use these helpers for canonicalBinding and the existing value.contentDigest and error.code dereferences throughout the tests, so regressions identify the failed operation instead of throwing from undefined properties.packages/runtime-contracts/src/registry.ts (1)
488-502: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
withdrawalandreleasesilently drop thefaultinput.
waiter,grant, andatomicRebindforwarddata(input, 'fault')toappend;withdrawal(Line 500) andrelease(Line 589) do not, so the ack-loss fault surface can't be exercised for those two variants. Either forward the fault or rejectfaultexplicitly for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/runtime-contracts/src/registry.ts` around lines 488 - 502, The withdrawal method currently omits the fault value when calling append; update its append invocation to forward data(input, 'fault'), matching waiter, grant, and atomicRebind. Apply the same change to the release method’s append invocation so both variants preserve the ack-loss fault surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/runtime-contracts/src/mediation.ts`:
- Around line 301-325: Harden the permit validation in the dispatch/lookup flow
before dereferencing fields in operationRoute and the validation predicate.
Validate that permitted.value is a non-null object with the required capability
and proof objects (and any other fields needed by the checks), returning
fail('FC-AUTHORITY', 'INVALID_DISPATCH_PERMIT') for malformed shapes so
caller-injected journal results always fail closed rather than throwing.
In `@packages/runtime-contracts/src/registry.ts`:
- Around line 531-542: The least-eligible waiter lookup in grant must preserve
each waiter’s record while sorting with the full compare comparator, then
validate least.record.contentDigest against the selected handle’s digest instead
of re-finding by story. Add the same-story ambiguous-priority/ordinal coverage
in packages/runtime-contracts/tests/registry-contract.test.mjs lines 358-373 to
verify the expected selection order; the test site requires a direct change.
- Around line 663-670: Update injectFault to derive and validate a normalized
fault string once, then store that normalized string in faults rather than the
raw fault input. Preserve the existing accepted-fault membership check and type
constraints so faultCode receives a valid string and returns its declared fault
code.
In `@packages/runtime-contracts/tests/artifact-contract.test.mjs`:
- Around line 323-330: Update the lookup objects passed to
restoreScriptedArtifactFixture in the affected tests to derive from the complete
lookup fixture, preserving protectedPosition and protectedHead while overriding
only position or headDigest for each mismatch case. Ensure the assertions
exercise head/position comparison rather than failing the exact-key shape
validation.
In `@packages/runtime-contracts/tests/ledger-contract.test.mjs`:
- Around line 410-435: Update the negative assertions in the semantic ledger
test to construct bare proposal-shaped inputs without the derived version and
contentDigest fields before testing position -1 and noncanonical content. Keep
the existing prepared proposal for append and other checks, but ensure
createLedgerRecord reaches the FC-INPUT validation for the invalid position and
NFC-noncanonical content specifically.
In `@packages/runtime-contracts/tests/mediation-contract.test.mjs`:
- Around line 205-221: The attestation mutation cases in the dispatch test reuse
the fixture after the successful dispatch, so they hit duplicate detection
before attestation validation. Create a fresh fixture for each mutation (or
otherwise reset dispatch state) while preserving the original fixture’s
invocation assertion, ensuring every changed attestation reaches
validateAttestation and is rejected for its mutation.
---
Nitpick comments:
In `@packages/authority-kernel/src/operation.ts`:
- Around line 575-599: Make the run parameter required in proofValue and remove
the run-less fallback logic. Validate raw.event and raw.transaction directly
against the supplied run, eliminating the conditional run check while preserving
the existing position, identity, and digest validation.
- Around line 1211-1272: Replace the full scan of operations’ reconciliations in
recordReconciliation with a Set<string> of consumed observation operation IDs
maintained alongside operations. Check the Set for duplicates before appending,
and add observation.value.operation to it only after the reconciliation record
is successfully appended, preserving existing validation behavior.
In `@packages/authority-kernel/tests/operation-contract.test.mjs`:
- Around line 665-675: Update the recordReconciliation test case to pass only
the accepted operation, observationOperation, and proof fields, removing
ordinal, outcome, and observationDigest. Assert the returned failure code as
well as ok being false, so the test exercises the missing linked-observation
certainty invariant rather than container field validation.
In `@packages/authority-kernel/tests/recovery-contract.test.mjs`:
- Around line 6-8: Add `@agentic-workflow-kit/jig-runtime-contracts` as a
devDependency in packages/authority-kernel/package.json, then update the runtime
import in the recovery-contract test to use the package export instead of the
relative ../../runtime-contracts/dist/index.js path; leave the operation and
recovery imports unchanged.
In `@packages/runtime-contracts/src/artifact.ts`:
- Around line 74-131: Replace the hand-rolled hash implementation in
packages/runtime-contracts/src/artifact.ts:74-131, including its constants and
compression loop, with a standard SHA-256 digest using node:crypto or a shared
internal helper. In packages/runtime-contracts/src/evidence.ts:257-316, remove
the duplicated sha256 implementation and reuse that same helper; if Node
built-ins are unsuitable, place the single shared implementation in an internal
digest module.
In `@packages/runtime-contracts/src/evidence.ts`:
- Around line 1104-1122: Update outcomeKey and its quarantine/rejection call
sites to derive a stable key from the configuration binding and outcome content
basis, without including the changing journal position. Ensure retries of
identical hostile or oversized payloads resolve to the existing journal outcome,
matching the deduplication behavior guarded by state.intents.has(key).
In `@packages/runtime-contracts/src/mediation.ts`:
- Line 418: Move the CanonicalJson and stageDigest import to the file’s
top-level import section, before all executable declarations. Keep
capabilityDigest and all other consumers unchanged.
- Around line 219-221: Update the operation classification logic near the
existing OPC-DEL- branch to make the artifact route conditional on
type.startsWith('OPC-ART-') rather than using an implicit fallback. For any
unmatched operation prefix, return the established UNKNOWN_OPERATION_CLASS
failure instead of routing to PORT-ARTIFACT/CB-STORE.
- Around line 1-34: Add a contract-test assertion comparing the mediation
module’s OPERATION_STATE_VERSION and OPERATION_TYPES with the authority kernel’s
exported catalog, ensuring both collections match exactly in value and order.
Keep the existing routing coverage test, and use the kernel symbols as the
authoritative reference so stale or extra mediation entries fail.
In `@packages/runtime-contracts/src/registry.ts`:
- Around line 488-502: The withdrawal method currently omits the fault value
when calling append; update its append invocation to forward data(input,
'fault'), matching waiter, grant, and atomicRebind. Apply the same change to the
release method’s append invocation so both variants preserve the ack-loss fault
surface.
In `@packages/runtime-contracts/tests/ledger-contract.test.mjs`:
- Around line 57-101: Extend the semantic ledger test around the existing
competing readback to add a same-generation, different-transaction scenario that
reaches the competing branch and asserts the expected kind: 'competing' outcome.
Keep the existing FC-FENCE/STALE_GENERATION assertion for the promoted competing
generation, and update the fixed-outcomes coverage only through the new readback
case.
In `@packages/runtime-contracts/tests/mediation-contract.test.mjs`:
- Around line 62-82: Clean up the case loop by replacing the unreachable
capabilityKind === 'CB-DELIVERY' authority branch with the intended constant
null value, and use the loop’s index instead of recomputing it via
cases.findIndex. Preserve the existing transaction and operation ID generation
for each case.
- Line 140: Replace the hardcoded 'jig.operation.v1' value in the mediation
contract test, including the occurrences near lines 140 and 227, with
kernel.OPERATION_STATE_VERSION. Ensure all operation state version assertions in
this test consistently use the kernel constant, matching the existing usage near
lines 84 and 307.
In `@packages/runtime-contracts/tests/registry-contract.test.mjs`:
- Around line 358-373: The registry tests lack coverage for two simultaneously
active waiters sharing the same story. Extend the relevant grant scenario around
store.waiter and store.grant to create two same-story waiters with different
priorities before granting, then assert the least-selection behavior resolves
the intended waiter unambiguously according to the registry contract.
- Line 14: Update registry-contract.test.mjs to add assertion helpers such as
okValue and errorOf that validate each operation result before returning its
value or error. Use these helpers for canonicalBinding and the existing
value.contentDigest and error.code dereferences throughout the tests, so
regressions identify the failed operation instead of throwing from undefined
properties.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 239e5ad5-d307-4e3f-8338-6d2a9b3cbb16
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
packages/authority-kernel/src/index.tspackages/authority-kernel/src/operation.tspackages/authority-kernel/src/recovery.tspackages/authority-kernel/tests/fixtures/operation-contract-oracle.jsonpackages/authority-kernel/tests/fixtures/operation-crash-corpus.jsonpackages/authority-kernel/tests/fixtures/recovery-contract-oracle.jsonpackages/authority-kernel/tests/operation-contract.test.mjspackages/authority-kernel/tests/recovery-contract.test.mjspackages/runtime-contracts/package.jsonpackages/runtime-contracts/src/artifact.tspackages/runtime-contracts/src/evidence.tspackages/runtime-contracts/src/index.tspackages/runtime-contracts/src/ledger.tspackages/runtime-contracts/src/mediation.tspackages/runtime-contracts/src/registry.tspackages/runtime-contracts/tests/artifact-contract.test.mjspackages/runtime-contracts/tests/evidence-contract.test.mjspackages/runtime-contracts/tests/fixtures/artifact-contract-oracle.jsonpackages/runtime-contracts/tests/fixtures/evidence-contract-oracle.jsonpackages/runtime-contracts/tests/fixtures/ledger-contract-oracle.jsonpackages/runtime-contracts/tests/fixtures/mediation-contract-oracle.jsonpackages/runtime-contracts/tests/fixtures/registry-contract-oracle.jsonpackages/runtime-contracts/tests/ledger-contract.test.mjspackages/runtime-contracts/tests/mediation-contract.test.mjspackages/runtime-contracts/tests/registry-contract.test.mjs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/authority-kernel/src/recovery.ts (1)
497-550: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftSnapshot resume doesn't actually save replay work —
projectionCanonicalalready did the full replay unconditionally.
projectionCanonical(Line 500) performs a full genesis-to-verifiedPositionreplay throughreduceAuthoritybefore therequestedSnapshotblock even checks whether a snapshot exists. When a valid snapshot is supplied, this block then does an additional prefix replay (covered, genesis torequestedSnapshot.position) plus a suffix replay (resumed), so the prefix range is replayed twice and the whole point of "reusing covered snapshots" (avoiding re-executing business logic for already-verified history) is not realized — snapshot resume currently adds work rather than saving it.To make the optimization real, the full unconditional replay would need to be deferred/skipped whenever a snapshot successfully verifies and resumes (while preserving the
before-replay/after-replayfault-injection semantics and theMISSING_TRANSITIONcheck, which currently rely on this eager computation).♻️ Illustrative restructuring sketch (not a drop-in diff — crash-point ordering needs care)
if (candidate.crashAt === 'before-replay') return failure('FC-TRUST', 'RECOVERY_REQUIRED'); let effectiveProjection: RecoveryResult<CanonicalJson>['value'] | undefined; let snapshotStatus: RecoveryObservation['snapshot'] = 'absent'; if (requestedSnapshot) { // ...verify + resume from prefix as today, WITHOUT first computing a full projectionCanonical... } if (!effectiveProjection) { const projectionCanonical = replayProjection( candidate.initialState as AuthorityState, ordered, generationControlPositions, verifiedPosition, verifiedDigest, ); if (!projectionCanonical.ok) return projectionCanonical; effectiveProjection = projectionCanonical.value; } if (candidate.crashAt === 'after-replay') return failure('FC-TRUST', 'RECOVERY_REQUIRED');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/authority-kernel/src/recovery.ts` around lines 497 - 550, Defer the full replay in the recovery flow until snapshot verification has failed or no snapshot was supplied. Restructure the logic around projectionCanonical and the requestedSnapshot resume path so a valid snapshot uses only its covered-prefix and suffix replay, while preserving before-replay and after-replay crash semantics and performing the MISSING_TRANSITION check on the selected effective projection.packages/runtime-contracts/tests/registry-contract.test.mjs (1)
129-159: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest only covers the success path; add the negative case that pinned the original bug.
This test correctly sets up the ambiguous scenario (two active waiters sharing a story) that caused the original defect, but it only asserts that granting to the correct least-eligible waiter (
earlier) succeeds. It never asserts that attempting to grant to the non-least waiter (later) is rejected withNOT_LEAST_ELIGIBLE_WAITER— which is the actual regression the original bug risked (a non-least waiter incorrectly matched viastorycould be granted authority). Without that negative assertion, a regression back to story-based matching (which would coincidentally still letearliersucceed in some orderings) would not necessarily be caught.♻️ Suggested addition
const granted = store.grant({ binding, expectedPosition: 1, expectedDigest: earlier.value.contentDigest, waiter: earlier.value.handle, eligibilityBasis: fixture.digests.basisB, }); assert.equal(granted.ok, true); assert.equal(granted.value.content.waiter.contentDigest, earlier.value.contentDigest); + + // Pin the regression: granting the non-least waiter for the same story must be rejected. + const rejectedGrant = store.grant({ + binding, + expectedPosition: 1, + expectedDigest: earlier.value.contentDigest, + waiter: later.value.handle, + eligibilityBasis: fixture.digests.basisA, + }); + assert.deepEqual(rejectedGrant, { + ok: false, + error: { family: 'FC-AUTHORITY', code: 'NOT_LEAST_ELIGIBLE_WAITER' }, + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/runtime-contracts/tests/registry-contract.test.mjs` around lines 129 - 159, Extend the test for least-eligible selection with a negative grant attempt using the non-least waiter `later` before or alongside the successful `earlier` grant. Assert that the result is rejected and reports `NOT_LEAST_ELIGIBLE_WAITER`, while preserving the existing successful grant assertion for `earlier`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/authority-kernel/src/recovery.ts`:
- Around line 497-550: Defer the full replay in the recovery flow until snapshot
verification has failed or no snapshot was supplied. Restructure the logic
around projectionCanonical and the requestedSnapshot resume path so a valid
snapshot uses only its covered-prefix and suffix replay, while preserving
before-replay and after-replay crash semantics and performing the
MISSING_TRANSITION check on the selected effective projection.
In `@packages/runtime-contracts/tests/registry-contract.test.mjs`:
- Around line 129-159: Extend the test for least-eligible selection with a
negative grant attempt using the non-least waiter `later` before or alongside
the successful `earlier` grant. Assert that the result is rejected and reports
`NOT_LEAST_ELIGIBLE_WAITER`, while preserving the existing successful grant
assertion for `earlier`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d397a46-7ab2-48c5-ae1e-edbb6e76cb6d
📒 Files selected for processing (13)
packages/authority-kernel/src/recovery.tspackages/authority-kernel/tests/fixtures/recovery-contract-oracle.jsonpackages/authority-kernel/tests/recovery-contract.test.mjspackages/runtime-contracts/src/artifact.tspackages/runtime-contracts/src/mediation.tspackages/runtime-contracts/src/registry.tspackages/runtime-contracts/tests/artifact-contract.test.mjspackages/runtime-contracts/tests/ledger-contract.test.mjspackages/runtime-contracts/tests/mediation-contract.test.mjspackages/runtime-contracts/tests/registry-contract.test.mjstools/repo-guard/bin/check-active-repository.mjstools/repo-guard/tests/check-active-repository.test.mjsturbo.json
1 participant
Summary
Verification
CI=true pnpm check— 18/18 tasks passedpnpm delivery:check— 48 stories and 7 phases validgit diff --check 6021352bbebc5d1e7a44514381795114199cab26..HEAD— passedCandidate
9e63b15be35aa7130827eb1a43d5417d498a29c516ce06549744e3a61130e0fd1c4cee1b223122746021352bbebc5d1e7a44514381795114199cab26Hosted CI and PR review state are the remaining phase gates. This PR does not authorize merge or cleanup.
Summary by CodeRabbit