feat(cmdutil): accept a bare node id wherever a node ref is taken (#336)

internal/cmd/commands_test.go

@@ -2487,3 +2487,47 @@ func TestNodeLsSearchWithSortSeqPaginates(t *testing.T) {
 		t.Errorf("search+seq must page with a large limit, got %v", vars.Limit)
 	}
 }
+
+// #336 — an id the CLI printed must be feedable straight back. The single-ref
+// path short-circuits resolveUrn (which returns null for a bare id), so
+// ResolveUrn is deliberately NOT registered: calling it would fail the test.
+func TestNodeGetAcceptsBareID(t *testing.T) {
+	const id = "019e61808abb79a38c66c4cd5a46fb14"
+	gql, captured := captureGraphQL(t, map[string]string{
+		"GetNode": `{"data":{"node":` + nodeDetailJSON + `}}`,
+	})
+	f, out := testFactory(t)
+	root := NewRootCmd(f)
+	root.SetArgs([]string{"node", "get", id, "--server", gql.URL})
+	if err := root.Execute(); err != nil {
+		t.Fatalf("a bare node id should resolve: %v", err)
+	}
+	if !strings.Contains(out.String(), "The CI is flaky") {
+		t.Errorf("node content missing: %s", out.String())
+	}
+	if _, resolved := captured["ResolveUrn"]; resolved {
+		t.Error("a bare id must not round-trip through resolveUrn — it returns null for one")
+	}
+	var vars struct {
+		Ref string `json:"ref"`
+	}
+	_ = json.Unmarshal(captured["GetNode"], &vars)
+	if vars.Ref != id {
+		t.Errorf("node(ref:) should receive the id verbatim, got %q", vars.Ref)
+	}
+}
+
+// A bare loc without -m is also colon-free; it must keep the usage error that
+// names -m rather than being misread as an id and failing as "not found".
+func TestNodeGetBareLocStillRejected(t *testing.T) {
+	f, _ := testFactory(t)
+	root := NewRootCmd(f)
+	root.SetArgs([]string{"node", "get", "start-here", "--server", "http://127.0.0.1:1"})
+	err := root.Execute()
+	if got := exitcode.FromError(err); got != exitcode.Usage {
+		t.Fatalf("a bare loc without -m should be a usage error, got %d", got)
+	}
+	if !strings.Contains(err.Error(), "-m") {
+		t.Errorf("the error should still point at -m, got %q", err.Error())
+	}
+}

internal/cmdutil/noderef.go

