Repository navigation
Compact mutation output + smarter unknown-flag recovery (pm-ch59, pm-8nyc) - #72
Conversation
…9, pm-8nyc) Two agent-UX / token-efficiency improvements under pm-rnpb, combined into one PR. pm-ch59 — compact mutation output: - New pure helper src/core/output/mutation-projection.ts (projectMutationResult) replaces the verbose changed_fields array with a deterministic changed_field_count. - Global --no-changed-fields CLI flag, threaded GlobalOptions -> OutputOptions -> printResult, so it applies uniformly to every mutation command. - MCP agent path (create/update/close/append/update-many) is compact-by-default; pass options.full=true to restore the full changed_fields delta. pm_create/ pm_update/pm_close tool descriptions document this. - Root-cause fix in extractCommandScopedOptions: strip the new global so it never leaks into per-command update fields (keeps update-many rollback + update no-op detection correct). - Discoverable via pm contracts (SUBCOMMAND_GLOBAL_FLAG_CONTRACTS) and bash/zsh/fish completion. pm-8nyc — unknown-option recovery: - Cross-command flag awareness: unknown flags valid elsewhere now hint the right command (e.g. --type on test-all -> create/list/list-all; --fields on create -> list/get), built from a declaration-ordered flag->commands index. - Prefix/abbreviation-aware nearest-flag suggestions (--desc -> --description). - list --sort aliases: updated->updated_at, created->created_at, with updated help and invalid-value error text. - Semantic map extended: update/create --add-tags/--add-tag/--tag -> --tags (existing --body -> --add retained). Also: refresh pm-changelog 2026.5.25 -> 2026.5.26 and regenerate CHANGELOG.md; docs (COMMANDS.md, AGENT_GUIDE.md) updated; redact an absolute-home-path that a sub-agent quoted into a pm-rnpb comment (audited pm history-redact). Verification: full coverage gate green (1819 tests, 100% stmts/branches/funcs/lines), quality:static, security:scan, version:check, typecheck, npm pack --dry-run, docs-skills gate, claude/codex plugin smokes, changelog:pm:check, and a 16-check temp-dir dogfood all pass. Closes pm-ch59, pm-8nyc. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
@codex review |
|
Warning Review limit reached
More reviews will be available in 11 minutes and 16 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 (7)
📝 WalkthroughWalkthroughThis PR delivers two feature completions: ChangesMutation Output Compaction
Unknown Option Recovery Improvements
PM Metadata, Plan Coordination, and Release
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant GlobalOpts as GlobalOptions
participant Output as printResult
participant Projection as projectMutationResult
participant Stdout as Stdout
CLI->>GlobalOpts: extract noChangedFields
CLI->>Output: printResult(result, options)
Output->>Projection: projectMutationResult(result, {changedFields: "compact"})
alt Compact Mode
Projection->>Projection: Remove changed_fields, add changed_field_count
else Full Mode
Projection->>Projection: Return unchanged
end
Projection->>Output: Projected result
Output->>Stdout: Print formatted output
sequenceDiagram
participant User as User
participant CLI as CLI Parser
participant Usage as CommanderUsageContext
participant Index as Flag->Command Index
participant Guidance as Error Guidance
participant Envelope as JSON Error Envelope
User->>CLI: Provide unknown --option
CLI->>Usage: resolveCommanderUsageContext
Usage->>Index: Query which commands accept --option
Index->>Usage: Return up to 3 other commands
Usage->>Guidance: Pass unknownOptionOtherCommands
Guidance->>Envelope: Generate unknown_option with next_steps hint
Envelope->>User: "flag valid on: create, list, list-all"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several agent-UX and token-efficiency enhancements. It adds a --no-changed-fields global CLI flag and compacts MCP mutation outputs by default to replace the verbose changed_fields array with a compact changed_field_count. It also improves unknown-option error recovery by suggesting nearest flags (including prefix/abbreviation matches like --desc to --description), providing cross-command flag hints, and mapping additive tag flags to --tags. Additionally, it introduces convenience aliases for list sorting (updated to updated_at and created to created_at). Comprehensive integration and unit tests have been added to verify these behaviors. There are no review comments to address.
There was a problem hiding this comment.
Code Review
This pull request introduces several agent-UX and token-efficiency improvements. It adds a global --no-changed-fields CLI flag and makes MCP mutation tools compact-by-default, replacing the verbose changed_fields array with a changed_field_count unless options.full is requested. It also enhances unknown-option recovery by supporting prefix/abbreviation suggestions (e.g., --desc to --description), providing cross-command flag hints, and mapping tag-related flags to --tags. Additionally, it introduces convenience aliases for --sort fields (such as updated to updated_at). The code reviewer identified a potential prototype pollution vulnerability in the sort field alias lookup that should be addressed by ensuring the resolved alias is a string.
| const aliased = LIST_SORT_FIELD_ALIASES[normalized]; | ||
| if (aliased) { | ||
| return aliased; | ||
| } |
There was a problem hiding this comment.
Using direct property lookup on a plain object with user-controlled input (normalized) can lead to prototype pollution or unexpected behavior if the input matches standard Object.prototype properties (e.g., toString, constructor, hasOwnProperty). Since LIST_SORT_FIELD_ALIASES is a plain object, we should safely check if the resolved alias is a string before returning it.
| const aliased = LIST_SORT_FIELD_ALIASES[normalized]; | |
| if (aliased) { | |
| return aliased; | |
| } | |
| const aliased = LIST_SORT_FIELD_ALIASES[normalized]; | |
| if (typeof aliased === "string") { | |
| return aliased; | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/chores/pm-ch59.toon:
- Around line 2-18: This change directly edits PM system-of-record entries under
.agents/pm, which is prohibited; revert those direct edits and recreate the
chore via the PM command workflow instead (use the pm CLI to create or update
the chore so the tool records the entry), ensuring the new feature/flag work
remains in your branch; if you need to reference implementation symbols for
locating the logic to keep (not edit in .agents), keep changes confined to the
code under the feature branch (e.g., projectMutationResult,
GlobalOptions.noChangedFields, printResult,
registration-helpers.getGlobalOptions, SUBCOMMAND_GLOBAL_FLAG_CONTRACTS) and
only generate/update the .agents/pm record by running the pm create/update
commands so the system-of-record is authored by the pm tooling rather than by
direct file edits.
In `@src/core/output/mutation-projection.ts`:
- Around line 35-47: projectMutationResult currently only compacts when the
top-level object contains CHANGED_FIELDS_KEY, so nested results (e.g.,
runUpdateMany rows) are not transformed; change projectMutationResult to handle
arrays and nested objects recursively: if result is an Array, return
result.map(r => projectMutationResult(r, options)); if result is a plain object,
first check for CHANGED_FIELDS_KEY and apply the existing compaction (return
rest plus CHANGED_FIELD_COUNT_KEY), otherwise iterate object properties and
recursively call projectMutationResult on each value to ensure any nested
changed_fields are compacted; keep the existing mode logic
(options.changedFields) and use the same CHANGED_FIELDS_KEY and
CHANGED_FIELD_COUNT_KEY symbols.
🪄 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: cdbf051d-a846-456e-b52b-28f46e5305ae
⛔ Files ignored due to path filters (9)
.agents/pm/extensions/pm-changelog/dist/cli.jsis excluded by!**/dist/**.agents/pm/extensions/pm-changelog/dist/cli.js.mapis excluded by!**/dist/**,!**/*.map.agents/pm/extensions/pm-changelog/dist/extension.d.ts.mapis excluded by!**/dist/**,!**/*.map.agents/pm/extensions/pm-changelog/dist/extension.jsis excluded by!**/dist/**.agents/pm/extensions/pm-changelog/dist/extension.js.mapis excluded by!**/dist/**,!**/*.map.agents/pm/extensions/pm-changelog/dist/release-context.d.tsis excluded by!**/dist/**.agents/pm/extensions/pm-changelog/dist/release-context.d.ts.mapis excluded by!**/dist/**,!**/*.map.agents/pm/extensions/pm-changelog/dist/release-context.jsis excluded by!**/dist/**.agents/pm/extensions/pm-changelog/dist/release-context.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (36)
.agents/pm/chores/pm-ch59.toon.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-rnpb.toon.agents/pm/history/pm-70mi.jsonl.agents/pm/history/pm-8nyc.jsonl.agents/pm/history/pm-ch59.jsonl.agents/pm/history/pm-rnpb.jsonl.agents/pm/issues/pm-8nyc.toon.agents/pm/plans/pm-70mi.toonCHANGELOG.mddocs/AGENT_GUIDE.mddocs/COMMANDS.mdsrc/cli/commander-usage.tssrc/cli/commands/completion.tssrc/cli/commands/list.tssrc/cli/error-guidance.tssrc/cli/main.tssrc/cli/register-list-query.tssrc/cli/registration-helpers.tssrc/core/output/mutation-projection.tssrc/core/output/output.tssrc/core/shared/command-types.tssrc/mcp/server.tssrc/sdk/cli-contracts.tstests/integration/help-runtime.spec.tstests/integration/mcp-dynamic-package-actions.spec.tstests/unit/completion-command.spec.tstests/unit/error-guidance.spec.tstests/unit/list-command.spec.tstests/unit/mutation-projection.spec.tstests/unit/output.spec.tstests/unit/registration-helpers.spec.tsvitest.config.ts
| title: Add --no-changed-fields flag and compact MCP mutation output to drop the redundant changed_fields array | ||
| description: "Repro: PM_AUTHOR=x pm create issue 'probe' (default TOON). Observed: 21-line output where 11 lines are a 'changed_fields' array listing id,title,description,type,status,priority,tags,created_at,updated_at,author on EVERY create; update emits the same pattern. The item body directly above already shows the set values, so changed_fields is largely redundant for the create case. Expected: a way to trim mutation output for agent loops, e.g. accept --fields (already works on get, rejected on create with 'Unknown option --fields'), or a --quiet/--no-changed-fields, or drop the full changed_fields list on create (keep it on update where it signals the delta). Severity: low (token efficiency in high-volume agent create loops; ~50% of create payload is the redundant array). Note: prior verbosity item pm-3pbs was scoped to pm activity/history only, not mutations." | ||
| type: Chore | ||
| status: open | ||
| status: closed | ||
| priority: 2 | ||
| tags[3]: agent-ux,dogfood,dx | ||
| created_at: "2026-05-25T18:30:34.477Z" | ||
| updated_at: "2026-05-25T20:25:05.565Z" | ||
| updated_at: "2026-05-25T23:47:52.695Z" | ||
| author: dogfood-agent | ||
| parent: pm-rnpb | ||
| comments[4]{created_at,author,text}: | ||
| comments[5]{created_at,author,text}: | ||
| "2026-05-25T18:30:41.628Z",dogfood-agent,"Dedup checks run against real tracker (no PM_PATH override) on 2026-05-25 before filing: (1) pm search 'create update changed_fields verbose suppress quiet flag' --limit 12 -> hits pm-rnpb(in_progress parent), pm-5k2w(init verbosity, closed), pm-swie/pm-ote/pm-j1v7/pm-phob/pm-kpz5/pm-wo8/pm-ysgr/pm-mfza/pm-06t (all unrelated create/update feature work, closed) -- none about output verbosity/changed_fields suppression. (2) pm search 'mutation output quiet compact' -> pm-3pbs (closed) reduces verbosity of pm ACTIVITY/HISTORY and adds compact mode to history, NOT create/update mutations -- scope differs. (3) pm list --status open --limit 30 -> pm-59gj,pm-gt82,pm-3cbk (none related). (4) pm list --status in-progress --limit 20 -> pm-rnpb,pm-7rlp,pm-cf1u (none related). Conclusion: no open or recently-closed item covers suppressing/trimming create/update mutation output. Filed as new under pm-rnpb." | ||
| "2026-05-25T19:47:52.743Z",codex-full-audit,"Full audit confirmation: mutation JSON output remains a real token-efficiency issue and is not solved in this slice. Manual/sub-agent dogfood still shows bulky changed_fields/linked object payloads as a high-value optimization for agents. This pass focused on command recovery, package validation, SDK/package boundary, and vector cache correctness; leave pm-ch59 open for explicit compact mutation-output projection work." | ||
| "2026-05-25T19:56:59.534Z",manual-dogfood-specialist,"Manual dogfood evidence 2026-05-25: create/update JSON outputs in the temp LaunchOps project still repeatedly emitted large changed_fields arrays. On first create, changed_fields listed id/title/description/type/status/priority/tags/created_at/updated_at/deadline/author/body; update with comment/note/learning emitted a full item plus changed_fields. This confirms the open token-cost concern remains relevant for agents; fields projections help reads but mutation outputs still need a compact/default projection or a way to suppress changed_fields without losing deterministic mutation evidence.\n" | ||
| "2026-05-25T20:25:05.565Z",manual-dogfood-subagent,"Manual dogfood reconfirmed mutation output verbosity: create/update/claim/close/comment-style commands returned full item/comment payloads by default. In small temp project this was usable, but comments on pm-rnpb returned a massive historical comment array, reinforcing the need for compact mutation acknowledgements / suppressible changed_fields and append outputs for token efficiency." | ||
| "2026-05-25T23:13:10.074Z",claude-code-agent,"IMPLEMENTED (branch agent-ux-compact-mutation-output-and-flag-recovery). Approach: shared pure helper src/core/output/mutation-projection.ts projectMutationResult() drops the verbose changed_fields array and replaces it with changed_field_count (deterministic mutation evidence, ~no token cost). Two surfaces: (1) CLI global flag --no-changed-fields (main.ts opt) -> GlobalOptions.noChangedFields (command-types.ts + registration-helpers.getGlobalOptions opts.changedFields===false) -> OutputOptions.noChangedFields -> printResult projects before formatOutput; applies uniformly to ALL mutation commands since they route through printResult. (2) MCP agent path: runAction create/update/close/append/update-many are compact-by-default (changed_field_count) unless options.full=true (withMutationCompaction strips the full flag before the runner); pm_create/pm_update/pm_close tool descriptions document it. Discoverability: added --no-changed-fields to SUBCOMMAND_GLOBAL_FLAG_CONTRACTS (pm contracts) + completion (bash global tail x26, zsh _arguments global block, fish global defs). Coverage: helper added to vitest include (100% via tests/unit/mutation-projection.spec.ts). Tests: mutation-projection unit (6 cases), output.spec printResult noChangedFields, mcp-dynamic compact-by-default+full opt-out. Manual smoke (temp dir): default keeps changed_fields; --no-changed-fields -> changed_field_count:10 (TOON+JSON); default --json still has changed_fields (backward compatible)." | ||
| close_reason: "Shipped on branch agent-ux-compact-mutation-output-and-flag-recovery. Added shared projectMutationResult helper (src/core/output/mutation-projection.ts, 100% covered), global --no-changed-fields CLI flag, and MCP compact-by-default for create/update/close/append/update-many (opt back in with options.full). Root-cause fix: extractCommandScopedOptions strips the new global so it never counts as an update field (fixes update-many rollback + update no-op). Full coverage gate green (1819 tests, 100%), 16/16 dogfood checks pass, check-secrets clean." |
There was a problem hiding this comment.
Do not directly modify PM system-of-record files.
Line 2 through Line 18 are direct edits under .agents/pm/**, which violates repo policy and should be done via the PM command workflow that generates these records.
As per coding guidelines: .agents/pm/**: pm is the system of record. Do not edit .agents/pm files directly.
🤖 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/chores/pm-ch59.toon around lines 2 - 18, This change directly
edits PM system-of-record entries under .agents/pm, which is prohibited; revert
those direct edits and recreate the chore via the PM command workflow instead
(use the pm CLI to create or update the chore so the tool records the entry),
ensuring the new feature/flag work remains in your branch; if you need to
reference implementation symbols for locating the logic to keep (not edit in
.agents), keep changes confined to the code under the feature branch (e.g.,
projectMutationResult, GlobalOptions.noChangedFields, printResult,
registration-helpers.getGlobalOptions, SUBCOMMAND_GLOBAL_FLAG_CONTRACTS) and
only generate/update the .agents/pm record by running the pm create/update
commands so the system-of-record is authored by the pm tooling rather than by
direct file edits.
…e-safe sort aliases - CodeRabbit (Major): projectMutationResult only compacted top-level changed_fields, so update-many (which reports rows[*].changed_fields) stayed verbose. Made the projection recursive so --no-changed-fields / MCP compact mode trims changed_fields wherever it appears; returns the same reference when nothing is compacted. - Gemini (High): the list --sort alias lookup used direct property access on a plain object with user input, enabling prototype-chain hits (__proto__/constructor/...). Switched to a Map so unknown/prototype keys resolve to undefined. - Tests: nested update-many-style compaction + no-op same-reference case; sort alias rejects __proto__/constructor/toString. Full coverage gate green (1821 tests, 100%). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Addressed review feedback in 7e4f9ea:
Full coverage gate green (1821 tests, 100%). @codex review |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Code Review
This pull request introduces several agent-UX and token-efficiency improvements. It adds a shared projectMutationResult helper to compact mutation command outputs by dropping the verbose changed_fields array and replacing it with changed_field_count. This compaction is exposed via a new global CLI flag --no-changed-fields and is enabled by default on the MCP agent path (with an opt-out option). Additionally, it enhances unknown-option error recovery by suggesting nearest flags (including prefix/abbreviation matches), adding cross-command flag awareness hints, extending the semantic flag map, and introducing convenience aliases for --sort fields (updated and created). Feedback was provided to harden the isPlainObject check in the new mutation projection helper to prevent prototype loss on non-plain objects.
| function isPlainObject(value: unknown): value is Record<string, unknown> { | ||
| return typeof value === "object" && value !== null && !Array.isArray(value); | ||
| } |
There was a problem hiding this comment.
The current implementation of isPlainObject returns true for any non-null object that is not an array. This includes built-in objects like Date, RegExp, Map, Set, or custom class instances. If any such object is traversed recursively by compactChangedFields, it will be treated as a plain object, and its prototype will be lost (e.g., a Date object would be incorrectly mapped to an empty plain object {}).
Using a stricter check that verifies the prototype is either null or Object.prototype prevents prototype loss and ensures robust handling of non-plain objects.
function isPlainObject(value: unknown): value is Record<string, unknown> {
if (typeof value !== "object" || value === null || Array.isArray(value)) {
return false;
}
const proto = Object.getPrototypeOf(value);
return proto === null || proto === Object.prototype;
}Gemini (medium): the recursive changed_fields projection used a loose isPlainObject
that returned true for Date/RegExp/Map/Set/class instances, so recursing into such
a value would rewrite it to {} and lose its prototype. Tighten the check to only
traverse genuine plain objects (prototype null or Object.prototype); built-ins and
class instances are now returned unchanged. Added regression tests (Date preserved,
null-prototype object still compacted).
Full coverage gate green (100%).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Addressed Gemini's medium finding in d596dbe: @codex review |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Code Review
This pull request introduces a shared mutation-result projection helper (projectMutationResult) that compacts verbose changed_fields arrays into a simple count (changed_field_count) for improved token efficiency in agent loops. This compaction is exposed via a new global CLI flag --no-changed-fields and is enabled by default on MCP mutation tools. Additionally, it enhances CLI error recovery by introducing prefix/abbreviation-aware flag suggestions, cross-command flag hints, and sort field aliases for list commands. The review feedback suggests preserving the prototype of null-prototype objects during compaction and adding corresponding test assertions.
| return value; | ||
| } | ||
|
|
||
| let changed = false; |
There was a problem hiding this comment.
When compacting a plain object that has a null prototype (e.g., created via Object.create(null)), initializing projected as {} will inadvertently assign Object.prototype as its prototype. To preserve the original prototype of the object, initialize projected dynamically based on the prototype of value.
| let changed = false; | |
| const projected: Record<string, unknown> = Object.getPrototypeOf(value) === null ? Object.create(null) : {}; |
| expect(projected.changed_fields).toBeUndefined(); | ||
| expect(projected.changed_field_count).toBe(2); |
There was a problem hiding this comment.
Add an assertion to verify that the prototype of the null-prototype object is correctly preserved after compaction.
expect(projected.changed_fields).toBeUndefined();
expect(projected.changed_field_count).toBe(2);
expect(Object.getPrototypeOf(projected)).toBeNull();…view, Gemini)
Gemini (medium): compacting a null-prototype object initialized the projected copy
as {} (Object.prototype), silently re-parenting it. Build the projected object from
Object.getPrototypeOf(value) so null-prototype (and Object.prototype) sources keep
their prototype. Test now asserts the compacted null-prototype object stays null-proto.
Full coverage gate green (100%).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Addressed Gemini's remaining two findings in 70cefec: the projected object now inherits the source prototype via @codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces a compact mutation output mode that replaces the verbose changed_fields array with a changed_field_count to improve token efficiency in agent loops. This is supported by a new global CLI flag --no-changed-fields and is enabled by default for MCP mutation tools. Additionally, the PR enhances unknown-option recovery with prefix/abbreviation-aware suggestions, cross-command flag hints, and list --sort aliases. The reviewer feedback is highly constructive and identifies opportunities to ensure deterministic sorting of flag suggestions, optimize plain object instantiation in the projection helper, and defensively guard against empty arrays when rendering cross-command hints.
| return candidates | ||
| .sort((left, right) => { | ||
| if (left.rank !== right.rank) { | ||
| return left.rank - right.rank; | ||
| } | ||
| if (left.distance !== right.distance) { | ||
| return left.distance - right.distance; | ||
| } |
There was a problem hiding this comment.
To ensure that the suggestions are completely deterministic and consistent across different environments, consider adding an alphabetical tie-breaker sort by the flag name when both rank and distance are equal.
| return candidates | |
| .sort((left, right) => { | |
| if (left.rank !== right.rank) { | |
| return left.rank - right.rank; | |
| } | |
| if (left.distance !== right.distance) { | |
| return left.distance - right.distance; | |
| } | |
| return candidates | |
| .sort((left, right) => { | |
| if (left.rank !== right.rank) { | |
| return left.rank - right.rank; | |
| } | |
| if (left.distance !== right.distance) { | |
| return left.distance - right.distance; | |
| } | |
| return left.flag.localeCompare(right.flag); |
| let changed = false; | ||
| // Preserve the source prototype (null or Object.prototype) so a null-prototype | ||
| // object is not silently re-parented to Object.prototype during compaction. | ||
| const projected: Record<string, unknown> = Object.create(Object.getPrototypeOf(value) as object | null); |
There was a problem hiding this comment.
Using Object.create(Object.getPrototypeOf(value)) for standard plain objects can be slower and is less idiomatic than using {}. Since isPlainObject guarantees that the prototype is either null or Object.prototype, we can optimize this by checking if the prototype is null and using {} otherwise.
| let changed = false; | |
| // Preserve the source prototype (null or Object.prototype) so a null-prototype | |
| // object is not silently re-parented to Object.prototype during compaction. | |
| const projected: Record<string, unknown> = Object.create(Object.getPrototypeOf(value) as object | null); | |
| let changed = false; | |
| const proto = Object.getPrototypeOf(value); | |
| const projected: Record<string, unknown> = proto === null ? Object.create(null) : {}; |
| ...(otherCommands | ||
| ? [`${optionName} is a valid option on: ${otherCommands.join(", ")}. If you meant one of those, run that command instead.`] | ||
| : []), |
There was a problem hiding this comment.
To prevent potential UX issues where an empty array [] is returned by normalizeContextList (which is truthy in JavaScript) and results in an empty list of commands being printed, check both otherCommands and otherCommands.length > 0 defensively. This also aligns with the pattern used for suggestions on the line above.
| ...(otherCommands | |
| ? [`${optionName} is a valid option on: ${otherCommands.join(", ")}. If you meant one of those, run that command instead.`] | |
| : []), | |
| ...(otherCommands && otherCommands.length > 0 | |
| ? [`${optionName} is a valid option on: ${otherCommands.join(", ")}. If you meant one of those, run that command instead.`] | |
| : []), |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…commands guard
- mutation-projection: use `proto === null ? Object.create(null) : {}` (idiomatic/
faster for the common plain-object case; still preserves null prototypes).
- error-guidance: guard the cross-command hint with `otherCommands.length > 0` for
consistency with the nearest-options check (normalizeContextList already returns
undefined for empty, this is belt-and-suspenders).
The suggested alphabetical tie-breaker in suggestNearestLongFlags was already present
(localeCompare tie-breaker). Full coverage gate green (100%).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Applied Gemini's latest nits in e6ced17:
Full coverage gate green (100%). @codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces several agent-UX and token-efficiency improvements. It adds a shared mutation-result projection helper to compact the verbose changed_fields array into a changed_field_count, controlled via a new global CLI flag --no-changed-fields and enabled by default on MCP mutation paths. It also enhances unknown-option error recovery with nearest, abbreviated, and cross-command flag suggestions, and introduces convenient sort aliases (updated and created) for list commands. These changes are supported by updated documentation, completion scripts, and comprehensive unit and integration tests. I have no feedback to provide as there are no review comments.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6ced17e44
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const full = runnerOptions.full === true; | ||
| delete runnerOptions.full; |
There was a problem hiding this comment.
full in MCP mutation options
Consuming and deleting options.full for compaction control breaks legitimate mutation payloads when a runtime schema defines a create/update field whose option key resolves to full: pm_create/pm_update/pm_append/pm_update-many will silently drop that field instead of persisting it. This regression is introduced here because runnerOptions.full is always removed before handing options to the command runner, so clients can no longer set that user data key through MCP.
Useful? React with 👍 / 👎.
|
|
||
| export function printResult(result: unknown, options: OutputOptions): void { | ||
| const rendered = formatOutput(result, options); | ||
| const projected = options.noChangedFields ? projectMutationResult(result, { changedFields: "compact" }) : result; |
There was a problem hiding this comment.
--no-changed-fields compaction to mutation envelopes
printResult now applies projectMutationResult to every command output whenever --no-changed-fields is set, so non-mutation responses that legitimately use changed_fields lose detailed data. A concrete regression is history --diff: each entry’s changed_fields list (the actual field names touched) is collapsed into changed_field_count, which removes information users/scripts rely on for auditing.
Useful? React with 👍 / 👎.
| if (key === CHANGED_FIELDS_KEY && Array.isArray(entry)) { | ||
| projected[CHANGED_FIELD_COUNT_KEY] = entry.length; |
There was a problem hiding this comment.
changed_fields metadata
The recursive projector rewrites any object key named changed_fields if the value is an array, even when it is normal item metadata rather than the mutation delta envelope. If a runtime schema defines a metadata key like changed_fields (for example a string-array field), --no-changed-fields and MCP default compact mode will silently replace that user data with changed_field_count, corrupting returned item content.
Useful? React with 👍 / 👎.
|
Addressed latest reviewer feedback in c969db4:\n\n- limited changed_fields compaction to mutation envelopes and update-many apply/rollback rows, preserving non-mutation/user changed_fields metadata\n- moved MCP full changed_fields control to fullChangedFields so mutation options.full is forwarded unchanged\n- added focused regressions and reran build/static/focused tests\n\n@codex review\n/gemini review\n@gemini-code-assist please re-review |
There was a problem hiding this comment.
Code Review
This pull request introduces a global --no-changed-fields CLI flag and compacts MCP mutation outputs by default to drop the verbose changed_fields array in favor of a changed_field_count for token efficiency. It also improves CLI unknown-option recovery with nearest-flag suggestions (including abbreviations like --desc -> --description), cross-command flag hints, and sort field aliases (updated -> updated_at, created -> created_at). Reviewers suggested preserving null prototypes when copying projected results in mutation-projection.ts and improving type safety in the MCP server's withMutationCompaction helper by making the options parameter optional and nullable.
| if (rowsChanged) { | ||
| projected = { ...projected, [ROWS_KEY]: nextRows }; | ||
| changed = true; | ||
| } |
There was a problem hiding this comment.
Using the object spread operator { ...projected } on an object with a null prototype will create a new object with Object.prototype as its prototype, thereby losing the null prototype. To preserve the null prototype of projected (which can happen if the original result had a null prototype), use Object.assign with a newly created null-prototype object instead.
if (rowsChanged) {
const nextProjected = Object.getPrototypeOf(projected) === null ? Object.create(null) : {};
projected = Object.assign(nextProjected, projected, { [ROWS_KEY]: nextRows });
changed = true;
}| function withMutationCompaction(args: Record<string, unknown>, options: Record<string, unknown>): { | ||
| changedFields: "full" | "compact"; | ||
| runnerOptions: Record<string, unknown>; | ||
| } { | ||
| return { changedFields: args.fullChangedFields === true ? "full" : "compact", runnerOptions: { ...options } }; | ||
| } |
There was a problem hiding this comment.
In the pm_close tool definition, options is not a required field and can be undefined at runtime. Typing the options parameter as non-nullable Record<string, unknown> in withMutationCompaction can lead to type-checking errors under strictNullChecks or potential runtime issues if not properly guarded. Making the parameter optional and nullable improves type safety and robustness.
| function withMutationCompaction(args: Record<string, unknown>, options: Record<string, unknown>): { | |
| changedFields: "full" | "compact"; | |
| runnerOptions: Record<string, unknown>; | |
| } { | |
| return { changedFields: args.fullChangedFields === true ? "full" : "compact", runnerOptions: { ...options } }; | |
| } | |
| function withMutationCompaction(args: Record<string, unknown>, options?: Record<string, unknown> | null): { | |
| changedFields: "full" | "compact"; | |
| runnerOptions: Record<string, unknown>; | |
| } { | |
| return { changedFields: args.fullChangedFields === true ? "full" : "compact", runnerOptions: { ...options } }; | |
| } |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/mutation-projection.spec.ts (1)
104-113: ⚡ Quick winAdd a regression for null-prototype update-many envelopes.
Great null-proto coverage for top-level compaction. Please add the same assertion for the update-many
rowsrewrite path, since that path rebuilds the envelope.🧪 Suggested test addition
+ it("preserves null-prototype for update-many envelopes when rows are compacted", () => { + const result = Object.assign(Object.create(null), { + mode: "apply", + rows: [Object.assign(Object.create(null), { id: "pm-1", changed_fields: ["status"] })], + }) as Record<string, unknown>; + + const projected = projectMutationResult(result, { changedFields: "compact" }) as Record<string, unknown>; + expect(Object.getPrototypeOf(projected)).toBeNull(); + });🤖 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/mutation-projection.spec.ts` around lines 104 - 113, Add a regression test that exercises the update-many rewrite path by passing a null-prototype envelope through projectMutationResult and asserting the rebuilt rows preserve the null prototype and compacted fields; specifically, reuse the nullProto object (Object.create(null) with item and changed_fields) inside an envelope shaped like { rows: [nullProto] } (or the same structure used by the update-many path in the test file), call projectMutationResult(envelope, { changedFields: "compact" }), then assert projected.rows[0].changed_fields is undefined, projected.rows[0].changed_field_count === 2, and Object.getPrototypeOf(projected.rows[0]) === null so the update-many rows rewrite path preserves null prototypes.
🤖 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 `@src/core/output/mutation-projection.ts`:
- Around line 104-109: The current rewrite uses object spread ({ ...projected,
[ROWS_KEY]: nextRows }) which converts null-prototype envelopes to plain
Object.prototype; instead detect the prototype of projected (const proto =
Object.getPrototypeOf(projected)) and construct the replacement envelope with
the same prototype (e.g. const newProjected = Object.create(proto);
Object.assign(newProjected, projected); newProjected[ROWS_KEY] = nextRows), then
set projected = newProjected and changed = true; avoid using object spread so
null-prototype envelopes remain preserved (refer to ROWS_KEY, projected,
nextRows, rowsChanged).
In `@src/mcp/server.ts`:
- Around line 140-144: The new top-level MCP transport flag fullChangedFields is
being passed through into dynamic extension options because
extensionOptionsFromArgs doesn't filter/reserve it; update
extensionOptionsFromArgs to treat fullChangedFields (and other MCP control
flags) as reserved and remove them from the returned options so pm_run and other
dynamic extension actions never receive fullChangedFields as an unexpected
option; ensure the same reservation logic is applied in the other similar block
referenced (the second occurrence around the pm_run handling).
---
Nitpick comments:
In `@tests/unit/mutation-projection.spec.ts`:
- Around line 104-113: Add a regression test that exercises the update-many
rewrite path by passing a null-prototype envelope through projectMutationResult
and asserting the rebuilt rows preserve the null prototype and compacted fields;
specifically, reuse the nullProto object (Object.create(null) with item and
changed_fields) inside an envelope shaped like { rows: [nullProto] } (or the
same structure used by the update-many path in the test file), call
projectMutationResult(envelope, { changedFields: "compact" }), then assert
projected.rows[0].changed_fields is undefined,
projected.rows[0].changed_field_count === 2, and
Object.getPrototypeOf(projected.rows[0]) === null so the update-many rows
rewrite path preserves null prototypes.
🪄 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: 1c4a2d74-a8cf-493c-a89f-82a149335247
📒 Files selected for processing (12)
.agents/pm/features/pm-rnpb.toon.agents/pm/history/pm-70mi.jsonl.agents/pm/history/pm-rnpb.jsonl.agents/pm/plans/pm-70mi.toonsrc/cli/commands/list.tssrc/cli/error-guidance.tssrc/core/output/mutation-projection.tssrc/mcp/server.tstests/integration/mcp-dynamic-package-actions.spec.tstests/unit/list-command.spec.tstests/unit/mutation-projection.spec.tstests/unit/output.spec.ts
✅ Files skipped from review due to trivial changes (2)
- .agents/pm/history/pm-rnpb.jsonl
- .agents/pm/history/pm-70mi.jsonl
…ide builder (#73) Two real nightly failures from run 26439727168 (2026-05-26) plus an in-place cleanup of the extension-policy override-normalization duplicate identified under the pm-3cbk maintainability decision. Nightly fixes: - tests/integration/cli.integration.spec.ts: add shell:true to execFileSync(npm.cmd) on win32. After CVE-2024-27980 Node hardening, spawnSync of a .cmd file requires shell mode or it fails with EINVAL. The previous PR #68 switched the binary to npm.cmd on Windows but did not enable shell mode, regressing Windows Node 20. - tests/unit/extension-command.spec.ts: realpath the mkdtemp temp root before comparing against resolveInstallSource output. macOS resolves /var/folders to /private/var/folders via fs.realpath, and the production code path canonicalizes, while the test was holding the raw os.tmpdir-prefixed path. pm-3cbk dedup: - Extract buildExtensionPolicyOverride + collectExtensionPolicyOverrides helpers inside src/core/extensions/extension-policy.ts to remove the 25-line duplicate between normalizeExtensionPolicy and hydrateExtensionPolicy override loops. Single-file extraction keeps the existing uncovered allowlist clean. Tracker hygiene: - Backfill resolution/expected_result/actual_result on three closed items (pm-ch59, pm-8nyc, pm-70mi) from PR #72 that pre-dated the --expected-result/--actual-result close flow. pm validate --check-resolution now warning-free. - Close pm-gf6f with full metadata; pnpm changelog:pm regenerated CHANGELOG.md to include the new Fixed entry. Verification: - Targeted tests pass (extension-command + cli.integration matchers). - extension-loader/settings-store/settings-validator/contracts-command: 90 tests pass. - Full release gate ok: build, typecheck, docs, static-quality, coverage (100%/100%/100%/100%), version-policy, secret-scan, npx-smoke, package-first-dogfood, npm-pack, compat, sentry-telemetry. Sentry critical=0 high=5 within threshold; telemetry finish_error_rate_pct=1.95, missing-error-code rows=0. - Manual temp-dir dogfood (init/create/list/search/context/health/ validate) happy path verified. - pm health ok; pm validate --check-resolution --check-history-drift ok; pnpm changelog:pm:check unchanged. Refs pm-7rlp, pm-3cbk, pm-gf6f. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Two agent-UX / token-efficiency items under pm-rnpb, combined into one PR. Both are implemented, fully covered, and dogfooded.
pm-ch59 — Compact mutation output
Every mutation echoed a verbose
changed_fieldsarray (~50% of a create payload), redundant with the item echo above it.src/core/output/mutation-projection.ts→projectMutationResult()replaceschanged_fieldswith a deterministicchanged_field_count(no lost mutation evidence).--no-changed-fieldsCLI flag (threadedGlobalOptions→OutputOptions→printResult) — applies uniformly to every mutation command.create/update/close/append/update-many) is compact-by-default; passoptions.full=truefor the full delta. Tool descriptions updated.extractCommandScopedOptions: the new global is stripped so it never leaks into per-command update fields (keepsupdate-manyrollback +updateno-op detection correct).pm contracts+ bash/zsh/fish completion.pm-8nyc — Smarter unknown-option recovery
--typeontest-all→create, list, list-all;--fieldsoncreate→list, get).--desc→--description).list --sortaliases:updated→updated_at,created→created_at, with updated help + invalid-value error.update/create--add-tags/--add-tag/--tag→--tags(existing--body→--addretained).Housekeeping
pm-changelog2026.5.25 → 2026.5.26 and regeneratedCHANGELOG.md.COMMANDS.md,AGENT_GUIDE.md).pm-rnpbcomment (auditedpm history-redact;check-secretsclean).Verification
quality:static,security:scan,version:check,tsc --noEmit,npm pack --dry-run,docs-skillsgate, Claude + Codex plugin smokes,changelog:pm:check— all pass.Closes pm-ch59, pm-8nyc.
🤖 Generated with Claude Code