Conversation
url-parse was used only to build the OPA client base URL. Node's native WHATWG URL (available since v10; engines.node already requires >=10) does the same, so extract a buildOpaBaseUrl() helper and drop url-parse and @types/url-parse. yarn.lock is pruned of url-parse and its now-orphaned transitive dependencies (querystringify, requires-port) only; every other entry is left byte-for-byte unchanged.
Lock the exact OPA base URL produced for the default PDP, trailing-slash, explicit-port, https, and path-prefix inputs so the url-parse -> native URL refactor is proven behaviour-equivalent on valid input and any regression fails here; assert a scheme-less PDP (bare host or //host:port) throws; and assert the Enforcer wires the OPA client baseURL to buildOpaBaseUrl(pdp).
Pin actions/checkout (v7.0.0) and actions/setup-node (v6.4.0) to full commit SHAs in both workflows, set persist-credentials: false on all checkouts, and bump the CI node matrix from 18/20 to 20/22 (18 is EOL). Run the full suite on PRs/pushes, not only on release. Same-repo events provision a throwaway Permit env via PROJECT_API_KEY, run a dockerized PDP (now with -e PDP_API_KEY/PERMIT_API_KEY and a /healthy readiness wait), execute test:ci:full, and delete the env on always(). Fork and secret-less runs fall back to the no-backend test:ci:unit suite. Add the two supporting scripts and quote $GITHUB_ENV in the publish workflow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace AVA 3 with Vitest 4.1 (vitest.config.ts with unit / module-imports
/ integration / e2e projects; backend projects run serially via forks +
maxWorkers=1 to avoid shared-env collisions). Keep test:ci:unit /
test:ci:full names so the CI workflow is unchanged.
Remove every timer-based propagation wait: a new waitFor/waitForCheck
helper polls the actual permit.check() until it converges, bounded by a
timeout, replacing the fixed sleep(10s) waits in the e2e suites.
Rewrite fixtures to a createTestClient() factory (handleApiError now
throws). Migrate all t.* assertions to expect. Module-import specs load
the built bundle (build/index.{js,mjs}) to keep packaging-regression
coverage. Wire in the two previously orphaned specs (bulk, lists) with
proper setup/cleanup; preserve bulkRelationshipTuples coverage. Keep the
inherently racy local_facts "skip wait" case as it.skip and add a
deterministic waitForSync header unit test. Drop ava/nyc/codecov/ts-node;
add vitest/@vitest/coverage-v8; bump @types/node to ^20; skipLibCheck for
Vitest's d.ts.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a shared mock seam (src/tests/helpers/mock-api.ts, createMockPermit) that patches the axios adapter on the REST, PDP and OPA transports and seeds API context without network, then add unit specs covering every API module (resources, roles, resource-roles, role-assignments, users, tenants, resource-instances, resource-relations, relationship-tuples, condition-sets, condition-set-rules, resource-actions/attributes/ action-groups, projects, environments, elements, deprecated), the enforcer (check/bulkCheck/getUserPermissions/checkAllTenants, string parsing, default-tenant, OPA path, response shaping, throwOnError) and the utils/config layer. Add one ABAC e2e (condition-sets) following the event-based, self-cleaning conventions. 262 new unit tests; full no-backend suite is 333 tests. Tests-only; no SDK source changes. Tests assert current behavior of two latent bugs (checkAllTenants payload PER-15318; unreachable PermitPDPStatusError), flagged in-code, not fixed here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
checkAllTenants passed { headers, params } as the axios POST body (2nd
arg), so the Authorization header was never sent and the query was
nested under `params` instead of being the request body — the PDP could
neither authenticate nor read the request.
Mirror check(): send the normalized { user, action, resource, context }
as the body and pass headers/timeout as the axios config arg. Normalize
the string forms of user/resource but skip default-tenant injection,
since an all-tenants query must not be pinned to a tenant. Add an AVA
regression test asserting the auth header is sent, the body shape is
correct, and no tenant is injected.
Fixes PER-15318
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The e2e suites are AVA with fixed sleep(10s) waits and fail fast on the first error. Against a freshly started PDP they hit a momentary ECONNREFUSED window right after the write burst (OPA reload), which kills the whole run even though the env, key, and policy sync are all healthy (/healthy passes). Scope the PR backend run to the suite that reliably passes — unit + integration + module-imports (what `yarn test` runs, the same set the publish workflow runs). The event-based, error-tolerant e2e lands in the stacked test-migration PR, which re-includes e2e in CI. Also add a PDP diagnostics step (docker logs + container state + /healthy) on backend-run failure so PDP connection errors, which surface with no HTTP response, are debuggable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…test-event-based-tests * per-15306/ci-pin-actions-tests-on-pr: ci: run unit/integration/module-imports on PR, defer e2e to next PR # Conflicts: # package.json
…rehensive-sdk-tests * per-15315/vitest-event-based-tests: ci: run unit/integration/module-imports on PR, defer e2e to next PR
Stacked PRs target feature branches, so a pull_request filter of branches:[main] meant they never ran CI. Drop the base-branch filter so every PR is tested regardless of base. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…test-event-based-tests * per-15306/ci-pin-actions-tests-on-pr: ci: run on all pull requests, not only those targeting main
…rehensive-sdk-tests * per-15315/vitest-event-based-tests: ci: run on all pull requests, not only those targeting main
Node resolves `localhost` to ::1 (IPv6) first, but the GitHub runner's Docker IPv6 port publish refuses connections, so e2e permit.check() calls hit ECONNREFUSED even though the PDP is healthy on IPv4 (curl /healthy returns 200). Set PDP_URL to http://127.0.0.1:7766 so the SDK uses the working IPv4 path, and pin the readiness probe to 127.0.0.1 too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…test-event-based-tests * per-15306/ci-pin-actions-tests-on-pr: ci: pin PDP connection to IPv4 (127.0.0.1) in the backend test run
…rehensive-sdk-tests * per-15315/vitest-event-based-tests: ci: pin PDP connection to IPv4 (127.0.0.1) in the backend test run
The dockerized PDP in CI doesn't expose OPA (port 8181), so rbac's direct useOpa checks hit ECONNREFUSED. Gate them behind PERMIT_RUN_OPA_E2E (default off) so they only run against an OPA-exposed setup. Raise the rebac convergence gate to 150s and the e2e test timeout to 300s, since the heavy ReBAC graph needs longer to propagate cloud->PDP on a cold env. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rehensive-sdk-tests * per-15315/vitest-event-based-tests: test: make rbac useOpa checks opt-in and widen rebac CI budget
bulkCheck and getUserPermissions query separate PDP endpoints that can lag a single permit.check, so the direct assertions raced cloud->PDP propagation and flaked on the slower matrix leg. Gate the complete-user read and poll bulkCheck/getUserPermissions until they converge before asserting. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rehensive-sdk-tests * per-15315/vitest-event-based-tests: test: poll the rbac multi-result reads to remove propagation races
A userset condition set referencing user.<attr> requires that attribute to exist on the built-in user resource; users.sync alone doesn't register it, so the condition-set creation failed with 400 MISSING_RESOURCE_ATTRIBUTE. Register a run-unique attribute on the __user resource before creating the userset, reference it consistently in the condition and the synced users, and remove it in the tolerant afterAll. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Condition sets compile to new policy (rego), which propagates slower than role/fact writes, so the 60s default left the ABAC check timing out in CI before the policy took effect. Match the heavier rebac budget. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PER-15318 Read allowed_tenants and return the tenant details. Merge the global context store into the request context, with caller keys taking precedence, as check() does. Replace adapter fixtures with real local HTTP tests for normalized POST bodies, authentication, SDK-language headers, attributes, empty decisions, and global context merging. Add a shared local PDP test server for these and later regression tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PER-16492 Merge each check's context over the method context before deriving the global context. Keep sibling checks and caller-owned contexts isolated. Add local HTTP regression coverage for precedence, optional method context, shallow merging, and unchanged inputs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PER-16494
Raise PermitPDPStatusError with statusCode and responseBody for HTTP
responses, including Axios rejections, in check, bulkCheck,
getUserPermissions and checkAllTenants. Preserve transport connection
errors.
HTTP error responses that Axios rejects, such as 401 and 500, used to
raise PermitConnectionError with a connection-failure message. Their
error name is now PermitPDPStatusError, and their message is the one
used for unexpected resolved statuses: "Permit.<method>() got an
unexpected status code: <status>, ...". The message does not include
the user, action or resource.
A 200 response with a body the SDK cannot read, such as {}, also used
to raise PermitConnectionError saying the SDK cannot connect to the
PDP. It now raises PermitPDPStatusError with statusCode 200, the raw
body in responseBody, and a message saying the PDP returned an
unexpected response body.
Make PermitPDPStatusError extend PermitConnectionError so existing
instanceof PermitConnectionError catches keep handling HTTP failures
that previously surfaced as connection errors. Keep one-argument
construction available; SDK-generated HTTP errors fill both new fields.
checkAllTenants no longer rethrows the raw AxiosError, which carried
the request config and its Authorization header. It maps PDP errors
like the other methods and, when throwing, logs each one once, without
the extra log in Permit.checkAllTenants. Like check and bulkCheck, it
applies the SDK throwOnError setting to every failure, including an
invalid resource string: with throwing disabled it logs the error and
returns an empty tenant list. With throwing disabled, bulkCheck
returns one false per input check instead of an empty array.
Cover 401, 500, unexpected resolved statuses, string response bodies,
unreadable 200 bodies, and transport timeouts for all four methods,
and invalid resource strings for the three methods that take a
resource, as separate tests, with per-call and global error-policy
overrides. Check that thrown errors contain neither the API key nor
user details.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PER-16493 Stop serializing SDK configuration in debug logs. Use plain Pino JSON for JSON mode. For pretty mode, write through a synchronous in-process pino-pretty stream instead of a Pino transport, so bundled apps need no worker-thread target and Permit instances add no process exit listeners. Keep JSON lines as the default output: log.json defaults to true. When log.json is omitted, PERMIT_LOG_JSON=false selects pretty output. The variable ignores letter case and surrounding whitespace, and any other value keeps JSON lines instead of making new Permit() throw. An explicit log.json always overrides the environment variable. Cover default, explicit and environment JSON and pretty settings with 12 instances each, PERMIT_LOG_JSON values and overrides, configured secret exclusion, and debug logging for successful calls, HTTP errors and connections the PDP closes without replying, for all four PDP methods, with throwing enabled and disabled. HTTP errors must surface as PermitPDPStatusError and closed connections as PermitConnectionError, and in JSON mode a thrown failure must produce exactly one error log record. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PER-16493 PER-16494 Add a Logging and errors section to the README. Describe log.level, the JSON default, how PERMIT_LOG_JSON is read, pretty output, and how an explicit log.json overrides the environment variable. Describe PermitPDPStatusError and PermitConnectionError, including unreadable 200 responses, the statusCode field and the raw responseBody, matching errors with instanceof, and what each PDP method returns when throwOnError is false. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Remove unused imports and constants from the e2e and module-import specs, and replace a non-null assertion with an equivalent type assertion. These warnings are pre-existing on main. There is no behaviour change: the emitted JavaScript differs only by two removed unused constants. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PER-16544 PermitApiError stored the raw AxiosError in its enumerable originalError field. The error's request config carried the Authorization header with the API key, and its Node request object carried the same header in its raw header block, so logging a failed REST call with util.inspect, JSON.stringify, pino's err serializer or an error tracker leaked the key. The deprecated permit.api methods rethrew the raw AxiosError, with the same exposure. Remove credentials from the Axios error before it is thrown. Reduce the request config to method, URL, params, body and timeout, redact the value of every request header except a short list that carries no credentials, redact the response Set-Cookie header, and drop the request objects. The status, response body, method and URL stay available for debugging. PermitApiError.request is now undefined. Cover 401 and 500 responses from a current and a deprecated REST method, and a connection reset, against a local server. Check that util.inspect, JSON.stringify and pino output contain neither the API key, a custom header secret nor a cookie, and that the useful fields remain. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PER-15306 PER-15315 PER-15317 Fold the stacked test and CI branches into this branch so the SDK fixes land together with the Vitest suite, the per-module unit tests and the CI changes. The merged head is #133 (a71e7ad), which contains #132 (64b288c) and #131 (fc2529b). Conflicts: - src/tests/e2e/lists.e2e.spec.ts, src/tests/e2e/rbac.e2e.spec.ts and src/tests/module-imports/esm-import.spec.ts: this branch only removed unused imports from the AVA versions. The stack rewrote these files for Vitest, so the stack's versions are kept. - src/tests/unit/config.spec.ts: both sides added the file. The stack's Vitest version is kept here; the next commit ports this branch's PERMIT_LOG_JSON cases into it. The SDK sources are this branch's; the stack did not touch them. The stack's package.json replaces AVA, nyc, ts-node, codecov and open-cli with Vitest, so this merge changes the dev dependencies and the lock file. This branch's AVA unit specs are ported to Vitest in the next commit, and the stack's tests that pin the old SDK behaviour are updated after that. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Sep 29, 2026
Split the condition-sets e2e test so the resource, attribute, condition sets, rule and user syncs still run and are asserted, while the decision checks are skipped until new condition-set policy reliably reaches the PDP. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zeevmoney
changed the title
Brings in #122 by @Kyzgor (PER-16497, closes #106). It builds the OPA base URL with the WHATWG URL instead of url-parse, removes url-parse and @types/url-parse, and makes new Permit() throw a PermitError that names the pdp option (without its value) when pdp is empty or not an absolute URL. That throw is a breaking change for REST-only apps with an empty or invalid pdp, so it ships with this PR. Conflict resolution: kept this branch's @types/node and lockfile, and dropped the url-parse, querystringify and requires-port entries. Ported the AVA spec to Vitest as src/tests/unit/enforcement/opa-base-url.spec.ts. Co-authored-by: Kyzgor <connordgordon95@gmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PSoip6dghQ62bLQ6GBMwTA
EliMoshkovich
approved these changes
Sep 29, 2026
EliMoshkovich
requested changes
Sep 30, 2026
EliMoshkovich
left a comment
EliMoshkovich
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed at 0a5b69f (my earlier approval was on 4c72664). Overall this is in good shape. The SDK fixes are correct: I checked checkAllTenants against the PDP (horizon/enforcer/api.py), and it really is POST /allowed/all-tenants returning allowed_tenants. The context precedence in bulkCheck is right. Making PermitPDPStatusError a subclass keeps existing instanceof PermitConnectionError handlers working. Every catch in src/api goes through handleApiError, so the redaction covers all the REST paths. The CI split, with the secret used only before checkout and cleanup keyed by env key, also looks good.
I'm requesting changes for two things we should settle before this ships as 3.0.0:
- The engines range locks out Node 26 (and 23/25).
^22.13.0 || ^24.0.0rejects Node 26, which has been Current since April and becomes LTS next month. npm and pnpm only warn about this, but yarn v1 fails the install when the engines don't match. So as soon as a customer moves to Node 26 LTS, theiryarn installbreaks. I suggest>=22.13.0(tested floors in CI, open upper bound). log.json: falsenow changes log output. On main,json: falsewas the default and produced JSON lines (prettyPrint: false). Now it switches to pino-pretty withsync: true. Anyone who setjson: falseorPERMIT_LOG_JSON=falseexplicitly will find human-readable text in their log pipeline after upgrading, and each log line becomes a synchronous stdout write on the check path. The fix itself is fine (the old flag was inverted, andjson: truecould not have worked on pino 8). It just needs a BREAKING entry in the 3.0.0 release notes. I'd also dropsync: trueor explain why it's needed.
Non-blocking:
- CI only tests the exact floors (22.13.0 / 24.0.0), never the latest 22.x/24.x that users actually run. Consider adding a
22.xorlts/*lane. codegen-guardpins different action SHAs (checkout v7.0.0, setup-node v6.4.0) from the other jobs (checkout v7.0.1, setup-node v7.0.0). Dependabot will fix this eventually, but it's cheap to align now.redactAxiosErrorassumesresponse.headersand a config are always present. A hand-builtAxiosError(from a customaxiosInstanceinterceptor or a mock adapter) without them makes thePermitApiErrorconstructor throw aTypeError, which hides the real error. A?? {}guard would prevent that. Also, the request body (config.data) is kept on purpose, so the docstring's "cannot leak" wording is slightly stronger than what the code guarantees.AllTenantsResponse.allowedTenants→allowed_tenantsis a type-level rename. It isn't re-exported from the index, so it's fine, but it's worth a line in the changelog along with the Node 20 drop.
Happy to approve once 1 and 2 are addressed.
| }, | ||
| "engines": { | ||
| "node": ">=10" | ||
| "node": "^22.13.0 || ^24.0.0" |
Collaborator
There was a problem hiding this comment.
^24.0.0 excludes Node 26 (LTS in October). yarn v1 turns an engines mismatch into a hard install error, not a warning. Suggest ">=22.13.0". CI already covers the floors.
| }; | ||
| return config.log.json | ||
| ? pino(options) | ||
| : pino(options, pretty({ levelFirst: true, sync: true })); |
Collaborator
There was a problem hiding this comment.
Behavior change: an explicit json: false used to produce JSON (it was the old default) and now produces pretty text. Please add a BREAKING note in the release. Also, why sync: true? It makes every log line a blocking stdout write.
| error.response = { | ||
| status: error.response.status, | ||
| statusText: error.response.statusText, | ||
| headers: redactHeaders(error.response.headers, (name) => name !== 'set-cookie'), |
Collaborator
There was a problem hiding this comment.
Nit: if response.headers is undefined (a hand-built AxiosError from an interceptor or mock), Object.entries throws inside the PermitApiError constructor and hides the original error. The same applies to the redactRequestConfig(error.response.config) fallback when both configs are missing. Consider headers ?? {} and a guard there.
| - node: '22' | ||
| node-version: '22.13.0' | ||
| - node: '24' | ||
| node-version: '24.0.0' |
Collaborator
There was a problem hiding this comment.
Non-blocking: only the exact floors are tested. A 24.x (or lts/*) lane would catch regressions on the versions consumers actually run.
| run_install: false | ||
|
|
||
| - name: Setup Node.js | ||
| uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 |
Collaborator
There was a problem hiding this comment.
Nit: this job pins setup-node v6.4.0 and checkout v7.0.0 (line 288), but the other jobs use v7.0.0 / v7.0.1. Align them?
This branch has not been deployed
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.
Summary
This release corrects permission-check payloads and context handling, makes SDK errors and logging safer, and moves the test suite to Vitest. It also sets the supported runtimes to Node
^22.13.0 || ^24.0.0, replaces Yarn with pinned pnpm 12.8.1, and checks authored and generated code with strict TypeScript, Oxlint and Oxfmt.Tracking: PER-16556. This description covers the implemented changes currently pushed to this PR.
SDK behavior
checkAllTenantssends an authenticated POST with the permission query in the body, normalizes users/resources, merges context, and does not inject a default tenant (PER-15318).bulkCheckrespects each item's context, followed by method and global context, without mutating caller inputs (PER-16492).PermitPDPStatusError; connection failures remainPermitConnectionError. WiththrowOnError: false, failed bulk checks return one denial per input and all-tenant checks return an empty list (PER-16494).url-parse. Invalid or empty PDP URLs fail during construction; dot segments are normalized. This includes @Kyzgor's contribution from refactor(deps): replace url-parse with the native URL API (PER-16497) #122 and closesurl-parseris an unnecessary dependency #106.Runtime and dependency management
Strict tooling and declaration integrity (PER-16558)
@ts-ignoresuppressions are removed; normalization has syntax/configuration checks and a final compiler gate.pnpm verifyruns the frozen dependency check, lint, formatting, strict types, both builds and local tests. CI uses the same command.Tests and CI
pnpm testbuilds and runs unit/module-import tests without backend credentials.pnpm test:codegenchecks generator guard failures.Verification
For pushed head
1702729(PER-16558; tooling tree2d06b9e83fe1d51d54b69ce5332b8ac65a685effplus reviewed Linux correction963e35f2d5fc27a56178fcc1f29378024c48a667):580 tests across 45 files pass on exact Node 22.13.0 and 24.0.0, including codegen/tooling tests.
Frozen installs, prek, lint, formatting, strict types, builds and packed CommonJS/ESM consumers pass. Documentation builds without warnings.
The pinned generator guard validates 346 models and 11 selected shapes.
Package comparison confirms 361 packed files, 356 declaration files and all 31 runtime exports. Entry points are preserved; the two additional declarations complete previously broken bulk types.
Production and clean-consumer audits report zero vulnerabilities. The full development tree reports 16 advisories, all rooted in the retained
standard-versiondependency; removal and security gates are tracked in PER-16559.Targeted mutations catch missing declaration exports, leaked aliases, weakened strictness and lockfile drift. Actionlint and the applicable zizmor checks pass.
Lead-built artifact SHA256:
e2e07507af1a018e90d6dffd0c62d648dacc8aad1a132b89b59de7027780eda8; it matches the reviewed author's package byte for byte.The local artifact harness passes all 18 implemented phases and 91 assertions on each support floor, with all 21 fixture deletions verified and no setup/cleanup errors. Runs include real RBAC grants/revocations, tenant boundaries, resource roles, direct OPA and generated ABAC policy.
Two independent reviews and lead integration validation passed. A follow-up fixes the compiler host callback for case-sensitive filesystems: both filesystem modes pass on both support floors, and removing the fix reproduces the Linux failure. SDK package bytes remain unchanged. CI run 36657165363 passes lint/build, generator guard, both Node integration lanes and environment cleanup. GitHub confirms no conflicts with current main.
Remaining validation
Credits
@Kyzgor contributed #122's native URL implementation and equivalence tests; the branch preserves that authorship. This PR also incorporates #131, #132 and #133.