Skip to content

[Issue #798] Transforms PoC: TypeScript#825

Draft
SnowboardTechie wants to merge 16 commits into
HOLD-transformsfrom
bryan/issue-798-transforms-poc-typescript
Draft

[Issue #798] Transforms PoC: TypeScript#825
SnowboardTechie wants to merge 16 commits into
HOLD-transformsfrom
bryan/issue-798-transforms-poc-typescript

Conversation

@SnowboardTechie
Copy link
Copy Markdown
Collaborator

@SnowboardTechie SnowboardTechie commented May 12, 2026

Summary

A TypeScript proof-of-concept for the plugin transformation framework, validating the contract shape from SPIKE #657 before it's locked into both SDKs. Mirrors the Python PoC (#810). Plugin authors compile declarative ADR-0017 mapping objects into typed (toCommon, fromCommon) callables, optionally validate toCommon output against an extended Zod schema, and attach them to a plugin via definePlugin({ transformSchemas }).

Changes proposed

New public surface under @common-grants/sdk/extensions:

  • buildTransforms() — compiles a pair of ADR-0017 mappings into typed (toCommon, fromCommon) callables, with call-time structural validation and an optional commonModel Zod schema (parse failures become PluginErrors). Six built-in handlers: const, field, match/switch, numberToString, stringToNumber.
  • definePlugin() — now accepts optional meta and transformSchemas; existing callers passing only extensions are unaffected.
  • Supporting types + PluginError, plus a runnable round-trip example.
  • ADR-0024 alignment — handlers preserve the three-state contract for optional fields (absent / explicit null / value) end-to-end through toCommon / fromCommon.

Context for reviewers

  • Scope: proof-of-concept. The goal is validating the contract shape, not shipping the full implementation. Deferred to the full SDK (ADR-0022 Decision ADR - Specification framework #7): auto-generating transforms from declarative extensions.schemas mappings, always-on commonModel validation, and the full definePlugin extension accepting schemas with toCommon/fromCommon ([TSX SDK] Extend definePlugin() to accept schemas with toCommon/fromCommon #756).
  • Base branch is HOLD-transforms (the transforms feature bucket), not main.
  • Exercised from a downstream consumer integration, not just the internal test suite.

Additional information

Run the round-trip demo from lib/ts-sdk/: pnpm example:transforms.

@github-actions github-actions Bot added dependencies Pull requests that update a dependency file sdk Issue or PR related to our SDKs typescript Issue or PR related to TypeScript tooling ts-sdk Related to TypeScript SDK labels May 12, 2026
@SnowboardTechie SnowboardTechie added the enhancement New feature or request label May 12, 2026
@SnowboardTechie SnowboardTechie force-pushed the bryan/issue-798-transforms-poc-typescript branch 2 times, most recently from 1754b34 to b408bef Compare May 14, 2026 16:40
SnowboardTechie added a commit that referenced this pull request May 14, 2026
Two foot-guns flagged in code review on PR #825:

1. Integer-shaped strings beyond Number.MAX_SAFE_INTEGER silently lost
   precision via Number(s) (e.g. "9999999999999999999" → 1e19). Plugin
   authors round-tripping 64-bit IDs through stringToNumber would have
   seen silent corruption. Guard the integer branch with
   Number.isSafeInteger and throw on overflow; callers needing arbitrary
   precision should declare the field as a string or write a custom
   handler returning BigInt.

2. Empty and whitespace-only strings coerced to 0 via Number("") /
   Number("  "), turning an implicit-absent CSV cell into a real zero on
   the transformed side. Explicit s === "" check after .trim() throws on
   both cases.

Docstring rewritten to document both divergences from Python's int()
alongside the existing "42.0" decimal-fallback note. Six new tests pin
the boundaries (MAX/MIN_SAFE_INTEGER accepted, beyond-safe rejected on
both signs, empty and whitespace-only rejected).

pnpm run checks clean; pnpm test 443/443; example:transforms round-trip
still verifies (legacy ID 12345 well within safe-integer range).
@SnowboardTechie
Copy link
Copy Markdown
Collaborator Author

HOLD-transforms synced from main on 2026-05-20: 12 commits — 7 Dependabot / catalog dep bumps (#829, #826, #822, #821, #819, #817, #808), 3 CI / docs chores (#834, #820, #816), 2 release version bumps [skip ci]. Merge was clean (no conflicts). You may want to rebase or merge HOLD-transforms back into your branch.

@SnowboardTechie SnowboardTechie force-pushed the bryan/issue-798-transforms-poc-typescript branch from 2a4f4c6 to 1fdd6e7 Compare May 20, 2026 17:08
SnowboardTechie added a commit that referenced this pull request May 20, 2026
Two foot-guns flagged in code review on PR #825:

1. Integer-shaped strings beyond Number.MAX_SAFE_INTEGER silently lost
   precision via Number(s) (e.g. "9999999999999999999" → 1e19). Plugin
   authors round-tripping 64-bit IDs through stringToNumber would have
   seen silent corruption. Guard the integer branch with
   Number.isSafeInteger and throw on overflow; callers needing arbitrary
   precision should declare the field as a string or write a custom
   handler returning BigInt.

2. Empty and whitespace-only strings coerced to 0 via Number("") /
   Number("  "), turning an implicit-absent CSV cell into a real zero on
   the transformed side. Explicit s === "" check after .trim() throws on
   both cases.

Docstring rewritten to document both divergences from Python's int()
alongside the existing "42.0" decimal-fallback note. Six new tests pin
the boundaries (MAX/MIN_SAFE_INTEGER accepted, beyond-safe rejected on
both signs, empty and whitespace-only rejected).

pnpm run checks clean; pnpm test 443/443; example:transforms round-trip
still verifies (legacy ID 12345 well within safe-integer range).
@SnowboardTechie SnowboardTechie force-pushed the bryan/issue-798-transforms-poc-typescript branch from 1fdd6e7 to f97a3ba Compare May 21, 2026 16:29
SnowboardTechie added a commit that referenced this pull request May 21, 2026
Two foot-guns flagged in code review on PR #825:

1. Integer-shaped strings beyond Number.MAX_SAFE_INTEGER silently lost
   precision via Number(s) (e.g. "9999999999999999999" → 1e19). Plugin
   authors round-tripping 64-bit IDs through stringToNumber would have
   seen silent corruption. Guard the integer branch with
   Number.isSafeInteger and throw on overflow; callers needing arbitrary
   precision should declare the field as a string or write a custom
   handler returning BigInt.

2. Empty and whitespace-only strings coerced to 0 via Number("") /
   Number("  "), turning an implicit-absent CSV cell into a real zero on
   the transformed side. Explicit s === "" check after .trim() throws on
   both cases.

Docstring rewritten to document both divergences from Python's int()
alongside the existing "42.0" decimal-fallback note. Six new tests pin
the boundaries (MAX/MIN_SAFE_INTEGER accepted, beyond-safe rejected on
both signs, empty and whitespace-only rejected).

pnpm run checks clean; pnpm test 443/443; example:transforms round-trip
still verifies (legacy ID 12345 well within safe-integer range).
@github-actions github-actions Bot added cli Issue or PR related to the @common-grants/cli library website Issues related to the website core Issues related to @common-grants/core library python Issue or PR related to Python tooling py-sdk Related to Python SDK labels May 21, 2026
@github-actions
Copy link
Copy Markdown
Contributor

🚀 Website Preview Deployed!

Preview your changes at: https://cg-pr-825.billy-daly.workers.dev

This preview will be automatically deleted when the PR is closed.

@SnowboardTechie SnowboardTechie force-pushed the bryan/issue-798-transforms-poc-typescript branch from f97a3ba to 1897cb1 Compare May 21, 2026 17:01
SnowboardTechie added a commit that referenced this pull request May 21, 2026
Two foot-guns flagged in code review on PR #825:

1. Integer-shaped strings beyond Number.MAX_SAFE_INTEGER silently lost
   precision via Number(s) (e.g. "9999999999999999999" → 1e19). Plugin
   authors round-tripping 64-bit IDs through stringToNumber would have
   seen silent corruption. Guard the integer branch with
   Number.isSafeInteger and throw on overflow; callers needing arbitrary
   precision should declare the field as a string or write a custom
   handler returning BigInt.

2. Empty and whitespace-only strings coerced to 0 via Number("") /
   Number("  "), turning an implicit-absent CSV cell into a real zero on
   the transformed side. Explicit s === "" check after .trim() throws on
   both cases.

Docstring rewritten to document both divergences from Python's int()
alongside the existing "42.0" decimal-fallback note. Six new tests pin
the boundaries (MAX/MIN_SAFE_INTEGER accepted, beyond-safe rejected on
both signs, empty and whitespace-only rejected).

pnpm run checks clean; pnpm test 443/443; example:transforms round-trip
still verifies (legacy ID 12345 well within safe-integer range).
@SnowboardTechie
Copy link
Copy Markdown
Collaborator Author

Synced mainHOLD-transforms on 2026-05-21 (merge commit 211ecec). 9 commits landed:

Notable for this PR: the lockfile-side effect of #842 bumps brace-expansion 5.0.5 → 5.0.6, which resolves GHSA-jxxr-4gwj-5jf2 — the advisory the Audit dependencies CI step was flagging. Re-run CI on this PR to refresh against the updated base.

Port the Python transforms PoC (PR #810, branch 799-transform-poc-fetch)
to @common-grants/sdk so the ADR-0022 / ADR-0017 contract is validated
in both SDKs before either is locked in for full implementation.

Public additions under @common-grants/sdk/extensions:

- buildTransforms() — compile a pair of ADR-0017 mapping objects into typed
  (toCommon, fromCommon) callables with call-time structural validation.
  Optional commonModel Zod schema turns parse failures into PluginError[]
  rather than thrown exceptions.
- TransformResult<T> — unconditional { result, errors } return shape
  (ADR-0022 Decision #7).
- PluginError — structured error class with path / handler / sourceValue /
  cause (ADR-0022 Decision #9). sourceValue and cause are stored
  non-enumerable; toJSON() omits them so JSON.stringify(err) is PII-safe.
  console.log(err) / util.inspect(err) still render [cause] via Node's
  default Error inspection — README PII callout calls this out and
  recommends a redacted projection. Zod-validation message is also
  data-bearing because Zod's default error map embeds rejected values;
  full-message sanitization tracked under #744.
- transformFromMapping(), getFromPath(), DEFAULT_HANDLERS — mapping
  runtime; six built-in handlers (const, field, match, switch alias,
  numberToString, stringToNumber).
- definePlugin() accepts optional meta and transformSchemas. Existing
  callers passing only `extensions` are unaffected.

Security hardening (mapping JSON may be reconstituted from untrusted
sources via mergeExtensions(), so the runtime must fail loud on hostile
shapes):

- buildTransforms() rejects custom handler names that collide with the
  default registry or shadow Object.prototype keys (constructor, toString,
  __proto__, etc.) at call time.
- validateMapping() rejects `__proto__` as an output field name at build
  time; transformFromMapping() rejects it again at walk time so the JSON
  attack vector (own-enumerable __proto__ key from JSON.parse) fails fast
  in both places. Shared DEFAULT_MAX_TRANSFORM_DEPTH = 500 across both
  walkers so adversarial mapping JSON can't pass build-time validation
  only to blow the stack at runtime.
- transformFromMapping() scrubs top-level own `__proto__` from
  plain-object handler returns (defense-in-depth: const / field / match
  can return JSON.parse-loaded objects with own __proto__ keys, and a
  downstream for-in deep-merge of the result would otherwise pollute
  Object.prototype).
- getFromPath() uses Object.prototype.hasOwnProperty.call rather than
  `in` so attacker-controlled field paths cannot resolve to
  prototype-chain properties.
- stringToNumber's error message does not embed the source value (would
  flow into PluginError.message and bypass the sourceValue PII guard).
- Handler JSDoc documents two contracts custom-handler authors must
  respect: don't return objects with a `__proto__` key (the walker
  treats handler return values as opaque beyond the top-level scrub),
  and don't throw Errors with PII in .message (it flows verbatim into
  PluginError.message, which is rendered by Node Error inspection).

Cross-SDK parity:

- switchOnValue mirrors Python's `lookup.get(val, default)` exactly:
  only string source values are candidate keys; numeric / boolean / object
  source values short-circuit to default. Pinned by parity tests.
- ClientConfig parity export added (Python `__init__.py` exports it as
  `dict[str, Any]`).
- Six placeholder type exports (ObjectSchemas, ObjectMappings,
  PluginExtensions, PluginExtensionsObjectConfig, PluginCapability,
  PluginMeta) define the ADR-0022 contract surface and parallel the
  Python PoC's extensions/__init__.py exports.

Documented divergences from ADR-0022's TS code blocks (justified in
findings.md):

- DefinePluginOptions.transformSchemas (not `schemas`) avoids collision
  with the existing Plugin.schemas field. Resolution deferred to #756.
- BuildTransformsOptions.commonModel uses
  `z.ZodType<TCommon, z.ZodTypeDef, any>` to admit schemas with
  input/output asymmetry (e.g. .transform() producing Date from string).
  ZodType<TCommon> would reject the SDK's own OpportunityBaseSchema.

Out of scope (matches Python PoC; deferred to full SDK):

- Auto-generation of transforms from declarative
  extensions.schemas[obj].mappings inside definePlugin() (Decision #6 TODO).
- Always-on commonModel validation inside definePlugin() — opt-in at
  buildTransforms() for now (Decision #7 TODO).
- Sanitizing Zod's default error map output before it lands in
  PluginError.message — tracked in #744.
- Defensive output-key rejection of __defineGetter__ / __defineSetter__ /
  constructor / toString — tracked in #744.
- Recursive (nested) sanitization of __proto__ in handler return values —
  tracked in #744 alongside other Decision #8 hardening.

Includes:
- examples/transforms.ts round-trip (`pnpm example:transforms`)
- README "Plugin transformations" section + API reference table
- 14 define-plugin specs, 35 transformation-handler specs,
  19 buildTransforms specs (439 tests across 24 suites, all passing)
- Minor changeset bump for @common-grants/sdk

Targets HOLD-transforms per the SDK Plugin Enhancements branching strategy.
Two foot-guns flagged in code review on PR #825:

1. Integer-shaped strings beyond Number.MAX_SAFE_INTEGER silently lost
   precision via Number(s) (e.g. "9999999999999999999" → 1e19). Plugin
   authors round-tripping 64-bit IDs through stringToNumber would have
   seen silent corruption. Guard the integer branch with
   Number.isSafeInteger and throw on overflow; callers needing arbitrary
   precision should declare the field as a string or write a custom
   handler returning BigInt.

2. Empty and whitespace-only strings coerced to 0 via Number("") /
   Number("  "), turning an implicit-absent CSV cell into a real zero on
   the transformed side. Explicit s === "" check after .trim() throws on
   both cases.

Docstring rewritten to document both divergences from Python's int()
alongside the existing "42.0" decimal-fallback note. Six new tests pin
the boundaries (MAX/MIN_SAFE_INTEGER accepted, beyond-safe rejected on
both signs, empty and whitespace-only rejected).

pnpm run checks clean; pnpm test 443/443; example:transforms round-trip
still verifies (legacy ID 12345 well within safe-integer range).
The runtime walker is first-key-wins, so `{ field: "x", const: "y" }`
would silently drop `const`. Almost always an author bug. Match Python
PoC `_validate_mapping` and fail loud at buildTransforms() call time.
The low-level transformFromMapping walker keeps lenient behavior so
programmatic users composing partial mappings aren't forced into the
strict shape.
Two behavior changes:

- PluginError: drop non-enumerable + toJSON() hardening to match
  ADR-0022 Decision #9 ("the SDK does not redact by default").
  README + JSDoc document the adopter-supplied redacted projection
  as the supported PII-safety path. Existing PII test inverted.
- switchOnValue: throw on non-object spec instead of silent undefined.
  Walker wraps as HandlerError → PluginError(handler: "match").

JSDoc / comment fixes: reframe "Mirrors Python PoC" claims that were
factually wrong (Python's _validate_mapping doesn't reject siblings;
Python has no __proto__ defenses; lookup.get accepts non-string val)
as honest "TS-only hardening; cf. #810 for parallel Python proposal."
Complete @throws lists on buildTransforms and transformFromMapping.
Document stringToNumber null/undefined return. Correct misleading
"re-exported" wording on DEFAULT_MAX_TRANSFORM_DEPTH. Correct
"trimmed to match Python's __init__.py" comment on the barrel
(TS exports a superset).

Test gap coverage (ADR-0022 Decision #8): getFromPath prototype-chain
safety; handler-shadow rejection for __proto__/toString/default-name
collisions beyond \`field\`; build-time __proto__ rejection in
validateMapping; fromCommonMapping depth-cap.

README + CI: add missing ClientConfig and TransformFromMappingOptions
rows to the API table; chain example:transforms into pnpm run ci as
a smoke step.

Simplify: extract deepMapping(levels) helper; PII test suite uses
beforeEach so both tests assert on the same PluginError instance.

Verification: pnpm run ci passes end-to-end (445 → 449 tests).

Cross-SDK follow-ups (file against #810 / amend ADR-0022 separately):
TS-only sibling-key rejection, __proto__ defenses, switchOnValue
non-object throw, stringToNumber strictness, handler-shadow rejection;
and the ADR-0022 Decision #9 default-redaction question itself.
Python's `_validate_mapping` accepts handler-dispatch nodes with sibling
keys; the TS `validateMapping` was rejecting them at build time, creating
the only genuine "TS adds mandatory validation Python doesn't" divergence
in this PR. The first-key-wins walker behavior already matches Python, so
both SDKs now share the same foot-gun: `{ field: "x", const: "fallback" }`
silently drops `const` in both.

Replaces the two rejection tests with one parity-pinning test
(`accepts (does not reject) sibling keys ... cross-SDK parity`) that
asserts `.not.toThrow()` — locks the decision so a future regression
re-introducing build-time rejection (and re-creating the divergence)
fails loudly.

Updates `validateMapping` JSDoc to call out the parity decision and
`transformFromMapping` JSDoc to drop the (now false) reference to a
stricter build-time check.

If we later want both SDKs to fail loud on this shape, the cleanest path
is to add the check to Python first and then re-introduce TS — keeps
divergence at zero.

Verification: pnpm run ci passes end-to-end (449 → 448 tests; -2 removed
rejection tests, +1 added parity-pinning test).
@SnowboardTechie SnowboardTechie force-pushed the bryan/issue-798-transforms-poc-typescript branch from 1897cb1 to 3e557b9 Compare May 21, 2026 18:18
Three findings from a /review pass over the PoC. All three tighten
existing contracts; none introduce new shapes.

- switchOnValue: reject arrays as spec. Arrays pass `typeof === "object"`
  and non-null but lack the structural shape — a mapping like
  `{ match: ["posted", "archived"] }` would otherwise silently resolve
  to s.field/s.case/s.default all-undefined and return undefined.
  Same fail-loud direction as the existing non-object guards.

- __proto__ scrub: shallow-clone on the pollution path rather than
  `delete`-in-place. `fieldValue` returns references plucked from caller
  input via `getFromPath`, so an in-place delete would silently mutate
  the caller's data — surprising for plugin authors caching parsed
  source records across `toCommon` calls (common in long-running adapter
  processes and multi-tenant deployments). Spread is the correct copy
  primitive here: CopyDataProperties -> CreateDataProperty bypasses the
  prototype setter. Object.assign would mutate the target's prototype
  chain instead — the source comment now warns against the regression.

- README error-handling snippet: log a named redacted projection rather
  than building the message via template-string interpolation. Makes
  the "what gets logged" surface a single audit point and matches the
  projection in the prose PII callout and in the redacted-projection
  test verbatim.

Verified: pnpm --filter @common-grants/sdk run ci passes (449 tests
across 24 suites, +1 source-preservation test; example:transforms
round-trip OK).
@SnowboardTechie SnowboardTechie removed python Issue or PR related to Python tooling py-sdk Related to Python SDK dependencies Pull requests that update a dependency file labels May 21, 2026
Mirror the Python PoC PR #838 commit a156d31 so plugin authors add a
single per-object entry under transformSchemas[Object] when introducing
a new object, rather than splitting customFields across a separate
top-level extensions dict.

- types.ts: add customFields?: Record<string, CustomFieldSpec> to
  ObjectSchemasInput; drop customFields from PluginExtensionsObjectConfig
  (now mappings-only, matching Python's PluginExtensionsSchema).
- define-plugin.ts: make extensions optional in DefinePluginOptions and
  Plugin; definePlugin() sources customFields from
  transformSchemas[name].customFields first, falling back to the legacy
  extensions[name] surface so existing customFields-only plugins keep
  working.
- examples/transforms.ts: consolidate to a single transformSchemas entry
  carrying customFields + toCommon + fromCommon together.
- tests: cover the consolidated path (customFields on transformSchemas[obj]),
  no-extensions-arg case, and transformSchemas-wins-over-extensions priority.

Open question — ADR-0022 as written places customFields inside
PluginExtensions.schemas[obj] specifically so declarations can be
combined across packages via mergeExtensions() (Decision driver line 23,
Decision #4). This commit follows Jeff's Python move out of that
serializable surface, which drops customFields from the cross-package
merge contract. Pending an ADR-0022 amendment formalizing the trade-off;
flagged inline on ObjectSchemasInput and PluginExtensionsObjectConfig.

Type inference for plugin.schemas[obj] still flows through the legacy
extensions parameter — the runtime applies customFields from either
source, but the compiled-schema type only narrows when customFields are
declared via extensions. Wiring the generic through transformSchemas is
a follow-up (parallel to the transformSchemas → schemas rename deferred
to #756).
@github-actions github-actions Bot added the dependencies Pull requests that update a dependency file label May 21, 2026
Preserve explicit `null` source values ("publisher asserts doesn't apply")
across `toCommon` / `fromCommon` instead of collapsing them to `undefined`
("not provided"). Without this, ADR-0024's third state would be unobservable
through the transforms layer — the ADR audited the Zod/Pydantic validation
surface and explicitly left SDKs untouched, but the handler layer is
downstream of that.

Handler changes (lib/ts-sdk/src/extensions/transformation.ts):
- numberToString / stringToNumber: null source -> null output (was undefined).
  Return types widen to string | null | undefined / number | null | undefined.
- switchOnValue: null source passes through as null by default; mapping
  authors opt in to target-side translation via a `case: { "null": ... }`
  key. `default` is NOT consulted for null source (default is for
  unrecognized values, not publisher assertions).
- field / getFromPath: unchanged — already preserves terminal null.
  JSDoc updated to document intermediate-null behavior as scoped-out
  ("parent N/A propagates to child paths").
- The walker places handler-returned null onto the output object as a real
  null (distinct from an absent key), so the three-state distinction
  survives end-to-end.

Docs:
- New "Null handling" subsection in src/extensions/README.md documenting
  the three-state contract for custom-handler authors.
- API reference table updated to reflect the new return semantics.
- Module-level JSDoc on transformation.ts documents the contract and the
  cross-SDK divergence (TS leads, Python PoC #810 parity tracked there).

Example (examples/transforms.ts):
- Source record now includes `source_url: null` demonstrating the publisher's
  "doesn't apply" assertion.
- Round-trip spot-check fails if a future regression coerces it back to
  undefined.

Tests:
- Existing "returns undefined on null or missing" tests split into two
  for both coercing handlers (null -> null; absent -> undefined).
- New switchOnValue coverage for null pass-through, opt-in "null" case-key
  precedence, and the no-default-for-null rule.
- New walker test pinning that handler-returned null lands on the output.
- New buildTransforms round-trip + commonModel Zod assertion using `source`
  (the .nullish() field on OpportunityBaseSchema).
- fieldValue regression pin for terminal-null preservation and intermediate-
  null collapse.

466 tests pass (was 452, +14). pnpm run checks / build / example:transforms
all clean.
Code/runtime behavior unchanged — these are JSDoc, README, and example-comment
fixes for code-vs-docs drift that accumulated across review iterations.

- `PluginError` JSDoc: rename the "Recommended redacted projection" snippet to
  "partial-redaction pattern" with an inline CAUTION on the data-bearing
  `message` field. The previous label invited copy-paste that contradicts the
  warning paragraph immediately below (Zod's default error map embeds runtime
  values into `issue.message`, which flows verbatim into `PluginError.message`).
- `Handler` JSDoc: remove stale reference to `PluginError.toJSON()`. The
  toJSON()/non-enumerable hardening was removed earlier in this PR for ADR-0022
  Decision #9 alignment; the JSDoc still described a protection mechanism that
  no longer exists. Replace with a pointer to the README PII warning and to
  `stringToNumber`'s generic-message pattern.
- `Handler` JSDoc: note the cosmetic ADR-0022 parameter-name divergence
  (`(data, arg)` vs ADR's `(value, context)`) so a custom-handler author
  reading the ADR interface block doesn't trip on the rename.
- `switchOnValue` and `DEFAULT_HANDLERS` JSDoc + README handler table: drop
  the phantom "ADR-0022 Decision #3" citation. Decision #3 is about per-object
  schema grouping, not handler naming. The `switch` alias is a convenience —
  no prior SDK API defined `switch`, so there is no backward-compatibility
  obligation. Cross-SDK parity rationale (if any) tracked separately.
- README API reference table: update `ObjectSchemasInput` row to include
  `customFields?` (added in d9d9bb2) and update `PluginExtensionsObjectConfig`
  row to drop `customFields?` (moved out in the same commit). Both rows had
  drifted from the implementing interfaces.
- `examples/transforms.ts`: add PII-warning comments on the two error-path
  `e.message` formatting blocks. Safe in this example (fixed PII-free source
  data) but adopters copy this shape into their own pipelines.

Checks:
- `pnpm run check:lint` — clean.
- `pnpm run check:format` — clean (prettier --write applied to README after
  the table-row edit shifted column widths).
- `pnpm run check:types` — clean.
- `pnpm run test` — 466 / 466 pass across 24 suites.
- `pnpm run example:transforms` — round-trip + ADR-0024 three-state spot-check
  both pass.
…tate (ADR-0024)

Resolves the walker/doc/test mismatch flagged in self-review (Important #1).

Behavior change (transformFromMapping output-shape branch): a child that
transforms to `undefined` (absent source — ADR-0024 "not provided") is now
omitted from the output object instead of written as `out[k] = undefined`.
`null` ("doesn't apply") is still written as a present key. So the in-memory
object now distinguishes all three states the same way the wire does:
absent → key omitted, `null` → present `null`, value → present value.

Why this shape:
- The wire output is unchanged — `JSON.stringify` already dropped
  `undefined`-valued keys, so absent→missing on the wire was already correct.
  This only fixes the in-memory representation to match.
- Makes the in-memory object a faithful three-state carrier: consumers can
  use key presence (`hasOwnProperty` / `in`) to tell "not provided" from
  "doesn't apply", which a present-`undefined` key made impossible.
- Aligns with the dict shape the Python SDK lands on once it adopts the
  three-state contract: Python has no `undefined`, so an absent field is a
  missing dict key there. (Python's PoC currently collapses absent+null into
  `None` — known non-conformant, per the cross-SDK note in the README.)

Doc/test corrections that depended on the old behavior:
- README "Null handling": the "distinct from an absent key" claim is now
  literally true; reworded to state the absent→omitted / null→present rule
  and that key presence is the probe.
- transformFromMapping JSDoc: output-shape bullet documents undefined-omission.
- transforms.spec.ts "distinguishes absent from null": the test comment
  claimed an absent output key but only asserted `toBeUndefined()` (satisfied
  by a present-undefined key too). Added `hasOwnProperty` assertions that pin
  the real contract — absent key for absent source, present key for null.
- transformation.spec.ts: new low-level test asserting an undefined-returning
  handler omits the key while a sibling present key is still written.

Checks: lint / prettier / tsc clean; test 467/467 (was 466, +1 walker test);
example:transforms round-trip + source_url:null three-state spot-check pass.
The "Null handling" cross-SDK note claimed the Python ADR-0024 parity
follow-up was "tracked there" and linked #810. Reading #810 directly (body +
diff) confirms it is the base Python Transform PoC (closes #799, merged
2026-05-13), scoped "to the transform layer only" with no null/three-state
content — its coercing handlers collapse absent and null into `None`
(`str(val) if val is not None else None`; `if val is None: return None`).
ADR-0024 (PR #855) was created and merged 2026-05-26, 13 days after #810, so
#810 neither did the parity work nor tracks it. No dedicated Python-parity
issue exists.

Reword: drop the false "tracked there" claim, link ADR-0024 directly as the
contract Python must meet, state plainly that Python predates the ADR and that
handler parity is a pending follow-up. No tracker is asserted.

Docs-only; prettier clean.
…rub inside try)

The walker's __proto__ scrub on a handler's return value ran *outside* the
handler try/catch. A custom handler returning a trap-throwing exotic value
(e.g. a Proxy whose getPrototypeOf/hasOwnProperty trap throws, or one that
throws on spread) made the scrub throw, which escaped as an unattributed
PluginError via runMapping's generic catch instead of HandlerError.

Not a security issue — handler code is trusted (author-supplied, registered
per buildTransforms call); ADR-0022 Decision #8's threat model is untrusted
mapping JSON, not handler code. The failure was already contained (PluginError
in errors[], no crash/pollution). This only restores handler attribution on
that exotic path.

Fix: move the scrub inside the existing handler try/catch. The scrub operates
on the handler's return value, so a throw there is correctly attributed to the
handler via HandlerError -> PluginError(handler). Added a comment marking the
placement as deliberate ("don't hoist it out") and a test: a handler returning
a Proxy with a throwing getPrototypeOf trap now yields PluginError(handler).

Checks: lint / prettier / tsc clean; test 468/468 (+1); example:transforms passes.
The 3e557b9 revert removed TS's build-time sibling-key rejection on the
premise that Python's `_validate_mapping` accepts the shape. It does not:
`build_transforms` -> `_validate_mapping` (python-sdk extensions/transforms.py)
raises on a handler key with sibling keys, and has since the Python PoC (#810).
The revert conflated Python's lenient runtime walker (transform_from_mapping,
first-key-wins) with its strict build-time validator, so it created the
cross-SDK divergence it claimed to remove.

Re-instates build-time rejection in validateMapping (mirroring Python),
restores the two rejection tests, and corrects the now-false JSDoc in
transforms.ts and transformation.ts. The low-level transformFromMapping
walker stays lenient, matching Python's runtime walker.

Verified: check:types + prettier + build + vitest (469 passed).
Pointers cost per occurrence and rot at different rates; keep the inline
reasoning, thin the citations.

- Remove all `#838` / commit `a156d31` references (unmerged-branch pointers
  that describe Python code not on the base branch). Replaced with
  "pending ADR-0022 amendment" framing that states the trade-off in its
  own terms.
- Collapse the ADR-0024 three-state explanation to one canonical block in
  transformation.ts + the README; handlers now note only their own
  departures instead of re-deriving the contract.
- Remove the 4 duplicated per-handler "Python #810 parity" notes; keep the
  statement once in the README (#810 is merged, durable).
- Thin repeated `(ADR-0022 Decision #8)` prototype-hardening tags in the
  walker; the security reasoning stays at every guard, one ADR anchor kept.

Comment/doc-only; no behavior change. tsc/eslint/prettier clean,
extension tests pass, example:transforms round-trips.
… framing

The "pending amendment / open question" language asserted a governance
artifact that exists nowhere: ADR-0022's doc never mentions it, no issue
tracks it, and the only files in the repo referencing an "ADR-0022
amendment" were this PR's own. Same failure mode as the #838 pointer —
a reference to something that isn't real.

Replace with the verifiable behavior + trade-off: customFields can live
on transformSchemas[Object] (preferred) or the legacy `extensions`
surface (kept for mergeExtensions cross-package composition);
transformSchemas wins when both are set. State the trade-off, drop the
speculative future-ADR claim.

Comment/doc-only. tsc/prettier clean, 159 extension tests pass.
The first pass cherry-picked the loudest offenders and left the broader
"talks a lot about other issues/PRs/ADRs" problem (originally flagged on
define-plugin.ts) largely intact. This applies one policy uniformly across
all PR files:

- ADR-0017: one canonical mention (README status line); elsewhere
  "mapping objects" stands alone.
- ADR-0022 "Decision #N" tags: keep the reasoning; one anchor each for the
  two security decisions (#8 prototype guard in getFromPath, #9 PII in the
  README warning). Drop the rest, including in test names.
- "full SDK will… (tracked under #756)": state PoC scope plainly; #756 kept
  once (define-plugin.ts).
- Cross-SDK Python-parity pointers ("Mirrors the Python SDK's X", parity
  export, superset-of-__init__, .-join-matches-Python): consolidated to the
  one README cross-SDK note. Behavioral divergence docs (int(s) semantics,
  safe-integer rationale) kept — they describe this code.
- #798/#810: one mention each (README/changeset opening).
- Test names/comments: bare ADR/Decision numbers stripped, behavioral
  phrasing kept ("doesn't apply", "hardening", sibling-key "Python PoC
  parity"). De-referenced a rot-prone transforms.ts:218 line pointer.

Comment/doc-only; no behavior change. tsc/eslint/prettier clean, 159
extension tests pass, example:transforms round-trips.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli Issue or PR related to the @common-grants/cli library core Issues related to @common-grants/core library dependencies Pull requests that update a dependency file enhancement New feature or request sdk Issue or PR related to our SDKs ts-sdk Related to TypeScript SDK typescript Issue or PR related to TypeScript tooling website Issues related to the website

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant