[improvement] 4 quality gates for close-chg flow — milestone retro feedback (v6.0.32)

[fallingstone]

## TL;DR

Used `paceflow:pace-workflow` to ship a complete milestone (24 tasks / 14 commits / ~2400 lines). The flow structure (plan → artifact → check → execute → verify, APPROVED/VERIFIED audit chain, artifact-writer write-lock) worked great for cross-session state, audit traceability, and artifact consistency.

But after `close-chg + archive`, an independent code review found **1 P0 + 5 P1 + 8 P2 + 8 P3** that the `verify-summary` 4-checks (typecheck/test/lint/format) couldn't catch. Ended up opening **4 hotfix CHGs** to clean up.

I think a few targeted additions to the flow could prevent most of these. Sharing 4 concrete gap → fix proposals below.

## Background

This is feedback from a real milestone, not theoretical. Specifically the gaps showed up at `close-chg` time:

- **Wire alignment drift**: the new `X-Api-Key` default header was correctly published in agent-card.json, set in verifier defaults, used in smoke shell, set in deployment.yaml — but the middleware was still hard-reading `'authorization'`. All 4-check verifications passed (existing tests still used `Authorization: Bearer`). Production observatory sending `X-Api-Key` would have been 401 forever.
- **Plan-comment invariants left behind**: the plan explicitly said *"Task 2 also changes the middleware injection path"*, but that comment didn't surface as a sub-checklist on T-002. Implementation just covered the headline ("implement StaticApiKeyVerifier") and the middleware change was missed.
- **TDD red of dubious quality**: 3 of 24 red tasks were "import-error red" — the spec imports a not-yet-existing module, vitest fails at collect with `Tests no tests`, no assertions actually ran. Spec shape (parameter order, return structure, status code expectations) never validated. Later review found e.g. an "unknown systemCode" test that asserted the body contains a string but didn't assert the HTTP status code — exactly the kind of thing import-error red hides.
- **Cross-cutting protocol changes**: changing a "default wire protocol" naturally touches 4-5 files (agent card / server impl / client examples / deploy yaml / fixtures). The plan said *"keep these 4 places consistent"* but didn't list which 4. Implementation missed one.

## Proposed gaps & fixes

### Gap 1: `close-chg` lacks a mandatory REVIEWED gate (highest impact)

The `verify-summary` 4-checks are about automation. They can't catch wire-alignment drift, plan-comment invariants left behind, or subtle contract issues (timing-safe compare, trim semantics, status-code assertions). These need a human/AI review pass.

**Proposal**: add a **REVIEWED** marker (parallel to APPROVED / VERIFIED). Block `close-chg` until it's set.

operation: close-chg
target: CHG-...
review-confirmed: true
review-source: code-review-expert | manual | second-opinion
review-findings: |
P0: 0
P1: 0 (or hotfix CHG link)
P2: 3 (annotated / backlog)
P3: 5 (backlog)
verification-confirmed: true
verify-summary: ...
walkthrough-summary: ...

