- Replace panic() with fmt.Fprintf+os.Exit(1) in provider switch default
(repo convention: never panic)
- Remove spurious 'event' field from DismissReview payload (GitHub dismiss
endpoint only documents 'message')
- Change translateReviewEvent default to return 'COMMENT' as canonical
fallback instead of passing unknown events through to GitHub API
- Refactor supersedeOldReviews to use explicit switch/case with default
error for exhaustiveness
- Replace interface{} with any in github/reviews.go (Go 1.18+ idiom)
- Add default panic case to VCS client init switch
- Refactor supersedeOldReviews to return error instead of os.Exit(1)
- Remove spurious blank lines in formatter.go and formatter_test.go
- Add doc comment to DeleteReview explaining when to use vs DismissReview
- Sanitize extractSentinelName output to prevent log injection
- Add --provider flag (gitea|github) for VCS backend selection
- Add --base-url flag for GitHub API endpoint configuration
- Rename --gitea-url to --vcs-url with backward-compatible alias
- Replace direct gitea.Client usage with vcs.Client interface
- Create vcs.Client via factory switch based on --provider value
- Implement Reviewer + Identity interfaces on github.Client
- Add verdictToEvent() using canonical vcs.ReviewEvent types
- Remove review.GiteaEvent() (replaced by verdictToEvent)
- GitHub supersede uses DismissReview; Gitea keeps EditComment flow
- Add VCS_PROVIDER, VCS_BASE_URL, VCS_URL env var support
Closes#82
Update the approved dependency table to document go-yaml subpackage
usage (ast, parser) and remove the deviation comment now that the
proper allowlist process is being followed.
Closes#91
- Clarify depth-aware short-circuit comment to unambiguously describe
the relationship between current depth and previous validation depth
- Add comment to MappingValueNode case explaining intentional depth+2
behavior from parent MappingNode perspective
- Restructure unmarshalYAMLWithDepthLimit doc comment as bullet list
covering all three safety checks (depth, multi-doc, strict fields)
- Replace t.Error with t.Fatal in TestYAMLEmptyFileRejection to remove
redundant nil guard on subsequent err.Error() call
- Add explicit case for *ast.MergeKeyNode in checkYAMLDepth switch to
make it clear this is an intentional leaf (no children to recurse)
rather than relying on the default case. Prevents future library
changes from silently bypassing depth checks.
- Add MaxPersonaFileSize bound check at the top of ParsePersonaBytes.
While callers already check size, the public API should defend itself
(defense in depth) against arbitrarily large inputs that could cause
excessive memory/CPU before AST validation runs.
- Add tests for both behaviors.
Addresses review #2879 findings.
- Add safety note on Strict() decoder not expanding aliases recursively,
since alias resolution uses the pre-validated AST (finding #1)
- Document that ast.Node map keys rely on pointer identity, which holds
because all goccy/go-yaml AST types are pointer receivers (finding #2)
- Clarify AnchorNode comment: effective depth budget is reduced for
anchor+alias pairs, not literally halved (finding #3)
- Improve test depth trace comment for accuracy (finding #4)
- Add HTML comment in CONVENTIONS.md referencing #91 for the two-step
process deviation (finding #5)
- Fix validated map comment: says 'minimum depth' but stores the maximum
depth at which a node was validated (overwritten on deeper visits).
- Replace dec.More() with explicit dec.Decode check for trailing JSON
content. More() is documented for use inside arrays/objects; the
explicit EOF check is clearer at the top-level stream.
MINOR fixes:
- docs/DESIGN-57-yaml-persona.md: fix Error Cases table entry to reflect
custom AST walk (checkYAMLDepth) instead of stale library-level reference
- review/persona.go: add EOF check after JSON decode to reject trailing
garbage after a valid JSON object (prevents silent acceptance of malformed
input like '{"name":"x"}garbage')
- review/persona_test.go: add TestJSONTrailingContentRejected test
NIT fixes:
- review/persona.go: add default case to checkYAMLDepth switch with
explanatory comment about scalar leaf nodes
- review/persona.go: document AnchorNode depth+1 conservative asymmetry
- review/persona.go: simplify redundant if-guard in ListBuiltinPersonas
- Move nodeCount increment after cycle detection to avoid over-counting
cyclic references (sonnet #2)
- Use underscores in test case names used as filenames (sonnet #3)
- Fix function comment: 'prevent silent data loss' → 'prevent confusing
behavior where additional documents are silently ignored' (sonnet #4)
- Mark design doc pseudocode as historical since implementation uses
goccy/go-yaml ast.Node, not gopkg.in/yaml.v3 yaml.Node (sonnet #5)
The global 'seen' set allowed anchored subtrees validated at a shallow
depth to be skipped when later referenced via alias at a greater depth.
This could let effective nesting exceed MaxYAMLDepth, enabling DoS.
Fix: replace the single 'seen' set with two tracking maps:
- validated (node -> min depth): only short-circuits when current depth
<= previously validated depth; re-checks at deeper contexts.
- visiting (node -> bool): per-path recursion stack for true cycle
detection (breaks alias loops without suppressing depth checks).
Add TestYAMLAliasDepthBypass that constructs a document with an
anchored 15-level subtree referenced via alias under 6 levels of
nesting, verifying the combined effective depth (22) is rejected.
Addresses security-review-bot findings on review #2774.
Fixes#87.
PR #58 incorrectly added gopkg.in/yaml.v3 (abandoned library) instead of
github.com/goccy/go-yaml as required by issue #57.
Changes:
- Replace gopkg.in/yaml.v3 with github.com/goccy/go-yaml v1.19.2
- Update review/persona.go to use goccy/go-yaml API:
- parser.ParseBytes for AST-based depth/node count checking
- yaml.Strict() decoder option instead of KnownFields(true)
- ast.Node types instead of yaml.Node for tree walking
- Update review/persona_test.go to use ast types for cycle tests
- Remove gopkg.in/yaml.v3 from go.mod and go.sum
All existing YAML tests pass with the new library.
- Add comment in DismissReview explaining why the Event field is required
by the GitHub API even though DISMISS is the only valid value (#18652).
- Change t.Errorf to t.Fatalf for method/path routing assertions in test
handlers so failures are immediately fatal instead of silently
continuing handler execution (#18653).
Address review findings from sonnet-review-bot (review 3086):
- PostReview now returns ErrConflictingCommitIDs when comments specify
different non-empty CommitIDs, since the GitHub API accepts only a
single commit_id per review. Previously the discrepancy was silently
ignored, using only the first commit's ID.
- Extract newTestClient into helpers_test.go to make cross-file sharing
between review_test.go and identity_test.go explicit.
Refs: #81
Address review feedback from round-3 sonnet review:
- PostReview doc comment now accurately describes vcs.ReviewEvent → GitHub
wire-format string cast and notes nil-Comments omitempty behavior.
- Rename 'expected' field to 'want' in TestTranslateGitHubReviewState to
match the project's established naming convention.
- Wrap ErrCannotDeleteSubmittedReview with operation context via fmt.Errorf
so callers get both sentinel identity and context (MINOR fix)
- Combine double iteration in PostReview into single loop (NIT)
- Remove extra trailing blank line in review_test.go (NIT)
- Clarify translateGitHubReviewState comment re: PENDING state (NIT)
- Update requestOptions.bodyFn comment to mention DELETE-with-body (NIT)
- MAJOR: Extract doRequestCore to eliminate doRequest/doRequestWithBody
duplication. Both now delegate to a shared implementation with the
retry/backoff logic in a single place.
- MINOR: Replace custom containsStr/containsSubstring helpers with
strings.Contains in review_test.go.
- MINOR: Use http.Method* constants (MethodPost, MethodDelete, MethodPut)
in review.go for consistency with doGet.
- MINOR: Remove redundant APPROVED/DISMISSED cases from
translateGitHubReviewState that were identical to the default passthrough.
- NIT: Clarify DeleteReview comment about COMMENTED being a GitHub API
state name.
- DismissReview Event field verified as required by GitHub API docs;
kept as-is.
Implement the remaining vcs.Client interface methods for github.Client:
Reviewer:
- PostReview: POST /repos/{owner}/{repo}/pulls/{number}/reviews
- ListReviews: GET /repos/{owner}/{repo}/pulls/{number}/reviews
with state translation (CHANGES_REQUESTED → REQUEST_CHANGES, etc.)
- DeleteReview: DELETE /repos/{owner}/{repo}/pulls/{number}/reviews/{id}
Returns ErrCannotDeleteSubmittedReview on 422
- DismissReview: PUT /repos/{owner}/{repo}/pulls/{number}/reviews/{id}/dismissals
Identity:
- GetAuthenticatedUser: GET /user
Infrastructure:
- Add doRequestWithBody helper for POST/PUT/DELETE with JSON bodies
- Update conformance_test.go: var _ vcs.Client = (*github.Client)(nil)
All unit tests pass including error cases (401, 404, 422, malformed).
Adds TestTranslate_HunkHeaderAtEnd covering the edge case where a
hunk-header is the last position in the file with no subsequent
new-file line. Mirrors TestBuildPositionToLineMap_DeletionAtEnd for
the hunk-header code path.
Addresses NIT from sonnet-review-bot on PR #104 (comment 18412).
BuildPositionToLineMap incremented position and updated maxPositions for
@@ hunk-header lines but did not store a map entry, causing Translate()
to return a hard error for any comment positioned at a hunk header.
Store sentinel value 0 for hunk-header positions (analogous to -1 for
deletions) and extend Translate() to fall through to the nearest
context/addition line below, matching the existing deletion-line
behavior.
Fixes#97
Pass 2 elements to SetRetryBackoff (matching maxRetryAttempts-1 = 2)
and check the error return. Previously passing 1 element silently
failed, causing tests to fall back to default {1s, 2s} backoffs.
Fixes self-review finding: 429Retry tests now run in <10ms instead
of ~1s.
Implement FileReader conformance on the GitHub client: GetFileContent,
ListContents, path helpers, base64 decode. Includes compile-time
conformance checks for both PRReader and FileReader.
Requires PR B (#102). Part 3 of 3 for #80.
- Add maxFileContentSize (10 MB) limit to decodeBase64Content to prevent
resource exhaustion from oversized file content (security MINOR)
- Fix reversed NewClient arg order in TestGetFileContentAtRef_DotSegmentError
(GPT MINOR + Sonnet NIT)
- Remove 'waiting' from mapCheckRunStatus conclusion cases since it is a
status value not a conclusion, update comment (GPT NIT)
- Add TestDecodeBase64Content_SizeLimit test
- escapePath now returns an error when paths contain dot-segments
(".", "..") instead of silently rewriting them. This prevents
subtle API misses where callers pass "foo/../bar" expecting to
hit "bar" but the old code produced "foo/bar".
- Uses path.Clean for canonical form after validation.
- CommitStatus.Description for check runs is now empty string
instead of the raw conclusion enum. The conclusion is already
captured in the Status field via mapCheckRunStatus; storing it
again in Description was semantically inconsistent with commit
statuses where Description carries a human-readable narrative.
- Removed unused derefString helper.
- Added tests for escapePath valid paths, dot-segment rejection,
and GetFileContentAtRef dot-segment error propagation.
- Separate maxPages into maxFilesPages and maxCheckRunPages constants
for clarity (sonnet MINOR #1)
- Add parallel to CheckRunConclusions subtests (sonnet MINOR #2)
- Add TestGetCommitStatuses_CheckRunsErrorAfterStatusesSucceed test
covering check-runs 500 after statuses succeed (sonnet MINOR #2)
- Expand mapCheckRunStatus doc comment with full mapping rules including
cancelled/skipped/neutral rationale and unknown value behavior
(sonnet MINOR #3, gpt MINOR #1)
- Expand GetPullRequest doc comment to mention error types returned
(sonnet NIT #4)
- Add inline comment on Description field clarifying it holds raw
conclusion value (gpt NIT #3)
- Implement GetFileContentAtRef on *Client to satisfy vcs.PRReader interface
- Add escapePath and decodeBase64Content helpers
- Fix conformance_test.go to properly import and qualify github.Client
(was using unqualified Client in package github_test)
Fixes CI failure: the PRReader interface requires GetFileContentAtRef
but it was missing from this PR (only present in the file-reader PR).
- MAJOR #1: Replace panic in doRequest with safe default fallback.
Validation now happens in SetRetryBackoff (returns error on invalid
length). doRequest gracefully falls back to default backoff if the
configured slice is somehow invalid.
- MINOR #2: SetRetryBackoff validates slice length at configuration
time, making the coupling between maxRetryAttempts and backoff
explicit and catching mismatches early with a clear error.
- MINOR #4: Reword oversized response error to remove '(truncated)'
which implied truncated data was returned when actually only an
error is returned.
- MINOR #5: Functional options kept as-is - idiomatic Go pattern
that allows future growth without breaking the API.
Address review feedback on PR #101:
1. Capture resp.StatusCode and Retry-After header *before* passing resp
to handleResponse, making ownership transfer explicit. Previously the
caller read resp.StatusCode after handleResponse had closed the body —
correct but fragile coupling.
2. Add panic guard ensuring backoff slice length equals maxAttempts-1.
Previously the relationship was implicit and could silently break if
maxAttempts were changed without updating the default backoff.
Add GitHub API client with configurable base URL and GHE support,
HTTP helpers with 429 retry and Retry-After handling.
Also adds Patch field to vcs.ChangedFile.
Part 1 of 3 for #80.
Add fmt.Errorf wrapping to the two remaining unwrapped error returns
in the adapter:
- GetPullRequest: 'get pull request: %w'
- PostReview (final client call): 'post review: %w'
This makes all error paths in the adapter consistent with the wrapping
pattern used by the diff-fetch and position-translation errors.
Addresses self-review findings #1 and #2 from b2eea502.
- position.go: Replace O(n) maxPosition scan with O(1) lookup by
tracking max position during map construction. Also eliminates
shadowing of the builtin max identifier (Go 1.21+).
- position.go: Add comment clarifying +++ prefix ordering intent.
- adapter.go: Document diff-fetch tradeoff in PostReview.
- adapter_test.go: Remove extra blank line between test functions.
- Remove unused error return from BuildPositionToLineMap (always nil)
- Add comment explaining intentional CommitID drop in PostReview
- Refactor TestAdapter_PostReview_WithComments to route by URL path
- Add TestAdapter_GetFileContent_RefRouting test
- Acknowledge maxPosition O(n) with code comment
- Remove redundant TestAdapter_CompileTimeCheck (compile-time var _ exists)
- Fix GetPullRequestFiles comment (Patch field is omitted, not 'set to empty')
- Acknowledge translateEvent fallback as intentional design
Implements the Gitea adapter (gitea.Adapter) that satisfies vcs.Client.
Key components:
- gitea/adapter.go: Adapter struct wrapping *Client with all vcs.Client methods
- gitea/position.go: BuildPositionToLineMap for diff-position → line translation
- gitea/adapter_test.go: Tests for all mapping methods and event translation
- gitea/position_test.go: Tests for position translation edge cases
Translation details:
- ReviewEvent: APPROVE → APPROVED (Gitea-native)
- PostReview: fetches diff, builds position map, translates each comment
- Deletion-targeted positions map to nearest non-deletion line below
- All field-mapping methods tested (GetPullRequest, GetPullRequestFiles,
ListReviews, GetCommitStatuses, ListContents)
Also:
- Added Base field to gitea.PullRequest struct
- Updated conformance tests to assert Adapter (not raw Client) satisfies vcs.Client
- Removed phase2 build tag from conformance tests
Closes#79
claw
rodin
aweiker