feat(telemetry): archive model-visible tool evidence - #268
feat(telemetry): archive model-visible tool evidence#268brotherzhao2019 wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
MingruiZhang
left a comment
MingruiZhang
left a comment
There was a problem hiding this comment.
Full review of the evidence-archive pipeline (10 review angles + line-level verification of every claim below). Direction is solid — separate lossless model-visible record, content-addressed identity, commit-last upload, double privacy gating are all the right calls. But there are 3 ship-blockers in the upload protocol that make the feature fail for typical runs, plus a cluster of durability bugs. All are cheap to fix relative to the PR. Details inline; summary here.
Blockers (inline comments 👇)
- Client batches by bytes only, proxy rejects >32 records → most real runs get a permanent 400 + infinite 30s retry (
mod.rs:417/evidence.js:128). - No poison-file handling: deterministic 400s retry forever with identical bytes, and the first-5 retry scan lets ≤5 poisoned files starve all newer evidence (
mod.rs:442,mod.rs:330). - Redaction corrupts adjacent content: the query-value scan doesn't stop at
\,)]etc., so it eats JSON escapes and neighboring fields — corruption is then hashed into the "lossless"content_sha256(trace.rs:996).
Bugs
- Restart-from-batch-zero on mid-file failure → unbounded duplicate ingestion + livelock under the 32MiB/min rate cap (
mod.rs:392). - Evidence silently lost on the cancel→delete path (deferred
EvidenceRunstaging racesremove_dir_all;.trace-stagedmarker only tracks the trace; droppedtry_sendhas no recovery) (mod.rs:147). write_json_atomicdeletes before renaming — a crash in between permanently losestrace.json; on Unix a bare rename is already atomic.run_logging.rshas a safer copy to reuse (evidence.rs:574).- Full archive build (read+redact+hash of every cumulative request file, O(steps²) bytes) runs synchronously on the caller's thread — desktop completion/cancel paths are async Tauri tasks (
mod.rs:141). - Proxy: rate limiter is bypassable by rotating
install_idand runs after validation (evidence.js:233); a 2xx Axiom response with unparsable body is acked as full ingestion (evidence.js:270); body cap measures the re-minified body, not wire bytes (evidence.js:87).
Design notes (non-blocking, worth deciding consciously)
- Transport-only convention:
traces.jsdocuments that proxies never shape payloads so the client can evolve the schema without a proxy deploy;evidence.jshand-duplicates the schema as a reject-on-unknown allowlist (evidence.js:156). Combined with blockers 1–2, any future schema evolution bricks the fleet's evidence pipeline until a synchronized proxy deploy. Either relax the proxy to pipe-guards or consciously revise the convention. - Facts inferred at a distance instead of recorded at write time: provider wire format is re-derived heuristically when
Backend::request_payloadknows it definitively (new providers silently land in the wrong extractor orunsupported);response_statusdecides "accepted" by attempting anLLMResponsedeserialization — a shape change silently turns every archivepartial(evidence.rs:438). Recordingwire_format+ an explicit status at write time removes both fragilities. - The redactor's 3-name query list is a hand-maintained subset of
SECRET_JSON_FIELDSfour lines above it —client_secret=,refresh_token=,password=in URL form pass through unredacted today. retry_pending_evidence/stage_evidence_when_ready/evidence_endpointare verbatim copies of the trace counterparts; consider one spool abstraction.patch_trace_summarymakesevidence.rsa second post-hoc writer oftrace.json(3 writers now, clobber window on cancellation) when the worker'senrich_trace_resourceseam already exists for upload-time attributes.
Minor
- O(n²) batch sizing: whole-batch clone + full re-serialization per record (
mod.rs:408) — keep a running byte total. - Dead
(_, bool)tuple inupload_run_trace; unreachable IP fallback inrateLimitKey;batch_sha256computed per response but never checked by the client. - Duplicated
request_statusexpression +evidence_idsJSON round-trip (evidence.rs:192). split_utf8'send == startfallback emits the whole remainder as one chunk — unreachable whilemax_bytes ≥ 4, but violates the contract if reused.- Identity enrichment at upload time (
app_session_id,app_version) means a legal retry after app restart/update produces duplicates that conflict on those fields — which the doc's reader rule ("reject conflicting values for the same stable key") would reject. Either enrich at staging time or exempt these fields in the reader contract.
Deployment checklist feedback (re: socai-evidence-remote-deployment-checklist)
The checklist is operationally solid (scoped ingest tokens, dev→prod, endpoint-before-client release order, rollback via SOCAI_TELEMETRY_EVIDENCE=off). Four adjustments:
- The dev acceptance test ("one complete task with tool calls") is too small to trip blocker 1 — a short task stays under 32 records and passes while real 15+-step runs fail. Add one long-run acceptance case (20+ steps).
- Sequence the client-side blocker fixes before the final "release client" step; the infra steps can proceed in parallel now.
- The "duplicates are expected, dedupe by stable ID" note conflicts with the repo doc's "reject conflicting values" rule per the enrichment issue above — one of the two needs updating.
- Treat a rising 400 ratio on
/v1/evidenceas page-worthy, not informational: under the current client every deterministic 400 is a permanently-retrying spool file on a user's machine.
Verified clean
No new Rust tests (per repo rule), lowercase branding, docs placement correct; chunk sizes compatible (24KB < 32KB), trace/span id formats match the proxy regex, step numbering 1-based as required, client field set matches the proxy allowlist today, and the redactor's byte cursor can't panic on multibyte input (all terminators ASCII).
| }); | ||
| let candidate_len = | ||
| serde_json::to_vec(&candidate_body).map_or(usize::MAX, |body| body.len()); | ||
| let should_flush = candidate_len > EVIDENCE_BATCH_MAX_BYTES && !batch.is_empty(); |
There was a problem hiding this comment.
Blocker — most runs can never upload. Batches are split by bytes only (512KB), but the proxy rejects any request with more than MAX_RECORDS = 32 records (evidence.js:128). A typical 20-step run produces 40–60 small records (one manifest per step + a few-KB chunk per tool result) totalling far under 512KB → one batch → 400 too_many_records → spool file retained → identical retry every 30s forever.
Fix: also flush when candidate.len() > 32 (or better, >= MAX_RECORDS shared as a constant with a comment pointing at the proxy).
| let Ok(response) = client.post(evidence_endpoint()).json(&body).send().await else { | ||
| return false; | ||
| }; | ||
| if !response.status().is_success() { |
There was a problem hiding this comment.
Blocker — no poison-file handling. Every non-2xx is treated identically: a deterministic 400 (schema rejection, too_many_records, >500 manifest ids, empty provider/model) is retried with byte-identical content every 30s forever — no dead-letter, no backoff, no give-up. Unlike traces, evidence has structural rejection modes, so this actually bites.
Suggest: on 4xx, move the file to a dead/ subdir (or delete after N attempts) and count it in telemetry; keep retrying only on 5xx/network.
| return; | ||
| }; | ||
| let mut paths = Vec::new(); | ||
| while paths.len() < TRACE_RETRY_BATCH_SIZE { |
There was a problem hiding this comment.
Compounding the poison-file issue: this scan takes only the first TRACE_RETRY_BATCH_SIZE (5) directory entries per tick, in read_dir order. Five permanently-failing files sitting first in that order starve every newer pending-evidence file indefinitely. (Also: this function is a verbatim copy of retry_pending_traces — consider one collect_pending_json(dir) helper so a quarantine fix lands in both.)
| .take_while(|byte| { | ||
| !matches!( | ||
| **byte, | ||
| b'&' | b'#' | b' ' | b'\t' | b'\r' | b'\n' | b'"' | b'\'' |
There was a problem hiding this comment.
Blocker — redaction corrupts adjacent content. The value scan stops only at & # " ' and whitespace — not at \ , ) ] } ; <. Two concrete failures:
access_token=abc,expires_in=3600,scope=read→access_token=[redacted]— the non-secret fields after the comma are destroyed.- In a string leaf containing escaped JSON (ubiquitous in XHS browser tool results — every URL carries
xsec_token=):...xsec_token=abc\"...— the scan eats the\of the\"escape, leaving the embedded JSON unparseable.
The corruption happens before hashing, so content_sha256 faithfully signs the damage — undetectable downstream in a feature whose whole point is losslessness. This also affects the pre-existing trace/chat text paths since it was appended to shared redact_secrets.
Fix: add the missing terminators. Also consider deriving the parameter list from SECRET_JSON_FIELDS above instead of a hand-maintained 3-name subset (client_secret=, refresh_token=, password= in URL form pass through unredacted today).
| if commits.len() != 1 { | ||
| return; | ||
| } | ||
| if !upload_evidence_batches(client, evaluation_id, body_records).await { |
There was a problem hiding this comment.
Bug — restart-from-zero on mid-file failure. There's no progress marker, and the proxy forwards each batch to Axiom before a later batch fails. An archive larger than the 32MiB/min per-install rate budget 429s at the same point on every retry: the leading ~32MiB is re-ingested into Axiom every minute, the turn_commit never lands, the file never deletes. Same duplicate-on-retry applies to any transient mid-sequence failure (though those at least eventually complete).
Mitigations: per-file attempt state (skip already-acked batch count), and/or honor 429 with a real backoff instead of the fixed 30s tick.
| let manifest_body = json!({ | ||
| "step": step, | ||
| "request_status": if extraction_error.is_some() { "unsupported" } else { request_status }, | ||
| "evidence_ids": evidence_ids, |
There was a problem hiding this comment.
Protocol gap: evidence_ids is unbounded here, but the proxy rejects manifests with more than 500 ids (evidence.js:194). Late-step requests carry the full accumulated history, so a long conversation with many tool results can legitimately exceed 500 — producing a permanently-rejected archive (same infinite-retry failure mode as the batch-size blocker). Either cap/truncate client-side with a marker, or raise/remove the proxy cap.
| const specificFields = RECORD_FIELDS[record.record_type]; | ||
| if (!specificFields) return 'unknown_record_type'; | ||
| const allowedFields = new Set([...COMMON_FIELDS, ...specificFields]); | ||
| if (Object.keys(record).some((field) => !allowedFields.has(field))) return 'unknown_record_field'; |
There was a problem hiding this comment.
Design decision to make consciously: traces.js documents the transport-only philosophy ("the proxy guards the pipe … never inspects span contents … so clients can evolve the schema without a proxy deploy"), but this route hand-duplicates the Rust record schema as a reject-on-unknown allowlist plus enum/count validation. Combined with the client's retry-forever spool, any future schema evolution — a new field, a new status value, a new record type — permanently poisons every fleet client's pending evidence until a synchronized proxy deploy.
Either relax this to pipe-guards (size, rate, chunk-hash recompute) per the existing convention, or explicitly revise the convention — but the current state contradicts the documented contract next door.
| return createHash('sha256').update(text, 'utf8').digest('hex'); | ||
| } | ||
|
|
||
| function rateLimitKey(req, records) { |
There was a problem hiding this comment.
Rate-limiter gaps: (1) keyed on the client-asserted install_id — rotating ids per request gets unlimited metered ingestion; (2) runs after full validation, so invalid 1MiB bodies get free per-request SHA-256 over up to 32×32KiB chunks with no metering; (3) the IP fallback is unreachable dead code — validation already guarantees a valid install_id on every record, so install:${records[0].install_id} is equivalent; (4) the in-memory Map is per-serverless-instance, so N concurrent instances = N× the documented cap. Same class as the sibling routes, but this endpoint does real CPU work per request, so worth noting before prod.
| }); | ||
| if (!response.ok) throw new Error(`Axiom evidence ingest failed: ${response.status}`); | ||
|
|
||
| const result = await response.json().catch(() => null); |
There was a problem hiding this comment.
A 2xx Axiom response with an unparsable or shapeless body (result === null) skips the partial-ingest check entirely and gets acked as accepted: records.length — after which the client deletes its spool file. If Axiom (or an intermediary) ever returns 200 with a truncated/non-JSON body or changes the response shape, records are silently dropped despite the "removed only after all acks" durability guarantee. Suggest treating an unverifiable ingest result as failure (5xx to the client, which retries).
|
|
||
| async function readJsonBody(req) { | ||
| if (req.body !== undefined && req.body !== null) { | ||
| const text = typeof req.body === 'string' ? req.body : JSON.stringify(req.body); |
There was a problem hiding this comment.
When Vercel has pre-parsed req.body, the 1MiB cap and rate-limit byte accounting measure JSON.stringify(req.body) — the re-minified serialization — not wire bytes. A whitespace-padded 1.4MiB wire body can minify under the cap, and bodyBytes per install undercounts. Also the stringify → parseJson round trip re-parses up to 1MiB for nothing — input is just req.body. Prefer Content-Length (or the raw stream path) for size accounting.
|
Implemented the review follow-up in Ship-blocker fixes
Additional review fixes
Verification
Remaining operator validationThe previously shared Preview bypass value must be revoked/rotated and is not used here. With the rotated secret, please replay a generated 33+/65-record archive and one 20+ step equivalent against |
Summary
llm/NNN.request.jsonartifacts rather than raw tool output or size-bounded trace previews/v1/evidenceVercel proxy backed by a separate Axiom evidence datasetBehavior and scope
The archive follows the Agent's existing context behavior: if socai kept, truncated, or omitted comments before sending a provider request, the evidence archive records that same model-visible value. It does not restore data from raw tool artifacts.
This change does not modify Agent prompts,
request_messages, context compaction, provider requests, or LLM call counts, so it adds no model input tokens. It only adds terminal-run telemetry processing, local durable staging, network upload, and Axiom storage.Evaluator integration is intentionally out of scope for this PR. Existing evaluations continue using the bounded trace packet until evaluator gains an evidence reader and integrity verifier.
Privacy and reliability
SOCAI_TELEMETRY_CHAT_TEXTandSOCAI_TELEMETRY_EVIDENCEdisabledcommitxsec_tokenare redacted before stagingDeployment prerequisites
Before enabling production uploads:
socai-evidence-devandsocai-evidence-prod.AXIOM_EVIDENCE_TOKENwith dataset-scoped ingest permission.AXIOM_EVIDENCE_DATASETand optionallyAXIOM_EVIDENCE_URL/AXIOM_ORG_ID./v1/evidenceVercel function before releasing the client.Verification
cargo check --workspacecargo test --workspace(99 passed, 0 failed, 2 ignored)pnpm buildinapp/pnpm buildinsite/rustfmt --check, Node syntax check, andgit diff --check