Repository navigation
fix: report a truncated walk instead of returning it as CLEAR - #1
Merged
Merged
Conversation
An asset beyond `max_hops` is never fetched, so it drops out of `shared_urns()`, produces no finding, and contributes CLEAR -- a verdict indistinguishable from a complete walk that found nothing. Reproduced on the bundled recording: gating fraud_detector_v3 at max_hops=2 turns the same dropped `transaction_amount` that BLOCKs at max_hops=5 into a green "5 assets checked, no material change", exit 0, in total silence. The resolver now asks for upstreams before the depth check so it can tell whether the cap actually cost anything, and records the boundary nodes that still had unvisited upstreams behind them. That costs one extra lineage query per node on the boundary, which is the same trade `profile_coverage` already makes: a gate is allowed to be bounded, it is not allowed to be quietly bounded. Only nodes that genuinely left something behind are recorded -- a real leaf sitting on the cap is not, because a warning that fires on healthy footprints is how a team learns to ignore it. `fail_on_truncation` (default off) promotes it to exit 2; default-on would flip every deep graph's passing build to an error on upgrade, which is how people learn to route around the gate. Three more, all in the same seam: - Column-level feature attribution. `AssetSnapshot.column_features` was defined, consumed by `features_for()`/`primary_feature()`, and populated by nothing, so `evidence["column_level_lineage"]` was False on every real run and a dropped column implicated every feature its table's descendants fed. `McpLineageSource.get_column_lineage` existed and was called from nowhere. The resolver now walks column lineage upstream from the datasets that feed features and propagates the feature set transitively, so a column nothing reads is no longer tarred with them. Discovered by capability probe, exactly like `get_latest_profile` -- a source that cannot answer leaves `column_features` empty, `features_for()` falls back to the asset-level answer, and no existing verdict moves. Only MCP resolves it today, and the fixture recording carries no column lineage yet, so this is unit-tested rather than demonstrated end-to-end. - Harden the deploy-gate Skill against config-level suppression. It forbade editing a severity and passing a stale --baseline, but never named `rules`, `exemptions`, `thresholds`, `max_hops`, or `fail_on_truncation`. Two of those clear a red box without touching a severity at all: lowering `max_hops` stops the gate seeing the finding, and `exemptions` looks like process rather than tampering precisely because it demands a reason and an expiry. The knob list is pinned to `Policy.model_fields`, so a new setting fails the suite instead of silently becoming an unforbidden escape hatch. - CachingLineageSource. `memo` lived inside one `resolve_footprint` call and no source cached anything, so `check --all` re-fetched shared upstream layers once per model. Measured over an N-model inventory on a shared warehouse: 36% fewer reads at 2 models, 65% at 10, 69% at 20. Wraps rather than subclasses, so it covers MCP, SDK and recorded sources alike, and its lifetime is deliberately one command run -- a gate answering from a stale cache would be a far worse bug than a slow one. It refuses to answer capability probes for methods the inner source lacks: a wrapper that always exposed `get_column_lineage` would make every SDK run claim column-level lineage and then resolve nothing, which is the same class of quiet blindness this commit exists to remove. `connection_error` passes through, so failing closed still works. 442 -> 484 tests, ruff, mypy clean. Demo output unchanged. Not verified against a live instance: no DataHub was reachable from this environment. The truncation path is demonstrated against the recording; the column-lineage path is covered by mocks that implement the capability, and needs `scripts/record_fixture.py` extended before the demo can show it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens Undertow’s “fails closed” guarantees by (1) explicitly reporting hop-cap truncation so incomplete walks no longer render as a clean CLEAR, (2) enabling column-level feature attribution when the lineage source supports it, and (3) reducing repeated DataHub reads across multi-model checks via a per-run caching wrapper.
Changes:
- Report hop-cap truncation in
DependencyFootprint+ console output, and addfail_on_truncationpolicy to optionally treat truncation as exit 2. - Implement column-level lineage propagation to narrow feature attribution from table-level to column-level where supported.
- Add
CachingLineageSourceand wire it into CLI lineage-source construction to reduce repeated entity/lineage/schema/profile reads across a single run.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| undertow.yaml | Documents max_hops consequences and introduces fail_on_truncation. |
| tests/test_skill.py | Pins deploy-gate skill’s forbidden config knobs and guidance direction for max_hops. |
| tests/test_resolver_traversal.py | Adds tests for hop-cap truncation and column-level attribution behavior. |
| tests/test_resolver_caching.py | Adds tests for caching behavior and capability-probe invariants. |
| tests/test_cli.py | Adds tests for fail_on_truncation behavior in CLI-level gating. |
| src/undertow/resolver/traversal.py | Records truncation boundary nodes and applies column-level feature propagation. |
| src/undertow/resolver/caching.py | Introduces caching wrapper for lineage sources. |
| src/undertow/resolver/init.py | Exports CachingLineageSource. |
| src/undertow/reporter/console.py | Adds truncation reporting line to console output. |
| src/undertow/policy.py | Adds fail_on_truncation policy field. |
| src/undertow/models.py | Adds truncated_urns, truncated, and truncation_summary() to DependencyFootprint. |
| src/undertow/cli.py | Wraps sources with CachingLineageSource, adds _assert_not_truncated, and passes truncation summary into reporting/demo. |
| skills/undertow-deploy-gate/SKILL.md | Expands “don’t suppress findings” guidance to include config knobs beyond severities/baselines. |
| README.md | Documents truncation reporting and column-level attribution behavior at a high level. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+137
to
+166
| def __getattr__(self, name: str) -> Any: | ||
| """Memoise the optional methods, and only when the inner source has them. | ||
|
|
||
| Reached only for attributes normal lookup did not find, so it never | ||
| shadows the cached methods above. Raising `AttributeError` for a | ||
| capability the inner source lacks is load-bearing rather than | ||
| incidental: `getattr(source, "get_column_lineage", None)` in the | ||
| traversal must still come back None for an SDK source, or it will | ||
| believe column-level lineage is available and resolve nothing. | ||
| """ | ||
| # Guards recursion during __init__, before _inner is bound. | ||
| if name.startswith("_") or name not in _OPTIONAL_METHODS: | ||
| raise AttributeError(name) | ||
|
|
||
| inner_fn: Callable[..., Any] | None = getattr(self._inner, name, None) | ||
| if not callable(inner_fn): | ||
| raise AttributeError(name) | ||
|
|
||
| cache = self._optional.setdefault(name, {}) | ||
|
|
||
| def call(*args: Any) -> Any: | ||
| if args in cache: | ||
| self.hits += 1 | ||
| return cache[args] | ||
| self.misses += 1 | ||
| result = inner_fn(*args) | ||
| cache[args] = result | ||
| return result | ||
|
|
||
| return call |
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 bug
An asset beyond
max_hopsis never fetched, so it drops out ofshared_urns(), produces no finding, and contributes CLEAR. A truncated walk and a complete one that found nothing render identically.Reproduced against the bundled recording — same graph, same dropped
transaction_amount, only the hop cap differs:Before this change the second block printed nothing else: a confident "5 assets checked, no material change" over a walk that never reached the asset holding the blocking change. It now carries a
coverage:line naming how many assets still had unwalked upstreams.This is the failure mode the project already guards everywhere else — unreachable GMS exits 2,
profile_coveragereports what the statistical differ could actually inspect. The hop cap had no equivalent.What changed
Truncation is reported. The resolver asks for upstreams before the depth check so it can tell whether the cap cost anything, and records boundary nodes that still had unvisited upstreams. Costs one extra lineage query per boundary node. Only nodes that genuinely left something behind are recorded — a real leaf sitting on the cap is not, because a warning that fires on healthy footprints is how a team learns to ignore it.
fail_on_truncation(default off) promotes it to exit 2; default-on would flip every deep graph's passing build to an error on upgrade.Column-level feature attribution now resolves.
AssetSnapshot.column_featureswas defined and consumed byfeatures_for()/primary_feature(), but populated by nothing — soevidence["column_level_lineage"]wasFalseon every real run, and a dropped column implicated every feature its table's descendants fed.McpLineageSource.get_column_lineageexisted and was called from nowhere. The resolver now walks column lineage upstream from feature-feeding datasets and propagates the feature set transitively.The deploy-gate Skill is hardened against config-level suppression. It forbade editing a severity and passing a stale
--baseline, but never namedrules,exemptions,thresholds,max_hops, orfail_on_truncation. Two of those clear a red box without touching a severity at all: loweringmax_hopsstops the gate seeing the finding, andexemptionslooks like process rather than tampering precisely because it demands a reason and an expiry. The knob list is pinned toPolicy.model_fields, so a new setting fails the suite instead of silently becoming an unforbidden escape hatch.CachingLineageSource.memolived inside oneresolve_footprintcall and no source cached anything, socheck --allre-fetched shared upstream layers once per model. Measured over an N-model inventory on a shared warehouse:Lifetime is deliberately one command run — a gate answering from a stale cache would be a far worse bug than a slow one.
The invariant worth reviewing closely
The cache wrapper must not answer capability probes for methods the inner source lacks. The traversal discovers
get_latest_profileandget_column_lineagewithgetattrand changes behaviour when they are absent. A wrapper that always exposedget_column_lineagewould make every SDK run claim column-level lineage and then resolve nothing — the same class of quiet blindness this PR exists to remove.__getattr__re-raisesAttributeErrorwhen the inner source lacks the method, with a test assertingnot hasattr(cached, "get_column_lineage")for a plain source.connection_errorpasses through as a property so failing closed still works.Column-level resolution degrades to the asset-level answer when the source cannot resolve columns, which is why this moves no existing verdict.
Verification
442 → 484 tests,
ruff,mypy --strictclean. Demo output unchanged.Not verified against a live instance — no DataHub was reachable from the authoring environment, and it was run on Windows against a single Python version. The truncation path is demonstrated against the recording; the column-lineage path is covered by mocks that implement the capability.
Follow-ups this does not do
scripts/record_fixture.pyneeds extending to capture column lineage before the demo can show column-level attribution end-to-end. The current recording carries none.SdkLineageSourcehas noget_column_lineage, so the SDK path is now strictly less precise than MCP. Undocumented asymmetry.docs/02-SYSTEM-ARCHITECTURE.md:77still describesmax_hopsas a plain config knob with no mention of truncation reporting.