Skip to content

Single-call mutation ergonomics: pm create/update --body-file, pm close -m/-r/-d, --resolution as close reason - #230

Merged
unbraind merged 2 commits into
mainfrom
feat/mutation-ergonomics-body-file-close-aliases-resolution
Jun 13, 2026
Merged

unbraind merged 2 commits into
mainfrom
feat/mutation-ergonomics-body-file-close-aliases-resolution

Conversation

@unbraind

@unbraind unbraind commented Jun 13, 2026 •

Copy link
Copy Markdown
Owner

Summary

Reduces agent round-trips on the core mutation surface (our project management = context management philosophy). Three closely-related ergonomics improvements surfaced by the 2026-06-12 dogfooding audit of a downstream project:

Change Issue / pm item
pm create/update --body-file <path> GH-214 / pm-7c48
pm close short aliases -m / -r / -d GH-226 / pm-i1mu
pm close --resolution accepted as the close reason GH-204 (remainder) / pm-7x8d

--body-file (GH-214)

Load a long markdown body (multi-paragraph specs with code blocks/tables) from a file instead of escaping a huge inline --body string or chaining a second pm append:

pm create Feature "Search overhaul" --body-file ./specs/search.md
pm update pm-a1b2 --body-file ./specs/updated.md
  • New src/core/io/body-file.ts resolveBodyFileContent helper — a CLI-only input alias onto the existing body field (no MCP surface: file paths are meaningless over MCP, mirroring pm history --diff).
  • Mutually exclusive with --body; actionable guided errors for conflict / empty-path / unreadable file.
  • Wired into create + update actions, CREATE/UPDATE_FLAG_CONTRACTS, zsh/fish completion, the contracts golden file, and docs.

pm close short aliases (GH-226)

-m (--message), -r (--reason), -d (--duplicate-of). No semantic change — verified no flag conflicts.

--resolution as close reason (pm-7x8d)

Under governance.require_close_reason, when no positional/--reason text is given the close reason is derived from the next-best signal in priority order: explicit reason → --duplicate-of (Duplicate of <id>) → --resolution. The resolution still persists to metadata.resolution, so pm close <id> --resolution "<summary>" no longer hard-blocks.

Testing

  • New 100%-covered unit spec tests/unit/core/io/body-file.spec.ts (placed in a subdirectory to respect the 120-files-per-dir static-gate cap).
  • In-process integration coverage for the create/update/close command wiring (cli-inprocess-runner.integration.spec.ts).
  • close-command precedence unit tests (resolution-as-reason, explicit-reason wins, still-blocks-when-no-source).
  • Full coverage suite green: 3275 tests, aggregate coverage above thresholds.
  • pnpm typecheck, pnpm quality:static, pnpm changelog:pm:check all pass.
  • Manual smoke test in a clean temp tracker confirmed body round-trip (incl. tables/code blocks) and all close paths.

Notes

🤖 Generated with Claude Code

Summary by Sourcery

Add CLI ergonomics for single-call mutations, improve close reason handling, and fix changelog prepend merge behavior.

New Features:

  • Allow pm create and pm update to load item bodies from files via a new --body-file flag, with shell completions and contracts wired up.
  • Add short flag aliases -m, -r, and -d to pm close for message, reason, and duplicate-of options.

Bug Fixes:

  • Ensure changelog prepend merges promote the Unreleased section into the newest generated release instead of duplicating it or misordering backfilled releases.
  • Allow pm close with governance.require_close_reason enabled to derive the close reason from duplicate-of or resolution fields instead of always hard-failing when no explicit reason text is given.

Enhancements:

  • Document new body-file and close command behaviors and update agent guidance for streamlined automation flows.

Build:

  • Refine pm-changelog build scripts to reuse the main changelog generator for check mode and bump the extension version.

Documentation:

  • Document --body-file usage on pm create/update, the new pm close short flags, and resolution-derived close reasons in COMMANDS and AGENT_GUIDE docs.

Tests:

  • Add unit tests for body-file resolution, close-command precedence, and shell completion scripts, plus integration coverage for the new CLI wiring.

Chores:

  • Update pm-changelog extension metadata, manifests, and managed extension state to the new version, and regenerate changelog entries for the new features.

…/-d, --resolution as close reason

Reduce agent round-trips on the core mutation surface (project management =
context management). Three changes from the 2026-06-12 dogfooding audit:

