HF-307 PR 2: public API guards (ensureCapability) - #1729
Open
marcin-kordas-hoc wants to merge 4 commits into
Open
HF-307 PR 2: public API guards (ensureCapability)#1729marcin-kordas-hoc wants to merge 4 commits into
marcin-kordas-hoc wants to merge 4 commits into
Conversation
Task 2.1: LicenseCapabilityMissingError in src/errors.ts, mirroring the existing ~30 error classes; private ensureCapability(feature) in HyperFormula.ts mirroring ensureEvaluationIsNotSuspended. Task 2.2: ensureCapability wired as the FIRST statement (before argument validation) in the ~20 methods spec'd for PR 2 - NamedExpressions (addNamedExpression, changeNamedExpression, removeNamedExpression), Clipboard (copy, cut, paste), Crud (addRows, removeRows, addColumns, removeColumns, moveCells, moveRows, moveColumns, addSheet, removeSheet, clearSheet, setSheetContent, renameSheet, setCellContents), UndoRedo (undo, redo), Batching (batch, suspendEvaluation, resumeEvaluation). Read-only accessors (listNamedExpressions, getNamedExpression, getAllNamedExpressionsSerialized) are left ungated, resolving the open "getter scope" question from the handoff: gate B's own precedent already draws this line at mutation vs. read (it blocks calling a function, not reading a cell's existing value), so a restricted entitlement can still see named expressions that already exist. Task 2.3: BuildEngineFactory.ensureNamedExpressionsCapability - same allowsFeature(FeatureId.NamedExpressions) check, applied only when the namedExpressions argument to buildFromSheets/buildFromSheet/buildEmpty (the three factories buildFromArray/buildFromSheets/buildEmpty resolve to) is non-empty. Deliberately not applied to rebuildWithConfig, which re-serializes named expressions an already-built instance was already allowed to create, rather than accepting them fresh from a caller. Every ensureCapability call is a single boolean read (config.isLicenseGateActive) on the fast path, matching gate B's hot-path property; this ships without a real license-key payload adapter (PR 3), so every entitlement Config can produce today is unrestricted and the guard is a correct, independently-testable no-op in production. Found while writing tests: PR 1's licence.spec.ts restrictEngine() test helper granted an empty feature set, which now also blocks the setCellContents calls those tests use to set up their formulas, before gate B ever runs. Fixed by having that helper grant Crud by default - those tests are about gate B's function-level check, not this PR's Crud feature gate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GuUdq242TtFaepKRNdEkj9
Contributor
|
Task linked: HF-107 Import/export files (XLSX, CSV) |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
hyperformula-docs | 88feb29 | Commit Preview URL Branch Preview URL |
Aug 18 2026, 08:56 AM |
Performance comparison of head (88feb29) vs base (0a966e3) |
marcin-kordas-hoc
marked this pull request as ready for review
August 11, 2026 12:34
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6689397. Configure here.
Self-review of the just-opened PR found four public mutating methods that ensureCapability never covered: swapRowIndexes, setRowOrder, swapColumnIndexes and setColumnOrder. They permute sheet structure exactly like moveRows/moveColumns, which were gated - so a restricted entitlement with no Crud grant could still reorder every row and column in a sheet, which defeats the gate for a whole class of structural mutation. Each of the four is gated in its own right rather than relying on the swap* method the set*Order pair delegates to, so the license error still precedes their own argument validation (task 2.2's first-statement rule). Also writes down where the line is drawn, because "we chose not to gate this" was previously indistinguishable from "we forgot this": - gated: mutations that create value (sheet, clipboard, undo history, named expressions) - not gated: reads, and teardown that only removes state (clearClipboard, clearUndoStack, clearRedoStack, destroy) - gating cleanup would strand an integration mid-teardown and give a licensee nothing And records the gate-A asymmetry as an invariant: ensureCapability checks entitlement only, never key validity, which is what preserves today's behaviour where a missing key yields #LIC! in cells but keeps the CRUD API working. A later PR that resolves an invalid key to a restricted rather than unrestricted entitlement would silently turn that into a breaking API change - the note is there so that happens on purpose or not at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GuUdq242TtFaepKRNdEkj9
Cursor Bugbot flagged this on the open PR; verified and fixed. resumeEvaluation is the only exit from a suspended engine, and _evaluationSuspended survives rebuildWithConfig. So an instance suspended while Batching was granted, whose entitlement then loses Batching through updateConfig, was stuck suspended permanently: every read throws EvaluationSuspendedError and the sole recovery path threw LicenseCapabilityMissingError. No public escape. suspendEvaluation and batch stay gated - those are the entry points that make the feature worth licensing, and if you cannot enter batching you can never extract value from it. Gating the release valve only strands the caller, which is the same reasoning that already left teardown (clearClipboard, clearUndoStack, clearRedoStack) ungated. Stated as a rule on ensureCapability so it does not get re-added: a capability check must never be reachable only on the way OUT of a state it let the caller into. Two regression tests cover it (resume works after the grant is revoked; the engine is actually left unsuspended afterwards). Both verified by mutation - re-adding the gate fails them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GuUdq242TtFaepKRNdEkj9
marcin-kordas-hoc
changed the base branch from
develop
to
hf-307-entitlement-gating-pr1
August 13, 2026 13:56
…kaging gap
Fixes three confirmed findings from an independent spec-to-ship review of PR2
(5-dimension multi-agent workflow, adversarially verified):
1. HIGH - hard-gating bypass. paste() checked only FeatureId.Clipboard, but
CrudOperations.paste() dispatches to moveCells() internally when the clipboard
holds a cut - the same cell-relocating mutation the public moveCells() requires
Crud for. A Clipboard-only entitlement could reach it via cut()+paste(), with
no Crud grant ever checked. Fixed by adding a public CrudOperations.isCutClipboard()
wrapper and gating paste() on Crud too when the clipboard holds a cut - still
checked before argument validation, matching every other ensureCapability call.
Verified by mutation: reverting the fix makes the new regression test fail
(paste succeeds and actually moves the cell) with the fix reverted.
2. HIGH - packaging gap. LicenseCapabilityMissingError was never imported/exported
from src/index.ts, so it was unreachable from the package's public entrypoint:
`import {LicenseCapabilityMissingError} from 'hyperformula'` failed to compile
(TS2614), the default-export static form was undefined, and no deep import
worked either (package.json's exports map only defines "." and the i18n
subpaths). A consumer had no supported way to catch this error by type. Fixed
by adding it alongside the other ~30 error classes already exported there.
3. MEDIUM - documentation. Added @throws [[LicenseCapabilityMissingError]] to all
27 gated instance methods and the 3 static build factories (buildFromArray/
buildFromSheets/buildEmpty, which throw it via BuildEngineFactory whenever
namedExpressions is non-empty against a restricted entitlement) - matching
this file's own established per-method @throws convention, which every other
exception type already follows. Also corrected the class-level @see list on
LicenseCapabilityMissingError itself: it wrongly named resumeEvaluation (which
this PR deliberately does NOT gate, to avoid stranding a suspended engine) and
omitted all 17 Crud methods; it now lists every method that can actually throw
it and explains the resumeEvaluation exclusion.
Also generalizes an earlier, narrower finding (a separate manual review had
flagged only setCellContents as untested): mutation-testing all 27 ensureCapability
call sites found 17 with zero regression protection of their own - a future
accidental removal of any one of them would ship silently, since the suite's
"one test per feature group" strategy only pinned the group representative.
Added one dedicated throw test per previously-uncovered method (setCellContents,
removeRows, addColumns, removeColumns, moveCells, moveRows, moveColumns, addSheet,
removeSheet, clearSheet, setSheetContent, renameSheet, cut, paste x3 for the
bypass fix itself, redo, changeNamedExpression, removeNamedExpression), plus one
test proving LicenseCapabilityMissingError is reachable from the public entrypoint
(importing from the package root rather than src/errors directly, which is why
the packaging gap went unnoticed by the existing suite).
Verified: tsc --noEmit clean; eslint 0 new errors; full private suite green
(511/511 suites, 6370/6373 tests, 3 pre-existing skips); the paste/cut fix and
the index.ts export both re-verified against a freshly rebuilt commonjs package.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GuUdq242TtFaepKRNdEkj9
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## hf-307-entitlement-gating-pr1 #1729 +/- ##
==============================================================
Coverage 97.32% 97.33%
==============================================================
Files 198 198
Lines 15789 15841 +52
Branches 3469 3402 -67
==============================================================
+ Hits 15367 15419 +52
- Misses 414 422 +8
+ Partials 8 0 -8
🚀 New features to boost your workflow:
|
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.

Context
HF-307 (feature packages / entitlement gating). This is PR 2 of 4: guards on the public API. Stacked on hf-307-entitlement-gating-pr1 (#1728) - depends on
CapabilityRegistry/allowsFeature/FeatureId/Config.licenseCapabilitiesfrom that PR. Mechanical, public-repo-only, no key-format knowledge needed.New:
src/errors.ts-LicenseCapabilityMissingError, mirroring the existing ~30 error classes. Now also exported from the public entrypoint (src/index.ts) - see finding 6.src/HyperFormula.ts- privateensureCapability(feature: FeatureId), mirroringensureEvaluationIsNotSuspended: a singleisLicenseGateActiveboolean read on the fast path, thenallowsFeatureagainst the resolved entitlement.src/BuildEngineFactory.ts- private staticensureNamedExpressionsCapability(config, namedExpressions), the build-time counterpart (task 2.3).src/CrudOperations.ts- publicisCutClipboard(), letting the public API layer tell a cut-paste (a cell move) apart from a copy-paste (a value duplication) - see finding 6.Modified:
ensureCapabilitywired as the first statement (before argument validation - a license error should beat a type error) in the ~20 methods from spec §05 task 2.2:addNamedExpression,changeNamedExpression,removeNamedExpressioncopy,cut,paste(plus Crud, conditionally - see finding 6)addRows,removeRows,addColumns,removeColumns,moveCells,moveRows,moveColumns,addSheet,removeSheet,clearSheet,setSheetContent,renameSheet,setCellContents,swapRowIndexes,setRowOrder,swapColumnIndexes,setColumnOrderundo,redobatch,suspendEvaluation— but notresumeEvaluation, see finding 4 belowCustomFunctionsis not gated anywhere, per HF-307 decision D1.ImportExporthas no methods to gate yet (HF-107).Where the line is drawn (written into the
ensureCapabilityJSDoc so a later change has to move it on purpose):getCellValue,listNamedExpressions,getAllNamedExpressionsSerialized, theisItPossibleTo*predicates) and teardown that only ever removes state (clearClipboard,clearUndoStack,clearRedoStack,destroy). Gating cleanup would let a restricted entitlement strand an integration mid-teardown while giving a licensee nothing. This mirrors gate B, which blocks calling a function rather than reading an already-computed value. There is a test pinning this, because "we chose not to gate cleanup" is otherwise indistinguishable from "we forgot these" — which is exactly how the reordering gap below survived the first pass, and how the cut/paste gap in finding 6 survived this one.Open question resolved: the handoff left "is
getAllNamedExpressionsgated or read-only-exempt?" open. There's no such method today (it'slistNamedExpressions,getNamedExpression,getAllNamedExpressionsSerialized) - I left all three ungated. This follows gate B's own precedent: it blocks calling a function, not reading a cell's already-computed value, so a restricted entitlement can still see named expressions that already exist. Flagging for reviewers in case the intended scope was broader.Task 2.3:
BuildEngineFactory.ensureNamedExpressionsCapabilityruns the sameallowsFeature(FeatureId.NamedExpressions)check when thenamedExpressionsargument tobuildFromSheets/buildFromSheet/buildEmpty(whatbuildFromArray/buildFromSheets/buildEmptyresolve to) is non-empty; an empty list is never checked. Deliberately not applied torebuildWithConfig- that path re-serializes named expressions an already-built instance was already allowed to create (e.g. onupdateConfig()), rather than accepting them fresh from a caller, so a later entitlement change must not retroactively break an existing instance's own state.This ships without a real license-key payload adapter (PR 3), so every entitlement
Configcan produce today is unrestricted -ensureCapabilityand the build-time check are correct, independently-testable no-ops in production until that adapter lands, exactly like gate B in PR 1.Self-review findings (after the PR was first opened)
Recording these in the open rather than quietly amending, since several are things a reviewer should get to disagree with. Findings 4 and 5 came from Cursor Bugbot's inline comments and were confirmed here before fixing. Finding 6 came from an independent spec-to-ship review run against this PR specifically after it had already been open for review (a 5-dimension multi-agent pass: guard completeness, test-quality-by-mutation, the shipped build artifact, docs/CHANGELOG accuracy, and merge mechanics - each finding adversarially re-verified before being reported).
1. The Crud gate was bypassable — fixed in
8a26e2be7. The first pass gatedmoveRows/moveColumnsbut missed four structurally identical methods:swapRowIndexes,setRowOrder,swapColumnIndexes,setColumnOrder. A restricted entitlement with no Crud grant could still permute every row and column in a sheet, which defeats the gate for a whole class of structural mutation. All four are now gated in their own right rather than relying on theswap*method that theset*Orderpair delegates to — otherwiseset*Order's ownmappingFromOrdervalidation would run first and a type error would beat the license error, violating task 2.2's first-statement rule.2.
ensureCapabilitychecks gate B only, never gate A — deliberate, and now an explicit invariant. Today a missing or invalid key yields#LIC!in cells while the CRUD API keeps working; this PR preserves that exactly. The risk is forward-looking: if PR 3's key adapter resolves an invalid key to a restricted entitlement rather than an unrestricted one, every gated method here starts throwing and that becomes a silent breaking API change. This PR is what makes that reachable, so the invariant is documented at the guard itself. Reviewers: worth confirming this is the intended split before PR 3 lands.3. Known evidence gap in the task 2.3 tests. The
BuildEngineFactorygate is exercised by calling the private static directly, because the public path (buildFromArray(sheet, config, namedExpressions)→ throws) cannot be reached today — the factories construct their ownConfig, and everyConfigcurrently resolves unrestricted. So Codecov's 100% patch coverage overstates the evidence for that one check: the integration path is unproven until PR 3 makes a restrictedConfigconstructible. Flagging rather than papering over it.4. Gating
resumeEvaluationcould brick an engine — fixed inc29a9ba40. Raised by Bugbot, confirmed.resumeEvaluationis the only exit from a suspended engine, and_evaluationSuspendedsurvivesrebuildWithConfig. So: suspend while Batching is granted →updateConfigproduces an entitlement without Batching → the instance is permanently unusable, because every read throwsEvaluationSuspendedErrorand the sole recovery path threwLicenseCapabilityMissingError. It is now ungated.suspendEvaluationandbatchstay gated — those are the entry points that make the feature worth licensing, and if you cannot enter batching you can never extract value from it. Generalised into a rule onensureCapabilityso it does not get re-added: a capability check must never be reachable only on the way out of a state it let the caller into — the same reasoning that leaves teardown ungated.5. One of PR 1's tests was vacuous — fixed in the paired tests PR. Also raised by Bugbot. PR 1's custom-function-exemption test built the sheet with
'=CUSTOMFUNC()'already in it, so the formula was evaluated duringbuildFromArraywhile the entitlement was still unrestricted;restrictEnginethen changed nothing thatgetCellValuewould re-read, and the assertion passed against a cached value. I verified this rather than assuming it — deleting the exemption fromInterpreter.tsoutright left the test still passing. It now writes the formula viasetCellContentsafter restricting, matching the shape its sibling tests already had, and the same mutation now correctly fails it. The two newresumeEvaluationregression tests were verified the same way (re-adding the gate fails them).6.
pastewas a hard-gating bypass, plus a packaging gap and a documentation gap — found by an independent spec-to-ship review, all fixed.paste()checked onlyFeatureId.Clipboard, butCrudOperations.paste()dispatches tomoveCells()internally when the clipboard holds a cut - the exact cell-relocating mutation the publicmoveCells()requires Crud for. AClipboard-only entitlement could reach it viacut()+paste(), with no Crud grant ever checked. Reproduced live (the move fully executed, no exception, under a Clipboard-only entitlement) before fixing. Fixed by addingCrudOperations.isCutClipboard()and additionally gatingpaste()onCrudwhen the clipboard holds a cut - still checked before argument validation.LicenseCapabilityMissingErrorwas never imported/exported fromsrc/index.ts, so it was unreachable from the package's public entrypoint:import {LicenseCapabilityMissingError} from 'hyperformula'failed to compile (TS2614), the default-export static form wasundefined, and no deep import worked either (theexportsmap inpackage.jsononly defines.and the i18n subpaths). A consumer had no supported way to catch this error by type - the existing test suite never caught it because it imports the class directly fromsrc/errors, bypassing the public entrypoint entirely. Fixed by exporting it alongside the other ~30 error classes already there.@throws [[LicenseCapabilityMissingError]]tag, breaking this file's own established per-method@throwsconvention (every other exception type already gets one). The class-level@seelist onLicenseCapabilityMissingErroritself also wrongly namedresumeEvaluation(which finding 4 deliberately excludes) and omitted all 17 Crud methods. Fixed: all 27 gated instance methods plus the 3 static build factories now carry the tag, and the@seelist is corrected and complete.The same review also generalised finding coverage on the test side: mutation-testing every one of the 27
ensureCapabilitycall sites found 17 with zero regression protection of their own (the "one test per feature group" strategy only pinned the group representative) - see the paired tests PR for the added coverage.Process note, since it generalises: Bugbot's findings 4 and 5 were missed by the automated review tier that scrapes them (a regex looking for
Severity: Mediumagainst Bugbot's actual**Medium Severity**), which reported a clean PASS. Finding 6 was missed by both Bugbot and a prior manual review pass (which had only caught the narrower, single-method version of the test-coverage gap). Worth reading Bugbot's inline comments directly rather than trusting an aggregator, and worth an independent adversarial pass even after a PR looks clean.How did you test your changes?
tsc --noEmitandtsc -p tsconfig.test.json: clean.eslinton all changed files: 0 errors (pre-existing warning categories only, none introduced by this change).LicenseCapabilityMissingErrorunder a simulated restricted entitlement, an unrestricted entitlement (today's default) is unaffected, andbuildFromArraywith named expressions under the default unrestricted entitlement does not throw. Re-verified against a freshly rebuiltcommonjspackage for finding 6 specifically (the cut+paste fix and theindex.tsexport).unit/license/public-api-guards.spec.ts(42 tests: one positive + one negative per feature group, a validation-order test, the build-time bypass pair, five covering the reordering gap, one pinning the ungated-cleanup rule, two for theresumeEvaluationdeadlock, one dedicated throw test per previously-group-only-covered method (15 of them), the cut/paste bypass control+regression pairs, and one provingLicenseCapabilityMissingErroris reachable from the public entrypoint) plus fixes to PR 1'sunit/licence.spec.ts(the test-helper interaction described in that PR, and the vacuous exemption test in finding 5).resumeEvaluationtests, the repaired custom-function test, the cut+paste bypass fix itself, and a sample of the newly-added per-method tests (removeRows,changeNamedExpression) were each verified by reverting the corresponding source line and confirming the new test fails, then restoring.Types of changes
Related issues:
Checklist:
Note
Medium Risk
Touches a large slice of the core public API and defines which operations throw under restricted entitlements; production behavior is unchanged until PR 3, but an invalid-key → restricted entitlement mapping there could suddenly start throwing on many methods.
Overview
Adds license entitlement guards on the public HyperFormula API so restricted licenses cannot use gated mutations once entitlement resolution is wired (HF-307 PR 2).
Introduces
LicenseCapabilityMissingErrorand a privateensureCapabilityhelper that runs before argument validation whenconfig.isLicenseGateActiveand the entitlement lacks the requestedFeatureId. It is wired into Crud (cell/sheet/row/column mutations, including row/column reorder helpers), UndoRedo, Clipboard (copy/cut), Batching (batch,suspendEvaluation), and NamedExpressions API mutations. Reads, cleanup/teardown (clearClipboard, undo stack clears,destroy), andresumeEvaluationstay ungated so callers are not stranded after suspension or mid-teardown.pasterequires Clipboard always and additionally Crud when the internal clipboard is a cut (newCrudOperations.isCutClipboard), closing a bypass where cut+paste could relocate cells without Crud.BuildEngineFactoryaddsensureNamedExpressionsCapabilityonbuildFromArray/buildFromSheets/buildEmptywhen initialnamedExpressionsis non-empty;rebuildWithConfigis intentionally excluded.With today’s unrestricted entitlements from PR 1, these checks are no-ops in production until a restricted entitlement adapter lands.
Reviewed by Cursor Bugbot for commit 88feb29. Bugbot is set up for automated code reviews on this repo. Configure here.