fix(cli): require explicit quads for batch verification#638
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| let quads: Array<{ subject: string; predicate: string; object: string; graph: string }> = []; | ||
| if (Array.isArray(parsed.quads)) { | ||
| quads = parsed.quads.map((q: any) => ({ | ||
| if (!Array.isArray(parsed.quads)) { |
There was a problem hiding this comment.
🔴 Bug: This turns quads from an optional field into a required one for the public verify-batch API. The existing contract in the repo docs/changelog still allows callers to omit quads and verify against locally persisted SWM/data-graph content, so this will 400 existing clients even in single-batch/public-CG cases that were previously valid. If the local-reconstruction path is unsafe only for batch-scoped requests, keep rejecting that narrower case (for example when batchId is set) or add a compatibility/versioned path instead of breaking the endpoint outright.
| log "✓ Scenario 1: verify-batch returned ok=true ($QUADS_CONSIDERED quads considered)" | ||
| log "verify-batch missing-quads response: $VERIFY_MISSING_QUADS" | ||
| MISSING_QUADS_ERROR=$(parse_json "$VERIFY_MISSING_QUADS" '.error') | ||
| if printf '%s' "$MISSING_QUADS_ERROR" | grep -q 'requires explicit `quads`'; then |
There was a problem hiding this comment.
🟡 Issue: This scenario only checks for an error substring and never asserts the HTTP status. A regression that returns a 500 with the same message would still pass the test, so it does not actually prove request validation is happening. Capture the response status from curl and assert 400 here in addition to the error text.
| if (!Array.isArray(parsed.quads)) { | ||
| return jsonResponse(res, 400, { | ||
| error: | ||
| `verify-batch requires explicit \`quads\` in the request body. ` + |
There was a problem hiding this comment.
🔴 Bug: Making quads mandatory turns this into a full-batch upload API, which is a breaking contract change for existing callers and also caps verification to whatever fits under SMALL_BODY_BYTES (256 KB) because the whole plaintext batch now has to be posted back to the daemon. Large batches that were previously verifiable from local state will start failing at the HTTP boundary before hashing. Please either keep a backwards-compatible server-side path for safe cases, or raise/stream the request limit and version/deprecate the old behavior explicitly.
|
Superseded by PR #645 (merged into integration) and #649 (release: rc.10 testnet-ready cut). All commits from this PR are now on |
…ANGELOG fix PR #625's runbook documented HTTP and CLI surfaces that don't exist on the rc.10 daemon — operators following it from step 1 would 404 / 401 their way through every section. PR #638's LU-8 CHANGELOG entry promised a server-side reconstruction path for `verify-batch` that the actual route refuses. Both are doc bugs only, no code change. docs/RFC38_LU6_TWO_LAPTOP_TESTNET_RUNBOOK.md: - Replaces `dkg show` (never a real top-level command) with the actual surface: `dkg status` (peerId/multiaddrs/role), `dkg auth show` (token, stripped of comments), and `GET /api/agent/identity` for the agent EOA. Same for `dkg show-cg` (compute the wire id as `keccak256(<cgId>)` since there's no CLI shortcut) and `dkg shared-memory host-mode stats` (use `GET /api/shared-memory/ host-mode/stats` directly). - Every `$(cat ~/.dkg/auth.token)` interpolation now uses `dkg auth show` instead. The token file has a commented-header preamble by default, so the literal `cat` would inject `#\n<token>` into the `Authorization` header and 401 even on a healthy node. `dkg auth show` strips comments + blank lines, matching what `packages/node-ui/vite.config.ts` does for the same reason. - `/api/context-graph/list` filter corrected — the response is `{ contextGraphs: [...] }` (envelope, not bare array) and items expose `accessPolicy` (numeric: 0=public, 1=curated), not `access`. The old `jq '.[] | select(.access=="curated")'` filter would have silently matched nothing. - Member catchup now pins `peerId` to the core's libp2p identity. Without `peerId`, `/api/shared-memory/catchup` fans out to every connected peer — which in a two-laptop+core topology can hit laptop A directly or no peer at all, and won't reliably validate the "via the core" host-catchup path the runbook claims to verify. Showed how to grab the peer id from `/api/status` first. - Replaced the `/api/shared-memory/list?contextGraphId=...` curl (404, no such route) with a SPARQL `SELECT (COUNT(*) AS ?n)` via `/api/query` against the `_shared_memory` graph suffix — the same shape every devnet script uses for the equivalent assertion. CHANGELOG.md: - LU-8 entry no longer claims "when `quads` is omitted the route reconstructs from the local SWM or post-publish CG data graph". That hasn't been true since the safety hardening in the published route (`packages/cli/src/daemon/routes/memory.ts:1042-1049`): `quads` is **required**, HTTP 400 on omission, because the daemon can't safely identify a single published batch's leaves inside a CG-wide store. Devnet scenario `scripts/devnet-test-rfc38-lu8.sh` SCENARIO 1 pins this contract with a HTTP-400 assertion. The CHANGELOG now matches the route's actual behaviour + rationale. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
Verification
Notes