- pm create/update --body-file <path> (GH-214, pm-7c48): load a long markdown
  body from a file instead of escaping a huge inline --body or chaining a second
  pm append. New src/core/io/body-file.ts resolveBodyFileContent helper is a
  CLI-only input alias onto the existing body field (no MCP surface — file paths
  are meaningless over MCP). Mutually exclusive with --body; actionable errors
  for conflict / empty-path / unreadable file. Wired into create+update actions
  before normalize, CREATE/UPDATE_FLAG_CONTRACTS, zsh/fish completion, contracts
  golden, and docs.

- pm close short aliases -m/-r/-d (GH-226, pm-i1mu) for --message/--reason/
  --duplicate-of. No semantic change.

- pm close --resolution as close reason (pm-7x8d): under
  governance.require_close_reason, when no positional/--reason text is given the
  close reason is derived from the next-best signal — explicit reason >
  --duplicate-of ("Duplicate of <id>") > --resolution. The resolution still
  persists to metadata.resolution, so `pm close <id> --resolution "..."` no
  longer hard-blocks.

Tests: 100%-covered src/core/io/body-file.ts unit spec (in tests/unit/core/io/
to respect the 120-file-per-dir cap); in-process integration coverage for the
create/update/close wiring; close-command precedence unit tests. Full coverage
suite green (3275 tests). Closes pm-7c48, pm-i1mu, pm-7x8d.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@sourcery-ai

sourcery-ai Bot commented Jun 13, 2026 •

Copy link
Copy Markdown

Reviewer's Guide

Adds a CLI-only --body-file input alias for pm create/update to load markdown bodies from files, improves pm close ergonomics (short flags and using --resolution as a fallback close reason), and fixes pm-changelog prepend-merge semantics around Unreleased promotion and release ordering, with tests, docs, contracts, completion, and changelog updates.

Sequence diagram for pm create/update with --body-file

sequenceDiagram
  actor User
  participant pm_cli
  participant resolveBodyFileContent
  participant BodyFileReader
  participant runCreate
  participant runUpdate

  User->>pm_cli: pm create ... --body-file path
  pm_cli->>resolveBodyFileContent: resolveBodyFileContent(bodyFile, body)
  resolveBodyFileContent->>resolveBodyFileContent: [inlineBody undefined]
  resolveBodyFileContent->>BodyFileReader: readFile(path)
  BodyFileReader-->>resolveBodyFileContent: bodyContent
  resolveBodyFileContent-->>pm_cli: bodyContent
  pm_cli->>pm_cli: options.body = bodyContent
  alt create command
    pm_cli->>runCreate: runCreate(normalizeCreateOptions)
  else update command
    pm_cli->>runUpdate: runUpdate(id, normalizeUpdateOptions)
  end
  runCreate-->>User: mutation result
  runUpdate-->>User: mutation result

  User->>pm_cli: pm update ... --body-file path
  pm_cli->>resolveBodyFileContent: resolveBodyFileContent(bodyFile, body)
  resolveBodyFileContent->>BodyFileReader: readFile(path)
  BodyFileReader-->>resolveBodyFileContent: bodyContent
  resolveBodyFileContent-->>pm_cli: bodyContent
  pm_cli->>runUpdate: runUpdate(id, normalizeUpdateOptions)
Loading

Sequence diagram for pm close close-reason precedence

sequenceDiagram
  actor User
  participant pm_cli
  participant runClose
  participant assertDuplicateTargetExists
  participant normalizeCloseReason

  User->>pm_cli: pm close id [text] [flags]
  pm_cli->>runClose: runClose(id, options, settings)
  runClose->>assertDuplicateTargetExists: assertDuplicateTargetExists(pmRoot, settings, options.duplicateOf, id)
  assertDuplicateTargetExists-->>runClose: duplicateOf
  runClose->>runClose: hasExplicitReason
  runClose->>runClose: trimmedResolution
  alt [has explicit reason]
    runClose->>runClose: effectiveCloseReasonText = closeReasonText
  else [duplicateOf defined]
    runClose->>runClose: effectiveCloseReasonText = Duplicate of duplicateOf
  else [resolution provided]
    runClose->>runClose: effectiveCloseReasonText = trimmedResolution
  else [no source]
    runClose->>runClose: effectiveCloseReasonText = closeReasonText
  end
  runClose->>normalizeCloseReason: normalizeCloseReason(effectiveCloseReasonText, require_close_reason)
  normalizeCloseReason-->>runClose: closeReason
  runClose-->>User: closed item result
