fix(chrome-extension-native-host): halt unauthorized origins and forward guardianId

[noanflaherty]

Summary

• Makes writeFrameAndExit synchronously terminate the process so unauthorized origins cannot fall through to the stdin listener
• Adds explicit return in the unauthorized branch as defense-in-depth
• Forwards guardianId from /v1/browser-extension-pair through the native messaging token_response frame, fixing Codex P1 where bootstrapLocalToken rejected frames missing the field
• Adds subprocess tests for unauthorized halt, guardianId forwarding, and missing-guardianId rejection
• Extends existing e2e test to assert guardianId presence

Addresses gaps 2 and 3 from PR #24159 self-review.

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d9e5eae783

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Wait for stdout flush before exiting native host

writeFrameAndExit now calls process.exit() immediately after process.stdout.write(...), which can drop or truncate the native-messaging frame on pipe-backed stdout (the normal Chrome host transport on macOS/Linux) whenever the pipe is buffered/backpressured. In that case the extension sees a disconnect instead of the expected token_response/error, so bootstrap can fail intermittently even though the helper exits. Because every success and error path funnels through this function, this regression affects the core pairing flow, not just the unauthorized-origin branch.

Useful? React with 👍 / 👎.

[devin-ai-integration]

Devin Review found 2 potential issues.

View 3 additional findings in Devin Review.

[devin-ai-integration]

🔴 Comment narrates refactoring history instead of describing current intent

The JSDoc for writeFrameAndExit at lines 140–147 says "Earlier versions of this helper used the callback form of process.stdout.write()…" — this narrates the implementation history of removed code. clients/AGENTS.md:407 requires: "Comments and docstrings must describe the code's intent and behavior, not its refactoring history." The design rationale can be conveyed without referencing past versions (e.g., "Using the callback form of process.stdout.write() would introduce a hazard: …").

Suggested change

	* Earlier versions of this helper used the callback form of
	* `process.stdout.write()` to defer `process.exit()` until the buffer had
	* been handed off, but that introduced a subtle bug: the function had to
	* return a forever-pending Promise to satisfy the `never` return type,
	* meaning callers that awaited it would yield control of the event loop
	* before the process terminated. In the unauthorized-origin path that
	* meant the stdin listener could be installed and start handling frames
	* before the helper actually exited.
	* Using the callback form of `process.stdout.write()` to defer
	* `process.exit()` would introduce a hazard: the function would need to
	* return a forever-pending Promise to satisfy the `never` return type,
	* meaning callers that `await` it would yield control of the event loop
	* before the process terminated. In the unauthorized-origin path that
	* would let the stdin listener be installed and start handling frames
	* before the helper actually exited.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

🔴 Test module docstring narrates refactoring history of removed code

The file-level docstring at lines 7–16 uses historical narrative to describe the bugs: "The previous writeFrameAndExit returned a forever-pending Promise…" (line 7–10) and "Earlier versions destructured only { token, expiresAt } and silently dropped the field…" (lines 14–16). clients/AGENTS.md:407 requires: "Comments and docstrings must describe the code's intent and behavior, not its refactoring history." These should be rephrased as current invariants the tests enforce, e.g., "writeFrameAndExit must terminate synchronously to prevent…" and "the helper must include guardianId in the token_response frame…".

Suggested change

	* The previous `writeFrameAndExit` returned a forever-pending Promise
	* after queuing the write, which meant `await`-ing it yielded the
	* event loop and let the stdin listener run before the helper
	* actually exited.
	*
	* - **Gap 3**: the helper must surface the `guardianId` field returned
	* by `/v1/browser-extension-pair` in its `token_response` frame.
	* Earlier versions destructured only `{ token, expiresAt }` and
	* silently dropped the field, causing `bootstrapLocalToken()` on the
	* consumer side to reject the frame as malformed.
	* - **Gap 2**: the unauthorized-origin branch must terminate the
	* process synchronously and never POST `/v1/browser-extension-pair`.
	* If `writeFrameAndExit` yields the event loop (e.g. via a
	* never-resolving Promise), callers that `await` it would let the
	* stdin listener run before the helper actually exits.
	*
	* - **Gap 3**: the helper must surface the `guardianId` field returned
	* by `/v1/browser-extension-pair` in its `token_response` frame.
	* If the field is not forwarded, `bootstrapLocalToken()` on the
	* consumer side rejects the frame as malformed.

---

Was this helpful? React with 👍 or 👎 to provide feedback.