Repository navigation
Schema & config universal-customization completeness: schema fields/presets/infer + config keys (GH-245) - #259
Merged
Conversation
…-vhbf/86ob/tb42/9byd/nnaq/izx5) Closes the gaps where schema/config customization still required hand-editing JSON, completing the "universal, fully-customizable PM" story under epic pm-8jdc. Schema field management (pm-vhbf): - pm schema add-field/remove-field/list-fields/show-field manage schema/fields.json - new core/schema/fields-file.ts (parse/serialize/upsert/remove + SCHEMA_FIELDS_LOCK_ID), mirroring item-types-file.ts / status-defs-file.ts - pm schema list now surfaces a fields section Type presets & inference: - pm schema apply-preset <agile|ops|research> adopts a type preset in an already-initialized project (idempotent); presets moved to shared core/schema/type-presets.ts consumed by both init and schema (pm-86ob) - pm schema add-type --infer scans existing item title prefixes (BUG:, SPIKE:) to infer custom types; preview by default, --apply to register, --min-count threshold (GH-245, pm-tb42) Config keys (pm-9byd, pm-nnaq): - pm config set id_prefix / author_default / output_default_format / locks_ttl_seconds / schema_unknown_field_policy via NESTED_SETTING_DESCRIPTORS Docs (pm-izx5): - CONFIGURATION.md gains a Custom Runtime Fields section (full RuntimeFieldDefinition shape + end-to-end example) and config-set examples Full surface wiring (CLI/MCP pm_schema/contracts/completion bash+zsh+fish/docs); golden contracts regenerated; 100/100/100/100 coverage held (4454 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Owner
Author
|
@coderabbitai full review |
Owner
Author
|
@codex review |
Owner
Author
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
✅ Action performedFull review finished. |
|
To use Codex here, create a Codex account and connect to github. |
|
Warning Review limit reached
More reviews will be available in 27 minutes and 57 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (37)
📝 WalkthroughWalkthroughAdds three new core schema modules ( ChangesSchema Fields, Presets, Type Inference, and Config Settings
Sequence Diagram(s)sequenceDiagram
participant User as User / MCP client
participant register as register-mutation.ts
participant schema_ts as schema.ts
participant fields_file as fields-file.ts
participant type_presets as type-presets.ts
participant type_inference as type-inference.ts
participant lock as File lock
rect rgba(70, 130, 180, 0.5)
note over User,lock: pm schema add-field
User->>register: pm schema add-field --type string --commands create,update
register->>schema_ts: dispatchSchemaSubcommand → runSchemaAddField(opts)
schema_ts->>fields_file: normalizeAddFieldInput(raw)
schema_ts->>lock: acquire SCHEMA_FIELDS_LOCK_ID
schema_ts->>fields_file: parseFieldsFile(existing JSON)
schema_ts->>fields_file: upsertField(file, normalized)
schema_ts->>fields_file: serializeFieldsFile(updated)
schema_ts-->>register: SchemaAddFieldResult
register-->>User: formatted output
end
rect rgba(100, 180, 100, 0.5)
note over User,lock: pm schema apply-preset
User->>register: pm schema apply-preset --type-preset agile
register->>schema_ts: dispatchSchemaSubcommand → runSchemaApplyPreset(opts)
schema_ts->>type_presets: normalizeTypePresetName("agile")
schema_ts->>type_presets: resolveTypePresetDefinitions("agile")
schema_ts->>lock: acquire SCHEMA_TYPES_LOCK_ID
schema_ts-->>register: SchemaApplyPresetResult {registered, replaced}
register-->>User: formatted output
end
rect rgba(180, 100, 100, 0.5)
note over User,lock: pm schema add-type --infer
User->>register: pm schema add-type --infer --min-count 5 --apply
register->>schema_ts: dispatchSchemaSubcommand → runSchemaInferTypes(opts)
schema_ts->>type_inference: inferTypesFromTitles(titles, {minCount:5})
type_inference-->>schema_ts: InferredTypeCandidate[]
schema_ts->>lock: acquire SCHEMA_TYPES_LOCK_ID (if --apply)
schema_ts-->>register: SchemaAddTypeInferResult {registered, skipped}
register-->>User: formatted output
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sdk/cli-contracts.ts (1)
1908-1912:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
schema add-type --inferis blocked by an over-strict required-field contract.The strict schema still requires
namewheneversubcommand=add-type, but the runtime infer path acceptsadd-typewithinfer=trueand noname. This creates a cross-layer contract rejection for a valid operation.Suggested fix
conditionalRequired: [ { property: "subcommand", value: "show", required: ["name"] }, { property: "subcommand", value: "show-status", required: ["name"] }, - { property: "subcommand", value: "add-type", required: ["name"] }, + // `add-type --infer` does not require `name`; runtime enforces `name` + // for non-infer add-type usage. { property: "subcommand", value: "remove-type", required: ["name"] },Also applies to: 1916-1922
🤖 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/sdk/cli-contracts.ts` around lines 1908 - 1912, The conditionalRequired configuration for the add-type subcommand (and similarly for remove-type in the referenced lines) incorrectly requires the name field unconditionally, but the runtime accepts add-type with infer=true and no name parameter. Remove the name requirement from the conditionalRequired entries where subcommand equals add-type and remove-type, or alternatively modify these conditions to only require name when the infer property is not set to true. This will allow the schema add-type --infer operation to pass contract validation without the name field when infer is explicitly provided.
🧹 Nitpick comments (3)
tests/unit/core/schema/type-inference.spec.ts (1)
22-29: ⚡ Quick winAdd a regression test for sequence-style prefixes (
S001-,E12-).The suite currently checks single-character and numeric-led rejection, but not the documented sequence-style cases. Add one test to lock that behavior.
💡 Suggested test case
it("rejects single-character (sequence/index) prefixes", () => { expect(extractTitlePrefix("S- something")).toBeUndefined(); expect(extractTitlePrefix("E- another")).toBeUndefined(); }); + + it("rejects sequence-style letter+digits prefixes", () => { + expect(extractTitlePrefix("S001- rollout")).toBeUndefined(); + expect(extractTitlePrefix("E12- investigation")).toBeUndefined(); + });🤖 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 `@tests/unit/core/schema/type-inference.spec.ts` around lines 22 - 29, Add a new test case to the test suite that specifically validates the behavior for sequence-style prefixes like "S001-" and "E12-" (letter followed by digits followed by dash). Create a test that calls extractTitlePrefix with these sequence-style prefix examples to ensure they are properly rejected, similar to the existing tests for single-character and numeric-led prefixes. This will lock in the documented behavior for sequence-style cases as a regression test.tests/unit/core/schema/fields-file.spec.ts (1)
145-219: ⚡ Quick winAdd regression coverage for duplicate normalized-key cleanup.
Current tests don’t assert behavior when input contains duplicate normalized keys (e.g.,
Owner+owner). Given key-normalized identity rules, add-field/remove-field should converge to a single entry / remove all matches.💡 Suggested test cases
describe("upsertField", () => { + it("collapses duplicate normalized-key entries on upsert", () => { + const seed = { + fields: [ + { key: "Owner", type: "string" }, + { key: "owner", type: "number" }, + ], + }; + const result = upsertField(seed as never, normalizeAddFieldInput({ key: "owner", type: "number" })); + expect(result.file.fields.filter((f) => f.key.toLowerCase() === "owner")).toHaveLength(1); + }); @@ describe("removeField", () => { + it("removes all entries matching the normalized key", () => { + const seed = { fields: [{ key: "Owner", type: "string" }, { key: "owner", type: "number" }] }; + const result = removeField(seed as never, "owner"); + expect(result.removed).toBe(true); + expect(result.file.fields).toHaveLength(0); + });🤖 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 `@tests/unit/core/schema/fields-file.spec.ts` around lines 145 - 219, Add regression test coverage for duplicate normalized-key scenarios in the upsertField and removeField test suites. Specifically, add a test case that verifies upsertField behavior when attempting to insert multiple fields with keys that normalize to the same value (e.g., "Owner" and "owner"), ensuring the function properly handles and consolidates these duplicates into a single entry. Additionally, add a test case for removeField that verifies it correctly removes all field entries that match the normalized key when duplicates exist in the stored state, rather than leaving partial matches behind.tests/unit/commands/schema-command.spec.ts (1)
1448-1455: ⚡ Quick winStrengthen the idempotent field-upsert assertion to cover boolean field constraints.
This test only checks
replaced; add explicit assertions forrequired,required_on_create, andallow_unsetso future option-normalization changes don’t silently regress field semantics.🤖 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 `@tests/unit/commands/schema-command.spec.ts` around lines 1448 - 1455, The idempotent field-upsert test for the schema.runSchemaAddField method currently only asserts that the replaced property is true, but does not verify that the boolean field constraints remain consistent. After the expect(again.replaced).toBe(true) assertion, add explicit assertions to verify the required, required_on_create, and allow_unset properties on the again object match their expected values from the initial field creation. This ensures that future option-normalization changes do not silently break field semantics during idempotent operations.
🤖 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/features/pm-nnaq.toon:
- Around line 3-11: The policy value terminology is inconsistent throughout the
feature tracker entry. The description field and acceptance_criteria field
reference strict mode, but the implementation uses reject instead. Choose one
consistent policy value name (either stick with strict or align with reject) and
update all references across the description, acceptance_criteria, and any other
fields in this feature entry to use that single consistent value.
In `@CHANGELOG.md`:
- Around line 7-8: The config key spelling in the first changelog entry uses
hyphens (schema-unknown-field-policy) while the rest of the documentation and
this PR uses underscores (schema_unknown_field_policy), creating an
inconsistency. Update the first line item to replace schema-unknown-field-policy
with schema_unknown_field_policy to match the underscore naming convention used
consistently throughout the PR and documentation.
In `@src/cli/commands/schema.ts`:
- Around line 1436-1437: The parseItemTypesFile call on the line containing
readFileIfExists(typesPath) is not wrapped in error handling, causing raw parser
errors to be thrown instead of normalized PmCliError when types.json is
malformed. Wrap the parseItemTypesFile call (and the subsequent assignment to
definitionsCount) in a try-catch block that catches any parsing errors and
converts them to PmCliError with an appropriate error message, following the
same error handling pattern used elsewhere in the schema command.
In `@src/cli/register-mutation.ts`:
- Around line 221-223: The "infer-types" case in the switch statement has an
early return statement that prevents execution of a shared warning-print block
that appears later in the function. Remove the early return statement from the
"infer-types" case (after the writeStdout call) and allow the code to fall
through to the warning emission block that handles extension hook warnings. The
same issue also applies to another case block around lines 237-241, so apply the
same fix there as well by removing early returns that prevent the shared warning
code from executing.
- Line 1425: The parseSchemaOrderOption function call for parsing the minCount
option allows zero and negative values, which are then silently ignored and
replaced with default thresholds without informing the user. Instead of using
parseSchemaOrderOption for minCount, implement explicit validation that rejects
non-positive values by throwing an error or exiting with a clear error message
when the user provides an invalid minCount value, ensuring user input is not
silently discarded.
In `@src/core/schema/fields-file.ts`:
- Around line 299-363: Both the upsertField and removeField functions only
operate on the first normalized-key match using findIndex, which leaves stale
duplicates behind if the fields array already contains duplicate entries. To fix
this, in the upsertField function, after finding the first match with findIndex,
remove all other fields with the same normalized key before updating or adding
the field. In the removeField function, instead of using findIndex and removing
only one field with splice, filter the entire fields array to remove all fields
that match the normalized key, ensuring complete deduplication rather than
leaving stale duplicates in the file.
In `@src/core/schema/type-inference.ts`:
- Around line 45-57: The extractTitlePrefix function currently only rejects
single-character tokens, but sequence-style prefixes like S001 or E12 (a single
letter followed by digits) are still passing through and being treated as valid
type prefixes. In addition to the existing token length check that rejects
tokens with length less than 2, add another validation condition to reject
tokens that match a sequence/index marker pattern (single letter followed by one
or more digits). This pattern check should be added after the existing length
validation in the extractTitlePrefix function to ensure these sequence markers
are properly excluded.
In `@src/sdk/cli-contracts/tool-parameter-tables.ts`:
- Around line 625-631: The `apply` parameter description in the
tool-parameter-tables.ts file is now ambiguous because it is referenced in
multiple contexts: file discovery and schema add-type inference (via the `infer`
parameter). Update the `apply` parameter's description to document both usages
clearly so that generated tool guidance accurately reflects how this parameter
affects both file discovery and schema inference behavior. Ensure the updated
description in the `apply` metadata covers both use cases mentioned in the
comments and diff locations provided.
In `@tests/fixtures/contracts/full.json`:
- Around line 18131-18133: The description property for the "name" field in the
SDK contracts is incomplete. Update the description to include "custom field key
(show-field/add-field/remove-field)" in addition to the existing references to
"Item type name" and "status id", making it consistent with the MCP tool
definition at line 9177 and covering all field operation contexts enforced by
the conditional requirements at lines 17953-18002.
---
Outside diff comments:
In `@src/sdk/cli-contracts.ts`:
- Around line 1908-1912: The conditionalRequired configuration for the add-type
subcommand (and similarly for remove-type in the referenced lines) incorrectly
requires the name field unconditionally, but the runtime accepts add-type with
infer=true and no name parameter. Remove the name requirement from the
conditionalRequired entries where subcommand equals add-type and remove-type, or
alternatively modify these conditions to only require name when the infer
property is not set to true. This will allow the schema add-type --infer
operation to pass contract validation without the name field when infer is
explicitly provided.
---
Nitpick comments:
In `@tests/unit/commands/schema-command.spec.ts`:
- Around line 1448-1455: The idempotent field-upsert test for the
schema.runSchemaAddField method currently only asserts that the replaced
property is true, but does not verify that the boolean field constraints remain
consistent. After the expect(again.replaced).toBe(true) assertion, add explicit
assertions to verify the required, required_on_create, and allow_unset
properties on the again object match their expected values from the initial
field creation. This ensures that future option-normalization changes do not
silently break field semantics during idempotent operations.
In `@tests/unit/core/schema/fields-file.spec.ts`:
- Around line 145-219: Add regression test coverage for duplicate normalized-key
scenarios in the upsertField and removeField test suites. Specifically, add a
test case that verifies upsertField behavior when attempting to insert multiple
fields with keys that normalize to the same value (e.g., "Owner" and "owner"),
ensuring the function properly handles and consolidates these duplicates into a
single entry. Additionally, add a test case for removeField that verifies it
correctly removes all field entries that match the normalized key when
duplicates exist in the stored state, rather than leaving partial matches
behind.
In `@tests/unit/core/schema/type-inference.spec.ts`:
- Around line 22-29: Add a new test case to the test suite that specifically
validates the behavior for sequence-style prefixes like "S001-" and "E12-"
(letter followed by digits followed by dash). Create a test that calls
extractTitlePrefix with these sequence-style prefix examples to ensure they are
properly rejected, similar to the existing tests for single-character and
numeric-led prefixes. This will lock in the documented behavior for
sequence-style cases as a regression test.
🪄 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: e7680f12-b175-4009-ba39-edde924753f9
📒 Files selected for processing (37)
.agents/pm/chores/pm-izx5.toon.agents/pm/extensions/.managed-extensions.json.agents/pm/features/pm-86ob.toon.agents/pm/features/pm-9byd.toon.agents/pm/features/pm-nnaq.toon.agents/pm/features/pm-tb42.toon.agents/pm/features/pm-vhbf.toon.agents/pm/history/pm-86ob.jsonl.agents/pm/history/pm-9byd.jsonl.agents/pm/history/pm-izx5.jsonl.agents/pm/history/pm-nnaq.jsonl.agents/pm/history/pm-tb42.jsonl.agents/pm/history/pm-vhbf.jsonlCHANGELOG.mddocs/COMMANDS.mddocs/CONFIGURATION.mdsrc/cli/commands/completion.tssrc/cli/commands/index.tssrc/cli/commands/init.tssrc/cli/commands/schema.tssrc/cli/register-mutation.tssrc/core/config/nested-settings.tssrc/core/schema/fields-file.tssrc/core/schema/type-inference.tssrc/core/schema/type-presets.tssrc/mcp/server.tssrc/mcp/tool-definitions.tssrc/sdk/cli-contracts.tssrc/sdk/cli-contracts/tool-parameter-tables.tstests/fixtures/contracts/full.jsontests/integration/mcp-handshake.spec.tstests/unit/commands/config-key-aliases-search.spec.tstests/unit/commands/register-commands.spec.tstests/unit/commands/schema-command.spec.tstests/unit/core/schema/fields-file.spec.tstests/unit/core/schema/type-inference.spec.tstests/unit/core/schema/type-presets.spec.ts
…lds dedup, error parity, docs) - schema add-type --infer: reject non-positive/non-integer --min-count with a USAGE error instead of silently defaulting to 10; wrap the preview-mode types.json parse in PmCliError(GENERIC_FAILURE) for parity with the apply path - type-inference: actually exclude single-letter+digit sequence/index prefixes (S001-, E12-, V1:) — the JSDoc claimed this but the code only rejected single-character tokens; multi-letter tokens ending in digits (BUG2) stay valid - register-mutation: infer-types human dispatch now `break` (not early `return`) so extension on-write hook warnings surface via the shared warning block - fields-file: upsertField/removeField collapse/remove ALL normalized-key matches so a hand-authored fields.json with duplicate keys leaves no stale dup - MCP pm_schema `name` param description now covers custom field keys (show-field/add-field/remove-field); `apply` description covers both files discovery and add-type inference - align pm-nnaq tracker text + auto-changelog with the implemented `reject` value (spec said `strict`) and underscore key form Golden contracts + CHANGELOG regenerated; 100/100/100/100 coverage held (4455 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai full review All 9 review findings addressed in 02972e2:
100% coverage held (4455 tests); golden + CHANGELOG regenerated. |
|
✅ Action performedFull review finished. |
This was referenced Jul 5, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Completes the universal, fully-customizable project-management story (epic pm-8jdc) by closing every remaining gap where schema/config customization still required hand-editing JSON. Six related items shipped together as one coherent surface.
Philosophy: project management = context management — these are agent/script-friendly primitives so any project can model its own vocabulary without touching
.agents/pm/**.What's new
Custom runtime fields are now CLI-managed (pm-vhbf)
pm schema add-field <key> --type <t> --commands create,update [--required|--required-on-create|--no-allow-unset|--cli-flag|--alias|--required-types|--description]pm schema remove-field <key>,pm schema list-fields,pm schema show-field <key>core/schema/fields-file.ts(parse/serialize/upsert/remove +SCHEMA_FIELDS_LOCK_ID) mirroringitem-types-file.ts/status-defs-file.tspm schema listnow surfaces afieldssectionType presets & inference (pm-86ob, pm-tb42 / GH-245)
pm schema apply-preset <agile|ops|research>adopts a domain type preset in an already-initialized project (idempotent batch upsert). Presets moved to sharedcore/schema/type-presets.tsconsumed by bothinitandschema.pm schema add-type --inferscans existing item title prefixes (BUG:,SPIKE:…) and infers custom item types — preview by default,--applyto register,--min-countthreshold.Config keys via
pm config set(pm-9byd, pm-nnaq)id_prefix,author_default,output_default_format,locks_ttl_seconds,schema_unknown_field_policyexposed throughNESTED_SETTING_DESCRIPTORS(dotted paths verified against the settings validator).Docs (pm-izx5)
CONFIGURATION.mdgains a Custom Runtime Fields section (fullRuntimeFieldDefinitionshape + end-to-endcomponentexample) andpm config setexamples for the 5 new keys.Surface wiring
CLI commander/contracts + MCP
pm_schema(subcommand enum, param tables, dispatch) + completion (bash/zsh/fish) + help/docs. Golden contracts regenerated (pnpm contracts:update).Testing
4454tests pass.pnpm typecheck,quality:static,quality:docs-skills,contracts:checkall green.create --component billinground-trip, apply-preset agile, add-type --infer (preview + idempotent --apply with threshold), remove-field, andconfig set/getfor all 5 keys (id_prefixconfirmed applied to real item IDs).Work tracking
Implemented by two specialized agents on disjoint file sets (schema-family / config-family); each tracked design decisions, surfaces wired, and surprises as comments on its pm items. CHANGELOG auto-generated by pm-changelog.
Closes #245.
🤖 Generated with Claude Code