From 8a5166598c6bcae87908d2e03a52bed2782cfe41 Mon Sep 17 00:00:00 2001 From: 0xdhx Date: Sat, 19 Sep 2026 23:01:21 -0500 Subject: [PATCH] fix(#4830): re-land #4768's letter-suffix phase-id fix and its lint-phase-id-drift ratchets on current next (#4873) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#4830): re-land #4768's letter-suffix phase-id fix and its lint-phase-id-drift ratchets on current next Commit 740ba0d8a (#4781) removed every change #4768 had merged for #4748: the first-non-digit split at execute-phase.md's two arithmetic sites, the init-emitted `padded_phase` the REVIEW.md lookup binds instead of `printf "%02d"`, the canonical-grammar extractions in autonomous.md and plan-review-convergence.md, the `.changeset/zesty-wolves-tumble.md` fragment, and the three lint-phase-id-drift ratchets with their tests. The guard and the code it guarded left together, so nothing went red. This is a cherry-pick of 092d9256b onto current `next`, resolved against the #4683 threat-id fields on execute-phase.md's Parse-JSON line, with the changeset `pr:` reset to the placeholder and the compact-content benchmark baseline regenerated against the current base. (cherry picked from commit 092d9256b89c9f6055928f3fd21b633036704dbc) Emitted-Drift-Ack-Growth: autonomous.md — restores #4768's canonical-grammar extraction and its explanatory comment for --from/--to/--only Emitted-Drift-Ack-Growth: execute-phase.md — restores #4768's first-non-digit split at two arithmetic sites and the padded_phase binding for the REVIEW.md lookup Emitted-Drift-Ack-Growth: plan-review-convergence.md — restores #4768's canonical-grammar phase extraction and its comment Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AX7LXxc3uAkGki6iaYiAMP * chore(#4830): set changeset fragment pr to 4873 --------- Co-authored-by: Claude Fable 5.1 Co-authored-by: Tom Boucher --- .changeset/zesty-wolves-tumble.md | 5 + gsd-core/references/phase-argument-parsing.md | 16 +- gsd-core/references/tdd.md | 5 +- gsd-core/workflows/autonomous.md | 9 +- gsd-core/workflows/execute-phase.md | 20 +- .../steps/completion-reconciliation.md | 6 +- gsd-core/workflows/plan-review-convergence.md | 4 +- scripts/lint-phase-id-drift.cjs | 164 +++++++++- src/init.cts | 7 + .../execute-phase-decimal-arithmetic.test.cjs | 16 +- .../compact-content-benchmark-baseline.json | 12 +- tests/init.test.cjs | 28 ++ tests/lint-phase-id-drift.test.cjs | 120 ++++++++ tests/nsegment-phase-grammar.test.cjs | 288 ++++++++++++++++++ tests/safe-resume-gate-anchoring.test.cjs | 24 +- 15 files changed, 674 insertions(+), 50 deletions(-) create mode 100644 .changeset/zesty-wolves-tumble.md diff --git a/.changeset/zesty-wolves-tumble.md b/.changeset/zesty-wolves-tumble.md new file mode 100644 index 000000000..7f986a528 --- /dev/null +++ b/.changeset/zesty-wolves-tumble.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4873 +--- +**`/gsd-execute-phase`, `/gsd-autonomous --from|--to|--only`, `/gsd-plan-review-convergence` and the TDD plan path now handle letter-variant phase ids (`12A`, `3A`, `23A.1.2`)** — seven shell sites outside #4660's six still assumed a phase number was digits-and-dots: the post-#4619 `$((10#$PHASE_INT))` split aborted bash on `03A`, the review-file lookup's `printf "%02d"` printed the wrong file (and read an already-padded `08` as octal), and the `--from`/`--to`/`--only` and plan-review-convergence extractions silently truncated `12A` to `12` (and `23.1.2` to `23.1`). The split now stops at the first non-digit, `init execute-phase` emits `padded_phase` for the lookup, the extractions use the canonical grammar, the legacy normalizer pads a letter id, a parity test drives every site's live shell against a letter-suffixed fixture, and `lint-phase-id-drift` gains three ratchets so none of the shapes can return silently. (#4748) diff --git a/gsd-core/references/phase-argument-parsing.md b/gsd-core/references/phase-argument-parsing.md index dea3ba534..8604fba58 100644 --- a/gsd-core/references/phase-argument-parsing.md +++ b/gsd-core/references/phase-argument-parsing.md @@ -27,16 +27,18 @@ Returns JSON with: ## Manual Normalization (Legacy) -Zero-pad integer phases to 2 digits. Preserve decimal suffixes. +Zero-pad the leading integer to 2 digits. Preserve a letter suffix and any dotted +segments — the canonical grammar in `src/phase-id.cts` (`normalizePhaseName`): +`8 → 08`, `2.1 → 02.1`, `3A → 03A`, `23.1.2 → 23.1.2`. ```bash # Normalize phase number -if [[ "$PHASE" =~ ^[0-9]+$ ]]; then - # Integer: 8 → 08 - PHASE=$(printf "%02d" "$PHASE") -elif [[ "$PHASE" =~ ^([0-9]+)\.([0-9]+)$ ]]; then - # Decimal: 2.1 → 02.1 - PHASE=$(printf "%02d.%s" "${BASH_REMATCH[1]}" "${BASH_REMATCH[2]}") +# #4748: one branch for the whole canonical token — digits, optional [A-Z], +# dotted segments. Pad through $((10#…)) so an already-padded `08` is not read +# as octal by printf; anything non-canonical passes through untouched. +if [[ "$PHASE" =~ ^([0-9]+)([A-Z]?)((\.[0-9]+)*)$ ]]; then + PHASE_INT=${BASH_REMATCH[1]} + PHASE=$(printf "%02d" "$((10#$PHASE_INT))")${BASH_REMATCH[2]}${BASH_REMATCH[3]} fi ``` diff --git a/gsd-core/references/tdd.md b/gsd-core/references/tdd.md index 6ec87fa65..867cb86f2 100644 --- a/gsd-core/references/tdd.md +++ b/gsd-core/references/tdd.md @@ -297,8 +297,9 @@ After completing a `type: tdd` plan, the executor validates the git log: # The commit protocol promises no zero-padding for ${PHASE}/${PLAN} — strip both and # match the commit-scope position anchored (#4003). #4619: PHASE may be decimal/ # N-segment; zero-strip only the leading integer segment, escape the rest. -PHASE_INT=${PHASE%%.*}; PHASE_FRAC=${PHASE#"$PHASE_INT"} -PHASE_N="$((10#$PHASE_INT))${PHASE_FRAC//./\\.}" +# #4748: it may also carry a letter suffix (03A), so split at the first non-digit. +PHASE_INT=${PHASE%%[!0-9]*}; PHASE_REST=${PHASE#"$PHASE_INT"} +PHASE_N="$((10#$PHASE_INT))${PHASE_REST//./\\.}" PLAN_N=$((10#${PLAN})) # Check for RED gate commit git log --oneline -E --grep="^test\((0*${PHASE_N})-(0*${PLAN_N})\):" | head -1 diff --git a/gsd-core/workflows/autonomous.md b/gsd-core/workflows/autonomous.md index 2d692fed7..7e78eb363 100644 --- a/gsd-core/workflows/autonomous.md +++ b/gsd-core/workflows/autonomous.md @@ -21,19 +21,22 @@ Read all files referenced by the invoking prompt's execution_context before star Parse `$ARGUMENTS` for `--from N`, `--to N`, `--only N`, `--interactive`, `--converge`/`--cross-ai`, reviewer selector flags, and `--max-cycles N`: ```bash +# #4748: the phase token is the canonical grammar (src/phase-id.cts) — digits, +# an optional uppercase letter, any number of dotted segments — so `12A` and +# `23.1.2` extract whole instead of truncating to `12` / `23.1`. FROM_PHASE="" if echo "$ARGUMENTS" | grep -qE '\-\-from\s+[0-9]'; then - FROM_PHASE=$(echo "$ARGUMENTS" | grep -oE '\-\-from\s+[0-9]+\.?[0-9]*' | awk '{print $2}') + FROM_PHASE=$(echo "$ARGUMENTS" | grep -oE '\-\-from\s+[0-9]+[A-Z]?(\.[0-9]+)*' | awk '{print $2}') fi TO_PHASE="" if echo "$ARGUMENTS" | grep -qE '\-\-to\s+[0-9]'; then - TO_PHASE=$(echo "$ARGUMENTS" | grep -oE '\-\-to\s+[0-9]+\.?[0-9]*' | awk '{print $2}') + TO_PHASE=$(echo "$ARGUMENTS" | grep -oE '\-\-to\s+[0-9]+[A-Z]?(\.[0-9]+)*' | awk '{print $2}') fi ONLY_PHASE="" if echo "$ARGUMENTS" | grep -qE '\-\-only\s+[0-9]'; then - ONLY_PHASE=$(echo "$ARGUMENTS" | grep -oE '\-\-only\s+[0-9]+\.?[0-9]*' | awk '{print $2}') + ONLY_PHASE=$(echo "$ARGUMENTS" | grep -oE '\-\-only\s+[0-9]+[A-Z]?(\.[0-9]+)*' | awk '{print $2}') FROM_PHASE="$ONLY_PHASE" fi diff --git a/gsd-core/workflows/execute-phase.md b/gsd-core/workflows/execute-phase.md index 864dccd17..ec602032f 100644 --- a/gsd-core/workflows/execute-phase.md +++ b/gsd-core/workflows/execute-phase.md @@ -92,7 +92,7 @@ if [[ "$INIT" == @file:* ]]; then INIT=$(cat "${INIT#@file:}"); fi AGENT_SKILLS=$(gsd_run query agent-skills gsd-executor) ``` -Parse JSON for: `executor_model`, `verifier_model`, `commit_docs`, `parallelization`, `branching_strategy`, `branch_name`, `phase_found`, `phase_dir`, `phase_number`, `phase_name`, `phase_slug`, `plans`, `incomplete_plans`, `plan_count`, `incomplete_count`, `state_exists`, `roadmap_exists`, `phase_req_ids`, `response_language`, `requirements_path`, `section_manifest`, `threat_id_duplicate_count`. +Parse JSON for: `executor_model`, `verifier_model`, `commit_docs`, `parallelization`, `branching_strategy`, `branch_name`, `phase_found`, `phase_dir`, `phase_number`, `padded_phase`, `phase_name`, `phase_slug`, `plans`, `incomplete_plans`, `plan_count`, `incomplete_count`, `state_exists`, `roadmap_exists`, `phase_req_ids`, `response_language`, `requirements_path`, `section_manifest`, `threat_id_duplicate_count`. **Threat-ID gate (#4683):** if `threat_id_duplicate_count` is non-zero, read and execute `execute-phase/steps/threat-id-gate.md` BEFORE any dispatch — it is a hard stop (the full duplicate list is in `threat_id_duplicates`). @@ -196,8 +196,11 @@ PHASE_NUMBER="{phase_number}" # #4619: {phase_number} may be decimal (01.1) or N-segment (23.1.2) — $((10#...)) # is a hard shell syntax error on a non-integer, so zero-strip only the LEADING # integer segment and keep the rest as an escaped-dot string for the ERE below. -PHASE_INT=${PHASE_NUMBER%%.*}; PHASE_FRAC=${PHASE_NUMBER#"$PHASE_INT"} -PHASE_N="$((10#$PHASE_INT))${PHASE_FRAC//./\\.}" +# #4748: it may also carry a letter suffix (03A, 23A.1.2 — the canonical grammar +# is digits, optional [A-Z], dotted segments), so split at the first NON-DIGIT, +# not the first dot: the letter rides along in the rest, unescaped. +PHASE_INT=${PHASE_NUMBER%%[!0-9]*}; PHASE_REST=${PHASE_NUMBER#"$PHASE_INT"} +PHASE_N="$((10#$PHASE_INT))${PHASE_REST//./\\.}" PLAN_N=$((10#{plan_padded})) PLAN_SCOPE_RE="^[a-z]+\((0*${PHASE_N})-(0*${PLAN_N})\):" MILESTONE_BASE=$(git describe --tags --abbrev=0 2>/dev/null || echo "") @@ -219,9 +222,10 @@ if [ "$TDD_MODE" = "true" ]; then # #4003: same anchored scope and milestone bound as safe_resume_gate — a padded # literal grep hard-halts on a correct unpadded RED commit. # #4619: PHASE_NUMBER may be decimal/N-segment; zero-strip only the leading - # integer segment, escape the rest for the ERE below. - PHASE_INT=${PHASE_NUMBER%%.*}; PHASE_FRAC=${PHASE_NUMBER#"$PHASE_INT"} - PHASE_N="$((10#$PHASE_INT))${PHASE_FRAC//./\\.}" + # integer segment, escape the rest for the ERE below. #4748: it may carry a + # letter suffix (03A), so the split is at the first non-digit, not the dot. + PHASE_INT=${PHASE_NUMBER%%[!0-9]*}; PHASE_REST=${PHASE_NUMBER#"$PHASE_INT"} + PHASE_N="$((10#$PHASE_INT))${PHASE_REST//./\\.}" PLAN_N=$((10#${PLAN_ID})) PLAN_SCOPE_RE="^[a-z]+\((0*${PHASE_N})-(0*${PLAN_N})\):" # TDD gate's own scope check TDD_MILESTONE_BASE=$(git describe --tags --abbrev=0 2>/dev/null || echo "") @@ -1159,7 +1163,9 @@ Skill(skill="gsd-${ref.skill}", args="${PHASE_NUMBER}") **Check results using deterministic path (not glob):** ```bash -PADDED=$(printf "%02d" "${PHASE_NUMBER}") +# #4748: bind init's normalized id — `printf "%02d"` cannot pad a letter id +# (03A → `03`, exit 1) and reads an already-padded `08` as octal (→ `00`). +PADDED="{padded_phase}" REVIEW_FILE="${PHASE_DIR}/${PADDED}-REVIEW.md" REVIEW_STATUS=$(sed -n '/^---$/,/^---$/p' "$REVIEW_FILE" | grep "^status:" | head -1 | cut -d: -f2 | tr -d ' ') ``` diff --git a/gsd-core/workflows/execute-phase/steps/completion-reconciliation.md b/gsd-core/workflows/execute-phase/steps/completion-reconciliation.md index fb55f574d..4a6b7c317 100644 --- a/gsd-core/workflows/execute-phase/steps/completion-reconciliation.md +++ b/gsd-core/workflows/execute-phase/steps/completion-reconciliation.md @@ -28,9 +28,9 @@ block indefinitely waiting for a signal; verify via filesystem and git state. SUMMARY_EXISTS=$(test -f "{phase_dir}/{plan_number}-{plan_padded}-SUMMARY.md" && echo "true" || echo "false") # #4003: anchored, zero-pad-tolerant scope (see safe_resume_gate); --since stays. SPOT_PHASE_NUMBER="{phase_number}" -# #4619: same decimal/N-segment handling as safe_resume_gate. -SPOT_PHASE_INT=${SPOT_PHASE_NUMBER%%.*}; SPOT_PHASE_FRAC=${SPOT_PHASE_NUMBER#"$SPOT_PHASE_INT"} -SPOT_PHASE_N="$((10#$SPOT_PHASE_INT))${SPOT_PHASE_FRAC//./\\.}" +# #4619 / #4748: same decimal/N-segment/letter-suffix handling as safe_resume_gate. +SPOT_PHASE_INT=${SPOT_PHASE_NUMBER%%[!0-9]*}; SPOT_PHASE_REST=${SPOT_PHASE_NUMBER#"$SPOT_PHASE_INT"} +SPOT_PHASE_N="$((10#$SPOT_PHASE_INT))${SPOT_PHASE_REST//./\\.}" SPOT_PLAN_N=$((10#{plan_padded})) COMMITS_FOUND=$(git log --oneline --all -E --grep="^[a-z]+\((0*${SPOT_PHASE_N})-(0*${SPOT_PLAN_N})\):" --since="1 hour ago" | head -1) COMMITS_SINCE_DISPATCH=$(git log "${EXPECTED_BRANCH}" --since="${DISPATCH_TS}" --oneline | head -1) diff --git a/gsd-core/workflows/plan-review-convergence.md b/gsd-core/workflows/plan-review-convergence.md index 0338da549..92c494744 100644 --- a/gsd-core/workflows/plan-review-convergence.md +++ b/gsd-core/workflows/plan-review-convergence.md @@ -21,7 +21,9 @@ Read all files referenced by the invoking prompt's execution_context before star Extract from $ARGUMENTS: phase number, reviewer flags (the declared reviewer lane flags, plus `--all`), `--max-cycles N`, `--text`, `--ws`. ```bash -PHASE=$(echo "$ARGUMENTS" | grep -oE '[0-9]+\.?[0-9]*' | head -1) +# #4748: canonical phase grammar (digits, optional [A-Z], dotted segments) — +# `12A` / `23.1.2` extract whole instead of truncating to `12` / `23.1`. +PHASE=$(echo "$ARGUMENTS" | grep -oE '[0-9]+[A-Z]?(\.[0-9]+)*' | head -1) # #2315: do NOT default REVIEWER_FLAGS to --codex here. The default is resolved # against review.default_reviewers in step 1.5 (after the config gate) so a bare diff --git a/scripts/lint-phase-id-drift.cjs b/scripts/lint-phase-id-drift.cjs index b811b0215..4d0b3bdca 100644 --- a/scripts/lint-phase-id-drift.cjs +++ b/scripts/lint-phase-id-drift.cjs @@ -246,11 +246,14 @@ function findBranchSlugFallbackDrift(text) { // 1. Prose mentioning the literal pattern in a full-line `#`-comment // (filtered by the caller, not this regex — see below). // 2. `$((10#$PHASE_INT))` / `$((10#$SPOT_PHASE_INT))` — arithmetic on the -// NOW-safe variable the #4619 fix produces via `PHASE_INT=${PHASE_NUMBER%%.*}`; -// a `%%.*`-stripped value can never contain a dot, so base-10 arithmetic -// on it can never hit the #4619 syntax-error class. Any name ending in -// `_INT` (case-insensitive) is that established "already reduced to a -// safe integer" convention. +// NOW-safe variable the #4619 fix produces via a leading-digit-run split +// (`PHASE_INT=${PHASE_NUMBER%%[!0-9]*}` since #4748; `%%.*` before it); +// a digit-run value can never contain a dot OR a letter, so base-10 +// arithmetic on it can never hit the #4619 / #4748 error classes. Any +// name ending in `_INT` (case-insensitive) is that established "already +// reduced to a safe integer" convention. That convention is a NAME, not +// a proof — `findDotOnlyIntegerSplitDrift` below polices that the split +// producing it actually stops at the first non-digit. // 3. `$((10#{plan_padded}))` / `$((10#${PLAN_ID}))` — plan ids are plain // integers and were never in scope; this rule only polices variables // that carry a *phase* id. @@ -449,6 +452,144 @@ function scanMarkdownLetterlessPhaseMirror(root) { return violations; } +// #4748 (epic #4634): three shell shapes OUTSIDE the grammar-mirror family the +// rules above police — consumers of a phase id rather than regexes for one — +// each of which the letter axis broke while every rule above reported clean: +// +// a. `PHASE_INT=${PHASE_NUMBER%%.*}` — the post-#4619 dot-only split. The +// `_INT` name it produces satisfies the shell-arithmetic rule's escape, +// but on `03A` the "integer" is `03A` and `$((10#03A))` aborts. The safe +// split stops at the first NON-digit: `${PHASE_NUMBER%%[!0-9]*}`. +// b. `[0-9]+\.?[0-9]*` (and `\d+\.?\d*`) — a digit-then-optional-dot +// extraction that is neither the bounded `(\.[0-9]+)?` shape the +// single-segment rule bans nor the unbounded `(\.[0-9]+)*` shape the +// letterless rule inspects, so both were blind to it. It captures `12` +// from `12A` and `23.1` from `23.1.2`, silently. +// c. `printf "%02d" "$PHASE_NUMBER"` — re-padding the whole id in shell. +// Rejects a letter id (prints `03`, exit 1) and misreads an already +// padded `08` as octal (prints `00`). Padding belongs to the canonical +// normalizer (`padded_phase` from init, or `normalizePhaseName`); the one +// legitimate shell pad is of an `_INT` value via `$((10#…))`. +// +// Same sanction as the other markdown rules: `` on +// the nearest preceding non-blank line. Same documented limit: a per-line +// textual scan for the common accidental shape, not an obfuscated one. + +// a. `_INT=${%%.*}` — the `_INT` destination is the +// discriminator, deliberately: it is the name the shell-arithmetic rule +// trusts as "already a safe integer", so a dot-only split INTO it is the +// exact promise this rule exists to check. A dot-only split into any other +// name is a different, legitimate operation — `PARENT_PHASE="${PHASE_NUMBER%%.*}"` +// (gap-closure-artifacts.md) wants everything before the first dot, letter +// included, and is correct. Widening to any destination was tried and flagged +// that site. The optional quote after `=` is the one spelling that site uses. +const DOT_ONLY_INT_SPLIT_DRIFT_RE = /[A-Za-z0-9_]*_INT="?\$\{([A-Za-z0-9_]+)%%\\?\.\*\}/i; + +/** + * Pure: find every unsanctioned dot-only integer split of a phase-carrying + * variable in `text`. Skips full-line `#` comments (prose). Returns [{ line, found }]. + */ +function findDotOnlyIntegerSplitDrift(text) { + const out = []; + const lines = text.split('\n'); + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + if (/^\s*#/.test(line)) continue; + const m = DOT_ONLY_INT_SPLIT_DRIFT_RE.exec(line); + if (!m) continue; + if (!/phase/i.test(m[1])) continue; + if (isSanctionedByPrecedingComment(lines, i, MD_OWNER_RE)) continue; + out.push({ line: i + 1, found: m[0] }); + } + return out; +} + +// b. The digit-then-optional-dot shape, in both `[0-9]` and `\d` spellings. +// Anchored on the trailing `*` of the second digit class so a bare `[0-9]+` +// probe or a `[0-9]+\.[0-9]+` (mandatory-dot) shape is not matched. +const LOOSE_DOTTED_PHASE_DRIFT_RE = /(?:\\{1,2}d|\[0-9\])\+\\{1,2}\.\?(?:\\{1,2}d|\[0-9\])\*/; + +/** + * Pure: find every unsanctioned `[0-9]+\.?[0-9]*`-shaped phase extraction in + * `text`, restricted to phase-carrying lines like its two sibling regex rules. + * Returns [{ line, found }]. + */ +function findLooseDottedPhaseRegexDrift(text) { + const out = []; + const lines = text.split('\n'); + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + const m = LOOSE_DOTTED_PHASE_DRIFT_RE.exec(line); + if (!m) continue; + if (!PHASE_CARRYING_LINE_RE.test(line)) continue; + if (isSanctionedByPrecedingComment(lines, i, MD_OWNER_RE)) continue; + out.push({ line: i + 1, found: m[0] }); + } + return out; +} + +// c. `printf "%Nd" …` / `printf '%0Nd' …` — any integer conversion, either +// quote, with or without the zero flag — whose argument list names a +// phase-carrying variable that is NOT an `_INT` (the `$((10#$PHASE_INT))` +// pad is the sanctioned shape). `%d` cannot parse a letter id under any +// width, so the flag is not the discriminator. Captures the first such name +// so the report says what was padded. +const SHELL_PHASE_PRINTF_PAD_RE = /printf\s+(?:"%0?\d*d[^"]*"|'%0?\d*d[^']*')\s+(.*)$/; +const SHELL_VAR_NAME_RE = /\$\{?([A-Za-z_][A-Za-z0-9_]*)/g; + +/** + * Pure: find every unsanctioned `printf "%02d"` re-pad of a phase-carrying, + * non-`_INT` shell variable in `text`. Skips full-line `#` comments. + * Returns [{ line, found }]. + */ +function findShellPhasePrintfPadDrift(text) { + const out = []; + const lines = text.split('\n'); + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + if (/^\s*#/.test(line)) continue; + const m = SHELL_PHASE_PRINTF_PAD_RE.exec(line); + if (!m) continue; + const offender = [...m[1].matchAll(SHELL_VAR_NAME_RE)] + .map((v) => v[1]) + .find((name) => /phase/i.test(name) && !/_int$/i.test(name)); + if (!offender) continue; + if (isSanctionedByPrecedingComment(lines, i, MD_OWNER_RE)) continue; + out.push({ line: i + 1, found: `printf "%0…d" …$${offender}` }); + } + return out; +} + +/** + * Scan the shell roots (`gsd-core/workflows/**\/*.md`, `gsd-core/references/**\/*.md`) + * for the two shell-idiom rules (a, c) and the three regex roots (those plus + * `agents/**\/*.md`) for the extraction-shape rule (b). Returns + * [{ file, kind, line, found }] with repo-relative paths. + */ +function scanMarkdownLetterAxisConsumers(root) { + const violations = []; + const collect = (dirs, finder, kind) => { + for (const dir of dirs) { + for (const file of walkMd(path.join(root, dir), [])) { + const rel = path.relative(root, file); + let text; + try { + text = fs.readFileSync(file, 'utf8'); + } catch { + continue; + } + for (const d of finder(text)) { + violations.push({ file: rel, kind, ...d }); + } + } + } + }; + collect(MD_SCAN_DIRS, findDotOnlyIntegerSplitDrift, 'dot-only-int-split'); + collect(SINGLE_SEGMENT_SCAN_DIRS, findLooseDottedPhaseRegexDrift, 'loose-dotted-phase-regex'); + collect(MD_SCAN_DIRS, findShellPhasePrintfPadDrift, 'shell-phase-printf-pad'); + return violations; +} + // Authored TypeScript source only (the generated bin/lib/*.cjs mirror it). const SCAN_DIRS = ['src']; const SCAN_EXT = new Set(['.cts', '.ts', '.mts']); @@ -602,6 +743,7 @@ function scanAll(root) { ...scanMarkdownShellArith(root), ...scanMarkdownSingleSegmentPhaseRegex(root), ...scanMarkdownLetterlessPhaseMirror(root), + ...scanMarkdownLetterAxisConsumers(root), ]; } @@ -631,6 +773,11 @@ function main() { process.stderr.write('A digit-only unbounded-segment phase regex `[0-9]+(\\.[0-9]+)*` on the same roots\n'); process.stderr.write('is missing the canonical letter axis (#4660) — widen to `[0-9]+[A-Z]?(\\.[0-9]+)*`\n'); process.stderr.write('or sanction with ``.\n'); + process.stderr.write('Three letter-hostile consumers of a phase id are banned on the same roots (#4748):\n'); + process.stderr.write('a dot-only integer split `X_INT=${PHASE%%.*}` (split at the first non-digit,\n'); + process.stderr.write('`${PHASE%%[!0-9]*}`); a `[0-9]+\\.?[0-9]*` extraction (use `[0-9]+[A-Z]?(\\.[0-9]+)*`);\n'); + process.stderr.write('and a `printf "%02d"` re-pad of a phase variable (bind init\'s `padded_phase`, or pad\n'); + process.stderr.write('only an `_INT` via `$((10#…))`) — or sanction with ``.\n'); process.stderr.write('A `.replace(\'{slug}\', ... || \'phase\')` fallback is banned outright (#4126) —\n'); process.stderr.write('use `renderPhaseBranchName(` or sanction with\n'); process.stderr.write('`// phase-id-owner: ` on the line directly above:\n'); @@ -650,9 +797,13 @@ module.exports = { findShellPhaseArithDrift, findSingleSegmentPhaseRegexDrift, findLetterlessPhaseMirrorDrift, + findDotOnlyIntegerSplitDrift, + findLooseDottedPhaseRegexDrift, + findShellPhasePrintfPadDrift, scanMarkdownShellArith, scanMarkdownSingleSegmentPhaseRegex, scanMarkdownLetterlessPhaseMirror, + scanMarkdownLetterAxisConsumers, scanRepo, scanAll, countSelectorBaselines, @@ -664,4 +815,7 @@ module.exports = { SHELL_PHASE_ARITH_DRIFT_RE, SINGLE_SEGMENT_PHASE_DRIFT_RE, LETTERLESS_PHASE_MIRROR_DRIFT_RE, + DOT_ONLY_INT_SPLIT_DRIFT_RE, + LOOSE_DOTTED_PHASE_DRIFT_RE, + SHELL_PHASE_PRINTF_PAD_RE, }; diff --git a/src/init.cts b/src/init.cts index 56eaca26d..8e8667efd 100644 --- a/src/init.cts +++ b/src/init.cts @@ -1028,6 +1028,13 @@ function cmdInitExecutePhase( ? toPosixPath(path.join(cwd, phaseInfo['directory'] as string)) : null, phase_number: phaseInfo?.['phase_number'] || null, + // #4748: the disk path hands back the directory's padded number (`03A`) + // but the ROADMAP fallback above hands back the heading's bare one (`3A`), + // and execute-phase.md's review lookup needs the padded form for + // `{PADDED}-REVIEW.md`. It used to re-pad in shell with `printf "%02d"`, + // which cannot pad a letter id and reads an already-padded `08` as octal. + // Emit the canonical normalization, as the plan-phase/code-review inits do. + padded_phase: phaseInfo?.['phase_number'] ? normalizePhaseName(phaseInfo['phase_number']) : null, // #3171: prefer the ROADMAP's curated display name for `phase_name`. When // the phase directory already exists on disk, the disk-lookup path // (searchPhaseInDir) derives phase_name from the directory-name remainder diff --git a/tests/execute-phase-decimal-arithmetic.test.cjs b/tests/execute-phase-decimal-arithmetic.test.cjs index 8f2932017..d6bc19cd8 100644 --- a/tests/execute-phase-decimal-arithmetic.test.cjs +++ b/tests/execute-phase-decimal-arithmetic.test.cjs @@ -37,9 +37,12 @@ const TIMEOUT = 5000; // site 1/2 (execute-phase.md): source PHASE_NUMBER, prefix PHASE // site 3 (completion-reconciliation.md): source SPOT_PHASE_NUMBER, prefix SPOT_PHASE // site 4 (tdd.md): source PHASE, prefix PHASE +// #4748: the split is at the first NON-DIGIT, not the first dot, so a letter +// suffix (`03A`, `23A.1.2`) rides through in the rest instead of aborting the +// base-10 arithmetic; the rest is `_REST`, no longer only a `_FRAC`. function fixedSnippet(sourceVar, prefix, indent = '') { - return `${indent}${prefix}_INT=\${${sourceVar}%%.*}; ${prefix}_FRAC=\${${sourceVar}#"$${prefix}_INT"}\n` + - `${indent}${prefix}_N="$((10#$${prefix}_INT))\${${prefix}_FRAC//./\\\\.}"`; + return `${indent}${prefix}_INT=\${${sourceVar}%%[!0-9]*}; ${prefix}_REST=\${${sourceVar}#"$${prefix}_INT"}\n` + + `${indent}${prefix}_N="$((10#$${prefix}_INT))\${${prefix}_REST//./\\\\.}"`; } function runFixed(phaseNumberValue) { @@ -64,6 +67,11 @@ describe('#4619 — execute-phase decimal/N-segment phase-number arithmetic', () assert.equal(runFixed('01'), '1'); }); + test('#4748: a letter-suffixed phase number keeps its letter and zero-strips the digit run (03A -> 3A, 23A.1.2 -> 23A\\.1\\.2)', () => { + assert.equal(runFixed('03A'), '3A'); + assert.equal(runFixed('23A.1.2'), '23A\\.1\\.2'); + }); + test('failing-first: the OLD $((10#...)) form is a hard shell syntax error on a decimal phase number', () => { assert.throws(() => { execFileSync('bash', ['-c', 'echo $((10#01.1))'], { encoding: 'utf8', timeout: TIMEOUT }); @@ -123,13 +131,13 @@ describe('#4619 — execute-phase decimal/N-segment phase-number arithmetic', () }); describe('source parity — each of the 4 production sites carries the fixed logic', () => { - test('execute-phase.md safe_resume_gate carries the fixed PHASE_NUMBER/PHASE_INT/PHASE_FRAC/PHASE_N logic', () => { + test('execute-phase.md safe_resume_gate carries the fixed PHASE_NUMBER/PHASE_INT/PHASE_REST/PHASE_N logic', () => { const w = fs.readFileSync(EXECUTE_PHASE, 'utf8'); assert.ok(w.includes(fixedSnippet('PHASE_NUMBER', 'PHASE')), 'safe_resume_gate must carry the byte-identical fixed decimal-tolerant snippet'); }); - test('execute-phase.md TDD gate carries the fixed PHASE_NUMBER/PHASE_INT/PHASE_FRAC/PHASE_N logic', () => { + test('execute-phase.md TDD gate carries the fixed PHASE_NUMBER/PHASE_INT/PHASE_REST/PHASE_N logic', () => { const w = fs.readFileSync(EXECUTE_PHASE, 'utf8'); // The TDD gate block is nested one level deeper (4-space indent) than // safe_resume_gate's top-level snippet. diff --git a/tests/fixtures/compact-content-benchmark-baseline.json b/tests/fixtures/compact-content-benchmark-baseline.json index ba29cdac1..cc6b87d62 100644 --- a/tests/fixtures/compact-content-benchmark-baseline.json +++ b/tests/fixtures/compact-content-benchmark-baseline.json @@ -18,9 +18,9 @@ "reductionPct": 16.51 }, "execute-phase": { - "offTokens": 26184, - "onTokens": 23894, - "reductionPct": 8.75 + "offTokens": 26344, + "onTokens": 24054, + "reductionPct": 8.69 }, "new-project": { "offTokens": 14308, @@ -39,8 +39,8 @@ } }, "aggregate": { - "offTokens": 109184, - "onTokens": 92497, - "reductionPct": 15.28 + "offTokens": 109344, + "onTokens": 92657, + "reductionPct": 15.26 } } diff --git a/tests/init.test.cjs b/tests/init.test.cjs index 93a6c720e..ac0e5d241 100644 --- a/tests/init.test.cjs +++ b/tests/init.test.cjs @@ -1087,6 +1087,34 @@ describe('init commands ROADMAP fallback when phase directory does not exist (#1 assert.strictEqual(output.phase_name, 'Foundation Setup'); assert.strictEqual(output.phase_slug, 'foundation-setup'); assert.strictEqual(output.phase_req_ids, 'R-01, R-02'); + // #4748: the ROADMAP fallback hands the workflow an UNPADDED number, and + // execute-phase.md used to re-pad it with `printf "%02d"` — which cannot + // pad a letter id and reads an already-padded `08` as octal. The + // normalized form is emitted here, like the plan-phase sibling above. + assert.strictEqual(output.padded_phase, '01'); + }); + + test('#4748 — init execute-phase emits padded_phase for a letter-suffixed phase, from a directory and from the ROADMAP fallback', () => { + fs.appendFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + '\n### Phase 3A: Letter Variant\n**Goal:** On disk\n\n### Phase 4B: Roadmap Only\n**Goal:** No directory yet\n', + ); + seedPhase(tmpDir, '03A-letter-variant', { '03A-01-PLAN.md': '# Plan' }); + + const onDisk = JSON.parse(runGsdTools('init execute-phase 3A', tmpDir).output); + assert.strictEqual(onDisk.phase_found, true); + assert.strictEqual(onDisk.phase_number, '03A'); + assert.strictEqual(onDisk.padded_phase, '03A'); + + const roadmapOnly = JSON.parse(runGsdTools('init execute-phase 4B', tmpDir).output); + assert.strictEqual(roadmapOnly.phase_found, true); + assert.strictEqual(roadmapOnly.phase_dir, null); + assert.strictEqual(roadmapOnly.phase_number, '4B'); + assert.strictEqual(roadmapOnly.padded_phase, '04B'); + + const missing = JSON.parse(runGsdTools('init execute-phase 9Z', tmpDir).output); + assert.strictEqual(missing.phase_found, false); + assert.strictEqual(missing.padded_phase, null); }); test('init verify-work falls back to ROADMAP when no phase directory exists', () => { diff --git a/tests/lint-phase-id-drift.test.cjs b/tests/lint-phase-id-drift.test.cjs index d59eb179a..463858901 100644 --- a/tests/lint-phase-id-drift.test.cjs +++ b/tests/lint-phase-id-drift.test.cjs @@ -12,6 +12,10 @@ const { scanMarkdownSingleSegmentPhaseRegex, findLetterlessPhaseMirrorDrift, scanMarkdownLetterlessPhaseMirror, + findDotOnlyIntegerSplitDrift, + findLooseDottedPhaseRegexDrift, + findShellPhasePrintfPadDrift, + scanMarkdownLetterAxisConsumers, } = require('../scripts/lint-phase-id-drift.cjs'); const ROOT = path.join(__dirname, '..'); @@ -212,3 +216,119 @@ test('findLetterlessPhaseMirrorDrift does NOT flag a site sanctioned with an HTM test('scanMarkdownLetterlessPhaseMirror against the real repo tree reports zero violations (#4660 fixed)', () => { assert.deepEqual(scanMarkdownLetterlessPhaseMirror(ROOT), []); }); + +// #4748 (epic #4634): the three letter-hostile CONSUMER shapes — a dot-only +// integer split, a `[0-9]+\.?[0-9]*` extraction, a `printf "%02d"` re-pad. + +// a. dot-only integer split +test('findDotOnlyIntegerSplitDrift flags the post-#4619 `PHASE_INT=${PHASE_NUMBER%%.*}` split', () => { + const text = 'PHASE_INT=${PHASE_NUMBER%%.*}; PHASE_FRAC=${PHASE_NUMBER#"$PHASE_INT"}'; + const found = findDotOnlyIntegerSplitDrift(text); + assert.equal(found.length, 1); + assert.equal(found[0].line, 1); + assert.equal(found[0].found, 'PHASE_INT=${PHASE_NUMBER%%.*}'); +}); + +test('findDotOnlyIntegerSplitDrift flags the prefixed SPOT_ variant and the bare $PHASE source', () => { + assert.equal(findDotOnlyIntegerSplitDrift('SPOT_PHASE_INT=${SPOT_PHASE_NUMBER%%.*}').length, 1); + assert.equal(findDotOnlyIntegerSplitDrift('PHASE_INT=${PHASE%%.*}; PHASE_FRAC=${PHASE#"$PHASE_INT"}').length, 1); +}); + +test('findDotOnlyIntegerSplitDrift flags the quoted spelling of an _INT split', () => { + assert.equal(findDotOnlyIntegerSplitDrift('PHASE_INT="${PHASE_NUMBER%%.*}"').length, 1); +}); + +test('findDotOnlyIntegerSplitDrift is SILENT on a dot split into a non-_INT name (a parent-phase derivation is correct as-is)', () => { + // gap-closure-artifacts.md: the parent of `03A.1` is `03A` — everything before + // the first dot, letter included. Not an integer, not fed to $((10#…)). + assert.deepEqual(findDotOnlyIntegerSplitDrift('PARENT_PHASE="${PHASE_NUMBER%%.*}"'), []); + assert.deepEqual(findDotOnlyIntegerSplitDrift('PHASE_PREFIX=${PHASE_NUMBER%%.*}'), []); +}); + +test('findDotOnlyIntegerSplitDrift is SILENT on the fixed first-non-digit split', () => { + const text = 'PHASE_INT=${PHASE_NUMBER%%[!0-9]*}; PHASE_REST=${PHASE_NUMBER#"$PHASE_INT"}'; + assert.deepEqual(findDotOnlyIntegerSplitDrift(text), []); +}); + +test('findDotOnlyIntegerSplitDrift does NOT flag a split of a non-phase variable (a version, a plan)', () => { + assert.deepEqual(findDotOnlyIntegerSplitDrift('MAJOR_INT=${VERSION%%.*}'), []); + assert.deepEqual(findDotOnlyIntegerSplitDrift('PLAN_INT=${PLAN_ID%%.*}'), []); +}); + +test('findDotOnlyIntegerSplitDrift skips a full-line comment and honours the HTML sanction', () => { + assert.deepEqual(findDotOnlyIntegerSplitDrift('# was: PHASE_INT=${PHASE_NUMBER%%.*}'), []); + const text = [ + '', + 'PHASE_INT=${PHASE_NUMBER%%.*}', + ].join('\n'); + assert.deepEqual(findDotOnlyIntegerSplitDrift(text), []); +}); + +// b. loose dotted extraction +test('findLooseDottedPhaseRegexDrift flags the `[0-9]+\\.?[0-9]*` grep -oE extraction on a phase-carrying line', () => { + const text = "FROM_PHASE=$(echo \"$ARGUMENTS\" | grep -oE '\\-\\-from\\s+[0-9]+\\.?[0-9]*' | awk '{print $2}')"; + const found = findLooseDottedPhaseRegexDrift(text); + assert.equal(found.length, 1); + assert.equal(found[0].found, '[0-9]+\\.?[0-9]*'); +}); + +test('findLooseDottedPhaseRegexDrift flags the \\d near-variant', () => { + assert.equal(findLooseDottedPhaseRegexDrift("PHASE=$(echo \"$ARGUMENTS\" | grep -oE '\\d+\\.?\\d*')").length, 1); +}); + +test('findLooseDottedPhaseRegexDrift is SILENT on the canonical `[0-9]+[A-Z]?(\\.[0-9]+)*` form', () => { + const text = "PHASE=$(echo \"$ARGUMENTS\" | grep -oE '[0-9]+[A-Z]?(\\.[0-9]+)*' | head -1)"; + assert.deepEqual(findLooseDottedPhaseRegexDrift(text), []); +}); + +test('findLooseDottedPhaseRegexDrift does NOT overlap the single-segment or letterless rules', () => { + const bounded = 'if ! [[ "$PADDED_PHASE" =~ ^[0-9]+(\\.[0-9]+)?$ ]]; then'; + const letterless = 'if ! [[ "$PADDED_PHASE" =~ ^[0-9]+(\\.[0-9]+)*$ ]]; then'; + assert.deepEqual(findLooseDottedPhaseRegexDrift(bounded), []); + assert.deepEqual(findLooseDottedPhaseRegexDrift(letterless), []); +}); + +test('findLooseDottedPhaseRegexDrift does NOT flag a non-phase-carrying line (a version) and honours the sanction', () => { + assert.deepEqual(findLooseDottedPhaseRegexDrift("MAJOR=$(echo \"$VERSION\" | grep -oE '[0-9]+\\.?[0-9]*')"), []); + const text = [ + '', + "PHASE=$(echo \"$ARGUMENTS\" | grep -oE '[0-9]+\\.?[0-9]*' | head -1)", + ].join('\n'); + assert.deepEqual(findLooseDottedPhaseRegexDrift(text), []); +}); + +// c. printf re-pad +test('findShellPhasePrintfPadDrift flags `printf "%02d"` of a phase variable, braced or bare', () => { + const found = findShellPhasePrintfPadDrift('PADDED=$(printf "%02d" "${PHASE_NUMBER}")'); + assert.equal(found.length, 1); + assert.equal(found[0].found, 'printf "%0…d" …$PHASE_NUMBER'); + assert.equal(findShellPhasePrintfPadDrift('PHASE=$(printf "%02d" "$PHASE")').length, 1); + assert.equal(findShellPhasePrintfPadDrift('PHASE=$(printf "%02d.%s" "${PHASE_MAJOR}" "${PHASE_MINOR}")').length, 1); +}); + +test('findShellPhasePrintfPadDrift flags the single-quoted format and a width without the zero flag', () => { + assert.equal(findShellPhasePrintfPadDrift("PADDED=$(printf '%02d' \"$PHASE_NUMBER\")").length, 1); + assert.equal(findShellPhasePrintfPadDrift('PADDED=$(printf "%2d" "$PHASE_NUMBER")').length, 1); +}); + +test('findShellPhasePrintfPadDrift is SILENT on a pad of an _INT via $((10#…)) and on init\'s padded_phase binding', () => { + assert.deepEqual(findShellPhasePrintfPadDrift('PHASE=$(printf "%02d" "$((10#$PHASE_INT))")${BASH_REMATCH[2]}'), []); + assert.deepEqual(findShellPhasePrintfPadDrift('PADDED="{padded_phase}"'), []); +}); + +test('findShellPhasePrintfPadDrift does NOT flag a pad of a non-phase variable (a plan number)', () => { + assert.deepEqual(findShellPhasePrintfPadDrift('PLAN_PADDED=$(printf "%02d" "$PLAN_ID")'), []); +}); + +test('findShellPhasePrintfPadDrift skips a full-line comment and honours the HTML sanction', () => { + assert.deepEqual(findShellPhasePrintfPadDrift('# PADDED=$(printf "%02d" "${PHASE_NUMBER}")'), []); + const text = [ + '', + 'PADDED=$(printf "%02d" "${PHASE_NUMBER}")', + ].join('\n'); + assert.deepEqual(findShellPhasePrintfPadDrift(text), []); +}); + +test('scanMarkdownLetterAxisConsumers against the real repo tree reports zero violations (#4748 fixed)', () => { + assert.deepEqual(scanMarkdownLetterAxisConsumers(ROOT), []); +}); diff --git a/tests/nsegment-phase-grammar.test.cjs b/tests/nsegment-phase-grammar.test.cjs index 891123b9e..bf4365700 100644 --- a/tests/nsegment-phase-grammar.test.cjs +++ b/tests/nsegment-phase-grammar.test.cjs @@ -191,6 +191,7 @@ describe('#4568 — plan-phase.md captures the full N-segment --research-phase v // The canonical grammar is read from the committed bin/lib mirror the other // grammar tests use (shell cannot import it; the test can). const { PHASE_NUMBER_TOKEN_SOURCE } = require('../gsd-core/bin/lib/phase-id.cjs'); +const { splitLines } = require('../gsd-core/bin/lib/text-lines.cjs'); const CANONICAL_ANCHORED = new RegExp('^(?:' + PHASE_NUMBER_TOKEN_SOURCE + ')$'); // Inputs the canonical grammar ACCEPTS. `03A` is what `normalizePhaseName('3A')` @@ -318,3 +319,290 @@ describe('#4660 — plan-phase.md captures the full letter-suffixed --research-p }); } }); + +// --------------------------------------------------------------------------- +// #4748 — the letter axis at the seven shell sites OUTSIDE #4660's six. These +// are not grammar mirrors but consumers of the id: the post-#4619 +// `PHASE_INT=${PHASE_NUMBER%%.*}; $((10#$PHASE_INT))` split (four sites), +// the `printf "%02d"` re-pad before the REVIEW.md lookup (one site), the +// `[0-9]+\.?[0-9]*` argument extraction (two files, four lines) and the +// legacy manual normalizer. On a letter-suffixed id the first aborts bash, +// the second prints the wrong file, the last two silently truncate. Same +// discipline as above: each site's live lines are read off disk by anchor +// and executed in a real bash subprocess. +// --------------------------------------------------------------------------- + +const EXECUTE_PHASE = path.join(__dirname, '..', 'gsd-core', 'workflows', 'execute-phase.md'); +const COMPLETION_RECONCILIATION = path.join( + __dirname, '..', 'gsd-core', 'workflows', 'execute-phase', 'steps', 'completion-reconciliation.md', +); +const TDD_REF = path.join(__dirname, '..', 'gsd-core', 'references', 'tdd.md'); +const AUTONOMOUS = path.join(__dirname, '..', 'gsd-core', 'workflows', 'autonomous.md'); +const PLAN_REVIEW_CONVERGENCE = path.join(__dirname, '..', 'gsd-core', 'workflows', 'plan-review-convergence.md'); +const PHASE_ARGUMENT_PARSING = path.join(__dirname, '..', 'gsd-core', 'references', 'phase-argument-parsing.md'); + +/** + * Pure: the indexes of every line containing `anchor`. Asserts the count so a + * site that is added, removed or renamed breaks this test loudly instead of + * silently narrowing what it covers (execute-phase.md carries the split TWICE + * — plan selection and the TDD gate — and both must stay under test). + */ +function findAnchoredLineIndexes(lines, anchor, expectedCount) { + const idx = []; + lines.forEach((l, i) => { + if (l.includes(anchor)) idx.push(i); + }); + assert.equal( + idx.length, + expectedCount, + `expected ${expectedCount} line(s) containing ${JSON.stringify(anchor)}, found ${idx.length}`, + ); + return idx; +} + +/** Run `script` in bash with `env` merged in; never throws — returns { status, stdout, stderr }. */ +function runBash(script, env) { + try { + const stdout = execFileSync('bash', [], { + input: script, + encoding: 'utf8', + timeout: TIMEOUT, + env: { ...process.env, ...env }, + stdio: ['pipe', 'pipe', 'pipe'], + }); + return { status: 0, stdout: stdout.trim(), stderr: '' }; + } catch (e) { + return { status: e.status, stdout: String(e.stdout || '').trim(), stderr: String(e.stderr || '').trim() }; + } +} + +// What each Class 1 site must compute from the id it is handed: the integer +// half zero-stripped for the anchored `0*` commit-scope ERE, everything after +// it carried through with dots escaped. `03A` is the padded form `init` emits +// for a `03A-slug/` directory; `12A` / `3A` are the bare forms; `03A.1.2` is +// the letter-and-N-segment combination the canonical grammar admits. +const CLASS1_CASES = [ + // [PHASE_NUMBER, expected PHASE_N] + ['03A', '3A'], + ['12A', '12A'], + ['3A', '3A'], + ['03A.1.2', '3A\\.1\\.2'], +]; +const CLASS1_CONTROLS = [ + ['06', '6'], + ['7', '7'], + ['08.5', '8\\.5'], + ['23.1.2', '23\\.1\\.2'], +]; + +describe('#4748 — the $((10#$PHASE_INT)) split sites carry a letter suffix into PHASE_N instead of aborting', () => { + const sites = [ + // execute-phase.md: plan selection (safe_resume_gate) and the TDD gate a + // few lines below are the same two lines twice; both must be under test. + { name: 'execute-phase.md', file: EXECUTE_PHASE, anchor: 'PHASE_INT=${PHASE_NUMBER%%', count: 2, input: 'PHASE_NUMBER', output: 'PHASE_N' }, + { name: 'completion-reconciliation.md', file: COMPLETION_RECONCILIATION, anchor: 'SPOT_PHASE_INT=${SPOT_PHASE_NUMBER%%', count: 1, input: 'SPOT_PHASE_NUMBER', output: 'SPOT_PHASE_N' }, + { name: 'tdd.md', file: TDD_REF, anchor: 'PHASE_INT=${PHASE%%', count: 1, input: 'PHASE', output: 'PHASE_N' }, + ]; + + for (const site of sites) { + describe(site.name, () => { + const lines = splitLines(fs.readFileSync(site.file, 'utf8')); + const indexes = findAnchoredLineIndexes(lines, site.anchor, site.count); + + indexes.forEach((i, n) => { + // The split line and the PHASE_N line directly below it, verbatim. + const splitLine = lines[i].trim(); + const nLine = lines[i + 1].trim(); + assert.ok(nLine.startsWith(`${site.output}=`), `line after the split must assign ${site.output}: ${nLine}`); + const snippet = ['set -e', splitLine, nLine, `printf '%s' "$${site.output}"`].join('\n'); + const label = site.count > 1 ? ` (occurrence ${n + 1})` : ''; + + for (const [id, expected] of CLASS1_CASES) { + test(`${id} → ${site.output}=${expected} without a shell error${label} (fails before the fix)`, () => { + const r = runBash(snippet, { [site.input]: id }); + assert.equal(r.status, 0, `bash exited ${r.status}: ${r.stderr}`); + assert.equal(r.stdout, expected); + }); + } + + for (const [id, expected] of CLASS1_CONTROLS) { + test(`regression control: ${id} → ${site.output}=${expected}${label}`, () => { + const r = runBash(snippet, { [site.input]: id }); + assert.equal(r.status, 0, `bash exited ${r.status}: ${r.stderr}`); + assert.equal(r.stdout, expected); + }); + } + + test(`the commit-scope ERE built from PHASE_N matches both the padded and the unpadded scope of a letter phase${label}`, () => { + // Each site feeds PHASE_N into `^[a-z]+\((0*${PHASE_N})-(0*${PLAN_N})\):` + // — the #4003 zero-pad-tolerant scope. Prove the value it now yields + // for `03A` matches the two subjects an executor could have written, + // and does NOT match the letter-less phase 3. + const script = [ + 'set -e', + splitLine, + nLine, + `SCOPE_RE="^[a-z]+\\((0*\${${site.output}})-(0*1)\\):"`, + 'for s in "feat(3A-01): x" "feat(03A-1): x"; do printf \'%s\\n\' "$s" | grep -qE "$SCOPE_RE" || { echo "MISS $s"; exit 3; }; done', + 'printf \'%s\\n\' "feat(3-01): x" | grep -qE "$SCOPE_RE" && { echo "FALSE-MATCH"; exit 4; }', + 'echo OK', + ].join('\n'); + const r = runBash(script, { [site.input]: '03A' }); + assert.equal(r.status, 0, `${r.stdout} ${r.stderr}`); + assert.equal(r.stdout, 'OK'); + }); + }); + }); + } +}); + +describe('#4748 — execute-phase.md resolves the REVIEW.md path from init\'s padded_phase, not a shell re-pad', () => { + const lines = splitLines(fs.readFileSync(EXECUTE_PHASE, 'utf8')); + const [i] = findAnchoredLineIndexes(lines, 'REVIEW_FILE="${PHASE_DIR}/${PADDED}-REVIEW.md"', 1); + const paddedLine = lines[i - 1].trim(); + + test('the PADDED binding directly above the lookup reads {padded_phase} (fails before the fix)', () => { + // `printf "%02d"` cannot pad `03A` (prints `03`, exits 1) — and cannot + // even re-pad an already-padded `08` (bash reads it as octal, prints + // `00`). `init execute-phase` now emits `padded_phase` through the + // canonical normalizer, so the workflow binds it instead of re-deriving. + assert.ok(paddedLine.startsWith('PADDED='), `line above the lookup must bind PADDED: ${paddedLine}`); + assert.equal(paddedLine, 'PADDED="{padded_phase}"'); + }); + + test('regression control: the lookup line itself is unchanged', () => { + assert.equal(lines[i].trim(), 'REVIEW_FILE="${PHASE_DIR}/${PADDED}-REVIEW.md"'); + }); + + test('composition: the value init emits, substituted into the live lookup lines, resolves the letter phase\'s own REVIEW.md', (t) => { + // The model substitutes `{padded_phase}` from the init JSON, which is + // `normalizePhaseName(phase_number)` (src/init.cts). Do that substitution + // here and run the three live lines against a fixture, so the emitted + // value, the binding, the path construction and the status extraction are + // exercised together — the executable half of a `{template}` site. + const { normalizePhaseName } = require('../gsd-core/bin/lib/phase-id.cjs'); + const { createTempDir, cleanup } = require('./helpers.cjs'); + const dir = createTempDir(); + t.after(() => cleanup(dir)); + for (const [id, status] of [['3A', 'clean'], ['8', 'issues'], ['9', 'skipped']]) { + const emitted = normalizePhaseName(id); + fs.writeFileSync(path.join(dir, `${emitted}-REVIEW.md`), `---\nstatus: ${status}\n---\n# review\n`); + const script = [ + 'set -e', + paddedLine.replace('{padded_phase}', emitted), + lines[i].trim(), + lines[i + 1].trim(), + 'printf \'%s %s\' "$PADDED" "$REVIEW_STATUS"', + ].join('\n'); + assert.ok(lines[i + 1].includes('REVIEW_STATUS='), `line after the lookup must extract REVIEW_STATUS: ${lines[i + 1]}`); + const r = runBash(script, { PHASE_DIR: dir }); + assert.equal(r.status, 0, `bash exited ${r.status}: ${r.stderr}`); + assert.equal(r.stdout, `${emitted} ${status}`); + } + }); + + test('the workflow\'s init parse list names padded_phase, so the binding is not a literal (fails before the fix)', () => { + // A `{field}` token is substituted from the init JSON only for fields the + // workflow tells the model to parse; `phase_number` is on that list and + // `padded_phase` was not (adversarial review, claim 2). + const [p] = findAnchoredLineIndexes(lines, 'Parse JSON for: `executor_model`', 1); + assert.match(lines[p], /`phase_number`, `padded_phase`,/); + }); +}); + +describe('#4748 — autonomous.md --from/--to/--only and plan-review-convergence.md extract the full letter-suffixed phase', () => { + const autonomousText = fs.readFileSync(AUTONOMOUS, 'utf8'); + const prcText = fs.readFileSync(PLAN_REVIEW_CONVERGENCE, 'utf8'); + + const sites = [ + { name: 'autonomous.md --from', pattern: extractGrepPattern(autonomousText, 'FROM_PHASE=$(echo "$ARGUMENTS" | grep -oE'), args: (v) => `--from ${v}`, tail: "| awk '{print $2}'" }, + { name: 'autonomous.md --to', pattern: extractGrepPattern(autonomousText, 'TO_PHASE=$(echo "$ARGUMENTS" | grep -oE'), args: (v) => `--from 1 --to ${v}`, tail: "| awk '{print $2}'" }, + { name: 'autonomous.md --only', pattern: extractGrepPattern(autonomousText, 'ONLY_PHASE=$(echo "$ARGUMENTS" | grep -oE'), args: (v) => `--only ${v} --interactive`, tail: "| awk '{print $2}'" }, + { name: 'plan-review-convergence.md', pattern: extractGrepPattern(prcText, 'PHASE=$(echo "$ARGUMENTS" | grep -oE'), args: (v) => `${v} --codex --max-cycles 3`, tail: '| head -1' }, + ]; + + function extract(site, v) { + const script = `echo "$ARGUMENTS" | grep -oE '${site.pattern}' ${site.tail}`; + return runBash(script, { ARGUMENTS: site.args(v) }).stdout; + } + + for (const site of sites) { + describe(site.name, () => { + // Before the fix `[0-9]+\.?[0-9]*` stops at the letter: `12A` → `12`, + // silently targeting a different phase. `23.1.2` → `23.1` is the same + // truncation one axis over (#4568's class in a spelling neither lint saw). + for (const v of ['12A', '3A', '23A.1.2', '23.1.2']) { + test(`${v} extracts ${v}, not a truncated prefix (fails before the fix)`, () => { + const got = extract(site, v); + assert.equal(got, v); + assert.equal(CANONICAL_ANCHORED.test(got), true); + }); + } + + for (const v of ['6', '36.14']) { + test(`regression control: ${v} extracts ${v}`, () => { + assert.equal(extract(site, v), v); + }); + } + }); + } + + test('autonomous.md: the three flags extract independently from one argument string', () => { + const script = [ + `FROM_PHASE=$(echo "$ARGUMENTS" | grep -oE '${sites[0].pattern}' | awk '{print $2}')`, + `TO_PHASE=$(echo "$ARGUMENTS" | grep -oE '${sites[1].pattern}' | awk '{print $2}')`, + 'printf \'%s %s\' "$FROM_PHASE" "$TO_PHASE"', + ].join('\n'); + assert.equal(runBash(script, { ARGUMENTS: '--from 3A --to 5B --max-cycles 2' }).stdout, '3A 5B'); + }); +}); + +describe('#4748 — phase-argument-parsing.md\'s legacy normalizer pads a letter-suffixed id instead of leaving it alone', () => { + const lines = splitLines(fs.readFileSync(PHASE_ARGUMENT_PARSING, 'utf8')); + const [start] = findAnchoredLineIndexes(lines, '# Normalize phase number', 1); + let end = start; + while (end < lines.length && lines[end].trim() !== 'fi') end++; + assert.ok(end < lines.length, 'normalizer block must close with `fi`'); + const block = lines.slice(start, end + 1).join('\n'); + + function normalize(v) { + return runBash(`set -e\n${block}\nprintf '%s' "$PHASE"`, { PHASE: v }); + } + + // Before the fix neither branch matches a letter id, so `12A` passes through + // unpadded and `3A` is never zero-padded to the `03A` a directory carries. + for (const [input, expected] of [['3A', '03A'], ['12A', '12A'], ['3A.1', '03A.1'], ['23A.1.2', '23A.1.2']]) { + test(`${input} → ${expected} (fails before the fix)`, () => { + const r = normalize(input); + assert.equal(r.status, 0, `bash exited ${r.status}: ${r.stderr}`); + assert.equal(r.stdout, expected); + assert.equal(CANONICAL_ANCHORED.test(r.stdout), true); + }); + } + + // `08` is the octal trap: `printf "%02d" 08` is an invalid octal number in + // bash (exit 1, prints `00`), so the old integer branch mangled any + // already-padded id it was handed. `23.1.2` matched neither old branch and + // passed through unchanged — the N-segment axis was silently unpadded. + for (const [input, expected] of [['08', '08'], ['23.1.2', '23.1.2']]) { + test(`${input} → ${expected} without a shell error (fails before the fix)`, () => { + const r = normalize(input); + assert.equal(r.status, 0, `bash exited ${r.status}: ${r.stderr}`); + assert.equal(r.stdout, expected); + }); + } + + for (const [input, expected] of [['8', '08'], ['2.1', '02.1'], ['36.14', '36.14']]) { + test(`regression control: ${input} → ${expected}`, () => { + const r = normalize(input); + assert.equal(r.status, 0, `bash exited ${r.status}: ${r.stderr}`); + assert.equal(r.stdout, expected); + }); + } + + test('a non-canonical value passes through untouched (the normalizer is not a validator)', () => { + const r = normalize('AUTH-101'); + assert.equal(r.status, 0); + assert.equal(r.stdout, 'AUTH-101'); + }); +}); diff --git a/tests/safe-resume-gate-anchoring.test.cjs b/tests/safe-resume-gate-anchoring.test.cjs index 91d1a6fb1..a04596d01 100644 --- a/tests/safe-resume-gate-anchoring.test.cjs +++ b/tests/safe-resume-gate-anchoring.test.cjs @@ -31,9 +31,9 @@ describe('#4003 — safe_resume_gate commit-scope greps', () => { const w = fs.readFileSync(WORKFLOW, 'utf8'); // Anchored, ERE, zero-pad-tolerant on BOTH components — matches feat(2-02): and // feat(02-02): alike, never a substring elsewhere in the message. - assert.ok(w.includes('PHASE_INT=${PHASE_NUMBER%%.*}; PHASE_FRAC=${PHASE_NUMBER#"$PHASE_INT"}') && - w.includes('PHASE_N="$((10#$PHASE_INT))${PHASE_FRAC//./\\\\.}"'), - 'phase component must be zero-stripped via arithmetic base-10 (#4619: leading integer segment only, decimal/N-segment tolerant)'); + assert.ok(w.includes('PHASE_INT=${PHASE_NUMBER%%[!0-9]*}; PHASE_REST=${PHASE_NUMBER#"$PHASE_INT"}') && + w.includes('PHASE_N="$((10#$PHASE_INT))${PHASE_REST//./\\\\.}"'), + 'phase component must be zero-stripped via arithmetic base-10 (#4619: leading integer segment only, decimal/N-segment tolerant; #4748: split at the first non-digit so a letter suffix rides through)'); assert.ok(w.includes('PLAN_N=$((10#{plan_padded}))'), 'plan component must be zero-stripped via arithmetic base-10'); assert.ok(w.includes('PLAN_SCOPE_RE="^[a-z]+\\((0*${PHASE_N})-(0*${PLAN_N})\\):"'), @@ -60,9 +60,9 @@ describe('#4003 — safe_resume_gate commit-scope greps', () => { test('tdd red gate tolerates both commit-scope spellings (#4011 keying untouched)', () => { const w = fs.readFileSync(WORKFLOW, 'utf8'); - assert.ok(w.includes('PHASE_INT=${PHASE_NUMBER%%.*}; PHASE_FRAC=${PHASE_NUMBER#"$PHASE_INT"}') && - w.includes('PHASE_N="$((10#$PHASE_INT))${PHASE_FRAC//./\\\\.}"') && w.includes('PLAN_N=$((10#${PLAN_ID}))'), - 'the TDD block derives zero-stripped components (#4619: leading integer segment only, decimal/N-segment tolerant)'); + assert.ok(w.includes('PHASE_INT=${PHASE_NUMBER%%[!0-9]*}; PHASE_REST=${PHASE_NUMBER#"$PHASE_INT"}') && + w.includes('PHASE_N="$((10#$PHASE_INT))${PHASE_REST//./\\\\.}"') && w.includes('PLAN_N=$((10#${PLAN_ID}))'), + 'the TDD block derives zero-stripped components (#4619: leading integer segment only, decimal/N-segment tolerant; #4748: letter-suffix tolerant)'); assert.ok(w.includes('RED_COMMIT=$(git log --oneline -E ${TDD_MILESTONE_BASE:+"$TDD_MILESTONE_BASE..HEAD"} --grep="${PLAN_SCOPE_RE}" -- "*.test.*"'), 'the RED grep must use the same anchored padding-tolerant scope, milestone-bounded'); assert.ok(!w.includes('--grep="^test(${PHASE_NUMBER}-${PLAN_ID})"'), @@ -80,9 +80,9 @@ describe('#4003 — safe_resume_gate commit-scope greps', () => { 'the padded-literal example grep must not remain'); assert.ok(ref.includes('--grep="^test\\((0*${PHASE_N})-(0*${PLAN_N})\\):"'), 'the RED example is anchored and zero-pad-tolerant'); - assert.ok(ref.includes('PHASE_INT=${PHASE%%.*}; PHASE_FRAC=${PHASE#"$PHASE_INT"}') && - ref.includes('PHASE_N="$((10#$PHASE_INT))${PHASE_FRAC//./\\\\.}"') && ref.includes('PLAN_N=$((10#${PLAN}))'), - 'the examples derive zero-stripped components (#4619: leading integer segment only, decimal/N-segment tolerant)'); + assert.ok(ref.includes('PHASE_INT=${PHASE%%[!0-9]*}; PHASE_REST=${PHASE#"$PHASE_INT"}') && + ref.includes('PHASE_N="$((10#$PHASE_INT))${PHASE_REST//./\\\\.}"') && ref.includes('PLAN_N=$((10#${PLAN}))'), + 'the examples derive zero-stripped components (#4619: leading integer segment only, decimal/N-segment tolerant; #4748: letter-suffix tolerant)'); }); test('completion spot-check uses the anchored scope and keeps its time bound', () => { @@ -96,10 +96,10 @@ describe('#4003 — safe_resume_gate commit-scope greps', () => { 'execute-phase', 'steps', 'completion-reconciliation.md'), 'utf8'); assert.ok(!w.includes('--grep="{phase_number}-{plan_padded}"') && !frag.includes('--grep="{phase_number}-{plan_padded}"'), 'the raw padded placeholder substring grep must not remain'); - assert.ok(frag.includes('SPOT_PHASE_INT=${SPOT_PHASE_NUMBER%%.*}; SPOT_PHASE_FRAC=${SPOT_PHASE_NUMBER#"$SPOT_PHASE_INT"}') && - frag.includes('SPOT_PHASE_N="$((10#$SPOT_PHASE_INT))${SPOT_PHASE_FRAC//./\\\\.}"') && + assert.ok(frag.includes('SPOT_PHASE_INT=${SPOT_PHASE_NUMBER%%[!0-9]*}; SPOT_PHASE_REST=${SPOT_PHASE_NUMBER#"$SPOT_PHASE_INT"}') && + frag.includes('SPOT_PHASE_N="$((10#$SPOT_PHASE_INT))${SPOT_PHASE_REST//./\\\\.}"') && frag.includes('SPOT_PLAN_N=$((10#{plan_padded}))'), - 'the spot-check derives zero-stripped components (#4619: leading integer segment only, decimal/N-segment tolerant)'); + 'the spot-check derives zero-stripped components (#4619: leading integer segment only, decimal/N-segment tolerant; #4748: letter-suffix tolerant)'); assert.ok(frag.includes('--since="1 hour ago"'), 'the spot-check keeps its temporal bound'); });