Repository navigation
Conversation
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
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.
Port @patlux's `/commandcode-diagnostics` work onto current main, where the stack it was opened against (#158, per-model endpoint routing) is not merged. The diagnostics report, per-request body-chunk timing, unknown-price warnings and the structured bug form are unchanged; only the endpoint-routing half of the stack is left out, so the report describes the selected wire from the catalog (`transport/resolveModelApi`) instead of a per-model override. Carried forward from #159 by @patlux so the work is not blocked behind a stack that is still dirty.
Port @patlux's `/commandcode-diagnostics` work onto current main, where the stack it was opened against (#158, per-model endpoint routing) is not merged. The diagnostics report, per-request body-chunk timing, unknown-price warnings and the structured bug form are unchanged; only the endpoint-routing half of the stack is left out, so the report describes the selected wire from the catalog (`transport/resolveModelApi`) instead of a per-model override. Carried forward from #159 by @patlux so the work is not blocked behind a stack that is still dirty.
Carries the diagnostics contribution from #159 while preserving the native provider and Go fallback transports. Add shareable content-free support reports, bounded latest-request measurements and unknown-price warnings; harden identifier redaction and abort classification, and verify real native telemetry, retries and upstream cancellation. Independent DeepSeek V4.1 Flash max review and follow-up completed. Full required Pi/OMP local suites and exact-head CI passed. Existing qualified host-entrypoint static debt is documented in the PR; no exhaustive clean-typecheck claim or CI bypass. Co-authored-by: Patrick Wozniak <email@patwoz.de>
|
Carried forward, integrated and released in v0.8.0. Thank you @patlux for the original contribution. It was adapted to current main through #180; your contribution is credited in the carry/merge history and the released changelog. The safe diagnostics command, body-chunk timing/error categories, unknown-price warnings and structured bug report were carried forward. The routing half of the original stack is integrated separately through #201. The generate-only observation path was subsequently retired by the Provider-only policy in #203; diagnostics remain on the supported inference path. The released npm tarball passed the required real Pi and OMP mock-API suites, including registry-package verification: release workflow. These are isolated mock tests, not live account, billing or performance verification. This original PR is superseded by the integrated carry, not abandoned. It is being closed without merging its old branch again. No source branch or linked issue is being deleted/closed as part of this cleanup. |
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.