Audit SDK against official Nequi specs: crash fixes, spec alignment, gift codes module - #8
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds extensive Turborepo skill/reference documentation, migrates the toolchain from Bun to pnpm with updated package and CI settings, and expands the Nequi server SDK with gift codes, subscription cancellation, revised error handling, and updated schemas and typed service wiring. ChangesTurborepo Skill Documentation
Tooling Migration and Build Config
Server SDK Feature and Error Handling
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Nequi
participant nequiAuth
participant Nequi API
Caller->>Nequi: post/get/auth call
Nequi->>Nequi: check cached token
alt token missing or expiring
Nequi->>nequiAuth: fetch token with AbortSignal
nequiAuth->>Nequi API: auth request
Nequi API-->>nequiAuth: token or error
nequiAuth-->>Nequi: token or NequiError
end
Nequi->>Nequi API: request with headers and timeout
alt non-2xx or embedded API failure
Nequi API-->>Nequi: error body / status payload
Nequi-->>Caller: mapped NequiError
else success
Nequi API-->>Nequi: response body
Nequi-->>Caller: typed response
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review for security regressions |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60325b59dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "node": ">=24" | ||
| }, | ||
| "packageManager": "bun@1.3.6" | ||
| "packageManager": "pnpm@11.10.0", |
There was a problem hiding this comment.
This switches the repo to pnpm@11.10.0 and replaces bun.lock with pnpm-lock.yaml, but the release path I inspected still runs .github/setup/action.yaml → bun install --frozen-lockfile and .github/workflows/release.yaml → bun run build. bun install --help describes --frozen-lockfile as only “Disallow changes to lockfile”, and Bun does not consume the new pnpm lockfile, so the privileged npm release job would either fail without bun.lock or resolve dependency ranges outside the reviewed lockfile while NPM_TOKEN is present. Update the setup/release workflow to use pnpm with pnpm-lock.yaml, or keep a Bun lockfile if Bun remains the release installer.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 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 @.agents/skills/turborepo/references/best-practices/dependencies.md:
- Around line 147-158: The pnpm Catalogs example in the dependencies guide is
inconsistent with the documented pnpm version requirement. Update the guidance
around the “Option 3: pnpm Catalogs” section so it matches the version implied
by the example: either raise the documented minimum pnpm version used by the
guide or remove this catalog example entirely. Make sure the references to
packageManager and the pnpm catalogs subsection are aligned.
In @.agents/skills/turborepo/references/best-practices/packages.md:
- Around line 56-58: The package export for the button entry is pointing `types`
at the source `./src/button.tsx` instead of the emitted declaration file. Update
the export map in the package metadata so the `button` entry uses the generated
`.d.ts` under `dist/`, matching the compiled runtime path and keeping the
`types` field aligned with the build output.
In @.agents/skills/turborepo/references/best-practices/RULE.md:
- Around line 95-97: The package export for the button entrypoint points types
to the source TSX file instead of the emitted declarations. Update the export
map in the relevant package config so the button entry uses the compiled
declaration file path, and make sure the `types` field for `./button` matches
the built output alongside `default` from `button.js`.
In @.agents/skills/turborepo/references/caching/remote-cache.md:
- Around line 39-47: Update the GitHub Actions CI example in the remote-cache
guide so the Build step uses the full turbo command form instead of the
shorthand. In the example under the Build step, replace the `npx turbo build`
usage with `npx turbo run build`, keeping the existing `env` variables
unchanged. Use the `Build` snippet in the markdown as the target location.
In @.agents/skills/turborepo/references/caching/RULE.md:
- Around line 148-153: The Key Points section in RULE.md is too broad about
caching behavior and should be tightened to refer only to restored file
artifacts. Update the wording in the outputs-related bullets so it clearly
states that outputs controls file artifacts restored on a cache hit, while logs
are still cached/replayed, and replace “cache nothing” with “do not restore file
artifacts” in the task wording.
- Around line 19-22: The caching rule is using the wrong configuration key for
file globs; `global.env` should only be used for environment variables, while
file-based inputs must go under `global.inputs` when
`futureFlags.globalConfiguration` is enabled. Update the guidance in RULE.md to
reference `global.inputs` for lockfiles and other file globs, and keep
`global.env` only for env-var keys, using the Turbo configuration symbols
`globalDependencies`, `globalEnv`, `global.inputs`, and `globalConfiguration` to
place the terminology correctly.
In @.agents/skills/turborepo/references/ci/github-actions.md:
- Around line 109-115: The cache key in the GitHub Actions example is
npm-specific because it only hashes package-lock.json, so update the turborepo
cache example to either clearly scope it to npm in the surrounding guidance or
include the appropriate lockfiles for other package managers such as
pnpm-lock.yaml and yarn.lock. Use the actions/cache@v4 example around the .turbo
cache and the turbo-${{ runner.os }}-${{ hashFiles(...) }} key to make the
lockfile coverage match the package manager being documented.
In @.agents/skills/turborepo/references/cli/commands.md:
- Around line 137-146: Update the `--output-logs` CLI reference to include the
missing `hash-only` option so it matches the task configuration docs. In the
`--output-logs` section of the command reference, add `hash-only` alongside the
existing values (`full`, `new-only`, `errors-only`, `none`) and keep the
description aligned with the behavior documented for this flag.
- Around line 116-125: The `--graph` CLI documentation still shows the
deprecated JSON output example, which conflicts with the current supported
formats. Update the `turbo build --graph` examples in this section to remove the
`graph.json` case and keep only the non-deprecated outputs referenced by the
docs, such as `.svg`, `.html`, `.mermaid`, and `.dot`, so the examples match the
behavior described for `--graph`.
In @.agents/skills/turborepo/references/cli/RULE.md:
- Around line 17-29: The CLI guidance is inconsistent: the top rule says
committed examples must use turbo run, but the quick-reference examples still
show turbo shorthand like turbo build and turbo test. Update the examples in the
RULE.md guidance to use turbo run for committed/static contexts, or adjust the
rule wording so it matches the examples; keep the references under the turbo run
and quick-reference sections aligned.
In @.agents/skills/turborepo/references/configuration/RULE.md:
- Around line 9-17: The tree example in the RULE.md guidance is using an
unlabeled fenced block, which triggers markdownlint MD040. Update the example
fence to include a language tag such as text so the snippet remains rendered
correctly while satisfying linting; this applies to the tree example under the
configuration section.
In @.agents/skills/turborepo/references/filtering/patterns.md:
- Around line 106-112: The “Complex Combinations” example in the filtering
patterns guide is too broad because `--filter=./apps/*` expands the run set to
every app instead of only changed apps plus dependents. Update the example under
that heading to use a change-based filter pattern that preserves the intended
behavior, and keep the guidance aligned with `turbo run build` and the
surrounding `--filter=...[HEAD^1]` example.
- Around line 142-146: The deploy example is using two filters, and the
`./apps/*` pattern makes it run against every app instead of only changed apps.
Update the example in the filtering patterns doc so the `turbo run deploy`
command uses only the changed-apps filter (`[main...HEAD]`) and remove the broad
`./apps/*` selector; keep the example aligned with the intent of deploying only
affected apps.
In `@package.json`:
- Around line 15-35: Update the CI setup and workflow commands to match the PNPM
migration: the current Bun-based steps in the setup action and Biome workflow
are still using bun install and bun run, which no longer align with the repo’s
packageManager setting. Replace the Bun installation/setup with PNPM setup, and
switch the affected commands to pnpm install and pnpm run in the relevant
workflow/action definitions so CI uses the same package manager as the project.
In `@packages/server/package.json`:
- Around line 43-45: The server package’s Node engine and type definitions are
out of sync: `engines.node` is pinned to `>=20` while the `@types/node`
dependency is on a Node 24 major. Update the server package’s type dependency to
match the declared runtime target, or otherwise ensure no Node 24-only APIs are
referenced from the server code; use the `@types/node` entry in the package
manifest to keep the types aligned with `engines.node`.
In `@packages/server/src/schemas/dispersions.ts`:
- Around line 6-17: Update DisperseFundsRQSchema in dispersions.ts to validate
the document fields together: add min(1) to documentNumber so empty strings are
rejected, and add a cross-field refine on the schema to ensure documentType and
documentNumber are either both present or both omitted. Keep the change anchored
in DisperseFundsRQSchema and DocumentTypeSchema so the pairing rule is enforced
consistently with the other required fields.
In `@packages/server/tsconfig.json`:
- Around line 19-20: The tsconfig entry with ignoreDeprecations is currently
silencing all TypeScript 6.0 deprecation warnings without context. Update the
packages/server tsconfig.json config near the existing types and
ignoreDeprecations settings to add a clear comment explaining which deprecated
compiler options are being temporarily ignored and that they should be removed
later. Keep the change scoped to the TypeScript config so future cleanup can
identify the reason quickly.
In `@pnpm-workspace.yaml`:
- Around line 1-6: The workspace patterns are duplicated between
pnpm-workspace.yaml and the root package.json workspaces field, which can drift
over time. Keep pnpm-workspace.yaml as the source of truth and remove the
redundant workspaces array from package.json so there is only one place to
maintain the workspace definition.
🪄 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: 20aba55e-423b-4c91-af60-0375893a496d
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (57)
.agents/skills/turborepo/SKILL.md.agents/skills/turborepo/command/turborepo.md.agents/skills/turborepo/references/best-practices/RULE.md.agents/skills/turborepo/references/best-practices/dependencies.md.agents/skills/turborepo/references/best-practices/packages.md.agents/skills/turborepo/references/best-practices/structure.md.agents/skills/turborepo/references/boundaries/RULE.md.agents/skills/turborepo/references/caching/RULE.md.agents/skills/turborepo/references/caching/gotchas.md.agents/skills/turborepo/references/caching/remote-cache.md.agents/skills/turborepo/references/ci/RULE.md.agents/skills/turborepo/references/ci/github-actions.md.agents/skills/turborepo/references/ci/patterns.md.agents/skills/turborepo/references/ci/vercel.md.agents/skills/turborepo/references/cli/RULE.md.agents/skills/turborepo/references/cli/commands.md.agents/skills/turborepo/references/configuration/RULE.md.agents/skills/turborepo/references/configuration/global-options.md.agents/skills/turborepo/references/configuration/gotchas.md.agents/skills/turborepo/references/configuration/tasks.md.agents/skills/turborepo/references/environment/RULE.md.agents/skills/turborepo/references/environment/gotchas.md.agents/skills/turborepo/references/environment/modes.md.agents/skills/turborepo/references/filtering/RULE.md.agents/skills/turborepo/references/filtering/patterns.md.agents/skills/turborepo/references/watch/RULE.md.claude/skills/turborepobiome.jsonpackage.jsonpackages/server/README.mdpackages/server/package.jsonpackages/server/src/auth/index.tspackages/server/src/constants/index.tspackages/server/src/dispersions/index.tspackages/server/src/error/index.tspackages/server/src/error/types.tspackages/server/src/giftcodes/index.tspackages/server/src/index.tspackages/server/src/nequi.tspackages/server/src/payments/index.tspackages/server/src/qr/index.tspackages/server/src/reports/index.tspackages/server/src/schemas/auth.tspackages/server/src/schemas/common.tspackages/server/src/schemas/dispersions.tspackages/server/src/schemas/giftcodes.tspackages/server/src/schemas/index.tspackages/server/src/schemas/payments.tspackages/server/src/schemas/qr.tspackages/server/src/schemas/reports.tspackages/server/src/schemas/subscriptions.tspackages/server/src/subscriptions/index.tspackages/server/src/utils/builders.tspackages/server/tsconfig.jsonpackages/server/tsup.config.tspnpm-workspace.yamlskills-lock.json
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ersions Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
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 @.github/workflows/biome.yaml:
- Around line 16-18: The workflow still references mutable GitHub Action tags,
so update the action references in the Biome workflow to full commit SHAs
instead of versions for actions/checkout, pnpm/action-setup, actions/setup-node,
and cycjimmy/semantic-release-action. Keep the current version as a comment next
to each pinned SHA so the workflow remains readable, and ensure the replacement
is done consistently wherever these action names appear in the repo.
🪄 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: 4b066cfd-c82a-454f-b524-252356457cba
⛔ Files ignored due to path filters (1)
apps/docs/public/favicon.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
.github/setup/action.yaml.github/workflows/biome.yaml.github/workflows/release.yamlapps/docs/src/layouts/Layout.astropackage.jsonpackages/server/package.jsonpackages/server/src/error/index.tspackages/server/src/schemas/dispersions.tspackages/server/tsconfig.json
| uses: actions/checkout@v6 | ||
| with: | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Pin GitHub Actions to commit SHAs for supply-chain security.
zizmor flagged actions/checkout@v6 as an unpinned action reference (error-level, required by blanket policy). This applies to all actions across the repo (pnpm/action-setup@v5, actions/setup-node@v6, cycjimmy/semantic-release-action@v4). Tag-pinned actions are mutable and vulnerable to tag reassignment attacks. Consider pinning to full commit SHAs with the version in a comment.
🔒 Example SHA-pinned action reference
- uses: actions/checkout@v6
+ uses: actions/checkout@<full-sha> # v6🧰 Tools
🪛 zizmor (1.26.1)
[warning] 15-18: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 16-16: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for 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.
In @.github/workflows/biome.yaml around lines 16 - 18, The workflow still
references mutable GitHub Action tags, so update the action references in the
Biome workflow to full commit SHAs instead of versions for actions/checkout,
pnpm/action-setup, actions/setup-node, and cycjimmy/semantic-release-action.
Keep the current version as a comment next to each pinned SHA so the workflow
remains readable, and ensure the replacement is done consistently wherever these
action names appear in the repo.
Source: Linters/SAST tools
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 806578a00e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.clientId, | ||
| this.clientSecret, | ||
| this.authUri, | ||
| AbortSignal.timeout(this.timeoutMs), |
There was a problem hiding this comment.
When timeoutMs is supplied from JavaScript/config as a string, NaN, a fractional/negative number, or a value above Node's delay limit, AbortSignal.timeout throws synchronously here before nequiAuth runs and before request()'s try/catch can convert failures into the [NequiError, null] tuple. Validate/coerce this public option in the constructor or wrap this call so a timeout misconfiguration follows the SDK's tuple error contract instead of rejecting the first SDK method call.
Useful? React with 👍 / 👎.
| "exports": { | ||
| ".": { | ||
| "import": "./dist/index.js" | ||
| "types": "./dist/index.d.ts", |
There was a problem hiding this comment.
For TypeScript consumers using the new CJS entry point with .cts/import = require() under moduleResolution: node16 or nodenext, this top-level types condition is selected before require, so the package resolves to the ESM dist/index.d.ts while runtime resolves to dist/index.cjs; TypeScript then reports that the module only resolves to ESM and cannot be imported with require. Split the export into nested import/require branches with a CJS declaration such as dist/index.d.cts for the require branch.
Useful? React with 👍 / 👎.
|
🎉 This PR is included in version 1.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Summary
Full audit of the SDK against the official Nequi API documentation and Swagger specs, fixing crash bugs, contract violations, and spec drift, plus a new Códigos Plata module.
Crash & contract fixes
qr.createQR()crashed on every call (ReferenceError: URLS is not defined)[error, data]tuple, rejecting the promise in consumer appsResponseHeader.Status.StatusCode !== "0") were returned as success — failed payments looked successful; now surfaced asapi_errorwithapiStatusCodeand the raw bodyAbortSignal.timeout(configurabletimeoutMs, default 30s)MessageIDnow matches the per-API spec format (10-char alphanumeric / 16-digit numeric) instead of a 36-char UUIDenv; an invalidenvnow throws instead of silently routing to productionauthentication_error, 429 →rate_limit_exceededtoken_typeno longer over-strict"Bearer"literal;expires_inNaN guard; caller headers normalized so SDK auth headers can't be overridden;NequiError.toJSON()and cross-realm-safeisNequiErrorSpec alignment
documentType/documentNumber;reference1-3required (max 45) per speccancelSubscription(); removed orphanSTATUS_PAYMENTendpoint (not in the spec)typenarrowed to documented literal"payment"; exportedPAYMENT_STATUSconstantsformataccepts any casing and normalizes"reversionRS": ""shape; all methods return typed responses instead ofunknownNew
nequi.giftCodes(Códigos Plata):generateCode()andreverseRedemption(). Note: the official doc'sDestinationexamples are placeholders (ExampleService/test); values are derived from the endpoint path convention and should be confirmed against the sandbox.Tooling
baseUrldeprecation + missingtypes: ["node"]) and gatedbuildontsc --noEmit— this is why theURLScrash shippedexportsmap updated;engines: node >=20; explicitnode:cryptoimportsBreaking changes (semver note)
reference1-3now requiredtypemust be"payment"validation_errorreports status 422 (was 403)Test plan
pnpm typecheckandpnpm buildpass (ESM, CJS, DTS)[NequiError, null]tuples (validation, bad credentials against live sandbox, timeouts) — nothing throwsapi_errorwith domain coderequire()smoke testDestinationvalues andgetSubscriptionoperation casing)🤖 Generated with Claude Code
Summary by CodeRabbit