Agent UX, search discoverability, calendar TOON + small refactors#81
Conversation
…ll refactors Combines work for 5 closed pm items: - pm-ydkl (Fixed): Add defensive HTML-entity decode at MCP server boundary so Claude/MCP-SDK-encoded "<type>" stored verbatim becomes "<type>" again. Single-pass via lookup map preserves intentional double-encoding. No-op for normal text. 19 unit tests, 100% coverage. - pm-9r7z (Fixed): MCP pm_create / pm_update crashed with "raw.trim is not a function" when priority arrived as a JSON number. resolvePriority now accepts string|number with strict Number.isInteger 0..4 range check; ensurePriority callers in create.ts and update.ts widened to match. Added 3 priority tests covering native-number happy-path, out-of-range rejection, and defense against null/undefined/object/array/boolean. - pm-7ilo (Added): 16 new CLI config aliases for search/provider/vector-store leaves (search_provider, ollama_base_url, openai_api_key, qdrant_url, lancedb_path, etc.) via a new descriptor table in src/core/config/nested-settings.ts. CONFIGURATION.md gains a "Custom search providers" section with copy-pasteable examples for Ollama / OpenAI / LM Studio / vLLM / Qdrant / LanceDB. Semantic + hybrid search now emit a one-line vector-index-stale warning at query time (stderr + result.warnings) by lifting collectStaleVectorizationIds from health.ts into a shared src/core/search/staleness.ts helper. - pm-xzrx (Added): "pm calendar --format toon" now emits actual TOON via a new renderCalendarToon() that runs CalendarResult through @toon-format/toon (previously fell through to markdown). Schedule-less Milestone / Meeting / Reminder / Event creates now emit a warning to stderr at create time including the just-allocated id and a copy-pasteable "pm update --deadline" suggestion. - pm-1b96 (Changed): Add splitCommaList helper at src/core/shared/split-comma-list.ts and consolidate 7 hand-rolled split/ trim/filter/dedup idioms (aggregate, linked-test-parsers x2, update-many, plan x2, extension/bundled-catalog). Drop 16 verified-dead exports across help-json-payload.ts (10) and registration-helpers.ts (6). Bigger items (runUpdate/runCreate split, cli-contracts barrel, toItemMetadata, parseEvents dedup) deferred to follow-up PRs. Verification: - 1933/1933 unit + integration tests pass (132 files) - 100% line/branch/function/statement coverage across the gated include list - End-to-end dogfood in /tmp/pm-e2e-2026-05-28: all four user-facing surfaces (config aliases, calendar TOON, schedule-less warning, search staleness warning) verified working against the freshly built dist - pnpm changelog:pm:check green (CHANGELOG auto-generated) Two follow-up items filed: pm-1lws (pm update should accept --expected / --actual aliases that pm close accepts, plus --add-tags vs --tags inconsistency in the retry hint). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Reviewer's GuideImplements agent-focused UX improvements: adds first-class CLI aliases and plumbing for search/provider/vector-store nested settings, introduces a proper TOON calendar output format with schedule-less item warnings, hardens MCP/priority handling and vector-index staleness surfacing, and performs small refactors (shared helpers, dead export removal, new tests/docs) to make these behaviors robust and discoverable. Sequence diagram for pm config nested search/provider/vector-store settingssequenceDiagram
actor User
participant CliConfigCommand as runConfig
participant NestedSettings as resolveNestedSettingDescriptor
participant Parser as parseNestedSettingValue
participant Writer as writeNestedSettingValue
participant Store as writeSettings
User->>CliConfigCommand: pm config project set search_provider openai
CliConfigCommand->>NestedSettings: resolveNestedSettingDescriptor(keyValue)
NestedSettings-->>CliConfigCommand: nestedSetting
CliConfigCommand->>CliConfigCommand: applyPositionalValue(action, keyValue, key, nestedSetting, valueValue, options)
CliConfigCommand->>Parser: parseNestedSettingValue(nestedSetting, rawValue)
Parser-->>CliConfigCommand: { ok: true, parsed }
CliConfigCommand->>Writer: writeNestedSettingValue(settings, nestedSetting, parsed.value)
Writer-->>CliConfigCommand: changed
CliConfigCommand->>Store: writeSettings(pmRoot, settings, "config:set:"+nestedSetting.path)
Store-->>CliConfigCommand: (completed)
CliConfigCommand-->>User: ConfigResult{ nested_setting, changed:true }
Sequence diagram for semantic search vector-index staleness warningsequenceDiagram
actor User
participant SearchCmd as runSearch
participant StaleWarn as maybeEmitVectorIndexStaleWarning
participant Ledger as readVectorizationStatusLedger
participant Staleness as collectStaleVectorizationIds
participant Stderr as process.stderr
User->>SearchCmd: pm search --mode semantic
SearchCmd->>SearchCmd: effectiveMode !== "keyword" & modeWasExplicit
SearchCmd->>StaleWarn: maybeEmitVectorIndexStaleWarning(pmRoot, filteredDocuments, warnings)
StaleWarn->>Ledger: readVectorizationStatusLedger(pmRoot)
Ledger-->>StaleWarn: ledger{ entries, warnings }
StaleWarn->>Staleness: collectStaleVectorizationIds(filteredDocuments, ledger.entries)
Staleness-->>StaleWarn: staleIds
alt staleIds.length > 0
StaleWarn->>StaleWarn: warnings.push("vector_index_stale:"+staleIds.length)
StaleWarn->>Stderr: write("[pm] warning: vector index has ...")
else no stale ids
StaleWarn-->>SearchCmd: (no-op)
end
SearchCmd-->>User: SearchResult with warnings
Sequence diagram for pm calendar TOON output formatsequenceDiagram
actor User
participant CalendarExt as renderCalendarPackageOutput
participant Core as runCalendar
participant Format as resolveCalendarOutputFormat
participant Toon as renderCalendarToon
User->>CalendarExt: pm calendar --format toon
CalendarExt->>Core: runCalendar(options, global)
Core-->>CalendarExt: CalendarResult
CalendarExt->>Format: resolveCalendarOutputFormat(options, global)
Format-->>CalendarExt: "toon"
CalendarExt->>Toon: renderCalendarToon(result)
Toon-->>CalendarExt: toonString
CalendarExt-->>User: toonString + "\n"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds shared splitCommaList, nested-setting catalog and CLI support (16 aliases), calendar TOON rendering and schedule-less warnings, query-time vector-index staleness detection, defensive HTML-entity decoding at MCP, numeric priority input support, removal of module exports, and tests/docs/changelog updates. ChangesCore Utilities, Cleanup, and Infrastructure
Nested configuration system
Search and Calendar Features
Command Refactoring and Testing
Project metadata and documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. ✨ Finishing Touches📝 Generate docstrings
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
|
/gemini review |
|
@gemini-code-assist review |
![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 code-quality refactors and features, including a new splitCommaList helper to consolidate comma-separated list parsing, nested-setting CLI aliases for search and vector-store configurations, a query-time index staleness warning, a calendar TOON renderer, and defensive HTML-entity decoding at the MCP server boundary. The review feedback highlights a few important issues: a bug in the nested settings --value option handling that incorrectly throws an error when positional values are omitted, a parsing issue where empty strings are coerced to 0 for numeric types, a potential prototype-stripping issue in the HTML decoder when traversing non-plain objects, and a missing defensive guard against a null or undefined vectorization ledger.
| if (nestedSetting) { | ||
| if (options.value !== undefined && options.value !== valueValue) { | ||
| throw new PmCliError( | ||
| `Config set ${keyValue} received both positional value "${valueValue}" and --value "${options.value}". Pass only one.`, | ||
| EXIT_CODE.USAGE, | ||
| ); | ||
| } | ||
| return { ...options, value: valueValue }; | ||
| } |
There was a problem hiding this comment.
Using --value with nested settings is currently broken. If a user passes --value <value> without a positional value, valueValue is undefined, which triggers the error condition options.value !== valueValue and throws a PmCliError. Additionally, returning { ...options, value: valueValue } would overwrite the --value option with undefined. We should allow either positional or --value options, throw only if both are provided, and correctly fall back to options.value.
| if (nestedSetting) { | |
| if (options.value !== undefined && options.value !== valueValue) { | |
| throw new PmCliError( | |
| `Config set ${keyValue} received both positional value "${valueValue}" and --value "${options.value}". Pass only one.`, | |
| EXIT_CODE.USAGE, | |
| ); | |
| } | |
| return { ...options, value: valueValue }; | |
| } | |
| if (nestedSetting) { | |
| if (options.value !== undefined && valueValue !== undefined) { | |
| throw new PmCliError( | |
| `Config set ${keyValue} received both positional value "${valueValue}" and --value "${options.value}". Pass only one.`, | |
| EXIT_CODE.USAGE, | |
| ); | |
| } | |
| return { ...options, value: valueValue ?? options.value }; | |
| } |
| const parsed = Number(trimmed); | ||
| if (!Number.isFinite(parsed)) { | ||
| return { | ||
| ok: false, | ||
| error: { message: `Config set ${descriptor.key} requires a finite number, got "${rawValue}"` }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
In JavaScript, Number("") evaluates to 0. If a user attempts to set a numeric, integer, or ratio setting to an empty string (e.g., pm config project set search_embedding_batch_size ""), it will be parsed as 0 and accepted as a valid value instead of being rejected. We should explicitly check for empty strings before parsing to prevent this behavior.
if (trimmed === "") {
return {
ok: false,
error: { message: `Config set ${descriptor.key} requires a valid number, got empty string` },
};
}
const parsed = Number(trimmed);
if (!Number.isFinite(parsed)) {
return {
ok: false,
error: { message: `Config set ${descriptor.key} requires a finite number, got "${rawValue}"` },
};
}| if (value !== null && typeof value === "object") { | ||
| if (seen.has(value as object)) { | ||
| return value; | ||
| } | ||
| seen.add(value as object); | ||
| const source = value as Record<string, unknown>; | ||
| const result: Record<string, unknown> = {}; | ||
| for (const [key, entry] of Object.entries(source)) { | ||
| result[key] = decodeValue(entry, seen); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
The decodeValue helper traverses any object where typeof value === "object". However, if a non-plain object (such as a Date, RegExp, or other class instance) is passed, Object.entries(source) will be called on it, converting it into a plain object and stripping its prototype and methods. We should add a guard to only traverse plain objects.
if (value !== null && typeof value === "object") {
const proto = Object.getPrototypeOf(value);
if (proto !== Object.prototype && proto !== null) {
return value;
}
if (seen.has(value as object)) {
return value;
}
seen.add(value as object);
const source = value as Record<string, unknown>;
const result: Record<string, unknown> = {};
for (const [key, entry] of Object.entries(source)) {
result[key] = decodeValue(entry, seen);
}
return result;
}| export function collectStaleVectorizationIds<T extends ItemWithUpdatedAt>( | ||
| items: readonly T[], | ||
| ledgerEntries: Readonly<Record<string, string>>, | ||
| ): string[] { | ||
| return items | ||
| .filter((item) => { | ||
| const trackedUpdatedAt = ledgerEntries[item.id]; | ||
| return trackedUpdatedAt !== item.updated_at; | ||
| }) | ||
| .map((item) => item.id) | ||
| .sort((left, right) => left.localeCompare(right)); | ||
| } |
There was a problem hiding this comment.
If the vectorization status ledger file is corrupted, empty, or missing entries at runtime, ledgerEntries could be null or undefined. Accessing ledgerEntries[item.id] would then throw a TypeError. We should defensively default ledgerEntries to an empty object to prevent runtime crashes.
| export function collectStaleVectorizationIds<T extends ItemWithUpdatedAt>( | |
| items: readonly T[], | |
| ledgerEntries: Readonly<Record<string, string>>, | |
| ): string[] { | |
| return items | |
| .filter((item) => { | |
| const trackedUpdatedAt = ledgerEntries[item.id]; | |
| return trackedUpdatedAt !== item.updated_at; | |
| }) | |
| .map((item) => item.id) | |
| .sort((left, right) => left.localeCompare(right)); | |
| } | |
| export function collectStaleVectorizationIds<T extends ItemWithUpdatedAt>( | |
| items: readonly T[], | |
| ledgerEntries: Readonly<Record<string, string>> | undefined | null, | |
| ): string[] { | |
| const entries = ledgerEntries ?? {}; | |
| return items | |
| .filter((item) => { | |
| const trackedUpdatedAt = entries[item.id]; | |
| return trackedUpdatedAt !== item.updated_at; | |
| }) | |
| .map((item) => item.id) | |
| .sort((left, right) => left.localeCompare(right)); | |
| } |
![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:
- The MCP HTML entity decoding currently only runs when the string contains
<or>, which means any arguments that have only&,", or'will be left encoded; consider either relaxing the guard or documenting that angle-brackets are required as the activation signal so upstream changes don’t silently bypass the decoder. - In
maybeEmitVectorIndexStaleWarningyou mutate the passed-inwarningsarray while also writing directly toprocess.stderr; you might want to align this with the existing error/warning plumbing (e.g., use the same helper or return a warning string) to keep all user-visible warnings going through a single abstraction.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The MCP HTML entity decoding currently only runs when the string contains `<` or `>`, which means any arguments that have only `&`, `"`, or `'` will be left encoded; consider either relaxing the guard or documenting that angle-brackets are required as the activation signal so upstream changes don’t silently bypass the decoder.
- In `maybeEmitVectorIndexStaleWarning` you mutate the passed-in `warnings` array while also writing directly to `process.stderr`; you might want to align this with the existing error/warning plumbing (e.g., use the same helper or return a warning string) to keep all user-visible warnings going through a single abstraction.
## Individual Comments
### Comment 1
<location path="src/core/config/nested-settings.ts" line_range="196" />
<code_context>
+ return { ok: true, parsed: { descriptor, value: trimmed } };
+ }
+
+ const parsed = Number(trimmed);
+ if (!Number.isFinite(parsed)) {
+ return {
</code_context>
<issue_to_address>
**issue (bug_risk):** Numeric nested settings currently treat an empty string as `0`, which is likely an invalid/accidental value.
In `parseNestedSettingValue`, `Number(trimmed)` turns `""` (or all-whitespace input) into `0`, so `pm config set search_max_results " "` would be stored as `0` without any warning. For `integer`/`number`/`ratio` kinds, consider treating empty/whitespace-only input as invalid and returning a parse error, consistent with the existing non-finite guard, to avoid accidental misconfigurations.
</issue_to_address>
### Comment 2
<location path="CHANGELOG.md" line_range="17" />
<code_context>
+### Fixed
+
+- pm comments/notes/learnings --add HTML-escapes angle brackets in stored text ([pm-ydkl](https://github.com/unbraind/pm-cli/blob/main/.agents/pm/issues/pm-ydkl.toon))
+- MCP pm\_create/pm\_update crashed with 'raw.trim is not a function' when priority sent as JSON number ([pm-9r7z](https://github.com/unbraind/pm-cli/blob/main/.agents/pm/issues/pm-9r7z.toon))
+
## 2026.5.28 - 2026-05-28
</code_context>
<issue_to_address>
**suggestion (typo):** Minor grammar tweak: add `was` in `when priority sent as JSON number`.
Consider changing this to `when priority was sent as a JSON number` to keep the tense consistent.
```suggestion
- MCP pm\_create/pm\_update crashed with 'raw.trim is not a function' when priority was sent as a JSON number ([pm-9r7z](https://github.com/unbraind/pm-cli/blob/main/.agents/pm/issues/pm-9r7z.toon))
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces several code-quality refactors, features, and bug fixes, including model-agnostic search configuration, calendar TOON output rendering, a shared comma-separated list parser, and defensive HTML-entity decoding at the MCP server boundary. The review comments correctly identify critical issues in the newly added files: (1) a logic bug in applyPositionalValue that erroneously throws an error when --value is used without a positional argument, (2) a restrictive guard in decodeHtmlEntitiesIfEscaped that prevents decoding other core HTML entities when angle brackets are absent, and (3) a parsing edge case where empty or whitespace-only strings are silently coerced to 0 by Number(). All three comments provide valid, actionable feedback and should be kept.
| if (nestedSetting) { | ||
| if (options.value !== undefined && options.value !== valueValue) { | ||
| throw new PmCliError( | ||
| `Config set ${keyValue} received both positional value "${valueValue}" and --value "${options.value}". Pass only one.`, | ||
| EXIT_CODE.USAGE, | ||
| ); | ||
| } | ||
| return { ...options, value: valueValue }; | ||
| } |
There was a problem hiding this comment.
This check is broken when the user specifies --value <value> without a positional value. In that case, valueValue is undefined, so options.value !== valueValue evaluates to true (e.g., "ollama" !== undefined), causing the command to throw a PmCliError erroneously.
To fix this, we should only throw an error if both options.value and valueValue are defined (i.e., not undefined), and otherwise use whichever one is provided.
| if (nestedSetting) { | |
| if (options.value !== undefined && options.value !== valueValue) { | |
| throw new PmCliError( | |
| `Config set ${keyValue} received both positional value "${valueValue}" and --value "${options.value}". Pass only one.`, | |
| EXIT_CODE.USAGE, | |
| ); | |
| } | |
| return { ...options, value: valueValue }; | |
| } | |
| if (nestedSetting) { | |
| if (options.value !== undefined && valueValue !== undefined) { | |
| throw new PmCliError( | |
| `Config set ${keyValue} received both positional value "${valueValue}" and --value "${options.value}". Pass only one.`, | |
| EXIT_CODE.USAGE, | |
| ); | |
| } | |
| return { ...options, value: valueValue !== undefined ? valueValue : options.value }; | |
| } |
| if (!input.includes("<") && !input.includes(">")) { | ||
| return input; | ||
| } | ||
| // The regex only matches keys present in ENTITY_MAP, so the lookup is total. | ||
| return input.replace(ENTITY_PATTERN, (match) => ENTITY_MAP[match] as string); |
There was a problem hiding this comment.
The guard !input.includes("<") && !input.includes(">") prevents decoding of other HTML entities (like &, ", and ') when they appear in a string without angle brackets. For example, if an agent sends a title or comment containing & (which gets encoded to &), it will remain encoded in the database because the string lacks < or >.
Since the single-pass regex replacement using ENTITY_PATTERN already handles double-encoding safely (e.g., &lt; correctly decodes to < and no further), this guard is unnecessary and introduces bugs for strings containing other HTML-escaped characters. Consider removing this guard to ensure all core HTML entities are correctly decoded.
return input.replace(ENTITY_PATTERN, (match) => ENTITY_MAP[match] as string);| const parsed = Number(trimmed); | ||
| if (!Number.isFinite(parsed)) { | ||
| return { | ||
| ok: false, | ||
| error: { message: `Config set ${descriptor.key} requires a finite number, got "${rawValue}"` }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
In JavaScript, Number("") and Number(" ") evaluate to 0. This means that if a user passes an empty or whitespace-only string for a numeric, integer, or ratio setting (e.g., pm config project set search_hybrid_semantic_weight ""), it will be silently parsed and saved as 0 instead of being rejected as an invalid number.
We should explicitly check if trimmed is empty before attempting to parse it as a number.
if (trimmed === "") {
return {
ok: false,
error: { message: `Config set ${descriptor.key} requires a valid number, got empty string` },
};
}
const parsed = Number(trimmed);
if (!Number.isFinite(parsed)) {
return {
ok: false,
error: { message: `Config set ${descriptor.key} requires a finite number, got "${rawValue}"` },
};
}
There was a problem hiding this comment.
Code Review
This pull request introduces several code-quality refactors and features, including a new splitCommaList helper to consolidate split/trim/dedup logic, 16 nested-setting CLI aliases for search and vector-store configurations, a query-time index staleness warning, a TOON renderer for calendar outputs, and defensive HTML-entity decoding at the MCP server boundary. The review comments identify three critical issues: a CLI crash when using the --value flag without a positional argument in config.ts, silent coercion of empty strings to 0 for numeric settings in nested-settings.ts, and prototype corruption of non-plain objects during HTML-entity decoding in html-entity-decode.ts.
| if (nestedSetting) { | ||
| if (options.value !== undefined && options.value !== valueValue) { | ||
| throw new PmCliError( | ||
| `Config set ${keyValue} received both positional value "${valueValue}" and --value "${options.value}". Pass only one.`, | ||
| EXIT_CODE.USAGE, | ||
| ); | ||
| } | ||
| return { ...options, value: valueValue }; | ||
| } |
There was a problem hiding this comment.
Using the --value flag without a positional value will crash the CLI. This is because options.value !== valueValue evaluates to true when valueValue is undefined (no positional value provided) and options.value is defined (via --value). Additionally, returning valueValue directly would overwrite the --value option with undefined.
We should only throw an error if both a positional value and --value are provided, and correctly fall back to options.value when valueValue is undefined.
| if (nestedSetting) { | |
| if (options.value !== undefined && options.value !== valueValue) { | |
| throw new PmCliError( | |
| `Config set ${keyValue} received both positional value "${valueValue}" and --value "${options.value}". Pass only one.`, | |
| EXIT_CODE.USAGE, | |
| ); | |
| } | |
| return { ...options, value: valueValue }; | |
| } | |
| if (nestedSetting) { | |
| if (options.value !== undefined && valueValue !== undefined) { | |
| throw new PmCliError( | |
| `Config set ${keyValue} received both positional value "${valueValue}" and --value "${options.value}". Pass only one.`, | |
| EXIT_CODE.USAGE, | |
| ); | |
| } | |
| return { ...options, value: valueValue ?? options.value }; | |
| } |
| const parsed = Number(trimmed); | ||
| if (!Number.isFinite(parsed)) { | ||
| return { | ||
| ok: false, | ||
| error: { message: `Config set ${descriptor.key} requires a finite number, got "${rawValue}"` }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
An empty or whitespace-only string passed to a numeric setting (such as integer, ratio, or number) will be silently coerced to 0 because Number("") evaluates to 0 in JavaScript. This can lead to unexpected configuration states instead of a validation failure.
We should explicitly reject empty strings for numeric settings.
if (trimmed === "") {
return {
ok: false,
error: { message: `Config set ${descriptor.key} requires a valid number, got empty string` },
};
}
const parsed = Number(trimmed);
if (!Number.isFinite(parsed)) {
return {
ok: false,
error: { message: `Config set ${descriptor.key} requires a finite number, got "${rawValue}"` },
};
}| if (value !== null && typeof value === "object") { | ||
| if (seen.has(value as object)) { | ||
| return value; | ||
| } | ||
| seen.add(value as object); | ||
| const source = value as Record<string, unknown>; | ||
| const result: Record<string, unknown> = {}; | ||
| for (const [key, entry] of Object.entries(source)) { | ||
| result[key] = decodeValue(entry, seen); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
The decodeValue function currently converts any non-null object (including non-plain objects like Date, RegExp, or custom class instances) into a plain object, which corrupts their prototype and internal slots. This violates the documented behavior: "Non-string scalars (numbers, booleans, null, undefined) and non-plain values pass through untouched."
We should check if the object is a plain object before attempting to recursively decode its properties.
if (value !== null && typeof value === "object") {
const proto = Object.getPrototypeOf(value);
if (proto !== Object.prototype && proto !== null) {
return value;
}
if (seen.has(value as object)) {
return value;
}
seen.add(value as object);
const source = value as Record<string, unknown>;
const result: Record<string, unknown> = {};
for (const [key, entry] of Object.entries(source)) {
result[key] = decodeValue(entry, seen);
}
return result;
}![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: b0b8575ecd
ℹ️ 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".
| key: descriptor.key, | ||
| path: descriptor.path, | ||
| kind: descriptor.kind, | ||
| value: readNestedSettingValue(settings, descriptor), |
There was a problem hiding this comment.
When a project has openai_api_key or qdrant_api_key configured, this adds those nested leaves to the normal pm config list result and returns the raw value, so both default output and --json will print API keys during a discoverability command. Please redact secret descriptors here (or omit their value) and keep full secret export limited to explicit settings export/get flows.
Useful? React with 👍 / 👎.
| }; | ||
| } | ||
| if (descriptor.kind === "integer") { | ||
| if (!Number.isInteger(parsed) || parsed < 0) { |
There was a problem hiding this comment.
For nested integer keys such as search_max_results, search_embedding_batch_size, and search_embedding_timeout_ms, accepting 0 reports a successful config change even though the runtime readers require a positive value and silently fall back (resolveSearchMaxResults uses 50, embedding batching uses 1/30000ms). In the scenario where a user runs pm config project set search_max_results 0 or passes an empty numeric value that coerces to 0, the persisted setting does not match the behavior they just configured; these descriptors need positive-integer validation rather than non-negative validation.
Useful? React with 👍 / 👎.
| summary: "Search embedding provider name (e.g. openai, ollama, or an extension provider).", | ||
| }, | ||
| { | ||
| key: "search_embedding_model", | ||
| path: "search.embedding_model", |
There was a problem hiding this comment.
These new config keys are advertised as controlling the active provider/model, but the built-in runtime ignores them: resolveEmbeddingProviders still picks the first configured built-in provider (openai before ollama) and reads only providers.*.model, while resolveVectorStores similarly picks qdrant before lancedb. In a project with both providers/stores configured, pm config project set search_provider ollama or search_embedding_model foo succeeds but reindex/search continue using the old built-in choice/model, so either the resolvers need to honor these selectors or the aliases should not claim they do.
Useful? React with 👍 / 👎.
Sourcery + Gemini code-assist findings:
1. **HIGH (gemini): src/cli/commands/config.ts:929** — `--value` handling was
broken: passing `--value foo` without a positional value triggered a
spurious "received both" error AND overwrote options.value with undefined.
Tightened the conflict check to require BOTH be present-and-different, and
pick whichever was supplied via `valueValue ?? options.value`.
2. **MEDIUM (gemini + sourcery): src/core/config/nested-settings.ts:202** —
`Number("")` and `Number(" ")` both evaluate to `0` and slipped past the
Number.isFinite guard, so `pm config project set search_max_results ""`
silently stored 0. Added an explicit empty-after-trim guard for numeric /
integer / ratio kinds returning a "requires a non-empty value" error.
3. **MEDIUM (gemini): src/core/shared/html-entity-decode.ts:88** — the walker
used `typeof value === "object"` and rebuilt every object as a fresh plain
record, stripping the prototype from Date / RegExp / Map / Set / etc.
Added `isPlainObject` (proto === Object.prototype || proto === null) so
class instances pass through untouched, and Object.create(null) objects
still traverse.
4. **MEDIUM (gemini): src/core/search/staleness.ts** — accessing
`ledgerEntries[item.id]` on a null / undefined ledger threw TypeError if
the ledger file was corrupted or partially written. Widened the parameter
to `Record<string, string> | null | undefined` and treat missing entries as
"every item stale" (same semantics as an empty ledger).
5. **LOW (sourcery): activation-signal documentation** — kept the narrow
`<`/`>` trigger (intentional design: only widen if upstream encoding
policy changes) but expanded the inline comment explaining the rationale
and the lockstep contract.
6. **TYPO (sourcery): CHANGELOG.md:17** — "when priority sent as JSON number"
→ "when priority was sent as a JSON number". Updated the pm-9r7z item title
(the CHANGELOG entry is auto-generated from it) and re-ran
`pnpm changelog:pm`.
New regression tests:
- 1 empty-string-numeric for parseNestedSettingValue (3 kinds × 3 inputs)
- 1 class-instance preservation + null-proto traversal for decodeHtmlEntitiesInOptions
- 1 null/undefined ledger tolerance for collectStaleVectorizationIds
- 3 --value flag conflict/no-positional/equal-positional for runConfig
All 1940 tests green at 100% line/branch/function/statement coverage.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Addressed review findings in f2da1af:
Added 6 new regression tests. 1940/1940 pass at 100% coverage. @codex review |
|
Thanks for addressing those points, @unbraind. The fixes for the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/pm/chores/pm-1b96.toon:
- Line 5: This change directly edits the PM system-of-record file
.agents/pm/chores/pm-1b96.toon by modifying the "status" field; revert that edit
(remove the commit or restore the original file content) and ensure no other
edits under .agents/pm/** remain (including the ranges referenced: 9-9 and
12-15); instead, make the intended update through the PM system-of-record (e.g.,
the PM UI or the canonical workflow) or open a ticket/PR against that system so
the maintainers can apply the change, then update this PR to exclude any direct
modifications to .agents/pm files.
In @.agents/pm/history/pm-xzrx.jsonl:
- Around line 2-3: The review flags direct edits to the agent tracker JSON (you
modified metadata fields like metadata/updated_at, metadata/resolution,
metadata/expected_result, metadata/actual_result, metadata/status and
metadata/close_reason) under the pm agent store; revert those hand-edits and
instead update the tracker via the pm workflow that owns this state (use the pm
CLI or the tracker API to create the resolution/close entries and to mark the
item closed), ensuring the tracker record is created/updated through the pm
service rather than committing changes to the .agents/pm history file directly.
In `@docs/CONFIGURATION.md`:
- Line 160: Replace the credential-like example value in the CLI example "pm
config project set openai_api_key sk-..." with a neutral placeholder to avoid
exposing secret formats; update the example to use a non-credential token such
as "<OPENAI_API_KEY>" (or "YOUR_OPENAI_API_KEY") in the "pm config project set
openai_api_key" command so docs/CONFIGURATION.md contains no credential-shaped
values.
In `@src/cli/commands/search.ts`:
- Around line 1284-1290: The stale-index warning is currently emitted before
runtime path resolution and so can appear even when an extension or fallback
keyword path will actually handle the query; move or gate the call to
maybeEmitVectorIndexStaleWarning so it only runs after the runtime semantic path
is resolved and confirmed to be the built-in semantic provider. Concretely: in
the search flow that checks modeWasExplicit, resolve the semantic runtime
path/provider (the same logic that decides extension vs built-in) and only call
maybeEmitVectorIndexStaleWarning(pmRoot, filteredDocuments, warnings) when that
resolved provider is the built-in semantic implementation (e.g., provider.id ===
'builtin' or provider.isBuiltin === true); otherwise skip emitting the warning.
In `@src/core/config/nested-settings.ts`:
- Around line 191-223: In parseNestedSettingValue, stop treating
empty/whitespace as 0 by rejecting trimmed === "" for non-"string" kinds: add an
early check after trimmed is computed that returns ok: false with a clear error
message (include descriptor.key) when trimmed === "" for kinds "integer" |
"ratio" | "number"; keep the existing numeric parsing/validation logic
(Number(trimmed), finite check, integer/non-negative check for "integer", [0,1]
check for "ratio") and ensure the "number" kind explicitly documents/retains
that negatives are allowed to match existing tests (or update the inline comment
to state negatives are permitted).
In `@src/core/shared/html-entity-decode.ts`:
- Around line 77-87: The object rebuild in decodeValue is vulnerable to
"__proto__" prototype pollution because it creates result = {} and assigns
result[key] = ...; change reconstruction to use a safe assignment path: create
result with Object.create(null) and for each entry call
Object.defineProperty(result, key, { value: decodeValue(entry, seen),
enumerable: true, configurable: true, writable: true }) so own properties
(including "__proto__") are defined as plain data properties and cannot mutate
the prototype; update the loop in decodeValue that iterates over
source/seen/result accordingly.
🪄 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: 63fe6616-bf0f-4e82-98ac-75754c3df7b0
📒 Files selected for processing (47)
.agents/pm/chores/pm-1b96.toon.agents/pm/extensions/.managed-extensions.json.agents/pm/features/pm-7ilo.toon.agents/pm/features/pm-xzrx.toon.agents/pm/history/pm-1b96.jsonl.agents/pm/history/pm-1lws.jsonl.agents/pm/history/pm-7ilo.jsonl.agents/pm/history/pm-9r7z.jsonl.agents/pm/history/pm-xzrx.jsonl.agents/pm/history/pm-ydkl.jsonl.agents/pm/issues/pm-1lws.toon.agents/pm/issues/pm-9r7z.toon.agents/pm/issues/pm-ydkl.toonCHANGELOG.mddocs/CONFIGURATION.mdpackages/pm-calendar/extensions/calendar/runtime.jspackages/pm-calendar/extensions/calendar/runtime.tssrc/cli/commands/aggregate.tssrc/cli/commands/calendar.tssrc/cli/commands/config.tssrc/cli/commands/create.tssrc/cli/commands/extension/bundled-catalog.tssrc/cli/commands/health.tssrc/cli/commands/linked-test-parsers.tssrc/cli/commands/plan.tssrc/cli/commands/search.tssrc/cli/commands/update-many.tssrc/cli/commands/update.tssrc/cli/help-json-payload.tssrc/cli/register-setup.tssrc/cli/registration-helpers.tssrc/core/config/nested-settings.tssrc/core/item/priority.tssrc/core/search/staleness.tssrc/core/shared/html-entity-decode.tssrc/core/shared/split-comma-list.tssrc/mcp/server.tssrc/sdk/runtime.tstests/unit/builtin-extension-entrypoints.spec.tstests/unit/calendar-toon-renderer.spec.tstests/unit/config-key-aliases-search.spec.tstests/unit/html-entity-decode.spec.tstests/unit/nested-settings.spec.tstests/unit/priority.spec.tstests/unit/search-staleness-warning.spec.tstests/unit/split-comma-list.spec.tsvitest.config.ts
| description: "From code-quality audit (pm-jvbt). (1) runUpdate() ~834 lines (update.ts:1102) and runCreate() ~670 lines (create.ts:670) are single mega-functions mixing parse/validate/mutate/output — split into staged sub-steps. (2) cli-contracts.ts is a 1805-LOC monolith of 62 inline exports despite an existing cli-contracts/ subdir — move groups into it. (3) Dedup: shared parseSingleEvent for create.parseEvents vs update.parseEventEntries; splitCommaList() helper for the 7+ hand-rolled [...new Set(split(',').map(trim).filter(len>0))] idioms; toItemMetadata() inverse helper for 9x 'as unknown as ItemMetadata'. (4) Drop unused exports in help-json-payload.ts (9 internal-only) and registration-helpers.ts. (5) Resolve/remove misleading @deprecated on ItemFrontMatter (still used 60+ sites) and front_matter_key." | ||
| type: Chore | ||
| status: open | ||
| status: closed |
There was a problem hiding this comment.
Do not modify .agents/pm records directly in PR content.
These updates are direct edits under .agents/pm/**, which violates the repository rule that PM is the system of record and these files must not be edited directly.
As per coding guidelines, .agents/pm/**: pm is the system of record. Do not edit .agents/pm files directly.
Also applies to: 9-9, 12-15
🤖 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-1b96.toon at line 5, This change directly edits the PM
system-of-record file .agents/pm/chores/pm-1b96.toon by modifying the "status"
field; revert that edit (remove the commit or restore the original file content)
and ensure no other edits under .agents/pm/** remain (including the ranges
referenced: 9-9 and 12-15); instead, make the intended update through the PM
system-of-record (e.g., the PM UI or the canonical workflow) or open a ticket/PR
against that system so the maintainers can apply the change, then update this PR
to exclude any direct modifications to .agents/pm files.
| {"ts":"2026-05-28T10:49:34.814Z","author":"maintainer-agent","op":"update","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-05-28T10:49:34.814Z"},{"op":"add","path":"/metadata/resolution","value":"Shipped 2 of 3 listed gaps: (A-2) Added a renderCalendarToon() that runs the JSON-serializable CalendarResult through @toon-format/toon. Wired into the calendar package extension (runtime.ts + hand-maintained runtime.js kept in sync). 'pm calendar --format toon' now emits real TOON instead of falling through to markdown. (A-2) Added a warning at create-time for Milestone/Meeting/Reminder/Event items that have NO deadline, NO reminders, and NO events — the warning includes the just-allocated id and a copy-pasteable pm update suggestion. NOT shipped (separate followups): (A-1) IANA timezone validation + UTC date math (large scope), (A-3) ICS/iCalendar export."},{"op":"add","path":"/metadata/expected_result","value":"pm calendar --format toon produces parseable TOON output (decodable by @toon-format/toon); calendar-relevant items created without any date field warn to stderr instead of silently never appearing."},{"op":"add","path":"/metadata/actual_result","value":"Verified end-to-end in /tmp/pm-e2e-2026-05-28 after pm install calendar: --format toon emits compact TOON; --format markdown still emits H1 + bullet section; --format json still emits JSON. Schedule-less Meeting create emits the correct warning. 2 new tests for the TOON renderer pass; existing 15 calendar + 52 create tests unchanged."}],"before_hash":"bd8f350e454ecaa5204d1da32ca513a3e8256a83d3378e62042c451c3f27fdab","after_hash":"a5514cd496dbcf502b1fe734bc4ec416274715a63a2af1fccfdbbffd4adc597e"} | ||
| {"ts":"2026-05-28T10:54:59.689Z","author":"maintainer-agent","op":"close","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-05-28T10:54:59.689Z"},{"op":"replace","path":"/metadata/status","value":"closed"},{"op":"add","path":"/metadata/close_reason","value":"Shipped calendar TOON renderer + schedule-less Milestone/Meeting/Reminder/Event warning. (A-1) IANA timezone and (A-3) ICS export deferred to followup work."}],"before_hash":"a5514cd496dbcf502b1fe734bc4ec416274715a63a2af1fccfdbbffd4adc597e","after_hash":"1bdede11a580d2583f6a373ee76f4911f151f6be8973ea0718d4203cddcea23b"} |
There was a problem hiding this comment.
Do not directly edit .agents/pm history files.
These are direct mutations under .agents/pm/**, which violates the repo process contract. Please apply these changes through the pm workflow that owns the tracker state instead of hand-editing tracked records.
As per coding guidelines, ".agents/pm/**: pm is the system of record. Do not edit .agents/pm files directly."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/pm/history/pm-xzrx.jsonl around lines 2 - 3, The review flags direct
edits to the agent tracker JSON (you modified metadata fields like
metadata/updated_at, metadata/resolution, metadata/expected_result,
metadata/actual_result, metadata/status and metadata/close_reason) under the pm
agent store; revert those hand-edits and instead update the tracker via the pm
workflow that owns this state (use the pm CLI or the tracker API to create the
resolution/close entries and to mark the item closed), ensuring the tracker
record is created/updated through the pm service rather than committing changes
to the .agents/pm history file directly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2da1af3df
ℹ️ 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".
| key: "search_provider", | ||
| path: "search.provider", | ||
| kind: "string", | ||
| summary: "Search embedding provider name (e.g. openai, ollama, or an extension provider).", |
There was a problem hiding this comment.
When a project already has OpenAI settings and then adds Ollama settings, pm config project set search_provider ollama writes search.provider but built-in selection still ignores it: resolveEmbeddingProviders() returns available[0], with OpenAI ordered before Ollama. In that scenario both pm reindex --mode hybrid and semantic/hybrid search continue using OpenAI even though the newly exposed key and docs say ollama selects the provider; either the resolver needs to prefer settings.search.provider for built-ins or this alias should not advertise built-in selection.
Useful? React with 👍 / 👎.
| key: "vector_store_adapter", | ||
| path: "vector_store.adapter", | ||
| kind: "string", | ||
| summary: "Vector store adapter name (lancedb, qdrant, or an extension adapter).", |
There was a problem hiding this comment.
When both Qdrant and LanceDB settings are present, setting vector_store_adapter to lancedb does not actually select LanceDB: resolveVectorStores() still returns available[0], with Qdrant ordered first. This makes the new config alias silently ineffective for switching stores in an already-configured project, so reindex/search can keep querying/upserting Qdrant despite the user setting vector_store.adapter to lancedb.
Useful? React with 👍 / 👎.
| key: "search_embedding_model", | ||
| path: "search.embedding_model", | ||
| kind: "string", | ||
| summary: "Default embedding model name (overrides provider-specific model when set).", |
There was a problem hiding this comment.
The new alias promises that search_embedding_model overrides provider-specific models, but the built-in OpenAI/Ollama path never reads settings.search.embedding_model: provider resolution still requires and uses providers.openai.model or providers.ollama.model, while only extension embeddings receive settings.search.embedding_model. In a project with openai_model already configured, pm config project set search_embedding_model new-model reports success but pm reindex --mode hybrid continues embedding with the old provider-specific model, making the exposed setting misleading unless the built-in provider config is derived from it.
Useful? React with 👍 / 👎.
| if (descriptor.kind === "integer") { | ||
| if (!Number.isInteger(parsed) || parsed < 0) { | ||
| return { | ||
| ok: false, | ||
| error: { message: `Config set ${descriptor.key} requires a non-negative integer, got "${rawValue}"` }, | ||
| }; | ||
| } | ||
| return { ok: true, parsed: { descriptor, value: parsed } }; |
There was a problem hiding this comment.
For the newly exposed integer aliases, accepting 0 writes values that the runtime immediately ignores: resolveSearchMaxResults() falls back unless max_results > 0, and embedding batch/timeout also fall back unless their values are > 0. As a result, commands like pm config project set search_max_results 0 or search_embedding_timeout_ms 0 report success and persist a value, but search/reindex silently use defaults instead; these aliases should reject zero (or otherwise align with the runtime semantics).
Useful? React with 👍 / 👎.
CodeRabbit + Codex follow-up after f2da1af: **Real bugs (alias contracts did not match runtime):** - **`search_provider` was not honored**: `resolveEmbeddingProviders` always picked the first available (openai > ollama) regardless of `settings.search.provider`. Fixed — when both built-ins are configured and a preferred name is set, the named one wins; otherwise the previous tie-break behavior is preserved. - **`vector_store_adapter` was not honored**: same pattern in `resolveVectorStores` (qdrant > lancedb regardless of adapter setting). Fixed the same way. - **`search_embedding_model` was advertised as an override but ignored**: the built-in `resolveOpenAiProvider`/`resolveOllamaProvider` always returned the provider-specific `model`. Added `resolveSearchEmbeddingModelOverride` that both built-ins consult after reading their own model, so the override applies to whichever built-in wins. - **Reject `0` for positive-integer settings**: `resolveSearchMaxResults` / embedding batch size / embedding timeout all silently fall back to the default when `0` is supplied, making `pm config set search_max_results 0` a misleading no-op. Added an optional `min` field on the descriptor table (`min: 1` for `search_embedding_batch_size`, `search_embedding_timeout_ms`, `search_max_results`) and a matching validation branch. **Security/hardening:** - **`__proto__` injection in `decodeHtmlEntitiesInOptions`**: an MCP caller could smuggle `__proto__` / `constructor` / `prototype` keys through the walker, which then `result[key] = ...` would have triggered prototype pollution. Switched the rebuilt record to `Object.create(null)` and skip those three dangerous keys explicitly. New regression test confirms Object.prototype stays unpolluted. **UX:** - **Gate vector-index-stale warning to the built-in semantic path**: when an extension search provider is registered, our local ledger is not what drives its freshness, so emitting the warning would be noise. Now only fires when explicit-mode AND no extension provider AND a built-in provider+store pair is configured. - **Replace `sk-...` placeholder in docs** with `<OPENAI_API_KEY>` to avoid any credential-shape confusion in public docs. - **Clarify `number` kind allows negatives** in the inline comment (matches the existing test for `score_threshold = -0.25`). **Tests:** - `tests/unit/search-provider-store-selection.spec.ts` (new) — 9 tests for `resolveEmbeddingProviders` + `resolveVectorStores` honoring preferences with both-configured / preferred-extension / model-override scenarios. - Extended `html-entity-decode.spec.ts` with `__proto__` pollution test. - Extended `nested-settings.spec.ts` with `min:1` rejection of `0` plus a synthetic `min` constraint for `number` kind. All 1947 tests green at 100% line/branch/function/statement coverage. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Second round of fixes in 7b9616b: Real alias-contract bugs (codex):
Security (coderabbit): UX (coderabbit): vector-index-stale warning now gated to built-in semantic path only (extension providers have their own indexing lifecycle); Validation (codex): 9 new tests covering the alias-honor + prototype-pollution + zero-reject behaviors. 1947/1947 pass at 100% coverage. @codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces several code-quality refactors and features, including a shared splitCommaList helper, 16 nested-setting CLI aliases for search and vector-store configurations, a query-time index staleness warning, and a calendar TOON renderer. It also adds schedule-less warnings for calendar-relevant items, supports native numbers for priority resolution to prevent MCP crashes, and implements defensive HTML-entity decoding at the MCP server boundary. Feedback was provided on html-entity-decode.ts to preserve the original prototype of plain objects during decoding instead of using Object.create(null), which can strip standard object methods and cause downstream runtime errors.
| // Use a null-prototype record + skip the dangerous keys to defend against | ||
| // prototype pollution if an MCP caller smuggles `__proto__` / `constructor` | ||
| // / `prototype` as own keys. | ||
| const result: Record<string, unknown> = Object.create(null); |
There was a problem hiding this comment.
Creating the cloned object with Object.create(null) strips the prototype chain from standard plain objects (i.e., those with Object.prototype). This will cause runtime errors (such as TypeError: result.hasOwnProperty is not a function or issues with toString()) in downstream code that expects standard object methods. Since you are already explicitly skipping dangerous keys (__proto__, constructor, prototype), you can safely preserve the original prototype by using Object.create(Object.getPrototypeOf(value)) instead.
| const result: Record<string, unknown> = Object.create(null); | |
| const result: Record<string, unknown> = Object.create(Object.getPrototypeOf(value)); |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/search/providers.ts`:
- Around line 189-194: The current lookup uses a case-sensitive compare when
resolving settings.search.provider to an entry (see preferredName and
available.find), so values like "Ollama" won't match "ollama" and may fall back
incorrectly; update the resolution to normalize both sides to the same case
(e.g., convert preferredName toLowerCase() and compare against
entry.name.toLowerCase()) and only attempt the case-insensitive match when
preferredName is a non-empty string, preserving the existing fallback behavior
if no match is found.
In `@src/core/search/vector-stores.ts`:
- Around line 481-486: preferred adapter matching is currently case-sensitive:
after computing preferredName from
toNonEmptyString(settings.vector_store?.adapter) the lookup that sets preferred
uses exact equality against available entries and will miss values like
"LanceDB" vs "lancedb"; normalize the preference and entry names (e.g., trim and
toLowerCase) before comparing so the find uses a case-insensitive match (update
the preferredName computation and the available.find predicate that sets
preferred to compare normalized strings from preferredName and entry.name).
🪄 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: b2d25156-ba4f-40e7-aea0-0474bd1dcc93
📒 Files selected for processing (17)
.agents/pm/extensions/.managed-extensions.json.agents/pm/history/pm-9r7z.jsonl.agents/pm/issues/pm-9r7z.toonCHANGELOG.mddocs/CONFIGURATION.mdsrc/cli/commands/config.tssrc/cli/commands/search.tssrc/core/config/nested-settings.tssrc/core/search/providers.tssrc/core/search/staleness.tssrc/core/search/vector-stores.tssrc/core/shared/html-entity-decode.tstests/unit/config-key-aliases-search.spec.tstests/unit/html-entity-decode.spec.tstests/unit/nested-settings.spec.tstests/unit/search-provider-store-selection.spec.tstests/unit/search-staleness-warning.spec.ts
✅ Files skipped from review due to trivial changes (4)
- .agents/pm/extensions/.managed-extensions.json
- .agents/pm/issues/pm-9r7z.toon
- CHANGELOG.md
- .agents/pm/history/pm-9r7z.jsonl
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/CONFIGURATION.md
- src/cli/commands/search.ts
- src/core/search/staleness.ts
- tests/unit/search-staleness-warning.spec.ts
- src/core/shared/html-entity-decode.ts
- src/cli/commands/config.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b9616b72b
ℹ️ 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".
| # or, with Qdrant: | ||
| pm config project set vector_store_adapter qdrant | ||
| pm config project set qdrant_url http://localhost:6333 | ||
| pm config project set qdrant_api_key <key-if-required> |
There was a problem hiding this comment.
When this bash example is copied as written, <key-if-required> is parsed by the shell as redirection syntax rather than a literal argument, causing a syntax error before pm runs. The OpenAI example above quotes its placeholder; this one should do the same or use a non-angle-bracket placeholder so users can copy the documented command safely.
Useful? React with 👍 / 👎.
| pm config project set openai_api_key '<OPENAI_API_KEY>' | ||
| pm config project set openai_model text-embedding-3-small | ||
| pm config project set search_provider openai |
There was a problem hiding this comment.
On a fresh tracker, providers.openai.base_url defaults to an empty string, and resolveOpenAiProvider returns null unless both base_url and model are non-empty, so following this OpenAI recipe leaves search_provider=openai selected but no built-in provider configured. Add an openai_base_url command (for example https://api.openai.com/v1) so the documented setup actually enables semantic search.
Useful? React with 👍 / 👎.
| if (!baseUrl || !model) { | ||
| return null; | ||
| } | ||
| const override = resolveSearchEmbeddingModelOverride(settings); |
There was a problem hiding this comment.
Because this override is only read after resolveOpenAiProvider has already returned when providers.openai.model is empty, search_embedding_model cannot act as the documented default model for a provider that only has its base URL/API key configured; the same ordering exists in the Ollama resolver. In that configuration pm search --mode semantic still reports no configured provider even though search_embedding_model was set, so resolve the override before the missing-model check and use it as the effective model.
Useful? React with 👍 / 👎.
CodeRabbit + Gemini + Codex follow-up after 7b9616b: 1. **gemini HIGH: prototype stripping in decodeValue** — `Object.create(null)` stripped the prototype chain from STANDARD plain objects, so downstream `result.hasOwnProperty(...)` / `result.toString()` would throw. Restored the original prototype via `Object.create(Object.getPrototypeOf(value))` while keeping the explicit `__proto__` / `constructor` / `prototype` key-skip-list as defense in depth. Updated the regression test to assert (a) no own-property pollution and (b) standard `hasOwnProperty` still works. 2. **coderabbit MAJOR x2: case-sensitive name matching** — `Ollama` / `LanceDB` / `LANCEDB` etc. silently fell through to the alphabetical tie-break instead of selecting the named provider/adapter. Lower-cased `preferredName` before the lookup in both `resolveEmbeddingProviders` and `resolveVectorStores`. Added case-insensitive regression tests. 3. **codex P2: docs/CONFIGURATION.md — add OpenAI base_url to setup recipe** — `providers.openai.base_url` defaults to empty so the canonical `https://api.openai.com/v1` must be set explicitly. Added the missing line and a one-sentence explainer. 4. **codex P3: docs/CONFIGURATION.md — quote Qdrant placeholder** — replaced `<key-if-required>` with `'<QDRANT_API_KEY>' # omit on unauthenticated dev servers` so copy-paste doesn't crash bash on `<`. Tests: 1949 / 1949 pass; 100% coverage across all gated dimensions. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Round 3 fixes in 887d23e:
4 new regression tests. 1949/1949 pass at 100% coverage. @codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements and bug fixes, including a shared splitCommaList helper, defensive HTML-entity decoding at the MCP server boundary, and support for native numbers in priority resolution. It also adds custom search provider configuration, query-time vector index staleness warnings, and a calendar TOON output format. Review feedback highlights opportunities to improve robustness, such as avoiding silent data loss by allowing 'constructor' and 'prototype' keys during object traversal, using nullish coalescing fallbacks in HTML decoding, and adding optional chaining when reading the vectorization status ledger.
| for (const [key, entry] of Object.entries(source)) { | ||
| if (key === "__proto__" || key === "constructor" || key === "prototype") { | ||
| continue; | ||
| } | ||
| result[key] = decodeValue(entry, seen); | ||
| } |
There was a problem hiding this comment.
Skipping 'constructor' and 'prototype' keys during object traversal causes silent data loss if these keys are legitimately used in user inputs (e.g., task descriptions, code snippets, or custom fields). Since we are creating a new object via Object.create(Object.getPrototypeOf(value)) and only copying own properties, setting 'constructor' or 'prototype' as own properties is completely safe and does not cause prototype pollution. Only 'proto' needs to be skipped to prevent prototype pollution.
| for (const [key, entry] of Object.entries(source)) { | |
| if (key === "__proto__" || key === "constructor" || key === "prototype") { | |
| continue; | |
| } | |
| result[key] = decodeValue(entry, seen); | |
| } | |
| for (const [key, entry] of Object.entries(source)) { | |
| if (key === "__proto__") { | |
| continue; | |
| } | |
| result[key] = decodeValue(entry, seen); | |
| } |
| return input; | ||
| } | ||
| // The regex only matches keys present in ENTITY_MAP, so the lookup is total. | ||
| return input.replace(ENTITY_PATTERN, (match) => ENTITY_MAP[match] as string); |
There was a problem hiding this comment.
If ENTITY_PATTERN is ever modified in the future to match additional entities without updating ENTITY_MAP, or if there is a mismatch, ENTITY_MAP[match] will return undefined, which will be coerced to the string 'undefined'. Using a nullish coalescing fallback ENTITY_MAP[match] ?? match is safer and more robust.
| return input.replace(ENTITY_PATTERN, (match) => ENTITY_MAP[match] as string); | |
| return input.replace(ENTITY_PATTERN, (match) => ENTITY_MAP[match] ?? match); |
| try { | ||
| const ledger = await readVectorizationStatusLedger(pmRoot); | ||
| if (ledger.warnings.length > 0) { | ||
| warnings.push(...ledger.warnings); | ||
| } | ||
| const staleIds = collectStaleVectorizationIds( | ||
| filteredDocuments.map((document) => ({ | ||
| id: document.metadata.id, | ||
| updated_at: document.metadata.updated_at, | ||
| })), | ||
| ledger.entries, | ||
| ); |
There was a problem hiding this comment.
The maybeEmitVectorIndexStaleWarning function is designed to be a 'best-effort' warning that shouldn't block queries. However, if readVectorizationStatusLedger returns null, undefined, or an object where warnings is missing, accessing ledger.warnings.length will throw a TypeError. Although caught by the try-catch block, it will silently fail to check for staleness. Adding optional chaining or nullish checks makes this much more robust.
| try { | |
| const ledger = await readVectorizationStatusLedger(pmRoot); | |
| if (ledger.warnings.length > 0) { | |
| warnings.push(...ledger.warnings); | |
| } | |
| const staleIds = collectStaleVectorizationIds( | |
| filteredDocuments.map((document) => ({ | |
| id: document.metadata.id, | |
| updated_at: document.metadata.updated_at, | |
| })), | |
| ledger.entries, | |
| ); | |
| try { | |
| const ledger = await readVectorizationStatusLedger(pmRoot); | |
| if (ledger?.warnings && ledger.warnings.length > 0) { | |
| warnings.push(...ledger.warnings); | |
| } | |
| const staleIds = collectStaleVectorizationIds( | |
| filteredDocuments.map((document) => ({ | |
| id: document.metadata.id, | |
| updated_at: document.metadata.updated_at, | |
| })), | |
| ledger?.entries, | |
| ); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 887d23ea66
ℹ️ 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".
| .option( | ||
| "--value <value>", | ||
| "Value for nested leaf settings keys (search_provider, openai_base_url, ollama_model, vector_store_adapter, qdrant_url, lancedb_path, etc.). Equivalent to the positional value.", |
There was a problem hiding this comment.
When agents discover config flags through pm contracts --command config --flags-only --json, this new --value option is omitted because CONFIG_FLAG_CONTRACTS in src/sdk/cli-contracts.ts still lists only --criterion, --clear-criteria, --format, --policy, and context flags. In the documented contracts-first workflow, that makes the non-positional way to set nested search/provider leaves invisible to automation and generated completions even though Commander accepts it; please add the flag to the config contract surface alongside this registration.
Useful? React with 👍 / 👎.
| if (!baseUrl || !model) { | ||
| return null; | ||
| } | ||
| const override = resolveSearchEmbeddingModelOverride(settings); |
There was a problem hiding this comment.
When a user follows the new nested settings surface and sets openai_base_url plus search_embedding_model (or the analogous Ollama base URL plus default model) without also setting the provider-specific openai_model, this override is never reached because the resolver has already returned null on the empty provider model. That leaves semantic search/reindex reporting no configured provider even though the documented default model is present; compute the override before the missing-model check or use it as the fallback model for provider availability.
Useful? React with 👍 / 👎.
| { | ||
| scope, | ||
| keys, | ||
| nested_settings: nestedSettings, |
There was a problem hiding this comment.
Adding nested_settings only to the list result leaves pm config project export --json omitting every new search/provider/vector-store leaf after users set them (for example openai_base_url, search_provider, or lancedb_path). Since the configuration docs direct users to config ... export for the active runtime shape, backup/audit/migration tooling that relies on export will silently miss the new semantic-search configuration; include these nested values in the export result as well.
Useful? React with 👍 / 👎.
…l safe)
Gemini round-3 follow-up noted that silently dropping `__proto__` /
`constructor` / `prototype` from decoded options is silent data loss for
legitimate cases (e.g. a settings field literally named `constructor`).
Switched the rebuilt-record assignment from `result[key] = ...` to
`Object.defineProperty(result, key, { value, enumerable, writable,
configurable })`. defineProperty bypasses JS's special
prototype-chain-assignment semantics for `__proto__`, so the smuggled
value lands as an ordinary own property without ever touching the
prototype chain.
Net result: data is preserved (no more silent drops), Object.prototype
stays unpolluted, and standard Object methods still work on the decoded
record. Updated the regression test to assert all three (own-property
presence + unchanged Object.prototype + working hasOwnProperty).
100% coverage retained.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Round 4 fix in a9884d5: gemini follow-up: switched decoded-key assignment from Coverage still 100%. @codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces several features and refactors, including support for configuring custom search providers and vector stores via pm config, query-time vector index staleness warnings, a TOON format renderer for the calendar command, a consolidated splitCommaList helper, and defensive HTML-entity decoding at the MCP server boundary. Feedback on these changes focuses on ensuring backward compatibility for the calendar extension's new TOON renderer, resolving global embedding model overrides before validating provider-specific configurations, ensuring consistent HTML entity decoding by removing conditional signal guards, and adding a safety check for ledger.warnings to prevent potential TypeErrors.
| interface CalendarCoreModule { | ||
| runCalendar: (options: CalendarOptions, global: GlobalOptions) => Promise<CalendarResult>; | ||
| renderCalendarMarkdown: (result: CalendarResult) => string; | ||
| renderCalendarToon: (result: CalendarResult) => string; |
There was a problem hiding this comment.
Making renderCalendarToon a required property of CalendarCoreModule can cause backward compatibility issues. If a user updates the calendar extension but has an older version of the pm-cli core installed (which lacks renderCalendarToon), the entire calendar extension will fail to load.
We should make renderCalendarToon optional in the interface and handle its absence gracefully at runtime.
| renderCalendarToon: (result: CalendarResult) => string; | |
| renderCalendarToon?: (result: CalendarResult) => string; |
| if ( | ||
| typeof loaded.runCalendar === "function" && | ||
| typeof loaded.renderCalendarMarkdown === "function" && | ||
| typeof loaded.renderCalendarToon === "function" && |
There was a problem hiding this comment.
Remove the strict check for renderCalendarToon from the module load guard to prevent load failures when running with older core versions.
| if (outputFormat === "toon") { | ||
| const rendered = calendarCore.renderCalendarToon(result); | ||
| return rendered.endsWith("\n") ? rendered : `${rendered}\n`; | ||
| } |
There was a problem hiding this comment.
Gracefully handle the case where renderCalendarToon is not supported by the loaded core module by throwing a clear, user-friendly error instead of crashing or failing silently.
if (outputFormat === "toon") {
if (typeof calendarCore.renderCalendarToon !== "function") {
throw new Error("The installed pm-cli core does not support the TOON calendar format. Please upgrade pm-cli.");
}
const rendered = calendarCore.renderCalendarToon(result);
return rendered.endsWith("\n") ? rendered : rendered + "\n";
}| if ( | ||
| typeof loaded.runCalendar === "function" && | ||
| typeof loaded.renderCalendarMarkdown === "function" && | ||
| typeof loaded.renderCalendarToon === "function" && |
There was a problem hiding this comment.
Remove the strict check for renderCalendarToon from the module load guard in the JavaScript runtime file to maintain parity with the TypeScript version and prevent load failures with older cores.
| if (outputFormat === "toon") { | ||
| const rendered = calendarCore.renderCalendarToon(result); | ||
| return rendered.endsWith("\n") ? rendered : `${rendered}\n`; | ||
| } |
There was a problem hiding this comment.
Gracefully handle the case where renderCalendarToon is not supported by the loaded core module in the JavaScript runtime file.
if (outputFormat === "toon") {
if (typeof calendarCore.renderCalendarToon !== "function") {
throw new Error("The installed pm-cli core does not support the TOON calendar format. Please upgrade pm-cli.");
}
const rendered = calendarCore.renderCalendarToon(result);
return rendered.endsWith("\n") ? rendered : rendered + "\n";
}| if (!input.includes("<") && !input.includes(">")) { | ||
| return input; | ||
| } |
There was a problem hiding this comment.
The activation signal guard !input.includes("<") && !input.includes(">") makes the decoding of other core entities (&, ", ') conditional on the presence of angle brackets. This introduces a significant inconsistency:
"A & B"remains"A & B""A & B <C>"becomes"A & B <C>"
If an MCP client or platform encodes & to & (which is standard for HTML encoding), any message containing & but no angle brackets will be stored in its escaped form (&), while messages with angle brackets will have their & decoded.
Consider removing this conditional guard to ensure consistent decoding of all five core HTML entities across all incoming MCP tool arguments.
| if (!baseUrl || !model) { | ||
| return null; | ||
| } | ||
| const override = resolveSearchEmbeddingModelOverride(settings); |
There was a problem hiding this comment.
If providers.openai.model is not configured but the global search.embedding_model override is set, resolveOpenAiProvider will return null because of the strict if (!baseUrl || !model) check.
We should resolve the override first and check if the effective model is present. This allows the global search_embedding_model to fully satisfy the model requirement without requiring a redundant provider-specific model configuration.
Example refactoring:
function resolveOpenAiProvider(settings: ProviderSettingsInput): EmbeddingProviderConfig | null {
const baseUrl = toNonEmptyString(settings.providers?.openai?.base_url);
const model = toNonEmptyString(settings.providers?.openai?.model);
const override = resolveSearchEmbeddingModelOverride(settings);
const effectiveModel = override ?? model;
if (!baseUrl || !effectiveModel) {
return null;
}
const apiKey = toNonEmptyString(settings.providers?.openai?.api_key);
return {
name: "openai",
base_url: baseUrl,
model: effectiveModel,
...(apiKey ? { api_key: apiKey } : {}),
};
}| if (!baseUrl || !model) { | ||
| return null; | ||
| } | ||
| const override = resolveSearchEmbeddingModelOverride(settings); |
There was a problem hiding this comment.
Similarly to the OpenAI provider, if providers.ollama.model is not configured but the global search.embedding_model override is set, resolveOllamaProvider will return null because of the strict if (!baseUrl || !model) check.
We should resolve the override first and check if the effective model is present.
Example refactoring:
function resolveOllamaProvider(settings: ProviderSettingsInput): EmbeddingProviderConfig | null {
const baseUrl = toNonEmptyString(settings.providers?.ollama?.base_url);
const model = toNonEmptyString(settings.providers?.ollama?.model);
const override = resolveSearchEmbeddingModelOverride(settings);
const effectiveModel = override ?? model;
if (!baseUrl || !effectiveModel) {
return null;
}
return {
name: "ollama",
base_url: baseUrl,
model: effectiveModel,
};
}| if (ledger.warnings.length > 0) { | ||
| warnings.push(...ledger.warnings); | ||
| } |
There was a problem hiding this comment.
If ledger.warnings is undefined or null, accessing ledger.warnings.length will throw a TypeError. Although this is wrapped in a try-catch block, throwing an error here will silently abort the rest of the staleness check (including the collectStaleVectorizationIds call and the stderr warning), meaning the staleness warning will never be shown if warnings is missing from the ledger.
We should guard this with ledger.warnings && ledger.warnings.length > 0 or ledger.warnings?.length.
| if (ledger.warnings.length > 0) { | |
| warnings.push(...ledger.warnings); | |
| } | |
| if (ledger.warnings && ledger.warnings.length > 0) { | |
| warnings.push(...ledger.warnings); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/shared/html-entity-decode.ts (1)
80-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrack clones in a
WeakMapinstead of returning the original node on re-entry.Line 81 and Line 95 currently return the source object/array once it has been seen. That avoids recursion, but it also leaks original nodes into the decoded result and leaves later aliases undecoded (
{ a: shared, b: shared }decodesabut returns the originalsharedforb). Seed the clone in aWeakMapbefore descending so both shared references and cycles resolve to the reconstructed node.♻️ Proposed fix
export function decodeHtmlEntitiesInOptions<T>(options: T): T { - return decodeValue(options, new WeakSet<object>()) as T; + return decodeValue(options, new WeakMap<object, unknown>()) as T; } -function decodeValue(value: unknown, seen: WeakSet<object>): unknown { +function decodeValue(value: unknown, seen: WeakMap<object, unknown>): unknown { if (typeof value === "string") { return decodeHtmlEntitiesIfEscaped(value); } if (Array.isArray(value)) { - if (seen.has(value)) { - return value; + const existing = seen.get(value); + if (existing !== undefined) { + return existing; } - seen.add(value); - return value.map((entry) => decodeValue(entry, seen)); + const result: unknown[] = []; + seen.set(value, result); + for (const entry of value) { + result.push(decodeValue(entry, seen)); + } + return result; } if (value !== null && typeof value === "object") { if (!isPlainObject(value)) { return value; } - if (seen.has(value as object)) { - return value; + const existing = seen.get(value as object); + if (existing !== undefined) { + return existing; } - seen.add(value as object); const source = value as Record<string, unknown>; const result: Record<string, unknown> = Object.create(Object.getPrototypeOf(value as object)); + seen.set(value as object, result); for (const [key, entry] of Object.entries(source)) { Object.defineProperty(result, key, { value: decodeValue(entry, seen), enumerable: true, writable: true, configurable: true, }); } return result; } return value; }Also applies to: 95-115
🤖 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 `@src/core/shared/html-entity-decode.ts` around lines 80 - 85, The current cycle/alias guard uses a Set and returns the original node when re-seen; change the guard to a WeakMap and seed a blank clone before recursing so shared references and cycles resolve to reconstructed nodes: replace the use of seen as a Set with a WeakMap, in the Array branch (Array.isArray(value)) check seen.has(value) and return seen.get(value) if present, otherwise create an empty array clone, call seen.set(value, clone) then populate it by descending with decodeValue; do the same in the object branch (the code around the object handling lines) by creating an empty object clone, set it in seen before recursing into properties, and return the clone.
🤖 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.
Outside diff comments:
In `@src/core/shared/html-entity-decode.ts`:
- Around line 80-85: The current cycle/alias guard uses a Set and returns the
original node when re-seen; change the guard to a WeakMap and seed a blank clone
before recursing so shared references and cycles resolve to reconstructed nodes:
replace the use of seen as a Set with a WeakMap, in the Array branch
(Array.isArray(value)) check seen.has(value) and return seen.get(value) if
present, otherwise create an empty array clone, call seen.set(value, clone) then
populate it by descending with decodeValue; do the same in the object branch
(the code around the object handling lines) by creating an empty object clone,
set it in seen before recursing into properties, and return the clone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a091a33e-12f5-4668-a9b1-2a628d70df19
📒 Files selected for processing (6)
docs/CONFIGURATION.mdsrc/core/search/providers.tssrc/core/search/vector-stores.tssrc/core/shared/html-entity-decode.tstests/unit/html-entity-decode.spec.tstests/unit/search-provider-store-selection.spec.ts
✅ Files skipped from review due to trivial changes (1)
- docs/CONFIGURATION.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/search/vector-stores.ts
- src/core/search/providers.ts
- tests/unit/search-provider-store-selection.spec.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9884d55cf
ℹ️ 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".
| if (!baseUrl || !model) { | ||
| return null; | ||
| } | ||
| const override = resolveSearchEmbeddingModelOverride(settings); |
There was a problem hiding this comment.
When users configure a provider with search_embedding_model as the documented model override but leave the provider-specific model empty (for example openai_base_url + openai_api_key + search_embedding_model + search_provider=openai), this resolver still returns null before it ever reads the override. That makes the provider appear unconfigured even though the new nested setting supplies the model; the same pattern affects the Ollama resolver below. Use the override as the effective model before the !model activation check.
Useful? React with 👍 / 👎.
| { | ||
| scope, | ||
| keys, | ||
| nested_settings: nestedSettings, |
There was a problem hiding this comment.
The new search/provider/vector-store keys are settable and now surfaced in pm config ... list, but pm config ... export --json still only builds values from the old ConfigKey set. In workflows that use export as the documented active runtime shape, settings like search_provider, openai_base_url, or vector_store_adapter are silently omitted after being configured, so backups/automation recreate an incomplete semantic-search configuration. Please include these nested_settings (or their dotted values) in the export result as well.
Useful? React with 👍 / 👎.
|
|
||
| ### Added | ||
|
|
||
| - Calendar best-practice: honor timezone, surface Milestone/Meeting items, ICS export ([pm-xzrx](https://github.com/unbraind/pm-cli/blob/main/.agents/pm/features/pm-xzrx.toon)) |
There was a problem hiding this comment.
This Unreleased entry advertises “ICS export”, but the changed calendar command only exposes --format markdown|toon|json and a repo-wide check for ICS/iCal support shows no export implementation in this patch. Users reading the changelog will expect a calendar export path that does not exist, so this should be reworded or held until the export feature is actually shipped.
Useful? React with 👍 / 👎.
unbraind
deleted the
feat/agent-ux-search-discoverability-calendar-quality
branch
…backfill, frontier notes - file pm-albl for GH-557 (contract layer intercepts -h/--help before variadic-positional handlers) under pm-ugqx - backfill estimate/risk/confidence on pm-d4ns/pm-evav/pm-topu/pm-zt1c (clears missing_estimate warning) - frontier notes on pm-usfg (remaining slices 3mna -> oxrw -> oslr) and pm-4k6b (catalog-to-shipped map for PR#546/#549/#554/#556/#559) - files-missing 162 drift comment on pm-j8z6; pass summary on pm-doxj Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDJ1m5HDPDBVp8BBAixtnP
1 participant
Summary
Combines 5 closed pm items into one focused PR around agent UX + token efficiency + model-agnostic search discoverability:
<type>stored verbatim becomes<type>again. Single-pass via lookup map preserves intentional double-encoding; no-op for normal text.pm_create/pm_updatepreviously crashed withraw.trim is not a functionwhen priority arrived as a JSON number (Claude always JSON-encodespriority: 3, not"3").resolvePrioritynow acceptsstring|numberwith strictNumber.isInteger0..4 range check.search_provider,ollama_base_url,openai_api_key,qdrant_url,lancedb_path, etc.) via a new descriptor table insrc/core/config/nested-settings.ts.docs/CONFIGURATION.mdgains a "Custom search providers" section with copy-pasteable Ollama / OpenAI / LM Studio / vLLM / Qdrant / LanceDB examples. Semantic + hybrid search now emit a one-line vector-index-stale warning at query time.pm calendar --format toonnow emits real TOON via a newrenderCalendarToon()(previously fell through to markdown). Schedule-less Milestone / Meeting / Reminder / Event creates emit a warning to stderr with a copy-pasteablepm update --deadlinesuggestion.splitCommaListhelper consolidates 7 hand-rolled split/trim/filter/dedup idioms; 16 verified-dead exports dropped fromhelp-json-payload.ts(10) andregistration-helpers.ts(6). Bigger items deferred to follow-up PRs.Two follow-up items filed: pm-1lws (pm update should accept
--expected/--actualaliases that pm close accepts;--add-tagsvs--tagsretry-hint inconsistency).Test plan
node scripts/run-tests.mjs test— 1933/1933 pass across 132 filesnode scripts/run-tests.mjs coverage— 100% line / branch / function / statementtsc --noEmitcleanpnpm changelog:pm:checkclean (CHANGELOG auto-generated)/tmp/pm-e2e-2026-05-28:pm config project set search_provider ollama→ writessearch.providerpm config project set search_hybrid_semantic_weight 1.5→ rejects withnumber in [0, 1]errorpm config project listsurfaces all 16nested_settingsentriespm install calendar+pm calendar --format toon→ TOON output decodable by@toon-format/toonpm create Meeting "no date attached"→ emits schedule-less warning with copy-pasteable retrypm search --mode semanticagainst stale ledger → emits one-line stale-index warning +vector_index_stale:Nin JSON warnings--addwith<type>brackets stored verbatim (unchanged); MCP boundary now decodes incoming<type>before storage🤖 Generated with Claude Code
Summary by Sourcery
Improve agent UX around configuration, search, and calendar output while tightening MCP robustness and shared utilities.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: