From 5eef9aec0ae129a91b22c232d0638f9fc670fd2a Mon Sep 17 00:00:00 2001 From: Holger Selover-Stephan Date: Thu, 30 Jul 2026 12:21:03 +0200 Subject: [PATCH 1/2] docs(plans): design doc for the `hadron coding` lint group (#325) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Design-ahead plan for #325, written after auditing the live memories. Nothing is implemented; the doc exists to settle the open questions before code, because the surface as filed produces a >50% false-positive rate. Five decisions resolved against live data: 1. Membership rule. The issue scopes to "every node under the review parent", but that parent legitimately has non-checklist neighbours (the router, the consuming tasks, a meta backlog, pattern nodes). Implementing its checks verbatim gives 9 findings on mmdata, 5 of them false. Proposed rule — has the `review` tag, no `meta` tag, not isRunnable — yields 3 findings, all real, zero false positives. 2. "Label isn't a loc" checks the wrong field: the `:rel:` string is the edge's derived loc, not its name, and appears when the name is empty. Replaced with an empty-label check; `:rel:` becomes corroboration in the message. 3. Added target-resolves as an error — the live review tree has a dangling edge to tasks:build-review-coverage, which lists but cannot be read. 4. Preflight action-phrasing is a warning: 24 of 71 mmdata routes fail it on day one, and spec lint already treats convention violations as warnings. 5. --fix uses the existing `edge update`/updateEdge path, so the MCP whole-node hazard doesn't apply and server#835 is not a dependency. Also records the repo constraints whoever implements it will hit: the agentic-usage completeness gate, spec lint's exit 5 (not 1), and the #23 pagination requirement. No GraphQL changes are needed. Co-Authored-By: Claude Opus 5 --- docs/plans/coding-command-group.md | 259 +++++++++++++++++++++++++++++ 1 file changed, 259 insertions(+) create mode 100644 docs/plans/coding-command-group.md diff --git a/docs/plans/coding-command-group.md b/docs/plans/coding-command-group.md new file mode 100644 index 0000000..0d0922e --- /dev/null +++ b/docs/plans/coding-command-group.md @@ -0,0 +1,259 @@ +# 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. + +## Context + +The `review:*` checklist trees and the `preflight` routers are executable +infrastructure. `tasks:review-changes` triages checks by reading each one's +`Applies when …` edge label back to the `review` parent; `preflight` routes +symptom → finding along its outgoing edges. When an edge label is malformed the +check is **silently skipped** — the node still exists, still looks maintained, +and never fires again. There is no mechanical detector today. + +`hadron spec lint` is the prior art: the same idea for the spec corpus, with a +finding DTO, per-rule severities, and a documented exit code. This mirrors it +for the coding-workflow graph. + +## What the live audit actually found + +Everything below was measured against the live memories with the current +binary; the numbers drive the decisions that follow. + +| Memory | Inbound edges on `review` | Genuine defects | +|---|---|---| +| `hadronmemory.com::hadron-portal` | 25 | **0** — all 25 carry a real `Applies when …` condition | +| `micromentor.org::mmdata` | 54 | **4** | + +The portal's "7 of 24" from the issue **does not reproduce**; it appears to have +been fixed between the audit and filing. The mmdata four are real and confirmed: + +| Node | Label | Defect | +|---|---|---| +| `review:posthog-backend-vs-app-event-routing` | `child-of` | not a condition | +| `review:role-vs-group-ident-vocabulary` | `child-of` | not a condition | +| `review:input-type-graphql-type` | *(empty)* | no label at all | +| `review:format-sources` | `Applies when Dart sources change` | foreign toolchain — lives in the TypeScript backend memory | + +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 + `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. + +## Decisions (resolved against live data) + +### 1. Membership rule — which nodes are checklist items + +**This is the load-bearing decision, and the issue does not specify it.** Its +scope is "every node under the `review` parent", but the `review` parent +legitimately has non-checklist neighbours: the router, the tasks that consume the +tree, a meta backlog, pattern nodes. Implementing the issue's checks verbatim +gives **9 findings on mmdata, of which 5 are false positives** — a linter that +cries wolf on more than half its output will not be adopted. + +A loc-prefix rule does not fix it either: `review:backlog` sits under `review:` +and is explicitly meta. + +The node metadata separates them cleanly: + +| Node | `review` tag | `meta` tag | `isRunnable` | is a check? | +|---|---|---|---|---| +| `review:thin-resolver-field` | ✅ | — | false | ✅ | +| `review:input-type-graphql-type` | ✅ | — | false | ✅ | +| `review:backlog` | ✅ | ✅ | false | ❌ meta | +| `tasks:review-changes` | — | — | **true** | ❌ task | +| `patterns:function-signatures` | — | — | false | ❌ pattern | + +**Decision — a node is a checklist item iff it has the `review` tag, does *not* +have the `meta` tag, and is not `isRunnable`.** + +Validated end to end on mmdata's 54 inbound edges: + +``` +members (checklist items): 31 +excluded (meta/task/other): 22 +unreadable: 1 + +findings among members: + [EMPTY LABEL] review:input-type-graphql-type '' + [NOT-A-CONDITION] review:posthog-backend-vs-app-event-routing 'child-of' + [NOT-A-CONDITION] review:role-vs-group-ident-vocabulary 'child-of' +``` + +**3 findings, all real, zero false positives** — versus 9/5 for the unscoped +rule. The five would-be false positives (`patterns:function-signatures`, +`preflight`, `review:backlog`, `tasks:review-changes`, +`tasks:update-review-parent-node`) are all correctly excluded. + +`review:format-sources` is correctly *not* flagged by the label-shape rules — it +needs the toolchain heuristic, which stays warn-only as filed. + +### 2. "Label isn't a loc" → "label is empty" + +The issue's example, `review:input-type-graphql-type:rel:review`, is that edge's +**`loc`**, not its **`name`**. The name is empty: + +```json +{ "name": "", "loc": "review:input-type-graphql-type:rel:review", + "otherNodeLoc": "review:input-type-graphql-type" } +``` + +Edge locs are auto-derived by slugifying the name — confirmed on the healthy +ones (`name='Applies when adding or modifying a GraphQL resolver…'` → +`loc=review:thin-resolver-field:Applies-when-adding-or-modifying-a-GraphQL-resolver-f`). +So `:rel:` is the **derived-loc fallback when the name is +empty**: a symptom, not an independent defect class. There is exactly **one** such +edge, so the "one bad batch of 5" reading does not hold either. + +**Decision — check `name` for empty/missing (error). Drop the loc-shape rule; +mention a `:rel:` loc in the finding message as corroboration only.** + +### 3. New check: the edge target must resolve + +Not in the issue for `review lint` (only for `preflight lint`), but the live +`review` tree has one (`tasks:build-review-coverage`, above). + +**Decision — add `target-resolves` as an error, and follow CLAUDE.md: a target +that lists but cannot be read is reported as `unavailable`, never silently +dropped.** + +### 4. Preflight action-phrasing is a warning, not an error + +24 of 71 mmdata routes fail it immediately. As an error the command is red until +someone relabels 24 edges, which trains people to ignore it. `spec lint` already +treats convention violations as warnings and shape violations as errors. + +**Decision — `route-label-phrasing` is a warning; `route-target-resolves` is an +error. `--strict` promotes warnings, as in `spec lint`.** + +### 5. `--fix` uses `edge update`; server#835 is not a dependency + +The issue's ⚠️ hazard is an MCP-side limitation. This repo is not exposed to it: +`hadron edge update` already exists ([`internal/cmd/edge/update.go`](../../internal/cmd/edge/update.go), +wired at `edge.go:54`) over the `UpdateEdge` mutation +([`nodes.graphql:507`](../../internal/api/queries/nodes.graphql)), whose optional +variables all carry `# @genqlient(omitempty: true)`. One edge, one field, no +whole-node write, sibling `documented-by` / `relates-to` edges untouched. + +**Decision — `--fix` calls `updateEdge` per edge and must never route through +`updateNode(edges:)`. hadron-memory/hadron-server#835 is not a blocker.** + +## Command surface + +``` +hadron coding + + review lint [-m ] [--json] [--strict] [--fix] [--yes] + preflight lint [-m ] [--json] [--strict] +``` + +`coding` as the parent leaves room for `coding review ls|add` (the +`tasks:add-review-node` procedure) and `coding preflight ls` later. Both +subcommands take `-m/--memory` like every other group. + +## Check catalogue + +### `coding review lint` — over members only (Decision 1) + +| Rule | Severity | Catches | +|---|---|---| +| `parent-edge-exists` | error | check invisible to `tasks:review-changes` | +| `label-present` | error | the empty label (Decision 2) | +| `label-is-condition` | error | `child-of`, `applies-when`, `related`, bare `Applies when` | +| `target-resolves` | error | dangling / unreadable target (Decision 3) | +| `description-has-trigger` | warning | second blind spot in `find_nodes` output | +| `duplicate-trigger` | warning | cloned check never re-pointed | +| `seq-unique` | warning | non-deterministic sibling ordering | +| `foreign-toolchain` | warning | the misfiled `format-sources` | + +### `coding preflight lint` + +| Rule | Severity | +|---|---| +| `route-target-resolves` | error | +| `route-label-phrasing` | warning (Decision 4) | +| `route-target-retired` | warning | +| `route-target-moved-memory` | warning | + +### `--fix` scope + +Mechanical subset only: **promote the node's description sentence into the edge +label** when the description carries a trigger and the label doesn't. That +covers the empty-label and `child-of`-with-a-good-description cases. Anything +whose description has no condition either is reported for a human. Gated like +other bulk writes — prompt on a TTY, `--yes` non-interactively. + +## Package layout — `internal/cmd/coding/` + +| File | Contents | +|---|---| +| `coding.go` | group root; `-m` resolution; shared DTOs | +| `review_lint.go` | `newCmdReviewLint`; the pure rule engine over an in-memory model | +| `preflight_lint.go` | `newCmdPreflightLint` + its rule engine | +| `membership.go` | the Decision-1 predicate, isolated and unit-testable | +| `fix.go` | `--fix` planner + `updateEdge` application | + +Wired in [`internal/cmd/root.go`](../../internal/cmd/root.go) alongside the other +groups. Rule engines take plain structs and injected fetch functions, so they +unit-test without a server — the pattern `spec/lint.go` uses. + +## GraphQL changes + +**None.** Every operation needed already exists: `GetNode` (returns a node's +edges with ids, direction, name, loc — what `edge ls` uses), the node listing +(`loc`, `tags`, `isRunnable`, `seq`, `nodeType`), `NodeBatch` for bulk reads, and +`UpdateEdge` for `--fix`. No `make generate`, no schema refresh. + +## Output and exit contract + +- Mirror `lintFindingDTO` (`spec/lint.go`) — node loc, rule, severity, message — + as an explicit DTO in the command package, slices initialised to `[]T{}`. +- Render via `output.Write` with an `output.NewTable` human branch. +- **Exit `5` (`exitcode.Conflict`) via `exitcode.Silent`** when any error-severity + finding is present, matching `spec lint` (`lint.go:199`). The issue says + "non-zero"; the existing convention is specifically 5. Warnings alone exit 0 + unless `--strict`. +- Adding these leaves means updating the embedded + [`agentic-usage.md`](../../internal/cmd/agentic/agentic-usage.md) in the same + PR, or `TestAgenticUsageDocumentsEveryCommand` fails the build. +- **Paginate.** Per CLAUDE.md and #23 an unbounded `nodes` query silently returns + one page, so the memory sweep uses `scanAllNodes`/`paginateNodes`, not a single + call. + +## Tests + +- **Pure-logic unit tests** (`internal/cmd/coding/*_test.go`): the membership + predicate against the five node shapes in Decision 1; each label rule + (`child-of`, `applies-when`, empty, bare stem, valid); the `:rel:` loc as + corroboration but not a rule; duplicate-trigger and seq-uniqueness; the + toolchain heuristic both directions. +- **Command/wiring tests** (`internal/cmd/coding_cmd_test.go`, via + `testFactory` + `fakeGraphQL`/`captureGraphQL`): findings → exit 5; warnings + only → exit 0; `--strict` promotion; `--json` shape; an unreadable target + reported as `unavailable` rather than dropped; `--fix` asserted to issue + `UpdateEdge` per edge and **never** `UpdateNode` (the regression that would + destroy sibling edges); `--fix` requires `--yes` non-interactively. +- **Read-only live smoke test** against `micromentor.org::mmdata` and + `hadronmemory.com::hadron-portal`: expect 3 errors + the `format-sources` + warning on the former, clean on the latter. + +## Out of scope (follow-ups) + +- `coding review ls|add`, `coding preflight ls` — the surface leaves room. +- Coding-guidelines cross-link linting (mentioned in the issue's rationale, no + checks specified). +- `--fix` for anything beyond description→label promotion. +- Auto-repair of dangling targets; the linter reports, a human decides whether + the target moved or the edge is stale. +- CI workflow wiring for memory hygiene — `--json` + exit 5 make it possible; + choosing which repos gate on it is a separate call. From 93dc2409fe182bbbafd6993078296ba2b975dc5b Mon Sep 17 00:00:00 2001 From: Holger Selover-Stephan Date: Thu, 30 Jul 2026 17:17:17 +0200 Subject: [PATCH 2/2] docs(plans): fix the endpoint asymmetry and enumerate the smoke expectation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the coding-lint design doc: - Decision 3 named its rule `target-resolves` for both subcommands, but the two walk opposite ends of their edges. `review lint` reads `incomingEdges`, whose far endpoint is `source` (nodes.graphql:195-205; edge/ls.go:81-84 reads e.Source for the incoming direction) — the target is `review` itself and cannot dangle, so the rule as named checked the one safe endpoint. Split into `check-node-resolves` (review, source) and `route-target-resolves` (preflight, target). - Set the review-side rule to warning rather than error, and say why: when the endpoint can't be read, the Decision-1 membership predicate can't be evaluated on it either. The live instance, tasks:build-review-coverage, sits beside the readable tasks:review-changes that the predicate excludes as a runnable task — so it is probably a non-member, and erroring would fail a build over a node that shouldn't be linted. It is still reported as `unavailable`, never dropped. - The smoke expectation said "3 errors + the format-sources warning", silently dropping the dangling endpoint the same doc had just introduced. Replaced the count with an enumerated table of all five findings, since the bare count is what hid the omission. - Corrected `spec/lint.go` to internal/cmd/spec/lint.go (2×) and `find_nodes` to hadron_find_nodes. Co-Authored-By: Claude Opus 5 --- docs/plans/coding-command-group.md | 67 ++++++++++++++++++++++++------ 1 file changed, 55 insertions(+), 12 deletions(-) diff --git a/docs/plans/coding-command-group.md b/docs/plans/coding-command-group.md index 0d0922e..4ecf1de 100644 --- a/docs/plans/coding-command-group.md +++ b/docs/plans/coding-command-group.md @@ -118,14 +118,41 @@ edge, so the "one bad batch of 5" reading does not hold either. **Decision — check `name` for empty/missing (error). Drop the loc-shape rule; mention a `:rel:` loc in the finding message as corroboration only.** -### 3. New check: the edge target must resolve +### 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). -**Decision — add `target-resolves` as an error, and follow CLAUDE.md: a target -that lists but cannot be read is reported as `unavailable`, never silently -dropped.** +**The two subcommands look at opposite ends of their edges**, which the rule +names must reflect: + +- **`review lint` walks `incomingEdges`** on the `review` parent, so the far + endpoint is the edge's **`source`** — the check node + ([`nodes.graphql:195-205`](../../internal/api/queries/nodes.graphql), and + `edge/ls.go:81-84` reads `e.Source` for the incoming direction). The `target` + is `review` itself and is resolved by construction, so a rule named + `target-resolves` would be checking the one endpoint that cannot dangle. +- **`preflight lint` walks outgoing routes**, where the far endpoint genuinely + *is* the `target`. + +**Decision — two differently-named rules:** + +- `check-node-resolves` on the review side, at **warning** severity. +- `route-target-resolves` on the preflight side, at **error** severity — a route + to a dead node actively misroutes, and CLAUDE.md's own line is that stale + routing is worse than missing routing. + +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. + +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 +indeterminate rather than asserting a defect. ### 4. Preflight action-phrasing is a warning, not an error @@ -170,8 +197,8 @@ subcommands take `-m/--memory` like every other group. | `parent-edge-exists` | error | check invisible to `tasks:review-changes` | | `label-present` | error | the empty label (Decision 2) | | `label-is-condition` | error | `child-of`, `applies-when`, `related`, bare `Applies when` | -| `target-resolves` | error | dangling / unreadable target (Decision 3) | -| `description-has-trigger` | warning | second blind spot in `find_nodes` output | +| `check-node-resolves` | warning | dangling / unreadable check node — the edge's `source` (Decision 3) | +| `description-has-trigger` | warning | second blind spot in `hadron_find_nodes` output | | `duplicate-trigger` | warning | cloned check never re-pointed | | `seq-unique` | warning | non-deterministic sibling ordering | | `foreign-toolchain` | warning | the misfiled `format-sources` | @@ -205,7 +232,8 @@ other bulk writes — prompt on a TTY, `--yes` non-interactively. Wired in [`internal/cmd/root.go`](../../internal/cmd/root.go) alongside the other groups. Rule engines take plain structs and injected fetch functions, so they -unit-test without a server — the pattern `spec/lint.go` uses. +unit-test without a server — the pattern +[`internal/cmd/spec/lint.go`](../../internal/cmd/spec/lint.go) uses. ## GraphQL changes @@ -216,7 +244,8 @@ edges with ids, direction, name, loc — what `edge ls` uses), the node listing ## Output and exit contract -- Mirror `lintFindingDTO` (`spec/lint.go`) — node loc, rule, severity, message — +- Mirror `lintFindingDTO` ([`internal/cmd/spec/lint.go`](../../internal/cmd/spec/lint.go)) + — node loc, rule, severity, message — as an explicit DTO in the command package, slices initialised to `[]T{}`. - Render via `output.Write` with an `output.NewTable` human branch. - **Exit `5` (`exitcode.Conflict`) via `exitcode.Silent`** when any error-severity @@ -239,13 +268,27 @@ edges with ids, direction, name, loc — what `edge ls` uses), the node listing toolchain heuristic both directions. - **Command/wiring tests** (`internal/cmd/coding_cmd_test.go`, via `testFactory` + `fakeGraphQL`/`captureGraphQL`): findings → exit 5; warnings - only → exit 0; `--strict` promotion; `--json` shape; an unreadable target - reported as `unavailable` rather than dropped; `--fix` asserted to issue + only → exit 0; `--strict` promotion; `--json` shape; an unreadable endpoint + reported as `unavailable` rather than dropped; the review side reading the + edge's `source` and the preflight side its `target` (Decision 3 — a test that + would fail if both used the same endpoint); `--fix` asserted to issue `UpdateEdge` per edge and **never** `UpdateNode` (the regression that would destroy sibling edges); `--fix` requires `--yes` non-interactively. - **Read-only live smoke test** against `micromentor.org::mmdata` and - `hadronmemory.com::hadron-portal`: expect 3 errors + the `format-sources` - warning on the former, clean on the latter. + `hadronmemory.com::hadron-portal`. On mmdata expect exactly **3 errors and 2 + warnings**: + + | 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 | + | `tasks:build-review-coverage` | `check-node-resolves` | warning | + + Enumerated rather than counted, because a bare count is what let the earlier + draft of this doc omit the dangling endpoint entirely. The portal is expected + clean (0 findings across all 25 checks). ## Out of scope (follow-ups)