fix(queue-state): parseable non-dict JSON no longer wedges scans and drains

[beardthelion]

Summary

Fixes #114240.

Every JSON-file scan guards except (OSError, ValueError) — "did it parse?" — but treats parses as is a record. A parseable non-object value (42, "oops", [1,2,3] — corruption, truncated write, foreign tool) passes the guard and crashes the sweep at the first .get()/[...], usually before any healthy sibling runs.

Worst site: cron/bot_chat_pending — one such receipt wedged the deferred Bot Chat drain on every tick (sorted() → TypeError on record["sequence"]), and defer() couldn't allocate a sequence either. Violates the file's documented invariant "one bad file must not wedge the dir" (and its bot_live_delivery precedent, #109820).

Reproduced live; 15 sites audited — all confirmed against their own contracts:

• Scans (_records, _scan_read, recover_pending_to_db, drain_transcript_spool, claim_pending_envelopes, _expire_if_stale, reap_orphaned_lightpanda, list_pending, _legacy_spawn_tree_entry, manifest_verified, load_conversation, batch_runner dataset/resume/combine, process_entry_async): non-dict rejected inside the existing parse guard → identical skip path as unreadable files.
• Exact-id reads (defer, scheduler_delivery pending/receipt checks, deliver_to_live_owner, complete_delivery, _admit_live_dm, _wait_live_dm, get_pending, spawn_tree.load): fail closed with the established different payload ValueError / RPC error / None — never overwrite or reinterpret a malformed file.
• Per-subsystem policy preserved: evidence kept for receipt dirs, atomic quarantine for relay claimed/, delete under existing cleanup policy for reaper state, honest filtered_entries bookkeeping in batch_runner.

Overlap / related work

• fix(gateway): keep one failed flush file from aborting pending-message recovery #87661 — covers recover_pending_to_db's processing boundary (broad except around _recover_one_payload); mine covers the parse boundary. Complementary scopes; happy to rebase whichever lands second.
• fix(gateway): replay the transcript spool in drop order with full fidelity on restart (supersedes #78323) #84785 — carries an equivalent isinstance inside its spool-ordering rewrite; small overlap on one site.
• All other sites: no upstream PR/issue coverage found.

Test plan

• 11 new regression tests across 9 files + tests/tui_gateway/test_spawn_tree_records.py — every site asserts non-dict no longer crashes and healthy siblings still process
• E2E: drain_in_background (real scheduler entry → drain → _drain → _records) delivers the healthy deferred message with a [1,2,3] receipt in the dir; bad file preserved
• ~330 focused tests pass across cron/tools/gateway/tui_gateway/hermes_cli/plugins; ruff clean
• Remaining failures in the local venv are environmental (openai/psutil/fire not installed; a pre-existing test_batch_runner_checkpoint → a2a cross-file pollution reproduced on clean HEAD)

[PRATHAMESH75]

Independent review — this fully addresses #114240, and the coverage is complete. I mapped every source site in the issue's audit to a guard in the diff:

site	guard	matches issue's prescribed contract
cron/bot_chat_delivery.py (_records/defer/_drain)	skip non-dict in scan; re-read guards	✅ preserve + skip, drain no longer wedges
cron/scheduler_delivery.py	pending/receipt non-dict guards	✅
tools/bot_live_delivery.py	scan skip; exact-id read ValueError("… different payload")	✅ fail closed, never overwrite
tools/bot_mode_dm.py	exact-id ValueError("… different payload")	✅
tools/bot_relay.py	ValueError on non-dict envelope, claimed atomically first	✅ can't re-deliver malformed
gateway/shutdown_flush.py (both fns)	payload isinstance guards	✅ incl. the previously-unguarded recover_pending_to_db
tools/browser_lightpanda.py	return data if isinstance(data, dict) else None	✅ reaper cleanup policy
tools/write_approval.py	sort-key/read guards	✅
tui_gateway/methods_session.py	raw/payload isinstance guards	✅ honors SpawnTreeLoadResult
hermes_cli/local_runtime/binaries.py	isinstance(data, dict) and …	✅
plugins/platforms/a2a/protocol.py	if isinstance(entry, dict) filter	✅ keeps list[dict]
batch_runner.py (3 sites)	non-dict entry skips	✅ honest accounting
trajectory_compressor.py	isinstance(entry, dict) or "conversations" not in entry	✅

All 13 files carry a matching test (the defer→drain wedge repro, the pytest.raises(ValueError, match="different payload") fail-closed cases, and the claim_pending_envelopes malformed-envelope case are the load-bearing ones). Nothing from the issue's audit is left out, and the "already-safe" sites it listed are correctly untouched.

One thing worth a maintainer's eye rather than a change request: the guards use two deliberately different shapes — skip-and-preserve for scans vs. raise ValueError for exact-id reads — which is exactly the issue's prescription (a scan must not wedge on one bad sibling; an exact-id read must fail closed rather than silently overwrite). Just flagging that the asymmetry is intentional so it isn't "cleaned up" toward uniformity in review.

LGTM — resolves the P1 drain-wedge and the sibling class in one pass.

[ehz0ah]

Request changes conclusion

GitHub does not permit this account to submit a formal change-request review, so this comment records the same blocking verdict.

Motivation

The reported P1 bug is valid. A parseable JSON scalar can reach object-only queue and state readers, then stop a whole scan before healthy sibling records are processed. The primary deferred Bot Chat reproduction is fixed at exact head d4b54568887e69b3ee3d363ebe4dcd657ccf64f9.

Approach

The patch applies the correct general distinction in most places. Bulk scans skip malformed record shapes. Exact-ID reads fail closed. The batch and JSONL paths reject or pass through non-object entries according to their existing contracts.

Changes reviewed

I reviewed all 24 changed files. The cron queue, scheduler delivery, live-owner mailbox, Bot DM, relay outbox, local runtime manifest, A2A log, batch runner, trajectory compressor, write-approval store, Lightpanda state, shutdown recovery, and spawn-tree snapshot changes are individually small. The exact-ID guards preserve the no-overwrite rule. The relay claim keeps malformed input out of delivery after the atomic rename. The batch combine counter remains accurate.

Risk to main

Two blocking defects remain.

1. tui_gateway/server.py:3079 still appends every parseable _index.jsonl value. tui_gateway/methods_session.py:2180 then calls e.get("path"). A line containing 42 still raises AttributeError and wedges spawn_tree.list. The new spawn-tree tests cover legacy snapshot files and direct load only. Filter non-object index rows at the index reader or before the list comprehension. Add a regression through _index.jsonl.

2. gateway/shutdown_flush.py:152 deletes every non-object pending-*.json before it can establish the record's reason or session. This directory and prefix are shared by shutdown recovery files and transcript spools. Calling drain_transcript_spool() for one session can therefore delete unclassifiable recovery evidence that may belong to another path. I reproduced this with a scalar pending-foreign.json. Recovery correctly preserves the same class of evidence. The transcript drain must also preserve and skip it. Reverse the deletion assertion in the new regression.

The branch also conflicts with current main 64ea66b03d44ead9ffea48161132e5deca5d255a. The conflict in recover_pending_to_db() must retain main's per-file failure isolation and its session_resolver and profile-store routing.

The patch adds 18 tests. Repository guidance limits one fix to one or two invariant tests. Please consolidate the cases around the scan-skip and exact-ID fail-closed contracts after the two correctness gaps are fixed.

Overall assessment

Request changes. The core fix is valid and most changed paths follow the intended policy, but the indexed spawn-tree path remains vulnerable and the transcript path can delete unrelated recovery evidence.

Local validation at the exact head: 304 focused tests passed. Ruff and git diff --check passed. A resolved composition on current main passed 306 focused tests and Ruff. The indexed-path and deletion reproductions still fail as described. No hosted checks are reported for this head.

English verdict: REQUEST_CHANGES at d4b54568887e69b3ee3d363ebe4dcd657ccf64f9. Fix the _index.jsonl non-object path, preserve unclassifiable pending evidence, and rebase without losing current main's recovery routing.

[PRATHAMESH75]

Correcting my earlier "fully-fixes" verdict. @ehz0ah's two blocking points are real — I re-checked both against d4b54568 and upstream/main, and my first pass missed them. Independent confirmation:

1. Indexed spawn-tree path is unguarded. The PR guards _legacy_spawn_tree_entry (non-dict raw → {}) and spawn_tree.load (non-dict payload → 5000), but the indexed path is separate: _read_spawn_tree_index (tui_gateway/server.py:3027) does out.append(json.loads(line)) for every parseable _index.jsonl line, and spawn_tree.list (methods_session.py) consumes it with entries.extend(e for e in indexed if (p := e.get("path")) and Path(p).exists()). A scalar line like 42 survives the reader and raises AttributeError on e.get("path"), wedging spawn_tree.list for the whole session. The added spawn-tree tests exercise legacy snapshots and direct load only, not a poisoned _index.jsonl. Fix belongs in _read_spawn_tree_index (drop non-dict rows) with a regression that routes a scalar through the index file.

2. drain_transcript_spool deletes another path's evidence. The new non-dict guard is inserted before the reason/session_key filter:

if not isinstance(payload, dict):
    logger.warning("Removing structurally invalid transcript spool file %s", path)
    path.unlink(missing_ok=True)
    continue
if (payload.get("reason") != TRANSCRIPT_CAP_DROP_REASON
        or payload.get("session_key") != session_id):
    continue

So it unlinks any non-dict pending-*.json in _get_flush_dir(), regardless of session or reason. But recover_pending_to_db scans the same dir with the wider *.json glob and — in this same PR — deliberately preserves non-dict files ("preserved for manual inspection"). Because drain_transcript_spool(session_X) can run first, it destroys recovery evidence the other function is meant to keep. The two policies on the identical file class contradict each other. Safer: make the transcript-drain guard preserve + skip (don't unlink) and, if a delete is wanted at all, gate it behind the reason == TRANSCRIPT_CAP_DROP_REASON && session_key == session_id check so it only ever touches this session's own spool. The new test_shutdown_flush case should assert the file survives.

Also noting the branch now conflicts with main (recover_pending_to_db() gained per-file isolation + session_resolver/profile routing upstream), and the 18-test count runs past the one-or-two-invariant guidance — both worth folding into the same revision.

The core scan-skip / exact-id-fail-closed design is right and most of the 24 sites are correct; these two gaps are the blockers. Apologies for the premature LGTM — @ehz0ah's read is the accurate one.

[teknium1]

Thanks @beardthelion. Your change was salvaged into #114759 with your authorship preserved (cherry-picked); #114759 — fix(cron): a parseable non-dict receipt no longer wedges the deferred Bot Chat drain or live-delivery mailbox (#114240, salvage #114241) — is now merged on main at 1c121280ac1a, closing issues . Closing this PR in favour of the landed change; if you see a case it does not cover, please open a fresh issue with the repro and tag it.