Add agent hooks and knowledge code execution#9
Merged
Conversation
- Add beforeHook/afterHook support for Navigator, Researcher, Tester, Captain agents - Hooks can execute Playwright or CodeceptJS code before/after agent runs - Support URL pattern matching for conditional hook execution - Add HooksRunner utility class for hook execution - Add `code` property to knowledge files for CodeceptJS execution on page visit - Document hooks system, knowledge automation, and HTML filtering Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
CodeAnt AI is reviewing your PR. |
codeant-ai
bot
added
the
size:XL
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
src/ai/captain.ts
Outdated
Comment on lines
362
to
365
| const pageContext = await this.getPageContext(); | ||
| const planContext = this.planSummary(); | ||
|
|
||
| // Clean up old page context from previous inputs when continuing conversation | ||
| await this.getHooksRunner().runBeforeHook('captain', startUrl); |
There was a problem hiding this comment.
Suggestion: The before-hook for this agent runs after the initial page context and plan summary are captured, so if the hook mutates the page (e.g., dismisses a modal, waits for extra content, or navigates), the AI will reason on stale page information; move the hook call before collecting context so the prompt reflects the post-hook state, matching how other agents use hooks. [logic error]
Severity Level: Major ⚠️
- ⚠️ Captain prompt uses stale page snapshot.
- ⚠️ Reasoning based on pre-hook DOM state.
- ❌ Actions may fail after hook mutations.
- ⚠️ Affects agent decisions and tool usage.
Suggested change
| const pageContext = await this.getPageContext(); | |
| const planContext = this.planSummary(); | |
| // Clean up old page context from previous inputs when continuing conversation | |
| await this.getHooksRunner().runBeforeHook('captain', startUrl); | |
| await this.getHooksRunner().runBeforeHook('captain', startUrl); | |
| const pageContext = await this.getPageContext(); | |
| const planContext = this.planSummary(); |
Steps of Reproduction ✅
1. Configure a beforeHook for the captain agent in explorbot.config.js that mutates the
page (for example, dismisses a modal or waits for content). This is the new hook mechanism
added in the PR (hooks runner invoked at src/ai/captain.ts:365).
2. Trigger the Captain flow by calling Captain.handle(...) (src/ai/captain.ts:334) with a
valid page already loaded. The code path reaches the section starting at
src/ai/captain.ts:361 where tools and page context are prepared.
3. Observe ordering: at src/ai/captain.ts:361 the code builds tools (this.tools), then at
src/ai/captain.ts:362 it calls getPageContext() to snapshot the page, then at
src/ai/captain.ts:363 it calls planSummary(), and only afterwards at src/ai/captain.ts:365
it executes getHooksRunner().runBeforeHook('captain', startUrl).
4. Because the pageContext (src/ai/captain.ts:362) is captured before the beforeHook runs
(src/ai/captain.ts:365), any DOM/state changes the hook performs (e.g., navigation,
dismiss modal, wait for networkidle) are not reflected in the prompt sent to the provider.
Reproduce by logging or asserting differences: add a beforeHook that clicks a cookie
banner then checks for element presence; compare the page HTML/ARIA in pageContext (line
362 -> ActionResult.fromState) vs the actual page after the hook runs and observe
mismatch.
5. Expected behavior per other agents (and the feature intent) is to run beforeHook before
capturing context so the LLM reasons about the post-hook page. The current ordering
(pageContext then runBeforeHook) produces stale context. This is verifiable by reading the
code flow at src/ai/captain.ts lines 361-365.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ai/captain.ts
**Line:** 362:365
**Comment:**
*Logic Error: The before-hook for this agent runs after the initial page context and plan summary are captured, so if the hook mutates the page (e.g., dismisses a modal, waits for extra content, or navigates), the AI will reason on stale page information; move the hook call before collecting context so the prompt reflects the post-hook state, matching how other agents use hooks.
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.| const isOnCurrentState = this.actionResult!.getStateHash() === this.stateManager.getCurrentState()?.hash; | ||
| await this.ensureNavigated(state.url, screenshot && this.provider.hasVision()); | ||
| await this.hooksRunner.runBeforeHook('researcher', state.url); | ||
|
|
There was a problem hiding this comment.
Suggestion: After running the configurable before-hook, the stored page state (actionResult) is not refreshed, so the researcher builds its HTML/ARIA-based prompt from a stale snapshot that does not include any changes the hook may have made (like dismissing modals or navigating), leading to incorrect or misleading research results; recapturing the page state immediately after the hook fixes this. [logic error]
Severity Level: Major ⚠️
- ❌ Research reports use stale DOM snapshots.
- ⚠️ UI map generation may include removed modals.
- ⚠️ Knowledge/context integration may be misleading.
- ⚠️ Investigations relying on researcher agent produce incorrect results.
Suggested change
| this.actionResult = await this.explorer | |
| .createAction() | |
| .capturePageState({ includeScreenshot: screenshot && this.provider.hasVision() }); |
Steps of Reproduction ✅
1. Call Researcher.research() for a page state: open code path in src/ai/researcher.ts
where research() is executed. At research() the sequence around lines 111-116 is:
- 111: const isOnCurrentState = ...
- 112: await this.ensureNavigated(state.url, screenshot && this.provider.hasVision());
- 113: await this.hooksRunner.runBeforeHook('researcher', state.url);
- 115: debugLog('Researching web page:', this.actionResult!.url);
2. Configure a researcher beforeHook in explorbot.config.js that modifies the page DOM
(realistic example from PR description): e.g. a Playwright hook that clicks a cookie
banner or closes a modal on '/app/*'.
3. Trigger researcher.research(...) (e.g., via any code path that calls
Researcher.research with a WebPageState). The code runs ensureNavigated (line 112) then
runs the beforeHook (line 113). The hook executes and alters the page (dismiss modal,
navigate fragment, etc.).
4. After the hook returns, research() continues using this.actionResult (captured before
the hook at ensureNavigated). Because there is no re-capture, subsequent code builds
prompts and analyzes HTML/ARIA from the stale this.actionResult (see debugLog at line ~115
and prompt construction later), producing research that doesn't reflect the hook-made
changes.
5. Expected observable failure: research output (files under output/research or returned
text) still includes modal content, wrong UI map, or misses elements revealed by the hook.
Reproduced by configuring a beforeHook that modifies DOM and comparing research output
with and without adding the proposed re-capture after the hook.
Explanation: The repository diff shows the hook call was added at line 113 but no
re-capture follows; thus the suggestion to recapture the page state after hooks is
actionable and directly addresses the stale-snapshot problem.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ai/researcher.ts
**Line:** 114:114
**Comment:**
*Logic Error: After running the configurable before-hook, the stored page state (`actionResult`) is not refreshed, so the researcher builds its HTML/ARIA-based prompt from a stale snapshot that does not include any changes the hook may have made (like dismissing modals or navigating), leading to incorrect or misleading research results; recapturing the page state immediately after the hook fixes this.
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.
src/explorer.ts
Outdated
| const actionResult = currentState ? ActionResult.fromState(currentState) : null; | ||
|
|
||
| const { statePush = false, wait, waitForElement } = this.knowledgeTracker.getStateParameters(actionResult!, ['statePush', 'wait', 'waitForElement']); | ||
| const { statePush = false, wait, waitForElement, code } = this.knowledgeTracker.getStateParameters(actionResult!, ['statePush', 'wait', 'waitForElement', 'code']); |
There was a problem hiding this comment.
Suggestion: The call to retrieve knowledge state parameters uses a non-null assertion on a potentially null actionResult, so if visit is called before any state exists (e.g., the very first navigation), getStateParameters will receive null and state.isMatchedBy(...) will throw at runtime; instead, derive a safe fallback ActionResult (for example, from the target url) when no current state is available and pass that into getStateParameters without using a non-null assertion. [null pointer]
Severity Level: Critical 🚨
- ❌ First navigation may throw runtime exception.
- ⚠️ Knowledge matching for URL patterns fails at startup.
- ⚠️ Explorer's page automation becomes unreliable on cold start.
Suggested change
| const { statePush = false, wait, waitForElement, code } = this.knowledgeTracker.getStateParameters(actionResult!, ['statePush', 'wait', 'waitForElement', 'code']); | |
| const knowledgeState = actionResult ?? new ActionResult({ url }); | |
| const { statePush = false, wait, waitForElement, code } = this.knowledgeTracker.getStateParameters(knowledgeState, ['statePush', 'wait', 'waitForElement', 'code']); |
Steps of Reproduction ✅
1. Trigger the very first navigation using Explorer.visit(url) (visit implementation in
src/explorer.ts; relevant lines around 204-207). At startup there is no current state so
actionResult is null (lines ~204-206).
2. The existing line at src/explorer.ts:207 force-unwraps actionResult and passes it to
this.knowledgeTracker.getStateParameters(...). If knowledge lookups call methods like
state.isMatchedBy(...) on the ActionResult, they will throw because actionResult is null.
3. Reproduce locally: instantiate Explorer, call start(), then call visit('/some/path') as
the first navigation. Observe an exception thrown from getStateParameters or inside its
match logic, preventing visit from completing and blocking subsequent action.execute(...)
calls (I.amOnPage or history push).
4. The proposed change constructs a minimal ActionResult from the target url when
actionResult is missing, ensuring getStateParameters receives a valid ActionResult
instance and avoids null dereferences while still allowing URL-based knowledge matching.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/explorer.ts
**Line:** 207:207
**Comment:**
*Null Pointer: The call to retrieve knowledge state parameters uses a non-null assertion on a potentially null `actionResult`, so if `visit` is called before any state exists (e.g., the very first navigation), `getStateParameters` will receive `null` and `state.isMatchedBy(...)` will throw at runtime; instead, derive a safe fallback `ActionResult` (for example, from the target `url`) when no current state is available and pass that into `getStateParameters` without using a non-null assertion.
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.| import micromatch from 'micromatch'; | ||
| import type { ExplorbotConfig, Hook, HookConfig } from '../config.ts'; | ||
| import type Explorer from '../explorer.ts'; | ||
| import { createDebug } from './logger.ts'; |
There was a problem hiding this comment.
Suggestion: The logger import uses a .ts extension, but all runtime imports in this codebase target the compiled .js files; this will cause a runtime module resolution error (Cannot find module './logger.ts') once the code is transpiled and executed under Node. [possible bug]
Severity Level: Critical 🚨
- ❌ Application startup fails due to module load error.
- ⚠️ Hooks subsystem unavailable until fixed.
- ⚠️ CI/runtime tests may break on Node environment.
Suggested change
| import { createDebug } from './logger.ts'; | |
| import { createDebug } from './logger.js'; |
Steps of Reproduction ✅
1. Inspect src/utils/hooks-runner.ts import lines (Read output shows line 4: import {
createDebug } from './logger.ts';).
2. Build/compile project to JS (typical ts->js pipeline). At runtime Node will load
compiled files; most build setups expect runtime imports to target .js or be
extensionless. With the current import './logger.ts', Node's resolver (or bundler) can try
to load a non-existent './logger.ts' file from the compiled output and throw "Cannot find
module './logger.ts'".
3. Trigger any code path that constructs HooksRunner (e.g., explorer initialization that
imports HooksRunner). The import failure occurs immediately when requiring/initializing
src/utils/hooks-runner.ts at startup, causing the process to crash before hooks run.
4. Replacing the import with './logger.js' or using an extensionless import that build
tooling rewrites prevents the runtime resolution error.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/utils/hooks-runner.ts
**Line:** 4:4
**Comment:**
*Possible Bug: The logger import uses a `.ts` extension, but all runtime imports in this codebase target the compiled `.js` files; this will cause a runtime module resolution error (`Cannot find module './logger.ts'`) once the code is transpiled and executed under Node.
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.
src/utils/hooks-runner.ts
Outdated
| if (this.isSingleHook(config)) return config as Hook; | ||
|
|
||
| const urlPath = this.extractPath(url); | ||
| for (const [pattern, hook] of Object.entries(config)) { |
There was a problem hiding this comment.
Suggestion: When multiple hook patterns are configured in a map, iteration over Object.entries means a catch-all pattern like * can overshadow more specific patterns if it appears first, so a generic hook may run instead of a route-specific one, which is counter to typical routing semantics. [logic error]
Severity Level: Major ⚠️
- ⚠️ Route-specific hooks bypassed by generic patterns.
- ⚠️ Unexpected hook behavior in Navigator/Researcher agents.
- ⚠️ Tests relying on specific hooks may fail.
Suggested change
| for (const [pattern, hook] of Object.entries(config)) { | |
| const entries = Object.entries(config); | |
| // Ensure more specific patterns are evaluated before generic ones like '*' | |
| entries.sort(([patternA], [patternB]) => { | |
| if (patternA === '*' && patternB !== '*') return 1; | |
| if (patternB === '*' && patternA !== '*') return -1; | |
| const aWildcard = patternA.endsWith('/*'); | |
| const bWildcard = patternB.endsWith('/*'); | |
| if (aWildcard !== bWildcard) return aWildcard ? 1 : -1; | |
| return 0; | |
| }); | |
| for (const [pattern, hook] of entries) { |
Steps of Reproduction ✅
1. Read src/utils/hooks-runner.ts to find findMatchingHook implementation at lines 36-44
(verified via Read/Grep). It iterates Object.entries(config) in insertion order and
returns the first matching pattern.
2. Create an agent config where a generic pattern '*' (or broad glob) is inserted before a
more specific pattern in the same object, for example in explorbot.config.js:
beforeHook: { '*': { ... }, '/admin/*': { ... } }
(Objects in JS preserve insertion order; if '*' is defined first, it will be checked
first.)
3. Call HooksRunner.runBeforeHook for a path like '/admin/users'. findMatchingHook will
iterate entries and match the '*' pattern first (lines 39-42), returning the generic hook
and never reaching the specific '/admin/*' rule.
4. Observe that a generic hook runs instead of the intended route-specific hook. The
improved code sorts entries so specific patterns take precedence, preventing this
shadowing.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/utils/hooks-runner.ts
**Line:** 40:40
**Comment:**
*Logic Error: When multiple hook patterns are configured in a map, iteration over `Object.entries` means a catch-all pattern like `*` can overshadow more specific patterns if it appears first, so a generic hook may run instead of a route-specific one, which is counter to typical routing semantics.
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.
Comment on lines
+60
to
+61
| private isSingleHook(config: HookConfig): boolean { | ||
| return 'type' in config && 'hook' in config; |
There was a problem hiding this comment.
Suggestion: The isSingleHook check currently only tests for the presence of type and hook keys, so a pattern map that happens to use "type" or "hook" as URL patterns will be misclassified as a single hook, causing all other hooks in the map to be ignored and the URL pattern matching logic to never run for that agent; tightening the check to verify the value's actual shape (hook is a function and type is 'playwright' or 'codeceptjs') avoids this misclassification. [logic error]
Severity Level: Major ⚠️
- ❌ URL-specific hooks ignored for misclassified configs.
- ⚠️ Researcher/Navigator agent hooks misbehave unpredictably.
- ⚠️ Hook debugging time increases for maintainers.
Suggested change
| private isSingleHook(config: HookConfig): boolean { | |
| return 'type' in config && 'hook' in config; | |
| private isSingleHook(config: HookConfig): config is Hook { | |
| const candidate = config as any; | |
| return ( | |
| candidate && | |
| typeof candidate === 'object' && | |
| typeof candidate.hook === 'function' && | |
| (candidate.type === 'playwright' || candidate.type === 'codeceptjs') | |
| ); |
Steps of Reproduction ✅
1. Open file src/utils/hooks-runner.ts and locate the isSingleHook implementation at lines
60-62 (verified via Read output). The function returns true when the object has keys
'type' and 'hook' regardless of their values.
2. Configure an agent in explorbot.config.js with an agent-specific hook map that uses a
URL string key literally named "type" or "hook", for example:
agents: { researcher: { beforeHook: { type: { type: 'playwright', hook: async () => {}
} } } }
(This configuration structure matches the shape of a map where a pattern key is the
literal 'type'.)
3. Run code path that triggers hook resolution: HooksRunner.runBeforeHook('researcher',
'/some/url') calls runHook -> findMatchingHook (see src/utils/hooks-runner.ts lines 22-34
and 36-44).
4. findMatchingHook calls isSingleHook(config) (line 36). Because the top-level object has
keys 'type' and 'hook' (even though it was intended as a map), the current isSingleHook
returns true and the entire config is treated as a single Hook object, bypassing pattern
iteration. As a result, pattern-specific hooks in the map are ignored and the intended
URL-based hook matching never runs (observed at lines 36-43).
5. Expected: only true single Hook values (with callable hook and valid type) should be
classified as a single hook; current loose check causes misclassification. The improved
check (type guard verifying hook is function and type is valid) prevents this
misclassification.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/utils/hooks-runner.ts
**Line:** 60:61
**Comment:**
*Logic Error: The `isSingleHook` check currently only tests for the presence of `type` and `hook` keys, so a pattern map that happens to use `"type"` or `"hook"` as URL patterns will be misclassified as a single hook, causing all other hooks in the map to be ignored and the URL pattern matching logic to never run for that agent; tightening the check to verify the value's actual shape (hook is a function and type is `'playwright'` or `'codeceptjs'`) avoids this misclassification.
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.
|
CodeAnt AI finished reviewing your PR. |
- Import tryTo, retryTo, within, hopeThat from codeceptjs/lib/effects.js - Pass effects to code execution in action.execute() and action.expect() - Document effects usage in knowledge.md Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
CodeAnt AI is running Incremental review |
codeant-ai
bot
added
size:XL
src/action.ts
Outdated
Comment on lines
192
to
193
| const codeFunction = new Function('I', 'tryTo', 'retryTo', 'within', 'hopeThat', codeString); | ||
| codeFunction(this.actor, tryTo, retryTo, within, hopeThat); |
There was a problem hiding this comment.
Suggestion: In execute, the dynamically created function is a regular (non-async) function that is immediately invoked without await, so any code block using await tryTo(...), await retryTo(...), or other async effects will either fail with a syntax error (await only valid in async functions) or not be properly awaited before the recorder delay and page-state capture, breaking the new effects-based knowledge code. [logic error]
Severity Level: Critical 🚨
- ❌ Knowledge code with await tryTo/retryTo throws SyntaxError on execute.
- ❌ Agent actions using documented async effects cannot run successfully.
- ⚠️ Async snippets returning promises are never awaited, risking races.
- ⚠️ Page state capture at src/action.ts:198 may occur before effects complete.
Suggested change
| const codeFunction = new Function('I', 'tryTo', 'retryTo', 'within', 'hopeThat', codeString); | |
| codeFunction(this.actor, tryTo, retryTo, within, hopeThat); | |
| const codeFunction = new Function( | |
| 'I', | |
| 'tryTo', | |
| 'retryTo', | |
| 'within', | |
| 'hopeThat', | |
| `return (async () => { ${codeString} })();` | |
| ); | |
| await codeFunction(this.actor, tryTo, retryTo, within, hopeThat); |
Steps of Reproduction ✅
1. In the repository documentation at `docs/knowledge.md:139-140`, knowledge code is shown
using async effects, e.g.:
`await tryTo(() => I.click('.cookie-dismiss'));` and `await retryTo(() => { ... }, 5,
500);`.
2. In runtime or a test, construct an `Action` instance from `src/action.ts:28-47` and
call its `execute` method at `src/action.ts:176-221` with a code string that matches the
documented pattern, for example:
`await action.execute("await tryTo(() => I.click('.cookie-dismiss'));");`.
3. Inside `execute`, at `src/action.ts:192`, the code builds a dynamic function via
`new Function('I', 'tryTo', 'retryTo', 'within', 'hopeThat', codeString)` using the
provided `codeString` as the function body.
4. Because the generated function is a regular (non-async) function and `codeString`
contains a top-level `await`, the JavaScript engine throws a `SyntaxError` ("await is only
valid in async functions or at the top level") when evaluating the `new Function` call at
line 192, so the action fails before `recorder.add`/`recorder.promise` and
`capturePageState` at lines 195-198 can run; async effects like `tryTo`/`retryTo` never
execute.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/action.ts
**Line:** 192:193
**Comment:**
*Logic Error: In `execute`, the dynamically created function is a regular (non-async) function that is immediately invoked without `await`, so any code block using `await tryTo(...)`, `await retryTo(...)`, or other async effects will either fail with a syntax error (`await` only valid in async functions) or not be properly awaited before the recorder delay and page-state capture, breaking the new effects-based knowledge code.
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.
src/action.ts
Outdated
Comment on lines
235
to
237
| codeFunction = new Function('I', 'tryTo', 'retryTo', 'within', 'hopeThat', codeString); | ||
| } | ||
| codeFunction(this.actor); | ||
| codeFunction(this.actor, tryTo, retryTo, within, hopeThat); |
There was a problem hiding this comment.
Suggestion: In expect, string-based expectations are compiled into a non-async function and invoked without await, so expectation code that uses await tryTo(...), await retryTo(...), or other async effects will either be syntactically invalid or not be awaited before recorder.promise(), meaning assertions and retries may not complete in time and can behave unpredictably. [logic error]
Severity Level: Major ⚠️
- ❌ String-based expectations with await tryTo/retryTo crash on parse.
- ⚠️ Agent expectation checks cannot reliably use documented effects.
- ⚠️ Async expectation snippets returning promises are never awaited.
- ⚠️ Recorder at src/action.ts:238 may resolve before async assertions finish.
Suggested change
| codeFunction = new Function('I', 'tryTo', 'retryTo', 'within', 'hopeThat', codeString); | |
| } | |
| codeFunction(this.actor); | |
| codeFunction(this.actor, tryTo, retryTo, within, hopeThat); | |
| codeFunction = new Function( | |
| 'I', | |
| 'tryTo', | |
| 'retryTo', | |
| 'within', | |
| 'hopeThat', | |
| `return (async () => { ${codeString} })();` | |
| ); | |
| } | |
| await codeFunction(this.actor, tryTo, retryTo, within, hopeThat); |
Steps of Reproduction ✅
1. The documentation at `docs/knowledge.md:139-140` demonstrates async effects usage like
`await tryTo(() => I.click('.cookie-dismiss'));`, which is a realistic pattern to reuse
in expectation snippets.
2. In code that uses the `Action` class from `src/action.ts:28-47`, call the `expect`
method defined at `src/action.ts:224-265` with an async string snippet, for example:
`await action.expect("await tryTo(() => I.click('.cookie-dismiss'));");`.
3. Inside `expect`, the string branch at `src/action.ts:231-236` executes, constructing a
dynamic function with
`new Function('I', 'tryTo', 'retryTo', 'within', 'hopeThat', codeString)` where
`codeString` contains the top-level `await`.
4. Because this dynamically created function is not declared as `async`, the top-level
`await` in `codeString` is invalid, so the engine throws a `SyntaxError` before
`codeFunction(this.actor, tryTo, retryTo, within, hopeThat);` at line 237 and before
`await recorder.promise();` at line 238, preventing async expectations and retries from
running.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/action.ts
**Line:** 235:237
**Comment:**
*Logic Error: In `expect`, string-based expectations are compiled into a non-async function and invoked without `await`, so expectation code that uses `await tryTo(...)`, `await retryTo(...)`, or other async effects will either be syntactically invalid or not be awaited before `recorder.promise()`, meaning assertions and retries may not complete in time and can behave unpredictably.
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.| const serializedUrl = JSON.stringify(url); | ||
| const currentState = this.stateManager.getCurrentState(); | ||
| const actionResult = currentState ? ActionResult.fromState(currentState) : null; | ||
| const actionResult = currentState ? ActionResult.fromState(currentState) : new ActionResult({ url }); |
There was a problem hiding this comment.
Suggestion: Knowledge parameters for navigation (statePush, wait, waitForElement, code) are derived from the current page state instead of the target URL, so after the first navigation the knowledge rules for the destination page will not be applied and code like waits or hooks defined for that URL will silently never run; always constructing the lookup state from the target URL fixes this. [logic error]
Severity Level: Major ⚠️
- ⚠️ Explorer.visit ignores knowledge for destination URL during navigation.
- ⚠️ Knowledge `wait` and `waitForElement` often never applied.
- ⚠️ Knowledge `code` blocks not executed for target pages.
Suggested change
| const actionResult = currentState ? ActionResult.fromState(currentState) : new ActionResult({ url }); | |
| const actionResult = new ActionResult({ url }); |
Steps of Reproduction ✅
1. Note that Explorer navigation uses knowledge parameters in `src/explorer.ts:200-215`
within `async visit(url: string)`. It creates `currentState` from
`this.stateManager.getCurrentState()` at line 204 and then builds `actionResult` from that
state at line 205: `const actionResult = currentState ?
ActionResult.fromState(currentState) : new ActionResult({ url });`.
2. Observe how knowledge is selected in `src/knowledge-tracker.ts:200-213`.
`getStateParameters(state, keys)` calls `getRelevantKnowledge(state)` (line 201), which
filters `this.knowledgeFiles` with `state.isMatchedBy(knowledge)` (lines 72-77) and logs
matches using `state.url` (line 80). This means the URL used for matching comes from the
passed `ActionResult`.
3. Configure a knowledge file for a destination URL only. For example, using the same
setup pattern as `tests/unit/explorer.test.ts:207-238` (which constructs an `Explorer`
with a base URL), create a markdown file in the knowledge directory determined in
`KnowledgeTracker`'s constructor (`src/knowledge-tracker.ts:22-37`) with frontmatter:
`url: mock://explorer.test/second` and `wait: 5`. Do not create any knowledge for
`mock://explorer.test/first`.
4. In a test similar to `tests/unit/explorer.test.ts:252-258`, first put the explorer into
a known "current" state for `/first` (e.g., call
`stateManager.updateStateFromBasic('mock://explorer.test/first', 'First', 'manual')` as in
`beforeEach` at lines 241-244), then call `await
explorer.visit('mock://explorer.test/second')`. At this moment, `currentState` (line 204
in `src/explorer.ts`) still describes `/first`, so `actionResult` is built from the
`/first` state. `KnowledgeTracker.getStateParameters` (line 200 in
`src/knowledge-tracker.ts`) therefore matches knowledge against `/first`, not `/second`,
finds no `wait`/`code` for `/second`, and the subsequent blocks in `Explorer.visit` (lines
217-227) never execute the configured `wait` or `code` for `/second`. The destination-page
knowledge silently never runs, demonstrating the bug.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/explorer.ts
**Line:** 205:205
**Comment:**
*Logic Error: Knowledge parameters for navigation (statePush, wait, waitForElement, code) are derived from the current page state instead of the target URL, so after the first navigation the knowledge rules for the destination page will not be applied and code like waits or hooks defined for that URL will silently never run; always constructing the lookup state from the target URL fixes this.
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.|
|
||
| if (pattern.startsWith('^')) { | ||
| try { | ||
| return new RegExp(pattern.slice(1)).test(path); |
There was a problem hiding this comment.
Suggestion: Regex URL patterns are handled incorrectly because the leading caret (^) is stripped before building the RegExp, which changes the intended anchoring semantics (e.g., a pattern like ^/admin$ will also match /foo/admin), causing hooks to run on URLs they shouldn't match. [logic error]
Severity Level: Major ⚠️
- ⚠️ Navigator beforeHook may run on unintended nested URLs.
- ⚠️ Researcher hooks can execute on more pages than configured.
- ⚠️ Tester and captain hooks may misfire, affecting automation flow.
- ⚠️ Regex hook semantics differ from standard anchored regex expectations.
Suggested change
| return new RegExp(pattern.slice(1)).test(path); | |
| return new RegExp(pattern).test(path); |
Steps of Reproduction ✅
1. Configure a regex-based hook pattern in the user config file loaded by `ConfigParser`
(see `src/config.ts:161-213` and `findConfigFile` at `src/config.ts:293-304`). For
example, in `explorbot.config.js`, set `ai.agents.navigator.beforeHook` to an object like
`{ '^/admin$': { type: 'playwright', hook: async ({ page }) => { /* admin-only setup */ }
} }`.
2. Start Explorbot so that `ConfigParser` loads this config and an `Explorer` is created
with it (Explorer stores the config in its `config` field, see `src/explorer.ts:45-53`).
Then create a `Navigator` instance, which constructs a `HooksRunner` with `new
HooksRunner(explorer, explorer.getConfig())` (see `src/ai/navigator.ts:54-63`).
3. Call `Navigator.visit('/foo/admin')` (method defined at `src/ai/navigator.ts:65-91`).
Inside this method, after navigating, it calls
`this.hooksRunner.runBeforeHook('navigator', url)` with `url` equal to `'/foo/admin'`
(lines `69-71`).
4. In `HooksRunner.runBeforeHook` (`src/utils/hooks-runner.ts:14-20`), `runHook` is
invoked, which fetches `agentConfig.beforeHook` (line `26`), then calls `findMatchingHook`
(line `29`). `findMatchingHook` delegates to `matchesPattern`
(`src/utils/hooks-runner.ts:80-98`), where the pattern `'^/admin$'` enters the
`pattern.startsWith('^')` branch at lines `89-95`. The current implementation constructs
`new RegExp(pattern.slice(1))`, i.e. `/admin$/`, which returns `true` for the path
`'/foo/admin'` because it only checks the end of the string. As a result, the admin-only
hook configured for `'^/admin$'` is executed for `'/foo/admin'` as well, even though
`'^/admin$'` as a regex should only match the exact path `'/admin'`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/utils/hooks-runner.ts
**Line:** 91:91
**Comment:**
*Logic Error: Regex URL patterns are handled incorrectly because the leading caret (`^`) is stripped before building the `RegExp`, which changes the intended anchoring semantics (e.g., a pattern like `^/admin$` will also match `/foo/admin`), causing hooks to run on URLs they shouldn't match.
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.
|
CodeAnt AI Incremental review completed. |
- Combined effects support (tryTo, retryTo, within, hopeThat) with code sanitization - Kept hooks runner in navigator while removing redundant URL expectation - Added isErrorPage import to researcher Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Summary
beforeHookandafterHooksupport for Navigator, Researcher, Tester, and Captain agentscodeproperty to knowledge files for executing CodeceptJS code on page navigationtryToandretryToeffects support in knowledge code for error handling and retriesFeatures
Agent Hooks
Configure hooks per-agent in
explorbot.config.js:Knowledge Code Execution
Execute CodeceptJS code when navigating to matching pages:
CodeceptJS Effects
Use
tryToandretryTofor robust page automation:tryTo(fn)true/falseretryTo(fn, maxTries, interval)within(context, fn)Files Changed
src/config.ts- Hook type definitionssrc/utils/hooks-runner.ts- New utility for hook executionsrc/action.ts- CodeceptJS effects supportsrc/ai/navigator.ts- Hook integrationsrc/ai/researcher.ts- Hook integrationsrc/ai/tester.ts- Hook integrationsrc/ai/captain.ts- Hook integrationsrc/explorer.ts- Knowledgecodeexecutiondocs/hooks.md- New documentationdocs/knowledge.md- Page automation and effects docsdocs/page-interaction.md- HTML filtering docsdocs/configuration.md- Hook options referenceTest plan
codeproperty, verify execution on page visittryToeffect - verify it catches errors and returns falseretryToeffect - verify it retries failed actions{ page, url }context{ I, url }context🤖 Generated with Claude Code
CodeAnt-AI Description
Run per-agent before/after hooks and execute page-level CodeceptJS code on navigation
What Changed
Impact
✅ Fewer flaky navigations due to agent-specific preconditions✅ Shorter test setup by automating page preparation (e.g., dismiss banners)✅ More robust page automation with retryable knowledge scripts💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.