Add CODEX_HOME .env support; patch Electron bundle; ignore .env#2
Open
JKamsker wants to merge 3 commits intoaidanqm:mainfrom
Open
Add CODEX_HOME .env support; patch Electron bundle; ignore .env#2JKamsker wants to merge 3 commits intoaidanqm:mainfrom
JKamsker wants to merge 3 commits intoaidanqm:mainfrom
Conversation
Reviewer's Guide.env-based CODEX_HOME configuration is added to the run script, Electron app bundles are post-processed to patch main/renderer behavior, and .env is now ignored in version control. Flow diagram for Electron bundle patching via Patch-AppMainJsflowchart TD
Start([Start Patch-AppMainJs]) --> MainPath[Compute mainJs = AppDir/.vite/build/main.js]
MainPath --> CheckMain{main.js exists?}
CheckMain -->|no| EndNoMain([Return without changes])
CheckMain -->|yes| ReadMain[Read main.js content as text]
ReadMain --> InitInsertions[Initialize empty insertions list]
InitInsertions --> CheckConfigPatch{text not matching Failed to open config.toml?}
CheckConfigPatch -->|yes| AddConfigPatch[Append open-config-toml patch snippet to insertions]
CheckConfigPatch -->|no| SkipConfigPatch[Do not modify config handler]
AddConfigPatch --> CheckSpawnPatch
SkipConfigPatch --> CheckSpawnPatch
CheckSpawnPatch{text not matching __codexWindowsPatched?}
CheckSpawnPatch -->|yes| AddSpawnPatch[Append Windows spawn patch snippet to insertions]
CheckSpawnPatch -->|no| SkipSpawnPatch[Do not modify spawn]
AddSpawnPatch --> AnyInsertions
SkipSpawnPatch --> AnyInsertions
AnyInsertions{insertions list is empty?} -->|yes| EndNoInsert([Return without changes])
AnyInsertions -->|no| BuildInsertionText[Join insertions with newlines into insertionText]
BuildInsertionText --> FindSourceMap{text matches //# sourceMappingURL line?}
FindSourceMap -->|yes| InsertBeforeMap[Replace sourceMappingURL line with insertionText then source map comment]
FindSourceMap -->|no| AppendInsertion[Append insertionText to end of file]
InsertBeforeMap --> WriteMain[Write updated main.js with UTF-8 no BOM]
AppendInsertion --> WriteMain
WriteMain --> AssetsPath[Compute assetsDir = AppDir/webview/assets]
AssetsPath --> CheckAssets{assetsDir exists?}
CheckAssets -->|no| EndNoAssets([Done patching main only])
CheckAssets -->|yes| ListIndexFiles[Get index-*.js files]
ListIndexFiles --> ForEachIndex[For each index-*.js file]
ForEachIndex --> ReadIndex[Read file content rt]
ReadIndex --> StoreOrig[Store orig = rt]
StoreOrig --> ReplaceErrors[Replace error log patterns with more verbose interpolation]
ReplaceErrors --> ReplaceCleanup[Replace automation cleanup logs to use sanitizeLogValue where desired]
ReplaceCleanup --> CompareChanges{rt differs from orig?}
CompareChanges -->|no| NextFile[Skip write for this file]
CompareChanges -->|yes| WriteIndex[Write updated file with UTF-8 no BOM]
WriteIndex --> NextFile
NextFile --> MoreFiles{More index-*.js files?}
MoreFiles -->|yes| ForEachIndex
MoreFiles -->|no| EndDone([Finished patching Electron bundle])
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The bundle patching in
Patch-AppMainJsrelies on exact string/regex matches in minified JS; consider centralizing the patterns/constants or adding some basic validation/logging so that silent mismatches or future upstream changes are easier to detect and debug. - Several
try { ... } catch {}blocks inPatch-AppMainJsand the asset-rewriting loop swallow all exceptions; it would be safer to at least log failures (or scope thetryblocks more narrowly) so unexpected issues with file IO or pattern replacements don’t silently fail. - The
.envparsing/writing helpers implement a custom subset of dotenv behavior (e.g., no inline comments orexportsupport); if this script is expected to coexist with other dotenv tooling, consider documenting or slightly extending the parser to handle the most common variations to avoid surprising users.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The bundle patching in `Patch-AppMainJs` relies on exact string/regex matches in minified JS; consider centralizing the patterns/constants or adding some basic validation/logging so that silent mismatches or future upstream changes are easier to detect and debug.
- Several `try { ... } catch {}` blocks in `Patch-AppMainJs` and the asset-rewriting loop swallow all exceptions; it would be safer to at least log failures (or scope the `try` blocks more narrowly) so unexpected issues with file IO or pattern replacements don’t silently fail.
- The `.env` parsing/writing helpers implement a custom subset of dotenv behavior (e.g., no inline comments or `export` support); if this script is expected to coexist with other dotenv tooling, consider documenting or slightly extending the parser to handle the most common variations to avoid surprising users.
## Individual Comments
### Comment 1
<location> `scripts/run.ps1:283-284` </location>
<code_context>
+ $p = [Environment]::ExpandEnvironmentVariables($Path).Trim().Trim('"').Trim("'")
+ $p = $p -replace "/", "\"
+ try {
+ $full = [System.IO.Path]::GetFullPath($p)
+ return $full.TrimEnd("\")
+ } catch {
+ return $p.TrimEnd("\")
</code_context>
<issue_to_address>
**issue:** Normalize-PathString can strip the trailing backslash from root paths like `C:\` or UNC shares.
`TrimEnd("\")` turns `C:\` into `C:`, and similarly alters UNC roots like `\\server\share\`, which is not equivalent and can change behavior. Consider only trimming a trailing separator when the path is longer than the minimum root (or after `Path.GetFullPath`, only when there is more than a root component).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add support for configuring and persisting CODEX_HOME via .env and environment variables and patch the Electron bundle for improved behavior and diagnostics.
New Features:
Enhancements: