tracker: categorize acceptance criteria in generated requirements guide

[agent-era-ai]

Summary

• Extends the requirements-stage prompt in defaultStageFileContent so the generated stage guide tells the agent to group acceptance criteria under ### sub-headings when they span multiple concerns, with continuous numbering across categories (so any criterion can be referenced as "AC #N").
• Agent's judgment when to group; no numeric threshold, no fixed vocabulary, no new stage setting.
• Adds a parametrised unit test covering all six combinations of style ∈ {interview, draft_first} × detail ∈ {minimal, standard, thorough}.

Test plan

• npm test — 786/786 passing (148 in tests/unit/tracker.test.ts, including 6 new parametrised cases)
• npm run typecheck — clean
• Manually previewed the generated requirements stage guide; the new instruction renders inline

🤖 Generated with Claude Code

[claude]

Code Review — PR #237: Categorize acceptance criteria in requirements guide

Overview

A focused, well-scoped prompt change. The production diff is one line in TrackerService.ts:1483; everything else is test coverage and tracker documentation. The instruction is clear, the decisions are documented, and the existing AC list in ralph-phase-automation validates the pattern organically. Low blast radius: only the text the requirements-stage agent reads changes — no schema, no exit criteria, no UI.

---

Code quality

src/services/TrackerService.ts

• The replacement string is long enough to hurt readability (~220 chars inline). Consider a named const above the sections array to make the diff easier to scan in future reviews:

  const acInstruction =
    '- **Acceptance criteria** — numbered testable conditions. ' +
    'When the criteria span multiple distinct concerns, group them under `###` sub-headings …';
  const sections: string[] = [
    '- **Problem** + **Why** …',
    '- **Summary** …',
    acInstruction,
  ];

  Not a blocking issue — the current form works — but worth considering.

tests/unit/tracker.test.ts

• The parametrised test.each over all six style × detail combinations is the right call. No issues there.

• One assertion is vacuous: expect(content).toMatch(/###/).
  defaultStageFileContent always appends renderStageProtocol, which emits ### Input mode: and ### Gate on advance: regardless of the new instruction (see TrackerService.ts:1404,1409). The /###/ check would pass even if the AC categorization line were removed entirely, so it doesn't guard what it intends to.
  The other four assertions (/sub-heading/i, /concern/i, /continuous/i, /AC #N/) are specific and meaningful — they would catch regressions. The safest fix is to drop the /###/ assertion (the others cover the intent) or replace it with something scoped to the AC instruction, e.g.:

  expect(content).toMatch(/acceptance criteria.*sub-heading/is);

---

Potential issues / risks

• None in production. The change is advisory text; it can't break existing behaviour.
• The instruction deliberately gives the agent full latitude on naming categories. That's the right call for now, but if future reviews show wide variance in output quality, a few example names in the prompt (commented out as examples rather than constraints) might help without locking in vocabulary.

---

Security

No concerns. No new code paths, no external I/O.

Performance

No concerns. Pure string template change.

---

Summary

Approve with one minor note: the /###/ test assertion should be removed or tightened — it passes vacuously and gives false confidence. Everything else is solid.

[claude]

Code Review — PR #237: categorize acceptance criteria in generated requirements guide

Overview

A focused, well-scoped change: one line replaced in TrackerService.ts and six parametrized unit tests added. The intent is clear, the approach fits the architecture, and the test suite passes cleanly. Most of the additions (288 lines) are tracker documents, not source code.

---

Issues

Blocker — AC #3 not implemented

The requirements.md for this item explicitly calls for a worked example in the prompt:

AC #3. The instruction includes a tiny worked example showing two H3 groups with continuous numbering — concise enough that it fits in the generated stage guide without bloating it.

The new prompt string contains only prose; there is no worked example:

'- **Acceptance criteria** — numbered testable conditions. When the criteria span multiple
distinct concerns, group them under `###` sub-headings …'

The implementation.md also claims the change carries "categorization guidance + worked example," but the diff shows no example. The tests don't assert for one either, so this gap isn't caught by CI.

If the worked example was intentionally dropped, the requirements doc (AC #3) and the implementation doc should be updated to reflect that decision. If it should be present, the prompt string needs amending and a test assertion added.

---

Suggestions (non-blocking)

Line length — TrackerService.ts:1481

The new string is ~260 characters on a single line. It is harder to read and edit than a template literal or a string built by array join (which is the pattern the surrounding code already uses for sections.push(...)). Splitting across lines would also make future prompt edits easier to review in diffs.

Test assertions are broad

expect(content).toMatch(/acceptance criteria.*sub-heading/is);

The s (dotAll) flag means this will pass even if "acceptance criteria" and "sub-heading" appear many lines apart in unrelated contexts. Tighter patterns (e.g. verifying the phrases are within the same sentence or at least the same paragraph) would make the tests more meaningful as regression guards, though this is low priority for a prompt-text test.

---

Positive notes

• Minimal blast radius — exactly one prompt string changed, no schema or config changes.
• Parametrized test.each over all six style × detail combinations is the right approach and good coverage of the detail-independence requirement (AC Add comprehensive e2e testing framework with GitHub Actions #6).
• Existing 780 passing tests are unchanged; no regressions.
• No new settings, no new exit criteria, no parser — matches the "advisory only" design decision.
• Tracker documents are thorough and provide good context for the trade-off decisions.

---

Summary

The implementation is conceptually correct and appropriately narrow. The main thing to resolve before merging is the missing worked example (AC #3) — either add it to the prompt and a corresponding test assertion, or explicitly record the decision to omit it in the tracker docs and remove AC #3 from the requirements.

[claude]

Code Review — PR #237: categorize acceptance criteria in generated requirements guide

Overview

A single-line production change to the requirements stage prompt template in TrackerService.ts, extending the Acceptance criteria bullet to instruct the agent to group criteria under ### sub-headings when they span multiple concerns and to number continuously across categories. Six parametrized unit tests and three tracker documentation files round out the PR.

---

Code quality and best practices ✅

• Minimal, scoped change. One line of production code changed, exactly as the AC specifies. The implementation has no unintended reach into other stages or settings.
• Parametrized tests are well-structured. test.each(STYLES.flatMap(...)) is clean and readable; the as const casts preserve literal types cleanly.
• No new settings surface. Deliberately omitting a STAGE_OPTION_DEFS knob keeps the prompt surface small while making the behavior available to everyone — a good default-before-escape-hatch decision.
• Tracker docs are thorough. The key decisions section in implementation.md documents the trade-offs (numbering scheme, vocabulary choice, detail independence) in a way that will make future changes easier to reason about.

---

Potential issues / minor observations

1. s (dotAll) flag is unnecessary on the regex (tracker.test.ts, new test block)

expect(content).toMatch(/acceptance criteria.*sub-heading/is);

The s flag lets . match newlines, which makes this broader than needed — the prompt string for that bullet has no embedded newlines, so . already matches everything within it. The flag won't cause a false positive here, but it's a mild readability issue (suggests multi-line content that isn't there). Consider dropping s, leaving just /acceptance criteria.*sub-heading/i.

2. Dense instruction string (TrackerService.ts:1483)

The new bullet is ~270 chars in a single line. It reads clearly in context, but if the instruction needs further iteration the diff becomes hard to parse. This follows the existing convention for this file, so it's not a blocker — just worth flagging if the team ever considers extracting prompt strings to a dedicated template layer.

3. detail variant adds signal but not coverage (minor)

Because the instruction is unconditional, testing across all three detail values mainly confirms there's no accidental early-return or string-replacement that would drop the instruction for a specific detail level. That's worth having, but it means the test suite doesn't cover what happens when detail = 'minimal' omits the instruction (which it shouldn't, per AC #5). The test is correct as written — just noting the reasoning in case anyone wonders why all six cases look identical.

---

Performance considerations ✅

No runtime impact. The change only affects the string returned by a template function at stage-guide generation time.

Security considerations ✅

No new inputs, no new parsing, no exec paths affected. The instruction is advisory text for an LLM agent.

Test coverage ✅

• All six style × detail combinations covered.
• Three assertions per case: categorization instruction, "continuous" numbering, literal AC #N reference.
• 786/786 tests pass; typecheck clean.
• The test correctly targets the public API (defaultStageFileContent) rather than the private string constant, which is the right level of abstraction.

---

Summary

This is a well-executed, low-risk change. The one actionable suggestion is dropping the unnecessary s flag from the regex. Everything else is clean — the scope is tight, the tests are meaningful, and the documentation is unusually good for a one-line edit.

[claude]

Code Review — PR #237: Categorize acceptance criteria in requirements guide

Overview

This PR makes a single-point prompt change in defaultStageFileContent to instruct the requirements-stage agent to group acceptance criteria under ### sub-headings (by its own judgment) with continuous numbering across categories. It backs this up with a parametrized unit test across all 6 style × detail combinations. The implementation is minimal, focused, and matches the spec in requirements.md precisely.

---

Code Quality

src/services/TrackerService.ts (+1 line)

The change is surgical and correct. The new instruction string reads clearly and is unambiguous about the three rules: group by concern when helpful, number continuously, invent category names freely.

One minor observation: the new bullet is a long single-line string (~300 chars). It's within acceptable range for an inline prompt string, but if this pattern of instructions grows further it may be worth factoring the AC instruction into a named constant or a short helper for readability. Not a blocker at this size.

tests/unit/tracker.test.ts (+21 lines)

The test.each structure is well-suited here and correctly covers all 6 parameter combinations. A couple of observations on the assertions:

• /acceptance criteria.*sub-heading/i — relies on .* matching within a single line (no s/dotAll flag). This is safe because both words appear in the same bullet string, but it's worth noting as a subtle assumption.
• /concern/i — very broad; the word "concern" could appear elsewhere in the template for unrelated reasons. A slightly tighter pattern like /distinct concern/i or /span.*concern/i would be more robust and better capture the intent.
• /AC #N/ — good literal check; catches any accidental reformulation.
• /continuous/i — appropriate.

Overall the test suite is a good proxy for the expected behaviour and is clearly written.

---

Correctness

• ✅ Instruction is applied unconditionally across all style and detail values, which matches AC Disable highlighting when agent is working or thinking #5.
• ✅ evaluateExitCriteria is untouched — the instruction stays advisory.
• ✅ No schema changes, no new settings surface.
• ✅ npm test reports 786/786 passing; typecheck is clean.

---

Potential Issues

None blocking. The one thing worth a second look before merging:

The test assertion /concern/i is weak enough that it could survive a future accidental deletion of the real intent from the prompt if some other part of the template happened to contain the word "concern." Tightening it to /multiple distinct concern/i or similar would make the intent of the test explicit and future-proof it.

---

Security / Performance

No concerns. This is a string constant change; no runtime logic, no external I/O, no parsing.

---

Summary

Clean, focused PR. The core change is correct and fully tested. The only suggestion worth addressing is tightening the /concern/i regex to something more specific — everything else is in good shape.

[claude]

Code Review — PR #237: categorize acceptance criteria in requirements guide

Overview

This is a well-scoped, surgical change. It extends one bullet in the requirements stage prompt template so the generating agent groups acceptance criteria under ### sub-headings by judgment, with continuous numbering across categories. No schema changes, no new settings, no retroactive impact. All 786 tests pass and typecheck is clean.

---

Code Quality

src/services/TrackerService.ts (line ~1483)

The single-line edit is clean and the intent is unambiguous. The new instruction is verbose, but that is appropriate here — it is a prose template that an AI agent reads, so precision matters more than brevity. No issues.

One minor readability note: the replacement string is now quite long (220+ characters in a single array element). This is not wrong — splitting it would fragment the logical unit — but if the file has a linting rule for line length, worth verifying it passes (the diff looks fine as-is since tests pass).

tests/unit/tracker.test.ts

The parametrized approach (test.each over all six style × detail combinations) is exactly right. Assertions cover the three key phrases:

• acceptance criteria.*sub-heading — confirms the grouping instruction is present
• distinct concern — confirms the conditional trigger is included
• continuous — confirms the numbering scheme is specified
• AC #N — confirms the reference convention is stated literally

One observation: the regex /acceptance criteria.*sub-heading/i uses . which does not match \n in JavaScript by default. If the long prompt string were ever reformatted across multiple lines (e.g., via a template literal), this assertion would silently start failing. Currently the string is a single concatenated element so there's no newline, but it might be worth using /acceptance criteria[\s\S]*sub-heading/i or splitting into two separate matchers (/acceptance criteria/i + /sub-heading/i) to be robust to future reformatting. Not a blocker — just a fragility to be aware of.

---

Test Coverage

• All six style × detail permutations covered ✓
• Existing suite (780 prior tests) unaffected ✓
• No new code paths were introduced (the change is purely textual), so no additional test types are needed.

---

Performance & Security

No concerns. This is a static string template — no runtime cost, no external I/O, no user-controlled input reaches the modified path.

---

Tracker Files (tracker/items/categorize-acceptance-criteria/)

The notes.md, requirements.md, and implementation.md files look correct as tracker artifacts. One stale detail:

implementation.md — "Notes for cleanup" section says "Nothing committed yet"

This was accurate when the agent wrote it, but it is now incorrect since the changes are committed on this branch. Minor, but worth a one-line fix before merge if the tracker's implementation.md files are meant to remain accurate post-ship.

---

Summary

Area	Status
Correctness	✓ No bugs introduced
Conventions	✓ Follows project patterns
Tests	✓ Parametrized; covers all combos
Performance	✓ No impact
Security	✓ No concerns
Scope	✓ Minimal and focused

Verdict: Looks good to merge. The one actionable suggestion is the implementation.md stale note and the optional regex robustness improvement in the test — neither is a blocker.