Allow retrying git commits without signing when signing fails#4219
Allow retrying git commits without signing when signing fails#4219tim-smart wants to merge 13 commits into
Conversation
github-actions
Bot
added
vouch:unvouched
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
tim-smart
changed the title
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 6 blocking correctness issues found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
2de8119 to
779bf4e
Compare
| const failure: E | VcsActionRemoteFailureError = | ||
| terminal?.kind === "action_failed" ? remoteFailure(terminal) : error; | ||
| return Effect.fail<E | VcsActionRemoteFailureError>(failure); | ||
| }), |
There was a problem hiding this comment.
Success lost after stream error
Medium Severity
The new Effect.catch prefers a terminal action_failed over a trailing stream error, but still fails when action_finished was already received. A successful stacked action (including an unsigned retry) can surface as a failure if the RPC stream errors after the terminal event, so the UI can prompt another attempt and create a duplicate commit.
Reviewed by Cursor Bugbot for commit 779bf4e. Configure here.
| }); | ||
| currentHookName = null; | ||
| } | ||
| yield* finalizeUnattributedOutput(sawCommitHook); |
There was a problem hiding this comment.
Hookless failures hide git stderr
Medium Severity
Unattributed commit output is buffered while no hook is active, then flushed only when sawCommitHook is true. In repos without commit hooks, non-signing commit failures never emit those stderr lines as progress, so the UI keeps a generic “Git command exited with a non-zero status” message instead of the underlying git diagnostic.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 779bf4e. Configure here.
63e8682 to
c7907a7
Compare
- Let users pick macOS .app bundles with system icons - Add open-with IPC, launch resolution, and settings plumbing - Cover bundle parsing, availability, and launch behavior with tests
- Share Effect-based icon resolution across dialogs and Open With - Prefer bundle ICNS assets with Electron fallback
- Make the application path field editable and clear stale icon and validation state on change
- Apply cwd-specific `.envrc` changes across supported provider runtimes - Preserve provider invariants and surface safe, actionable direnv errors
- Run `direnv allow` for a root `.envrc` after creating a worktree - Keep worktree creation successful when approval fails
- Resolve approved direnv variables for provider processes and Git commands - Share live and no-op direnv layers with cached executable lookup
- Detect commit signing failures across server and client - Let stacked git actions retry once with signing disabled - Preserve unsigned commit behavior in commit, push, and PR flows
- Prevent stale action controls from appearing on Git progress toasts
c7907a7 to
90c665d
Compare
github-actions
Bot
removed
the
size:L
github-actions
Bot
added
the
size:XXL
| }); | ||
| export type OpenWithEntry = typeof OpenWithEntry.Type; | ||
|
|
||
| export const OpenWithEntries = Schema.Array(OpenWithEntry).check( |
There was a problem hiding this comment.
🟡 Medium src/openWith.ts:54
OpenWithEntries validates array length but does not enforce uniqueness of OpenWithEntry.id, so a decoded collection can contain multiple entries with the same id. Callers resolve entries with .find(candidate => candidate.id === input.entryId), which returns the first match — meaning when a duplicate id exists, launching the later entry silently launches the earlier one instead. Consider adding a uniqueness-by-id filter to the schema.
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/contracts/src/openWith.ts around line 54:
`OpenWithEntries` validates array length but does not enforce uniqueness of `OpenWithEntry.id`, so a decoded collection can contain multiple entries with the same `id`. Callers resolve entries with `.find(candidate => candidate.id === input.entryId)`, which returns the first match — meaning when a duplicate `id` exists, launching the later entry silently launches the earlier one instead. Consider adding a uniqueness-by-`id` filter to the schema.
| ); | ||
| }); | ||
|
|
||
| const entryIsAvailable = Effect.fn("desktop.openWith.entryIsAvailable")(function* ( |
There was a problem hiding this comment.
🟡 Medium shell/DesktopOpenWith.ts:59
entryIsAvailable marks any directory ending in .app as available without checking for Contents/Info.plist or a valid executable. In open-target mode the code passes the path straight to /usr/bin/open with no further validation, so an incomplete or fake .app directory is reported as available and open can succeed after detaching even though nothing actually launches. Consider validating the bundle structure (e.g. calling resolveMacBundleExecutable) before reporting the entry as available.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/shell/DesktopOpenWith.ts around line 59:
`entryIsAvailable` marks any directory ending in `.app` as available without checking for `Contents/Info.plist` or a valid executable. In `open-target` mode the code passes the path straight to `/usr/bin/open` with no further validation, so an incomplete or fake `.app` directory is reported as available and `open` can succeed after detaching even though nothing actually launches. Consider validating the bundle structure (e.g. calling `resolveMacBundleExecutable`) before reporting the entry as available.
| export function readLegacyPreferredEditor(): EditorId | null { | ||
| return getLocalStorageItem(LAST_EDITOR_KEY, EditorId); | ||
| } |
There was a problem hiding this comment.
🟡 Medium src/editorPreferences.ts:17
readLegacyPreferredEditor calls getLocalStorageItem directly, so a malformed or stale t3code:last-editor value causes OpenInPicker to throw LocalStorageOperationError during render instead of falling back. Unlike useLocalStorage, which catches decode failures, this path has no error handling. Wrap the call in a try/catch and return null on failure so invalid stored values fall back gracefully.
| export function readLegacyPreferredEditor(): EditorId | null { | |
| return getLocalStorageItem(LAST_EDITOR_KEY, EditorId); | |
| } | |
| export function readLegacyPreferredEditor(): EditorId | null { | |
| try { | |
| return getLocalStorageItem(LAST_EDITOR_KEY, EditorId); | |
| } catch { | |
| return null; | |
| } | |
| } |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/editorPreferences.ts around lines 17-19:
`readLegacyPreferredEditor` calls `getLocalStorageItem` directly, so a malformed or stale `t3code:last-editor` value causes `OpenInPicker` to throw `LocalStorageOperationError` during render instead of falling back. Unlike `useLocalStorage`, which catches decode failures, this path has no error handling. Wrap the call in a try/catch and return `null` on failure so invalid stored values fall back gracefully.
| const findDirenv = Effect.fn("DirenvEnvironment.findDirenv")(function* ( | ||
| environment: NodeJS.ProcessEnv, | ||
| ) { | ||
| const cacheKey = environment.PATH ?? ""; |
There was a problem hiding this comment.
🟡 Medium provider/DirenvEnvironment.ts:123
findDirenv uses only environment.PATH as its cache key, but on Windows command resolution also reads the case-insensitive Path key and depends on PATHEXT. Two environments that both omit PATH (or share the same PATH value) but differ in Path/PATHEXT collide in the cache. If the first lookup resolves to undefined, a later session whose Path actually contains direnv gets the cached undefined, so direnv is never invoked and its .envrc environment is silently skipped. Consider building the cache key from the normalized, case-insensitive PATH and including PATHEXT (or avoid caching across distinct environments).
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/DirenvEnvironment.ts around line 123:
`findDirenv` uses only `environment.PATH` as its cache key, but on Windows command resolution also reads the case-insensitive `Path` key and depends on `PATHEXT`. Two environments that both omit `PATH` (or share the same `PATH` value) but differ in `Path`/`PATHEXT` collide in the cache. If the first lookup resolves to `undefined`, a later session whose `Path` actually contains `direnv` gets the cached `undefined`, so direnv is never invoked and its `.envrc` environment is silently skipped. Consider building the cache key from the normalized, case-insensitive PATH and including `PATHEXT` (or avoid caching across distinct environments).
|
|
||
| if (entry.directoryMode === "open-target") { | ||
| if (entry.invocation.type === "mac-application") { | ||
| return { |
There was a problem hiding this comment.
🟡 Medium shell/DesktopOpenWith.ts:156
The open-target branch for mac-application entries builds the open command as /usr/bin/open -a <app> <directory>, silently discarding entry.arguments. A mac-application entry saved with custom arguments launches without any of those arguments reaching the application, while the command branch and both other directory modes all forward configured arguments. Consider including entry.arguments in the open invocation, e.g. via --args.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/shell/DesktopOpenWith.ts around line 156:
The `open-target` branch for `mac-application` entries builds the `open` command as `/usr/bin/open -a <app> <directory>`, silently discarding `entry.arguments`. A mac-application entry saved with custom arguments launches without any of those arguments reaching the application, while the `command` branch and both other directory modes all forward configured arguments. Consider including `entry.arguments` in the `open` invocation, e.g. via `--args`.
| options.spawn.command, | ||
| options.spawn.args, | ||
| options.spawn.env ? { env: options.spawn.env, extendEnv: true } : {}, | ||
| options.spawn.env ? { env: options.spawn.env, extendEnv } : {}, |
There was a problem hiding this comment.
🟡 Medium acp/AcpSessionRuntime.ts:337
When options.spawn.env is omitted, extendEnv: false is silently ignored — the spawned ACP process still inherits the parent environment. Both call sites that would use extendEnv replace the entire options object with {} whenever env is undefined, so the flag never reaches resolveSpawnCommand or ChildProcess.make. A caller who sets only extendEnv: false (without env) does not get an isolated environment. Pass extendEnv independently of env, or require env whenever extendEnv is false.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/acp/AcpSessionRuntime.ts around line 337:
When `options.spawn.env` is omitted, `extendEnv: false` is silently ignored — the spawned ACP process still inherits the parent environment. Both call sites that would use `extendEnv` replace the entire options object with `{}` whenever `env` is undefined, so the flag never reaches `resolveSpawnCommand` or `ChildProcess.make`. A caller who sets only `extendEnv: false` (without `env`) does not get an isolated environment. Pass `extendEnv` independently of `env`, or require `env` whenever `extendEnv` is `false`.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 90c665d. Configure here.
| entryId: entry.id, | ||
| executable: "Custom arguments must include {directory}", | ||
| }); | ||
| } |
There was a problem hiding this comment.
Wrong error for bad arguments
Low Severity
custom-arguments entries missing {directory} fail as OpenWithUnavailableApplicationError with the validation text stuffed into executable. Users see an “application unavailable” message for a configuration problem, which misstates the failure mode.
Reviewed by Cursor Bugbot for commit 90c665d. Configure here.
1 participant
Previously, when git signing failed (due to non-interactive environments etc),
the git commit would be aborted.
With this change, it allows you to retry with signing disabled.
Note
High Risk
Touches detached process spawning, provider session environments, git commit/signing paths, and new desktop IPC—multiple security- and workflow-sensitive areas in one PR.
Overview
This PR bundles several capabilities beyond the titled commit-signing work: desktop “Open With”, direnv-backed provider environments, and git commit signing retry with related server/git fixes.
Open With (desktop, macOS-focused) adds configurable client settings for launching folders in Terminal, other commands, or
.appbundles. The Electron layer gainspickApplication(Applications picker + optional icon viaMacApplicationIcon), IPC/preload hooks (pickOpenWithApplication,resolveOpenWithPresentations,openWith), andDesktopOpenWithto resolve availability, icons, and detached launches (open -a, cwd/argument modes). Tests cover dialog, IPC, and launch resolution.Direnv introduces
DirenvEnvironmenton the server: discover nearest.envrc, rundirenv export jsonwith an exact non-extending env (extendEnv: falseonprocessRunner), redact secrets in errors, andallowonly a worktree-root.envrc. All shipped provider drivers passresolveEnvironmentinto adapters so local sessioncwdgets a merged env (Claude/Codex/Cursor/Grok/OpenCode); external OpenCode URLs skip resolution.Git commit signing retry adds
disableCommitSigningthrough stacked actions, classifies signing failures (failureKind: commit_signing_failed), and on mobile shows an Alert to retry viabuildUnsignedCommitRetryInput.GitManagerimproves commit hook output attribution, forwards the unsigned override, and adds tests for signing failure/retry behavior.initRepotests disable GPG sign by default.Risk note: Large surface area—spawn/env handling, new desktop IPC, and git workflow changes.
Reviewed by Cursor Bugbot for commit 90c665d. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add unsigned commit retry when GPG signing fails in git commit workflow
commit_signing_failedas a classifiedGitActionFailureKind, detected by matching stderr against known GPG, pinentry, and SSH signing error patterns inGitVcsDriverCore.disableCommitSigningflag through the full stack: contracts, server-sideGitManager/GitVcsDriver, client-runtimevcsAction, and UI hooks, allowing a single commit attempt to bypass GPG signing via--no-gpg-sign.GitActionsControldetects signing failures and updates the toast with a🖇️ Linked Issues
"Retry without signing" action that re-runs the commit with
disableCommitSigning: true.use-selected-thread-git-actionsshows a native Alert prompting the user to retry without signing.OpenInPickeroverhaul andDirenvEnvironmentintegration for per-session environment resolution, but the signing retry is the primary behavioral change.extendEnvdefaulting changes inprocessRunnermean processes launched without an explicit env no longer inherit the parent environment by default."AC_linked_issues":""}Macroscope summarized 90c665d.