Repository navigation
fix(telemetry): prevent deploy success with warning from recording as failure - #2769
Conversation
Package TarballHow to installgh release download pr-2769-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-1.0.1.tgz |
|
Claude Security Review: no high-confidence findings. (run) |
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Changes requested
I confirmed the premise independently: in the published @aws/agentcore@0.31.2 bundle, the only process.exit(2) is in the CLI deploy path, after printDeployResult, and only reachable when result.success is true and postDeployWarnings.length > 0 (failures already process.exit(1) above it). Since the compat path always passes --target, v0 always takes that CLI branch, so exitCode === 2 ⇒ success with warnings holds. The error modeling, the shared shouldRecordCommandFailure predicate (replacing the duplicated exitCode !== 0 check in src/index.ts and src/testing/telemetry.ts), and the new tests all look right, and LegacyDeployError now names the exit code in its message, which the old SilentCLIError did not.
One issue to resolve before merging.
agentcore dev on a v0 harness project now records an aborted run as a success
delegateV0Deploy is not reached only from deploy. runHarnessDev deploys through the same handler (src/handlers/project/dev/index.ts:259-264), and that path exists specifically for v0 projects (DevProjectHandlerConfig.runV0Deploy, "Runs the final v0 deployer when harness dev is opened from a v0 project").
So for agentcore dev against a v0 harness project whose deploy succeeds with post-deploy warnings:
delegateV0DeploythrowsLegacyDeployWarningExit(src/handlers/project/deploy/index.ts:267), which nothing catches inrunHarnessDev;- the dev session never starts, and because
LegacyDeployWarningExitis aSilentCLIError, v1 prints nothing about why; - with this PR,
shouldRecordCommandFailurereturnsfalse, socli.command_runforcommand_path=/agentcore/devis emitted withexit_reason: "success"while the process exits 2 and the user got no dev server.
That is the same class of metric infidelity this PR is fixing, inverted: before the change the run was at least counted as a failure. Options:
- Preferred: catch
LegacyDeployWarningExitaround the deploy call inrunHarnessDevand continue into the dev session — the deploy did succeed, so the warning should not abortdev. (This also fixes the silent abort, which is a real UX bug on that path today.) - Keep the abort, but make it honest for telemetry — e.g. have
runHarnessDevrethrow something that is not exempted from failure recording, sodevis not booked as a success when no dev session ran.
Either way, please add a dev test with runV0Deploy: async () => 2; src/handlers/project/dev/index.test.ts:210 only covers exit 0, so the warning path on this surface is currently untested.
Minor notes (non-blocking)
LegacyDeployWarningExit(src/errors/errors.tsx:81) does not end inError/Exception, which is the conventionerrorNameSchemainsrc/telemetry/shapes.tsxenforces — if this name ever does reacherror_name, the schema's.catchsilently rewrites it toUnknownError. Harmless today since the whole point is that it never gets recorded, but a name likeLegacyDeployWarningErrorwould stay inside the convention.LegacyDeployErrorinherits the defaultsource: INTERNAL, so every v0 subprocess failure reportserror_source: "internal"even though the cause is almost always user/service-side. Pre-existing, but since this PR is about metric fidelity it may be worth classifying while you're here.meta: { legacyDeployExitCode: exitCode }onLegacyDeployErrorduplicates thelegacy_deploy_exit_codeattribute the handler already sets; fine, just redundant.- In the new test,
JSON.parse((await readFile(subject.telemetryFilePath, "utf8")).trim())assumes the sink file holds exactly one JSONL record. True today (onlycli.command_runis emitted), but it will fail with a confusing parse error the moment a second metric is added —assertMetricEmitted's line-splitting approach would be more durable.
|
Claude Security Review: no high-confidence findings. (run) |
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
The blocking issue from the previous round — agentcore dev on a v0 harness project aborting silently and then being recorded as a success — is resolved with option 1: runHarnessDev now catches LegacyDeployWarningExit and continues into the dev session (src/handlers/project/dev/index.ts:260-271), and the new test at src/handlers/project/dev/index.test.ts:882 drives runV0Deploy: async () => 2 through the real router and asserts the Inspector actually starts. The remaining notes from that review were minor and non-blocking.
I re-verified the premise independently rather than taking it on trust: across @aws/agentcore 0.20.0, 0.25.0, 0.29.0 and 0.31.2, process.exit(2) occurs exactly once in dist/cli/index.mjs, in handleDeployCLI, after the !success → exit(1) branch and guarded by postDeployWarnings.length > 0. Passing --target (which the compat path always does) forces that CLI branch rather than handleDeployTUI, so exitCode === 2 ⇒ deploy succeeded with post-deploy warnings is stable for any 0.x npx can resolve, including a stale cached one.
Also checked and fine:
createDeployProjectHandlerhas only two call sites (project/index.ts,dev/index.ts); both are now accounted for, andrunV0Deployis not reached from anywhere else.error.exitCode !== 0as a failure predicate existed in exactly two places (src/index.ts:119,src/testing/telemetry.ts:80); both now shareshouldRecordCommandFailure, so the prod and test paths can't drift.- Nothing between
delegateV0Deployand the rootcatchrewraps errors (AgentCoreCLIError.fromErrorreturns the instance unchanged), so theinstanceof LegacyDeployWarningExitcheck cannot be defeated;runWithExitCodestill surfaces exit 2 with no stderr noise. legacy_deploy_exit_codeis added tocommandRunSchema— required, since the sink strips unknown attributes — and the 0–255 bound can't be exceeded byrunInheritedProcess(worst case128 + signal).- The earlier note about
JSON.parseover the whole sink file is addressed by the line-splitting read in the deploy test.
One piece of context worth recording, not a change request: v0's postDeployWarnings includes genuine per-resource failures after a successful stack deploy (e.g. Knowledge base "x": <error>), yet v0 itself returns success: true and exit 2 for those. So "success with warnings" matches v0's own classification, and the new legacy_deploy_exit_code attribute keeps those runs findable in metrics — which is the right trade-off here.
Description
We observed two unusual trends in our metrics:
deployfailures, stemming from v0 compatibility deployments via v1.x CLISilentCLIErrortied to a legacy deployResearch exposed the following explanation: a v0 deploy via the v1 CLI treats all non-zero exit codes as a failure. This includes
exitCode == 2, which v0 deploy returns for a successful deploy that had a post-deploy warning. Thus, the command was marked as having failed, and it showed as aSilentCLIError.This PR does two things to mitigate this:
LegacyDeployErrorandLegacyDeployWarningExit, separating the two paths for non-zero exit codes during legacy deploydeploycommand result isLegacyDeployWarningExit, record command as a success instead of a failure in metricsType of Change
Testing
How have you tested the change?
bun testbun run test:e2e, or explained why they are not applicable(CI does not run them for you)
bun run typecheckbun run lint:checkbun run format:checkbun run auditbun run secrets:checkbun run buildsrc/assets/, I updated affected snapshots withbun test <test-file> --update-snapshotsand committed themby this PR
command.md(the release workflow regenerates it)Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.