fix(workflow): correct grep anchor and add threshold=0 guard (#1979)
Two correctness bugs from @trek-e review: 1. Grep pattern `^<task` only matched unindented task tags, missing indented tasks in PLAN.md templates that use indentation. Fixed to `^\s*<task[[:space:]>]` which matches at any indentation level and avoids false positives on <tasks> or </task>. 2. Threshold=0 was documented to disable inline routing but the condition `TASK_COUNT <= INLINE_THRESHOLD` evaluated 0<=0 as true, routing empty plans inline even when the feature was disabled. Fixed by guarding with `INLINE_THRESHOLD > 0`. Added tests/inline-plan-threshold.test.cjs (8 tests) covering: - config-set accepts the key and threshold=0 - VALID_CONFIG_KEYS and planning-config.md contain the entry - Routing pattern matches indented tasks and rejects <tasks>/</task> - Inline routing is guarded by INLINE_THRESHOLD > 0 Review feedback on #2061 from @trek-e.
This commit is contained in:
@@ -61,16 +61,17 @@ PLAN_START_EPOCH=$(date +%s)
|
||||
|
||||
<step name="parse_segments">
|
||||
```bash
|
||||
TASK_COUNT=$(grep -c "^<task" .planning/phases/XX-name/{phase}-{plan}-PLAN.md 2>/dev/null || echo "0")
|
||||
# Count tasks — match <task tag at any indentation level
|
||||
TASK_COUNT=$(grep -cE '^\s*<task[[:space:]>]' .planning/phases/XX-name/{phase}-{plan}-PLAN.md 2>/dev/null || echo "0")
|
||||
INLINE_THRESHOLD=$(node "$HOME/.claude/get-shit-done/bin/gsd-tools.cjs" config-get workflow.inline_plan_threshold --default 2 2>/dev/null || echo "2")
|
||||
grep -n "type=\"checkpoint" .planning/phases/XX-name/{phase}-{plan}-PLAN.md
|
||||
```
|
||||
|
||||
**Primary routing: task count threshold (#1979)**
|
||||
|
||||
If `TASK_COUNT <= INLINE_THRESHOLD`: Use Pattern C (inline) regardless of checkpoint type. Small plans execute faster inline — avoids ~14K token subagent spawn overhead and preserves prompt cache. Configure threshold via `workflow.inline_plan_threshold` (default: 2).
|
||||
If `INLINE_THRESHOLD > 0` AND `TASK_COUNT <= INLINE_THRESHOLD`: Use Pattern C (inline) regardless of checkpoint type. Small plans execute faster inline — avoids ~14K token subagent spawn overhead and preserves prompt cache. Configure threshold via `workflow.inline_plan_threshold` (default: 2, set to `0` to always spawn subagents).
|
||||
|
||||
If `TASK_COUNT > INLINE_THRESHOLD`: Apply checkpoint-based routing below.
|
||||
Otherwise: Apply checkpoint-based routing below.
|
||||
|
||||
**Checkpoint-based routing (plans with > threshold tasks):**
|
||||
|
||||
|
||||
131
tests/inline-plan-threshold.test.cjs
Normal file
131
tests/inline-plan-threshold.test.cjs
Normal file
@@ -0,0 +1,131 @@
|
||||
/**
|
||||
* Tests for workflow.inline_plan_threshold config key and routing logic (#1979).
|
||||
*
|
||||
* Verifies:
|
||||
* 1. The config key is accepted by config-set (VALID_CONFIG_KEYS contains it)
|
||||
* 2. The key is documented in planning-config.md
|
||||
* 3. The execute-plan.md routing instruction uses the correct grep pattern
|
||||
* (matches <task at any indentation, since PLAN.md templates differ)
|
||||
* 4. The workflow guards threshold=0 to disable inline routing
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
const { test, describe, beforeEach, afterEach } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs');
|
||||
|
||||
const repoRoot = path.resolve(__dirname, '..');
|
||||
const executePlanPath = path.join(repoRoot, 'get-shit-done', 'workflows', 'execute-plan.md');
|
||||
const planningConfigPath = path.join(repoRoot, 'get-shit-done', 'references', 'planning-config.md');
|
||||
const configCjsPath = path.join(repoRoot, 'get-shit-done', 'bin', 'lib', 'config.cjs');
|
||||
|
||||
describe('inline_plan_threshold config key (#1979)', () => {
|
||||
let tmpDir;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
test('config-set accepts workflow.inline_plan_threshold', () => {
|
||||
const result = runGsdTools('config-set workflow.inline_plan_threshold 3', tmpDir);
|
||||
assert.ok(result.success, `config-set should accept workflow.inline_plan_threshold: ${result.error}`);
|
||||
});
|
||||
|
||||
test('config-set accepts threshold=0 to disable inline routing', () => {
|
||||
const result = runGsdTools('config-set workflow.inline_plan_threshold 0', tmpDir);
|
||||
assert.ok(result.success, `config-set should accept 0: ${result.error}`);
|
||||
});
|
||||
|
||||
test('VALID_CONFIG_KEYS in config.cjs contains workflow.inline_plan_threshold', () => {
|
||||
const content = fs.readFileSync(configCjsPath, 'utf-8');
|
||||
assert.match(
|
||||
content,
|
||||
/['"]workflow\.inline_plan_threshold['"]/,
|
||||
'workflow.inline_plan_threshold must be in VALID_CONFIG_KEYS'
|
||||
);
|
||||
});
|
||||
|
||||
test('planning-config.md documents workflow.inline_plan_threshold', () => {
|
||||
const content = fs.readFileSync(planningConfigPath, 'utf-8');
|
||||
assert.match(
|
||||
content,
|
||||
/workflow\.inline_plan_threshold/,
|
||||
'planning-config.md must document workflow.inline_plan_threshold'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('execute-plan.md routing instruction (#1979)', () => {
|
||||
test('grep pattern matches <task at any indentation level', () => {
|
||||
const content = fs.readFileSync(executePlanPath, 'utf-8');
|
||||
|
||||
// The new pattern should use \s* for leading whitespace, not ^ anchor alone
|
||||
// Must match both "<task type=" (unindented) and " <task type=" (indented)
|
||||
assert.match(
|
||||
content,
|
||||
/TASK_COUNT=\$\(grep -cE '\^\\s\*<task/,
|
||||
'grep pattern must allow any leading whitespace before <task'
|
||||
);
|
||||
});
|
||||
|
||||
test('inline routing is guarded by INLINE_THRESHOLD > 0', () => {
|
||||
const content = fs.readFileSync(executePlanPath, 'utf-8');
|
||||
assert.match(
|
||||
content,
|
||||
/INLINE_THRESHOLD\s*>\s*0.*TASK_COUNT\s*<=\s*INLINE_THRESHOLD/s,
|
||||
'inline routing must be guarded by INLINE_THRESHOLD > 0 so threshold=0 disables it'
|
||||
);
|
||||
});
|
||||
|
||||
test('grep pattern does NOT use ^<task alone (would miss indented tasks)', () => {
|
||||
const content = fs.readFileSync(executePlanPath, 'utf-8');
|
||||
// The old buggy pattern: grep -c "^<task" with no whitespace allowance
|
||||
const buggyPattern = /grep -c "\^<task"/;
|
||||
assert.doesNotMatch(
|
||||
content,
|
||||
buggyPattern,
|
||||
'must not use the buggy "^<task" pattern which misses indented tasks'
|
||||
);
|
||||
});
|
||||
|
||||
test('grep pattern matches real-world indented task formats', () => {
|
||||
// Simulate how the grep pattern would behave against sample PLAN.md content
|
||||
// Extract the pattern from execute-plan.md
|
||||
const content = fs.readFileSync(executePlanPath, 'utf-8');
|
||||
const patternMatch = content.match(/TASK_COUNT=\$\(grep -cE '([^']+)'/);
|
||||
assert.ok(patternMatch, 'must find TASK_COUNT grep pattern');
|
||||
|
||||
const regexSource = patternMatch[1].replace(/\\s/g, '\\s').replace(/\[\[:space:\]>\]/, '[\\s>]');
|
||||
const re = new RegExp(regexSource, 'gm');
|
||||
|
||||
// Test cases: should match all of these as single tasks
|
||||
const samples = [
|
||||
'<task type="auto">',
|
||||
' <task type="auto">',
|
||||
' <task type="checkpoint:decision">',
|
||||
'\t<task type="auto">',
|
||||
];
|
||||
for (const sample of samples) {
|
||||
const matches = sample.match(re);
|
||||
assert.ok(matches && matches.length > 0, `Pattern must match: ${JSON.stringify(sample)}`);
|
||||
}
|
||||
|
||||
// Non-task lines should not match
|
||||
const nonMatches = [
|
||||
'<tasks>',
|
||||
'</task>',
|
||||
'// <task comment',
|
||||
];
|
||||
for (const sample of nonMatches) {
|
||||
const matches = sample.match(re);
|
||||
assert.ok(!matches || matches.length === 0, `Pattern must NOT match: ${JSON.stringify(sample)}`);
|
||||
}
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user