golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

enhance(#3883): route every slug call site through its canonical owner (#3896)

Browse Source 
* docs(#3883): correct section 8.3 against the tree

I wrote this section in Phase 0 stating rules I had not executed against the
tree. Measured at 832dcbb75, three statements are wrong.

The slug count is 11 across 5 files, not 13; two of the thirteen were an
unrelated tokenizer regex. The divergence is real and reproduced.

resolveRuntime reads no install marker at all and has no cache; PR #3382,
cited as prior art for that rung, is closed unmerged.

The Codex sandbox is still a hand-maintained subset map with a silent
read-only fallback, and validate agents checks file presence only -- both
halves of that claim are false.

The shortFormToId rule is accurate. The guard roster names no casualty for
this rule.

Section 8.3 is therefore a work list, not a conformance check.

Refs #3883

* test(#3883): failing-first rows for slug re-implementation divergence

Refs #3883

* feat(#3883): route every slug call site through its canonical owner

Migrated commands.cts:209, init.cts:176/1935/1957/3109, phase-id.cts:229/380, phase-locator.cts:269, workstream-name-policy.cts:75 to generateSlugInternal (core-utils.cts:107).

Declared different: gsd2-import.cts:97 (no truncation), active-workstream-store.cts:97 (fixed ASCII env-key domain, already matches).

Fixed a latent circular-require bug: core-utils.cts top-level destructured comparePhaseNum/scopeToPhase from phase-id.cjs; switched both sides of the new circular require to lazy function-body requires.

Refs #3883

* fix(#3883): restore per-site truncation contracts broken by the slug consolidation

Refs #3883

* fix(#3883): close review findings — changeset, miscounts, loose rows, cyclic destructure

Refs #3883

* docs(#3883): correct the same slug miscount in the Context table

Section 8.3's rule text was corrected to 11 copies across 7 files; the Context
table above it still carried the original 13 across 5. Same wrong claim, second
location, both mine.

Refs #3883

* chore(#3883): backfill changeset PR number

Refs #3883

* test(#3883): keep the core-utils mutation shard inside its time budget

PR #3896's Stryker (core-utils) shard was cancelled at the 15-minute
shard cap. Stryker's command-runner bills one `node --test <file>`
invocation as a single unit costing whatever the file's slowest run
costs, re-run once per mutant (documented in
tests/state-contract.test.cjs's header, #2790 precedent). A3/A5 drove
every CLI-reachable slug site through runGsdTools (a real child-process
spawn per call, ~85-170ms each across ~70 calls), accounting for ~7.1s
of the file's ~7.9s wall time.

commands.cts:cmdGenerateSlug and init.cts:cmdInitExecutePhase/
cmdInitPhaseOp/cmdInitProgress are plain functions reachable in-process
from the built gsd-core/bin/lib/*.cjs, so this calls them directly
instead of spawning gsd-tools, capturing their fd-1 JSON output with the
bug #1008 fs.writeSync-mock pattern already used in tests/io.test.cjs
and tests/init.test.cjs. A hermetic-env helper reproduces the isolation
runGsdTools's { HOME: tmpDir } + testEnvBase() gave the child process.

File wall time drops from ~7.97s to ~1.1s (246/246 passing, same
coverage), well inside the 15-minute cap even at hundreds of mutants.

Refs #3883

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-26 16:03:11 -04:00
committed by GitHub
parent a638ca4332
commit 389bc86e02
11 changed files with 748 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
src/active-workstream-store.cts
View File

@@ -91,6 +91,14 @@ function getControllingTtyToken(): string | null {
}
function getWorkstreamSessionKey(): string | null {
// #3883 (ADR-3473 §8.3): DECLARED DIFFERENT from generateSlugInternal
// (core-utils.cts), the canonical slug formula — deliberately, not a
// consolidation gap. The text slugified here is never caller-supplied: it
// is always one of the fixed ASCII WORKSTREAM_SESSION_ENV_KEYS identifier
// names above (e.g. "GSD_SESSION_KEY"), which already agree with the
// canonical's output byte-for-byte on this restricted input domain (no
// unicode, no length anywhere near the 60-char truncation boundary) — so
// delegating would add a require() edge for zero behavioral change.
for (const envKey of WORKSTREAM_SESSION_ENV_KEYS) {
const raw = process.env[envKey];
const token = sanitizeWorkstreamSessionToken(raw);

10
src/commands.cts
View File

@@ -206,11 +206,11 @@ function cmdGenerateSlug(text: string | undefined, raw: boolean): void {
error('text required for slug generation');
}
const slug = (text as string)
.toLowerCase()
.replace(/[^a-z0-9]+/g, '-')
.replace(/^-+|-+$/g, '')
.substring(0, 60);
// #3883 (ADR-3473 §8.3): delegate to the canonical slug formula
// (generateSlugInternal, core-utils.cts) instead of re-implementing it —
// this call site previously diverged from it (Cyrillic collapsed to "",
// and truncation could leave a trailing hyphen; #2848/#2849).
const slug = coreUtilsMod.generateSlugInternal(text) ?? '';
const result = { slug };
output(result, raw, slug);

42
src/core-utils.cts
View File

@@ -14,16 +14,30 @@
* - node:fs / node:path (stdlib)
* - ./phase-id.cjs (comparePhaseNum, used by readSubdirectories)
* - ./planning-workspace.cjs (findContextMdIn, used by getPhaseFileStats)
*
* #3883 (ADR-3473 §8.3): two of this module's cyclic partners require
* generateSlugInternal, the canonical slug formula:
* - phase-id.cjs requires this module directly.
* - planning-workspace.cjs is a cyclic partner via a longer path:
* core-utils.cjs -> planning-workspace.cjs -> active-workstream-store.cjs
* -> workstream-name-policy.cjs -> core-utils.cjs.
* Both are genuine circular requires. They are safe ONLY because every side
* accesses the other's exports lazily, through the live module-namespace
* object (`phaseIdModule.foo(...)` / `planningWorkspace.foo(...)`) inside a
* function body, never via a top-level destructure — a top-level
* `const { foo } = require(...)` copies the binding at import time and would
* silently capture `undefined` whichever module loses the load-order race.
* This is an absolute rule with no exception in this file: every cyclic
* partner's export is accessed through its module-namespace object, never
* destructured at the top level.
*/
import fs from 'node:fs';
import path from 'node:path';
// eslint-disable-next-line @typescript-eslint/no-require-imports
import phaseIdModule = require('./phase-id.cjs');
const { comparePhaseNum, scopeToPhase } = phaseIdModule;
// eslint-disable-next-line @typescript-eslint/no-require-imports
import planningWorkspace = require('./planning-workspace.cjs');
const { findContextMdIn } = planningWorkspace;
// eslint-disable-next-line @typescript-eslint/no-require-imports
import shellCommandProjection = require('./shell-command-projection.cjs');
@@ -104,14 +118,28 @@ function pathExistsInternal(cwd: string, targetPath: string): boolean {
}
}
function generateSlugInternal(text: string | null | undefined): string | null {
/**
* #3883 (ADR-3473 §8.3 remediation): `maxLen` lets a caller state its own
* truncation contract instead of being forced into this function's
* historical 60-char cap. Some call sites truncated at 60 before the #3883
* consolidation (commands.cts:cmdGenerateSlug) and some never truncated at
* all (phase-id.cts toDir/getPhaseDirFromPhaseId, the init.cts/phase-locator
* phase_slug sites, workstream-name-policy.cts toWorkstreamSlug) — collapsing
* every caller onto a single hard-coded 60 introduced two identity
* collisions (distinct >60-char names/phase-slugs truncating to the same
* value) that did not exist pre-migration. `maxLen: 60` remains the default
* so untouched callers keep prior behavior; pass `null` for no truncation.
*/
function generateSlugInternal(text: string | null | undefined, maxLen: number | null = 60): string | null {
if (!text) return null;
// #2849: strip leading/trailing hyphens AFTER truncation, not only before.
// .substring(0, 60) can land on a separator, re-introducing a trailing hyphen
// the strip step exists to prevent. Truncation cannot add a leading hyphen, so
// running the full ^-+|-+$ pass last is equivalent for leading hyphens and
// fixes the trailing-hyphen-after-truncation case.
return transliterateForSlug(text).replace(/[^a-z0-9]+/g, '-').substring(0, 60).replace(/^-+|-+$/g, '');
const collapsed = transliterateForSlug(text).replace(/[^a-z0-9]+/g, '-');
const truncated = maxLen === null ? collapsed : collapsed.substring(0, maxLen);
return truncated.replace(/^-+|-+$/g, '');
}
// ─── Transliteration (#2848) ─────────────────────────────────────────────────
@@ -234,13 +262,13 @@ function getPhaseFileStats(phaseDir: string): PhaseFileStats {
};
}
const scopedFiles = scopeToPhase(files, path.basename(phaseDir));
const scopedFiles = phaseIdModule.scopeToPhase(files, path.basename(phaseDir));
return {
plans: scan.planFiles,
summaries: scan.summaryFiles,
hasResearch: scopedFiles.some(f => f.endsWith('-RESEARCH.md') || f === 'RESEARCH.md'),
hasContext: findContextMdIn(scopedFiles) !== null,
hasContext: planningWorkspace.findContextMdIn(scopedFiles) !== null,
hasVerification: scopedFiles.some(f => f.endsWith('-VERIFICATION.md') || f === 'VERIFICATION.md'),
hasReviews: scopedFiles.some(f => f.endsWith('-REVIEWS.md') || f === 'REVIEWS.md'),
scope: scan.scope,
@@ -256,7 +284,7 @@ function readSubdirectories(dirPath: string, sort = false): string[] {
try {
const entries = fs.readdirSync(dirPath, { withFileTypes: true });
const dirs = entries.filter(e => e.isDirectory()).map(e => e.name);
return sort ? dirs.sort((a, b) => comparePhaseNum(a, b)) : dirs;
return sort ? dirs.sort((a, b) => phaseIdModule.comparePhaseNum(a, b)) : dirs;
} catch {
return [];
}

31
src/init.cts
View File

@@ -174,9 +174,12 @@ function guardedGetRoadmapPhase(
// directory exists yet) identically at every synthetic-fallback call site
// below — factored out once so the slugification formula itself cannot drift.
function slugifyPhaseName(phaseName: string | null): string | null {
return phaseName
? phaseName.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, '')
: null;
// #3883 (ADR-3473 §8.3): delegate to the canonical slug formula
// (generateSlugInternal, core-utils.cts) rather than re-implementing it.
// `maxLen: null` preserves this site's pre-migration untruncated contract —
// the 60-char default would collapse two distinct >60-char phase names onto
// the same reported phase_slug.
return phaseName ? coreUtils.generateSlugInternal(phaseName, null) : null;
}
/**
@@ -1931,9 +1934,11 @@ function cmdInitPhaseOp(cwd: string, phase: string, raw: boolean): void {
directory: null,
phase_number: roadmapPhase['phase_number'],
phase_name: phaseName,
phase_slug: phaseName
? phaseName.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, '')
: null,
// #3883 (ADR-3473 §8.3): delegate to the canonical slug formula
// (generateSlugInternal, core-utils.cts) rather than re-implementing
// it. `maxLen: null` preserves this site's pre-migration untruncated
// contract.
phase_slug: phaseName ? coreUtils.generateSlugInternal(phaseName, null) : null,
plans: [],
summaries: [],
incomplete_plans: [],
@@ -1953,9 +1958,11 @@ function cmdInitPhaseOp(cwd: string, phase: string, raw: boolean): void {
directory: null,
phase_number: roadmapPhase['phase_number'],
phase_name: phaseName,
phase_slug: phaseName
? phaseName.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, '')
: null,
// #3883 (ADR-3473 §8.3): delegate to the canonical slug formula
// (generateSlugInternal, core-utils.cts) rather than re-implementing
// it. `maxLen: null` preserves this site's pre-migration untruncated
// contract.
phase_slug: phaseName ? coreUtils.generateSlugInternal(phaseName, null) : null,
plans: [],
summaries: [],
incomplete_plans: [],
@@ -3111,7 +3118,11 @@ function cmdInitProgress(cwd: string, raw: boolean, options: Record<string, unkn
const status = 'not_started';
const phaseInfo: Record<string, unknown> = {
number: num,
name: name.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, ''),
// #3883 (ADR-3473 §8.3): delegate to the canonical slug formula
// (generateSlugInternal, core-utils.cts) rather than re-implementing
// it. `maxLen: null` preserves this site's pre-migration untruncated
// contract.
name: coreUtils.generateSlugInternal(name, null) ?? '',
directory: null,
status,
plan_count: 0,

40
src/phase-id.cts
View File

@@ -10,6 +10,17 @@
* Dependencies:
* - ./pattern.cjs (escapeRegex — #3212 Phase 1 seam; this module is no
* longer the owner of pattern-escaping, only a consumer)
* - ./core-utils.cjs (generateSlugInternal — #3883/ADR-3473 §8.3: the
* canonical slug formula). core-utils.cjs also requires THIS module
* (comparePhaseNum, scopeToPhase), so a top-level require here would be
* circular and — per this codebase's compiled-.cjs convention of a
* single `module.exports = {...}` reassignment at the bottom of each
* file — a top-level circular require captures a stale, still-empty
* exports object forever (verified live: it throws
* "generateSlugInternal is not a function" when core-utils.cjs happens
* to load first). The require is deferred (lazy, inside each function
* body) instead, mirroring the same cycle-break already used by
* core-utils.cts's own getPhaseFileStats/plan-scan.cjs seam.
*/
import { escapeRegex } from './pattern.cjs';
@@ -226,9 +237,15 @@ function getPhaseDirFromPhaseId(phaseId: unknown, phaseName: string | null | und
const milestone = String(parseInt(m[1], 10)).padStart(2, '0');
const subParts = m[2].split('-').map(p => String(parseInt(p, 10)).padStart(2, '0'));
const sub = subParts.join('-');
const slug = phaseName
? phaseName.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, '')
: '';
// #3883 (ADR-3473 §8.3): delegate to the canonical slug formula
// (generateSlugInternal, core-utils.cts) rather than re-implementing it.
// `maxLen: null` preserves this site's pre-migration untruncated contract —
// the 60-char default would silently shadow one on-disk phase dir's
// reported phase_slug behind another distinct >60-char phase name's.
// Lazy require to break the core-utils.cjs <-> phase-id.cjs cycle (see the
// module dependency doc comment above).
// eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call
const slug = phaseName ? ((require('./core-utils.cjs').generateSlugInternal(phaseName, null) as string | null) ?? '') : '';
const parts = [milestone, sub, slug].filter(Boolean);
const base = parts.join('-');
return projectCode ? `${projectCode}-${base}` : base;
@@ -377,7 +394,22 @@ function toDir(id: PhaseId, slug: string): string {
const sub = id.subphase ? `.${id.subphase}` : '';
// Slug guard: the slug becomes an on-disk path segment, so collapse it to a
// safe lowercase token — never a path separator or `..` traversal.
const safeSlug = slug.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, '');
// #3883 (ADR-3473 §8.3): delegate the sanitize formula itself to the
// canonical (generateSlugInternal, core-utils.cts) — this fixes the
// Cyrillic-collapses-to-empty defect (#2848-class) that toDir carried
// before (it never transliterated). The empty-sanitize and all-digit
// throw guards below stay: they are a DECLARED DIFFERENCE from every
// other slug call site, not a bug — a slug here becomes a real directory
// name, and toDir protects the parsePhaseId dir↔identity bijection
// (see the toDir docstring above) by refusing to emit an unusable name,
// where every other site silently accepts "" or a re-truncated value.
// `maxLen: null` preserves toDir's pre-migration untruncated contract — the
// 60-char default let two distinct >60-char phase names collapse onto the
// identical directory name, one silently shadowing the other on disk.
// Lazy require to break the core-utils.cjs <-> phase-id.cjs cycle (see the
// module dependency doc comment above).
// eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call
const safeSlug = (require('./core-utils.cjs').generateSlugInternal(slug, null) as string | null) ?? '';
// A slug that sanitizes to nothing (e.g. '!!!') would otherwise emit a
// dangling trailing hyphen.
if (!safeSlug) {

7
src/phase-locator.cts
View File

@@ -266,7 +266,12 @@ function searchPhaseInDir(baseDir: string, relBase: string, normalized: string):
directory: toPosixPath(path.join(relBase, match)),
phase_number: phaseNumber,
phase_name: phaseName,
phase_slug: phaseName ? phaseName.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, '') : null,
// #3883 (ADR-3473 §8.3): delegate to the canonical slug formula
// (generateSlugInternal, core-utils.cts) rather than re-implementing
// it. `maxLen: null` preserves this site's pre-migration untruncated
// contract — the 60-char default would drop an on-disk phase slug's
// reported value out of sync with the real directory name.
phase_slug: phaseName ? coreUtilsModule.generateSlugInternal(phaseName, null) : null,
plans,
summaries,
incomplete_plans: incompletePlans,

29
src/workstream-name-policy.cts
View File

@@ -5,6 +5,24 @@
* byte-for-behaviour from the prior hand-written .cjs; only types are added.
*
* Used by active-workstream-store.cjs, planning-workspace.cjs, workstream.cjs.
*
* #3883 (ADR-3473 §8.3): toWorkstreamSlug delegates to core-utils.cjs's
* generateSlugInternal (the canonical slug formula), passing `maxLen: null`
* to preserve this site's pre-migration untruncated contract — the 60-char
* default collided distinct >60-char workstream names onto the same slug
* (verified: `"a".repeat(60)+"alpha"` and `"a".repeat(60)+"beta"` truncated
* identically). core-utils.cjs already
* requires (transitively, at module-init time) THIS module:
* core-utils.cjs -> planning-workspace.cjs -> active-workstream-store.cjs ->
* workstream-name-policy.cjs. A top-level require of core-utils.cjs here
* would therefore close that cycle and — per this codebase's compiled-.cjs
* convention of a single `module.exports = {...}` reassignment at the bottom
* of core-utils.cjs — capture a stale, still-empty exports object forever
* (verified live: "generateSlugInternal is not a function" whichever module
* loads first). The require is deferred (lazy, inside toWorkstreamSlug's
* body) instead, mirroring the same cycle-break already used by
* core-utils.cts's own getPhaseFileStats/plan-scan.cjs seam and by
* phase-id.cts's toDir/getPhaseDirFromPhaseId.
*/
export const INVALID_ACTIVE_WORKSTREAM_NAME_MESSAGE =
@@ -73,10 +91,13 @@ export function validateWorkstreamName(name: string | null | undefined): boolean
* Lowercases, collapses non-alphanumeric runs to hyphens, strips leading/trailing hyphens.
*/
export function toWorkstreamSlug(name: string | null | undefined): string {
return String(name ?? '')
.toLowerCase()
.replace(/[^a-z0-9]+/g, '-')
.replace(/^-+|-+$/g, '');
// #3883 (ADR-3473 §8.3): delegate to the canonical slug formula
// (generateSlugInternal, core-utils.cts) rather than re-implementing it —
// this call site previously diverged from it (no transliteration, no
// 60-char truncation). Lazy require to break the core-utils.cjs cycle
// (see the module dependency doc comment above).
// eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call
return (require('./core-utils.cjs').generateSlugInternal(String(name ?? ''), null) as string | null) ?? '';
}
/**