Skip to content

Never block agents: close-via-update, semantic fallback, create form, calendar --date crash#40

Merged
unbraind merged 2 commits into
mainfrom
agent-ux-telemetry-search-audit-2026-05-21
May 21, 2026
Merged

Never block agents: close-via-update, semantic fallback, create form, calendar --date crash#40
unbraind merged 2 commits into
mainfrom
agent-ux-telemetry-search-audit-2026-05-21

Conversation

@unbraind
Copy link
Copy Markdown
Owner

@unbraind unbraind commented May 21, 2026

Summary

Telemetry (close_through_update: 86 hits over 14d), a full manual dogfood, and a code-quality audit surfaced several agent-blocking paths, a calendar crash, and verified dead code. This PR makes the highest-friction commands never hard-fail an agent, fixes the calendar bug, and removes dead code — without removing any feature.

Agent UX — never block (pm-j1v7)

  • pm update <id> --status closed auto-routes to the auditable close workflow instead of throwing close_through_update. Other field updates in the same call are applied first, then the item is closed with --close-reason, --message, or a derived default. Warnings: auto_routed_from_update_to_close, and close_reason_defaulted only when a generic placeholder was truly invented.
  • pm search --semantic / --hybrid degrade to keyword search (with a search_<mode>_fallback:<reason>:using_keyword_mode warning) when the embedding/vector backend is unreachable or unconfigured, instead of surfacing unknown_error. Keyword hits are computed locally first, so the fallback always succeeds.
  • pm create <type> <title> — the natural subcommand form now works alongside pm create "<title>"; an invalid type yields an actionable error listing allowed types.

Calendar (pm-l292)

  • pm cal <view> --date <value> no longer crashes (was extension_command_handler_failed). The positional-view guard counted loosely-parsed flag tokens as extra positionals; it now only treats leading non-flag tokens as positionals. Two positional views are still rejected. Fixed in index.ts and the committed index.js.

Code quality — remove dead code (pm-b7do)

  • Delete src/core/output/command-aware.ts (389 LOC, orphaned, previously allowlisted as zero-incoming-import).
  • Remove 5 dead exports: getActiveCommandContext, applyDynamicExtensionOptions, normalizeCalendarOptions, listDocumentedHelpPaths, uniqueTokens.
  • Remove unused direct dependency undici (transitive undici@6 via @sentry/node remains).

Verification

  • 1414 tests pass at 100% coverage (1622/1622 stmts, 1188/1188 branches, 279/279 funcs, 1568/1568 lines).
  • typecheck, static-quality, secret scan, docs-skills, version policy, npm pack --dry-run, and package-first dogfood all green.
  • Regression tests added for every behavior change; verified live in isolated sandboxes.

Follow-ups filed (not in this PR)

  • pm-7rlp CI/test performance (in-process CLI runner; dedupe redundant matrix legs — auto-release change-detection audited and confirmed correct).
  • pm-rjgh docs hygiene (stop shipping PRD.md; dedupe PRD↔docs; slim CHANGELOG; reconcile marketplace.json).
  • pm-fu5d remaining dogfood polish (test --add key validation, semantic labeling, close active-children info, etc.).
  • pm-mbdu large-file split plan (loader/extension/cli-contracts) + runUpdate field-table refactor.

🤖 Generated with Claude Code

Summary by Sourcery

Ensure high-friction CLI flows never hard-fail agents, improve calendar argument handling, and remove verified dead code and unused dependencies.

New Features:

  • Allow semantic and hybrid search modes to transparently fall back to keyword search with machine-readable warnings when semantic backends are unavailable or fail.
  • Support the natural pm create <type> <title> positional form alongside the existing title-only form.

Bug Fixes:

  • Route pm update --status closed through the auditable close workflow, applying other field updates first and deriving a close reason when needed so the operation never blocks.
  • Fix the calendar extension so pm cal <view> --date <value> treats only leading non-flag arguments as positionals, preventing crashes and still rejecting multiple positional views.

Enhancements:

  • Relax validation so non-close status transitions via pm update proceed normally while preserving close-specific auditing via the dedicated workflow.
  • Emit detailed warnings that distinguish implicit and explicit semantic mode fallbacks for better observability of degraded search behavior.
  • Tighten CLI argument normalization for create and calendar commands to align with real-world usage patterns and avoid agent-blocking UX.

Tests:

  • Update and extend unit and integration tests around update, search, create, and calendar flows to cover new routing, fallback behavior, and argument handling.

Chores:

  • Remove the orphaned command-aware output module and several unused exports from core and CLI utilities.
  • Drop the unused direct undici dependency and update static-quality scripts to reflect the removed module.
  • Add internal agent issue and chore artifacts documenting these UX, calendar, and code-quality changes.

… calendar --date

Telemetry (close_through_update: 86 hits) plus a full manual dogfood and a
code-quality audit surfaced several agent-blocking paths and dead code. This
makes the highest-friction commands never hard-fail an agent, fixes a calendar
crash, and removes verified dead code.

