From 91e2a42916659dd5d0b9e05b36222e38d0d972b9 Mon Sep 17 00:00:00 2001 From: Holger Selover-Stephan Date: Sun, 25 Oct 2026 13:49:37 +0000 Subject: [PATCH 1/2] test: pin guarded node update revision forwarding Worker: Jane (cli-engineer) --- .../cmd/node_expected_revision_820_test.go | 226 ++++++++++++++++++ 1 file changed, 226 insertions(+) create mode 100644 internal/cmd/node_expected_revision_820_test.go diff --git a/internal/cmd/node_expected_revision_820_test.go b/internal/cmd/node_expected_revision_820_test.go new file mode 100644 index 00000000..549391ed --- /dev/null +++ b/internal/cmd/node_expected_revision_820_test.go @@ -0,0 +1,226 @@ +package cmd + +import ( + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" +) + +type revision820Request struct { + OperationName string + Variables map[string]json.RawMessage +} +type revision820NoRead struct{ reads int } + +func (r *revision820NoRead) Read([]byte) (int, error) { r.reads++; return 0, io.EOF } + +func revision820Run(t *testing.T, kind string, conflict bool, args ...string) (string, string, int, []revision820Request, int) { + t.Helper() + var node map[string]any + if err := json.Unmarshal([]byte(nodeDetailJSON), &node); err != nil { + t.Fatal(err) + } + node["revision"] = 99 + node["role"] = nil + node["isRunnable"] = false + switch kind { + case "Task": + node["isRunnable"] = true + case "Spec": + node["role"] = "spec.rule" + case "Review": + node["role"] = "review" + } + raw, _ := json.Marshal(node) + var mu sync.Mutex + requests := []revision820Request{} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var q revision820Request + if err := json.NewDecoder(r.Body).Decode(&q); err != nil { + t.Error(err) + return + } + mu.Lock() + requests = append(requests, q) + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + switch q.OperationName { + case "ResolveUrn": + fmt.Fprint(w, resolveNodeJSON) + case "GetNode", "GetNodeRaw": + fmt.Fprintf(w, `{"data":{"node":%s}}`, raw) + default: + if !strings.HasPrefix(q.OperationName, "Update") { + t.Errorf("unexpected %s", q.OperationName) + return + } + if conflict { + fmt.Fprint(w, `{"errors":[{"message":"revision changed","extensions":{"code":"NODE_WRITE_CONFLICT","currentRevision":9}}]}`) + return + } + root := "update" + kind + "Node" + if kind == "" { + root = "updateNode" + } + fmt.Fprintf(w, `{"data":{%q:%s}}`, root, raw) + } + })) + defer srv.Close() + f, out := testFactory(t) + stderr := &strings.Builder{} + f.IOStreams.ErrOut = stderr + reader := &revision820NoRead{} + f.IOStreams.In = reader + root := NewRootCmd(f) + root.SetArgs(append([]string{"node", "update", nodeURN, "--server", srv.URL}, args...)) + err := root.Execute() + code := 0 + if err != nil { + code = renderError(f, err) + } + mu.Lock() + result := append([]revision820Request(nil), requests...) + mu.Unlock() + return out.String(), stderr.String(), code, result, reader.reads +} +func TestNodeExpectedRevision820Doors(t *testing.T) { + for _, kind := range []string{"", "Task", "Spec", "Review"} { + for _, clear := range []bool{false, true} { + for _, rev := range []string{"", "7", "2147483647"} { + t.Run(fmt.Sprintf("%s/clear%v/rev%s", kind, clear, rev), func(t *testing.T) { + args := []string{"--name", "new", "--json"} + if clear { + args = append(args, "--role", "") + } + if rev != "" { + args = append(args, "--expected-revision", rev) + } + out, stderr, code, requests, _ := revision820Run(t, kind, false, args...) + if code != 0 { + t.Fatalf("%d %s %s", code, out, stderr) + } + mutations := 0 + for _, q := range requests { + if strings.HasPrefix(q.OperationName, "Update") { + mutations++ + want := "Update" + kind + "Node" + if kind == "" { + want = "UpdateNode" + } + if clear { + want += "ClearingRole" + } + if q.OperationName != want { + t.Fatalf("door %s want%s", q.OperationName, want) + } + vars := q.Variables + if !clear { + if err := json.Unmarshal(vars["input"], &vars); err != nil { + t.Fatal(err) + } + } + got, ok := vars["expectedRevision"] + if rev == "" { + if ok { + t.Fatalf("omitted guard serialized: %s", got) + } + } else if !ok || string(got) != rev { + t.Fatalf("expected%s got%s", rev, got) + } + } + } + if mutations != 1 { + t.Fatalf("mutations%d requests%v", mutations, requests) + } + }) + } + } + } +} +func TestNodeExpectedRevision820EarlyRefusals(t *testing.T) { + cases := [][]string{} + for _, v := range []string{"0", "-1", "2147483648", "1.5", "abc"} { + cases = append(cases, []string{"--name", "new", "--expected-revision", v}) + } + cases = append(cases, []string{"--expected-revision", "3"}) + for _, merge := range [][]string{{"--data-merge", "-"}, {"--data-merge-file", "/not-a-readable-fixture"}, {"--data-merge", "-", "--name", "new"}, {"--content", "-", "--data-merge", "{}"}} { + cases = append(cases, append(merge, "--expected-revision", "7")) + } + for _, args := range cases { + t.Run(strings.Join(args, "/"), func(t *testing.T) { + _, stderr, code, requests, reads := revision820Run(t, "", false, args...) + if code != 2 || len(requests) != 0 || reads != 0 { + t.Fatalf("exit%d requests%d stdin%d: %s", code, len(requests), reads, stderr) + } + if len(args) == 2 && !strings.Contains(stderr, "nothing to update") { + t.Fatal(stderr) + } + }) + } +} +func TestNodeExpectedRevision820DryRun(t *testing.T) { + for _, j := range []bool{false, true} { + t.Run(fmt.Sprint(j), func(t *testing.T) { + args := []string{"--name", "new", "--expected-revision", "7", "--dry-run"} + if j { + args = append(args, "--json") + } + out, stderr, code, requests, _ := revision820Run(t, "", false, args...) + if code != 0 { + t.Fatalf("%d %s %s", code, out, stderr) + } + for _, q := range requests { + if strings.HasPrefix(q.OperationName, "Update") { + t.Fatal("dry-run wrote", q) + } + } + if !strings.Contains(out, "not validated") { + t.Fatalf("CAS claim not advisory: %s", out) + } + if j { + var d map[string]any + if err := json.Unmarshal([]byte(out), &d); err != nil { + t.Fatal(err) + } + if d["expectedRevision"] != float64(7) { + t.Fatal(d) + } + } + }) + } +} +func TestNodeExpectedRevision820Conflict(t *testing.T) { + for _, j := range []bool{false, true} { + t.Run(fmt.Sprint(j), func(t *testing.T) { + args := []string{"--name", "new", "--expected-revision", "7"} + if j { + args = append(args, "--json") + } + out, stderr, code, requests, _ := revision820Run(t, "", true, args...) + if code != 5 { + t.Fatalf("%d %s %s", code, out, stderr) + } + mutations := 0 + for _, q := range requests { + if strings.HasPrefix(q.OperationName, "Update") { + mutations++ + } + } + if mutations != 1 || len(requests) != 3 { + t.Fatalf("retry or extra request: %v", requests) + } + if j { + if !strings.Contains(out, `"currentRevision": 9`) { + t.Fatal(out) + } + } else if !strings.Contains(stderr, "currentRevision: 9") { + t.Fatal(stderr) + } + }) + } +} From 9ce0ebfd8e5184832d257beee4fb8ddefc910beb Mon Sep 17 00:00:00 2001 From: Holger Selover-Stephan Date: Sun, 25 Oct 2026 13:53:51 +0000 Subject: [PATCH 2/2] Forward explicit node update revision preconditions Reject guarded data merges before reads, validate GraphQL Int bounds, and keep dry-run advisory. Omitted guards retain last-write-wins; conflict metadata and no-retry behavior stay unchanged. Worker: Jane (cli-engineer) --- internal/cmd/agentic/agentic-usage.md | 15 +++- internal/cmd/node/update.go | 88 ++++++++++++------- .../cmd/node_expected_revision_820_test.go | 29 ++++++ 3 files changed, 100 insertions(+), 32 deletions(-) diff --git a/internal/cmd/agentic/agentic-usage.md b/internal/cmd/agentic/agentic-usage.md index 086fdceb..f7d0e3a6 100644 --- a/internal/cmd/agentic/agentic-usage.md +++ b/internal/cmd/agentic/agentic-usage.md @@ -320,7 +320,7 @@ non-zero naming the nodes it already created, each complete with its edges. hadron auth login | logout | whoami | status | token create|list|validate|revoke hadron usage events [--memory ] [--loc ] [--type ] [--limit 1-200] [--offset N] hadron memory list [--shared-with-me | [--owned-by-me] [--include-agent-system]] | get (cli#1014: shows the stored last-sync outcome, Memory.lastSyncReport v1; `--json` adds `lastSyncedAt`, `syncError` (URL credentials masked), `syncReport` (the parsed v1 object {version,outcome,recordedAt,entries[{kind,loc,fromLoc?,code}],code?}, every entry, unknown kind/code passed through) and `syncReportStatus` recorded|none|unavailable; an unreadable report is `syncReport: null` + `unavailable`, never clean; text prints a `sync:` block only for a memory with a source or sync history, quoting server strings and capping at 50 entries) | set [] [--org | --owner-me | --app --agent ] [--class ] [--max-rev-count ] [--draft-corpus] [--schema | --schema-file ] [--listed-on-marketplace[=false]] (update only, cli#592: the opt-in marketplace catalogue flag, cor:acl:080:04, advertisement not access; omitted preserves it; `get` shows it as `marketplace: listed|not listed` and `--json` `listedOnMarketplace`, also on a `set` update's receipt) | attach --app --agent | set-active | rm | clone --target-urn hrn:mem:: | transfer (--org | --user ) [--class ] [--reset-group-members] [--apply | --yes] | extract [--move] | export [--out ] | member list|add|set-role|rm --user [--role ] | share list|create|set-role|revoke --grantee [--role ] | subscription list|create|set-role|rm --org [--role ] | encrypt --data-key - | link-user --external-user [--data-key -] --yes | approve-all -m [--yes] | validate [--check ]... [--limit N] [--fail-on-findings] | reembed-failed | config get | config rule add|update [--author-task ] [--validation-task ] [--description-node ] [--writers all|admin|owner] [--validate-by agent|platform] [--strict-sub-roles[=false]] [--enabled[=false]] | config rule rm --yes | config rule unlock [--yes] | config template list [--owner-server | --owner-org | --owner-me | --owner-app ] | config template get | config template create --file (--owner-server | --owner-org | --owner-me | --owner-app ) | config template update --file [--expected-revision ] | config template rm --yes | config template apply [--dry-run] [--expected-revision ] [--yes] | usage -hadron node list [-m ] [--prefix ] [--type ] [--object-type ] [--tag ]... [--where ] [--sort-property ] [--with-properties] [--with-data] [--sort-seq asc|desc] [--seq-gt N] | get ... [--raw] | get ... -m [--raw] | get | -m --body-only | get --prefix -m | add [--type ] [--object-type ] [--data |--data-file ] [--properties |--properties-file ] | update [--content | --content-file ] [--force] [--dry-run] [--type ] [--object-type |""] [--data |--data-file |--data-merge |--data-merge-file ] [--properties |--properties-file ] | move (--to-urn | --to-memory ) | clone (--to-urn | --to-memory ) | merge --into [--field ]... [--delete-source] --yes | rm [--hard] [--recursive|-r] | export [-o ] [--format md|json|pdf] | import > [-m ] [--with-edges] [--task [--task-args ] [--app ]] | approve [-m ] [--revision N] | verify [-m ] | mint -m [--dry-run] [--yes] | sign [-m ] [--revision N] [--signer ] | signatures [-m ] [--check] | signing-request get | signing-request list [-m ] [--state open|completed|declined|expired|stale|withdrawn] [--limit 1-100] | signing-request withdraw | revision list [-m ] [--limit N] | revision get | revision restore [--truncate [--yes]] | revision label --label | revision delete [--yes] | revision clear [-m ] [--yes] | recovery list -m [--limit 1-200] [--offset N | --all] | recovery get || [-m ] [--content] | recovery graph -m [--limit 1-200] [--offset N | --all] | restore || [-m ] | next [--after ] [-m ] [--raw] | open [-m ] [--print] +hadron node list [-m ] [--prefix ] [--type ] [--object-type ] [--tag ]... [--where ] [--sort-property ] [--with-properties] [--with-data] [--sort-seq asc|desc] [--seq-gt N] | get ... [--raw] | get ... -m [--raw] | get | -m --body-only | get --prefix -m | add [--type ] [--object-type ] [--data |--data-file ] [--properties |--properties-file ] | update [--expected-revision N] [--content | --content-file ] [--force] [--dry-run] [--type ] [--object-type |""] [--data |--data-file |--data-merge |--data-merge-file ] [--properties |--properties-file ] | move (--to-urn | --to-memory ) | clone (--to-urn | --to-memory ) | merge --into [--field ]... [--delete-source] --yes | rm [--hard] [--recursive|-r] | export [-o ] [--format md|json|pdf] | import > [-m ] [--with-edges] [--task [--task-args ] [--app ]] | approve [-m ] [--revision N] | verify [-m ] | mint -m [--dry-run] [--yes] | sign [-m ] [--revision N] [--signer ] | signatures [-m ] [--check] | signing-request get | signing-request list [-m ] [--state open|completed|declined|expired|stale|withdrawn] [--limit 1-100] | signing-request withdraw | revision list [-m ] [--limit N] | revision get | revision restore [--truncate [--yes]] | revision label --label | revision delete [--yes] | revision clear [-m ] [--yes] | recovery list -m [--limit 1-200] [--offset N | --all] | recovery get || [-m ] [--content] | recovery graph -m [--limit 1-200] [--offset N | --all] | restore || [-m ] | next [--after ] [-m ] [--raw] | open [-m ] [--print] hadron object create -m --type --fields |--fields-file [--key ] [--name ] | get | update --fields |--fields-file [--reason ] | delete [--hard] --yes | find -m --type [--match ] [--where ] [--sort ] [--limit N] [--offset N] hadron asset list -m [--mine] [--mime ] [--include-deleted] [--limit N] [--offset N] | get [-o |-] [--force] | url [-m ] | upload -m [--mime ] [--name ] [--description ] | rm [--yes] | restore | public-link (--on | --off) [--yes] | link --node [--name ] [--description ] hadron task run | -m [--arg k=v]... [--app [--as-self]] @@ -3474,3 +3474,16 @@ root node read, no transcript fetch or cursor/binding write. The root read recor a read and may decrypt internally. A concealed/missing/failed root read leaves the URL null; initial lookup failures remain errors. Existing channel get/list commands are unchanged. + +## Guarded node field updates (#820) + +`hadron node update --expected-revision N ` forwards exactly +your positive 32-bit baseline (1..2147483647) to the current kind's update door, +including role clears. Omit it for last-write-wins. The flag alone is not an +edit. Guarded data merges (--data-merge / --data-merge-file), including mixed +field+merge calls, refuse before any request or input read; --data replacement +uses the guarded door. Dry-run previews the supplied expectedRevision (nullable +JSON key) and explicitly says it is not validated by the server. A mismatch +keeps NODE_WRITE_CONFLICT/exit5 and provider currentRevision metadata; re-read +and retry deliberately. Drift can advance a revision without another user's +edit. No automatic refresh/retry or selector-identity guarantee is added. diff --git a/internal/cmd/node/update.go b/internal/cmd/node/update.go index c4df9402..0d6e087a 100644 --- a/internal/cmd/node/update.go +++ b/internal/cmd/node/update.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "io" + "math" "os" "strings" @@ -19,27 +20,28 @@ import ( func newCmdUpdate(f *cmdutil.Factory) *cobra.Command { var ( - memory string - name string - content string - contentFile string - nodeType string - objectType string - role string - description string - abstract string - abstractFile string - data string - dataFile string - dataMerge string - dataMergeFile string - properties string - propertiesFile string - runnable bool - force bool - dryRun bool - reason string - tags []string + memory string + name string + content string + contentFile string + nodeType string + objectType string + role string + description string + abstract string + abstractFile string + data string + dataFile string + dataMerge string + dataMergeFile string + properties string + propertiesFile string + runnable bool + force bool + dryRun bool + expectedRevision int + reason string + tags []string ) cmd := &cobra.Command{ Use: "update | -m ", @@ -63,6 +65,14 @@ The data bag can be written two ways: Replace and merge are different operations, so --data and --data-merge are mutually exclusive. +--expected-revision requires the node's live revision to match your supplied +positive 32-bit baseline. Omitted means last-write-wins. A mismatch is a conflict +(exit 5); re-read and retry deliberately. Drift can advance a revision without +another user edit. The CLI never refreshes the baseline or retries a write. +This guard cannot be used with --data-merge or --data-merge-file; use --data +for a guarded full replacement. In --dry-run it is advisory, not validated by +the server, and reserves no revision. + EDITING EXISTING CONTENT: read it with ` + "`node get --body-only`" + `, which prints the stored body byte for byte (#873). A plain ` + "`node get`" + ` compiles the node's Mustache templates, so writing that text back deletes every {{…}} @@ -126,6 +136,14 @@ schema-governed memory the server validates the result and rejects a violation.) if replaceData && mergeData { return exitcode.Newf(exitcode.Usage, "--data (replace) and --data-merge (merge) are mutually exclusive") } + if changed("expected-revision") { + if expectedRevision < 1 || expectedRevision > math.MaxInt32 { + return exitcode.Newf(exitcode.Usage, "--expected-revision must be between 1 and 2147483647") + } + if mergeData { + return exitcode.Newf(exitcode.Usage, "--expected-revision cannot guard --data-merge or --data-merge-file; use --data to replace the bag through the guarded update") + } + } // The inline/-file pairs are mutually exclusive. Guard on Changed() // (not the resolved value): an explicit --X "" would otherwise slip past // the value check and let the file silently win. @@ -271,6 +289,9 @@ schema-governed memory the server validates the result and rejects a violation.) input := gen.UpdateNodeInput{ Id: &nodeID, } + if changed("expected-revision") { + input.ExpectedRevision = &expectedRevision + } if changed("name") { input.Name = &name } @@ -350,7 +371,7 @@ schema-governed memory the server validates the result and rejects a violation.) } } if dryRun { - return writeUpdatePreview(f, stored, changed, body, contentChanged, warnings) + return writeUpdatePreview(f, stored, changed, body, contentChanged, warnings, input.ExpectedRevision) } for _, w := range warnings { fmt.Fprintf(f.IOStreams.ErrOut, "warning: %s\n", w) @@ -426,6 +447,7 @@ schema-governed memory the server validates the result and rejects a violation.) cmd.Flags().StringVar(&properties, "properties", "", `replace the structured-storage JSON properties (#725; "null" clears)`) cmd.Flags().StringVar(&propertiesFile, "properties-file", "", "read the replacement JSON properties object from a file") cmd.Flags().BoolVar(&runnable, "runnable", false, "mark the node runnable by 'hadron task run' (--runnable=false clears it; omit to preserve)") + cmd.Flags().IntVar(&expectedRevision, "expected-revision", 0, "require the supplied live revision (1-2147483647); omit for last-write-wins; unavailable with data merge") cmd.Flags().StringVar(&reason, "reason", "", "why this change was made (recorded in version history)") cmd.Flags().StringArrayVar(&tags, "tag", nil, "replace tags (repeatable)") return cmd @@ -485,13 +507,14 @@ var updateFields = []struct { // updatePreviewDTO is `node update --dry-run --json` (#873). content is null // unless the body changes; warnings is always a list. type updatePreviewDTO struct { - DryRun bool `json:"dryRun"` - ID string `json:"id"` - URN string `json:"urn"` - Loc string `json:"loc"` - Fields []string `json:"fields"` - Content *contentPreviewDTO `json:"content"` - Warnings []string `json:"warnings"` + DryRun bool `json:"dryRun"` + ID string `json:"id"` + URN string `json:"urn"` + Loc string `json:"loc"` + Fields []string `json:"fields"` + Content *contentPreviewDTO `json:"content"` + Warnings []string `json:"warnings"` + ExpectedRevision *int `json:"expectedRevision"` } // contentPreviewDTO is the body change: stored (raw) → proposed, and their @@ -502,8 +525,11 @@ type contentPreviewDTO struct { Diff string `json:"diff"` } -func writeUpdatePreview(f *cmdutil.Factory, stored *gen.GetNodeNode, changed func(string) bool, body string, contentChanged bool, warnings []string) error { - dto := updatePreviewDTO{DryRun: true, ID: stored.Id, URN: stored.Urn, Loc: stored.Loc, Fields: []string{}, Warnings: warnings} +func writeUpdatePreview(f *cmdutil.Factory, stored *gen.GetNodeNode, changed func(string) bool, body string, contentChanged bool, warnings []string, expectedRevision *int) error { + if expectedRevision != nil { + warnings = append(warnings, fmt.Sprintf("expected revision %d is advisory and not validated by the server in a dry run", *expectedRevision)) + } + dto := updatePreviewDTO{DryRun: true, ID: stored.Id, URN: stored.Urn, Loc: stored.Loc, Fields: []string{}, Warnings: warnings, ExpectedRevision: expectedRevision} for _, uf := range updateFields { for _, fl := range uf.flags { if changed(fl) { diff --git a/internal/cmd/node_expected_revision_820_test.go b/internal/cmd/node_expected_revision_820_test.go index 549391ed..55df7a3e 100644 --- a/internal/cmd/node_expected_revision_820_test.go +++ b/internal/cmd/node_expected_revision_820_test.go @@ -157,6 +157,9 @@ func TestNodeExpectedRevision820EarlyRefusals(t *testing.T) { if code != 2 || len(requests) != 0 || reads != 0 { t.Fatalf("exit%d requests%d stdin%d: %s", code, len(requests), reads, stderr) } + if strings.Contains(strings.Join(args, " "), "--data-merge") && !strings.Contains(stderr, "cannot guard") { + t.Fatalf("merge failed for wrong reason: %s", stderr) + } if len(args) == 2 && !strings.Contains(stderr, "nothing to update") { t.Fatal(stderr) } @@ -224,3 +227,29 @@ func TestNodeExpectedRevision820Conflict(t *testing.T) { }) } } + +func TestNodeExpectedRevision820Replacement(t *testing.T) { + out, stderr, code, requests, _ := revision820Run(t, "", false, "--data", `{"x":1}`, "--expected-revision", "7", "--json") + if code != 0 { + t.Fatalf("%d %s %s", code, out, stderr) + } + mutations := 0 + for _, q := range requests { + if strings.HasPrefix(q.OperationName, "Update") { + mutations++ + if q.OperationName != "UpdateNode" { + t.Fatal(q.OperationName) + } + var input map[string]json.RawMessage + if err := json.Unmarshal(q.Variables["input"], &input); err != nil { + t.Fatal(err) + } + if string(input["expectedRevision"]) != "7" || string(input["data"]) != `{"x":1}` { + t.Fatal(input) + } + } + } + if mutations != 1 { + t.Fatal(mutations) + } +}