-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: act, extract, and observe not respecting timeout param
#1330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: act, extract, and observe not respecting timeout param
#1330
Conversation
🦋 Changeset detectedLatest commit: d6bbfb8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Greptile OverviewGreptile SummaryReplaces Promise.race-based timeouts with step-wise timeout guards across Key improvements:
The refactor eliminates the race condition where Promise.race would allow background operations to continue running after timeout, which could cause unexpected state changes or resource consumption. Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant ActHandler
participant TimeoutGuard
participant Snapshot
participant LLM
participant Action
User->>ActHandler: act(instruction, timeout=5000ms)
ActHandler->>TimeoutGuard: createTimeoutGuard(5000ms)
TimeoutGuard-->>ActHandler: ensureTimeRemaining()
ActHandler->>TimeoutGuard: ensureTimeRemaining() [check 1]
Note over TimeoutGuard: elapsed < 5000ms ✓
ActHandler->>ActHandler: waitForDomNetworkQuiet()
ActHandler->>TimeoutGuard: ensureTimeRemaining() [check 2]
Note over TimeoutGuard: elapsed < 5000ms ✓
ActHandler->>Snapshot: captureHybridSnapshot()
Snapshot-->>ActHandler: combinedTree, xpathMap
ActHandler->>TimeoutGuard: ensureTimeRemaining() [check 3]
Note over TimeoutGuard: elapsed < 5000ms ✓
ActHandler->>LLM: getActionFromLLM()
LLM-->>ActHandler: action
ActHandler->>TimeoutGuard: ensureTimeRemaining() [check 4]
Note over TimeoutGuard: elapsed >= 5000ms ✗
TimeoutGuard-->>ActHandler: throw ActTimeoutError
ActHandler-->>User: ActTimeoutError("act() timed out after 5000ms")
Note over User,Action: Alternative: Two-Step Flow
ActHandler->>TimeoutGuard: ensureTimeRemaining() [before step 2 snapshot]
ActHandler->>Snapshot: captureHybridSnapshot() [step 2]
ActHandler->>TimeoutGuard: ensureTimeRemaining() [before step 2 LLM]
ActHandler->>LLM: getActionFromLLM() [step 2]
Note over User,Action: Alternative: Self-Heal Flow
ActHandler->>Action: performUnderstudyMethod()
Action-->>ActHandler: Error
ActHandler->>TimeoutGuard: ensureTimeRemaining() [before retry snapshot]
ActHandler->>Snapshot: captureHybridSnapshot() [retry]
ActHandler->>TimeoutGuard: ensureTimeRemaining() [before retry LLM]
ActHandler->>LLM: getActionFromLLM() [retry]
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6 files reviewed, no comments
| v3Logger({ | ||
| category: "extraction", | ||
| message: completed | ||
| ? "Extraction completed successfully" | ||
| : "Extraction incomplete after processing all data", | ||
| level: 1, | ||
| auxiliary: { | ||
| prompt_tokens: { value: String(prompt_tokens), type: "string" }, | ||
| completion_tokens: { value: String(completion_tokens), type: "string" }, | ||
| inference_time_ms: { | ||
| value: String(inference_time_ms), | ||
| type: "string", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should move this to after injecting urls to prevent confusion on logs whenever users extract with urls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9 files reviewed, no comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 9 files
Prompt for AI agents (all 2 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/tests/timeout-handlers.test.ts">
<violation number="1" location="packages/core/tests/timeout-handlers.test.ts:181">
P2: `expect.fail()` is not available in Vitest's default `expect` API. If the expected error isn't thrown, this will fail with 'expect.fail is not a function' rather than a clear test failure message. Use `throw new Error()` or a failing assertion instead.</violation>
<violation number="2" location="packages/core/tests/timeout-handlers.test.ts:238">
P3: Typo in comment: 'performUndertudy' should be 'performUnderstudy'.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
8d8e569 to
08d8454
Compare
83dbace to
6d4c8e3
Compare
act, extract, and observe not respecting timeout param
# why - to clean up the actHandler before #1330 <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Refactors actHandler to centralize LLM action parsing and execution, reduce duplication, and improve metrics reporting. Behavior stays the same, with clearer naming and more reliable two-step and fallback flows. ## Why: - Reduce duplicated LLM calls and normalization logic. - Improve readability and maintainability. - Ensure consistent metrics and variable substitution. - Make the self-heal/fallback path more robust. ## What: - Renamed actFromObserveResult to takeDeterministicAction and updated all call sites (ActCache, AgentCache, v3). - Added getActionFromLLM for inference, metrics, normalization, and variable substitution. - Added recordActMetrics to centralize ACT metrics reporting. - Extracted normalizeActInferenceElement and substituteVariablesInArguments helpers. - Simplified two-step act flow and fallback retry using shared helpers. - Kept existing behavior (selector normalization, variable substitution, retries). ## Test Plan: - [ ] Run unit tests for actHandler to confirm no regressions. - [ ] Verify single-step actions execute as before. - [ ] Verify two-step flow triggers when LLM returns twoStep and executes the second action. - [ ] Confirm fallback self-heal path updates selector and retries successfully. - [ ] Check metrics are recorded once per inference call in both steps and fallback. - [ ] Validate variable substitution replaces %key% tokens in action arguments. - [ ] Exercise AgentCache and ActCache paths to ensure takeDeterministicAction works end-to-end. - [ ] Build passes and type checks for all renamed method references. <sup>Written for commit 08d8454. Summary will update automatically on new commits.</sup> <!-- End of auto-generated description by cubic. -->
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
c2541f3 to
d6bbfb8
Compare
why
what changed
test plan
Summary by cubic
Fixes act, extract, and observe to truly honor the timeout parameter with step-wise guards that abort early and return clear errors. Deterministic actions now use the same guard path in v3.
Written for commit d6bbfb8. Summary will update automatically on new commits.