Agent UX — never block (pm-j1v7):
- pm update <id> --status closed now auto-routes to the auditable close
  workflow instead of throwing close_through_update. Any other field updates in
  the same call are applied first, then the item is closed with --close-reason,
  --message, or a derived default. Warnings: auto_routed_from_update_to_close,
  and close_reason_defaulted only when a generic placeholder was truly invented.
- Explicit pm search --semantic/--hybrid degrade to keyword search (with a
  search_<mode>_fallback:<reason>:using_keyword_mode warning) when the embedding
  or vector backend is unreachable or unconfigured, instead of surfacing
  unknown_error. Keyword hits are always computed locally, so the fallback is
  guaranteed to succeed.
- pm create now accepts the natural subcommand form pm create <type> <title>
  alongside pm create "<title>"; an invalid type yields an actionable error.

Calendar (pm-l292):
- pm cal <view> --date <value> no longer crashes. The positional-view guard
  counted loosely-parsed flag tokens as extra positionals; it now only treats
  leading non-flag tokens as positionals. Two positional views are still
  rejected. Fixed in both index.ts and the committed index.js.

Code quality — remove dead code (pm-b7do):
- Delete src/core/output/command-aware.ts (389 LOC, orphaned, previously
  allowlisted as zero-incoming-import in the static gate).
- Remove 5 dead exports: getActiveCommandContext, applyDynamicExtensionOptions,
  normalizeCalendarOptions, listDocumentedHelpPaths, uniqueTokens.
- Remove unused direct dependency undici (zero imports; transitive undici@6 via
  @sentry/node remains). Update lockfile and gate allowlists.

Verification: 1414 tests pass at 100% coverage (stmts/branches/funcs/lines);
typecheck, static-quality, secret scan, docs-skills, version policy,
npm pack dry-run, and package-first dogfood all green. Added regression tests
for every behavior change.

Follow-ups filed: pm-7rlp (CI/test perf), pm-rjgh (docs hygiene), pm-fu5d
(remaining dogfood polish), pm-mbdu (large-file split plan).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@sourcery-ai
Copy link
Copy Markdown

sourcery-ai Bot commented May 21, 2026

Reviewer's Guide

Makes high-friction CLI paths non-blocking for agents (update close routing, semantic search fallback, create positional handling), fixes a calendar CLI crash, and removes dead code and an unused dependency, with tests updated to lock in behavior.

Sequence diagram for pm update auto-routing close status to runClose

sequenceDiagram
  participant CLI
  participant runUpdate
  participant runClose

  CLI->>runUpdate: runUpdate(id, options, global)
  activate runUpdate
  alt options.status normal
    runUpdate-->>CLI: { item, changed_fields, warnings }
  else options.status is close_status
    runUpdate->>runUpdate: compute otherFieldKeys
    alt otherFieldKeys not empty
      runUpdate->>runUpdate: runUpdate(id, options_without_status_closeReason_message, global)
      runUpdate-->>runUpdate: preUpdate{ changed_fields, warnings }
    end
    runUpdate->>runClose: runClose(id, closeReason, closeOptions, global)
    runClose-->>runUpdate: closeResult{ item, changed_fields, warnings }
    runUpdate-->>CLI: { item: closeResult.item, changed_fields: preChangedFields + closeResult.changed_fields, warnings: routeWarnings + closeResult.warnings + auto_routed_from_update_to_close [+ close_reason_defaulted] }
  end
  deactivate runUpdate
Loading

Sequence diagram for semantic search fallback to keyword mode

sequenceDiagram
  participant CLI
  participant runSearch

  CLI->>runSearch: runSearch(query, options, global)
  activate runSearch
  runSearch->>runSearch: determine effectiveMode, modeWasExplicit
  runSearch->>runSearch: compute keywordHits
  alt effectiveMode is keyword
    runSearch-->>CLI: { hits: keywordHits, warnings }
  else effectiveMode is semantic or hybrid
    runSearch->>runSearch: attempt semantic/hybrid search
    alt semantic/hybrid succeeds
      runSearch-->>CLI: { hits, warnings }
    else [semantic or hybrid search throws error]
      alt modeWasExplicit
        runSearch->>runSearch: buildExplicitSemanticFallbackWarning(effectiveMode, error)
      else modeWasExplicit is false
        runSearch->>runSearch: buildImplicitSemanticFallbackWarning(error)
      end
      runSearch->>runSearch: effectiveMode = keyword
      runSearch->>runSearch: hits = keywordHits
      runSearch->>runSearch: warnings.push(fallbackWarning)
      runSearch-->>CLI: { hits: keywordHits, warnings }
    end
  end
  deactivate runSearch
Loading

Flow diagram for pm create positional type/title handling