@@ -1,6 +1,7 @@
 package cmdutil
 
 import (
+	"regexp"
 	"strings"
 
 	"github.com/Khan/genqlient/graphql"
@@ -12,6 +13,34 @@ import (
 	"github.com/hadron-memory/hadron-cli/internal/exitcode"
 )
 
+// reNodeID matches an opaque node id as the server mints them: 32 lowercase
+// hex characters.
+var reNodeID = regexp.MustCompile(`^[0-9a-f]{32}$`)
+
+// IsNodeID reports whether ref is a bare node id rather than a URN or loc.
+//
+// Every --json surface prints these (`id`, and `otherNodeId` on each edge) and
+// both node(ref:) and nodeBatch(refs:) accept them, so a ref the CLI just
+// emitted has to be feedable straight back (#336). It is matched by SHAPE, not
+// merely by "contains no colon": a bare loc typed without -m (`start-here`) is
+// also colon-free, and treating that as an id would swap a usage error naming
+// -m for a bare "not found".
+//
+// Note resolveUrn does NOT accept an id — it returns null for one — so callers
+// short-circuit rather than round-tripping.
+//
+// NOT widened to the CUID the schema also names as a PK form ("PK (CUID /
+// 32-char hex)"). A CUID-shaped rule — letter-led lowercase alphanumeric — is
+// indistinguishable from an ordinary loc, and would capture 16 real ones in the
+// sampled memories, including `preflight`, `instructions`, `conventions` and
+// `findings`: `node get preflight` would stop reporting "pass -m" and start
+// reporting "not found". Nothing is lost by the narrow gate — a CUID-backed
+// node is still addressable by its URN — whereas widening breaks refs that work
+// today.
+func IsNodeID(ref string) bool {
+	return reNodeID.MatchString(strings.TrimSpace(ref))
+}
+
 // EdgeDisplay is the human handle for an edge: its name, or its loc when the
 // name is empty (spec 037 — an edge's name is optional, its loc is the
 // identity, so a nameless edge still prints something addressable).
@@ -29,6 +58,12 @@ func EdgeDisplay(name *string, loc string) string {
 // joined and resolved. The memory form is the additive convenience; without it
 // the strict-URN behavior is unchanged.
 func ResolveNodeRef(cmd *cobra.Command, client graphql.Client, memory, ref string) (string, error) {
+	// An id is unambiguous by shape, so it wins before -m is considered:
+	// otherwise `node get <id> -m <memory>` would compose the id into a loc and
+	// look up a node that doesn't exist. -m is simply redundant here.
+	if IsNodeID(ref) {
+		return strings.TrimSpace(ref), nil
+	}
 	if memory = strings.TrimSpace(memory); memory != "" {
 		loc := strings.TrimSpace(ref)
 		if loc == "" {
@@ -70,6 +105,10 @@ func ResolveNodeRef(cmd *cobra.Command, client graphql.Client, memory, ref strin
 // returned Usage error onto their own unavailable list.
 func BatchNodeRef(memory, ref string) (string, error) {
 	ref = strings.TrimSpace(ref)
+	// Same as ResolveNodeRef: an id is unambiguous, so -m can't turn it into a loc.
+	if IsNodeID(ref) {
+		return ref, nil
+	}
 	if memory = strings.TrimSpace(memory); memory != "" {
 		if ref == "" {
 			return "", exitcode.Newf(exitcode.Usage, "a bare node loc is required with -m/--memory <org::memory>")
@@ -107,7 +146,8 @@ func BatchNodeRef(memory, ref string) (string, error) {
 		return ref, nil
 	}
 	// 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. (A bare id was
+	// already returned above, before -m composition.)
 	if strings.Count(ref, "::") < 2 || urnlib.AssertFullyQualifiedUrn(ref, "node") != nil {
 		return "", exitcode.Newf(exitcode.Usage,
 			"%q is not a fully-qualified node URN — expected <org>::<memory>::<loc> (e.g. hadronmemory.com::dev::start-here), or pass -m <org::memory> (single-colon <org:memory> also accepted) with a bare loc", ref)
@@ -140,7 +180,15 @@ func CanonicalNodeRef(ref string) string {
 // across memories made anything less ambiguous). A URN that resolves
 // to a different entity kind is a usage error too.
 func ResolveNodeURN(cmd *cobra.Command, client graphql.Client, ref string) (string, error) {
-	urn := ref
+	urn := strings.TrimSpace(ref)
+	// A bare node id addresses the node directly. resolveUrn would return null
+	// for it, so return it as the id rather than round-tripping — the same
+	// shape as resolveMemoryID's raw-id fast path. Kind isn't verified (that
+	// would need the round-trip resolveUrn can't serve); a wrong-kind id fails
+	// cleanly at the caller's own read.
+	if IsNodeID(urn) {
+		return urn, nil
+	}
 	// Accept either scheme prefix: hrn: is canonical (issue #239), urn: is
 	// legacy-but-accepted-forever. A prefixed URN passes through verbatim
 	// (the server accepts both); a bare ref gets the canonical hrn:node:.

internal/cmdutil/noderef_test.go

@@ -111,3 +111,97 @@ func TestBatchNodeRefValidatesPrefixedRefs(t *testing.T) {
 		}
 	}
 }
+
+// #336 — the CLI prints node ids on every --json surface, so a ref it just
+// emitted has to be feedable straight back.
+func TestIsNodeID(t *testing.T) {
+	for _, ref := range []string{
+		"019e61808abb79a38c66c4cd5a46fb14", // a real id, as printed
+		"  019e61808abb79a38c66c4cd5a46fb14  ",
+		"00000000000000000000000000000000",
+	} {
+		if !IsNodeID(ref) {
+			t.Errorf("%q should be recognised as a node id", ref)
+		}
+	}
+	// Matched by SHAPE, not by "colon-free": a bare loc typed without -m is
+	// also colon-free, and must keep its usage error naming -m.
+	for _, ref := range []string{
+		"start-here",                        // bare loc
+		"tasks:review-changes",              // bare loc with colons
+		"019E61808ABB79A38C66C4CD5A46FB14",  // uppercase
+		"019e61808abb79a38c66c4cd5a46fb1",   // 31 chars
+		"019e61808abb79a38c66c4cd5a46fb14a", // 33 chars
+		"019e61808abb79a38c66c4cd5a46fb1g",  // non-hex
+		"acme.com::kb::start-here",          // fully-qualified URN
+		"hrn:node:acme.com:kb:start-here",   // prefixed URN
+		"",
+	} {
+		if IsNodeID(ref) {
+			t.Errorf("%q must NOT be treated as a node id", ref)
+		}
+	}
+}
+
+// The batch path takes the same refs as the single path, so a bare id must
+// reach nodeBatch(refs:) verbatim rather than being rejected.
+func TestBatchNodeRefAcceptsBareID(t *testing.T) {
+	const id = "019e61808abb79a38c66c4cd5a46fb14"
+	got, err := BatchNodeRef("", id)
+	if err != nil {
+		t.Fatalf("a bare node id should be accepted, got %v", err)
+	}
+	if got != id {
+		t.Errorf("the id must pass through unrewritten: got %q", got)
+	}
+
+	// A non-id colon-free ref still gets the usage error that names -m.
+	if _, err := BatchNodeRef("", "start-here"); err == nil {
+		t.Error("a bare loc without -m should still be rejected")
+	} else if !strings.Contains(err.Error(), "-m") {
+		t.Errorf("the rejection should still point at -m, got %q", err.Error())
+	}
+}
+
+// An id is unambiguous by shape, so -m must not turn it into a loc. Without
+// this, `node get <id> -m <memory>` composed the id into a node URN and looked
+// up a node that doesn't exist.
+func TestBareIDWinsOverMemoryFlag(t *testing.T) {
+	const id = "019e61808abb79a38c66c4cd5a46fb14"
+	for _, mem := range []string{"", "acme.com::kb", "acme.com:kb", "acme.com::agent:app-mem:notes"} {
+		got, err := BatchNodeRef(mem, id)
+		if err != nil {
+			t.Errorf("-m %q: a bare id should still be accepted, got %v", mem, err)
+			continue
+		}
+		if got != id {
+			t.Errorf("-m %q: the id must pass through unrewritten, got %q", mem, got)
+		}
+	}
+
+	// A genuine bare loc with -m still composes, unchanged.
+	got, err := BatchNodeRef("acme.com::kb", "start-here")
+	if err != nil {
+		t.Fatalf("bare loc + -m should compose: %v", err)
+	}
+	if got != "hrn:node:acme.com:kb:start-here" {
+		t.Errorf("composition changed: %q", got)
+	}
+}
+
+// The CUID the schema also names as a PK form is deliberately NOT accepted: a
+// CUID-shaped rule is indistinguishable from an ordinary loc. These are real
+// locs from the sampled memories that such a rule would have swallowed.
+func TestLocsThatACUIDShapedRuleWouldSwallow(t *testing.T) {
+	for _, loc := range []string{
+		"preflight", "instructions", "conventions", "findings",
+		"discussions", "handoffs", "patterns", "register", "services",
+	} {
+		if IsNodeID(loc) {
+			t.Errorf("%q is a real loc and must not be read as an id", loc)
+		}
+		if _, err := BatchNodeRef("", loc); err == nil {
+			t.Errorf("%q without -m should still be a usage error", loc)
+		}
+	}
+}