fix(walk): reject a non-boundary before making the request - #83
Merged
Conversation
`walk <behavior> --boundary` returned a bare `service error (404)` instead of naming what the node actually is. The guard added in #81 lives inside `render_boundary_scoped`, which only runs on the RESPONSE — and the server 404s a non-boundary id, so the check could never fire. The whole-graph fallback still gave the good message, so the quality of the error depended on whether a local index happened to exist, which is what #81 set out to fix. The kind is already in the pulled index (`node_info`), so the check moves ahead of the request. `hydrate walk cachetools.Cache.clear --boundary` now says it is a behavior and points at the neighborhood read. Found by running the released binary against a real project — the tests call `render_boundary_scoped` directly, so they never reach the dispatch or the server's ordering. The new test asserts NO request is made, which is the only way to prove the guard preempts rather than trails. Mutation-verified: restoring the shipped shape (no local kind lookup) fails it. Unknown kind, or no index, still defers to the server rather than guessing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four-agent review of #83. No P0. The guard itself was upheld — the CLI reviewer argued both sides of "server is the sole authority for validation" and landed on legitimate input guidance: no spec rule is mirrored, a node's kind is data rather than a rule, and both the whole-graph path and `boundary flatten` already do this same local check. THE MESSAGE STATED A SNAPSHOT AS FACT (all four agents). The kind comes from an index of unknown age, and kind is MUTABLE over the wire (UpdateNodeDataDelta.after carries it), so a node that was a behavior at pull time may be a boundary now. The guard would refuse a request the server would have served while asserting something false, with no remedy named and no way past it. It now attributes the claim and names both fixes: "…this working copy's index has it as a behavior. Run `hydrate walk X` for its neighborhood, or `hydrate pull` if the index is behind." That is the register `fallback_note(PathNotInIndex)` already uses for the same hazard. AN UNRECOGNISED KIND NOW DEFERS INSTEAD OF REJECTING. `kind != "boundary"` refused any token this build didn't know, so an index written by a newer CLI would block a legal request with no override — the opposite of the posture `unaddressable_label` states ten lines away for an unrecognised reason. Only a RECOGNISED non-boundary rejects locally. AN INDEX WITH NO KIND WAS SILENT. `node_info` is #[serde(default)] precisely so an older pull still loads, and in that state the local check silently did nothing and the request 404'd as before. The two existing `node_info` consumers both fail loud with a pull hint — `flatten_boundary` asks this very question — so this was the third consumer and the first silent one. It now says the check was skipped and why. ONE INDEX LOAD, NOT TWO. `plan` already had the index open; `node_kind` re-read and re-parsed the same file. Beyond the wasted I/O the two facts the guard combines (path->id, id->kind) could come from different snapshots if a `pull` interleaved. The kind now travels with the plan. Also: one message builder instead of three verbatim copies (that drift is exactly the scoped-vs-fallback divergence this work exists to remove); the kind is sanitized before reaching a terminal; the renderer's check is documented honestly as defence-in-depth against the /boundary route's contract rather than claimed to be unreachable; the guard test pins the exit code and that the error goes to stderr, and pins the contract (problem, remedy, staleness hint) rather than the phrasing; `--depth` and the `--boundary` failure are finally documented in the README and `hydrate guide`, two PRs late. 397 unit + 7 integration tests. Verified with the locally built binary against a real project before this was pushed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merged
rennehan
added a commit
that referenced
this pull request
Jul 29, 2026
* fix(walk): reject a non-boundary before making the request `walk <behavior> --boundary` returned a bare `service error (404)` instead of naming what the node actually is. The guard added in #81 lives inside `render_boundary_scoped`, which only runs on the RESPONSE — and the server 404s a non-boundary id, so the check could never fire. The whole-graph fallback still gave the good message, so the quality of the error depended on whether a local index happened to exist, which is what #81 set out to fix. The kind is already in the pulled index (`node_info`), so the check moves ahead of the request. `hydrate walk cachetools.Cache.clear --boundary` now says it is a behavior and points at the neighborhood read. Found by running the released binary against a real project — the tests call `render_boundary_scoped` directly, so they never reach the dispatch or the server's ordering. The new test asserts NO request is made, which is the only way to prove the guard preempts rather than trails. Mutation-verified: restoring the shipped shape (no local kind lookup) fails it. Unknown kind, or no index, still defers to the server rather than guessing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(walk): review findings on the boundary preflight Four-agent review of #83. No P0. The guard itself was upheld — the CLI reviewer argued both sides of "server is the sole authority for validation" and landed on legitimate input guidance: no spec rule is mirrored, a node's kind is data rather than a rule, and both the whole-graph path and `boundary flatten` already do this same local check. THE MESSAGE STATED A SNAPSHOT AS FACT (all four agents). The kind comes from an index of unknown age, and kind is MUTABLE over the wire (UpdateNodeDataDelta.after carries it), so a node that was a behavior at pull time may be a boundary now. The guard would refuse a request the server would have served while asserting something false, with no remedy named and no way past it. It now attributes the claim and names both fixes: "…this working copy's index has it as a behavior. Run `hydrate walk X` for its neighborhood, or `hydrate pull` if the index is behind." That is the register `fallback_note(PathNotInIndex)` already uses for the same hazard. AN UNRECOGNISED KIND NOW DEFERS INSTEAD OF REJECTING. `kind != "boundary"` refused any token this build didn't know, so an index written by a newer CLI would block a legal request with no override — the opposite of the posture `unaddressable_label` states ten lines away for an unrecognised reason. Only a RECOGNISED non-boundary rejects locally. AN INDEX WITH NO KIND WAS SILENT. `node_info` is #[serde(default)] precisely so an older pull still loads, and in that state the local check silently did nothing and the request 404'd as before. The two existing `node_info` consumers both fail loud with a pull hint — `flatten_boundary` asks this very question — so this was the third consumer and the first silent one. It now says the check was skipped and why. ONE INDEX LOAD, NOT TWO. `plan` already had the index open; `node_kind` re-read and re-parsed the same file. Beyond the wasted I/O the two facts the guard combines (path->id, id->kind) could come from different snapshots if a `pull` interleaved. The kind now travels with the plan. Also: one message builder instead of three verbatim copies (that drift is exactly the scoped-vs-fallback divergence this work exists to remove); the kind is sanitized before reaching a terminal; the renderer's check is documented honestly as defence-in-depth against the /boundary route's contract rather than claimed to be unreachable; the guard test pins the exit code and that the error goes to stderr, and pins the contract (problem, remedy, staleness hint) rather than the phrasing; `--depth` and the `--boundary` failure are finally documented in the README and `hydrate guide`, two PRs late. 397 unit + 7 integration tests. Verified with the locally built binary against a real project before this was pushed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
walk <behavior> --boundaryreturns a bareservice error (404)instead of naming what the node actually is.The guard added in #81 lives inside
render_boundary_scoped, which only runs on the response — and the server 404s a non-boundary id, so the check can never fire. The whole-graph fallback still gives the good message, so the quality of the error depends on whether a local index happens to exist — which is precisely what #81 set out to fix.Before:
After:
The kind is already in the pulled index (
node_info), so the check moves ahead of the request.How it was found
By running the released binary against a real project. The unit tests call
render_boundary_scopeddirectly, so they never reach the dispatch or the server's ordering — the guard looked correct in code and in tests and did nothing in practice.That is the third time in this line of work that a fix passed its tests and failed against the real thing, and the first one to reach a published release. Building the release binary and exercising the error paths locally before tagging is now the practice.
Test
The new test asserts no request is made — the only way to prove the guard preempts rather than trails. Mutation-verified: restoring the shipped shape (no local kind lookup) fails it.
Unknown kind, or no index, still defers to the server rather than guessing.
394 unit + 5 integration tests;
fmt,clippy -D warningsclean.