feat: add --show-request, --show-response, --curl, --show-secrets flags#23
Merged
feat: add --show-request, --show-response, --curl, --show-secrets flags#23
Conversation
Adds four global flags that write API traffic to stderr for debugging: - --show-request: outgoing requests as HTTP-style blocks - --show-response: responses (success and error) as HTTP-style blocks - --curl: outgoing requests as curl commands - --show-secrets: opt out of redacting Authorization (default: redacted to "Api-Key ***" / "Bearer ***", keeping the scheme prefix) When stderr is a TTY and --no-color is not set, JSON bodies and HTTP headers are syntax-highlighted via cli-highlight + chalk. The Chalk instance is forced to level 1 so coloring is gated on the caller's boolean rather than chalk's own TTY heuristics, which keeps output deterministic when stdout and stderr are routed differently. The new response interceptor is registered before the existing OAuth refresh and error-translation interceptors, so retries (axios-retry, 401 -> refresh -> retry) emit one log line per attempt. The existing -v/--verbose flag is unchanged.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds request/response logging and formatting across the HTTP client and download utilities. Introduces a new request-logger module (HTTP and curl formatters, redaction, suppression notices, error formats). Extends the Axios HTTP client to emit configurable stderr logs, fingerprint and suppress repeated requests, record elapsed time, and support curl rendering and secret disclosure. Propagates new global/CLI flags (showRequest, showResponse, curl, showSecrets, headersOnly, statusOnly, colorDisabled/noColor) through helpers and download code. Adds extensive tests and updates .gitignore. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/api/request-logger.ts`:
- Around line 184-190: The generated curl lines currently interpolate raw ${url}
and header values ${v} into single-quoted shell strings (in the lines.push that
builds the curl command and inside the headerEntries loop), which breaks when
those values contain single quotes; add a small helper (e.g.,
escapeSingleQuotesForShell) that replaces every single-quote in a string with
the shell-safe sequence ('\'') (the standard '\'' escape) and use that helper to
escape the URL and header name/value before inserting into the template strings
so the resulting single-quoted curl command is safe to paste into a shell.
In `@src/lib/utils/api-helper.ts`:
- Around line 85-90: The noColor flag is currently forwarded from
options.noColor (which is derived from !process.stdout.isTTY) and can wrongly
disable colors when stderr is a TTY; update the construction that sets
showRequest/showResponse/curl/showSecrets/noColor so that noColor defaults to
options.noColor ?? !process.stderr.isTTY (or otherwise prefer an explicit
options.noColor when provided) instead of relying on stdout.isTTY; change any
upstream derivation to use stderr.isTTY for logging output to stderr and ensure
functions that emit logs use this noColor value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01a6958b-5639-4aa1-81db-ef109a497dea
📒 Files selected for processing (9)
.gitignoresrc/index.tssrc/lib/api/axios-adapter.test.tssrc/lib/api/axios-adapter.tssrc/lib/api/client.tssrc/lib/api/request-logger.test.tssrc/lib/api/request-logger.tssrc/lib/utils/api-helper.test.tssrc/lib/utils/api-helper.ts
Previously --show-response wrote response headers and bodies to stderr
verbatim, so:
- Set-Cookie / Cookie / X-Api-Key / Proxy-Authorization in responses
were emitted in cleartext
- response bodies for endpoints that mint secrets (POST .../api_keys
returning {key}, PUT .../reset-password returning {password}, OAuth
token endpoints returning JWTs) were emitted in cleartext
Both now redact by default. --show-secrets opts out, mirroring the
request-side behaviour.
Sensitive headers redact to *** (Cookie/Set-Cookie/X-Api-Key) or to
"<scheme> ***" (Authorization, Proxy-Authorization). Sensitive body
field names (key, password, token, secret, access_token, refresh_token,
api_key, apikey) are matched case-insensitively and walked recursively
through nested objects and arrays. Non-string values for those fields
pass through unchanged so that null/0/false don't become "***".
Also hardens redactHeaders against non-string sensitive values: a
malformed Authorization that isn't a string now redacts to *** instead
of throwing TypeError on .indexOf.
If an upstream request interceptor (or future custom interceptor) throws a plain Error or rejects with a non-Axios value, the response error handler used to silently skip it because the only branch was guarded by axios.isAxiosError. With --show-response on, this would leave the user staring at a request line with no response line. Adds a generic-error fallback that prints '← (non-HTTP error) <msg>' so the user can tell they're looking at a synthetic failure rather than a real HTTP response.
extractHeaders previously kept only string/number/boolean header values and silently dropped everything else, which meant Set-Cookie (returned by Node's HTTP layer as string[]) never appeared under --show-response. A user debugging cookies would see headers that look complete but silently omit the very ones they want. Joins arrays with ", " (so the user sees all values) and surfaces unexpected object values as "[unprintable: <type>]" rather than dropping them. Sensitive multi-value headers (Set-Cookie/Cookie) are fully redacted to *** by the existing redaction pass after joining, without leaking individual values.
Previously a Content-Type: application/json response whose body failed to parse was silently emitted as raw text, indistinguishable from a properly formatted response. Now prepends a dim marker line so the user can tell the body did not parse. Also drops the dead 'pretty' parameter from formatJsonBody (only one caller, hard-coded true).
…y duplication The same eight-field shape was inlined in three places: the AxiosHttpClient constructor, the createAxiosHttpClient factory, and ClientOptions in client.ts. Adding a field meant editing all three with no compiler-enforced link between them. Define once in axios-adapter.ts and reuse. ClientOptions becomes a type alias to keep its public name.
GlobalOptions.noColor previously folded together two distinct ideas: 1. The user's explicit intent (--no-color flag), and 2. Whether stdout is a TTY. Stdout-bound consumers (the formatter) want the conflated value. Stderr-bound consumers (--show-request / --show-response) want only the user intent — they make their own TTY decision against stderr in axios-adapter. Adds GlobalOptions.colorDisabled holding only the user-intent variant (--no-color OR NO_COLOR env). getAPIClientFromOptions now forwards colorDisabled as the noColor option to the API client, so a user with piped stdout but TTY stderr still gets colored diagnostics by default. Adds NO_COLOR env support (https://no-color.org) to the user-intent side. Chalk honors NO_COLOR on its auto-detect path, but the request logger uses a forced Chalk instance that doesn't, so we check the env explicitly here.
- AxiosHeaders.toJSON() branch in extractHeaders (production hits this via axios; previous tests only used plain object headers). - OAuth refresh + --show-response: confirms both the original 401 and the retry response are logged, no double-counting. - Stderr-TTY override branches: ANSI emitted when TTY + noColor false; suppressed when noColor true; suppressed when stderr is not a TTY.
… checks The four status-code coloring tests asserted exact escape-code bytes (\x1b[32m / \x1b[33m / \x1b[31m / a 5xx bold-red regex). Chalk has historically changed how it composes nested styles (e.g. merging \x1b[1;31m vs separate \x1b[1m\x1b[31m). A future chalk update would break all four despite identical visible output. Now strip ANSI and assert the visible status text is correct, plus ANSI presence overall. The 2xx test additionally compares its colored sequence against 4xx to ensure the bands produce different coloring without coupling to specific bytes.
Polling loops (e.g. abs experiments export --wait) hit the same URL every couple of seconds, and --show-request faithfully printed each iteration, drowning the actual interesting events in noise. The client now fingerprints each outgoing request (method + URL + params + body + Authorization + axios-retry retryCount) and on a match to the previous fingerprint: - skips the request log - skips the matching response log - counts the suppression When a different request comes through, a single line is emitted: (N identical requests suppressed) before the new request log. Authorization is included in the fingerprint so OAuth refresh retries (token changes) are still logged distinctly. retryCount is included so axios-retry attempts are logged distinctly. Polling stays suppressed because all three (URL, auth, retry) match across iterations.
abs experiments export --download invokes downloadFile, which uses raw http.get rather than the AxiosHttpClient — so neither --show-request nor --show-response captured the actual download GET (or any intermediate redirect-resolution requests). The user only saw the polling traffic, then the download finished without a logged request. Threads showRequest/showResponse/curl/showSecrets/noColor through DownloadOptions and emits synthetic axios-shaped log entries from inside downloadFile using the same formatters as the axios path. Redirect-resolution GETs and Range-resume GETs are also logged. The downloaded body itself is not echoed (it's potentially gigabytes). Only headers and status are captured.
- S1: drop dead `original` capture and stale comment in captureStderr
test helper. spy.mockRestore() already restores the real write.
- S2: trim the chalk-instance comment to one line. The Chalk({level:1})
+ visible per-call gating is self-documenting.
- S3: add a one-line comment explaining why showResponse is in the
request-interceptor gate (it stamps startTime).
- S5: shell-escape single quotes in URL and header values too, not just
the body. URLs with apostrophes are exotic but possible.
- S9: mark constructor-stored option fields readonly.
Also picks up prettier-driven reflow on touched files.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/lib/utils/download.ts (1)
128-142: Consider logging network failures in download mode for parity.
httpGetlogs request/response, butreq.on('error', reject)bypasses debug output. With--show-response, this leaves a visibility gap exactly when troubleshooting is needed most.🔧 Proposed direction
import { formatRequestHTTP, formatRequestCurl, formatResponseHTTP, + formatNetworkError, type FormatOptions, } from '../api/request-logger.js'; @@ - req.on('error', reject); + req.on('error', (err) => { + if (logger?.showResponse) { + process.stderr.write( + formatNetworkError(err as never, Date.now() - start, logger.formatOpts) + '\n' + ); + } + reject(err); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/download.ts` around lines 128 - 142, The httpGet function currently attaches req.on('error', reject) which skips any debug logging; update httpGet so the 'error' event handler logs the network failure (using the same logger used by logRequest/logResponse) including the error message and elapsed time before rejecting the promise. Locate the httpGet function and the req.on('error', ...) line and replace the direct reject with a handler that calls the logger (e.g., logger?.debug or an existing logResponse/logError helper) with a clear message and the error object, then rejects; ensure this only runs when a logger is present and preserves the original reject behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/api/axios-adapter.ts`:
- Around line 312-314: The request interceptor currently does
JSON.stringify(config.params) and JSON.stringify(config.data) directly which can
throw for non-serialisable values and abort the request; wrap these
serialisations in try/catch inside the axios request interceptor (the code
setting const params and const body) and on error fall back to a safe string
(e.g. empty string or a best-effort alternative like util.inspect/JSON.stringify
with a replacer) so that serialization failures do not throw and the request
proceeds, and ensure the catch includes a debug log mentioning
config.params/config.data to aid debugging.
In `@src/lib/api/request-logger.ts`:
- Around line 72-83: redactBody currently only replaces sensitive keys when the
value is a non-empty string, so objects/arrays/numbers under sensitive keys
remain unredacted; update the logic in redactBody (referencing
SENSITIVE_BODY_FIELDS, REDACTED) so that if a lowercased key is in
SENSITIVE_BODY_FIELDS it is replaced with REDACTED regardless of the value type,
otherwise continue the existing recursive handling (including Array.isArray and
recursive redactBody calls), and keep the initial showSecrets/primitive
short-circuit as-is.
- Around line 167-183: The logging code calls JSON.stringify directly (notably
in formatJsonBody and where redactBody output is serialized), which will throw
on circular or non-serialisable values and break requests; add a small safe
serializer (e.g., safeStringify or safeSerialize) that catches errors and
handles circular refs (or returns a clear fallback string like "(unserializable
value)" plus a brief diagnostic) and use it everywhere JSON.stringify is called
in this module (replace uses inside formatJsonBody and the places the review
mentions). Ensure redactBody continues to redact but feed its result through
safeStringify before logging so the logging path never throws; keep the
serializer single-responsibility and reuse it across formatJsonBody, redactBody
callers, and other direct JSON.stringify sites.
---
Nitpick comments:
In `@src/lib/utils/download.ts`:
- Around line 128-142: The httpGet function currently attaches req.on('error',
reject) which skips any debug logging; update httpGet so the 'error' event
handler logs the network failure (using the same logger used by
logRequest/logResponse) including the error message and elapsed time before
rejecting the promise. Locate the httpGet function and the req.on('error', ...)
line and replace the direct reject with a handler that calls the logger (e.g.,
logger?.debug or an existing logResponse/logError helper) with a clear message
and the error object, then rejects; ensure this only runs when a logger is
present and preserves the original reject behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aefabea9-85a9-4143-8f7b-48b4ebb6c640
📒 Files selected for processing (8)
src/commands/experiments/export.tssrc/lib/api/axios-adapter.test.tssrc/lib/api/axios-adapter.tssrc/lib/api/client.tssrc/lib/api/request-logger.test.tssrc/lib/api/request-logger.tssrc/lib/utils/api-helper.tssrc/lib/utils/download.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/api/request-logger.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/api/client.ts
- src/lib/utils/api-helper.ts
The previous suppression compared each request against the immediately previous one. Real polling loops alternate between several URLs (e.g. GET /export_configs/N then GET /.../export_histories), so consecutive fingerprints never matched and every iteration printed. Track the full set of fingerprints we've already printed in this client. A request whose fingerprint is in the set is suppressed regardless of whether it followed an exact repeat. The first new fingerprint after a run of suppressed requests still flushes a "(N identical requests suppressed)" summary line. Adds a test covering the alternating A,B,A,B,... pattern. Existing back-to-back-identical and "different params" tests still pass.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/api/axios-adapter.test.ts`:
- Around line 229-236: The helper withStderrTTY currently runs the callback
synchronously so its finally block restores process.stderr.isTTY before an async
callback has finished; make withStderrTTY async and change its signature to
accept a () => Promise<T>, capture original = process.stderr.isTTY, set the
override, then await fn() inside the try and restore the original in finally so
the TTY override remains active while the callback (and constructors like
AxiosHttpClient used inside it) execute; keep Object.defineProperty usage and
configurable: true.
In `@src/lib/api/axios-adapter.ts`:
- Around line 186-245: The request-suppression logic currently sets
config.metadata.suppressed and causes the response interceptor
(client.interceptors.response.use) to skip logging those responses; instead keep
suppression scoped to request/curl emission by only running the
fingerprint/suppression branch when this.showRequest || this.curl (or move that
block inside that conditional) and store a request-only flag (e.g.,
config.metadata.requestSuppressed) rather than config.metadata.suppressed; then
remove the early "if (response.config.metadata?.suppressed) return response" and
the error-branch checks that reject/log-skip based on suppressed so response
logging (formatResponseHTTP/formatNetworkError/formatGenericError) always runs
regardless of request suppression, while suppression notices and suppressedCount
remain tied only to request/curl output (formatSuppressionNotice,
seenFingerprints, suppressedCount, fingerprint).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db91643a-2441-435b-8875-73b7fe9a6db2
📒 Files selected for processing (2)
src/lib/api/axios-adapter.test.tssrc/lib/api/axios-adapter.ts
Useful when responses are large (e.g. paginated lists, export-history status payloads) but you only need the URL, status, and headers to debug auth, caching, or cookie-handling issues. - HTTP-block format: omits the body section entirely; status line and headers still printed. - curl format: drops the -d flag (the resulting curl is no longer a faithful reproduction, by design — the user opted in to bodyless). Threads through GlobalOptions -> AxiosHttpClientOptions -> FormatOptions (as `omitBody`) plus DownloadLoggingOptions, so it covers axios traffic and the raw http.get traffic from `experiments export --download`.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/lib/api/axios-adapter.ts (2)
188-205:⚠️ Potential issue | 🟠 MajorRequest suppression is incorrectly suppressing response logs.
metadata.suppressedis set in the request path and then used to skip response logging. This can make--show-responsemiss changing server states for repeated requests.🔧 Proposed fix
- const fp = this.fingerprint(config); - if (this.seenFingerprints.has(fp)) { - this.suppressedCount++; - config.metadata.suppressed = true; - return config; - } - if (this.suppressedCount > 0) { - process.stderr.write( - formatSuppressionNotice(this.suppressedCount, this.formatOpts.color) + '\n' - ); - this.suppressedCount = 0; - } - this.seenFingerprints.add(fp); + if (this.showRequest || this.curl) { + const fp = this.fingerprint(config); + if (this.seenFingerprints.has(fp)) { + this.suppressedCount++; + return config; + } + if (this.suppressedCount > 0) { + process.stderr.write( + formatSuppressionNotice(this.suppressedCount, this.formatOpts.color) + '\n' + ); + this.suppressedCount = 0; + } + this.seenFingerprints.add(fp); + } @@ - if (response.config.metadata?.suppressed) return response; const start = response.config.metadata?.startTime ?? Date.now(); @@ - if (error.config?.metadata?.suppressed) return Promise.reject(error); const start = error.config?.metadata?.startTime ?? Date.now();Also applies to: 224-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/api/axios-adapter.ts` around lines 188 - 205, The request interceptor is marking config.metadata.suppressed which the response interceptor then uses to skip response logging, causing --show-response to miss updates; change the metadata flag to be explicit for the request side (e.g., config.metadata.suppressedRequest or suppressedRequestFingerprint) and only use that flag to suppress printing the repeated request in the request interceptor, then update the response interceptor logic (the code around the response logging checks, currently referencing metadata.suppressed at lines ~224-233) to not skip response logs based on that flag — instead use a separate response suppression flag if you truly need response suppression. Ensure you update references to fingerprint(), this.seenFingerprints, and this.suppressedCount handling so only requests are suppressed while responses remain logged when --show-response is enabled.
316-317:⚠️ Potential issue | 🟠 MajorFingerprint serialisation can throw and prevent request dispatch.
Lines 316-317 can throw for circular structures/BigInt/functions, and this happens before the request is sent.
🔧 Proposed fix
+ private safeFingerprintPart(value: unknown): string { + if (value === undefined || value === null) return ''; + try { + return JSON.stringify(value); + } catch { + return '[unserialisable]'; + } + } + private fingerprint(config: { @@ - const params = config.params ? JSON.stringify(config.params) : ''; - const body = config.data ? JSON.stringify(config.data) : ''; + const params = this.safeFingerprintPart(config.params); + const body = this.safeFingerprintPart(config.data);#!/bin/bash set -euo pipefail echo "Fingerprint and direct serialisation in axios adapter:" rg -n "fingerprint\\(|JSON\\.stringify\\(config\\.params\\)|JSON\\.stringify\\(config\\.data\\)" src/lib/api/axios-adapter.ts -C3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/api/axios-adapter.ts` around lines 316 - 317, The JSON.stringify calls for config.params and config.data can throw (circular refs, BigInt, functions) and currently may abort the request; wrap the serialisation in try/catch when computing the local variables params and body (the const params and const body in the axios adapter) and on error fall back to a safe string (e.g., '' or a small marker like '<unserializable>') or use a non-throwing safeSerialize helper; ensure you update the code that references params/body to use these safe values so request dispatch is not prevented.src/lib/api/request-logger.ts (2)
80-85:⚠️ Potential issue | 🟠 MajorSensitive body keys can still leak when values are non-string.
Line 81 only redacts when the sensitive field value is a non-empty string. If
token/api_key/etc. is an object, array, number, or boolean, it is logged unredacted.🔧 Proposed fix
for (const [k, v] of Object.entries(data as Record<string, unknown>)) { - if (SENSITIVE_BODY_FIELDS.has(k.toLowerCase()) && typeof v === 'string' && v !== '') { - out[k] = REDACTED; - } else { - out[k] = redactBody(v, showSecrets); - } + if (SENSITIVE_BODY_FIELDS.has(k.toLowerCase())) { + out[k] = REDACTED; + continue; + } + out[k] = redactBody(v, showSecrets); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/api/request-logger.ts` around lines 80 - 85, The loop that applies redaction in request-logger uses a type-restricted check so keys in SENSITIVE_BODY_FIELDS only get redacted when the value is a non-empty string; change the logic in the for (...) block (the code referencing SENSITIVE_BODY_FIELDS, out, and redactBody) to redact any matching sensitive key regardless of the value type (e.g., remove the "typeof v === 'string' && v !== ''" requirement) by setting out[k] = REDACTED for SENSITIVE_BODY_FIELDS.has(k.toLowerCase()), and otherwise call redactBody(v, showSecrets) as before.
171-184:⚠️ Potential issue | 🟠 MajorUnsafe JSON serialisation in logging path can break requests.
Lines 183, 197, and 252 call
JSON.stringifydirectly on unknown input. If serialisation throws, logging changes runtime behaviour and can abort the request.🔧 Proposed fix
+function safeStringify(value: unknown, space?: number): string { + try { + return JSON.stringify(value, null, space); + } catch { + return '[unserialisable value]'; + } +} + function formatJsonBody(data: unknown, color: boolean): string { let json: string; @@ } else { - json = JSON.stringify(data, null, 2); + json = safeStringify(data, 2); } @@ - return typeof redacted === 'string' ? redacted : JSON.stringify(redacted); + return typeof redacted === 'string' ? redacted : safeStringify(redacted); } @@ - const body = typeof redacted === 'string' ? redacted : JSON.stringify(redacted); + const body = typeof redacted === 'string' ? redacted : safeStringify(redacted);#!/bin/bash set -euo pipefail echo "Direct JSON.stringify call-sites in request logger:" rg -n "JSON\\.stringify\\(" src/lib/api/request-logger.ts -C2 echo echo "Safe serialiser helper presence:" rg -n "safeStringify|safeSerial" src/lib/api/request-logger.tsAlso applies to: 197-197, 250-253
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/api/request-logger.ts` around lines 171 - 184, The logger calls JSON.stringify on unknown input which can throw and abort requests; add a small safeStringify(data: unknown, spacing?: number): string helper that wraps JSON.stringify in try/catch and returns a sensible fallback (e.g. a dimmed "(unserializable value)" tag or the raw toString) on error, then replace all direct JSON.stringify usages in formatJsonBody and the other logger call-sites (the three spots flagged where JSON.stringify is used) to call safeStringify(...) instead so logging cannot throw and affect request handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/api/axios-adapter.ts`:
- Around line 188-205: The request interceptor is marking
config.metadata.suppressed which the response interceptor then uses to skip
response logging, causing --show-response to miss updates; change the metadata
flag to be explicit for the request side (e.g.,
config.metadata.suppressedRequest or suppressedRequestFingerprint) and only use
that flag to suppress printing the repeated request in the request interceptor,
then update the response interceptor logic (the code around the response logging
checks, currently referencing metadata.suppressed at lines ~224-233) to not skip
response logs based on that flag — instead use a separate response suppression
flag if you truly need response suppression. Ensure you update references to
fingerprint(), this.seenFingerprints, and this.suppressedCount handling so only
requests are suppressed while responses remain logged when --show-response is
enabled.
- Around line 316-317: The JSON.stringify calls for config.params and
config.data can throw (circular refs, BigInt, functions) and currently may abort
the request; wrap the serialisation in try/catch when computing the local
variables params and body (the const params and const body in the axios adapter)
and on error fall back to a safe string (e.g., '' or a small marker like
'<unserializable>') or use a non-throwing safeSerialize helper; ensure you
update the code that references params/body to use these safe values so request
dispatch is not prevented.
In `@src/lib/api/request-logger.ts`:
- Around line 80-85: The loop that applies redaction in request-logger uses a
type-restricted check so keys in SENSITIVE_BODY_FIELDS only get redacted when
the value is a non-empty string; change the logic in the for (...) block (the
code referencing SENSITIVE_BODY_FIELDS, out, and redactBody) to redact any
matching sensitive key regardless of the value type (e.g., remove the "typeof v
=== 'string' && v !== ''" requirement) by setting out[k] = REDACTED for
SENSITIVE_BODY_FIELDS.has(k.toLowerCase()), and otherwise call redactBody(v,
showSecrets) as before.
- Around line 171-184: The logger calls JSON.stringify on unknown input which
can throw and abort requests; add a small safeStringify(data: unknown, spacing?:
number): string helper that wraps JSON.stringify in try/catch and returns a
sensible fallback (e.g. a dimmed "(unserializable value)" tag or the raw
toString) on error, then replace all direct JSON.stringify usages in
formatJsonBody and the other logger call-sites (the three spots flagged where
JSON.stringify is used) to call safeStringify(...) instead so logging cannot
throw and affect request handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ff05643-0b65-4a56-8513-c9de34465391
📒 Files selected for processing (7)
src/commands/experiments/export.tssrc/index.tssrc/lib/api/axios-adapter.tssrc/lib/api/request-logger.test.tssrc/lib/api/request-logger.tssrc/lib/utils/api-helper.tssrc/lib/utils/download.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/index.ts
- src/commands/experiments/export.ts
- src/lib/utils/download.ts
- src/lib/utils/api-helper.ts
Previously redactBody only replaced string values. A request body like
{ token: { value: "leaked-jwt" } } would recurse into the inner object
and emit `value: "leaked-jwt"` because `value` is not a sensitive key.
Numbers/arrays/objects under the same sensitive key set (token, key,
password, secret, access_token, refresh_token, api_key, apikey) all
escaped redaction.
Now any value under a sensitive key is replaced with *** unconditionally.
The presence of the key alone is enough — the type doesn't matter, and
even null/0/false are masked since "the key is present" is itself a
signal worth concealing.
Updates the existing test (which previously asserted the lax behaviour)
to lock in the new contract with object, array, number, and null cases.
…ponses The previous logic stamped config.metadata.suppressed when a duplicate request fingerprint was seen, and the response interceptor used that flag to skip logging. Two problems with that: 1. State transitions vanish from --show-response. Polling fires the same request URLs, but the response payload changes (status: "running" -> "complete"). Suppressing both directions hid exactly the part the user is debugging. 2. Running --show-response *alone* still ran the suppression branch and could emit "(N requests suppressed)" notices despite no request output ever being printed — confusing. Fix: move the fingerprint+suppression block inside the `if (this.showRequest || this.curl)` branch. Drop the metadata.suppressed flag entirely. The response interceptor now logs every response when --show-response is on, regardless of whether the matching request was suppressed for emission. Adds a test that locks in the new behaviour: --show-response alone prints no "→" lines and no "suppressed" notice. The combined-flags test now also asserts every poll response is logged.
JSON.stringify on an unknown payload could throw — circular refs, BigInts, anything else V8 rejects — and that throw landed inside the request interceptor or the response formatter, breaking the request or the response log purely because debug logging was enabled. - New safeStringify() helper in request-logger wraps JSON.stringify and returns '"[unserialisable]"' on failure. Used by formatJsonBody (for object payloads), formatBodyHTTP (non-JSON branch), and formatRequestCurl (curl -d body). - AxiosHttpClient.fingerprint() also goes through safeStringify, so a pathological config doesn't crash the suppression path. - redactBody now tracks visited objects via a WeakSet so a circular reference no longer recurses to a stack overflow before stringify is reached. Cycles render as "[circular]". Adds tests for both safeStringify directly and a circular-body request path end-to-end.
The synchronous finally restored process.stderr.isTTY before the async callback's promise had settled. The tests happened to pass because AxiosHttpClient reads isTTY in its constructor (synchronous prefix of the callback) and caches color into formatOpts. But any future test that reads isTTY after an await would silently see the original value. Make the helper async and await the callback inside try, so the override stays active for the entire callback duration.
When responses carry many headers (CORS, CSP, cache, cf-ray, etc.) and the user only wants to confirm the status code, --show-response is too noisy. --status-only collapses the response to a single line: ← 200 OK (175ms) Composes with --show-response (and --show-request, which is unaffected: requests have no "status" — use --headers-only on the request side). Threads through GlobalOptions / AxiosHttpClientOptions / FormatOptions (as `statusOnly`) and DownloadLoggingOptions, so it covers axios traffic and the raw http.get traffic from `experiments export --download`.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/utils/download.ts`:
- Around line 57-92: The request/response logging in logRequest and logResponse
currently passes raw URLs and headers to the shared formatters and can leak
presigned redirect credentials; sanitize the URL query string and the Location
response header before calling
formatRequestHTTP/formatRequestCurl/formatResponseHTTP unless logger.showSecrets
is true (e.g., replace query or entire sensitive value with a fixed "[REDACTED]"
token), ensure the sanitized URL is used for both HTTP and curl formatting and
the sanitized headers are used for response formatting, and add/adjust tests to
assert that a 302 Location with a presigned query is redacted by default but
shown when showSecrets is enabled.
- Around line 132-145: The httpGet function currently rejects on network errors
but doesn't log them, so update the req.on('error') handler in httpGet to log
the failure (including the URL and the error message/stack) before rejecting;
use the same ResolvedLogger used by logRequest/logResponse (e.g., call
logger.error or fall back to console.error/stderr when logger is absent) so the
`--show-response` debug path shows DNS/TLS/socket/redirect failures as well as
successful responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 70efd143-9c79-467c-a4a0-897af8d8be82
📒 Files selected for processing (8)
src/commands/experiments/export.tssrc/index.tssrc/lib/api/axios-adapter.test.tssrc/lib/api/axios-adapter.tssrc/lib/api/request-logger.test.tssrc/lib/api/request-logger.tssrc/lib/utils/api-helper.tssrc/lib/utils/download.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/utils/api-helper.ts
- src/lib/api/request-logger.test.ts
- src/lib/api/request-logger.ts
…n README The previous run showed `--show-request --headers-only --status-only` producing only request output: --status-only had no effect because the user hadn't also enabled --show-response. Since "status" is a response-only concept, --status-only now auto-enables --show-response in getGlobalOptions so the flag does what its name suggests on its own. Also adds a README section documenting all six debug flags, the default redaction behaviour, the suppression-on-polling logic, and a few example invocations.
The previous behaviour (per CR2 review) kept every response visible when the matching request was deduped. In practice this defeats the purpose of suppression for typical --show-response usage: polling loops that fire the same request every 2s now print the response on every iteration, drowning out the actual interesting events. Restore the original pair-suppression model: when a request's fingerprint matches a previous one, both the request emission AND the response emission are skipped, and the "(N identical requests suppressed)" notice flushes when the next distinct request arrives. Trade-off acknowledged: state changes that show up only in the body of an otherwise-identical request will be hidden. The user can vary the request, run a separate invocation, or accept the noise loss. This was an explicit user call. Updates two tests to lock in the new behaviour: - showRequest+showResponse with 3 identical polls + 1 done: now 2 responses (was 4). - showResponse alone: now suppresses repeats too and emits a suppression notice (was: every response logged, no notice).
CR5: --show-request / --curl / --show-response could leak live download
credentials. The export-download flow follows a 302 redirect to object
storage with a presigned query string (X-Amz-Signature, X-Amz-Credential,
etc.); the redirect target URL went straight into the formatters and
the 302 Location response header was emitted verbatim.
* New redactUrl() helper: when the query string contains any of a
known set of presigned-credential param names (AWS X-Amz-*, GCP
X-Goog-*, signature, sig, token, auth, access_token), the entire
query is replaced with '?[redacted-presigned]'. Whole-query
(not just the matched key) because the surrounding params are
part of the signed bundle and a partial reveal can still
attribute the URL.
* formatRequestHTTP / formatRequestCurl now run the URL through
redactUrl before emission.
* redactHeaders treats Location and Content-Location as URL-bearing
and runs them through redactUrl too. --show-secrets disables
both redactions.
CR6: req.on('error') in the raw http.get download path used to reject
silently. DNS / TLS / socket failures and redirect loops never produced
an HTTP response and stayed invisible under --show-response. Now logs
via formatGenericError, matching the axios path's behaviour for
network-level errors.
Adds 7 new tests (presigned URL detection, Location-header redaction,
request-URL redaction).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--show-request— outgoing requests as HTTP-style blocks--show-response— responses (success and error) as HTTP-style blocks--curl— outgoing requests as runnable curl commands--show-secrets— opt out ofAuthorizationredaction (default isApi-Key ***/Bearer ***, scheme prefix preserved)chalk+cli-highlight); a forcedChalkinstance keeps coloring deterministic regardless of stdout vs stderr routing.axios-retryretries and401 → onExpired → retryflows emit one log entry per attempt.-v/--verboseflag is unchanged.Example
Test plan
bun run test:run— 2214 passed / 4 skipped / 0 failed (added 40 new tests acrossrequest-logger.test.tsandaxios-adapter.test.ts)bun run lint— cleanbun run typecheck— cleantest-1profile:--show-request,--show-response,--curl,--show-secretsbehave as designed; redaction is on by default; 4xx responses are dumped before the existing error path runs.abs --show-request --curl <any-command>in a TTY to confirm the colored output.--show-secretsis the only path that reveals the API key/JWT.Summary by CodeRabbit
New Features
Tests
Chores