Six items from the round-2 adversarial review's "important" list.
N5 — restricted JSON encoder replaces ``default=str``:
``default=str`` silently stringified custom objects, sets, and bytes
to ``"<MyObj at 0x...>"`` — masking worker output bugs. Now uses a
restricted encoder (``tools/controller/_json_safe.safe_json_dumps``)
with an allowlist:
- datetime / date → ISO-8601 string
- Decimal → str (preserves precision)
- UUID → canonical string
- Path → str
- set / frozenset → sorted list (best-effort)
- Everything else → TypeError (a worker output regression surfaces
loudly instead of writing garbage to the DB)
Replaces ``json.dumps(..., default=str)`` at:
- ``worker/runner.py`` (terminal-state UPDATE write)
- ``master/scheduler.py`` (input_payload INSERT + UPDATE)
Tests: ``test_json_safe.py`` (+15 tests) — every allowlisted type +
rejected types (custom class, bytes, complex) + nested structures
+ kwargs forwarding.
N6 — strict parser check now runs BEFORE backfill + loop:
Previously the strict-parser exit could run AFTER backfill (the test
``test_strict_parser_coverage_blocks_startup_with_stubs`` passed
because backfill's ``fail_if_called`` AssertionError was swallowed
by the bare ``except Exception``, then strict exited 2). The test
asserted the right outcome via the wrong path.
Fix:
- ``master/__main__.py``: parser-coverage check moved to immediately
after engine creation, BEFORE backfill + loop. Strict-mode failure
exits 2 without wasting a Forgejo round-trip + without dependent
code paths firing.
- Test refactored: count-based assertions on backfill and loop call
counts (0 each) instead of fail_if_called. Catches regressions
where the strict check moves back below either.
N7 — _to_aware_datetime unit-tested in isolation:
Previously exercised only via end-to-end comment-filter test. New
``TestToAwareDatetime`` (+12 tests) covers: None, empty string, Z
suffix, +00:00 offset, microseconds preserved, naive datetime →
UTC, malformed string → None, partial string → None, unsupported
types → None, timezone abbreviations → None, equality across
Z + offset forms (the regression the helper exists to defend).
N8 — runtime=None lazy-import branch tested:
Previously all 24 HTTP factory tests injected a fake runtime; the
production path (``build_callbacks(cfg=None)`` → sys.path injection
+ lazy import of ``tools._claim_runtime``) was untested.
``TestBuildCallbacksDefaultRuntime`` (+2 tests): verifies the import
succeeds + every ForgejoCallbacks attribute is callable; verifies
the import is idempotent (second call doesn't crash on sys.path
re-insert).
N9 — resume event-row emission asserted:
Round-1's batch B added the PAUSE event-row test
(``test_label_removed_emits_event_with_reason``) but not RESUME.
``test_label_restored_emits_event_with_reason`` pins the resume's
controller_events shape (from_state=PAUSED, to_state=<prior>,
reason="opt-in-label-restored", source="reconciliation") so
operators auditing the timeline see both pause + resume.
N10 — concurrent SQLite dequeue test:
The dequeue docstring claims SQLite BEGIN-DEFERRED concurrent
dequeues "retry via busy_timeout (5s)" — but no test verified.
``TestSQLiteConcurrentDequeue::test_two_threads_racing_one_wins``
spawns two threads, both attempt dequeue simultaneously via a
threading.Barrier. Asserts: neither thread raises SQLITE_BUSY-
without-retry, exactly one acquires the attempt, the loser sees
no_pending_eligible (winner committed first).
Total: 691 controller tests pass (+31 net), 0 regressions.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
drew
c4a7f0027d