Skip to content

feat: add RelationshipChecker for advisory JOIN validation (v0.7.0) - #2

Merged
flyersworder merged 13 commits into
mainfrom
feat/relationship-checker
Apr 11, 2026
Merged

feat: add RelationshipChecker for advisory JOIN validation (v0.7.0)#2
flyersworder merged 13 commits into
mainfrom
feat/relationship-checker

Conversation

@flyersworder

Copy link
Copy Markdown
Owner

Summary

  • Add RelationshipChecker that validates SQL JOINs against declared semantic relationships with three advisory detection modes: join-key correctness, required-filter enforcement, and fan-out risk detection
  • Wire into Validator via optional semantic_source parameter — fully backward-compatible
  • Warnings only (never blocks), silent on undeclared joins — no false positives from incomplete relationship definitions

What it does

When a SemanticSource with relationships is passed to the Validator, the checker:

  1. Join-key correctness — warns if agent joins on wrong columns vs. declared from/to (supports both ON and USING syntax)
  2. Required-filter enforcement — warns if a relationship's required_filter column is missing from WHERE
  3. Fan-out risk — warns if aggregating (SUM, COUNT, etc.) across a one_to_many join (top-level SELECT only, ignores subqueries)

Test plan

  • 18 unit tests covering all three detection modes + edge cases (aliases, bare names, case-insensitive, USING clause, subquery aggregation)
  • 3 integration tests for Validator + SemanticSource wiring
  • 280/280 tests passing, ruff + ty clean
  • Pre-commit hooks pass

🤖 Generated with Claude Code

flyersworder and others added 10 commits April 11, 2026 09:29
Advisory-only checker that validates SQL JOINs against declared
semantic relationships — covers join-key correctness, required-filter
enforcement, and fan-out risk detection.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Clarify RelationshipChecker does NOT implement Checker protocol
  (uses check_joins() returning list[str] instead of check_ast())
- Add bidirectional lookup for relationship matching
- Scope v1 to explicit JOINs only (defer implicit comma-joins)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6-task TDD plan covering join-key correctness, required-filter
enforcement, fan-out detection, and Validator integration.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extends RelationshipChecker.check_joins() to track matched relationships
and warn when a required_filter column is absent from the query's WHERE
clause. Adds TestRequiredFilterEnforcement class with 4 tests; updates
pre-existing TestJoinKeyCorrectness queries to satisfy the orders->customers
required_filter now enforced by the checker.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds _check_fan_out() to warn when a query aggregates (SUM/AVG/COUNT/MIN/MAX)
across a one_to_many join, where row multiplication can silently inflate results.
Includes five new tests in TestFanOutDetection covering all cardinality types.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ource

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Support USING clause joins (previously silently ignored)
- Scope fan-out detection to top-level SELECT only (ignore subquery aggs)
- Skip relationship warnings on already-blocked queries

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…re docs

Add RelationshipChecker documentation: CHANGELOG entry with all three
detection modes, README section on relationship validation, and
architecture doc updates including validation flow diagram.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@flyersworder

Copy link
Copy Markdown
Owner Author

Code review

Found 2 issues:

  1. USING clause picks wrong from_table in 3+ table queries (bug)

In _extract_join_columns, the USING branch selects the from-table as [t for t in alias_map.values() if t != joined_table][0]. Since alias_map contains all tables in the query, this picks an arbitrary first match rather than the actual preceding table in the join chain. For FROM orders JOIN customers ON ... JOIN addresses USING (customer_id), the second join resolves from_table to orders instead of customers, causing incorrect relationship lookups or silent misses.

# Find the FROM table: first table in alias_map that isn't the joined table
from_tables = [t for t in alias_map.values() if t != joined_table]
from_table = from_tables[0] if from_tables else ""
for ident in using_clause:
col_name = ident.name.lower()
# USING means both sides use the same column name
results.append((from_table, col_name, joined_table, col_name))
return results

  1. _has_aggregation false positive for scalar subqueries in SELECT list (bug)

