[codex] Improve agent UX from manual pm audit#36
Merged
Conversation
Manual temp-project dogfood and sub-agent review found several small blockers in the agent loop. This patch makes config default to project-scope shorthands, accepts calendar positional views, improves GC/reindex recovery guidance, honors PM_NO_TELEMETRY, warns when closing parents with active children, lists allowed dependency kinds, and deduplicates extension action schema branches while preserving extension flag metadata. Verification: pnpm build; focused unit suites for contracts, health, telemetry, update, close, calendar, built-in extension entrypoints, and gc; pnpm quality:static; node scripts/check-secrets.mjs; package-first dogfood with and without semantic qwen Ollama mode; required Sentry/telemetry release gate with selected 100% coverage.
Reviewer's GuideImproves CLI UX and telemetry/validation guidance, enriches extension flag contracts, and deduplicates extension schema branches while updating tests and docs accordingly. Sequence diagram for close command validation with active child itemssequenceDiagram
actor User
participant CliCloseCommand as runClose
participant FindActiveChildren as findActiveChildIds
participant Mutator as mutateItem
participant CloseValidation as close_validation_block
User ->> CliCloseCommand: runClose(id, closeReasonText, options)
CliCloseCommand ->> FindActiveChildren: findActiveChildIds(pmRoot, settings, id, statusRegistry)
FindActiveChildren -->> CliCloseCommand: activeChildIds
CliCloseCommand ->> Mutator: mutateItem({ onMutate })
Mutator ->> CloseValidation: onMutate(document, mutationWarnings)
CloseValidation ->> CloseValidation: findMissingCloseValidationFields(frontMatter)
alt activeChildIds not empty and validateCloseMode strict
CloseValidation ->> CliCloseCommand: throw PmCliError
else activeChildIds not empty and validateCloseMode warn
CloseValidation ->> CloseValidation: mutationWarnings.push(close_validation_active_children)
end
CloseValidation -->> Mutator: updated document
Mutator -->> CliCloseCommand: result
CliCloseCommand -->> User: result
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ 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 (6)
📝 WalkthroughWalkthroughThis PR implements telemetry opt-out support via PM_NO_TELEMETRY, enhances the calendar command with optional positional view arguments, enforces validation on close for active children, consolidates extension contract schema generation with deduplication, makes config command arguments flexible with shorthand, and improves error messaging across multiple commands. ChangesProject Management Backlog & Evidence
CLI Features & Improvements
Sequence DiagramssequenceDiagram
participant Process as Process Environment
participant Runtime as telemetry/runtime.ts
participant Health as commands/health.ts
participant Telemetry as Telemetry System
Process->>Runtime: PM_NO_TELEMETRY set
Runtime->>Runtime: telemetryDisabledByEnvironment checks env
Runtime-->>Telemetry: disables collection
Runtime-->>Health: reports env_overrides
Health-->>Process: exposes pm_no_telemetry flag state
sequenceDiagram
participant ExtCmd as Extension Commands
participant ToContract as toExtensionFlagContract
participant Merge as mergeExtensionContractsByAction
participant BuildBranch as buildExtensionActionSchemaBranch
participant Output as Schema Output
ExtCmd->>ToContract: flag metadata
ToContract-->>Merge: CliFlagContract with description/required/value_type
Merge->>Merge: group by action, dedupe flags
Merge-->>BuildBranch: merged contracts (pipe-delimited commands)
BuildBranch->>BuildBranch: compute JSON schema types from flag.value_type
BuildBranch-->>Output: schema branch with x-extension-commands
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
![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 CLI improvements, including positional view arguments for the calendar command, validation to prevent closing items with active children, and a shorthand for project-scoped configuration. It also enhances the extension contract system with richer flag metadata and deduplication logic. Review feedback highlighted a missing import for isTerminal in the close command and identified logic errors in the merging of extension command aliases, which could lead to duplicate entries in the generated schema.
| @@ -1,10 +1,11 @@ | |||
| import { pathExists } from "../../core/fs/fs-utils.js"; | |||
| import { normalizeStatusInput } from "../../core/item/status.js"; | |||
There was a problem hiding this comment.
The isTerminal function is used in findActiveChildIds (line 96) but is not imported in this file. It should be imported from ../../core/item/status.js along with normalizeStatusInput.
Suggested change
| import { normalizeStatusInput } from "../../core/item/status.js"; | |
| import { isTerminal, normalizeStatusInput } from "../../core/item/status.js"; |
| additionalProperties: true, | ||
| "x-extension-source": contract.source, | ||
| "x-extension-command": contract.command, | ||
| "x-extension-commands": [contract.command], |
There was a problem hiding this comment.
The x-extension-commands field should be an array of individual command names. Since contract.command can be a pipe-separated string (especially after the merge logic introduced in this PR), it should be split here. This also allows for simplifying the caller logic in runContracts.
Suggested change
| "x-extension-commands": [contract.command], | |
| "x-extension-commands": contract.command.split("|"), |
Comment on lines
+798
to
+800
| existing.command = [...new Set([existing.command, contract.command])] | ||
| .sort((left, right) => left.localeCompare(right)) | ||
| .join("|"); |
There was a problem hiding this comment.
The deduplication logic for existing.command is incorrect when merging multiple contracts. Since existing.command or contract.command might already be a pipe-separated string (e.g., from a previous merge), simply putting them into a Set will result in duplicates like "a|b|a". You should split the strings before deduplicating.
Suggested change
| existing.command = [...new Set([existing.command, contract.command])] | |
| .sort((left, right) => left.localeCompare(right)) | |
| .join("|"); | |
| existing.command = [...new Set([...existing.command.split("|"), ...contract.command.split("|")])] | |
| .sort((left, right) => left.localeCompare(right)) | |
| .join("|"); |
Comment on lines
+1556
to
+1560
| .map((contract) => { | ||
| const branch = buildExtensionActionSchemaBranch(contract); | ||
| branch["x-extension-commands"] = contract.command.split("|"); | ||
| return branch; | ||
| }); |
There was a problem hiding this comment.
With the fix to buildExtensionActionSchemaBranch to handle command splitting internally, this manual assignment and the multi-line map block become redundant. You can revert this to a concise single-line map.
.map((contract) => buildExtensionActionSchemaBranch(contract));![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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-5k2w.toon:
- Around line 9-43: This change illegally edits the system-of-record file
.agents/pm/chores/pm-5k2w.toon; revert the manual edits and regenerate the chore
via the official pm workflow/commands (use the pm CLI/operations that produce
the chore output tied to parent id pm-rnpb), then commit only the generated
artifacts that the pm workflow permits; do not directly modify any files under
.agents/pm/** — locate the chore by its filename
(.agents/pm/chores/pm-5k2w.toon) and parent reference (pm-rnpb) and run the pm
regeneration pipeline to produce an approved update.
In @.agents/pm/history/pm-rnpb.jsonl:
- Around line 419-424: This change directly edits system-of-record `.agents/pm`
history entries by applying JSON patch ops (replace/remove/add) to metadata
fields (e.g., metadata.assignee, metadata.updated_at and metadata.comments) —
revert these edits in pm-rnpb.jsonl and instead perform the same state
transitions through the pm workflow/commands (use the pm CLI or automation to
claim/release and add comments) so that the repository file is not edited
directly; specifically undo the applied patch operations (the add/replace/remove
of metadata.comments, metadata.assignee, metadata.updated_at) and re-apply the
intended claim/comment/release actions via the pm process.
In `@src/cli/commands/contracts.ts`:
- Around line 1554-1560: Validation is using unmerged extension contracts so
aliases get rejected; update the command/action validation to use the merged
results from mergeExtensionContractsByAction (or the derived extensionBranches
built by buildExtensionActionSchemaBranch) instead of the original
extensionContracts, and when matching a selected command against an action split
the stored command string into aliases (contract.command.split("|") /
extensionBranches["x-extension-commands"]) so the check is alias-aware; ensure
schemaActionSet logic still prevents duplicate actions but command matching uses
the merged alias list.
- Around line 524-529: The current fallback assigns rawValueType from
definition.value_type, definition.type, or valueName but doesn't validate that
definition.type is one of the allowed primitives, causing invalid types like
"integer" to block the valueName -> "string" fallback; change the logic around
rawValueType to validate each candidate in order (check
toOptionalTrimmedString(definition.value_type) first and accept it only if it's
"string"|"number"|"boolean", otherwise check
toOptionalTrimmedString(definition.type) and accept it only if it's one of those
three, and only if neither candidate is valid use valueName ? "string" : null),
then set contract.value_type from that validated rawValueType.
In `@src/cli/register-setup.ts`:
- Around line 401-407: The shorthand parsing moves an action into scope but
doesn't shift the subsequent arguments, so when scope matches actionShorthands
(see actionShorthands, resolvedScope, resolvedAction) the original key argument
must be pulled from the next positional parameter; update the resolution logic
before calling runConfig to set key = scope && actionShorthands.has(scope) ?
action : key (or equivalent) so runConfig(resolvedScope, resolvedAction, key,
...) receives the correct third argument when using the shorthand syntax.
🪄 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: 656b663b-12bd-437c-add4-772e443f3833
📒 Files selected for processing (21)
.agents/pm/chores/pm-5k2w.toon.agents/pm/features/pm-rnpb.toon.agents/pm/history/pm-5k2w.jsonl.agents/pm/history/pm-rnpb.jsonlpackages/pm-calendar/extensions/calendar/index.jspackages/pm-calendar/extensions/calendar/index.tssrc/cli/commands/close.tssrc/cli/commands/contracts.tssrc/cli/commands/gc.tssrc/cli/commands/health.tssrc/cli/commands/update.tssrc/cli/main.tssrc/cli/register-setup.tssrc/core/telemetry/runtime.tssrc/sdk/cli-contracts.tstests/unit/close-command.spec.tstests/unit/contracts-command.spec.tstests/unit/gc-command.spec.tstests/unit/health-command.spec.tstests/unit/telemetry-runtime.spec.tstests/unit/update-command.spec.ts
Comment on lines
+9
to
+43
| updated_at: "2026-05-21T04:42:31.292Z" | ||
| author: claude-code-agent | ||
| acceptance_criteria: Each listed sub-item is either fixed with a regression test or explicitly declined with rationale; no change drops coverage below 100%; doc/validator drift (L6) resolved; default-safety inconsistencies (L10) reviewed. | ||
| parent: pm-rnpb | ||
| comments[3]{created_at,author,text}: | ||
| "2026-05-21T04:30:32.081Z",codex-full-manual-audit,"Evidence: focused manual smoke passed config list shorthand, cal week positional view, close active-child warning, gc reindex install guidance, and PM_NO_TELEMETRY health reporting. Local checks passed so far: pnpm build; focused 7-file suite 37 tests after calendar runtime fix and 6-file suite 129 tests before that; pnpm quality:static; node scripts/check-secrets.mjs." | ||
| "2026-05-21T04:40:05.493Z",codex-full-manual-audit,"Evidence: full release gate passed after stale GC expectation update. Passed build, typecheck, docs-skills, static quality, coverage 96 files / 1399 tests at 100 percent, version policy, secret scan, npx packed smoke, package-first dogfood 84 workflows, npm pack dry-run, compatibility, and required Sentry/telemetry gate with Sentry critical=0 high=1 and telemetry finish_error_rate_pct=2.71." | ||
| "2026-05-21T04:42:28.101Z",codex-full-manual-audit,"Pause evidence: focused subset fixed and release-gated. Fixed config list shorthand, calendar positional view handling, GC missing-reindex guidance, PM_NO_TELEMETRY opt-out reporting/runtime, invalid dependency kind recovery, close active-child warning, and extension contract metadata/alias dedupe. Remaining backlog items on this chore stay open: init verbosity, help alias display bloat, create changed_fields noise, calendar include scheduled doc drift, guide install hints, priority wording drift, template starter content, broader default-safety review, and event equal-timestamp hinting. Full gate passed with run-gates telemetry-mode required and 100% selected coverage." | ||
| files[19]{path,scope,note}: | ||
| packages/pm-calendar/extensions/calendar/index.js,project,compiled calendar package entry kept in sync | ||
| packages/pm-calendar/extensions/calendar/index.ts,project,calendar positional view support | ||
| src/cli/commands/close.ts,project,close validation warns on active children | ||
| src/cli/commands/contracts.ts,project,extension action schema aliases and flag metadata | ||
| src/cli/commands/gc.ts,project,gc reindex install guidance | ||
| src/cli/commands/health.ts,project,health reports PM_NO_TELEMETRY alias | ||
| src/cli/commands/update.ts,project,dependency kind recovery lists allowed values | ||
| src/cli/main.ts,project,dynamic extension command action arg handling | ||
| src/cli/register-setup.ts,project,config action shorthand for agent UX | ||
| src/core/telemetry/runtime.ts,project,PM_NO_TELEMETRY opt-out alias | ||
| src/sdk/cli-contracts.ts,project,extension flag contract metadata | ||
| tests/unit/builtin-extension-entrypoints.spec.ts,project,Built-in extension entrypoint smoke coverage | ||
| tests/unit/calendar-command.spec.ts,project,Calendar positional view smoke coverage | ||
| tests/unit/close-command.spec.ts,project,Regression test for active-child close warning | ||
| tests/unit/contracts-command.spec.ts,project,Regression tests for extension contract metadata and alias dedupe | ||
| tests/unit/gc-command.spec.ts,project,gc guidance regression coverage | ||
| tests/unit/health-command.spec.ts,project,Regression test for telemetry env override reporting | ||
| tests/unit/telemetry-runtime.spec.ts,project,Regression test for PM_NO_TELEMETRY opt-out | ||
| tests/unit/update-command.spec.ts,project,Regression test for dependency kind guidance | ||
| tests[6]{command,path,scope,timeout_seconds,pm_context_mode,env_set,env_clear,shared_host_safe,assert_stdout_contains,assert_stdout_regex,assert_stderr_contains,assert_stderr_regex,assert_stdout_min_lines,assert_json_field_equals,assert_json_field_gte,note}: | ||
| node scripts/check-secrets.mjs,null,project,120,null,null,null,null,null,null,null,null,null,null,null,null | ||
| node scripts/release/run-gates.mjs --telemetry-mode required --max-sentry-critical 10 --max-sentry-high 20 --max-telemetry-error-rate 10 --json,null,project,420,null,null,null,null,null,null,null,null,null,null,null,null | ||
| PM_RUN_TESTS_SKIP_BUILD=1 node scripts/run-tests.mjs test -- tests/unit/contracts-command.spec.ts tests/unit/health-command.spec.ts tests/unit/telemetry-runtime.spec.ts tests/unit/update-command.spec.ts tests/unit/close-command.spec.ts tests/unit/calendar-command.spec.ts tests/unit/builtin-extension-entrypoints.spec.ts --reporter=dot,null,project,180,null,null,null,null,null,null,null,null,null,null,null,null | ||
| PM_RUN_TESTS_SKIP_BUILD=1 node scripts/run-tests.mjs test -- tests/unit/gc-command.spec.ts tests/unit/calendar-command.spec.ts --reporter=dot,null,project,120,null,null,null,null,null,null,null,null,null,null,null,null | ||
| pnpm build,null,project,120,null,null,null,null,null,null,null,null,null,null,null,null | ||
| "pnpm quality:static",null,project,120,null,null,null,null,null,null,null,null,null,null,null,null |
There was a problem hiding this comment.
Blocked: direct edits to pm system-of-record files
This change directly edits .agents/pm/chores/pm-5k2w.toon. Please move these updates through the pm workflow/commands instead of committing direct file edits (e.g., regenerate records via pm operations, then commit resulting artifacts only if that workflow permits it).
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-5k2w.toon around lines 9 - 43, This change illegally
edits the system-of-record file .agents/pm/chores/pm-5k2w.toon; revert the
manual edits and regenerate the chore via the official pm workflow/commands (use
the pm CLI/operations that produce the chore output tied to parent id pm-rnpb),
then commit only the generated artifacts that the pm workflow permits; do not
directly modify any files under .agents/pm/** — locate the chore by its filename
(.agents/pm/chores/pm-5k2w.toon) and parent reference (pm-rnpb) and run the pm
regeneration pipeline to produce an approved update.
Comment on lines
+419
to
+424
| {"ts":"2026-05-21T04:10:57.898Z","author":"codex-full-manual-audit","op":"claim","patch":[{"op":"replace","path":"/metadata/assignee","value":"codex-full-manual-audit"},{"op":"replace","path":"/metadata/updated_at","value":"2026-05-21T04:10:57.898Z"}],"before_hash":"30796ce302702e59341965498e3172f4043864d1251ec049573f392b5e5968f6","after_hash":"5e91b390a1b9c6967b7b490415564b6a1ea8cbc4f499258d97bb6a70d717d735"} | ||
| {"ts":"2026-05-21T04:11:00.194Z","author":"codex-full-manual-audit","op":"comment_add","patch":[{"op":"add","path":"/metadata/comments/155","value":{"created_at":"2026-05-21T04:11:00.194Z","author":"codex-full-manual-audit","text":"Investigation note: Reusing existing canonical audit item after duplicate check. Scope covers full latest-build manual temp-project dogfood, CLI/SDK/calendar/search/performance/security/CI review, GitHub issue/PR/dependency review, telemetry/Sentry analysis, and targeted fixes discovered during the run."}},{"op":"replace","path":"/metadata/updated_at","value":"2026-05-21T04:11:00.194Z"}],"before_hash":"5e91b390a1b9c6967b7b490415564b6a1ea8cbc4f499258d97bb6a70d717d735","after_hash":"7e8b173524da7f9af4816265f23c608168a0c242aa019957b47c88c8f76d9a27"} | ||
| {"ts":"2026-05-21T04:17:16.756Z","author":"codex-full-manual-audit","op":"comment_add","patch":[{"op":"add","path":"/metadata/comments/156","value":{"created_at":"2026-05-21T04:17:16.756Z","author":"codex-full-manual-audit","text":"Sub-agent/manual audit findings mapped without duplicate items: reuse pm-5k2w for config/calendar/telemetry/dependency/gc polish, pm-59gj for multi-package install/catalog UX, pm-rnpb for semantic provider staleness and auto-index provider parity, pm-orrl for blocked-by semantics, pm-nbht for health brevity, and pm-gt82/pm-mbdu for startup and large-file follow-ups."}},{"op":"replace","path":"/metadata/updated_at","value":"2026-05-21T04:17:16.756Z"}],"before_hash":"7e8b173524da7f9af4816265f23c608168a0c242aa019957b47c88c8f76d9a27","after_hash":"a5dc529e78b7a274c7a8cba1ac06ba14ec119c2112b57d3541ec5ec2b53c2907"} | ||
| {"ts":"2026-05-21T04:40:06.326Z","author":"codex-full-manual-audit","op":"comment_add","patch":[{"op":"add","path":"/metadata/comments/157","value":{"created_at":"2026-05-21T04:40:06.326Z","author":"codex-full-manual-audit","text":"Evidence: latest sub-agent/manual audit and fixes completed under pm-5k2w. Dogfood passed normal 84 workflows and semantic 86 workflows using qwen3-embedding:0.6b; release gate passed with coverage 96 files / 1399 tests at 100 percent and required Sentry/telemetry gate. Residual performance/coverage breadth/semantic provider-staleness findings remain mapped to existing pm-rnpb/pm-gt82/pm-mbdu/pm-59gj follow-ups, not duplicated."}},{"op":"replace","path":"/metadata/updated_at","value":"2026-05-21T04:40:06.326Z"}],"before_hash":"a5dc529e78b7a274c7a8cba1ac06ba14ec119c2112b57d3541ec5ec2b53c2907","after_hash":"8d3151783bec6150d9703bde1d6fcefc4e0e4db1933448b42f2db708fe9899a3"} | ||
| {"ts":"2026-05-21T04:42:32.110Z","author":"codex-full-manual-audit","op":"comment_add","patch":[{"op":"add","path":"/metadata/comments/158","value":{"created_at":"2026-05-21T04:42:32.110Z","author":"codex-full-manual-audit","text":"Audit closeout evidence: broad manual dogfood and sub-agent review completed for this slice. Local release gate passed with telemetry required, package-first dogfood passed including semantic qwen Ollama mode, Sentry/telemetry gate passed against current baseline, GitHub issue/PR/vulnerability scan found no open actionable GitHub items. Residuals remain tracked under existing lineage: broader read-path performance, semantic provider staleness/auto-index parity, CI duration reduction beyond current selected coverage gate, and telemetry queue backlog investigation."}},{"op":"replace","path":"/metadata/updated_at","value":"2026-05-21T04:42:32.110Z"}],"before_hash":"8d3151783bec6150d9703bde1d6fcefc4e0e4db1933448b42f2db708fe9899a3","after_hash":"e517f235a79c56993d9b37491d2aea0e5db9acfd15af3484b6463de782d66197"} | ||
| {"ts":"2026-05-21T04:42:33.995Z","author":"codex-full-manual-audit","op":"release","patch":[{"op":"remove","path":"/metadata/assignee"},{"op":"replace","path":"/metadata/updated_at","value":"2026-05-21T04:42:33.995Z"}],"before_hash":"e517f235a79c56993d9b37491d2aea0e5db9acfd15af3484b6463de782d66197","after_hash":"7bb74c15805433c505e385db88b4ed37dc5ba9be420eaf75a24683a4498f38e1"} |
There was a problem hiding this comment.
Do not directly edit .agents/pm history records.
Lines 419-424 directly modify .agents/pm/history/pm-rnpb.jsonl, which violates the repository rule for system-of-record data. Please apply these state transitions through the pm workflow/commands instead of committing file edits.
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/history/pm-rnpb.jsonl around lines 419 - 424, This change
directly edits system-of-record `.agents/pm` history entries by applying JSON
patch ops (replace/remove/add) to metadata fields (e.g., metadata.assignee,
metadata.updated_at and metadata.comments) — revert these edits in pm-rnpb.jsonl
and instead perform the same state transitions through the pm workflow/commands
(use the pm CLI or automation to claim/release and add comments) so that the
repository file is not edited directly; specifically undo the applied patch
operations (the add/replace/remove of metadata.comments, metadata.assignee,
metadata.updated_at) and re-apply the intended claim/comment/release actions via
the pm process.
![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 2 issues, and left some high level feedback:
- In
mergeExtensionContractsByAction, flags are deduplicated solely byflag.flag|flag.short, so if two commands define the same flag name with differing descriptions/required/value_type metadata, one copy’s richer metadata may be silently discarded; consider either validating consistency between merged flags or explicitly defining which source takes precedence so that extension authors get predictable results. - The handling of
PM_NO_TELEMETRYis now duplicated betweentelemetryDisabledByEnvironmentinsrc/core/telemetry/runtime.tsandtelemetryEnvFlagEnabledconsumers inhealth.ts; it may be worth centralizing this environment flag resolution in a single helper to avoid drift if semantics aroundPM_NO_TELEMETRYchange in the future.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `mergeExtensionContractsByAction`, flags are deduplicated solely by `flag.flag|flag.short`, so if two commands define the same flag name with differing descriptions/required/value_type metadata, one copy’s richer metadata may be silently discarded; consider either validating consistency between merged flags or explicitly defining which source takes precedence so that extension authors get predictable results.
- The handling of `PM_NO_TELEMETRY` is now duplicated between `telemetryDisabledByEnvironment` in `src/core/telemetry/runtime.ts` and `telemetryEnvFlagEnabled` consumers in `health.ts`; it may be worth centralizing this environment flag resolution in a single helper to avoid drift if semantics around `PM_NO_TELEMETRY` change in the future.
## Individual Comments
### Comment 1
<location path="src/cli/commands/contracts.ts" line_range="772" />
<code_context>
additionalProperties: true,
"x-extension-source": contract.source,
"x-extension-command": contract.command,
+ "x-extension-commands": [contract.command],
};
}
</code_context>
<issue_to_address>
**suggestion:** Keep `x-extension-command` consistent with the new multi-command merge behavior.
After `mergeExtensionContractsByAction`, `contract.command` can be a `|`-joined list. `buildExtensionActionSchemaBranch` copies this into both `"x-extension-command"` and `"x-extension-commands"`, and later `runContracts` converts only `"x-extension-commands"` to an array, leaving `"x-extension-command"` as the joined string.
To avoid ambiguous semantics for consumers, consider either deriving `"x-extension-command"` from the split array (e.g., first element), removing `"x-extension-command"` in favor of `"x-extension-commands"`, or explicitly documenting that `"x-extension-command"` may be `|`-delimited.
```suggestion
const commands =
typeof contract.command === "string"
? contract.command
.split("|")
.map((cmd) => cmd.trim())
.filter((cmd) => cmd.length > 0)
: Array.isArray(contract.command)
? contract.command
: [];
return {
additionalProperties: true,
"x-extension-source": contract.source,
"x-extension-command": commands[0],
"x-extension-commands": commands,
};
```
</issue_to_address>
### Comment 2
<location path="tests/unit/health-command.spec.ts" line_range="127-130" />
<code_context>
attempted: false,
},
env_overrides: {
- telemetry_disabled: telemetryEnvFlagEnabled("PM_TELEMETRY_DISABLED"),
+ telemetry_disabled: telemetryEnvFlagEnabled("PM_TELEMETRY_DISABLED") || telemetryEnvFlagEnabled("PM_NO_TELEMETRY"),
</code_context>
<issue_to_address>
**suggestion (testing):** Add a health check test case where PM_NO_TELEMETRY is the sole disabling flag
To cover this path, please add a test where `PM_NO_TELEMETRY` is set while `PM_TELEMETRY_DISABLED` and `PM_TELEMETRY_OTEL_DISABLED` are unset, and assert: `env_overrides.telemetry_disabled === true`, `env_overrides.pm_no_telemetry === true`, and `env_overrides.telemetry_otel_disabled === false`. This confirms `PM_NO_TELEMETRY` alone disables telemetry and is reported distinctly from the other flags in the health output.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
127
to
130
| env_overrides: { | ||
| telemetry_disabled: true, | ||
| pm_no_telemetry: false, | ||
| telemetry_otel_disabled: true, |
There was a problem hiding this comment.
suggestion (testing): Add a health check test case where PM_NO_TELEMETRY is the sole disabling flag
To cover this path, please add a test where PM_NO_TELEMETRY is set while PM_TELEMETRY_DISABLED and PM_TELEMETRY_OTEL_DISABLED are unset, and assert: env_overrides.telemetry_disabled === true, env_overrides.pm_no_telemetry === true, and env_overrides.telemetry_otel_disabled === false. This confirms PM_NO_TELEMETRY alone disables telemetry and is reported distinctly from the other flags in the health output.
Fix config shorthand key shifting, validate extension flag value_type fallbacks before using valueName defaults, make merged extension command metadata deterministic, merge duplicate flag metadata predictably, and cover PM_NO_TELEMETRY health reporting. Verification: pnpm exec tsc --noEmit -p tsconfig.json; PM_RUN_TESTS_SKIP_BUILD=1 node scripts/run-tests.mjs test -- tests/unit/contracts-command.spec.ts tests/unit/health-command.spec.ts tests/integration/cli.integration.spec.ts -t "extension|PM_NO_TELEMETRY|config" --reporter=dot.
Split merged extension command aliases when matching selected commands, deriving scoped command names, and resolving runtime availability. This keeps schema dedupe while preserving invocability for aliases like cal/calendar and package-provided action commands. Verification: pnpm exec tsc --noEmit -p tsconfig.json; PM_RUN_TESTS_SKIP_BUILD=1 node scripts/run-tests.mjs test -- tests/unit/calendar-command.spec.ts tests/unit/contracts-command.spec.ts tests/unit/health-command.spec.ts --reporter=dot; node scripts/dogfood-package-first.mjs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pm configdefault to project-scope shorthand formspm cal weekpositional view usage and preserves dynamic extension command routingValidation
pnpm buildpnpm quality:staticnode scripts/check-secrets.mjsnode scripts/dogfood-package-first.mjsPM_DOGFOOD_SEMANTIC=1 node scripts/dogfood-package-first.mjsnode scripts/release/run-gates.mjs --telemetry-mode required --max-sentry-critical 10 --max-sentry-high 20 --max-telemetry-error-rate 10 --jsonNotes
Summary by Sourcery
Improve CLI UX and telemetry controls, enrich extension contracts, and tighten validation and guidance across close, calendar, GC, and update workflows.
New Features:
Bug Fixes:
Enhancements:
Tests: