Skip to content

Fix public Context Graph subscription catch-up - #1848

Merged
lupuszr merged 3 commits into
testnet-canaryfrom
v10/fix-public-cg-subscribe
Jul 20, 2026
Merged

Fix public Context Graph subscription catch-up#1848
lupuszr merged 3 commits into
testnet-canaryfrom
v10/fix-public-cg-subscribe

Conversation

@zsculac

@zsculac zsculac commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • expose passively discovered public Context Graphs in the Context Oracle
  • route explicitly public graphs through the public /api/subscribe path, including when an older curator invite is pasted
  • recognize authoritative public metadata from an explicit public root definition or identity-bound active public on-chain registration
  • preserve existing tracked durable + SWM catch-up, preferred-peer ordering, fallback peers, and readiness protections
  • keep private/curated metadata and join approval fail-closed

Root cause

The UI hid a discovered public graph until it was subscribed or synced, then forced it through the curator-invite flow. After approval, runImmediatePostApprovalSync() correctly used the private metadata refresh path, whose proof correctly rejected the public snapshot because it had neither a private definition nor an approved-member delegation. hasConfirmedMetaState() also did not recognize the graph's exact root accessPolicy="public" definition, so SWM catch-up remained unauthorized.

The daemon's public subscribe route and catch-up tracker were already correct; the UI simply never reached that path for this graph.

Reproduction graph:

0x00a9D0dcab936a418ffEbc734476C91D4027d359/balkan-places-to-visit

Security boundary

  • publisher allowlists on explicit-public graphs do not restrict reading, subscription, or catch-up
  • private metadata still requires the complete private definition and a current member delegation bound to the local peer or operational key
  • forged/stale join approval remains unable to create a private subscription
  • empty responses alone cannot mark a graph synced; authoritative metadata and clean plane completion remain required

Verification

  • pnpm --dir packages/agent exec vitest run --config vitest.unit.config.ts test/private-cg-membership-bootstrap.test.ts — 27 passed
  • relevant agent discovery/peer-selection/SWM suites — 59 passed
  • pnpm --dir packages/agent exec vitest run test/cg-resolve-refresh.test.ts -t "requires a current approved member delegation" --reporter=verbose — 1 passed, 13 skipped
  • pnpm --dir packages/cli exec vitest run --config vitest.unit.config.ts test/context-graph-subscribe-readiness.test.ts — 17 passed
  • pnpm --dir packages/node-ui exec vitest run test/contextGraphSidebar.test.ts test/join-project-modal.test.ts test/panel-left.test.ts — 50 passed
  • pnpm --dir packages/query exec vitest run test/query-handler.test.ts — 35 passed
  • agent, node-ui, CLI TypeScript builds passed
  • node-ui production Vite build passed
  • git diff --check passed

Remote query note

The separate ACCESS_DENIED: Context graph is not queryable response remains unchanged. Remote query access is controlled by queryAccess; the current testnet chain did not contain a committed registration for the exact graph hash, so denial under the default policy is expected unless the curator explicitly enables per-graph query access.

