feat(coding): hadron coding review|preflight lint (#325) - #328
Conversation
Lints the coding-workflow graph — the review:* checklist tree and the preflight router — which are executable infrastructure: tasks:review-changes triages checks by reading each one's "Applies when …" edge label, so a label that isn't a condition makes the check silently stop firing. hadron coding review lint -m <memory> [--root] [--toolchain] [--strict] [--fix [--yes]] [--json] hadron coding preflight lint -m <memory> [--root] [--strict] [--json] Implements docs/plans/coding-command-group.md. Errors exit 5 via exitcode.Silent(Conflict), matching spec lint; warnings alone exit 0 unless --strict. --fix relabels via updateEdge only, never updateNode(edges:), which would replace a node's whole outgoing edge set and destroy sibling edges. The plan's Decision 1 did not survive contact with the data and changed twice (recorded under Deviations): - Its tag-only membership rule silently ignored 17 of mmdata's 48 checks, which carry no tags at all — including review:format-sources, the misfiled-toolchain check that motivated the command. The original validation measured only false positives, so it never surfaced. - The obvious repair (tag OR prefix) then admitted a findings node — a resolved incident writeup tagged `review` — as a member missing its parent edge. Across mmdata and hadron-portal, every `review`-tagged node outside the review: prefix is a finding and every real check is inside it, so the tag was dropped entirely. Membership is now the lint root's child prefix, minus meta, minus runnable, and the prefix follows --root. Also deviating from the plan: description-has-trigger became description-present (real descriptions state the rule, not the condition, so requiring a trigger would flag nearly every check); duplicate-trigger ignores already-broken labels so a child-of pair isn't reported twice; foreign-toolchain infers over each member's loc/name/description/trigger because triggers alone tie 1-1 on mmdata, and stays silent when no family wins. Fixes a bug the plan would have shipped: CanonicalMemoryRef emits the flat hrn:mem:<root>:<slug> form, so memURN+"::"+loc builds refs that resolve to nothing. Node refs go through cmdutil.NodeURN, and unavailable refs map back to bare locs via a ref→loc map rather than a prefix trim. Read-only live runs: mmdata 3 errors + 6 warnings (all true positives), hadron-portal clean at 24 checks, preflight 25 warnings and exit 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 113b9a8c96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| locs = append(locs, r.Other) | ||
| } | ||
| sort.Strings(locs) | ||
| targets, unavailable, err := fetchNodes(ctx, client, mem, locs) |
There was a problem hiding this comment.
For any valid preflight edge whose target lives in another memory, this call reconstructs every target reference using the router's mem, so nodeBatch looks for the target loc in the wrong memory. Cross-memory edges are explicitly supported by internal/cmd/edge/add.go, and this linter even defines a moved-memory warning, but such a route is instead reported as unresolvable (exit 5), or—if the home memory happens to contain the same loc—the wrong node is linted. Preserve and batch-read the target IDs already returned by GetNode rather than rebuilding home-memory URNs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in bdbd43e — and this one was worth more than it looked.
You're right on the mechanism: fetchNodes rebuilt a URN per target from the router's memory, so a cross-memory route was looked up in the wrong place. Endpoints are now batch-read by the node ids GetNode already returns, for both subcommands. Added a command test asserting the NodeBatch refs are ids and never hrn:-prefixed.
The part I didn't expect: this bug had been manufacturing a finding I documented as real. The plan doc cited tasks:build-review-coverage as a live instance of CLAUDE.md's list-vs-read visibility gap. It isn't — it resolves fine by id:
$ hadron api 'query($r:[ID!]){ nodeBatch(refs:$r){ unavailable nodes{ loc tags } } }' \
-F r='["019eedd0a981749fa5eff355efa2bf47"]'
unavailable: []
loc: tasks:build-review-coverage tags: [meta contributing review coverage …]
It's tagged meta, sits outside the review: prefix, and is correctly excluded as a non-check. Reading by id made the warning vanish. I've retracted that claim in the plan doc (Deviations §7) and dropped the mmdata expectation to 3 errors + 5 warnings. check-node-resolves stays — the gap is real and an unreadable endpoint is possible — but it now has no known live instance, which is a more honest description than the one I shipped.
| for _, r := range routes { | ||
| if r.Other == "" || seen[r.Other] { | ||
| continue |
There was a problem hiding this comment.
When GraphQL redacts an edge target because its memory is unreadable, fetchRootEdges leaves Other empty and this branch silently drops the route. Consequently route-target-resolves—the command's only error rule—never reports exactly this unreadable-target case, and the command can claim the router is clean. Retain an edge-level identifier/placeholder for null targets and emit the unresolved-target finding instead of skipping it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct — fixed in bdbd43e.
With Target nil, Other was "" and the loop's if r.Other == "" { continue } dropped the route entirely, so route-target-resolves — the command's only error rule — was blind to precisely the unreadable-target case it exists for, and the command could report a clean router.
Redacted endpoints are now reported as route-target-resolves errors, identified by the route's label ((unreadable target of "to reach the hidden thing")) since there's no loc to name them by. TestRouteWithRedactedTargetIsReported covers it.
| func sameMemory(edgeMemoryID, scope string) bool { | ||
| if edgeMemoryID == scope { | ||
| return true | ||
| } | ||
| // A PK-shaped id can't be compared to a URN-shaped scope; don't guess. | ||
| return !strings.Contains(scope, "::") || !strings.Contains(edgeMemoryID, "::") |
There was a problem hiding this comment.
home is assigned mem.Ref, which CanonicalMemoryRef emits as flat hrn:mem:<root>:<slug> and therefore never contains ::. For every unequal edge memory ID, this return consequently evaluates to true via its first disjunct, making route-target-moved-memory unreachable in real command execution; the unit test only passes because it supplies the legacy org::mem spelling directly. Capture the root node's actual MemoryId and compare it with the endpoint's MemoryId.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed dead, fixed in bdbd43e. I verified it before changing anything:
sameMemory("019f76f283c27bc39c7f906c798e4268", "hrn:mem:acme.com:kb") = true
sameMemory("other.org::elsewhere", "hrn:mem:acme.com:kb") = true
mem.Ref never contains ::, so the don't-guess-across-spellings guard's first disjunct matched every time and the rule could not fire in a real run. As you note, the test passed only because it fed the legacy org::mem spelling by hand — my own fixture was hiding it.
fetchRootEdges now also returns the root node's MemoryId, and sameMemory compares that against the endpoint's MemoryId — both from the same projection, so directly comparable, and the spelling heuristic is gone. The test uses PK-shaped ids on both sides and covers moved / same / absent.
There was a problem hiding this comment.
Pull request overview
Adds a new hadron coding command group to lint “executable infrastructure” in Hadron memories: the review:* checklist tree and the preflight router. This fits alongside existing hygiene commands like spec lint, with deterministic --json output and exit-code conventions intended for CI/agent use.
Changes:
- Adds
hadron coding review lintwith rule-engine + optional--fixthat relabels edges viaupdateEdge(notupdateNode(edges:)). - Adds
hadron coding preflight lintto validate outgoing router routes and conventions. - Updates the agentic usage surface and the plan doc with “as-built” deviations and verification notes.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/cmd/root.go | Wires the new top-level coding command into the CLI root. |
| internal/cmd/coding/coding.go | Core coding command group, shared DTOs/models, GraphQL fetch helpers, and listing scan helper. |
| internal/cmd/coding/review_lint.go | Implements coding review lint rules, output, strict/exit semantics, and --fix integration. |
| internal/cmd/coding/review_lint_test.go | Unit tests for review lint rule engine behavior and determinism. |
| internal/cmd/coding/preflight_lint.go | Implements coding preflight lint rules, output, and strict/exit semantics. |
| internal/cmd/coding/preflight_lint_test.go | Unit tests for preflight lint rule engine behavior. |
| internal/cmd/coding/membership.go | Membership predicate for what counts as a checklist item under a lint root. |
| internal/cmd/coding/membership_test.go | Tests membership predicate and listing-vs-full predicate agreement. |
| internal/cmd/coding/fix.go | Planning and applying mechanical edge-label fixes via updateEdge. |
| internal/cmd/coding/fix_test.go | Tests trigger extraction and fix planning determinism/safety. |
| internal/cmd/coding_cmd_test.go | Command-level tests covering exit codes, JSON output, endpoint directionality, and --fix safety (UpdateEdge-only). |
| internal/cmd/agentic/agentic-usage.md | Documents the new hadron coding … command surface for completeness gating. |
| docs/plans/coding-command-group.md | Updates the design doc to “implemented” and records deviations/verification as-built. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Two sources of candidate checks: nodes carrying the review tag or | ||
| // loc 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 := scanTagged(ctx, client, mem, nil) |
There was a problem hiding this comment.
Right, fixed in bdbd43e. That was a leftover from when membership was tag-based: scanTagged took a tag filter, and once membership became the loc prefix I left the call passing nil — which lists every node in the memory (369 on mmdata, over two pages).
It's now scanPrefix, scoped by NodeFilter.LocPrefix to the lint root's child prefix, derived from --root. Anything outside that prefix can't be a checklist item, so there's no reason to fetch it. Renamed the helper too, since "tagged" no longer describes what it does.
| // The router's own memory, to spot a route that left it. | ||
| home = mem.Ref | ||
|
|
There was a problem hiding this comment.
Confirmed and fixed in bdbd43e — same root cause Codex flagged, replied there in detail.
Short version: mem.Ref is hrn:mem:<root>:<slug>, which contains no ::, so sameMemory's guard returned true for every route and the rule was unreachable in real runs. It now compares the endpoint's MemoryId against the root node's MemoryId — both from the same GraphQL projection — and the test uses PK-shaped ids on both sides instead of the org::mem spelling that was masking it.
…ead moved-memory rule Review follow-ups on #328. Three of the five were real bugs in the shipped implementation, and one of them had been manufacturing a finding I documented as a genuine defect. - **Cross-memory routes resolved against the wrong memory.** fetchNodes rebuilt a URN per target from the ROUTER's memory, so a route that legitimately crosses into another memory (edge/add.go supports these, and this linter even warns about them) was reported unresolvable — or, if the home memory happened to hold the same loc, the wrong node was linted. Endpoints are now batch-read by the node ids GetNode already returns. - **route-target-moved-memory could never fire.** `home` was mem.Ref, which CanonicalMemoryRef emits as flat hrn:mem:<root>:<slug> — no "::" — so sameMemory's don't-guess-across-spellings guard matched every time. Verified dead: sameMemory(<any pk>, "hrn:mem:acme.com:kb") == true. Its test passed only because it supplied the legacy org::mem spelling by hand. Now compares the endpoint's MemoryId against the ROOT NODE's MemoryId — both from the same projection — and the test uses PK-shaped ids on both sides. - **Routes with a redacted target projection were silently dropped.** With Target nil, Other was "" and the loop skipped it, so route-target-resolves — the command's only error rule — was blind to exactly the unreadable-target case, and the command could report a clean router. Now reported, identified by the route's label. - **The listing sweep paginated the entire memory.** A leftover from tag-based membership: scanTagged passed no filter. Membership is the root's child prefix, so the sweep is now scoped by LocPrefix (scanPrefix). - Retracts a claim in the plan doc. `tasks:build-review-coverage` was documented as a live instance of the list-vs-read visibility gap; it is fully readable by id (nodeBatch returns it, tagged meta, outside the review: prefix) and was only ever "unreadable" because of the URN-rebuilding bug above. Fixing that made the finding disappear. check-node-resolves is kept — the gap is real and an unreadable endpoint is possible — but it now has no known live instance. Deviations §7 records this; the mmdata expectation drops to 3 errors + 5 warnings. Live re-run: mmdata 3 errors + 5 warnings, portal clean at 24 checks, preflight 25 warnings and exit 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Implements #325 per the design in docs/plans/coding-command-group.md (merged in #326), which this PR updates with the as-built deviations.
Errors exit 5 via
exitcode.Silent(Conflict), matchingspec lint; warnings alone exit 0 unless--strict.Decision 1 changed twice — the plan's membership rule was wrong
The merged plan defined a checklist item as a node tagged
review, validated as "3 findings, 0 false positives". That validation only ever measured false positives. Building it surfaced two failures in the other direction:The tag-only rule silently ignored 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 the whole command. The shipped design would never have linted it.The obvious repair (
tag OR prefix) then produced a false positive on the first live run —findings:nightly-recommendation-search-idempotency-pre-enqueue-race, a resolved incident writeup carryingreviewamong six tags, reported as a check missing its parent edge.Checking both memories settled it:
review:review-tagged outside itmicromentor.org::mmdatahadronmemory.com::hadron-portalEvery real check is inside the prefix; every tagged node outside it is a finding. The tag was dropped entirely. Membership is the lint root's child prefix, minus
meta, minus runnable — and the prefix derives from--root, so it follows a differently-named parent.Other deviations (all in the plan doc)
description-has-trigger→description-present. Real descriptions state the rule ("Resolver field must be thin…"), not the condition, so requiring a trigger would flag nearly every check.duplicate-triggerignores already-broken labels, or mmdata's twochild-oflabels get reported twice for one defect.foreign-toolchaininfers over loc+name+description+trigger — triggers alone tie 1-1 on mmdata and would have missedformat-sources— and stays silent when no family wins.--toolchainoverrides,-disables.A bug the plan would have shipped
cmdutil.CanonicalMemoryRefemits the flathrn:mem:<root>:<slug>form, so the plan's impliedmemURN + "::" + locbuilds node refs that resolve to nothing. Node refs now go throughcmdutil.NodeURN, and unavailable refs map back to bare locs via a ref→loc map rather than a prefix trim (which would depend on URN spelling). Caught by a command test, covered by one.--fixsafety--fixpromotes a check's description into a broken edge label, and only when the description already spells the trigger — it never invents a condition. Every write is a single-edgeupdateEdge; a test assertsUpdateNodeis never called, sinceupdateNode(edges:)replaces the whole outgoing edge set and would destroy siblingdocumented-by/relates-toedges (the hazard in #325). Prompts on a TTY, requires--yesotherwise.Verification
go test ./...— 16 packages green.make lint— 0 issues. Theagentic-usage.mdcompleteness gate caught the new group exactly as the plan predicted; surface line added.Read-only live runs (no writes;
--fixnot exercised against a live memory):All eight mmdata findings are true positives — the 3 label errors from the original audit, plus
format-sources(foreign-toolchain), a genuine cloned trigger (×2), and a sharedseq(×2). Zero false positives. The portal result matches the audit prediction exactly.Retraction, from review: the plan doc originally cited
tasks:build-review-coverageas a live dangling endpoint. It is fully readable by id — the "unreadable" verdict was produced by a URN-rebuilding bug in my own code, which review caught. Fixing it made that warning disappear, so the expectation is now 3 errors + 5 warnings andcheck-node-resolvesis a rule with no known live instance. Recorded as Deviations §7.The preflight run exiting 0 with 25 warnings is Decision 4 working as intended: a third of the router failing a labelling convention doesn't turn the build red.
🤖 Generated with Claude Code