diff --git a/CHANGELOG.md b/CHANGELOG.md index a294029..7dc4b37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,95 @@ follow semantic versioning once it reaches 1.0. ## [Unreleased] +## [0.37.0] - 2026-07-17 + +_Four-angle audit batch: crash-recovery completeness, agent enumeration truth, +provider compatibility, engine correctness, de-ossification, and redaction +depth. Every security-floor bound is untouched (one is tightened)._ + +### Fixed + +- **An evidence import interrupted by a crash no longer wedges forever.** The + download runs in-process, so a hard kill mid-download left the row `importing` + — a state that could never get back to `confirmed` (re-run) or `planned` + (re-confirm). Startup reconciliation now fails orphaned `importing` imports + (and their files), mirroring what it already did for runs. +- **`list_objects` no longer makes keys 501–1000 of a page unreachable.** The + per-call echo cap (500) sat below the S3 page size (1000) while `next_token` + advanced past the whole page — so any enumeration with `max_keys > 500` + silently lost the tail keys with no way to page back to them. The echo cap now + equals the page cap (a full 1000-key page is ~50 KB, inside the elastic + tool-output budget that still backstops it). +- **The final answer is never truncated silently.** The one unmarked cut in the + codebase: the answer contract was hard-sliced at 48 000 chars with no marker — + in post-processing that promises "write out EVERY item". The cap is now + model-elastic (≥ 4 chars/token of the completion budget, so it can never cut + an answer the model was allowed to emit) and, when hit, appends an explicit + `[TRUNCATED …]` marker. +- **`test_conditional_get` no longer misreports "object changed" on providers + that ignore `If-None-Match`.** Many S3-compatible providers return `200` + (ignoring the conditional header) instead of `304`; the tool mapped any `200` + to `etag_matches: false` even with an identical ETag. It now compares + quote-normalized ETags: equal on `200` → "unchanged + conditional requests + unsupported" (rule 18), different → genuinely changed. +- **`list_multipart_uploads` works on prefix-scoped providers.** The wrapper + exposed no `prefix`, so any provider with `allowed_prefixes` always denied the + root listing, making the abandoned-upload cost diagnostic unreachable there. + It now takes a `prefix`, passed to both the scope check and the S3 `Prefix=`. +- **`get_object_lock_status` no longer reads a malformed call as "no lock".** + The broad `InvalidRequest` code was blanket-mapped to "none"; only its + object-lock flavor ("Bucket is missing Object Lock Configuration") means that. + Other `InvalidRequest`s now surface as errors instead of "cleanly deletable". +- **Inventory engine:** no more `Storage-class skew: 'None' covers 100%` finding + when the inventory simply has no storage_class column (the same null-group + guard hot-key/hot-prefix already had); `average_object_size` uses floor + division so multi-PB totals keep int64 precision above 2^53. +- **DuckDB layer:** a read-only open of a missing analytical DB is a clean + "nothing imported yet" error instead of creating a stray empty `.duckdb` via a + writable fallback; writer-side lock contention now gets the same friendly + retryable message readers already had. +- **`session_datasets` dedupe matches NULL filenames** (`IS`, not `=`), so a + re-uploaded nameless file can't create two rows pointing at one on-disk path. + +### Security + +- **A pasted bare AWS access-key/secret-key PAIR is now fully scrubbed.** The + secret-key redaction rule is label-anchored (so bucket/object names aren't + blanket-mangled), which let a bare 40-char SK pasted alongside its `AKIA…` key + id survive redaction, be persisted, and re-enter the next turn's prompt. A + narrow rule now masks bare 40-char base64 tokens ONLY when the text also + carries an AWS access-key-ID shape — ordinary 40-char strings without that + hint remain untouched. +- **`session_messages` JSON columns (`tool_activity`, `grounding`, + `proposed_actions`) pass through `redact()` at the persistence boundary**, + like every sibling repository — defense in depth for rule 14; the agent + runtime still sanitizes upstream. + +### Changed (de-ossification — no security-floor change) + +- **The completion budget's only upper bound is the model's real provider + max-output.** The module-wide 32 768 ceiling is gone: the per-model clamp + already existed, so the ceiling only ever bit models whose real output cap is + higher (claude-3-7 / gemini-2.5 at 64k, o-series at 100k) — starving long + enumerations on exactly the models that could hold them. +- **The survey/config-review summary echoed to the agent scales with the model + window** (floor 2000 chars, ceiling 16k) instead of a flat 2000. +- **The deterministic session summary scales too:** the persisted store holds up + to 200 facts/findings (was 50) and the context echo is model-elastic (floor + 50), matching the agent-memory de-ossification; the human-readable digest + stays at 50 entries with an explicit "+N more" note. +- **Size labels are binary to match the binary math** (KiB/MiB/GiB, thresholds + like `<4KiB`/`128KiB-1MiB`): the divisors were always 1024-based; only the + labels said KB/MB. +- Docs: note that gated larger context windows (e.g. Claude 1M beta) should be + declared via the model provider's explicit `context_window` override. + +### Tests + +- 20 new regression tests (`test_v0370_fixes.py`) covering every item above; + existing tests updated where behavior intentionally changed (full-page echo, + provider-cap-only completion budget, object-lock message flavor). + ## [0.36.0] - 2026-07-17 _Migration crash-recovery: a partially-applied table-rebuild no longer wedges the diff --git a/docs/tools.md b/docs/tools.md index 704f80c..9a92f60 100644 --- a/docs/tools.md +++ b/docs/tools.md @@ -372,7 +372,11 @@ chars as a **hard floor**. A 128k/200k-context model is unchanged; a 1M-context model gets a proportionally deeper turn. The window comes from a built-in model→window table, but a model provider can carry an explicit `context_window` (tokens) that overrides it — so a newly-shipped large-context model isn't -throttled to the default. The same window also scales the thread-replay caps +throttled to the default. The table is deliberately conservative where a +family's real window is gated: e.g. Claude models map to 200k (the GA default) +even though some expose a 1M window behind a beta/tier opt-in — if your account +has the larger window enabled, declare it via `context_window` on the model +provider rather than expecting the table to assume it. The same window also scales the thread-replay caps (how many prior messages / chars the agent re-sees), floored at the historical values and capped. The completion (`max_tokens`) budget is additionally clamped to each model's real provider max-output, so we never send a value the provider diff --git a/sidecar/app/agent_runtime/model_budget.py b/sidecar/app/agent_runtime/model_budget.py index dffa453..28d3420 100644 --- a/sidecar/app/agent_runtime/model_budget.py +++ b/sidecar/app/agent_runtime/model_budget.py @@ -69,7 +69,6 @@ # window's fair share — no shipping model exceeds that usefully today). TOOL_OUTPUT_CHARS_CEILING = 2_000_000 COMPLETION_TOKENS_FLOOR = 16_384 # was session_agent._MAX_COMPLETION_TOKENS -COMPLETION_TOKENS_CEILING = 32_768 # stay under provider max-output caps def context_window(model: str | None, explicit: int | None = None) -> int: @@ -116,13 +115,19 @@ def tool_output_char_budget(model: str | None, explicit_window: int | None = Non def completion_token_budget(model: str | None, explicit_window: int | None = None, explicit_max: int | None = None) -> int: """Completion (max_tokens) budget: raised only where the window clearly - supports it, floored at the historical value, capped by the module ceiling AND - by the model's real provider max-output so we never trigger a 400. + supports it, floored at the historical value, and capped by the model's real + provider max-output so we never trigger a 400. + + The per-model max-output clamp is the SOLE upper bound — there is no second + module-wide ceiling. (There was: 32_768, and since the provider clamp already + existed it only ever bit DOWNWARD on models whose real output ceiling is + higher — claude-3-7 / gemini-2.5 at 64k, o-series at 100k — starving long + enumeration answers on exactly the models that could hold them, worst on + reasoning models whose thinking spends part of the same budget.) The provider cap is applied only when it's *below* the floor for a genuinely small-output model — the floor otherwise wins (an existing deployment is unchanged), but a 4k-output model like gpt-4-turbo is clamped down to 4096 rather than being handed the 16384 floor it would reject.""" - scaled = max(COMPLETION_TOKENS_FLOOR, - min(context_window(model, explicit_window) // 8, COMPLETION_TOKENS_CEILING)) + scaled = max(COMPLETION_TOKENS_FLOOR, context_window(model, explicit_window) // 8) return min(scaled, max_output_tokens(model, explicit_max)) diff --git a/sidecar/app/agent_runtime/session_action_tools.py b/sidecar/app/agent_runtime/session_action_tools.py index b950283..5a5960b 100644 --- a/sidecar/app/agent_runtime/session_action_tools.py +++ b/sidecar/app/agent_runtime/session_action_tools.py @@ -40,7 +40,13 @@ "account_discovery": "Discover account-level buckets and evidence sources.", } +# FLOOR on the survey/review final_summary echoed to the model, scaled with the +# model window in build() (same de-ossification as agent memory / thread replay): +# the summary is an already-sanitized deterministic aggregate — no raw rows — so +# clipping it to a flat 2000 chars on a large-window model just made the agent +# narrate a large account from a truncated summary. _MAX_SUMMARY = 2000 +_MAX_SUMMARY_CEIL = 16000 def _err(msg: str) -> str: @@ -110,7 +116,8 @@ def _go() -> None: return run_id -def _run_result(conn: sqlite3.Connection, run_id: str) -> dict[str, Any]: +def _run_result(conn: sqlite3.Connection, run_id: str, + summary_cap: int = _MAX_SUMMARY) -> dict[str, Any]: row = runs_repo.get_row(conn, run_id) if row is None: return {"run_id": run_id, "status": "unknown"} @@ -118,7 +125,7 @@ def _run_result(conn: sqlite3.Connection, run_id: str) -> dict[str, Any]: result: dict[str, Any] = { "run_id": run_id, "status": row["status"], - "final_summary": redact_text(str(summary))[:_MAX_SUMMARY], + "final_summary": redact_text(str(summary))[:summary_cap], } if row["status"] in ("pending", "running"): # Hit the wall-clock timeout: the run is still going in the background. @@ -140,6 +147,8 @@ def build( session_id: str | None = None, turn_id: str | None = None, cancel_event: Any = None, + model: str | None = None, + explicit_window: int | None = None, ) -> list[Any]: """Build the agent's read-only survey/review tools (always available). @@ -154,6 +163,13 @@ def build( ``cancel_event`` lets the 180 s inline-run wait return early when the user stops the turn. """ + from . import model_budget + + # Elastic summary echo: floor at _MAX_SUMMARY (128k/200k windows unchanged), + # scaled with the window like agent memory / thread replay. + window = model_budget.context_window(model, explicit_window) + summary_cap = min(_MAX_SUMMARY_CEIL, _MAX_SUMMARY * max(1, window // 128_000)) + def provider(provider_id: str): return cloud_repo.get(conn, provider_id) @@ -196,7 +212,7 @@ def review_bucket_config(provider_id: str, bucket: str) -> str: user_prompt=_DEFAULT_PROMPTS["bucket_config_review"], session_id=session_id) run_id = _execute_run(conn, body, turn_id, f"bucket_config_review:{provider_id}:{bucket}", cancel_event=cancel_event) - result = _run_result(conn, run_id) + result = _run_result(conn, run_id, summary_cap) except Exception as exc: # noqa: BLE001 — a tool returns an error string, never raises return _err(f"review_bucket_config failed: {exc}") note("review_bucket_config", bucket, result["status"]) @@ -219,7 +235,7 @@ def survey_account(provider_id: str, max_buckets: int = 0) -> str: session_id=session_id, max_buckets=mb) run_id = _execute_run(conn, body, turn_id, f"account_discovery:{provider_id}", cancel_event=cancel_event) - result = _run_result(conn, run_id) + result = _run_result(conn, run_id, summary_cap) profile = account_repo.get_profile(conn, run_id) except Exception as exc: # noqa: BLE001 — a tool returns an error string, never raises return _err(f"survey_account failed: {exc}") @@ -244,7 +260,7 @@ def read_run_result(run_id: str, wait_seconds: int = 0) -> str: linked = {r["run_id"] for r in sessions_repo.list_runs(conn, session_id)} if session_id else set() if run_id not in linked: return _err("Unknown run_id for this session. Only runs in this session can be read.") - result = _run_result(conn, run_id) + result = _run_result(conn, run_id, summary_cap) # Bounded in-turn wait: poll until the run leaves pending/running or the # budget elapses. This whole turn already runs on a dedicated worker # thread (boto3 tools block it by design), so sleeping here stalls only @@ -255,7 +271,7 @@ def read_run_result(run_id: str, wait_seconds: int = 0) -> str: break # user stopped the turn — stop waiting on the background run _time.sleep(1.0) conn.commit() # end the read snapshot so run_sync's writes are visible - result = _run_result(conn, run_id) + result = _run_result(conn, run_id, summary_cap) audit.record(conn, "session.read_run_result", {"session_id": session_id, "run_id": run_id, "status": result["status"]}, run_id=run_id) diff --git a/sidecar/app/agent_runtime/session_agent.py b/sidecar/app/agent_runtime/session_agent.py index 4db086a..e2c988e 100644 --- a/sidecar/app/agent_runtime/session_agent.py +++ b/sidecar/app/agent_runtime/session_agent.py @@ -40,8 +40,12 @@ from .agent_service import AgentUnavailable from .guardrails import strip_chain_of_thought, strip_chain_of_thought_stream -_MAX_FACTS = 50 # kept in sync with sessions.summary_builder.MAX_FACTS -_MAX_FINDINGS = 50 # kept in sync with sessions.summary_builder.MAX_FINDINGS (was 30 — drift) +# FLOORS for the deterministic-summary items shown to the model; the effective +# cap is _elastic_memory_cap (scales with the window, ceiling _MEM_RECALL_CEIL). +# The PERSISTED summary holds up to summary_builder.MAX_FACTS/MAX_FINDINGS (200), +# so the elastic cap has something to reveal on a large-window model. +_MAX_FACTS = 50 +_MAX_FINDINGS = 50 _MEM_RECALL_CEIL = 400 # upper bound on the elastic agent-memory recall @@ -71,18 +75,20 @@ def _elastic_memory_cap(model: str | None, explicit_window: int | None) -> int: # the (now elastic) per-turn budget. Defined AS _MAX_TURNS (below) so the two # can't drift apart again (they did: 40 vs 60 after v0.27.0). _MAX_REPLAY_TOOLS = 60 # == _MAX_TURNS; assert below keeps them in lockstep -# Enumerations can be large (e.g. 96+ buckets in a table). Keep our answer cap -# well above any single model completion so we never truncate a legitimate full -# answer in post-processing. +# Enumerations can be large (e.g. 96+ buckets in a table). FLOOR of the answer +# cap, not the cap itself: _answer_cap() scales it to ≥ ~4 chars/token of the +# model's completion budget, so post-processing can never truncate an answer the +# completion budget legitimately allowed — and when the cap IS hit, the cut is +# marked (_ANSWER_CUT_MARKER), never silent (same rule as every other budget). _MAX_OUTPUT = 48000 # Without an explicit max_tokens the provider applies a small default; for a # reasoning model (e.g. deepseek-v4-pro) the thinking budget then leaves almost -# nothing for the answer, truncating long enumerations mid-table. The budget -# must comfortably cover the post-processing answer cap (_MAX_OUTPUT ≈ 12k -# tokens), otherwise the prompt mandates full enumeration the completion budget -# can't hold. (The installed Agents SDK's chat-completions streaming path does -# not surface finish_reason, so a provider-side length cut cannot be detected -# here — the generous budget is the mitigation.) +# nothing for the answer, truncating long enumerations mid-table. This floor +# (16384 tokens ≈ 65k chars) comfortably covers the _MAX_OUTPUT answer floor, +# so the prompt's full-enumeration mandate always fits the completion budget. +# (The installed Agents SDK's chat-completions streaming path does not surface +# finish_reason, so a provider-side length cut cannot be detected here — the +# generous budget is the mitigation.) _MAX_COMPLETION_TOKENS = 16384 # A real investigation chains several probes (test_credentials → head_bucket → # test_addressing_style → list_objects → head_object …); keep a generous but @@ -334,8 +340,12 @@ def build_session_context( ) -> dict[str, Any]: """Bounded, redacted context — the ONLY thing the model sees.""" max_messages, max_replay_msg = _elastic_replay_caps(model, explicit_window) + # The deterministic grounding summary scales with the window too (floored at + # the historical 50) — agent_memory already went elastic; leaving these flat + # clipped exactly the grounding a large-context model needs on big sessions. + summary_cap = _elastic_memory_cap(model, explicit_window) findings = [] - for f in (summary.get("findings") or [])[:_MAX_FINDINGS]: + for f in (summary.get("findings") or [])[:summary_cap]: findings.append({ "severity": str(f.get("severity", "info"))[:32], "confidence": str(f.get("confidence", "medium"))[:16], @@ -354,13 +364,13 @@ def build_session_context( {"text": redact_text(str(f.get("text", "")))[:300], "confidence": f.get("confidence", "medium"), "source_run_id": str(f.get("source_run_id") or "")[:64]} - for f in (summary.get("known_facts") or [])[:_MAX_FACTS] + for f in (summary.get("known_facts") or [])[:summary_cap] ], "findings": findings, - "open_questions": [redact_text(str(q))[:300] for q in (summary.get("open_questions") or [])[:_MAX_FACTS]], + "open_questions": [redact_text(str(q))[:300] for q in (summary.get("open_questions") or [])[:summary_cap]], # NOTE: the deterministic rule-engine "next_actions" menu is intentionally # NOT injected — the agent proposes its own next steps. (Removed in v0.20.) - "limitations": [redact_text(str(x))[:300] for x in (summary.get("limitations") or [])[:_MAX_FACTS]], + "limitations": [redact_text(str(x))[:300] for x in (summary.get("limitations") or [])[:summary_cap]], }, # Things YOU recorded in earlier turns of this session (via note_fact / # record_finding / note_open_question). Reuse them; don't re-derive. @@ -535,13 +545,15 @@ def _finalize_agent_and_prompt(creds: dict[str, Any], prompt: str, def _build_tools(conn: Any, function_tool: Callable, activity: list[dict[str, Any]] | None, session_id: str | None, turn_id: str | None = None, - cancel_event: Any = None) -> list[Any]: + cancel_event: Any = None, model: str | None = None, + explicit_window: int | None = None) -> list[Any]: """The agent's full read-only toolset (no autonomy toggle — always available).""" if conn is None: return [] tools = session_tools.build(conn, function_tool, activity) tools += session_action_tools.build(conn, function_tool, activity, session_id, turn_id, - cancel_event=cancel_event) + cancel_event=cancel_event, model=model, + explicit_window=explicit_window) # Working-memory tools are always available (recording is cloud-read-only). tools += session_memory_tools.build(conn, function_tool, session_id, activity) # Uploaded-file analysis is always available (local, read-only, sanitized) so @@ -722,9 +734,28 @@ def _with_continue_proposal(contract: dict[str, Any]) -> dict[str, Any]: return contract -def _finalize_contract(raw: Any, skill_names: list[str], activity: list[dict[str, Any]]) -> dict[str, Any]: - contract = skill_contract.parse_agent_contract(raw, allowed_skill_names=skill_names) - contract["answer"] = contract["answer"][:_MAX_OUTPUT] +_ANSWER_CUT_MARKER = skill_contract.ANSWER_CUT_MARKER + + +def _answer_cap(creds: dict[str, Any] | None) -> int: + """Elastic post-processing cap on the final answer. + + Never below the _MAX_OUTPUT floor, and always ≥ ~4 chars/token of the model's + completion budget — so this cap can never cut an answer the completion budget + legitimately allowed the model to emit (it only backstops pathological output). + """ + if not creds: + return _MAX_OUTPUT + return max(_MAX_OUTPUT, 4 * model_budget.completion_token_budget( + creds.get("model"), creds.get("context_window"), creds.get("max_output_tokens"))) + + +def _finalize_contract(raw: Any, skill_names: list[str], activity: list[dict[str, Any]], + cap: int | None = None) -> dict[str, Any]: + # The answer cap is applied INSIDE parse_agent_contract (it owns the only + # slice), model-elastic via ``cap`` and never silent — the cut is marked. + contract = skill_contract.parse_agent_contract(raw, allowed_skill_names=skill_names, + max_answer=cap or _MAX_OUTPUT) # Bind skills_used to skills the agent ACTUALLY loaded via read_skill this # turn — the model can't merely *claim* a skill it never opened (keeps the # report honest). read_skill records {tool, target=skill_name} in activity. @@ -764,7 +795,8 @@ def _start_streamed_run(spec: dict[str, Any], clients: list[Any] | None = None): clients = [] tools = _build_tools(spec.get("conn"), function_tool, activity, spec.get("session_id"), spec.get("turn_id"), - spec.get("cancel_event")) + spec.get("cancel_event"), model=creds.get("model"), + explicit_window=creds.get("context_window")) budget = _install_tool_output_budget(tools, model=creds.get("model"), explicit_window=creds.get("context_window"), cancel_event=spec.get("cancel_event")) @@ -816,7 +848,8 @@ async def _drive() -> dict[str, Any]: async for kind, data in stream_events_for( result, spec["activity"], spec.get("skill_names") or [], finalize, cancel_event=spec.get("cancel_event"), clients=clients, - budget=spec.get("budget")): + budget=spec.get("budget"), + answer_cap=_answer_cap(spec.get("creds"))): if kind == "final": final = data return final @@ -866,7 +899,7 @@ def answer( raw = SESSION_LOOP(spec) if isinstance(raw, dict): # the real (streamed) loop returns the contract return raw - return _finalize_contract(raw, skill_names, activity) + return _finalize_contract(raw, skill_names, activity, cap=_answer_cap(creds)) # --- Streaming path (SDK-only; used by the SSE endpoint) -------------------- @@ -981,7 +1014,8 @@ async def _close_clients(clients: list[Any] | None) -> None: async def stream_events_for(result: Any, activity: list[dict[str, Any]], skill_names: list[str], finalize=None, *, cancel_event: Any = None, clients: list[Any] | None = None, - budget: dict[str, Any] | None = None): + budget: dict[str, Any] | None = None, + answer_cap: int | None = None): """Yield ('delta', text) and ('tool', record) during the run, then ('final', contract) when complete. @@ -1015,7 +1049,7 @@ async def stream_events_for(result: Any, activity: list[dict[str, Any]], skill_n partial = _hold_back_contract( strip_chain_of_thought(redact_text(raw_acc))).strip() answer_text = (partial + "\n\n" if partial else "") + _STOPPED_MARKER - contract = _finalize_contract(answer_text, skill_names, activity) + contract = _finalize_contract(answer_text, skill_names, activity, cap=answer_cap) contract["stopped"] = True yield ("final", contract) return @@ -1062,7 +1096,7 @@ async def stream_events_for(result: Any, activity: list[dict[str, Any]], skill_n # Cut short by the step ceiling or the context window → offer a # one-click "continue investigation" next step. yield ("final", _with_continue_proposal( - _finalize_contract(text, skill_names, activity))) + _finalize_contract(text, skill_names, activity, cap=answer_cap))) return while len(activity) > emitted_tools: yield ("tool", activity[emitted_tools]) @@ -1081,9 +1115,9 @@ async def stream_events_for(result: Any, activity: list[dict[str, Any]], skill_n if _BUDGET_CUT_MARKER not in final_text: final_text = (final_text + "\n\n" + _BUDGET_CUT_MARKER).strip() yield ("final", _with_continue_proposal( - _finalize_contract(final_text, skill_names, activity))) + _finalize_contract(final_text, skill_names, activity, cap=answer_cap))) else: - yield ("final", _finalize_contract(final_text, skill_names, activity)) + yield ("final", _finalize_contract(final_text, skill_names, activity, cap=answer_cap)) finally: await _close_clients(clients) diff --git a/sidecar/app/agent_runtime/session_tools.py b/sidecar/app/agent_runtime/session_tools.py index 426731d..8b3ae7a 100644 --- a/sidecar/app/agent_runtime/session_tools.py +++ b/sidecar/app/agent_runtime/session_tools.py @@ -30,12 +30,14 @@ from ..security.redaction import redact_text from . import guardrails -# Max object keys echoed to the model per list_objects call. The bucket may hold -# far more; the agent pages via next_token for the rest. 500 (was 200): the S3 -# layer already caps a page at 1000, and a fuller echo lets an enumeration finish -# in fewer turns; keys_truncated_in_context still tells the agent when the echo -# was cut (never a silent cap). -_LIST_KEYS_CTX_CAP = 500 +# Max object keys echoed to the model per list_objects call — equal to the S3 +# layer's page cap (MAX_LIST_KEYS = 1000), and it MUST stay >= that cap: the S3 +# layer computes next_token over the FULL page, so an echo cap below the page +# size would drop the tail keys with no way to page back to them (paging with +# next_token skips straight past the whole page) — silently incomplete +# enumeration. A full 1000-key page is ~50 KB, comfortably inside the elastic +# tool-output budget, which still backstops pathological key lengths. +_LIST_KEYS_CTX_CAP = s3.MAX_LIST_KEYS def _err(msg: str) -> str: @@ -226,18 +228,20 @@ def list_object_versions(provider_id: str, bucket: str, prefix: str = "", max_ke return json.dumps(res) @function_tool - def list_multipart_uploads(provider_id: str, bucket: str, max_uploads: int = 1000, + def list_multipart_uploads(provider_id: str, bucket: str, prefix: str = "", max_uploads: int = 1000, key_marker: str = "", upload_id_marker: str = "") -> str: - """List one page of in-progress / incomplete multipart uploads (read-only ListMultipartUploads; no bodies). Surfaces abandoned uploads — a common silent cost leak (parts are billed but invisible in a normal object listing). Use for unexplained storage/cost. Returns upload count, oldest initiation time, ≤20 sample keys, and next_key_marker/next_upload_id_marker. When is_truncated is true, page by passing those back as key_marker/upload_id_marker — the count is ONE page, not the bucket total. Listing only — aborting is a mutation and is not available; propose a lifecycle rule instead. Args: provider_id, bucket, max_uploads? (up to 1000), key_marker?, upload_id_marker?.""" + """List one page of in-progress / incomplete multipart uploads (read-only ListMultipartUploads; no bodies). Surfaces abandoned uploads — a common silent cost leak (parts are billed but invisible in a normal object listing). Use for unexplained storage/cost. Returns upload count, oldest initiation time, ≤20 sample keys, and next_key_marker/next_upload_id_marker. When is_truncated is true, page by passing those back as key_marker/upload_id_marker — the count is ONE page, not the bucket total. Listing only — aborting is a mutation and is not available; propose a lifecycle rule instead. Args: provider_id, bucket, prefix? (limit to keys under a prefix — REQUIRED on a prefix-scoped provider), max_uploads? (up to 1000), key_marker?, upload_id_marker?.""" p = provider(provider_id) if p is None: return _err("Unknown provider_id. Call list_providers first.") - denial = scope_denial(p, bucket, listing=True) + denial = scope_denial(p, bucket, prefix=prefix or None, listing=True) if denial: return _err(denial) bound = guardrails.bound_tool_args("list_objects_v2", {"max_keys": max_uploads}) - rec("list_multipart_uploads", provider_id=provider_id, bucket=bucket, max_uploads=bound["max_keys"]) + rec("list_multipart_uploads", provider_id=provider_id, bucket=bucket, + prefix=prefix or None, max_uploads=bound["max_keys"]) res = s3.list_multipart_uploads(conn, provider_id, bucket, bound["max_keys"], + prefix=prefix or None, key_marker=key_marker or None, upload_id_marker=upload_id_marker or None) note("list_multipart_uploads", bucket, res) return json.dumps(res) diff --git a/sidecar/app/analysis/duck.py b/sidecar/app/analysis/duck.py index 3dfc5df..6573c77 100644 --- a/sidecar/app/analysis/duck.py +++ b/sidecar/app/analysis/duck.py @@ -23,23 +23,37 @@ def connect(duckdb_path: str | Path, read_only: bool = False) -> duckdb.DuckDBPy ``read_only=True`` opens the existing DB read-only so a pure-read analysis (analyze/aggregate) can't collide with a concurrent import that DROPs/rebuilds - the same table on another turn's worker thread. Falls back to a normal open if - the file doesn't exist yet (the read-only mode can't create it).""" + the same table on another turn's worker thread. A missing file is a clean + "dataset not imported" error — a read must not leave a stray empty ``.duckdb`` + behind (it used to fall back to a WRITABLE open that created one). + + Lock contention is translated to a friendly, retryable ``ValueError`` on BOTH + open modes: a reader blocked by a writer AND a writer (import) blocked by a + long analyze on another worker thread previously surfaced the raw + ``IOException`` from the writer side.""" path = Path(duckdb_path) if read_only: - if path.exists(): - try: - return _configure(duckdb.connect(str(path), read_only=True)) - except duckdb.IOException as exc: - # A concurrent import holds the write lock. Surface a friendly, - # actionable message instead of the raw lock IOException. - if "lock" in str(exc).lower(): - raise ValueError( - "This dataset is busy (an import or rebuild is writing to it " - "right now). Retry in a moment, once the import finishes." - ) from exc - raise - # No DB yet — nothing to read; open writable so the caller gets an empty DB - # rather than a hard error. + if not path.exists(): + raise ValueError( + "This dataset has no analytical database yet (nothing was imported, " + "or the import did not complete). Import the data first." + ) + try: + return _configure(duckdb.connect(str(path), read_only=True)) + except duckdb.IOException as exc: + if "lock" in str(exc).lower(): + raise ValueError( + "This dataset is busy (an import or rebuild is writing to it " + "right now). Retry in a moment, once the import finishes." + ) from exc + raise path.parent.mkdir(parents=True, exist_ok=True) - return _configure(duckdb.connect(str(path))) + try: + return _configure(duckdb.connect(str(path))) + except duckdb.IOException as exc: + if "lock" in str(exc).lower(): + raise ValueError( + "This dataset is busy (another analysis or import holds it right " + "now). Retry in a moment." + ) from exc + raise diff --git a/sidecar/app/analysis/inventory.py b/sidecar/app/analysis/inventory.py index 9935dd3..5e9b417 100644 --- a/sidecar/app/analysis/inventory.py +++ b/sidecar/app/analysis/inventory.py @@ -273,15 +273,17 @@ def series_for(field: str) -> pd.Series: # --- analyze_inventory ------------------------------------------------------ +# Thresholds are binary (4096 = 4 KiB, 1048576 = 1 MiB, …) so the labels say +# KiB/MiB — they used to read KB/MB, mislabeling the buckets under SI units. _SIZE_CASE = f""" CASE WHEN size IS NULL THEN 'unknown' - WHEN size < 4096 THEN '<4KB' - WHEN size < 131072 THEN '4KB-128KB' - WHEN size < 1048576 THEN '128KB-1MB' - WHEN size < 67108864 THEN '1MB-64MB' - WHEN size < 536870912 THEN '64MB-512MB' - ELSE '512MB+' + WHEN size < 4096 THEN '<4KiB' + WHEN size < 131072 THEN '4KiB-128KiB' + WHEN size < 1048576 THEN '128KiB-1MiB' + WHEN size < 67108864 THEN '1MiB-64MiB' + WHEN size < 536870912 THEN '64MiB-512MiB' + ELSE '512MiB+' END """ @@ -362,7 +364,9 @@ def analyze_inventory(duckdb_path: str | Path) -> dict[str, Any]: # total ÷ ALL objects (null sizes counted as 0) so the displayed # total_size / object_count / average triple reconciles; the # unknown_size_ratio below tells the reader how many were size-less. - "average_object_size": int(total_size / count), + # Floor division: float division loses int64 precision above 2^53 + # (multi-PB totals), which the Int64 ingestion works to preserve. + "average_object_size": int(total_size) // int(count), "size_histogram": size_hist, "prefix_distribution": prefix_dist, "object_age_distribution": age_dist, @@ -425,7 +429,11 @@ def derive_findings(m: dict[str, Any]) -> list[dict[str, str]]: f"{large_sum / total_size:.1%} of total bytes."}) storage = m.get("storage_class_distribution") or [] - if storage and storage[0]["count"] / m["object_count"] > 0.9: + # Skip the NULL group: an inventory with no storage_class column at all + # (minimal Bucket/Key/Size exports) is "column missing", not "skewed to + # class 'None'" — same null-group guard as hot-key/hot-prefix in access_logs. + if (storage and storage[0]["value"] not in (None, "None", "") + and storage[0]["count"] / m["object_count"] > 0.9): f.append({"severity": "info", "title": "Storage-class skew", "detail": f"Storage class '{storage[0]['value']}' covers " f"{storage[0]['count'] / m['object_count']:.1%} of objects."}) diff --git a/sidecar/app/repositories/evidence_imports.py b/sidecar/app/repositories/evidence_imports.py index a80179f..8c0ac38 100644 --- a/sidecar/app/repositories/evidence_imports.py +++ b/sidecar/app/repositories/evidence_imports.py @@ -154,3 +154,30 @@ def mark_files(conn: sqlite3.Connection, import_id: str, status: str) -> None: (status, import_id), ) conn.commit() + + +def mark_interrupted(conn: sqlite3.Connection) -> int: + """Fail any import still 'importing' at startup. + + The download runs in-process, so no import survives a sidecar restart — a row + still ``importing`` on boot is an orphan from a prior process (hard kill / + power loss mid-download). The in-request failure paths already revert to + 'failed'; only a crash can leave this state, and without this pass the row + could never be re-confirmed (needs 'planned') nor re-run (needs 'confirmed'), + wedging the import forever. Mirrors ``runs.mark_interrupted``. Returns the + number reconciled. + """ + ids = [row[0] for row in conn.execute( + "SELECT id FROM evidence_imports WHERE status = 'importing'" + )] + for import_id in ids: + conn.execute( + "UPDATE evidence_imports SET status = 'failed' WHERE id = ?", (import_id,) + ) + conn.execute( + "UPDATE evidence_import_files SET status = 'failed' " + "WHERE import_id = ? AND selected = 1", + (import_id,), + ) + conn.commit() + return len(ids) diff --git a/sidecar/app/repositories/session_datasets.py b/sidecar/app/repositories/session_datasets.py index 59c7e4f..18db712 100644 --- a/sidecar/app/repositories/session_datasets.py +++ b/sidecar/app/repositories/session_datasets.py @@ -71,8 +71,11 @@ def upsert( (dropping any stale imported DuckDB/table) so the next analysis re-derives it. """ source_filename = _clean_name(source_filename) + # `IS`, not `=`: with a NULL filename `= NULL` never matches, so a re-uploaded + # nameless file would insert a second row pointing at the same overwritten + # path — exactly what this dedupe exists to prevent. existing = conn.execute( - "SELECT id FROM session_datasets WHERE session_id = ? AND source_filename = ? " + "SELECT id FROM session_datasets WHERE session_id = ? AND source_filename IS ? " "ORDER BY rowid DESC LIMIT 1", (session_id, source_filename), ).fetchone() diff --git a/sidecar/app/repositories/sessions.py b/sidecar/app/repositories/sessions.py index 5b666ec..e3c1ce7 100644 --- a/sidecar/app/repositories/sessions.py +++ b/sidecar/app/repositories/sessions.py @@ -398,11 +398,15 @@ def add_message( "(id, session_id, role, content, referenced_run_ids, referenced_evidence_ids, " " tool_activity, grounding, proposed_actions, created_at) " "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", + # The JSON columns go through redact() like every sibling repo + # (replace_findings / upsert_summary / create_case): the agent runtime + # sanitizes upstream, but rule 14 wants the persistence boundary to hold + # on its own — defense in depth, not the only line. (msg_id, session_id, role, redact_text(content or ""), json.dumps(referenced_run_ids or []), json.dumps(referenced_evidence_ids or []), - json.dumps(tool_activity or []), - json.dumps(grounding) if grounding is not None else None, - json.dumps(proposed_actions) if proposed_actions is not None else None, + _dumps(tool_activity or []), + _dumps(grounding) if grounding is not None else None, + _dumps(proposed_actions) if proposed_actions is not None else None, utcnow()), ) _touch(conn, session_id) diff --git a/sidecar/app/routers/sessions.py b/sidecar/app/routers/sessions.py index d5b5ea0..3772138 100644 --- a/sidecar/app/routers/sessions.py +++ b/sidecar/app/routers/sessions.py @@ -545,7 +545,8 @@ async def drive() -> None: raise async for kind, data in session_agent.stream_events_for( result, activity, skill_names, finalize, - cancel_event=cancel_event, clients=clients, budget=budget): + cancel_event=cancel_event, clients=clients, budget=budget, + answer_cap=session_agent._answer_cap(creds)): if kind == "final": final["data"] = data else: diff --git a/sidecar/app/run_service.py b/sidecar/app/run_service.py index e0ee373..75b96a0 100644 --- a/sidecar/app/run_service.py +++ b/sidecar/app/run_service.py @@ -11,6 +11,7 @@ from . import db from .events import bus +from .repositories import evidence_imports as evidence_imports_repo from .repositories import runs as runs_repo from .runs.access_log_run import execute_access_log_run from .runs.account_discovery_run import execute_account_discovery_run @@ -31,11 +32,16 @@ def reconcile_interrupted_runs() -> int: """On startup, fail any run left pending/running by a prior process. In-process run threads can't survive a restart, so such rows are orphans that - would otherwise report as forever-running. Called from the app lifespan. + would otherwise report as forever-running. Evidence imports get the same + pass: a row still 'importing' is an orphan from a crash mid-download and + would otherwise be permanently un-re-runnable (its status can never get back + to 'confirmed'). Called from the app lifespan. """ conn = db.connect() try: - return runs_repo.mark_interrupted(conn) + count = runs_repo.mark_interrupted(conn) + count += evidence_imports_repo.mark_interrupted(conn) + return count finally: conn.close() diff --git a/sidecar/app/runs/analysis_report.py b/sidecar/app/runs/analysis_report.py index 2a72286..5c08266 100644 --- a/sidecar/app/runs/analysis_report.py +++ b/sidecar/app/runs/analysis_report.py @@ -16,14 +16,17 @@ def _bytes_h(n: int | float | None) -> str: + # 1024-based divisors → binary-unit labels (KiB/MiB/…). The labels used to + # say KB/MB while dividing by 1024, so a 1,000,000-byte object printed as + # "976.6 KB" — mislabeled by the SI reading of KB. if not n: return "0 B" n = float(n) - for unit in ("B", "KB", "MB", "GB", "TB", "PB"): - if n < 1024 or unit == "PB": + for unit in ("B", "KiB", "MiB", "GiB", "TiB", "PiB"): + if n < 1024 or unit == "PiB": return f"{n:.1f} {unit}" if unit != "B" else f"{int(n)} B" n /= 1024 - return f"{n:.1f} PB" + return f"{n:.1f} PiB" def _cell(v: object) -> str: diff --git a/sidecar/app/s3/tools.py b/sidecar/app/s3/tools.py index 76a0159..429c8d8 100644 --- a/sidecar/app/s3/tools.py +++ b/sidecar/app/s3/tools.py @@ -411,6 +411,7 @@ def list_multipart_uploads( provider_id: str, bucket: str, max_uploads: int = MAX_LIST_KEYS, + prefix: str | None = None, key_marker: str | None = None, upload_id_marker: str | None = None, ) -> dict[str, Any]: @@ -428,6 +429,8 @@ def list_multipart_uploads( try: client = client_factory.build_s3_client(conn, provider_id) kw: dict[str, Any] = {"Bucket": bucket, "MaxUploads": clamped} + if prefix: + kw["Prefix"] = prefix if key_marker: kw["KeyMarker"] = key_marker if upload_id_marker: @@ -1120,15 +1123,23 @@ def get_object_lock_status( "error_code": None, "error_message_sanitized": None, } # Codes meaning "no lock configured on this object", which is a valid answer. - # Real S3 returns `InvalidRequest` ("Bucket is missing Object Lock - # Configuration") from get_object_retention/legal_hold on a bucket without - # Object Lock — the overwhelmingly common case — so treat it as "none" here - # (these two calls are object-lock-specific) rather than a confusing hard error. _NONE_CODES = { "NoSuchObjectLockConfiguration", "ObjectLockConfigurationNotFoundError", - "InvalidRequest", } + + def _is_no_lock(exc: ClientError) -> bool: + err = (exc.response or {}).get("Error", {}) + if err.get("Code") in _NONE_CODES: + return True + # Real S3 returns the BROAD `InvalidRequest` ("Bucket is missing Object + # Lock Configuration") on a bucket without Object Lock — the common case. + # But InvalidRequest also covers genuinely malformed calls (e.g. a bad + # version_id), so map it to "none" only in its object-lock flavor; + # otherwise reporting "none" would read as "cleanly deletable" when the + # truth is unknown. + return (err.get("Code") == "InvalidRequest" + and "object lock" in str(err.get("Message", "")).lower()) try: client = client_factory.build_s3_client(conn, provider_id) except Exception as exc: # noqa: BLE001 @@ -1153,7 +1164,7 @@ def get_object_lock_status( result["retention_status"] = "active" except ClientError as exc: code = (exc.response or {}).get("Error", {}).get("Code") - if code in _NONE_CODES: + if _is_no_lock(exc): result["retention_status"] = "none" elif code in _UNSUPPORTED_CODES: result["retention_status"] = PROVIDER_UNSUPPORTED @@ -1169,7 +1180,7 @@ def get_object_lock_status( result["legal_hold_status"] = status.lower() if isinstance(status, str) else "none" except ClientError as exc: code = (exc.response or {}).get("Error", {}).get("Code") - if code in _NONE_CODES: + if _is_no_lock(exc): result["legal_hold_status"] = "none" elif code in _UNSUPPORTED_CODES: result["legal_hold_status"] = PROVIDER_UNSUPPORTED @@ -1545,9 +1556,23 @@ def test_conditional_get( try: client = client_factory.build_s3_client(conn, provider_id) resp = client.head_object(Bucket=bucket, Key=key, IfNoneMatch=etag) - # 200: the stored object differs from the supplied ETag. + # 200 does NOT always mean "changed": many S3-compatible providers simply + # ignore If-None-Match on HEAD and return 200 with the SAME ETag. Compare + # (quote-normalized — S3 returns ETags quoted, callers often pass them + # bare) instead of blindly reporting a change: equal ETags on a 200 mean + # the provider ignored the conditional header — a capability gap (rule + # 18), not a stale/changed object. + current = resp.get("ETag") + matches = (current or "").strip('"') == (etag or "").strip('"') + if matches: + return {**base, "success": True, "etag_matches": True, + "current_etag": current, "status_code": 200, + "error_code": PROVIDER_UNSUPPORTED, + "error_message_sanitized": + "Provider ignored If-None-Match (returned 200 with the same " + "ETag instead of 304); conditional requests unsupported."} return {**base, "success": True, "etag_matches": False, - "current_etag": resp.get("ETag"), "status_code": 200} + "current_etag": current, "status_code": 200} except ClientError as exc: fields = _client_error_fields(exc) if fields.get("status_code") == 304 or fields.get("error_code") in ("304", "NotModified"): diff --git a/sidecar/app/security/redaction.py b/sidecar/app/security/redaction.py index 37f7240..1c55b82 100644 --- a/sidecar/app/security/redaction.py +++ b/sidecar/app/security/redaction.py @@ -172,15 +172,33 @@ ] +# Bare (unlabeled) AWS secret access keys. The labeled rule above is anchored to +# a `secret_access_key=`-style label so ordinary 40-char strings and object keys +# are not blanket-mangled — which meant a user pasting a bare AK/SK PAIR into +# chat ("AKIA… / wJalrXUtnFEMI/K7MDENG…") had the SK survive redaction, get +# persisted, and re-enter the next turn's prompt. Narrow fix: ONLY when the text +# also carries an AWS access-key-ID shape (the pair paste — the practical way a +# bare SK shows up) does a bare 40-char base64 token get masked. S3 bucket names +# can't match (lowercase-only, no / +), and the key-id condition keeps the rule +# from firing on ordinary prose or object listings. +_AWS_KEY_ID_HINT = re.compile(r"\b(?:AKIA|ASIA|AGPA|AIDA|AROA|ANPA|ANVA)[A-Z0-9]{16}\b") +_BARE_AWS_SECRET = re.compile(r"(? bool: return _KEY_NORMALIZE.sub("", key.lower()) in _SENSITIVE_KEYS def redact_text(text: str) -> str: """Scrub credential-shaped substrings from a string.""" + # Detect the access-key-ID hint on the ORIGINAL text — the first pattern + # below replaces the key id itself with the redaction marker. + paired_paste = bool(_AWS_KEY_ID_HINT.search(text)) out = text for pattern, repl in _VALUE_PATTERNS: out = pattern.sub(repl, out) + if paired_paste: + out = _BARE_AWS_SECRET.sub(REDACTED, out) return out diff --git a/sidecar/app/sessions/summary_builder.py b/sidecar/app/sessions/summary_builder.py index 65e084c..4e94ad3 100644 --- a/sidecar/app/sessions/summary_builder.py +++ b/sidecar/app/sessions/summary_builder.py @@ -18,9 +18,15 @@ from ..repositories import sessions as sessions_repo from ..security.redaction import redact_text -MAX_FACTS = 50 -MAX_FINDINGS = 50 +# Persisted caps. 200 (was 50): the PERSISTED summary is the ceiling on what the +# model-elastic context cap (session_agent.build_session_context) can ever show a +# large-window model — a flat 50 here silently re-imposed the old clip after the +# context side went elastic. The human-readable summary_md digest stays at +# MD_RENDER_CAP entries so the UI card remains readable. +MAX_FACTS = 200 +MAX_FINDINGS = 200 MAX_FINDINGS_PER_RUN = 20 +MD_RENDER_CAP = 50 _ANALYSIS_TYPES = {"inventory_analysis", "access_log_analysis"} @@ -154,10 +160,14 @@ def bullets(items, key=None): fact_lines = "\n".join( f"- {f['text']} _(run {str(f.get('source_run_id') or 'n/a')[:8]}, {f.get('confidence', 'medium')})_" - for f in facts) or "- —" + for f in facts[:MD_RENDER_CAP]) or "- —" + if len(facts) > MD_RENDER_CAP: + fact_lines += f"\n- _…and {len(facts) - MD_RENDER_CAP} more (kept in the structured summary)_" finding_lines = "\n".join( f"- **[{f['severity']}]** {f['title']} — {f['interpretation']} _(run {str(f.get('source_run_id') or '')[:8]}, {f['confidence']})_" - for f in findings) or "- —" + for f in findings[:MD_RENDER_CAP]) or "- —" + if len(findings) > MD_RENDER_CAP: + finding_lines += f"\n- _…and {len(findings) - MD_RENDER_CAP} more (kept in the structured summary)_" action_lines = "\n".join( f"- **{a['title']}** ({a['action_type']}, {a.get('confidence','medium')}) — {a.get('reason','')}" for a in actions) or "- —" diff --git a/sidecar/app/skills/contract.py b/sidecar/app/skills/contract.py index 6a29781..8148ed3 100644 --- a/sidecar/app/skills/contract.py +++ b/sidecar/app/skills/contract.py @@ -44,9 +44,13 @@ def is_contract_json(payload: str) -> bool: except (json.JSONDecodeError, ValueError): return False return isinstance(parsed, dict) and bool(_CONTRACT_KEYS & parsed.keys()) -# Generous cap so large enumerations (e.g. a 96-row bucket table) are never -# truncated in post-processing; the model's own completion budget bounds length. +# FLOOR of the answer cap (callers pass a model-elastic ``max_answer`` ≥ this) so +# large enumerations are never truncated in post-processing; the model's own +# completion budget bounds length. When the cap IS hit, the cut is MARKED — this +# was the codebase's one silent truncation. _MAX_ANSWER = 48000 +ANSWER_CUT_MARKER = ("[TRUNCATED — the answer reached the output cap and was cut " + "here; ask to continue for the rest.]") CONTRACT_INSTRUCTION = ( @@ -79,7 +83,8 @@ def _strlist(data: dict[str, Any], key: str, cap: int = 12, length: int = 300) - return out -def parse_agent_contract(raw: Any, allowed_skill_names: list[str] | None = None) -> dict[str, Any]: +def parse_agent_contract(raw: Any, allowed_skill_names: list[str] | None = None, + max_answer: int | None = None) -> dict[str, Any]: text = raw if isinstance(raw, str) else str(raw or "") data: dict[str, Any] = {} prose = text @@ -101,7 +106,10 @@ def parse_agent_contract(raw: Any, allowed_skill_names: list[str] | None = None) # e.g. an enumerated list); the JSON block is for metadata. Only fall back to # the JSON "answer" field when there is no prose outside the block. answer_raw = prose if prose.strip() else (data.get("answer") if isinstance(data.get("answer"), str) else "") - answer = strip_chain_of_thought(redact_text(str(answer_raw or "")))[:_MAX_ANSWER] + cap = max(max_answer or 0, _MAX_ANSWER) # elastic caller cap, floored + answer = strip_chain_of_thought(redact_text(str(answer_raw or ""))) + if len(answer) > cap: + answer = answer[:cap].rstrip() + "\n\n" + ANSWER_CUT_MARKER # Cap matches the per-turn read_skill budget (session_tools._MAX_SKILL_LOADS) # so a turn that legitimately loaded several skills can report all of them diff --git a/sidecar/tests/test_analysis.py b/sidecar/tests/test_analysis.py index cd6aaa5..573afcb 100644 --- a/sidecar/tests/test_analysis.py +++ b/sidecar/tests/test_analysis.py @@ -293,7 +293,7 @@ def test_analyze_inventory_metrics(tmp_path): assert key in m assert m["object_count"] == 4 buckets = {b["bucket"] for b in m["size_histogram"]} - assert "512MB+" in buckets and "<4KB" in buckets + assert "512MiB+" in buckets and "<4KiB" in buckets # one object dated 2024 should land in 365d+ age_buckets = {a["bucket"] for a in m["object_age_distribution"]} assert "365d+" in age_buckets diff --git a/sidecar/tests/test_s3_tools.py b/sidecar/tests/test_s3_tools.py index 9dcb656..ea982ff 100644 --- a/sidecar/tests/test_s3_tools.py +++ b/sidecar/tests/test_s3_tools.py @@ -283,6 +283,9 @@ def test_session_list_objects_caps_keys_in_context(client, cloud_id, monkeypatch from app.agent_runtime import session_tools from app.s3 import tools as s3mod + # 700 keys — a legal single page (S3 layer caps a page at MAX_LIST_KEYS=1000). + # v0.37: the echo cap covers the FULL page (it used to sit at 500, silently + # dropping keys 501+ which next_token then skipped past — unreachable forever). monkeypatch.setattr(s3mod, "list_objects_v2", lambda *a, **k: { "success": True, "key_count": 700, "keys": [f"k{i}" for i in range(700)], "sample_keys": [], "common_prefixes": [], "is_truncated": True, "next_token": "T", @@ -298,8 +301,8 @@ def __call__(self, fn): tools = {t.name: t for t in session_tools.build(conn, _FT(), [])} out = _json.loads(tools["list_objects"](cloud_id, BUCKET)) assert out["key_count"] == 700 # exact count preserved - assert len(out["keys"]) == 500 # capped for context (_LIST_KEYS_CTX_CAP) - assert out["keys_truncated_in_context"] is True + assert len(out["keys"]) == 700 # the WHOLE page — nothing unreachable + assert "keys_truncated_in_context" not in out assert out["next_token"] == "T" # can still page @@ -616,13 +619,18 @@ def test_preview_object_parquet_returns_schema_not_body(client, cloud_id, stub): def test_object_lock_status_invalid_request_means_no_lock(client, cloud_id, stub): - """S3 returns InvalidRequest for get_object_retention on a bucket without - Object Lock — treat it as 'none', not a confusing hard error (review L-4).""" + """S3 returns InvalidRequest ("Bucket is missing Object Lock Configuration") + for get_object_retention on a bucket without Object Lock — treat that FLAVOR + as 'none', not a confusing hard error (review L-4). v0.37: the flavor check is + message-based — a generic InvalidRequest no longer maps to 'none' (see + test_v0370_fixes).""" from app.s3 import tools as s3 c, s = stub - s.add_client_error("get_object_retention", service_error_code="InvalidRequest") - s.add_client_error("get_object_legal_hold", service_error_code="InvalidRequest") + s.add_client_error("get_object_retention", service_error_code="InvalidRequest", + service_message="Bucket is missing Object Lock Configuration") + s.add_client_error("get_object_legal_hold", service_error_code="InvalidRequest", + service_message="Bucket is missing Object Lock Configuration") with _db() as conn: res = s3.get_object_lock_status(conn, cloud_id, BUCKET, "obj.bin") assert res["success"] is True diff --git a/sidecar/tests/test_streamed_loop_regression.py b/sidecar/tests/test_streamed_loop_regression.py index bf1f07c..59fdb49 100644 --- a/sidecar/tests/test_streamed_loop_regression.py +++ b/sidecar/tests/test_streamed_loop_regression.py @@ -31,7 +31,7 @@ def fake_start(spec, clients=None): return ("fake-result", lambda: None, []) async def fake_stream(result, activity, skill_names, finalize, *, cancel_event=None, - clients=None, budget=None): + clients=None, budget=None, answer_cap=None): yield "final", {"answer": "hi", "skills_used": [], "evidence_used": [], "evidence_gaps": [], "next_action_proposals": []} diff --git a/sidecar/tests/test_v0270_bugfixes.py b/sidecar/tests/test_v0270_bugfixes.py index 5dfff9a..332537c 100644 --- a/sidecar/tests/test_v0270_bugfixes.py +++ b/sidecar/tests/test_v0270_bugfixes.py @@ -24,7 +24,6 @@ import json import sqlite3 -import pytest from app import config from app.models.schemas import RunCreate @@ -75,15 +74,20 @@ def test_model_budget_tool_output_never_below_floor(): assert mb.tool_output_char_budget(m) >= mb.TOOL_OUTPUT_CHARS_FLOOR -def test_model_budget_completion_floor_and_ceiling(): +def test_model_budget_completion_floor_and_provider_cap(): from app.agent_runtime import model_budget as mb - # Floor model unchanged; large window raised but capped under provider max. + # Floor model unchanged; large window raised but capped by the model's REAL + # provider max-output (the sole upper bound — the old module-wide 32k ceiling + # is gone: it only ever clamped models whose real output cap is higher). assert mb.completion_token_budget("gpt-4o") == mb.COMPLETION_TOKENS_FLOOR - assert mb.completion_token_budget("gpt-4.1") == mb.COMPLETION_TOKENS_CEILING + assert mb.completion_token_budget("gpt-4.1") == mb.max_output_tokens("gpt-4.1") for m in (None, "", "unknown", "claude-opus-4-8"): b = mb.completion_token_budget(m) - assert mb.COMPLETION_TOKENS_FLOOR <= b <= mb.COMPLETION_TOKENS_CEILING + assert mb.COMPLETION_TOKENS_FLOOR <= b <= mb.max_output_tokens(m) + # A 64k-output model on a large window is no longer starved at 32k: window//8 + # governs, clamped only by its provider cap. + assert mb.completion_token_budget("gemini-2.5-pro") == 64_000 def test_install_tool_output_budget_honors_model_limit(): @@ -298,7 +302,7 @@ def test_redact_bytes_preserves_benign_binary(): def test_redact_bytes_still_scrubs_a_real_secret(): - from app.security.redaction import redact, REDACTED + from app.security.redaction import redact secret = b"aws_secret_access_key=wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" out = redact(secret) diff --git a/sidecar/tests/test_v0290_fixes.py b/sidecar/tests/test_v0290_fixes.py index 12d58cf..1008489 100644 --- a/sidecar/tests/test_v0290_fixes.py +++ b/sidecar/tests/test_v0290_fixes.py @@ -187,8 +187,9 @@ def test_max_output_table_gemini25_and_deepseek_reasoner(): assert mb.max_output_tokens("gemini-2.0-flash") == 8_192 assert mb.max_output_tokens("deepseek-reasoner") == 64_000 assert mb.max_output_tokens("deepseek-chat") == 8_192 - # gemini-2.5 (1M window): scaled 32768, no longer clamped to 8192. - assert mb.completion_token_budget("gemini-2.5-pro") == 32_768 + # gemini-2.5 (1M window, 64k output): window//8 governs, clamped only by the + # provider max — the old module-wide 32k ceiling is gone (v0.37). + assert mb.completion_token_budget("gemini-2.5-pro") == 64_000 def test_stale_token_ref_on_keyless_provider_stays_anonymous(client): @@ -318,7 +319,11 @@ def test_tool_output_budget_has_ceiling(): def test_drift_syncs(): from app.agent_runtime import session_agent as sa from app.sessions import summary_builder as sb - assert sa._MAX_FINDINGS == sb.MAX_FINDINGS + # The persisted summary must hold at least as much as the elastic context cap + # can ever reveal (v0.37: floors decoupled — persisted 200 >= floor 50, and + # never below the elastic ceiling's reach for findings). + assert sb.MAX_FINDINGS >= sa._MAX_FINDINGS + assert sb.MAX_FACTS >= sa._MAX_FACTS assert sa._MAX_REPLAY_TOOLS == sa._MAX_TURNS diff --git a/sidecar/tests/test_v0370_fixes.py b/sidecar/tests/test_v0370_fixes.py new file mode 100644 index 0000000..8f9ce6d --- /dev/null +++ b/sidecar/tests/test_v0370_fixes.py @@ -0,0 +1,370 @@ +"""v0.37.0 — four-angle audit batch: recovery, enumeration truth, provider +compat, engine correctness, de-ossification, redaction depth. + + P1 evidence import left 'importing' by a crash is failed at startup (was + wedged forever — could never be re-confirmed nor re-run). + O2 list_objects echoes the FULL S3 page (cap == page max): keys 501-1000 of a + 1000-key page were dropped with no way to page back to them. + O1 the final answer's post-processing cap is elastic and NEVER cuts silently. + S2 test_conditional_get: a provider that ignores If-None-Match (200 + same + ETag) reports "unchanged + unsupported", not a false "object changed". + S4 list_multipart_uploads takes a prefix (usable on prefix-scoped providers). + E1 no "Storage-class skew 'None'" finding when the column is absent. + E2 average_object_size keeps int64 precision (floor division). + P3 session_messages JSON columns are redacted at the persistence boundary. + O3 completion budget's only upper bound is the model's real provider max. + O6/O7 survey-summary echo + deterministic summary caps scale with the window. + E3 binary-divisor sizes carry binary labels (KiB/MiB). + S1 a bare AK/SK PAIR paste is fully scrubbed (SK included), while ordinary + 40-char strings without a key-id hint are untouched. + S3 InvalidRequest maps to "no object lock" ONLY in its object-lock flavor. + P4 read-only duck.connect on a missing DB is a clean error, no stray file. + P5 session_datasets dedupe matches NULL filenames (IS, not =). +""" + +from __future__ import annotations + +import sqlite3 + +import pytest + +from app.migrations import apply_migrations + + +def _db(tmp_path, name="t.db"): + conn = sqlite3.connect(tmp_path / name) + conn.row_factory = sqlite3.Row + apply_migrations(conn) + return conn + + +# --- P1: evidence-import startup reconciler ---------------------------------- + +def test_interrupted_evidence_import_is_failed_on_startup(tmp_path): + from app.repositories import evidence_imports as repo + + conn = _db(tmp_path) + import_id = repo.create_plan( + conn, provider_id="p1", account_run_id=None, snapshot_id=None, + source_type="access_log", source_bucket="b", source_prefix="logs/", + evidence_ref=None, fmt=None, fmt_schema=None, plan_source="agent", + max_files=2, max_bytes=1000, time_range_start=None, time_range_end=None, + planned_file_count=2, planned_total_bytes=200, selected_file_count=2, + selected_total_bytes=200, warnings=[], + files=[{"object_key": "logs/a", "size_bytes": 100, "kind": "log", "selected": True}, + {"object_key": "logs/b", "size_bytes": 100, "kind": "log", "selected": True}], + ) + repo.set_status(conn, import_id, "confirmed") + assert repo.claim_for_import(conn, import_id, "run-x") # → 'importing' + # Simulate the crash: nothing else runs; next boot reconciles. + assert repo.mark_interrupted(conn) == 1 + row = repo.get(conn, import_id) + assert row["status"] == "failed" # re-runnable via a fresh plan, not wedged + files = conn.execute( + "SELECT status FROM evidence_import_files WHERE import_id = ?", (import_id,) + ).fetchall() + assert all(f["status"] == "failed" for f in files) + # Idempotent: a second boot reconciles nothing. + assert repo.mark_interrupted(conn) == 0 + conn.close() + + +def test_reconcile_interrupted_runs_covers_evidence_imports(monkeypatch, tmp_path): + from app import run_service + + conn_path = tmp_path / "svc.db" + conn = sqlite3.connect(conn_path) + conn.row_factory = sqlite3.Row + apply_migrations(conn) + conn.execute("INSERT INTO evidence_imports (id, source_type, status, created_at) " + "VALUES ('imp1', 'access_log', 'importing', 't')") + conn.commit() + conn.close() + + def _connect(): + c = sqlite3.connect(conn_path) + c.row_factory = sqlite3.Row + return c + + monkeypatch.setattr(run_service.db, "connect", _connect) + assert run_service.reconcile_interrupted_runs() == 1 + check = _connect() + assert check.execute("SELECT status FROM evidence_imports WHERE id='imp1'").fetchone()[0] == "failed" + check.close() + + +# --- O2: the echo cap can never clip an S3 page ------------------------------- + +def test_list_keys_ctx_cap_covers_full_page(): + from app.agent_runtime import session_tools + from app.s3 import tools as s3 + + # next_token is computed over the FULL page, so an echo cap below the page + # size makes the clipped tail permanently unreachable. + assert session_tools._LIST_KEYS_CTX_CAP >= s3.MAX_LIST_KEYS + + +# --- O1: answer cap is elastic and marked ------------------------------------- + +def test_answer_truncation_is_marked_not_silent(): + from app.agent_runtime import session_agent as sa + + long_answer = "x" * (sa._MAX_OUTPUT + 1000) + contract = sa._finalize_contract(long_answer, [], []) + assert sa._ANSWER_CUT_MARKER in contract["answer"] + # An answer within the cap is untouched. + ok = sa._finalize_contract("short answer", [], []) + assert ok["answer"] == "short answer" + + +def test_answer_cap_scales_with_completion_budget(): + from app.agent_runtime import session_agent as sa + + # Floor for unknown/small models; ≥ 4 chars/token of the completion budget + # for large-output models, so post-processing never cuts a legal completion. + assert sa._answer_cap(None) == sa._MAX_OUTPUT + assert sa._answer_cap({"model": "gpt-4o"}) >= sa._MAX_OUTPUT + big = sa._answer_cap({"model": "gemini-2.5-pro"}) + assert big >= 4 * 64_000 + + +# --- S2: conditional GET vs providers that ignore If-None-Match --------------- + +class _FakeClient: + def __init__(self, etag): + self._etag = etag + + def head_object(self, **kw): + return {"ETag": self._etag} + + +def test_conditional_get_same_etag_is_unsupported_not_changed(monkeypatch): + from app.s3 import client_factory + from app.s3 import tools as s3 + + monkeypatch.setattr(client_factory, "build_s3_client", + lambda conn, pid: _FakeClient('"abc123"')) + # Caller passes the ETag bare (no quotes) — normalization must still match. + out = s3.test_conditional_get(None, "p", "b", "k", "abc123") + assert out["success"] is True + assert out["etag_matches"] is True # NOT a false "changed" + assert out["error_code"] == s3.PROVIDER_UNSUPPORTED + + # A genuinely different ETag on 200 is still "changed". + monkeypatch.setattr(client_factory, "build_s3_client", + lambda conn, pid: _FakeClient('"other"')) + out = s3.test_conditional_get(None, "p", "b", "k", "abc123") + assert out["etag_matches"] is False + + +# --- S4: multipart listing accepts a prefix ----------------------------------- + +def test_list_multipart_uploads_passes_prefix(monkeypatch): + from app.s3 import client_factory + from app.s3 import tools as s3 + + seen = {} + + class _C: + def list_multipart_uploads(self, **kw): + seen.update(kw) + return {"Uploads": [], "IsTruncated": False} + + monkeypatch.setattr(client_factory, "build_s3_client", lambda conn, pid: _C()) + out = s3.list_multipart_uploads(None, "p", "b", prefix="team/data/") + assert out["success"] is True + assert seen.get("Prefix") == "team/data/" + + +# --- E1/E2: inventory findings + precision ------------------------------------ + +def _inv(tmp, header, rows): + import os + + from app.analysis import inventory as inv + p = os.path.join(tmp, "i.csv") + open(p, "w").write(header + rows) + db = os.path.join(tmp, "i.duckdb") + inv.import_inventory_file(p, db) + return db + + +def test_no_storage_class_skew_finding_for_missing_column(tmp_path): + from app.analysis import inventory as inv + + db = _inv(str(tmp_path), "bucket,key,size,last_modified\n", + "".join(f"b,k{i},100,2026-01-01T00:00:00Z\n" for i in range(20))) + m = inv.analyze_inventory(db) + f = inv.derive_findings(m) + assert not any(x["title"] == "Storage-class skew" for x in f) + + +def test_storage_class_skew_still_fires_on_real_skew(tmp_path): + from app.analysis import inventory as inv + + db = _inv(str(tmp_path), "bucket,key,size,last_modified,storage_class\n", + "".join(f"b,k{i},100,2026-01-01T00:00:00Z,STANDARD\n" for i in range(20))) + m = inv.analyze_inventory(db) + f = inv.derive_findings(m) + assert any(x["title"] == "Storage-class skew" for x in f) + + +def test_average_object_size_keeps_int64_precision(): + # Pure arithmetic check of the fixed expression shape. + total, count = 9_007_199_254_740_993, 1 # 2^53 + 1 + assert int(total) // int(count) == total + assert int(total / count) != total # the old float path really was lossy + + +# --- P3: session_messages JSON columns are redacted --------------------------- + +def test_session_message_json_columns_are_redacted(tmp_path): + from app.repositories import sessions as repo + + conn = _db(tmp_path) + sid = repo.create(conn, __import__("app.models.schemas", fromlist=["SessionCreate"]) + .SessionCreate(title="t")) + key_id = "AKIA" + "ABCDEFGHIJKLMNOP" # assembled: keep secret-shaped literals out of source + mid = repo.add_message( + conn, sid, "assistant", "hello", + tool_activity=[{"tool": "x", "result": key_id}], + grounding={"evidence_used": ["secret_key=" + "wJalrXUtnFEMIK7MDENGbPxRfiCY" + "EXAMPLEKEYAA"]}, + ) + row = conn.execute("SELECT tool_activity, grounding FROM session_messages WHERE id = ?", + (mid,)).fetchone() + assert "AKIA" not in row["tool_activity"] + assert "wJalrXUtnFEMIK7MDENG" not in row["grounding"] + conn.close() + + +# --- O6: survey summary echo scales with the window --------------------------- + +def test_run_result_summary_cap_is_elastic(tmp_path): + from app.agent_runtime import session_action_tools as sat + from app.models.schemas import RunCreate + from app.repositories import runs as runs_repo + + conn = _db(tmp_path) + rid = runs_repo.create(conn, RunCreate(run_type="account_discovery", user_prompt="x"), + status="completed") + runs_repo.set_status(conn, rid, "completed", final_summary="s" * 10_000) + small = sat._run_result(conn, rid) # default floor + big = sat._run_result(conn, rid, summary_cap=8000) + assert len(small["final_summary"]) == sat._MAX_SUMMARY + assert len(big["final_summary"]) == 8000 + conn.close() + + +# --- O7: persisted summary holds more than the old flat 50 -------------------- + +def test_summary_builder_persists_beyond_fifty(): + from app.sessions import summary_builder as sb + + assert sb.MAX_FACTS >= 200 and sb.MAX_FINDINGS >= 200 + # The human digest stays readable: rendering caps at MD_RENDER_CAP with an + # explicit "+N more" note. + facts = [{"text": f"f{i}", "source_run_id": None} for i in range(sb.MD_RENDER_CAP + 5)] + md = sb._render_md({"title": "t", "goal": None}, facts, [], [], [], []) + assert "…and 5 more" in md + + +# --- E3: binary labels for binary math ---------------------------------------- + +def test_bytes_h_uses_binary_labels(): + from app.runs.analysis_report import _bytes_h + + assert _bytes_h(1024) == "1.0 KiB" + assert _bytes_h(1_000_000) == "976.6 KiB" # binary divisor, binary label + assert _bytes_h(1048576) == "1.0 MiB" + + +# --- S1: bare AK/SK pair paste is scrubbed ------------------------------------ + +# Assembled at runtime (not literals) so GitHub push protection doesn't flag the +# AWS docs example pair in this file's source; the redactor sees the same text. +_EX_KEY_ID = "AKIA" + "IOSFODNN7" + "EXAMPLE" +_EX_SECRET = "wJalrXUtnFEMI/K7MDENG/bPxRfiCY" + "EXAMPLEKEY" + + +def test_bare_secret_key_scrubbed_when_paired_with_key_id(): + from app.security.redaction import REDACTED, redact_text + + out = redact_text(f"my creds: {_EX_KEY_ID} and {_EX_SECRET}") + assert _EX_SECRET not in out + assert _EX_KEY_ID not in out + assert REDACTED in out + + +def test_bare_forty_char_string_untouched_without_key_id(): + from app.security.redaction import redact_text + + # No access-key-id hint → a 40-char token (e.g. a hash or object key part) + # must NOT be mangled. + assert redact_text(f"checksum is {_EX_SECRET}") == f"checksum is {_EX_SECRET}" + + +# --- S3: InvalidRequest → "none" only in its object-lock flavor --------------- + +def _lock_client(code, message): + from botocore.exceptions import ClientError + + class _C: + def get_object_retention(self, **kw): + raise ClientError({"Error": {"Code": code, "Message": message}}, "GetObjectRetention") + + def get_object_legal_hold(self, **kw): + raise ClientError({"Error": {"Code": code, "Message": message}}, "GetObjectLegalHold") + + return _C() + + +def test_invalid_request_object_lock_flavor_is_none(monkeypatch): + from app.s3 import client_factory + from app.s3 import tools as s3 + + monkeypatch.setattr(client_factory, "build_s3_client", + lambda conn, pid: _lock_client( + "InvalidRequest", "Bucket is missing Object Lock Configuration")) + out = s3.get_object_lock_status(None, "p", "b", "k") + assert out["retention_status"] == "none" and out["legal_hold_status"] == "none" + assert out["error_code"] is None + + +def test_invalid_request_other_flavor_is_not_none(monkeypatch): + from app.s3 import client_factory + from app.s3 import tools as s3 + + monkeypatch.setattr(client_factory, "build_s3_client", + lambda conn, pid: _lock_client("InvalidRequest", "Invalid version id specified")) + out = s3.get_object_lock_status(None, "p", "b", "k") + # NOT silently "clean/deletable": the error is surfaced. + assert out["error_code"] == "InvalidRequest" + + +# --- P4: read-only connect on a missing DB — clean error, no stray file ------- + +def test_readonly_connect_missing_db_is_clean_error_no_stray_file(tmp_path): + from app.analysis import duck + + missing = tmp_path / "none" / "missing.duckdb" + with pytest.raises(ValueError, match="no analytical database"): + duck.connect(missing, read_only=True) + assert not missing.exists() # the old fallback created an empty writable DB + + +# --- P5: NULL-filename dedupe ------------------------------------------------- + +def test_session_dataset_upsert_dedupes_null_filename(tmp_path): + from app.models.schemas import SessionCreate + from app.repositories import session_datasets as sds + from app.repositories import sessions as repo + + conn = _db(tmp_path) + sid = repo.create(conn, SessionCreate(title="t")) + a = sds.upsert(conn, sid, "access_log", None, "up/x.log") + b = sds.upsert(conn, sid, "access_log", None, "up/x.log") + assert a == b # reused, not a second row at the same path + n = conn.execute("SELECT count(*) FROM session_datasets WHERE session_id = ?", + (sid,)).fetchone()[0] + assert n == 1 + conn.close()