Repository navigation
Document the agent skill and the script_path guarantee - #153
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="README.md">
<violation number="1" location="README.md:172">
P2: `agent --json` can still return `status: "ok"` with `script_path: null`, so this documents an invariant the CLI does not enforce. Make the null-path case an `engine_failure` before documenting this guarantee, or soften the table entry.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| | `mode` | `string` \| `null` | `"auto"`, `"chrome-mcp"`, or `"agent-browser"`. | | ||
| | `har_path` | `string` \| `null` | Captured HAR. | | ||
| | `script_path` | `string` \| `null` | Generated client. | | ||
| | `script_path` | `string` \| `null` | Generated client; exists whenever `status` is `"ok"`. | |
There was a problem hiding this comment.
P2: agent --json can still return status: "ok" with script_path: null, so this documents an invariant the CLI does not enforce. Make the null-path case an engine_failure before documenting this guarantee, or soften the table entry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At README.md, line 172:
<comment>`agent --json` can still return `status: "ok"` with `script_path: null`, so this documents an invariant the CLI does not enforce. Make the null-path case an `engine_failure` before documenting this guarantee, or soften the table entry.</comment>
<file context>
@@ -169,7 +169,7 @@ Pass `--no-interactive` (and/or `--json`) to skip prompts. With `--json`, stdout
| `mode` | `string` \| `null` | `"auto"`, `"chrome-mcp"`, or `"agent-browser"`. |
| `har_path` | `string` \| `null` | Captured HAR. |
-| `script_path` | `string` \| `null` | Generated client. |
+| `script_path` | `string` \| `null` | Generated client; exists whenever `status` is `"ok"`. |
| `usage` | `object` | `{input_tokens, output_tokens, total_cost}`. |
| `error` | `string` \| `null` | When `status == "error"`. |
</file context>
| | `mode` | `string \| null` | Provider (`"auto"`, `"chrome-mcp"`, `"agent-browser"`). | | ||
| | `har_path` | `string \| null` | Absolute path to the captured HAR (`recording.har`). | | ||
| | `script_path` | `string \| null` | Absolute path to the generated client when reverse engineering ran. | | ||
| | `script_path` | `string \| null` | Absolute path to the generated client. It exists whenever `status` is `"ok"`; a run that ends without writing it reports `engine_failure`. | |
There was a problem hiding this comment.
The guarantee also covers agent --dry-run --json, which returns status: "ok" with script_path: null when its checks pass. This page documents that command just above the table. Wrapper authors could therefore mistake a successful setup check for proof that a client exists. This is a non-blocking documentation mismatch: limit the guarantee to completed capture or engineering runs, and qualify the matching README.md row too.
Artifacts
- The executed Python harness invokes the real CLI against base and head, captures responses, and asserts successful checks, a null script path, and early exit; no CLI functions are mocked.
- Ran the reproduction against the base revision and captured source, commands, JSON, tracing, and passing assertions; a successful dry run already returned a null script path.
- Ran the same reproduction against the supplied head and captured the changed documentation alongside JSON and passing assertions; the new guarantee contradicts the unchanged successful dry-run behavior.
Ran code and verified through TREX
Prompt To Fix With AI
This is a comment left during a code review.
Path: website/content/docs/cli/scripted-usage.mdx
Line: 109
Comment:
**Dry runs generate no client**
The guarantee also covers `agent --dry-run --json`, which returns `status: "ok"` with `script_path: null` when its checks pass. This page documents that command just above the table. Wrapper authors could therefore mistake a successful setup check for proof that a client exists. This is a non-blocking documentation mismatch: limit the guarantee to completed capture or engineering runs, and qualify the matching `README.md` row too.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45bfd012b9
ℹ️ 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".
| | `mode` | `string \| null` | Provider (`"auto"`, `"chrome-mcp"`, `"agent-browser"`). | | ||
| | `har_path` | `string \| null` | Absolute path to the captured HAR (`recording.har`). | | ||
| | `script_path` | `string \| null` | Absolute path to the generated client when reverse engineering ran. | | ||
| | `script_path` | `string \| null` | Absolute path to the generated client. It exists whenever `status` is `"ok"`; a run that ends without writing it reports `engine_failure`. | |
There was a problem hiding this comment.
Exclude dry runs from the script existence guarantee
For agent --dry-run --json, a successful preflight returns status: "ok" with script_path: null: _build_dry_run_payload explicitly constructs that response, and the command exits without generating a client. Since this page also documents dry runs, the unconditional existence guarantee can mislead wrappers into treating a successful preflight as a generated artifact. Qualify the guarantee as applying to completed capture/engineering runs, and retain the preflight exception in both JSON tables.
Useful? React with 👍 / 👎.
Why
The agent skill (#147) is only mentioned in the README, and the JSON docs don't say that
script_pathnow always exists onstatus: ok(#148).What
cli/coding-agents: install line, what the agent does step by step, and when it uses your Chrome (including Chrome's connection prompt). Linked from Installation and Scripted usage.script_pathin the Scripted usage table and the README JSON table: exists wheneverstatusisok; a run without a client reportsengine_failure.Verification
pnpm buildpasses; the new page renders and every internal link on it resolves to a built page.Summary by cubic
Adds docs for the agent skill and documents that
script_pathalways exists whenstatusis"ok".clickhouse/cli/coding-agentspage covers installation, step-by-step behavior, and when the skill connects to your Chrome (including the connection prompt).script_pathin the README and Scripted usage tables now guarantees existence onstatus: "ok"; a run without a client reportsengine_failure.Written for commit 45bfd01. Summary will update on new commits.
The confirmed documentation mismatch is non-blocking; the reviewed scope contains no issue that must prevent merging.
Findings
Fix with agent prompt
Summary
Adds a coding-agent guide and links it from Installation and Scripted usage.
This documentation mismatch is non-blocking. No blocking issue was found in the reviewed scope.
Reviews (1) · Last reviewed commit: "Document the agent skill and the script_..." · Reviewed by Greptile