Skip to content

feat(execute): add --require-verified and --timeout to execute status - #95

Open
vaibhav4046 wants to merge 1 commit into
KeeperHub:mainfrom
vaibhav4046:feat/execute-status-require-verified
Open

feat(execute): add --require-verified and --timeout to execute status#95
vaibhav4046 wants to merge 1 commit into
KeeperHub:mainfrom
vaibhav4046:feat/execute-status-require-verified

Conversation

@vaibhav4046

Copy link
Copy Markdown

Problem

kh execute status treats status=completed as final proof of success. But a completed execution without chain-verified receipts only proves the transaction was submitted, not that it landed. Agents and CI scripts that gate on this command can proceed on unproven state.

Separately, kh execute status --watch has no deadline: if an execution never reaches a terminal state, the command polls forever. (kh execute transfer --wait already has a --timeout for exactly this reason; status --watch does not.)

Related: #49 asks for better executionId -> status lookup ergonomics for agent workflows; this PR makes the existing lookup safe to gate on.

Changes

  • Parse receipts[] from GET /api/execute/{id}/status (hash, chainId, verified, receiptStatus, blockNumber, gasUsed, verifiedAt) and render each receipt in the status table.
  • New --require-verified flag: exit non-zero unless the execution is completed AND at least one receipt exists AND every receipt has verified=true with receiptStatus="success". Fails closed on reverted, not_found, timeout, and safe_inner_failure.
  • New --timeout flag (default 5m) for --watch, mirroring the deadline pattern in pollExecStatus used by transfer --wait.
  • Back-compat: without --require-verified, output and exit behavior are unchanged (receipts are shown when present).

Why fail-closed

A transactionHash proves submission; a verified receipt proves landing. For agent pipelines that chain onchain steps (kh ex st <id> --watch --require-verified && ./next-step.sh), the safe default for the strict flag is: no proof -> non-zero exit.

Tests

7 new cases in cmd/execute/status_verified_test.go (httptest fake server, same idiom as the existing status tests):

  1. completed + verified success receipt + --require-verified -> exit 0, receipt rendered
  2. completed + no receipts + --require-verified -> non-zero, "no receipts"
  3. completed + verified=false receipt -> non-zero, offending hash in error
  4. completed + receiptStatus in {reverted, not_found, timeout, safe_inner_failure} -> non-zero each (subtests)
  5. completed + no receipts WITHOUT the flag -> exit 0 (back-compat)
  6. --watch --require-verified -> polls pending -> completed+verified -> exit 0
  7. --watch --timeout 100ms against a never-terminal execution -> non-zero with "timeout"

go vet clean, gofmt clean, go test ./cmd/execute/ green.

Note: on Windows, 8 pre-existing agentic-wallet/doctor tests fail on a clean clone of main (appears to be a HOME vs USERPROFILE issue in test setup); untouched by this PR.

kh execute status previously treated status=completed as final proof,
but a completed execution without chain-verified receipts only proves
submission, not landing. It also looped forever under --watch because
the poll loop had no deadline (unlike transfer's pollExecStatus).

- Parse receipts[] from the status response (hash, chainId, verified,
  receiptStatus, blockNumber, gasUsed, verifiedAt) and render them in
  the status table.
- Add --require-verified: exit non-zero unless the execution completed
  AND at least one receipt exists AND every receipt has verified=true
  with receiptStatus "success". Fails closed on reverted, not_found,
  timeout and safe_inner_failure.
- Add --timeout (default 5m) to --watch, mirroring the deadline
  pattern already used by kh execute transfer --wait.
- Back-compat: without --require-verified, behavior is unchanged.

Tests: 7 new cases in status_verified_test.go covering verified pass,
no-receipts fail, unverified fail, each non-success receiptStatus,
back-compat, watch + verify, and watch timeout.

@suisuss suisuss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This repo has no CONTRIBUTING.md; the conventions that apply are enforced by CI and README - conventional-commit PR titles, go vet/gofmt clean, and tests alongside the changed command.

What this changes

cmd/execute/status.go:

  • ExecStatusResponse gains a Receipts []ExecReceipt field; renderExecStatus prints one table row per receipt (hash (receiptStatus, verified|unverified)).
  • New --require-verified flag on kh ex status: renderExecStatusChecked calls verifyExecReceipts, which fails unless Status == "completed", at least one receipt exists, and every receipt has Verified == true and ReceiptStatus == "success".
  • New --timeout flag (default 5m) on --watch. watchExecStatus now computes deadline := time.Now().Add(timeout) and checks it on both the ticker branch and the idle-sleep branch, returning a timeout error once exceeded. Previously --watch polled with no deadline.
  • status_verified_test.go (new, 246 lines): covers verifyExecReceipts's branches (no receipts, unverified, each non-success receiptStatus), back-compat without the flag, --watch --require-verified reaching a verified terminal state, and --watch --timeout 100ms against a permanently-pending execution.

Does it match the description

Scope creep - two independently shippable changes are bundled under one PR, and the description itself names them as separate ("Separately, kh execute status --watch has no deadline..."). Split test:

  • Receipts parsing/rendering + --require-verified is opt-in and purely additive - it does not need the --watch timeout to exist or be correct.
  • The --timeout/deadline addition to --watch is unrelated to receipt verification and changes default behavior for every existing --watch caller (previously unbounded polling, now capped at 5m unless overridden) - it does not need receipts or --require-verified to exist or be correct.

Each side deploys independently: status.go's receipt struct/rendering/--require-verified path could ship without touching watchExecStatus's deadline logic, and the deadline logic could ship without the Receipts field existing at all. Recommend splitting into (a) receipts + --require-verified and (b) --watch --timeout, each with its own subset of status_verified_test.go.

Blocking

  • cmd/execute/status.go:184-214 (watchExecStatus) - the deadline check only runs after fetchExecStatus returns or in the idle-sleep branch, and nothing in the request path (fetchExecStatus -> client.Do -> retryablehttp.Client.Do in internal/http/client.go) attaches a context.WithTimeout/context.WithDeadline or sets http.Client.Timeout -> if the server accepts the connection but never responds (stalled connection, silent network partition), client.inner.Do(req) blocks inside case <-ticker.C: indefinitely and the deadline check at line 208 is never reached -> kh ex st <id> --watch --timeout 5s hangs forever despite the flag's own doc string promising "Give up watching after this long." -> fix: make the request itself bounded, e.g. thread context.WithTimeout(ctx, timeout) through client.NewRequest/client.Do. Note this same gap already exists in pollExecStatus (transfer.go, --wait --timeout), which this PR's description says it mirrors - see Needs a decision.

Mechanical - actionable as-is

  • cmd/execute/status.go:209 vs status.go:213 - the ticker-branch timeout error is "timeout after %s: execution %s still %s" (includes the actual status) while the idle-branch timeout error is "execution %s still not terminal" (no status, different wording). Both fire at the same logical point (deadline exceeded mid-poll); use one consistent message.
  • status_verified_test.go's --watch --timeout test only exercises a server that responds promptly with pending on every poll - it does not cover a stalled/non-responding request, so it does not catch the Blocking item above. Add a case using a handler that blocks past the timeout duration.

Needs a decision

  • The hung-request gap in watchExecStatus mirrors an identical, pre-existing gap in pollExecStatus (transfer.go, --wait --timeout) - fixing it only in the new code duplicates the defect rather than removing it. (a) Fix both call sites now by adding context-aware requests to internal/http/client.go, touching code outside this PR's stated scope; (b) fix only watchExecStatus here and file the pollExecStatus gap separately; (c) leave both and document the limitation in both --timeout flag descriptions.

Verdict

Changes requested - the --timeout flag does not reliably bound a hung request, and the PR bundles two independently shippable changes.

@suisuss suisuss added changes-requested Triage: reviewed, changes needed from the contributor decision-needed Blocked on a maintainer decision, not on the contributor labels Aug 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes-requested Triage: reviewed, changes needed from the contributor decision-needed Blocked on a maintainer decision, not on the contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants