Repository navigation
fix(claude): carry permission mode across remote/local mode switches - #10
leeroybrun wants to merge 3 commits into
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdded structured permission-mode constants and type guards; extended Metadata with permissionMode and timestamp; CLI parsing for --permission-mode/--yolo; runners (Codex/Gemini/Claude) initialize and persist permissionMode; Claude preserves and reapplies permission mode across remote→local transitions and records incoming mode updates. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Runner as Agent Runner (Codex / Gemini / Claude)
participant Session as Session / Metadata Store
participant Launcher as Claude Launcher (local)
participant Model as Remote Model
CLI->>Runner: start (includes permissionMode / --yolo)
Runner->>Session: create session with permissionMode & timestamp
Runner->>Model: start remote session (permissionMode may apply)
Model-->>Runner: messages (may include permissionMode updates)
Runner->>Session: update permissionMode & timestamp
alt Claude local transition
Session->>Launcher: provide lastPermissionMode
Launcher->>Model: spawn local process with sanitized args + mode
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @leeroybrun, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a long-standing issue where the chosen permission mode for AI agents was not reliably maintained when users switched between local and remote modes or initiated sessions via the command line. By introducing mechanisms to track, propagate, and infer permission modes through session metadata and dynamic argument handling, the changes significantly improve the consistency and predictability of agent behavior, eliminating the need for users to repeatedly reconfigure their settings. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
@codex @coderabbitai @gemini-code-assist please review |
|
@leeroybrun: I'll review the changes in this pull request. ✅ Actions performedReview triggered.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request does a great job of adding persistent permission modes across local and remote switches for Claude, and extends similar support to Codex and Gemini. The changes are well-structured and address the problem described effectively by leveraging session metadata. I've identified a bug in the command-line argument parsing and some areas for code improvement to enhance maintainability and readability. Overall, this is a solid contribution.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/index.ts:
- Around line 91-99: Validate the value assigned to permissionMode when parsing
args: instead of blindly casting args[++i] to PermissionMode, check it against
an allowed set of modes (e.g. an array of valid PermissionMode strings including
'yolo' and whatever modes are defined in import('@/api/types').PermissionMode),
and if the value is not included, log a clear error and either exit or leave
permissionMode undefined; ensure this validation happens before runCodex is
called so only valid PermissionMode values are passed into runCodex.
🧹 Nitpick comments (2)
src/gemini/runGemini.ts (1)
196-201: Consider removing dead code.Since
currentPermissionModeis now initialized toinitialPermissionModeon line 171 (which is always defined—defaulting to'default'), this condition will never be true. The code block is unreachable.Suggested removal
- // Initialize permission mode if not set yet - if (currentPermissionMode === undefined) { - currentPermissionMode = 'default'; - updatePermissionMode('default'); - }src/index.ts (1)
214-222: Duplicate permission mode parsing logic.The
--permission-modeand--yoloparsing in the gemini subcommand is identical to codex. Consider extracting a shared helper function to reduce duplication and ensure consistent behavior.♻️ Suggested refactor
// At the top level of the file or in a shared utils module function parsePermissionModeArgs(args: string[], startIndex: number): { permissionMode: import('@/api/types').PermissionMode | undefined; startedBy: 'daemon' | 'terminal' | undefined; } { let permissionMode: import('@/api/types').PermissionMode | undefined = undefined; let startedBy: 'daemon' | 'terminal' | undefined = undefined; for (let i = startIndex; i < args.length; i++) { if (args[i] === '--started-by') { startedBy = args[++i] as 'daemon' | 'terminal'; } else if (args[i] === '--permission-mode') { permissionMode = args[++i] as import('@/api/types').PermissionMode; } else if (args[i] === '--yolo') { permissionMode = 'yolo'; } } return { permissionMode, startedBy }; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/api/types.tssrc/claude/claudeLocalLauncher.tssrc/claude/loop.tssrc/claude/runClaude.tssrc/claude/session.tssrc/claude/utils/permissionHandler.tssrc/codex/runCodex.tssrc/gemini/runGemini.tssrc/index.tssrc/utils/createSessionMetadata.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}: Use descriptive variable and function names that clearly convey intent
Write clear, concise comments explaining the 'why' behind complex logic
Keep functions small and focused on a single responsibility
Use consistent code formatting and adhere to the project's style guide
Files:
src/api/types.tssrc/claude/session.tssrc/gemini/runGemini.tssrc/utils/createSessionMetadata.tssrc/claude/loop.tssrc/claude/claudeLocalLauncher.tssrc/claude/utils/permissionHandler.tssrc/claude/runClaude.tssrc/index.tssrc/codex/runCodex.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: No untyped code - use strict typing in TypeScript with explicit parameter and return types
Use comprehensive JSDoc comments - each file includes header comments explaining responsibilities
ALL imports must be at the top of the file - NEVER import modules mid-code
Use named exports preferred, with occasional default exports for main functions
Avoid creating stupid small functions, getters, or setters - minimize unnecessary function decomposition
Avoid excessive use ofifstatements - especially if you can avoid control flow changes with a better design
Files:
src/api/types.tssrc/claude/session.tssrc/gemini/runGemini.tssrc/utils/createSessionMetadata.tssrc/claude/loop.tssrc/claude/claudeLocalLauncher.tssrc/claude/utils/permissionHandler.tssrc/claude/runClaude.tssrc/index.tssrc/codex/runCodex.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Use@/alias for src imports, e.g.,import { logger } from '@/ui/logger'
Use graceful error handling with proper error messages, try-catch blocks with specific error logging, and abort controllers for cancellable operations
All debugging must be through file logs to avoid disturbing Claude sessions - console output only for user-facing messages
Handle large JSON objects with truncation in logs to avoid excessive output
Minimize use of classes - prefer functional approaches in TypeScript
Files:
src/api/types.tssrc/claude/session.tssrc/gemini/runGemini.tssrc/utils/createSessionMetadata.tssrc/claude/loop.tssrc/claude/claudeLocalLauncher.tssrc/claude/utils/permissionHandler.tssrc/claude/runClaude.tssrc/index.tssrc/codex/runCodex.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
.tsfile extensions for TypeScript files
Files:
src/api/types.tssrc/claude/session.tssrc/gemini/runGemini.tssrc/utils/createSessionMetadata.tssrc/claude/loop.tssrc/claude/claudeLocalLauncher.tssrc/claude/utils/permissionHandler.tssrc/claude/runClaude.tssrc/index.tssrc/codex/runCodex.ts
src/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/api/**/*.{ts,tsx}: Implement end-to-end encryption for all communications using TweetNaCl
Use Socket.IO for real-time messaging in API sessions
Implement optimistic concurrency control for state updates in sessions
Implement session isolation through unique session IDs
Files:
src/api/types.ts
src/api/types.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Zod schemas for type-safe API communication in types files
Files:
src/api/types.ts
🧠 Learnings (14)
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/api/**/*.{ts,tsx} : Implement optimistic concurrency control for state updates in sessions
Applied to files:
src/claude/session.tssrc/utils/createSessionMetadata.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/api/**/*.{ts,tsx} : Implement session isolation through unique session IDs
Applied to files:
src/claude/session.tssrc/utils/createSessionMetadata.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/claude/watcher.{ts,tsx} : Implement file system watcher for Claude session files in interactive mode
Applied to files:
src/claude/session.tssrc/claude/claudeLocalLauncher.tssrc/claude/utils/permissionHandler.tssrc/claude/runClaude.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Emit 'machine-update-metadata' WebSocket events when machine metadata changes (rare updates like version changes), including expectedVersion for version mismatch handling
Applied to files:
src/utils/createSessionMetadata.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/**/*.{ts,tsx} : All debugging must be through file logs to avoid disturbing Claude sessions - console output only for user-facing messages
Applied to files:
src/claude/claudeLocalLauncher.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/api/apiSession.{ts,tsx} : Register RPC handlers for remote procedure calls in session clients
Applied to files:
src/claude/claudeLocalLauncher.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Register RPC handlers for 'spawn-happy-session', 'stop-session', and 'requestShutdown' on the daemon's HTTP server and WebSocket connection
Applied to files:
src/claude/claudeLocalLauncher.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : When daemon receives RPC 'spawn-happy-session' from backend, create directory if needed, spawn detached Happy process with flags '--happy-starting-mode remote --started-by daemon', add to pidToTrackedSession map, and set up 10-second awaiter for session webhook
Applied to files:
src/claude/claudeLocalLauncher.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/src/index.ts : In CLI startup phase, check if machine ID exists in settings; if not, create ID locally only without creating machine on server (daemon's responsibility on startup)
Applied to files:
src/claude/runClaude.tssrc/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Perform authentication via authAndSetupMachineIfNeeded() during daemon startup to ensure credentials exist before establishing backend connections
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/*.{ts,tsx} : When daemon receives 'daemon start' command in src/index.ts, spawn a detached process via spawnHappyCLI(['daemon', 'start-sync'], { detached: true }) that calls startDaemon() from src/daemon/run.ts
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : During daemon startup, perform version check by reading daemon.state.json and comparing startedWithCliVersion with configuration.currentCliVersion; if version mismatch detected, call stopDaemon() to kill old daemon before proceeding
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : On daemon startup, POST to /v1/machines REST endpoint with encrypted metadata and daemonState, sending machineId, and receive metadataVersion and daemonStateVersion from server response
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Ensure caffeinate process is started by the daemon (not individual sessions) and tracked in daemon.state.json to prevent it from becoming a runaway process
Applied to files:
src/index.ts
🧬 Code graph analysis (7)
src/api/types.ts (1)
src/claude/loop.ts (1)
PermissionMode(12-12)
src/claude/session.ts (2)
src/api/types.ts (1)
PermissionMode(16-16)src/claude/loop.ts (1)
PermissionMode(12-12)
src/gemini/runGemini.ts (1)
src/utils/createSessionMetadata.ts (1)
createSessionMetadata(70-96)
src/claude/claudeLocalLauncher.ts (2)
src/api/types.ts (1)
PermissionMode(16-16)src/claude/loop.ts (1)
PermissionMode(12-12)
src/claude/runClaude.ts (1)
src/api/types.ts (1)
PermissionMode(16-16)
src/index.ts (2)
src/codex/runCodex.ts (1)
runCodex(66-808)src/gemini/runGemini.ts (1)
runGemini(59-1140)
src/codex/runCodex.ts (1)
src/utils/createSessionMetadata.ts (1)
createSessionMetadata(70-96)
🔇 Additional comments (23)
src/api/types.ts (1)
328-336: LGTM!The new
permissionModeandpermissionModeUpdatedAtfields are well-documented with JSDoc comments. The optional fields maintain backward compatibility, and the types align with the existingPermissionModetype defined earlier in the file. The timestamp approach for "latest wins" arbitration is a sound design for cross-device synchronization.src/claude/session.ts (2)
27-33: LGTM!The new fields for tracking permission mode state are well-documented. Initializing
lastPermissionModeto'default'andlastPermissionModeUpdatedAtto0provides sensible defaults. The JSDoc clearly explains the purpose for carrying permission settings across remote ↔ local mode switches.
88-96: LGTM!The
setLastPermissionModemethod correctly updates both local state and publishes to session metadata atomically. UsingDate.now()as the default forupdatedAtis convenient for callers. The implementation follows the existing pattern ofonSessionFoundfor metadata updates and aligns with the optimistic concurrency control pattern per coding guidelines.src/claude/utils/permissionHandler.ts (2)
59-62: LGTM!Adding
session.setLastPermissionMode(mode)inhandleModeChangeensures the session metadata stays synchronized when the permission mode changes. This is essential for preserving mode across remote ↔ local switches.
83-87: LGTM!Propagating the permission mode from responses to session state ensures that mode changes initiated by the app (via permission responses) are correctly tracked. The
Date.now()default timestamp will be used, which is appropriate for response-driven updates.src/utils/createSessionMetadata.ts (2)
34-37: LGTM!The new optional fields in
CreateSessionMetadataOptionsare well-documented with JSDoc comments explaining their purpose. The optional nature maintains backward compatibility with existing callers.
90-93: LGTM!The conditional spreading pattern is idiomatic for optional object properties. Using
typeof opts.permissionModeUpdatedAt === 'number'correctly handles the edge case where0would be a valid timestamp value (unlike a simple truthy check). The truthy check forpermissionModeis safe since allPermissionModevalues are non-empty strings.src/gemini/runGemini.ts (5)
62-62: LGTM!Adding the optional
permissionModeparameter allows CLI-specified modes to be passed through to the Gemini runner, consistent with the PR's objective of supporting--permission-modeand--yoloflags for codex/gemini commands.
111-118: LGTM!The IIFE correctly validates the permission mode against Gemini-compatible modes (
'default','read-only','safe-yolo','yolo'), appropriately filtering out Claude-specific modes. Defaulting to'default'for invalid or missing values is sensible.
120-126: LGTM!Passing
permissionModeandpermissionModeUpdatedAttocreateSessionMetadataensures the initial permission mode is published to session metadata at creation time. This enables the app to seed UI state immediately, addressing the PR objective.
171-171: LGTM!Initializing
currentPermissionModefrominitialPermissionModeensures the in-memory state reflects the CLI-specified or default mode from the start, rather than beingundefined.
185-189: LGTM!Updating session metadata when a valid permission mode arrives in a user message ensures the app stays synchronized with runtime mode changes. Using
Date.now()for the timestamp supports the "latest wins" arbitration pattern.src/codex/runCodex.ts (3)
69-76: LGTM - Permission mode type handling is correct.The inline import type syntax and local type alias work correctly. The
EnhancedModeinterface appropriately encapsulates permission mode with model for message queue hashing.
112-119: LGTM - Initial permission mode correctly propagated to session metadata.The initialization correctly:
- Defaults to
'default'whenopts.permissionModeis undefined- Passes both
permissionModeandpermissionModeUpdatedAttocreateSessionMetadataThis aligns with the pattern in
src/utils/createSessionMetadata.tswhich conditionally includes these fields.
165-179: LGTM - Permission mode state tracking and metadata sync.The implementation correctly:
- Seeds
currentPermissionModefrominitialPermissionMode(line 165)- Updates in-memory state when user message includes override (line 173)
- Persists to session metadata with fresh timestamp (lines 175-179)
This ensures the app can reflect permission mode changes immediately.
src/claude/loop.ts (1)
65-67: LGTM - Early permission mode publication ensures UI consistency.Publishing the initial permission mode immediately after session creation (before
onSessionReady) ensures the app can display the correct mode from the start, even for CLI-specified modes like--dangerously-skip-permissions.src/claude/runClaude.ts (3)
113-116: LGTM - Permission mode resolution chain is correct.The priority order is sensible:
- Explicit
options.permissionMode(e.g., from programmatic call)- Inferred from
claudeArgs(e.g.,--dangerously-skip-permissions)- Default to
'default'Assigning back to
options.permissionModeensures downstream code (likeloop()) receives the resolved value.
134-136: LGTM - Session metadata correctly initialized with permission mode.The metadata includes both
permissionModeandpermissionModeUpdatedAt, consistent with the pattern established increateSessionMetadata.tsandrunCodex.ts.
46-71: Code correctly validates Claude CLI permission modes.The function properly recognizes only the 4 modes supported by Claude Code CLI (
default,acceptEdits,bypassPermissions,plan). ThePermissionModetype includes 7 modes total because it also covers Codex modes (read-only,safe-yolo,yolo), which are intentionally excluded from Claude's CLI argument parsing. These Codex modes are translated to Claude equivalents via themapToClaudeMode()mapping at the SDK boundary. The implementation is correct as-is.src/claude/claudeLocalLauncher.ts (3)
9-36: Consider edge case when all args are permission-related.When the input
argscontains only permission-related flags (e.g.,['--dangerously-skip-permissions']) andmodeis'default', the function returnsundefinedinstead of an empty array. This is handled correctly sinceclaudeLocallikely acceptsundefined, but worth noting.The logic correctly:
- Strips existing
--permission-mode <value>and--dangerously-skip-permissions- Appends the mapped Claude mode if not
'default'
107-110: LGTM - Permission mode recorded on message receipt.Calling
session.setLastPermissionMode(mode.permissionMode)when a message is received ensures the session tracks the latest permission mode, which is then used when spawning local Claude.
133-136: Mutation is intentional and safe.The reassignment of
session.claudeArgsat line 135 is intentional and properly documented in the comments. The purpose is to ensure the session's args are updated with the correct permission mode arguments before spawning local Claude, which is essential for handling remote→local switches. The mutatedclaudeArgsis used immediately afterward on line 144, and no code paths depend on the original value persisting after this mutation.src/index.ts (1)
105-105: LGTM - Permission mode correctly passed to agent runners.Both
runCodexandrunGemininow receive thepermissionModeparameter, enabling CLI-specified permission modes to be reflected in the app immediately upon session creation.Also applies to: 242-242
There was a problem hiding this comment.
Code Review
The pull request effectively addresses the problem of permission mode loss across Claude local and remote switches, as well as ensuring consistent initial states for Codex and Gemini agents. The introduction of lastPermissionMode and lastPermissionModeUpdatedAt in the Session class, along with the mechanisms to update and propagate these values, provides a robust solution. The parsing of CLI arguments for initial permission mode and its publication to session metadata are also well-implemented. Overall, the changes enhance the consistency and user experience when interacting with different agent modes.
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ 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". |
73bb10c to
402bdcf
Compare
|
@codex @coderabbitai @gemini-code-assist please review the fixes we added/force-pushed to ensure they fixes the issues you reported, and that no new issue exists or was introduced |
|
@leeroybrun: I'll review the updated changes to verify that the previously reported issues have been fixed and check for any new issues that may have been introduced. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/utils/createSessionMetadata.ts (1)
13-14: Consider consolidating imports from the same module.Both imports are from
@/api/types. Merging them into a single import statement would be cleaner.Suggested consolidation
-import type { AgentState, Metadata } from '@/api/types'; -import type { PermissionMode } from '@/api/types'; +import type { AgentState, Metadata, PermissionMode } from '@/api/types';src/api/types.ts (1)
16-30: Consider deriving the type from the constant to avoid duplication.The
PermissionModetype andPERMISSION_MODESarray contain the same values, creating a maintenance burden where changes must be made in both places.Alternative pattern using `as const`
-export type PermissionMode = 'default' | 'acceptEdits' | 'bypassPermissions' | 'plan' | 'read-only' | 'safe-yolo' | 'yolo' - -export const PERMISSION_MODES: PermissionMode[] = [ +export const PERMISSION_MODES = [ 'default', 'acceptEdits', 'bypassPermissions', 'plan', 'read-only', 'safe-yolo', 'yolo', -] +] as const + +export type PermissionMode = (typeof PERMISSION_MODES)[number] export function isPermissionMode(value: string): value is PermissionMode { - return (PERMISSION_MODES as readonly string[]).includes(value) + return PERMISSION_MODES.includes(value as PermissionMode) }This pattern ensures the type and constant are always in sync.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/api/types.tssrc/codex/runCodex.tssrc/gemini/runGemini.tssrc/index.tssrc/utils/createSessionMetadata.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/codex/runCodex.ts
- src/gemini/runGemini.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}: Use descriptive variable and function names that clearly convey intent
Write clear, concise comments explaining the 'why' behind complex logic
Keep functions small and focused on a single responsibility
Use consistent code formatting and adhere to the project's style guide
Files:
src/utils/createSessionMetadata.tssrc/index.tssrc/api/types.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: No untyped code - use strict typing in TypeScript with explicit parameter and return types
Use comprehensive JSDoc comments - each file includes header comments explaining responsibilities
ALL imports must be at the top of the file - NEVER import modules mid-code
Use named exports preferred, with occasional default exports for main functions
Avoid creating stupid small functions, getters, or setters - minimize unnecessary function decomposition
Avoid excessive use ofifstatements - especially if you can avoid control flow changes with a better design
Files:
src/utils/createSessionMetadata.tssrc/index.tssrc/api/types.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Use@/alias for src imports, e.g.,import { logger } from '@/ui/logger'
Use graceful error handling with proper error messages, try-catch blocks with specific error logging, and abort controllers for cancellable operations
All debugging must be through file logs to avoid disturbing Claude sessions - console output only for user-facing messages
Handle large JSON objects with truncation in logs to avoid excessive output
Minimize use of classes - prefer functional approaches in TypeScript
Files:
src/utils/createSessionMetadata.tssrc/index.tssrc/api/types.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
.tsfile extensions for TypeScript files
Files:
src/utils/createSessionMetadata.tssrc/index.tssrc/api/types.ts
src/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/api/**/*.{ts,tsx}: Implement end-to-end encryption for all communications using TweetNaCl
Use Socket.IO for real-time messaging in API sessions
Implement optimistic concurrency control for state updates in sessions
Implement session isolation through unique session IDs
Files:
src/api/types.ts
src/api/types.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Zod schemas for type-safe API communication in types files
Files:
src/api/types.ts
🧠 Learnings (16)
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/api/**/*.{ts,tsx} : Implement optimistic concurrency control for state updates in sessions
Applied to files:
src/utils/createSessionMetadata.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/api/**/*.{ts,tsx} : Implement session isolation through unique session IDs
Applied to files:
src/utils/createSessionMetadata.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/*.{ts,tsx} : When daemon receives 'daemon start' command in src/index.ts, spawn a detached process via spawnHappyCLI(['daemon', 'start-sync'], { detached: true }) that calls startDaemon() from src/daemon/run.ts
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : When daemon receives RPC 'spawn-happy-session' from backend, create directory if needed, spawn detached Happy process with flags '--happy-starting-mode remote --started-by daemon', add to pidToTrackedSession map, and set up 10-second awaiter for session webhook
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Track spawned Happy processes by storing their PID and happySessionId in daemon.state.json for cleanup purposes; include process information for both daemon-spawned and user-spawned sessions
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : When version mismatch is detected during heartbeat, spawn a new daemon via spawnHappyCLI(['daemon', 'start']) and wait to be killed by the new daemon instance
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/doctor/**/*.{ts,tsx} : In 'happy doctor' command, use 'ps aux | grep' to find all Happy processes and categorize by command args: daemon, daemon-spawned, user-session, doctor; match production patterns 'happy.mjs', 'happy-coder', 'dist/index.mjs' and development pattern 'tsx.*src/index.ts'
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : During daemon startup, perform version check by reading daemon.state.json and comparing startedWithCliVersion with configuration.currentCliVersion; if version mismatch detected, call stopDaemon() to kill old daemon before proceeding
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/claude/watcher.{ts,tsx} : Implement file system watcher for Claude session files in interactive mode
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/**/*.{ts,tsx} : All debugging must be through file logs to avoid disturbing Claude sessions - console output only for user-facing messages
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/src/index.ts : In CLI startup phase, check if machine ID exists in settings; if not, create ID locally only without creating machine on server (daemon's responsibility on startup)
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Register RPC handlers for 'spawn-happy-session', 'stop-session', and 'requestShutdown' on the daemon's HTTP server and WebSocket connection
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Perform authentication via authAndSetupMachineIfNeeded() during daemon startup to ensure credentials exist before establishing backend connections
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Ensure caffeinate process is started by the daemon (not individual sessions) and tracked in daemon.state.json to prevent it from becoming a runaway process
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement notifyDaemonSessionStarted() webhook endpoint that receives POST requests to /session-started and creates TrackedSession entries with startedBy identifier
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement stopSession() to find session by happySessionId, send SIGTERM to process, and remove from tracking map on exit event
Applied to files:
src/index.ts
🧬 Code graph analysis (2)
src/utils/createSessionMetadata.ts (1)
src/api/types.ts (1)
PermissionMode(16-16)
src/index.ts (3)
src/api/types.ts (3)
PermissionMode(16-16)PERMISSION_MODES(18-26)isPermissionMode(28-30)src/codex/runCodex.ts (1)
runCodex(66-808)src/gemini/runGemini.ts (1)
runGemini(59-1130)
🔇 Additional comments (6)
src/utils/createSessionMetadata.ts (1)
90-93: LGTM!The conditional spreading logic correctly handles optional fields. The truthiness check for
permissionModeworks since all valid modes are non-empty strings, and thetypeofcheck forpermissionModeUpdatedAtcorrectly handles the case where0would be a valid (though unlikely) timestamp.src/api/types.ts (1)
342-350: LGTM!The metadata extension is well-documented with clear JSDoc comments explaining the purpose of each field. The types align correctly with the
PermissionModetype and the optional nature is appropriate for backward compatibility.src/index.ts (4)
37-74: LGTM!The
parseSessionStartArgshelper is well-structured with proper validation and helpful error messages. The loop correctly starts at index 1 to skip the subcommand (codex/gemini). The use ofisPermissionModetype guard ensures type safety.
129-134: LGTM!The permission mode is correctly parsed and passed to
runCodex. The integration aligns with the function signature that acceptspermissionMode?: PermissionMode.
241-260: LGTM!The permission mode handling for Gemini mirrors the Codex pattern correctly, ensuring consistent behavior across both agent types.
432-436: Note: Claude path handles permission mode differently than Codex/Gemini.The Claude path converts
--yoloto--dangerously-skip-permissionsand passes it through to the Claude subprocess, whereas Codex/Gemini extractpermissionModefor session metadata. This asymmetry is intentional per the PR design—Claude's permission mode is inferred and synchronized viarunClaudeand the permission handler layer rather than at CLI parsing time.
![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: 402bdcfc6e
ℹ️ 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".
b429fc7 to
0f1d139
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils/createSessionMetadata.ts (1)
89-91: Truthy check onpermissionModemay exclude valid falsy values in the future.Currently safe since
PermissionModevalues are non-empty strings like'default','yolo', etc. However, consider using an explicitundefinedcheck for consistency with the timestamp check on line 91:- ...(opts.permissionMode && { permissionMode: opts.permissionMode }), + ...(opts.permissionMode !== undefined && { permissionMode: opts.permissionMode }),This makes intent clearer and guards against future type changes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/api/types.tssrc/codex/runCodex.tssrc/gemini/runGemini.tssrc/index.tssrc/utils/createSessionMetadata.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/codex/runCodex.ts
- src/api/types.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}: Use descriptive variable and function names that clearly convey intent
Write clear, concise comments explaining the 'why' behind complex logic
Keep functions small and focused on a single responsibility
Use consistent code formatting and adhere to the project's style guide
Files:
src/gemini/runGemini.tssrc/index.tssrc/utils/createSessionMetadata.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: No untyped code - use strict typing in TypeScript with explicit parameter and return types
Use comprehensive JSDoc comments - each file includes header comments explaining responsibilities
ALL imports must be at the top of the file - NEVER import modules mid-code
Use named exports preferred, with occasional default exports for main functions
Avoid creating stupid small functions, getters, or setters - minimize unnecessary function decomposition
Avoid excessive use ofifstatements - especially if you can avoid control flow changes with a better design
Files:
src/gemini/runGemini.tssrc/index.tssrc/utils/createSessionMetadata.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Use@/alias for src imports, e.g.,import { logger } from '@/ui/logger'
Use graceful error handling with proper error messages, try-catch blocks with specific error logging, and abort controllers for cancellable operations
All debugging must be through file logs to avoid disturbing Claude sessions - console output only for user-facing messages
Handle large JSON objects with truncation in logs to avoid excessive output
Minimize use of classes - prefer functional approaches in TypeScript
Files:
src/gemini/runGemini.tssrc/index.tssrc/utils/createSessionMetadata.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
.tsfile extensions for TypeScript files
Files:
src/gemini/runGemini.tssrc/index.tssrc/utils/createSessionMetadata.ts
🧠 Learnings (13)
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/*.{ts,tsx} : When daemon receives 'daemon start' command in src/index.ts, spawn a detached process via spawnHappyCLI(['daemon', 'start-sync'], { detached: true }) that calls startDaemon() from src/daemon/run.ts
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : When daemon receives RPC 'spawn-happy-session' from backend, create directory if needed, spawn detached Happy process with flags '--happy-starting-mode remote --started-by daemon', add to pidToTrackedSession map, and set up 10-second awaiter for session webhook
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : When version mismatch is detected during heartbeat, spawn a new daemon via spawnHappyCLI(['daemon', 'start']) and wait to be killed by the new daemon instance
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Track spawned Happy processes by storing their PID and happySessionId in daemon.state.json for cleanup purposes; include process information for both daemon-spawned and user-spawned sessions
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/doctor/**/*.{ts,tsx} : In 'happy doctor' command, use 'ps aux | grep' to find all Happy processes and categorize by command args: daemon, daemon-spawned, user-session, doctor; match production patterns 'happy.mjs', 'happy-coder', 'dist/index.mjs' and development pattern 'tsx.*src/index.ts'
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : During daemon startup, perform version check by reading daemon.state.json and comparing startedWithCliVersion with configuration.currentCliVersion; if version mismatch detected, call stopDaemon() to kill old daemon before proceeding
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/src/index.ts : In CLI startup phase, check if machine ID exists in settings; if not, create ID locally only without creating machine on server (daemon's responsibility on startup)
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Register RPC handlers for 'spawn-happy-session', 'stop-session', and 'requestShutdown' on the daemon's HTTP server and WebSocket connection
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Perform authentication via authAndSetupMachineIfNeeded() during daemon startup to ensure credentials exist before establishing backend connections
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Ensure caffeinate process is started by the daemon (not individual sessions) and tracked in daemon.state.json to prevent it from becoming a runaway process
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement notifyDaemonSessionStarted() webhook endpoint that receives POST requests to /session-started and creates TrackedSession entries with startedBy identifier
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement stopSession() to find session by happySessionId, send SIGTERM to process, and remove from tracking map on exit event
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/api/**/*.{ts,tsx} : Implement optimistic concurrency control for state updates in sessions
Applied to files:
src/utils/createSessionMetadata.ts
🧬 Code graph analysis (2)
src/gemini/runGemini.ts (2)
src/api/types.ts (4)
PermissionMode(26-26)isGeminiPermissionMode(41-43)GEMINI_PERMISSION_MODES(32-37)GeminiPermissionMode(39-39)src/utils/createSessionMetadata.ts (1)
createSessionMetadata(69-95)
src/utils/createSessionMetadata.ts (1)
src/api/types.ts (1)
PermissionMode(26-26)
🔇 Additional comments (12)
src/utils/createSessionMetadata.ts (2)
13-13: LGTM!The
PermissionModetype import aligns with the new optional fields in the metadata options.
33-36: LGTM!The optional fields are well-documented with JSDoc comments explaining their purpose, including the timestamp's role in cross-device arbitration.
src/gemini/runGemini.ts (5)
41-41: LGTM!The imports are correctly structured, bringing in both the runtime constants (
GEMINI_PERMISSION_MODES) for validation and the type definitions (GeminiPermissionMode,PermissionMode) for type safety.
59-63: LGTM!The function signature is properly extended with an optional
permissionModeparameter typed asPermissionMode, allowing the CLI to pass through the parsed permission mode.
111-122: LGTM!Good defensive initialization pattern:
- Validates
opts.permissionModeusingisGeminiPermissionModetype guard- Falls back to
'default'for invalid/missing values- Seeds session metadata with both
permissionModeandpermissionModeUpdatedAtat creation timeThis ensures the app can display the correct permission mode state immediately.
167-167: LGTM!Initializing
currentPermissionModefrominitialPermissionModeensures consistent in-memory state from the start, rather than leaving it undefined until the first user message.
174-184: LGTM!The permission mode update flow is well-implemented:
- Validates incoming mode against
GEMINI_PERMISSION_MODESbefore applying- Updates in-memory state, permission handler, and session metadata atomically
- Uses functional updater pattern for
updateMetadatawhich aligns with optimistic concurrency best practicesBased on learnings, this follows the recommended pattern for state updates in sessions.
src/index.ts (5)
31-31: LGTM!The imports are correctly organized, bringing in both runtime constants for validation and the type for type safety.
37-74: LGTM!The
parseSessionStartArgshelper consolidates argument parsing for--started-by,--permission-mode, and--yoloin a reusable function. Key points:
- Loop starts at index 1, correctly skipping the subcommand
- Uses
++ito consume option values, preventing double-processing--yolois correctly treated as a flag (no value expected)- Error messages include valid values for user guidance
- If both
--permission-modeand--yoloare provided, last one wins (standard CLI behavior)
129-134: LGTM!The Codex startup flow correctly uses the shared
parseSessionStartArgshelper and threadspermissionModetorunCodex.
241-246: LGTM!The Gemini startup includes additional validation using
isGeminiPermissionModesince Gemini supports a subset of permission modes. The error message helpfully suggests--yoloas an alternative and lists valid Gemini-specific modes.
265-265: LGTM!The
permissionModeis correctly passed torunGeminiafter validation.
0f1d139 to
f59f882
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/api/types.ts (1)
266-275: Consider derivingpermissionModeenum fromPERMISSION_MODESconstant.The
MessageMetaSchema.permissionModeuses a hardcodedz.enum([...])that duplicates the values defined inPERMISSION_MODES. This creates a maintenance risk where the constant and schema could drift apart.♻️ Suggested refactor
export const MessageMetaSchema = z.object({ sentFrom: z.string().optional(), // Source identifier - permissionMode: z.enum(['default', 'acceptEdits', 'bypassPermissions', 'plan', 'read-only', 'safe-yolo', 'yolo']).optional(), // Permission mode for this message + permissionMode: z.enum(PERMISSION_MODES).optional(), // Permission mode for this message model: z.string().nullable().optional(), // Model name for this message (null = reset)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/api/types.tssrc/codex/runCodex.tssrc/gemini/runGemini.tssrc/index.tssrc/utils/createSessionMetadata.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/codex/runCodex.ts
- src/utils/createSessionMetadata.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}: Use descriptive variable and function names that clearly convey intent
Write clear, concise comments explaining the 'why' behind complex logic
Keep functions small and focused on a single responsibility
Use consistent code formatting and adhere to the project's style guide
Files:
src/gemini/runGemini.tssrc/index.tssrc/api/types.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: No untyped code - use strict typing in TypeScript with explicit parameter and return types
Use comprehensive JSDoc comments - each file includes header comments explaining responsibilities
ALL imports must be at the top of the file - NEVER import modules mid-code
Use named exports preferred, with occasional default exports for main functions
Avoid creating stupid small functions, getters, or setters - minimize unnecessary function decomposition
Avoid excessive use ofifstatements - especially if you can avoid control flow changes with a better design
Files:
src/gemini/runGemini.tssrc/index.tssrc/api/types.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Use@/alias for src imports, e.g.,import { logger } from '@/ui/logger'
Use graceful error handling with proper error messages, try-catch blocks with specific error logging, and abort controllers for cancellable operations
All debugging must be through file logs to avoid disturbing Claude sessions - console output only for user-facing messages
Handle large JSON objects with truncation in logs to avoid excessive output
Minimize use of classes - prefer functional approaches in TypeScript
Files:
src/gemini/runGemini.tssrc/index.tssrc/api/types.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
.tsfile extensions for TypeScript files
Files:
src/gemini/runGemini.tssrc/index.tssrc/api/types.ts
src/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/api/**/*.{ts,tsx}: Implement end-to-end encryption for all communications using TweetNaCl
Use Socket.IO for real-time messaging in API sessions
Implement optimistic concurrency control for state updates in sessions
Implement session isolation through unique session IDs
Files:
src/api/types.ts
src/api/types.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Zod schemas for type-safe API communication in types files
Files:
src/api/types.ts
🧠 Learnings (12)
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/*.{ts,tsx} : When daemon receives 'daemon start' command in src/index.ts, spawn a detached process via spawnHappyCLI(['daemon', 'start-sync'], { detached: true }) that calls startDaemon() from src/daemon/run.ts
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : When daemon receives RPC 'spawn-happy-session' from backend, create directory if needed, spawn detached Happy process with flags '--happy-starting-mode remote --started-by daemon', add to pidToTrackedSession map, and set up 10-second awaiter for session webhook
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : When version mismatch is detected during heartbeat, spawn a new daemon via spawnHappyCLI(['daemon', 'start']) and wait to be killed by the new daemon instance
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Track spawned Happy processes by storing their PID and happySessionId in daemon.state.json for cleanup purposes; include process information for both daemon-spawned and user-spawned sessions
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/doctor/**/*.{ts,tsx} : In 'happy doctor' command, use 'ps aux | grep' to find all Happy processes and categorize by command args: daemon, daemon-spawned, user-session, doctor; match production patterns 'happy.mjs', 'happy-coder', 'dist/index.mjs' and development pattern 'tsx.*src/index.ts'
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : During daemon startup, perform version check by reading daemon.state.json and comparing startedWithCliVersion with configuration.currentCliVersion; if version mismatch detected, call stopDaemon() to kill old daemon before proceeding
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/src/index.ts : In CLI startup phase, check if machine ID exists in settings; if not, create ID locally only without creating machine on server (daemon's responsibility on startup)
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Register RPC handlers for 'spawn-happy-session', 'stop-session', and 'requestShutdown' on the daemon's HTTP server and WebSocket connection
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Perform authentication via authAndSetupMachineIfNeeded() during daemon startup to ensure credentials exist before establishing backend connections
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Ensure caffeinate process is started by the daemon (not individual sessions) and tracked in daemon.state.json to prevent it from becoming a runaway process
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement notifyDaemonSessionStarted() webhook endpoint that receives POST requests to /session-started and creates TrackedSession entries with startedBy identifier
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement stopSession() to find session by happySessionId, send SIGTERM to process, and remove from tracking map on exit event
Applied to files:
src/index.ts
🧬 Code graph analysis (3)
src/gemini/runGemini.ts (2)
src/api/types.ts (4)
PermissionMode(29-29)isGeminiPermissionMode(37-39)GEMINI_PERMISSION_MODES(17-17)GeminiPermissionMode(35-35)src/utils/createSessionMetadata.ts (1)
createSessionMetadata(69-95)
src/index.ts (3)
src/api/types.ts (5)
PermissionMode(29-29)PERMISSION_MODES(23-27)isPermissionMode(31-33)isGeminiPermissionMode(37-39)GEMINI_PERMISSION_MODES(17-17)src/codex/runCodex.ts (1)
runCodex(66-808)src/gemini/runGemini.ts (1)
runGemini(59-1129)
src/api/types.ts (1)
src/claude/loop.ts (1)
PermissionMode(12-12)
🔇 Additional comments (10)
src/api/types.ts (2)
16-39: LGTM! Permission mode constants and type guards are well-structured.The derived types from
as constarrays and the type guard functions provide good type safety. The separation between Gemini-specific and Claude-only modes is clear.
351-358: LGTM! Metadata fields for permission mode tracking.The
permissionModeandpermissionModeUpdatedAtfields are correctly typed and documented. The timestamp field enables "latest wins" arbitration as noted in the JSDoc.src/gemini/runGemini.ts (4)
41-41: LGTM! Imports are correctly structured.The permission mode types are properly imported using the
@/alias as required by coding guidelines.
111-122: LGTM! Permission mode initialization and metadata propagation.The code correctly validates the incoming permission mode against Gemini-supported modes, defaults to
'default'if invalid, and propagates both the mode and timestamp to session metadata.
167-167: LGTM!currentPermissionModeproperly initialized.Initializing from
initialPermissionModeensures the runtime state matches the session metadata from the start.
170-190: LGTM! User message permission mode handling is correct.The validation against
GEMINI_PERMISSION_MODESand the metadata update with timestamp ensure proper synchronization between runtime state and persisted session metadata.src/index.ts (4)
31-31: LGTM! Imports are properly structured.All required permission mode types and guards are imported using the
@/alias.
37-74: LGTM! Clean argument parsing helper.The
parseSessionStartArgsfunction correctly:
- Skips the subcommand at
args[0]by starting ati = 1- Validates
--started-byvalues- Validates
--permission-modeusing the type guard- Handles
--yoloas a shortcut foryolopermission mode- Provides clear error messages with valid values
129-134: LGTM! Codex permission mode propagation.The parsed
permissionModeis correctly passed through torunCodex.
241-265: LGTM! Gemini-specific validation and propagation.The additional validation with
isGeminiPermissionModecorrectly restricts Gemini to its supported permission modes and provides a helpful hint about--yolo. The validated mode is then passed torunGemini.
f59f882 to
cea2385
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/api/types.ts (1)
268-268: Hardcoded enum duplicates PERMISSION_MODES constant.The
permissionModeenum values inMessageMetaSchemaare hardcoded rather than derived from thePERMISSION_MODESconstant. This creates a maintenance risk where the two could drift apart.Consider using
z.enum(PERMISSION_MODES)to keep them in sync:♻️ Suggested refactor
- permissionMode: z.enum(['default', 'acceptEdits', 'bypassPermissions', 'plan', 'read-only', 'safe-yolo', 'yolo']).optional(), // Permission mode for this message + permissionMode: z.enum(PERMISSION_MODES).optional(), // Permission mode for this messagesrc/index.ts (1)
129-134: Consider validating Codex permission modes.Codex passes
permissionModewithout validating it's a valid Codex/Gemini mode. If a user passes--permission-mode bypassPermissions(Claude-only), it will be accepted here but may cause issues downstream.For consistency with the Gemini validation at lines 242-246, consider adding validation:
♻️ Suggested validation
const { startedBy, permissionMode } = parseSessionStartArgs() + if (permissionMode && !isCodexGeminiPermissionMode(permissionMode)) { + console.error(chalk.red(`Invalid --permission-mode for codex: ${permissionMode}. Valid values: ${CODEX_GEMINI_PERMISSION_MODES.join(', ')}`)) + console.error(chalk.gray('Tip: use --yolo for full bypass-like behavior.')) + process.exit(1) + } const { credentials } = await authAndSetupMachineIfNeeded(); await runCodex({credentials, startedBy, permissionMode});
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/api/types.tssrc/codex/runCodex.tssrc/gemini/runGemini.tssrc/index.tssrc/utils/createSessionMetadata.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/codex/runCodex.ts
- src/gemini/runGemini.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}: Use descriptive variable and function names that clearly convey intent
Write clear, concise comments explaining the 'why' behind complex logic
Keep functions small and focused on a single responsibility
Use consistent code formatting and adhere to the project's style guide
Files:
src/index.tssrc/api/types.tssrc/utils/createSessionMetadata.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: No untyped code - use strict typing in TypeScript with explicit parameter and return types
Use comprehensive JSDoc comments - each file includes header comments explaining responsibilities
ALL imports must be at the top of the file - NEVER import modules mid-code
Use named exports preferred, with occasional default exports for main functions
Avoid creating stupid small functions, getters, or setters - minimize unnecessary function decomposition
Avoid excessive use ofifstatements - especially if you can avoid control flow changes with a better design
Files:
src/index.tssrc/api/types.tssrc/utils/createSessionMetadata.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Use@/alias for src imports, e.g.,import { logger } from '@/ui/logger'
Use graceful error handling with proper error messages, try-catch blocks with specific error logging, and abort controllers for cancellable operations
All debugging must be through file logs to avoid disturbing Claude sessions - console output only for user-facing messages
Handle large JSON objects with truncation in logs to avoid excessive output
Minimize use of classes - prefer functional approaches in TypeScript
Files:
src/index.tssrc/api/types.tssrc/utils/createSessionMetadata.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
.tsfile extensions for TypeScript files
Files:
src/index.tssrc/api/types.tssrc/utils/createSessionMetadata.ts
src/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/api/**/*.{ts,tsx}: Implement end-to-end encryption for all communications using TweetNaCl
Use Socket.IO for real-time messaging in API sessions
Implement optimistic concurrency control for state updates in sessions
Implement session isolation through unique session IDs
Files:
src/api/types.ts
src/api/types.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Zod schemas for type-safe API communication in types files
Files:
src/api/types.ts
🧠 Learnings (14)
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Perform authentication via authAndSetupMachineIfNeeded() during daemon startup to ensure credentials exist before establishing backend connections
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/*.{ts,tsx} : When daemon receives 'daemon start' command in src/index.ts, spawn a detached process via spawnHappyCLI(['daemon', 'start-sync'], { detached: true }) that calls startDaemon() from src/daemon/run.ts
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : When daemon receives RPC 'spawn-happy-session' from backend, create directory if needed, spawn detached Happy process with flags '--happy-starting-mode remote --started-by daemon', add to pidToTrackedSession map, and set up 10-second awaiter for session webhook
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : When version mismatch is detected during heartbeat, spawn a new daemon via spawnHappyCLI(['daemon', 'start']) and wait to be killed by the new daemon instance
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Track spawned Happy processes by storing their PID and happySessionId in daemon.state.json for cleanup purposes; include process information for both daemon-spawned and user-spawned sessions
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/doctor/**/*.{ts,tsx} : In 'happy doctor' command, use 'ps aux | grep' to find all Happy processes and categorize by command args: daemon, daemon-spawned, user-session, doctor; match production patterns 'happy.mjs', 'happy-coder', 'dist/index.mjs' and development pattern 'tsx.*src/index.ts'
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : During daemon startup, perform version check by reading daemon.state.json and comparing startedWithCliVersion with configuration.currentCliVersion; if version mismatch detected, call stopDaemon() to kill old daemon before proceeding
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/src/index.ts : In CLI startup phase, check if machine ID exists in settings; if not, create ID locally only without creating machine on server (daemon's responsibility on startup)
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Register RPC handlers for 'spawn-happy-session', 'stop-session', and 'requestShutdown' on the daemon's HTTP server and WebSocket connection
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/doctor/**/*.{ts,tsx} : Implement 'happy doctor clean' command that calls findRunawayHappyProcesses() to filter likely orphans, then killRunawayHappyProcesses() to send SIGTERM, wait 1 second, and send SIGKILL if still alive
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Ensure caffeinate process is started by the daemon (not individual sessions) and tracked in daemon.state.json to prevent it from becoming a runaway process
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement notifyDaemonSessionStarted() webhook endpoint that receives POST requests to /session-started and creates TrackedSession entries with startedBy identifier
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement stopSession() to find session by happySessionId, send SIGTERM to process, and remove from tracking map on exit event
Applied to files:
src/index.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/api/**/*.{ts,tsx} : Implement optimistic concurrency control for state updates in sessions
Applied to files:
src/utils/createSessionMetadata.ts
🧬 Code graph analysis (3)
src/index.ts (3)
src/api/types.ts (5)
PermissionMode(29-29)PERMISSION_MODES(23-27)isPermissionMode(31-33)isCodexGeminiPermissionMode(37-39)CODEX_GEMINI_PERMISSION_MODES(17-17)src/codex/runCodex.ts (1)
runCodex(66-808)src/gemini/runGemini.ts (1)
runGemini(59-1129)
src/api/types.ts (1)
src/claude/loop.ts (1)
PermissionMode(12-12)
src/utils/createSessionMetadata.ts (1)
src/api/types.ts (1)
PermissionMode(29-29)
🔇 Additional comments (7)
src/api/types.ts (2)
16-39: Well-structured permission mode constants and type guards.The layered composition of permission modes (Claude-only, Codex/Gemini, combined) with derived types and type guards is clean and maintainable. The
as constassertions enable proper type inference.
351-358: LGTM - new Metadata fields for permission mode tracking.The
permissionModeandpermissionModeUpdatedAtfields with JSDoc comments clearly document their purpose for cross-device arbitration and UI state seeding. Using a timestamp for "latest wins" is a sensible approach for eventual consistency.src/utils/createSessionMetadata.ts (2)
33-36: LGTM - well-documented optional fields.The new
permissionModeandpermissionModeUpdatedAtoptions are properly typed and documented with JSDoc comments explaining their purpose for cross-device arbitration.
89-91: Clean conditional spreading pattern.The conditional inclusion of
permissionModeandpermissionModeUpdatedAtis idiomatic. Thetypeof === 'number'check for the timestamp correctly handles edge cases like0orNaN.src/index.ts (3)
37-74: Well-structured argument parsing helper.The
parseSessionStartArgsfunction cleanly handles--started-by,--permission-mode, and--yoloflags with proper validation and error messages. Starting the loop at index 1 correctly skips the subcommand.
241-246: Good validation with helpful error message.The Gemini path properly validates that the permission mode is compatible with Codex/Gemini using
isCodexGeminiPermissionMode. The hint about--yoloimproves UX for users who want bypass behavior.
265-265: LGTM - permission mode propagation.The validated
permissionModeis correctly passed through torunGemini.
cea2385 to
1c6d683
Compare
|
@codex @coderabbitai @gemini-code-assist please review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@leeroybrun: I'll review the changes in this pull request. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request does a great job of adding persistence for permission modes across local and remote sessions, and extending this functionality to Codex and Gemini agents. The changes are well-structured, particularly the refactoring of permission mode types and constants in src/api/types.ts and the centralized argument parsing in src/index.ts.
I've identified a few areas for improvement:
- A potential bug in argument parsing within
claudeLocalLauncher.tsthat could lead to incorrect behavior. - Opportunities to make argument parsing in
index.tsmore robust against user error. - A minor refactoring suggestion in
runClaude.tsto improve maintainability by removing a hardcoded list.
Overall, these are solid changes that address an important usability issue.
| if (i + 1 < input.length) { | ||
| i++; | ||
| } |
There was a problem hiding this comment.
There's a potential bug here. If arg is --permission-mode and it's either the last argument or the next argument is another flag (e.g., ['--permission-mode', '--some-other-flag']), this logic will incorrectly skip the next argument.
You should only skip the next argument if it's a value associated with --permission-mode, not another flag. A check like !input[i + 1].startsWith('-') would fix this, similar to what's done in inferPermissionModeFromClaudeArgs.
| if (i + 1 < input.length) { | |
| i++; | |
| } | |
| if (i + 1 < input.length && !input[i + 1].startsWith('-')) { | |
| i++; | |
| } |
| if (next === 'default' || next === 'acceptEdits' || next === 'bypassPermissions' || next === 'plan') { | ||
| inferred = next as PermissionMode; | ||
| } |
There was a problem hiding this comment.
To avoid hardcoding the list of valid permission modes and improve maintainability, you could define a constant for them and use .includes() for the check. This aligns with the approach taken for other permission mode sets in this PR.
| if (next === 'default' || next === 'acceptEdits' || next === 'bypassPermissions' || next === 'plan') { | |
| inferred = next as PermissionMode; | |
| } | |
| const validClaudeModes = ['default', 'acceptEdits', 'bypassPermissions', 'plan']; | |
| if (validClaudeModes.includes(next)) { | |
| inferred = next as PermissionMode; | |
| } |
| if (i + 1 >= args.length) { | ||
| console.error(chalk.red('Missing value for --started-by (expected: daemon|terminal)')) | ||
| process.exit(1) | ||
| } |
There was a problem hiding this comment.
The argument parsing for --started-by could be more robust. If a user provides another flag immediately after it (e.g., happy codex --started-by --yolo), the next flag (--yolo) will be incorrectly interpreted as the value, leading to a validation error.
You should also check if the next argument is a flag itself to provide a clearer error message.
if (i + 1 >= args.length || args[i + 1].startsWith('-')) {
console.error(chalk.red('Missing value for --started-by (expected: daemon|terminal)'))
process.exit(1)
}| if (i + 1 >= args.length) { | ||
| console.error(chalk.red(`Missing value for --permission-mode. Valid values: ${PERMISSION_MODES.join(', ')}`)) | ||
| process.exit(1) | ||
| } |
There was a problem hiding this comment.
Similar to the --started-by flag, the argument parsing for --permission-mode could be more robust. If a user provides another flag immediately after it (e.g., happy codex --permission-mode --yolo), the next flag (--yolo) will be incorrectly interpreted as the value, leading to a validation error.
You should also check if the next argument is a flag itself to provide a clearer error message.
if (i + 1 >= args.length || args[i + 1].startsWith('-')) {
console.error(chalk.red(`Missing value for --permission-mode. Valid values: ${PERMISSION_MODES.join(', ')}`))
process.exit(1)
}
Summary
Problem
Permission mode was lost when switching between remote and local modes:
--dangerously-skip-permissions) was not reflected in the appThis caused users to have to re-enable YOLO mode repeatedly when switching between mobile control and terminal usage.
Solution
Track permission mode in Session: Add
lastPermissionModeandlastPermissionModeUpdatedAtto the Session class.Publish to metadata: When permission mode changes (via message or permission response), publish to encrypted session metadata so the app can read it immediately.
Pass to local Claude: When switching remote → local, dynamically inject
--permission-modeargs to the spawned Claude process viaupsertClaudePermissionModeArgs().Infer from CLI args: Parse initial permission mode from CLI args (e.g.,
--dangerously-skip-permissions) and publish to metadata on session creation.Support Codex/Gemini: Publish and update permission mode in metadata for non-Claude agents.
Changes
src/claude/session.ts: AddlastPermissionMode,setLastPermissionMode()method that publishes to metadatasrc/claude/claudeLocalLauncher.ts: AddupsertClaudePermissionModeArgs()to inject permission mode when spawning local Claudesrc/claude/utils/permissionHandler.ts: Callsession.setLastPermissionMode()on mode changessrc/claude/runClaude.ts: Infer permission mode from CLI args, publish to metadata on session creationsrc/claude/loop.ts: Set initial permission mode on session startsrc/api/types.ts: AddpermissionModeandpermissionModeUpdatedAtto Metadata typesrc/codex/runCodex.ts,src/gemini/runGemini.ts: Publish permission mode to metadatasrc/utils/createSessionMetadata.ts: Support permission mode in session metadata creationsrc/index.ts: Accept--permission-modeand--yoloflags for codex/gemini commandsTesting
--dangerously-skip-permissions, open app → App should show "bypass" immediately--yolo, check app → Should show correct modeFixes
Related
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.