docs(workflow): design and build a new PRD and task planning workflow - #370

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

Inline comments:
In `@tools/dev-workflow-v2/commands/planning-status.md`:
- Around line 10-18: Document explicit deterministic rules for deriving PRD and
architecture artifact paths from planningId (e.g., a formula or template such as
PRD path = `plans/{planningId}/prd.md` and architecture path =
`plans/{planningId}/architecture.md`) and add a clear stage→next-command mapping
table for the command output (list each stage name and the exact next CLI
command string to print). Update the planning-status command contract to
reference the planningId→path templates and the stage-to-next-command mapping so
implementations can deterministically compute and print planning topic, current
stage, PRD path, architecture path, tasks-created flag, next command, and
blockers; reference the planningId variable and the "next command" field in the
output spec when editing the doc.

In `@tools/dev-workflow-v2/commands/start-planning.md`:
- Around line 18-26: The current flow creates the PRD then the marker separately
which can leave an orphaned PRD if the marker write fails; change the
implementation that handles planningId/PRD/marker creation to either persist
both artifacts atomically (write to temp files and rename into place for both
docs/project/planning/<slug>.yml and the PRD path derived from planningId, then
atomically move) or, if atomic rename across those paths isn't possible, write
the marker first (initialized with planningId, stage: prd-drafting,
githubMilestone: null, githubIssuesCreated: false, githubIssueNumbers: []) and
only then write the PRD; additionally, add a failure handler so that if the
second write fails you delete the already-written artifact (cleanup the PRD if
marker failed or delete marker if PRD failed) and surface an error so retries
don't see inconsistent state.

