Skip to content

Added extra debug when doing kosli attest pullrequest github if the debug flag is set#839

Merged
ToreMerkely merged 10 commits intomainfrom
githbub-extra-debug
Apr 30, 2026
Merged

Added extra debug when doing kosli attest pullrequest github if the debug flag is set#839
ToreMerkely merged 10 commits intomainfrom
githbub-extra-debug

Conversation

@ToreMerkely
Copy link
Copy Markdown
Contributor

  • debug(github): log GitHub HTTP requests/responses under --debug
  • fix(github): silence errcheck on debug Fprintf calls
  • ci: fix Debug build trigger to match this branch name
  • ci: debug-build runs on any branch and produces all platforms
  • ci: split debug-build into one artifact per platform
  • ci: write each artifact URL to the run summary
  • ci: also expose nightly.link URLs in debug-build summary
  • debug(github): redact Cookie/Set-Cookie/Proxy-Authorization too
  • ci: remove temporary debug-build workflow

@ToreMerkely ToreMerkely enabled auto-merge (squash) April 30, 2026 13:38
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 30, 2026

Claude finished @ToreMerkely's task in 1m 58s —— View job


PR Review: Extra debug logging for GitHub API calls

  • Run static analysis (build, vet, mod tidy)
  • Review changed files
  • Post review findings

Static Analysis

All three checks pass cleanly:

  • go build ./... — compiles
  • go vet ./... — no warnings
  • go mod tidy — no diff

Review Summary

This is a well-scoped PR. It threads a debug bool through the GitHub client stack and wraps the HTTP transport to dump requests/responses to stderr, with credential redaction. The previous review round's test coverage suggestion was addressed — TestRedactAuthHeader and TestRedactSensitiveHeader have solid branch coverage.

No blocking issues found. A few observations below.

What's done well

  • Clean transport wrappingdebugTransport composes via http.RoundTripper, keeping it isolated from production paths.
  • Redaction is thoughtful — Authorization keeps last 4 chars for diagnostics; cookies and proxy-auth are fully redacted. The doc comments explain the "why" behind each choice.
  • logf silences errcheck correctly_, _ = fmt.Fprintf(...) is the right idiom for intentionally-ignored write errors on debug output.
  • Good test coverage — Both redaction functions have table-driven tests covering boundary conditions (empty, short, exactly-4-char tokens, case-insensitive header matching, pass-through for non-sensitive headers). The X-OAuth-Scopes intentional non-redaction is explicitly documented in a test case.
  • Signature changes are mechanically consistent — all callers of NewGithubConfig, NewGithubClientFromToken, NewGithubRetrieverFunc, and defaultNewGithubRetriever have been updated, including test fakes and contract tests.

Minor observations (non-blocking)

  1. Response body on ReadAll error path (github.go:130-135): When io.ReadAll(resp.Body) fails, the code replaces resp.Body with an empty NopCloser. This is a sensible defensive choice — it prevents the caller from reading a closed body — but it does silently transform the response. In practice, response bodies from the GitHub API are in-memory buffers from the TLS layer, so ReadAll failure here is extremely unlikely. Just calling it out as a known trade-off, not something to change.

  2. Request body on ReadAll error path (github.go:106-116): If io.ReadAll(req.Body) fails, the request body is closed but not restored. The subsequent base.RoundTrip(req) would see a nil/closed body. As the author noted in review discussion, this is effectively unreachable for in-memory request bodies, and the current behavior (surfacing the error to the caller) is actually more correct than silently substituting http.NoBody.

  3. Header iteration order: req.Header / resp.Header map iteration is non-deterministic in Go. Debug output for the same request may show headers in different orders across runs. This is fine for human-readable debug logs — just noting it for completeness.

This PR is clean and ready to merge. The review discussion from the previous round was handled well — the author made good calls on which suggestions to accept (test coverage) and which to skip (body truncation, io.Writer plumbing, X-OAuth-Scopes redaction), with clear reasoning.

Comment thread internal/github/github.go
Comment thread internal/github/github.go
Comment thread internal/github/github.go
Comment thread internal/github/github.go
Comment thread internal/github/github.go
Comment thread internal/github/github.go
Comment thread internal/github/github.go
Comment thread internal/github/github.go
ToreMerkely and others added 10 commits April 30, 2026 16:20
Adds a debug http.RoundTripper to the github client so every REST and
GraphQL call (request method/URL/headers/body and response status/headers/body)
is dumped to stderr with the [debug-github] prefix when --debug is set.
Authorization header is redacted to the last 4 chars to make CI logs safe
to share. Wires global.Debug through NewGithubRetrieverFunc / NewGithubConfig
/ NewGithubClientFromToken and updates affected tests.

