fix(#5186): resolve custom phase directories consistently by bshiggins · Pull Request #5509 · open-gsd/gsd-core

bshiggins · 2026-10-18T00:13:54Z

Fix PR

Linked Issue

What was broken

With phase_naming: custom, phase add --id PRODUCT-BOUNDARY wrote a directory named [CODE-]PRODUCT-BOUNDARY-<slug> that nothing could resolve by its id: find-phase PRODUCT-BOUNDARY reported not found, init.plan-phase returned a null phase_dir while expected_phase_dir named the existing directory, and roadmap.update-plan-progress reported the phase missing. It happened with or without a project_code.

What this fix does

The directory shape custom writers produce and the shape readers match come from one definition, phaseDirectoryName in src/phase-id.cts. Every custom writer (phase add, add-batch, insert, custom scaffold) records the exact id in a .phase-id file inside the directory, so ownership is the directory's own record, not an inference from roadmap headings or prefix length. Resolution order is: an exact recorded id, then a markerless directory that exactly one known id can own, then the full directory name. Writers share one preflight that refuses, with a typed reason and before any write, an id equal to or case- or padding-equivalent to a known id (Foo/FOO, 1/01), a repeated id in one batch, an id that already carries the project code, and a target directory that already exists or that another entry in the batch claims. Segment-prefix pairs (SETUP and SETUP-2) are allowed and each resolves to its own directory; only a markerless legacy directory that would make the pair ambiguous is refused. Destructive commands (phase remove, renumbering) act on a markerless directory when its owner is unique and refuse only a genuinely ambiguous one, naming the candidates and the .phase-id fix. Removing a recorded or custom id deletes only its own heading, row and directory; unmarked numeric phases, including inserted decimals, keep their padding-tolerant removal and renumbering. Writers and readers read phase_naming and project_code through the same resolution; a corrupt project config is reported as such. Lifecycle readers are custom-aware and report ids in roadmap order: phase complete, progress, roadmap.analyze, init.progress, init.manager, validate.health, phase-plan-index, STATE and smart entry. Marker reads are bounded (regular file, no symlink or FIFO, 1 KiB). Numeric, letter-suffixed, decimal, bracket and full-directory-name resolution is unchanged. docs/CONFIGURATION.md documents custom naming, the marker and the id grammar.

Root cause

As the triage found: the custom-id write path arrived with the TS migration (df04aae, #602), but the token grammar and project-code stripping (#1455) were digit-anchored and never extended to custom ids. extractPhaseToken found no token in a custom directory name, so phaseTokenMatches and matchPhaseDirs never matched it, and the writer had no reader-side inverse.

Testing

How I verified the fix

Twenty regressions in tests/phase.test.cjs, parameterized with project code CCM and with none, fail on upstream/next and pass with the fix: phase add --id X then find-phase X, init.plan-phase X with phase_dir equal to expected_phase_dir, roadmap.update-plan-progress X for PRODUCT-BOUNDARY and SETUP-2, prefix and slug collisions, a renamed roadmap heading, an archived custom phase, and a fast-check add-then-resolve round trip. Copying the resolved path into the expected path fails both divergence tests. The numeric scaffold control passes on both; 21 focused checks and 304 affected tests pass, and tests/phase-resolution-parity.test.cjs is unchanged and green. lint:ci (including check:phase-id-drift) and lint:changeset pass. Round-5 regressions in tests/custom-phase-readers.test.cjs (identity precedence, occupied-directory and fold-equal collisions, markerless ownership, reader order, typed refusals) fail on 6efa98166 and pass on the head. Round-6 regressions (custom lifecycle identity, custom hidden identity, custom reader context) fail on 857ec0a1f and pass on the head. Clean Node 24 full suite: 46,861 passed, 0 failed, 104 skipped.

Regression test added?

Yes — added a test that would have caught this bug
No — explain why:

Platforms tested

macOS
Windows (including backslash path handling)
Linux
N/A (not platform-specific)

Runtimes tested

Self-test report

{
  "schema": 1,
  "head_sha": "e139b996474fbe6c466326cdb5b2033bea63d471",
  "tool": "npm-test",
  "failing_first": {
    "test": "custom lifecycle identity: phase complete SETUP advances to SETUP-2",
    "fails_on_base": true,
    "passes_on_head": true
  },
  "commands": [
    "npm run build:lib",
    "npx eslint src tests scripts --max-warnings 0",
    "npm run lint:ci",
    "node --test tests/custom-phase-readers.test.cjs tests/phase.test.cjs",
    "npm test"
  ],
  "full_suite": {
    "command": "npm test",
    "result": "passed",
    "passed": 46861,
    "failed": 0
  },
  "environment": {
    "os": "macOS 26.6.2",
    "node": "v24.22.0"
  },
  "output_excerpt": "npm test exit 0 at head; summed over all chunks: 46861 passed, 0 failed, 104 skipped. Run as on CI: Node 24, an empty HOME, GIT_CONFIG_NOSYSTEM=1 (Apple's system gitconfig sets init.defaultBranch=main) and TMPDIR as its real path. Last chunk:\nrun-tests: chunk 28/29 completed in 56690ms\n# tests 2029\n# pass 2027\n# fail 0\n# skipped 2\n# duration_ms 47487.287\nrun-tests: chunk 29/29 completed in 47605ms"
}

Checklist

Issue linked above with Fixes #NNN — PR will be auto-closed if missing
Linked issue has the confirmed-bug label
Fix is scoped to the reported bug — no unrelated changes included
Regression test added (or explained why not)
All existing tests pass (npm test)
.changeset/ fragment added if this is a user-facing fix (npm run changeset -- --type Fixed --pr <NNN> --body "...") — or no-changelog label applied
No unnecessary dependencies added

Breaking changes

phase.add --id (and add-batch) now returns a usage error unless the project's effective phase_naming is custom. Before, it wrote a custom-shaped directory that no reader could resolve. When ~/.gsd/defaults.json sets custom but the project config shadows it, the error says so.
New custom ids are refused when they equal a known id up to case or zero padding, already carry the project code, map to a directory that already exists, fall in the numeric range phase readers reserve for backlog and icebox phases (999, 0.5, 00-00), or contain whitespace, a colon or a leading [; such ids could not be resolved unambiguously. The refusals carry typed reasons (phase_id_collision, ambiguous_phase_owner, phase_naming_not_custom).
A destructive command refuses a markerless custom directory only when more than one known id could own it.
Non-custom projects are otherwise unchanged, including scaffold phase-dir with milestone-qualified and bracket arguments.

Share custom phase directory construction and directory-side matching in phase-id. Match longest known IDs without relying on roadmap titles, and derive expected paths independently. Keep config loading in callers and cover active and archived phases. Concurrent files left unstaged: none.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-18T00:14:06Z

Please use the PR template format

This PR is allowed to stay open because the author is a contributor or higher, but the body does not follow one of the required PR templates.

Please update the PR description to use the fix, enhancement, or feature template format from CONTRIBUTING.md so the automated documentation modules can do their job.

Detected problem: PR body does not match the fix, enhancement, or feature template.

trek-e

Status note (posted against head 2dc5a411e171dac3e2c8d6156470c15acc5ccf30): next is green again as of ad90644e23 (Tests run 39099590460). Any finding below that is attributed to base-branch health (Base branch health, Required tests) or to the branch being behind next is base drift, not a defect in this PR; it clears on a refresh of the branch and is not one of the open findings. The open findings are the ones listed under Findings that name a file and line in this PR.

Review — fix(#5186): resolve custom phase directories consistently

Reviewed head 2dc5a411e171dac3e2c8d6156470c15acc5ccf30 (fetched live this turn: mergeStateStatus BEHIND, mergeable MERGEABLE, reviewDecision REVIEW_REQUIRED). No prior maintainer review or inline comments exist on the PR; the only conversation item is the gsd-pr-template-policy bot comment (see Classification).

Summary

The fix introduces phaseDirectoryName in src/phase-id.cts as the single writer-side shape, adds a directory-side matcher (customPhaseQuery / customPhaseDirectoryParts / matchPhaseDirs(..., custom)), plumbs it through findPhaseInternal, cmdFindPhase, cmdPhasePlanIndex, guardedFindPhase (init.*), cmdInitPlanPhase (expected_phase_dir), cmdScaffold and cmdRoadmapUpdatePlanProgress. The 8-file diff matches the diagnosis in the triage comment. The five issue symptoms that have tests (find-phase, init.plan-phase, roadmap.update-plan-progress, SETUP vs SETUP-2, legacy resolution) are addressed. Gaps: the new matcher is only wired into a subset of readers; readers outside that subset stay on the old path or on an unconfigured heuristic mode (Findings 1 and 2). Remaining items are minor.

Classification & gate compliance

Linked issue #5186 labels (read live): bug, confirmed-bug -> Bug-Fix track. Pass.
PR has no labels (labels: []). Fact, not a blocker for the bug track, but no no-changelog is needed because a changeset exists.
Branch fix/5186-custom-code-dirs: <prefix>/NNN-slug OK. Title fix(#5186): ... OK. Not draft. Body has Fixes #5186.
PR template: body uses the Fix template. The bot comment "PR body does not match the fix, enhancement, or feature template" (posted 2026-10-18T00:14:06Z) is still on the PR, but the later Pull request template format check run (39082252621) is SUCCESS at this head. Author should confirm the comment is stale.
Changeset .changeset/lucky-deer-travel.md: type Fixed, pr: 5509 backfilled (OK). Body is a plain sentence and does not use the required **<Bold user-visible change>** — <symptom-led explanation>. form (CONTRIBUTING.md:194-195). See Findings (Minor).
Docs gate: type Fixed is exempt from docs/. No references/ or workflows/ files added, so no INVENTORY change.
One concern per PR: yes. Unrelated churn: src/phase-locator.cts header comment edit (em dash replaced by ;, plus a new dependency line). Nit.

Must-Have Acceptance Checklist (from the Agent Brief on #5186)

With custom naming and a project code, add then find-phase X returns the created directory. PASS by reasoning: custom ... find-phase resolves a newly added custom ID with project code CCM asserts found.directory === dir; cmdFindPhase threads config into customPhaseQueryFor.
Same without a project code. PASS by reasoning (same test, null code; customDirectoryBodies leaves dir as the body).
init.plan-phase X returns phase_dir equal to expected_phase_dir. PASS by reasoning: asserted in init.plan-phase agrees with expected_phase_dir ...; expected_phase_dir is now always computed for custom ids via phaseDirectoryName.
roadmap.update-plan-progress X <plan> complete ticks the Phase X entry. PASS by reasoning for PRODUCT-BOUNDARY and SETUP-2 (literal heading via phaseMarkdownRegexSourceExact). Asserts the persisted ROADMAP.md.
Prefix-of-another-id (SETUP, SETUP-2) resolves each to its own directory. PASS by reasoning: two tests (collisions and headings-removed variant).
Numeric, letter-suffixed and bracket-convention results unchanged. PASS by reasoning for find-phase on 3, 3A, 3.2 dirs and findPhaseInternal(..., 'bracket') (bracket short-circuits custom to null at phase-locator.cts findPhaseInternal). Not executed here. cmdScaffold changed branch (custom id literal) has no test (Finding 4).
Regression test fails before the fix and passes after, run via gsd-test. NOT VERIFIABLE by me (no local execution allowed). Reasoned: every add-then-resolve test fails on base because find-phase returns found:false. CI: Tests shards and conformance are SUCCESS at this head, but the aggregate Required tests is FAILURE (base-health gate, see CI). I do not decide that is benign.

Root-cause / functional verdict

Root cause (one sentence): the custom-id writer (cmdPhaseAdd) emitted [CODE-]<ID>-<slug> while every reader matched via digit-anchored extractPhaseToken, so no reader had an inverse of the writer; the fix gives readers that inverse via the same phaseDirectoryName shape. This is a root-cause fix for the readers it reaches, not a special case of the symptom.

Not a complete root-cause fix at the "everywhere" level required by the issue (resolves by X everywhere). The inverse lives in matchPhaseDirs's optional 4th parameter, so every caller that does not pass custom still runs the old digit-anchored matching. See Findings 1 and 2.

Blast radius

CodeGraph (worktree index at head; Memtrace unavailable):

src/phase-id.cts is imported by 16 files; matchPhaseDirs signature change is additive (optional 4th param), and the export surface gains phaseDirectoryName, customPhaseQuery, customPhaseDirectoryParts, isCustomPhaseId. phase-locator.cts exports customPhaseQueryFor.
findPhaseInternal gains an optional 4th param. Direct callers (grep of src): 20+ sites. Only init.cts (guardedFindPhase), roadmap.cts:1132, and the find-phase/plan-index paths pass config. Callers left on the default: gate-phase-context.cts:82, roadmap.cts:1800, init.cts:3065, refactor-trigger-command-router.cts:217, gate-api-coverage-verify-pre.cts:190, state.cts:5665, commands.cts:2051/3546/3766, verify.cts:1450, template.cts:110, phase.cts:3733/4961/5213/5240.
Direct matchPhaseDirs callers that cannot see custom ids: roadmap.cts:583/665/871, milestone.cts:778, state.cts:1004/1008, phase.cts:269/357/2589, smart-entry.cts:360, gate-phase-context.cts:115, health-diagnostic-rules/state-consistency.cts:377, health-diagnostic-rules/roadmap-disk-consistency.cts:140.
phase-id.cts now imports markdown-sectionizer.cjs (new edge into a pure module); check:phase-id-drift passed in CI per PR body and the lint jobs are green.

Co-change completeness

tests/phase-resolution-parity.test.cjs unchanged; PR states it stays green. The parity requirement (writer shape vs reader shape) is covered by the add-then-resolve and divergence tests; no dedicated unit test pins phaseDirectoryName output against cmdPhaseAdd/cmdScaffold/expected_phase_dir call sites.
Readers listed in Blast radius that were not updated (Findings 1 and 2).
CONTEXT.md: no entry for the new shared directory-shape owner. Fixed changesets are docs-exempt; vocabulary change is Nit-level.

Test quality

Behavioral CLI tests, assert persisted artifacts (directory name, ROADMAP.md checkbox text). No source-grep, no wall-clock. homeSandboxEnv is used for HOME isolation.
Control tests: legacy numeric/letter/decimal/bracket resolution assert specific values.
The fast-check property (custom phase add-then-resolve round trip) generates only uppercase letters ^[A-Z]{2,8}-[A-Z]{2,8}$ ids and a single added phase besides INITIAL. It cannot generate ids that prefix another id, ids ending in digits (SETUP-2), or lowercase/mixed ids, so it does not exercise the longest-id-wins logic it exists to guard. Each of 20 runs spawns several CLI processes; no per-test timeout is set. Minor (Finding 5).
No boundary test at limit-1/limit/limit+1 for the 60-char slug truncation in expected_phase_dir (generateSlugInternal(...).substring(0, 60)), which the PR touches. Minor (Finding 5).

Memtrace Evidence

get_impact: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT).
get_symbol_context: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT).
recall_decision: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT).
find_code_review_issues: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT).
_graph_state: not obtainable (Memtrace down).

Substitute evidence: a CodeGraph index built in a detached worktree at head (codegraph init -y), codegraph_explore/codegraph node/codegraph query for phaseMarkdownRegexSourceExact (src/phase-id.cts:786, exported at :1872), hasProjectCodePrefix (:401, regex /^[A-Z][A-Z0-9_]*-(?=\d)/i), tokenizeHeadings, searchPhaseInDir (src/phase-locator.cts:208-361), and roadmap.cts:19,41 (loadConfigForScope is loadConfig). Caller enumeration used a grep of src (CodeGraph does not index destructured-import call sites). No ADR decision lookup was possible; no governing ADR is cited by the PR.

Findings

Major

(judgment, verified by static reading; not executed) The directory-side inverse is applied only where a caller passes custom/options, so "resolves by X everywhere" (issue Desired behavior) is not met. Callers of matchPhaseDirs listed under Blast radius (roadmap analyze at src/roadmap.cts:583/665/871, milestone complete hasDirectory at src/milestone.cts:778, src/state.cts:1004/1008, src/smart-entry.cts:360, health rules state-consistency.cts:377 and roadmap-disk-consistency.cts:140, phase.cts:269/357/2589, gate-phase-context.cts:115) still use digit-anchored matching and will report a phase created by phase add --id X as having no directory. Fix: make the custom match part of the canonical matchPhaseDirs contract (derive the custom query inside the owner from (dirs, normalized, namingConfig) or have one shared resolvePhaseDirs helper) so every reader goes through the single owner, and add a test for at least roadmap analyze disk status and milestone complete directory check with a custom id.
(judgment, verified by static reading) Unconfigured heuristic mode can mis-resolve. findPhaseInternal callers that omit options (list under Blast radius, e.g. src/verify.cts:1450, src/phase.cts:3733/5213/5240, src/roadmap.cts:1800) get phaseNaming === undefined and projectCode === undefined, so customDirectoryBodies (src/phase-id.cts) strips any leading ^[A-Z][A-Z0-9_]*- as a project code. For a project without a project code and a directory PRODUCT-BOUNDARY-product-boundary, query BOUNDARY produces body BOUNDARY-product-boundary, which customPhaseDirectoryParts accepts (owner === query.id), so BOUNDARY resolves to PRODUCT-BOUNDARY's directory. It also activates custom matching in non-custom (numeric) projects. Fix: resolve naming config inside findPhaseInternal (or require callers to pass it) and never guess the project code from the directory; add a negative test (find-phase BOUNDARY is not found when only PRODUCT-BOUNDARY exists, with and without project code).
(deterministic fact) The issue's symptom table lists roadmap.annotate-dependencies PRODUCT-BOUNDARY and the plan-phase probes (check verify-command-paths, check verify-failure-directions, verify context-drift) as broken; the PR neither tests nor mentions any of them (no occurrence of these verbs in the diff or tests). roadmap.cts:1800 and verify.cts:1450 are on the unconfigured path of Finding 2. Fix: add add-then-resolve tests for each verb the issue names.

Minor

(deterministic fact) src/commands.cts cmdScaffold changed behavior for phase_naming: custom with a custom id (now writes <ID>-<slug> literal instead of normalizePhaseName(id)); the only scaffold test (custom naming preserves legacy directory resolution) covers numeric ids. Fix: add a scaffold test for a custom id with and without project code, asserting the directory equals the one phase add --id writes.
(judgment) Test strength: the fast-check property cannot generate prefix-colliding, digit-suffixed or lowercase ids (see Test quality) so it does not discriminate the longest-id-wins rule; no slug-length boundary test (limit-1/limit/limit+1, 60 chars) for expected_phase_dir. Fix: generate ids from a small alphabet so one id prefixes another and include digit and lowercase segments; add 59/60/61 char title cases.
(deterministic fact) Changeset body Phases created with custom IDs resolve to their directories in find-phase, planning initialization, and roadmap progress updates. lacks the required **<Bold user-visible change>** — lead (CONTRIBUTING.md:194-195). Fix: **Custom phase IDs resolve to their directories** — phases created with phase add --id now resolve in find-phase, init.plan-phase and roadmap.update-plan-progress. (#5186).
(deterministic fact) The gsd-pr-template-policy bot comment remains on the PR though the later template check passed; author should confirm the body or resolve the comment.

Nit

src/phase-locator.cts header comment: unrelated edit (— to ;) and an added dependency line; revert the punctuation churn.
isCustomPhaseId (src/phase-id.cts) compiles a RegExp on every call and is on the hot path of every findPhaseInternal; hoist to a module constant.
customPhaseQueryFor (src/phase-locator.cts) reads ROADMAP.md with an unbounded readFileSync on every custom-candidate lookup; the roadmap is only used to extract ids. Consider a bounded read or reuse the parsed roadmap.

CI

Head 2dc5a411e171dac3e2c8d6156470c15acc5ccf30, latest-per-name non-success/non-skipped runs (live):

failure Base branch health (Tests workflow, run 39082013651)
failure Required tests (Tests workflow, run 39082013651)

Verbatim failing log line from Required tests: ##[error]the base branch's own last Tests run is red — see the 'Base branch health' job above for the failing run. Wait for a fix-forward merge, or if this PR IS the fix, ask a maintainer to apply the 'fix-next' label to override. The same step's env shows NEXT_HEALTH_RESULT: failure and TEST_RESULT: success, TEST_CONFORMANCE_RESULT: success, COVERAGE_GATE_RESULT: success, QA_LOOP_WALK_RESULT: success, LINT_RESULT: success. Attribution: the red is the base-branch-health gate, not a test file touched by this PR (no failing test in the PR's own matrix); I report it verbatim and do not rule on whether the maintainer's fix-next override applies.

action_required runs for this head sha: none (0).
Mergeability: MERGEABLE but BEHIND next (merge-base aeabede8e9, origin/next at review time ad90644e23); rebase is needed before merge.

Verdict

Changes requested (zero tolerance: 3 Major, 4 Minor, 3 Nit). Substance is close, but the custom-id inverse is not owned by the single shared matcher, and the unconfigured heuristic path can mis-resolve. CI is not clean (Required tests and Base branch health red) and the branch is behind next.

…e-dirs

Require naming context at the canonical directory matcher and resolve it silently through the scoped config owner. Preserve raw custom IDs, remove guessed project prefixes, and bound roadmap identity hints without config migration or warnings. Cover all reviewed readers, numeric compatibility, and prefix collisions with digit-ending custom segments.

bshiggins · 2026-10-18T18:00:24Z

Thanks for the review. Every finding is addressed on the new head 5c12b4fcb: a merge of next@b224cc9ea as 411ce471a (no force push), then one fix commit.

Major, the inverse was opt-in: the custom-id inverse now belongs to the canonical matchPhaseDirs, which takes a required naming context, so a reader cannot forget it. Every reader you listed delegates through it: roadmap analyze disk status, milestone complete directory checks, state, smart-entry, both health rules, the phase.cts sites, gate-phase-context, and the findPhaseInternal callers. New tests cover roadmap analyze disk status and the milestone complete directory check with a custom id.
Major, project-code guessing: removed. Naming (phase_naming, project_code) is resolved inside the locator through the scoped-config reader, as you suggested, silently and without migrating config, so custom matching stays off for non-custom projects. Negative tests: find-phase BOUNDARY is not found when only PRODUCT-BOUNDARY exists, with and without a project code, and a numeric project is unaffected.
Major, the issue's other verbs: add-then-resolve tests now cover every verb the issue names, including roadmap.annotate-dependencies, check verify-command-paths, check verify-failure-directions and verify context-drift, for PRODUCT-BOUNDARY and SETUP-2, with and without a project code.
Minor, scaffold: scaffold tests for a custom id in both project-code configurations assert the directory equals what phase add --id writes.
Minor, test strength: the fast-check property (seed 5509) now draws from a small alphabet so one id prefixes another, with digit-ending and mixed-case segments; 59, 60 and 61 character titles cover the expected_phase_dir slug truncation.
Minor, changeset body: now in the **<Bold user-visible change>** — <explanation> form; still type: Fixed, pr: 5509.
Bot comment: a bot post I cannot remove; the later template check passed at the reviewed head.
Nit, header churn: the src/phase-locator.cts header punctuation is restored and the roadmap-parser dependency line is gone. The planning-workspace line now names readScopedConfigValue, the real new dependency; the locator still imports no loadConfig.
Nit: the isCustomPhaseId RegExp is hoisted to a module constant.
Nit, unbounded read: roadmap identity hints are read only for custom-naming projects, capped at 1 MiB, and reuse roadmap text a caller already holds. A bounded-read regression fails on 2dc5a411e and passes on this head.

Tests: 33 new cases. The 18 regressions fail on 2dc5a411e and pass on 5c12b4fcb; the other 15 are controls that pass on both by design (numeric, letter-suffixed, decimal and bracket resolution, the BOUNDARY negatives in numeric projects). tests/phase-resolution-parity.test.cjs is untouched and green.

Gates on 5c12b4fcb: lint:ci, check:phase-id-drift and lint:changeset pass; focused verification 1,625/1,625; the clean Node 24 full suite ran 45,677 tests, 45,575 passed, 0 failed, 101 skipped, 1 todo.

Ready for your re-review of 5c12b4fcb.

trek-e

Review — fix(#5186): resolve custom phase directories consistently (re-review at `5c12b4fcb8`)

I verified the author's claim that the ten earlier findings are fixed: every matchPhaseDirs and findPhaseInternal caller in src/ now passes a naming context, naming is resolved inside the locator, and the missing reader tests are added. The issue's expected behavior ("a phase created by phase add --id X resolves by X everywhere") is still not met. CI is green on this head. Three Major, three Minor and three Nit remain.

Findings

Major

src/phase-id.cts:1468-1487 (customIdsFromDirectories, customPhaseDirectoryParts): a directory written by phase add becomes unreadable when the title's slug begins with a token that has no lowercase letter. Reproduced on a build of the head with {"phase_naming":"custom"} and no project code: query phase.add "2024 roadmap" --id SETUP creates .planning/phases/SETUP-2024-roadmap, and query find-phase SETUP returns "found": false. The directory yields the inferred id SETUP-2024, the longer id sorts first, and it becomes the owner instead of the query id. Resolve against the known id set only (query id plus roadmap and config ids), preferring the roadmap-listed id when two known ids both prefix a directory, or encode an unambiguous id/slug separator in phaseDirectoryName. Add tests with digit-leading titles (2024 roadmap, 3 steps), with and without a project code.
Phase-key readers still do not understand custom ids. On the head build phaseKeyFromDir('PRODUCT-BOUNDARY-boundary') returns PRODUCT-BOUNDARY-BOUNDARY while phaseKeyFromToken('PRODUCT-BOUNDARY') returns PRODUCT-BOUNDARY, so a ROADMAP entry and its directory never compare equal in src/planning-inspect.cts:1254,1336-1337, src/workstream-inventory.cts:628,686 and src/state.cts:2862,2870,6413,6680. phaseKeyFromToken('SETUP-2') returns 02, so SETUP-2 collides with numeric phase 2. Route these through the same custom-aware owner and test at least planning-inspect and workstream-inventory with a custom id.
Unvalidated custom id reaches a path join. query phase.add x --id '../evil' creates .planning/evil-x outside .planning/phases, and scaffold phase-dir --phase '../zz' --name Foo creates .planning/zz-foo. Both reproduced. The write shape existed before this PR, but phaseDirectoryName is now the single owner of it and the reader already rejects / and \ (customPhaseQuery, src/phase-id.cts:1437), so writer and reader disagree. Reject ids containing /, \, .. or NUL in phaseDirectoryName (or in cmdPhaseAdd and cmdScaffold) with a usage error, assert containment under the phases directory, and add a test.

Minor
4. matchPhaseDirs custom branch is quadratic (src/phase-id.cts:1566 recomputes and sorts the id set per directory). Measured: 100 dirs 5 ms, 500 dirs 141 ms, 1000 dirs 778 ms per lookup, and roadmap analyze calls it once per phase. Compute the sorted id list once per matchPhaseDirs call.
5. phaseDirectoryOptions(cwd) is the default argument of findPhaseInternal and is re-evaluated at about 30 call sites, each doing two scoped config reads and, for custom projects, a 1 MiB buffer and a roadmap read; src/state.cts:1040,1044 call it twice in one function. Resolve once per command and thread it, as cmdRoadmapAnalyze does.
6. Both fast-check properties (tests/custom-phase-readers.test.cjs, tests/phase.test.cjs) use a constant title and a fixed seed with 20 runs, so they replay one sample and cannot fail on finding 1. Generate the title, including digit-leading words, and keep the seed only as a recorded example.

Nit
7. src/phase-id.cts:1561: const normalized = context ? normalizePhaseName(query) : query; makes behavior depend on whether a context was passed, while the signature and CONTEXT.md say it is required. Enforce it or normalize unconditionally.
8. tests/custom-phase-readers.test.cjs bounded-read test: locator.phaseDirectoryOptions ? ... : options(null) is a dead fallback that would mask a missing export. Call it unconditionally.
9. src/phase-locator.cts header lists phase-id imports as (normalizePhaseName, matchPhaseDirs, phaseNumberForMatch); add the custom-id symbols it now consumes.

Process
10. The branch is behind next by 4 commits and merges cleanly. Rebase and push.

Memtrace Evidence

get_impact: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT)
get_symbol_context: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT)
recall_decision: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT)
find_code_review_issues: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT)
Substitute evidence: CodeGraph index of a detached worktree at the head (matchPhaseDirs -> customPhaseDirectoryParts -> customIdsFromDirectories), git grep of src/ at the head and next to enumerate callers, and CLI probes against a tsc build of the head (commands as stated in findings 1 to 4).

Verdict

Changes requested (zero tolerance: Major 1 to 3, Minor 4 to 6, Nit 7 to 9; branch behind).

…e-dirs

Resolve custom directories from explicit identities rather than slug text. Share custom phase keys across planning readers, reuse command naming contexts, and reject path-like IDs before creating directories. Add regressions for digit-leading titles, custom key collisions, and unsafe IDs, plus generated-title round trips. Preserve literal continuation IDs after normalization.

bshiggins · 2026-10-18T23:50:47Z

Round 4 for review 5565188388, pushed as a fast-forward 5c12b4fcb..67a4d2ca4: merge 54f91b800 of next@8940d2a13, then one fix commit 67a4d2ca4.

Major

Directory matching now resolves against known ids only (the query id plus roadmap and config ids); a roadmap-listed id wins when two known ids prefix one directory, and a directory-inferred id never outranks a known one. phase.add "2024 roadmap" --id SETUP then find-phase SETUP returns found: true at .planning/phases/SETUP-2024-roadmap. Tests cover digit-leading titles (2024 roadmap, 3 steps) with and without a project code.
planning-inspect, workstream-inventory and the state key sites go through the same custom-aware owner. PRODUCT-BOUNDARY-boundary keys as PRODUCT-BOUNDARY, and SETUP-2 stays distinct from numeric phase 2 (tested in inspect, inventory and STATE).
phaseDirectoryName rejects /, \, .. and NUL in any part with a usage error; phase.add and scaffold phase-dir both assert containment under the phases directory. --id ../evil and --phase ../zz now return reason: usage, and neither directory is created.

