Self-Review: PR #106
Self-review against 5c7c7d2250552f4d843f4c76fc47851690baf35e
Phase 1: Independent Findings
No new issues found. Changes are minimal and well-scoped:
- **--gitea-url…
Self-Review: PR #105
Self-review against dbc25f4e8a423bb76091c208271e4f8d87395543
Phase 1: Independent Findings
Reviewing the diff at HEAD (dbc25f4e) as a fresh set of eyes:
**1. [NIT]…
Pushing back on this one: Bearer is the correct and standard scheme per RFC 6750. GitHub's own docs recommend it for fine-grained personal access tokens and GitHub App tokens. The token scheme is a GitHub-specific legacy convention that predates their OAuth2 adoption. Modern GitHub tokens (fine-grained PATs, GitHub App installation tokens) all use Bearer. No change needed.
Confirmed: doGet delegates to doRequest, which enforces the same HTTPS-only check (lines 219-226 of client.go) when c.token != "" && !c.allowInsecureHTTP. DeleteReview also uses doRequest. All credential-bearing HTTP methods share the same HTTPS enforcement. No gap exists in this PR or in the existing code.
Confirmed: doRequest (which doGet and DeleteReview use) already enforces the identical HTTPS-only check at lines 219-226 of client.go. doJSONRequest has its own copy because it handles the request lifecycle differently (JSON marshaling, bytes.NewReader, content headers) and doesn't route through doRequest. The security posture is consistent across all HTTP methods — no gap exists.
Added: // All comments in a single review are expected to reference the same commit. — fixed in 7d6fe27.
Added a comment explaining the shared *string pointer mechanism. flag.StringVar(vcsURL, ...) binds --gitea-url to the same underlying *string as --vcs-url, so there's no divergence risk — both flags always resolve to the same value after flag.Parse(). Comment added in 7d6fe27.
Updated the inline comment from "hidden alias" to "backward-compatible alias" in 7d6fe27. The flag does appear in --help output (Go's flag package doesn't support hiding flags), so "hidden" was inaccurate. "Backward-compatible" better describes the intent.
Good catch — reworded from "Fall through to Gitea-specific logic below the switch" to "Continue to Gitea-specific logic below the switch" in 7d6fe27. The term "fall through" is misleading in Go context since it implies the fallthrough keyword.
Pushing back on this one. The current approach works correctly, is tested, and the nil-body contract is already documented via the inline comment added in a prior revision (// nil body: the GitHub DELETE endpoint for reviews requires no request body.). Additionally, doGet does not accept a method parameter — it always uses GET — so refactoring to use it for DELETE is not trivial without introducing a new helper. Leaving as-is.
Fixed in dbc25f4 — changed all t.Errorf routing assertions (method and path checks) in test handlers to t.Fatalf so failures are immediately fatal instead of continuing handler execution with an incorrect request.
Fixed in dbc25f4 — added a comment explaining that the GitHub API requires the Event field for dismissal requests even though DISMISS is the only valid value.
Self-Review: PR #108
Self-review against bf52fceea048d4b2bdfb2b0489fad8b2f155c7fe
Phase 1: Independent Findings
None — diff looks clean.
The change is documentation-only: two targeted…