fix(ce-plan): carry registration obligations into the unit that triggers them - #845
Conversation
…ers them Some directories require a new file to be listed somewhere else — a codemap, a module table, a manifest, a generated artifact. A plan that names the new file and its test looks complete while omitting the registration entirely, and the obligation then surfaces when a gate fails, or never. Planning already reads the repository's contributor guidance, so the information is in hand at the point units are defined. This states that the unit creating the file should also name the surfaces that must list it, and the command that regenerates them. The emphasis is on unenforced obligations. A gated one announces itself eventually; an ungated one is only ever caught by a reader who already knew.
fro-bot
left a comment
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
This is a 2-line documentation-only addition to skills/ce-plan/SKILL.md §3.5, instructing planners to name registration surfaces (codemap, module table, manifest, generated artifact) and their regeneration command in the unit that creates a file requiring them. No TypeScript, no plugin API surface, no tool/hook changes — so most of the type-safety/ESM/error-handling review criteria don't apply here.
Verified:
- The claim "Phase 1.1 already collects this from the repository's own contributor guidance" checks out — §1.1 (line 236) explicitly collects "AGENTS.md guidance that materially affects the plan," so the new paragraph correctly reuses information already gathered rather than adding new research burden.
- The addition is deliberately generic (manifest/index/codemap/module table/generated artifact) with no repo-specific paths, consistent with
ce:planbeing a bundled, cross-repo skill — matches the PR body's stated intent. bun run registry:driftpasses (registry.jsonc up to date) — this SKILL.md body edit didn't trip the very registration-obligation gate the change is about, since it's prose in the body, not a frontmatterdescriptionchange.bun run typecheckandbun run lintare clean of new issues (pre-existing warnings in unrelated files only).bun test tests/unit/transform-content.test.ts tests/unit/config-handler.test.ts(the two suites that reference ce-plan's SKILL.md / registration-related content) pass: 85/85.- Placement is correct — sits right beside the existing sibling rule ("Every feature-bearing unit should include the test file path in
**Files:**") in §3.5, same voice and register as the rest of the section.
Blocking issues
None
Non-blocking concerns
- Minor internal inconsistency in the new sentence: it writes
`**Files:**`(colon inside backticks) but`**Verification**`(no colon), while the canonical field names defined in the bullet list above (§3.5) are**Files**and**Verification**without colons. Not worth blocking on — the pre-existing sentence directly above already uses the**Files:**w/-colon form, so this new text is at least consistent with local precedent, just not with the field-list source of truth. Optional cleanup only. - This is a prompt/instruction change for an LLM-driven planning skill, not enforced by any test or gate — its effectiveness (whether planners actually reference it) can't be verified mechanically, only observed over subsequent plan outputs. That's inherent to this class of change (as the PR body itself acknowledges: "an obligation no gate enforces is the one most likely to be missed"), not a defect in this PR.
Missing tests
None expected — this is a prose addition to a Markdown skill instruction file, not executable logic. No existing test asserts on this specific paragraph's presence/wording, and adding one would be low-value (asserting on exact skill prose is brittle and not this repo's convention for skill content).
Risk assessment: LOW
Single-file, additive, 2-line documentation change to bundled skill content. No code paths, no plugin hooks, no config schema, no runtime behavior affected. Blast radius is limited to future ce:plan runs, which may now produce slightly longer unit definitions when registration obligations apply — a strictly beneficial, non-breaking behavioral nudge. Verified via typecheck/lint/tests/registry-drift with no regressions introduced.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | marcusrbrown/systematic |
| Run ID | 32649248031 |
| Cache | hit |
| Session | ses_fd0b8590dffeCinMP5XqOsHmYw |
fro-bot
left a comment
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
Re-review after 401c616 ("Merge branch 'main' into fix/plan-registration-obligations"). Confirmed this merge commit is a no-op for the PR's content: I diffed current main against the PR head and the substantive change is unchanged — still exactly the same 2-line addition to skills/ce-plan/SKILL.md §3.5 that I reviewed and approved in the prior pass. No new files, no conflict-resolution edits, no additional commits touching PR content.
Standing verification from the prior review still holds and was not invalidated by the merge:
- The added paragraph correctly reflects that Phase 1.1 (§1.1, line 236) already collects "AGENTS.md guidance that materially affects the plan."
bun run registry:drift,bun run typecheck, andbun run lintwere clean.- The two test suites referencing ce-plan's SKILL.md content (
tests/unit/transform-content.test.ts,tests/unit/config-handler.test.ts) passed 85/85. - Placement and phrasing are consistent with the surrounding §3.5 rule it sits beside.
No new blocking issues introduced by the merge commit.
Blocking issues
None
Non-blocking concerns
- (Carried from prior review, unchanged) Minor colon-usage inconsistency between
`**Files:**`and`**Verification**`in the new sentence — consistent with pre-existing local text, not worth blocking on.
Missing tests
None expected — prose addition to a Markdown skill instruction file, not executable logic.
Risk assessment: LOW
Unchanged from the prior review: single-file, additive, 2-line documentation change to bundled skill content, no code paths or plugin hooks affected. The merge-from-main commit adds no new risk to this PR's diff.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | marcusrbrown/systematic |
| Run ID | 32650486633 |
| Cache | hit |
| Session | ses_fd0b8590dffeCinMP5XqOsHmYw |
2 participants
The planning half of #833. The enforcement half is #844.
A plan unit lists the file it creates and the test that covers it, and looks complete. If that directory requires the file to be registered somewhere else — a codemap, a module table, a manifest, a generated artifact — nothing in the plan says so. The obligation surfaces when a gate fails mid-implementation, or it does not surface at all.
That is what happened in #830. The unit named the module, its test, and its fixtures. The codemap entry was missing and the gate caught it after the module was written. The module-table row was missing and a ten-persona review did not catch it, including the persona whose job is auditing against project standards.
The change
One paragraph in section 3.5, beside the existing rule that feature-bearing units name their test file. A unit that creates a file in a directory with registration obligations names those surfaces in its own
**Files:**and the regeneration command in its**Verification**.Phase 1.1 already collects contributor guidance from the repository, so this asks the planner to carry information it has rather than to go find more.
Deliberately generic
No repository-specific paths.
ce:planis bundled and runs against any codebase, so it names the shape — manifest, index, codemap, module table, generated artifact — rather than this repository's instances of it.The paragraph closes on the case that matters most: an obligation no gate enforces is the one most likely to be missed, and is worth stating even when nothing will fail.