marvin8/cang

Watch

Fork 0

Fix agent-reviews pipeline: all reviews via DeepSeek, lowercase secrets #89

Merged

coding-agent-marvin8 merged 0 commits from refs/pull/89/head into main

2026-06-05 07:23:54 +00:00

coding-agent-marvin8 commented

2026-06-05 06:45:32 +00:00

(Migrated from codeberg.org)

Fixes the new agent-reviews CI pipeline and script before they are merged.

YAML changes:

Remove ANTHROPIC_API_KEY env var (Anthropic removed entirely)
Lowercase from_secret names: deepseek_api_key, codeberg_token

Python changes:

Add SPDX-License-Identifier header
Remove ANTHROPIC_API_KEY config and call_anthropic() function
Switch Security and Performance agents from Anthropic to deepseek-chat
Extract system prompts to module-level constants (avoids E501 at 8-space indent)
Rename resp → response, raw → raw_text, c → comment

Closes #88

Fixes the new agent-reviews CI pipeline and script before they are merged. **YAML changes:** - Remove `ANTHROPIC_API_KEY` env var (Anthropic removed entirely) - Lowercase `from_secret` names: `deepseek_api_key`, `codeberg_token` **Python changes:** - Add SPDX-License-Identifier header - Remove `ANTHROPIC_API_KEY` config and `call_anthropic()` function - Switch Security and Performance agents from Anthropic to `deepseek-chat` - Extract system prompts to module-level constants (avoids E501 at 8-space indent) - Rename `resp` → `response`, `raw` → `raw_text`, `c` → `comment` Closes #88

coding-agent-marvin8 commented

2026-06-05 07:18:17 +00:00

(Migrated from codeberg.org)

Thanks for the thorough review. I've addressed two genuine points (invalid line numbers in the LLM output now skip cleanly instead of crashing, and PR_INDEX is validated at startup). Here's my reasoning for not implementing the rest:

Secrets as env vars (lines 36, 39, 41; yaml line 14)

This is the standard Woodpecker pattern — secrets are injected as env vars, and Woodpecker masks them in logs automatically. os.environ['KEY'] (not .get()) is intentional: it fails loudly at startup if a secret is missing rather than propagating None halfway through. File-based token storage would require baking secrets into the image or mounting volumes — significantly more complex CI config for no real security gain in an already-trusted container.

Diff sanitisation before sending to AI (line 191)

The diff is the content being reviewed. Sanitising it before sending would remove the information the AI needs to do its job. A diff containing credentials would be a finding from the Security agent — the AI doesn't execute the diff content.

Hardcoded DeepSeek URL (line 247)

This is deliberate. Making the endpoint configurable via env var would let anyone with pipeline write access redirect all diffs (and the API key) to an arbitrary server. Hardcoding is the secure choice here.

JSON parsing without error handling (line 269)

Already implemented — lines 276–281 wrap json.loads() in a try/except that logs the error and returns []. This comment was incorrect.

XSS from AI output (line 210)

Codeberg sanitises HTML in markdown comments. Sanitising the AI output before posting would corrupt the review text (stripping backticks, symbols, inline code). This is an internal CI tool posting to a trusted platform, not a public web app.

Retry logic (lines 209, 232)

For a CI review tool, the right retry mechanism is re-running the pipeline step. Exponential backoff adds ~40 lines of complexity for an edge case the CI UI already handles. The warning printed on failure is sufficient signal.

requests.Session and frozen headers (line 48)

The script makes at most 6 HTTP requests total. Connection-reuse benefit is negligible. Converting to Session would add complexity without measurable gain in a short-lived process.

AGENTS as a config file (line 136)

Moving AGENTS to a separate YAML/JSON file adds file-loading code, error handling for missing files, a new file to keep in sync, and no testability improvement — the agents are tested end-to-end anyway. The 20-line inline list is clearer.

Rename file/line to path/new_position in prompts (line 80)

"file" and "line" are natural English words that produce better LLM JSON compliance than API field names. The translation to Forgejo's path/new_position is an intentional and explicit boundary in post_review().

Combine the two Forgejo API calls (line 270)

