feat: recording gate observability + user feedback#7425
Conversation
Add structured logging for CU session initialization, recording gate status transitions, and destructive action blocking. Enhance recording handshake timeout error messages with full session context and actionable diagnostics. Increase Swift overlay error text maxHeight from 60 to 80 and enable text selection on recording warnings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7403096117
ℹ️ 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".
| log.info( | ||
| { | ||
| sessionId, | ||
| qaMode: !!requiresRecording, |
There was a problem hiding this comment.
Derive qaMode from QA flag, not recording requirement
The new initialization log sets qaMode to !!requiresRecording, but QA sessions are not always recording-gated (for example, requiresRecording is only set when config.qaRecording.enforceStartBeforeActions is enabled in assistant/src/daemon/handlers/shared.ts:277-299). When that flag is off, real QA sessions will now be logged as qaMode: false, which undermines the observability this commit is trying to improve and makes QA incident triage misleading.
Useful? React with 👍 / 👎.
| log.info( | ||
| { | ||
| sessionId, | ||
| qaMode: !!requiresRecording, |
There was a problem hiding this comment.
🟡 qaMode log field incorrectly derived from requiresRecording instead of actual qaMode
The new constructor log at line 147 computes qaMode: !!requiresRecording, but qaMode and requiresRecording are independent fields in the IPC contract (assistant/src/daemon/ipc-contract.ts:183 and assistant/src/daemon/ipc-contract.ts:189). The ComputerUseSession constructor never receives the actual qaMode value from the CuSessionCreate message.
Root Cause
In assistant/src/daemon/handlers/computer-use.ts:89-102, the handler creates a ComputerUseSession passing msg.requiresRecording but not msg.qaMode. The constructor then logs qaMode: !!requiresRecording as a proxy. However, a session can be in QA mode (msg.qaMode: true) without requiring recording (msg.requiresRecording: undefined), making the logged qaMode value false when it should be true. This produces misleading structured logs that will confuse debugging of QA session initialization — the exact scenario this PR is trying to improve observability for.
Impact: Incorrect qaMode field in structured logs. Operators filtering/alerting on qaMode in the CU session initialized log entry will get wrong results.
Prompt for agents
The qaMode field in the log at assistant/src/daemon/computer-use-session.ts:147 is derived from requiresRecording, but qaMode is a separate IPC field. Either:
1. Pass qaMode as a separate constructor parameter (from msg.qaMode in assistant/src/daemon/handlers/computer-use.ts:89-102) and log it directly, OR
2. Remove the qaMode field from this log since the constructor doesn't have access to it, and rely on the existing log in the handler at line 124 which could be enhanced to include qaMode from msg.qaMode.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Addressed in #7462 |
Summary
.textSelection(.enabled)on recording warning text so users can copy error detailsTest plan
CU session initializedwith all metadata fields when a new CU session is createdRecording gate status changedwith previous/new status🤖 Generated with Claude Code