feat(dev-workflow-v2): route reviews through record-review - #366

Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@tools/dev-workflow-v2/src/features/workflow/domain/fixtures/workflow-test-fixtures.ts`:
- Around line 84-95: The fixture reviewRecorded currently hardcodes reviewId: 1
which makes events share the same ID; change reviewRecorded to assign a unique
incremental ID per call (e.g., introduce a module-level counter like
nextReviewId and use/increment it inside reviewRecorded) so each returned
WorkflowEvent has a distinct reviewId; update references to reviewId in the
function signature/return only, leaving reviewType and verdict unchanged.

In `@tools/dev-workflow-v2/src/features/workflow/domain/fold.spec.ts`:
- Around line 233-270: The test suite is missing coverage for the code-review
and bug-scanner branches in applyRecordedReviewVerdict invoked via applyEvent;
add tests similar to the existing ones that create WorkflowEvent objects with
reviewType 'code-review' and 'bug-scanner' (each with verdict 'PASS' and 'FAIL'
where applicable), call applyEvent(EMPTY_STATE, event) or with a pre-set state,
and assert the corresponding state flags or fields updated by
applyRecordedReviewVerdict (mirror how taskCheckPassed and
architectureReviewPassed are asserted) to ensure both PASS and FAIL behavior for
code-review and bug-scanner are covered.

In `@tools/dev-workflow-v2/src/features/workflow/domain/fold.ts`:
- Around line 4-32: The switch in applyRecordedReviewVerdict is not exhaustive
and can return undefined for new review types; add an explicit exhaustiveness
assertion fallback (e.g., a default case that calls an assertNever-like helper
or throws with the unexpected reviewType) so every branch returns a
WorkflowState; reference applyRecordedReviewVerdict and the event.reviewType
discriminant, and either import/define assertNever or throw an Error including
the unexpected event.reviewType to ensure the function never silently returns
undefined.

In `@tools/dev-workflow-v2/src/features/workflow/domain/output-messages.spec.ts`:
- Around line 14-16: The test title "handles three-word operations" is
misleading because it asserts getOperationBody('record-review') (two words);
update the spec so the test name matches behavior: either remove this redundant
it-block entirely, or change the asserted input to a true 3+ word operation
(e.g., pass a hyphenated string like 'record-feedback-clean' to
getOperationBody) and update the expectation accordingly; locate the test in
output-messages.spec.ts referencing getOperationBody to apply the change.

In `@tools/dev-workflow-v2/src/features/workflow/domain/workflow-events.spec.ts`:
- Around line 256-289: Add negative test cases to the existing REVIEW block to
assert enum validation on WORKFLOW_EVENT_SCHEMA: inside the
describe('WORKFLOW_EVENT_SCHEMA — review-recorded') add tests that call
WORKFLOW_EVENT_SCHEMA.parse with an unknown verdict (e.g. verdict: 'MAYBE' or
'pass') and with an unknown reviewType (e.g. reviewType: 'arch-review') and
assert they throw (use expect(() =>
WORKFLOW_EVENT_SCHEMA.parse(...)).toThrow(...) or a toThrow matching the Zod
error). This targets the enum constraints for verdict and reviewType so
malformed review JSON is rejected; keep the same surrounding fields (type:
'review-recorded', at: AT, reviewId) in each test.

In
`@tools/dev-workflow-v2/src/features/workflow/entrypoint/workflow-cli-transitions.spec.ts`:
- Around line 153-304: Duplicate review setup across tests should be extracted
into a reusable helper: create a fixture helper (e.g., function
applyStandardReviewBatch or recordStandardReviews) that accepts the test context
(ctx) and an optional task-check verdict, and inside calls runReviewCommand for
'architecture-review', 'code-review', 'bug-scanner' (all PASS) and conditionally
runReviewCommand for 'task-check' with the provided verdict; replace the
repeated blocks in the five tests with a single call to this helper and move the
helper into a new fixtures module alongside the spec so other tests can import
it.

In `@tools/dev-workflow-v2/src/features/workflow/entrypoint/workflow-cli.spec.ts`:
- Around line 218-231: Update the "blocks workflow without recording review when
reviewer omits required fields" test to mirror the invalid-JSON test by also
asserting that no "review-recorded" event was appended; after calling
runReviewCommandWithInput and the existing assertions on
exitCode/output/listSessionReviews, add an assertion against
ctx.engineDeps.store's event list (the same event-check used in the invalid-JSON
test) to ensure it does not contain a review-recorded event (use the same helper
or expectation pattern as in the invalid-JSON test so the test fails if a bogus
review-recorded event is persisted).

---

Outside diff comments:
In `@tools/dev-workflow-v2/agents/architecture-review.md`:
- Around line 51-98: The "Audit Report" header creates a conflict with the
JSON-only response contract by instructing both free-form sections and "return
review JSON"; update the phrasing around the block that starts with "Your
response must include, in this exact order:" so it clearly states that those
sections (Findings, Full Audit Trail, Audit Summary) are the expected contents
of the file written to "Report Path" and not the agent's HTTP response;
specifically, change the sentence that references "return review JSON" and the
header text to explicitly say "Write the following content to Report Path (the
report file) in this exact order; the agent's API response must still return
only the JSON payload as specified elsewhere," and ensure the literal strings
"return review JSON" and "Your response must include, in this exact order:" are
adjusted to remove ambiguity (affecting the Audit Report header and surrounding
instructions).

In `@tools/dev-workflow-v2/agents/task-check.md`:
- Around line 92-113: Clarify the ambiguity between the human-readable
"Verification Report" checklist section and the machine "Output Format" JSON by
updating the document so it explicitly states that the checklist, PRD compliance
section, and unmet-criteria details are the contents of the separate
verification report written to the Report Path (the human-readable report),
while the agent's immediate response must be the JSON object matching the schema
under "Output Format" (verdict/summary/findings); reference the section headings
"Verification Report" and "Output Format" so reviewers can find and verify the
change.

In
`@tools/dev-workflow-v2/src/features/workflow/entrypoint/fixtures/workflow-cli-test-fixtures.ts`:
- Around line 119-138: The function progressToState should validate the shape of
'record-review' steps from STATE_STEPS before reading step[2]; update
progressToState to assert or guard that for steps where step[0] ===
'record-review' the step array has at least three elements and step[1] ===
'--type' (or destructure like const [cmd, flag, reviewType] and check flag ===
'--type' && reviewType), and if the check fails throw a WorkflowStateError with
a clear message; then continue to call runReviewCommand(ctx, reviewType, ...) as
before so malformed STATE_STEPS cannot silently misinterpret arguments.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@tools/dev-workflow-v2/src/features/workflow/domain/fixtures/workflow-test-fixtures.ts`:
- Line 31: The fixture's makeDeps().listSessionReviews currently always returns
an empty array, diverging from fixture methods that emit review-recorded events
(reviewRecorded(...), allReviewsPassed()), so update the fixture to maintain an
internal in-memory review log that those helper functions append to and have
makeDeps().listSessionReviews return a readonly snapshot of that log;
specifically modify the helpers (reviewRecorded, allReviewsPassed) to push
StoredReview entries into the fixture-side array and change
makeDeps().listSessionReviews to return that array (or a shallow copy) so
Workflow.getRecordedReviews() and getLatestReviewByType() see the same reviews
emitted by the fixture while still allowing tests to override listSessionReviews
when needed.

