Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44cf04b0e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case string: | ||
| return secretValueRe.ReplaceAllString(value, "[REDACTED]") |
There was a problem hiding this comment.
When -vv logs a response containing a credential-bearing URL, this branch only redacts Two API-key-shaped substrings, so the credential remains in stderr. This occurs in the repository’s own PaymentLinkResponseSchema, whose payment_link example is https://checkout.two.inc/payment/initiation?token=abc123 (openapi/checkout-api.yaml:5560-5572): the key does not match the sensitive-name regex and abc123 does not match secretValueRe. Parse URL-valued strings and redact sensitive query parameters (or otherwise recognize embedded credential URLs) before emitting the supposedly redacted diagnostic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9c387b5. Body string redaction now parses URL values, redacts sensitive query parameters, userinfo and fragments, and fails closed on malformed URLs. Tests cover nested JSON, plain URLs and the real payment-link token example. CI passed on that head. The separate header-value finding is still open and is being addressed independently.
✨ via Codex session
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c387b5336
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if sensitiveHeaderRe.MatchString(key) { | ||
| out[key] = "[REDACTED]" | ||
| } else { | ||
| out[key] = secretValueRe.ReplaceAllString(out[key], "[REDACTED]") |
There was a problem hiding this comment.
When -vv processes a request or response header such as Location: https://checkout.two.inc/payment/initiation?token=abc123, the header name does not match sensitiveHeaderRe and the ordinary token does not match secretValueRe, so the credential is emitted to stderr unchanged. Apply the URL-aware redaction already used for body strings to non-sensitive-named header values before logging them.
Useful? React with 👍 / 👎.
Context
PLAT-1314 specifies opt-in HTTP diagnostics:
-vfor timings and request IDs on successful and failed calls, and-vvfor redacted request/response details. twoctl currently captures request IDs only in HTTP errors and has no verbosity flag.Changes
-v/--verboseJSON diagnostics on stderr with method, URL, total duration, status, request ID, and transport/body-read errors. Pagination emits a diagnostic per page.-vvrequest/response headers and bodies, redacting sensitive headers/query/JSON values and API-key-shaped strings while preserving numeric precision.Scope / Non-goals
Durations include any retries and response-body reading; this does not change retry policy, credentials, or upstream API behavior. It starts from refreshed main including #9 and is independent of the current retry/context/keychain/version PRs. Existing Bharat ownership and recent activity/branch/PR checks found no competing diagnostics implementation.
Validation
On exact head
9c387b5336c5e4993ae3a0ef7785e6d750dbd8c9, CI run 40862346375 passed both jobs:go build ./..., embedded-spec drift verification, and all tests throughmake test-coverpassed, including the 80% coverage gate.Ticket
Refs PLAT-1314.
✨ via Codex session