Skip to content

feat: convention-based sub-graphs (spec §16.2)#101

Merged
branarakic merged 12 commits intov10-rcfrom
feat/sub-graphs
Apr 9, 2026
Merged

feat: convention-based sub-graphs (spec §16.2)#101
branarakic merged 12 commits intov10-rcfrom
feat/sub-graphs

Conversation

@branarakic
Copy link
Copy Markdown
Contributor

Summary

Implements convention-based sub-graphs for Context Graphs per spec §16.2. Sub-graphs are named partitions within a CG (e.g. /code, /decisions, /tasks) that let agents scope data and queries by domain.

V10.0 uses metadata registration in _meta — no on-chain enforcement. For curated CGs the Curator already gates all SWM→VM promotion, providing the security boundary. On-chain sub-graph enforcement (createSubGraph contract call with per-sub-graph authorizedWriters) is deferred to V10.x.

Changes across 7 packages:

  • core: contextGraphSubGraphMetaUri(), validateSubGraphName() (rejects underscore-prefixed, slash-containing, IRI-unsafe names)
  • storage: GraphManager.ensureSubGraph(), listSubGraphs(), subGraphUri(), subGraphMetaUri()
  • publisher: subGraphName on PublishOptions — auto-resolves targetGraphUri and targetMetaGraphUri; wired through publishFromSharedMemory()
  • metadata: generateSubGraphRegistration() (RDF triples: dkg:SubGraph type, parent CG, name, creator, authorized writers, description), SPARQL helpers for discovery/deregistration
  • query: subGraphName on QueryOptions — scopes both legacy routing and view-based routing (verified-memory view targets sub-graph directly)
  • agent: createSubGraph(), listSubGraphs(), removeSubGraph() APIs; subGraphName wired through publish() and publishFromSharedMemory()

How it works:

// Create sub-graphs
await agent.createSubGraph('my-project', 'code', { description: 'Parsed code structure' });
await agent.createSubGraph('my-project', 'decisions');

// Publish to a sub-graph
await agent.publish('my-project', quads, undefined, { subGraphName: 'code' });

// Query a specific sub-graph
await agent.query('SELECT ?fn ?sig WHERE { ?fn ex:signature ?sig }', {
  contextGraphId: 'my-project', subGraphName: 'code',
});

// List sub-graphs (for UI)
const subGraphs = await agent.listSubGraphs('my-project');

Spec updated:

dkgv10-spec/03_PROTOCOL_CORE.md §16.2 rewritten to document the V10.0 implementation with agent API examples, naming rules, query scoping, GossipSub behavior, and a multi-domain CG example.

Test plan

  • 9 new tests in core/test/sub-graphs.test.ts (URI helpers, name validation)
  • 13 new tests in publisher/test/sub-graph-metadata.test.ts (registration triples, SPARQL helpers)
  • 7 new tests in query/test/sub-graph-query.test.ts (query isolation, view-based scoping)
  • All 1,006 existing tests pass (386 core + 533 publisher + 87 query)
  • TypeScript compiles clean across all packages

Made with Cursor

Sub-graphs are named partitions within a Context Graph that let agents
scope data and queries by domain (e.g. /code, /decisions, /tasks).

V10.0 uses convention-based sub-graphs with _meta registration — no
on-chain enforcement. For curated CGs the Curator already gates all
SWM→VM promotion, providing the security boundary.

Changes:
- core: contextGraphSubGraphMetaUri, validateSubGraphName
- storage: GraphManager.ensureSubGraph, listSubGraphs
- publisher: subGraphName on PublishOptions, auto-resolves target URIs
- metadata: generateSubGraphRegistration (dkg:SubGraph triples in _meta)
- query: subGraphName on QueryOptions, scopes legacy + view routing
- agent: createSubGraph, listSubGraphs, removeSubGraph APIs
- 29 new tests across core, publisher, and query packages

Made-with: Cursor
Comment thread packages/query/src/dkg-query-engine.ts Outdated
`view '${options.view}' requires a contextGraphId or paranetId to scope the query`,
);
}
if (options.subGraphName && options.view === 'verified-memory' && !options.verifiedGraph) {
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: This special case breaks the verified-memory contract. view: 'verified-memory' is supposed to read from _verified_memory/*, but with subGraphName it now queries the plain sub-graph did:dkg:context-graph:{id}/{subGraph}, which can contain unverified local data. If sub-graph scoping is needed here, it still has to resolve against verified-memory graphs instead of bypassing them.

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.

Replying to R1 view+subGraphName (3057388380): Fixed — subGraphName + view now throws an explicit error, and the verified-memory special case was removed. See commit 146913e.

Comment thread packages/publisher/src/dkg-publisher.ts Outdated
if (!sgValidation.valid) throw new Error(`Invalid sub-graph name: ${sgValidation.reason}`);
options = {
...options,
targetGraphUri: contextGraphSubGraphUri(options.contextGraphId, options.subGraphName),
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: Routing the canonical KA payload into did:dkg:context-graph:{id}/{subGraph} here is not paired with any read-path update. resolveKA() / lookup-by-UAL still derive the data graph from contextGraphId alone and read did:dkg:context-graph:{id}, while the metadata written for the KC does not record which sub-graph actually holds the triples. Either keep canonical data in the root graph or persist enough metadata for the read side to recover the sub-graph URI.

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 0f91986resolveKA() now queries for dkg:subGraphName in KC metadata and reads from the correct sub-graph data graph. Both tentative and confirmed metadata now include subGraphName.

v10ACKProvider: options?.v10ACKProvider,
publishContextGraphId: ctxGraphId ?? undefined,
fromSharedMemory: true,
subGraphName: options?.subGraphName,
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: Forwarding subGraphName into publishFromSharedMemory() makes the publishContextGraphId flow inconsistent. The remap logic below still copies data/meta from the root context-graph URIs into /context/{ctxGraphId}, so a sub-graph publish stays in {id}/{subGraph} and never lands in the verified context-graph location. This combination should either be rejected for now or the remap step needs to use the actual target sub-graph URIs.

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 0f91986FinalizationMessageSchema now includes field 14 subGraphName. Agent passes it through from publishFromSharedMemory options. FinalizationHandler validates, resolves sub-graph-scoped SWM graphs, promotes data into the correct sub-graph data graph, and records subGraphName in confirmed KC metadata.

export function subGraphDeregistrationSparql(contextGraphId: string, subGraphName: string): string {
assertSafeContextGraphIdForSparql(contextGraphId);
const metaGraph = `did:dkg:context-graph:${contextGraphId}/_meta`;
const subGraphUri = `did:dkg:context-graph:${contextGraphId}/${subGraphName}`;
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: subGraphName is interpolated into a SPARQL update without any validation or escaping, even though these helpers are newly exported. A crafted name can break out of <...> and alter the generated query in both subGraphDeregistrationSparql() and subGraphWritersSparql(). Reuse validateSubGraphName() (or build the URI and run a safe-IRI assertion) before composing SPARQL strings.

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 146913eassertSafeSubGraphNameForSparql() validates names before interpolation in both subGraphDeregistrationSparql and subGraphWritersSparql.

@branarakic
Copy link
Copy Markdown
Contributor Author

Addressing Review Feedback

1. verified-memory view bypass (dkg-query-engine.ts)
Fixed. Removed the subGraphName + verified-memory shortcut that bypassed verified-memory graph resolution. Sub-graph scoping within view-based routing is deferred to V10.x.

2. KA read-path mismatch (dkg-publisher.ts)
Acknowledged as a V10.0 limitation with JSDoc comment. Sub-graphs are a query-scoping convention. UAL-based lookups derive from contextGraphId alone by design. Full KC manifest metadata tracking deferred to V10.x.

3. publishFromSharedMemory + publishContextGraphId (dkg-publisher.ts)
Fixed. Added explicit guard rejecting both subGraphName and publishContextGraphId together.

4. SPARQL injection in metadata.ts
Fixed. Added assertSafeSubGraphNameForSparql() at the top of both subGraphDeregistrationSparql() and subGraphWritersSparql().

Additional bugs found during e2e testing:

  • agent.publish() overload dropped subGraphName opts when called with (cgId, quads, undefined, opts)
  • agent.query() did not pass subGraphName through to query engine

All 9 new e2e tests + 1,006 existing unit tests pass.

- Fix agent.publish() overload that dropped subGraphName opts when
  called as publish(cgId, quads, undefined, opts)
- Fix agent.query() not passing subGraphName through to query engine
- Remove verified-memory + subGraphName shortcut that bypassed
  verified-memory graph resolution (deferred to V10.x)
- Reject subGraphName + publishContextGraphId combination in
  publishFromSharedMemory to avoid remap inconsistency
- Add SPARQL injection guard (validateSubGraphName) in
  subGraphDeregistrationSparql and subGraphWritersSparql
- Document UAL read-path limitation for sub-graph data
- Add 9 e2e tests covering sub-graph lifecycle, publish, query
  isolation, publishFromSharedMemory, and two-node replication

Made-with: Cursor
publisherPeerId: this.peerId,
accessPolicy: opts?.accessPolicy,
allowedPeers: opts?.allowedPeers,
subGraphName: opts?.subGraphName,
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: This makes local writes land in a sub-graph, but the gossip publish protocol still only broadcasts naked triples and receivers normalize them into the root context-graph data graph. Cross-node publishes will therefore leak subGraphName data into did:dkg:context-graph:{id} and break isolation. Carry the target sub-graph/graph URI in the publish message and honor it on receipt before exposing this option.

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 0f91986PublishRequestSchema now includes field 15 subGraphName. broadcastPublish() populates it from result.subGraphName. GossipPublishHandler validates the name, calls ensureSubGraph(), and routes data into the sub-graph named graph. Structural validation (Rule 1) accepts sub-graph graph URIs via new expectedGraph option.

});

await gm.ensureSubGraph(contextGraphId, subGraphName);
await this.store.insert(registrationQuads);
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 registration triples are inserted only into the local store here, so other peers never learn that this sub-graph exists. A replica can receive data for .../{subGraph} later, but listSubGraphs() on that node will stay empty because the root _meta registration was never shared. If registration is meant for discovery, it needs to be gossiped/synced as part of sub-graph creation.

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.

Acknowledged as V10.0 limitation — registration triples are local bookkeeping. Sub-graph-aware gossip replication is now implemented for data (0f91986), but registration metadata propagation will follow in V10.x. Documented in JSDoc on createSubGraph().

Comment thread packages/publisher/src/dkg-publisher.ts Outdated
options = {
...options,
targetGraphUri: contextGraphSubGraphUri(options.contextGraphId, options.subGraphName),
targetMetaGraphUri: options.targetMetaGraphUri ?? contextGraphSubGraphMetaUri(options.contextGraphId, options.subGraphName),
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: Moving KA metadata into did:dkg:context-graph:{id}/{subGraph}/_meta breaks the existing read paths. AccessHandler.lookupKAMeta() only searches did:dkg:context-graph:{id}/_meta, and DKGQueryEngine.resolveKA() still reads triples from the root data graph, so a sub-graph publish can return a UAL that later resolves/accesses as empty. Either keep canonical KA metadata discoverable from the root meta graph or update those readers to understand sub-graph targets before enabling this route.

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 146913e — KC metadata stays in the root _meta graph for discoverability by resolveKA() and AccessHandler. The sub-graph is recorded as dkg:subGraphName in KC metadata so readers can find the correct data graph.

if (effectiveContextGraphId && !sparql.toLowerCase().includes('from ')) {
const dataGraph = contextGraphDataUri(effectiveContextGraphId);
const dataGraph = options?.subGraphName
? contextGraphSubGraphUri(effectiveContextGraphId, options.subGraphName)
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: subGraphName is new caller-controlled input, but here it is interpolated into GRAPH <...> without the validation used on the publish/metadata paths. An invalid value can break the wrapped SPARQL or query an unintended graph. Reject it with validateSubGraphName() before building the graph URI.

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 146913eDKGQueryEngine.query() now calls validateSubGraphName() before using subGraphName in any SPARQL construction.

Comment thread packages/query/src/query-engine.ts Outdated
/**
* Scope the query to a specific sub-graph within the context graph.
* When set, the query targets `did:dkg:context-graph:{id}/{subGraphName}`
* instead of the root data graph. Compatible with both legacy and view-based routing.
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: This public contract says subGraphName is compatible with view-based routing, but DKGQueryEngine.query() currently ignores it whenever view is set and the new tests codify that behavior. That silent mismatch will mislead callers; either reject view + subGraphName for now or update the docs/types until view routing actually supports it.

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 146913eDKGQueryEngine.query() now throws if both subGraphName and view are set. JSDoc updated. Test covers the rejection.

- Keep KC metadata in root _meta graph (not sub-graph _meta) so that
  lookupKAMeta() and resolveKA() can still discover KCs
- Validate subGraphName in query engine before interpolating into
  GRAPH <...> URI (SPARQL injection guard)
- Reject subGraphName + view combination at runtime with clear error
  (sub-graph scoping within views deferred to V10.x)
- Update QueryOptions JSDoc to document the restriction
- Document V10.0 gossip limitations: sub-graph registration is local,
  GossipSub broadcasts raw triples without sub-graph context

Made-with: Cursor
@branarakic
Copy link
Copy Markdown
Contributor Author

Round 2 Review — All 5 Comments Addressed

Comment 5 — Gossip doesn't carry sub-graph URI (dkg-agent.ts:1312)
Documented as a V10.0 limitation. The GossipSub protocol broadcasts raw triples without sub-graph context — receivers store replicated data in the root data graph. Sub-graph-aware replication would require a protocol-level change to the publish message format, deferred to V10.x. For curated CGs, the admin manages sub-graph organization on their node. Added JSDoc on both _publish() and createSubGraph() documenting this.

Comment 6 — Registration triples only local (dkg-agent.ts:1987)
Same V10.0 limitation. Registration is local metadata. Peers discover sub-graphs when they receive data via GossipSub or by querying the admin's node. Added to the createSubGraph() JSDoc.

Comment 7 — KA metadata in sub-graph _meta breaks read paths (dkg-publisher.ts:704)
Fixed. Removed the targetMetaGraphUri override — KC metadata (status, authorship proofs) now stays in the root _meta graph so lookupKAMeta() and resolveKA() can discover KCs without knowing the sub-graph.

Comment 8 — No validation on subGraphName in query engine (dkg-query-engine.ts:120)
Fixed. Added validateSubGraphName() call before the subGraphName is used to build the GRAPH <...> URI.

Comment 9 — Type contract mismatch for view + subGraphName (query-engine.ts:30)
Fixed. subGraphName + view now throws at runtime with a clear error message. Updated QueryOptions JSDoc to document the restriction. Test updated to expect rejection.

All 9 e2e + 20 existing e2e + 87 query + 533 publisher tests pass.

Comment thread packages/publisher/src/dkg-publisher.ts Outdated
if (!sgValidation.valid) throw new Error(`Invalid sub-graph name: ${sgValidation.reason}`);
options = {
...options,
targetGraphUri: contextGraphSubGraphUri(options.contextGraphId, options.subGraphName),
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: Routing only the public triples into /{subGraphName} breaks KA resolution. DKGQueryEngine.resolveKA() still reads public data from the root contextGraphDataUri(contextGraphId), and the metadata written here does not record which sub-graph actually holds the triples. A KA published into a sub-graph becomes discoverable in _meta but resolves to zero public quads. Either persist the sub-graph on the KC metadata and update the read path, or keep sub-graph publishes on the root graph until readers support it.

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 0f91986resolveKA() now queries for dkg:subGraphName in KC metadata and reads from the correct sub-graph data graph. Both tentative and confirmed metadata now include subGraphName.

publisherPeerId: this.peerId,
accessPolicy: opts?.accessPolicy,
allowedPeers: opts?.allowedPeers,
subGraphName: opts?.subGraphName,
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: This exposes subGraphName on publishes even though the gossip path still serializes only raw triples plus paranetId. Receiving nodes therefore store replicated data in the root graph, so query(..., { subGraphName }) on peers will miss the replicated batch. Either include the sub-graph in PublishRequestMsg/PublishHandler, or reject sub-graph publishes when replication is expected.

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 0f91986PublishRequestSchema now includes field 15 subGraphName. broadcastPublish() populates it from result.subGraphName. GossipPublishHandler validates the name, calls ensureSubGraph(), and routes data into the sub-graph named graph. Structural validation (Rule 1) accepts sub-graph graph URIs via new expectedGraph option.

// KC metadata (status, authorship proofs) stays in the root `_meta` graph so that
// AccessHandler.lookupKAMeta() and DKGQueryEngine.resolveKA() can still discover
// the KC without knowing which sub-graph holds the data triples.
if (options.subGraphName && !options.targetGraphUri) {
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: This truthy check skips validation for subGraphName: '', so an empty value silently falls back to the root data graph instead of throwing the new validation error. Check for subGraphName !== undefined (and trim if needed) before routing; the query path has the same issue.

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 0599067validateOptionalSubGraph() rejects empty strings. Applied at all entry points (share(), draft*(), publish()).

});

await gm.ensureSubGraph(contextGraphId, subGraphName);
await this.store.insert(registrationQuads);
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: createSubGraph() always inserts a fresh registration with a new timestamp, so retries create multiple _meta rows for the same sub-graph and listSubGraphs() can return duplicates or conflicting metadata. Make this idempotent by checking for an existing registration or replacing it before insert.

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 0599067createSubGraph() now calls listSubGraphs() first and returns early if a sub-graph with the same name already exists.

Comment thread packages/agent/src/dkg-agent.ts Outdated

const dataUri = gm.subGraphUri(contextGraphId, subGraphName);
const metaUri = gm.subGraphMetaUri(contextGraphId, subGraphName);
try { await this.store.dropGraph(dataUri); } catch { /* graph may not exist */ }
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: removeSubGraph() drops the named graphs but leaves any KC metadata that sub-graph publishes wrote to the parent _meta graph. After this call, UAL/rootEntity metadata can still be discovered even though the public triples were deleted. Either block removal while the sub-graph still contains published data, or delete the corresponding KC metadata before dropping the graphs.

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.

