fix(compliance): route hosted 3.0 basic auth runs#5194
Conversation
898b4d7 to
3784b89
Compare
3784b89 to
169b27c
Compare
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Right shape for a temporary SDK compatibility shim: hash-pinned, fail-closed, idempotent, marker-versioned (V2 over V1), and the patch body itself is in-repo so reviewers can audit the resulting runtime.
Things I checked
withHostedAuthTestKit(server/src/services/hosted-compliance-version.ts:327-360) preserves caller-suppliedtest_kit.auth.{api_key|basic|probe_task}and only injects the auto probe-task marker whendeclaresStaticAuthis true. Bearer-vs-Basic don't fight: the patched SDK dispatches on the storyboard step'sauth.type, not on credential presence, so both can coexist for steps that need different directives.- Idempotency across
npm postinstalland the two DockerRUNcalls (Dockerfile:15, 109-112): oncePATCH_MARKERis present in the SDK runner source, all fourpatch*helpers inscripts/patch-adcp-sdk-basic-auth.cjs:344-468short-circuit tostatus: undefined→result.status === 'already-patched'→ no write. - Fail-closed gates at
scripts/patch-adcp-sdk-basic-auth.cjs:507-510(SDK version pin) and per-region SHA-256 (EXPECTED_*_SHA256constants). A future@adcp/sdkbump bricksnpm installloudly with a guiding error — preferred over a silent partial patch. - Basic-auth header construction at
scripts/patch-adcp-sdk-basic-auth.cjs:217-235: rejectsstep.auth.value(literal creds disallowed in storyboards), enforces RFC 7617 colon rule on username, and rejects CR/LF/NUL plus non-printable ASCII before base64 — error strings name the field but never echo the value. tests/patch-adcp-sdk-basic-auth.test.cjs:1034-1045re-derives all four expected hashes against the in-fixture SDK preimage, so a fixture/constant drift is caught locally before deploy.- CI: TypeScript Build, Server integration tests, all Storyboards matrices (current + 3.0-compat across signals/sales/governance/creative/creative-builder/brand), CodeQL, Semgrep, GitGuardian, Migration Smoke all green. Changeset Check green.
- Test plan: 11 reproducible npm invocations listed; no live-agent end-to-end against an
Authorization: Basicstep, but the patched function is executed in-sandbox attests/patch-adcp-sdk-basic-auth.test.cjs:917-925. Acceptable for a patch-shape change; CI's 3.0-compat storyboard matrix is the safety net.
Follow-ups (non-blocking — file as issues)
- Empty changeset frontmatter.
.changeset/basic-auth-prod-runs.mdhas---\n---with no"adcontextprotocol": patchline. Changeset Check CI passes (it verifies presence, not contents), and this PR is server-only + forward-merge of already-released artifacts, so no version bump is the defensible outcome — but every sibling under.changeset/declares a package + level. Either add"adcontextprotocol": patchor land a one-paragraph convention note so the empty case isn't mistaken for an oversight on the next forward-merge. - Fail-closed test matrix covers 1 of 4 hash-pinned regions.
tests/patch-adcp-sdk-basic-auth.test.cjs:906-913exercisesauthHeadersForStephash-mismatch;resolveTaskName, theassert_contributionsnippet, and therunStoryboardStepprofile snippet have no equivalentassert.throws(... /hash mismatch/). Add three more cases so the SDK-bump regression is caught in a unit run, not innpm installon the deploy host. - Unicode passwords rejected. The header validator at
scripts/patch-adcp-sdk-basic-auth.cjs:171-175, 251-253rejects any non-printable ASCII. RFC 7617 explicitly recommends UTF-8 forAuthorization: Basic. The choice is defensible (undici's header validator is strict) but worth documenting in the test-kit auth docs — adopters with accented or non-Latin passwords will hit a hard failure at probe time with no obvious fix path. - Defense-in-depth on patched
resolveTaskName. The patched body mutatesoptions.test_kit.auth._probe_task_unsupported = trueon the caller's object (scripts/patch-adcp-sdk-basic-auth.cjs:290-292in the patched snippet). Safe today becausewithHostedAuthTestKitbuilds a fresh shallow copy per call, but a future caller that reuses anoptionsobject across runs would carry the flag forward. Comment the invariant onwithHostedAuthTestKitor deep-copyauthbefore threading it through. _probe_task_autotrust boundary. The marker is intended as an internal signal butwithHostedAuthTestKitreadscurrentKit.auth ?? {}and would preserve a caller-supplied_probe_task_auto: true. All current call sites build options literally and never passtest_kit.auththrough from a request body — worth a one-line invariant comment so future contributors don't open that path.
Minor nits (non-blocking)
- Stray indent on legacy assert-contribution refresh.
scripts/patch-adcp-sdk-basic-auth.cjs:399-403inserts\nafterpatchedAssertContributionSnippet(), but the originallegacyCurrentslice already includes the leading whitespace before the nextelse if. Cosmetic — bounded by the marker check on the next run — but a follow-on legacy → V2 refresh against a fresh checkout would leave a stray blank-ish line. Drop the trailing\nor trim the leading whitespace fromsource.slice(legacyEnd).
Notable that this lands as a patch script with V2 markers already in place — the V1 → V2 refresh paths are tested, which is the right way to ship a temporary shim that's going to get edited a few more times before @adcp/sdk catches up.
Safe to merge.
169b27c to
1972ff2
Compare
There was a problem hiding this comment.
Shape looks right — credential mirror is well-scoped, three call sites route through it consistently, SDK bump trades the local patch for upstream beta.17. One question on emitFrameworkTaskWebhook gates my approval.
Things I checked
withHostedAuthTestKit(hosted-compliance-version.ts:326-360) — mirrors bearer/basic only, no-clobber on pre-settest_kit.auth, returns input unchanged when nothing to do. Three call sites (withHostedComplianceRunOptions,withHostedStoryboardRunOptions,withHostedTestOptions) all route through it.3.1-rc.4allowlist additions land in all three places (task-handlers.ts:591,router.ts:50, plus six test files). No stale3.1-beta.7literals left.- SDK bump
8.1.0-beta.16 → beta.17is one line + lockfile, integrity hash recorded, no transitive churn. - Credentials flow only in-memory through the SDK call. No logging of
options.test_kit, no persistence path, no cross-tenant sink. Thetest_kitfield in registry/storyboard/.../runresponses is the brand-only static fixture, not the runtime auth-mirrored kit. - Default
probe_task = 'list_creatives'isreadOnlyHint: true, idempotentHint: true— safe to invoke on the user's behalf with their saved creds. - Empty changeset is correct per playbook: server-only change, no protocol surface.
Open question (gating)
server/src/training-agent/tenants/registry.ts:196-205 — the fallback payloadOperationId = params.operation_id conflicts with the contract documented in server/src/training-agent/webhooks.ts:119-137,194-196: WebhookEmitter.emit({operation_id}) is the seller-side idempotency scope (${principal}|${tool}.${id}) and is explicitly never placed on the wire. The legacy emitter uses extractBuyerOperationId(...) ?? webhookTaskId for the wire payload.operation_id exactly to avoid this.
What does the SDK's framework path actually pass as params.operation_id to taskWebhookEmitter.emit? If it passes the idempotency scope (same convention as legacy, which the shared WebhookEmitParams type implies), this fallback puts a principal-scoped value on the wire to a buyer-controlled receiver. For the training agent that's static:public:shared|... or b:<domain>|... — not a raw token, but tenant fingerprinting that crosses receivers.
Two ways to close it:
- Confirm the SDK's framework path normalizes
params.operation_idto a buyer-safe value before calling the emitter hook. If so, add a one-line comment explaining why the fallback is safe. - Drop the fallback to
params.operation_id. Fall back toparams.payload.task_id, or omit entirely.
Follow-ups (non-blocking — file as issues)
- OAuth path silently skipped.
withHostedAuthTestKit:336-340handlesbearerandbasiconly.resolve-user-agent-auth.ts:36-75producesoauthandoauth_client_credentialsvariants too. Hosted runs against OAuth-protected agents get no test_kit auth wired and no warning. Either intentional (state it in a comment) or a coverage gap. - Per-tenant
probe_taskdefaults.DEFAULT_HOSTED_AUTH_PROBE_TASK = 'list_creatives'is global.run-storyboards.ts:375-378already hasPROBE_TASK_BY_TENANTshowingsignals/governanceneed different tasks. Hosted runs against those tenants will probe a task they don't serve. LoadedTestKit.basicshape drift. Widened type inrun-storyboards.ts:347-351accepts{credentials?: string}(no username/password). The SDK consumes{username, password}. A YAML test kit shipping base64credentialswill silently fail the probe. Either narrow the type to match the SDK, or pre-decode via the existingdecodeBasicCredentialsinserver/src/db/compliance-db.ts:56-67.- Empty-credential guard.
withHostedAuthTestKit:336-340would mirror an empty bearer token or emptyusername/passwordinto the test kit. Fails late with a confusingBasic Og==rather than fast with "no creds." Debuggability nit. RuntimeTestKitAuthis an escape hatch. The intersection athosted-compliance-version.ts:26-33exists because the SDK's static types don't yet exposebasic. Add a TODO pinning the SDK version that will obsolete it. If a future beta ships a typedbasicwith a different shape (e.g.{credentials: base64}), this code compiles and silently sends the wrong thing.
Minor nits (non-blocking)
- Test coverage in
storyboards.test.ts:205-222— bearer + basic happy paths only. Missing cases:options === undefined(early return), pre-settest_kit.auth.api_key(no-clobber),auth.type === 'oauth'(no-op), caller-suppliedprobe_task(no stomp), and theas anycast onauth.basic(line 219) papers over a type seam worth asserting at the structural level. - PR is
mergeable: CONFLICTING— rebase before merge. - Test now asserts
DEFAULT_HOSTED_COMPLIANCE_VERSION === '3.0.14'(storyboards.test.ts:167,178) — fine for now but will drift when the cache advances. Either pin the line (/^3\.0\.\d+$/, already done at L179) and drop the exact literal, or accept that this test goes stale on the next cache bump.
Holding on the operation_id question. LGTM once that's confirmed or the fallback is tightened.
1972ff2 to
cce248f
Compare
There was a problem hiding this comment.
Approving. Mirroring stored creds into test_kit.auth is the right shape — the SDK's beta.17 static-auth probes can now actually authenticate against the 3.0.14 receiver in hosted/Addie runs, and profile-aware probe-task selection stops forcing list_creatives on signals and governance agents.
Things I checked
withHostedAuthTestKitinserver/src/services/hosted-compliance-version.ts— mutation-safe:currentKit,currentAuth, andauth.basicare all spread / re-objected, no shared-reference writes; the!nextAuth.probe_taskguard preserves caller-supplied overrides. Ordering ofwithHostedAuthTestKit→withHostedComplianceOptionsis fine: the latter only touchesversion/complianceDir/schemaRoot, nevertest_kit.frameworkWebhookContext.enterWith(...)inserver/src/training-agent/tenants/registry.tsis contained today: every MCP request is wrapped inrunWithSessionContext(tenants/router.ts:299, :364), which provides the.run()boundary that scopes theenterWitheffect to that request's async chain.security-reviewerwalked the request entry points and confirmed no cross-tenant leak. Fragile, but safe.hostedAuthProbeTaskForProfile— protocol map first, then fallback allow-list, then'list_creatives'. A__proto__injection insupported_protocolslands onObject.prototype, throws inside.find, and is caught by all three callers — only the eight allow-listed strings or the default are returnable on the wire.3.1-rc.4propagates through every site referencing the prior3.1-beta.7pin:task-handlers.ts,tenants/router.ts:50,tenant-smoke.test.ts,training-agent.test.ts,training-agent-3-0-compat-tools.test.ts. No stale3.1-beta.7left in PR HEAD.- Webhook
operation_idround-trip is covered by the new assertion intests/integration/training-agent-webhooks.test.ts:146. - Changeset frontmatter
---\n---\nmatches the project convention (see.changeset/3573-agent-url-canonicalization-cleanup.md) — informational-only, no workspace bump declared. Not a bug.
Follow-ups (non-blocking — file as issues)
- Basic password may reach production logs via SDK error capture.
compliance-testing.ts:69,member-tools.ts:453, andregistry-api.ts:6297log{err, agentUrl}fromtestCapabilityDiscovery. The pino redact paths inlogger.ts:151list'password'but unwildcarded paths do not match nested keys likeerr.cause.config.headers.authorization. If beta.17's fetch wrapper embeds the failing request intoerr.message/err.stack/err.cause, the base64-encoded Basic credential lands in logs verbatim. Add wildcarded redact paths ('*.headers.authorization','*.config.auth') or wrap the catch site in a stripper before passing to the logger. test_kit.auth.basic.passwordmay persist inrequest_jsonb.compliance-testing.ts:766writesstep.request.payloadto DB. Confirm against beta.17 that the SDK does not echotest_kit.authinto the capturedstep.request.payload; if it does, striptest_kit.auth.api_key/test_kit.auth.basicinsidecapPayloadas defense in depth.operationIdByToolkeyed by tool name corrupts on double-call.tenants/registry.ts— if the same tool fires twice in one request with differentpush_notification_config.operation_idvalues, call #2 overwrites call #1's entry; the framework emitter then resolves call #1 bytask_typeand picks up call #2'soperation_id. Narrow but real. Key by resolvedtask_id, or attachoperation_iddirectly to the framework task record so the emitter reads it back viaparams.task_id.- Discovery 401 is masked by fail-open fallback.
member-tools.ts:443-453andregistry-api.ts:6291-6299swallow discovery errors and default tolist_creatives. If saved auth is wrong, the storyboard probe step also 401s — but the user sees the probe-step error, not the upstream discovery 401. Bubble OAuth-required errors, or annotate the eventual step error with the discovery-failure detail. - Probe-task default fires even when the agent doesn't advertise it.
hosted-compliance-version.ts—HOSTED_AUTH_PROBE_TASK_FALLBACKS.find(...) ?? 'list_creatives'. An older or malformed discovery response that loses bothsupported_protocolsand the eight fallback tool names lands onlist_creativesand 404s with no signal that probe selection was the cause. Returningundefinedand letting the SDK pick its own default would surface a cleaner failure. - Three near-identical
discover → probe-taskwrappers.compliance-testing.ts:57-72,member-tools.ts:442-457,registry-api.ts:6291-6299all do the same auth-narrowing +testCapabilityDiscovery+hostedAuthProbeTaskForProfiledance — notable for a fix PR that's already touching three call sites. Hoist a single helper alongsidewithHostedAuthTestKit. - Extra discovery roundtrip per storyboard step.
member-tools.ts:4628— every individual step click now pays a discovery call. Memoize per(agentUrl, auth)for the Addie session lifetime or per-request via aWeakMap.
LGTM. Follow-ups noted below.
cce248f to
3fedd25
Compare
There was a problem hiding this comment.
LGTM. Follow-ups noted below. The two-step fix — mirror saved creds into test_kit.auth and infer the probe task from the discovered profile — is the right shape: hosted runs no longer force list_creatives on agents that don't expose it.
Things I checked
withHostedAuthTestKit(server/src/services/hosted-compliance-version.ts:291-327) —changedflag correctly avoids mutating when nothing's new;nextAuth.api_key/nextAuth.basiconly set when absent;probe_taskdefaults only whendeclaresStaticAuthbecomes true. No null-deref.hostedAuthProbeTaskForProfile—profile?.tools ?? []is null-safe; always returns a string from the protocol map or the fallback list, never undefined.- Discovery fallback path in all three call sites (
member-tools.ts:447,compliance-testing.ts:67,registry-api.ts:6293) — on failure,authProbeTaskstaysundefined,withHostedAuthTestKitfalls back toDEFAULT_HOSTED_AUTH_PROBE_TASK = 'list_creatives'. Fail-open back to prior behavior, which is correct here. - Tenant isolation on the new pre-discovery call at
registry-api.ts:6288-6297—orgIdstill gatesresolveUserAgentAuth; the newtestCapabilityDiscoveryconsumes the same scopedsdkAuth.validateAgentUrlParamstill in front. - Credential exposure from duplicating tokens into
test_kit.auth— no logger, error response, telemetry, or LLM-context path serializesoptions.test_kit.security-reviewer: clean (one defense-in-depth note, see below). '3.1-rc.4'addition toSUPPORTED_RELEASE_VERSIONSis purely additive —3.1-beta.7preserved. Backwards-compatible. All matching test fixtures updated (training-agent.test.ts,tenant-smoke.test.ts,training-agent-3-0-compat-tools.test.ts).- Empty-frontmatter changeset matches the repo convention for no-package-bump infra changes (cf.
.changeset/3-1-rc-release-notes-audit.md).
Follow-ups (non-blocking — file as issues)
compliance-testing.ts:67callstestCapabilityDiscovery(agentUrl, options)with rawComplyOptions— the other two probe-inference sites wrap viawithHostedTestOptions(..., target). Functionally benign for discovery (we only readcaps.profile), but inconsistent and a foot-gun if the SDK ever validates the probe response againstschemaRoot. Either threadtargetthroughhostedAuthProbeTaskForRunor call throughwithHostedTestOptions.emitFrameworkTaskWebhookprecedence (registry.ts:200) isparams.payload.operation_id ?? params.operation_id. Falls back to the framework-supplied scope value if the SDK ever fails to populate the wire field.webhooks.ts's legacy emitter falls back totask_idinstead. Worth either matching that or adding a one-line comment pinning the contract.- Double round-trip per hosted run — pre-discovery + actual run. Fine for interactive Addie flows; the
comply()path runs across many agents and could cache the profile for the duration of the run. Non-urgent. - Defense-in-depth on
test_kit.auth(security-reviewerLow): the mirrored credentials are enumerable on the same options object the SDK consumes. If a future callerlogger.debug({ options }, ...)lands anywhere on this path, bothoptions.authandoptions.test_kit.authserialize. Addapi_key/basicto the logger redactor allow-list — cheap, future-proofs the refactor.
Safe to merge.
3fedd25 to
e568ab6
Compare
# Conflicts: # server/src/addie/mcp/member-tools.ts # server/src/addie/services/compliance-testing.ts # server/src/routes/registry-api.ts # server/src/training-agent/tenants/registry.ts # server/tests/integration/training-agent-webhooks.test.ts # server/tests/unit/storyboards.test.ts
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Right shape: removes the local SDK patch, plumbs the credential into test_kit.auth exactly where beta.18 expects it, and keeps the probe-task selection inside a closed read-only allowlist.
Things I checked
- Credential flow stays in-tenant.
resolveUserAgentAuth(orgId, agentUrl)andresolveAgentOwnerOrg(req.user.id, agentUrl)gate the lookup atserver/src/routes/registry-api.ts:6286-6307; the same scoping holds atserver/src/addie/mcp/member-tools.ts:4448-4470and:4635-4658. Bearer/Basic only flow to the agent they were issued for. No cross-tenant smuggling path. - Probe-task allowlist is closed.
HOSTED_AUTH_PROBE_TASKS_BY_PROTOCOLandHOSTED_AUTH_PROBE_TASK_FALLBACKSatserver/src/services/hosted-compliance-version.ts:251-269are constant, all read-only (list_*/get_*). An adversarial agent profile can steer to a different read but cannot escape to a mutation. withHostedAuthTestKitis idempotent. Never overwrites caller-suppliedapi_key,basic, orprobe_task; setsprobe_taskonly when creds are present and one isn't already declared (hostedAuthProbeTaskForRunatcompliance-testing.ts:65-72short-circuits on caller-set probe_task). Fail-closed default oflist_creatives.- Discovery error handling fails to the safe default. All three call sites catch and warn with
{ err, agentUrl }only — no credential reflection, no Authorization-header echo through pino's standard serializer. - Version enum updates are consistent.
3.1-rc.4added toSUPPORTED_RELEASE_VERSIONSattask-handlers.ts:637andtenants/router.ts:50and to all the matching test fixtures — no drift across the six edited files. - Changeset is present (
.changeset/basic-auth-prod-runs.md) and the empty-frontmatter form is the established convention here for server-side fixes that don't bump theadcontextprotocolnpm package. - No
static/schemas/source/**changes, no docs drift to audit.
Follow-ups (non-blocking — file as issues)
emitFrameworkTaskWebhookoperation_id fallback.server/src/training-agent/tenants/registry.ts:196-208falls backpayload.operation_id ?? params.operation_id. The existingwebhooks.ts:206convention treats the emitter'soperation_idargument as a seller-side idempotency scope (the comment atwebhooks.ts:194says it's never placed on the wire because it embeds the principal token). If the framework SDK'staskWebhookEmitter.emitfollows the same semantic, the fallback could publish that scope as the wireoperation_id. The integration test attraining-agent-webhooks.test.ts:127,146only covers the buyer-supplied path — the fallback branch is uncovered. Either confirm the SDK uses a distinct wire-safe value forparams.operation_id(and drop a comment), or fall back toparams.payload.task_idthe waymaybeEmitCompletionWebhookdoes atwebhooks.ts:196. Bothcode-reviewerandsecurity-reviewerlanded on this independently.- Double discovery round-trip per storyboard step. Every Addie / compliance / storyboard run now fires
testCapabilityDiscoverybefore the actual run, and the SDK does its own discovery insiderunStoryboard. Net ~2× wall-clock and ~2× outbound request volume against the target agent. Cache the resolved profile per(orgId, agentUrl, complianceTarget)for the run window, or thread the discovered caps into the SDK call so it can skip the second trip. - Inconsistent
withHostedTestOptionswrap at discovery sites.compliance-testing.ts:65callstestCapabilityDiscovery(agentUrl, options)raw, whileregistry-api.ts:6293andmember-tools.ts:445wrap withwithHostedTestOptions(..., target). Pick one — silent drift the first time a target-specificschemaRootoradcpVersionstarts mattering for discovery.
Minor nits (non-blocking)
RuntimeTestKitAuthindex signature.server/src/services/hosted-compliance-version.ts:236-242declares[key: string]: unknownand the test atstoryboards.test.ts:224uses(options.test_kit?.auth as any)?.basic. Both signs the local widening is hiding an SDK type that doesn't yet exposebasic. Prefer keying offNonNullable<NonNullable<TestOptions['test_kit']>['auth']>so the next SDK rename (e.g.basic→credentials) trips at typecheck.declaresStaticAuthsimplification.hosted-compliance-version.ts:307-310—Boolean(api_key || basic || probe_task)combined with&& !probe_taskcollapses to(api_key || basic) && !probe_task. Same behavior, easier to read.- Dual key for sponsored intelligence.
hosted-compliance-version.ts:265-269carries both'sponsored_intelligence'and'sponsored-intelligence'— reasonable defensive aliasing between the wire snake_case and manifest kebab-case, but a one-line comment would prevent a future "dedupe" PR from quietly dropping the kebab form. - Caret on prerelease SDK.
package.json:122keeps^8.1.0-beta.16while the lockfile pins beta.18. Caret semantics on prereleases admit any8.1.0-beta.X. A pin (8.1.0-beta.18) would be safer for fresh installs that miss cache.
Approving on the strength of the contained credential plumbing, the closed probe-task allowlist, and consistent version-enum updates across all six fixture sites. Worth following up on the operation_id fallback before the next 3.0.14 storyboard pass.
There was a problem hiding this comment.
One question on the framework webhook fallback before approving. Other findings are non-blocking.
Things I checked
server/src/services/hosted-compliance-version.ts—withHostedAuthTestKitmirror is per-request, scoped to a freshly-resolvedComplyOptionsobject; no shared cache or persistence path picks it up. Tenant isolation preserved by the upstreamresolveUserAgentAuth(orgId, agentUrl, …). Surface widens within one in-memory options object — acceptable.hostedAuthProbeTaskForProfilecorrectly disambiguates bysupported_protocolsfirst, then falls back across the union, then tolist_creatives. Profile-aware selection looks right for non-creative agents..changeset/basic-auth-prod-runs.mdempty frontmatter is consistent with other no-version-bump entries in this repo (3-1-active-development-banner.md, etc.). Fine.- Test additions in
server/tests/unit/storyboards.test.ts:217-252exercise both bearer/basic mirror and profile-based probe selection. Integration test inserver/tests/integration/training-agent-webhooks.test.ts:130,146pins the buyer-suppliedoperation_idround-trip.
Question that blocks approval
server/src/training-agent/webhooks.ts:389-396 — the new fallback:
operation_id: params.payload.operation_id ?? params.operation_id,The legacy path in the same file at line 207 passes operation_id: idempotencyScope to emitter.emit, where idempotencyScope is principal-scoped (${principal}|...) and the comment at lines 134-138 / 191-196 is explicit: this value is never placed on the wire because it embeds the seller-side principal token. resolveIdempotencyPrincipal at tenants/registry.ts:215-232 returns ctx.authInfo?.clientId, and clientId is bridged at tenants/router.ts:198 as static:demo:<bearer-token> — i.e., it embeds the demo bearer token as a substring.
If params.operation_id reaching emitFrameworkTaskWebhook carries that same principal-scoped value in beta.18, the fallback emits the seller principal — and a substring of the demo bearer — in a signed, retried, idempotency-stored wire payload to a buyer-controlled URL. That happens precisely when the buyer omits push_notification_config.operation_id. The new test only covers the buyer-supplied case.
The PR description says "relying on beta.18's native operation-id threading," which implies the SDK now threads the buyer-supplied value to params.operation_id at the framework boundary. That would make the fallback safe. But the same WebhookEmitParams.operation_id field is used as the idempotency scope in the legacy call (line 207), so the semantics inside this codebase are now contradictory.
To flip to approve, please confirm one of:
- In
@adcp/sdk8.1.0-beta.18's framework task webhook path,params.operation_idis the buyer-suppliedpush_notification_config.operation_id(not the principal-scoped idempotency key) — link to SDK code or release note. - Or change the fallback to match the safe pattern at line 197:
params.payload.operation_id ?? (params.payload.task_id as string \| undefined), and add a regression test asserting that when buyer omitsoperation_idand the caller authenticates with a billable bearer token,body.operation_iddoes not equalparams.operation_idand does not contain the bearer-token substring.
Follow-ups (non-blocking — file as issues)
server/src/addie/services/compliance-testing.ts:60-78—hostedAuthProbeTaskForRuncallstestCapabilityDiscovery(agentUrl, options)with the raw options, whilemember-tools.ts:447andregistry-api.ts:6291wrap withwithHostedTestOptions. Discovery here will probe with the SDK default schema root rather than the hosted target. Likely benign for capability discovery, but worth aligning for consistency.- Pre-discovery adds one extra round trip per storyboard run, storyboard step, and comply call — no caching. Tail latency on long storyboards roughly doubles for the first probe. Consider memoizing per
(agentUrl, auth-fingerprint)for the request lifetime, or accept and note the cost. server/src/services/hosted-compliance-version.ts:35-42— bothsponsored_intelligenceandsponsored-intelligencekeys exist inHOSTED_AUTH_PROBE_TASKS_BY_PROTOCOL. If this aliases an SDK protocol-name drift, a comment would help; if not, pick one.
Minor nits (non-blocking)
- Test casts.
server/tests/unit/storyboards.test.ts:235,238useas anyto reachtest_kit.auth.basic. ExportRuntimeTestKitAuthor widen the SDK type so the cast goes away.
Will re-review once the operation_id semantics question is answered.
There was a problem hiding this comment.
LGTM. Follow-ups noted below. The split between idempotency-scope (in-memory) and wire operation_id (payload) is preserved correctly, and the probe-task allowlist is closed against hostile agent profiles.
Things I checked
withHostedAuthTestKit(server/src/services/hosted-compliance-version.ts) only fills empty slots — existingtest_kit.auth.api_key/basicis preserved. Mirrored creds do not reach DB writes:compliance-testing.ts:687-705header allowlist excludes Authorization, andoutbound-log-dblogs no credential fields.- Adversarial-agent profile cannot inject arbitrary task names.
hostedAuthProbeTaskForProfilereturns only values fromHOSTED_AUTH_PROBE_TASKS_BY_PROTOCOL/HOSTED_AUTH_PROBE_TASK_FALLBACKS/DEFAULT_HOSTED_AUTH_PROBE_TASK. All allowlisted tasks are read-only AdCP reads. - Multi-tenant isolation preserved.
resolveUserAgentAuth(agentContextDb, orgId, agentUrl, logger)still org-scopes every cred fetch; the new pre-discovery call reuses the samesdkAuth, no cross-tenant leakage. emitFrameworkTaskWebhook(server/src/training-agent/webhooks.ts:389-397) overlayspayload.operation_idwithparams.payload.operation_id ?? taskId. Top-levelparams.operation_idis the SDK's in-memory idempotency-store key and is never serialized — the new test attraining-agent-webhooks.test.ts"does not copy the framework idempotency scope into webhook operation_id" pins this invariant.- Probe-task selection threads through the three callers (
member-tools.ts:4467/4655,compliance-testing.ts:comply,registry-api.ts:6306). Failure mode is alogger.warnand a fall-back to the default probe — fail-closed against the auth path, fail-open against discovery brittleness, which is the right shape. - Changeset present (
.changeset/basic-auth-prod-runs.md). Empty-type frontmatter matches the existing repo pattern, so non-blocking.
Follow-ups (non-blocking — file as issues)
- De-duplicate the pre-discovery pattern. Three call sites duplicate the same
testCapabilityDiscovery → hostedAuthProbeTaskForProfileblock:server/src/addie/mcp/member-tools.ts:441-454,server/src/addie/services/compliance-testing.ts:56-71,server/src/routes/registry-api.ts:6289-6296. They already drift:compliance-testing.tsshort-circuits on a pre-setprobe_task, the other two don't; the registry-api inline copy runs before thecontext/dry_runbody validation at:6299-6304, so malformed requests now incur an outbound probe. Extract one helper fromhosted-compliance-version.ts(e.g.inferHostedAuthProbeTask(agentUrl, auth, target)) and call it from all three. - Pre-discovery options drift in
compliance-testing.ts:65. The pre-discovery call uses rawoptionsand notwithHostedTestOptions(options, target)— so it probes without the hostedschemaRoot/adcpVersionthe real run uses. The other two paths wrap. Worst case is the inferred profile differs from the production probe path and you fall back tolist_creatives, which is harmless, but the asymmetry is worth straightening. - Doubles authenticated probe traffic. Every storyboard run now hits the agent twice (once for discovery, once for the run). Defense-in-depth, not a security issue. Cache discovery per
(orgId, agentUrl, version)for the request scope. - Logger error redaction is not depth-wildcarded.
logger.ts:151-171redactsauthorization/token/api_keyat top-level only. The newlogger.warn({ error, agentUrl }, ...)paths rely on the SDK's thrown errors not embedding the Authorization header in.message/.stack. Currently true. Add'*.headers.authorization'to the redact list as belt-and-braces.
Minor nits (non-blocking)
RuntimeTestKitAuthwidens with[key: string]: unknown.server/src/services/hosted-compliance-version.ts:20-25. The intermediate widening means future SDK auth-shape tightening won't surface at the type level. Narrow to the SDK's actual auth union if@adcp/sdk/testingexports one.- Empty-string
operation_idslips through.server/src/training-agent/webhooks.ts:395usesparams.payload.operation_id ?? taskId— an empty-stringoperation_id(no framework path supplies one today) would pass through as the wire value.||instead of??if any framework caller might pass''. (options.test_kit?.auth as any)?.basicatserver/tests/unit/storyboards.test.ts:228. Cast to the newRuntimeTestKitAuthinstead ofany.
Approving on the strength of the credential-scope separation in emitFrameworkTaskWebhook plus the closed-allowlist hostedAuthProbeTaskForProfile.
# Conflicts: # server/tests/integration/training-agent-webhooks.test.ts
There was a problem hiding this comment.
Approving. Focused fix that pushes Basic-auth handling out of local SDK patches and onto the published beta.18 path — narrower blast radius, fewer drift points.
Things I checked
- 9 files, +325/-13. Largest deltas:
server/src/services/hosted-compliance-version.ts(+93/-3) and the new integration test (+103/-1). No file crosses the 200-net-line rule. - Changeset present (
.changeset/basic-auth-prod-runs.md). Empty frontmatter matches the repo convention for non-spec-versioning entries (e.g..changeset/3-1-rc-release-notes-audit.md). Nostatic/schemas/source/**changes → spec-coherence audit N/A. withHostedAuthTestKitprecedence (!nextAuth.api_key) is the right shape: buyer-supplied test_kit credentials win over an OAuth-resolved bearer. Unit coverage atserver/tests/unit/storyboards.test.ts:217-254exercises both bearer and basic threading.- Webhook fallback at
server/src/training-agent/webhooks.ts:389-397:payload.operation_id ?? task_id. Test atserver/tests/integration/training-agent-webhooks.test.ts:267-296proves the framework's signed scopestatic:demo:demo-billing-agent-billable-v1|create_media_buy.mb_secretdoes NOT leak into the wire body. Right invariant, right assertion. - OAuth user tokens are not mirrored into
test_kit.auth.api_key—adaptAuthForSdkreturns user-OAuth astype: 'oauth', so the bearer-mirror branch only fires for already-exchanged client-credentials tokens. No new exposure surface. - Pre-discovery probe goes only to the storyboard's
agentUrl, which the user already authorized — no new attacker surface.
Follow-ups (non-blocking — file as issues)
- Protocol keys are kebab-case but the schema enum is snake-case.
server/src/services/hosted-compliance-version.ts:37-44lists'media-buy'and'sponsored-intelligence', butstatic/schemas/source/protocol/get-adcp-capabilities-response.json:138-146restrictssupported_protocolstomedia_buy/sponsored_intelligence. The hyphen keys never match — bug is latent because the flat fallback list athosted-compliance-version.ts:46-55happens to cover the same probe tasks. Rename to snake_case and drop the kebab dupes. Add a test assertinghostedAuthProbeTaskForProfile({ tools: ['get_media_buy_delivery'], supported_protocols: ['media_buy'] })returns'get_media_buy_delivery'. - Discovery wrap drift across the three pre-discovery sites.
server/src/addie/services/compliance-testing.ts:64callstestCapabilityDiscovery(agentUrl, options)with rawComplyOptions. The other two callers (server/src/addie/mcp/member-tools.ts:451,server/src/routes/registry-api.ts:6293) wrap viawithHostedTestOptions(..., target). Addie's compliance path therefore probes against SDK defaults, not the buyer-selected compliance target — likely returns a differentprofilefor agents pinned to a non-default version. Three near-identical implementations, and one of them has already drifted; extract a singleinferHostedAuthProbeTask(agentUrl, sdkAuth, target, { existingProbeTask? })inhosted-compliance-version.tsand call it from all three. - Missing precedence test for
withHostedAuthTestKit.server/tests/unit/storyboards.test.ts:217-254covers the happy path but not the precedence claim that the function will not overwrite a pre-settest_kit.auth.api_keyortest_kit.auth.basic. Oneexpect(withHostedAuthTestKit({ auth: { type: 'bearer', token: 'fresh' }, test_kit: { auth: { api_key: 'preset' } } }).test_kit.auth.api_key).toBe('preset')plus the analogous basic case is enough — without it, a future refactor that flips the!nextAuth.api_keyguard ships silently. - Discovery-failure log path can round-trip credentials.
server/src/addie/mcp/member-tools.ts:453,server/src/addie/services/compliance-testing.ts:67,server/src/routes/registry-api.ts:6296log{ err, agentUrl }. Pino redact atserver/src/logger.ts:151-171is path-based and will not scrubAuthorization: Bearer ...orBasic ...substrings embedded inerr.message/err.stack(the common axios/undici behavior on auth-rejected requests). No known leak today, but the new pre-discovery call is now a credential-bearing path. Either log{ name: err.name, code: err.code, agentUrl }only, or scrub bearer/basic patterns before logging.
Minor nits (non-blocking)
- Buyer
operation_idechoed unbounded.server/src/training-agent/webhooks.ts:389-397now round-trips a buyer-suppliedpayload.operation_idthrough the JWS-signed body with only a non-empty check upstream. A 1 MB string or one with control characters rides through. The fallback totask_idis server-derived and safe — only the explicit buyer-echo path needs a length and charset bound.
LGTM. Follow-ups noted below.
Summary
@adcp/sdkto8.1.0-beta.18and remove the temporary SDK Basic-auth patch plumbingtest_kit.authso prod/Addie 3.0 runs execute the new static-auth probeslist_creatives3.1-rc.4, and preserving buyeroperation_idon framework task webhook bodiesValidation
git diff --checknpm exec vitest run server/tests/unit/storyboards.test.tsnpm exec vitest run server/tests/integration/training-agent-webhooks.test.ts -t "delivers a signed completion webhook"npm exec vitest run server/tests/integration/training-agent-webhooks.test.tsnpm exec vitest run server/tests/unit/addie/basic-auth-normalization.test.tsnpm exec vitest run server/src/training-agent/tenants/tenant-smoke.test.ts server/tests/integration/training-agent-3-0-compat-tools.test.ts server/tests/integration/training-agent-webhooks.test.tsnpm exec vitest run server/tests/unit/training-agent.test.ts -t "AdCP protocol compliance|get_adcp_capabilities handler|MCP Tasks protocol"npm run typechecknpm run test:storyboards:3.0-compatnpm run test:storyboardstest:unit, dynamic-import lint, callapi-state-change lint,typecheckNotes
This now relies on the published SDK beta.18 Basic-auth runner and framework operation-id support instead of local SDK patches.