Skip to content

feat: add SKILL.MD agent onboarding and MarkItDown document processor#104

Merged
branarakic merged 13 commits intov10-rcfrom
feat/skill-md-and-document-processor
Apr 9, 2026
Merged

feat: add SKILL.MD agent onboarding and MarkItDown document processor#104
branarakic merged 13 commits intov10-rcfrom
feat/skill-md-and-document-processor

Conversation

@branarakic
Copy link
Copy Markdown
Contributor

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.

    • New GET /.well-known/skill.md public endpoint with ETag/304 caching
    • Dynamic node info injected at serve-time (version, peer ID, node role, extraction pipelines, subscribed context graphs)
    • Added to PUBLIC_PATHS in auth.ts and rate limiter exempt list
    • Skill file ships with the npm package via packages/cli/skills/dkg-node/SKILL.md
  • Document processor (dkgv10-spec#74): Adds the extraction pipeline infrastructure for converting non-RDF files (PDF, DOCX, etc.) to Markdown intermediates.

    • ExtractionPipeline interface and ExtractionPipelineRegistry in @origintrail-official/dkg-core
    • MarkItDownConverter implementation in packages/cli/src/extraction/ that invokes the standalone MarkItDown binary
    • Registry initialized at daemon startup with graceful degradation when binary is unavailable
    • Extraction registry passed through to handleRequest for future import-file wiring

Files changed

File Change
packages/core/src/extraction-pipeline.ts New: ExtractionPipeline interface + registry
packages/core/src/index.ts Export extraction types
packages/cli/src/extraction/markitdown-converter.ts New: MarkItDown binary wrapper
packages/cli/src/extraction/index.ts New: barrel export
packages/cli/skills/dkg-node/SKILL.md New: V10 Agent Skills document
packages/cli/src/daemon.ts SKILL.md endpoint, extraction registry init
packages/cli/src/auth.ts Add /.well-known/skill.md to public paths
packages/cli/package.json Add skills to npm files

Test plan

  • npx tsc --noEmit passes in both packages/core and packages/cli (verified locally)
  • GET /.well-known/skill.md returns valid markdown with dynamic node info
  • GET /.well-known/skill.md with If-None-Match returns 304
  • Node starts without MarkItDown binary (graceful degradation log message)
  • Node starts with MarkItDown binary (extraction pipelines logged)
  • isMarkItDownAvailable() correctly detects binary presence

Made with Cursor

Comment thread packages/cli/src/daemon.ts Outdated
if (cachedSkillMd) return cachedSkillMd;
const skillPath = new URL('../skills/dkg-node/SKILL.md', import.meta.url);
cachedSkillMd = readFileSync(skillPath, 'utf-8');
return cachedSkillMd;
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: 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.

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.

Valid — will fix the strict compilation error separately. This is in the SKILL.md feature commit, not the assertion rename.

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 d74fc99

Comment thread packages/cli/skills/dkg-node/SKILL.md Outdated
**Step 1 — Register** (if not already registered):

```bash
curl -X POST $BASE_URL/api/agent/register \
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 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.

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 — the SKILL.md is a scaffolding placeholder. Routes will be implemented in subsequent PRs. Adding a disclaimer to the skill file.

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 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.

Comment thread packages/cli/src/daemon.ts Outdated
// 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}`;
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: 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.

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.

Valid point. Will derive from config or request headers in a follow-up. Localhost is the correct default for local dev/testing.

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 d74fc99 — baseUrl now derived from request Host header + X-Forwarded-Proto/X-Forwarded-Host for reverse proxy support.

Comment thread packages/cli/src/daemon.ts Outdated
const listenPort = config.listenPort ?? 9200;
const baseUrl = `http://localhost:${listenPort}`;
const cgMap = agent.getSubscribedContextGraphs();
const cgNames = [...cgMap.keys()];
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: /.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.

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.

Good catch. Will filter to public context graphs only when auth is wired up.

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 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);
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: 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.

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.

Valid — will switch to fileURLToPath() in a follow-up. Not blocking since we do not target Windows for V10.0.

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 d74fc99 — switched to fileURLToPath() from node:url for correct Windows path handling.

Comment thread packages/core/src/constants.ts Outdated
}

/** @deprecated Use contextGraphAssertionUri */
export const contextGraphDraftUri = contextGraphAssertionUri;
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 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.

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.

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.

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

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> {
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: 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.

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.

Same as above — intentional breaking rename, not a released API. No external callers exist. Removing the deprecated wrappers entirely for a clean codebase.

Comment thread packages/cli/src/daemon.ts Outdated
// 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}`;
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: 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.

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.

Duplicate of the earlier baseUrl comment — addressing together.

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 d74fc99 (same fix as the earlier baseUrl comment).

Comment thread packages/cli/skills/dkg-node/SKILL.md Outdated
**Step 1 — Register** (if not already registered):

```bash
curl -X POST $BASE_URL/api/agent/register \
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 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.

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.

Duplicate of the earlier SKILL.md routes comment — addressing together.

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 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}`;
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: 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.

Comment thread packages/cli/src/daemon.ts Outdated
// 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}`;
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: 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.

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 d74fc99 — baseUrl now derived from request headers, not listenPort.

Comment thread packages/cli/skills/dkg-node/SKILL.md Outdated
**Step 1 — Register** (if not already registered):

```bash
curl -X POST $BASE_URL/api/agent/register \
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 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.

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 d74fc99 — disclaimer added noting incremental rollout.

throw new Error('agentAddress is required for the working-memory view');
}
if (opts.draftName) {
if (opts.assertionName) {
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 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() {
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: 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.

Comment thread packages/core/src/constants.ts Outdated

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 {
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: 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() {
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 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.

Comment thread packages/cli/src/daemon.ts Outdated
peerId: agent.peerId,
nodeRole: config.nodeRole ?? 'edge',
extractionPipelines: [...new Set(pipelines)],
contextGraphs: cgNames,
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: /.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.

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 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.

Comment thread packages/cli/skills/dkg-node/SKILL.md Outdated

### Working Memory (WM) — Private drafts

- `POST /api/draft/create` — create a named draft
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 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.

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 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];
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: 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.

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 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() {
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: 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.

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.

Already addressed — get draft() { return this.assertion; } exists at the bottom of the getter. Existing callers of agent.draft.* continue to work unchanged.

Comment thread packages/core/src/constants.ts Outdated

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 {
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 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.

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 586b762contextGraphDraftUri is re-exported as a deprecated alias pointing to contextGraphAssertionUri.

Comment thread packages/cli/skills/dkg-node/SKILL.md Outdated
curl -X POST $BASE_URL/api/context-graph/create \
-H "Authorization: Bearer $TOKEN" \
-H "Content-Type: application/json" \
-d '{"name": "my-context-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: 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.

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 9408de8 — Quick Start step 1 now shows {"id": "my-context-graph", "name": "My Context Graph"} with both required fields.

Comment thread packages/cli/skills/dkg-node/SKILL.md Outdated
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"}'
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 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.

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 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.

Comment thread packages/cli/package.json
"files": [
"dist",
"network",
"skills",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Issue: The new 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.

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 — 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.

Branimir Rakic added 7 commits April 9, 2026 23:34
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
- 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
@branarakic branarakic force-pushed the feat/skill-md-and-document-processor branch from 1d52e8e to 586b762 Compare April 9, 2026 21:56
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;
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: 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.

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 9408de8share() 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.

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 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}`;
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 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.

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.

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.

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.

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() {
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: 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.

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.

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.

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.

Intentional rename for V10.0. The agent.draft getter still exists as a deprecated alias delegating to agent.assertion.

Comment thread packages/cli/skills/dkg-node/SKILL.md Outdated
curl -X POST $BASE_URL/api/context-graph/create \
-H "Authorization: Bearer $TOKEN" \
-H "Content-Type: application/json" \
-d '{"name": "my-context-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: 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.

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 9408de8 — Quick Start step 1 now shows {"id": "my-context-graph", "name": "My Context Graph"} with both required fields.

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 9408de8 — quick-start example now includes both id and name fields.

Comment thread packages/cli/skills/dkg-node/SKILL.md Outdated
-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"},
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: 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".

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 9408de8 — example quads now use empty graph: "" (server normalizes to target graph) and proper RDF literal format for string values.

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 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 {
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 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() {
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: 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',
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 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.

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 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
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 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
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: 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() {
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 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.

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 52d106c — added get draft() as a deprecated alias delegating to get assertion().

Comment thread packages/core/src/constants.ts Outdated
}

/** @deprecated Use contextGraphAssertionUri */
export const contextGraphDraftUri = contextGraphAssertionUri;
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 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.

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.

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 });
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 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.

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 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)) {
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 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.

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 2ba7db9 — the catch block now maps view-related errors (removed views, missing agentAddress/contextGraphId, unsupported combinations) to 400 instead of 500.

Comment thread packages/core/src/constants.ts Outdated
}

/** @deprecated Use contextGraphAssertionUri */
export const contextGraphDraftUri = contextGraphAssertionUri;
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 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.

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 previously — intentional for V10.0 (unreleased). No persisted WM data exists under the old URI shape since V10 has not shipped.

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.

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;
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: 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.

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 2ba7db9QueryOptions now accepts draftName as a deprecated alias for assertionName, normalized internally via options.assertionName ?? options.draftName.

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 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> {
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 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.

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 2ba7db9 — added deprecated draftCreate/draftWrite/draftQuery/draftPromote/draftDiscard method aliases on DKGPublisher, each delegating to the corresponding assertion* method.

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 needed — V10 is unreleased, no downstream consumers of @origintrail-official/dkg-publisher exist. All draft* method aliases removed in 67afc26.

Branimir Rakic added 2 commits April 10, 2026 01:01
- 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
Comment thread packages/agent/src/dkg-agent.ts Outdated
}

/** @deprecated Use `assertion`. Will be removed in V10.1. */
get draft() {
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: 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.

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 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;
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: 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.

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 applicable — V10 is unreleased, no TypeScript callers use draftName. Removed intentionally in 67afc26.

}

export interface DraftDescriptor {
export interface AssertionDescriptor {
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: 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.

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.

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
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 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.

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 — 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.

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 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
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: 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.

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 — 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() {
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: 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.

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 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}/`],
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 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.

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 applicable — V10 unreleased, no nodes have persisted /draft/ graphs.

Comment thread packages/agent/src/dkg-agent.ts Outdated
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) {
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: 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.

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 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') ||
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: 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)));
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 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
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 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 {
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: 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') ||
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: 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.

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 PR #106 (3bd6d9c) — substring checks now match the actual error messages from the query engine.

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()];
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: /.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.

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 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
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 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.

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 PR #106 — replaced dead file references with inline Common Workflows section.

@branarakic branarakic merged commit 6f7e551 into v10-rc Apr 9, 2026
3 checks passed
branarakic pushed a commit that referenced this pull request Apr 10, 2026
- 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
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