Conversation
📝 WalkthroughWalkthroughThis PR introduces timeout support across the module discovery and Python IR fetching pipeline by adding optional Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
coderabbitai
bot
added
area:tooling
![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: e57a4b6c63
ℹ️ 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".
| const tempDir = await mkdtemp(resolve(tmpdir(), 'tywrap-config-')); | ||
| const tmpPath = resolve(tempDir, `.tywrap.config.${randomUUID()}.mjs`); |
There was a problem hiding this comment.
In loadConfigFile, emitting the transpiled .mjs into an OS temp directory changes the module resolution base, so configs that use relative imports (for example import './helper') now fail with Cannot find module because Node resolves from /tmp/... instead of the config’s directory. This is a regression from the previous behavior and breaks valid multi-file TS config setups.
Useful? React with 👍 / 👎.
| .replaceAll("'tywrap'", `'${tywrapEntryHref}'`) | ||
| .replaceAll('\"tywrap\"', `\"${tywrapEntryHref}\"`); |
There was a problem hiding this comment.
The replaceAll calls rewrite every 'tywrap'/"tywrap" string in transpiled output, not just import specifiers, so ordinary config values are mutated (for example a literal 'tywrap' value becomes a file://... URL). This silently alters user configuration and should be constrained to import/export source strings only.
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/runtime.ts (1)
444-453:⚠️ Potential issue | 🟡 MinorDeno and Bun runtimes do not receive
timeoutMsorenv/cwdsupport.The
optionsparameter is accepted at the function signature level, but the Deno and Bun branches ignoretimeoutMs,env, andcwd. If these runtimes are in scope, subprocess calls from discovery/tywrap will run without timeout enforcement there.Also applies to: 455-473
🤖 Fix all issues with AI agents
In `@src/config/index.ts`:
- Around line 330-352: The finally block currently calls rm(tmpPath, ...) then
rm(tempDir, { recursive: true, force: true }); remove the redundant explicit
removal of tmpPath (the line invoking rm(tmpPath, ...)) since removing tempDir
recursively already deletes tmpPath; keep the recursive rm(tempDir, { recursive:
true, force: true }) and leave mkdtemp, tmpPath, transpiledOutput, writeFile,
import(pathToFileURL(...)), and ensureConfigObject untouched.
- Around line 276-288: The current naive replaceAll on transpiledOutput risks
replacing non-module string literals; update the logic that runs when
emitCommonJs is false (the block using transpiledOutput and
import.meta.resolve('tywrap')) to only rewrite module specifiers in
import/export/from and dynamic import/require calls by matching targeted
patterns (e.g. "from 'tywrap'", "from \"tywrap\"", "import('tywrap')",
"require('tywrap')", and export-from forms) using regex-based replacements
rather than blind replaceAll, so only module specifiers are swapped to the
tywrapEntryHref while leaving other string literals (e.g. name: 'tywrap')
unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/config/index.tssrc/core/discovery.tssrc/tywrap.tssrc/utils/runtime.tstest/config_loading.test.tstest/discovery.test.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2026-01-20T16:01:14.323Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In `src/runtime/node-bridge.ts` (NodeBridge), a test message is sent to the Python subprocess to confirm the bridge is responsive before marking it as ready.
Applied to files:
test/discovery.test.ts
📚 Learning: 2026-01-19T21:48:27.823Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/runtime_bridge_fixtures.test.ts:59-66
Timestamp: 2026-01-19T21:48:27.823Z
Learning: In fixture-based tests (e.g., test/runtime_bridge_fixtures.test.ts) and similar tests in the tywrap repository, prefer early returns when Python or fixture files are unavailable. Do not rely on Vitest dynamic skip APIs; a silent pass is intentional for environments lacking Python/fixtures. Treat missing fixtures as optional soft-guards and ensure the test remains non-disruptive in non-availability scenarios.
Applied to files:
test/discovery.test.tstest/config_loading.test.ts
📚 Learning: 2026-01-20T01:34:07.064Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:444-458
Timestamp: 2026-01-20T01:34:07.064Z
Learning: When reviewing promise-based polling patterns (e.g., recursive setTimeout in waitForAvailableWorker functions), ensure that any recursive timer is tracked and cleared on timeout or promise resolution to prevent timer leaks. Do not rely on no-op checks after settlement; verify clearTimeout is invoked in all paths and consider using an explicit cancellation (e.g., AbortController) or a guard to cancel pending timers when the promise settles.
Applied to files:
test/discovery.test.tssrc/core/discovery.tssrc/utils/runtime.tstest/config_loading.test.tssrc/config/index.tssrc/tywrap.ts
📚 Learning: 2026-01-20T18:37:05.670Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: src/runtime/process-io.ts:37-40
Timestamp: 2026-01-20T18:37:05.670Z
Learning: In the tywrap repo, ESLint is used for linting (not Biome). Do not flag regex literals that contain control characters (e.g., \u001b, \u0000-\u001F) as lint errors, since the current ESLint configuration allows them. When reviewing TypeScript files (e.g., src/**/*.ts), rely on ESLint results and avoid raising issues about these specific control-character regex literals unless there is a competing config change or a policy requiring explicit escaping.
Applied to files:
test/discovery.test.tssrc/core/discovery.tssrc/utils/runtime.tstest/config_loading.test.tssrc/config/index.tssrc/tywrap.ts
📚 Learning: 2026-01-20T16:00:49.829Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:203-211
Timestamp: 2026-01-20T16:00:49.829Z
Learning: In the BridgeProtocol implementation (tywrap), reject Map and Set explicitly before serialization (e.g., in safeStringify or the serialization entrypoint) with a clear error like "Bridge protocol does not support Map/Set values". Do not rely on post-hoc checks of non-string keys at the point of JSON.stringify, since Maps/Sets cannot round-trip. This should be enforced at the boundary where data is prepared for serialization to ensure deterministic errors and prevent invalid data from propagating.
Applied to files:
test/discovery.test.tssrc/core/discovery.tssrc/utils/runtime.tstest/config_loading.test.tssrc/config/index.tssrc/tywrap.ts
📚 Learning: 2026-01-19T21:48:45.693Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Applied to files:
src/tywrap.ts
🧬 Code graph analysis (4)
src/core/discovery.ts (1)
src/utils/runtime.ts (1)
processUtils(422-549)
test/config_loading.test.ts (1)
src/config/index.ts (1)
loadConfigFile(228-356)
src/config/index.ts (2)
scripts/act.mjs (1)
output(59-59)src/utils/runtime.ts (2)
resolve(302-320)writeFile(386-416)
src/tywrap.ts (1)
src/utils/runtime.ts (1)
processUtils(422-549)
🔇 Additional comments (7)
test/discovery.test.ts (1)
171-187: LGTM!The test correctly verifies that
timeoutMsis propagated through toprocessUtils.execwhenModuleDiscoveryis constructed with a timeout. Mock setup and teardown are clean, and the assertion properly usestoHaveBeenCalledWithwith matchers for the non-timeout arguments.test/config_loading.test.ts (1)
36-51: Good test for read-only directory handling; note platform limitation.This test validates that TS config loading works when the source directory is read-only, confirming the new OS-temp-dir approach. The
try/finallycleanup pattern correctly restores permissions before removal.Note:
chmod(dir, 0o555)has no effect on Windows, so this test only exercises the read-only scenario on Unix-like systems. If Windows CI coverage matters, consider gating with a platform check or using a Windows-specific approach.src/tywrap.ts (1)
254-267: LGTM!The
timeoutMsparameter is cleanly threaded throughfetchPythonIrto both the primary and fallbackprocessUtils.execcalls. The optional parameter correctly defaults toundefined, which the runtime layer handles by skipping timeout setup.src/core/discovery.ts (2)
29-29: LGTM — consistenttimeoutMspropagation across all subprocess calls.The new
timeoutMsoption is correctly forwarded toresolvePythonPath,getPythonSearchPaths, andgetModuleVersionexec invocations.
46-52: No action needed. TheresolvePythonExecutablefunction does not internally spawn subprocesses—it performs only path manipulation to resolve the Python executable location. The function definition insrc/utils/python.tscontains no subprocess calls and accepts onlypythonPath,virtualEnv, andcwdoptions. Since no subprocess spawning occurs, there is no timeout concern to address.Likely an incorrect or invalid review comment.
src/utils/runtime.ts (2)
435-436: LGTM — clean environment construction and options forwarding.The env clone via
Object.create(null)avoids prototype pollution, undefined values are correctly filtered, and theoptions.envoverrides including deletions are well-handled. PYTHONPATH prepending is preserved.Also applies to: 481-501
504-543: Timeout mechanism is correctly implemented with proper settlement guards.The
settledflag +finishhelper pattern ensures single resolution/rejection across all event paths (close, error, timeout), and the timer is cleared in every settlement path — consistent with best practices for avoiding timer leaks. Based on learnings, ensuringclearTimeoutis invoked in all paths and using a guard to cancel pending timers when the promise settles is the expected pattern.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| let transpiledOutput = output.outputText; | ||
| if (!emitCommonJs) { | ||
| try { | ||
| // Preserve support for `import { defineConfig } from 'tywrap'` when ESM config | ||
| // is evaluated from an OS temp directory outside the package scope. | ||
| const tywrapEntryHref = await import.meta.resolve('tywrap'); | ||
| transpiledOutput = transpiledOutput | ||
| .replaceAll("'tywrap'", `'${tywrapEntryHref}'`) | ||
| .replaceAll('\"tywrap\"', `\"${tywrapEntryHref}\"`); | ||
| } catch { | ||
| // Best-effort: leave source as-is when tywrap cannot be resolved. | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Naive string replacement may over-match non-import occurrences of 'tywrap'.
replaceAll("'tywrap'", ...) replaces every occurrence of the literal string 'tywrap' in the transpiled output, not just import/from specifiers. If a user config contains a string value like name: 'tywrap', it will be rewritten to a file:// URL. For a typical config file this is unlikely to cause issues in practice, but a targeted regex (e.g. matching from 'tywrap' or import('tywrap')) would be safer.
♻️ More targeted replacement
- transpiledOutput = transpiledOutput
- .replaceAll("'tywrap'", `'${tywrapEntryHref}'`)
- .replaceAll('\"tywrap\"', `\"${tywrapEntryHref}\"`);
+ transpiledOutput = transpiledOutput
+ .replace(/from\s+['"]tywrap['"]/g, `from '${tywrapEntryHref}'`)
+ .replace(/import\s*\(\s*['"]tywrap['"]\s*\)/g, `import('${tywrapEntryHref}')`);🤖 Prompt for AI Agents
In `@src/config/index.ts` around lines 276 - 288, The current naive replaceAll on
transpiledOutput risks replacing non-module string literals; update the logic
that runs when emitCommonJs is false (the block using transpiledOutput and
import.meta.resolve('tywrap')) to only rewrite module specifiers in
import/export/from and dynamic import/require calls by matching targeted
patterns (e.g. "from 'tywrap'", "from \"tywrap\"", "import('tywrap')",
"require('tywrap')", and export-from forms) using regex-based replacements
rather than blind replaceAll, so only module specifiers are swapped to the
tywrapEntryHref while leaving other string literals (e.g. name: 'tywrap')
unchanged.
| const tempDir = await mkdtemp(resolve(tmpdir(), 'tywrap-config-')); | ||
| const tmpPath = resolve(tempDir, `.tywrap.config.${randomUUID()}.mjs`); | ||
| try { | ||
| // eslint-disable-next-line security/detect-non-literal-fs-filename -- temp path is derived from config location | ||
| await writeFile(tmpPath, output.outputText, 'utf-8'); | ||
| const localNodeModules = resolve(process.cwd(), 'node_modules'); | ||
| if (safeExists(localNodeModules)) { | ||
| const tempNodeModules = resolve(tempDir, 'node_modules'); | ||
| try { | ||
| // eslint-disable-next-line security/detect-non-literal-fs-filename -- temp path is derived from OS temp directory | ||
| await symlink(localNodeModules, tempNodeModules, 'dir'); | ||
| } catch { | ||
| // Best-effort only; regular resolution may still succeed without a symlink. | ||
| } | ||
| } | ||
|
|
||
| // eslint-disable-next-line security/detect-non-literal-fs-filename -- temp path is derived from OS temp directory | ||
| await writeFile(tmpPath, transpiledOutput, 'utf-8'); | ||
| const mod = (await import(pathToFileURL(tmpPath).href)) as Record<string, unknown>; | ||
| const loaded = mod.default ?? mod; | ||
| return ensureConfigObject(loaded ?? {}, resolved); | ||
| } finally { | ||
| await rm(tmpPath, { force: true }); | ||
| await rm(tempDir, { recursive: true, force: true }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Temp directory handling and cleanup look solid; minor redundancy in cleanup.
The approach of creating an isolated temp directory with a symlinked node_modules for ESM evaluation is well-designed. The finally block ensures cleanup in all paths.
Line 350 (rm(tmpPath, ...)) is redundant since line 351 already removes tempDir recursively (which contains tmpPath). Not a bug, but the explicit file removal can be dropped.
♻️ Remove redundant cleanup step
} finally {
- await rm(tmpPath, { force: true });
await rm(tempDir, { recursive: true, force: true });
}📝 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.
| const tempDir = await mkdtemp(resolve(tmpdir(), 'tywrap-config-')); | |
| const tmpPath = resolve(tempDir, `.tywrap.config.${randomUUID()}.mjs`); | |
| try { | |
| // eslint-disable-next-line security/detect-non-literal-fs-filename -- temp path is derived from config location | |
| await writeFile(tmpPath, output.outputText, 'utf-8'); | |
| const localNodeModules = resolve(process.cwd(), 'node_modules'); | |
| if (safeExists(localNodeModules)) { | |
| const tempNodeModules = resolve(tempDir, 'node_modules'); | |
| try { | |
| // eslint-disable-next-line security/detect-non-literal-fs-filename -- temp path is derived from OS temp directory | |
| await symlink(localNodeModules, tempNodeModules, 'dir'); | |
| } catch { | |
| // Best-effort only; regular resolution may still succeed without a symlink. | |
| } | |
| } | |
| // eslint-disable-next-line security/detect-non-literal-fs-filename -- temp path is derived from OS temp directory | |
| await writeFile(tmpPath, transpiledOutput, 'utf-8'); | |
| const mod = (await import(pathToFileURL(tmpPath).href)) as Record<string, unknown>; | |
| const loaded = mod.default ?? mod; | |
| return ensureConfigObject(loaded ?? {}, resolved); | |
| } finally { | |
| await rm(tmpPath, { force: true }); | |
| await rm(tempDir, { recursive: true, force: true }); | |
| } | |
| const tempDir = await mkdtemp(resolve(tmpdir(), 'tywrap-config-')); | |
| const tmpPath = resolve(tempDir, `.tywrap.config.${randomUUID()}.mjs`); | |
| try { | |
| const localNodeModules = resolve(process.cwd(), 'node_modules'); | |
| if (safeExists(localNodeModules)) { | |
| const tempNodeModules = resolve(tempDir, 'node_modules'); | |
| try { | |
| // eslint-disable-next-line security/detect-non-literal-fs-filename -- temp path is derived from OS temp directory | |
| await symlink(localNodeModules, tempNodeModules, 'dir'); | |
| } catch { | |
| // Best-effort only; regular resolution may still succeed without a symlink. | |
| } | |
| } | |
| // eslint-disable-next-line security/detect-non-literal-fs-filename -- temp path is derived from OS temp directory | |
| await writeFile(tmpPath, transpiledOutput, 'utf-8'); | |
| const mod = (await import(pathToFileURL(tmpPath).href)) as Record<string, unknown>; | |
| const loaded = mod.default ?? mod; | |
| return ensureConfigObject(loaded ?? {}, resolved); | |
| } finally { | |
| await rm(tempDir, { recursive: true, force: true }); | |
| } |
🤖 Prompt for AI Agents
In `@src/config/index.ts` around lines 330 - 352, The finally block currently
calls rm(tmpPath, ...) then rm(tempDir, { recursive: true, force: true });
remove the redundant explicit removal of tmpPath (the line invoking rm(tmpPath,
...)) since removing tempDir recursively already deletes tmpPath; keep the
recursive rm(tempDir, { recursive: true, force: true }) and leave mkdtemp,
tmpPath, transpiledOutput, writeFile, import(pathToFileURL(...)), and
ensureConfigObject untouched.
bbopen
deleted the
codex/implement-tywrap-architectural-remediation-plan
branch
1 participant
Codex generated this pull request, but encountered an unexpected error after generation. This is a placeholder PR message.
Codex Task