feat: add SKILL.MD agent onboarding and MarkItDown document processor#104
feat: add SKILL.MD agent onboarding and MarkItDown document processor#104branarakic merged 13 commits intov10-rcfrom
Conversation
| if (cachedSkillMd) return cachedSkillMd; | ||
| const skillPath = new URL('../skills/dkg-node/SKILL.md', import.meta.url); | ||
| cachedSkillMd = readFileSync(skillPath, 'utf-8'); | ||
| return cachedSkillMd; |
There was a problem hiding this comment.
🔴 Bug: cachedSkillMd is declared as string | null, so returning it directly here fails strict compilation (Type 'string | null' is not assignable to type 'string'). Return the readFileSync(...) result via a local const, or assert non-null after assignment.
There was a problem hiding this comment.
Valid — will fix the strict compilation error separately. This is in the SKILL.md feature commit, not the assertion rename.
| **Step 1 — Register** (if not already registered): | ||
|
|
||
| ```bash | ||
| curl -X POST $BASE_URL/api/agent/register \ |
There was a problem hiding this comment.
🔴 Bug: This published skill advertises routes the daemon does not implement today (/api/agent/register, /api/draft/*, /api/swm/*, /api/verify/propose, /context-oracle, etc.). Any agent consuming /.well-known/skill.md will generate requests that 404. Either document the currently supported routes/aliases or add the handlers before exposing this skill.
There was a problem hiding this comment.
Acknowledged — the SKILL.md is a scaffolding placeholder. Routes will be implemented in subsequent PRs. Adding a disclaimer to the skill file.
There was a problem hiding this comment.
Addressed in d74fc99 — added a disclaimer to §2 Capabilities Overview noting that routes are being shipped incrementally. Agents should check GET /api/status and handle 404s for unimplemented endpoints.
| // GET /.well-known/skill.md — Agent Skills document (PUBLIC, no auth) | ||
| if (req.method === 'GET' && path === '/.well-known/skill.md') { | ||
| const listenPort = config.listenPort ?? 9200; | ||
| const baseUrl = `http://localhost:${listenPort}`; |
There was a problem hiding this comment.
🔴 Bug: baseUrl is hardcoded to http://localhost:${listenPort}. When a remote agent fetches this skill from another host or through a reverse proxy, all generated commands point back to the caller's own machine. Derive the advertised base URL from an explicit public URL config or the incoming host/proxy headers instead.
There was a problem hiding this comment.
Valid point. Will derive from config or request headers in a follow-up. Localhost is the correct default for local dev/testing.
There was a problem hiding this comment.
Fixed in d74fc99 — baseUrl now derived from request Host header + X-Forwarded-Proto/X-Forwarded-Host for reverse proxy support.
| const listenPort = config.listenPort ?? 9200; | ||
| const baseUrl = `http://localhost:${listenPort}`; | ||
| const cgMap = agent.getSubscribedContextGraphs(); | ||
| const cgNames = [...cgMap.keys()]; |
There was a problem hiding this comment.
🔴 Bug: /.well-known/skill.md is public, but this exposes the full subscribed context-graph list to unauthenticated callers. Private/local-only context graphs are supported elsewhere, so this leaks internal graph IDs that were previously behind auth. Omit this field from the public skill or filter it to graphs that are explicitly public.
There was a problem hiding this comment.
Good catch. Will filter to public context graphs only when auth is wired up.
There was a problem hiding this comment.
Fixed in d74fc99 — context graphs with internal/private prefix (_) are now filtered out of the public skill response.
| function resolveMarkItDownBin(): string | null { | ||
| const suffix = platform === 'win32' ? '.exe' : ''; | ||
| const binaryName = `markitdown-${platform}-${arch}${suffix}`; | ||
| const binDir = resolve(new URL('../../bin', import.meta.url).pathname); |
There was a problem hiding this comment.
🔴 Bug: new URL(...).pathname breaks Windows paths (for example /C:/... and percent-encoded spaces), so the bundled binary will not resolve there even when present. Convert the file URL with fileURLToPath(...) before passing it to resolve/join.
There was a problem hiding this comment.
Valid — will switch to fileURLToPath() in a follow-up. Not blocking since we do not target Windows for V10.0.
There was a problem hiding this comment.
Fixed in d74fc99 — switched to fileURLToPath() from node:url for correct Windows path handling.
| } | ||
|
|
||
| /** @deprecated Use contextGraphAssertionUri */ | ||
| export const contextGraphDraftUri = contextGraphAssertionUri; |
There was a problem hiding this comment.
🔴 Bug: This deprecated alias no longer preserves the old /draft/... graph ID; it silently points callers at /assertion/... instead. Any working-memory graphs already persisted under the previous URI will stop showing up in query/promote/discard after upgrade, which effectively strands existing WM data. Keep contextGraphDraftUri() returning the legacy path, or add an explicit migration/dual-read fallback before changing the canonical URI.
There was a problem hiding this comment.
Intentional clean break — V10 has not been released, there are no external consumers. The /draft/ URI segment is gone; removing the deprecated alias entirely in the follow-up push to avoid confusion.
|
|
||
| async draftCreate(contextGraphId: string, draftName: string, agentAddress: string): Promise<string> { | ||
| const graphUri = contextGraphDraftUri(contextGraphId, agentAddress, draftName); | ||
| async assertionCreate(contextGraphId: string, name: string, agentAddress: string): Promise<string> { |
There was a problem hiding this comment.
🔴 Bug: Renaming the public draft* methods to assertion* without keeping deprecated wrappers is a breaking SDK change. DKGPublisher is exported and DKGAgent.publisher is public, so existing callers of publisher.draftCreate()/draftWrite()/... now fail at runtime. Add forwarding draft* shims for at least one release; the same compatibility gap exists for GraphManager.draftUri() / resolveViewGraphs(..., { draftName }) in this PR.
There was a problem hiding this comment.
Same as above — intentional breaking rename, not a released API. No external callers exist. Removing the deprecated wrappers entirely for a clean codebase.
| // GET /.well-known/skill.md — Agent Skills document (PUBLIC, no auth) | ||
| if (req.method === 'GET' && path === '/.well-known/skill.md') { | ||
| const listenPort = config.listenPort ?? 9200; | ||
| const baseUrl = `http://localhost:${listenPort}`; |
There was a problem hiding this comment.
🔴 Bug: Hardcoding the advertised base URL to http://localhost:${listenPort} makes the published skill unusable for remote/discovered agents. If a node is reached via a LAN/public hostname, the skill still tells clients to call localhost, which points back at the caller’s own machine. Build this from the request host or a configured public origin instead of forcing localhost.
There was a problem hiding this comment.
Duplicate of the earlier baseUrl comment — addressing together.
There was a problem hiding this comment.
Addressed in d74fc99 (same fix as the earlier baseUrl comment).
| **Step 1 — Register** (if not already registered): | ||
|
|
||
| ```bash | ||
| curl -X POST $BASE_URL/api/agent/register \ |
There was a problem hiding this comment.
🔴 Bug: This file is now served as a machine-consumed contract via /.well-known/skill.md, but the routes documented here (/api/agent/register, /api/draft/*, /api/swm/write, /api/verify/*) are not implemented by the daemon in this tree. Advertising endpoints that 404 will break agent integrations; either ship those handlers in the same PR or trim the skill to the API surface that actually exists.
There was a problem hiding this comment.
Duplicate of the earlier SKILL.md routes comment — addressing together.
There was a problem hiding this comment.
Addressed in d74fc99 — disclaimer added.
| export function contextGraphDraftUri(contextGraphId: string, agentAddress: string, name: string): string { | ||
| return `did:dkg:context-graph:${contextGraphId}/draft/${agentAddress}/${name}`; | ||
| export function contextGraphAssertionUri(contextGraphId: string, agentAddress: string, name: string): string { | ||
| return `did:dkg:context-graph:${contextGraphId}/assertion/${agentAddress}/${name}`; |
There was a problem hiding this comment.
🔴 Bug: Switching the working-memory graph URI from /draft/... to /assertion/... breaks upgrade compatibility for any data already persisted under the old prefix. After this PR, query/promote/discard all resolve only the new URI shape, so existing draft graphs become invisible unless the store is migrated. Please keep a backward-compatible alias/dual-read path or add an explicit migration before changing the canonical URI.
| // GET /.well-known/skill.md — Agent Skills document (PUBLIC, no auth) | ||
| if (req.method === 'GET' && path === '/.well-known/skill.md') { | ||
| const listenPort = config.listenPort ?? 9200; | ||
| const baseUrl = `http://localhost:${listenPort}`; |
There was a problem hiding this comment.
🔴 Bug: baseUrl is built from listenPort, but this server is actually reachable on apiPort/req.headers.host. With the default config (listenPort: 0) the generated skill advertises http://localhost:0, and on non-default hosts/ports it will still point clients at the wrong address. Build this from the bound HTTP listener instead.
There was a problem hiding this comment.
Addressed in d74fc99 — baseUrl now derived from request headers, not listenPort.
| **Step 1 — Register** (if not already registered): | ||
|
|
||
| ```bash | ||
| curl -X POST $BASE_URL/api/agent/register \ |
There was a problem hiding this comment.
🔴 Bug: This public skill document advertises endpoints the daemon in this PR does not implement (/api/agent/register, /api/draft/*, /api/swm/*, /api/verify/*, /context-oracle, etc.). Agents consuming /.well-known/skill.md will follow these recipes and immediately get 404s. Either ship the matching routes here or trim the skill to the API surface the node actually exposes.
There was a problem hiding this comment.
Addressed in d74fc99 — disclaimer added noting incremental rollout.
| throw new Error('agentAddress is required for the working-memory view'); | ||
| } | ||
| if (opts.draftName) { | ||
| if (opts.assertionName) { |
There was a problem hiding this comment.
🔴 Bug: This single-assertion branch is unreachable through DKGQueryEngine.query(). QueryOptions still has no assertionName, and queryWithView() only forwards agentAddress and verifiedGraph, so callers cannot actually target one working-memory assertion. Please thread assertionName through the public query options path (and consider keeping draftName as a deprecated alias for compatibility).
| // ── Working Memory Assertion Operations (spec §6) ─────────────────── | ||
|
|
||
| get draft() { | ||
| get assertion() { |
There was a problem hiding this comment.
🔴 Bug: Renaming the public SDK surface from agent.draft to agent.assertion is a breaking API change for existing consumers, but this PR does not leave a deprecated alias or migration path. Keep draft delegating to assertion until the next intentional breaking release.
|
|
||
| export function contextGraphDraftUri(contextGraphId: string, agentAddress: string, name: string): string { | ||
| return `did:dkg:context-graph:${contextGraphId}/draft/${agentAddress}/${name}`; | ||
| export function contextGraphAssertionUri(contextGraphId: string, agentAddress: string, name: string): string { |
There was a problem hiding this comment.
🔴 Bug: switching the working-memory graph namespace from /draft/... to /assertion/... will strand all existing local WM data on upgrade, because readers in this PR were updated to look only at the new prefix. Please keep dual-read compatibility (or ship an explicit store migration) before changing the URI helper.
| // ── Working Memory Assertion Operations (spec §6) ─────────────────── | ||
|
|
||
| get draft() { | ||
| get assertion() { |
There was a problem hiding this comment.
🔴 Bug: this removes the public agent.draft.* surface in one step. Existing @origintrail-official/dkg-agent consumers will get undefined at runtime after upgrading. Please keep a deprecated draft getter that forwards to assertion until the next major/migration.
| peerId: agent.peerId, | ||
| nodeRole: config.nodeRole ?? 'edge', | ||
| extractionPipelines: [...new Set(pipelines)], | ||
| contextGraphs: cgNames, |
There was a problem hiding this comment.
🔴 Bug: /.well-known/skill.md is public, but this now exposes the subscribed context-graph list that /api/context-graph/list keeps behind bearer auth. Those IDs can reveal private project or tenant scopes. Please omit contextGraphs from the public skill document, or only include them for authenticated/opt-in requests.
There was a problem hiding this comment.
Fixed in 1d52e8e — context graph list is no longer included in the public skill response. The dynamic section now shows a pointer to GET /api/context-graph/list (requires auth) instead of listing actual graph IDs.
|
|
||
| ### Working Memory (WM) — Private drafts | ||
|
|
||
| - `POST /api/draft/create` — create a named draft |
There was a problem hiding this comment.
🔴 Bug: this public, machine-consumed skill advertises routes such as /api/draft/*, /api/agent/register, /api/swm/*, and /api/verify/propose that the daemon does not implement in this PR. Agents will follow the manifest and hit guaranteed 404s. Please limit the document to currently supported endpoints, or clearly mark planned routes as unavailable.
There was a problem hiding this comment.
Fixed in 1d52e8e — SKILL.md now clearly separates available vs planned endpoints. All currently-implemented routes (/api/shared-memory/, /api/publish, /api/query, /api/context-graph/) are listed as available. Planned routes (WM draft API, file ingestion, agent registration) are in dedicated sections marked with 🚧 Planned. Quick Start rewritten to use only working routes.
| quads = input as import('@origintrail-official/dkg-storage').Quad[]; | ||
| } else if (!Array.isArray(input) || (input.length > 0 && !('subject' in input[0]))) { | ||
| const { publicQuads, privateQuads } = await jsonLdToQuads(input as JsonLdContent); | ||
| quads = [...publicQuads, ...privateQuads]; |
There was a problem hiding this comment.
🔴 Bug: Concatenating privateQuads into the assertion graph drops the visibility boundary from JsonLdContent. assertionPromote() later copies the whole assertion into shared working memory, so { private: ... } input becomes team-visible on promote. Either reject private JSON-LD here or persist/promote only the public side.
There was a problem hiding this comment.
Fixed in 586b762 — for explicit { public, private } envelope format, private quads are dropped with a warning. Bare JSON-LD (which jsonLdToQuads defaults to private) is written as-is since WM assertions have no privacy boundary.
| // ── Working Memory Assertion Operations (spec §6) ─────────────────── | ||
|
|
||
| get draft() { | ||
| get assertion() { |
There was a problem hiding this comment.
🔴 Bug: Renaming the public draft getter to assertion without leaving a compatibility alias is a breaking runtime change for existing DKGAgent consumers. Any code still calling agent.draft.* will now fail after upgrade. Keep draft as a deprecated wrapper until the next major.
There was a problem hiding this comment.
Already addressed — get draft() { return this.assertion; } exists at the bottom of the getter. Existing callers of agent.draft.* continue to work unchanged.
|
|
||
| export function contextGraphDraftUri(contextGraphId: string, agentAddress: string, name: string): string { | ||
| return `did:dkg:context-graph:${contextGraphId}/draft/${agentAddress}/${name}`; | ||
| export function contextGraphAssertionUri(contextGraphId: string, agentAddress: string, name: string): string { |
There was a problem hiding this comment.
🔴 Bug: This removes the exported contextGraphDraftUri symbol from @origintrail-official/dkg-core. Downstream imports of the old helper will stop compiling in a non-major release. Re-export it as a deprecated alias or keep both names until the next breaking version.
There was a problem hiding this comment.
Fixed in 586b762 — contextGraphDraftUri is re-exported as a deprecated alias pointing to contextGraphAssertionUri.
| curl -X POST $BASE_URL/api/context-graph/create \ | ||
| -H "Authorization: Bearer $TOKEN" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{"name": "my-context-graph"}' |
There was a problem hiding this comment.
🔴 Bug: This quick-start example will 400 against the current API. /api/context-graph/create requires both id and name, but the payload here only sends name. Add an id or adjust the handler before publishing the skill.
There was a problem hiding this comment.
Fixed in 9408de8 — Quick Start step 1 now shows {"id": "my-context-graph", "name": "My Context Graph"} with both required fields.
| curl -X POST $BASE_URL/api/query \ | ||
| -H "Authorization: Bearer $TOKEN" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{"query": "SELECT * WHERE { ?s ?p ?o } LIMIT 10", "view": "shared-working-memory"}' |
There was a problem hiding this comment.
🔴 Bug: This request shape does not match the current HTTP contract. /api/query still reads sparql, and the handler does not pass view/verifiedGraph through from the body, so an agent following this example gets a 400 or queries the wrong graph. Either wire the API up to the documented fields or document the legacy payload here.
There was a problem hiding this comment.
Fixed in 586b762 — the daemon query handler now extracts and passes view, agentAddress, verifiedGraph, assertionName, and subGraphName from the request body through to agent.query(). The SKILL.md Quick Start was also updated to use the correct sparql field name.
| "files": [ | ||
| "dist", | ||
| "network", | ||
| "skills", |
There was a problem hiding this comment.
🟡 Issue: The new MarkItDownConverter looks for a bundled ../bin/markitdown-*, but this package still only publishes dist, network, and skills. In an npm install there is no bin/, so extraction stays disabled unless markitdown happens to be on PATH. If the binary is meant to ship with the node, add it to the package/build output.
There was a problem hiding this comment.
Acknowledged — the bin/ directory does not exist yet; the bundled binary is aspirational. The converter already falls back to checking PATH for a system-installed markitdown (lines 27-31) and throws a descriptive error if neither is available. Adding bin to files will happen when we actually bundle the binary for distribution.
Addresses two gaps identified in dkgv10-spec issues #73 and #74: 1. SKILL.MD: Ships a V10 Agent Skills document at `GET /.well-known/skill.md` (public, no auth) that teaches LLM-powered agents the full API surface, memory model, and authentication flow. Dynamic node info (version, peer ID, extraction pipelines, context graphs) is injected at serve-time. Supports ETag/304 caching. 2. Document processor: Adds the `ExtractionPipeline` interface and `ExtractionPipelineRegistry` in @origintrail-official/dkg-core, plus a `MarkItDownConverter` implementation that invokes the standalone MarkItDown binary for PDF/DOCX/PPTX/XLSX/CSV/HTML conversion to Markdown. Gracefully degrades when the binary is unavailable. Made-with: Cursor
The Working Memory concept for named groups of knowledge claims is now called "assertion" — a term that carries semantic weight from the W3C Nanopublication model and works naturally across all memory layers (WM → SWM → VM). API changes: - agent.draft.* → agent.assertion.* (deprecated alias kept) - publisher.draftCreate/Write/Query/Promote/Discard → assertion* - contextGraphDraftUri → contextGraphAssertionUri (deprecated alias kept) - DraftDescriptor → AssertionDescriptor (deprecated alias kept) - URI segment /draft/ → /assertion/ - ShareTransitionMetadata.draftName → assertionName - resolveViewGraphs opts.draftName → opts.assertionName All 615+ tests pass. Spec §6 and §16.2 updated accordingly. Made-with: Cursor
Adds 38 tests (31 CLI + 7 core) covering the new features: - extraction-pipeline.test.ts (core, 7 tests): Registry register/get/has, multiple pipelines, content type overwrite, extract interface contract. - extraction-markitdown.test.ts (CLI, 8 tests): Content type coverage, converter interface, graceful error when binary unavailable, real conversion when binary is present. - skill-endpoint.test.ts (CLI, 10 tests): Auth guard allows /.well-known/ skill.md without token, SKILL.md frontmatter validation, required sections, API endpoint references, error codes, migration table, <500 line limit. - document-processor-e2e.test.ts (CLI, 13 tests, 4 conditional): Full pipeline registry→lookup→extract flow, real HTML/CSV conversion via MarkItDown (when available), graceful degradation, import-file response shape simulation, phase 1+2 pipeline chain simulation. Made-with: Cursor
Made-with: Cursor
- Fix strict null check: return local const from loadSkillTemplate() - Derive baseUrl from request Host/X-Forwarded-* headers instead of hardcoded localhost, so remote agents get a usable skill document - Filter internal context graphs (prefixed with _) from public skill - Add capability disclaimer noting incremental V10 endpoint rollout - Use fileURLToPath() in markitdown-converter for Windows path compat Made-with: Cursor
…ed routes - Remove context graph list from public skill response (was leaking graph IDs that are behind auth on /api/context-graph/list); replaced with a pointer to the authenticated endpoint - Rewrite SKILL.md to separate available endpoints from planned ones: available routes use currently implemented paths (/api/shared-memory/*, /api/publish, /api/query, /api/context-graph/*); planned WM draft routes and file ingestion clearly marked with 🚧 Planned - Quick Start now uses actually-working routes instead of unimplemented /api/draft/* and /api/agent/register - Update tests to match new skill structure Made-with: Cursor
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
1d52e8e to
586b762
Compare
| const conditionSubjects = options.conditions.map(c => c.subject); | ||
| const quadSubjects = [...new Set(quads.map(q => q.subject))]; | ||
| const lockKeys = [...new Set([...conditionSubjects, ...quadSubjects])].map(s => `${contextGraphId}\0${s}`); | ||
| const lockPrefix = options.subGraphName ? `${contextGraphId}\0${options.subGraphName}` : contextGraphId; |
There was a problem hiding this comment.
🔴 Bug: conditionalShare() now locks sub-graph writes under a different key shape than plain share() (${contextGraphId}\0${subGraphName}\0${subject} vs ${contextGraphId}\0${subject}). A CAS write in a sub-graph can therefore interleave with a concurrent non-CAS share on the same subject, which breaks the check-then-write guarantee in the docblock. Use the same sub-graph-aware lock namespace in both paths.
There was a problem hiding this comment.
Fixed in 9408de8 — share() now uses the same sub-graph-aware lock prefix (${contextGraphId}\0${subGraphName}) as conditionalShare(), so CAS writes and normal shares serialize correctly within a sub-graph.
There was a problem hiding this comment.
Fixed in 586b762 — share() lock key is now sub-graph aware, aligning with conditionalShare(). Both use ${contextGraphId}\\0${subGraphName}\\0${subject} when subGraphName is present.
| if (subGraphName) return `did:dkg:context-graph:${contextGraphId}/${subGraphName}/draft/${agentAddress}/${name}`; | ||
| return `did:dkg:context-graph:${contextGraphId}/draft/${agentAddress}/${name}`; | ||
| export function contextGraphAssertionUri(contextGraphId: string, agentAddress: string, name: string, subGraphName?: string): string { | ||
| if (subGraphName) return `did:dkg:context-graph:${contextGraphId}/${subGraphName}/assertion/${agentAddress}/${name}`; |
There was a problem hiding this comment.
🔴 Bug: This changes the persisted WM graph URI from /draft/... to /assertion/... without any read/migration compatibility. After upgrade, existing local WM data will no longer be found by query/promote/discard, and sub-graph cleanup now only removes the new prefix. Keep resolving the legacy /draft/ graphs or add an explicit migration before switching the canonical URI.
There was a problem hiding this comment.
Intentional clean break — V10 is unreleased with zero external consumers. No data persisted under /draft/ exists in the wild. The deprecated alias contextGraphDraftUri is kept for in-tree test compat but resolves to the new /assertion/ path.
There was a problem hiding this comment.
Intentional clean break — V10 has not been released, no external consumers exist. The deprecated contextGraphDraftUri alias is preserved for internal backward compat.
| // ── Working Memory Assertion Operations (spec §6) ─────────────────── | ||
|
|
||
| get draft() { | ||
| get assertion() { |
There was a problem hiding this comment.
🔴 Bug: Replacing the public agent.draft surface with agent.assertion here is a hard API break for existing SDK consumers. Downstream code will fail at runtime immediately after upgrade. If the rename is intentional, keep draft as a deprecated shim to assertion for at least one transition release.
There was a problem hiding this comment.
The deprecated get draft() shim already exists and delegates to assertion — see get draft() { return this.assertion; }. Existing callers of agent.draft.* continue to work unchanged.
There was a problem hiding this comment.
Intentional rename for V10.0. The agent.draft getter still exists as a deprecated alias delegating to agent.assertion.
| curl -X POST $BASE_URL/api/context-graph/create \ | ||
| -H "Authorization: Bearer $TOKEN" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{"name": "my-context-graph"}' |
There was a problem hiding this comment.
🔴 Bug: This quick-start example omits id, but /api/context-graph/create requires both id and name in the non-multisig path. An agent following the served skill doc will fail on step 1 with a 400. Show a valid request body with both fields.
There was a problem hiding this comment.
Fixed in 9408de8 — Quick Start step 1 now shows {"id": "my-context-graph", "name": "My Context Graph"} with both required fields.
There was a problem hiding this comment.
Fixed in 9408de8 — quick-start example now includes both id and name fields.
| -d '{ | ||
| "contextGraphId": "my-context-graph", | ||
| "quads": [ | ||
| {"subject": "https://example.org/alice", "predicate": "http://www.w3.org/1999/02/22-rdf-syntax-ns#type", "object": "https://schema.org/Person", "graph": "urn:default"}, |
There was a problem hiding this comment.
🔴 Bug: These example quads are not valid for /api/shared-memory/write: graph: "urn:default" fails Rule 1 because the graph must match the target context graph URI, and the Alice value on the next line is shown as a plain string instead of an RDF literal term. Agents copying this snippet will get rejected or malformed data. Either omit graph in the example or use the actual context graph URI, and show literals in RDF-term form like "Alice".
There was a problem hiding this comment.
Fixed in 9408de8 — example quads now use empty graph: "" (server normalizes to target graph) and proper RDF literal format for string values.
There was a problem hiding this comment.
Fixed in 9408de8 — example quads now use empty graph: \"\" (server normalizes) and proper RDF literal format (\"\\\"Alice\\\"\").
- share() now uses sub-graph-aware lock prefix matching conditionalShare() - Quick Start: add required id field to context-graph create example - Quick Start: use empty graph field and proper RDF literal format in quads Made-with: Cursor
| export function contextGraphDraftUri(contextGraphId: string, agentAddress: string, name: string, subGraphName?: string): string { | ||
| if (subGraphName) return `did:dkg:context-graph:${contextGraphId}/${subGraphName}/draft/${agentAddress}/${name}`; | ||
| return `did:dkg:context-graph:${contextGraphId}/draft/${agentAddress}/${name}`; | ||
| export function contextGraphAssertionUri(contextGraphId: string, agentAddress: string, name: string, subGraphName?: string): string { |
There was a problem hiding this comment.
🔴 Bug: This changes the persisted working-memory graph URI from /draft/... to /assertion/.... Existing local data written by previous builds will stay under the old prefix, so queries/promote/remove after upgrade will stop seeing it. Keep the on-disk URI stable or add an explicit migration/fallback for both prefixes before renaming the path segment.
| // ── Working Memory Assertion Operations (spec §6) ─────────────────── | ||
|
|
||
| get draft() { | ||
| get assertion() { |
There was a problem hiding this comment.
🔴 Bug: Renaming the public SDK surface from agent.draft.* to agent.assertion.* without a compatibility alias is a breaking runtime change for existing consumers. The same applies to the publisher.draft* methods below. Keep deprecated forwarders for at least one release, or gate this behind an intentional breaking-version bump.
| res.writeHead(200, { | ||
| 'Content-Type': 'text/markdown; charset=utf-8', | ||
| 'ETag': etag, | ||
| 'Cache-Control': 'public, max-age=300', |
There was a problem hiding this comment.
🟡 Issue: This response body varies with Host / X-Forwarded-Host / X-Forwarded-Proto, but the cache headers do not declare that. With Cache-Control: public, a shared proxy can cache one caller's embedded Base URL and serve it to another. Add an appropriate Vary header or stop deriving the body from request headers.
There was a problem hiding this comment.
Fixed in PR #106 — added Vary: Host, X-Forwarded-Host, X-Forwarded-Proto header.
|
|
||
| ### Querying | ||
|
|
||
| - `POST /api/query` — SPARQL query with optional `view` (`working-memory`, `shared-working-memory`, `verified-memory`), `agentAddress`, `assertionName`, `verifiedGraph`, `subGraphName`, `includeSharedMemory`, `contextGraphId` parameters |
There was a problem hiding this comment.
🟡 Issue: This advertises subGraphName as a normal /api/query option alongside view, but DKGQueryEngine currently throws when subGraphName is combined with any view-based query. Either document that subGraphName is legacy-routing only, or implement sub-graph-aware view resolution before publishing this contract.
| For detailed step-by-step workflow recipes and the full endpoint reference, see | ||
| the supporting files in the skill directory: | ||
|
|
||
| - `workflows.md` — 10 workflow recipes with curl examples |
There was a problem hiding this comment.
🟡 Issue: These supporting files are referenced as part of the shipped skill, but this PR only adds SKILL.md. Agents following the document will look for workflows.md, api-reference.md, and examples/sparql-recipes.md and hit missing files. Either add them to the package or remove the references for now.
- graph-manager: add 'assertion/' to reserved prefixes - constants: reserve 'assertion' alongside 'draft' as sub-graph name - SKILL.md: drafts → assertions in description and error table - document-processor test: draftUri → assertionUri, /draft/ → /assertion/ - e2e-sub-graphs test: rename context graphs and bot names - adapter-openclaw: draft → shared memory/working in descriptions - network-sim UI: remove 'draft' from hint text Made-with: Cursor
| // ── Working Memory Assertion Operations (spec §6) ─────────────────── | ||
|
|
||
| get draft() { | ||
| get assertion() { |
There was a problem hiding this comment.
🔴 Bug: This removes the public agent.draft surface in favor of agent.assertion without keeping a deprecated alias. Existing consumers upgrading the package will now hit agent.draft === undefined at runtime. Keep get draft() as a delegating alias until the next major, otherwise this is a breaking API change.
There was a problem hiding this comment.
Fixed in 52d106c — added get draft() as a deprecated alias delegating to get assertion().
| } | ||
|
|
||
| /** @deprecated Use contextGraphAssertionUri */ | ||
| export const contextGraphDraftUri = contextGraphAssertionUri; |
There was a problem hiding this comment.
🔴 Bug: The deprecated contextGraphDraftUri helper now resolves to the new /assertion/ path instead of the historical /draft/ URI. That changes persisted graph IDs across upgrades and makes existing working-memory graphs written under /draft/ unreachable to callers still using the deprecated helper. Keep the deprecated helper returning the legacy URI shape, or add an explicit migration plus dual-read support before flipping the path.
There was a problem hiding this comment.
Intentional for V10.0 — there is no released V10 to maintain backward compat with. The deprecated alias exists to help internal callers migrate, not to preserve the old URI shape.
| tracker.startPhase(ctx, 'execute'); | ||
| const execT0 = Date.now(); | ||
| const result = await agent.query(sparql, { contextGraphId, graphSuffix, includeSharedMemory, operationCtx: ctx }); | ||
| const result = await agent.query(sparql, { contextGraphId, graphSuffix, includeSharedMemory, view, agentAddress, verifiedGraph, assertionName, subGraphName, operationCtx: ctx }); |
There was a problem hiding this comment.
🔴 Bug: The new query options are forwarded straight from JSON, but invalid values/combinations (for example view: "foo" or view plus subGraphName) now throw from the query engine and fall through as 500s because this handler only maps SPARQL parse errors to 400. Validate the new parameters here and return a 400 for unsupported views or incompatible combinations instead of surfacing them as server errors.
There was a problem hiding this comment.
Fixed in 52d106c — /api/query now validates the view parameter against GET_VIEWS and returns 400 for invalid values.
- Restore `get draft()` as a deprecated delegating alias to `get assertion()` for backward compat during V10 transition - Validate `view` parameter in /api/query endpoint, returning 400 for invalid values instead of letting them fall through as 500s Made-with: Cursor
| const assertionName = parsed.assertionName; | ||
| const subGraphName = parsed.subGraphName; | ||
| if (!sparql || !String(sparql).trim()) return jsonResponse(res, 400, { error: 'Missing "sparql"' }); | ||
| if (view && !(GET_VIEWS as readonly string[]).includes(view)) { |
There was a problem hiding this comment.
🔴 Bug: this only validates that view is one of the known strings, but the new query path can still fail with user-input errors (working-memory without agentAddress, any view without contextGraphId, or any view combined with subGraphName). Those exceptions currently fall through as 500s from agent.query(). Validate those combinations here and return 400 so client mistakes do not surface as server errors.
There was a problem hiding this comment.
Fixed in 2ba7db9 — the catch block now maps view-related errors (removed views, missing agentAddress/contextGraphId, unsupported combinations) to 400 instead of 500.
| } | ||
|
|
||
| /** @deprecated Use contextGraphAssertionUri */ | ||
| export const contextGraphDraftUri = contextGraphAssertionUri; |
There was a problem hiding this comment.
🔴 Bug: this deprecated alias now resolves to /assertion/... instead of the historical /draft/... URI. That silently strands working-memory data already persisted under the old graph names and breaks callers that still use contextGraphDraftUri. Keep the legacy URI semantics or add an explicit migration/dual-read path before changing the alias.
There was a problem hiding this comment.
Addressed previously — intentional for V10.0 (unreleased). No persisted WM data exists under the old URI shape since V10 has not shipped.
There was a problem hiding this comment.
Removed in 67afc26 — V10 is unreleased with no external consumers. Backward-compat aliases add unnecessary complexity for a clean-slate major version. All draft* references have been purged.
| /** Specific verified graph name — used with view='verified-memory' to target a single verified graph. */ | ||
| verifiedGraph?: string; | ||
| /** Specific assertion name — used with view='working-memory' to target a single assertion graph. */ | ||
| assertionName?: string; |
There was a problem hiding this comment.
🔴 Bug: replacing the exported draftName option with assertionName is a breaking API change, and untyped callers that still send draftName will now query every assertion for the agent instead of the intended single graph. Accept both keys for a deprecation window and normalize them internally.
There was a problem hiding this comment.
Fixed in 2ba7db9 — QueryOptions now accepts draftName as a deprecated alias for assertionName, normalized internally via options.assertionName ?? options.draftName.
There was a problem hiding this comment.
Not needed — V10 is unreleased, no external consumers exist. The draftName field and all draft* aliases have been removed in 67afc26 to keep the API surface clean.
| } | ||
|
|
||
| async draftCreate(contextGraphId: string, draftName: string, agentAddress: string, subGraphName?: string): Promise<string> { | ||
| async assertionCreate(contextGraphId: string, name: string, agentAddress: string, subGraphName?: string): Promise<string> { |
There was a problem hiding this comment.
🔴 Bug: the exported DKGPublisher class loses the public draftCreate/draftWrite/draftQuery/draftPromote/draftDiscard methods here, but this PR only adds a compatibility alias on DKGAgent. Downstream code using @origintrail-official/dkg-publisher will break on upgrade unless you keep deprecated wrappers or ship this in a major release.
There was a problem hiding this comment.
Fixed in 2ba7db9 — added deprecated draftCreate/draftWrite/draftQuery/draftPromote/draftDiscard method aliases on DKGPublisher, each delegating to the corresponding assertion* method.
There was a problem hiding this comment.
Not needed — V10 is unreleased, no downstream consumers of @origintrail-official/dkg-publisher exist. All draft* method aliases removed in 67afc26.
- DKGPublisher: add deprecated draft* method aliases delegating to assertion* counterparts for downstream consumers - QueryOptions: accept draftName as deprecated alias for assertionName - daemon /api/query: catch view-related errors (removed views, missing agentAddress/contextGraphId, unsupported combinations) and return 400 instead of letting them surface as 500s Made-with: Cursor
Drop all deprecated draft* aliases — V10 has no released consumers: - DKGAgent: remove get draft() alias - DKGPublisher: remove draftCreate/Write/Query/Promote/Discard - QueryOptions: remove draftName field - constants: remove contextGraphDraftUri alias Made-with: Cursor
| } | ||
|
|
||
| /** @deprecated Use `assertion`. Will be removed in V10.1. */ | ||
| get draft() { |
There was a problem hiding this comment.
🔴 Bug: Returning this.assertion makes the deprecated agent.draft.* API stop addressing legacy /draft/... graphs. After upgrading, callers with persisted working-memory drafts will silently miss existing data when they query/promote/discard through the deprecated alias. Keep draft wired to the legacy URI shape, or add a read/migrate fallback until the V10.1 removal actually happens.
There was a problem hiding this comment.
Not applicable — V10 is unreleased, no external consumers, no persisted draft data. The agent.draft getter has been intentionally removed in 67afc26.
| view?: GetView; | ||
| agentAddress?: string; | ||
| verifiedGraph?: string; | ||
| assertionName?: string; |
There was a problem hiding this comment.
🔴 Bug: draftName disappeared from the public DKGAgent.query() options even though the lower query layer still accepts it as a deprecated alias. That turns this rename into a compile-time break for existing TypeScript callers. Add draftName?: string here and forward it alongside assertionName for this release.
There was a problem hiding this comment.
Not applicable — V10 is unreleased, no TypeScript callers use draftName. Removed intentionally in 67afc26.
| } | ||
|
|
||
| export interface DraftDescriptor { | ||
| export interface AssertionDescriptor { |
There was a problem hiding this comment.
🔴 Bug: Renaming the exported DraftDescriptor type to AssertionDescriptor without a deprecated alias is a breaking change for consumers of @origintrail-official/dkg-core. If the removal is really deferred to V10.1, keep a compatibility alias such as export type DraftDescriptor = AssertionDescriptor in this release.
There was a problem hiding this comment.
False positive — DraftDescriptor has zero references anywhere in the codebase. The type was renamed to AssertionDescriptor and all usages updated.
|
|
||
| ### Querying | ||
|
|
||
| - `POST /api/query` — SPARQL query with optional `view` (`working-memory`, `shared-working-memory`, `verified-memory`), `agentAddress`, `assertionName`, `verifiedGraph`, `subGraphName`, `includeSharedMemory`, `contextGraphId` parameters |
There was a problem hiding this comment.
🟡 Issue: This advertises subGraphName as part of the general /api/query surface, but the daemon currently rejects any request that combines subGraphName with view. Because this file is agent-consumed, that mismatch will generate 400s. Document subGraphName as legacy-routing-only for now, or implement the combination before listing it here.
There was a problem hiding this comment.
Acknowledged — subGraphName combined with view is documented as deferred to V10.x in the QueryOptions JSDoc. Will add a note to SKILL.md in a follow-up. Not blocking.
There was a problem hiding this comment.
Fixed in PR #106 — added restriction note to SKILL.md that subGraphName is legacy-routing-only and cannot be combined with view.
| For detailed step-by-step workflow recipes and the full endpoint reference, see | ||
| the supporting files in the skill directory: | ||
|
|
||
| - `workflows.md` — 10 workflow recipes with curl examples |
There was a problem hiding this comment.
🟡 Issue: These follow-up docs are referenced as if they ship with the skill, but this PR only adds SKILL.md. Agents that try to open them will hit missing files in the published package. Either add the referenced files under packages/cli/skills/dkg-node/ or remove the references from the served skill.
There was a problem hiding this comment.
Acknowledged — SKILL.md §2 already includes a disclaimer that routes are shipped incrementally. Referenced docs will be added as they are implemented. Not blocking.
| // ── Working Memory Assertion Operations (spec §6) ─────────────────── | ||
|
|
||
| get draft() { | ||
| get assertion() { |
There was a problem hiding this comment.
🔴 Bug: Renaming the public working-memory API from agent.draft.* to agent.assertion.* without keeping a deprecated alias is a breaking change for existing SDK consumers. This PR also drops the underlying draft* methods/types, so upgrades will fail at compile time and at runtime. Keep draft as a compatibility shim until you can ship a documented migration.
There was a problem hiding this comment.
Not applicable — V10 unreleased, no external consumers.
| return { | ||
| graphs: [], | ||
| graphPrefixes: [`did:dkg:context-graph:${contextGraphId}/draft/${opts.agentAddress}/`], | ||
| graphPrefixes: [`did:dkg:context-graph:${contextGraphId}/assertion/${opts.agentAddress}/`], |
There was a problem hiding this comment.
🔴 Bug: This only discovers /assertion/ graphs. Nodes upgraded from the previous working-memory layout still have local /draft/{agent}/... graphs on disk, so view='working-memory' will suddenly return empty results for existing data. Support both prefixes (and the exact-name path) until a storage migration rewrites legacy graph URIs.
There was a problem hiding this comment.
Not applicable — V10 unreleased, no nodes have persisted /draft/ graphs.
| const { publicQuads, privateQuads } = await jsonLdToQuads(input as JsonLdContent); | ||
| quads = [...publicQuads, ...privateQuads]; | ||
| const isEnvelope = !Array.isArray(input) && ('public' in (input as Record<string, unknown>) || 'private' in (input as Record<string, unknown>)); | ||
| if (isEnvelope && privateQuads.length > 0) { |
There was a problem hiding this comment.
🔴 Bug: Dropping privateQuads with only a warning causes partial writes: callers can pass a mixed public/private JSON-LD envelope, get a successful response, and later discover that the private half was discarded. Reject this input with an error instead, or require callers to split the payload before writing to working memory.
There was a problem hiding this comment.
Fixed in f0ac40a — WM assertions are inherently agent-local with no privacy boundary. Now always merges both public and private quads from envelope input instead of warning and dropping. The public/private distinction is meaningless for WM.
| msg.startsWith('SPARQL rejected:') || msg.startsWith('Parse error') || | ||
| /must start with (SELECT|CONSTRUCT|ASK|DESCRIBE)/i.test(msg) || | ||
| msg.includes('was removed in V10') || | ||
| msg.includes('requires agentAddress') || msg.includes('requires contextGraphId') || |
There was a problem hiding this comment.
🔴 Bug: These substring checks do not match the errors currently thrown by the query layer (agentAddress is required... / view '...' requires a contextGraphId...), so malformed view-based requests fall through and return 500 instead of 400. Match the actual phrases here or, better, classify these failures by error type.
| function resolveMarkItDownBin(): string | null { | ||
| const suffix = platform === 'win32' ? '.exe' : ''; | ||
| const binaryName = `markitdown-${platform}-${arch}${suffix}`; | ||
| const binDir = resolve(fileURLToPath(new URL('../../bin', import.meta.url))); |
There was a problem hiding this comment.
🟡 Issue: This assumes a bin/markitdown-* asset ships with the CLI, but this PR only adds skills to packages/cli/package.json and the repo currently has no packages/cli/bin directory. In published installs isMarkItDownAvailable() will stay false unless users separately install markitdown on PATH. Either ship the binary/package it explicitly or make PATH-only support the documented contract.
|
|
||
| ### Querying | ||
|
|
||
| - `POST /api/query` — SPARQL query with optional `view` (`working-memory`, `shared-working-memory`, `verified-memory`), `agentAddress`, `assertionName`, `verifiedGraph`, `subGraphName`, `includeSharedMemory`, `contextGraphId` parameters |
There was a problem hiding this comment.
🟡 Issue: This documents subGraphName as a normal /api/query parameter alongside view, but the daemon now rejects subGraphName whenever view-based routing is used. Agents following this skill will generate 400s. Either document that restriction here or implement sub-graph-aware view resolution.
WM assertions are inherently agent-local — the public/private distinction is meaningless. Always merge both halves instead of warning and dropping private quads from envelope-format input. Made-with: Cursor
| export function contextGraphDraftUri(contextGraphId: string, agentAddress: string, name: string, subGraphName?: string): string { | ||
| if (subGraphName) return `did:dkg:context-graph:${contextGraphId}/${subGraphName}/draft/${agentAddress}/${name}`; | ||
| return `did:dkg:context-graph:${contextGraphId}/draft/${agentAddress}/${name}`; | ||
| export function contextGraphAssertionUri(contextGraphId: string, agentAddress: string, name: string, subGraphName?: string): string { |
There was a problem hiding this comment.
🔴 Bug: switching the working-memory graph URI from /draft/... to /assertion/... is a storage-format break, not just a rename. Existing nodes that already persisted WM graphs under the old prefix will stop being readable after upgrade, and removeSubGraph() now only cleans the new prefix. Please add backward-compatible reads/cleanup or an explicit migration before changing the stored URI, and keep deprecated draft aliases for callers in the meantime.
| msg.startsWith('SPARQL rejected:') || msg.startsWith('Parse error') || | ||
| /must start with (SELECT|CONSTRUCT|ASK|DESCRIBE)/i.test(msg) || | ||
| msg.includes('was removed in V10') || | ||
| msg.includes('requires agentAddress') || msg.includes('requires contextGraphId') || |
There was a problem hiding this comment.
🔴 Bug: these substring checks do not match the actual errors thrown by the query layer (agentAddress is required... and view '...' requires a contextGraphId...). Invalid /api/query requests for the new view mode will therefore fall through as 500s instead of 400s. Match the real message text or use a typed error path here, and add a regression test for the missing-agentAddress / missing-contextGraphId cases.
| const proto = req.headers['x-forwarded-proto'] ?? 'http'; | ||
| const host = req.headers['x-forwarded-host'] ?? req.headers.host ?? `localhost:${config.listenPort ?? 9200}`; | ||
| const baseUrl = `${proto}://${host}`; | ||
| const pipelines = ['text/markdown', ...extractionRegistry.availableContentTypes()]; |
There was a problem hiding this comment.
🟡 Issue: /.well-known/skill.md will always advertise text/markdown as an available extraction pipeline, but the daemon never registers a markdown ExtractionPipeline anywhere. On a stock node this endpoint now reports a capability that does not exist. Either register that pipeline or derive the list entirely from extractionRegistry.
There was a problem hiding this comment.
Fixed in PR #106 — removed hardcoded text/markdown; now only reports what's actually registered in ExtractionPipelineRegistry.
|
|
||
| ## 10. Workflow Recipes | ||
|
|
||
| For detailed step-by-step workflow recipes and the full endpoint reference, see |
There was a problem hiding this comment.
🟡 Issue: this section points readers to workflows.md, api-reference.md, and examples/sparql-recipes.md, but those files are not added anywhere in this PR or shipped under packages/cli/skills/dkg-node/. Agents following the published skill will hit dead references. Either add the referenced files to the package or remove this section until they exist.
There was a problem hiding this comment.
Fixed in PR #106 — replaced dead file references with inline Common Workflows section.
- Fix query error→400 mapping: substring checks now match the actual
error messages from the query engine ('agentAddress is required',
'requires a contextGraphId') so invalid view-based requests return
400 instead of falling through as 500s
- Add Vary header (Host, X-Forwarded-Host, X-Forwarded-Proto) to the
skill endpoint so proxies don't serve cached responses with wrong
Base URL to different callers
- Remove hardcoded 'text/markdown' from extraction pipelines list;
only report what's actually registered in the ExtractionPipelineRegistry
- Document subGraphName restriction in SKILL.md query section (cannot
be combined with view-based routing)
- Replace references to non-existent workflow/api-reference files with
inline Common Workflows section showing actual usage patterns
Made-with: Cursor
Summary
Implementation for dkgv10-spec issues #73 and #74. Companion spec PR: https://github.com/OriginTrail/dkgv10-spec/pull/75
SKILL.MD (dkgv10-spec#73): Ships a V10 Agent Skills document that teaches LLM-powered agents the full DKG node API surface, memory model, and authentication flow.
GET /.well-known/skill.mdpublic endpoint with ETag/304 cachingPUBLIC_PATHSin auth.ts and rate limiter exempt listpackages/cli/skills/dkg-node/SKILL.mdDocument processor (dkgv10-spec#74): Adds the extraction pipeline infrastructure for converting non-RDF files (PDF, DOCX, etc.) to Markdown intermediates.
ExtractionPipelineinterface andExtractionPipelineRegistryin@origintrail-official/dkg-coreMarkItDownConverterimplementation inpackages/cli/src/extraction/that invokes the standalone MarkItDown binaryhandleRequestfor futureimport-filewiringFiles changed
packages/core/src/extraction-pipeline.tspackages/core/src/index.tspackages/cli/src/extraction/markitdown-converter.tspackages/cli/src/extraction/index.tspackages/cli/skills/dkg-node/SKILL.mdpackages/cli/src/daemon.tspackages/cli/src/auth.ts/.well-known/skill.mdto public pathspackages/cli/package.jsonskillsto npm filesTest plan
npx tsc --noEmitpasses in bothpackages/coreandpackages/cli(verified locally)GET /.well-known/skill.mdreturns valid markdown with dynamic node infoGET /.well-known/skill.mdwithIf-None-Matchreturns 304isMarkItDownAvailable()correctly detects binary presenceMade with Cursor