Hardwired PR review discipline into VietConnect CI and agent skill pipeline

The user shared 6 battle-tested PR review principles for solo/small teams working with AI agents. The session translated each principle into an actual enforcement point — CI steps, a GitHub PR template, skill prompt injections, and a new test-quality auditor — so agents enforce the discipline automatically without being reminded each time.

StatusLiveScopeVietConnect CI + global agent skillsDate2026-06-16Files changed9

ReasoningLập luận

6-layer thinking chainChuỗi tư duy 6 tầng

L1

Raw requestYêu cầu gốc

"tôi có thể ứng dụng lời khuyên này trong các workflow skill agent hiện tại như nào" (how can I apply this PR review advice to the existing agent workflow skills)

L2

Reframe — what was really neededDiễn giải lại — nhu cầu thật

The user had refined principles from real experience managing AI agents on PRs — agents claim tests pass without evidence, add fake assertions to pass CI, skip decision logs. The real ask: stop relying on instruction and start enforcing discipline at the points where enforcement actually works — CI gates, skill prompts, and PR templates that GitHub auto-fills. Not documentation. Guardrails.

L3

ConstraintsRàng buộc

- CI already runs lint/tsc/test/build; can't break existing PRs by raising coverage threshold above reality (actual: 34%, not the hoped 50%) - E2E tests exist in e2e/ but need real Supabase creds and seeded data; placeholder creds fail DB queries - Global skills (~/.claude) affect all projects — changes must be additive, not breaking - /ship is the primary PR creation path — must inject evidence/decision-log steps there, not in a new parallel skill

L4

Options weighedCác lựa chọn cân nhắc

Option A — Update CLAUDE.md only: agents skip docs mid-task, no real enforcement Option B — CI + PR template only: CI enforces but agents still write bad PR bodies before CI runs Option C — Full-stack: skills + CI + PR template (chosen): each layer catches different failure modes Option D — New /pr-review skill: duplicates /ship infrastructure, fragmentation, agents skip optional skills

L5

Principle invokedNguyên tắc áp dụng

Enforcement lives closest to the action: gates embedded in CI and skill prompts catch issues before the human ever sees them — documentation alone relies on the agent reading and remembering it.

L6

Pick + recognition signalLựa chọn + dấu hiệu nhận biết

Chose Option C — full-stack. PR template (GitHub auto-fills on every PR), CI scan (automated, runs on push), /ship Steps 1a+4a (agent collects evidence and checks decision log before creating PR), test-quality-audit.md (specialized subagent). Rejected Option A because agents consistently skip instructions they must look up; rejected Option D to avoid fragmenting the ship pipeline. Recognition signal: when you want agents to reliably do X, put X inside a mandatory flow they already take, not as a new optional step.

PipelineQuy trình

9 changes implemented in order

1

Install @vitest/coverage-v8

npm install --save-dev. Enables npm run test:coverage command.

2

GitHub PR Template

Created .github/PULL_REQUEST_TEMPLATE.md — GitHub auto-fills on every PR. Sections: Decision Log, Evidence (collapsible terminal output), Test Change Audit, CI Gate checklist.

3

vitest.config.ts coverage

Coverage block: v8 provider, 30% thresholds (below actual 34%), scoped to src/lib/** + src/app/api/**, excludes page.tsx/layout.tsx/components.

4

CI: coverage + test scan + E2E

ci.yml: npm test → npm run test:coverage; test quality scan step (::warning:: on toBe(true)/.skip); new e2e job (continue-on-error: true, chromium only, Playwright report artifact).

5

test-quality-audit.md (new)

Subagent prompt file: mock depth audit (NECESSARY/SUSPICIOUS/FAKE), assertion quality flags, coverage gaps, overall verdict: TRUSTWORTHY / NEEDS_SCRUTINY / FAKE_COVERAGE.

6

Wire test-quality-auditor into /code-review

Updated parallel-review-workflow.md (Step 3) and requesting-code-review.md to launch test-quality-auditor as 3rd parallel angle when diff has test files.

7

/ship Step 1a: Decision Log Check

Injected before Link Issues. Agent determines if change needs docs/decisions.md entry, drafts it, stores DECISION_LOG_STATUS for PR body.

8

/ship Step 4a: Evidence Collection

Injected after Tests pass. Captures actual npm test/lint/tsc terminal output. Injected verbatim into PR body at Step 12 — no paraphrasing.

9

CLAUDE.md + ship/pr-template.md

Session ritual item 6 (PR checklist gate). Task router new row. ship/pr-template.md: added Decision Log, Evidence, Test Change Audit sections.

DecisionsQuyết định

Layered decision cardsCác quyết định theo tầng

Coverage threshold: 30% not 50%

L1Set coverage threshold to enforce quality

L2A coverage threshold that immediately breaks CI is useless — it becomes a fire to put out rather than a signal to improve. The goal is a regression gate, not a minimum bar to hit on day one.

L3Actual current coverage: 34% scoped to src/lib/** + src/app/api/**. Setting 50% would fail on this very PR.

L450%: fails immediately, blocks CI — sets a bad precedent 0%: no regression protection — useless 34% exact: catches any drop but no buffer — brittle 30% (chosen): 4% buffer, catches major regressions, passes today

L5A gate that breaks the flow immediately gets disabled — set the threshold where it passes today, raise it as tests are written.

L6Chose 30%. Recognition signal: when adding coverage thresholds to a project with existing low coverage, set it 3-5% below current — not at target. The target is aspirational; the threshold is the floor.

E2E tests: non-blocking Phase 1

L1Add Playwright E2E tests to CI

L2E2E tests that always fail due to infrastructure constraints create noise, not signal. They need real Supabase creds and seeded DB; CI has placeholder creds only.

L3Playwright boots dev server on port 3100. Supabase calls happen at request time. Supplier browse and RFQ tests fail with placeholder creds (need seeded DB).

L4Blocking immediately: any E2E failure blocks all PRs until E2E passes — too aggressive Skip entirely: no value added Non-blocking (chosen): runs, reports artifact, doesn't block — builds CI muscle without creating noise Only run API tests: hard to maintain a passing subset list

L5Ship the infrastructure first; make it a hard gate only after it proves stable over 2-3 PRs.

L6Chose continue-on-error: true. Plan: remove that flag after E2E stabilizes with real DB. Recognition signal: new CI jobs on projects with incomplete infrastructure → non-blocking first.

Test scan: ::warning:: not ::error::

L1Detect suspicious test patterns in CI diff

L2Not all toBe(true) is fake — password validation returns a real boolean. Grep can find patterns but can't judge context. A blocking scan produces false positives and gets disabled.

L3::error:: blocks the PR and forces a bypass commit or skip. ::warning:: appears as a GitHub annotation on the diff without blocking — visible during review.

L4Block on pattern (::error::): false positives, developers bypass with comments Skip scan: no signal ::warning:: (chosen): visible, requires human judgment, doesn't create friction to bypass Report in PR comment via gh API: extra complexity for same result

L5Automate the detection, delegate the judgment — computers find patterns, humans decide if they're problems.

L6Chose ::warning::. The PR template Test Change Audit section is where the agent justifies any flagged pattern. Recognition signal: CI quality scans work best as annotations, not blocks.

Decision log check inside /ship, not a new optional skill

L1Enforce docs/decisions.md entry for every architectural PR

L2If the check is a separate skill the agent must remember to invoke, it will be skipped. It needs to be inside an existing mandatory flow that runs before PR creation.

L3docs/decisions.md already has 10 entries, a clean format, and is part of the session ritual. /ship is the mandatory PR creation path. The check must run before Step 12.

L4New /check-decisions skill (optional call): skipped under task pressure In CLAUDE.md instructions only: inconsistently read mid-task Step 1a in ship-workflow.md (chosen): mandatory on every /ship run PR template only: agent can write 'No architectural decision' without actually checking

L5Mandatory steps inside existing flows get executed; optional new skills get forgotten.

L6Injected as Step 1a. Agent assesses: new pattern/dependency/behavior change → draft entry; pure bug fix → skip. Stores DECISION_LOG_STATUS for PR body. Recognition signal: compliance that needs 100% reliability goes inside mandatory flows.

FilesTệp

Artifact mapBản đồ tệp tạo ra

PathĐường dẫn	WhatLà gì	Who reads itAi dùng
`.github/PULL_REQUEST_TEMPLATE.md`	GitHub PR template (new)	Auto-filled by GitHub on every PR. Evidence, Decision Log, Test Audit, CI Gate.
`.github/workflows/ci.yml`	CI pipeline (modified)	test:coverage + test quality scan + E2E job (non-blocking).
`vitest.config.ts`	Vitest config (modified)	Coverage block: v8, 30% thresholds, scoped to src/lib + src/app/api.
`CLAUDE.md`	Agent session guide (modified)	Session ritual item 6. Task router: Create or review a PR row.
`~/.claude/skills/code-review/references/test-quality-audit.md`	Test auditor prompt (new)	Mock audit, assertion flags, coverage gaps. Verdict: TRUSTWORTHY / NEEDS_SCRUTINY / FAKE_COVERAGE.
`~/.claude/skills/code-review/references/parallel-review-workflow.md`	Parallel review (modified)	Step 3: launch test-quality-auditor when test files in diff.
`~/.claude/skills/code-review/references/requesting-code-review.md`	Requesting review (modified)	Note: dispatch test-quality-auditor in parallel for test file diffs.
`~/.claude/skills/ship/references/ship-workflow.md`	/ship workflow (modified)	Step 1a: decision log check. Step 4a: evidence collection.
`~/.claude/skills/ship/references/pr-template.md`	/ship PR body template (modified)	Decision Log, Evidence (collapsible), Test Change Audit sections added.

Self-testTự kiểm tra

Check your understanding

Why was the coverage threshold set at 30% instead of the originally planned 50%?

50% was too difficult to implement in vitest config

The current actual coverage is 34%; setting 50% would immediately break CI on every PR including this one

30% is the recommended minimum for Next.js API routes

Coverage scoped to src/lib only makes higher thresholds impossible

In your own words: why was the decision log check added as Step 1a inside /ship, rather than as a separate skill the agent calls optionally?

The test quality scan emits ::warning:: instead of ::error::. What is the key reason?

GitHub Actions limits ::error:: to specific step types

toBe(true) on a boolean return value is legitimate; grep cannot distinguish real assertions from fake ones — human judgment is required

Warnings appear in Slack but errors do not

The test scan runs last so it cannot block earlier steps

Mastery checklist — tick what you can explain unpromptedBảng tự đánh giá — tích những gì bạn tự giải thích được