Skip to content

fix(addie): surface compliance diagnostics and bump SDK#5763

Merged
bokelley merged 1 commit into
mainfrom
review-prs-and-issues-v1
Jun 30, 2026
Merged

fix(addie): surface compliance diagnostics and bump SDK#5763
bokelley merged 1 commit into
mainfrom
review-prs-and-issues-v1

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

  • Bump @adcp/sdk to 9.3.0 so seed_account routes through the SDK fix for seed.account.
  • Classify malformed supported_protocols near-misses distinctly in Addie compliance tools, e.g. media-buy vs media_buy.
  • Surface first-failure validation details in the registry API and dashboard storyboard drilldown for owner/debug views.
  • Fix date-only brand rights end-date validation to match the emitted end-of-day rights_constraint.valid_until.
  • Run server unit tests through the existing server Vitest config so shared server singletons are not tested with file parallelism.

Validation

  • npx vitest run server/tests/unit/capability-resolution-error-classifier.test.ts tests/addie/member-tools.test.ts server/tests/unit/compliance-db-last-write-wins.test.ts
  • npx vitest run tests/addie/brand-sandbox-tools.test.ts -t "rights_constraint uses date-time format" --pool=threads
  • npm run test:server-unit
  • npm run typecheck
  • node scripts/check-changeset-protocol-scope.cjs origin/main
  • git diff --check
  • pre-commit hook during commit
  • pre-push hook during push, including current and 3.0 compatibility storyboard matrices

No changeset: no protocol release-surface changes detected; scope check passes.

@mintlify

mintlify Bot commented Jun 30, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
adcp 🟢 Ready View Preview Jun 30, 2026, 6:21 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@bokelley bokelley marked this pull request as ready for review June 30, 2026 05:50

@aao-release-bot aao-release-bot 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.

Code's clean and well-fenced. It can't merge in this state, and one hunk is a stale rewrite of a fix that already landed. Rebase and drop that hunk, and this is an approve.

Why this isn't an approve yet

The PR is CONFLICTING / DIRTY. The conflict is server/src/training-agent/brand-handlers.ts:940. This hunk rewrites the old const endMs = new Date(campaign.end_date).getTime() block to append T23:59:59Z, but main already shipped #5760 (16e3d58f) which rewrote the same block to compare against startOfTodayMs. The PR branched before #5760 merged.

The two fixes are not equivalent. #5760 keeps a campaign ending today licensable by comparing endMs < startOfTodayMs (00:00:00Z today). This PR's version compares end_date+23:59:59Z against Date.now(). Resolving the conflict by taking this side silently reverts #5760's deliberate calendar-day semantics. Drop the brand-handlers change entirely, keep #5760, rebase. That clears the conflict and the regression in one move.

Things I checked

  • Prompt-injection surface holds. declaredProtocol and expectedProtocol come from the remote agent's supported_protocols. Both are sanitized at the source via sanitizeClassifiedValue(raw, 80) (compliance-testing.ts:296), then double-fenced through fenceAgentValue before reaching Addie's markdown (member-tools.ts:4789, 5040). Headline asserts single-line. security-reviewer: no High/Medium.
  • The re-probe is not a new SSRF or auth-leak target. classifyCapabilityResolutionErrorWithDeclaredProtocols (member-tools.ts:580) re-hits the same resolved.resolvedUrl with the same authOption the main comply() path already used this invocation. Single-shot, try/caught, logs probeError not the body. Gated to kind === 'specialism_parent_protocol_missing'.
  • Diagnostics stay owner-gated. first_failure_validations rides the same includeDiagnostics flag as first_failure_message; unauthorized callers get []. The two omitting callers are an is_owner path and the caller's own just-run serialization, not cross-tenant reads. security-reviewer confirmed tenant isolation.
  • LATERAL join keys correctly. agent_url AND run_id AND storyboard_id, failed_validations_jsonb IS NOT NULL, deterministic ORDER BY d.captured_at DESC, d.id DESC LIMIT 1. NULL run_id comparison yields [] safely. No app-level N+1 — it's one statement on the bulk path.
  • No changeset required. No static/schemas/source/**, no docs/reference/**, no wire surface. Internal server diagnostics only. Scope check claim is sound.

Follow-ups (non-blocking — file as issues)

  • first_failed_step_id IS NULL attaches an arbitrary step's validations (compliance-db.ts:1077, 1199). When the first-failed step id is null, the LATERAL returns the most-recent failing step's validations, which the UI then labels as the "first failure." Deterministic via d.id DESC, best-effort owner-debug surface — but the label implies a precision it doesn't have.
  • Raw jsonb array is uncapped in the REST response (registry-api.ts:394). normalizeValidationList passes the agent-influenced payload straight through z.array(z.any()); the UI slices to 3 client-side but the API returns the full array. Owner-only and escaped on render, so not a leak — consider a server-side length cap for symmetry.
  • Confirm typecheck is green on a clean SDK 9.3.0 install. knownProtocolsFromIndex now reads index.protocols?.map(p => p.id) (compliance-testing.ts:275). Runtime-safe via ?./?? [], but it depends on ComplianceIndex.protocols existing on the 9.3.0 type. PR claims typecheck passed; let CI on a clean install confirm.

Minor nits (non-blocking)

  1. Script path form is inconsistent. test:server-unit now passes tests/unit/ (config-root-relative) while sibling test:server-integration passes the server/-prefixed server/tests/integration (package.json:94). Both resolve via root: __dirname, but the differing conventions read as a mistake.

Rebase onto main, drop the brand-handlers hunk in favor of #5760, and I'll approve.

@bokelley bokelley force-pushed the review-prs-and-issues-v1 branch from 44a73e4 to 53ea4ae Compare June 30, 2026 06:50
@bokelley bokelley changed the title [codex] Surface compliance diagnostics and bump SDK fix(addie): surface compliance diagnostics and bump SDK Jun 30, 2026

@aao-release-bot aao-release-bot 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.

Clean, well-scoped diagnostics surfacing plus a near-miss protocol classifier. The right shape: agent-declared strings stay sanitized at extraction and re-fenced at the markdown sink, and the new diagnostics stay gated behind includeDiagnostics.

Things I checked

  • Classifier precedence (server/src/addie/services/compliance-testing.ts:301-345): nearMissProtocolDeclaration fires on hyphen→underscore confusion (media-buy vs media_buy) before the knownProtocolsFromIndex() smuggling defense, and only when declaredProtocol !== parentProtocol. index.protocols?.map(...) is optional-chained, so a missing SDK field won't throw. Presenter parity at :432-447 is complete across all four sinks; headline asserts no CR/LF.
  • Protocol-id semantics (ad-tech-protocol-expert: sound): canonical supported_protocols enum is snake_case per static/schemas/source/protocol/get-adcp-capabilities-response.json — kebab-case is reserved for compliance URL paths and the task-categorization axis. A hyphen→underscore collapse can only map onto the same logical protocol in a closed 7-value enum, so the heuristic cannot conflate two distinct ids. Guidance is correct in context.
  • Fencing on the LLM path (security-reviewer: no High/Medium): both new unrecognized_supported_protocol branches in member-tools.ts (:4789-4798, :5040-5049) wrap declaredProtocol/expectedProtocol/specialism in fenceAgentValue(..., 80) — double sanitization. The reprobe at member-tools.ts:580-595 re-hits the same validateAgentUrl-checked endpoint (no new SSRF surface) and logs failures via logger.warn rather than leaking to the model.
  • SQL LATERAL joins (compliance-db.ts:1073-1089, :1196-1212): predicate scopes by agent_url/run_id/storyboard_id/step_id; ORDER BY captured_at DESC, d.id DESC LIMIT 1 is fully deterministic (id BIGSERIAL); LEFT JOIN LATERAL ... LIMIT 1 cannot fan out rows. NULL run_id yields NULL validations, which is safe. Only failed_validations_jsonb is pulled — wire-payload columns left out.
  • Test-script fix (package.json:94): vitest run --config server/vitest.config.ts tests/unit/ runs the same server/tests/unit/*.test.ts files but under the server config's fileParallelism: false, which guards the shared pool singleton the old root-config invocation raced. Same tests, run safely.
  • No changeset is correct: changed files are all under server/ plus the SDK bump — none match PROTOCOL_SCOPED_PATHS in scripts/check-changeset-protocol-scope.cjs. No wire-shape change.

Follow-ups (non-blocking — file as issues)

  • No test coverage for the reprobe path. classifyCapabilityResolutionErrorWithDeclaredProtocols and the two unrecognized_supported_protocol tool-handler branches are untested — only the pure classifier/presenter got unit tests. A test exercising specialism_parent_protocol_missing → reprobe → reclassify would lock in the kind-guard, the caps.profile?.supported_protocols ?? [] fallback, and the catch-and-fall-back-to-initial behavior.
  • index.protocols may be a no-op. knownProtocolsFromIndex now reads index.protocols?.map(p => p.id). Worth confirming SDK 9.3.0's index actually ships protocols[], otherwise that half of the cross-check is dead code (the specialisms[].protocol set still gates — not a correctness or security issue).

Minor nits (non-blocking)

  1. Stale PR body. The Summary claims a "date-only brand rights end-date validation" fix, but no rights file is in the diff — that landed as 16e3d58 already on main. Trim the body to the eight files actually changed.
  2. Title isn't conventional-commits ([codex] Surface compliance diagnostics and bump SDK). The underlying commit conforms; if the squash uses the PR title as subject, it won't.

LGTM. Follow-ups noted below.

@bokelley bokelley force-pushed the review-prs-and-issues-v1 branch from 53ea4ae to 77c6b2a Compare June 30, 2026 06:56
aao-release-bot[bot]
aao-release-bot Bot previously approved these changes Jun 30, 2026

@aao-release-bot aao-release-bot 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.

Clean, contained diagnostic surface plus a first-party SDK bump. Right shape: sanitize-at-classify, fence-at-LLM-sink, gate-diagnostics-behind-ownership — the PR follows the existing pattern at every new site.

Things I checked

  • Tenant isolation on the new SQL. Both LEFT JOIN LATERAL blocks in compliance-db.ts (getStoryboardStatuses + bulk variant) scope diagnostics by agent_url + run_id + storyboard_id, and the outer query is already filtered to the requested agent_url. No cross-owner join path. security-reviewer: PASS, no leak.
  • Prompt-injection fencing. Every agent-controlled value in the two new unrecognized_supported_protocol branches (member-tools.ts:4789-4798, 5040-5048) goes through fenceAgentValue; the classifier inputs go through sanitizeClassifiedValue first. Double-protected. The unit test asserts the headline carries no \r\n.
  • Diagnostics gating. first_failure_validations is gated behind includeDiagnostics in serializeStoryboardRunStatus (registry-api.ts:410-412), which resolves to owner/static-admin only. Non-owners get [].
  • test:server-unit path. vitest run --config server/vitest.config.ts tests/unit/ resolves the positional against the config root: __dirname (= server/), landing on the same server/tests/unit/ set. No repo-root tests/unit/ exists, confirming config-relative resolution. The new form additionally inherits fileParallelism: false — which is the stated point (shared server singletons).
  • Protocol-id claim. media_buy (snake_case) is the canonical supported_protocols value per static/schemas/source/protocol/get-adcp-capabilities-response.json; media-buy is not in that enum. Hyphen→underscore is the right normalization direction for this field. ad-tech-protocol-expert: sound.
  • No changeset is correct. No file under static/schemas/** or dist/schemas/** changed; scope check passes. Diagnostic text only, no wire surface touched.

Follow-ups (non-blocking — file as issues)

  • NULL-step fallback can mismatch the message. compliance-db.ts:1075-1084 (and the batch at :1199-1208): when first_failed_step_id IS NULL but failures exist, the lateral returns the most-recent diagnostic for any step in the run, which may not be the step whose first_failure_message the UI renders beside it. Either surface nothing on the NULL branch or note the fallback is intentional.
  • Over-broad message string. "Protocol ids use underscores, not hyphens" is true for supported_protocols but false as a universal AdCP statement — adcp-protocol.json (tasks-list, webhook payloads, compliance storyboard paths) is kebab-case by design. Scope the sentence to the field, e.g. "In `supported_protocols`, protocol ids use underscores." Correct where it fires today; just fragile if reused.
  • Redundant re-probe. classifyCapabilityResolutionErrorWithDeclaredProtocols (member-tools.ts:580-595) fires a second get_adcp_capabilities round-trip in the catch path rather than threading the supported_protocols already fetched in the try block. Only on the rare specialism_parent_protocol_missing kind, wrapped in try/catch with warn-and-fallback — fine to ship, cheap to tighten later.
  • Index coverage. Neither existing diagnostics index fully covers the lateral's (agent_url, run_id, storyboard_id) + ORDER BY captured_at DESC. LIMIT 1 per row keeps it acceptable now; a composite index would make it index-only if the batch endpoint gets hot.

Minor nits (non-blocking)

  1. Ordering comment. nearMissProtocolDeclaration runs before the defense-in-depth known.has(parentProtocol) check (compliance-testing.ts:333-344). Safe because the regex is start-anchored and the string is resolver-produced, but a one-line comment noting the ordering is deliberate would save the next reader the trace.

One observation: the PR body lists a "Fix date-only brand rights end-date validation" bullet and a brand-sandbox-tools.test.ts validation run, but no such change is in this diff — that work landed in 16e3d58 on main. Stale description, not a code problem.

LGTM. Follow-ups noted below.

@aao-release-bot aao-release-bot 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.

Clean, contained diagnostics fix. The classifier near-miss path and the LATERAL-join surfacing are the right shape: a hyphen/underscore protocol-id mismatch (media-buy vs media_buy) gets a specific, actionable diagnosis instead of falling through to unknown_specialism.

Things I checked

  • Classifier ordering (compliance-testing.ts:301-318): near-miss is evaluated before the known-protocol defense-in-depth check. Correct — unrecognized_supported_protocol is the more specific diagnosis, and nearMissProtocolDeclaration only fires when a sanitized declared value normalizes (-_) to the already-regex-validated parentProtocol. An attacker can't smuggle an arbitrary string into declaredProtocol.
  • LLM-context fencing (member-tools.ts:4789-4798, :5040-5048): all four agent-declared strings (declaredProtocol, expectedProtocol, specialism, parentProtocol) are double-sanitized — sanitizeClassifiedValue (strips \r\n, backticks, control chars, 80-char cap) at classify time, then fenceAgentValue at render time. security-reviewer: PASS, no bypass.
  • Data exposure (compliance-db.ts LATERAL JOIN → registry-api.ts first_failure_validations): gated behind canViewAgentDebugData (static-admin or verified owner); non-owners get []. Payloads are redacted at write time via stripValidationRequestResponse + redactForDiagnostics before storage, so the join surfaces an already-redacted column. security-reviewer: PASS.
  • test:server-unit switch (package.json): vitest run --config server/vitest.config.ts tests/unit/ resolves to server/tests/unit/ because the config sets root: __dirname (= server/), and now picks up fileParallelism: false. That's the actual fix — stops the shared db/index.ts pool singleton from being nulled mid-query by a sibling file's afterAll(closeDatabase).
  • index.protocols null-safety (compliance-testing.ts:266-275): index.protocols?.map(p => p.id).filter(Boolean) ?? [] is safe for missing field / missing id. Typecheck passes against SDK 9.3.0.
  • Dashboard render (dashboard-agents.html:1726-1738): agent-controlled values flow through escapeHtml into text/<code> contexts. No XSS.
  • SDK bump 9.2.0→9.3.0 pinned exact with updated integrity hash; no new deps, no install scripts. Changeset omission is defensible — no protocol release-surface change, and the PR body confirms check-changeset-protocol-scope.cjs passes. The static/openapi/registry.yaml delta is generated, not hand-edited.

Follow-ups (non-blocking — file as issues)

  • Nullable run_id → silent empty validations (compliance-db.ts:1078-1086 and the batch query ~:1196-1204): agent_storyboard_status.run_id is nullable (migration 390), so the LATERAL predicate d.run_id = s.run_id never matches when s.run_id IS NULL and first_failure_validations comes back empty even when diagnostics exist. Best-effort is fine, but a one-line comment would save a future "why are validations missing" investigation.
  • first_failed_step_id IS NULL fallback (compliance-db.ts:1080-1086): with no first-failed step, the join attaches the most-recent (captured_at DESC, id DESC) failing diagnostic for any step of the run. The surfaced validations then don't correspond to a named step. Acceptable as best-effort.
  • Redaction by key-name/format only (compliance-testing.ts:1086-1103, pre-existing — not regressed here): a non-standard plaintext secret echoed by a hostile agent into a validation expected/actual under a benign key name survives to storage. Exposure is owner/admin-only and request/response blobs are already stripped, so impact is bounded. Worth a belt-and-suspenders pass if this view widens.

The body still lists a "date-only brand rights end-date validation" fix that isn't in this diff — landed separately in 16e3d58 on main. Stale bullet, not a blocker.

LGTM. Follow-ups noted below.

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