feat(cli): attribute CLI traffic to the invoking agent skill - #19387
Open
nwadams wants to merge 1 commit into
Open
feat(cli): attribute CLI traffic to the invoking agent skill#19387nwadams wants to merge 1 commit into
nwadams wants to merge 1 commit into
Conversation
`datahub -C skill=<name>` was accepted but had no effect on any command that
talks to GMS. The value landed on the Click context and was read only by the
`with_telemetry` decorator, which `search`, `get`, `lineage`, and `graphql` do
not use — so the skills that follow the documented convention emitted nothing,
and nothing reached the server either.
Resolve it into the client component instead, which rides the User-Agent every
request already carries:
User-Agent: DataHub-Client/1.0 (cli; skill-datahub-search/claude-code; 1.7.0)
GMS already parses that field into `agentClass`/`agentName` and can roll it up
as the `agent_name` usage dimension, so per-skill attribution needs no server
change — only that the value gets there.
Resolved inside `get_default_graph` rather than at each of its ~70 call sites,
so commands are attributed without opting in one at a time. An explicit
`datahub_component` argument still wins; `DATAHUB_COMPONENT` still applies when
no skill is passed. Values are normalized before they are sent, since ";" and
"/" delimit the field GMS parses.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
ingestion
PR SummaryOverview
Docs list Reviewed by Cursor Bugbot for commit 93c5cb4. Bugbot is set up for automated code reviews on this repo. Configure here. |
❌ 4 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="metadata-ingestion/src/datahub/cli/skill_context.py">
<violation number="1" location="metadata-ingestion/src/datahub/cli/skill_context.py:72">
P2: Custom agent: **Enforce Strict Maintainability Standards**
Unexpected errors in `infer_skill_component()` are swallowed and converted into missing attribution. The existing `silent=True` lookup and input checks cover the expected no-context cases, so catch only known optional-context failures and validate the nested context mapping explicitly; otherwise implementation defects silently fall back to the generic component.</violation>
</file>
<file name="metadata-ingestion/src/datahub/ingestion/graph/client.py">
<violation number="1" location="metadata-ingestion/src/datahub/ingestion/graph/client.py:2426">
P2: The skill component is resolved inside the lru-cached `get_default_graph`, but it is not part of the cache key `(client_mode, datahub_component)`. For a normal one-shot CLI process the root Group fixes the skill for the process, so this is safe. However, `get_default_graph` is a public library function called from 77 sites, including programmatic/embedded use (e.g. `forms.py`) and test suites that run multiple invocations in one interpreter. In any process where the Click context changes between calls with the same key, the cache returns a graph built with the first invocation's skill (or no skill), silently dropping or mis-attributing later requests. The unit test itself has to call `cache_clear()` around the call precisely because the cache is process-global and would otherwise leak the skill attribution across cases. Consider passing the resolved skill as `datahub_component` (already in the key) or removing the cache so the result always reflects the current context.</violation>
</file>
<file name="docs/cli.md">
<violation number="1" location="docs/cli.md:121">
P3: The normalization description is inaccurate: `_`, `.`, and `-` are also preserved, not just alphanumerics. A skill such as `my.skill_name` yields `skill-my.skill_name`, not `skill-my-skill-name` as the 'non-alphanumerics collapsed to `-`' wording implies. Restate it as 'anything outside `[a-z0-9._-]` collapsed to `-`' so users know which skill names survive intact.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return None | ||
|
|
||
| return sanitize_skill_component(raw) | ||
| except Exception as e: |
Contributor
There was a problem hiding this comment.
P2: Custom agent: Enforce Strict Maintainability Standards
Unexpected errors in infer_skill_component() are swallowed and converted into missing attribution. The existing silent=True lookup and input checks cover the expected no-context cases, so catch only known optional-context failures and validate the nested context mapping explicitly; otherwise implementation defects silently fall back to the generic component.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At metadata-ingestion/src/datahub/cli/skill_context.py, line 72:
<comment>Unexpected errors in `infer_skill_component()` are swallowed and converted into missing attribution. The existing `silent=True` lookup and input checks cover the expected no-context cases, so catch only known optional-context failures and validate the nested context mapping explicitly; otherwise implementation defects silently fall back to the generic component.</comment>
<file context>
@@ -0,0 +1,75 @@
+ return None
+
+ return sanitize_skill_component(raw)
+ except Exception as e:
+ # Attribution is best-effort telemetry; it must never fail a command.
+ logger.debug(f"Could not resolve skill attribution context: {e}")
</file context>
| # CLI process, so leaving it out of the lru_cache key can't serve a stale | ||
| # component. An explicit argument still wins, as does DATAHUB_COMPONENT | ||
| # when neither is set (resolved downstream in the emitter). | ||
| graph_config.datahub_component = datahub_component or infer_skill_component() |
Contributor
There was a problem hiding this comment.
P2: The skill component is resolved inside the lru-cached get_default_graph, but it is not part of the cache key (client_mode, datahub_component). For a normal one-shot CLI process the root Group fixes the skill for the process, so this is safe. However, get_default_graph is a public library function called from 77 sites, including programmatic/embedded use (e.g. forms.py) and test suites that run multiple invocations in one interpreter. In any process where the Click context changes between calls with the same key, the cache returns a graph built with the first invocation's skill (or no skill), silently dropping or mis-attributing later requests. The unit test itself has to call cache_clear() around the call precisely because the cache is process-global and would otherwise leak the skill attribution across cases. Consider passing the resolved skill as datahub_component (already in the key) or removing the cache so the result always reflects the current context.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At metadata-ingestion/src/datahub/ingestion/graph/client.py, line 2426:
<comment>The skill component is resolved inside the lru-cached `get_default_graph`, but it is not part of the cache key `(client_mode, datahub_component)`. For a normal one-shot CLI process the root Group fixes the skill for the process, so this is safe. However, `get_default_graph` is a public library function called from 77 sites, including programmatic/embedded use (e.g. `forms.py`) and test suites that run multiple invocations in one interpreter. In any process where the Click context changes between calls with the same key, the cache returns a graph built with the first invocation's skill (or no skill), silently dropping or mis-attributing later requests. The unit test itself has to call `cache_clear()` around the call precisely because the cache is process-global and would otherwise leak the skill attribution across cases. Consider passing the resolved skill as `datahub_component` (already in the key) or removing the cache so the result always reflects the current context.</comment>
<file context>
@@ -2415,7 +2416,14 @@ def get_default_graph(
+ # CLI process, so leaving it out of the lru_cache key can't serve a stale
+ # component. An explicit argument still wins, as does DATAHUB_COMPONENT
+ # when neither is set (resolved downstream in the emitter).
+ graph_config.datahub_component = datahub_component or infer_skill_component()
graph = DataHubGraph(graph_config)
graph.test_connection()
</file context>
| ``` | ||
|
|
||
| The skill name becomes the client component in the `User-Agent` header every request | ||
| carries, alongside the automatically detected caller: |
Contributor
There was a problem hiding this comment.
P3: The normalization description is inaccurate: _, ., and - are also preserved, not just alphanumerics. A skill such as my.skill_name yields skill-my.skill_name, not skill-my-skill-name as the 'non-alphanumerics collapsed to -' wording implies. Restate it as 'anything outside [a-z0-9._-] collapsed to -' so users know which skill names survive intact.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/cli.md, line 121:
<comment>The normalization description is inaccurate: `_`, `.`, and `-` are also preserved, not just alphanumerics. A skill such as `my.skill_name` yields `skill-my.skill_name`, not `skill-my-skill-name` as the 'non-alphanumerics collapsed to `-`' wording implies. Restate it as 'anything outside `[a-z0-9._-]` collapsed to `-`' so users know which skill names survive intact.</comment>
<file context>
@@ -107,6 +108,26 @@ Commands:
+```
+
+The skill name becomes the client component in the `User-Agent` header every request
+carries, alongside the automatically detected caller:
+
+```
</file context>
maggiehays
added
the
needs-review
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
2 participants
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.
What
Makes
datahub -C skill=<name>actually attribute the requests a command makes, by resolving it into the client component of theUser-Agentthat every request already carries:Why
-C skill=was accepted but had no effect on any command that talks to GMS. The value landed on the Click context and was read only by thewith_telemetrydecorator — whichsearch,get,lineage, andgraphqldo not use. So callers following the documented convention (docs/cli.md, and the agent skills that were told to pass it) emitted no telemetry, and nothing reached the server either.The server side needs no change.
RequestContextalready parsescomponent/callerout of the User-Agent intoagentClass/agentName(metadata-operation-context/src/main/resources/datahub_user_agents.yaml), andUsageDimensionscan rollagentNameup as the opt-inagent_nameusage dimension. The gap was purely that the value never got there.How
datahub/cli/skill_context.py: reads theskillpair off the Click context and normalizes it into askill-<name>component.get_default_graphresolves it when no explicitdatahub_componentwas passed.Resolved inside
get_default_graphrather than at its ~70 call sites, so commands are attributed without opting in one at a time and a new command cannot forget to. The value is parsed once on the root group and fixed for the life of a CLI process, so leaving it out of thelru_cachekey cannot serve a stale component.Precedence — explicit
datahub_componentargument >-C skill=>DATAHUB_COMPONENT>datahub. A flag passed for one invocation is more specific than an environment default.Normalization — lowercased, non-
[a-z0-9._-]runs collapsed to-, capped at 64 chars.;and/delimit the field GMS parses out of the User-Agent, so an unsanitized value would corrupt parsing for the whole request.No behavior change when the flag is absent:
infer_skill_component()returnsNoneoutside the CLI and when noskillpair was passed, leaving the existingDATAHUB_COMPONENTresolution untouched.Testing
metadata-ingestion/tests/unit/cli/test_skill_context.py— 8 tests covering normalization, User-Agent-breaking characters, unusable values, truncation, absent Click context, unrelated-Cpairs, and thatget_default_graphapplies the resolved component.RequestContextTest.testRequestContextPreservesAgentSkillComponent— proves the server-side leg: a skill-attributed CLI User-Agent parses toAgentClass.CLIwith the skill preserved in the agent name. (:metadata-operation-context:test→ 46 tests, green.)Also verified end-to-end against a local socket sniffer:
./gradlew :metadata-ingestion:lintFixand:metadata-operation-context:spotlessApplyclean.Checklist
docs/cli.mddocuments-C/--context(it was missing from the options block) and the attribution behaviorRelated
The agent-side half is acryldata/datahub-skills#48, which adds the hooks that pass this flag automatically. This PR is independently useful: it makes the documented convention work for anyone passing it by hand.
🤖 Generated with Claude Code
Summary by cubic
Attributes CLI requests to the invoking agent skill via
-C skill=<name>by setting the client component in theUser-Agent, enabling per-skill usage attribution in GMS. Previously the flag was accepted but had no effect; now it takes effect without changing individual commands. Addresses Linear AI-1017.datahub.cli.skill_contextand applied centrally inget_default_graph, so existing commands inherit attribution with no call-site changes.datahub_component>-C skill=>DATAHUB_COMPONENT>datahub.[a-z0-9._-]to-, prefixskill-, max 64 chars; prevents breakingUser-Agentparsing.-C/--contextand the attribution behavior.Written for commit 93c5cb4. Summary will update on new commits.