Repository navigation
Guard pm tracker path targeting for agents - #142
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
@codex review Please review the pm tracker-path guardrail changes, especially |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
To use Codex here, create a Codex account and connect to github. |
|
Warning Review limit reached
More reviews will be available in 26 minutes and 20 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (21)
📝 WalkthroughWalkthroughThis PR implements guardrails and clearer semantics for tracker-path targeting in the PM CLI. It introduces ChangesPM CLI path flag and init guardrails
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/cli/bootstrap-args.ts (1)
49-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnify
--pm-pathvs--pathprecedence with runtime option resolution.Bootstrap parsing currently applies “last seen wins” across both flags, while runtime
getGlobalOptionspreferspmPath. If both flags are supplied with different values, bootstrap-time root resolution can diverge from execution-time root resolution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/bootstrap-args.ts` around lines 49 - 87, parseBootstrapGlobalOptions currently applies "last seen wins" to path flags which can diverge from runtime getGlobalOptions that prefers pmPath; update parseBootstrapGlobalOptions (and adjust parseBootstrapPathToken usage) to track pmPath and legacy path separately instead of a single pathValue, and after parsing apply the same precedence as getGlobalOptions (use pmPath when present, otherwise fall back to --path). Ensure parseBootstrapPathToken still returns enough info to distinguish a pmPath vs legacy --path so you can set pmPath/path variables and finally set pathValue = pmPath ?? path.src/cli/main.ts (1)
605-616:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrip
pmPathfrom command-scoped options to avoid polluting mutation payloads.Line 1615 introduces
--pm-path, butextractCommandScopedOptionsonly deletespath. When--pm-pathis used,pmPathcan remain incommandOptionsand be propagated as an unintended field on create/update flows.Suggested fix
function extractCommandScopedOptions( command: Command, commandArgs: string[], extensionFlagDefinitions: Array<Record<string, unknown>> = [], ): Record<string, unknown> { const allOptions = command.optsWithGlobals() as Record<string, unknown>; const scoped: Record<string, unknown> = { ...allOptions }; delete scoped.json; delete scoped.quiet; delete scoped.path; + delete scoped.pmPath; delete scoped.noExtensions; delete scoped.extensions; delete scoped.profile; delete scoped.pager;Also applies to: 1615-1616
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/main.ts` around lines 605 - 616, The extractCommandScopedOptions function currently deletes generic keys like path but does not remove the pmPath option introduced for --pm-path, so pmPath can leak into commandOptions and mutation payloads; update extractCommandScopedOptions (used with command.optsWithGlobals()) to also delete scoped.pmPath (and any camelCase variant if present) along with json, quiet, path, noExtensions, and extensions so that --pm-path is stripped from command-scoped options before returning.src/sdk/cli-contracts.ts (1)
426-434:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
init --forcewas added to flags but not to the action schema contract.The new
INIT_FLAG_CONTRACTSentry includes--force, butPM_TOOL_ACTION_SCHEMA_CONTRACTS.init.optionalstill omitsforce. This creates a CLI/SDK contract mismatch.Suggested fix
- init: { optional: ["prefix", "preset", "typePreset", "defaults", "author", "agentGuidance", "withPackages", "verbose"] }, + init: { optional: ["prefix", "preset", "typePreset", "defaults", "author", "agentGuidance", "withPackages", "verbose", "force"] },Also applies to: 1542-1542
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sdk/cli-contracts.ts` around lines 426 - 434, The INIT_FLAG_CONTRACTS array was updated to include the "--force" flag but the action schema contract PM_TOOL_ACTION_SCHEMA_CONTRACTS.init.optional does not include a corresponding "force" entry, causing a contract mismatch; update PM_TOOL_ACTION_SCHEMA_CONTRACTS.init.optional to include "force" (matching the flag name used in INIT_FLAG_CONTRACTS) so the init action schema recognizes the new optional flag (ensure you add the "force" key to the optional array/object alongside the other init optional entries).tests/unit/completion-command.spec.ts (1)
635-643:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd test assertion for
--pm-pathin fish global flags.Once the fish completion implementation is updated to include
--pm-path(per the comment on the source file), this test should be updated to verify that--pm-pathappears in the generated fish completion script.🧪 Proposed fix
it("includes global flag completions", () => { const script = generateFishScript(); expect(script).toContain("-l json"); expect(script).toContain("-l quiet"); + expect(script).toContain("-l pm-path"); expect(script).toContain("-l path"); expect(script).toContain("-l no-extensions"); expect(script).toContain("-l profile"); expect(script).toContain("-l version"); expect(script).toContain("-l help"); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/completion-command.spec.ts` around lines 635 - 643, Update the "includes global flag completions" test in tests/unit/completion-command.spec.ts (the test that calls generateFishScript()) to assert the new pm-path global flag is present; add an expectation similar to the other global flags, e.g. expect(script).toContain("-l pm-path"), so the generated fish completion script includes the pm-path long flag.src/cli/commands/completion.ts (1)
1469-1477:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
--pm-pathin fish global flags.The fish completion global flags section includes
--pathbut is missing the new--pm-pathflag. Both bash and zsh completions correctly include both flags (lines 79-83, 586-587), but the fish completion at lines 1469-1477 only defines--path.Add a corresponding line for
--pm-pathin the fish global flags section to maintain parity with bash and zsh.🐟 Proposed fix
# Global flags (available for all subcommands) complete -c pm -l json -d 'Output JSON instead of TOON' complete -c pm -l quiet -d 'Suppress stdout output' complete -c pm -l no-changed-fields -d 'Omit changed_fields array from mutation output' +complete -c pm -l pm-path -d 'Explicit tracker storage path for this command' -r complete -c pm -l path -d 'Override PM path for this command' -r complete -c pm -l no-extensions -d 'Disable extension loading' complete -c pm -l profile -d 'Print deterministic timing diagnostics'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/commands/completion.ts` around lines 1469 - 1477, Add the missing fish global flag for --pm-path by inserting a line mirroring the existing --path entry in the fish completion global flags block; specifically add a completion for the pm command with the long flag name "pm-path" and the same description/option behavior as "--path" (e.g., "Override PM path for this command", with any required/argument marker like -r) next to the existing "complete -c pm -l path" entry so fish matches bash/zsh; reference the existing fish block entries "complete -c pm -l path -d 'Override PM path for this command' -r" and the missing flag "--pm-path".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 7: The Unreleased changelog entry "Add schema show-status status
inspection" is out of scope for this PR; either remove or replace that line with
the correct guardrail entry for this change set (e.g., the `--pm-path` /
workspace-root guardrail text) or move the exact "Add schema show-status status
inspection" item into the PR that actually introduced it; update CHANGELOG.md so
the Unreleased section contains only items relevant to this PR and ensure the
replacement mentions the guardrail flag `--pm-path` (or references the intended
workspace-root guardrail) for clarity.
---
Outside diff comments:
In `@src/cli/bootstrap-args.ts`:
- Around line 49-87: parseBootstrapGlobalOptions currently applies "last seen
wins" to path flags which can diverge from runtime getGlobalOptions that prefers
pmPath; update parseBootstrapGlobalOptions (and adjust parseBootstrapPathToken
usage) to track pmPath and legacy path separately instead of a single pathValue,
and after parsing apply the same precedence as getGlobalOptions (use pmPath when
present, otherwise fall back to --path). Ensure parseBootstrapPathToken still
returns enough info to distinguish a pmPath vs legacy --path so you can set
pmPath/path variables and finally set pathValue = pmPath ?? path.
In `@src/cli/commands/completion.ts`:
- Around line 1469-1477: Add the missing fish global flag for --pm-path by
inserting a line mirroring the existing --path entry in the fish completion
global flags block; specifically add a completion for the pm command with the
long flag name "pm-path" and the same description/option behavior as "--path"
(e.g., "Override PM path for this command", with any required/argument marker
like -r) next to the existing "complete -c pm -l path" entry so fish matches
bash/zsh; reference the existing fish block entries "complete -c pm -l path -d
'Override PM path for this command' -r" and the missing flag "--pm-path".
In `@src/cli/main.ts`:
- Around line 605-616: The extractCommandScopedOptions function currently
deletes generic keys like path but does not remove the pmPath option introduced
for --pm-path, so pmPath can leak into commandOptions and mutation payloads;
update extractCommandScopedOptions (used with command.optsWithGlobals()) to also
delete scoped.pmPath (and any camelCase variant if present) along with json,
quiet, path, noExtensions, and extensions so that --pm-path is stripped from
command-scoped options before returning.
In `@src/sdk/cli-contracts.ts`:
- Around line 426-434: The INIT_FLAG_CONTRACTS array was updated to include the
"--force" flag but the action schema contract
PM_TOOL_ACTION_SCHEMA_CONTRACTS.init.optional does not include a corresponding
"force" entry, causing a contract mismatch; update
PM_TOOL_ACTION_SCHEMA_CONTRACTS.init.optional to include "force" (matching the
flag name used in INIT_FLAG_CONTRACTS) so the init action schema recognizes the
new optional flag (ensure you add the "force" key to the optional array/object
alongside the other init optional entries).
In `@tests/unit/completion-command.spec.ts`:
- Around line 635-643: Update the "includes global flag completions" test in
tests/unit/completion-command.spec.ts (the test that calls generateFishScript())
to assert the new pm-path global flag is present; add an expectation similar to
the other global flags, e.g. expect(script).toContain("-l pm-path"), so the
generated fish completion script includes the pm-path long flag.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 8166dacf-3904-441f-8341-b7919e2701d9
📒 Files selected for processing (20)
.agents/pm/history/pm-xm98.jsonl.agents/pm/history/pm-yo3f.jsonl.agents/pm/plans/pm-xm98.toon.agents/pm/tasks/pm-yo3f.toonAGENTS.mdCHANGELOG.mddocs/CONFIGURATION.mddocs/EXTENSIONS.mdsrc/cli/bootstrap-args.tssrc/cli/commands/completion.tssrc/cli/commands/extension/scaffold.tssrc/cli/commands/init.tssrc/cli/main.tssrc/cli/register-setup.tssrc/cli/registration-helpers.tssrc/sdk/cli-contracts.tstests/fixtures/contracts/full.jsontests/integration/cli.integration.spec.tstests/unit/bootstrap-args.spec.tstests/unit/completion-command.spec.ts
💤 Files with no reviewable changes (1)
- docs/EXTENSIONS.md
|
|
||
| ### Added | ||
|
|
||
| - Add schema show-status status inspection ([pm-qpus](https://github.com/unbraind/pm-cli/blob/main/.agents/pm/features/pm-qpus.toon)) |
There was a problem hiding this comment.
Unreleased “Added” item appears out of scope for this PR.
Add schema show-status status inspection doesn’t match the path-targeting/guardrail objective in this change set, so the Unreleased section may misrepresent what shipped in this PR. Please either replace it with the --pm-path/workspace-root guardrail entry or move this item to the PR that introduced it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` at line 7, The Unreleased changelog entry "Add schema
show-status status inspection" is out of scope for this PR; either remove or
replace that line with the correct guardrail entry for this change set (e.g.,
the `--pm-path` / workspace-root guardrail text) or move the exact "Add schema
show-status status inspection" item into the PR that actually introduced it;
update CHANGELOG.md so the Unreleased section contains only items relevant to
this PR and ensure the replacement mentions the guardrail flag `--pm-path` (or
references the intended workspace-root guardrail) for clarity.
2cfeaae to
32b99a8
Compare
|
@codex review Re-triggering after the static-gate fix: moved the workspace-root --pm-path regression into a focused integration spec, kept the omnibus integration spec below the static line threshold, and reran linked pm evidence gates. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
To use Codex here, create a Codex account and connect to github. |
Add the preferred --pm-path global option while retaining --path as a compatibility alias for tracker storage roots. Guard pm init when an explicit tracker path points at a likely workspace root, with --force as the intentional override. Update completions, contracts, docs, changelog, and regression coverage for the new path semantics. Record the broad CLI/SDK/ecosystem audit in pm, including sub-agent evidence for external package smoke, GitHub/observability, SDK review, and docs/agent-UX review. Verification: pm test pm-yo3f --run --progress; node scripts/run-tests.mjs coverage; pm validate --check-resolution --check-history-drift; pm health --check-only --summary; node scripts/release/sentry-telemetry-gate.mjs --json --telemetry-mode best-effort --max-critical 0 --max-high 0 --max-telemetry-error-rate 10 --max-telemetry-missing-error-rows 0; node dist/cli.js changelog generate --output CHANGELOG.md --title Changelog --mode replace --all-release-tags --status closed --item-url-base https://github.com/unbraind/pm-cli/blob/main/.agents/pm --check.
32b99a8 to
fd4869e
Compare
|
@codex review Re-triggering after addressing valid review feedback: aligned --pm-path precedence, stripped pmPath from scoped mutation options, added init.force action schema coverage/snapshot, added fish --pm-path completion coverage, and reran linked pm gates. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
To use Codex here, create a Codex account and connect to github. |
|
🧠 Learnings used✅ Action performedReview finished.
|
Summary
--pm-pathglobal option while keeping--pathas a tracker-root compatibility aliaspm initwhen an explicit tracker path points at a likely workspace root, with--forcefor intentional root-level tracker filespmviapm-yo3fandpm-xm98Closes #140.
Verification
pm test pm-yo3f --run --progressnode scripts/run-tests.mjs coveragepm validate --check-resolution --check-history-driftpm health --check-only --summarynode scripts/release/sentry-telemetry-gate.mjs --json --telemetry-mode best-effort --max-critical 0 --max-high 0 --max-telemetry-error-rate 10 --max-telemetry-missing-error-rows 0node dist/cli.js changelog generate --output CHANGELOG.md --title "Changelog" --mode replace --all-release-tags --status closed --item-url-base https://github.com/unbraind/pm-cli/blob/main/.agents/pm --checkNotes
pnpm audit --audit-level moderatepassed in the GitHub/observability sub-agent lane; later local retries hit npm registryETIMEDOUT, so this PR also includespnpm security:scanand live Sentry/telemetry evidence.