Repository navigation
Conversation
patlux
left a comment
patlux
left a comment
There was a problem hiding this comment.
CI did not run the correctness gates on this PR. Only CodeQL and Semgrep OSS ran; ci.yml filters pull_request to branches: [main] and this PR targets model-endpoint-overrides. The "full npm test passes" claim is local-only right now. Either land #162 first (pull_request: {}) or re-push after #158 merges — GitHub retargets the base automatically but does not re-trigger CI on a base change.
The privacy design of the report itself is careful and the tests back it up. Comments inline are about scope creep into /commandcode-status, the warning cadence, and a few subtle behavior changes on the main path.
| [ | ||
| formatCommandCodeStatus(status), | ||
| this.options.getRoutingStatus?.(ctx.model), | ||
| this.options.getDiagnostics?.(ctx.model), |
There was a problem hiding this comment.
Appending the whole diagnostics report to /commandcode-status duplicates information (model endpoint: from getRoutingStatus and route: from the report say the same thing) and roughly doubles the status output.
More importantly: status still prints cache path: (a local path), while the appended footer states "No … local paths are included." — which is now false for that combined output. Keep status lean and leave the shareable report to /commandcode-diagnostics only.
| timestamp: now(), | ||
| stopReason: record.outcome === "aborted" ? "aborted" : "error", | ||
| errorMessage: | ||
| "Command Code request failed before completing the stream; see /commandcode-diagnostics", |
There was a problem hiding this comment.
This replaces the original error with a fixed string. With the current transport router it is practically unreachable (the router turns rejections into error events), but if it ever fires the user loses the actual reason and is sent to a report that only contains a category. Forward the message through the existing redactor instead of a constant.
| record.requestId = | ||
| requestId && /^[a-f0-9-]{16,64}$/i.test(requestId) ? requestId : undefined | ||
| if (!response.body) return response | ||
| return new Response( |
There was a problem hiding this comment.
new Response(body, { status, statusText, headers }) drops url, redirected and type. Nothing in the current chain reads them as far as I can see (isUpgradeRequired and the Responses normalizer use status/body/headers only), but a short comment would save the next person who tries response.url and silently gets "".
| catalogModels.clear() | ||
| for (const model of models) catalogModels.set(model.id, model) | ||
| return createProviderConfig(models, apiBase, transport.stream) | ||
| return createProviderConfig(models, apiBase, measuredStream) |
There was a problem hiding this comment.
createProviderConfig now receives measuredStream, which also applies resolveStreamOptions to the main host path — previously only the compat path did that. Probably a no-op because the host already supplies apiKey, but it is an unannounced change on the primary path. Mention it in the PR/commit body, or add an assertion that a host-supplied key still wins over the configured key.
| : "unknown" | ||
| const diagnostics = createRequestDiagnostics() | ||
| const warnedPrices = new Set<string>() | ||
| pi.on("before_agent_start", (_event, ctx) => { |
There was a problem hiding this comment.
This fires on every host start for anyone whose daily model is unpriced — a recurring nag for a condition the user cannot fix. Consider warning once per model per catalog snapshot (persist next to the routing file), or only when the model is actually switched to, rather than on every before_agent_start.
| @@ -1,3 +1,4 @@ | |||
| import "./test-diagnostics.ts" | |||
There was a problem hiding this comment.
Same double-run as in #158: this import plus npm run test:diagnostics runs the suite twice. Keep the script, drop the import.
| try { | ||
| try { | ||
| response = await fetchImpl(`${apiBase}/alpha/generate`, { | ||
| response = await (options?.fetch ?? fetchImpl)(`${apiBase}/alpha/generate`, { |
There was a problem hiding this comment.
options.fetch now takes precedence over deps.fetchImpl on the generate path — the inverse of the previous precedence. Fine for the diagnostics use case, but worth a sentence in the commit body. (Becomes dead code if #164 lands.)
pierreraby
left a comment
pierreraby
left a comment
There was a problem hiding this comment.
Approved. Verified locally at the exact head in an isolated worktree (npm ci): tsc --noEmit PASS (confirms the before_agent_start hook typechecks against real host typings), test-diagnostics 2/2, full test:unit exit 0 (415 checks), test-pi-local PASS (incl. the /commandcode-diagnostics no-leak smoke), Prettier + git diff --check PASS. I also had the previous head independently reviewed in a fresh context (APPROVE, no P0/P1); the rebase delta is main integration plus diagnostics privacy checks wired into the default suites. Privacy discipline verified: whitelisted endpoints, hex-gated request IDs, enum-only error categories, single bounded in-memory record, no payload/raw-error/config retention; core.ts one-liner is fallback-compatible.
Non-blocking notes: (1) error fidelity dropped on the throw path (generic message, category kept only in the record — appending the classified category would help); (2) route: is the one unsanitized free-text field (callers are enum-composed today); (3) minor: safeIdentifier heuristic gap, pending snapshot if a transport violates its terminal-event contract, no dedicated test:diagnostics script entry. One watch item: upstream CI on this head is missing the catalog-sync and memory-benchmark runs — worth confirming they land before merge.
Summary
Stacked on #158; review only this delta.
Verification
Full npm test passes with required real Pi 1.1.0 and OMP 18.11.1 against mock APIs. New synthetic tests cover privacy, measured chunk gaps, and unknown/free prices; real Pi RPC exercises the diagnostics command. Formatting and diff checks pass.
No telemetry, automatic upload, persistent request logs, or authenticated live requests. Timing is chunk-based, not tokens/second. Host-level retries are outside one stream summary.