feat(databases)!: lineage walks the whole fork family - #288
Conversation
Live-verified against productionRan the full sequence in workspace AgentRyan (throwaway databases, 2h expiry, all cleaned up): create → fork → fork (
Bug found and fixed during verification (49a2807): the server annotates each Note for a follow-up: the flat view has the same ancestor-entry semantics — |
Second live e2e pass (post flat-removal, tree-by-default)Branching topology this time — root A with forks B and C, grandchild D under B, grandchild E under C:
All throwaway databases removed; config verified intact. Observation: the server returns fork lists newest-first. |
The lineage endpoint returns one ancestral path plus one generation of
direct forks, so a fork of a fork is invisible from its grandparent.
--tree assembles the full family client-side: walk down from root_id,
one lineage request per database — N+1, acceptable for the small fork
trees the feature targets — and render it with proper rail glyphs
(│/├─/└─), the queried database marked as before.
- the queried database's already-fetched lineage seeds the walk and is
reused, so its endpoint is hit exactly once
- a node whose own lineage can't be fetched (e.g. a deleted generation)
degrades to forks_unknown instead of aborting, and the known descent
toward the queried database is grafted so it always appears
- a visited-set guards against server-side cycles; per-node fork pages
keep the '⋯ N more — see --forks-limit' marker, applied per database
- -o json/yaml return {database_id, root_id, tree} with a recursive
node shape; forks_unknown / unlisted_forks appear only when set
- docs: skill synopsis + lineage bullet cover --tree
Live verification surfaced the server's ancestor-entry semantics: each
ancestors[] entry carries the fork edge BELOW it — the snapshot of that
ancestor the next generation copied, and when — while forks[] entries
carry their own creation edge. The tree walk took the root's entry from
the ancestor chain verbatim, so the root row claimed a fork event it
never had ('— root, forked …, snapshot 0').
Shift the chain metadata one generation down so every tree row carries
its own creation edge (the forks-list convention), and strip it from
the top of the chain: a root has no fork event, and below a truncation
the edge above is unknown. As a bonus the grafted rows on the
deleted-generation path now show their true fork dates too.
Live-verified against production (create → fork → fork → lineage --tree
from every node → delete middle → --tree again → deleted): the walk
renders grandchildren, the server 404s lineage for deleted ids, and the
graft path keeps the queried database visible under the deleted
generation with '⋯ other forks unknown'.
Drop flat mode and the --tree flag: 'databases lineage' now renders the
full family tree by default. The tree is an informational superset of
the flat view — same ancestors (grafted even past deleted generations),
same direct forks with paging — plus every deeper generation, and it
displays each row's own creation edge instead of the server's
queried-database-relative ancestor annotations, which made the flat
view label the child row with the grandchild's fork date.
BREAKING: the -o json/yaml shape changes from
{ancestors, ancestors_truncated, forks, fork_count} to
{database_id, root_id, tree} with a recursive tree node. The flat shape
shipped in 0.29.0 (today); this is the cheapest moment to change it.
Trade-offs accepted: one request per database instead of one total
(fork families are small in practice), no atomic snapshot, and per-node
degradation ('⋯ forks unknown') instead of one all-or-nothing response.
ancestors_truncated is no longer carried: the walk detects truncation
structurally (the top of the ancestor chain won't match root_id).
Live-verified: create → fork → lineage renders the tree by default;
--tree is rejected; cleanup done.
7ab3353 to
2f46826
Compare
| Ok(r) => { | ||
| let unlisted_forks = (r.fork_count - r.forks.len() as i64).max(0); | ||
| let mut forks = Vec::new(); | ||
| for f in r.forks { |
There was a problem hiding this comment.
The walk drops the queried database when a parent's fork list is paginated. descent is consulted only in the Err arm below, so a truncated fork page gets no graft.
Failure scenario: a root database has 150 forks, and the server returns only the newest page. Query the oldest fork. The walk descends only the returned page, never reaches the queried fork, and renders a tree with no ← this database row. The exit code stays 0. --forks-limit cannot recover this case, because the server clamps the page to 100.
The skill doc claims the queried database is always marked in place, so the contract breaks for wide families. The old flat view always showed the queried database, so this is a regression.
Fix: in the Ok arm, also graft descent.get(&id) when the listed forks do not already contain that child. Subtract the grafted child from unlisted_forks so the ⋯ N more count stays accurate. Add a test with fork_count greater than the returned fork list.
| .to_string(), | ||
| ); | ||
| } | ||
| Err(_) => { |
There was a problem hiding this comment.
nit: Err(_) treats an expired token, a 429, and a transport timeout exactly like a deleted generation (not blocking). Every such node renders ⋯ forks unknown (lineage unavailable) and the command still exits 0. A reader cannot tell a genuinely un-enumerable generation from a failed request, and the walk issues one request per database, so a rate limit is reachable.
database_exists_or_unverifiable (databases.rs:2044) already matches ApiError::Status { status, .. } for this kind of distinction. Match NOT_FOUND for the deleted case, and print a stderr warning naming the id for any other error.
There was a problem hiding this comment.
Review
Blocking Issues
src/commands/databases.rs:1907— the walk omits the queried database when an ancestor's fork list is paginated.descentis grafted only on a fetch error, not on a truncated fork page. A root with more than 100 forks makes the queried fork unreachable, and--forks-limitcannot raise the page past the server clamp of 100. The command then prints a tree without the← this databaserow and exits 0.
Action Required
- Graft
descent.get(&id)in theOkarm ofwalk_lineage_treewhen the listed forks omit that child. Adjustunlisted_forksfor the grafted child. - Add a walk test where
fork_countexceeds the returned fork list and the queried database sits off the page.
CI status was queued or in progress for CI / test, CI / fmt, and Integration Tests / integration at review time, so no test result is confirmed here.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
The review caught a hole in the walk: descent was grafted only when a node's lineage fetch FAILED, not when it succeeded with a truncated fork page. A root with more forks than the page (server clamps to 100, newest-first) could page out the very generation that leads to the queried database — the tree then rendered without its '← this database' row and exited 0, breaking the always-marked contract the skill doc states. Now the Ok arm grafts the known descent child whenever the listed page didn't include it, counting it out of the unlisted tally. Regression test: fork_count 3, page of 1, queried database off the page. Also split the Err arm's diagnostics: a 404 stays quiet (the expected shape of a deleted generation), while any other failure (auth, rate limit, transport) still renders '⋯ forks unknown' but says why on stderr, naming the id — so a failed request no longer masquerades as a deleted-generation artifact.
|
Both review findings addressed in the latest commit:
|
| #[serde(flatten)] | ||
| entry: LineageEntry, | ||
| /// Direct forks the server listed but the walk page did not include | ||
| /// (`fork_count` minus the page) — raise --forks-limit to see them. |
There was a problem hiding this comment.
super nit: this doc line no longer matches the code (not blocking). walk_lineage_tree now also subtracts the grafted descent child, so the value is fork_count minus the page minus the graft. A reader of the unlisted_forks JSON field takes the doc as the definition of a published output key. Suggested wording: "fork_count minus the forks this node already lists".
There was a problem hiding this comment.
Both prior findings are fixed in 25d5a5e.
- Paginated fork page: the
Okarm graftsdescent.get(&id)and decrementsunlisted_forks, guarded by the visited set so a listed child is not added twice.lineage_tree_grafts_past_paginated_fork_pagepins the case. - Error conflation: the
Errarm matchesNOT_FOUNDfor the deleted generation and warns on stderr for every other failure.ApiErrorhas exactly the two variants the match covers.
One super nit inline, non-blocking. CI was still queued at review time, so the test result is unknown here.
Summary
databases lineagenow renders the full fork family tree — every reachable generation, walked from the root down, one lineage request per database:Flat mode (one ancestral path + direct forks only, fork-of-a-fork invisible from the grandparent) is removed rather than kept behind a flag: the tree is an informational superset, and the flat JSON shape shipped only in 0.29.0 (same day), so this is the cheapest moment to change the contract.
BREAKING:
-o json/yamlchange from{ancestors, ancestors_truncated, forks, fork_count}to{database_id, root_id, tree}with a recursivetreenode.Design notes
root_id; the walk starts there. The initial fetch still hard-exits on auth/network errors; per-node failures during the walk degrade gracefully..expect(1)in tests).⋯ forks unknownbranch — and the known descent toward the queried database is grafted past it, so it always appears in its own tree.ancestors[]entries with the fork edge below each ancestor, whileforks[]entries carry their own creation edge. Chain metadata is shifted one generation down so every row shows its own creation edge — this also removes the flat view's misleading annotation (child row labeled with the grandchild's fork date).--forks-limitapplies per database; a truncated branch closes with⋯ N more, and-o jsoncarriesunlisted_forks.Testing
lineage_tree_*); 399 unit + integration tests pass; clippy/fmt clean, zero warningsStacked on #287
Based on
docs/skills-lineage-jobs-refresh— retarget tomainafter #287 merges.