GET /pulls/{index} returns JSON metadata. GET /pulls/{index}.diff returns text/x-diff. They're different content types with different endpoints — there's no single Forgejo API call that returns both.

Diff chunking for large PRs (line 268)

Valid future concern, but chunking (split by file, track diff-position offsets, reassemble) is a significant feature addition, not appropriate for this bug-fix PR. Worth a follow-up issue.

Synchronous LLM calls (line 232)

The reviewer notes this is "acceptable in CI context" — agreed, it is a CI context. Async/threading for four sequential agents adds complexity with no meaningful improvement when each call takes 10–60 s anyway.

Memory for large comment lists (line 210)

Each agent output is capped at 2048 tokens. Even in the worst case that's a few hundred small strings — kilobytes. Batching would add complexity for a non-problem.

Thanks for the thorough review. I've addressed two genuine points (invalid line numbers in the LLM output now skip cleanly instead of crashing, and `PR_INDEX` is validated at startup). Here's my reasoning for not implementing the rest: --- **Secrets as env vars (lines 36, 39, 41; yaml line 14)** This is the standard Woodpecker pattern — secrets are injected as env vars, and Woodpecker masks them in logs automatically. `os.environ['KEY']` (not `.get()`) is intentional: it fails loudly at startup if a secret is missing rather than propagating `None` halfway through. File-based token storage would require baking secrets into the image or mounting volumes — significantly more complex CI config for no real security gain in an already-trusted container. **Diff sanitisation before sending to AI (line 191)** The diff is the content being reviewed. Sanitising it before sending would remove the information the AI needs to do its job. A diff containing credentials would be a *finding* from the Security agent — the AI doesn't execute the diff content. **Hardcoded DeepSeek URL (line 247)** This is deliberate. Making the endpoint configurable via env var would let anyone with pipeline write access redirect all diffs (and the API key) to an arbitrary server. Hardcoding is the secure choice here. **JSON parsing without error handling (line 269)** Already implemented — lines 276–281 wrap `json.loads()` in a try/except that logs the error and returns `[]`. This comment was incorrect. **XSS from AI output (line 210)** Codeberg sanitises HTML in markdown comments. Sanitising the AI output before posting would corrupt the review text (stripping backticks, symbols, inline code). This is an internal CI tool posting to a trusted platform, not a public web app. **Retry logic (lines 209, 232)** For a CI review tool, the right retry mechanism is re-running the pipeline step. Exponential backoff adds ~40 lines of complexity for an edge case the CI UI already handles. The warning printed on failure is sufficient signal. **`requests.Session` and frozen headers (line 48)** The script makes at most 6 HTTP requests total. Connection-reuse benefit is negligible. Converting to Session would add complexity without measurable gain in a short-lived process. **AGENTS as a config file (line 136)** Moving AGENTS to a separate YAML/JSON file adds file-loading code, error handling for missing files, a new file to keep in sync, and no testability improvement — the agents are tested end-to-end anyway. The 20-line inline list is clearer. **Rename `file`/`line` to `path`/`new_position` in prompts (line 80)** `"file"` and `"line"` are natural English words that produce better LLM JSON compliance than API field names. The translation to Forgejo's `path`/`new_position` is an intentional and explicit boundary in `post_review()`. **Combine the two Forgejo API calls (line 270)** `GET /pulls/{index}` returns JSON metadata. `GET /pulls/{index}.diff` returns `text/x-diff`. They're different content types with different endpoints — there's no single Forgejo API call that returns both. **Diff chunking for large PRs (line 268)** Valid future concern, but chunking (split by file, track diff-position offsets, reassemble) is a significant feature addition, not appropriate for this bug-fix PR. Worth a follow-up issue. **Synchronous LLM calls (line 232)** The reviewer notes this is "acceptable in CI context" — agreed, it is a CI context. Async/threading for four sequential agents adds complexity with no meaningful improvement when each call takes 10–60 s anyway. **Memory for large comment lists (line 210)** Each agent output is capped at 2048 tokens. Even in the worst case that's a few hundred small strings — kilobytes. Batching would add complexity for a non-problem.

No reviewers

No labels

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

marvin8/cang!89

No description provided.

Rows
Columns