feat(act): support structured same-tab action steps#1098
Conversation
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Add a structured steps input path to act so common same-tab browser loops can skip natural-language parsing and cache lookup while reusing existing per-step executors and verification behavior. Constraint: Extended the existing act tool instead of adding a new top-level macro tool, matching the issue's non-overlap direction. Rejected: Parallel step execution | same-tab browser actions are order-dependent and unsafe to run concurrently. Confidence: high Scope-risk: moderate Directive: Keep structured macro actions inside the existing ParsedAction verbs unless a future issue adds explicit safety semantics. Tested: ./node_modules/.bin/jest tests/tools/act.test.ts --runInBand Tested: ./node_modules/.bin/tsc -p tsconfig.json --noEmit Not-tested: Live same-tab macro smoke; included as PR OpenChrome validation checklist.
shaun0927
force-pushed
the
feat/969-act-structured-steps
branch
from
b676a03 to
0d6aac5
Compare
![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: 0d6aac57fe
ℹ️ 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".
| if (!structured.actions && (!instruction || instruction.trim().length === 0)) { | ||
| return { content: [{ type: 'text', text: 'Error: instruction or steps is required' }], isError: true }; | ||
| } | ||
| if (missingVariables.length > 0) { |
There was a problem hiding this comment.
When steps are supplied, this handler intentionally executes the structured path and skips natural-language parsing, but it still validates %var% placeholders from instruction and can return Error: Missing variable(s) even though instruction is not used. This breaks mixed callers that send both fields (e.g., legacy instruction template plus concrete steps) and causes valid structured runs to fail before execution. Placeholder validation should be scoped to the selected execution source (or include placeholders from steps instead).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d6aac57fe
ℹ️ 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".
| if (!structured.actions && (!instruction || instruction.trim().length === 0)) { | ||
| return { content: [{ type: 'text', text: 'Error: instruction or steps is required' }], isError: true }; |
There was a problem hiding this comment.
When steps are present, the handler still evaluates %variable% placeholders from instruction and returns Missing variable(s) before executing, even though this code path explicitly prefers structured actions. This makes otherwise valid structured-step calls fail whenever callers also send a templated or placeholder-filled instruction field (common in mixed/transition clients). Gate variable-missing validation to the natural-language path (or validate placeholders from structured steps instead) so steps truly works as an alternative input.
Useful? React with 👍 / 👎.
The CORS allowlist suite was racing transport.start() against the http server actually binding to the loopback port — on ubuntu-18 / macos-22 the 100 ms fixed sleep lost the race intermittently and tests failed with `connect ECONNREFUSED 127.0.0.1:36014`. Replace the fixed sleep with a poll loop that opens a probe TCP socket to the allocated port and resolves the moment it accepts a connection. The poll budget is 5 s with 25 ms retries, mirroring the bear-token suite's setup, and surfaces a deterministic timeout message if the server truly fails to bind. Resolves the ubuntu-18 / macos-18 / macos-22 build-and-test failures on PR #1098.
There was a problem hiding this comment.
💡 Codex Review
Line 725 in ff99e69
Validate placeholders from structured steps
When callers use steps with %name% placeholders, missing variable detection never runs because it only scans instruction. In the structured path this allows unresolved placeholders to flow into execution (for example, typing the literal %password%), which silently produces incorrect actions instead of the existing explicit Missing variable(s) error behavior. This was introduced with structured-step support and affects any workflow that templates step fields.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14fce2ece0
ℹ️ 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".
| if (structured.actions) { | ||
| actions = structured.actions; | ||
| source = 'structured'; |
There was a problem hiding this comment.
When the structured path is selected here, %name% validation still only scans instruction, so placeholders embedded in steps[*].target/value/condition are never checked for missing variables. In a steps-only call like type %password%, this now proceeds and can type the literal placeholder instead of failing fast with Missing variable(s), which is a behavioral regression from the instruction flow and can silently break scripted macros.
Useful? React with 👍 / 👎.
| const TEST_PORT_START = 20_000 + (process.pid % 400) * 100; | ||
| let nextTestPort = TEST_PORT_START; | ||
| let activePort = TEST_PORT_START; | ||
|
|
||
| function reserveLoopbackPort(): Promise<number> { | ||
| return new Promise((resolve, reject) => { | ||
| const server = net.createServer(); | ||
| server.unref(); | ||
| server.on('error', reject); | ||
| server.listen(0, '127.0.0.1', () => { | ||
| const address = server.address(); | ||
| const port = typeof address === 'object' && address ? address.port : 0; | ||
| server.close((err) => { | ||
| if (err) { | ||
| reject(err); | ||
| return; | ||
| } | ||
| if (!port) { | ||
| reject(new Error('Failed to reserve loopback test port')); | ||
| return; | ||
| } | ||
| activePort = port; | ||
| resolve(port); | ||
| }); | ||
| }); | ||
| }); | ||
| function allocatePort(): number { | ||
| activePort = nextTestPort++; | ||
| return activePort; |
There was a problem hiding this comment.
This change replaces OS-assigned ephemeral ports with a fixed arithmetic sequence and never probes availability before use. If any process on the runner already occupies one of these ports, transport.start() can fail with EADDRINUSE before assertions run, introducing host-dependent flakiness that the previous listen(0) reservation avoided.
Useful? React with 👍 / 👎.
…tter The 'ref counting: two beginHeavyOperation, one endHeavyOperation — still in heavy mode' test sets heavyOpFatalThresholdMs=200 and then busy-waits ~100ms. On a slow ubuntu-18 / macos-18 hosted runner the combined latency (busy-wait + jest worker scheduler jitter) drifted to 218 ms and tripped the fatal handler, breaking the assertion. Raise the heavyOp threshold to 2000 ms so the test still exercises the "still in heavy mode" ref-count gate but has a comfortable margin over CI host jitter. Resolves the latest #1098 build-and-test failures.
1 participant
Progress / Review status
Auto-refreshed 2026-05-13 — owner comments cleaned up to reduce review noise.
feat/969-act-structured-steps→develop0d6aac5— Let act run structured same-tab action stepsOwner comment cleanup: 1 issue + 0 inline review comments deleted. Outstanding feedback from automated/external reviewers above is unchanged.
Summary
acttool with a structuredstepsinput for same-tab macro execution.instructionsupport unchanged; callers may now provide eitherinstructionorsteps.Closes #969
Fit / non-overlap review
actAPI and the issue refinement.Tests
./node_modules/.bin/jest tests/tools/act.test.ts --runInBand./node_modules/.bin/tsc -p tsconfig.json --noEmitOpenChrome real-validation checklist
After merge, validate with OpenChrome itself:
actwith structured steps, for example{ "tabId": "...", "steps": [{ "action": "click", "target": "Login" }], "verify": false }.[structured], executes the expected step, and does not requireinstruction.actwith onlyinstruction; confirm the existing natural-language path still works.