Skip to content

fix(cli): fail react-devtools component reads that cannot observe an app - #2432

Closed
boorad wants to merge 2 commits into
callstack:mainfrom
boorad:fix/react-devtools-attachment-gate
Closed

fix(cli): fail react-devtools component reads that cannot observe an app#2432
boorad wants to merge 2 commits into
callstack:mainfrom
boorad:fix/react-devtools-attachment-gate

Conversation

@boorad

@boorad boorad commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Summary

react-devtools errors printed No components with errors or warnings with 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 dump half, so it should not auto-close on this PR).

errors, find, count, and get now probe attachment through the passthrough's own status before running:

$ agent-device react-devtools errors            # daemon up, 0 apps
Error (COMMAND_FAILED): react-devtools errors observed nothing: the React DevTools daemon has 0 apps connected.
$ agent-device react-devtools errors --json     # details.subcommand, details.connectedApps: 0

A status without a parseable app count leaves the passthrough untouched; status, wait, start, and stop are 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.0 stdout, 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 in agent-react-devtools and picked up by a pin bump? Happy to reshape as a warning, an opt-in flag, or drop it.

Validation

3fd90160. pnpm check:affected --run passes on that commit (typecheck, layering, fallow, build, vitest-related, command-docs). Full pnpm test:unit: 9438 pass; the 2 code-signature.test.ts failures reproduce unchanged on a clean tree (macOS fs.rmSync on 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 to ws://127.0.0.1:8097: status went to Apps: 1 connected and 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.

@thymikee

Copy link
Copy Markdown
Member

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.

@thymikee

Copy link
Copy Markdown
Member

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.

@boorad
boorad force-pushed the fix/react-devtools-attachment-gate branch from 3fd9016 to 6b6bcfb Compare September 10, 2026 13:01
@boorad

boorad commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

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 3bbeb619 so the branch is no longer conflicting (head 6b6bcfb7), but I am not pushing further changes here, since the approach itself is the thing you have redirected.

I would like to do this the way you describe: check attachment inside the operation that reads the component tree in agent-react-devtools, return a structured failure when observation is unavailable, and cover disconnect-before-read and unavailable-attachment-state. Two questions before I start:

  1. Do you want that PR against callstackincubator/agent-react-devtools, and is there a preferred error shape for the structured failure so the agent-device passthrough surfaces it as a typed error rather than text?
  2. Should this PR stay open as a placeholder for the eventual pin bump, or would you rather close it and have the pin bump ride with the release?

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.

@boorad

boorad commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

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 status probe cannot guarantee the later read observed an app, and my unparseable-status branch explicitly let the original vacuous result through — I'd made "fail open on format drift" the default in the one place where a false clean is the bug being fixed. Checking in the operation that reads the tree removes both problems: connection health is read in the same synchronous turn as the tree, so nothing can attach or detach in between.

Upstream, get tree, get component, find, count and errors now return { ok: false, code: 'NO_APP_CONNECTED' }, so the reason is machine-readable rather than parsed from text. cli.ts needed no change — every read already exits non-zero on ok: false — which means this repo gets the correct behavior through the existing 1:1 passthrough with no wrapper logic at all.

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: get tree used to attach a disconnected Ns ago hint to an empty success, which is the same defect in miniature; that context now qualifies the refusal.

What I'd like to do with this PR. Leave it draft and reduce it to the agent-react-devtools pin bump once #60 lands and is published, dropping the probe entirely. Say the word if you'd rather I close it and open the bump fresh — or if you want the bump to carry the react-devtools docs note about component reads failing without an attachment, which is the only part of the current diff worth keeping.

No CI checks are reported on this head; I can't trigger them from a fork branch. Rebased onto 3bbeb619 and pnpm check:affected --run passes on 6b6bcfb7, for whatever that's worth in the meantime.

@thymikee

Copy link
Copy Markdown
Member

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.
@boorad
boorad force-pushed the fix/react-devtools-attachment-gate branch from 6b6bcfb to 9f728f3 Compare September 10, 2026 14:33
@boorad

boorad commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Probe removed at 9f728f3e, rebased onto 805ffb46. The revert cancels the original commit exactly, so this PR is now 0 files changed — everything substantive lives upstream in callstackincubator/agent-react-devtools#60, and this repo inherits it through the existing 1:1 passthrough with no wrapper logic at all.

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 agent-react-devtools@0.4.0. Documenting it before the pin moves would be the same class of problem as the bug — stating a guarantee the shipped version does not make.

That leaves this as an empty placeholder, which I'd rather not leave sitting open indefinitely. Two options, your call:

  1. I close this now and open a fresh chore(deps) PR when fix: stop daemon/runner lifecycle leaks on iOS #60 is published — that PR bumps AGENT_REACT_DEVTOOLS_VERSION, restores the docs note, and carries a live before/after against a real simulator.
  2. It stays draft and I push the bump onto this branch when the release lands.

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 get tree behavior beyond the refusal. It previously attached a disconnected Ns ago, waiting for reconnect... hint to an empty success; that context now qualifies a refusal instead. Anything in this repo reading that hint would need to move to the error path — I checked and found nothing, but you'd know better than a grep would.

@thymikee

Copy link
Copy Markdown
Member

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.

@boorad

boorad commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Agreed — closing. The fix lives in callstackincubator/agent-react-devtools#60; once that's released I'll open a fresh chore(deps) PR bumping AGENT_REACT_DEVTOOLS_VERSION, restoring the docs note, and carrying before/after validation against a real simulator.

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.

@boorad boorad closed this Sep 10, 2026
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