In `@tools/dev-workflow-v2/src/features/workflow/domain/workflow.ts`:
- Around line 240-257: createPr currently only blocks --draft/-d but then calls
this.deps.createPullRequest(createArgs) which can launch an editor if
non-interactive required flags are missing; add a pre-check in createPr (after
parsing via CREATE_PR_ARGS_SCHEMA and before this.deps.createPullRequest) that
verifies the arg array contains either --fill OR (--title or -t) and at least
one of (--body or -b or --body-file) (treat --fill as satisfying both title and
body). If the check fails, return fail(...) with a clear message about required
non-interactive flags (e.g. "create-pr requires --fill or --title (-t) plus
--body/-b/--body-file for non-interactive use") so createPullRequest is only
invoked with deterministic, non-interactive arguments.

In
`@tools/dev-workflow-v2/src/features/workflow/entrypoint/fixtures/workflow-cli-test-fixtures.ts`:
- Around line 133-140: The code currently coerces any non-'FAIL' third tuple
entry to 'PASS', which masks malformed steps; update the validation around the
tuple in the block that references WorkflowStateError, reviewType and verdict
before calling runReviewCommand so that the third element (step[2]) must be
present and strictly equal to either 'PASS' or 'FAIL' — otherwise throw a
WorkflowStateError with a clear message about expected shape ['record-review',
<reviewType>, 'PASS'|'FAIL']; then pass the validated verdict to
runReviewCommand.

In `@tools/dev-workflow-v2/src/features/workflow/entrypoint/workflow-cli.spec.ts`:
- Around line 293-301: The test "blocks when draft flag is provided" should also
assert that no "pr-recorded" event was persisted: after calling runCommand(ctx,
['create-pr', '--draft']) and the existing exit/output assertions, add an
assertion against the test context (ctx) event store/published events (the same
test fixture used by setup()/progressToState()/runCommand()) to ensure there is
no event with type or name "pr-recorded" (e.g., expect the ctx's
persisted/published events not to include any item matching { type:
'pr-recorded' }); place this new assertion immediately after the existing expect
lines so the blocked path verifies no side-effect event was emitted.

In
`@tools/dev-workflow-v2/src/features/workflow/infra/external-clients/github/create-pull-request.ts`:
- Around line 56-69: The current readPullRequestReference function relies on
brittle whitespace parsing; change the gh pr create invocation to include the
machine-readable flag (--json url) and update readPullRequestReference to parse
the JSON output and return the url field (e.g., JSON.parse(createOutput).url),
throwing PullRequestCreationError if parsing fails or url is missing; optionally
keep a robust fallback that extracts the first HTTP(S) URL via regex if JSON
parsing fails to maintain resilience.

In `@tools/dev-workflow-v2/states/reviewing.md`:
- Line 19: Update the checklist item so the CLI invocation for
/dev-workflow-v2:workflow record-review clearly instructs callers to pass a
compacted/one-line JSON string and to wrap it in quotes to avoid shell
splitting; specifically mention the command name "/dev-workflow-v2:workflow
record-review" and the "<review-json>" placeholder and give an explicit example
format such as quoting the JSON (e.g., '\"{...}\"' or using single quotes) or
piping a compacted JSON string, so consumers know to remove newlines/whitespace
and quote the argument to prevent broken invocations.

---

Outside diff comments:
In `@tools/dev-workflow-v2/commands/continue-planning.md`:
- Around line 15-23: The workflow lacks an explicit terminal rule for the
`planning-complete` stage: update the `continue-planning` behavior to treat
`planning-complete` as a terminal/no-op state by adding a clear rule that if the
current stage equals `planning-complete` the command returns immediately (no
transitions, no side-effects) and documents this in the Stage order section next
to `planning-complete` so implementations follow the same stop behavior.

In `@tools/dev-workflow-v2/src/features/workflow/domain/states/submitting-pr.ts`:
- Around line 10-17: The transitionGuard's failure message is stale: when
allowedWorkflowOperations includes 'create-pr' the guard should reference both
paths; update the failure text in transitionGuard (the check on
ctx.state.prNumber that currently calls fail('prNumber not set. Run record-pr
first.')) to mention the new create-pr flow (e.g., "Run record-pr or create-pr
first.") so it accurately instructs users when ctx.state.prNumber is missing;
keep the existing fail()/pass() logic unchanged.

In `@tools/dev-workflow-v2/src/features/workflow/domain/workflow-events.ts`:
- Around line 5-19: KNOWN_WORKFLOW_EVENT_TYPES is missing event types that
WORKFLOW_EVENT_SCHEMA accepts; update the KNOWN_WORKFLOW_EVENT_TYPES array to
include 'architecture-review-completed', 'code-review-completed', and
'bug-scanner-completed' so getKnownWorkflowEventTypes() returns the full
canonical discriminator list; locate the KNOWN_WORKFLOW_EVENT_TYPES constant in
workflow-events.ts and add those three string literals (preserving the as const
typing) so the helper aligns with WORKFLOW_EVENT_SCHEMA.

In
`@tools/dev-workflow-v2/src/features/workflow/infra/persistence/workflow-definition.spec.ts`:
- Around line 11-35: The mock in makeWorkflowDeps defines createPullRequest as
createPullRequest: () => {...} but it should accept the same args as the real
dependency; change the mock signature to createPullRequest: (args) => ({
prNumber: 1, prUrl: 'https://github.com/example/repo/pull/1', isDraft: false })
(and add an explicit parameter type if needed) so the mock matches the
dependency contract used by functions calling createPullRequest.

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

Inline comments:
In `@tools/dev-workflow-v2/commands/start-planning.md`:
- Line 14: Update the sentence that currently reads "Stop if exactly one active
marker exists and tell the user to use `planning-status` or
`continue-planning`." to use a softer tone by replacing "tell the user" with
"ask the user" so it reads "Stop if exactly one active marker exists and ask the
user to use `planning-status` or `continue-planning`." This is the exact line to
change in the start-planning.md content.

In `@tools/dev-workflow-v2/README.md`:
- Around line 39-44: Combine and rephrase the three consecutive "If ..."
sentences into one or two concise sentences to avoid repetition: mention the
three cases (exactly one active marker — commands planning-status and
continue-planning use it; no active marker — those commands stop and
start-planning may create one; more than one active marker — commands stop and
report all) and reference the command names planning-status, continue-planning,
and start-planning so readers can quickly scan the behavior without repeated "If
..." clauses.

In
`@tools/dev-workflow-v2/src/features/workflow/domain/pull-request-description.spec.ts`:
- Around line 36-142: Add a regression test to pull-request-description.spec.ts
that verifies parsePullRequestDescriptionOptions correctly handles a value that
looks like a flag (e.g. pass ['--title', '--description', '--description',
'Actual description', ...other required options...]) so the parser does not
treat the value '--description' as a new option; call
parsePullRequestDescriptionOptions and assert the result is ok: true and that
input.title equals the literal value '--description' while the real
--description option is parsed into input.description (or use
VALID_PULL_REQUEST_DESCRIPTION_INPUT/VALID_CREATE_PR_OPTIONS as a template to
construct expected input). Ensure the new it() case names the intent (e.g.
"handles flag-like values in value positions") and includes all other required
options so parsing succeeds.

In
`@tools/dev-workflow-v2/src/features/workflow/domain/pull-request-description.ts`:
- Around line 184-197: The readRequiredOption function is vulnerable because
indexOf(optionName) can match a value token (e.g. a title equal to
"--description") and return a wrong index; change the lookup to scan tokens for
the option token only (match token === optionName among the argument tokens) and
then validate the next token exists and is not another option (e.g., does not
start with "--") before parsing; update the logic in readRequiredOption to use
this targeted find (or a simple loop over commandTokens) and return a clear
missing/invalid-value error when the next token is absent or looks like an
option.

---

Outside diff comments:
In `@packages/riviere-role-enforcement/skills/plan-review.md`:
- Around line 99-109: Add a concrete, repeatable checklist to Step 6 in
plan-review.md that tells annexers exactly how to detect forbidden
imports/dependencies in pre-implementation plans: enumerate checks for
forbiddenImports (verify any proposed file/module paths against the role's
forbiddenImports list, flag if plan text names or implies imports from those
namespaces), forbiddenDependencies (check for explicit or implied
use-case-to-use-case or repository-to-repository dependencies and flag),
entrypoint/ vs domain/ vs infra/ violations (ensure proposed file
responsibilities map to allowed layer imports per role-selection-guide.md),
aggregate-repository access rules (ensure only command-use-case mentions
aggregate-repository), and consumer-mapping placement (flag presenters/adapters
placed in domain/); for each check require a one-line evidence string from the
plan (quoted text or path) and a binary pass/fail decision so annexers follow a
consistent, repeatable process.
- Around line 184-193: Step 9's "append annex to issue body" is ambiguous and
needs a newline-safe, non-destructive procedure: update the instructions around
the `gh issue view ... > /tmp/issue-body.md` and `gh issue edit ... --body-file
/tmp/issue-body.md` flow to explicitly (1) fetch the current body into a temp
file (keep original intact), (2) ensure exactly one blank line separator is
inserted before the annex (describe using a guaranteed newline/blank-line append
operation to the temp file), (3) append the annex content to the temp file (not
overwrite), and (4) call `gh issue edit --body-file` with that temp file;
reference the commands (`gh issue view`, `/tmp/issue-body.md`, `gh issue edit
--body-file`) in the text so implementers know where to apply the safe append
behavior.
- Around line 110-176: Step 7 is ambiguous about how aggregates should be
represented in the "Proposed Roles" table, causing Step 8's "New aggregate
proposed" blocker to be missed; update the Step 7 template so every aggregate
must appear as a row in "Proposed Roles" with role exactly `aggregate` and an
extra column or note indicating whether that aggregate is already approved
(reference `approvedInstances` entries in `.riviere/roles.ts` and the
`userHasApproved` flag); ensure the Step 7 text explicitly instructs
implementers to list unapproved aggregates as "aggregate" and mark them for user
approval so Step 8's blocker rule (the "New aggregate proposed" check) can
unambiguously detect them.

---

Duplicate comments:
In `@tools/dev-workflow-v2/commands/planning-status.md`:
- Around line 18-20: Clarify the rule that determines "whether tasks have been
created" by updating the doc to state which marker field is authoritative (e.g.,
use githubIssuesCreated === true as the primary signal, or define that
githubIssueNumbers.length > 0 indicates created tasks); explicitly pick one rule
(or define precedence if both exist) and show the exact condition (for example:
"Tasks are considered created when githubIssuesCreated === true; alternatively,
if you use githubIssueNumbers, tasks are created when githubIssueNumbers.length
> 0") so readers know which field drives the output (referencing the
githubIssuesCreated and githubIssueNumbers marker fields).

In
`@tools/dev-workflow-v2/src/features/workflow/infra/external-clients/github/create-pull-request.ts`:
- Around line 47-48: The code currently calls readPullRequest(runGh,
pullRequestUrl) and if that call throws the function hides the validated
pullRequestUrl; wrap the readPullRequest call in a try/catch, and on error throw
or rethrow an Error that includes the original pullRequestUrl (either in the
message or as an error property) so callers can detect the partially-created PR;
update the block around readPullRequestUrl(createOutput) and
readPullRequest(...) accordingly (referencing readPullRequestUrl and
readPullRequest) to preserve and surface pullRequestUrl when the second call
fails.

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

Inline comments:
In `@scripts/archive-prd.sh`:
- Line 34: The sed invocation using sed -i '' is macOS/BSD-specific and breaks
on GNU sed; update both occurrences (the sed call that edits MARKER_FILE and the
similar call around line 49) to use a portable pattern: invoke sed with an
inline backup suffix (e.g., sed -i.bak 's/^stage: .*/stage: planning-complete/'
"$MARKER_FILE") and then remove the .bak file (rm -f "$MARKER_FILE.bak") so the
edit works on both GNU and BSD sed; locate the sed calls in
scripts/archive-prd.sh to apply this change to the commands that modify
MARKER_FILE.

In `@tools/dev-workflow-v2/README.md`:
- Around line 31-88: The Commands section is being closed early because "##
Planning marker" and "## Planning stages" are h2 headings; change those two
headings to h3 (e.g., "### Planning marker" and "### Planning stages") so the
`continue-planning`, `choose-next-task`, `start-implementation`, and `Workflow
(internal)` entries remain nested under `## Commands`; make the same h3 change
for any subheadings that should stay within Commands so AI agents can discover
`continue-planning`, `/dev-workflow-v2:choose-next-task`, and
`/dev-workflow-v2:start-implementation` as command entries.

---

Outside diff comments:
In `@scripts/archive-prd.sh`:
- Around line 37-46: The script currently looks up milestones by the PRD folder
slug ($PRD_NAME); change it to read the githubMilestone field from the marker
(set a new variable like GITHUB_MILESTONE by parsing marker.yml for
githubMilestone) and use that when building the gh API jq selector (replace uses
of $PRD_NAME in the select(.title == ...) with $GITHUB_MILESTONE); if
githubMilestone is missing, fall back to $PRD_NAME, and adjust the echo
warnings/messages to reference the milestone title variable used so failures are
visible and the archival step can reliably close the correct milestone.

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

Inline comments:
In `@tools/dev-workflow-v2/commands/choose-next-task.md`:
- Line 13: The current instructions run separate GH CLI queries for
label:"idea", label:"bug", and label:"tech improvement" which can return the
same issue multiple times; update the aggregation step that collects outputs
from the commands like `gh issue list --search 'label:"idea" no:milestone
state:open' --json number,title,assignees,body,labels'` (and the analogous `bug`
and `tech improvement` queries) to deduplicate results by the issue `number`
before counting or recommending tasks (e.g., merge JSON outputs and filter
unique `number` values using jq or a small script) so each issue is only counted
once.
- Around line 7-10: The discovery logic currently anchored to
docs/project/PRD/*/marker.yml misses markers stored at the canonical location
docs/project/planning/<slug>.yml; update the task-listing instructions so the
command scans the canonical planning-marker path (docs/project/planning/*.yml)
and/or both locations (docs/project/planning/*.yml and
docs/project/PRD/*/marker.yml) before reading githubMilestone from each marker
and then reading docs/project/PRD/<planningId>/delivery.md for folders that have
one (ensure references to scanning, reading githubMilestone, and reading
delivery.md in the choose-next-task command/docs reflect the new path(s)).
- Line 21: Replace occurrences of the inline secret pattern
`GITHUB_TOKEN=<token>` (seen around the examples at lines 12–13, 21, and 109)
with command-substitution or exporting the token to avoid leaking secrets;
specifically, use `GITHUB_TOKEN="$(gh auth token)"` for single-command
invocation or `export GITHUB_TOKEN="$(gh auth token)"` for a session, and update
the examples/instructions that mention `gh auth token` to show these safer forms
instead of pasting raw tokens.

In `@tools/dev-workflow-v2/commands/continue-planning.md`:
- Around line 58-84: The command currently discovers markers under
docs/project/PRD/*/marker.yml but the project contract expects per-topic markers
at docs/project/planning/<slug>.yml; update the discovery logic and
documentation in continue-planning to look for markers in
docs/project/planning/*.yml (matching the slug), keep the same active-marker
check (stage != planning-complete), and when exactly one active marker is found
derive sibling paths from that planning file's slug (use planningId/slug to form
docs/project/PRD/<planningId>/problem-definition.md, /solution-exploration.md,
/PRD.md, /ARCH.md, /delivery.md) so the command aligns with the specified
contract and can locate topic-based markers.

In `@tools/dev-workflow-v2/commands/start-planning.md`:
- Around line 61-65: The template header "Problem Definition: <title>" is
ambiguous about how <title> is derived; update the document text near the
skeleton/template to explicitly state the derivation rule (e.g., "use the
original planning topic", "use a title-cased slug of the topic", or "use an
explicit user-provided title") and give the chosen rule as the canonical
instruction so all generated Problem Definition artifacts are consistent;
reference the template header string "Problem Definition: <title>" when adding
this one-line clarification and an example showing the expected resulting title
form.

In `@tools/dev-workflow-v2/planning-stages/architecture-drafting.md`:
- Around line 417-420: The doc currently references a "Component Archetypes"
section for role archetype names (and uses the `custom:` prefix rule), but that
target section is missing; to fix, add a new "Component Archetypes" subsection
that enumerates allowed archetype names and examples (including the `custom:`
prefix usage and example `custom:bulk-copy-script`), or alternatively update the
note to point to the correct existing section (e.g., "Component naming
guidelines") if one already exists; ensure the note mentioning "role archetypes"
and the `custom:` prefix is synchronized with the new or corrected section so
the rule is resolvable during architecture option authoring.

In `@tools/dev-workflow-v2/planning-stages/parallelization.md`:
- Around line 3-5: The file uses the British spelling "parallelisation" in the
body while the stage/filename is "parallelization"; update all occurrences of
"parallelisation" (lines referencing the legacy note and the second sentence) to
"parallelization" so the content consistently matches the stage name and
improves searchability—ensure you change both instances shown in the diff and
verify no other occurrences remain in the document.

In `@tools/dev-workflow-v2/planning-stages/success-criteria.md`:
- Line 5: The sentence in success-criteria.md implies the document itself
approves the product concept; change the wording to state approval is an outcome
recorded in PRD after stakeholders approve the concept from
solution-exploration.md. Update the line to something like: "Once a product
concept is approved following solution-exploration.md, its success criteria
should be recorded in PRD.md; delivery verification belongs in delivery.md," and
ensure the phrasing clarifies approval is an outcome by stakeholders, not an
action performed by the file.

In `@tools/dev-workflow-v2/planning-stages/task-creation.md`:
- Around line 218-237: The docs currently instruct inlining raw tokens
(GITHUB_TOKEN=<token> ...) which can leak credentials; update the examples to
remove any inline GITHUB_TOKEN usage and instead instruct users to authenticate
via gh (e.g. run gh auth login / gh auth status) and then call gh repo view, gh
api, gh label list and gh label create directly; replace the three example
command lines that start with "GITHUB_TOKEN=<token> gh ..." and the two that
start with "GITHUB_TOKEN=<token> gh label ..." with the same commands but
without the prefixed GITHUB_TOKEN, and add a short note to run gh auth login /
gh auth status beforehand.
- Around line 169-209: The current Step 4 annex (the "## Architectural Annex"
block) does not enforce the required "Relevant Architecture Decisions" payload;
update the task template to include a mandatory "## Relevant Architecture
Decisions" section immediately after the annex header that requires: Decision
Name, Chosen Design, Boundaries (scope), Forbidden Alternatives (what was
rejected), and Verification Checks (how to validate the decision). Modify the
"## Architectural Annex" template in the Step 4 content so the new "Relevant
Architecture Decisions" subsection is present and clearly labeled as required
(e.g., a table or bulleted fields) and ensure any instructions around consulting
`.riviere/*` remain, so implementers must fill these four fields when creating
the issue.

In `@tools/dev-workflow-v2/planning-stages/terminology.md`:
- Line 5: Update the guidance in terminology.md so it explicitly mandates using
the canonical glossary file for all term definitions referenced in
solution-exploration.md, PRD.md, ARCH.md, delivery.md and any generated tasks:
change the routing rule on line 5 to instruct authors to add or update terms
only in docs/architecture/domain-terminology/contextive/definitions.glossary.yml
(and not create standalone terminology stages), and include a short note in
terminology.md pointing to that file as the single source of truth for domain
terms to prevent divergence.

---

Outside diff comments:
In `@docs/project/PRD/riviere-extraction-workflows-v1/solution-exploration.md`:
- Around line 84-99: The paragraph that currently claims workflows can run
"including during local exploration, in CI, or whenever the code changes"
conflicts with the later success signal that "V1 does not require a hard success
goal that workflows run in CI"; to align scope, edit the sentence in the block
starting "After this PRD, users can define a workflow..." (the sentence
containing "including during local exploration, in CI, or whenever the code
changes") to remove or qualify "in CI" (e.g., "including during local
exploration or whenever the code changes" or "including local exploration and
optionally in CI") so the wording matches the explicit V1 scope in the
"Previously captured success signals" section.