// alter that read/subscription policy. Keep this separate from the private
// definition above so private bootstrap still requires the current local
// member delegation.
const authoritativePublicDefinitionResult = await this.store.query(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: Extract public meta confirmation instead of adding another inline branch

What's wrong
This adds another policy branch to an already dense lifecycle method. The method now owns placeholder detection, private proof setup, public proof SPARQL, on-chain proof caching, and legacy ontology fallback ordering. That makes the public/private authority model harder to scan and easier to drift, especially because one proof path is helper-backed and the new one is embedded directly in the lifecycle flow.

Example
A future change to what counts as authoritative public metadata would need to update raw SPARQL here while private metadata stays in the proof helper, then re-check the rejectUnregisteredPlaceholder branch and the second on-chain fallback ordering.

Suggested direction
Promote the public proof into the same canonical proof layer as the private ASK builder, and have hasConfirmedMetaState orchestrate named authority checks rather than carrying raw SPARQL plus a mutable tri-state cache.

For Agents
Look in packages/agent/src/dkg-agent-lifecycle.ts around hasConfirmedMetaState and packages/agent/src/context-graph-private-meta-proof.ts. Preserve the current public/private/on-chain acceptance behavior, but move public metadata proof construction into a dedicated helper or a small resolveConfirmedMetaAuthority policy that returns an explicit authority result. Existing bootstrap tests should continue to prove the same accepted/rejected cases.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in 409e12c. Public authority now lives in context-graph-public-meta-proof.ts: fetched curator snapshots and the hasConfirmedMetaState ASK are driven by the same canonical requirements. The proof also rejects contradictory public/private policy rows, preserving the private-wins ratchet. Added snapshot/ASK parity plus public curator-refresh regressions; the focused agent authorization suite passes 50/50.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: Public authority resolution is being bolted into an already overloaded lifecycle method

What's wrong
The change adds more sequential control flow, shared mutable cache state, repeated operation-context construction, and a dense boolean guard to a 7k-line lifecycle file. The behavior may be right, but the structure makes the confirmation policy harder to scan and easier to accidentally break on the next authority source.

Example
A reader now has to reason through system graph bypass, placeholder detection, private proof, public metadata proof, lazy on-chain proof, placeholder rejection, private detection, and ontology fallback in one long method before understanding what confirms metadata.

Suggested direction
Extract the public-proof and on-chain-proof orchestration behind a named helper or small decision model so this method stops accumulating special-case branches.

For Agents
Move the new public metadata/on-chain authority decision into a focused helper such as resolveConfirmedMetaAuthority or hasAuthoritativePublicMetaState. Keep the existing ordering and rejectUnregisteredPlaceholder behavior, but make hasConfirmedMetaState read as a small dispatcher over named authority checks.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: hasConfirmedMetaState is accumulating authority policy branches inside the lifecycle class

What's wrong
The PR adds another authority source directly into a 7k-line lifecycle class and threads it through a nullable sentinel plus several dependent conditionals. Even if each branch is locally justified, the structure makes the confirmation policy harder to scan and harder to extend safely. This is a classic spaghetti-growth point: new public authority, private authority, chain authority, and legacy fallback are now coupled by incidental control flow rather than by an explicit model.

Example
A reader trying to answer “why is this CG authoritative?” now has to trace the private ASK, the public ASK, the optional placeholder rejection, the lazy on-chain lookup, and the later fallback block in order. The boolean | undefined sentinel is a symptom that this method is doing policy orchestration and cache timing at the same time.

Suggested direction
Pull the metadata confirmation policy into a dedicated helper/module, ideally one that names the authority source and hides the placeholder/on-chain memoization detail. This would turn the lifecycle method back into orchestration rather than a growing policy state machine.

For Agents
Focus on LifecycleSyncMethods.hasConfirmedMetaState. Preserve the current precedence, including rejectUnregisteredPlaceholder, private member proof, public metadata proof, active public on-chain proof, and existing fallback behavior. Extract a focused metadata-authority resolver/helper that returns a typed result or enum, and have this method delegate to it. Tests should prove the existing public/private/placeholder cases still resolve the same way.


setError(null);

if (publicSubscription) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: Model public subscribe as an explicit intent instead of an inline join-handler branch

What's wrong
The modal now mixes two workflows through an ad-hoc publicSubscription boolean. That boolean controls validation, button copy, and submit behavior, while the public branch duplicates post-success UI orchestration and uses any casts despite having ContextGraph in scope. The structure works locally, but it makes the modal more stateful and branch-heavy instead of making the new mode a first-class concept.

Example
Changing how the modal opens a context graph after success now requires checking transitionToApproved, the existing-project short-circuit, and the new public-subscribe branch. The only thing that differs is the operation before opening: /api/subscribe versus signed join submission.

Suggested direction
Derive one typed intent from inviteCode and contextGraphs (publicSubscribe vs privateJoin) that owns validation and labels, then share the post-success project activation/open-tab code across both flows.

For Agents
In JoinProjectModal.tsx, preserve public subscription and private join behavior, but introduce a small JoinIntent/resolveJoinIntent model and a shared refreshAndOpenContextGraph helper. The submit handler should dispatch by intent kind rather than embedding a second workflow inside the private join handler.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in 409e12c. JoinProjectModal now derives a typed JoinIntent (publicSubscribe or privateJoin) that owns validation and labels, dispatches by intent kind, and shares refresh/open/activation helpers across both workflows. The changed production paths no longer use any casts. Modal regressions and the UI build are green.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: The join modal adds a mode flag to validation instead of separating the two intents cleanly

What's wrong
Public subscription and private curator join have different contracts, but the implementation keeps one validateInvite function and pokes a boolean hole through it for the public path. That makes the old private-invite validator carry a second mode, and it invites more option flags as the flows diverge. A cleaner model would make the discriminated intent own the boundary explicitly and remove the special-case boolean.

Example
Today the only mode flag is allowBareContextGraphId. If the public subscribe path later needs different legacy-multiaddr handling or copy, validateInvite becomes a growing set of intent-specific options instead of a simple private invite validator.

Suggested direction
Split validation by intent: parse once, classify as public subscribe or private join, then call a small validator for that intent. Keep button labels/copy in a separate map keyed by intent kind rather than embedding presentation text in the intent constructor.

Confidence note
This is a maintainability concern from the diff structure; the current behavior may be intentional, but the abstraction boundary is still muddier than necessary.

For Agents
In JoinProjectModal.tsx, keep parseInviteCode and the two user-visible flows. Replace the allowBareContextGraphId flag with separate validation for publicSubscribe and privateJoin, or make the discriminated JoinIntent carry a validated payload produced by separate constructors. Keep the rendered public subscribe and private join tests passing.

if (belongsInMyProjectsSidebar(cg, identity)) return false;
if (normalizeAccessPolicy(cg.accessPolicy) !== 'public') return false;
return cg.subscribed === true || cg.synced === true;
return true;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: Keep catalogue-only graphs out of the project-row abstraction

What's wrong
The predicate change redefines Oracle membership from 'public graph we have interacted with' to 'any public graph we know about', but the surrounding UI abstraction still treats every Oracle item as a selectable project. That conflates discovery/catalogue state with active local project state and pushes a new concept through an old row component without making the boundary explicit.

Example
A ContextGraph with accessPolicy: 'public', callerInvolved: false, subscribed: false, and synced: false now belongs in the Oracle. The sidebar row still behaves like a local project row rather than a catalogue result that should drive the subscribe flow described by the new empty-state copy.

Suggested direction
Return a richer sidebar classification, or render a separate Oracle catalogue item/action for unsubscribed public graphs. The predicate can still include discovered public graphs, but the UI layer should not inherit project navigation semantics by accident.

Confidence note
This is a structural concern inferred from the changed predicate plus the unchanged PanelLeft row behavior in the same PR scope.

For Agents
Look at contextGraphSidebar.ts and PanelLeft.tsx. Preserve the new ability to show discovered public graphs before subscription, but split the sidebar model or row action so catalogue-only entries are not treated as ordinary project rows until subscribed/synced. Add/update UI tests around the row action for an unsubscribed public oracle item.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in 409e12c. Context Oracle entries are now classified as catalogue or project. Passive public discoveries render a separate OracleCatalogueItem with explicit Browse and Subscribe actions; Browse opens a tab without setting activeProjectId, while Subscribe opens the prefilled public-subscribe modal. The rendered PanelLeft regression covers that boundary.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: Oracle filtering and classification should be one typed model

What's wrong
The PR splits one domain decision across two exported functions. That makes the classification context-dependent and pushes an ordering invariant onto every caller instead of making the sidebar model explicit.

Example
classifyContextOracleEntry({ id: 'x', accessPolicy: 'private', subscribed: true }) returns 'project' even though that graph does not belong in the Oracle at all. Current callers filter first, but the exported helper does not encode that invariant.

Suggested direction
Collapse the two-step predicate/classifier into a single getContextOracleEntry-style helper so callers cannot classify invalid rows out of context.

For Agents
Replace belongsInContextOracleSidebar + classifyContextOracleEntry with one function that returns null or a typed Oracle entry, for example { kind: 'catalogue' | 'project', graph }. Update PanelLeft to render that list directly and keep tests around public passive/subscribed/synced cases.

// Public read/subscription is an explicit local opt-in. The daemon
// queues the same tracked durable+SWM catch-up job used by the CLI;
// no curator approval or member delegation participates.
await subscribeToContextGraph(cgId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: Public subscribe flow is not covered at the UI interaction boundary

What's wrong
The PR adds a new user-facing path where a bare discovered public Context Graph ID subscribes directly instead of sending a curator join request. The added tests verify the helper decision, but they would still pass if the modal button stopped calling subscribeToContextGraph, failed to refresh the list, or accidentally used the private request flow.

Example
A component test could seed the projects store with { id: 'open-project', accessPolicy: 'public', subscribed: false }, render JoinProjectModal, enter open-project, click Subscribe, and assert subscribeToContextGraph('open-project'), fetchContextGraphs(), active-project/tab updates, and onClose(). A regression that accidentally falls through to the private join request path would pass the current helper-only tests.

Suggested direction
Cover the new rendered-modal public subscription path, not just the helper predicate, because the changed behavior depends on the button handler wiring and side effects.

For Agents
Add a node-ui component test around JoinProjectModal for the public bare-ID path. Mock subscribeToContextGraph and fetchContextGraphs, seed useProjectsStore with a discovered public graph, click the primary button, and prove the public branch runs while existing private invite behavior remains covered.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in 409e12c. A rendered JoinProjectModal test now covers both a first public subscription and the production state accessPolicy=public, subscribed=true, synced=false. It proves subscribe, refresh, active-project/tab updates, close, and no private signing/join calls. The fix always invokes the daemon's idempotent subscribe route so an unsynced subscription gets a tracked catch-up job.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: Public subscription is overloading the private join modal

What's wrong
This modal was already responsible for parsing curator invites, signing delegations, handling approval SSEs, and launching workspace wiring. Adding public subscribe as an inferred branch increases coupling to the project store and makes the component harder to reason about without deleting any of the existing complexity.

Example
The same primary button can mean Subscribe or Request to Join based on resolveJoinIntent scanning the current contextGraphs store, while the component also owns private invite validation and join approval transitions.

Suggested direction
Separate the public subscribe workflow from the private join workflow, or at least move both workflows behind focused subcomponents/hooks with an explicit intent prop.

For Agents
Extract a small public subscribe controller/component or route PanelLeft’s Subscribe action through a dedicated public-subscribe path. Leave JoinProjectModal focused on private invite validation, signing, approval, and workspace wiring. Preserve the existing subscribeToContextGraph + refresh/open behavior.

@lupuszr lupuszr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Preserve the curator peer when routing a public invite through /api/subscribe

shouldSubscribePublicGraph() intentionally returns true for both a valid two-line curator peer ID and a legacy multiaddr, but the new public branch in JoinProjectModal.tsx jumps straight to subscribeToContextGraph(cgId) before the existing best-effort dial code. That discards invite.curatorPeerId / legacyMultiaddr.

The catch-up worker can only ensure the peer returned by resolvePreferredSyncPeerId() and then selects from live agent.node.libp2p.getConnections(). The new public authority rule requires only an explicit type+public-policy definition (or active chain proof), so a passively discovered row is not guaranteed to carry enough curator metadata for that resolver. If the catalog entry is stale and the curator is not already connected, pasting a currently valid invite still queues an empty/unreachable catch-up even though the invite supplied the exact dial target.

Please best-effort dial the parsed peer ID/multiaddr before subscribing, or extend the subscribe request to carry a preferred peer into catch-up. The UI interaction test should cover a discovered public graph plus a disconnected curator invite and prove the invite peer is used before the job is queued.

@Jurij89

Jurij89 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Adversarial review — Fix public Context Graph subscription catch-up

Multi-lens review against the PR head (c31dac3ca), with a focused adversarial pass on the security-relevant hasConfirmedMetaState change. Net: this is a solid fix and the on-chain proof swap is a genuine improvement. Everything below is LOW severity — one defense-in-depth regression against a documented invariant (cheap to fix), a couple of trade-offs from the swap, and two UX points.


✅ The proof swap is a security improvement

Swapping contextGraphActivePublicOnChainFromRegistryisContextGraphPublicOnChain is the right direction. The old registry proof (dkg-agent-cg-resolve.ts:865) trusts the locally-tracked subscribedContextGraphs[id].onChainId with no identity binding — after a devnet reset / numeric-id reuse it could read the policy of an unrelated slot. isContextGraphPublicOnChain → resolveOnChainAccessPolicyState rebinds the id to the on-chain name-hash (localCgMatchesOnChainSlot) before trusting it. Good. (Two caveats on this swap in §2/§3.)


🟢 LOW (but violates a documented invariant) — the new public gate bypasses the one-way privacy ratchet

The new authoritative-public branch (dkg-agent-lifecycle.ts:~6906-6929) is a raw this.store.query ASK that returns confirmed=true whenever a <cgUri> rdf:type dkg:ContextGraph ; dkg:accessPolicy "public" triple exists. It never checks for a co-present accessPolicy="private" triple, and — unlike the ontology fallback below it — never consults isPrivateContextGraph.

That contradicts the one-way privacy ratchet your own projection documents and enforces (context-graph-meta-projection.ts:608):

"PRIVACY IS A ONE-WAY RATCHET … a CG cannot be re-declared public via a racing/stale/forged public row."

applyAccessPolicy makes private stick in the projection, and isPrivateContextGraph honors that (getExplicitAccessPolicygetCgMeta(), private-wins). But the new gate reads the raw store, where a private and a public quad can coexist (meta-sync inserts additively, and the CG-root accessPolicy subject is not covered by the per-KA PEER_UNTRUSTED_METADATA_PREDICATES quarantine in requester/durable-sync.ts:574-580 — that set only strips graph-scoped KA subjects). So a stale, racing, or peer-served public row makes hasConfirmedMetaState report a CG that also carries an authoritative private declaration as "confirmed public."

Concrete (bounded) sink: gossip-publish-handler.ts:~415-425. When allowedPeers === null and metaSynced === false, it consults hasConfirmedMetaState alone (no canReadContextGraph AND) and, if confirmed, flips metaSynced=true and accepts the gossip publish — the very "deny until _meta sync … can't leak through" protection the comment describes. The ratchet-bypassing gate turns that deny into an accept inside the partial-sync window.

Why it's LOW, not higher: the sink only fires for a CG with no local peer allowlist, where end-state authorization is the same once metaSynced flips via any path; the exposure is integrity (a non-member's gossip publish still faces downstream seal/structural validation), not confidentiality — reads stay ratchet-protected (canReadContextGraph / canUseSharedMemoryForContextGraph = hasConfirmedSharedMemoryMetaState && canReadContextGraph still resolve private via the projection). But it reopens a fail-closed window against a deliberate, documented invariant.

Fix (cheap): make the public ASK fail closed against the ratchet — e.g. add FILTER NOT EXISTS { <cgUri> dkg:accessPolicy ?p . FILTER(LCASE(STR(?p)) = "private") }, or gate the branch on !(await this.isPrivateContextGraph(contextGraphId)) so it reuses the projection that already enforces private-wins.


🟢 LOW — the identity-bound proof adds a liveness dependency and eth_calls the old path didn't have

Two consequences of routing confirmation through isContextGraphPublicOnChain:

  • Transient-flake regression. The name-hash binding issues an extra eth_call (dkg-agent-crypto.ts localCgMatchesOnChainSlot / readLiveOnChainAccessPolicy). A transient RPC timeout on it now makes public-CG confirmation fail (.catch(() => false)), where the old registry proof — reading the already-tracked onChainId — did not depend on that call. The identity-binding win comes with a new liveness coupling for confirmation.
  • Up to 3 live eth_calls on the gossip receive path. hasConfirmedMetaState can now call isContextGraphPublicOnChain (which does 2-3 fresh reads, uncached by design — same as noted on fix(swm): accept plaintext SWM on public-on-chain agent-gated context graphs #1843) and the placeholder branch can call it a second time. Reached from the per-gossip allowedPeers===null path above, this puts chain reads on a hot path that previously issued none. Consider caching the proof within the call, and short-circuiting when the local definition already settles it.

🟢 LOW / please confirm — capability regression for name-hash "opt-out" public CGs

Because the new proof requires the name-hash identity bind, a public CG whose local id does not affirmatively match the on-chain committed name-hash (the "rare" opt-out / wire-id-keyed shapes resolveOnChainAccessPolicyState documents) is now rejected where contextGraphActivePublicOnChainFromRegistry (tracked onChainId + active + policy 0) previously accepted it. For a "fix public subscribe" PR it's worth confirming there is no legitimate class of discovered public CGs this now strands — a devnet check on a graph subscribed under a non-cleartext id would settle it.


🟢 LOW (UX) — "subscribe by ID" over-promises for a not-yet-discovered graph

The new copy ("Public graphs appear here … subscribe by ID", PanelLeft.tsx; "Enter a discovered public Context Graph ID to subscribe", JoinProjectModal.tsx) invites pasting a public CG ID. But shouldSubscribePublicGraph only returns true when the ID is already in the local contextGraphs list with accessPolicy === 'public'. A public CG the node hasn't discovered yet falls through to validateInvite and is rejected with "This invite is missing the curator peer id…" — a wrong, confusing error for exactly the flow the copy advertises. Consider detecting the bare-ID-not-yet-discovered case and surfacing "not discovered yet / still syncing" instead of the curator-invite error.


🟢 LOW (UX regression) — the Context Oracle is now an unbounded list again

belongsInContextOracleSidebar now returns true for every public CG (contextGraphSidebar.ts), dropping the subscribed || synced filter. That filter's now-deleted comment stated its purpose plainly: stop "hundreds of stale *-smoke / *-test entries whose curators are long gone" from flooding the Oracle on a long-running testnet node. Exposing passively-discovered graphs is a reasonable product call, but the dumping-ground the filter prevented returns with no TTL, cap, pagination, or search to replace it. Worth pairing this with a bound (recency/last-seen cap or a search box) so a long-lived node's Oracle stays usable.


Method: independent lenses (security meta-forgery / gate logic / proof-swap regression / UI / SPARQL+tests / open-ended) plus two adversarial verifiers per candidate that re-read the head code and tried to refute each; 10 candidates were refuted (including several test-coverage angles and a data-leak framing that verified as bounded). The ratchet finding and the gossip sink were re-verified by hand.

@zsculac

zsculac commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

@lupuszr Addressed the changes-requested review in pushed commit 409e12c. The public-subscribe path now best-effort warms the curator target parsed from the invite before invoking /api/subscribe, so the peer is present in the connected fallback cohort when the tracked catch-up job is queued. The shared helper preserves the private warm-then-sign flow and supports the legacy target fallback. A rendered regression proves the curator dial occurs before subscribe and that the path never falls through to private signing/join APIs. Focused modal tests pass 34/34 and the UI TypeScript build passes.

@zsculac

zsculac commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

@Jurij89 Follow-up on the adversarial review in pushed commit 409e12c:\n\n- Privacy ratchet: fixed. The canonical public proof now requires an unambiguous public policy and rejects any co-present non-public policy in both fetched-snapshot validation and the generated ASK. The contradictory public/private parity regression passes.\n- Public proof structure/hot path: the inline ASK moved to a canonical helper. The existing tri-state variable caches the identity-bound chain proof within one hasConfirmedMetaState call, and an authoritative local public definition short-circuits the chain read except where placeholder rejection explicitly requires live chain proof. RPC uncertainty remains intentionally fail-closed.\n- Name-hash binding: kept intentionally. Active registration is authoritative only when identity-bound; explicit authoritative public metadata remains an independent confirmation path, so a valid metadata-bearing public graph is not stranded.\n- UI semantics: the modal and empty state explicitly say discovered public graph, and passive discoveries now render separate Browse and Subscribe actions instead of project-row activation. Unknown/private bare IDs still fail closed to the invite path.\n- Catalogue scale: passive entries are independently hideable now. Search/pagination/retention remains separate product work rather than changing synchronization semantics in this bug fix.\n\nThe focused authorization suite passes 50/50; the earlier full agent, CLI, and UI suites also passed.

options.memberProof,
)) {
);
if (!hasAuthoritativePublicDefinition && !hasAuthoritativePrivateDefinition) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Bug: Public snapshots bypass the required member proof during private join refresh

What's wrong
The new OR condition weakens the post-approval private join path. Callers that supply memberProof are explicitly asking the refresh to prove that the approved local member delegation exists, but a public-only root definition now satisfies the refresh and is persisted without proving that membership.

Example
A private join is approved and memberProof is supplied for the local agent. If the curator snapshot contains only <cg> rdf:type dkg:ContextGraph and <cg> dkg:accessPolicy "public", with no dkg:allowedAgent delegation for this node, hasAuthoritativePublicDefinition is true and line 367 lets the snapshot replace local _meta. Before this change, the same snapshot was rejected because the approved-member delegation was missing.

Suggested direction
Gate the new public-proof acceptance on the absence of options.memberProof, or otherwise split public-subscribe refresh from approved-member private refresh so a private join cannot be satisfied by a public-only root definition.

For Agents
In packages/agent/src/curator-meta-refresh.ts, keep the public snapshot path for public subscriptions, but when options.memberProof is present preserve the private-join contract: require hasAuthoritativePrivateMetaDefinition(..., options.memberProof) or add an explicit caller option that says public downgrade is acceptable. Add a regression test where a post-approval/memberProof refresh receives a public-only snapshot and must be rejected.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 242a7cc. When memberProof is supplied, curator refresh now preserves the fail-closed private contract and requires the authoritative private definition plus the matching delegation. Public-only metadata is accepted only when no memberProof was requested. Added a refreshMetaFromCurator regression proving public-only metadata plus memberProof returns false and causes no metadata-store mutation. Focused authorization tests pass 44/44, the full agent suite passes 2550 tests (5 skipped), and agent type checks pass.