Also adds .github/workflows/debug-build.yml to build the CLI on this
branch and upload it as the kosli-debug artifact for ad-hoc debugging.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI golangci-lint flagged the Fprintf calls in the debug RoundTripper
because their error returns weren't checked. Wrap them in a small logf
helper that explicitly discards the error — write failures to a debug
stderr stream are not actionable.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Trigger on every push (and workflow_dispatch). Cross-compile for the
five platforms GoReleaser produces (linux/darwin/windows × amd64/arm64
where applicable) and bundle them into a single kosli-debug artifact.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Use a matrix so each platform builds in its own job and uploads a
separate kosli-debug-<os>-<arch> artifact, instead of one large zip.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
upload-artifact@v4+ returns an artifact-url output. Append a markdown
link for each platform's artifact to $GITHUB_STEP_SUMMARY so the run
page shows direct download links instead of having to dig into the
Artifacts panel.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a second public-download link alongside each GitHub artifact link.
nightly.link is an OAuth proxy that lets anyone download a public
repo's Actions artifacts without signing in to GitHub.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The debug RoundTripper was only redacting Authorization. Add Cookie,
Set-Cookie, and Proxy-Authorization to the redaction list so a corporate
proxy password (Proxy-Authorization is commonly Basic <base64(user:pass)>
in HTTPS_PROXY setups) doesn't leak into shared debug logs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This workflow was added to produce ad-hoc kosli-debug binaries while
debugging the GitHub auth issue. The debug logging itself remains in
the github transport.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
These are pure security-relevant helpers in the debug RoundTripper.
Add table tests covering the branches: scheme + long/short/4-char
token, no-scheme long/short/empty values, the case-insensitive
Authorization match, the fully-redacted families (Cookie, Set-Cookie,
Proxy-Authorization), pass-through for non-sensitive headers, and
explicit assertion that X-OAuth-Scopes is intentionally not redacted
(useful for diagnosing GitHub permission failures).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ToreMerkely ToreMerkely force-pushed the githbub-extra-debug branch from c8b9d38 to ff4cef4 Compare April 30, 2026 14:20
@ToreMerkely ToreMerkely merged commit fad5e49 into main Apr 30, 2026
16 checks passed
@ToreMerkely ToreMerkely deleted the githbub-extra-debug branch April 30, 2026 14:28
github-actions Bot pushed a commit that referenced this pull request Apr 30, 2026
… if the debug flag is set (#839)

* debug(github): log GitHub HTTP requests/responses under --debug

Adds a debug http.RoundTripper to the github client so every REST and
GraphQL call (request method/URL/headers/body and response status/headers/body)
is dumped to stderr with the [debug-github] prefix when --debug is set.
Authorization header is redacted to the last 4 chars to make CI logs safe
to share. Wires global.Debug through NewGithubRetrieverFunc / NewGithubConfig
/ NewGithubClientFromToken and updates affected tests.

Also adds .github/workflows/debug-build.yml to build the CLI on this
branch and upload it as the kosli-debug artifact for ad-hoc debugging.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(github): silence errcheck on debug Fprintf calls

CI golangci-lint flagged the Fprintf calls in the debug RoundTripper
because their error returns weren't checked. Wrap them in a small logf
helper that explicitly discards the error — write failures to a debug
stderr stream are not actionable.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci: fix Debug build trigger to match this branch name

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci: debug-build runs on any branch and produces all platforms

Trigger on every push (and workflow_dispatch). Cross-compile for the
five platforms GoReleaser produces (linux/darwin/windows × amd64/arm64
where applicable) and bundle them into a single kosli-debug artifact.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci: split debug-build into one artifact per platform

Use a matrix so each platform builds in its own job and uploads a
separate kosli-debug-<os>-<arch> artifact, instead of one large zip.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci: write each artifact URL to the run summary

upload-artifact@v4+ returns an artifact-url output. Append a markdown
link for each platform's artifact to $GITHUB_STEP_SUMMARY so the run
page shows direct download links instead of having to dig into the
Artifacts panel.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci: also expose nightly.link URLs in debug-build summary

Add a second public-download link alongside each GitHub artifact link.
nightly.link is an OAuth proxy that lets anyone download a public
repo's Actions artifacts without signing in to GitHub.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* debug(github): redact Cookie/Set-Cookie/Proxy-Authorization too

The debug RoundTripper was only redacting Authorization. Add Cookie,
Set-Cookie, and Proxy-Authorization to the redaction list so a corporate
proxy password (Proxy-Authorization is commonly Basic <base64(user:pass)>
in HTTPS_PROXY setups) doesn't leak into shared debug logs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci: remove temporary debug-build workflow

This workflow was added to produce ad-hoc kosli-debug binaries while
debugging the GitHub auth issue. The debug logging itself remains in
the github transport.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(github): cover redactAuthHeader and redactSensitiveHeader

These are pure security-relevant helpers in the debug RoundTripper.
Add table tests covering the branches: scheme + long/short/4-char
token, no-scheme long/short/empty values, the case-insensitive
Authorization match, the fully-redacted families (Cookie, Set-Cookie,
Proxy-Authorization), pass-through for non-sensitive headers, and
explicit assertion that X-OAuth-Scopes is intentionally not redacted
(useful for diagnosing GitHub permission failures).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants