fix(pcc-node): emit execution_failed, never execution_completed, on failed device results (OH-1, evidence sec-10) - #333
Draft
LamaSu wants to merge 6 commits into
Draft
fix(pcc-node): emit execution_failed, never execution_completed, on failed device results (OH-1, evidence sec-10)#333LamaSu wants to merge 6 commits into
LamaSu wants to merge 6 commits into
Conversation
…vents
fix: pcc-node reported every job that did not raise as a success.
build_evidence_bundle synthesized an "execution_completed" event
unconditionally, embedding the device result verbatim as that event's
payload, and execute() reported job status "completed" without ever
inspecting the returned dict. Adapters signal failure by RETURNING a
dict that says so (printed:False, error:..., status:failed) rather than
by raising, so a genuinely failed run produced a bundle typed
execution_completed with no execution_failed anywhere. A verifier whose
program is (execution_completed present AND execution_failed absent)
would release funds for the exact failure it exists to dispute.
Implements evidence contract sec-10 (ledger OH-1):
- classify_execution_result(result) -> success | failure | unclassifiable.
Pure, first-match-wins over every censused adapter shape: non-dict and
empty dict are unclassifiable; a truthy "error" is a failure and
outranks the boolean flags so a stale printed:True cannot mask it;
status in {failed,error} / {completed,success,ok}; then the per-adapter
flags printed / submitted / executed, each requiring `is True` rather
than truthiness. Anything unrecognised falls through to unclassifiable
rather than defaulting to success.
- build_evidence_bundle emits exactly one outcome event, chosen by that
classification: execution_completed for success, execution_failed
(payload {error, result}) for failure, and execution_unclassified
(payload {reason: "unclassifiable_result", result}) otherwise -- so an
unrecognised result claims neither completion nor failure and fails
closed. The explicit events= override remains a full bypass.
- execute() classifies the same result object and gates the reported
status to match: "completed" only for success, "failed" for a failure,
and "failed" with reason unclassifiable_result otherwise. Evidence is
still pushed for every outcome so a verifier can dispute rather than
seeing nothing at all.
The no_device_found early return and the exception handler already
reported "failed" correctly and are left untouched.
Verified: python -m pytest tests/ -> 338 passed, unchanged from the
pre-change baseline.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012PxrWRej6UAvZepXkpxJPi
test: pin the evidence contract's failure semantics so the emitter cannot regress to reporting a failed run as a completed one. Adds 201 tests to packages/pcc-node/tests/test_job_executor.py, built from fixtures that mirror verbatim what each adapter method actually returns: four success shapes, thirteen failure shapes across all four adapters, and eight unclassifiable shapes. - TestClassifyExecutionResult exercises every rule of the classifier, including the two precedence choices that keep it fail closed: an error key outranks a boolean success flag, and a truthy-but-not-True flag (1, "true", [1]) counts as a failure rather than a success. Also covers the unhashable status value that would otherwise raise, and asserts the classifier does not mutate its input. - TestEvidenceBundleOutcomeEvents asserts the exact event list per verdict, that exactly one outcome event is ever emitted, that job_started survives in all three branches, and that the events= override still bypasses classification entirely. The golden-style check asserts the literal string "execution_completed" appears nowhere in a failure bundle -- the same presence/absence test the settlement oracle's program performs. - TestExecuteStatusGating drives full execute() calls per adapter and asserts the reported job status agrees with the emitted event type, that evidence is still pushed on failure so a dispute is possible, and that an unclassifiable result reports failed with reason unclassifiable_result and never reports completed. Both octoprint and generic-http failure conventions (error key vs boolean False) are covered separately since they exercise different classifier rules. The pre-existing no_device_found and raised-exception paths are pinned as unchanged. These assertions were run against the pre-fix module to confirm they are not tautological: it produced ['job_started', 'execution_completed'] for a failed print, and both acceptance assertions failed there. No existing test was modified. Verified: python -m pytest tests/ -> 539 passed in 16.57s (338 pre-change baseline + 201 new, 0 failed). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012PxrWRej6UAvZepXkpxJPi
…inel fix: an unreachable generic-HTTP device settled as a success. http_util.http returns status_code 0 when a request never completes (connection refused / DNS failure / timeout -- http_util.py:66, docstring :42). _execute_generic_http derived `"executed": status < 400`, so the sentinel read as True: a dead device emitted execution_completed and reported job status "completed", and golden-v4's and(execution_completed present, execution_failed absent) RELEASED the exact failure evidence contract sec-10 exists to dispute. Two changes, defence in depth: - _execute_generic_http: explicit success band `200 <= status < 400`, mirroring _execute_octoprint's allowlist; when status <= 0 the transport error nested under "response" is lifted to a top-level "error" so the failure carries a readable cause. - classify_execution_result: new rule 4, status_code <= 0 -> failure, ranked above the status-string and boolean-flag rules so a claimed success cannot outrank "never reached the device". Any future adapter forwarding the sentinel is fail-closed by default rather than by review. describe_execution_failure gained the matching branch. Why the first census missed it: the fixtures enumerated each adapter's literal RETURN STATEMENTS, not the value domain of the `status` variable feeding them; the 0 sentinel is documented only in http_util, so no hand-copied fixture could contain it. The new tests therefore drive the REAL adapters with only urlopen faked. Verified they fail against the pre-fix source (10 failures, including `assert 'execution_failed' in ['job_started', 'execution_completed']`) and pass after -- source reverted and restored byte-identical by sha256. Blast radius is one adapter, one expression: `status < 400` appears nowhere else in pcc_node. octoprint (`status in (200, 201, 204)`) and opentrons (`status not in (200, 201)`) use allowlists and were already safe; their live-adapter tests passed pre-fix too, which proves that rather than asserting it. ipp is subprocess-based (returncode). pcc-node suite: 573 passed (was 539). Test file: 157 added, 0 deleted -- no pre-existing assertion edited or removed. No crypto, hashing or signing in the diff; no file outside packages/pcc-node changed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012PxrWRej6UAvZepXkpxJPi
…tarts the run
fix: a protocol that never started settled as a success.
_execute_opentrons makes two calls -- POST /runs (create) then
POST /runs/<id>/actions (play) -- and only the play call actually starts the
protocol. Its http() return was DISCARDED (the one discarded http() return in
the module; the other three call sites all bind and check status), and the
adapter then returned {"submitted": True} unconditionally. So an OT-2 that
accepted the run and then refused, or never received, the play emitted
execution_completed with no execution_failed, and golden-v4's
and(execution_completed present, execution_failed absent) RELEASED the exact
failure evidence contract sec-10 exists to dispute.
The fix mirrors the run-creation guard seven lines above: bind the play status
and return a failure dict when it falls outside (200, 201). That dict carries
"error" (classify_execution_result rule 3) AND "submitted": False (rule 7), so
the failure survives either rule being edited, plus runId/protocolId so the
un-played run stays findable. Consistent with commit 9547fdf, which took the
same "an unreachable device is not a successful run" fix for the sibling
_execute_generic_http adapter.
Fail-closed choice: the allowlist is (200, 201), matching run creation and the
OT-2 HTTP API's documented 201 for POST /runs/<id>/actions. A device answering
the play with some other 2xx would be disputed rather than released -- the
recoverable direction.
Why the previous round missed it, and why its test appeared to cover it: with
every socket dead, POST /runs fails first at its own guard, so
test_unreachable_device_disputes_end_to_end[opentrons] returned through run
creation and never entered the play branch -- the name claimed coverage the
assertion did not provide. The new tests keep run creation healthy and break
ONLY the play call, and every one of them asserts the /actions request was
actually issued, so the coverage cannot quietly regress to that shortcut again;
test_dead_socket_stops_at_run_creation_not_the_play_action locks the reason in
place.
Tests: 10 added, pure additions (211 inserted, 0 deleted in the test file).
- Play failure x3 (transport dead / HTTP 500 / HTTP 409), at the adapter and
end to end: execution_failed present, execution_completed absent from both
the event list and the serialized bundle, job status failed, evidence still
pushed so the verifier can dispute.
- Negative controls: play 200 and 201 still submit; a healthy run still emits
execution_completed and reports status completed.
Only urlopen is faked -- http_util, the adapter, the classifier and execute()
all run for real underneath.
Verified rather than assumed: the 6 failure tests FAIL against the pre-fix
source (assert 'execution_failed' in ['job_started', 'execution_completed'])
and pass after; the source was reverted and restored byte-identical by sha256
(3e978e6798cba45c5e983086071f5361f019319550429100a850409925866f77). The
reviewer's own probe, run unmodified, flips from VERDICT: COMPLETED to
VERDICT: FAILED while its classification section stays identical, so the
classifier did not regress.
pcc-node suite: 583 passed in 23.32s (was 573). No crypto, hashing or signing
in the diff; no file outside packages/pcc-node changed.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012PxrWRej6UAvZepXkpxJPi
…before claiming completion
Round 3 guaranteed the contract only for results the adapters label honestly.
Two verifiers showed the adapters do not: a reachable device that reports its
own failure still minted a success flag, and golden-v4's and(execution_completed
present, execution_failed absent) RELEASED it.
Measured before/after through the real adapters (only urlopen faked; http_util,
the adapters, the classifier and execute() all real). BEFORE all seven released;
AFTER none do:
case before -> after
generic 200 + JSON-RPC error RELEASES -> disputes
generic 200 + {status:"error"} RELEASES -> disputes
generic 200 + {success:false} RELEASES -> disputes
generic 200 + SOAP fault body RELEASES -> disputes
generic 304 (inside <400 band) RELEASES -> disputes
octoprint 204 + error body RELEASES -> disputes
opentrons run status=failed RELEASES -> disputes (poll issued: 0 -> 1)
WHAT WAS WRONG
R2 (widest path). _execute_generic_http derived success from the TRANSPORT alone
-- `executed: 200 <= status < 400` -- and lifted a nested error only when
`status <= 0`. It is the catch-all branch of _execute_on_device: modbus, opcua,
http, serial, mdns, camera and unknown all land there, plus anything
_find_device step 4 drops on an unmapped capability. Transport-succeeds /
application-fails is the ordinary instrument failure mode; the repo's own
executor.py GenericHTTPAdapter returns {"status", "result"} precisely because
the BODY carries the outcome. _execute_octoprint had the same defect against
(200, 201, 204).
R1. Opentrons `submitted: True` meant "the play action was accepted", never "the
run finished". Round 3 closed only the case where the play call itself was
rejected; a protocol that started and then failed at step 40 still reported
submitted -> execution_completed -> released.
R4 + hardening. Classifier rule order let a success `status` outrank a False
flag ({"status":"ok","printed":False} -> success); status matching was
case-sensitive ({"status":"Failed","printed":True} -> success); flag checking
was first-key-present-wins ({"printed":True,"executed":False} -> success); and a
failure nested under `response`/`data` was invisible.
WHAT CHANGED (packages/pcc-node only)
Adapters now read the device's answer before minting a flag:
- _extract_device_error() is the single reader of POSITIVE device-failure
signals -- nested error/errors/fault/faultstring, success/ok/succeeded is
False, nested status in the failure set; for non-JSON bodies a tight XML/SOAP
fault-marker list. It reads positive signals only, so it never invents a
failure (negative controls: {"ok":true}, {"errors":[]}, "", "OK", opaque JSON
all still succeed).
- _execute_generic_http: band narrowed to 2xx (a 3xx is a redirect nobody
followed); the device's reason is lifted to the top level at EVERY status;
executed = transport_ok AND no device error.
- _execute_octoprint: same lift against (200, 201, 204).
- _execute_opentrons: new _await_opentrons_run polls GET /runs/<id> to a
terminal state. succeeded -> status "completed"; failed/stopped, or a
populated data.errors, -> status "failed" + error; budget expiry -> status
"running", which classifies as unclassifiable and never releases. Bounded
(120s default, 2s interval) because the daemon runs jobs synchronously;
per-device runPollTimeout / runPollInterval override, 0 disables polling.
KNOWN LIMIT, stated rather than hidden: a protocol longer than the budget
returns non-terminal and so never settles. Raising the budget or adding an
async completion watcher is the fix; releasing on acceptance is not.
Classifier (so a future adapter is fail-closed by construction, not by review):
- COMPLETION flags (printed, executed) split from ACCEPTANCE flags (submitted).
Acceptance True alone is no longer a success -- it emits neither
execution_completed nor execution_failed.
- New order: 3 error, 3b nested device failure, 4 status_code outside 2xx,
4b returncode not 0, 5 failure status, 6 ANY flag present and not True,
7 success status, 8 unrecognised status string -> unclassifiable,
9 completion flag -> success, 10 unclassifiable.
- Rule 6 before rule 7 so a False flag outranks a success status, and it checks
every flag rather than the first found. Rule 8 keeps the device's own word
authoritative, so {"submitted":True,"status":"running"} cannot be voted up by
a flag. Status matching is case-insensitive.
Bundle hashing/signing format and existing event schema names are unchanged;
the events= override still bypasses classification. Gateway and spec packages
untouched.
OUT OF SCOPE, recorded not fixed: the gateway is a second, unguarded producer of
execution_completed -- PUT /api/jobs/:jobId/complete synthesizes it
unconditionally (packages/gateway/src/routes/paid-job-flow.ts:1013) and never
branches on call.status. This commit is pcc-node-only per the task boundary;
that door needs its own change.
TESTS: 452 in test_job_executor.py (up 56), 765 across packages/pcc-node.
New TestDeviceReportedFailureInABody drives 9 device-failure responses through
the real generic-http and octoprint adapters plus 5 negative controls; new
Opentrons cases cover terminal failure, data.errors, non-terminal, unreadable
poll and a disabled budget. Two existing assertions were TIGHTENED, not
weakened: submitted:True alone is now unclassifiable rather than success, and
the OT_SUCCESS fixture carries the polled terminal status; the pre-fix
acceptance-only shape is retained as a locked never-releasing shape.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012PxrWRej6UAvZepXkpxJPi
…never acknowledged `update_job_status` returns False after a bare log.warning when the gateway rejects the PATCH (ws_client.py:183), and all seven call sites in job_executor.py discarded that return. A dropped 'failed' report leaves the job non-terminal upstream, which matters beyond this node: the gateway's own completion route (paid-job-flow.ts) is gated on the job not already being 'failed', so a silently-dropped report re-opens that door. `_report_terminal_failure` now checks the return and logs at ERROR naming the job and the reason when the gateway did not acknowledge it. Both terminal non-success paths (failure and unclassifiable) route through it; the success path is untouched. Stated plainly rather than overclaimed: this makes the drop OBSERVABLE, it does not close it. Closing it needs a retry/outbox here or a change on the gateway side -- neither is in this module's scope, and the gateway is out of scope for this task entirely. Tests: 6 added (458 in test_job_executor.py, 771 across packages/pcc-node) -- both non-success verdicts log the error when the report is dropped, the acknowledged case stays quiet, evidence is still pushed when the report drops, and a successful job is unaffected. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012PxrWRej6UAvZepXkpxJPi
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes memo ownership hole OH-1 (pcc-node emitter, evidence contract sec-10): a device call that returns a failure-shaped result must emit
execution_failedand neverexecution_completed, so the oracle disputes instead of releasing. Ledger: OH-1 / G-2 / LO-EV-1 emitter side. Built by a bounded Ralph loop (4 implementer rounds, in-loop verifier ACCEPT, two adversarial refuters whose HIGH findings were fixed in round 4), branch offlamasu/master7a86491. Scope: exactly two files underpackages/pcc-node/.What changes
job_executor.py: device results are CLASSIFIED before any evidence event is synthesized (_synthesize_eventsis the sole emitter). Success ->execution_completed; failure ->execution_failed; unclassifiable -> NEITHER (job_started only, per evidence sec-10 8.1) and job statusfailed(8.2). Covers every censused return shape of the four adapters that feed the evidence path (IPP print 4, Opentrons 9, OctoPrint 2, generic-HTTP 2), including the pre-fix holes:executed:True + status_code:0(transport sentinel),executed:True + 200 body error,executed:True + 304,printed:True + status_code:0.execute()reportsfailedto the gateway for every failure and unclassifiable case; evidence is still pushed so the verifier can dispute.crypto.py,ws_client.pyabsent from the diff); bundle key set unchanged; tests purely additive (zero deleted test lines).Verification (verifier's own runs, not the implementer's claims)
python -m pytest tests/-> 771 passed (338 before OH-1, 539 / 573 / 583 across rounds).returnstatements: 29 failure ->execution_failedonly; 5 success ->execution_completedonly; 9 unclassifiable -> neither. ALL PASS.execute()probe with a fake gateway: 6 HTTP-level failures + 3 unclassifiable -> statusfailed; the 200/ok case ->completed.Known limits / follow-ups (not in this PR)
packages/adapter-pylabrobot/src/adapter.ts:417,src/evidence.ts:166, andpackages/gateway/src/routes/paid-job-flow.ts:1013emitexecution_completedunconditionally -> own ledger item.statusvalue would skip one rule (no evidence-path adapter emits one).__pycache__/*.pycare tracked on master (pre-existing hygiene).Reviewers: evidence c25c8f97 (contract owner; its round-2 red test on this branch is the acceptance), oracle c158bf91 (golden-v4 receiving side). Steward 57c2a412 graded. Operator merges (no lane merges to master).
🤖 Generated with Claude Code
https://claude.ai/code/session_012PxrWRej6UAvZepXkpxJPi