Skip to content

fix: respect persisted monitor approval policy - #574

Merged
andrei-hasna merged 2 commits into
mainfrom
fix/44186ac8-monitor-approval-policy
Aug 12, 2026
Merged

fix: respect persisted monitor approval policy#574
andrei-hasna merged 2 commits into
mainfrom
fix/44186ac8-monitor-approval-policy

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Skip monitor command approval when the persisted approval policy is Never.
  • Preserve approval behavior for OnRequest and other approval policies, with stale-state checks unchanged.

Validation

  • Blacksmith 31600550966: codex-core monitor policy test PASS (1 passed, 3138 skipped).
  • Blacksmith 31600553258: codex-app-server monitor policy test PASS (1 passed, 1317 skipped).

Todos task: 44186ac8-757a-49fb-a1ef-f56acb0143a2


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #574 @ d49a0a5 — lens: correctness+security+gates, reviewer unresolved-account002 (1 of 1)

Blocking P1

  • The required Linux Bazel clippy gate fails on this candidate at both new helpers: monitor_command_requires_approval(policy: &AskForApproval) in codex-rs/core/src/tools/handlers/monitor_control.rs:493 and codex-rs/app-server/src/request_processors/thread_monitor_processor.rs:754. CI runs clippy with -D clippy::trivially-copy-pass-by-ref, and the failure is directly attributable to these new signatures. Small remedy: accept AskForApproval by value and pass the persisted before.approval_policy value.

What I ran

  • git log --oneline origin/main..HEAD — exit 0; head is d49a0a5e5 fix: respect persisted monitor approval policy.
  • git diff origin/main...HEAD --stat — exit 0; 2 files, 56 insertions, 25 deletions.
  • bun install — exit 0; setup only, not a repository gate.
  • gh pr view 574 --repo hasna/codewith --json statusCheckRollup — exit 0; forge rollup is 23 SUCCESS, 2 FAILURE, 3 SKIPPED.
  • gh run view 31601452478 --repo hasna/codewith --log-failed — exit 0; cargo-deny reports pre-existing webbrowser 1.0.6 advisory RUSTSEC-2026-0257 from codex-rs/Cargo.lock, which is unchanged by this PR.
  • gh run view 31601452502 --repo hasna/codewith --log-failed — exit 0; Linux Bazel clippy reports the blocking errors above.

Repository-declared gates

  • This repository declares no test script: there is no repository test gate.
  • This repository declares no typecheck script.
  • The forge checks are the authoritative build/test gates for this review. Every check and conclusion from the rollup:
    • cargo-deny — FAILURE
    • Bazel test on ubuntu-24.04 for x86_64-unknown-linux-gnu — SUCCESS
    • cla — SUCCESS
    • Check for spelling errors — SUCCESS
    • Blob size policy — SUCCESS
    • build-test — SUCCESS
    • Build Codewith Linux CLI — SUCCESS
    • Detect changed areas — SUCCESS
    • python-sdk — SUCCESS
    • Bazel test on ubuntu-24.04 for x86_64-unknown-linux-musl — SUCCESS
    • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 1/4 — SUCCESS
    • Format / etc — SUCCESS
    • sdks — SUCCESS
    • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 2/4 — SUCCESS
    • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 3/4 — SUCCESS
    • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 4/4 — SUCCESS
    • Bazel test on windows-latest for x86_64-pc-windows-gnullvm (native main) — SKIPPED
    • cargo shear — SUCCESS
    • Bazel clippy on ubuntu-24.04 for x86_64-unknown-linux-gnu — FAILURE
    • Argument comment lint package — SKIPPED
    • Bazel clippy on windows-latest for x86_64-pc-windows-gnullvm — SUCCESS
    • Verify release build on ubuntu-24.04 for x86_64-unknown-linux-gnu — SUCCESS
    • Argument comment lint - Linux — SUCCESS
    • Verify release build on windows-latest for x86_64-pc-windows-gnullvm — SUCCESS
    • Argument comment lint - Windows — SUCCESS
    • Bazel test on windows-latest for x86_64-pc-windows-gnullvm — SUCCESS
    • CI results (required) — SUCCESS
    • [code]smith — SKIPPED

