fix+feat: v0.34.0 — engine correctness, lifecycle robustness, agent de-ossification, model config#120
Conversation
…e-ossification, model config A three-angle audit (agent ossification, DuckDB engine math, state-machine lifecycle) plus the reviewed third-party findings. Bugs that misreported numbers, wedged state forever, or needlessly boxed the agent in. Analysis-engine correctness: - Integer columns stay Int64: status codes no longer render "404.0" and int64 sizes above 2^53 keep precision (a single null coerced the column to DOUBLE). - Access-log error rates divide by parsed requests, not all ingested lines (they were diluted by the unparsed fraction). - Inventory average = total/count (reconciles with the shown pair) and small_object_ratio is over known-size objects. - Age bucketing pins DuckDB TimeZone=UTC (was sidecar-tz-dependent, ±1 bucket). Lifecycle robustness: - Evidence-import post-download persistence is guarded: a DB error there no longer wedges the import in 'importing' forever (it reverts to failed). - The blocking turn path resolves its turn_guard handle on a persist failure (streaming already did) — otherwise every later turn stalled 120s. - The loser of a concurrent import claim fails its orphan pending run. - A report_ready publish failure can't downgrade a completed run to failed; the executor's failure branch won't overwrite a terminal state. Model provider & budget (reviewed findings): - Provider test no longer false-green (404/405 → "reachable, key unverified", not a confident pass) or false-red (empty base_url is valid → OpenAI default). - Tool-output budget is a hard cap: a single oversized output is withheld with a valid JSON envelope instead of blowing past the per-turn budget. De-ossification: - Custom aggregation: 2nd group-by dim (cross-tabs), day/weekday buckets, distinct_ips/keys/p99/min-max metrics, deterministic top-N tiebreaker — all fixed SQL fragments, whitelist+bound-params floor intact. - Operator-declarable max_output_tokens clamps the completion budget (symmetric with context_window) so a lower-cap endpoint doesn't 400. - Agent-memory recall scales with the model window (floored 50); skill-load guard raised (elastic byte budget is the real bound); survey ceiling 500→2000. Frontend: - The 409 blocking-fallback captures its answer baseline BEFORE the turn (not after the 409), closing a race that hung the UI "running" for ~2.5 min. Deferred (low value): OSS5/OSS6 (elastic turn cap + context-echo caps) — the elastic tool-output budget already governs the real concern. Tests: +8 (test_v0340_fixes.py) + updated budget/model/migration tests. Full sidecar suite 593 passed; ruff clean; tsc + frontend build clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3YojFPzjkfMj6YYSwvzdx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fc2e56341
ℹ️ 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".
| except Exception as exc: # noqa: BLE001 - persist failure must not wedge the import | ||
| repo.set_status(conn, import_id, "failed") | ||
| repo.mark_files(conn, import_id, "failed") | ||
| shutil.rmtree(dest_dir, ignore_errors=True) |
There was a problem hiding this comment.
Remove the committed dataset during persist cleanup
If any operation after datasets_repo.create() fails—for example, marking the import files or recording the audit event—the dataset row has already been committed because datasets.create() commits internally. This handler then deletes dest_dir but never removes that dataset row, leaving a failed run with an uploaded dataset whose stored_path no longer exists. Either make the persistence sequence transactional or explicitly delete the committed dataset before removing its file.
Useful? React with 👍 / 👎.
What changed
A three-angle audit (agent ossification, DuckDB engine math, state-machine lifecycle) plus the reviewed third-party findings. Bugs that misreported numbers, wedged state forever, or needlessly boxed the agent in.
🔴 Analysis-engine correctness
Int64. A single unparsed log line (or missing inventory size) coerced the numeric column to float64 → DuckDB DOUBLE, so reports showed status codes as404.0and sizes above 2^53 lost precision. Fixed at ingest.error_rate_4xx/5xx+ the 206/404/403 shares divided by every ingested line (incl. null-status text-fallback rows), diluting them by the unparsed fraction. Now divide by parsed requests.average_object_size = total_size / object_count;small_object_ratiois over known-size objects.TimeZone='UTC'sodatediffvscurrent_timestampis tz-independent (was ±1 bucket off-UTC).🟠 Lifecycle robustness
importingforever (post-download persistence was outside any try/except and had no reconciler).turn_guardhandle on a persist failure, like the streaming worker — otherwise every later turn in the session stalled 120s.pendingrun.report_readypublish failure can't downgrade a completed run tofailed, and the executor's failure branch won't overwrite a terminal state.🟡 Model provider & budget (reviewed findings)
/models→ "reachable, key unverified" caution (not a confident pass); emptybase_urlis valid (OpenAI default), not "incomplete".🟢 De-ossification (don't box the agent in)
day/weekdaybuckets,distinct_ips/distinct_keys/p99/min-max-bytes metrics, deterministic top-N tiebreaker — all fixed SQL fragments, zero raw-row exposure.max_output_tokensclamps the completion budget (symmetric withcontext_window) so a lower-cap third-party endpoint doesn't 400. Migration 019 + full plumbing + a provider-form field.Frontend
What checks were run
cd sidecar && pytest -q— 593 passed (+8 intests/test_v0340_fixes.py; updated the budget/model/migration tests).ruff check app— clean.cd frontend && npm run typecheck && npm run build— clean.Known gaps / deferred
Security invariants
No new writes, no shell, no destructive S3 op. The aggregate de-ossification only widened the fixed-fragment vocabulary — identifiers still resolve from the whitelist and all values stay bound params (no raw rows, no arbitrary SQL). Audit still records the actual SQL. Secrets handling unchanged.
🤖 Generated with Claude Code
https://claude.ai/code/session_01M3YojFPzjkfMj6YYSwvzdx
Generated by Claude Code