From c3cd8f776577dc90fa440d055eef0c7d1381b78d Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Wed, 29 Jul 2026 12:04:27 -0400 Subject: [PATCH] feat: SHOC PR review runner, phase 1 Manually-dispatched GitHub Actions workflow that reviews SHOC pull requests in a clean environment: exact-head checkout of shoc-frontend-new and shoc-backend, clean build/test gates, a truthful evidence report, a single-shot Fireworks review, deterministic output validation, and published artifacts. The runner never writes to the product repositories or their pull requests. The review checklists move here from the reviewers' local Cursor commands so the instructions live outside both product repos. Phase 1 does not provision a database, start either application, or run live browser flows; the evidence report records those as NOT_RUN so a review cannot claim them. Security architecture: building a PR executes its author's code, so the workflow is split. The gates job runs that code holding no Fireworks key and revokes its App token first; the review job holds the key, executes no product code, and re-checks out this repo fresh. Product checkouts live outside the workspace, the App token is downscoped at mint time, gate results fail closed on any duplicate key, changed files are read from git objects rather than the filesystem, and the validator re-checks every claim against the gate table. --- .github/dependabot.yml | 6 + .github/workflows/ci.yaml | 60 ++ .github/workflows/review-pr.yml | 322 ++++++ README.md | 167 ++- docs/phase-2-todo.md | 147 +++ review/runner-config.yml | 57 ++ review/schemas/review-input.schema.json | 53 + scripts/checkout-repositories.sh | 94 ++ scripts/collect-context.sh | Bin 0 -> 5038 bytes scripts/generate-evidence.sh | 138 +++ scripts/lib.sh | 105 ++ scripts/redact-check.sh | 93 ++ scripts/resolve-inputs.sh | 62 ++ scripts/resolve-pr-head.sh | 59 ++ scripts/run-backend-gates.sh | 45 + scripts/run-frontend-gates.sh | 70 ++ scripts/run-review-agent.sh | 199 ++++ scripts/validate-review-output.sh | 203 ++++ skills/pr-review/SKILL.md | 94 ++ .../references/backend-review-checklist.md | 714 +++++++++++++ .../references/frontend-review-checklist.md | 965 ++++++++++++++++++ .../references/review-output-format.md | 76 ++ templates/evidence-report.md | 67 ++ templates/failure-summary.md | 12 + templates/review-request.md | 11 + tests/fixtures/artifacts/frontend-pr.json | 1 + tests/fixtures/artifacts/gates-all-pass.tsv | 5 + tests/fixtures/artifacts/gates-lint-fail.tsv | 5 + tests/fixtures/artifacts/gates-tampered.tsv | 3 + tests/fixtures/inputs/invalid-bad-model.json | 1 + tests/fixtures/inputs/invalid-missing-pr.json | 1 + tests/fixtures/inputs/invalid-wrong-repo.json | 1 + tests/fixtures/inputs/valid-frontend.json | 1 + tests/fixtures/inputs/valid-paired-urls.json | 1 + .../reviews/invalid-blocker-no-fix.md | 15 + .../reviews/invalid-duplicate-sections.md | 28 + tests/fixtures/reviews/invalid-full-sha.md | 11 + tests/fixtures/reviews/invalid-gate-claim.md | 11 + tests/fixtures/reviews/invalid-live-claim.md | 11 + .../reviews/invalid-missing-heading.md | 7 + .../fixtures/reviews/invalid-rc-no-blocker.md | 11 + .../reviews/invalid-shared-fixtest.md | 22 + .../reviews/invalid-spliced-delimiters.md | 13 + .../fixtures/reviews/invalid-startup-claim.md | 11 + .../reviews/invalid-verdict-laundering.md | 11 + tests/fixtures/reviews/valid-approve.md | 11 + .../fixtures/reviews/valid-request-changes.md | 16 + tests/test-gate-integrity.sh | 84 ++ tests/test-input-validation.sh | 50 + tests/test-output-validation.sh | 61 ++ 50 files changed, 4209 insertions(+), 2 deletions(-) create mode 100644 .github/dependabot.yml create mode 100644 .github/workflows/ci.yaml create mode 100644 .github/workflows/review-pr.yml create mode 100644 docs/phase-2-todo.md create mode 100644 review/runner-config.yml create mode 100644 review/schemas/review-input.schema.json create mode 100755 scripts/checkout-repositories.sh create mode 100755 scripts/collect-context.sh create mode 100755 scripts/generate-evidence.sh create mode 100755 scripts/lib.sh create mode 100755 scripts/redact-check.sh create mode 100755 scripts/resolve-inputs.sh create mode 100755 scripts/resolve-pr-head.sh create mode 100755 scripts/run-backend-gates.sh create mode 100755 scripts/run-frontend-gates.sh create mode 100755 scripts/run-review-agent.sh create mode 100755 scripts/validate-review-output.sh create mode 100644 skills/pr-review/SKILL.md create mode 100644 skills/pr-review/references/backend-review-checklist.md create mode 100644 skills/pr-review/references/frontend-review-checklist.md create mode 100644 skills/pr-review/references/review-output-format.md create mode 100644 templates/evidence-report.md create mode 100644 templates/failure-summary.md create mode 100644 templates/review-request.md create mode 100644 tests/fixtures/artifacts/frontend-pr.json create mode 100644 tests/fixtures/artifacts/gates-all-pass.tsv create mode 100644 tests/fixtures/artifacts/gates-lint-fail.tsv create mode 100644 tests/fixtures/artifacts/gates-tampered.tsv create mode 100644 tests/fixtures/inputs/invalid-bad-model.json create mode 100644 tests/fixtures/inputs/invalid-missing-pr.json create mode 100644 tests/fixtures/inputs/invalid-wrong-repo.json create mode 100644 tests/fixtures/inputs/valid-frontend.json create mode 100644 tests/fixtures/inputs/valid-paired-urls.json create mode 100644 tests/fixtures/reviews/invalid-blocker-no-fix.md create mode 100644 tests/fixtures/reviews/invalid-duplicate-sections.md create mode 100644 tests/fixtures/reviews/invalid-full-sha.md create mode 100644 tests/fixtures/reviews/invalid-gate-claim.md create mode 100644 tests/fixtures/reviews/invalid-live-claim.md create mode 100644 tests/fixtures/reviews/invalid-missing-heading.md create mode 100644 tests/fixtures/reviews/invalid-rc-no-blocker.md create mode 100644 tests/fixtures/reviews/invalid-shared-fixtest.md create mode 100644 tests/fixtures/reviews/invalid-spliced-delimiters.md create mode 100644 tests/fixtures/reviews/invalid-startup-claim.md create mode 100644 tests/fixtures/reviews/invalid-verdict-laundering.md create mode 100644 tests/fixtures/reviews/valid-approve.md create mode 100644 tests/fixtures/reviews/valid-request-changes.md create mode 100755 tests/test-gate-integrity.sh create mode 100755 tests/test-input-validation.sh create mode 100755 tests/test-output-validation.sh diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..5ace460 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,6 @@ +version: 2 +updates: + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "weekly" diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml new file mode 100644 index 0000000..0b7bca8 --- /dev/null +++ b/.github/workflows/ci.yaml @@ -0,0 +1,60 @@ +name: ci + +# Repo-local CI for the runner itself. No org reusable fits a bash/workflow +# tooling repo, so this thin workflow lints every script and workflow, validates +# the input schema, and runs the bash test suite. The job is named `ci` so the +# required status context is `ci / ci`, matching the org ruleset convention. + +on: + pull_request: + branches: [main] + +permissions: + contents: read + +jobs: + ci: + runs-on: ubuntu-latest + timeout-minutes: 15 + concurrency: + group: ci-${{ github.ref }} + cancel-in-progress: true + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: shellcheck all scripts + run: | + shopt -s nullglob + files=(scripts/*.sh tests/*.sh) + echo "checking: ${files[*]}" + shellcheck --external-sources --source-path=scripts "${files[@]}" + + - name: actionlint all workflows + run: | + curl -sSfL -o actionlint.tar.gz \ + https://github.com/rhysd/actionlint/releases/download/v1.7.12/actionlint_1.7.12_linux_amd64.tar.gz + echo "8aca8db96f1b94770f1b0d72b6dddcb1ebb8123cb3712530b08cc387b349a3d8 actionlint.tar.gz" | sha256sum -c - + tar -xzf actionlint.tar.gz actionlint + ./actionlint -color + + - name: validate input schema + fixtures + run: | + # Pinned: the same integrity bar the actionlint download above meets. + python3 -m pip install --quiet 'check-jsonschema==0.37.4' + check-jsonschema --check-metaschema review/schemas/review-input.schema.json + for f in tests/fixtures/inputs/valid-*.json; do + check-jsonschema --schemafile review/schemas/review-input.schema.json "$f" + done + for f in tests/fixtures/inputs/invalid-*.json; do + if check-jsonschema --schemafile review/schemas/review-input.schema.json "$f" 2>/dev/null; then + echo "expected $f to FAIL schema validation" >&2; exit 1 + fi + done + + - name: bash tests + run: | + ./tests/test-input-validation.sh + ./tests/test-output-validation.sh + ./tests/test-gate-integrity.sh diff --git a/.github/workflows/review-pr.yml b/.github/workflows/review-pr.yml new file mode 100644 index 0000000..5a05441 --- /dev/null +++ b/.github/workflows/review-pr.yml @@ -0,0 +1,322 @@ +name: Review PR + +# SHOC PR Review Runner — Phase 1 (spec §8, §26). +# Manually dispatched. Checks out exact PR heads read-only, runs clean +# build/test gates, generates a truthful evidence report, invokes the Fireworks +# review agent, validates its output, and publishes artifacts. +# +# This workflow NEVER writes to the product repositories or their PRs: the +# GITHUB_TOKEN carries contents:read only (listing any permission zeroes every +# unlisted scope), and product-repo access uses a GitHub App installation token +# downscoped at mint time to contents/pull-requests/metadata READ. +# +# SECURITY ARCHITECTURE — why this is two jobs: +# Running the product repos' build gates executes code authored in the PR under +# review (npm lifecycle scripts, eslint/vite/vitest configs, MSBuild targets). +# That code must never share a job with a secret, because step-level `env:` is +# not an isolation boundary: PR code can poison $GITHUB_ENV for later steps, +# read a later step's /proc environ, or overwrite the runner's own scripts. +# Therefore: +# job `gates` — executes untrusted PR code. Holds NO Fireworks key, and the +# App token is revoked before the first build command runs. +# job `review` — holds the Fireworks key. Executes NO product-repo code; it +# re-checks out this repo fresh (so tampered scripts from the +# gates job cannot follow) and consumes only text artifacts. + +on: + workflow_dispatch: + inputs: + review_type: + description: Review type + required: true + type: choice + options: [frontend, backend, paired] + frontend_pr: + description: Frontend PR number (required for frontend/paired) + required: false + type: string + backend_pr: + description: Backend PR number (required for backend/paired) + required: false + type: string + ticket: + description: SH ticket identifier or URL + required: false + type: string + review_notes: + description: "Reviewer context. Sent to the Fireworks model and kept in run artifacts for 30 days — do not paste credentials." + required: false + type: string + run_mocked_e2e: + description: Run the mocked Playwright suite (frontend/paired) + required: true + type: boolean + default: true + model: + description: Fireworks review model + required: true + type: choice + default: deepseek-v4-pro + options: [deepseek-v4-pro, kimi-k2p6] + +permissions: + contents: read + +concurrency: + group: review-pr-${{ github.event.inputs.review_type }}-${{ github.event.inputs.frontend_pr }}-${{ github.event.inputs.backend_pr }} + cancel-in-progress: false + +env: + FRONTEND_REPO: Sea-Haven-Industries/shoc-frontend-new + BACKEND_REPO: Sea-Haven-Industries/shoc-backend + COMPANION_BRANCH: dev + REVIEW_TYPE: ${{ github.event.inputs.review_type }} + TICKET: ${{ github.event.inputs.ticket }} + REVIEW_NOTES: ${{ github.event.inputs.review_notes }} + +jobs: + gates: + name: Gates (${{ github.event.inputs.review_type }}) + runs-on: ubuntu-latest + timeout-minutes: 60 + steps: + - name: Checkout runner + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Configure workspace paths + # The product checkouts live OUTSIDE github.workspace so PR code is + # never a sibling of this repo's scripts. RUNNER_TEMP is only available + # as a shell variable, not in a workflow-level env block. + run: | + { + echo "WORKSPACE_DIR=$RUNNER_TEMP/workspace" + echo "ARTIFACTS_DIR=$RUNNER_TEMP/workspace/artifacts" + echo "STATE_DIR=$RUNNER_TEMP/runner-state-$(openssl rand -hex 8)" + } >> "$GITHUB_ENV" + + - name: Resolve and validate inputs + id: inputs + env: + FRONTEND_PR: ${{ github.event.inputs.frontend_pr }} + BACKEND_PR: ${{ github.event.inputs.backend_pr }} + run: ./scripts/resolve-inputs.sh + + - name: Mint read-only App token + id: app-token + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + app-id: ${{ secrets.SHOC_REVIEW_APP_ID }} + private-key: ${{ secrets.SHOC_REVIEW_APP_PRIVATE_KEY }} + owner: Sea-Haven-Industries + repositories: shoc-frontend-new,shoc-backend + # Downscope at mint time so the token stays read-only even if the App + # installation is later granted broader permissions. + permission-contents: read + permission-pull-requests: read + permission-metadata: read + + - name: Resolve frontend PR head + if: steps.inputs.outputs.frontend_pr != '' + id: frontend-head + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} + PR_NUMBER: ${{ steps.inputs.outputs.frontend_pr }} + run: ./scripts/resolve-pr-head.sh frontend "$FRONTEND_REPO" "$PR_NUMBER" + + - name: Resolve backend PR head + if: steps.inputs.outputs.backend_pr != '' + id: backend-head + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} + PR_NUMBER: ${{ steps.inputs.outputs.backend_pr }} + run: ./scripts/resolve-pr-head.sh backend "$BACKEND_REPO" "$PR_NUMBER" + + - name: Checkout product repositories at exact heads + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} + FRONTEND_SHA: ${{ steps.frontend-head.outputs.frontend_sha }} + BACKEND_SHA: ${{ steps.backend-head.outputs.backend_sha }} + run: ./scripts/checkout-repositories.sh + + - name: Collect review context + # Runs before any PR-authored code executes, so the collected diff and + # file contents cannot be tampered with by the build. + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} + run: ./scripts/collect-context.sh + + - name: Revoke App token before running untrusted code + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} + run: | + gh api -X DELETE /installation/token --silent || echo "token revoke returned non-zero (it also expires on its own)" + + # Everything below this line may execute code authored in the PR. + # No secret is present in this job from here on. + + - name: Set up .NET + if: inputs.review_type == 'backend' || inputs.review_type == 'paired' + uses: actions/setup-dotnet@a98b56852c35b8e3190ac28c8c2271da59106c68 # v6.0.0 + with: + dotnet-version: 8.0.x + + - name: Backend gates + if: inputs.review_type == 'backend' || inputs.review_type == 'paired' + continue-on-error: true + run: ./scripts/run-backend-gates.sh + + - name: Set up Node + if: inputs.review_type == 'frontend' || inputs.review_type == 'paired' + uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: "24" + + - name: Frontend gates + if: inputs.review_type == 'frontend' || inputs.review_type == 'paired' + continue-on-error: true + env: + RUN_MOCKED_E2E: ${{ inputs.run_mocked_e2e }} + run: ./scripts/run-frontend-gates.sh + + - name: Stop stray background processes + if: always() + run: | + # PR-authored scripts can background processes that would otherwise + # keep running and mutate files after the gates finish. + pkill -u "$(id -u)" -f 'node|dotnet|vite|playwright' 2>/dev/null || true + sleep 2 + + - name: Stage gate results for the review job + if: always() + run: | + cp "$STATE_DIR/gate-status.tsv" "$ARTIFACTS_DIR/gate-status.tsv" 2>/dev/null || true + ls -la "$ARTIFACTS_DIR" + + - name: Upload gates context + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + if: always() + with: + name: gates-context-${{ github.run_id }} + path: ${{ runner.temp }}/workspace/artifacts/ + retention-days: 1 + if-no-files-found: warn + + review: + name: Review + needs: gates + if: always() && needs.gates.result != 'cancelled' + runs-on: ubuntu-latest + timeout-minutes: 30 + steps: + - name: Checkout runner + # Fresh checkout: scripts tampered with in the gates job cannot follow. + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Configure workspace paths + # The product checkouts live OUTSIDE github.workspace so PR code is + # never a sibling of this repo's scripts. RUNNER_TEMP is only available + # as a shell variable, not in a workflow-level env block. + run: | + { + echo "WORKSPACE_DIR=$RUNNER_TEMP/workspace" + echo "ARTIFACTS_DIR=$RUNNER_TEMP/workspace/artifacts" + echo "STATE_DIR=$RUNNER_TEMP/runner-state-$(openssl rand -hex 8)" + } >> "$GITHUB_ENV" + + - name: Download gates context + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + name: gates-context-${{ github.run_id }} + path: ${{ runner.temp }}/workspace/artifacts + + - name: Point the gate table at the downloaded results + run: | + mkdir -p "$STATE_DIR" + cp "$ARTIFACTS_DIR/gate-status.tsv" "$STATE_DIR/gate-status.tsv" 2>/dev/null || true + + - name: Generate evidence report + run: ./scripts/generate-evidence.sh + + - name: Run review agent + id: agent + continue-on-error: true + env: + FIREWORKS_API_KEY: ${{ secrets.FIREWORKS_API_KEY }} + MODEL: ${{ inputs.model }} + run: ./scripts/run-review-agent.sh + + - name: Artifact redaction check + id: redact + # Runs even when earlier steps failed so nothing is uploaded unscanned. + if: always() + env: + FIREWORKS_API_KEY: ${{ secrets.FIREWORKS_API_KEY }} + run: ./scripts/redact-check.sh + + - name: Job summary + if: always() && steps.redact.outcome == 'success' + run: | + { + echo "## SHOC PR Review — ${REVIEW_TYPE}" + echo "" + echo "### Gate status (machine-recorded, authoritative)" + echo "" + echo "| Gate | Status |" + echo "| --- | --- |" + if [ -f "$STATE_DIR/gate-status.tsv" ]; then + awk -F'\t' '{printf "| %s | %s |\n", $1, $2}' "$STATE_DIR/gate-status.tsv" + fi + echo "" + if [ -f "$ARTIFACTS_DIR/review.md" ] && [ "${{ steps.agent.outcome }}" = "success" ]; then + echo "### Review (model-generated, copy into GitHub manually)" + echo "" + echo "The block below is model output influenced by PR content. The" + echo "gate table above is the authoritative record of what ran." + echo "" + echo '```markdown' + cat "$ARTIFACTS_DIR/review.md" + echo '```' + else + echo "### No valid review produced" + echo "" + echo "Agent step outcome: ${{ steps.agent.outcome }} — see validation-errors.txt in the artifacts." + fi + } >> "$GITHUB_STEP_SUMMARY" + + - name: Upload review + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + if: always() && steps.redact.outcome == 'success' + with: + name: review-${{ github.run_id }} + path: | + ${{ runner.temp }}/workspace/artifacts/review.md + ${{ runner.temp }}/workspace/artifacts/review-evidence.md + ${{ runner.temp }}/workspace/artifacts/gate-status.tsv + ${{ runner.temp }}/workspace/artifacts/validation-errors.txt + ${{ runner.temp }}/workspace/artifacts/logs/ + retention-days: 30 + if-no-files-found: warn + + - name: Upload prompt and diff material + # Contains full private-repo source; kept briefly for debugging only. + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + if: always() && steps.redact.outcome == 'success' + with: + name: review-context-${{ github.run_id }} + path: | + ${{ runner.temp }}/workspace/artifacts/agent-prompt*.txt + ${{ runner.temp }}/workspace/artifacts/agent-raw-response* + ${{ runner.temp }}/workspace/artifacts/prompt-*.txt + ${{ runner.temp }}/workspace/artifacts/*.diff + retention-days: 2 + if-no-files-found: warn + + - name: Fail run if agent or validation failed + if: steps.agent.outcome != 'success' + run: | + echo "Review agent or output validation failed — see artifacts." >&2 + exit 1 diff --git a/README.md b/README.md index c76e608..0bd4bb4 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,169 @@ # shoc-pr-review-runner Private, cloud-hosted PR review runner for the SHOC project -(`shoc-frontend-new` + `shoc-backend`). +(`shoc-frontend-new` + `shoc-backend`). A manually-dispatched GitHub Actions +workflow checks out the exact PR head(s) in a clean environment, runs the +repositories' real build/test gates, generates a truthful evidence report, +invokes a Fireworks-hosted review model with the SHOC review skill, validates +the output deterministically, and publishes the review + evidence as run +artifacts for a human to copy into GitHub. -Implementation lands in the phase 1 pull request. +**The runner never writes to the product repositories or their PRs.** The +workflow token has `contents: read` only, product-repo access uses a read-only +GitHub App installation token, and no write-scoped credential exists in this +repository. + +## Running a review + +1. Actions → **Review PR** → *Run workflow*. +2. Choose `review_type` (`frontend` | `backend` | `paired`) and enter the PR + number(s) or URL(s). Optional: ticket, reviewer notes, model. +3. When the run finishes, the review appears in the job summary and in the + `review-` artifact together with `review-evidence.md` and all logs. +4. Copy `review.md` into the GitHub PR manually. The runner never posts it. + +Single-repo reviews check out the companion repository at its `dev` head for +contract context; it is not built or reviewed. + +## Phase status + +Phase 1 (current): input validation, exact-head checkout, clean build/test +gates, mocked Playwright, evidence report, agent invocation, output validation, +artifacts. **Not yet implemented** (spec Phases 2–4): disposable SQL Server + +migrations, backend/frontend startup + health gates, live Playwright against a +real backend, stacked/paired PR intelligence. The evidence report marks all of +these NOT_RUN — reviews cannot claim them. + +## Layout + +| Path | Purpose | +| --- | --- | +| `skills/pr-review/` | Coordinating skill + frontend/backend checklists + output contract | +| `.github/workflows/review-pr.yml` | The review workflow (workflow_dispatch) | +| `.github/workflows/ci.yaml` | Repo CI: shellcheck, actionlint, schema check, bash tests | +| `scripts/` | Orchestration scripts (see headers in each) | +| `review/runner-config.yml` | Source-of-truth config record (mirrored by the workflow env) | +| `review/schemas/` | Input contract schema | +| `templates/` | Evidence / request / failure-summary templates | +| `tests/` | Bash test suites + fixtures (run in CI) | + +## Required secrets + +| Secret | Purpose | +| --- | --- | +| `SHOC_REVIEW_APP_ID` | GitHub App ID (read-only app, see below) | +| `SHOC_REVIEW_APP_PRIVATE_KEY` | The App's private key (PEM) | +| `FIREWORKS_API_KEY` | Fireworks inference API key | + +`run-review-agent.sh` fails fast with a clear error when the Fireworks key is +missing or rejected. + +## GitHub App + +The App must have **exactly** these permissions and nothing else, installed on +**only** `shoc-frontend-new` and `shoc-backend`: + +- Repository permissions: Contents **Read-only**, Pull requests **Read-only**, + Metadata **Read-only**. + +Verify after installing (and re-verify when the App changes): + +```sh +APP_INSTALLS=$(gh api /orgs/Sea-Haven-Industries/installations --jq \ + '.installations[] | select(.app_slug=="")') +echo "$APP_INSTALLS" | jq '{permissions, repository_selection}' +# permissions must be exactly {contents: "read", pull_requests: "read", metadata: "read"} +gh api "/user/installations/$(echo "$APP_INSTALLS" | jq -r .id)/repositories" \ + --jq '.repositories[].full_name' +# must list exactly the two product repos +``` + +## Security architecture + +Reviewing a PR means building it, and building it means executing code the PR +author wrote (npm lifecycle scripts, eslint/vite/vitest configs, MSBuild +targets). The workflow is therefore split into two jobs: + +| Job | Executes PR code | Secrets present | +| --- | --- | --- | +| `gates` | yes | App token, revoked before the first build command runs | +| `review` | no | Fireworks API key only | + +Step-level `env:` is not an isolation boundary inside a job — PR code can +append to `$GITHUB_ENV` to alter later steps, read a later step's process +environment, or overwrite the runner's own scripts. The job split is what makes +those attacks worthless: by the time any PR code runs, the gates job holds no +usable credential, and the review job re-checks out this repository fresh so +tampered scripts cannot follow it. + +Further controls: + +- Product repos are checked out under `$RUNNER_TEMP`, never beside this repo's + `scripts/`. +- The App token is downscoped **at mint time** (`permission-contents: read` and + friends), so it stays read-only even if the App installation is later granted + broader permissions. +- Fork PR heads are refused outright. +- Gate results are recorded once per key outside the workspace, and every + decision path calls `assert_gate_table_intact` first: a duplicate key means + something other than the runner wrote the table, and the run fails closed + rather than trusting a forged `PASS`. +- Changed-file contents are read from git objects, never the filesystem, so a + symlink committed in a PR cannot pull host files into the prompt. +- The review prompt fences untrusted material with a per-run nonce, and + `validate-review-output.sh` re-checks every claim against the recorded gate + table rather than trusting the model. + +**Residual risk, stated plainly:** someone with push access to a product repo +can make their own PR's gates report success by having the build fake it. The +integrity check turns the obvious forms of that into a hard failure, but a CI +system that builds untrusted code cannot fully certify its own results. The +review is a reviewing aid, not an authority — a human still reads the diff. + +## Data flow to third parties + +The PR diff and the contents of changed files are sent to **Fireworks AI** +(`api.fireworks.ai`) as the review prompt. This is private SHOC source leaving +the Sea Haven boundary to an external inference provider. Confirm the Fireworks +account has training and retention disabled before reviewing anything sensitive. + +Artifacts are split so raw source is not retained as long as the review: + +| Artifact | Contents | Retention | +| --- | --- | --- | +| `review-` | review, evidence report, gate table, gate logs | 30 days | +| `review-context-` | prompt, model responses, raw diffs | 2 days | + +Anyone with read access to this repository can read those artifacts. Keep this +repository's read audience no broader than both product repositories'. + +## Artifact hygiene + +`scripts/redact-check.sh` scans every staged artifact for the run's secret +values before upload and blocks the upload on any hit. If sensitive content is +ever discovered in a published artifact, delete it immediately: + +```sh +gh api repos/Sea-Haven-Industries/shoc-pr-review-runner/actions/artifacts \ + --jq '.artifacts[] | {id, name, created_at}' +gh api -X DELETE \ + repos/Sea-Haven-Industries/shoc-pr-review-runner/actions/artifacts/ +``` + +Artifact retention is 30 days. + +## Provisioning notes + +- Repo settings: `allow_auto_merge` + `delete_branch_on_merge` enabled; org + Code Security Configuration "Sea Haven Standard" attached. +- **CodeQL exemption:** this repository contains only shell, YAML, Markdown, + and JSON — no CodeQL-supported language — so CodeQL default setup is not + enabled. Revisit if a supported language is ever added. +- Dependabot: `github-actions` ecosystem, weekly. + +## Review instructions live here, not in the product repos + +The frontend and backend review checklists are deliberately **not** committed +to `shoc-frontend-new` or `shoc-backend` (spec §27.1). They were ported from +the reviewers' local `.cursor/commands/pr-review.md` files on 2026-07-29; this +repository is now their source of truth. diff --git a/docs/phase-2-todo.md b/docs/phase-2-todo.md new file mode 100644 index 0000000..4b9d3f2 --- /dev/null +++ b/docs/phase-2-todo.md @@ -0,0 +1,147 @@ +# Phase 2 TODO — Runtime Environment (disposable DB, startup, API checks) + +Scope (spec Phase 2): disposable database, migration validation, backend +startup + health gate, frontend startup, shared environment variables, API +runtime checks, process/log management. Everything below turns an existing +`NOT_RUN` line in `review-evidence.md` into a real gate recorded through +`scripts/lib.sh` (`record_gate`/`run_gate`). Facts about the product repos were +verified 2026-07-29; trust them over the original spec draft (which wrongly +said PostgreSQL — the backend is EF Core 8.0.8 + SQL Server). + +## Ordered work plan + +### 1. Workflow: SQL Server service container +- Add a `services: mssql` block to the `review` job in + `.github/workflows/review-pr.yml`: `mcr.microsoft.com/mssql/server:2022-latest`, + port `1433:1433`, `ACCEPT_EULA=Y`, `MSSQL_SA_PASSWORD` (see open questions), + container health-cmd so the job waits for readiness. No compose file exists in + shoc-backend; the service container is the whole database story. +- Install `sqlcmd` on the runner (`mssql-tools18` apt package) in a step gated + on backend/paired, before provisioning. +- Generate per-run runtime secrets in an early step: `JWT_SECRET` + (`openssl rand -hex 32` — must be ≥32 chars or login 500s) and the DB + password if per-run. Add both to `redact-check.sh`'s scan set. + +### 2. New scripts (all source `lib.sh`, all gated on backend/paired unless noted) +- `scripts/proc.sh` — shared process-management helpers sourced next to + `lib.sh`: `start_bg ` (nohup, PID to + `$ARTIFACTS_DIR/pids/.pid`, log to `$LOG_DIR`), `stop_bg`, + `stop_all_bg` (kill + wait, idempotent, never fails the caller). +- `scripts/provision-database.sh` — wait for SQL Server, `CREATE DATABASE + ShocReview` via sqlcmd; gate `backend.db_provision`. +- `scripts/run-migration-gates.sh` — migration validation (see §3); gates + `backend.migration_list`, `backend.migration_script`, `backend.migration_apply`. +- `scripts/seed-admin-user.sh` — direct-SQL identity seed (see §4); gate + `backend.seed_admin`. +- `scripts/start-backend.sh` — start the API via `proc.sh`, poll health, run + the DB-touching assertion (see §5); gates `backend.startup`, `backend.health`. +- `scripts/run-api-checks.sh` — authenticated runtime scenarios (see §6); + gate `backend.api_runtime` (plus per-scenario detail in the gate table). +- `scripts/start-frontend.sh` — frontend runtime startup (see §7); gates + `frontend.dev_startup`, `frontend.preview_build`, `frontend.preview_startup`. + Gated on frontend/paired; runtime API checks require paired (else NOT_RUN + with reason "no live backend in frontend-only review"). +- `scripts/stop-runtime.sh` — calls `stop_all_bg`; wired as an `if: always()` + workflow step so backend/frontend processes are stopped even after failed + stages (spec requirement), before evidence generation and artifact upload. + +### 3. Migration validation detail +- 43 migrations live in `Data.SeaHavenIndustries/Migrations/`; there is NO + migrate-on-startup and NO `IDesignTimeDbContextFactory`. +- `dotnet tool restore` must run from `Api.SeaHavenIndustries/` (the tool + manifest — dotnet-ef 8.0.8, rollForward false — is at + `Api.SeaHavenIndustries/.config/dotnet-tools.json`). +- Every `dotnet ef` call needs `--project Data.SeaHavenIndustries/... --startup-project + Api.SeaHavenIndustries/...`. +- Gates: `migrations list` (enumerates cleanly), `migrations script + --idempotent -o $ARTIFACTS_DIR/migrations.sql` (SQL generation as artifact), + then apply. Prefer `database update`; the repo's own deploy uses + `ef migrations bundle --self-contained -r linux-x64` (see its + `scripts/package-elastic-beanstalk.sh`) — bundle is the fidelity option if + `database update` misbehaves. + +### 4. Seeding strategy (first-admin bootstrap gap) +- No register endpoint; `UserController.AddUser` is `[Authorize]`; API user + creation emails passwords via SendGrid; the Program.cs role-seeding block is + fully commented out (Program.cs:199-253). So: seed by direct SQL. +- `seed-admin-user.sh` runs a `templates/seed-admin.sql` via sqlcmd inserting + `AspNetRoles` ("Admin" — the only role enforced in `[Authorize]` attributes), + `AspNetUsers` (fixed reviewer account, precomputed ASP.NET Identity v3 + PBKDF2 password hash), `AspNetUserRoles`. +- The known password + hash pair is committed (disposable localhost-only DB; + document as non-sensitive). Run after migration apply, before startup. + +### 5. Backend startup + health +- Start `dotnet run --project Api.SeaHavenIndustries/... --no-build -c Release` + (or run the built DLL) via `proc.sh` with the shared env (§8). +- NO anonymous /health endpoint exists (the only health-ish route is + Admin-authorized). Health probe = `GET /swagger/v1/swagger.json` with retry + budget; requires `ASPNETCORE_ENVIRONMENT=Development` (anything else disables + Swagger AND enables HTTPS redirect). +- Swagger 200 ≠ DB configured: the committed appsettings placeholder + `"${CONNECTION_STRING}"` lets the app start and fail per-request. So the + health gate also asserts DB wiring: `POST /api/Authentication/login` with bad + creds must return 401 — a 500 means the connection string didn't take. +- Expect log noise: 4 hosted services start at boot; the vendor-document scan + worker polls the DB every 30s. Capture full stdout/stderr to + `logs/backend-runtime.log`; evidence links it. + +### 6. API runtime checks +- With the seeded admin: login → 200 + JWT; call one Admin-authorized + endpoint with the token → 200; call it unauthenticated → 401. +- Record known limitation: ClamAV__Host is empty, so vendor uploads return + 423 — assert-and-record as limitation, not failure. + +### 7. Frontend runtime +- Dev startup gate: `npm run dev` with `VITE_API_TARGET=http://127.0.0.1:5141` + (the dev proxy target env var), probe `http://127.0.0.1:3000/` for 200. +- Production preview: `vite preview` (4173) has NO /api proxy, so a preview + against the local backend needs a second build with absolute + `VITE_API_URL=http://127.0.0.1:5141/api` baked at BUILD time (must end in + `/api` or the build-time contract guard throws; the Phase-1 gate build uses + relative `/api` and cannot be reused). Then probe 4173. +- Playwright's own dev server on 4173 is untouched (mocked suite unchanged). + +### 8. Shared environment variables (one place: workflow env + runner-config) +- `ConnectionStrings__DefaultConnection` = `Server=127.0.0.1,1433;Database=ShocReview;User Id=sa;Password=...;TrustServerCertificate=True` +- `JWT__Secret` = per-run ≥32 chars (the committed `"${JWT_SECRET}"` + placeholder passes the null check but breaks login — never rely on it) +- `ASPNETCORE_ENVIRONMENT=Development`, `ASPNETCORE_URLS=http://127.0.0.1:5141` +- `WorkOrderIngest__Enabled=false`, `Sync__Enabled=false`, + `WorkOrderReconciliation__Enabled=false`, `ClamAV__Host=` (empty) + +### 9. `review/runner-config.yml` additions +- `database:` extend with db name, sa-password sourcing, sqlcmd tooling. +- `backend:` add `runtime_env` block (§8 values), health retry budget, + seed account name, migration project paths. +- `frontend:` add `preview_port: 4173`, `dev_api_target`, preview build env. +- Keep the workflow-env mirror rule (CI cross-checks the pair). + +### 10. Evidence report (`generate-evidence.sh`) +- Backend Gates: Migration list/script/apply, Startup, Health endpoint, API + runtime scenarios switch from hardcoded NOT_RUN to `$(be_gate ...)`; add DB + provision + admin seed lines. +- Frontend Gates: Development startup and Production preview switch to + `$(fe_gate ...)`. +- Runtime Limitations rewritten: enumerate disabled integrations (ingest, + sync, reconciliation, ClamAV → uploads 423), note JWT secret and DB are + runner-provided, keep "mocked Playwright ≠ live coverage". +- Tests: new fixtures in `tests/fixtures/artifacts/` covering runtime-gate + PASS/FAIL rows; extend CI bash tests for `proc.sh` start/stop semantics. + +## Out of scope (Phase 3+) +- Live Playwright against the running stack, affected-route walking, console/ + failed-request capture (Phase 3). Stacked/paired PR intelligence (Phase 4). +- Fixing shoc-backend itself (health endpoint, seeding block) — record gaps. + +## Open questions +1. `MSSQL_SA_PASSWORD`: fixed throwaway (service env can't consume step + outputs) vs repo secret? Leaning fixed + documented non-sensitive. +2. sqlcmd via apt `mssql-tools18` vs `docker exec` into the service container? +3. Migration apply: `dotnet ef database update` vs the repo's own bundle path? +4. Is the dev-server startup gate worth its runtime once preview startup + exists, or is preview + dev-proxy config check enough? +5. Which Admin endpoint is the canonical authenticated smoke check (stable, + read-only, no side effects/emails)? +6. Should `backend.health` failing hard-block the frontend preview gates in + paired runs (BLOCKED) or let them probe independently? diff --git a/review/runner-config.yml b/review/runner-config.yml new file mode 100644 index 0000000..e0f39d2 --- /dev/null +++ b/review/runner-config.yml @@ -0,0 +1,57 @@ +# SHOC PR Review Runner configuration record (spec §23). +# +# This file is the human-readable source of truth for the values the workflow +# and scripts use. The workflow's env block mirrors these values; if you change +# one here, change it there in the same PR (CI cross-checks the pair). +# Values were verified against the real repositories on 2026-07-29. + +repositories: + frontend: + name: Sea-Haven-Industries/shoc-frontend-new + path: workspace/frontend + default_branch: dev # dev is the live integration branch + node_version: "24" # repo has no .nvmrc; engines >=22.22.1, CI uses 24 + install_command: npm ci # HUSKY=0 (prepare: husky runs on install) + lint_command: npm run lint + build_command: npm run build # tsc -b && vite build (no separate typecheck script) + test_command: npm test # vitest run + e2e_mocked_command: npm run test:e2e # Playwright, fully page.route-mocked + build_env: + VITE_API_URL: /api # must end in /api (build-time contract guard); + # committed .env.production would otherwise bake + # the deployed dev API URL into the bundle + port: 3000 # dev server; Playwright drives its own on 4173 + + backend: + name: Sea-Haven-Industries/shoc-backend + path: workspace/backend + default_branch: dev + dotnet_version: 8.0.x # no global.json; matches repo CI + solution: SeaHavenIndustries.sln + startup_project: Api.SeaHavenIndustries/Api.SeaHavenIndustries.csproj # Phase 2 + restore_command: dotnet restore SeaHavenIndustries.sln + build_command: dotnet build SeaHavenIndustries.sln --configuration Release --no-restore + test_command: dotnet test SeaHavenIndustries.sln --configuration Release --no-build + port: 5141 # Phase 2: launchSettings http profile + health_path: /swagger/v1/swagger.json # Phase 2: no anonymous /health exists; + # requires ASPNETCORE_ENVIRONMENT=Development + +database: # Phase 2 (not provisioned by Phase 1) + engine: sqlserver # spec draft said postgres; the backend is EF Core + image: mcr.microsoft.com/mssql/server:2022-latest # + SqlServer — corrected + port: 1433 + +agent: + provider: fireworks + base_url: https://api.fireworks.ai/inference/v1 + default_model: deepseek-v4-pro + allowed_models: [deepseek-v4-pro, kimi-k2p6] # enforced by the workflow choice input + diff_max_bytes: 200000 + files_max_bytes: 120000 + file_max_bytes: 65536 + +review: + artifact_retention_days: 30 + companion_branch: dev # single-repo reviews check out the companion here + require_live_browser_for_frontend_approval: false # Phase 3 flips this + allow_mocked_suite_as_live_evidence: false diff --git a/review/schemas/review-input.schema.json b/review/schemas/review-input.schema.json new file mode 100644 index 0000000..36b97f6 --- /dev/null +++ b/review/schemas/review-input.schema.json @@ -0,0 +1,53 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "title": "SHOC PR review request", + "description": "Contract for the review-pr workflow_dispatch inputs (spec §8). The workflow enforces this via resolve-inputs.sh; the schema documents the contract and validates test fixtures in CI.", + "type": "object", + "additionalProperties": false, + "required": ["review_type"], + "properties": { + "review_type": { + "enum": ["frontend", "backend", "paired"] + }, + "frontend_pr": { + "type": "string", + "pattern": "^([0-9]+|https://github\\.com/Sea-Haven-Industries/shoc-frontend-new/pull/[0-9]+(/.*)?)$", + "description": "Required when review_type is frontend or paired" + }, + "backend_pr": { + "type": "string", + "pattern": "^([0-9]+|https://github\\.com/Sea-Haven-Industries/shoc-backend/pull/[0-9]+(/.*)?)$", + "description": "Required when review_type is backend or paired" + }, + "ticket": { + "type": "string", + "maxLength": 200 + }, + "review_notes": { + "type": "string", + "maxLength": 4000 + }, + "run_mocked_e2e": { + "type": "boolean", + "default": true + }, + "model": { + "enum": ["deepseek-v4-pro", "kimi-k2p6"], + "default": "deepseek-v4-pro" + } + }, + "allOf": [ + { + "if": { "properties": { "review_type": { "const": "frontend" } } }, + "then": { "required": ["review_type", "frontend_pr"] } + }, + { + "if": { "properties": { "review_type": { "const": "backend" } } }, + "then": { "required": ["review_type", "backend_pr"] } + }, + { + "if": { "properties": { "review_type": { "const": "paired" } } }, + "then": { "required": ["review_type", "frontend_pr", "backend_pr"] } + } + ] +} diff --git a/scripts/checkout-repositories.sh b/scripts/checkout-repositories.sh new file mode 100755 index 0000000..2eeaa6a --- /dev/null +++ b/scripts/checkout-repositories.sh @@ -0,0 +1,94 @@ +#!/usr/bin/env bash +# Check out the product repositories at exact SHAs (spec §10). +# +# Repos under review are checked out at their PR head SHA. The companion repo of +# a single-repo review is checked out at its default branch head for contract +# context (spec §10.2) and recorded as such. +# +# Auth: the read-only App installation token is injected ONLY as a host-scoped +# Basic http.extraHeader via GIT_CONFIG_* environment variables for the fetch +# commands. It is never placed in a URL, never Bearer, and never written to +# .git/config, so nothing credential-bearing persists after the run. +# +# Usage: checkout-repositories.sh +# Reads: GH_TOKEN, REVIEW_TYPE, FRONTEND_REPO, BACKEND_REPO, +# FRONTEND_SHA / BACKEND_SHA (empty when that side has no PR), +# COMPANION_BRANCH (default: dev) + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=lib.sh +source "$SCRIPT_DIR/lib.sh" + +require_env GH_TOKEN REVIEW_TYPE FRONTEND_REPO BACKEND_REPO +COMPANION_BRANCH="${COMPANION_BRANCH:-dev}" + +# The Basic-auth header is a derived credential: GitHub masks the raw token it +# minted, but not this transformation of it. Register the mask explicitly so an +# accidental trace or debug flag cannot print a working credential to the log. +AUTH_HEADER_B64="$(printf 'x-access-token:%s' "$GH_TOKEN" | base64 | tr -d '\n')" +if [ -n "${GITHUB_ACTIONS:-}" ]; then + echo "::add-mask::$AUTH_HEADER_B64" +fi + +auth_git() { + # git with the Basic auth header injected via env for this invocation only. + GIT_CONFIG_COUNT=1 \ + GIT_CONFIG_KEY_0="http.https://github.com/.extraHeader" \ + GIT_CONFIG_VALUE_0="Authorization: Basic $AUTH_HEADER_B64" \ + GIT_TERMINAL_PROMPT=0 \ + git "$@" +} + +# fetch_at