fix(publisher): classify rejected KA updates - #1747
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>
…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>
| * check and its chain-submit path so both classify rejections identically. | ||
| */ | ||
| function extractV10UpdateRejectionName(err: unknown): string | undefined { | ||
| return enrichEvmError(err) ?? (err as { revert?: { name?: string } })?.revert?.name; |
There was a problem hiding this comment.
🟡 Issue: Keep EVM error-shape knowledge in the chain layer
What's wrong
This adds an EVM-provider-specific error-shape fallback inside the publisher. That muddies the package boundary and creates a second canonical place for EVM error decoding, which makes future rejection classifiers more likely to drift.
Example
The publisher now has to know that an ethers error may expose err.revert.name, while other callers still use enrichEvmError(err) directly. The next update/publish/daemon classifier that needs the same predecoded shape will either miss it or add another local cast.
Suggested direction
Make chain error classification the boundary. The publisher should ask for an error name, not inspect ethers-specific object internals with a cast.
For Agents
Move the revert.name fallback into the chain package, either by making enrichEvmError return an existing predecoded name or by exporting a typed extractEvmErrorName helper. Update dkg-publisher.ts to consume that canonical helper and preserve the current definitive-error behavior.
| status: 'failed', | ||
| publicQuads: allSkolemizedQuads, | ||
| }; | ||
| earlyReturn = await buildFailedUpdateResult(); |
There was a problem hiding this comment.
🟡 Issue: Model submit outcomes directly instead of using sentinel state
What's wrong
The change centralizes the failed result shape, but it still threads control flow through two mutable sentinels. That makes readers reason about earlyReturn, txResult.success, and the write-ahead finally together just to understand one terminal state. In a 9k-line publisher class, this is exactly the kind of incidental state that makes future edits brittle.
Example
A definitive submit rejection sets earlyReturn = await buildFailedUpdateResult() and also sets txResult = { success: false, hash: '' }; a non-throwing failed TxResult skips earlyReturn and reaches the separate !txResult.success branch that builds the same result again.
Suggested direction
Replace the earlyReturn plus fake txResult coupling with an explicit submit outcome. That would remove one mutable mode from this already large update() path and make the rejection/failure exits read as one policy.
For Agents
In DKGPublisher.update, extract the V10 submit block into a helper or local function that returns a discriminated outcome like { kind: 'submitted', txResult } or { kind: 'definitive-rejection', errorName }. Keep the write-ahead finally, preserve phase-end ordering, and have one failed-result exit for rejected/unsuccessful outcomes.
2 participants
Summary
Ports the useful, main-applicable fixes from #1744 onto the post-#1746 tree.
publisher.update()now converts deterministic preflight and submission reverts for non-existent, expired, or immutable KAs into the documented{ status: "failed" }result while preserving unexpected/infrastructure errors as thrown failures. It also carries the chain mock-parity and storage scheduler test corrections that were needed by the consolidated tree.Why now
The missing non-existent-KA classification is the exact failure currently making #1745's publisher EVM integration lane red (
ERC721NonexistentToken).Verification
git diff --checkSupersedes the relevant committed portion of #1744; unrelated local-only queue work is intentionally not included.