fix: Windows npm path for Gemini OAuth + feat: WORKING.md bootstrap (#46368, #46367)#46371
Closed
xuwei-xy wants to merge 1 commit intoopenclaw:mainfrom
Closed
fix: Windows npm path for Gemini OAuth + feat: WORKING.md bootstrap (#46368, #46367)#46371xuwei-xy wants to merge 1 commit intoopenclaw:mainfrom
xuwei-xy wants to merge 1 commit intoopenclaw:mainfrom
Conversation
Contributor
Greptile SummaryThis PR combines two fixes: a Windows-specific fallback for resolving Gemini CLI OAuth credentials via
Confidence Score: 3/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/google-gemini-cli-auth/oauth.test.ts
Line: 192-237
Comment:
**`process.env.PATH` not restored in finally block**
The test saves and restores `process.platform` and `APPDATA`, but it modifies `process.env.PATH = "/nonexistent"` without saving the original value or restoring it in the `finally` block. This leaks the corrupted `PATH` into all subsequently running tests in the same test file, which can cause `findInPath` to silently return `null` for every subsequent test that depends on the real PATH.
```suggestion
it("falls back to APPDATA npm global path on Windows when gemini is not in PATH", async () => {
const originalPlatform = process.platform;
const originalAppdata = process.env.APPDATA;
const originalPath = process.env.PATH;
try {
Object.defineProperty(process, "platform", { value: "win32", configurable: true });
process.env.APPDATA = join(rootDir, "fake", "AppData", "Roaming");
process.env.PATH = "/nonexistent";
```
And in the `finally` block, add:
```
} finally {
Object.defineProperty(process, "platform", { value: originalPlatform, configurable: true });
if (originalAppdata !== undefined) {
process.env.APPDATA = originalAppdata;
} else {
delete process.env.APPDATA;
}
if (originalPath !== undefined) {
process.env.PATH = originalPath;
} else {
delete process.env.PATH;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/workspace.ts
Line: 526-533
Comment:
**WORKING.md excluded from subagent/cron sessions**
`DEFAULT_WORKING_FILENAME` is not added to `MINIMAL_BOOTSTRAP_ALLOWLIST` (line 555–561), so WORKING.md will be silently stripped by `filterBootstrapFilesForSession` when the session is a subagent or cron job. If WORKING.md is meant to convey the current active task context, subagents spawned during that task would lack this context, which could impair their effectiveness.
If the exclusion is intentional (e.g., WORKING.md is private to the main session), a short comment near `MINIMAL_BOOTSTRAP_ALLOWLIST` clarifying the design decision would help future readers. Otherwise, add `DEFAULT_WORKING_FILENAME` to that set.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: ce79b76 |
Comment on lines
+192
to
+237
| it("falls back to APPDATA npm global path on Windows when gemini is not in PATH", async () => { | ||
| const originalPlatform = process.platform; | ||
| const originalAppdata = process.env.APPDATA; | ||
| try { | ||
| Object.defineProperty(process, "platform", { value: "win32", configurable: true }); | ||
| process.env.APPDATA = join(rootDir, "fake", "AppData", "Roaming"); | ||
| process.env.PATH = "/nonexistent"; | ||
|
|
||
| const appdataOauth2Path = join( | ||
| process.env.APPDATA, | ||
| "npm", | ||
| "node_modules", | ||
| "@google", | ||
| "gemini-cli", | ||
| "node_modules", | ||
| "@google", | ||
| "gemini-cli-core", | ||
| "dist", | ||
| "src", | ||
| "code_assist", | ||
| "oauth2.js", | ||
| ); | ||
|
|
||
| mockExistsSync.mockImplementation((p: string) => { | ||
| const normalized = normalizePath(p); | ||
| if (normalized === normalizePath(appdataOauth2Path)) { | ||
| return true; | ||
| } | ||
| return false; | ||
| }); | ||
| mockReadFileSync.mockReturnValue(FAKE_OAUTH2_CONTENT); | ||
|
|
||
| const { extractGeminiCliCredentials, clearCredentialsCache } = await import("./oauth.js"); | ||
| clearCredentialsCache(); | ||
| const result = extractGeminiCliCredentials(); | ||
|
|
||
| expectFakeCliCredentials(result); | ||
| } finally { | ||
| Object.defineProperty(process, "platform", { value: originalPlatform, configurable: true }); | ||
| if (originalAppdata !== undefined) { | ||
| process.env.APPDATA = originalAppdata; | ||
| } else { | ||
| delete process.env.APPDATA; | ||
| } | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
process.env.PATH not restored in finally block
The test saves and restores process.platform and APPDATA, but it modifies process.env.PATH = "/nonexistent" without saving the original value or restoring it in the finally block. This leaks the corrupted PATH into all subsequently running tests in the same test file, which can cause findInPath to silently return null for every subsequent test that depends on the real PATH.
Suggested change
| it("falls back to APPDATA npm global path on Windows when gemini is not in PATH", async () => { | |
| const originalPlatform = process.platform; | |
| const originalAppdata = process.env.APPDATA; | |
| try { | |
| Object.defineProperty(process, "platform", { value: "win32", configurable: true }); | |
| process.env.APPDATA = join(rootDir, "fake", "AppData", "Roaming"); | |
| process.env.PATH = "/nonexistent"; | |
| const appdataOauth2Path = join( | |
| process.env.APPDATA, | |
| "npm", | |
| "node_modules", | |
| "@google", | |
| "gemini-cli", | |
| "node_modules", | |
| "@google", | |
| "gemini-cli-core", | |
| "dist", | |
| "src", | |
| "code_assist", | |
| "oauth2.js", | |
| ); | |
| mockExistsSync.mockImplementation((p: string) => { | |
| const normalized = normalizePath(p); | |
| if (normalized === normalizePath(appdataOauth2Path)) { | |
| return true; | |
| } | |
| return false; | |
| }); | |
| mockReadFileSync.mockReturnValue(FAKE_OAUTH2_CONTENT); | |
| const { extractGeminiCliCredentials, clearCredentialsCache } = await import("./oauth.js"); | |
| clearCredentialsCache(); | |
| const result = extractGeminiCliCredentials(); | |
| expectFakeCliCredentials(result); | |
| } finally { | |
| Object.defineProperty(process, "platform", { value: originalPlatform, configurable: true }); | |
| if (originalAppdata !== undefined) { | |
| process.env.APPDATA = originalAppdata; | |
| } else { | |
| delete process.env.APPDATA; | |
| } | |
| } | |
| }); | |
| it("falls back to APPDATA npm global path on Windows when gemini is not in PATH", async () => { | |
| const originalPlatform = process.platform; | |
| const originalAppdata = process.env.APPDATA; | |
| const originalPath = process.env.PATH; | |
| try { | |
| Object.defineProperty(process, "platform", { value: "win32", configurable: true }); | |
| process.env.APPDATA = join(rootDir, "fake", "AppData", "Roaming"); | |
| process.env.PATH = "/nonexistent"; |
And in the finally block, add:
} finally {
Object.defineProperty(process, "platform", { value: originalPlatform, configurable: true });
if (originalAppdata !== undefined) {
process.env.APPDATA = originalAppdata;
} else {
delete process.env.APPDATA;
}
if (originalPath !== undefined) {
process.env.PATH = originalPath;
} else {
delete process.env.PATH;
}
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/google-gemini-cli-auth/oauth.test.ts
Line: 192-237
Comment:
**`process.env.PATH` not restored in finally block**
The test saves and restores `process.platform` and `APPDATA`, but it modifies `process.env.PATH = "/nonexistent"` without saving the original value or restoring it in the `finally` block. This leaks the corrupted `PATH` into all subsequently running tests in the same test file, which can cause `findInPath` to silently return `null` for every subsequent test that depends on the real PATH.
```suggestion
it("falls back to APPDATA npm global path on Windows when gemini is not in PATH", async () => {
const originalPlatform = process.platform;
const originalAppdata = process.env.APPDATA;
const originalPath = process.env.PATH;
try {
Object.defineProperty(process, "platform", { value: "win32", configurable: true });
process.env.APPDATA = join(rootDir, "fake", "AppData", "Roaming");
process.env.PATH = "/nonexistent";
```
And in the `finally` block, add:
```
} finally {
Object.defineProperty(process, "platform", { value: originalPlatform, configurable: true });
if (originalAppdata !== undefined) {
process.env.APPDATA = originalAppdata;
} else {
delete process.env.APPDATA;
}
if (originalPath !== undefined) {
process.env.PATH = originalPath;
} else {
delete process.env.PATH;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+526
to
+533
| // WORKING.md is opt-in: only loaded when present in the workspace. | ||
| const workingPath = path.join(resolvedDir, DEFAULT_WORKING_FILENAME); | ||
| try { | ||
| await fs.access(workingPath); | ||
| entries.push({ name: DEFAULT_WORKING_FILENAME, filePath: workingPath }); | ||
| } catch { | ||
| // WORKING.md not present — skip | ||
| } |
Contributor
There was a problem hiding this comment.
WORKING.md excluded from subagent/cron sessions
DEFAULT_WORKING_FILENAME is not added to MINIMAL_BOOTSTRAP_ALLOWLIST (line 555–561), so WORKING.md will be silently stripped by filterBootstrapFilesForSession when the session is a subagent or cron job. If WORKING.md is meant to convey the current active task context, subagents spawned during that task would lack this context, which could impair their effectiveness.
If the exclusion is intentional (e.g., WORKING.md is private to the main session), a short comment near MINIMAL_BOOTSTRAP_ALLOWLIST clarifying the design decision would help future readers. Otherwise, add DEFAULT_WORKING_FILENAME to that set.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/workspace.ts
Line: 526-533
Comment:
**WORKING.md excluded from subagent/cron sessions**
`DEFAULT_WORKING_FILENAME` is not added to `MINIMAL_BOOTSTRAP_ALLOWLIST` (line 555–561), so WORKING.md will be silently stripped by `filterBootstrapFilesForSession` when the session is a subagent or cron job. If WORKING.md is meant to convey the current active task context, subagents spawned during that task would lack this context, which could impair their effectiveness.
If the exclusion is intentional (e.g., WORKING.md is private to the main session), a short comment near `MINIMAL_BOOTSTRAP_ALLOWLIST` clarifying the design decision would help future readers. Otherwise, add `DEFAULT_WORKING_FILENAME` to that set.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close as superseded. The OAuth half targets the removed So I’m closing this here and keeping the remaining discussion on the canonical linked item. Review detailsBest possible solution: Close this combined PR. Keep the separately merged active-provider OAuth fixes, leave Security review: Security review needs attention: The diff broadens OAuth credential-source discovery and should not be merged without maintainer security review, especially because the branch is already superseded.
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 9e5d6c70917d. |
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.
Combines fixes from upstream PRs:
Merged from openclaw/openclaw PRs.