chore: roadmap planning — 6 spec closures, 3 new roadmap specs, FSM short-circuit#33
Open
darko-mijic wants to merge 3 commits intomainfrom
Open
chore: roadmap planning — 6 spec closures, 3 new roadmap specs, FSM short-circuit#33darko-mijic wants to merge 3 commits intomainfrom
darko-mijic wants to merge 3 commits intomainfrom
Conversation
… files The decider previously only bypassed FSM checks for new files with unlock-reason, blocking legitimate retroactive completions of existing specs. This removes the isNewFile constraint, making unlock-reason a universal FSM escape hatch for both new files and existing files. Adds test scenario verifying existing file with unlock-reason bypasses FSM check for roadmap → completed transition.
…hort-circuit Spec closures (retroactive completions): - ArchitectureDiagramAdvanced, ArchitectureDiagramCore → completed - DocsConsolidationStrategy → completed (all 16 deliverables) - StepLintExtendedRules → completed (all 6 deliverables) - KebabCaseSlugs, RichContentHelpersTesting → completed - DataAPIPlatformIntegration → completed (split into dedicated specs) New roadmap specs: - SetupCommand (Phase 45) — interactive project initialization - MCPServerIntegration (Phase 46) — MCP server for Claude Code - MonorepoSupport (Phase 100) — cross-package queries (deferred) Other changes: - TraceabilityGenerator spec rewritten for codec architecture - FSM short-circuit in process-api CLI (~2x faster for static queries) - lint-steps help text updated (8 → 12 rules) - INDEX.md broken link fixed (PUBLISHING.md → MAINTAINERS.md) - Deleted obsolete _claude-md/workflow/session-workflows.md
📝 WalkthroughWalkthroughThe PR marks multiple delivery process specifications as completed with unlock reasons, introduces new BDD specifications for MCP server integration, monorepo support, and setup command features, implements FSM short-circuiting in the CLI for static queries, adjusts process guard validation to bypass FSM checks when unlock reasons are present, and reorganizes documentation references from PUBLISHING to MAINTAINERS. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@delivery-process/specs/monorepo-support.feature`:
- Around line 69-90: Add an explicit edge-case scenario exercising overlapping
package globs: create a new Scenario (e.g., "Package field selects first
matching glob when globs overlap") that uses a multi-package config with two
overlapping globs (for example "packages/*" and "packages/platform-*") where the
config ordering is explicit, provide a source file path that matches both globs
(e.g., "packages/platform-core/src/events.ts"), scan and extract the file, and
assert the resulting ExtractedPattern.package equals the package from the
first-listed glob; name the scenario to complement the existing "Package field
is set from matching glob" and "Package field is undefined without packages
config" scenarios so behavior is unambiguous.
- Around line 41-58: The spec mixes config types by saying "packages without
their own `features` or `stubs` inherit from top-level `sources`;" update the
text and acceptance criteria to be explicit: state that PackageConfig entries
inherit top-level `sources` only for TypeScript/source globs (used to build the
total source set), and separately state whether `features` and `stubs` are
inherited from top-level (e.g., "packages without their own `features` or
`stubs` inherit top-level `features`/`stubs`" or "packages do not inherit
top-level `features`/`stubs` unless explicitly set"); then adjust the Scenario
"Multi-package config is parsed and validated" to assert the exact inheritance
behavior for `sources`, `features`, and `stubs` so the resolver behavior and
PackageConfig schema are unambiguous.
In `@delivery-process/specs/setup-command.feature`:
- Around line 80-81: Define the --yes behavior unambiguously by updating the
feature text and scenarios so that "--yes" explicitly skips all prompts and
defaults to overwriting existing files (i.e., resolves the
overwrite-confirmation conflict); change the sentence to "The --yes flag skips
all prompts and defaults to overwriting existing files" and update the scenarios
referencing overwrite-confirmation (the examples around lines noted in the
comment) to expect silent overwrite when --yes is present, and if you need the
opposite behavior add a separate flag (e.g., --no-overwrite or
--abort-on-conflict) with its own scenario instead of leaving --yes ambiguous.
- Around line 49-50: Update the init command invariant to detect
delivery-process.config.js as well as delivery-process.config.ts so the init
flow will prompt instead of writing a new config; specifically, modify wherever
the spec or safety-checks reference delivery-process.config.ts (the init command
and related checks mentioned in the comment) to include
delivery-process.config.js as an acceptable existing contract and update the
three other occurrences referenced (lines ~111-113 and ~132-136) to perform the
same check for .js as well as .ts.
- Around line 142-145: The spec currently makes the init command set
package.json's "type" to "module" by default; change it to preserve existing
package.json.module type unless the user explicitly opts into an ESM migration:
update the setup-command.feature scenario for the init command to remove the
default mutation of the "type" field and instead add an explicit prompt/flag for
ESM migration, ensure the acceptance criteria no longer require "type": "module"
to be present (remove that check from the scenario), and mention the recommended
flow using delivery-process.config.js + tsx wrappers as the default path.
In `@src/cli/lint-steps.ts`:
- Line 135: Update the CLI help text for the "keyword-in-description" lint rule
in lint-steps.ts so it matches the implemented checker: mention that description
lines starting with Given, When, Then, And, or But are flagged (the behavior is
implemented in src/lint/steps/feature-checks.ts). Locate the help/description
string for the "keyword-in-description" rule in lint-steps.ts and expand it to
list "And" and "But" alongside "Given/When/Then" so the CLI documentation
reflects the actual rule.
In `@src/cli/process-api.ts`:
- Around line 512-599: The short-circuit path (tryFsmShortCircuit) calls
fsmIsValidTransition, fsmGetValidTransitionsFrom and fsmGetProtectionSummary
without verifying the given status strings, which leads to runtime crashes or
invalid returns; fix by checking that the provided status keys exist in the FSM
valid-status map (VALID_TRANSITIONS) before calling those helpers and throw a
QueryApiError('INVALID_ARGUMENT', '<usage>') when unknown. Specifically: in
tryFsmShortCircuit before calling fsmIsValidTransition (case
'isValidTransition') verify VALID_TRANSITIONS[from] and VALID_TRANSITIONS[to];
before returning fsmGetValidTransitionsFrom (case 'getValidTransitionsFrom')
validate VALID_TRANSITIONS[status]; and before calling fsmGetProtectionSummary
(case 'getProtectionInfo') validate VALID_TRANSITIONS[status]; keep the existing
usage strings and reuse fsmValidateTransition for the 'checkTransition' case
which already validates.
In `@src/lint/process-guard/decider.ts`:
- Around line 301-303: The unconditional skip on transition.hasUnlockReason in
decider.ts is too broad; instead of always continuing when
transition.hasUnlockReason === true, gate the bypass to only the
retroactive-completion path and only when the unlock reason has been validated
by derive-state.ts (the non-placeholder / length check done after
detect-changes.ts). Update the logic in the function that evaluates transitions
(referencing transition.hasUnlockReason and checkProtectionLevel()) to: 1)
consult the derived validation flag produced in derive-state.ts (e.g., a field
like transition.unlockReasonValidated or fileState.unlockReasonValid) rather
than the raw presence bit from detect-changes.ts, and 2) only allow the FSM
bypass when the transition target is the retroactive completion route to
completed (not for arbitrary transitions). Leave checkProtectionLevel() behavior
unchanged for terminal exemptions. This ensures only validated unlock reasons on
the retro-complete path skip the FSM check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 784699b8-b6a4-438d-a296-d81621a3f2dc
⛔ Files ignored due to path filters (1)
_claude-md/workflow/session-workflows.mdis excluded by none and included by none
📒 Files selected for processing (21)
delivery-process/specs/architecture-diagram-advanced.featuredelivery-process/specs/architecture-diagram-core.featuredelivery-process/specs/data-api-cli-ergonomics.featuredelivery-process/specs/data-api-platform-integration.featuredelivery-process/specs/docs-consolidation-strategy.featuredelivery-process/specs/mcp-server-integration.featuredelivery-process/specs/monorepo-support.featuredelivery-process/specs/setup-command.featuredelivery-process/specs/step-lint-extended-rules.featuredelivery-process/specs/traceability-enhancements.featuredelivery-process/specs/traceability-generator.featuredocs/INDEX.mdsrc/cli/lint-steps.tssrc/cli/process-api.tssrc/lint/process-guard/decider.tstests/features/behavior/kebab-case-slugs.featuretests/features/behavior/rich-content-helpers.featuretests/features/validation/process-guard.featuretests/steps/behavior/kebab-case-slugs.steps.tstests/steps/behavior/rich-content-helpers.steps.tstests/steps/validation/process-guard.steps.ts
Comment on lines
+41
to
+58
| **Invariant:** When a packages field is present in the config, each entry maps | ||
| a package name to its source globs. The top-level sources field becomes optional. | ||
| Packages without their own features or stubs inherit from top-level sources. | ||
| Repos without packages work exactly as before (backward compatible). | ||
|
|
||
| **Rationale:** The consumer monorepo has no config file because the system only | ||
| supports flat glob arrays. Adding packages enables a single config file to | ||
| replace duplicated globs across 15+ scripts. | ||
|
|
||
| **Verified by:** Multi-package config parsing, | ||
| Single-package backward compatibility | ||
|
|
||
| @acceptance-criteria @happy-path | ||
| Scenario: Multi-package config is parsed and validated | ||
| Given a config file with two package entries | ||
| When the config is loaded and resolved | ||
| Then each package has resolved TypeScript and feature globs | ||
| And the total source set is the union of all package globs |
There was a problem hiding this comment.
Clarify which globs package entries inherit.
Line 43 says packages without their own features or stubs inherit from top-level sources. That mixes different config types and makes the intended resolver behavior unclear for the PackageConfig schema and its acceptance criteria.
Proposed spec clarification
- Packages without their own features or stubs inherit from top-level sources.
+ Packages without their own feature or stub globs inherit those values from the
+ corresponding top-level feature and stub settings.+ `@acceptance-criteria` `@edge-case`
+ Scenario: Package entries inherit top-level feature and stub globs
+ Given a config file with packages but no package-specific feature or stub globs
+ When the config is loaded and resolved
+ Then each package inherits the top-level feature and stub globsAs per coding guidelines, delivery-process/**/*.feature: "Ensure scenarios define clear acceptance criteria."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@delivery-process/specs/monorepo-support.feature` around lines 41 - 58, The
spec mixes config types by saying "packages without their own `features` or
`stubs` inherit from top-level `sources`;" update the text and acceptance
criteria to be explicit: state that PackageConfig entries inherit top-level
`sources` only for TypeScript/source globs (used to build the total source set),
and separately state whether `features` and `stubs` are inherited from top-level
(e.g., "packages without their own `features` or `stubs` inherit top-level
`features`/`stubs`" or "packages do not inherit top-level `features`/`stubs`
unless explicitly set"); then adjust the Scenario "Multi-package config is
parsed and validated" to assert the exact inheritance behavior for `sources`,
`features`, and `stubs` so the resolver behavior and PackageConfig schema are
unambiguous.
Comment on lines
+69
to
+90
| **Invariant:** When packages config is active, every ExtractedPattern has an | ||
| optional package field set from the matching glob. If no packages config exists, | ||
| the field is undefined. First match wins on overlapping globs. | ||
|
|
||
| **Rationale:** Package provenance must be derived automatically from config, | ||
| not from manual annotation. This ensures zero additional developer effort. | ||
|
|
||
| **Verified by:** Package derived from glob match, | ||
| No package when config lacks packages field | ||
|
|
||
| @acceptance-criteria @happy-path | ||
| Scenario: Package field is set from matching glob | ||
| Given a multi-package config with "platform-core" and "platform-bc" | ||
| And a source file at "packages/platform-core/src/events.ts" | ||
| When the file is scanned and extracted | ||
| Then the resulting pattern has package "platform-core" | ||
|
|
||
| @acceptance-criteria @edge-case | ||
| Scenario: Package field is undefined without packages config | ||
| Given a single-package config with no packages field | ||
| When a source file is scanned | ||
| Then the resulting pattern has no package field |
There was a problem hiding this comment.
Add a scenario for overlapping package globs.
Lines 69-71 define First match wins on overlapping globs, but the scenarios only cover single-match and no-packages cases. Without an explicit edge-case scenario, different implementations can resolve the same file to different packages.
Proposed acceptance scenario
+ `@acceptance-criteria` `@edge-case`
+ Scenario: First matching package wins when globs overlap
+ Given a multi-package config where "platform-core" and "platform-shared" both match the same source file
+ And "platform-core" is defined before "platform-shared"
+ When the file is scanned and extracted
+ Then the resulting pattern has package "platform-core"As per coding guidelines, delivery-process/**/*.feature: "Ensure scenarios define clear acceptance criteria."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **Invariant:** When packages config is active, every ExtractedPattern has an | |
| optional package field set from the matching glob. If no packages config exists, | |
| the field is undefined. First match wins on overlapping globs. | |
| **Rationale:** Package provenance must be derived automatically from config, | |
| not from manual annotation. This ensures zero additional developer effort. | |
| **Verified by:** Package derived from glob match, | |
| No package when config lacks packages field | |
| @acceptance-criteria @happy-path | |
| Scenario: Package field is set from matching glob | |
| Given a multi-package config with "platform-core" and "platform-bc" | |
| And a source file at "packages/platform-core/src/events.ts" | |
| When the file is scanned and extracted | |
| Then the resulting pattern has package "platform-core" | |
| @acceptance-criteria @edge-case | |
| Scenario: Package field is undefined without packages config | |
| Given a single-package config with no packages field | |
| When a source file is scanned | |
| Then the resulting pattern has no package field | |
| **Invariant:** When packages config is active, every ExtractedPattern has an | |
| optional package field set from the matching glob. If no packages config exists, | |
| the field is undefined. First match wins on overlapping globs. | |
| **Rationale:** Package provenance must be derived automatically from config, | |
| not from manual annotation. This ensures zero additional developer effort. | |
| **Verified by:** Package derived from glob match, | |
| No package when config lacks packages field | |
| `@acceptance-criteria` `@happy-path` | |
| Scenario: Package field is set from matching glob | |
| Given a multi-package config with "platform-core" and "platform-bc" | |
| And a source file at "packages/platform-core/src/events.ts" | |
| When the file is scanned and extracted | |
| Then the resulting pattern has package "platform-core" | |
| `@acceptance-criteria` `@edge-case` | |
| Scenario: Package field is undefined without packages config | |
| Given a single-package config with no packages field | |
| When a source file is scanned | |
| Then the resulting pattern has no package field | |
| `@acceptance-criteria` `@edge-case` | |
| Scenario: First matching package wins when globs overlap | |
| Given a multi-package config where "platform-core" and "platform-shared" both match the same source file | |
| And "platform-core" is defined before "platform-shared" | |
| When the file is scanned and extracted | |
| Then the resulting pattern has package "platform-core" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@delivery-process/specs/monorepo-support.feature` around lines 69 - 90, Add an
explicit edge-case scenario exercising overlapping package globs: create a new
Scenario (e.g., "Package field selects first matching glob when globs overlap")
that uses a multi-package config with two overlapping globs (for example
"packages/*" and "packages/platform-*") where the config ordering is explicit,
provide a source file path that matches both globs (e.g.,
"packages/platform-core/src/events.ts"), scan and extract the file, and assert
the resulting ExtractedPattern.package equals the package from the first-listed
glob; name the scenario to complement the existing "Package field is set from
matching glob" and "Package field is undefined without packages config"
scenarios so behavior is unambiguous.
Comment on lines
+49
to
+50
| **Invariant:** The init command reads the target directory for package.json, | ||
| tsconfig.json, delivery-process.config.ts, and monorepo markers before prompting |
There was a problem hiding this comment.
Detect existing delivery-process.config.js before writing a new config.
The safety checks only look for delivery-process.config.ts. In a repo already using delivery-process.config.js, init can miss the existing contract and create a second config instead of prompting.
♻️ Suggested spec adjustment
- tsconfig.json, delivery-process.config.ts, and monorepo markers before prompting
+ tsconfig.json, delivery-process.config.ts or delivery-process.config.js, and
+ monorepo markers before prompting
...
- Given a directory with an existing delivery-process.config.ts
+ Given a directory with an existing delivery-process.config.ts or
+ delivery-process.config.jsAlso applies to: 111-113, 132-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@delivery-process/specs/setup-command.feature` around lines 49 - 50, Update
the init command invariant to detect delivery-process.config.js as well as
delivery-process.config.ts so the init flow will prompt instead of writing a new
config; specifically, modify wherever the spec or safety-checks reference
delivery-process.config.ts (the init command and related checks mentioned in the
comment) to include delivery-process.config.js as an acceptable existing
contract and update the three other occurrences referenced (lines ~111-113 and
~132-136) to perform the same check for .js as well as .ts.
Comment on lines
+80
to
+81
| for source glob paths with defaults inferred from project structure. The --yes | ||
| flag skips all prompts and uses defaults. |
There was a problem hiding this comment.
Resolve the --yes vs overwrite-confirmation conflict.
The non-interactive rule says all prompts are skipped, but the overwrite scenario still requires an explicit confirmation. Right now --yes can mean either silent overwrite or silent abort, depending on the implementation.
♻️ Suggested spec adjustment
- flag skips all prompts and uses defaults.
+ flag skips non-destructive selection prompts and uses defaults.
+ Destructive overwrites require an explicit --force flag; otherwise init exits
+ without modifying existing files.
...
+ `@acceptance-criteria` `@validation`
+ Scenario: Non-interactive mode refuses to overwrite existing config
+ Given a directory with an existing delivery-process config file
+ When running the init command with --yes flag
+ Then the command prints a message requiring --force to overwrite existing config
+ And exits with code 1Also applies to: 102-107, 132-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@delivery-process/specs/setup-command.feature` around lines 80 - 81, Define
the --yes behavior unambiguously by updating the feature text and scenarios so
that "--yes" explicitly skips all prompts and defaults to overwriting existing
files (i.e., resolves the overwrite-confirmation conflict); change the sentence
to "The --yes flag skips all prompts and defaults to overwriting existing files"
and update the scenarios referencing overwrite-confirmation (the examples around
lines noted in the comment) to expect silent overwrite when --yes is present,
and if you need the opposite behavior add a separate flag (e.g., --no-overwrite
or --abort-on-conflict) with its own scenario instead of leaving --yes
ambiguous.
Comment on lines
+142
to
+145
| The package.json "type" field is set to "module" if not already present. | ||
|
|
||
| **Rationale:** The tutorial uses long fragile dist paths. Bin commands are the | ||
| stable public API. Setting type:module ensures ESM imports work for the config. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the feature file to see the full context around lines 142-145 and 150-156
cat -n delivery-process/specs/setup-command.feature | sed -n '130,160p'Repository: libar-dev/delivery-process
Length of output: 1697
🏁 Script executed:
# Also check if there are any related config or setup files to understand the broader context
fd -t f "setup" delivery-process/ | head -20Repository: libar-dev/delivery-process
Length of output: 114
🏁 Script executed:
# Search for references to type:module in the codebase to understand current usage
rg -i "type.*module" delivery-process/ -A 2 -B 2Repository: libar-dev/delivery-process
Length of output: 5182
🌐 Web query:
Does setting "type": "module" in package.json change how Node.js interprets .js files
💡 Result:
Yes. In Node.js, setting "type": "module" in a package.json makes all .js files in that package scope (the directory containing that package.json and its subdirectories, unless another closer package.json overrides it) be interpreted as ES modules (so import/export syntax is expected, and require() behaves differently). [1][2]
Overrides still apply:
.mjsis always treated as ESM.cjsis always treated as CommonJS (even inside a"type": "module"package) [1][2]
🏁 Script executed:
# Check how delivery-process currently intends to load config files
rg -i "delivery-process\.config" delivery-process/ -A 3 -B 1 | head -50Repository: libar-dev/delivery-process
Length of output: 5574
🏁 Script executed:
# Look for any existing init command implementation or references
rg "init.*command" delivery-process/specs/ -iRepository: libar-dev/delivery-process
Length of output: 1554
Avoid making type: module a default mutation in the init command.
Setting "type": "module" changes how Node.js interprets all .js files in the package scope—a breaking change for existing CommonJS code. The learnings indicate the recommended architecture uses delivery-process.config.js with tsx-based wrappers, which does not require forcing ESM on the entire package. For a safe-adoption command, either make this an explicit opt-in migration step or remove the requirement and follow the recommended .js config + wrapper pattern.
Update the spec to preserve existing package module settings unless the user explicitly confirms an ESM migration, and remove the "type": "module" requirement from the acceptance criteria at lines 156.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@delivery-process/specs/setup-command.feature` around lines 142 - 145, The
spec currently makes the init command set package.json's "type" to "module" by
default; change it to preserve existing package.json.module type unless the user
explicitly opts into an ESM migration: update the setup-command.feature scenario
for the init command to remove the default mutation of the "type" field and
instead add an explicit prompt/flag for ESM migration, ensure the acceptance
criteria no longer require "type": "module" to be present (remove that check
from the scenario), and mention the recommended flow using
delivery-process.config.js + tsx wrappers as the default path.
src/cli/lint-steps.ts
Outdated
| error missing-and-destructuring And steps but no And destructured | ||
| error missing-rule-wrapper Rule: blocks but no Rule() wrapper | ||
| warning hash-in-step-text Mid-line # in step text silently truncates | ||
| error keyword-in-description Description line starts with Given/When/Then |
There was a problem hiding this comment.
Expand keyword-in-description help text to match the implemented rule.
Line 135 says this only catches Given/When/Then, but the checker also flags description lines starting with And and But (src/lint/steps/feature-checks.ts:240-306). The CLI help is currently narrower than the actual behavior.
📝 Proposed fix
- error keyword-in-description Description line starts with Given/When/Then
+ error keyword-in-description Description line starts with Given/When/Then/And/But📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| error keyword-in-description Description line starts with Given/When/Then | |
| error keyword-in-description Description line starts with Given/When/Then/And/But |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/lint-steps.ts` at line 135, Update the CLI help text for the
"keyword-in-description" lint rule in lint-steps.ts so it matches the
implemented checker: mention that description lines starting with Given, When,
Then, And, or But are flagged (the behavior is implemented in
src/lint/steps/feature-checks.ts). Locate the help/description string for the
"keyword-in-description" rule in lint-steps.ts and expand it to list "And" and
"But" alongside "Given/When/Then" so the CLI documentation reflects the actual
rule.
Comment on lines
+512
to
+599
| const FSM_SHORT_CIRCUIT_METHODS: ReadonlySet<string> = new Set([ | ||
| 'isValidTransition', | ||
| 'checkTransition', | ||
| 'getValidTransitionsFrom', | ||
| 'getProtectionInfo', | ||
| ]); | ||
|
|
||
| /** | ||
| * Attempt to handle an FSM query without building the pipeline. | ||
| * | ||
| * @returns The FSM result data if this is a short-circuit candidate, or | ||
| * `undefined` if the query should go through the normal pipeline path. | ||
| */ | ||
| function tryFsmShortCircuit(subcommand: string, subArgs: readonly string[]): unknown { | ||
| if (subcommand !== 'query') return undefined; | ||
|
|
||
| const methodName = subArgs[0]; | ||
| if (methodName === undefined || !FSM_SHORT_CIRCUIT_METHODS.has(methodName)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const fsmArgs = subArgs.slice(1); | ||
|
|
||
| switch (methodName) { | ||
| case 'isValidTransition': { | ||
| const from = fsmArgs[0]; | ||
| const to = fsmArgs[1]; | ||
| if (from === undefined || to === undefined) { | ||
| throw new QueryApiError( | ||
| 'INVALID_ARGUMENT', | ||
| 'Usage: process-api query isValidTransition <fromStatus> <toStatus>' | ||
| ); | ||
| } | ||
| return fsmIsValidTransition(from as ProcessStatusValue, to as ProcessStatusValue); | ||
| } | ||
|
|
||
| case 'checkTransition': { | ||
| const from = fsmArgs[0]; | ||
| const to = fsmArgs[1]; | ||
| if (from === undefined || to === undefined) { | ||
| throw new QueryApiError( | ||
| 'INVALID_ARGUMENT', | ||
| 'Usage: process-api query checkTransition <fromStatus> <toStatus>' | ||
| ); | ||
| } | ||
| const result = fsmValidateTransition(from, to); | ||
| return { | ||
| from: result.from, | ||
| to: result.to, | ||
| valid: result.valid, | ||
| error: result.error, | ||
| validAlternatives: result.validAlternatives, | ||
| }; | ||
| } | ||
|
|
||
| case 'getValidTransitionsFrom': { | ||
| const status = fsmArgs[0]; | ||
| if (status === undefined) { | ||
| throw new QueryApiError( | ||
| 'INVALID_ARGUMENT', | ||
| 'Usage: process-api query getValidTransitionsFrom <status>' | ||
| ); | ||
| } | ||
| return fsmGetValidTransitionsFrom(status as ProcessStatusValue); | ||
| } | ||
|
|
||
| case 'getProtectionInfo': { | ||
| const status = fsmArgs[0]; | ||
| if (status === undefined) { | ||
| throw new QueryApiError( | ||
| 'INVALID_ARGUMENT', | ||
| 'Usage: process-api query getProtectionInfo <status>' | ||
| ); | ||
| } | ||
| const summary = fsmGetProtectionSummary(status as ProcessStatusValue); | ||
| return { | ||
| status, | ||
| level: summary.level, | ||
| description: summary.description, | ||
| canAddDeliverables: summary.canAddDeliverables, | ||
| requiresUnlock: summary.requiresUnlock, | ||
| }; | ||
| } | ||
|
|
||
| default: | ||
| return undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find ProcessStatusValue definition
rg "ProcessStatusValue" -t ts --max-count=20Repository: libar-dev/delivery-process
Length of output: 8511
🏁 Script executed:
# Check the FSM transitions file to understand status handling
fd "transitions.ts" --type f -x cat -n {}Repository: libar-dev/delivery-process
Length of output: 6141
🏁 Script executed:
# Check for existing status validation patterns
rg "VALID.*STATUS\|ProcessStatus" -t ts -A 2 | head -40Repository: libar-dev/delivery-process
Length of output: 52
🏁 Script executed:
# Check the actual file context around the lines mentioned
cat -n src/cli/process-api.ts | sed -n '500,600p'Repository: libar-dev/delivery-process
Length of output: 3847
🏁 Script executed:
# Check if isValidStatusValue is exported from validation/fsm and accessible
rg "export.*isValidStatusValue" src/validation/fsm/ -A 1
# Check imports in process-api.ts to see what FSM functions are imported
cat -n src/cli/process-api.ts | sed -n '1,40p'Repository: libar-dev/delivery-process
Length of output: 1995
🏁 Script executed:
# Check fsmGetProtectionSummary to understand what happens with invalid input
rg "function fsmGetProtectionSummary|export.*fsmGetProtectionSummary" -A 5 src/Repository: libar-dev/delivery-process
Length of output: 52
🏁 Script executed:
# Check where isValidStatusValue is defined
rg "isValidStatusValue" src/ -B 2 -A 5Repository: libar-dev/delivery-process
Length of output: 2245
🏁 Script executed:
# Check all imports in process-api.ts from validation/fsm
cat -n src/cli/process-api.ts | sed -n '40,100p' | grep -A 20 "import.*fsm\|import.*validation"Repository: libar-dev/delivery-process
Length of output: 1161
🏁 Script executed:
# Look at the full import section of process-api.ts
head -150 src/cli/process-api.ts | tail -100Repository: libar-dev/delivery-process
Length of output: 3021
🏁 Script executed:
# Check what fsmGetProtectionSummary does with invalid input
cat -n src/validation/fsm/validator.ts | grep -A 30 "export function getProtectionSummary"Repository: libar-dev/delivery-process
Length of output: 964
🏁 Script executed:
# Check the entire validator.ts to understand all functions and their validation
wc -l src/validation/fsm/validator.tsRepository: libar-dev/delivery-process
Length of output: 105
🏁 Script executed:
# Get key sections of validator.ts to understand protection summary behavior
sed -n '1,50p' src/validation/fsm/validator.ts && echo "---" && sed -n '100,170p' src/validation/fsm/validator.tsRepository: libar-dev/delivery-process
Length of output: 3727
🏁 Script executed:
# Check getProtectionLevel to see if it's safe with invalid input
rg "export function getProtectionLevel|PROTECTION_LEVELS" src/validation/fsm/states.ts -A 3Repository: libar-dev/delivery-process
Length of output: 832
🏁 Script executed:
# Verify what happens when you index into PROTECTION_LEVELS with invalid key
cat -n src/validation/fsm/states.ts | head -50Repository: libar-dev/delivery-process
Length of output: 2311
Validate FSM status names before calling the short-circuit helpers.
Three of the four FSM short-circuit methods lack runtime validation of CLI input. In src/validation/fsm/transitions.ts, lines 80-98, invalid status values cause failures:
isValidTransition()crashes when indexingVALID_TRANSITIONS[from]returnsundefinedand calls.includes()getValidTransitionsFrom()silently returnsundefinedinstead of a structured errorgetProtectionSummary()returns invalid data whengetProtectionLevel()returnsundefined
Only checkTransition is safe because fsmValidateTransition() validates internally. Parse and reject unknown statuses up front in the short-circuit path so all methods return proper CLI errors.
🛡️ Proposed fix
+const VALID_PROCESS_STATUSES: ReadonlySet<ProcessStatusValue> = new Set([
+ 'roadmap',
+ 'active',
+ 'completed',
+ 'deferred',
+]);
+
+function parseProcessStatus(
+ value: string | undefined,
+ usage: string,
+ label: string
+): ProcessStatusValue {
+ if (value === undefined) {
+ throw new QueryApiError('INVALID_ARGUMENT', usage);
+ }
+ if (!VALID_PROCESS_STATUSES.has(value as ProcessStatusValue)) {
+ throw new QueryApiError(
+ 'INVALID_ARGUMENT',
+ `Unknown ${label}: "${value}". Expected one of: ${[...VALID_PROCESS_STATUSES].join(', ')}`
+ );
+ }
+ return value as ProcessStatusValue;
+}
+
function tryFsmShortCircuit(subcommand: string, subArgs: readonly string[]): unknown {
@@
case 'isValidTransition': {
- const from = fsmArgs[0];
- const to = fsmArgs[1];
- if (from === undefined || to === undefined) {
- throw new QueryApiError(
- 'INVALID_ARGUMENT',
- 'Usage: process-api query isValidTransition <fromStatus> <toStatus>'
- );
- }
- return fsmIsValidTransition(from as ProcessStatusValue, to as ProcessStatusValue);
+ const usage = 'Usage: process-api query isValidTransition <fromStatus> <toStatus>';
+ const from = parseProcessStatus(fsmArgs[0], usage, 'fromStatus');
+ const to = parseProcessStatus(fsmArgs[1], usage, 'toStatus');
+ return fsmIsValidTransition(from, to);
}
@@
case 'getValidTransitionsFrom': {
- const status = fsmArgs[0];
- if (status === undefined) {
- throw new QueryApiError(
- 'INVALID_ARGUMENT',
- 'Usage: process-api query getValidTransitionsFrom <status>'
- );
- }
- return fsmGetValidTransitionsFrom(status as ProcessStatusValue);
+ const status = parseProcessStatus(
+ fsmArgs[0],
+ 'Usage: process-api query getValidTransitionsFrom <status>',
+ 'status'
+ );
+ return fsmGetValidTransitionsFrom(status);
}
case 'getProtectionInfo': {
- const status = fsmArgs[0];
- if (status === undefined) {
- throw new QueryApiError(
- 'INVALID_ARGUMENT',
- 'Usage: process-api query getProtectionInfo <status>'
- );
- }
- const summary = fsmGetProtectionSummary(status as ProcessStatusValue);
+ const status = parseProcessStatus(
+ fsmArgs[0],
+ 'Usage: process-api query getProtectionInfo <status>',
+ 'status'
+ );
+ const summary = fsmGetProtectionSummary(status);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/process-api.ts` around lines 512 - 599, The short-circuit path
(tryFsmShortCircuit) calls fsmIsValidTransition, fsmGetValidTransitionsFrom and
fsmGetProtectionSummary without verifying the given status strings, which leads
to runtime crashes or invalid returns; fix by checking that the provided status
keys exist in the FSM valid-status map (VALID_TRANSITIONS) before calling those
helpers and throw a QueryApiError('INVALID_ARGUMENT', '<usage>') when unknown.
Specifically: in tryFsmShortCircuit before calling fsmIsValidTransition (case
'isValidTransition') verify VALID_TRANSITIONS[from] and VALID_TRANSITIONS[to];
before returning fsmGetValidTransitionsFrom (case 'getValidTransitionsFrom')
validate VALID_TRANSITIONS[status]; and before calling fsmGetProtectionSummary
(case 'getProtectionInfo') validate VALID_TRANSITIONS[status]; keep the existing
usage strings and reuse fsmValidateTransition for the 'checkTransition' case
which already validates.
src/lint/process-guard/decider.ts
Outdated
Comment on lines
301
to
303
| // Files with unlock-reason bypass FSM check (supports retroactive completions and file splits) | ||
| if (transition.hasUnlockReason === true) { | ||
| continue; |
There was a problem hiding this comment.
Don't use raw unlock-reason diff detection as a blanket FSM bypass.
transition.hasUnlockReason is only a presence bit from src/lint/process-guard/detect-changes.ts, Lines 420-425; the 10+ character / non-placeholder validation happens later in src/lint/process-guard/derive-state.ts, Lines 195-227. With the unconditional continue, an added @libar-docs-unlock-reason:todo will still skip FSM validation, and checkProtectionLevel() already exempts transitions that land in a terminal state. That means an existing file can now go roadmap -> completed with an invalid unlock reason. Gate this on the validated file state and scope it to the retroactive-completion path rather than every transition.
🔒 Proposed fix
- // Files with unlock-reason bypass FSM check (supports retroactive completions and file splits)
- if (transition.hasUnlockReason === true) {
+ // Only validated unlock reasons may bypass FSM, and only for retroactive completion flows.
+ const fileState = state.files.get(file);
+ if (
+ transition.to === 'completed' &&
+ transition.hasUnlockReason === true &&
+ fileState?.hasUnlockReason === true
+ ) {
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Files with unlock-reason bypass FSM check (supports retroactive completions and file splits) | |
| if (transition.hasUnlockReason === true) { | |
| continue; | |
| // Only validated unlock reasons may bypass FSM, and only for retroactive completion flows. | |
| const fileState = state.files.get(file); | |
| if ( | |
| transition.to === 'completed' && | |
| transition.hasUnlockReason === true && | |
| fileState?.hasUnlockReason === true | |
| ) { | |
| continue; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lint/process-guard/decider.ts` around lines 301 - 303, The unconditional
skip on transition.hasUnlockReason in decider.ts is too broad; instead of always
continuing when transition.hasUnlockReason === true, gate the bypass to only the
retroactive-completion path and only when the unlock reason has been validated
by derive-state.ts (the non-placeholder / length check done after
detect-changes.ts). Update the logic in the function that evaluates transitions
(referencing transition.hasUnlockReason and checkProtectionLevel()) to: 1)
consult the derived validation flag produced in derive-state.ts (e.g., a field
like transition.unlockReasonValidated or fileState.unlockReasonValid) rather
than the raw presence bit from detect-changes.ts, and 2) only allow the FSM
bypass when the transition target is the retroactive completion route to
completed (not for arbitrary transitions). Leave checkProtectionLevel() behavior
unchanged for terminal exemptions. This ensures only validated unlock reasons on
the retro-complete path skip the FSM check.
… FSM input - Scope unlock-reason FSM bypass to validated reasons targeting completed only (was unconditionally bypassing on raw presence bit from detect-changes) - Add parseProcessStatus() to validate CLI input before FSM short-circuit calls (invalid status values previously caused runtime TypeError crashes) - Expand keyword-in-description help text to include And/But keywords - Clarify monorepo config inheritance wording and add overlapping globs scenario - Detect .config.js alongside .config.ts in setup-command spec - Resolve --yes vs overwrite conflict with --force flag for destructive ops - Make type:module opt-in via --esm flag instead of default mutation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
roadmapstatusSpec Closures
roadmap→completed(all deliverables terminal)roadmap→completed(all deliverables terminal)roadmap→completed(all 16 deliverables terminal)roadmap→completed(all 6 deliverables terminal)roadmap→completed(all 3 deliverables terminal)roadmap→completed(3 deliverables added + terminal)roadmap→completed(split into dedicated specs)All closures use
@libar-docs-unlock-reason:Retroactive-completion(orSplit-into-dedicated-specs).New Roadmap Specs
npx @libar-dev/delivery-process initsetup flowOther Changes
process-api.ts—tryFsmShortCircuit()dispatches directly to FSM modules forisValidTransition,getValidTransitionsFrom,checkTransition,getProtectionInfowithout building the pipelineisNewFileconstraint from unlock-reason bypass incheckStatusTransitions(), enabling retroactive completions of existing filesPUBLISHING.md→MAINTAINERS.md)_claude-md/workflow/session-workflows.mdTest Plan
pnpm typecheck— no type errorspnpm test— 8,104 tests pass (125 files)pnpm lint— no lint errorspnpm validate:all— 46/46 phases pass, 0 anti-patternsSummary by CodeRabbit
New Features
Improvements