If review surfaces P0/P1 → must fix in a hotfix CHG (or annotate as won't-fix) before `close-chg`.

### Gap 2: plan-comment invariants don't get hoisted into task checklists

`create-chg` task fields are free-form text. LLMs implementing a task read the title + acceptance, but inline plan prose like *"also change X"* tends to get treated as background.

**Proposal**: add an `invariants` field on each task (required, can be empty array):

• T-002: Implement StaticApiKeyVerifier (green)
  files: [packages/server/src/auth/staticApiKeyVerifier.ts]
  invariants:
  • middleware switches from 'authorization' to env.API_KEY_HEADER ← hoisted from plan prose
  • mockVerifier.ts kept as deprecated alias for one cycle ← hoisted from plan prose
    acceptance: 8 tests pass

`update-status status=done` checks each invariant has a tick (note must explicitly address each one). Missing one = can't mark done.

### Gap 3: TDD-red quality not enforced — import-error red is silently accepted

Right now `[red]` and `[green]` are just title tags. `update-status` doesn't validate that the red actually ran assertions.

**Proposal**: red-task `update-status` requires a `red-evidence` field:

update-chg action=update-status task-id=T-NNN status=done
red-evidence: |
pnpm --filter test -- 2>&1 | tail
expected: "Tests N failed (N)" — not "Tests no tests"

Tool parses output. `Tests no tests` == import-error red, reject `done`.

Lighter version: add it to artifact-management documentation as a hard rule ("red tasks must produce N≥1 assertion failures, not import errors") and let humans/LLMs self-enforce.

### Gap 4 (minor): cross-cutting protocol changes need a plan-level checklist

When a milestone changes a protocol default that touches multiple files, the plan should require an explicit hit list, not just prose like "keep these consistent".

**Proposal**: plan template adds a `protocol-change-checklist:` section:

protocol-change: default wire protocol = X-Api-Key + raw mode
hit-list:
[ ] packages/server/src/routes/agentCard.ts securitySchemes.apiKey.name
[ ] packages/server/src/auth/verifier.ts fromEnv default
[ ] packages/server/src/auth/middleware.ts header read source
[ ] scripts/smoke.sh curl X-Api-Key header
[ ] deploy/k8s/deployment.yaml API_KEY_HEADER env value

Each item gets a tick during implementation. Unticked items at `close-chg` time = block.

## Priority matrix

If only one of these lands, **Gap 1 (REVIEWED gate) gives the most leverage** — a review agent naturally catches the issues that Gaps 2/3/4 are upstream defenses for.

| Gap | Leverage | Implementation cost |
|---|---|---|
| 1 REVIEWED gate | very high (catches everything) | medium — protocol field + artifact-writer flow addition |
| 2 invariants hoist | high | low — schema field + done check |
| 3 red-quality check | medium | low — output parsing |
| 4 cross-cutting checklist | medium | medium — plan template addition |

## Why this matters

The current paceflow is already strong on **process structure** (audit / state / consistency / APPROVED / VERIFIED) — much better than ad-hoc plan-execute workflows. Adding these quality gates would take it from "process tool" to "quality tool", which is what teams shipping production milestones actually need.

Happy to share more details, the actual milestone retro doc, or contribute a PR if there's interest.

---

**Environment**: paceflow v6.0.32, single milestone retrospective.

[paceaitian]

Thanks for the detailed milestone retro. This is useful feedback and I agree the reported failures are real quality gaps, not just noise.

I discussed the design tradeoff before changing code, and I do not want to rush a naive REVIEWED marker into the flow yet.

Current design notes:

• A review gate is valuable only if it is a state-machine gate, not just another prompt reminder. In other words: close-chg without review evidence should be mechanically rejected.
• At the same time, if review is required too often, it can become pure ceremony and make PaceFlow feel heavy. Review frequency needs a careful rule.
• Hooks should not judge whether a review is true or high quality. That would require AI judging AI and would make the hook layer too complex. The hook should only check mechanical evidence: review-confirmed, review-source, review-summary, and possibly P0/P1 disposition format.
• This is similar to APPROVED and VERIFIED: those markers do not prove perfect human approval or perfect test coverage; they prove that auditable process evidence exists.

Tentative direction:

1. Do not review every task by default.
2. Default review timing should be close to close-chg, after implementation and verification evidence exist.
3. Small/trivial CHGs may allow review-source: manual.
4. Code/security/protocol/API/deployment/cross-module CHGs should prefer an independent review agent or second-opinion review.
5. If review-findings contains P0/P1 items, close-chg probably should require a linked hotfix CHG or explicit wont-fix evidence before closing.

On the other three proposals:

• invariants, red-evidence, and protocol-change-checklist are good ideas, but they should not depend on artifact-writer magically discovering everything. If the main session misses a plan-comment invariant, a writer agent may miss it too.
• These are better treated first as review checklist inputs, not hard schema. The review pass should explicitly ask whether plan prose was not hoisted into tasks, whether red tests were real assertion failures rather than import/collection failures, and whether cross-cutting protocol changes have a complete hit-list.
• Once this has been dogfooded, the common patterns can be promoted into artifact schema or mechanical gates.

I recorded this as a P1-design item in the action plan. Temporary decision: no immediate implementation in v6.0.35; first design a low-friction close-chg review gate that preserves PaceFlow's core boundary: hooks mechanically enforce process evidence, models/humans judge content quality.