feat(sdk): golden parity harness and query handler CJS alignment (#2302 Track A)#2341
Merged
trek-e merged 11 commits intogsd-build:mainfrom Apr 20, 2026
Merged
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:
📝 WalkthroughWalkthroughThis PR implements Phase 3 of the SDK query contract and golden parity initiative ( Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as gsd-sdk query
participant Norm as normalizeQueryCommand
participant Reg as createRegistry()
participant Handler as Query Handler
participant CJS as gsd-tools.cjs
CLI->>Norm: (command, args)
Norm-->>CLI: (normalized_cmd, args)
CLI->>Reg: resolveQueryArgv(normalized_cmd, ...)
alt Handler found
Reg-->>Handler: dispatch
Handler->>Handler: execute logic
Handler-->>CLI: JSON result
else No handler + GSD_QUERY_FALLBACK=on
Reg-->>CLI: no match
CLI->>CJS: shell exec with argv mapping
CJS-->>CLI: stdout (JSON/raw)
CLI->>CLI: apply --pick filter
CLI-->>CLI: parse & emit
else No handler + GSD_QUERY_FALLBACK=off
Reg-->>CLI: error
end
sequenceDiagram
participant User as User/Agent
participant Ingest as /gsd-ingest-docs
participant Classify as gsd-doc-classifier
participant Synth as gsd-doc-synthesizer
participant Gate as Conflict Gate
participant Plan as .planning/
User->>Ingest: (path, --mode, --manifest)
Ingest->>Ingest: discover docs (ADR/PRD/SPEC/DOC dirs)
Ingest->>Classify: classify each doc (parallel)
Classify-->>Ingest: type, confidence, locked status
Ingest->>Synth: synthesize with precedence rules
Synth->>Synth: merge (ADR > SPEC > PRD > DOC)
Synth->>Synth: detect conflicts & cycles
Synth-->>Ingest: INGEST-CONFLICTS.md (3 buckets)
Ingest->>Gate: check BLOCKER gate
alt BLOCKER exists
Gate-->>Ingest: abort, no writes
else Only WARNING/INFO
Gate->>Gate: prompt approval
Gate-->>Ingest: approved or rejected
else No conflicts
Gate-->>Ingest: proceed
end
Ingest->>Plan: write .planning/ROADMAP.md, REQUIREMENTS.md, etc.
Ingest-->>User: completion summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Closed
11 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
sdk/src/query/index.ts (1)
119-149:⚠️ Potential issue | 🟡 MinorRemove
state.validatefromQUERY_MUTATION_COMMANDS.The
stateValidatehandler only reads and validates state consistency without performing any write operations. It checks for drift betweenSTATE.mdand disk but returns validation results without modifying files. Mutation commands should only include handlers that write state changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/index.ts` around lines 119 - 149, The QUERY_MUTATION_COMMANDS set currently includes the read-only state validation command; remove 'state.validate' (and its space-separated variant 'state validate' if present) from the QUERY_MUTATION_COMMANDS Set so only true mutating commands remain—update the exported constant QUERY_MUTATION_COMMANDS accordingly in sdk/src/query/index.ts (look for the QUERY_MUTATION_COMMANDS declaration) to exclude those entries.sdk/src/query/state-mutation.ts (2)
843-854:⚠️ Potential issue | 🟡 MinorOnly report
resolved: truewhen a blocker was actually removed.
resolvedflips totruewhenever the blockers section exists, even if no-line matchessearchTextand the content is unchanged. That gives false success to callers and automation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/state-mutation.ts` around lines 843 - 854, The current logic sets resolved = true whenever the blockers section exists even if no matching bullet was removed; change it to set resolved = true only when at least one '- ' line containing searchText (case-insensitive) was actually removed. Locate the block using filtered, newBody, sectionPattern and content.replace; compute whether any original lines starting with '- ' matched searchText (or compare original section text vs newBody) and only set resolved = true when that removal count > 0, leaving resolved false if nothing changed.
196-204:⚠️ Potential issue | 🔴 CriticalDon't return a lock path unless the lock was actually acquired.
Both the final
EEXISTbranch and the non-EEXISTbranch returnlockPathwithout creating the lock file or recording ownership. Callers then enter the read/modify/write critical section unlocked, which defeats the atomicity guarantee and can corrupt concurrentSTATE.mdwrites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/state-mutation.ts` around lines 196 - 204, The code currently returns lockPath in the final EEXIST retry branch and in the non-EEXIST branch without actually creating/owning the lock; update the logic so you never return lockPath unless the lock file was successfully created. Specifically, in the retry-exhausted branch (where i === maxRetries - 1) remove the "return lockPath" and instead throw a clear error (e.g., "Failed to acquire lock: " + lockPath) after attempting unlink, and in the non-EEXIST branch do not return lockPath—rethrow the caught error or throw a new error with context so callers do not enter the critical section unlocked; use the existing symbols lockPath, maxRetries, retryDelay and unlink to locate and update this behavior.
🟡 Minor comments (8)
sdk/src/query/config-query.ts-59-63 (1)
59-63:⚠️ Potential issue | 🟡 MinorGuard unknown profile keys in
getAgentToModelMapForProfile.The function’s return type is
Record<string, string>, but unknownnormalizedProfilevalues can produceundefinedmodel entries at runtime.💡 Suggested fix
export function getAgentToModelMapForProfile(normalizedProfile: string): Record<string, string> { + const profile = VALID_PROFILES.includes(normalizedProfile) ? normalizedProfile : 'balanced'; const agentToModelMap: Record<string, string> = {}; for (const [agent, profileToModelMap] of Object.entries(MODEL_PROFILES)) { - agentToModelMap[agent] = profileToModelMap[normalizedProfile]; + agentToModelMap[agent] = profileToModelMap[profile] ?? profileToModelMap.balanced ?? 'sonnet'; } return agentToModelMap; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/config-query.ts` around lines 59 - 63, getAgentToModelMapForProfile currently assigns profileToModelMap[normalizedProfile] directly which can be undefined for unknown profiles, violating the Record<string,string> return type; update getAgentToModelMapForProfile to guard against missing keys from MODEL_PROFILES by checking that profileToModelMap[normalizedProfile] is a defined string before assigning to agentToModelMap (or alternatively throw a clear error if an unknown profile should be considered exceptional), ensuring the function only returns agents with defined model strings.sdk/src/query/config-mutation.ts-294-295 (1)
294-295:⚠️ Potential issue | 🟡 MinorNormalize and validate
previousProfilebefore returning it.
previousProfilecurrently accepts any non-empty string, so invalid/stale values can leak into response payloads instead of falling back tobalanced.💡 Suggested fix
- const prev = config.model_profile; - previousProfile = (typeof prev === 'string' && prev ? prev : 'balanced') as string; + const prev = typeof config.model_profile === 'string' + ? config.model_profile.toLowerCase().trim() + : ''; + previousProfile = VALID_PROFILES.includes(prev) ? prev : 'balanced';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/config-mutation.ts` around lines 294 - 295, The code sets previousProfile from config.model_profile without validating allowed values; update the logic around previousProfile (the assignment using config.model_profile) to normalize and validate against the supported profile names (e.g., a whitelist like ['balanced','fast','accurate'] or the canonical set used elsewhere), trim/ensure it's a string, and if it is missing, empty, or not in the allowed set fall back to 'balanced' before returning or using previousProfile; ensure the check is applied where previousProfile is declared/assigned in config-mutation.ts so stale/invalid strings cannot leak into responses.sdk/src/query/config-gates.test.ts-1-49 (1)
1-49:⚠️ Potential issue | 🟡 MinorAdd temp-dir teardown to prevent
/tmpbuildup across test runs.Both tests create unique directories but never remove them. This can accumulate quickly in CI and local repeated runs.
Suggested cleanup pattern
-import { mkdtemp, mkdir, writeFile } from 'node:fs/promises'; +import { mkdtemp, mkdir, writeFile, rm } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { checkConfigGates } from './config-gates.js'; describe('checkConfigGates', () => { + let dir: string; + + beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), 'gsd-cg-')); + await mkdir(join(dir, '.planning'), { recursive: true }); + }); + + afterEach(async () => { + await rm(dir, { recursive: true, force: true }); + }); + it('returns merged workflow defaults when config is absent', async () => { - const dir = await mkdtemp(join(tmpdir(), 'gsd-cg-')); - await mkdir(join(dir, '.planning'), { recursive: true }); const { data } = await checkConfigGates([], dir); expect(data).toMatchObject({ workflow: null, @@ it('reflects workflow overrides from config.json', async () => { - const dir = await mkdtemp(join(tmpdir(), 'gsd-cg-')); - await mkdir(join(dir, '.planning'), { recursive: true }); await writeFile( join(dir, '.planning', 'config.json'), JSON.stringify({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/config-gates.test.ts` around lines 1 - 49, Tests create temporary dirs via mkdtemp in the two it blocks but never delete them; add cleanup that removes each created temp directory after each test (or in an afterEach/afterAll) using node:fs/promises rm (or rmdir) with recursive/force options to avoid leftover /tmp directories; track the created dir variable(s) from the tests (the local dir returned by mkdtemp in these tests) or push them into an array and purge them in afterEach/afterAll so checkConfigGates tests clean up their temp dirs.sdk/HANDOVER-QUERY-LAYER.md-99-99 (1)
99-99:⚠️ Potential issue | 🟡 MinorEscape the pipe in this table cell.
Line 99 uses
ls | wc -linside a Markdown table cell, which creates an extra column and breaks the row rendering.Suggested fix
-| **Offloading decisions** | **New or consolidated** queries replace repeated `grep`, `ls | wc -l`, many `config-get`s, and inline `node -e` in workflows — so the model does less parsing and branching. | Fewer inline shell blocks; measurable token/step reduction on representative workflows. | +| **Offloading decisions** | **New or consolidated** queries replace repeated `grep`, `ls \| wc -l`, many `config-get`s, and inline `node -e` in workflows — so the model does less parsing and branching. | Fewer inline shell blocks; measurable token/step reduction on representative workflows. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/HANDOVER-QUERY-LAYER.md` at line 99, The Markdown table cell containing the snippet "ls | wc -l" is breaking the table because the pipe is interpreted as a column separator; update that cell by escaping the pipe or making the whole snippet inline code (e.g., replace the literal pipe with an escaped pipe or wrap the snippet in backticks) so the table renders as a single cell; look for the exact text "ls | wc -l" in the table row and change it accordingly.sdk/HANDOVER-PARITY-DOCS.md-86-86 (1)
86-86:⚠️ Potential issue | 🟡 MinorFix the broken inline formatting here.
Line 86 has malformed Markdown around
npm run build/sdk/, so the verification note won't render cleanly.Suggested fix
-- No code behavior change required for pure docs; run `**npm run build*`* in `**sdk/**` only if TypeScript-adjacent files were touched. +- No code behavior change required for pure docs; run `npm run build` in `sdk/` only if TypeScript-adjacent files were touched.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/HANDOVER-PARITY-DOCS.md` at line 86, The Markdown on the line mentioning "npm run build" and "sdk/" has malformed inline formatting; update the text to use proper backticks around the commands/paths (e.g., `npm run build` and `sdk/`) and remove stray asterisks so the verification note renders correctly in HANDOVER-PARITY-DOCS.md (look for the sentence referencing `npm run build` and `sdk/` and fix the inline code formatting).sdk/src/query/profile.ts-213-214 (1)
213-214:⚠️ Potential issue | 🟡 MinorPotential
NaNfromparseIntwhen--limitvalue is missing or invalid.If
--limitis the last argument or followed by another flag,args[limitIdx + 1]could be undefined or a non-numeric string, resulting inNaN. The|| nullfallback handles undefined but notNaN.🛡️ Proposed defensive fix
const limitIdx = args.indexOf('--limit'); - const limit = limitIdx !== -1 ? (parseInt(args[limitIdx + 1]!, 10) || null) : null; + const limitRaw = limitIdx !== -1 ? parseInt(args[limitIdx + 1] ?? '', 10) : NaN; + const limit = Number.isNaN(limitRaw) ? null : limitRaw;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/profile.ts` around lines 213 - 214, The current parse of --limit can produce NaN when args[limitIdx + 1] is missing or non-numeric; update the logic around limitIdx/limit to first ensure a nextArg exists and is a valid integer (e.g., check typeof args[limitIdx + 1] !== 'undefined' and that parseInt returns a finite number or match with a /^\d+$/ check) and only then assign the parsed integer to limit, otherwise set limit to null; apply this change where limitIdx and limit are computed so parseInt(args[limitIdx + 1]) is validated and NaN is never assigned.sdk/src/query/skill-manifest.ts-205-211 (1)
205-211:⚠️ Potential issue | 🟡 MinorMissing error handling for
writeFileSync.If writing
skill-manifest.jsonfails (e.g., permissions, disk full), the error propagates uncaught. Consider wrapping in try/catch to provide a meaningful error response.🛡️ Proposed fix
if (args.includes('--write')) { const planningDir = join(projectDir, '.planning'); if (existsSync(planningDir)) { const manifestPath = join(planningDir, 'skill-manifest.json'); - writeFileSync(manifestPath, JSON.stringify(manifest, null, 2), 'utf-8'); + try { + writeFileSync(manifestPath, JSON.stringify(manifest, null, 2), 'utf-8'); + } catch (err) { + return { data: { ...manifest, write_error: String(err) } }; + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/skill-manifest.ts` around lines 205 - 211, The current block that handles the '--write' flag calls writeFileSync for manifestPath without error handling; wrap the file write in a try/catch around the writeFileSync call (inside the if (args.includes('--write')) branch where planningDir and manifestPath are computed) and on error log or throw a clear, user-friendly message that includes the planningDir/manifestPath and the caught error (use processLogger or throw a new Error mentioning manifestPath and the original error) so permission/disk errors are surfaced gracefully.sdk/src/query/docs-init.ts-41-51 (1)
41-51:⚠️ Potential issue | 🟡 MinorPotential file descriptor leak if
readSyncthrows.If
readSyncthrows afteropenSyncsucceeds,closeSyncis never called and the file descriptor leaks. Use try/finally:🐛 Proposed fix
function hasGsdMarker(filePath: string): boolean { + let fd: number | null = null; try { const buf = Buffer.alloc(500); - const fd = openSync(filePath, 'r'); - const bytesRead = readSync(fd, buf, 0, 500, 0); - closeSync(fd); + fd = openSync(filePath, 'r'); + const bytesRead = readSync(fd, buf, 0, 500, 0); return buf.subarray(0, bytesRead).toString('utf-8').includes(GSD_MARKER); } catch { return false; + } finally { + if (fd !== null) { + try { closeSync(fd); } catch { /* ignore */ } + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/docs-init.ts` around lines 41 - 51, The hasGsdMarker function can leak a file descriptor if openSync succeeds but readSync throws; change the implementation to open the file, then perform readSync inside a try block and always call closeSync in a finally block so the descriptor is closed regardless of errors; keep the existing behavior of returning false on errors but ensure closeSync(fd) is invoked in finally when fd was created.
🧹 Nitpick comments (31)
sdk/src/query/skills.test.ts (1)
28-31: Add a whitespace-only agent type test case.Since
agentSkillsnow trims input (Line 26 inskills.ts), add a case likeagentSkills([' '], ...)to lock in that parity behavior and prevent regressions.Proposed test addition
it('returns empty string when no agent type (matches gsd-tools)', async () => { const r = await agentSkills([], tmpdir()); expect(r.data).toBe(''); }); + + it('returns empty string for whitespace-only agent type', async () => { + const r = await agentSkills([' '], tmpdir()); + expect(r.data).toBe(''); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/skills.test.ts` around lines 28 - 31, Add a whitespace-only agent type test to ensure trimming parity: in skills.test.ts add a new it block that calls agentSkills([' '], tmpdir()) (referencing the agentSkills function) and asserts that the returned r.data is an empty string, mirroring the existing "no agent type" test to prevent regressions now that input is trimmed in skills.ts.sdk/src/query/check-ship-ready.ts (2)
72-79: Clarify or consider:verification_passedis true when status is'missing'.The condition
vdata.status !== 'fail'meansverification_passedwill betruewhen the verification file doesn't exist (status: 'missing') or is partially complete (status: 'partial'). If this is intentional (e.g., allowing ship when verification isn't set up yet), consider adding a comment. Otherwise, you may want to explicitly check for'pass':- verification_passed = vdata.status !== 'fail'; + verification_passed = vdata.status === 'pass';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/check-ship-ready.ts` around lines 72 - 79, The current logic sets verification_passed using vdata.status !== 'fail', which treats 'missing' and 'partial' as passed; update the check in the try block to only treat an explicit pass as success by testing vdata.status === 'pass' (or, if the intent is to allow 'missing'/'partial', add a clarifying comment above the verification_passed assignment explaining that checkVerificationStatus([raw], projectDir) should allow missing/partial statuses). Refer to the variables verification_passed, checkVerificationStatus, verRes, and vdata.status when making the change.
64-66: Minor: redundantghavailability check.If
gh --versionsucceeds,which ghis unnecessary. The||short-circuits, so this is functionally correct but slightly redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/check-ship-ready.ts` around lines 64 - 66, The gh availability check in the gh_available assignment is redundant: boolSyncSafe('gh --version', projectDir) already verifies presence so remove the second call to boolSyncSafe('which gh', projectDir). Update the gh_available declaration (the variable named gh_available) to use only boolSyncSafe('gh --version', projectDir) and keep projectDir and the boolSyncSafe helper unchanged.get-shit-done/bin/lib/state.cjs (1)
34-40: Nice fix here—consider applying the same horizontal-whitespace rule to sibling field regexes.
stateExtractFieldis now safe, butcmdStateGetandstateReplaceFieldstill use\s*and can still consume a newline when a field has no inline value.♻️ Suggested consistency patch
-const boldPattern = new RegExp(`\\*\\*${fieldEscaped}:\\*\\*\\s*(.*)`, 'i'); +const boldPattern = new RegExp(`\\*\\*${fieldEscaped}:\\*\\*[ \\t]*(.*)`, 'i'); -const plainPattern = new RegExp(`^${fieldEscaped}:\\s*(.*)`, 'im'); +const plainPattern = new RegExp(`^${fieldEscaped}:[ \\t]*(.*)`, 'im'); -const boldPattern = new RegExp(`(\\*\\*${fieldEscaped}:\\*\\*\\s*)(.*)`, 'i'); +const boldPattern = new RegExp(`(\\*\\*${fieldEscaped}:\\*\\*[ \\t]*)(.*)`, 'i'); -const plainPattern = new RegExp(`(^${fieldEscaped}:\\s*)(.*)`, 'im'); +const plainPattern = new RegExp(`(^${fieldEscaped}:[ \\t]*)(.*)`, 'im'); -const boldPattern = new RegExp(`(\\*\\*${escaped}:\\*\\*\\s*)(.*)`, 'i'); +const boldPattern = new RegExp(`(\\*\\*${escaped}:\\*\\*[ \\t]*)(.*)`, 'i'); -const plainPattern = new RegExp(`(^${escaped}:\\s*)(.*)`, 'im'); +const plainPattern = new RegExp(`(^${escaped}:[ \\t]*)(.*)`, 'im');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@get-shit-done/bin/lib/state.cjs` around lines 34 - 40, The sibling functions cmdStateGet and stateReplaceField still use \s* which allows newlines to be consumed; update their regexes to mirror stateExtractField by restricting whitespace after the colon to horizontal-only (e.g., use a character class like [ \t]* or equivalent) and ensure patterns are anchored similarly (use ^ with the 'm' flag or the same escaped field token) so they do not match multi-line values; change the regex construction in cmdStateGet and stateReplaceField to the same form used in stateExtractField to prevent newline consumption when a field has no inline value.sdk/src/query/state-mutation.test.ts (1)
274-274: Strengthen thestatePatchassertion for regression signal.
length > 0is permissive; assert expected updated keys to ensure the intended fields were patched.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/state-mutation.test.ts` at line 274, The test uses a permissive check on data.updated length; tighten it by asserting the exact updated keys (or at least that the array contains the specific keys you expect) from the statePatch operation. Replace the length assertion on data.updated in state-mutation.test.ts with an assertion that references data.updated (e.g., expect(data.updated).toEqual(expectedUpdatedKeys) or expect(data.updated).toEqual(expect.arrayContaining([...expectedKeys]))), using the concrete expected key names for the statePatch being tested.sdk/src/query/route-next-action.test.ts (1)
8-60: Add temp directory cleanup to avoid filesystem leakage in test runs.Each test creates a new
mkdtempdirectory but never removes it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/route-next-action.test.ts` around lines 8 - 60, Tests in route-next-action.test.ts create temp dirs via mkdtemp(join(tmpdir(), ...)) but never remove them; wrap each test's temp-dir setup in a try/finally (or add an afterEach cleanup) and delete the created directory using fs.promises.rm/fs.rmSync with recursive:true and force:true to avoid leakage. Locate the mkdtemp usage in the three it blocks (the tests that call routeNextAction) and ensure the created dir is removed in finally (or store the path in a variable and remove it in afterEach) so the temp directories are always cleaned even on test failures.sdk/src/query/phase-ready.test.ts (1)
33-64: Please clean upmkdtempdirectories in this suite.Add
afterEach(or per-testfinally) to remove temp folders and keep test runs side-effect free.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/phase-ready.test.ts` around lines 33 - 64, The tests create temporary directories via mkdtemp(tmpdir(), ...) but never remove them; add cleanup to avoid side effects by capturing each created dir (e.g., a test-scoped variable set inside each it or an array pushed to) and remove them after each test using fs.rm(dir, { recursive: true, force: true }) in an afterEach hook (or wrap individual tests with try/finally to rm on finally). Update the suite around checkPhaseReady, writeMinimalRoadmap and tests that call mkdtemp so each created temp directory is removed after the test finishes.sdk/src/query/phase-lifecycle.test.ts (1)
250-295: Add registry assertions forphase.add-batchso dispatch coverage stays complete.This suite now validates
phaseAddBatchbehavior, but registry integration in the same file still asserts only 7 lifecycle commands. Addphase.add-batchandphase add-batchthere to prevent silent registration regressions.Suggested test update
const commands = [ - 'phase.add', 'phase.insert', 'phase.remove', 'phase.complete', + 'phase.add', 'phase.add-batch', 'phase.insert', 'phase.remove', 'phase.complete', 'phase.scaffold', 'phases.clear', 'phases.archive', ];const commands = [ - 'phase add', 'phase insert', 'phase remove', 'phase complete', + 'phase add', 'phase add-batch', 'phase insert', 'phase remove', 'phase complete', 'phase scaffold', 'phases clear', 'phases archive', ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/phase-lifecycle.test.ts` around lines 250 - 295, Update the registry assertions that currently expect seven lifecycle commands to also include the new entries 'phase.add-batch' and 'phase add-batch' so dispatch coverage includes the batch command; find the test block that asserts the list/count of lifecycle commands (the assertion expecting 7 commands) and add these two strings to the expected array (and update the expected count from 7 to 9), ensuring the test references the same command names used by the CLI and the phaseAddBatch test.sdk/src/query/normalize-query-command.test.ts (1)
36-49: Test name doesn't match test content.The test named "merges phase add-batch for future handler" primarily tests
check.*androute.*prefixes. Thephase.add-batchcase is only one of five assertions. Consider splitting or renaming for clarity.♻️ Suggested rename/split
- it('merges phase add-batch for future handler', () => { + it('merges check.*, route.*, and phase.* prefixes', () => { expect(normalizeQueryCommand('check', ['config-gates', 'plan-phase'])).toEqual([ 'check.config-gates', ['plan-phase'], ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/normalize-query-command.test.ts` around lines 36 - 49, Test name is misleading because the spec mostly asserts normalizeQueryCommand results for 'check.*' and 'route.*' cases and only one 'phase.add-batch' case; either rename the test to reflect multiple command prefixes (e.g., "normalizes various command prefixes including phase.add-batch") or split into two tests—one focused on 'check'/'route' behavior and another dedicated to 'phase add-batch'—by moving the expect(normalizeQueryCommand(...)) assertion for 'phase.add-batch' into its own it(...) block; reference normalizeQueryCommand and the existing it(...) description when renaming or extracting the new test.sdk/src/query/state-project-load.ts (2)
63-65: Clarifystate_existssemantics.
state_existsistrueonly whenSTATE.mdhas content (stateRaw.length > 0), not when the file merely exists. An emptySTATE.mdfile returnsstate_exists: false. This may be intentional for parity but could surprise callers expecting file-existence semantics.Consider documenting this behavior in the JSDoc or renaming to
state_has_content.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/state-project-load.ts` around lines 63 - 65, The variable stateExists is currently set to stateRaw.length > 0 which makes it true only when STATE.md has content (not mere existence); update the code to either (A) rename stateExists to stateHasContent (and update all uses) to clearly reflect semantics, or (B) add JSDoc above the function/constant that explains stateExists means "STATE.md has content" (reference stateRaw and stateExists in the doc); pick one approach and apply consistently across the function that reads STATE.md so callers are not misled.
36-47: Add type guard forloadConfigimport.The CJS require returns
unknownand the type assertion assumesloadConfigexists with the correct signature. Ifcore.cjsexports change, this will fail at runtime with a confusing error.🛡️ Proposed defensive check
function loadConfigCjs(projectDir: string): Record<string, unknown> { const corePath = resolveCoreCjsPath(projectDir); if (!corePath) { throw new GSDError( 'state load: get-shit-done/bin/lib/core.cjs not found. Install GSD (e.g. npm i -g get-shit-done-cc) or clone with get-shit-done next to the SDK.', ErrorClassification.Blocked, ); } const req = createRequire(import.meta.url); - const { loadConfig } = req(corePath) as { loadConfig: (cwd: string) => Record<string, unknown> }; + const coreModule = req(corePath) as Record<string, unknown>; + const loadConfig = coreModule.loadConfig as ((cwd: string) => Record<string, unknown>) | undefined; + if (typeof loadConfig !== 'function') { + throw new GSDError( + 'state load: core.cjs does not export loadConfig function', + ErrorClassification.Blocked, + ); + } return loadConfig(projectDir); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/state-project-load.ts` around lines 36 - 47, The import of loadConfig in loadConfigCjs assumes the required module exports loadConfig with the expected signature; add a runtime type guard after req(corePath) to validate that the module has a loadConfig property that is a function and optionally that calling typeof loadConfig === 'function' before invoking it, and if the guard fails throw a GSDError with a clear message (including the resolved corePath) using ErrorClassification.Blocked; update loadConfigCjs (and the local req/corePath handling) to perform this defensive check and only call loadConfig(projectDir) when the guard passes.sdk/src/query/normalize-query-command.ts (1)
37-56: Consider consolidatingprogress/statsinto the prefix set.The
progressandstatsprefixes (lines 51-53) use identical logic to theMERGE_FIRST_WITH_SUBCOMMANDhandling (lines 46-49). Adding them to the set would eliminate the duplicate branch.♻️ Optional consolidation
const MERGE_FIRST_WITH_SUBCOMMAND = new Set<string>([ 'state', 'template', 'frontmatter', 'verify', 'phase', 'phases', 'roadmap', 'requirements', 'validate', 'init', 'workstream', 'intel', 'learnings', 'uat', 'todo', 'milestone', 'check', 'detect', 'route', + 'progress', + 'stats', ]); export function normalizeQueryCommand(command: string, args: string[]): [string, string[]] { if (command === 'scaffold') { return ['phase.scaffold', args]; } if (command === 'state' && args.length === 0) { return ['state.load', []]; } if (MERGE_FIRST_WITH_SUBCOMMAND.has(command) && args.length > 0) { const sub = args[0]; return [`${command}.${sub}`, args.slice(1)]; } - if ((command === 'progress' || command === 'stats') && args.length > 0) { - return [`${command}.${args[0]}`, args.slice(1)]; - } - return [command, args]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/normalize-query-command.ts` around lines 37 - 56, The branch handling progress/stats duplicates the MERGE_FIRST_WITH_SUBCOMMAND logic; add 'progress' and 'stats' to the MERGE_FIRST_WITH_SUBCOMMAND set and remove the explicit if ((command === 'progress' || command === 'stats') ...) block in normalizeQueryCommand so the existing MERGE_FIRST_WITH_SUBCOMMAND branch (which creates `${command}.${sub}` and slices args) handles them identically, preserving the same args.length > 0 guard and fallback behavior.sdk/src/query/detect-custom-files.ts (3)
40-44: Missing bounds check for--config-dirargument value.If
--config-diris the last argument with no value,args[configDirIdx + 1]will beundefined, which passes the!configDircheck but could be clearer.♻️ Explicit bounds check
const configDirIdx = args.indexOf('--config-dir'); - const configDir = configDirIdx !== -1 ? args[configDirIdx + 1] : null; + const configDir = configDirIdx !== -1 && configDirIdx + 1 < args.length ? args[configDirIdx + 1] : null; if (!configDir) { return { data: { error: 'Usage: detect-custom-files --config-dir <path>' } }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/detect-custom-files.ts` around lines 40 - 44, The code currently reads args[configDirIdx + 1] without ensuring a value exists; update the logic around configDirIdx/configDir in detect-custom-files.ts to verify configDirIdx !== -1 AND that args[configDirIdx + 1] is defined and non-empty (e.g., typeof args[configDirIdx + 1] === 'string' && args[configDirIdx + 1].length > 0) before assigning configDir, and return the Usage error when the value is missing or undefined; reference the variables configDirIdx, configDir and the args array when making the change.
7-7: Sync file operations differ from other handlers.This handler uses
readdirSync/readFileSyncwhile most other query handlers use async equivalents. For consistency and to avoid blocking the event loop on large directory trees, consider using async operations.♻️ Async version of walkDir
-import { existsSync, readdirSync, readFileSync } from 'node:fs'; +import { readdir, readFile } from 'node:fs/promises'; +import { existsSync } from 'node:fs'; import { join, relative, resolve } from 'node:path'; -function walkDir(dir: string, baseDir: string): string[] { - const results: string[] = []; +async function walkDir(dir: string, baseDir: string): Promise<string[]> { + const results: string[] = []; if (!existsSync(dir)) return results; - for (const entry of readdirSync(dir, { withFileTypes: true })) { + for (const entry of await readdir(dir, { withFileTypes: true })) { const fullPath = join(dir, entry.name); if (entry.isDirectory()) { - results.push(...walkDir(fullPath, baseDir)); + results.push(...await walkDir(fullPath, baseDir)); } else { const relPath = relative(baseDir, fullPath).split('\\').join('/'); results.push(relPath); } } return results; }And update the caller to await walkDir and use async readFile for the manifest.
Also applies to: 21-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/detect-custom-files.ts` at line 7, The handler currently uses synchronous fs APIs (existsSync, readdirSync, readFileSync) which block the event loop; replace them with async counterparts from 'node:fs/promises' and convert walkDir (and any callers in this file) to async/await: change imports to use fs/promises, make walkDir an async function that uses await readdir and recursion to collect files, replace readFileSync usage when loading the manifest with await readFile, and update any callers to await walkDir and manifest reads so all file ops are non-blocking (look for functions named walkDir and any manifest read logic in this module).
62-74: Distinguish "manifest not found" from "manifest parse error".Both cases return
manifest_found: false, but the parse error case also includes anerrorfield. Consider returningmanifest_found: truewith a separatemanifest_valid: falsefor the parse error case, or at least document this behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/detect-custom-files.ts` around lines 62 - 74, The catch block around JSON.parse(readFileSync(manifestPath, 'utf8')) should distinguish file-not-found from parse errors: change the catch to capture the thrown error (e.g., catch (err)) and if err.code === 'ENOENT' return the existing response with manifest_found: false, but for other errors return manifest_found: true and add a new manifest_valid: false (or set manifest_valid: boolean in the success path) and include the error message in error; update the returned shape (and any types) so callers can tell "manifest present but invalid" vs "manifest missing" and reference manifestPath, manifest_found, manifest_valid, and the error field when making changes.sdk/src/query/validate.ts (1)
752-756: Relative path assumption is fragile.The path
resolve(here, '..', '..', '..', 'agents')assumes this module runs fromsdk/dist/query/orsdk/src/query/. If the SDK is bundled differently or the directory structure changes, this fallback will break silently.Consider adding a validation or logging when the fallback path doesn't exist and
GSD_AGENTS_DIRisn't set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/validate.ts` around lines 752 - 756, The fallback path in getAgentsDirForValidateAgents() is fragile; after resolving the fallback (resolve(here, '..', '..', '..', 'agents')) verify the directory exists and is readable (e.g., fs.existsSync or fs.access) and if it does not, emit a clear warning or error (using the existing logger or console.warn/console.error) that advises setting GSD_AGENTS_DIR or fixing packaging, and return a safe value or throw so callers don't silently proceed with a bad path; ensure references to process.env.GSD_AGENTS_DIR and the resolved path remain in the message for debugging.sdk/src/query/profile-sample.ts (3)
38-39: Falsy check treatslimit: 0andmaxChars: 0as invalid.Using
options.limit || 150means an explicitlimit: 0would be replaced with150. If zero is a valid edge case (skip all), use nullish coalescing. If zero is invalid, consider explicit validation.💡 Suggested fix if zero should be respected
- const limit = options.limit || 150; - const maxChars = options.maxChars || 500; + const limit = options.limit ?? 150; + const maxChars = options.maxChars ?? 500;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/profile-sample.ts` around lines 38 - 39, The current falsy checks replace explicit zeros because of `options.limit || 150` and `options.maxChars || 500`; change these to use nullish coalescing so explicit 0 is respected (e.g., use `options.limit ?? 150` and `options.maxChars ?? 500`), or alternatively add explicit validation in the same function to reject/normalize 0 if zero is not allowed; update the assignments for `limit` and `maxChars` accordingly in the code that defines those variables.
155-157: Silent error swallowing may hide session read failures.The empty
catchblock silently continues whenstreamExtractMessagesfails. While this is defensive, consider logging at debug level or tracking skipped sessions for troubleshooting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/profile-sample.ts` around lines 155 - 157, The empty catch in the loop swallowing errors from streamExtractMessages hides failures; change the catch to capture the exception (e) and emit a debug-level log (e.g., logger.debug or console.debug) with context including the session id or index and the error, and optionally increment or record a skippedSessions/skippedCount variable in the same scope so callers can see how many sessions were skipped; update the catch in the block that calls streamExtractMessages in profile-sample.ts to implement these changes.
169-173: Consider batching writes instead of per-messageappendFileSync.For large sample sizes, calling
appendFileSyncin a loop can be slow. Since messages are already collected inallMessages, a singlewriteFileSyncwith joined content would be more efficient:⚡ Performance improvement
const tmpDir = mkdtempSync(join(tmpdir(), 'gsd-profile-')); const outputPath = join(tmpDir, 'profile-sample.jsonl'); - for (const msg of allMessages) { - appendFileSync(outputPath, JSON.stringify(msg) + '\n'); - } + const content = allMessages.map(msg => JSON.stringify(msg)).join('\n') + (allMessages.length ? '\n' : ''); + writeFileSync(outputPath, content);Note: You'd need to import
writeFileSyncfromnode:fs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/profile-sample.ts` around lines 169 - 173, Replace the per-message synchronous appends with a single write using writeFileSync: instead of looping over allMessages and calling appendFileSync(outputPath, ...), build the joined content from allMessages (e.g., map/JSON.stringify + join('\n') + '\n') and call writeFileSync(outputPath, content). Update imports to include writeFileSync from 'node:fs'. Modify references around tmpDir, outputPath, allMessages and remove the loop that uses appendFileSync.sdk/scripts/gen-profile-questionnaire-data.mjs (1)
13-18: Generator regex patterns are fragile but acceptable for one-off use.The regexes
/const PROFILING_QUESTIONS = (\[[\s\S]*?\]);/and/const CLAUDE_INSTRUCTIONS = (\{[\s\S]*?\n\});/use non-greedy matching which could fail if:
- The source has multiple matching patterns
- Comments contain similar declarations
- The closing delimiter appears in string literals
Since this is a one-off generator (not runtime code), this is acceptable. Consider adding a validation step that re-parses the extracted JSON to catch extraction errors early.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/scripts/gen-profile-questionnaire-data.mjs` around lines 13 - 18, The regex extraction for PROFILING_QUESTIONS and CLAUDE_INSTRUCTIONS (using m1/m2 on cjs) is fragile; after extracting the groups (m1[1] and m2[1]) add a validation step that attempts to parse them as JSON (or JSON5 if appropriate) and throw/log a clear error if parsing fails so extraction errors are caught early; keep the existing regex logic but validate parsed data for PROFILING_QUESTIONS and CLAUDE_INSTRUCTIONS before proceeding.sdk/src/query/verify.ts (1)
602-606: Fragile regex forfiles_modifiedextraction.The regex
/files_modified:\s*\[([^\]]*)\]/doesn't handle:
- Quoted file paths containing
]characters- Multiline YAML arrays
- Nested brackets
This could silently skip files if paths contain special characters. Consider using the existing
extractFrontmatter+ proper YAML parsing for robustness, or document that only simple comma-separated lists are supported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/verify.ts` around lines 602 - 606, Replace the fragile regex-based extraction around fmMatch with a robust frontmatter + YAML parse: use the existing extractFrontmatter(content) to get the frontmatter block, parse it with the project's YAML parser (or js-yaml) to read the files_modified key as an array, then normalize and push entries into allFiles (instead of using fmMatch and splitting on commas). Update the code that references fmMatch/files to read parsedFrontmatter.files_modified (handle string vs array cases) and preserve trimming/filtering logic to avoid duplicates.sdk/src/query/route-next-action.ts (1)
116-143:buildContextis called multiple times with same phase argument.The
buildContextasync function callsfindPhaseinternally. Throughout the handler,findPhaseis called separately (lines 100, 215, 129) andbuildContextis called 9+ times. Several of these calls use the samecurrentPhasevalue.Consider caching the
findPhaseresult for the current phase to avoid redundant disk reads. This is a performance optimization, not a correctness issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/route-next-action.ts` around lines 116 - 143, buildContext currently calls findPhase each time, causing redundant disk reads when invoked multiple times with the same phase; cache the result for the active phase by performing findPhase once for currentPhase and reusing that parsed phase data (or memoize findPhase) when calling buildContext and other places that need phase data (e.g., where findPhase is currently invoked at lines near currentPhase usage). Modify buildContext to accept a pre-fetched phase result (or check a local cache keyed by phase string) and update all callers to pass the cached phase or use the memoized findPhase so repeated calls with the same currentPhase reuse the same fp.data instead of re-reading from disk.sdk/src/query/docs-init.ts (1)
113-159:detectProjectTypereadspackage.jsonmultiple times.The function reads and parses
package.jsonup to 3 times (lines 118, 126, 134). Consider reading once and reusing:♻️ Suggested optimization
export function detectProjectType(cwd: string): Record<string, boolean> { const exists = (rel: string): boolean => pathExistsInternal(cwd, rel); + let pkg: Record<string, unknown> | null = null; + try { + pkg = JSON.parse(readFileSync(join(cwd, 'package.json'), 'utf-8')) as Record<string, unknown>; + } catch { /* no package.json */ } + let has_cli_bin = false; - try { - const pkg = JSON.parse(readFileSync(join(cwd, 'package.json'), 'utf-8')) as Record<string, unknown>; - const bin = pkg.bin; + if (pkg) { + const bin = pkg.bin; has_cli_bin = !!(bin && (typeof bin === 'string' || Object.keys(bin as object).length > 0)); - } catch { /* no package.json */ } + } let is_monorepo = exists('pnpm-workspace.yaml') || exists('lerna.json'); - if (!is_monorepo) { - try { - const pkg = JSON.parse(readFileSync(join(cwd, 'package.json'), 'utf-8')) as Record<string, unknown>; - is_monorepo = Array.isArray(pkg.workspaces) && (pkg.workspaces as unknown[]).length > 0; - } catch { /* ignore */ } + if (!is_monorepo && pkg) { + is_monorepo = Array.isArray(pkg.workspaces) && (pkg.workspaces as unknown[]).length > 0; } // ... similar for has_tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/docs-init.ts` around lines 113 - 159, detectProjectType currently parses package.json multiple times; load and parse package.json once at the top (e.g. const pkg = tryReadJson(join(cwd, 'package.json'))) and reuse that object for determining has_cli_bin (inspect pkg.bin), is_monorepo (inspect pkg.workspaces), and has_tests (inspect pkg.devDependencies) while falling back to the existing pathExistsInternal checks for files; update references to remove the repeated readFileSync/JSON.parse calls and keep existing try/catch behavior so missing/invalid package.json is handled gracefully.sdk/src/query/roadmap-update-plan-progress.ts (1)
108-117: Edge case:planIdmay be empty after replacement.When
summaryFileis exactly'SUMMARY.md', both replacements produce an empty string, but thecontinueon line 110 handles this. However, if a file is named like-SUMMARY.md(with leading dash),planIdbecomes'-'which may not match any checkbox.This is a minor edge case but worth noting for parity validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/roadmap-update-plan-progress.ts` around lines 108 - 117, The current extraction of planId from summaryFile can produce invalid values (e.g., '-' from '-SUMMARY.md'); update the logic that derives planId (used in the loop over summaries and later in planCheckboxPattern/escapeRegex) to strip only the trailing 'SUMMARY.md' suffix (case-insensitive), then trim and remove any leading hyphens/whitespace before using it, and if the resulting planId is empty skip the iteration; make sure roadmapContent replacement still uses the cleaned planId when constructing planCheckboxPattern.sdk/src/query/skill-manifest.ts (1)
166-168: Redundant double-negation.
!!('deprecated' in rootInfo && rootInfo.deprecated)can be simplified sincerootInfo.deprecatedis already boolean when present.♻️ Suggested simplification
- deprecated: !!('deprecated' in rootInfo && rootInfo.deprecated), + deprecated: 'deprecated' in rootInfo && !!rootInfo.deprecated,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/skill-manifest.ts` around lines 166 - 168, The deprecated field construction is using a redundant double-negation pattern; in skill-manifest.ts where the object sets installed: rootInfo.scope !== 'import-only' and deprecated: !!('deprecated' in rootInfo && rootInfo.deprecated), simplify the deprecated assignment to a single, clear boolean coercion (e.g., use Boolean(rootInfo.deprecated) or !!rootInfo.deprecated) or explicitly fall back to false when absent (e.g., 'deprecated' in rootInfo ? rootInfo.deprecated : false) so the deprecated property is a plain boolean derived from rootInfo.deprecated.sdk/src/query/detect-phase-type.ts (1)
98-110:readdirSynccalled on files will throw, caught silently.On line 102,
readdirSync(sub)is called on every entry from the parent directory, butsubmight be a file (not a directory). Thetry/catchhandles this, but the comment says "Not a directory — ignore" which is correct behavior. However, this is inefficient for directories with many files.Consider checking
entry.isDirectory()from the parentreaddirSync({ withFileTypes: true })call:♻️ Suggested optimization
- if (phaseDirFull && dirFiles.length > 0) { + if (phaseDirFull && existsSync(phaseDirFull)) { // Also check subdirectory one level deep (e.g. prisma/schema.prisma) const allRelPaths: string[] = [...dirFiles]; + let dirEntries: Dirent[] = []; + try { + dirEntries = readdirSync(phaseDirFull, { withFileTypes: true }) as Dirent[]; + } catch { /* ignore */ } - for (const f of dirFiles) { + for (const entry of dirEntries) { + if (!entry.isDirectory()) continue; + const f = entry.name.toString(); const sub = join(phaseDirFull, f); - if (existsSync(sub)) { - try { - const subStat = readdirSync(sub); - for (const sf of subStat) { - allRelPaths.push(`${f}/${sf}`); - } - } catch { - // Not a directory — ignore - } + try { + const subStat = readdirSync(sub); + for (const sf of subStat) { + allRelPaths.push(`${f}/${sf}`); + } + } catch { + // read error — ignore } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/detect-phase-type.ts` around lines 98 - 110, The loop currently calls readdirSync(sub) on every entry in dirFiles which can be files and causes thrown errors and inefficiency; change the parent directory read to use readdirSync(phaseDirFull, { withFileTypes: true }) (so dirFiles becomes Dirent objects) and in the loop check dirent.isDirectory() (instead of existsSync/sub try/catch) before building sub and calling readdirSync(sub) to collect child names; update references to dirFiles, sub, and readdirSync accordingly.sdk/src/query/audit-open.ts (2)
648-652: Type assertion foritem.questionsmay fail silently.The cast
(item.questions as string[])assumes the array contains strings, but the data comes fromextractFrontmatterwhich could return mixed types. Consider adding a defensive filter.🛡️ Defensive type handling
- for (const q of (item.questions as string[]) || []) { + for (const q of (item.questions as unknown[] || []).map(String)) { lines.push(` - ${q}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/audit-open.ts` around lines 648 - 652, item.questions is being forcibly cast to string[] which can silently fail; update the iteration in audit-open.ts to defensively validate and coerce questions produced by extractFrontmatter before using them. Replace the direct cast of (item.questions as string[]) with a safe value like: if Array.isArray(item.questions) then create a filtered array using a type guard (e.g., filter((q): q is string => typeof q === 'string')) or map/coerce non-string entries to strings, and iterate over that resulting array; ensure any downstream code (the loop that pushes `- ${q}`) uses this validated array so mixed types are handled safely.
480-513: Redundant try-catch wrapping inauditOpenArtifacts.Each scan function already handles errors internally and returns
[{ scan_error: true }]on failure. The additional IIFE try-catch wrappers (lines 483-513) are redundant since the scan functions don't throw. Consider removing the outer try-catch blocks for cleaner code.♻️ Suggested simplification
export function auditOpenArtifacts(projectDir: string): AuditOpenResult { const planDir = planningPaths(projectDir).planning; - const debugSessions = (() => { - try { return scanDebugSessions(planDir); } catch { return [{ scan_error: true }]; } - })(); + const debugSessions = scanDebugSessions(planDir); - const quickTasks = (() => { - try { return scanQuickTasks(planDir); } catch { return [{ scan_error: true }]; } - })(); + const quickTasks = scanQuickTasks(planDir); // ... similar for other scan functions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/audit-open.ts` around lines 480 - 513, The IIFE try-catch wrappers in auditOpenArtifacts are redundant because each scan function (scanDebugSessions, scanQuickTasks, scanThreads, scanTodos, scanSeeds, scanUatGaps, scanVerificationGaps, scanContextQuestions) already returns [{ scan_error: true }] on failure; remove the outer IIFE+try/catch blocks and directly assign the results (e.g., const debugSessions = scanDebugSessions(planDir);) while keeping the planDir calculation (const planDir = planningPaths(projectDir).planning;) and leaving all scan function calls and their return behavior unchanged.sdk/src/query/profile-output.ts (1)
314-326: Sensitive pattern list provides good coverage but may need updates.The patterns cover common secrets (API keys, tokens, paths) but consider adding patterns for:
- AWS access keys (
AKIA...)- Private keys (
-----BEGIN)This is a good baseline for initial release.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/profile-output.ts` around lines 314 - 326, Update the SENSITIVE_PATTERNS array used in profile-output to also detect AWS access keys and PEM private key headers: add a pattern that matches AWS access key IDs (strings starting with "AKIA" followed by the expected length of alphanumerics) and a pattern that matches PEM private key start markers (e.g., "-----BEGIN ...PRIVATE KEY-----"), ensuring appropriate flags (global and case-insensitive where needed) so these secrets are redacted by the existing scanning logic in SENSITIVE_PATTERNS.sdk/src/query/workstream.ts (1)
248-253: Phase status determination logic may produce unexpected results.The condition
summaries.length >= plans.length && plans.length > 0marks a phase as'complete', but ifplans.length === 0, it falls through to'pending'. However, if there are summaries but no plans (summaries > 0 && plans === 0), this returns'pending'which may be misleading. Consider whether this edge case needs handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/workstream.ts` around lines 248 - 253, The status expression for the phase using variables summaries and plans can mislabel cases where there are summaries but no plans; update the conditional in the status assignment (the "status:" expression that references summaries and plans) to explicitly handle the edge case where plans.length === 0 and summaries.length > 0 — e.g., treat that as 'complete' (or your desired state) by adding a check like (plans.length === 0 && summaries.length > 0) || summaries.length >= plans.length to yield 'complete', otherwise keep the existing checks for 'in_progress' and 'pending'.sdk/src/query/phase-lifecycle.ts (1)
1712-1715: Milestone audit file move could leave orphan if rename fails.If
renamefails afterexistsSyncreturns true (e.g., permission error), the audit file remains in the original location but the milestone may be partially completed. Consider wrapping in try-catch or documenting this behavior.🛡️ Handle rename failure gracefully
const auditFile = join(projectDir, '.planning', `${version}-MILESTONE-AUDIT.md`); - if (existsSync(auditFile)) { - await rename(auditFile, join(archiveDir, `${version}-MILESTONE-AUDIT.md`)); - } + let auditMoved = false; + if (existsSync(auditFile)) { + try { + await rename(auditFile, join(archiveDir, `${version}-MILESTONE-AUDIT.md`)); + auditMoved = true; + } catch { /* audit file move is best-effort */ } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/phase-lifecycle.ts` around lines 1712 - 1715, The rename of auditFile (const auditFile) can fail after existsSync returns true; wrap the await rename(auditFile, join(archiveDir, `${version}-MILESTONE-AUDIT.md`)) in a try-catch inside the function in phase-lifecycle.ts so failures are handled: catch the error, log or surface it (use existing logger or throw a clear error) and optionally attempt a safe fallback (copyFile then unlink as a fallback) to avoid leaving an inconsistent state; ensure the catch references auditFile and the target path and preserves behavior expected by the surrounding milestone completion logic.
davesienkowski
added a commit
to davesienkowski/get-shit-done
that referenced
this pull request
Apr 17, 2026
- readTextArgOrFile: use path.relative containment vs project root (no startsWith bypass) - stateSync: only set highestIncompletePhase when summaries < plans - statePrune: match Performance Metrics rows from stateRecordMetric (| Phase N Pn |) - summary: numeric sort for milestone archives; summaryExtract uses resolvePathUnderProject - verifyKeyLinks: resolve key_link from/to paths under project root
Contributor
Author
|
Track A follow-up: remaining CodeRabbit items are addressed in
Inline review threads are resolved. @coderabbitai please re-review when CI is green. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/src/query/state-mutation.ts (1)
835-869:⚠️ Potential issue | 🟡 MinorReturn reason is inaccurate when blocker section exists but no line matches.
The handler returns “Blockers section not found…” even if the section exists and the text just didn’t match any entry.
Suggested fix
- let removedMatchingLine = false; + let removedMatchingLine = false; + let blockersSectionFound = false; await readModifyWriteStateMd(projectDir, (content) => { const sectionPattern = /(###?\s*(?:Blockers|Blockers\/Concerns|Concerns)\s*\n)([\s\S]*?)(?=\n###?|\n##[^#]|$)/i; const match = content.match(sectionPattern); if (match) { + blockersSectionFound = true; const sectionBody = match[2]; const lines = sectionBody.split('\n'); const filtered = lines.filter(line => { if (!line.startsWith('- ')) return true; const matches = line.toLowerCase().includes(searchText.toLowerCase()); if (matches) removedMatchingLine = true; return !matches; }); if (!removedMatchingLine) { return content; } @@ if (removedMatchingLine) { return { data: { resolved: true, blocker: searchText } }; } - return { data: { resolved: false, reason: 'Blockers section not found in STATE.md' } }; + return { + data: { + resolved: false, + reason: blockersSectionFound + ? 'No matching blocker found' + : 'Blockers section not found in STATE.md', + }, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/state-mutation.ts` around lines 835 - 869, The code currently only tracks removedMatchingLine so when a Blockers section exists but no line matches, the final return incorrectly says "Blockers section not found"; update the logic in readModifyWriteStateMd handling (symbols: readModifyWriteStateMd, sectionPattern, match, removedMatchingLine, searchText) to also track whether the section was found (e.g., set a sectionFound boolean when match is truthy) and after the async operation return a clearer result: if sectionFound is true but removedMatchingLine is false return { data: { resolved: false, reason: `No blocking entry matching "${searchText}" found` } } instead of the current "section not found" message, otherwise keep the existing resolved:true response when removedMatchingLine is true and the current not-found reason when sectionFound is false.
🧹 Nitpick comments (1)
sdk/src/query/profile-extract-messages.ts (1)
24-30: Extract a shared JSONL record type to avoid type drift.The same record shape is duplicated in multiple places; a single alias/interface will keep filter and parser contracts aligned.
♻️ Proposed refactor
+type SessionJsonlRecord = { + type?: string; + userType?: string; + isMeta?: boolean; + isSidechain?: boolean; + message?: { content?: string }; + cwd?: string; + timestamp?: string | number; +}; export function isGenuineUserMessage(record: { - type?: string; - userType?: string; - isMeta?: boolean; - isSidechain?: boolean; - message?: { content?: string }; -}): boolean { + type?: string; + userType?: string; + isMeta?: boolean; + isSidechain?: boolean; + message?: { content?: string }; +}): boolean {-export function isGenuineUserMessage(record: { - type?: string; - userType?: string; - isMeta?: boolean; - isSidechain?: boolean; - message?: { content?: string }; -}): boolean { +export function isGenuineUserMessage(record: SessionJsonlRecord): boolean {- filterFn: (r: { type?: string; userType?: string; isMeta?: boolean; isSidechain?: boolean; message?: { content?: string } }) => boolean, + filterFn: (r: SessionJsonlRecord) => boolean,- let record: { type?: string; userType?: string; isMeta?: boolean; isSidechain?: boolean; message?: { content?: string }; cwd?: string; timestamp?: string | number }; + let record: SessionJsonlRecord;Also applies to: 54-55, 80-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/profile-extract-messages.ts` around lines 24 - 30, Multiple places duplicate the same JSONL record shape (e.g., the inline parameter type in isGenuineUserMessage); extract a single exported interface or type alias (e.g., JsonlRecord or ProfileMessageRecord) that captures { type?: string; userType?: string; isMeta?: boolean; isSidechain?: boolean; message?: { content?: string } } and replace the inline annotations in isGenuineUserMessage and the other occurrences noted (the duplicated types around the other filters/parsers) to import/use this shared type so filter and parser contracts stay aligned.
🤖 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/golden/read-only-parity.integration.test.ts`:
- Around line 93-100: The current catch block falls back to setting both a and b
to the same SHA (variables a and b retrieved via execSync), which weakens the
subsequent verify.commits parity check; instead, do not fabricate identical SHAs
— in the catch for the execSync('git rev-parse HEAD~1', ...) failure either
rethrow the error or explicitly skip/fail the test (e.g., throw new Error or
call the test framework's skip) so the test aborts rather than comparing
identical commits; update the catch to surface the failure and include a clear
message referencing HEAD~1 resolution so verify.commits runs only when a and b
are true distinct commits.
In `@sdk/src/query/check-completion.ts`:
- Around line 87-93: The code uses blocking existsSync/readdirSync and relies on
filesystem order to pick verification/UAT files (phaseDirFull -> readdirSync ->
verFile/uatFile) which makes selection non-deterministic and blocking; replace
the sync checks with async fs.promises methods (or use readdir with a try/catch
instead of existsSync), use await readdir(phaseDirFull) to get files, filter
candidates for "-VERIFICATION.md"/"VERIFICATION.md" and "-UAT.md"/"UAT.md", sort
each candidate list deterministically (e.g., alphabetical) and pick the first
result to set verificationContent/uatContent by awaiting
readFileSafe(join(phaseDirFull, chosenFile)), and ensure errors are caught so
missing dirs/files don’t throw.
In `@sdk/src/query/state-mutation.ts`:
- Around line 912-916: The handler that constructs the "planned-phase" state
currently allows a missing --phase and accepts invalid --plans, causing "Phase
null" and NaN output; update the code around
parseNamedArgs/parsed/phaseNumber/plansRaw/planCount to: validate that
parsed.phase is non-null and non-empty and return or throw a clear user error if
absent, and sanitize plansRaw by attempting parseInt and treating any
non-finite/NaN or negative result as null (or a safe default) before building
the activity text; apply the same validation/sanitization to the other identical
block that uses parsed/phaseNumber/plansRaw/planCount (the one around lines
940-951).
- Around line 400-417: The parse/positional fallback logic in parseNamedArgs
usage allows positional args to override or be misread when named flags are
present and also converts non-numeric plans into NaN; update the logic in
state.begin-phase parsing: use parseNamedArgs result (named.phase, named.name,
named.plans) as authoritative and only attempt positional fallback for
phaseNumber/phaseName/plansStr when the corresponding named value is strictly
null/undefined (not just empty string), ensuring you check the correct
positional index relative to missing named values (do not unconditionally read
args[1] if named.phase exists), and sanitize plansStr before computing planNum
by testing numeric validity (e.g., Number.isInteger(parseInt(...)) or using
isNaN) so non-numeric values produce null instead of NaN; apply the same fixes
to the duplicate block around the other occurrence (the block near the later
reference).
In `@sdk/src/query/summary.ts`:
- Around line 184-189: The currentDirs array is being sorted with default
lexical .sort(), causing multi-digit phase directories to order incorrectly;
change its sort call to use the existing phase comparator (comparePhaseNum) so
phase directories are sorted numerically like the archive path. Locate the
currentDirs definition in summary.ts (the
readdirSync(...).filter(...).map(...).sort() chain) and replace .sort() with
.sort(comparePhaseNum) so historyDigest() processes phases in phase-aware order.
- Around line 202-205: The summaries array from readdirSync is unsorted causing
non-deterministic JSON order; before iterating in historyDigest (where summaries
is defined), sort the summaries array (e.g., use summaries.sort with
localeCompare and numeric:true) so that plain "SUMMARY.md" and numbered
"*-SUMMARY.md" files are folded in a stable, numeric-aware order; update the
code that defines summaries (the const summaries =
readdirSync(dirPath).filter(...)) to sort the result prior to the for (const
summary of summaries) loop.
- Around line 136-143: The frontmatter fields used when building fullResult
(specifically 'key-files', 'patterns-established', and 'requirements-completed')
are only cast and may be scalar; normalize them to arrays in the fullResult
construction: for each of fm['key-files'], fm['patterns-established'], and
fm['requirements-completed'] detect if the value is undefined/null -> return [],
if Array -> return as-is, otherwise wrap the scalar/object into a single-element
array; update the assignment in the fullResult object to perform this
normalization so those keys always produce an array.
---
Outside diff comments:
In `@sdk/src/query/state-mutation.ts`:
- Around line 835-869: The code currently only tracks removedMatchingLine so
when a Blockers section exists but no line matches, the final return incorrectly
says "Blockers section not found"; update the logic in readModifyWriteStateMd
handling (symbols: readModifyWriteStateMd, sectionPattern, match,
removedMatchingLine, searchText) to also track whether the section was found
(e.g., set a sectionFound boolean when match is truthy) and after the async
operation return a clearer result: if sectionFound is true but
removedMatchingLine is false return { data: { resolved: false, reason: `No
blocking entry matching "${searchText}" found` } } instead of the current
"section not found" message, otherwise keep the existing resolved:true response
when removedMatchingLine is true and the current not-found reason when
sectionFound is false.
---
Nitpick comments:
In `@sdk/src/query/profile-extract-messages.ts`:
- Around line 24-30: Multiple places duplicate the same JSONL record shape
(e.g., the inline parameter type in isGenuineUserMessage); extract a single
exported interface or type alias (e.g., JsonlRecord or ProfileMessageRecord)
that captures { type?: string; userType?: string; isMeta?: boolean;
isSidechain?: boolean; message?: { content?: string } } and replace the inline
annotations in isGenuineUserMessage and the other occurrences noted (the
duplicated types around the other filters/parsers) to import/use this shared
type so filter and parser contracts stay aligned.
🪄 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: 5ee254b8-f407-4271-8b73-9883e59bf398
📒 Files selected for processing (15)
sdk/src/cli.tssdk/src/golden/read-only-golden-rows.tssdk/src/golden/read-only-parity.integration.test.tssdk/src/query/check-completion.tssdk/src/query/commit.tssdk/src/query/config-gates.test.tssdk/src/query/config-mutation.tssdk/src/query/config-query.tssdk/src/query/frontmatter-mutation.tssdk/src/query/intel.tssdk/src/query/profile-extract-messages.tssdk/src/query/state-mutation.tssdk/src/query/state.tssdk/src/query/summary.tssdk/src/query/validate.ts
✅ Files skipped from review due to trivial changes (1)
- sdk/src/golden/read-only-golden-rows.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- sdk/src/query/state.ts
- sdk/src/query/config-query.ts
- sdk/src/query/config-mutation.ts
- sdk/src/query/config-gates.test.ts
- sdk/src/query/frontmatter-mutation.ts
- sdk/src/query/commit.ts
- sdk/src/query/validate.ts
davesienkowski
added a commit
to davesienkowski/get-shit-done
that referenced
this pull request
Apr 17, 2026
- Golden verify.commits: require two distinct SHAs via git rev-list (no identical fallback) - check.completion: async readdir, sorted filenames, deterministic VERIFICATION/UAT pick - state.begin-phase: positional argv only when args[0] is not a flag; sanitize plans NaN - state.planned-phase: sanitize plans NaN; safe phase label in activity strings - summary: coerce list frontmatter to arrays; comparePhaseNum for current phases; sort summary files - Tests: stateBeginPhase named-args regression
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/src/query/state-mutation.ts (1)
835-868:⚠️ Potential issue | 🟡 MinorReturn the correct miss reason when no blocker matches.
removedMatchingLineis false both when the section is missing and when the section exists but the requested blocker text is absent, so Line 868 reports the wrong reason for a normal miss. Track section presence separately to avoid misleading callers.Suggested fix
- let removedMatchingLine = false; + let removedMatchingLine = false; + let foundBlockersSection = false; ... const match = content.match(sectionPattern); if (match) { + foundBlockersSection = true; const sectionBody = match[2]; const lines = sectionBody.split('\n'); ... - return { data: { resolved: false, reason: 'Blockers section not found in STATE.md' } }; + return { + data: { + resolved: false, + reason: foundBlockersSection + ? 'Blocker text not found in STATE.md' + : 'Blockers section not found in STATE.md', + }, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/state-mutation.ts` around lines 835 - 868, The code currently uses removedMatchingLine to decide the miss reason, which conflates "section missing" and "blocker not found"; modify readModifyWriteStateMd usage in state-mutation.ts so you track section presence separately (e.g., introduce a sectionFound boolean set to true when match is truthy inside the callback) while kept removedMatchingLine only for whether a line was removed, then after the await return { data: { resolved: removedMatchingLine, blocker: searchText }} when removedMatchingLine is true, otherwise return a miss with reason set to "Blockers section not found in STATE.md" if sectionFound is false or "Blocker not found in STATE.md" (or similar) if sectionFound is true; use the existing identifiers removedMatchingLine, readModifyWriteStateMd, sectionPattern and searchText to locate and implement this change.
♻️ Duplicate comments (3)
sdk/src/query/state-mutation.ts (3)
125-132:⚠️ Potential issue | 🟠 MajorThe project-root check still allows symlink escapes.
Line 128 only constrains the lexical path.
readFileSync()on Line 132 will still follow a symlink that lives under the repo but points outside it, so this helper can read arbitrary files outsideprojectDir. Resolve and validate the real path, or reject symlinks before reading.Suggested fix
-import { - constants, unlinkSync, existsSync, mkdirSync, writeFileSync, readdirSync, readFileSync, -} from 'node:fs'; +import { + constants, unlinkSync, existsSync, mkdirSync, writeFileSync, readdirSync, readFileSync, realpathSync, +} from 'node:fs'; ... const root = resolve(projectDir); - const resolved = isAbsolute(filePath) ? resolve(filePath) : resolve(root, filePath); - const rel = relative(root, resolved); + const candidate = isAbsolute(filePath) ? resolve(filePath) : resolve(root, filePath); + const resolved = realpathSync(candidate); + const realRoot = realpathSync(root); + const rel = relative(realRoot, resolved); if (rel.startsWith('..') || isAbsolute(rel)) { throw new Error(`${label} path rejected: outside project directory`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/state-mutation.ts` around lines 125 - 132, The current path check uses lexical resolves (resolve/isAbsolute/relative) which can be bypassed by symlinks; before calling readFileSync(resolved, ...) validate the real filesystem location by calling realpathSync (or lstat to detect symlinks) on the resolved path and ensure the real path starts with the realpath of projectDir (or reject if any path component is a symlink), then proceed to readFileSync only after that verification; update the helper that uses resolve, isAbsolute, relative, readFileSync, projectDir, filePath, and label to perform this real-path/symlink check and throw the same styled Error if validation fails.
912-919:⚠️ Potential issue | 🟠 Major
state.planned-phasestill succeeds without a phase.Lines 917-918 normalize a missing
--phaseto'?', and Lines 928-950 still mark STATE as ready to execute. That mutates the file even though the target phase is unknown. This should fail fast before any write.Suggested fix
const parsed = parseNamedArgs(args, ['phase', 'name', 'plans']); const phaseNumber = parsed.phase as string | null; + if (!phaseNumber || String(phaseNumber).trim() === '') { + return { data: { error: 'phase required' } }; + } const plansRaw = parsed.plans as string | null; const plansParsed = plansRaw !== null && plansRaw !== '' ? parseInt(String(plansRaw), 10) : NaN; const planCount = Number.isFinite(plansParsed) && !Number.isNaN(plansParsed) ? plansParsed : null; const phaseLabel = phaseNumber != null && String(phaseNumber).trim() !== '' ? String(phaseNumber).trim() : '?';Also applies to: 928-954
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/state-mutation.ts` around lines 912 - 919, The code currently normalizes a missing phase to '?' (using parseNamedArgs -> phaseNumber -> phaseLabel) which allows state.planned-phase to proceed; instead validate parsed.phase (phaseNumber) immediately after parseNamedArgs and fail fast by throwing or returning an error when phaseNumber is null/empty/whitespace so no mutation occurs; update the logic that uses phaseLabel and planCount (references: parseNamedArgs, phaseNumber, phaseLabel, plansParsed, planCount) to assume a present phase only after this early validation and ensure any code paths that mark STATE as ready-to-execute are gated by this check.
414-416:⚠️ Potential issue | 🟠 MajorReject negative plan counts in both phase handlers.
parseInt('-3', 10)passes the current finite/NaN check, so both handlers can write impossible values likeTotal Plans in Phase: -3andPlan: 1 of -3. That will poison downstream progress/sync logic. Treat anything below 1 as invalid.Suggested fix
- const planNum = Number.isFinite(plansParsed) && !Number.isNaN(plansParsed) ? plansParsed : null; + const planNum = Number.isInteger(plansParsed) && plansParsed > 0 ? plansParsed : null; ... - const planCount = Number.isFinite(plansParsed) && !Number.isNaN(plansParsed) ? plansParsed : null; + const planCount = Number.isInteger(plansParsed) && plansParsed > 0 ? plansParsed : null;Also applies to: 915-916
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/state-mutation.ts` around lines 414 - 416, The parsed plan count currently accepts negative integers; update the validation around plansParsed and planNum so any parsed value less than 1 is treated as invalid (null). Concretely, after computing plansParsed (from parseInt) change the planNum assignment to require plansParsed >= 1 (e.g. planNum = Number.isFinite(plansParsed) && !Number.isNaN(plansParsed) && plansParsed >= 1 ? plansParsed : null), and apply the same guard to the other identical occurrence of plansParsed/planNum in the file used by the second phase handler.
🧹 Nitpick comments (1)
sdk/src/golden/read-only-parity.integration.test.ts (1)
16-23: Optional: factor repeated registry-dispatch pattern into a helper.The repeated
createRegistry()+dispatch()setup is consistent but verbose; a small helper would reduce duplication and make future row additions easier.♻️ Suggested refactor
+async function dispatchData(command: string, args: string[] = []): Promise<unknown> { + const registry = createRegistry(); + const result = await registry.dispatch(command, args, REPO_ROOT); + return result.data; +} ... - const registry = createRegistry(); - const sdkResult = await registry.dispatch(row.canonical, row.sdkArgs, REPO_ROOT); - expect(sdkResult.data).toEqual(gsdOutput); + const sdkData = await dispatchData(row.canonical, row.sdkArgs); + expect(sdkData).toEqual(gsdOutput);Also applies to: 25-33, 36-47, 50-61, 64-70, 73-86, 100-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/golden/read-only-parity.integration.test.ts` around lines 16 - 23, Factor the repeated createRegistry() + dispatch(...) pattern into a small test helper (e.g., dispatchWithRegistry or runDispatch) that accepts the canonical id, sdkArgs and repo root and returns the dispatched result (or result.data), then replace each test block's createRegistry() + registry.dispatch(...) sequence (references: createRegistry(), registry.dispatch(...), and captureGsdToolsOutput(...) where applicable) with calls to that helper to reduce duplication and make adding rows simpler.
🤖 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/state-mutation.ts`:
- Around line 1327-1341: The parsed keep-recent value can be negative, which
will invert the cutoff calculation; validate and clamp it before use: after
parseNamedArgs and before computing cutoff, coerce the parsed['keep-recent']
into a non-negative integer (e.g., Math.max(0, parsedValue) or throw a
validation error) and assign that to keepRecent so the cutoff = currentPhase -
keepRecent cannot unexpectedly increase the prune target; update references
around parseNamedArgs, keepRecent, currentPhase, and cutoff to use the
validated/clamped value.
---
Outside diff comments:
In `@sdk/src/query/state-mutation.ts`:
- Around line 835-868: The code currently uses removedMatchingLine to decide the
miss reason, which conflates "section missing" and "blocker not found"; modify
readModifyWriteStateMd usage in state-mutation.ts so you track section presence
separately (e.g., introduce a sectionFound boolean set to true when match is
truthy inside the callback) while kept removedMatchingLine only for whether a
line was removed, then after the await return { data: { resolved:
removedMatchingLine, blocker: searchText }} when removedMatchingLine is true,
otherwise return a miss with reason set to "Blockers section not found in
STATE.md" if sectionFound is false or "Blocker not found in STATE.md" (or
similar) if sectionFound is true; use the existing identifiers
removedMatchingLine, readModifyWriteStateMd, sectionPattern and searchText to
locate and implement this change.
---
Duplicate comments:
In `@sdk/src/query/state-mutation.ts`:
- Around line 125-132: The current path check uses lexical resolves
(resolve/isAbsolute/relative) which can be bypassed by symlinks; before calling
readFileSync(resolved, ...) validate the real filesystem location by calling
realpathSync (or lstat to detect symlinks) on the resolved path and ensure the
real path starts with the realpath of projectDir (or reject if any path
component is a symlink), then proceed to readFileSync only after that
verification; update the helper that uses resolve, isAbsolute, relative,
readFileSync, projectDir, filePath, and label to perform this real-path/symlink
check and throw the same styled Error if validation fails.
- Around line 912-919: The code currently normalizes a missing phase to '?'
(using parseNamedArgs -> phaseNumber -> phaseLabel) which allows
state.planned-phase to proceed; instead validate parsed.phase (phaseNumber)
immediately after parseNamedArgs and fail fast by throwing or returning an error
when phaseNumber is null/empty/whitespace so no mutation occurs; update the
logic that uses phaseLabel and planCount (references: parseNamedArgs,
phaseNumber, phaseLabel, plansParsed, planCount) to assume a present phase only
after this early validation and ensure any code paths that mark STATE as
ready-to-execute are gated by this check.
- Around line 414-416: The parsed plan count currently accepts negative
integers; update the validation around plansParsed and planNum so any parsed
value less than 1 is treated as invalid (null). Concretely, after computing
plansParsed (from parseInt) change the planNum assignment to require plansParsed
>= 1 (e.g. planNum = Number.isFinite(plansParsed) && !Number.isNaN(plansParsed)
&& plansParsed >= 1 ? plansParsed : null), and apply the same guard to the other
identical occurrence of plansParsed/planNum in the file used by the second phase
handler.
---
Nitpick comments:
In `@sdk/src/golden/read-only-parity.integration.test.ts`:
- Around line 16-23: Factor the repeated createRegistry() + dispatch(...)
pattern into a small test helper (e.g., dispatchWithRegistry or runDispatch)
that accepts the canonical id, sdkArgs and repo root and returns the dispatched
result (or result.data), then replace each test block's createRegistry() +
registry.dispatch(...) sequence (references: createRegistry(),
registry.dispatch(...), and captureGsdToolsOutput(...) where applicable) with
calls to that helper to reduce duplication and make adding rows simpler.
🪄 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: 226166e3-f6d1-4a4b-8f26-a5a9a35bcff0
📒 Files selected for processing (5)
sdk/src/golden/read-only-parity.integration.test.tssdk/src/query/check-completion.tssdk/src/query/state-mutation.test.tssdk/src/query/state-mutation.tssdk/src/query/summary.ts
✅ Files skipped from review due to trivial changes (1)
- sdk/src/query/summary.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/src/query/state-mutation.test.ts
davesienkowski
added a commit
to davesienkowski/get-shit-done
that referenced
this pull request
Apr 17, 2026
- check-completion: async readdir with sorted names; prefer VERIFICATION.md/UAT.md - statePlannedPhase: require --phase; sanitize plan count; update stub test - statePrune: reject negative/non-integer keep-recent - read-only-parity: verify.commits uses rev-list (two distinct SHAs) Refs: CodeRabbit PR gsd-build#2341 (open threads)
trek-e
reviewed
Apr 17, 2026
Collaborator
trek-e
left a comment
trek-e
left a comment
There was a problem hiding this comment.
Adversarial review.
Large SDK PR adding the golden parity harness and query handler CJS alignment. 23 CodeRabbit findings — 20 confirmed addressed in commits, 3 unaddressed (posted validation replies on those threads).
Unaddressed issues I validated:
- check-completion.ts (3098218930) —
readdirSyncwithout sort produces non-deterministic file selection for verification/UAT status. Real bug. - state-mutation.ts (3098218948) —
state.planned-phasemissing--phasevalidation writes "Phase null" to STATE.md. Real bug. - state-mutation.ts (3098265474) —
--keep-recent -1passes the|| 3guard and can archive the current phase. Real bug.
These three should be fixed before merge.
Additional finding not caught by CodeRabbit:
sdk/src/golden/read-only-golden-rows.ts — readOnlyGoldenCanonicals() notes from CodeRabbit (id 3097721634) add state.load to the set, but the parity test file (id 3097721638) uses the state.load name while actually testing state.json semantics (comparing against gsd-tools state json). These two issues interact: adding state.load to canonicals without a real state.load handler means coverage accounting passes but the semantic test is wrong. Both commits were marked "Addressed" — worth double-checking the final state of both files together.
davesienkowski
added a commit
to davesienkowski/get-shit-done
that referenced
this pull request
Apr 17, 2026
- statePrune: validate --keep-recent is a non-negative integer before use (negative values previously bypassed the `|| 3` guard) - statePlannedPhase: require --phase arg; return error when missing instead of writing "Phase null ..." to STATE.md; simplify planCount derivation to match CodeRabbit suggestion - stateResolveBlocker: track blockersSectionFound separately from removedMatchingLine so the failure reason correctly distinguishes "section not found" from "blocker text not found in section"
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/state-mutation.ts`:
- Around line 414-416: plansParsed/planNum currently accept zero and negative
integers as valid plan counts; change the validation so plansParsed is only
considered usable when it's a positive integer (> 0). Specifically, update the
logic that computes planNum (and the analogous logic at the other occurrence
around the plan parsing at lines 920-921) to check Number.isFinite(plansParsed)
&& !Number.isNaN(plansParsed) && plansParsed > 0 and otherwise set planNum to
null so non-positive values are rejected before rendering STATE.md.
- Around line 988-1018: stateSignalWaiting currently uses waitingDir(projectDir)
(which prefers .gsd) to compute waitingPath and writes WAITING.json there, but
the current reader still reads .planning/WAITING.json; update stateSignalWaiting
so it writes the WAITING.json to both locations (join(projectDir, '.gsd',
'WAITING.json') and join(projectDir, '.planning', 'WAITING.json')) instead of
only using waitingPath, ensuring mkdirSync is called for both dirs and both
files are written (preserve the same JSON content and encoding) so the signal is
visible to the existing reader while readers are aligned.
- Around line 1337-1341: The code currently uses "parsedKeepRecent || 3" which
converts a validated 0 into 3; update the assignment for keepRecent so an
explicit zero is preserved (e.g., replace the falsy-or default with a
nullish/default check). Locate parsedKeepRecent and keepRecent in this block
(the variables named parsedKeepRecent and keepRecent in state-mutation.ts) and
set keepRecent to parsedKeepRecent when it is present (including 0) and only
default to 3 when parsedKeepRecent is absent/undefined (use a nullish coalescing
or equivalent check).
🪄 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: fe662821-c8b7-46c4-86cd-b57eb50ef946
📒 Files selected for processing (1)
sdk/src/query/state-mutation.ts
18 tasks
davesienkowski
added a commit
to davesienkowski/get-shit-done
that referenced
this pull request
Apr 17, 2026
Single squashed SDK tree vs upstream/main: gsd-sdk query handlers aligned with gsd-tools.cjs, golden integration/read-only parity, decision-routing queries, profile-output, and registry canonical policy. Tests: sdk tsc + vitest unit. Intentionally excludes repo-root docs, .planning, and merged Track B/C-only paths.
davesienkowski
force-pushed
the
feat/2302-track-a-golden-v2
branch
from
964d142 to
a4c2e2a
Compare
davesienkowski
added a commit
to davesienkowski/get-shit-done
that referenced
this pull request
Apr 17, 2026
…anning - state.begin-phase / state.planned-phase: only treat numeric --plans as valid when > 0 - state.signal-waiting: write WAITING.json to both .gsd and .planning for reader parity Refs gsd-build#2341
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
davesienkowski
added a commit
to davesienkowski/get-shit-done
that referenced
this pull request
Apr 17, 2026
…-build#2302 Track A) Golden/read-only parity tests and registry alignment, query handler fixes (check-completion, state-mutation, commit, validate, summary, etc.), and WAITING.json dual-write for .gsd/.planning readers. Refs gsd-build#2341
davesienkowski
force-pushed
the
feat/2302-track-a-golden-v2
branch
from
f34ffef to
def6d7d
Compare
Contributor
Author
Branch update (2026-04-18)Synced \ eat/2302-track-a-golden-v2\ to *\ 28b788* on the fork. GitHub should show the new commits on this PR automatically. Latest commit highlights
Note on root \pm test\ (11 failures locally) Ready for re-review when maintainers have cycles. |
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
sdk/src/query/intel.ts (1)
346-350:⚠️ Potential issue | 🔴 CriticalPath traversal vulnerability in file-writing handler — critical severity.
Both
intelExtractExportsandintelPatchMetause the same unsafe pattern:resolve(projectDir, args[0])without path containment validation. An attacker-controlled path can escape the project directory. However,intelPatchMeta(line 359) writes to files withwriteFileSync, making it significantly more severe—arbitrary files can be overwritten or created.🛡️ Proposed fix to add path containment check
export const intelPatchMeta: QueryHandler = async (args, projectDir) => { const filePath = args[0] ? resolve(projectDir, args[0]) : ''; + const normalizedProjectDir = resolve(projectDir); + if (filePath && !filePath.startsWith(normalizedProjectDir + '/') && filePath !== normalizedProjectDir) { + return { data: { patched: false, error: 'Path must be within project directory' } }; + } if (!filePath || !existsSync(filePath)) { return { data: { patched: false, error: `File not found: ${filePath}` } }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/intel.ts` around lines 346 - 350, intelPatchMeta (and similarly intelExtractExports) currently resolves attacker-controlled args[0] with resolve(projectDir, args[0]) and can escape projectDir leading to arbitrary file writes; fix by validating path containment after resolving: compute the absolute target with path.resolve(projectDir, args[0]) and ensure path.relative(projectDir, target) does not start with '..' and is not an absolute escape, returning an error if the check fails; apply this check before any existsSync or writeFileSync operations in intelPatchMeta (and mirror the same containment guard in intelExtractExports) so only files inside projectDir may be read or written.sdk/src/query/frontmatter.ts (1)
113-133:⚠️ Potential issue | 🟠 MajorArray-of-object YAML items are parsed incorrectly as strings.
- key: valueentries are currently pushed as string values, which breaks structured frontmatter arrays (e.g., object items underuser_setup/ similar blocks).🐛 Proposed fix direction
- } else if (line.trim().startsWith('- ')) { - // Array item - const itemValue = line.trim().slice(2).replace(/^["']|["']$/g, ''); + } else if (line.trim().startsWith('- ')) { + // Array item (string or inline object: "- key: value") + const rawItem = line.trim().slice(2); + const kvItem = rawItem.match(/^([a-zA-Z0-9_-]+):\s*(.*)$/); + const parsedItem: unknown = kvItem + ? { [kvItem[1]]: kvItem[2].replace(/^["']|["']$/g, '') } + : rawItem.replace(/^["']|["']$/g, ''); // If current context is an empty object, convert to array if (typeof current.obj === 'object' && !Array.isArray(current.obj) && Object.keys(current.obj).length === 0) { // Find the key in parent that points to this object and convert it const parent = stack.length > 1 ? stack[stack.length - 2] : null; if (parent && !Array.isArray(parent.obj)) { for (const k of Object.keys(parent.obj as Record<string, unknown>)) { if ((parent.obj as Record<string, unknown>)[k] === current.obj) { - (parent.obj as Record<string, unknown>)[k] = [itemValue]; + (parent.obj as Record<string, unknown>)[k] = [parsedItem]; current.obj = (parent.obj as Record<string, unknown>)[k] as unknown[]; break; } } } } else if (Array.isArray(current.obj)) { - current.obj.push(itemValue); + current.obj.push(parsedItem); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/frontmatter.ts` around lines 113 - 133, The array-item branch treating every "- ..." as a string is wrong for object-style entries like "- key: value"; update the block in frontmatter.ts that handles lines starting with '- ' (the code referencing current.obj, stack, parent and itemValue) to detect a "key: value" pattern (e.g. match /^([^:]+):\s*(.*)$/) and construct an object { key: value } instead of a plain string; when converting an empty object to an array (the parent conversion logic that assigns (parent.obj as Record<string, unknown>)[k] = [itemValue]) ensure you push the parsed object form (not the raw itemValue) and, when current.obj is already an array, push the parsed object for object-like items and the string for scalar items so arrays of objects (e.g., user_setup entries) are preserved.sdk/src/query/commit.ts (1)
251-280:⚠️ Potential issue | 🟠 MajorScope the commit to the requested files before calling
git commit.After
--filesbecame mandatory, this handler still commits whatever is already staged in the index. Ifgit addfails/no-ops, or the caller already has unrelated staged changes,git commitcan succeed with the wrong payload. Check thegit addexit status and reject when the cached diff contains paths outside the requested file set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/commit.ts` around lines 251 - 280, The commit path currently calls spawnSync('git', ['-C', projectDir, 'add', ...fileArgs]) and then runs git commit without verifying add succeeded or that the index only contains the requested files; update the logic in the handler around resolvePathUnderProject, spawnSync, fileArgs and commitResult to first check the exit status of the git add call and return {committed:false,...} including add stderr on non-zero exit, then run git diff --cached --name-only (spawnSync) and compare the returned staged file list to the requested files set (or to ['.'] when files was empty) and reject the commit with a clear reason if any staged path is outside the requested set before calling git commit. Ensure sanitized (from sanitizeCommitMessage) is still used for the commit message.sdk/src/query/roadmap.ts (1)
73-131:⚠️ Potential issue | 🟠 Major
🟡current-milestone support is only half wired.
getMilestoneInfo()now recognizes🟡, butextractCurrentMilestone()still only falls back to🚧. On ROADMAPs using the🟡marker, callers likeroadmap.analyze,statsJson(), andprogresskeep operating on the full file instead of the active milestone slice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/roadmap.ts` around lines 73 - 131, getMilestoneInfo recognizes the 🟡 active-marker but extractCurrentMilestone still only looks for 🚧, so callers (roadmap.analyze, statsJson, progress) process the whole file instead of the active milestone slice; update extractCurrentMilestone to treat 🟡 the same as 🚧 (e.g., accept either emoji in its detection/regex and selection logic) so it returns the same milestone slice for both markers and downstream functions use the active milestone correctly.
🟡 Minor comments (10)
sdk/src/query/workstream.ts-352-354 (1)
352-354:⚠️ Potential issue | 🟡 MinorRemove unused variable
root.The
rootvariable is declared but never used in this function.🧹 Proposed fix
export const workstreamProgress: QueryHandler = async (_args, projectDir) => { - const root = planningRoot(projectDir); const wsRoot = workstreamsDir(projectDir);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/workstream.ts` around lines 352 - 354, The variable `root` is declared but unused in the `workstreamProgress` function; remove the unused declaration (the line that calls `planningRoot(projectDir)`) so only `wsRoot = workstreamsDir(projectDir)` remains, and update any related imports if `planningRoot` becomes unused elsewhere in the file.sdk/src/query/commit.ts-105-110 (1)
105-110:⚠️ Potential issue | 🟡 MinorPreserve literal
--footokens in commit messages.Both parsers drop every pre-
--filestoken that starts with--, so a message likedocs: document --help handlingis silently rewritten before it reaches git. Parse the known flags separately instead of filtering all--*tokens out of the message body.Also applies to: 227-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/commit.ts` around lines 105 - 110, The current parsing uses messageArgs = allArgs.slice(0, endIndex).filter(a => !a.startsWith('--')) which strips any token beginning with "--" (e.g. "--help") from commit messages; change the logic to preserve literal "--foo" tokens by parsing known flags explicitly instead of blanket-filtering. Specifically, in the block that computes filesIndex, endIndex, messageArgs, message and filePaths (variables/filesIndex, endIndex, messageArgs, message, filePaths, allArgs), remove the .filter(a => !a.startsWith('--')) behavior and instead: scan allArgs for recognized flags (e.g. "--files" and any other supported flags), separate their values into filePaths or flag variables, then build messageArgs from the remaining tokens exactly as-is so that tokens beginning with "--" that are not recognized flags remain in the message. Ensure the same change is applied to the other occurrence noted (lines ~227-230).sdk/src/query/phase-list-queries.ts-126-143 (1)
126-143:⚠️ Potential issue | 🟡 MinorDon’t return an empty
idforPLAN.md.
'PLAN.md'.replace('-PLAN.md', '').replace('PLAN.md', '')becomes'', so the base plan entry has no identifier even though this handler explicitly listsPLAN.md. Special-case the root file or derive its id from the phase instead of returning an empty string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/phase-list-queries.ts` around lines 126 - 143, The current planId calculation in the loop over planFiles (the planId variable created from planFile.replace(...)) can produce an empty string for the root PLAN.md; change the logic in the loop that computes planId (before the plans.push) to special-case 'PLAN.md' (or files that equal 'PLAN.md' ignoring case) and assign a meaningful id (for example derive it from the phase name or directory) instead of ''. Update the code that builds the plans entry (the object passed to plans.push) to use this non-empty id so the base plan has a stable identifier.bin/install.js-80-85 (1)
80-85:⚠️ Potential issue | 🟡 MinorAdd the new SDK flags to
--help.
--sdkand--no-sdkare parsed here, but the usage text never mentions them, so users have no discoverable way to force or skip the new default SDK bootstrap from the CLI itself.📝 Suggested help text update
${cyan}--portable-hooks${reset} Emit \$HOME-relative hook paths in settings.json (for WSL/Docker bind-mount setups; also GSD_PORTABLE_HOOKS=1) + ${cyan}--sdk${reset} Force rebuild/reinstall of gsd-sdk from sdk/ + ${cyan}--no-sdk${reset} Skip automatic gsd-sdk installation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/install.js` around lines 80 - 85, The CLI parses new flags hasSdk and hasNoSdk in bin/install.js but the usage/help output doesn't mention them; update the help/usage text generation (where the CLI prints help for install) to include descriptions for --sdk and --no-sdk so users can discover forcing or skipping the SDK bootstrap. Locate the help string or function that prints usage for the install command and add concise entries explaining --sdk (force SDK bootstrap) and --no-sdk (skip default SDK bootstrap), keeping styling consistent with existing option lines.sdk/src/cli.ts-268-277 (1)
268-277:⚠️ Potential issue | 🟡 Minor
@file:indirection path is not validated for traversal or containment.The
parseCliQueryJsonOutputfunction reads from@file:paths without validating that the path is contained within the project directory. While this processes CJS output (not user input directly), a malicious or buggy CJS handler could return@file:/etc/passwdand have it read.Consider adding path containment validation similar to what PR comments indicate was done for
readTextArgOrFile.🛡️ Suggested fix
async function parseCliQueryJsonOutput(raw: string): Promise<unknown> { const trimmed = raw.trim(); if (trimmed === '') return null; let jsonStr = trimmed; if (jsonStr.startsWith('@file:')) { const filePath = jsonStr.slice(6).trim(); + // Basic traversal check - CJS output should not reference paths outside cwd + if (filePath.includes('..') || filePath.startsWith('/')) { + throw new Error(`Invalid `@file`: path in CJS output: ${filePath}`); + } jsonStr = await readFile(filePath, 'utf-8'); } return JSON.parse(jsonStr); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/cli.ts` around lines 268 - 277, The parseCliQueryJsonOutput function reads files referenced via the `@file`: indirection without validating the path; update parseCliQueryJsonOutput to resolve and canonicalize the extracted filePath (using path.resolve and/or fs.realpath) and ensure it is contained within the allowed project root (use the same projectRoot logic as readTextArgOrFile), throwing an error if the resolved path is outside that directory before calling readFile; keep the existing behavior for normal JSON strings and reuse readTextArgOrFile’s containment checks or helper if available to avoid duplication.sdk/src/query/profile.ts-206-223 (1)
206-223:⚠️ Potential issue | 🟡 MinorEdge case:
--sessionwithout a value results inundefinedbeing passed.If
--sessionis the last argument (e.g.,['project', '--session']),args[sessionIdx + 1]isundefined. The non-null assertion!doesn't prevent this, andrunExtractMessagesreceivesnull(orundefined). Consider validating that the flag has a value.🛡️ Suggested validation
const sessionIdx = args.indexOf('--session') !== -1 ? args.indexOf('--session') : args.indexOf('--session-id'); - const sessionId = sessionIdx !== -1 ? args[sessionIdx + 1]! : null; + const sessionId = sessionIdx !== -1 && args[sessionIdx + 1] ? args[sessionIdx + 1] : null; const limitIdx = args.indexOf('--limit'); - const limit = limitIdx !== -1 ? (parseInt(args[limitIdx + 1]!, 10) || null) : null; + const limit = limitIdx !== -1 && args[limitIdx + 1] ? (parseInt(args[limitIdx + 1], 10) || null) : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/profile.ts` around lines 206 - 223, The extractMessages handler can pass undefined when `--session` has no following value; update the parsing in extractMessages to validate that when `sessionIdx` is found the next token exists and is not another flag (e.g., `args[sessionIdx + 1]` is defined and does not start with '--'); if the value is missing, throw a GSDError (Validation) with a clear message about the missing session id instead of using the non-null assertion, then call runExtractMessages with the validated sessionId; refer to symbols `extractMessages`, `sessionIdx`, `sessionId`, and `runExtractMessages`.sdk/src/query/progress.ts-39-45 (1)
39-45:⚠️ Potential issue | 🟡 MinorPass the new
Not Starteddefault throughprogressJson()too.
statsJson()now usesNot Started, butprogressJson()still callsdeterminePhaseStatus()without the new arg. That leavesprogress,progress.bar, andprogress.tablereportingPendingfor the same empty phase dirs thatstatsnow callsNot Started.Suggested fix
- const status = await determinePhaseStatus(plans, summaries, join(phasesDir, dir)); + const status = await determinePhaseStatus(plans, summaries, join(phasesDir, dir), 'Not Started');Also applies to: 105-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/progress.ts` around lines 39 - 45, The determinePhaseStatus function now accepts a defaultWhenNoPlans of 'Not Started', but progressJson still calls determinePhaseStatus without that argument, causing progress, progress.bar and progress.table to show 'Pending' for empty phase dirs; update the calls to determinePhaseStatus inside progressJson (both occurrences) to pass the same 'Not Started' default string (i.e., determinePhaseStatus(..., 'Not Started')) so progressJson matches statsJson behavior; reference determinePhaseStatus and progressJson (and the progress.bar/progress.table consumers) when making the change.get-shit-done/workflows/ingest-docs.md-285-293 (1)
285-293:⚠️ Potential issue | 🟡 MinorDrop the hard-coded
#2387from the commit template.This workflow is generic, so every ingest would inherit an unrelated issue/PR number. Keep the message generic or make the reference caller-supplied.
Suggested tweak
-gsd-sdk query commit "docs: ingest {N} docs from {SCAN_PATH} (`#2387`)" \ +gsd-sdk query commit "docs: ingest {N} docs from {SCAN_PATH}" \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@get-shit-done/workflows/ingest-docs.md` around lines 285 - 293, The commit template in the gsd-sdk command contains a hard-coded issue/PR reference "(`#2387`)" which makes every ingest reference the same unrelated number; update the commit message used in the gsd-sdk call (the string "docs: ingest {N} docs from {SCAN_PATH} (`#2387`)") to either remove the "(`#2387`)" suffix or replace it with a caller-supplied placeholder like "{ISSUE}" so callers can pass an issue/PR number when needed; ensure the caller/invocation passes the new "{ISSUE}" value if you choose the placeholder approach.sdk/src/query/route-next-action.ts-179-189 (1)
179-189:⚠️ Potential issue | 🟡 MinorUse the same numeric phase ordering in the bootstrap branch.
Line 181 picks
phases[0]before applying thecomparePhaseNum(...)sort used later. IfROADMAPentries are out of source order,/gsd-nextcan route to the wrong phase when no phase directories exist yet.💡 Suggested fix
+ const sorted = [...phases].sort((a, b) => + comparePhaseNum(String(a.number), String(b.number)), + ); + // Route 1 — ROADMAP lists phases but no phase directories - if (phases.length > 0 && dirCount === 0) { - const first = String(phases[0].number); + if (sorted.length > 0 && dirCount === 0) { + const firstPhase = sorted[0]!; + const first = String(firstPhase.number); const ctx = await buildContext(first); return { data: { command: '/gsd-discuss-phase', args: first, reason: 'ROADMAP has phases but no phase directories on disk yet', current_phase: first, - phase_name: String(phases[0].name ?? ''), + phase_name: String(firstPhase.name ?? ''), gates, context: ctx, }, }; } - - const sorted = [...phases].sort((a, b) => - comparePhaseNum(String(a.number), String(b.number)), - );Also applies to: 221-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/route-next-action.ts` around lines 179 - 189, The code picks phases[0] without applying the numeric sort used elsewhere, so sort the phases array with the existing comparePhaseNum comparator before selecting the first element; locate the block that checks (phases.length > 0 && dirCount === 0) and replace uses of phases[0] with the first element after phases.sort(comparePhaseNum), and do the same fix in the other branch that selects phases[0] (the similar selection around the buildContext/return for routing) so both routing decisions use the numerically ordered first phase; ensure you still pass the same values into buildContext, command args, current_phase and phase_name.tests/ingest-docs.test.cjs-43-45 (1)
43-45:⚠️ Potential issue | 🟡 MinorAvoid reading fixtures during suite definition.
These
readFileSync(...)calls run while the test file is loading, so a missing contract file throws before the explicit existence tests can report the real failure. Read inside eachtest(...)(or behind a lazy helper) so the suite fails on the intended assertion instead of bailing out withENOENT.Also applies to: 67-69, 92-94, 188-190, 216-218, 259-260, 285-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ingest-docs.test.cjs` around lines 43 - 45, Top-level synchronous reads like fs.readFileSync(CMD_PATH, 'utf-8') are executed during suite load and can throw before tests run; move these reads into each test or a lazy helper so existence assertions run first. Replace direct top-level fs.readFileSync calls (e.g., fs.readFileSync(CMD_PATH, ...)) with either a function readFixture = (p)=>fs.readFileSync(p,'utf-8') called inside each test or inline the fs.readFileSync(CMD_PATH, ...) inside the relevant test bodies; ensure the same change is applied for the other top-level fixture reads referenced in this file so tests fail via assertions rather than ENOENT during module load.
🧹 Nitpick comments (10)
sdk/src/query/audit-open.ts (1)
268-321: Extract the repeated phase-directory walk.These three scanners duplicate the same phase discovery, phase-number parsing, and per-file iteration flow. Pulling that into one helper would reduce drift the next time parity logic changes in one category but not the others.
Also applies to: 324-375, 377-449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/audit-open.ts` around lines 268 - 321, Multiple scanner functions (e.g., scanUatGaps) repeat the same phase-directory discovery, phase-number parsing, and per-file iteration logic; extract a shared helper (e.g., walkPhaseFiles or forEachPhaseFile) that accepts planDir and a file-filter callback and yields (phaseNum, fileName, filePath, fileContent) or invokes a handler for each matching file so scanUatGaps, the scanner at lines ~324-375, and the scanner at ~377-449 can call this helper; inside the helper implement the existing steps (join phases dir, read and sort directories, parse phaseMatch /^\d+[A-Z]?(?:\.\d+)*/i to get phaseNum, read phase files, filter by predicate, read file contents with existing error handling) and then update each scanner to use sanitizeForDisplay, extractFrontmatter, and the existing per-file logic while delegating traversal to the new function.sdk/src/init-runner.ts (1)
41-42: Resolve agents path at runtime, not module import time.
GSD_AGENTS_DIRis computed once during import, so later env/runtime changes won’t be picked up. Resolve inreadAgentFile()(or store per-instance in the constructor) instead.♻️ Proposed change
-const GSD_AGENTS_DIR = resolveAgentsDir(); ... - const fullPath = join(GSD_AGENTS_DIR, filename); + const fullPath = join(resolveAgentsDir(), filename);Also applies to: 661-662
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/init-runner.ts` around lines 41 - 42, GSD_AGENTS_DIR is computed at module import time which prevents picking up later env/runtime changes; remove or stop using the top-level const GSD_AGENTS_DIR and instead call resolveAgentsDir() inside readAgentFile() (or capture it as an instance property in the InitRunner constructor) and update all uses (including the other occurrences mentioned around lines 661-662) to reference the runtime-resolved value or the instance property; ensure readAgentFile() and any methods that load agent files use the new runtime-resolved path so environment changes are honored.sdk/src/query/profile-output.ts (1)
877-887: Consider clarifyingprofileStatuslogic.The condition at line 879-887 checks if the profile marker exists in
finalContentbut setsprofileStatus = 'already_present'in the else branch (when marker doesn't exist). This naming seems inverted—'already_present'implies the marker exists, but it's set when the marker is absent.This doesn't cause functional issues since the status is only used for messaging, but the naming could confuse future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/profile-output.ts` around lines 877 - 887, The profileStatus naming is inverted: when finalContent contains the marker ('<!-- GSD:profile-start') the code currently sets 'already_present' only in the else (marker absent) branch; update the assignments so they accurately reflect presence/absence—e.g., when finalContent.indexOf('<!-- GSD:profile-start') !== -1 set profileStatus = 'already_present' (and keep distinguishing 'placeholder_added' vs 'exists' when action === 'created' or existingContent lacks the marker), otherwise set profileStatus = 'missing' (or 'already_absent'); adjust the profileStatus values in the finalContent/if branches (variables: finalContent, profileStatus, action, existingContent, marker '<!-- GSD:profile-start') accordingly so the names match the actual condition.get-shit-done/references/doc-conflict-engine.md (1)
19-40: Specify fence languages for both examples.Please label these fenced blocks (e.g.,
text) to satisfy markdownlint MD040.Also applies to: 51-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@get-shit-done/references/doc-conflict-engine.md` around lines 19 - 40, The fenced code blocks in the "Conflict Detection Report" template lack a language specifier (triggering markdownlint MD040); update each triple-backtick fence (the initial example block and the other occurrence at lines referenced 51-53) to include a language label such as text (e.g., change ``` to ```text) so both fenced blocks are explicitly labeled and satisfy MD040.agents/gsd-doc-classifier.md (1)
145-147: Add a fence language for markdownlint compliance.The example code fence should declare a language (
textis sufficient) to satisfy MD040 consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agents/gsd-doc-classifier.md` around lines 145 - 147, The markdown example code fence containing the template string "Classified: {filename} → {TYPE} ({confidence}){, LOCKED if true}" should declare a fence language to satisfy MD040; update the triple-backtick fence surrounding that example to include a language identifier (e.g., use ```text) so the snippet is explicitly marked as plain text.sdk/src/query/requirements-extract-from-plans.ts (1)
33-37: Normalize requirement strings before aggregation.Trimming and filtering empty values will keep
requirements/by_plancleaner and avoid propagating whitespace-only entries.♻️ Suggested improvement
function normalizeReqList(v: unknown): string[] { if (!v) return []; - if (Array.isArray(v)) return v.map((x) => String(x)); - if (typeof v === 'string') return [v]; + if (Array.isArray(v)) return v.map((x) => String(x).trim()).filter(Boolean); + if (typeof v === 'string') return v.trim() ? [v.trim()] : []; return []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/requirements-extract-from-plans.ts` around lines 33 - 37, The normalizeReqList function should trim requirement strings and filter out empty or whitespace-only entries before returning; update normalizeReqList so that for Array inputs it maps each element to String(x).trim() and filters out empty results, for string inputs it returns [v.trim()] only if non-empty, and for other falsy values return []; this ensures requirements collected by normalizeReqList are trimmed and contain no blank entries.sdk/src/query/profile-scan-sessions.ts (1)
44-59: Consider wrappingstatSyncin try-catch for robustness.If a
.jsonlfile is deleted or becomes inaccessible betweenreaddirSyncandstatSync, this will throw and abort scanning the entire project. For parity tooling this may be acceptable, but a defensive pattern would continue processing other files.♻️ Optional defensive pattern
for (const entry of entries) { if (!entry.endsWith('.jsonl')) continue; const sessionId = entry.replace('.jsonl', ''); const filePath = join(projectDirPath, entry); - const stat = statSync(filePath); - - sessions.push({ - sessionId, - filePath, - size: stat.size, - modified: stat.mtime, - }); + try { + const stat = statSync(filePath); + sessions.push({ + sessionId, + filePath, + size: stat.size, + modified: stat.mtime, + }); + } catch { + // File removed/inaccessible — skip + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/profile-scan-sessions.ts` around lines 44 - 59, The loop that reads directory entries uses statSync(filePath) without error handling so a removed or inaccessible .jsonl file will throw and abort the whole scan; wrap the statSync call in a try-catch inside the for (const entry of entries) loop and on error skip that entry (optionally log a warning via your logger), then continue pushing successful results into sessions (keep sessionId and filePath logic intact) so other files are still processed.sdk/src/query/detect-phase-type.ts (1)
95-118: Consider checkingisDirectory()before callingreaddirSyncon subdirectories.The current code uses
readdirSync(sub)inside a try-catch to discover subdirectory contents, relying on ENOTDIR exceptions for regular files. UsingstatSync(sub).isDirectory()first would be more explicit.♻️ Optional clarity improvement
for (const f of dirFiles) { const sub = join(phaseDirFull, f); - if (existsSync(sub)) { - try { - const subStat = readdirSync(sub); - for (const sf of subStat) { + try { + const stat = statSync(sub); + if (stat.isDirectory()) { + const subFiles = readdirSync(sub); + for (const sf of subFiles) { allRelPaths.push(`${f}/${sf}`); } - } catch { - // Not a directory — ignore } + } catch { + // Inaccessible — ignore } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/query/detect-phase-type.ts` around lines 95 - 118, The loop that inspects subpaths currently calls readdirSync(sub) inside a try-catch relying on exceptions for non-directories; change it to explicitly check the path type first by calling statSync(sub) (or lstatSync) and verifying stat.isDirectory() before calling readdirSync. Update the loop that iterates over dirFiles (using variables phaseDirFull, sub, subStat) to skip non-directories without catching errors, so only directories have their entries appended to allRelPaths.sdk/HANDOVER-GOLDEN-PARITY.md (1)
225-231: Consider adding a language identifier to the fenced code block.The static analysis tool flags line 225 for missing a language specifier. Since this is a commit log, you could use
textorplaintextas the identifier, though this is purely cosmetic.-``` +```text 95db59c docs(sdk): update handover for profile-output and mutation subprocess batch🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/HANDOVER-GOLDEN-PARITY.md` around lines 225 - 231, The fenced code block in HANDOVER-GOLDEN-PARITY.md is missing a language identifier; update the opening fence to include a plain/text specifier (for example change ``` to ```text) so the commit log block is marked as plain text and satisfies the static analysis rule for the block starting with "95db59c docs(sdk): update handover...".agents/gsd-doc-synthesizer.md (1)
117-138: Consider adding language identifiers to template code blocks.The conflict report example and confirmation template blocks (lines 117 and 163) are missing language identifiers. Since these show plain text output format,
textwould be appropriate.-``` +```text ## Conflict Detection Report🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agents/gsd-doc-synthesizer.md` around lines 117 - 138, The markdown code fences for the "Conflict Detection Report" example and the confirmation template blocks are missing language identifiers; update the triple-backtick fences that wrap the Conflict Detection Report block and the confirmation template block (search for the literal block headers "## Conflict Detection Report" and the confirmation template around line 163) to use ```text so the blocks are explicitly marked as plain text, ensuring proper rendering and syntax highlighting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agents/gsd-doc-classifier.md`:
- Around line 113-114: The current output naming rule that writes to
{OUTPUT_DIR}/{slug}.json uses slug derived only from filename and causes
collisions (e.g., README.md in different dirs). Change the slug generation used
before writing outputs (the code that computes slug from the filename) to
incorporate directory context or a collision-resistant suffix — e.g., include
the file's relative path with path separators normalized to '-', or append a
short content/path hash — and then write to {OUTPUT_DIR}/{newSlug}.json; update
any references to "slug" generation logic so functions that compute the slug
(wherever filename -> slug is implemented) produce unique slugs per file path to
avoid parallel overwrite.
In `@bin/install.js`:
- Around line 6658-6664: The current early return when a `gsd-sdk` binary is
found (the `hasSdk` branch using `spawnSync`/`probe`) must be replaced with a
version compatibility check against the local SDK (e.g., read `sdk/package.json`
version or run the installed `gsd-sdk --version`) and only skip rebuild if
versions match; otherwise proceed with reinstall unless the user passed
`--no-sdk`. Concretely: in the `if (!hasSdk)` / `probe` logic, remove the
unconditional `return`, run a version query against the discovered `gsd-sdk` or
compare its version to `sdk/package.json`, and only `return` when versions are
equal; if they differ, fall through to reinstall; ensure the `--no-sdk` flag is
respected to skip any reinstall attempts.
In `@get-shit-done/workflows/ingest-docs.md`:
- Around line 33-42: The current path validation only checks for ".." substrings
and misses absolute or symlinked paths; update the logic that handles SCAN_PATH
and MANIFEST_PATH to canonicalize each path against the repository root (e.g.,
resolve via realpath -m or equivalent) and then enforce that the resolved path
has the repo root as its prefix before running test -d / test -f; ensure you
apply this for both SCAN_PATH and MANIFEST_PATH checks (replacing the case
"{SCAN_PATH}" and case "{MANIFEST_PATH}" checks) and fail with the
SECURITY_ERROR or PATH/MANIFEST_NOT_FOUND messages if the resolved path is
outside the repo root.
In `@sdk/src/golden/capture.ts`:
- Around line 51-69: parseGsdToolsJson currently treats `@file`: paths as relative
to the Vitest process instead of the CLI's cwd; update parseGsdToolsJson to
accept a projectDir parameter (propagate projectDir from execGsdTools where exec
runs the CLI with cwd: projectDir) and when jsonStr startsWith('@file:') use
path.isAbsolute to check the extracted filePath and, if not absolute, resolve it
against projectDir via path.resolve(projectDir, filePath) before calling
readFile; apply the same projectDir-based resolution to the other `@file` handling
sites around lines 79-80 so non-absolute paths are read from the CLI project
directory.
In `@sdk/src/gsd-tools.ts`:
- Around line 173-199: withRegistryDispatchTimeout currently races the timeout
against the work Promise but never cancels the underlying operation (e.g.,
registry.dispatch), so the in-process handler can keep mutating state after the
caller times out. Fix by creating an AbortController inside
withRegistryDispatchTimeout, pass its signal into the work creation (or change
the work parameter to be a function that accepts an AbortSignal and returns
Promise<T>), and on timeout call controller.abort() before rejecting with the
GSDToolsError; ensure callers (e.g., calls that invoke registry.dispatch) are
updated to accept and propagate the AbortSignal so the in-process handler stops
when aborted.
In `@sdk/src/query/audit-open.ts`:
- Around line 32-35: The code in sdk/src/query/audit-open.ts currently swallows
readFileSync errors (e.g., the try/catch blocks around readFileSync at multiple
locations), causing unreadable artifacts to be omitted; instead, modify the
catch handlers for each readFileSync usage to either append an "unreadable"
sentinel/audit row for that artifact (including filePath and error message) OR
mark the parent category's result with a scan_error entry so the audit reflects
the failure; look for the readFileSync try/catch sites (the occurrences shown
around lines handling artifact content) and replace the continue behavior with
logic to emit the sentinel row or escalate to category-level scan_error and
ensure the error details are captured in the emitted record.
- Around line 539-541: The return value currently sets has_open_items based only
on counts.total, which can report a clean audit even when some categories have
scan_error: true; update the logic in the return object(s) that set
has_open_items (the lines around has_open_items: counts.total > 0 and the
similar block at 568-573) to consider scan failures too — e.g., compute a
boolean like scanFailed = categories.some(c => c.scan_error) (or aggregate any
per-category scan_error flags you already compute) and set has_open_items =
scanFailed || (counts.total > 0) so any failed scan makes the audit be reported
as incomplete.
In `@sdk/src/query/config-gates.ts`:
- Around line 26-49: The boolean normalization currently uses Boolean(...) which
treats string values like "false" as true and misses the aliased key
plan_checker; update the gate initializations (e.g., plan_checker_enabled,
plan_check, research_enabled, ui_phase, ui_safety_gate, text_mode, auto_advance,
auto_chain_active, code_review, discuss_mode toggle fields,
research_before_questions, verifier, skip_discuss) to use a robust parse that
accepts true/false booleans and string "true"/"false" (e.g., check for wf.key
=== true || wf.key === 'true' or use a small helper like parseBoolean) and
ensure plan_checker_enabled also reads wf.plan_checker if wf.plan_check is
undefined so both aliases are honored; preserve existing defaults when the field
is undefined and continue to use existing symbols like contextWindow and
CONFIG_DEFAULTS.workflow.subagent_timeout for non-boolean values.
In `@sdk/src/query/intel.ts`:
- Around line 250-255: The resolved file path in intelExtractExports can escape
projectDir (path traversal); after computing filePath from resolve(projectDir,
raw) validate containment by computing the relative path (e.g.,
path.relative(projectDir, filePath)) or comparing normalized prefixes and ensure
it does not start with '..' or escape the projectDir; if the resolved path is
outside projectDir or the file does not exist, return the same safe { data: {
file: filePath, exports: [], method: 'none' } } response. Ensure checks
reference intelExtractExports, filePath, projectDir, and raw so the validation
is applied before any file access.
In `@sdk/src/query/plan-task-structure.ts`:
- Around line 20-23: The current path computation (using rel and resolve) allows
absolute paths and traversal outside projectDir; update the logic that computes
path (the code handling rel → path in plan-task-structure.ts) to first reject
absolute inputs or normalize and resolve rel against projectDir, then verify
containment by computing the resolvedPath (via resolve/projectDir and rel) and
ensuring path.relative(projectDir, resolvedPath) does not start with '..' (or
that resolvedPath startsWith projectDir + path.sep); if the check fails throw an
error instead of reading the file. Also ensure you use
path.isAbsolute/path.normalize as needed and reference the variables rel,
projectDir, resolve, and path in the fix.
In `@sdk/src/query/requirements-extract-from-plans.ts`:
- Line 68: The current planId computation (variable planId) strips "PLAN.md"
into an empty string for files named exactly "PLAN.md"; fix it by deriving the
identifier from the filename without the extension and optionally removing a
trailing "-PLAN" suffix — e.g., compute the base name (filename without ".md")
and then remove a trailing "-PLAN" only if present so that "PLAN.md" yields
"PLAN" and "foo-PLAN.md" yields "foo"; update the expression that sets planId
accordingly (replace the current planFile.replace('-PLAN.md',
'').replace('PLAN.md', '') usage).
In `@sdk/src/query/state-mutation.ts`:
- Around line 1082-1088: stateValidate() is matching phase directories with
startsWith(padded), which misses unpadded names like "2-foo" and can incorrectly
match "1" to "10-*"; replace that check by reusing the shared phase-token
matching logic (the same function used elsewhere in the codebase for locating
phase dirs) instead of startsWith — e.g., call the shared matcher (or mirror its
behavior) when computing phaseDir in the block that uses currentPhase,
phasesDir, normalized and padded so the lookup matches tokens exactly (allowing
optional zero-padding and separators) rather than prefix-matching.
In `@sdk/src/query/verify.ts`:
- Around line 599-606: The regex in the plan file loop (variables: planFiles,
phaseDir, pf, content, fmMatch, allFiles) only matches single-line inline arrays
and misses YAML frontmatter block lists; update the logic to extract and parse
the file's frontmatter instead of using /files_modified:\s*\[([^\]]*)\]/ —
identify the frontmatter block (between --- markers) from content, parse it with
a YAML/frontmatter parser (e.g., js-yaml or a front-matter library) to read
files_modified as an array (handling both inline and block list forms), then
push those parsed entries into allFiles so block-style lists are included.
- Around line 574-586: Replace the fuzzy includes() lookup and raw join() with a
safe, exact token-validated resolution: first validate phaseArg against the
project's existing token matcher (reject any path separators like '/' or '\\' in
phaseArg), then search entries using exact equality (entry.name === phaseArg)
rather than includes(), and if falling back to a candidate path compute it with
path.resolve(phasesDir, phaseArg) and ensure the resolved path is inside
phasesDir (e.g., check path.relative(phasesDir, candidate) does not start with
'..') before assigning phaseDir; reference phaseArg, phaseDir, phasesDir and the
directory entries when making these changes.
---
Outside diff comments:
In `@sdk/src/query/commit.ts`:
- Around line 251-280: The commit path currently calls spawnSync('git', ['-C',
projectDir, 'add', ...fileArgs]) and then runs git commit without verifying add
succeeded or that the index only contains the requested files; update the logic
in the handler around resolvePathUnderProject, spawnSync, fileArgs and
commitResult to first check the exit status of the git add call and return
{committed:false,...} including add stderr on non-zero exit, then run git diff
--cached --name-only (spawnSync) and compare the returned staged file list to
the requested files set (or to ['.'] when files was empty) and reject the commit
with a clear reason if any staged path is outside the requested set before
calling git commit. Ensure sanitized (from sanitizeCommitMessage) is still used
for the commit message.
In `@sdk/src/query/frontmatter.ts`:
- Around line 113-133: The array-item branch treating every "- ..." as a string
is wrong for object-style entries like "- key: value"; update the block in
frontmatter.ts that handles lines starting with '- ' (the code referencing
current.obj, stack, parent and itemValue) to detect a "key: value" pattern (e.g.
match /^([^:]+):\s*(.*)$/) and construct an object { key: value } instead of a
plain string; when converting an empty object to an array (the parent conversion
logic that assigns (parent.obj as Record<string, unknown>)[k] = [itemValue])
ensure you push the parsed object form (not the raw itemValue) and, when
current.obj is already an array, push the parsed object for object-like items
and the string for scalar items so arrays of objects (e.g., user_setup entries)
are preserved.
In `@sdk/src/query/intel.ts`:
- Around line 346-350: intelPatchMeta (and similarly intelExtractExports)
currently resolves attacker-controlled args[0] with resolve(projectDir, args[0])
and can escape projectDir leading to arbitrary file writes; fix by validating
path containment after resolving: compute the absolute target with
path.resolve(projectDir, args[0]) and ensure path.relative(projectDir, target)
does not start with '..' and is not an absolute escape, returning an error if
the check fails; apply this check before any existsSync or writeFileSync
operations in intelPatchMeta (and mirror the same containment guard in
intelExtractExports) so only files inside projectDir may be read or written.
In `@sdk/src/query/roadmap.ts`:
- Around line 73-131: getMilestoneInfo recognizes the 🟡 active-marker but
extractCurrentMilestone still only looks for 🚧, so callers (roadmap.analyze,
statsJson, progress) process the whole file instead of the active milestone
slice; update extractCurrentMilestone to treat 🟡 the same as 🚧 (e.g., accept
either emoji in its detection/regex and selection logic) so it returns the same
milestone slice for both markers and downstream functions use the active
milestone correctly.
---
Minor comments:
In `@bin/install.js`:
- Around line 80-85: The CLI parses new flags hasSdk and hasNoSdk in
bin/install.js but the usage/help output doesn't mention them; update the
help/usage text generation (where the CLI prints help for install) to include
descriptions for --sdk and --no-sdk so users can discover forcing or skipping
the SDK bootstrap. Locate the help string or function that prints usage for the
install command and add concise entries explaining --sdk (force SDK bootstrap)
and --no-sdk (skip default SDK bootstrap), keeping styling consistent with
existing option lines.
In `@get-shit-done/workflows/ingest-docs.md`:
- Around line 285-293: The commit template in the gsd-sdk command contains a
hard-coded issue/PR reference "(`#2387`)" which makes every ingest reference the
same unrelated number; update the commit message used in the gsd-sdk call (the
string "docs: ingest {N} docs from {SCAN_PATH} (`#2387`)") to either remove the
"(`#2387`)" suffix or replace it with a caller-supplied placeholder like "{ISSUE}"
so callers can pass an issue/PR number when needed; ensure the caller/invocation
passes the new "{ISSUE}" value if you choose the placeholder approach.
In `@sdk/src/cli.ts`:
- Around line 268-277: The parseCliQueryJsonOutput function reads files
referenced via the `@file`: indirection without validating the path; update
parseCliQueryJsonOutput to resolve and canonicalize the extracted filePath
(using path.resolve and/or fs.realpath) and ensure it is contained within the
allowed project root (use the same projectRoot logic as readTextArgOrFile),
throwing an error if the resolved path is outside that directory before calling
readFile; keep the existing behavior for normal JSON strings and reuse
readTextArgOrFile’s containment checks or helper if available to avoid
duplication.
In `@sdk/src/query/commit.ts`:
- Around line 105-110: The current parsing uses messageArgs = allArgs.slice(0,
endIndex).filter(a => !a.startsWith('--')) which strips any token beginning with
"--" (e.g. "--help") from commit messages; change the logic to preserve literal
"--foo" tokens by parsing known flags explicitly instead of blanket-filtering.
Specifically, in the block that computes filesIndex, endIndex, messageArgs,
message and filePaths (variables/filesIndex, endIndex, messageArgs, message,
filePaths, allArgs), remove the .filter(a => !a.startsWith('--')) behavior and
instead: scan allArgs for recognized flags (e.g. "--files" and any other
supported flags), separate their values into filePaths or flag variables, then
build messageArgs from the remaining tokens exactly as-is so that tokens
beginning with "--" that are not recognized flags remain in the message. Ensure
the same change is applied to the other occurrence noted (lines ~227-230).
In `@sdk/src/query/phase-list-queries.ts`:
- Around line 126-143: The current planId calculation in the loop over planFiles
(the planId variable created from planFile.replace(...)) can produce an empty
string for the root PLAN.md; change the logic in the loop that computes planId
(before the plans.push) to special-case 'PLAN.md' (or files that equal 'PLAN.md'
ignoring case) and assign a meaningful id (for example derive it from the phase
name or directory) instead of ''. Update the code that builds the plans entry
(the object passed to plans.push) to use this non-empty id so the base plan has
a stable identifier.
In `@sdk/src/query/profile.ts`:
- Around line 206-223: The extractMessages handler can pass undefined when
`--session` has no following value; update the parsing in extractMessages to
validate that when `sessionIdx` is found the next token exists and is not
another flag (e.g., `args[sessionIdx + 1]` is defined and does not start with
'--'); if the value is missing, throw a GSDError (Validation) with a clear
message about the missing session id instead of using the non-null assertion,
then call runExtractMessages with the validated sessionId; refer to symbols
`extractMessages`, `sessionIdx`, `sessionId`, and `runExtractMessages`.
In `@sdk/src/query/progress.ts`:
- Around line 39-45: The determinePhaseStatus function now accepts a
defaultWhenNoPlans of 'Not Started', but progressJson still calls
determinePhaseStatus without that argument, causing progress, progress.bar and
progress.table to show 'Pending' for empty phase dirs; update the calls to
determinePhaseStatus inside progressJson (both occurrences) to pass the same
'Not Started' default string (i.e., determinePhaseStatus(..., 'Not Started')) so
progressJson matches statsJson behavior; reference determinePhaseStatus and
progressJson (and the progress.bar/progress.table consumers) when making the
change.
In `@sdk/src/query/route-next-action.ts`:
- Around line 179-189: The code picks phases[0] without applying the numeric
sort used elsewhere, so sort the phases array with the existing comparePhaseNum
comparator before selecting the first element; locate the block that checks
(phases.length > 0 && dirCount === 0) and replace uses of phases[0] with the
first element after phases.sort(comparePhaseNum), and do the same fix in the
other branch that selects phases[0] (the similar selection around the
buildContext/return for routing) so both routing decisions use the numerically
ordered first phase; ensure you still pass the same values into buildContext,
command args, current_phase and phase_name.
In `@sdk/src/query/workstream.ts`:
- Around line 352-354: The variable `root` is declared but unused in the
`workstreamProgress` function; remove the unused declaration (the line that
calls `planningRoot(projectDir)`) so only `wsRoot = workstreamsDir(projectDir)`
remains, and update any related imports if `planningRoot` becomes unused
elsewhere in the file.
In `@tests/ingest-docs.test.cjs`:
- Around line 43-45: Top-level synchronous reads like fs.readFileSync(CMD_PATH,
'utf-8') are executed during suite load and can throw before tests run; move
these reads into each test or a lazy helper so existence assertions run first.
Replace direct top-level fs.readFileSync calls (e.g., fs.readFileSync(CMD_PATH,
...)) with either a function readFixture = (p)=>fs.readFileSync(p,'utf-8')
called inside each test or inline the fs.readFileSync(CMD_PATH, ...) inside the
relevant test bodies; ensure the same change is applied for the other top-level
fixture reads referenced in this file so tests fail via assertions rather than
ENOENT during module load.
---
Nitpick comments:
In `@agents/gsd-doc-classifier.md`:
- Around line 145-147: The markdown example code fence containing the template
string "Classified: {filename} → {TYPE} ({confidence}){, LOCKED if true}" should
declare a fence language to satisfy MD040; update the triple-backtick fence
surrounding that example to include a language identifier (e.g., use ```text) so
the snippet is explicitly marked as plain text.
In `@agents/gsd-doc-synthesizer.md`:
- Around line 117-138: The markdown code fences for the "Conflict Detection
Report" example and the confirmation template blocks are missing language
identifiers; update the triple-backtick fences that wrap the Conflict Detection
Report block and the confirmation template block (search for the literal block
headers "## Conflict Detection Report" and the confirmation template around line
163) to use ```text so the blocks are explicitly marked as plain text, ensuring
proper rendering and syntax highlighting.
In `@get-shit-done/references/doc-conflict-engine.md`:
- Around line 19-40: The fenced code blocks in the "Conflict Detection Report"
template lack a language specifier (triggering markdownlint MD040); update each
triple-backtick fence (the initial example block and the other occurrence at
lines referenced 51-53) to include a language label such as text (e.g., change
``` to ```text) so both fenced blocks are explicitly labeled and satisfy MD040.
In `@sdk/HANDOVER-GOLDEN-PARITY.md`:
- Around line 225-231: The fenced code block in HANDOVER-GOLDEN-PARITY.md is
missing a language identifier; update the opening fence to include a plain/text
specifier (for example change ``` to ```text) so the commit log block is marked
as plain text and satisfies the static analysis rule for the block starting with
"95db59c docs(sdk): update handover...".
In `@sdk/src/init-runner.ts`:
- Around line 41-42: GSD_AGENTS_DIR is computed at module import time which
prevents picking up later env/runtime changes; remove or stop using the
top-level const GSD_AGENTS_DIR and instead call resolveAgentsDir() inside
readAgentFile() (or capture it as an instance property in the InitRunner
constructor) and update all uses (including the other occurrences mentioned
around lines 661-662) to reference the runtime-resolved value or the instance
property; ensure readAgentFile() and any methods that load agent files use the
new runtime-resolved path so environment changes are honored.
In `@sdk/src/query/audit-open.ts`:
- Around line 268-321: Multiple scanner functions (e.g., scanUatGaps) repeat the
same phase-directory discovery, phase-number parsing, and per-file iteration
logic; extract a shared helper (e.g., walkPhaseFiles or forEachPhaseFile) that
accepts planDir and a file-filter callback and yields (phaseNum, fileName,
filePath, fileContent) or invokes a handler for each matching file so
scanUatGaps, the scanner at lines ~324-375, and the scanner at ~377-449 can call
this helper; inside the helper implement the existing steps (join phases dir,
read and sort directories, parse phaseMatch /^\d+[A-Z]?(?:\.\d+)*/i to get
phaseNum, read phase files, filter by predicate, read file contents with
existing error handling) and then update each scanner to use sanitizeForDisplay,
extractFrontmatter, and the existing per-file logic while delegating traversal
to the new function.
In `@sdk/src/query/detect-phase-type.ts`:
- Around line 95-118: The loop that inspects subpaths currently calls
readdirSync(sub) inside a try-catch relying on exceptions for non-directories;
change it to explicitly check the path type first by calling statSync(sub) (or
lstatSync) and verifying stat.isDirectory() before calling readdirSync. Update
the loop that iterates over dirFiles (using variables phaseDirFull, sub,
subStat) to skip non-directories without catching errors, so only directories
have their entries appended to allRelPaths.
In `@sdk/src/query/profile-output.ts`:
- Around line 877-887: The profileStatus naming is inverted: when finalContent
contains the marker ('<!-- GSD:profile-start') the code currently sets
'already_present' only in the else (marker absent) branch; update the
assignments so they accurately reflect presence/absence—e.g., when
finalContent.indexOf('<!-- GSD:profile-start') !== -1 set profileStatus =
'already_present' (and keep distinguishing 'placeholder_added' vs 'exists' when
action === 'created' or existingContent lacks the marker), otherwise set
profileStatus = 'missing' (or 'already_absent'); adjust the profileStatus values
in the finalContent/if branches (variables: finalContent, profileStatus, action,
existingContent, marker '<!-- GSD:profile-start') accordingly so the names match
the actual condition.
In `@sdk/src/query/profile-scan-sessions.ts`:
- Around line 44-59: The loop that reads directory entries uses
statSync(filePath) without error handling so a removed or inaccessible .jsonl
file will throw and abort the whole scan; wrap the statSync call in a try-catch
inside the for (const entry of entries) loop and on error skip that entry
(optionally log a warning via your logger), then continue pushing successful
results into sessions (keep sessionId and filePath logic intact) so other files
are still processed.
In `@sdk/src/query/requirements-extract-from-plans.ts`:
- Around line 33-37: The normalizeReqList function should trim requirement
strings and filter out empty or whitespace-only entries before returning; update
normalizeReqList so that for Array inputs it maps each element to
String(x).trim() and filters out empty results, for string inputs it returns
[v.trim()] only if non-empty, and for other falsy values return []; this ensures
requirements collected by normalizeReqList are trimmed and contain no blank
entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
davesienkowski
force-pushed
the
feat/2302-track-a-golden-v2
branch
from
f28b788 to
d1f2727
Compare
davesienkowski
added a commit
to davesienkowski/get-shit-done
that referenced
this pull request
Apr 18, 2026
…-build#2302 Track A) Golden/read-only parity tests and registry alignment, query handler fixes (check-completion, state-mutation, commit, validate, summary, etc.), and WAITING.json dual-write for .gsd/.planning readers. Refs gsd-build#2341
Contributor
Author
Phase 3 / #2302 — merge readiness (maintainers)Track C is already on This PR is one of the remaining Track A / B / D items, together with #2340 (B) and #2343 (D). CI: On the latest pushes, the required checks we rely on (PR gate, issue link, security, Merge order: I simulated merging the three branch tips onto current Suggested narrative order (track letters after C): #2341 (A) → #2340 (B) → #2343 (D). If GitHub still shows |
Merged
12 tasks
5 tasks
trek-e
requested changes
Apr 20, 2026
Collaborator
trek-e
left a comment
trek-e
left a comment
There was a problem hiding this comment.
Adversarial review — REQUEST CHANGES.
This is a substantial PR (11,469 additions, 35 new tests passing, 34 golden parity rows). The implementation scope matches the approved issue #2302 Track A. The last commit addresses CodeRabbit's major inline findings. However, there are three unresolved issues:
1. Pre-existing critical bug now in scope: lockPath returned without lock acquired (CodeRabbit, Critical, outside diff range)
CodeRabbit flagged sdk/src/query/state-mutation.ts lines 196-204: lockPath is returned in both the EEXIST retry-exhausted branch and the non-EEXIST branch without the lock file being created or owned. This means callers enter the read/modify/write critical section without holding the lock, which can corrupt concurrent STATE.md writes. This bug predates this PR — it is outside the diff range — but this PR's additions are the first to rely heavily on readModifyWriteStateMdFull for new mutation handlers (statePlannedPhase, record-session, add-decision, add-blocker, etc.). Shipping 9+ new mutation handlers that depend on a broken locking primitive without fixing the primitive is not acceptable. The fix is straightforward: in the retry-exhausted branch, throw rather than return; in the non-EEXIST branch, rethrow rather than return lockPath.
2. state.validate miscategorized in QUERY_MUTATION_COMMANDS (CodeRabbit, Minor, outside diff range)
stateValidate is read-only (checks drift, writes nothing). It should not be in QUERY_MUTATION_COMMANDS. Callers that key off this set to decide whether to take exclusive access will take unnecessary locks on read-only queries.
3. resolved: true false positive in resolve-blocker handler (CodeRabbit, Minor, outside diff range)
The resolved flag is set to true whenever the blockers section exists, even if no matching - line was removed. Automation relying on resolved to confirm a blocker was cleared will silently get false positives.
Items 2 and 3 are both in state-mutation.ts which this PR substantially rewrites (794 additions, 111 deletions). The "outside diff range" designation means these are in pre-existing lines, but since this PR is already heavily modifying the file, fixing them in the same PR is the correct approach rather than creating follow-up technical debt.
No security issues found in new code. The path containment checks (resolvePathUnderProject) added in the last fix commit are correct. The CJS fallback with GSD_QUERY_FALLBACK=off override is sound.
Fix required:
- Fix the
lockPathacquisition bug instate-mutation.ts(critical — new mutation handlers depend on this) - Remove
state.validatefromQUERY_MUTATION_COMMANDSinsdk/src/query/index.ts - Fix
resolved: trueinresolve-blockerto require an actual line removal
Collaborator
|
CodeRabbit finding audit — lock acquisition bug confirmed valid. CodeRabbit flagged two issues in Finding 1 — Lock path returned without lock held (VALID, critical) In
The comment says "graceful degradation" but this is incorrect — callers use the returned path as evidence of a held lock and proceed with STATE.md writes. Under concurrent load, two callers can both reach this branch and both proceed, corrupting STATE.md. The fix is to throw (or return a sentinel null) in both branches instead of returning Finding 2 — CodeRabbit claimed Finding 3 — The second CodeRabbit review flagged that |
davesienkowski
force-pushed
the
feat/2302-track-a-golden-v2
branch
from
529575a to
8c1b695
Compare
…-build#2302 Track A) Golden/read-only parity tests and registry alignment, query handler fixes (check-completion, state-mutation, commit, validate, summary, etc.), and WAITING.json dual-write for .gsd/.planning readers. Refs gsd-build#2341
…allback) - Recognize in-flight 🟡 milestone bullets like 🚧. - Derive from last **vX.Y Title** before ## Phases when emoji absent. - Fall back to STATE.md milestone when ROADMAP is missing; use last bare vX.Y in cleaned text instead of first (avoids v1.0 from shipped list). - Fixes init.execute-phase milestone_version and buildStateFrontmatter after state.begin-phase (syncStateFrontmatter).
…lers - Register phase.list-plans, phase.list-artifacts, plan.task-structure, requirements.extract-from-plans (SDK-only; golden-policy exceptions). - Add unit tests; document in QUERY-HANDLERS.md. - writeProfile: honor --output, render dimensions, return profile_path and dimensions_scored.
Extract agent directory resolution to helpers (GSD_AGENTS_DIR, primary ~/.claude/agents, legacy path). Use from init and docs-init init bundles. docs(15): add 15-CONTEXT for autonomous phase-15 run.
- createRegistry(eventStream, sessionId) threads correlation into mutation events - gsd-sdk query falls back to gsd-tools.cjs when no native handler matches (disable with GSD_QUERY_FALLBACK=off); stderr bridge warnings - Export createRegistry from @gsd-build/sdk; add sdk/README.md - Update QUERY-HANDLERS.md and registry module docs for fallback + sessionId - Agents: prefer node dist/cli.js query over cat/grep for STATE and plans
… extract - Normalize findPhase not-found to null before roadmap fallback (matches findPhaseInternal) - docs-init: use detectRuntime + resolveAgentsDir for checkAgentsInstalled - state.cjs stateExtractField: horizontal whitespace only after colon (YAML progress guard) - Tests: commit_docs default true; config-get golden uses temp config; golden integration green Refs: gsd-build#2302
CodeRabbit nit: dedupe JSONL record shape for isGenuineUserMessage and streamExtractMessages.
- Resolve @file: and CLI JSON indirection relative to projectDir; guard empty normalized query command - plan.task-structure + intel extract/patch-meta: resolvePathUnderProject containment - check.config-gates: safe string booleans; plan_checker alias precedence over plan_check default - state.validate/sync: phaseTokenMatches + comparePhaseNum ordering - verify.schema-drift: token match phase dirs; files_modified from parsed frontmatter - audit-open: has_scan_errors, unreadable rows, human report when scans fail - requirements PLANNED key PLAN for root PLAN.md; gsd-tools timeout note - ingest-docs: repo-root path containment; classifier output slug-hash Golden parity test strips has_scan_errors until CJS adds field.
- Secure intel.ts and cli.ts against path traversal - Catch and validate git add status in commit.ts - Expand roadmap milestone marker extraction - Fix parsing array-of-objects in frontmatter YAML - Fix unhandled config evaluations - Improve coverage test parity mapping
davesienkowski
force-pushed
the
feat/2302-track-a-golden-v2
branch
from
8c1b695 to
1d28ed0
Compare
trek-e
pushed a commit
that referenced
this pull request
Apr 20, 2026
…#2340) * feat(sdk): golden parity harness and query handler CJS alignment (#2302 Track A) Golden/read-only parity tests and registry alignment, query handler fixes (check-completion, state-mutation, commit, validate, summary, etc.), and WAITING.json dual-write for .gsd/.planning readers. Refs #2341 * fix(sdk): getMilestoneInfo matches GSD ROADMAP (🟡, last bold, STATE fallback) - Recognize in-flight 🟡 milestone bullets like 🚧. - Derive from last **vX.Y Title** before ## Phases when emoji absent. - Fall back to STATE.md milestone when ROADMAP is missing; use last bare vX.Y in cleaned text instead of first (avoids v1.0 from shipped list). - Fixes init.execute-phase milestone_version and buildStateFrontmatter after state.begin-phase (syncStateFrontmatter). * feat(sdk): phase list, plan task structure, requirements extract handlers - Register phase.list-plans, phase.list-artifacts, plan.task-structure, requirements.extract-from-plans (SDK-only; golden-policy exceptions). - Add unit tests; document in QUERY-HANDLERS.md. - writeProfile: honor --output, render dimensions, return profile_path and dimensions_scored. * feat(sdk): centralize getGsdAgentsDir in query helpers Extract agent directory resolution to helpers (GSD_AGENTS_DIR, primary ~/.claude/agents, legacy path). Use from init and docs-init init bundles. docs(15): add 15-CONTEXT for autonomous phase-15 run. * feat(sdk): query CLI CJS fallback and session correlation - createRegistry(eventStream, sessionId) threads correlation into mutation events - gsd-sdk query falls back to gsd-tools.cjs when no native handler matches (disable with GSD_QUERY_FALLBACK=off); stderr bridge warnings - Export createRegistry from @gsd-build/sdk; add sdk/README.md - Update QUERY-HANDLERS.md and registry module docs for fallback + sessionId - Agents: prefer node dist/cli.js query over cat/grep for STATE and plans * fix(sdk): init phase_found parity, docs-init agents path, state field extract - Normalize findPhase not-found to null before roadmap fallback (matches findPhaseInternal) - docs-init: use detectRuntime + resolveAgentsDir for checkAgentsInstalled - state.cjs stateExtractField: horizontal whitespace only after colon (YAML progress guard) - Tests: commit_docs default true; config-get golden uses temp config; golden integration green Refs: #2302 * refactor(sdk): share SessionJsonlRecord in profile-extract-messages CodeRabbit nit: dedupe JSONL record shape for isGenuineUserMessage and streamExtractMessages. * fix(sdk): address CodeRabbit major threads (paths, gates, audit, verify) - Resolve @file: and CLI JSON indirection relative to projectDir; guard empty normalized query command - plan.task-structure + intel extract/patch-meta: resolvePathUnderProject containment - check.config-gates: safe string booleans; plan_checker alias precedence over plan_check default - state.validate/sync: phaseTokenMatches + comparePhaseNum ordering - verify.schema-drift: token match phase dirs; files_modified from parsed frontmatter - audit-open: has_scan_errors, unreadable rows, human report when scans fail - requirements PLANNED key PLAN for root PLAN.md; gsd-tools timeout note - ingest-docs: repo-root path containment; classifier output slug-hash Golden parity test strips has_scan_errors until CJS adds field. * fix: Resolve CodeRabbit security and quality findings - Secure intel.ts and cli.ts against path traversal - Catch and validate git add status in commit.ts - Expand roadmap milestone marker extraction - Fix parsing array-of-objects in frontmatter YAML - Fix unhandled config evaluations - Improve coverage test parity mapping * docs(sdk): registry docs and gsd-sdk query call sites (#2302 Track B) Update CHANGELOG, architecture and user guides, workflow call sites, and read-guard tests for gsd-sdk query; sync ARCHITECTURE.md command/workflow counts and directory-tree totals with the repo (80 commands, 77 workflows). Address CodeRabbit: fix markdown tables and emphasis; align CLI-TOOLS GSDTools and state.read docs with implementation; correct roadmap handler name in universal-anti-patterns; resolve settings workflow config path without relying on config_path from state.load. Refs #2340 * test: raise planner character extraction limit to 48K * fix(sdk): resolve build TS error and doc conflict markers
This was referenced Apr 22, 2026
This was referenced Apr 30, 2026
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
2 participants
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.
Enhancement PR
Linked Issue
Closes #2302
What this enhancement improves
Expands SDK query handler coverage to match
gsd-tools.cjsfor golden parity. Adds thesdk/src/golden/harness (JSON parity rows, policy, integration tests) and ports decision-routing Tier 1–3 handlers plus CJS alignment fixes for existing handlers.Before / After
Before:
gsd-sdk querycovered a subset ofgsd-tools.cjscommands. No golden parity harness. Decision-routing Tier 1–3 handlers (check.config-gates, check.auto-mode, route.next-action, etc.) did not exist in the SDK.After:
sdk/src/golden/read-only-golden-rows.ts: 34 JSON parity rows verified againstgsd-tools.cjscheck.config-gates,check.auto-mode,check.phase-ready,route.next-action,detect.phase-type,check.completion,check.gates,check.verification-status,check.ship-readyscan-sessions,workstream.status,intel,agent-skills,summary.extract,verify.schema-drift,verify.key-links,uat.render-checkpoint,audit-uat,validate.agents,skill-manifest,stats.table,progress.tableresolveQueryArgv()as the clean public dispatch entry point inregistry.tsnormalize-query-command.tsextended withcheck,detect,routeprefixesHow it was implemented
34 commits cherry-picked onto
upstream/main. Key files:sdk/src/golden/—read-only-golden-rows.ts,golden-policy.ts,read-only-parity.integration.test.tssdk/src/query/— new:detect-custom-files.ts,docs-init.ts,state-project-load.ts,profile-extract-messages.ts,profile-scan-sessions.ts,profile-sample.ts,skill-manifest.ts,audit-open.ts+ tier 1–3 routing handlerssdk/src/query/index.ts— registers all new handlers increateRegistry()sdk/src/query/registry.ts— addsresolveQueryArgv()(longest-prefix public API)sdk/HANDOVER-GOLDEN-PARITY.md,sdk/HANDOVER-PARITY-DOCS.md— parity coverage documentationInvariant: Do not shrink golden assertions to go green; fix handlers or document normalization.
Testing
How I verified the enhancement works
npm testpasses — 3911 passing (Linux covered by CI) on Track A vs 3876 onmainbaseline (35 new passing tests); same 9 pre-existing failuressdk/src/golden/read-only-parity.integration.test.tsall 34 rows pass JSON parity againstgsd-tools.cjssdk/src/query/normalize-query-command.test.tscoverscheck.*,detect.*,route.*normalizationsdk/src/query/registry.test.tscoversresolveQueryArgvlongest-prefix matchingPlatforms tested
ubuntu-latest— fullnpm test)Runtimes tested
Scope confirmation
Checklist
Closes #2302— PR will be auto-closed if missingapproved-enhancementlabel — PR will be closed if missingnpm test)Breaking changes
None
Summary by CodeRabbit
New Features
/gsd-ingest-docscommand for bulk documentation ingestion with conflict detection and synthesis.gsd-sdkinstallation during setup.Bug Fixes
Documentation
Tests