DevEx: tywrap generate --check #32
Conversation
bbopen
added
documentation
📝 WalkthroughWalkthroughThis PR adds a "check mode" to the tywrap CLI that verifies generated wrappers without writing files, introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/tywrap.ts`:
- Around line 244-248: The function currently returns { written, warnings } when
checkMode is false, causing consumers to sometimes miss outOfDate; update the
normal-mode return to include outOfDate (e.g., return { written, warnings,
outOfDate: [] }) so the return shape matches the checkMode branch and the
declared optional outOfDate?: string[]; adjust the return at the non-checkMode
path where checkMode is evaluated to always include outOfDate (use the existing
outOfDate variable or an empty array).
- Around line 98-104: The safeReadFile function currently swallows all errors;
change it to only return null for file-not-found errors and rethrow other errors
so permission/I/O issues surface. In safeReadFile (which calls fsUtils.readFile)
inspect the caught error (e.g., err.code) and if it equals 'ENOENT' return null,
otherwise throw the original error (or wrap with context) so callers can detect
real failures.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
README.mddocs/getting-started.mdsrc/cli.tssrc/tywrap.tstest/cli.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/tywrap.ts (3)
src/utils/cache.ts (1)
safeReadFile(127-130)src/utils/runtime.ts (2)
fsUtils(347-417)pathUtils(261-342)src/config/index.ts (1)
createConfig(186-190)
src/cli.ts (3)
src/utils/runtime.ts (3)
resolve(302-320)readFile(358-381)writeFile(386-416)src/utils/logger.ts (1)
log(140-159)src/tywrap.ts (1)
generate(110-249)
test/cli.test.ts (1)
src/utils/runtime.ts (1)
join(265-273)
⏰ 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). (1)
- GitHub Check: os (windows-latest)
🔇 Additional comments (14)
src/tywrap.ts (4)
75-91: Well-designed interfaces for the check mode feature.The
GenerateRunOptionsandGenerateResultinterfaces cleanly extend the API surface to support the new verification mode. The optionaloutOfDatearray being present only in check mode is a good design choice.
93-96: Line ending normalization looks correct.The function handles CRLF→LF and standalone CR→LF conversions properly to avoid false positives on Windows checkouts.
129-136: Correct: directory creation skipped in check mode.This ensures no side effects occur during verification runs, which is essential for CI use cases.
177-213: Clean batched emission with check mode branching.The
filesToEmitqueue pattern is a good refactor that enables both comparison (check mode) and writing (normal mode) paths cleanly. The normalization before comparison handles cross-platform line ending differences correctly.src/cli.ts (5)
102-140: Robust package.json script injection with proper error handling.The helper correctly:
- Checks for package.json existence before attempting modification
- Preserves existing scripts object structure
- Only writes if changes are needed
- Logs warnings on failure without crashing
One consideration: the JSON serialization uses 2-space indent which is common but may not match the user's existing formatting preferences.
200-204: Well-documented --check option.The option description clearly explains the purpose for CI verification without writing files.
281-298: Review the exit code logic when both --check and --fail-on-warn are used.The current logic at lines 292-294 suppresses exit code 3 when
--fail-on-warnis set and there are warnings (falling through to exit code 2 at line 303). This means:
- Out-of-date files + warnings +
--fail-on-warn→ exit 2- Out-of-date files + no warnings → exit 3
This precedence may be intentional (warnings are more severe), but consider documenting this behavior or ensuring both issues are surfaced in the output. Currently, users might miss that files are out of date if they only check the exit code.
The interaction is subtle—verify this is the intended precedence for CI workflows.
348-353: Good UX: --scripts defaults to true with --no-scripts to opt out.This follows yargs conventions and the description clearly explains how to disable the feature.
387-389: Script injection happens after config creation—correct ordering.The conditional ensures
--no-scriptsis respected properly.README.md (1)
27-35: Clear documentation of the new features.The README updates accurately describe:
- The
initcommand's package.json script wiring- The CI verification workflow with
--checkThe use case description "verify a dependency upgrade didn't change the generated surface" is helpful context.
docs/getting-started.md (1)
263-265: Helpful CI guidance and init documentation.The Build Integration section now provides actionable advice for:
- Running
--checkin CI to verify committed wrappers- Regenerating after Python dependency upgrades
- The
--no-scriptsflag to opt out of package.json modificationtest/cli.test.ts (3)
2-2: Import addition is necessary for the new test.The
delimiterimport is used in the--checktest to constructPYTHONPATHcorrectly across platforms.
245-291: Good coverage for init --scripts behavior.The tests verify both the default behavior (scripts added) and the opt-out behavior (
--no-scripts). The assertions correctly check for the expected script values.
522-578: Comprehensive end-to-end test for --check mode.This test effectively validates the full workflow:
- Check with missing files → exit 3, stderr shows "out of date", no files written
- Generate normally → exit 0, files created
- Check again → exit 0, stdout shows "up to date"
The
PYTHONPATHsetup ensures thetywrap_irmodule is discoverable. Good use ofdelimiterfor cross-platform compatibility.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| async function safeReadFile(path: string): Promise<string | null> { | ||
| try { | ||
| return await fsUtils.readFile(path); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider distinguishing file-not-found from other read errors.
The current implementation returns null for any error, including permission errors or I/O failures. This could mask genuine issues in CI environments where a file exists but is unreadable. Consider narrowing the catch to ENOENT only.
♻️ Proposed fix to distinguish error types
async function safeReadFile(path: string): Promise<string | null> {
try {
return await fsUtils.readFile(path);
- } catch {
+ } catch (err: unknown) {
+ // Only treat "file not found" as a missing file; rethrow other errors
+ if (
+ err instanceof Error &&
+ 'code' in err &&
+ (err as NodeJS.ErrnoException).code === 'ENOENT'
+ ) {
+ return null;
+ }
+ throw err;
}
- return null;
- }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function safeReadFile(path: string): Promise<string | null> { | |
| try { | |
| return await fsUtils.readFile(path); | |
| } catch { | |
| return null; | |
| } | |
| } | |
| async function safeReadFile(path: string): Promise<string | null> { | |
| try { | |
| return await fsUtils.readFile(path); | |
| } catch (err: unknown) { | |
| // Only treat "file not found" as a missing file; rethrow other errors | |
| if ( | |
| err instanceof Error && | |
| 'code' in err && | |
| (err as NodeJS.ErrnoException).code === 'ENOENT' | |
| ) { | |
| return null; | |
| } | |
| throw err; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/tywrap.ts` around lines 98 - 104, The safeReadFile function currently
swallows all errors; change it to only return null for file-not-found errors and
rethrow other errors so permission/I/O issues surface. In safeReadFile (which
calls fsUtils.readFile) inspect the caught error (e.g., err.code) and if it
equals 'ENOENT' return null, otherwise throw the original error (or wrap with
context) so callers can detect real failures.
| if (checkMode) { | ||
| return { written: [], warnings, outOfDate }; | ||
| } | ||
|
|
||
| return { written, warnings }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Return type is correct but consider adding outOfDate to normal mode return.
In check mode, outOfDate is populated. In normal mode, it's not returned at all. The interface allows outOfDate?: string[], so this is technically correct, but returning an empty array in normal mode would provide consistent typing for consumers.
♻️ Optional: return empty outOfDate in normal mode for consistency
- return { written, warnings };
+ return { written, warnings, outOfDate: [] };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (checkMode) { | |
| return { written: [], warnings, outOfDate }; | |
| } | |
| return { written, warnings }; | |
| if (checkMode) { | |
| return { written: [], warnings, outOfDate }; | |
| } | |
| return { written, warnings, outOfDate: [] }; |
🤖 Prompt for AI Agents
In `@src/tywrap.ts` around lines 244 - 248, The function currently returns {
written, warnings } when checkMode is false, causing consumers to sometimes miss
outOfDate; update the normal-mode return to include outOfDate (e.g., return {
written, warnings, outOfDate: [] }) so the return shape matches the checkMode
branch and the declared optional outOfDate?: string[]; adjust the return at the
non-checkMode path where checkMode is evaluated to always include outOfDate (use
the existing outOfDate variable or an empty array).
![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: 908ff09219
ℹ️ 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".
| if (caching && fsUtils.isAvailable()) { | ||
| try { | ||
| const modFs = await import('fs/promises'); | ||
| await modFs.mkdir(cacheDir, { recursive: true }); | ||
| } catch {} | ||
| try { | ||
| const cached = await fsUtils.readFile(pathUtils.join(cacheDir, cacheKey)); | ||
| ir = JSON.parse(cached); |
There was a problem hiding this comment.
In check mode the generator still reads cached IR whenever performance.caching is enabled, so a stale .tywrap/cache entry can make tywrap generate --check report “up to date” even after the underlying Python package changes (e.g., dependency upgrade without changing the configured module version). That defeats the CI‑verification use case described in the docs. Consider skipping cache reads when runOptions.check is true or invalidating the cache with actual package versions so --check reflects current Python state.
Useful? React with 👍 / 👎.
What
tywrap generate --checkto verify generated wrappers are up to date without writing files (CI-friendly).tywrap init(tywrap:generate,tywrap:check; disable with--no-scripts).Behavior
.tywrap/cache, and.tywrap/reports.3when wrappers are out of date (2remains reserved for--fail-on-warn).Tests
npm run check:all