Upstream fixes#30
Open
supasympa wants to merge 21 commits into
Open
Conversation
Prevent 422 'Path could not be resolved' errors when creating review comments on files that don't exist in the PR's diff (e.g., deleted files). - Add ListPRFiles() to fetch PR's changed files - Filter buildReviewComments() to only include files present in PR diff - Add tests for groupFindings and buildReviewComments
- TestReport_RetryWithoutCommentsOnPositionError: verifies retry logic - TestReport_FallsBackToStickyCommentWhenBothReviewAttemptsFail - TestReport_SuccessfulReviewOnFirstTry - TestReport_UsesPRFilesFilter Also converts GitHubReporter.client to use interface for testability.
- Add GetPRFileDiffs() to fetch file diffs with line counts - Add validateLine() to cap line numbers to actual changed lines - buildReviewComments now validates line numbers against actual diff - Skip comments for invalid/outsized line numbers instead of failing This should fix 'Line could not be resolved' errors when AI reports line numbers beyond the actual changes in the diff.
There was a problem hiding this comment.
acig: warn | risk=medium | 14 finding(s) | $0.0006
| Severity | Critic | Title | File |
|---|---|---|---|
| info | risk_classifier | Public GitHub App ID in Workflow | .github/workflows/acig.yml |
| info | risk_classifier | Dynamic ACIG Version Retrieval | .github/workflows/acig.yml |
| low | risk_classifier | New GitHub Client Interface | internal/reporters/github.go |
| low | risk_classifier | PR File Diff Parsing | internal/githubclient/review.go |
| low | risk_classifier | Comment Line Validation | internal/reporters/github.go |
| medium | test_coverage_smell | Missing tests for ListPRFiles | internal/githubclient/review.go |
| medium | test_coverage_smell | Missing tests for GetPRFileDiffs | internal/githubclient/review.go |
| medium | test_coverage_smell | Missing tests for validateLine | internal/reporters/github.go |
| medium | test_coverage_smell | buildReviewComments filtering by PR files not tested | internal/reporters/github.go |
| medium | test_coverage_smell | buildReviewComments line validation logic not exercised | internal/reporters/github.go |
| medium | test_coverage_smell | Reporter CreateReview error handling not fully tested | internal/reporters/github.go |
| medium | test_coverage_smell | Sticky comment fallback formatting not verified | internal/reporters/github_report_test.go |
| medium | test_coverage_smell | buildReviewComments behavior when fileDiffs is nil | internal/reporters/github.go |
| info | adjudicator | Failed to parse critic output | — |
Verdict JSON
{"schema_version":"1","repo":"https://github.com/helloodokai/acig","sha":"cb6f9f4ed0d91d40639dbbfe13139950f01290c8","base_sha":"main","risk":"medium","decision":"warn","summary":"Decision: warn | Risk: medium | Findings: 0 blocking, 0 high, 8 medium, 3 low, 3 info | Cost: $0.0006","findings":[{"critic":"risk_classifier","severity":"info","title":"Public GitHub App ID in Workflow","detail":"The workflow now uses a public variable for the GitHub App ID instead of a secret. This is acceptable as the ID is public, but the change should be documented to avoid accidental exposure of sensitive values.","file":".github/workflows/acig.yml","line_start":5,"line_end":12},{"critic":"risk_classifier","severity":"info","title":"Dynamic ACIG Version Retrieval","detail":"The workflow fetches the latest ACIG release via curl and parses the tag. While this introduces a network dependency, it does not expose secrets and is unlikely to cause a security issue.","file":".github/workflows/acig.yml","line_start":15,"line_end":22},{"critic":"risk_classifier","severity":"low","title":"New GitHub Client Interface","detail":"A new interface abstracts GitHub operations, improving testability. No unsafe operations or external dependencies are introduced.","file":"internal/reporters/github.go","line_start":20,"line_end":45},{"critic":"risk_classifier","severity":"low","title":"PR File Diff Parsing","detail":"The code parses patch diffs using a custom diff package. The input comes from GitHub's API, which is trusted, and no file I/O or unsafe parsing is performed.","file":"internal/githubclient/review.go","line_start":30,"line_end":70},{"critic":"risk_classifier","severity":"low","title":"Comment Line Validation","detail":"The buildReviewComments function validates line numbers against added lines in the diff. The logic ensures indices are within bounds, preventing out‑of‑range errors.","file":"internal/reporters/github.go","line_start":120,"line_end":170},{"critic":"test_coverage_smell","severity":"medium","title":"Missing tests for ListPRFiles","detail":"No unit tests cover the normal return of file paths or the error path when GitHub API fails.","file":"internal/githubclient/review.go","suggested_fix":"Add tests that mock the GitHub client to return a list of files and to return an error, verifying the returned slice and error handling."},{"critic":"test_coverage_smell","severity":"medium","title":"Missing tests for GetPRFileDiffs","detail":"The function is untested for normal diff parsing, error from ListFiles, nil Patch, and parse errors from diff.FromFiles.","file":"internal/githubclient/review.go","suggested_fix":"Create tests that mock ListFiles to return files with and without Patch, simulate a parse error, and verify the resulting map and error propagation."},{"critic":"test_coverage_smell","severity":"medium","title":"Missing tests for validateLine","detail":"Edge cases such as nil FileDiff, deleted files, zero added lines, and requested lines beyond the added count are not exercised.","file":"internal/reporters/github.go","suggested_fix":"Write unit tests calling validateLine with each of these scenarios and assert the expected return value."},{"critic":"test_coverage_smell","severity":"medium","title":"buildReviewComments filtering by PR files not tested","detail":"When a finding's file is not present in the PR file list, the comment should be skipped, but no test verifies this.","file":"internal/reporters/github.go","suggested_fix":"Add a test that provides a verdict with a finding on a file not returned by ListPRFiles and assert that no comment is generated."},{"critic":"test_coverage_smell","severity":"medium","title":"buildReviewComments line validation logic not exercised","detail":"The logic that adjusts lineStart/lineEnd based on fileDiffs is complex and currently untested.","file":"internal/reporters/github.go","suggested_fix":"Create tests with fileDiffs containing added lines and verify that the resulting comment uses the validated line numbers."},{"critic":"test_coverage_smell","severity":"medium","title":"Reporter CreateReview error handling not fully tested","detail":"Scenarios where the first CreateReview fails but the retry succeeds, or both fail, are not covered.","file":"internal/reporters/github.go","suggested_fix":"Use the mock client to simulate failure on the first call and success on the second, and another test where both fail, asserting the correct fallback path."},{"critic":"test_coverage_smell","severity":"medium","title":"Sticky comment fallback formatting not verified","detail":"The Markdown body and JSON summary formatting in the fallback sticky comment are not asserted in tests.","file":"internal/reporters/github_report_test.go","suggested_fix":"Add assertions that the body passed to PostStickyComment contains the acig marker, the verdict markdown, and the JSON summary wrapped in the correct details tags."},{"critic":"test_coverage_smell","severity":"medium","title":"buildReviewComments behavior when fileDiffs is nil","detail":"When fileDiffs is nil, the function should skip line validation but still generate comments; this path is not tested.","file":"internal/reporters/github.go","suggested_fix":"Write a test where GetPRFileDiffs returns nil and verify that comments are produced with original line numbers."},{"critic":"adjudicator","severity":"info","title":"Failed to parse critic output","detail":"Model returned invalid JSON (1885 bytes). This usually means the output was truncated. Raw: {\n \"findings\": [\n {\n \"severity\": \"medium\",\n \"title\": \"Missing pagination in ListPRFiles and GetPRFileDiffs\",\n \"detail\": \"Both functions use PerPage:100 but never follow GitHub's Link header to fetch subsequent pages. PRs with \u003e100 files will silently have some files dropped, causi..."}],"critic_results":[{"critic":"perf_smell","model":"qwen3-coder:480b","findings":[],"cost_usd":0.00019878,"duration_ms":1007,"tokens_in":3274,"tokens_out":13},{"critic":"risk_classifier","model":"gpt-oss:20b","findings":[{"critic":"risk_classifier","severity":"info","title":"Public GitHub App ID in Workflow","detail":"The workflow now uses a public variable for the GitHub App ID instead of a secret. This is acceptable as the ID is public, but the change should be documented to avoid accidental exposure of sensitive values.","file":".github/workflows/acig.yml","line_start":5,"line_end":12},{"critic":"risk_classifier","severity":"info","title":"Dynamic ACIG Version Retrieval","detail":"The workflow fetches the latest ACIG release via curl and parses the tag. While this introduces a network dependency, it does not expose secrets and is unlikely to cause a security issue.","file":".github/workflows/acig.yml","line_start":15,"line_end":22},{"critic":"risk_classifier","severity":"low","title":"New GitHub Client Interface","detail":"A new interface abstracts GitHub operations, improving testability. No unsafe operations or external dependencies are introduced.","file":"internal/reporters/github.go","line_start":20,"line_end":45},{"critic":"risk_classifier","severity":"low","title":"PR File Diff Parsing","detail":"The code parses patch diffs using a custom diff package. The input comes from GitHub's API, which is trusted, and no file I/O or unsafe parsing is performed.","file":"internal/githubclient/review.go","line_start":30,"line_end":70},{"critic":"risk_classifier","severity":"low","title":"Comment Line Validation","detail":"The buildReviewComments function validates line numbers against added lines in the diff. The logic ensures indices are within bounds, preventing out‑of‑range errors.","file":"internal/reporters/github.go","line_start":120,"line_end":170}],"cost_usd":0.00006400000000000001,"duration_ms":6679,"tokens_in":3253,"tokens_out":1049},{"critic":"test_coverage_smell","model":"gpt-oss:20b","findings":[{"critic":"test_coverage_smell","severity":"medium","title":"Missing tests for ListPRFiles","detail":"No unit tests cover the normal return of file paths or the error path when GitHub API fails.","file":"internal/githubclient/review.go","suggested_fix":"Add tests that mock the GitHub client to return a list of files and to return an error, verifying the returned slice and error handling."},{"critic":"test_coverage_smell","severity":"medium","title":"Missing tests for GetPRFileDiffs","detail":"The function is untested for normal diff parsing, error from ListFiles, nil Patch, and parse errors from diff.FromFiles.","file":"internal/githubclient/review.go","suggested_fix":"Create tests that mock ListFiles to return files with and without Patch, simulate a parse error, and verify the resulting map and error propagation."},{"critic":"test_coverage_smell","severity":"medium","title":"Missing tests for validateLine","detail":"Edge cases such as nil FileDiff, deleted files, zero added lines, and requested lines beyond the added count are not exercised.","file":"internal/reporters/github.go","suggested_fix":"Write unit tests calling validateLine with each of these scenarios and assert the expected return value."},{"critic":"test_coverage_smell","severity":"medium","title":"buildReviewComments filtering by PR files not tested","detail":"When a finding's file is not present in the PR file list, the comment should be skipped, but no test verifies this.","file":"internal/reporters/github.go","suggested_fix":"Add a test that provides a verdict with a finding on a file not returned by ListPRFiles and assert that no comment is generated."},{"critic":"test_coverage_smell","severity":"medium","title":"buildReviewComments line validation logic not exercised","detail":"The logic that adjusts lineStart/lineEnd based on fileDiffs is complex and currently untested.","file":"internal/reporters/github.go","suggested_fix":"Create tests with fileDiffs containing added lines and verify that the resulting comment uses the validated line numbers."},{"critic":"test_coverage_smell","severity":"medium","title":"Reporter CreateReview error handling not fully tested","detail":"Scenarios where the first CreateReview fails but the retry succeeds, or both fail, are not covered.","file":"internal/reporters/github.go","suggested_fix":"Use the mock client to simulate failure on the first call and success on the second, and another test where both fail, asserting the correct fallback path."},{"critic":"test_coverage_smell","severity":"medium","title":"Sticky comment fallback formatting not verified","detail":"The Markdown body and JSON summary formatting in the fallback sticky comment are not asserted in tests.","file":"internal/reporters/github_report_test.go","suggested_fix":"Add assertions that the body passed to PostStickyComment contains the acig marker, the verdict markdown, and the JSON summary wrapped in the correct details tags."},{"critic":"test_coverage_smell","severity":"medium","title":"buildReviewComments behavior when fileDiffs is nil","detail":"When fileDiffs is nil, the function should skip line validation but still generate comments; this path is not tested.","file":"internal/reporters/github.go","suggested_fix":"Write a test where GetPRFileDiffs returns nil and verify that comments are produced with original line numbers."}],"cost_usd":0.00007867,"duration_ms":9014,"tokens_in":3223,"tokens_out":1548},{"critic":"style_conformance","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009353,"duration_ms":10470,"tokens_in":3209,"tokens_out":2048},{"critic":"security_smell","model":"qwen3-coder:480b","findings":[],"cost_usd":0.00020082000000000002,"duration_ms":18070,"tokens_in":3308,"tokens_out":13},{"critic":"adjudicator","model":"glm-5.1","findings":[{"critic":"adjudicator","severity":"info","title":"Failed to parse critic output","detail":"Model returned invalid JSON (1885 bytes). This usually means the output was truncated. Raw: {\n \"findings\": [\n {\n \"severity\": \"medium\",\n \"title\": \"Missing pagination in ListPRFiles and GetPRFileDiffs\",\n \"detail\": \"Both functions use PerPage:100 but never follow GitHub's Link header to fetch subsequent pages. PRs with \u003e100 files will silently have some files dropped, causi..."}],"cost_usd":0,"duration_ms":29751,"tokens_in":4913,"tokens_out":2048}],"total_cost_usd":0.0006358,"total_duration_ms":74991,"budget_remaining_usd":0.2493642,"generated_at":"2026-05-07T23:31:39.981912341Z"}There was a problem hiding this comment.
acig: warn | risk=high | 6 finding(s) | $0.0008
| Severity | Critic | Title | File |
|---|---|---|---|
| high | perf_smell | N+1 query pattern in GetPRFileDiffs | internal/githubclient/review.go |
| medium | perf_smell | Missing context cancellation check in loop | internal/githubclient/review.go |
| medium | perf_smell | Unnecessary double API call for PR files | internal/githubclient/review.go |
| medium | perf_smell | Inefficient string building in comment generation | internal/reporters/github.go |
| low | perf_smell | Potential unbounded slice growth | internal/reporters/github.go |
| low | perf_smell | Redundant map initialization | internal/reporters/github.go |
Verdict JSON
{"schema_version":"1","repo":"https://github.com/helloodokai/acig","sha":"cb6f9f4ed0d91d40639dbbfe13139950f01290c8","base_sha":"main","risk":"high","decision":"warn","summary":"Decision: warn | Risk: high | Findings: 0 blocking, 1 high, 3 medium, 2 low, 0 info | Cost: $0.0008","findings":[{"critic":"perf_smell","severity":"high","title":"N+1 query pattern in GetPRFileDiffs","detail":"GetPRFileDiffs calls diff.FromFiles for each file in a loop, potentially causing parsing overhead. While ListFiles is paginated, the diff parsing happens per-file without batching.","file":"internal/githubclient/review.go","line_start":45,"line_end":65,"suggested_fix":"Consider batch processing of patches or lazy parsing of diffs only when needed."},{"critic":"perf_smell","severity":"medium","title":"Missing context cancellation check in loop","detail":"The loop processing files in GetPRFileDiffs does not check for context cancellation, which can cause delays in shutdown or timeout scenarios.","file":"internal/githubclient/review.go","line_start":54,"line_end":64,"suggested_fix":"Add ctx.Err() check inside the loop to respect cancellation signals."},{"critic":"perf_smell","severity":"medium","title":"Unnecessary double API call for PR files","detail":"Both ListPRFiles and GetPRFileDiffs call c.client.PullRequests.ListFiles independently, resulting in two API requests for the same data.","file":"internal/githubclient/review.go","line_start":30,"line_end":45,"suggested_fix":"Share the file listing result between functions or cache the response."},{"critic":"perf_smell","severity":"medium","title":"Inefficient string building in comment generation","detail":"Multiple calls to fmt.Sprintf and strings.Builder.WriteString in a loop can cause excessive allocations during comment generation.","file":"internal/reporters/github.go","line_start":100,"line_end":120,"suggested_fix":"Pre-calculate buffer size or reuse a single strings.Builder across comments."},{"critic":"perf_smell","severity":"low","title":"Potential unbounded slice growth","detail":"The comments slice in buildReviewComments grows dynamically with append, which may lead to reallocations if many comments exist.","file":"internal/reporters/github.go","line_start":75,"line_end":95,"suggested_fix":"Use make([]githubclient.ReviewComment, 0, expectedCap) if approximate count is known."},{"critic":"perf_smell","severity":"low","title":"Redundant map initialization","detail":"prFilesSet is created from prFiles even when prFiles is nil, leading to unnecessary empty map creation.","file":"internal/reporters/github.go","line_start":78,"line_end":80,"suggested_fix":"Skip map creation when prFiles is nil since it's only used for filtering."}],"critic_results":[{"critic":"security_smell","model":"qwen3-coder:480b","findings":[],"cost_usd":0.00020082000000000002,"duration_ms":6239,"tokens_in":3308,"tokens_out":13},{"critic":"style_conformance","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009353,"duration_ms":9250,"tokens_in":3209,"tokens_out":2048},{"critic":"risk_classifier","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009397,"duration_ms":10406,"tokens_in":3253,"tokens_out":2048},{"critic":"test_coverage_smell","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009366999999999999,"duration_ms":10478,"tokens_in":3223,"tokens_out":2048},{"critic":"perf_smell","model":"qwen3-coder:480b","findings":[{"critic":"perf_smell","severity":"high","title":"N+1 query pattern in GetPRFileDiffs","detail":"GetPRFileDiffs calls diff.FromFiles for each file in a loop, potentially causing parsing overhead. While ListFiles is paginated, the diff parsing happens per-file without batching.","file":"internal/githubclient/review.go","line_start":45,"line_end":65,"suggested_fix":"Consider batch processing of patches or lazy parsing of diffs only when needed."},{"critic":"perf_smell","severity":"medium","title":"Missing context cancellation check in loop","detail":"The loop processing files in GetPRFileDiffs does not check for context cancellation, which can cause delays in shutdown or timeout scenarios.","file":"internal/githubclient/review.go","line_start":54,"line_end":64,"suggested_fix":"Add ctx.Err() check inside the loop to respect cancellation signals."},{"critic":"perf_smell","severity":"medium","title":"Unnecessary double API call for PR files","detail":"Both ListPRFiles and GetPRFileDiffs call c.client.PullRequests.ListFiles independently, resulting in two API requests for the same data.","file":"internal/githubclient/review.go","line_start":30,"line_end":45,"suggested_fix":"Share the file listing result between functions or cache the response."},{"critic":"perf_smell","severity":"medium","title":"Inefficient string building in comment generation","detail":"Multiple calls to fmt.Sprintf and strings.Builder.WriteString in a loop can cause excessive allocations during comment generation.","file":"internal/reporters/github.go","line_start":100,"line_end":120,"suggested_fix":"Pre-calculate buffer size or reuse a single strings.Builder across comments."},{"critic":"perf_smell","severity":"low","title":"Potential unbounded slice growth","detail":"The comments slice in buildReviewComments grows dynamically with append, which may lead to reallocations if many comments exist.","file":"internal/reporters/github.go","line_start":75,"line_end":95,"suggested_fix":"Use make([]githubclient.ReviewComment, 0, expectedCap) if approximate count is known."},{"critic":"perf_smell","severity":"low","title":"Redundant map initialization","detail":"prFilesSet is created from prFiles even when prFiles is nil, leading to unnecessary empty map creation.","file":"internal/reporters/github.go","line_start":78,"line_end":80,"suggested_fix":"Skip map creation when prFiles is nil since it's only used for filtering."}],"cost_usd":0.00031487999999999997,"duration_ms":24838,"tokens_in":3274,"tokens_out":658},{"critic":"adjudicator","model":"glm-5.1","findings":null,"cost_usd":0,"duration_ms":24097,"tokens_in":4177,"tokens_out":2048}],"total_cost_usd":0.0007968700000000001,"total_duration_ms":85308,"budget_remaining_usd":0.24920313,"generated_at":"2026-05-07T23:34:11.169507514Z"}Three root causes for GitHub API 422 errors when creating review comments: 1. Path normalization: GetPRFileDiffs used f.GetFilename() directly instead of diff.FromFiles() which applies newPath() to strip a/b/ prefixes, causing path mismatches between findings and diff map keys. 2. Line number validation used array indices instead of actual line numbers: validateLine compared requestedLine against len(fd.Added) (array count), but Added stores content strings, not line numbers. A finding at line 50 in a file with 10 added lines would pass validation (50 > 10 → clamped to 10), sending an invalid line to GitHub. Now uses StartLine from diff hunk headers for accurate remapping. 3. Nil fileDiffs bypassed all validation: when GetPRFileDiffs failed (common in CI), fileDiffs was nil and the if fileDiffs != nil block was skipped entirely, allowing unvalidated line numbers and paths through to the API. Now buildReviewComments requires fileDiffs entries for inline comments — missing files are skipped. Key changes: - FileDiff.StartLine tracks actual hunk start line from @@ headers - validateLine uses StartLine + Added count for bounds checking - GetPRFileDiffs uses diff.FromFiles for consistent path normalization - buildReviewComments skips files absent from fileDiffs (fail-safe) - Comprehensive tests for validateLine and buildReviewComments
There was a problem hiding this comment.
acig: block | risk=critical | 13 finding(s) | $0.0008
| Severity | Critic | Title | File |
|---|---|---|---|
| info | risk_classifier | Failed to parse critic output | — |
| medium | test_coverage_smell | Missing tests for ListPRFiles | internal/githubclient/review.go |
| medium | test_coverage_smell | Missing tests for GetPRFileDiffs | internal/githubclient/review.go |
| medium | test_coverage_smell | validateLine function untested | internal/reporters/github.go |
| medium | test_coverage_smell | buildReviewComments logic not covered | internal/reporters/github.go |
| medium | test_coverage_smell | GitHubReporter create review retry path untested | internal/reporters/github.go |
| medium | test_coverage_smell | diff/git.go StartLine assignment not tested | internal/diff/git.go |
| low | test_coverage_smell | GitHubReporter ListPRFiles/GetPRFileDiffs error handling not covered | internal/reporters/github.go |
| low | test_coverage_smell | New GitHubClient interface not exercised in tests | internal/reporters/github.go |
| high | security_smell | SSRF via Unvalidated GitHub API URL Fetch | Makefile |
| blocking | security_smell | Authentication Bypass in GitHub App Token Logic | .github/workflows/acig.yml |
| medium | security_smell | Potential Path Traversal in Diff File Handling | internal/reporters/github.go |
| medium | security_smell | Missing Input Validation in Line Number Validation | internal/reporters/github.go |
Verdict JSON
{"schema_version":"1","repo":"https://github.com/helloodokai/acig","sha":"f08cc8eef9c0b48396b4de0a1d43fba91ab7a4f6","base_sha":"main","risk":"critical","decision":"block","summary":"Decision: block | Risk: critical | Findings: 1 blocking, 1 high, 8 medium, 2 low, 1 info | Cost: $0.0008","findings":[{"critic":"risk_classifier","severity":"info","title":"Failed to parse critic output","detail":"Model returned invalid JSON (872 bytes). This usually means the output was truncated. Raw: {\n \"risk\": \"low\",\n \"reasoning\": \"The diff adds functionality and refactors existing logic without introducing critical security flaws. No secrets are exposed, and potential issues are limited to non‑critical misconfigurations or edge‑case failures.\",\n \"findings\": [\n {\n \"severity\": \"in..."},{"critic":"test_coverage_smell","severity":"medium","title":"Missing tests for ListPRFiles","detail":"The new ListPRFiles method in githubclient/review.go is never exercised. Tests should cover normal return of file paths and the error path when the underlying GitHub API call fails.","file":"internal/githubclient/review.go","line_start":1,"line_end":30,"suggested_fix":"Add unit tests that mock the GitHub client to return a slice of filenames and another test that forces an error and verifies the returned error."},{"critic":"test_coverage_smell","severity":"medium","title":"Missing tests for GetPRFileDiffs","detail":"GetPRFileDiffs parses patch data into FileDiffs and skips deleted or unparsable files. No tests cover normal parsing, skipping logic, or error handling when ListFiles fails.","file":"internal/githubclient/review.go","line_start":30,"line_end":70,"suggested_fix":"Create tests that mock ListFiles to return patches, including nil/empty patches, delete flags, and malformed patches, and assert the resulting map."},{"critic":"test_coverage_smell","severity":"medium","title":"validateLine function untested","detail":"validateLine performs boundary checks on requested line numbers. Edge cases such as requestedLine \u003e lastAddedLine, \u003c StartLine, and deleted files are not exercised.","file":"internal/reporters/github.go","line_start":120,"line_end":160,"suggested_fix":"Add unit tests that construct FileDiffs with known StartLine and Added slices, then call validateLine with various requestedLine values and assert the returned line."},{"critic":"test_coverage_smell","severity":"medium","title":"buildReviewComments logic not covered","detail":"The new buildReviewComments function now filters by PR files, uses fileDiffs, and validates line ranges. No tests verify that comments are omitted for files not in the PR, for deleted files, or when line validation fails.","file":"internal/reporters/github.go","line_start":80,"line_end":120,"suggested_fix":"Write tests that provide a Verdict with findings, mock prFiles and fileDiffs, and assert that the returned ReviewComment slice matches expectations for each scenario."},{"critic":"test_coverage_smell","severity":"medium","title":"GitHubReporter create review retry path untested","detail":"When CreateReview fails with inline comments, the reporter retries without comments and falls back to a sticky comment. This error path is not exercised.","file":"internal/reporters/github.go","line_start":50,"line_end":90,"suggested_fix":"Mock the GitHubClient to return an error on the first CreateReview call and success on the second, then verify that PostStickyComment is called with the correct body."},{"critic":"test_coverage_smell","severity":"medium","title":"diff/git.go StartLine assignment not tested","detail":"The logic that sets FileDiff.StartLine from h.NewStartLine when it is zero is not covered by tests.","file":"internal/diff/git.go","line_start":1,"line_end":20,"suggested_fix":"Add a test that parses a patch with a hunk having NewStartLine \u003e 0 and asserts that the resulting FileDiff.StartLine equals that value."},{"critic":"test_coverage_smell","severity":"low","title":"GitHubReporter ListPRFiles/GetPRFileDiffs error handling not covered","detail":"When ListPRFiles or GetPRFileDiffs return errors, the reporter logs a warning and proceeds. These paths are not tested.","file":"internal/reporters/github.go","line_start":30,"line_end":70,"suggested_fix":"Mock the client to return errors for ListPRFiles and GetPRFileDiffs and assert that the reporter still attempts to create a review (with or without comments)."},{"critic":"test_coverage_smell","severity":"low","title":"New GitHubClient interface not exercised in tests","detail":"The reporter now depends on an interface; tests should use a mock that implements all methods, including ListPRFiles and GetPRFileDiffs.","file":"internal/reporters/github.go","line_start":1,"line_end":30,"suggested_fix":"Update existing tests to use a mock that implements the new interface and verify that the methods are called as expected."},{"critic":"security_smell","severity":"high","title":"SSRF via Unvalidated GitHub API URL Fetch","detail":"The Makefile fetches the latest ACIG version from a GitHub API URL without validating the response, potentially allowing server-side request forgery if the API returns a redirect to an internal endpoint.","file":"Makefile","line_start":8,"line_end":12,"suggested_fix":"Validate the fetched version string against an allowlist of expected semver patterns before using it in the RELEASE_URL."},{"critic":"security_smell","severity":"blocking","title":"Authentication Bypass in GitHub App Token Logic","detail":"The GitHub workflow now only checks for ACIG_APP_ID in vars but still uses the private key from secrets, creating inconsistent authentication requirements that could be bypassed if vars are less protected than secrets.","file":".github/workflows/acig.yml","line_start":5,"line_end":8,"suggested_fix":"Ensure both app-id and private-key use the same secret storage mechanism (preferably both as secrets) and validate both are present together."},{"critic":"security_smell","severity":"medium","title":"Potential Path Traversal in Diff File Handling","detail":"The diff parsing logic directly uses file paths from patch content without sanitization when building review comments, which could lead to path traversal if upstream parsing fails.","file":"internal/reporters/github.go","line_start":105,"line_end":110,"suggested_fix":"Sanitize and validate all file paths from diffs using filepath.Clean and ensure they're within expected repository boundaries."},{"critic":"security_smell","severity":"medium","title":"Missing Input Validation in Line Number Validation","detail":"The validateLine function does boundary checking but doesn't prevent potential integer overflow or negative values in line numbers from malformed diffs.","file":"internal/reporters/github.go","line_start":175,"line_end":187,"suggested_fix":"Add explicit checks for negative line numbers and integer overflow conditions before performing comparisons."}],"critic_results":[{"critic":"perf_smell","model":"qwen3-coder:480b","findings":[],"cost_usd":0.00019878,"duration_ms":1191,"tokens_in":3274,"tokens_out":13},{"critic":"style_conformance","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009353,"duration_ms":9996,"tokens_in":3209,"tokens_out":2048},{"critic":"risk_classifier","model":"gpt-oss:20b","findings":[{"critic":"risk_classifier","severity":"info","title":"Failed to parse critic output","detail":"Model returned invalid JSON (872 bytes). This usually means the output was truncated. Raw: {\n \"risk\": \"low\",\n \"reasoning\": \"The diff adds functionality and refactors existing logic without introducing critical security flaws. No secrets are exposed, and potential issues are limited to non‑critical misconfigurations or edge‑case failures.\",\n \"findings\": [\n {\n \"severity\": \"in..."}],"cost_usd":0.00009397999999999999,"duration_ms":11165,"tokens_in":3254,"tokens_out":2048},{"critic":"test_coverage_smell","model":"gpt-oss:20b","findings":[{"critic":"test_coverage_smell","severity":"medium","title":"Missing tests for ListPRFiles","detail":"The new ListPRFiles method in githubclient/review.go is never exercised. Tests should cover normal return of file paths and the error path when the underlying GitHub API call fails.","file":"internal/githubclient/review.go","line_start":1,"line_end":30,"suggested_fix":"Add unit tests that mock the GitHub client to return a slice of filenames and another test that forces an error and verifies the returned error."},{"critic":"test_coverage_smell","severity":"medium","title":"Missing tests for GetPRFileDiffs","detail":"GetPRFileDiffs parses patch data into FileDiffs and skips deleted or unparsable files. No tests cover normal parsing, skipping logic, or error handling when ListFiles fails.","file":"internal/githubclient/review.go","line_start":30,"line_end":70,"suggested_fix":"Create tests that mock ListFiles to return patches, including nil/empty patches, delete flags, and malformed patches, and assert the resulting map."},{"critic":"test_coverage_smell","severity":"medium","title":"validateLine function untested","detail":"validateLine performs boundary checks on requested line numbers. Edge cases such as requestedLine \u003e lastAddedLine, \u003c StartLine, and deleted files are not exercised.","file":"internal/reporters/github.go","line_start":120,"line_end":160,"suggested_fix":"Add unit tests that construct FileDiffs with known StartLine and Added slices, then call validateLine with various requestedLine values and assert the returned line."},{"critic":"test_coverage_smell","severity":"medium","title":"buildReviewComments logic not covered","detail":"The new buildReviewComments function now filters by PR files, uses fileDiffs, and validates line ranges. No tests verify that comments are omitted for files not in the PR, for deleted files, or when line validation fails.","file":"internal/reporters/github.go","line_start":80,"line_end":120,"suggested_fix":"Write tests that provide a Verdict with findings, mock prFiles and fileDiffs, and assert that the returned ReviewComment slice matches expectations for each scenario."},{"critic":"test_coverage_smell","severity":"medium","title":"GitHubReporter create review retry path untested","detail":"When CreateReview fails with inline comments, the reporter retries without comments and falls back to a sticky comment. This error path is not exercised.","file":"internal/reporters/github.go","line_start":50,"line_end":90,"suggested_fix":"Mock the GitHubClient to return an error on the first CreateReview call and success on the second, then verify that PostStickyComment is called with the correct body."},{"critic":"test_coverage_smell","severity":"medium","title":"diff/git.go StartLine assignment not tested","detail":"The logic that sets FileDiff.StartLine from h.NewStartLine when it is zero is not covered by tests.","file":"internal/diff/git.go","line_start":1,"line_end":20,"suggested_fix":"Add a test that parses a patch with a hunk having NewStartLine \u003e 0 and asserts that the resulting FileDiff.StartLine equals that value."},{"critic":"test_coverage_smell","severity":"low","title":"GitHubReporter ListPRFiles/GetPRFileDiffs error handling not covered","detail":"When ListPRFiles or GetPRFileDiffs return errors, the reporter logs a warning and proceeds. These paths are not tested.","file":"internal/reporters/github.go","line_start":30,"line_end":70,"suggested_fix":"Mock the client to return errors for ListPRFiles and GetPRFileDiffs and assert that the reporter still attempts to create a review (with or without comments)."},{"critic":"test_coverage_smell","severity":"low","title":"New GitHubClient interface not exercised in tests","detail":"The reporter now depends on an interface; tests should use a mock that implements all methods, including ListPRFiles and GetPRFileDiffs.","file":"internal/reporters/github.go","line_start":1,"line_end":30,"suggested_fix":"Update existing tests to use a mock that implements the new interface and verify that the methods are called as expected."}],"cost_usd":0.00008575,"duration_ms":11209,"tokens_in":3223,"tokens_out":1784},{"critic":"security_smell","model":"qwen3-coder:480b","findings":[{"critic":"security_smell","severity":"high","title":"SSRF via Unvalidated GitHub API URL Fetch","detail":"The Makefile fetches the latest ACIG version from a GitHub API URL without validating the response, potentially allowing server-side request forgery if the API returns a redirect to an internal endpoint.","file":"Makefile","line_start":8,"line_end":12,"suggested_fix":"Validate the fetched version string against an allowlist of expected semver patterns before using it in the RELEASE_URL."},{"critic":"security_smell","severity":"blocking","title":"Authentication Bypass in GitHub App Token Logic","detail":"The GitHub workflow now only checks for ACIG_APP_ID in vars but still uses the private key from secrets, creating inconsistent authentication requirements that could be bypassed if vars are less protected than secrets.","file":".github/workflows/acig.yml","line_start":5,"line_end":8,"suggested_fix":"Ensure both app-id and private-key use the same secret storage mechanism (preferably both as secrets) and validate both are present together."},{"critic":"security_smell","severity":"medium","title":"Potential Path Traversal in Diff File Handling","detail":"The diff parsing logic directly uses file paths from patch content without sanitization when building review comments, which could lead to path traversal if upstream parsing fails.","file":"internal/reporters/github.go","line_start":105,"line_end":110,"suggested_fix":"Sanitize and validate all file paths from diffs using filepath.Clean and ensure they're within expected repository boundaries."},{"critic":"security_smell","severity":"medium","title":"Missing Input Validation in Line Number Validation","detail":"The validateLine function does boundary checking but doesn't prevent potential integer overflow or negative values in line numbers from malformed diffs.","file":"internal/reporters/github.go","line_start":175,"line_end":187,"suggested_fix":"Add explicit checks for negative line numbers and integer overflow conditions before performing comparisons."}],"cost_usd":0.00028758,"duration_ms":30781,"tokens_in":3308,"tokens_out":495},{"critic":"adjudicator","model":"glm-5.1","findings":null,"cost_usd":0,"duration_ms":70916,"tokens_in":5349,"tokens_out":2048}],"total_cost_usd":0.0007596199999999999,"total_duration_ms":135258,"budget_remaining_usd":0.24924038,"generated_at":"2026-05-08T00:31:46.254054882Z"}The previous fix used StartLine + len(Added) to compute valid line
ranges, but Added only contains + lines, not context lines. GitHub's
line parameter must reference ANY line visible in the PR diff hunks,
including context lines (lines starting with space in the diff).
Now tracks HunkRange{Start, End} from each @@ -old,count +new,count @@
hunk header, where Start=NewStartLine and End=NewStartLine+NewLines-1.
validateLine checks if the requested line falls within any hunk range,
and clamps to the nearest range start if it doesn't.
Also adds integration tests using httptest to mock GitHub API responses,
validating path normalization, hunk range parsing, and review payload
construction.
The checksums.txt download was failing silently in CI, returning an empty expected hash. Added GitHub API auth headers for rate limiting and a check that the expected hash is non-empty with file contents output for debugging.
acig re-run: replacing with updated review
There was a problem hiding this comment.
acig: warn | risk=high | 3 finding(s) | $0.0008
| Severity | Critic | Title | File |
|---|---|---|---|
| high | security_smell | SSRF via user-controlled URL in ACIG version fetch | .github/workflows/acig.yml |
| medium | security_smell | Potential command injection in ACIG installation | .github/workflows/acig.yml |
| high | security_smell | Insecure credential handling in GitHub Actions | .github/workflows/acig.yml |
Verdict JSON
{"schema_version":"1","repo":"https://github.com/helloodokai/acig","sha":"972087a3b7add49e9afb1c4f44388dde75974fda","base_sha":"main","risk":"high","decision":"warn","summary":"Decision: warn | Risk: high | Findings: 0 blocking, 2 high, 1 medium, 0 low, 0 info | Cost: $0.0008","findings":[{"critic":"security_smell","severity":"high","title":"SSRF via user-controlled URL in ACIG version fetch","detail":"The Makefile fetches the latest ACIG version from a GitHub API endpoint using a URL that includes user-controlled input. While currently limited to github.com, this could be exploited if the environment is compromised or in a different context where the URL can be manipulated.","file":".github/workflows/acig.yml","line_start":20,"line_end":25,"suggested_fix":"Validate and sanitize the GitHub API response, use a fixed version instead of fetching latest, or implement proper URL validation if dynamic fetching is required."},{"critic":"security_smell","severity":"medium","title":"Potential command injection in ACIG installation","detail":"The ACIG_VERSION variable is used directly in shell commands without proper sanitization. If the GitHub API returns malicious data, it could lead to command injection.","file":".github/workflows/acig.yml","line_start":20,"line_end":30,"suggested_fix":"Sanitize the ACIG_VERSION variable before using it in shell commands. Use fixed versions or implement strict validation of the fetched version format."},{"critic":"security_smell","severity":"high","title":"Insecure credential handling in GitHub Actions","detail":"The workflow condition changed from checking both APP_ID and PRIVATE_KEY secrets to only checking APP_ID as a variable, potentially exposing the private key without proper protection.","file":".github/workflows/acig.yml","line_start":5,"line_end":8,"suggested_fix":"Ensure both APP_ID and PRIVATE_KEY are properly protected and validated. Revert to using secrets for both values or implement proper access controls."}],"critic_results":[{"critic":"perf_smell","model":"qwen3-coder:480b","findings":[],"cost_usd":0.00019878,"duration_ms":1100,"tokens_in":3274,"tokens_out":13},{"critic":"style_conformance","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009353,"duration_ms":10332,"tokens_in":3209,"tokens_out":2048},{"critic":"risk_classifier","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009397999999999999,"duration_ms":10562,"tokens_in":3254,"tokens_out":2048},{"critic":"test_coverage_smell","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009366999999999999,"duration_ms":12008,"tokens_in":3223,"tokens_out":2048},{"critic":"security_smell","model":"qwen3-coder:480b","findings":[{"critic":"security_smell","severity":"high","title":"SSRF via user-controlled URL in ACIG version fetch","detail":"The Makefile fetches the latest ACIG version from a GitHub API endpoint using a URL that includes user-controlled input. While currently limited to github.com, this could be exploited if the environment is compromised or in a different context where the URL can be manipulated.","file":".github/workflows/acig.yml","line_start":20,"line_end":25,"suggested_fix":"Validate and sanitize the GitHub API response, use a fixed version instead of fetching latest, or implement proper URL validation if dynamic fetching is required."},{"critic":"security_smell","severity":"medium","title":"Potential command injection in ACIG installation","detail":"The ACIG_VERSION variable is used directly in shell commands without proper sanitization. If the GitHub API returns malicious data, it could lead to command injection.","file":".github/workflows/acig.yml","line_start":20,"line_end":30,"suggested_fix":"Sanitize the ACIG_VERSION variable before using it in shell commands. Use fixed versions or implement strict validation of the fetched version format."},{"critic":"security_smell","severity":"high","title":"Insecure credential handling in GitHub Actions","detail":"The workflow condition changed from checking both APP_ID and PRIVATE_KEY secrets to only checking APP_ID as a variable, potentially exposing the private key without proper protection.","file":".github/workflows/acig.yml","line_start":5,"line_end":8,"suggested_fix":"Ensure both APP_ID and PRIVATE_KEY are properly protected and validated. Revert to using secrets for both values or implement proper access controls."}],"cost_usd":0.00027246000000000003,"duration_ms":28647,"tokens_in":3308,"tokens_out":411}],"total_cost_usd":0.00075242,"total_duration_ms":62649,"budget_remaining_usd":0.24924758,"generated_at":"2026-05-08T01:04:19.848038569Z"}Replaces raw curl calls to GitHub API with `gh release` commands, which handle authentication via GITHUB_TOKEN automatically. This eliminates SSRF risk from user-controlled URL construction, command injection from untrusted version interpolation, and insecure credential handling in curl headers.
There was a problem hiding this comment.
acig: pass | risk=low | 8 finding(s) | $0.0007
| Severity | Critic | Title | File |
|---|---|---|---|
| info | risk_classifier | No secret exposure | .github/workflows/acig.yml |
| info | risk_classifier | Safe use of GH CLI | .github/workflows/acig.yml |
| info | risk_classifier | Diff parsing logic added | internal/diff/diff.go |
| info | risk_classifier | Git diff parsing safe | internal/diff/git.go |
| info | risk_classifier | No file I/O without checks | internal/githubclient/review.go |
| info | risk_classifier | No SQL or XSS patterns | — |
| info | risk_classifier | No unsafe goroutines | — |
| info | risk_classifier | Critical paths unchanged | — |
Verdict JSON
{"schema_version":"1","repo":"https://github.com/helloodokai/acig","sha":"ecf5991806b8d431a6025159b512c64c59fca3c4","base_sha":"main","risk":"low","decision":"pass","summary":"Decision: pass | Risk: low | Findings: 0 blocking, 0 high, 0 medium, 0 low, 8 info | Cost: $0.0007","findings":[{"critic":"risk_classifier","severity":"info","title":"No secret exposure","detail":"All secrets remain protected; only non-sensitive vars are used in the workflow.","file":".github/workflows/acig.yml","line_start":1,"line_end":20},{"critic":"risk_classifier","severity":"info","title":"Safe use of GH CLI","detail":"The workflow uses `gh release view` and `gh release download` with error checks; no sensitive data is printed to logs.","file":".github/workflows/acig.yml","line_start":21,"line_end":45},{"critic":"risk_classifier","severity":"info","title":"Diff parsing logic added","detail":"New HunkRange struct and StartLine tracking are purely internal and contain no unsafe operations.","file":"internal/diff/diff.go","line_start":1,"line_end":30},{"critic":"risk_classifier","severity":"info","title":"Git diff parsing safe","detail":"Parsing uses bytes.Split and string conversion without external input; no injection vectors.","file":"internal/diff/git.go","line_start":1,"line_end":40},{"critic":"risk_classifier","severity":"info","title":"No file I/O without checks","detail":"All file operations are performed by the GitHub API or by the workflow runner; no direct file writes are added.","file":"internal/githubclient/review.go","line_start":1,"line_end":70},{"critic":"risk_classifier","severity":"info","title":"No SQL or XSS patterns","detail":"The codebase does not interact with databases or render HTML; no injection patterns detected."},{"critic":"risk_classifier","severity":"info","title":"No unsafe goroutines","detail":"The added functions are synchronous and do not spawn goroutines without context handling."},{"critic":"risk_classifier","severity":"info","title":"Critical paths unchanged","detail":"No modifications to authentication, payment, or security modules."}],"critic_results":[{"critic":"security_smell","model":"qwen3-coder:480b","findings":[],"cost_usd":0.00020082000000000002,"duration_ms":924,"tokens_in":3308,"tokens_out":13},{"critic":"perf_smell","model":"qwen3-coder:480b","findings":[],"cost_usd":0.00019878,"duration_ms":1412,"tokens_in":3274,"tokens_out":13},{"critic":"risk_classifier","model":"gpt-oss:20b","findings":[{"critic":"risk_classifier","severity":"info","title":"No secret exposure","detail":"All secrets remain protected; only non-sensitive vars are used in the workflow.","file":".github/workflows/acig.yml","line_start":1,"line_end":20},{"critic":"risk_classifier","severity":"info","title":"Safe use of GH CLI","detail":"The workflow uses `gh release view` and `gh release download` with error checks; no sensitive data is printed to logs.","file":".github/workflows/acig.yml","line_start":21,"line_end":45},{"critic":"risk_classifier","severity":"info","title":"Diff parsing logic added","detail":"New HunkRange struct and StartLine tracking are purely internal and contain no unsafe operations.","file":"internal/diff/diff.go","line_start":1,"line_end":30},{"critic":"risk_classifier","severity":"info","title":"Git diff parsing safe","detail":"Parsing uses bytes.Split and string conversion without external input; no injection vectors.","file":"internal/diff/git.go","line_start":1,"line_end":40},{"critic":"risk_classifier","severity":"info","title":"No file I/O without checks","detail":"All file operations are performed by the GitHub API or by the workflow runner; no direct file writes are added.","file":"internal/githubclient/review.go","line_start":1,"line_end":70},{"critic":"risk_classifier","severity":"info","title":"No SQL or XSS patterns","detail":"The codebase does not interact with databases or render HTML; no injection patterns detected."},{"critic":"risk_classifier","severity":"info","title":"No unsafe goroutines","detail":"The added functions are synchronous and do not spawn goroutines without context handling."},{"critic":"risk_classifier","severity":"info","title":"Critical paths unchanged","detail":"No modifications to authentication, payment, or security modules."}],"cost_usd":0.00006748999999999999,"duration_ms":7766,"tokens_in":3254,"tokens_out":1165},{"critic":"style_conformance","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009353,"duration_ms":11695,"tokens_in":3209,"tokens_out":2048},{"critic":"test_coverage_smell","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009366999999999999,"duration_ms":12391,"tokens_in":3223,"tokens_out":2048}],"total_cost_usd":0.00065429,"total_duration_ms":34188,"budget_remaining_usd":0.24934571,"generated_at":"2026-05-08T01:15:54.3553096Z"}Critics like perf_smell and test_coverage_smell (gpt-oss models) would
hallucinate file paths not present in the diff (e.g. inventing
service/get_plan_context.go in a TypeScript codebase). These fabricated
findings caused GitHub review comment 422 errors and polluted the verdict
table with references to non-existent files.
Three-layer defense:
- Pipeline: FilterHallucinatedPaths() strips findings referencing files
not in the diff before verdict finalization, with warning logs
- Reporter: Defensive slog.Warn on skip points in buildReviewComments
- Prompts: Added CRITICAL constraint + explicit file list ({{.ChangedFiles}})
to all 6 critic prompts to reduce model hallucination at the source
Bump version to 1.6.0
There was a problem hiding this comment.
acig: warn | risk=medium | 7 finding(s) | $0.0006
| Severity | Critic | Title | File |
|---|---|---|---|
| medium | test_coverage_smell | ListPRFiles not tested | internal/githubclient/review.go |
| medium | test_coverage_smell | GetPRFileDiffs error paths missing | internal/githubclient/review.go |
| low | test_coverage_smell | Empty or nil patch handling not tested | internal/githubclient/review_test.go |
| low | test_coverage_smell | Invalid patch parsing not tested | internal/githubclient/review_test.go |
| low | test_coverage_smell | ListPRFiles with no files not tested | internal/githubclient/review_test.go |
| low | test_coverage_smell | ListPRFiles with nil patch not tested | internal/githubclient/review_test.go |
| low | test_coverage_smell | git.go HunkRanges and StartLine population not verified | internal/diff/git.go |
Verdict JSON
{"schema_version":"1","repo":"https://github.com/helloodokai/acig","sha":"a3c02aa56ae4af2faf5880003b3b94a04ef60290","base_sha":"main","risk":"medium","decision":"warn","summary":"Decision: warn | Risk: medium | Findings: 0 blocking, 0 high, 2 medium, 5 low, 0 info | Cost: $0.0006","findings":[{"critic":"test_coverage_smell","severity":"medium","title":"ListPRFiles not tested","detail":"The new ListPRFiles method has no unit tests covering normal operation or error handling.","file":"internal/githubclient/review.go","line_start":1,"line_end":200,"suggested_fix":"Add tests that mock the GitHub API to verify returned file paths and that errors from ListFiles are propagated."},{"critic":"test_coverage_smell","severity":"medium","title":"GetPRFileDiffs error paths missing","detail":"GetPRFileDiffs does not have tests for API failures, parse errors, or empty patches.","file":"internal/githubclient/review.go","line_start":1,"line_end":200,"suggested_fix":"Create tests that simulate a GitHub API error, a patch that fails to parse, and a file with an empty or nil patch to ensure these cases are handled correctly."},{"critic":"test_coverage_smell","severity":"low","title":"Empty or nil patch handling not tested","detail":"When a file's Patch field is nil or an empty string, GetPRFileDiffs should skip it. No test covers this scenario.","file":"internal/githubclient/review_test.go","line_start":1,"line_end":200,"suggested_fix":"Add a test case with a file entry having Patch: nil or \"\" and assert that the result map does not contain that file."},{"critic":"test_coverage_smell","severity":"low","title":"Invalid patch parsing not tested","detail":"GetPRFileDiffs skips files when diff.FromFiles returns an error. No test verifies this behavior.","file":"internal/githubclient/review_test.go","line_start":1,"line_end":200,"suggested_fix":"Add a test with a malformed patch string and confirm that the file is omitted from the result."},{"critic":"test_coverage_smell","severity":"low","title":"ListPRFiles with no files not tested","detail":"When the GitHub API returns an empty list, ListPRFiles should return an empty slice without error.","file":"internal/githubclient/review_test.go","line_start":1,"line_end":200,"suggested_fix":"Add a test where the mocked API returns zero files and assert the returned slice is empty."},{"critic":"test_coverage_smell","severity":"low","title":"ListPRFiles with nil patch not tested","detail":"ListPRFiles should ignore files with nil Patch values. No test covers this scenario.","file":"internal/githubclient/review_test.go","line_start":1,"line_end":200,"suggested_fix":"Add a test case with a file having Patch: nil and verify it is not included in the returned slice."},{"critic":"test_coverage_smell","severity":"low","title":"git.go HunkRanges and StartLine population not verified","detail":"The logic in git.go that populates HunkRanges and StartLine is not exercised by tests.","file":"internal/diff/git.go","line_start":1,"line_end":200,"suggested_fix":"Add tests that construct a FileDiff with multiple hunks and assert that HunkRanges and StartLine are set correctly."}],"critic_results":[{"critic":"risk_classifier","model":"gpt-oss:20b","findings":[],"cost_usd":0.0000509,"duration_ms":3729,"tokens_in":3254,"tokens_out":612},{"critic":"perf_smell","model":"qwen3-coder:480b","findings":[],"cost_usd":0.00019878,"duration_ms":5814,"tokens_in":3274,"tokens_out":13},{"critic":"security_smell","model":"qwen3-coder:480b","findings":[],"cost_usd":0.00020082000000000002,"duration_ms":6439,"tokens_in":3308,"tokens_out":13},{"critic":"test_coverage_smell","model":"gpt-oss:20b","findings":[{"critic":"test_coverage_smell","severity":"medium","title":"ListPRFiles not tested","detail":"The new ListPRFiles method has no unit tests covering normal operation or error handling.","file":"internal/githubclient/review.go","line_start":1,"line_end":200,"suggested_fix":"Add tests that mock the GitHub API to verify returned file paths and that errors from ListFiles are propagated."},{"critic":"test_coverage_smell","severity":"medium","title":"GetPRFileDiffs error paths missing","detail":"GetPRFileDiffs does not have tests for API failures, parse errors, or empty patches.","file":"internal/githubclient/review.go","line_start":1,"line_end":200,"suggested_fix":"Create tests that simulate a GitHub API error, a patch that fails to parse, and a file with an empty or nil patch to ensure these cases are handled correctly."},{"critic":"test_coverage_smell","severity":"low","title":"Empty or nil patch handling not tested","detail":"When a file's Patch field is nil or an empty string, GetPRFileDiffs should skip it. No test covers this scenario.","file":"internal/githubclient/review_test.go","line_start":1,"line_end":200,"suggested_fix":"Add a test case with a file entry having Patch: nil or \"\" and assert that the result map does not contain that file."},{"critic":"test_coverage_smell","severity":"low","title":"Invalid patch parsing not tested","detail":"GetPRFileDiffs skips files when diff.FromFiles returns an error. No test verifies this behavior.","file":"internal/githubclient/review_test.go","line_start":1,"line_end":200,"suggested_fix":"Add a test with a malformed patch string and confirm that the file is omitted from the result."},{"critic":"test_coverage_smell","severity":"low","title":"ListPRFiles with no files not tested","detail":"When the GitHub API returns an empty list, ListPRFiles should return an empty slice without error.","file":"internal/githubclient/review_test.go","line_start":1,"line_end":200,"suggested_fix":"Add a test where the mocked API returns zero files and assert the returned slice is empty."},{"critic":"test_coverage_smell","severity":"low","title":"ListPRFiles with nil patch not tested","detail":"ListPRFiles should ignore files with nil Patch values. No test covers this scenario.","file":"internal/githubclient/review_test.go","line_start":1,"line_end":200,"suggested_fix":"Add a test case with a file having Patch: nil and verify it is not included in the returned slice."},{"critic":"test_coverage_smell","severity":"low","title":"git.go HunkRanges and StartLine population not verified","detail":"The logic in git.go that populates HunkRanges and StartLine is not exercised by tests.","file":"internal/diff/git.go","line_start":1,"line_end":200,"suggested_fix":"Add tests that construct a FileDiff with multiple hunks and assert that HunkRanges and StartLine are set correctly."}],"cost_usd":0.00008584,"duration_ms":8617,"tokens_in":3223,"tokens_out":1787},{"critic":"style_conformance","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009353,"duration_ms":9932,"tokens_in":3209,"tokens_out":2048}],"total_cost_usd":0.00062987,"total_duration_ms":34531,"budget_remaining_usd":0.24937013,"generated_at":"2026-05-08T10:12:35.249159243Z"}- Replace FilterHallucinatedPaths with CategorizeFindingsByPath that splits findings into: in-diff (can be inline-commented), dangling (plausible suggestions like test files not yet in the PR), and hallucinated (fabricated paths with no relation to the diff) - Dangling findings are kept in Verdict.DanglingFindings and shown in the review body under 'Observations' but NOT posted as inline review comments (which would cause 422 errors) - Test file suggestions (e.g. plan-tools.test.ts) are now correctly recognized as plausible instead of being discarded - Upgrade GitHub review body with emoji severity badges, separated sections for inline vs observation findings, collapsible critic results table, and cleaner formatting - Upgrade markdown reporter with same improved formatting Bump version to 1.7.0
There was a problem hiding this comment.
acig: pass | risk=low | 2 finding(s) | $0.0007
| Severity | Critic | Title | File |
|---|---|---|---|
| low | risk_classifier | Public exposure of GitHub App ID | .github/workflows/acig.yml |
| low | risk_classifier | Silent diff parsing errors in GetPRFileDiffs | internal/githubclient/review.go |
Verdict JSON
{"schema_version":"1","repo":"https://github.com/helloodokai/acig","sha":"cf8aaaa01746c93bb522782f13243ffabc5781e4","base_sha":"main","risk":"low","decision":"pass","summary":"Decision: pass | Risk: low | Findings: 0 blocking, 0 high, 0 medium, 2 low, 0 info | Cost: $0.0007","findings":[{"critic":"risk_classifier","severity":"low","title":"Public exposure of GitHub App ID","detail":"The workflow now uses a public variable `vars.ACIG_APP_ID` instead of a secret to provide the GitHub App ID. While the ID is not a secret, exposing it publicly may reveal internal infrastructure details.","file":".github/workflows/acig.yml","line_start":6,"line_end":12},{"critic":"risk_classifier","severity":"low","title":"Silent diff parsing errors in GetPRFileDiffs","detail":"The `GetPRFileDiffs` method ignores errors returned by `diff.FromFiles` and silently skips files with parsing failures. This could hide problematic diffs but does not introduce a security risk.","file":"internal/githubclient/review.go","line_start":30,"line_end":60}],"critic_results":[{"critic":"risk_classifier","model":"gpt-oss:20b","findings":[{"critic":"risk_classifier","severity":"low","title":"Public exposure of GitHub App ID","detail":"The workflow now uses a public variable `vars.ACIG_APP_ID` instead of a secret to provide the GitHub App ID. While the ID is not a secret, exposing it publicly may reveal internal infrastructure details.","file":".github/workflows/acig.yml","line_start":6,"line_end":12},{"critic":"risk_classifier","severity":"low","title":"Silent diff parsing errors in GetPRFileDiffs","detail":"The `GetPRFileDiffs` method ignores errors returned by `diff.FromFiles` and silently skips files with parsing failures. This could hide problematic diffs but does not introduce a security risk.","file":"internal/githubclient/review.go","line_start":30,"line_end":60}],"cost_usd":0.00006859999999999999,"duration_ms":5461,"tokens_in":3521,"tokens_out":1113},{"critic":"style_conformance","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009621,"duration_ms":9716,"tokens_in":3477,"tokens_out":2048},{"critic":"test_coverage_smell","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009633999999999999,"duration_ms":10267,"tokens_in":3490,"tokens_out":2048},{"critic":"perf_smell","model":"qwen3-coder:480b","findings":[],"cost_usd":0.0002148,"duration_ms":29887,"tokens_in":3541,"tokens_out":13},{"critic":"security_smell","model":"qwen3-coder:480b","findings":[],"cost_usd":0.00021684,"duration_ms":29895,"tokens_in":3575,"tokens_out":13}],"total_cost_usd":0.00069279,"total_duration_ms":85226,"budget_remaining_usd":0.24930721,"generated_at":"2026-05-08T12:30:46.533953118Z"}Findings with a file but LineStart=0 (common from risk_classifier and test_coverage_smell) were being silently dropped because groupFindings and buildReviewComments both skipped them. This caused reviews to have no inline comments at all. Now: findings with LineStart=0 are grouped by file and clamped to the first hunk range of the diff, so they still appear as inline comments on the correct file in the PR. Bump version to 1.8.0
There was a problem hiding this comment.
✅ acig verdict: PASS | risk=low | 0 finding(s) | $0.0007
Critic Results
| Critic | Model | Findings | Cost | Duration |
|---|---|---|---|---|
| risk_classifier | gpt-oss:20b | 0 | $0.0001 | 10529ms |
| test_coverage_smell | gpt-oss:20b | 0 | $0.0001 | 11081ms |
| style_conformance | gpt-oss:20b | 0 | $0.0001 | 12854ms |
| security_smell | qwen3-coder:480b | 0 | $0.0002 | 40390ms |
| perf_smell | qwen3-coder:480b | 0 | $0.0002 | 41200ms |
Verdict JSON
{"schema_version":"1","repo":"https://github.com/helloodokai/acig","sha":"d0344f2084ef5053c49188f1b08be106f8b880bf","base_sha":"main","risk":"low","decision":"pass","summary":"Decision: pass | Risk: low | Findings: 0 blocking, 0 high, 0 medium, 0 low, 0 info | Cost: $0.0007","findings":null,"critic_results":[{"critic":"risk_classifier","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009664999999999999,"duration_ms":10529,"tokens_in":3521,"tokens_out":2048},{"critic":"test_coverage_smell","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009633999999999999,"duration_ms":11081,"tokens_in":3490,"tokens_out":2048},{"critic":"style_conformance","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009621,"duration_ms":12854,"tokens_in":3477,"tokens_out":2048},{"critic":"security_smell","model":"qwen3-coder:480b","findings":[],"cost_usd":0.00021684,"duration_ms":40390,"tokens_in":3575,"tokens_out":13},{"critic":"perf_smell","model":"qwen3-coder:480b","findings":[],"cost_usd":0.0002148,"duration_ms":41200,"tokens_in":3541,"tokens_out":13}],"total_cost_usd":0.0007208399999999999,"total_duration_ms":116054,"budget_remaining_usd":0.24927916,"generated_at":"2026-05-08T13:22:45.299427208Z"}
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
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.
No description provided.