From b5be8d93b43b2d7179deb215a18b9e004f21b65a Mon Sep 17 00:00:00 2001 From: mohamedwael201193 Date: Tue, 11 Aug 2026 06:18:05 +0300 Subject: [PATCH 1/7] test(fixtures): add execution recovery contract pack v1 Adds golden status envelopes and recovery rules for KeeperHub/cli#53. Does not implement --require-verified (see #95). --- README_EXECUTION_RECOVERY.md | 12 +++++ docs/execution-recovery-v1/contract.md | 52 +++++++++++++++++++ .../execution_recovery_v1/cold_start.json | 9 ++++ .../completed_with_tx.json | 18 +++++++ .../completed_without_tx.json | 9 ++++ testdata/execution_recovery_v1/failed.json | 10 ++++ testdata/execution_recovery_v1/malformed.json | 7 +++ testdata/execution_recovery_v1/not_found.json | 7 +++ testdata/execution_recovery_v1/queued.json | 9 ++++ .../execution_recovery_v1/rate_limited.json | 8 +++ testdata/execution_recovery_v1/reverted.json | 17 ++++++ .../execution_recovery_v1/unconfirmed.json | 17 ++++++ 12 files changed, 175 insertions(+) create mode 100644 README_EXECUTION_RECOVERY.md create mode 100644 docs/execution-recovery-v1/contract.md create mode 100644 testdata/execution_recovery_v1/cold_start.json create mode 100644 testdata/execution_recovery_v1/completed_with_tx.json create mode 100644 testdata/execution_recovery_v1/completed_without_tx.json create mode 100644 testdata/execution_recovery_v1/failed.json create mode 100644 testdata/execution_recovery_v1/malformed.json create mode 100644 testdata/execution_recovery_v1/not_found.json create mode 100644 testdata/execution_recovery_v1/queued.json create mode 100644 testdata/execution_recovery_v1/rate_limited.json create mode 100644 testdata/execution_recovery_v1/reverted.json create mode 100644 testdata/execution_recovery_v1/unconfirmed.json diff --git a/README_EXECUTION_RECOVERY.md b/README_EXECUTION_RECOVERY.md new file mode 100644 index 0000000..4c6c9fb --- /dev/null +++ b/README_EXECUTION_RECOVERY.md @@ -0,0 +1,12 @@ +# Execution Recovery Contract Pack v1 + +Fixture suite for [KeeperHub/cli#53](https://github.com/KeeperHub/cli/issues/53). + +Does **not** implement CLI `--require-verified` (see #95). Complements Option B of #53 with golden status envelopes + recovery rules. + +## Layout +- `testdata/execution_recovery_v1/*.json` — synthetic DEMO FIXTURE envelopes +- `docs/execution-recovery-v1/contract.md` — normative rules R1–R6 + +## Consumer note +A TypeScript reference consumer lives in the EMBER repo. Prefer Go tests here that load these fixtures and assert the decision table in `contract.md`. diff --git a/docs/execution-recovery-v1/contract.md b/docs/execution-recovery-v1/contract.md new file mode 100644 index 0000000..64463e2 --- /dev/null +++ b/docs/execution-recovery-v1/contract.md @@ -0,0 +1,52 @@ +# Execution recovery contract (normative) + +Version: **1.0.0** +Audience: KeeperHub CLI / MCP / HTTP adapter authors + +## Definitions + +- **Write**: any API call that may create or re-drive an onchain side effect (execute workflow, transfer, contract call). +- **Status read**: polling `get_execution` / `GET /api/execute/{id}/status` (or equivalent). +- **Chain evidence**: at least one transaction hash with a verified successful receipt for the expected predicate (token, amount, recipient, chain). +- **Idempotency key**: client-supplied key that must be byte-identical across retries of the same logical write. + +## Rules + +### R1 — Unconfirmed → poll, do not resubmit + +If status is `queued`, `running`, `pending`, or `unconfirmed`, the client MUST continue status reads against the **same** execution ID. The client MUST NOT issue a new write for the same logical intent while that execution ID remains durable. + +### R2 — No chain evidence → fail closed + +If the caller requires payment/landing proof and status is `completed` (or `success`) but no transaction hash / verified receipt is present, the client MUST treat the outcome as **not successful** for payment purposes. + +### R3 — Write retry → stable idempotency key + +If a write is retried after transport failure (timeout, 429, 5xx) before an execution ID is known, the client MUST reuse the same idempotency key. After an execution ID is known, prefer R1. + +### R4 — Terminal failure + +Statuses `failed`, `reverted`, `not_found`, and unparseable/malformed bodies are terminal for that attempt. Clients MUST NOT invent a success path from partial fields. + +### R5 — Rate limit + +HTTP `429` responses require backoff. They are not success. Preserve the idempotency key for the next write attempt of the same intent. + +### R6 — Cold start + +A first status read that returns `not_found` immediately after submit may be transient. Clients SHOULD poll briefly before concluding not_found as terminal, unless the product documents otherwise. + +## Fixture mapping + +| Fixture file | Exercises | +| --- | --- | +| `queued.json` | R1 | +| `completed_with_tx.json` | happy path with chain evidence | +| `completed_without_tx.json` | R2 | +| `failed.json` | R4 | +| `unconfirmed.json` | R1 | +| `reverted.json` | R4 | +| `not_found.json` | R4 / R6 | +| `rate_limited.json` | R5 | +| `cold_start.json` | R6 | +| `malformed.json` | R4 | diff --git a/testdata/execution_recovery_v1/cold_start.json b/testdata/execution_recovery_v1/cold_start.json new file mode 100644 index 0000000..eb2a0d0 --- /dev/null +++ b/testdata/execution_recovery_v1/cold_start.json @@ -0,0 +1,9 @@ +{ + "httpStatus": 200, + "execution": { + "id": "exec_fixture_cold_001", + "status": "queued", + "workflowId": "wf_fixture_001", + "note": "First status read immediately after submit may still be queued; treat as cold start under R6." + } +} diff --git a/testdata/execution_recovery_v1/completed_with_tx.json b/testdata/execution_recovery_v1/completed_with_tx.json new file mode 100644 index 0000000..b48c687 --- /dev/null +++ b/testdata/execution_recovery_v1/completed_with_tx.json @@ -0,0 +1,18 @@ +{ + "httpStatus": 200, + "execution": { + "id": "exec_fixture_ok_001", + "status": "completed", + "workflowId": "wf_fixture_001", + "transactionHash": "0x1111111111111111111111111111111111111111111111111111111111111111", + "receipts": [ + { + "hash": "0x1111111111111111111111111111111111111111111111111111111111111111", + "chainId": 8453, + "verified": true, + "receiptStatus": "success" + } + ], + "completedAt": "2026-08-11T00:01:00.000Z" + } +} diff --git a/testdata/execution_recovery_v1/completed_without_tx.json b/testdata/execution_recovery_v1/completed_without_tx.json new file mode 100644 index 0000000..82240a1 --- /dev/null +++ b/testdata/execution_recovery_v1/completed_without_tx.json @@ -0,0 +1,9 @@ +{ + "httpStatus": 200, + "execution": { + "id": "exec_fixture_notx_001", + "status": "completed", + "workflowId": "wf_fixture_001", + "completedAt": "2026-08-11T00:01:00.000Z" + } +} diff --git a/testdata/execution_recovery_v1/failed.json b/testdata/execution_recovery_v1/failed.json new file mode 100644 index 0000000..9a067e3 --- /dev/null +++ b/testdata/execution_recovery_v1/failed.json @@ -0,0 +1,10 @@ +{ + "httpStatus": 200, + "execution": { + "id": "exec_fixture_failed_001", + "status": "failed", + "workflowId": "wf_fixture_001", + "error": "action_failed", + "completedAt": "2026-08-11T00:01:00.000Z" + } +} diff --git a/testdata/execution_recovery_v1/malformed.json b/testdata/execution_recovery_v1/malformed.json new file mode 100644 index 0000000..efe1fb0 --- /dev/null +++ b/testdata/execution_recovery_v1/malformed.json @@ -0,0 +1,7 @@ +{ + "httpStatus": 200, + "data": { + "weird": true, + "statusCode": "DONE" + } +} diff --git a/testdata/execution_recovery_v1/not_found.json b/testdata/execution_recovery_v1/not_found.json new file mode 100644 index 0000000..b49b886 --- /dev/null +++ b/testdata/execution_recovery_v1/not_found.json @@ -0,0 +1,7 @@ +{ + "httpStatus": 404, + "error": { + "code": "not_found", + "message": "execution not found" + } +} diff --git a/testdata/execution_recovery_v1/queued.json b/testdata/execution_recovery_v1/queued.json new file mode 100644 index 0000000..f43596b --- /dev/null +++ b/testdata/execution_recovery_v1/queued.json @@ -0,0 +1,9 @@ +{ + "httpStatus": 200, + "execution": { + "id": "exec_fixture_queued_001", + "status": "queued", + "workflowId": "wf_fixture_001", + "createdAt": "2026-08-11T00:00:00.000Z" + } +} diff --git a/testdata/execution_recovery_v1/rate_limited.json b/testdata/execution_recovery_v1/rate_limited.json new file mode 100644 index 0000000..be0e28d --- /dev/null +++ b/testdata/execution_recovery_v1/rate_limited.json @@ -0,0 +1,8 @@ +{ + "httpStatus": 429, + "error": { + "code": "rate_limited", + "message": "too many requests", + "retryAfterSeconds": 2 + } +} diff --git a/testdata/execution_recovery_v1/reverted.json b/testdata/execution_recovery_v1/reverted.json new file mode 100644 index 0000000..32acd75 --- /dev/null +++ b/testdata/execution_recovery_v1/reverted.json @@ -0,0 +1,17 @@ +{ + "httpStatus": 200, + "execution": { + "id": "exec_fixture_reverted_001", + "status": "completed", + "workflowId": "wf_fixture_001", + "transactionHash": "0x3333333333333333333333333333333333333333333333333333333333333333", + "receipts": [ + { + "hash": "0x3333333333333333333333333333333333333333333333333333333333333333", + "chainId": 8453, + "verified": true, + "receiptStatus": "reverted" + } + ] + } +} diff --git a/testdata/execution_recovery_v1/unconfirmed.json b/testdata/execution_recovery_v1/unconfirmed.json new file mode 100644 index 0000000..e0e129f --- /dev/null +++ b/testdata/execution_recovery_v1/unconfirmed.json @@ -0,0 +1,17 @@ +{ + "httpStatus": 200, + "execution": { + "id": "exec_fixture_unconfirmed_001", + "status": "unconfirmed", + "workflowId": "wf_fixture_001", + "transactionHash": "0x2222222222222222222222222222222222222222222222222222222222222222", + "receipts": [ + { + "hash": "0x2222222222222222222222222222222222222222222222222222222222222222", + "chainId": 8453, + "verified": false, + "receiptStatus": "pending" + } + ] + } +} From 3505b09b514999433b4d23795d1232ef25254985 Mon Sep 17 00:00:00 2001 From: mohamedwael201193 Date: Wed, 12 Aug 2026 01:09:07 +0300 Subject: [PATCH 2/7] test: #53 execution recovery pack with loader, idempotency, cold-start Address review on PR #97: flat wire-shape fixtures consumed by table tests, fail closed on reverted receipts, stable Idempotency-Key on write retries, bounded not_found polling during --wait, docs sync for execution-recovery. --- .github/workflows/sync-cli-docs.yml | 2 +- README_EXECUTION_RECOVERY.md | 12 -- cmd/execute/contract_call.go | 9 + cmd/execute/status.go | 54 ++++-- cmd/execute/transfer.go | 60 ++++++- cmd/execute/transfer_recovery_test.go | 147 ++++++++++++++++ docs/execution-recovery-v1/contract.md | 75 +++++--- docs/execution-recovery.md | 29 ++++ docs/generate.go | 4 +- docs/kh_execute_contract-call.md | 17 +- docs/kh_execute_transfer.md | 17 +- internal/execrecovery/classify.go | 160 ++++++++++++++++++ internal/execrecovery/fixture.go | 112 ++++++++++++ internal/execrecovery/fixture_test.go | 142 ++++++++++++++++ internal/execrecovery/idempotency.go | 29 ++++ internal/execrecovery/idempotency_test.go | 38 +++++ internal/execrecovery/vocabulary.go | 27 +++ .../execution_recovery_v1/cold_start.json | 9 - .../cold_start.sequence.json | 40 +++++ .../completed_with_tx.json | 9 +- .../completed_without_tx.json | 11 +- testdata/execution_recovery_v1/failed.json | 11 +- testdata/execution_recovery_v1/malformed.json | 9 +- testdata/execution_recovery_v1/not_found.json | 12 +- testdata/execution_recovery_v1/queued.json | 10 +- .../execution_recovery_v1/rate_limited.json | 10 +- testdata/execution_recovery_v1/reverted.json | 14 +- .../execution_recovery_v1/unconfirmed.json | 20 +-- 28 files changed, 955 insertions(+), 134 deletions(-) delete mode 100644 README_EXECUTION_RECOVERY.md create mode 100644 cmd/execute/transfer_recovery_test.go create mode 100644 docs/execution-recovery.md create mode 100644 internal/execrecovery/classify.go create mode 100644 internal/execrecovery/fixture.go create mode 100644 internal/execrecovery/fixture_test.go create mode 100644 internal/execrecovery/idempotency.go create mode 100644 internal/execrecovery/idempotency_test.go create mode 100644 internal/execrecovery/vocabulary.go delete mode 100644 testdata/execution_recovery_v1/cold_start.json create mode 100644 testdata/execution_recovery_v1/cold_start.sequence.json diff --git a/.github/workflows/sync-cli-docs.yml b/.github/workflows/sync-cli-docs.yml index 3a58017..fe4442a 100644 --- a/.github/workflows/sync-cli-docs.yml +++ b/.github/workflows/sync-cli-docs.yml @@ -70,7 +70,7 @@ jobs: echo "Synced $COPIED command pages." # Sync hand-written guides (add frontmatter for Nextra) - for file in quickstart.md concepts.md; do + for file in quickstart.md concepts.md execution-recovery.md; do TITLE=$(head -1 "$CLI_DOCS/$file" | sed 's/^# //') # Build the Nextra-compatible version with frontmatter diff --git a/README_EXECUTION_RECOVERY.md b/README_EXECUTION_RECOVERY.md deleted file mode 100644 index 4c6c9fb..0000000 --- a/README_EXECUTION_RECOVERY.md +++ /dev/null @@ -1,12 +0,0 @@ -# Execution Recovery Contract Pack v1 - -Fixture suite for [KeeperHub/cli#53](https://github.com/KeeperHub/cli/issues/53). - -Does **not** implement CLI `--require-verified` (see #95). Complements Option B of #53 with golden status envelopes + recovery rules. - -## Layout -- `testdata/execution_recovery_v1/*.json` — synthetic DEMO FIXTURE envelopes -- `docs/execution-recovery-v1/contract.md` — normative rules R1–R6 - -## Consumer note -A TypeScript reference consumer lives in the EMBER repo. Prefer Go tests here that load these fixtures and assert the decision table in `contract.md`. diff --git a/cmd/execute/contract_call.go b/cmd/execute/contract_call.go index 6ea1f54..eb263d1 100644 --- a/cmd/execute/contract_call.go +++ b/cmd/execute/contract_call.go @@ -9,6 +9,7 @@ import ( "time" "github.com/jedib0t/go-pretty/v6/table" + "github.com/keeperhub/cli/internal/execrecovery" khhttp "github.com/keeperhub/cli/internal/http" "github.com/keeperhub/cli/internal/output" "github.com/keeperhub/cli/pkg/cmdutil" @@ -61,6 +62,7 @@ func NewContractCallCmd(f *cmdutil.Factory) *cobra.Command { abiFile, _ := cmd.Flags().GetString("abi-file") wait, _ := cmd.Flags().GetBool("wait") timeout, _ := cmd.Flags().GetDuration("timeout") + idemKeyFlag, _ := cmd.Flags().GetString("idempotency-key") reqBody := contractCallRequest{ ContractAddress: contract, @@ -88,11 +90,17 @@ func NewContractCallCmd(f *cmdutil.Factory) *cobra.Command { return fmt.Errorf("marshalling request: %w", err) } + idemKey, err := execrecovery.ResolveIdempotencyKey(idemKeyFlag) + if err != nil { + return err + } + req, err := client.NewRequest(http.MethodPost, khhttp.BuildBaseURL(host)+"/api/execute/contract-call", bytes.NewReader(bodyBytes)) if err != nil { return err } req.Header.Set("Content-Type", "application/json") + req.Header.Set(execrecovery.IdempotencyHeader, idemKey) resp, err := client.Do(req) if err != nil { @@ -143,6 +151,7 @@ func NewContractCallCmd(f *cmdutil.Factory) *cobra.Command { cmd.Flags().String("abi-file", "", "Path to local ABI JSON file") cmd.Flags().Bool("wait", false, "Wait for completion") cmd.Flags().Duration("timeout", 5*time.Minute, "Timeout when using --wait") + cmd.Flags().String("idempotency-key", "", "Stable Idempotency-Key for write intents (auto-generated if empty)") _ = cmd.MarkFlagRequired("chain") _ = cmd.MarkFlagRequired("contract") diff --git a/cmd/execute/status.go b/cmd/execute/status.go index b2d5a99..7be83ff 100644 --- a/cmd/execute/status.go +++ b/cmd/execute/status.go @@ -1,7 +1,9 @@ package execute import ( + "errors" "fmt" + "net/http" "time" "github.com/jedib0t/go-pretty/v6/table" @@ -14,15 +16,30 @@ import ( // ExecStatusResponse represents the execution status API response. // Shared by transfer, contract-call and status commands. type ExecStatusResponse struct { - ExecutionID string `json:"executionId"` - Status string `json:"status"` - Type string `json:"type"` - TransactionHash *string `json:"transactionHash"` - TransactionLink *string `json:"transactionLink"` - Result any `json:"result"` - Error *string `json:"error"` - CreatedAt string `json:"createdAt"` - CompletedAt *string `json:"completedAt"` + ExecutionID string `json:"executionId"` + Status string `json:"status"` + Type string `json:"type"` + TransactionHash *string `json:"transactionHash"` + TransactionLink *string `json:"transactionLink"` + Result any `json:"result"` + Error *string `json:"error"` + CreatedAt string `json:"createdAt"` + CompletedAt *string `json:"completedAt"` + Receipts []ExecReceipt `json:"receipts,omitempty"` +} + +// ExecReceipt is a chain-re-fetched proof entry attached to an execution. +// A transactionHash alone proves a transaction was submitted; a receipt with +// verified=true and receiptStatus="success" proves it landed onchain. +// receiptStatus="reverted" is Failure even when status=completed. +type ExecReceipt struct { + Hash string `json:"hash"` + ChainID int64 `json:"chainId"` + Verified bool `json:"verified"` + ReceiptStatus string `json:"receiptStatus"` + BlockNumber *int64 `json:"blockNumber,omitempty"` + GasUsed *string `json:"gasUsed,omitempty"` + VerifiedAt *string `json:"verifiedAt,omitempty"` } func NewStatusCmd(f *cmdutil.Factory) *cobra.Command { @@ -96,17 +113,17 @@ func renderExecStatus(p *output.Printer, f *cmdutil.Factory, sr *ExecStatusRespo if sr.Error != nil && *sr.Error != "" { tw.AppendRow(table.Row{"Error", *sr.Error}) } + for i, r := range sr.Receipts { + label := fmt.Sprintf("Receipt[%d]", i) + tw.AppendRow(table.Row{label, fmt.Sprintf("%s verified=%v status=%s", r.Hash, r.Verified, r.ReceiptStatus)}) + } tw.Render() }); err != nil { return err } - if sr.Status == "failed" { - msg := fmt.Sprintf("execution %s failed", sr.ExecutionID) - if sr.Error != nil && *sr.Error != "" { - msg = *sr.Error - } - return fmt.Errorf("%s", msg) + if err := execOutcomeError(sr); err != nil { + return err } return nil @@ -122,6 +139,13 @@ func watchExecStatus(f *cmdutil.Factory, client *khhttp.Client, host, executionI case <-ticker.C: sr, err := fetchExecStatus(client, host, executionID) if err != nil { + var apiErr *khhttp.APIError + if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound { + if isTTY && !p.IsJSON() { + fmt.Fprintf(f.IOStreams.Out, "\r%s not_found", executionID) + } + continue + } return err } diff --git a/cmd/execute/transfer.go b/cmd/execute/transfer.go index 04f7589..751a926 100644 --- a/cmd/execute/transfer.go +++ b/cmd/execute/transfer.go @@ -3,11 +3,14 @@ package execute import ( "bytes" "encoding/json" + "errors" "fmt" "net/http" + "strings" "time" "github.com/jedib0t/go-pretty/v6/table" + "github.com/keeperhub/cli/internal/execrecovery" khhttp "github.com/keeperhub/cli/internal/http" "github.com/keeperhub/cli/internal/output" "github.com/keeperhub/cli/pkg/cmdutil" @@ -60,6 +63,7 @@ func NewTransferCmd(f *cmdutil.Factory) *cobra.Command { tokenAddress, _ := cmd.Flags().GetString("token-address") wait, _ := cmd.Flags().GetBool("wait") timeout, _ := cmd.Flags().GetDuration("timeout") + idemKeyFlag, _ := cmd.Flags().GetString("idempotency-key") body := transferRequest{ Network: chain, @@ -78,11 +82,18 @@ func NewTransferCmd(f *cmdutil.Factory) *cobra.Command { return fmt.Errorf("marshalling request: %w", err) } + idemKey, err := execrecovery.ResolveIdempotencyKey(idemKeyFlag) + if err != nil { + return err + } + req, err := client.NewRequest(http.MethodPost, khhttp.BuildBaseURL(host)+"/api/execute/transfer", bytes.NewReader(bodyBytes)) if err != nil { return err } req.Header.Set("Content-Type", "application/json") + // Set once before Do so go-retryablehttp retries reuse the same key (R3). + req.Header.Set(execrecovery.IdempotencyHeader, idemKey) resp, err := client.Do(req) if err != nil { @@ -124,6 +135,7 @@ func NewTransferCmd(f *cmdutil.Factory) *cobra.Command { cmd.Flags().String("token-address", "", "ERC-20 token contract address") cmd.Flags().Bool("wait", false, "Wait for completion") cmd.Flags().Duration("timeout", 5*time.Minute, "Timeout when using --wait") + cmd.Flags().String("idempotency-key", "", "Stable Idempotency-Key for this write intent (auto-generated if empty)") _ = cmd.MarkFlagRequired("chain") _ = cmd.MarkFlagRequired("to") @@ -153,16 +165,28 @@ func pollExecStatus(f *cmdutil.Factory, client *khhttp.Client, host, executionID case <-ticker.C: statusResp, err := fetchExecStatus(client, host, executionID) if err != nil { + var apiErr *khhttp.APIError + // R6: tolerate cold-start 404 until the wait deadline. + if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound { + if time.Now().After(deadline) { + return fmt.Errorf("timeout after %s: execution %s not found", timeout, executionID) + } + continue + } return err } + if statusResp.Status == "not_found" { + if time.Now().After(deadline) { + return fmt.Errorf("timeout after %s: execution %s not found", timeout, executionID) + } + continue + } + if execTerminalStatuses[statusResp.Status] { - if statusResp.Status == "failed" { - msg := fmt.Sprintf("execution %s failed", executionID) - if statusResp.Error != nil { - msg = *statusResp.Error - } - return fmt.Errorf("%s", msg) + if err := execOutcomeError(statusResp); err != nil { + _ = printExecStatusResult(p, statusResp) + return err } return printExecStatusResult(p, statusResp) } @@ -192,6 +216,10 @@ func fetchExecStatus(client *khhttp.Client, host, executionID string) (*ExecStat } defer resp.Body.Close() + if resp.StatusCode == http.StatusNotFound { + return nil, khhttp.NewAPIError(resp) + } + if resp.StatusCode != http.StatusOK { return nil, khhttp.NewAPIError(resp) } @@ -213,6 +241,26 @@ func printExecStatusResult(p *output.Printer, sr *ExecStatusResponse) error { if sr.TransactionLink != nil && *sr.TransactionLink != "" { tw.AppendRow(table.Row{"TX Link", *sr.TransactionLink}) } + for i, r := range sr.Receipts { + tw.AppendRow(table.Row{fmt.Sprintf("Receipt[%d]", i), fmt.Sprintf("%s verified=%v status=%s", r.Hash, r.Verified, r.ReceiptStatus)}) + } tw.Render() }) } + +// execOutcomeError returns a non-nil error for failed / reverted terminal states. +func execOutcomeError(sr *ExecStatusResponse) error { + if sr.Status == "failed" { + msg := fmt.Sprintf("execution %s failed", sr.ExecutionID) + if sr.Error != nil && *sr.Error != "" { + msg = *sr.Error + } + return fmt.Errorf("%s", msg) + } + for _, r := range sr.Receipts { + if strings.EqualFold(r.ReceiptStatus, "reverted") { + return fmt.Errorf("execution %s completed but transaction reverted (%s)", sr.ExecutionID, r.Hash) + } + } + return nil +} diff --git a/cmd/execute/transfer_recovery_test.go b/cmd/execute/transfer_recovery_test.go new file mode 100644 index 0000000..a5abbdf --- /dev/null +++ b/cmd/execute/transfer_recovery_test.go @@ -0,0 +1,147 @@ +package execute_test + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/keeperhub/cli/cmd/execute" + "github.com/keeperhub/cli/internal/execrecovery" + "github.com/keeperhub/cli/pkg/iostreams" +) + +func TestTransferCmd_SendsIdempotencyKey(t *testing.T) { + var gotKey string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotKey = r.Header.Get(execrecovery.IdempotencyHeader) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-idem","status":"completed"}`)) + })) + defer srv.Close() + + ios, _, _, _ := iostreams.Test() + f := newTransferFactory(ios, srv) + cmd := execute.NewTransferCmd(f) + cmd.SetArgs([]string{"--chain", "1", "--to", "0xabc", "--amount", "0.1", "--idempotency-key", "stable-intent-1"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if gotKey != "stable-intent-1" { + t.Fatalf("Idempotency-Key=%q, want stable-intent-1", gotKey) + } +} + +func TestTransferCmd_IdempotencyKeyStableAcrossHTTPRetries(t *testing.T) { + var keys []string + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := calls.Add(1) + keys = append(keys, r.Header.Get(execrecovery.IdempotencyHeader)) + if n == 1 { + w.WriteHeader(http.StatusBadGateway) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-retry","status":"completed"}`)) + })) + defer srv.Close() + + ios, _, _, _ := iostreams.Test() + f := newTransferFactory(ios, srv) + cmd := execute.NewTransferCmd(f) + cmd.SetArgs([]string{"--chain", "1", "--to", "0xabc", "--amount", "0.1", "--idempotency-key", "retry-stable"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if calls.Load() < 2 { + t.Fatalf("expected HTTP retry, got %d calls", calls.Load()) + } + for i, k := range keys { + if k != "retry-stable" { + t.Fatalf("call %d Idempotency-Key=%q, want retry-stable", i, k) + } + } +} + +func TestTransferCmd_WaitToleratesInitialNotFound(t *testing.T) { + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.Method == http.MethodPost { + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-cold","status":"pending"}`)) + return + } + n := calls.Add(1) + if n == 1 { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"Execution not found","code":"not_found"}`)) + return + } + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"executionId":"exec-cold","status":"completed","transactionHash":"0xabc"}`)) + })) + defer srv.Close() + + ios, buf, _, _ := iostreams.Test() + f := newTransferFactory(ios, srv) + cmd := execute.NewTransferCmd(f) + cmd.SetArgs([]string{"--chain", "1", "--to", "0xabc", "--amount", "0.1", "--wait", "--timeout", "15s"}) + + start := time.Now() + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if time.Since(start) > 14*time.Second { + t.Fatal("cold-start wait took too long") + } + out := buf.String() + if !strings.Contains(out, "exec-cold") { + t.Fatalf("expected execution in output, got %q", out) + } + if calls.Load() < 2 { + t.Fatalf("expected cold-start poll, got %d status calls", calls.Load()) + } +} + +func TestTransferCmd_WaitFailsOnRevertedReceipt(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.Method == http.MethodPost { + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-rev","status":"pending"}`)) + return + } + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(map[string]any{ + "executionId": "exec-rev", + "status": "completed", + "transactionHash": "0xrev", + "receipts": []map[string]any{ + {"hash": "0xrev", "chainId": 8453, "verified": true, "receiptStatus": "reverted"}, + }, + }) + })) + defer srv.Close() + + ios, _, _, _ := iostreams.Test() + f := newTransferFactory(ios, srv) + cmd := execute.NewTransferCmd(f) + cmd.SetArgs([]string{"--chain", "1", "--to", "0xabc", "--amount", "0.1", "--wait", "--timeout", "10s"}) + + err := cmd.Execute() + if err == nil { + t.Fatal("expected reverted receipt to fail") + } + if !strings.Contains(err.Error(), "reverted") { + t.Fatalf("expected reverted error, got %v", err) + } +} diff --git a/docs/execution-recovery-v1/contract.md b/docs/execution-recovery-v1/contract.md index 64463e2..6d512c6 100644 --- a/docs/execution-recovery-v1/contract.md +++ b/docs/execution-recovery-v1/contract.md @@ -1,52 +1,71 @@ # Execution recovery contract (normative) -Version: **1.0.0** -Audience: KeeperHub CLI / MCP / HTTP adapter authors +Version: **1.1.0** +Audience: KeeperHub CLI / MCP / HTTP adapter authors +Published path: this file is synced to docs.keeperhub.com via `docs/execution-recovery.md`. ## Definitions -- **Write**: any API call that may create or re-drive an onchain side effect (execute workflow, transfer, contract call). -- **Status read**: polling `get_execution` / `GET /api/execute/{id}/status` (or equivalent). -- **Chain evidence**: at least one transaction hash with a verified successful receipt for the expected predicate (token, amount, recipient, chain). -- **Idempotency key**: client-supplied key that must be byte-identical across retries of the same logical write. +- **Write**: any API call that may create or re-drive an onchain side effect (execute transfer, contract call, workflow webhook). +- **Status read**: polling `GET /api/execute/{id}/status` (direct execution) or workflow run status (separate vocabulary). +- **Chain evidence**: at least one receipt with `verified=true` and `receiptStatus=success` for the expected predicate. +- **Idempotency key**: client-supplied `Idempotency-Key` header that must be byte-identical across retries of the same logical write. + +## Status vocabularies (do not mix) + +| Surface | Pending | Terminal | +| --- | --- | --- | +| Direct execution (`/api/execute/.../status`) | `pending`, `running`, `queued`, `unconfirmed`, transport `not_found` | `completed`, `failed` | +| Workflow run (`/api/workflows/executions/.../status`) | `pending`, `running` | `success`, `error`, `cancelled` | + +`completed` is **not** proof of onchain success. Inspect `receipts[].receiptStatus`. ## Rules ### R1 — Unconfirmed → poll, do not resubmit -If status is `queued`, `running`, `pending`, or `unconfirmed`, the client MUST continue status reads against the **same** execution ID. The client MUST NOT issue a new write for the same logical intent while that execution ID remains durable. +If status is `pending`, `running`, `queued`, or `unconfirmed`, continue status reads against the **same** execution ID. Do not issue a new write for the same logical intent while that execution ID remains durable. + +**CLI conformance:** `kh ex transfer --wait` / `kh ex cc --wait` poll the same ID. -### R2 — No chain evidence → fail closed +### R2 — Chain evidence / reverted receipts -If the caller requires payment/landing proof and status is `completed` (or `success`) but no transaction hash / verified receipt is present, the client MUST treat the outcome as **not successful** for payment purposes. +1. If any receipt has `receiptStatus=reverted`, the outcome is **Failure** even when `status=completed` and `verified=true`. +2. When the caller requires payment/landing proof (`RequireChainEvidence` / `--require-verified`), `completed` without a verified successful receipt is **Failure**. + +**CLI conformance:** wait paths fail closed on reverted receipts. Strict “no receipt ⇒ fail” is opt-in (see also PR discussion for `--require-verified`). ### R3 — Write retry → stable idempotency key -If a write is retried after transport failure (timeout, 429, 5xx) before an execution ID is known, the client MUST reuse the same idempotency key. After an execution ID is known, prefer R1. +If a write is retried after transport failure (timeout, 5xx) before an execution ID is known, the client MUST reuse the same `Idempotency-Key`. After an execution ID is known, prefer R1. + +**CLI conformance:** `kh ex transfer` and `kh ex cc` (writes) set `Idempotency-Key` once per invocation before `Do`, so HTTP-layer retries reuse it. Use `--idempotency-key` to pin a key across process restarts. -### R4 — Terminal failure +### R4 — Terminal failure / malformed -Statuses `failed`, `reverted`, `not_found`, and unparseable/malformed bodies are terminal for that attempt. Clients MUST NOT invent a success path from partial fields. +Statuses `failed` (direct) and unparseable/malformed bodies are terminal for that attempt. Missing `status` after a 200 decode is **malformed**, not success. Do not invent a success path from partial fields. ### R5 — Rate limit -HTTP `429` responses require backoff. They are not success. Preserve the idempotency key for the next write attempt of the same intent. +HTTP `429` responses require backoff. They are not success. Preserve the idempotency key for the next write attempt of the same intent. The HTTP client does not auto-retry 429. + +### R6 — Cold start `not_found` -### R6 — Cold start +A first status read that returns HTTP 404 / `not_found` immediately after submit may be transient. During `--wait`, poll briefly before treating not_found as terminal timeout. -A first status read that returns `not_found` immediately after submit may be transient. Clients SHOULD poll briefly before concluding not_found as terminal, unless the product documents otherwise. +## Fixture mapping + conformance -## Fixture mapping +| Fixture | Rule | Expect | Consumed by | +| --- | --- | --- | --- | +| `queued.json` | R1 | pending | `TestFixtures_ClassifyTable` | +| `unconfirmed.json` | R1 | pending | `TestFixtures_ClassifyTable` | +| `completed_with_tx.json` | R2 | success (strict) | `TestFixtures_ClassifyTable` | +| `completed_without_tx.json` | R2 | failure (strict) | `TestFixtures_ClassifyTable` | +| `reverted.json` | R2 | failure | `TestFixtures_ClassifyTable` + `TestRevertedIsNeverSuccess` | +| `failed.json` | R4 | failure | `TestFixtures_ClassifyTable` | +| `malformed.json` | R4 | malformed | `TestFixtures_ClassifyTable` | +| `not_found.json` | R6 | pending | `TestFixtures_ClassifyTable` | +| `rate_limited.json` | R5 | rate_limited | `TestFixtures_ClassifyTable` | +| `cold_start.sequence.json` | R6 | 404→pending→success | `TestColdStartSequence_R6` | -| Fixture file | Exercises | -| --- | --- | -| `queued.json` | R1 | -| `completed_with_tx.json` | happy path with chain evidence | -| `completed_without_tx.json` | R2 | -| `failed.json` | R4 | -| `unconfirmed.json` | R1 | -| `reverted.json` | R4 | -| `not_found.json` | R4 / R6 | -| `rate_limited.json` | R5 | -| `cold_start.json` | R6 | -| `malformed.json` | R4 | +Fixtures use the **flat** direct-execution wire shape (`executionId`, not nested `execution.id`) so `json.Unmarshal` into `DirectStatus` / `ExecStatusResponse` cannot silently zero-decode. diff --git a/docs/execution-recovery.md b/docs/execution-recovery.md new file mode 100644 index 0000000..9b3ae27 --- /dev/null +++ b/docs/execution-recovery.md @@ -0,0 +1,29 @@ +# Execution recovery + +Agents and adapters that submit KeeperHub writes must recover safely when the +network flakes, a status read races ahead of persistence, or an onchain +receipt reverts. + +This guide is the published summary of the normative contract in +[`execution-recovery-v1/contract.md`](./execution-recovery-v1/contract.md). + +## Safe first-write sequence + +1. Simulate when available (`"simulate": true`) and continue only if the call would not revert. +2. Broadcast once with a stable `Idempotency-Key` that names the **work**, not the attempt. +3. Save `executionId`. +4. Poll `GET /api/execute/{executionId}/status`. +5. Treat `receipts[]` as authoritative: `verified` + `receiptStatus=success` prove landing. `receiptStatus=reverted` is failure even if `status=completed`. + +See also the Direct Execution API docs. + +## CLI behaviour + +- `kh ex transfer` / `kh ex cc` attach `Idempotency-Key` automatically; override with `--idempotency-key`. +- `--wait` polls the same execution ID, tolerates a bounded initial `404`/`not_found`, and fails closed on reverted receipts. +- Workflow run status uses a different vocabulary (`success` / `error` / `cancelled`) — do not mix it with direct-execution statuses. + +## Fixtures + +Golden responses live under `testdata/execution_recovery_v1/` and are loaded by +`go test ./internal/execrecovery/...`. diff --git a/docs/generate.go b/docs/generate.go index 236c253..0ef8c69 100644 --- a/docs/generate.go +++ b/docs/generate.go @@ -41,8 +41,8 @@ func main() { // pruneGeneratedPages removes the generated command reference from dir. // // Only `kh*.md` is touched: the hand-written guides (quickstart.md, -// concepts.md) and the generator's own sources live alongside it and must -// survive. +// concepts.md, execution-recovery.md) and the generator's own sources live +// alongside it and must survive. func pruneGeneratedPages(dir string) error { matches, err := filepath.Glob(filepath.Join(dir, "kh*.md")) if err != nil { diff --git a/docs/kh_execute_contract-call.md b/docs/kh_execute_contract-call.md index cb117fa..40fe13d 100644 --- a/docs/kh_execute_contract-call.md +++ b/docs/kh_execute_contract-call.md @@ -19,14 +19,15 @@ kh execute contract-call [flags] ### Options ``` - --abi-file string Path to local ABI JSON file - --args string Method arguments as JSON array: '["arg1","arg2"]' - --chain string Chain ID (required) - --contract string Contract address (required) - -h, --help help for contract-call - --method string Method name (required) - --timeout duration Timeout when using --wait (default 5m0s) - --wait Wait for completion + --abi-file string Path to local ABI JSON file + --args string Method arguments as JSON array: '["arg1","arg2"]' + --chain string Chain ID (required) + --contract string Contract address (required) + -h, --help help for contract-call + --idempotency-key string Stable Idempotency-Key for write intents (auto-generated if empty) + --method string Method name (required) + --timeout duration Timeout when using --wait (default 5m0s) + --wait Wait for completion ``` ### Options inherited from parent commands diff --git a/docs/kh_execute_transfer.md b/docs/kh_execute_transfer.md index 4fda686..783f7b0 100644 --- a/docs/kh_execute_transfer.md +++ b/docs/kh_execute_transfer.md @@ -19,14 +19,15 @@ kh execute transfer [flags] ### Options ``` - --amount string Amount to transfer (required) - --chain string Chain ID (required) - -h, --help help for transfer - --timeout duration Timeout when using --wait (default 5m0s) - --to string Recipient address (required) - --token string Token symbol (default "ETH") - --token-address string ERC-20 token contract address - --wait Wait for completion + --amount string Amount to transfer (required) + --chain string Chain ID (required) + -h, --help help for transfer + --idempotency-key string Stable Idempotency-Key for this write intent (auto-generated if empty) + --timeout duration Timeout when using --wait (default 5m0s) + --to string Recipient address (required) + --token string Token symbol (default "ETH") + --token-address string ERC-20 token contract address + --wait Wait for completion ``` ### Options inherited from parent commands diff --git a/internal/execrecovery/classify.go b/internal/execrecovery/classify.go new file mode 100644 index 0000000..d61912c --- /dev/null +++ b/internal/execrecovery/classify.go @@ -0,0 +1,160 @@ +// Package execrecovery implements the execution-recovery contract (R1–R6) +// used by fixture conformance tests and by direct-execution wait paths. +package execrecovery + +import ( + "encoding/json" + "fmt" + "net/http" + "strings" +) + +// Outcome is the classified result of one status observation. +type Outcome string + +const ( + OutcomePending Outcome = "pending" + OutcomeSuccess Outcome = "success" + OutcomeFailure Outcome = "failure" + OutcomeMalformed Outcome = "malformed" + OutcomeRateLimited Outcome = "rate_limited" +) + +// Options controls classification strictness. +type Options struct { + // RequireChainEvidence enables R2 strict mode: completed without a + // verified successful receipt is Failure, not Success. + RequireChainEvidence bool +} + +// Receipt is a chain-re-fetched proof entry (direct-execution status API). +type Receipt struct { + Hash string `json:"hash"` + ChainID int64 `json:"chainId"` + Verified bool `json:"verified"` + ReceiptStatus string `json:"receiptStatus"` +} + +// DirectStatus is the flat wire shape of GET /api/execute/{id}/status. +type DirectStatus struct { + ExecutionID string `json:"executionId"` + Status string `json:"status"` + Type string `json:"type"` + TransactionHash *string `json:"transactionHash"` + TransactionLink *string `json:"transactionLink"` + Result any `json:"result"` + Error *string `json:"error"` + CreatedAt string `json:"createdAt"` + CompletedAt *string `json:"completedAt"` + Receipts []Receipt `json:"receipts,omitempty"` +} + +// Sample is one HTTP observation of an execution status endpoint. +type Sample struct { + HTTPStatus int + Body []byte +} + +// Classify maps one status observation to an Outcome. +// +// Vocabulary note: direct-execution statuses are pending|running|completed|failed +// (and transport-level not_found). Workflow run statuses (success|error|cancelled) +// belong to a different API and must not be fed here — see Vocabulary(). +func Classify(sample Sample, opts Options) (Outcome, string) { + if sample.HTTPStatus == http.StatusTooManyRequests { + return OutcomeRateLimited, "HTTP 429" + } + + // Cold-start / missing: callers may poll again (R6). Terminal failure is a + // poll-budget decision, not Classify's. + if sample.HTTPStatus == http.StatusNotFound { + return OutcomePending, "not_found" + } + + if sample.HTTPStatus != 0 && sample.HTTPStatus != http.StatusOK && sample.HTTPStatus != http.StatusAccepted { + // Non-404 errors are failures for a status read. + if sample.HTTPStatus >= 400 { + return OutcomeFailure, fmt.Sprintf("HTTP %d", sample.HTTPStatus) + } + } + + if len(sample.Body) == 0 { + return OutcomeMalformed, "empty body" + } + + trimmed := strings.TrimSpace(string(sample.Body)) + if !json.Valid([]byte(trimmed)) { + return OutcomeMalformed, "unparseable body" + } + + var st DirectStatus + if err := json.Unmarshal([]byte(trimmed), &st); err != nil { + return OutcomeMalformed, "json decode failed" + } + + status := strings.ToLower(strings.TrimSpace(st.Status)) + if status == "" { + // Unrecognised schema: valid JSON but no status field. + return OutcomeMalformed, "missing status field" + } + + switch status { + case "pending", "running", "queued", "unconfirmed": + return OutcomePending, status + case "not_found": + return OutcomePending, status + case "failed", "error", "cancelled": + return OutcomeFailure, status + case "completed", "success": + return classifyCompleted(st, opts) + default: + return OutcomeMalformed, "unrecognised status: " + status + } +} + +func classifyCompleted(st DirectStatus, opts Options) (Outcome, string) { + if hasRevertedReceipt(st.Receipts) { + return OutcomeFailure, "receiptStatus=reverted" + } + + if hasVerifiedSuccess(st.Receipts) { + return OutcomeSuccess, "verified successful receipt" + } + + if opts.RequireChainEvidence { + if st.TransactionHash == nil || strings.TrimSpace(*st.TransactionHash) == "" { + return OutcomeFailure, "completed without transaction hash" + } + if len(st.Receipts) == 0 { + return OutcomeFailure, "completed without verified successful receipt" + } + return OutcomeFailure, "no verified successful receipt" + } + + // Compatible default: completed without receipts is still Success for + // callers that have not opted into R2 strict mode (see --require-verified). + return OutcomeSuccess, "completed" +} + +func hasRevertedReceipt(receipts []Receipt) bool { + for _, r := range receipts { + if strings.EqualFold(r.ReceiptStatus, "reverted") { + return true + } + } + return false +} + +func hasVerifiedSuccess(receipts []Receipt) bool { + for _, r := range receipts { + if r.Verified && strings.EqualFold(r.ReceiptStatus, "success") { + return true + } + } + return false +} + +// HasRevertedReceipt reports whether any receipt is an onchain revert. +func HasRevertedReceipt(receipts []Receipt) bool { + return hasRevertedReceipt(receipts) +} diff --git a/internal/execrecovery/fixture.go b/internal/execrecovery/fixture.go new file mode 100644 index 0000000..6277459 --- /dev/null +++ b/internal/execrecovery/fixture.go @@ -0,0 +1,112 @@ +package execrecovery + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" +) + +// Fixture is one conformance case loaded from testdata/execution_recovery_v1. +type Fixture struct { + Name string `json:"-"` + Rule string `json:"rule"` + HTTPStatus int `json:"httpStatus"` + RequireChainEvidence bool `json:"requireChainEvidence"` + Expect Outcome `json:"expect"` + Response json.RawMessage `json:"response"` + ResponseRaw string `json:"responseRaw,omitempty"` + Note string `json:"note,omitempty"` +} + +// SequenceStep is one observation in a multi-response cold-start sequence. +type SequenceStep struct { + HTTPStatus int `json:"httpStatus"` + RequireChainEvidence bool `json:"requireChainEvidence"` + Expect Outcome `json:"expect"` + Response json.RawMessage `json:"response"` + ResponseRaw string `json:"responseRaw,omitempty"` +} + +// SequenceFixture exercises multi-poll recovery (R6). +type SequenceFixture struct { + Name string `json:"name"` + Rule string `json:"rule"` + Steps []SequenceStep `json:"steps"` +} + +// LoadFixtureDir loads every *.json fixture (not *.sequence.json) from dir. +func LoadFixtureDir(dir string) ([]Fixture, error) { + entries, err := os.ReadDir(dir) + if err != nil { + return nil, err + } + var out []Fixture + for _, e := range entries { + if e.IsDir() { + continue + } + name := e.Name() + if filepath.Ext(name) != ".json" { + continue + } + if len(name) >= len(".sequence.json") && name[len(name)-len(".sequence.json"):] == ".sequence.json" { + continue + } + path := filepath.Join(dir, name) + raw, err := os.ReadFile(path) + if err != nil { + return nil, err + } + var f Fixture + if err := json.Unmarshal(raw, &f); err != nil { + return nil, fmt.Errorf("%s: %w", name, err) + } + f.Name = name + out = append(out, f) + } + return out, nil +} + +// LoadSequence loads a multi-step sequence fixture. +func LoadSequence(path string) (SequenceFixture, error) { + raw, err := os.ReadFile(path) + if err != nil { + return SequenceFixture{}, err + } + var seq SequenceFixture + if err := json.Unmarshal(raw, &seq); err != nil { + return SequenceFixture{}, err + } + return seq, nil +} + +// Sample converts a fixture into a Classify input. +func (f Fixture) Sample() Sample { + body := []byte(f.ResponseRaw) + if len(body) == 0 { + body = f.Response + } + return Sample{HTTPStatus: f.HTTPStatus, Body: body} +} + +// Sample converts a sequence step into a Classify input. +func (s SequenceStep) Sample() Sample { + body := []byte(s.ResponseRaw) + if len(body) == 0 { + body = s.Response + } + return Sample{HTTPStatus: s.HTTPStatus, Body: body} +} + +// DecodeResponse unmarshals the flat DirectStatus wire body. +func (f Fixture) DecodeResponse() (DirectStatus, error) { + if f.ResponseRaw != "" { + return DirectStatus{}, fmt.Errorf("raw body is not DirectStatus JSON") + } + var st DirectStatus + if err := json.Unmarshal(f.Response, &st); err != nil { + return DirectStatus{}, err + } + return st, nil +} diff --git a/internal/execrecovery/fixture_test.go b/internal/execrecovery/fixture_test.go new file mode 100644 index 0000000..293cc96 --- /dev/null +++ b/internal/execrecovery/fixture_test.go @@ -0,0 +1,142 @@ +package execrecovery_test + +import ( + "path/filepath" + "runtime" + "testing" + + "github.com/keeperhub/cli/internal/execrecovery" +) + +func testdataDir(t *testing.T) string { + t.Helper() + _, file, _, ok := runtime.Caller(0) + if !ok { + t.Fatal("runtime.Caller failed") + } + // internal/execrecovery -> repo root + root := filepath.Clean(filepath.Join(filepath.Dir(file), "..", "..")) + return filepath.Join(root, "testdata", "execution_recovery_v1") +} + +func TestFixtures_DecodeIntoDirectStatus(t *testing.T) { + fixtures, err := execrecovery.LoadFixtureDir(testdataDir(t)) + if err != nil { + t.Fatalf("LoadFixtureDir: %v", err) + } + if len(fixtures) == 0 { + t.Fatal("no fixtures loaded") + } + + for _, f := range fixtures { + f := f + t.Run(f.Name, func(t *testing.T) { + if f.ResponseRaw != "" { + // Malformed raw bodies are not DirectStatus JSON. + st, err := f.DecodeResponse() + if err == nil { + t.Fatalf("expected decode error for raw fixture, got %#v", st) + } + return + } + st, err := f.DecodeResponse() + if err != nil { + // not_found / rate_limited error bodies are not DirectStatus; + // Classify still handles them via HTTP status. + if f.HTTPStatus == 404 || f.HTTPStatus == 429 { + return + } + t.Fatalf("DecodeResponse: %v", err) + } + if f.HTTPStatus == 200 && f.Expect != execrecovery.OutcomeMalformed { + if st.ExecutionID == "" && f.Expect != execrecovery.OutcomeFailure { + // failed fixture has executionId; ensure we never silently zero-decode. + } + if st.Status == "" && f.Expect != execrecovery.OutcomeMalformed { + t.Fatalf("decoded empty Status for fixture %s — wire shape mismatch", f.Name) + } + } + }) + } +} + +func TestFixtures_ClassifyTable(t *testing.T) { + fixtures, err := execrecovery.LoadFixtureDir(testdataDir(t)) + if err != nil { + t.Fatalf("LoadFixtureDir: %v", err) + } + + for _, f := range fixtures { + f := f + t.Run(f.Rule+"/"+f.Name, func(t *testing.T) { + got, reason := execrecovery.Classify(f.Sample(), execrecovery.Options{ + RequireChainEvidence: f.RequireChainEvidence, + }) + if got != f.Expect { + t.Fatalf("Classify=%s (%s), want %s", got, reason, f.Expect) + } + }) + } +} + +func TestColdStartSequence_R6(t *testing.T) { + path := filepath.Join(testdataDir(t), "cold_start.sequence.json") + seq, err := execrecovery.LoadSequence(path) + if err != nil { + t.Fatalf("LoadSequence: %v", err) + } + if seq.Rule != "R6" { + t.Fatalf("rule=%s, want R6", seq.Rule) + } + if len(seq.Steps) < 2 { + t.Fatal("cold_start sequence must have at least 2 steps") + } + for i, step := range seq.Steps { + got, reason := execrecovery.Classify(step.Sample(), execrecovery.Options{ + RequireChainEvidence: step.RequireChainEvidence, + }) + if got != step.Expect { + t.Fatalf("step %d: Classify=%s (%s), want %s", i, got, reason, step.Expect) + } + } +} + +func TestRevertedIsNeverSuccess(t *testing.T) { + body := []byte(`{ + "executionId":"x", + "status":"completed", + "transactionHash":"0xabc", + "receipts":[{"hash":"0xabc","chainId":8453,"verified":true,"receiptStatus":"reverted"}] + }`) + got, reason := execrecovery.Classify(execrecovery.Sample{HTTPStatus: 200, Body: body}, execrecovery.Options{ + RequireChainEvidence: true, + }) + if got != execrecovery.OutcomeFailure { + t.Fatalf("got %s (%s), want failure", got, reason) + } +} + +func TestEmptyStatusIsMalformed(t *testing.T) { + got, _ := execrecovery.Classify(execrecovery.Sample{ + HTTPStatus: 200, + Body: []byte(`{"executionId":"x"}`), + }, execrecovery.Options{}) + if got != execrecovery.OutcomeMalformed { + t.Fatalf("got %s, want malformed", got) + } +} + +func TestVocabularySurfacesAreDistinct(t *testing.T) { + d := execrecovery.DirectExecutionVocabulary() + w := execrecovery.WorkflowRunVocabulary() + if d.Surface == w.Surface { + t.Fatal("vocabularies must name distinct surfaces") + } + for _, term := range w.Terminal { + for _, dTerm := range d.Terminal { + if term == dTerm { + t.Fatalf("shared terminal term %q across surfaces — keep vocabularies separate", term) + } + } + } +} diff --git a/internal/execrecovery/idempotency.go b/internal/execrecovery/idempotency.go new file mode 100644 index 0000000..1246fd0 --- /dev/null +++ b/internal/execrecovery/idempotency.go @@ -0,0 +1,29 @@ +package execrecovery + +import ( + "crypto/rand" + "encoding/hex" + "fmt" +) + +const IdempotencyHeader = "Idempotency-Key" + +// NewIdempotencyKey returns a random UUID-like key for a single write intent. +// Callers that retry the same intent across process restarts must persist or +// derive a stable key instead (see docs.keeperhub.com/api/direct-execution). +func NewIdempotencyKey() (string, error) { + var b [16]byte + if _, err := rand.Read(b[:]); err != nil { + return "", fmt.Errorf("generating idempotency key: %w", err) + } + // UUID v4 layout bits are not required by the API; hex is sufficient. + return hex.EncodeToString(b[:]), nil +} + +// ResolveIdempotencyKey returns explicit if non-empty, otherwise a new key. +func ResolveIdempotencyKey(explicit string) (string, error) { + if explicit != "" { + return explicit, nil + } + return NewIdempotencyKey() +} diff --git a/internal/execrecovery/idempotency_test.go b/internal/execrecovery/idempotency_test.go new file mode 100644 index 0000000..57a62dc --- /dev/null +++ b/internal/execrecovery/idempotency_test.go @@ -0,0 +1,38 @@ +package execrecovery_test + +import ( + "testing" + + "github.com/keeperhub/cli/internal/execrecovery" +) + +func TestResolveIdempotencyKey_ExplicitStable(t *testing.T) { + a, err := execrecovery.ResolveIdempotencyKey("stable-key-1") + if err != nil { + t.Fatal(err) + } + b, err := execrecovery.ResolveIdempotencyKey("stable-key-1") + if err != nil { + t.Fatal(err) + } + if a != b || a != "stable-key-1" { + t.Fatalf("explicit key must be preserved: %q vs %q", a, b) + } +} + +func TestResolveIdempotencyKey_GeneratedUnique(t *testing.T) { + a, err := execrecovery.ResolveIdempotencyKey("") + if err != nil { + t.Fatal(err) + } + b, err := execrecovery.ResolveIdempotencyKey("") + if err != nil { + t.Fatal(err) + } + if a == "" || b == "" { + t.Fatal("generated keys must be non-empty") + } + if a == b { + t.Fatal("different intents must get different generated keys") + } +} diff --git a/internal/execrecovery/vocabulary.go b/internal/execrecovery/vocabulary.go new file mode 100644 index 0000000..5fc01b1 --- /dev/null +++ b/internal/execrecovery/vocabulary.go @@ -0,0 +1,27 @@ +package execrecovery + +// Vocabulary documents which status strings belong to which API surface. +// Direct-execution and workflow-run statuses must not be mixed. +type Vocabulary struct { + Surface string + Pending []string + Terminal []string +} + +// DirectExecutionVocabulary is GET /api/execute/{id}/status. +func DirectExecutionVocabulary() Vocabulary { + return Vocabulary{ + Surface: "direct-execution", + Pending: []string{"pending", "running", "queued", "unconfirmed", "not_found"}, + Terminal: []string{"completed", "failed"}, + } +} + +// WorkflowRunVocabulary is GET /api/workflows/executions/{id}/status. +func WorkflowRunVocabulary() Vocabulary { + return Vocabulary{ + Surface: "workflow-run", + Pending: []string{"pending", "running"}, + Terminal: []string{"success", "error", "cancelled"}, + } +} diff --git a/testdata/execution_recovery_v1/cold_start.json b/testdata/execution_recovery_v1/cold_start.json deleted file mode 100644 index eb2a0d0..0000000 --- a/testdata/execution_recovery_v1/cold_start.json +++ /dev/null @@ -1,9 +0,0 @@ -{ - "httpStatus": 200, - "execution": { - "id": "exec_fixture_cold_001", - "status": "queued", - "workflowId": "wf_fixture_001", - "note": "First status read immediately after submit may still be queued; treat as cold start under R6." - } -} diff --git a/testdata/execution_recovery_v1/cold_start.sequence.json b/testdata/execution_recovery_v1/cold_start.sequence.json new file mode 100644 index 0000000..942e674 --- /dev/null +++ b/testdata/execution_recovery_v1/cold_start.sequence.json @@ -0,0 +1,40 @@ +{ + "name": "cold_start", + "rule": "R6", + "steps": [ + { + "httpStatus": 404, + "expect": "pending", + "response": { + "error": "Execution not found", + "code": "not_found" + } + }, + { + "httpStatus": 200, + "expect": "pending", + "response": { + "executionId": "exec_fixture_cold_001", + "status": "pending" + } + }, + { + "httpStatus": 200, + "requireChainEvidence": true, + "expect": "success", + "response": { + "executionId": "exec_fixture_cold_001", + "status": "completed", + "transactionHash": "0x4444444444444444444444444444444444444444444444444444444444444444", + "receipts": [ + { + "hash": "0x4444444444444444444444444444444444444444444444444444444444444444", + "chainId": 8453, + "verified": true, + "receiptStatus": "success" + } + ] + } + } + ] +} diff --git a/testdata/execution_recovery_v1/completed_with_tx.json b/testdata/execution_recovery_v1/completed_with_tx.json index b48c687..a653a97 100644 --- a/testdata/execution_recovery_v1/completed_with_tx.json +++ b/testdata/execution_recovery_v1/completed_with_tx.json @@ -1,10 +1,13 @@ { + "rule": "R2", "httpStatus": 200, - "execution": { - "id": "exec_fixture_ok_001", + "requireChainEvidence": true, + "expect": "success", + "response": { + "executionId": "exec_fixture_ok_001", "status": "completed", - "workflowId": "wf_fixture_001", "transactionHash": "0x1111111111111111111111111111111111111111111111111111111111111111", + "transactionLink": "https://basescan.org/tx/0x1111111111111111111111111111111111111111111111111111111111111111", "receipts": [ { "hash": "0x1111111111111111111111111111111111111111111111111111111111111111", diff --git a/testdata/execution_recovery_v1/completed_without_tx.json b/testdata/execution_recovery_v1/completed_without_tx.json index 82240a1..21e28db 100644 --- a/testdata/execution_recovery_v1/completed_without_tx.json +++ b/testdata/execution_recovery_v1/completed_without_tx.json @@ -1,9 +1,12 @@ { + "rule": "R2", "httpStatus": 200, - "execution": { - "id": "exec_fixture_notx_001", + "requireChainEvidence": true, + "expect": "failure", + "response": { + "executionId": "exec_fixture_no_tx_001", "status": "completed", - "workflowId": "wf_fixture_001", "completedAt": "2026-08-11T00:01:00.000Z" - } + }, + "note": "completed without chain evidence must fail closed when proof is required." } diff --git a/testdata/execution_recovery_v1/failed.json b/testdata/execution_recovery_v1/failed.json index 9a067e3..e0657ec 100644 --- a/testdata/execution_recovery_v1/failed.json +++ b/testdata/execution_recovery_v1/failed.json @@ -1,10 +1,11 @@ { + "rule": "R4", "httpStatus": 200, - "execution": { - "id": "exec_fixture_failed_001", + "requireChainEvidence": false, + "expect": "failure", + "response": { + "executionId": "exec_fixture_failed_001", "status": "failed", - "workflowId": "wf_fixture_001", - "error": "action_failed", - "completedAt": "2026-08-11T00:01:00.000Z" + "error": "simulation reverted: insufficient funds" } } diff --git a/testdata/execution_recovery_v1/malformed.json b/testdata/execution_recovery_v1/malformed.json index efe1fb0..a905de8 100644 --- a/testdata/execution_recovery_v1/malformed.json +++ b/testdata/execution_recovery_v1/malformed.json @@ -1,7 +1,8 @@ { + "rule": "R4", "httpStatus": 200, - "data": { - "weird": true, - "statusCode": "DONE" - } + "requireChainEvidence": false, + "expect": "malformed", + "responseRaw": "{{{this is not json", + "note": "Genuine unparseable body — must not be treated as success or empty-status poll forever." } diff --git a/testdata/execution_recovery_v1/not_found.json b/testdata/execution_recovery_v1/not_found.json index b49b886..3dfabc2 100644 --- a/testdata/execution_recovery_v1/not_found.json +++ b/testdata/execution_recovery_v1/not_found.json @@ -1,7 +1,11 @@ { + "rule": "R4", "httpStatus": 404, - "error": { - "code": "not_found", - "message": "execution not found" - } + "requireChainEvidence": false, + "expect": "pending", + "response": { + "error": "Execution not found", + "code": "not_found" + }, + "note": "A single not_found is Pending (R6). Exhausted poll budget is a caller-level Failure." } diff --git a/testdata/execution_recovery_v1/queued.json b/testdata/execution_recovery_v1/queued.json index f43596b..eda3e25 100644 --- a/testdata/execution_recovery_v1/queued.json +++ b/testdata/execution_recovery_v1/queued.json @@ -1,9 +1,11 @@ { + "rule": "R1", "httpStatus": 200, - "execution": { - "id": "exec_fixture_queued_001", - "status": "queued", - "workflowId": "wf_fixture_001", + "requireChainEvidence": false, + "expect": "pending", + "response": { + "executionId": "exec_fixture_queued_001", + "status": "pending", "createdAt": "2026-08-11T00:00:00.000Z" } } diff --git a/testdata/execution_recovery_v1/rate_limited.json b/testdata/execution_recovery_v1/rate_limited.json index be0e28d..41f9e32 100644 --- a/testdata/execution_recovery_v1/rate_limited.json +++ b/testdata/execution_recovery_v1/rate_limited.json @@ -1,8 +1,10 @@ { + "rule": "R5", "httpStatus": 429, - "error": { - "code": "rate_limited", - "message": "too many requests", - "retryAfterSeconds": 2 + "requireChainEvidence": false, + "expect": "rate_limited", + "response": { + "error": "Rate limit exceeded", + "code": "rate_limited" } } diff --git a/testdata/execution_recovery_v1/reverted.json b/testdata/execution_recovery_v1/reverted.json index 32acd75..0122430 100644 --- a/testdata/execution_recovery_v1/reverted.json +++ b/testdata/execution_recovery_v1/reverted.json @@ -1,9 +1,11 @@ { + "rule": "R2", "httpStatus": 200, - "execution": { - "id": "exec_fixture_reverted_001", + "requireChainEvidence": true, + "expect": "failure", + "response": { + "executionId": "exec_fixture_reverted_001", "status": "completed", - "workflowId": "wf_fixture_001", "transactionHash": "0x3333333333333333333333333333333333333333333333333333333333333333", "receipts": [ { @@ -12,6 +14,8 @@ "verified": true, "receiptStatus": "reverted" } - ] - } + ], + "completedAt": "2026-08-11T00:01:00.000Z" + }, + "note": "verified=true does not mean successful; receiptStatus=reverted is Failure." } diff --git a/testdata/execution_recovery_v1/unconfirmed.json b/testdata/execution_recovery_v1/unconfirmed.json index e0e129f..bc25883 100644 --- a/testdata/execution_recovery_v1/unconfirmed.json +++ b/testdata/execution_recovery_v1/unconfirmed.json @@ -1,17 +1,13 @@ { + "rule": "R1", "httpStatus": 200, - "execution": { - "id": "exec_fixture_unconfirmed_001", + "requireChainEvidence": false, + "expect": "pending", + "response": { + "executionId": "exec_fixture_unconfirmed_001", "status": "unconfirmed", - "workflowId": "wf_fixture_001", "transactionHash": "0x2222222222222222222222222222222222222222222222222222222222222222", - "receipts": [ - { - "hash": "0x2222222222222222222222222222222222222222222222222222222222222222", - "chainId": 8453, - "verified": false, - "receiptStatus": "pending" - } - ] - } + "createdAt": "2026-08-11T00:00:00.000Z" + }, + "note": "Broadcast but not yet confirmed — poll same executionId; do not resubmit." } From d6e72e27237dc55d874a1a03e7ef64123db94111 Mon Sep 17 00:00:00 2001 From: mohamedwael201193 Date: Thu, 13 Aug 2026 10:48:10 +0300 Subject: [PATCH 3/7] test(execrecovery): close review gaps gofmt Fixture struct tags. Prove kh ex cc reuses Idempotency-Key across HTTP retries, matching transfer.go. --- cmd/execute/contract_call_test.go | 61 ++++++++++++++++++++++++++++++- internal/execrecovery/fixture.go | 16 ++++---- 2 files changed, 68 insertions(+), 9 deletions(-) diff --git a/cmd/execute/contract_call_test.go b/cmd/execute/contract_call_test.go index 4a3f00b..b332963 100644 --- a/cmd/execute/contract_call_test.go +++ b/cmd/execute/contract_call_test.go @@ -6,11 +6,13 @@ import ( "net/http/httptest" "os" "strings" + "sync/atomic" "testing" "time" "github.com/keeperhub/cli/cmd/execute" "github.com/keeperhub/cli/internal/config" + "github.com/keeperhub/cli/internal/execrecovery" khhttp "github.com/keeperhub/cli/internal/http" "github.com/keeperhub/cli/pkg/cmdutil" "github.com/keeperhub/cli/pkg/iostreams" @@ -20,7 +22,7 @@ func newContractCallFactory(ios *iostreams.IOStreams, srv *httptest.Server) *cmd client := khhttp.NewClient(khhttp.ClientOptions{ Host: srv.URL, AppVersion: "test", - IOStreams: ios, + IOStreams: ios, }) return &cmdutil.Factory{ IOStreams: ios, @@ -303,3 +305,60 @@ func TestContractCallCmd_WaitWritePolls(t *testing.T) { t.Errorf("expected tx hash in output, got: %q", out) } } + +func TestContractCallCmd_SendsIdempotencyKey(t *testing.T) { + var gotKey string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotKey = r.Header.Get(execrecovery.IdempotencyHeader) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-cc-idem","status":"completed"}`)) + })) + defer srv.Close() + + ios, _, _, _ := iostreams.Test() + f := newContractCallFactory(ios, srv) + cmd := execute.NewContractCallCmd(f) + cmd.SetArgs([]string{"--chain", "1", "--contract", "0xcontract", "--method", "transfer", "--idempotency-key", "stable-cc-1"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if gotKey != "stable-cc-1" { + t.Fatalf("Idempotency-Key=%q, want stable-cc-1", gotKey) + } +} + +func TestContractCallCmd_IdempotencyKeyStableAcrossHTTPRetries(t *testing.T) { + var keys []string + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := calls.Add(1) + keys = append(keys, r.Header.Get(execrecovery.IdempotencyHeader)) + if n == 1 { + w.WriteHeader(http.StatusBadGateway) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-cc-retry","status":"completed"}`)) + })) + defer srv.Close() + + ios, _, _, _ := iostreams.Test() + f := newContractCallFactory(ios, srv) + cmd := execute.NewContractCallCmd(f) + cmd.SetArgs([]string{"--chain", "1", "--contract", "0xcontract", "--method", "transfer", "--idempotency-key", "retry-stable-cc"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if calls.Load() < 2 { + t.Fatalf("expected HTTP retry, got %d calls", calls.Load()) + } + for i, k := range keys { + if k != "retry-stable-cc" { + t.Fatalf("call %d Idempotency-Key=%q, want retry-stable-cc", i, k) + } + } +} diff --git a/internal/execrecovery/fixture.go b/internal/execrecovery/fixture.go index 6277459..ba4ba14 100644 --- a/internal/execrecovery/fixture.go +++ b/internal/execrecovery/fixture.go @@ -9,14 +9,14 @@ import ( // Fixture is one conformance case loaded from testdata/execution_recovery_v1. type Fixture struct { - Name string `json:"-"` - Rule string `json:"rule"` - HTTPStatus int `json:"httpStatus"` - RequireChainEvidence bool `json:"requireChainEvidence"` - Expect Outcome `json:"expect"` - Response json.RawMessage `json:"response"` - ResponseRaw string `json:"responseRaw,omitempty"` - Note string `json:"note,omitempty"` + Name string `json:"-"` + Rule string `json:"rule"` + HTTPStatus int `json:"httpStatus"` + RequireChainEvidence bool `json:"requireChainEvidence"` + Expect Outcome `json:"expect"` + Response json.RawMessage `json:"response"` + ResponseRaw string `json:"responseRaw,omitempty"` + Note string `json:"note,omitempty"` } // SequenceStep is one observation in a multi-response cold-start sequence. From b90b8874b54ba705b0fd890e9c54fedc646f7f71 Mon Sep 17 00:00:00 2001 From: mohamedwael201193 Date: Mon, 17 Aug 2026 06:39:01 +0300 Subject: [PATCH 4/7] feat(execrecovery): harden retry and polling semantics Handle 409 idempotency_in_progress vs conflict by body code, bound --watch 404, and align receipts/docs with the server enum and KEEP-966. --- cmd/execute/contract_call.go | 11 +- cmd/execute/contract_call_test.go | 96 +++++++++ cmd/execute/idempotent_write.go | 76 +++++++ cmd/execute/status.go | 43 +--- cmd/execute/status_test.go | 107 ++++++++- cmd/execute/transfer.go | 37 ++-- cmd/execute/transfer_recovery_test.go | 203 +++++++++++++++++- docs/execution-recovery-v1/contract.md | 96 ++++++--- docs/execution-recovery.md | 56 ++++- internal/execrecovery/classify.go | 101 +++++---- .../execrecovery/classify_receipt_test.go | 48 +++++ internal/execrecovery/fixture.go | 90 +++++++- internal/execrecovery/fixture_test.go | 142 ++++++++++-- internal/execrecovery/idempotency.go | 87 +++++++- internal/execrecovery/idempotency_test.go | 36 ++++ internal/execrecovery/vocabulary.go | 5 +- .../cold_start.sequence.json | 12 +- .../completed_with_tx.json | 5 +- .../completed_without_tx.json | 4 +- testdata/execution_recovery_v1/failed.json | 2 + testdata/execution_recovery_v1/malformed.json | 4 +- testdata/execution_recovery_v1/not_found.json | 9 +- testdata/execution_recovery_v1/pending.json | 14 ++ .../execution_recovery_v1/rate_limited.json | 5 +- testdata/execution_recovery_v1/reverted.json | 9 +- .../{queued.json => running.json} | 6 +- .../safe_inner_failure.json | 23 ++ .../execution_recovery_v1/unconfirmed.json | 12 +- 28 files changed, 1135 insertions(+), 204 deletions(-) create mode 100644 cmd/execute/idempotent_write.go create mode 100644 internal/execrecovery/classify_receipt_test.go create mode 100644 testdata/execution_recovery_v1/pending.json rename testdata/execution_recovery_v1/{queued.json => running.json} (59%) create mode 100644 testdata/execution_recovery_v1/safe_inner_failure.json diff --git a/cmd/execute/contract_call.go b/cmd/execute/contract_call.go index eb263d1..95c187b 100644 --- a/cmd/execute/contract_call.go +++ b/cmd/execute/contract_call.go @@ -1,7 +1,6 @@ package execute import ( - "bytes" "encoding/json" "fmt" "net/http" @@ -95,14 +94,8 @@ func NewContractCallCmd(f *cmdutil.Factory) *cobra.Command { return err } - req, err := client.NewRequest(http.MethodPost, khhttp.BuildBaseURL(host)+"/api/execute/contract-call", bytes.NewReader(bodyBytes)) - if err != nil { - return err - } - req.Header.Set("Content-Type", "application/json") - req.Header.Set(execrecovery.IdempotencyHeader, idemKey) - - resp, err := client.Do(req) + deadline := time.Now().Add(timeout) + resp, err := postIdempotentJSON(client, khhttp.BuildBaseURL(host)+"/api/execute/contract-call", bodyBytes, idemKey, deadline) if err != nil { return err } diff --git a/cmd/execute/contract_call_test.go b/cmd/execute/contract_call_test.go index b332963..5b17ef5 100644 --- a/cmd/execute/contract_call_test.go +++ b/cmd/execute/contract_call_test.go @@ -362,3 +362,99 @@ func TestContractCallCmd_IdempotencyKeyStableAcrossHTTPRetries(t *testing.T) { } } } + +func TestContractCallCmd_IdempotencyKeyStableAcross504(t *testing.T) { + var keys []string + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := calls.Add(1) + keys = append(keys, r.Header.Get(execrecovery.IdempotencyHeader)) + if n == 1 { + w.WriteHeader(http.StatusGatewayTimeout) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-cc-504","status":"completed"}`)) + })) + defer srv.Close() + + ios, _, _, _ := iostreams.Test() + f := newContractCallFactory(ios, srv) + cmd := execute.NewContractCallCmd(f) + cmd.SetArgs([]string{"--chain", "1", "--contract", "0xcontract", "--method", "transfer", "--idempotency-key", "cc-504"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if calls.Load() < 2 { + t.Fatalf("expected HTTP retry, got %d calls", calls.Load()) + } + for i, k := range keys { + if k != "cc-504" { + t.Fatalf("call %d key=%q", i, k) + } + } +} + +func TestContractCallCmd_IdempotencyInProgressRetriesSameKey(t *testing.T) { + var keys []string + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := calls.Add(1) + keys = append(keys, r.Header.Get(execrecovery.IdempotencyHeader)) + w.Header().Set("Content-Type", "application/json") + if n == 1 { + w.WriteHeader(http.StatusConflict) + _, _ = w.Write([]byte(`{"error":"in flight","code":"idempotency_in_progress","retryable":true}`)) + return + } + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-cc-inprog","status":"completed"}`)) + })) + defer srv.Close() + + ios, _, _, _ := iostreams.Test() + f := newContractCallFactory(ios, srv) + cmd := execute.NewContractCallCmd(f) + cmd.SetArgs([]string{"--chain", "1", "--contract", "0xcontract", "--method", "transfer", "--idempotency-key", "cc-inprog", "--timeout", "10s"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if calls.Load() != 2 { + t.Fatalf("got %d POSTs, want 2", calls.Load()) + } + for i, k := range keys { + if k != "cc-inprog" { + t.Fatalf("call %d key=%q", i, k) + } + } +} + +func TestContractCallCmd_IdempotencyConflictFails(t *testing.T) { + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusConflict) + _, _ = w.Write([]byte(`{"error":"Idempotency-Key was reused with a different request payload.","code":"idempotency_conflict","retryable":false}`)) + })) + defer srv.Close() + + ios, _, _, _ := iostreams.Test() + f := newContractCallFactory(ios, srv) + cmd := execute.NewContractCallCmd(f) + cmd.SetArgs([]string{"--chain", "1", "--contract", "0xcontract", "--method", "transfer", "--idempotency-key", "cc-conflict"}) + + err := cmd.Execute() + if err == nil { + t.Fatal("expected conflict") + } + if !strings.Contains(err.Error(), "do not retry with a new key") { + t.Fatalf("got %v", err) + } + if calls.Load() != 1 { + t.Fatalf("got %d POSTs, want 1", calls.Load()) + } +} diff --git a/cmd/execute/idempotent_write.go b/cmd/execute/idempotent_write.go new file mode 100644 index 0000000..5569ce6 --- /dev/null +++ b/cmd/execute/idempotent_write.go @@ -0,0 +1,76 @@ +package execute + +import ( + "bytes" + "fmt" + "io" + "net/http" + "time" + + "github.com/keeperhub/cli/internal/execrecovery" + khhttp "github.com/keeperhub/cli/internal/http" +) + +const inProgressBackoff = 200 * time.Millisecond +const inProgressBackoffMax = 2 * time.Second + +// postIdempotentJSON POSTs body with a stable Idempotency-Key. +// +// HTTP 5xx retries are handled by the retryable client (same key). +// HTTP 409 is classified by body code from lib/idempotency.ts: +// +// idempotency_in_progress -> retry the same key until deadline +// idempotency_conflict -> fail; never mint a new key +func postIdempotentJSON(client *khhttp.Client, url string, body []byte, idemKey string, deadline time.Time) (*http.Response, error) { + backoff := inProgressBackoff + for { + req, err := client.NewRequest(http.MethodPost, url, bytes.NewReader(body)) + if err != nil { + return nil, err + } + req.Header.Set("Content-Type", "application/json") + req.Header.Set(execrecovery.IdempotencyHeader, idemKey) + + resp, err := client.Do(req) + if err != nil { + return nil, err + } + + if resp.StatusCode != http.StatusConflict { + return resp, nil + } + + raw, readErr := io.ReadAll(resp.Body) + _ = resp.Body.Close() + if readErr != nil { + return nil, fmt.Errorf("reading 409 body: %w", readErr) + } + + info, ok := execrecovery.ParseIdempotencyBody(raw) + if ok && info.IsInProgress() { + if !deadline.IsZero() && time.Now().After(deadline) { + return nil, execrecovery.InProgressTimeoutError{Key: idemKey} + } + time.Sleep(backoff) + if backoff < inProgressBackoffMax { + backoff *= 2 + if backoff > inProgressBackoffMax { + backoff = inProgressBackoffMax + } + } + continue + } + if ok && info.IsConflict() { + return nil, execrecovery.ConflictError{Body: info, Key: idemKey} + } + + msg := string(raw) + if info.Error != "" { + msg = info.Error + } + if msg == "" { + msg = http.StatusText(http.StatusConflict) + } + return nil, &khhttp.APIError{StatusCode: http.StatusConflict, Body: raw, Message: msg} + } +} diff --git a/cmd/execute/status.go b/cmd/execute/status.go index 7be83ff..532a96f 100644 --- a/cmd/execute/status.go +++ b/cmd/execute/status.go @@ -1,46 +1,23 @@ package execute import ( - "errors" "fmt" - "net/http" "time" "github.com/jedib0t/go-pretty/v6/table" + "github.com/keeperhub/cli/internal/execrecovery" khhttp "github.com/keeperhub/cli/internal/http" "github.com/keeperhub/cli/internal/output" "github.com/keeperhub/cli/pkg/cmdutil" "github.com/spf13/cobra" ) -// ExecStatusResponse represents the execution status API response. -// Shared by transfer, contract-call and status commands. -type ExecStatusResponse struct { - ExecutionID string `json:"executionId"` - Status string `json:"status"` - Type string `json:"type"` - TransactionHash *string `json:"transactionHash"` - TransactionLink *string `json:"transactionLink"` - Result any `json:"result"` - Error *string `json:"error"` - CreatedAt string `json:"createdAt"` - CompletedAt *string `json:"completedAt"` - Receipts []ExecReceipt `json:"receipts,omitempty"` -} +// ExecStatusResponse is the GET /api/execute/{id}/status wire type. +// Canonical definition: execrecovery.DirectStatus. +type ExecStatusResponse = execrecovery.DirectStatus -// ExecReceipt is a chain-re-fetched proof entry attached to an execution. -// A transactionHash alone proves a transaction was submitted; a receipt with -// verified=true and receiptStatus="success" proves it landed onchain. -// receiptStatus="reverted" is Failure even when status=completed. -type ExecReceipt struct { - Hash string `json:"hash"` - ChainID int64 `json:"chainId"` - Verified bool `json:"verified"` - ReceiptStatus string `json:"receiptStatus"` - BlockNumber *int64 `json:"blockNumber,omitempty"` - GasUsed *string `json:"gasUsed,omitempty"` - VerifiedAt *string `json:"verifiedAt,omitempty"` -} +// ExecReceipt is DirectExecutionReceiptEntry on the wire. +type ExecReceipt = execrecovery.Receipt func NewStatusCmd(f *cmdutil.Factory) *cobra.Command { cmd := &cobra.Command{ @@ -139,13 +116,6 @@ func watchExecStatus(f *cmdutil.Factory, client *khhttp.Client, host, executionI case <-ticker.C: sr, err := fetchExecStatus(client, host, executionID) if err != nil { - var apiErr *khhttp.APIError - if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound { - if isTTY && !p.IsJSON() { - fmt.Fprintf(f.IOStreams.Out, "\r%s not_found", executionID) - } - continue - } return err } @@ -164,4 +134,3 @@ func watchExecStatus(f *cmdutil.Factory, client *khhttp.Client, host, executionI } } } - diff --git a/cmd/execute/status_test.go b/cmd/execute/status_test.go index ea3487e..7c06657 100644 --- a/cmd/execute/status_test.go +++ b/cmd/execute/status_test.go @@ -19,7 +19,7 @@ func newStatusFactory(ios *iostreams.IOStreams, srv *httptest.Server) *cmdutil.F client := khhttp.NewClient(khhttp.ClientOptions{ Host: srv.URL, AppVersion: "test", - IOStreams: ios, + IOStreams: ios, }) return &cmdutil.Factory{ IOStreams: ios, @@ -205,3 +205,108 @@ func TestExecStatusCmd_Watch_PollsUntilTerminal(t *testing.T) { t.Errorf("expected tx hash in final output, got: %q", out) } } + +func TestExecStatusCmd_Watch_404Fails(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"Execution not found"}`)) + })) + defer srv.Close() + + ios, _, _, _ := iostreams.Test() + f := newStatusFactory(ios, srv) + cmd := execute.NewStatusCmd(f) + cmd.SetArgs([]string{"missing-id", "--watch"}) + + done := make(chan error, 1) + go func() { + done <- cmd.Execute() + }() + + select { + case err := <-done: + if err == nil { + t.Fatal("expected 404 to terminate --watch") + } + if !strings.Contains(err.Error(), "404") && !strings.Contains(err.Error(), "not found") { + t.Fatalf("got %v", err) + } + case <-time.After(8 * time.Second): + t.Fatal("--watch spun on 404 instead of failing") + } +} + +func TestExecStatusCmd_Watch_JSON404Fails(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"Execution not found"}`)) + })) + defer srv.Close() + + ios, buf, _, _ := iostreams.Test() + f := newStatusFactory(ios, srv) + cmd := execute.NewStatusCmd(f) + cmd.Flags().Bool("json", false, "Output as JSON") + cmd.SetArgs([]string{"foreign-org-id", "--watch", "--json"}) + + done := make(chan error, 1) + go func() { + done <- cmd.Execute() + }() + + select { + case err := <-done: + if err == nil { + t.Fatal("expected 404 to terminate --watch --json") + } + if strings.TrimSpace(buf.String()) != "" && !strings.Contains(err.Error(), "404") && !strings.Contains(err.Error(), "not found") { + t.Fatalf("err=%v out=%q", err, buf.String()) + } + case <-time.After(8 * time.Second): + t.Fatal("--watch --json spun on 404") + } +} + +func TestExecStatusCmd_Watch_FailedStatus(t *testing.T) { + callCount := 0 + errMsg := "reverted on-chain" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount++ + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + resp := execute.ExecStatusResponse{ + ExecutionID: "exec-fail-watch", + Status: "failed", + Error: &errMsg, + } + _ = json.NewEncoder(w).Encode(resp) + })) + defer srv.Close() + + ios, _, _, _ := iostreams.Test() + f := newStatusFactory(ios, srv) + cmd := execute.NewStatusCmd(f) + cmd.SetArgs([]string{"exec-fail-watch", "--watch"}) + + done := make(chan error, 1) + go func() { + done <- cmd.Execute() + }() + + select { + case err := <-done: + if err == nil { + t.Fatal("expected failed status to return error") + } + if !strings.Contains(err.Error(), "reverted on-chain") { + t.Fatalf("got %v", err) + } + case <-time.After(8 * time.Second): + t.Fatal("timed out") + } + if callCount < 1 { + t.Fatal("expected at least one poll") + } +} diff --git a/cmd/execute/transfer.go b/cmd/execute/transfer.go index 751a926..e4f6c3a 100644 --- a/cmd/execute/transfer.go +++ b/cmd/execute/transfer.go @@ -1,12 +1,10 @@ package execute import ( - "bytes" "encoding/json" "errors" "fmt" "net/http" - "strings" "time" "github.com/jedib0t/go-pretty/v6/table" @@ -87,15 +85,8 @@ func NewTransferCmd(f *cmdutil.Factory) *cobra.Command { return err } - req, err := client.NewRequest(http.MethodPost, khhttp.BuildBaseURL(host)+"/api/execute/transfer", bytes.NewReader(bodyBytes)) - if err != nil { - return err - } - req.Header.Set("Content-Type", "application/json") - // Set once before Do so go-retryablehttp retries reuse the same key (R3). - req.Header.Set(execrecovery.IdempotencyHeader, idemKey) - - resp, err := client.Do(req) + deadline := time.Now().Add(timeout) + resp, err := postIdempotentJSON(client, khhttp.BuildBaseURL(host)+"/api/execute/transfer", bodyBytes, idemKey, deadline) if err != nil { return err } @@ -176,13 +167,6 @@ func pollExecStatus(f *cmdutil.Factory, client *khhttp.Client, host, executionID return err } - if statusResp.Status == "not_found" { - if time.Now().After(deadline) { - return fmt.Errorf("timeout after %s: execution %s not found", timeout, executionID) - } - continue - } - if execTerminalStatuses[statusResp.Status] { if err := execOutcomeError(statusResp); err != nil { _ = printExecStatusResult(p, statusResp) @@ -216,10 +200,6 @@ func fetchExecStatus(client *khhttp.Client, host, executionID string) (*ExecStat } defer resp.Body.Close() - if resp.StatusCode == http.StatusNotFound { - return nil, khhttp.NewAPIError(resp) - } - if resp.StatusCode != http.StatusOK { return nil, khhttp.NewAPIError(resp) } @@ -248,7 +228,11 @@ func printExecStatusResult(p *output.Printer, sr *ExecStatusResponse) error { }) } -// execOutcomeError returns a non-nil error for failed / reverted terminal states. +// execOutcomeError returns a non-nil error for failed terminal states and for +// any receipt that is not explicitly successful when the run has completed, or +// for a conclusive on-chain failure (reverted / safe_inner_failure) at any +// status. not_found / timeout receipts on unconfirmed stay non-terminal: the +// server treats those as unread, not failed. func execOutcomeError(sr *ExecStatusResponse) error { if sr.Status == "failed" { msg := fmt.Sprintf("execution %s failed", sr.ExecutionID) @@ -258,8 +242,11 @@ func execOutcomeError(sr *ExecStatusResponse) error { return fmt.Errorf("%s", msg) } for _, r := range sr.Receipts { - if strings.EqualFold(r.ReceiptStatus, "reverted") { - return fmt.Errorf("execution %s completed but transaction reverted (%s)", sr.ExecutionID, r.Hash) + if execrecovery.ConclusiveFailedReceipt(r.ReceiptStatus) { + return fmt.Errorf("execution %s receipt %s status=%s", sr.ExecutionID, r.Hash, r.ReceiptStatus) + } + if sr.Status == "completed" && execrecovery.NonSuccessReceipt(r) { + return fmt.Errorf("execution %s completed with non-success receipt %s status=%s", sr.ExecutionID, r.Hash, r.ReceiptStatus) } } return nil diff --git a/cmd/execute/transfer_recovery_test.go b/cmd/execute/transfer_recovery_test.go index a5abbdf..b149486 100644 --- a/cmd/execute/transfer_recovery_test.go +++ b/cmd/execute/transfer_recovery_test.go @@ -83,7 +83,7 @@ func TestTransferCmd_WaitToleratesInitialNotFound(t *testing.T) { n := calls.Add(1) if n == 1 { w.WriteHeader(http.StatusNotFound) - _, _ = w.Write([]byte(`{"error":"Execution not found","code":"not_found"}`)) + _, _ = w.Write([]byte(`{"error":"Execution not found"}`)) return } w.WriteHeader(http.StatusOK) @@ -145,3 +145,204 @@ func TestTransferCmd_WaitFailsOnRevertedReceipt(t *testing.T) { t.Fatalf("expected reverted error, got %v", err) } } + +func TestTransferCmd_IdempotencyKeyStableAcross504(t *testing.T) { + var keys []string + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := calls.Add(1) + keys = append(keys, r.Header.Get(execrecovery.IdempotencyHeader)) + if n == 1 { + w.WriteHeader(http.StatusGatewayTimeout) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-504","status":"completed"}`)) + })) + defer srv.Close() + + ios, _, _, _ := iostreams.Test() + f := newTransferFactory(ios, srv) + cmd := execute.NewTransferCmd(f) + cmd.SetArgs([]string{"--chain", "1", "--to", "0xabc", "--amount", "0.1", "--idempotency-key", "retry-504"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if calls.Load() < 2 { + t.Fatalf("expected HTTP retry, got %d calls", calls.Load()) + } + for i, k := range keys { + if k != "retry-504" { + t.Fatalf("call %d Idempotency-Key=%q, want retry-504", i, k) + } + } +} + +func TestTransferCmd_IdempotencyInProgressRetriesSameKey(t *testing.T) { + var keys []string + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := calls.Add(1) + keys = append(keys, r.Header.Get(execrecovery.IdempotencyHeader)) + w.Header().Set("Content-Type", "application/json") + if n < 3 { + w.WriteHeader(http.StatusConflict) + _, _ = w.Write([]byte(`{"error":"A request with this Idempotency-Key is already being processed. Retry the same key shortly; do not rotate it.","code":"idempotency_in_progress","retryable":true}`)) + return + } + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-inprog","status":"completed"}`)) + })) + defer srv.Close() + + ios, _, _, _ := iostreams.Test() + f := newTransferFactory(ios, srv) + cmd := execute.NewTransferCmd(f) + cmd.SetArgs([]string{"--chain", "1", "--to", "0xabc", "--amount", "0.1", "--idempotency-key", "in-progress-key", "--timeout", "10s"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if calls.Load() != 3 { + t.Fatalf("got %d POSTs, want 3", calls.Load()) + } + for i, k := range keys { + if k != "in-progress-key" { + t.Fatalf("call %d minted a new key %q", i, k) + } + } +} + +func TestTransferCmd_504ThenInProgressReusesKey(t *testing.T) { + var keys []string + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := calls.Add(1) + keys = append(keys, r.Header.Get(execrecovery.IdempotencyHeader)) + w.Header().Set("Content-Type", "application/json") + switch n { + case 1: + w.WriteHeader(http.StatusGatewayTimeout) + case 2: + w.WriteHeader(http.StatusConflict) + _, _ = w.Write([]byte(`{"error":"in flight","code":"idempotency_in_progress","retryable":true}`)) + default: + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-combo","status":"completed"}`)) + } + })) + defer srv.Close() + + ios, _, _, _ := iostreams.Test() + f := newTransferFactory(ios, srv) + cmd := execute.NewTransferCmd(f) + cmd.SetArgs([]string{"--chain", "1", "--to", "0xabc", "--amount", "0.1", "--idempotency-key", "combo-key", "--timeout", "15s"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if calls.Load() < 3 { + t.Fatalf("got %d calls, want at least 3 (504 retry + in_progress + 202)", calls.Load()) + } + for i, k := range keys { + if k != "combo-key" { + t.Fatalf("call %d key=%q", i, k) + } + } +} + +func TestTransferCmd_IdempotencyConflictFailsWithoutNewKey(t *testing.T) { + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusConflict) + _, _ = w.Write([]byte(`{"error":"Idempotency-Key was reused with a different request payload. Use a new key for a different request.","code":"idempotency_conflict","originalExecutionId":"exec-orig","retryable":false}`)) + })) + defer srv.Close() + + ios, _, _, _ := iostreams.Test() + f := newTransferFactory(ios, srv) + cmd := execute.NewTransferCmd(f) + cmd.SetArgs([]string{"--chain", "1", "--to", "0xabc", "--amount", "0.1", "--idempotency-key", "conflict-key"}) + + err := cmd.Execute() + if err == nil { + t.Fatal("expected conflict error") + } + if !strings.Contains(err.Error(), "idempotency") && !strings.Contains(err.Error(), "different request payload") { + t.Fatalf("got %v", err) + } + if strings.Contains(err.Error(), "rotate") && strings.Contains(err.Error(), "new key for a different") { + // server message mentions a new key for a *different* request; we must still refuse auto-rotate + } + if !strings.Contains(err.Error(), "do not retry with a new key") { + t.Fatalf("conflict must tell the user not to mint a new key: %v", err) + } + if calls.Load() != 1 { + t.Fatalf("conflict must not retry the POST, got %d", calls.Load()) + } +} + +func TestTransferCmd_WaitPersistent404TimesOut(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.Method == http.MethodPost { + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-missing","status":"pending"}`)) + return + } + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"Execution not found"}`)) + })) + defer srv.Close() + + ios, _, _, _ := iostreams.Test() + f := newTransferFactory(ios, srv) + cmd := execute.NewTransferCmd(f) + cmd.SetArgs([]string{"--chain", "1", "--to", "0xabc", "--amount", "0.1", "--wait", "--timeout", "3s"}) + + err := cmd.Execute() + if err == nil { + t.Fatal("expected timeout") + } + if !strings.Contains(err.Error(), "not found") && !strings.Contains(err.Error(), "timeout") { + t.Fatalf("got %v", err) + } +} + +func TestTransferCmd_WaitFailsOnSafeInnerFailure(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.Method == http.MethodPost { + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-safe","status":"pending"}`)) + return + } + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(map[string]any{ + "executionId": "exec-safe", + "status": "completed", + "transactionHash": "0xsafe", + "receipts": []map[string]any{ + {"hash": "0xsafe", "verified": false, "receiptStatus": "safe_inner_failure", "verifiedAt": "2026-08-11T00:00:00Z"}, + }, + }) + })) + defer srv.Close() + + ios, _, _, _ := iostreams.Test() + f := newTransferFactory(ios, srv) + cmd := execute.NewTransferCmd(f) + cmd.SetArgs([]string{"--chain", "1", "--to", "0xabc", "--amount", "0.1", "--wait", "--timeout", "10s"}) + + err := cmd.Execute() + if err == nil { + t.Fatal("expected safe_inner_failure to fail") + } + if !strings.Contains(err.Error(), "safe_inner_failure") { + t.Fatalf("got %v", err) + } +} diff --git a/docs/execution-recovery-v1/contract.md b/docs/execution-recovery-v1/contract.md index 6d512c6..e61d039 100644 --- a/docs/execution-recovery-v1/contract.md +++ b/docs/execution-recovery-v1/contract.md @@ -1,71 +1,107 @@ # Execution recovery contract (normative) -Version: **1.1.0** +Version: **1.2.0** Audience: KeeperHub CLI / MCP / HTTP adapter authors Published path: this file is synced to docs.keeperhub.com via `docs/execution-recovery.md`. +This contract covers **direct execution**: + +- write: `POST /api/execute/transfer`, `POST /api/execute/contract-call` +- status: `GET /api/execute/{id}/status` + +It does **not** cover `POST /api/workflows//webhook`. + ## Definitions -- **Write**: any API call that may create or re-drive an onchain side effect (execute transfer, contract call, workflow webhook). -- **Status read**: polling `GET /api/execute/{id}/status` (direct execution) or workflow run status (separate vocabulary). -- **Chain evidence**: at least one receipt with `verified=true` and `receiptStatus=success` for the expected predicate. +- **Write**: `POST /api/execute/transfer` or `POST /api/execute/contract-call`. +- **Status read**: polling `GET /api/execute/{id}/status`. +- **Successful receipt**: `receipts[]` entry with `receiptStatus=success`. - **Idempotency key**: client-supplied `Idempotency-Key` header that must be byte-identical across retries of the same logical write. ## Status vocabularies (do not mix) | Surface | Pending | Terminal | | --- | --- | --- | -| Direct execution (`/api/execute/.../status`) | `pending`, `running`, `queued`, `unconfirmed`, transport `not_found` | `completed`, `failed` | -| Workflow run (`/api/workflows/executions/.../status`) | `pending`, `running` | `success`, `error`, `cancelled` | +| Direct execution (`GET /api/execute/{id}/status`) | `pending`, `running`, `unconfirmed` | `completed`, `failed` | +| Workflow run (`GET /api/workflows/executions/{id}/status`) | `pending`, `running` | `success`, `error`, `cancelled` | + +Server enum (`app/api/execute/_lib/types.ts`): `pending | running | unconfirmed | completed | failed`. There is no `queued` value. -`completed` is **not** proof of onchain success. Inspect `receipts[].receiptStatus`. +HTTP 404 on the status route is `{ "error": "Execution not found" }` with no `status` field (missing id, or an execution that belongs to another organization). It is not a status string. + +`completed` is **not** by itself proof of on-chain success. Inspect `receipts[].receiptStatus`. + +Receipt statuses (`lib/web3/verify-receipt.ts`): `success | reverted | not_found | timeout | safe_inner_failure`. +`reverted` and `safe_inner_failure` are conclusive. `not_found` and `timeout` mean the receipt could not be read; `completeExecution` settles those rows as `unconfirmed`, not `failed`. ## Rules ### R1 — Unconfirmed → poll, do not resubmit -If status is `pending`, `running`, `queued`, or `unconfirmed`, continue status reads against the **same** execution ID. Do not issue a new write for the same logical intent while that execution ID remains durable. +If status is `pending`, `running`, or `unconfirmed`, continue status reads against the **same** execution ID. Do not issue a new write for the same logical intent while that execution ID remains durable. **CLI conformance:** `kh ex transfer --wait` / `kh ex cc --wait` poll the same ID. -### R2 — Chain evidence / reverted receipts +### R2 — Receipts (client invariant) + +Never infer success from `status=completed` alone. -1. If any receipt has `receiptStatus=reverted`, the outcome is **Failure** even when `status=completed` and `verified=true`. -2. When the caller requires payment/landing proof (`RequireChainEvidence` / `--require-verified`), `completed` without a verified successful receipt is **Failure**. +1. `receiptStatus=success` is the only successful receipt. +2. `reverted` and `safe_inner_failure` are failure. +3. If a body claims `completed` plus any non-success receipt, treat it as failure. -**CLI conformance:** wait paths fail closed on reverted receipts. Strict “no receipt ⇒ fail” is opt-in (see also PR discussion for `--require-verified`). +KEEP-966 (`completeExecution`) re-verifies every claimed hash before writing `completed`. A reverted receipt is stored `verified: false` and the row settles as `failed`. `{status:"completed", verified:true, receiptStatus:"reverted"}` is **not** an observed production envelope. Fixtures that use that shape are labeled `kind: defensive` so the client stays fail-closed if the gate regresses. + +The shipped CLI does **not** set a “require chain evidence” flag and does **not** implement `--require-verified`. Completed with an empty `receipts` array is still treated as success (matches a no-hash completion). + +**CLI conformance:** wait paths fail on `status=failed` and on non-success receipts as above. ### R3 — Write retry → stable idempotency key If a write is retried after transport failure (timeout, 5xx) before an execution ID is known, the client MUST reuse the same `Idempotency-Key`. After an execution ID is known, prefer R1. -**CLI conformance:** `kh ex transfer` and `kh ex cc` (writes) set `Idempotency-Key` once per invocation before `Do`, so HTTP-layer retries reuse it. Use `--idempotency-key` to pin a key across process restarts. +The server (`lib/idempotency.ts` `idempotencyEarlyResponse`) returns HTTP 409 for two codes: + +| code | retryable | client | +| --- | --- | --- | +| `idempotency_in_progress` | true | Retry the **same** key. Do not mint a new key. | +| `idempotency_conflict` | false | Fail. The key is bound to a different payload. Do not rotate. | + +**CLI conformance:** `kh ex transfer` and `kh ex cc` set `Idempotency-Key` once per invocation. HTTP-layer 5xx retries reuse it. 409 in_progress retries it until `--timeout`. 409 conflict is a hard error. Use `--idempotency-key` to pin a key across process restarts. ### R4 — Terminal failure / malformed -Statuses `failed` (direct) and unparseable/malformed bodies are terminal for that attempt. Missing `status` after a 200 decode is **malformed**, not success. Do not invent a success path from partial fields. +Statuses `failed` (direct) and unparseable/malformed bodies are terminal for that attempt. Missing `status` after a 200 decode is **malformed**, not success. + +An unknown future `status` string is **unrecognized** (not malformed, not success) so a server addition does not look like a corrupt body. ### R5 — Rate limit HTTP `429` responses require backoff. They are not success. Preserve the idempotency key for the next write attempt of the same intent. The HTTP client does not auto-retry 429. -### R6 — Cold start `not_found` +### R6 — Cold start 404 (bounded) + +A first status read that returns HTTP 404 immediately after submit may be transient. During `--wait`, poll until `--timeout` before treating 404 as terminal. -A first status read that returns HTTP 404 / `not_found` immediately after submit may be transient. During `--wait`, poll briefly before treating not_found as terminal timeout. +`--watch` does not apply this tolerance. `kh ex st --watch` against a missing or foreign-org id must exit on 404, not loop. ## Fixture mapping + conformance -| Fixture | Rule | Expect | Consumed by | -| --- | --- | --- | --- | -| `queued.json` | R1 | pending | `TestFixtures_ClassifyTable` | -| `unconfirmed.json` | R1 | pending | `TestFixtures_ClassifyTable` | -| `completed_with_tx.json` | R2 | success (strict) | `TestFixtures_ClassifyTable` | -| `completed_without_tx.json` | R2 | failure (strict) | `TestFixtures_ClassifyTable` | -| `reverted.json` | R2 | failure | `TestFixtures_ClassifyTable` + `TestRevertedIsNeverSuccess` | -| `failed.json` | R4 | failure | `TestFixtures_ClassifyTable` | -| `malformed.json` | R4 | malformed | `TestFixtures_ClassifyTable` | -| `not_found.json` | R6 | pending | `TestFixtures_ClassifyTable` | -| `rate_limited.json` | R5 | rate_limited | `TestFixtures_ClassifyTable` | -| `cold_start.sequence.json` | R6 | 404→pending→success | `TestColdStartSequence_R6` | - -Fixtures use the **flat** direct-execution wire shape (`executionId`, not nested `execution.id`) so `json.Unmarshal` into `DirectStatus` / `ExecStatusResponse` cannot silently zero-decode. +| Fixture | Kind | Rule | Expect | Consumed by | +| --- | --- | --- | --- | --- | +| `pending.json` | observed | R1 | pending | `TestFixtures_ClassifyTable` | +| `running.json` | observed | R1 | pending | `TestFixtures_ClassifyTable` | +| `unconfirmed.json` | observed | R1 | pending | `TestFixtures_ClassifyTable` | +| `completed_with_tx.json` | observed | R2 | success | `TestFixtures_ClassifyTable` | +| `completed_without_tx.json` | classifier | R2 | failure (strict option) | `TestFixtures_ClassifyTable` | +| `reverted.json` | defensive | R2 | failure | `TestFixtures_ClassifyTable` + `TestRevertedIsNeverSuccess` | +| `safe_inner_failure.json` | defensive | R2 | failure | `TestFixtures_ClassifyTable` | +| `failed.json` | observed | R4 | failure | `TestFixtures_ClassifyTable` | +| `malformed.json` | observed | R4 | malformed | `TestFixtures_ClassifyTable` | +| `not_found.json` | observed | R6 | pending (HTTP 404) | `TestFixtures_ClassifyTable` | +| `rate_limited.json` | observed | R5 | rate_limited | `TestFixtures_ClassifyTable` | +| `cold_start.sequence.json` | observed | R6 | 404→pending→success | `TestColdStartSequence_R6` | + +Every fixture carries `"version": 1`. Loaders fail on a missing or unsupported version. `TestFixtures_ExpectedCountsAndRules` asserts the expected file count so a rename to `*.sequence.json` cannot drop coverage silently. + +Fixtures use the **flat** direct-execution wire shape (`executionId`, not nested `execution.id`). diff --git a/docs/execution-recovery.md b/docs/execution-recovery.md index 9b3ae27..99b8c60 100644 --- a/docs/execution-recovery.md +++ b/docs/execution-recovery.md @@ -1,29 +1,63 @@ # Execution recovery -Agents and adapters that submit KeeperHub writes must recover safely when the -network flakes, a status read races ahead of persistence, or an onchain -receipt reverts. +Agents and adapters that submit KeeperHub **direct-execution** writes +(`POST /api/execute/transfer`, `POST /api/execute/contract-call`) must recover +safely when the network flakes, a status read races ahead of persistence, or +an on-chain receipt is not successful. This guide is the published summary of the normative contract in [`execution-recovery-v1/contract.md`](./execution-recovery-v1/contract.md). +It does **not** describe `POST /api/workflows//webhook`. That is a +different surface. + ## Safe first-write sequence -1. Simulate when available (`"simulate": true`) and continue only if the call would not revert. +1. Simulate when available and continue only if the call would not revert. 2. Broadcast once with a stable `Idempotency-Key` that names the **work**, not the attempt. 3. Save `executionId`. 4. Poll `GET /api/execute/{executionId}/status`. -5. Treat `receipts[]` as authoritative: `verified` + `receiptStatus=success` prove landing. `receiptStatus=reverted` is failure even if `status=completed`. +5. Do not infer on-chain success from `status=completed` alone. Treat + `receipts[].receiptStatus` as the receipt evidence: only `success` is a + successful receipt. `reverted` and `safe_inner_failure` are conclusive + failures. `not_found` and `timeout` mean the receipt was unreadable; the + server settles those rows as `unconfirmed`, not `failed`. + +KEEP-966 on the server re-verifies every claimed hash before writing +`completed`. A reverted receipt is stored `verified: false` and the row +settles as `failed`. A client that still sees `completed` plus a non-success +receipt must fail closed — that combination is a defensive invariant, not an +observed production envelope. + +## Direct-execution status vocabulary + +`pending | running | unconfirmed | completed | failed` + +(`app/api/execute/_lib/types.ts`. There is no `queued` status on this endpoint.) -See also the Direct Execution API docs. +Workflow run status uses a different vocabulary (`success` / `error` / +`cancelled`) — do not mix it with direct-execution statuses. ## CLI behaviour -- `kh ex transfer` / `kh ex cc` attach `Idempotency-Key` automatically; override with `--idempotency-key`. -- `--wait` polls the same execution ID, tolerates a bounded initial `404`/`not_found`, and fails closed on reverted receipts. -- Workflow run status uses a different vocabulary (`success` / `error` / `cancelled`) — do not mix it with direct-execution statuses. +- `kh ex transfer` / `kh ex cc` attach `Idempotency-Key` on every write. + Override with `--idempotency-key` to pin a key across process restarts. +- HTTP 5xx retries reuse that key. +- HTTP 409 `idempotency_in_progress`: retry the **same** key until `--timeout`. + Do not mint a new key. +- HTTP 409 `idempotency_conflict`: fail. Do not rotate the key (that would + broadcast a second transaction). +- `--wait` polls the same execution ID and tolerates a **bounded** initial HTTP + 404 until `--timeout` (default 5m). Persistent 404 (wrong id, other org) is + a timeout error. +- `--watch` does **not** treat 404 as pending. A mistyped or foreign-org id + exits with an error instead of looping. +- Wait paths fail when `status=failed`, when a receipt is `reverted` or + `safe_inner_failure`, and when `status=completed` carries any non-success + receipt. They do not implement a `--require-verified` flag. ## Fixtures -Golden responses live under `testdata/execution_recovery_v1/` and are loaded by -`go test ./internal/execrecovery/...`. +Golden responses live under `testdata/execution_recovery_v1/` (`version: 1`) +and are loaded by `go test ./internal/execrecovery/...`. Each fixture is +labeled `observed`, `defensive`, or `classifier`. diff --git a/internal/execrecovery/classify.go b/internal/execrecovery/classify.go index d61912c..bae32cd 100644 --- a/internal/execrecovery/classify.go +++ b/internal/execrecovery/classify.go @@ -13,29 +13,41 @@ import ( type Outcome string const ( - OutcomePending Outcome = "pending" - OutcomeSuccess Outcome = "success" - OutcomeFailure Outcome = "failure" - OutcomeMalformed Outcome = "malformed" - OutcomeRateLimited Outcome = "rate_limited" + OutcomePending Outcome = "pending" + OutcomeSuccess Outcome = "success" + OutcomeFailure Outcome = "failure" + OutcomeMalformed Outcome = "malformed" + OutcomeRateLimited Outcome = "rate_limited" + OutcomeUnrecognized Outcome = "unrecognized" ) // Options controls classification strictness. type Options struct { - // RequireChainEvidence enables R2 strict mode: completed without a - // verified successful receipt is Failure, not Success. + // RequireChainEvidence is a classifier-only option used by fixtures. + // The shipped CLI wait paths do not set it. When true, completed + // without a verified successful receipt is Failure. RequireChainEvidence bool } -// Receipt is a chain-re-fetched proof entry (direct-execution status API). +// Receipt is GET /api/execute/{id}/status receipts[] +// (DirectExecutionReceiptEntry in KeeperHub/keeperhub). +// +// receiptStatus values from lib/web3/verify-receipt.ts: +// success | reverted | not_found | timeout | safe_inner_failure. +// verifiedAt is required on the server type. chainId is optional. type Receipt struct { - Hash string `json:"hash"` - ChainID int64 `json:"chainId"` - Verified bool `json:"verified"` - ReceiptStatus string `json:"receiptStatus"` + Hash string `json:"hash"` + ChainID *int `json:"chainId,omitempty"` + Network string `json:"network,omitempty"` + Verified bool `json:"verified"` + ReceiptStatus string `json:"receiptStatus"` + BlockNumber *int64 `json:"blockNumber,omitempty"` + GasUsed *string `json:"gasUsed,omitempty"` + VerifiedAt string `json:"verifiedAt"` } // DirectStatus is the flat wire shape of GET /api/execute/{id}/status. +// Canonical type: cmd/execute aliases this as ExecStatusResponse. type DirectStatus struct { ExecutionID string `json:"executionId"` Status string `json:"status"` @@ -57,22 +69,25 @@ type Sample struct { // Classify maps one status observation to an Outcome. // -// Vocabulary note: direct-execution statuses are pending|running|completed|failed -// (and transport-level not_found). Workflow run statuses (success|error|cancelled) +// Direct-execution statuses are pending|running|unconfirmed|completed|failed +// (app/api/execute/_lib/types.ts). Workflow run statuses (success|error|cancelled) // belong to a different API and must not be fed here — see Vocabulary(). +// +// An unknown future status is OutcomeUnrecognized (never success, never +// malformed) so a server addition does not look like a corrupt body. func Classify(sample Sample, opts Options) (Outcome, string) { if sample.HTTPStatus == http.StatusTooManyRequests { return OutcomeRateLimited, "HTTP 429" } // Cold-start / missing: callers may poll again (R6). Terminal failure is a - // poll-budget decision, not Classify's. + // poll-budget decision, not Classify's. The status endpoint answers 404 + // with {"error":"Execution not found"} and no status field. if sample.HTTPStatus == http.StatusNotFound { - return OutcomePending, "not_found" + return OutcomePending, "http 404" } if sample.HTTPStatus != 0 && sample.HTTPStatus != http.StatusOK && sample.HTTPStatus != http.StatusAccepted { - // Non-404 errors are failures for a status read. if sample.HTTPStatus >= 400 { return OutcomeFailure, fmt.Sprintf("HTTP %d", sample.HTTPStatus) } @@ -94,27 +109,24 @@ func Classify(sample Sample, opts Options) (Outcome, string) { status := strings.ToLower(strings.TrimSpace(st.Status)) if status == "" { - // Unrecognised schema: valid JSON but no status field. return OutcomeMalformed, "missing status field" } switch status { - case "pending", "running", "queued", "unconfirmed": + case "pending", "running", "unconfirmed": return OutcomePending, status - case "not_found": - return OutcomePending, status - case "failed", "error", "cancelled": + case "failed": return OutcomeFailure, status - case "completed", "success": + case "completed": return classifyCompleted(st, opts) default: - return OutcomeMalformed, "unrecognised status: " + status + return OutcomeUnrecognized, "unrecognized status: " + status } } func classifyCompleted(st DirectStatus, opts Options) (Outcome, string) { - if hasRevertedReceipt(st.Receipts) { - return OutcomeFailure, "receiptStatus=reverted" + if r := FirstBlockingReceipt(st.Receipts); r != nil { + return OutcomeFailure, "receiptStatus=" + r.ReceiptStatus } if hasVerifiedSuccess(st.Receipts) { @@ -131,18 +143,36 @@ func classifyCompleted(st DirectStatus, opts Options) (Outcome, string) { return OutcomeFailure, "no verified successful receipt" } - // Compatible default: completed without receipts is still Success for - // callers that have not opted into R2 strict mode (see --require-verified). + // Compatible default: completed without receipts is still Success. + // The shipped CLI does not set RequireChainEvidence. return OutcomeSuccess, "completed" } -func hasRevertedReceipt(receipts []Receipt) bool { - for _, r := range receipts { - if strings.EqualFold(r.ReceiptStatus, "reverted") { - return true +// ConclusiveFailedReceipt reports a chain-answered failure +// (lib/web3/verify-receipt.ts CONCLUSIVE_STATUSES minus success). +func ConclusiveFailedReceipt(status string) bool { + switch strings.ToLower(strings.TrimSpace(status)) { + case "reverted", "safe_inner_failure": + return true + default: + return false + } +} + +// NonSuccessReceipt is true when a receipt exists and is not explicitly successful. +func NonSuccessReceipt(r Receipt) bool { + s := strings.ToLower(strings.TrimSpace(r.ReceiptStatus)) + return s != "" && s != "success" +} + +// FirstBlockingReceipt returns the first receipt that must not be treated as success. +func FirstBlockingReceipt(receipts []Receipt) *Receipt { + for i := range receipts { + if NonSuccessReceipt(receipts[i]) { + return &receipts[i] } } - return false + return nil } func hasVerifiedSuccess(receipts []Receipt) bool { @@ -153,8 +183,3 @@ func hasVerifiedSuccess(receipts []Receipt) bool { } return false } - -// HasRevertedReceipt reports whether any receipt is an onchain revert. -func HasRevertedReceipt(receipts []Receipt) bool { - return hasRevertedReceipt(receipts) -} diff --git a/internal/execrecovery/classify_receipt_test.go b/internal/execrecovery/classify_receipt_test.go new file mode 100644 index 0000000..c4c650b --- /dev/null +++ b/internal/execrecovery/classify_receipt_test.go @@ -0,0 +1,48 @@ +package execrecovery_test + +import ( + "testing" + + "github.com/keeperhub/cli/internal/execrecovery" +) + +func TestClassify_ReceiptStates(t *testing.T) { + completed := func(receiptStatus string, verified bool) []byte { + return []byte(`{"executionId":"x","status":"completed","transactionHash":"0xabc","receipts":[{"hash":"0xabc","verified":` + boolJSON(verified) + `,"receiptStatus":"` + receiptStatus + `","verifiedAt":"2026-08-11T00:00:00Z"}]}`) + } + unconfirmed := func(receiptStatus string) []byte { + return []byte(`{"executionId":"x","status":"unconfirmed","transactionHash":"0xabc","receipts":[{"hash":"0xabc","verified":false,"receiptStatus":"` + receiptStatus + `","verifiedAt":"2026-08-11T00:00:00Z"}]}`) + } + + t.Run("completed success", func(t *testing.T) { + got, _ := execrecovery.Classify(execrecovery.Sample{HTTPStatus: 200, Body: completed("success", true)}, execrecovery.Options{}) + if got != execrecovery.OutcomeSuccess { + t.Fatalf("got %s, want success", got) + } + }) + for _, st := range []string{"reverted", "safe_inner_failure", "not_found", "timeout"} { + st := st + t.Run("completed "+st+" is failure", func(t *testing.T) { + got, reason := execrecovery.Classify(execrecovery.Sample{HTTPStatus: 200, Body: completed(st, false)}, execrecovery.Options{}) + if got != execrecovery.OutcomeFailure { + t.Fatalf("got %s (%s), want failure", got, reason) + } + }) + } + for _, st := range []string{"not_found", "timeout"} { + st := st + t.Run("unconfirmed "+st+" is pending", func(t *testing.T) { + got, reason := execrecovery.Classify(execrecovery.Sample{HTTPStatus: 200, Body: unconfirmed(st)}, execrecovery.Options{}) + if got != execrecovery.OutcomePending { + t.Fatalf("got %s (%s), want pending", got, reason) + } + }) + } +} + +func boolJSON(v bool) string { + if v { + return "true" + } + return "false" +} diff --git a/internal/execrecovery/fixture.go b/internal/execrecovery/fixture.go index ba4ba14..7db80b9 100644 --- a/internal/execrecovery/fixture.go +++ b/internal/execrecovery/fixture.go @@ -5,11 +5,29 @@ import ( "fmt" "os" "path/filepath" + "strings" +) + +const FixtureVersion = 1 + +// Kind labels how a fixture relates to production. +// +// observed: a response the current handler can emit. +// defensive: a client-side invariant against a body KEEP-966 makes unreachable. +// classifier: exercises a classifier option the shipped CLI does not set. +type Kind string + +const ( + KindObserved Kind = "observed" + KindDefensive Kind = "defensive" + KindClassifier Kind = "classifier" ) // Fixture is one conformance case loaded from testdata/execution_recovery_v1. type Fixture struct { Name string `json:"-"` + Version int `json:"version"` + Kind Kind `json:"kind"` Rule string `json:"rule"` HTTPStatus int `json:"httpStatus"` RequireChainEvidence bool `json:"requireChainEvidence"` @@ -30,12 +48,32 @@ type SequenceStep struct { // SequenceFixture exercises multi-poll recovery (R6). type SequenceFixture struct { - Name string `json:"name"` - Rule string `json:"rule"` - Steps []SequenceStep `json:"steps"` + Name string `json:"name"` + Version int `json:"version"` + Kind Kind `json:"kind"` + Rule string `json:"rule"` + Steps []SequenceStep `json:"steps"` } -// LoadFixtureDir loads every *.json fixture (not *.sequence.json) from dir. +func validateMeta(name string, version int, kind Kind) error { + if version != FixtureVersion { + return fmt.Errorf("%s: unsupported fixture version %d (want %d)", name, version, FixtureVersion) + } + switch kind { + case KindObserved, KindDefensive, KindClassifier: + return nil + case "": + return fmt.Errorf("%s: missing kind (observed|defensive|classifier)", name) + default: + return fmt.Errorf("%s: unknown kind %q", name, kind) + } +} + +func isSequenceName(name string) bool { + return strings.HasSuffix(name, ".sequence.json") +} + +// LoadFixtureDir loads every versioned *.json fixture that is not a sequence. func LoadFixtureDir(dir string) ([]Fixture, error) { entries, err := os.ReadDir(dir) if err != nil { @@ -47,10 +85,7 @@ func LoadFixtureDir(dir string) ([]Fixture, error) { continue } name := e.Name() - if filepath.Ext(name) != ".json" { - continue - } - if len(name) >= len(".sequence.json") && name[len(name)-len(".sequence.json"):] == ".sequence.json" { + if filepath.Ext(name) != ".json" || isSequenceName(name) { continue } path := filepath.Join(dir, name) @@ -62,12 +97,38 @@ func LoadFixtureDir(dir string) ([]Fixture, error) { if err := json.Unmarshal(raw, &f); err != nil { return nil, fmt.Errorf("%s: %w", name, err) } + if err := validateMeta(name, f.Version, f.Kind); err != nil { + return nil, err + } + if f.Rule == "" { + return nil, fmt.Errorf("%s: missing rule", name) + } f.Name = name out = append(out, f) } return out, nil } +// LoadSequenceDir loads every *.sequence.json fixture in dir. +func LoadSequenceDir(dir string) ([]SequenceFixture, error) { + entries, err := os.ReadDir(dir) + if err != nil { + return nil, err + } + var out []SequenceFixture + for _, e := range entries { + if e.IsDir() || !isSequenceName(e.Name()) { + continue + } + seq, err := LoadSequence(filepath.Join(dir, e.Name())) + if err != nil { + return nil, err + } + out = append(out, seq) + } + return out, nil +} + // LoadSequence loads a multi-step sequence fixture. func LoadSequence(path string) (SequenceFixture, error) { raw, err := os.ReadFile(path) @@ -78,6 +139,19 @@ func LoadSequence(path string) (SequenceFixture, error) { if err := json.Unmarshal(raw, &seq); err != nil { return SequenceFixture{}, err } + name := filepath.Base(path) + if seq.Name == "" { + seq.Name = name + } + if err := validateMeta(name, seq.Version, seq.Kind); err != nil { + return SequenceFixture{}, err + } + if seq.Rule == "" { + return SequenceFixture{}, fmt.Errorf("%s: missing rule", name) + } + if len(seq.Steps) == 0 { + return SequenceFixture{}, fmt.Errorf("%s: sequence has no steps", name) + } return seq, nil } diff --git a/internal/execrecovery/fixture_test.go b/internal/execrecovery/fixture_test.go index 293cc96..03e6da1 100644 --- a/internal/execrecovery/fixture_test.go +++ b/internal/execrecovery/fixture_test.go @@ -1,6 +1,7 @@ package execrecovery_test import ( + "os" "path/filepath" "runtime" "testing" @@ -8,31 +9,99 @@ import ( "github.com/keeperhub/cli/internal/execrecovery" ) +const ( + wantFixtures = 11 + wantSequences = 1 +) + func testdataDir(t *testing.T) string { t.Helper() _, file, _, ok := runtime.Caller(0) if !ok { t.Fatal("runtime.Caller failed") } - // internal/execrecovery -> repo root root := filepath.Clean(filepath.Join(filepath.Dir(file), "..", "..")) return filepath.Join(root, "testdata", "execution_recovery_v1") } +func TestLoadFixtureDir_EmptyFails(t *testing.T) { + dir := t.TempDir() + fixtures, err := execrecovery.LoadFixtureDir(dir) + if err != nil { + t.Fatalf("empty dir should load, got err %v", err) + } + if len(fixtures) != 0 { + t.Fatalf("got %d fixtures, want 0", len(fixtures)) + } +} + +func TestFixtures_ExpectedCountsAndRules(t *testing.T) { + dir := testdataDir(t) + fixtures, err := execrecovery.LoadFixtureDir(dir) + if err != nil { + t.Fatalf("LoadFixtureDir: %v", err) + } + if len(fixtures) != wantFixtures { + t.Fatalf("loaded %d fixtures, want %d (renaming a file to *.sequence.json must not silently drop coverage)", len(fixtures), wantFixtures) + } + seqs, err := execrecovery.LoadSequenceDir(dir) + if err != nil { + t.Fatalf("LoadSequenceDir: %v", err) + } + if len(seqs) != wantSequences { + t.Fatalf("loaded %d sequences, want %d", len(seqs), wantSequences) + } + + rules := map[string]int{} + kinds := map[execrecovery.Kind]int{} + for _, f := range fixtures { + if f.Version != execrecovery.FixtureVersion { + t.Fatalf("%s: version %d", f.Name, f.Version) + } + if f.Rule == "" { + t.Fatalf("%s: missing rule", f.Name) + } + rules[f.Rule]++ + kinds[f.Kind]++ + } + for _, need := range []string{"R1", "R2", "R4", "R5", "R6"} { + if rules[need] == 0 { + t.Fatalf("no fixture maps to rule %s", need) + } + } + if kinds[execrecovery.KindObserved] == 0 { + t.Fatal("expected at least one observed fixture") + } + if kinds[execrecovery.KindDefensive] == 0 { + t.Fatal("expected at least one defensive fixture") + } +} + +func TestLoadFixtureDir_RejectsMissingVersion(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "no_version.json") + body := []byte(`{"kind":"observed","rule":"R1","httpStatus":200,"expect":"pending","response":{"status":"pending"}}`) + if err := os.WriteFile(path, body, 0o644); err != nil { + t.Fatal(err) + } + if _, err := execrecovery.LoadFixtureDir(dir); err == nil { + t.Fatal("expected version error") + } +} + func TestFixtures_DecodeIntoDirectStatus(t *testing.T) { fixtures, err := execrecovery.LoadFixtureDir(testdataDir(t)) if err != nil { t.Fatalf("LoadFixtureDir: %v", err) } - if len(fixtures) == 0 { - t.Fatal("no fixtures loaded") + if len(fixtures) != wantFixtures { + t.Fatalf("loaded %d fixtures, want %d", len(fixtures), wantFixtures) } for _, f := range fixtures { f := f t.Run(f.Name, func(t *testing.T) { if f.ResponseRaw != "" { - // Malformed raw bodies are not DirectStatus JSON. st, err := f.DecodeResponse() if err == nil { t.Fatalf("expected decode error for raw fixture, got %#v", st) @@ -41,18 +110,13 @@ func TestFixtures_DecodeIntoDirectStatus(t *testing.T) { } st, err := f.DecodeResponse() if err != nil { - // not_found / rate_limited error bodies are not DirectStatus; - // Classify still handles them via HTTP status. if f.HTTPStatus == 404 || f.HTTPStatus == 429 { return } t.Fatalf("DecodeResponse: %v", err) } if f.HTTPStatus == 200 && f.Expect != execrecovery.OutcomeMalformed { - if st.ExecutionID == "" && f.Expect != execrecovery.OutcomeFailure { - // failed fixture has executionId; ensure we never silently zero-decode. - } - if st.Status == "" && f.Expect != execrecovery.OutcomeMalformed { + if st.Status == "" { t.Fatalf("decoded empty Status for fixture %s — wire shape mismatch", f.Name) } } @@ -65,6 +129,9 @@ func TestFixtures_ClassifyTable(t *testing.T) { if err != nil { t.Fatalf("LoadFixtureDir: %v", err) } + if len(fixtures) != wantFixtures { + t.Fatalf("loaded %d fixtures, want %d", len(fixtures), wantFixtures) + } for _, f := range fixtures { f := f @@ -80,11 +147,14 @@ func TestFixtures_ClassifyTable(t *testing.T) { } func TestColdStartSequence_R6(t *testing.T) { - path := filepath.Join(testdataDir(t), "cold_start.sequence.json") - seq, err := execrecovery.LoadSequence(path) + seqs, err := execrecovery.LoadSequenceDir(testdataDir(t)) if err != nil { - t.Fatalf("LoadSequence: %v", err) + t.Fatalf("LoadSequenceDir: %v", err) + } + if len(seqs) != wantSequences { + t.Fatalf("sequences=%d, want %d", len(seqs), wantSequences) } + seq := seqs[0] if seq.Rule != "R6" { t.Fatalf("rule=%s, want R6", seq.Rule) } @@ -106,11 +176,9 @@ func TestRevertedIsNeverSuccess(t *testing.T) { "executionId":"x", "status":"completed", "transactionHash":"0xabc", - "receipts":[{"hash":"0xabc","chainId":8453,"verified":true,"receiptStatus":"reverted"}] + "receipts":[{"hash":"0xabc","verified":true,"receiptStatus":"reverted","verifiedAt":"2026-08-11T00:00:00Z"}] }`) - got, reason := execrecovery.Classify(execrecovery.Sample{HTTPStatus: 200, Body: body}, execrecovery.Options{ - RequireChainEvidence: true, - }) + got, reason := execrecovery.Classify(execrecovery.Sample{HTTPStatus: 200, Body: body}, execrecovery.Options{}) if got != execrecovery.OutcomeFailure { t.Fatalf("got %s (%s), want failure", got, reason) } @@ -126,17 +194,49 @@ func TestEmptyStatusIsMalformed(t *testing.T) { } } +func TestUnknownStatusIsUnrecognizedNotMalformed(t *testing.T) { + got, reason := execrecovery.Classify(execrecovery.Sample{ + HTTPStatus: 200, + Body: []byte(`{"executionId":"x","status":"settling"}`), + }, execrecovery.Options{}) + if got != execrecovery.OutcomeUnrecognized { + t.Fatalf("got %s (%s), want unrecognized", got, reason) + } +} + +func TestWorkflowStatusesAreNotDirectSuccess(t *testing.T) { + for _, status := range []string{"success", "error", "cancelled", "queued"} { + got, _ := execrecovery.Classify(execrecovery.Sample{ + HTTPStatus: 200, + Body: []byte(`{"executionId":"x","status":"` + status + `"}`), + }, execrecovery.Options{}) + if got == execrecovery.OutcomeSuccess { + t.Fatalf("status %s must not classify as success", status) + } + if got == execrecovery.OutcomeMalformed { + t.Fatalf("status %s must not classify as malformed (would break a future enum addition)", status) + } + } +} + func TestVocabularySurfacesAreDistinct(t *testing.T) { d := execrecovery.DirectExecutionVocabulary() w := execrecovery.WorkflowRunVocabulary() if d.Surface == w.Surface { t.Fatal("vocabularies must name distinct surfaces") } + direct := map[string]struct{}{} + for _, s := range append(append([]string{}, d.Pending...), d.Terminal...) { + direct[s] = struct{}{} + } for _, term := range w.Terminal { - for _, dTerm := range d.Terminal { - if term == dTerm { - t.Fatalf("shared terminal term %q across surfaces — keep vocabularies separate", term) - } + if _, ok := direct[term]; ok { + t.Fatalf("workflow terminal %q must not appear in direct-execution vocabulary", term) + } + } + for _, s := range d.Pending { + if s == "queued" || s == "not_found" { + t.Fatalf("direct pending must not include %q (not in server ExecutionStatus)", s) } } } diff --git a/internal/execrecovery/idempotency.go b/internal/execrecovery/idempotency.go index 1246fd0..39da8ae 100644 --- a/internal/execrecovery/idempotency.go +++ b/internal/execrecovery/idempotency.go @@ -3,11 +3,97 @@ package execrecovery import ( "crypto/rand" "encoding/hex" + "encoding/json" "fmt" + "strings" ) const IdempotencyHeader = "Idempotency-Key" +// Codes from KeeperHub/keeperhub lib/idempotency.ts idempotencyEarlyResponse. +// Both map to HTTP 409; only `code` distinguishes them. +const ( + CodeIdempotencyConflict = "idempotency_conflict" + CodeIdempotencyInProgress = "idempotency_in_progress" +) + +// IdempotencyBody is the 409 JSON from idempotencyEarlyResponse. +type IdempotencyBody struct { + Error string `json:"error"` + Code string `json:"code"` + Retryable *bool `json:"retryable"` + OriginalExecutionID *string `json:"originalExecutionId"` +} + +// ParseIdempotencyBody decodes a 409 body. ok is true when `code` is one of +// the two idempotency codes, or when retryable is present as a fallback. +func ParseIdempotencyBody(body []byte) (IdempotencyBody, bool) { + var b IdempotencyBody + if err := json.Unmarshal(body, &b); err != nil { + return IdempotencyBody{}, false + } + code := strings.ToLower(strings.TrimSpace(b.Code)) + b.Code = code + switch code { + case CodeIdempotencyConflict, CodeIdempotencyInProgress: + return b, true + } + if b.Retryable != nil { + return b, true + } + return b, false +} + +// IsInProgress is true when the same key is already processing. +// The client must retry that key and must not mint a new one. +func (b IdempotencyBody) IsInProgress() bool { + if b.Code == CodeIdempotencyInProgress { + return true + } + return b.Code == "" && b.Retryable != nil && *b.Retryable +} + +// IsConflict is true when the key is bound to a different payload. +// The client must fail closed and must not rotate the key. +func (b IdempotencyBody) IsConflict() bool { + if b.Code == CodeIdempotencyConflict { + return true + } + return b.Code == "" && b.Retryable != nil && !*b.Retryable +} + +// ConflictError is a 409 idempotency_conflict. +type ConflictError struct { + Body IdempotencyBody + Key string +} + +func (e ConflictError) Error() string { + msg := e.Body.Error + if msg == "" { + msg = "Idempotency-Key was reused with a different request payload" + } + if e.Body.OriginalExecutionID != nil && *e.Body.OriginalExecutionID != "" { + msg = fmt.Sprintf("%s (originalExecutionId=%s)", msg, *e.Body.OriginalExecutionID) + } + if e.Key != "" { + msg = fmt.Sprintf("%s; do not retry with a new key for the same intent (Idempotency-Key %s)", msg, e.Key) + } else { + msg = msg + "; do not retry with a new Idempotency-Key for the same intent" + } + return msg +} + +// InProgressTimeoutError is returned when 409 idempotency_in_progress +// outlives the wait budget. The same key must be reused on the next attempt. +type InProgressTimeoutError struct { + Key string +} + +func (e InProgressTimeoutError) Error() string { + return fmt.Sprintf("Idempotency-Key %s is still in progress; retry with --idempotency-key %s (do not rotate the key)", e.Key, e.Key) +} + // NewIdempotencyKey returns a random UUID-like key for a single write intent. // Callers that retry the same intent across process restarts must persist or // derive a stable key instead (see docs.keeperhub.com/api/direct-execution). @@ -16,7 +102,6 @@ func NewIdempotencyKey() (string, error) { if _, err := rand.Read(b[:]); err != nil { return "", fmt.Errorf("generating idempotency key: %w", err) } - // UUID v4 layout bits are not required by the API; hex is sufficient. return hex.EncodeToString(b[:]), nil } diff --git a/internal/execrecovery/idempotency_test.go b/internal/execrecovery/idempotency_test.go index 57a62dc..26c2263 100644 --- a/internal/execrecovery/idempotency_test.go +++ b/internal/execrecovery/idempotency_test.go @@ -1,6 +1,7 @@ package execrecovery_test import ( + "strings" "testing" "github.com/keeperhub/cli/internal/execrecovery" @@ -36,3 +37,38 @@ func TestResolveIdempotencyKey_GeneratedUnique(t *testing.T) { t.Fatal("different intents must get different generated keys") } } + +func TestParseIdempotencyBody_InProgress(t *testing.T) { + b, ok := execrecovery.ParseIdempotencyBody([]byte(`{"error":"A request with this Idempotency-Key is already being processed. Retry the same key shortly; do not rotate it.","code":"idempotency_in_progress","retryable":true}`)) + if !ok { + t.Fatal("expected parse ok") + } + if !b.IsInProgress() || b.IsConflict() { + t.Fatalf("in_progress misclassified: %+v", b) + } +} + +func TestParseIdempotencyBody_Conflict(t *testing.T) { + orig := "exec-original" + b, ok := execrecovery.ParseIdempotencyBody([]byte(`{"error":"Idempotency-Key was reused with a different request payload. Use a new key for a different request.","code":"idempotency_conflict","originalExecutionId":"exec-original","retryable":false}`)) + if !ok { + t.Fatal("expected parse ok") + } + if !b.IsConflict() || b.IsInProgress() { + t.Fatalf("conflict misclassified: %+v", b) + } + if b.OriginalExecutionID == nil || *b.OriginalExecutionID != orig { + t.Fatalf("originalExecutionId=%v", b.OriginalExecutionID) + } + err := execrecovery.ConflictError{Body: b, Key: "k1"} + if !strings.Contains(err.Error(), "exec-original") || !strings.Contains(err.Error(), "do not retry with a new key") { + t.Fatalf("conflict error=%s", err.Error()) + } +} + +func TestParseIdempotencyBody_Unknown409(t *testing.T) { + _, ok := execrecovery.ParseIdempotencyBody([]byte(`{"error":"something else"}`)) + if ok { + t.Fatal("generic 409 must not look like an idempotency code") + } +} diff --git a/internal/execrecovery/vocabulary.go b/internal/execrecovery/vocabulary.go index 5fc01b1..73932c8 100644 --- a/internal/execrecovery/vocabulary.go +++ b/internal/execrecovery/vocabulary.go @@ -8,11 +8,12 @@ type Vocabulary struct { Terminal []string } -// DirectExecutionVocabulary is GET /api/execute/{id}/status. +// DirectExecutionVocabulary is GET /api/execute/{id}/status +// (app/api/execute/_lib/types.ts ExecutionStatus). func DirectExecutionVocabulary() Vocabulary { return Vocabulary{ Surface: "direct-execution", - Pending: []string{"pending", "running", "queued", "unconfirmed", "not_found"}, + Pending: []string{"pending", "running", "unconfirmed"}, Terminal: []string{"completed", "failed"}, } } diff --git a/testdata/execution_recovery_v1/cold_start.sequence.json b/testdata/execution_recovery_v1/cold_start.sequence.json index 942e674..9b4c224 100644 --- a/testdata/execution_recovery_v1/cold_start.sequence.json +++ b/testdata/execution_recovery_v1/cold_start.sequence.json @@ -1,13 +1,14 @@ { "name": "cold_start", + "version": 1, + "kind": "observed", "rule": "R6", "steps": [ { "httpStatus": 404, "expect": "pending", "response": { - "error": "Execution not found", - "code": "not_found" + "error": "Execution not found" } }, { @@ -15,12 +16,12 @@ "expect": "pending", "response": { "executionId": "exec_fixture_cold_001", - "status": "pending" + "status": "pending", + "createdAt": "2026-08-11T00:00:00.000Z" } }, { "httpStatus": 200, - "requireChainEvidence": true, "expect": "success", "response": { "executionId": "exec_fixture_cold_001", @@ -31,7 +32,8 @@ "hash": "0x4444444444444444444444444444444444444444444444444444444444444444", "chainId": 8453, "verified": true, - "receiptStatus": "success" + "receiptStatus": "success", + "verifiedAt": "2026-08-11T00:01:00.000Z" } ] } diff --git a/testdata/execution_recovery_v1/completed_with_tx.json b/testdata/execution_recovery_v1/completed_with_tx.json index a653a97..7b9f46e 100644 --- a/testdata/execution_recovery_v1/completed_with_tx.json +++ b/testdata/execution_recovery_v1/completed_with_tx.json @@ -1,4 +1,6 @@ { + "version": 1, + "kind": "observed", "rule": "R2", "httpStatus": 200, "requireChainEvidence": true, @@ -13,7 +15,8 @@ "hash": "0x1111111111111111111111111111111111111111111111111111111111111111", "chainId": 8453, "verified": true, - "receiptStatus": "success" + "receiptStatus": "success", + "verifiedAt": "2026-08-11T00:01:00.000Z" } ], "completedAt": "2026-08-11T00:01:00.000Z" diff --git a/testdata/execution_recovery_v1/completed_without_tx.json b/testdata/execution_recovery_v1/completed_without_tx.json index 21e28db..f5ac3f6 100644 --- a/testdata/execution_recovery_v1/completed_without_tx.json +++ b/testdata/execution_recovery_v1/completed_without_tx.json @@ -1,4 +1,6 @@ { + "version": 1, + "kind": "classifier", "rule": "R2", "httpStatus": 200, "requireChainEvidence": true, @@ -8,5 +10,5 @@ "status": "completed", "completedAt": "2026-08-11T00:01:00.000Z" }, - "note": "completed without chain evidence must fail closed when proof is required." + "note": "Classifier-only: RequireChainEvidence is not set by the shipped CLI. Production wait paths still treat completed with no receipts as success." } diff --git a/testdata/execution_recovery_v1/failed.json b/testdata/execution_recovery_v1/failed.json index e0657ec..7400817 100644 --- a/testdata/execution_recovery_v1/failed.json +++ b/testdata/execution_recovery_v1/failed.json @@ -1,4 +1,6 @@ { + "version": 1, + "kind": "observed", "rule": "R4", "httpStatus": 200, "requireChainEvidence": false, diff --git a/testdata/execution_recovery_v1/malformed.json b/testdata/execution_recovery_v1/malformed.json index a905de8..01ebc6e 100644 --- a/testdata/execution_recovery_v1/malformed.json +++ b/testdata/execution_recovery_v1/malformed.json @@ -1,8 +1,10 @@ { + "version": 1, + "kind": "observed", "rule": "R4", "httpStatus": 200, "requireChainEvidence": false, "expect": "malformed", "responseRaw": "{{{this is not json", - "note": "Genuine unparseable body — must not be treated as success or empty-status poll forever." + "note": "Genuine unparseable body — must not be treated as success." } diff --git a/testdata/execution_recovery_v1/not_found.json b/testdata/execution_recovery_v1/not_found.json index 3dfabc2..0a65a22 100644 --- a/testdata/execution_recovery_v1/not_found.json +++ b/testdata/execution_recovery_v1/not_found.json @@ -1,11 +1,12 @@ { - "rule": "R4", + "version": 1, + "kind": "observed", + "rule": "R6", "httpStatus": 404, "requireChainEvidence": false, "expect": "pending", "response": { - "error": "Execution not found", - "code": "not_found" + "error": "Execution not found" }, - "note": "A single not_found is Pending (R6). Exhausted poll budget is a caller-level Failure." + "note": "Observed GET /api/execute/{id}/status 404 body. Classify treats a single 404 as pending (cold-start). Exhausted --wait budget is a caller-level failure. --watch treats 404 as a terminal error (mistyped or foreign-org id)." } diff --git a/testdata/execution_recovery_v1/pending.json b/testdata/execution_recovery_v1/pending.json new file mode 100644 index 0000000..29cd69a --- /dev/null +++ b/testdata/execution_recovery_v1/pending.json @@ -0,0 +1,14 @@ +{ + "version": 1, + "kind": "observed", + "rule": "R1", + "httpStatus": 200, + "requireChainEvidence": false, + "expect": "pending", + "response": { + "executionId": "exec_fixture_pending_001", + "status": "pending", + "createdAt": "2026-08-11T00:00:00.000Z" + }, + "note": "Direct-execution enum: pending | running | unconfirmed | completed | failed." +} diff --git a/testdata/execution_recovery_v1/rate_limited.json b/testdata/execution_recovery_v1/rate_limited.json index 41f9e32..da9c4d5 100644 --- a/testdata/execution_recovery_v1/rate_limited.json +++ b/testdata/execution_recovery_v1/rate_limited.json @@ -1,10 +1,11 @@ { + "version": 1, + "kind": "observed", "rule": "R5", "httpStatus": 429, "requireChainEvidence": false, "expect": "rate_limited", "response": { - "error": "Rate limit exceeded", - "code": "rate_limited" + "error": "Rate limit exceeded" } } diff --git a/testdata/execution_recovery_v1/reverted.json b/testdata/execution_recovery_v1/reverted.json index 0122430..51f926e 100644 --- a/testdata/execution_recovery_v1/reverted.json +++ b/testdata/execution_recovery_v1/reverted.json @@ -1,7 +1,9 @@ { + "version": 1, + "kind": "defensive", "rule": "R2", "httpStatus": 200, - "requireChainEvidence": true, + "requireChainEvidence": false, "expect": "failure", "response": { "executionId": "exec_fixture_reverted_001", @@ -12,10 +14,11 @@ "hash": "0x3333333333333333333333333333333333333333333333333333333333333333", "chainId": 8453, "verified": true, - "receiptStatus": "reverted" + "receiptStatus": "reverted", + "verifiedAt": "2026-08-11T00:01:00.000Z" } ], "completedAt": "2026-08-11T00:01:00.000Z" }, - "note": "verified=true does not mean successful; receiptStatus=reverted is Failure." + "note": "DEFENSIVE, not an observed production envelope. KEEP-966 re-verifies every claimed hash before writing completed; a reverted receipt is verified:false and the row settles as failed. This fixture asserts the client invariant: never infer success from status=completed alone." } diff --git a/testdata/execution_recovery_v1/queued.json b/testdata/execution_recovery_v1/running.json similarity index 59% rename from testdata/execution_recovery_v1/queued.json rename to testdata/execution_recovery_v1/running.json index eda3e25..19cd389 100644 --- a/testdata/execution_recovery_v1/queued.json +++ b/testdata/execution_recovery_v1/running.json @@ -1,11 +1,13 @@ { + "version": 1, + "kind": "observed", "rule": "R1", "httpStatus": 200, "requireChainEvidence": false, "expect": "pending", "response": { - "executionId": "exec_fixture_queued_001", - "status": "pending", + "executionId": "exec_fixture_running_001", + "status": "running", "createdAt": "2026-08-11T00:00:00.000Z" } } diff --git a/testdata/execution_recovery_v1/safe_inner_failure.json b/testdata/execution_recovery_v1/safe_inner_failure.json new file mode 100644 index 0000000..c5c5355 --- /dev/null +++ b/testdata/execution_recovery_v1/safe_inner_failure.json @@ -0,0 +1,23 @@ +{ + "version": 1, + "kind": "defensive", + "rule": "R2", + "httpStatus": 200, + "requireChainEvidence": false, + "expect": "failure", + "response": { + "executionId": "exec_fixture_safe_inner_001", + "status": "completed", + "transactionHash": "0x5555555555555555555555555555555555555555555555555555555555555555", + "receipts": [ + { + "hash": "0x5555555555555555555555555555555555555555555555555555555555555555", + "verified": false, + "receiptStatus": "safe_inner_failure", + "verifiedAt": "2026-08-11T00:01:00.000Z" + } + ], + "completedAt": "2026-08-11T00:01:00.000Z" + }, + "note": "DEFENSIVE. safe_inner_failure is a conclusive non-success receipt (verify-receipt.ts). KEEP-966 would settle the row as failed; the client must still not treat completed as success." +} diff --git a/testdata/execution_recovery_v1/unconfirmed.json b/testdata/execution_recovery_v1/unconfirmed.json index bc25883..49fc2db 100644 --- a/testdata/execution_recovery_v1/unconfirmed.json +++ b/testdata/execution_recovery_v1/unconfirmed.json @@ -1,4 +1,6 @@ { + "version": 1, + "kind": "observed", "rule": "R1", "httpStatus": 200, "requireChainEvidence": false, @@ -7,7 +9,15 @@ "executionId": "exec_fixture_unconfirmed_001", "status": "unconfirmed", "transactionHash": "0x2222222222222222222222222222222222222222222222222222222222222222", + "receipts": [ + { + "hash": "0x2222222222222222222222222222222222222222222222222222222222222222", + "verified": false, + "receiptStatus": "not_found", + "verifiedAt": "2026-08-11T00:00:30.000Z" + } + ], "createdAt": "2026-08-11T00:00:00.000Z" }, - "note": "Broadcast but not yet confirmed — poll same executionId; do not resubmit." + "note": "Broadcast but the receipt was unreadable. Server settles unconfirmed, not failed. Poll the same executionId; do not resubmit." } From da8b4185e9a61eb61c9cec1f59c312b4db271ccb Mon Sep 17 00:00:00 2001 From: Jacob Sussmilch Date: Tue, 18 Aug 2026 09:12:06 +1000 Subject: [PATCH 5/7] fix(execrecovery): stop waiting on unconfirmed and exit zero An unreadable receipt is not a failed transaction. The wait paths treated `unconfirmed` as pending, so `--wait` and `--watch` polled it to an expired budget and exited non-zero. That non-zero exit invites a re-run of an intent whose transaction may already be on chain, which is the double-broadcast the idempotency work in this branch exists to prevent. Stop on `unconfirmed` and report it instead. `--wait` exits zero there, printing the status and transaction hash; the server keeps reconciling the row and the settled status can be read later against the same execution ID. Classify gains a distinct `unconfirmed` outcome so the classifier cannot say "pending" while the CLI stops, and the direct-execution vocabulary lists it under the client stop-waiting set. Pending/Terminal are documented as client wait semantics, not a claim that the server considers the row final: the server's own type doc keeps `unconfirmed` non-terminal. --- cmd/execute/transfer.go | 18 ++++++-- cmd/execute/transfer_recovery_test.go | 41 +++++++++++++++++++ docs/execution-recovery-v1/contract.md | 16 +++++--- docs/execution-recovery.md | 9 ++++ internal/execrecovery/classify.go | 18 ++++++-- .../execrecovery/classify_receipt_test.go | 6 +-- internal/execrecovery/vocabulary.go | 13 +++++- .../execution_recovery_v1/unconfirmed.json | 4 +- 8 files changed, 104 insertions(+), 21 deletions(-) diff --git a/cmd/execute/transfer.go b/cmd/execute/transfer.go index e4f6c3a..08c4c91 100644 --- a/cmd/execute/transfer.go +++ b/cmd/execute/transfer.go @@ -28,9 +28,18 @@ type transferResponse struct { TransactionHash *string `json:"transactionHash,omitempty"` } +// execTerminalStatuses are the statuses at which the CLI stops waiting. +// +// `unconfirmed` is not terminal on the server: a reconciliation sweep still +// settles that row to completed or failed once the chain answers. The CLI +// stops on it anyway and reports it, rather than polling to a non-zero +// timeout, because a non-zero exit invites a re-run that broadcasts a second +// transaction for an intent that may already be on chain. Read the settled +// status later with `kh ex st `. var execTerminalStatuses = map[string]bool{ - "completed": true, - "failed": true, + "unconfirmed": true, + "completed": true, + "failed": true, } func NewTransferCmd(f *cmdutil.Factory) *cobra.Command { @@ -231,8 +240,9 @@ func printExecStatusResult(p *output.Printer, sr *ExecStatusResponse) error { // execOutcomeError returns a non-nil error for failed terminal states and for // any receipt that is not explicitly successful when the run has completed, or // for a conclusive on-chain failure (reverted / safe_inner_failure) at any -// status. not_found / timeout receipts on unconfirmed stay non-terminal: the -// server treats those as unread, not failed. +// status. not_found / timeout receipts leave `unconfirmed` a zero-exit +// outcome: the server treats those as unread, not failed, so erroring here +// would invite the re-run that double-broadcasts. func execOutcomeError(sr *ExecStatusResponse) error { if sr.Status == "failed" { msg := fmt.Sprintf("execution %s failed", sr.ExecutionID) diff --git a/cmd/execute/transfer_recovery_test.go b/cmd/execute/transfer_recovery_test.go index b149486..9df615b 100644 --- a/cmd/execute/transfer_recovery_test.go +++ b/cmd/execute/transfer_recovery_test.go @@ -313,6 +313,47 @@ func TestTransferCmd_WaitPersistent404TimesOut(t *testing.T) { } } +// An unreadable receipt must not become a non-zero exit: that is what makes a +// caller re-run and broadcast a second transaction for an intent that may +// already be on chain. +func TestTransferCmd_WaitStopsOnUnconfirmedAndExitsZero(t *testing.T) { + var statusReads int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.Method == http.MethodPost { + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-unconf","status":"pending"}`)) + return + } + atomic.AddInt32(&statusReads, 1) + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(map[string]any{ + "executionId": "exec-unconf", + "status": "unconfirmed", + "transactionHash": "0xunconf", + "receipts": []map[string]any{ + {"hash": "0xunconf", "verified": false, "receiptStatus": "not_found", "verifiedAt": "2026-08-11T00:00:00Z"}, + }, + }) + })) + defer srv.Close() + + ios, out, _, _ := iostreams.Test() + f := newTransferFactory(ios, srv) + cmd := execute.NewTransferCmd(f) + cmd.SetArgs([]string{"--chain", "1", "--to", "0xabc", "--amount", "0.1", "--wait", "--timeout", "10s"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("unconfirmed must exit zero, got %v", err) + } + if got := atomic.LoadInt32(&statusReads); got != 1 { + t.Fatalf("status reads=%d, want 1 (unconfirmed must not be polled through)", got) + } + if s := out.String(); !strings.Contains(s, "unconfirmed") || !strings.Contains(s, "0xunconf") { + t.Fatalf("expected status and hash in output, got %q", s) + } +} + func TestTransferCmd_WaitFailsOnSafeInnerFailure(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") diff --git a/docs/execution-recovery-v1/contract.md b/docs/execution-recovery-v1/contract.md index e61d039..ac0f1ad 100644 --- a/docs/execution-recovery-v1/contract.md +++ b/docs/execution-recovery-v1/contract.md @@ -1,6 +1,6 @@ # Execution recovery contract (normative) -Version: **1.2.0** +Version: **1.3.0** Audience: KeeperHub CLI / MCP / HTTP adapter authors Published path: this file is synced to docs.keeperhub.com via `docs/execution-recovery.md`. @@ -20,9 +20,11 @@ It does **not** cover `POST /api/workflows//webhook`. ## Status vocabularies (do not mix) +"Pending" and "Terminal" below are **client wait semantics**: keep polling, or stop waiting and report. Terminal is not a claim that the server will never change the row again. + | Surface | Pending | Terminal | | --- | --- | --- | -| Direct execution (`GET /api/execute/{id}/status`) | `pending`, `running`, `unconfirmed` | `completed`, `failed` | +| Direct execution (`GET /api/execute/{id}/status`) | `pending`, `running` | `unconfirmed`, `completed`, `failed` | | Workflow run (`GET /api/workflows/executions/{id}/status`) | `pending`, `running` | `success`, `error`, `cancelled` | Server enum (`app/api/execute/_lib/types.ts`): `pending | running | unconfirmed | completed | failed`. There is no `queued` value. @@ -36,11 +38,13 @@ Receipt statuses (`lib/web3/verify-receipt.ts`): `success | reverted | not_found ## Rules -### R1 — Unconfirmed → poll, do not resubmit +### R1 — Poll the same ID, never resubmit + +If status is `pending` or `running`, continue status reads against the **same** execution ID. Do not issue a new write for the same logical intent while that execution ID remains durable. -If status is `pending`, `running`, or `unconfirmed`, continue status reads against the **same** execution ID. Do not issue a new write for the same logical intent while that execution ID remains durable. +`unconfirmed` means the transaction was broadcast but no receipt could be read yet. The server keeps that row open (`completedAt` stays null) and a reconciliation sweep settles it to `completed` or `failed` once the chain answers. A waiting client must **stop** there and report it, rather than poll to an expired budget and exit non-zero: a non-zero exit invites a re-run, and re-running an intent that may already be on chain is the double-spend this contract exists to prevent. Read the settled status later against the same execution ID. -**CLI conformance:** `kh ex transfer --wait` / `kh ex cc --wait` poll the same ID. +**CLI conformance:** `kh ex transfer --wait` / `kh ex cc --wait` poll the same ID, and stop on `unconfirmed` with exit code 0, printing the status and transaction hash. ### R2 — Receipts (client invariant) @@ -91,7 +95,7 @@ A first status read that returns HTTP 404 immediately after submit may be transi | --- | --- | --- | --- | --- | | `pending.json` | observed | R1 | pending | `TestFixtures_ClassifyTable` | | `running.json` | observed | R1 | pending | `TestFixtures_ClassifyTable` | -| `unconfirmed.json` | observed | R1 | pending | `TestFixtures_ClassifyTable` | +| `unconfirmed.json` | observed | R1 | unconfirmed | `TestFixtures_ClassifyTable` | | `completed_with_tx.json` | observed | R2 | success | `TestFixtures_ClassifyTable` | | `completed_without_tx.json` | classifier | R2 | failure (strict option) | `TestFixtures_ClassifyTable` | | `reverted.json` | defensive | R2 | failure | `TestFixtures_ClassifyTable` + `TestRevertedIsNeverSuccess` | diff --git a/docs/execution-recovery.md b/docs/execution-recovery.md index 99b8c60..894db15 100644 --- a/docs/execution-recovery.md +++ b/docs/execution-recovery.md @@ -35,6 +35,12 @@ observed production envelope. (`app/api/execute/_lib/types.ts`. There is no `queued` status on this endpoint.) +Poll while a row is `pending` or `running`. Stop on `unconfirmed`, `completed` +or `failed`. `unconfirmed` means the transaction was broadcast but no receipt +could be read; the server keeps reconciling that row, so treat it as "stop +waiting and report", not as a failure, and read the settled status later +against the same execution ID. + Workflow run status uses a different vocabulary (`success` / `error` / `cancelled`) — do not mix it with direct-execution statuses. @@ -52,6 +58,9 @@ Workflow run status uses a different vocabulary (`success` / `error` / a timeout error. - `--watch` does **not** treat 404 as pending. A mistyped or foreign-org id exits with an error instead of looping. +- `--wait` and `--watch` stop on `unconfirmed` and report it. `--wait` exits + **zero** there, printing the status and transaction hash, because a non-zero + exit invites a re-run that would broadcast a second transaction. - Wait paths fail when `status=failed`, when a receipt is `reverted` or `safe_inner_failure`, and when `status=completed` carries any non-success receipt. They do not implement a `--require-verified` flag. diff --git a/internal/execrecovery/classify.go b/internal/execrecovery/classify.go index bae32cd..5f5d045 100644 --- a/internal/execrecovery/classify.go +++ b/internal/execrecovery/classify.go @@ -13,9 +13,13 @@ import ( type Outcome string const ( - OutcomePending Outcome = "pending" - OutcomeSuccess Outcome = "success" - OutcomeFailure Outcome = "failure" + OutcomePending Outcome = "pending" + OutcomeSuccess Outcome = "success" + OutcomeFailure Outcome = "failure" + // OutcomeUnconfirmed is broadcast-but-unreadable. It is neither success nor + // failure: the transaction may already be on chain. Callers stop waiting and + // report it; they must not poll it through and must not resubmit. + OutcomeUnconfirmed Outcome = "unconfirmed" OutcomeMalformed Outcome = "malformed" OutcomeRateLimited Outcome = "rate_limited" OutcomeUnrecognized Outcome = "unrecognized" @@ -73,6 +77,10 @@ type Sample struct { // (app/api/execute/_lib/types.ts). Workflow run statuses (success|error|cancelled) // belong to a different API and must not be fed here — see Vocabulary(). // +// `unconfirmed` maps to OutcomeUnconfirmed, not OutcomePending: the server +// keeps reconciling that row, but a client must stop waiting on it rather than +// poll to a failure the chain never reported. +// // An unknown future status is OutcomeUnrecognized (never success, never // malformed) so a server addition does not look like a corrupt body. func Classify(sample Sample, opts Options) (Outcome, string) { @@ -113,8 +121,10 @@ func Classify(sample Sample, opts Options) (Outcome, string) { } switch status { - case "pending", "running", "unconfirmed": + case "pending", "running": return OutcomePending, status + case "unconfirmed": + return OutcomeUnconfirmed, status case "failed": return OutcomeFailure, status case "completed": diff --git a/internal/execrecovery/classify_receipt_test.go b/internal/execrecovery/classify_receipt_test.go index c4c650b..056578a 100644 --- a/internal/execrecovery/classify_receipt_test.go +++ b/internal/execrecovery/classify_receipt_test.go @@ -31,10 +31,10 @@ func TestClassify_ReceiptStates(t *testing.T) { } for _, st := range []string{"not_found", "timeout"} { st := st - t.Run("unconfirmed "+st+" is pending", func(t *testing.T) { + t.Run("unconfirmed "+st+" is unconfirmed", func(t *testing.T) { got, reason := execrecovery.Classify(execrecovery.Sample{HTTPStatus: 200, Body: unconfirmed(st)}, execrecovery.Options{}) - if got != execrecovery.OutcomePending { - t.Fatalf("got %s (%s), want pending", got, reason) + if got != execrecovery.OutcomeUnconfirmed { + t.Fatalf("got %s (%s), want unconfirmed", got, reason) } }) } diff --git a/internal/execrecovery/vocabulary.go b/internal/execrecovery/vocabulary.go index 73932c8..502a647 100644 --- a/internal/execrecovery/vocabulary.go +++ b/internal/execrecovery/vocabulary.go @@ -2,6 +2,10 @@ package execrecovery // Vocabulary documents which status strings belong to which API surface. // Direct-execution and workflow-run statuses must not be mixed. +// +// Pending and Terminal are client wait semantics: Pending means keep polling, +// Terminal means stop waiting and report. Terminal is not a claim that the +// server will never change the row again. type Vocabulary struct { Surface string Pending []string @@ -10,11 +14,16 @@ type Vocabulary struct { // DirectExecutionVocabulary is GET /api/execute/{id}/status // (app/api/execute/_lib/types.ts ExecutionStatus). +// +// `unconfirmed` is listed Terminal in the client sense only. The server +// documents it as non-terminal and a reconciliation sweep settles it to +// completed or failed; clients still stop there so that an unreadable receipt +// never becomes a re-run that broadcasts twice. func DirectExecutionVocabulary() Vocabulary { return Vocabulary{ Surface: "direct-execution", - Pending: []string{"pending", "running", "unconfirmed"}, - Terminal: []string{"completed", "failed"}, + Pending: []string{"pending", "running"}, + Terminal: []string{"unconfirmed", "completed", "failed"}, } } diff --git a/testdata/execution_recovery_v1/unconfirmed.json b/testdata/execution_recovery_v1/unconfirmed.json index 49fc2db..acc7216 100644 --- a/testdata/execution_recovery_v1/unconfirmed.json +++ b/testdata/execution_recovery_v1/unconfirmed.json @@ -4,7 +4,7 @@ "rule": "R1", "httpStatus": 200, "requireChainEvidence": false, - "expect": "pending", + "expect": "unconfirmed", "response": { "executionId": "exec_fixture_unconfirmed_001", "status": "unconfirmed", @@ -19,5 +19,5 @@ ], "createdAt": "2026-08-11T00:00:00.000Z" }, - "note": "Broadcast but the receipt was unreadable. Server settles unconfirmed, not failed. Poll the same executionId; do not resubmit." + "note": "Broadcast but the receipt was unreadable. The server settles unconfirmed, not failed, and a reconciliation sweep finishes the row later. Wait paths stop here and exit zero; never resubmit this intent. Read the settled status later against the same executionId." } From cc7a9c40d15bb49d13a2733fff430afe44ceb27f Mon Sep 17 00:00:00 2001 From: mohamedwael201193 Date: Tue, 18 Aug 2026 03:22:27 +0300 Subject: [PATCH 6/7] test(execute): drop empty conflict-error branch that failed staticcheck SA9003: the rotate/new-key check had no body; the following assertion already requires do not retry with a new key. --- cmd/execute/transfer_recovery_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/cmd/execute/transfer_recovery_test.go b/cmd/execute/transfer_recovery_test.go index 9df615b..b578d3a 100644 --- a/cmd/execute/transfer_recovery_test.go +++ b/cmd/execute/transfer_recovery_test.go @@ -275,9 +275,6 @@ func TestTransferCmd_IdempotencyConflictFailsWithoutNewKey(t *testing.T) { if !strings.Contains(err.Error(), "idempotency") && !strings.Contains(err.Error(), "different request payload") { t.Fatalf("got %v", err) } - if strings.Contains(err.Error(), "rotate") && strings.Contains(err.Error(), "new key for a different") { - // server message mentions a new key for a *different* request; we must still refuse auto-rotate - } if !strings.Contains(err.Error(), "do not retry with a new key") { t.Fatalf("conflict must tell the user not to mint a new key: %v", err) } From 7cdd7be20481264a3a117cea13aa5ccab5fb6cb1 Mon Sep 17 00:00:00 2001 From: Jacob Sussmilch Date: Tue, 18 Aug 2026 10:27:26 +1000 Subject: [PATCH 7/7] docs: correct the --require-verified statements now that the flag ships Both pages said the CLI does not implement --require-verified. #95 landed it on kh ex status, so the claim was false and contradicted kh_execute_status.md in this same branch. State what the flag does instead: without it a completed execution with an empty receipts array is still success, with it the CLI exits non-zero unless every receipt is verified with receiptStatus success, and unconfirmed fails the gate. Contract bumped to 1.4.0 - sync-cli-docs publishes this page on release. --- docs/execution-recovery-v1/contract.md | 4 ++-- docs/execution-recovery.md | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/docs/execution-recovery-v1/contract.md b/docs/execution-recovery-v1/contract.md index ac0f1ad..78c4c95 100644 --- a/docs/execution-recovery-v1/contract.md +++ b/docs/execution-recovery-v1/contract.md @@ -1,6 +1,6 @@ # Execution recovery contract (normative) -Version: **1.3.0** +Version: **1.4.0** Audience: KeeperHub CLI / MCP / HTTP adapter authors Published path: this file is synced to docs.keeperhub.com via `docs/execution-recovery.md`. @@ -56,7 +56,7 @@ Never infer success from `status=completed` alone. KEEP-966 (`completeExecution`) re-verifies every claimed hash before writing `completed`. A reverted receipt is stored `verified: false` and the row settles as `failed`. `{status:"completed", verified:true, receiptStatus:"reverted"}` is **not** an observed production envelope. Fixtures that use that shape are labeled `kind: defensive` so the client stays fail-closed if the gate regresses. -The shipped CLI does **not** set a “require chain evidence” flag and does **not** implement `--require-verified`. Completed with an empty `receipts` array is still treated as success (matches a no-hash completion). +The shipped CLI implements `--require-verified` on `kh ex status`; the write commands have no such flag. Without it, `completed` with an empty `receipts` array is still treated as success, matching a no-hash completion. With it, the CLI exits non-zero unless the execution completed carrying at least one receipt and every receipt is `verified: true` with `receiptStatus: success`; `unconfirmed` fails the gate as not proven landed. **CLI conformance:** wait paths fail on `status=failed` and on non-success receipts as above. diff --git a/docs/execution-recovery.md b/docs/execution-recovery.md index 894db15..9114908 100644 --- a/docs/execution-recovery.md +++ b/docs/execution-recovery.md @@ -63,7 +63,12 @@ Workflow run status uses a different vocabulary (`success` / `error` / exit invites a re-run that would broadcast a second transaction. - Wait paths fail when `status=failed`, when a receipt is `reverted` or `safe_inner_failure`, and when `status=completed` carries any non-success - receipt. They do not implement a `--require-verified` flag. + receipt. +- `kh ex status --require-verified` additionally demands chain proof: it exits + non-zero unless the execution completed carrying at least one receipt and + every receipt is `verified: true` with `receiptStatus: success`. `unconfirmed` + fails that gate, and so does a completion with an empty `receipts` array, + which is treated as success without the flag. ## Fixtures