fix(execute): reconcile writes that report completed without a transaction hash

[winsznx]

The bug

A successful POST /api/execute/contract-call can return 202 with status: "completed" and no transactionHash. The hash only appears on GET /api/execute/{id}/status.

Both kh ex transfer and kh ex contract-call treat any completed as terminal:

if execTerminalStatuses[execResp.Status] {
    return printTransferResult(p, &execResp)
}

So --wait prints a success and returns while the caller still has no transaction to verify. That is the one outcome the flag exists to prevent.

This is not hypothetical. Here is a real response from the live API:

POST /api/execute/contract-call -> HTTP 202
{"executionId":"exnn6k0y1ojnnvb8sa1fu","status":"completed"}

The transaction was real and did land: 0xb4098917…d452dc. The hash was only obtainable from the status endpoint.

The fix

--wait now reconciles against the status endpoint when a completed write carries no hash, and returns immediately when it does, so the common case costs no extra request.

if execTerminalStatuses[execResp.Status] && !writeNeedsReconciliation(execResp.Status, execResp.TransactionHash) {

Also: X-Poll-Interval-Hint had no effect

fetchExecStatus discarded the *http.Response, so the header could not be read, and both poll loops ran on a hardcoded time.NewTicker(2 * time.Second). The documented pacing header was silently ignored.

The hint is now honoured, a hint of 0 is treated as terminal, and an absent or unparseable hint falls back to the previous 2s default. Polling remains bounded by --timeout, and the wait is clamped so it never overshoots the deadline.

A side effect: the existing cmd/execute suite went from 8.5s to 2.5s, because tests no longer wait on a fixed timer.

Not changed

unconfirmed is already handled correctly. It is absent from execTerminalStatuses, so it is treated as non-terminal and polling continues. No change needed.

Tests

• 6 cases for the reconciliation predicate, including empty-string and failed shapes
• 6 cases for hint parsing: absent, honoured, zero, whitespace, unparseable, negative
• 3 behavioural tests driving the real commands against httptest

I checked that each behavioural test fails against the previous behaviour rather than passing regardless:

--- FAIL: TestTransferCmd_WaitReconcilesCompletedWithoutHash
    expected the status endpoint to be polled so the caller ends up with a transaction hash
--- FAIL: TestContractCallCmd_WaitReconcilesCompletedWithoutHash
    expected the status endpoint to be polled for a completed write with no hash
--- FAIL: TestPollHonoursServerInterval
    second poll came after 1.987899583s, which suggests the old fixed 2s timer rather than the hint

go build ./..., go vet ./... and the full go test ./... are green on Go 1.26.

Scope

Deliberately narrow: no new flags, no request-shape changes, no network to chainId migration. Those are worth doing but are separate conversations, and I did not want a correctness fix to depend on agreeing about them.

Found while building keeperhub-flightcheck, an onboarding execution-conformance check, for the Agents Onchain hackathon. Happy to adjust anything here.

[winsznx]

Heads-up on a file overlap, so this doesn't collide with work already in flight.

#95 (feat(execute): require chain-verified receipts in status) also touches cmd/execute/status.go. As far as I can tell the two changes are complementary rather than competing: #95 tightens what counts as a verified receipt, this one stops --wait returning before a transaction hash exists and makes polling honour X-Poll-Interval-Hint. But they will conflict textually in status.go, and #95 was opened first.

I'm happy to rebase on top of #95 once it lands, or to split the X-Poll-Interval-Hint part out if that makes either easier to review. Whatever suits the maintainers and @vaibhav4046.

I also want to be explicit that this PR deliberately does not touch idempotency-key handling or execution recovery, since #97 and related work already cover that ground.

[suisuss]

This is your first PR here - ISSUES.md requires an accepted issue before any behavior-changing PR, referenced in the title as fix: #<n> <description> per CONTRIBUTING.md.

What this changes

cmd/execute/transfer.go:112 and cmd/execute/contract_call.go:127 gate the "is this write terminal" check behind a new writeNeedsReconciliation check (transfer.go:215-217): a broadcast response reporting status: "completed" with a nil or empty transactionHash no longer short-circuits --wait; it falls through to poll /api/execute/<id>/status instead. fetchExecStatus (transfer.go:182-206) now also reads the X-Poll-Interval-Hint response header (status.go:26-39, new nextPollDelay) and returns the delay to sleep plus a "server says terminal" bool alongside the status. pollExecStatus (transfer.go:146-178) and watchExecStatus (status.go:142-166) drop their time.NewTicker(2s) + select{default: sleep(50ms)} busy-loop and instead fetch immediately, then sleep for whatever the server asked for (falling back to 2s, capped to the remaining --timeout in pollExecStatus; uncapped in watchExecStatus). Two new test files cover the reconciliation predicate, hint parsing, and end-to-end behavior against httptest servers.

Does it match the description

Scope creep - the write-up accurately describes both changes it makes: the reconciliation fix matches the diff exactly, including the live-API repro, and the X-Poll-Interval-Hint fix is also accurately described, including its effect on test-suite runtime. Nothing is undersold or oversold about what each piece does. But the two are independent: writeNeedsReconciliation gating the broadcast-response check works correctly with the old fixed-2s ticker still in place, and the poll-hint honoring works correctly with no reconciliation gate at all - reverting either half in isolation leaves the other fully correct and deployable. The body's own "Scope" section calls the change "deliberately narrow" in the same breath as bundling this second, unrelated fix. ISSUES.md is explicit on this exact situation: "confirm it is one issue: if any part could be fixed and shipped while another stays broken, those are separate issues." The contributor's comment already volunteers to split the hint change out.

Blocking

• No issue reference anywhere in the title (fix(execute): reconcile writes that report completed without a transaction hash), and ISSUES.md requires an accepted issue before any PR that changes command behavior - this qualifies twice over, under both "Command behaviour, output format, or exit codes" and "Anything that changes what an existing flag does" (--wait's and --watch's polling/termination semantics both change). check-issue-link is failing on the head commit right now, confirming this mechanically. Close by filing an issue (or, given the scope-creep finding below, two issues - one for the reconciliation bug, one for the poll-interval-hint behavior), getting each accepted, then retitling fix: #<n> <description> per CONTRIBUTING.md, splitting into two PRs if the maintainers agree with the split.
• cmd/execute/transfer.go:155 (pollExecStatus), cmd/execute/status.go:155 (watchExecStatus), cmd/execute/status.go:88 (plain kh ex status) - the reconciliation check added at the broadcast response is not applied to the status responses these three consume. All three treat any status: "completed" from GET /api/execute/<id>/status as final via execTerminalStatuses["completed"], with no equivalent of writeNeedsReconciliation guarding it. Concrete scenario: the status endpoint itself returns {"executionId":"e1","status":"completed"} with no transactionHash key - the same response shape the PR's own bug report shows the live API producing on the broadcast endpoint, and nothing in the diff or the new tests establishes the status endpoint is immune to the same condition. pollExecStatus returns via printExecStatusResult (transfer.go:219-231), which omits the TX Hash row when the pointer is nil/empty and returns nil; watchExecStatus/kh ex status do the same via renderExecStatus (status.go:104-140). kh ex transfer --wait (or kh ex st --watch) then exits 0 reporting "completed" with no hash and no error - the exact outcome writeNeedsReconciliation's own comment names as "the one outcome the flag exists to prevent," now one hop downstream of the fix. Apply the same completed-without-hash check everywhere a status response is treated as terminal, not only at the broadcast response.

Mechanical - actionable as-is

• cmd/execute/status.go:26-39 - nextPollDelay accepts any non-negative integer with no upper bound. pollExecStatus clamps the sleep to the remaining --timeout, but watchExecStatus has no deadline at all, so a large-but-parseable hint (a units mismatch, e.g. milliseconds sent where seconds are expected, or a server bug) leaves kh ex st --watch sleeping for that entire duration with nothing capping it short of Ctrl-C. Clamp to a small sane ceiling, e.g. min(secs, 30).
• cmd/execute/transfer.go:182 - fetchExecStatus now returns four values (*ExecStatusResponse, time.Duration, bool, error); at status.go:88 two of the four are already discarded with _, _. A small result struct would read more clearly at each of the three call sites. Not required.

With the team

Two things worth a maintainer decision rather than a mechanical fix. First, whether to require the contributor's own offered split (reconciliation fix and poll-interval-hint honoring as two issues/PRs) given ISSUES.md's scope rule matches this case directly. Second, how to sequence this against #95 (feat(execute): require chain-verified receipts in status, open, also rewrites cmd/execute/status.go, opened before this PR) - the contributor already flagged the textual collision and offered to rebase once #95 lands; worth confirming which lands first before either gets a deeper review pass.

Verdict

Changes requested: the issue gate is unmet (and CI already says so), the reconciliation fix does not cover the polling paths that reach the same status field, and a real independent seam should be split before merge.

[suisuss]

Answering the two things you offered, which I left hanging in the review.

Split it. The reconciliation guard and the X-Poll-Interval-Hint handling are each correct with the other reverted, so they are two changes, and the poll-hint half is the smaller and lands faster on its own.

On the overlap you flagged: you are right that #95 touches cmd/execute/status.go as well - both branches change that file. Do not sequence around it yourself. Whichever lands first, the other rebases, and the reconciliation half is the one that will need the rebase since it touches more of that file.

The remaining code ask from the review still stands: the completed-without-hash guard belongs on the paths that consume a status response, not only the broadcast response - pollExecStatus in transfer.go, watchExecStatus in status.go, and plain kh ex status. What the CLI should do when the status endpoint itself reports completed with no hash - keep polling, error, or exit non-zero - is the part I am still settling with the team, and I will come back with a verdict shortly. Nothing there is blocked on you.

[suisuss]

I have settled the question this PR turned on, and it went the other way from the premise you built on, so I pushed the change rather than asking you to rework it.

A completed execution with no transaction hash is not an error. It is reachable for any non-transaction action - a read-only contract call, a non-transaction plugin step - which completes with no hash by design. So all three status paths now report the condition and exit zero: kh ex status, --watch, and transfer/cc --wait each show a Transaction none submitted row instead of failing. Turning that into a non-zero exit is opt-in and belongs behind --require-verified, which is #95's flag, so it is not duplicated here.

Your poll-hint work stands as filed. The hint is now clamped to 30 seconds - watchExecStatus has no deadline to fall back on, so an oversized hint stalled the loop indefinitely. The zero and negative guards you already had needed no change; only the ceiling was missing.

unconfirmed is now terminal, and --wait exits zero on it printing the status and hash. The reason for zero rather than non-zero is fund safety: a non-zero exit invites a retry, and a retry that mints a fresh idempotency key is what broadcasts a second transaction.

Two things to flag. First, #95 also adds unconfirmed to execTerminalStatuses and reworks renderExecStatus and watchExecStatus in the same blocks this branch touches, so whichever of the two lands first, the other needs a rebase - keep this branch's hint-driven delay rather than reinstating the hardcoded two-second ticker. Second, your description still says unconfirmed needs no change and frames the fix as preventing an unverifiable success; both are now out of date. I have left the description alone since it is yours to edit.

Seven tests added. I checked none of them are vacuous by reverting each fix in turn and confirming the matching test fails.

[suisuss]

Merged main in and resolved the conflicts here rather than leaving them with you.

main picked up two things since you last pushed: a shared terminalExecError (the immediate write path used to exit zero on a fast failure while the poll path errored on the same status), and #95's --require-verified gate with its unconfirmed notice.

Your loop shape won everywhere it mattered. The hint-driven delay, the 30-second clamp and serverSaysTerminal all survive; main's time.NewTicker(2 * time.Second) and its select/default busy-wait are gone from both pollExecStatus and watchExecStatus. The terminal branches now run the failure check first, then print, then the unconfirmed notice - and your && !completedWithoutTransaction(...) guard still sends a completed-with-no-hash write through the reconciling fetch. renderExecStatus is the union: receipt rows from #95, your Transaction none submitted row kept where you put it, after the TX Link row.

One thing to know about your branch: you and #95 had both written a test called TestTransferCmd_WaitStopsOnUnconfirmed, in different files. Two top-level declarations of the same name do not compile, so yours is now TestTransferCmd_WaitStopsOnUnconfirmedWithPollHint - it is the one that exercises the poll-hint path, so the longer name is the more accurate one anyway. Both tests are kept.

The resolution was mutation-checked rather than eyeballed: removing unconfirmed from the terminal set fails six tests across both sides, and dropping the terminalExecError call fails the fast-failure test. Build, vet and go test -race ./... are green, and go generate ./docs/ leaves the tree clean.