Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,16 @@ export interface AttachBackend {
readTab(tab: string, selector?: string, maxChars?: number): Promise<string>;
}

/**
* Human-in-the-loop navigation decision. Called before every `goto` (after the
* URL clears the deny-by-default policy). Return `false`/`{ approved: false }`
* to block the navigation with a `POLICY_BLOCKED` result. Async so a caller can
* prompt a real operator.
*/
export type NavigationApprover = (
url: string,
) => boolean | { approved: boolean; reason?: string } | Promise<boolean | { approved: boolean; reason?: string }>;
Comment on lines +56 to +58

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Guard against null decisions from NavigationApprover to avoid runtime errors.

In goto, NavigationApprover results are handled as either boolean or object, but typeof null === 'object'. If an approver erroneously returns null, the branch typeof decision === 'object' && decision.reason will dereference reason on null and throw. Please add a null-safe check (e.g. decision && typeof decision === 'object') to avoid this runtime error while keeping the existing API.


/** Controller configuration. */
export interface AttachControllerOptions {
backend: AttachBackend;
Expand All @@ -62,6 +72,17 @@ export interface AttachControllerOptions {
urlPolicy?: UrlPolicyOptions;
/** Per-operation deadline in ms (default 45s). */
deadlineMs?: number;
/**
* Dry-run: navigation and reads are validated and reported but NOT performed
* against the browser. Lets a mission preview its plan (or run in CI) without
* touching the live session.
*/
dryRun?: boolean;
/**
* HITL gate on navigation. Runs after the URL policy passes; a denial blocks
* the goto. Use to require operator approval for each page an assistant opens.
*/
onNavigate?: NavigationApprover;
}

/** Status snapshot returned by {@link AttachController.status}. */
Expand All @@ -71,6 +92,14 @@ export interface AttachStatus {
leaseHeld: boolean;
agentTab?: string;
lastError?: StructuredAttachError;
/** Session paused — operations are refused until resume(). */
paused: boolean;
/** Dry-run mode — navigation/reads are simulated, not performed. */
dryRun: boolean;
/** Count of navigations performed this session. */
navigations: number;
/** Last URL the agent tab navigated to. */
lastUrl?: string;
}

/**
Expand All @@ -84,10 +113,15 @@ export class AttachController {
private lease?: AttachLease;
private tab?: string;
private lastError?: StructuredAttachError;
private paused = false;
private dryRun: boolean;
private navigations = 0;
private lastUrl?: string;

constructor(opts: AttachControllerOptions) {
this.backend = opts.backend;
this.opts = opts;
this.dryRun = !!opts.dryRun;
}

private get deadline(): number {
Expand All @@ -102,9 +136,28 @@ export class AttachController {
leaseHeld: !!this.lease,
agentTab: this.tab,
lastError: this.lastError,
paused: this.paused,
dryRun: this.dryRun,
navigations: this.navigations,
lastUrl: this.lastUrl,
};
}

/** Pause the session — navigation/read refuse until {@link resume}. */
pause(): void {
this.paused = true;
}

/** Resume a paused session. */
resume(): void {
this.paused = false;
}

/** Toggle dry-run at runtime (simulate navigation/reads, don't perform them). */
setDryRun(on: boolean): void {
this.dryRun = on;
}

/**
* Acquire the lease, open the transport, verify profile identity, and claim
* the marker agent tab. Aborts (never launches) on any verification failure.
Expand Down Expand Up @@ -132,17 +185,34 @@ export class AttachController {
}
}

/** Navigate the agent tab (policy-checked, redirect-revalidated). */
/** Navigate the agent tab (policy-checked, HITL-gated, redirect-revalidated). */
async goto(url: string): Promise<string> {
this.requireReady();
const verdict = isNavigationAllowed(url, this.opts.urlPolicy);
if (!verdict.allowed) {
throw new AttachError('POLICY_BLOCKED', `navigation to "${url}" rejected: ${verdict.reason}`);
}
// HITL gate: after the policy passes, an approver may still block the page.
if (this.opts.onNavigate) {
const decision = await this.opts.onNavigate(url);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Put async approval under the operation guard

With an async onNavigate implementation, such as one waiting for an operator, this await happens while the controller still reports ready and before the existing deadline-wrapped navigation path starts. In hosts that can dispatch another attach tool call during that prompt, the second call can pass requireReady() and navigate/read the same agent tab before the first approved navigation resumes; if the prompt stalls, deadlineMs also no longer bounds the goto() call. Move the approval wait under a busy state/mutex and deadline, restoring ready on denial.

Useful? React with 👍 / 👎.

const approved = typeof decision === 'boolean' ? decision : decision.approved;
if (!approved) {
const reason = typeof decision === 'object' && decision.reason ? decision.reason : 'operator denied';
throw new AttachError('POLICY_BLOCKED', `navigation to "${url}" denied by approver: ${reason}`);
}
}
requireLease(this.opts.leaseFile, this.lease!.nonce);
Comment on lines +196 to 204

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Bound approval waits and re-check pause state.

Line 197 awaits onNavigate without deadlineMs, so an operator prompt that never resolves leaves goto() stuck indefinitely. A pause issued during that await is also bypassed when approval later resolves. Wrap the decision in withDeadline(...) and call requireReady() again before the dry-run/backend side effect.

Proposed fix
 if (this.opts.onNavigate) {
-  const decision = await this.opts.onNavigate(url);
+  const decision = await withDeadline(
+    Promise.resolve(this.opts.onNavigate(url)),
+    this.deadline,
+    'onNavigate',
+  );
   const approved = typeof decision === 'boolean' ? decision : decision.approved;
   if (!approved) {
     const reason = typeof decision === 'object' && decision.reason ? decision.reason : 'operator denied';
     throw new AttachError('POLICY_BLOCKED', `navigation to "${url}" denied by approver: ${reason}`);
   }
 }
+this.requireReady();
 requireLease(this.opts.leaseFile, this.lease!.nonce);
📝 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.

Suggested change
if (this.opts.onNavigate) {
const decision = await this.opts.onNavigate(url);
const approved = typeof decision === 'boolean' ? decision : decision.approved;
if (!approved) {
const reason = typeof decision === 'object' && decision.reason ? decision.reason : 'operator denied';
throw new AttachError('POLICY_BLOCKED', `navigation to "${url}" denied by approver: ${reason}`);
}
}
requireLease(this.opts.leaseFile, this.lease!.nonce);
if (this.opts.onNavigate) {
const decision = await withDeadline(
Promise.resolve(this.opts.onNavigate(url)),
this.deadline,
'onNavigate',
);
const approved = typeof decision === 'boolean' ? decision : decision.approved;
if (!approved) {
const reason = typeof decision === 'object' && decision.reason ? decision.reason : 'operator denied';
throw new AttachError('POLICY_BLOCKED', `navigation to "${url}" denied by approver: ${reason}`);
}
}
this.requireReady();
requireLease(this.opts.leaseFile, this.lease!.nonce);
🤖 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`
around lines 196 - 204, Update the navigation approval flow in the
AttachController method containing onNavigate to await the decision through
withDeadline using the applicable deadlineMs, preventing an unresolved operator
prompt from blocking indefinitely. After the approval resolves and before
requireLease or any dry-run/backend navigation side effect, call requireReady()
again so pauses issued during the wait are enforced; preserve the existing
denial handling.

// Dry-run: report the URL as the landed target without touching the browser.
if (this.dryRun) {
this.navigations += 1;
this.lastUrl = url;
return url;
}
this.machine.to('navigating');
try {
const landed = await withDeadline(this.backend.gotoTab(this.tab!, url), this.deadline, 'goto');
this.navigations += 1;
this.lastUrl = landed;
const landedVerdict = revalidateRedirect(landed, this.opts.urlPolicy);
if (!landedVerdict.allowed) {
this.machine.to('ready');
Expand All @@ -161,6 +231,7 @@ export class AttachController {
async read(selector?: string, maxChars?: number): Promise<string> {
this.requireReady();
requireLease(this.opts.leaseFile, this.lease!.nonce);
if (this.dryRun) return '[dry-run] read skipped';
this.machine.to('reading');
try {
const text = await withDeadline(this.backend.readTab(this.tab!, selector, maxChars), this.deadline, 'read');
Expand Down Expand Up @@ -193,6 +264,9 @@ export class AttachController {
}

private requireReady(): void {
if (this.paused) {
throw new AttachError('UNKNOWN', 'attach session is paused — call resume() before further operations');
}
if (this.machine.state !== 'ready' || !this.tab || !this.lease) {
throw new AttachError('UNKNOWN', `attach session not ready (state=${this.machine.state}) — call claim() first`);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,52 @@ describe('AttachController', () => {
expect(c.status().state).toBe('disconnected');
});

it('HITL approver can deny a policy-clean navigation', async () => {
const { backend, calls } = makeBackend();
const c = controller(backend, { onNavigate: (url) => !url.includes('secret') });
await c.claim();
await expect(c.goto('https://ok.example.com/secret')).rejects.toMatchObject({ code: 'POLICY_BLOCKED' });
expect(calls.filter((x) => x.startsWith('goto:'))).toHaveLength(0); // never reached the backend
await expect(c.goto('https://ok.example.com/public')).resolves.toContain('public');
await c.detach();
});

it('dry-run reports the target without touching the backend, and counts navigations', async () => {
const { backend, calls } = makeBackend();
const c = controller(backend, { dryRun: true });
await c.claim();
expect(await c.goto('https://ok.example.com/')).toBe('https://ok.example.com/');
expect(await c.read()).toContain('dry-run');
expect(calls.filter((x) => x.startsWith('goto:'))).toHaveLength(0);
expect(c.status().dryRun).toBe(true);
expect(c.status().navigations).toBe(1);
expect(c.status().lastUrl).toBe('https://ok.example.com/');
await c.detach();
});

it('pause refuses operations until resume', async () => {
const { backend } = makeBackend();
const c = controller(backend);
await c.claim();
c.pause();
expect(c.status().paused).toBe(true);
await expect(c.goto('https://ok.example.com/')).rejects.toThrow(/paused/i);
c.resume();
await expect(c.goto('https://ok.example.com/')).resolves.toContain('ok.example.com');
await c.detach();
});

it('status tracks navigation count and last url on real ops', async () => {
const { backend } = makeBackend();
const c = controller(backend);
await c.claim();
await c.goto('https://a.example.com/');
await c.goto('https://b.example.com/');
expect(c.status().navigations).toBe(2);
expect(c.status().lastUrl).toBe('https://b.example.com/');
await c.detach();
});

it('ops enforce the lease nonce at call time (foreign takeover is detected)', async () => {
let t = 1_000_000;
const { backend } = makeBackend();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ describe('browser-automation attach registration', () => {
const attach = pack.descriptors.filter((d) => d.id.startsWith('browser_attach_'));
expect(attach.map((d) => d.id).sort()).toEqual([
'browser_attach_claim',
'browser_attach_control',
'browser_attach_goto',
'browser_attach_read',
'browser_attach_release',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { AttachController, type AttachBackend } from '../AttachController.js';
import {
ATTACH_TOOL_IDS,
AttachClaimTool,
AttachControlTool,
AttachGotoTool,
AttachReadTool,
AttachReleaseTool,
Expand Down Expand Up @@ -40,16 +41,27 @@ beforeEach(() => {
});

describe('attach tools', () => {
it('exposes exactly the five ids and no eval/js tool', () => {
it('exposes exactly the six ids and no eval/js tool', () => {
expect(ATTACH_TOOL_IDS).toEqual([
'browser_attach_status',
'browser_attach_claim',
'browser_attach_goto',
'browser_attach_read',
'browser_attach_release',
'browser_attach_control',
]);
expect(ATTACH_TOOL_IDS).not.toContain('browser_attach_eval');
expect(createAttachTools(controller)).toHaveLength(5);
expect(createAttachTools(controller)).toHaveLength(6);
});

it('control tool pauses, resumes, and toggles dry-run at runtime', async () => {
const ctl = new AttachControlTool(controller);
await new AttachClaimTool(controller).execute();
expect((await ctl.execute({ action: 'pause' })).data.paused).toBe(true);
expect((await ctl.execute({ action: 'resume' })).data.paused).toBe(false);
expect((await ctl.execute({ action: 'dry_run_on' })).data.dryRun).toBe(true);
expect((await ctl.execute({ action: 'dry_run_off' })).data.dryRun).toBe(false);
expect((await ctl.execute({ action: 'status' })).data.leaseHeld).toBe(true);
});

it('status and read are non-side-effecting; claim/goto/release are side-effecting', () => {
Expand Down
47 changes: 47 additions & 0 deletions registry/curated/system/browser-automation/src/attach/tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,51 @@ export class AttachReleaseTool {
}
}

/** `browser_attach_control` — runtime session controls (pause/resume/dry-run). */
export class AttachControlTool {
readonly id = 'browser_attach_control';
readonly name = 'browser_attach_control';
readonly displayName = 'Attach: session control';
readonly description =
'Control the attach session at runtime: pause (refuse further ops), resume, or toggle dry-run (simulate navigation/reads without touching the browser). Returns the updated status.';
readonly category = 'browser';
readonly version = '0.1.0';
readonly hasSideEffects = true;
readonly inputSchema = {
Comment on lines +138 to +147

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Align the execute argument typing with inputSchema for extensibility.

inputSchema treats action as a generic string enum, while execute uses a hard-coded union ('pause' | 'resume' | 'dry_run_on' | 'dry_run_off' | 'status'). This requires manual updates whenever new actions are added. Define a shared type (or derive the union from a constant) so inputSchema and execute stay in sync automatically.

type: 'object' as const,
properties: {
action: {
type: 'string',
enum: ['pause', 'resume', 'dry_run_on', 'dry_run_off', 'status'],
description: 'The control action to apply',
},
},
required: ['action'],
};
constructor(private controller: AttachController) {}
async execute(args: { action: 'pause' | 'resume' | 'dry_run_on' | 'dry_run_off' | 'status' }) {
return envelope(async () => {
switch (args.action) {
case 'pause':
this.controller.pause();
break;
case 'resume':
this.controller.resume();
break;
case 'dry_run_on':
this.controller.setDryRun(true);
break;
case 'dry_run_off':
this.controller.setDryRun(false);
break;
case 'status':
break;
}
return this.controller.status();
});
}
}

/** Instantiate the full attach tool set sharing one controller. */
export function createAttachTools(controller: AttachController) {
return [
Expand All @@ -142,6 +187,7 @@ export function createAttachTools(controller: AttachController) {
new AttachGotoTool(controller),
new AttachReadTool(controller),
new AttachReleaseTool(controller),
new AttachControlTool(controller),
];
}

Expand All @@ -152,4 +198,5 @@ export const ATTACH_TOOL_IDS = [
'browser_attach_goto',
'browser_attach_read',
'browser_attach_release',
'browser_attach_control',
] as const;
6 changes: 6 additions & 0 deletions registry/curated/system/browser-automation/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ export interface BrowserAutomationOptions {
transport?: 'jxa' | 'cdp';
/** Optional https host allowlist for navigation. */
allowHosts?: string[];
/** Start in dry-run (simulate navigation/reads without touching the browser). */
dryRun?: boolean;
/** Per-operation deadline in ms (default 45s). */
deadlineMs?: number;
};
}

Expand Down Expand Up @@ -154,6 +158,8 @@ export function createExtensionPack(context: ExtensionContext): ExtensionPack {
leaseFile: a.leaseFile ?? join(homedir(), '.wunderland', 'attach.lease'),
expectedIdentity: a.expectedIdentity,
urlPolicy: a.allowHosts ? { allowHosts: a.allowHosts } : undefined,
dryRun: a.dryRun,
deadlineMs: a.deadlineMs,
});
for (const tool of createAttachTools(controller)) {
attachDescriptors.push({ id: tool.id, kind: 'tool', priority: 50, enableByDefault: false, payload: tool });
Expand Down