Conversation
![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: e7a06378ab
ℹ️ 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".
| let status = loop { | ||
| if let Some(status) = child.try_wait().map_err(|error| { | ||
| APWError::new( | ||
| Status::ProcessNotRunning, | ||
| format!( |
There was a problem hiding this comment.
This loop waits for the child to exit via try_wait() before draining stdout/stderr, even though both streams are configured as pipes. If the provider emits more than a pipe buffer (common for large bw list items or op item list JSON), the child can block on write and never reach exit, so APW hits the timeout path and kills a healthy process. The regression is specific to commands with larger output and causes false CommunicationTimeout errors under normal usage with bigger vaults.
Useful? React with 👍 / 👎.
athena-omt
left a comment
athena-omt
left a comment
There was a problem hiding this comment.
Found one substantive blocker.
reserve_external_provider_invocation implements the per-session cap with an unprotected read/modify/write of fallback-provider-session.json. Concurrent native-app fallback requests can all read the same invocations value, each pass the limit check, and then overwrite the file with the same incremented count. That means the configured cap is not actually enforceable under parallel browser/native-host requests, which is the security boundary this PR is trying to add.
Suggested fix: make the reservation atomic across processes, e.g. lock a sidecar/state file with flock/fs2, then read-check-write while holding the lock; or use an atomic append/counter mechanism. Add a concurrency regression test with max invocations set low.
Validation note: git diff --check origin/main...HEAD passed. I attempted the targeted Rust fallback-provider tests, but this Athena Linux environment lacks OpenSSL pkg-config/dev metadata, so the local cargo run failed before tests executed. GitHub CI for the PR is green.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7a06378ab
ℹ️ 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".
| let limits = external_provider_limits(config); | ||
| reserve_external_provider_invocation(provider, &limits)?; | ||
|
|
There was a problem hiding this comment.
The invocation counter is incremented before command.spawn() runs, so failed launches (for example, missing execute permission or a transient ENOENT) still consume the session budget. With a low fallbackProviderMaxInvocations, users can be locked out by repeated spawn failures even though no external provider process actually ran, which contradicts the configured “invocations” semantics and makes recovery depend on reauth/editing config.
Useful? React with 👍 / 👎.
| let state = fs::read_to_string(&path) | ||
| .ok() | ||
| .and_then(|content| serde_json::from_str::<Value>(&content).ok()); | ||
| let current_count = state | ||
| .as_ref() |
There was a problem hiding this comment.
This reads the state file and later writes back an incremented count without any inter-process lock, so concurrent apw calls can both observe the same current_count and proceed, allowing the configured max to be exceeded. The limit is documented as a hard session bound, but this read-modify-write race makes enforcement unreliable under parallel logins/fills.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7a06378ab
ℹ️ 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".
athena-omt
left a comment
athena-omt
left a comment
There was a problem hiding this comment.
I could not complete a source-backed review because the required Athena worker node is currently not connected. The mandated first smoke/setup check against /openclaw-data/src/_worktrees/athena/omt-global/apw-cli/pr-32 failed before any repository inspection could run.
I’m not approving or requesting changes from source-only/coordinator inspection. The existing CHANGES_REQUESTED review about non-atomic fallback invocation reservation remains the current substantive review signal until a worker-backed follow-up can run.
2 participants
Summary
Closes #3
Validation
cargo fmt --manifest-path rust/Cargo.toml -- --checkcargo test --manifest-path rust/Cargo.toml native_app::tests::external_provider_ -- --nocapturecargo test --manifest-path rust/Cargo.toml external_provider -- --nocapturecargo test --manifest-path rust/Cargo.toml native_app::tests::login_can_fallback_to_1password_clicargo test --manifest-path rust/Cargo.toml native_app::tests::login_bitwarden_fallback_matches_uri_before_selecting_itemcargo test --manifest-path rust/Cargo.toml --test security_regressionscargo test --manifest-path rust/Cargo.toml --test native_app_e2e login_works_via_direct_fallback_when_service_not_runningcargo test --manifest-path rust/Cargo.toml --test native_app_e2e fill_works_via_direct_fallback_when_service_not_runningcargo test --manifest-path rust/Cargo.toml --test native_app_e2e direct_fallback_maps_malformed_response_to_proto_invalid_responsegit diff --checkNotes
native_app_e2estill hangs in existing socket-backed tests (fill_uses_fill_intent_over_socket); the direct fallback e2e cases relevant to this issue passed individually.