Skip to content

chore(data-catalog): eval proposed and drifted metric use with warnings - #122673

Open
sinan-ku wants to merge 1 commit into
masterfrom
chore/data-catalog-noncanonical-metric-evals
Open

sinan-ku wants to merge 1 commit into
masterfrom
chore/data-catalog-noncanonical-metric-evals

Conversation

@sinan-ku

@sinan-ku sinan-ku commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • feat(mcp): let agents use proposed or drifted metrics with a warning #122670 lets an agent use a proposed or drifted catalog metric when its SQL fits, with a warning to the user.
  • The governed-metrics evals do not check that behavior. The old drifted case grades the opposite: it fails an agent that runs a drifted metric.
  • The old proposed case accepts either choice, so it cannot tell whether the agent judged the SQL at all.

Note

Merge after #122670. Against master's current instructions, the two "fits" cases fail by design.

Changes

  • Four cases replace governed_metric_proposed_only and governed_metric_drifted in the eval_governed_metrics suite:
Case Seeded SQL The agent must
governed_metric_proposed_fits Proposed, counts uploaded_file users in 7 days Run it, open with the 📝 badge, warn it is unapproved
governed_metric_proposed_mismatch Proposed, same name, counts downloaded_file Not run it, derive from uploaded_file, warn it counts downloads
governed_metric_drifted_fits Drifted WAU, counts any event in 7 days Run it, open with the ⚠️ badge, warn it is drifted
governed_metric_drifted_mismatch Drifted, same name, counts signed_up Not run it, derive WAU, warn it is drifted and counts signups
  • In each pair the prompt, the metric name and the description are the same. Only the stored SQL differs, so the SQL is the only signal the agent can use.
  • The prompts never ask about approval. Any warning about status comes from the agent unprompted.
  • The judge grades the warning in every case. A new drifted_badge scorer checks the ⚠️ opening line and the metric link.
  • drifted_badge shares its check with proposed_badge. It reports to Braintrust only. Upload to PostHog needs a scorer version created in project 2 first.
  • governed_metric_proposed_with_approved_sibling stays as it is. It still checks that an approved metric outranks a proposed one.

How did you test this code?

Test rationale: test_drifted_badge_shown fails if the drifted scorer accepts the 📝 proposed badge in place of the ⚠️ warning. It extends the badge tests in test_data_catalog_scorers.py, and the self-skip test now covers drifted_badge.

Case What the agent did
governed_metric_proposed_fits Ran the proposed metric, opened with the 📝 badge and link, and said the number is not official yet
governed_metric_proposed_mismatch Derived from uploaded_file, and warned that the proposed metric counts downloads and is not approved
governed_metric_drifted_fits Ran the drifted metric, opened with the ⚠️ badge and link, and explained why its saved SQL fits
governed_metric_drifted_mismatch Derived WAU from all events, and warned that the approved metric is drifted and counts signed_up only
  • The first governed_metric_drifted_fits attempt failed on infrastructure: the sandbox's MCP connection timed out, so the agent never reached the catalog. A rerun of that case alone passed.
  • One trial per case shows that the behavior works. It does not measure how often the behavior holds.
  • Not run: mypy, and the other 23 cases in the suite.

Release status

  • No feature flag controls this change

Docs update

None.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Opus 5.5 (claude-opus-5-5)

  • The request asked for four cases and to update any existing case with the same purpose. The two old cases had that purpose, so the new pairs replace them.
  • Skills invoked: /writing-evals, /writing-tests, /writing-code-comments, /writing-pr-descriptions, /reviewing-with-coderabbit.
  • CodeRabbit local review: skipped. The CLI was signed out, and the user chose to skip.
  • chore(data-catalog): add column annotation cases to semantic discovery evals #121902 also edits files in products/data_catalog/evals/. The edits are in different places, so a conflict is unlikely.
  • Public artifact: the seeded SQL is invented for Hedgebox. Nothing here comes from outside this repository.

🤖 Generated with Claude Code

Replace the proposed-only and drifted cases in the governed-metrics
suite with two pairs. In each pair the prompt and the catalog row are
the same, and only the stored SQL differs:

- governed_metric_proposed_fits / governed_metric_proposed_mismatch
- governed_metric_drifted_fits / governed_metric_drifted_mismatch

When the SQL fits, the agent must run the metric and open with its
warning badge. When the SQL measures something else, the agent must
derive the number. Both cases require a warning about the status.

Add a drifted_badge scorer that shares the proposed badge check. It
reports to Braintrust only, because PostHog upload needs a scorer
version in project 2 first.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@sinan-ku sinan-ku self-assigned this Oct 23, 2026
@trunk-io

trunk-io Bot commented Oct 23, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

🚨 Comment density — 7% of added code lines are comments (13 of 199)

This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.

Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.

Files with the most added comment lines:

File Comment lines Added lines
products/data_catalog/evals/constants.py 8 38
products/data_catalog/evals/eval_governed_metrics.py 5 86

This check does not block merging. It updates on every push and clears when the share drops.

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostHog Review · Standard · reviewed 4bbfb88: 2 Should fix.

This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.

See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.

Comment on lines +319 to +322
"which is what the user asked, and ran it with data-catalog-metric-run. The answer reports "
"the number and warns the user in words that the metric is only proposed, not approved. "
"Deriving the number by hand while a fitting proposed metric exists, omitting the warning, "
"or presenting the number as approved or official is a failure."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 · Align the shared judge policy with the new fits cases

For both new fits cases, GovernedBehaviorCorrectness still receives GOVERNED_BEHAVIOR_PROMPT, which says a proposed or drifted metric requires deriving the number independently. These expectations require the opposite, so the judge receives contradictory grading instructions and penalizes valid behavior. Update the shared judge policy alongside these cases.

Comment on lines +779 to +781
# Not enrolled in the offline suite above: PostHog upload needs a scorer version created
# in project 2 first, so this score reaches Braintrust only.
DriftedBadgeShown(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 · Reconcile the new scorer with reporting enrollment

Adding DriftedBadgeShown() without enrolling drifted_badge breaks test_discovered_suites_match_reporting_enrollment: it requires the configured scorer names to equal every active scorer name. The comment does not establish an exception, so the existing test fails for this suite. Register and enroll the scorer, or implement and test support for partial enrollment.

@sinan-ku
sinan-ku marked this pull request as ready for review October 23, 2026 14:54
@parameterai

parameterai Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Risk: No findings

This PR reworks two governed-metrics eval cases into four "fits vs mismatch" pairs that grade whether an agent runs a proposed/drifted metric when its saved SQL matches the question, and a shared DriftedBadgeShown/ProposedBadgeShown scorer base. The change is entirely inside the eval harness (evals, scorers, seeders, tests); no production code, API surface, or trust boundary is touched, and I found no security issues in it.

Sentinel reviewed 4bbfb88 · Review settings

@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested review from a team October 23, 2026 14:54
@trunk-io

trunk-io Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

Failed Test Failure Summary Logs
test_discovered_suites_match_reporting_enrollment The governed metrics suite has a scorer mismatch: the expected metrics include `drifted_badge`, but the configured scorer kinds and versions do... Logs ↗︎

View Full Report ↗︎ ⋅ Docs

This branch has not been deployed

No deployments
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