diff --git a/docs/plans/coding-command-group.md b/docs/plans/coding-command-group.md index 4ecf1de..ea32c15 100644 --- a/docs/plans/coding-command-group.md +++ b/docs/plans/coding-command-group.md @@ -1,11 +1,12 @@ # Implementation Plan: `hadron coding` — lint the review checklist tree and the preflight router -> **Status: proposed — not yet implemented.** This is a design-ahead doc for -> [#325](https://github.com/hadron-memory/hadron-cli/issues/325), written after -> auditing the live memories. It exists to settle the open questions *before* -> code, because the surface as filed produces a >50% false-positive rate. The -> resolutions in [Decisions](#decisions-resolved-against-live-data) are the part -> that needs review. +> **Status: implemented.** Originally a design-ahead doc for +> [#325](https://github.com/hadron-memory/hadron-cli/issues/325), written to +> settle the open questions *before* code because the surface as filed produces +> a >50% false-positive rate. The [Decisions](#decisions-resolved-against-live-data) +> below are as designed; where building it against the live memories proved a +> decision wrong, [Deviations](#deviations-as-built) records what changed and on +> what evidence. Decision 1 in particular did not survive contact with the data. ## Context @@ -45,7 +46,8 @@ Two further facts shaped the design: - **`preflight` in mmdata has 71 outgoing routes, 24 of them labelled the generic `routes-to`** rather than an action phrase — a third of the router fails the proposed convention check on day one. -- **The `review` tree contains a dangling edge.** An inbound edge points at +- ~~**The `review` tree contains a dangling edge.**~~ **Retracted — see + [Deviations §7](#7-the-dangling-edge-was-mine).** An inbound edge points at `tasks:build-review-coverage`, which is absent from the node listing *and* unreadable (`node get` → exit 4, not found). This is the list-vs-read visibility gap CLAUDE.md warns about, live in the data this command must scan. @@ -120,8 +122,10 @@ mention a `:rel:` loc in the finding message as corroboration only.** ### 3. New check: the edge's far endpoint must resolve — and it isn't the same endpoint on both sides -Not in the issue for `review lint` (only for `preflight lint`), but the live -`review` tree has one (`tasks:build-review-coverage`, above). +Not in the issue for `review lint` (only for `preflight lint`). It was added +because the `review` tree appeared to contain one — a claim +[deviation 7](#7-the-dangling-edge-was-mine) later retracts. The rule is kept +anyway; the endpoint-direction reasoning below is what makes it correct. **The two subcommands look at opposite ends of their edges**, which the rule names must reflect: @@ -144,11 +148,9 @@ names must reflect: The asymmetric severity is deliberate. When the far endpoint can't be read, the linter **cannot evaluate the Decision-1 membership predicate on it** — it may -not be a checklist item at all. The one live instance is a good example: -`tasks:build-review-coverage` sits beside `tasks:review-changes`, which *is* -readable and *is* excluded as a runnable task, so the unreadable node is most -likely a non-member too. Erroring on it would fail a build over a node that -probably shouldn't be linted in the first place. +not be a checklist item at all, so erroring would fail a build over a node that +probably shouldn't be linted in the first place. A route, by contrast, is broken +no matter what sits at the other end. Either way it is **reported as `unavailable`, never silently dropped**, per CLAUDE.md's list-vs-read visibility rule — the finding says the membership is @@ -276,7 +278,7 @@ edges with ids, direction, name, loc — what `edge ls` uses), the node listing destroy sibling edges); `--fix` requires `--yes` non-interactively. - **Read-only live smoke test** against `micromentor.org::mmdata` and `hadronmemory.com::hadron-portal`. On mmdata expect exactly **3 errors and 2 - warnings**: + warnings** *(superseded — see [Verification (as built)](#verification-as-built))*: | Finding | Rule | Severity | |---|---|---| @@ -290,6 +292,138 @@ edges with ids, direction, name, loc — what `edge ls` uses), the node listing draft of this doc omit the dangling endpoint entirely. The portal is expected clean (0 findings across all 25 checks). +## Deviations (as built) + +### 1. Membership is the loc prefix, not the `review` tag + +Decision 1 above says a checklist item is a node **tagged `review`**, not tagged +`meta`, not runnable — validated at the time as 3 findings, 0 false positives. +That validation measured only false *positives*. Building it surfaced two +failures in the other direction, in this order: + +**A tag-only rule silently ignores 35% of the checklist.** 17 of mmdata's 48 +checks carry **no tags at all** — including `review:format-sources`, the +misfiled-toolchain check that motivated this whole command. The original +predicate would never have linted it. + +**Accepting the tag as an *alternative* to the prefix admits findings nodes.** +The obvious repair (`tag OR prefix`) put +`findings:nightly-recommendation-search-idempotency-pre-enqueue-race` — a +resolved incident writeup carrying `review` among six tags — into the member set +and reported it as missing a parent edge. A false positive on the first live run. + +Checking both memories settled it: every node tagged `review` outside the +`review:` prefix is a finding, and every real check is inside it. + +| Memory | checks under `review:` | `review`-tagged outside it | +|---|---|---| +| `micromentor.org::mmdata` | 49 | 1 — a findings node | +| `hadronmemory.com::hadron-portal` | 24 | 1 — a findings node | + +**As built: a checklist item is a node under the lint root's child prefix, +neither tagged `meta` nor runnable.** The tag contributes nothing but noise and +was dropped. The prefix is derived from `--root`, so pointing the command at a +differently-named parent moves the membership rule with it. `review:backlog` +still shares the prefix, so the `meta` disqualifier is what keeps it out. + +### 2. `description-has-trigger` → `description-present` + +Real descriptions state the *rule* ("Resolver field must be thin …"), not the +condition, so requiring an "Applies when" in the description would have flagged +almost every check. Mechanical detection stops at "the description is empty", +which is the strictly-worse case and unambiguous. Requiring a description to +*contain* a trigger needs judgement, not a linter. + +### 3. `duplicate-trigger` only considers well-formed triggers + +mmdata has two checks both labelled `child-of`. Counting them as a duplicate +trigger would report one defect twice — once as `label-is-condition`, once as +`duplicate-trigger`. Only labels that pass the condition check take part. + +### 4. `foreign-toolchain` infers over the whole member corpus, and stays silent when unsure + +Inferring the memory's toolchain from triggers alone ties 1–1 on mmdata (one +Dart, one TypeScript), so the rule would have missed `format-sources` — the case +it exists for. It now infers over each member's loc, name, description and +trigger together, which resolves mmdata to `ts` and flags the Dart trigger. When +no family wins outright the rule emits nothing rather than guessing, and +`--toolchain ` overrides it (`--toolchain -` disables). + +### 5. A `--root` flag, not a hardcoded loc + +Both linters take `--root` (default `review` / `preflight`) so a memory that +names its parent differently is still lintable, and — per deviation 1 — the +membership prefix follows it. + +### 6. Bug found while building: memory ref vs node URN + +`cmdutil.CanonicalMemoryRef` emits the flat `hrn:mem::` form, so the +plan's implied `memURN + "::" + loc` would have produced node refs that resolve +to nothing. Node refs are composed with `cmdutil.NodeURN` instead, and the +package keeps both spellings explicitly (`codingMemory.Ref` for filters, the raw +pair for URNs). Unavailable refs are mapped back to bare locs through a +ref→loc map rather than by trimming a prefix, which would have depended on the +URN spelling. A command test covers it. + +### 7. The "dangling edge" was mine + +The evidence section above cites `tasks:build-review-coverage` — an inbound edge +whose endpoint was "absent from the node listing *and* unreadable" — as a live +instance of CLAUDE.md's list-vs-read visibility gap, and Decision 3 leans on it. + +**It was never dangling.** The node is fully readable; it is simply not +addressable by a URN rebuilt from its loc: + +``` +$ hadron api 'query($r:[ID!]){ nodeBatch(refs:$r){ unavailable nodes{ id loc tags } } }' \ + -F r='["019eedd0a981749fa5eff355efa2bf47"]' + unavailable: [] + loc: tasks:build-review-coverage tags: [meta contributing review coverage …] +``` + +Reading endpoints by id (deviation 6 above, from review) made the finding +disappear: the node resolves, is tagged `meta`, sits outside the `review:` +prefix, and is correctly excluded as a non-check. The original "unreadable" +verdict was produced by the URN-rebuilding bug, not by the data. + +`check-node-resolves` is kept regardless — a genuinely unreadable endpoint is +possible and the visibility gap is real — but it is now a rule with no known +live instance rather than one with a motivating example. Nothing in either +memory currently trips it. + +## Verification (as built) + +`go test ./...` (16 packages) and `make lint` (0 issues) are green. Read-only +live runs — no writes, `--fix` not exercised against a live memory: + +``` +$ hadron coding review lint -m micromentor.org::mmdata → exit 5 + 3 errors, 5 warnings +$ hadron coding review lint -m hadronmemory.com::hadron-portal → exit 0 + ✓ 24 check(s) OK +$ hadron coding preflight lint -m micromentor.org::mmdata → exit 0 + 25 warnings (all route-label-phrasing), 0 errors +``` + +The mmdata findings, all true positives: + +| Finding | Rule | Severity | +|---|---|---| +| `review:input-type-graphql-type` | `label-present` | error | +| `review:posthog-backend-vs-app-event-routing` | `label-is-condition` | error | +| `review:role-vs-group-ident-vocabulary` | `label-is-condition` | error | +| `review:format-sources` | `foreign-toolchain` | warning | +| `review:matching-engine-rule-contract` / `-defaults` | `duplicate-trigger` | warning ×2 | +| `review:get-core-service-throws` / `review:provider-calls-behind-seam` | `seq-unique` | warning ×2 | + +This replaces the smoke table above, which predicted 3 errors + 2 warnings. The +extra warnings are the `duplicate-trigger` and `seq-unique` rules, which the +plan specified but never measured — both fire, both on real defects. The +predicted `check-node-resolves` warning is absent for the reason in deviation 7: +that endpoint was never unreadable. The +preflight run exits 0 with 25 warnings, which is Decision 4 working as intended: +a third of the router failing a convention does not turn the build red. + ## Out of scope (follow-ups) - `coding review ls|add`, `coding preflight ls` — the surface leaves room. diff --git a/internal/cmd/agentic/agentic-usage.md b/internal/cmd/agentic/agentic-usage.md index 957cf70..13a0b87 100644 --- a/internal/cmd/agentic/agentic-usage.md +++ b/internal/cmd/agentic/agentic-usage.md @@ -75,6 +75,7 @@ hadron search [-m ]... [--mode hybrid|keyword|vector|regex] [--p hadron replace text --field (--node | -m ) [--prefix ] [--regex] [-i] [--dry-run] [--yes] [--max-nodes N] hadron edge list | add | update | rm hadron spec list [-m ] | get |--prefix | describe | use [] | register [--check] | find [--match-exactly] | grep [--regex] [-i] [--field content|abstract] [--prefix ] | replace [--regex] [--word-boundary=false] [--field content|abstract] [--dry-run] [--yes] [--max-specs N] | new ... | edit | extract --to-feature | link | lint [] | check-tools [--prefix ] | supersede | import spec-kit|code +hadron coding review lint -m [--root ] [--toolchain |-] [--strict] [--fix [--yes]] [--json] | preflight lint -m [--root ] [--strict] [--json] hadron app list --org | install (--org | --owner-me) --agent --name [--type ] [--urn ] [--description ] | uninstall | use hadron ai-config list [--app ] [--agent ] | create (--app|--agent|--org ) --name --provider

