Repository navigation
docs(agents): stop local simulators and emulators on close - #4166
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Apex SentinelVerdict: Since the last review: 0 new · 0 still open · 0 no longer raised No findings. Not in the reviewers' prompt (8 files)
Reply under an inline comment with Earlier reviewsReview of
|
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
I reviewed 9f2271c. The docs change has one problem that stops the merge: the Ownership and context budgets check fails. The added sentence in AGENTS.md takes the file from 3999 to 4071 bytes (limit 4000), and the device-verification.md addition pushes docs/agents to 40346 bytes (limit 40000). That required check is red, so the PR cannot merge. Please remove the AGENTS.md sentence, because device-verification.md already owns this rule and AGENTS.md routes device work there. Then cut the 4-line addition in device-verification.md to 1-2 lines and remove about 350 bytes of duplication elsewhere in docs/agents, as the assertion message says. That failure is likely related to this PR, since both files are edited here. I did not inspect the Coverage job log, so I cannot say if it relates to this change. Smoke Tests were still running when I reviewed. There are no conflicts. Not blocking, and you can take or leave these: device-verification.md:34 and the other texts state the same rule with three different exceptions (always for workers, only when the user says so, and unless the device must stay up), so one owner in device-verification.md and the close help with one exception wording would be clearer, and skills could keep the one-line command only; cli-help.ts:81 says to shut down any local simulator or emulator the session touched, while #4162 says to stop only devices that open booted, so please scope it to a device your session booted or that no other session uses; and app.ts:96 edits close metadata that #4162 will rewrite again. Could this PR be smaller? For example, add 1-2 lines to device-verification.md for repo workers (scoped to a device the worker booted, with --session), leave skills and help alone, and fix the overnight leak in #4162 so plain close stops a boot-owned device. For that, the session record needs to know the device was booted by the session, and close needs to stop it. Then the interim text can be deleted. I did not read the daemon close/shutdown handler. I took the behavior from #4162, sessions.md and the flag declarations. The open inline threads on the keep-up exception in AGENTS.md, dogfood skill exception, help versus docs exception, shutdown ownership check, missing --session, physical and provider devices and AGENTS.md scope still stand. Three lower-priority threads also still apply. Before merge, fix the two budget limits, unify the rule and its exception, and decide whether to wait for #4162. |
Agents finish with plain close, which never stops the device, while open boots a stopped one. Tell them to finish with close --shutdown on local simulators and emulators, in close help, workflow/manual-qa/validate help, the skills, and the repo agent docs. Refs #4162
Address review: one rule and one exception everywhere (the session booted the local simulator or emulator, unless the user asks to keep it running). Drop the AGENTS.md sentence; device-verification.md owns the rule. Remove two cleanup lines that duplicate CLI help and the line above them.
The workflow card was 9092 of 9100 bytes on main. Leave it unchanged; the close command help and the manual-qa/validate topics carry the rule.
9f2271c to
6fdb246
Compare
|
Addressed the review at 6fdb246 (rebased on main):
All 10 inline threads are answered and resolved. |
|
The changes look good at 6fdb246: the condition now comes first in the shutdown rule, which fixes what the earlier review at 9f2271c raised (#4166 (comment)). Not blocking: the same sentence now appears in 7 places with slight wording drift, and the skill contract test only checks the substring The inline thread on device-verification.md:34 (#4166 (comment)) is fixed at this head, since the line now starts with "If your session booted a local simulator or emulator, add --shutdown ...". It is already resolved. Repo Guards, Coverage and Integration Tests now pass; only Smoke Tests is still running. The diff only touches docs, help strings and a skill contract test, so it does not overlap the Coverage and Integration routes. I did not run |
thymikee
added
the
ready-for-human
Summary
After an overnight agent run, many local simulators were left running.
openboots a stopped simulator or emulator, but plainclosenever stops it, and no instruction told agents to add--shutdown.This PR states one interim rule with one exception: if your session booted a local simulator or emulator, use
close --shutdown, unless the user asks to keep it running. It appears in:closehelp and themanual-qaandvalidatehelp topics. Theworkflowcard stays unchanged because it is at its 9100-byte budget.agent-device,dogfood,ios-simulator,android-emulator(one line each). The simulator skill contract test requires the line.docs/agents/device-verification.md(owner for repo workers). I deleted two cleanup lines that repeat CLI help to stay under thedocs/agentsbudget.9 files. Instructions only. #4162 (0.22) moves the rule into the daemon: plain
closestops a device that the session'sopenbooted. This interim text is then deleted.Validation
Head 6fdb246, rebased on 7b3f63f.
pnpm gate agent-guidance: pass (AGENTS.md3999/4000,docs/agents39893/40000).pnpm check:affected --run --head 6fdb246a0e(full FAIL-OPEN set): every stage passed exceptunit.unithad 1 failure,daemon-client-lifecycle.test.ts, and that file passes alone (19/19). The previous round showed different flakes under load (daemon-client-timeout-route,provider-revyl/runtime), and both pass alone. Receipt:.tmp/validation/run-7YyBOZ/receipt.json(local).cli-help,cli-help-topics,simulator-skills-contract,help-conformance-benchpass.