[codex] fix simple-git unsafe env handling#4602
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis PR centralizes simple-git initialization across the codebase by introducing a shared factory function that applies unsafe options needed for Git hooks and editor/pager support. It updates all git client instantiations to use this factory, stops removing paging variables from the environment, and adds enforcement rules to prevent future direct simple-git usage. ChangesCentralized Simple-Git Factory and Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Kitenite
force-pushed
the
codex/simple-git-user-env-root-cause
branch
from
b0a4c9c to
cdfe2fc
Compare
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes
Confidence Score: 3/5The core bug fix and wrapper centralization are correct, but the new lint guard is wired into lint.sh in a way that makes it a no-op for CI enforcement. check-simple-git-usage.sh correctly exits 1 on violations, but lint.sh captures biome's exit code before the custom checks run and exits with that stored value — a failing guard never propagates to the caller. The fix the PR was specifically designed to enforce (blocking future direct simpleGit() usage) is therefore not actually enforced in CI today. scripts/lint.sh — needs || exit 1 (or equivalent) after each custom check script call to make the guards effective.
|
| Filename | Overview |
|---|---|
| packages/shared/src/simple-git-options.ts | New shared module exporting all simple-git unsafe option flags and the OPTIONS object; uses an as cast instead of importing SimpleGitOptions, so flag correctness is only checked in consumer files via satisfies. |
| scripts/lint.sh | Adds bash ./scripts/check-simple-git-usage.sh to lint, but lint.sh lacks set -e and exits with biome's stored exit code, so a failure in the new guard never blocks the lint run. |
| scripts/check-simple-git-usage.sh | New lint guard using ripgrep to detect direct simpleGit imports/calls outside approved wrappers; exits 1 on violations but is not effectively wired up in lint.sh. |
| packages/host-service/src/runtime/git/simple-git.ts | New host-service wrapper that constructs simple-git instances with the shared unsafe options; clean and minimal. |
| apps/desktop/src/lib/trpc/routers/workspaces/utils/git-client.ts | Replaces bare simpleGit() calls with a local createUserSimpleGit wrapper using the shared unsafe options; removes PAGER stripping. |
| apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts | Removes the PAGER/GIT_PAGER deletion workaround; user env values are now preserved and handled via simple-git's unsafe allow-list instead. |
| packages/host-service/src/trpc/router/project/utils/resolve-repo.ts | All six bare simpleGit() calls replaced with createUserSimpleGit(); no logic changes. |
| packages/shared/src/simple-git-options.test.ts | Unit tests verifying the flags list and that every flag is enabled in the options object. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["@superset/shared\nsimple-git-options.ts\nUSER_GIT_ENV_SIMPLE_GIT_OPTIONS\n(all allowUnsafe* flags)"] --> B
A --> C
B["apps/desktop\ngit-client.ts\ncreateUserSimpleGit()"]
C["packages/host-service\nruntime/git/simple-git.ts\ncreateUserSimpleGit()"]
B --> D["getSimpleGitWithShellPath()\n(desktop callers)"]
C --> E["git.ts / git-helpers.ts\nproject.ts / resolve-repo.ts\nproject-helpers.ts"]
F["scripts/check-simple-git-usage.sh\n(lint guard)"] -.->|"wired via lint.sh\n(exit code not propagated)"| B
F -.-> C
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
scripts/lint.sh:16
**Lint guard exit code not propagated**
`check-simple-git-usage.sh` calls `exit 1` when it finds a violation, but `lint.sh` has no `set -e` and ends with `exit $exit_code` (which holds biome's exit code, captured before the custom checks run). A violation in `check-simple-git-usage.sh` will print its output but will never cause `lint.sh` to exit non-zero on its own, so CI won't be blocked. The same is true for the two pre-existing `check-*.sh` calls above it. Adding `|| exit 1` after the script call (or adding `set -e` to `lint.sh`) would make the guard effective.
### Issue 2 of 3
packages/shared/src/simple-git-options.ts:27-33
**Type assertion bypasses `simple-git` type checking in the shared package**
`Object.fromEntries(...).map(...)` is widened to `Record<string, boolean>` by TypeScript, then forced to `Record<SimpleGitUnsafeOptionFlag, true>` via an `as` cast. Nothing in this file imports `SimpleGitOptions` from `simple-git`, so if `simple-git` renames or removes a flag, this module will not produce a compile error. The safety net lives entirely in the two consumer files (via `satisfies Partial<SimpleGitOptions>`), meaning a consumer that skips the `satisfies` constraint would silently carry stale flags.
### Issue 3 of 3
apps/desktop/src/lib/trpc/routers/workspaces/utils/git-client.test.ts:11-50
**Duplicate test helpers across two test files**
`makeBlockedGitEnv` and `expectUnsafeEnvRejected` are copied verbatim in both `git-client.test.ts` and `packages/host-service/src/runtime/git/simple-git.test.ts`. If the blocked env list changes, both copies need to be updated. Since `@superset/shared` already centralizes the options, a shared test utility would keep these in sync.
Reviews (1): Last reviewed commit: "fix simple-git unsafe env handling" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| ./scripts/check-desktop-git-env.sh | ||
| ./scripts/check-git-ref-strings.sh | ||
| bash ./scripts/check-simple-git-usage.sh |
There was a problem hiding this comment.
check-simple-git-usage.sh calls exit 1 when it finds a violation, but lint.sh has no set -e and ends with exit $exit_code (which holds biome's exit code, captured before the custom checks run). A violation in check-simple-git-usage.sh will print its output but will never cause lint.sh to exit non-zero on its own, so CI won't be blocked. The same is true for the two pre-existing check-*.sh calls above it. Adding || exit 1 after the script call (or adding set -e to lint.sh) would make the guard effective.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/lint.sh
Line: 16
Comment:
**Lint guard exit code not propagated**
`check-simple-git-usage.sh` calls `exit 1` when it finds a violation, but `lint.sh` has no `set -e` and ends with `exit $exit_code` (which holds biome's exit code, captured before the custom checks run). A violation in `check-simple-git-usage.sh` will print its output but will never cause `lint.sh` to exit non-zero on its own, so CI won't be blocked. The same is true for the two pre-existing `check-*.sh` calls above it. Adding `|| exit 1` after the script call (or adding `set -e` to `lint.sh`) would make the guard effective.
How can I resolve this? If you propose a fix, please make it concise.| export const USER_GIT_ENV_SIMPLE_GIT_OPTIONS = { | ||
| unsafe: Object.fromEntries( | ||
| SIMPLE_GIT_UNSAFE_OPTION_FLAGS.map((flag) => [flag, true]), | ||
| ), | ||
| } as { | ||
| unsafe: Record<SimpleGitUnsafeOptionFlag, true>; | ||
| }; |
There was a problem hiding this comment.
simple-git type checking in the shared package
Object.fromEntries(...).map(...) is widened to Record<string, boolean> by TypeScript, then forced to Record<SimpleGitUnsafeOptionFlag, true> via an as cast. Nothing in this file imports SimpleGitOptions from simple-git, so if simple-git renames or removes a flag, this module will not produce a compile error. The safety net lives entirely in the two consumer files (via satisfies Partial<SimpleGitOptions>), meaning a consumer that skips the satisfies constraint would silently carry stale flags.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/shared/src/simple-git-options.ts
Line: 27-33
Comment:
**Type assertion bypasses `simple-git` type checking in the shared package**
`Object.fromEntries(...).map(...)` is widened to `Record<string, boolean>` by TypeScript, then forced to `Record<SimpleGitUnsafeOptionFlag, true>` via an `as` cast. Nothing in this file imports `SimpleGitOptions` from `simple-git`, so if `simple-git` renames or removes a flag, this module will not produce a compile error. The safety net lives entirely in the two consumer files (via `satisfies Partial<SimpleGitOptions>`), meaning a consumer that skips the `satisfies` constraint would silently carry stale flags.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| function makeBlockedGitEnv(workRoot: string): Record<string, string> { | ||
| const globalConfig = join(workRoot, "global.gitconfig"); | ||
| const systemConfig = join(workRoot, "system.gitconfig"); | ||
| const configFile = join(workRoot, "gitconfig"); | ||
| const templateDir = join(workRoot, "template"); | ||
| mkdirSync(templateDir); | ||
| writeFileSync(globalConfig, ""); | ||
| writeFileSync(systemConfig, ""); | ||
| writeFileSync(configFile, ""); | ||
|
|
||
| return { | ||
| EDITOR: "true", | ||
| GIT_ASKPASS: "/bin/echo", | ||
| GIT_CONFIG: configFile, | ||
| GIT_CONFIG_COUNT: "0", | ||
| GIT_CONFIG_GLOBAL: globalConfig, | ||
| GIT_CONFIG_SYSTEM: systemConfig, | ||
| GIT_EDITOR: "true", | ||
| GIT_EXEC_PATH: execSync("git --exec-path", { encoding: "utf8" }).trim(), | ||
| GIT_EXTERNAL_DIFF: "true", | ||
| GIT_PAGER: "cat", | ||
| GIT_PROXY_COMMAND: "true", | ||
| GIT_SEQUENCE_EDITOR: "true", | ||
| GIT_SSH: "ssh", | ||
| GIT_SSH_COMMAND: "ssh", | ||
| GIT_TEMPLATE_DIR: templateDir, | ||
| PAGER: "cat", | ||
| PREFIX: workRoot, | ||
| SSH_ASKPASS: "/bin/echo", | ||
| }; | ||
| } | ||
|
|
||
| async function expectUnsafeEnvRejected(git: SimpleGit): Promise<void> { | ||
| try { | ||
| await git.raw(["status", "--short"]); | ||
| } catch (err) { | ||
| expect(String(err)).toContain("not permitted without enabling allowUnsafe"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
makeBlockedGitEnv and expectUnsafeEnvRejected are copied verbatim in both git-client.test.ts and packages/host-service/src/runtime/git/simple-git.test.ts. If the blocked env list changes, both copies need to be updated. Since @superset/shared already centralizes the options, a shared test utility would keep these in sync.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/lib/trpc/routers/workspaces/utils/git-client.test.ts
Line: 11-50
Comment:
**Duplicate test helpers across two test files**
`makeBlockedGitEnv` and `expectUnsafeEnvRejected` are copied verbatim in both `git-client.test.ts` and `packages/host-service/src/runtime/git/simple-git.test.ts`. If the blocked env list changes, both copies need to be updated. Since `@superset/shared` already centralizes the options, a shared test utility would keep these in sync.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Fixes #4599
Supersedes #4600
What changed
PAGER/GIT_PAGERstripping workaround with a simple-git configuration that explicitly allows the user's Git environment/config hooks.@superset/shared/simple-git-optionsso desktop and host-service use the same policy.simpleGit(...)usage outside approved wrappers.Why
The
allowUnsafeEditor/allowUnsafePagerfailures are thrown bysimple-git@3.36.0, not by Git itself. Superset is a local Git client, so honoring the user's local Git environment is expected. Deleting individual env vars treated one symptom and changed user Git semantics.Validation
bun test packages/shared/src/simple-git-options.test.ts apps/desktop/src/lib/trpc/routers/workspaces/utils/git-client.test.ts packages/host-service/src/runtime/git/simple-git.test.ts apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.test.tsbun run lintbun run typecheckinpackages/sharedbun run typecheckinpackages/host-servicebun run typecheckinapps/desktopallowUnsafeEditorandallowUnsafeAlias; both cases failed as expected before restoration.Summary by cubic
Fixes
simple-gitunsafe env errors by allowing user Git env/config and centralizing options to honor local Git behavior. Removes thePAGERstripping workaround and routes production Git usage through approved wrappers.Bug Fixes
simple-git@3.36unsafe flags via@superset/shared/simple-git-optionsso editor/pager/hooks work as expected.PAGER/GIT_PAGERin desktop shell env.Refactors
createUserSimpleGitwrapper (host-service) and use shared options in desktop and host-service.simple-gitusage with wrappers in production paths.scripts/check-simple-git-usage.shand run it inscripts/lint.shto forbid direct imports/constructors outside tests and approved wrappers.Written for commit cdfe2fc. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Tests