Skip to content
Closed
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
4 changes: 3 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ dist/
build/
out/

# Example folder
# App specific folders
example/
browser-scripts/
knowledge/

# Output directories
output/
Expand Down
1 change: 1 addition & 0 deletions explorbot.config.example.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ interface PlaywrightConfig {
height: number;
};
args: string[];
setupScripts?: string[];
}

interface AppConfig {
Expand Down
8 changes: 7 additions & 1 deletion src/action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,15 @@ class Action {
private expectation: string | null = null;
public lastError: Error | null = null;
public playwrightHelper: any;
private explorer: any;

constructor(actor: CodeceptJS.I, stateManager: StateManager) {
constructor(actor: CodeceptJS.I, stateManager: StateManager, explorer?: any) {
this.actor = actor;
this.stateManager = stateManager;
this.experienceTracker = stateManager.getExperienceTracker();
this.config = ConfigParser.getInstance().getConfig();
this.playwrightHelper = container.helpers('Playwright');
this.explorer = explorer;
Copy link
Contributor

Choose a reason for hiding this comment

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

this breaks dependency hierarchy

action and managed by explorer, not vice versa

}

async caputrePageWithScreenshot(): Promise<ActionResult> {
Expand Down Expand Up @@ -194,6 +196,10 @@ class Action {
await recorder.add(() => sleep(this.config.action?.delay || 500)); // wait for the action to be executed
await recorder.promise();

if (this.explorer?.waitForPendingSetupScripts) {
Copy link

Choose a reason for hiding this comment

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

Suggestion: The call to waitForPendingSetupScripts on the loosely-typed explorer object only checks that the property exists, not that it is actually a function, so if a non-conforming object is ever passed in as explorer (for example via a misconfigured caller), this will throw a TypeError: this.explorer.waitForPendingSetupScripts is not a function at runtime and break the whole action execution. [possible bug]

Severity Level: Major ⚠️
- ❌ Action.execute may crash on misconfigured explorer.
- ❌ capturePageState() not reached after action.
- ⚠️ Before-hook setup scripts silently not awaited.
- ⚠️ Test flows using manual Action construction affected.
Suggested change
if (this.explorer?.waitForPendingSetupScripts) {
if (this.explorer && typeof this.explorer.waitForPendingSetupScripts === 'function') {
Steps of Reproduction ✅
1. Construct an Action with a non-function explorer value:

   - In any script or test instantiate Action as `new Action(actor, stateManager,
   explorer)` where the constructor is defined at `src/action.ts:41-48`
   (`constructor(actor: CodeceptJS.I, stateManager: StateManager, explorer?: any)`).

2. Pass a mis-shaped explorer object:

   - Provide `explorer = { waitForPendingSetupScripts: true }` (boolean) or any
   non-callable value for that property when creating the Action instance.

3. Execute an action that runs the normal execute flow:

   - Call `await action.execute("I.click('selector')")`. The `execute` function flow is in
   `src/action.ts` around lines 186-206 (where `codeFunction` is invoked, `recorder.add()`
   and `recorder.promise()` are awaited).

4. Observe the runtime TypeError immediately after navigation wait:

   - After `await recorder.promise()` completes, the code at `src/action.ts:199-201` runs:

     - `199 if (this.explorer?.waitForPendingSetupScripts) {`

     - `200 await this.explorer.waitForPendingSetupScripts();`

     - `201 }`

   - Because `this.explorer.waitForPendingSetupScripts` is not a function (it's boolean),
   awaiting it throws `TypeError: this.explorer.waitForPendingSetupScripts is not a
   function`.

   - The TypeError prevents the subsequent `capturePageState()` call at
   `src/action.ts:203` and aborts the action.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** src/action.ts
**Line:** 199:199
**Comment:**
	*Possible Bug: The call to `waitForPendingSetupScripts` on the loosely-typed `explorer` object only checks that the property exists, not that it is actually a function, so if a non-conforming object is ever passed in as `explorer` (for example via a misconfigured caller), this will throw a `TypeError: this.explorer.waitForPendingSetupScripts is not a function` at runtime and break the whole action execution.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

await this.explorer.waitForPendingSetupScripts();
}

const pageState = await this.capturePageState();
if (executedSteps.length > 0) {
codeString = executedSteps.join('\n');
Expand Down
1 change: 1 addition & 0 deletions src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ interface PlaywrightConfig {
height: number;
};
args?: string[];
setupScripts?: string[];
}

interface AgentConfig {
Expand Down
22 changes: 20 additions & 2 deletions src/explorer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type { ExplorbotConfig } from './config.js';
import { ConfigParser } from './config.js';
import type { UserResolveFunction } from './explorbot.js';
import { KnowledgeTracker } from './knowledge-tracker.js';
import { PageSetupManager } from './page-setup-manager.js';
import { Reporter } from './reporter.ts';
import { StateManager } from './state-manager.js';
import { Test } from './test-plan.ts';
Expand Down Expand Up @@ -47,6 +48,8 @@ class Explorer {
private options?: { show?: boolean; headless?: boolean; incognito?: boolean };
private reporter!: Reporter;
private otherTabs: TabInfo[] = [];
private pageSetupManager!: PageSetupManager;
private pendingSetupScripts: Promise<void> | null = null;

constructor(config: ExplorbotConfig, aiProvider: AIProvider, options?: { show?: boolean; headless?: boolean; incognito?: boolean }) {
this.config = config;
Expand All @@ -56,6 +59,7 @@ class Explorer {
this.stateManager = new StateManager({ incognito: this.options?.incognito });
this.knowledgeTracker = new KnowledgeTracker();
this.reporter = new Reporter();
this.pageSetupManager = new PageSetupManager(this, config);
}

private initializeContainer() {
Expand Down Expand Up @@ -180,6 +184,9 @@ class Explorer {
}
await this.playwrightHelper._startBrowser();
await this.playwrightHelper._createContextPage();

await this.pageSetupManager.loadSetupScripts();

const I = codeceptjs.container.support('I');

this.actor = I;
Expand All @@ -195,7 +202,13 @@ class Explorer {
}

createAction() {
return new Action(this.actor, this.stateManager);
return new Action(this.actor, this.stateManager, this);
}

async waitForPendingSetupScripts(): Promise<void> {
if (this.pendingSetupScripts) {
await this.pendingSetupScripts;
}
}

async visit(url: string) {
Expand Down Expand Up @@ -224,6 +237,8 @@ class Explorer {
await action.execute(`I.waitForElement(${JSON.stringify(waitForElement)})`);
}

await this.pageSetupManager.executeAfterNavigation(this.playwrightHelper.page, url);
Copy link

Choose a reason for hiding this comment

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

Suggestion: After adding the framenavigated listener that already invokes executeAfterNavigation, the explicit call to pageSetupManager.executeAfterNavigation inside visit() causes setup scripts to run twice for navigations triggered via visit(), which can double-submit forms or perform other non-idempotent actions and lead to inconsistent browser state. Instead, visit() should only wait for any pending setup started by the navigation listener. [logic error]

Severity Level: Critical 🚨
- ❌ Duplicate setup runs cause double form submissions.
- ❌ Cookie-banner dismissal may run twice unexpectedly.
- ⚠️ Explorer.visit flows may produce flaky captures.
Suggested change
await this.pageSetupManager.executeAfterNavigation(this.playwrightHelper.page, url);
await this.waitForPendingSetupScripts();
Steps of Reproduction ✅
1. Call Explorer.visit(url) (src/explorer.ts visit() function, see the line that executes
await this.pageSetupManager.executeAfterNavigation(this.playwrightHelper.page, url) at
line 240). visit() issues I.amOnPage(...) which triggers navigation.

2. The page navigation emits Playwright's 'framenavigated' event; the listener registered
in listenToStateChanged (src/explorer.ts framenavigated handler at lines ~328-337) also
calls pageSetupManager.executeAfterNavigation(...) for the same navigation.

3. Because visit() calls executeAfterNavigation directly (line 240) while the
framenavigated handler also calls it, the same setup scripts run twice for a single
navigation (PageSetupManager.executeAfterNavigation is invoked from both locations).

4. Observe duplicated side effects when running a non-idempotent setup script (e.g.,
browser-scripts that fill-and-submit login forms or click cookie banners) leading to
double submits or unexpected page state. The duplicate invocation originates from the two
call sites above.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** src/explorer.ts
**Line:** 240:240
**Comment:**
	*Logic Error: After adding the `framenavigated` listener that already invokes `executeAfterNavigation`, the explicit call to `pageSetupManager.executeAfterNavigation` inside `visit()` causes setup scripts to run twice for navigations triggered via `visit()`, which can double-submit forms or perform other non-idempotent actions and lead to inconsistent browser state. Instead, `visit()` should only wait for any pending setup started by the navigation listener.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.


return action;
}

Expand Down Expand Up @@ -313,10 +328,13 @@ class Explorer {
debugLog('Failed to get page title:', error);
}

// // Update state from navigation
this.stateManager.updateStateFromBasic(newUrl, newTitle, 'navigation');

await new Promise((resolve) => setTimeout(resolve, 500));

this.pendingSetupScripts = this.pageSetupManager.executeAfterNavigation(page, newUrl);
await this.pendingSetupScripts;
this.pendingSetupScripts = null;
Comment on lines +335 to +337
Copy link

Choose a reason for hiding this comment

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

Suggestion: Using the shared pendingSetupScripts field directly for both storing and awaiting the promise in the framenavigated handler creates a race condition when multiple navigations happen quickly: later events overwrite the field before earlier awaits complete, so earlier handlers may end up awaiting the wrong promise and one or more setup runs can be skipped or left untracked. Capturing the promise in a local variable and only clearing the field if it still matches that promise also ensures the field is reset correctly even if the setup throws. [race condition]

Severity Level: Major ⚠️
- ❌ Page setup scripts tracking becomes unreliable.
- ❌ Auto-login or cookie dismissal may be skipped.
- ⚠️ capturePageState may capture preconditioned pages incorrectly.
Suggested change
this.pendingSetupScripts = this.pageSetupManager.executeAfterNavigation(page, newUrl);
await this.pendingSetupScripts;
this.pendingSetupScripts = null;
const setupPromise = this.pageSetupManager.executeAfterNavigation(page, newUrl);
this.pendingSetupScripts = setupPromise;
try {
await setupPromise;
} finally {
if (this.pendingSetupScripts === setupPromise) {
this.pendingSetupScripts = null;
}
}
Steps of Reproduction ✅
1. Start the explorer by calling Explorer.start() so listeners are installed (see
src/explorer.ts around the start() method where it calls await
this.pageSetupManager.loadSetupScripts(); and this.listenToStateChanged()). This
initializes Playwright and registers the framenavigated handler in listenToStateChanged.

2. Trigger two navigations quickly (e.g. call explorer.visit(urlA) then immediately
explorer.visit(urlB) from the same process). visit() performs navigation (src/explorer.ts
visit() function, the I.amOnPage call) which causes Playwright to emit 'framenavigated'.

3. Each framenavigated event enters the handler defined in listenToStateChanged
(src/explorer.ts at the framenavigated listener, lines shown at 328-337). The handler runs
the snippet that assigns this.pendingSetupScripts =
this.pageSetupManager.executeAfterNavigation(...); then awaits it (lines 335-337).

4. Because the code stores the promise in the shared field and an immediately subsequent
framenavigated overwrites it, the first handler's await may end up awaiting the later
promise and the finally clearing (this.pendingSetupScripts = null) may clear the field
incorrectly. Result: one setup run is left untracked or awaited incorrectly, causing setup
scripts (login/cookie dismissal) to be skipped or race between runs.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** src/explorer.ts
**Line:** 335:337
**Comment:**
	*Race Condition: Using the shared `pendingSetupScripts` field directly for both storing and awaiting the promise in the `framenavigated` handler creates a race condition when multiple navigations happen quickly: later events overwrite the field before earlier awaits complete, so earlier handlers may end up awaiting the wrong promise and one or more setup runs can be skipped or left untracked. Capturing the promise in a local variable and only clearing the field if it still matches that promise also ensures the field is reset correctly even if the setup throws.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

});

debugLog('Listening for automatic state changes');
Expand Down
100 changes: 100 additions & 0 deletions src/page-setup-manager.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import { existsSync } from 'node:fs';
import { resolve } from 'node:path';
import type { ExplorbotConfig } from './config.js';
import type Explorer from './explorer.js';
import type { KnowledgeTracker } from './knowledge-tracker.js';
import type { StateManager } from './state-manager.js';
import { createDebug, tag } from './utils/logger.js';

const debugLog = createDebug('explorbot:page-setup');

export interface SetupScriptContext {
page: any;
context: any;
explorer: Explorer;
stateManager: StateManager;
knowledgeTracker: KnowledgeTracker;
config: ExplorbotConfig;
log: (...args: any[]) => void;
}

interface SetupScriptModule {
setup: (context: SetupScriptContext) => Promise<void>;
}

export class PageSetupManager {
private scripts: SetupScriptModule[] = [];
private explorer: Explorer;
private config: ExplorbotConfig;
private isLoaded = false;

constructor(explorer: Explorer, config: ExplorbotConfig) {
this.explorer = explorer;
this.config = config;
}

async loadSetupScripts(): Promise<void> {
if (this.isLoaded) return;

const scriptPaths = this.config.playwright?.setupScripts || [];
if (scriptPaths.length === 0) {
debugLog('No setup scripts configured');
this.isLoaded = true;
return;
}

for (const scriptPath of scriptPaths) {
try {
const fullPath = resolve(scriptPath);

if (!existsSync(fullPath)) {
tag('warning').log(`Setup script not found: ${scriptPath}`);
continue;
}

const module = await import(fullPath);

if (typeof module.setup !== 'function') {
tag('warning').log(`Setup script ${scriptPath} does not export a setup() function`);
continue;
}

this.scripts.push(module);
debugLog(`Loaded setup script: ${scriptPath}`);
tag('substep').log(`Loaded setup script: ${scriptPath}`);
} catch (error) {
tag('error').log(`Failed to load setup script ${scriptPath}: ${error}`);
}
}

this.isLoaded = true;
}

async executeAfterNavigation(page: any, url: string): Promise<void> {
if (this.scripts.length === 0) return;
Copy link

Choose a reason for hiding this comment

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

Suggestion: The executeAfterNavigation method assumes that loadSetupScripts has already been called and completed, so if callers forget to load scripts or don't await the loader, this.scripts remains empty and setup scripts silently never run after navigation. Because loadSetupScripts is asynchronous and guarded by isLoaded, it is safe and more robust to call it from executeAfterNavigation itself so scripts are always loaded before execution. [possible bug]

Severity Level: Major ⚠️
- ❌ Post-navigation setup scripts silently never execute.
- ⚠️ Cookie banners remain and block interactions.
- ⚠️ Auto-login/setup flows don't run before capture.
- ⚠️ Explorer framenavigated handler continues without preconditions.
Suggested change
if (this.scripts.length === 0) return;
await this.loadSetupScripts();
Steps of Reproduction ✅
1. Configure Playwright setup scripts in runtime config (Explorbot config passed into
Explorer). The PageSetupManager expects script paths to come from
config.playwright.setupScripts (see src/page-setup-manager.ts: lines defining
loadSetupScripts at lines 36-71 in the PR hunk).

2. Start the Explorer and navigate pages so the Playwright page emits frame navigation. In
src/explorer.ts (file excerpt in Additional Downstream Code Context, within the page
'framenavigated' handler) the code calls:

   this.pendingSetupScripts = this.pageSetupManager.executeAfterNavigation(page, newUrl);

   await this.pendingSetupScripts;

   (this is the only invocation path that runs setup scripts after navigation).

3. Execution enters PageSetupManager.executeAfterNavigation at src/page-setup-manager.ts
lines 73-87. That function immediately returns when this.scripts.length === 0 (line 74),
but loadSetupScripts() is never invoked here and there is no other guaranteed call site in
the shown codebase that loads scripts prior to first navigation.

4. Observe behavior: despite having configured setup script paths, setup code never runs
after navigation (no cookie dismissal, no auto-login). The check at
src/page-setup-manager.ts line 74 causes silent early return because this.scripts is still
empty (loadSetupScripts was not awaited or invoked). This reproduces reliably whenever
setupScripts are configured but loadSetupScripts hasn't been called before the first frame
navigation.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** src/page-setup-manager.ts
**Line:** 74:74
**Comment:**
	*Possible Bug: The `executeAfterNavigation` method assumes that `loadSetupScripts` has already been called and completed, so if callers forget to load scripts or don't await the loader, `this.scripts` remains empty and setup scripts silently never run after navigation. Because `loadSetupScripts` is asynchronous and guarded by `isLoaded`, it is safe and more robust to call it from `executeAfterNavigation` itself so scripts are always loaded before execution.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.


debugLog(`Executing ${this.scripts.length} setup scripts after navigation to ${url}`);

const context = this.createContext(page);

for (const script of this.scripts) {
try {
await script.setup(context);
} catch (error) {
debugLog(`Setup script error: ${error}`);
}
}
}

private createContext(page: any): SetupScriptContext {
return {
page,
context: page.context(),
explorer: this.explorer,
stateManager: this.explorer.getStateManager(),
knowledgeTracker: this.explorer.getKnowledgeTracker(),
config: this.config,
log: (...args: any[]) => tag('setup').log(...args),
};
}
}
Loading