fix(#2767): pass paths via --files to gsd-sdk query commit + lint guard#2781
fix(#2767): pass paths via --files to gsd-sdk query commit + lint guard#2781
Conversation
Workflows, agents, commands, and references passed file paths positionally to `gsd-sdk query commit`, which silently appended them to the commit subject and triggered the `.planning/` wholesale-stage fallback in sdk/src/query/commit.ts:136. Regression of #733/#798. Inserted `--files` before the path list at every site (81 invocations across 50 files). Added tests/bug-2767-gsd-sdk-commit-files-flag.test.cjs as a permanent lint that scans every shipped .md file and asserts each `gsd-sdk query commit[-to-subrepo]` invocation either uses `--files` or carries no path arguments. Closes #2767
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR systematically updates all Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes The changes are extensive in file count (50+) but highly homogeneous and repetitive — the same pattern ( Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
get-shit-done/references/git-planning-commit.md (1)
7-13:⚠️ Potential issue | 🟠 MajorFix contradictory guidance in the same section.
Line 7 says not to use
--filesforcommit, but Line 12 now uses--files(correctly). This contradiction will confuse users and can cause incorrect command usage. Please update the prose to match the examples.✏️ Suggested doc fix
-Pass the message first, then file paths (positional). Do not use `--files` for `commit` (that flag is only for `commit-to-subrepo`). +Pass the message first, then provide target paths after `--files`. +Use `--files` whenever scoping `gsd-sdk query commit` to specific artifacts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@get-shit-done/references/git-planning-commit.md` around lines 7 - 13, The prose contradicts itself about using --files with the commit command; update the text so it matches the example: state clearly that for commit you must pass the message first and then positional file paths (do not use --files for commit unless referring to commit-to-subrepo), and note that for .planning/ files the recommended gsd-sdk query commit invocation is: gsd-sdk query commit "docs({scope}): {description}" --files .planning/STATE.md .planning/ROADMAP.md (explain this handles commit_docs and gitignore checks automatically); replace the sentence on Line 7 with wording that instructs using positional files and clarifies that --files is only for commit-to-subrepo, so the prose and the example are consistent.
🧹 Nitpick comments (1)
tests/bug-2767-gsd-sdk-commit-files-flag.test.cjs (1)
109-109:--filesposition is not validated, so a misordered invocation slips through.
args.includes('--files')accepts the flag anywhere, including before the message (e.g.gsd-sdk query commit --files "subject"). Per the SDK atsdk/src/query/commit.ts:108-119, this would setendIndex = 0, leavingmessageArgsempty and producingcommit message required. The lint would still pass it as “well-formed”.Low severity since this misordering is unlikely in hand-written workflow markdown, but the heuristic could be hardened to require
--filesto appear after at least one non-flag (message) token.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bug-2767-gsd-sdk-commit-files-flag.test.cjs` at line 109, The current check "if (args.includes('--files')) return true;" accepts the flag in any position; change it to require the "--files" flag appears after at least one non-flag (message) token by computing const filesIdx = args.indexOf('--files') and the index of the first non-flag token (e.g. first token where !token.startsWith('-')), then return true only if filesIdx > -1 && filesIdx > firstNonFlagIdx; update the test helper that contains the args array check to use these indexes instead of args.includes('--files').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/zh-CN/references/git-planning-commit.md`:
- Line 12: The sentence in the docs that claims the `commit` command should not
use `--files` is now contradictory with the examples; update the text in
docs/zh-CN/references/git-planning-commit.md so it matches the examples and
explicitly allow using `--files` with the `gsd-sdk query commit` invocation
(e.g., `gsd-sdk query commit "docs({scope}): {description}" --files
.planning/STATE.md .planning/ROADMAP.md`), replacing the old prohibition wording
with a short statement that `--files` is supported for targeting specific files.
In `@tests/bug-2767-gsd-sdk-commit-files-flag.test.cjs`:
- Around line 9-13: Update the test file docstring to match the actual behavior
of isWellFormed: remove the claim that message-only commits are "marked by `#
message-only`" and instead state that any `gsd-sdk query commit` invocation with
no positional path arguments is treated as message-only (i.e., the linter
accepts absent paths regardless of comments). Reference the isWellFormed
function in your commit message and docstring change so future contributors
understand the linting rule rather than assuming the nonexistent `#
message-only` marker is required.
---
Outside diff comments:
In `@get-shit-done/references/git-planning-commit.md`:
- Around line 7-13: The prose contradicts itself about using --files with the
commit command; update the text so it matches the example: state clearly that
for commit you must pass the message first and then positional file paths (do
not use --files for commit unless referring to commit-to-subrepo), and note that
for .planning/ files the recommended gsd-sdk query commit invocation is: gsd-sdk
query commit "docs({scope}): {description}" --files .planning/STATE.md
.planning/ROADMAP.md (explain this handles commit_docs and gitignore checks
automatically); replace the sentence on Line 7 with wording that instructs using
positional files and clarifies that --files is only for commit-to-subrepo, so
the prose and the example are consistent.
---
Nitpick comments:
In `@tests/bug-2767-gsd-sdk-commit-files-flag.test.cjs`:
- Line 109: The current check "if (args.includes('--files')) return true;"
accepts the flag in any position; change it to require the "--files" flag
appears after at least one non-flag (message) token by computing const filesIdx
= args.indexOf('--files') and the index of the first non-flag token (e.g. first
token where !token.startsWith('-')), then return true only if filesIdx > -1 &&
filesIdx > firstNonFlagIdx; update the test helper that contains the args array
check to use these indexes instead of args.includes('--files').
🪄 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: 614c4dd1-1926-4653-93d2-182ea8d78c15
📒 Files selected for processing (51)
agents/gsd-code-fixer.mdagents/gsd-debugger.mdagents/gsd-executor.mdagents/gsd-phase-researcher.mdagents/gsd-planner.mdagents/gsd-research-synthesizer.mdagents/gsd-ui-researcher.mdcommands/gsd/add-backlog.mdcommands/gsd/review-backlog.mdcommands/gsd/thread.mddocs/zh-CN/references/git-integration.mddocs/zh-CN/references/git-planning-commit.mddocs/zh-CN/references/planning-config.mdget-shit-done/references/autonomous-smart-discuss.mdget-shit-done/references/git-integration.mdget-shit-done/references/git-planning-commit.mdget-shit-done/references/planner-revision.mdget-shit-done/references/planning-config.mdget-shit-done/workflows/add-todo.mdget-shit-done/workflows/autonomous.mdget-shit-done/workflows/check-todos.mdget-shit-done/workflows/cleanup.mdget-shit-done/workflows/complete-milestone.mdget-shit-done/workflows/diagnose-issues.mdget-shit-done/workflows/discuss-phase-assumptions.mdget-shit-done/workflows/discuss-phase.mdget-shit-done/workflows/execute-phase.mdget-shit-done/workflows/execute-plan.mdget-shit-done/workflows/explore.mdget-shit-done/workflows/import.mdget-shit-done/workflows/ingest-docs.mdget-shit-done/workflows/map-codebase.mdget-shit-done/workflows/milestone-summary.mdget-shit-done/workflows/new-milestone.mdget-shit-done/workflows/new-project.mdget-shit-done/workflows/pause-work.mdget-shit-done/workflows/plan-milestone-gaps.mdget-shit-done/workflows/plan-phase.mdget-shit-done/workflows/plant-seed.mdget-shit-done/workflows/quick.mdget-shit-done/workflows/remove-phase.mdget-shit-done/workflows/review.mdget-shit-done/workflows/ship.mdget-shit-done/workflows/sketch-wrap-up.mdget-shit-done/workflows/sketch.mdget-shit-done/workflows/spike-wrap-up.mdget-shit-done/workflows/spike.mdget-shit-done/workflows/ui-phase.mdget-shit-done/workflows/ui-review.mdget-shit-done/workflows/verify-work.mdtests/bug-2767-gsd-sdk-commit-files-flag.test.cjs
The original test walked every shipped .md file and regex-tokenized `gsd-sdk query commit` invocations to assert `--files` was present. CONTRIBUTING.md prohibits this source-grep pattern. Rewrite as behavioral SDK tests against `sdk/dist/cli.js` over a real tmp git project (createTempGitProject helper). Cover both the well-formed (`--files <paths>`) form — clean subject, exactly-staged files, .planning/ left untouched — and the buggy positional form, asserting the documented misbehavior (paths leak into subject + the `.planning/` wholesale-stage fallback at commit.ts:136). Also asserts `commit-to-subrepo` rejects when `--files` is omitted (commit.ts:258). The doc-lint is retained as a supplementary defense-in-depth guard since agent-prompt markdown invocations cannot be exercised end-to-end — but it is no longer the primary contract.
…+ fix test docstring
TL;DR
Every
gsd-sdk query commitcallsite that passed paths positionally was broken — paths were spliced into the commit subject and the SDK silently fell back to staging.planning/wholesale. Fixed all 81 sites and added a permanent lint test.What
--filesbefore the path list at 81 invocations across 50 files — workflows, agents, commands, references, and zh-CN docs.tests/bug-2767-gsd-sdk-commit-files-flag.test.cjs: scans every shipped.mdfile underagents/,commands/,get-shit-done/,docs/,scripts/and asserts eachgsd-sdk query commit[-to-subrepo]invocation either uses--filesor has no path arguments.Why
sdk/src/query/commit.tsparses argv as: everything before--filesis the commit message, everything after is paths. Without--files:filesToStageto['.planning/'](commit.ts:136), staging every untracked planning artifact instead of the targeted set.The triage on #2767 identified 5 sites in
discuss-phase.mdandplan-phase.md; the actual blast radius was the entire workflow surface — every commit step exceptplan-phase.md:1422(the one site already using--files, which was the discovery vector). This is a regression of #733 / #798 that crept back in via the #2122 "normalization" sweep, which standardized on the wrong form.How
--filesafter the message token, before the first positional path. Trailing flags (--amend,--force,--no-verify) preserved at the tail.\\) preserved and reformatted: message on its own line,--fileson its own line, paths follow..mdfile, joins shell line continuations, tokenizes args with quote-aware splitting, and flags any non-flag token after the message when--filesis absent.Test plan
node --test tests/bug-2767-gsd-sdk-commit-files-flag.test.cjs— passes (was RED before fix; 81 broken sites reported with file/line/cmd).npm test— full suite passes (5676 tests, 0 failures).discuss-phase.md,plan-phase.md,gsd-executor.md,gsd-code-fixer.md(multi-line continuation),gsd-planner.md.Knock-on audit
Audited every other
gsd-sdk query <subcommand>invocation for the same positional-vs-flag drift.commit-to-subrepoalready enforces--filesat runtime (commit.ts:258) and is now also covered by the lint. No other subcommand exhibits the pattern — they take typed positional args (phase numbers, paths-as-first-arg) where the SDK explicitly distinguishes them from flags.Closes #2767
Summary by CodeRabbit
Documentation
--filesflag when specifying target files for commit operations, replacing positional argument usage.Tests
gsd-sdk query commitinvocations in documentation properly use the--filesflag for file argument specification.