_has_aggregation iterates select.expressions and calls find_all(*AGG_TYPES) on each, which recurses into scalar subquery nodes. A query like SELECT (SELECT AVG(price) FROM products), o.id FROM orders o JOIN order_items oi ON o.id = oi.order_id triggers a fan-out warning even though the outer query has no aggregation. The existing parent_select guard only skips subqueries in FROM/WHERE, not scalar subqueries embedded in SELECT expressions.

@staticmethod
def _has_aggregation(ast: exp.Expression) -> bool:
"""Check if the top-level SELECT contains any aggregation functions.
Ignores aggregations inside subqueries to avoid false positives.
"""
for select in ast.find_all(exp.Select):
if select.parent_select is not None:
continue
# Check only the SELECT's own expressions, not subqueries
for expr in select.expressions:

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

flyersworder and others added 3 commits April 11, 2026 10:17
- USING clause: generate candidate pairs for all other tables instead
  of picking arbitrary [0] from alias_map — fixes wrong from_table
  resolution in 3+ table queries
- Scalar subquery agg: filter out aggregations inside exp.Subquery
  ancestors — fixes false positive fan-out warnings for queries like
  SELECT (SELECT AVG(...) FROM t2), o.id FROM t1 JOIN t2 ...

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ader

- Add seen set to _check_required_filters to prevent duplicate warnings
  when USING clause generates multiple candidate pairs matching the same
  relationship (consistent with _check_fan_out dedup pattern)