flowchart TD
  A[Start pm create] --> B{typeOrTitle provided?}
  B -->|no| Z[Use only flag options]
  B -->|yes| C{secondTitle provided?}
  C -->|yes| D[Set positionalType = typeOrTitle\nSet positionalTitle = secondTitle]
  C -->|no| E[Set positionalTitle = typeOrTitle]
  D --> F
  E --> F{options.type undefined\nAND positionalType set?}
  F -->|yes| G[options.type = positionalType]
  F -->|no| H[Leave options.type unchanged]
  G --> I
  H --> I{options.title undefined\nAND positionalTitle set?}
  I -->|yes| J[options.title = positionalTitle]
  I -->|no| K[Leave options.title unchanged]
  J --> L[Call runCreate with options]
  K --> L[Call runCreate with options]
  Z --> L
Loading

File-Level Changes

Change Details Files
Route pm update --status closed through the dedicated close workflow instead of erroring, while preserving other field updates and emitting audit warnings.
  • Intercept status updates targeting the close status before mutateItem and call runClose instead, after optionally applying other field updates via a recursive runUpdate call without status/closeReason/message.
  • Derive the close reason from --close-reason, then --message, falling back to a generic "Closed via pm update" string and track whether the reason was defaulted.
  • Merge pre-close changed fields and warnings with close-result warnings, appending auto_routed_from_update_to_close and conditionally close_reason_defaulted.
  • Remove the prior close_through_update validation that rejected closed status values in the mutation path.
  • Add unit and integration tests asserting auto-routing behavior, derived reasons, warnings, and that pm update --status closed no longer fails.
src/cli/commands/update.ts
tests/unit/update-command.spec.ts
tests/integration/release-readiness-runtime.spec.ts
Ensure semantic and hybrid search modes gracefully degrade to keyword search with machine-readable fallback warnings instead of failing when semantic backends are unavailable or erroring.
  • Introduce buildExplicitSemanticFallbackWarning to construct warning codes for explicit semantic/hybrid requests based on the classified failure reason.
  • Update runSearch error handling to always fall back to keyword hits for any semantic/hybrid failure, using explicit vs implicit fallback warning builders and reusing precomputed keyword hits.
  • Adjust unit tests for runSearch to expect successful keyword-mode results and specific fallback warnings when no embedding provider/vector store is configured or when embedding backends fail or time out.
  • Update advanced search runtime tests to assert degradation to keyword mode with appropriate fallback warnings instead of thrown errors in unconfigured environments.
  • Update extension-provider and vector-adapter failure tests to assert keyword fallback with semantic fallback warnings.
src/cli/commands/search.ts
tests/unit/search-command.spec.ts
tests/unit/search-advanced-runtime.spec.ts
Fix calendar CLI crash by correctly distinguishing positional view arguments from loosely parsed flag tokens.
  • Change calendar extension handlers to treat only leading non-flag tokens in context.args as true positional arguments, stopping accumulation when a flag-like token is encountered.
  • Keep at most one positional view and throw a clear error when more than one positional view is supplied, independent of additional flags.
  • Ensure the runtime call to runCalendarPackage passes the parsed positional view along with options.
  • Add tests to validate that pm cal <view> --date <value> runs correctly and that supplying two positional views is still rejected.
packages/pm-calendar/extensions/calendar/index.ts
packages/pm-calendar/extensions/calendar/index.js
tests/unit/builtin-extension-entrypoints.spec.ts
Extend pm create CLI to support both pm create "<title>" and pm create <type> "<title>" forms and surface actionable errors for invalid types.
  • Modify create command registration to accept two optional positional arguments: a combined type-or-title and an optional second title.
  • In the create action, interpret the two positionals such that when both are present the first is treated as type and the second as title; with one positional, treat it as the title.
  • Populate options.type from the positional type when present and not already set, and maintain existing behavior for positional title -> options.title.
  • Add integration tests confirming the dual positional forms work, that invalid types return a structured invalid_argument_value error, and that the single-positional title-only form still functions.
src/cli/register-mutation.ts
tests/integration/release-readiness-runtime.spec.ts
Remove dead code and unused dependencies to improve code quality and static analysis hygiene.
  • Delete the unused command-aware output module and its orphan-module allowlist entry from the static quality gate.
  • Remove unused exports getActiveCommandContext, applyDynamicExtensionOptions, listDocumentedHelpPaths, normalizeCalendarOptions, and uniqueTokens from their respective modules.
  • Drop the direct undici dependency from package.json while keeping the transitive Sentry dependency intact, and update pnpm-lock.yaml accordingly.
  • Clean up imports and types now that CalendarOptions normalization and related helpers are removed.
  • Update tests that asserted the presence of the removed module in packaging and static-quality lists.
src/core/output/command-aware.ts
scripts/release/static-quality-gate.mjs
src/core/extensions/index.ts
src/cli/extension-command-help.ts
src/cli/help-content.ts
src/cli/registration-helpers.ts
src/core/shared/text-normalization.ts
package.json
pnpm-lock.yaml
tests/integration/release-readiness-runtime.spec.ts

Tips and commands

Interacting with Sourcery

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

Customizing Your Experience

Access your dashboard to:

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

Getting Help

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 21, 2026

Review Change Stack

Warning

Rate limit exceeded

@unbraind has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 41 minutes and 21 seconds before requesting another review.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans 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 configuration

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

Review profile: CHILL

Plan: Pro

Run ID: 5eed5aab-b58f-4fd7-b3a0-a1d29746e08c

📥 Commits

Reviewing files that changed from the base of the PR and between 3d5d60e and 7d2dc4b.

📒 Files selected for processing (6)
  • .agents/pm/chores/pm-mbdu.toon
  • .agents/pm/features/pm-rnpb.toon
  • .agents/pm/history/pm-mbdu.jsonl
  • .agents/pm/history/pm-rnpb.jsonl
  • packages/pm-calendar/extensions/calendar/index.js
  • packages/pm-calendar/extensions/calendar/index.ts
📝 Walkthrough

Walkthrough

This PR implements three interconnected agent UX behavior fixes (calendar argument parsing, search semantic fallback, update auto-routing), adds create command positional argument support, and removes 399 lines of dead code and five orphaned exported functions plus an unused dependency.

Changes

Agent UX Behavior Fixes and CLI Enhancements

Layer / File(s) Summary
Calendar positional argument parsing fix
packages/pm-calendar/extensions/calendar/index.ts, packages/pm-calendar/extensions/calendar/index.js, tests/unit/builtin-extension-entrypoints.spec.ts, .agents/pm/issues/pm-l292.toon, .agents/pm/history/pm-l292.jsonl
Calendar command now parses leading non-flag tokens as positional view arguments, correctly handling pm cal day --date 2025-05-20 by stopping at the first - prefix and validating at most one positional view, with regression tests added.
Search semantic/hybrid fallback to keyword mode
src/cli/commands/search.ts, tests/unit/search-command.spec.ts, tests/unit/search-advanced-runtime.spec.ts, .agents/pm/issues/pm-j1v7.toon, .agents/pm/history/pm-j1v7.jsonl
Explicit --semantic and --hybrid searches now degrade to keyword mode with search_<mode>_fallback:<reason> warnings when the embedding backend is unavailable or unconfigured, instead of throwing errors; both caller and implicitly-triggered fallbacks use the same degradation path.
Update command auto-routing to close workflow
src/cli/commands/update.ts, tests/unit/update-command.spec.ts, tests/integration/release-readiness-runtime.spec.ts
pm update --status closed now auto-routes through the auditable close workflow: applies other field updates first (recursively, with status/closeReason/message cleared), then closes with a reason derived from --close-reason, --message, or a default; warnings are augmented with auto_routed_from_update_to_close and close_reason_defaulted markers.
Create command positional argument support
src/cli/register-mutation.ts, tests/integration/release-readiness-runtime.spec.ts
pm create now accepts both legacy [<title>] and natural [<type> <title>] positional forms, with type defaulting when only one positional argument is provided.
Dead code and unused export removal
src/core/output/command-aware.ts, src/core/shared/text-normalization.ts, src/core/extensions/index.ts, src/cli/extension-command-help.ts, src/cli/help-content.ts, src/cli/registration-helpers.ts, package.json, scripts/release/static-quality-gate.mjs, .agents/pm/chores/pm-b7do.toon, .agents/pm/history/pm-b7do.jsonl
Removed the entire 399-line command-aware.ts module (buildCommandAwareEnvelope), deleted unused functions (normalizeCalendarOptions, getActiveCommandContext, applyDynamicExtensionOptions, listDocumentedHelpPaths, uniqueTokens), dropped undici direct dependency (transitive via @sentry/node remains), and updated static-gate allowlist to exempt src/mcp/server.ts, src/sdk/index.ts, src/types/index.ts.

Project Management Metadata and Issue Tracking

