Enable unchecked indexed access checks#415
Conversation
# Conflicts: # src/functions/OpenCapTable/capTable/CapTableBatch.ts
# Conflicts: # test/declarations/publicApi.types.ts
# Conflicts: # src/functions/OpenCapTable/issuer/createIssuer.ts
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change enables ChangesIndexed access and prototype safety hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
CI is green on the latest head, including the full OCP QuickStart integration suite. @copilot review |
|
✅ Action performedReview finished.
|
|
@coderabbitai review Please run one substantive review of exact current head |
|
✅ Action performedReview finished.
|
|
@coderabbitai review The exact adaptive-limit window shown on the prior sticky has fully elapsed. Please now run and publish the complete substantive pipeline for exact current head |
|
Please note: CodeRabbit is an incremental review system and does not re-review commits that have already been reviewed. If this exact commit was already reviewed in a prior pass (as the chain indicates), there won't be a new incremental review to publish unless automatic reviews were paused or new changes are pushed. If you'd like a full re-analysis regardless of prior incremental state, use ✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/converters/warrantIssuanceConverters.test.ts`:
- Around line 700-711: Update the test setup around the existing trigger
extraction so the already-extracted single trigger `trig` is used directly for
`conversion_right`, rather than passing it to `requireFirst`. Keep
`requireFirst` only on the original trigger collection and preserve the
subsequent tagged-enum assertions.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fb7f30c5-556d-4279-beec-e6e16cbf488b
📒 Files selected for processing (43)
scripts/audit-ocf-schema-alignment.tsscripts/optimize-fixtures.tsscripts/prepare-release.tssrc/functions/OpenCapTable/capTable/archiveFullCapTable.tssrc/functions/OpenCapTable/capTable/getCapTableState.tssrc/functions/OpenCapTable/valuation/damlToOcf.tssrc/functions/OpenCapTable/vestingTerms/getVestingTermsAsOcf.tssrc/utils/cantonOcfExtractor.tssrc/utils/ocfMetadata.tssrc/utils/ocfZodSchemas.tssrc/utils/planSecurityAliases.tssrc/utils/replicationHelpers.tssrc/utils/requireDefined.tssrc/utils/transactionHelpers.tstest/batch/CapTableBatch.test.tstest/batch/damlToOcfConverters.test.tstest/batch/damlToOcfDispatcher.test.tstest/batch/remainingOcfTypes.test.tstest/capTable/archiveFullCapTable.test.tstest/capTable/getCapTableState.test.tstest/converters/convertibleIssuanceConverters.test.tstest/converters/stockClassConverters.test.tstest/converters/valuationVestingConverters.test.tstest/converters/warrantIssuanceConverters.test.tstest/createOcf/falsyFieldRoundtrip.test.tstest/integration/entities/exerciseConversionTypes.integration.test.tstest/integration/entities/valuationVesting.integration.test.tstest/integration/production/productionDataRoundtrip.integration.test.tstest/integration/setup/contractDeployment.tstest/integration/setup/integrationTestHarness.tstest/integration/workflows/capTableWorkflow.integration.test.tstest/schemaAlignment/dateBoundaryInvariants.test.tstest/schemaAlignment/enumAlignment.test.tstest/scripts/auditOcfSchemaAlignment.test.tstest/utils/ocfMetadata.test.tstest/utils/ocfZodSchemas.test.tstest/utils/planSecurityAliases.test.tstest/utils/replicationHelpers.test.tstest/utils/transactionHelpers.test.tstest/utils/transactionSorting.test.tstest/validation/boundaries.test.tstest/validation/generatedDamlBoundary.test.tstsconfig.json
💤 Files with no reviewable changes (1)
- src/functions/OpenCapTable/vestingTerms/getVestingTermsAsOcf.ts
|
Ready for final human review on exact head Final gate evidence:
Assigned to @HardlyDifficult for final review. No standing +1 reaction is present, so this has not been merged. |
|
Readiness retracted for the DAML-validation boundary cleanup. This PR will be rebuilt without the abandoned upstream runtime-domain validation, then revalidated and freshly reviewed on its exact resulting head before merge. |
|
Pivoted and restacked on merged #412. Exact PR head 06a8c00 contains only the clean noUncheckedIndexedAccess hardening slice; SDK-side business validation remains excluded. Local build, strict declarations, typecheck, 2,777 tests, lint, formatting, and diff checks pass. Keeping this draft until exact-head CI and automated reviews complete. |
Reviewed exact head |
|
@coderabbitai review Please perform a substantive review of exact head 06a8c00, focusing on noUncheckedIndexedAccess soundness and the DAML-responsibility pivot. |
|
✅ Action performedReview finished.
|
|
Addressed both exact-head Copilot findings in 4e1d322: string-keyed valuation and schema-alias maps now require own properties, and the valuation path has a prototype-key regression test. Build, declarations, typecheck, all 2,778 tests, lint, focused formatting, and diff checks pass locally. PR remains draft for new exact-head CI. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 64 out of 65 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/utils/transactionHelpers.ts:29
safeGetstill useskey in currand then readscurr[key], which will treat inherited/prototype properties (e.g.toString) as present. This reintroduces the prototype-key hazard that the PR is otherwise hardening against. Use an own-property guard (Object.prototype.hasOwnProperty.call) for each path segment and keep traversal typed asunknownso primitives can be returned at the leaf.
Reviewed exact head |
|
@coderabbitai review Please perform a substantive review of corrected exact head 4e1d322, including the prototype-chain fixes and DAML-responsibility scope. |
|
Please note: CodeRabbit is an incremental review system and does not re-review commits that have already been reviewed. If this exact commit was already reviewed in a prior pass, there won't be a new incremental review to publish unless automatic reviews were paused or new changes were pushed since. If you'd like a full re-analysis regardless of prior incremental state, use ✅ Action performedReview finished.
|
|
Also addressed the suppressed prototype-chain note from the formal Copilot review in 3f10aa3: safeGet now traverses own properties only while keeping the leaf typed as unknown, with direct and inherited-path regression tests. Typecheck, focused tests, lint, formatting, and diff checks pass. Draft exact-head CI is running again. |
| const cliffPortion = requireDefined(conditions[1], 'cliff vesting condition').portion; | ||
| const monthlyPortion = requireDefined(conditions[2], 'monthly vesting condition').portion; | ||
| expect(cliffPortion).toBeDefined(); | ||
| expect(cliffPortion === undefined ? true : 'remainder' in cliffPortion).toBe(false); | ||
| expect(monthlyPortion).toBeDefined(); | ||
| expect(monthlyPortion === undefined ? true : 'remainder' in monthlyPortion).toBe(false); |
Reviewed exact head |
Summary
Why
Index access previously produced unsound non-optional values even when a key or array element could be absent. This change makes those uncertainty points visible to TypeScript and resolves them at the nearest boundary.
The runtime checks retained here are limited to configuration, decoded/transport shape, and SDK-internal invariants. Business correctness remains the DAML contracts responsibility.
Validation
Validated locally on the clean tree based on merged main:
Note
Medium Risk
Broad touch across OCF read paths and cap-table classification with new throws and date fallbacks; behavior should match prior intent but warrants regression on replication and extraction flows.
Overview
Turns on
noUncheckedIndexedAccessintsconfig.jsonand updates the SDK, scripts, and tests so array/object indexing is explicitly narrowed instead of assumed present.Adds
requireDefined/requireFirstfor script and test boundaries, and applies that pattern (plus??fallbacks onsplit('T')[0]date trimming) across OpenCapTable DAML→OCF converters and related utilities.Hardens own-property lookups where
inwas unsafe: replication entity maps (getOwnEntityType), valuation enum maps,safeGet, schema alias resolution, and issuer contract iteration incantonOcfExtractor.classifyIssuerCapTablesnow throwsSCHEMA_MISMATCHif a “non-empty” query yields no first row; archive matching uses explicit single-element checks.Tests cover inherited keys like
toStringand addtransactionHelpers.safeGetcoverage;ocfMetadata.ocfIdPathis typed as a readonly tuple path.Reviewed by Cursor Bugbot for commit 3f10aa3. Bugbot is set up for automated code reviews on this repo. Configure here.