Repository navigation
Agent self-repair: pm health remediation_map + pm validate --fix-hints - #111
Conversation
…--fix-hints Introduce a shared, machine-executable remediation registry (src/core/diagnostics/remediation.ts) as the single source of truth mapping each pm health / pm validate warning-code prefix to the executable pm command that resolves it, so agents gating on these commands no longer hardcode warning-code -> fix mappings. - pm health --json (pm-0hnu): every non-extension check whose warnings have a known code gains details.remediation_map (code -> fix command). Default/--full include it; --brief/--summary omit it; extension checks keep their richer details.triage.remediation. - pm validate --fix-hints (pm-6m3y): new read-only flag adds a uniform details.fix_hints[] of executable pm commands per failing check. The resolution check aliases its existing per-row commands; other checks derive one generic command per warning code from the registry. Never mutates items. - Settings-read warning surfacing (pm-7tcw): main.ts threads settings_read_* warnings through the discovery snapshot (zero extra reads) and prints them once per command to stderr, so create/list/search no longer silently fall back to defaults on a corrupt settings.json (stdout/--json stays clean). Closes pm-sz6w (umbrella story), pm-0hnu, pm-6m3y, pm-7tcw. Wiring: register-operations, cli-contracts (VALIDATE_FLAG_CONTRACTS + validate schema contract), tool-parameter-tables, bash/zsh/fish completions, and docs (COMMANDS/AGENT_GUIDE/CONFIGURATION). The registry module is coverage-gated at 100% (tests/integration/remediation-registry.spec.ts); command-level tests merged into existing health/validate/cli-main specs. Full suite 2301 passed, coverage 100%. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reviewer's GuideAdds a shared remediation registry powering machine-executable fix commands for pm health/validate, exposes them via health.remediation_map and validate --fix-hints, and surfaces settings-read warnings on stderr while keeping JSON/stdout clean, with full wiring through CLI options, contracts, completions, docs, tests, and coverage. Sequence diagram for pm validate --fix-hints flowsequenceDiagram
actor User
participant CLI as pm validate
participant Run as runValidate
participant Fix as attachValidateFixHints
participant Reg as buildRemediationCommands
participant RegData as REMEDIATION_REGISTRY
User->>CLI: pm validate --fix-hints --json
CLI->>Run: runValidate({ fixHints: true, ... })
loop per built check
Run->>Fix: attachValidateFixHints(check, warnings)
alt resolution check with per-row hints
Fix-->>Run: details.fix_hints from missing_resolution_remediation_hints
else other checks
Fix->>Reg: buildRemediationCommands(warnings)
Reg->>RegData: resolveRemediation(warning)
Reg-->>Fix: ["pm ...", ...]
Fix-->>Run: details.fix_hints updated
end
end
Run-->>CLI: checks with details.fix_hints
CLI-->>User: JSON result
Sequence diagram for surfacing settings_read warnings on stderrsequenceDiagram
actor User
participant CLI as pm <command>
participant Snap as loadRuntimeExtensionDiscoverySnapshot
participant Read as readSettingsWithMetadata
participant Maybe as maybeLoadRuntimeExtensions
participant Emit as emitSettingsReadWarnings
participant Err as printError
User->>CLI: pm create / list / search ...
CLI->>Snap: loadRuntimeExtensionDiscoverySnapshot(pmRoot)
Snap->>Read: readSettingsWithMetadata(pmRoot)
Read-->>Snap: { settings, warnings }
Snap-->>CLI: discoverySnapshot{ settingsReadWarnings }
CLI->>Maybe: maybeLoadRuntimeExtensions(command, globalOptions)
Maybe->>Emit: emitSettingsReadWarnings(settingsReadWarnings)
loop each warning starting with settings_read_
Emit->>Err: printError("[pm] warning: ...")
end
Maybe-->>CLI: runtime extension result
CLI-->>User: stdout / --json output (unchanged)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@codex review |
|
/gemini review |
|
Warning Review limit reached
More reviews will be available in 19 minutes and 35 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 (8)
📝 WalkthroughWalkthroughThis PR delivers machine-executable remediation commands across ChangesRemediation Delivery Feature
Project Management Closure Records
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
emitSettingsReadWarningsyou currently surface everysettings_read_warning, but the comment says only actionable ones should be shown while benign informational warnings stay quiet; consider explicitly whitelisting the actionable codes (e.g.settings_read_invalid_json,settings_read_invalid_schema,settings_read_merge_failed) instead of using a broadstartsWith("settings_read_")filter.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `emitSettingsReadWarnings` you currently surface every `settings_read_` warning, but the comment says only actionable ones should be shown while benign informational warnings stay quiet; consider explicitly whitelisting the actionable codes (e.g. `settings_read_invalid_json`, `settings_read_invalid_schema`, `settings_read_merge_failed`) instead of using a broad `startsWith("settings_read_")` filter.
## Individual Comments
### Comment 1
<location path="src/cli/commands/validate.ts" line_range="646-653" />
<code_context>
+ * registry. Read-only: this only enriches the diagnostic output, never mutates
+ * any item.
+ */
+function attachValidateFixHints(check: ValidateCheck, checkWarnings: string[]): void {
+ const existingResolutionHints = check.details.missing_resolution_remediation_hints;
+ const fixHints =
+ Array.isArray(existingResolutionHints) && existingResolutionHints.length > 0
+ ? existingResolutionHints.filter((hint): hint is string => typeof hint === "string")
+ : buildRemediationCommands(checkWarnings);
+ if (fixHints.length > 0) {
+ check.details = { ...check.details, fix_hints: fixHints };
+ }
+}
</code_context>
<issue_to_address>
**issue:** Clarify that fix_hints entries may contain placeholders and are not always directly executable as-is.
The current JSDoc suggests `fix_hints` is a uniform, directly executable list of `pm` commands, but some values produced by `buildRemediationCommands` can contain `<id>` placeholders (as per `RemediationEntry.command`). This means they require substitution before execution. Please either adjust the wording to avoid "machine-executable" or ensure `fix_hints` only contains fully concrete commands so the documented contract matches the actual values.
</issue_to_address>
### Comment 2
<location path="src/cli/main.ts" line_range="1055-1058" />
<code_context>
+ */
+function emitSettingsReadWarnings(warnings: readonly string[]): void {
+ for (const warning of warnings) {
+ if (warning.startsWith("settings_read_")) {
+ printError(
+ `[pm] warning: ${warning} — settings.json could not be loaded and pm fell back to defaults; run pm health for remediation.`,
+ );
+ }
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The implementation may surface non-actionable settings_read_* warnings despite the docstring promise.
The docstring promises that only actionable `settings_read_*` warnings are surfaced, but this code logs every `settings_read_*` warning. If upstream emits benign `settings_read_*` warnings, they will now appear too. Please either restrict the check to the specific actionable codes, or update the docstring to match this broader behavior so callers aren’t misled about what is suppressed.
</issue_to_address>
### Comment 3
<location path="src/core/diagnostics/remediation.ts" line_range="1" />
<code_context>
return `pm update ${row.id} ${fieldArguments} --message \"Backfill resolution metadata\"`;
}
+/**
+ * Attach a uniform, machine-executable `fix_hints` array (executable `pm`
+ * commands) to a validate check's details when `--fix-hints` is requested. The
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The documented "longest-prefix precedence" is not enforced by resolveRemediation and relies on registry ordering.
The type comment for `code` promises longest-prefix precedence (e.g., `settings` vs `settings:id_prefix_empty`), but `resolveRemediation` just walks `REMEDIATION_REGISTRY` in order and returns the first match on equality or `<code>:`. Precedence is therefore determined by registry order, not prefix length. To avoid surprises when adding broader codes, either implement actual longest-prefix selection in `resolveRemediation` or update the comment to state that registry order defines precedence.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (warning.startsWith("settings_read_")) { | ||
| printError( | ||
| `[pm] warning: ${warning} — settings.json could not be loaded and pm fell back to defaults; run pm health for remediation.`, | ||
| ); |
There was a problem hiding this comment.
issue (bug_risk): The implementation may surface non-actionable settings_read_* warnings despite the docstring promise.
The docstring promises that only actionable settings_read_* warnings are surfaced, but this code logs every settings_read_* warning. If upstream emits benign settings_read_* warnings, they will now appear too. Please either restrict the check to the specific actionable codes, or update the docstring to match this broader behavior so callers aren’t misled about what is suppressed.
![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 a shared machine-executable remediation registry that maps warning codes to actionable pm commands, enabling automated self-repair. This registry is integrated into pm health --json as a remediation_map and into pm validate under a new --fix-hints flag. Additionally, the PR surfaces settings-read warnings proactively to stderr instead of silently falling back to defaults. The review feedback highlights two important improvements: ensuring that resolveRemediation correctly respects longest-prefix precedence rather than returning the first linear match, and using optional chaining on check.details in attachValidateFixHints to avoid potential runtime errors.
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.
| export function resolveRemediation(warning: string): RemediationEntry | undefined { | ||
| const normalized = warning.trim(); | ||
| for (const entry of REMEDIATION_REGISTRY) { | ||
| if (normalized === entry.code || normalized.startsWith(`${entry.code}:`)) { | ||
| return entry; | ||
| } | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
The documented behavior of resolveRemediation specifies 'longest-prefix precedence so settings:id_prefix_empty can be more specific than a hypothetical bare settings entry.' However, the current implementation performs a simple linear scan and returns the first match. If a less specific prefix entry is placed earlier in the registry, it will shadow more specific entries. To guarantee longest-prefix matching regardless of registry order, we should find the matching entry with the longest code length.
export function resolveRemediation(warning: string): RemediationEntry | undefined {
const normalized = warning.trim();
let bestMatch: RemediationEntry | undefined = undefined;
for (const entry of REMEDIATION_REGISTRY) {
if (normalized === entry.code || normalized.startsWith(`${entry.code}:`)) {
if (!bestMatch || entry.code.length > bestMatch.code.length) {
bestMatch = entry;
}
}
}
return bestMatch;
}| function attachValidateFixHints(check: ValidateCheck, checkWarnings: string[]): void { | ||
| const existingResolutionHints = check.details.missing_resolution_remediation_hints; | ||
| const fixHints = | ||
| Array.isArray(existingResolutionHints) && existingResolutionHints.length > 0 | ||
| ? existingResolutionHints.filter((hint): hint is string => typeof hint === "string") | ||
| : buildRemediationCommands(checkWarnings); | ||
| if (fixHints.length > 0) { | ||
| check.details = { ...check.details, fix_hints: fixHints }; | ||
| } | ||
| } |
There was a problem hiding this comment.
To prevent potential runtime errors, use optional chaining when accessing check.details since details can be undefined or null depending on the specific validation check implementation.
| function attachValidateFixHints(check: ValidateCheck, checkWarnings: string[]): void { | |
| const existingResolutionHints = check.details.missing_resolution_remediation_hints; | |
| const fixHints = | |
| Array.isArray(existingResolutionHints) && existingResolutionHints.length > 0 | |
| ? existingResolutionHints.filter((hint): hint is string => typeof hint === "string") | |
| : buildRemediationCommands(checkWarnings); | |
| if (fixHints.length > 0) { | |
| check.details = { ...check.details, fix_hints: fixHints }; | |
| } | |
| } | |
| function attachValidateFixHints(check: ValidateCheck, checkWarnings: string[]): void { | |
| const existingResolutionHints = check.details?.missing_resolution_remediation_hints; | |
| const fixHints = | |
| Array.isArray(existingResolutionHints) && existingResolutionHints.length > 0 | |
| ? existingResolutionHints.filter((hint): hint is string => typeof hint === "string") | |
| : buildRemediationCommands(checkWarnings); | |
| if (fixHints.length > 0) { | |
| check.details = { ...check.details, fix_hints: fixHints }; | |
| } | |
| } |
There was a problem hiding this comment.
Code Review
This pull request introduces a shared machine-executable remediation registry to provide uniform fix commands across pm health (via remediation_map) and pm validate (via the new --fix-hints flag), alongside proactive surfacing of settings-read warnings to stderr. The review feedback highlights three key areas for improvement: ensuring the resolveRemediation helper correctly implements the documented longest-prefix precedence rather than relying on array order, using optional chaining when accessing check.details to prevent potential runtime errors, and safely defaulting settingsReadWarnings to an empty array during destructuring.
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.
| export function resolveRemediation(warning: string): RemediationEntry | undefined { | ||
| const normalized = warning.trim(); | ||
| for (const entry of REMEDIATION_REGISTRY) { | ||
| if (normalized === entry.code || normalized.startsWith(`${entry.code}:`)) { | ||
| return entry; | ||
| } | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
The comment above REMEDIATION_REGISTRY states that matching is resolved with longest-prefix precedence so that more specific codes (e.g., settings:id_prefix_empty) take precedence over hypothetical shorter prefix codes (e.g., settings). However, the current implementation simply returns the first match encountered in the array order. If a shorter prefix code is ever added earlier in the registry, it will incorrectly shadow the more specific one.
To guarantee the documented longest-prefix precedence regardless of array order, we should find the match with the longest code length.
export function resolveRemediation(warning: string): RemediationEntry | undefined {
const normalized = warning.trim();
let bestMatch: RemediationEntry | undefined = undefined;
for (const entry of REMEDIATION_REGISTRY) {
if (normalized === entry.code || normalized.startsWith(`${entry.code}:`)) {
if (!bestMatch || entry.code.length > bestMatch.code.length) {
bestMatch = entry;
}
}
}
return bestMatch;
}| function attachValidateFixHints(check: ValidateCheck, checkWarnings: string[]): void { | ||
| const existingResolutionHints = check.details.missing_resolution_remediation_hints; | ||
| const fixHints = | ||
| Array.isArray(existingResolutionHints) && existingResolutionHints.length > 0 | ||
| ? existingResolutionHints.filter((hint): hint is string => typeof hint === "string") | ||
| : buildRemediationCommands(checkWarnings); | ||
| if (fixHints.length > 0) { | ||
| check.details = { ...check.details, fix_hints: fixHints }; | ||
| } | ||
| } |
There was a problem hiding this comment.
To ensure robust defensive programming, we should use optional chaining when accessing check.details as it could potentially be undefined or null in some contexts (e.g., custom or extension checks, or future refactors). This prevents a potential runtime TypeError when trying to read missing_resolution_remediation_hints.
| function attachValidateFixHints(check: ValidateCheck, checkWarnings: string[]): void { | |
| const existingResolutionHints = check.details.missing_resolution_remediation_hints; | |
| const fixHints = | |
| Array.isArray(existingResolutionHints) && existingResolutionHints.length > 0 | |
| ? existingResolutionHints.filter((hint): hint is string => typeof hint === "string") | |
| : buildRemediationCommands(checkWarnings); | |
| if (fixHints.length > 0) { | |
| check.details = { ...check.details, fix_hints: fixHints }; | |
| } | |
| } | |
| function attachValidateFixHints(check: ValidateCheck, checkWarnings: string[]): void { | |
| const existingResolutionHints = check.details?.missing_resolution_remediation_hints; | |
| const fixHints = | |
| Array.isArray(existingResolutionHints) && existingResolutionHints.length > 0 | |
| ? existingResolutionHints.filter((hint): hint is string => typeof hint === "string") | |
| : buildRemediationCommands(checkWarnings); | |
| if (fixHints.length > 0) { | |
| check.details = { ...check.details, fix_hints: fixHints }; | |
| } | |
| } |
| try { | ||
| const startedAt = Date.now(); | ||
| const settings = await readSettings(pmRoot); | ||
| const { settings, warnings: settingsReadWarnings } = await readSettingsWithMetadata(pmRoot); |
There was a problem hiding this comment.
If readSettingsWithMetadata ever returns an object where warnings is undefined or optional, spreading settingsReadWarnings at line 1118 will throw a runtime TypeError (since undefined is not iterable). Defaulting it to an empty array ensures safe execution.
| const { settings, warnings: settingsReadWarnings } = await readSettingsWithMetadata(pmRoot); | |
| const { settings, warnings: settingsReadWarnings = [] } = await readSettingsWithMetadata(pmRoot); |
![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: c651b26875
ℹ️ 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".
| // --- pm validate: lifecycle --- | ||
| { | ||
| code: "validate_lifecycle_active_closure_like_metadata", | ||
| command: 'pm update <id> --resolution "none"', |
There was a problem hiding this comment.
When pm validate --fix-hints reports validate_lifecycle_active_closure_like_metadata for an active item with resolution set, this hint cannot be executed to fix it: pm update treats --resolution as a scalar setter and rejects the legacy clear token "none" via assertNoLegacyScalarToken, so the suggested self-repair command fails instead of clearing the field. The hint should use the supported clear path such as --unset resolution or otherwise point to a valid close/update flow.
Useful? React with 👍 / 👎.
| }, | ||
| { | ||
| code: "validate_lifecycle_stale_blockers", | ||
| command: 'pm update <id> --unblock-note "<resolution>"', |
There was a problem hiding this comment.
For stale-blocker findings, this fix hint does not resolve the condition that buildLifecycleCheck reports: the validator only checks status, blocked_by, and blocked_reason, while --unblock-note writes the separate unblock_note metadata field. In cases like an active non-blocked item that still has blocked_by/blocked_reason, running the suggested command leaves the same warning on the next pm validate --check-stale-blockers --fix-hints run; the hint needs to clear or update the blocker fields/status that the validator actually inspects.
Useful? React with 👍 / 👎.
| // --- pm validate: metadata --- | ||
| { | ||
| code: "validate_metadata_missing_author", | ||
| command: 'pm update <id> --author "<name>"', |
There was a problem hiding this comment.
For validate_metadata_missing_author, this fix hint does not update the missing item metadata: pm update --author is the mutation/audit author used for history entries, and the update path never writes document.metadata.author. When an item file is missing its author field, running the suggested command records a new audit entry but leaves the same validation warning, so the remediation needs a command that actually sets or restores the item author field.
Useful? React with 👍 / 👎.
| // --- pm validate: command_references --- | ||
| { | ||
| code: "validate_command_references_stale_pm_ids", | ||
| command: "pm update <id> --body <corrected-body>", |
There was a problem hiding this comment.
validate_command_references_stale_pm_ids is raised from buildCommandReferencesCheck by scanning item.tests[].command, but this hint edits the item's body instead of the linked test entry. For an item whose linked test command references a deleted pm-* id, running pm update <id> --body ... leaves the stale test command unchanged, so pm validate --check-command-references --fix-hints reports the same warning again; the hint should route through the linked-test mutation path instead.
Useful? React with 👍 / 👎.
| // --- pm validate: files --- | ||
| { | ||
| code: "validate_files_missing_linked_paths", | ||
| command: "pm files <id> --remove <path>", |
There was a problem hiding this comment.
The files validation check combines both item.files and item.docs before emitting validate_files_missing_linked_paths, but this hint only removes entries through pm files. When the missing path is a linked doc, pm files <id> --remove <path> cannot remove it from metadata.docs, so the warning remains on the next validation run; the remediation needs to either mention pm docs <id> --remove <path> or choose the command based on which artifact list contains the missing path.
Useful? React with 👍 / 👎.
Codex (P2) correctly found several remediation commands did not actually resolve their findings; corrected each to the real fix path: - validate_metadata_missing_author: removed — `pm update --author` sets the audit/history author, not metadata.author, and no CLI flag writes that field. - validate_lifecycle_active_closure_like_metadata: --resolution "none" -> `pm update <id> --unset resolution` (the explicit clear path; the legacy "none" sentinel is deprecated in favor of --unset). - validate_lifecycle_stale_blockers: --unblock-note -> `pm update <id> --unset blocked-by --unset blocked-reason` (the fields the lifecycle validator actually inspects). - validate_command_references_stale_pm_ids: --body -> `pm update <id> --replace-tests --test ...` (the check scans tests[].command). - validate_files_missing/orphaned_paths: summaries now cover the pm docs path (the files check spans both item.files and item.docs). Doc/impl drift (Sourcery + Gemini): - RemediationEntry.code JSDoc reworded: resolver is first-match over disjoint colon-boundary codes, not longest-prefix (no ordering dependency). - attachValidateFixHints JSDoc notes generic hints are <id>/<field>/<path> templates, not always directly executable. - emitSettingsReadWarnings docstring clarified (surfaces settings_read_* codes). All corrected commands re-verified executable in a sandbox; full suite 2301 passed; remediation registry still 100% covered. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the thorough reviews — addressed in 37a7d85. Codex (P2 — all valid; these commands didn't actually resolve their findings):
Sourcery / Gemini (doc-impl drift — valid):
Gemini (defensive nits — no change, by design):
All corrected commands re-verified executable in a sandbox; full suite 2301 passed; registry module still 100% covered. @codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces a shared machine-executable remediation registry to map diagnostic warning codes to actionable pm fix commands. This registry is integrated into pm health --json via a new remediation_map and into pm validate via a new --fix-hints flag. Additionally, settings-read warnings are now proactively surfaced to stderr once per command instead of silently falling back to defaults. The changes are supported by updated documentation, shell completions, SDK contracts, and new integration and unit tests. Feedback on the code changes suggests adding optional chaining when accessing check.details in validate.ts and implementing a defensive type check for the warning parameter in resolveRemediation to prevent potential runtime crashes.
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.
| * output, never mutates any item. | ||
| */ | ||
| function attachValidateFixHints(check: ValidateCheck, checkWarnings: string[]): void { | ||
| const existingResolutionHints = check.details.missing_resolution_remediation_hints; |
There was a problem hiding this comment.
To prevent potential runtime TypeError crashes if check.details is undefined or null, use optional chaining when accessing missing_resolution_remediation_hints.
| const existingResolutionHints = check.details.missing_resolution_remediation_hints; | |
| const existingResolutionHints = check.details?.missing_resolution_remediation_hints; |
| export function resolveRemediation(warning: string): RemediationEntry | undefined { | ||
| const normalized = warning.trim(); |
There was a problem hiding this comment.
To ensure robustness and prevent potential runtime crashes if warning is null, undefined, or not a string, add a defensive type check at the beginning of resolveRemediation.
export function resolveRemediation(warning: string): RemediationEntry | undefined {
if (typeof warning !== "string") {
return undefined;
}
const normalized = warning.trim();
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37a7d85976
ℹ️ 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".
| code: "validate_metadata_custom_profile_missing_required_fields", | ||
| command: 'pm update <id> --<field> "<value>"', | ||
| summary: "Backfill the custom-profile required fields on the reported item(s).", |
There was a problem hiding this comment.
When pm validate --metadata-profile custom --fix-hints runs with no configured custom metadata fields, the validator emits validate_metadata_custom_profile_missing_required_fields because the validation settings are incomplete and it falls back to core fields (src/cli/commands/validate.ts); updating an item with --<field> cannot clear that configuration warning. I checked the config command registration, which supports config set metadata-required-fields --criterion ..., so this hint sends agents to mutate arbitrary items instead of fixing the setting that actually drives the finding.
Useful? React with 👍 / 👎.
| // Surface actionable settings-read warnings once per command, before the | ||
| // activation-skipped fast path returns, so create/list/search/... all report a | ||
| // corrupt settings.json instead of silently running on defaults. | ||
| emitSettingsReadWarnings(discoverySnapshot.settingsReadWarnings); |
There was a problem hiding this comment.
Because the new warning emission is only reached after the --no-extensions early return in maybeLoadRuntimeExtensions, commands such as pm --no-extensions list --json still read invalid settings through their normal command path and silently fall back to defaults. I checked runList, which still calls readSettings() directly, so users/agents that disable extensions—the common safe mode—miss the settings_read_invalid_* warning this change is meant to expose.
Useful? React with 👍 / 👎.
| code: "validate_lifecycle_active_closure_like_metadata", | ||
| command: "pm update <id> --unset resolution", |
There was a problem hiding this comment.
For validate_lifecycle_active_closure_like_metadata, buildLifecycleCheck reports whichever active-item field matched the configured closure-like patterns (blocked_reason, resolution, or actual_result), but this generic hint always unsets only resolution. When the row is, for example, pm-123:actual_result or pm-123:blocked_reason, running the advertised command leaves the same lifecycle warning in place.
Useful? React with 👍 / 👎.
| code: "validate_lifecycle_stale_blockers", | ||
| command: "pm update <id> --unset blocked-by --unset blocked-reason", |
There was a problem hiding this comment.
This code also backs validate_lifecycle_stale_blockers, but buildLifecycleCheck uses that same warning when a blocked item has neither blocked_by nor blocked_reason (blocked_status_missing_blocker_context). In that scenario both fields are already absent, so --unset blocked-by --unset blocked-reason is a no-op and the fix hint cannot clear the finding; it needs to either set blocker context or move the item out of a blocked status.
Useful? React with 👍 / 👎.
| code: "validate_files_missing_linked_paths", | ||
| command: "pm files <id> --remove <path>", |
There was a problem hiding this comment.
buildFilesCheck raises validate_files_missing_linked_paths across both item.files and item.docs, so a missing doc-only link gets the same warning code. For those rows this advertised pm files <id> --remove <path> command won't remove the doc entry, leaving validation failing; the remediation needs to distinguish docs or point agents to pm docs <id> --remove <path> when the missing link came from docs.
Useful? React with 👍 / 👎.
|
|
||
| Use dry-run modes before broad lifecycle or cleanup changes. | ||
|
|
||
| `--fix-hints` is a read-only flag: each failing check gains `details.fix_hints`, an array of executable `pm` commands derived from the warning codes it raised (for example `pm history-repair <id>` for history drift, or `pm update <id> --author "<name>"` for missing metadata). It never mutates items. The mapping comes from the shared remediation registry that also backs `pm health --json` (see Self-Repair Remediation below), so agents gating on `pm validate` can auto-repair findings without hardcoding warning-code-to-command lookups. |
There was a problem hiding this comment.
This new documentation still tells agents that missing metadata can be fixed with pm update <id> --author "<name>", but the registry intentionally removed that remediation because --author changes the audit/history author rather than metadata.author. Following this example for a missing-author validation warning records an audit mutation without clearing the finding.
Useful? React with 👍 / 👎.
| code: "validate_command_references_stale_pm_ids", | ||
| command: 'pm update <id> --replace-tests --test "command=<corrected-command>"', |
There was a problem hiding this comment.
For validate_command_references_stale_pm_ids, this hint uses pm update <id> --replace-tests --test ...; I checked runUpdate, and --replace-tests replaces the entire linked-test array with only the provided --test entries. If an item has multiple linked tests and only one command references a stale PM id, running this remediation drops every unrelated linked test on that item while fixing the stale one.
Useful? React with 👍 / 👎.
| const existingResolutionHints = check.details.missing_resolution_remediation_hints; | ||
| const fixHints = | ||
| Array.isArray(existingResolutionHints) && existingResolutionHints.length > 0 | ||
| ? existingResolutionHints.filter((hint): hint is string => typeof hint === "string") | ||
| : buildRemediationCommands(checkWarnings); |
There was a problem hiding this comment.
When more than five closed items are missing resolution metadata and --verbose-diagnostics is not set, buildResolutionCheck truncates missing_resolution_remediation_hints to the diagnostic summary limit before this alias copies it into fix_hints. The new fix_hints field has no truncation marker of its own, so an agent consuming the advertised uniform fix list will repair only the first few items and leave the remaining validation failures unresolved.
Useful? React with 👍 / 👎.
Codex re-review found 8 more cases where a hint did not reliably resolve its finding; corrected each: - validate_metadata_custom_profile_missing_required_fields: this is config-driven (the custom profile has no fields and falls back to core), so the hint now points at `pm config set metadata-required-fields --criterion ...` instead of mutating an item. - validate_lifecycle_active_closure_like_metadata: the matched field varies (resolution / actual_result / blocked_reason), so the hint is now a `--unset <closure-like-field>` template the agent fills from the detail row. - validate_lifecycle_stale_blockers: summary now also covers the blocked_status_missing_blocker_context case, where clearing the (already empty) blocker fields is a no-op and the fix is to add context or change status. - validate_files_missing/orphaned_paths: one code spans item.files and item.docs, so the hint is now a `pm <files|docs> <id> ...` template. - validate_command_references_stale_pm_ids: summary warns that --replace-tests overwrites the whole linked-test list, so unaffected tests must be re-included. - main.ts: settings_read_* warnings were skipped under --no-extensions (the common safe mode) because emission sat after the early return; now surfaced there too via a single settings read on that path. - validate --fix-hints: aliased resolution hints now propagate fix_hints_truncated so agents know the list is partial. - docs/COMMANDS.md: removed the stale `pm update --author` example (the registry intentionally dropped that hint). Also added a defensive non-string guard to resolveRemediation (exported helper SDK consumers may call from untyped JS). Full suite 2303 passed; registry 100%. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Round 2 — thanks again, Codex; all 8 follow-ups were valid and are fixed in e80d49a. Codex (round 2):
Gemini:
All corrected commands re-verified executable in a sandbox; full suite 2303 passed; registry module 100% covered. @codex review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
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". |
|
@codex review |
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
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". |
Final status — ready to mergeAll 13 CI checks pass on `e80d49aa`. Two substantive review rounds were fully addressed:
The currently-open review threads are GitHub re-anchoring the rounds 1–2 comments onto the latest commit — each is already resolved in HEAD (verified: non-string guard, optional chaining, Every corrected command was re-verified executable in a sandbox; full suite 2303 passed; the remediation module is 100% covered. |
Filed as a follow-up from PR #111 dependency review; deferred from the feature PR to avoid mixing a risky major dep bump into it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Adds machine-executable remediation so coding agents gating on
pm health/pm validatecan auto-repair findings without hardcoding warning-code → fix-command mappings. A single shared registry (src/core/diagnostics/remediation.ts) is the source of truth, consumed by both commands.This closes a tightly-related cluster under the Observability/health epic (pm-5oj5) and Schema/config epic (pm-8jdc):
pm health --jsonper-checkremediation_mappm validate --fix-hintssettings_read_*warnings proactivelyWhat changed
1. Shared remediation registry (keystone)
src/core/diagnostics/remediation.tsmaps each stable warning-code prefix → an executablepmfix command (e.g.history_drift_missing_stream→pm history-repair <id>,vectorization_stale_items_remaining→pm health --refresh-vectors). First-match resolution (codes are disjoint under colon-boundary matching). Coverage-gated at 100%.2.
pm health --json→ per-checkremediation_map(pm-0hnu)Every non-extension check whose warnings have a known code gains
details.remediation_map(code → fix command). Present in default/--full, omitted in--brief/--summary(token-efficient). Extension checks keep their richerdetails.triage.remediation.3.
pm validate --fix-hints(pm-6m3y)New read-only flag adds a uniform
details.fix_hints[]of executablepmcommands to each failing check. The resolution check aliases its existing per-row commands; other checks derive one generic command per warning code from the registry. Never mutates items.4. Settings-read warning surfacing (pm-7tcw)
readSettings()silently fell back to defaults on a corruptsettings.json. Nowmain.tsthreadssettings_read_*warnings through the discovery snapshot (zero extra reads) and prints them once per command to stderr —create/list/search/… all reportsettings_read_invalid_schema/settings_read_invalid_jsoninstead of silently using defaults. stdout/--jsonoutput is unaffected.Wiring
register-operations·cli-contracts(VALIDATE_FLAG_CONTRACTS+ validate schema contract) ·tool-parameter-tables(MCP) · bash/zsh/fish completions · docs (COMMANDS/AGENT_GUIDE/CONFIGURATION).Testing
tests/integration/remediation-registry.spec.ts) — 100% lines/branches/functions.health-command/validate-command/cli-main-errorsspecs (unit dir is at its 120-file cap).remediation_map(history drift), brief-mode omission, validate--fix-hints(generic + resolution aliasing), and stderr surfacing for both corrupt-settings cases with clean stdout.🤖 Generated with Claude Code
Summary by Sourcery
Introduce a shared remediation registry to provide machine-executable fix commands for health and validate diagnostics, expose them via new CLI surfaces, and surface settings-read issues proactively while keeping JSON output clean.
New Features:
Enhancements:
Tests: