Repository navigation
Conversation
…mit subjects `planSubjectPattern` split a plan id at its FIRST dash, so `PRJ-01-01` yielded phase `PRJ` and plan `01-01`; the plan half fails the alphanumeric test, the id fell through to the LITERAL pattern, and every gate that builds its `--plan` from `init.execute-phase`'s project-coded `phase_number` (`safe_resume_gate`, the TDD RED-commit gate, the step-5 spot-check, completion reconciliation) resolved `no-matching-commits` at exit 0 — the correct answer for a plan that has NOT started, and the wrong one for a plan that has, so the safe-resume gate would re-dispatch an executor over a half-executed plan. The split itself stays the first attempt (a bare `03-01` is unchanged); only when it fails does the id retry on the project-code-stripped form, using the phase-id owner's own prefix grammar rather than a second copy of it. The code becomes an optional group, so `PRJ-01-01` resolves the commits the executor actually wrote — `feat(01-01):`, because `init.plan-phase` hands out the BARE phase token — as well as the coded spelling, while a different code, the id boundary (`01-010`), a code-without-a-plan id, and every literal/metacharacter-hostile id behave exactly as before. The optional group is a capturing `(PRJ-)?` and never `(?:PRJ-)`: the same string is handed to `git log --extended-regexp`, and POSIX ERE has no non-capturing group, so `(?:…)` is a syntax error on the git side while the JS-side `RegExp` used by the subject filter stays green.
a9ed5b0 to
032aa7e
Compare
trek-e
left a comment
trek-e
left a comment
There was a problem hiding this comment.
Review of head 032aa7e6a107 — changes requested
Summary. The root-cause fix is correct and well placed: planSubjectPattern split a coded plan id at the first dash, fell through to a literal, and answered resolved with an empty scope at exit 0. Two Majors and several Minors remain.
Classification and gates. #5271 carries bug + confirmed-bug; bug-fix track. Fixes #5271, Fix template, conventional title, not draft. Three files: src/gate-evaluation-scope.cts, tests/gate-evaluation-scope.test.cjs, .changeset/eager-jaguars-tumble.md (type Fixed, pr: 5283, bold lead). No shipped workflow, agent or command changed, so no compact-content baseline, INVENTORY or Emitted-Drift-Ack-Growth requirement. No duplicate PR; the fix is not on next.
Root cause. resolveBySubject's single caller path (src/gate-evaluation-scope.cts:469) builds the subject pattern; the fix reuses phase-id's stripProjectCodePrefix at the resolver instead of special-casing. The (PRJ-)? capturing group is valid ERE.
Fail-first [by reading]. On base, PRJ-01-01 becomes a literal pattern, so commits are []; the new assertions on reason and subjects fail there. The behavioral test uses a real git fixture, so git's ERE is actually exercised.
Security. The id is length-capped (200), control and whitespace characters are rejected, and the prefix goes through escapeEre; no new argv, file or subprocess surface. No finding.
Findings
Major 1 — acceptance criterion 5 is not delivered
The triage Agent Brief says: "The grammar change is shared with the workstream-qualified id from #5177", and the "Coupled (must not miss)" note says the fix here and #5177 "must share one scope grammar accepting <CODE>-<N>-<M> and the workstream-qualified form" (#5177 is open; no PR addresses it). The PR body says it lands "the project-code half". Acceptance criteria are release blockers. Either define the grammar here so it also accepts the workstream-qualified form (with a shared constant or a parity test, so the two cannot diverge), or ask in the issue for criterion 5 to be restated.
Major 2 — the prefix strip is wider than the configured project code [judgment]
The fix strips any [A-Z][A-Z0-9_]*- prefix before a digit (the phase-id grammar), not the configured project_code as the issue's fix sketch has it. So --plan FOO-01-01 resolves plan 01-01's commits, and a literal plan-file id such as setup-1-2, which used to be matched literally, now also matches a bare (1-2). That conflicts with "existing literal plan-file ids behave as before" for hyphenated-alpha names, and no test pins it. Either restrict the strip to the configured code or add rows that document and pin the widened behavior.
Minor 3 — criterion 3 has no test. "A not-started plan still resolves empty at exit 0" is untested for a coded id (for example PRJ-01-09).
Minor 4 — a misleading, non-discriminating assertion. The comment "a code WITHOUT a plan segment is still matched literally" for PRJ-01 is wrong: PRJ-01 takes the direct path (phase PRJ, plan 01), and !matches('PRJ-01','feat(01): a') passes either way.
Minor 5 — missing test classes. No fast-check property for the pattern transform (this repo requires one for parsers and bijective contracts); no limit-1 / limit / limit+1 rows for the 200-character cap with a coded id; and the fall-through where bare !== planId but phasePlanPattern(bare) is null (for example PRJ-01-01-extra) is untested, a surviving-mutant risk under the 80% Stryker threshold.
Minor 6 — branch name. fix-5271-coded-plan-subject-scope does not follow CONTRIBUTING.md (fix/NNN-slug). branch-naming.yml (lines 28-60) only warns, but please resend from fix/5271-coded-plan-subject-scope.
Nit 7. The changeset body uses a curly apostrophe (plan’s), and docs/ARCHITECTURE.md ("Plan commits are matched on the subject") and the CONTEXT.md Evaluation Scope Module entry still describe <type>(<phase>-<plan>) only; extend the grammar sentence to the coded form.
Branch state. BEHIND next by one commit. Note that updating a fork PR's branch re-queues all CI for approval.
Not verified. I did not execute any test (local node --test is not permitted).
CI. On this head the fork matrix has never executed: eight runs are waiting at action_required (Tests, Mutation Testing, Install Smoke, Changeset Required, Docs Required, Validate Branch Name, Default Flip Documentation, Dependabot Auto-Merge). The completed checks (validate-target, template format, validate-title, Issue link required, GitGuardian, Socket ×2) are SUCCESS. That is a maintainer-side gate, not your defect. I am withholding the release: the Majors require a push, which re-triggers the whole matrix.
CodeGraph Evidence
Memtrace unavailable: monthly quota exhausted, so get_impact, get_symbol_context, recall_decision and find_code_review_issues were not run. The recorded-decision check was not evaluated. The index is of the main checkout, so branch-new symbols are absent.
codegraph_impact planSubjectPattern: 6 symbols (planSubjectPatternatsrc/gate-evaluation-scope.cts:184,resolveBySubject:469,resolveEvaluationScope:272, plustests/gate-evaluation-scope.test.cjs,tests/safe-resume-gate-anchoring.test.cjsandtests/execute-phase-decimal-arithmetic.test.cjs).codegraph_callers resolveBySubject: one caller,resolveEvaluationScope(src/gate-evaluation-scope.cts:272).- Finding: a single resolver path, so blast radius is low, but those two other test files pin the grammar and are the places the widened strip (Major 2) could change behavior.
Verdict
Changes requested. Blocker 0, Major 2, Minor 4, Nit 1.
trek-e
added
the
review: changes requested
trek-e
left a comment
trek-e
left a comment
There was a problem hiding this comment.
Review of head 032aa7e6a107 — changes requested
Summary. The reported case works: --plan PRJ-01-01 now resolves both feat(PRJ-01-01): and feat(01-01): commits. The fix uses the generic project-code grammar instead of the configured code, so ids that were never coded now match other plans' commits (AUTH-01-02, setup-1-2, FOO-01-01), the milestone-prefixed coded shape (PRJ-01-02-03) still resolves the wrong spelling, and the shared-grammar acceptance item is not delivered. Blocker 1 (maintainer-side CI), Major 3, Minor 6, Nit 6.
Classification and gates. #5271 carries bug + confirmed-bug (RULESET.CONTRIB.CLASSIFY.fix=requires confirmed-bug before implementation (legacy 'confirmed' label is back-compat only for duplicate-sweep exemption, not a valid implementation gate)). Fixes #5271, conventional title, Fix template present, not draft. Changeset .changeset/eager-jaguars-tumble.md: type Fixed, pr: 5283, bold lead; docs-exempt. No shipped workflow, agent or command changed (no compact-content baseline or Emitted-Drift-Ack-Growth requirement); no references/ or workflows/ file added.
Acceptance criteria, #5271 Agent Brief (verbatim)
- "
--plan PRJ-01-01and--plan 01-01return identical commits in a repo with project codePRJ." — Met for this id (the test fixture sets noproject_code; see Major 1). - "A commit subject
feat(PRJ-01-01):matches the same plan." — Met at the pattern level; not run through git (Minor 3). - "A plan with no commits still resolves empty at exit 0." — Not tested for a coded id (Minor 1).
- "Existing zero-padding, decimal, letter-suffixed and literal plan-file ids behave as before." — Not met:
setup-1-2andAUTH-01-02change (Major 1). - "The grammar change is shared with the workstream-qualified id from #5177." — Not met (Major 2).
Reproduction (plain node: the head and base src/gate-evaluation-scope.cts and src/phase-id.cts transpiled and overlaid on the built library, calling the real planSubjectPattern; each pattern tested against seven commit subjects)
| Plan id | Base matches | Head matches |
|---|---|---|
PRJ-01-01 |
feat(PRJ-01-01): |
feat(PRJ-01-01):, feat(01-01): |
AUTH-01-02 |
none | feat(01-02): unrelated, feat(1-2): |
FOO-01-01 |
none | feat(01-01): |
prj-01-01 |
feat(PRJ-01-01): |
feat(PRJ-01-01):, feat(01-01): |
setup-1-2 |
feat(setup-1-2): |
feat(01-02): unrelated, feat(1-2):, feat(setup-1-2): |
PRJ-01-02-03 |
feat(PRJ-01-02-03): |
feat(PRJ-01-02-03): only (not feat(01-02-03):) |
01-02-03 |
feat(01-02-03): |
feat(01-02-03): |
PRJ-01, PRJ-01-01-extra |
none | none |
Findings
Blocker 1 — the fork matrix never executed on this head [verified] Tests, Mutation Testing, Install Smoke, Docs Required, Changeset Required, Validate Branch Name, Default Flip Documentation, Dependabot Auto-Merge and Dismiss Unauthorized PR Approvals are all completed/action_required for this sha; the rollup holds only the seven policy and scanner checks (all SUCCESS). Maintainer-side, not your defect. I am withholding the release: Majors 1-3 need a push, which re-gates the whole matrix. The branch is also BEHIND next (a rebase is a CI condition, not a finding).
Major 1 — the strip is wider than the configured project code, so non-coded ids now match other plans' commits [verified by executing the head code: table above] src/gate-evaluation-scope.cts planSubjectPattern calls stripProjectCodePrefix, whose grammar is /^[A-Z][A-Z0-9_]*-(?=\d)/i (src/phase-id.cts:32, case-insensitive by default). The resolver never reads project_code. The issue's fix sketch: "Using the configured code rather than a generic ^[A-Za-z]+- rule keeps letter-suffixed phase ids out of it" and "when the --plan id starts with the project's configured project_code followed by a dash, strip that prefix." Consequences: a hyphenated plan-file id such as setup-1-2 (previously literal) now also matches bare feat(1-2): and feat(01-02):, so it resolves another plan's commits — contradicting acceptance 4 — and the planSubjectPattern doc comment ("non-numbered ids are matched literally, can only match a commit that names that exact id") is now false for this class (Minor 4). Fix: strip only the exact configured code plus a dash, case-sensitively, and keep the literal fallback otherwise; add rows for AUTH-01-02, setup-1-2 and a lowercase code. The adversarial pass rated this Minor, the delegated pass and the Spec axis Major; both ratings are kept.
Major 2 — acceptance 5 (shared grammar with #5177) is not delivered [verified] Issue comment: "must share one scope grammar accepting <CODE>-<N>-<M> and the workstream-qualified form". planSubjectPattern/phasePlanPattern contain no workstream dimension and there is no shared constant or parity test; the PR body says it lands "the project-code half". Whether to hold the PR for the shared grammar or restate the item in #5271 is the maintainer's call; the finding stays.
Major 3 — the milestone-prefixed coded shape still resolves the wrong spelling [verified by executing the head code: PRJ-01-02-03 matches feat(PRJ-01-02-03): only, never feat(01-02-03):] After stripping, phasePlanPattern(bare) splits at the first dash (01 / 02-03), the plan half fails the alphanumeric test and the code falls to the literal PRJ-01-02-03 with no optional group. A started plan in a coded milestone-prefixed phase reads as unstarted, the symptom #5271 reports. Fix: when the stripped form falls to the literal branch, emit the literal of bare with the optional code group, and add a row for PRJ-01-02-03.
Minor 1 — acceptance 3 and the --plan PRJ-1-1 form are untested [verified] No coded-id not-started row (e.g. PRJ-01-09 resolving empty at exit 0), and only feat(PRJ-1-1) is tested as a commit subject, never --plan PRJ-1-1 as an id.
Minor 2 — a "matched literally" assertion exercises a different path [verified] The last assertion of the second test (!matches('PRJ-01','feat(01): a'), commented "a code WITHOUT a plan segment is still matched literally") takes the direct branch (phase PRJ, plan 01), so it passes on base and head and does not touch the new code. Replace it with a case that reaches the literal fall-through (which is also Major 3).
Minor 3 — missing test classes [verified] No fast-check property over the transform (RULESET.TESTS.property-based-testing; the file already imports it), e.g. the coded id matches the same subjects as the bare id plus the coded spelling and never another code; no limit-1/limit/limit+1 rows at the 200-character cap with a coded id (RULESET.TESTS.boundary-coverage); no row for the fall-through (bare !== planId with phasePlanPattern(bare) null, e.g. PRJ-01-01-extra) or the code-only PRJ-; nothing kills the obvious mutants (bare !== planId flipped, the ? on the optional group dropped, the slice altered, the if (coded) guard removed) under the Stryker 80% threshold; and the coded commit is only matched with a JavaScript RegExp (test 2), which accepts (?: where git's ERE does not — the divergence the source comment itself cites — so add a feat(PRJ-01-01): commit to the real-git test and assert it appears in the scope.
Minor 4 — the planSubjectPattern doc comment is now false [verified] It says ids that are not numbered are matched literally and can only match a commit naming that exact id; after Major 1 that is untrue for any id of the shape <letters>-<digit…>. Update it with the fix.
Minor 5 — branch name does not follow the convention [verified] fix-5271-coded-plan-subject-scope against <prefix>/NNN-slug (CONTRIBUTING.md branch table, fix/NNN-slug); Validate Branch Name has not run.
Minor 6 — no end-to-end test through check evaluation-scope --plan [verified, Spec axis] Triage lists gsd-core/workflows/execute-plan.md:191 and the other workflow call sites as consumers; the PR tests the module and the regex only, so the CLI exit code for a coded id is unasserted.
Nit 1 [verified] .changeset/eager-jaguars-tumble.md:5 uses a curly apostrophe (plan’s).
Nit 2 [verified] docs/ARCHITECTURE.md:364 and CONTEXT.md:45 (Evaluation Scope Module) still describe <type>(<phase>-<plan>): only; extend them to the coded form (co-change gap, not a gate for Fixed).
Nit 3 [judgment] The grammar is asymmetric: bare 01-01 does not match a feat(PRJ-01-01): commit; state it or test it, since #5177 asks for one grammar accepted by every consumer.
Nit 4 [judgment] coded, direct, bare and codePrefix blur "code" and "stripped"; strippedPattern/literalSplitPattern say what they hold.
Nit 5 [judgment] planSubjectPattern re-derives the code with planId.slice(0, planId.length - bare.length) after stripProjectCodePrefix consumed it, which silently breaks if the stripper ever trims or normalizes; prefer an owner API returning { code, rest } from phase-id.cts, which owns the grammar.
Nit 6 [judgment] The 18-line narrative comment belongs in the PR, not the source, and ^[a-z]+\( / \)!?: repeat three times in the function (subjectOf(inner)).
Considered, not findings: ERE and option injection (the prefix holds only letters, digits, _ and - and is escaped; control characters and whitespace are rejected; the pattern starts with ^[a-z]+\(); no ReDoS (one optional group, linear in git ERE and JavaScript, 200-character cap); (PRJ-)? cannot make the subject paren optional; the direct path for 03-01 is byte-identical to base.
Passes run
| Pass | Result |
|---|---|
/diagnose |
run; executed head and base planSubjectPattern (table above) |
/root-cause-analysis |
run; the first-dash split is addressed for the reported id, but the generic strip widens non-coded ids and the literal fall-through still catches the milestone-prefixed shape |
/qa-test-architect |
run; Minors 1-3, 6 |
/skills-from-the-artificer (bugfix-review) |
run; block below |
/code-review |
run as two parallel sub-agents: Standards (Minor 3, Nits 4-6) and Spec (Majors 1-3, Minors 1, 6) |
/security-review |
run: identification sub-agent found no issue above 80% confidence; cross-plan confusion within the optional group rated 3/10 as exploitable and recorded as the issue's intended design |
| Isolated adversarial pass | run by a fresh agent with no prior context; found Major 3 and Minors 2 and 4 independently |
Not run (Memtrace is unavailable): the Deterministic Review Pass (detect_changes, find_cross_module_issues, find_code_review_issues, find_yaml_rule_matches, review_github_pr), preflight_check, risk tiering (find_central_symbols, find_bridge_symbols), the recorded-decision check (recall_decision, governing_rules) and the co-change gate. None is reported clean. I did not execute any test (local node --test is not permitted).
Laws that apply
Subject: PR #5283 makes planSubjectPattern accept a project-coded plan id by stripping a generic code prefix and adding an optional coded group.
- hyrums-law — fired: callers passing
setup-1-2,AUTH-01-02or a lowercase code now resolve a different commit set (src/gate-evaluation-scope.cts), and the doc comment promising literal matching is false. Action: Major 1, Minor 4. - postels-law — fired: the id grammar is made lenient at the edge using the generic phase-id regex rather than the configured code, so lenient input is normalized to more spellings than the project defines. Action: Major 1.
- leaky-abstractions — fired: the resolver re-derives the code by
sliceover the output ofstripProjectCodePrefix, depending on the stripper's internals, and the git-ERE versus JavaScript(?:difference leaks into the test seam. Action: Nit 5, Minor 3. - goodharts-law — fired: the new tests assert hand-picked JavaScript
RegExpmatches that pass on base for the literal case, so a green run does not show the git behavior. Action: Minors 2, 3.
Considered and cleared: kerckhoffs-principle (no guard or secret mechanism), knuths-optimization-principle (no optimization), kernighans-law (the function is short; the comment mass is a Nit).
CI. Withheld per Blocker 1.
CodeGraph Evidence
Memtrace unavailable: monthly quota exhausted and the server disconnected, so get_impact, get_symbol_context, recall_decision and find_code_review_issues were not run, and the recorded-decision check was not evaluated.
codegraph_impact planSubjectPattern: exported; one resolver caller (resolveBySubject→resolveEvaluationScope); consumersexecute-phase.md,completion-reconciliation.md,execute-plan.md:191per the issue triage (the prior review's figure, not re-run on this index).codegraph_node/codegraph_callers(file mode):src/pattern.cts(escapeEre) andsrc/phase-id.cts(stripProjectCodePrefix, grammar at line 32) read through the main-checkout index; the branch-newphasePlanPatternis absent from it, so the head function was exercised by execution instead.- Finding: the change is local to one resolver, but the resolver feeds the gates that decide whether a plan has started, so a widened match (Major 1) or a missed spelling (Major 3) changes a gate answer.
Verdict
Changes requested. Blocker 1, Major 3, Minor 6, Nit 6.
…n id The generic `stripProjectCodePrefix` grammar (`^[A-Z][A-Z0-9_]*-(?=\d)`, case- insensitive) recognises ANY letter prefix, so ids this project never assigned a code to widened to another plan's commits: `AUTH-01-02` and `setup-1-2` both started matching `feat(01-02):` and `feat(1-2):`. Read `project_code` through the sanctioned `readScopedConfigValue` ladder and strip only that exact value plus a dash, case-sensitively; with no configured code every id keeps the literal match. Also closes the milestone-prefixed coded shape: `PRJ-01-02-03` strips to `01-02-03`, whose plan half still carries a dash, so it fell to the LITERAL `PRJ-01-02-03` — a spelling no executor commit carries, reading a started plan as unstarted. The optional code group now survives the literal fall-through, so that id matches both spellings like every other shape. The changeset states the full consequence and the `project_code` dependency.
|
Addressed at Major 1 — the strip was wider than the project. Fixed at the root, with the rule you named: the resolver now reads
The doc comment that promised literal matching is corrected in the same edit, and I dropped the Major 3 — the milestone-prefixed coded shape. You were right about the mechanism: Major 2 (shared grammar with #5177) — not actioned, and that is a maintainer's call, not mine. You wrote that yourself. I have not touched the workstream-qualified dimension and the PR body says plainly that this lands the project-code half. If you would rather hold the PR until #5177 lands, say so and I will stand down; I would rather not guess. Minor 1 — On the parts I did not take: Minor 3's property/boundary/mutation rows are not added. The transform is now a prefix Minor 6 (body) — updated: the body no longer quotes Nit 1 — curly apostrophe fixed. Nit 2 — I did not touch Blocker 1 is yours — the fork matrix is |
trek-e
left a comment
trek-e
left a comment
There was a problem hiding this comment.
Review of head 8e051e9c4090 — changes requested
Summary. This reviews the push after the 04:37Z review (032aa7e6a107 → 8e051e9c4090, "strip only the CONFIGURED project code from a plan id"); earlier reviews are not edited. The two defects I reproduced on the previous head are fixed: only the configured code is stripped now, and the milestone-prefixed coded shape resolves. The shared-grammar acceptance item is still open, the property and boundary tests are still missing, and the new code adds two Minors. Blocker 1 (maintainer-side CI), Major 1, Minor 5, Nit 6.
Delta checked by execution (head and base src/gate-evaluation-scope.cts, src/phase-id.cts and the config reader transpiled over the built gsd-core/, driving the real planSubjectPattern; probes probe-5283-delta.cjs and p3d.cjs in the delegated pass). The code is read through configuredProjectCode(projectDir) → readScopedConfigValue(projectDir, ['project_code']) (src/planning-workspace.cts:214, never throws; unreadable or unparseable config reads as absent), then must match /^[A-Z][A-Z0-9_]*$/; planSubjectPattern(planId, projectCode = null) takes it as a parameter.
| Plan id | project_code PRJ |
no code |
|---|---|---|
PRJ-01-01 |
PRJ-01-01 + 01-01 (base: PRJ-01-01 only) |
= base |
PRJ-01-02-03 |
01-02-03 + PRJ-01-02-03 (base: coded only) |
= base |
PRJ-01-01-extra |
01-01-extra + PRJ-01-01-extra |
= base |
AUTH-01-02, FOO-01-01, setup-1-2, prj-01-01, PRJ-01, PRJX-01-01, XPRJ-01-01, PR-01-01 |
= base | = base |
PRJ-PRJ-01-01 |
also matches feat(PRJ-01-01) (base: none) |
= base |
End to end through git (resolveEvaluationScope): PRJ finds feat(01-01) for the coded id; a missing, malformed, numeric, empty, array, null or directory config, a lowercase prj, and a hyphenated AUTH-X all stay literal (equal to base), never wider.
Classification and gates. #5271 carries bug + confirmed-bug. Fixes #5271, Fix template, conventional title, not draft. Changeset .changeset/eager-jaguars-tumble.md: type Fixed, pr: 5283, bold lead, states the project_code dependency. No shipped workflow, agent or command changed.
Acceptance, #5271 Agent Brief (verbatim)
- "
--plan PRJ-01-01and--plan 01-01return identical commits in a repo with project codePRJ." — Met for a configuredPRJ. - "A commit subject
feat(PRJ-01-01):matches the same plan." — Met, including a real-git test. - "A plan with no commits still resolves empty at exit 0." — Met (the new test asserts
PRJ-01-09,PRJ-1-1andPRJ-01-02-03resolve empty; status assertedresolvedrather than an exit code). - "Existing zero-padding, decimal, letter-suffixed and literal plan-file ids behave as before." — Met (
AUTH-01-02,setup-1-2,prj-01-01now equal base). - "The grammar change is shared with the workstream-qualified id from #5177." — Not met (Major 1).
Findings
Blocker 1 — the fork matrix has not executed on this head [verified; maintainer-side] The head carries only the policy and scanner checks (SUCCESS); the Tests, Mutation Testing and Install Smoke runs are action_required and the branch is BEHIND next. Not your defect. I am withholding the release: the open items below need a push.
Major 1 — the shared grammar with #5177 is not delivered [judgment, maintainer call; earlier Major 2, open] The issue comment says the change "must share one scope grammar accepting <CODE>-<N>-<M> and the workstream-qualified form"; planSubjectPattern/phasePlanPattern still have no workstream dimension, and there is no shared constant or parity test. Your reply defers it to the maintainer. The finding stays until the item is delivered or restated in #5271 by the maintainer.
Minor 1 — no property test and no coded boundary rows [verified; earlier Minor 3, open] No fast-check property covers the transform (RULESET.TESTS.property-based-testing; the file already imports fc), and only the uncoded ids have the 200/201-character rows (tests ~L308-309). "Would generate ids the grammar rejects" does not exempt it: generate valid codes, then assert that the code-prefixed id matches the same subjects as the bare id plus the coded spelling and that a non-configured prefix never matches the bare spelling.
Minor 2 — an empty remainder widens the pattern [verified, new in this push] planSubjectPattern('PRJ-', 'PRJ') returns ^[a-z]+\((PRJ-)?\)!?:, which matches feat(): (base matched only feat(PRJ-):). The fall-through emits escapeEre(stripped) with stripped = ''; a remainder that is not a valid plan has the same shape (PRJ-a.b yields an optional-group literal a\.b, matching feat(a.b):, another plan's literal id). Require a non-empty stripped id and apply the optional group only when the stripped id is plan-number shaped or phase-prefixed (digit start).
Minor 3 — a lowercase or hyphenated project_code silently stays literal [judgment, new] project_code is stored unvalidated (src/config.cts:914-917 persists any string) and phase-id supports case-insensitive codes, but configuredProjectCode re-validates against /^[A-Z][A-Z0-9_]*$/, so prj or AUTH-X gets no fix and the original #5271 symptom (a started plan reading empty at exit 0) remains with no signal. Either validate and document the supported grammar (changeset and docs) or match the configured value verbatim (escaped) instead of re-validating with a second grammar. The same silent reversion happens when the config is absent or unparseable; that degradation is safe (never wider) but should be stated.
Minor 4 — the branch name does not follow the convention [verified; earlier Minor 5, open] fix-5271-coded-plan-subject-scope against <prefix>/NNN-slug (CONTRIBUTING.md branch table, fix/NNN-slug); Validate Branch Name has not run.
Minor 5 — no end-to-end test through check evaluation-scope --plan [verified; earlier Minor 6, open] The consumers are the workflow call sites (gsd-core/workflows/execute-plan.md:191 and others); the new tests drive resolveEvaluationScope over real git, but the CLI exit code for a coded id is still unasserted.
Nit 1 [verified; earlier Nit 2, open] docs/ARCHITECTURE.md:364 and CONTEXT.md:45 (Evaluation Scope Module) still describe <type>(<phase>-<plan>): only; extend them to the coded form.
Nit 2 [judgment] The grammar /^[A-Z][A-Z0-9_]*$/ appears in both configuredCodePrefix and configuredProjectCode, and the phase-id owner (src/phase-id.cts:30-35) holds the grammar; use one owner constant.
Nit 3 [verified] PRJ-PRJ-01-01 (code PRJ) now also resolves plan PRJ-01-01's commits: the double prefix is stripped once and the remainder matches through the direct branch. Exotic, noted only.
Nit 4 [verified] The tests contain assert.ok(matches(...) === false, ...) (~L372), which is !matches; and no test pins that an invalid configured code (prj, PRJ-X) or an unreadable config stays literal (the tests pass only null).
Nit 5 [verified] .changeset/eager-jaguars-tumble.md has no trailing newline and no (#5271) backlink (the backlink is optional).
Nit 6 [judgment] The earlier naming and comment-mass points (coded/direct/bare/codePrefix, the long narrative comment, the repeated ^[a-z]+\( and \)!?: fragments) are partly addressed and partly declined in your reply; the remaining narrative comment belongs in the PR.
Resolved since the last review. Major 1 (the strip wider than the configured code), Major 3 (PRJ-01-02-03), Minor 1 (coded not-started untested), Minor 2 (the vacuous PRJ-01 assertion), Minor 3's real-git coded test, Minor 4 (the doc comment), Nit 1 (the curly apostrophe), Nit 5 (the prefix is now carried as one value).
Passes run
| Pass | Result |
|---|---|
/diagnose, /root-cause-analysis, /qa-test-architect, /code-review, /security-review, /skills-from-the-artificer |
run on the previous head; re-applied to this delta (security: the configured code is validated to [A-Z0-9_] and escaped; no ERE or option injection; no ReDoS) |
| Isolated delta pass | run by a fresh agent with no prior context: the prior-finding table, the execution above, Minors 2 and 3, Nits 2-4 |
Not run (Memtrace is unavailable): detect_changes, find_cross_module_issues, find_code_review_issues, find_yaml_rule_matches, review_github_pr, preflight_check, find_central_symbols, find_bridge_symbols, recall_decision, governing_rules and the co-change gate. None is reported clean. I did not execute any test (local node --test is not permitted).
Laws that apply
Subject: PR #5283 makes planSubjectPattern accept a project-coded plan id by stripping the configured code and matching an optional coded group.
- postels-law — fired: lenient input is normalized with a second code grammar that disagrees with what
config-setaccepts (Minor 3, Nit 2). Action: one grammar owner. - hyrums-law — fired: callers passing the empty-remainder or double-prefix shapes resolve a different commit set (Minor 2, Nit 3). Action: reject the degenerate remainders.
- goodharts-law — fired: hand-picked matches over a fixed fixture read as coverage while no property or boundary row exercises the transform (Minor 1).
Considered and cleared: kerckhoffs-principle, knuths-optimization-principle, kernighans-law, leaky-abstractions (the slice re-derivation is gone).
CI. Withheld per Blocker 1.
CodeGraph Evidence
Memtrace unavailable: the server failed to connect (monthly quota exhausted earlier), so get_impact, get_symbol_context, recall_decision and find_code_review_issues were not run.
codegraph_impact planSubjectPattern: one resolver caller (resolveBySubject→resolveEvaluationScope), consumed by the gate workflows (the earlier review's figure, not re-run).codegraph_node(file mode):src/planning-workspace.ctsreadScopedConfigValue(:214) andsrc/phase-id.cts(grammar at:30-35).- Finding: the change is local to one resolver, but it feeds the gates that decide whether a plan has started, so a widened or missed match changes a gate answer. The index is the main checkout, so branch-new symbols were exercised by execution.
Verdict
Changes requested. Blocker 1, Major 1, Minor 5, Nit 6.
…teral; one project_code grammar owner Minor 2: the OPTIONAL code group rode on the bare literal fall-through, so 'PRJ-' stripped to an empty remainder and emitted (PRJ-)? alone - matching feat(): - and 'PRJ-a.b' matched feat(a.b):, another plan's literal id. Both are strictly WIDER than the pre-open-gsd#5271 pattern. The group now rides only on a remainder that could be a plan number (phasePlanPattern, or the digit-start milestone-prefixed coded shape 01-02-03, so Major 3 keeps its resolution). Nit 2: /^[A-Z][A-Z0-9_]*$/ is now PROJECT_CODE_VALUE_RE, exported from the phase-id owner that already anchors it as a prefix, and read by both configuredCodePrefix and configuredProjectCode - one grammar, not two that can disagree with what config-set accepts. Minor 3: the supported grammar and the ABSENT-on-unsupported-code degradation are now documented on the reader (never wider, but open-gsd#5271's symptom returns for a project whose code sits outside it). Nit 4: the === false assertion is !matches, and the invalid-code (prj, AUTH-X, empty) and unparseable-config rows are pinned rather than assumed. Minor 1: a fast-check property over generated codes/ids replaces the hand-picked fixture as the evidence for the transform's invariant. Nit 5: changeset newline + (open-gsd#5271) backlink.
|
Addressed at Minor 2 — fixed, and it was a real widening. Nit 2 — one grammar owner. Minor 3 — documented, and I took the "validate and document" branch rather than match verbatim. Minor 1 — a property, not a fixture. Nit 4 — fixed and extended. Nit 5 — changeset newline added, Verification on the pushed head: Not delivered, and why.
|
trek-e
left a comment
trek-e
left a comment
There was a problem hiding this comment.
Review of head 2451a71b73bc — changes requested
Summary. The 13:30Z push closes the property test, the empty-remainder widening, the single grammar owner (PROJECT_CODE_VALUE_RE), and the Nit 4/5 items. The fix remains root-cause and narrow (configured code only, optional ERE group, escaped, bounded id). Remaining: CI never executed, the #5177 acceptance item, and a docs gate.
Classification. #5271 bug + confirmed-bug; Fix template, Fixes #5271, conventional title, changeset present (Fixed, pr: 5283). No shipped workflow/agent/command changed.
Merge state. BEHIND next by 42; git merge-tree is clean, so a plain rebase suffices.
Findings
Blocker 1 — fork matrix not executed. Only the seven policy/scanner checks ran on this head; Tests, Mutation Testing and Install Smoke are action_required (maintainer-side). Approval is withheld until the matrix runs on the final head.
Major 1 — #5271 criterion 5 (grammar shared with the workstream-qualified id of #5177) is not delivered. Maintainer ruling on your question: it is in the issue, so it must be done in this PR. Deliver the shared constant and a parity test. There is no waiver, and the verbatim-match arm is not a substitute.
Minor 1 — false doc claim. src/gate-evaluation-scope.cts configuredProjectCode comment says the supported grammar "is stated in .changeset/eager-jaguars-tumble.md and docs/ARCHITECTURE.md"; neither states it (no docs file is in the diff). State ^[A-Z][A-Z0-9_]*$ and that other project_code values keep the literal behaviour, or drop the sentence.
Minor 2 — docs gate. Extend docs/ARCHITECTURE.md:364 and the CONTEXT.md:44 Evaluation Scope Module entry to the coded form in this PR.
Minor 3 — no 199/200/201 rows for a coded id in tests/gate-evaluation-scope.test.cjs (only uncoded 201 at L308).
Minor 4 — no CLI-level test of check evaluation-scope --plan PRJ-01-01 (exit code + JSON).
Nit 1. Review-round labels ("Major 3", "Minor 2") and ~25 narrative lines live in planSubjectPattern's body; keep the why in one short comment and the history in the PR.
Nit 2. PRJ-PRJ-01-01 with code PRJ resolves plan PRJ-01-01's commits; noted only.
Not run: Memtrace evidence (server unavailable); no tests executed locally.
Verdict
Changes requested. Blocker 1 (CI withheld until the Major and Minors are fixed), Major 1, Minor 4, Nit 2.
Memtrace Evidence
The Memtrace MCP server was unavailable for this review (it failed to connect: memtrace not found in PATH), so no graph-backed check ran. None of the four checks below is a clean result.
get_impact: not run. Blast radius comes from reading the diff and grepping callers.get_symbol_context: not run. Callers and callees of the changed symbols were checked by grep.recall_decision: not run. The recorded-decision gate did not execute, so this does not show that no decision governs the change. ADRs andCONTEXT.mdpredicates were checked by grep instead.find_code_review_issues: not run. The deterministic review pass did not run. A manual diff review stood in for it, so the findings above are judgment, not graph facts.
… length-bound rows Major 1: `PROJECT_CODE_VALUE_SOURCE` and `optionalProjectCodeGroupSource` are exported from the phase-id owner, and the evaluation-scope reader now builds its `(PRJ-)?` group from that owner instead of its own copy, so this reader, a open-gsd#5177 workstream-qualified `<CODE>-<phase>-<plan>` id and `config-set`'s acceptance cannot disagree. The group needs no metacharacter escaping: `PROJECT_CODE_VALUE_RE` admits only `A-Z`, `0-9` and `_`. tests/phase-id-project-code-grammar.test.cjs pins the ERE source and the matcher to the same set (fixed rows + a fast-check property), and pins that the group is a REAL capture group -- POSIX ERE has no `(?:...)`, so the non-capturing spelling is a git-side syntax error the JS RegExp accepts. Minor 1: `configuredProjectCode` no longer claims the supported grammar is stated in the changeset and ARCHITECTURE.md -- neither stated it. It now states `^[A-Z][A-Z0-9_]*$` and that other values keep literal behaviour. Minor 2: docs/ARCHITECTURE.md and the CONTEXT.md Evaluation Scope entry carry the coded form. Minor 3: the 199/200/201 length boundary now has a coded-id row too -- the configured prefix does not buy back the characters it adds. Nit 1: the review-round labels and ~25 narrative lines are out of `planSubjectPattern`'s body; the two load-bearing properties stay, with the history in the PR.
|
Addressed at Major 1 — the shared constant, and a parity test.
Two properties of that group are asserted rather than narrated: it is a REAL capture group (POSIX ERE has no Minor 1. You were right that the doc claim was false — neither file stated the grammar. Minor 2. Minor 3. The length bound is on the whole id, so the coded rows now sit at the same boundary: Nit 1. The review-round labels and ~25 narrative lines are out of the body. What stays is the two load-bearing properties in three lines each — the capture-group form and the Blocker 1 is yours to run; Minor 4 is not done. A CLI-level |
… and JSON Minor 4. The four gates that read this resolver are shell callers branching on `gsd_run check evaluation-scope`, so the exit status and the stdout JSON are part of the contract, not an implementation detail of the resolver. Three cases through the REAL `gsd-tools.cjs` in a child process: - `PRJ-01-01` resolves the plan's commits and exits 0; - `PRJ-01-09` is an empty-but-resolved scope: exit 0, `reason: no-matching-commits`, never a silent pass -- this is the open-gsd#5271 symptom as the gates actually observe it, and the reason has to survive to stdout; - an out-of-grammar configured code (`prj`) leaves the coded id literal, so the plan does NOT resolve, pinning the documented degradation at the CLI rather than only at the unit level. The parity tests from the previous commit move into `tests/phase-id.test.cjs` rather than a third phase-id file: `scripts/lint-test-file-count.cjs` caps the module at 2 and the repo prefers consolidation over a new file.
|
Minor 4 is in after all —
So all six of the code/doc findings are delivered. Blocker 1 is still the matrix, which is maintainer-side. Local on |
This branch has not been deployed
3 participants
Fix PR
Linked Issue
Fixes #5271
What was broken
In any project with
project_codeset,check evaluation-scope --plan "PRJ-01-01"resolved to zero commits at exit 0 —status: resolved,reason: "no-matching-commits". That is the correct answer for a plan that has not started, so the #5170 fail-closed arm (which fires only on a non-zero exit) could not catch it. Every gate that builds its plan id frominit.execute-phase's project-codedphase_numberread a started plan as unstarted;safe_resume_gatethen allowed a fresh executor over a half-executed plan.What this fix does
planSubjectPatternsplits a plan id at its first dash, soPRJ-01-01yields phasePRJ/ plan01-01; the plan half fails the alphanumeric test and the id falls through to the LITERAL pattern, which no executor commit can ever match. The split stays the first attempt (a bare03-01is byte-identical before and after); only when it fails does the id retry with the project's configuredproject_codestripped, making the code an optional group so the id matches bothfeat(PRJ-01-01):andfeat(01-01):(the executor writes the bare token —init.plan-phasehands outpadded_phase: "01").The code is read through the sanctioned
readScopedConfigValueladder (.planning/config.json→project_code). Only that exact value is stripped, case-sensitively, so an id that merely looks coded (AUTH-01-02,setup-1-2,prj-01-01) stays a literal match and can never resolve another plan's commits.Root cause
init.plan-phasereturnspadded_phase: "01"andinit.execute-phasereturnsphase_number: "PRJ-01", so one plan has two spellings.execute-phase.mdbuilds the gate's id from the latter (--plan "${PHASE_NUMBER}-${plan_padded}"→PRJ-01-01), and the plan-subject grammar only understood the former.Testing
How I verified the fix
node --test tests/gate-evaluation-scope.test.cjs tests/safe-resume-gate-anchoring.test.cjs tests/execute-phase-decimal-arithmetic.test.cjs tests/phase-id.test.cjs tests/evaluation-scope-drift-lint.test.cjs— 240 pass / 0 fail.tests/gate-*.test.cjs tests/evaluation-*.test.cjs tests/phase-status.test.cjs) — 642 pass / 0 fail.RegExp: a fixture commitsfeat(PRJ-01-01):and the resolver is asserted to find it through realgit log --extended-regexp. That is what caught the first attempt's(?:PRJ-)— POSIX ERE has no non-capturing group, so it is a fatal pattern-compile error undergit logwhile the JS-side subject filter stays green. There is now a dedicated regression test for it.npm run build:libclean;npx eslint src/gate-evaluation-scope.cts tests/gate-evaluation-scope.test.cjs --max-warnings 0clean.Regression test added?
tests/gate-evaluation-scope.test.cjs, inside the existingresolveEvaluationScope — plan and quick unitssuite (no new file):--plan PRJ-01-01returns the plan's own two commits (notno-matching-commits) and resolves the same shas as the bare01-01;01-010boundary is unchanged, an underscore-bearing code (PROJ_V2-03-01) works, a code with no plan segment (PRJ-01) stays literal, and with no configured code the coded id stays literal;AUTH-01-02,setup-1-2andprj-01-01each match only their own commit and never another plan's bare/padded commits, and the code match is case-sensitive;PRJ-01-02-03(the Major 3 fix): bothfeat(PRJ-01-02-03):andfeat(01-02-03):match, a different sub-phase does not, and the emitted pattern is pinned so the optional code group cannot be silently dropped again;git log, not just the JSRegExp;PRJ-01-09,PRJ-1-1,PRJ-01-02-03) resolve empty at exit 0.Platforms tested
git logplumbing, identical on every platform)Runtimes tested
src/gate-evaluation-scope.ctsis compiled into the sharedgsd-toolssurface every runtime calls)Checklist
Fixes #5271— PR will be auto-closed if missingconfirmed-buglabel.changeset/fragment added (eager-jaguars-tumble.md, typeFixed)Breaking changes
None. The emitted pattern is byte-identical for every plan id that already parsed (a bare
03-01never reaches the new branch). The only ids whose pattern changes are those that previously fell through to the literal branch, and they change from "matches nothing / matches exactly one spelling" to "matches this plan in either spelling" — and only when the project configures thatproject_code.