fix(trigger): recognize Google ADK tools=[...] as a tool-surface change - #349
Conversation
8f53aba to
145f083
Compare
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Review findings (not an approval): I found two reproducible routing regressions that should be fixed before merge. The existing-list ADK case still fails open, and the new dependency-manifest allowlist removes advisory coverage for common real bump surfaces. CI and the public-surface tests are green, but neither adversarial case is covered. Shipgate's expected trust-root review remains separate: the exact-ref verifier reports human_review_required / review_required with 0 blockers and 4 review items.
| "when": { | ||
| "all_of": [ | ||
| {"any_of": [ | ||
| {"diff_contains": "google.adk"}, |
There was a problem hiding this comment.
[P1] Existing ADK tool-list edits still bypass this trigger. This leg requires google.adk/google-adk to occur in the unified diff, but an ordinary edit adding a tool to an existing LlmAgent(..., tools=[...]) list does not repeat the distant import. On this head, a hunk containing LlmAgent(, tools=[, and +delete_account returns should_run=false with skip_reason="no_match". That contradicts the published “Adds/changes … tools list | Yes” contract and leaves the common modified-list case unfixed. Please derive ADK context from the head file/existing detector (or another source-aware signal) and add a regression test that modifies an existing list rather than adding the whole file.
There was a problem hiding this comment.
Confirmed and fixed in 735d003. Reproduced your exact case first — a hunk with LlmAgent(, tools=[, and +delete_account returned should_run=false, skip_reason="no_match" on the previous head.
You identified the root cause precisely: git diff carries three lines of context, so the constructor and the list are in the hunk and the import is dozens of lines outside it. Requiring the import meant the rule covered whole-file additions and nothing else — which does contradict the "Adds/changes" row I published.
The context leg is now a disjunction:
{"all_of": [
{"diff_contains": "tools=["},
{"any_of": [
{"diff_contains": "LlmAgent("},
{"all_of": [
{"any_of": [{"diff_contains": "google.adk"}, {"diff_contains": "google-adk"}]},
{"diff_contains": "Agent("}
]}
]}
]}LlmAgent( carries the modified-list case on its own — no other supported framework exports that class name, so it is an ADK-identifying signal that survives outside the import hunk. Regression test test_triggers_google_adk_modified_tools_list_routes_run uses a real modified-list hunk (-tools=[lookup_account] / +tools=[lookup_account, delete_account]) rather than a whole-file addition.
On deriving context from the head file. I did not take that route, and want to be explicit about why rather than have it look like an oversight. The catalog's contract is that an agent applies it to a PR diff; the vocabulary has no predicate that reads head-tree content (file_present is real vocabulary, but the evaluator implements it only for shipgate.yaml, keyed off manifest_present). Generalizing it means new predicate semantics and a schema_version bump, which every external consumer that pre-fetched triggers.json would then have to implement — the surface expansion #315's own scoping ruled out. LlmAgent( is a source-aware signal that closes the reported case inside the existing vocabulary.
Residual gap, stated rather than hidden. A modified list on the Agent alias (Agent(..., tools=[...]) with no ADK token in the hunk) is still unmatched. It was also unmatched on the base catalog, so it is not a regression, but it is real. I deliberately did not close it by widening: Agent( + tools=[ is indistinguishable from CrewAI's Agent(role=..., tools=[...]) by diff text alone, and routing CrewAI under a rule ID naming Google ADK would be precisely the "states a conclusion its evidence does not support" defect this PR exists to fix. test_triggers_bare_agent_call_without_adk_context_does_not_claim_adk pins that boundary. The gap is documented in the rule rationale and in the AGENTS.md trigger section.
Closing it properly needs the head-file signal you suggested. Happy to open a follow-up issue for a source-aware predicate (catalog schema_version bump, head-tree read, external-consumer contract) if you want that as its own change.
| {"glob": "**/pyproject.toml"}, | ||
| {"glob": "**/setup.py"}, | ||
| {"glob": "**/setup.cfg"}, | ||
| {"glob": "**/requirements*.txt"}, |
There was a problem hiding this comment.
[P2] This closed manifest allowlist regresses real framework bumps. Common pip-tools inputs such as requirements.in and constraints.in are omitted. For requirements.in with google-adk==1.0.0 changed to 1.1.0, the base catalog sets dry_run_recommended=true, while this head returns skip_reason="no_match" and dry_run_recommended=false. That contradicts the CHANGELOG claim that real-bump coverage is unchanged. Please include requirements*.in/constraints*.in (and review modern lock formats such as pylock.toml/pdm.lock), preferably via a shared canonical manifest set, with regression fixtures.
There was a problem hiding this comment.
Confirmed and fixed in 735d003. Reproduced exactly as you described — requirements.in with google-adk==1.0.0 → 1.1.0:
| catalog | verdict |
|---|---|
base (origin/main) |
dry_run_recommended: true, TRIGGER-FRAMEWORK-VERSION-BUMP |
| previous head | skip_reason: "no_match", dry_run_recommended: false |
| now | dry_run_recommended: true, TRIGGER-FRAMEWORK-VERSION-BUMP |
Same for constraints.in, pylock.toml, and pdm.lock. You are right that this contradicted the CHANGELOG claim; that sentence was false and is corrected — real-bump coverage is now wider than before this PR, not "unchanged". The failure mode was exactly what you named: a closed allowlist written from memory, which silently regressed every pip-tools repository, where the bump is authored in .in and compiled to .txt.
Taken the shared-canonical-set route you preferred. New DEPENDENCY_MANIFEST_GLOBS in src/agents_shipgate/core/dependency_manifests.py is now the single answer to "is this a file where a dependency version is declared or locked?", with is_dependency_manifest(path) alongside it. docs/triggers.json projects the tuple, and test_dependency_manifest_projection_matches_runtime_set asserts the catalog's glob leg equals the constant element-for-element — the same drift guard boundary_adapters already gets from test_trigger_boundary_adapter_projection_matches_runtime_registry. Adding a manifest in one place without the other now fails CI.
Coverage added beyond the four you flagged, keeping the "declares or locks a version" rule as the membership test:
- pip-tools, both halves —
requirements*.in,requirements/*.in,constraints*.in(plus the existing.txtforms) - modern Python locks —
pdm.lock, PEP 751pylock.tomlandpylock.*.toml(the spec permits the named form) - conda —
environment.yamlandconda-lock.{yml,yaml}alongside the existingenvironment.yml - Node —
npm-shrinkwrap.json,bun.lock,bun.lockbalongside npm/pnpm/yarn - JVM —
gradle/libs.versions.tomlversion catalogs alongsidepom.xmland the Gradle build files, sinceconductor-ossis a JVM token
Regression fixtures: _DEPENDENCY_MANIFEST_SAMPLE_PATHS holds one representative path per glob (34 paths, including nested spellings like services/api/pyproject.toml and requirements/base.in). Each is parametrized through test_dependency_manifest_samples_are_recognized_by_both, asserting both that is_dependency_manifest() matches it and that a framework bump there routes dry_run_recommended: true — so a glob that reads plausibly but matches nothing fails loudly instead of leaving a silent hole in the direction you found. test_non_manifest_paths_are_not_dependency_manifests is the negative control (README.md, Dockerfile, src/agent.py stay out: a file that merely mentions a package cannot support the claim that a dependency changed).
Full suite green (exit 0) and ruff check . clean on the rebased tree.
…ange An ADK sample whose root agent is `LlmAgent(name="support_agent", tools=[lookup_account, delete_account])` routed as `dry_run_only`. The only rule that fired was `TRIGGER-FRAMEWORK-VERSION-BUMP`, on a raw `google-adk` string in the diff, and it reported the result as a framework upgrade. Two defects sat behind that. Catalog drift, not a missing capability: detection (`GOOGLE_ADK_AGENT_CLASSES`), the ADK adapter, and the binding graph all resolve this shape already — `docs/triggers.json` was the one component with no ADK rule. Plain functions passed to `tools=[...]` carry no decorator, so `@function_tool` / `FunctionTool(` never sees them. `TRIGGER-GOOGLE-ADK-AGENT-TOOLS-CHANGED` conjoins a `google.adk` module path, an `Agent(` / `LlmAgent(` construction, and a `tools=[...]` argument — static, and drawn from the engine's own signal vocabulary. A rule stating a conclusion its evidence cannot support: nothing about the string `google-adk` establishes that a version moved. It comes just as easily from install prose or a sample import, which is why a docs-only change could be classified as a framework upgrade. `TRIGGER-FRAMEWORK-VERSION-BUMP` now needs both halves of its claim — the package token AND a changed dependency manifest — and its rationale reports the co-occurrence it observed rather than the upgrade it inferred. Real bumps are unaffected. The spaced form `tools = [` is deliberately not watched: `diff_contains` is a substring match, so it also swallows `enabled_tools = [...]` in a Codex TOML config — a token with no structural meaning, which is the reporting defect this change is fixing. Rule IDs and the catalog `schema_version` (0.3) are unchanged; this is rule precision inside the existing schema, so a pre-fetched catalog keeps working. Fixes #315 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…p routing Addresses both PR review findings, each reproduced first. [P1] An edit adding a tool to an existing `LlmAgent(..., tools=[...])` returned `no_match`. The rule required a `google.adk` token in the diff, but `git diff` shows three lines of context — the constructor and the list are in the hunk, the import is dozens of lines away and is not. So the rule covered whole-file additions only, and contradicted the published "Adds/changes ... tools list | Yes" row for every subsequent edit to the same list. The context leg is now a disjunction: `LlmAgent(` on its own, since no other supported framework exports that class name, OR a `google.adk` module path together with `Agent(`. `Agent(` stays gated because CrewAI constructs `Agent(..., tools=[...])` too, and routing that under a rule ID naming Google ADK would be the same overstatement this PR is fixing. Residual gap, documented in the rule and AGENTS.md: a *modified* list on the `Agent` alias, which diff text alone cannot attribute to ADK. [P2] The hand-written manifest allowlist dropped real bumps. pip-tools repositories author a bump in `requirements.in` and compile it to `requirements.txt`; `google-adk==1.0.0` -> `1.1.0` in a `.in` file went from `dry_run_recommended: true` on the base catalog to `no_match` here. `pylock.toml` and `pdm.lock` were missing for the same reason: a closed list written from memory. The set now lives in one place — `DEPENDENCY_MANIFEST_GLOBS` in `core/dependency_manifests.py` — covering both halves of the pip-tools pair, the modern lockfiles, and Node/JVM manifests for the JS and `conductor-oss` tokens. `docs/triggers.json` projects it, and a contract test pins the projection against the constant, the same drift guard `boundary_adapters` already had. Real-bump coverage is now wider than before this PR, not merely preserved; the CHANGELOG claim that it was "unchanged" was false and is corrected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
145f083 to
735d003
Compare
|
Both findings were reproducible as written, and both are fixed in 735d003. I reproduced each against the base catalog before changing anything, so the fix and the regression both have evidence. [P1] Modified ADK tool lists. Root cause was exactly as you diagnosed: the rule demanded a [P2] Dependency-manifest allowlist. A genuine regression against base —
One residual gap I want on the record rather than buried. A modified tools list on the Also rebased onto On the trust-root review: expected and correct — the PR edits |
Summary
LlmAgent(name="support_agent", tools=[lookup_account, delete_account])— two directly reachable tools, one of them destructive — routed asskip_reason: "dry_run_only". The only rule that fired wasTRIGGER-FRAMEWORK-VERSION-BUMP, on a rawgoogle-adkstring somewhere in the diff, and it reported the result as a framework upgrade.TRIGGER-GOOGLE-ADK-AGENT-TOOLS-CHANGED(surface_class: capability,run_shipgate). This was catalog drift, not a missing capability: detection (GOOGLE_ADK_AGENT_CLASSES = {"Agent", "LlmAgent"}incli/discovery/signals.py), the ADK adapter (inputs/google_adk.py), and the binding graph all resolve this shape already —docs/triggers.jsonwas the one component carrying no ADK rule at all. Plain functions handed totools=[...]carry no decorator, so@function_tool/FunctionTool(never sees them, and ADK's most common agent spelling had no positive route. The new rule conjoins agoogle.adkmodule path, anAgent(/LlmAgent(construction, and atools=[...]argument, using the engine's own signal vocabulary.TRIGGER-FRAMEWORK-VERSION-BUMP. Nothing about the stringgoogle-adkestablishes that a dependency version moved; it comes just as easily from install prose or a sample import, which is why a docs-only change could be classified as a framework upgrade. The rule now requires both halves of its claim — the package token and a changed dependency manifest (pyproject.toml,requirements*.txt,package.json, a lockfile,pom.xml, …) — and its rationale reports the co-occurrence it observed rather than the upgrade it inferred. Coverage of real bumps is unchanged.Both changes stay inside the current catalog
schema_version(0.3, as set by #340), add no predicate vocabulary, and keep every existing rule ID, so an external agent that pre-fetchedtriggers.jsonkeeps working. No new public surface — this closes a gap between the catalog and the engine's existing ADK detection.Rebased onto
mainafter #330, #331, and #340 landed; the full suite andruff check .were re-run green on the new base, and the reproduction below was re-confirmed there.Type
triggerverdict andverify --previewrouting)Verification
CI is authoritative for
python -m ruff check .,python -m compileall -q src tests, andpython -m pytest.Additional local checks run:
Reproduced the reported defect and its fix against the real PR. Pulled all 9 files and their actual patches from
google/adk-python#6605viagh apiand ran the evaluator against the pre-change catalog and this branch's:should_run/run_shipgatefalsetrueskip_reasondry_run_onlynullTRIGGER-FRAMEWORK-VERSION-BUMP(dry_run)TRIGGER-GOOGLE-ADK-AGENT-TOOLS-CHANGED(run)diff_tokens["google-adk"]["Agent(", "LlmAgent(", "google-adk", "google.adk", "tools=["]End-to-end through the issue's own command. Built a git repo carrying the
contributing/samples/agent_hooks/agent.pysample and ranagents-shipgate verify --preview --workspace . --base … --head … --format jsonplusagents-shipgate trigger --base … --head … --json. Both returnshould_run: true,run_shipgate: true,skip_reason: null,dry_run_recommended: false.All four acceptance criteria from the issue confirmed by hand, including the ADK quickstart spelling (the
Agentalias), the docs-onlygoogle-adkmention,FunctionTool(/LongRunningFunctionTool(retention, and a real dependency bump still routingdry_run.Full suite green (
pytest tests/exit 0),python -m ruff check .clean,docs/triggers.jsonparses.llms-full.txtregenerated withpython scripts/build-llms-full.py(AGENTS.md is its source).One finding worth a reviewer's attention
The first draft of the ADK rule also watched the spaced form
tools = [.tests/test_codex_boundary_check.pycaught it:diff_containsis a plain substring match, so that token also swallowsenabled_tools = [...]in a Codex TOML config, and the token started appearing indiff_tokenson unrelated boundary checks. That is a smaller instance of exactly the defect this PR is fixing — a token with no structural meaning being published as if it were structure — so the spaced form is deliberately not matched. The reasoning is recorded in the rule's own rationale and pinned bytest_triggers_do_not_watch_the_spaced_toml_tools_array_token.Consequence of dropping it: an ADK agent that builds its list separately (
tools = [...]thenAgent(..., tools=tools)) is not matched by this rule. That is the conservative side of the trade, and the issue scopes the fix totools=[...].Coverage
8 new tests in
tests/test_public_surface_contract.py, alongside the existing evaluator contract tests:#6605shape routesrun_shipgateand does not match the dependency rule;Agentalias routes the same way;FunctionTool(/LongRunningFunctionTool(still carried byTRIGGER-FUNCTION-TOOL-DECORATOR;google-adkmention is not classified as a version bump (and still reports the token indiff_tokens, which draws no conclusion);pyproject.toml,requirements-dev.txt,services/api/package.json,uv.lock;whenkeeps a path leg, so reverting it to a bareany_ofof tokens fails CI;Note for the reviewer
This PR changes
docs/triggers.json, so Shipgate's own gate will raiseSHIP-VERIFY-TRIGGER-CATALOG-DRIFT(medium → human review). That is the check working as designed — it exists so a human confirms a catalog edit does not carve out a gate-evasion path. The two rule changes here move in opposite directions on that axis: one adds a positive route that did not exist, and the other narrows an advisory rule so it stops claiming evidence it does not have. Neither removes arun_shipgateroute.Release-readiness notes
diff_contains/globpredicates only; nothing imports, executes, or fetchesdocs/checks.md— n/a, no check IDs added or changedSTABILITY.md— n/a, no report or schema change; catalogschema_versionstays0.3and every rule ID is preserved🤖 Generated with Claude Code