fix: check for empty strings and escapement for ID formatting

[sanny-io]

1. '' is no longer accepted as a valid format string.
2. The format arg now properly checks for escapement of %s and throws an error if there are no unescaped %s
3. Removed the escaped test case in the ORM client suite as it did not have an unescaped %s, meaning all rows would be assigned the same ID (this should always be considered an error by ZModel, and is now reported as such).
4. Added some other edge cases.

Summary by CodeRabbit

• Bug Fixes
  • Improved validation for ID format strings (UUID, ULID, CUID, Nanoid) to correctly identify and enforce unescaped placeholders in format patterns.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR tightens validation for ID format strings by requiring an unescaped %s placeholder. Using negative lookbehind regex, the validator now distinguishes between escaped (\%s) and unescaped (%s) patterns. Test coverage expands to validate both valid and invalid escaped format string scenarios across unit and E2E tests.

Changes

Cohort / File(s)	Summary
Format string validation logic packages/language/src/validators/function-invocation-validator.ts	Implements negative lookbehind regex to require unescaped %s in UUID/ULID/CUID/Nanoid format arguments; rejects escaped patterns and undefined formats with validation errors
Unit test coverage packages/language/test/function-invocation.test.ts	Adds valid test cases for escaped patterns (\%s_%s, %s_\%s) and error cases for empty, incomplete, and improperly escaped format strings
E2E test schema updates tests/e2e/orm/client-api/generated-id-format-strings.test.ts	Replaces EscapedTest.escaped field with mixedEscaped2 and mixedEscaped3 fields; updates corresponding test assertions for generated UUID pattern validation

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

• feat: ID formatting (prefixing/suffixing) #509: Adds initial %s format validation/formatting support for generated IDs, directly preceding this PR's tightened validation for escaped versus unescaped patterns.

Suggested reviewers

• ymc9

Poem

🐰 Escaped or not, the patterns align,
A lookbehind regex, precise and fine,
\%s rejected, %s accepted with cheer,
Format strings validated, crystal clear!
This bunny hops through the tests with glee,
Strong validation is what it should be! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix: check for empty strings and escapement for ID formatting' directly and clearly describes the main changes: validation now rejects empty strings and checks for proper escapement of %s in ID format strings.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

packages/language/src/validators/function-invocation-validator.ts (1)

93-100: LGTM! The negative lookbehind correctly validates unescaped placeholders.

The validation logic correctly:

• Allows undefined formatArg (optional parameter)
• Rejects empty strings (new behavior per PR objectives)
• Requires at least one unescaped %s placeholder
• Handles escaped placeholders like \%s properly

The negative lookbehind (?<!\\) is an ES2018 feature, which should be well-supported given the project's modern JavaScript/TypeScript stack.

Optional: The `g` flag is unnecessary for test()

The g (global) flag doesn't affect the behavior of test() when checking for the presence of a pattern. You could simplify to:

-            !/(?<!\\)%s/g.test(formatArg)
+            !/(?<!\\)%s/.test(formatArg)

However, this is purely a stylistic nitpick with no functional impact.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 549a026 and e911b28.

📒 Files selected for processing (3)

• packages/language/src/validators/function-invocation-validator.ts
• packages/language/test/function-invocation.test.ts
• tests/e2e/orm/client-api/generated-id-format-strings.test.ts

🧰 Additional context used

📓 Path-based instructions (1)

tests/e2e/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

E2E tests should validate real-world schema compatibility with established projects

Files:

• tests/e2e/orm/client-api/generated-id-format-strings.test.ts

🧠 Learnings (3)

📚 Learning: 2025-11-26T01:55:04.540Z

Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests

Applied to files:

• packages/language/test/function-invocation.test.ts

📚 Learning: 2025-11-26T01:55:04.540Z

Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects

Applied to files:

• packages/language/test/function-invocation.test.ts
• tests/e2e/orm/client-api/generated-id-format-strings.test.ts

📚 Learning: 2025-11-26T01:55:04.540Z

Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests

Applied to files:

• tests/e2e/orm/client-api/generated-id-format-strings.test.ts

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: build-test (22.x, sqlite)
• GitHub Check: build-test (22.x, postgresql)

🔇 Additional comments (4)

packages/language/test/function-invocation.test.ts (2)

151-175: Excellent test coverage for escaped placeholder patterns.

The new test cases correctly verify that format strings with both escaped (\%s) and unescaped (%s) placeholders are accepted, as long as at least one unescaped placeholder exists. These align well with the E2E test expectations.

---

179-212: Comprehensive invalid format string test coverage.

The new error cases correctly validate rejection of:

• Empty format strings
• Format strings with only escaped placeholders (\%s)
• Format strings with incomplete patterns (%, no s)
• Format strings with no placeholders at all

This ensures the validator properly enforces the requirement for at least one unescaped %s.

tests/e2e/orm/client-api/generated-id-format-strings.test.ts (2)

148-149: Schema updates correctly reflect the new validation rules.

The removal of the escaped field (which presumably contained only escaped %s patterns) and addition of mixedEscaped2 and mixedEscaped3 fields align with the validator's requirement for at least one unescaped %s placeholder. These edge cases provide valuable real-world validation of the format string behavior.

As per coding guidelines, this E2E test validates real-world schema compatibility with the updated validation rules.

---

168-172: Test expectations correctly validate escaped vs. unescaped placeholders.

The expectations properly verify:

• mixedEscaped2: First %s is replaced with UUID, \%s produces literal %s
• mixedEscaped3: Two escaped \%s produce literal %s, third %s is replaced

This confirms the end-to-end behavior matches the validator's intent.

[ymc9]

LGTM. Thanks!