Loading

File-Level Changes

Change Details Files
Add --body-file support for pm create/update as a CLI-only alias onto the existing body field.
  • Introduce src/core/io/body-file.ts with resolveBodyFileContent to read body content from a file, enforcing mutual exclusion with inline --body and providing structured PmCliError errors for empty/invalid paths and unreadable files.
  • Wire --body-file into pm create/update in register-mutation.ts, resolving the file into options.body before normalization and stripping bodyFile from the options payload.
  • Update CLI flag contracts, completions (bash/zsh/fish), integration tests, and the contracts golden fixture to include --body-file, plus new unit tests in tests/unit/core/io/body-file.spec.ts and documentation in COMMANDS.md and AGENT_GUIDE.md.
src/core/io/body-file.ts
src/cli/register-mutation.ts
src/sdk/cli-contracts.ts
src/cli/commands/completion.ts
tests/unit/core/io/body-file.spec.ts
tests/unit/completion-command.spec.ts
tests/integration/cli-inprocess-runner.integration.spec.ts
tests/fixtures/contracts/full.json
docs/COMMANDS.md
docs/AGENT_GUIDE.md
CHANGELOG.md
.agents/pm/features/pm-7c48.toon
.agents/pm/history/pm-7c48.jsonl
Improve pm close ergonomics with short-flag aliases and resolution-as-reason precedence handling under governance.require_close_reason.
  • Expose short aliases -m/--message, -r/--reason, and -d/--duplicate-of on the close command in register-mutation.ts without changing semantics.
  • Extend runClose in src/cli/commands/close.ts to derive the effective close_reason from explicit text, then --duplicate-of, then --resolution when governance.require_close_reason is enabled, while still writing resolution to metadata.resolution.
  • Add unit tests in tests/unit/close-command.spec.ts to cover resolution-as-reason, precedence ordering, and the still-required-reason failure case, and document the behavior in COMMANDS.md and AGENT_GUIDE.md plus changelog and pm history updates.
src/cli/register-mutation.ts
src/cli/commands/close.ts
tests/unit/close-command.spec.ts
docs/COMMANDS.md
docs/AGENT_GUIDE.md
CHANGELOG.md
.agents/pm/tasks/pm-7x8d.toon
.agents/pm/history/pm-7x8d.jsonl
.agents/pm/features/pm-i1mu.toon
.agents/pm/history/pm-i1mu.jsonl
Fix pm-changelog prepend merge behavior around Unreleased promotion and chronological insertion, and update extension packaging/metadata.
  • Modify dist/generator.js so that when generated output has no Unreleased section, an existing leading Unreleased section is promoted into the newest generated release (index 0) instead of being left alongside the new version, using a new UNRELEASED_HEADING_KEY constant and guarding against older sections stealing the pending Unreleased.
  • Introduce insertReleaseSection in dist/generator.js to insert new release sections in chronological order (Unreleased first, then newest-to-oldest by version) rather than always inserting after the title, ensuring backfilled older releases don’t appear above newer ones or Unreleased.
  • Bump pm-changelog version from 2026.6.13 to 2026.6.13-1 in package.json, extension.js, and manifest.json, update the pm-changelog CHANGELOG with the Unreleased-promotion fix, and simplify changelog:check to delegate to the prepend-mode changelog script with --check.
  • Regenerate top-level CHANGELOG.md via pm-changelog and refresh managed extension metadata/artifacts (.managed-extensions.json and sourcemaps/history files).
.agents/pm/extensions/pm-changelog/dist/generator.js
.agents/pm/extensions/pm-changelog/dist/extension.js
.agents/pm/extensions/pm-changelog/dist/extension.js.map
.agents/pm/extensions/pm-changelog/dist/generator.d.ts.map
.agents/pm/extensions/pm-changelog/dist/generator.js.map
.agents/pm/extensions/pm-changelog/CHANGELOG.md
.agents/pm/extensions/pm-changelog/package.json
.agents/pm/extensions/pm-changelog/manifest.json
.agents/pm/extensions/.managed-extensions.json
CHANGELOG.md

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai

coderabbitai Bot commented Jun 13, 2026 •

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@unbraind, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 51 minutes and 44 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: f079936c-ed24-4e85-bc5e-4f2aa6f87d71

📥 Commits

Reviewing files that changed from the base of the PR and between 5e3c469 and 3f92bcc.

📒 Files selected for processing (3)
  • src/cli/register-mutation.ts
  • src/core/io/body-file.ts
  • tests/unit/core/io/body-file.spec.ts
📝 Walkthrough

Walkthrough

This PR implements two CLI enhancements for pm-cli: a --body-file option to load request bodies from files, and short flag aliases for pm close combined with precedence-based close-reason derivation. It includes comprehensive validation, shell completions, updated flag contracts, integration and unit tests, and documentation. Related PM tracking files record the feature implementations and a future audit-scope task.

Changes

CLI Feature Implementation and Enhancement

Layer / File(s) Summary
Body-file resolution helper and validation
src/core/io/body-file.ts, tests/unit/core/io/body-file.spec.ts
New BodyFileReader type and resolveBodyFileContent() function validate mutual exclusivity with inline --body, normalize paths, and wrap read failures as actionable PmCliError instances.
Body-file CLI registration and option resolution
src/cli/register-mutation.ts
--body-file <path> flags added to create and update commands; command actions call resolveBodyFileContent() to merge file content into options.body before downstream normalization.
Close command aliases and close-reason precedence
src/cli/commands/close.ts, src/cli/register-mutation.ts, tests/unit/close-command.spec.ts
-r short alias for --reason added; effectiveCloseReasonText now uses precedence: explicit reason > --duplicate-of string > trimmed --resolution > fallback, with unit tests covering all cases.
Shell completions for body-file flag
src/cli/commands/completion.ts
Zsh and Fish completion definitions add --body-file as a file-completion option for both create and update subcommands.
CLI flag contracts and test fixture updates
src/sdk/cli-contracts.ts, tests/fixtures/contracts/full.json
--body-file added to CREATE_FLAG_CONTRACTS and UPDATE_FLAG_CONTRACTS; full.json fixture updated with matching flag definitions.
Integration tests for body-file and close aliases
tests/integration/cli-inprocess-runner.integration.spec.ts
New test blocks verify body-file round-trip, mutual exclusivity with --body, missing-file errors, and close alias flag parsing (-r for close_reason, -d for duplicate_of).
User and developer documentation updates
CHANGELOG.md, docs/COMMANDS.md, docs/AGENT_GUIDE.md
Root changelog and commands guide document new --body-file usage and close-reason precedence; agent guide adds compact convenience bullets for both features.

Project Management and Extension Updates

Layer / File(s) Summary
Feature tracking and history for body-file and close aliases
.agents/pm/features/pm-7c48.toon, pm-i1mu.toon, .agents/pm/history/pm-7c48.jsonl, pm-i1mu.jsonl
Feature specs and history logs track GH-214 (body-file) and GH-226 (close aliases) through creation, planning, and closure with test coverage notes.
Close-reason precedence task and future audit-update task
.agents/pm/tasks/pm-7x8d.toon, pm-kanu.toon, .agents/pm/history/pm-7x8d.jsonl, pm-kanu.jsonl
Close-reason precedence task marked closed with explicit rules; new open task (GH-207) proposes append-only evidence under --allow-audit-update, with deferred design-tension comment on audit scope.
pm-changelog extension version update and script refactoring
.agents/pm/extensions/.managed-extensions.json, pm-changelog/manifest.json, pm-changelog/package.json, pm-changelog/CHANGELOG.md
Extension version bumped 2026.6.13 → 2026.6.13-1; changelog:check npm script refactored to delegate to existing changelog script.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • unbraind/pm-cli#62: Updates pm-changelog extension and changelog-generation configuration, directly related to this PR's extension version/manifest/script changes.
  • unbraind/pm-cli#67: Extends in-process CLI runner integration tests with new assertions, providing foundation for this PR's integration test additions.
  • unbraind/pm-cli#78: Modifies pm close flow in src/cli/commands/close.ts to handle resolution/expected/actual fields, overlapping with this PR's close-reason precedence logic.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and specifically describes the three main changes: --body-file support, close command short aliases, and --resolution as close reason.
Description check ✅ Passed The PR description comprehensively details all three ergonomics improvements with issue references, implementation details, testing coverage, and deferred work.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@unbraind

Copy link
Copy Markdown
Owner Author

@codex review

@unbraind

Copy link
Copy Markdown
Owner Author

/gemini review

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 2 issues, and left some high level feedback:

  • For consistency with other flags like --close-reason/--close_reason, consider adding the --body_file alias to CREATE_FLAG_CONTRACTS/UPDATE_FLAG_CONTRACTS (and any related parsing paths) so the new --body-file option also supports the underscore variant.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- For consistency with other flags like `--close-reason`/`--close_reason`, consider adding the `--body_file` alias to `CREATE_FLAG_CONTRACTS`/`UPDATE_FLAG_CONTRACTS` (and any related parsing paths) so the new `--body-file` option also supports the underscore variant.

## Individual Comments

### Comment 1
<location path="src/core/io/body-file.ts" line_range="51-38" />
<code_context>
+  }
+  try {
+    return await readFileImpl(path);
+  } catch {
+    throw new PmCliError(
+      `--body-file could not read "${path}". Check that the file exists and is readable.`,
+      EXIT_CODE.NOT_FOUND,
+      {
+        code: "body_file_unreadable",
+        required: "Point --body-file at an existing, readable file.",
+        why: "The body content is loaded from the file at the supplied path, so an unreadable path cannot produce a body.",
+        nextSteps: [`Verify the path: ls -l "${path}"`, "Or pass the body inline with --body <text>."],
+      },
+    );
+  }
</code_context>
<issue_to_address>
**suggestion:** Preserve the underlying file error as context on the PmCliError

The catch block replaces all underlying errors with a generic NOT_FOUND, which hides important distinctions (e.g., permission denied vs ENOENT vs EISDIR) that are useful for debugging and support. Consider catching `error: unknown` and either setting it as the `cause` of `PmCliError` or including a minimal, sanitized snippet (e.g., `String(error)` or `(error as NodeJS.ErrnoException).code`) in the metadata to retain root-cause detail without compromising UX.

Suggested implementation:

```typescript
  const path = bodyFile.trim();
  if (path.length === 0) {
    throw new PmCliError("--body-file requires a file path.", EXIT_CODE.USAGE, {
      code: "body_file_missing_path",
      required: "Pass a readable file path: --body-file <path>.",
      why: "An empty --body-file value has no file to read the body from.",
    });
  }
  try {
    return await readFileImpl(path);
  } catch (error: unknown) {
    const fileError = error as NodeJS.ErrnoException | Error | undefined;

    throw new PmCliError(
      `--body-file could not read "${path}". Check that the file exists and is readable.`,
      EXIT_CODE.NOT_FOUND,
      {
        code: "body_file_unreadable",
        required: "Point --body-file at an existing, readable file.",
        why: "The body content is loaded from the file at the supplied path, so an unreadable path cannot produce a body.",
        nextSteps: [`Verify the path: ls -l "${path}"`, "Or pass the body inline with --body <text>."],
        fileErrorCode: (fileError as NodeJS.ErrnoException | undefined)?.code,
        fileErrorMessage: fileError && String(fileError),
      },
    );
  }

```

If `PmCliError` supports a `cause` option (e.g., via `ErrorOptions`), you may also want to:
1. Extend the metadata object or constructor usage to include `cause: error`.
2. Or, if `PmCliError`’s signature is `(message, exitCode, options)` where `options` already supports `cause`, add `cause: error` to the options object alongside `code`, `required`, etc.
</issue_to_address>

### Comment 2
<location path="tests/unit/core/io/body-file.spec.ts" line_range="8-14" />
<code_context>
+import { resolveBodyFileContent } from "../../../../src/core/io/body-file.js";
+import { PmCliError } from "../../../../src/core/shared/errors.js";
+
+async function expectRejectionCode(promise: Promise<unknown>, code: string): Promise<void> {
+  await expect(promise).rejects.toBeInstanceOf(PmCliError);
+  await promise.catch((error: unknown) => {
+    expect(error).toBeInstanceOf(PmCliError);
+    expect((error as PmCliError).context.code).toBe(code);
+  });
+}
+
</code_context>
<issue_to_address>
**suggestion:** Simplify `expectRejectionCode` to avoid double-consuming the promise and tighten failure diagnostics

This helper both awaits `expect(promise).rejects...` and then `promise.catch(...)`, effectively consuming the rejection twice. To avoid surprising behavior and keep the assertion path single and clearer, consider using only `await expect(promise).rejects.toMatchObject({ context: { code } });` (or similar) and dropping the manual `catch`. This keeps diagnostics focused and easier to interpret.

