feat(harness): stage reviewed browser agent inputs - #803
Conversation
cebbca8 to
f967288
Compare
0b57620 to
f9402c3
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: f9402c3e5f
ℹ️ 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".
| this whole path exists to avoid. An optional browser-supplied ``plan`` is | ||
| materialized into a temporary ``--plan-file`` so the CLI retains ownership | ||
| of plan scanning and hashing; ``read_files`` travel as repeatable | ||
| ``--read-file=<path>`` arguments and are resolved only inside the clone. |
There was a problem hiding this comment.
Move this newly added plan/read-file explanation out of the run_agentic_op function docstring and express it with # comments instead. CLAUDE.md's Critical Python Coding Requirement forbids multiline docstrings outside the beginning of a Python file, and AGENTS.md identifies that file as the detailed operating contract, so extending this function docstring violates the repository's explicit Python convention.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| } else if (pendingAgentRun.read_files.includes(path)) { | ||
| sys('read path is already declared: ' + path); | ||
| } else { | ||
| pendingAgentRun.read_files.push(path); |
There was a problem hiding this comment.
When an operator stages a ninth distinct path, the console accepts and displays it even though AgentRunRequest.read_files allows at most eight. /agent confirm then receives a 422 response, and because the console clears pendingAgentRun before making that request, the instruction, reviewed plan, checks, and paths are all lost. Reject the ninth entry in the staging UI—or preserve the staged run when server validation fails—rather than allowing an unrecoverable confirmation failure.
Useful? React with 👍 / 👎.
…nsole tests Resolve EOF conflict in tests/test_harness_console_contract.py by keeping test_pending_agent_diff_is_shown_before_approval and test_staged_agent_plan_read_paths_and_checks_reach_confirmation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 397f284962
ℹ️ 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".
| """A plan emitted by the CLI must fit through the browser handoff path. | ||
|
|
||
| Harness cannot import agentic at runtime (I6), so the intentional duplicate | ||
| is checked here. ``generate_plan`` appends its fixed marker after the 6k | ||
| body limit, which is why equality would reject a valid generated plan. |
There was a problem hiding this comment.
Convert this newly added multiline function docstring to # comments. CLAUDE.md's Critical Python Coding Requirement forbids multiline docstrings outside the beginning of a Python file, so correcting only the already-commented utils/ops_runner.py occurrence would still leave this commit in violation of the repository's Python convention.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| def _read_files_are_bounded_text(cls, values: list[str]) -> list[str]: | ||
| for path in values: | ||
| if not path or len(path) > _MAX_READ_FILE_LEN or "\x00" in path: | ||
| raise ValueError("read_files must contain non-empty bounded paths without NUL bytes") |
There was a problem hiding this comment.
Validate read_files as safe repo-relative paths here rather than accepting only length and NUL constraints. Requests containing /etc/passwd, ../README.md, or drive-qualified paths currently pass this model, but agentic.real_repo_loop._render_existing_files catches the jailed reader's AgenticError and silently skips them; a confirmed run therefore incurs the clone/model loop without the context the console said was declared and can repeatedly refuse the intended existing-file edit.
Useful? React with 👍 / 👎.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…confirm fail Codex P2s on #803: reject non-repo-relative read paths at the control plane (utils.repo_paths, same rules as agentic canonical_repo_path), enforce the 8 path cap in the console before confirm, and preserve pendingAgentRun when /agent confirm gets a 422 so plan/instruction/checks are not wiped. Also rename plan/read validators (WPS110) and move plan/read ops_runner prose out of the function docstring into comments (CLAUDE.md convention). Drift test keeps utils and agentic path acceptance in lockstep without harness importing agentic (I6).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28cdc8435c
ℹ️ 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".
| sys('cloning, planning, patching and verifying — this blocks for up to 15 minutes…'); | ||
| try { | ||
| const rec = agentRecord(await api('/api/agent/run', 'POST', body)); | ||
| if (pendingAgentRun === stagedRun) pendingAgentRun = null; |
There was a problem hiding this comment.
When the CLI rejects a staged plan, reports an environment error, or the agentic layer is disabled, /api/agent/run still returns HTTP 200 with ok=false or no parsed record, so api() resolves and agentRecord() returns null rather than throwing. This line nevertheless clears pendingAgentRun, losing the reviewed plan, paths, and checks despite the new recovery behavior; clear the staged run only after agentRecord() returns a real run record.
Useful? React with 👍 / 👎.
| plan, checks, and paths with no recovery short of re-staging. */ | ||
| const stagedRun = pendingAgentRun; | ||
| const body = Object.assign({}, stagedRun, { reason: why, confirm: true }); | ||
| sendBtn.disabled = true; |
There was a problem hiding this comment.
While the potentially 15-minute confirmation request is running, this disables only the button and deliberately leaves pendingAgentRun populated. The input's existing Enter handler calls onSend() without checking sendBtn.disabled, so entering /agent confirm ... again launches a second concurrent clone/model/verification run for the same staged proposal; use an explicit in-flight guard or disable the input path as well.
Useful? React with 👍 / 👎.
|
"harness/schemas.py:127:5: WPS231 Found function with too much cognitive complexity: 15 > 12" ^ HA for me or you genuine question |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84f0108a66
ℹ️ 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".
|
|
||
|
|
||
| def canonical_repo_relative_path(target: str) -> str | None: | ||
| """Return ``target``'s canonical repo-relative form, or ``None`` if unsafe. |
There was a problem hiding this comment.
Convert this multiline function docstring to # comments. CLAUDE.md's Critical Python Coding Requirement forbids multiline docstrings outside the beginning of a Python file, and AGENTS.md identifies CLAUDE.md as the detailed operating contract, so the new helper violates the repository's explicit Python convention.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
|
PR template check (advisory, non-blocking) This PR body doesn't look like it covers everything
This isn't a required check -- it won't block merge. See the template's own "Notes for contributors" for which sections lighter-path PRs can skip. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83f840c15e
ℹ️ 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".
| parts = tuple(part for part in normalized.split("/") if part not in {"", "."}) | ||
| if not parts or any(part == ".." or ":" in part for part in parts): |
There was a problem hiding this comment.
Fresh evidence after the earlier path-validation report: this new validator mirrors canonical_repo_path, but the actual read goes through ScopedRoots.read_bytes and split_components, which additionally rejects trailing-dot/space components and device-namespace spellings. Consequently, inputs such as /agent read README.md. are accepted and staged here, but after the repository is cloned _render_existing_files catches the reader's AgenticError and silently omits the declared content, so the coder runs without the context the operator confirmed. Validate against the jailed reader's complete acceptance contract rather than only canonical_repo_path.
Useful? React with 👍 / 👎.
Fixes four issues flagged by chatgpt-codex-connector on PR #803: - static/harness.html: /agent confirm unconditionally cleared the staged run after the request resolved, even when the server rejected it (agentRecord() returns null rather than throwing on ok=false or an unparsed response) -- losing the reviewed plan/read paths/checks with no recovery. Only clear the staged run once a real record confirms the server accepted it. - static/harness.html: the Enter-key handler bypassed sendBtn's disabled state, so pressing Enter during the up-to-15-minute /agent confirm request launched a second concurrent clone/model/verification run for the same staged proposal. Guard onSend() on the existing in-flight signal. - utils/repo_paths.py: canonical_repo_relative_path mirrored the WRITE jail's rules (canonical_repo_path) but harness's declared read_files actually flow through the stricter READ jail (ScopedRoots.read_bytes / split_components), which additionally rejects a trailing space or dot in a path component. A path like "README.md." validated here, got staged and confirmed, then silently vanished from the coder's context when agentic.real_repo_loop._render_existing_files caught the reader's AgenticError -- reject it up front instead. - utils/ops_runner.py: converted run_agentic_op's multi-line docstring to a one-line summary + `#` comments per CLAUDE.md's Critical Python Coding Requirement (no multi-line docstrings outside a file's opening module docstring). Added regression tests for the two path-validation fixes and verified against the console contract test and the full targeted harness/ops_runner suite (233 passed), ruff, invariant-guard (33/0), and a JS syntax check of the embedded harness.html script. Co-Authored-By: CyClaw Agent <cyclaw-agent@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 514abc8310
ℹ️ 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".
| normalized = target.replace("\\", "/") | ||
| if normalized.startswith(("/", "-")) or PureWindowsPath(target).is_absolute(): |
There was a problem hiding this comment.
Fresh evidence after the earlier path-validation report: the trailing-dot case is now handled, but normalization still erases the read jail's device-namespace markers. For example, foo\\?\\bar is accepted and canonicalized to foo/?/bar, whereas agentic.fsconnect.pathsafe.split_components rejects the original spelling; a staged run can therefore read a different path or silently omit the declared context. Reject \\?\ and \\.\ spellings before replacing backslashes.
Useful? React with 👍 / 👎.
2 participants
Summary
Implements reviewed browser plan/read-file/check parity, stacked on #802.
Invariant / CG-Security Invariant Check (CyClaw)
agentic/only through the existingutils.ops_runnersubprocess shim.Verification Steps before commit
main-> fix(harness): show candidate diff before approval #802 -> this branch passed without conflicts.Scope and risks