Layer / File(s) Summary
PM chores and issue tracking records
.agents/pm/chores/*, .agents/pm/history/*, .agents/pm/issues/*
Added chore records documenting CI/test performance optimization (pm-7rlp), code quality cleanup (pm-b7do), dogfood follow-ups (pm-fu5d), command file refactor audit notes (pm-mbdu), and package/documentation hygiene (pm-rjgh); added closed issue records for UX behavior fixes (pm-j1v7) and calendar crash fix (pm-l292); includes JSONL history entries for all records.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • unbraind/pm-cli#36: Both PRs modify calendar CLI positional argument handling in packages/pm-calendar/extensions/calendar/index.ts/js and option parsing/registration logic.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the four main changes in this PR: preventing agent blocking through close-via-update auto-routing and semantic search fallback, supporting a new create command form, and fixing a calendar crash.
Description check ✅ Passed The description is comprehensive and directly related to the changeset, providing clear summaries of agent UX improvements, the calendar fix, code-quality removals, and verification details.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • The runUpdate close-routing path now recursively calls runUpdate with modified options to apply pre-close field changes; consider extracting the shared mutation logic into a helper to avoid recursion and make it easier to reason about telemetry, warnings aggregation, and potential future side effects.
  • Semantic/hybrid search now always degrades to keyword on any thrown error; you may want to distinguish configuration/backend issues from programmer errors (e.g. misbehaving extensions) so that truly unexpected failures can still surface clearly instead of being silently converted into keyword results.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `runUpdate` close-routing path now recursively calls `runUpdate` with modified options to apply pre-close field changes; consider extracting the shared mutation logic into a helper to avoid recursion and make it easier to reason about telemetry, warnings aggregation, and potential future side effects.
- Semantic/hybrid search now always degrades to keyword on any thrown error; you may want to distinguish configuration/backend issues from programmer errors (e.g. misbehaving extensions) so that truly unexpected failures can still surface clearly instead of being silently converted into keyword results.

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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
.agents/pm/chores/pm-7rlp.toon (1)

1-14: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Do not edit .agents/pm files directly.

All files under .agents/pm/** in this PR (pm-7rlp.toon, pm-7rlp.jsonl, pm-fu5d.toon, pm-fu5d.jsonl, pm-mbdu.toon, pm-mbdu.jsonl, pm-rjgh.toon, pm-rjgh.jsonl) should be created and updated through the pm CLI tool commands rather than direct file edits, since pm is designated as the system of record. 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-7rlp.toon around lines 1 - 14, This change incorrectly
edits files under .agents/pm directly; revert those manual edits and instead use
the pm CLI to create/update the records (e.g., run the pm commands that
correspond to creating/updating entries with ids like pm-7rlp, pm-fu5d, pm-mbdu,
pm-rjgh and their .jsonl variants) so the system-of-record is authoritative; do
not modify files under .agents/pm by hand—use the pm tool’s create/update
commands to regenerate pm-7rlp.toon, pm-7rlp.jsonl, pm-fu5d.*, pm-mbdu.*,
pm-rjgh.* with the intended metadata.
🤖 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-b7do.toon:
- Around line 1-14: This .agents/pm file (id: pm-b7do, title: "Remove dead code:
command-aware.ts...") was added manually but .agents/pm/** is managed by the pm
tool and must not be edited directly; remove this file from the PR (or revert
the commit that added it) and instead apply the change through the pm tool so it
generates the canonical entry, or if this change must be represented in source
control, put it in the tool-managed configuration source the pm tool uses;
ensure the manual addition of pm-b7do is undone before merging.

In @.agents/pm/history/pm-b7do.jsonl:
- Around line 1-2: This change adds a manual .agents/pm history file
(pm-b7do.jsonl) which must not be created/edited directly; remove the added file
from the PR and instead re-create the same metadata using the official pm tool
(do not hand-edit .agents/pm/**). Specifically, drop the new
.agents/pm/history/pm-b7do.jsonl entry from the commit/branch and run the pm
workflow to record the metadata/title ("Remove dead code: command-aware.ts
module...") and status updates so the system-of-record is updated correctly;
ensure the resulting pm-managed entry contains the same metadata fields (id,
title, description, status, timestamps, close_reason) so the repo no longer
contains manual .agents/pm edits.

---

Outside diff comments:
In @.agents/pm/chores/pm-7rlp.toon:
- Around line 1-14: This change incorrectly edits files under .agents/pm
directly; revert those manual edits and instead use the pm CLI to create/update
the records (e.g., run the pm commands that correspond to creating/updating
entries with ids like pm-7rlp, pm-fu5d, pm-mbdu, pm-rjgh and their .jsonl
variants) so the system-of-record is authoritative; do not modify files under
.agents/pm by hand—use the pm tool’s create/update commands to regenerate
pm-7rlp.toon, pm-7rlp.jsonl, pm-fu5d.*, pm-mbdu.*, pm-rjgh.* with the intended
metadata.
🪄 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: 8f514252-1946-4404-88ef-2801f8c58ba7

📥 Commits

Reviewing files that changed from the base of the PR and between 29286fe and 3d5d60e.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (32)
  • .agents/pm/chores/pm-7rlp.toon
  • .agents/pm/chores/pm-b7do.toon
  • .agents/pm/chores/pm-fu5d.toon
  • .agents/pm/chores/pm-mbdu.toon
  • .agents/pm/chores/pm-rjgh.toon
  • .agents/pm/history/pm-7rlp.jsonl
  • .agents/pm/history/pm-b7do.jsonl
  • .agents/pm/history/pm-fu5d.jsonl
  • .agents/pm/history/pm-j1v7.jsonl
  • .agents/pm/history/pm-l292.jsonl
  • .agents/pm/history/pm-mbdu.jsonl
  • .agents/pm/history/pm-rjgh.jsonl
  • .agents/pm/issues/pm-j1v7.toon
  • .agents/pm/issues/pm-l292.toon
  • package.json
  • packages/pm-calendar/extensions/calendar/index.js
  • packages/pm-calendar/extensions/calendar/index.ts
  • scripts/release/static-quality-gate.mjs
  • src/cli/commands/search.ts
  • src/cli/commands/update.ts
  • src/cli/extension-command-help.ts
  • src/cli/help-content.ts
  • src/cli/register-mutation.ts
  • src/cli/registration-helpers.ts
  • src/core/extensions/index.ts
  • src/core/output/command-aware.ts
  • src/core/shared/text-normalization.ts
  • tests/integration/release-readiness-runtime.spec.ts
  • tests/unit/builtin-extension-entrypoints.spec.ts
  • tests/unit/search-advanced-runtime.spec.ts
  • tests/unit/search-command.spec.ts
  • tests/unit/update-command.spec.ts
💤 Files with no reviewable changes (8)
  • src/core/shared/text-normalization.ts
  • src/core/output/command-aware.ts
  • src/core/extensions/index.ts
  • scripts/release/static-quality-gate.mjs
  • src/cli/extension-command-help.ts
  • src/cli/registration-helpers.ts
  • package.json
  • src/cli/help-content.ts

Comment on lines +1 to +14
id: pm-b7do
title: "Remove dead code: command-aware.ts module, 5 orphaned exported functions, unused undici dependency"
description: "Code-quality audit (grep-verified zero call sites). Deleted src/core/output/command-aware.ts (389 LOC, orphaned by commit 5211414a; was explicitly allowlisted in static-quality-gate as zero-incoming-import). Removed dead exports getActiveCommandContext, applyDynamicExtensionOptions, normalizeCalendarOptions, listDocumentedHelpPaths, uniqueTokens. Removed unused direct dependency undici (^8.1.0; zero imports, only transitive undici@6 via @sentry/node remains). Updated the coverage-include allowlist and static-gate allowlist accordingly."
type: Chore
status: closed
priority: 2
tags[3]: code-quality,dead-code,refactor
created_at: "2026-05-21T21:56:52.288Z"
updated_at: "2026-05-21T22:09:30.435Z"
author: claude-code-agent
acceptance_criteria: All dead symbols removed with zero remaining references; build/typecheck/static-gate green; 100% coverage maintained.
parent: pm-rnpb
close_reason: "Removed src/core/output/command-aware.ts (389 LOC) + 5 orphaned exports + unused undici dep; updated coverage-include and static-gate allowlists. Build/typecheck/static-gate green, 100% coverage maintained."
body: ""
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Do not create .agents/pm files directly.

This file should be managed by the pm tool, not manually created or edited in the repository. As per coding guidelines, .agents/pm/** files are the system of record and should not be edited directly.

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-b7do.toon around lines 1 - 14, This .agents/pm file
(id: pm-b7do, title: "Remove dead code: command-aware.ts...") was added manually
but .agents/pm/** is managed by the pm tool and must not be edited directly;
remove this file from the PR (or revert the commit that added it) and instead
apply the change through the pm tool so it generates the canonical entry, or if
this change must be represented in source control, put it in the tool-managed
configuration source the pm tool uses; ensure the manual addition of pm-b7do is
undone before merging.

Comment on lines +1 to +2
{"ts":"2026-05-21T21:56:52.288Z","author":"claude-code-agent","op":"create","patch":[{"op":"add","path":"/metadata/id","value":"pm-b7do"},{"op":"add","path":"/metadata/title","value":"Remove dead code: command-aware.ts module, 5 orphaned exported functions, unused undici dependency"},{"op":"add","path":"/metadata/description","value":"Code-quality audit (grep-verified zero call sites). Deleted src/core/output/command-aware.ts (389 LOC, orphaned by commit 5211414a; was explicitly allowlisted in static-quality-gate as zero-incoming-import). Removed dead exports getActiveCommandContext, applyDynamicExtensionOptions, normalizeCalendarOptions, listDocumentedHelpPaths, uniqueTokens. Removed unused direct dependency undici (^8.1.0; zero imports, only transitive undici@6 via @sentry/node remains). Updated the coverage-include allowlist and static-gate allowlist accordingly."},{"op":"add","path":"/metadata/type","value":"Chore"},{"op":"add","path":"/metadata/status","value":"open"},{"op":"add","path":"/metadata/priority","value":2},{"op":"add","path":"/metadata/tags","value":["code-quality","dead-code","refactor"]},{"op":"add","path":"/metadata/created_at","value":"2026-05-21T21:56:52.288Z"},{"op":"add","path":"/metadata/updated_at","value":"2026-05-21T21:56:52.288Z"},{"op":"add","path":"/metadata/author","value":"claude-code-agent"},{"op":"add","path":"/metadata/acceptance_criteria","value":"All dead symbols removed with zero remaining references; build/typecheck/static-gate green; 100% coverage maintained."},{"op":"add","path":"/metadata/parent","value":"pm-rnpb"}],"before_hash":"3cc22dff72be7b14824654a7a64ea62b04799939b2fee54c1b5f52ca60bf6df0","after_hash":"114abd429fbb6a720519fa04c0bc0fa58f132091ade15557b20eefe4c30c2a16","message":""}
{"ts":"2026-05-21T22:09:30.435Z","author":"claude-code-agent","op":"close","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-05-21T22:09:30.435Z"},{"op":"replace","path":"/metadata/status","value":"closed"},{"op":"add","path":"/metadata/close_reason","value":"Removed src/core/output/command-aware.ts (389 LOC) + 5 orphaned exports + unused undici dep; updated coverage-include and static-gate allowlists. Build/typecheck/static-gate green, 100% coverage maintained."}],"before_hash":"114abd429fbb6a720519fa04c0bc0fa58f132091ade15557b20eefe4c30c2a16","after_hash":"64d5feb49a9d69fdf3e4023b3fb1dc6be239240589c9edc36822dcd5c8a6b923"}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Do not create .agents/pm files directly.

This history file should be managed by the pm tool, not manually created or edited in the repository. As per coding guidelines, .agents/pm/** files are the system of record and should not be edited directly.

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-b7do.jsonl around lines 1 - 2, This change adds a
manual .agents/pm history file (pm-b7do.jsonl) which must not be created/edited
directly; remove the added file from the PR and instead re-create the same
metadata using the official pm tool (do not hand-edit .agents/pm/**).
Specifically, drop the new .agents/pm/history/pm-b7do.jsonl entry from the
commit/branch and run the pm workflow to record the metadata/title ("Remove dead
code: command-aware.ts module...") and status updates so the system-of-record is
updated correctly; ensure the resulting pm-managed entry contains the same
metadata fields (id, title, description, status, timestamps, close_reason) so
the repo no longer contains manual .agents/pm edits.

@unbraind
Copy link
Copy Markdown
Owner Author

Thanks @coderabbitai and @sourcery-ai — addressing the feedback:

CodeRabbit — .agents/pm/** files "do not edit directly": False positive here. These .toon/.jsonl files were created by the pm tool (pm create/pm close/pm update), not hand-edited — pm is the system of record and this is exactly how it manages those files. Committing the resulting tracker changes alongside the work is the established convention in this repo (PRs #36#39 all did the same). No manual edits were made.

Sourcery #1 — recursion in the close-route: The recursive runUpdate call is bounded to exactly one level: the inner call passes status: undefined/closeReason: undefined, so it can never re-enter the close gate. It produces two intentional audit events (field update, then close) and does not double-emit telemetry (telemetry is recorded at the CLI layer, not in runUpdate). A fuller extraction of the large mutate closure into a shared helper is tracked in pm-mbdu (the runUpdate field-table refactor), where it belongs.

Sourcery #2 — semantic/hybrid degrade-on-any-error could mask unexpected failures: Good point, and intentional. The product requirement is that search must never block an agent (it's the most-used retrieval path and must be model/endpoint-agnostic). Observability is preserved without blocking: every degrade emits a machine-readable search_<mode>_fallback:<reason>:using_keyword_mode warning (reason ∈ timeout|connection|error) that is returned to the caller and captured by telemetry, so unexpected :error: fallbacks are queryable. Routing these through Sentry was considered but rejected: Sentry is lazily initialized only in error paths, so capturing here would either no-op or force async Sentry init onto the hot (successful) search path, which conflicts with the latency budget. The keyword fallback is always computed locally first, so it cannot itself fail.

CI is green and 100% coverage is maintained; merging once all required checks finish.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 21, 2026

Tip

For best results, initiate chat on the files or code changes.

@unbraind Thanks for the thorough context — all three points are well-reasoned:

  1. .agents/pm/** files — acknowledged. I'll record this convention so it isn't flagged in future PRs. These files (.toon / .jsonl) are written by the pm CLI itself as its system of record; committing them alongside the work is the correct pattern here.

  2. Bounded recursion in close-route — makes sense. Clearing status/closeReason/message on the inner call provides a clean structural guard against re-entry, and the two-event audit trail is a deliberate consequence. Happy to leave the deeper extraction to the pm-mbdu refactor where it has the right scope.

  3. Degrade-on-any-error for search — well justified. The machine-readable search_<mode>_fallback:<reason>:using_keyword_mode warnings (with reason ∈ timeout|connection|error) preserve observability for :error: cases without introducing Sentry-init latency on the hot path. The design trade-off is sound given the never-block-agent requirement.

No further concerns on my end — CI green + 100% coverage looks good. 🚀


✏️ Learnings added
Learnt from: unbraind
URL: https://github.com/unbraind/pm-cli/pull/40

Timestamp: 2026-05-21T22:17:12.521Z
Learning: In the `unbraind/pm-cli` repository, files under `.agents/pm/` (with extensions `.toon` and `.jsonl`) are generated and managed exclusively by the `pm` CLI tool itself (via `pm create`, `pm close`, `pm update`, etc.). They are never manually edited. Committing these files alongside the work they track is the established convention in this repo (see PRs `#36`–#39). These files should NOT be flagged with "do not edit directly" or similar warnings in code review.

Learnt from: unbraind
URL: https://github.com/unbraind/pm-cli/pull/40

Timestamp: 2026-05-21T22:17:12.521Z
Learning: In `unbraind/pm-cli`, the semantic/hybrid search fallback-on-any-error behavior in `src/cli/commands/search.ts` is intentional. The product requirement is that `pm search` must never block an agent. Every degradation emits a machine-readable `search_<mode>_fallback:<reason>:using_keyword_mode` warning (reason ∈ `timeout`|`connection`|`error`) returned to the caller and captured by telemetry. Sentry was explicitly rejected for this path because it is lazily initialized only in error paths and would impose async init latency on the hot (successful) search path.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several agent-UX improvements and bug fixes across the CLI. Key changes include auto-routing pm update --status closed to the auditable close workflow to prevent blocking errors, implementing a keyword fallback for explicit semantic/hybrid searches when backends are unconfigured, and supporting the pm create <type> <title> positional form. Additionally, it fixes a crash in the calendar extension when combining positional views with flags and removes significant dead code, including the command-aware.ts module and the undici dependency. Review feedback identifies a potential atomicity issue in the runUpdate refactor that could lead to partial updates and suggests using more idiomatic array methods for argument parsing in the calendar extension.

Comment on lines +1529 to 1537
if (otherFieldKeys.length > 0) {
const preUpdate = await runUpdate(
id,
{ ...options, status: undefined, closeReason: undefined, message: undefined },
global,
);
preChangedFields = preUpdate.changed_fields;
routeWarnings.push(...preUpdate.warnings);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

This recursive call to runUpdate breaks the atomicity of the update operation when closing an item. It results in two separate database transactions and history entries for a single command invocation. If the second transaction (runClose) fails, the item is left in a partially updated state.

To ensure atomicity, all changes should be performed within a single mutateItem call. I recommend refactoring the mutation logic from the main mutateItem callback into a reusable function. This function can then be used to apply all field updates (both standard updates and close-related changes) in one transaction, preventing data inconsistency and simplifying the history.

Comment on lines +46 to +52
const positionalArgs = [];
for (const arg of context.args) {
if (arg.startsWith("-")) {
break;
}
positionalArgs.push(arg);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

This for loop to extract positional arguments can be made more concise and idiomatic by using array methods like findIndex and slice. This improves readability and reduces verbosity.

      const flagIndex = context.args.findIndex(arg => arg.startsWith("-"));
      const positionalArgs = flagIndex === -1 ? context.args : context.args.slice(0, flagIndex);

Comment on lines +52 to +58
const positionalArgs: string[] = [];
for (const arg of context.args) {
if (arg.startsWith("-")) {
break;
}
positionalArgs.push(arg);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

This for loop to extract positional arguments can be made more concise and idiomatic by using array methods like findIndex and slice. This improves readability and reduces verbosity.

      const flagIndex = context.args.findIndex(arg => arg.startsWith("-"));
      const positionalArgs = flagIndex === -1 ? context.args : context.args.slice(0, flagIndex);

Apply gemini-code-assist MEDIUM suggestion: replace the manual leading-token
loop with findIndex/slice in the calendar positional-view parser (index.ts and
the committed index.js). Behaviorally identical; clearer and less verbose.

The gemini/Sourcery HIGH note on the close-route's two-write non-atomicity (for
the rare pm update --status closed + other-fields case) is tracked in pm-mbdu:
the proper single-transaction fix depends on extracting runUpdate's field
application into a reusable mutate helper that runClose can also apply. The
common close-only path is already a single atomic close.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@unbraind
Copy link
Copy Markdown
Owner Author

Thanks @gemini-code-assist:

  • MEDIUM (calendar positional parsing): Applied — replaced the manual loop with findIndex/slice in both index.ts and the committed index.js (7d2dc4b).
  • HIGH (recursive runUpdate breaks atomicity on close-with-fields): Valid, and intentionally scoped. The common pm update --status closed path (no extra fields — the actual telemetry pain point) is already a single atomic runClose, so this only affects the rare combined case. A true single-transaction fix needs runUpdate's field application extracted into a reusable mutate helper that runClose can apply (exactly your recommendation). That extraction is the runUpdate field-table refactor tracked in pm-mbdu, where it can land with full close-audit test coverage rather than being rushed onto the close path here. Until then the worst case is a valid intermediate state (fields applied, not closed) with an actionable error, not data corruption.

@gemini-code-assist
Copy link
Copy Markdown

Thanks for the update and for outlining the plan for the atomicity concern. Extracting the field-application logic into a reusable mutate helper is indeed the correct architectural path forward to ensure that runClose can handle field deltas atomically. Given that the current implementation is scoped to a rare edge case and you have a clear plan for the pm-mbdu refactor, I am satisfied with this approach. The fixes for the calendar positional parsing and the search fallback logic look solid. Since CI is green and coverage is maintained, you are good to proceed with the merge.

@unbraind unbraind merged commit 0afe2e0 into main May 21, 2026
10 checks passed
@unbraind unbraind deleted the agent-ux-telemetry-search-audit-2026-05-21 branch May 21, 2026 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant