Enforce exact optional source boundaries#418
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
CI is green on the latest head, including the full OCP QuickStart integration suite. @copilot review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR tightens TypeScript “exact optional” object-shape boundaries across production src by omitting absent optional fields (instead of materializing undefined), and introduces a permanent exactOptionalPropertyTypes compile gate for production source.
Changes:
- Add
tsconfig.exact-source.jsonand wiretest:exact-sourceintonpm run typecheckto enforce zeroexactOptionalPropertyTypesdiagnostics insrc. - Refactor multiple boundary objects (read scope, telemetry/template identity, OCF converters) to conditionally omit optional keys rather than assigning
undefined. - Make template identity comparison a success/failure discriminated union and strengthen unknown wire-value validation for stock issuance type.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.exact-source.json | Adds a dedicated exact-optional compile gate for src/**/*.ts. |
| package.json | Wires the new exact-source gate into typecheck. |
| src/observability.ts | Adjusts command context application typing/shape to avoid undefined optional assignments. |
| test/observability/observability.test.ts | Removes an unsafe cast and exercises the updated observability helper typing. |
| src/utils/templateIdentity.ts | Introduces a discriminated union for template identity comparison and omits absent optional fields in error context. |
| src/utils/cantonOcfExtractor.ts | Routes read scope through ledgerReadScope without materializing undefined and threads read scope through extraction. |
| src/functions/OpenCapTable/shared/singleContractRead.ts | Omits absent template identity fields in read results and in template identity validation inputs. |
| src/functions/OpenCapTable/capTable/getCapTableState.ts | Omits absent readAs instead of passing undefined. |
| src/functions/OpenCapTable/capTable/CapTableBatch.ts | Avoids passing undefined telemetry/templateId fields and adjusts context construction. |
| src/functions/OpenCapTable/capTable/archiveCapTable.ts | Omits absent optional command fields in command construction/telemetry. |
| src/functions/OpenCapTable/vestingTerms/getVestingTermsAsOcf.ts | Omits absent optional OCF vesting period fields rather than emitting undefined. |
| src/functions/OpenCapTable/stockPlan/getStockPlanAsOcf.ts | Omits absent optional OCF stock plan fields; normalizes optional cancellation behavior handling. |
| src/functions/OpenCapTable/stockIssuance/getStockIssuanceAsOcf.ts | Validates unknown issuance type wire values and omits absent optional issuance type. |
| src/functions/OpenCapTable/issuer/getIssuerAsOcf.ts | Tightens helper return types for optional issuer contact fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Current head a9da845 is green and merge-clean; it preserves complete caller parameter subtypes through applyCommandContext and reuses a single read-scope value. All prior review threads are addressed and resolved. @copilot review |
|
cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a9da845. Configure here.
… into codex/exact-optional-boundaries
… into codex/exact-optional-boundaries
… into codex/exact-optional-boundaries
|
cursor review |
|
@copilot review |
|
CodeRabbit exact-head review is intentionally deferred until the current organization-wide rate-limit window expires; this PR remains unassigned Stage 2. |
|
Material parent integration and command-context soundness fix are complete at exact head ac8df1f. Final #417 head 0aa47b1 was merged normally with no history rewrite. AppliedCommandContext now exposes the canonical plain ledger-submit shape with omission-only, readonly command-context fields. Runtime merging normalizes and snapshots params context through #417 immutable machinery, preserves params < default < per-call precedence, adds context fields to the public result, and no longer promises arbitrary prototype or non-enumerable members through an object spread. Source, built declaration, exact-optional, and runtime regressions cover the contract. Validation is green: focused observability/OcpClient/authorization 3 suites / 248 tests; full 73 suites / 3,737 tests; build; declarations; exact-source and exact-public-config compilers; regular typecheck; ESLint with zero warnings; Prettier; and diff checks. This PR remains draft and unassigned; no automated reviews or downstream propagation were requested. |
|
Independent exact-head audit follow-up is pushed at The audit found and fixed two soundness gaps before automated review:
Regressions cover source and built declarations plus prototype-getter/non-enumerable runtime inputs. Precedence and immutability remain params < default < per-call, explicit undefined skips rather than clears, snapshots are detached/frozen, and custom prototype helpers are not promised. Validation on this exact revision: focused observability 8/8; full 73 suites / 3,738 tests; build; typecheck and exact source; declarations and exact built; lint; format; diff check; and a manual built-package adversarial runtime check. The PR remains draft and unassigned while exact-head CI and QuickStart run. |
|
Fresh exact-projection audit hardening is pushed at exact head The audit reproduced one projection root cause: spread-based copying silently dropped valid optional Canton fields supplied through prototype getters/non-enumerable properties, while leaking unrelated enumerable helper/caller fields into the supposedly canonical ledger submit object. The result is now built from a compile-time-exhaustive list of every required and optional Validation on this exact revision: focused observability/type gates; full 73 suites / 3,738 tests; build; typecheck; declarations; exact source+built checks; lint; format; and a built adversarial projection probe. The PR remains draft and unassigned. Its base #417 subsequently advanced, so this head is intentionally awaiting a conflict-aware parent merge; no reviews were requested. |
|
Corrected stack propagation complete at exact head Merged corrected #417 ( Validation:
All local checks pass. PR remains draft and unassigned while CI and downstream propagation run. |
…act-optional-boundaries
|
Re-propagated corrected PR #417 head Preserved #418 exact optional boundaries and plain applied-command-context semantics together with the corrected full-payload PlanSecurity pricing delegation and six-variant Monetary parity. Validation is green locally: 5 focused suites / 183 tests; full coverage run 73 suites / 4,047 tests; clean build; typecheck; declaration, exact source, and exact public-config tests; lint; Prettier; and diff checks. This PR remains draft and unassigned pending downstream propagation and fresh exact-head reviews. |
|
@copilot review Please review exact current head |
Reviewed exact head |
|
@cursor review Please review exact current head |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 199d2b9. Configure here.
|
Returned to draft because upstream #417 received a material exact-head corrective revision after full CodeRabbit review. This stacked PR will remain held until the audited #417 head and each intervening parent are propagated normally; fresh CI and Copilot-first reviewer gates are required afterward. |
|
Updated this draft to exact upstream #417 head The integration preserves omission-only optional fields while retaining #417’s exact command snapshots, field-wise trace-context precedence, and disclosed-contract handling. It also adds an exhaustive nested submit A clean-checkout audit exposed and I fixed two additional blockers before pushing: an impossible |
|
Follow-up hardening is now on exact head The applied submit boundary now rejects Proxy, accessor, custom-prototype, and symbol-key carriers without invoking traps; reads canonical values only from captured own data descriptors; rejects explicit Local validation on this exact revision: build; typecheck/exact-source; declaration/public-config gates; lint; Prettier; |
… into codex/exact-optional-boundaries
|
Restacked this draft onto corrected #417 head The independent audit’s final parity finding is also fixed: source and built declaration fixtures now use valid plain own-data submit carriers, and Local exact-head validation after propagation passes: lint, Prettier, build, typecheck/exact-source, declarations/exact-public-config, |
… into codex/exact-optional-boundaries
… into codex/exact-optional-boundaries
|
Restacked this draft onto corrected #417 exact head |
Summary
exactOptionalPropertyTypesas a permanent production-source gate and eliminate every source diagnosticundefinedwithout invoking trapsAppliedCommandContextcontaining only canonical submit fields with detached immutable trace contextundefinedValidation
Exact head
afb3b58344a696e89212efcee84a9db6df1fc1c2:npm run lintnpm run formatnpm run buildnpm run typechecknpm run test:declarationsnpm run test:ci: 74 suites / 4,082 testsgit diff --checkStack
Depends on #417.