import type { Quad } from '@origintrail-official/dkg-storage';
import { stripLiteral } from './dkg-agent-utils.js';

type PublicMetaObjectRequirement =

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: The new public proof duplicates the private meta-proof DSL

What's wrong
This creates two implementations of the same concept: requirement object matching, normalized literal comparison, and ASK fragment rendering. That is avoidable indirection and makes future proof changes harder to audit because public and private authority no longer share one canonical implementation.

Example
If literal normalization changes, the author now has to update both the public matcher/query renderer and the private matcher/query renderer or the two authority paths can drift structurally.

Suggested direction
Delete the parallel public copy and parameterize the existing meta-proof model around access policy plus optional extra requirements/member proof.

For Agents
Extract the shared requirement DSL and renderer into a small meta-proof utility, then express public/private definitions as policy-specific configs. Preserve the public conflict-policy check and private member-proof extension; existing parity tests should still prove snapshot and ASK behavior match.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that this is real maintainability debt, but I intentionally deferred it from this security fix. Consolidating the shared requirement matcher and ASK renderer would change both public and private authorization proof engines without addressing another demonstrated behavioral defect. The existing snapshot/query parity tests guard the current implementations against internal drift. This PR keeps the policy evaluators separate and auditable; the low-level DSL extraction is better handled in a focused follow-up with parity coverage for both policies.

key={cg.id}
cg={cg}
isBrowsing={activeTabId === `project:${cg.id}`}
onBrowse={() => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Bug: Browsing a catalogue graph can leave actions targeting the previous graph

What's wrong
The new catalogue browse path opens a project tab without changing activeProjectId. Since other parts of the UI use activeProjectId as the active context graph for chat and project-scoped actions, a user can browse one graph while actions still apply to the previously selected graph.

Example
Select private-a from My Context Graphs, so activeProjectId === 'private-a'. Then switch to Context Oracle and click Browse on catalogue graph public-b. The center opens project:public-b, but activeProjectId still points to private-a, so right-panel chat/actions remain scoped to the previous graph while the user is looking at public-b.

Suggested direction
When Browse opens project:${cg.id}, also update the project store consistently, or clear the active project so downstream actions cannot use a stale context graph.

For Agents
In PanelLeft.tsx, update the catalogue Browse handler to keep activeProjectId coherent with the opened project tab. Decide whether browsing should activate cg.id or explicitly clear the project target, then add a test that starts from a non-null previous active project and clicks Browse on a catalogue entry.

// policy must still bypass the private membership gate.
allowedAgents: ['0x1111111111111111111111111111111111111111'],
callerAddress: '0x2222222222222222222222222222222222222222',
result: publicDurableAndSharedMemoryResult(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Bug: The public two-plane catch-up test stubs away the plane selection it claims to verify

What's wrong
The test gives false confidence for the changed public subscription readiness behavior. It verifies that a synthetic result containing shared-memory counts is surfaced, but not that the subscription route actually requested shared-memory catch-up.

Example
A regression that changed the route to call catchupRunner.run({ contextGraphId, includeSharedMemory: false }) would still get the stubbed result with sharedMemorySynced: 4, and this test would still pass.

Suggested direction
Add an assertion on the catch-up runner request, or make the fake runner fail/return different results when includeSharedMemory is false.

For Agents
In packages/cli/test/context-graph-subscribe-readiness.test.ts, capture the argument passed to daemonState.catchupRunner.run in the subscribe helper and assert this public subscription case calls it with the generated contextGraphId and includeSharedMemory: true. Keep the existing status/result assertions as downstream evidence.


describe('authoritative public metadata proof', () => {
it('keeps fetched-snapshot evaluation and the generated store query in lockstep', async () => {
const cases = [

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: Public metadata proof tests miss the named-graph boundary

What's wrong
The new public-authority proof is security-sensitive because data or unrelated graph facts must not authorize public access. The current tests do not exercise that boundary, so they would still pass if either admission path stopped requiring the _meta graph.

Example
Add a case such as mutate: quads => quads.map(q => ({ ...q, graph: contextGraphDataGraphUri(contextGraphId) })) with expected: false; both hasAuthoritativePublicMetaDefinition and buildAuthoritativePublicMetaAskQuery should reject it.

Suggested direction
Extend the parity table with a wrong-graph case so removing the quad.graph === metaGraph check or the GRAPH <metaGraph> clause fails the test.

For Agents
In packages/agent/test/context-graph-public-meta-proof.test.ts, add a negative case for a complete public definition in the wrong graph. The behavior to preserve is that only facts in the root _meta graph authorize public metadata, for both snapshot evaluation and the generated ASK query.

@lupuszr
lupuszr merged commit 7bbbd95 into testnet-canary Jul 20, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants