Skip to content

Remove redundant validateClaims from @moq/token#1196

Merged
kixelated merged 4 commits into
mainfrom
token-cleanup
Apr 3, 2026
Merged

Remove redundant validateClaims from @moq/token#1196
kixelated merged 4 commits into
mainfrom
token-cleanup

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Summary

  • Remove validateClaims() — the z.refine in ClaimsSchema already enforces the same put || get check, making it a duplicate
  • Remove the claimsSchema lowercase re-export (backward compat alias no longer needed)

Test plan

  • All 100 existing tests pass (bun test)

🤖 Generated with Claude Code

The ClaimsSchema already enforces the put/get requirement via z.refine,
making validateClaims a duplicate check. The lowercase claimsSchema alias
was only for backward compatibility that's no longer needed.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@kixelated kixelated enabled auto-merge (squash) April 3, 2026 00:28
@coderabbitai

coderabbitai Bot commented Apr 3, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4667e90d-dc1a-4507-9bba-244df2e459b4

📥 Commits

Reviewing files that changed from the base of the PR and between 84c6d2b and 099e1db.

📒 Files selected for processing (2)
  • doc/js/@moq/token.md
  • doc/rs/crate/moq-token.md
✅ Files skipped from review due to trivial changes (1)
  • doc/js/@moq/token.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • doc/rs/crate/moq-token.md

Walkthrough

Removed the exported validateClaims() function and the claimsSchema alias from the JS token package; ClaimsSchema remains and continues to enforce claim constraints. sign() and verify() no longer call validateClaims() after schema parsing. Documentation was revised for a key lifecycle API and renamed claim fields (e.g., put/get), the package version was bumped to 0.2.0, CLI examples were adjusted, and cross-language interoperability tests were added for JS and Rust.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title clearly and accurately summarizes the main change: removing the redundant validateClaims function from @moq/token.
Description check ✅ Passed The pull request description is directly related to the changeset, explaining why validateClaims is being removed and detailing both code removals and test validation.
Docstring Coverage ✅ Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch token-cleanup
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch token-cleanup

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Fix incorrect CLI syntax in README and docs (--key was shown as top-level
flag, generate used non-existent --key instead of --out). Rewrite JS doc
with actual API. Fix claims field names (pub/sub -> put/get). Add hard-coded
cross-language test fixtures so Rust tests verify JS-generated tokens and
JS tests verify Rust-generated tokens. Bump @moq/token to 0.2.0.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
js/token/src/interop.test.ts (1)

10-20: Consider extracting duplicated test fixtures to module scope.

The RUST_HS256_KEY and RUST_HS256_TOKEN constants are duplicated between the two describe blocks. Extracting them to module-level constants would reduce duplication.

♻️ Suggested refactor
 import { describe, expect, test } from "bun:test";
 import { generate } from "./generate.ts";
 import { load, loadPublic, sign, verify } from "./key.ts";

+// Shared fixtures for Rust-generated keys/tokens
+const RUST_HS256_KEY = JSON.stringify({
+	alg: "HS256",
+	key_ops: ["verify", "sign"],
+	kty: "oct",
+	k: "jvS2B_SKkvhtObaiKg9n2vq_Iqdus-nVCVVYBWf0BQA",
+	kid: "d55b1f13e6bda281",
+});
+
+const RUST_HS256_TOKEN =
+	"eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiIsImtpZCI6ImQ1NWIxZjEzZTZiZGEyODEifQ.eyJyb290IjoiZGVtbyIsInB1dCI6WyJhbGljZSJdLCJnZXQiOlsiYm9iIl19.eyOkZtTtj_MDkLF4Eu11cygb7B_6DyP-6e0TtwVE4UE";
+
 // Tests that the JS `@moq/token` package can load keys generated by the Rust
 // moq-token crate and verify tokens signed by Rust, and vice versa.

 describe("Rust-generated fixtures", () => {
-	// cargo run -p moq-token-cli -- generate --out /tmp/rust-hs256.jwk
-	const RUST_HS256_KEY = JSON.stringify({
-		alg: "HS256",
-		...
-	});
-	const RUST_HS256_TOKEN = "...";
 	// ... use module-level constants
 });

 describe("wrong key rejects token", () => {
-	const RUST_HS256_KEY = JSON.stringify({...});
-	const RUST_HS256_TOKEN = "...";
 	// ... use module-level constants
 });

Also applies to: 100-110

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@js/token/src/interop.test.ts` around lines 10 - 20, The duplicated test
fixtures RUST_HS256_KEY and RUST_HS256_TOKEN should be pulled out of the
describe blocks into module-level constants so both test suites reuse the same
values; locate the two definitions of RUST_HS256_KEY and RUST_HS256_TOKEN in
interop.test.ts, move a single copy to the top of the file (outside any
describe/it), and remove the duplicated local definitions in each describe so
tests reference the shared constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@doc/js/`@moq/token.md:
- Around line 98-102: The Markdown table in doc/js/@moq/token.md has unescaped
pipe characters in the type column (rows like `put` and `get`) causing extra
columns; fix by ensuring the type cell content is treated as a single cell—wrap
the type values in code spans or escape internal pipes (e.g., use `\|` or
&#124;) so the `put`, `get`, `cluster`, `exp`, and `iat` rows have exactly the
same number of columns as the header and the table alignment remains consistent.

In `@doc/rs/crate/moq-token.md`:
- Around line 161-165: The Markdown table's type cells (rows for `put`, `get`,
etc.) contain unescaped pipe characters (e.g., "string | string[]?") which break
the table; update those cells by wrapping the entire type text in inline code
ticks or escaping the pipes (e.g., `string | string[]?` or string \| string\[]?)
for the `put`, `get`, and any other affected rows so the table renders with the
correct number of columns.

---

Nitpick comments:
In `@js/token/src/interop.test.ts`:
- Around line 10-20: The duplicated test fixtures RUST_HS256_KEY and
RUST_HS256_TOKEN should be pulled out of the describe blocks into module-level
constants so both test suites reuse the same values; locate the two definitions
of RUST_HS256_KEY and RUST_HS256_TOKEN in interop.test.ts, move a single copy to
the top of the file (outside any describe/it), and remove the duplicated local
definitions in each describe so tests reference the shared constants.
🪄 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: CHILL

Plan: Pro

Run ID: fa19e068-cc41-43a4-81ec-205472f209de

📥 Commits

Reviewing files that changed from the base of the PR and between 0c4a55b and 84c6d2b.

📒 Files selected for processing (6)
  • doc/js/@moq/token.md
  • doc/rs/crate/moq-token.md
  • js/token/package.json
  • js/token/src/interop.test.ts
  • rs/moq-token-cli/README.md
  • rs/moq-token/src/key.rs
✅ Files skipped from review due to trivial changes (2)
  • js/token/package.json
  • rs/moq-token-cli/README.md

Comment thread doc/js/@moq/token.md Outdated
Comment thread doc/rs/crate/moq-token.md Outdated
kixelated and others added 2 commits April 2, 2026 21:29
Escape pipe characters in type column to prevent extra column parsing.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@kixelated kixelated disabled auto-merge April 3, 2026 04:48
@kixelated kixelated merged commit fe0fbd4 into main Apr 3, 2026
1 of 2 checks passed
@kixelated kixelated deleted the token-cleanup branch April 3, 2026 04:48
This was referenced Apr 3, 2026
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