Repository navigation
Add validation self-repair and agent ergonomics primitives - #200
Conversation
|
@codex review /gemini review @gemini-code-assist review |
|
To use Codex here, create a Codex account and connect to github. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @unbraind, your pull request is larger than the review limit of 150000 diff characters
|
To use Codex here, create a Codex account and connect to github. |
|
Warning Review limit reached
More reviews will be available in 9 minutes and 40 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ 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 (78)
📝 WalkthroughWalkthroughThe PR updates several PM CLI flows: standalone note handling for files/docs, repeatable plan steps, bulk history repair, validate auto-fix and file-classification logic, a new health locks check, and related completions, docs, tests, and metadata records. ChangesPM CLI behavior and contract updates
Sequence Diagram(s)sequenceDiagram
participant CLI
participant validateTs as runValidate
participant fixPlanning as fix-planning
participant commands as update.js/files.js/docs.js
CLI->>validateTs: --auto-fix / --prune-missing request
validateTs->>fixPlanning: build planned fixes
fixPlanning->>commands: apply applicable fixes
validateTs->>CLI: fixes summary and check output
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
|
![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 implements comprehensive self-repair and validation enhancements, including automatic remediation via pm validate --auto-fix (with dry-run and scoped lifecycle gating), bulk link pruning with --prune-missing, grouped metadata reports, bulk history drift repair with pm history-repair --all, and a read-only locks check in pm health. It also resolves issues with repeated plan steps, inverted CI telemetry guards, hoisted subcommand flags, and MCP file discovery. Feedback highlights an efficiency bottleneck in the auto-fix execution, recommending that prune fixes be grouped by item and link kind to avoid sequential lock acquisitions and file writes.
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.
| for (const fix of applicable) { | ||
| try { | ||
| await applyValidateFix(fix, global); | ||
| appliedFixRows.push(toFixOutputRow(fix)); | ||
| } catch (error) { | ||
| failedFixRows.push({ | ||
| ...toFixOutputRow(fix), | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Efficiency Bottleneck: Sequential Lock Acquisition and File Writes
Applying each prune fix individually via sequential calls to applyValidateFix (which internally invokes runFiles or runDocs per file/doc) can be highly inefficient if an item has multiple stale links. Each call independently acquires/releases the item lock, reads/writes the item file, and triggers any associated write hooks.
Consider grouping the prune fixes by item_id and link_kind (or combining them into a single update/prune batch per item) before execution to perform the removals in a single runFiles or runDocs call per item.
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of self-repair, validation, and CLI/MCP usability enhancements. Key updates include the addition of --auto-fix, --dry-run, --fix-scope, and --prune-missing flags to pm validate for automated metadata backfills and stale link pruning, as well as the implementation of pm history-repair --all for bulk drift repair. It also adds a read-only locks check to pm health, a standalone --note flag for file/doc linking, and support for accumulating repeated --step values on plan creation. Additionally, several bugs are resolved, including an inverted CI telemetry guard, hoisted subcommand option dropping, and MCP pm_files discovery routing. There are no review comments to assess, and I have no further feedback to provide as the implementation is robust and thoroughly tested.
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.
c219464 to
e3be3bb
Compare
|
Review fix pushed for Gemini feedback (batched validate prune-missing removals). Re-requesting review on the new head e3be3bb. @codex review /gemini review @gemini-code-assist review @coderabbitai review |
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/cli/commands/health.ts`:
- Around line 1289-1291: buildLocksCheck currently lets scanLockHealth
exceptions (e.g., non-ENOENT readdir errors from lock-gc) bubble up and abort
the whole `pm health`; wrap the call to scanLockHealth in a try/catch inside
buildLocksCheck, push a descriptive warning into the warnings array on error
(including the caught error message and context), and return a HealthCheck
object representing a warning state (not throw) along with the warnings array;
reference the buildLocksCheck and scanLockHealth symbols and ensure the returned
check follows the same HealthCheck shape used elsewhere for warn-level results.
In `@src/sdk/cli-contracts.ts`:
- Around line 326-327: The docs command contract is missing the --list flag:
update the DOCS_FLAG_CONTRACTS array to include { flag: "--list" } (matching the
existing { flag: "--note" } entry) and add the same --list entry to the docs
tool/schema export used for exposed contracts (the docs tool schema/contract
object) so that the docs command surface and completions/MCP consumers expose
the supported --list option (ensure the identifier names DOCS_FLAG_CONTRACTS and
the docs tool/schema object are the targets you modify).
- Around line 1715-1717: The schema added new action parameters but the exported
version constant PM_TOOL_PARAMETERS_SCHEMA_VERSION was not bumped; update the
PM_TOOL_PARAMETERS_SCHEMA_VERSION string to a new patch (or minor if
appropriate) version (e.g., "4.0.3") and ensure every exported occurrence of
that constant in this module (PM_TOOL_PARAMETERS_SCHEMA_VERSION) and any related
export sites is updated consistently so downstream consumers can detect the
additive change.
- Around line 1715-1717: The schema for the history-repair command currently
uses anyOfRequired which allows both id and all to be present; update the
contract so exactly one of id or all is required by replacing anyOfRequired:
[["id"], ["all"]] with an exclusive requirement (either use oneOfRequired:
[["id"], ["all"]] if the generator supports it, or add a not block that forbids
both present, e.g. add "not": {"required": ["id","all"]}) while keeping the
optional list (optional: ["id","all","dryRun",
...AUTHOR_MESSAGE_FORCE_PARAMETER_KEYS]) intact so the validation for the
history-repair payload enforces mutual exclusivity between id and all.
In `@tests/fixtures/contracts/full.json`:
- Around line 18338-18344: The schema for the action objects "files" and "docs"
currently allows "addNote" independently but the docs state addNote requires
"add" or "addGlob"; update both action object schemas (the objects that define
properties for files and docs) to enforce that constraint by adding a JSON
Schema conditional: if "addNote" is present then require at least one of "add"
or "addGlob" (use an "anyOf" with required:["add"] or required:["addGlob"] or an
"if": {"required":["addNote"]} "then":
{"anyOf":[{"required":["add"]},{"required":["addGlob"]}]}) so addNote cannot
appear alone.
- Around line 15286-15306: Replace the ambiguous "anyOf" with "oneOf" for the
schema branch where "action" is "history-repair" so the payload must include
either "id" or "all" but not both; update the schema fragment that currently has
"anyOf" (the block containing "required":["id"] and "required":["all"]) to use
"oneOf" and keep the "properties" entries for "id" and "all" unchanged (the
"action":"history-repair" const, the "id" requirement, and the "all" boolean
property). Apply the same change to the second occurrence referenced in the
comment (the similar "anyOf" near lines ~15374-15375).
🪄 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: 9fa809ec-0d58-43b6-8f34-3e72ce29a452
📒 Files selected for processing (74)
.agents/pm/chores/pm-2cgu.toon.agents/pm/chores/pm-6qi8.toon.agents/pm/extensions/.managed-extensions.json.agents/pm/features/pm-0v2m.toon.agents/pm/features/pm-8jss.toon.agents/pm/features/pm-9ftr.toon.agents/pm/features/pm-c3sz.toon.agents/pm/features/pm-pfnx.toon.agents/pm/features/pm-pmyq.toon.agents/pm/features/pm-xo1n.toon.agents/pm/history/pm-0hx2.jsonl.agents/pm/history/pm-0v2m.jsonl.agents/pm/history/pm-2cgu.jsonl.agents/pm/history/pm-6mit.jsonl.agents/pm/history/pm-6qi8.jsonl.agents/pm/history/pm-8jss.jsonl.agents/pm/history/pm-9ftr.jsonl.agents/pm/history/pm-c3sz.jsonl.agents/pm/history/pm-df9k.jsonl.agents/pm/history/pm-dhrp.jsonl.agents/pm/history/pm-lg9i.jsonl.agents/pm/history/pm-pfnx.jsonl.agents/pm/history/pm-pmyq.jsonl.agents/pm/history/pm-wcaa.jsonl.agents/pm/history/pm-xo1n.jsonl.agents/pm/issues/pm-0hx2.toon.agents/pm/issues/pm-6mit.toon.agents/pm/issues/pm-df9k.toon.agents/pm/issues/pm-wcaa.toon.agents/pm/plans/pm-dhrp.toon.agents/pm/tasks/pm-lg9i.toonCHANGELOG.mddocs/AGENT_GUIDE.mddocs/COMMANDS.mdscripts/release/sentry-telemetry-gate.mjssrc/cli/commands/completion.tssrc/cli/commands/create.tssrc/cli/commands/docs.tssrc/cli/commands/files.tssrc/cli/commands/health.tssrc/cli/commands/history-repair.tssrc/cli/commands/index.tssrc/cli/commands/linked-artifacts.tssrc/cli/commands/plan.tssrc/cli/commands/validate.tssrc/cli/help-content.tssrc/cli/register-mutation.tssrc/cli/register-operations.tssrc/cli/register-setup.tssrc/core/diagnostics/remediation.tssrc/core/lock/lock-gc.tssrc/core/telemetry/consent.tssrc/core/validate/fix-planning.tssrc/core/validate/missing-by-type.tssrc/core/validate/stale-file-classification.tssrc/mcp/server.tssrc/sdk/cli-contracts.tssrc/sdk/cli-contracts/tool-parameter-tables.tstests/fixtures/contracts/full.jsontests/integration/cli.integration.spec.tstests/integration/mcp-handshake.spec.tstests/integration/release-automation-contract.spec.tstests/integration/remediation-registry.spec.tstests/unit/commands/register-commands.spec.tstests/unit/completion-command.spec.tstests/unit/contracts-command.spec.tstests/unit/core/lock/lock-gc.spec.tstests/unit/create-command.spec.tstests/unit/files-docs-command.spec.tstests/unit/health-command.spec.tstests/unit/history-repair-command.spec.tstests/unit/plan-command.spec.tstests/unit/telemetry-consent.spec.tstests/unit/validate-command.spec.ts
There was a problem hiding this comment.
Code Review
This pull request introduces several validation, self-repair, and UX enhancements, including automatic remediations via pm validate --auto-fix, bulk drift repair with pm history-repair --all, a read-only locks health check, standalone --note support for files and docs, and ordered step accumulation on plan creation. The review feedback highlights three important improvement opportunities: safely filtering out potential undefined values in applyValidateFixes to prevent a TypeError, enhancing basenameOf to support Windows backslashes for cross-platform compatibility, and defensively handling null or non-string values in toOrderedStepTitles to prevent runtime errors during MCP/JSON payload processing.
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.
| if (!first) { | ||
| continue; | ||
| } | ||
| const remove = batch.map((fix) => fix.path!).filter((value) => value.length > 0); |
There was a problem hiding this comment.
In applyValidateFixes, batch.map((fix) => fix.path!) can return undefined values since path is optional on ValidateFixRecord. Calling value.length on undefined inside the filter will throw a TypeError. We should safely filter out undefined values first.
| const remove = batch.map((fix) => fix.path!).filter((value) => value.length > 0); | |
| const remove = batch.map((fix) => fix.path).filter((value): value is string => typeof value === "string" && value.length > 0); |
| function basenameOf(relativePath: string): string { | ||
| const lastSlash = relativePath.lastIndexOf("/"); | ||
| return lastSlash === -1 ? relativePath : relativePath.slice(lastSlash + 1); | ||
| } |
There was a problem hiding this comment.
The basenameOf function only checks for forward slashes (/). On Windows systems, paths may contain backslashes (\\). To ensure cross-platform compatibility, the function should handle both forward and backward slashes.
| function basenameOf(relativePath: string): string { | |
| const lastSlash = relativePath.lastIndexOf("/"); | |
| return lastSlash === -1 ? relativePath : relativePath.slice(lastSlash + 1); | |
| } | |
| function basenameOf(relativePath: string): string { | |
| const lastSlash = Math.max(relativePath.lastIndexOf("/"), relativePath.lastIndexOf("\\")); | |
| return lastSlash === -1 ? relativePath : relativePath.slice(lastSlash + 1); | |
| } |
| function toOrderedStepTitles(value: string | string[] | undefined): string[] { | ||
| const values = Array.isArray(value) ? value : value === undefined ? [] : [value]; | ||
| return values.map((entry) => entry.trim()).filter((entry) => entry.length > 0); | ||
| } |
There was a problem hiding this comment.
The toOrderedStepTitles function assumes that value is either an array of strings, undefined, or a single string. If value is null (which can happen with MCP/JSON payloads) or contains non-string elements, calling entry.trim() will throw a runtime error. We should handle null and non-string values defensively.
function toOrderedStepTitles(value: string | string[] | undefined | null): string[] {
const values = Array.isArray(value) ? value : (value === undefined || value === null) ? [] : [value];
return values
.map((entry) => (typeof entry === "string" ? entry.trim() : String(entry).trim()))
.filter((entry) => entry.length > 0);
}
e3be3bb to
83d1813
Compare
|
Review hardening pushed for CodeRabbit/Gemini feedback on new head 83d1813. Re-requesting review on the updated PR head. @codex review /gemini review @gemini-code-assist review @coderabbitai review |
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive self-repair and auto-fix capabilities across the CLI and MCP surfaces. Key additions include pm validate --auto-fix (with --dry-run and gated --fix-scope lifecycle scopes), --prune-missing for bulk-removing deleted stale links, and pm history-repair --all for bulk audited drift repair across all streams. It also adds a read-only locks check to pm health, a standalone --note flag for batch file/doc links, repeated --step accumulation on plan creation, and fixes an inverted CI telemetry prompt guard. A critical review comment was kept pointing out a missing EXIT_CODE import in src/cli/register-mutation.ts that would cause a runtime ReferenceError when a bulk history repair stream fails.
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.
| if (result.totals.failed > 0) { | ||
| // Per-stream failures are collected (one bad stream never aborts the | ||
| // pass) but must still fail the command for gating callers. | ||
| process.exitCode = EXIT_CODE.GENERIC_FAILURE; | ||
| } |
There was a problem hiding this comment.
The EXIT_CODE constant is used here to set process.exitCode but it is not imported in this file. This will cause a ReferenceError at runtime when a history repair stream fails in bulk mode. Please import EXIT_CODE from ../core/shared/constants.js.
| if (result.totals.failed > 0) { | |
| // Per-stream failures are collected (one bad stream never aborts the | |
| // pass) but must still fail the command for gating callers. | |
| process.exitCode = EXIT_CODE.GENERIC_FAILURE; | |
| } | |
| if (result.totals.failed > 0) { | |
| // Per-stream failures are collected (one bad stream never aborts the | |
| // pass) but must still fail the command for gating callers. | |
| const { EXIT_CODE } = await import("../core/shared/constants.js"); | |
| process.exitCode = EXIT_CODE.GENERIC_FAILURE; | |
| } |
83d1813 to
b407312
Compare
|
Tracker evidence follow-up pushed on new head b407312. Gemini latest EXIT_CODE comment was verified obsolete because src/cli/register-mutation.ts already imports EXIT_CODE; focused build/history-repair/register tests passed and the pm item records that assessment. Re-requesting review on the updated head. @codex review /gemini review @gemini-code-assist review @coderabbitai review |
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Code Review
This pull request introduces several features and fixes to the pm CLI, including automated validation fixes with pm validate --auto-fix and --prune-missing, bulk history stream repairs via pm history-repair --all, a read-only locks check in pm health, and a standalone --note flag for files and docs. It also resolves issues with repeated --step flags in plan creation, the telemetry CI guard, and hoisted subcommand options. Feedback on the changes identifies dead code in applyValidateFix where the file and doc pruning cases are redundant due to batching in applyValidateFixes.
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.
| case "prune_file_link": { | ||
| const { runFiles } = await import("./files.js"); | ||
| await runFiles(fix.item_id, { remove: [fix.path!], message: VALIDATE_AUTO_FIX_MESSAGE }, global); | ||
| return; | ||
| } | ||
| default: { | ||
| const { runDocs } = await import("./docs.js"); | ||
| await runDocs(fix.item_id, { remove: [fix.path!], message: VALIDATE_AUTO_FIX_MESSAGE }, global); | ||
| } |
There was a problem hiding this comment.
The "prune_file_link" and "prune_doc_link" cases in "applyValidateFix" are dead code. Because "pruneBatchKey" always returns a non-null batch key for these kinds, they are always batched and processed in "applyValidateFixes" rather than being passed individually to "applyValidateFix". We can safely remove these cases and simplify the default case to throw an error for any unsupported non-batched fix kinds.
default:
throw new Error("Unsupported non-batched fix kind: " + fix.kind);
Add validation auto-fix and prune-missing primitives, MCP files discovery wiring, strict SDK action-contract hardening, and PR review follow-ups for agent-oriented project context repair. This keeps project management as context management by making stale linked files/docs easier to classify, preview, batch-prune, and document through pm items while preserving generated contracts and package-owned changelog output. Review hardening included in this amend: - fail open pm health locks scans with structured warning details - expose docs --list through CLI contracts and strict MCP schemas - bump strict tool schema to 4.0.3 with exclusive history-repair id/all validation and addNote preconditions - handle MCP-style plan step coercion, Windows stale-path basenames, and safe prune path filtering - record pm-im0s evidence and regenerate CHANGELOG.md via pm-changelog Verification: pnpm build; focused tests for contracts/health/plan/validate; pnpm contracts:check; pm validate --check-resolution --check-history-drift; pnpm changelog:pm:check; pnpm quality:static; node scripts/run-tests.mjs coverage; pnpm security:scan; pnpm audit --audit-level low; pnpm smoke:npx; Sentry/telemetry release gate; isolated PM_PATH/PM_GLOBAL_PATH manual temp smoke.
b407312 to
5211a75
Compare
|
Gemini dead-code follow-up fixed on new head 5211a75: removed non-batched prune_file_link/prune_doc_link mutation branches from applyValidateFix; prune removals remain handled by batching in applyValidateFixes. Focused validation passed locally. Re-requesting review on the updated head. @codex review /gemini review @gemini-code-assist review @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Code Review
This pull request introduces several features and bug fixes, including the pm validate --auto-fix and --prune-missing remediation options, per-item-type grouping of missing required fields, bulk history repair via pm history-repair --all, a read-only locks check in pm health, a standalone --note flag for files and docs, and repeated --step accumulation on plan creation. It also fixes an inverted CI telemetry prompt guard and hoists parent options correctly on lifecycle subcommands. No review comments were provided, so there is no feedback to address.
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.
Summary
pm tracking
Verification
Review agents
Requested after PR creation: @codex review, /gemini review, @gemini-code-assist review.