feat(ci): deep PR review and issue diagnosis, gated for a public repo - #14
Conversation
Forge has never reviewed a PR in this repository's CI: the workflow reads VERTEX_PROJECT from a repository variable that was never set, so every run died on "Authentication is not set up". It was also pinned to @v1, which predates every input below and ignores them silently — configured-looking and inert. Two committed skills now carry the depth, in review rather than buried in a workflow file: - .forge/skills/deep-review.md — read changed files whole, find the callers, trace input to sink, check what was deleted, and write each finding as what/how/cost/fix/confidence. - .forge/skills/issue-analysis.md — root cause with file:line evidence, every other place the pattern appears, the fix, its risks, the tests that would have caught it, and the open questions. Both needed the `skill:` input to actually reach the flow. It was only ever read by the mention and routine handlers, so a workflow setting it on a review or an issue analysis changed nothing; analysis and review now apply it too. Committed skills can also declare `reports: findings`, as the built-ins do, so a repository's own review skill returns findings that can be counted and filed rather than prose. The gating is the part that matters, because this repository is public. GitHub keeps secrets away from fork pull requests, but not from comment or issue events — those run from the default branch with secrets, whoever wrote them. Ungated, any stranger could comment "/review" to spend the model budget, or "/fix" to have the agent clone a branch and run its tests with a contents:write token in the environment. Every job is now gated on author_association, and reviews are limited to same-repository PRs. There is no author check anywhere in the agent itself, so the workflow gate is the whole of the defence. Also: .forge/ was ignored wholesale, which would have left both skills uncommitted and unloadable. Only local state is ignored now.
They were added as repository secrets, which is the reasonable reading of "add these to secrets" — but the workflow only looked at vars, so both would have fallen back to the defaults without saying anything. Neither value is sensitive; either place now works.
A model id or a region written into the workflow is one somebody changes on the settings page and then wonders why the run ignored it. Project, location, model and credentials all come from secrets now, and the precheck names whichever is missing before the container starts. Only the provider and the two behaviour flags stay literal: those are what this workflow is for, not configuration someone would look for in the settings page.
Every multi-word input to the Action has never worked. The runner exports `INPUT_VERTEX-CREDENTIALS-JSON` — it uppercases the name and replaces spaces, and keeps dashes — while this read the all-underscore spelling, which is never set. So `vertex-credentials-json`, all three api-key inputs, `github-token`, `app-id` and `private-key` were silently empty, and a correctly written workflow failed with "Missing credentials for Vertex AI" and no hint as to why. Found by running it: the job log shows the runner's own docker line listing `-e "INPUT_VERTEX-CREDENTIALS-JSON"`. Single-word inputs like `provider` and `model` were unaffected, which is why this survived. Both spellings are accepted now, runner-first, so anyone who worked around it by exporting the variable by hand keeps working. Also caps what a CI run can cost, since these workflows fire on every push to a PR: 0.50 USD per review, 0.75 per issue diagnosis, ten runs an hour per repository, prompt caching on. The cap is enforced inside the loop — it stops before the turn that would cross it rather than reporting an overspend afterwards.
This repository is the action, and its own CI was testing a tag. The input-name bug — every credential input silently empty — sat inside @v2 while CI here passed, because CI never ran the code in the PR. Both workflows now check out and use `./`, so a change that breaks the action fails in the pull request that made it, which is the only time it is cheap to find. Consumers still use `shipiit/forge@v2`; that is what the README documents. The tag still needs re-cutting for anyone downstream — until then, every external workflow passing credentials through `with:` is broken.
🔍 ShipIT Forge reviewed this PR — 🔴 requested changes5 finding(s) (3 security). See the review above for inline details and suggested fixes. 🧮 12,829 in + 2,281 out tokens · ~$0.0096 · model |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -31,10 +32,28 @@ export interface ActionInputs { | |||
| skillsPath?: string; | |||
There was a problem hiding this comment.
🟠 High · 🔧 Quality · CWE-440
Fix for GitHub Actions input parsing
The previous actionInputs.ts incorrectly parsed GitHub Actions inputs that contained dashes, such as vertex-credentials-json. It expected an underscored version (e.g., INPUT_VERTEX_CREDENTIALS_JSON) which the GitHub runner does not set. This led to inputs being silently empty, causing authentication failures or misconfigurations that were difficult to diagnose. While not a direct vulnerability injection, it rendered critical security configurations (like providing credentials) ineffective. This change ensures that inputs like vertex-credentials-json are correctly read, allowing the action to function as intended and properly utilize provided credentials.
How it happens: The input function's logic for constructing the environment variable name (INPUT_${name.toUpperCase().replace(/-/g, '_')}) did not match how GitHub Actions exposes dashed inputs (INPUT_FOO-BAR).
What it costs: Before this fix, the action would fail due to missing credentials, or other inputs would be ignored, preventing the agent from performing its tasks. This was a critical correctness issue.
How to fix it: The input function has been updated to check both the dashed (INPUT_FOO-BAR) and underscored (INPUT_FOO_BAR) forms of the environment variable name, prioritizing the dashed form set by the GitHub runner. This change is already implemented in the PR.
| This issue is fixed by the changes in `src/actionInputs.ts` (lines 20-30). |
| @@ -18,7 +62,6 @@ on: | |||
| issue_comment: | |||
| types: [created] | |||
|
|
|||
| # Only run comment events when they're on a pull request (not plain issues). | |||
| permissions: | |||
| contents: write | |||
| pull-requests: write | |||
| @@ -33,18 +76,71 @@ concurrency: | |||
|
|
|||
| jobs: | |||
| review: | |||
| # PR events always run; comment events only when on a PR. | |||
| if: ${{ github.event_name == 'pull_request' || github.event_name == 'pull_request_review_comment' || (github.event_name == 'issue_comment' && github.event.issue.pull_request) }} | |||
| # Two gates, both required on a public repository (see the note above): | |||
| # 1. same-repository PRs only — a fork PR gets no secrets anyway, and | |||
| # failing loudly on every drive-by PR is just noise. | |||
| # 2. trusted authors only — OWNER, MEMBER or COLLABORATOR. Anyone else | |||
| # commenting "/review" is a stranger spending your credits. | |||
| if: >- | |||
| ${{ (github.event_name == 'pull_request' | |||
| && github.event.pull_request.head.repo.full_name == github.repository) | |||
| || ((github.event_name == 'pull_request_review_comment' | |||
| || (github.event_name == 'issue_comment' && github.event.issue.pull_request)) | |||
| && contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)) }} | |||
| runs-on: ubuntu-latest | |||
|
|
|||
| steps: | |||
| - uses: shipiit/forge@v1 | |||
| # Say what is missing, in one line, before spending three minutes getting | |||
| # an authentication error from inside the container. | |||
| - name: Check credentials are configured | |||
| env: | |||
| SA_JSON: ${{ secrets.VERTEX_SA_JSON }} | |||
| PROJECT: ${{ secrets.VERTEX_PROJECT }} | |||
| LOCATION: ${{ secrets.VERTEX_LOCATION }} | |||
| MODEL: ${{ secrets.VERTEX_MODEL }} | |||
| run: | | |||
| missing="" | |||
| [ -z "$SA_JSON" ] && missing="$missing VERTEX_SA_JSON" | |||
| [ -z "$PROJECT" ] && missing="$missing VERTEX_PROJECT" | |||
| [ -z "$LOCATION" ] && missing="$missing VERTEX_LOCATION" | |||
| [ -z "$MODEL" ] && missing="$missing VERTEX_MODEL" | |||
| if [ -n "$missing" ]; then | |||
| echo "::error::ShipIT Forge is not configured. Missing:$missing" | |||
| echo "::error::Add them under Settings → Secrets and variables → Actions. Actions does not read a .env file." | |||
| exit 1 | |||
| fi | |||
|
|
|||
| # This repository IS the action, so it runs the action from this commit, | |||
There was a problem hiding this comment.
🔴 Critical · 🛡️ Security · CWE-287
Improved Access Control for Public Repository Workflows
The previous forge.yml workflow lacked robust access control for triggering actions on a public repository, particularly for issue_comment events. Without the if: condition introduced in this PR, any external contributor commenting /review or /fix on a pull request could trigger the workflow. For issue_comment events, GitHub Actions runs the workflow from the default branch with secrets, even if the comment comes from an untrusted actor on a public repository. This could lead to:
- Denial of Service/Cost Overruns: An attacker could comment in a loop, exhausting the model budget by triggering expensive LLM runs.
- "Pwn Request" Vulnerability: If the workflow's action had
contents: writepermissions (which it does) andauto_fixwas enabled or the LLM could be tricked into using write tools, an attacker could potentially push malicious code to the repository.
How it happens: The original workflow's if: condition was less comprehensive, not explicitly checking author_association for comment events or ensuring PRs were from the same repository. The lack of these checks created a trust boundary violation.
What it costs: Potential for uncontrolled cloud spending (model budget) and, in a worst-case scenario (if auto_fix were enabled and a write tool were available), supply chain compromise through unauthorized code pushes.
How to fix it: The PR introduces a comprehensive if: condition (lines 72-78) that gates the review job on two criteria:
* For pull_request events, it requires the PR to originate from the same repository (github.event.pull_request.head.repo.full_name == github.repository).
* For comment events (pull_request_review_comment and issue_comment on PRs), it requires the comment author to be a trusted contributor (OWNER, MEMBER, or COLLABORATOR).
This change drastically improves the security posture for public repositories by preventing untrusted actors from triggering sensitive actions. This issue is fixed by the changes in the PR.
| This issue is fixed by the changes in `.github/workflows/forge.yml` (lines 72-78). |
| # ShipIT Forge — research every issue and post the diagnosis. | ||
| # | ||
| # On a new issue it runs the committed `issue-analysis` skill: it reads the | ||
| # code, finds the root cause, and comments with the fix, the risks, the tests | ||
| # to add, and every other place the same pattern appears. It does not change | ||
| # code here — say `/fix` on the issue for that. | ||
| # | ||
| # ───────────────────────────────────────────────────────────────────────────── | ||
| # CONFIGURATION — Settings → Secrets and variables → Actions | ||
| # | ||
| # The same two values the PR workflow uses. A .env file is not read by Actions; | ||
| # .env is for running the agent on your own machine. | ||
| # | ||
| # All four are required Secrets. Nothing is defaulted in this file: a model or | ||
| # a region hardcoded here is one somebody changes in the settings page and then | ||
| # wonders why the run ignored them. | ||
| # | ||
| # VERTEX_SA_JSON the full service-account JSON, pasted whole | ||
| # VERTEX_PROJECT your GCP project id | ||
| # VERTEX_LOCATION region, e.g. us-central1 | ||
| # VERTEX_MODEL e.g. gemini-2.5-pro, or gemini-2.5-flash for cheaper runs | ||
| # | ||
| # WHY THE `if:` BELOW MATTERS — this repository is public | ||
| # | ||
| # Anyone on the internet can open an issue or comment on one. Issue events run | ||
| # with access to secrets, so without a gate a stranger could open issues in a | ||
| # loop and spend your model budget, or say `/fix` and have the agent push a | ||
| # branch with GITHUB_TOKEN. Every job is therefore gated on author_association: | ||
| # owner, organization member, or collaborator only. | ||
| # ───────────────────────────────────────────────────────────────────────────── | ||
|
|
||
| name: ShipIT Forge (issue analysis) | ||
|
|
||
| on: | ||
| issues: | ||
| types: [opened, reopened, labeled] | ||
| issue_comment: | ||
| types: [created] | ||
|
|
||
| permissions: | ||
| contents: write | ||
| issues: write | ||
| pull-requests: write | ||
|
|
||
| concurrency: | ||
| group: forge-issue-${{ github.event.issue.number }} | ||
| cancel-in-progress: false | ||
|
|
||
| jobs: | ||
| analyze: | ||
| # Issues only — comments on pull requests belong to the PR workflow — and | ||
| # only from an author the repository already trusts. | ||
| if: >- | ||
| ${{ !github.event.issue.pull_request | ||
| && ((github.event_name == 'issues' | ||
| && contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.issue.author_association)) | ||
| || (github.event_name == 'issue_comment' | ||
| && contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association))) }} | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| # Say what is missing in one line, rather than an authentication error | ||
| # from inside the container three minutes later. | ||
| - name: Check credentials are configured | ||
| env: | ||
| SA_JSON: ${{ secrets.VERTEX_SA_JSON }} | ||
| PROJECT: ${{ secrets.VERTEX_PROJECT }} | ||
| LOCATION: ${{ secrets.VERTEX_LOCATION }} | ||
| MODEL: ${{ secrets.VERTEX_MODEL }} | ||
| run: | | ||
| missing="" | ||
| [ -z "$SA_JSON" ] && missing="$missing VERTEX_SA_JSON" | ||
| [ -z "$PROJECT" ] && missing="$missing VERTEX_PROJECT" | ||
| [ -z "$LOCATION" ] && missing="$missing VERTEX_LOCATION" | ||
| [ -z "$MODEL" ] && missing="$missing VERTEX_MODEL" | ||
| if [ -n "$missing" ]; then | ||
| echo "::error::ShipIT Forge is not configured. Missing:$missing" | ||
| echo "::error::Add them under Settings → Secrets and variables → Actions. Actions does not read a .env file." | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Runs the action from this commit rather than a tag — see the note in | ||
| # forge.yml. Consumers use `shipiit/forge@v2`. | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: ./ | ||
| with: | ||
| provider: vertex | ||
| vertex-credentials-json: ${{ secrets.VERTEX_SA_JSON }} | ||
| github-token: ${{ github.token }} | ||
| # The committed skill in .forge/skills/issue-analysis.md — the depth | ||
| # of the write-up lives there, in review, rather than in this file. | ||
| skill: issue-analysis | ||
| max-turns: '30' | ||
| env: | ||
| LLM_PROVIDER: vertex | ||
| VERTEX_PROJECT: ${{ secrets.VERTEX_PROJECT }} | ||
| VERTEX_LOCATION: ${{ secrets.VERTEX_LOCATION }} | ||
| VERTEX_MODEL: ${{ secrets.VERTEX_MODEL }} | ||
| # Analyse and explain. Opening a PR unasked on every issue is noise; | ||
| # `/fix` in a comment is the deliberate act that writes code. | ||
| FORGE_SPEND_CAP_RUN: '0.75' | ||
| FORGE_MAX_RUNS_PER_HOUR: '10' | ||
| FORGE_PROMPT_CACHE: '1' | ||
| FORGE_AUTO_FIX: 'off' | ||
| GITHUB_TOKEN: ${{ github.token }} |
There was a problem hiding this comment.
🔴 Critical · 🛡️ Security · CWE-287
Improved Access Control for Public Repository Issue Workflows
Similar to forge.yml, the forge-issues.yml workflow, designed for issue analysis, previously exposed issue_comment and issues events to untrusted actors on a public repository. Without the explicit author_association checks introduced in this PR, any external contributor could trigger the workflow.
How it happens: GitHub Actions runs issue and issue_comment events from the default branch with secrets, regardless of the author's trust level. An untrusted actor could, for example, open issues in a loop or comment /review to trigger expensive model runs.
What it costs: Potential for uncontrolled cloud spending (model budget) due to repeated, unauthorized LLM invocations.
How to fix it: The PR introduces a robust if: condition (lines 35-42) that gates the analyze job to run only when the issue or comment author is a trusted contributor (OWNER, MEMBER, or COLLABORATOR). This prevents untrusted actors from consuming resources or potentially interacting with sensitive workflows. This issue is fixed by the changes in the PR.
| # ShipIT Forge — research every issue and post the diagnosis. | |
| # | |
| # On a new issue it runs the committed `issue-analysis` skill: it reads the | |
| # code, finds the root cause, and comments with the fix, the risks, the tests | |
| # to add, and every other place the same pattern appears. It does not change | |
| # code here — say `/fix` on the issue for that. | |
| # | |
| # ───────────────────────────────────────────────────────────────────────────── | |
| # CONFIGURATION — Settings → Secrets and variables → Actions | |
| # | |
| # The same two values the PR workflow uses. A .env file is not read by Actions; | |
| # .env is for running the agent on your own machine. | |
| # | |
| # All four are required Secrets. Nothing is defaulted in this file: a model or | |
| # a region hardcoded here is one somebody changes in the settings page and then | |
| # wonders why the run ignored them. | |
| # | |
| # VERTEX_SA_JSON the full service-account JSON, pasted whole | |
| # VERTEX_PROJECT your GCP project id | |
| # VERTEX_LOCATION region, e.g. us-central1 | |
| # VERTEX_MODEL e.g. gemini-2.5-pro, or gemini-2.5-flash for cheaper runs | |
| # | |
| # WHY THE `if:` BELOW MATTERS — this repository is public | |
| # | |
| # Anyone on the internet can open an issue or comment on one. Issue events run | |
| # with access to secrets, so without a gate a stranger could open issues in a | |
| # loop and spend your model budget, or say `/fix` and have the agent push a | |
| # branch with GITHUB_TOKEN. Every job is therefore gated on author_association: | |
| # owner, organization member, or collaborator only. | |
| # ───────────────────────────────────────────────────────────────────────────── | |
| name: ShipIT Forge (issue analysis) | |
| on: | |
| issues: | |
| types: [opened, reopened, labeled] | |
| issue_comment: | |
| types: [created] | |
| permissions: | |
| contents: write | |
| issues: write | |
| pull-requests: write | |
| concurrency: | |
| group: forge-issue-${{ github.event.issue.number }} | |
| cancel-in-progress: false | |
| jobs: | |
| analyze: | |
| # Issues only — comments on pull requests belong to the PR workflow — and | |
| # only from an author the repository already trusts. | |
| if: >- | |
| ${{ !github.event.issue.pull_request | |
| && ((github.event_name == 'issues' | |
| && contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.issue.author_association)) | |
| || (github.event_name == 'issue_comment' | |
| && contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association))) }} | |
| runs-on: ubuntu-latest | |
| steps: | |
| # Say what is missing in one line, rather than an authentication error | |
| # from inside the container three minutes later. | |
| - name: Check credentials are configured | |
| env: | |
| SA_JSON: ${{ secrets.VERTEX_SA_JSON }} | |
| PROJECT: ${{ secrets.VERTEX_PROJECT }} | |
| LOCATION: ${{ secrets.VERTEX_LOCATION }} | |
| MODEL: ${{ secrets.VERTEX_MODEL }} | |
| run: | | |
| missing="" | |
| [ -z "$SA_JSON" ] && missing="$missing VERTEX_SA_JSON" | |
| [ -z "$PROJECT" ] && missing="$missing VERTEX_PROJECT" | |
| [ -z "$LOCATION" ] && missing="$missing VERTEX_LOCATION" | |
| [ -z "$MODEL" ] && missing="$missing VERTEX_MODEL" | |
| if [ -n "$missing" ]; then | |
| echo "::error::ShipIT Forge is not configured. Missing:$missing" | |
| echo "::error::Add them under Settings → Secrets and variables → Actions. Actions does not read a .env file." | |
| exit 1 | |
| fi | |
| # Runs the action from this commit rather than a tag — see the note in | |
| # forge.yml. Consumers use `shipiit/forge@v2`. | |
| - uses: actions/checkout@v4 | |
| - uses: ./ | |
| with: | |
| provider: vertex | |
| vertex-credentials-json: ${{ secrets.VERTEX_SA_JSON }} | |
| github-token: ${{ github.token }} | |
| # The committed skill in .forge/skills/issue-analysis.md — the depth | |
| # of the write-up lives there, in review, rather than in this file. | |
| skill: issue-analysis | |
| max-turns: '30' | |
| env: | |
| LLM_PROVIDER: vertex | |
| VERTEX_PROJECT: ${{ secrets.VERTEX_PROJECT }} | |
| VERTEX_LOCATION: ${{ secrets.VERTEX_LOCATION }} | |
| VERTEX_MODEL: ${{ secrets.VERTEX_MODEL }} | |
| # Analyse and explain. Opening a PR unasked on every issue is noise; | |
| # `/fix` in a comment is the deliberate act that writes code. | |
| FORGE_SPEND_CAP_RUN: '0.75' | |
| FORGE_MAX_RUNS_PER_HOUR: '10' | |
| FORGE_PROMPT_CACHE: '1' | |
| FORGE_AUTO_FIX: 'off' | |
| GITHUB_TOKEN: ${{ github.token }} | |
| This issue is fixed by the changes in `.github/workflows/forge-issues.yml` (lines 35-42). |
| if [ -n "$missing" ]; then | ||
| echo "::error::ShipIT Forge is not configured. Missing:$missing" | ||
| echo "::error::Add them under Settings → Secrets and variables → Actions. Actions does not read a .env file." | ||
| exit 1 |
There was a problem hiding this comment.
🟡 Medium · 🛡️ Security · CWE-798
Migration of VERTEX_PROJECT to GitHub Secrets for Enhanced Security
The previous forge.yml workflow configured VERTEX_PROJECT as a GitHub Actions vars (variable), and VERTEX_LOCATION and VERTEX_MODEL as hardcoded values. While a project ID might not be considered a highly sensitive secret like an API key, storing configuration that is part of a cloud environment's identity as a public variable in a public repository carries a risk of information exposure. More critically, hardcoding VERTEX_LOCATION and VERTEX_MODEL in the workflow itself makes the configuration less flexible and potentially less secure if these values were to change or require more restricted access.
How it happens: GitHub Actions vars are designed for non-sensitive configuration data and their visibility might be different from secrets in fork PR contexts. Hardcoding values directly into the workflow also reduces flexibility and relies on explicit changes for updates.
What it costs: Potential for accidental exposure of cloud project IDs (which could aid in reconnaissance for an attacker) or other environment details if GitHub's variable handling for public repos changed. Lack of centralized control for configuration.
How to fix it: The PR changes VERTEX_PROJECT from vars.VERTEX_PROJECT to secrets.VERTEX_PROJECT (line 109) and replaces hardcoded VERTEX_LOCATION and VERTEX_MODEL with secrets.VERTEX_LOCATION and secrets.VERTEX_MODEL respectively (lines 109-110). This ensures all these values are retrieved from GitHub Secrets, which are encrypted and explicitly not exposed to untrusted fork PRs. This enhances the security posture by treating cloud configuration more carefully and centralizing sensitive settings. This issue is fixed by the changes in the PR.
| if [ -n "$missing" ]; then | |
| echo "::error::ShipIT Forge is not configured. Missing:$missing" | |
| echo "::error::Add them under Settings → Secrets and variables → Actions. Actions does not read a .env file." | |
| exit 1 | |
| This issue is fixed by the changes in `.github/workflows/forge.yml` (lines 107-110). |
| } | ||
|
|
||
| // If a Vertex service-account JSON is provided inline (secret), materialize it. | ||
| const saJson = process.env.VERTEX_CREDENTIALS_JSON || process.env.INPUT_VERTEX_CREDENTIALS_JSON; | ||
| const saJson = process.env.VERTEX_CREDENTIALS_JSON || actionInput('vertex-credentials-json'); |
There was a problem hiding this comment.
🔵 Low · 🔧 Quality · CWE-776
Improved Secret Handling for Vertex AI Service Account JSON
The action previously relied on process.env.VERTEX_CREDENTIALS_JSON or the incorrect process.env.INPUT_VERTEX_CREDENTIALS_JSON to retrieve the Vertex AI service account JSON. The INPUT_VERTEX_CREDENTIALS_JSON was problematic due to the incorrect handling of dashed input names, as detailed in another finding. While VERTEX_CREDENTIALS_JSON could be set manually, using actionInput('vertex-credentials-json') leverages the corrected input parsing logic.
How it happens: The original code (const saJson = process.env.VERTEX_CREDENTIALS_JSON || process.env.INPUT_VERTEX_CREDENTIALS_JSON;) might have silently failed to retrieve credentials if only the dashed GitHub Action input was used and VERTEX_CREDENTIALS_JSON was not manually set as an environment variable.
What it costs: Difficulty in configuring and running the action, leading to authentication errors.
How to fix it: The PR updates the credential retrieval to use the actionInput helper (const saJson = process.env.VERTEX_CREDENTIALS_JSON || actionInput('vertex-credentials-json');). This change ensures that the vertex-credentials-json input is correctly read from the environment, fixing a previous misconfiguration issue and ensuring proper authentication with Vertex AI. This issue is fixed by the changes in the PR.
| } | |
| // If a Vertex service-account JSON is provided inline (secret), materialize it. | |
| const saJson = process.env.VERTEX_CREDENTIALS_JSON || process.env.INPUT_VERTEX_CREDENTIALS_JSON; | |
| const saJson = process.env.VERTEX_CREDENTIALS_JSON || actionInput('vertex-credentials-json'); | |
| This issue is fixed by the changes in `src/action.ts` (line 61). |
The first real CI review requested changes on this branch and listed five "findings" that were each the change working: every body ended "this issue is fixed by the changes in the PR". A review that reads the author's own fixes back to them as problems is worse than no review — it costs a read and teaches people to skim the next one. Worse, each carried a `suggestion` block containing prose. GitHub commits a suggestion verbatim when someone clicks it, so "This issue is fixed by the changes in forge.yml (lines 72-78)" would have replaced working code with that sentence. Both the default review prompt and the deep-review skill now state the test plainly — would this still be wrong if the PR merged exactly as it is? — and that a suggestion holds literal replacement code or is omitted.
🔍 ShipIT Forge reviewed this PR — 💬 commented (no blocking issues)0 finding(s) (0 security). See the review above for inline details and suggested fixes. 🧮 14,129 in + 1 out tokens · ~$0.0042 · model |
There was a problem hiding this comment.
ShipIT Forge review
✅ No blocking issues found. I reviewed the changed files and ran:
- 🛡️ Security checks — SSRF, injection (SQL/command/template), broken auth/authz, hardcoded secrets, unsafe deserialization, path traversal, weak crypto.
- 🔧 Code review — correctness, error handling, missing tests, clarity.
Nothing to flag. This is a comment, not an approval — ShipIT Forge never approves PRs; a human reviewer should approve and merge.
`app-id`, `private-key` and `github-token` read the same all-underscore env names the runner never sets, so App authentication could not engage: a workflow passing `app-id:` looked configured, quietly fell back to the workflow token, and every review was signed github-actions[bot] rather than by the app whose name is on the comment. Both workflows now pass the App credentials when they exist; without them it still works, it just says the wrong name. Needs FORGE_APP_ID and FORGE_PRIVATE_KEY to take effect. The security checklist also covers the classes it was quiet about, all of them things that bite in production rather than in theory: secret leakage on every path out (logs, URLs, CI output, container layers, git history), supply chain (mutable action tags, lockfile drift, untrusted input reaching a job that holds secrets), tenancy separation, unbounded cost on a metered API, and privacy of stored data.
🔍 ShipIT Forge reviewed this PR — 💬 commented (no blocking issues)3 finding(s) (3 security). See the review above for inline details and suggested fixes. 🧮 66,437 in + 1,176 out tokens · 31,140 cached (saved ~$0.0084) · ~$0.0238 · model |
| env: | ||
| LLM_PROVIDER: vertex | ||
| VERTEX_PROJECT: ${{ secrets.VERTEX_PROJECT }} | ||
| VERTEX_LOCATION: ${{ secrets.VERTEX_LOCATION }} |
There was a problem hiding this comment.
🔵 Low · 🛡️ Security · CWE-939
Hardcoded max-turns in workflow
The max-turns input for the Forge action is hardcoded to '30' in the forge-issues.yml workflow file. While this might be a reasonable default, it prevents repository administrators from overriding this value via agent.yml configuration without modifying the workflow file directly. This reduces flexibility and can lead to duplicated configuration logic if the intention is for max-turns to be configurable per repository.
To exploit this, an attacker might craft issues that intentionally cause the model to take more turns, leading to increased cost, knowing that the workflow-level cap is fixed and cannot be overridden by agent.yml if the repository owner wished to lower it for cost control.
The cost of this is limited to increased model spend. However, for a public repository, repeated excessive runs could incur unexpected charges.
To fix this, retrieve the max-turns value from the repository's agent.yml configuration or allow it to be passed as a variable to the workflow, similar to other configurable parameters.
| VERTEX_LOCATION: ${{ secrets.VERTEX_LOCATION }} | |
| max-turns: ${{ vars.FORGE_MAX_TURNS || 30 }} |
| private-key: ${{ secrets.FORGE_PRIVATE_KEY }} | ||
| # The committed skill in .forge/skills/deep-review.md — how deep to | ||
| # look and what a finding must contain lives there, in review, rather | ||
| # than in this file. |
There was a problem hiding this comment.
🔵 Low · 🛡️ Security · CWE-939
Hardcoded max-turns in workflow
The max-turns input for the Forge action is hardcoded to '30' in the forge.yml workflow file. While this might be a reasonable default, it prevents repository administrators from overriding this value via agent.yml configuration without modifying the workflow file directly. This reduces flexibility and can lead to duplicated configuration logic if the intention is for max-turns to be configurable per repository.
To exploit this, an attacker might craft PRs that intentionally cause the model to take more turns, leading to increased cost, knowing that the workflow-level cap is fixed and cannot be overridden by agent.yml if the repository owner wished to lower it for cost control.
The cost of this is limited to increased model spend. However, for a public repository, repeated excessive runs could incur unexpected charges.
To fix this, retrieve the max-turns value from the repository's agent.yml configuration or allow it to be passed as a variable to the workflow, similar to other configurable parameters.
| # than in this file. | |
| max-turns: ${{ vars.FORGE_MAX_TURNS || 30 }} |
Dismissed: this review reported the PR's own fixes as findings — every body ended "this issue is fixed by the changes in the PR" — and its suggestion blocks contained prose that would have replaced working code if applied. Both causes are fixed in this branch (e2b18d8); the re-review on the same code found 0 issues.
The second CI review labelled hardcoded max-turns and max-nits as security findings under CWE-939, with an invented story about "an attacker crafting PRs to cause more turns" — in a repository where opening a PR already requires trust. A configuration value not being overridable is a design preference, not a vulnerability, and a CWE that does not describe the defect makes every other citation worth less. Its suggestions were also anchored inside the `env:` block while proposing a `with:` key. Applying one produced invalid YAML. The prompt now says both plainly: the security lens is for harm an actor who is not already trusted can cause, and a suggestion must be valid where it lands — same block, same indentation.
🔍 ShipIT Forge reviewed this PR — 💬 commented (no blocking issues)0 finding(s) (0 security). See the review above for inline details and suggested fixes. 🧮 16,408 in + 1 out tokens · ~$0.0049 · model |
There was a problem hiding this comment.
ShipIT Forge review
✅ No blocking issues found. I reviewed the changed files and ran:
- 🛡️ Security checks — SSRF, injection (SQL/command/template), broken auth/authz, hardcoded secrets, unsafe deserialization, path traversal, weak crypto.
- 🔧 Code review — correctness, error handling, missing tests, clarity.
Nothing to flag. This is a comment, not an approval — ShipIT Forge never approves PRs; a human reviewer should approve and merge.
Every comment ended with what the run cost. On a public repository that publishes a team's spend to anyone reading a pull request, on every comment, forever. `show_cost: false` in agent.yml, or FORGE_SHOW_COST=0, hides it. Default is unchanged, so nobody else's comments lose a line they were relying on. Both workflows here set it off. The number is not lost — the run is still recorded with its tokens and its dollars. It is unpublished, not untracked.
|
👀 ShipIT Forge is running a code + security review on this PR… I'll post my review shortly. |
1 participant
Forge has never reviewed a PR in this repository's CI. Two reasons, both fixed here:
forge.ymlreadVERTEX_PROJECTfrom a repository variable that was never set, so every run died onAuthentication is not set up.@v1, which predates every input in the file and ignores them silently — configured-looking and inert.The depth lives in committed skills
.forge/skills/deep-review.md.forge/skills/issue-analysis.mdfile:lineevidence, every other place the pattern appears, the fix, its risks, the tests that would have caught it, and open questions.Skills, not workflow YAML, so the review standard is reviewable in a diff.
skill:was a no-op on these flowsThe input was only read by the mention and routine handlers. A workflow setting it on a review or an issue analysis changed nothing. Analysis and review now apply it too, and committed skills can declare
reports: findingslike the built-ins — so a repo's own review skill returns findings that get counted and filed instead of prose.Gating — the part that matters, because this repo is public
GitHub keeps secrets away from fork PRs. It does not do that for
issue_commentorissuesevents: those run from the default branch with secrets, whoever wrote them.Ungated, on a public repo, any stranger could:
/reviewin a loop and spend the model budget;/fixand have the agent clone a branch and run its tests with acontents: writetoken in the environment — the "pwn request" pattern, ending in a push.Every job is now gated on
author_association(OWNER / MEMBER / COLLABORATOR), and reviews are limited to same-repository PRs. There is no author check anywhere in the agent itself, so this workflow gate is the whole of the defence — worth keeping in mind if these files are copied elsewhere.Before this can run
Settings → Secrets and variables → Actions:
VERTEX_SA_JSONVERTEX_PROJECTOptional variables:
VERTEX_MODEL(defaultsgemini-2.5-pro— setgemini-2.5-flashfor a cheaper, faster review),VERTEX_LOCATION.A
.envfile does not work in Actions — it is gitignored and never read from the repo..envis forforge run/forge fixon your own machine.A precheck step now fails in seconds with the missing name, instead of an auth error three minutes into the container.
Verification
npm test— 550 passing (4 new: skill front matter, both committed skills parse, read-only tool sets)npx tsc --noEmit— clean