browser-automation: attach-lane primitives (Phase 1)#35
Conversation
DevToolsActivePort parser (re-read per connection, GUID rotates per boot), SingletonLock PID resolver (disambiguates multi-instance Chrome), deny-by- default https-only URL policy with private-net + credential blocks and redirect revalidation, structured attach errors with credential redaction, an explicit no-replay state machine with op deadlines, and a PID+nonce fenced cross-process lease so exactly one session drives the browser.
Reviewer's GuideImplements foundational, pure attach-lane primitives for browser automation (DevTools port parsing, Chrome PID discovery, navigation policy, structured error handling, attach session state machine with deadlines, and a cross‑process lease), all covered by unit tests and not yet wired into existing launch-mode behavior. Sequence diagram for withDeadline attach operation timeout handlingsequenceDiagram
participant Caller
participant withDeadline
participant TransportOp as TransportPromise
participant Timer
Caller->>withDeadline: withDeadline(promise, ms, label)
activate withDeadline
withDeadline->>Timer: setTimeout(ms)
withDeadline->>TransportOp: await p
alt promise resolves first
TransportOp-->>withDeadline: result
withDeadline->>Timer: clearTimeout
withDeadline-->>Caller: result
else deadline fires first
Timer-->>withDeadline: timeout
withDeadline-->>Caller: throws AttachError(NAV_TIMEOUT)
end
deactivate withDeadline
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesAdds an opt-in browser attach workflow with validated discovery, URL policies, structured errors, lifecycle and lease controls, a coordinating controller, CDP and JXA backends, five attach tools, extension-pack wiring, and Vitest coverage. Browser attach workflow
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
acquireLease, you callnow()twice when constructing the lease, which can yield slightly inconsistentacquiredAt/expiresAtvalues and complicates testing; consider capturingconst ts = now()once and deriving both fields from that. - The
readLeasehelper silently treats any JSON parse failure as “no lease”; if the lease file becomes corrupted this will look like a missing lease rather than a structural problem—consider surfacing an explicitAttachErroror distinguishing parse errors from absence.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `acquireLease`, you call `now()` twice when constructing the lease, which can yield slightly inconsistent `acquiredAt`/`expiresAt` values and complicates testing; consider capturing `const ts = now()` once and deriving both fields from that.
- The `readLease` helper silently treats any JSON parse failure as “no lease”; if the lease file becomes corrupted this will look like a missing lease rather than a structural problem—consider surfacing an explicit `AttachError` or distinguishing parse errors from absence.
## Individual Comments
### Comment 1
<location path="registry/curated/system/browser-automation/src/attach/lease.ts" line_range="56-51" />
<code_context>
+ }
+}
+
+function readLease(file: string): AttachLease | undefined {
+ try {
+ return JSON.parse(readFileSync(file, 'utf8')) as AttachLease;
+ } catch {
+ return undefined;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Malformed or partially-written lease files are silently treated as "no lease".
If JSON parsing fails (e.g., corruption, partial or concurrent write), the code treats this as "no lease" and allows a new lease to be acquired. That can violate the "at most one session" guarantee when the existing holder is still alive but the file is temporarily unreadable. It would be safer to distinguish parse errors from a missing file and either surface a dedicated AttachError (e.g., a `STALE_PORT_FILE`-style code) or fail closed instead of re-acquiring when a lease file exists but cannot be parsed.
</issue_to_address>
### Comment 2
<location path="registry/curated/system/browser-automation/src/attach/__tests__/attach-primitives.test.ts" line_range="12-21" />
<code_context>
+import { AttachStateMachine, withDeadline } from '../state.js';
+import { acquireLease, releaseLease, requireLease } from '../lease.js';
+
+describe('parseDevToolsActivePort', () => {
+ it('parses a well-formed file', () => {
+ const r = parseDevToolsActivePort('9222\n/devtools/browser/07797f92-4fc3-432a-91dd-0e982e1a3bc0\n');
+ expect(r.port).toBe(9222);
+ expect(r.wsUrl).toBe('ws://127.0.0.1:9222/devtools/browser/07797f92-4fc3-432a-91dd-0e982e1a3bc0');
+ });
+ it('rejects a single-line file', () => {
+ expect(() => parseDevToolsActivePort('9222\n')).toThrow(/malformed/i);
+ });
+ it('rejects a non-numeric port', () => {
+ expect(() => parseDevToolsActivePort('nope\n/devtools/browser/x\n')).toThrow(/invalid port/i);
+ });
+ it('rejects a non-browser-target ws path', () => {
+ expect(() => parseDevToolsActivePort('9222\n/devtools/page/abc\n')).toThrow(/unexpected ws path/i);
+ });
</code_context>
<issue_to_address>
**suggestion (testing):** Add edge-case tests for whitespace, extra lines, and out-of-range ports in DevToolsActivePort parsing
Since the parser trims lines, skips empties, and enforces 1–65535, please add tests that: (1) confirm files with trailing blank lines/extra whitespace and additional lines still parse correctly; and (2) assert that ports like `0` and `65536` are rejected, covering the `<= 0 || > 65535` logic and clarifying behavior on slightly malformed inputs.
</issue_to_address>
### Comment 3
<location path="registry/curated/system/browser-automation/src/attach/__tests__/attach-primitives.test.ts" line_range="41-50" />
<code_context>
+describe('url-policy', () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Extend URL policy tests to cover IPv6 localhost and allowlist corner cases
Since the policy explicitly blocks `[::1]` and uses a host allowlist, it’d be good to cover those semantics in tests. Please add cases for: (1) an IPv6 loopback URL like `https://[::1]/` being rejected with `PRIVATE_HOST`; (2) an allowed host with mixed case (to confirm case-insensitive matching) and a similar-looking but disallowed domain (e.g. `evilfounderscard.com`); and (3) the difference between `allowHosts: []` and `allowHosts` being omitted, to show that an empty array does not behave as “deny all.”
Suggested implementation:
```typescript
describe('url-policy', () => {
it('allows https and about:blank', () => {
expect(isNavigationAllowed('https://founderscard.com/home').allowed).toBe(true);
expect(isNavigationAllowed('about:blank').allowed).toBe(true);
});
it('rejects IPv6 localhost with PRIVATE_HOST', () => {
const result = isNavigationAllowed('https://[::1]/');
expect(result.allowed).toBe(false);
expect(result.reason).toBe('PRIVATE_HOST');
});
it('matches allowHosts case-insensitively and does not allow similar-looking domains', () => {
const allowed = isNavigationAllowed('https://FoundersCard.com/path', {
allowHosts: ['founderscard.com'],
});
const blocked = isNavigationAllowed('https://evilfounderscard.com/', {
allowHosts: ['founderscard.com'],
});
expect(allowed.allowed).toBe(true);
expect(blocked.allowed).toBe(false);
});
it('treats an empty allowHosts array the same as the default allowlist', () => {
const url = 'https://founderscard.com/home';
const defaultResult = isNavigationAllowed(url);
const explicitEmptyAllowlist = isNavigationAllowed(url, { allowHosts: [] });
expect(defaultResult.allowed).toBe(true);
expect(explicitEmptyAllowlist.allowed).toBe(true);
});
it.each([
['file:///etc/passwd', 'SCHEME_BLOCKED'],
```
If `isNavigationAllowed` does not currently accept an options object with an `allowHosts` property, or if the shape of the options object differs, you will need to:
1. Adjust the second and third new tests to pass configuration using the actual `isNavigationAllowed` signature (e.g., different parameter order or property names).
2. If the return type does not expose a `reason` field, adapt the IPv6 test to assert on whatever field or enum is used to expose `PRIVATE_HOST` (or update `isNavigationAllowed` to include `reason` in its result).
</issue_to_address>
### Comment 4
<location path="registry/curated/system/browser-automation/src/attach/__tests__/attach-primitives.test.ts" line_range="29-39" />
<code_context>
+ });
+});
+
+describe('pidFromSingletonLock', () => {
+ it('extracts the pid', () => {
+ expect(pidFromSingletonLock('MacBook-Air.local-666')).toBe(666);
+ });
+ it('handles hostnames containing dashes', () => {
+ expect(pidFromSingletonLock('my-host-name.local-12345')).toBe(12345);
+ });
+ it('rejects a target without a pid suffix', () => {
+ expect(() => pidFromSingletonLock('just-a-hostname.local')).toThrow(/malformed/i);
+ });
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for trimming behavior and large-PID parsing in singleton-lock PID extraction
Since `pidFromSingletonLock` trims the symlink target and parses a trailing `-<digits>`, consider adding tests for: (1) leading/trailing whitespace around the target (e.g. `' Mac.local-123 '`); (2) a large PID near the typical macOS upper bound; and (3) a non-numeric suffix like `-abc` to assert that it throws. This would better cover both the parsing and error paths.
```suggestion
describe('pidFromSingletonLock', () => {
it('extracts the pid', () => {
expect(pidFromSingletonLock('MacBook-Air.local-666')).toBe(666);
});
it('handles hostnames containing dashes', () => {
expect(pidFromSingletonLock('my-host-name.local-12345')).toBe(12345);
});
it('trims surrounding whitespace from the target', () => {
expect(pidFromSingletonLock(' Mac.local-123 ')).toBe(123);
});
it('parses a large numeric pid suffix', () => {
expect(pidFromSingletonLock('my-host.local-98765')).toBe(98765);
});
it('rejects a target without a pid suffix', () => {
expect(() => pidFromSingletonLock('just-a-hostname.local')).toThrow(/malformed/i);
});
it('rejects a non-numeric pid suffix', () => {
expect(() => pidFromSingletonLock('my-host.local-abc')).toThrow(/malformed/i);
});
});
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| try { | ||
| process.kill(pid, 0); | ||
| return true; | ||
| } catch { |
There was a problem hiding this comment.
issue (bug_risk): Malformed or partially-written lease files are silently treated as "no lease".
If JSON parsing fails (e.g., corruption, partial or concurrent write), the code treats this as "no lease" and allows a new lease to be acquired. That can violate the "at most one session" guarantee when the existing holder is still alive but the file is temporarily unreadable. It would be safer to distinguish parse errors from a missing file and either surface a dedicated AttachError (e.g., a STALE_PORT_FILE-style code) or fail closed instead of re-acquiring when a lease file exists but cannot be parsed.
| describe('parseDevToolsActivePort', () => { | ||
| it('parses a well-formed file', () => { | ||
| const r = parseDevToolsActivePort('9222\n/devtools/browser/07797f92-4fc3-432a-91dd-0e982e1a3bc0\n'); | ||
| expect(r.port).toBe(9222); | ||
| expect(r.wsUrl).toBe('ws://127.0.0.1:9222/devtools/browser/07797f92-4fc3-432a-91dd-0e982e1a3bc0'); | ||
| }); | ||
| it('rejects a single-line file', () => { | ||
| expect(() => parseDevToolsActivePort('9222\n')).toThrow(/malformed/i); | ||
| }); | ||
| it('rejects a non-numeric port', () => { |
There was a problem hiding this comment.
suggestion (testing): Add edge-case tests for whitespace, extra lines, and out-of-range ports in DevToolsActivePort parsing
Since the parser trims lines, skips empties, and enforces 1–65535, please add tests that: (1) confirm files with trailing blank lines/extra whitespace and additional lines still parse correctly; and (2) assert that ports like 0 and 65536 are rejected, covering the <= 0 || > 65535 logic and clarifying behavior on slightly malformed inputs.
| describe('url-policy', () => { | ||
| it('allows https and about:blank', () => { | ||
| expect(isNavigationAllowed('https://founderscard.com/home').allowed).toBe(true); | ||
| expect(isNavigationAllowed('about:blank').allowed).toBe(true); | ||
| }); | ||
| it.each([ | ||
| ['file:///etc/passwd', 'SCHEME_BLOCKED'], | ||
| ['javascript:alert(1)', 'SCHEME_BLOCKED'], | ||
| ['data:text/html,<b>x</b>', 'SCHEME_BLOCKED'], | ||
| ['devtools://devtools/bundled/inspector.html', 'SCHEME_BLOCKED'], |
There was a problem hiding this comment.
suggestion (testing): Extend URL policy tests to cover IPv6 localhost and allowlist corner cases
Since the policy explicitly blocks [::1] and uses a host allowlist, it’d be good to cover those semantics in tests. Please add cases for: (1) an IPv6 loopback URL like https://[::1]/ being rejected with PRIVATE_HOST; (2) an allowed host with mixed case (to confirm case-insensitive matching) and a similar-looking but disallowed domain (e.g. evilfounderscard.com); and (3) the difference between allowHosts: [] and allowHosts being omitted, to show that an empty array does not behave as “deny all.”
Suggested implementation:
describe('url-policy', () => {
it('allows https and about:blank', () => {
expect(isNavigationAllowed('https://founderscard.com/home').allowed).toBe(true);
expect(isNavigationAllowed('about:blank').allowed).toBe(true);
});
it('rejects IPv6 localhost with PRIVATE_HOST', () => {
const result = isNavigationAllowed('https://[::1]/');
expect(result.allowed).toBe(false);
expect(result.reason).toBe('PRIVATE_HOST');
});
it('matches allowHosts case-insensitively and does not allow similar-looking domains', () => {
const allowed = isNavigationAllowed('https://FoundersCard.com/path', {
allowHosts: ['founderscard.com'],
});
const blocked = isNavigationAllowed('https://evilfounderscard.com/', {
allowHosts: ['founderscard.com'],
});
expect(allowed.allowed).toBe(true);
expect(blocked.allowed).toBe(false);
});
it('treats an empty allowHosts array the same as the default allowlist', () => {
const url = 'https://founderscard.com/home';
const defaultResult = isNavigationAllowed(url);
const explicitEmptyAllowlist = isNavigationAllowed(url, { allowHosts: [] });
expect(defaultResult.allowed).toBe(true);
expect(explicitEmptyAllowlist.allowed).toBe(true);
});
it.each([
['file:///etc/passwd', 'SCHEME_BLOCKED'],If isNavigationAllowed does not currently accept an options object with an allowHosts property, or if the shape of the options object differs, you will need to:
- Adjust the second and third new tests to pass configuration using the actual
isNavigationAllowedsignature (e.g., different parameter order or property names). - If the return type does not expose a
reasonfield, adapt the IPv6 test to assert on whatever field or enum is used to exposePRIVATE_HOST(or updateisNavigationAllowedto includereasonin its result).
| describe('pidFromSingletonLock', () => { | ||
| it('extracts the pid', () => { | ||
| expect(pidFromSingletonLock('MacBook-Air.local-666')).toBe(666); | ||
| }); | ||
| it('handles hostnames containing dashes', () => { | ||
| expect(pidFromSingletonLock('my-host-name.local-12345')).toBe(12345); | ||
| }); | ||
| it('rejects a target without a pid suffix', () => { | ||
| expect(() => pidFromSingletonLock('just-a-hostname.local')).toThrow(/malformed/i); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
suggestion (testing): Add tests for trimming behavior and large-PID parsing in singleton-lock PID extraction
Since pidFromSingletonLock trims the symlink target and parses a trailing -<digits>, consider adding tests for: (1) leading/trailing whitespace around the target (e.g. ' Mac.local-123 '); (2) a large PID near the typical macOS upper bound; and (3) a non-numeric suffix like -abc to assert that it throws. This would better cover both the parsing and error paths.
| describe('pidFromSingletonLock', () => { | |
| it('extracts the pid', () => { | |
| expect(pidFromSingletonLock('MacBook-Air.local-666')).toBe(666); | |
| }); | |
| it('handles hostnames containing dashes', () => { | |
| expect(pidFromSingletonLock('my-host-name.local-12345')).toBe(12345); | |
| }); | |
| it('rejects a target without a pid suffix', () => { | |
| expect(() => pidFromSingletonLock('just-a-hostname.local')).toThrow(/malformed/i); | |
| }); | |
| }); | |
| describe('pidFromSingletonLock', () => { | |
| it('extracts the pid', () => { | |
| expect(pidFromSingletonLock('MacBook-Air.local-666')).toBe(666); | |
| }); | |
| it('handles hostnames containing dashes', () => { | |
| expect(pidFromSingletonLock('my-host-name.local-12345')).toBe(12345); | |
| }); | |
| it('trims surrounding whitespace from the target', () => { | |
| expect(pidFromSingletonLock(' Mac.local-123 ')).toBe(123); | |
| }); | |
| it('parses a large numeric pid suffix', () => { | |
| expect(pidFromSingletonLock('my-host.local-98765')).toBe(98765); | |
| }); | |
| it('rejects a target without a pid suffix', () => { | |
| expect(() => pidFromSingletonLock('just-a-hostname.local')).toThrow(/malformed/i); | |
| }); | |
| it('rejects a non-numeric pid suffix', () => { | |
| expect(() => pidFromSingletonLock('my-host.local-abc')).toThrow(/malformed/i); | |
| }); | |
| }); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6f5fd498b
ℹ️ 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".
| expiresAt: now() + (opts.ttlMs ?? DEFAULT_TTL_MS), | ||
| }; | ||
| mkdirSync(dirname(opts.file), { recursive: true }); | ||
| writeFileSync(opts.file, JSON.stringify(lease)); |
There was a problem hiding this comment.
When two attach processes start at the same time and the lease file does not exist yet (or has just become reclaimable), both can pass the existsSync/read check and then both return success after this unconditional write. That leaves two callers holding different nonces and able to drive the user's browser concurrently, which is the race this cross-process lease is supposed to prevent; acquisition needs an atomic create/replace protocol rather than check-then-write.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| const PRIVATE_HOST_RE = | ||
| /^(localhost|127\.\d+\.\d+\.\d+|0\.0\.0\.0|10\.\d+\.\d+\.\d+|192\.168\.\d+\.\d+|172\.(1[6-9]|2\d|3[01])\.\d+\.\d+|\[::1\]|.*\.local)$/i; |
There was a problem hiding this comment.
For IPv6 private-network targets such as https://[fd00::1]/ or link-local https://[fe80::1]/, URL.hostname is a bracketed IPv6 literal that does not match this regex unless it is exactly [::1], so isNavigationAllowed returns allowed for internal IPv6 services. In attach mode that bypasses the stated private/loopback host block for a user's logged-in browser.
Useful? React with 👍 / 👎.
| return { allowed: false, reason: 'MALFORMED_URL' }; | ||
| } | ||
| if (parsed.protocol !== 'https:') return { allowed: false, reason: 'SCHEME_BLOCKED' }; | ||
| if (parsed.username || parsed.password) return { allowed: false, reason: 'CREDENTIALS_IN_URL' }; |
There was a problem hiding this comment.
When credentials are carried in the query string, for example https://example.com/callback?access_token=... or ?id_token=..., parsed.username and parsed.password are empty so the policy allows the navigation. Those parameters are already treated as credential-class data by the redactor, so allowing them here violates the attach lane's deny-by-default credential-bearing URL policy and can send sensitive tokens through navigation/tool diagnostics.
Useful? React with 👍 / 👎.
The controller is the only object allowed to touch an attached browser: attach-only (aborts on identity mismatch, never launches), marker-tab bound, lease-nonce checked per op, no-replay state machine, policy-checked navigation with redirect revalidation. The backend contract exposes NO browser/tab close surface, so a detach can only drop our transport — the structural fix for the disconnect-closes-browser hazard. JXA backend addresses Chrome by PID from SingletonLock (multi-instance safe) with a claim/goto/url/read driver (no arbitrary JS); CDP backend re-reads DevToolsActivePort per connect and reference-drops on disconnect.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@registry/curated/system/browser-automation/src/attach/__tests__/attach-primitives.test.ts`:
- Around line 78-84: Update the redaction test in the “errors” describe block to
replace the ambiguous access_token fixture value and matching assertion with a
longer distinctive credential value that cannot occur in the unchanged URL
scheme or host. Preserve the existing checks for abc123 and xyz987 and continue
verifying that the access-token value is absent from redactDiagnostic output.
In `@registry/curated/system/browser-automation/src/attach/errors.ts`:
- Around line 63-74: Update the LEASE_DENIED matcher in CODE_MATCHERS to match
“lease” as a standalone word, or the specific lease-held message produced by
lease.ts, so unrelated text such as “please retry” is not classified as a lease
denial.
- Around line 44-49: Update CREDENTIAL_PATTERNS so the bearer credential form
also redacts tokens separated from “Bearer” by whitespace alone, while
preserving the existing colon/equal assignment handling for other credential
keywords. Ensure Authorization-style values such as “Bearer <token>” are fully
replaced before diagnostic text is emitted.
In `@registry/curated/system/browser-automation/src/attach/lease.ts`:
- Around line 70-90: Update acquireLease and releaseLease to use one atomic
CAS-style guard for lease ownership: create the lease file exclusively when
absent, atomically reclaim stale leases only if the expected nonce/content still
matches, and never overwrite a competing live lease. Apply the same
compare-and-delete protocol in releaseLease so it cannot remove a lease replaced
after validation, while preserving existing liveness, expiry, and AttachError
behavior.
- Around line 47-53: Update defaultIsPidAlive to inspect the error code from
process.kill(pid, 0): return false only for ESRCH, return true for EPERM, and
rethrow any other unexpected error instead of treating all failures as dead
processes.
In `@registry/curated/system/browser-automation/src/attach/state.ts`:
- Around line 73-76: Validate ms at the start of withDeadline before
constructing the gate or scheduling setTimeout, requiring it to be a finite
positive integer no greater than 2,147,483,647. Reject invalid values with the
existing error-handling convention, while preserving the current timeout
behavior for valid deadlines.
In `@registry/curated/system/browser-automation/src/attach/url-policy.ts`:
- Around line 36-37: Update PRIVATE_HOST_RE to deny the entire 169.254.0.0/16
link-local range, including 169.254.169.254, and add a regression test verifying
that metadata endpoint is rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 359f05a2-4412-43f4-9353-e7082a2cc36c
📒 Files selected for processing (7)
registry/curated/system/browser-automation/src/attach/__tests__/attach-primitives.test.tsregistry/curated/system/browser-automation/src/attach/devtools-port.tsregistry/curated/system/browser-automation/src/attach/errors.tsregistry/curated/system/browser-automation/src/attach/lease.tsregistry/curated/system/browser-automation/src/attach/singleton-lock.tsregistry/curated/system/browser-automation/src/attach/state.tsregistry/curated/system/browser-automation/src/attach/url-policy.ts
| const CREDENTIAL_PATTERNS: Array<[RegExp, string]> = [ | ||
| [/(token|secret|password|passwd|apikey|api_key|auth|bearer|session|cookie)(["']?\s*[:=]\s*["']?)[^\s"'&;]+/gi, '$1$2[redacted]'], | ||
| [/eyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{4,}/g, '[redacted-jwt]'], | ||
| [/\b(sk|pk|rk|ak)-[A-Za-z0-9_-]{12,}\b/g, '[redacted-key]'], | ||
| [/[?&](code|state|id_token|access_token|refresh_token)=[^\s&"']+/gi, '?[redacted-param]'], | ||
| ]; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Credential redaction misses space-separated Bearer <token> values.
The first pattern requires a literal :/= between the keyword and value: (["']?\s*[:=]\s*["']?)[^\s"'&;]+. A raw diagnostic containing Authorization: Bearer abc.def.ghi won't be redacted by the bearer branch since there's only whitespace, not :/=, between "Bearer" and the token (the JWT sub-pattern would only catch it if the token happens to look like a 3-part JWT). Given this module's stated purpose is to guarantee no credential-class substring reaches a report/prompt, this is a meaningful leak vector for the common Bearer <token> header shape.
🔒 Proposed fix: allow whitespace-only separators for select keywords
const CREDENTIAL_PATTERNS: Array<[RegExp, string]> = [
[/(token|secret|password|passwd|apikey|api_key|auth|bearer|session|cookie)(["']?\s*[:=]\s*["']?)[^\s"'&;]+/gi, '$1$2[redacted]'],
+ [/\bbearer\s+[^\s"'&;]+/gi, 'bearer [redacted]'],
[/eyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{4,}/g, '[redacted-jwt]'],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const CREDENTIAL_PATTERNS: Array<[RegExp, string]> = [ | |
| [/(token|secret|password|passwd|apikey|api_key|auth|bearer|session|cookie)(["']?\s*[:=]\s*["']?)[^\s"'&;]+/gi, '$1$2[redacted]'], | |
| [/eyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{4,}/g, '[redacted-jwt]'], | |
| [/\b(sk|pk|rk|ak)-[A-Za-z0-9_-]{12,}\b/g, '[redacted-key]'], | |
| [/[?&](code|state|id_token|access_token|refresh_token)=[^\s&"']+/gi, '?[redacted-param]'], | |
| ]; | |
| const CREDENTIAL_PATTERNS: Array<[RegExp, string]> = [ | |
| [/(token|secret|password|passwd|apikey|api_key|auth|bearer|session|cookie)(["']?\s*[:=]\s*["']?)[^\s"'&;]+/gi, '$1$2[redacted]'], | |
| [/\bbearer\s+[^\s"'&;]+/gi, 'bearer [redacted]'], | |
| [/eyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{4,}/g, '[redacted-jwt]'], | |
| [/\b(sk|pk|rk|ak)-[A-Za-z0-9_-]{12,}\b/g, '[redacted-key]'], | |
| [/[?&](code|state|id_token|access_token|refresh_token)=[^\s&"']+/gi, '?[redacted-param]'], | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@registry/curated/system/browser-automation/src/attach/errors.ts` around lines
44 - 49, Update CREDENTIAL_PATTERNS so the bearer credential form also redacts
tokens separated from “Bearer” by whitespace alone, while preserving the
existing colon/equal assignment handling for other credential keywords. Ensure
Authorization-style values such as “Bearer <token>” are fully replaced before
diagnostic text is emitted.
| const CODE_MATCHERS: Array<[RegExp, AttachErrorCode]> = [ | ||
| [/timeout.*connectovercdp|connectovercdp.*timeout|cdp.*timeout/i, 'CDP_TIMEOUT'], | ||
| [/econnrefused|ws error|socket hang up|cdp.*unavailable/i, 'CDP_UNAVAILABLE'], | ||
| [/javascript through applescript is turned off/i, 'JS_DISABLED'], | ||
| [/not authorized to send apple events|accessibility/i, 'ACCESSIBILITY_DENIED'], | ||
| [/profile.*mismatch|wrong profile/i, 'PROFILE_MISMATCH'], | ||
| [/tab .* not found|window .* not found|target closed/i, 'TAB_CLOSED'], | ||
| [/navigation.*timeout|nav.*timeout|net::err_timed_out/i, 'NAV_TIMEOUT'], | ||
| [/policy|blocked|not allowed/i, 'POLICY_BLOCKED'], | ||
| [/lease/i, 'LEASE_DENIED'], | ||
| [/devtoolsactiveport.*malformed|stale.*port/i, 'STALE_PORT_FILE'], | ||
| ]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
/lease/i matcher will misclassify unrelated messages containing "please".
Line 72's /lease/i has no word boundary, so any thrown message containing the substring "please" (e.g. "please retry", "please wait") is incorrectly classified as LEASE_DENIED. Add \b anchors, or require the fuller "lease held"/"lease" phrasing actually thrown by lease.ts (attach lease held by pid ...).
🩹 Proposed fix
- [/lease/i, 'LEASE_DENIED'],
+ [/\blease\b/i, 'LEASE_DENIED'],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const CODE_MATCHERS: Array<[RegExp, AttachErrorCode]> = [ | |
| [/timeout.*connectovercdp|connectovercdp.*timeout|cdp.*timeout/i, 'CDP_TIMEOUT'], | |
| [/econnrefused|ws error|socket hang up|cdp.*unavailable/i, 'CDP_UNAVAILABLE'], | |
| [/javascript through applescript is turned off/i, 'JS_DISABLED'], | |
| [/not authorized to send apple events|accessibility/i, 'ACCESSIBILITY_DENIED'], | |
| [/profile.*mismatch|wrong profile/i, 'PROFILE_MISMATCH'], | |
| [/tab .* not found|window .* not found|target closed/i, 'TAB_CLOSED'], | |
| [/navigation.*timeout|nav.*timeout|net::err_timed_out/i, 'NAV_TIMEOUT'], | |
| [/policy|blocked|not allowed/i, 'POLICY_BLOCKED'], | |
| [/lease/i, 'LEASE_DENIED'], | |
| [/devtoolsactiveport.*malformed|stale.*port/i, 'STALE_PORT_FILE'], | |
| ]; | |
| const CODE_MATCHERS: Array<[RegExp, AttachErrorCode]> = [ | |
| [/timeout.*connectovercdp|connectovercdp.*timeout|cdp.*timeout/i, 'CDP_TIMEOUT'], | |
| [/econnrefused|ws error|socket hang up|cdp.*unavailable/i, 'CDP_UNAVAILABLE'], | |
| [/javascript through applescript is turned off/i, 'JS_DISABLED'], | |
| [/not authorized to send apple events|accessibility/i, 'ACCESSIBILITY_DENIED'], | |
| [/profile.*mismatch|wrong profile/i, 'PROFILE_MISMATCH'], | |
| [/tab .* not found|window .* not found|target closed/i, 'TAB_CLOSED'], | |
| [/navigation.*timeout|nav.*timeout|net::err_timed_out/i, 'NAV_TIMEOUT'], | |
| [/policy|blocked|not allowed/i, 'POLICY_BLOCKED'], | |
| [/\blease\b/i, 'LEASE_DENIED'], | |
| [/devtoolsactiveport.*malformed|stale.*port/i, 'STALE_PORT_FILE'], | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@registry/curated/system/browser-automation/src/attach/errors.ts` around lines
63 - 74, Update the LEASE_DENIED matcher in CODE_MATCHERS to match “lease” as a
standalone word, or the specific lease-held message produced by lease.ts, so
unrelated text such as “please retry” is not classified as a lease denial.
| function defaultIsPidAlive(pid: number): boolean { | ||
| try { | ||
| process.kill(pid, 0); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file structure first.
git ls-files registry/curated/system/browser-automation/src/attach | sed -n '1,120p'
echo
echo "== outline =="
ast-grep outline registry/curated/system/browser-automation/src/attach/lease.ts --view expanded || true
echo
echo "== file with line numbers =="
cat -n registry/curated/system/browser-automation/src/attach/lease.ts | sed -n '1,220p'
echo
echo "== references to defaultIsPidAlive / process.kill(pid, 0) =="
rg -n "defaultIsPidAlive|process\.kill\(pid,\s*0\)|isPidAlive" registry/curated/system/browser-automation/src -SRepository: framerslab/agentos-extensions
Length of output: 7992
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect callers/usages of the lease module to understand the failure mode.
rg -n "lease\.ts|attach/lease|isPidAlive|defaultIsPidAlive|EPERM|ESRCH" registry/curated/system/browser-automation/src -SRepository: framerslab/agentos-extensions
Length of output: 1789
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the minimal surrounding context in the file.
sed -n '1,220p' registry/curated/system/browser-automation/src/attach/lease.tsRepository: framerslab/agentos-extensions
Length of output: 4081
Treat EPERM as live in defaultIsPidAlive.
process.kill(pid, 0) can throw EPERM for an existing PID you can’t signal. Returning false there lets a second process reclaim a live lease; return false only for ESRCH, true for EPERM, and rethrow unexpected errors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@registry/curated/system/browser-automation/src/attach/lease.ts` around lines
47 - 53, Update defaultIsPidAlive to inspect the error code from
process.kill(pid, 0): return false only for ESRCH, return true for EPERM, and
rethrow any other unexpected error instead of treating all failures as dead
processes.
| export function acquireLease(opts: LeaseOptions): AttachLease { | ||
| const now = opts.now ?? Date.now; | ||
| const alive = opts.isPidAlive ?? defaultIsPidAlive; | ||
| const existing = existsSync(opts.file) ? readLease(opts.file) : undefined; | ||
| if (existing) { | ||
| const fresh = existing.expiresAt > now(); | ||
| if (fresh && alive(existing.pid)) { | ||
| throw new AttachError( | ||
| 'LEASE_DENIED', | ||
| `attach lease held by pid ${existing.pid} until ${new Date(existing.expiresAt).toISOString()}`, | ||
| ); | ||
| } | ||
| } | ||
| const lease: AttachLease = { | ||
| pid: opts.pid ?? process.pid, | ||
| nonce: randomBytes(16).toString('hex'), | ||
| acquiredAt: now(), | ||
| expiresAt: now() + (opts.ttlMs ?? DEFAULT_TTL_MS), | ||
| }; | ||
| mkdirSync(dirname(opts.file), { recursive: true }); | ||
| writeFileSync(opts.file, JSON.stringify(lease)); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Make acquisition and replacement atomic.
The existsSync → read → liveness check → writeFileSync sequence lets two processes both decide the lease is available and both return a lease; the later write merely overwrites the first. The same non-atomic read/validate/unlink pattern in releaseLease can delete a lease reclaimed by another process after the nonce check. Protect acquire, stale reclaim, and release with one atomic guard/CAS-style protocol rather than overwritable writes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@registry/curated/system/browser-automation/src/attach/lease.ts` around lines
70 - 90, Update acquireLease and releaseLease to use one atomic CAS-style guard
for lease ownership: create the lease file exclusively when absent, atomically
reclaim stale leases only if the expected nonce/content still matches, and never
overwrite a competing live lease. Apply the same compare-and-delete protocol in
releaseLease so it cannot remove a lease replaced after validation, while
preserving existing liveness, expiry, and AttachError behavior.
| export async function withDeadline<T>(p: Promise<T>, ms: number, label: string): Promise<T> { | ||
| let timer: ReturnType<typeof setTimeout> | undefined; | ||
| const gate = new Promise<never>((_, reject) => { | ||
| timer = setTimeout(() => reject(new AttachError('NAV_TIMEOUT', `${label} exceeded ${ms}ms deadline`)), ms); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby callers/types.
git ls-files registry/curated/system/browser-automation/src/attach/state.ts
wc -l registry/curated/system/browser-automation/src/attach/state.ts
sed -n '1,160p' registry/curated/system/browser-automation/src/attach/state.ts
printf '\n--- callers ---\n'
rg -n "withDeadline\\(" registry/curated/system/browser-automation/src -S
printf '\n--- deadline-related validation ---\n'
rg -n "NAV_TIMEOUT|deadline|timeout" registry/curated/system/browser-automation/src -SRepository: framerslab/agentos-extensions
Length of output: 6140
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- BrowserService timeout wiring ---'
sed -n '1,180p' registry/curated/system/browser-automation/src/BrowserService.ts
printf '\n%s\n' '--- index timeout wiring ---'
sed -n '1,140p' registry/curated/system/browser-automation/src/index.ts
printf '\n%s\n' '--- attach tests around withDeadline ---'
sed -n '100,150p' registry/curated/system/browser-automation/src/attach/__tests__/attach-primitives.test.tsRepository: framerslab/agentos-extensions
Length of output: 12041
🏁 Script executed:
node - <<'JS'
const cases = [NaN, -5, 0, 0.2, 1, Infinity, 2_147_483_647, 2_147_483_648, 1e12];
for (const ms of cases) {
const t = setTimeout(() => {}, ms);
const info = {
input: ms,
_idleTimeout: t._idleTimeout,
};
clearTimeout(t);
console.log(JSON.stringify(info));
}
JSRepository: framerslab/agentos-extensions
Length of output: 1072
Reject invalid deadlines before scheduling.
setTimeout coerces NaN, <= 0, and values above 2_147_483_647 to ~1ms, so an invalid or oversized ms turns into an immediate NAV_TIMEOUT. Validate ms as a finite positive integer first.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@registry/curated/system/browser-automation/src/attach/state.ts` around lines
73 - 76, Validate ms at the start of withDeadline before constructing the gate
or scheduling setTimeout, requiring it to be a finite positive integer no
greater than 2,147,483,647. Reject invalid values with the existing
error-handling convention, while preserving the current timeout behavior for
valid deadlines.
| const PRIVATE_HOST_RE = | ||
| /^(localhost|127\.\d+\.\d+\.\d+|0\.0\.0\.0|10\.\d+\.\d+\.\d+|192\.168\.\d+\.\d+|172\.(1[6-9]|2\d|3[01])\.\d+\.\d+|\[::1\]|.*\.local)$/i; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and nearby tests
printf '\n== file list ==\n'
git ls-files 'registry/curated/system/browser-automation/src/attach/*' 'registry/curated/system/browser-automation/src/attach/**' | sed -n '1,120p'
printf '\n== outline ==\n'
ast-grep outline registry/curated/system/browser-automation/src/attach/url-policy.ts --view expanded || true
printf '\n== line-numbered source ==\n'
cat -n registry/curated/system/browser-automation/src/attach/url-policy.ts | sed -n '1,220p'
printf '\n== related tests/search ==\n'
rg -n "PRIVATE_HOST_RE|169\\.254|fe80|fc00|fd00|localhost|0\\.0\\.0\\.0|172\\.(1[6-9]|2\\d|3[01])" registry/curated/system/browser-automation/src -g '*.{ts,tsx,js,jsx}' || trueRepository: framerslab/agentos-extensions
Length of output: 5242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the policy logic in detail if the file is short enough.
wc -l registry/curated/system/browser-automation/src/attach/url-policy.ts
sed -n '1,220p' registry/curated/system/browser-automation/src/attach/url-policy.tsRepository: framerslab/agentos-extensions
Length of output: 3022
Block 169.254.0.0/16 in the private-host denylist
This policy still allows 169.254.169.254, the common cloud metadata endpoint. Add a regression test for that host.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@registry/curated/system/browser-automation/src/attach/url-policy.ts` around
lines 36 - 37, Update PRIVATE_HOST_RE to deny the entire 169.254.0.0/16
link-local range, including 169.254.169.254, and add a regression test verifying
that metadata endpoint is rejected.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
registry/curated/system/browser-automation/src/attach/__tests__/AttachController.test.ts (1)
12-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpy backend doesn't model the real backends' lazy claim in
probeIdentity. Incdp.ts/jxa.ts,probeIdentity()invokesclaimAgentTab()when no tab is claimed, so the controller ends up claiming twice; this spy treats the two as independent, so the suite passes and can't catch that regression. Consider a spy whoseprobeIdentitycallsclaimAgentTabto assert the tab is claimed exactly once.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@registry/curated/system/browser-automation/src/attach/__tests__/AttachController.test.ts` around lines 12 - 41, The makeBackend test spy does not model probeIdentity lazily claiming a tab, so it cannot detect duplicate claims. Update probeIdentity in makeBackend to invoke claimAgentTab when no tab has been claimed, while tracking the claimed state so repeated calls reuse it; preserve the existing identity result and enable tests to assert claimAgentTab occurs exactly once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@registry/curated/system/browser-automation/src/attach/AttachController.ts`:
- Line 142: Update the attach flow around AttachController and requireLease so
LEASE_DENIED errors from foreign takeover are caught before they escape,
recorded as lastError, and transition the controller state from ready to failed.
Ensure subsequent operations fail fast without re-probing the lease.
In `@registry/curated/system/browser-automation/src/attach/backends/cdp.ts`:
- Around line 90-97: Make claimAgentTab idempotent in both backends: in
registry/curated/system/browser-automation/src/attach/backends/cdp.ts lines
90-97, return MARKER immediately when this.page is already set; in
registry/curated/system/browser-automation/src/attach/backends/jxa.ts lines
151-157, return the existing claimed window/tab handle when this.claimed is set
instead of rerunning the claim command. Preserve the existing claim behavior
when no handle has been cached.
In `@registry/curated/system/browser-automation/src/attach/backends/jxa.ts`:
- Around line 76-81: The goto branch in the command handler must keep its
polling and trailing delay within the configured execTimeoutMs deadline. Update
the loop around t.loading() to use a poll budget below the default timeout, or
derive the iteration count from execTimeoutMs, while preserving the final return
of t.url() for slow but completed navigations.
---
Nitpick comments:
In
`@registry/curated/system/browser-automation/src/attach/__tests__/AttachController.test.ts`:
- Around line 12-41: The makeBackend test spy does not model probeIdentity
lazily claiming a tab, so it cannot detect duplicate claims. Update
probeIdentity in makeBackend to invoke claimAgentTab when no tab has been
claimed, while tracking the claimed state so repeated calls reuse it; preserve
the existing identity result and enable tests to assert claimAgentTab occurs
exactly once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c5ae1c7-b6ec-4ec4-a1ae-b0f3dc2d4293
📒 Files selected for processing (5)
registry/curated/system/browser-automation/src/attach/AttachController.tsregistry/curated/system/browser-automation/src/attach/__tests__/AttachController.test.tsregistry/curated/system/browser-automation/src/attach/__tests__/jxa-args.test.tsregistry/curated/system/browser-automation/src/attach/backends/cdp.tsregistry/curated/system/browser-automation/src/attach/backends/jxa.ts
| if (!verdict.allowed) { | ||
| throw new AttachError('POLICY_BLOCKED', `navigation to "${url}" rejected: ${verdict.reason}`); | ||
| } | ||
| requireLease(this.opts.leaseFile, this.lease!.nonce); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
LEASE_DENIED on foreign takeover leaves the controller in ready with a stale lastError. requireLease throws before the try/state transition, so after a detected takeover status().state stays ready and lastError is not updated. Consider recording the error and moving to failed so subsequent ops fail fast rather than re-probing the lease each call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@registry/curated/system/browser-automation/src/attach/AttachController.ts` at
line 142, Update the attach flow around AttachController and requireLease so
LEASE_DENIED errors from foreign takeover are caught before they escape,
recorded as lastError, and transition the controller state from ready to failed.
Ensure subsequent operations fail fast without re-probing the lease.
| async claimAgentTab(): Promise<string> { | ||
| const ctx = this.browser?.contexts()[0]; | ||
| if (!ctx) throw new AttachError('CDP_UNAVAILABLE', 'no default browser context on the attached browser'); | ||
| const page = (await ctx.newPage()) as NonNullable<typeof this.page>; | ||
| await page.evaluate(`window.name = ${JSON.stringify(MARKER)}`); | ||
| this.page = page; | ||
| return MARKER; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make claimAgentTab() idempotent to stop the double-claim. AttachController.claim() calls probeIdentity() then claimAgentTab(), and both backends already claim the agent tab lazily inside probeIdentity() — so the controller's explicit claimAgentTab() is a second claim on an already-claimed session. Have each backend return the existing handle instead of re-claiming.
-
registry/curated/system/browser-automation/src/attach/backends/cdp.ts#L90-L97: return early with the existing tab whenthis.pageis already set (e.g.if (this.page) return MARKER;) instead of always callingctx.newPage(), which currently orphans the probed tab and repointsthis.pageat an empty new tab. -
registry/curated/system/browser-automation/src/attach/backends/jxa.ts#L151-L157: return the cached handle whenthis.claimedis set (e.g.if (this.claimed) return \${this.claimed.wid} ${this.claimed.tid}`;) instead of re-running theclaimcommand, which currently fails withNO_BLANK_TABor binds a different tab after the probe consumed theabout:blank` tab.
📍 Affects 2 files
registry/curated/system/browser-automation/src/attach/backends/cdp.ts#L90-L97(this comment)registry/curated/system/browser-automation/src/attach/backends/jxa.ts#L151-L157
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@registry/curated/system/browser-automation/src/attach/backends/cdp.ts` around
lines 90 - 97, Make claimAgentTab idempotent in both backends: in
registry/curated/system/browser-automation/src/attach/backends/cdp.ts lines
90-97, return MARKER immediately when this.page is already set; in
registry/curated/system/browser-automation/src/attach/backends/jxa.ts lines
151-157, return the existing claimed window/tab handle when this.claimed is set
instead of rerunning the claim command. Preserve the existing claim behavior
when no handle has been cached.
| if (cmd === 'goto') { | ||
| t.url = argv[4]; | ||
| for (let i = 0; i < 80; i++) { delay(0.5); try { if (t.loading() === false) break; } catch (e) { break; } } | ||
| delay(1); | ||
| return t.url(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Driver poll budget exceeds execTimeoutMs. The goto poll runs 80 × delay(0.5) = 40s plus a trailing delay(1), so on a slow/hanging page the embedded loop needs ~41s while the default execTimeoutMs is 40s (Line 106) — osascript is killed before the loop can return the landed URL, turning every slow load into a spurious UNKNOWN/timeout. Bound the poll below the exec deadline (or derive it from execTimeoutMs).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@registry/curated/system/browser-automation/src/attach/backends/jxa.ts` around
lines 76 - 81, The goto branch in the command handler must keep its polling and
trailing delay within the configured execTimeoutMs deadline. Update the loop
around t.loading() to use a poll budget below the default timeout, or derive the
iteration count from execTimeoutMs, while preserving the final return of t.url()
for slow but completed navigations.
…tration Five tools over AttachController — status/claim/goto/read/release — and NO raw-JS tool (arbitrary page script would defeat the read-only-commerce and no-cookie-export contract). status/read are non-side-effecting so a deny-side-effects policy still permits inspection while gating navigation. Page reads are labeled untrusted. The pack registers them only when attach.expectedIdentity is set, as default-off descriptors — launch-mode tools are unchanged in both modes.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@registry/curated/system/browser-automation/src/index.ts`:
- Around line 150-151: Update the backend selection around CdpBackend and
JxaBackend so an omitted transport never defaults to JxaBackend on non-macOS
hosts. Choose a platform-appropriate transport and profileRoot defaults, or
reject unsupported configurations during pack creation; preserve explicit
transport behavior while ensuring registered packs can successfully claim on
supported platforms.
- Around line 152-157: Retain the AttachController created in the pack
initialization flow at pack scope instead of limiting it to the local block,
then update onDeactivate to call detach() when a session was successfully
claimed. Add a regression test covering claim followed by deactivation and
verify that the attach lease is released.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d3b9eee1-dc57-466c-9b82-836935fa4e37
📒 Files selected for processing (4)
registry/curated/system/browser-automation/src/attach/__tests__/pack-registration.test.tsregistry/curated/system/browser-automation/src/attach/__tests__/tools.test.tsregistry/curated/system/browser-automation/src/attach/tools.tsregistry/curated/system/browser-automation/src/index.ts
| const backendOpts = { profileRoot: a.profileRoot, identityProbeUrl: a.identityProbeUrl }; | ||
| const backend = a.transport === 'cdp' ? new CdpBackend(backendOpts) : new JxaBackend(backendOpts); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not select JXA by default on every platform.
When transport is omitted, this always creates JxaBackend, whose operations invoke macOS osascript. On non-macOS hosts, opt-in registration succeeds but claim cannot work. Select platform-appropriate defaults (including profile-root defaults), or reject unsupported configurations during pack creation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@registry/curated/system/browser-automation/src/index.ts` around lines 150 -
151, Update the backend selection around CdpBackend and JxaBackend so an omitted
transport never defaults to JxaBackend on non-macOS hosts. Choose a
platform-appropriate transport and profileRoot defaults, or reject unsupported
configurations during pack creation; preserve explicit transport behavior while
ensuring registered packs can successfully claim on supported platforms.
| const controller = new AttachController({ | ||
| backend, | ||
| leaseFile: a.leaseFile ?? join(homedir(), '.wunderland', 'attach.lease'), | ||
| expectedIdentity: a.expectedIdentity, | ||
| urlPolicy: a.allowHosts ? { allowHosts: a.allowHosts } : undefined, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Release the attach session during pack deactivation.
controller is scoped to this block, so onDeactivate cannot call detach(). Deactivating after a successful claim leaves the attach lease held until process exit and may strand the transport. Retain the controller and detach it during deactivation; add a regression test that claims, deactivates, then verifies the lease is released.
Proposed cleanup wiring
const attachDescriptors = [];
+let attachController: { detach(): Promise<void> } | undefined;
// ...
const controller = new AttachController({
backend,
leaseFile: a.leaseFile ?? join(homedir(), '.wunderland', 'attach.lease'),
expectedIdentity: a.expectedIdentity,
urlPolicy: a.allowHosts ? { allowHosts: a.allowHosts } : undefined,
});
+attachController = controller;
// ...
onDeactivate: async () => {
- await browser.shutdown();
- sharedBrowser = null;
- sharedCaptcha = null;
- sharedProxy = null;
+ try {
+ await attachController?.detach();
+ } finally {
+ await browser.shutdown();
+ sharedBrowser = null;
+ sharedCaptcha = null;
+ sharedProxy = null;
+ }
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const controller = new AttachController({ | |
| backend, | |
| leaseFile: a.leaseFile ?? join(homedir(), '.wunderland', 'attach.lease'), | |
| expectedIdentity: a.expectedIdentity, | |
| urlPolicy: a.allowHosts ? { allowHosts: a.allowHosts } : undefined, | |
| }); | |
| const controller = new AttachController({ | |
| backend, | |
| leaseFile: a.leaseFile ?? join(homedir(), '.wunderland', 'attach.lease'), | |
| expectedIdentity: a.expectedIdentity, | |
| urlPolicy: a.allowHosts ? { allowHosts: a.allowHosts } : undefined, | |
| }); | |
| attachController = controller; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@registry/curated/system/browser-automation/src/index.ts` around lines 152 -
157, Retain the AttachController created in the pack initialization flow at pack
scope instead of limiting it to the local block, then update onDeactivate to
call detach() when a session was successfully claimed. Add a regression test
covering claim followed by deactivation and verify that the attach lease is
released.
1 participant
Foundation for attach-mode browsing (driving an already-running, logged-in Chrome — never launching or closing it). Adds six pure, unit-tested modules under
src/attach/: DevToolsActivePort parser, SingletonLock PID resolver, deny-by-default https-only URL policy (private-net/credential blocks + redirect revalidation), structured errors with credential redaction, a no-replay state machine with op deadlines, and a PID+nonce-fenced cross-process lease. No behavior change to the existing launch-mode tools; nothing registers these yet (controller + typed tools follow in Phase 2/3).Summary by Sourcery
Introduce foundational attach-mode primitives for driving an existing Chrome session without launch/shutdown responsibilities.
New Features:
Tests:
Summary by CodeRabbit
browser_attach_*tools (status/claim/goto/read/release) for controlling an existing authenticated browser session.