[cli] Show rich abort info in local simulation vm_status#19490
[cli] Show rich abort info in local simulation vm_status#19490
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
👀
Differential Security Review — PR #19490: [cli] Show rich abort info in local simulation vm_status
Scope: vgao/cli-rich-vm-status → main | 4 files, 213 additions / 16 deletions
Modules touched: crates/aptos-cli-common, crates/aptos, aptos-move/cli
Executive Summary
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| HIGH | 0 |
| MEDIUM | 0 |
| LOW | 3 |
Overall risk: Low. This is a CLI-only formatting change isolated to local simulation output. No consensus, VM execution, gas metering, or authentication paths are touched. The new format_txn_status helper is well-tested (9 unit tests covering every ExecutionStatus arm).
Recommendation: APPROVE WITH NOTES
What Changed
| File | Lines +/- | Risk |
|---|---|---|
crates/aptos-cli-common/src/types.rs |
+199 / -2 | Low — new formatting helper + unit tests |
crates/aptos/src/aptos_context.rs |
+4 / -4 | Low — call-site wiring only |
aptos-move/cli/src/transactions.rs |
+4 / -4 | Low — call-site wiring only |
aptos-move/cli/src/commands.rs |
+6 / -6 | Low — call-site wiring only |
Findings
[LOW] F-1: Script MoveAbort format diverges from explain_vm_status in the REST API layer
File: crates/aptos-cli-common/src/types.rs:346–370
Blast radius: CLI local simulation paths only; no on-chain or consensus impact.
Test coverage: move_abort_in_script test covers this arm, but does not compare against API output.
The new loc() helper returns "script" for AbortLocation::Script, causing the MoveAbort arms to emit:
"Move abort in script: 0x2a"
explain_vm_status in api/types/src/convert.rs:1415 instead always emits:
"Move abort: code 0x2a"
and unconditionally ignores AbortInfo for script aborts.
The two strings share no common prefix or shape. Any tooling or test infrastructure that parses vm_status strings from either path and compares them will see a mismatch. The behavior introduced by this PR (showing location + optional AbortInfo for script aborts) is actually more informative than the API version, but the inconsistency is observable.
[LOW] F-2: MiscellaneousError(None) message differs from explain_vm_status
File: crates/aptos-cli-common/src/types.rs:382–385
Blast radius: CLI only; no runtime impact.
| Context | None arm output |
|---|---|
format_txn_status |
"Miscellaneous error" |
explain_vm_status (api/types/src/convert.rs:1450) |
"Execution failed with miscellaneous error and no status code" |
The CLI emits a shorter, different string for the same condition. Low-impact since MiscellaneousError is rare in practice, but adds to the surface area of format divergence between local simulation and API-served vm_status.
[LOW] F-3: AbortInfo.description is included unconditionally — empty value produces a trailing colon
File: crates/aptos-cli-common/src/types.rs:362–370
The two Some(i) branches always append i.description as the last segment. AbortInfo is populated from module metadata where both reason_name and description can be empty strings (e.g., in types/src/transaction/mod.rs:1672–1674, reason_name is initialized to "" before enrichment). If description is empty, the output becomes:
"Move abort in 0x1::code: ENAME(0x1): " ← trailing colon + space
"Move abort in 0x1::code: 0x1: " ← same for empty-reason arm
No type safety or correctness failure — just visually awkward output for partially-enriched abort metadata. The unit tests use non-empty descriptions and do not exercise this edge.
Test Coverage
| Changed Function | Coverage | Notes |
|---|---|---|
format_txn_status |
Well-tested — 9 unit tests across all arms | Empty description case not covered |
| Call sites (4×) | Not independently tested | Covered transitively by unit tests of the helper |
Blast Radius
format_txn_status is pub in aptos-cli-common (re-exported via pub use types::*). It is currently called in exactly 4 places (all local simulation paths). The function has no callers in consensus, VM execution, storage, or mempool paths. Caller count is LOW (4 non-test callers, all CLI-tier).
Historical Context
No removed security checks. The only change to existing logic is replacing vm_status.to_string() (a lossy Display call) with format_txn_status(output.status(), &vm_status) (a richer match over ExecutionStatus). Git history for the modified lines contains no security-fix commits.
Sent by Cursor Automation: Security Review Bot
| AbortLocation::Module(m) => { | ||
| format!("{}::{}", m.address().to_hex_literal(), m.name()) | ||
| }, | ||
| AbortLocation::Script => "script".to_string(), |
There was a problem hiding this comment.
[LOW F-1] Script abort format diverges from explain_vm_status. This arm returns "script", causing all three MoveAbort branches (including those with AbortInfo) to produce "Move abort in script: …". explain_vm_status (api/types/src/convert.rs:1415) always emits "Move abort: code {:#x}" for script aborts and never surfaces AbortInfo there. The CLI output is richer but inconsistent in prefix/shape with the API-layer string.
| ), | ||
| TransactionStatus::Keep(ExecutionStatus::MiscellaneousError(code)) => match code { | ||
| Some(c) => format!("Miscellaneous error: {:?}", c), | ||
| None => "Miscellaneous error".to_string(), |
There was a problem hiding this comment.
[LOW F-2] MiscellaneousError(None) message differs from explain_vm_status, which emits "Execution failed with miscellaneous error and no status code" for the same case (api/types/src/convert.rs:1450). Low practical impact since this variant is rare, but adds to the observable divergence between local simulation and REST API vm_status strings.
| code, | ||
| i.description | ||
| ), | ||
| Some(i) => format!("Move abort in {}: {:#x}: {}", loc(location), code, i.description), |
There was a problem hiding this comment.
[LOW F-3] i.description is appended unconditionally. If AbortInfo is present but description is an empty string (possible before metadata enrichment populates it — see types/src/transaction/mod.rs:1672), this produces "Move abort in …: 0x…: " with a trailing : . The unit tests don't exercise description = "". Consider guarding: if i.description.is_empty() { … } else { format!("… {}", i.description) } — or add a test asserting the current trailing-colon behavior is intentional.
| /// `TransactionSummary`. Prefers the VM-enriched `ExecutionStatus` (which | ||
| /// carries abort location and `AbortInfo`) over the top-level `VMStatus`, | ||
| /// whose `Display` impl drops both. `fallback` is used only for `Retry`. | ||
| pub fn format_txn_status(status: &TransactionStatus, fallback: &VMStatus) -> String { |
There was a problem hiding this comment.
I wonder if there is a way to combine this in explain_vm_status in api/types/src/convert.rs, since they do more or less the same thing.
There was a problem hiding this comment.
Left a TODO. There are currently some minor differences -- probably not worth addressing at the moment.
| info, | ||
| }) => match info { | ||
| Some(i) if !i.reason_name.is_empty() => format!( | ||
| "Move abort in {}: {}({:#x}): {}", |
There was a problem hiding this comment.
nit: This will have a dangling colon if description is empty. But that's the same issue in explain_vm_status, so probably not worth fixing.
There was a problem hiding this comment.
It now prints "(no description)" if it's empty.
Local simulation paths (`aptos move publish --session`, `aptos move replay`, and the debugger-backed `--local` / `--profile-gas` / `--benchmark` flows) were stringifying the top-level `VMStatus` via its `Display` impl, which drops abort location and `AbortInfo`. Users saw opaque output like `status ABORTED of type Execution with sub status 393221` with no module or reason. The VM already enriches `ExecutionStatus::MoveAbort.info` with `reason_name` + `description` from the aborting module's `#[error]`-annotated constants (see `AptosVM::finish_user_transaction`). That enrichment lands on `TransactionOutput.status()` but the CLI was formatting the un-enriched top-level `VMStatus` instead. Add `format_txn_status` in `aptos-cli-common` that prefers the enriched `ExecutionStatus` and falls back to `VMStatus::to_string()` only for `Retry`. Wire all four existing call sites. Schema unchanged — `vm_status` remains `Option<String>`. After: `Move abort in 0x1::code: EPACKAGE_DEP_MISSING(0x60005): ...`. Follow-up: port `explain_vm_status` so `ExecutionFailure` paths can resolve function names instead of printing `function #N`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vgao1996
force-pushed
the
vgao/cli-rich-vm-status
branch
from
a19ad84 to
fc3631d
Compare
vgao1996
force-pushed
the
vgao/cli-rich-vm-status
branch
2 times, most recently
from
fdaffff to
9e75c1a
Compare
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vgao1996
force-pushed
the
vgao/cli-rich-vm-status
branch
from
9e75c1a to
c398582
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
5 participants
Summary
Local simulation paths (
aptos move publish --session,aptos move replay, and the debugger-backed--local/--profile-gas/--benchmarkflows) previously stringified the top-levelVMStatusvia itsDisplayimpl, which drops abort location andAbortInfo. Users saw:No module, no reason — useless for debugging, especially for publish-time checks like Decibel's backward-compatibility check.
Root cause
The VM already enriches
ExecutionStatus::MoveAbort.infowithreason_name+descriptionfrom the aborting module's#[error]-annotated constants insideAptosVM::finish_user_transaction(viaRuntimeModuleMetadataV1::extract_abort_info). That enrichment lands onTransactionOutput.status(), but the CLI was formatting the un-enriched top-levelVMStatusinstead — effectively the same string thatVMStatus::Displayproduces, withlocationandmessageomitted.Fix
format_txn_status(&TransactionStatus, &VMStatus) -> Stringinaptos-cli-commonthat prefers the enrichedExecutionStatusand falls back toVMStatus::to_string()only forRetry.crates/aptos/src/aptos_context.rs(session + debugger paths)aptos-move/cli/src/transactions.rs(aptos-move-clidebugger path)aptos-move/cli/src/commands.rs(aptos move replay)Schema unchanged —
vm_statusremainsOption<String>.After
Follow-up
Port
explain_vm_status(currently only inapi/types/src/convert.rs) into a shared crate soExecutionFailurepaths can resolve function names instead of printingfunction #N. Out of scope for this PR.Test plan
cargo test -p aptos-cli-common— 9 new unit tests covering everyExecutionStatus/TransactionStatusarmcargo check -p aptos -p aptos-move-cliEPACKAGE_DEP_MISSINGvia session publish, confirm new output format🤖 Generated with Claude Code
Note
Low Risk
Low risk: changes only CLI formatting of
TransactionSummary.vm_statusfor local/simulation/replay paths and adds unit tests; no transaction execution logic or schema changes.Overview
Improves local simulation/replay output by formatting
vm_statusfromTransactionStatus(enrichedExecutionStatus) instead of the lossyVMStatusdisplay, so Move aborts include module/script location plus optionalreason_nameand description.Adds shared
format_txn_statusinaptos-cli-common(with unit tests) and wires it into existing local simulation call sites inaptos-move-cli(simulate_locally,move replay) and the full CLI (aptos_contextsession/debugger flows).Reviewed by Cursor Bugbot for commit c398582. Bugbot is set up for automated code reviews on this repo. Configure here.