fix(server): a pushed commit no longer reports failure when the final git status read fails by only21mil · Pull Request #32705 · pingdotgg/t3code

only21mil · 2026-11-07T23:06:35Z

Problem

After Push, Commit & Push, Create PR or Commit, Push & PR, an unguarded final git-status read while building the completion toast could report "Action failed" with no success state or PR link after the commit and push had already landed, for example when another git process held .git/index.lock and the read returned "Git index is locked". Expected: the normal success state, as when nothing holds the lock.

Change

In buildCompletionToast, the final statusDetails read is now best-effort (Effect.orElseSucceed(() => null)), like the existing open-PR lookup right below it. When the read fails, the toast skips the "Create PR" button, because it cannot tell whether the branch is the default branch. A PR link from the action's own result still shows. Nothing else changes.

Scope

A bug fix: the server reports an error for work that already succeeded. It is a small fix of an obvious bug in one function of apps/server/src/git/GitManager.ts plus one test, with no contract or client change, so it needs no issue. The client-side message that hides the real failure reason is a separate problem and is not part of this PR.

Verification

To reproduce against the server: (1) give a test repo's bare remote a post-receive hook that starts a detached git commit -a --allow-empty in the working repo with inherited push descriptors closed, waits for .git/index.lock, then returns while the editor waits 90 seconds; (2) commit a README change on a branch from a shell; (3) click Push in the web client's git actions menu. main shows "Action failed"; this PR shows Pushed to origin/<branch> with the lock still held.

I ran exactly that in the Browser panel against a fresh test server on macOS per revision: main (2de337003d) first, then this head (e0b50fabd6), then this head merged locally with main (2de337003d). In all three runs the commit reached the remote and the lock was still held when the action finished. On Node 26.12.1, the new test passes on the merge and fails with main's GitManager.ts. A Linux verification of this exact head used a real post-receive hook: "Action failed" on main, "Pushed to origin/..." on this head with the lock present.

Re-checked 2026-11-12 on Linux against main f419698aea: "Action failed" there, "Pushed" on the clean local merge 9793b48fa9; the test fails on that main, passes here.

Before, on main (2de337003d): Action failed with the locked-index error

After, on this head (e0b50fabd6): Pushed to origin/feature/readme-update, no Create PR action

After, on the local merge (623cc6ac7a): Pushed to origin/feature/readme-update

Not checked: the test covers commit_push and the captures cover push. create_pr and commit_push_pr reach the same changed line. Desktop and mobile show the same server result and were not run separately.

Cause, the test, test output and the main run's repo state

Since #9845, the status read fails fast with "Git index is locked" whenever index.lock exists. Any other git process can hold that lock for a moment: an editor's git integration, a terminal, a hook, or an agent in the same worktree. The action then reports action_failed, and a retry fails with "nothing to commit" or "PR already exists".

New case in apps/server/src/git/GitManager.test.ts, "finishes a pushed commit when the final status read fails". It uses real git and a real bare remote. The existing spawn gate holds the action just after git push exits, the test creates .git/index.lock, then releases the gate. It checks that the action succeeds, push.status is pushed, the toast offers no Create PR button, the last progress event is action_finished, and the commit is on the remote.

On this head (e0b50fabd6, on its base 25bf84ab07) the whole file passed (Tests 126 passed (126)); with the new finalStatus !== null check taken out of the Create PR condition, the case failed on expected { kind: 'run_action', … label: 'Create PR' } to deeply equal { kind: 'none' }; vp run check:changed passed.

On Node 26.12.1, I compared the local merge 623cc6ac7a (this head merged with main 2de337003d) with the same merge using main's GitManager.ts; these log excerpts list the failing result first:

$ grep -E "^node | FAIL |Tests |^exit=" test-m05-mainfile.txt test-m05.txt
test-m05-mainfile.txt:node v26.12.1
test-m05-mainfile.txt:⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯
test-m05-mainfile.txt: FAIL  src/git/GitManager.test.ts > GitManager > finishes a pushed commit when the final status read fails
test-m05-mainfile.txt:      Tests  1 failed | 131 passed (132)
test-m05-mainfile.txt:exit=1
test-m05.txt:node v26.12.1
test-m05.txt:      Tests  132 passed (132)
test-m05.txt:exit=0

The hook's git commit -a runs with an editor that waits 90 seconds, then aborts on the empty message and commits nothing. Each run committed a README change on feature/readme-update from a shell before clicking Push. The test repo on main right after the action, with the lock held by the hook's git process and the commit on the remote. The block is the output of the lane's state script, which runs date -u +%FT%TZ, test -e .git/index.lock, git rev-parse HEAD, git branch --show-current and git ls-remote origin refs/heads/feature/readme-update in the working repo, then prints the hook's log and ps -o pid=,command= -p <holder pid>; the script shortens the full path of the git binary to git and its first line, an echoed summary of these commands, is left out:

