feat: add Kilo Gastown methodology spec and SAST tooling#942
feat: add Kilo Gastown methodology spec and SAST tooling#942KooshaPari wants to merge 13 commits intomainfrom
Conversation
Document Kilo Gastown concepts for cliproxyapi++: - Convoys, beads, polecats, rigs, and towns - Delegation mechanisms (gt_sling, gt_sling_batch) - Bead lifecycle and coordination - Merge modes and agent workflow - Bot review governance - Quality gates for pre-submission validation Part of convoy AgilePlus + Kilo Specs: cliproxyapi++
- Add semgrep configuration and custom rules - Add full and quick SAST GitHub workflows
|
CodeAnt AI is reviewing your PR. |
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
GitHub Governance .github/RULESET_BASELINE.md, .github/rulesets/main.json |
Adds branch protection ruleset baseline documentation and enforced merge rules (PR requirement, no force-push, stale review dismissal, approved reviews, enabled code quality and Copilot review). |
CI Workflows .github/workflows/ci.yml |
Updates models catalog refresh step to write to pkg/llmproxy/registry/models/models.json and creates the destination directory. |
SAST Security Workflows .github/workflows/sast-full.yml, .github/workflows/sast-quick.yml, .github/workflows/security-guard.yml |
Adds daily full SAST analysis (CodeQL, Trivy, Semgrep, TruffleHog) and PR quick-check workflows (Semgrep, TruffleHog, Go fmt/vet, license inventory); upgrades security-guard with ggshield scanning and workflow_call triggers. |
Semgrep Rules & Configuration .semgrep-rules/architecture-violations.yml, .semgrep-rules/secrets-detection.yml, .semgrep-rules/unsafe-patterns.yml, .semgrep.yaml |
Adds Go architecture checks (no dot imports, restricted blank imports, no env reads in handlers), secret detection patterns (AWS keys, API tokens, passwords, Slack/GitHub tokens), unsafe patterns (shell exec, SQL injection, HTTP timeouts), and root Semgrep config excluding VCS/build/test paths. |
Stabilization & Planning Docs PLAN.md, docs/plans/KILO_GASTOWN_SPEC.md |
Marks plan complete with maintenance-only guidance; generalizes Kilo Gastown concepts from clipproxyapi++-specific to rig/town/agent abstractions with expanded lifecycle and operational sections. |
PR Readiness Session Documentation docs/sessions/2026-04-01-pr-readiness-refresh/* |
Adds session overview, research findings on archived actions, acceptance criteria, implementation strategy, known issues (import cycles, Snyk billing, Semgrep false positives), and testing validation results. |
Code Refactor pkg/llmproxy/executor/codex_executor.go |
Converts else if from == "openai" to standalone case "openai" within existing switch statement; no logic change. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
HELIOS-CODEX, HELIOS-CODEX-L0
Poem
🐰 A ruleset hops into place,
SAST guards scan the space,
Semgrep rules bound and bright,
Security's set just right,
The docs dance—stabilized with delight! ✨
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The PR title accurately summarizes the two main additions: Kilo Gastown methodology specification and SAST tooling infrastructure (workflows, rules, configuration). Both are prominently reflected in the file changes. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feat/kilo-gastown-spec-and-sast
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request formalizes the development methodology for the cliproxyapi++ repository by updating the Kilo Gastown specification and integrating automated static analysis security testing (SAST) tools. These changes aim to standardize agent-based development workflows and enforce security best practices across the codebase. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
codeant-ai
bot
added
the
size:XL
|
CodeAnt AI finished reviewing your PR. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a suite of Semgrep rules for architecture violations, secrets detection, and unsafe patterns, alongside a major update to the Kilo Gastown methodology specification. The review feedback correctly identifies a significant mismatch where the newly added Semgrep rules target Rust syntax and patterns despite the project being written in Go. Additionally, the feedback highlights opportunities to improve secret detection patterns for Go's assignment operators, resolve redundant configuration in the main Semgrep file, and update the documentation to include SAST as a required quality gate.
| - pattern: | | ||
| use crate::adapters::*; | ||
| message: Potential circular dependency. Check module import hierarchy. | ||
| languages: [rust] |
There was a problem hiding this comment.
The repository cliproxyapi++ is a Go project (as evidenced by the Taskfile.yml, AGENTS.md, and .go source files), but these Semgrep rules are configured for Rust (languages: [rust]) and use Rust-specific syntax like crate:: and async fn. These rules will not execute on the Go source code. Please update the rules to use languages: [go] and patterns appropriate for the Go project structure.
| - pattern: | | ||
| bincode::deserialize($X) | ||
| message: Unsafe deserialization without validation. Consider using `serde` with validation attributes. | ||
| languages: [rust] |
There was a problem hiding this comment.
These unsafe pattern rules are targeting Rust. For this Go project, consider adding rules for common Go issues such as unhandled errors, fmt.Sprintf in SQL queries (injection), or improper use of the unsafe package.
| - id: circular-module-dependency | ||
| patterns: | ||
| - pattern-either: | ||
| - pattern: | | ||
| use crate::domain::*; | ||
| - pattern: | | ||
| use crate::adapters::*; | ||
| message: Potential circular dependency. Check module import hierarchy. |
There was a problem hiding this comment.
The rule circular-module-dependency is misleadingly named. It currently flags any wildcard import (*) in the domain or adapters modules, which is a style preference rather than a detection of circular dependencies. Consider renaming the rule to no-wildcard-imports or implementing a more robust check for actual dependency cycles.
| api_key = "..." | ||
| - pattern: | | ||
| apiKey = "..." | ||
| - pattern: | | ||
| API_KEY = "..." |
There was a problem hiding this comment.
The patterns for hardcoded keys use the = operator, which is common in configuration files but may miss assignments in Go source code where the short variable declaration operator := is frequently used. Consider adding patterns that account for := or using a more flexible regex to catch various assignment styles in the generic language mode.
| metavariable: $X | ||
| comparison: match_regex | ||
| regex: | | ||
| (?!(log|debug|info|warn|error)).* |
There was a problem hiding this comment.
The regex (?!(log|debug|info|warn|error)).* used in the unwrap-without-context rule is confusing. It appears to exclude variables starting with logging levels from being flagged when .unwrap() is called on them. However, in production code, .unwrap() is generally discouraged regardless of the variable name. It would be clearer to flag all occurrences and exclude test files via the global configuration.
| exclude: | ||
| - tests/ | ||
| - "**/*_test.rs" | ||
| - "**/*_test.go" | ||
| - "**/*.test.ts" | ||
| - "**/*.test.js" | ||
| - "**/__pycache__" | ||
| - "**/__tests__" | ||
| - node_modules/ | ||
| - target/ | ||
| - build/ | ||
| - dist/ | ||
|
|
||
| max-lines-per-finding: 10 | ||
| paths: | ||
| exclude: | ||
| - ".git/" | ||
| - "target/" | ||
| - ".archive/" | ||
| - "node_modules/" | ||
| - "dist/" | ||
| - "build/" |
There was a problem hiding this comment.
There is redundant exclusion logic in this configuration. The top-level exclude key (lines 6-17) and the paths: exclude: key (lines 20-27) contain overlapping entries (e.g., target/, node_modules/). It is recommended to consolidate all exclusions under paths: exclude: for better maintainability and adherence to standard Semgrep configuration patterns.
| | Tests | `go test ./...` | 80% coverage | | ||
| | Lint | `golangci-lint run` | 0 errors | | ||
| | Vet | `go vet ./...` | 0 errors | | ||
| | Format | `go fmt ./...` | No diff | |
There was a problem hiding this comment.
Since this PR introduces Semgrep for SAST, the "Quality Gates" table should be updated to include it. This ensures that agents and contributors are aware of the requirement to run security scans before submission.
| | Format | `go fmt ./...` | No diff | | |
| | Format | `go fmt ./...` | No diff | | |
| | SAST | `semgrep scan` | 0 findings | |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| @@ -0,0 +1,27 @@ | |||
| rules: | |||
| - id: config | |||
| include: | |||
There was a problem hiding this comment.
CRITICAL: Invalid Semgrep configuration structure.
The rules: key with nested id: and include: is not valid Semgrep config syntax. Semgrep expects rules: to be a list of rule definitions OR references to config files using the rules: key with - path/to/rules.yml entries. The current structure will cause Semgrep to fail to parse this file, breaking all CI SAST scans.
Correct format:
rules:
- .semgrep-rules/architecture-violations.yml
- .semgrep-rules/secrets-detection.yml
- .semgrep-rules/unsafe-patterns.yml| matrix: | ||
| language: [python, cpp, javascript] | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
WARNING: CodeQL matrix missing Go language.
The matrix lists [python, cpp, javascript] but this is primarily a Go codebase (per cargo clippy in sast-quick.yml, the AGENTS.md build instructions, and the codebase structure). Without go in the matrix, CodeQL will not analyze the core Go code, making the full SAST scan incomplete for this repository.
| - uses: actions/checkout@v4 | |
| language: [go, python, javascript] |
Note: also removed cpp since this doesn't appear to be a C++ project. Adjust if there is C++ code present.
| name: Trivy Repository Scan | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
WARNING: Using @master branch reference for trivy-action.
Pinning to a mutable branch (@master) introduces supply-chain risk and non-reproducible builds. If the upstream action is compromised or introduces breaking changes, your CI will silently adopt them.
Recommend pinning to a specific version tag or commit SHA:
- uses: aquasecurity/trivy-action@v0.28.0| with: | ||
| config: >- | ||
| p/security-audit | ||
| p/owasp-top-ten |
There was a problem hiding this comment.
WARNING: returntocorp/semgrep-action@v1 is deprecated.
The returntocorp/semgrep-action has been deprecated in favor of running the Semgrep CLI directly or using the semgrep/semgrep-action container. The @v1 tag may stop receiving updates.
Recommended replacement:
- uses: semgrep/semgrep-action@v1Or run semgrep CLI directly:
- run: semgrep ci --config p/security-audit --config p/owasp-top-ten --config p/cwe-top-25 --config .semgrep-rules/ --sarif --output semgrep.sarif| path: ./ | ||
| extra_args: --only-verified --fail | ||
| continue-on-error: true | ||
|
|
There was a problem hiding this comment.
WARNING: Using @main branch reference for trufflehog.
Pinning to @main introduces supply-chain risk and non-reproducible builds. Pin to a specific release tag or commit SHA for security:
- uses: trufflesecurity/trufflehog@v3.82.12
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Incremental Update (since commit 576b830)This commit migrates Go imports from Overview
New Issues from Incremental Diff (click to expand)WARNING
SUGGESTION
OBSERVATION
Resolved Issues from Previous Review (click to expand)
Carried Forward (not in diff)
Positive Changes
Files Reviewed (28 files)New files:
Modified files:
Fix these issues in Kilo Cloud Reviewed by mimo-v2-pro-20260318 · 594,036 tokens |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| timeout-minutes: 30 | ||
| strategy: | ||
| matrix: | ||
| language: [python, cpp, javascript] |
There was a problem hiding this comment.
CodeQL scans wrong languages, misses Go entirely
High Severity
The CodeQL language matrix is [python, cpp, javascript], but this is a Go repository (the go.mod declares module github.com/kooshapari/CLIProxyAPI/v7). The existing codeql.yml correctly targets [go]. This full SAST analysis will scan three languages that have no source files in the repo while completely missing the actual Go codebase — rendering the CodeQL job useless.
| with: | ||
| components: clippy | ||
| - uses: Swatinem/rust-cache@v2 | ||
| - run: cargo clippy --all-targets --all-features -- -D warnings |
There was a problem hiding this comment.
Rust lint job runs on Go-only repository
Medium Severity
The lint-rust job runs cargo clippy on every PR and push to main, but this repository contains no Rust code — no Cargo.toml, no .rs files. This job will either fail (wasting CI time and creating noise) or require cargo setup for nothing. For a Go repo, this likely intended to be a Go lint step.
| return Err(e) | ||
| message: Error propagated without context. Consider using `.context()` from `anyhow` or `.map_err()`. | ||
| languages: [rust] | ||
| severity: LOW |
There was a problem hiding this comment.
Semgrep custom rules target Rust, not Go
Medium Severity
All custom semgrep rules in unsafe-patterns.yml and architecture-violations.yml specify languages: [rust] with Rust-specific patterns (unwrap(), format!(), use crate::, etc.). This is a Go repository with no Rust code, so none of these rules will ever match anything. The entire custom rule suite is inert.
Additional Locations (1)
| with: | ||
| path: ./ | ||
| extra_args: --only-verified --fail | ||
| continue-on-error: true |
There was a problem hiding this comment.
Secret scan continue-on-error defeats --fail flag
Medium Severity
The trufflehog step uses --fail (exit non-zero on verified secrets) but also sets continue-on-error: true, which means the job still reports success. In sast-quick.yml this runs on PRs, so a pull request containing verified leaked secrets will not be blocked by this check — undermining the purpose of secret scanning in CI.
Additional Locations (1)
| - ".archive/" | ||
| - "node_modules/" | ||
| - "dist/" | ||
| - "build/" |
There was a problem hiding this comment.
Semgrep config file has invalid rule structure
Medium Severity
The .semgrep.yaml rule entry id: config with include: is missing all required semgrep rule fields: pattern/patterns, message, severity, and languages. The include key is not a recognized rule field. Top-level keys like exclude: and max-lines-per-finding: are CLI flags, not valid YAML config keys. This file will cause parse errors when semgrep auto-discovers it (e.g., semgrep --config .), breaking local development usage.
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - uses: trufflesecurity/trufflehog@main |
There was a problem hiding this comment.
Security actions pinned to mutable branch references
Medium Severity
trufflesecurity/trufflehog@main and aquasecurity/trivy-action@master are pinned to mutable branch refs rather than commit SHAs or immutable version tags. A compromised upstream repo could inject malicious code that runs in CI with contents: read and security-events: write permissions. This is especially concerning for security-focused workflows running on every PR.
Additional Locations (2)
There was a problem hiding this comment.
Pull request overview
This PR introduces a Kilo Gastown methodology spec for this repository and adds CI-based SAST/security scanning via Semgrep and TruffleHog (plus additional “full” scheduled tooling).
Changes:
- Expanded/updated
docs/plans/KILO_GASTOWN_SPEC.mdwith additional terminology, workflows, and references. - Added Semgrep configuration and custom rule packs under
.semgrep-rules/. - Added two GitHub Actions workflows: a PR/push “quick” SAST check and a scheduled/manual “full” SAST analysis.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/plans/KILO_GASTOWN_SPEC.md | Updates/expands methodology spec and adds references/tooling guidance. |
| .semgrep.yaml | Adds local Semgrep configuration file. |
| .semgrep-rules/unsafe-patterns.yml | Adds custom Semgrep rules for unsafe patterns. |
| .semgrep-rules/secrets-detection.yml | Adds custom Semgrep rules intended for secret detection. |
| .semgrep-rules/architecture-violations.yml | Adds custom Semgrep rules intended for architecture violations. |
| .github/workflows/sast-quick.yml | Adds PR/push Semgrep + TruffleHog + (Rust) lint + license checks. |
| .github/workflows/sast-full.yml | Adds scheduled/manual CodeQL + Trivy + Semgrep + TruffleHog scans. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | `in_review` | Work submitted to review queue; refinery handles merge | | ||
| | `closed` | Work fully completed and merged | | ||
|
|
There was a problem hiding this comment.
The "Merge Modes" table mixes actual merge strategies (squash/rebase/merge) with bead status values (in_review, closed). That makes the section internally inconsistent and can confuse readers about what a merge mode is. Consider keeping this table to merge strategies only, and documenting bead lifecycle states in the Beads/Workflow sections instead.
| | `in_review` | Work submitted to review queue; refinery handles merge | | |
| | `closed` | Work fully completed and merged | | |
| Bead lifecycle states such as `in_review` and `closed` are documented in the Beads/Workflow sections. |
| - [cliproxyapi++ SPEC.md](./SPEC.md) — Technical architecture | ||
| - [cliproxyapi++ FEATURE_CHANGES_PLUSPLUS.md](./FEATURE_CHANGES_PLUSPLUS.md) — ++ vs baseline changes | ||
| - [Kush multi-repo system overview](../AGENTS.md) | ||
| - [AGENTS.md: Agent guidance for this repository](./AGENTS.md) | ||
| - [Phenotype Governance Overlay v1: TDD/BDD/SDD requirements](../governance/PHENOTYPE_GOVERNANCE.md) | ||
| - [Kush Ecosystem: Multi-repo system overview](../README.md) |
There was a problem hiding this comment.
The relative links in References appear to be incorrect from docs/plans/: ./SPEC.md, ./FEATURE_CHANGES_PLUSPLUS.md, and ./AGENTS.md do not exist in this directory, and ../governance/PHENOTYPE_GOVERNANCE.md does not exist in the repo. Update these to point at the actual paths (e.g., ../SPEC.md, ../FEATURE_CHANGES_PLUSPLUS.md, and the correct location of AGENTS.md / governance doc).
| rules: | ||
| - id: config | ||
| include: | ||
| - .semgrep-rules/ |
There was a problem hiding this comment.
This Semgrep config is not valid Semgrep rule syntax: include: is nested under a rule (- id: config), but include is not a rule property. If the intent is to include the rule directory, use Semgrep's supported config composition (e.g., top-level include: / extends: depending on Semgrep version) or run Semgrep with --config .semgrep-rules/ instead.
| rules: | |
| - id: config | |
| include: | |
| - .semgrep-rules/ | |
| include: | |
| - .semgrep-rules/ |
| - id: hardcoded-aws-key | ||
| patterns: | ||
| - pattern-either: | ||
| - pattern: | | ||
| "AKIA[0-9A-Z]{16}" | ||
| - pattern: | | ||
| AKIA[0-9A-Z]{16} |
There was a problem hiding this comment.
These secret-detection rules are written like regexes, but pattern: matches literal code/text structure, not a regular expression. As written, this will only match the literal string AKIA[0-9A-Z]{16} (and similarly for the token/webhook patterns), so it won’t detect real secrets. Use pattern-regex: (or regex:-based constructs) for token-like detectors, and avoid "..." which currently matches only three dots.
| - id: unsafe-sql | ||
| patterns: | ||
| - pattern: | | ||
| format!($QUERY, $ARG) |
There was a problem hiding this comment.
format!($QUERY, $ARG) will flag any two-argument format! usage, even when the formatted string isn't SQL, while missing common multi-argument format! calls. This makes the rule's "SQL injection" message inaccurate/noisy. Consider constraining matches to known SQL contexts (e.g., variables/identifiers used in query APIs) or using a regex that checks for SQL keywords in the format string.
| format!($QUERY, $ARG) | |
| format!($QUERY, $ARG, ...) | |
| - metavariable-regex: | |
| metavariable: $QUERY | |
| regex: '(?i)\b(SELECT|INSERT|UPDATE|DELETE|FROM|WHERE|JOIN)\b' |
| lint-rust: | ||
| name: Rust Lint | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: dtolnay/rust-toolchain@stable | ||
| with: | ||
| components: clippy | ||
| - uses: Swatinem/rust-cache@v2 | ||
| - run: cargo clippy --all-targets --all-features -- -D warnings |
There was a problem hiding this comment.
The lint-rust job runs cargo clippy, but this repo doesn’t contain a Cargo project (no Cargo.toml found). This job will fail on every run. Remove the Rust lint job, or guard it (e.g., only run when Cargo.toml exists / when Rust paths change).
| codeql: | ||
| name: CodeQL Analysis | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 30 | ||
| strategy: | ||
| matrix: | ||
| language: [python, cpp, javascript] | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Initialize CodeQL | ||
| uses: github/codeql-action/init@v3 | ||
| with: | ||
| languages: ${{ matrix.language }} | ||
|
|
||
| - name: Perform CodeQL Analysis | ||
| uses: github/codeql-action/analyze@v3 |
There was a problem hiding this comment.
This CodeQL matrix doesn’t include the repo’s primary language (Go) and includes cpp even though there are no C/C++ sources in the repository. Also, the repo already has a dedicated Go CodeQL workflow (.github/workflows/codeql.yml). Consider either extending the existing CodeQL workflow for additional languages present here (Go/Python/JS) or adjusting this job to scan the actual languages in the repo and avoid duplicating CodeQL uploads.
| - uses: actions/checkout@v4 | ||
| - uses: aquasecurity/trivy-action@master | ||
| with: | ||
| scan-type: fs | ||
| scan-ref: . | ||
| format: sarif | ||
| output: trivy-results.sarif | ||
| continue-on-error: true |
There was a problem hiding this comment.
aquasecurity/trivy-action@master and trufflesecurity/trufflehog@main are floating refs. For a security workflow, these should be pinned to a version tag or (ideally) a commit SHA to reduce supply-chain risk and ensure reproducible scans.
| with: | ||
| path: ./ | ||
| extra_args: --only-verified --fail | ||
| continue-on-error: true |
There was a problem hiding this comment.
Same as in the quick workflow: trufflehog is run with --fail but the step is continue-on-error: true, so verified secret findings won’t fail the scheduled/manual run. Decide whether this should be enforcing (remove continue-on-error) or informational (remove --fail).
| continue-on-error: true |
| - uses: trufflesecurity/trufflehog@main | ||
| with: | ||
| path: ./ | ||
| extra_args: --only-verified --fail | ||
| continue-on-error: true |
There was a problem hiding this comment.
trufflesecurity/trufflehog@main is a floating ref. For supply-chain safety and reproducible results, pin to a version tag or commit SHA (especially in security workflows).
- Add ruleset baseline and rulesets/main.json - Update security-guard workflow - Update PLAN.md
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
|
CodeAnt AI is running Incremental review |
codeant-ai
bot
added
size:XL
|
CodeAnt AI Incremental review completed. |
Resolve the Kilo Gastown spec merge conflict and retain the FOSSA README badges. Co-authored-by: Codex <noreply@openai.com>
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with: | ||
| path: ./ | ||
| extra_args: --only-verified --fail | ||
| continue-on-error: true |
There was a problem hiding this comment.
This step sets --fail in TruffleHog args, but the action step is marked continue-on-error: true, which prevents failures from ever gating. Decide whether secret findings should block (remove continue-on-error) or be informational (remove --fail).
| continue-on-error: true |
| permissions: | ||
| contents: read | ||
| security-events: write | ||
| pull-requests: write |
There was a problem hiding this comment.
Workflow permissions include pull-requests: write, but this workflow doesn’t appear to create/update PR comments or reviews. Consider dropping it to follow least-privilege (keep contents: read and security-events: write).
| pull-requests: write |
| - uses: actions/checkout@v4 | ||
| - uses: aquasecurity/trivy-action@master | ||
| with: | ||
| scan-type: fs | ||
| scan-ref: . | ||
| format: sarif | ||
| output: trivy-results.sarif | ||
| continue-on-error: true |
There was a problem hiding this comment.
The Trivy action is referenced from the moving @master branch. Pin to a version tag or commit SHA to avoid unexpected behavior changes and reduce supply-chain risk.
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - uses: trufflesecurity/trufflehog@main | ||
| with: | ||
| path: ./ | ||
| extra_args: --only-verified --fail | ||
| continue-on-error: true |
There was a problem hiding this comment.
The TruffleHog action is referenced from the moving @main branch. Pin to a release tag or commit SHA for reproducibility and supply-chain safety.
| - id: hardcoded-aws-key | ||
| patterns: | ||
| - pattern-either: | ||
| - pattern: | | ||
| "AKIA[0-9A-Z]{16}" | ||
| - pattern: | | ||
| AKIA[0-9A-Z]{16} | ||
| message: Potential AWS Access Key detected. Use environment variables instead. | ||
| languages: [generic] | ||
| severity: CRITICAL | ||
|
|
||
| - id: hardcoded-api-key-env | ||
| patterns: | ||
| - pattern-either: | ||
| - pattern: | | ||
| api_key = "..." | ||
| - pattern: | | ||
| apiKey = "..." | ||
| - pattern: | | ||
| API_KEY = "..." | ||
| message: Hardcoded API key detected. Use environment variables or secrets management. | ||
| languages: [generic] | ||
| severity: HIGH | ||
|
|
There was a problem hiding this comment.
These secret-detection rules appear to use regex syntax inside pattern, but Semgrep won’t treat pattern as a regex (so strings like AKIA[0-9A-Z]{16} and "..." will match literally and likely never trigger). Consider switching to pattern-regex (or using metavariables) so the rules actually detect real secrets.
| - [cliproxyapi++ SPEC.md](./SPEC.md) — Technical architecture | ||
| - [cliproxyapi++ FEATURE_CHANGES_PLUSPLUS.md](./FEATURE_CHANGES_PLUSPLUS.md) — ++ vs baseline changes | ||
| - [AGENTS.md: Agent guidance for this repository](./AGENTS.md) | ||
| - [Kush AGENTS.md](../AGENTS.md) — Full Kilo Gastown methodology reference | ||
| - [Phenotype Governance Overlay v1: TDD/BDD/SDD requirements](../governance/PHENOTYPE_GOVERNANCE.md) | ||
| - [Kush Ecosystem: Multi-repo system overview](../README.md) |
There was a problem hiding this comment.
The reference links here are currently relative to docs/plans/, but the targets live elsewhere (e.g., docs/SPEC.md, docs/FEATURE_CHANGES_PLUSPLUS.md, and the repo root AGENTS.md). Update the paths so these links resolve, and replace the ../governance/PHENOTYPE_GOVERNANCE.md link with the actual location (the Phenotype Governance section exists in AGENTS.md).
| - [cliproxyapi++ SPEC.md](./SPEC.md) — Technical architecture | |
| - [cliproxyapi++ FEATURE_CHANGES_PLUSPLUS.md](./FEATURE_CHANGES_PLUSPLUS.md) — ++ vs baseline changes | |
| - [AGENTS.md: Agent guidance for this repository](./AGENTS.md) | |
| - [Kush AGENTS.md](../AGENTS.md) — Full Kilo Gastown methodology reference | |
| - [Phenotype Governance Overlay v1: TDD/BDD/SDD requirements](../governance/PHENOTYPE_GOVERNANCE.md) | |
| - [Kush Ecosystem: Multi-repo system overview](../README.md) | |
| - [cliproxyapi++ SPEC.md](../SPEC.md) — Technical architecture | |
| - [cliproxyapi++ FEATURE_CHANGES_PLUSPLUS.md](../FEATURE_CHANGES_PLUSPLUS.md) — ++ vs baseline changes | |
| - [AGENTS.md: Agent guidance for this repository](../../AGENTS.md) | |
| - [Kush AGENTS.md](../../AGENTS.md) — Full Kilo Gastown methodology reference | |
| - [Phenotype Governance Overlay v1: TDD/BDD/SDD requirements](../../AGENTS.md#phenotype-governance) | |
| - [Kush Ecosystem: Multi-repo system overview](../../README.md) |
| - name: Initialize CodeQL | ||
| uses: github/codeql-action/init@v3 | ||
| with: | ||
| languages: ${{ matrix.language }} | ||
|
|
||
| - name: Perform CodeQL Analysis | ||
| uses: github/codeql-action/analyze@v3 | ||
|
|
There was a problem hiding this comment.
This workflow uses CodeQL action v3 (github/codeql-action/*@v3), but the existing .github/workflows/codeql.yml uses v4 (see .github/workflows/codeql.yml:24-36). Aligning on a single major version reduces maintenance overhead and avoids behavioral differences across workflows.
Resolve the lingering Kilo Gastown spec branch residue, replace deprecated or broken SAST workflow wiring, and record the current blocker set in a session bundle. Co-authored-by: Codex <noreply@openai.com>
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
Point the CI refresh step at the embedded registry path that exists in this repository so the workflow can proceed past setup on pull requests. Co-authored-by: Codex <noreply@openai.com>
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
|
CodeAnt AI is running Incremental review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/plans/KILO_GASTOWN_SPEC.md (1)
346-356: 🧹 Nitpick | 🔵 TrivialDuplicate ecosystem diagram—consider removing one instance.
This "Kush Ecosystem" diagram (lines 346-356) is nearly identical to the diagram at lines 236-242. The only difference is the additional entries (
4sgm/,civ/,parpour/,pheno-sdk/) in this second instance. Consider consolidating into a single, complete diagram to avoid maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/KILO_GASTOWN_SPEC.md` around lines 346 - 356, The file contains a duplicated "Kush Ecosystem" ASCII tree (the code block starting with "kush/" and entries like "thegent/", "agentapi++/", "cliproxy++/") — keep a single, complete diagram and remove the duplicate: merge the additional entries (`4sgm/`, `civ/`, `parpour/`, `pheno-sdk/`) into the primary diagram and delete the redundant block so there is one canonical ecosystem diagram in the document.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/RULESET_BASELINE.md:
- Around line 31-37: The manifests `.github/required-checks.txt` and
`.github/release-required-checks.txt` only list `pr-test-build` and
`pr-path-guard` but are missing the documented workflow names; update both
manifest files to include the full set of documented checks (`policy-gate`,
`required-check-names-guard`, `quality-gate`, `security-guard`, `codeql`,
`sast-quick`, `sast-full`, plus the existing `pr-test-build` and
`pr-path-guard`) so the manifests match the workflows listed in lines 21-29 and
can reliably drive the next ruleset wave. Ensure the names exactly match the
workflow job names used in the repository (case-sensitive) and save both
`.github/required-checks.txt` and `.github/release-required-checks.txt`.
- Around line 1-43: Add metadata to the RULESET_BASELINE.md baseline by
inserting a small header block containing a version identifier (e.g., "v1.0" or
date), a one-line changelog section and a link/reference to the corresponding
ruleset JSON file; update the top of the document (the existing header
"cliproxyapi-plusplus Ruleset Baseline") to include the version/date and add a
"Changelog" subsection and a "Related files" or "Ruleset JSON" bullet that
points to the JSON ruleset filename so future changes can be tracked and
referenced.
In @.github/rulesets/main.json:
- Around line 15-26: The pull request ruleset currently has
"require_code_owner_review": false and "require_last_push_approval": false in
the parameters object; either enable these protections by setting
require_code_owner_review and require_last_push_approval to true to enforce
code-owner approvals and prevent self-approval after new pushes, or, if this is
intentional, add a clear comment or documentation entry in your baseline
explaining the rationale for leaving these flags false so reviewers understand
the decision.
- Around line 27-32: Remove the unsupported "code_quality" rule object from the
ruleset JSON (the entry whose "type" is "code_quality") and, if you need an
equivalent enforcement, replace it with a supported rule type such as
"code_scanning" or configure Code Quality via repository settings; ensure you
remove the entire object that contains "type": "code_quality" and its
"parameters" block and, if adding a replacement, use "type": "code_scanning"
with appropriate parameters instead.
In @.github/workflows/ci.yml:
- Around line 15-16: Update every CI workflow that still writes or reads the old
embedded models JSON path so it uses the new path
pkg/llmproxy/registry/models/models.json; specifically change any occurrences of
internal/registry/models/models.json in the workflows (release.yaml,
pr-test-build.yml, docker-image.yml) to pkg/llmproxy/registry/models/models.json
and ensure the step that extracts the file uses git show FETCH_HEAD:models.json
> pkg/llmproxy/registry/models/models.json to match the go:embed directive in
pkg/llmproxy/registry/model_updater.go.
In @.github/workflows/sast-full.yml:
- Around line 31-47: Add an explicit timeout-minutes setting to the scheduled
workflow jobs so they can't run indefinitely: update the job definitions for
trivy-repo and full-secrets to include a timeout-minutes: <minutes> entry
(choose an appropriate value like 10 or 30) directly under each job name (e.g.,
the trivy-repo job and the full-secrets job) so the GitHub Actions runner will
cancel long-running scans; keep the rest of the steps unchanged.
In @.github/workflows/sast-quick.yml:
- Around line 66-75: The shell uses unquoted variable expansion for files and
passes "$files" into gofmt (commands/vars: git ls-files, files, gofmt -l,
unformatted) which breaks on filenames with spaces; change the flow to use
NUL-separated output and xargs to avoid word-splitting (e.g., produce file list
with git ls-files -z and feed it to gofmt via xargs -0) and update the
unformatted check to read xargs output instead of using the unquoted $files
variable.
- Around line 56-77: The go-quality and license-check GitHub Actions jobs are
missing explicit timeouts which can allow runaway runs; add a timeout-minutes
entry to each job definition (for example timeout-minutes: 15) under the job
keys go-quality and license-check so the workflow will cancel long-running jobs
automatically; update both job blocks to include the timeout-minutes field with
an appropriate value.
In @.github/workflows/security-guard.yml:
- Around line 7-10: The workflow job ggshield-scan lacks an explicit permissions
block; add a least-privilege GitHub Actions token permission (e.g., set
permissions: contents: read) at the workflow or job level so the checkout +
local secret scan only has read access to repository contents; update the
jobs.ggshield-scan definition to include the permissions entry rather than
relying on the default token scope.
- Around line 11-14: The workflow uses mutable references: update the actions
and pip install to fixed, immutable versions by replacing actions/checkout@v4
and actions/setup-python@v5 with their corresponding full commit SHA references
and pin the scanner by changing pip install ggshield to a specific version (for
example pip install ggshield==1.25.0); ensure you locate the entries containing
"actions/checkout@v4", "actions/setup-python@v5", and "pip install ggshield" and
replace them with the exact commit SHAs for the actions and a fixed version
specifier for ggshield.
- Around line 4-5: The workflow's reusable-call contract doesn't declare the
required secret, so callers won't be validated at wiring time; under the
existing workflow_call key, add a secrets section that declares
GITGUARDIAN_API_KEY as a required secret (e.g., under workflow_call -> secrets
-> GITGUARDIAN_API_KEY: { required: true }) so callers must supply the secret
when invoking the workflow; update any documentation or callers if needed to
match the new required secret name.
In @.semgrep-rules/secrets-detection.yml:
- Around line 19-21: The regex in the rule with id "hardcoded-password" uses a
backslash escape sequence for newline (currently written as \\n) that will be
misinterpreted in YAML; update the pattern-regex value so the backslash is
preserved in the actual regex (e.g., escape the backslash for YAML or quote the
scalar properly) — modify the pattern-regex entry to ensure the sequence for
newline is represented as a literal backslash + n in the resulting regex (fix
the pattern-regex line so the `[^"\'\\n]{8,}` part becomes `[^"\'\\\\n]{8,}` or
use a quoted/block scalar to avoid needing extra escaping).
In @.semgrep-rules/unsafe-patterns.yml:
- Around line 1-26: The Semgrep custom rules defined in
.semgrep-rules/unsafe-patterns.yml are not being loaded because the Semgrep scan
steps in the CI workflows do not pass the --config .semgrep-rules/ flag; update
the Semgrep invocation in both sast-quick.yml and sast-full.yml to include
--config .semgrep-rules/ so these rules (ids shell-command-with-shell,
sql-query-built-with-sprintf, direct-http-get-without-timeout) are applied, or
alternatively add a short note in 04_IMPLEMENTATION_STRATEGY.md indicating the
intentional omission if you want to keep CI gated only on upstream packs.
In `@docs/plans/KILO_GASTOWN_SPEC.md`:
- Around line 367-371: The listed documentation links in this section (SPEC.md,
FEATURE_CHANGES_PLUSPLUS.md, AGENTS.md, and ../AGENTS.md) are broken; update
docs/plans/KILO_GASTOWN_SPEC.md to either create and add the missing files or
change the link targets to existing files; specifically ensure the entries
referencing SPEC.md, FEATURE_CHANGES_PLUSPLUS.md, AGENTS.md (same directory) and
../AGENTS.md are replaced with correct filenames or relative paths (leave
../README.md as-is) and verify each link resolves by opening the target files or
adding placeholder documents with those exact names if intended.
---
Outside diff comments:
In `@docs/plans/KILO_GASTOWN_SPEC.md`:
- Around line 346-356: The file contains a duplicated "Kush Ecosystem" ASCII
tree (the code block starting with "kush/" and entries like "thegent/",
"agentapi++/", "cliproxy++/") — keep a single, complete diagram and remove the
duplicate: merge the additional entries (`4sgm/`, `civ/`, `parpour/`,
`pheno-sdk/`) into the primary diagram and delete the redundant block so there
is one canonical ecosystem diagram in the document.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e597682-b139-4ac9-baad-3d5defabba63
📒 Files selected for processing (20)
.github/RULESET_BASELINE.md.github/rulesets/main.json.github/workflows/ci.yml.github/workflows/sast-full.yml.github/workflows/sast-quick.yml.github/workflows/security-guard.yml.semgrep-rules/architecture-violations.yml.semgrep-rules/secrets-detection.yml.semgrep-rules/unsafe-patterns.yml.semgrep.yamlPLAN.mddocs/plans/KILO_GASTOWN_SPEC.mddocs/sessions/2026-04-01-pr-readiness-refresh/00_SESSION_OVERVIEW.mddocs/sessions/2026-04-01-pr-readiness-refresh/01_RESEARCH.mddocs/sessions/2026-04-01-pr-readiness-refresh/02_SPECIFICATIONS.mddocs/sessions/2026-04-01-pr-readiness-refresh/03_DAG_WBS.mddocs/sessions/2026-04-01-pr-readiness-refresh/04_IMPLEMENTATION_STRATEGY.mddocs/sessions/2026-04-01-pr-readiness-refresh/05_KNOWN_ISSUES.mddocs/sessions/2026-04-01-pr-readiness-refresh/06_TESTING_STRATEGY.mdpkg/llmproxy/executor/codex_executor.go
| Current required check manifests: | ||
|
|
||
| - `.github/required-checks.txt` | ||
| - `.github/release-required-checks.txt` | ||
|
|
||
| Those manifests should drive the next remote ruleset wave once the stable job names are re-verified | ||
| against live workflow output. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify manifest files exist and check their contents
echo "=== Checking for manifest files ==="
for manifest in ".github/required-checks.txt" ".github/release-required-checks.txt"; do
if [[ -f "$manifest" ]]; then
echo -e "\n✓ Found: $manifest"
echo "Contents:"
cat "$manifest"
else
echo -e "\n✗ Missing: $manifest"
fi
done
echo -e "\n=== Cross-checking documented workflows against manifests ==="
documented_workflows=("policy-gate" "pr-path-guard" "pr-test-build" "required-check-names-guard" "quality-gate" "security-guard" "codeql" "sast-quick" "sast-full")
for workflow in "${documented_workflows[@]}"; do
if grep -q "$workflow" .github/required-checks.txt 2>/dev/null || grep -q "$workflow" .github/release-required-checks.txt 2>/dev/null; then
echo "✓ $workflow found in manifests"
else
echo "✗ $workflow not found in manifests"
fi
doneRepository: KooshaPari/cliproxyapi-plusplus
Length of output: 1151
Manifest files exist but are incomplete—most documented workflows are missing.
Both .github/required-checks.txt and .github/release-required-checks.txt exist, but the manifest contents significantly diverge from the documented workflow names (lines 21-29). The manifests currently list only pr-test-build and pr-path-guard, while 7 documented workflows are absent: policy-gate, required-check-names-guard, quality-gate, security-guard, codeql, sast-quick, and sast-full. These manifests must be updated to align with the documented workflows before they can reliably drive the next ruleset wave.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/RULESET_BASELINE.md around lines 31 - 37, The manifests
`.github/required-checks.txt` and `.github/release-required-checks.txt` only
list `pr-test-build` and `pr-path-guard` but are missing the documented workflow
names; update both manifest files to include the full set of documented checks
(`policy-gate`, `required-check-names-guard`, `quality-gate`, `security-guard`,
`codeql`, `sast-quick`, `sast-full`, plus the existing `pr-test-build` and
`pr-path-guard`) so the manifests match the workflows listed in lines 21-29 and
can reliably drive the next ruleset wave. Ensure the names exactly match the
workflow job names used in the repository (case-sensitive) and save both
`.github/required-checks.txt` and `.github/release-required-checks.txt`.
| mkdir -p pkg/llmproxy/registry/models | ||
| git show FETCH_HEAD:models.json > pkg/llmproxy/registry/models/models.json |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all workflow files using the models.json path
echo "=== Checking all workflows for models.json path references ==="
rg -n "models\.json" .github/workflows/
echo ""
echo "=== Checking go:embed directives for expected path ==="
rg -n "go:embed.*models" --type=goRepository: KooshaPari/cliproxyapi-plusplus
Length of output: 851
Critical: Three other workflows still use the old models path and will fail.
This CI workflow correctly writes to pkg/llmproxy/registry/models/models.json to match the go:embed directive in pkg/llmproxy/registry/model_updater.go (line 27). However, three other workflow files still use the old path internal/registry/models/models.json:
.github/workflows/release.yaml(line 19).github/workflows/pr-test-build.yml(line 20).github/workflows/docker-image.yml(lines 19, 54)
These inconsistencies will cause build failures in release, PR test, and Docker image workflows because the embedded file won't exist at the expected path.
Required fixes for other workflows
Apply the same path update to all three files:
mkdir -p pkg/llmproxy/registry/models
-git show FETCH_HEAD:models.json > internal/registry/models/models.json
+git show FETCH_HEAD:models.json > pkg/llmproxy/registry/models/models.json🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 15 - 16, Update every CI workflow that
still writes or reads the old embedded models JSON path so it uses the new path
pkg/llmproxy/registry/models/models.json; specifically change any occurrences of
internal/registry/models/models.json in the workflows (release.yaml,
pr-test-build.yml, docker-image.yml) to pkg/llmproxy/registry/models/models.json
and ensure the step that extracts the file uses git show FETCH_HEAD:models.json
> pkg/llmproxy/registry/models/models.json to match the go:embed directive in
pkg/llmproxy/registry/model_updater.go.
| rules: | ||
| - id: shell-command-with-shell | ||
| patterns: | ||
| - pattern-either: | ||
| - pattern: exec.Command("sh", "-c", ...) | ||
| - pattern: exec.Command("bash", "-c", ...) | ||
| message: Avoid shell evaluation in exec.Command. Pass explicit argv slices instead. | ||
| languages: [go] | ||
| severity: CRITICAL | ||
|
|
||
| - id: sql-query-built-with-sprintf | ||
| patterns: | ||
| - pattern: fmt.Sprintf($QUERY, ...) | ||
| - metavariable-regex: | ||
| metavariable: $QUERY | ||
| regex: (?i).*(select|insert|update|delete|from|where).* | ||
| message: SQL assembled with fmt.Sprintf is injection-prone. Use parameter binding instead. | ||
| languages: [go] | ||
| severity: CRITICAL | ||
|
|
||
| - id: direct-http-get-without-timeout | ||
| patterns: | ||
| - pattern: http.Get(...) | ||
| message: http.Get uses the default client without an explicit timeout. Prefer a configured http.Client. | ||
| languages: [go] | ||
| severity: LOW |
There was a problem hiding this comment.
Custom rules not loaded by SAST workflows.
These Go-focused rules are well-designed, but neither sast-quick.yml nor sast-full.yml includes --config .semgrep-rules/ in the Semgrep scan command. The workflows only use the upstream packs (p/security-audit, p/owasp-top-ten, p/cwe-top-25).
To activate these custom rules, add the config flag to both workflow files:
semgrep scan \
--config p/security-audit \
--config p/owasp-top-ten \
--config p/cwe-top-25 \
+ --config .semgrep-rules/ \
--error \Alternatively, if the intent is to gate CI only on upstream packs initially (as mentioned in 04_IMPLEMENTATION_STRATEGY.md), consider documenting this explicitly so future maintainers know these rules exist but are intentionally not enforced yet.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.semgrep-rules/unsafe-patterns.yml around lines 1 - 26, The Semgrep custom
rules defined in .semgrep-rules/unsafe-patterns.yml are not being loaded because
the Semgrep scan steps in the CI workflows do not pass the --config
.semgrep-rules/ flag; update the Semgrep invocation in both sast-quick.yml and
sast-full.yml to include --config .semgrep-rules/ so these rules (ids
shell-command-with-shell, sql-query-built-with-sprintf,
direct-http-get-without-timeout) are applied, or alternatively add a short note
in 04_IMPLEMENTATION_STRATEGY.md indicating the intentional omission if you want
to keep CI gated only on upstream packs.
| run: echo "Security audit placeholder - no script available yet" | ||
| - uses: actions/setup-python@v5 | ||
| - name: Install ggshield | ||
| run: pip install ggshield |
There was a problem hiding this comment.
WARNING: Unpinned pip install ggshield — supply-chain risk
No version is specified, so every CI run pulls the latest release. A compromised upstream release would immediately affect all pipelines. Pin to a specific version (e.g., pip install ggshield==25.1.0).
| @@ -0,0 +1,28 @@ | |||
| rules: | |||
| - id: no-dot-imports | |||
| pattern: import . "$PKG" | |||
There was a problem hiding this comment.
WARNING: Literal $PKG won't match real Go imports
The pattern import . "$PKG" treats $PKG as a literal string, not a Semgrep metavariable. This rule will only match code that imports a package literally named $PKG. Use pattern-regex or a metavariable to match arbitrary package paths.
|
|
||
| - id: no-blank-imports-outside-entrypoints | ||
| patterns: | ||
| - pattern: import _ "$PKG" |
There was a problem hiding this comment.
WARNING: Same literal $PKG issue — won't match real blank imports
Same as no-dot-imports: import _ "$PKG" matches only the literal string $PKG.
| message: Hardcoded password detected. Use environment variables or secrets management. | ||
| languages: [generic] | ||
| severity: CRITICAL | ||
| paths: |
There was a problem hiding this comment.
SUGGESTION: Missing third_party/** exclusion for secret detection
The hardcoded-password rule excludes **/*_test.go, examples/**, and test/**, but not third_party/**. Vendored or third-party code may contain password-like strings in config examples or documentation, causing false positives.
KooshaPari
added
the
ci-billing-exception
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 39 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pkg/llmproxy/auth/claude/utls_transport.go:15
- The imports in this file don’t match the identifiers used below: the code references
config.SDKConfigandproxyutil.*, butconfig/proxyutilaren’t imported (and the config package is currently imported aspkgconfig). As written, this won’t compile. Please either import the config package asconfig(or update the signatures/usages topkgconfig.SDKConfig) and add the missingproxyutilimport used byproxyutil.BuildDialer/ModeInherit.
import (
"net/http"
"strings"
"sync"
pkgconfig "github.com/kooshapari/CLIProxyAPI/v7/pkg/llmproxy/config"
tls "github.com/refraction-networking/utls"
log "github.com/sirupsen/logrus"
"golang.org/x/net/http2"
"golang.org/x/net/proxy"
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mkdir -p pkg/llmproxy/registry/models | ||
| git show FETCH_HEAD:models.json > pkg/llmproxy/registry/models/models.json |
There was a problem hiding this comment.
This workflow now writes the models catalog to pkg/llmproxy/registry/models/models.json, but other workflows (e.g. docker-image, pr-test-build, release) still write to internal/registry/models/models.json. Since the Go code embeds pkg/llmproxy/registry/models/models.json, consider updating the other workflows as well (or writing to both locations) to avoid builds/releases using a stale or missing embedded catalog.
| mkdir -p pkg/llmproxy/registry/models | |
| git show FETCH_HEAD:models.json > pkg/llmproxy/registry/models/models.json | |
| mkdir -p pkg/llmproxy/registry/models internal/registry/models | |
| git show FETCH_HEAD:models.json | tee pkg/llmproxy/registry/models/models.json > internal/registry/models/models.json |
Co-authored-by: Codex <noreply@openai.com>
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| @@ -63,9 +63,9 @@ type cursorSession struct { | |||
| pending []pendingMcpExec | |||
There was a problem hiding this comment.
Suggestion: Resuming a stored session does not verify that the underlying H2 stream is still alive before switching output and sending tool results. If the stream already ended, this path returns a new response channel that will never receive data or close, causing the resumed request to hang. Check stream liveness (and request context cancellation) before injecting tool results. [race condition]
Severity Level: Critical 🚨
- ❌ Resumed Cursor MCP tool calls can hang indefinitely.
- ❌ Conductor goroutine blocks forever on never-closing channel.
- ⚠️ Affects any future wiring of CursorExecutor streaming.
- ⚠️ Degrades developer experience using Cursor MCP tooling.| pending []pendingMcpExec | |
| select { | |
| case <-session.stream.Done(): | |
| return nil, fmt.Errorf("cursor: cannot resume session: stream already closed") | |
| case <-ctx.Done(): | |
| return nil, ctx.Err() | |
| case session.toolResultCh <- parsed.ToolResults: | |
| } |
Steps of Reproduction ✅
1. A streaming request is executed through the auth conductor for some provider that is
wired to use `CursorExecutor.ExecuteStream` (see
`sdk/cliproxy/auth/conductor_execution.go:70-77` and the `ProviderExecutor.ExecuteStream`
interface in `sdk/cliproxy/auth/conductor.go:20-22`). The conductor calls the executor
implementation's `ExecuteStream`, which for Cursor lives in
`pkg/llmproxy/executor/cursor_executor.go:15-27` (`func (e *CursorExecutor)
ExecuteStream(...)`).
2. During this first streaming call, Cursor's server sends an MCP tool call message. In
`ExecuteStream`'s goroutine, `processH2SessionFrames` (defined at
`pkg/llmproxy/executor/cursor_executor.go:51-61`) decodes a `ServerMsgExecMcpArgs` and
invokes the `onMcpExec` callback at `pkg/llmproxy/executor/cursor_executor.go:70-107`.
That callback closes the current SSE output channel, creates a new `resumeOut` channel,
and stores a `cursorSession` in `e.sessions[sessionKey]` with a non‑nil `stream`,
`toolResultCh`, and `resumeOutCh`, plus a `switchOutput` closure.
3. Before the client sends back tool results (i.e. while the user/tool is working), the
underlying H2 stream to Cursor terminates (e.g. server timeout, RST/GOAWAY, or network
error). While `processH2SessionFrames` is in its inline wait loop for tool results (the
`waitLoop` inside the `ServerMsgExecMcpArgs` case at
`pkg/llmproxy/executor/cursor_executor.go:160-239`), one of the `select` branches `case
<-stream.Done(): return stream.Err()` or `case waitData, ok := <-stream.Data(): if !ok {
return stream.Err() }` fires. `processH2SessionFrames` returns a non‑nil error, causing
the surrounding goroutine in `ExecuteStream` (started at
`pkg/llmproxy/executor/cursor_executor.go:46-54`) to hit the error handling at
`pkg/llmproxy/executor/cursor_executor.go:39-60`, which logs, calls `sessionCancel()` and
`stream.Close()`, and exits. Critically, the previously stored `cursorSession` entry in
`e.sessions` is not removed in this path, and no goroutine remains reading from
`session.toolResultCh`.
4. Later, the client sends the MCP tool results in a follow‑up streaming request.
`CursorExecutor.ExecuteStream` parses the OpenAI request, detects `parsed.ToolResults` and
looks up the saved session in `e.sessions[sessionKey]` at
`pkg/llmproxy/executor/cursor_executor.go:65-12`. Because `session.stream` is non‑nil and
`session.authID == authID`, it calls `resumeWithToolResults` at
`pkg/llmproxy/executor/cursor_executor.go:35-67`. Inside `resumeWithToolResults`, the
current code (lines 58-63) unconditionally switches output
(`session.switchOutput(session.resumeOutCh)`) and then performs a blocking send
`session.toolResultCh <- parsed.ToolResults` without checking `session.stream.Done()` or
`ctx.Done()`. Since the original goroutine running `processH2SessionFrames` has already
exited and nothing is receiving from `toolResultCh`, this send blocks forever.
`ExecuteStream` has already returned a `*cliproxyexecutor.StreamResult{Chunks:
session.resumeOutCh}` to the conductor
(`sdk/cliproxy/auth/conductor_execution.go:250-75`), and the conductor's forwarding
goroutine at `sdk/cliproxy/auth/conductor_execution.go:41-71` ranges over
`streamResult.Chunks`. Because `session.resumeOutCh` never gets any values and is never
closed in this stale-session scenario, the HTTP streaming response to the caller hangs
indefinitely.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/llmproxy/executor/cursor_executor.go
**Line:** 63:63
**Comment:**
*Race Condition: Resuming a stored session does not verify that the underlying H2 stream is still alive before switching output and sending tool results. If the stream already ended, this path returns a new response channel that will never receive data or close, causing the resumed request to hang. Check stream liveness (and request context cancellation) before injecting tool results.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
|
||
| var gitLabModelAliases = map[string]string{ | ||
| "duo-chat-haiku-4-6": "duo-chat-haiku-4-5", | ||
| "duo-chat-haiku-4-6": "duo-chat-haiku-4-5", |
There was a problem hiding this comment.
Suggestion: This alias silently downgrades any explicit duo-chat-haiku-4-6 request to duo-chat-haiku-4-5, so callers asking for 4.6 never get the model they requested. Remove this forced remap so model resolution preserves the requested version and avoids incorrect model behavior. [logic error]
Severity Level: Major ⚠️
- ⚠️ OpenAI /v1/chat/completions hides requested GitLab Haiku 4.6.
- ⚠️ OpenAI /v1/responses also returns Haiku 4.5 instead.
- ⚠️ Usage metrics misattribute Haiku 4.6 calls to 4.5.| "duo-chat-haiku-4-6": "duo-chat-haiku-4-5", | |
| // keep explicit model selection; do not remap Haiku 4.6 to an older model |
Steps of Reproduction ✅
1. Register a GitLab auth and executor using the pattern in
`sdk/api/handlers/openai/gitlab_duo_handler_test.go:115-139`, where
`manager.RegisterExecutor(runtimeexecutor.NewGitLabExecutor(&internalconfig.Config{}))`
and `registry.GetGlobalRegistry().RegisterClient(registered.ID, registered.Provider,
runtimeexecutor.GitLabModelsFromAuth(registered))` load GitLab models from
`GitLabModelsFromAuth`.
2. Note that `GitLabModelsFromAuth` at
`pkg/llmproxy/executor/gitlab_executor.go:1324-1361` iterates `for alias, upstream :=
range gitLabModelAliases` (line 1353) and calls `addModel(alias, displayName, provider)`,
so the alias key `"duo-chat-haiku-4-6"` from `gitLabModelAliases` (defined at line 77-79)
is exposed to the global model registry and thus to `/v1/models` via
`OpenAIAPIHandler.OpenAIModels` in `sdk/api/handlers/openai/openai_handlers.go:52-90`.
3. Send a normal OpenAI-compatible request to `POST /v1/chat/completions` handled by
`OpenAIAPIHandler.ChatCompletions` at `sdk/api/handlers/openai/openai_handlers.go:93-147`
with body containing `"model": "duo-chat-haiku-4-6"`. The handler reads the `model` field
(line 116), then calls `handleNonStreamingResponse` or `handleStreamingResponse`, which in
turn call `ExecuteWithAuthManager` with that `modelName` (see `handleNonStreamingResponse`
at lines 519-525 and `handleStreamingResponse` at lines 60-62 in the same file).
4. Inside `GitLabExecutor`, the request reaches `Execute` / `ExecuteStream` in
`pkg/llmproxy/executor/gitlab_executor.go:88-170`, which compute `responseModel :=
gitLabResolvedModel(auth, req.Model)` at lines 111, 158 and similarly for gateway paths at
lines 279, 284, 385. `gitLabResolvedModel` is defined at lines 1193-1203 and, for any
non-empty `requested` model not equal to `"gitlab-duo"`, looks up
`gitLabModelAliases[strings.ToLower(requested)]` (line 1196). Because `gitLabModelAliases`
maps `"duo-chat-haiku-4-6"` to `"duo-chat-haiku-4-5"` at line 78, the `responseModel`
becomes `"duo-chat-haiku-4-5"`, and `buildGitLabOpenAIResponse` /
`buildGitLabOpenAIStream` (lines 121-210) serialize the response JSON with `"model":
"duo-chat-haiku-4-5"`. The client therefore always sees model `duo-chat-haiku-4-5` in
responses even when explicitly requesting `duo-chat-haiku-4-6`, and there is no way to
obtain a distinct 4.6 model via this alias.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/llmproxy/executor/gitlab_executor.go
**Line:** 78:78
**Comment:**
*Logic Error: This alias silently downgrades any explicit `duo-chat-haiku-4-6` request to `duo-chat-haiku-4-5`, so callers asking for 4.6 never get the model they requested. Remove this forced remap so model resolution preserves the requested version and avoids incorrect model behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| // Unconditional rule - no models specified | ||
| Models: []config.PayloadModelRule{}, | ||
| Params: map[string]any{"maxTokens": 1000}, | ||
| Models: []config.PayloadModelRule{}, |
There was a problem hiding this comment.
Suggestion: Using an empty Models slice does not create an unconditional match here—payloadModelRulesMatch returns false when no model rules are provided—so this default rule never runs and the test assertion for inherited maxTokens will fail. Use a wildcard model rule so the default rule actually applies to the test model. [logic error]
Severity Level: Major ⚠️
- ❌ TestApplyPayloadConfigWithRoot_UnconditionalRules consistently fails in executor package.
- ⚠️ `go test ./...` in this repo will fail CI.
- ⚠️ Unconditional default-rule behavior remains untested for configured models.| Models: []config.PayloadModelRule{}, | |
| Models: []config.PayloadModelRule{{Name: "*", Protocol: ""}}, |
Steps of Reproduction ✅
1. Run `go test ./pkg/llmproxy/executor -run
TestApplyPayloadConfigWithRoot_UnconditionalRules` to execute the test defined at
`pkg/llmproxy/executor/payload_helpers_test.go:159-197`.
2. Observe the test config building `cfg.Payload.Default` with a single rule whose
`Models` field is an empty slice at `payload_helpers_test.go:165-169`, intended (per
comment) to be an unconditional rule.
3. Inside `applyPayloadConfigWithRoot` at
`pkg/llmproxy/executor/payload_helpers.go:19-63`, the default rule is evaluated via
`payloadModelRulesMatch(rule.Models, protocol, candidates)` at line 42, passing an empty
`[]config.PayloadModelRule` slice and non-empty model candidates derived from
`"gemini-2.0-flash"`.
4. `payloadModelRulesMatch` at `payload_helpers.go:154-185` immediately returns `false`
when `len(rules) == 0` (line 155), so the supposed unconditional default rule never runs;
as a result, `maxTokens` remains `500` from `payload` instead of the test's expected
`200`, causing the assertion at `payload_helpers_test.go:192-193` to fail and the test
suite to break.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/llmproxy/executor/payload_helpers_test.go
**Line:** 167:167
**Comment:**
*Logic Error: Using an empty `Models` slice does not create an unconditional match here—`payloadModelRulesMatch` returns false when no model rules are provided—so this default rule never runs and the test assertion for inherited `maxTokens` will fail. Use a wildcard model rule so the default rule actually applies to the test model.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI Incremental review completed. |
Load repo-local Semgrep rules in CI, pin the security guard workflow, tighten the checked-in ruleset baseline, and fix Kilo Gastown doc links to real repo artifacts. Co-authored-by: Codex <noreply@openai.com>
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 41 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sast-quick.yml|semgrep | ||
| sast-quick.yml|secrets | ||
| sast-quick.yml|go-quality | ||
| sast-quick.yml|license-check |
There was a problem hiding this comment.
The required-check manifest entries for the SAST quick workflow use job IDs (semgrep/secrets/go-quality/license-check), but the workflow defines explicit job name: values (e.g., "Semgrep Scan", "Secret Scanning"). The required-check-names-guard script greps for exact name: matches, so these entries will be reported as missing and the required check contexts may not match what GitHub emits. Update the manifest to the workflow job names or remove/align the job name: fields so they match.
| sast-quick.yml|semgrep | |
| sast-quick.yml|secrets | |
| sast-quick.yml|go-quality | |
| sast-quick.yml|license-check | |
| sast-quick.yml|Semgrep Scan | |
| sast-quick.yml|Secret Scanning | |
| sast-quick.yml|Go Quality | |
| sast-quick.yml|License Check |
| sast-quick.yml|semgrep | ||
| sast-quick.yml|secrets | ||
| sast-quick.yml|go-quality | ||
| sast-quick.yml|license-check |
There was a problem hiding this comment.
Same issue as .github/required-checks.txt: the SAST quick entries reference job IDs, but the workflow uses job name: values ("Semgrep Scan", "Secret Scanning", etc.). This will cause required-check-names-guard to fail and can prevent required checks from matching actual check run names. Align the manifest values with the workflow job names (or align workflow job names with the IDs).
| sast-quick.yml|semgrep | |
| sast-quick.yml|secrets | |
| sast-quick.yml|go-quality | |
| sast-quick.yml|license-check | |
| sast-quick.yml|Semgrep Scan | |
| sast-quick.yml|Secret Scanning | |
| sast-quick.yml|Go Quality | |
| sast-quick.yml|License Check |
| secrets: | ||
| GITGUARDIAN_API_KEY: | ||
| required: true | ||
| workflow_dispatch: |
There was a problem hiding this comment.
This workflow is listed as a required check (security-guard.yml|ggshield-scan), but it only triggers via workflow_dispatch/workflow_call and is not invoked by any other workflow. As a result, PRs/branch updates won't automatically produce the required status check, potentially blocking merges unless run manually. Consider adding pull_request/push triggers (or wiring a caller workflow) or removing it from the required-check manifest if it should remain manual-only.
| workflow_dispatch: | |
| workflow_dispatch: | |
| push: | |
| pull_request: |
| semgrep scan \ | ||
| --config .semgrep-rules/ \ | ||
| --config p/security-audit \ | ||
| --config p/owasp-top-ten \ | ||
| --config p/cwe-top-25 \ | ||
| --error \ | ||
| --sarif \ | ||
| --output semgrep.sarif \ | ||
| "${changed_files[@]}" |
There was a problem hiding this comment.
Semgrep is invoked with --error, which will fail the job on findings. Given the repo includes INFO/LOW severity custom rules and the PR notes the custom ruleset may be noisy, this is likely to gate merges unexpectedly. If the intent is to upload SARIF without blocking, drop --error (or restrict failures to a severity threshold / only upstream packs until the rules are tuned).
| --config p/security-audit \ | ||
| --config p/owasp-top-ten \ | ||
| --config p/cwe-top-25 \ | ||
| --error \ |
There was a problem hiding this comment.
Same as in sast-quick: running Semgrep with --error will fail the scheduled full scan on any finding, including low-signal rules. If this workflow is intended to be informational (nightly signal) rather than gating, consider removing --error or filtering by severity / excluding the in-repo custom rules until noise is reduced.
| --error \ |
| - id: hardcoded-api-key | ||
| pattern-regex: '(?i)(api[_-]?key|token|secret)\s*(?:[:=])\s*["''][^"''\\n]{8,}["'']' | ||
| message: Hardcoded API key detected. Use environment variables or secrets management. | ||
| languages: [generic] | ||
| severity: HIGH |
There was a problem hiding this comment.
The character class uses \\n which (after YAML parsing) typically excludes a literal backslash and the letter n, not an actual newline. If the goal is to avoid matching across lines, use \n (newline escape) in the regex (and consider \r as well). As written, this can increase false positives by allowing matches that span newlines.
| - id: hardcoded-password | ||
| pattern-regex: '(?i)(password|passwd|pwd)\s*(?:[:=])\s*["''][^"''\\n]{8,}["'']' | ||
| message: Hardcoded password detected. Use environment variables or secrets management. | ||
| languages: [generic] | ||
| severity: CRITICAL |
There was a problem hiding this comment.
Same newline-escape issue as the API key rule: \\n inside the negated character class is unlikely to exclude actual newline characters. Using \n (and possibly \r) would better prevent cross-line matches and reduce false positives.
- Add CODEOWNERS with @KooshaPari as sponsor - Add AGENTS.md extending phenotype-governance - Add issue/PR templates for task discovery - Add linter configs where applicable - Add CI workflow for automated quality gates
|
CodeAnt AI is running Incremental review |
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
codeant-ai
bot
added
size:XL
| @@ -0,0 +1,8 @@ | |||
| { | |||
There was a problem hiding this comment.
WARNING: Test artifact committed — placeholder credential fixture
This file contains stub data ("project_id": "p", empty strings) and matches the pattern of files generated by TestVertexCredentialStorage_SaveTokenToFileRejectsTraversalPath (which calls SaveTokenToFile("../vertex.json")). It should not be tracked in version control. Add pkg/llmproxy/auth/vertex.json and pkg/llmproxy/auth/vertex/path to .gitignore and remove them from tracking with git rm --cached.
| @@ -0,0 +1,6 @@ | |||
| { | |||
There was a problem hiding this comment.
WARNING: Test artifact committed — extensionless file with stub JSON
This file has no extension and contains placeholder data matching SaveTokenToFile("path") test output. It appears to be a leftover from test execution. Files without extensions in pkg/ directories are confusing and should not be committed. Remove from tracking and add to .gitignore.
| "time" | ||
|
|
||
| cursorauth "github.com/router-for-me/CLIProxyAPI/v6/internal/auth/cursor" | ||
| ) |
There was a problem hiding this comment.
SUGGESTION: Fragile dependency on external module's internal package
This wrapper imports github.com/router-for-me/CLIProxyAPI/v6/internal/auth/cursor. While Go allows this for versioned module dependencies, internal packages are not part of the public API contract and may change without notice in minor/patch releases. Consider vendoring the code or negotiating a public API surface from the upstream module.
| # Default owner (sponsor) | ||
| * @kooshapari |
There was a problem hiding this comment.
Suggestion: Using a global * ownership with a single user creates a merge bottleneck under your enforced ruleset (require_code_owner_review + require_last_push_approval), because every PR in the repo depends on one reviewer and can become unmergeable when that reviewer is unavailable or is the last pusher. Scope ownership to the security/governance paths introduced in this PR instead of all files. [logic error]
Severity Level: Critical 🚨
- ❌ PRs authored by @kooshapari can become permanently unmergeable.
- ❌ All changes depend on availability of single code owner.
- ⚠️ Slows security tooling rollout when sponsor must approve everything.| # Default owner (sponsor) | |
| * @kooshapari | |
| # Security and governance owners | |
| .github/ @kooshapari | |
| .semgrep* @kooshapari | |
| .semgrep-rules/ @kooshapari | |
| docs/plans/KILO_GASTOWN_SPEC.md @kooshapari |
Steps of Reproduction ✅
1. Observe the branch ruleset at `.github/rulesets/main.json:16-25`, which defines a
`"pull_request"` rule with `"require_code_owner_review": true` and
`"require_last_push_approval": true` for the default branch.
2. Observe the code owners configuration at `.github/CODEOWNERS:1-2`, where line 2 assigns
all paths via `*` to the single owner `@kooshapari`, making every file in the repository
require that owner's review.
3. Have user `@kooshapari` push a commit that changes any file in the repo to a branch and
open a pull request targeting the default branch; GitHub now applies the rules from
`main.json` and the global ownership from `CODEOWNERS`.
4. Attempt to merge the PR: GitHub requires a code-owner review that is not from the last
pusher, but the only code owner is `@kooshapari` who is also the last pusher, so no valid
reviewer exists and the PR becomes permanently unmergeable until ownership or rules are
changed.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** .github/CODEOWNERS
**Line:** 1:2
**Comment:**
*Logic Error: Using a global `*` ownership with a single user creates a merge bottleneck under your enforced ruleset (`require_code_owner_review` + `require_last_push_approval`), because every PR in the repo depends on one reviewer and can become unmergeable when that reviewer is unavailable or is the last pusher. Scope ownership to the security/governance paths introduced in this PR instead of all files.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| "github.com/kooshapari/CLIProxyAPI/v7/pkg/llmproxy/tui" | ||
| "github.com/kooshapari/CLIProxyAPI/v7/pkg/llmproxy/usage" | ||
| "github.com/kooshapari/CLIProxyAPI/v7/pkg/llmproxy/util" | ||
| sdkAuth "github.com/kooshapari/CLIProxyAPI/v7/sdk/auth" |
There was a problem hiding this comment.
Suggestion: The registry.StartModelsUpdater(...) call later in this file depends on the registry package, but that package is not imported in the updated import block. This causes an undefined identifier failure when building. Add the missing registry import alongside the other llmproxy imports. [possible bug]
Severity Level: Critical 🚨
- ❌ Server binary fails to build due to undefined registry symbol.
- ❌ Any CI job running go build/tests on cmd/server fails.| sdkAuth "github.com/kooshapari/CLIProxyAPI/v7/sdk/auth" | |
| "github.com/kooshapari/CLIProxyAPI/v7/pkg/llmproxy/registry" |
Steps of Reproduction ✅
1. From the repository root `/workspace/cliproxyapi-plusplus`, run `go build ./cmd/server`
to build the CLI Proxy API server binary (this compiles `cmd/server/main.go` as the `main`
package).
2. The Go compiler parses the imports in `cmd/server/main.go:6-35` (see `functions.Read`
output), which include `util` at line 32 but do not include any import for the `registry`
package.
3. Later in the same file, in the TUI standalone server branch at
`cmd/server/main.go:615-617` (see `functions.Grep` and `functions.Read`), the code calls
`registry.StartModelsUpdater(context.Background())` inside the `if !localModel { ... }`
block.
4. Because `registry` is not imported, the build fails with an error similar to
`cmd/server/main.go:616:2: undefined: registry`, even though `StartModelsUpdater` is
correctly defined in `pkg/llmproxy/registry/model_updater.go:77` (verified via
`functions.Grep`), blocking any successful build.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** cmd/server/main.go
**Line:** 33:33
**Comment:**
*Possible Bug: The `registry.StartModelsUpdater(...)` call later in this file depends on the `registry` package, but that package is not imported in the updated import block. This causes an undefined identifier failure when building. Add the missing `registry` import alongside the other llmproxy imports.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| "time" | ||
|
|
||
| cursorauth "github.com/router-for-me/CLIProxyAPI/v6/internal/auth/cursor" | ||
| ) | ||
|
|
||
| type AuthParams = cursorauth.AuthParams | ||
| type TokenPair = cursorauth.TokenPair | ||
|
|
||
| const ( | ||
| CursorLoginURL = cursorauth.CursorLoginURL | ||
| CursorPollURL = cursorauth.CursorPollURL | ||
| CursorRefreshURL = cursorauth.CursorRefreshURL | ||
| ) | ||
|
|
||
| func GeneratePKCE() (string, string, error) { return cursorauth.GeneratePKCE() } | ||
| func GenerateAuthParams() (*AuthParams, error) { return cursorauth.GenerateAuthParams() } | ||
| func PollForAuth(ctx time.Time, uuid, verifier string) (*TokenPair, error) { | ||
| return cursorauth.PollForAuth(ctx, uuid, verifier) | ||
| } | ||
| func RefreshToken(ctx time.Time, refreshToken string) (*TokenPair, error) { |
There was a problem hiding this comment.
Suggestion: The wrapper uses time.Time for request-scoped parameters where the underlying auth API expects a cancellable context. This breaks the contract with callers that pass contexts and prevents cancellation/deadline propagation during network polling/refresh calls. Update both wrappers to accept context.Context and import context. [type error]
Severity Level: Critical 🚨
- ❌ Package `sdk/auth` fails to compile due to type mismatch.
- ❌ Cursor PKCE login flow cannot be built or executed.
- ⚠️ Context cancellation for Cursor polling not propagated by wrapper.| "time" | |
| cursorauth "github.com/router-for-me/CLIProxyAPI/v6/internal/auth/cursor" | |
| ) | |
| type AuthParams = cursorauth.AuthParams | |
| type TokenPair = cursorauth.TokenPair | |
| const ( | |
| CursorLoginURL = cursorauth.CursorLoginURL | |
| CursorPollURL = cursorauth.CursorPollURL | |
| CursorRefreshURL = cursorauth.CursorRefreshURL | |
| ) | |
| func GeneratePKCE() (string, string, error) { return cursorauth.GeneratePKCE() } | |
| func GenerateAuthParams() (*AuthParams, error) { return cursorauth.GenerateAuthParams() } | |
| func PollForAuth(ctx time.Time, uuid, verifier string) (*TokenPair, error) { | |
| return cursorauth.PollForAuth(ctx, uuid, verifier) | |
| } | |
| func RefreshToken(ctx time.Time, refreshToken string) (*TokenPair, error) { | |
| "context" | |
| "time" | |
| cursorauth "github.com/router-for-me/CLIProxyAPI/v6/internal/auth/cursor" | |
| ) | |
| type AuthParams = cursorauth.AuthParams | |
| type TokenPair = cursorauth.TokenPair | |
| const ( | |
| CursorLoginURL = cursorauth.CursorLoginURL | |
| CursorPollURL = cursorauth.CursorPollURL | |
| CursorRefreshURL = cursorauth.CursorRefreshURL | |
| ) | |
| func GeneratePKCE() (string, string, error) { return cursorauth.GeneratePKCE() } | |
| func GenerateAuthParams() (*AuthParams, error) { return cursorauth.GenerateAuthParams() } | |
| func PollForAuth(ctx context.Context, uuid, verifier string) (*TokenPair, error) { | |
| return cursorauth.PollForAuth(ctx, uuid, verifier) | |
| } | |
| func RefreshToken(ctx context.Context, refreshToken string) (*TokenPair, error) { |
Steps of Reproduction ✅
1. From the repo root `/workspace/cliproxyapi-plusplus`, run `go test ./...` or `go build
./...` to compile all packages; this compiles `sdk/auth/cursor.go` and
`pkg/llmproxy/auth/cursor/cursor.go`.
2. During compilation of `sdk/auth/cursor.go:35-36`, the `Login` method of
`CursorAuthenticator` is type-checked; it accepts a `ctx context.Context` and at
`sdk/auth/cursor.go:65` calls `cursor.PollForAuth(ctx, authParams.UUID,
authParams.Verifier)`.
3. The imported `cursor` package is
`github.com/kooshapari/CLIProxyAPI/v7/pkg/llmproxy/auth/cursor`, implemented in
`pkg/llmproxy/auth/cursor/cursor.go:1-28`, where `PollForAuth` is declared as `func
PollForAuth(ctx time.Time, uuid, verifier string) (*TokenPair, error)` at line 20, taking
a `time.Time` instead of `context.Context`.
4. The Go compiler fails with a type error like `cannot use ctx (variable of type
context.Context) as type time.Time in argument to cursor.PollForAuth` (or equivalent),
because `context.Context` is not assignable to `time.Time`; as a result, builds and tests
cannot complete, and no code using Cursor authentication can be executed.
5. Independently, the underlying implementation `internal/auth/cursor/oauth.go:13-15`
defines `func PollForAuth(ctx context.Context, uuid, verifier string) (*TokenPair, error)`
and `func RefreshToken(ctx context.Context, refreshToken string) (*TokenPair, error)`
(lines 68-69), confirming that these operations are designed to be cancellable via
`context.Context`; the wrapper's `time.Time` parameter both causes the compile-time
failure above and, once fixed to compile, must use `context.Context` to propagate
cancellation correctly.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/llmproxy/auth/cursor/cursor.go
**Line:** 4:23
**Comment:**
*Type Error: The wrapper uses `time.Time` for request-scoped parameters where the underlying auth API expects a cancellable context. This breaks the contract with callers that pass contexts and prevents cancellation/deadline propagation during network polling/refresh calls. Update both wrappers to accept `context.Context` and import `context`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
|
||
| // Poll for the auth result | ||
| tokens, err := cursorauth.PollForAuth(ctx, authParams.UUID, authParams.Verifier) | ||
| tokens, err := cursor.PollForAuth(ctx, authParams.UUID, authParams.Verifier) |
There was a problem hiding this comment.
Suggestion: ctx is passed directly into polling without a nil guard, but the poller dereferences ctx.Done(). If a caller provides a nil context, this will panic at runtime. Normalize nil to context.Background() before calling the poll function. [null pointer]
Severity Level: Major ⚠️
- ❌ Library callers using Login(nil, cfg, opts) will panic.
- ⚠️ Cursor authenticator diverges from other nil-tolerant Login implementations.| tokens, err := cursor.PollForAuth(ctx, authParams.UUID, authParams.Verifier) | |
| pollCtx := ctx | |
| if pollCtx == nil { | |
| pollCtx = context.Background() | |
| } | |
| tokens, err := cursor.PollForAuth(pollCtx, authParams.UUID, authParams.Verifier) |
Steps of Reproduction ✅
1. In a test or caller code, construct a Cursor authenticator using
`auth.NewCursorAuthenticator()` from `sdk/auth/cursor.go:18-21`.
2. Call `Login` with a nil context, e.g. `auth.NewCursorAuthenticator().Login(nil,
&config.Config{}, &auth.LoginOptions{})`, which enters `CursorAuthenticator.Login` at
`sdk/auth/cursor.go:35-41`.
3. Inside `Login`, the nil `ctx` is passed directly to `cursor.PollForAuth(ctx,
authParams.UUID, authParams.Verifier)` at `sdk/auth/cursor.go:64-65` without
normalization.
4. `cursor.PollForAuth` ultimately calls `internal/auth/cursor/oauth.go:83-135`, where
`select { case <-ctx.Done(): ... }` at line 91 and `http.NewRequestWithContext(ctx, ...)`
at line 97 dereference the nil context, causing a panic and crashing the login flow.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** sdk/auth/cursor.go
**Line:** 65:65
**Comment:**
*Null Pointer: `ctx` is passed directly into polling without a nil guard, but the poller dereferences `ctx.Done()`. If a caller provides a nil context, this will panic at runtime. Normalize nil to `context.Background()` before calling the poll function.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI Incremental review completed. |
User description
Summary
Testing
Note
Medium Risk
Primarily CI and documentation changes, but new security/lint workflows and custom Semgrep rules may introduce noisy findings or gating behavior that can affect developer throughput and PR mergeability.
Overview
Adds two new GitHub Actions SAST workflows: a quick PR/main pipeline running Semgrep (SARIF upload), Trufflehog secret scanning, Rust
clippy, and license checks, plus a nightly/manual full scan that also runs CodeQL (python/cpp/javascript) and Trivy filesystem scanning with SARIF uploads.Introduces a repository Semgrep configuration (excludes tests/build outputs) and three custom rule packs for architecture violations, unsafe Rust patterns, and generic secret detection under
.semgrep-rules/.Expands and restructures
docs/plans/KILO_GASTOWN_SPEC.mdinto a more complete methodology spec (convoys/beads/roles/tooling/quality gates) and bumps the document version to 1.1.Written by Cursor Bugbot for commit f44d1fd. This will update automatically on new commits. Configure here.
CI Exception
security/snyk (kooshapari)is currently failing due private-test quota exhaustion, not a code regression.ci-billing-exceptionlabel while that external billing/quota limit is in effect.Summary by CodeRabbit
Release Notes
New Features
Chores
CodeAnt-AI Description
Add SAST checks, repo governance rules, and a fuller Kilo Gastown spec
What Changed
Impact
✅ Fewer insecure changes reaching main✅ Earlier detection of secret leaks and unsafe code✅ Clearer merge requirements for contributors💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.