```suggestion
async function expectRejectionCode(promise: Promise<unknown>, code: string): Promise<void> {
  await expect(promise).rejects.toMatchObject({
    context: expect.objectContaining({ code }),
  });
}
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread src/core/io/body-file.ts Resolved
Comment on lines +8 to +14
async function expectRejectionCode(promise: Promise<unknown>, code: string): Promise<void> {
await expect(promise).rejects.toBeInstanceOf(PmCliError);
await promise.catch((error: unknown) => {
expect(error).toBeInstanceOf(PmCliError);
expect((error as PmCliError).context.code).toBe(code);
});
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Simplify expectRejectionCode to avoid double-consuming the promise and tighten failure diagnostics

This helper both awaits expect(promise).rejects... and then promise.catch(...), effectively consuming the rejection twice. To avoid surprising behavior and keep the assertion path single and clearer, consider using only await expect(promise).rejects.toMatchObject({ context: { code } }); (or similar) and dropping the manual catch. This keeps diagnostics focused and easier to interpret.

Suggested change
async function expectRejectionCode(promise: Promise<unknown>, code: string): Promise<void> {
await expect(promise).rejects.toBeInstanceOf(PmCliError);
await promise.catch((error: unknown) => {
expect(error).toBeInstanceOf(PmCliError);
expect((error as PmCliError).context.code).toBe(code);
});
}
async function expectRejectionCode(promise: Promise<unknown>, code: string): Promise<void> {
await expect(promise).rejects.toMatchObject({
context: expect.objectContaining({ code }),
});
}

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several enhancements to the project management CLI, including a new --body-file option for loading item bodies from files, short aliases for pm close flags, and fallback logic to derive close reasons from resolutions or duplicates under strict governance. It also improves the changelog generator to chronologically insert release sections and promote the Unreleased section during releases. The review feedback suggests making the mutual exclusion validation for --body and --body-file more robust by checking if options.body is defined rather than strictly checking if it is a string, which prevents non-string values from bypassing conflict checks.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/cli/register-mutation.ts Outdated
if (typeof options.bodyFile === "string") {
options.body = await resolveBodyFileContent(
options.bodyFile,
typeof options.body === "string" ? options.body : undefined,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using options.body !== undefined instead of typeof options.body === "string" makes the mutual exclusion robust against cases where options.body might be parsed as a non-string value (such as a boolean). If options.body is defined but not a string, the current implementation passes undefined to resolveBodyFileContent, which silently bypasses the conflict validation.

Suggested change
typeof options.body === "string" ? options.body : undefined,
options.body !== undefined ? String(options.body) : undefined,

Comment thread src/cli/register-mutation.ts Outdated
if (typeof options.bodyFile === "string") {
options.body = await resolveBodyFileContent(
options.bodyFile,
typeof options.body === "string" ? options.body : undefined,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using options.body !== undefined instead of typeof options.body === "string" makes the mutual exclusion robust against cases where options.body might be parsed as a non-string value (such as a boolean). If options.body is defined but not a string, the current implementation passes undefined to resolveBodyFileContent, which silently bypasses the conflict validation.

Suggested change
typeof options.body === "string" ? options.body : undefined,
options.body !== undefined ? String(options.body) : undefined,

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several enhancements to the pm CLI tool. Key additions include a new --body-file <path> option for pm create and pm update to load markdown bodies from a file, and short aliases (-m, -r, -d) for the pm close command. Additionally, pm close now fallback-derives the close reason from the --resolution summary when require_close_reason is enabled and no explicit reason is provided. The pm-changelog extension was also updated to version 2026.6.13-1 to promote the Unreleased section correctly and sort release sections chronologically during merges. I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
tests/unit/completion-command.spec.ts (1)

98-117: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Mirror the new flag in the zsh/fish completion assertions too.

Right now this spec only verifies --body-file in the bash generator. Since the PR wires the flag into zsh and fish as well, add matching assertions there to avoid shell-specific regressions slipping through.

🤖 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 98 - 117, The tests
currently assert that the bash completion script (via generateBashScript)
contains "--body-file" but the zsh and fish completion assertions were not
updated; add matching assertions to the zsh and fish checks by calling
generateZshScript and generateFishScript (or the existing zsh/fish script
helpers in the file) and adding expect(zshScript).toContain("--body-file") and
expect(fishScript).toContain("--body-file") in their respective "includes ..."
tests so all three shells verify the new flag.
🤖 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 `@tests/integration/cli-inprocess-runner.integration.spec.ts`:
- Around line 22-24: The test is asserting the body at the top-level payload but
the CLI returns an envelope { item: ... }, so update the assertions to read the
body from fetched.json.item.body (not fetched.json.body); specifically change
the assertion that uses fetched.json as { body?: string } to access
fetched.json.item.body and do the same for the second occurrence (lines 32–33)
so the tests compare bodyContent against fetched.json.item.body.

---

Outside diff comments:
In `@tests/unit/completion-command.spec.ts`:
- Around line 98-117: The tests currently assert that the bash completion script
(via generateBashScript) contains "--body-file" but the zsh and fish completion
assertions were not updated; add matching assertions to the zsh and fish checks
by calling generateZshScript and generateFishScript (or the existing zsh/fish
script helpers in the file) and adding
expect(zshScript).toContain("--body-file") and
expect(fishScript).toContain("--body-file") in their respective "includes ..."
tests so all three shells verify the new 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: 5d7629a0-638d-4ba1-be55-82eb90bc15d1

📥 Commits

Reviewing files that changed from the base of the PR and between a09dc22 and 5e3c469.

⛔ Files ignored due to path filters (5)
  • .agents/pm/extensions/pm-changelog/dist/extension.js is excluded by !**/dist/**
  • .agents/pm/extensions/pm-changelog/dist/extension.js.map is excluded by !**/dist/**, !**/*.map
  • .agents/pm/extensions/pm-changelog/dist/generator.d.ts.map is excluded by !**/dist/**, !**/*.map
  • .agents/pm/extensions/pm-changelog/dist/generator.js is excluded by !**/dist/**
  • .agents/pm/extensions/pm-changelog/dist/generator.js.map is excluded by !**/dist/**, !**/*.map
📒 Files selected for processing (25)
  • .agents/pm/extensions/.managed-extensions.json
  • .agents/pm/extensions/pm-changelog/CHANGELOG.md
  • .agents/pm/extensions/pm-changelog/manifest.json
  • .agents/pm/extensions/pm-changelog/package.json
  • .agents/pm/features/pm-7c48.toon
  • .agents/pm/features/pm-i1mu.toon
  • .agents/pm/history/pm-7c48.jsonl
  • .agents/pm/history/pm-7x8d.jsonl
  • .agents/pm/history/pm-i1mu.jsonl
  • .agents/pm/history/pm-kanu.jsonl
  • .agents/pm/tasks/pm-7x8d.toon
  • .agents/pm/tasks/pm-kanu.toon
  • CHANGELOG.md
  • docs/AGENT_GUIDE.md
  • docs/COMMANDS.md
  • src/cli/commands/close.ts
  • src/cli/commands/completion.ts
  • src/cli/register-mutation.ts
  • src/core/io/body-file.ts
  • src/sdk/cli-contracts.ts
  • tests/fixtures/contracts/full.json
  • tests/integration/cli-inprocess-runner.integration.spec.ts
  • tests/unit/close-command.spec.ts
  • tests/unit/completion-command.spec.ts
  • tests/unit/core/io/body-file.spec.ts

Comment on lines +22 to +24
const fetched = await context.runCliInProcess(["get", createdId, "--json"], { expectJson: true });
expect((fetched.json as { body?: string }).body).toBe(bodyContent);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fix get --json body assertions to read from item.body.

These assertions read body from the top-level payload, but this suite also treats get --json as an { item: ... } envelope. Using the wrong path can fail the test even when --body-file behavior is correct.

💡 Proposed fix
-      expect((fetched.json as { body?: string }).body).toBe(bodyContent);
+      expect((fetched.json as { item: { body?: string } }).item.body).toBe(bodyContent);
@@
-      expect((refetched.json as { body?: string }).body).toBe("updated from file");
+      expect((refetched.json as { item: { body?: string } }).item.body).toBe("updated from file");

Also applies to: 32-33

🤖 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/integration/cli-inprocess-runner.integration.spec.ts` around lines 22 -
24, The test is asserting the body at the top-level payload but the CLI returns
an envelope { item: ... }, so update the assertions to read the body from
fetched.json.item.body (not fetched.json.body); specifically change the
assertion that uses fetched.json as { body?: string } to access
fetched.json.item.body and do the same for the second occurrence (lines 32–33)
so the tests compare bodyContent against fetched.json.item.body.

…ntext)

- register-mutation: use `options.body !== undefined ? String(options.body)`
  for the --body/--body-file mutual-exclusion check so a non-string body value
  can't silently bypass the conflict guard (Gemini).
- body-file: preserve the underlying errno in the unreadable-file error and give
  a directory-specific message for EISDIR (a real footgun when pointing
  --body-file at a folder); keeps the typed PmCliError context shape (Sourcery).
- body-file.spec: simplify expectRejectionCode to a single rejects assertion
  (no double-consume) and cover the new errno/EISDIR branches.

CodeRabbit's "read item.body" suggestion is a false positive: `pm get --json`
returns `body` at the top level (siblings: item/body/linked/claim_state), which
the passing tests confirm.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@unbraind

Copy link
Copy Markdown
Owner Author

Thanks for the reviews — addressed in 3f92bcc:

  • Gemini (mutual-exclusion robustness): adopted options.body !== undefined ? String(options.body) : undefined at both --body-file sites.
  • Sourcery (error context): the unreadable-file error now preserves the underlying errno and emits a directory-specific message for EISDIR; test helper simplified to a single rejects assertion.
  • CodeRabbit (read item.body): this is a false positive — pm get --json returns body at the top level (siblings: item, body, linked, claim_state), not nested under item. The assertions read fetched.json.body and pass; switching to item.body would break them. Left as-is.

