Skip to content

Fix/422 path could not be resolved on deleted files#27

Open
supasympa wants to merge 11 commits into
mainfrom
fix/422-path-could-not-be-resolved-on-deleted-files
Open

Fix/422 path could not be resolved on deleted files#27
supasympa wants to merge 11 commits into
mainfrom
fix/422-path-could-not-be-resolved-on-deleted-files

Conversation

@supasympa

Copy link
Copy Markdown
Contributor

No description provided.

supasympa added 10 commits May 7, 2026 21:12
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.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acig: pass | risk=low | 8 finding(s) | $0.0006

Severity Critic Title File
info risk_classifier No secret exposure .github/workflows/acig.yml
info risk_classifier Safe use of ListPRFiles internal/githubclient/review.go
info risk_classifier No SQL or XSS patterns internal/reporters/github.go
info risk_classifier No unsafe file I/O internal/reporters/github.go
info risk_classifier No naked goroutines internal/reporters/github.go
info risk_classifier Tests use mocks safely internal/reporters/github_report_test.go
info risk_classifier Interface abstraction does not reduce security internal/reporters/github.go
info risk_classifier No critical path changes Makefile
Verdict JSON
{"schema_version":"1","repo":"https://github.com/helloodokai/acig","sha":"c810690b353d00b2227ed1d02a9bf4a5ce22e022","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.0006","findings":[{"critic":"risk_classifier","severity":"info","title":"No secret exposure","detail":"All GitHub App credentials remain in secrets; only the app ID is moved to a variable, which is safe.","file":".github/workflows/acig.yml","line_start":1,"line_end":10},{"critic":"risk_classifier","severity":"info","title":"Safe use of ListPRFiles","detail":"The new ListPRFiles method is called with error handling; failures fall back to using all findings.","file":"internal/githubclient/review.go","line_start":1,"line_end":30},{"critic":"risk_classifier","severity":"info","title":"No SQL or XSS patterns","detail":"The code deals only with GitHub API interactions and string formatting; no database queries or HTML rendering that could lead to injection.","file":"internal/reporters/github.go","line_start":1,"line_end":120},{"critic":"risk_classifier","severity":"info","title":"No unsafe file I/O","detail":"The repository does not perform any file system operations; all I/O is via the GitHub API.","file":"internal/reporters/github.go","line_start":1,"line_end":120},{"critic":"risk_classifier","severity":"info","title":"No naked goroutines","detail":"The code does not spawn goroutines without proper synchronization or error handling.","file":"internal/reporters/github.go","line_start":1,"line_end":120},{"critic":"risk_classifier","severity":"info","title":"Tests use mocks safely","detail":"Unit tests employ mock implementations that do not interact with external systems, preventing side effects.","file":"internal/reporters/github_report_test.go","line_start":1,"line_end":200},{"critic":"risk_classifier","severity":"info","title":"Interface abstraction does not reduce security","detail":"The GitHubClient interface only exposes methods used by the reporter; it does not expose any sensitive operations.","file":"internal/reporters/github.go","line_start":1,"line_end":120},{"critic":"risk_classifier","severity":"info","title":"No critical path changes","detail":"The diff does not modify authentication, payment, or other security-critical modules.","file":"Makefile","line_start":1,"line_end":20}],"critic_results":[{"critic":"security_smell","model":"qwen3-coder:480b","findings":[],"cost_usd":0.00020082000000000002,"duration_ms":989,"tokens_in":3308,"tokens_out":13},{"critic":"risk_classifier","model":"gpt-oss:20b","findings":[{"critic":"risk_classifier","severity":"info","title":"No secret exposure","detail":"All GitHub App credentials remain in secrets; only the app ID is moved to a variable, which is safe.","file":".github/workflows/acig.yml","line_start":1,"line_end":10},{"critic":"risk_classifier","severity":"info","title":"Safe use of ListPRFiles","detail":"The new ListPRFiles method is called with error handling; failures fall back to using all findings.","file":"internal/githubclient/review.go","line_start":1,"line_end":30},{"critic":"risk_classifier","severity":"info","title":"No SQL or XSS patterns","detail":"The code deals only with GitHub API interactions and string formatting; no database queries or HTML rendering that could lead to injection.","file":"internal/reporters/github.go","line_start":1,"line_end":120},{"critic":"risk_classifier","severity":"info","title":"No unsafe file I/O","detail":"The repository does not perform any file system operations; all I/O is via the GitHub API.","file":"internal/reporters/github.go","line_start":1,"line_end":120},{"critic":"risk_classifier","severity":"info","title":"No naked goroutines","detail":"The code does not spawn goroutines without proper synchronization or error handling.","file":"internal/reporters/github.go","line_start":1,"line_end":120},{"critic":"risk_classifier","severity":"info","title":"Tests use mocks safely","detail":"Unit tests employ mock implementations that do not interact with external systems, preventing side effects.","file":"internal/reporters/github_report_test.go","line_start":1,"line_end":200},{"critic":"risk_classifier","severity":"info","title":"Interface abstraction does not reduce security","detail":"The GitHubClient interface only exposes methods used by the reporter; it does not expose any sensitive operations.","file":"internal/reporters/github.go","line_start":1,"line_end":120},{"critic":"risk_classifier","severity":"info","title":"No critical path changes","detail":"The diff does not modify authentication, payment, or other security-critical modules.","file":"Makefile","line_start":1,"line_end":20}],"cost_usd":0.00006037,"duration_ms":5510,"tokens_in":3253,"tokens_out":928},{"critic":"perf_smell","model":"qwen3-coder:480b","findings":[],"cost_usd":0.00019878,"duration_ms":6533,"tokens_in":3274,"tokens_out":13},{"critic":"test_coverage_smell","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009366999999999999,"duration_ms":12280,"tokens_in":3223,"tokens_out":2048},{"critic":"style_conformance","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009353,"duration_ms":13054,"tokens_in":3209,"tokens_out":2048}],"total_cost_usd":0.00064717,"total_duration_ms":38366,"budget_remaining_usd":0.24935283,"generated_at":"2026-05-07T23:09:01.154443708Z"}

@@ -19,9 +19,9 @@ jobs:
- name: Get GitHub App token

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acig found 1 issue(s) here:

  • [info] No secret exposure (risk_classifier)
    All GitHub App credentials remain in secrets; only the app ID is moved to a variable, which is safe.

@@ -99,4 +99,16 @@ func (c *Client) RemoveStaleAcigComments(ctx context.Context, owner, repo string
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acig found 1 issue(s) here:

  • [info] Safe use of ListPRFiles (risk_classifier)
    The new ListPRFiles method is called with error handling; failures fall back to using all findings.

@@ -7,18 +7,30 @@ import (
"log/slog"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acig found 4 issue(s) here:

  • [info] No SQL or XSS patterns (risk_classifier)
    The code deals only with GitHub API interactions and string formatting; no database queries or HTML rendering that could lead to injection.

  • [info] No unsafe file I/O (risk_classifier)
    The repository does not perform any file system operations; all I/O is via the GitHub API.

  • [info] No naked goroutines (risk_classifier)
    The code does not spawn goroutines without proper synchronization or error handling.

  • [info] Interface abstraction does not reduce security (risk_classifier)
    The GitHubClient interface only exposes methods used by the reporter; it does not expose any sensitive operations.

@@ -0,0 +1,159 @@
package reporters

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acig found 1 issue(s) here:

  • [info] Tests use mocks safely (risk_classifier)
    Unit tests employ mock implementations that do not interact with external systems, preventing side effects.

Comment thread Makefile
@@ -12,6 +12,8 @@ test:
lint:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acig found 1 issue(s) here:

  • [info] No critical path changes (risk_classifier)
    The diff does not modify authentication, payment, or other security-critical modules.

@github-actions

github-actions Bot commented May 7, 2026

Copy link
Copy Markdown

Acig Verdict: PASS

Risk: low | Cost: $0.0007 | Duration: 35918ms

Decision: pass | Risk: low | Findings: 0 blocking, 0 high, 0 medium, 0 low, 4 info | Cost: $0.0007

Findings

[ℹ️ INFO] Dynamic ACIG version retrieval

  • Critic: risk_classifier
  • Location: .github/workflows/acig.yml:12

The workflow now fetches the latest ACIG release via curl/grep. This external call could fail if network access is blocked, but it does not expose any secrets.

[ℹ️ INFO] PR files pagination limit

  • Critic: risk_classifier
  • Location: internal/githubclient/review.go:1

ListPRFiles uses a single page of 100 files. PRs with more than 100 files will not be fully listed, potentially missing some findings. This is a functional limitation, not a security issue.

[ℹ️ INFO] Comment filtering fallback

  • Critic: risk_classifier
  • Location: internal/reporters/github.go:70

If ListPRFiles fails, the reporter falls back to commenting on all findings, including deleted files. This may produce irrelevant comments but does not compromise security.

[ℹ️ INFO] Use of vars for GitHub App ID

  • Critic: risk_classifier
  • Location: .github/workflows/acig.yml:5

The workflow now checks vars.ACIG_APP_ID instead of a secret. The ID is not sensitive, so no secret exposure occurs.

Critic Results

Critic Model Findings Cost Duration
security_smell qwen3-coder:480b 0 $0.0002 1230ms
perf_smell qwen3-coder:480b 0 $0.0002 1246ms
risk_classifier gpt-oss:20b 4 $0.0001 8869ms
test_coverage_smell gpt-oss:20b 0 $0.0001 11355ms
style_conformance gpt-oss:20b 0 $0.0001 13218ms

Generated by acig • SHA: 858e32b

Verdict JSON
{"schema_version":"1","repo":"https://github.com/helloodokai/acig","sha":"858e32b338a99d02f0845fb2d954655ab9f348ee","base_sha":"main","risk":"low","decision":"pass","summary":"Decision: pass | Risk: low | Findings: 0 blocking, 0 high, 0 medium, 0 low, 4 info | Cost: $0.0007","findings":[{"critic":"risk_classifier","severity":"info","title":"Dynamic ACIG version retrieval","detail":"The workflow now fetches the latest ACIG release via curl/grep. This external call could fail if network access is blocked, but it does not expose any secrets.","file":".github/workflows/acig.yml","line_start":12,"line_end":20},{"critic":"risk_classifier","severity":"info","title":"PR files pagination limit","detail":"ListPRFiles uses a single page of 100 files. PRs with more than 100 files will not be fully listed, potentially missing some findings. This is a functional limitation, not a security issue.","file":"internal/githubclient/review.go","line_start":1,"line_end":20},{"critic":"risk_classifier","severity":"info","title":"Comment filtering fallback","detail":"If ListPRFiles fails, the reporter falls back to commenting on all findings, including deleted files. This may produce irrelevant comments but does not compromise security.","file":"internal/reporters/github.go","line_start":70,"line_end":90},{"critic":"risk_classifier","severity":"info","title":"Use of vars for GitHub App ID","detail":"The workflow now checks vars.ACIG_APP_ID instead of a secret. The ID is not sensitive, so no secret exposure occurs.","file":".github/workflows/acig.yml","line_start":5,"line_end":10}],"critic_results":[{"critic":"security_smell","model":"qwen3-coder:480b","findings":[],"cost_usd":0.00020082000000000002,"duration_ms":1230,"tokens_in":3308,"tokens_out":13},{"critic":"perf_smell","model":"qwen3-coder:480b","findings":[],"cost_usd":0.00019878,"duration_ms":1246,"tokens_in":3274,"tokens_out":13},{"critic":"risk_classifier","model":"gpt-oss:20b","findings":[{"critic":"risk_classifier","severity":"info","title":"Dynamic ACIG version retrieval","detail":"The workflow now fetches the latest ACIG release via curl/grep. This external call could fail if network access is blocked, but it does not expose any secrets.","file":".github/workflows/acig.yml","line_start":12,"line_end":20},{"critic":"risk_classifier","severity":"info","title":"PR files pagination limit","detail":"ListPRFiles uses a single page of 100 files. PRs with more than 100 files will not be fully listed, potentially missing some findings. This is a functional limitation, not a security issue.","file":"internal/githubclient/review.go","line_start":1,"line_end":20},{"critic":"risk_classifier","severity":"info","title":"Comment filtering fallback","detail":"If ListPRFiles fails, the reporter falls back to commenting on all findings, including deleted files. This may produce irrelevant comments but does not compromise security.","file":"internal/reporters/github.go","line_start":70,"line_end":90},{"critic":"risk_classifier","severity":"info","title":"Use of vars for GitHub App ID","detail":"The workflow now checks vars.ACIG_APP_ID instead of a secret. The ID is not sensitive, so no secret exposure occurs.","file":".github/workflows/acig.yml","line_start":5,"line_end":10}],"cost_usd":0.00008878,"duration_ms":8869,"tokens_in":3253,"tokens_out":1875},{"critic":"test_coverage_smell","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009366999999999999,"duration_ms":11355,"tokens_in":3223,"tokens_out":2048},{"critic":"style_conformance","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009353,"duration_ms":13218,"tokens_in":3209,"tokens_out":2048}],"total_cost_usd":0.0006755800000000001,"total_duration_ms":35918,"budget_remaining_usd":0.24932442,"generated_at":"2026-05-07T23:12:31.989686882Z"}

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.

1 participant