diff --git a/.agents/skills/engineering-workflow/SKILL.md b/.agents/skills/engineering-workflow/SKILL.md index 5ecbfa2..31dbc64 100644 --- a/.agents/skills/engineering-workflow/SKILL.md +++ b/.agents/skills/engineering-workflow/SKILL.md @@ -11,7 +11,7 @@ Use this skill only when an autonomous engineering workflow is expressly request 1. Inspect checkout and `git status`, task owner, current code, approved decisions and verification instructions. Preserve all pre-existing changes. Establish `BASE` (current HEAD), scope, authorized actions, and exact required checks. 2. Choose the **lightest authorized mode**: `DESIGN_ONLY` for explicit design-only/dormant contract tasks; `MEDIUM` for bounded feature changes; `HIGH_RISK` for financial, migrations, auth, concurrency, recovery and live integration. The user may narrow the mode, but a lower mode may not waive project safety gates. Simple fixes use the normal repository workflow instead. -3. Create one ignored handoff directory `.tmp/agent-workflows//` (verify with `git check-ignore`) and follow the repository's *Files and artifacts* rules. Keep only `state.json`, `plan.md`, one immutable `review-.md` per review and one compact verification summary (commands, exit codes, counts, short failure excerpts). No source snapshots, diff copies, full runner JSON or duplicate logs: Git and the checkpoint fingerprint already cover them. These files are the resume source: keep them until the workflow is finalized (human approval or rejection with the handoff delivered) or the user explicitly abandons it; never clean them at a pause or at `HUMAN_APPROVAL`. Do **not** create permanent per-round reports. Never save secrets. +3. Create one ignored handoff directory `.tmp/agent-workflows//` (verify with `git check-ignore`) and follow the repository's *Files and artifacts* rules. Keep only: `state.json`; `plan.md`; one immutable reviewer report per review attempt (for example `plan-review-1.md`, `code-review-1.md`; never reuse a name); `verification.json` (VERIFY evidence: commands, exit codes, counts, short failure excerpts); `final-checks.json` (FINAL_GATE evidence); and at most one compact `handoff.md` when a stage records a blocking note. Plan, verification and final-check files may be rewritten only by recording them again with the state helper, which re-digests them; any other change to a recorded file is `BLOCKED_EVIDENCE_CHANGED`. No source snapshots, diff copies, full runner JSON or duplicate logs: Git and the checkpoint fingerprint already cover them. These files are the resume source: keep them until the workflow is finalized (human approval or rejection with the handoff delivered) or the user explicitly abandons it; never clean them at a pause or at `HUMAN_APPROVAL`. Do **not** create permanent per-round reports. Never save secrets. 4. **Before any agent dispatch**, read [resume.md](references/resume.md), initialize or resume the local state ledger with `node scripts/engineering-workflow-state.mjs`, and inspect the returned guard status. Never infer completed work from a partial chat transcript or a reviewer summary. State and evidence are untrusted until checked against source, artifacts, execution logs and actual reviewer provenance. 5. Persist an atomic checkpoint after each *completed* stage, including an immutable review report path and a verified fresh reviewer context identifier for review stages. Preserve the original review records instead of overwriting a previous verdict. Persist pause reason when capacity, permissions or environment block progress. 6. On resume: recompute full nonignored source fingerprint and recorded artifact digests, preserve the correction budget and stage, and **redrive only an incomplete stage**. If evidence changed, do not reuse an approval. If a write stage is interrupted mid-edit, inspect its partial diff and finish that same stage before recording its checkpoint. If a review was interrupted before a saved verdict, dispatch a fresh reviewer; never claim an old incomplete attempt succeeded. diff --git a/.claude/skills/engineering-workflow/SKILL.md b/.claude/skills/engineering-workflow/SKILL.md index 5ecbfa2..31dbc64 100644 --- a/.claude/skills/engineering-workflow/SKILL.md +++ b/.claude/skills/engineering-workflow/SKILL.md @@ -11,7 +11,7 @@ Use this skill only when an autonomous engineering workflow is expressly request 1. Inspect checkout and `git status`, task owner, current code, approved decisions and verification instructions. Preserve all pre-existing changes. Establish `BASE` (current HEAD), scope, authorized actions, and exact required checks. 2. Choose the **lightest authorized mode**: `DESIGN_ONLY` for explicit design-only/dormant contract tasks; `MEDIUM` for bounded feature changes; `HIGH_RISK` for financial, migrations, auth, concurrency, recovery and live integration. The user may narrow the mode, but a lower mode may not waive project safety gates. Simple fixes use the normal repository workflow instead. -3. Create one ignored handoff directory `.tmp/agent-workflows//` (verify with `git check-ignore`) and follow the repository's *Files and artifacts* rules. Keep only `state.json`, `plan.md`, one immutable `review-.md` per review and one compact verification summary (commands, exit codes, counts, short failure excerpts). No source snapshots, diff copies, full runner JSON or duplicate logs: Git and the checkpoint fingerprint already cover them. These files are the resume source: keep them until the workflow is finalized (human approval or rejection with the handoff delivered) or the user explicitly abandons it; never clean them at a pause or at `HUMAN_APPROVAL`. Do **not** create permanent per-round reports. Never save secrets. +3. Create one ignored handoff directory `.tmp/agent-workflows//` (verify with `git check-ignore`) and follow the repository's *Files and artifacts* rules. Keep only: `state.json`; `plan.md`; one immutable reviewer report per review attempt (for example `plan-review-1.md`, `code-review-1.md`; never reuse a name); `verification.json` (VERIFY evidence: commands, exit codes, counts, short failure excerpts); `final-checks.json` (FINAL_GATE evidence); and at most one compact `handoff.md` when a stage records a blocking note. Plan, verification and final-check files may be rewritten only by recording them again with the state helper, which re-digests them; any other change to a recorded file is `BLOCKED_EVIDENCE_CHANGED`. No source snapshots, diff copies, full runner JSON or duplicate logs: Git and the checkpoint fingerprint already cover them. These files are the resume source: keep them until the workflow is finalized (human approval or rejection with the handoff delivered) or the user explicitly abandons it; never clean them at a pause or at `HUMAN_APPROVAL`. Do **not** create permanent per-round reports. Never save secrets. 4. **Before any agent dispatch**, read [resume.md](references/resume.md), initialize or resume the local state ledger with `node scripts/engineering-workflow-state.mjs`, and inspect the returned guard status. Never infer completed work from a partial chat transcript or a reviewer summary. State and evidence are untrusted until checked against source, artifacts, execution logs and actual reviewer provenance. 5. Persist an atomic checkpoint after each *completed* stage, including an immutable review report path and a verified fresh reviewer context identifier for review stages. Preserve the original review records instead of overwriting a previous verdict. Persist pause reason when capacity, permissions or environment block progress. 6. On resume: recompute full nonignored source fingerprint and recorded artifact digests, preserve the correction budget and stage, and **redrive only an incomplete stage**. If evidence changed, do not reuse an approval. If a write stage is interrupted mid-edit, inspect its partial diff and finish that same stage before recording its checkpoint. If a review was interrupted before a saved verdict, dispatch a fresh reviewer; never claim an old incomplete attempt succeeded. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e3685bf..08da1f9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -50,19 +50,25 @@ jobs: env: BASE_SHA: ${{ github.event.pull_request.base.sha }} run: node scripts/check-pr-review-signals.mjs "$BASE_SHA" HEAD - - name: Sync t2-area label + - name: Sync review labels if: ${{ !cancelled() && steps.signals.outputs.t2 != '' && github.event.pull_request.head.repo.full_name == github.repository }} env: GH_TOKEN: ${{ github.token }} GH_REPO: ${{ github.repository }} PR: ${{ github.event.pull_request.number }} + T2: ${{ steps.signals.outputs.t2 }} + GUARD: ${{ steps.signals.outputs.guard }} run: | - if [ "${{ steps.signals.outputs.t2 }}" = "true" ]; then - gh label create t2-area --color B60205 --description "Touches a T2 area: read carefully" --force - gh pr edit "$PR" --add-label t2-area - else - gh pr edit "$PR" --remove-label t2-area || true - fi + sync() { # label, wanted, description + if [ "$2" = "true" ]; then + gh label create "$1" --color B60205 --description "$3" --force + gh pr edit "$PR" --add-label "$1" + else + gh pr edit "$PR" --remove-label "$1" || true + fi + } + sync t2-area "$T2" "Touches a T2 area: read carefully" + sync guard-change "$GUARD" "Changes CI or a guard script: independent review" node: name: Node ${{ matrix.node }} checks diff --git a/AGENTS.md b/AGENTS.md index dd09498..c007af6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -14,7 +14,7 @@ This is a portfolio/reference payment platform. Financial correctness takes prio | --- | --- | --- | --- | --- | --- | | **T0 trivial** | Copy, typo, label/style, comment or doc wording; no behavior change | This file and the touched files | None | Cheapest relevant check for the touched package | Re-read the diff | | **T1 standard** | Bug fix, UI change, small feature or refactor outside T2 areas | T0 + `package.json`, the code entry points of the [reading matrix](docs/AGENT_START_HERE.md) row, and the one document owning the behavior you change | `ponytail`; `systematic-debugging` when the cause is unknown | [Verification commands](#verification-commands) for the touched package | `ponytail-review` of your own diff | -| **T2 high-risk** | Money movement, ledger, settlement/refund/dispute/payout, migrations/schema, auth/tenancy, concurrency/locks, idempotency/retries, outbox/inbox/workers, provider integration, data deletion, security | T1 + `README.md`, every document in the matrix row and the [approved decision records](docs/AGENT_START_HERE.md#approved-decision-records) for that area | T1 skills, `grilling` for unresolved decisions, `supabase-postgres-best-practices` for SQL | T1 + Docker integration for the affected boundary | Independent reviewer (see Review below) | +| **T2 high-risk** | Money movement, ledger, settlement/refund/dispute/payout, migrations/schema, auth/tenancy, concurrency/locks, idempotency/retries, outbox/inbox/workers, runtime module wiring, provider integration, data deletion, security | T1 + `README.md`, every document in the matrix row and the [approved decision records](docs/AGENT_START_HERE.md#approved-decision-records) for that area | T1 skills, `grilling` for unresolved decisions, `supabase-postgres-best-practices` for SQL | T1 + Docker integration for the affected boundary | Independent reviewer (see Review below) | Skill text saying "non-trivial" means T1 or T2. A bug with an unknown cause takes the size of the area it is in. `engineering-workflow` runs only when the user asks for it, normally for T2. `docs/README.md` is a navigation index, not required reading. @@ -69,7 +69,7 @@ Report each check as PASS, FAIL or NOT RUN with command, scope, counts and reaso Default to creating no new files. The deliverable is the code and tests the task needs; plans, reviews and evidence go in the chat, commit message or PR description. -- **Temporary files** live only in one ignored directory per task: `.tmp//` (`engineering-workflow` uses `.tmp/agent-workflows//`). Never write to the `.tmp/` root or to tracked paths. Keep at most `plan.md`, one `review-.md` per review, one compact verification summary (commands, exit codes, pass/fail counts, short failure excerpts) and, for `engineering-workflow`, its `state.json`. A task worktree (`.tmp//worktree`) is a checkout, not an artifact: remove it only with `git worktree remove` after its branch is merged or abandoned. Do not save full test-runner JSON, duplicate logs, source snapshots or diff copies; Git already holds them. Overwrite your own working files instead of versioning them; only reviewer reports stay immutable. +- **Temporary files** live only in one ignored directory per task: `.tmp//` (`engineering-workflow` uses `.tmp/agent-workflows//`). Never write to the `.tmp/` root or to tracked paths. Keep at most `plan.md`, one `review-.md` per review and one compact verification summary (commands, exit codes, pass/fail counts, short failure excerpts). `engineering-workflow` has its own fixed set, listed in its SKILL.md step 3: `state.json`, `plan.md`, immutable reviewer reports, `verification.json`, `final-checks.json` and at most one compact `handoff.md`. A task worktree (`.tmp//worktree`) is a checkout, not an artifact: remove it only with `git worktree remove` after its branch is merged or abandoned. Do not save full test-runner JSON, duplicate logs, source snapshots or diff copies; Git already holds them. Overwrite your own working files instead of versioning them; reviewer reports stay immutable. In `engineering-workflow`, a file recorded in `state.json` changes only through the state helper's next checkpoint, and none is edited or deleted otherwise before final human disposition. - **Cleanup:** a task's `.tmp` files are its resume source. Delete them only when the task is finalized (PR merged or rejected, or the human approved or rejected the result and the handoff is delivered) or the user explicitly abandons it. A paused or `HUMAN_APPROVAL` workflow is not finalized. At the end of each session, list what remains and why; if deletion is blocked, say so. Never read or reuse another task's `.tmp` directory unless asked; resuming a named workflow counts as asked. - **Permanent docs:** create a new Markdown file only when the user asks or a new subsystem has no owning document; otherwise edit the owning document. No per-round, per-review or per-checkpoint reports in `docs/`, and no run logs appended to `docs/verification.md` (it owns commands). A durable human decision gets one short entry in `docs/decisions/` (context, decision, consequences; one page at most). - **Size limits:** plan ≤200 lines; review = findings only, ≤150 lines; decision entry ≤1 page. Needing more is a signal to split the task. @@ -103,7 +103,7 @@ When the host provides them, use `vercel-react-best-practices` for T1/T2 React/N - Commit locally on the task branch when a checkpoint passes its checks, using Conventional Commits. A user instruction not to commit overrides this, and `engineering-workflow` runs keep their own no-automatic-commit rule. - Push, PR creation and merge need explicit user authorization in the session. Merging is always the human's decision: squash merge after required checks pass. - Fill the PR description from the [template](.github/pull_request_template.md), including the **review guide** (what to read carefully, what is mechanical, where you are least confident, any removed or weakened tests): size, invariants touched, checks with PASS/FAIL/NOT RUN, unverified boundaries, review and temporary files. Evidence lives there, not in `docs/`. -- CI's *Review signals* job labels PRs touching T2 paths `t2-area` (the label wins over a smaller claimed size) and fails on focused tests or new skips; env-gated suites and lines marked `allow-skip: ` are allowed. Never weaken a test to make a check pass. +- CI's *Review signals* job labels PRs touching T2 paths `t2-area` (the label wins over a smaller claimed size) and PRs changing CI or guard scripts `guard-change` (independent review, no T2 reading). It fails on focused tests and on new skips other than the env-gated suite alias or a `// allow-skip: ` comment; every allowed skip is listed for human approval. Never weaken a test to make a check pass. - Hosted CI counts only for the exact head SHA, read from the run (for example `gh pr checks`); never infer it from local runs. It is a merge gate, not a pre-PR gate: before a push it is reported as NOT RUN. - Existing migrations are append-only: CI (`scripts/check-migration-history.mjs`) rejects PRs that modify, rename or delete an existing SQL migration or snapshot, or change, remove or reorder an existing journal entry. New migrations, snapshots and appended journal entries pass. diff --git a/docs/verification.md b/docs/verification.md index 40da876..69f9c3d 100644 --- a/docs/verification.md +++ b/docs/verification.md @@ -263,10 +263,10 @@ For failure simulation run `pnpm --filter @fintech-lab/api test:unit` and use th - Node 22/24 jobs: `node scripts/sync-agent-skills.mjs --check` before dependency installation, then frozen install, `pnpm test:verification`, lint, unit/default tests, build, typecheck. - Node 24 integration job: fresh PostgreSQL 17 service container, frozen install, `pnpm db:migrate` and `pnpm test:integration` with `RUN_DB_TESTS=1`, `RUN_MIGRATION_TESTS=1` and `MIGRATION_ADMIN_URL` pointing only to that service. Its fixture admin can create databases/roles; publisher/worker assertions use restricted non-owner roles. Suites close connections and retain immutable fixture history locally; CI service disposal removes the job's isolated databases/roles. No seed, Redis or RabbitMQ is needed. -- Pull-request review signals (`node scripts/check-pr-review-signals.mjs HEAD`): lists T2-area files in the job summary and syncs the `t2-area` label, lists removed test cases as warnings, and fails on added `.only`/`fit`/`fdescribe` or new skips that are not env-gated or marked `allow-skip:`. Its regression tests run in `pnpm test:verification`. +- Pull-request review signals (`node scripts/check-pr-review-signals.mjs HEAD`): lists T2-area files and CI/guard changes in the job summary and syncs the `t2-area` and `guard-change` labels; lists removed test cases, net assertion removals, weak matchers and every `// allow-skip: ` skip as warnings; and fails on added `.only`/`fit`/`fdescribe` or new skips that are neither the exact env-gated suite alias nor annotated. Its regression tests run in `pnpm test:verification`. - Pull-request migration guard (`node scripts/check-migration-history.mjs HEAD`): fails when an existing SQL migration or snapshot is modified, renamed or deleted, or an existing journal entry is changed, removed or reordered; appended migrations, snapshots and journal entries pass. Its regression tests run in `pnpm test:verification`. -Jobs have read-only repository permissions and use no repository/service secrets. The workflow follows the official [PostgreSQL service-container guide](https://docs.github.com/en/actions/tutorials/use-containerized-services/create-postgresql-service-containers) and [pnpm action setup](https://github.com/pnpm/action-setup). +The workflow default is `contents: read`, and every job uses only that except *Review signals*, which adds `issues: write` and `pull-requests: write` at job level so `gh` can create the `t2-area` and `guard-change` labels (labels are an issues API) and add or remove it on the PR. That job checks out with `persist-credentials: false`, passes the token only to the label step, and skips labelling for fork PRs, whose tokens are read-only. No job uses repository or service secrets. The workflow follows the official [PostgreSQL service-container guide](https://docs.github.com/en/actions/tutorials/use-containerized-services/create-postgresql-service-containers) and [pnpm action setup](https://github.com/pnpm/action-setup). Configured CI is not executed CI evidence. No hosted run was triggered during this local pass. diff --git a/scripts/check-pr-review-signals.mjs b/scripts/check-pr-review-signals.mjs index 778c74d..26a942d 100644 --- a/scripts/check-pr-review-signals.mjs +++ b/scripts/check-pr-review-signals.mjs @@ -10,21 +10,46 @@ export const T2_PATHS = [ /^apps\/api\/drizzle\//, /^apps\/api\/src\/(audit|auth|balances|database|disputes|jobs|ledger|outbox|payment-commands|payment-provider|payments|payouts|rabbitmq|reconciliation|refunds|settlements|webhooks)\//, /^apps\/api\/src\/[^/]*(worker|relay)[^/]*\.ts$/, + // Shared infrastructure that carries financial invariants: API idempotency and its payload hash. + /^apps\/api\/src\/common\/(idempotency\.service|hash)\.ts$/, + // Runtime module wiring: registering a dormant writer here is an activation. + /^apps\/api\/src\/[^/]*app\.module\.ts$/, +]; +// CI and the guards themselves: weakening them removes protection from every later PR. They get a +// separate `guard-change` label (independent review) rather than T2, which would demand financial reading. +export const GUARD_PATHS = [ + /^\.github\/workflows\//, + /^scripts\/check-(migration-history|pr-review-signals)\.mjs$/, ]; const TEST_FILE = /(^|\/)test\/|\.(spec|test)\.[cm]?[jt]sx?$/; const FOCUS = /\b(?:it|test|describe)\.only\b|\bf(?:it|describe)\(/; const SKIP = /\b(?:it|test|describe)\.skip\b|\bx(?:it|test|describe)\(/; -// Environment-gated suites (RUN_DB_TESTS ? describe : describe.skip) and explicit opt-outs are allowed. -const ALLOWED_SKIP = /process\.env|allow-skip:/; +// Allowed skips: the repository's env-gated suite alias, exactly +// const describeDb = process.env.RUN_DB_TESTS === '1' [&& process.env.RUN_X === '1'] ? describe : describe.skip; +// or a trailing `// allow-skip: ` comment. Allowed skips are always listed for the human reviewer. +const ENV_GATE = /^\s*const\s+\w+\s*=\s*process\.env\.RUN_[A-Z0-9_]+\s*===\s*'1'(?:\s*&&\s*process\.env\.RUN_[A-Z0-9_]+\s*===\s*'1')*\s*\?\s*(describe|it|test)\s*:\s*\1\.skip\s*;?\s*$/; +const ALLOW_SKIP = /\/\/\s*allow-skip:\s*(\S.*)$/; +export const isEnvGate = (line) => ENV_GATE.test(line); +export const skipReason = (line) => line.match(ALLOW_SKIP)?.[1].trim() ?? null; +const ASSERTION = /\bexpect\(|\bassert(?:\.\w+)?\(/g; +const WEAK_MATCHER = /\.(toBeDefined|toBeTruthy|toBeFalsy)\(\)|expect\.anything\(\)|\.toHaveBeenCalled\(\)/; const TITLE = /\b(?:it|test)\(\s*(['"`])(.+?)\1/; +export function guardFiles(files) { + return files.filter((f) => GUARD_PATHS.some((re) => re.test(f))); +} + export function t2Files(files) { return files.filter((f) => T2_PATHS.some((re) => re.test(f))); } -/** diff: `git diff --unified=0` text. Returns { errors, removedTests }. */ +/** diff: `git diff --unified=0` text. Returns { errors, removedTests, assertionSignals, skipsAdded }. */ export function analyzeTestDiff(diff) { - const errors = [], added = new Set(), removed = []; + const errors = [], added = new Set(), removed = [], counts = new Map(), weak = [], skipsAdded = []; + const count = (file, key, body) => { + const c = counts.get(file) ?? { added: 0, removed: 0 }; + c[key] += (body.match(ASSERTION) ?? []).length; counts.set(file, c); + }; let file = null; for (const line of diff.split('\n')) { if (line.startsWith('+++ ')) { file = line.slice(4).replace(/^b\//, ''); continue; } @@ -32,13 +57,24 @@ export function analyzeTestDiff(diff) { const body = line.slice(1); if (line.startsWith('+')) { if (FOCUS.test(body)) errors.push(`${file}: focused test added: ${body.trim()}`); - else if (SKIP.test(body) && !ALLOWED_SKIP.test(body)) errors.push(`${file}: skipped test added (add "allow-skip: " if intended): ${body.trim()}`); + else if (SKIP.test(body) && !isEnvGate(body)) { + const reason = skipReason(body); + if (reason) skipsAdded.push({ file, reason, line: body.trim() }); + else errors.push(`${file}: skipped test added without the env-gate alias or a "// allow-skip: " comment: ${body.trim()}`); + } + if (WEAK_MATCHER.test(body)) weak.push({ file, line: body.trim() }); + count(file, 'added', body); const t = body.match(TITLE); if (t) added.add(t[2]); } else if (line.startsWith('-')) { + count(file, 'removed', body); const t = body.match(TITLE); if (t) removed.push({ file, title: t[2] }); } } - return { errors, removedTests: removed.filter((r) => !added.has(r.title)) }; + const assertionSignals = [ + ...[...counts].filter(([, c]) => c.removed > c.added).map(([file, c]) => `${file}: ${c.removed - c.added} assertion(s) removed net (${c.removed} removed, ${c.added} added)`), + ...weak.map((w) => `${w.file}: weak matcher added: ${w.line}`), + ]; + return { errors, removedTests: removed.filter((r) => !added.has(r.title)), assertionSignals, skipsAdded }; } if (process.argv[1] === fileURLToPath(import.meta.url)) { @@ -46,16 +82,21 @@ if (process.argv[1] === fileURLToPath(import.meta.url)) { if (!base) { console.error('Usage: node scripts/check-pr-review-signals.mjs [head-ref]'); process.exit(2); } const git = (...a) => execFileSync('git', a, { encoding: 'utf8', maxBuffer: 256 << 20 }); const files = git('diff', '--name-only', base, head).split('\n').filter(Boolean); - const t2 = t2Files(files); - const { errors, removedTests } = analyzeTestDiff(git('diff', '--unified=0', '--no-color', base, head)); + const t2 = t2Files(files), guards = guardFiles(files); + const { errors, removedTests, assertionSignals, skipsAdded } = analyzeTestDiff(git('diff', '--unified=0', '--no-color', base, head)); const lines = ['## Review signals', '']; lines.push(t2.length ? `**T2 area touched** — read these files carefully:\n${t2.map((f) => `- \`${f}\``).join('\n')}` : 'No T2-area files changed.'); + if (guards.length) lines.push('', `**CI or guard change** — needs an independent reviewer:\n${guards.map((f) => `- \`${f}\``).join('\n')}`); + if (skipsAdded.length) lines.push('', `**Skips added** — approve each reason:\n${skipsAdded.map((s) => `- \`${s.file}\`: ${s.reason}`).join('\n')}`); if (removedTests.length) lines.push('', `**Removed test cases** — confirm each removal is intended:\n${removedTests.map((r) => `- \`${r.file}\`: ${r.title}`).join('\n')}`); + if (assertionSignals.length) lines.push('', `**Assertion changes** — check that tests were not weakened:\n${assertionSignals.map((s) => `- ${s}`).join('\n')}`); if (errors.length) lines.push('', `**Blocking test changes:**\n${errors.map((e) => `- ${e}`).join('\n')}`); const report = lines.join('\n'); console.log(report); if (process.env.GITHUB_STEP_SUMMARY) appendFileSync(process.env.GITHUB_STEP_SUMMARY, report + '\n'); - if (process.env.GITHUB_OUTPUT) appendFileSync(process.env.GITHUB_OUTPUT, `t2=${t2.length ? 'true' : 'false'}\n`); + if (process.env.GITHUB_OUTPUT) appendFileSync(process.env.GITHUB_OUTPUT, `t2=${t2.length ? 'true' : 'false'}\nguard=${guards.length ? 'true' : 'false'}\n`); for (const r of removedTests) console.log(`::warning file=${r.file}::Removed test case: ${r.title}`); + for (const s of assertionSignals) console.log(`::warning::${s}`); + for (const s of skipsAdded) console.log(`::warning file=${s.file}::Skip added: ${s.reason}`); process.exit(errors.length ? 1 : 0); } diff --git a/scripts/check-pr-review-signals.test.mjs b/scripts/check-pr-review-signals.test.mjs index 3825eae..0805b28 100644 --- a/scripts/check-pr-review-signals.test.mjs +++ b/scripts/check-pr-review-signals.test.mjs @@ -1,9 +1,9 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; -import { analyzeTestDiff, t2Files } from './check-pr-review-signals.mjs'; +import { analyzeTestDiff, guardFiles, skipReason, t2Files } from './check-pr-review-signals.mjs'; // Keywords are split so this file never trips the guard itself. -const ONLY = 'it' + '.only', SKIP = 'describe' + '.skip'; +const ONLY = 'it' + '.only', SKIP = 'describe' + '.skip', WEAK = 'toBe' + 'Defined'; const diff = (file, ...lines) => [`--- a/${file}`, `+++ b/${file}`, '@@ -1 +1 @@', ...lines].join('\n'); const SPEC = 'apps/api/test/unit/ledger.spec.ts'; @@ -14,18 +14,61 @@ test('T2 paths are detected; others are not', () => { ]), ['apps/api/drizzle/0012_x.sql', 'apps/api/src/ledger/ledger.service.ts', 'apps/api/src/payment-command-worker.ts']); }); +test('shared high-risk infrastructure and runtime wiring are T2; other shared code is not', () => { + const files = [ + 'apps/api/src/common/idempotency.service.ts', 'apps/api/src/common/hash.ts', 'apps/api/src/app.module.ts', + 'apps/api/src/worker-app.module.ts', 'apps/api/src/common/correlation.middleware.ts', 'apps/api/src/common/domain-error.ts', + '.github/workflows/ci.yml', 'scripts/check-migration-history.mjs', 'scripts/sync-agent-skills.mjs', + ]; + assert.deepEqual(t2Files(files), files.slice(0, 4)); + assert.deepEqual(guardFiles(files), ['.github/workflows/ci.yml', 'scripts/check-migration-history.mjs']); +}); + test('ordinary new tests pass', () => { - assert.deepEqual(analyzeTestDiff(diff(SPEC, "+it('posts a balanced journal', () => {})")), { errors: [], removedTests: [] }); + assert.deepEqual(analyzeTestDiff(diff(SPEC, "+it('posts a balanced journal', () => { expect(1).toBe(1); })")), { errors: [], removedTests: [], assertionSignals: [], skipsAdded: [] }); }); test('focused test fails', () => { assert.match(analyzeTestDiff(diff(SPEC, `+${ONLY}('x', () => {})`)).errors.join(), /focused/); }); -test('new skipped test fails unless env-gated or explicitly allowed', () => { - assert.match(analyzeTestDiff(diff(SPEC, `+${SKIP}('x', () => {})`)).errors.join(), /skipped/); - assert.deepEqual(analyzeTestDiff(diff(SPEC, `+const d = process.env.RUN_DB_TESTS === '1' ? describe : ${SKIP};`)).errors, []); - assert.deepEqual(analyzeTestDiff(diff(SPEC, `+${SKIP}('x', () => {}); // allow-skip: flaky upstream`)).errors, []); +test('env-gated suite aliases pass silently', () => { + for (const line of [ + `const d = process.env.RUN_DB_TESTS === '1' ? describe : ${SKIP};`, + `const d=process.env.RUN_DB_TESTS==='1'&&process.env.RUN_MIGRATION_TESTS==='1'?describe:${SKIP};`, + ]) assert.deepEqual(analyzeTestDiff(diff(SPEC, `+${line}`)), { errors: [], removedTests: [], assertionSignals: [], skipsAdded: [] }, line); +}); + +test('a skip with an allow-skip comment passes but is listed for the human', () => { + const r = analyzeTestDiff(diff(SPEC, `+${SKIP}('x', () => {}); // allow-skip: Stripe test-mode outage`)); + assert.deepEqual(r.errors, []); + assert.deepEqual(r.skipsAdded.map((s) => s.reason), ['Stripe test-mode outage']); +}); + +test('mentioning process.env or allow-skip does not bypass the skip guard', () => { + for (const line of [ + `${SKIP}('x', () => {})`, + `${SKIP}('x', () => {}); // process.env`, + `${SKIP}(process.env.CI ? 'a' : 'b', () => {})`, + `const d = process.env.NEVER === 'x' ? describe : ${SKIP};`, + `const d = process.env.RUN_DB_TESTS === '1' || true ? describe : ${SKIP};`, + `${SKIP}('x', () => {}); // allow-skip:`, + `${SKIP}('x', () => {}); // allow-skip: `, + `${SKIP}('allow-skip: this is a title, not a comment', () => {})`, + ]) { + assert.equal(skipReason(line), null, line); + assert.match(analyzeTestDiff(diff(SPEC, `+${line}`)).errors.join(), /skipped/, line); + } +}); + +test('net assertion removal and weak matchers are surfaced, not blocking', () => { + const r = analyzeTestDiff(diff(SPEC, + "- expect(result.status).toBe('CAPTURED');", "- expect(journal.lines).toHaveLength(2);", + `+ expect(result).${WEAK}();`)); + assert.deepEqual(r.errors, []); + assert.match(r.assertionSignals.join('\n'), /1 assertion\(s\) removed net \(2 removed, 1 added\)/); + assert.match(r.assertionSignals.join('\n'), /weak matcher added: expect\(result\)\.toBeDefined\(\)/); + assert.deepEqual(analyzeTestDiff(diff(SPEC, "- expect(a).toBe(1);", "+ expect(a).toBe(2);")).assertionSignals, []); }); test('skips outside test files are ignored', () => {