fix: v0.38.0 — concurrency, API robustness, audit coverage, teaching drift, frontend UX#124
Conversation
…drift, frontend UX
Concurrency: turn_guard.discard() now resolves the handle + clears the
session-active pointer (a failed turn no longer hangs the session's next turn
120s); bus.create() resets a re-created done run so a retry gets a fresh SSE
stream; set_result/fail are session-bound (no cross-session result on a turn_id
collision); a missing client turn_id is synthesized server-side so serialization
always applies.
API layer: _safe_filename maps "."/".."/"" to a safe default (no os.replace onto
a dir → 500); GET /runs/{id}/events 404s an unknown run; a run that fails to
launch after the atomic claim reverts to pending; upload racing a session-delete
cleans up + 409s instead of orphaning a tree; config.scrub_paths collapses the
data/home dirs out of surfaced error text.
Audit (rule 17): dataset import audited at the import; diagnostic report gen
routed through the audited tool path; session report / run create+start /
compare_to_last_survey / list_uploaded_files audited.
Teaching drift (from v0.37 tool changes): test_conditional_get docstring + skill
+ S3 docstring + trace label corrected for the 200-same-ETag = provider-ignored
semantics; list_objects drops the stale 500-cap teaching; lifecycle-cost skill
documents the new list_multipart_uploads prefix; query_account_profile
public_buckets help fixed; skills_used cap tracks _MAX_SKILL_LOADS (20).
Data race: mark_imported guarded by expected stored_path so a re-upload
mid-analysis can't stamp a stale table imported.
Frontend: Stop during a slow tool call waits for the persisted partial (no
vanished turn); a failure while viewing another session keeps the message; steer
preserves the composer draft; session switch no longer flashes prior content;
rename refreshes the header; blocking fallback keeps the stopped marker; reload
race no longer mislabels a healthy session stalled; KiB/MiB labels; model-404
hint only for provider-shaped errors; evidence dialog Escape + busy-guarded ✕;
model-provider blank-name validation.
642 sidecar tests pass (new test_v0380_fixes.py); ruff clean; frontend tsc +
build clean.
Co-Authored-By: Claude Opus 4.8 <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: b39f079e3c
ℹ️ 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".
| run_tool_with_events( | ||
| conn, run_id, "generate_markdown_report", {"run_id": run_id}, | ||
| lambda: {"report_path": config.rel_path( | ||
| write_report(run, evidence, findings, summary)[0]), "format": "markdown"}, |
There was a problem hiding this comment.
Fail the run when diagnostic report generation fails
When write_report raises (for example, because the disk is full or the report directory is unwritable), run_tool_with_events catches the exception and returns a success: false result rather than re-raising it. Because this result is ignored, the body returns its summary and run_executor marks the diagnostic completed and records a report path even though no report exists. Require success here, as the analysis executors do, so report-generation failures make the run fail instead of exposing a broken completed report.
Useful? React with 👍 / 👎.
| "UPDATE session_datasets SET duckdb_path=?, table_name=?, row_count=?, " | ||
| "detected_format=?, status='imported' WHERE id=? AND stored_path=?", | ||
| (duckdb_path_rel, table_name, row_count, detected_format, | ||
| dataset_id, expected_stored_path), |
There was a problem hiding this comment.
Version uploads instead of comparing the stable path
This predicate does not detect the concurrent re-upload it is intended to guard: uploading the same filename in the same session always writes to the same sessions/{session_id}/raw/{filename} path, and upsert reuses that exact stored_path. If a re-upload resets the row after the old file has been imported but before this update, the path still matches, so the stale DuckDB table is marked imported and reused for the new upload. Compare a changing generation/content identifier or give each upload a unique stored path.
Useful? React with 👍 / 👎.
| audit.record(conn, "run.create", | ||
| {"run_id": run_id, "run_type": body.run_type, | ||
| "provider_id": body.provider_id, "bucket": body.bucket}, run_id=run_id) |
There was a problem hiding this comment.
Commit the run creation audit record
audit.record explicitly does not commit, and this handler performs only a read before returning after inserting this new record. The request dependency then closes the connection, rolling the audit insert back, so every successful POST /runs still lacks the promised run.create audit event. Commit after recording it.
Useful? React with 👍 / 👎.
What changed
A four-angle mining round (frontend, router/API + SSE, agent-teaching drift, audit coverage + concurrency) turned up ~28 real items. Every security-floor bound is untouched.
Concurrency
discard()no longer hangs the session's next turn 120 s. A clean failure (fresh-install "no model key") left the session-active pointer on a handle whosedone_eventwas never set;discard()now resolves the handle + clears the pointer. (Independently found by two mining agents; reproduced.)bus.create()resets a previously-doneentry instead of replaying the old failure + instant close.set_result/failare session-bound — aturn_idcollision can't deliver session A's result to session B.turn_idis synthesized server-side so per-session serialization always applies.API layer
./..no longer 500s (sanitizer maps to a safe default).GET /runs/{id}/events404s an unknown run (was 200 + instant "done").pending(was wedgedrunning).config.scrub_pathscollapses the data/home dirs out of surfaced error text (redact_textscrubbed secrets, not paths).Audit (rule 17)
compare_to_last_survey/list_uploaded_filesnow audited.Agent-teaching drift (from the v0.37 tool changes)
test_conditional_getdocstring + data-consistency skill + S3 docstring + trace label all still taught "HTTP 200 → object changed" — but v0.37 made 200-with-same-ETag mean "provider ignored If-None-Match" (unchanged +provider_unsupported). Corrected to key offetag_matches.list_objectsdrops the stale 500-cap teaching; lifecycle-cost skill documents the new requiredprefix;query_account_profilepublic_bucketshelp fixed;skills_usedcap tracks_MAX_SKILL_LOADS(10 → 20).Data race
mark_importedguarded by expectedstored_path— a re-upload mid-analysis can't stamp a stale table as imported.Frontend
Checks run
cd sidecar && pytest -q— 642 passed (newtest_v0380_fixes.py; diagnostic/SSE/cap tests updated where behavior intentionally changed).ruff check app— clean.cd frontend && npx tsc --noEmitandnpm run build— clean.Known gaps / deferred
donegrowth backstop and making the blocking handlerasyncto avoid threadpool occupancy — the latter is a larger structural change.parallel_tool_calls=Falsestays global (real DeepSeek workaround).🤖 Generated with Claude Code
https://claude.ai/code/session_01M3YojFPzjkfMj6YYSwvzdx
Generated by Claude Code