refactor: unify result types with discriminated Result<T, E> union

[Hweinstock]

Description

Problem

The codebase uses an inconsistent result-like type that changes depending on the context. The implementations vary slightly, and are not explicit about (usually) sharing a common shape. This common shape also strips error types, and distills their information down to a string, which leads to brittle behavior when catching errors.

As an example, systems that span modules (ex. telemetry) must handle each result type individually, resulting in excessive branching and complexity. Additionally, loss of error information, means loss of this information in telemetry, making it difficult to impossible to tell the type of error, and if it was user/application caused.

Solution

• unify result types behind a common flexible interface loosely based on Rust results. The main difference is that data is hoisted to the top level to be consistent with existing patterns.
• store full error information on the result, allowing for instanceof checking and error name matching for stronger exception handling and telemetry.

Future Work

There are other result types that could be generalized with further work to support spread operators on the error as well. I left this OOS since the PR is already very large. Some other small patterns that this PR continues, but I think should change in the future:

• excessive mocking (Ex. some tests mock entire modules, rather than the underlying dependencies. We mock our own code in some places).
• testing implementation rather than behavior (error messages are matches verbatim in many places, likely because thats all we had before. However, we should change these tests the verify error "shape" rather than exact content.
• higher level utilities for working with results. There are common patterns that could be useful to abstract. As an example, a mapResult function that allows data transformations with clear error propagation. This could avoid some nesting of try/catches in a few places.
• add more error types as we see fit.
• avoid using console.log directly for more flexibility.

Related Issue

Closes #

Documentation PR

N/A — internal refactor, no user-facing changes.

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update
• Other (please describe): Internal refactor — unify result types

Testing

How have you tested the change?

• I ran npm run test:unit and npm run test:integ
• I ran npm run typecheck
• I ran npm run lint
• If I modified src/assets/, I ran npm run test:update-snapshots and committed the updated snapshots

Checklist

• I have read the CONTRIBUTING document
• I have added any necessary tests that prove my fix is effective or my feature works
• I have updated the documentation accordingly
• I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
• My changes generate no new warnings
• Any dependent changes have been merged and published

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.

[github-actions]

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	43.33%	9089 / 20976
🔵	Statements	42.6%	9652 / 22654
🔵	Functions	40.15%	1574 / 3920
🔵	Branches	40.1%	5862 / 14616

Generated in workflow #2815 for commit b1f6977 by the Vitest Coverage Report Action

[padmak30]

Bug Bash Report — refactor: unify result types with discriminated Result<T, E> union

Branch: refactor/unified-result-type-v2 | Version: 0.13.0 | Base: main

TL;DR

No in-scope blockers, highs, or mediums found. Refactor is behavior-preserving: every happy/error path exercised produces identical output to main. Two pre-existing quirks re-observed (documented below as pre-existing). 1 note for maintainers.

In-scope bugs

None.

Pre-existing bugs (reproduce identically on main)

[medium] Commands exit 0 on error

• Reproducer:

  cd /tmp/empty-dir
  node dist/cli/index.mjs validate; echo "exit=$?"   # prints "No agentcore project found…" exit=0
  node dist/cli/index.mjs status;   echo "exit=$?"   # same
  node dist/cli/index.mjs status --json; echo "exit=$?"
  # inside a project, with malformed agentcore.json
  echo '{invalid json' > agentcore/agentcore.json
  node dist/cli/index.mjs validate;  echo "exit=$?"  # exit=0
  node dist/cli/index.mjs status --json; echo "exit=$?"  # exit=0

• Evidence it's pre-existing: Checked out origin/main (9afd3f0), rebuilt, reproduced identical output + exit=0 for every case. Not introduced by this PR.

[low] status --json prints human-readable text on error, not JSON

• Reproducer: With a malformed agentcore.json, status --json prints Error: Failed to parse JSON in config file… as plain text instead of a JSON error envelope.
• Evidence it's pre-existing: Same output on origin/main.

Notes

• invoke/logs/traces without a TTY print command help instead of an error. When running invoke --name X --prompt hello in a non-TTY shell, the CLI renders the subcommand help and exits 0 instead of reporting that an interactive terminal is required (or attempting non-interactive execution). Reproduces identically on main, so out of scope for this refactor; flagging for future UX work. The sole "interactive terminal required" message only surfaces for deploy.

What works ✓

• agentcore create --project-name bbtest --name myagent --defaults --skip-git --skip-install produces a valid project with correct agentcore.json.
• validate on a good project → Valid, exit=0.
• status / status --json on a good project → correct human + JSON output; resources array and deploymentState: local-only reported.
• Schema violations are surfaced with precise per-field errors:
  • {} → name: expected "string", version: expected "number".
  • runtimes[0].build: "BogusBuild" → expected "CodeZip" | "Container".
• Malformed JSON → Invalid JSON in agentcore.json: Expected property name… (position included).
• remove agent --name nonexistent -y → structured JSON {"success":false,"error":"Agent \"nonexistent\" not found."}.
• remove agent --name myagent -y → structured success JSON; subsequent status --json shows resources: [].
• remove all -y → {"success":true,"message":"All schemas reset to empty state"}.
• Build clean, --version prints 0.13.0, all CLI help rendered correctly.

Test coverage

Surface	Tested	Result
Build	yes	pass
CLI happy path (create/validate/status)	yes	pass
Error paths (missing project, malformed JSON, bad schema)	yes	pass (behavior identical to base)
Schema validation	yes	pass
Removal paths (remove agent, remove all, nonexistent)	yes	pass
Partition correctness	n/a	PR does not touch ARN/endpoint code

Refactor LGTM from a black-box behavioral standpoint — every tested surface produces identical stdout/stderr/exit code to main.

[github-actions]

Package Tarball

aws-agentcore-0.13.1.tgz

How to install

npm install https://github.com/aws/agentcore-cli/releases/download/pr-1125-tarball/aws-agentcore-0.13.1.tgz

[agentcore-cli-automation]

Thanks for tackling this — the unified Result<T, E> type and Error-typed error field will pay off in telemetry and instanceof checks.

The bulk of this is a clean mechanical migration, but several of the converted return sites silently dropped fields that used to be part of the success/failure shape (log paths, console URLs, session IDs, exec payloads, warnings). Because many of these are exposed through --json, they're effectively public CLI output contracts. I've left inline comments on the spots that look like real behavior regressions. Once those are addressed (or intentionally accepted), this should be good to go.

A couple of smaller items not worth individual comments:

• src/cli/operations/traces/insights-query.ts around L80: err.message ? new Error(err.message) : new Error(String(error)) — this could just be toError(error) for consistency with the rest of the migration.
• src/cli/commands/deploy/actions.ts:664: if (!tsResult.success && tsResult.error) — with the new discriminated union, tsResult.error is guaranteed present whenever !tsResult.success, so the second clause is redundant. Just if (!tsResult.success) reads cleaner.

[agentcore-cli-automation]

Nice work on the unified Result<T, E> refactor — the typed error field is a clear improvement, and on the latest commit the behavioral regressions earlier reviewers flagged (exec-mode payload, sessionId on failures, warnings passthrough, consoleUrl on traces failures, NoProjectError typing) all appear to be addressed.

One systemic issue to flag before approving: there's a cluster of catch blocks that still strip the original error's type by doing new Error(err.message) (or an equivalent string round-trip). This directly undermines the PR's stated goal — downstream instanceof AccessDeniedError / instanceof DependencyCheckError / etc. checks, error-name matching in telemetry, and cause chains all break at these seams, because every one of them reconstructs a plain Error and forgets the original.

toError(err) in src/cli/errors.ts already exists for exactly this purpose; every one of these sites should use it (or, if a wrapping message is really wanted, pass { cause: err } so the original is still reachable via error.cause and instanceof still works on the cause). Inline comments below point out each occurrence I found.

[agentcore-cli-automation]

Reviewed the latest revision (fc93cd7).

I walked through the diff and cross-checked against the existing review threads. The behavior regressions flagged earlier (status --json missing fields, invoke --exec --json payload, sessionId on invoke failures, traces get consoleUrl, dependency-check warnings, deploy log path on stderr vs stdout, NoProjectError typing in AgentPrimitive) have all been fixed in this revision. The new Error(err.message) type-stripping cluster (primitives remove(), remove-gateway-target, run-batch-evaluation, run-recommendation, handleImport, resource-import, import-gateway, MCP/A2A/AGUI failure paths in invoke/action.ts, insights-query.ts) has also been converted to toError(err) or wrapped with { cause: err }, so instanceof / error.name / cause-chain inspection now survives the seams. Ran grep for the pre-existing patterns to double-check — none remain in files touched by this PR.

No new serious issues found beyond what was already flagged. LGTM — approving.

[Hweinstock]

Will fix security audit as a follow-up since its unrelated to changes here.

#1211

[avi-alpert]

wouldn't the expects in this test file be redundant? if you have assert(!result.success); right after

[Hweinstock]

Yep, I tried to swap to assert everywhere where we needed the narrowed type, but it looks like I missed removing the existing expect here. It should be harmless to have both, and can also follow-up with a clean-up to remove this.

[avi-alpert]

why arent the errors in this file ValidationErrors?

[Hweinstock]

good question, we technically don't know if its an error from the schema being invalidated, or from issues reading from the filesystem. See implementation:

agentcore-cli/src/lib/schemas/io/config-io.ts

Line 195 in 6dc4b7f

async readDeployedState(): Promise<DeployedState> {

.

We could have some logic that checks the error thrown and determines if it should be validation or something else, but I left that OOS.

[avi-alpert]

Can you update the Agents.MD file i see a lot of stuff in there referencing the old result shapes