- Update architecture doc header to v0.7.0

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Internal spec and plan files are no longer needed — the design is
captured in architecture.md, CHANGELOG, and the code itself.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@flyersworder
flyersworder merged commit 93093fd into main Apr 11, 2026
3 checks passed
@flyersworder
flyersworder deleted the feat/relationship-checker branch April 11, 2026 08:21
flyersworder added a commit that referenced this pull request Aug 18, 2026
* docs: spec for declared attribution convention (#67)

Design doc for the vocabulary the #67 pilot's outcome #2 implies: a
convention / convention_operand pair on Decomposition, a source-level
default resolved at load, loud validation, round-trip through both
sources, and a pure attribution kernel shipped unwired.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: resolve four ambiguities in the attribution spec

Self-review pass:

- shares is None (not {}) when delta_parent is zero, so "undefined" is
  distinguishable from "all zero".
- contributions excludes the residual under explicit and includes it
  under the other two; interaction always reports the raw residual so the
  placement stays auditable.
- reported's interaction key is named by a module constant, required
  under explicit and rejected otherwise -- reporting it after it has been
  distributed double-counts.
- matches is independent of sums_to_delta. A breakdown can sum correctly
  and still use the wrong convention; that is the #67 failure.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: spec carries the convention on both agent channels (#67)

Toolset audit, from the agent's side rather than the library's.

A decomposition reaches the agent through two tools, not one, and the
spec only patched lookup_metric. trace_metric_impacts returns identity
edges carrying `operator`, and its own description tells the agent to
"walk 'identity' first to localize the change" for root cause -- which is
the attribution workflow. An agent following that guidance would get the
operator and no convention: the one tool whose description names "why did
revenue drop?" would omit the field governing the answer. IdentityEdge,
identity_edges_from_metrics() and the edge renderer now carry the pair.

That also settles the tool question in the other direction. No tenth
tool: it would fire at the last step, after the operands are measured,
to do arithmetic the pilot showed the agent performs correctly 16/16.
Its only real content is the convention, which both channels above
already deliver -- so it adds a round-trip and no information, while
diluting attention on inspect_query and run_query every turn.

And it shrinks the kernel's claim. Its customer is the measurement, not
the runtime: check_attribution is the scoring pass #67 hand-wrote, made
reusable for the arm that asks whether declaring the convention changes
anything. attribute_change is its computed half.

Also records why pull-only is sufficient here, which the earlier draft
was too pessimistic about: an attribution question cannot be answered
without pulling the decomposition, since the agent cannot name the
factors otherwise.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: implementation plan for the declared attribution convention (#67)

Nine TDD tasks, each ending in an independently testable deliverable:
vocabulary and validation, source-level default, dump round-trip with a
digest-stability guard, Ossie parity, IdentityEdge propagation, both tool
channels, the kernel, check_attribution plus exports, docs and release.

Self-review caught four things worth recording:

- Task 6 breaks a passing test on purpose. test_decomposition_tools
  asserts an exact dict for the fixture's decomposition, so extending
  that fixture makes the dict stop matching. The task says so up front
  and updates the assertion, rather than leaving a fresh engineer to
  debug it.
- SEMANTIC_KEYS gaining a member is a public API change, not an internal
  tweak: it is re-exported at the top level, README and the v0.41.0
  CHANGELOG name it, and test_public_api asserts its exact contents. It
  gets its own task with the test and doc updates attached.
- The architecture.md paragraph and the CHANGELOG requirements are
  written out rather than described, so the docs task carries the same
  weight of instruction as the code tasks.
- Test helpers use explicit keyword parameters instead of **kwargs, and
  the frozen-result test matches the AttributeError style
  test_reconciliation already uses.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: correct Task 4 — singular semantic_model key, per-model default scoping

Pre-flight scan against the loader source found two defects in the plan's
Ossie task. OssieSource iterates raw.get("semantic_model"), singular, so
the plural key would have built a file the loader ignores. And
self._metrics accumulates across that loop, so applying the default to the
whole list would stamp one model's house convention onto another's --
silent, and exactly the class of bug this feature exists to prevent. The
default is now scoped to the slice each model contributes, with a
regression test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* feat: declare a cross-term attribution convention on Decomposition

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* feat: source-level decomposition_convention default, resolved at load

* feat: convention round-trips through dump_semantic_source, digest stays stable

Emit `convention`/`convention_operand` from dump_semantic_source only when
set on a Decomposition, matching the existing omit-when-empty discipline
so a contract declaring no convention keeps byte-identical canonical bytes
and every published ARD attestation digest.

* feat: OssieSource carries the attribution convention, scoped per model

* feat: identity edges carry the attribution convention

* feat: lookup_metric and trace_metric_impacts carry the convention

Both delivery surfaces for a decomposition now propagate convention and
convention_operand, omitting the keys entirely when unset so contracts that
declare no convention stay digest-identical. trace_metric_impacts' identity
edges needed this too: its own tool description tells the agent to walk
"identity" first for root-cause attribution, so that channel must not hand
over the operator without the convention that says where the cross term goes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* feat: pure attribution kernel applies the declared convention

attribute_change() breaks a metric's change into per-factor
contributions and places the product/ratio cross term exactly where
the decomposition's convention says (explicit/split_evenly/fold_into),
instead of leaving the placement to an agent's silent, undeclared
choice. Reuses _apply_operator from validation/reconciliation.py for
the operator arithmetic; a single lumped interaction residual
(delta - sum(main_effects)) rather than the full 2**n-1 expansion.

A cross-term operator (product/ratio) that declares no convention
raises -- the kernel only works on a governed metric.

* feat: check_attribution scores a reported breakdown against the contract

Also fixes a defect in attribute_change: a hand-built Decomposition with
convention="fold_into" and a missing or invalid convention_operand hit an
AssertionError (or KeyError under python -O) deep in _place's arithmetic
instead of a clean ValueError raised before any arithmetic runs, which is
the module's own contract.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: declared attribution convention (v0.43.0)

Documents the convention/convention_operand vocabulary shipped over the
prior eight tasks: README sections for declaring the cross-term
placement and for attribute_change/check_attribution, an
architecture.md paragraph explaining the design decisions, the
resolved variance-diagnosis pilot outcome, and the CHANGELOG entry.
Removes the spec/plan scaffolding now superseded by the shipped docs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: declared attribution convention docs and v0.43.0 bump

README: declaring a decomposition_convention, the trace_metric_impacts
edge now carrying it, and attribute_change/check_attribution usage.
docs/architecture.md: design-decision paragraph for the convention and
the resolved variance-diagnosis pilot outcome (#67, 16 sessions: 16/16
arithmetically correct, three cross-term placements, 13.5% span).
CHANGELOG.md: v0.43.0 entry. pyproject.toml/uv.lock: version bump.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: apply final review's fix wave for attribution convention docs/tests

Final whole-branch review of feat/decomposition-attribution-convention caught
a README example that couldn't run and contradicted its own YAML, a stale key
count, and gaps in test/doc symmetry:

- README: contract.semantic_source doesn't exist; use load_semantic_source().
- README: attribute_change example's numbers (2000.0/1250.0, split_evenly)
  didn't match the declared activations metric (fold_into: rate), which
  yields 1750.0/1500.0. Corrected and verified by running the snippet.
- README: SEMANTIC_KEYS is five keys now (decomposition_convention added in
  this branch), not four.
- README: split a comment that described convention_operand: onto the
  convention: line it actually documents.
- CHANGELOG: decomposition_convention is never emitted by dump_semantic_source
  (its effect is resolved onto each decomposition's convention at load), not
  merely omitted-when-unset like convention/convention_operand.
- Tests: cover convention=None with convention_operand set (distinct error
  message from the split_evenly+operand case already tested); cover
  sums_to_delta for the explicit convention, the only one where it depends on
  summing in INTERACTION_KEY; make the trace_metric_impacts
  undeclared-convention test check convention_operand too, matching
  lookup_metric's existing symmetry.
- attribution.py: document why AttributionResult.shares sums to less than 1
  under explicit, so an eval harness normalizing shares doesn't read it as a
  bug.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: examples declare an attribution convention

The examples are executable documentation -- the CI job that runs them
says so -- and both of the ones carrying a decomposition carry a
cross-term operator: revenue_agent's product and growth_agent's ratio.
Neither declared a convention, so the release shipped vocabulary the
examples did not demonstrate.

revenue_agent gets the source-level default. Its contract already sets
`expected_extras: [column_hints, join_paths]`, so a top-level key there
is a real test of the SEMANTIC_KEYS change: `decomposition_convention`
loads cleanly because it is interpreted vocabulary rather than a consumer
extra. lookup_metric then reports the resolved value, which is the
teaching moment -- the default is stamped onto the decomposition at load,
not carried alongside it.

growth_agent declares `explicit` per decomposition, on the operator whose
own comment frames the question as "numerator or denominator?" -- folding
the residual into either factor would credit one with movement it did not
cause.

That comment also told the agent to walk kinds="identity" first for root
cause, and the example never did: it walked upstream with the default
kinds, so the identity edges never appeared in its output. It now makes
that walk, which is both the documented workflow and the second delivery
channel -- each edge carries the convention, so an agent that never calls
lookup_metric still learns where the cross term goes.

Verified by running all three examples exactly as CI does, including the
grep markers; 1063 tests pass, prek clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: scale the attribution check to the breakdown, not to delta_parent

Three findings from the PR review, all reproduced before fixing.

delta_parent was the module's only reference magnitude, which silently
assumes the total change is non-zero. A metric whose factors offset --
customers double while spend halves -- has a delta of exactly 0.0 and
contributions of +/-100. That is the attribution question most worth
asking, and there the tolerance became rel_tol * 0.0 == 0.0, i.e. exact
float equality: a reported figure 1e-7 off failed the check. Tolerance
now scales to the largest magnitude in the breakdown. Verified the looser
scale is not toothless -- scale 100 at rel_tol 1e-4 gives 0.01, so a
5.0 deviation is still rejected.

The same assumption broke `shares`. Its guard was an exact `delta == 0`,
but factors that offset are flat in decimal and float noise in binary:
0.1*3.0 against 0.3*1.0 differ by 5.6e-17, so the guard missed and shares
came back at ~1e16 where the docstring promises None. The guard is now
relative to the breakdown's own magnitude.

Third: an operand literally named `interaction` collided with the key an
`explicit` breakdown reports its residual under. Nothing at load time
forbids that name, and the collision overwrote the operand's own
contribution -- making a correct breakdown inexpressible and returning a
self-contradictory verdict (matches True with sums_to_delta False).
check_attribution now refuses the ambiguous check with a clear error
rather than scoring it. Only `explicit` reports a separate residual line,
so the other conventions stay usable with such an operand.

The pilot table and the #67 detection case are unchanged: split_evenly
still gives 2000/1250, and a fold_into-shaped report against a declared
split_evenly still returns sums_to_delta True with matches False.

1070 passing, prek clean, all three examples run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant