[harness] - close four unhandled-error paths and add first tests for main()'s bind guard - #807
Conversation
… guard - /api/harness/runs: guard the glob->mtime sort against a file deleted mid-listing (same race SessionStore.list() already tolerates) instead of 500ing - ollama: a malformed usage block from an OpenAI-compatible proxy now degrades token tallies to 0 instead of raising an unparseable 500 - main(): a non-numeric CYCLAW_HARNESS_PORT now fails closed like the adjacent range check instead of silently binding the stored port - create_app(): missing static/harness.html raises the typed HarnessConfigError instead of a bare FileNotFoundError - tests: first coverage for main()'s loopback/port guards, plus the race, missing-asset, and malformed-usage regressions Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015VHRCPLFdeWvtp21SKo5QQ
…elper WPS229/WPS210/WPS231/WPS429 flagged the inline try/except in _parse_chat_response. _token_count degrades each malformed usage field to 0 independently, so a valid sibling count now survives (test updated to pin the per-field behavior). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015VHRCPLFdeWvtp21SKo5QQ
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c9cf8be36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| with suppress(OSError): | ||
| json_files.sort(key=os.path.getmtime, reverse=True) |
There was a problem hiding this comment.
Preserve mtime ordering when one run file vanishes
When any accepted artifact disappears or becomes unstatable between the glob and the getmtime key calculation, this suppresses the exception from the whole in-place sort rather than dropping only that path. The route then slices json_files[:_MAX_RUNS] in arbitrary glob order, so in a busy accepted-runs directory the console can omit the newest runs and show stale entries instead of the documented newest-first list; compute mtimes per file, skip failures, and sort the surviving pairs before slicing.
Useful? React with 👍 / 👎.
Proposed changes
A CyClaw-Optimize scan of the harness console surfaced four unhandled-exception paths, each turning into a bare HTTP 500 (or a startup traceback) that
static/harness.html's fetch helper cannot parse — the exact failure class the existing_timeout_errand_agentic_callexcept OSErrorguards were written to eliminate. This PR closes all four and lands the first-ever tests forharness.server.main()'s loopback bind guard./api/harness/runsglob→mtime-sort race (harness/server.py): a run artifact deleted between the glob andos.path.getmtimesort raisedOSErrorstraight out of the route. Now guarded withwith suppress(OSError):— the byte-identical patternSessionStore.list()already carries inharness/sessions.pyfor the same race.usageblock from an OpenAI-compatible proxy (harness/ollama.py):"usage": "none"gaveAttributeError,"prompt_tokens": "abc"gaveValueError— neither wasHarnessLLMError, so/api/chat's 502 handler missed them. Token tallies are cosmetic; they now degrade to 0 and the answer is delivered.CYCLAW_HARNESS_PORTfailed open silently (harness/server.pymain()): a non-numeric override (abc,-1,8790x) was silently discarded and the harness bound the stored port instead of the one the operator asked for. It now fails closed with a clear message, matching the adjacent out-of-rangesys.exit.static/harness.html(harness/server.pycreate_app()): a trimmed checkout (e.g.macos/install-cyclaw.sh's--depth 1clone gone wrong) made startup traceback with a bareFileNotFoundError. It now raises the typedHarnessConfigErrorwith the missing path in details.New
tests/test_harness_robustness.py(14 tests, auto-discovered by pytest — noci.ymlchange needed since no new source module is added): first coverage ofmain()'s non-loopback-host refusal and port-env guards, the runs-listing race, the missing-asset path, and four malformed-usageshapes.Invariant / Governance Impact: none of the 6 invariants touched.
harness/is out-of-band (I6); no import into/out of the core three changed —invariant-guardpasses 33/33. The loopback-only bind guard is strengthened: it gains its first regression tests, and the port override now fails closed instead of open.Types of changes
main()'s loopback/port guards)Scope note: Out-of-band harness layer only (
harness/server.py,harness/ollama.py, one new test file).Benefits / why
main()— the layer the PEP-562__getattr__note identifies as the bind-address guarduvicorn harness.server:appbypasses — was previously entirely untested; the new tests pin it.Risks to monitor
usagedegradation reports 0 tokens instead of erroring — a proxy that consistently sends malformed usage will under-count the console tally silently (debug-level visibility only). Judged acceptable: the alternative loses a good answer over a cosmetic counter.CYCLAW_HARNESS_PORTbeing ignored will now exit at startup with an explicit message. That is the intended behavior change; the message names the variable and value.harness/server.py): merges clean in both directions, both sides' edits survive, harness tests green on the combined tree.Checklist
SECURITY.mdposture (threat model: single-operator, loopback-only)check_invariants.py: 33 passed, 0 failed)GROK_API_KEY=dummy pytest tests/ -q— all pass, incl. 14 new tests);ruff check --select E,F,I,B,C4,UP,Scleanguardedchain, CSRF, TrustedHost, rate limit all unchanged)Further comments
ELI5: four places where the harness could crash with an ugly, unexplained error now either keep working (deleted file mid-listing, weird token counts from the model server) or fail with a clear, typed message (bad port override, missing console HTML). Nothing about who may connect, what routes require auth, or how the graph routes queries changed — the loopback guard just got stricter and got its first tests. Before/after: all 33
invariant-guardchecks pass identically on both sides of the diff.🤖 Generated with Claude Code
https://claude.ai/code/session_015VHRCPLFdeWvtp21SKo5QQ
Generated by Claude Code