fix(logs): sanitize the shadow helper marker at the logging layer - #2166
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change records the original helper model for opt-in shadow-call rewrites. It adds sanitization across request logs and usage persistence, restores the field after restart, extends tests, updates usage documentation, and records residual backlog dispositions. ChangesShadow-call source logging
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ShadowCallRewrite
participant handleResponses
participant addFinalRequestLog
participant UsageLog
ShadowCallRewrite->>handleResponses: original helper model in RequestLogContext
handleResponses->>addFinalRequestLog: shadowCallRewrittenFrom
addFinalRequestLog->>UsageLog: sanitized usage metadata
UsageLog-->>addFinalRequestLog: persisted marker available after hydration
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The logging-layer direction is correct, but one public ingress still bypasses the new boundary on the current head (70b947550).
addRequestLog(entry) calls retainRequestLogEntry(entry) before rebuilding the persisted row. A direct caller can therefore place the raw shadowCallRewrittenFrom value in the in-memory /api/logs ring, while appendUsageEntry() sanitizes only the disk copy. I reproduced this at the exact head: control/secret-shaped content remained in memory at length 111, the persisted value was sanitized to length 37, and the two values differed. The focused tests/request-log.test.ts suite still passes (51/51), so the existing tests do not cover this ingress.
Please sanitize a copied entry at the start of addRequestLog, then use that same sanitized entry for both retainRequestLogEntry and persistence. Add a direct-addRequestLog regression covering control characters, secret-shaped content, the 64-character cap, and memory/disk equality. Keeping the normalization at this shared ingress will also protect relay and future direct callers, instead of relying on addFinalRequestLog alone.
리뷰 · 우선순위 52 / 80#2157 백엔드 반쪽임. 섀도 인터셉트가 헬퍼를 다른 모델로 바꿔도 #2163을 가져옴. 원본은 넣는 한 군데만 sanitize. 메모리 로그는 호출자가 준 값을 그대로 둠. 업스트림 모델 id라 개행이 라인 로그를 쪼갤 수 있음. 길이도 없음. 이번엔 대시보드 점수는 52임. 라벨이 있어야 필터가 됨. 2.28. UI 후속 필요. 해결방안: #2165 다음. #2157은 Logs 필터 붙이면 닫기. 개행/64자 테스트 유지. 이 댓글은 grok-bot이 작성했습니다 |
|
Current head is still The statement that the in-memory row and The review remains blocked until the copied entry is sanitized at the start of |
…abled GET /api/subagent-models built `available` purely from currently-pickable models, so a featured model disabled elsewhere vanished from it. The dashboard filters `chosen` against `available` and then PUTs exactly the rows it holds, which turned a hide into a delete: the next Save wrote the truncated roster to config.json, and the user read it as "ocx service lost my subagent models". Retain a chosen id in `available` when it is not otherwise selectable, appended after the selectable set and deduplicated. Models that are disabled and NOT in the roster stay excluded, so the picker behavior is unchanged for every model the user has not deliberately featured. The combo test asserted the old truncating behavior; it now asserts retention while a roster slot is held, and full exclusion once the slot is released. Closes #2133
…claims opencode-free sent no User-Agent, so Zen saw the bare runtime default (Bun/x.y.z) and rate-limited it harder than a client that identifies itself. Adds "User-Agent: opencode" alongside the existing x-opencode-client: desktop marker. The value is deliberately unversioned. OmniRoute, an independent open-source broker against the same Zen upstream, defaults to exactly this pair and reached it by retreating from its own earlier opencode-cli/1.0.0 pin: a pinned version is a claim about an install we do not have, and it goes stale on the vendor's schedule. The registry edit alone would have shipped to nobody. staticHeaders is documented as merged into every upstream request, but it was only ever copied at seed time, so any config written before a header existed -- or carrying any header of its own -- never received it. routedProviderConfig and buildModelsRequest now fill registry static headers beneath user headers, matched case-insensitively so an override replaces rather than duplicates: spreading "User-Agent" over a user's "user-agent" leaves both keys, which Headers serializes as one comma-joined value. Model discovery gets the same treatment because a provider identified as opencode when it completes but anonymous when it lists its own models reads as two different clients to a rate limiter.
…non-English AgentRouter answers 400 content-blocked when the first user message is not in English (#2074) while the identical English request returns 200. The gateway inspects the opening user content, so an Anthropic system string never reaches the filter -- the framing has to sit in that turn. Two corrections on top of @yzxcj797's #2082. The host test was hostname.includes("agentrouter"), which also matches notagentrouter.example and agentrouter.org.attacker.example. A prompt mutation keyed on a provider's identity has to be keyed on that identity exactly, so this matches agentrouter.org or a real subdomain of it. The original spliced the marker into the user's own string. That edits what the user wrote: logs, retries, and any upstream echo then show a sentence the user never typed as if they had. The framing is now its own leading text block, so the original text survives byte-for-byte. Idempotence is keyed on the leading block being exactly the marker rather than a substring test, so a user who quotes the marker later in their prompt does not suppress their own framing.
…t shape Review on #2162 asked for the three branches that are the actual safety boundary here: this code writes into the outbound first user turn, so "does it ever duplicate, drop, or reorder what the caller sent" has to be pinned per content shape rather than only for a plain string. Adds: an already-framed turn stays single and ordered; image-only content keeps its image block behind the preamble rather than losing or reordering it; assistant-only block content keeps its tail and is followed by the synthesized [PREAMBLE, "(continue)"] user turn. All three pass against the existing implementation, which is the point -- they are guards on a prompt mutation, not a fix.
…amed A multi-account setup points several provider rows at the same OpenCode Go endpoint under names the registry has never heard of -- opencode-go-2 through -5. Quota dispatch gated on the literal name "opencode-go", so those rows had no dashboard quota panel and no report in `ocx provider quota --refresh --json` even though each one holds a working key for the same upstream (#1924). Identity is now answered by registryEntryForProviderDestination, the predicate this repository already uses for renamed fixed-key rows: it matches on normalized endpoint plus adapter plus key auth. A bare URL comparison would have been enough for the reported symptom but would also probe a row that points at that host through a different adapter, which speaks a different protocol and is not the provider whose quota shape we parse. The defensive canonical-URL check inside fetchOpenCodeGoQuota stays. Whether an API key may be sent to a host must not depend on the dispatch gate above it being correct. Absorbed from #2027 by @yzxcj797.
…d is known Some OpenAI-compatible streamers repeat an already-sent id, name, or arguments as a non-string placeholder on a continuation delta rather than as null. Validation ran before the pending-call lookup, so the whole turn died with a 502 and the tool never ran -- even though the value being repeated was already held in canonical form. The lookup now happens first and tolerance is per field, keyed on that field's own provenance. Two corrections on top of @waw4303's #2155. It gated arguments acceptance on the call having a canonical NAME. A name says nothing about whether arguments was ever sent as a string, so a real argument payload could be silently dropped. PendingToolCall now carries sawArgumentsString; an empty string counts, because it proves the upstream sent the field with the right wire type. It also left a non-string repeated id unconditionally terminal even after a canonical id was stored. Ids now follow the same rule as the other two. Diagnostics are passed from the rejection site instead of rescanned. A stateless rescan stops at the first structurally odd value, so a stream carrying accepted padding on call 0 and a real defect on call 1 blamed call 0.
Builds on @Ingwannu's #2163, which types and persists shadowCallRewrittenFrom so an intercepted helper request keeps its original model in usage.jsonl and across restart hydration. The original sanitized the value at the single call site that populates it today, which left the in-memory /api/logs row carrying whatever the caller sent. The marker originates in an upstream-supplied model id, so an unsanitized newline lets one field forge a record boundary in any line-oriented log viewer, and nothing bounded its length on that path. addFinalRequestLog now runs it through sanitizeLogMetadataString itself. A future caller cannot reintroduce the hole by forgetting to sanitize first, and the in-memory row matches what usage.jsonl already stored. The added regression writes an unsafe overlong marker and asserts the newline is gone and the 64-character bound holds on both paths. The original test used a safe short slug, so it passed identically whether the sanitizer ran or not.
70b9475 to
70cebd4
Compare
…lper-attribution # Conflicts: # devlog/_plan/260820_bug_pr_backlog_consolidation/080_residual_dispositions.md # tests/anthropic-agentrouter-language-framing.test.ts
The Codex App now sends gpt-5.6-luna helper requests on every message and every turn completion, not only for title generation (#2157). That traffic originates in the App, so stopping it is not ours to do; what is ours is letting the operator tell it apart from their own work. #2166 landed the backend half. shadowCallRewrittenFrom already reaches /api/logs -- requestLogDto spreads the whole entry -- but the GUI's LogEntry type omitted it, so TypeScript and the UI ignored a field that was sitting right there in the payload. Adds the type field, a row badge naming the original helper model, and an "Intercepted helpers only" filter following the same local-state plus predicate-conjunct shape the surface filter already uses. The label is deliberately narrow. The marker is written only when Shadow Call Intercept actually rewrote the request, so a helper request that was not intercepted -- interception off, no replacement model, or a slug the matcher does not recognize -- carries no marker and is indistinguishable from ordinary traffic here. "Helper traffic" or "hide agent traffic" would promise a classification this data cannot support.
2 participants
Summary
Absorbs #2163 by @Ingwannu. Stack layer 6 — base is
codex/absorb-openai-chat-padding-repeats(#2165), notdev.What it fixes. When the opt-in shadow-call route intercepts a Codex App helper request and rewrites it to an operator-selected model, the original helper model was lost.
shadowCallRewrittenFromwas written through an untyped cast atcore.ts, butRequestLogContext/RequestLogEntrynever declared it andaddFinalRequestLogrebuilds its row field by field — so the marker reached neither/api/logsnorusage.jsonl, and could not survive restart hydration. Recurring helper spend showed up as an unattributable row (#2157).@Ingwannu's typing, persistence projection, and hydration work is carried unchanged in principle. Both of the author's commits are preserved.
The correction. The original sanitizes at the one call site that populates the field today, which leaves the in-memory
/api/logsrow carrying whatever the caller sent. The value originates in an upstream-supplied model id, so an unsanitized newline lets a single field forge a record boundary in any line-oriented log viewer, and nothing bounded its length on that path.addFinalRequestLognow runs it throughsanitizeLogMetadataStringitself — a future caller cannot reintroduce the hole by forgetting to sanitize first, and the in-memory row matches whatusage.jsonlalready stored.Scope note. This is the backend attribution half. The dashboard does not yet render or filter on the field (
gui/src/pages/Logs.tsx), so #2157 stays open until that lands — closing it on this PR would claim a UI affordance that does not exist.Closes #2163.
Verification
bun run typecheck— clean.bun test --isolate tests/request-log.test.ts -t 'shadow'— 2 pass / 0 fail.bun run testat this tip — 13564 pass / 10 skip / 0 fail across 857 files.bun run privacy:scan— passed.Checklist
Only a model-id scalar is stored, through the shared control-stripping, secret-redacting, 64-character sanitizer. No request content is logged.
Stack map
Merge bottom-up; each layer's base is the branch below it.
codex/fix-subagent-roster-truncationdev)codex/absorb-opencode-free-static-headerscodex/absorb-agentrouter-language-framingcodex/absorb-opencode-go-quota-siblingscodex/absorb-openai-chat-padding-repeatscodex/absorb-shadow-helper-attributionNo file overlap with any layer below — this sits at the top for a single clean chain, not because the code requires it.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation