feat(cmdutil): accept a bare node id wherever a node ref is taken (#336) - #339
Conversation
The CLI printed node ids on every --json surface (`id`, and `otherNodeId` on each edge) and then refused to take one back, so reading a node by an id it had just given you meant dropping to `hadron api` with hand-written GraphQL. That gap produced a wrong conclusion in this repo, not just friction: probing tasks:build-review-coverage by loc failed, I recorded it as a live instance of the list-vs-read visibility gap in docs/plans/coding-command-group.md, and had to retract it in #332 once reading by id returned the node. The same root cause was a real bug in `coding preflight lint` — rebuilding a URN per route target from the router's memory broke cross-memory routes — also fixed in #332. Both ref paths now accept one: - ResolveNodeURN short-circuits and returns the id. It does NOT round-trip through resolveUrn, which returns null for a bare id (verified against the live server); node(ref:) and nodeBatch(refs:) both accept ids directly. - BatchNodeRef passes it through verbatim to nodeBatch(refs:). Matched by SHAPE (32 lowercase hex), not by "contains no colon". Colon-free alone would have swallowed a bare loc typed without -m: `node get start-here` would have degraded from a usage error naming -m into a bare "not found". A test pins that message. Kind isn't verified for a bare id — that would need the round-trip resolveUrn can't serve — so a wrong-kind id fails at the caller's own read. Same trade resolveMemoryID already makes for raw memory ids. Verified live: `node get <id>`, `edge ls <id>`, and the batch path `node get <id> <urn>` all resolve; a bare loc without -m still gets its -m guidance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6632b99b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // reNodeID matches an opaque node id as the server mints them: 32 lowercase | ||
| // hex characters. | ||
| var reNodeID = regexp.MustCompile(`^[0-9a-f]{32}$`) |
There was a problem hiding this comment.
For nodes whose primary key uses the still-supported legacy CUID format, such as the documented clr8x2k9p0000 example in internal/cmd/node/revision.go, this regex makes IsNodeID return false, so both ResolveNodeURN and BatchNodeRef reject an ID copied from JSON as an invalid URN instead of sending it to the server. The committed API schema explicitly defines node selectors as accepting “CUID / 32-char hex” (schema/schema.graphql:5582), so limiting the new bare-ID support to hex leaves the reported round-trip failure unfixed for existing CUID-backed nodes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right that the schema names CUID — schema/schema.graphql:5582 says PK (CUID / 32-char hex) — but I'm not widening the gate, and I think the evidence is fairly decisive. Recorded in IsNodeID's doc and pinned by a test rather than left implicit.
A CUID-shaped rule cannot be told apart from an ordinary loc. CUIDs are letter-led lowercase alphanumeric, and so are plenty of locs. Across the 907 nodes in four memories, a ^[a-z][a-z0-9]{7,31}$ rule captures 16 real locs:
conventions deployment discussions findings handoffs
incidents instructions integrations patterns preflight
register services …
So hadron node get preflight would stop reporting "pass -m" and start reporting "not found" — trading a working, helpful path for a theoretical one. That's a strictly worse outcome than the gap it closes.
And the gap is theoretical on this server. Every id I could sample is 32-hex — 907 nodes, 45 memories, 10 revisions, zero CUIDs. The identical CUID / 32-char hex wording appears on the Memory and Agent ref descriptions too, which reads like a description of the PK column type rather than a live guarantee for node ids specifically.
Nothing is lost. The narrow gate doesn't remove any capability: a CUID-backed node is still addressable by its URN exactly as before, and the round-trip failure #336 reports — an id the CLI printed not being accepted back — is fully fixed, because the ids it prints are 32-hex.
I did consider a server probe (try node(ref:) for a colon-free non-hex ref, fall back to the usage error) which would handle CUIDs without the collision. I rejected it because it turns a pure client-side usage error into a network round-trip: TestNodeGetBareLocStillRejected runs against an unreachable http://127.0.0.1:1 today and gets clean guidance, and with a probe that becomes a transport error. Degrading the common typo case to fix an absent one isn't a good trade.
If CUID-backed deployments turn out to be real, I'd rather add an explicit --id flag than guess from shape — happy to file that.
There was a problem hiding this comment.
Pull request overview
This PR updates the CLI’s node-ref parsing so that opaque node IDs emitted in --json output can be fed back into commands that accept node references, aligning the CLI’s input behavior with what the server accepts.
Changes:
- Add
IsNodeID(32-char lowercase hex) detection and short-circuit resolution for bare node IDs. - Extend batch ref canonicalization to accept node IDs (in addition to URNs/qualified refs).
- Add unit + command-level tests asserting the new ID behavior and guarding against regressions (including ensuring
resolveUrnis not called for bare IDs).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/cmdutil/noderef.go | Adds node-id shape detection and uses it in node-ref resolution/canonicalization paths. |
| internal/cmdutil/noderef_test.go | Adds unit tests for IsNodeID and for accepting bare IDs in batch ref handling. |
| internal/cmd/commands_test.go | Adds a command-level regression test for node get <id> ensuring the ID is passed verbatim and resolveUrn is not invoked. |
Suppressed comments (1)
internal/cmdutil/noderef.go:165
- The ResolveNodeURN doc comment now needs to mention that a bare node id is accepted and returned directly (no resolveUrn call), otherwise the top-of-function contract is misleading for callers.
// across memories made anything less ambiguous). A URN that resolves
// to a different entity kind is a usage error too.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Same client-side grammar gate as ResolveNodeURN, so `node get <ref>` and | ||
| // `node get <ref> <ref>` accept exactly the same refs. | ||
| // `node get <ref> <ref>` accept exactly the same refs — including a bare | ||
| // node id, which nodeBatch(refs:) takes verbatim. | ||
| if IsNodeID(ref) { | ||
| return ref, nil | ||
| } |
There was a problem hiding this comment.
Good catch — fixed in 11c5fae. Both BatchNodeRef and ResolveNodeRef applied the -m composition before the id check, so node get <id> -m <memory> composed the id into a node URN and looked up something that cannot exist.
An id is unambiguous by shape, so it now short-circuits ahead of -m in both paths; -m is simply redundant alongside one. Verified live:
$ hadron node get 019e61808abb79a38c66c4cd5a46fb14 -m hadronmemory.com::specs
OK loc=cor:urn:010:01
TestBareIDWinsOverMemoryFlag covers it across an empty -m, both simple spellings, and a compound app-mem memory, and asserts a genuine bare loc + -m still composes to hrn:node:acme.com:kb:start-here unchanged.
Review follow-up on #339. BatchNodeRef and ResolveNodeRef applied -m/--memory composition before the id check, so `node get <id> -m <memory>` treated the id as a loc, composed it into a node URN, and looked up a node that cannot exist. An id is unambiguous by shape, so it now short-circuits ahead of -m in both; -m is simply redundant alongside one. Verified live: `node get <id> -m …::specs` resolves. Also records, in IsNodeID's doc and a test, why the gate is NOT widened to the CUID the schema names alongside 32-char hex. A CUID-shaped rule (letter-led lowercase alphanumeric) cannot be told apart from an ordinary loc: across the 907 nodes sampled in four memories it would capture 16 real ones — `preflight`, `instructions`, `conventions`, `findings` among them — so `node get preflight` would stop saying "pass -m" and start saying "not found". No CUID-shaped id exists on this server either (907 nodes, 45 memories, 10 revisions: all 32-hex), and a CUID-backed node stays addressable by URN, so the narrow gate removes nothing while widening would break refs that work today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… line Review follow-ups on #340. An empty value alone can't be told from an absent flag, so `--to "$TARGET"` with TARGET unset reached the filter as "" and was treated as "no filter": the command returned every edge — 20 of 20 on the dev preflight router, identical to unfiltered — which is the opposite of what the caller asked for, and silent. validate now takes the set of flags cobra saw as Changed and rejects an explicitly-supplied empty (or whitespace-only) value on any of --direction/--name/--to/--from: hadron: --to was given an empty value — omit the flag to not filter on it (a shell variable that expanded to nothing?) Filter values are also trimmed on apply, so a ref padded by shell quoting still matches. agentic-usage.md's surface line named only <node-urn>; `edge list` has taken `<loc> -m <memory>` all along and a bare <node-id> since #339. That line is meant to be an accurate at-a-glance contract, so it now lists all three. validate takes a provided-set rather than the pflag FlagSet so the rule stays unit-testable without cobra. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…om (#337) (#340) * feat(edge): filters for `edge list` — --direction, --name, --to, --from (#337) `edge list` returned every edge in both directions with no way to narrow, so any question smaller than "all edges of this node" meant piping --json into a script. Building and running `coding review|preflight lint` I wrote the same shape of filter a dozen times, and filing hadron-server#845/#850 meant extracting edge ids and labels by hand. The three scripts named in #337 are now single commands: edge ls review -m …::mmdata --direction incoming → 55 edges edge ls preflight -m …::dev --name routes-to → 15 edges edge ls preflight -m …::dev --to findings:prisma-upsert-… → 019ed2e5327b7d1… That last id matches what I extracted by script for hadron-server#850. --to and --from are directional by construction: --to names a node this one points AT (outgoing), --from a node pointing at THIS one (incoming). Each matches the far endpoint by loc OR id, since --json prints both and #339 just made ids first-class refs. Combinations that can only ever match nothing — --to with --direction incoming, --to plus --from, an unknown --direction — are usage errors rather than an empty list that reads like "no such edges". A redacted cross-memory endpoint (#781) carries neither loc nor id, so it never satisfies --to/--from; matching on "" would have made every such edge look like the one asked for. It stays listable unfiltered and by --name. Purely client-side over what GetNode already returns: no new query, no server change, and the row shape is unchanged so existing scripts keep working — a test asserts a filtered row carries every field an unfiltered one does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(edge): reject an explicitly-empty filter value; widen the surface line Review follow-ups on #340. An empty value alone can't be told from an absent flag, so `--to "$TARGET"` with TARGET unset reached the filter as "" and was treated as "no filter": the command returned every edge — 20 of 20 on the dev preflight router, identical to unfiltered — which is the opposite of what the caller asked for, and silent. validate now takes the set of flags cobra saw as Changed and rejects an explicitly-supplied empty (or whitespace-only) value on any of --direction/--name/--to/--from: hadron: --to was given an empty value — omit the flag to not filter on it (a shell variable that expanded to nothing?) Filter values are also trimmed on apply, so a ref padded by shell quoting still matches. agentic-usage.md's surface line named only <node-urn>; `edge list` has taken `<loc> -m <memory>` all along and a bare <node-id> since #339. That line is meant to be an accurate at-a-glance contract, so it now lists all three. validate takes a provided-set rather than the pflag FlagSet so the rule stays unit-testable without cobra. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Holger Selover-Stephan <holger@baragaun.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Closes #336.
The CLI printed node ids on every
--jsonsurface and refused to take one back:Now:
Why this was a correctness issue
It produced a wrong conclusion that reached a merged doc. Probing
tasks:build-review-coverageby loc failed, so I recorded it indocs/plans/coding-command-group.mdas a live instance of the list-vs-read visibility gap — and retracted it in #332 once reading by id returned the node immediately. The same root cause was a genuine bug incoding preflight lint: rebuilding a URN per route target from the router's memory broke cross-memory routes, also fixed in #332. The CLI's own linter fell into the gapnode getcreated.Both ref paths
ResolveNodeURNshort-circuits and returns the id. Deliberately noresolveUrnround-trip — I checked against the live server, andresolveUrnreturnsnullfor a bare id, whilenode(ref:)andnodeBatch(refs:)both accept one:BatchNodeRefpasses it through verbatim tonodeBatch(refs:), sonode get <ref> <ref>takes the same inputs as the single-ref form — which is what the existing comment there already promises.Matched by shape, not by "colon-free"
The issue suggested colon-free → id, following
normalizeResourceRef's precedent. I went narrower: 32 lowercase hex.A bare loc typed without
-m(start-here) is also colon-free, so the loose rule would have degraded a helpful usage error naming-minto a bare "not found". Shape-matching keeps both behaviours, and a test pins the-mmessage:Verified real ids are consistently 32-hex across sampled nodes.
Kind isn't verified
For a bare id the caller gets it back unverified — checking kind would need exactly the round-trip
resolveUrncan't serve. A wrong-kind id fails cleanly at the caller's own read. This is the same traderesolveMemoryIDalready makes ("a raw id — no round-trip needed"), so it's consistent rather than new. The URN path still validates kind as before.Verification
go test ./...— 17 packages green.make lint— 0 issues.gofmtclean.New tests:
IsNodeIDacross ids and every near-miss (uppercase, 31/33 chars, non-hex, bare loc, colon-bearing loc, full URN, prefixed URN);BatchNodeRefaccepting an id and still rejecting a bare loc with an-m-naming message; command-levelnode get <id>assertingResolveUrnis never called andnode(ref:)receives the id verbatim; and the bare-loc rejection.Live, against the acceptance criteria in #336:
🤖 Generated with Claude Code