In
`@tools/dev-workflow-v2/src/features/workflow/domain/workflow-reviewing-submitting.spec.ts`:
- Around line 118-137: The test directly calls
getReviewingTransitionGuard()/reviewingState.transitionGuard with a handcrafted
TransitionContext and asserts the exact human-readable reason string, which
makes the spec brittle; update the test to either (a) drive the transition
through the engine using the existing spec.given(...).when(...) pattern used
elsewhere (so engine-level checks like currentStateMachineState/from/to are
exercised consistently), or (b) keep the unit guard call to
getReviewingTransitionGuard() but stop asserting the full prose in result.reason
— instead assert result.pass === false and assert on a structured marker (e.g.,
inspect result.unmetReviews or that result.reason contains the specific unmet
review identifiers such as 'architecture-review', 'code-review', 'bug-scanner')
so the test verifies behavior without being tied to exact copyedits of the human
message.
- Line 31: The createdAt timestamp template in createStoredReview embeds the raw
id into the seconds field, which breaks for ids >= 10; update the createdAt
construction in workflow-reviewing-submitting.spec.ts (the createStoredReview
test helper) to produce a valid ISO-8601 seconds value by zero-padding the id to
two digits (e.g., use String(id).padStart(2, '0')) or by building the Date/ISO
string from a Date object so seconds are always two digits and valid for all
ids.

In `@tools/dev-workflow-v2/src/features/workflow/domain/workflow.ts`:
- Around line 204-209: getLatestReviewByType currently picks the last element
from reviewsOfType using reviewsOfType.at(-1), which is unsafe if the source
ordering isn't guaranteed; update getLatestReviewByType to sort reviewsOfType by
their createdAt timestamp (e.g., ascending) and then return the last element so
the function deterministically returns the most recent StoredReview; reference
the getLatestReviewByType method and the RecordedReview/StoredReview.createdAt
field (or the output of getRecordedReviews()/WorkflowDeps.listSessionReviews)
when implementing the sort.

In
`@tools/dev-workflow-v2/src/features/workflow/entrypoint/fixtures/workflow-cli-test-fixtures.ts`:
- Around line 124-136: The loop handling 'record-review' in progressToState
currently always sets verdict: 'PASS' when calling runReviewCommand(ctx,
reviewType, ...), which hides non-passing scenarios; update this branch to (a)
read an optional verdict from step[3] (e.g., const verdict = step[3] ?? 'PASS')
and pass that verdict through to runReviewCommand, or if you intend to only
create passing reviews, rename the branch to recordPassingReview and add a JSDoc
comment on progressToState/runReviewCommand usage to document the enforced PASS
behavior so callers aren’t surprised.

In
`@tools/dev-workflow-v2/src/features/workflow/infra/persistence/workflow-definition.ts`:
- Line 47: The Set construction for known workflow event types is being
recreated inside the hot fold path; hoist it to module scope by creating a
single top-level constant (e.g., a module-scoped Set built from
getKnownWorkflowEventTypes()) and replace the local const knownTypes = new
Set(getKnownWorkflowEventTypes()) usage inside the fold routine with that
module-scoped constant so events are checked against the prebuilt Set instead of
allocating a new one per call.

In `@tools/dev-workflow-v2/states/reviewing.md`:
- Around line 60-62: The current transition rules deadlock because the "Cannot
transition to IMPLEMENTING if all 3 reviews passed (go to SUBMITTING_PR
instead)" rule is unconditional; change it to only redirect to SUBMITTING_PR
when the task-check requirement (when githubIssue exists) is satisfied. In other
words, keep the SUBMITTING_PR guard that requires architecture-review,
code-review, bug-scanner and, if githubIssue is present, the latest task-check
to be passed, but modify the IMPLEMENTING guard so it only blocks when the core
3 reviews passed AND (githubIssue is absent OR the latest task-check passed); if
githubIssue exists and task-check failed allow transitioning to IMPLEMENTING.

---

Outside diff comments:
In `@tools/dev-workflow-v2/src/features/workflow/domain/fold.ts`:
- Around line 56-72: The conditional branch that checks stateOverrides ===
undefined is redundant because spreading undefined is a no-op; simplify fold by
removing the if/early return and replace both returns with a single return that
spreads ...state, ...stateOverrides, sets currentStateMachineState to event.to
and preBlockedState to newPreBlockedState (keep the existing newPreBlockedState
computation and the stateOverrides variable), so update the function around
newPreBlockedState/stateOverrides to return one consolidated object instead of
two branches.

---

Duplicate comments:
In `@tools/dev-workflow-v2/src/features/workflow/domain/workflow-events.ts`:
- Around line 128-131: getKnownWorkflowEventTypes currently assumes each
WORKFLOW_EVENT_SCHEMA option exposes schema.shape.type.value (i.e., a flat
z.object with a z.literal), which is fragile for nested discriminated unions or
extended objects; update it to derive the discriminator values from a stable
source-of-truth instead of depending on .shape: either (A) maintain and read
from an explicit constant list of allowed event type strings (use that constant
in workflow-definition.ts and here), or (B) implement a robust extractor that
walks WORKFLOW_EVENT_SCHEMA options to locate the z.literal discriminator
(handling nested unions/extends) and returns those literal values; also add a
unit test that asserts getKnownWorkflowEventTypes() equals the expected list of
discriminators so schema changes fail loudly.