--model [--api-key -] [--file ] | update ... | rm hadron org list [--mine] | create --name --urn | get | public | update | rm | member list|add|set-role|rm --user [--role ] | invite create --org --role | invite accept | invite show diff --git a/internal/cmd/coding/coding.go b/internal/cmd/coding/coding.go new file mode 100644 index 0000000..6f37ed7 --- /dev/null +++ b/internal/cmd/coding/coding.go @@ -0,0 +1,296 @@ +// Package coding lints the coding-workflow graph: the review:* checklist tree +// and the preflight router. Both are executable infrastructure — a malformed +// edge label makes a check or a route silently stop firing — so the defects are +// mechanically detectable and worth a linter. See +// docs/plans/coding-command-group.md. +package coding + +import ( + "context" + "sort" + "strconv" + "strings" + + "github.com/Khan/genqlient/graphql" + "github.com/spf13/cobra" + + "github.com/hadron-memory/hadron-cli/internal/api" + "github.com/hadron-memory/hadron-cli/internal/api/gen" + "github.com/hadron-memory/hadron-cli/internal/cmdutil" + "github.com/hadron-memory/hadron-cli/internal/exitcode" +) + +// Severity levels, spelled as `spec lint` spells them so the two linters' +// --json output is interchangeable. +const ( + sevError = "error" + sevWarning = "warning" +) + +// Loc of the two roots this group lints. Both are conventional names in a +// coding-workflow memory, overridable per command with --root. +const ( + reviewRootLoc = "review" + preflightRootLoc = "preflight" +) + +// findingDTO is the --json contract, mirroring spec's lintFindingDTO (node loc, +// rule, severity, message) so agents can treat both linters the same. +type findingDTO struct { + Node string `json:"node"` + Rule string `json:"rule"` + Severity string `json:"severity"` // error | warning + Message string `json:"message"` +} + +// checkNode is the lint-friendly projection of a node — decoupled from the +// genqlient types so the rule engines unit-test without a server. +type checkNode struct { + Loc string + Name string + Description string + Tags []string + Seq *int + IsRunnable bool +} + +// graphEdge is one edge incident to a lint root. Other* describe the far +// endpoint — the edge's source for the review tree's incoming edges, its target +// for preflight's outgoing routes (see docs/plans/coding-command-group.md, +// Decision 3: the two subcommands read opposite ends). +// +// OtherID is the endpoint's node id, which is what the far node is read by: a +// route may legitimately cross into another memory, so rebuilding a URN from +// the root's memory would look the wrong node up (or miss it entirely). +// OtherLoc is empty when the server redacted the endpoint projection — an +// unreadable endpoint the linter must report, not skip. +type graphEdge struct { + ID string + Label string + Loc string // the edge's own (usually name-derived) loc + OtherID string + Other string // the far endpoint's loc; "" when the projection was redacted + MemoryID string // the far endpoint's memory id, for the moved-memory route check +} + +// endpointName identifies an edge in a finding when its far endpoint has no +// loc to name it by. +func (e graphEdge) endpointName() string { + if e.Other != "" { + return e.Other + } + if e.Label != "" { + return "(unreadable target of " + strconv.Quote(e.Label) + ")" + } + return "(unreadable target of edge " + e.ID + ")" +} + +// NewCmdCoding builds the `hadron coding` group. +func NewCmdCoding(f *cmdutil.Factory) *cobra.Command { + cmd := &cobra.Command{ + Use: "coding ", + Short: "Lint the coding-workflow graph (review checklist, preflight router)", + Long: `Lint the coding-workflow graph in a Hadron memory. + +The review:* checklist tree and the preflight router are executable +infrastructure, not prose: ` + "`tasks:review-changes`" + ` triages checks by +reading each one's "Applies when …" edge label back to the review parent, +and preflight routes symptom → finding along its outgoing edges. A +malformed edge label makes the check or route silently stop firing — the +node still exists and never matches again. + +These commands detect that mechanically. Errors exit 5; --strict promotes +warnings to errors. Every subcommand takes -m/--memory.`, + } + cmd.AddCommand(newCmdReview(f)) + cmd.AddCommand(newCmdPreflight(f)) + return cmd +} + +func newCmdReview(f *cmdutil.Factory) *cobra.Command { + cmd := &cobra.Command{ + Use: "review ", + Short: "Work with the review:* checklist tree", + } + cmd.AddCommand(newCmdReviewLint(f)) + return cmd +} + +func newCmdPreflight(f *cmdutil.Factory) *cobra.Command { + cmd := &cobra.Command{ + Use: "preflight ", + Short: "Work with the preflight router", + } + cmd.AddCommand(newCmdPreflightLint(f)) + return cmd +} + +// codingMemory holds the two spellings of the target memory this package needs: +// the canonical ref the server's memory(ref:) dispatch wants, and the raw +// :: pair that node URNs are composed from. They are NOT +// interchangeable — CanonicalMemoryRef emits the flat hrn:mem:: +// form, so pasting a loc onto it yields a ref that resolves to nothing. +type codingMemory struct { + Ref string // for filters and memory(ref:) + raw string // as the user spelled it, for cmdutil.NodeURN +} + +func codingMemoryURN(memory string) (codingMemory, error) { + if strings.TrimSpace(memory) == "" { + return codingMemory{}, exitcode.Newf(exitcode.Usage, "-m/--memory is required (org::memory)") + } + return codingMemory{Ref: cmdutil.CanonicalMemoryRef(memory), raw: strings.TrimSpace(memory)}, nil +} + +// nodeRef composes the fully-qualified node URN for a bare loc in this memory. +func (m codingMemory) nodeRef(loc string) (string, error) { + if u := cmdutil.NodeURN(m.raw, loc); u != "" { + return u, nil + } + return "", exitcode.Newf(exitcode.Usage, + "-m/--memory %q must be an :: pair to address nodes by bare loc", m.raw) +} + +// fetchRootEdges reads a lint root and projects the edges on the requested +// side. incoming=true reads incomingEdges (whose far endpoint is `source`), +// incoming=false reads outgoingEdges (far endpoint `target`). +// It also returns the root's own memory id, which is the only value comparable +// against an endpoint's MemoryId — both come from the same projection, whereas +// the -m flag's canonical ref is a URN and would never match. +func fetchRootEdges(ctx context.Context, client graphql.Client, mem codingMemory, rootLoc string, incoming bool) ([]graphEdge, string, error) { + ref, err := mem.nodeRef(rootLoc) + if err != nil { + return nil, "", err + } + resp, err := gen.GetNode(ctx, client, ref) + if err != nil { + return nil, "", api.MapError(err) + } + if resp.Node == nil { + return nil, "", exitcode.Newf(exitcode.NotFound, + "no %q node in %s — nothing to lint", rootLoc, mem.raw) + } + rootMemoryID := resp.Node.MemoryId + var out []graphEdge + if incoming { + for _, e := range resp.Node.IncomingEdges { + if e == nil { + continue + } + ge := graphEdge{ID: e.Id, Loc: e.Loc} + if e.Name != nil { + ge.Label = *e.Name + } + if e.Source != nil { + ge.OtherID, ge.Other, ge.MemoryID = e.Source.Id, e.Source.Loc, e.Source.MemoryId + } + out = append(out, ge) + } + return out, rootMemoryID, nil + } + for _, e := range resp.Node.OutgoingEdges { + if e == nil { + continue + } + ge := graphEdge{ID: e.Id, Loc: e.Loc} + if e.Name != nil { + ge.Label = *e.Name + } + if e.Target != nil { + ge.OtherID, ge.Other, ge.MemoryID = e.Target.Id, e.Target.Loc, e.Target.MemoryId + } + out = append(out, ge) + } + return out, rootMemoryID, nil +} + +// fetchNodes bulk-reads locs into the lint model. The second return is the refs +// the server would not hand back: a node can list but be unreadable, and +// CLAUDE.md requires those be surfaced rather than silently dropped. +// Nodes are addressed by **id**, never by a URN rebuilt from the root's memory: +// an edge may legitimately cross into another memory, and rebuilding the ref +// would then look up the wrong memory — reporting a live node as unresolvable, +// or silently linting a same-loc node from the home memory instead. +func fetchNodes(ctx context.Context, client graphql.Client, byID map[string]string) (map[string]checkNode, []string, error) { + if len(byID) == 0 { + return map[string]checkNode{}, nil, nil + } + // Keep ref → loc, so an unavailable ref maps back to the loc every other + // row is keyed by. + locByRef := make(map[string]string, len(byID)) + refs := make([]string, 0, len(byID)) + for id, loc := range byID { + locByRef[id] = loc + refs = append(refs, id) + } + sort.Strings(refs) // deterministic batching + nodes, unavailable, err := api.CollectNodeBatch(refs, func(chunk []string) (*gen.NodeBatchNodeBatchNodeBatchResult, error) { + resp, ferr := gen.NodeBatch(ctx, client, chunk, nil, nil) + if ferr != nil { + return nil, api.MapError(ferr) + } + return resp.NodeBatch, nil + }) + if err != nil { + return nil, nil, err + } + out := make(map[string]checkNode, len(nodes)) + for _, n := range nodes { + if n == nil { + continue + } + cn := checkNode{Loc: n.Loc, Name: n.Name, Tags: n.Tags, Seq: n.Seq} + if n.Description != nil { + cn.Description = *n.Description + } + if n.IsRunnable != nil { + cn.IsRunnable = *n.IsRunnable + } + out[n.Loc] = cn + } + // Report unavailable refs by loc, not by the fully-qualified ref, so the + // finding lines up with every other row in the table. + bare := make([]string, 0, len(unavailable)) + for _, u := range unavailable { + if loc, ok := locByRef[u]; ok { + bare = append(bare, loc) + continue + } + bare = append(bare, u) // unrecognised spelling — report it verbatim + } + return out, bare, nil +} + +// scanPrefix pages every node under a loc prefix to exhaustion. An unbounded +// query returns one page and silently drops the rest (#23), so the sweep must +// paginate. +// +// Scoped by prefix rather than listing the whole memory: membership is decided +// by the lint root's child prefix, so anything outside it can never be a +// checklist item and need not be fetched. +func scanPrefix(ctx context.Context, client graphql.Client, mem codingMemory, locPrefix string) ([]*api.ListNode, error) { + const pageSize = 200 + var all []*api.ListNode + for offset := 0; ; offset += pageSize { + limit, off, pfx := pageSize, offset, locPrefix + page, err := api.FindNodes(ctx, client, nil, nil, + &gen.NodeFilter{MemoryIds: []string{mem.Ref}, LocPrefix: &pfx}, nil, nil, &limit, &off) + if err != nil { + return nil, api.MapError(err) + } + all = append(all, page.Nodes...) + if len(page.Nodes) < pageSize { + return all, nil + } + } +} + +// hasTag reports whether tags contains want, case-insensitively. +func hasTag(tags []string, want string) bool { + for _, t := range tags { + if strings.EqualFold(strings.TrimSpace(t), want) { + return true + } + } + return false +} diff --git a/internal/cmd/coding/fix.go b/internal/cmd/coding/fix.go new file mode 100644 index 0000000..943a475 --- /dev/null +++ b/internal/cmd/coding/fix.go @@ -0,0 +1,107 @@ +package coding + +import ( + "context" + "fmt" + "sort" + "strings" + + "github.com/Khan/genqlient/graphql" + + "github.com/hadron-memory/hadron-cli/internal/api" + "github.com/hadron-memory/hadron-cli/internal/api/gen" + "github.com/hadron-memory/hadron-cli/internal/cmdutil" +) + +// plannedFix is one edge relabel: promote a check's description into an edge +// label that carries no condition. +type plannedFix struct { + Loc string + EdgeID string + OldLabel string + NewLabel string +} + +// planReviewFix selects the mechanical subset of findings that can be repaired +// without a human: an empty or non-condition label on a check whose description +// already states the trigger. Anything else — a broken label on a node whose +// description has no condition either — needs a person and is left alone. +func planReviewFix(in reviewInput, findings []findingDTO) []plannedFix { + broken := map[string]bool{} + for _, f := range findings { + if f.Rule == "label-present" || f.Rule == "label-is-condition" { + broken[f.Node] = true + } + } + var out []plannedFix + for loc := range broken { + e, ok := in.Edges[loc] + if !ok { + continue + } + trigger := triggerFromDescription(in.Members[loc].Description) + if trigger == "" { + continue + } + out = append(out, plannedFix{Loc: loc, EdgeID: e.ID, OldLabel: e.Label, NewLabel: trigger}) + } + sort.Slice(out, func(i, j int) bool { return out[i].Loc < out[j].Loc }) + return out +} + +// triggerFromDescription extracts an "Applies when …" sentence from a +// description, or "" when it carries no condition. Deliberately conservative: +// it only promotes text that is already spelled as a trigger, so --fix never +// invents a condition. +func triggerFromDescription(desc string) string { + d := strings.TrimSpace(desc) + if d == "" { + return "" + } + lower := strings.ToLower(d) + i := strings.Index(lower, triggerStem) + if i < 0 { + return "" + } + rest := d[i:] + // Stop at the first sentence end so a long description contributes only its + // trigger clause. + if j := strings.IndexAny(rest, ".\n"); j > 0 { + rest = rest[:j] + } + rest = strings.TrimSpace(rest) + if len(strings.TrimSpace(rest[len(triggerStem):])) == 0 { + return "" // the stem with nothing after it is not a condition either + } + return rest +} + +// applyReviewFix writes the planned relabels and returns how many landed. +// +// Each write is a single-edge updateEdge. It must never go through +// updateNode(edges:), which REPLACES a node's whole outgoing edge set and would +// destroy the check's sibling documented-by / relates-to edges — the hazard +// that makes the MCP surface unsafe for this repair (issue #325). +func applyReviewFix(ctx context.Context, client graphql.Client, f *cmdutil.Factory, in reviewInput, findings []findingDTO, yes bool) (int, error) { + plan := planReviewFix(in, findings) + if len(plan) == 0 { + fmt.Fprintln(f.IOStreams.ErrOut, "--fix: nothing mechanically fixable (a broken label needs a description that already states its trigger)") + return 0, nil + } + for _, p := range plan { + fmt.Fprintf(f.IOStreams.ErrOut, " %s: %q → %q\n", p.Loc, p.OldLabel, p.NewLabel) + } + if err := cmdutil.Confirm(f.IOStreams, yes, + fmt.Sprintf("Relabel %d edge(s)?", len(plan))); err != nil { + return 0, err + } + applied := 0 + for _, p := range plan { + label := p.NewLabel + if _, err := gen.UpdateEdge(ctx, client, p.EdgeID, &label, nil, nil, nil, nil, nil, nil); err != nil { + return applied, fmt.Errorf("relabelling %s: %w", p.Loc, api.MapError(err)) + } + applied++ + } + return applied, nil +} diff --git a/internal/cmd/coding/fix_test.go b/internal/cmd/coding/fix_test.go new file mode 100644 index 0000000..420cbc5 --- /dev/null +++ b/internal/cmd/coding/fix_test.go @@ -0,0 +1,92 @@ +package coding + +import "testing" + +func TestTriggerFromDescription(t *testing.T) { + cases := []struct{ desc, want string }{ + {"Applies when a resolver changes. Then check X.", "Applies when a resolver changes"}, + {"Verifies the thing. Applies when a schema changes.", "Applies when a schema changes"}, + {"applies when the model moves", "applies when the model moves"}, + {"Applies when a rule changes\nmore prose", "Applies when a rule changes"}, + // Nothing to promote — --fix must never invent a condition. + {"Verifies a bg-code-gen Input modelDef sets graphqlType.", ""}, + {"", ""}, + {" ", ""}, + {"Applies when", ""}, // bare stem is not a condition either + {"Applies when .", ""}, + } + for _, tc := range cases { + if got := triggerFromDescription(tc.desc); got != tc.want { + t.Errorf("triggerFromDescription(%q) = %q, want %q", tc.desc, got, tc.want) + } + } +} + +func TestPlanReviewFix(t *testing.T) { + in := reviewInput{ + Members: map[string]checkNode{ + // Fixable: broken label, description states the trigger. + "review:a": {Loc: "review:a", Tags: []string{"review"}, Description: "Applies when a resolver changes. More."}, + // Not fixable: description has no condition, needs a human. + "review:b": {Loc: "review:b", Tags: []string{"review"}, Description: "Verifies the codegen output."}, + // Healthy: must never be touched. + "review:c": {Loc: "review:c", Tags: []string{"review"}, Description: "Applies when c changes."}, + }, + Edges: map[string]graphEdge{ + "review:a": edge("review:a", "child-of"), + "review:b": edge("review:b", ""), + "review:c": edge("review:c", "Applies when c changes"), + }, + Toolchain: "-", + } + plan := planReviewFix(in, lintReview(in)) + if len(plan) != 1 { + t.Fatalf("expected exactly one fixable edge, got %+v", plan) + } + if plan[0].Loc != "review:a" { + t.Errorf("wrong node planned: %+v", plan[0]) + } + if plan[0].NewLabel != "Applies when a resolver changes" { + t.Errorf("unexpected new label %q", plan[0].NewLabel) + } + if plan[0].EdgeID == "" { + t.Error("plan must carry the edge id — the fix is a single-edge updateEdge") + } +} + +// A check with no edge at all has nothing to relabel; --fix must not try to +// invent one (creating an edge is a different, non-mechanical decision). +func TestPlanReviewFixSkipsMissingEdge(t *testing.T) { + in := reviewInput{ + Members: map[string]checkNode{"review:orphan": {Loc: "review:orphan", Tags: []string{"review"}, Description: "Applies when x changes."}}, + Edges: map[string]graphEdge{}, + Toolchain: "-", + } + if plan := planReviewFix(in, lintReview(in)); len(plan) != 0 { + t.Errorf("expected no plan for a check with no edge, got %+v", plan) + } +} + +func TestPlanReviewFixIsDeterministic(t *testing.T) { + mk := func(loc string) checkNode { + return checkNode{Loc: loc, Tags: []string{"review"}, Description: "Applies when " + loc + " changes."} + } + in := reviewInput{ + Members: map[string]checkNode{"review:c": mk("review:c"), "review:a": mk("review:a"), "review:b": mk("review:b")}, + Edges: map[string]graphEdge{ + "review:a": edge("review:a", "child-of"), + "review:b": edge("review:b", "child-of"), + "review:c": edge("review:c", "child-of"), + }, + Toolchain: "-", + } + for i := 0; i < 5; i++ { + plan := planReviewFix(in, lintReview(in)) + if len(plan) != 3 { + t.Fatalf("expected 3 fixes, got %d", len(plan)) + } + if plan[0].Loc != "review:a" || plan[1].Loc != "review:b" || plan[2].Loc != "review:c" { + t.Fatalf("plan order is unstable: %+v", plan) + } + } +} diff --git a/internal/cmd/coding/membership.go b/internal/cmd/coding/membership.go new file mode 100644 index 0000000..a7bc24d --- /dev/null +++ b/internal/cmd/coding/membership.go @@ -0,0 +1,64 @@ +package coding + +import "strings" + +// Membership — which nodes hanging off the review parent are actually checklist +// items. +// +// This is the load-bearing predicate of `coding review lint`. The review parent +// legitimately has non-checklist neighbours: the preflight router, the tasks +// that consume the tree, a meta backlog, pattern nodes, and findings. Linting +// every inbound edge (the obvious reading) produced 9 findings on +// micromentor.org::mmdata of which 5 were false positives — a linter that cries +// wolf on more than half its output does not get used. +// +// The rule is the child-loc prefix of the tree being linted, minus two +// disqualifiers: +// +// review:thin-resolver-field tags=[review …] isRunnable=false → check +// review:format-sources tags=[] isRunnable=false → check +// review:backlog tags=[review meta …] isRunnable=false → meta, excluded +// tasks:review-changes tags=[] isRunnable=true → task, excluded +// patterns:function-signatures tags=[conventions …] isRunnable=false → pattern, excluded +// findings:nightly-…-race tags=[… review …] isRunnable=false → finding, excluded +// +// Two signals were considered and rejected on evidence: +// +// - A `review` TAG requirement excludes 17 of mmdata's 48 checks, which carry +// no tags at all — including `review:format-sources`, the misfiled-toolchain +// example that motivated this command. A tag-only rule silently ignored 35% +// of the checklist: a false negative that counting false positives alone +// would never have surfaced. +// - Accepting the `review` tag as an ALTERNATIVE to the prefix admits +// findings nodes, which carry the tag because they are review-relevant. +// Across micromentor.org::mmdata and hadronmemory.com::hadron-portal, every +// node tagged `review` outside the prefix is a finding and every real check +// is inside it, so the tag contributes nothing but noise. +// +// Note that `review:backlog` shares the prefix, so the `meta` disqualifier is +// what keeps it out. +// +// See docs/plans/coding-command-group.md, Decision 1. +const metaTag = "meta" + +// childPrefix is the loc prefix a lint root's children carry: `review` → `review:`. +func childPrefix(rootLoc string) string { return rootLoc + ":" } + +// isChecklistItem reports whether n is a check under the given root. +func isChecklistItem(rootLoc string, n checkNode) bool { + return isChecklistItemListing(rootLoc, n.Loc, n.Tags, &n.IsRunnable) +} + +// isChecklistItemListing is the same predicate over the cheaper listing +// projection, so the memory sweep can filter before the bulk read. A nil +// isRunnable means unset, which most nodes leave that way and which does not +// disqualify. +func isChecklistItemListing(rootLoc, loc string, tags []string, isRunnable *bool) bool { + if isRunnable != nil && *isRunnable { + return false + } + if hasTag(tags, metaTag) { + return false + } + return strings.HasPrefix(loc, childPrefix(rootLoc)) +} diff --git a/internal/cmd/coding/membership_test.go b/internal/cmd/coding/membership_test.go new file mode 100644 index 0000000..9ac2c73 --- /dev/null +++ b/internal/cmd/coding/membership_test.go @@ -0,0 +1,62 @@ +package coding + +import "testing" + +func ptrBool(b bool) *bool { return &b } + +// The five node shapes the predicate has to separate, taken from +// micromentor.org::mmdata. See docs/plans/coding-command-group.md, Decision 1. +func TestIsChecklistItem(t *testing.T) { + cases := []struct { + name string + node checkNode + want bool + }{ + {"tagged review", checkNode{Loc: "review:thin-resolver-field", Tags: []string{"review", "graphql"}}, true}, + // 17 of mmdata's 48 checks carry no tags at all; a tag-only rule + // silently ignored 35% of the checklist, including the misfiled + // format-sources that motivated the command. + {"untagged but under review:", checkNode{Loc: "review:format-sources"}, true}, + // review:backlog shares the review: prefix, so only the meta tag keeps + // it out — the case that makes a prefix-only rule wrong. + {"meta backlog", checkNode{Loc: "review:backlog", Tags: []string{"review", "meta"}}, false}, + {"runnable task", checkNode{Loc: "tasks:review-changes", IsRunnable: true}, false}, + {"pattern node", checkNode{Loc: "patterns:function-signatures", Tags: []string{"conventions"}}, false}, + {"router", checkNode{Loc: "preflight"}, false}, + {"runnable even under review:", checkNode{Loc: "review:something", IsRunnable: true}, false}, + // A findings node carries the `review` tag because it is + // review-relevant, not because it is a check. Both mmdata and + // hadron-portal have exactly one, and treating the tag as an + // alternative to the prefix reported both as missing a parent edge. + {"tagged finding is not a check", checkNode{Loc: "findings:some-race", Tags: []string{"review", "gotcha"}}, false}, + } + for _, tc := range cases { + if got := isChecklistItem("review", tc.node); got != tc.want { + t.Errorf("%s: isChecklistItem(%q, tags=%v, runnable=%v) = %v, want %v", + tc.name, tc.node.Loc, tc.node.Tags, tc.node.IsRunnable, got, tc.want) + } + } +} + +// The listing projection must agree with the full-node predicate, or the +// pre-filter would drop nodes the engine would have kept. +func TestListingPredicateAgrees(t *testing.T) { + cases := []checkNode{ + {Loc: "review:a", Tags: []string{"review"}}, + {Loc: "review:b"}, + {Loc: "review:backlog", Tags: []string{"review", "meta"}}, + {Loc: "tasks:t", IsRunnable: true}, + {Loc: "patterns:p", Tags: []string{"conventions"}}, + } + for _, n := range cases { + full := isChecklistItem("review", n) + listing := isChecklistItemListing("review", n.Loc, n.Tags, ptrBool(n.IsRunnable)) + if full != listing { + t.Errorf("%s: full=%v listing=%v — predicates disagree", n.Loc, full, listing) + } + } + // A nil isRunnable means unset, which most nodes leave that way. + if !isChecklistItemListing("review", "review:x", nil, nil) { + t.Error("nil isRunnable should not disqualify") + } +} diff --git a/internal/cmd/coding/preflight_lint.go b/internal/cmd/coding/preflight_lint.go new file mode 100644 index 0000000..ffc6572 --- /dev/null +++ b/internal/cmd/coding/preflight_lint.go @@ -0,0 +1,209 @@ +package coding + +import ( + "fmt" + "io" + "sort" + "strings" + + "github.com/spf13/cobra" + + "github.com/hadron-memory/hadron-cli/internal/cmdutil" + "github.com/hadron-memory/hadron-cli/internal/exitcode" + "github.com/hadron-memory/hadron-cli/internal/output" +) + +// routePrefix is preflight's labelling convention: routes read as an action, +// "to do X …", so the router scans like a table of contents. +const routePrefix = "to " + +// retiredTags mark a node that should no longer be routed to. +var retiredTags = []string{"retired", "superseded", "tombstone", "deprecated"} + +// preflightInput is everything the preflight rule engine needs. +type preflightInput struct { + Routes []graphEdge + Targets map[string]checkNode // resolved route targets, by loc + Unavailable []string // route targets that could not be read + HomeMemory string // the preflight node's own memory id +} + +func newCmdPreflightLint(f *cmdutil.Factory) *cobra.Command { + var memory, root string + var strict bool + cmd := &cobra.Command{ + Use: "lint", + Aliases: []string{"check", "validate"}, + Short: "Validate the preflight router's outgoing routes", + Long: `Validate every route out of the preflight node. + +preflight routes symptom → finding along its outgoing edges, so a route to +a node that no longer resolves sends a reader nowhere — stale routing is +worse than missing routing. That is the one error here; the labelling and +lifecycle conventions are warnings. + +Errors exit 5; --strict promotes warnings to errors too.`, + Example: ` hadron coding preflight lint -m micromentor.org::mmdata + hadron coding preflight lint -m hadronmemory.com::dev --json --strict`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + mem, err := codingMemoryURN(memory) + if err != nil { + return err + } + client, err := f.GraphQLClient() + if err != nil { + return err + } + ctx := cmd.Context() + + // Outgoing, so the far endpoint is the edge's target — the opposite + // end from the review tree's inbound edges. The root's own memory + // id comes back too: it is the only value comparable against an + // endpoint's MemoryId. + routes, homeMemory, err := fetchRootEdges(ctx, client, mem, root, false) + if err != nil { + return err + } + // Read targets by id, so a route that legitimately crosses into + // another memory resolves there instead of being looked up in this + // one. + byID := map[string]string{} + for _, r := range routes { + if r.OtherID != "" { + byID[r.OtherID] = r.Other + } + } + targets, unavailable, err := fetchNodes(ctx, client, byID) + if err != nil { + return err + } + + findings := lintPreflight(preflightInput{ + Routes: routes, Targets: targets, Unavailable: unavailable, HomeMemory: homeMemory, + }) + if strict { + for i := range findings { + if findings[i].Severity == sevWarning { + findings[i].Severity = sevError + } + } + } + hasError := false + for _, fd := range findings { + if fd.Severity == sevError { + hasError = true + break + } + } + + if err := output.Write(f.IOStreams, f.JSON, findings, func(w io.Writer) error { + if len(findings) == 0 { + fmt.Fprintf(w, "✓ %d route(s) OK\n", len(routes)) + return nil + } + t := output.NewTable(w, "ROUTE", "SEVERITY", "RULE", "MESSAGE") + for _, fd := range findings { + t.Row(fd.Node, fd.Severity, fd.Rule, fd.Message) + } + return t.Flush() + }); err != nil { + return err + } + if hasError { + return exitcode.Silent(exitcode.Conflict) + } + return nil + }, + } + cmd.Flags().StringVarP(&memory, "memory", "m", "", "memory to lint (org::memory)") + cmd.Flags().StringVar(&root, "root", preflightRootLoc, "loc of the preflight router node") + cmd.Flags().BoolVar(&strict, "strict", false, "treat warnings as errors") + return cmd +} + +// lintPreflight is the pure rule engine over the router's outgoing routes. +func lintPreflight(in preflightInput) []findingDTO { + out := []findingDTO{} + unavailable := map[string]bool{} + for _, u := range in.Unavailable { + unavailable[u] = true + } + + routes := append([]graphEdge(nil), in.Routes...) + sort.SliceStable(routes, func(i, j int) bool { return routes[i].Other < routes[j].Other }) + + reported := map[string]bool{} + for _, r := range routes { + // A redacted endpoint projection leaves no loc at all. Skipping it + // would make the command's only error rule blind to exactly the + // unreadable-target case it exists for, and let it report a clean + // router. + if r.Other == "" { + name := r.endpointName() + if reported[name] { + continue + } + reported[name] = true + out = append(out, findingDTO{name, "route-target-resolves", sevError, + "route target could not be read — the route sends a reader nowhere"}) + continue + } + if unavailable[r.Other] { + if reported[r.Other] { + continue + } + reported[r.Other] = true + out = append(out, findingDTO{r.Other, "route-target-resolves", sevError, + "route target could not be read — the route sends a reader nowhere"}) + continue // no point checking conventions on a node we can't see + } + label := strings.TrimSpace(r.Label) + switch { + case label == "": + out = append(out, findingDTO{r.Other, "route-label-phrasing", sevWarning, + "route has no label — preflight scans by label, so this route is unreadable"}) + case !strings.HasPrefix(strings.ToLower(label), routePrefix): + out = append(out, findingDTO{r.Other, "route-label-phrasing", sevWarning, + fmt.Sprintf("route label %q is not action-phrased — expected it to start %q", label, strings.TrimSpace(routePrefix))}) + } + t, ok := in.Targets[r.Other] + if !ok { + continue + } + for _, rt := range retiredTags { + if hasTag(t.Tags, rt) { + out = append(out, findingDTO{r.Other, "route-target-retired", sevWarning, + fmt.Sprintf("route target is tagged %q — retired nodes should not be routed to", rt)}) + break + } + } + if in.HomeMemory != "" && r.MemoryID != "" && !sameMemory(r.MemoryID, in.HomeMemory) { + out = append(out, findingDTO{r.Other, "route-target-moved-memory", sevWarning, + fmt.Sprintf("route target lives in %s, not the router's own memory", r.MemoryID)}) + } + } + + sort.SliceStable(out, func(i, j int) bool { + if out[i].Node != out[j].Node { + return out[i].Node < out[j].Node + } + return out[i].Rule < out[j].Rule + }) + return out +} + +// sameMemory compares two memory ids from the SAME GraphQL projection — the +// root node's and the endpoint's — so they are directly comparable. +// +// An earlier version compared against the -m flag's canonical ref instead. +// CanonicalMemoryRef emits the flat hrn:mem:: form, which contains +// no "::", so its "don't guess across spellings" guard matched every time and +// route-target-moved-memory could never fire in a real run. Its unit test +// passed only because it supplied the legacy org::mem spelling by hand. +func sameMemory(endpointMemoryID, rootMemoryID string) bool { + if endpointMemoryID == "" || rootMemoryID == "" { + return true // nothing conclusive to compare + } + return endpointMemoryID == rootMemoryID +} diff --git a/internal/cmd/coding/preflight_lint_test.go b/internal/cmd/coding/preflight_lint_test.go new file mode 100644 index 0000000..b86cbe7 --- /dev/null +++ b/internal/cmd/coding/preflight_lint_test.go @@ -0,0 +1,152 @@ +package coding + +import ( + "strings" + "testing" +) + +func route(target, label string) graphEdge { + return graphEdge{ID: "edg_" + target, Label: label, OtherID: "id_" + target, Other: target, MemoryID: "mem1"} +} + +func TestRouteLabelPhrasing(t *testing.T) { + cases := []struct{ label, want string }{ + {"to review your code before opening a PR", ""}, + {"To debug a failing run", ""}, // case-insensitive + {"routes-to", "route-label-phrasing"}, + {"", "route-label-phrasing"}, + {"related", "route-label-phrasing"}, + } + for _, tc := range cases { + in := preflightInput{ + Routes: []graphEdge{route("findings:x", tc.label)}, + Targets: map[string]checkNode{"findings:x": {Loc: "findings:x"}}, + HomeMemory: "mem1", + } + rules := rulesFor(lintPreflight(in), "findings:x") + switch { + case tc.want == "" && len(rules) > 0: + t.Errorf("label %q: expected clean, got %v", tc.label, rules) + case tc.want != "" && !has(rules, tc.want): + t.Errorf("label %q: expected %s, got %v", tc.label, tc.want, rules) + } + } +} + +// Stale routing is worse than missing routing, so an unresolvable target is the +// one error on this side — unlike the review tree, where an unreadable endpoint +// only warns. +func TestRouteTargetUnresolvableIsError(t *testing.T) { + in := preflightInput{ + Routes: []graphEdge{route("findings:gone", "to do the thing")}, + Targets: map[string]checkNode{}, + Unavailable: []string{"findings:gone"}, + HomeMemory: "mem1", + } + fs := lintPreflight(in) + if len(fs) != 1 || fs[0].Rule != "route-target-resolves" { + t.Fatalf("expected route-target-resolves, got %v", fs) + } + if fs[0].Severity != sevError { + t.Errorf("a dead route should be an error, got %q", fs[0].Severity) + } +} + +// Once a target is known unreadable, the convention checks have nothing to say +// about it — piling on would triple-report one broken route. +func TestUnresolvableTargetSuppressesOtherRules(t *testing.T) { + in := preflightInput{ + Routes: []graphEdge{route("findings:gone", "routes-to")}, + Unavailable: []string{"findings:gone"}, + HomeMemory: "mem1", + } + fs := lintPreflight(in) + if len(fs) != 1 { + t.Fatalf("expected exactly one finding for a dead route, got %v", fs) + } +} + +func TestRouteTargetRetired(t *testing.T) { + for _, tag := range retiredTags { + in := preflightInput{ + Routes: []graphEdge{route("findings:old", "to do the thing")}, + Targets: map[string]checkNode{"findings:old": {Loc: "findings:old", Tags: []string{tag}}}, + HomeMemory: "mem1", + } + if !has(rulesFor(lintPreflight(in), "findings:old"), "route-target-retired") { + t.Errorf("tag %q should trigger route-target-retired", tag) + } + } +} + +// Both ids come from the same GraphQL projection, so they compare directly. +// The earlier version compared the endpoint id against the -m flag's canonical +// ref, which is a URN and never matched a PK — the rule could not fire in a +// real run, and its test passed only by supplying a spelling the command never +// produces. These cases use PK-shaped ids on both sides, as the command does. +func TestRouteTargetMovedMemory(t *testing.T) { + const homePK = "019f76f283c27bc39c7f906c798e4268" + const otherPK = "019aaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + + r := route("findings:moved", "to do the thing") + r.MemoryID = otherPK + in := preflightInput{ + Routes: []graphEdge{r}, + Targets: map[string]checkNode{"findings:moved": {Loc: "findings:moved"}}, + HomeMemory: homePK, + } + if !has(rulesFor(lintPreflight(in), "findings:moved"), "route-target-moved-memory") { + t.Error("a target in another memory should be flagged") + } + + // Same memory — the overwhelmingly common case — must stay silent. + r.MemoryID = homePK + in.Routes = []graphEdge{r} + if has(rulesFor(lintPreflight(in), "findings:moved"), "route-target-moved-memory") { + t.Error("a same-memory target must not be flagged") + } + + // Nothing conclusive to compare. + r.MemoryID = "" + in.Routes = []graphEdge{r} + if has(rulesFor(lintPreflight(in), "findings:moved"), "route-target-moved-memory") { + t.Error("an absent memory id must not produce a finding") + } +} + +// A redacted endpoint projection leaves no loc. Skipping it would blind the +// command's only error rule to exactly the unreadable-target case. +func TestRouteWithRedactedTargetIsReported(t *testing.T) { + in := preflightInput{ + Routes: []graphEdge{{ID: "e9", Label: "to reach the hidden thing"}}, // no OtherID/Other + Targets: map[string]checkNode{}, + HomeMemory: "mem1", + } + fs := lintPreflight(in) + if len(fs) != 1 || fs[0].Rule != "route-target-resolves" { + t.Fatalf("a redacted target must be reported, got %v", fs) + } + if fs[0].Severity != sevError { + t.Errorf("expected an error, got %q", fs[0].Severity) + } + if !strings.Contains(fs[0].Node, "to reach the hidden thing") { + t.Errorf("the finding should identify the route by its label, got %q", fs[0].Node) + } +} + +func TestPreflightCleanRouter(t *testing.T) { + in := preflightInput{ + Routes: []graphEdge{ + route("findings:a", "to diagnose a slow query"), + route("findings:b", "to trace an auth failure"), + }, + Targets: map[string]checkNode{ + "findings:a": {Loc: "findings:a"}, + "findings:b": {Loc: "findings:b"}, + }, + HomeMemory: "mem1", + } + if fs := lintPreflight(in); len(fs) != 0 { + t.Errorf("a clean router should yield no findings, got %v", fs) + } +} diff --git a/internal/cmd/coding/review_lint.go b/internal/cmd/coding/review_lint.go new file mode 100644 index 0000000..25da990 --- /dev/null +++ b/internal/cmd/coding/review_lint.go @@ -0,0 +1,403 @@ +package coding + +import ( + "fmt" + "io" + "regexp" + "sort" + "strings" + + "github.com/spf13/cobra" + + "github.com/hadron-memory/hadron-cli/internal/cmdutil" + "github.com/hadron-memory/hadron-cli/internal/exitcode" + "github.com/hadron-memory/hadron-cli/internal/output" +) + +// triggerStem is the conventional opening of a check's edge label; the label +// must carry a real condition after it. +const triggerStem = "applies when" + +// reRelLoc matches the server's derived `:rel:` edge loc, which +// appears when an edge has no name. It corroborates an empty label rather than +// being a defect of its own — the label is what broke. +var reRelLoc = regexp.MustCompile(`(?i):rel:`) + +// reviewInput is everything the rule engine needs, so the engine itself is pure +// and unit-testable without a server. +type reviewInput struct { + Members map[string]checkNode // checklist items, by loc + Edges map[string]graphEdge // loc → its edge to the review parent + Unavailable []string // edge sources that could not be read + Toolchain string // "" = infer; "-" = disabled +} + +func newCmdReviewLint(f *cmdutil.Factory) *cobra.Command { + var memory, root, toolchain string + var strict bool + var fix, yes bool + cmd := &cobra.Command{ + Use: "lint", + Aliases: []string{"check", "validate"}, + Short: "Validate the review checklist tree's trigger edges", + Long: `Validate every check under the review parent. + +A check is triaged by ` + "`tasks:review-changes`" + ` reading its edge label +back to the review parent, so a label that is not a condition — "child-of", +"related", an empty label — makes the check silently stop firing. + +A node counts as a checklist item when it sits under the parent's loc +prefix (` + "`review:`" + ` by default) and is neither tagged ` + "`meta`" + ` +nor runnable. That excludes the router, the tasks that consume the tree, the +meta backlog, pattern nodes, and findings — all of which legitimately hang +off the same parent, some of them carrying a ` + "`review`" + ` tag. + +Errors exit 5; --strict promotes warnings to errors too.`, + Example: ` hadron coding review lint -m micromentor.org::mmdata + hadron coding review lint -m hadronmemory.com::hadron-portal --json + hadron coding review lint -m micromentor.org::mmdata --fix --yes`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + if yes && !fix { + return exitcode.Newf(exitcode.Usage, "--yes only applies with --fix") + } + mem, err := codingMemoryURN(memory) + if err != nil { + return err + } + client, err := f.GraphQLClient() + if err != nil { + return err + } + ctx := cmd.Context() + + edges, _, err := fetchRootEdges(ctx, client, mem, root, true) + if err != nil { + return err + } + + // Two sources of candidate checks: nodes under the root's child + // prefix (which catches a check with no edge at all — the + // highest-severity finding), and the far endpoints of the parent's + // inbound edges (which catches one that is unreadable). + listed, err := scanPrefix(ctx, client, mem, childPrefix(root)) + if err != nil { + return err + } + candidates := map[string]string{} // node id → loc + for _, n := range listed { + if n == nil { + continue + } + if isChecklistItemListing(root, n.Loc, n.Tags, n.IsRunnable) { + candidates[n.Id] = n.Loc + } + } + edgeByLoc := map[string]graphEdge{} + var redacted []graphEdge + for _, e := range edges { + if e.OtherID == "" { + // The server redacted the endpoint projection, so there is + // no node to read or classify — report it rather than + // letting it vanish from the sweep. + redacted = append(redacted, e) + continue + } + if e.Other != "" { + edgeByLoc[e.Other] = e + } + candidates[e.OtherID] = e.Other + } + + nodes, unavailable, err := fetchNodes(ctx, client, candidates) + if err != nil { + return err + } + for _, e := range redacted { + unavailable = append(unavailable, e.endpointName()) + } + + members := map[string]checkNode{} + for loc, n := range nodes { + if isChecklistItem(root, n) { + members[loc] = n + } + } + // An unreadable node can't be tested against the predicate, so its + // membership is indeterminate — reported, never dropped. + in := reviewInput{Members: members, Edges: edgeByLoc, Unavailable: unavailable, Toolchain: toolchain} + findings := lintReview(in) + + if fix { + applied, ferr := applyReviewFix(ctx, client, f, in, findings, yes) + if ferr != nil { + return ferr + } + if applied > 0 { + // Re-derive from the repaired state so the report reflects + // what is true after the write, not before. + for _, fx := range planReviewFix(in, findings) { + e := in.Edges[fx.Loc] + e.Label = fx.NewLabel + in.Edges[fx.Loc] = e + } + findings = lintReview(in) + fmt.Fprintf(f.IOStreams.ErrOut, "fixed %d edge label(s)\n", applied) + } + } + + if strict { + for i := range findings { + if findings[i].Severity == sevWarning { + findings[i].Severity = sevError + } + } + } + hasError := false + for _, fd := range findings { + if fd.Severity == sevError { + hasError = true + break + } + } + + if err := output.Write(f.IOStreams, f.JSON, findings, func(w io.Writer) error { + if len(findings) == 0 { + fmt.Fprintf(w, "✓ %d check(s) OK\n", len(members)) + return nil + } + t := output.NewTable(w, "NODE", "SEVERITY", "RULE", "MESSAGE") + for _, fd := range findings { + t.Row(fd.Node, fd.Severity, fd.Rule, fd.Message) + } + return t.Flush() + }); err != nil { + return err + } + if hasError { + return exitcode.Silent(exitcode.Conflict) + } + return nil + }, + } + cmd.Flags().StringVarP(&memory, "memory", "m", "", "memory to lint (org::memory)") + cmd.Flags().StringVar(&root, "root", reviewRootLoc, "loc of the review parent node") + cmd.Flags().StringVar(&toolchain, "toolchain", "", `the memory's toolchain for the foreign-toolchain check (e.g. "ts"; "-" disables; default: inferred)`) + cmd.Flags().BoolVar(&strict, "strict", false, "treat warnings as errors") + cmd.Flags().BoolVar(&fix, "fix", false, "promote a check's description into an empty/non-condition edge label where possible") + cmd.Flags().BoolVar(&yes, "yes", false, "skip the confirmation prompt for --fix") + return cmd +} + +// lintReview is the pure rule engine. Findings come back sorted (node, rule) so +// the output and the --json contract are deterministic. +func lintReview(in reviewInput) []findingDTO { + out := []findingDTO{} + + locs := make([]string, 0, len(in.Members)) + for l := range in.Members { + locs = append(locs, l) + } + sort.Strings(locs) + + // Per-check rules. + valid := map[string]string{} // loc → normalised trigger, for duplicate detection + for _, loc := range locs { + n := in.Members[loc] + e, hasEdge := in.Edges[loc] + if !hasEdge { + out = append(out, findingDTO{loc, "parent-edge-exists", sevError, + "no edge to the review parent — invisible to tasks:review-changes"}) + continue + } + label := strings.TrimSpace(e.Label) + switch { + case label == "": + msg := "edge label is empty — the check can never match a diff" + if reRelLoc.MatchString(e.Loc) { + msg += fmt.Sprintf(" (its derived loc %q confirms the name was never set)", e.Loc) + } + out = append(out, findingDTO{loc, "label-present", sevError, msg}) + case !strings.HasPrefix(strings.ToLower(label), triggerStem): + out = append(out, findingDTO{loc, "label-is-condition", sevError, + fmt.Sprintf("edge label %q is not a condition — expected %q followed by the trigger", label, triggerStem)}) + case len(strings.TrimSpace(label[len(triggerStem):])) == 0: + out = append(out, findingDTO{loc, "label-is-condition", sevError, + fmt.Sprintf("edge label is the bare stem %q with no condition after it", label)}) + default: + valid[loc] = strings.ToLower(strings.Join(strings.Fields(label), " ")) + } + if strings.TrimSpace(n.Description) == "" { + out = append(out, findingDTO{loc, "description-present", sevWarning, + "no description — list and search output show the description, so the trigger is invisible there"}) + } + } + + out = append(out, lintDuplicateTriggers(valid)...) + out = append(out, lintSeq(in, locs)...) + out = append(out, lintToolchain(in, locs, valid)...) + + for _, u := range sortedCopy(in.Unavailable) { + out = append(out, findingDTO{u, "check-node-resolves", sevWarning, + "edge source could not be read — membership is indeterminate, so it was neither linted nor dismissed"}) + } + + sort.SliceStable(out, func(i, j int) bool { + if out[i].Node != out[j].Node { + return out[i].Node < out[j].Node + } + return out[i].Rule < out[j].Rule + }) + return out +} + +// lintDuplicateTriggers flags checks sharing a trigger — usually one was cloned +// and never re-pointed. Only well-formed triggers take part, so a pair of +// `child-of` labels is reported once by label-is-condition, not twice. +func lintDuplicateTriggers(valid map[string]string) []findingDTO { + byTrigger := map[string][]string{} + for loc, t := range valid { + byTrigger[t] = append(byTrigger[t], loc) + } + var out []findingDTO + for t, locs := range byTrigger { + if len(locs) < 2 { + continue + } + sort.Strings(locs) + for _, loc := range locs { + others := make([]string, 0, len(locs)-1) + for _, o := range locs { + if o != loc { + others = append(others, o) + } + } + out = append(out, findingDTO{loc, "duplicate-trigger", sevWarning, + fmt.Sprintf("trigger %q is shared with %s — likely a clone that was never re-pointed", t, strings.Join(others, ", "))}) + } + } + return out +} + +// lintSeq flags siblings sharing a seq, which makes their order +// non-deterministic. An unset seq is not a finding — most checks leave it nil. +func lintSeq(in reviewInput, locs []string) []findingDTO { + bySeq := map[int][]string{} + for _, loc := range locs { + if s := in.Members[loc].Seq; s != nil { + bySeq[*s] = append(bySeq[*s], loc) + } + } + var out []findingDTO + for s, ls := range bySeq { + if len(ls) < 2 { + continue + } + sort.Strings(ls) + for _, loc := range ls { + out = append(out, findingDTO{loc, "seq-unique", sevWarning, + fmt.Sprintf("seq %d is shared with %s — sibling ordering is non-deterministic", s, strings.Join(without(ls, loc), ", "))}) + } + } + return out +} + +// toolchainFamilies maps a family to the words that name it in a trigger. +var toolchainFamilies = map[string]*regexp.Regexp{ + "dart": regexp.MustCompile(`(?i)\b(dart|flutter|pubspec)\b`), + "ts": regexp.MustCompile(`(?i)(\btypescript\b|\bjavascript\b|\.tsx?\b|\bnode\.js\b|\bnpm\b|\btsconfig\b)`), + "go": regexp.MustCompile(`(?i)(\bgolang\b|\bgo\.mod\b|\.go\b)`), + "swift": regexp.MustCompile(`(?i)\b(swift|xcode|cocoapods)\b`), + "kotlin": regexp.MustCompile(`(?i)\b(kotlin|gradle)\b`), + "python": regexp.MustCompile(`(?i)(\bpython\b|\bpip\b|\.py\b)`), +} + +// lintToolchain flags a trigger naming a toolchain the memory is not about — +// the misfiled check that can never match a diff in its own repo. +// +// Heuristic and warn-only. The memory's own family is inferred from the whole +// member corpus (names and descriptions, not just triggers — triggers alone are +// too sparse to break a tie), and the rule stays SILENT when it cannot pick a +// clear winner rather than guessing. --toolchain overrides; "-" disables. +func lintToolchain(in reviewInput, locs []string, valid map[string]string) []findingDTO { + if in.Toolchain == "-" { + return nil + } + home := in.Toolchain + if home == "" { + home = inferToolchain(in, locs) + } + if home == "" { + return nil + } + var out []findingDTO + for _, loc := range locs { + trigger, ok := valid[loc] + if !ok { + continue // a broken label is already reported; don't pile on + } + fams := familiesIn(trigger) + if len(fams) != 1 || fams[0] == home { + continue + } + out = append(out, findingDTO{loc, "foreign-toolchain", sevWarning, + fmt.Sprintf("trigger names %s but the memory looks like %s — it can never match a diff here", fams[0], home)}) + } + return out +} + +// inferToolchain picks the family the member corpus mentions most, requiring a +// strict winner so an even split yields no opinion. +func inferToolchain(in reviewInput, locs []string) string { + counts := map[string]int{} + for _, loc := range locs { + n := in.Members[loc] + text := n.Loc + " " + n.Name + " " + n.Description + if e, ok := in.Edges[loc]; ok { + text += " " + e.Label + } + for _, fam := range familiesIn(text) { + counts[fam]++ + } + } + best, bestN, tied := "", 0, false + for fam, c := range counts { + switch { + case c > bestN: + best, bestN, tied = fam, c, false + case c == bestN: + tied = true + } + } + if tied || bestN == 0 { + return "" + } + return best +} + +func familiesIn(text string) []string { + var out []string + for fam, re := range toolchainFamilies { + if re.MatchString(text) { + out = append(out, fam) + } + } + sort.Strings(out) + return out +} + +func without(all []string, drop string) []string { + out := make([]string, 0, len(all)) + for _, s := range all { + if s != drop { + out = append(out, s) + } + } + return out +} + +func sortedCopy(in []string) []string { + out := append([]string(nil), in...) + sort.Strings(out) + return out +} diff --git a/internal/cmd/coding/review_lint_test.go b/internal/cmd/coding/review_lint_test.go new file mode 100644 index 0000000..90a884d --- /dev/null +++ b/internal/cmd/coding/review_lint_test.go @@ -0,0 +1,309 @@ +package coding + +import ( + "strings" + "testing" +) + +func member(loc string) checkNode { + return checkNode{Loc: loc, Tags: []string{"review"}, Description: "Applies when something changes. Then do the thing."} +} + +func edge(loc, label string) graphEdge { + return graphEdge{ID: "edg_" + loc, Label: label, Loc: loc + ":" + label + ":review", Other: loc} +} + +// rulesFor returns the rules fired against one node, for compact assertions. +func rulesFor(fs []findingDTO, loc string) []string { + var out []string + for _, f := range fs { + if f.Node == loc { + out = append(out, f.Rule) + } + } + return out +} + +func has(rules []string, want string) bool { + for _, r := range rules { + if r == want { + return true + } + } + return false +} + +func TestLabelRules(t *testing.T) { + cases := []struct { + label string + want string // "" = no label finding + }{ + {"Applies when a resolver changes", ""}, + {"applies when a resolver changes", ""}, // stem is case-insensitive + {"", "label-present"}, + {" ", "label-present"}, + {"child-of", "label-is-condition"}, + {"applies-when", "label-is-condition"}, + {"related", "label-is-condition"}, + {"Applies when", "label-is-condition"}, // bare stem + {"Applies when ", "label-is-condition"}, // stem + whitespace only + } + for _, tc := range cases { + in := reviewInput{ + Members: map[string]checkNode{"review:x": member("review:x")}, + Edges: map[string]graphEdge{"review:x": edge("review:x", tc.label)}, + Toolchain: "-", + } + rules := rulesFor(lintReview(in), "review:x") + switch { + case tc.want == "" && len(rules) > 0: + t.Errorf("label %q: expected clean, got %v", tc.label, rules) + case tc.want != "" && !has(rules, tc.want): + t.Errorf("label %q: expected %s, got %v", tc.label, tc.want, rules) + } + } +} + +// A check with no edge to the parent is invisible to tasks:review-changes — +// the highest-severity finding, and one only the memory sweep can see. +func TestParentEdgeMissing(t *testing.T) { + in := reviewInput{ + Members: map[string]checkNode{"review:orphan": member("review:orphan")}, + Edges: map[string]graphEdge{}, + Toolchain: "-", + } + fs := lintReview(in) + if !has(rulesFor(fs, "review:orphan"), "parent-edge-exists") { + t.Fatalf("expected parent-edge-exists, got %v", fs) + } + if fs[0].Severity != sevError { + t.Errorf("parent-edge-exists should be an error, got %q", fs[0].Severity) + } +} + +// The :rel: loc is the server's derived fallback for a nameless edge, so it +// corroborates the empty label rather than being its own rule. +func TestEmptyLabelMentionsRelLoc(t *testing.T) { + e := graphEdge{ID: "e1", Label: "", Loc: "review:x:rel:review", Other: "review:x"} + in := reviewInput{ + Members: map[string]checkNode{"review:x": member("review:x")}, + Edges: map[string]graphEdge{"review:x": e}, + Toolchain: "-", + } + fs := lintReview(in) + var msg string + for _, f := range fs { + if f.Rule == "label-present" { + msg = f.Message + } + } + if msg == "" { + t.Fatalf("expected label-present, got %v", fs) + } + if !strings.Contains(msg, "review:x:rel:review") { + t.Errorf("empty-label message should cite the derived loc, got %q", msg) + } + for _, f := range fs { + if f.Rule == "label-is-loc" { + t.Error("the :rel: loc must not be its own rule") + } + } +} + +// A pair of `child-of` labels is one defect reported once per node by +// label-is-condition — duplicate-trigger must not double-report it. +func TestDuplicateTriggerOnlyCountsValidConditions(t *testing.T) { + in := reviewInput{ + Members: map[string]checkNode{"review:a": member("review:a"), "review:b": member("review:b")}, + Edges: map[string]graphEdge{ + "review:a": edge("review:a", "child-of"), + "review:b": edge("review:b", "child-of"), + }, + Toolchain: "-", + } + for _, f := range lintReview(in) { + if f.Rule == "duplicate-trigger" { + t.Fatalf("broken labels must not also be reported as duplicate triggers: %+v", f) + } + } + + // Two genuinely valid, identical triggers do get flagged. + in.Edges = map[string]graphEdge{ + "review:a": edge("review:a", "Applies when a rule subclass changes"), + "review:b": edge("review:b", "Applies when a rule subclass changes"), + } + n := 0 + for _, f := range lintReview(in) { + if f.Rule == "duplicate-trigger" { + n++ + if f.Severity != sevWarning { + t.Errorf("duplicate-trigger should be a warning, got %q", f.Severity) + } + } + } + if n != 2 { + t.Errorf("expected both siblings flagged, got %d", n) + } +} + +func TestSeqUniqueness(t *testing.T) { + s34, other := 34, 35 + a, b, c := member("review:a"), member("review:b"), member("review:c") + a.Seq, b.Seq, c.Seq = &s34, &s34, &other + in := reviewInput{ + Members: map[string]checkNode{"review:a": a, "review:b": b, "review:c": c}, + Edges: map[string]graphEdge{ + "review:a": edge("review:a", "Applies when a changes"), + "review:b": edge("review:b", "Applies when b changes"), + "review:c": edge("review:c", "Applies when c changes"), + }, + Toolchain: "-", + } + fs := lintReview(in) + if !has(rulesFor(fs, "review:a"), "seq-unique") || !has(rulesFor(fs, "review:b"), "seq-unique") { + t.Errorf("expected both seq-34 siblings flagged, got %v", fs) + } + if has(rulesFor(fs, "review:c"), "seq-unique") { + t.Error("a unique seq must not be flagged") + } + + // An unset seq is the norm, not a finding. + for loc := range in.Members { + m := in.Members[loc] + m.Seq = nil + in.Members[loc] = m + } + for _, f := range lintReview(in) { + if f.Rule == "seq-unique" { + t.Errorf("unset seq must not be flagged: %+v", f) + } + } +} + +func TestDescriptionPresent(t *testing.T) { + n := member("review:x") + n.Description = "" + in := reviewInput{ + Members: map[string]checkNode{"review:x": n}, + Edges: map[string]graphEdge{"review:x": edge("review:x", "Applies when x changes")}, + Toolchain: "-", + } + fs := lintReview(in) + if !has(rulesFor(fs, "review:x"), "description-present") { + t.Errorf("expected description-present, got %v", fs) + } + if fs[0].Severity != sevWarning { + t.Errorf("description-present should be a warning, got %q", fs[0].Severity) + } +} + +func TestForeignToolchain(t *testing.T) { + // A corpus that clearly reads as TypeScript, plus one Dart trigger. + mk := func(loc, desc string) checkNode { + return checkNode{Loc: loc, Tags: []string{"review"}, Description: desc} + } + in := reviewInput{ + Members: map[string]checkNode{ + "review:a": mk("review:a", "Applies when a TypeScript file changes"), + "review:b": mk("review:b", "Guards npm dependency drift in TypeScript"), + "review:c": mk("review:c", "TypeScript resolver conventions for .ts modules"), + "review:d": mk("review:d", "Runs the formatter"), + }, + Edges: map[string]graphEdge{ + "review:a": edge("review:a", "Applies when any TypeScript file changes"), + "review:b": edge("review:b", "Applies when package.json changes"), + "review:c": edge("review:c", "Applies when a .ts resolver changes"), + "review:d": edge("review:d", "Applies when Dart sources change"), + }, + } + fs := lintReview(in) + if !has(rulesFor(fs, "review:d"), "foreign-toolchain") { + t.Errorf("expected the Dart trigger flagged in a TS corpus, got %v", fs) + } + for _, loc := range []string{"review:a", "review:b", "review:c"} { + if has(rulesFor(fs, loc), "foreign-toolchain") { + t.Errorf("%s is native to the corpus and must not be flagged", loc) + } + } + + // Explicit --toolchain overrides inference. + in.Toolchain = "dart" + if has(rulesFor(lintReview(in), "review:d"), "foreign-toolchain") { + t.Error("--toolchain dart should make the Dart trigger native") + } + // And "-" disables the heuristic outright. + in.Toolchain = "-" + for _, f := range lintReview(in) { + if f.Rule == "foreign-toolchain" { + t.Errorf("--toolchain - must disable the rule: %+v", f) + } + } +} + +// The heuristic must stay silent when it cannot pick a winner, rather than +// guessing — an even split is exactly the mmdata trigger corpus. +func TestForeignToolchainSilentWhenAmbiguous(t *testing.T) { + in := reviewInput{ + Members: map[string]checkNode{ + "review:a": {Loc: "review:a", Tags: []string{"review"}, Description: "d"}, + "review:b": {Loc: "review:b", Tags: []string{"review"}, Description: "d"}, + }, + Edges: map[string]graphEdge{ + "review:a": edge("review:a", "Applies when Dart sources change"), + "review:b": edge("review:b", "Applies when a TypeScript file changes"), + }, + } + for _, f := range lintReview(in) { + if f.Rule == "foreign-toolchain" { + t.Errorf("a 1-1 split must yield no opinion, got %+v", f) + } + } +} + +// An unreadable edge source can't be tested against the membership predicate, +// so it is reported as indeterminate rather than dropped (CLAUDE.md) — and as a +// warning, since it may well not be a checklist item at all. +func TestUnavailableSurfaced(t *testing.T) { + in := reviewInput{ + Members: map[string]checkNode{}, + Edges: map[string]graphEdge{}, + Unavailable: []string{"tasks:build-review-coverage"}, + Toolchain: "-", + } + fs := lintReview(in) + if len(fs) != 1 || fs[0].Rule != "check-node-resolves" { + t.Fatalf("expected check-node-resolves, got %v", fs) + } + if fs[0].Node != "tasks:build-review-coverage" { + t.Errorf("finding should name the unreadable loc, got %q", fs[0].Node) + } + if fs[0].Severity != sevWarning { + t.Errorf("indeterminate membership should warn, not error; got %q", fs[0].Severity) + } +} + +func TestFindingsAreDeterministic(t *testing.T) { + in := reviewInput{ + Members: map[string]checkNode{ + "review:b": {Loc: "review:b", Tags: []string{"review"}}, + "review:a": {Loc: "review:a", Tags: []string{"review"}}, + }, + Edges: map[string]graphEdge{ + "review:a": edge("review:a", "child-of"), + "review:b": edge("review:b", "child-of"), + }, + Toolchain: "-", + } + first := lintReview(in) + for i := 0; i < 5; i++ { + got := lintReview(in) + if len(got) != len(first) { + t.Fatalf("unstable finding count: %d vs %d", len(got), len(first)) + } + for j := range got { + if got[j] != first[j] { + t.Fatalf("unstable order at %d: %+v vs %+v", j, got[j], first[j]) + } + } + } +} diff --git a/internal/cmd/coding_cmd_test.go b/internal/cmd/coding_cmd_test.go new file mode 100644 index 0000000..9eab275 --- /dev/null +++ b/internal/cmd/coding_cmd_test.go @@ -0,0 +1,343 @@ +package cmd + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/hadron-memory/hadron-cli/internal/exitcode" +) + +const codingMem = "acme.com::kb" + +// codingRootJSON is a lint root (review or preflight) carrying the given edges. +func codingRootJSON(loc, incoming, outgoing string) string { + return `{"data":{"node":{"id":"root","memoryId":"mem1","loc":"` + loc + `","name":"` + loc + `", + "description":null,"abstract":null,"abstractOriginHash":null,"nodeType":"info","objectType":null, + "tags":[],"content":null,"data":null,"properties":null,"seq":null,"isRunnable":false, + "createdAt":"2026-07-30T00:00:00Z","updatedAt":"2026-07-30T00:00:00Z", + "outgoingEdges":[` + outgoing + `],"incomingEdges":[` + incoming + `]}}}` +} + +// inEdge is an edge pointing AT the root; its far endpoint is the source. +func inEdge(id, label, sourceLoc string) string { + return `{"id":"` + id + `","name":` + jsonStr(label) + `,"loc":"` + sourceLoc + `:x:review","isRunnable":false, + "priority":0,"source":{"id":"s_` + id + `","loc":"` + sourceLoc + `","memoryId":"` + codingMem + `"}}` +} + +// outEdge is a route OUT of the root; its far endpoint is the target. +func outEdge(id, label, targetLoc string) string { + return `{"id":"` + id + `","name":` + jsonStr(label) + `,"loc":"preflight:x:` + targetLoc + `","isRunnable":false, + "priority":0,"target":{"id":"t_` + id + `","loc":"` + targetLoc + `","memoryId":"` + codingMem + `"}}` +} + +func jsonStr(s string) string { + b, _ := json.Marshal(s) + return string(b) +} + +// codingListNode is one node in the FindNodes listing projection. Membership +// is decided by the loc prefix, so the tag set is immaterial here. +func codingListNode(loc string) string { + return `{"id":"n_` + loc + `","memoryId":"mem1","loc":"` + loc + `","name":"` + loc + `", + "nodeType":"info","tags":[],"seq":null,"isRunnable":false,"updatedAt":"2026-07-30T00:00:00Z"}` +} + +// codingBatchNode is one node in the nodeBatch projection. +func codingBatchNode(loc, tags, description string) string { + return `{"id":"n_` + loc + `","memoryId":"mem1","loc":"` + loc + `","name":"` + loc + `", + "alias":null,"nodeType":"info","objectType":null,"isRunnable":false,"description":` + jsonStr(description) + `, + "abstract":null,"abstractOriginHash":null,"tags":[` + tags + `],"seq":null,"data":null,"properties":null, + "content":null,"createdAt":"2026-07-30T00:00:00Z","updatedAt":"2026-07-30T00:00:00Z", + "outgoingEdges":[],"incomingEdges":[]}` +} + +func codingBatch(nodes []string, unavailable string) string { + return `{"data":{"nodeBatch":{"truncated":false,"omitted":[],"unavailable":[` + unavailable + `], + "nodes":[` + strings.Join(nodes, ",") + `]}}}` +} + +func TestCodingReviewLintClean(t *testing.T) { + gql := fakeGraphQL(t, map[string]string{ + "GetNode": codingRootJSON("review", inEdge("e1", "Applies when a resolver changes", "review:ok"), ""), + "FindNodes": `{"data":{"nodes":[` + codingListNode("review:ok") + `]}}`, + "NodeBatch": codingBatch([]string{codingBatchNode("review:ok", `"review"`, "Applies when a resolver changes.")}, ""), + }) + f, out := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"coding", "review", "lint", "-m", codingMem, "--server", gql.URL}) + if err := root.Execute(); err != nil { + t.Fatalf("clean corpus should exit 0, got %v", err) + } + if !strings.Contains(out.String(), "1 check(s) OK") { + t.Errorf("expected the OK summary, got %q", out.String()) + } +} + +func TestCodingReviewLintErrorsExit5(t *testing.T) { + gql := fakeGraphQL(t, map[string]string{ + "GetNode": codingRootJSON("review", + inEdge("e1", "child-of", "review:bad")+","+inEdge("e2", "", "review:empty"), ""), + "FindNodes": `{"data":{"nodes":[` + + codingListNode("review:bad") + `,` + codingListNode("review:empty") + `]}}`, + "NodeBatch": codingBatch([]string{ + codingBatchNode("review:bad", `"review"`, "Verifies a thing."), + codingBatchNode("review:empty", `"review"`, "Verifies another thing."), + }, ""), + }) + f, out := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"coding", "review", "lint", "-m", codingMem, "--server", gql.URL}) + if got := exitcode.FromError(root.Execute()); got != exitcode.Conflict { + t.Errorf("errors should exit 5 (Conflict), got %d", got) + } + s := out.String() + if !strings.Contains(s, "label-is-condition") || !strings.Contains(s, "label-present") { + t.Errorf("expected both label rules in the table, got %q", s) + } +} + +// Warnings alone exit 0; --strict promotes them and flips the exit code. +func TestCodingReviewLintStrict(t *testing.T) { + mk := func() string { + return codingRootJSON("review", inEdge("e1", "Applies when a thing changes", "review:nodesc"), "") + } + responses := map[string]string{ + "GetNode": mk(), + "FindNodes": `{"data":{"nodes":[` + codingListNode("review:nodesc") + `]}}`, + "NodeBatch": codingBatch([]string{codingBatchNode("review:nodesc", `"review"`, "")}, ""), + } + + gql := fakeGraphQL(t, responses) + f, _ := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"coding", "review", "lint", "-m", codingMem, "--server", gql.URL}) + if err := root.Execute(); err != nil { + t.Fatalf("warnings alone should exit 0, got %v", err) + } + + gql2 := fakeGraphQL(t, responses) + f2, _ := testFactory(t) + root2 := NewRootCmd(f2) + root2.SetArgs([]string{"coding", "review", "lint", "-m", codingMem, "--strict", "--server", gql2.URL}) + if got := exitcode.FromError(root2.Execute()); got != exitcode.Conflict { + t.Errorf("--strict should promote warnings to errors (exit 5), got %d", got) + } +} + +func TestCodingReviewLintJSON(t *testing.T) { + gql := fakeGraphQL(t, map[string]string{ + "GetNode": codingRootJSON("review", inEdge("e1", "child-of", "review:bad"), ""), + "FindNodes": `{"data":{"nodes":[` + codingListNode("review:bad") + `]}}`, + "NodeBatch": codingBatch([]string{codingBatchNode("review:bad", `"review"`, "Verifies a thing.")}, ""), + }) + f, out := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"coding", "review", "lint", "-m", codingMem, "--json", "--server", gql.URL}) + _ = root.Execute() + + var findings []struct { + Node string `json:"node"` + Rule string `json:"rule"` + Severity string `json:"severity"` + Message string `json:"message"` + } + if err := json.Unmarshal([]byte(out.String()), &findings); err != nil { + t.Fatalf("--json must emit a JSON array: %v (%q)", err, out.String()) + } + if len(findings) != 1 || findings[0].Node != "review:bad" || findings[0].Rule != "label-is-condition" { + t.Errorf("unexpected --json payload: %+v", findings) + } + if findings[0].Severity != "error" { + t.Errorf("severity should be error, got %q", findings[0].Severity) + } +} + +// A node that lists but cannot be read must be surfaced, not dropped. +func TestCodingReviewLintSurfacesUnavailable(t *testing.T) { + gql := fakeGraphQL(t, map[string]string{ + "GetNode": codingRootJSON("review", inEdge("e1", "related", "tasks:ghost"), ""), + "FindNodes": `{"data":{"nodes":[]}}`, + "NodeBatch": codingBatch(nil, `"s_e1"`), + }) + f, out := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"coding", "review", "lint", "-m", codingMem, "--server", gql.URL}) + if err := root.Execute(); err != nil { + t.Fatalf("an unreadable endpoint should warn, not error: %v", err) + } + s := out.String() + if !strings.Contains(s, "check-node-resolves") || !strings.Contains(s, "tasks:ghost") { + t.Errorf("expected the unreadable node surfaced by bare loc, got %q", s) + } +} + +// Decision 3: review reads the edge's SOURCE, preflight reads its TARGET. A +// single-endpoint implementation would fail one of these two. +func TestCodingLintReadsOppositeEndpoints(t *testing.T) { + // review: only incomingEdges carry the check; an outgoing edge is not one. + gql := fakeGraphQL(t, map[string]string{ + "GetNode": codingRootJSON("review", + inEdge("e1", "child-of", "review:src"), + outEdge("e2", "to somewhere", "review:tgt")), + "FindNodes": `{"data":{"nodes":[` + codingListNode("review:src") + `]}}`, + "NodeBatch": codingBatch([]string{codingBatchNode("review:src", `"review"`, "Verifies.")}, ""), + }) + f, out := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"coding", "review", "lint", "-m", codingMem, "--server", gql.URL}) + _ = root.Execute() + if !strings.Contains(out.String(), "review:src") { + t.Errorf("review lint must read the incoming edge's source, got %q", out.String()) + } + if strings.Contains(out.String(), "review:tgt") { + t.Errorf("review lint must ignore outgoing edges, got %q", out.String()) + } + + // preflight: only outgoingEdges are routes. + gql2 := fakeGraphQL(t, map[string]string{ + "GetNode": codingRootJSON("preflight", + inEdge("e3", "whatever", "findings:inbound"), + outEdge("e4", "routes-to", "findings:route")), + "NodeBatch": codingBatch([]string{codingBatchNode("findings:route", "", "")}, ""), + }) + f2, out2 := testFactory(t) + root2 := NewRootCmd(f2) + root2.SetArgs([]string{"coding", "preflight", "lint", "-m", codingMem, "--server", gql2.URL}) + _ = root2.Execute() + if !strings.Contains(out2.String(), "findings:route") { + t.Errorf("preflight lint must read the outgoing edge's target, got %q", out2.String()) + } + if strings.Contains(out2.String(), "findings:inbound") { + t.Errorf("preflight lint must ignore incoming edges, got %q", out2.String()) + } +} + +func TestCodingPreflightLintDeadRouteExit5(t *testing.T) { + gql := fakeGraphQL(t, map[string]string{ + "GetNode": codingRootJSON("preflight", "", outEdge("e1", "to do the thing", "findings:gone")), + "NodeBatch": codingBatch(nil, `"t_e1"`), + }) + f, out := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"coding", "preflight", "lint", "-m", codingMem, "--server", gql.URL}) + if got := exitcode.FromError(root.Execute()); got != exitcode.Conflict { + t.Errorf("a dead route should exit 5, got %d", got) + } + if !strings.Contains(out.String(), "route-target-resolves") { + t.Errorf("expected route-target-resolves, got %q", out.String()) + } +} + +// Route targets are read by node id. Rebuilding a URN from the router's own +// memory would look a cross-memory target up in the wrong place — reporting a +// live node as a dead route, or linting a same-loc node from the home memory. +func TestCodingPreflightReadsTargetsByID(t *testing.T) { + gql, captured := captureGraphQL(t, map[string]string{ + "GetNode": codingRootJSON("preflight", "", outEdge("e1", "to do the thing", "findings:elsewhere")), + "NodeBatch": codingBatch([]string{codingBatchNode("findings:elsewhere", "", "")}, ""), + }) + f, _ := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"coding", "preflight", "lint", "-m", codingMem, "--server", gql.URL}) + _ = root.Execute() + + var got struct { + Refs []string `json:"refs"` + } + if err := json.Unmarshal(captured["NodeBatch"], &got); err != nil { + t.Fatalf("decoding NodeBatch vars: %v", err) + } + if len(got.Refs) != 1 || got.Refs[0] != "t_e1" { + t.Errorf("targets must be batch-read by node id, got %v", got.Refs) + } + for _, r := range got.Refs { + if strings.HasPrefix(r, "hrn:") { + t.Errorf("a rebuilt URN (%q) resolves in the router's memory, not the target's", r) + } + } +} + +// --fix must relabel via updateEdge. Going through updateNode(edges:) would +// replace the node's whole outgoing edge set and destroy sibling edges (#325). +func TestCodingReviewFixUsesUpdateEdgeOnly(t *testing.T) { + gql, captured := captureGraphQL(t, map[string]string{ + "GetNode": codingRootJSON("review", inEdge("e1", "child-of", "review:fixable"), ""), + "FindNodes": `{"data":{"nodes":[` + codingListNode("review:fixable") + `]}}`, + "NodeBatch": codingBatch([]string{codingBatchNode("review:fixable", `"review"`, "Applies when a resolver changes. More prose.")}, ""), + "UpdateEdge": `{"data":{"updateEdge":{"id":"e1","name":"Applies when a resolver changes","loc":"l", + "isRunnable":false,"priority":0,"source":{"id":"s","loc":"review:fixable"},"target":{"id":"r","loc":"review"}}}}`, + }) + f, _ := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"coding", "review", "lint", "-m", codingMem, "--fix", "--yes", "--server", gql.URL}) + _ = root.Execute() + + vars, ok := captured["UpdateEdge"] + if !ok { + t.Fatal("--fix must issue UpdateEdge") + } + var got struct { + EdgeID string `json:"edgeId"` + Name *string `json:"name"` + } + if err := json.Unmarshal(vars, &got); err != nil { + t.Fatalf("decoding UpdateEdge vars: %v", err) + } + if got.EdgeID != "e1" { + t.Errorf("UpdateEdge should target the edge id, got %q", got.EdgeID) + } + if got.Name == nil || *got.Name != "Applies when a resolver changes" { + t.Errorf("unexpected promoted label: %v", got.Name) + } + if _, bad := captured["UpdateNode"]; bad { + t.Error("--fix must never call UpdateNode — it would replace the whole edge set") + } +} + +// A broken label whose description carries no trigger needs a human; --fix must +// leave it alone rather than invent a condition. +func TestCodingReviewFixSkipsUnfixable(t *testing.T) { + gql, captured := captureGraphQL(t, map[string]string{ + "GetNode": codingRootJSON("review", inEdge("e1", "child-of", "review:manual"), ""), + "FindNodes": `{"data":{"nodes":[` + codingListNode("review:manual") + `]}}`, + "NodeBatch": codingBatch([]string{codingBatchNode("review:manual", `"review"`, "Verifies the codegen output.")}, ""), + }) + f, _ := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"coding", "review", "lint", "-m", codingMem, "--fix", "--yes", "--server", gql.URL}) + if got := exitcode.FromError(root.Execute()); got != exitcode.Conflict { + t.Errorf("the unfixable error should still be reported, got exit %d", got) + } + if _, bad := captured["UpdateEdge"]; bad { + t.Error("--fix must not write when no description carries a trigger") + } +} + +func TestCodingReviewFixRequiresYes(t *testing.T) { + gql := fakeGraphQL(t, map[string]string{ + "GetNode": codingRootJSON("review", inEdge("e1", "child-of", "review:fixable"), ""), + "FindNodes": `{"data":{"nodes":[` + codingListNode("review:fixable") + `]}}`, + "NodeBatch": codingBatch([]string{codingBatchNode("review:fixable", `"review"`, "Applies when a resolver changes.")}, ""), + }) + f, _ := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"coding", "review", "lint", "-m", codingMem, "--fix", "--server", gql.URL}) + if err := root.Execute(); err == nil { + t.Error("--fix without --yes must fail non-interactively") + } +} + +func TestCodingLintRequiresMemory(t *testing.T) { + for _, args := range [][]string{ + {"coding", "review", "lint"}, + {"coding", "preflight", "lint"}, + } { + f, _ := testFactory(t) + root := NewRootCmd(f) + root.SetArgs(append(args, "--server", "http://127.0.0.1:1")) + if got := exitcode.FromError(root.Execute()); got != exitcode.Usage { + t.Errorf("%v without -m should be a usage error, got %d", args, got) + } + } +} diff --git a/internal/cmd/root.go b/internal/cmd/root.go index 42f7199..a6019a7 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -18,6 +18,7 @@ import ( appcmd "github.com/hadron-memory/hadron-cli/internal/cmd/app" authcmd "github.com/hadron-memory/hadron-cli/internal/cmd/auth" chatcmd "github.com/hadron-memory/hadron-cli/internal/cmd/chat" + codingcmd "github.com/hadron-memory/hadron-cli/internal/cmd/coding" "github.com/hadron-memory/hadron-cli/internal/cmd/configcmd" connectioncmd "github.com/hadron-memory/hadron-cli/internal/cmd/connection" edgecmd "github.com/hadron-memory/hadron-cli/internal/cmd/edge" @@ -71,6 +72,7 @@ func NewRootCmd(f *cmdutil.Factory) *cobra.Command { root.AddCommand(chatcmd.NewCmdChat(f)) root.AddCommand(replacecmd.NewCmdReplace(f)) root.AddCommand(speccmd.NewCmdSpec(f)) + root.AddCommand(codingcmd.NewCmdCoding(f)) root.AddCommand(appcmd.NewCmdApp(f)) root.AddCommand(orgcmd.NewCmdOrg(f)) root.AddCommand(agentcmd.NewCmdAgent(f))