Degrade OpenAPI action_id collisions instead of crashing the scan#226
Merged
Conversation
7a11f38 to
9160a28
Compare
… scan
A valid third-party OpenAPI spec (block/goose) crashed `agents-shipgate
scan` with `exit 2 / Config error: Duplicate action_surface action_id`.
The OpenAPI action_id is derived from METHOD + normalized path only —
the operationId is dropped — so two distinct operations whose paths
normalize identically (goose ships `GET /sessions/{session_id}` plus a
trailing-slash variant, each with its own operationId) collapse to one
action_id and hard-fail the duplicate-id guard.
Make this fail-soft, matching the symlink-loop (#212) and MCP-as-tools
(#214) precedent: a third-party spec must never crash a scan.
- build_action_surface_facts takes an optional `warnings` sink. On the
live scan path, collisions between purely INFERRED ids (e.g. OpenAPI
method+path normalization) are disambiguated deterministically (first
operation by tool_name keeps the bare id, the rest gain a
`#<operationId>` suffix, ordinal fallback on ties), the identity_hash is
refreshed, and one warning is recorded per collision.
- A collision that touches a manifest-authored action identity stays a
hard ConfigError on both paths — a declared id is a contract, never
silently rewritten. "Manifest-authored" means any action_surface
declaration that sets an explicit action_id component: `id`, `provider`,
or `operation` (all three feed build_action / _provider / _operation).
Without a sink the legacy ConfigError is preserved for all collisions,
so the redaction path's public-only ordinal disambiguator is untouched.
- Thread the warnings from Phase 5 into report.source_warnings (appended
last, so byte-stable for the common no-collision case). A source_warning
routes the release to review_required rather than crashing.
- Add network-free regression tests: inferred OpenAPI collisions degrade;
explicit collisions via `id` and via `provider`/`operation` still raise
on both the no-sink and warnings-sink paths; plus a full run_scan
integration test on the goose-shaped fixture.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9160a28 to
08dfa76
Compare
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.
Summary
agents-shipgate scanwithexit 2 / Config error: Duplicate action_surface action_id. The OpenAPIaction_idis derived fromMETHOD + normalized_pathonly — theoperationIdis dropped — so two distinct operations whose paths normalize identically (goose shipsGET /sessions/{session_id}plus a trailing-slash variant, each with its ownoperationId) collapse to oneaction_idand hard-fail the duplicate-id guard.source_warning(which routes the release toreview_required) while keeping both operations as distinct actions.build_action_surface_factsgains an optionalwarningssink. When provided (the live scan path),_disambiguate_duplicate_action_idsdeterministically resolves collisions: the first operation (bytool_name) keeps the bare id, the rest gain a#<operationId>suffix (ordinal fallback whenoperationIds also tie),identity_hashis refreshed, and one warning is recorded per collision. Without a sink the legacyConfigErroris preserved, so the redaction path's existing public-only ordinal disambiguator (surface_redaction.py) is untouched.decision.py→_ChecksDecision→sanitization.py) intoreport.source_warnings, appended last so output stays byte-stable for the common no-collision case (the list is empty there).Type
Verification
CI is authoritative for
python -m ruff check .,python -m compileall -q src tests, andpython -m pytest.Additional local checks run:
pytestsuite passes (exit 0);tests/test_adapter_static_only.pystatic-only invariant holds;ruff checkclean on all changed files.test_action_surface_disambiguates_openapi_action_id_collisions_fail_soft(unit) andtest_openapi_action_id_collision_degrades_instead_of_crashing(fullrun_scanon the goose-shaped fixture). The existingtest_action_surface_rejects_action_id_collisions(no sink →ConfigError) still passes.source_warningand two distinctaction_ids, instead of the previousexit 2 / Config error.Release-readiness notes
docs/checks.md— n/a, no new check IDsSTABILITY.md— no schema change;source_warningsonly gains an extra entry in the (previously crashing) collision case🤖 Generated with Claude Code