@codex review
/gemini review

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several enhancements to the PM CLI. It adds a --body-file option to pm create and pm update for loading markdown bodies from files, introduces short aliases (-m, -r, -d) for pm close, and allows deriving the close reason from the resolution summary when a close reason is required. Additionally, the pm-changelog extension is updated to promote stale Unreleased sections and insert release sections chronologically. Feedback is provided regarding the robustness of the version comparison logic in the changelog generator, suggesting a fallback mechanism to handle non-SemVer strings safely.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +246 to +259
for (const [index, releaseSection] of releaseSections.entries()) {
const replacement = releaseSection.markdown.trimEnd();
const replaced = replaceReleaseSection(next, releaseSection.heading, replacement);
if (replaced.replaced) {
next = replaced.markdown;
action = "replaced";
continue;
}
next = insertAfterTitle(next, replacement);
// Restrict promotion to the newest generated section (index 0). Otherwise,
// when the newest version already exists (replaced above) an older section
// that is missing could steal the pending `## Unreleased` and overwrite it,
// losing the pending changes (GH #48 review).
if (canPromoteUnreleased && index === 0) {
const promoted = replaceReleaseSection(next, "Unreleased", replacement);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If compareVersionStrings is called with version strings that do not strictly conform to SemVer (for example, custom release tags or unbracketed date-based versions), it might throw an error and crash the changelog generation process. Wrapping the version comparison in a try-catch block with a fallback (such as localeCompare) would make the sorting more robust against non-standard version headings.

    for (const [index, releaseSection] of releaseSections.entries()) {
        const replacement = releaseSection.markdown.trimEnd();
        const replaced = replaceReleaseSection(next, releaseSection.heading, replacement);
        if (replaced.replaced) {
            next = replaced.markdown;
            action = "replaced";
            continue;
        }
        // Restrict promotion to the newest generated section (index 0). Otherwise,
        // when the newest version already exists (replaced above) an older section
        // that is missing could steal the pending `## Unreleased` and overwrite it,
        // losing the pending changes (GH #48 review).
        if (canPromoteUnreleased && index === 0) {
            const promoted = replaceReleaseSection(next, "Unreleased", replacement);
            if (promoted.replaced) {
                next = promoted.markdown;
                action = "replaced";
                continue;
            }
        }
        next = insertReleaseSection(next, releaseSection.heading, replacement);
        if (action !== "replaced")
            action = "inserted";
    }

@unbraind

Copy link
Copy Markdown
Owner Author

Re: Gemini's note on .agents/pm/extensions/pm-changelog/dist/generator.js (compareVersionStrings) — non-finding, and it's vendored compiled output of the separate pm-changelog package (refreshed by changelog:pm:install), not code authored in this repo. The function already implements exactly the suggested hardening: it splits on ./-, parses each segment with parseInt guarded by Number.isNaN, and falls back to localeCompare for non-numeric segments — so it can't throw on non-SemVer/date-based tags (e.g. 2026.6.13). The changelog:pm:check gate passing on this repo's real tags confirms it. No change needed.

Codex is not connected to this repo (the connector replies with a setup link), so no Codex review is available.

All actionable review findings are addressed and every check is green.

@unbraind
unbraind merged commit 2fd2f18 into main Jun 13, 2026
15 checks passed
@unbraind
unbraind deleted the feat/mutation-ergonomics-body-file-close-aliases-resolution branch June 13, 2026 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant