Skip to content

recipes: surface success:false refusals (silent local-file loss on delete; start timeouts) - #92

Merged
zaynelt merged 4 commits into
workato-devs:developfrom
hoswork:feat/surface-start-code-errors
Jul 21, 2026
Merged

recipes: surface success:false refusals (silent local-file loss on delete; start timeouts)#92
zaynelt merged 4 commits into
workato-devs:developfrom
hoswork:feat/surface-start-code-errors

Conversation

@hoswork

@hoswork hoswork commented Jul 16, 2026

Copy link
Copy Markdown

wk recipes delete on a running recipe reports success, deletes the local .recipe.json, and leaves the server recipe running — silent local/server divergence. Root cause (same as the #70 start timeout): the recipe lifecycle endpoints return HTTP 200 for refusals with success:false in the body, and the CLI discards the body.

Four commits, one mechanism:

  1. Start() decodes the body → typed ActivationError (code_errors + config_errors); the start command prints step errors immediately instead of polling; bulk/--no-wait collects blocked recipes and continues the batch; exit code 2 (ExitValidation).
  2. Delete()/Update()/Move() decode refusals → typed MutationRefusedError, exit 2; the delete command no longer removes local files on a refused server delete. (wk push is unaffected — it uses the packaging API, which legitimately hot-updates running recipes.)
  3. Decoder robustness from live shapes: detail tuples may have 3 elements (no path) or trailing extras at either nesting level; config_errors reuse the layout with a non-string tail. All tolerated; success:false can never be silently swallowed.
  4. Command-level tests (following the repo's existing httptest + file-store profile pattern from sync_discover_test.go) pinning the advertised CLI behavior: blocked start errors without entering the poll loop, bulk start continues past blocked recipes, refused delete preserves local files.

Claim → evidence:

claim evidence
200 + success:false envelope, 4 failure classes test fixtures in recipes_test.go, recorded live 2026-07-02/03 (trial)
identical on GA prod US (3 classes replayed) author-run replays; byte-identical bodies (broken schema, unknown datapill, invalid operand)
delete/update refusal shape fixtures in recipes_test.go, recorded live
behavior claims (no-poll on blocked start, batch continue, local files kept, exit 2) command-level + unit tests in-repo; timings (0.7 s vs 2 min) additionally author-live-verified against a trial workspace

Verification (re-run 2026-07-16 after rebasing onto current develop, a544213): go build ./... + go test ./... -count=1 green; golangci-lint run --new-from-rev=upstream/develop → 4 findings, all unchecked w.Write/Encode errcheck in httptest handlers, matching the repo's existing test idiom; gosec → 5 findings in touched files, all on pre-existing lines from the initial commit (G306/G104), none on lines added or modified here; govulncheck → reports GO-2026-5856, which is also present on clean develop — pre-existing dependency exposure, not introduced by this change, flagged here as an FYI.

Known limitations (called out for review): doRaw sends no Accept header and skips do()'s JSON error-message extraction on non-2xx (status-based exit-code classification is unaffected); command-layer coverage is limited to the three refusal-path tests added here (no broader harness existed); POST /recipes import reads but doesn't check success (fails indirectly via Get(0) — follow-up candidate, see inventory below).

API response-handling inventory (per-endpoint, live-probed)

wk ↔ Workato API response-handling inventory

Compiled 2026-07-02 for the surface-refusals PR. Every claim marked probed was
observed live (trial workspace; start/delete also replicated on GA prod US).
Everything else is honestly marked unprobed — we only claim what we saw.

The pattern

Several recipe-lifecycle endpoints return HTTP 200 for refusals, carrying the
real outcome in the body (success:false + reasons). Any wk call that passes a
nil decode target treats these as success. Some other endpoint families (folders,
projects) correctly use 4xx for refusals — the idiom is inconsistent across the API.

Probed endpoints (evidence in fixtures/)

endpoint on refusal wk before this PR fixed here suggested upstream layer
PUT /recipes/:id/start (blocked activation) 200 + success:false + code_errors 2-min poll timeout, zero info (#70) ✅ typed ActivationError, instant wk ✅ · docs (shape undocumented) · platform (4xx or keep, their call)
DELETE /recipes/:id (running) 200 + success:false + errors.running silent false success + deletes the local .recipe.json ✅ typed MutationRefusedError, local files preserved wk ✅ · docs (DELETE doc shows only success; UPDATE doc documents its prohibition, DELETE doesn't)
PUT /recipes/:id update (running) 200 + success:false "can't modify running recipe" command pre-checks running → exposure is the check-then-act race only ✅ API-layer defense in depth (also covers Move, same endpoint) wk ✅ · docs (prohibition documented, 200-shape not)
PUT /recipes/:id/start (already running) n/a — 200 success:true, idempotent fine
PUT /recipes/:id/stop (running or stopped) n/a — 200 success:true, idempotent fine
DELETE /folders/:id (non-empty) 400 + message error surfaces correctly — (well-behaved)
DELETE /projects/:id (with recipes) 400 + message error surfaces correctly — (well-behaved)
POST /recipes import (success path) body decoded; success field read but unchecked — a false would surface indirectly as Get(0) 404 indirect not changed (works, but error would be confusing) wk follow-up candidate

Previously-unprobed mutations — now probed (2026-07-03)

Every other nil-result mutation probed refuses correctly with 4xx or
succeeds cleanly — the 200+success:false idiom appears confined to the
recipes lifecycle:

call probed scenario behavior
apiEndpoints.Enable flow recipe stopped 400 "The underlying recipe is inactive" ✓
apiEndpoints.Disable active endpoint 200 success:true ✓
connections.Disconnect never-connected shell 200 success:true (idempotent) ✓
connections.Delete unused connection 200 success:true, deleted ✓
apiClients.Delete (v2) client with an active key 200 success:true, cascades key ✓
mcpServers.Delete existing / already-deleted 204 / 404
mcpServers.DeleteTool nonexistent tool 404
mcpServers.AssignUserGroups wrong param shape 400 param error (wants idp_user_group_ids) ✓
recipes.Connect bogus connection id 404

Round 3 (2026-07-03) closed the remainder: connections.Delete on a connection
referenced by a stopped recipe genuinely deletes (allowed by design; the
success:true is truthful — no CLI hazard); recipes.Connect happy path returns
200 success:true; mcpServers.AssignTools with a real recipe tool works and is
idempotent on duplicates. Round 4 captured the last unknown shape:
config_errors (unauthorized connection) — same positional layout as
code_errors but 3-element details plus a non-string i18n tail; recorded in
fixtures/start-unauthorized-connection.txt and now decoded by the PR (this is
the disconnected-connection scenario #70 was originally filed about).
Nothing in the inventory remains unprobed except platform behaviors gated by
role privileges (apiCollections.Delete → 401 create-but-not-delete asymmetry).
apiCollections.Delete is blocked by a client-role privilege gap (401
under an Admin-role dev-API token that can CREATE collections — itself worth
noting: create-but-not-delete role asymmetry).

Refusal-decoder scope

parseMutationRefusal decodes the one 2xx refusal shape observed live
(success:false + errors: {field: [messages]}). A different 2xx refusal
body would still surface as an error, but with the generic message and no
detail lines. Adopting the helper on further endpoints after probing them is
a mechanical change.

Operational notes (not code)

  • APIM endpoint enable → gateway propagation can take ~1 min; the first 401
    after enabling is not a configuration error.
  • Docs gaps: start/delete/update refusal response shapes are undocumented;
    DELETE /recipes/:id documents only the success response.

heiwad and others added 4 commits July 16, 2026 16:18
PUT /recipes/{id}/start returns HTTP 200 for both outcomes; when
activation is blocked the body carries success:false plus a code_errors
payload with the same step-level detail the recipe editor shows inline.
Start() previously discarded the body, so blocked recipes fell through
to the full poll timeout with no actionable output (#70).

Start() now decodes the body and returns *ActivationError. The start
command prints the step errors immediately and skips polling; bulk /
--no-wait mode collects blocked recipes via errors.Join and continues
with the rest of the batch. classifyError maps ActivationError to
ExitValidation (2). Parsing is strictly best-effort: the shape is
undocumented, so anything that doesn't decode as success:false keeps
the previous poll-and-timeout behavior, and a success:false with
undecodable details still errors (with the detail lines omitted).

Fixture bodies recorded live from a trial workspace (2026-07-02)
across three failure classes: unpopulated required schema field,
unknown datapill, invalid if-condition operand.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Same root cause as the start fix: DELETE /recipes/{id} and PUT
/recipes/{id} return HTTP 200 for refusals (e.g. the recipe is
running), carrying success:false + reasons in the body. Delete
discarded the body, reported success, and then removed the local
.recipe.json — leaving the server recipe running and local state
silently wrong. Update/Move shared the exposure at the API layer
(the update command pre-checks the running state, so its exposure
was the check-then-act race).

doRaw gains an optional request body (existing callers pass nil).
Delete/Update/Move decode via parseMutationRefusal — best-effort
like parseActivationError: anything that doesn't decode as
success:false keeps the previous behavior. MutationRefusedError
maps to ExitValidation. Refusal bodies recorded live (trial
workspace 2026-07-02; delete-while-running also observed on GA
prod US).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Probing an invalid action name surfaced a code_errors detail with only
[label, value, message] — no path — so require 3 elements and decode
the path tail best-effort (config_errors reuse the layout with a
non-string fourth element). Decode and render config_errors alongside
code_errors: disconnected-connection activation failures — the exact
scenario #70 was filed about — live there. Fixture recorded live
(trial workspace, 2026-07-03).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Command-level tests (httptest + file-store profile, per the
sync_discover_test pattern) for the behaviors the API-layer changes
exist to produce: a blocked start errors with step detail and never
enters the poll loop, bulk start continues past blocked recipes and
reports each, and a refused server delete preserves the local
.recipe.json + sidecar.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@zaynelt zaynelt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for an exceptionally thorough first contribution, @hoswork — the claim→evidence table and recorded-live fixtures made this straightforward to validate.

Static: go build, go vet, and go test ./... -count=1 all pass on the branch rebased onto develop (a544213). golangci-lint --new-from-rev=origin/develop reports 4 errcheck findings, all unchecked w.Write/Encode in httptest handlers — consistent with the repo's existing test idiom (develop already has 11 in recipes_test.go). gosec findings in the package are all pre-existing; none fall on lines this PR adds or modifies, and recipes.go/http_client.go are clean. No go.mod/go.sum changes.

Live (trial workspace): confirmed the core fix end-to-end. wk recipes delete <running-recipe> surfaced the platform refusal (Can't change the recipe state: invalid state running), exited 2, preserved the local .recipe.json + .meta.json, and left the server recipe running — the exact data-loss path, fixed. Start happy-path still activates and exits 0.

One small note (non-blocking):

  • internal/api/recipes.go:456 — the doc comment ends mid-sentence (...see the fixtures in). Worth completing or trimming. Heads-up so this doesn't get "fixed" into a dangling reference: the fixtures for these shapes live inline in the test files (recipes_test.go / recipe_lifecycle_test.go), not in a fixtures/ directory in this repo — so please point it there, or just drop the trailing clause, rather than adding a fixtures/*.txt path that wouldn't resolve here.

(Validation for this review was run with Claude Code.)

@zaynelt
zaynelt self-requested a review July 16, 2026 22:40
@zaynelt
zaynelt merged commit 4636160 into workato-devs:develop Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants