refactor(query): manifest-backed routing seam + family adapters#2908
Merged
refactor(query): manifest-backed routing seam + family adapters#2908
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDelegates top-level CLI dispatch for seven command families to newly added thin CJS routers and generated alias artifacts; introduces typed command manifests, a generator and freshness checker, updates SDK registry and normalization to consume generated alias metadata, and adds tests and docs to cover the manifest → artifact → router → registry seam. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as gsd-tools.cjs (CLI)
participant Router as Family Router (CJS)
participant Generated as Command Aliases (generated)
participant Registry as SDK Registry
participant Handler as Command Handler
CLI->>Router: forward family args (e.g., "state validate ...")
Router->>Generated: consult *_SUBCOMMANDS / alias lists
Router->>Handler: invoke cmdXxx with parsed positional/named args
CLI->>Registry: (alternate flow) normalized dotted command via normalizeQueryCommand
Registry->>Generated: resolve canonical / aliases
Registry->>Handler: invoke registered handler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. Review rate limit: 6/8 reviews remaining, refill in 10 minutes and 56 seconds.Comment |
trek-e
added
type: chore
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
get-shit-done/bin/lib/init-command-router.cjs (1)
62-62: ⚡ Quick winDerive unknown-workflow “Available” list from generated metadata.
Line 62 hardcodes workflow names; this can diverge from the manifest-backed command seam.
Proposed refactor
'use strict'; +const { INIT_SUBCOMMANDS } = require('./command-aliases.generated.cjs'); @@ default: - error(`Unknown init workflow: ${workflow}\nAvailable: execute-phase, plan-phase, new-project, new-milestone, quick, ingest-docs, resume, verify-work, phase-op, todos, milestone-op, map-codebase, progress, manager, new-workspace, list-workspaces, remove-workspace`); + error(`Unknown init workflow: ${workflow}\nAvailable: ${Array.from(INIT_SUBCOMMANDS).join(', ')}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@get-shit-done/bin/lib/init-command-router.cjs` at line 62, The error message hardcodes the workflow names; instead import/require the generated manifest/metadata used by the command seam (the same module that exposes available workflows), compute the available list dynamically (e.g., derive keys or names from that manifest), and replace the hardcoded list in the error(`Unknown init workflow: ${workflow} ...`) call with the generated list so the message always reflects the manifest-backed commands; ensure you use the same symbol/source the router uses for routing to avoid divergence and format the list consistently for the error output.get-shit-done/bin/lib/phases-command-router.cjs (1)
24-24: ⚡ Quick winUse generated subcommand metadata for unknown-subcommand messaging.
Line 24 hardcodes available values, which can drift from manifest-generated routing metadata.
Proposed refactor
'use strict'; +const { PHASES_SUBCOMMANDS } = require('./command-aliases.generated.cjs'); @@ } else { - error('Unknown phases subcommand. Available: list, clear'); + error(`Unknown phases subcommand. Available: ${Array.from(PHASES_SUBCOMMANDS).join(', ')}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@get-shit-done/bin/lib/phases-command-router.cjs` at line 24, The error message currently hardcodes available subcommands in the error(...) call; replace that static text with the runtime/generated subcommand metadata (e.g., the manifest variable holding subcommands such as availableSubcommands or subcommandMetadata) so the message always reflects the actual routing data — update the error(...) invocation in phases-command-router.cjs to interpolate or join the generated list (use the existing manifest variable that the router uses) instead of the hardcoded "list, clear" string.sdk/scripts/gen-command-aliases.ts (1)
76-76: 💤 Low valuePath resolution assumes script runs from repo root.
resolve('sdk/src/query/command-aliases.generated.ts')resolves relative to CWD. This works correctly when invoked via npm scripts from the repo root but would produce incorrect paths if run from elsewhere. This is a standard pattern for build scripts, just worth noting.If portability is needed, use
__dirnameorimport.meta.url:import { dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; const __dirname = dirname(fileURLToPath(import.meta.url)); const outPath = resolve(__dirname, '../src/query/command-aliases.generated.ts');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/scripts/gen-command-aliases.ts` at line 76, The outPath constant in gen-command-aliases.ts currently uses resolve('sdk/src/query/command-aliases.generated.ts') which depends on the CWD; change the path construction for outPath to be based on the script file location instead (use __dirname or derive it from import.meta.url/fileURLToPath) so the generated path is correct regardless of where the script is invoked; update the code that sets outPath (the symbol outPath in sdk/scripts/gen-command-aliases.ts) to join the script directory with the relative ../src/query/command-aliases.generated.ts path using path.resolve/path.join.get-shit-done/bin/lib/state-command-router.cjs (1)
65-66: 💤 Low valueRouter handles
complete-phasebut it's not in the manifest.The
complete-phasesubcommand is handled here but doesn't appear inSTATE_COMMAND_ALIASES(and thusSTATE_SUBCOMMANDS). This means:
- The command works when invoked directly
- But it won't appear in the "Available:" error message at line 73
If this is intentional (deprecated/internal command), consider adding a comment. Otherwise, add it to
STATE_COMMAND_MANIFEST.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@get-shit-done/bin/lib/state-command-router.cjs` around lines 65 - 66, The router handles the 'complete-phase' subcommand via state.cmdStateCompletePhase(cwd, raw) but 'complete-phase' is missing from STATE_COMMAND_ALIASES / STATE_COMMAND_MANIFEST so it won't appear in the "Available:" list (STATE_SUBCOMMANDS); either add 'complete-phase' to STATE_COMMAND_MANIFEST/STATE_COMMAND_ALIASES so it shows up in help, or if it's intended to be internal/deprecated, add an inline comment next to the state.cmdStateCompletePhase usage documenting that intent and why it's omitted from STATE_COMMAND_MANIFEST.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/src/query/index.ts`:
- Around line 147-148: The list of validate commands ('validate.health',
'validate.context') are hardcoded into QUERY_MUTATION_COMMANDS but
VALIDATE_COMMAND_ALIASES marks validate commands with mutation: false, so
reconcile the manifest-as-truth: either update the validate manifest to set
mutation: true, regenerate the aliases and expose a VALIDATE_MUTATION_COMMANDS
export and include that instead of hardcoding these strings, or remove
'validate.health' and 'validate.context' from QUERY_MUTATION_COMMANDS so they
match VALIDATE_COMMAND_ALIASES; update the code paths that reference
QUERY_MUTATION_COMMANDS/VALIDATE_COMMAND_ALIASES accordingly.
---
Nitpick comments:
In `@get-shit-done/bin/lib/init-command-router.cjs`:
- Line 62: The error message hardcodes the workflow names; instead
import/require the generated manifest/metadata used by the command seam (the
same module that exposes available workflows), compute the available list
dynamically (e.g., derive keys or names from that manifest), and replace the
hardcoded list in the error(`Unknown init workflow: ${workflow} ...`) call with
the generated list so the message always reflects the manifest-backed commands;
ensure you use the same symbol/source the router uses for routing to avoid
divergence and format the list consistently for the error output.
In `@get-shit-done/bin/lib/phases-command-router.cjs`:
- Line 24: The error message currently hardcodes available subcommands in the
error(...) call; replace that static text with the runtime/generated subcommand
metadata (e.g., the manifest variable holding subcommands such as
availableSubcommands or subcommandMetadata) so the message always reflects the
actual routing data — update the error(...) invocation in
phases-command-router.cjs to interpolate or join the generated list (use the
existing manifest variable that the router uses) instead of the hardcoded "list,
clear" string.
In `@get-shit-done/bin/lib/state-command-router.cjs`:
- Around line 65-66: The router handles the 'complete-phase' subcommand via
state.cmdStateCompletePhase(cwd, raw) but 'complete-phase' is missing from
STATE_COMMAND_ALIASES / STATE_COMMAND_MANIFEST so it won't appear in the
"Available:" list (STATE_SUBCOMMANDS); either add 'complete-phase' to
STATE_COMMAND_MANIFEST/STATE_COMMAND_ALIASES so it shows up in help, or if it's
intended to be internal/deprecated, add an inline comment next to the
state.cmdStateCompletePhase usage documenting that intent and why it's omitted
from STATE_COMMAND_MANIFEST.
In `@sdk/scripts/gen-command-aliases.ts`:
- Line 76: The outPath constant in gen-command-aliases.ts currently uses
resolve('sdk/src/query/command-aliases.generated.ts') which depends on the CWD;
change the path construction for outPath to be based on the script file location
instead (use __dirname or derive it from import.meta.url/fileURLToPath) so the
generated path is correct regardless of where the script is invoked; update the
code that sets outPath (the symbol outPath in
sdk/scripts/gen-command-aliases.ts) to join the script directory with the
relative ../src/query/command-aliases.generated.ts path using
path.resolve/path.join.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 481638cb-307d-4779-bccc-b76d6132a1a5
📒 Files selected for processing (26)
get-shit-done/bin/gsd-tools.cjsget-shit-done/bin/lib/command-aliases.generated.cjsget-shit-done/bin/lib/init-command-router.cjsget-shit-done/bin/lib/phase-command-router.cjsget-shit-done/bin/lib/phases-command-router.cjsget-shit-done/bin/lib/roadmap-command-router.cjsget-shit-done/bin/lib/state-command-router.cjsget-shit-done/bin/lib/validate-command-router.cjsget-shit-done/bin/lib/verify-command-router.cjssdk/scripts/check-command-aliases-fresh.mjssdk/scripts/gen-command-aliases.tssdk/src/query/QUERY-HANDLERS.mdsdk/src/query/command-aliases.generated.tssdk/src/query/command-manifest.init.tssdk/src/query/command-manifest.phase.tssdk/src/query/command-manifest.phases.tssdk/src/query/command-manifest.roadmap.tssdk/src/query/command-manifest.state.tssdk/src/query/command-manifest.tssdk/src/query/command-manifest.types.tssdk/src/query/command-manifest.validate.tssdk/src/query/command-manifest.verify.tssdk/src/query/command-seam-coverage.test.tssdk/src/query/index.tssdk/src/query/normalize-query-command.test.tssdk/src/query/normalize-query-command.ts
trek-e
force-pushed
the
pr/command-seam-foundation
branch
from
980c094 to
f3f9493
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@get-shit-done/bin/lib/phases-command-router.cjs`:
- Around line 12-24: The router only handles 'list' and 'clear' so 'archive'
falls through; add an else-if branch for subcommand === 'archive' that calls the
archive handler on the same object used for list (e.g. phase.cmdPhasesArchive)
with the same arguments pattern (cwd, raw and any remaining args as
appropriate—likely args.slice(2) or the options pattern used for list), so that
'phases archive' is routed to phase.cmdPhasesArchive instead of hitting the
unknown-subcommand error.
In `@get-shit-done/bin/lib/state-command-router.cjs`:
- Around line 16-17: The router calls state.cmdStateGet which isn't exported
from the state module and will crash at runtime; fix by either exporting the
handler named cmdStateGet from the state module (add cmdStateGet to the module's
export list and ensure the function exists) or change the router to call the
actual exported symbol (e.g., state.get or state.cmdGet) that implements the
"get" behavior so the name used in the call matches an exported function.
- Around line 67-74: The router fails to handle the advertised
"add-roadmap-evolution" subcommand, falling through to the unknown-subcommand
error; add a branch for subcommand === 'add-roadmap-evolution' (alongside the
existing 'milestone-switch' and 'load' branches) and invoke the matching SDK
handler (e.g., call state.cmdStateAddRoadmapEvolution with the appropriate
parsed args or raw input), or parse required named args via parseNamedArgs and
forward them to state.cmdStateAddRoadmapEvolution(cwd, ...); ensure the
subcommand string matches the entry in STATE_SUBCOMMANDS so manifest-backed
parity is restored.
In `@get-shit-done/bin/lib/verify-command-router.cjs`:
- Around line 20-22: The router is passing args[2] directly to
verify.cmdVerifySchemaDrift which treats a flag like '--skip' as the schema
target; instead, from the schema-drift branch (where skipFlag is computed) build
positionalArgs = args.slice(2) and pick the first element that does not start
with '-' (the first non-flag positional) and pass that as the schema/phase
argument to verify.cmdVerifySchemaDrift(cwd, <positional>, skipFlag, raw),
leaving skipFlag calculation intact.
In `@sdk/scripts/gen-command-aliases.ts`:
- Around line 76-118: The generator currently sets outPath using process cwd
(outPath = resolve('sdk/src/query/command-aliases.generated.ts')), which breaks
when running from inside sdk; change it to resolve relative to the script file
location in gen-command-aliases.ts (use __dirname or equivalent import.meta.url
resolution) so outPath points to the sdk/src/query/command-aliases.generated.ts
path regardless of cwd; update the outPath assignment in gen-command-aliases.ts
to build the path from the script directory (e.g., resolve(__dirname, '..',
'src', 'query', 'command-aliases.generated.ts')) before calling writeFile.
In `@sdk/src/query/command-seam-coverage.test.ts`:
- Around line 28-30: The test builds a "generated" Map from alias arrays but
omits entry.mutation; update the map population in command-seam-coverage.test.ts
to carry entry.mutation into the stored object (e.g., store { aliases,
subcommand, mutation } when iterating over STATE_COMMAND_ALIASES,
VERIFY_COMMAND_ALIASES, INIT_COMMAND_ALIASES, PHASE_COMMAND_ALIASES,
PHASES_COMMAND_ALIASES, VALIDATE_COMMAND_ALIASES, ROADMAP_COMMAND_ALIASES) and
then add assertions that compare the expected/generated mutation value alongside
aliases and subcommand so the test fails when mutation metadata drifts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 02a17beb-cb5b-4e5a-94b1-3838a5c937de
📒 Files selected for processing (26)
get-shit-done/bin/gsd-tools.cjsget-shit-done/bin/lib/command-aliases.generated.cjsget-shit-done/bin/lib/init-command-router.cjsget-shit-done/bin/lib/phase-command-router.cjsget-shit-done/bin/lib/phases-command-router.cjsget-shit-done/bin/lib/roadmap-command-router.cjsget-shit-done/bin/lib/state-command-router.cjsget-shit-done/bin/lib/validate-command-router.cjsget-shit-done/bin/lib/verify-command-router.cjssdk/scripts/check-command-aliases-fresh.mjssdk/scripts/gen-command-aliases.tssdk/src/query/QUERY-HANDLERS.mdsdk/src/query/command-aliases.generated.tssdk/src/query/command-manifest.init.tssdk/src/query/command-manifest.phase.tssdk/src/query/command-manifest.phases.tssdk/src/query/command-manifest.roadmap.tssdk/src/query/command-manifest.state.tssdk/src/query/command-manifest.tssdk/src/query/command-manifest.types.tssdk/src/query/command-manifest.validate.tssdk/src/query/command-manifest.verify.tssdk/src/query/command-seam-coverage.test.tssdk/src/query/index.tssdk/src/query/normalize-query-command.test.tssdk/src/query/normalize-query-command.ts
✅ Files skipped from review due to trivial changes (8)
- sdk/src/query/command-manifest.state.ts
- sdk/src/query/command-manifest.phase.ts
- sdk/src/query/command-manifest.init.ts
- sdk/src/query/command-manifest.verify.ts
- sdk/src/query/command-manifest.types.ts
- sdk/src/query/QUERY-HANDLERS.md
- get-shit-done/bin/lib/init-command-router.cjs
- get-shit-done/bin/lib/command-aliases.generated.cjs
🚧 Files skipped from review as they are similar to previous changes (10)
- sdk/src/query/command-manifest.roadmap.ts
- sdk/src/query/command-manifest.ts
- sdk/src/query/command-manifest.validate.ts
- sdk/src/query/command-manifest.phases.ts
- get-shit-done/bin/lib/phase-command-router.cjs
- sdk/src/query/command-aliases.generated.ts
- sdk/src/query/normalize-query-command.ts
- sdk/scripts/check-command-aliases-fresh.mjs
- get-shit-done/bin/lib/roadmap-command-router.cjs
- get-shit-done/bin/gsd-tools.cjs
Comment on lines
+12
to
+24
| if (subcommand === 'list') { | ||
| const typeIndex = args.indexOf('--type'); | ||
| const phaseIndex = args.indexOf('--phase'); | ||
| const options = { | ||
| type: typeIndex !== -1 ? args[typeIndex + 1] : null, | ||
| phase: phaseIndex !== -1 ? args[phaseIndex + 1] : null, | ||
| includeArchived: args.includes('--include-archived'), | ||
| }; | ||
| phase.cmdPhasesList(cwd, options, raw); | ||
| } else if (subcommand === 'clear') { | ||
| milestone.cmdPhasesClear(cwd, raw, args.slice(2)); | ||
| } else { | ||
| error('Unknown phases subcommand. Available: list, clear'); |
There was a problem hiding this comment.
phases.archive is missing from the CJS router.
This router only recognizes list and clear, but the migrated phases family also registers archive in sdk/src/query/index.ts Lines 459-463. gsd-tools phases archive ... will fall into the unknown-subcommand path even though the manifest-backed SDK treats it as supported.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@get-shit-done/bin/lib/phases-command-router.cjs` around lines 12 - 24, The
router only handles 'list' and 'clear' so 'archive' falls through; add an
else-if branch for subcommand === 'archive' that calls the archive handler on
the same object used for list (e.g. phase.cmdPhasesArchive) with the same
arguments pattern (cwd, raw and any remaining args as appropriate—likely
args.slice(2) or the options pattern used for list), so that 'phases archive' is
routed to phase.cmdPhasesArchive instead of hitting the unknown-subcommand
error.
Comment on lines
+16
to
+17
| } else if (subcommand === 'get') { | ||
| state.cmdStateGet(cwd, args[2], raw); |
There was a problem hiding this comment.
state.get looks like a runtime crash path.
Line 17 calls state.cmdStateGet(...), but the provided get-shit-done/bin/lib/state.cjs export list does not include cmdStateGet. If gsd-tools.cjs passes that module through, state get will throw instead of dispatching.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@get-shit-done/bin/lib/state-command-router.cjs` around lines 16 - 17, The
router calls state.cmdStateGet which isn't exported from the state module and
will crash at runtime; fix by either exporting the handler named cmdStateGet
from the state module (add cmdStateGet to the module's export list and ensure
the function exists) or change the router to call the actual exported symbol
(e.g., state.get or state.cmdGet) that implements the "get" behavior so the name
used in the call matches an exported function.
Comment on lines
+67
to
+74
| } else if (subcommand === 'milestone-switch') { | ||
| const { milestone, name } = parseNamedArgs(args, ['milestone', 'name']); | ||
| state.cmdStateMilestoneSwitch(cwd, milestone, name, raw); | ||
| } else if (subcommand === undefined || subcommand === 'load') { | ||
| state.cmdStateLoad(cwd, raw); | ||
| } else { | ||
| error(`Unknown state subcommand: "${subcommand}". Available: ${['load', ...STATE_SUBCOMMANDS.filter((s) => s !== 'load')].join(', ')}`); | ||
| } |
There was a problem hiding this comment.
state.add-roadmap-evolution is advertised but unreachable here.
The generated STATE_SUBCOMMANDS set already includes add-roadmap-evolution, and the SDK registry wires state.add-roadmap-evolution, but this router falls straight to the unknown-subcommand branch. That breaks the manifest-backed parity this PR is introducing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@get-shit-done/bin/lib/state-command-router.cjs` around lines 67 - 74, The
router fails to handle the advertised "add-roadmap-evolution" subcommand,
falling through to the unknown-subcommand error; add a branch for subcommand ===
'add-roadmap-evolution' (alongside the existing 'milestone-switch' and 'load'
branches) and invoke the matching SDK handler (e.g., call
state.cmdStateAddRoadmapEvolution with the appropriate parsed args or raw
input), or parse required named args via parseNamedArgs and forward them to
state.cmdStateAddRoadmapEvolution(cwd, ...); ensure the subcommand string
matches the entry in STATE_SUBCOMMANDS so manifest-backed parity is restored.
Collaborator
Author
|
Addressed CI + CodeRabbit feedback in commit 56268ec:
This targets the failing test groups reported in CI: inventory parity, dispatcher error paths, and |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
get-shit-done/bin/lib/state-command-router.cjs (1)
72-74: ⚡ Quick winAdd a manifest-to-router drift guard in the fallback branch.
If a subcommand exists in
STATE_SUBCOMMANDSbut has no router branch, this currently looks like a normal unknown command. Add an explicit drift error before the generic fallback.Suggested hardening
} else { + if (subcommand && STATE_SUBCOMMANDS.includes(subcommand)) { + return error(`Router drift: "${subcommand}" is declared in command aliases but not wired in state-command-router.cjs`); + } const available = ['load', 'complete-phase', ...STATE_SUBCOMMANDS.filter((s) => s !== 'load')]; error(`Unknown state subcommand: "${subcommand}". Available: ${available.join(', ')}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@get-shit-done/bin/lib/state-command-router.cjs` around lines 72 - 74, Add a manifest-to-router drift guard in the fallback branch: before constructing the generic "Unknown state subcommand" message, check if the subcommand exists in STATE_SUBCOMMANDS (use STATE_SUBCOMMANDS.includes(subcommand)); if it does, emit a specific drift/error message via error() indicating "Router drift: manifest contains '<subcommand>' but no router handler exists" (or similar) so maintainers can distinguish a missing router branch from a truly unknown command; otherwise fall back to the existing available list message. Reference: STATE_SUBCOMMANDS, subcommand, error, available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@get-shit-done/bin/lib/state-command-router.cjs`:
- Around line 47-56: The handlers for 'begin-phase' and 'planned-phase' call
state.cmdStateBeginPhase and state.cmdStatePlannedPhase with plans parsed into a
Number that can be NaN when --plans is non-numeric; validate the plans value
returned from parseNamedArgs (from the variable named plans) before calling
these state methods: if plans is null keep null, otherwise parseInt and check
Number.isFinite/Number.isInteger (or isNaN) and reject non-numeric values by
logging an error or exiting with a non-zero code instead of passing NaN to
cmdStateBeginPhase/cmdStatePlannedPhase; ensure the validation logic sits just
before the existing calls so you only pass a valid integer or null to those
functions.
---
Nitpick comments:
In `@get-shit-done/bin/lib/state-command-router.cjs`:
- Around line 72-74: Add a manifest-to-router drift guard in the fallback
branch: before constructing the generic "Unknown state subcommand" message,
check if the subcommand exists in STATE_SUBCOMMANDS (use
STATE_SUBCOMMANDS.includes(subcommand)); if it does, emit a specific drift/error
message via error() indicating "Router drift: manifest contains '<subcommand>'
but no router handler exists" (or similar) so maintainers can distinguish a
missing router branch from a truly unknown command; otherwise fall back to the
existing available list message. Reference: STATE_SUBCOMMANDS, subcommand,
error, available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1322bb39-687d-4773-b9ec-de62bf5e44f3
📒 Files selected for processing (5)
docs/INVENTORY-MANIFEST.jsondocs/INVENTORY.mdget-shit-done/bin/lib/state-command-router.cjssdk/src/query/index.tstests/gsd-sdk-query-registry-integration.test.cjs
✅ Files skipped from review due to trivial changes (1)
- docs/INVENTORY-MANIFEST.json
There was a problem hiding this comment.
♻️ Duplicate comments (1)
get-shit-done/bin/lib/phases-command-router.cjs (1)
11-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
phases.archiveis still being dropped.The CJS router still advertises only
list/clear, sogsd-tools phases archive ...will hit the unknown-subcommand path even though the manifest-backed SDK now recognizes the family. Please wire the archive handler through this router instead of excluding it fromPHASES_SUBCOMMANDS.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@get-shit-done/bin/lib/phases-command-router.cjs` around lines 11 - 31, The router is dropping the archive subcommand: update routePhasesCommand to route 'archive' to the SDK handler instead of excluding it; specifically, add a branch for subcommand === 'archive' that invokes the archive handler (e.g., call phase.cmdPhasesArchive or the appropriate phase.archive entry with cwd, raw, and the remaining args) and remove the special-case exclusion from how cjsSupported is built (don’t filter out 'archive' from PHASES_SUBCOMMANDS) so the unknown-subcommand message lists archive correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@get-shit-done/bin/lib/phases-command-router.cjs`:
- Around line 11-31: The router is dropping the archive subcommand: update
routePhasesCommand to route 'archive' to the SDK handler instead of excluding
it; specifically, add a branch for subcommand === 'archive' that invokes the
archive handler (e.g., call phase.cmdPhasesArchive or the appropriate
phase.archive entry with cwd, raw, and the remaining args) and remove the
special-case exclusion from how cjsSupported is built (don’t filter out
'archive' from PHASES_SUBCOMMANDS) so the unknown-subcommand message lists
archive correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: de78132b-975d-485b-b244-ccaa753ce92b
📒 Files selected for processing (6)
get-shit-done/bin/lib/init-command-router.cjsget-shit-done/bin/lib/phases-command-router.cjsget-shit-done/bin/lib/state-command-router.cjsget-shit-done/bin/lib/verify-command-router.cjssdk/scripts/gen-command-aliases.tssdk/src/query/command-seam-coverage.test.ts
✅ Files skipped from review due to trivial changes (1)
- get-shit-done/bin/lib/init-command-router.cjs
🚧 Files skipped from review as they are similar to previous changes (1)
- get-shit-done/bin/lib/state-command-router.cjs
Collaborator
Author
|
Follow-up CodeRabbit fixes pushed:
Local verification run:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@get-shit-done/bin/lib/state-command-router.cjs`:
- Around line 48-50: The --plans argument currently uses Number.parseInt(...)
with Number.isNaN(...) which permits partial numeric strings like "1.5" or
"2abc"; update both places that parse --plans (replace the Number.parseInt +
Number.isNaN pattern around the --plans handling) to perform strict integer
validation by first asserting the raw string matches /^\d+$/ (or /^\-?\d+$/ if
negatives are allowed) and only then calling Number.parseInt(..., 10); if the
check fails, reject/throw an error or show a validation message so invalid
values never reach the state mutation handlers that consume plans.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5415855e-1570-4e32-9f83-c97b1ef6afb4
📒 Files selected for processing (1)
get-shit-done/bin/lib/state-command-router.cjs
Comment on lines
+48
to
+50
| const parsedPlans = plans == null ? null : Number.parseInt(plans, 10); | ||
| if (plans != null && Number.isNaN(parsedPlans)) { | ||
| return error('Invalid --plans value. Expected an integer.'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the two plans-validation sites and confirm strict integer guards are present.
rg -n -C3 "begin-phase|planned-phase|Number\.parseInt\(plans|Invalid --plans value|/\\^-\?\\\\d\\+\\$/" get-shit-done/bin/lib/state-command-router.cjsRepository: gsd-build/get-shit-done
Length of output: 1427
🌐 Web query:
In JavaScript, what do these return: Number.parseInt("1.5", 10) and Number.parseInt("2abc", 10)? Does parseInt enforce whole-string integer validation?
💡 Result:
Number.parseInt("1.5", 10) returns 1 and Number.parseInt("2abc", 10) returns 2. No, parseInt does not enforce whole-string integer validation. It parses valid integer digits from the start of the string until it encounters the first invalid character (not a digit in the given radix), then stops and returns the integer value up to that point, ignoring the rest. The decimal point '.' and letters like 'a', 'b', 'c' are invalid digits in base 10, so parsing stops there. For "1.5": parses "1", stops at '.', returns 1. For "2abc": parses "2", stops at 'a', returns 2.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/parseInt
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt?retiredLocale=de
- 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt.
- 5: https://www.stefanjudis.com/today-i-learned/parseint-parses-anything-starting-with-a-number/
Enforce strict integer validation for --plans argument.
Lines 48-50 and 60-63 use Number.parseInt(..., 10) followed by Number.isNaN() check, which accepts partial numeric strings like "1.5" (parses to 1) and "2abc" (parses to 2). This allows unintended values to reach state mutation handlers.
Suggested fix
} else if (subcommand === 'begin-phase') {
const { phase: p, name, plans } = parseNamedArgs(args, ['phase', 'name', 'plans']);
- const parsedPlans = plans == null ? null : Number.parseInt(plans, 10);
- if (plans != null && Number.isNaN(parsedPlans)) {
+ const plansToken = plans == null ? null : String(plans).trim();
+ if (plansToken != null && !/^-?\d+$/.test(plansToken)) {
return error('Invalid --plans value. Expected an integer.');
}
+ const parsedPlans = plansToken == null ? null : Number.parseInt(plansToken, 10);
state.cmdStateBeginPhase(cwd, p, name, parsedPlans, raw);
} else if (subcommand === 'signal-waiting') {
@@
} else if (subcommand === 'planned-phase') {
const { phase: p, plans } = parseNamedArgs(args, ['phase', 'name', 'plans']);
- const parsedPlans = plans == null ? null : Number.parseInt(plans, 10);
- if (plans != null && Number.isNaN(parsedPlans)) {
+ const plansToken = plans == null ? null : String(plans).trim();
+ if (plansToken != null && !/^-?\d+$/.test(plansToken)) {
return error('Invalid --plans value. Expected an integer.');
}
+ const parsedPlans = plansToken == null ? null : Number.parseInt(plansToken, 10);
state.cmdStatePlannedPhase(cwd, p, parsedPlans, raw);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@get-shit-done/bin/lib/state-command-router.cjs` around lines 48 - 50, The
--plans argument currently uses Number.parseInt(...) with Number.isNaN(...)
which permits partial numeric strings like "1.5" or "2abc"; update both places
that parse --plans (replace the Number.parseInt + Number.isNaN pattern around
the --plans handling) to perform strict integer validation by first asserting
the raw string matches /^\d+$/ (or /^\-?\d+$/ if negatives are allowed) and only
then calling Number.parseInt(..., 10); if the check fails, reject/throw an error
or show a validation message so invalid values never reach the state mutation
handlers that consume plans.
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
Implements a manifest-backed routing foundation so SDK and CJS command routing share canonical command identity/aliases across migrated families.
What changed
gen-command-aliases) and generated TS/CJS artifactsgsd-tools.cjsmanifest -> generated -> registry/adapters)Verification
cd sdk && npm run buildcd sdk && npx vitest run src/query/command-seam-coverage.test.ts src/query/normalize-query-command.test.tsCloses #2905
Summary by CodeRabbit
Refactor
Chores
Tests
Documentation