feat(scoped-reads): walk and show --depth fetch only the slice - #81
Merged
Conversation
Before this, every "scoped" read fetched the WHOLE branch graph and sliced it
locally: `walk`, `walk --boundary` and `show <path>` all called
fetch_branch_graph. Scoped was true of the output and false of the request, so
reading one node of a 10,000-node project pulled all 10,000 — the context
problem the reads exist to solve, unsolved.
They now call the branch-addressed endpoints:
walk <path> -> GET /v1/branches/{id}/node/{id}
walk <path> --boundary -> GET /v1/branches/{id}/boundary/{id}
show <path> --depth N -> GET /v1/branches/{id}/subtree/{id}?depth=N
Branch-addressed on purpose: the /v1/graph/{project_id}/... twins resolve the
project's MAIN branch, and edits happen on working branches, so those cannot
serve an authoring client at all.
Paths come from the server. A scoped read returns a slice, and a dotted path is
built from ancestors the slice does not contain, so local reconstruction fails
with "references a missing parent" — which is exactly what `show --depth` did
against a real project until this change. It was invisible in unit tests
because their fixtures include the ancestors.
The path map is deliberately NOT total: the server returns nodes it could not
address (an unnamed node is legal while designing) and reports why in
`unaddressable`. So nothing indexes it. `label_of` renders the reason instead —
"<unnamed — give it a name to address it>" — and both output modes carry the
map so a consumer can act. Indexing was the pre-existing shape and would panic
on an ordinary graph; a test pins that, and reverting to indexing fails three.
Resolution needs the node's id, which the pulled index supplies. Without one
(no working copy, never pulled, stale path) the whole-graph read still runs and
says so — asking for a slice must never deny you your graph.
build_view takes optional server paths rather than always reconstructing, and
skips ports of an unaddressable node instead of indexing its path.
380 tests. Verified against a real project: all three reads scoped, validate
and status unaffected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…spatch Five-agent review of #81. Four P0s; this reworks rather than patches. NOTHING TESTED THAT THE SCOPED PATH WAS TAKEN. Mutation: make `walk` always fall through to the whole-graph read — 380 passed. Mutation: read the wrong branch — 380 passed. The PR's entire claim could be reverted with CI green, and the branch-addressing invariant the server work was built around was unguarded. The decision now lives in `scoped::plan`, unit-tested on its own, and a new integration test drives the real binary against a local listener and asserts the request line is /v1/branches/{bound}/node/{id} and that /graph is never hit. Both mutations now fail. (No new dependency: one request line does not justify a mock-server crate.) `show --depth` PANICKED on an unaddressable node. The guard added for the port loop was defeated 25 lines later, where the code indexed the same non-total map. The contract is explicit that a returned node may be in `unaddressable` instead of `paths` — an unnamed node is legal while designing. Skipping was the wrong remedy (its ports then vanish from the port table and the next edge lookup fails blaming the server); the node is now LABELLED, as `walk` does. `show --depth` FAILED on any slice with an edge leaving it. Cross-boundary edges were merged into `edges` and pushed through the port resolver, but by definition one endpoint is outside the slice, so it errored — accusing the server of corruption when the server did exactly what its contract says. Every real subtree with a dependency hit this. The server already classified those edges; take its count. `unaddressable` LEAKED RAW NODE IDS in JSON, violating the no-ids decision, in the very map meant to explain unaddressability — and a raw id joins to nothing else in the payload. Both verbs now emit the label the human sees plus the reason. The show-side fixture could not reach any of it: a single parentless node, no edges, a total path map. Local reconstruction succeeds on that, so the server-path branch was never load-bearing. Replaced with a real slice — a root whose own ancestor is outside the payload, a child, and a crossing edge. Also: - `show`'s scoped read is gated on the resolved branch BEING the bound branch. The index records no branch identity, so `--branch other` could resolve an id from the wrong branch and return a different node under the name typed. - `--depth` without PATH was silently ignored, fetching the whole branch while the user believed they had bounded it. Now `requires = "path"`, and the server's 1..=32 range is checked locally instead of costing a round trip. - `walk --boundary` regained the "not a boundary" guidance on the scoped path; the quality of that error no longer depends on whether an index exists. - The fallback note names its ACTUAL cause (no working copy / no index / path not in index / not the bound branch). Telling someone to `hydrate pull` when they mistyped a path sends them to the wrong fix. - `show` no longer keeps private copies of the shared plumbing — the two fallback messages had already drifted in one PR. - A non-uuid key in `paths` fails loud instead of silently becoming "no path". - Server strings are stripped of control characters before reaching a terminal. Names have no charset validation server-side, and the realistic source is an LLM naming nodes from imported content, not a hostile collaborator. - `show --depth` reports `scoped`/`root` so a scoped read is distinguishable from a whole-graph fetch on stdout, and surfaces `unaddressable` in both modes as `walk` does. 394 unit + 3 integration tests. Verified live: a slice with a crossing edge now renders, depth bounds 96 nodes to 15, and truncated flips exactly at the cut. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Jul 28, 2026
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>
rennehan
added a commit
that referenced
this pull request
Jul 29, 2026
* feat(scoped-reads): walk and show --depth fetch only the slice
Before this, every "scoped" read fetched the WHOLE branch graph and sliced it
locally: `walk`, `walk --boundary` and `show <path>` all called
fetch_branch_graph. Scoped was true of the output and false of the request, so
reading one node of a 10,000-node project pulled all 10,000 — the context
problem the reads exist to solve, unsolved.
They now call the branch-addressed endpoints:
walk <path> -> GET /v1/branches/{id}/node/{id}
walk <path> --boundary -> GET /v1/branches/{id}/boundary/{id}
show <path> --depth N -> GET /v1/branches/{id}/subtree/{id}?depth=N
Branch-addressed on purpose: the /v1/graph/{project_id}/... twins resolve the
project's MAIN branch, and edits happen on working branches, so those cannot
serve an authoring client at all.
Paths come from the server. A scoped read returns a slice, and a dotted path is
built from ancestors the slice does not contain, so local reconstruction fails
with "references a missing parent" — which is exactly what `show --depth` did
against a real project until this change. It was invisible in unit tests
because their fixtures include the ancestors.
The path map is deliberately NOT total: the server returns nodes it could not
address (an unnamed node is legal while designing) and reports why in
`unaddressable`. So nothing indexes it. `label_of` renders the reason instead —
"<unnamed — give it a name to address it>" — and both output modes carry the
map so a consumer can act. Indexing was the pre-existing shape and would panic
on an ordinary graph; a test pins that, and reverting to indexing fails three.
Resolution needs the node's id, which the pulled index supplies. Without one
(no working copy, never pulled, stale path) the whole-graph read still runs and
says so — asking for a slice must never deny you your graph.
build_view takes optional server paths rather than always reconstructing, and
skips ports of an unaddressable node instead of indexing its path.
380 tests. Verified against a real project: all three reads scoped, validate
and status unaffected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(scoped-reads): review findings — three crashes and an untested dispatch
Five-agent review of #81. Four P0s; this reworks rather than patches.
NOTHING TESTED THAT THE SCOPED PATH WAS TAKEN. Mutation: make `walk` always
fall through to the whole-graph read — 380 passed. Mutation: read the wrong
branch — 380 passed. The PR's entire claim could be reverted with CI green,
and the branch-addressing invariant the server work was built around was
unguarded. The decision now lives in `scoped::plan`, unit-tested on its own,
and a new integration test drives the real binary against a local listener and
asserts the request line is /v1/branches/{bound}/node/{id} and that /graph is
never hit. Both mutations now fail. (No new dependency: one request line does
not justify a mock-server crate.)
`show --depth` PANICKED on an unaddressable node. The guard added for the port
loop was defeated 25 lines later, where the code indexed the same non-total
map. The contract is explicit that a returned node may be in `unaddressable`
instead of `paths` — an unnamed node is legal while designing. Skipping was the
wrong remedy (its ports then vanish from the port table and the next edge
lookup fails blaming the server); the node is now LABELLED, as `walk` does.
`show --depth` FAILED on any slice with an edge leaving it. Cross-boundary
edges were merged into `edges` and pushed through the port resolver, but by
definition one endpoint is outside the slice, so it errored — accusing the
server of corruption when the server did exactly what its contract says. Every
real subtree with a dependency hit this. The server already classified those
edges; take its count.
`unaddressable` LEAKED RAW NODE IDS in JSON, violating the no-ids decision, in
the very map meant to explain unaddressability — and a raw id joins to nothing
else in the payload. Both verbs now emit the label the human sees plus the
reason.
The show-side fixture could not reach any of it: a single parentless node, no
edges, a total path map. Local reconstruction succeeds on that, so the
server-path branch was never load-bearing. Replaced with a real slice — a root
whose own ancestor is outside the payload, a child, and a crossing edge.
Also:
- `show`'s scoped read is gated on the resolved branch BEING the bound branch.
The index records no branch identity, so `--branch other` could resolve an id
from the wrong branch and return a different node under the name typed.
- `--depth` without PATH was silently ignored, fetching the whole branch while
the user believed they had bounded it. Now `requires = "path"`, and the
server's 1..=32 range is checked locally instead of costing a round trip.
- `walk --boundary` regained the "not a boundary" guidance on the scoped path;
the quality of that error no longer depends on whether an index exists.
- The fallback note names its ACTUAL cause (no working copy / no index / path
not in index / not the bound branch). Telling someone to `hydrate pull` when
they mistyped a path sends them to the wrong fix.
- `show` no longer keeps private copies of the shared plumbing — the two
fallback messages had already drifted in one PR.
- A non-uuid key in `paths` fails loud instead of silently becoming "no path".
- Server strings are stripped of control characters before reaching a terminal.
Names have no charset validation server-side, and the realistic source is an
LLM naming nodes from imported content, not a hostile collaborator.
- `show --depth` reports `scoped`/`root` so a scoped read is distinguishable
from a whole-graph fetch on stdout, and surfaces `unaddressable` in both
modes as `walk` does.
394 unit + 3 integration tests. Verified live: a slice with a crossing edge now
renders, depth bounds 96 nodes to 15, and truncated flips exactly at the cut.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
The CLI half of the scoped reads. Until now every "scoped" read fetched the whole branch graph and sliced it locally —
walk,walk --boundary, andshow <path>all calledfetch_branch_graph. Scoped was true of the output and false of the request, so reading one node of a 10,000-node project pulled all 10,000. That is the context problem these reads exist to solve, unsolved.Branch-addressed on purpose. The
/v1/graph/{project_id}/...twins resolve the project's main branch, and edits happen on working branches — so those routes cannot serve an authoring client at all.Paths come from the server
A scoped read returns a slice; a dotted path is built from ancestors the slice doesn't contain. Local reconstruction fails with
references a missing parent— which is exactly whatshow --depthdid against a real project until this change. It was invisible in unit tests because their fixtures include the ancestors. Found by running it, not by testing it.The map is deliberately not total
The server returns nodes it could not address — an unnamed node is legal while designing — and reports why in
unaddressable. So nothing indexes it:Indexing was the pre-existing shape and would panic on an ordinary graph. A test pins that; reverting
label_ofto indexing fails three tests with the panic at that line. (Verified the mutation actually applied before trusting the result —cargo fmthad silently defeated an earlier attempt.)Both output modes carry the map, so a consumer can act on it rather than infer from a missing key.
Fallback
Resolution needs the node's id, which the pulled index supplies. Without one — no working copy, never pulled, stale path — the whole-graph read still runs and says so. Asking for a slice must never deny you your graph.
Verification
cargo fmt,clippy -D warningscleanvalidateandstatusunaffectedNote for review
build_viewnow takes optional server paths instead of always reconstructing, and skips the ports of an unaddressable node rather than indexing its path. That's the shared seam between the scoped and wholesale renderers and the place a regression would hide.