fix: testnet-canary test failures (parity, storage scheduler, update fail-closed) + triage - #1744
fix: testnet-canary test failures (parity, storage scheduler, update fail-closed) + triage#1744branarakic wants to merge 4 commits into
Conversation
mock-adapter-parity.test.ts [CH-8] enforces that MockChainAdapter mirror every EVMChainAdapter public method minus documented exemptions. Canary commit c4fd81e added verifyContractSignature (an EIP-1271 isValidSignature on-chain view call) to EVMChainAdapter, turning the audit red with missing = ['verifyContractSignature']. Resolved via the exemption route the test's own comment sanctions ('Either add it to the mock or put it in MOCK_EXEMPT_FROM_EVM with a comment'), rather than a mock impl: the method's only production caller (dkg-publisher.ts contract-author update authorization) is gated by hasContractCode(author), which the mock hardcodes false, so the branch is structurally unreachable on the mock. A true shim would be a false-accept on a security path; a false shim would be dead code; a faithful impl would have to model deployed 1271 bytecode. The two tests exercising the contract-author branch stub both methods per-test, so no mock-based flow needs a native impl. Same family as the on-chain-derived helpers already exempted. Verified: parity 15/15 (was red with exactly ['verifyContractSignature']); teeth intact — any other unmirrored EVM method still reddens the audit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tests Two GraphSetIndexStore tests construct a StorePriorityScheduler with a contrived maxConcurrent:2 pool. Canary commit 4057c25 added a health lane (DEFAULT_HEALTH_RESERVED_SLOTS=1); with the tests not setting healthReservedSlots, the health lane now reserves the 2nd of 2 slots and the normalReservedSlots:1 floor is clamped to 0, so the normal read can no longer bypass the in-flight background seed — turning both tests red ('lets a normal seed bypass an in-flight background seed', 'ignores an older failed background revalidation'). This is a stale test config, not a code regression: production uses DEFAULT_MAX_CONCURRENT=4, so after ack(1)+health(1) the ordinary pool is still 2 and normal+background run concurrently — the bypass invariant the tests guard still holds in prod. Pinning healthReservedSlots:0 restores the 2-slot ordinary pool the tests were designed around. No assertion is touched; every behavioral check (listGraphsCalls, options ordering, inflight snapshot counts, next-scan timing) is unchanged. Verified: 37/37 green; teeth intact — flipping healthReservedSlots back to 1 re-reds exactly these two tests; full storage suite 437 passing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tent KA
The rootless cutover moved the update authorization check
(assertValidPrecomputedUpdateAttestation) ahead of staging, where it reads
chain-truth via getKnowledgeAssetOwner -> ownerOf BEFORE the chain-submit
try/catch. For a non-existent or expired KA, ownerOf reverts
(ERC721NonexistentToken / KnowledgeAssetExpired), and update() propagated
that revert as a throw — breaking the fail-closed contract that update()
returns {status:'failed'} and mutates nothing when there is no updatable KA
on chain.
Wrap the pre-staging authorization in a try/catch that converts ONLY that
definitive 'no updatable KA' revert class to a failed result (mirroring the
existing V10_DEFINITIVE_ERRORS handling on the submit path). Every
authorization failure still throws: wrong owner of an existing KA
(KA_UPDATE_AUTHOR_NOT_OWNER), signer mismatch, and missing-adapter-method
config errors are re-raised unchanged, preserving reject-unauthorized-
before-staging.
Verified against real Hardhat: ka-update 20/20, security-regressions 26/26
(both were red on the non-existent-KA update). Teeth: mutating the
classifier to re-raise the nonexistent case re-reds the test, so the fix is
load-bearing. publisher-evm-e2e 'updating non-existent KC returns failed'
exercises the same path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| kcMerkleRoot, | ||
| updateSeal, | ||
| ); | ||
| } catch (attestErr) { |
There was a problem hiding this comment.
🟡 Issue: Do not add a second inline V10 update-failure classifier inside the publisher flow
What's wrong
The change fixes a narrow placement problem by embedding EVM revert decoding, a hard-coded no-updatable-KA list, phase cleanup, logging, and a failed-result constructor directly into an already sprawling orchestration method. That makes the update failure model harder to reason about because the same concept is now split between this new preflight catch and the existing V10 submit catch below. The inline cast to a provider-specific { revert?: { name?: string } } shape is another sign that the boundary is not explicit enough.
Example
Today KnowledgeAssetExpired appears in both the new pre-staging list and the later V10_DEFINITIVE_ERRORS list, while the failed PublishResult object is also assembled in both places. A future definitive update rejection now has to preserve two classifiers and two early-return shapes in a nearly 9k-line publisher file.
Suggested direction
Centralize this as an update rejection classification/result helper instead of bolting a new try/catch taxonomy into the middle of update(). The code-judo move is to make both the preflight owner lookup and the chain submission path feed the same classifyV10UpdateRejection / buildFailedUpdateResult boundary, so the huge method does not gain another bespoke branch with duplicated error-name lists, phase endings, logging, and return-object assembly.
Confidence note
I verified the same method already has a later V10 definitive-error classifier and failed-result construction path; I did not run tests because this review is diff-focused and the workspace is read-only.
For Agents
In packages/publisher/src/dkg-publisher.ts, keep the behavior that non-existent/expired KA preflight failures return status: 'failed' while authorization/config failures still throw. Move the revert-name extraction and failed-update result construction behind a small shared update failure helper, or have the attestation preflight return/throw a typed publisher-level classification that the existing V10 failure path can consume. Add or keep focused coverage proving expired/non-existent preflight returns failed and unauthorized attestation still rejects before staging.
There was a problem hiding this comment.
Done in 2368740. Centralized the taxonomy so both paths feed one boundary:
extractV10UpdateRejectionName(err)— the shared revert-name decode (was duplicated: bareenrichEvmErrorin the submit catch,enrichEvmError ?? revert.namein the preflight).buildFailedUpdateResult()— one closure now used by all three failed-result sites (preflight catch, submitearlyReturn,!txResult.success).- Named module constants
PRE_STAGING_NO_UPDATABLE_KA_ERRORSandV10_DEFINITIVE_UPDATE_ERRORS.
The two error sets stay deliberately distinct rather than merged: pre-staging is a strict subset (only "no updatable KA") because an authorization failure — wrong owner (KA_UPDATE_AUTHOR_NOT_OWNER), signer mismatch — must still throw before staging, whereas at submit those same classes convert to a failed result. That difference is now documented on the constants. No behavior change: ka-update 20/20 + security-regressions 26/26 unchanged.
There was a problem hiding this comment.
🟡 Issue: Move EVM rejection-name decoding out of the publisher
What's wrong
This adds low-level EVM/RPC error-shape knowledge and raw contract error taxonomy directly to an already large publisher orchestrator. That splits the error-decoding boundary across packages and makes future provider-shape or contract-error changes require edits in dkg-publisher.ts instead of the canonical chain adapter error module.
Example
revertError('KnowledgeAssetExpired') in the new test is classified only because dkg-publisher.ts now knows about { revert: { name } }; enrichEvmError() itself still returns null for that same shape, so the next caller that needs a revert name has to copy this publisher-local cast or remember this special helper exists.
Suggested direction
Make the chain package the single place that understands provider/ethers error shapes. The publisher can still own the decision to return status: 'failed', but it should not parse low-level EVM error objects or carry ad-hoc casts for one update path.
For Agents
Look in packages/chain/src/evm-adapter-errors.ts and packages/publisher/src/dkg-publisher.ts. Preserve the current failed-result behavior, but move generic EVM revert-name extraction to the chain error boundary, or expose a small getEvmRevertName(err)/update-rejection classifier. Cover both raw revert data and ethers revert.name shapes in chain-level tests, then have the publisher consume the semantic helper.
There was a problem hiding this comment.
🟡 Issue: Avoid adding another hand-balanced early return in update()
What's wrong
The new pre-staging catch handles a local policy decision and phase finalization inline inside a very busy orchestration method. That keeps growing the same spaghetti pattern: each new failure mode brings its own cleanup and return branch instead of flowing through one obvious exit path.
Example
A future maintainer adding one more chain-level cleanup, metric, or phase invariant after chain:submit starts now has to update every early-exit branch: the new pre-staging failure branch, the definitive-submit branch, the generic failed-tx branch, and the success path. Missing one would leave the orchestration inconsistent even though each local branch reads reasonably.
Suggested direction
Restructure the chain section so phase cleanup happens once instead of being copied into every rejection path. A small outcome object or helper would remove the need for another inline onPhase(..., 'end') pair and make later changes less brittle.
Confidence note
The surrounding function already had multiple phase-closing exits; this change materially worsens that pattern by adding another one in the new pre-staging rejection branch.
For Agents
Focus on DKGPublisher.update() around the chain phase. Preserve the emitted phases and returned PublishResult shapes, but collapse the chain phase into a single finalization point: for example, return a discriminated submit outcome from the preflight/submit block and close chain:submit/chain in one finally or finishChainResult helper. Tests should prove the existing phase sequence for failed pre-staging and definitive submit rejections remains unchanged.
| // reject-unauthorized-before-staging semantics. | ||
| const errorName = enrichEvmError(attestErr) | ||
| ?? (attestErr as { revert?: { name?: string } })?.revert?.name; | ||
| const NO_UPDATABLE_KA = ['ERC721NonexistentToken', 'KnowledgeAssetExpired']; |
There was a problem hiding this comment.
🟡 Issue: The new expired-KA failure path is not covered
What's wrong
This change explicitly adds KnowledgeAssetExpired to the pre-staging attestation failure conversion, but the visible tests only cover a bogus/non-existent KA ID. If the expired-existing-KA branch regresses, the suite can still stay green while update() throws instead of returning the documented failed result.
Example
A regression test could publish a KA with a short lifetime, advance Hardhat until it expires, build a valid precomputedUpdateAttestation for that same kaId, then call publisher.update(...). Expected: the promise resolves with status: 'failed' and does not mutate local store; it should not reject from the pre-staging attestation check.
Suggested direction
Add a focused EVM integration or adapter-stubbed publisher test for an expired existing KA, separate from the already-covered non-existent-ID case.
Confidence note
I found existing coverage for non-existent KA updates, but no publisher-level expired-KA update case in the searched tests.
For Agents
Look near the EVM publisher update tests, especially publisher-evm-e2e.test.ts or security-regressions.test.ts. Add an expired-existing-KA update regression that reaches assertValidPrecomputedUpdateAttestation, preserves the non-mutation guarantee, and proves KnowledgeAssetExpired maps to a failed result before staging.
There was a problem hiding this comment.
Good catch — investigating it surfaced that KnowledgeAssetExpired can't reach the pre-staging check at all, so I removed it from that list (it was dead) and added real coverage for where it does fire.
The pre-staging check calls getKnowledgeAssetOwner → DKGKnowledgeAssets.ownerOf (the storage contract, standard ERC-721: reverts ERC721NonexistentToken for a missing token, no expiry logic). KnowledgeAssetExpired is thrown only by KnowledgeAssetsLifecycle.sol (lines 946, 1546) at update submit — a different contract. So an expired KA passes pre-staging and fails at submit, handled by the existing V10_DEFINITIVE_UPDATE_ERRORS path.
2368740 therefore: (1) drops the dead pre-staging KnowledgeAssetExpired; (2) adds ka-update-submit-failure.test.ts — a mock-based unit test (no Hardhat/epoch time-travel) that drives update() past pre-staging and rejects the broadcast, proving KnowledgeAssetExpired and CannotUpdateImmutableKnowledgeAsset map to {status:'failed'} with no store mutation, and that a non-definitive rejection still throws (teeth). This also covers the submit earlyReturn branch, which the Hardhat ka-update tests didn't reach (they either succeed or fail at pre-staging). (3) Fixes the now-stale // V10 catches KnowledgeAssetExpired comment in security-regressions.test.ts — that non-existent-KA path is ERC721NonexistentToken at pre-staging.
There was a problem hiding this comment.
🟡 Issue: The new pre-staging nonexistent-KA mapping is not covered
What's wrong
The PR changes update() so a chain owner lookup revert for a missing KA returns a failed PublishResult instead of throwing. The added tests cover submit-time reverts after the owner check succeeds, but they do not exercise the new pre-staging catch path. That leaves the main behavior described in this block without a regression test.
Example
A focused regression test could pass a valid precomputedUpdateAttestation, have getKnowledgeAssetOwner() throw an error shaped as { revert: { name: 'ERC721NonexistentToken' } }, and assert publisher.update() resolves with status: 'failed' and leaves the store unchanged. Without this PR's catch, that test would reject instead.
Suggested direction
Add a regression test for the assertValidPrecomputedUpdateAttestation catch path itself, separate from the submit-time rejection tests.
For Agents
Add coverage around DKGPublisher.update() before submit: use a mock chain where signature prerequisites pass, getKnowledgeAssetOwner throws ERC721NonexistentToken, and updateKnowledgeCollectionV10 is not called. Assert failed result shape, no local mutation, and keep an unauthorized-existing-owner case throwing if practical.
…r submit path Addresses PR #1744 review on the update() fail-closed fix: 1. Centralize the update-rejection taxonomy (review: two inline classifiers + two failed-result constructors in a 9k-line method). Extract extractV10UpdateRejectionName() and named PRE_STAGING_NO_UPDATABLE_KA_ERRORS / V10_DEFINITIVE_UPDATE_ERRORS constants, and a single buildFailedUpdateResult() that both the pre-staging owner check and the chain-submit path feed. No behavior change; the two error sets stay intentionally distinct (pre-staging is a strict subset — auth failures must still throw before staging). 2. Drop KnowledgeAssetExpired from the pre-staging set: it is thrown only by KnowledgeAssetsLifecycle at update SUBMIT, never by the storage contract's ownerOf (which the pre-staging check calls), so it was dead code there. 3. Cover the previously-untested submit-definitive path: new mock-based ka-update-submit-failure.test.ts drives update() past pre-staging and rejects the broadcast, proving KnowledgeAssetExpired / CannotUpdateImmutable map to {status:'failed'} with no store mutation, and a non-definitive rejection still throws (teeth). No Hardhat/epoch time-travel needed. Also corrects the stale 'V10 catches KnowledgeAssetExpired' comment in security-regressions.test.ts (that non-existent-KA path is now ERC721NonexistentToken at pre-staging). Verified: new test 3/3; publisher unit suite 417; ka-update 20/20 + security-regressions 26/26 unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Closing — this PR's entire content landed on
Latest main push CI is fully green (43 jobs). The triage notes in this PR's description remain accurate for the record — notably, the discard-after-promote contradiction was resolved in favor of the deliberate post-promote guard (the conflicting agent e2e flows were migrated in |
2 participants
Summary
testnet-canaryhas 46 failing tests across 19 files — the first time the full Node/EVM lanes ran over the rootless-KA cutover delta (they only run advisory checks on canary-targeted PRs, so these accumulated undetected). This PR lands the three clusters that are code-vs-test-decided, fixed, and locally verified, and documents the full triage of the rest.Every failure was diagnosed against
docs/rfcs/ka-graph-content-scope.mdwith the rule: a test is stale only if it asserts a behavior the RFC overturned (root-membership metadata, per-root manifests, legacy-KA mutation, partial-subset state); it's a code regression if it asserts a behavior the RFC preserves (owner-only access, fail-closed update, discard idempotency, exact-graph replacement). Each verdict got an independent adversarial second pass — which changed or blocked several fixes (see "Deliberately not fixed here").Fixed and verified in this PR
verifyContractSignaturemock exemptionDetails:
test(chain): exempt verifyContractSignature— canary added the EIP-1271 methodverifyContractSignaturetoEVMChainAdapter;MockChainAdapterlacks it and the CH-8 parity audit went red. It's a real-chain-only on-chain view call, structurally unreachable on the mock (hasContractCodereturns false), and the two tests that exercise the contract-author branch stub both methods per-test — so the parity audit's own sanctioned exemption route is correct (a mock impl would be misleading dead code).test(storage): pin pre-health-lane pool— canary's health lane (DEFAULT_HEALTH_RESERVED_SLOTS=1, commit 4057c25) steals the 2nd of 2 slots in two tests' contrivedmaxConcurrent:2scheduler, clamping the normal lane so it can't bypass a background seed. Production (maxConcurrent=4) keeps a 2-slot ordinary pool, so the bypass invariant still holds; only the test config was stale. No assertion touched.fix(publisher): return failed status when update() targets a non-existent KA— a real regression. The cutover moved the update authorization check ahead of staging, wheregetKnowledgeAssetOwner→ownerOfreverts for a non-existent/expired KA before the chain-submit try/catch, soupdate()threw instead of honoring the fail-closed contract ({status:'failed'}, no local mutation). Now only that definitive "no updatable KA" class becomes a failed result; every authorization failure (wrong owner →KA_UPDATE_AUTHOR_NOT_OWNER, signer mismatch) still throws.Deliberately NOT fixed here (need the design owner or the e2e harness)
The adversarial verify pass blocked two "fixes" that would have been wrong, and flagged others that need the CI e2e harness (2-node libp2p + hardhat, which is heavy/flaky locally and even throws
NoEligibleContextGraph):rootless-graph-lifecycle-e2e, ~4 tests) — the initial diagnosis called the WM-mutable guard inassertionDiscardUnlockedan accidental paste, but the verify found commit 148c306 also shipped a deliberate test (draft-lifecycle.test.ts:2483— "full lifecycle … rejects post-promote discard") asserting the exact opposite. So the codebase contains a genuine contradiction between the new guard and these e2e tests — a design-intent decision for the rootless owner, not a mechanical fix.assertion.write(publish-jsonld-envelope, 1 test) — the verify showed bare (non-envelope) JSON-LD → private/no-anchor is the deliberate rootless contract (a passing sibling test confirms it), so the proposeddefaultVisibility:'public'code edit would mutate product code to satisfy a stale test. That test should be updated, not the code.draft-lifecycle, 6 tests) — adversarially confirmed a real promote-recovery regression (commit 1122376 broke the durable share-operation-id / WM-empty recovery preconditions inassertionPromoteUnlocked). Fix is understood but intricate; wanted a focused follow-up PR with hardhat verification rather than bundling.e2e-publish-protocol, 4 tests) — a real cross-node regression: commit ee689fd rewrote the SWM-gossip receiver to requirecontentScopeVersion===2but the sender still emits the legacy shape, so peers receive nothing. Needs the 2-node libp2p harness to verify.access-protocol+e2e-security, 9 tests) — stale test setup: canary's trust-boundary gate (commit 6eff621) now requires a CG root-registration marker the raw-publisher tests never seed (the same commit patched the siblingaccess-verification.test.tsbut missed these). The marker is necessary but, for the real-publisher tests, not sufficient — the per-KA…/context/n/_metametadata graph interacts with the filter's sub-graph exclusion in a way that needs a bit more investigation. The 2 collapsed-multi-root cases in the file are a clean marker-seed; the 6 real-publish cases need the deeper fix.kafka-plugin-api.e2e+e2e-joinare real (heavy e2e);host-mode-public-ingest,vm-reconcile-self-prime,workspaceare stale wire-fixture/scaffolding tests.Note on canary CI
.github/workflows/ci.ymlpull_request.branchesdoes not includetestnet-canary, so this PR (and other canary-targeted PRs) run only advisory checks. The verifications above were run locally against the built packages and, where a chain was needed, the hardhat harness.🤖 Generated with Claude Code