Minor
4. The prefix list is sorted once per matchPhaseDirs call. 1,000 directories on Node 24: 203.657 ms before, 0.080 ms after (mean of five lookups).
5. Naming context is resolved once per command and threaded, as cmdRoadmapAnalyze does; typed locator calls require it, and the double read in state.cts is gone.
6. Both properties generate the title, including digit-leading words, with 30 runs and no fixed seed; the 2024 roadmap case is kept as a recorded example.

Nit
7. Normalization is unconditional. The existing property caught a 0-00 regression on the way, and its counterexample now passes.
8. The dead fallback in the bounded-read test is removed.
9. The locator header lists the custom-id symbols it imports.

Evidence

New regressions in tests/custom-phase-round4.test.cjs, run at the old head 5c12b4fcb and at the new head:
- Major 1: 4 fail at the old head, 4 pass at the new head.
- Major 2: 7 fail, 7 pass.
- Major 3: 11 fail, 11 pass.
- The two generated-title properties: 2 fail, 2 pass.
tests/phase-resolution-parity.test.cjs is unchanged and green.
lint:ci, check:phase-id-drift and lint:changeset pass.
Clean Node 24 full suite: 45,747 tests, 45,644 passed, 0 failed, 102 skipped, 1 todo.
The changeset stays Fixed with pr: 5509.

trek-e · 2026-10-21T00:48:37Z

Merge conflict with `next` — re-review blocked

Head 67a4d2ca4a does not merge into current next (mergeStateStatus: DIRTY, mergeable: CONFLICTING).

Conflicting file: tests/phase.test.cjs (content conflict; every other file merges automatically).

Resolve it on your branch: fetch origin/next, rebase onto it, resolve tests/phase.test.cjs, and force-with-lease push the branch.

I have not re-reviewed the substance of the latest push against a base it cannot merge into. My earlier CHANGES_REQUESTED review remains the standing decision; this comment neither retires nor extends it. Once the branch merges cleanly I will re-review the new commits against the open findings. CI is not released while the tree is conflicting.

Retain complete custom-ID round-4 regressions and the upstream phase-completion and milestone-fallback regression blocks.

Keep custom phase parsing context with upstream status phrase handling.

bshiggins · 2026-10-21T16:55:06Z

@trek-e Thanks for flagging the conflict. The branch now merges cleanly into next (head 9adebfba2).

I merged next into the branch instead of rebasing, the same way earlier rounds did, so the commits you already reviewed keep their hashes and nothing was force-pushed. The tests/phase.test.cjs conflict keeps both next's new regression blocks and the custom-phase block. A second merge picked up #5596, which touched src/phase-id.cts.

The full Node 24 suite passes with 0 failures, along with lint:ci, check:phase-id-drift and lint:changeset. Round 4 is ready for your review whenever you have time.

trek-e · 2026-10-22T01:11:53Z

Heads-up on a change to how fixes land in next, and what it means for this PR. Nothing here blocks you today; it is how to be ready.

1. No need to keep merging next into your branch. next now uses a GitHub merge queue. Once a PR is approved and green, it is queued, and the queue re-tests it on top of next plus whatever is ahead of it, then merges. "Update branch" is no longer a prerequisite to merging, so please skip it unless you have a real conflict or want a fresh signal.

2. Add a Self-test report (fix PRs). Fix PRs now carry a short ## Self-test report section with a JSON block, in the fix template. It records the failing-first regression test, the commands you ran, your OS/Node, and a passing full-suite run, bound to your final commit sha. It is a head start for the reviewer, not proof; CI on the queue commit is authoritative. The advisory Self-test report check on this PR shows what is missing. Fill it in after your last push (a new push makes the sha stale).

3. Review. The advisory Reviewed or maintainer check is red until someone with write access approves. That is expected and resolves with a review, not with a change from you.

4. CI cost. Windows/macOS lanes now run on the queue, not on every push. If your change is platform-specific, add the full-matrix label to get them on the PR.

Questions are welcome here. Details: docs/branch-protection.md.

Posted at the maintainer's request.

trek-e

Re-review of `9adebfba29` (round 4 + merge of `next`)

Prior findings 1 to 9 (SETUP-2024 slug, phase-key readers, path-like ids, per-directory sort, header, dead fallback) are verified fixed on this head by CLI probes; the rest of the original list is superseded by the findings below. CI for this exact head is complete and green (latest-per-name, no pending, no action_required runs). The issue's expected behavior ("a phase created by phase add --id X resolves by X everywhere") is still not met. Branch merges cleanly into next (13 behind, 8 ahead; git merge-tree clean); this is not a finding.

Findings

Major

src/phase-id.cts customPhaseQuery (the LEADING_DIGIT_RUN_PREFIX_RE early return null): custom ids that begin with a digit run followed by letters are written by phase add but never resolve. Reproduced with {"phase_naming":"custom"}: query phase.add "T 2FA" --id 2FA writes 2FA-t-2fa, find-phase 2FA returns found:false; same for 7B and 12AB. 3 and 2024-Q1 resolve. Source: deterministic (CLI probe). The fast-check properties (tests/custom-phase-readers.test.cjs, tests/phase.test.cjs) only generate letter-leading ids, so they cannot fail on this.
Writer and readers read naming from different sources. cmdPhaseAdd/cmdScaffold use loadConfig (merges ~/.gsd/defaults.json); phaseDirectoryOptions (src/phase-locator.cts) uses readScopedConfigValue, which reads only project config files. Reproduced: HOME with ~/.gsd/defaults.json = {"phase_naming":"custom"} and project config.json = {}: phase.add Boundary --id PRODUCT-BOUNDARY writes PRODUCT-BOUNDARY-boundary, find-phase PRODUCT-BOUNDARY returns found:false. This is the #5186 symptom for a supported config source. Same for project_code. Source: deterministic.
src/smart-entry.cts detectVerifyFailed: in a custom-naming project the STATE token is currentPhaseRaw.trim().split(/\s+/)[0] and every directory is accepted as a phase token. Reproduced: dir 03-foo with STATUS: failed, current_phase: "Phase 3": verify_failed is true with phase_naming unset and false with phase_naming: custom ("3" and "3 of 5 (Foo)" stay true). A silent routing regression for numeric phases in custom projects. Source: deterministic. tests/custom-phase-readers.test.cjs covers only current_phase: PRODUCT-BOUNDARY.
Adversarial pass (isolated reviewer, reproduced, details in its file): prefix ownership depends on ROADMAP headings, so phase.remove A --force removed A-B-second when A-B was not listed in the roadmap (destructive, src/phase-id.cts customPhaseQuery/customDirectoryOwner); phase.complete next-phase scan in src/phase.cts (~4840-4910) is not custom-aware (AUTH complete reports is_last_phase:true, STATE not advanced); progress shows number AUTH-s with empty name and validate.health W005 flags every custom directory; phase-plan-index for A-2 prints phase 02. Source: judgment plus reproduction by the isolated reviewer; verify against /private/tmp/claude-501/-Users-trekkie-projects-gsd-core--claude-worktrees-nostalgic-turing-176e72/d3f576f1-5f2a-4712-8469-1ed12dbb6380/scratchpad/pr-5509-adversarial.md (local).

Minor
5. customPhaseQuery re-runs tokenizeHeadings(roadmap) on every matchPhaseDirs/phaseKeyFromToken/phaseKeyFromDir call, so readers are O(phases x roadmap bytes). Measured on a custom project, one directory per roadmap heading: 600 phases / 183 KB roadmap.analyze 1.63 s; 1200 phases / 366 KB 8.32 s. Compute the known-id list once per context. Source: deterministic (timing).
6. phaseDirectoryOptions allocates Buffer.alloc(1 MiB) per call (about 30 call sites); size the buffer from fstat.
7. cmdPhaseAdd with --id but without phase_naming: custom writes a custom-shaped directory that no reader resolves (adversarial pass); either require custom naming or resolve it.

Nit
8. tests/custom-phase-round4.test.cjs and its titles (round 4 Major 1) name a review round, not behavior; rename by behavior.
9. tests/phase.test.cjs test "directory IDs disambiguate SETUP from SETUP-2 without headings" injects knownIds that no production path supplies; the name overstates the covered behavior.

Process

PR body has no ## Self-test report JSON block bound to the final head sha (advisory check; fix template). Add it after the last push.
The "Reviewed or maintainer" check is advisory and clears with an approval on a clean head.

Memtrace Evidence

get_impact / get_symbol_context (target matchPhaseDirs, repo gsd-core): both returned ambiguous with only stale, branch-mixed constant declarations (graph indexed on another branch, no head episodes). No dependency traversal ran; not a clean result. Blast radius taken from git grep of src/ callers at the head instead: every matchPhaseDirs caller passes a context; about 25 files import phase-id.
recall_decision (query: custom phase_naming directory resolution): returned ADR-612 and ADR-2121 (phase-identifier parsing consolidation) as ranked candidates; no ban violated by the diff that I can prove. Not proof that no decision applies.
find_code_review_issues (graph_mode off, review_mode online, max_candidates 100): 0 issues over src/phase-id.cts (full diff), src/phase-locator.cts, src/phase.cts, src/commands.cts, src/init.cts (diff hunks reconstructed for the tool call, not byte-identical to the PR diff), and a hunk subset of roadmap.cts, state.cts, workstream-inventory.cts, planning-workspace.cts, planning-snapshot.cts. find_yaml_rule_matches: 0 issues over src/phase-id.cts and a hunk subset of phase-locator, phase, commands, smart-entry. Not run on the test files, milestone, planning-inspect, health rules or gates. Zero findings from these tools do not cover findings 1 to 7, which are semantic. Cross-module check not run (graph stale).
Substitute evidence: tsc build of the head and CLI probes in throwaway projects; CodeGraph caller listing.

Verdict

Changes requested (Major 1 to 4, Minor 5 to 7, Nit 8 to 9). CI green on this head does not retire any finding above.

Preserve complete custom tokens before numeric normalization. Cover mixed case and digit-leading IDs in both generated round-trip properties. Concurrent files left unstaged: none.

…rojects Parse STATE values through the shared prose parser and exclude non-phase directories from the latest-phase fallback. Concurrent files left unstaged: none.

Record custom IDs beside phase artifacts, including scaffold and batch creation. Refuse destructive ownership guesses for legacy directories. Concurrent files left unstaged: none.

Remove injected identity hints from the heading-free SETUP regression. Exercise directory ownership through the real read path. Concurrent files left unstaged: none.

Reuse heading IDs, their membership set and sorted directory prefixes. Remove the redundant roadmap argument at every query caller. Concurrent files left unstaged: none.

Allocate only the file size while retaining the one MiB read bound. Cover empty, small and oversized roadmaps. Concurrent files left unstaged: none.

Keep configured numeric, suffix and decimal tokens on their legacy path unless the writer recorded that exact custom ID. Pin CLI and bracket parity. Concurrent files left unstaged: none.

Reject equivalent IDs and prefer exact records over aliases. Permit proven prefix owners and refuse only ambiguous markerless mutations. Concurrent files left unstaged: none.

Match explicit basenames before identity filtering and report the owner ID. Use that owner when removing the roadmap entry. Concurrent files left unstaged: none.

Record inserted identities only when their parent has a custom identity. Legacy numeric children retain their parent's renumbering behavior. Concurrent files left unstaged: none.

Resolve directory identities before ordering and shaping init records. Keep custom phase IDs and names consistent across progress and manager readers. Concurrent files left unstaged: none.

Accept custom naming case-insensitively without changing the config loader. Report corrupt project config separately from valid config shadowing defaults. Concurrent files left unstaged: none.

Use one preflight and writer tail for add, batch, insert and scaffold. Export the context type, preserve optional locator calls and document dependencies. Concurrent files left unstaged: none.

Share fixture helpers and assert on typed errors and parsed roadmap data. Thread custom identity into checkbox metadata and remove global test patches. Concurrent files left unstaged: none.

Describe custom naming, identity markers, grammar and refusal behavior. Classify the change and disclose the dangling-symlink verification fix. Concurrent files left unstaged: none.

Regenerate the Windows tier after routing the probes through the process seam. Remove the fixture import made obsolete by the shared helpers. Concurrent files left unstaged: none.

Retain the existing reason keys and add the three typed phase refusals. Keep the exit-contract matrix exhaustive across all thirty reasons. Concurrent files left unstaged: none.

Resolve recorded identities before interpreting a query as a full basename. Cover both creation orders across find, init and destructive removal. Concurrent files left unstaged: none.

Preflight existing paths and duplicate batch destinations before any write. Preserve existing identity markers across add, batch, insert and scaffold. Concurrent files left unstaged: none.

Allow a unique markerless target without claiming its longer sibling. Retain ambiguous-owner refusal and document the identity safety fixes. Concurrent files left unstaged: none.

Align two older refusal assertions with the unique-owner ruling. Keep ambiguous-target refusal and prove the untouched sibling survives. Concurrent files left unstaged: none.

bshiggins · 2026-10-23T22:51:31Z

Thanks for the round-5 review. All 19 findings are addressed on 857ec0a1f. Each behaviour finding has a regression test that fails on 6efa98166 and passes on the head. An independent re-run of your probes then found two more wrong-target cases, which are fixed and tested in the last four commits (listed at the end).

#	Change	Commit
1	Path assertions compare through `toPosixPath` / `split(path.sep).join('/')`: slug boundaries 59/60/61, active and archived dirs sharing a basename, root versus ambient-workstream archive, null-workstream fallback, and the new init assertions	`f502bf6c4`, `7600bf55f`
2	Ids that differ only in zero padding (`1`/`01`, `7.1`/`07.1`) are refused as `phase_id_collision`, and the resolver prefers an exact recorded id over a fold match	`c01918fe6`
3	Full directory names resolve again in `find-phase`, `phase remove` and the init readers	`e25eb3708`
4	Inserted decimals renumber with their unmarked numeric parent: `01-two 01.1-extra 02-three`, as on `next`	`64f49f4c6`
5	Equal ids, case-folded ids, an id repeated in one batch, and ids that already carry the project code are refused before any write	`c01918fe6`
6	Kept the acceptance criterion. `SETUP` then `SETUP-2` (and `A` then `A-B`) are both creatable, and each resolves to its own directory. A roadmap heading alone no longer blocks the pair. Only a markerless legacy directory that would make the pair ambiguous is refused.	`c01918fe6`
7	No new migration command. A markerless custom directory whose owner is unique resolves and removes as before; only a genuinely ambiguous one is refused, naming the candidates and the `.phase-id` fix. We'd welcome your confirmation of the overall contract.	`c01918fe6`, `6519bce42`
8	The changeset-prose test is deleted. Refusals carry typed reasons (`phase_id_collision`, `ambiguous_phase_owner`, `phase_naming_not_custom`), and tests assert on `reason`, not message text. The error-registry fixtures cover the three new members.	`c01918fe6`, `30e1796ca`, `416c27d55`
9	`docs/CONFIGURATION.md` documents `phase_naming: "custom"`, the `.phase-id` marker, the id grammar and the `phase.add --id` refusal	`c6b3a67f5`
10	The NUL and FIFO probes go through `process-seam`	`f502bf6c4`
11	CONTEXT.md, the dependency headers and the `phaseTokenFromState` doc are corrected	`216aecc8f`
12	`progress`, `roadmap.analyze`, `init.progress` and `init.manager` report ids in roadmap order	`86d674ae6`
13	A corrupt `config.json` reports `config_parse_failed`, naming the file, and `"CUSTOM"` is accepted as custom	`acbb57b71`
14	The changeset is typed `Changed` and mentions the `detectVerifyFailed` dangling-symlink fix	`c6b3a67f5`
15	Fixtures are shared in `tests/helpers.cjs`, and the global `fs`/sectionizer monkey-patching is removed	`30e1796ca`
16	One shared writer preflight and creation path for all four writers, and `PhaseDirectoryContext` is exported	`216aecc8f`
17	A comment at the no-follow open records the Windows `lstat` fallback	`216aecc8f`
18	`findPhaseInternal`'s `context` parameter is optional in its type	`216aecc8f`
19	The smart-entry import order is fixed and it uses the `phaseLocatorMod` alias	`216aecc8f`

Two further cases the re-run found:

9fea755da: an id equal to another phase's directory name (API, which lives in API-v2, and the id API-v2) resolved to that directory, so phase remove API-v2 deleted phase API. A recorded id now wins over a directory-name match, in either creation order.
2b6b67ce7: two ids could map to one directory (A with title b x, and A-b with title x, both give A-b-x), and the second add took over the first phase's directory and marker. This also happens on next. Every writer now refuses an existing or batch-claimed target directory before writing.

We have no Windows host, so finding 1 is fixed by reading the assertions rather than by a Windows run; the Windows shard on this push is the real check. Full suite in a CI-like environment (Node 24, empty HOME, no system gitconfig): 46,781 passed, 0 failed, 103 skipped. The Self-test report in the body is bound to this head. The branch is not merged with next, per your merge-queue note.

trek-e

Re-review of `857ec0a1f5` (round 6: the 19 findings of the 2026-10-23 review plus the four follow-up commits)

Verdict: changes requested. 0 Blocker, 2 Major, 1 Minor. All 19 prior findings are verified fixed on this head; the two Majors below are new and sit in the same area (a custom id treated as a prefix of another).

Classification and gate compliance

Linked issue #5186: OPEN, labels bug, confirmed-bug (Bug-Fix Track). RULESET.CONTRIB.CLASSIFY.fix=requires confirmed/confirmed-bug before implementation: satisfied.
Author follow-up (2026-10-23 22:51, "all 19 findings addressed") is the subject of this review; no question is left open for me. The branch is 10 commits behind next and merges cleanly (git merge-tree).

Verification of the prior round (CLI probes in throwaway projects, head built)

Fixed and re-probed: fold-equal ids (1/01, Foo/FOO, CCM-FOO, batch same same) refused before any write with typed reasons; SETUP then SETUP-2 creatable and each resolves to its own directory; API / API-v2 resolve and phase remove API-v2 deletes only its own directory; the A+title b x / A-b shared-target case is refused; full directory-name lookup works again; progress and roadmap.analyze report roadmap order; Windows conformance shards 1 to 3 are now green; docs/CONFIGURATION.md documents the contract. node --test on the three PR test files: 918 pass, 0 fail.

Blast radius and risk tier

matchPhaseDirs and findPhaseInternal (signature changed) are the shared locators: 14 and 17 callers, all in files this PR changes. The graph head is 0ce32128b3 on next, so none of this PR's new symbols exist in it; caller counts are pre-PR and partial evidence. No changed symbol is in the PageRank or bridge top lists (same as the previous round).

Co-change completeness

Same result as the previous round: no missing co-change partner of substance; docs, CONTEXT.md, changeset and conformance tier lists are all in the diff.

Memtrace Evidence

get_symbol_context (matchPhaseDirs, branch next): ambiguous (eight declarations, seven of them import constants); the resolved function is src/phase-id.cts:1474, pre-PR shape. The graph predates this PR, so it cannot see the new resolvers. Partial evidence only.
get_impact (matchPhaseDirs, upstream): returned the same ambiguity (risk: UNKNOWN, no traversal performed). I rely on the previous round's node-id-pinned impact (HIGH, 48 symbols) and on the diff.
recall_decision (custom phase_naming, .phase-id, project_code): returned ADR-612 and ADR-2121 (phase-identifier parsing consolidation) among unrelated rows. governing_rules on src/phase-id.cts returned only ADR-443 (effort routing), unrelated. No recorded decision is proven to govern this change; that is not proof none applies. check:phase-id-drift passes in CI.
find_code_review_issues / review_github_pr (post:false, online, minSeverity low, maxComments 30, whole PR): 0 comments, sources ast 0 / cross_module 0 / online 0 / yaml 0, graphState: ready. Deterministic tools found nothing; every finding below comes from CLI probes (source tag [probe]) or the isolated adversarial pass (reproduced by me).

Findings

Major

[probe] phase complete skips the next prefix-related custom phase. Custom project with phases SETUP, SETUP-2, API in roadmap order: phase complete SETUP returns next_phase: "API", skipping SETUP-2 (reproduced on this head). The cause is in comparePhaseNum in src/phase-id.cts: its identity step runs the id through customDirectoryOwner, so SETUP-2 collapses to SETUP and compares equal. The same aliasing in isCompletePhaseNum means a [x] A-b checkbox marks A complete (A, A-b, B: phase complete X yields B, skipping A). This is the exact id family the PR now allows to coexist, so the writer accepts what the lifecycle then mishandles. State is persisted with the wrong current_phase/next_phase. Fix: compare custom ids by exact recorded id first and fall back to directory ownership only for markerless input; add a regression for SETUP/SETUP-2/API and A/A-b/B.
[probe, adversarial pass] Same root cause on the completion side: isCompletePhaseNum aliasing marks a phase complete from another phase's checkbox (see 1). Needs its own assertion in the regression.

Minor
3. [adversarial pass] validateCustomPhaseIds in src/phase-locator.cts accepts ids that phases list and roadmap.analyze then hide (999, 0, 0.5, 00-00: all numeric-shaped). Refuse numeric-shaped custom ids at the writer, the same way fold-equal ids are refused.

CI state for `857ec0a1f5` (latest run per name)

Failing: test (ubuntu-latest, 24, shard 3/3), test (ubuntu-latest, 24) and conformance test (macos-latest, 24), all the single test differential attribution over the real tree (tests/emitted-attribution.test.cjs): plan-phase.md grew 773 bytes plus Emitted-Drift-Ack-Growth trailer key "plan-phase.md" is declared twice with ambiguous, conflicting reasons. Attribution: this PR touches no gsd-core/workflows/* file. The merge-base is d8c3947; since then bc5dfc2887 (#5651) and 39bb1b367a (#5644) landed on next and each grew plan-phase.md with its own ack, and the gate (run on the merge ref against the older base sha) counts both. That is base drift, not this diff; a merge of current next clears the red check. Required tests is the aggregate and Reviewed or maintainer is expected until approval. Windows shards, Stryker, lint, docs-lint, changeset-lint are green. Nothing queued or action_required.

This CI attribution says nothing about the diff: the 2 Major findings and 1 Minor finding above stand independently and still require a push. CI release is moot (nothing pending). I did not push a rebase.

Verdict

Changes requested (Major 1 to 2, Minor 3). Please fix the comparator/complete aliasing with the regression, then merge next (or rely on the merge queue) to clear the base-drift red.

trek-e

Re-review of `857ec0a1f5` (round 6: the 19 findings of the 2026-10-23 review plus the four follow-up commits)

Verdict: changes requested. 0 Blocker, 2 Major, 1 Minor. All 19 prior findings are verified fixed on this head; the two Majors below are new and sit in the same area (a custom id treated as a prefix of another).

Classification and gate compliance

Linked issue #5186: OPEN, labels bug, confirmed-bug (Bug-Fix Track). RULESET.CONTRIB.CLASSIFY.fix=requires confirmed/confirmed-bug before implementation: satisfied.
Author follow-up (2026-10-23 22:51, "all 19 findings addressed") is the subject of this review; no question is left open for me. The branch is 10 commits behind next and merges cleanly (git merge-tree).

Verification of the prior round (CLI probes in throwaway projects, head built)

Fixed and re-probed: fold-equal ids (1/01, Foo/FOO, CCM-FOO, batch same same) refused before any write with typed reasons; SETUP then SETUP-2 creatable and each resolves to its own directory; API / API-v2 resolve and phase remove API-v2 deletes only its own directory; the A+title b x / A-b shared-target case is refused; full directory-name lookup works again; progress and roadmap.analyze report roadmap order; Windows conformance shards 1 to 3 are now green; docs/CONFIGURATION.md documents the contract. node --test on the three PR test files: 918 pass, 0 fail.

Blast radius and risk tier

matchPhaseDirs and findPhaseInternal (signature changed) are the shared locators: 14 and 17 callers, all in files this PR changes. The graph head is 0ce32128b3 on next, so none of this PR's new symbols exist in it; caller counts are pre-PR and partial evidence. No changed symbol is in the PageRank or bridge top lists (same as the previous round).

Co-change completeness

Same result as the previous round: no missing co-change partner of substance; docs, CONTEXT.md, changeset and conformance tier lists are all in the diff.

Memtrace Evidence

get_symbol_context (matchPhaseDirs, branch next): ambiguous (eight declarations, seven of them import constants); the resolved function is src/phase-id.cts:1474, pre-PR shape. The graph predates this PR, so it cannot see the new resolvers. Partial evidence only.
get_impact (matchPhaseDirs, upstream): returned the same ambiguity (risk: UNKNOWN, no traversal performed). I rely on the previous round's node-id-pinned impact (HIGH, 48 symbols) and on the diff.
recall_decision (custom phase_naming, .phase-id, project_code): returned ADR-612 and ADR-2121 (phase-identifier parsing consolidation) among unrelated rows. governing_rules on src/phase-id.cts returned only ADR-443 (effort routing), unrelated. No recorded decision is proven to govern this change; that is not proof none applies. check:phase-id-drift passes in CI.
find_code_review_issues / review_github_pr (post:false, online, minSeverity low, maxComments 30, whole PR): 0 comments, sources ast 0 / cross_module 0 / online 0 / yaml 0, graphState: ready. Deterministic tools found nothing; every finding below comes from CLI probes (source tag [probe]) or the isolated adversarial pass (reproduced by me).

Findings

Major

[probe] phase complete skips the next prefix-related custom phase. Custom project with phases SETUP, SETUP-2, API in roadmap order: phase complete SETUP returns next_phase: "API", skipping SETUP-2 (reproduced on this head). The cause is in comparePhaseNum in src/phase-id.cts: its identity step runs the id through customDirectoryOwner, so SETUP-2 collapses to SETUP and compares equal. The same aliasing in isCompletePhaseNum means a [x] A-b checkbox marks A complete (A, A-b, B: phase complete X yields B, skipping A). This is the exact id family the PR now allows to coexist, so the writer accepts what the lifecycle then mishandles. State is persisted with the wrong current_phase/next_phase. Fix: compare custom ids by exact recorded id first and fall back to directory ownership only for markerless input; add a regression for SETUP/SETUP-2/API and A/A-b/B.
[probe, adversarial pass] Same root cause on the completion side: isCompletePhaseNum aliasing marks a phase complete from another phase's checkbox (see 1). Needs its own assertion in the regression.

Minor
3. [adversarial pass] validateCustomPhaseIds in src/phase-locator.cts accepts ids that phases list and roadmap.analyze then hide (999, 0, 0.5, 00-00: all numeric-shaped). Refuse numeric-shaped custom ids at the writer, the same way fold-equal ids are refused.

CI state for `857ec0a1f5` (latest run per name)

Failing: test (ubuntu-latest, 24, shard 3/3), test (ubuntu-latest, 24) and conformance test (macos-latest, 24), all the single test differential attribution over the real tree (tests/emitted-attribution.test.cjs): plan-phase.md grew 773 bytes plus Emitted-Drift-Ack-Growth trailer key "plan-phase.md" is declared twice with ambiguous, conflicting reasons. Attribution: this PR touches no gsd-core/workflows/* file. The merge-base is d8c3947; since then bc5dfc2887 (#5651) and 39bb1b367a (#5644) landed on next and each grew plan-phase.md with its own ack, and the gate (run on the merge ref against the older base sha) counts both. That is base drift, not this diff; a merge of current next clears the red check. Required tests is the aggregate and Reviewed or maintainer is expected until approval. Windows shards, Stryker, lint, docs-lint, changeset-lint are green. Nothing queued or action_required.

This CI attribution says nothing about the diff: the 2 Major findings and 1 Minor finding above stand independently and still require a push. CI release is moot (nothing pending). I did not push a rebase.

Skills and passes run

/skills-from-the-artificer (preset bugfix-review, block below) and one isolated adversarial reviewer (pr-5509-adversarial.md). I did not run /diagnose, /qa-test-architect, /code-review or /security-review as separate skill calls this round; reproduction was by CLI probes against the head.

Verdict

Changes requested (Major 1 to 2, Minor 3). Please fix the comparator/complete aliasing with the regression, then merge next (or rely on the merge queue) to clear the base-drift red.

Laws that apply

Subject: PR #5509 gives custom phase ids a reader-side inverse (.phase-id markers, phaseDirectoryName, custom-aware readers) and re-threads the lifecycle commands; phase.add --id outside custom naming becomes a usage error.

postels-law — fired: writers are now strict (grammar, fold-equal and prefix refusals) but the lifecycle comparators are not: comparePhaseNum/isCompletePhaseNum in src/phase-id.cts normalise a custom id to its directory owner, so SETUP-2 equals SETUP and phase complete SETUP skips it (Major 1 and 2); validateCustomPhaseIds in src/phase-locator.cts accepts numeric-shaped ids the readers hide (Minor 3). Action: normalise once at the edge and compare the exact recorded id downstream.
hyrums-law — fired: phase.add --id outside custom naming, markerless-directory refusal in phase remove, and the typed refusal reasons are new observable behaviour; the PR discloses them under Breaking changes and restored directory-name lookup. Action: none beyond the findings; disclosure is adequate.
leaky-abstractions — fired: phaseDirectoryOptions in src/phase-locator.cts re-implements the config loader's file selection instead of asking the loader for a quiet read (CONTEXT.md now records it). Action: none required this round; candidate follow-up.
kernighans-law — fired: the comparator/ownership resolution layers (recorded id, fold match, markerless owner, directory name) are what produced the wrong-target defect in Major 1. Action: a decision table in the src/phase-id.cts header plus the regression for SETUP/SETUP-2/API.
Considered and cleared: kerckhoffs-principle (marker reads and path containment hold, no secret involved), knuths-optimization-principle (perf measured by the author and previous round).
Action items: 1. Fix comparator/complete aliasing (Major 1 to 2). 2. Refuse numeric-shaped custom ids (Minor 3).

trek-e

Re-review of `857ec0a1f5` (round 6: the 19 findings of the 2026-10-23 review plus the four follow-up commits)

Verdict: changes requested. 0 Blocker, 2 Major, 3 Minor. All 19 prior findings are verified fixed on this head; the two Majors below are new and sit in the same area (a custom id treated as a prefix of another).

Classification and gate compliance

Linked issue #5186: OPEN, labels bug, confirmed-bug (Bug-Fix Track). RULESET.CONTRIB.CLASSIFY.fix=requires confirmed/confirmed-bug before implementation: satisfied.
Author follow-up (2026-10-23 22:51, "all 19 findings addressed") is the subject of this review; no question is left open for me. The branch is 10 commits behind next and merges cleanly (git merge-tree).

Verification of the prior round (CLI probes in throwaway projects, head built)

Fixed and re-probed: fold-equal ids (1/01, Foo/FOO, CCM-FOO, batch same same) refused before any write with typed reasons; SETUP then SETUP-2 creatable and each resolves to its own directory; API / API-v2 resolve and phase remove API-v2 deletes only its own directory; the A+title b x / A-b shared-target case is refused; full directory-name lookup works again; progress and roadmap.analyze report roadmap order; Windows conformance shards 1 to 3 are now green; docs/CONFIGURATION.md documents the contract. node --test on the three PR test files: 918 pass, 0 fail.

Blast radius and risk tier

matchPhaseDirs and findPhaseInternal (signature changed) are the shared locators: 14 and 17 callers, all in files this PR changes. The graph head is 0ce32128b3 on next, so none of this PR's new symbols exist in it; caller counts are pre-PR and partial evidence. No changed symbol is in the PageRank or bridge top lists (same as the previous round).

Co-change completeness

Same result as the previous round: no missing co-change partner of substance; docs, CONTEXT.md, changeset and conformance tier lists are all in the diff.

Memtrace Evidence

Graph caveat for everything below: the index is at 0ce32128b3 on next, so none of this PR's new symbols (customPhaseQuery, phaseDirectoryOptions, validateCustomPhaseIds) exist in it. Graph results describe the pre-PR code only.

get_symbol_context (matchPhaseDirs, branch next): returned ambiguous (eight same-named declarations, seven of them imported constants), no traversal; the function is src/phase-id.cts:1474. Reissuing with a node id was not done; caller facts come from analyze_relationships and preflight_check below.
get_impact (matchPhaseDirs, upstream, depth 3): returned ambiguous, risk: UNKNOWN, no traversal. No blast-radius number from this call. analyze_relationships (comparePhaseNum, find_callers, depth 1): 19 callers, all global-fuzzy confidence 0.6, spanning phase.cts, smart-entry.cts, init.cts, commands.cts, state-transition.cts, phase-locator.cts, verify-command-grounding.cts, core-utils.cts, health-diagnostic-rules/phase-structure.cts. preflight_check (comparePhaseNum) listed 12 target files and its impact block was likewise ambiguous. get_function_quality_metrics (comparePhaseNum, pre-PR): cyclomatic 32, cognitive 47, 16 direct callers, risk "high" (this tool also weighs callers); the PR adds a custom branch to it.
recall_decision (custom phase_naming, .phase-id, project_code): returned ADR-612 (bracket convention) and ADR-2121 (phase-identifier parsing consolidation) among unrelated rows; governing_rules on src/phase-id.cts returned only ADR-443 (effort routing), unrelated. I did not run verify_intent/get_arc on those ids. No recorded decision is proven to govern this change; that is not proof none applies.
find_code_review_issues (standalone, review_mode: online, max_candidates: 200, graph_mode: strict, repo_id set): 0 issues, _graph_state: ready, sources ast 0 / cross_module 0 / online 0 / yaml 0. Scope limit: I passed a condensed diff (the signature-changing and security-relevant hunks of src/phase-id.cts and src/phase-locator.cts: comparePhaseNum, matchPhaseDirs, phaseMarkdownRegexSource, createPhaseDirectories, the marker read), not the full 4867-line diff and not phase.cts, state.cts, init.cts, roadmap.cts or the rest of src/. find_cross_module_issues (signature hunks of comparePhaseNum, matchPhaseDirs, searchPhaseInDir, findPhaseInternal, no base_sha_files): 0 issues, _graph_state: ready; without base files the removed-symbol and signature-drift patterns could not fire. find_yaml_rule_matches (marker-read and containment hunks only): 0. find_ast_review_issues: skipped, no .py in the diff. review_github_pr (post:false, online, minSeverity low, maxComments 30, whole PR fetched by the tool): 0 comments, same source counts. Earlier in this run, detect_changes with a bare file list returned empty affected_symbols (the graph predates the PR), so it carries no information.
Risk tier: find_central_symbols and find_bridge_symbols (bounded projection, 100000 of 100001 nodes, next): top 25 of each are test fixtures and scripts (runGsdTools, worktree-safety.test.cjs helpers); no changed symbol appears, so there is no PageRank or chokepoint signal. find_dead_code returned only unrelated scripts/ entries (limit 5) and cannot see the PR's new symbols. find_hotspots (14 d): one hotspot, cellResult in src/phase.cts:2432, score 14, low. get_style_fingerprint (src/phase-id.cts): the only delta is early-return share 0.72 versus codebase 0.88 (a Nit by this tool's own calibration, not a finding). get_cochange_context (src/phase-id.cts, 30 d): single-commit coupling only (docs/INVENTORY*, eslint.config.mjs, package.json, one commit), nothing absent from the diff that matters.
Deterministic tools found nothing; every finding below is tagged [probe] (reproduced by command on this head), [adversarial] or [std]/[spec] (sub-agent passes, with files), and the cause of Major 1 is read from the comparePhaseNum hunk (identity() passes the raw text through customDirectoryOwner).

Findings

Major

[probe] phase complete skips the next prefix-related custom phase. Custom project with phases SETUP, SETUP-2, API in roadmap order: phase complete SETUP returns next_phase: "API", skipping SETUP-2 (reproduced on this head). The cause is in comparePhaseNum in src/phase-id.cts: its identity step runs the id through customDirectoryOwner, so SETUP-2 collapses to SETUP and compares equal. The same aliasing in isCompletePhaseNum means a [x] A-b checkbox marks A complete (A, A-b, B: phase complete X yields B, skipping A). This is the exact id family the PR now allows to coexist, so the writer accepts what the lifecycle then mishandles. State is persisted with the wrong current_phase/next_phase. Fix: compare custom ids by exact recorded id first and fall back to directory ownership only for markerless input; add a regression for SETUP/SETUP-2/API and A/A-b/B.
[probe, adversarial pass] Same root cause on the completion side: isCompletePhaseNum aliasing marks a phase complete from another phase's checkbox (see 1). Needs its own assertion in the regression.

Minor
3. [adversarial pass] validateCustomPhaseIds in src/phase-locator.cts accepts ids that phases list and roadmap.analyze then hide (999, 0, 0.5, 00-00: all numeric-shaped). Refuse numeric-shaped custom ids at the writer, the same way fold-equal ids are refused.

[spec pass, judgment] A markerless legacy directory SETUP beside a SETUP-2-b directory makes find-phase SETUP return found:false (ambiguity refusal, by design per the author). Acceptable if documented in docs/CONFIGURATION.md with the .phase-id remedy; please confirm it is.
[std pass] No regression test pins Major 1: the existing completion tests cover AUTH->BILLING and A-2, not an id that is a segment-prefix of a later phase (SETUP/SETUP-2/API, A/A-b/B). Add one in tests/custom-phase-readers.test.cjs that fails on this head.

CI state for `857ec0a1f5` (latest run per name)

Failing: test (ubuntu-latest, 24, shard 3/3), test (ubuntu-latest, 24) and conformance test (macos-latest, 24), all the single test differential attribution over the real tree (tests/emitted-attribution.test.cjs): plan-phase.md grew 773 bytes plus Emitted-Drift-Ack-Growth trailer key "plan-phase.md" is declared twice with ambiguous, conflicting reasons. Attribution: this PR touches no gsd-core/workflows/* file. The merge-base is d8c3947; since then bc5dfc2887 (#5651) and 39bb1b367a (#5644) landed on next and each grew plan-phase.md with its own ack, and the gate (run on the merge ref against the older base sha) counts both. That is base drift, not this diff; a merge of current next clears the red check. Required tests is the aggregate and Reviewed or maintainer is expected until approval. Windows shards, Stryker, lint, docs-lint, changeset-lint are green. Nothing queued or action_required.

This CI attribution says nothing about the diff: the 2 Major findings and 3 Minor findings above stand independently and still require a push. CI release is moot (nothing pending). I did not push a rebase.

Skills and passes run

Skill-tool calls made: skills-from-the-artificer (preset bugfix-review), diagnosing-bugs, qa-test-architect, code-review, security-review. Notes on how each was actually applied: diagnosing-bugs and qa-test-architect were applied by me through CLI probes of the head (the red loop is phase complete SETUP returning next_phase: "API"); code-review ran as a Standards sub-agent and a Spec sub-agent (pr-5509-standards2.md, pr-5509-spec2.md); the security-review skill's embedded diff was for a different branch, so I ran its brief as a sub-agent against this PR (pr-5509-security2.md: clean, no finding at confidence 8 or higher). The isolated adversarial reviewer is pr-5509-adversarial.md. /root-cause-analysis is not an installed skill and rubber-duck is not part of the bug-fix track; neither was run. Deterministic tools were limited as stated under Memtrace Evidence.

Verdict

Changes requested (Major 1 to 2, Minor 3 to 5). Please fix the comparator/complete aliasing with the regression, then merge next (or rely on the merge queue) to clear the base-drift red.

Laws that apply

Subject: PR #5509 gives custom phase ids a reader-side inverse (.phase-id markers, phaseDirectoryName, custom-aware readers) and re-threads the lifecycle commands; phase.add --id outside custom naming becomes a usage error.

postels-law — fired: writers are now strict (grammar, fold-equal and prefix refusals) but the lifecycle comparators are not: comparePhaseNum/isCompletePhaseNum in src/phase-id.cts normalise a custom id to its directory owner, so SETUP-2 equals SETUP and phase complete SETUP skips it (Major 1 and 2); validateCustomPhaseIds in src/phase-locator.cts accepts numeric-shaped ids the readers hide (Minor 3). Action: normalise once at the edge and compare the exact recorded id downstream.
hyrums-law — fired: phase.add --id outside custom naming, markerless-directory refusal in phase remove, and the typed refusal reasons are new observable behaviour; the PR discloses them under Breaking changes and restored directory-name lookup. Action: none beyond the findings; disclosure is adequate.
leaky-abstractions — fired: phaseDirectoryOptions in src/phase-locator.cts re-implements the config loader's file selection instead of asking the loader for a quiet read (CONTEXT.md now records it). Action: none required this round; candidate follow-up.
kernighans-law — fired: the comparator/ownership resolution layers (recorded id, fold match, markerless owner, directory name) are what produced the wrong-target defect in Major 1. Action: a decision table in the src/phase-id.cts header plus the regression for SETUP/SETUP-2/API.
Considered and cleared: kerckhoffs-principle (marker reads and path containment hold, no secret involved), knuths-optimization-principle (perf measured by the author and previous round).
Action items: 1. Fix comparator/complete aliasing (Major 1 to 2). 2. Refuse numeric-shaped custom ids (Minor 3).

trek-e

Re-review of `857ec0a1f5` (round 6: the 19 findings of the 2026-10-23 review plus the four follow-up commits)

Verdict: changes requested. 0 Blocker, 2 Major, 4 Minor. All 19 prior findings are verified fixed on this head; the two Majors below are new and sit in the same area (a custom id treated as a prefix of another).

Classification and gate compliance

Linked issue #5186: OPEN, labels bug, confirmed-bug (Bug-Fix Track). RULESET.CONTRIB.CLASSIFY.fix=requires confirmed/confirmed-bug before implementation: satisfied.
Author follow-up (2026-10-23 22:51, "all 19 findings addressed") is the subject of this review; no question is left open for me. The branch is 10 commits behind next and merges cleanly (git merge-tree).

Verification of the prior round (CLI probes in throwaway projects, head built)

Fixed and re-probed: fold-equal ids (1/01, Foo/FOO, CCM-FOO, batch same same) refused before any write with typed reasons; SETUP then SETUP-2 creatable and each resolves to its own directory; API / API-v2 resolve and phase remove API-v2 deletes only its own directory; the A+title b x / A-b shared-target case is refused; full directory-name lookup works again; progress and roadmap.analyze report roadmap order; Windows conformance shards 1 to 3 are now green; docs/CONFIGURATION.md documents the contract. node --test on the three PR test files: 918 pass, 0 fail.

Blast radius and risk tier

matchPhaseDirs and findPhaseInternal (signature changed) are the shared locators: 14 and 17 callers, all in files this PR changes. The graph head is 0ce32128b3 on next, so none of this PR's new symbols exist in it; caller counts are pre-PR and partial evidence. No changed symbol is in the PageRank or bridge top lists (same as the previous round).

Co-change completeness

Same result as the previous round: no missing co-change partner of substance; docs, CONTEXT.md, changeset and conformance tier lists are all in the diff.

Memtrace Evidence

Graph caveat for everything below: the index is at 0ce32128b3 on next, so none of this PR's new symbols (customPhaseQuery, phaseDirectoryOptions, validateCustomPhaseIds) exist in it. Graph results describe the pre-PR code only.

get_symbol_context (matchPhaseDirs, branch next): returned ambiguous (eight same-named declarations, seven of them imported constants), no traversal; the function is src/phase-id.cts:1474. Reissuing with a node id was not done; caller facts come from analyze_relationships and preflight_check below.
get_impact (matchPhaseDirs, upstream, depth 3): returned ambiguous, risk: UNKNOWN, no traversal. No blast-radius number from this call. analyze_relationships (comparePhaseNum, find_callers, depth 1): 19 callers, all global-fuzzy confidence 0.6, spanning phase.cts, smart-entry.cts, init.cts, commands.cts, state-transition.cts, phase-locator.cts, verify-command-grounding.cts, core-utils.cts, health-diagnostic-rules/phase-structure.cts. preflight_check (comparePhaseNum) listed 12 target files and its impact block was likewise ambiguous. get_function_quality_metrics (comparePhaseNum, pre-PR): cyclomatic 32, cognitive 47, 16 direct callers, risk "high" (this tool also weighs callers); the PR adds a custom branch to it.
recall_decision (custom phase_naming, .phase-id, project_code): returned ADR-612 (bracket convention) and ADR-2121 (phase-identifier parsing consolidation) among unrelated rows; governing_rules on src/phase-id.cts returned only ADR-443 (effort routing), unrelated. I did not run verify_intent/get_arc on those ids. No recorded decision is proven to govern this change; that is not proof none applies.
find_code_review_issues (standalone, review_mode: online, max_candidates: 200, graph_mode: strict, repo_id set): 0 issues, _graph_state: ready, sources ast 0 / cross_module 0 / online 0 / yaml 0. Scope limit: I passed a condensed diff (the signature-changing and security-relevant hunks of src/phase-id.cts and src/phase-locator.cts: comparePhaseNum, matchPhaseDirs, phaseMarkdownRegexSource, createPhaseDirectories, the marker read), not the full 4867-line diff and not phase.cts, state.cts, init.cts, roadmap.cts or the rest of src/. find_cross_module_issues (signature hunks of comparePhaseNum, matchPhaseDirs, searchPhaseInDir, findPhaseInternal, no base_sha_files): 0 issues, _graph_state: ready; without base files the removed-symbol and signature-drift patterns could not fire. find_yaml_rule_matches (marker-read and containment hunks only): 0. find_ast_review_issues: skipped, no .py in the diff. review_github_pr (post:false, online, minSeverity low, maxComments 30, whole PR fetched by the tool): 0 comments, same source counts. Earlier in this run, detect_changes with a bare file list returned empty affected_symbols (the graph predates the PR), so it carries no information.
Diff-tool coverage, exactly (a follow-up pass pasted per-file U0 diffs; several delegated runs were cut off, so coverage is partial and I am not claiming more): src/phase-id.cts full file: detect_changes, find_cross_module_issues (no base_sha_files, base is over 100 KB), find_code_review_issues (online, graph strict; the server reported _max_candidates 100 although 500 was requested) and find_yaml_rule_matches, all 0 issues, _graph_state: ready. src/phase-locator.cts full file: detect_changes, find_code_review_issues, find_cross_module_issues (one hunk header was altered, so treat as partial), 0 issues; find_yaml_rule_matches NOT covered beyond my earlier marker-read and containment hunks. Tests: tests/phase.test.cjs, custom-phase-identity-boundaries.test.cjs, helpers.cjs, cli-exit.test.cjs, io.test.cjs, smart-entry.unit.test.cjs each ran detect_changes, find_code_review_issues and find_yaml_rule_matches (18 calls, 0 issues). NOT covered by any deterministic diff tool: src/phase.cts, src/state.cts, src/init.cts, src/roadmap.cts, src/commands.cts (beyond one detect_changes that returned no affected symbols), src/smart-entry.cts, the other 14 small src/ files, tests/custom-phase-readers.test.cjs (a single 1204-line hunk), docs, CONTEXT.md, changeset and the two generated tier lists. review_github_pr did fetch and scan the whole PR with 0 comments, which is the only whole-diff deterministic result.
get_cochange_context ran for 33 changed files (all src/ files, docs/CONFIGURATION.md, CONTEXT.md and five test files; 17 returned empty). The only partner with 2 or more co-changes absent from the PR is tests/roadmap.test.cjs (pairs with src/roadmap.cts); low signal. analyze_relationships ran on matchPhaseDirs, findPhaseInternal, searchPhaseInDir, phaseMarkdownRegexSource, phaseKeyFromToken, phaseKeyFromDir, parsePhaseFromProse, buildPhaseHeadingScanRegex, getMilestonePhaseFilter, listMilestonePhaseDirs (find_callers; pre-PR counts 14, 17, 1, 13, 8, 7, 5, 7, 4, 18) and comparePhaseNum (19); the exporters queries were inconclusive. Call sites were then checked on the head checkout: matchPhaseDirs, findPhaseInternal and searchPhaseInDir are fully re-threaded.
Risk tier: find_central_symbols and find_bridge_symbols (bounded projection, 100000 of 100001 nodes, next): top 25 of each are test fixtures and scripts (runGsdTools, worktree-safety.test.cjs helpers); no changed symbol appears, so there is no PageRank or chokepoint signal. find_dead_code returned only unrelated scripts/ entries (limit 5) and cannot see the PR's new symbols. find_hotspots (14 d): one hotspot, cellResult in src/phase.cts:2432, score 14, low. get_style_fingerprint (src/phase-id.cts): the only delta is early-return share 0.72 versus codebase 0.88 (a Nit by this tool's own calibration, not a finding). get_cochange_context (src/phase-id.cts, 30 d): single-commit coupling only (docs/INVENTORY*, eslint.config.mjs, package.json, one commit), nothing absent from the diff that matters.
Deterministic tools found nothing; every finding below is tagged [probe] (reproduced by command on this head), [adversarial] or [std]/[spec] (sub-agent passes, with files), and the cause of Major 1 is read from the comparePhaseNum hunk (identity() passes the raw text through customDirectoryOwner).

Findings

Major

[probe] phase complete skips the next prefix-related custom phase. Custom project with phases SETUP, SETUP-2, API in roadmap order: phase complete SETUP returns next_phase: "API", skipping SETUP-2 (reproduced on this head). The cause is in comparePhaseNum in src/phase-id.cts: its identity step runs the id through customDirectoryOwner, so SETUP-2 collapses to SETUP and compares equal. The same aliasing in isCompletePhaseNum means a [x] A-b checkbox marks A complete (A, A-b, B: phase complete X yields B, skipping A). This is the exact id family the PR now allows to coexist, so the writer accepts what the lifecycle then mishandles. State is persisted with the wrong current_phase/next_phase. Fix: compare custom ids by exact recorded id first and fall back to directory ownership only for markerless input; add a regression for SETUP/SETUP-2/API and A/A-b/B.
[probe, adversarial pass] Same root cause on the completion side: isCompletePhaseNum aliasing marks a phase complete from another phase's checkbox (see 1). Needs its own assertion in the regression.

Minor
3. [adversarial pass] validateCustomPhaseIds in src/phase-locator.cts accepts ids that phases list and roadmap.analyze then hide (999, 0, 0.5, 00-00: all numeric-shaped). Refuse numeric-shaped custom ids at the writer, the same way fold-equal ids are refused.

[spec pass, judgment] A markerless legacy directory SETUP beside a SETUP-2-b directory makes find-phase SETUP return found:false (ambiguity refusal, by design per the author). Acceptable if documented in docs/CONFIGURATION.md with the .phase-id remedy; please confirm it is.
[std pass] No regression test pins Major 1: the existing completion tests cover AUTH->BILLING and A-2, not an id that is a segment-prefix of a later phase (SETUP/SETUP-2/API, A/A-b/B). Add one in tests/custom-phase-readers.test.cjs that fails on this head.
[graph + checkout read] New-context argument is not passed at call sites where a context is in scope, so those sites keep the context-free (numeric) behaviour in custom projects: src/phase.cts:4109 and :4429 (phaseMarkdownRegexSource(phaseNum) in phase complete, directoryContext in scope; the roadmap checkbox lookup is the place Major 2's aliasing should be pinned), src/phase.cts:596 and :880 (comparePhaseNum as a directory sort comparator), src/smart-entry.cts:351, src/health-diagnostic-rules/phase-structure.cts:121; src/state.cts:7182/:7255 (parsePhaseFromProse) and src/planning-inspect.cts:1099 receive no context from their callers. Lower risk: listMilestonePhaseDirs(dir) without options in src/estimate-cli.cts:243 and src/learnings.cts:219 (unchanged by the PR). Please thread the context or state why each is deliberately numeric-only.

CI state for `857ec0a1f5` (latest run per name)

Failing: test (ubuntu-latest, 24, shard 3/3), test (ubuntu-latest, 24) and conformance test (macos-latest, 24), all the single test differential attribution over the real tree (tests/emitted-attribution.test.cjs): plan-phase.md grew 773 bytes plus Emitted-Drift-Ack-Growth trailer key "plan-phase.md" is declared twice with ambiguous, conflicting reasons. Attribution: this PR touches no gsd-core/workflows/* file. The merge-base is d8c3947; since then bc5dfc2887 (#5651) and 39bb1b367a (#5644) landed on next and each grew plan-phase.md with its own ack, and the gate (run on the merge ref against the older base sha) counts both. That is base drift, not this diff; a merge of current next clears the red check. Required tests is the aggregate and Reviewed or maintainer is expected until approval. Windows shards, Stryker, lint, docs-lint, changeset-lint are green. Nothing queued or action_required.

This CI attribution says nothing about the diff: the 2 Major findings and 4 Minor findings above stand independently and still require a push. CI release is moot (nothing pending). I did not push a rebase.

Skills and passes run

Skill-tool calls made: skills-from-the-artificer (preset bugfix-review), diagnosing-bugs, qa-test-architect, code-review, security-review. Notes on how each was actually applied: diagnosing-bugs and qa-test-architect were applied by me through CLI probes of the head (the red loop is phase complete SETUP returning next_phase: "API"); code-review ran as a Standards sub-agent and a Spec sub-agent (pr-5509-standards2.md, pr-5509-spec2.md); the security-review skill's embedded diff was for a different branch, so I ran its brief as a sub-agent against this PR (pr-5509-security2.md: clean, no finding at confidence 8 or higher). The isolated adversarial reviewer is pr-5509-adversarial.md. /root-cause-analysis is not an installed skill and rubber-duck is not part of the bug-fix track; neither was run. Deterministic tools were limited as stated under Memtrace Evidence.

Verdict

Changes requested (Major 1 to 2, Minor 3 to 6). Please fix the comparator/complete aliasing with the regression, then merge next (or rely on the merge queue) to clear the base-drift red.

Laws that apply

Subject: PR #5509 gives custom phase ids a reader-side inverse (.phase-id markers, phaseDirectoryName, custom-aware readers) and re-threads the lifecycle commands; phase.add --id outside custom naming becomes a usage error.

postels-law — fired: writers are now strict (grammar, fold-equal and prefix refusals) but the lifecycle comparators are not: comparePhaseNum/isCompletePhaseNum in src/phase-id.cts normalise a custom id to its directory owner, so SETUP-2 equals SETUP and phase complete SETUP skips it (Major 1 and 2); validateCustomPhaseIds in src/phase-locator.cts accepts numeric-shaped ids the readers hide (Minor 3). Action: normalise once at the edge and compare the exact recorded id downstream.
hyrums-law — fired: phase.add --id outside custom naming, markerless-directory refusal in phase remove, and the typed refusal reasons are new observable behaviour; the PR discloses them under Breaking changes and restored directory-name lookup. Action: none beyond the findings; disclosure is adequate.
leaky-abstractions — fired: phaseDirectoryOptions in src/phase-locator.cts re-implements the config loader's file selection instead of asking the loader for a quiet read (CONTEXT.md now records it). Action: none required this round; candidate follow-up.
kernighans-law — fired: the comparator/ownership resolution layers (recorded id, fold match, markerless owner, directory name) are what produced the wrong-target defect in Major 1. Action: a decision table in the src/phase-id.cts header plus the regression for SETUP/SETUP-2/API.
Considered and cleared: kerckhoffs-principle (marker reads and path containment hold, no secret involved), knuths-optimization-principle (perf measured by the author and previous round).
Action items: 1. Fix comparator/complete aliasing (Major 1 to 2). 2. Refuse numeric-shaped custom ids (Minor 3). 3. Thread the context at the Minor 6 sites.

Compare recorded ids before directory ownership so prefix phases stay distinct. Pin next-phase selection, checkbox membership and persisted STATE position. Concurrent files left unstaged: none.

Reuse the readers' sentinel predicate before any custom directory write. Cover add, scaffold and whole-batch refusals without rejecting 2FA or 7B. Concurrent files left unstaged: none.

Thread naming context into completion, state, ordering and goal lookups. Keep health collision keys tied to recorded owners instead of token prefixes. Concurrent files left unstaged: none.

Explain markerless lookup refusals and the exact .phase-id repair. Describe reserved writer ids and distinct completion identities. Concurrent files left unstaged: none.

…e-dirs Bring current next into the custom phase fixes and clear attribution base drift. Concurrent files left unstaged: none.

Exclude reserved zero-leading tokens from the successful round-trip domain. Keep lowercase and digit-leading ids while explicit refusal tests cover sentinels. Concurrent files left unstaged: none.

Use the recorded owner in custom roadmap anchors instead of the query alias. Preserve padded numeric completion while threading custom naming context. Concurrent files left unstaged: none.

bshiggins · 2026-10-24T16:24:18Z

Thanks for the round-6 review. Everything is addressed on e139b9964. Each behaviour finding has regressions that fail on 857ec0a1f and pass on the head.

#	Change	Commit
1, 2, 5	`comparePhaseNum` and `isCompletePhaseNum` compare custom ids by exact recorded id, and fold through directory ownership only for markerless input. New `custom lifecycle identity` tests: with `SETUP`, `SETUP-2`, `API`, `phase complete SETUP` gives `next_phase: "SETUP-2"`. With `A`, `A-b`, `B`, `[x] A-b` no longer marks `A` complete and `A` is not skipped. Persisted STATE current and next are asserted.	`b94506fa7`
3	The writer refuses ids the readers' own backlog/icebox predicate would hide (`999`, `0`, `0.5`, `00-00`), before any write. Digit-leading ids that resolve (`2FA`, `7B`, `1.0`) are still accepted. The fast-check round trip now generates only visible ids (`e2df071f5`).	`7dcee82a4`
4	`docs/CONFIGURATION.md` documents the markerless-ambiguity refusal and the exact `.phase-id` remedy	`5c64062d4`
6	Sites below	`c601258e6`, `e139b9964`

Site	Action
`phase.cts:4109`, `:4429`	Context threaded into the checkbox and requirement-section anchors (`e139b9964` keeps a recorded `03` answering a completion query of `3`)
`phase.cts:596`, `:880`	Context threaded into the directory comparators
`smart-entry.cts:351`	Latest-phase fallback uses roadmap order
`health phase-structure.cts:121`	Custom identity keys and contextual sort; legacy grouping kept
`state.cts:7182`, `:7255`	Context threaded through current-phase resolution and the superseded-phase guard
`planning-inspect.cts:1099`	Resolves the directory-owned id and threads context into goal lookup
`estimate-cli.cts:243`	Deliberately unscoped: calibration pairs artifacts across all milestones
`learnings.cts:219`	Deliberately unscoped: selection uses the latest artifact mtime across all milestones

The six threaded sites are covered by new custom reader context tests.

Per your CI note, 8d0c5e179 merges next at eedc1968a to clear the emitted-attribution base drift. The merge was clean; workflow files match that tip, and the attribution test passes locally. Full suite in a CI-like environment (Node 24, empty HOME, no system gitconfig): 46,861 passed, 0 failed, 104 skipped. The Self-test report in the body is bound to this head.

Use the shared home environment for both HOME and USERPROFILE so health checks reach the phase diagnostics instead of the home-directory guard. Assert the Windows home override on every platform. Concurrent files left unstaged: none.

…e-dirs Preserve milestone-prefixed prose parsing alongside custom directory identity context. Regenerate the platform conformance lists. Concurrent files left unstaged: none.

bshiggins requested review from Solvely-Colin, davesienkowski, jeremymcs and trek-e as code owners October 18, 2026 00:13

chore(open-gsd#5186): backfill changeset pr number

2dc5a41

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

trek-e requested changes Oct 18, 2026

View reviewed changes

bshiggins added 2 commits October 18, 2026 10:10

Merge remote-tracking branch 'upstream/next' into fix/5186-custom-cod…

411ce47

…e-dirs

trek-e requested changes Oct 18, 2026

View reviewed changes

bshiggins added 2 commits October 18, 2026 16:41

Merge remote-tracking branch 'upstream/next' into fix/5186-custom-cod…

54f91b8

…e-dirs

bshiggins added 2 commits October 21, 2026 08:29

Merge upstream/next into fix/5186-custom-code-dirs

cc99398

Retain complete custom-ID round-4 regressions and the upstream phase-completion and milestone-fallback regression blocks.

Merge upstream/next into fix/5186-custom-code-dirs

9adebfb

Keep custom phase parsing context with upstream status phrase handling.

trek-e requested changes Oct 22, 2026

View reviewed changes

trek-e added the review: changes requested PR reviewed — changes required before merge label Oct 22, 2026

trek-e mentioned this pull request Oct 22, 2026

fix(#5528): phase add and add-batch count bracket phases when allocating a number #5613

Open

10 of 18 tasks

bshiggins added 7 commits October 22, 2026 15:36

fix(open-gsd#5186): resolve digit-leading custom phase IDs

c371335

Preserve complete custom tokens before numeric normalization. Cover mixed case and digit-leading IDs in both generated round-trip properties. Concurrent files left unstaged: none.

fix(open-gsd#5186): preserve numeric verification routing in custom p…

2e23f1f

…rojects Parse STATE values through the shared prose parser and exclude non-phase directories from the latest-phase fallback. Concurrent files left unstaged: none.

fix(open-gsd#5186): preserve directory-owned custom phase identities

d86672b

Record custom IDs beside phase artifacts, including scaffold and batch creation. Refuse destructive ownership guesses for legacy directories. Concurrent files left unstaged: none.

test(open-gsd#5186): resolve prefix IDs through the production locator

7e63b20

Remove injected identity hints from the heading-free SETUP regression. Exercise directory ownership through the real read path. Concurrent files left unstaged: none.

fix(open-gsd#5186): index custom identities once per naming context

8f85e1c

Reuse heading IDs, their membership set and sorted directory prefixes. Remove the redundant roadmap argument at every query caller. Concurrent files left unstaged: none.

fix(open-gsd#5186): size roadmap hint reads from the file descriptor

7babc6b

Allocate only the file size while retaining the one MiB read bound. Cover empty, small and oversized roadmaps. Concurrent files left unstaged: none.

fix(open-gsd#5186): retain legacy numeric prefix resolution

b679a50

Keep configured numeric, suffix and decimal tokens on their legacy path unless the writer recorded that exact custom ID. Pin CLI and bracket parity. Concurrent files left unstaged: none.

bshiggins added 14 commits October 23, 2026 12:14

fix(open-gsd#5186): enforce distinct custom identities safely

c01918f

Reject equivalent IDs and prefer exact records over aliases. Permit proven prefix owners and refuse only ambiguous markerless mutations. Concurrent files left unstaged: none.

fix(open-gsd#5186): restore full phase directory handles

e25eb37

Match explicit basenames before identity filtering and report the owner ID. Use that owner when removing the roadmap entry. Concurrent files left unstaged: none.

fix(open-gsd#5186): keep inserted numeric families renumberable

64f49f4

Record inserted identities only when their parent has a custom identity. Legacy numeric children retain their parent's renumbering behavior. Concurrent files left unstaged: none.

fix(open-gsd#5186): order custom phase readers by the roadmap

86d674a

Resolve directory identities before ordering and shaping init records. Keep custom phase IDs and names consistent across progress and manager readers. Concurrent files left unstaged: none.

fix(open-gsd#5186): distinguish invalid naming configuration

acbb57b

Accept custom naming case-insensitively without changing the config loader. Report corrupt project config separately from valid config shadowing defaults. Concurrent files left unstaged: none.

fix(open-gsd#5186): share phase directory write validation

216aecc

Use one preflight and writer tail for add, batch, insert and scaffold. Export the context type, preserve optional locator calls and document dependencies. Concurrent files left unstaged: none.

fix(open-gsd#5186): keep parsed custom phase checks coherent

30e1796

Share fixture helpers and assert on typed errors and parsed roadmap data. Thread custom identity into checkbox metadata and remove global test patches. Concurrent files left unstaged: none.

fix(open-gsd#5186): document custom phase naming changes

c6b3a67

Describe custom naming, identity markers, grammar and refusal behavior. Classify the change and disclose the dangling-symlink verification fix. Concurrent files left unstaged: none.

test(open-gsd#5186): refresh portable phase test classification

7600bf5

Regenerate the Windows tier after routing the probes through the process seam. Remove the fixture import made obsolete by the shared helpers. Concurrent files left unstaged: none.

test(open-gsd#5186): cover the extended phase refusal registry

416c27d

Retain the existing reason keys and add the three typed phase refusals. Keep the exit-contract matrix exhaustive across all thirty reasons. Concurrent files left unstaged: none.

fix(open-gsd#5186): prefer recorded ids over directory handles

9fea755

Resolve recorded identities before interpreting a query as a full basename. Cover both creation orders across find, init and destructive removal. Concurrent files left unstaged: none.

fix(open-gsd#5186): refuse occupied phase directory targets

2b6b67c

Preflight existing paths and duplicate batch destinations before any write. Preserve existing identity markers across add, batch, insert and scaffold. Concurrent files left unstaged: none.

fix(open-gsd#5186): limit removal preflight to proven owners

6519bce

Allow a unique markerless target without claiming its longer sibling. Retain ambiguous-owner refusal and document the identity safety fixes. Concurrent files left unstaged: none.

test(open-gsd#5186): distinguish unique and ambiguous removal targets

857ec0a

Align two older refusal assertions with the unique-owner ruling. Keep ambiguous-target refusal and prove the untouched sibling survives. Concurrent files left unstaged: none.

trek-e requested changes Oct 23, 2026

View reviewed changes

bshiggins added 7 commits October 24, 2026 09:12

fix(open-gsd#5186): preserve recorded identity during phase completion

b94506f

Compare recorded ids before directory ownership so prefix phases stay distinct. Pin next-phase selection, checkbox membership and persisted STATE position. Concurrent files left unstaged: none.

fix(open-gsd#5186): refuse custom ids hidden by phase readers

7dcee82

Reuse the readers' sentinel predicate before any custom directory write. Cover add, scaffold and whole-batch refusals without rejecting 2FA or 7B. Concurrent files left unstaged: none.

fix(open-gsd#5186): carry custom identity through remaining readers

c601258

Thread naming context into completion, state, ordering and goal lookups. Keep health collision keys tied to recorded owners instead of token prefixes. Concurrent files left unstaged: none.

fix(open-gsd#5186): document ambiguous lookups and reserved ids

5c64062

Explain markerless lookup refusals and the exact .phase-id repair. Describe reserved writer ids and distinct completion identities. Concurrent files left unstaged: none.

Merge remote-tracking branch 'upstream/next' into fix/5186-custom-cod…

8d0c5e1

…e-dirs Bring current next into the custom phase fixes and clear attribution base drift. Concurrent files left unstaged: none.

test(open-gsd#5186): generate visible custom round trip ids

e2df071

Exclude reserved zero-leading tokens from the successful round-trip domain. Keep lowercase and digit-leading ids while explicit refusal tests cover sentinels. Concurrent files left unstaged: none.

fix(open-gsd#5186): anchor roadmap writes to the resolved custom id

e139b99

Use the recorded owner in custom roadmap anchors instead of the query alias. Preserve padded numeric completion while threading custom naming context. Concurrent files left unstaged: none.

bshiggins added 2 commits October 24, 2026 17:58

Merge remote-tracking branch 'upstream/next' into fix/5186-custom-cod…

9cc1a24

…e-dirs Preserve milestone-prefixed prose parsing alongside custom directory identity context. Regenerate the platform conformance lists. Concurrent files left unstaged: none.

bshiggins mentioned this pull request Oct 25, 2026

fix(#5179): preflight install destinations before writes #5306

Open

11 of 18 tasks

Conversation

bshiggins commented Oct 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix PR

Linked Issue

What was broken

What this fix does

Root cause

Testing

How I verified the fix

Regression test added?

Platforms tested

Runtimes tested

Self-test report

Checklist

Breaking changes

Uh oh!

github-actions Bot commented Oct 18, 2026

Please use the PR template format

Uh oh!

trek-e left a comment

Choose a reason for hiding this comment

Review — fix(#5186): resolve custom phase directories consistently

Summary

Classification & gate compliance

Must-Have Acceptance Checklist (from the Agent Brief on #5186)

Root-cause / functional verdict

Blast radius

Co-change completeness

Test quality

Memtrace Evidence

Findings

CI

Verdict

Uh oh!

bshiggins commented Oct 18, 2026

Uh oh!

trek-e left a comment

Choose a reason for hiding this comment

Review — fix(#5186): resolve custom phase directories consistently (re-review at 5c12b4fcb8)

Findings

Memtrace Evidence

Verdict

Uh oh!

bshiggins commented Oct 18, 2026

Uh oh!

trek-e commented Oct 21, 2026

Merge conflict with next — re-review blocked

Uh oh!

bshiggins commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trek-e commented Oct 22, 2026

Uh oh!

trek-e left a comment

Choose a reason for hiding this comment

Re-review of 9adebfba29 (round 4 + merge of next)

Findings

Process

Memtrace Evidence

Verdict

Uh oh!

bshiggins commented Oct 23, 2026

Uh oh!

trek-e left a comment

Choose a reason for hiding this comment

Re-review of 857ec0a1f5 (round 6: the 19 findings of the 2026-10-23 review plus the four follow-up commits)

Classification and gate compliance

Verification of the prior round (CLI probes in throwaway projects, head built)

Blast radius and risk tier

Co-change completeness

Memtrace Evidence

Findings

CI state for 857ec0a1f5 (latest run per name)

Verdict

Uh oh!

trek-e left a comment

Choose a reason for hiding this comment

Re-review of 857ec0a1f5 (round 6: the 19 findings of the 2026-10-23 review plus the four follow-up commits)

Classification and gate compliance

bshiggins commented Oct 18, 2026 •

edited

Loading

Review — fix(#5186): resolve custom phase directories consistently (re-review at `5c12b4fcb8`)

Merge conflict with `next` — re-review blocked

bshiggins commented Oct 21, 2026 •

edited

Loading

Re-review of `9adebfba29` (round 4 + merge of `next`)

Re-review of `857ec0a1f5` (round 6: the 19 findings of the 2026-10-23 review plus the four follow-up commits)

CI state for `857ec0a1f5` (latest run per name)

Re-review of `857ec0a1f5` (round 6: the 19 findings of the 2026-10-23 review plus the four follow-up commits)

CI state for `857ec0a1f5` (latest run per name)

Re-review of `857ec0a1f5` (round 6: the 19 findings of the 2026-10-23 review plus the four follow-up commits)

CI state for `857ec0a1f5` (latest run per name)

Re-review of `857ec0a1f5` (round 6: the 19 findings of the 2026-10-23 review plus the four follow-up commits)

CI state for `857ec0a1f5` (latest run per name)