Skip to content

fix(windows): keep background work alive and hide every child process window - #1775

Open
SaulMoro wants to merge 11 commits into
Tencent:mainfrom
SaulMoro:test/windows-background-lifetime
Open

SaulMoro wants to merge 11 commits into
Tencent:mainfrom
SaulMoro:test/windows-background-lifetime

Conversation

@SaulMoro

@SaulMoro SaulMoro commented Nov 6, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

#1761 dropped detached on Windows from the five spawns whose child must outlive its parent. Without it, libuv puts a Windows child in a kill-on-close job the parent holds, so the child ends when the parent exits: the workflow resume a session start's pull begins dies with the --bg-only pass, and its run stays running. This puts detached: true back at those five sites and keeps #1761's headless conhost for the WMI launch, which is what hides the background pass.

hook-dispatch-cli  background pass, plain fallback   detached: !win32 -> true
hook-handlers      pull after a git hook             detached: !win32 -> true
hook-handlers      workflow resume                   detached: !win32 -> true
local-agent        source reconcile-plugins          detached: !win32 -> true
skill-runner       workflow runner                   detached: !win32 -> true

It also closes #1763: a teamai/windows-hide rule fails a node:child_process call in production source (src/, outside src/__tests__/) whose options object literal does not set windowsHide: true, and the 56 calls it found on main now set it, whereOnWindows in src/utils/cli-path.ts among them. The rule follows the module through named, namespace and default imports, await import(), require and promisify.

Options built elsewhere are reported rather than trusted: the call site spreads them into a literal that sets the option ({ ...options, windowsHide: true }). A variable can be built far from the call, so only a literal keeps the option checkable at the call; a shared helper would be one more hop for the same line. No call needs a visible window, so nothing is suppressed. simple-git stays out of scope, as the issue says.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Test Plan

New src/__tests__/e2e/background-lifetime-windows.test.ts, run as the last step of the windows CI job, drives the built CLI on the runner:

  • (a0) control: a --bg-only pass started by a parent that stays alive reaches its prePull script under a temp path that is a file.
  • (a) the same temp path fails the WMI payload write, so the dispatcher falls back to its own spawn; the pass still reaches prePull after the dispatcher exits, and logs its summary once released.
  • (b) the WMI launch works; the teamai-finished-sessions run the --bg-only pass starts holds on a gate until every --bg-only process is gone, then succeeds.

On the Windows runner, the head before rebasing onto a docs-only commit (same code):

✓ (a0) control: with its parent alive, a --bg-only pass under the same unusable temp path reaches its prePull script  2385ms
✓ (a) WMI launch fails: the --bg-only pass the dispatcher starts finishes its handlers after the dispatcher exits  4450ms
✓ (b) WMI launch works: the workflow resume the --bg-only pass starts ends its run after that pass exits  8199ms
Tests  3 passed (3)

Without the fix, (b) failed on every run: the run stayed running and no workflow resume process was alive.

For #1763, after npm run build, a session start through the real CLI on macOS, with a user-scope install of a local team repo, in a git repository with no remote, reaches the fixed git remote get-url origin call in mr-hint.ts:

$ echo '{"cwd":"<tmp>/app","session_id":"wh-record"}' | HOME=<tmp>/home node dist/index.js hook-dispatch session-start --tool claude
dispatch exit=0
[DEBUG] mr-hint: no git remote, skipping
[DEBUG] [hook-dispatch] session-start/claude foreground: 89ms total, 7 handlers, ... mr-hint=64ms(ok) ...
[DEBUG] [hook-dispatch] session-start/claude background: 601ms total, 3 handlers, slowest pull=601ms | pull=601ms(ok) ...

whereOnWindows runs only on Windows; the windows job's providers-cli-cmd step calls it there.

  • npm run typecheck and npm run lint pass
  • npx vitest run passes (22720 tests), and npx vitest run oxlint-plugin-teamai passes (639, 18 of them the new rule's ruleHarness fixtures)
  • Added/updated tests for the change

Related Issues

Fixes a regression from #1761. Closes #1763. Related to #1747. #1805 was fixed by #1845.

Notes for Reviewers

  • On Windows detached also sets DETACHED_PROCESS, which cancels CREATE_NO_WINDOW, so a console program that the resume, the runner or the git-hook pull starts (git, an AI tool) may open a window again. The --bg-only pass itself still launches hidden through WMI. Keeping that work finishing came first; hiding its descendants is a follow-up.
  • Wrapping these spawns in conhost --headless the way the WMI launch does was tried and does not work: a detached conhost never starts its child.
  • Case (b) writes ~/.teamai in the runner's own profile, because a WMI-created process does not inherit the caller's environment. It runs only under GITHUB_ACTIONS, sets aside a ~/.teamai left by earlier steps and restores it, and stays the last step of the job.

@SaulMoro
SaulMoro force-pushed the test/windows-background-lifetime branch from 0551216 to b74ee6f Compare November 6, 2026 17:21
@SaulMoro SaulMoro changed the title fix(hooks): keep Windows background work alive after its parent exits fix(windows): keep background work alive and hide every child process window Nov 6, 2026
@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown
  • [P2 non-blocking] oxlint-plugin-teamai/rules/windows-hide.js:38 — hidesWindow accepts any windowsHide: true property, even when a later spread overrides it. For example, spawn('git', [], { windowsHide: true, ...options }) passes lint when options.windowsHide is false, defeating the rule’s guarantee. Reject potentially overriding spreads after the property and add a fixture.

The PR description documents sufficient testing, including representative Windows real-CLI runs and a post-build macOS CLI run. The older-head Windows record is not blocking under the review rules.

Review was read-only; nothing from the PR was run, built, or installed.

@SaulMoro

SaulMoro commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed in b96d2ce. The last spread or windowsHide property in the object now decides, so { windowsHide: true, ...options } and { windowsHide: true, windowsHide: false } are rejected, with a fixture for both. The stricter check found two calls in src/scorers/scoring-evidence.ts that put windowsHide before ...timeLeft(gitIn). That spread only ever adds timeout, but both calls now put windowsHide: true last.

@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown
  • [P3 nit] oxlint-plugin-teamai/rules/windows-hide.js:45 — Computed properties are ignored when checking overrides. execSync('git status', { windowsHide: true, ['windowsHide']: false }) passes lint despite disabling window hiding. Recognize computed literal keys and add a fixture.

The earlier spread-override finding is resolved: later spreads and non-computed overrides are rejected, with fixture coverage.

The PR description documents sufficient testing, including Windows real-CLI runs and a post-build macOS CLI run. The older-head Windows record is not blocking under the trusted review rules.

Review was read-only; nothing from the PR was run, built, or installed.

@SaulMoro

SaulMoro commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed in the latest commit. A computed key now counts like a spread: it may name windowsHide, so windowsHide: true must come after it. A computed key also never counts as windowsHide: true itself, so { [key]: true } is rejected. Both cases have a fixture.

@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown

No new findings in the scoped diff.

  • Both earlier findings are resolved: later spreads, explicit overrides, and computed keys are rejected, with fixture coverage.
  • The PR description documents sufficient testing, including representative Windows real-CLI runs and a post-build macOS CLI run. The older-head Windows record is not blocking under the trusted review rules.

Review was read-only; nothing from the PR was run, built, or installed.

@github-actions

github-actions Bot commented Nov 7, 2026

Copy link
Copy Markdown

No new findings in the scoped diff.

  • Both earlier findings are resolved: later spreads, explicit overrides, and computed keys are rejected, with fixture coverage.
  • The PR description documents sufficient testing, including representative Windows real-CLI runs and a post-build macOS CLI run. The older-head Windows record is not blocking under the trusted review rules.

Review was read-only; nothing from the PR was run, built, or installed.

{ windowsHide: true, ...options } passed the rule while options could set it back to false. The last spread or windowsHide property now decides.
{ windowsHide: true, ['windowsHide']: false } passed the rule. A computed key may name windowsHide, so it now counts like a spread, and it never stands in for windowsHide: true.
…n place on a failed e2e setup

Tencent#1777 added a spawnSync('ps') without windowsHide, which the new
teamai/windows-hide rule rejects once this branch sits on main.

Case (b) of the Windows lifetime e2e moved ~/.teamai aside before its
setup, outside the try/finally that restores it. Move it aside last.
…rebase

The hook fast path's background child, the clean-filter hash-object in team history, and the insteadOf lookup in session sync now pass windowsHide: true, as teamai/windows-hide requires.
@SaulMoro
SaulMoro force-pushed the test/windows-background-lifetime branch from e2f435f to 6645313 Compare November 9, 2026 22:23
@github-actions

github-actions Bot commented Nov 9, 2026

Copy link
Copy Markdown

No new findings in the scoped diff.

  • Both earlier findings are resolved: later spreads, explicit overrides, and computed keys are rejected, with fixture coverage.
  • The PR description documents sufficient testing, including representative Windows real-CLI runs and a post-build macOS CLI run. The older-head Windows record is not blocking under the trusted review criteria.

Review was read-only; nothing from the PR was run, built, or installed.

@github-actions

github-actions Bot commented Nov 9, 2026

Copy link
Copy Markdown

No new findings in the scoped diff.

  • Both earlier findings are resolved: later spreads, explicit overrides, and computed keys are rejected, with fixture coverage.
  • The PR description documents sufficient testing, including representative Windows real-CLI runs and a post-build macOS CLI run. The older-head Windows record is not blocking under the trusted review criteria.

Review was read-only; nothing from the PR was run, built, or installed.

@wstczyw

wstczyw commented Nov 10, 2026

Copy link
Copy Markdown

Data point from a Windows 11 host for this PR (teamai-cli 0.29.0, Windows Terminal 1.24 as the default terminal), since the triage of #1747 noted that no Windows host was available:

  • Each Claude Code session start produced about 14 visible Windows Terminal windows. Chain: hook-dispatch session-start --bg-only (WMI child) → score-sessions (spawned with detached: true, so it has no console) → whereOnWindows() → where.exe <tool> without windowsHide. Each where.exe was followed 40–100 ms later by OpenConsole.exe -Embedding and WindowsTerminal.exe -Embedding (parent svchost).
  • whereOnWindows in src/utils/cli-path.ts on main still has no windowsHide, as Lint rule: require windowsHide: true on every child_process call in production source #1763 notes.
  • Adding windowsHide: true to whereOnWindows and to 17 other console spawns reachable from score-sessions and the detached pull (patched in dist/cli.js) stopped it. In a repeat session start: 8 where.exe calls, 0 OpenConsole / WindowsTerminal -Embedding, 0 new visible top-level windows.
  • The other sites: taskkill on judge abort, the sqlite3 CLI in the workbuddy and cursor stores, git in session-flow sync and in scoring-evidence, git --version and git cat-file --batch in src/utils/git.ts, git cat-file --batch-check in self-improvement, git ls-files in the repo and env resources, and the post-pull / team-scripts node spawns when stdio is not inherited. The lint rule from Lint rule: require windowsHide: true on every child_process call in production source #1763 should catch all of them.

@STiFLeR7

Copy link
Copy Markdown
Collaborator

@SaulMoro I resolved the three conflicts with main in merge commit 6656b913. Could you fast-forward test/windows-background-lifetime to that commit and push it to your fork?

git switch test/windows-background-lifetime
git fetch https://github.com/STiFLeR7/teamai-cli.git qa/1775-merge
git merge --ff-only FETCH_HEAD
git push origin test/windows-background-lifetime

Typecheck, Oxlint, 160 focused tests, and two isolated Windows subprocess end-to-end tests passed on the merge. The WMI-success case still needs the disposable CI runner.

@SaulMoro

Copy link
Copy Markdown
Collaborator Author

This was generated by AI during triage.

@STiFLeR7 thanks, I took your merge as is: 6656b913 is on the branch with your authorship. Main had moved since (#1827, #1831 and #1832), so f07cae0a merges 33fe3bbb on top. That merge had no conflicts. Typecheck and npm run lint pass, and so do 688 tests across the files the merge touches: hook-fast-path, mr-hint, pid-monitor, git/push, YAML merge, and the windows-hide rule.

@wstczyw thanks for the data point, it matches what this PR fixes. whereOnWindows in src/utils/cli-path.ts sets windowsHide: true on this branch. The other sites you list are covered too: the teamai/windows-hide lint rule fails any spawn/exec* call without it, including one where a later spread or a computed key could override it, and lint passes on the merged head.

@github-actions

Copy link
Copy Markdown

No new findings in the scoped diff.

  • Both earlier findings are resolved: later spreads, explicit overrides, and computed keys are rejected, with fixture coverage.
  • The PR description documents sufficient testing, including representative Windows real-CLI runs and a post-build macOS CLI run. The older-head Windows record is not blocking under the trusted review criteria.

Review was read-only; nothing from the PR was run, built, or installed.

# Conflicts:
#	src/team-scripts.ts
@github-actions

Copy link
Copy Markdown

No new findings in the scoped diff.

  • Both earlier findings are resolved: later spreads, explicit overrides, and computed keys are rejected, with fixture coverage.
  • The PR description documents sufficient testing, including representative Windows real-CLI runs and a post-build macOS CLI run. The older-head Windows record is not blocking under the trusted review criteria.

Review was read-only; nothing from the PR was run, built, or installed.

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.

Lint rule: require windowsHide: true on every child_process call in production source

4 participants