refactor(workflows): extract discuss-phase modes/templates/advisor for progressive disclosure (closes #2551)#2607
Merged
trek-e merged 6 commits intogsd-build:mainfrom Apr 23, 2026
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (27)
📝 WalkthroughWalkthroughRefactors discuss-phase into a thin dispatcher that lazy-loads per-invocation mode docs, templates, and references; extracts modes, advisor logic, templates, and checkpoints into separate files; and adds CI tests enforcing workflow size budgets and progressive-disclosure constraints. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Invoker
participant Dispatcher as discuss-phase.md (Dispatcher)
participant Mode as modes/<mode>.md
participant Template as templates/*
User->>Dispatcher: invoke /gsd:discuss-phase with flags
Dispatcher->>Dispatcher: parse $ARGUMENTS, select active mode(s)
Dispatcher->>Mode: lazily read modes/<mode>.md (if required)
Dispatcher->>Template: lazily read templates/context.md or discussion-log.md (when write/git_commit)
Mode-->>Dispatcher: mode-specific steps / decisions
Dispatcher-->>User: prompts, questions, or auto-advance per mode
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
tests/chain-flag-plan-phase.test.cjs (1)
21-24: Fail fast ifmodes/chain.mdis missing.Using
existsSyncas a silent filter weakens regression coverage for the#2551split. This test should assert the extracted chain-mode file exists.✅ Suggested hardening
const discussChainPath = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'discuss-phase', 'modes', 'chain.md'); const readDiscuss = () => { - const parts = [discussPath, discussChainPath].filter(p => fs.existsSync(p)); + assert.ok(fs.existsSync(discussChainPath), 'discuss-phase/modes/chain.md should exist after `#2551` split'); + const parts = [discussPath, discussChainPath]; return parts.map(p => fs.readFileSync(p, 'utf8')).join('\n'); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/chain-flag-plan-phase.test.cjs` around lines 21 - 24, The test silently skips the chain-mode file because readDiscuss filters out missing paths; change it to fail fast by asserting the chain file exists before reading: in the readDiscuss helper (and where discussPath/discussChainPath are defined) check fs.existsSync(discussChainPath) and throw or use an assertion with a clear message that the chain-mode file is missing (e.g., "modes/chain.md not found"), then proceed to read both files (you can still check discussPath optionally) so the test reliably ensures the extracted chain-mode file is present.tests/bug-2549-2550-2552-discuss-phase-context.test.cjs (1)
32-35: Assertscout-codebase.mdexists in this regression suite.Given this PR explicitly extracts scout guidance into
references/scout-codebase.md, the test should fail if that file disappears.✅ Suggested hardening
test('discuss-phase.md source exists', () => { assert.ok(fs.existsSync(DISCUSS_PHASE), 'discuss-phase.md must exist'); + assert.ok(fs.existsSync(SCOUT_REF), 'references/scout-codebase.md must exist after `#2551` extraction'); src = readDiscussContext(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bug-2549-2550-2552-discuss-phase-context.test.cjs` around lines 32 - 35, The test "discuss-phase.md source exists" currently only asserts DISCUSS_PHASE exists and calls readDiscussContext(); add an assertion that the extracted scout guidance file exists as well by checking fs.existsSync for the scout guidance path used by readDiscussContext (e.g., references/scout-codebase.md or the constant/variable that represents it), so the test fails if scout-codebase.md is removed—update the test to assert.ok(fs.existsSync(SCOUT_CODEBASE || 'references/scout-codebase.md'), 'scout-codebase.md must exist') before calling readDiscussContext().tests/discuss-checkpoint.test.cjs (1)
22-27: Don’t silently skip missing split checkpoint docs.Using
filter(fs.existsSync)can hide regressions when a required extracted file disappears. Prefer explicit existence asserts, then read all files.Suggested tightening
function readAll() { - return [workflowPath, defaultModePath, checkpointTplPath] - .filter(p => fs.existsSync(p)) - .map(p => fs.readFileSync(p, 'utf8')) - .join('\n'); + for (const p of [workflowPath, defaultModePath, checkpointTplPath]) { + assert.ok(fs.existsSync(p), `Required discuss-phase checkpoint source missing: ${p}`); + } + return [workflowPath, defaultModePath, checkpointTplPath] + .map(p => fs.readFileSync(p, 'utf8')) + .join('\n'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/discuss-checkpoint.test.cjs` around lines 22 - 27, The readAll function currently filters out missing files with filter(p => fs.existsSync(p)), which can hide regressions; instead, explicitly assert each required path (workflowPath, defaultModePath, checkpointTplPath) exists (using fs.existsSync or fs.statSync) and throw or fail the test with a clear message if any are missing, then read all three files and join their contents—update the readAll function to perform these existence checks before calling fs.readFileSync so missing split checkpoint docs fail loudly.get-shit-done/workflows/discuss-phase/modes/advisor.md (1)
51-56: Consider clarifying priority when multiple signals match.The
NON_TECHNICAL_OWNERdetection lists four signals with "ANY of the following" but doesn't specify behavior if signals conflict (e.g.,explanation_depth: practical-detailedwithout a technical modifier, but also having a technical background elsewhere). This likely won't cause issues in practice but could be clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@get-shit-done/workflows/discuss-phase/modes/advisor.md` around lines 51 - 56, The guidance for setting NON_TECHNICAL_OWNER is ambiguous about conflicts between signals (e.g., learning_style: guided, frustration_triggers containing "jargon", explanation_depth values) so update the spec to define a clear precedence and override behavior: state which signals are highest priority (for example, explicit owner background/technical_flag overrides inferred signals), whether multiple matches should short-circuit or be aggregated, and add how to treat contradictory explanation_depth values (e.g., a technical modifier on explanation_depth always wins). Reference and update the terms NON_TECHNICAL_OWNER, learning_style, frustration_triggers, and explanation_depth in the document so implementers know the deterministic tie-breaker rule.get-shit-done/workflows/discuss-phase.md (1)
345-365: Clarify overlay combination rules in the discuss_areas step.The universal rules section mentions overlays (
--text,--batch,--analyze) can combine with the active mode, but doesn't specify what happens if multiple overlays are passed together (e.g.,--batch --analyze). Consider documenting the precedence or whether they stack.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@get-shit-done/workflows/discuss-phase.md` around lines 345 - 365, The discuss_areas step's overlay rules are ambiguous about multiple overlays; update the discuss_areas documentation to clearly state whether overlays stack or have precedence (e.g., define that overlays are combinable and apply in a defined order such as --analyze -> --batch -> --text, or declare that only one overlay may be used and additional flags are errors), and add a short example showing the result of combining two overlays (e.g., "--batch --analyze produces grouped questions with a pre-question trade-off table"); edit the discuss_areas section and the referenced mode overlay list to include the explicit rule, mention the exact overlays (--text, --batch, --analyze) and the canonical behavior when combined, and update any wording in workflows/discuss-phase to reference the chosen precedence/stacking semantics.get-shit-done/references/scout-codebase.md (1)
32-40: Verify fallback grep pattern syntax for cross-platform compatibility.The no-maps fallback uses
grep -rl "{term1}\|{term2}"which relies on basic regex alternation. This syntax works with GNU grep but may behave differently on BSD grep (macOS). Consider using-Efor extended regex or documenting the GNU grep requirement.🔧 Suggested pattern for better portability
-2. `grep -rl "{term1}\|{term2}" src/ app/ --include="*.ts" ...` and `ls` +2. `grep -rlE "{term1}|{term2}" src/ app/ --include="*.ts" ...` and `ls`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@get-shit-done/references/scout-codebase.md` around lines 32 - 40, The grep alternation pattern used in the no-maps fallback ("grep -rl \"{term1}\\|{term2}\"") is not portable to BSD grep; update the fallback to use a portable pattern or explicitly enable extended regex by using the -E flag (i.e., "grep -rEl ...") or document that GNU grep is required; locate the logic that builds the search command for the `.planning/codebase/*.md` fallback and change the constructed command to include -E (or switch to a POSIX-compatible approach) so the search behaves consistently across Linux and macOS.get-shit-done/workflows/discuss-phase/modes/default.md (1)
24-24: Add language tags to fenced examples to satisfy Markdown lint.Line 24, Line 43, Line 52, Line 101, and Line 112 use unlabeled fenced blocks (
MD040). Add a language (e.g.,text) to keep docs lint-clean.Proposed diff
-``` +```text Let's talk about [Authentication Strategy]. @@ With that context: How should users authenticate? -``` +``` - ``` + ```text Let's talk about [Area]. - ``` + ``` - ``` + ```text "How should posts be displayed?" - Cards (reuses existing Card component — consistent with Messages) - List (simpler, would be a new pattern) - Timeline (needs new Timeline component — none exists yet) - ``` + ``` -``` +```text I notice competing priorities here — {option_A} optimizes for {goal_A} while {option_B} optimizes for {goal_B}. @@ Want me to think through the tradeoffs before we lock this in? [Yes, analyze] / [No, decision made] -``` +``` -``` +```text "[Feature] sounds like a new capability — that belongs in its own phase. I'll note it as a deferred idea. @@ Back to [current area]: [return to current question]" -``` +```Also applies to: 43-43, 52-52, 101-101, 112-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@get-shit-done/workflows/discuss-phase/modes/default.md` at line 24, The Markdown contains several unlabeled fenced code blocks (e.g., the blocks starting with "Let's talk about [Authentication Strategy].", "Let's talk about [Area].", the block containing ""How should posts be displayed?"", the paragraph beginning "I notice competing priorities here — {option_A}...", and the block starting ""[Feature] sounds like a new capability — that belongs in its own phase.") that trigger MD040; add a language tag (for example, text) to each opening triple-backtick fence so they become ```text to satisfy the linter while preserving content.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/INVENTORY.md`:
- Line 268: The document header "## References (51 shipped)" and the later
in-text note "50 top-level references" are inconsistent; update the in-text
count to match the header (change "50 top-level references" to "51 top-level
references") or adjust both to a single correct number so the document is
consistent; search for the exact phrase "50 top-level references" and update it
to "51 top-level references" (or vice versa) to ensure "## References (51
shipped)" and the internal note match.
In `@get-shit-done/workflows/discuss-phase/modes/analyze.md`:
- Around line 17-29: The fenced code block that contains the trade-off table and
the "How should users authenticate?" prompt is missing a language tag and
triggers markdownlint MD040; update the opening triple-backtick for that block
(the block around the table and the final question) to include a language label
(for example "markdown") so the fence reads ```markdown to silence MD040 and
keep rendering semantics intact.
In `@get-shit-done/workflows/discuss-phase/modes/auto.md`:
- Around line 10-13: The documentation is inconsistent: the auto-mode behavior
for the check_existing step is described as auto-selecting "Skip" in this file
but the parent discuss-phase.md says auto-select "Update it"; reconcile and pick
one behavior, then update the text in modes/auto.md (the description for
**`check_existing`**) to match the parent policy (or update the parent if the
local file is correct). Ensure the chosen behavior is consistently described as
either "Skip" or "Update it" and include the resulting action (e.g., "load
existing context and continue to analyze_phase" if choosing Update it) plus a
note that decisions are logged so users can audit.
In `@get-shit-done/workflows/discuss-phase/modes/text.md`:
- Around line 28-45: The markdown code fences in the examples are unlabeled
which triggers markdownlint MD040; update both fenced blocks to include a
language label (e.g., "text") so the first block containing the
AskUserQuestion(...) snippet and the second human-readable "Layout — How should
posts be displayed?" block are changed from ``` to ```text; ensure both fenced
blocks use the same label and nothing else is altered in the content.
In `@get-shit-done/workflows/discuss-phase/templates/context.md`:
- Around line 134-135: Replace the hardcoded footer literals "*Phase: XX-name*"
and "*Context gathered: [date]*" with the template variables declared above (use
the exact placeholder names used in the template header) so the footer prints
the real phase and date values instead of the sample text; update the lines that
render the footer (*Phase: ...* and *Context gathered: ...*) to reference those
declared placeholders rather than "XX-name" and "[date]".
In `@tests/workflow-size-budget.test.cjs`:
- Around line 300-308: The test currently checks
inlineDiscussionLogSignal.test(parent) then computes occurrences and asserts
occurrences === 0 which is contradictory; update the assertion to allow one
example reference by replacing the assert in that block to assert.ok(occurrences
<= 1, `Parent discuss-phase.md still contains the inline DISCUSSION-LOG.md table
— that block must move to workflows/discuss-phase/templates/discussion-log.md.`)
(or alternatively remove the surrounding if and assert occurrences <= 1
unconditionally); reference inlineDiscussionLogSignal, parent, occurrences and
the assert.ok call to locate the change.
---
Nitpick comments:
In `@get-shit-done/references/scout-codebase.md`:
- Around line 32-40: The grep alternation pattern used in the no-maps fallback
("grep -rl \"{term1}\\|{term2}\"") is not portable to BSD grep; update the
fallback to use a portable pattern or explicitly enable extended regex by using
the -E flag (i.e., "grep -rEl ...") or document that GNU grep is required;
locate the logic that builds the search command for the
`.planning/codebase/*.md` fallback and change the constructed command to include
-E (or switch to a POSIX-compatible approach) so the search behaves consistently
across Linux and macOS.
In `@get-shit-done/workflows/discuss-phase.md`:
- Around line 345-365: The discuss_areas step's overlay rules are ambiguous
about multiple overlays; update the discuss_areas documentation to clearly state
whether overlays stack or have precedence (e.g., define that overlays are
combinable and apply in a defined order such as --analyze -> --batch -> --text,
or declare that only one overlay may be used and additional flags are errors),
and add a short example showing the result of combining two overlays (e.g.,
"--batch --analyze produces grouped questions with a pre-question trade-off
table"); edit the discuss_areas section and the referenced mode overlay list to
include the explicit rule, mention the exact overlays (--text, --batch,
--analyze) and the canonical behavior when combined, and update any wording in
workflows/discuss-phase to reference the chosen precedence/stacking semantics.
In `@get-shit-done/workflows/discuss-phase/modes/advisor.md`:
- Around line 51-56: The guidance for setting NON_TECHNICAL_OWNER is ambiguous
about conflicts between signals (e.g., learning_style: guided,
frustration_triggers containing "jargon", explanation_depth values) so update
the spec to define a clear precedence and override behavior: state which signals
are highest priority (for example, explicit owner background/technical_flag
overrides inferred signals), whether multiple matches should short-circuit or be
aggregated, and add how to treat contradictory explanation_depth values (e.g., a
technical modifier on explanation_depth always wins). Reference and update the
terms NON_TECHNICAL_OWNER, learning_style, frustration_triggers, and
explanation_depth in the document so implementers know the deterministic
tie-breaker rule.
In `@get-shit-done/workflows/discuss-phase/modes/default.md`:
- Line 24: The Markdown contains several unlabeled fenced code blocks (e.g., the
blocks starting with "Let's talk about [Authentication Strategy].", "Let's talk
about [Area].", the block containing ""How should posts be displayed?"", the
paragraph beginning "I notice competing priorities here — {option_A}...", and
the block starting ""[Feature] sounds like a new capability — that belongs in
its own phase.") that trigger MD040; add a language tag (for example, text) to
each opening triple-backtick fence so they become ```text to satisfy the linter
while preserving content.
In `@tests/bug-2549-2550-2552-discuss-phase-context.test.cjs`:
- Around line 32-35: The test "discuss-phase.md source exists" currently only
asserts DISCUSS_PHASE exists and calls readDiscussContext(); add an assertion
that the extracted scout guidance file exists as well by checking fs.existsSync
for the scout guidance path used by readDiscussContext (e.g.,
references/scout-codebase.md or the constant/variable that represents it), so
the test fails if scout-codebase.md is removed—update the test to
assert.ok(fs.existsSync(SCOUT_CODEBASE || 'references/scout-codebase.md'),
'scout-codebase.md must exist') before calling readDiscussContext().
In `@tests/chain-flag-plan-phase.test.cjs`:
- Around line 21-24: The test silently skips the chain-mode file because
readDiscuss filters out missing paths; change it to fail fast by asserting the
chain file exists before reading: in the readDiscuss helper (and where
discussPath/discussChainPath are defined) check fs.existsSync(discussChainPath)
and throw or use an assertion with a clear message that the chain-mode file is
missing (e.g., "modes/chain.md not found"), then proceed to read both files (you
can still check discussPath optionally) so the test reliably ensures the
extracted chain-mode file is present.
In `@tests/discuss-checkpoint.test.cjs`:
- Around line 22-27: The readAll function currently filters out missing files
with filter(p => fs.existsSync(p)), which can hide regressions; instead,
explicitly assert each required path (workflowPath, defaultModePath,
checkpointTplPath) exists (using fs.existsSync or fs.statSync) and throw or fail
the test with a clear message if any are missing, then read all three files and
join their contents—update the readAll function to perform these existence
checks before calling fs.readFileSync so missing split checkpoint docs fail
loudly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8118747e-73cd-4b4a-91aa-ea62013aafe9
📒 Files selected for processing (25)
CONTRIBUTING.mddocs/ARCHITECTURE.mddocs/INVENTORY-MANIFEST.jsondocs/INVENTORY.mdget-shit-done/references/scout-codebase.mdget-shit-done/workflows/discuss-phase.mdget-shit-done/workflows/discuss-phase/modes/advisor.mdget-shit-done/workflows/discuss-phase/modes/all.mdget-shit-done/workflows/discuss-phase/modes/analyze.mdget-shit-done/workflows/discuss-phase/modes/auto.mdget-shit-done/workflows/discuss-phase/modes/batch.mdget-shit-done/workflows/discuss-phase/modes/chain.mdget-shit-done/workflows/discuss-phase/modes/default.mdget-shit-done/workflows/discuss-phase/modes/power.mdget-shit-done/workflows/discuss-phase/modes/text.mdget-shit-done/workflows/discuss-phase/templates/checkpoint.jsonget-shit-done/workflows/discuss-phase/templates/context.mdget-shit-done/workflows/discuss-phase/templates/discussion-log.mdtests/agent-frontmatter.test.cjstests/bug-2549-2550-2552-discuss-phase-context.test.cjstests/chain-flag-plan-phase.test.cjstests/discuss-checkpoint.test.cjstests/discuss-phase-power.test.cjstests/thinking-partner.test.cjstests/workflow-size-budget.test.cjs
Comment on lines
+300
to
+308
| const inlineDiscussionLogSignal = /\| Option \| Description \| Selected \|/; | ||
| if (inlineDiscussionLogSignal.test(parent)) { | ||
| // Allowed only if the parent shows it as a one-line example reference; flag if duplicated | ||
| const occurrences = (parent.match(/\| Option \| Description \| Selected \|/g) || []).length; | ||
| assert.ok(occurrences === 0, | ||
| `Parent discuss-phase.md still contains the inline DISCUSSION-LOG.md table — ` + | ||
| `that block must move to workflows/discuss-phase/templates/discussion-log.md.`); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Test assertion logic issue: occurrences === 0 will always pass when the regex doesn't match.
The test enters the if block only when the signal is found (occurrences >= 1), then asserts occurrences === 0, which will always fail. The logic should either assert occurrences <= 1 (allowing one example reference) or remove the conditional entirely.
🐛 Proposed fix
test('parent does not leak per-mode bodies inline (would defeat extraction)', () => {
const parent = fs.readFileSync(path.join(WORKFLOWS_DIR, 'discuss-phase.md'), 'utf-8');
- // Heuristic: the parent should not contain the full DISCUSSION-LOG.md template body
- // (extracted to templates/discussion-log.md) — that's the heaviest single block.
- // Look for unique strings that ONLY appear in the original inline template.
const inlineDiscussionLogSignal = /\| Option \| Description \| Selected \|/;
- if (inlineDiscussionLogSignal.test(parent)) {
- // Allowed only if the parent shows it as a one-line example reference; flag if duplicated
- const occurrences = (parent.match(/\| Option \| Description \| Selected \|/g) || []).length;
- assert.ok(occurrences === 0,
- `Parent discuss-phase.md still contains the inline DISCUSSION-LOG.md table — ` +
- `that block must move to workflows/discuss-phase/templates/discussion-log.md.`);
- }
+ const occurrences = (parent.match(inlineDiscussionLogSignal) || []).length;
+ assert.ok(occurrences === 0,
+ `Parent discuss-phase.md still contains the inline DISCUSSION-LOG.md table — ` +
+ `that block must move to workflows/discuss-phase/templates/discussion-log.md.`);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const inlineDiscussionLogSignal = /\| Option \| Description \| Selected \|/; | |
| if (inlineDiscussionLogSignal.test(parent)) { | |
| // Allowed only if the parent shows it as a one-line example reference; flag if duplicated | |
| const occurrences = (parent.match(/\| Option \| Description \| Selected \|/g) || []).length; | |
| assert.ok(occurrences === 0, | |
| `Parent discuss-phase.md still contains the inline DISCUSSION-LOG.md table — ` + | |
| `that block must move to workflows/discuss-phase/templates/discussion-log.md.`); | |
| } | |
| }); | |
| const inlineDiscussionLogSignal = /\| Option \| Description \| Selected \|/; | |
| const occurrences = (parent.match(inlineDiscussionLogSignal) || []).length; | |
| assert.ok(occurrences === 0, | |
| `Parent discuss-phase.md still contains the inline DISCUSSION-LOG.md table — ` + | |
| `that block must move to workflows/discuss-phase/templates/discussion-log.md.`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/workflow-size-budget.test.cjs` around lines 300 - 308, The test
currently checks inlineDiscussionLogSignal.test(parent) then computes
occurrences and asserts occurrences === 0 which is contradictory; update the
assertion to allow one example reference by replacing the assert in that block
to assert.ok(occurrences <= 1, `Parent discuss-phase.md still contains the
inline DISCUSSION-LOG.md table — that block must move to
workflows/discuss-phase/templates/discussion-log.md.`) (or alternatively remove
the surrounding if and assert occurrences <= 1 unconditionally); reference
inlineDiscussionLogSignal, parent, occurrences and the assert.ok call to locate
the change.
Collaborator
Author
CodeRabbit review — addressed (14/14)All findings reviewed against the original
Refactor-equivalence verified. Compared current parent + extracted files against Notes on test hardening (#6, #8, #9, #10). CodeRabbit was right that the
|
…r progressive disclosure (closes #2551) Splits 1,347-line workflows/discuss-phase.md into a 495-line dispatcher plus per-mode files in workflows/discuss-phase/modes/ and templates in workflows/discuss-phase/templates/. Mirrors the progressive-disclosure pattern that #2361 enforced for agents. - Per-mode files: power, all, auto, chain, text, batch, analyze, default, advisor - Templates lazy-loaded at the step that produces the artifact (CONTEXT.md template at write_context, DISCUSSION-LOG.md template at git_commit, checkpoint.json schema when checkpointing) - Advisor mode gated behind `[ -f $HOME/.claude/get-shit-done/USER-PROFILE.md ]` — inverse of #2174's --advisor flag (don't pay the cost when unused) - scout_codebase phase-type→map selection table extracted to references/scout-codebase.md - New tests/workflow-size-budget.test.cjs enforces tiered budgets across all workflows/*.md (XL=1700 / LARGE=1500 / DEFAULT=1000) plus the explicit <500 ceiling for discuss-phase.md per #2551 - Existing tests updated to read from the new file locations after the split (functional equivalence preserved — content moved, not removed) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…, not Skip) CodeRabbit flagged drift between the parent step (which auto-selects "Update it") and modes/auto.md (which documented "Skip"). The pre-refactor file had both — line 182 said "Skip" in the overview, line 250 said "Update it" in the actual step. The step is authoritative. Fix the new mode file to match. Refs: PR #2607 review comment 3127783430
CodeRabbit identified four test smells where the split weakened coverage:
- workflow-size-budget: assertion was unreachable (entered if-block on match,
then asserted occurrences === 0 — always failed). Now unconditional.
- bug-2549-2550-2552: bounded-read assertion checked concatenated source, so
src.includes('3') was satisfied by unrelated content in scout-codebase.md
(e.g., "3-5 most relevant files"). Now reads parent only with a stricter
regex. Also asserts SCOUT_REF exists.
- chain-flag-plan-phase: filter(existsSync) silently skipped a missing
modes/chain.md. Now fails loudly via explicit asserts.
- discuss-checkpoint: same silent-filter pattern across three sources. Now
asserts each required path before reading.
Refs: PR #2607 review comments 3127783457, 3127783452, plus nitpicks for
chain-flag-plan-phase.test.cjs:21-24 and discuss-checkpoint.test.cjs:22-27
… portability - INVENTORY.md: subdirectory note said "50 top-level references" but the section header now says 51. Updated to 51. - templates/context.md: footer hardcoded XX-name instead of declared placeholders [X]/[Name], which would leak sample text into generated CONTEXT.md files. Now uses the declared placeholders. - references/scout-codebase.md: no-maps fallback used grep -rl with "\\|" alternation (GNU grep only — silent on BSD/macOS grep). Switched to grep -rlE with extended regex for portability. Refs: PR #2607 review comments 3127783404, 3127783448, plus nitpick for scout-codebase.md:32-40
- analyze.md / text.md / default.md: add language tags (markdown/text) to fenced example blocks to silence markdownlint MD040 warnings flagged by CodeRabbit (one fence in analyze.md, two in text.md, five in default.md). - discuss-phase.md: document overlay stacking rules in discuss_areas — fixed outer→inner order --analyze → --batch → --text, with a pointer to each overlay file for mode-specific precedence. - advisor.md: add tie-breaker rules for NON_TECHNICAL_OWNER signals — explicit technical_background overrides inferred signals; otherwise OR-aggregate; contradictory explanation_depth values resolve by most-recent-wins. Refs: PR #2607 review comments 3127783415, 3127783437, plus nitpicks for default.md:24, discuss-phase.md:345-365, and advisor.md:51-56
…der XL budget PR #2605 added 80 lines to execute-phase.md (1622 -> 1702), pushing it over the XL_BUDGET=1700 line cap enforced by tests/workflow-size-budget.test.cjs (introduced by this PR). Per the test's own remediation hint and #2551's progressive-disclosure pattern, extract the codebase_drift_gate step body to get-shit-done/workflows/execute-phase/steps/codebase-drift-gate.md and leave a brief pointer in the workflow. execute-phase.md is now 1633 lines. Budget is NOT relaxed; the offending workflow is tightened.
trek-e
force-pushed
the
refactor/2551-discuss-phase-progressive-disclosure
branch
from
8f38ff6 to
ddb5e21
Compare
Collaborator
Author
|
Rebased on Conflict resolution:
All other rebased commits (modes/auto.md alignment, regression test hardening, INVENTORY count + context.md + scout-codebase fix, fenced-example labels) applied without conflict. Tightening to keep XL budget intact (new commit
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2551.
TL;DR
workflows/discuss-phase.mdshrinks from 1,347 → 495 lines (-63%). Per-mode bodies, output templates, and the advisor flow are lazy-loaded only when their flag/condition fires. New CI test (tests/workflow-size-budget.test.cjs) enforces a tiered workflow line-count budget mirroring #2361's agent budget, with an explicit <500-line ceiling fordiscuss-phase.md.Line-count breakdown (Phase 1 walkthrough)
Before —
workflows/discuss-phase.md: 1,347 lines / ~15k tokens, loaded on every/gsd:discuss-phaseinvocation regardless of mode.<purpose>…<gray_area_identification>(1–112)<answer_validation>(114–140)initializestep + mode dispatch (146–192)check_blocking_antipatterns→present_gray_areas(194–672)advisor_researchstep (674–716)modes/advisor.md(gated on USER-PROFILE.md)discuss_areas(718–981) — inlines auto/text/batch/analyze/advisor/defaultmodes/{auto,text,batch,analyze,advisor,default}.mdwrite_context(983–1121) — inlines CONTEXT.md templatetemplates/context.md; lazy-load at write stepgit_commit(1163–1221) — inlines DISCUSSION-LOG.md templatetemplates/discussion-log.mdauto_advance(1239–1308) — chain orchestrationmodes/chain.md<power_user_mode>(1312–1325)modes/power.md(delegates to existingdiscuss-phase-power.md)<success_criteria>(1327–1347)templates/checkpoint.jsonAfter —
workflows/discuss-phase.md: 495 lines (under #2551's <500 target).Directory layout
Mode dispatch
Parent's
initializestep parses$ARGUMENTSand Reads exactly one mode file per invocation (plus overlays for--text/--batch/--analyze):--powermodes/power.md(then exit standard flow)ADVISOR_MODE = true(USER-PROFILE.md exists)modes/advisor.md--automodes/auto.md+modes/chain.md(auto-advance)--chainmodes/default.md+modes/chain.md--allmodes/all.mdoverlay--text(orworkflow.text_mode: true)modes/text.mdoverlay--batchmodes/batch.mdoverlay--analyzemodes/analyze.mdoverlaymodes/default.mdAdvisor existence-check guard
Bash test -foverRead-with-try-catch /Globbecause: single subprocess, clean exit code, zero token cost when the file doesn't exist (the common case for users who never ran/gsd-profile-user). Mirrors the inverse of #2174's--advisorflag — don't pay the cost when unused.When
ADVISOR_MODE=false,modes/advisor.mdis never Read. Detection block, calibration tier resolution, non-technical-owner reframing, and theadvisor_researchsubstep all live in that file.Templates loaded at the right step
templates/context.md— Read insidewrite_context, immediately before writing CONTEXT.mdtemplates/discussion-log.md— Read insidegit_commit, immediately before writing DISCUSSION-LOG.mdtemplates/checkpoint.json— Read bymodes/default.mdwhen checkpointing after each areaA test (
parent reads CONTEXT.md template at the write step (not at top)) explicitly asserts the CONTEXT.md template is NOT in<required_reading>— that would defeat the lazy-load savings.CI budget test
New
tests/workflow-size-budget.test.cjsmirrors #2361'stests/agent-size-budget.test.cjs:XLLARGEDEFAULTdiscuss-phase.mdhas its own explicit< 500assertion per #2551. The other workflows are grandfathered at their current sizes plus headroom — future growth fails CI; future shrinks (using the same progressive-disclosure pattern) are welcome.Changes
get-shit-done/workflows/discuss-phase.md(1,347 → 495 lines)get-shit-done/workflows/discuss-phase/modes/{power,all,auto,chain,text,batch,analyze,default,advisor}.mdget-shit-done/workflows/discuss-phase/templates/{context.md,discussion-log.md,checkpoint.json}get-shit-done/references/scout-codebase.md(phase-type→map selection table)tests/workflow-size-budget.test.cjs(16 assertions across line budgets, mode-file existence, parent-references-modes, advisor existence guard, template loading discipline, regression checks for each mode's documented behavior)Documentation
docs/ARCHITECTURE.md— new "Progressive disclosure for workflows" subsection under Components → Workflows, with the budget table and a pointer toworkflows/discuss-phase/as the canonical exampleCONTRIBUTING.md— File Structure section now points contributors atworkflows/<name>/modes/<mode>.mdfor adding new modes and at the budget test for the line ceilingdocs/INVENTORY.md— References count 50 → 51, new row forscout-codebase.mddocs/INVENTORY-MANIFEST.json— regenerated vianode scripts/gen-inventory-manifest.cjs --writeTests
npm testnpm run test:coverageexits 0tests/{agent-frontmatter,bug-2549-2550-2552-discuss-phase-context,chain-flag-plan-phase,discuss-checkpoint,discuss-phase-power,thinking-partner}.test.cjs. Each test now reads parent + relevant extracted file(s) so the assertion still matches the documented behavior.Design decisions
discuss-phase-power.md— issue listed 7 modes (power, all, auto, chain, text, batch, analyze). I addeddefault.md(the baseline interactive flow has its own discuss_areas body that needed a home) andadvisor.md(gated on USER-PROFILE.md per the issue's bullet opencode port #3).modes/power.mdis a thin dispatch to the existingdiscuss-phase-power.md— this keeps existing@-file references in the wild resolving against the unchanged path while still satisfying the issue's per-mode-file requirement.tests/workflow-size-budget.test.cjsover generalizingtests/agent-size-budget.test.cjs. Keeps each budget concern's failure isolated, mirrors the establishedbug-NNNN-X.test.cjsper-concern convention, and avoids an in-flight refactor that could mask bugs in Enhancement: Enforce agent-size budget + extract duplicated boilerplate to shared references #2361's test.workflows/discuss-phase/templates/, notget-shit-done/templates/— keeps the discuss-phase output templates colocated with the workflow they belong to, mirroring how the modes/ subdirectory is colocated. The existingget-shit-done/templates/context.md(a high-level documentation file about CONTEXT.md, not the substitution-ready template body) is unchanged.🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Tests