Conversation
Test Coverage ReportNo TypeScript source files changed in this PR. |
Greptile SummaryThis PR makes the daemon child reaping integration test headless by replacing the
Confidence Score: 5/5Safe to merge; the test refactor removes real network/dashboard/browser side-effects and correctly wires the fake state to the ao stop --all code path via HOME-scoped file fixtures. The HOME-mutation approach correctly threads through both the test-process registry calls and the subprocess reads. The sweepRegisteredDaemonChildren chain is exercised end-to-end. The only weakness is that the final getDaemonChildren() assertion is redundant once both exit checks pass, but this does not represent incorrect behaviour. No files require special attention; the single changed file is a test and the production code paths it exercises are unmodified.
|
| Filename | Overview |
|---|---|
| packages/integration-tests/src/daemon-children.integration.test.ts | Test refactored to inject fake running.json and an in-process registered child, exercising ao stop --all without starting the dashboard; logic is sound but the final getDaemonChildren() assertion is trivially satisfied by the preceding exit checks, weakening its ability to detect sweep-cleanup regressions. |
Sequence Diagram
sequenceDiagram
participant T as Test (Vitest)
participant DP as daemonParent (sleeper)
participant RC as registeredChild (sleeper)
participant CLI as ao stop --all subprocess
participant FS as ~/.agent-orchestrator (tmpHome)
T->>T: "beforeEach: set HOME=tmpHome, clearDaemonChildrenRegistry()"
T->>DP: spawn spawnSleeper()
T->>RC: spawn spawnSleeper()
T->>FS: "registerDaemonChild({pid:childPid, parentPid:daemonPid})"
T->>FS: writeFakeRunningState(tmpHome, daemonPid)
T->>CLI: "execFileAsync(node cliEntry stop --all, HOME=tmpHome)"
CLI->>FS: getRunning() reads running.json finds daemonPid alive
CLI->>FS: sweepRegisteredDaemonChildren(daemonPid)
CLI->>RC: killProcessTree(childPid, SIGTERM)
CLI->>FS: pruneSweptDaemonChildren() clears daemon-children.json
CLI->>DP: killProcessTree(daemonPid, SIGTERM)
CLI->>FS: unregister() removes running.json
CLI-->>T: stdout contains Swept 1 registered daemon child(ren)
T->>T: "assert stdout, waitForChildExit x2, getDaemonChildren()==[]"
T->>T: afterEach: terminateChild, clearRegistry, restore HOME, rm tmpHome
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/integration-tests/src/daemon-children.integration.test.ts:155
**Registry-cleanup assertion is trivially satisfied by the preceding exit checks**
`getDaemonChildren()` filters entries with `isProcessAlive` before returning — if both processes are already confirmed dead by the `registeredChildExited` / `daemonParentExited` assertions above, this call will return `[]` regardless of whether `ao stop --all` actually called `pruneSweptDaemonChildren`. A regression where the sweep runs but the file-cleanup step is skipped would not be caught here, because the dead-process auto-prune inside `getDaemonChildren` masks it. To make this a meaningful assertion, read the raw file after the stop (e.g., check `existsSync` of the daemon-children registry file) before the processes have been confirmed exited, or call it synchronously right after `execFileAsync` returns and before the `waitForChildExit` race resolves.
Reviews (2): Last reviewed commit: "test(integration): address daemon reapin..." | Re-trigger Greptile
There was a problem hiding this comment.
Pull request overview
This PR makes the daemon child reaping integration test fully headless by removing its dependency on ao start (and the dashboard auto-open side effects) while still exercising the ao stop --all cleanup path via a fake running.json and a registered daemon child.
Changes:
- Reworked the test to write a fake
~/.agent-orchestrator/running.jsonand register a dummy daemon child in the daemon-children registry. - Executes
ao stop --alldirectly and asserts both the registered child and the dummy “daemon parent” process exit. - Removes random port allocation, dashboard startup, and browser-opening behavior from the test.
Fixes #1964
Summary
ao startpath with fakerunning.jsonstate and a registered dummy daemon child.ao stop --alldirectly and assert both the registered child and dummy daemon parent exit.Tests
pnpm buildpnpm typecheckpnpm lint(passes with existing warnings)pnpm --filter @aoagents/ao-integration-tests test -- daemon-children.integration.test.tspnpm test(fails locally on pre-existing/environment-specific tests:notifier-desktop.integration.test.tsexpectsosascriptwhile this macOS host resolvesterminal-notifier, andpath-equality.test.tssees/varvs/private/var; both failures reproduce independently of this change)