Skip to content

Git/GitHub cleanup: naming collision, sync-block hazard, e2e fixture duplication - #364

Merged
Olga Lavrichenko (OLavrik) merged 5 commits into
mainfrom
olga.lavrichenko/git
Sep 1, 2026
Merged

Git/GitHub cleanup: naming collision, sync-block hazard, e2e fixture duplication#364
Olga Lavrichenko (OLavrik) merged 5 commits into
mainfrom
olga.lavrichenko/git

Conversation

@OLavrik

Copy link
Copy Markdown
Collaborator

Three small, independent fixes found while auditing how git/GitHub plumbing is used across the codebase.

  1. Resolve a name collision in the git env builders
    packages/server/src/pr/pr.ts defined its own nonInteractiveGitEnv(base, hasSshCommandConfig) — a different function, with a different signature, from the git module's barrel-exported nonInteractiveGitEnv()
    (packages/server/src/git/gitExec.ts) that pr.ts already imports. Renamed the local one to pushGitEnv to remove the ambiguity.

  2. Move gh auth status off spawnSync onto the shared bounded runner
    github.ts's githubAuthStatus() ran gh auth status via a synchronous, unbounded Bun.spawnSync — capable of freezing the entire single-threaded host if gh ever hangs. Its sibling ghSetupProblem(), in the same
    file, already worked around this with a hand-rolled async TERM+KILL timeout — and subprocess/SPEC.md explicitly listed githubAuthStatus as known debt to migrate onto runBounded. Both probes now share
    runBounded at the same 8s budget; the hand-rolled escalation logic is gone. Wire handler and web client needed no changes (already promise-based).

  3. Centralize the duplicated raw-git e2e fixture helper
    ~10 e2e spec files each reimplemented their own execFileSync("git", ["-C", cwd, ...args]) wrapper (six near-identical local git(...) closures, plus two byte-identical commitFile helpers). Extracted a shared
    e2e/fixtures/git.ts (git, gitQuiet, gitText, gitAs, commitFile) and migrated every call site.

@jetbrains-air jetbrains-air Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes — please address the blocking inline finding.

Comment thread packages/server/src/github/github.ts

@jetbrains-air jetbrains-air Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes — please address the blocking inline finding.

Comment thread e2e/fixtures/git.ts Outdated

@jetbrains-air jetbrains-air Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved — ready to merge.

@danyaberezun
danyaberezun added this pull request to the merge queue Sep 1, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Sep 1, 2026
@OLavrik
Olga Lavrichenko (OLavrik) added this pull request to the merge queue Sep 1, 2026
Merged via the queue into main with commit 09f10af Sep 1, 2026
6 checks passed
@OLavrik
Olga Lavrichenko (OLavrik) deleted the olga.lavrichenko/git branch September 1, 2026 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants