feat(custom-properties): add organization custom property structures and per-scenario custom properties data - #76
Merged
Merged
Conversation
Remove unused MAKE_ORGANIZATION var in scenario-custom-properties integration test and reformat lines exceeding the print width in the new custom-properties source/test files, per final verification pass.
…n mock timestamp, document belongers casing risk
…ecific status code
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds first-class SDK support for Make “Custom Properties” by introducing endpoints to manage organization-level custom property structures/items and to read/write per-scenario custom properties data, along with MCP tool definitions and client wiring.
Changes:
- Added new SDK endpoints:
customPropertyStructures(plus.items) andscenarios.customProperties. - Added MCP tool definitions for the new endpoints and registered them in the global tools aggregator.
- Added unit + integration tests and mock fixtures, and updated README tool categories.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/scenario-custom-properties.spec.ts | Unit tests for scenario custom properties CRUD behavior. |
| test/scenario-custom-properties.integration.test.ts | Integration coverage for scenario custom properties lifecycle against live API. |
| test/scenario-custom-properties-tools.spec.ts | Unit tests for MCP tool wrappers for scenario custom properties. |
| test/mocks/scenario-custom-properties/get.json | Mock response for scenario custom properties get. |
| test/mocks/scenario-custom-properties/create.json | Mock response for scenario custom properties create. |
| test/mocks/scenario-custom-properties/replace.json | Mock response for scenario custom properties replace. |
| test/mocks/scenario-custom-properties/update.json | Mock response for scenario custom properties update. |
| test/mocks/scenario-custom-properties/delete.json | Mock response for scenario custom properties delete. |
| test/mocks/custom-property-structures/list.json | Mock response for listing custom property structures. |
| test/mocks/custom-property-structures/create.json | Mock response for creating a custom property structure. |
| test/mocks/custom-property-structure-items/list.json | Mock response for listing structure items (with pagination). |
| test/mocks/custom-property-structure-items/create.json | Mock response for creating a structure item. |
| test/mocks/custom-property-structure-items/update.json | Mock response for updating a structure item. |
| test/mocks/custom-property-structure-items/delete.json | Mock response for deleting a structure item. |
| test/custom-property-structures.spec.ts | Unit tests for structure list/create endpoints. |
| test/custom-property-structures.integration.test.ts | Integration coverage for structures + item create/update/delete/list. |
| test/custom-property-structures-tools.spec.ts | Unit tests for MCP tools for custom-property-structures. |
| test/custom-property-structure-items.spec.ts | Unit tests for structure items list/create/update/delete (with confirmation). |
| test/custom-property-structure-items-tools.spec.ts | Unit tests for MCP tools for custom-property-structure-items. |
| src/tools.ts | Registers the new endpoint tool arrays into MakeTools. |
| src/make.ts | Wires customPropertyStructures into the Make client. |
| src/index.ts | Re-exports the new endpoint/public types from src/endpoints/*. |
| src/endpoints/scenarios.ts | Adds scenarios.customProperties sub-endpoint on the Scenarios client. |
| src/endpoints/scenario-custom-properties.ts | Implements scenario custom properties data operations (get/create/replace/update/delete). |
| src/endpoints/scenario-custom-properties.tools.ts | Adds MCP/CLI tool definitions for scenario custom properties operations. |
| src/endpoints/custom-property-structures.ts | Implements organization custom property structure list/create + .items accessor. |
| src/endpoints/custom-property-structures.tools.ts | Adds MCP/CLI tool definitions for structure list/create. |
| src/endpoints/custom-property-structure-items.ts | Implements structure items list/create/update/delete (including delete confirmation option). |
| src/endpoints/custom-property-structure-items.tools.ts | Adds MCP/CLI tool definitions for structure items operations. |
| README.md | Documents the new tool categories under “Custom Properties”. |
Suppressed comments (1)
src/endpoints/custom-property-structures.ts:38
- This field JSDoc includes internal SQL references and speculation about
create()output format. Public SDK docs should avoid references to internal server files and unverified behavior; keep the description focused on the API’s observable contract.
* `create()`'s format could not be verified live — creating a second structure is
* permanent (fails with IM005) — so treat it as possibly date-only per
* `structure_add.sql`'s `created::date` cast until verified.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ck date - #1/#2 (r3740513413) custom-property-structures.ts: strip internal SQL filenames and unverified casing speculation from public JSDoc - #3 (r3740513423) custom-property-structure-items.ts: remove "Task 1's findings" reference, not meaningful outside the PR - #4 (r3740513429) scenario-custom-properties.ts: class JSDoc no longer claims the class is bound to a single scenario - #5 (r3740513436) scenario-custom-properties/create.json: fix invalid calendar date (2024-09-31 has no such day) in mock timestamp Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- [self-review, silent-failure-hunter] ScenarioCustomProperties.delete() hardcoded confirmed:true unconditionally, unlike the sibling structure-items.delete(), which correctly exposes it as a caller-controlled option. Added DeleteScenarioCustomPropertiesOptions with an optional confirmed flag, wired through the SDK method, MCP tool, and exports. - [self-review, comment-analyzer] Integration test's "400 is this SDK's convention" comment cited an unrelated mock (a different app's proxy error) as false authority for an unverified assumption. Reworded to state the guess honestly and added a console.warn so a wrong guess is visible in CI output instead of silently passing. - [self-review, pr-test-analyzer + code-reviewer] CustomPropertyStructureItems.list()'s full option surface (cols/pg/id/name/label/description/type/required) was untested. Added a test exercising every option together. - [self-review, type-design-analyzer] CreateCustomPropertyStructureItemBody's type/options coupling (required for dropdown/multiselect, forbidden otherwise) was only documented in prose; converted to a discriminated union so misuse is a compile error. Updated the one MCP tool call site accordingly. - [self-review, comment-analyzer] Removed a leftover "Confirmed live:" phrasing inconsistent with the round-1 cleanup; fixed "which also report" grammar. - [self-review, type-design-analyzer] Added doc notes: associatedType/associatedTypes asymmetry, why CustomPropertyStructures has no get()/update(), and that update() needs null (not undefined) to clear a custom property key. Dropped as out-of-scope/informational: untracked scratch files at repo root (not part of this diff, not swept into any commit here) and a suggestion to add unit tests asserting specific API error codes end-to-end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- [self-review, code-reviewer + type-design-analyzer] Round 2 added
DeleteScenarioCustomPropertiesOptions.confirmed to fix a silent-failure risk, but
round 3 correctly called that same option speculative generality (the API has never
used confirmed for this endpoint). Resolved by removing the option entirely — delete()
now sends no query params, which satisfies both concerns: no false confirmation
bypass, no speculative unused parameter.
- [self-review, code-reviewer] custom-property-structure-items_update's
destructiveHint:false contradicted its own description (irreversible option removal);
flipped to true.
- [self-review, pr-test-analyzer + type-design-analyzer + code-reviewer] The
discriminated union's dropdown/multiselect+options branch was never exercised by any
test. Added unit + MCP-tool-level tests constructing such an item.
- [self-review, multiple] Added replace() (PUT) integration coverage — previously zero;
softened a comment overclaiming 401/404/405 would definitely mean an SDK bug (a 403
from a read-scoped key isn't ruled out by get() succeeding).
- [self-review, pr-test-analyzer] Added a test asserting update() sends `null` (not
omitted) for a cleared key — the one documented behavioral gotcha had zero coverage.
- [self-review, pr-test-analyzer + type-design-analyzer] create()'s belongers casing
risk was still unverified with no live assertion; added one to the integration test
(exercises whenever a fresh org's create() branch actually runs).
- Suggestions applied: fixed a self-contradictory JSDoc ("defaults to false" on a
required field), reworded an inaccurate as-cast comment (JSONSchema does support
oneOf/const; reworded rather than restructuring, matching this repo's
simplify-for-MCP convention), fixed an overstated tool description ("at most one
structure per organization" vs. the actual per-combination constraint), imported
ScenarioCustomPropertiesData instead of inline Record types in the MCP tools file,
added missing id/description filters to custom-property-structure-items_list plus
tests, aligned a mock timestamp format with the documented shape.
Dropped as cosmetic/subjective: consolidating README's Custom Properties bullet to
match Data Store's per-resource layout, splitting structure-items integration coverage
into its own file (content coverage already exists, this is organizational-only), and
strengthening toBeDefined()-only integration assertions (would require guessing
org-specific structure fields).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- [self-review, code-reviewer] Structure-item integration test leaked the item on any
update() failure (delete() was only reachable on the happy path). Wrapped in
try/finally so cleanup always runs.
- [self-review, silent-failure-hunter + pr-test-analyzer] Scenario-custom-properties
integration test only asserted toBeDefined() on create()/update()/replace() writes.
Since an empty body only succeeds when the structure has no required items, the
result should be {} — strengthened to toStrictEqual({}), matching the existing
post-delete assertion. Also consolidated two adjacent comments describing
inconsistent causal stories for the same 400, and dropped an unnecessary `as MakeError`
cast in favor of instanceof narrowing.
- [self-review, type-design-analyzer] cols: ['options'] on
CustomPropertyStructureItems.list() type-checked to a guaranteed non-null array via
PickColumns, contradicting the field's own documented conditional-omission behavior.
Excluded 'options' from the selectable cols keys, mirroring the existing pg/Omit
precedent two lines away.
- [self-review, pr-test-analyzer] UpdateCustomPropertyStructureItemBody.options — the
destructive full-replacement field driving destructiveHint:true — was never exercised
by any test. Added unit + MCP-tool-level tests. Also added the bare (no-confirmation)
path for structure-item delete at the tool layer, previously only tested with
confirmed:true.
- [self-review, comment-analyzer] A comment added in round 3 ("Resolves the casing
doubt documented on CustomPropertyStructure.belongers") referenced JSDoc that round 1
had already deleted — reworded to describe what the assertion actually does.
- [self-review, code-reviewer] Round 3's mock-timestamp "consistency" fix for create()
silently asserted the wrong shape: the (untracked, pre-existing) planning notes record
that create()'s internal SQL casts the timestamp to date-only, unlike list()'s full
timestamp — the opposite of what round 3 assumed. Restored a narrowly-worded (no
internal implementation details) caveat on the `created` field and changed the mock to
a date-only value; added a live integration check that only verifies the value parses
as a real date, since the actual format is still unconfirmed either way.
- [self-review, code-reviewer] ScenarioCustomPropertiesData now excludes `undefined` at
the top level (Record<string, Exclude<JSONValue, undefined>>), turning the documented
"pass null, not undefined, to clear a key" rule into a compile error.
Dropped as low-confidence/not worth doing speculatively: a non-empty-array constraint on
dropdown/multiselect options, aligning silent-skip-guard styles across integration test
files (pre-existing pattern elsewhere too), and adding an IM027 precondition mention to
create()'s docs (unconfirmed whether it actually applies to that endpoint).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/endpoints/custom-property-structures.ts:31
- The
createdfield JSDoc still speculates aboutcreate()possibly returning a different (date-only) format. Unverified API behavior in public docs is likely to get stale and can confuse SDK consumers; it’s safer to describe only what’s known and treat the format as API-defined.
/**
* Creation timestamp, e.g. `"2026-07-15T13:31:53.345339+00:00"` when returned by `list()`.
* `create()`'s response format for this field hasn't been verified live and may differ
* (possibly date-only) — treat it as unconfirmed until checked against a real response.
*/
JanKulhavy
marked this pull request as ready for review
August 10, 2026 07:08
|
@make-review-bot |
Andrii Chumak (andriichumak)
approved these changes
Aug 10, 2026
JanKulhavy
added a commit
that referenced
this pull request
Aug 10, 2026
Bump version to 1.6.7 for release. Changes since v1.6.6: - feat(custom-properties): add organization custom property structures and per-scenario custom properties data (#76)
Andrii Chumak (andriichumak)
pushed a commit
that referenced
this pull request
Aug 10, 2026
## Summary - Bump version to 1.6.7 for release. Changes since v1.6.6: - feat(custom-properties): add organization custom property structures and per-scenario custom properties data (#76)
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.
customPropertyStructures,.items) for defining custom-property field schemasscenarios.customProperties) for reading/writing the actual field values against a scenarioMakeclient,src/index.tsexports, andREADME.md