Repository navigation
CLI surface correctness & agent ergonomics: get body parity, type-error path, comment edit/delete, validate dup-codes (GH-252/253/243/235) - #255
Merged
Conversation
… validate dup-codes (GH-252/253/243/235) Cohesive CLI command-surface correctness & agent-ergonomics pass under the pm-mpbb epic, bundling four agent-facing items: - GH-252 (pm-hofv): pm get --json now returns `body` INSIDE the `item` object (.item.body), matching `pm list --include-body` and where description/ acceptance_criteria live. Removed the top-level `result.body` sibling that agents kept missing. JSON + TOON + MCP pm_get all align. - GH-253 (pm-nd08): the invalid `--type` error (create/update/config) now reflects the active `--pm-path` via new `resolveItemTypesFilePath(pmRoot, schema)`, instead of hardcoding `.agents/pm/schema/types.json`. - GH-243 (pm-4swf): `pm comments --edit <index>` / `--delete <index>` (1-based) lifecycle commands for obsolete orchestration-note cleanup. New comment_edit/comment_delete ops (ownership-aware, --allow-audit-comment honored); edit preserves created_at/author and stamps edited_at. Surfaced on CLI, MCP pm_comments, completions, flag contracts, and docs. Refactored resolveCommentInput to reuse the single resolveCommentTextSource path. - GH-235 (pm-rpag): `pm validate` detects duplicate logical issue-code prefixes (e.g. ISSUE-004 shared by 2+ items) via new core/governance/issue-codes.ts; advisory warning + details.duplicate_issue_codes. Coverage held at literal 100/100/100/100 across all source. Contracts golden regenerated; changelog regenerated from closed items. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Owner
Author
|
@coderabbitai full review |
|
Warning Review limit reached
More reviews will be available in 24 minutes and 29 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ 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 (49)
📝 WalkthroughWalkthroughDelivers four independent features: (1) ChangesComment edit/delete lifecycle (GH-243)
Invalid-type error uses active
Duplicate issue-code advisory in
Changelog, plan closure, and docs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedFull review finished. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
1 similar comment
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/commands/completion-command.spec.ts (1)
227-248:⚠️ Potential issue | 🟡 MinorAdd zsh and fish assertions for --edit and --delete flag descriptions.
The completion source generates descriptions for
--editand--deletein both zsh and fish shells, but the test only asserts descriptions for--stdinand--file. For consistent test coverage, add assertions for the new flags:
- Zsh:
expect(zshScript).toContain("--edit[Replace the comment at 1-based index");andexpect(zshScript).toContain("--delete[Delete the comment at 1-based index");- Fish:
expect(fishScript).toContain("-l edit -d 'Replace the comment at 1-based index");andexpect(fishScript).toContain("-l delete -d 'Delete the comment at 1-based index");🤖 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/commands/completion-command.spec.ts` around lines 227 - 248, The test "includes comments mutation metadata flags in bash completion" currently checks for description assertions only for the --stdin and --file flags in zsh and fish shell completions, but is missing assertions for the newly added --edit and --delete flags. Add four additional expect statements after the existing zsh and fish assertions: two for zsh completion (one for --edit with "Replace the comment at 1-based index" and one for --delete with "Delete the comment at 1-based index"), and two for fish completion (one for --edit with "-l edit -d 'Replace the comment at 1-based index'" and one for --delete with "-l delete -d 'Delete the comment at 1-based index'") to ensure these flags are properly represented in both shell completions.
🤖 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 @.agents/pm/features/pm-rpag.toon:
- Around line 12-17: The actual_result field contradicts the close_reason by
still describing the pre-fix defect (duplicate codes went undetected) instead of
the current working behavior. Update actual_result to reflect that duplicate
logical issue-code prefixes are now successfully detected via the
extractIssueCode and findDuplicateIssueCodes functions in
core/governance/issue-codes.ts, with advisory warnings displayed through
buildMetadataCheck and the details.duplicate_issue_codes projection. Then verify
the comments[1] history entry is consistent with the closed state and update the
audit log entry if needed to ensure metadata aligns with the shipped
functionality and close_reason.
In `@src/cli/commands/completion.ts`:
- Around line 362-363: The fish shell completion block for the comments command
is missing the --edit and --delete flags that were added to the bash/zsh
completions. Locate the fish completion block (which handles the comments
command similar to the bash/zsh block shown) and add both the --edit and
--delete flags to the completion options list to maintain parity across all
supported shells. Ensure the flag list in the fish completion matches the
updated bash/zsh flags.
In `@src/cli/commands/get.ts`:
- Around line 48-51: The GetResult type in the get command now nests body inside
item (as item.body), but the runtime help text still documents the old structure
with body as a top-level field. Find where the help text for the get command is
defined (likely as a help string constant or documentation in the same file or a
related help module) and update it to document the new JSON shape where body is
located at .item.body instead of as a top-level sibling to item, linked,
claim_state, and children. Ensure the documented structure precisely matches the
actual GetResult type definition to prevent misleading agent consumers.
In `@src/core/governance/issue-codes.ts`:
- Around line 39-41: The docblock comment describing the regex pattern behavior
in the issue-codes.ts file incorrectly states that `ISSUE-004foo` extracts
`ISSUE-004` with the `\b` word boundary anchor. Word boundaries do not match
between numeric and alphabetic characters, so this case would not match at all.
Update the comment around the example to accurately describe the word boundary
behavior and correct the misleading example, either by removing the incorrect
case or replacing it with a case that actually demonstrates how the word
boundary works as intended.
In `@src/sdk/cli-contracts.ts`:
- Around line 306-307: The COMMENTS_FLAG_CONTRACTS now exposes the --edit and
--delete flags, but PM_TOOL_ACTION_SCHEMA_CONTRACTS.comments.optional does not
include these keys in its schema definition, causing a mismatch between the
runtime flag contracts and the strict schema contract. Add the --edit and
--delete flag entries to the PM_TOOL_ACTION_SCHEMA_CONTRACTS.comments.optional
array to ensure the schema accurately reflects all available flags at runtime.
In `@src/sdk/cli-contracts/tool-parameter-tables.ts`:
- Around line 275-276: The edit and delete parameter definitions use type
"number" which allows invalid values like decimals, zero, and negative numbers
that violate 1-based integer semantics. Change the type from "number" to
"integer" for both the edit and delete properties, and add a minimum constraint
of 1 to each to enforce that only 1-based positive integer values are accepted.
In `@tests/fixtures/contracts/full.json`:
- Line 20464: The allowAuditComment field descriptions at lines 20464 and 20570
in tests/fixtures/contracts/full.json incorrectly reference "For comments
action" when these fields actually appear in the notes and learnings action
schemas respectively. Update the description at line 20464 to correctly describe
the notes action behavior instead of comments action, and update the description
at line 20570 to correctly describe the learnings action behavior instead of
comments action. Ensure each description accurately reflects its respective
action schema context.
- Around line 1562-1567: The comments action schema around line 19882+ is
missing the edit and delete field definitions that correspond to the --edit and
--delete flags added to the comments action in the fixture at lines 1563 and
1566. Update the strict action schema for comments to include edit and delete
fields that match the flags being added in the fixture, ensuring the schema and
fixture definitions remain in sync for contract validation.
---
Outside diff comments:
In `@tests/unit/commands/completion-command.spec.ts`:
- Around line 227-248: The test "includes comments mutation metadata flags in
bash completion" currently checks for description assertions only for the
--stdin and --file flags in zsh and fish shell completions, but is missing
assertions for the newly added --edit and --delete flags. Add four additional
expect statements after the existing zsh and fish assertions: two for zsh
completion (one for --edit with "Replace the comment at 1-based index" and one
for --delete with "Delete the comment at 1-based index"), and two for fish
completion (one for --edit with "-l edit -d 'Replace the comment at 1-based
index'" and one for --delete with "-l delete -d 'Delete the comment at 1-based
index'") to ensure these flags are properly represented in both shell
completions.
🪄 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: f137662c-0b27-44a5-8b7b-3ec92c1eb0c8
📒 Files selected for processing (48)
.agents/pm/features/pm-4swf.toon.agents/pm/features/pm-rpag.toon.agents/pm/history/pm-4swf.jsonl.agents/pm/history/pm-hofv.jsonl.agents/pm/history/pm-nd08.jsonl.agents/pm/history/pm-php4.jsonl.agents/pm/history/pm-rpag.jsonl.agents/pm/issues/pm-hofv.toon.agents/pm/issues/pm-nd08.toon.agents/pm/plans/pm-php4.toonCHANGELOG.mddocs/COMMANDS.mdsrc/cli/commands/annotation-command.tssrc/cli/commands/comments.tssrc/cli/commands/completion.tssrc/cli/commands/config.tssrc/cli/commands/create.tssrc/cli/commands/get.tssrc/cli/commands/update.tssrc/cli/commands/validate.tssrc/cli/register-mutation.tssrc/core/governance/issue-codes.tssrc/core/schema/item-types-file.tssrc/core/schema/runtime-schema.tssrc/core/store/item-store.tssrc/mcp/server.tssrc/mcp/tool-definitions.tssrc/sdk/cli-contracts.tssrc/sdk/cli-contracts/tool-parameter-tables.tstests/fixtures/contracts/full.jsontests/integration/cli-inprocess-runner.integration.spec.tstests/integration/cli.integration.spec.tstests/integration/copy-command.spec.tstests/integration/help-runtime.spec.tstests/integration/mcp-handshake.spec.tstests/integration/release-readiness-runtime.spec.tstests/unit/commands/comments-command.spec.tstests/unit/commands/completion-command.spec.tstests/unit/commands/get-append-command.spec.tstests/unit/commands/register-commands.spec.tstests/unit/commands/restore-command.spec.tstests/unit/commands/update-command.spec.tstests/unit/commands/validate-command.spec.tstests/unit/core/governance/issue-codes.spec.tstests/unit/core/schema/item-types-file.spec.tstests/unit/core/schema/runtime-schema-command-flags.spec.tstests/unit/packages/beads-command.spec.tstests/unit/packages/todos-extension.spec.ts
Comment on lines
+12
to
+17
| resolution: New core/governance/issue-codes.ts + metadata-check advisory warning (details.duplicate_issue_codes). | ||
| expected_result: validate flags codes like ISSUE-004 shared by 2+ items. | ||
| actual_result: duplicate logical codes went undetected. | ||
| comments[1]{created_at,author,text}: | ||
| "2026-06-16T19:30:11.283Z",maintainer-agent,"Integration (lead): validate dup issue-code detection shipped via new core/governance/issue-codes.ts (extractIssueCode/findDuplicateIssueCodes) + buildMetadataCheck advisory warning + summarizeDuplicateIssueCodes projection (details.duplicate_issue_codes). Added summarize unit test for truncation/verbose branches." | ||
| close_reason: "Added: pm validate detects duplicate logical issue-code prefixes (advisory)." |
There was a problem hiding this comment.
Closed feature outcome is internally inconsistent (actual_result still states the bug).
For a closed item, actual_result should describe the shipped behavior, not the pre-fix defect. Please update actual_result to reflect detection now works, then regenerate/update the corresponding history entry so metadata and audit log remain consistent.
🤖 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 @.agents/pm/features/pm-rpag.toon around lines 12 - 17, The actual_result
field contradicts the close_reason by still describing the pre-fix defect
(duplicate codes went undetected) instead of the current working behavior.
Update actual_result to reflect that duplicate logical issue-code prefixes are
now successfully detected via the extractIssueCode and findDuplicateIssueCodes
functions in core/governance/issue-codes.ts, with advisory warnings displayed
through buildMetadataCheck and the details.duplicate_issue_codes projection.
Then verify the comments[1] history entry is consistent with the closed state
and update the audit log entry if needed to ensure metadata aligns with the
shipped functionality and close_reason.
Comment on lines
+48
to
+51
| // `body` lives inside `item` (alongside `description`/`acceptance_criteria`) | ||
| // for parity with `pm list --include-body`, so agents reliably find it at | ||
| // `.item.body` in JSON output instead of a top-level sibling. | ||
| item: Partial<ItemFrontMatter> & { body?: string }; |
There was a problem hiding this comment.
Update runtime help contract to .item.body to match this new JSON shape.
GetResult now nests body under item, but runtime help text still documents { item, body, linked, claim_state, children } with top-level body. That cross-layer mismatch will mislead agent consumers.
Suggested docs/help wording update
- JSON output shape is { item, body, linked, claim_state, children }; body is top-level (not item.body), and children appears for requested/container rollups.
+ JSON output shape is { item, linked, claim_state, children }; body is nested at item.body (parity with list --include-body), and children appears for requested/container rollups.🤖 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/get.ts` around lines 48 - 51, The GetResult type in the get
command now nests body inside item (as item.body), but the runtime help text
still documents the old structure with body as a top-level field. Find where the
help text for the get command is defined (likely as a help string constant or
documentation in the same file or a related help module) and update it to
document the new JSON shape where body is located at .item.body instead of as a
top-level sibling to item, linked, claim_state, and children. Ensure the
documented structure precisely matches the actual GetResult type definition to
prevent misleading agent consumers.
| }, | ||
| "allowAuditComment": { | ||
| "description": "For comments action, allow non-owner append-only comment audits without requiring --force.", | ||
| "description": "For comments action, allow non-owner append-only comment audits (add/edit/delete) without requiring --force.", |
There was a problem hiding this comment.
allowAuditComment descriptions are incorrect in non-comments schemas
Line 20464 and Line 20570 are under notes/learnings action schemas, but the description says “For comments action ... (add/edit/delete)”. That text is misleading for these actions.
Also applies to: 20570-20570
🤖 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/fixtures/contracts/full.json` at line 20464, The allowAuditComment
field descriptions at lines 20464 and 20570 in
tests/fixtures/contracts/full.json incorrectly reference "For comments action"
when these fields actually appear in the notes and learnings action schemas
respectively. Update the description at line 20464 to correctly describe the
notes action behavior instead of comments action, and update the description at
line 20570 to correctly describe the learnings action behavior instead of
comments action. Ensure each description accurately reflects its respective
action schema context.
- get help-content: document JSON shape with body nested at item.body (was top-level) - cli-contracts: add edit/delete to strict PM_TOOL_ACTION_SCHEMA_CONTRACTS.comments.optional - tool-parameter-tables: constrain edit/delete to integer minimum 1; revert shared allowAuditComment description (it is reused by notes/learnings schemas) - completion: add fish completions for comments --edit/--delete (shell parity) - issue-codes: correct the \b word-boundary docblock (ISSUE-004foo does NOT match); make the boundary test explicit (toBeNull) instead of self-referential - contracts golden regenerated Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
Addressed CodeRabbit's findings in b21e546:
Declined (with rationale): the @coderabbitai full review |
|
The rationale for declining the Kicking off the full review now. ✅ Action performedFull review finished. |
…deRabbit) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
Pushed 7fada84: added zsh + fish completion assertions for @coderabbitai full review |
|
Kicking off the full review now. ✅ Action performedFull review finished. |
This was referenced Jun 16, 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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A cohesive pass on the CLI command-surface & agent-ergonomics epic (
pm-mpbb), bundling four agent-facing correctness/completeness items. Two are fresh correctness bugs reported today (GH-252/253); two complete the command surface (GH-243/235).GH-252 —
pm get --jsonbody parity (pm-hofv)pm get --jsonplacedbodyas a top-level sibling ofitem, whiledescription/acceptance_criterialive insideitemandpm list --include-bodyputsbodyinside each row. Agents reading.item.bodygotnull. Nowbodylives insideitemfor both JSON and TOON (and MCPpm_get), so a single read exposes every field at a consistent path. Body is included atstandarddepth and above;--depth briefomits it.GH-253 — invalid
--typeerror path (pm-nd08)The invalid-
--typerejection (create/update/config set governance-create-default-type) hardcoded.agents/pm/schema/types.json, ignoring the active--pm-path. NewresolveItemTypesFilePath(pmRoot, schema)threads the real configuredschema/types.jsonpath into the hint — matching whatpm schema add/remove-typealready report.GH-243 —
pm comments --edit/--delete(pm-4swf)Comments supported only list/add; long-lived agent trackers accumulate obsolete orchestration notes with no cleanup path. Added 1-based
--edit <index>(replacement text from[text]/--add/--stdin/--file) and--delete <index>. Newcomment_edit/comment_deletehistory ops, ownership-aware (--allow-audit-commenthonored,--forceworks); edit preservescreated_at/authorand stampsedited_at. Surfaced on CLI, MCPpm_comments, shell completions, flag contracts, and docs.GH-235 —
pm validateduplicate logical issue-codes (pm-rpag)Trackers adopt title conventions like
ISSUE-004: ...; two items sharing a code break auditability. New purecore/governance/issue-codes.ts(extractIssueCode/findDuplicateIssueCodes) drives an advisory validate warning (details.duplicate_issue_codes) with per-code ids/titles + remediation hint. Advisory, never fails an otherwise-clean tracker.Also closed
pm-php4closed.Testing
resolveCommentInputto reuse the singleresolveCommentTextSourcepath (no duplication).🤖 Generated with Claude Code