Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c8f41eab9
ℹ️ 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 = match response.scope { | ||
| PermissionGrantScope::Turn => ToolReviewStatus::Approved, | ||
| PermissionGrantScope::Session => ToolReviewStatus::ApprovedForSession, | ||
| }; |
There was a problem hiding this comment.
Classify empty permission grants as denied reviews
PermissionsRequestApproval maps status from scope only, so Turn is always emitted as approved. But UI denial paths return an empty permissions profile with scope=Turn (see approval_overlay::handle_permissions_decision). That causes denied/timed-out permission reviews to be reported as approvals and denormalized into incorrect final_approval_outcome values on tool item events.
Useful? React with 👍 / 👎.
| let started = self.guardian_review_starts.remove(¬ification.review_id); | ||
| let created_at = started | ||
| .as_ref() | ||
| .map(|start| start.created_at) | ||
| .unwrap_or(completed_at); | ||
| let Some(status) = guardian_review_status(notification.review.status) else { |
There was a problem hiding this comment.
Deduplicate guardian review completion notifications
This handler emits a review even when guardian_review_starts.remove(review_id) returns None. Because notifications are tracked per connection, multi-client threads ingest the same completion once per connection, creating duplicate codex_tool_call_review_event records and inflating per-item review counters.
Useful? React with 👍 / 👎.
f61bae4 to
dfed0b4
Compare
6158941 to
edf6a86
Compare
507ec65 to
7a1a231
Compare
a51cdee to
e70e29a
Compare
0835613 to
9795736
Compare
ed9f62b to
fd08089
Compare
e246f33 to
637e3bb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4cc9b8873
ℹ️ 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".
| turn_id: params.turn_id, | ||
| item_id: Some(params.item_id), | ||
| review_id: user_review_id(&request_id), | ||
| subject_kind: ReviewSubjectKind::CommandExecution, |
There was a problem hiding this comment.
Classify network approval reviews as network access
When params.network_approval_context is present, this request is the app-server's Network approval presentation, not a command execution review. Recording it unconditionally as CommandExecution with subject_name of shell/unified_exec makes user network-access review telemetry inconsistent with guardian network reviews and the commit's stated first-class network access subjects.
Useful? React with 👍 / 👎.
| .. | ||
| } => {} | ||
| completed_at_ms, | ||
| response, |
There was a problem hiding this comment.
This path consumes ServerResponse facts that app-server records from the raw JSON-RPC result before the approval-specific handlers apply their effective fallback/validation. If the client returns a malformed command/file approval response, the handler later falls back to Decline, but analytics emits no review event because response_from_result never produced a typed response. For permissions, analytics can mark a raw non-empty grant as approved before the handler intersects/validates it and potentially grants nothing. Consider tracking the effective approval decision after the approval handler computes it, so analytics matches the behavior actually applied to the turn.
There was a problem hiding this comment.
^ codex-generated - if it complicates the implementation too much I actually don't think it's worth doing, fwiw
There was a problem hiding this comment.
feels like we should be tracking malformed client responses separately anyways
There was a problem hiding this comment.
fixed up permissions issue, agree on pushing off malformed response tracking
| /// | ||
| /// Uses `#[serde(default)]` for backwards compatibility with older senders. | ||
| #[serde(default = "default_guardian_command_source")] | ||
| pub source: GuardianCommandSource, |
There was a problem hiding this comment.
let's remove this, I don't want to add this implementation detail to the public app-server API.
can we populate codex_review_event.subject_name as just command_execution instead?
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
| /// | ||
| /// Uses `#[serde(default)]` for backwards compatibility with older senders. | ||
| #[serde(default = "default_guardian_command_source")] | ||
| pub source: GuardianCommandSource, |
There was a problem hiding this comment.
Can we remove this too? We also polluted the protocol a bit
There was a problem hiding this comment.
whoops yes my bad
| let (assessment, count_denial_for_circuit_breaker, completed_at_ms) = match outcome { | ||
| GuardianReviewOutcome::Completed(assessment) => { | ||
| let approved = matches!(assessment.outcome, GuardianAssessmentOutcome::Allow); | ||
| let completed_at_ms = now_unix_timestamp_ms(); |
There was a problem hiding this comment.
nit: we do this in each of the match outcome branch arms below. can we define it once right above the match outcome?
owenlin0
left a comment
There was a problem hiding this comment.
just some small comments at this point, but preapproving
Why
Review telemetry should describe reviews as first-class events, not only as counters denormalized onto terminal tool-item events. That lets us analyze guardian and user reviews consistently across command execution, file changes, permissions, and network access, while still preserving the terminal item summaries that existing tool analytics need.
To make those review events accurate, analytics also needs the observed completion time for each review and enough command metadata to distinguish
shellfromunified_execreviews.What changed
codex_review_eventrows for completed user and guardian reviews, with review subjects, reviewer, trigger, terminal status, resolution, and observed durationsourcethrough the protocol and app-server layers so review analytics can distinguishshellfromunified_execVerification
cargo test -p codex-analyticsStack created with Sapling. Best reviewed with ReviewStack.