Skip to content

feat: add rust stdnum core#130

Open
jan-kubica wants to merge 4 commits into
mainfrom
codex/rust-core
Open

feat: add rust stdnum core#130
jan-kubica wants to merge 4 commits into
mainfrom
codex/rust-core

Conversation

@jan-kubica

Copy link
Copy Markdown
Contributor

Summary

  • add a strict Rust workspace for stdnum
  • add stella-stdnum-core boolean validation for the identifier set needed by native consumers
  • add thin NAPI and PyO3 binding crates around the shared core
  • add Rust CI/scripts alongside the existing TypeScript checks

Validation

  • bun run rust:fmt:check
  • bun run rust:lint
  • bun run rust:test
  • bun run format:check
  • bun run typecheck
  • bun run lint
  • bun test

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new Rust workspace containing standard identifier validation libraries (stella-stdnum-core, stella-stdnum-napi, and stella-stdnum-py) with native bindings for Node.js and Python. Feedback on the core validation library focuses on fixing critical correctness bugs in CNPJ character mapping and Czech special DIC checksums, resolving a potential compilation error with is_multiple_of in a const fn, and improving robustness for Spanish DNI length variations. Additionally, safety and performance improvements are suggested, such as using strip_suffix to avoid risky manual slicing, optimizing string operations to reduce heap allocations, and consolidating multi-pass string checks into a single pass.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread crates/stdnum-core/src/lib.rs
Comment thread crates/stdnum-core/src/lib.rs
Comment thread crates/stdnum-core/src/lib.rs Outdated
Comment thread crates/stdnum-core/src/lib.rs
Comment thread crates/stdnum-core/src/lib.rs Outdated
Comment thread crates/stdnum-core/src/lib.rs
Comment thread crates/stdnum-core/src/lib.rs
Comment thread crates/stdnum-core/src/lib.rs
@jan-kubica jan-kubica marked this pull request as ready for review June 25, 2026 11:18

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7c602fde6a

ℹ️ 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".

Comment thread crates/stdnum-core/src/lib.rs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 614ac70ef7

ℹ️ 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".

Comment thread src/es/dni.ts
Comment on lines +28 to +31
if (v.length < 2 || v.length > 9) {
return err(
"INVALID_LENGTH",
"DNI must be 8 digits and 1 letter",
"DNI must be 1-8 digits and 1 letter",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Update DNI discovery metadata for short numbers

After this change, es.dni.validate("1234567L") succeeds, but callers using the patterns API still will not find that value: toRegex() infers DNI length from the single 8-digit example and no lengths metadata, so it builds a 9-character pattern and toRegex(es.dni).test("1234567L") is false. If 1-7 digit DNIs are now valid inputs, the validator metadata/pattern coverage needs to be widened with the validation change, otherwise extraction workflows silently miss newly valid DNIs.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b4949ece89

ℹ️ 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".

affiliate.saturating_add(province.saturating_mul(10_000_000))
} else {
province
.saturating_mul(100_000_000)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use a wider NSS base before modulo

For Spanish NSS values with province 43-52 and an affiliate number at least 10,000,000, the concatenated base exceeds u32::MAX; for example 493961665403 needs base 4,939,616,654, but this u32 arithmetic saturates before % 97, so the native validator rejects an ID accepted by src/es/nss.ts. Compute the base in u64 or reduce modulo 97 without saturating.

Useful? React with 👍 / 👎.

let month = d2.saturating_mul(10).saturating_add(d3);
let yy = d4.saturating_mul(10).saturating_add(d5);
let year = dk_cpr_century(yy, d6).saturating_add(yy);
valid_date(year, month, day)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject future CPR birth dates

When the CPR serial head maps the year into the 2000s, this only checks that the date exists, so future birth dates are accepted; as of 2026-06-25, 3112575000 resolves to 2057-12-31 and returns true here, while src/dk/cpr.ts rejects future birth dates. Native callers will therefore treat not-yet-assignable Danish personal IDs as valid.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant