Skip to content

fix(compliance): avoid valid_actions advisory false positives#2172

Open
sangilish wants to merge 1 commit into
adcontextprotocol:mainfrom
sangilish:sangilish/fix-valid-actions-advisory-scan
Open

fix(compliance): avoid valid_actions advisory false positives#2172
sangilish wants to merge 1 commit into
adcontextprotocol:mainfrom
sangilish:sangilish/fix-valid-actions-advisory-scan

Conversation

@sangilish
Copy link
Copy Markdown
Contributor

Summary

Tests

  • npm run build:lib
  • NODE_ENV=test node --test-timeout=60000 --test test/lib/comply-advisory-rule-source.test.js
  • npm run typecheck
  • npm run test:lib:fast
  • npx prettier --check src/lib/testing/compliance/comply.ts test/lib/comply-advisory-rule-source.test.js .changeset/valid-actions-advisory-scan.md
  • git diff --check -- src/lib/testing/compliance/comply.ts test/lib/comply-advisory-rule-source.test.js .changeset/valid-actions-advisory-scan.md

Follow-up for adcontextprotocol/adcp#5319.

@sangilish sangilish marked this pull request as ready for review June 4, 2026 21:52
@aao-ipr-bot
Copy link
Copy Markdown

aao-ipr-bot Bot commented Jun 4, 2026

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

This is an automated message from the Argus AI review workflow.

Copy link
Copy Markdown
Contributor

@bokelley bokelley left a comment

Choose a reason for hiding this comment

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

Thanks for the narrow runner fix. I think this still misses the response shape from adcontextprotocol/adcp#5319, so I’d request one more change before merge.

collectObservations() now scans all get_media_buys observations, but the predicate only checks top-level obs.valid_actions:

const hasAnyValidActions = getMediaBuyObservations.some(
  ({ obs }) => obs.valid_actions !== undefined && obs.valid_actions !== null
);

The reported case has valid_actions inside each returned buy object, i.e. observation_data.media_buys[].valid_actions. Storyboard-backed compliance maps the full step response into observation_data, so a valid get_media_buys response like this still triggers missing-valid-actions:

{
  "media_buys": [
    {
      "media_buy_id": "mb1",
      "status": "pending_start",
      "valid_actions": ["cancel"]
    }
  ]
}

I confirmed this against the PR head with a lightweight tsx repro: two get_media_buys observations where every returned buy had nested valid_actions still produced one missing-valid-actions advisory.

Can you update the predicate and tests to cover both shapes?

  • compact scenario observation data: observation_data.valid_actions
  • real/storyboard response data: observation_data.media_buys[].valid_actions

The new regression tests currently only exercise the top-level synthetic shape, so they don’t catch the issue case.

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.

2 participants