Security/correctness review found no additional concrete, reachable P0/P1 issue in the changed monitor authorization paths. Non-blocking follow-up: upgrade or otherwise resolve webbrowser 1.0.6 / RUSTSEC-2026-0257; it is pre-existing and not changed by this PR. The PR remains open pending the clippy fix and a fresh exact-head review.

Agent: unresolved-account002
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] FIXED — #574 @ 84a4c83 — reviewer unresolved-account002

Fixed the named clippy blocker by changing both monitor_command_requires_approval helpers to take the Copy AskForApproval policy by value and updating their call sites/tests. This is limited to the two changed Rust files. The staged secrets scan passed (exit 0), the commit uses the required conventional subject and ends with Agent: unresolved-account002, and the branch push passed (exit 0).

The original NO_GO remains scoped to the prior head d49a0a5e52610682f1a58efdb2269c10cc590367; the current head requires the focused clippy re-check and direct regressions only.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #574 @ 84a4c83 — lens: correctness+security+gates, reviewer unresolved-account002 (1 of 1)

Reviewed the exact PR diff against origin/main at 8f761c0. The change preserves the persisted AskForApproval policy for monitor-command authorization. The focused fix changes the two new helper parameters from &AskForApproval to AskForApproval by value, clearing the required Linux Bazel clippy finding without changing behavior.

Commands and results:

  • git log --oneline origin/main..HEAD — exit 0; head includes 84a4c83.
  • git diff origin/main...HEAD --stat — exit 0; 2 files, 54 insertions, 25 deletions.
  • bun install — exit 0; setup only, not a repository gate.
  • cargo fmt -- --check — exit 0; THE LANE’S CHOSEN COMMAND, not a declared repository gate.
  • just fmt — exit 1 because the local formatter requires unavailable uv; THE LANE’S CHOSEN COMMAND, not a declared repository gate and not a blocker here.
  • git diff --check — exit 0.
  • secrets scan staged --json — exit 0; no findings.

This package.json declares no typecheck script and no test script. The repository declares no test gate; I did not invent one.

Forge statusCheckRollup command exited 0. Current check conclusions: 24 SUCCESS, 1 FAILURE, 3 SKIPPED. Checks read:

  • cargo-deny — FAILURE; pre-existing webbrowser 1.0.6 advisory in Cargo.lock, unchanged by this PR; non-blocking follow-up.
  • Bazel test on ubuntu-24.04 for x86_64-unknown-linux-gnu — SUCCESS.
  • cla — SUCCESS.
  • Check for spelling errors — SUCCESS.
  • Blob size policy — SUCCESS.
  • build-test — SUCCESS.
  • Build Codewith Linux CLI — SUCCESS.
  • Detect changed areas — SUCCESS.
  • python-sdk — SUCCESS.
  • Bazel test on ubuntu-24.04 for x86_64-unknown-linux-musl — SUCCESS.
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 1/4 — SUCCESS.
  • Format / etc — SUCCESS.
  • sdks — SUCCESS.
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 2/4 — SUCCESS.
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 3/4 — SUCCESS.
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 4/4 — SUCCESS.
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm (native main) — SKIPPED.
  • cargo shear — SUCCESS.
  • Argument comment lint package — SKIPPED.
  • Bazel clippy on ubuntu-24.04 for x86_64-unknown-linux-gnu — SUCCESS.
  • Bazel clippy on windows-latest for x86_64-pc-windows-gnullvm — SUCCESS.
  • Verify release build on ubuntu-24.04 for x86_64-unknown-linux-gnu — SUCCESS.
  • Argument comment lint - Linux — SUCCESS.
  • Verify release build on windows-latest for x86_64-pc-windows-gnullvm — SUCCESS.
  • Argument comment lint - Windows — SUCCESS.
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm — SUCCESS.
  • CI results (required) — SUCCESS.
  • [code]smith — SKIPPED.

No additional concrete, reachable in-scope P0/P1 correctness, security, integrity, unsafe-mutation, rollback, or required-gate defects found. The pre-existing cargo-deny advisory is recorded as a non-blocking follow-up because the PR does not change Cargo.lock and the required aggregate is successful.

@andrei-hasna
andrei-hasna merged commit c5a0398 into main Aug 12, 2026
27 of 28 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 12, 2026
@andrei-hasna
andrei-hasna deleted the fix/44186ac8-monitor-approval-policy branch August 12, 2026 14:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant