Skip to content

feat: recording gate observability + user feedback #7425

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jasonnnz merged 1 commit into from
Feb 24, 2026
Merged

feat: recording gate observability + user feedback #7425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 31 additions & 5 deletions assistant/src/daemon/computer-use-session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,18 @@ export class ComputerUseSession {
this.targetAppName = targetAppName;
this.targetAppBundleId = targetAppBundleId;
this.requiresRecording = requiresRecording ?? false;

log.info(
{
sessionId,
qaMode: !!requiresRecording,
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

requiresRecording: this.requiresRecording,
targetAppName,
targetAppBundleId,
recordingGateStatus: this.recordingGateStatus,
},
'CU session initialized',
);
}

// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -197,11 +209,19 @@ export class ComputerUseSession {
this.recordingHandshakeTimer = setTimeout(() => {
if (this.recordingGateStatus === 'pending') {
log.error(
{ sessionId: this.sessionId, timeoutMs: RECORDING_HANDSHAKE_TIMEOUT_MS },
{
sessionId: this.sessionId,
timeoutMs: RECORDING_HANDSHAKE_TIMEOUT_MS,
recordingGateStatus: this.recordingGateStatus,
requiresRecording: this.requiresRecording,
targetAppName: this.targetAppName,
targetAppBundleId: this.targetAppBundleId,
stepCount: this.stepCount,
},
'Recording handshake timeout — recording never started',
);
this.abortWithError(
'Recording handshake timed out: recording did not start within 8 seconds. Session aborted to prevent unrecorded actions.',
`Recording handshake timed out after ${RECORDING_HANDSHAKE_TIMEOUT_MS / 1000} seconds. The screen recording did not start. Session aborted because recording is required for QA sessions. Check screen recording permissions in System Settings > Privacy & Security.`,
);
}
}, RECORDING_HANDSHAKE_TIMEOUT_MS);
Expand Down Expand Up @@ -692,16 +712,22 @@ export class ComputerUseSession {
if (this.requiresRecording && this.recordingGateStatus !== 'started') {
if (this.recordingGateStatus === 'failed') {
const reason = this.recordingFailureReason ?? 'unknown';
this.abortWithError(`Recording failed: ${reason}. Session cannot proceed without recording.`);
this.abortWithError(`Recording failed to start: ${reason}. Session aborted because recording is required for QA sessions.`);
return {
content: `Recording failed: ${reason}. Session aborted.`,
isError: true,
};
}
if (this.recordingGateStatus === 'pending' && DESTRUCTIVE_TOOLS.has(toolName)) {
log.warn(
{ sessionId: this.sessionId, toolName, recordingGateStatus: this.recordingGateStatus },
'Blocked destructive action — recording has not started yet',
{
sessionId: this.sessionId,
toolName,
recordingGateStatus: this.recordingGateStatus,
requiresRecording: this.requiresRecording,
stepCount: this.stepCount,
},
'Blocked destructive action — recording not started',
);
return {
content: 'Recording has not started yet. Waiting for recording confirmation before executing destructive actions. Use computer_use_wait to wait, or computer_use_done/computer_use_respond if the task can be completed without interaction.',
Expand Down
14 changes: 10 additions & 4 deletions assistant/src/daemon/handlers/computer-use.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,14 +389,20 @@ function handleCuRecordingStatus(msg: CuRecordingStatus, _socket: net.Socket, ct
log.warn({ sessionId: msg.sessionId }, 'CU recording status for unknown session');
return;
}
log.info(
{ sessionId: msg.sessionId, status: msg.status, reason: msg.reason },
'CU recording status update',
);
const previousStatus = session.recordingGateStatus;
session.recordingGateStatus = msg.status;
if (msg.status === 'failed' && msg.reason) {
session.recordingFailureReason = msg.reason;
}
log.info(
{
sessionId: msg.sessionId,
previousStatus,
newStatus: msg.status,
reason: msg.reason,
},
'Recording gate status changed',
);
}

export const computerUseHandlers = defineHandlers({
Expand Down
3 changes: 2 additions & 1 deletion clients/macos/vellum-assistant/Features/Session/SessionOverlayView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,9 @@ struct SessionOverlayView: View {
.foregroundColor(VColor.error)
.fixedSize(horizontal: false, vertical: true)
.frame(maxWidth: .infinity, alignment: .leading)
.textSelection(.enabled)
}
.frame(maxHeight: 60)
.frame(maxHeight: 80)
}
.padding(.horizontal, VSpacing.xs)
} else if session.isRecordingActive {
Expand Down
Loading