fix(lineage): warn when an unqualified table id resolves to several projects (#568) - #579
Conversation
| def _ambiguity_warning(identifier: str, candidates: list[str]) -> str: | ||
| """Build the warning shown when an identifier matches several projects.""" | ||
| projects = [fqn.split(":", 1)[0] for fqn in candidates] | ||
| return ( | ||
| f"'{identifier}' exists in {len(candidates)} projects " | ||
| f"({', '.join(projects)}); showing '{candidates[0]}' only. " | ||
| f"Query a specific one with '--upstream/--downstream " | ||
| f"<project>:{identifier}' or scope with --project." | ||
| ) |
There was a problem hiding this comment.
🟡 Ambiguity warning claims a table exists in several projects even when the duplicates are all in one project, and offers advice that does not work
The warning text is built by counting candidate matches as if each one were a different project (_ambiguity_warning at src/keboola_agent_cli/services/deep_lineage_service.py:374-382), so when the same table name appears in several buckets of one project the user is told it "exists in 2 projects (alpha, alpha)" and is pointed at a lookup form that finds nothing.
Impact: Users querying a table by its bare name get a confusing, factually wrong warning plus a suggested retry command that fails with "node not found".
How the name-only fallback path produces same-project candidates
_find_node_candidates (src/keboola_agent_cli/services/deep_lineage_service.py:1065-1069) first tries an exact :{identifier} suffix match; if that yields nothing it falls back to matching any FQN whose last segment ends with .{identifier}. That fallback matches on the table name only, so --downstream orders can return alpha:in.c-a.orders and alpha:in.c-b.orders — two candidates, one project.
_query (src/keboola_agent_cli/services/deep_lineage_service.py:535-537) then attaches warnings=[_ambiguity_warning(...)] whenever len(candidates) > 1, and the message uses len(candidates) as the project count and lists fqn.split(":", 1)[0] per candidate, producing duplicated project names.
Additionally, in this fallback case the suggested remedies are invalid: '<project>:orders' is treated as a fully-qualified FQN and must be present verbatim in the graph (src/keboola_agent_cli/services/deep_lineage_service.py:1058-1059), and --project alpha builds alpha:orders (:1061-1063) — neither exists, so both retries return "Node not found".
Prompt for agents
In src/keboola_agent_cli/services/deep_lineage_service.py, _ambiguity_warning assumes every candidate FQN belongs to a distinct project and that the identifier is a full bucket_id.table_name. Both assumptions break for the name-only fallback branch in _find_node_candidates (the `f.split(":")[-1].endswith(f".{identifier}")` branch), which can return several FQNs from the same project and where the identifier is just a table name. Consider (a) deduplicating/counting distinct project aliases for the message, and (b) when candidates share a project or the identifier is not already a bucket-qualified id, listing the full candidate FQNs and telling the user to re-run with one of those exact FQNs instead of suggesting '<project>:<identifier>' or --project, which cannot resolve in that case.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # -- Output formatting helpers ---------------------------------------------- | ||
|
|
||
|
|
||
| def _emit_query_warnings(formatter, query_result: dict) -> None: |
There was a problem hiding this comment.
🟡 New helper function is missing a type annotation required by the project's coding rules
The newly added warning-printing helper leaves its first parameter untyped (_emit_query_warnings at src/keboola_agent_cli/commands/lineage.py:373), which contradicts the repository rule that every function signature carries type hints.
Impact: None at runtime; it violates an explicit contribution requirement and weakens static checking.
Rule reference
CONTRIBUTING.md, "Python conventions": "Type hints on all function signatures". The formatter parameter should be annotated (e.g. OutputFormatter), as done elsewhere in the codebase where the formatter type is imported.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if len(candidates) > 1: | ||
| result["ambiguous_matches"] = candidates | ||
| result["warnings"] = [_ambiguity_warning(identifier, candidates)] | ||
| return result |
There was a problem hiding this comment.
🔍 New JSON keys are also returned by the serve REST endpoints
_query now attaches ambiguous_matches and warnings to every ambiguous result, and the HTTP surface returns the service dict verbatim (src/keboola_agent_cli/server/routers/lineage.py:89-93, :189-214). The keys are additive so existing consumers keep working, but the REST response shape has changed without any doc/gotchas note; worth confirming that no response model restricts the payload and whether agent-facing docs (gotchas.md, AGENT_CONTEXT) should mention that lineage query results can now carry warnings.
Was this helpful? React with 👍 or 👎 to provide feedback.
| msg += "\nDid you mean: " + ", ".join(suggestions[:5]) | ||
| formatter.error(message=msg, error_code=ErrorCode.NODE_NOT_FOUND) | ||
| raise typer.Exit(code=1) | ||
| _emit_query_warnings(formatter, query_result) |
There was a problem hiding this comment.
🔍 Interactive lineage REPL does not surface the new ambiguity warnings
_emit_query_warnings is only called from the lineage show command path. The REPL/server query handler (src/keboola_agent_cli/commands/lineage.py:1308-1344) calls query_upstream/query_downstream directly and ignores warnings, so an ambiguous unqualified table id still answers for one project silently there — the same gap the PR fixes for lineage show.
Was this helpful? React with 👍 or 👎 to provide feedback.
padak
left a comment
There was a problem hiding this comment.
Review of #579 — fix(lineage): warn when an unqualified table id resolves to several projects (#568)
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check, not duplicated here.
Summary
This PR fixes issue #568: lineage show --upstream/--downstream <table> used to silently pick the alphabetically-first project when an unqualified bucket_id.table_name matched nodes in several projects, contradicting the command's own --help text ("auto-resolves, warns if ambiguous"). The fix unifies node resolution behind a new _find_node_candidates() that returns every match, and query_upstream/query_downstream (now sharing one _query() body) attach additive ambiguous_matches + warnings keys when more than one candidate is found; commands/lineage.py prints the warning via formatter.warning() (stderr in human mode, no-op in JSON — where it rides along in the payload). Verdict: does resolve issue #568 as described — reproduced live (see Verification log): the reporter's exact scenario (shared table across two projects, one has a real dependency the other lacks) now returns the same first-candidate answer as before, but with ambiguous_matches listing both projects and a warnings line telling the caller how to disambiguate. I found no BLOCKING issues; a handful of NON-BLOCKING gaps (a REST sub-endpoint that silently drops the new warning, dead code left behind by the refactor, and thin doc-sync) are worth the author's attention but do not block merge. Overall verdict: COMMENT.
Verdict
- Verdict: COMMENT
- Blocking findings: 0
- Non-blocking findings: 5
- Nits: 2
Blocking findings
(none)
Non-blocking findings
[NB-1] src/keboola_agent_cli/server/routers/lineage.py:193-226 — GET /lineage/mermaid silently drops the new ambiguity warning
I verified empirically (see Verification log) that POST /lineage/show and GET /lineage/walk pass ambiguous_matches/warnings straight through to kbagent serve clients — the router returns the service dict verbatim with no response_model filtering, so this PR's fix is not CLI-only as originally suspected. However, GET /lineage/mermaid (line 211-214) calls query_upstream/query_downstream and then only reads result.get("edges", []) to render the diagram — the warnings/ambiguous_matches keys are computed and then discarded, and the endpoint returns PlainTextResponse with no side channel to carry them. A React-UI user viewing the Mermaid graph for an ambiguous table gets a silently-partial diagram with zero indication, the exact failure mode #568 describes, just on a different surface. This PR didn't touch server/routers/lineage.py at all (confirmed unchanged), so this gap pre-dates it structurally but is now live for a case that previously never warned anywhere. Consider at minimum a response header (e.g. X-Lineage-Ambiguous: 2) or documenting the gap.
[NB-2] src/keboola_agent_cli/services/deep_lineage_service.py:1036-1038 (PR branch) — _find_node() is now dead code
The PR's own description says _find_node() "stays a thin wrapper returning the first, so node selection is unchanged and deterministic." I verified this is not true: query_upstream/query_downstream now call _query(), which calls self._find_node_candidates(...) directly — _find_node() is never called anywhere in the diff or the rest of the tree (confirmed by grepping every .py file in the PR branch for ._find_node(, zero hits outside its own definition). ruff/ty don't catch unused private methods, so make check stays green. Either remove _find_node() or restore a real caller; leaving it invites a future reader to "fix" a code path that no longer runs.
[NB-3] src/keboola_agent_cli/services/deep_lineage_service.py:213-220 (PR branch) — partial-match fallback branch (f.split(":")[-1].endswith(...)) has no dedicated ambiguity test
Per the focus question: I confirmed by direct reproduction (see Verification log) that the partial match branch (queries like "orders" with no bucket prefix, matched via f.split(":")[-1].endswith(f".{identifier}")) is also fixed — because _find_node_candidates() unifies both the exact-suffix and partial-suffix branches into one return path that _query() treats uniformly. This is a genuine strength of the refactor. However, TestAmbiguousNodeResolution (the new test class) only exercises the exact bucket_id.table_name branch (SHARED_TABLE = "in.c-shared.orders", always matched via the f.endswith(f":{identifier}") path) — there is no test with two different buckets sharing only a bare table name across projects, which is the only way to exercise the partial-match branch. Add one case to lock in the behavior I verified manually.
[NB-4] plugins/kbagent/skills/kbagent/references/gotchas.md:1785 — "Response structure varies by command" table entry for lineage show is stale, and this PR adds to the staleness
The table's lineage show row reads {"lineage_links": [...], "errors": [...]}, which does not match the actual shape (node, direction, node_info, edges, and now additively ambiguous_matches/warnings) returned by query_upstream/query_downstream — this mismatch pre-dates the PR, but the new additive keys make it further out of sync. The PR description states "No CLI surface change ... no plugin/agent doc sync required," which is true in the narrow sense (no new command/flag/error code), but the new JSON keys are exactly the kind of thing AI-agent consumers benefit from knowing about per CONTRIBUTING.md's Plugin synchronization map (gotchas.md, "always tag with (since vX.Y.Z)"). I did not find a version number assigned to this PR yet (no pyproject.toml bump in the diff), so a precise (since vX.Y.Z) tag isn't possible until release time — I found (during verification, outside this PR's diff) that src/keboola_agent_cli/changelog.py on the target branch already carries a drafted "Fix (#568)" bullet under a pending 0.84.0 entry, suggesting doc-sync for this fix is already being tracked at the release step per CONTRIBUTING.md's checklist. Flagging this NON-BLOCKING rather than BLOCKING for that reason — but the gotchas.md table row itself should be corrected (or removed) whenever that release-time pass happens.
[NB-5] tests/test_deep_lineage_service.py — no E2E case for the ambiguous-table scenario
tests/test_e2e_lineage_deep.py has existing lineage show coverage but nothing that builds two projects with a shared bucket to exercise the new ambiguous_matches/warnings path end-to-end. Per CONTRIBUTING.md, "every CLI command must have E2E coverage" — this is a behavior fix on an existing command rather than a new command, and standing up a real cross-project bucket-share fixture is a genuinely heavier E2E lift than most fixes, so I'm not blocking on it. The unit (TestAmbiguousNodeResolution, 6 cases) and CLI-layer (CliRunner, stderr + JSON) coverage in this PR is solid and gives good confidence regardless.
Nits
[NIT-1]src/keboola_agent_cli/services/deep_lineage_service.py— this file was already over its 1000-code-line soft ceiling onmain(1017 lines) before this PR; the PR's net addition pushes it to 1031. Not a blocker (soft ceiling only warns,make loc-checkstays green), but worth keeping in mind for the next PR that touches this file.[NIT-2]src/keboola_agent_cli/commands/lineage.py:60-68—_emit_query_warnings(formatter, query_result: dict)leavesformatteruntyped, matching the pre-existing (also untyped) pattern in sibling helpers_output_mermaid_or_html/_format_lineage_treein the same file, so not new drift introduced by this PR — just noting the file as a whole could use anOutputFormattertype hint on all three someday.
Verification log
- Read
CONTRIBUTING.md"Plugin synchronization map", "Checklist: Adding a New CLI Command", "Releasing a new version";CLAUDE.mdconvention #17 and## All CLI Commands;plugins/kbagent/agents/keboola-expert.md§1-3 — nolineagewrite/destructive group changes needed (all 4lineage.*ops arereadinOPERATION_REGISTRY). gh issue view 568 --repo keboola/cli→ confirmed the exact reported scenario (bucket shared A→B, unqualified id resolves to A only, B's real dependency invisible, no warning, exit 0).gh pr view 579 --json title,body,files,additions,deletions,...→ 3 files, +263/-53, conventionalfix(lineage):✓ matches change type (bug fix, no new surface).gh pr diff 579→ read in full; confirmed CLI + service + test changes only,server/routers/lineage.pyuntouched.- Isolated worktree (
git worktree add --detach ... origin/claude/issue-568-lineage-ambiguous-warning, own.venvviauv sync --extra server), removed after use — shared working tree at.claude/worktrees/issues-568-569-resolution-33f4e9was never checked out or mutated (git status/git rev-parse --abbrev-ref HEADconfirmed clean and on its own branch before and after). uv run pytest tests/test_deep_lineage_service.py -q→ 53 passed.make check(background, full run) → exit 0;5455 passed, 12 skipped, 147 deselected— matches the PR description's own claimed numbers exactly.ruff check,ruff format --check,ty check(targeted at the two changed source files),make loc-check,make command-sync-check,make check-error-codes,make check-sentinel-guards,make version-check→ all clean/OK.- Focus point 1 (REST parity) — built a FastAPI
TestClientagainstcreate_app()with a synthetic two-project shared-table lineage cache and hit the real endpoints:POST /lineage/show {"downstream": "in.c-shared.orders"}→ response body includes"ambiguous_matches": ["alpha:...", "beta:..."]and"warnings": [...]. ✓ reaches REST.GET /lineage/walk?node=...&direction=downstream→ same, keys present. ✓ reaches REST.GET /lineage/mermaid?node=...&direction=downstream→ 200,text/plainMermaid source only, no warning anywhere in body or headers. ✗ does NOT reach this one endpoint (see NB-1).
- Focus point 2 (
--help/doc promise) — readcommands/lineage.py:292(unchanged docstring: "Table only: bucket_id.table_name (auto-resolves, warns if ambiguous)") andlineage-deep-workflow.md:110(same claim). Confirmed viaoutput.py:197-207thatformatter.warning()writes to astderr=TrueRich console and is a no-op in JSON mode — matches the PR's stated stdout-clean-for-mermaid-piping design. The documented promise is now true where it was false before this PR; no doc text change needed on those two files (see NB-4 for the separate stale-schema-table issue ingotchas.md). - Focus point 3 (partial-match branch) — wrote a standalone repro script building two projects with different buckets (
in.c-sales.orders,in.c-crm.orders) sharing only the bare table nameorders, then calledservice.query_downstream(graph, "orders")directly against the PR branch's service. Result:ambiguous_matches: ['alpha:in.c-sales.orders', 'beta:in.c-crm.orders']and a populatedwarningslist — confirms the partial-match fallback is fixed too, not just the exact-suffix branch (see NB-3 for the test-coverage gap this uncovered). grep -rn "._find_node("across every.pyfile in the PR branch tree → only the definition itself, no callers (see NB-2).- Confirmed
OPERATION_REGISTRYalready haslineage.build/info/show/serverall as"read"— no registry change needed since no new command/flag was added.
Open questions for the author
- The changelog draft I found (outside this PR's diff, in the local repo state) already stages a "Fix (#568)" bullet under a pending future version. Worth confirming with whoever owns that release-prep step that
gotchas.md's stalelineage showresponse-shape row (NB-4) gets corrected in the same pass, since it's adjacent housekeeping the release checklist already touches.
…t matches _ambiguity_warning treated every candidate as a distinct project, but the name-only fallback in _find_node_candidates matches a bare table name across buckets and can return several nodes from ONE project. That produced a warning claiming a table "exists in 2 projects (alpha, alpha)" and pointed the user at '<project>:orders', which is not a node id -- the real ones carry a bucket -- so following the advice returned "Node not found". The cross-project case #568 reports is unchanged: it still counts distinct projects and still suggests the project-qualified form, which resolves there. The fallback case now reports node counts and names the full candidate ids, capped so a name matched across many buckets stays readable. Also annotates _emit_query_warnings' formatter parameter, which CONTRIBUTING requires on every signature.
…oo (#584) #579 gave `lineage show` and the JSON routes an ambiguity warning, but the mermaid renderer had no way to carry one: a diagram has no metadata channel, so all three callers dropped it. A Web UI or --format mermaid user therefore still got one project's answer looking like the whole picture -- the exact shape of #568, just on a different surface. render_mermaid now takes an optional `warnings` list and emits each as a standalone, deliberately unconnected note node, styled as a warning so it does not read as part of the dependency graph. All three callers pass what the query result carries: the CLI's --format mermaid/html, the `lineage server` browser, and GET /lineage/mermaid. Warnings go through the same label escaping as every other API-derived string (issue #269 sec-05), covered by a test. Also removes _find_node, which #579 left behind with no callers. Leaving it invites a future caller to bypass candidate resolution entirely, which is the bug #568 reported.
…584) #579 added the ambiguity warning for an unqualified table id that resolves to several projects, but only the text surfaces got it. Neither mermaid renderer had a channel to carry a warning, so every diagram caller dropped it -- a Web UI or `--format mermaid`/`er` user still saw one project's answer presented as the whole picture. Both renderers now take an optional `warnings` list. The flowchart emits each as a standalone, deliberately unconnected note node; `erDiagram` has no free-standing annotation, so the ER view carries them as one relationship-less note entity. All call sites pass what the query result holds: CLI `--format mermaid`/`html`/`er`, the `lineage server` browser, and `GET /lineage/mermaid` in both views. Warnings go through the same escaping as every other API-derived string (#269 sec-05). Rendering the result in a real browser caught a second defect: the remedy read `'--upstream/--downstream :id'` because mermaid renders the escaped entity back into SVG text as a literal `<project>`, which the browser drops as an unknown tag. Escaping was correct and held -- the loss happened one layer further on, where it looked right in a terminal and passed every string assertion. The placeholder is now `PROJECT:`, guarded by a test keeping angle brackets out of warning text. Also removes `_find_node`, left with no callers by #579 -- keeping it invites a future caller to bypass candidate resolution, which is the bug #568 reported.
What
kbagent lineage show --upstream/--downstream <table_id>silently answered for a single project when the same unqualifiedbucket_id.table_nameexisted in several project namespaces (a bucket shared from one project and linked into another produces one node per project)._find_node()didreturn sorted(matches)[0]and threw the other candidates away, so the command exited 0 with output that looked complete — while contradicting its own--helptext: "Table only:bucket_id.table_name(auto-resolves, warns if ambiguous)".Why
Real dependencies were invisible: a config in project B reading the shared table did not show up at all, and nothing told the user to retry with
B:bucket_id.table_name. A lineage answer that is quietly partial is worse than one that is loudly incomplete — "is anything downstream of this table" is exactly the question people ask before dropping it.How
DeepLineageService._find_node_candidates()(new) returns every FQN an identifier could refer to, best match first._find_node()stays a thin wrapper returning the first, so node selection is unchanged and deterministic.query_upstream()/query_downstream()now share one_query()body and, when more than one candidate matched, attach two additive keys to the result:ambiguous_matches(the full candidate list) andwarnings(a human-readable line naming the projects and how to disambiguate).commands/lineage.pyprints those warnings viaformatter.warning()— stderr in human mode, no-op in JSON mode where they already ride along in the payload. stdout stays byte-clean for--format mermaid/html/erpiping.project:tableFQN or--project ALIASis unambiguous by construction and stays silent.The two near-identical
if upstream:/if downstream:blocks in the command are folded into one loop — behaviour identical (upstream first, then downstream), and it pays for the added lines socommands/lineage.pystays inside its grandfathered file-size budget.Testing
TestAmbiguousNodeResolution(6 cases): warns and lists both candidates for downstream and upstream; explicit FQN,--projectscope, and a genuinely unique id stay silent; an unknown id still errors.Warning: ... exists in 2 projectson stderr;--jsoncarriesambiguous_matches+warningsinside the envelope and stdout still parses.make checkgreen: 5455 passed, 12 skipped; lint/format/ty/loc-check clean.No CLI surface change (no new command, flag or error code), so no changelog entry and no plugin/agent doc sync required.
Fixes #568