docs(plans): design doc for the hadron coding lint group (#325) - #326
Conversation
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 <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: 5eef9aec0a
ℹ️ 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".
| - **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. |
There was a problem hiding this comment.
The audit describes three label errors and separately adds the dangling tasks:build-review-coverage relationship as a target-resolves error, so the proposed live smoke test should expect four errors plus the format-sources warning. Leaving the expectation at three permits the new resolution check to be omitted or implemented against the wrong endpoint; because review relationships are incomingEdges, the unresolved remote endpoint is their source (internal/api/queries/nodes.graphql:195-205), not the already-resolved review target.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Both halves confirmed, and the endpoint point is the more valuable one — fixed in 93dc240.
Endpoint direction: you're right, and the rule name was wrong. Verified in the source rather than taking it on trust — incomingEdges selects source (nodes.graphql:195-205), and edge/ls.go:81-84 reads e.Source for the incoming direction. So on the review side the far endpoint is the check node, and the target is review itself, which cannot dangle. A rule named target-resolves was checking the one endpoint that is resolved by construction. Split into:
check-node-resolves— review side, the edge'ssourceroute-target-resolves— preflight side, where outgoing routes maketargetgenuinely correct
Smoke expectation: agreed it was incomplete, resolved differently than proposed. I made the review-side rule a warning, so it's 3 errors + 2 warnings rather than 4 errors. Reasoning: when the endpoint can't be read, the Decision-1 membership predicate can't be evaluated on it either — the linter doesn't know it's a checklist item. tasks:build-review-coverage sits beside tasks:review-changes, which is readable and is excluded as a runnable task, so it's most likely a non-member; erroring would fail a build over a node that shouldn't be linted at all. It's still reported as unavailable per CLAUDE.md, just as an indeterminate-membership warning rather than an asserted defect. Happy to reconsider if you think a dangling edge is worth failing on regardless of what it points at.
I also replaced the bare count with an enumerated table of all five expected findings — the count is precisely what let the omission slip through, so counting again seemed like the wrong fix.
There was a problem hiding this comment.
Pull request overview
Adds a design-ahead implementation plan for the proposed hadron coding command group (refs #325), focusing on linting the review checklist tree and preflight router with rules calibrated against observed live-memory data.
Changes:
- Introduces a new
docs/plans/design doc defining the proposedcoding review lintandcoding preflight lintsurfaces. - Records five key design decisions (membership predicate, label-empty vs
:rel:heuristic, target-resolves check, warning/error split, and--fiximplementation viaupdateEdge). - Documents expected output/exit-code conventions and test strategy aligned with existing
spec lintpatterns.
Comments suppressed due to low confidence (1)
docs/plans/coding-command-group.md:219
- This parenthetical looks like a file path but
spec/lint.godoesn't exist in the repo (the file isinternal/cmd/spec/lint.go). Updating the reference avoids confusion for implementers.
- Mirror `lintFindingDTO` (`spec/lint.go`) — node loc, rule, severity, message —
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | `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 | |
There was a problem hiding this comment.
Fixed in 93dc240 — now hadron_find_nodes, matching CLAUDE.md and the tool's registered name.
|
|
||
| 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. |
There was a problem hiding this comment.
Fixed in 93dc240 — corrected to internal/cmd/spec/lint.go in both places, and made them relative links (../../internal/cmd/spec/lint.go) so they resolve from docs/plans/.
Worth noting the existing plan docs get this wrong too — spec-command-group.md links [internal/cmd/root.go](internal/cmd/root.go), which 404s from that directory. Not fixing those here to keep this PR to one change, but it's a real papercut across docs/plans/.
…tation 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 <noreply@anthropic.com>
Design-ahead plan doc for #325, per the CLAUDE.md convention that substantial features get a
docs/plans/doc bundled for review. Nothing is implemented — the doc exists to settle the open questions before code, because the surface as filed produces a >50% false-positive rate.Refs #325. My audit notes are in this comment; this turns them into a reviewable design.
The five decisions that need your review
1. Membership rule — the load-bearing one. The issue scopes to "every node under the
reviewparent", but that parent legitimately has non-checklist neighbours: the router, the tasks that consume the tree, a meta backlog, pattern nodes. I implemented its checks verbatim and ran them:reviewtag, nometatag, notisRunnable)A loc-prefix rule doesn't work either —
review:backlogsits underreview:and is explicitly meta. The node metadata separates them cleanly; the doc has the table.2. "Label isn't a loc" checks the wrong field.
review:input-type-graphql-type:rel:reviewis that edge'sloc, not itsname— the name is empty. Edge locs are slugified from names, so:rel:is the derived fallback when the name is empty: a symptom, not a defect class. There's exactly 1, not the batch of 5 implied. Replaced with an empty-label check.3. New check for a dangling endpoint — on opposite ends per subcommand. The live
reviewtree has an edge totasks:build-review-coverage, which is absent from the node listing and unreadable (node get→ exit 4). Note the two subcommands look at opposite ends:review lintwalksincomingEdges, so the far endpoint is thesource;preflight lintwalks outgoing routes, where it is thetarget. Hencecheck-node-resolves(warning — an unreadable node cannot be tested against the membership predicate, so membership is indeterminate) androute-target-resolves(error — a route to a dead node actively misroutes). Either way reported asunavailable, never dropped.4. Preflight action-phrasing is a warning. 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 lintalready treats convention violations as warnings.5.⚠️ hazard in the issue is MCP-side. This repo already has
--fixneeds no server work. Thehadron edge updateoverupdateEdge(edgeRef:)with per-field omitempty — one edge, one field, sibling edges untouched. hadron-server#835 is not a dependency.Also recorded
Repo constraints whoever implements it will hit:
TestAgenticUsageDocumentsEveryCommandgates new leaves on the embeddedagentic-usage.md;spec lintexits 5 (exitcode.Silent(exitcode.Conflict)), not 1; the #23 pagination requirement. And no GraphQL changes are needed —GetNode, the node listing,NodeBatch, andUpdateEdgeall already exist.Note on the motivation
The portal's "7 of 24" doesn't reproduce — 25 inbound edges, all carrying real conditions, 0 suspect. It looks fixed between the audit and filing. The mmdata four are all confirmed real. Worth weighing before sizing, since the portal case was half the stated motivation.
Docs-only; one new file, no code.
🤖 Generated with Claude Code