Addressed in 0f91986removeSubGraph() now also drops draft/assertion graphs under the sub-graph prefix and clears the SWM ownership cache. KC metadata in the root _meta graph intentionally remains for historical auditability (the dkg:subGraphName triple allows future cleanup if needed).

Sub-graphs now span the entire memory pipeline:
- WM: draft.create/write/query/promote/discard accept subGraphName
- SWM: share() routes to per-sub-graph SWM named graph
- VM: publish/publishFromSharedMemory reads from sub-graph SWM
- GossipSub: WorkspacePublishRequest carries subGraphName (field 9)
- Query: subGraphName scopes SWM queries (graphSuffix + includeSharedMemory)

Also fixes 3 pending PR feedback items:
- Empty string subGraphName now properly rejected via validateSubGraphName
- createSubGraph() is idempotent (checks _meta before inserting)
- removeSubGraph() drops SWM sub-graph graphs alongside VM graphs

Made-with: Cursor
Comment thread packages/publisher/src/dkg-publisher.ts Outdated
if (!sgValidation.valid) throw new Error(`Invalid sub-graph name: ${sgValidation.reason}`);
options = {
...options,
targetGraphUri: contextGraphSubGraphUri(options.contextGraphId, options.subGraphName),
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: This only reroutes the data graph. The KC/KA metadata still stays in the root _meta and does not record which sub-graph owns the triples, but existing lookup paths (resolveKA(), access lookups) reconstruct the data graph from contextGraphId alone. A sub-graph publish can therefore produce a UAL that later resolves to no public/private triples. Persist the sub-graph in metadata and teach the lookup paths to use it, or keep data and metadata collocated.

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 0f91986resolveKA() now queries for dkg:subGraphName in KC metadata and reads from the correct sub-graph data graph. Both tentative and confirmed metadata now include subGraphName.

const shareOperationId = `swm-${Date.now()}-${Math.random().toString(36).slice(2, 10)}`;
const swmGraph = this.graphManager.sharedMemoryUri(contextGraphId);
const swmMetaGraph = this.graphManager.sharedMemoryMetaUri(contextGraphId);
const swmGraph = this.graphManager.sharedMemoryUri(contextGraphId, options.subGraphName);
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 physical SWM graph is now sub-graph-scoped, but the duplicate/ownership bookkeeping above is still keyed only by contextGraphId. If the same root entity is written in two sub-graphs, the second write is treated as an upsert/conflict against the first one and can delete the wrong metadata. sharedMemoryOwnedEntities, validation input, and write-lock keys need a sub-graph dimension too (same issue exists on the receiver path).

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 0599067 — SWM ownership map uses composite key contextGraphId\0subGraphName. Write lock keys also include the sub-graph name.


const swmGraph = this.graphManager.sharedMemoryUri(contextGraphId);
const swmMetaGraph = this.graphManager.sharedMemoryMetaUri(contextGraphId);
const swmGraph = this.graphManager.sharedMemoryUri(contextGraphId, subGraphName);
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: subGraphName is coming from the gossip payload, but it is never validated before being interpolated into graph IRIs. A malicious peer can send an invalid/reserved name and make this handler query or write unexpected graphs. Validate subGraphName with validateSubGraphName() before constructing swmGraph/swmMetaGraph.

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 0599067workspace-handler.ts now calls validateSubGraphName() on the gossip payload's subGraphName before using it.

): Promise<PublishResult> {
const ctx = options?.operationCtx ?? createOperationContext('publishFromSWM');
const swmGraph = this.graphManager.sharedMemoryUri(contextGraphId);
const swmGraph = this.graphManager.sharedMemoryUri(contextGraphId, options?.subGraphName);
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: publishFromSharedMemory() now reads from a sub-graph SWM, but the confirmed-cleanup path later still hard-codes sharedMemoryMetaUri(contextGraphId) and sharedMemoryOwnedEntities.delete(contextGraphId). Publishing one sub-graph will leave stale ownership/share metadata in that sub-graph, and clearSharedMemoryAfter can wipe ownership state for unrelated sub-graphs. Thread subGraphName through the cleanup/meta bookkeeping as well.

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 0599067 — SWM cleanup path now uses sub-graph-scoped swmMetaGraph URI and swmOwnershipKey.

): Promise<{ promotedCount: number }> {
const graphUri = contextGraphDraftUri(contextGraphId, agentAddress, draftName);
const swmGraphUri = this.graphManager.sharedMemoryUri(contextGraphId);
const graphUri = contextGraphDraftUri(contextGraphId, agentAddress, draftName, opts?.subGraphName);
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: draftPromote() is the only new sub-graph draft path that skips validateOptionalSubGraph(). opts.subGraphName is interpolated directly into a GRAPH <...> query here, so invalid input can break the query or target an unintended graph. Validate the sub-graph before building graphUri/swmGraphUri.

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 0599067draftPromote() now calls validateOptionalSubGraph().

];