2026-11-12T08:39:12Z
.git/index.lock: present
local  HEAD d2aa2cb16a41613d38f85fec574c0f3898e97158 (feature/readme-update)
remote d2aa2cb16a41613d38f85fec574c0f3898e97158	refs/heads/feature/readme-update
--- post-receive hook log (sample-remote.git)
0000000000000000000000000000000000000000 d2aa2cb16a41613d38f85fec574c0f3898e97158 refs/heads/feature/readme-update
holder pid 3726 started 2026-11-12T08:39:02Z
lock present: /tmp/demo-main2/sample/.git/index.lock at 2026-11-12T08:39:02Z
  PID COMMAND
 3726 git -C /tmp/demo-main2/sample -c user.name=Test Committer -c [email protected] commit -a --allow-empty
--- holder process now
 3726 git -C /tmp/demo-main2/sample -c user.name=Test Committer -c [email protected] commit -a --allow-empty

Generated with Claude Opus 5.5 and GPT-6.1 Sol in T3 Code Nightly

…git action The completion toast read git status after the commit and push had landed. If that read failed, for example on a locked index, the whole action failed. The read is now best-effort like the PR lookup next to it, and the Create PR button is left out when the branch state is unknown. Generated with Claude Opus 5.5 in T3 Code Nightly

macroscopeapp · 2026-11-07T23:08:27Z

Approvability

Verdict: Approved at e0b50fa

Macroscope's review found this PR approvable — Completed Git actions no longer report failure solely because the final status refresh fails. The fallback omits branch-dependent suggestions without changing commit, push, or PR execution.

^{You can add or adjust custom eligibility rules. Learn more.}

coderabbitai · 2026-11-07T23:12:07Z

Walkthrough

Completion toast generation now handles unavailable final Git status. A regression test checks that a commit-and-push action completes when the final status read fails after the push.

Changes: Completion toast status handling

Layer / File(s)	Summary
Handle unavailable final status `apps/server/src/git/GitManager.ts`, `apps/server/src/git/GitManager.test.ts`	Commit actions skip the final status read. Other status-read failures are treated as unavailable status, and the Create PR CTA requires an available final status. The regression test checks that the push succeeds, the toast has no CTA, an `action_finished` event is published, and the commit appears on the remote.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge, marcodotio

Merge Risk: Minimal · up to e0b50

A successful push can complete even when the final status read fails; the toast omits the Create PR button when branch status is unknown. No actionable merge risk is established.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title names the main change: a pushed commit no longer reports failure when the final git status read fails. It matches the code change, which makes the final statusDetails read best-effort in bui…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description check		The description follows the repository template closely. It has Problem, Change, Scope, and Verification sections, and each is filled in with specific detail. The Problem section states the expected b…

Full details: Title check

Explanation

The title names the main change: a pushed commit no longer reports failure when the final git status read fails. It matches the code change, which makes the final statusDetails read best-effort in buildCompletionToast, and the added regression test. It follows the conventional commit format from the template and is specific enough for a teammate scanning history. It is a little long at 91 characters.

Full details: Description check

Explanation

The description follows the repository template closely. It has Problem, Change, Scope, and Verification sections, and each is filled in with specific detail. The Problem section states the expected behavior and the lock-related reproduction. The Change section explains the best-effort read and the Create PR button behavior. The Scope section explains why the change is a bug fix and needs no issue. The Verification section gives reproduction steps, observed results on main and on the head, a named regression test, test output, and an explicit list of what was not checked (create_pr and commit_push_pr, desktop and mobile). The template's opening comment asks for a conventional commit title. The description does not show a title, so the title cannot be confirmed from this text. The author also names the agent model used, which the template asks for. The description is long and includes some detail beyond the template, such as the collapsed log section, but the required sections are complete.

Finishing Touches

Generate unit tests (beta)

Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

coderabbitai

Additional comments: 1

apps/server/src/git/GitManager.ts: 1
1948-1951: Trivial · Maintainability & Code Quality · Low value

Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Verify that swallowing every statusDetails failure is intended.

Effect.orElseSucceed(() => null) converts every GitCommandError into an unavailable status, not only the index-lock case from the PR. The effect is limited here. The commit and push have already succeeded, and the only consequence is a missing "Create PR" CTA and no default-branch or upstream context. That is acceptable for this read.

The silent conversion leaves no trace of why the CTA is missing. Log the failure at warning level, as other best-effort reads in this file do (for example readThreadContext). This keeps the failure diagnosable. The retrieved learning also asks that git errors for actionable conditions should not be blanket-swallowed. A post-success read has no action the user can take, so a warning log is enough.
Proposed change
-        : yield* gitCore.statusDetails(cwd).pipe(Effect.orElseSucceed(() => null));
+        : yield* gitCore.statusDetails(cwd).pipe(
+            Effect.catch((error) =>
+              Effect.logWarning("GitManager.buildCompletionToast final status unavailable", {
+                errorTag: error._tag,
+              }).pipe(Effect.as(null)),
+            ),
+          );
Based on learnings: "don't blanket-swallow errors. Only silently handle known benign failure cases ... with targeted catch logic."

Source: Learnings

Review info

Run configuration

Source mode: Auto

Repository: pingdotgg/t3code

Branch: main

File: .coderabbit.config.ts

Commit: a9e709f

Layers: Repository TS config

Review profile: CHILL

Plan: Advanced

Run ID: `28e82fe0-1779-46b1-a467-c7931b991827`

Commits

Reviewing files that changed from the base of the PR and between `a9e709f` and `e0b50fa`.

Files selected for processing: 2

`apps/server/src/git/GitManager.test.ts`

`apps/server/src/git/GitManager.ts`

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

maria-rcks · 2026-11-10T06:35:00Z

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

verified as-is at e0b50fabd649b4ca7ec47a8c3b8796951053be77 on a disposable linux boat vm. used the real dev web app, repository playwright, and /usr/bin/google-chrome. added the project through the app and ran real codex gpt-6.1-sol turns on main and the pr to create and edit a real git repository. no mocks or demo data.

reproduced on github/main at 934c7f8d8e: a real bare remote's post-receive hook created the working repository's index lock after accepting the push. the app reported "action failed" even though local and remote refs both held b8dce2804df4d94bdb5996dc46b376b01f1e6584.
stopped main, installed the exact pr head, and repeated the same first-push scenario with the same app data and commit. the app showed "pushed to origin/feature/status-after-push" while the lock remained present. the completion state offered no create-pr action. the remote ref matched.
neighboring behavior: committed a real provider edit through the commit dialog. a real pre-receive rejection still reported "action failed" and left the remote unchanged. removing the rejection and retrying push succeeded, advanced the remote to 860e667ae7703d3fc31f01c7dab3481ea90c31a3, and left a clean working tree. the ordinary create-pr action remained available.
blacksmith: vp i && cd apps/server && vp test run src/git/GitManager.test.ts passed all 126 tests, including the final-status regression. existing ci reports 30 successful checks, zero pending or failing. no source changes or push were needed.

command transcript

captures inspected; uploaded video playback and seeking checked in chrome.

Not verified

native desktop, ios and android clients; macos/windows; relay and tunnel connections; providers other than codex. live create-pr and commit-push-pr flows were not exercised because this verification used a local bare remote and did not create another external pr. the existing tests cover those paths, but that is not live integration proof. commit-push with the lock is covered by the existing regression test; the live locked-index case above used push.

only21mil · 2026-11-10T06:37:05Z

Note

🤖 Opus 5.5 replying on behalf of Victor.

Thanks @maria-rcks for testing this end to end in a real app session. Your main run reproduced the exact bug this targets: the push went through, both refs matched, and the app still said "action failed" because a remote hook held the index lock. On e0b50fabd6, the same push reports success with the lock still held. Your neighboring checks matter just as much. A rejected push still reports failure and leaves the remote unchanged, and the create-PR action is still offered after a clean push. So the fallback only covers the final status read and doesn't hide real failures.

Your notes on what you didn't cover are fair. That includes the native desktop and mobile clients, macOS and Windows, relay and tunnel connections, other providers, and live create-PR or commit-push-PR flows. On those paths only GitManager.test.ts covers it. There's no live run.

github-actions Bot added vouch:unvouched Author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Nov 7, 2026

macroscopeapp Bot approved these changes Nov 7, 2026

View reviewed changes

coderabbitai Bot reviewed Nov 7, 2026

View reviewed changes

coderabbitai Bot mentioned this pull request Nov 10, 2026

fix(server): a git action that fails partway refreshes the Git panel #29651

Merged

Navigation Menu

Repository navigation

fix(server): a pushed commit no longer reports failure when the final git status read fails - #32705

fix(server): a pushed commit no longer reports failure when the final git status read fails#32705
only21mil wants to merge 1 commit into
pingdotgg:mainfrom
only21mil:fix/git-action-final-status-read

only21mil commented Nov 7, 2026 •

edited

Loading

Uh oh!

macroscopeapp Bot commented Nov 7, 2026

Uh oh!

coderabbitai Bot commented Nov 7, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

maria-rcks commented Nov 10, 2026 •

edited

Loading

Uh oh!

only21mil commented Nov 10, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

only21mil commented Nov 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Change

Scope

Verification

Uh oh!

macroscopeapp Bot commented Nov 7, 2026

Approvability

Uh oh!

coderabbitai Bot commented Nov 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

maria-rcks commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Verified in a real dev environment

Not verified

Uh oh!

only21mil commented Nov 10, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

only21mil commented Nov 7, 2026 •

edited

Loading

coderabbitai Bot commented Nov 7, 2026 •

edited

Loading

maria-rcks commented Nov 10, 2026 •

edited

Loading