feat(decode-svc): pool hardening - deadline/TIMEOUT, PV counter contract, meta clamps (fauxcasa-i92.3.1/.3.2/.3.3) - #111
Merged
Conversation
…r assertions (fauxcasa-i92.3.2)
Codex review PR110 round 5 deferral: plain `!=` on the response id lets
JSON true/1.0 slip past an int expect_id (Python's numeric-tower
equality), and `resp.get("ok") is not True` routes ANY non-True value
("yes", 1, null, []) into the honest-error branch, where a worker could
pair it with an honest-looking error code and escape as a plain
DecodeServiceError -- no counter increment, no kill, even though a
non-bool/non-int-typed field is itself evidence of a malformed/hostile
response (design doc sec 1/2.5: ProtocolViolation = evidence of
compromise, no retry). Both checks are now type-strict before the
value comparison.
Also wires the gate-3 fuzz suite (design doc sec 7 gate 3) to assert
the per-session protocol_violations counter contract directly: every
existing fuzz test expecting ProtocolViolation from _validate_response
now asserts the counter incremented to 1 on a fresh validator, and the
honest-error test asserts it stayed 0 -- makes the counter's semantics
executable, not just documented in comments.
Agent-Signature: claude-fable-5-high on behalf of matt wilkie
Co-Authored-By: Claude <noreply@anthropic.com>
…ry pool (fauxcasa-i92.3.1) WinSandboxWorker.recv_response() previously blocked forever on a stalled or hostile worker with no way for the broker to reclaim control (design doc sec 1: every job carries a deadline). Add an opt-in timeout_ms that arms a threading.Timer hard-killing the child (TerminateProcess, never a polite request) on expiry; the resulting WORKER_CRASHED-from-EOF is re-reported as TIMEOUT since the crash was our own kill, not the worker dying on its own. A ProtocolViolation still propagates unchanged even if the timer fires late -- it's evidence of compromise, not a timing artifact. decode()/probe() default to a 20s deadline and kill+re-raise on TIMEOUT; they do not retry themselves, by design. Add WinDecodePool: the minimal single-slot pool that owns the retry policy the transport itself defers -- TIMEOUT/WORKER_CRASHED get exactly one retry on a fresh worker then become permanent, ProtocolViolation never retries, and the honest worker-reported codes (CORRUPT/UNSUPPORTED/ TOO_LARGE) are permanent for the file without touching the worker. protocol_violations/timeouts/crashes are counted on the POOL rather than the transport so they survive a respawn (the transport's own counter resets to 0 on every fresh worker) -- this is also where fauxcasa-i92.3.2's counter-assertion contract needs to live. Add a test-only `stall` probe (decodesvc_worker_win.py) so the deadline path has a live, entirely non-hostile worker to exercise end to end, alongside a cross-platform fake-child unit test for recv_response's timer and monkeypatch-level pool retry/permanent-failure tests. Co-Authored-By: Claude <noreply@anthropic.com> Agent-Signature: claude-fable-5-high on behalf of matt wilkie
…casa-i92.3.3) Adds parse_meta_fields() (decodesvc_win.py) implementing checklist item 5 (design doc sec 2.4): caption/keywords/faces are size-CLAMPED because a worker can legitimately produce too much of a real thing, while type/shape/finiteness claims (wrong types, NaN/inf, out-of-[0,1] face geometry, non-int rating) are REJECTED as ProtocolViolation because those can only be lies. Lands ahead of the index/poster ops that will actually carry MetaFields in a response, so that seam exists and is fuzz-tested before anything routes through it. WinSandboxWorker.validate_meta() wraps it with the same counting seam as _validate_response (protocol_violations increments on ProtocolViolation, no increment on a clamp), keeping the i92.3.2 per-session counter contract intact for the future caller. Co-Authored-By: Claude <noreply@anthropic.com> Agent-Signature: claude-fable-5-high on behalf of matt wilkie
…i92.3.1/.3.2/.3.3) Nine review-pass fixes to decodesvc_win.py, all on security-boundary code (the Windows sandboxed decode transport), plus matching tests. - P1: parse_meta_fields could raise UnicodeEncodeError uncaught on a lone UTF-16 surrogate in caption/taken/keywords/face-names (legal JSON, illegal Unicode), escaping past the ProtocolViolation counter/kill contract entirely. New _checked_str() helper centralizes the type-reject/size-clamp split for every meta string field, with a new MAX_META_ITEM_BYTES=1024 per-item bound for the fields the design doc doesn't itself cap. - P2: recv_response's deadline timer and a successful read could race -- timer.cancel() was a no-op once the callback had already started, so a response arriving right as the deadline expired could still be handed back from a worker mid-TerminateProcess. New _DeadlineGuard class (lock-serialized, unit-testable without real timers) makes exactly one of "deadline fired" / "read completed" win, and replaces the old instance-level _deadline_hit flag that could leak state across jobs. - P2: WinDecodePool.decode() called _ensure_worker() outside its retry try/except, so a DecodeServiceError raised from spawn() itself (worker died in loader init) bypassed the counter/retry/kill taxonomy entirely. Moved inside the try so spawn failures get identical treatment to job failures. - P2: MetaFields is an all-optional contract, but an explicit JSON null on any field but gps raised ProtocolViolation instead of defaulting -- our own upcoming i92.4 exiv2 wrapper will emit nulls for missing metadata, so this was a guaranteed false positive on the escape-gate counter. Null now normalizes to omitted for every optional field. Also: pool ProtocolViolation branch now closes the dropped worker instead of relying invisibly on the callee's kill path (P3); probe()'s id check uses the same type-strict guard as _validate_response_checks, closing a JSON true==1/1.0==1 gap, without adding counter semantics (P3, deliberate -- probe PV also covers harness-side mistakes); a tautological pool test asserting a list that was already empty regardless of behavior now counts actual _ensure_worker invocations (P3); added an honest-CORRUPT-error pool test proving a healthy worker survives and is reused (P3); decode requests now carry deadline_ms per design doc sec 2.2 (contract drift). QT_QPA_PLATFORM=offscreen uv run apps/desktop-python/test_decodesvc_win.py -q -> 142 passed (was 124; +18 covering every fix above). Agent-Signature: claude-fable-5-high on behalf of matt wilkie Co-Authored-By: Claude <noreply@anthropic.com>
…92.3.1/.3.3) Three security-boundary gaps found by a Codex cross-vendor review pass on this branch's pool-hardening work, all in the trusted-broker side of the Windows sandboxed decode transport: - parse_meta_fields left an exception surface open: a JSON-valid huge integer literal (e.g. 10**400) for a gps coordinate or face x/y/w/h passes the isinstance(v, (int, float)) type check, then raises a bare OverflowError converting to float -- an uncaught, non-ProtocolViolation exception escaping the validator, bypassing the counter/kill contract the same way a lone UTF-16 surrogate did for strings before an earlier fix. A new _checked_finite() helper closes it the same way: reject the type or an unrepresentable/non-finite magnitude as a ProtocolViolation, never let the OverflowError itself propagate. - WinDecodePool.decode() was not safe for concurrent callers: two threads sharing the pool's single worker/single control pipe could interleave requests and responses (one caller's response consumed by the other, producing a spurious id-mismatch ProtocolViolation that kills a healthy worker) or both observe self._worker as None and double-spawn, leaking a worker. Since this pool is deliberately a minimal single-slot stepping stone toward a future multi-worker DecodeService, the correct fix at this layer is whole-job serialization: a lock around decode()'s entire retry loop (and close(), which must not race a live job's respawn). Concurrency lanes remain the future DecodeService's job, not this class's -- documented in the class docstring. - A worker whose process died while idle (OOM-killed by the job object, crashed between jobs) hit two un-taxonomied failure modes: _duplicate_into_child raised a raw OSError (not DecodeServiceError(WORKER_CRASHED)) when handed a dead target process handle, escaping the pool's retry/respawn branch entirely; and _ensure_worker handed the same dead, cached worker back out for the next job without checking liveness. Both are fixed to route through the existing crash taxonomy -- reclassifying only when the worker is confirmed not alive (a genuine CreateFileW/file-open failure stays an ordinary OSError, never misclassified) -- without inflating pool.crashes, which counts failed jobs, not workers found dead between them. Full suite green: 150 passed (142 baseline + 8 new), tail below. Agent-Signature: claude-fable-5-high on behalf of matt wilkie Co-Authored-By: Claude <noreply@anthropic.com>
…ions (fauxcasa-i92.3.3) parse_meta_fields required face.name to be a string, so a face region with the name omitted or explicit JSON null -- the honest common case for detected-but-unidentified faces -- raised ProtocolViolation. Once index() wires validate_meta in, each such photo would increment the compromise counter and kill the worker, and PROTOCOL never retries, so a library of unnamed faces would fail permanently one worker per photo. FIX 4's null->omitted normalization only covers the six top-level keys. Normalize None -> "" inside the face loop, exactly (any other non-str name remains a type lie). Found by post-PR review workflow (19 raw findings, 18 adversarially refuted, this one confirmed + reproduced). Agent-Signature: claude-fable-5-high on behalf of matt wilkie Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HpJMPedvZjVCjZAAKWvxok
Owner
Author
Post-PR review sweepRan a 4-dimension review workflow over this diff (concurrency/deadline correctness, escape-gate bypass from a compromised-worker mindset, test integrity, and an audit that the fixes claimed in the PR body actually landed), with each finding adversarially verified by an independent reviewer before being accepted. 23 agents, 19 raw findings, 18 refuted under verification, 1 confirmed. Confirmed P2 (fixed in eeb94fe): unnamed face regions were protocol violations.
Fix: normalize Audit results worth recording: all seven fixes claimed in the PR body's review trail were verified as actually present at the branch tip, and the fix-audit reviewer could not reconstruct any of the original defects. The refuted findings were mostly deferrals already tracked in fauxcasa-i92.3.4, pre-existing-on-main behavior, or scenarios that turned out to be unreachable on close reading. No remaining concerns from this pass. Merging on green CI per sweep instructions. claude-fable-5-high on behalf of matt wilkie |
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.
Why
Stage B (PR 110) shipped the Windows AppContainer transport with three deliberately deferred hardening gaps, tracked as beads under fauxcasa-i92.3. Until they close, the transport cannot be trusted with a hostile worker: a worker that stalls mid-payload blocks the broker forever, the escape-gate counter contract is only partially wired, and the checklist item-5 metadata clamps do not exist yet. This branch closes all three.
What
fauxcasa-i92.3.1 - per-job deadline + TIMEOUT taxonomy + retry pool
recv_response(timeout_ms): athreading.Timerwatchdog hard-kills the worker on expiry (design sec 1: never a polite request); the resulting pipe EOF is re-reported asTIMEOUT, notWORKER_CRASHED, since the kill was ours. A_DeadlineGuardlock serializes "deadline fired" against "read finished" so a response arriving as the deadline expires is never handed to a trusted caller from a half-killed worker.WinDecodePool: minimal single-slot pool owning the sec-1/2.5 retry policy - TIMEOUT/WORKER_CRASHED retry exactly once on a fresh worker then become permanent; PROTOCOL never retries; honest CORRUPT/UNSUPPORTED/TOO_LARGE leave the worker alive. Whole-job serialization via a lock; per-session counters (protocol_violations/timeouts/crashes) survive respawns.stallprobe in the worker so a live sandboxed timeout is exercised end to end.fauxcasa-i92.3.2 - counter contract strictness
id(rejects JSONtrue/1.0aliasing an int id) andok(non-bool values are now protocol violations instead of slipping into the honest-error branch uncounted) - the two Codex PR110 round-5 deferrals recorded on the bead.protocol_violationscounter on every violation path, and that honest worker errors leave it untouched.fauxcasa-i92.3.3 - checklist item-5 metadata clamps
parse_meta_fields(): the broker-side seam the future index/poster ops must route through. Clamp-vs-reject rule: size claims are clamped (caption <=8 KiB UTF-8-safe truncation, <=64 keywords, <=128 faces, per-item 1 KiB bound), type/shape/finiteness lies areProtocolViolation(non-finite or out-of-[0,1] face geometry, wrong types, lone surrogates, overflow-scale integers). Explicit JSONnull== field omitted, so our own i92.4 exiv2 wrapper can't trip the compromise counter on ordinary files. Routed throughvalidate_meta()so violations count.Review trail
Two independent review passes ran before this PR, both fully reconciled (all findings were new-in-branch, none pre-existing):
UnicodeEncodeError, defeating the counter/kill contract), a P2 timer race (deadline firing after a successful read kills a healthy worker), a P2 spawn-failure bypass of the pool retry policy, and six smaller findings - all fixed in b05b3d2 except the deliberately deferred ones below.codex-agent reviewer --diff --base main): found a P1OverflowErrorescape (math.isfinite(10**400)), a P1 pool concurrency hole, and a P2 dead-idle-workerOSErrorpath bypassing the crash taxonomy - all fixed in 414584e.Deferred with rationale to fauxcasa-i92.3.4: handshake-phase deadline (pre-
lockedreads are still unbounded), background (off-hot-path) respawn, and the probe() violation-counting policy.Validation
test_decodesvc_win.py: 150 passed (was 80 on main; +70 new tests), including live AppContainer spawn/kill/stall tests on Windows 11.scripts/preflight.py: 6/6 gates green in the worktree.🤖 Generated with Claude Code
https://claude.ai/code/session_01HpJMPedvZjVCjZAAKWvxok