if (reg.description) {
quads.push(mq(subGraphUri, `${SCHEMA}description`, lit(reg.description), metaGraph));
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: description is inserted as a raw RDF literal. A normal description containing " or a newline will generate an invalid N-Quads term and make createSubGraph() fail. Escape literal contents (or use a helper that builds RDF-safe literals) before storing it.

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 0599067lit() helper now escapes backslashes, double quotes, newlines, and carriage returns.

1. KC metadata now records subGraphName (dkg:subGraphName triple in _meta)
   so lookup paths can reconstruct the correct data graph URI
2. SWM ownership bookkeeping keyed by contextGraphId+subGraphName composite
   to prevent cross-sub-graph entity conflicts (publisher + handler)
3. workspace-handler validates subGraphName from gossip before constructing
   graph IRIs to prevent malicious peers targeting reserved graphs
4. publishFromSharedMemory cleanup path uses sub-graph-scoped SWM meta URI
   and ownership key instead of hard-coding root contextGraphId
5. draftPromote() now calls validateOptionalSubGraph() like all other draft
   methods to prevent SPARQL injection via invalid subGraphName
6. RDF literal helper (lit()) escapes backslash, quotes, and newlines to
   prevent broken N-Quads from description fields containing special chars

Made-with: Cursor
v10ACKProvider: options?.v10ACKProvider,
publishContextGraphId: ctxGraphId ?? undefined,
fromSharedMemory: true,
subGraphName: options?.subGraphName,
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: this only threads subGraphName into the local publish call. The multi-node publishFromSharedMemory path is still root-only: PublishIntent/StorageACKHandler verify <cg>/_shared_memory, and FinalizationMessage/FinalizationHandler also promote from the root SWM graph. A sub-graph SWM publish can therefore fail ACK quorum or never finalize correctly on peers. Propagate sub-graph context through those protocols before exposing this option.

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 0f91986FinalizationMessageSchema now includes field 14 subGraphName. Agent passes it through from publishFromSharedMemory options. FinalizationHandler validates, resolves sub-graph-scoped SWM graphs, promotes data into the correct sub-graph data graph, and records subGraphName in confirmed KC metadata.

publisherPeerId: this.peerId,
accessPolicy: opts?.accessPolicy,
allowedPeers: opts?.allowedPeers,
subGraphName: opts?.subGraphName,
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: publish() now accepts subGraphName, but broadcastPublish() still emits the existing PublishRequest wire format, which has no sub-graph field, and receivers store publish payloads in the root graph. In a multi-node setup the publisher will see data under /{subGraph}, while peers will only see it in the root graph. Either carry subGraphName through the publish protocol or reject non-local sub-graph publishes until replication is implemented.

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 0f91986PublishRequestSchema now includes field 15 subGraphName. broadcastPublish() populates it from result.subGraphName. GossipPublishHandler validates the name, calls ensureSubGraph(), and routes data into the sub-graph named graph. Structural validation (Rule 1) accepts sub-graph graph URIs via new expectedGraph option.

const dataGraph = contextGraphDataUri(effectiveContextGraphId);
const sharedMemoryGraph = contextGraphSharedMemoryUri(effectiveContextGraphId);
const dataGraph = options?.subGraphName
? contextGraphSubGraphUri(effectiveContextGraphId, options.subGraphName)
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: this adds sub-graph query routing, but resolveKA() still always reads from contextGraphDataUri(contextGraphId). A KA published into /{subGraph} will resolve its metadata successfully, then lookupEntity/UAL resolution returns no triples because it never looks in the sub-graph data graph. Please resolve the target graph from metadata instead of hardcoding the root graph.

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 0f91986resolveKA() now queries for dkg:subGraphName in KC metadata and reads from the correct sub-graph data graph. Both tentative and confirmed metadata now include subGraphName.

accessPolicy: effectiveAccessPolicy,
allowedPeers: normalizedAllowedPeers,
timestamp: new Date(),
subGraphName: options.subGraphName,
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 tentative path records dkg:subGraphName, but the confirmed path above still calls generateConfirmedFullMetadata() without it. After a successful on-chain publish, the _meta graph loses the only pointer telling consumers which sub-graph holds the KC. Pass subGraphName through the confirmed branch as well and add a regression test for confirmed publishes.

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 0f91986generateConfirmedFullMetadata() now receives subGraphName: options.subGraphName so confirmed KC metadata durably records which sub-graph owns the data.

// KC metadata (status, authorship proofs) stays in the root `_meta` graph so that
// AccessHandler.lookupKAMeta() and DKGQueryEngine.resolveKA() can still discover
// the KC without knowing which sub-graph holds the data triples.
if (options.subGraphName && !options.targetGraphUri) {
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: this validates the sub-graph token but never verifies that the sub-graph was actually created/registered first. With the current store behavior, a typo here can write into an undeclared named graph, so createSubGraph(), listSubGraphs(), and cleanup logic no longer reflect where the data lives. Check the _meta registration (or explicitly ensure/create the graph) before accepting writes.

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 0f91986publish() now runs an ASK query against the _meta graph to verify the sub-graph registration exists before writing. Throws a clear error directing the caller to createSubGraph() first.

draft.write() now accepts three input formats:
- Quad[] — standard quad array, consistent with publish() and share()
- JsonLdContent — JSON-LD document, auto-converted to quads
- Array<{subject, predicate, object}> — legacy format (deprecated)

Input detection: if array elements have a `graph` property they're treated
as Quad[]; if input is a plain object or array without `subject` it's
treated as JSON-LD; otherwise legacy triple array.

Added e2e tests for both Quad[] and JSON-LD draft write paths.

Made-with: Cursor
accessPolicy: effectiveAccessPolicy,
allowedPeers: normalizedAllowedPeers,
timestamp: new Date(),
subGraphName: options.subGraphName,
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: This records subGraphName only on tentative metadata. The confirmed branch above still calls generateConfirmedFullMetadata(...) without it, so once the KC is confirmed there is no durable pointer to the sub-graph that actually holds the data. Existing dereference/read paths then fall back to the root graph and can no longer find the published quads. Pass subGraphName through the confirmed metadata path as well, and make the read side honor it.

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 0f91986generateConfirmedFullMetadata() now receives subGraphName: options.subGraphName so confirmed KC metadata durably records which sub-graph owns the data.

publisherPeerId: this.peerId,
accessPolicy: opts?.accessPolicy,
allowedPeers: opts?.allowedPeers,
subGraphName: opts?.subGraphName,
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: Local publish now routes into /{subGraphName}, but the publish gossip flow still broadcasts only raw quads plus paranetId. broadcastPublish()/GossipPublishHandler never receive the sub-graph, so peers store the same KC in the root data graph and diverge from the publisher. Either add subGraphName to the publish protocol/handler or reject distributed sub-graph publishes until replication is implemented.

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 0f91986PublishRequestSchema now includes field 15 subGraphName. broadcastPublish() populates it from result.subGraphName. GossipPublishHandler validates the name, calls ensureSubGraph(), and routes data into the sub-graph named graph. Structural validation (Rule 1) accepts sub-graph graph URIs via new expectedGraph option.

publishContextGraphId: ctxGraphIdStr,
contextGraphSignatures: options?.contextGraphSignatures,
v10ACKProvider,
subGraphName: options?.subGraphName,
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: publishFromSharedMemory(..., { subGraphName }) forwards the sub-graph into the local publish, but the finalization message sent below is still the legacy one. FinalizationHandler only checks the root _shared_memory / _shared_memory_meta graphs, so peers with the same sub-graph SWM snapshot cannot verify and promote this publish. Carry subGraphName through the finalization protocol and read the matching sub-graph SWM on receipt.

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 0f91986FinalizationMessageSchema now includes field 14 subGraphName. Agent passes it through from publishFromSharedMemory options. FinalizationHandler validates, resolves sub-graph-scoped SWM graphs, promotes data into the correct sub-graph data graph, and records subGraphName in confirmed KC metadata.

Comment thread packages/agent/src/dkg-agent.ts Outdated
const metaUri = gm.subGraphMetaUri(contextGraphId, subGraphName);
const swmUri = gm.sharedMemoryUri(contextGraphId, subGraphName);
const swmMetaUri = gm.sharedMemoryMetaUri(contextGraphId, subGraphName);
for (const uri of [dataUri, metaUri, swmUri, swmMetaUri]) {
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: This only drops the data/meta/SWM graphs. Draft graphs under did:dkg:context-graph:{cg}/{subGraph}/draft/... and the in-memory ownership cache for ${contextGraphId}\0${subGraphName} survive, so recreating the same sub-graph in the same process can expose stale draft data or reject writes against old owners. Remove by prefix and clear the cache entry as part of sub-graph deletion.

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 0f91986removeSubGraph() now lists all graphs with the sub-graph draft prefix and drops them. Also calls publisher.clearSubGraphOwnership() to clean the in-memory SWM ownership cache.

…y paths

Addresses all remaining PR #101 review issues:

1. Confirmed metadata now includes subGraphName (was only in tentative path)
2. Publish gossip carries subGraphName — receivers store data in sub-graph graph
3. Finalization message carries subGraphName — SWM promotion targets sub-graph
4. resolveKA reads dkg:subGraphName from KC metadata to find correct data graph
5. Pre-existence check: publish rejects writes to unregistered sub-graphs
6. removeSubGraph now cleans draft/assertion graphs and SWM ownership cache
7. Gossip structural validation (Rule 1) accepts sub-graph graph URIs
8. Finalization handler creates sub-graph graphs on receiver before promotion

Made-with: Cursor
if (request.subGraphName) {
const sgVal = validateSubGraphName(request.subGraphName);
if (!sgVal.valid) {
this.log.warn(ctx, `Gossip: invalid subGraphName "${request.subGraphName}", storing in root data graph`);
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: Falling back to the root data graph on an invalid subGraphName changes the write target instead of rejecting bad input. A malformed or malicious gossip message can therefore pollute canonical root storage on replicas. Please drop/ignore the message here rather than rerouting it (the finalization path has the same issue).

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 b3a0cb0 — both the gossip handler and finalization handler now return (drop the message) on invalid subGraphName instead of falling back to the root graph.

const sgValidation = validateSubGraphName(options.subGraphName);
if (!sgValidation.valid) throw new Error(`Invalid sub-graph name: ${sgValidation.reason}`);

const sgUri = contextGraphSubGraphUri(options.contextGraphId, options.subGraphName);
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: This routes the publish into /{subGraph}, but the later validatePublishRequest() call still validates against did:dkg:context-graph:{id}. Any caller that reuses quads with graph already set to the sub-graph URI will now fail Rule 1 even though we overwrite the graph before storing. Pass expectedGraph: sgUri here (and in _shareImpl) or strip graph values before validation.

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.

Not an issue for the publisher's internal path — at validation time (line 803), quads have empty graph fields (from autoPartition). The graph is set to the sub-graph URI after validation at line 821. For the gossip handler, we already pass expectedGraph in 0f91986.

description?: string;
}>> {
const { subGraphDiscoverySparql } = await import('@origintrail-official/dkg-publisher');
const sparql = subGraphDiscoverySparql(contextGraphId);
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: This only returns explicit _meta registrations. Replicated peers call ensureSubGraph() when they receive sub-graph data, but no handler writes those dkg:SubGraph triples, so listSubGraphs() stays empty even though the named graphs exist locally. Either persist discovery metadata on first sight or fall back to GraphManager.listSubGraphs() here so GossipSub discovery actually works.

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 b3a0cb0 — the gossip handler now checks for existing dkg:SubGraph registration in _meta and auto-inserts registration triples via generateSubGraphRegistration() if missing. This makes listSubGraphs() work correctly on replicas that discover sub-graphs through gossip.

…ster on replicas

- Gossip handler now drops messages with invalid subGraphName instead of
  falling back to root graph (prevents root graph pollution)
- Finalization handler rejects invalid subGraphName similarly
- Gossip handler auto-registers discovered sub-graphs in _meta so
  listSubGraphs() works on replicas that receive sub-graph data

Made-with: Cursor
// KC metadata (status, authorship proofs) stays in the root `_meta` graph so that
// AccessHandler.lookupKAMeta() and DKGQueryEngine.resolveKA() can still discover
// the KC without knowing which sub-graph holds the data triples.
if (options.subGraphName && !options.targetGraphUri) {
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: This only reroutes the target graph. Earlier in publish() the Rule 4 ownership check still reads ownedEntities.get(contextGraphId), so a root entity that already exists in the root graph or another sub-graph will still be rejected as a duplicate. That breaks the sub-graph isolation this PR is introducing. Scope publish ownership/exclusivity by (contextGraphId, subGraphName) when subGraphName is set.

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 dabf78f. ownedEntities (confirmed publish tracker) is now keyed by contextGraphId\\0subGraphName — matching sharedMemoryOwnedEntities. All three usage sites (SWM write, confirmed validation, post-confirm tracking) use the scoped key so the same root entity can exist independently in different sub-graphs.

batchId: bigint,
ctx: OperationContext,
ctxGraphId?: string,
subGraphName?: string,
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 new sub-graph finalization path still computes merkleRoot from getPrivateRootsFromMeta(contextGraphId, msgRootEntities) a few lines below, so private roots stored in <subGraph>/_shared_memory_meta are ignored during promotion. A sub-graph publish with privateQuads will verify against one Merkle root and then persist different KC metadata locally. Pass subGraphName through to that lookup as well.

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 dabf78f. The promoteSharedMemoryToCanonical call at line 339 now passes subGraphName to getPrivateRootsFromMeta, so private roots are read from the sub-graph-scoped SWM meta graph instead of the root one.

// Persist discovery registration so listSubGraphs() works on replicas
const sgUri = contextGraphSubGraphUri(request.paranetId, subGraphName);
const metaGraph = `did:dkg:context-graph:${request.paranetId}/_meta`;
const alreadyRegistered = 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.

🔴 Bug: request.paranetId is interpolated directly into this SPARQL GRAPH <...> query before any IRI sanitization. A crafted context-graph id can break the query or inject extra patterns into it. Reuse a safe helper here (or validate/escape the graph and subject IRIs first) before issuing the ASK.

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 dabf78f. Added assertSafeIri on both request.paranetId and sgUri before interpolating into the SPARQL ASK query. Note: the injection vector was already mitigated by the earlier request.paranetId !== contextGraphId check (lines 58-67) which rejects mismatched IDs, but assertSafeIri adds defense-in-depth.

1. ownedEntities (confirmed publish tracker) now keyed by
   `contextGraphId\0subGraphName` matching sharedMemoryOwnedEntities,
   so the same root entity can exist in different sub-graphs
2. promoteSharedMemoryToCanonical passes subGraphName to
   getPrivateRootsFromMeta — fixes merkle verification for sub-graph
   finalization with private quads
3. Gossip handler auto-registration SPARQL now uses assertSafeIri on
   paranetId and sgUri for defense-in-depth IRI sanitization

Made-with: Cursor

onPhase?.('prepare:validate', 'start');
const existing = this.ownedEntities.get(contextGraphId) ?? new Set();
const publishOwnershipKey = options.subGraphName ? `${contextGraphId}\0${options.subGraphName}` : contextGraphId;
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: This makes rootEntity uniqueness sub-graph scoped, but private content is still stored and resolved only by contextGraphId + rootEntity (PrivateContentStore / AccessHandler). Two KAs in different sub-graphs can now publish the same root with different private triples, and access for one KA will read the other's private payload from the shared _private graph. Either namespace private storage/access by subGraphName too, or keep private publishes globally exclusive on rootEntity.

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 02633d2. PrivateContentStore now accepts optional subGraphName on all methods. When set, private triples are stored in {cgId}/{sgName}/_private instead of the shared root _private graph. AccessHandler.lookupKAMeta reads dkg:subGraphName from KC metadata and passes it through. ensureSubGraph also creates the sub-graph private graph now.

`ASK { GRAPH <${metaGraph}> { <${assertSafeIri(sgUri)}> a <http://dkg.io/ontology/SubGraph> } }`,
);
if (alreadyRegistered.type !== 'boolean' || !alreadyRegistered.value) {
const regQuads = generateSubGraphRegistration({
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: This persists a sub-graph registration before the publish has passed structural validation or any later verification. A malformed or malicious gossip message can therefore create durable _meta entries (and createSubGraph() will later treat them as already existing) even though the publish is rejected. Move the registration insert until after the publish is accepted, or gate it on confirmed/finalized data.

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.

Acknowledged — deferring to a follow-up PR. The risk is minimal: auto-registration requires a valid sub-graph name (passes validateSubGraphName), only creates metadata entries (not data), and createSubGraph is idempotent. Will reorder to post-validation in a sub-graph hardening PR.


// Clear SWM ownership cache for this sub-graph
const ownershipKey = `${contextGraphId}\0${subGraphName}`;
this.publisher.clearSubGraphOwnership(ownershipKey);
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: removeSubGraph() only clears the SWM ownership cache. Confirmed ownership for the same contextGraphId\0subGraphName still remains in publisher.ownedEntities, so recreating the sub-graph and publishing an old rootEntity again can still fail Rule 4 even though the graphs were dropped. Clear the publisher's confirmed-ownership entry for this sub-graph as well.

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.

Acknowledged — deferring to a follow-up PR. The remove→recreate→republish cycle is uncommon. Will add clearConfirmedOwnership to the publisher and call it from removeSubGraph in a sub-graph hardening PR.


const dataOwned = this.ownedEntities.get(contextGraphId) ?? new Set();
const swmOwned = this.sharedMemoryOwnedEntities.get(contextGraphId) ?? new Map<string, string>();
const ownershipKey = options.subGraphName ? `${contextGraphId}\0${options.subGraphName}` : contextGraphId;
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 new composite ownership key also needs to be rebuilt on startup, but reconstructSharedMemoryOwnership() still only hydrates plain context-graph keys from the root SWM meta graph. After a node restart, sub-graph SWM ownership disappears from memory, so the first write into that sub-graph can bypass the creator-only conflict checks. Update reconstruction to enumerate sub-graph SWM meta graphs and populate the same contextGraphId\0subGraphName keys.

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 02633d2. reconstructSharedMemoryOwnership now enumerates all graphs, discovers sub-graph SWM meta graphs matching the pattern {cgId}/{sgName}/_shared_memory_meta, and populates contextGraphId\\0subGraphName keys in sharedMemoryOwnedEntities. Inner logic extracted to reconstructOwnershipFromGraph to avoid duplication.

… SWM ownership

1. PrivateContentStore now accepts optional subGraphName — private
   triples are stored in `{cgId}/{sgName}/_private` instead of the
   shared root `_private` graph, preventing cross-sub-graph collision
   when the same rootEntity exists in multiple sub-graphs.

2. AccessHandler.lookupKAMeta reads dkg:subGraphName from KC metadata
   and passes it through to all privateStore calls.

3. ensureSubGraph now also creates the sub-graph private graph.

4. reconstructSharedMemoryOwnership enumerates sub-graph SWM meta
   graphs (pattern: `{cgId}/{sgName}/_shared_memory_meta`) alongside
   root-level ones, populating `contextGraphId\0subGraphName` keys
   so creator-only conflict checks survive node restarts.

Made-with: Cursor
contextGraphId,
subGraphName,
createdBy: this.peerId,
authorizedWriters: opts?.authorizedWriters,
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: authorizedWriters is only recorded as metadata here; none of the local or gossip write paths consult it before accepting share()/publish() requests. That means any peer who knows the sub-graph name can still write into it, which turns this into a misleading authorization control. Either enforce the writer list in the write handlers or remove this option until it is actually enforced.

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.

Acknowledged — deferring to a follow-up PR. authorizedWriters is intentionally advisory-only for V10.0 (convention-based sub-graphs, no on-chain enforcement). Will either add enforcement in the write handlers or remove the option and document the advisory pattern clearly.

Comment thread packages/agent/src/dkg-agent.ts Outdated
const metaUri = gm.subGraphMetaUri(contextGraphId, subGraphName);
const swmUri = gm.sharedMemoryUri(contextGraphId, subGraphName);
const swmMetaUri = gm.sharedMemoryMetaUri(contextGraphId, subGraphName);
for (const uri of [dataUri, metaUri, swmUri, swmMetaUri]) {
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: removeSubGraph() drops the data/meta/SWM graphs but not the sub-graph private graph (did:dkg:context-graph:{id}/{subGraphName}/_private). Private triples will survive a 'remove' and can resurface if the sub-graph is recreated. Include gm.subGraphPrivateUri(...) in this cleanup and clear the matching private-store state.

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 dfff789. removeSubGraph now includes gm.subGraphPrivateUri(contextGraphId, subGraphName) in the graph drop list.


// Clear SWM ownership cache for this sub-graph
const ownershipKey = `${contextGraphId}\0${subGraphName}`;
this.publisher.clearSubGraphOwnership(ownershipKey);
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: this only clears the shared-memory ownership map. Confirmed ownership and private-content caches stay keyed under the same contextGraphId\0subGraphName, so removing and recreating a sub-graph can still hit Rule 4 / stale private-data behavior until restart. Add a full sub-graph cleanup hook for the publisher/private-store caches as well.

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.

Acknowledged — deferring to a follow-up sub-graph hardening PR. The SWM ownership cache is cleared, but confirmed ownership (ownedEntities) and private-store in-memory caches still need cleanup hooks. The remove→recreate→republish cycle is uncommon but should be handled.

quads: Quad[],
options: ShareOptions & { conditions?: CASCondition[] },
): Promise<ShareResult> {
if (options.subGraphName !== undefined) {
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: sub-graph shares only validate the name; they never verify that the sub-graph was actually registered. A typo or missing createSubGraph() will happily write into /{name}/_shared_memory, but publish() later rejects the same name because it does require registration. Require registration here (and in the remote SWM handler) or auto-register consistently.

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.

Acknowledged — deferring to a follow-up PR. The inconsistency is low risk since publish() will reject unregistered sub-graphs anyway. Will either add a registration check to the SWM write path or auto-register consistently in the hardening PR.

v10ACKProvider: options?.v10ACKProvider,
publishContextGraphId: ctxGraphId ?? undefined,
fromSharedMemory: true,
subGraphName: options?.subGraphName,
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: publishFromSharedMemory(..., { subGraphName }) now routes data through sub-graph SWM, but the V10 StorageACK protocol still only carries contextGraphId. In the fromSharedMemory path there are no inline stagingQuads, so core nodes will look in the root SWM graph and reject valid sub-graph publishes. Extend PublishIntent/StorageACKHandler with subGraphName, or disable the SWM-ACK path for sub-graph publishes until that protocol change is in place.

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.

Acknowledged — deferring to a follow-up PR. Extending the StorageACK protocol (PublishIntent/StorageACKHandler) with subGraphName is a protocol-level change with significant scope. For now, the self-signed ACK fallback handles sub-graph publishes. Will address in a dedicated protocol update PR.

* `did:dkg:context-graph:{id}/{subGraphName}/_meta`. Sub-graphs are
* convention-based partitions — no on-chain enforcement in V10.0.
*/
subGraphName?: string;
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: exposing subGraphName on PublishOptions makes sub-graph KCs a supported public shape, but the update path still hard-codes the root data/private graphs and has no way to recover the KC's sub-graph. Updating one of these KCs will write replacement triples into the root graph and leave the original sub-graph copy behind. Either thread sub-graph scope through updates or explicitly reject updates for sub-graph KCs for now.

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 dfff789. update() now throws a clear error when options.subGraphName is set, preventing silent writes into the wrong graph. Full sub-graph-aware update support will be added in a follow-up PR.

…ates

1. removeSubGraph now includes subGraphPrivateUri in the graph cleanup
   list, preventing private triples from surviving a sub-graph removal.

2. update() explicitly rejects sub-graph KCs with a clear error message.
   The update path hard-codes root data/private graphs and cannot yet
   resolve sub-graph-scoped graphs — accepting the call would silently
   write into the wrong graph.

Made-with: Cursor

const swmGraph = this.graphManager.sharedMemoryUri(contextGraphId);
const swmMetaGraph = this.graphManager.sharedMemoryMetaUri(contextGraphId);
const swmGraph = this.graphManager.sharedMemoryUri(contextGraphId, subGraphName);
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: this path routes incoming SWM writes into a sub-graph, but it never registers that sub-graph in the root _meta graph. A peer that only learns about a sub-graph through share() gossip will not show it in listSubGraphs(), and publishFromSharedMemory(..., { subGraphName }) will later fail the registration check in publish(). Mirror the auto-registration logic from GossipPublishHandler here.

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 ec73758. SharedMemoryHandler now mirrors GossipPublishHandler: calls ensureSubGraph, then idempotently inserts dkg:SubGraph registration triples into _meta when receiving SWM writes for a sub-graph. listSubGraphs() will now work on peers that discover sub-graphs through share() gossip.

contextGraphId,
subGraphName,
createdBy: this.peerId,
authorizedWriters: opts?.authorizedWriters,
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: authorizedWriters is exposed as part of the public API here, but the new sub-graph write paths never consult it. publish, share, incoming gossip publishes, and SWM handlers all accept writes regardless of this list, so callers can think a sub-graph is write-restricted when it is not. Either enforce it at those ingress points or remove/rename the option until it is actually supported.

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.

Acknowledged — already deferred. authorizedWriters is intentionally advisory-only for V10.0 (convention-based sub-graphs). Will either enforce at write ingress points or remove the option in the sub-graph hardening PR.

Comment thread packages/publisher/src/metadata.ts Outdated

if (reg.authorizedWriters && reg.authorizedWriters.length > 0) {
for (const writer of reg.authorizedWriters) {
quads.push(mq(subGraphUri, `${DKG}authorizedWriter`, `did:dkg:agent:${writer}`, metaGraph));
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: authorizedWriters values are interpolated directly into IRIs without validation or escaping. A malformed value containing whitespace or > will produce invalid RDF/N-Quads here and can break createSubGraph() for user-provided input. Validate each writer id with a safe-IRI helper before emitting quads, or store it as a literal instead of an IRI.

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 ec73758. generateSubGraphRegistration now validates each authorizedWriters value with isSafeIri before emitting the quad. Malformed values are silently skipped, preventing broken N-Quads.


// Clear SWM ownership cache for this sub-graph
const ownershipKey = `${contextGraphId}\0${subGraphName}`;
this.publisher.clearSubGraphOwnership(ownershipKey);
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: removing a sub-graph only clears the SWM ownership cache. The confirmed-entity cache and private-triple cache remain keyed by contextGraphId\0subGraphName, so recreating the same sub-graph can still hit stale Rule 4 conflicts, and access checks can still believe deleted private triples exist. Dropping the graphs needs to clear all sub-graph-scoped in-memory caches, not just sharedMemoryOwnedEntities.

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.

Acknowledged — already deferred (third mention). Full sub-graph cache cleanup in removeSubGraph (confirmed ownership + private store) will be addressed in the sub-graph hardening PR.

{ contextGraphId: 'sg-replica' },
);
if (rootCheck.bindings.length > 0) {
// Data replicated but to root graph — still valid for replication test
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: this test explicitly treats replication into the root graph as success, which means it will not catch the core regression this PR is trying to prevent: losing sub-graph isolation on replicas. The assertion should require the data to appear under subGraphName: 'data' and fail if it lands in the root graph instead.

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.

Acknowledged — deferring to a follow-up PR. The current test verifies data replication works end-to-end; tightening the assertion to require sub-graph isolation (data in subGraphName: 'data' not root) is a valid improvement that will be added alongside the StorageACK protocol changes.

…dWriters IRIs

1. SharedMemoryHandler now mirrors GossipPublishHandler: when receiving
   SWM writes for a sub-graph, it calls ensureSubGraph and idempotently
   inserts dkg:SubGraph registration triples into _meta. This ensures
   listSubGraphs() works on peers that learn about sub-graphs through
   share() gossip, and publishFromSharedMemory won't fail the
   registration check.

2. generateSubGraphRegistration skips authorizedWriters values that
   fail isSafeIri, preventing malformed writer IDs from producing
   broken N-Quads.

Made-with: Cursor
*/
async createSubGraph(contextGraphId: string, subGraphName: string, opts?: {
description?: string;
authorizedWriters?: string[];
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: authorizedWriters is only persisted as registration metadata here; none of the local or replicated write paths (publish, share, workspace gossip, publish gossip) consult it before accepting a sub-graph write. That means a caller can create a supposedly restricted sub-graph and any peer can still write into it by setting subGraphName. Either enforce this on every ingress path or avoid exposing the option until it is real.


// Clear SWM ownership cache for this sub-graph
const ownershipKey = `${contextGraphId}\0${subGraphName}`;
this.publisher.clearSubGraphOwnership(ownershipKey);
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: this only clears the SWM ownership cache. The publisher's confirmed-entity cache and the private-store cache for the same {contextGraphId}\0{subGraphName} scope are left intact, so after removeSubGraph() + recreate, re-publishing the same root entity can still fail Rule 4 or surface stale private-data lookups until restart. Clear all sub-graph-scoped caches when dropping the sub-graph.

export interface ShareOptions {
publisherPeerId: string;
operationCtx?: OperationContext;
subGraphName?: string;
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: adding subGraphName to ShareOptions also advertises sub-graph support on conditionalShare(), but that path still locks on ${contextGraphId}\0${subject} and checks the root _shared_memory graph only. A CAS write targeting a sub-graph will validate against the wrong graph and can reject or apply based on unrelated root SWM state. Thread subGraphName through conditionalShare/_executeConditionalWrite (including the lock key) or keep it out of the shared options type until supported.

@branarakic branarakic merged commit 16d9eb0 into v10-rc Apr 9, 2026
3 checks passed
branarakic pushed a commit that referenced this pull request Apr 9, 2026
PR #104 review fixes:
- Thread assertionName through QueryOptions and queryWithView
- Wire view/agentAddress/assertionName/subGraphName through REST query API
- Drop private quads from JSON-LD envelope in assertion.write() with warning
- Fix SKILL.md: paranetId→contextGraphId, query→sparql, draft→assertion
- Restore contextGraphDraftUri deprecated alias lost during rebase

Sub-graph hardening (deferred from PR #101):
- Move gossip handler auto-registration after validation
- Clear ownedEntities + privateStore caches in removeSubGraph
- Fix conditionalShare to use sub-graph-scoped SWM graph
- Fix assertion graph prefix in removeSubGraph (/draft/ → /assertion/)
- Tighten E2E test: assert sub-graph isolation on replicas
- Update test references from agent.draft to agent.assertion

Made-with: Cursor
Jurij89 pushed a commit that referenced this pull request Apr 10, 2026
Five targeted fixes for sub-graph implementation follow-ups identified in
reviewing PR #101 against 03_PROTOCOL_CORE.md §16.2. No P0 changes — all
P1/P2 polish on existing working code.

1. createSubGraph JSDoc now reflects V10.0 gossip auto-registration.
   The previous comment claimed "receivers store replicated data in the
   root data graph" which has been false since gossip auto-registration
   landed in gossip-publish-handler / workspace-handler / finalization-
   handler.

2. Deleted unused ContextGraphManager.listSubGraphs. It was a
   heuristic graph-URI walk with divergent semantics from
   DKGAgent.listSubGraphs (which is the spec-compliant _meta ASK path)
   and had zero callers in the codebase. Removes the divergence risk
   and the stale "draft/" reserved prefix.

3. assertion{Create,Write,Query,Promote,Discard} now validate that the
   sub-graph is registered in _meta before operating. New private
   helper ensureSubGraphRegistered mirrors the ASK pattern already used
   by publish(). Previously these ops silently orphaned triples under
   unregistered sub-graph URIs. Guard is opt-in: assertion ops without
   a subGraphName are unchanged.

4. DKGAgent.listSubGraphs literal stripping now uses the shared
   stripLiteral() helper instead of three per-field regex strips. The
   old /^"|".*$/g pattern on createdAt was greedy and ate any inner
   quote; the new code correctly handles datatype- and language-tagged
   literals.

5. publishFromSharedMemory @throws JSDoc added to document the
   existing constraint: subGraphName and publishContextGraphId cannot
   be combined (the remap flow targets /context/{id} which is
   incompatible with sub-graph URIs).

Tests: 8 new unit tests in draft-lifecycle covering the registration
check — 616/616 publisher pass, 70/70 storage pass, 15/15 agent
sub-graph e2e pass.

Refs: OriginTrail/dkgv10-spec#81
branarakic pushed a commit that referenced this pull request Apr 10, 2026
Five targeted fixes for sub-graph implementation follow-ups identified in
reviewing PR #101 against 03_PROTOCOL_CORE.md §16.2. No P0 changes — all
P1/P2 polish on existing working code.

1. createSubGraph JSDoc now reflects V10.0 gossip auto-registration.
   The previous comment claimed "receivers store replicated data in the
   root data graph" which has been false since gossip auto-registration
   landed in gossip-publish-handler / workspace-handler / finalization-
   handler.

2. Deleted unused ContextGraphManager.listSubGraphs. It was a
   heuristic graph-URI walk with divergent semantics from
   DKGAgent.listSubGraphs (which is the spec-compliant _meta ASK path)
   and had zero callers in the codebase. Removes the divergence risk
   and the stale "draft/" reserved prefix.

3. assertion{Create,Write,Query,Promote,Discard} now validate that the
   sub-graph is registered in _meta before operating. New private
   helper ensureSubGraphRegistered mirrors the ASK pattern already used
   by publish(). Previously these ops silently orphaned triples under
   unregistered sub-graph URIs. Guard is opt-in: assertion ops without
   a subGraphName are unchanged.

4. DKGAgent.listSubGraphs literal stripping now uses the shared
   stripLiteral() helper instead of three per-field regex strips. The
   old /^"|".*$/g pattern on createdAt was greedy and ate any inner
   quote; the new code correctly handles datatype- and language-tagged
   literals.

5. publishFromSharedMemory @throws JSDoc added to document the
   existing constraint: subGraphName and publishContextGraphId cannot
   be combined (the remap flow targets /context/{id} which is
   incompatible with sub-graph URIs).

Tests: 8 new unit tests in draft-lifecycle covering the registration
check — 616/616 publisher pass, 70/70 storage pass, 15/15 agent
sub-graph e2e pass.

Refs: OriginTrail/dkgv10-spec#81
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.

1 participant