feat(observability): solution attribution via native AWS_SDK_UA_APP_ID (#319, alt to #338) - #345
feat(observability): solution attribution via native AWS_SDK_UA_APP_ID (#319, alt to #338)#345scottschreckengaust wants to merge 8 commits into
Conversation
✅ Tested & verified — outbound User-Agent on the wire (both runtime tiers)Validated that this PR's solution-attribution segments land on the actual outbound What was checkedTwo standalone wire-capture scripts import the real helpers and print the assembled
Result — every call, both tiers (
|
❓ Follow-up: is there an intentional asymmetry in the opt-out? (
|
💡 Follow-up (design): make
|
| Surface | Where the id comes from | Honors an override? |
|---|---|---|
app/{id}#{stackName} (wire) |
buildAppId() → AWS_SDK_UA_APP_ID env set by SolutionUaAspect |
✅ -c sdkUaAppId=... (and '' opts out) |
md/{id}#{component} (wire) |
hardcoded SOLUTION_ID in each helper |
❌ literal |
({id}) (stack description) |
hardcoded literal in main.ts |
❌ literal |
Proposed ideal behavior
A customer provides the solution id once at synth time and that value is used consistently across all three surfaces. Either input path is acceptable and they can share one resolver — the synth-time AWS_SDK_UA_APP_ID env var or the existing -c sdkUaAppId=... context flag (with the same precedence buildAppId already uses):
- unset → default
uksb-wt64nei4u6everywhere (today's behavior, unchanged); - set to
myco-xyz→app/myco-xyz#{stackName},md/myco-xyz#{component}, and description(myco-xyz); -c sdkUaAppId=''(orAWS_SDK_UA_APP_ID='') → noapp/, nomd/, and a plain description (full opt-out).
This collapses four hardcoded literals into one synth-time input and makes the opt-out coherent (one switch governs all three, instead of -c sdkUaAppId='' silencing only app/ while md/ and the description still carry uksb-wt64nei4u6).
Feasibility / notes (no change requested — just scoping the conversation)
- Synth-time only.
main.tsis plain Node, soprocess.env.AWS_SDK_UA_APP_IDandtryGetContextare available there; the description is assembled before the template is written. CloudFormation'sDescriptionfield itself is a static string (noRef/Fn::Sub), so this must be resolved at synth — which is exactly where the value already lives. - Today
AWS_SDK_UA_APP_IDcarries the full app-id (uksb#stack), not just the solution-id prefix. This proposal would treat the synth-time env/context value as the solution id and letbuildAppId()continue composing#{stackName}. Worth deciding explicitly: is the customer input the solution id (prefix) or the entire app-id? They differ for themd/and description surfaces. - Ordering: the description is built on
main.ts:42, but the override is read on:50. Gating the description means hoisting that read above stack construction — small, but a real reorder. - Parity: the three
md/helpers must consume the override identically (the cross-surface parity the docstrings already require), and the wire-capture tests would gain an override + opt-out assertion.
Does treating a single synth-time AWS_SDK_UA_APP_ID (solution-id) as the source of truth for app/ + md/ + description fit the intended design for #319/#292?
e4c53e2 to
2db8b4d
Compare
…uard (#319) The UA rework swapped resolve_linear_api_token's boto3.client(...) for platform_client(...), which imports boto3 lazily at call time. That moved the SDK import outside the try/except ImportError guard, so a missing SDK would raise an uncaught ImportError (propagating through the pipeline) instead of the pre-feature graceful skip. Re-add an explicit `import boto3` availability probe inside the guard so the "boto3 unavailable → skip Linear MCP" degradation path is restored, while still building the client via UA-carrying platform_client. Found during /review_pr self-review of the #345 rebase. Closes #319 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
✅ Acceptance summary (for the reviewer)#319 — outbound AWS SDK User-Agent now carries solution attribution via the native
Chosen over the alternative #338 (raw-UA-path approach): the native 🔀 Merge guidance (for the reviewer)Cluster 🤖 orchestrator note (agent) — promotion is orchestrator-driven; merge remains a human action. |
theagenticguy
left a comment
theagenticguy
left a comment
There was a problem hiding this comment.
Reviewed the full diff and independently verified the SDK behavior claims against botocore 1.43.x locally. The core design decision is sound and the evidence in the description holds up: _build_app_id in botocore/useragent.py calls sanitize_user_agent_string_component(raw_str=..., allow_hash=True), so # genuinely survives verbatim in AWS_SDK_UA_APP_ID. Picking the native field over a per-request middleware is the right call, and the aspect-based app/ wiring means future Lambdas are covered structurally. Confirmed the AgentCore runtime and the ECS container both receive the env var, and that ECS containerOverrides merge rather than replace task-definition env, so the app id survives per-task overrides.
Requesting changes on two defects and one coverage gap. Details inline; summary here.
1. buildAppId's override branch mangles #, reintroducing the exact bug this design exists to avoid. -c sdkUaAppId='uksb-wt64nei4u6#my-stack' yields uksb-wt64nei4u6-my-stack. The test at solution-ua-aspect.test.ts:44 locks in the mangling by only checking has/slash, so CI stays green. Blocking.
2. Every S3Client in cdk/src is unattributed, and this is not in the admitted-gaps list. All 8 construction sites lack ...abcaUserAgent(); grep -c abcaUserAgent over them returns 0. ecs-strategy.ts is the clearest case: line 30 was edited by this PR to add the UA, and line 38 in the same file was left alone. S3 carries the attachment and trace-artifact traffic, so the description's "every outbound AWS SDK call" claim fails on a high-volume plane. Blocking, or move it into the documented follow-up explicitly.
3. _merge_ua_config clobbers a caller's user_agent_extra while the test name claims the opposite. Config.merge(other) gives precedence to other, so a caller-supplied extra is dropped. Verified: Config(read_timeout=7, user_agent_extra='caller/1.0').merge(ua) returns md/uksb-wt64nei4u6#agent with caller/1.0 gone. No call site passes config= today, so this is latent rather than live. Should-fix.
Smaller items
jira-integration.tsis the only integration that never getsComponentUaAspect; Slack, Linear, and github-screenshot all do. Its Lambdas fall through to theapidefault, so Jira webhook traffic reports as the REST API surface. The Jira client sites are also skipped while Linear was converted by the same mechanical edit in this PR, which makes "pre-existing gap" an imprecise framing. Consider saying "Linear converted, Jira deferred" so the follow-up is scoped honestly.webhookCreateTaskFn(task-api.ts) inheritsABCA_COMPONENT: 'api'fromcreateTaskEnvwhile its sibling webhook Lambdas getwebhook, so one surface is labeled inconsistently.- The CLI admitted-gaps list says "etc." and omits four real misses:
runtime-status.ts:111,platform-doctor.ts:135and:214,webhook-test.ts:46. - No test asserts
AWS_SDK_UA_APP_IDon a synthesized template anywhere.agent.test.tsandecs-agent-cluster.test.tshave zero matches for it, and the aspect tests build one Lambda directly under a stack, so nested-scope traversal and the "every Lambda" claim are both unpinned. The newtask-api.test.tsloop runs over the 6-Lambda base template, which excludes the api-key Lambdas that actually lack the label, so it passes vacuously. - Nit: the aspect is added at default priority while cdk-nag runs at 500, so nag visits each Lambda before the env mutation. Harmless under current rules, and
agent.tsalready usesAspectPriority.MUTATINGfor this hazard.
What is genuinely strong: the wire-format tests. All three surfaces capture through the real middleware stack and assert the literal # across app-id set, unset, and empty-opt-out. Sanitization parity across the three modules is real; byte-comparing the Python frozenset against both JS regexes over ASCII shows an identical accepted set. Import hygiene checks out too: agent/pyproject.toml sets pythonpath = ["src"], there is no cycle, and the re-added import boto3 probe does close the graceful-skip hole because sys.modules caching prevents platform_client's lazy import from raising afterward.
| export function buildAppId(stackName: string, override?: string): string | undefined { | ||
| if (override !== undefined) { | ||
| const trimmed = override.trim(); | ||
| return trimmed === '' ? undefined : trimmed.replace(UA_TOKEN_UNSAFE, '-').slice(0, APP_ID_MAX_LEN); |
There was a problem hiding this comment.
Blocking: this sanitizes the entire override string with UA_TOKEN_UNSAFE, which deliberately excludes #. So the documented canonical form gets mangled:
buildAppId('X', 'uksb-wt64nei4u6#my-stack') => 'uksb-wt64nei4u6-my-stack'
That is precisely the app-uksb-wt64nei4u6-{stack} mangling the PR description cites as the reason the / design in #338 was rejected. The default branch on line 52 is correct only because it appends # after sanitizing the stack name.
Suggest sanitizing the override per #-delimited segment, or allowing # in the override charset and clipping after.
There was a problem hiding this comment.
@theagenticguy Fixed in a503ee7. Added sanitizeAppId() in solution-ua-aspect.ts that splits the override on #, runs UA_TOKEN_UNSAFE per segment, and rejoins — so buildAppId('X','uksb-wt64nei4u6#my-stack') now returns uksb-wt64nei4u6#my-stack verbatim (only other unsafe chars become -), then clips to APP_ID_MAX_LEN. Both branches now share the same #-separator semantics. 🤖 @scottschreckengaust (agent:w1)
|
|
||
| test('explicit override is used verbatim (sanitized)', () => { | ||
| expect(buildAppId('stack', 'custom-value')).toBe('custom-value'); | ||
| expect(buildAppId('stack', 'has/slash')).toBe('has-slash'); |
There was a problem hiding this comment.
This test locks in the bug above. has/slash -> has-slash is correct behavior, but there is no case with a #, so the override path mangling the solution separator never trips CI. Worth adding:
expect(buildAppId('stack', 'uksb-wt64nei4u6#my-stack')).toBe('uksb-wt64nei4u6#my-stack');That assertion fails today.
There was a problem hiding this comment.
@theagenticguy Added your assertion plus adversarial cases in solution-ua-aspect.test.ts: buildAppId('stack','uksb-wt64nei4u6#my-stack') -> unchanged (was failing before the fix); multi-# (my#stack) preserved; #+slash mix (uksb-wt64nei4u6#a/b -> uksb-wt64nei4u6#a-b, a/b#c/d -> a-b#c-d); and an over-length clip that still contains an earlier #. All green in a503ee7. 🤖 @scottschreckengaust (agent:w1)
| function getClient(): ECSClient { | ||
| if (!sharedClient) { | ||
| sharedClient = new ECSClient({}); | ||
| sharedClient = new ECSClient({ ...abcaUserAgent() }); |
There was a problem hiding this comment.
This line got the UA and getS3Client() eight lines below did not (sharedS3Client = new S3Client({})). Same file, same pattern, same PR.
It generalizes: all 8 new S3Client( sites in cdk/src are unattributed, and none appear in the admitted-gaps list:
confirm-uploads.ts:42get-trace-url.ts:34cleanup-pending-uploads.ts:50shared/create-task-core.ts:107shared/orchestrator.ts:511shared/strategies/ecs-strategy.ts:38github-webhook-processor.ts:33jira-webhook-processor.ts:71
Six of those eight live in files this PR already touched. Given S3 carries attachments and trace artifacts, this is likely the highest-volume service in the solution, so the md/ plane misses its biggest contributor.
There was a problem hiding this comment.
@theagenticguy Fixed all 8 new S3Client({}) sites with ...abcaUserAgent() (confirm-uploads, get-trace-url, cleanup-pending-uploads, shared/create-task-core, shared/orchestrator, shared/strategies/ecs-strategy incl. getS3Client(), github-webhook-processor, jira-webhook-processor) in a503ee7. Also attributed the co-located naked DynamoDB + Bedrock clients in jira-webhook-processor (that whole file lacked the UA). Zero naked AWS SDK clients now remain in cdk/src. No sites deferred. 🤖 @scottschreckengaust (agent:w1)
|
|
||
| ua_config = ua.client_config() | ||
| existing = kwargs.get("config") | ||
| kwargs["config"] = existing.merge(ua_config) if existing is not None else ua_config |
There was a problem hiding this comment.
The docstring says "Preserves a caller-supplied Config by merging rather than overwriting," but botocore's Config.merge(other) gives precedence to other (it does config_options.update(other._user_provided_options)). Verified against botocore 1.43.53:
Config(read_timeout=7, user_agent_extra='caller/1.0').merge(
Config(user_agent_extra='md/uksb-wt64nei4u6#agent')
).user_agent_extra
# => 'md/uksb-wt64nei4u6#agent' ('caller/1.0' dropped)Non-colliding keys like read_timeout do survive, so only user_agent_extra itself is affected. No call site passes config= today, so it is latent. Note the asymmetry: the scoped-session path keeps both segments, so the two paths behave differently for the same input.
Fix would be concatenating when the caller already set an extra:
if existing is not None and existing.user_agent_extra:
ua_config = Config(user_agent_extra=f"{existing.user_agent_extra} {static_user_agent_extra()}")There was a problem hiding this comment.
@theagenticguy Fixed in a503ee7. _merge_ua_config now checks the caller's user_agent_extra: when set, it concatenates f"{caller_extra} {static_user_agent_extra()}" into a fresh Config and merges that (so both segments survive and non-colliding keys like read_timeout still merge); when unset, our md/ config wins as before. This matches the scoped-session path, which already kept both. Docstring updated to describe the concat. 🤖 @scottschreckengaust (agent:w1)
| cfg = mk.call_args.kwargs["config"] | ||
| assert cfg.user_agent_extra == "md/uksb-wt64nei4u6#agent" | ||
|
|
||
| def test_caller_config_is_merged_not_overwritten(self, monkeypatch): |
There was a problem hiding this comment.
The name promises "merged not overwritten," and the comment on line 357 says "Both the caller's setting and our UA survive the merge," but the fixture passes Config(read_timeout=7), the one key that cannot collide. The test asserts a property it never exercises.
Switching to Config(read_timeout=7, user_agent_extra='caller/1.0') and asserting 'caller/1.0' in cfg.user_agent_extra makes it fail, which is the actual behavior.
There was a problem hiding this comment.
@theagenticguy Rewrote the test in test_aws_session.py to pass Config(read_timeout=7, user_agent_extra='caller/1.0') and assert BOTH 'caller/1.0' and 'md/uksb-wt64nei4u6#agent' are in the merged user_agent_extra (plus read_timeout == 7). This failed before the concat fix and passes now. Kept a second test for the no-collision path (Config(read_timeout=7) -> md/ segment applied). 8/8 UA tests + 18/18 aws_session tests green. 🤖 @scottschreckengaust (agent:w1)
|
|
||
| /** The component label for this Lambda (from env, sanitized). */ | ||
| function componentLabel(): string { | ||
| return sanitizeUaValue(process.env[COMPONENT_ENV]?.trim() || DEFAULT_COMPONENT); |
There was a problem hiding this comment.
The api fallback is reasonable as a default, but it silently mislabels surfaces whose constructs never set ABCA_COMPONENT. jira-integration.ts is the only integration missing ComponentUaAspect (Slack, Linear, and github-screenshot all call it), so its 3 Lambdas report md/uksb-wt64nei4u6#api. Reachable in production: jira-webhook-processor calls into shared/create-task-core.ts, which does carry the UA, so live Jira webhook traffic is attributed as the REST API surface.
The api-key Lambdas are in the same position, since apiKeyEnv in task-api.ts is the only env object that does not spread commonEnv.
Adding Aspects.of(this).add(new ComponentUaAspect('webhook')) to jira-integration.ts matches the other three integrations.
There was a problem hiding this comment.
@theagenticguy Fixed in a503ee7. Added Aspects.of(this).add(new ComponentUaAspect('webhook')) to jira-integration.ts (matching slack/linear/github-screenshot), so its 3 Lambdas now report md/...#webhook instead of the #api fallback. For the api-key Lambdas: apiKeyEnv and the api-key authorizer env now set ABCA_COMPONENT: 'api' explicitly (no longer relying on the fallback), and I relabeled WebhookCreateTaskFn to webhook (it inherited createTask's api label but is a webhook surface). Added a jira-integration test asserting the webhook label and a task-api test covering the api-key/webhook split (the prior loop passed vacuously). 🤖 @scottschreckengaust (agent:w1)
…319) Alternative to PR #338: emit the app/ segment via the SDK-native AWS_SDK_UA_APP_ID env var (botocore + JS v3 read it automatically) using '#' instead of '/' as the separator, so no raw-user-agent-path machinery is needed. Each ua module owns only the static md/#{component} segment; there is no per-request {TRACE} handle, no before-send/middleware, and no module trace state — request correlation stays with X-Ray / structured logs (#245), and connection pools are never re-pinned. CloudFormation stack names are [A-Za-z0-9-], a subset of both the UA-token and app-id charsets, so {STACKNAME} needs no sanitization; the only sanitize() left is defensive cover on the component label. This commit adds the three mirrored helpers + tests (agent/src/ua.py, cdk/src/handlers/shared/ua.ts, cli/src/ua.ts). Each has a wire-capture test asserting both app/ and md/ segments land on a real outbound User-Agent header (and that app/ is omitted when AWS_SDK_UA_APP_ID is unset/empty — the customer opt-out). 12 agent + 12 cdk + 10 cli tests, 100% coverage on the new modules. Wiring the client sites + CDK env threading follow in subsequent commits. Part of #319 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sites (#319) Session-level user_agent_extra on both the scoped (refreshable) and the plain ambient session covers every tenant_client/tenant_resource caller. New platform_client() carries the static md/ segment (merged into any caller Config) for the 8 direct boto3.client sites that bypass the session by design — logs x5 (shell, server x2, telemetry x2), secrets manager x2 (config), bedrock-agentcore x1 (memory) — plus the ambient STS client used for role chaining. No per-request trace handle and no before-send appender: the md/ segment is fully static, so cached clients and the singleton session pool are never re-pinned. The app/ segment is contributed separately by the SDK from AWS_SDK_UA_APP_ID (threaded by CDK, next commit). 4 new aws_session tests assert the md/ segment rides platform_client, the unscoped tenant_client, a merged caller Config, and the scoped session. Full agent suite green (1070 tests). Part of #319 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Spread ...abcaUserAgent() into all 60 SDK v3 client constructors across 43 handler files (DynamoDB/Secrets Manager/Lambda/Bedrock/ECS/ BedrockAgentCore). DocumentClient sites instrument the inner DynamoDBClient (shared middleware stack). No withAbcaTrace/middleware — the md/ segment is fully static, so module-level cached clients keep their connection pools; the app/ segment rides native AWS_SDK_UA_APP_ID (threaded next commit). No behavior change beyond the UA header: all 2051 existing CDK tests pass unmodified (the new spread arg merges into the constructor config the tests already assert on / mock). Part of #319 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
) applyDefaultAppId() at startup defaults AWS_SDK_UA_APP_ID to the solution id (only when unset — an explicit '' opts out) so the CLI's own SDK calls carry the app/ segment with no per-site code. Spread ...abcaUserAgent() into all 18 AWS SDK v3 client sites (Cognito x3, Secrets Manager, CloudFormation, DynamoDB) across auth/admin/github/ slack/linear; the bgagent REST ApiClient is not an AWS SDK client and is untouched. auth.test.ts asserts the Cognito client constructor receives the md/ customUserAgent pair. Full CLI suite green (365 + new tests). Part of #319 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…319) The `app/` segment is now SDK-native: a stack-level SolutionUaAspect sets AWS_SDK_UA_APP_ID=uksb-wt64nei4u6#{stackName} on every Lambda (current and future, structurally), and the AgentCore runtime + ECS container set the same value explicitly (the Lambda-only aspect can't reach them). botocore and JS v3 both read AWS_SDK_UA_APP_ID natively, so no client code builds the app/ segment. `-c sdkUaAppId=''` opts the whole stack out; any other `-c sdkUaAppId=` value overrides. The `md/#{component}` label is per-surface ABCA_COMPONENT: 'api' (task-api commonEnv), 'orchestr' (orchestrator/reconcilers/cleanup/fanout), 'webhook' (slack/linear/github-screenshot integrations, via a per-construct ComponentUaAspect so every function in the integration — including future ones — is labeled without editing each env block). buildAppId() centralizes the value: defaults to uksb-wt64nei4u6#{stack}, sanitizes a non-CFN override, clips to the documented 50-char cap, and returns undefined for the empty-string opt-out. CloudFormation stack names are [A-Za-z0-9-] (already app-id-safe), so no stack-name sanitization is needed in the default path. New tests: solution-ua-aspect.test.ts (buildAppId vectors + both aspects); task-api/orchestrator template assertions for the component label. Full CDK suite green (2061 tests). Local synth fails only on the pre-existing ec2:DescribeAvailabilityZones cred gap (CI runs the real synth). Part of #319 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
New "Common mistakes" bullet directing agent/handler/CLI code to the per-surface ua helpers and explaining the app/ (SDK-native via AWS_SDK_UA_APP_ID) vs md/ (explicit per-surface label) split, plus the customer opt-out. Root-level file — no Starlight sync needed. Part of #319 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…uard (#319) The UA rework swapped resolve_linear_api_token's boto3.client(...) for platform_client(...), which imports boto3 lazily at call time. That moved the SDK import outside the try/except ImportError guard, so a missing SDK would raise an uncaught ImportError (propagating through the pipeline) instead of the pre-feature graceful skip. Re-add an explicit `import boto3` availability probe inside the guard so the "boto3 unavailable → skip Linear MCP" degradation path is restored, while still building the client via UA-carrying platform_client. Found during /review_pr self-review of the #345 rebase. Closes #319 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…e S3/Jira clients (#319) Remediate @theagenticguy's CHANGES_REQUESTED review on PR #345. - BLOCKING: buildAppId override branch sanitized the whole string with the UA-token charset (which excludes '#'), mangling the canonical 'uksb-wt64nei4u6#{stack}' separator back to '-'. Added sanitizeAppId() that splits on '#', sanitizes each segment, and rejoins so the separator survives; both branches now clip to APP_ID_MAX_LEN. Tests exercise '#' preservation, multi-'#', '#'+slash mix, and over-length clip. - BLOCKING: attributed all 8 `new S3Client({})` sites in cdk/src with ...abcaUserAgent() (confirm-uploads, get-trace-url, cleanup-pending-uploads, create-task-core, orchestrator, ecs-strategy, github-webhook-processor, jira-webhook-processor). Also attributed the co-located naked DynamoDB + Bedrock clients in jira-webhook-processor (previously fully unattributed). - SHOULD-FIX: _merge_ua_config dropped a caller's user_agent_extra because botocore Config.merge gives precedence to the argument. Now concatenates caller-extra + our md/ segment so both survive; non-colliding keys still merge. Rewrote the test to pass a colliding user_agent_extra (was read_timeout only, which never collides) and assert both segments survive. - Added ComponentUaAspect('webhook') to jira-integration (the only integration missing it; matches slack/linear/github-screenshot), so its 3 Lambdas report md/...#webhook instead of falling back to #api. - task-api: set ABCA_COMPONENT explicitly on apiKeyEnv + the api-key authorizer (api); relabeled WebhookCreateTaskFn to webhook (it inherited createTask's api label but is a webhook surface). - Applied SolutionUaAspect at AspectPriority.MUTATING in main.ts so it runs before cdk-nag (500), matching the agent stack's aspects. - CLI: attributed every remaining naked AWS SDK client (runtime-status, platform-doctor x2, webhook-test, github-token x2, dynamo-clients x2, and the jira command's SecretsManager/DynamoDB/CloudFormation clients). - Tests: nested-scope synth assertion in agent.test.ts (AWS_SDK_UA_APP_ID on deeply-nested integration Lambdas; two CDK-internal custom-resource provider Lambdas are framework-owned and excluded); task-api api-key/webhook label coverage; jira-integration webhook-label guard. No new dependencies (native SDK field + existing helpers). Closes #319 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
ea551c7 to
a503ee7
Compare
|
🤖 @scottschreckengaust (agent:w1) — signing out, over and out. DONE: PR #345 remediated per @theagenticguy's review (6 inline threads + summary nits addressed, replied in-thread), rebased onto latest |
2 participants
Summary
Attribute every outbound AWS SDK call from ABCA (agent runtime, Lambda handlers,
bgagentCLI) to the solution via the SDK-nativeAWS_SDK_UA_APP_IDenv var plus a staticmd/User-Agent segment — no per-request middleware.Closes #319
Review remediation (theagenticguy)
CHANGES_REQUESTED addressed in commit
a503ee76. Each unresolved inline thread + summary nit maps to a concrete fix (also replied in-thread):solution-ua-aspect.ts:50— override sanitized withUA_TOKEN_UNSAFEmangled#→-sanitizeAppId(): split on#, sanitize each segment, rejoin.buildAppId('X','uksb-wt64nei4u6#my-stack')now returns it verbatim; both branches clip toAPP_ID_MAX_LEN.solution-ua-aspect.test.ts:44— no#case, bug never tripped CI#-preservation assertion + adversarial cases (multi-#,#+slash mix, over-length clip preserving#). Failed before the fix, passes now.ecs-strategy.ts:30— 8new S3Client({})sites unattributed...abcaUserAgent()(confirm-uploads, get-trace-url, cleanup-pending-uploads, shared/create-task-core, shared/orchestrator, ecs-strategygetS3Client(), github-webhook-processor, jira-webhook-processor). Also attributed the co-located naked DynamoDB + Bedrock clients in jira-webhook-processor. Zero naked AWS SDK clients remain incdk/src.aws_session.py:271—Config.mergedrops caller'suser_agent_extra_merge_ua_confignow concatenatesf"{caller_extra} {static_user_agent_extra()}"when the caller set an extra (both survive; non-colliding keys still merge). Docstring corrected.test_aws_session.py:347— fixture used the one non-colliding keyConfig(read_timeout=7, user_agent_extra='caller/1.0')and assert bothcaller/1.0andmd/…#agentsurvive. Added a second test for the no-collision path.ua.ts:83apifallback — jira mislabeledComponentUaAspect('webhook')tojira-integration.ts(matches slack/linear/github-screenshot); its 3 Lambdas now reportmd/…#webhook.Summary nits:
WebhookCreateTaskFnrelabeledwebhook(it inheritedcreateTask'sapilabel but is a webhook surface).apiKeyEnv+ api-key authorizer now setABCA_COMPONENT: 'api'explicitly (no longer relying on the fallback).runtime-status.ts,platform-doctor.ts(x2),webhook-test.ts,github-token.ts(x2),dynamo-clients.ts(x2), and the jira command's SecretsManager / DynamoDB / CloudFormation clients. No "etc.".agent.test.tsassertsAWS_SDK_UA_APP_IDon deeply-nested integration Lambdas (Jira/Slack/Linear), with the two CDK-internal custom-resource provider Lambdas explicitly excluded (see honest-gaps note below);task-api.test.tsnow covers the api-key (api) vs webhook (webhook) label split (the prior loop passed vacuously);jira-integration.test.tsguards thewebhooklabel.SolutionUaAspectapplied atAspectPriority.MUTATINGinmain.tsso it runs before cdk-nag (500), matching the agent stack.Reproduced root cause + evidence
#319 asks that outbound AWS API calls carry ABCA solution-attribution segments so AWS can measure usage. The original constraint (and the reason the earlier
/-separated design in #338 was rejected): the SDK'sAWS_SDK_UA_APP_IDfield sanitizes disallowed characters to-, which would mangle aapp/uksb-wt64nei4u6/{stack}separator intoapp-uksb-wt64nei4u6-{stack}.Verified against the pinned SDKs in this repo:
AWS_SDK_UA_APP_ID=uksb-wt64nei4u6#my-stack-nameanduser_agent_extra='md/uksb-wt64nei4u6#agent', the assembled UA is:… app/uksb-wt64nei4u6#my-stack-name md/uksb-wt64nei4u6#agent— both segments verbatim,#survives.UA_VALUE_ESCAPE_REGEX = /[^!$%&'*+\-.^_|~\w#]/g**allowlists#**;isValidUserAgentAppIdonly enforceslength <= 50(no charset filter). App-id value andcustomUserAgentvalues render#verbatim; a/-form value escapes toapp-uksb-wt64nei4u6-my-stack(proving why/` was unusable).The fix, and why native
AWS_SDK_UA_APP_IDis best-practiceapp/segment (universal, SDK-native) — aSolutionUaAspect(cdk/src/constructs/solution-ua-aspect.ts) setsAWS_SDK_UA_APP_IDon every Lambda at the stack level (atAspectPriority.MUTATING); the AgentCore runtime + ECS container get it too.buildAppId(stackName)emitsuksb-wt64nei4u6#{stackName};buildAppId's override branch now sanitizes per#-delimited segment so the separator is never mangled, clipped to the documented 50-char cap. No client code touchesapp/; new clients inherit it structurally.md/segment (per-surface label) — three parity helpers buildmd/uksb-wt64nei4u6#{component}and spread it into client constructors:agent/src/ua.py(user_agent_extra, viaaws_session),cdk/src/handlers/shared/ua.ts(...abcaUserAgent()),cli/src/ua.ts(...abcaUserAgent()). The three modules are identical in solution id, wire format, and RFC-7230 token sanitization (all threemd/-label sanitizers still deliberately exclude#; only the CDK-only app-id builder preserves#as the separator).Customer opt-out preserved: deploy with
-c sdkUaAppId=''(aspect becomes a no-op, noapp/segment) or exportAWS_SDK_UA_APP_ID=''for the CLI.Deviations from #319 spec
#{TRACE}correlation plane. feat(observability): inject solution into outbound AWS SDK User-Agent #319 described a per-request trace handle appended to the UA (before-send handler / JS middleware). This PR deliberately omits it: request correlation is owned by X-Ray and structured-log request ids (feat(observability): end-to-end task attribution and cross-plane trace correlation #245), not the User-Agent. The UA carries only stable solution attribution (app/+ staticmd/), which avoids re-pinning cached module-level clients on every call. This is a conscious simplification, not an oversight.#separator chosen deliberately for sanitization-safety. Rather than bypass the nativeAWS_SDK_UA_APP_IDfield to force a/(as the rejected feat(observability): inject solution into outbound AWS SDK User-Agent (#319) #338 approach did), this PR uses#, which is a legal app-id/UA-value character in both botocore and JS v3 (verified above). The token renders verbatim; no raw-customUserAgentbypass is needed.Testing
Run from the rebased worktree (
feat/319-sdk-user-agent-appid, on latestorigin/main):MISE_EXPERIMENTAL=1 mise //cdk:eslint→ clean;mise //cli:eslint→ clean (both--fix, no mutations)MISE_EXPERIMENTAL=1 mise //cdk:compile+//cli:compile→ cleanMISE_EXPERIMENTAL=1 mise //cdk:test→ 138 suites, 2562 tests pass; 1 snapshot pass (ua.ts100%; includes the new#-override, nested-scope synth, and api-key/webhook label tests)MISE_EXPERIMENTAL=1 mise //cli:test→ 53 suites, 653 tests passMISE_EXPERIMENTAL=1 mise //agent:quality→ 1315 tests pass, coverage 82.00% (ruff + ty clean; includes the merge-concat test)mise run security:sast→ clean;mise run security:secretsscoped to this diff (staged) → no leaks found#-survival verified empirically against botocore 1.43.42 and @aws-sdk/core 3.974.29 (see evidence above)Known baseline (pre-existing on
main, not introduced by this PR, out of scope):mise //cdk:synthfails onec2:DescribeAvailabilityZones— a documented hermeticity/creds gap on fresh checkouts (needs live AWS creds for the VPC AZ lookup), not a code defect.//cdk:compileand//cdk:testare green.aws-account-idfalse positives (commit1cfa5b9f, not in this diff); the per-diff staged scan is clean.Honest coverage gaps (deliberately not attributed)
CustomS3AutoDeleteObjects…andCustomVpcRestrictDefaultSG…are CDK-framework-ownedCfnResource-backed handlers (notlambda.Functionconstructs), so the aspect'sinstanceof lambda.Functionguard does not visit them. They fire only at deploy time and are not runtime solution traffic. The nested-scope synth test excludes them explicitly and asserts every ABCA-authored Lambda IS covered.agent/src/bedrock_creds_helper.py(STS assume-role) — out-of-process helper; STS assume-role inaws_session._build_scoped_sessionalready carriesua.client_config(). Left as an explicit follow-up, not silently deferred.Rebase reconciliation notes
Rebased onto latest
origin/main; the CLI Cognito/CloudFormation clients live incli/src/cognito-admin.ts+cli/src/stack-outputs.tsand carry...abcaUserAgent().AGENTS.md's #319 note is a single condensed bullet in## Common mistakes.Self-review note
An earlier self-review pass fixed a regression this PR introduced:
resolve_linear_api_tokeninagent/src/config.pymoved an SDK import outside the graceful-skip guard; re-added an explicitimport boto3availability probe inside the guard.Dependencies / related
/-separated / non-native design). This PR is the adopted approach for theua-broadcluster.AWS_SDK_UA_APP_IDis a built-in AWS SDK feature.🤖 Generated with Claude Code