fix(protocol): authorize curated publish by attested author OR payer (#1689)#1971
fix(protocol): authorize curated publish by attested author OR payer (#1689)#1971Jurij89 wants to merge 7 commits into
Conversation
| // operators actually read it. | ||
| if ( | ||
| !contextGraphs.isAuthorizedPublisher(p.contextGraphId, msg.sender) && | ||
| !contextGraphs.isAuthorizedPublisher(p.contextGraphId, p.authorAddress) |
There was a problem hiding this comment.
🔴 Bug: Author-side CG authorization accepts a CG-independent author seal
What's wrong
The new OR branch changes the meaning of an author seal from attribution to publish authorization, but the signed payload does not bind the target context graph. That lets a seal be replayed as authorization for any context graph where the author address is authorized, while the payer chooses the CG in calldata.
Example
A signs/finalizes content root R with reservedKaId K, but that signature does not name a context graph. Any holder of that seal can choose contextGraphId X where A is the curated publish authority, pay from an otherwise unauthorized wallet P, and line 910 authorizes the publish. That also consumes A's namespaced K for the chosen CG, even if A never consented to publishing R into X.
Suggested direction
Do not use the existing CG-independent AuthorAttestation as the authorization credential for curated-CG publish admission. Either bind contextGraphId into the author-side authorization signature, introduce a separate scoped delegation/consent, or keep payer-only authorization when the seal is not scoped to the target CG.
Confidence note
This is based on the on-chain authorization change plus the existing AuthorAttestation digest shape, which explicitly omits contextGraphId. If the intended product contract is that one seal authorizes publication into any CG where that author is permitted, this needs a human policy decision; otherwise this is a replay/consent bug.
For Agents
Look at KnowledgeAssetsLifecycle._executePublishCore and the AuthorAttestation typed-data builders. Preserve the desired delegated-payer flow, but require the author-side authorization credential to be scoped to the target CG or some explicit publish-policy consent before using p.authorAddress as a CG authorization principal. Add a test proving the same author seal cannot be redirected to a different curated CG by an unauthorized payer.
There was a problem hiding this comment.
The technical analysis is correct and the closing note identifies the crux exactly: "If the intended product contract is that one seal authorizes publication into any CG where that author is permitted, this needs a human policy decision."
It is a human policy decision, and it was taken deliberately before implementation — it is disclosed in the PR body under "Known residual risk, stated plainly." Not resolving this thread: the policy call belongs to a human reviewer, and this is the right place for them to overrule it.
Where the bound actually sits, since the example understates it in one direction and overstates it in another:
- The redirect target is not arbitrary. Line 910 authorizes only when
isAuthorizedPublisher(cgId, p.authorAddress)is true, so for a curated CG the target must be one where the author is the stored publish authority — i.e. a CG the author themselves curates. An attacker cannot redirect a seal into a CG they control. - The open-CG case (
publishPolicy == 1, which authorizes any non-zero address) is pre-existing and unchanged: an attacker is already authorized there via the payer branch today, without this PR. So the incremental exposure is confined to curated/PCA CGs where the author is the authority or an affiliated agent. - You are right about the sharpest harm, and it is worth naming precisely: the publish consumes A's namespaced
reservedKaIdK permanently, andkaToContextGraphbinding is irreversible — so A can never republish that slot to the intended CG. The attacker also choosesepochsandisImmutable. - Bounding the other way: the attacker pays (every cost leg is keyed to
msg.sender— allowance,transferFrom, escrow, PCA discount, all deliberately unchanged by this PR), and the ERC-721 still mints to the author. It is attacker-funded griefing and false publisher-of-record attribution, never asset theft, and there is no victim-funded path.
On the three suggested directions:
- Bind
contextGraphIdinto the author attestation — this reverses Clarify knowledge asset lifecycle when SWM share succeeds without a seal but VM publish requires finalization #1116, which removed it specifically so seals could be produced before CG registration. It also changes the EIP-712 typehash, requiring client/contract lockstep. Deferred to a follow-up, not rejected. Note the KAL redeploy this PR already needs would be the cheapest moment to change the attestation shape, since rotation invalidates every outstanding attestation regardless — that argument is recorded for the follow-up. - A separate scoped delegation/consent — the cleanest long-term answer, but it needs new storage, which means redeploying
ContextGraphStorageand migrating all live CG state. Out of scope here. - Keep payer-only when the seal is not CG-scoped — this is the status quo, i.e. not fixing bug(protocol): curated publish authorization checks the transaction payer instead of the attested agent #1689 at all. Since no seal is CG-scoped today, this branch would be dead code.
A deadline field is the cheapest partial mitigation: it bounds the bearer window without reversing #1116's CG-independence. That plus withholding authorAttestationR/VS from the durable-_meta sync responder is the shape of the follow-up.
Flagging one thing your analysis did not reach, in case it sharpens the policy call: the seal is persisted to the CG's _meta graph at finalize time and replicated to peers over the durable _meta sync lane — and for a public CG, authorizeSyncRequest returns true unconditionally, so the attestation is world-readable there. For private CGs the barrier is CG membership, i.e. exactly the co-tenants best positioned to grief a curator.
There was a problem hiding this comment.
Follow-up filed: #1972, carrying the full analysis — the bearer-token shape, the reachability path through the durable _meta sync lane, the honest bounds in both directions, and the four options with tradeoffs.
It also records the timing argument you implicitly raise: this PR already requires a KAL redeploy, and rotation invalidates every outstanding attestation anyway (the KAL address is in the EIP-712 domain separator), so that is the cheapest moment to change the attestation shape — the migration cost is already being paid.
Leaving this thread unresolved for a human to rule on, per your confidence note.
| 'isAuthorizedPublisher', contextGraphId, s.address, | ||
| ))) return false; | ||
| // (mirrors nextAuthorizedSigner); that fail-open lives in `_cgPublishAdmits`. | ||
| if (!(await this._cgPublishAdmits(contextGraphId, s.address, attestedAuthorAddress)).admitted) { |
There was a problem hiding this comment.
🟡 Issue: Unify the signer admission model instead of re-running it in fundability probing
What's wrong
The PR centralizes the new publish policy in _cgPublishAdmits, but signer-pool selection and insufficient-funds enrichment still assemble admitted candidates independently. That leaves two flows to keep mentally synchronized and already produces different orchestration: one checks the author once up front, the other calls the full two-principal predicate per wallet. This is the kind of policy duplication that will drift the next time publish admission changes.
Example
With a 10-wallet pool and an authorized attested author, _authorizedPublisherSigners can admit the whole pool after one author policy read, but poolHasFundableSigner fans out through every wallet and repeats the same admission policy in a different order.
Suggested direction
Introduce one canonical admission/candidate helper for signer-pool publish eligibility, and let _authorizedPublisherSigners add throwing/error-shaping on top of it. poolHasFundableSigner should ask that helper for the admitted candidate wallets and then only run funding checks.
For Agents
Look in packages/chain/src/evm-adapter-base.ts around _authorizedPublisherSigners and poolHasFundableSigner. Preserve payer-only legacy behavior, author-widens-to-whole-pool behavior, and no-funded enrichment behavior. Extract a non-throwing admittedPublisherSigners/policy helper returning the admitted wallet list plus admission facts; have selection, error shaping, and fundability probing consume that single result. Existing admission and no-funded tests should still pass, with a focused test proving the author-admitted pool path does not re-evaluate per signer.
There was a problem hiding this comment.
Deferring the refactor, but taking the performance observation seriously — they are separable.
On the refactor: the asymmetry you describe is deliberate, not drift. _authorizedPublisherSigners evaluates the author once for the whole pool specifically to avoid per-wallet read amplification (the #1583/#1549 class); collapsing both flows through one candidate helper would either reintroduce that per-wallet cost or push the one-read optimisation down into the shared helper, where it becomes a behaviour both callers must not accidentally change. The current code is mutation-proven — 8 mutations, 7 distinct properties, each killing exactly its intended test — and a no-behaviour-change refactor of an authorization path with zero CI on this branch trades real risk for readability.
Recording the shape so it survives: a non-throwing admittedPublisherSigners(ordered, cgId, author) -> { wallets, admission }, with _authorizedPublisherSigners adding only throwing/error-shaping and poolHasFundableSigner consuming the wallet list before funding checks.
On the performance point, which is the part I think is actually load-bearing: you are right that poolHasFundableSigner runs the full two-principal predicate per wallet while the selection path does not. I have asked the implementer to confirm whether those reads coalesce through KeyedSingleFlight — one reviewer believed the per-wallet fan-out collapses to roughly a single read, and if that is wrong this is an N-read regression on a common path rather than a tidiness issue. I will post the answer here either way, and file it as a follow-up if the coalescing does not hold.
There was a problem hiding this comment.
Correction to my previous reply: the reads do NOT coalesce. Your performance observation is right, and the assumption I passed on was wrong.
Traced rather than assumed. isAuthorizedPublisher goes this.readContract → rpcFailover.readContract (rpc-failover-client.ts:381) — a plain failover read with no cache and no single-flight. KeyedSingleFlight only reaches production through ReadThroughTtlCache, and its complete set of consumers in packages/chain/src is:
evm-adapter-base.ts:274 identityIdCache
evm-adapter-base.ts:899 resolvedContractAddressCache (#1583 address memo)
evm-adapter-base.ts:912 deployedLifecycleVersionCache (#1689, this PR)
pca-read-cache.ts:14,18 PCA agent/account reads
No CG-authorization read is among them. So poolHasFundableSigner issues one isAuthorizedPublisher read per wallet, and with an author supplied up to 2N — a payer read per wallet plus an author read per wallet that fails the payer check. Only version() is memoized, so that part stays at 1.
Severity is low and we are still not refactoring, but for a different and now-honest reason than the one I gave: this is enrichInsufficientPublisherFundsError, a cold path that runs once per already-failed publish with N ≈ 1-5 wallets. It is not on the publish hot path.
Filed as a follow-up rather than closed. The fix is a one-line hoist of the author branch out of the per-wallet loop — exactly the shape already used in _authorizedPublisherSigners. The original tradeoff (routing per-wallet through the single predicate was worth more than saving reads on an error path) still holds, but it is worth restating against the correct cost rather than against a wrong one.
Thanks for pushing on this — the sub-point turned out to be the load-bearing half of the finding, and my first answer would have closed it on a false premise.
There was a problem hiding this comment.
🟡 Issue: Move the publish-admission policy out of the base adapter
What's wrong
This PR pushes a fairly self-contained publish-authorization feature into EVMChainAdapterBase, which is already a very large shared class. The new code mixes capability detection, cache ownership, admission policy, diagnostic shaping, signer-pool filtering, and funding-reroute probing in one layer. That makes the feature harder to scan and increases the chance that the next publish path calls the wrong helper or recomputes the same author-side facts differently.
Example
A reader now has to understand the #1689 policy across version caching, _cgAdmitsAttestedAuthor, _cgPublishAdmits, _publishAuthorizationRejection, _authorizedPublisherSigners, poolHasFundableSigner, and createKnowledgeAssets. poolHasFundableSigner also re-enters _cgPublishAdmits per wallet, while _authorizedPublisherSigners precomputes author admission once, which is a sign that the missing model is an admission evaluator/context rather than another base-class helper.
Suggested direction
Create a dedicated admission module or small policy object that is initialized with the chain read dependencies and contextGraphId/optional author. The base adapter should orchestrate signer/funding selection; the publish policy should answer admission and build rejection details. This would delete several feature-specific methods from an already sprawling base class and make the two-principal gate easier to reason about.
For Agents
Look in packages/chain/src/evm-adapter-base.ts and packages/chain/src/evm-adapter-publish.ts. Preserve the same payer-or-author admission behavior and diagnostics, but extract a focused evm-adapter-publish-admission policy/evaluator that owns lifecycle-version support, author admission, payer admission, and rejection details. Then have signer selection, publish planning, and funds enrichment call that object. Existing tests around #1689 should still pass; add/adjust one unit test proving the fundability path uses one author admission result rather than per-wallet re-evaluation.
There was a problem hiding this comment.
Deferring, and recording the shape rather than dismissing it.
This is the second round asking for the same restructuring in different words (the earlier one proposed unifying the signer admission model). I read that repetition as a genuine signal about the design, not noise — but it does not change the risk calculus for this PR.
Why not here: it is a no-behaviour-change restructuring of an authorization path, on a branch where ci.yml is filtered out entirely, packages/evm-module/** is absent from the only path-triggered workflow, and the code as it stands is mutation-proven (8 mutations, 7 distinct properties, each killing exactly its intended test). Restructuring it now trades a verified arrangement for a better-shaped unverified one, with no CI to catch the difference. That is a bad trade to make in the same PR as the behaviour change.
Recording the shape so it survives: an admission evaluator constructed with the chain read dependencies plus contextGraphId and optional attested author, owning lifecycle-version support, payer admission, author admission and rejection-detail construction — with the base adapter orchestrating only signer selection and funding. Signer selection, publish planning and funds enrichment all consume that one object.
One correction to the example. You cite poolHasFundableSigner re-entering _cgPublishAdmits per wallet while _authorizedPublisherSigners precomputes author admission once, and read the asymmetry as evidence of a missing model. The asymmetry is deliberate: the selection path hoists the author check to one read for the whole pool specifically to avoid per-wallet read amplification (the #1583/#1549 class), while the enrichment path is cold — it runs once per already-failed publish over 1-5 wallets — and there the uniformity of routing every wallet through one predicate was judged worth more than the reads.
That said, your underlying observation was right and sharper than I first credited: those reads do not coalesce (traced — no CG-authorization read is a ReadThroughTtlCache consumer), so the enrichment path costs up to 2N reads. I have corrected that publicly on the sibling thread and filed it as a follow-up. The hoist is a one-line fix; it just is not worth a refactor of the surrounding structure.
Not resolving — leaving it for a human to overrule if they want the restructuring in this PR.
There was a problem hiding this comment.
Sixth framing of the same request, and tracked as #1973 since the fourth. The issue carries the shape, the invariants any extraction must preserve, the 2N-read perf item folded in as scope, and the hazard that the version memo is keyed on the resolved lifecycle address specifically so a Hub rotation invalidates it for free.
I take the repetition as a real signal about the design rather than noise — which is why it is an open issue rather than another decline. It is not landing in this PR for the stated reason: no CI on this branch, and the current arrangement is mutation-proven at every guard.
One update worth adding to your case, in your favour: a 🔴 landed this round showing the admission result being collapsed to a boolean in the funding-reroute probe, dropping the transient-vs-denied distinction. That is exactly the class of bug a typed policy object with explicit states would have made hard to write — so the extraction has now earned a concrete argument beyond cohesion. Recorded on #1973.
There was a problem hiding this comment.
🟡 Issue: Extract the publish-admission policy instead of growing the base adapter
What's wrong
This PR adds a substantial, feature-specific authorization state machine directly into an already very large adapter base class. The behavior is heavily documented, but structurally it is still scattered across several protected/private methods and callers, so the reader has to reconstruct one policy from multiple partial predicates. That worsens cohesion and makes future changes to publish authorization easy to apply incompletely.
Example
A future admission change now has to be threaded through several methods with subtly different semantics: pinned rejection uses _cgPublishAdmits, pool selection pre-checks _cgAdmitsAttestedAuthor, and funding enrichment loops back through _cgPublishAdmits per signer while separately remembering versionReadError. Missing one path would leave the adapter internally inconsistent even if each helper reads correctly in isolation.
Suggested direction
Move the context-graph publish admission state machine behind a small dedicated abstraction, e.g. resolveCgPublishAdmission / selectAdmittedPublishers, and let the adapter call that from the three orchestration sites. That should delete the repeated helper layering and the mutable cross-path error plumbing rather than just centralizing comments.
For Agents
Look in packages/chain/src/evm-adapter-base.ts around the new admission helpers and pool/funding callers. Preserve payer-first behavior, deployed-version fail-closed behavior, transport-error propagation, and legacy no-author error text. Extract a focused publish-admission/candidate policy module or helper that returns one typed result used by pinned selection, pool selection, and funding enrichment; prove pinned, unpinned, and enrichment paths all consume the same result.
There was a problem hiding this comment.
Seventh framing, tracked as #1973 since the fourth. Not declining again — the repetition is a signal about the design and it is recorded as one.
Your example is sharper than the earlier versions and I have added it to the issue: a future admission change must be threaded through _cgPublishAdmits (pinned rejection), _cgAdmitsAttestedAuthor (pool pre-check), and the funding-enrichment loop that calls _cgPublishAdmits per signer while separately remembering versionReadError — and missing one leaves the adapter internally inconsistent even though each helper reads correctly alone.
That is not hypothetical any more. Exactly that happened this round: the two authorization gates were taught to distinguish a transient read from a policy denial, and the third consumer — the funding-reroute probe — kept collapsing it to a boolean, so a transient blip became a terminal NO_FUNDED_PUBLISHER_WALLET. Fixed in d39ab18b9. Your argument now has a concrete instance behind it rather than a cohesion claim, and #1973 says so.
Still not landing in this PR: no CI on this branch, and every guard is currently mutation-proven. The extraction deserves a branch where a behaviour-neutral restructuring of an authorization path is actually verified.
There was a problem hiding this comment.
🟡 Issue: Author pre-probe can block payer-authorized pool publishes
What's wrong
The new unpinned pool path probes the attested author before checking whether any payer wallet is authorized. That probe catches only lifecycle version transport failures; a transport failure while reading the author's context-graph authorization aborts the whole selection immediately. This adds a new failure mode to publishes that do not need author-side authorization at all because the paying wallet already satisfies the policy.
Example
In an unpinned publish where walletA is already authorized for the context graph and attestedAuthorAddress is present, a transient RPC failure on the author-side isAuthorizedPublisher(cg, author) read makes _authorizedPublisherSigners reject before it ever reaches the payer loop at lines 2273-2275. The on-chain gate would accept through msg.sender, and the pinned path checks the payer first, so this only affects the new pool pre-probe ordering.
Suggested direction
Either check payer eligibility before the author optimization, or have _cgAdmitsAttestedAuthor return an inconclusive admission for author authorization transport failures the same way it does for version-read transport failures, then only rethrow if no payer-authorized signer is available.
For Agents
Look at _authorizedPublisherSigners and _cgAdmitsAttestedAuthor in packages/chain/src/evm-adapter-base.ts. Preserve the optimization that an admitted author returns the pool unnarrowed, but treat author-read transport failures as inconclusive and defer throwing until after payer-authorized wallets have been checked. Add a test where the author read throws a ChainRpcTransportError, the first pool wallet is payer-authorized, and _authorizedPublisherSigners still returns that wallet.
There was a problem hiding this comment.
🟡 Issue: The deployed-version cache rotation behavior is not verified
What's wrong
The PR relies on the new deployed lifecycle version cache being keyed by the resolved KAL address so a running node starts using the author-aware gate after Hub rotation. The added tests cover threshold comparisons, unreadable versions, and failed-read non-memoization, but they do not exercise a version change caused by the lifecycle address changing. That leaves the self-healing rotation behavior unverified.
Example
A regression that keyed deployedLifecycleVersionCache.getOrLoad() by a constant like 'KnowledgeAssetsLifecycle' would still pass the current one-address tests: first read 10.1.6 from the old KAL, then after replacing contracts.knowledgeAssetsLifecycle with a new handle at a different address returning 10.1.7, the adapter could keep using the cached old version and continue rejecting author-authorized publishes.
Suggested direction
Cover the two-address lifecycle rotation scenario that this cache design is meant to support, not only the single-address threshold matrix.
Confidence note
I did not run the suite because the workspace is read-only, but the added tests I inspected all exercise a single lifecycle address, so this looks like an unbound behavior rather than a failed implementation.
For Agents
Add a focused chain unit test around deployedKnowledgeAssetsLifecycleVersion or the public planning path. Use one adapter, first set the lifecycle handle to address A returning one-patch-below-threshold and assert the author branch rejects, then swap the handle to address B returning ATTESTED_AUTHOR_PUBLISH_AUTHZ_MIN_KAL_VERSION and assert the same author-authorized publish is admitted without waiting for TTL. The test should fail if the version cache is keyed by anything other than the resolved lifecycle address.
| * `rpc_unavailable` default that made this failure loop to `maxRetries`. Same | ||
| * shape and convention as {@link InsufficientPublisherFundsError}. | ||
| */ | ||
| export class PublisherNotAuthorizedForContextGraphError extends Error { |
There was a problem hiding this comment.
🟡 Issue: Keep the authorization diagnostics structured on the error object
What's wrong
The new exported details type is more precise than the exported error class. The class accepts the structured diagnostic facts, uses them to build text, then drops several of them. That weakens the type boundary and pushes future consumers back toward message-string coupling for information the adapter already computed exactly once.
Example
A downstream handler that wants to show “chain not rotated yet” vs “author was checked and refused” cannot read that from the structured error; it has to parse error.message, even though the typed details already existed at throw time.
Suggested direction
Make the error carry the full details object, or remove the exported detail model if it is intentionally only a formatter input. The cleaner boundary is to preserve the typed facts and let message formatting be a presentation layer.
For Agents
Look in packages/chain/src/evm-adapter-errors.ts. Preserve the public code/message contract, but either store readonly details: PublisherNotAuthorizedForCgDetails on PublisherNotAuthorizedForContextGraphError or expose all detail fields explicitly. Add/update a narrow unit assertion that the real error object carries the same diagnostic facts passed to the formatter.
There was a problem hiding this comment.
Fixed in 0eb8a4c70.
PublisherNotAuthorizedForContextGraphError now carries readonly details: PublisherNotAuthorizedForCgDetails, frozen so a consumer cannot mutate one error's diagnosis into disagreeing with its own text. code, the message contract and the flattened convenience fields are unchanged — publisher's classifier keys on code with a message-prefix fallback, and evm-adapter.unit.test.ts:4277 asserts payerAddress, so both keep working.
Your example is now directly answerable from the object: attestedAuthorConsidered together with deployedLifecycleVersion separate "author checked and refused" from "author never weighed because the deployed lifecycle predates the capability, or its version was unreadable". No message parsing.
You were pointing at this PR violating its own principle one layer down — we told the publisher's classifier to key on structured codes instead of message text, then shipped an error that forced message-parsing for the next question anyone asked. That is the more useful framing of the finding than the type-boundary one, and it is why this was worth fixing rather than deferring.
Covered by a new test asserting the thrown object carries the same facts the message was rendered from, plus one for the signer-pool shape. Chain unit suite 853 → 857 passing; closure build clean.
There was a problem hiding this comment.
🟡 Issue: Avoid duplicating the new error payload as both details and top-level fields
What's wrong
The new error class introduces two representations of the same diagnostic facts. Because this is a new public error contract, the flattened properties do not buy much compatibility, but they do add API surface and future maintenance decisions for every new field.
Example
Consumers can now read either error.details.payerAddress or error.payerAddress. When the next diagnostic field is added, authors have to decide whether it belongs only in details or also as a top-level field, which makes the error contract noisier than it needs to be.
Suggested direction
Make details the canonical structured surface, with optional read-only getters only if needed. Avoid storing the same facts twice on a brand-new exported error type.
Confidence note
This assumes the public PublisherNotAuthorizedForContextGraphError class is new in this PR, as shown in the supplied diff. If another unrevealed downstream consumer already depends on the flattened fields, getters can preserve that surface without duplicating state.
For Agents
In packages/chain/src/evm-adapter-errors.ts, keep details as the single machine-readable payload. Either remove the flattened properties and update tests to read error.details, or expose them as getters that delegate to details if the ergonomic top-level API is important. Preserve the formatted message and code contract.
There was a problem hiding this comment.
Fixed in db0c322fe. Your confidence note was right — the class is new in this PR, so the flattened fields bought no compatibility and only doubled the state.
details is now the single source; the four flattened properties are getters delegating to it. code, the message contract and every accessor name are unchanged, so the publisher's classifier and existing error.payerAddress assertions keep working while the two representations can no longer disagree.
The serialization risk was checked before writing it, not after: getters are not own enumerable properties, so anything spreading or JSON.stringify-ing the error would silently lose these fields. Grepped {...err}, Object.assign({}, err) and JSON.stringify(err|error) across publisher/chain/agent/core — no hits; the publisher reads .code/.message by plain property access, and errorMessage() takes the instanceof Error branch before its stringify fallback. That dependency is now recorded in a code comment, since it is the condition under which this shape stays safe.
The part worth reporting, because it is a finding rather than a fix: the first mutation of this change survived. Hard-coding one flattened accessor to ignore details left 47 tests passing. The existing tests asserted details.* and message === format(details), but nothing asserted the accessors actually agree with details — so "one source of state", the entire property this fix exists to guarantee, was itself unverified.
Assertions added that each accessor equals its details counterpart, on both the pinned and pool error shapes. The same mutation now fails 2.
That makes it the fourth instance on this PR of a check confirming what it expected rather than proving what executes — this time inside the fix for the third instance. It was caught by mutating rather than by reading, which is the only reason it did not ship looking correct.
There was a problem hiding this comment.
🟡 Issue: Make the structured authorization error own its message formatting
What's wrong
The new error model says details is the machine surface and the message is rendered from those facts, but the constructor still accepts message separately. That leaves an attractive footgun where future callers can create an error whose .message and .details disagree, then downstream classifiers and operator diagnostics reason from different representations.
Example
The tests already construct new PublisherNotAuthorizedForContextGraphError('context graph publish policy declined this publish', { ...details }), which proves the class can carry details that do not describe its own message. That undercuts the “single source of facts” invariant the new type is trying to establish.
Suggested direction
Collapse the two inputs into one source of truth: construct the error from PublisherNotAuthorizedForCgDetails and derive message inside the constructor, or expose a factory that does that. That deletes the caller-side obligation to keep message and details synchronized.
For Agents
In packages/chain/src/evm-adapter-errors.ts, change PublisherNotAuthorizedForContextGraphError to accept details and optional cause, and have it call formatPublisherNotAuthorizedForCgMessage(details) internally. Update _publishAuthorizationRejection and tests accordingly. If custom messages are required for tests, use a separate generic Error there rather than weakening the production error type.
There was a problem hiding this comment.
Fixed in 65c9bbb96. Sharp finding, and it cited our own test as the proof — which is the strongest form the argument could take.
The constructor now accepts details alone (plus optional cause) and derives the message internally via formatPublisherNotAuthorizedForCgMessage. An error whose .message contradicts its own .details is no longer merely untested, it is unconstructible. _publishAuthorizationRejection and the tests are updated; the one test that wanted arbitrary text now uses a plain Error, per your suggestion.
Worth recording the sequence, because the finding lands in the middle of it. Last round we made details the single source of state and asserted message === format(details) — which looked like it closed this. It did not: it verified the values agreed, while the constructor signature still allowed a caller to supply text that disagreed. The assertion checked the instances we happened to build; your finding removed the capability. We kept the assertion anyway, since it now pins that the constructor has not regressed rather than being the only guard.
Same shape as the other findings on this PR: a check that confirms what it expects, sitting next to a door left open.
There was a problem hiding this comment.
🟡 Issue: Avoid exporting an error shape that depends on non-enumerable prototype getters
What's wrong
The new error type creates a second machine-readable surface over details, but implements that surface as prototype getters specifically because current consumers do not serialize or spread the error. That makes the exported contract harder to reason about and easier to misuse. The implementation is more clever than it needs to be for a diagnostic object.
Example
A consumer that logs or forwards { ...err } will see own fields like code and details, but not the flattened contextGraphId, payerAddress, payerPoolAddresses, or attestedAuthorAddress getters. That is surprising for an exported error type, especially next to InsufficientPublisherFundsError, which uses straightforward own fields.
Suggested direction
Prefer one clear contract: either consumers read error.details, or the flattened fields are own properties. The current hybrid adds indirection and depends on a fragile usage audit captured in comments.
Confidence note
This is an API-shape concern rather than an observed current caller break; the code comments themselves state the assumption that callers do not spread or JSON-serialize this error.
For Agents
Look at PublisherNotAuthorizedForContextGraphError in packages/chain/src/evm-adapter-errors.ts. Preserve the derived message invariant and frozen details. Either make details the only machine-readable surface and remove the flattened getters, or define the flattened fields as own enumerable properties derived from the same frozen normalized details.
There was a problem hiding this comment.
Validated independently. The JavaScript observation is correct; every step of its chain to actual harm is broken in this codebase. Declining, with the evidence rather than an assurance.
Prototype getters are indeed not own-enumerable, so spreading, Object.assign-copying, structuredClone-ing or JSON.stringify-ing such an error silently drops those fields. That is a real hazard in general.
It does not reach harm here. Re-checked across packages/{chain,publisher,agent,core,cli}/src: nothing spreads or clones this error, and the one path that could have made it real — the async publisher persisting a failure to a job record — does not serialize the error object. createLiftJobFailureMetadata records code and message only, both of which survive any transform, since message is an own property set by the Error constructor and code is an own field on the class. Nothing carries this error across a process, IPC or HTTP boundary.
Worth being explicit about what would flip this: if a future consumer serialized the error into a job payload or shipped it over the daemon API, the diagnostic fields would vanish silently — no type error, no test failure, just an empty object where the facts used to be. That is a genuine trap, and the honest mitigation is a toJSON() rather than reverting to duplicated own-properties, which is what the previous round removed for good reason.
Recording it as a known property of the shape rather than fixing something that cannot currently misbehave. If a serialization consumer appears, toJSON() is the fix, and it is a smaller change than either alternative.
There was a problem hiding this comment.
🟡 Issue: Avoid exported error getters that rely on consumers not serializing the error
What's wrong
The new error class deliberately creates non-enumerable getter fields and justifies that by assuming no consumer will spread or serialize the error. That is a brittle API boundary for an exported error type whose reason to exist is machine-readable diagnostics.
Example
error.payerAddress works, but { ...error } or JSON-style capture will omit payerAddress, contextGraphId, and attestedAuthorAddress because they are getters on the prototype rather than own enumerable fields. A future logger or job serializer can lose the data while tests that use direct property reads still pass.
Suggested direction
Prefer one clear public contract. If flat fields are part of the API, make them own readonly fields. If details is the API, remove the flattened accessors so consumers do not have two subtly different surfaces.
Confidence note
The current publisher paths may only use direct property access today, but this is exported API and the comment itself documents the fragility.
For Agents
In packages/chain/src/evm-adapter-errors.ts, either expose only details as the machine-readable contract, or assign the compatibility fields as own readonly properties from the frozen details in the constructor. Preserve the derived message invariant and update tests to cover the chosen public shape.
| // threaded through because it widens WHICH wallets are viable reroutes | ||
| // (#1689) — probing on the narrower payer-only rule could claim the pool is | ||
| // unfundable while a funded wallet was in fact admissible. | ||
| if (selectedShort && !(await this.poolHasFundableSigner( |
There was a problem hiding this comment.
🟡 Issue: The new author-aware insufficient-funds enrichment path lacks a regression test
What's wrong
The diff changes the publish failure enrichment path so an attested author can widen which wallets count as viable funded reroutes. That path controls whether a publish failure becomes terminal NO_FUNDED_PUBLISHER_WALLET, but the added tests do not exercise it with an attested author. A regression could silently turn recoverable author-authorized reroutes into terminal insufficient-funds failures.
Example
A pinned payer wallet A is authorized by the attested author but has zero TRAC; wallet B is funded and not payer-authorized. With lifecycle version >= 10.1.7 and AUTHOR authorized, createKnowledgeAssets should preserve the original selected-wallet funds error so a retry can reroute. If the author argument is dropped from the poolHasFundableSigner call, the existing tests still pass but this scenario is misclassified as terminal NO_FUNDED_PUBLISHER_WALLET.
Suggested direction
Cover the author-authorized reroute case in the same test group that already verifies NO_FUNDED enrichment, so omitting attestedAuthorAddress from this probe fails loudly.
For Agents
Add a chain adapter unit test near the existing insufficient-funds enrichment cases in packages/chain/test/evm-adapter.unit.test.ts: configure a supported lifecycle version, authorize only the attested author, make the selected/pinned payer short, make another pool wallet fundable, force a transferFrom-style revert, and assert the error is not InsufficientPublisherFundsError. Keep the existing payer-only NO_FUNDED tests unchanged.
There was a problem hiding this comment.
Fixed in 0eb8a4c70. Your reasoning was exactly right, including the failure mode.
Added to evm-adapter-publish-admission.unit.test.ts: supported lifecycle version, only the attested author authorized, pinned payer short on TRAC, another pool wallet fundable — asserting the error is not InsufficientPublisherFundsError, so the reroute stays recoverable. Plus a payer-only negative control asserting an unauthorized-and-unfunded pool is still terminal NO_FUNDED_PUBLISHER_WALLET, so the legacy path is pinned unchanged rather than merely assumed.
Mutation-proven, since a passing test proves nothing on its own here: dropping attestedAuthorAddress from the poolHasFundableSigner call fails exactly the new author-authorized row and leaves the payer-only control green. That one-and-only-one kill is what shows the row covers something nothing else did.
Worth naming why this finding mattered more than its severity label suggests: this is the third instance on this PR of a line whose deletion nothing notices. The adversarial review caught the same shape at dkg-publisher.ts:3055, where removing a single forwarding line type-checked cleanly and left 602 tests green while silently un-fixing the issue for the async lane. Your example — "if the author argument is dropped from this probe, the existing tests still pass but this scenario is misclassified as terminal" — is that pattern stated precisely, and it is the class of gap worth prioritising over most style findings.
(The test lives in evm-adapter-publish-admission.unit.test.ts rather than beside the existing NO_FUNDED cases in evm-adapter.unit.test.ts — same suite, same runner, but a different file was being edited concurrently by another author and we avoid shared-file contention. Happy to relocate it if you would rather they sat together.)
There was a problem hiding this comment.
🟡 Issue: Direct createKnowledgeAssets author threading is not verified
What's wrong
The PR adds the direct EVM write path as another place where the attested author must be threaded into authorization. Current added tests verify the contract behavior, planner request forwarding, protected admission helpers, and publisher-level planner forwarding, but not that createKnowledgeAssets itself passes params.author?.address into its authorization gates. That leaves a user-facing sync/direct publish regression unprotected.
Example
A regression that removes attestedAuthorAddress from the createKnowledgeAssets calls would still leave the planner tests and contract tests green, but a.createKnowledgeAssets({ publisherAddress: payer, author: { address: AUTHOR, ... } }) would pre-reject when only AUTHOR is authorized on the CG.
Suggested direction
Cover the public write path that derives attestedAuthorAddress from params.author, not only the lower helper/planner paths.
Confidence note
I did not run the suite because the workspace is read-only; this is based on the diff and the checked test files.
For Agents
Add a chain adapter unit test around EVMChainAdapter.createKnowledgeAssets itself. Configure a funded pinned payer that is not CG-authorized, authorize only params.author.address, return KAL version at the threshold, stub signing to throw a sentinel after selection, and assert the sentinel is reached rather than a publisher-not-authorized error. Add the unpinned variant if that path is supported directly.
There was a problem hiding this comment.
Fixed in db0c322fe. This was the most valuable finding of the round.
Three tests through the public createKnowledgeAssets, using a lifecycle handle that throws a sentinel the instant a signer is selected — reaching the sentinel proves the gate admitted, a PublisherNotAuthorizedForContextGraphError proves it did not:
- pinned payer unauthorized + author authorized → sentinel reached
- unpinned pool, same → sentinel reached
- neither principal authorized → still rejects
That third one exists to stop the first two passing for the wrong reason: if the threading were "fixed" by skipping the gate entirely, both positives would still go green and only this control would fail.
Mutation-proven per call site, because a proof covering one would leave the other exactly as unguarded as it was:
drop author from the PINNED call → 1 failed | 46 passed (× PINNED payer: an authorized author admits…)
drop author from the UNPINNED call → 1 failed | 46 passed (× UNPINNED pool: an authorized author admits…)
Worth naming why this one mattered beyond its severity label: it is the third instance of the same shape on this PR — after dkg-publisher.ts:3055 (caught in adversarial review) and the funds-enrichment probe (your round-1 finding). Three different files, one pattern: a line whose deletion leaves every suite green while silently breaking one lane. At three occurrences it is systemic to this change rather than incidental — every place the attested author gets threaded needed its own guard, and we found them one at a time instead of sweeping the class up front. Your example stated the failure precisely enough to act on directly.
There was a problem hiding this comment.
🔴 Bug: Funding reroute probe drops unreadable author-admission state
What's wrong
The main authorization gates carefully distinguish “policy denied” from “could not determine author admission because RPC failed.” The new funding reroute probe collapses that result to a boolean, so a transient lifecycle-version transport failure can be treated as proof that no admissible funded wallet exists.
Example
Signer A is pinned and short on TRAC; signer B is funded. Neither wallet is payer-authorized, but the attested author is authorized on a 10.1.7 lifecycle. If the version cache is cold or expired during enrichment and version() fails with ChainRpcTransportError, _cgPublishAdmits returns admitted: false with versionReadError for B. poolHasFundableSigner returns false, so the job is recorded as terminal NO_FUNDED even though B would be a valid reroute once RPC recovers.
Suggested direction
Return a richer result from poolHasFundableSigner, or rethrow/preserve versionReadError when no payer-authorized fundable signer was found, so a transient version-read failure does not become a terminal no-funded-wallet diagnosis.
For Agents
Update poolHasFundableSigner and/or enrichInsufficientPublisherFundsError to preserve the versionReadError distinction when the author branch is needed. Keep the no-author and payer-authorized behavior unchanged, and add a test with selected signer short, another signer funded, only the attested author authorized, and a ChainRpcTransportError from the lifecycle version read during enrichment.
There was a problem hiding this comment.
Confirmed. Real, accepted, and being fixed — and it is a hole created by the instruction that specified the earlier fix, which is worth stating plainly.
The previous round closed "a transient read is not a policy denial" at the two rejection sites. Your finding is that the third consumer still collapses it: poolHasFundableSigner reduces the admission to .admitted, so {admitted:false, versionReadError} is indistinguishable from "this wallet is not admissible". Your scenario reproduces exactly — pinned signer short, another funded, neither payer-authorized, author authorized on 10.1.7, version read fails transiently during enrichment ⇒ every wallet probes false ⇒ terminal insufficient_funds on a blip.
Why it was there: the fix instruction said the version helper must never throw because that probe consumes only .admitted. That constraint protected the probe and, in the same stroke, preserved the precise collapse being removed everywhere else. The lesson is about how the constraint was phrased — in terms of a mechanism ("must not throw") rather than the invariant it serves ("must not report inconclusive as denied").
The fix is smaller than your suggested direction, because the calling code already wants this behaviour. enrichInsufficientPublisherFundsError ends in catch { /* never let enrichment mask the original failure */ } return err;, and poolHasFundableSigner has exactly one production caller — that one, inside the try. So an inconclusive probe throwing yields precisely "preserve the original error so a retry can reroute", with no richer return type and no new plumbing.
One refinement on top of that, to make it provable rather than merely arguable: the throw does not happen inside the Promise.all map. The versionReadError is captured in a closure, the map settles normally, and the decision is made afterwards — so a different rejection still rejects first and cannot be masked, and with no versionReadError present the function is byte-for-byte what it is today.
Tests: the transport-failure row asserting the result is not InsufficientPublisherFundsError, plus the control you would want — author supplied but genuinely unauthorized, version readable ⇒ still terminal NO_FUNDED_PUBLISHER_WALLET. Without that control, "never emit NO_FUNDED" would pass the new row and quietly destroy the feature. Mutation-proven both ways.
There was a problem hiding this comment.
Fixed in d39ab18b9. Confirmed exactly as you described, including the scenario.
poolHasFundableSigner now captures versionReadError during the pool scan and throws it only when nothing was fundable and some admission was inconclusive. The sole production caller's existing catch { /* never let enrichment mask the original failure */ } return err turns that into "preserve the original funds error so a retry can reroute" — which is the behaviour your suggested direction asks for, reached without a richer return type, because the calling code already wanted it.
One refinement worth noting: the throw is not inside the Promise.all map. The error is captured in a closure, the map settles, and the decision is made afterwards. That makes three properties true by construction rather than by argument — a different rejection still rejects first and cannot be masked; with no versionReadError the function is behaviourally identical to before; and the throw fires only in the branch that would otherwise assert the whole pool is unfundable.
Two rows, mutation-proven. Removing the rethrow kills exactly one — "an INCONCLUSIVE admission during enrichment does not become terminal NO_FUNDED", 1 failed / 895 passed. The control (author supplied, genuinely unauthorized, version readable ⇒ still terminal NO_FUNDED_PUBLISHER_WALLET) stays green under that same mutation, which is what proves the new row is not simply "NO_FUNDED got weaker".
Worth recording why this existed, because it is the useful part. The instruction that specified the previous round's fix said the version helper must never throw because this probe consumes only .admitted. That constraint protected the probe and, in the same stroke, preserved the exact collapse being removed at the other two sites. It was phrased as a mechanism ("must not throw") rather than as the invariant it served ("must not report inconclusive as denied") — and a mechanism-shaped constraint protects the site you were looking at while silently licensing the same defect everywhere you were not.
Chain 894 → 896.
| // The revert argument stays `msg.sender` — the ABI is unchanged. The | ||
| // richer two-principal diagnosis is produced client-side, where | ||
| // operators actually read it. | ||
| if ( |
There was a problem hiding this comment.
💡 Suggestion: Extract the two-principal publish gate out of the busy lifecycle flow
Why it matters
The current placement makes the main lifecycle function harder to scan and keeps policy explanation mixed with transaction orchestration. A helper would make the behavioral change feel like a named policy, not an inline special case.
Suggestion
Move the authorization predicate into a small named private helper and put the rationale there. _executePublishCore would read as a direct sequence of operations, while the helper name carries the new model.
There was a problem hiding this comment.
Declining, with reasons rather than preference.
The comment block is the decision record. The rationale sitting directly above the gate documents that this supersedes — and does not revert — the N17 closure, which addressed a different axis (recovered node signer vs paying principal). "This reverts N17" is the most likely way a reviewer misreads this change, so the explanation is deliberately placed at the exact line a reviewer scrutinises. Moving it into a helper puts one indirection between the code and the argument for it.
Contract changes get zero CI on this branch. ci.yml is branch-filtered away from testnet-canary, and packages/evm-module/** is not in the RFC-64 path list — so the Solidity matrix, the ABI-freshness gate and the hardhat suites do not run here at all. That makes a no-behaviour-change refactor of the authorization gate strictly worse value than the same refactor on a branch where CI would catch a slip.
A named private helper is a legitimate improvement and I would take it on a PR targeting main, or in a follow-up cleanup once this has soaked. Recording the shape so it is not lost: _isPublishAuthorized(PublishParams calldata p) internal view returns (bool), with the rationale block moved onto it and _executePublishCore reduced to if (!_isPublishAuthorized(p)) revert ....
Not resolving — leaving it for a human to overrule if they disagree.
…hor-aware reroute probe (#1689) Two review findings from PR #1971. PublisherNotAuthorizedForContextGraphError accepted the structured diagnostic details, rendered a message from them, then dropped several. A consumer asking the very next question -- "is this chain simply not rotated yet, or was the author checked and refused?" -- had to parse `error.message` for facts the adapter had already computed exactly once. This PR exists to remove that coupling, so re-introducing it one question later was self-defeating. The error now carries the full (frozen) details object; `code`, the message contract and the flattened convenience fields are unchanged, so publisher's classifier and existing consumers are unaffected. The author-aware `poolHasFundableSigner` call in `enrichInsufficientPublisherFundsError` had no test. Dropping the author argument there would leave every existing test green while silently turning a recoverable author-authorized reroute into a terminal NO_FUNDED_PUBLISHER_WALLET -- the same shape as the dkg-publisher.ts forwarding line the adversarial review caught. Now covered, with a payer-only negative control proving the legacy path is unchanged. Chain unit suite 853 -> 857 passing. Closure build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| * `clearAgents` version gate (`evm-adapter-conviction.ts`), so a pre-versioned or | ||
| * oddly-formatted contract sorts BELOW every real release rather than throwing. | ||
| */ | ||
| function parseContractVersionTriple(raw: string): [number, number, number] { |
There was a problem hiding this comment.
🟡 Issue: Use one canonical deployed-contract version comparator
What's wrong
The PR adds a private semver-ish parser while explicitly documenting that it mirrors another deployed-contract version gate. Keeping the new helper private preserves the duplication instead of deleting it. Version gates are capability boundaries; duplicating the parsing policy makes future gates harder to audit and easier to get subtly inconsistent.
Example
The new helper says it intentionally matches the PCA clearAgents gate, but that gate still has its own inline parse/compare expression. The next deployed-contract capability gate now has two patterns to copy, and a small change in tolerance or comparison semantics can drift silently.
Suggested direction
Extract this into a shared helper and migrate the existing clearAgentsSupported gate to it in the same PR. The codebase should have one obvious way to ask whether a deployed contract version supports a capability.
For Agents
Move the version comparison into a shared chain utility, for example packages/chain/src/contract-version.ts, with parseContractVersionTriple and contractVersionAtLeast. Use it from this new KAL gate and from getPcaContractContext in evm-adapter-conviction.ts. Keep the existing tolerant behavior for malformed/pre-versioned contracts and keep the #1689 threshold tests green.
There was a problem hiding this comment.
Fixed in db0c322fe. Accepted specifically because this PR created the duplication — the new helper's own doc said it mirrored the PCA gate, which is exactly the second-copy-to-copy-from problem you describe.
packages/chain/src/contract-version.ts now holds parseContractVersionTriple + contractVersionAtLeast; both the #1689 gate and clearAgentsSupported use it. Verified no inline version parsing remains anywhere in packages/chain/src.
Migrating a live capability gate on a branch with no CI needed proof rather than confidence, so the equivalence is executable: test/contract-version.unit.test.ts reproduces the pre-migration PCA expression verbatim, including its own parse — deliberately not sharing the new parse, since a shared parse would make the assertion circular — and asserts agreement across 20 named cases plus an exhaustive 13×4×9 grid. Zero divergences, including '', '10.0', garbage, 10.0.6-rc.1, and negative components like 10.-1.0.
The agreement is also structural rather than coincidental: both use the identical parseInt(n,10) || 0 parse, and for minimum = '10.0.6' the generic comparator's branches reduce exactly to the PCA expression. The grid confirms it; the reduction explains it.
Mutation-proven on the shared comparator, which is now a single point of failure for two capability gates:
pat >= minPat → pat > minPat → 14 failed | 33 passed (both gates + the equivalence matrix)
lexical string compare fallback → 8 failed | 39 passed
The first is the one that matters: an off-by-one in the shared comparator moves the #1689 and PCA boundaries simultaneously, and it is now loud in both rather than silent in either.
The PCA threshold is exported as CLEAR_AGENTS_MIN_PCA_VERSION so the equivalence test couples to the production value instead of re-typing '10.0.6'.
… comparator, single-source error facts (#1689) Three review findings from PR #1971 round 2. createKnowledgeAssets passes params.author?.address into its authorization gates on both the pinned and unpinned paths, and neither was tested. Removing that threading left the planner tests, contract tests and publisher forwarding tests all green while the direct/sync publish path pre-rejected whenever only the author was authorized. This is the third instance of the same shape on this PR, after dkg-publisher.ts:3055 and the funds-enrichment probe. Now covered through the public method on both paths, with a neither-principal-authorized control so the positives cannot pass by the gate being skipped entirely; each call site is mutation-proven independently. The deployed-contract version comparator this PR introduced explicitly documented that it mirrored the PCA clearAgents gate, leaving two copies of a capability-boundary policy that drift silently. Extracted to contract-version.ts and both gates migrated; no inline version parsing remains in packages/chain/src. The equivalence is proven executably: the test reproduces the pre-migration PCA expression verbatim, including its own parse so the assertion is not circular, and asserts agreement across 20 named cases plus an exhaustive grid. Mutating the shared comparator now fails both gates at once, which is the point. PublisherNotAuthorizedForContextGraphError stored the diagnostic facts twice. The flattened fields are now getters delegating to `details`, so there is one source of state. Verified first that no consumer spreads or JSON-serializes the error, since getters are not own enumerable properties. Found while mutation-testing that last fix: hard-coding a flattened accessor to ignore `details` left 47 tests passing. The fix for "two representations can disagree" was itself unverified. Assertions added that each accessor equals its `details` counterpart on both error shapes; the same mutation now fails 2. Chain unit suite 857 -> 885 passing. Publisher 604, closure build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| * dotted integers by convention. | ||
| */ | ||
| export function parseContractVersionTriple(raw: string): [number, number, number] { | ||
| const parts = String(raw).split('.').map((n) => parseInt(n, 10) || 0); |
There was a problem hiding this comment.
🟡 Issue: Do not make capability gates inherit a lenient parseInt version parser
What's wrong
The new shared comparator is positioned as the one capability-gate boundary, but it has no explicit parse-failure state and partially parses malformed values. That makes the contract of the helper harder to reason about: future gates will look fail-closed by name and comments, while actually accepting some malformed values as newer releases.
Example
contractVersionAtLeast('11.garbage', '10.1.7') evaluates as true because the major component parses as 11 and the malformed minor becomes 0. contractVersionAtLeast('10.1.7junk', '10.1.7') also evaluates as true because parseInt truncates the suffix. That is much looser than a major.minor.patch contract literal boundary reads.
Suggested direction
Use an explicit VersionTriple | null parser with a full-string match for contract _VERSION literals, then have contractVersionAtLeast fail closed on parse failure. If there is a real need for legacy PCA equivalence on malformed strings, keep that as a local compatibility shim rather than the canonical comparator.
For Agents
In packages/chain/src/contract-version.ts, introduce a strict parser that returns null unless the whole value is a dotted integer triple, and make capability gates return false on null. If preserving the old PCA comparator’s lenient behavior is required, isolate that compatibility under an explicitly named helper instead of making every future capability gate inherit it.
There was a problem hiding this comment.
Fixed in 65c9bbb96. Accepted on the principle, not the practical risk — and the distinction is worth stating.
parseContractVersion is now a full-string dotted-integer match returning VersionTriple | null, and contractVersionAtLeast fails closed on null. Both capability gates inherit that. Your examples are the reason: '11.garbage' → 11.0.0 and '10.1.7junk' → 10.1.7 both resolved to enable, and a parser that turns malformed input into a confident "yes" is precisely the wrong default for a capability boundary.
The practical risk today is ~zero — every _VERSION is a hardcoded literal — but that is an argument about the current two callers, not about the helper. This is the one canonical comparator future gates will reuse, and leniency inherited by default is exactly what makes a boundary quietly wrong later.
On the PCA equivalence this deliberately breaks: last round's proof rested on both comparators being lenient in the same way, so going strict is a real behaviour change for the PCA gate on malformed input. The equivalence test was updated to assert the divergence deliberately rather than deleted — it now documents that strict and lenient agree on every well-formed M.m.p and differ only on inputs no deployed _VERSION can produce. Deleting it would have removed the record of where old and new part company, which is the thing most worth keeping.
| const selectedPlan = await this._withSignerSelection(async (ordered) => { | ||
| const authorized = await this._authorizedPublisherSigners(ordered, request.contextGraphId); | ||
| const authorized = await this._authorizedPublisherSigners( | ||
| ordered, request.contextGraphId, request.attestedAuthorAddress, |
There was a problem hiding this comment.
🟡 Issue: Unpinned publish-plan author admission is not covered through the public planning API
What's wrong
The diff adds author-aware admission to the unpinned adapter-owned planning path, but the tests do not appear to drive resolvePublisherPublishPlan without a pinned publisher while only the attested author is authorized. That leaves the exact RHS threading at this line unverified for a user-facing planning path.
Example
A regression that changes line 157 to this._authorizedPublisherSigners(ordered, request.contextGraphId) would still leave the protected-helper and direct-publish tests green, but resolvePublisherPublishPlan({ contextGraphId, attestedAuthorAddress: AUTHOR, ... }) with no publisherAddress would reject when only AUTHOR is authorized.
Suggested direction
Cover the unpinned resolvePublisherPublishPlan branch directly so the request-to-helper threading is regression-tested, not only the helper behavior itself.
For Agents
Add a public adapter-owned plan test in packages/chain/test/evm-adapter.unit.test.ts or evm-adapter-publish-admission.unit.test.ts: use a funded multi-wallet adapter, supported lifecycle version, authorize only AUTHOR, call resolvePublisherPublishPlan without publisherAddress but with attestedAuthorAddress, and assert it resolves with a wallet from the pool. Keep a negative row without the author or below threshold if useful.
There was a problem hiding this comment.
Already covered — verified by mutation rather than by reading. No change made.
Your confidence note says you did not run the suite and reasoned from the diff and the checked test files. That is a fair method, and it is why I checked empirically instead of taking either view on faith. Dropping request.attestedAuthorAddress from evm-adapter-publish.ts:157 and running the full chain suite:
× UNPINNED pool: an authorized author admits the pool for funding-aware selection
Tests 1 failed | 882 passed
Exactly one test dies, and it is a test of the unpinned planner branch — so the RHS threading at that line is regression-tested today. The coverage arrived in db0c322fe, which also added the createKnowledgeAssets unpinned case; both drive the same pool-admission path.
This is a real distinction rather than pedantry: reading a test file tells you a test exists nearby; only removing the code tells you a test binds. That difference has cut both ways on this PR — a test we believed was coverage turned out vacuous under mutation last round, and here a line you reasonably read as uncovered turns out guarded.
Leaving the thread open for you to disagree if you think the covering test exercises a different path than you intended.
There was a problem hiding this comment.
Retracting my previous reply on this thread. The measurement I cited is not valid, and I would rather say so than leave it standing.
I reported that dropping request.attestedAuthorAddress from :157 killed exactly one test, and concluded the line was already guarded. That run happened while a mutation sweep was concurrently rewriting the same files — a long-running script that was still active despite appearing finished. Any sample taken during that window cannot distinguish "this line is guarded" from "some other mutation was live at that instant". So the result is unattributable and I am withdrawing it, not defending it.
This line is now recorded as unmeasured, and is being re-measured cleanly in an isolated window. I will post the real result here either way — including if it turns out you were right and it is genuinely unguarded.
Two things worth stating plainly, since they bear on how much weight to give the rest of the evidence in this PR:
The same sweep has already found one real gap you did not flag — createKnowledgeAssets → enrichInsufficientPublisherFundsError. Removing that threading leaves all 883 chain tests passing. It is the same shape as your finding here: a test that exercises the inner function directly pins the inner link and looks like it covers the chain.
Your review method was sound and mine was not. You reasoned from the diff and said so in your confidence note. I ran a measurement and trusted it without checking whether the tree was stable underneath it — which is a worse error, because it produces a number that looks like proof.
Everything else in this PR that is claimed as mutation-proven was measured in a stable tree; this thread's claim was the exception, and it is now withdrawn until re-measured.
There was a problem hiding this comment.
Re-measured cleanly: you were right. The line was unguarded. Fixed in c1fd3c50b.
On a quiet tree, in an isolated single-command window, dropping request.attestedAuthorAddress from :157:
884 passed, nothing died
Nothing bound it. My earlier "1 test dies, therefore guarded" reading was an artifact of a concurrent mutation sweep rewriting the same files — which is why I withdrew it rather than defend it. Your original analysis, reasoned from the diff without running anything, was correct; my measurement was the thing that was wrong.
The reason the pinned branch looked like coverage: :134 (pinned) and :157 (pool) are separate call sites, and the existing planner tests only exercised the pinned one. Exactly the distinction your example drew.
Now covered by a test driving the public resolvePublisherPublishPlan unpinned with only the author authorized, asserting a plan resolves. Mutation-proven: removing the threading kills that test and nothing else.
Your finding also triggered the thing that closed the whole class. Rather than fix a fifth instance individually, every site where the attested author is threaded from a public entry point into an admission decision was enumerated — 11, derived by tracing call chains rather than grepping — and each was mutation-measured. Nine were already guarded. Two were not: this one, and createKnowledgeAssets → enrichInsufficientPublisherFundsError, which nobody had flagged and which no review round would plausibly have caught, since it also passed with the entire suite green.
The rule that explains all five instances, and now applied everywhere:
For pass-through threading, the only binding test asserts the value at the far end of the chain, through the public entry point. Testing an inner hop proves the hop works — not that anything calls it.
The old enrichment test called the helper directly, which is precisely why it bound its own hop and left its caller open. Chain suite 883 → 885.
…ersion parser (#1689) Two review findings from PR #1971 round 3. PublisherNotAuthorizedForContextGraphError still took `message` and `details` as separate constructor arguments, so a caller could build an error whose text did not describe its own facts -- and our own tests did exactly that. The constructor now takes `details` alone and derives the message internally, making that state unconstructible rather than merely untested. The shared contract-version comparator inherited a lenient parseInt parse: `contractVersionAtLeast('11.garbage', '10.1.7')` returned true, because the major component parsed and the malformed remainder read as zero. Harmless today, since every _VERSION is a hardcoded literal, but this is the one canonical helper future capability gates will reuse, and a parser that turns malformed input into a confident "yes" is the wrong thing for a capability boundary to inherit. Now a full-string dotted -integer match returning null, with gates failing closed on null. Chain 883 passing, publisher 604, closure build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| // state falls back to the classifier. | ||
| const isPublisherNotAuthorizedForCg = errorCode === PUBLISHER_NOT_AUTHORIZED_FOR_CG_CODE | ||
| || messageIndicatesPublisherNotAuthorizedForCg(lower); | ||
| const code = isNoFundedWallet && input.failedFromState === 'broadcast' |
There was a problem hiding this comment.
🟡 Issue: Replace the growing classifier ternary with an explicit ordered rule boundary
What's wrong
This mapper was already a classification boundary; the PR adds another broadcast-only exception directly into the nested ternary. The result is branch-order coupling hidden in expression syntax, with comments carrying the design instead of the code structure. This is exactly the kind of special-case growth that will become brittle as more terminal publish-plan failures are added.
Example
The new authorization branch has to sit after NO_FUNDED_PUBLISHER_WALLET but before quorum/default classification. That priority is not represented as data or named control flow; it is implicit in a nested ternary, so the next terminal broadcast error will likely add another boolean and deepen the branch chain.
Suggested direction
Extract the broadcast-only overrides into a small ordered helper or rule table, then fall back to classifyPublishFailureCode. That keeps the ordering intentional and removes the ad-hoc conditional growth from the main mapper.
For Agents
Refactor mapPublishExceptionToLiftJobFailure in packages/publisher/src/async-lift-publish-result.ts. Preserve the current classification priority and state restrictions. A simple classifyBroadcastOverride(...) with early returns, or an ordered rule table of { matches, code }, would make the special cases explicit and keep future codes from growing this ternary.
There was a problem hiding this comment.
🟡 Issue: Avoid growing the publish failure classifier by special-case branches
What's wrong
This change adds another ad-hoc branch to a classifier that is already accumulating special cases. The structure works today, but it makes every new permanent publish failure a local edit to the same conditional chain instead of data in a clear mapping.
Example
Both special cases follow the same shape: errorCode === SOME_CODE || messageIndicatesSomeMarker(lower), gated by failedFromState === 'broadcast', then mapped to a terminal failure code. Adding the next structured publish error would require another boolean and another ternary arm.
Suggested direction
Replace the repeated boolean-plus-ternary pattern with a compact dispatch table for structured broadcast-phase failure codes.
For Agents
In async-lift-publish-result.ts, introduce a small ordered table for structured broadcast overrides, for example entries containing matches(errorCode, lowerMessage) and failureCode. Keep quorum handling and generic classification order intact, and prove no-funded, publisher-not-authorized, included-phase fallback, and quorum cases still map as they do now.
There was a problem hiding this comment.
Validated (raised twice — answering both here). Declining the restructure; accepting the ordering point as a real, currently-unpinned property.
Your ordering observation is correct and is the part worth keeping: the ternary chain's branch order carries semantics. The authorization branch sits between the funded-wallet branch and the quorum branch, and each is guarded on failedFromState === 'broadcast', so a reordering could silently shadow one case with another. Nothing currently asserts that ordering — the branches are individually tested, the sequence is not. That is the same "a change nothing notices" shape this PR has spent four rounds closing, and it is a fair hit.
Declining the rule-table rewrite for the reason four other restructurings have been declined on this branch: it is behaviour-neutral by your own framing, on a diff where ci.yml does not run, against a classifier that is currently mutation-proven (removing each branch kills exactly its own test and nothing else). Swapping a verified arrangement for a better-shaped unverified one is the wrong trade here, not in general.
Recorded with the ordering point attached, since a rule table's main benefit is precisely that it makes the precedence explicit and testable — which is the argument for doing it properly on a branch with CI rather than partially here.
There was a problem hiding this comment.
🟡 Issue: Replace the growing failure-classifier ternary with an explicit rule table
What's wrong
This PR adds another special case to an already dense nested classifier and then adds a second post-classification correction path. The implementation still works, but the structure makes precedence hard to audit and encourages future failures to be handled by appending more conditional branches.
Example
The current flow checks no-funded, then publisher-authz, then quorum, then calls classifyPublishFailureCode, then optionally rewrites tx_reverted/confirmation_mismatch if the original error is a chain transport error. Adding the next structured error will require threading another precedence rule through the same chain.
Suggested direction
Use an ordered FailureRule[] or a small dispatcher that operates on { code, lowerMessage, failedFromState, isTransportError }. That keeps precedence local and makes structured-code rules distinct from message-text fallback rules.
For Agents
Refactor packages/publisher/src/async-lift-publish-result.ts. Preserve existing outputs for no-funded, authority-forbidden, quorum, timeout, revert, mismatch, and transport cases. Add focused tests around rule precedence after converting the classifier to ordered rules or explicit pattern handlers.
There was a problem hiding this comment.
Repeat of the round-4 finding, and the answer is unchanged — but the ordering half is worth restating because it is the part I agree with.
Branch order in that ternary carries semantics: the authorization branch sits between the funded-wallet branch and the quorum branch, each guarded on failedFromState === 'broadcast', and nothing asserts the precedence. A reordering could silently shadow one case with another. That is a genuine unpinned property.
This round added a transport-code branch to that same chain, so the case for a rule table is marginally stronger than it was. Still declining it here: it is behaviour-neutral, on a branch where ci.yml does not run, against a classifier where every branch is mutation-proven to kill exactly its own test. Swapping a verified arrangement for a better-shaped unverified one is the wrong trade in a converging PR — not in general.
Recorded with the ordering point attached, since making precedence explicit and testable is the main thing a rule table buys.
There was a problem hiding this comment.
🟡 Issue: Failure classification is becoming order-dependent spaghetti
What's wrong
The new classification rules are layered onto an existing branch chain, making policy precedence implicit in expression nesting. This is exactly the kind of special-case growth that makes a failure mapper hard to audit: terminal/retryable classification is an operational policy, but the implementation reads as incidental control flow.
Example
Adding the next structured chain failure requires knowing whether it belongs before quorum, before generic text classification, or after the transport correction. The current nested ternary does not make those precedence rules explicit; they live in comments and branch order.
Suggested direction
Replace the nested ternary and after-the-fact transport patch with an explicit ordered classifier. A small list of named guards would make precedence reviewable and would keep new failure contracts from being bolted into the middle of an already dense expression.
For Agents
In packages/publisher/src/async-lift-publish-result.ts, preserve existing mappings and allowed-state behavior. Refactor mapPublishExceptionToLiftJobFailure into named ordered rules or a small classifier pipeline, with each rule returning LiftJobFailureCode | undefined; keep transportSafeClassification as one explicit rule/normalization step with tests covering precedence.
There was a problem hiding this comment.
Filed as #1974. Fourth framing of this request, so tracking it rather than declining a fourth time.
The ordering point is right and I want it recorded as the reason, not as a concession. Precedence in that chain is real policy — no-funded-wallet → publisher-not-authorized → quorum-unmet → generic message-text → transport correction — and it is expressed only through expression nesting and comments. The individual branches are each mutation-proven; the order between them is asserted nowhere. That is the same "a change nothing notices" shape this PR spent several rounds closing elsewhere, which makes it a genuine finding rather than a style preference.
Still declining it here, for the reason four other restructurings have been declined on this branch: behaviour-neutral, no CI (ci.yml is filtered away from testnet-canary), and every branch currently kills exactly its own test under mutation. Swapping a verified arrangement for a better-shaped unverified one is the wrong trade in a converging PR.
#1974 carries the invariants a refactor must not break — structured-code matching before message text; the transport correction suppressing only tx_reverted/confirmation_mismatch and only from 'broadcast' (from 'included' the transaction was mined, and rpc_unavailable is not a legal code there, so an ungated version throws); timeout handling staying out of the transport rule or #1851's double-submit hole re-opens; and isDefinitivePreAcceptanceSendFailure staying decoupled.
It also carries the sequencing point your finding implies: add the precedence tests first. They pin the property the current implementation leaves unpinned, and they are what makes the restructure verifiable rather than merely tidier.
There was a problem hiding this comment.
🟡 Issue: Flatten the publish-failure classifier before adding more special cases
What's wrong
This change adds another branch to an already string-driven classifier by extending the nested ternary and bolting on a post-filter. The behavior may be right, but structurally the function is becoming a precedence puzzle: permanent authority failures, funded-wallet failures, quorum failures, generic text matching, and transport overrides are all mixed in one expression.
Example
Adding another permanent broadcast-phase error now requires editing the boolean prelude, finding the correct slot in the nested ternary, checking LIFT_JOB_FAILURE_ALLOWED_STATES, and deciding whether transportSafeClassification should apply. The structure makes precedence an emergent property of expression nesting rather than a readable policy.
Suggested direction
Make the classifier an explicit ordered decision table or straightforward if chain with named predicates. That would delete the need for most precedence commentary and make future failure classes local additions instead of nested-expression surgery.
For Agents
Refactor packages/publisher/src/async-lift-publish-result.ts around mapPublishExceptionToLiftJobFailure. Preserve all current codes and state gates, but replace the nested ternary with early returns or an ordered list of classifier rules over shared readPublishErrorFacts plus structured predicates. Keep transport downgrading as a named rule in that ordered flow.
| // The REAL planner, by source path — `PublisherPlanner` is not re-exported from | ||
| // the publisher package index. Cross-package `../../<pkg>/(src|test)/` imports | ||
| // are established in this suite (see `agent.shared.ts`). | ||
| import { PublisherPlanner } from '../../publisher/src/publisher-planning.js'; |
There was a problem hiding this comment.
🟡 Issue: Do not add another cross-package source import for publisher internals
What's wrong
The test reaches from the agent package into the publisher package's src tree because PublisherPlanner is not exported. That is a boundary leak: it makes internal publisher structure part of the agent test contract and normalizes source-path coupling. The PR already adds a publisher-owned test for the real handoff, so this extra cross-package dependency looks avoidable.
Example
A private refactor that renames or moves publisher-planning.ts can now break the agent package even if the publisher package public API remains stable. The new publisher-side threading test already exercises the real DKGPublisher.publish() path and records the chain plan request, so this agent test does not need to depend on the publisher's internal file layout.
Suggested direction
Keep package-private planner tests in the publisher package, and let the agent suite verify agent-owned author resolution through its public flow. That preserves the package boundary and avoids coupling agent tests to publisher internals.
For Agents
Move the planner handoff assertion fully into packages/publisher/test or exercise it from the agent through a public publisher/agent boundary. If direct planner testing is genuinely needed cross-package, expose an intentional test/internal entry point instead of importing from ../../publisher/src.
There was a problem hiding this comment.
Validated. Declining — the import is consistent with established precedent in this repo, and removing it would weaken what the test proves without buying a boundary that is actually enforced anywhere.
Checked rather than assumed: there is no tsconfig path restriction, eslint rule, knip config or package.json exports constraint that this violates, and cross-package test imports of this shape already exist in several files. The test exists to pin GH#1778 (a curator publishing a member-authored KA) together with the #1689 author threading, and the import is what lets it assert against the real types rather than a local re-declaration that could drift from them silently — which is the failure mode this PR has spent four rounds closing.
A smaller point inside your finding does survive: the accompanying comment block is longer than it needs to be and could be trimmed without touching the import. Not worth a round on its own; noted for whenever that file is next edited.
| const v = String(await logic.version()).split('.').map((n) => parseInt(n, 10) || 0); | ||
| const [maj, min, pat] = [v[0] ?? 0, v[1] ?? 0, v[2] ?? 0]; | ||
| clearAgentsSupported = maj > 10 || (maj === 10 && (min > 0 || (min === 0 && pat >= 6))); | ||
| clearAgentsSupported = contractVersionAtLeast( |
There was a problem hiding this comment.
🟡 Issue: The PCA clearAgents gate is only tested at the helper level, not at the public context method
What's wrong
The PR changes the user-facing wallet-connect capability flag, but the added coverage only proves the shared comparator’s behavior. It does not prove the changed public method is wired to that comparator, the production threshold, or the deployed contract's version read, so a wiring regression would still pass green.
Example
A regression such as passing the wrong minimum version, hard-coding clearAgentsSupported to false, or accidentally skipping logic.version() in getPcaContractContext would not be caught by the new comparator-only tests. A focused test could stub resolveContract('PublishingConviction').version() to return '10.0.6', '10.0.5', a malformed string, and throw, then assert getPcaContractContext().clearAgentsSupported is true/false as expected.
Suggested direction
Add a getPcaContractContext regression test that verifies the method actually reads the deployed logic version and returns the expected clearAgentsSupported flag across the threshold and failure cases.
For Agents
Look at packages/chain/src/evm-adapter-conviction.ts getPcaContractContext. Add a unit test that drives the public method with stubbed NFT/token/logic contracts and proves the returned clearAgentsSupported follows the deployed PublishingConviction version, including fail-closed unreadable/malformed cases.
There was a problem hiding this comment.
Accepted and fixed in 81cdc6954 — test-only, and accepted specifically because this PR changed that gate's live behaviour when it migrated clearAgentsSupported onto the shared strict comparator.
Your framing was right: with coverage only over the comparator, a regression in how the flag is wired — as opposed to how versions compare — would have been invisible. Mutation-reasoned it before accepting: hardcoding the flag false would have failed nothing.
Now driven through the public consumer rather than the comparator, asserting the flag below, at, and above the threshold plus the unreadable case. Mutation-proven: hardcoding the flag kills the new rows and nothing else.
This is the same defect shape the PR spent four rounds closing for author threading — a line whose deletion nothing notices — sitting on a gate we touched. Worth the ~25 lines.
Author-threading coverage — the full enumerationReview surfaced the same defect shape five times across four rounds and three files: a line whose deletion leaves every suite green while silently breaking one lane. Fixing them individually was clearly not converging, so every site where the attested author is threaded from a public entry point into an admission decision was enumerated and each was mutation-measured — threading removed, full suite run, result recorded. The list was derived by tracing call chains, not by grepping the identifier (a raw grep gives ~120 hits, dominated by seal fields, receipt parsing and the update path, and would have been longer while covering less).
Nine were already guarded. Two were not — and only one of those two had been flagged by review. The rule this produced
That explains every instance. The previous enrichment test called the helper directly, so it bound its own hop (S4) and left its caller (S3) open while looking like chain coverage. Conversely one end-to-end publisher test binds S9, S10 and S11 with a single assertion, because it checks the value where it lands. Method notes, since the numbers depend on them
Chain unit suite 883 → 885; publisher 604; closure build clean. One flaky test to be aware of
|
| lifecycle, 'knowledgeAssetsLifecycle.version', 'version', | ||
| ), | ||
| )); | ||
| } catch { |
There was a problem hiding this comment.
🔴 Bug: Transient lifecycle version read failures become terminal authorization failures
What's wrong
The new client-side gate needs KnowledgeAssetsLifecycle.version() to decide whether the attested author can be consulted. When that read fails for a transient RPC reason, the code returns null and later emits the same structured authorization error used for a real policy denial. The publisher then marks the job terminal, so a recoverable infrastructure blip can permanently fail a publish that the chain would accept.
Example
Scenario: payer P is not authorized, attested author A is authorized, and the deployed KAL is 10.1.7. If the first version() eth_call times out, _cgAdmitsAttestedAuthor never checks A, resolvePinnedPublisherSigner throws PUBLISHER_NOT_AUTHORIZED_FOR_CG, and the async publisher records a non-retryable authority_forbidden job. A retry after the RPC blip would have read 10.1.7 and the publish would have been accepted.
Suggested direction
Keep the author branch fail-closed for planning, but carry a distinct reason for version() read failure or throw a retryable chain/RPC error instead of PublisherNotAuthorizedForContextGraphError. Only map confirmed policy denials or confirmed unsupported deployed versions to terminal authority_forbidden.
For Agents
Look at deployedKnowledgeAssetsLifecycleVersion, _cgAdmitsAttestedAuthor, and mapPublishExceptionToLiftJobFailure. Preserve fail-closed behavior for unrotated/unsupported lifecycles, but do not classify transport/read failures as permanent authorization failures. Add a test where version() throws once then succeeds through the async publish failure mapper/job lifecycle, proving the first failure remains retryable or otherwise distinguishable from a real policy denial.
There was a problem hiding this comment.
Confirmed and fixed in 81cdc6954. Your causal chain held at every step, and validating it turned up a second half you did not reach.
The defect, crisply: inside one predicate, two reads answering the same question had opposite failure semantics. A transport failure on the payer read propagated and classified retryable; the same failure on the version read was swallowed into a policy-denial verdict and went terminal. PUBLISHER_NOT_AUTHORIZED_FOR_CG is supposed to mean "policy denied" — it was also being used for "I could not determine whether policy denies", and only the second is unsafe to call terminal.
One correction to the framing, in the PR's favour: this is not a regression. Pre-PR, that same publish failed anyway — it looped to maxRetries without ever succeeding. Nothing that used to work now breaks; it is an incompletely-delivered improvement. Hence fixed, but not treated as blocking.
Your suggested fix would have broken the failure-recording path. authority_unavailable is retryable, but its allowed-states are ['claimed','validated'] — emitting it from 'broadcast' makes createLiftJobFailureMetadata throw, stranding the job in 'broadcast'. Flagging it because the suggestion reads plausibly and the whitelist is easy to miss.
What shipped instead, no new error code and no whitelist edit: the version helper reports why it could not read, discriminated on isChainRpcTransportError — deliberately not isRetryableRpcError, which counts BAD_DATA as retryable, and BAD_DATA is precisely the "this lifecycle has no version() to decode" case that must stay terminal or the forever-retry trap this PR closes re-opens. The transport error is rethrown at the two rejection sites only, never from the helper: _authorizedPublisherSigners probes the author before the payer loop, and poolHasFundableSigner consumes only .admitted inside the NO_FUNDED enrichment probe, so a throwing helper would fail a payer-authorized publish and corrupt the insufficient-funds classification.
The half your analysis stopped short of — and it mattered. Fixing the chain side alone was necessary and insufficient. RPC_ENDPOINTS_EXHAUSTED splices the underlying node text into its message, and missing revert data — the standard response for a failed eth_call, which is exactly what version() is — classified as tx_reverted: terminal. Same job death, one layer down, from the same 503. Measured, not reasoned:
| underlying RPC text | code | outcome |
|---|---|---|
connection refused |
rpc_unavailable |
retryable |
missing revert data |
tx_reverted |
terminal |
chain id mismatch |
confirmation_mismatch |
terminal |
So the publisher now suppresses exactly those two outcomes for a transport-coded error, on the by-construction argument that a proven transport failure means no transaction was mined. Timeout handling is deliberately untouched — a genuine RPC_TIMEOUT during send must keep tx_submit_timeout → check_chain_then_finalize_or_reset, or #1851's double-submit hole re-opens.
Mutation-proven on both sides; the two rows that matter fail before the fix and pass after. Chain 885 → 894, publisher 604 → 608.
| // alongside the paying wallet, so the planner must see it BEFORE it picks | ||
| // (and rejects) a signer. Already resolved on `options` from the start of | ||
| // publish(); this is a pass-through, never a re-resolution. | ||
| attestedAuthorAddress: options.precomputedAttestation?.authorAddress, |
There was a problem hiding this comment.
🔴 Bug: Planner trusts the seal author before validating the seal
What's wrong
The new author-aware planning path uses an unverified precomputedAttestation.authorAddress as an authorization principal. Because the existing seal-integrity preflight happens later, a forged or corrupt seal can get past signer planning and trigger encryption/ACK work before it is rejected.
Example
A caller whose payer wallet is not authorized for a curated CG can submit a bogus precomputedAttestation with authorAddress set to the CG authority and an invalid signature. The planner can admit the request using that unverified author, then encryptInlineChunked and the ACK provider can run before the later signature mismatch rejects the publish. Expected behavior is to reject the bogus seal before using its author as an authorization principal or before staging data with peers.
Suggested direction
Validate precomputedAttestation.expectedMerkleRoot and the author signature before passing its authorAddress to the authorization planner, or pass an author only after it has been proven. If chain id / KAL address are needed for validation, resolve them before planning on the on-chain path.
Confidence note
This depends on whether callers outside the trusted agent path can supply precomputedAttestation; the public PublishOptions type and hosted publisher flow appear to allow that, but deployments may restrict access at a higher layer.
For Agents
Move or factor the publish seal validation so the author address is verified before publisherPlanning.finalize receives it, at least for on-chain publishes. Preserve the existing missing-seal handling for intentional local publishes. Add a test with unauthorized payer, authorized claimed author, invalid seal signature, and an encryption/ACK spy proving no staging hooks are invoked.
There was a problem hiding this comment.
Validated independently. The ordering claim is exactly right; the security claim does not hold. Classifying this as defence-in-depth rather than a bug, with reasoning rather than assertion.
Confirmed: dkg-publisher.ts:3055 does pass precomputedAttestation.authorAddress into planning, and the seal-integrity preflight — typed-data build, expectedMerkleRoot compare, signature recover-and-compare — runs later, around :3641-3705. So the client-side admission decision is made on an unverified claim. That part is real.
Refuted — it is not an authorization bypass, and no forged seal can publish. The on-chain gate verifies the EIP-712 attestation at _verifyAuthorAttestation before anything else in _executePublishCore, and reverts on a forged author. The client gate is an optimisation that fails fast; it is not the boundary. A forged seal that slipped past planning still cannot produce a publish.
So the real cost is wasted work, not exposure: encryption and ACK collection could run before the forged seal is rejected. Worth noting the attacker pays for that in their own node's compute and their own peers' patience, and gains nothing.
On the suggested fix. The smallest correct form is not new validation logic — it is relocating the existing preflight above publisherPlanning.finalize, together with the v10ChainId/v10KavAddress resolution it depends on. The guards turn out to be exactly equivalent (canAttemptOnChainPublish === (state.kind !== 'local') reduces to the same predicate as the existing if/else if/else chain), so intentional local publishes would still skip it.
Declining it for this PR, and recording why. It relocates ~65 lines through the most delicate function in the publisher, on a branch where ci.yml does not run — the same reason four other restructurings have been deferred here. The property it buys is "a forged seal wastes less of the attacker's own resources", which does not justify that risk profile against a boundary that is already sound. The missing-seal check at :3743-3750 also must not move, since a test pins that planning runs first for a seal-less publish — which is a good illustration of why this is not the free relocation it looks like.
Recorded for follow-up with the exact shape and the guard-equivalence proof, so whoever picks it up does not have to re-derive either.
| * is not an authorized publisher" for an author that was never consulted, sending | ||
| * an operator to rewrite a publish policy over a transient RPC fault. | ||
| */ | ||
| export interface CgPublishAdmission { |
There was a problem hiding this comment.
🟡 Issue: Model publish admission as states, not boolean/null combinations
What's wrong
The new result type hides several distinct states behind booleans and nullable fields. That is why the surrounding comments are so long: readers have to remember which combinations are meaningful and which fields were skipped for performance or compatibility. This is exactly the kind of capability boundary that benefits from a stronger model.
Example
The same-payer shortcut returns { admitted: false, authorConsulted: true, deployedVersion: null }, while an unreadable lifecycle also carries a null version but means a different diagnostic state. The distinction is preserved by convention and comments, not by the type.
Suggested direction
Use an explicit discriminated union for admission outcomes and rejection reasons so downstream formatting does not reconstruct intent from loosely related flags.
For Agents
Replace CgPublishAdmission with a discriminated union such as admitted by payer|author|no-context-graphs and rejected because no-author|same-principal|unsupported-version|unreadable-version|author-refused. Update PublisherNotAuthorizedForCgDetails and formatPublisherNotAuthorizedForCgMessage to switch on that explicit reason. Preserve current message behavior with focused unit tests for each union case.
There was a problem hiding this comment.
Validated. Declining the union rewrite as raised — but you identified the structural cause of the 🔴 on the version gate, and that half is being fixed.
The type does carry an ambiguity, and it is exactly the one that matters: deployedVersion: null currently means three different things — no lifecycle bound, version read failed transiently, and version present but unparseable. The first and third are correctly terminal. The second is not, and collapsing it into the same value is precisely why a transient RPC failure became a permanent authority_forbidden.
So this is not a style finding — it is the same defect as the red one, seen from the type side.
What is shipping is the single-bit form rather than the full state union: one optional field carrying why the version was unreadable, set only on that branch, consumed only where the admission was already about to become a terminal rejection. That resolves the ambiguity where it causes harm, without a rewrite of a type whose exact shape is pinned by an existing toEqual assertion.
Declining the broader union for the reason four other restructurings have been declined here: no behaviour change, no CI on this branch, and the current arrangement is mutation-proven. The modelling argument is sound and is recorded in the follow-up alongside the extraction work — the two belong together, since a policy object would naturally own this type.
Credit where it is due: this was raised as a modelling concern and turned out to be load-bearing. That is a better outcome than the finding was framed as.
There was a problem hiding this comment.
🟡 Issue: Model publish admission as a discriminated decision instead of a nullable boolean bag
What's wrong
The PR adds a complex capability gate, but represents its state with loosely-related booleans and nullable fields. That makes the contract between helpers implicit and forces each caller to know which combinations mean policy denial, unsupported lifecycle, no author, no ContextGraphs, or inconclusive RPC. This is exactly the kind of branching-heavy state model that will be hard to extend safely for the next publish-policy change.
Example
deployedVersion: null now means several different things depending on the call path: payer already admitted, no ContextGraphs surface, no lifecycle bound, or a failed version read. The identical-author shortcut also returns authorConsulted: true with no author read and no deployed version. Those combinations are subtle enough that each consumer has to carry its own explanatory branching.
Suggested direction
Introduce a small CgPublishAdmissionPolicy or pure decision helper that owns version reading, principal normalization, payer/author checks, and rejection facts. Return an explicit union rather than optional fields whose meanings depend on combinations.
For Agents
Look in packages/chain/src/evm-adapter-base.ts around the new admission helpers and their consumers. Preserve the payer-first behavior, author widening, version gate, and transport-error distinction, but extract a focused publish-admission policy/decision module with a discriminated result such as admitted/denied/inconclusive plus matched principal and author status. Update pinned signer, pool selection, and funding enrichment to consume that one decision shape.
| chainId: 'evm:31337', | ||
| staticNetwork: false, | ||
| } as EVMAdapterConfig; | ||
| const a = new EVMChainAdapter(config) as any; |
There was a problem hiding this comment.
🟡 Issue: The admission tests lock in private adapter structure too heavily
What's wrong
The new tests are valuable, but their current shape is very white-box and documentation-heavy. They encode internal method names, protected helpers, and hand-mutated adapter internals, which makes the suite resist exactly the kind of structural cleanup this PR needs.
Example
A narrow implementation change, such as moving _cgAdmitsAttestedAuthor into a helper module or renaming the protected method, would require broad test rewrites even if public behavior and the admission policy stay identical.
Suggested direction
After extracting the admission policy, test the policy as a first-class unit and reserve adapter tests for public flows like resolvePublisherPublishPlan and createKnowledgeAssets. Move long rationale into concise case names or a short top-level note.
Confidence note
This is about maintainability of the new test code, not whether the scenarios are valuable.
For Agents
In packages/chain/test/evm-adapter-publish-admission.unit.test.ts, keep the behavioral matrix but collapse repeated scenarios into table-driven tests. Prefer direct tests against the extracted admission policy from the production module, with only a small number of public adapter integration tests for wiring.
There was a problem hiding this comment.
Fair observation, and I am declining it here for a reason that is really about sequencing rather than disagreement.
The suite is deliberately in two halves. The white-box rows you are looking at pin the admission policy — payer-first ordering, fail-closed on each unreadable-version flavour, fail-open when ContextGraphs is absent, the transport-vs-denial distinction. Alongside them are far-end rows driving the public surfaces (createKnowledgeAssets pinned and unpinned, resolvePublisherPublishPlan unpinned, the enrichment path), which exist precisely because an earlier version of this work tested inner hops and left two threading sites unguarded — a gap that shipped undetected through three review rounds.
So the public-flow coverage you are asking for is already there; what is additionally white-box is the policy matrix, and today it has nowhere else to live.
Your suggestion is correct but ordered after the extraction (#1973): once the admission policy is a first-class unit, that matrix belongs on it, and the adapter keeps only the wiring tests. Doing the test reshape before the extraction would mean rewriting it twice.
Recorded on #1973 as part of that work, with the note that the far-end rows must survive the move — they are the ones that caught the real gaps, and they are the ones a policy-level unit test would not have caught.
…1689) Curated Context Graph publish authorization was evaluated against the transaction payer (`msg.sender`) instead of the EIP-712-attested author, 111 lines after the author had already been cryptographically proven. For a curated CG in EOA/Safe mode `ContextGraphs.isAuthorizedPublisher` requires exact equality with the single stored `publishAuthority`, so a curator whose agent identity is wallet A could only ever publish from A itself. Every distinct funded operational or async-publisher wallet was rejected, and delegating payment meant sharing the curator's key. The gate now authorizes when EITHER principal a publish carries is permitted: the paying principal (evaluated first, so the common case still costs one eth_call) or the attested author. This is a pure widening -- the accepted set is a strict superset, so no publish that authorizes today can fail after it. Authorizing the author *instead of* the payer, as the issue literally requests, would break shipped behaviour: a curator publishing a member-authored KA (#1778) has an unauthorized author and an authorized payer. Row R3 pins that flow; mutating the gate to author-only fails it. Scope is authorization only. Publisher-of-record, the OT-RFC-53 CG registration escrow, the PCA discount and every TRAC transfer remain keyed to `msg.sender` -- deliberately, since re-keying any of them to the author would let a holder of an author attestation spend the AUTHOR's funds, turning an attacker-funded action into a victim-funded one. This supersedes but does not revert the N17 closure, which addressed a different axis (recovered node signer vs paying principal). Client: - one admission predicate returning the decision facts it used, so the rejection message is explained from the gate's own evaluation rather than re-derived by a second version() read that could disagree - the relaxation is gated on the DEPLOYED KnowledgeAssetsLifecycle version (>= 10.1.7), memoized by lifecycle address and failing CLOSED, so a node on new code against an un-rotated chain keeps today's exact behaviour instead of broadcasting a transaction certain to revert - a mechanical guard couples the client threshold to the contract's _VERSION; drift between them would fail nothing and leave the fix silently dormant forever after rotation - the authorization rejection is now a structured, TERMINAL authority_forbidden instead of a retryable rpc_unavailable that looped a permanently-failing job to maxRetries ABI unchanged (verified by semantic comparison of the freshly compiled artifact against the committed ABI: 58 vs 58 entries, zero signature adds or removes), so no regeneration and no PINNED_DIGESTS churn. Requires a KnowledgeAssetsLifecycle redeploy + Hub repoint; the contracts are not upgradeable. See CURATED_PUBLISH_AUTHZ_ROLLOUT_RUNBOOK.md -- the rotation invalidates every outstanding author attestation and in-flight StorageACK, so the async publisher queue must be drained first. Fixes #1689 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hor-aware reroute probe (#1689) Two review findings from PR #1971. PublisherNotAuthorizedForContextGraphError accepted the structured diagnostic details, rendered a message from them, then dropped several. A consumer asking the very next question -- "is this chain simply not rotated yet, or was the author checked and refused?" -- had to parse `error.message` for facts the adapter had already computed exactly once. This PR exists to remove that coupling, so re-introducing it one question later was self-defeating. The error now carries the full (frozen) details object; `code`, the message contract and the flattened convenience fields are unchanged, so publisher's classifier and existing consumers are unaffected. The author-aware `poolHasFundableSigner` call in `enrichInsufficientPublisherFundsError` had no test. Dropping the author argument there would leave every existing test green while silently turning a recoverable author-authorized reroute into a terminal NO_FUNDED_PUBLISHER_WALLET -- the same shape as the dkg-publisher.ts forwarding line the adversarial review caught. Now covered, with a payer-only negative control proving the legacy path is unchanged. Chain unit suite 853 -> 857 passing. Closure build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… comparator, single-source error facts (#1689) Three review findings from PR #1971 round 2. createKnowledgeAssets passes params.author?.address into its authorization gates on both the pinned and unpinned paths, and neither was tested. Removing that threading left the planner tests, contract tests and publisher forwarding tests all green while the direct/sync publish path pre-rejected whenever only the author was authorized. This is the third instance of the same shape on this PR, after dkg-publisher.ts:3055 and the funds-enrichment probe. Now covered through the public method on both paths, with a neither-principal-authorized control so the positives cannot pass by the gate being skipped entirely; each call site is mutation-proven independently. The deployed-contract version comparator this PR introduced explicitly documented that it mirrored the PCA clearAgents gate, leaving two copies of a capability-boundary policy that drift silently. Extracted to contract-version.ts and both gates migrated; no inline version parsing remains in packages/chain/src. The equivalence is proven executably: the test reproduces the pre-migration PCA expression verbatim, including its own parse so the assertion is not circular, and asserts agreement across 20 named cases plus an exhaustive grid. Mutating the shared comparator now fails both gates at once, which is the point. PublisherNotAuthorizedForContextGraphError stored the diagnostic facts twice. The flattened fields are now getters delegating to `details`, so there is one source of state. Verified first that no consumer spreads or JSON-serializes the error, since getters are not own enumerable properties. Found while mutation-testing that last fix: hard-coding a flattened accessor to ignore `details` left 47 tests passing. The fix for "two representations can disagree" was itself unverified. Assertions added that each accessor equals its `details` counterpart on both error shapes; the same mutation now fails 2. Chain unit suite 857 -> 885 passing. Publisher 604, closure build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ersion parser (#1689) Two review findings from PR #1971 round 3. PublisherNotAuthorizedForContextGraphError still took `message` and `details` as separate constructor arguments, so a caller could build an error whose text did not describe its own facts -- and our own tests did exactly that. The constructor now takes `details` alone and derives the message internally, making that state unconstructible rather than merely untested. The shared contract-version comparator inherited a lenient parseInt parse: `contractVersionAtLeast('11.garbage', '10.1.7')` returned true, because the major component parsed and the malformed remainder read as zero. Harmless today, since every _VERSION is a hardcoded literal, but this is the one canonical helper future capability gates will reuse, and a parser that turns malformed input into a confident "yes" is the wrong thing for a capability boundary to inherit. Now a full-string dotted -integer match returning null, with gates failing closed on null. Chain 883 passing, publisher 604, closure build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review surfaced the same defect shape five times across four rounds: a line whose deletion leaves every suite green while silently breaking one lane. Rather than keep fixing them individually, every site where the attested author is threaded from a public entry point into an admission decision was enumerated -- 11 sites, derived by tracing call chains rather than grepping the identifier -- and each was mutation-measured: threading removed, full suite run, result recorded. Nine were already guarded. Two were not: createKnowledgeAssets -> enrichInsufficientPublisherFundsError resolvePublisherPublishPlan (unpinned) -> _authorizedPublisherSigners Both could be deleted with the entire chain suite passing. The second is the site the review bot flagged; an earlier measurement of it reported GUARDED, but that run happened while a mutation sweep was concurrently rewriting the same files, so it was withdrawn and re-measured cleanly. Both are now covered by tests driving the PUBLIC entry point and asserting the value at the far end of the chain, per the rule that explains all five instances: testing an inner hop proves the hop works, not that anything calls it. The previous enrichment test called the helper directly, which is exactly why it bound its own hop and left its caller unguarded. Each new test mutation-proven: removing the threading kills that test and nothing else. Chain unit suite 883 -> 885. Publisher 604. Closure build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rminal authorization failure (#1689) Review round 5. The client-side author gate reads KnowledgeAssetsLifecycle.version() to decide whether the attested author may be consulted, and caught every failure indiscriminately, returning null. Null then meant three different things -- no lifecycle bound, the version was unparseable, and the read failed transiently -- and all three produced PUBLISHER_NOT_AUTHORIZED_FOR_CG, which the publisher maps to a TERMINAL authority_forbidden that neither retry() nor the auto-retry sweep will touch. So inside one predicate, two reads answering the same question had opposite failure semantics: a transport failure on the payer read propagated and classified retryable, while the same failure on the version read was swallowed into a policy-denial verdict and went terminal. On a single-RPC node one 503 during a curator author-publish permanently killed a job the chain would have accepted. Chain: the version helper now reports WHY it could not read, discriminated on isChainRpcTransportError -- deliberately not isRetryableRpcError, which counts BAD_DATA as retryable, and BAD_DATA is exactly the "this lifecycle has no version() to decode" case that must stay terminal or the forever-retry trap this PR closes re-opens. The transport error is rethrown at the two rejection sites only, never from the helper: _authorizedPublisherSigners probes the author before the payer loop, and poolHasFundableSigner consumes only .admitted inside the NO_FUNDED enrichment probe, so a throwing helper would fail a payer-authorized publish and corrupt the insufficient-funds classification. Publisher: the chain fix alone was necessary and insufficient. RPC_ENDPOINTS_EXHAUSTED splices the underlying node text into its message, and "missing revert data" -- the standard response for a failed eth_call, which is what version() is -- classified as tx_reverted, terminal. The same job death, one layer down, from the same 503. A proven transport error means no transaction was mined, so tx_reverted and confirmation_mismatch are wrong by construction; only those two outcomes are suppressed, and only from 'broadcast'. Timeout handling is deliberately untouched: a genuine RPC_TIMEOUT during send must keep tx_submit_timeout -> check_chain_then_finalize_or_reset, or #1851's double-submit hole re-opens. Also adds public-consumer coverage for the PCA clearAgents version gate, whose behaviour this PR changed when it migrated onto the shared strict comparator. Chain 885 -> 894, publisher 604 -> 608, closure build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… denied (#1689) Third consumer of the same defect, and it was created by the instruction that specified the fix for the first two. The previous round taught the authorization gates to distinguish "policy denied" from "could not determine whether policy denies", and rethrew the transport error at the two rejection sites. The fix instruction said the version helper must never throw, BECAUSE poolHasFundableSigner consumes only `.admitted`. That constraint protected the probe and, in the same stroke, preserved the exact collapse being removed everywhere else. So: pinned signer short on TRAC, another wallet funded, neither payer-authorized, the attested author authorized on a 10.1.7 lifecycle, and the version cache cold during enrichment. Every wallet probes false, enrichInsufficientPublisherFundsError emits NO_FUNDED_PUBLISHER_WALLET, and a transient blip becomes a terminal insufficient_funds on a publish the chain would have accepted. The calling code already wanted the right behaviour: enrichment ends in `catch { /* never let enrichment mask the original failure */ } return err`, and the probe has exactly one production caller, inside that try. So an inconclusive probe now throws the captured versionReadError, which that catch converts into "preserve the original error so a retry can reroute" -- no richer return type, no new plumbing. The throw is NOT inside the Promise.all map: the error is captured in a closure, the map settles, and the decision is made afterwards. That makes three properties true by construction rather than by argument -- a different rejection still rejects first and cannot be masked, the no-error path is behaviourally identical to before, and the throw fires only in the branch that would otherwise assert the whole pool is unfundable. Two rows, mutation-proven: an inconclusive admission keeps the original error, and a genuinely unfundable pool still produces terminal NO_FUNDED_PUBLISHER_WALLET. The second is the control that proves the first is not simply "NO_FUNDED got weaker" -- it stays green under the mutation that kills the first. The generalisable lesson is about how the earlier constraint was phrased: stated as a mechanism ("must not throw") rather than as the invariant it served ("must not report inconclusive as denied"). Mechanism-shaped constraints protect the site you were looking at and silently license the same defect everywhere you were not. Chain 894 -> 896. Publisher 608 unchanged. Closure build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d39ab18 to
1ec4531
Compare
2 participants
Summary
Curated CG publish authorization was evaluated against the transaction payer (
msg.sender) instead of the EIP-712-attested author — 111 lines after the author had already been cryptographically proven. For a curated CG in EOA/Safe mode,ContextGraphs.isAuthorizedPublisherrequires exact equality with the single storedpublishAuthority, so a curator whose agent identity is walletAcould only ever publish fromAitself. Every distinct funded operational or async-publisher wallet was rejected, and delegating payment meant sharing the curator's key. The async lane — which pays frompublisher-wallets.jsonand therefore never holds the curator key — could never publish to a curated CG.The gate now authorizes when EITHER principal a publish carries is permitted: the paying principal (evaluated first, so the already-working case still costs one
eth_call) or the attested author. This is a pure widening — the accepted set is a strict superset, so no publish that authorizes today can fail after it. That is the central regression contract of this PR.Authorizing the author instead of the payer — the issue's literal request — would break shipped behaviour.
lift-job-types.ts:45records it: "Resolved assertion AUTHOR (may differ from the caller — GH#1778 curator publish)". A curator publishing a member-authored KA has an unauthorized author and an authorized payer; a strict author-only gate rejects it. Row R3 pins that flow, and mutating the gate to author-only fails it. The issue's negative requirement still holds via row R4: unauthorized author and unauthorized payer is rejected.Scope is authorization only. Publisher-of-record (
createKnowledgeAsset(msg.sender, …)), the OT-RFC-53 CG registration escrow, the PCA discount lookup and every TRAC transfer remain keyed tomsg.sender— deliberately. Re-keying any of them to the author would let a holder of an author attestation spend the author's escrow or discount, turning an attacker-funded action into a victim-funded one.This supersedes but does not revert the "N17 closure." N17 addressed a different axis — recovered node signer vs paying principal — and its conclusion is retained: we pass the paying principal, never the node signer. bug(protocol): curated publish authorization checks the transaction payer instead of the attested agent #1689 only adds a second accepted principal on top. The in-code comment says so explicitly, so this is not read as a silent inversion.
The client mirror is version-gated and fails closed. The relaxation activates only when the deployed
KnowledgeAssetsLifecycle.version() >= 10.1.7, memoized by lifecycle address so it self-heals on Hub rotation without a restart. Unknown or unreadable ⇒ unsupported. Without this, a node on new code against an un-rotated chain would broadcast transactions certain to revert, burning gas.The authorization rejection is now terminal. It was classified as retryable
rpc_unavailable, so a permanently-failing job looped tomaxRetries = 10while remaining "occupying" — every re-publish silently re-accepted the same poisoned job. It is now a structured, terminalauthority_forbidden, so a corrected re-publish gets a fresh job.Related
isAuthorizedPublisherselectedAuthorAgentAddress), which is not on this basedeadline, seal distribution), CG escrow / PCA discount when payer ≠ CG owner, StorageACK requester authorization, update-path attestation replay,ContextGraphs.updatePublishAuthorityclient bindingsDiagrams
Async VM publish to a curated CG (curator authors, distinct wallet pays)
Before — rejected at plan time, then misclassified as a transient outage:
sequenceDiagram participant Curator as Curator agent A participant Queue as Async publisher participant Planner as Chain planner participant CG as ContextGraphs Curator->>Queue: publish-async (seal: author=A) Queue->>Planner: plan (payer=P, pinned) Planner->>CG: isAuthorizedPublisher(cg, P) CG-->>Planner: false Planner-->>Queue: throw "not authorized" Queue-->>Curator: rpc_unavailable, retryable Note over Queue: job stays occupying — re-publish re-accepts it — loops to maxRetriesAfter — the author admits the publish, any funded wallet may pay:
sequenceDiagram participant Curator as Curator agent A participant Queue as Async publisher participant Planner as Chain planner participant CG as ContextGraphs participant KAL as KnowledgeAssetsLifecycle Curator->>Queue: publish-async (seal: author=A) Queue->>Planner: plan (payer=P, attestedAuthor=A) Planner->>KAL: version() KAL-->>Planner: 10.1.7 (author authz supported) Planner->>CG: isAuthorizedPublisher(cg, P) CG-->>Planner: false Planner->>CG: isAuthorizedPublisher(cg, A) CG-->>Planner: true Planner-->>Queue: plan ok (P pays) Queue->>KAL: publish (msg.sender=P, authorAddress=A) KAL->>CG: isAuthorizedPublisher(cg, msg.sender=P) → false KAL->>CG: isAuthorizedPublisher(cg, p.authorAddress=A) → true KAL-->>Queue: KA minted to A — P is publisher-of-record and payerGenuinely unauthorized publish (neither principal permitted)
Before — permanent failure requeued as a transient RPC outage:
sequenceDiagram participant Queue as Async publisher participant Planner as Chain planner participant Classifier as Failure classifier Queue->>Planner: plan (payer=P) Planner-->>Queue: throw (untyped Error) Queue->>Classifier: classify(message, state=broadcast) Note over Classifier: matches no substring → default Classifier-->>Queue: rpc_unavailable, retryable, reset_to_acceptedAfter — structured code, terminal, job no longer occupying:
sequenceDiagram participant Queue as Async publisher participant Planner as Chain planner participant Classifier as Failure classifier Queue->>Planner: plan (payer=P, attestedAuthor=M) Planner-->>Queue: throw PUBLISHER_NOT_AUTHORIZED_FOR_CG Queue->>Classifier: classify(code, state=broadcast) Classifier-->>Queue: authority_forbidden, terminal, fail_job Note over Queue: job non-occupying → corrected re-publish gets a FRESH jobFiles changed
packages/evm-module/contracts/KnowledgeAssetsLifecycle.solmsg.senderorp.authorAddress._VERSION10.1.6 → 10.1.7 (the client's capability signal). Comment rewritten to state that N17 is superseded, not reverted. ABI unchanged.packages/evm-module/contracts/ContextGraphs.solpackages/evm-module/test/v10-curated-publish-authz.test.ts@integrationsuite. No coverage ofUnauthorizedPublisherexisted anywhere in the repo before thispackages/evm-module/scripts/CURATED_PUBLISH_AUTHZ_ROLLOUT_RUNBOOK.mdpackages/core/src/errors.ts,index.tsPUBLISHER_NOT_AUTHORIZED_FOR_CGcontract, alongside theNO_FUNDED_PUBLISHER_WALLETprecedentpackages/chain/src/evm-adapter-base.ts_cgPublishAdmits(returns the decision facts it used),_cgAdmitsAttestedAuthor, the deployed-version capability gate; all three authorization sites routed through the pairpackages/chain/src/evm-adapter-errors.tsPublisherNotAuthorizedForContextGraphError+ message formatter naming both principalspackages/chain/src/evm-adapter-publish.tspackages/chain/src/chain-adapter.tsPublisherPublishPlanRequest.attestedAuthorAddresspackages/chain/src/evm-adapter.ts,index.tspackages/chain/test/evm-adapter-publish-admission.unit.test.ts_VERSIONdrift guardpackages/chain/test/evm-adapter.unit.test.tspackages/chain/test/chain-lifecycle-extra.test.tspackages/chain/test/mock-adapter-parity.test.tspackages/publisher/src/dkg-publisher.tsprecomputedAttestation.authorAddressinto the plan requestpackages/publisher/src/publisher-planning.tsfinalize→resolvePublisherPublishPlanpackages/publisher/src/async-lift-publish-result.tsauthority_forbidden(code match, message-marker fallback)packages/publisher/src/lift-job-failures.tsauthority_forbiddenallowed frombroadcast— it previously threw when emitted from that statepackages/publisher/test/publish-author-threading.test.tspublish()and asserts the seal author reaches the plannerpackages/publisher/test/async-lift-publish-result.test.tspackages/publisher/vitest.unit.config.ts[P-14]policy suite, which an acceptance criterion depended on and which was therefore unverifiable bytest:unit), plus the new author-threading testpackages/agent/test/publish-foreign-author-resolution.test.tsTest plan
Contract — the 8-row matrix
UnauthorizedPublisheraddress(0)AuthorRequiredR2 also asserts
merkleRoots[0].publisher == PandownerOf(kaId) == A— authorization moved, attribution did not.One methodological note, which is worth more than the individual fixes
Review turned up the same defect shape five times across three rounds and three files: a line whose deletion leaves every suite green while silently breaking one lane. Rather than keep fixing them one at a time, every site where the attested author is threaded from a public entry point into an admission decision was enumerated (11 sites, derived by tracing call chains rather than grepping the identifier) and each was mutation-measured — the threading removed, the full suite run, and the result recorded.
The rule that explains all five, and would have prevented them:
That is exactly why one end-to-end test binds three consecutive links in the publisher lane, while a test calling the enrichment helper directly bound only its own hop and left the caller's threading unguarded. Reading a test file tells you a test exists nearby; only removing the code tells you a test binds.
Suites (all re-run by the author on the final commit)
hardhat test … v10-curated-publish-authz.test.tshardhat test … ContextGraphs + v10-pca-lifecycle + v10-pca-agent-dospnpm --filter …dkg-chain test:unitpnpm --filter …dkg-publisher test:unitpnpm --filter "…dkg-agent..." run buildscripts/sparql-scale-lint.mjs --diffTwo load-sensitive tests, flagged rather than smoothed over
Two long-duration chain tests each failed once during a mutation run and passed in every other run today, including all final verification runs:
executes the complete scan over the real strict loopback transport(~4956ms)fails over in canonical order after wrong-chain evidence without endpoint stickiness(~7207ms)Both are RPC-failover/transport timing tests, and in both cases the mutation applied at the time could not plausibly have caused them — one mutation only hardcoded a boolean in the PCA context builder, which cannot affect failover ordering. The common factor is that another heavy suite was running concurrently.
Recorded as an observation with evidence, not a diagnosis — neither was chased. If either appears in CI, suspect load-sensitivity before hunting a regression in this diff.
Two traps for anyone re-running these. (1) The contract regression suites need the machine to themselves — run concurrently with the agent suite they hit a 40s
"before each"timeout that is load, not a regression. (2) Do not usegit diff --stat packages/evm-module/abi/as the ABI check:hardhat.node.config.tsdoes not loadhardhat-abi-exporter(onlyhardhat.config.tsdoes), so that compile never writesabi/and the check prints clean whether or not the ABI changed. Use the semantic comparison.Rollout — an ops action, and not a hot swap
KnowledgeAssetsLifecycleis not upgradeable. Contracts are plain deployments behind a Hub name→address registry, so this requires deploying a new KAL and repointing the Hub. Full procedure inCURATED_PUBLISH_AUTHZ_ROLLOUT_RUNBOOK.md.dkg-agent-publish.ts:5012hard-fails any queued VM publish whose seal binds the old address. The async publisher queue must be drained before rotation. It is a hard cutover: the old KAL loses Hub membership and all storage-write permission immediately — no coexistence window, and no rollback without a second rotation.Nodes need no config edit and no restart; addresses are Hub-resolved at runtime. Until rotation, the client's version gate holds every node at today's exact behaviour.
Known residual risk, stated plainly
Making the attested author an authorization principal turns the author attestation into a usable bearer credential for curated CGs. It binds only
(merkleRoot, author, schemeVersion, reservedKaId)— nocontextGraphId(removed by #1116), no nonce, no deadline — and is replicated to CG peers over the durable_metalane. A member holding that seal can pay to publish the curator's draft early, choosingepochsandisImmutable.Bounded: the attacker pays, the NFT still mints to the author, authorization still requires the author to be authorized for that CG (so it cannot be redirected to an arbitrary CG), and the equivalent exposure already exists on open CGs, where
publishPolicy == 1authorizes any non-zero address. Hardening is deferred to a follow-up rather than expanded into this PR.🤖 Generated with Claude Code