fix: address remaining PR #104 review feedback#106
fix: address remaining PR #104 review feedback#106branarakic wants to merge 3 commits intov10-rcfrom
Conversation
- 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
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| - `examples/sparql-recipes.md` — SPARQL query patterns | ||
| 1. Create a context graph (`POST /api/context-graph/create`) | ||
| 2. Write triples to shared memory (`POST /api/shared-memory/write`) | ||
| 3. Publish to verified memory (`POST /api/publish`) |
There was a problem hiding this comment.
🔴 Bug: This workflow switches from staged shared-memory writes to the direct publish endpoint. After POST /api/shared-memory/write, the matching promotion step is POST /api/shared-memory/publish; POST /api/publish expects a fresh quad payload and does not publish the SWM data from step 2.
There was a problem hiding this comment.
Fixed in 2f09d31 — step 3 now uses POST /api/shared-memory/publish to promote the SWM data written in step 2, instead of POST /api/publish which expects its own payload.
|
|
||
| **Query across layers:** | ||
|
|
||
| - Shared memory: `{"sparql": "...", "contextGraphId": "...", "includeSharedMemory": true}` |
There was a problem hiding this comment.
🔴 Bug: These query examples do not actually select the layer they claim. includeSharedMemory: true queries the base graph plus SWM, the verified-memory example needs "view": "verified-memory", and the working-memory example below will 400 without contextGraphId/paranetId. Please update the examples to use the view parameter for layer routing and include a graph id for WM.
There was a problem hiding this comment.
Fixed in 2f09d31 — all query examples now use the view parameter: view: "shared-memory", view: "verified-memory", view: "working-memory". Working memory example now includes contextGraphId.
| /must start with (SELECT|CONSTRUCT|ASK|DESCRIBE)/i.test(msg) || | ||
| msg.includes('was removed in V10') || | ||
| msg.includes('requires agentAddress') || msg.includes('requires contextGraphId') || | ||
| msg.includes('agentAddress is required') || msg.includes('requires a contextGraphId') || |
There was a problem hiding this comment.
🟡 Issue: The 400/500 split here still depends on exact exception text, and this change makes that coupling tighter. A small wording change in @origintrail-official/dkg-query will turn these client errors back into 500s, so add a request-level regression test for missing agentAddress and missing contextGraphId view queries or switch to typed errors/status codes.
There was a problem hiding this comment.
Acknowledged — this is a known coupling risk. A typed error hierarchy (e.g. QueryValidationError with a statusCode property) in dkg-query would be the clean fix. Tracking for a follow-up.
- Quick Start step 3 now uses POST /api/shared-memory/publish (promotes SWM data written in step 2) instead of POST /api/publish (which expects its own quad payload) - Query examples now use the view parameter for layer routing: view: "shared-memory", view: "verified-memory", view: "working-memory" - Working memory query example includes required contextGraphId Made-with: Cursor
| -H "Authorization: Bearer $TOKEN" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{"sparql": "SELECT * WHERE { ?s ?p ?o } LIMIT 10", "contextGraphId": "my-context-graph", "includeSharedMemory": true}' | ||
| -d '{"sparql": "SELECT * WHERE { ?s ?p ?o } LIMIT 10", "contextGraphId": "my-context-graph", "view": "shared-memory"}' |
There was a problem hiding this comment.
🔴 Bug: shared-memory is not a supported view value. The query handler only accepts working-memory, shared-working-memory, and verified-memory, so copying this example will return 400 Invalid view. Please update this example, and the repeated shared-memory snippet below, to use the real enum (shared-working-memory, or verified-memory here if the intent is to show the post-publish result).
There was a problem hiding this comment.
Fixed in ef5c1a5 — corrected shared-memory to shared-working-memory in both the Quick Start and Common Workflows query examples. These now match the GET_VIEWS enum in core.
shared-memory → shared-working-memory (matches GET_VIEWS in core) Made-with: Cursor
| -H "Authorization: Bearer $TOKEN" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{"sparql": "SELECT * WHERE { ?s ?p ?o } LIMIT 10", "contextGraphId": "my-context-graph", "includeSharedMemory": true}' | ||
| -d '{"sparql": "SELECT * WHERE { ?s ?p ?o } LIMIT 10", "contextGraphId": "my-context-graph", "view": "shared-working-memory"}' |
There was a problem hiding this comment.
🔴 Bug: Step 3 publishes from shared memory with clearAfter defaulting to true, so querying view: "shared-working-memory" in the next step will usually come back empty. If this quick start is meant to confirm the publish, query the context graph without the SWM view instead, or add "clearAfter": false to the publish example.
| /must start with (SELECT|CONSTRUCT|ASK|DESCRIBE)/i.test(msg) || | ||
| msg.includes('was removed in V10') || | ||
| msg.includes('requires agentAddress') || msg.includes('requires contextGraphId') || | ||
| msg.includes('agentAddress is required') || msg.includes('requires a contextGraphId') || |
There was a problem hiding this comment.
🔴 Bug: agent.query() still throws Invalid sub-graph name for query: ... for bad subGraphName values, but this 400 allowlist does not catch that case, so malformed client input still falls through as a 500. Since this PR now documents subGraphName, add that validation error here or reject it before calling agent.query.
1 participant
Summary
Follow-up to PR #104 (merged). Addresses the remaining Codex Review bot findings:
daemon.ts:2119): The substring checks for catching query-layer errors and returning 400 didn't match the actual error messages (agentAddress is required...,requires a contextGraphId...). Fixed to use the correct substrings, preventing invalid view-based requests from falling through as 500s.Varyheader (daemon.ts:1246): The skill endpoint response body varies byHost/X-Forwarded-*headers but lacked aVaryheader, so shared proxies could serve cached responses with the wrong Base URL. AddedVary: Host, X-Forwarded-Host, X-Forwarded-Proto.text/markdownfalsely advertised (daemon.ts:1230): The extraction pipelines list hardcodedtext/markdowneven though no such pipeline is registered. Now reports only what's actually in theExtractionPipelineRegistry.subGraphName+viewincompatibility (SKILL.md:114): Documented the restriction thatsubGraphNameis legacy-routing-only and cannot be combined withview.SKILL.md:187): Replaced dead references toworkflows.md,api-reference.md, andexamples/sparql-recipes.mdwith an inline Common Workflows section.Test plan
tsc --noEmitclean for packages/cliMade with Cursor