fix(cli): fail react-devtools component reads that cannot observe an app - #2432
fix(cli): fail react-devtools component reads that cannot observe an app#2432boorad wants to merge 2 commits into
Conversation
|
I would fix this in agent-react-devtools and pick up the updated pin. The separate status probe cannot guarantee that a later component read observed an app: an app can disconnect between those calls, and an unparseable status explicitly lets the original false-clean result through. Check attachment in the operation that reads the component tree and return a structured failure when observation is unavailable. Add regressions for disconnect-before-read and unavailable attachment state. The reported simulator test proves the common zero-app case, but not those remaining paths; no CI checks are reported on this head. |
|
This head now also conflicts with main. The previously reported code findings still apply; please resolve those and the conflict before rerunning the affected checks. |
3fd9016 to
6b6bcfb
Compare
|
Thanks — that answers the question I opened this as a draft to ask, and your correctness points land. Agreed the separate status probe cannot guarantee the later read observed an app: the disconnect-between-calls window is real and unclosable from the wrapper, and the unparseable-status path deliberately falls through to the original false-clean result. Both are inherent to checking attachment outside the operation that reads the tree, so no amount of reshaping this PR fixes them. Rebased onto I would like to do this the way you describe: check attachment inside the operation that reads the component tree in
Happy to close it now if that keeps the queue clean — the branch is on my fork either way. Confirming the repo before opening anything there. |
|
Agreed on all counts — the wrapper gate is the wrong shape and I've moved the fix upstream: callstackincubator/agent-react-devtools#60. Your objections held up exactly as stated. A separate Upstream, Regressions there cover the paths you named: each read refusing with no app ever connected, and a read issued after the app disconnected refusing rather than answering from the stale tree. Each was verified to fail with the guard reverted. One behavior change surfaced while doing it: What I'd like to do with this PR. Leave it draft and reduce it to the No CI checks are reported on this head; I can't trigger them from a fork branch. Rebased onto |
|
The rebase resolves the conflict, but 6b6bcfb still contains the separate status probe and its false-clean paths. The proposed upstream fix is the right direction; this remains blocked until the probe is removed and the published dependency update is reviewed. No CI checks are reported on this head. |
`react-devtools errors` printed "No components with errors or warnings" with nothing attached, so a check that was never performed rendered identically to a check that passed. The passthrough starts a daemon on demand and answers component reads from its empty tree, which makes the vacuous pass reachable even with no daemon running. Gate `errors`, `find`, `count`, and `get` on attachment, probed through the passthrough's own `status`. An unreachable daemon or a parsed zero connected apps fails the read with COMMAND_FAILED and the connected-app count in details; a status without a parseable count leaves the passthrough untouched. `status`, `wait`, `start`, and `stop` are never gated. Fixes callstack#2430
The probe cannot guarantee the read that follows it observed an app, and its unparseable-status branch let the vacuous result through — fail-open by default in the one place a false clean is the bug. Attachment is checked in the operation that reads the component tree instead, upstream in callstackincubator/agent-react-devtools#60, which this repo picks up through the existing 1:1 passthrough once the pin bumps.
6b6bcfb to
9f728f3
Compare
|
Probe removed at I dropped the docs and CHANGELOG lines too, rather than keep them: they described component reads failing without an attachment, which is not true of pinned That leaves this as an empty placeholder, which I'd rather not leave sitting open indefinitely. Two options, your call:
Either is fine; 1 is tidier unless you want the thread kept in one place. One thing worth flagging for whenever the bump happens: #60 changes |
|
The unsafe probe is fully removed at 9f728f3. This PR is now empty, so it does not yet fix disconnected component reads: the wrapper still uses 0.4.0 and upstream callstackincubator/agent-react-devtools#60 is open. I recommend closing this placeholder and reviewing a dependency bump with the before/after validation once the upstream release is published. |
|
Agreed — closing. The fix lives in callstackincubator/agent-react-devtools#60; once that's released I'll open a fresh Thanks for the push back to the right layer — the probe was wrong in exactly the ways you named, and the upstream version has no disconnect window and no fail-open path. |
Summary
react-devtools errorsprintedNo components with errors or warningswith zero apps attached, so a check that was never performed rendered identically to a check that passed. For an agent collecting evidence, a vacuous pass is worse than an error. Refs #2430 (that issue has a second,network dumphalf, so it should not auto-close on this PR).errors,find,count, andgetnow probe attachment through the passthrough's ownstatusbefore running:A
statuswithout a parseable app count leaves the passthrough untouched;status,wait,start, andstopare never gated. 4 files (1 production, 1 new test, CHANGELOG, commands.md).Question for maintainers, hence draft. This gate reconstructs attachment state in the wrapper by parsing pinned
agent-react-devtools@0.4.0stdout, which sits awkwardly against AGENTS.md ("repair at the owning type... do not add guards that reconstruct another source of truth"; "key behavior on typed reasons, never error text"), and it breaks the documented 1:1 passthrough for four subcommands. Should this instead be fixed upstream inagent-react-devtoolsand picked up by a pin bump? Happy to reshape as a warning, an opt-in flag, or drop it.Validation
3fd90160.pnpm check:affected --runpasses on that commit (typecheck, layering, fallow, build, vitest-related, command-docs). Fullpnpm test:unit: 9438 pass; the 2code-signature.test.tsfailures reproduce unchanged on a clean tree (macOSfs.rmSyncon a symlinked dir) and are unrelated.Verified live against a RN 0.87.1 app on an iOS 26 simulator, which never attaches to DevTools. Published 0.20.10 printed the clean pass, exit 0; this build fails with exit 1. With no daemon running, 0.20.10 auto-started an empty one (
Uptime: 1s) and still passed — this build fails instead and leaves no stray daemon. Happy path checked by attaching a client tows://127.0.0.1:8097:statuswent toApps: 1 connectedand all four reads passed through at exit 0; detaching re-engaged the gate.Risk: the probe adds one
npm exec(~0.4s measured warm) to those four subcommands. Reading the daemon's unix socket would make it ~free but couples to its private IPC; not done without a maintainer steer.