Skip to content

feat: add rollout-backed Codex task status - #73

Open
bbnotcode wants to merge 9 commits into
ericjypark:mainfrom
bbnotcode:codex/task-status-split
Open

bbnotcode wants to merge 9 commits into
ericjypark:mainfrom
bbnotcode:codex/task-status-split

Conversation

@bbnotcode

@bbnotcode bbnotcode commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

English

Why

Some CodexIsland users subscribe to Codex without using Claude. When the Claude provider is hidden, the left half of the expanded island can provide a useful, privacy-preserving signal instead of remaining empty: whether a Codex task is running, waiting for approval, idle, cancelled, failed, or unavailable.

What changed

  • Infers top-level Codex task state from local rollout JSONL lifecycle events without reading or displaying prompt contents.
  • Adds balanced compact and expanded status layouts, icon-only or localized icon-and-text display modes, and click-through to Codex when possible.
  • Adds event-specific sounds: Glass for completion, Ping for approval, Funk for cancellation, and Basso for errors.
  • Emits one completion notification per completed top-level task, including short and parallel tasks completed between polling passes.
  • Excludes guardian/subagent rollout files so internal Codex work cannot produce false completion alerts.
  • Preserves long-running and terminal failure state across bounded tail reads, partial JSONL records, cache continuation, and temporary unavailable scans.
  • Prevents stale scans and queued sounds from leaking across provider or settings changes.
  • Filters subagents before applying the 24-file tracking limit, so internal sessions cannot crowd out user tasks.
  • Documents behavior and limitations in English and Simplified Chinese.

Privacy and limitations

Only lifecycle metadata is parsed. Prompts, commands, tool output, and assistant text are never shown. Approval waits are detected from unresolved request_permissions calls. The current rollout format does not expose a reliable user-input-wait event, so this PR deliberately does not claim that state.

Validation

  • Full scripts/run-tests.sh suite passes.
  • Added regression coverage for bounded tail parsing, failures outside the tail window, partial records, short and parallel tasks, per-event sounds, approval resolution, subagent filtering, and file-limit ordering.
  • Swift type-check and localization plist validation pass.

中文

设计原因

有些 CodexIsland 用户只订阅 Codex,并不同时使用 Claude。当 Claude 被隐藏时,展开界面左半部分不必一直空置,可以在不显示任务具体内容的前提下,提供 Codex 任务是否正在运行、等待审批、空闲、已取消、报错或不可用的状态信号。

修改内容

  • 从本地 rollout JSONL 生命周期事件推断顶层 Codex 任务状态,不读取或显示提示词具体内容。
  • 增加对称的收起/展开状态布局,支持纯图标和本地化图标+文字模式,并在可行时点击跳转 Codex。
  • 为不同事件使用不同提示音:完成 Glass、等待审批 Ping、取消 Funk、报错 Basso。
  • 每个顶层任务完成时单独提醒,包括两次轮询之间快速完成的短任务和并行任务。
  • 排除 guardian/子任务日志,避免内部任务在主任务尚未完成时误播放完成音。
  • 在有界尾部读取、JSONL 半行、缓存续读和临时不可用时,保留长时间运行和终止错误状态。
  • 防止旧扫描结果和排队提示音跨设置/提供商切换继续生效。
  • 先排除子任务,再应用 24 个日志文件限制,避免子任务挤掉真实用户任务。
  • 在英文和简体中文 README 中记录行为和限制。

隐私与限制

仅解析生命周期元数据,不显示提示词、命令、工具输出或助手回复。等待审批通过尚未解决的 request_permissions 调用识别。当前 rollout 格式没有可靠的“等待用户输入”事件,因此本 PR 不声称能够识别该状态。

验证

  • 完整 scripts/run-tests.sh 测试通过。
  • 新增有界尾部读取、尾窗口外错误、半行记录、短任务、并行任务、逐事件声音、审批恢复、子任务过滤和文件限制顺序的回归测试。
  • Swift 类型检查和本地化 plist 验证通过。

Summary by CodeRabbit

  • New Features

    • Added optional Codex task-status monitoring when Codex is visible and Claude is hidden.
    • Displays running, approval-required, idle, cancelled, error, or unavailable states with elapsed and update times.
    • Added icon-only or labeled display modes and configurable completion, cancellation, and failure sounds.
    • Expanded status entries can open the related Codex task.
  • Documentation

    • Updated English and Simplified Chinese settings documentation and localized status messaging.
  • Bug Fixes

    • Restored per-model token usage when Codex task status is disabled.

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This change adds optional local Codex task-status monitoring. It parses rollout logs, publishes prioritized states, supports conditional polling and sounds, adds status views and settings, updates localization and documentation, and adds parser tests.

Changes

Codex task status

Layer / File(s) Summary
Rollout log parsing
Sources/Model/CodexTaskStatusLogParser.swift, Tests/CodexTaskStatusLogParserTests.swift, scripts/run-tests.sh
Parses incremental Codex JSONL lifecycle events, preserves cached state, prioritizes statuses, detects sound events, and validates behavior with executable tests.
Status store and polling
Sources/Model/CodexTaskStatusStore.swift, Sources/App.swift
Adds persisted preferences, conditional polling, rollout discovery, snapshot selection, task opening, and completion or attention sounds.
Status views and settings
Sources/Views/CodexTaskStatusView.swift, Sources/Views/IslandRootView.swift, Sources/Views/PanelHeader.swift, Sources/Views/SettingsView.swift, Sources/Views/UsageView.swift
Adds Codex status settings and displays status information in usage, header, expanded, and compact views.
Documentation and localization
README.md, README.zh-CN.md, Resources/en.lproj/Localizable.strings, Resources/zh-Hans.lproj/Localizable.strings
Documents task-status behavior and adds English and Simplified Chinese strings.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

Sequence Diagram(s)

sequenceDiagram
  participant App
  participant CodexTaskStatusStore
  participant CodexTaskStatusLogParser
  participant CodexTaskStatusView
  App->>CodexTaskStatusStore: start status monitoring
  CodexTaskStatusStore->>CodexTaskStatusLogParser: parse rollout JSONL files
  CodexTaskStatusLogParser-->>CodexTaskStatusStore: return status snapshot
  CodexTaskStatusStore-->>CodexTaskStatusView: publish status
  CodexTaskStatusView->>CodexTaskStatusStore: open Codex thread
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding rollout-backed Codex task status support.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@bbnotcode

Copy link
Copy Markdown
Contributor Author

Added two focused follow-ups:

  • Optional task-status sounds (default off): a light completion sound for running → idle, and an attention sound for running → error/cancelled.
  • Long-running log stability: when log growth exceeds the bounded tail window, the parser preserves the last trusted running state instead of reporting unavailable.
  • The sound tracker survives a temporary unavailable read, so a later completion/error still produces the expected sound without making unavailable itself noisy.
  • Idle compact elapsed time is fixed at 0s and no longer schedules periodic elapsed-time updates.

Validation:

  • 20 task-status parser/policy regression tests pass, including large growth gaps and running → unavailable → idle sound delivery.
  • EN/ZH localization plists pass.
  • Swift sources compile for both architectures. Final ad-hoc Sparkle signing in this isolated worktree still hits the resource-fork issue addressed separately by fix: clear Sparkle xattrs without following symlinks #72, which is intentionally not mixed into this PR.

@bbnotcode
bbnotcode marked this pull request as ready for review August 7, 2026 09:17

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (3)
Tests/CodexTaskStatusLogParserTests.swift (1)

43-51: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Reuse one FileHandle for the growth loop.

Data.append(to:) opens and closes a file handle on each iteration. The loop runs about 2900 times to reach 530 KiB. Open the handle once outside the loop to reduce test runtime.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Tests/CodexTaskStatusLogParserTests.swift` around lines 43 - 51, Update the
growth loop in the test around CodexTaskStatusLogParser.parse to open one
writable FileHandle before iterating, reuse it for each update append, and close
it after the loop completes. Preserve the existing data growth and periodic
parser invocation behavior.
Sources/Model/CodexTaskStatusStore.swift (2)

171-183: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Reset refreshInFlight with defer.

refreshInFlight is cleared only on the success path at line 182. The early return at line 177 leaves the flag set. Any future error path or cancellation inside this Task also leaves it set, and polling then stops permanently because the guard at line 170 always fails. A defer makes the reset unconditional.

♻️ Proposed change
         refreshInFlight = true
         let previousFingerprint = lastScanFingerprint
         Task { [weak self] in
             let result = await Task.detached(priority: .utility) {
                 Self.scan(previousFingerprint: previousFingerprint)
             }.value
             guard let self else { return }
+            defer { self.refreshInFlight = false }
             self.lastScanFingerprint = result.fingerprint
             if let snapshot = result.snapshot {
                 self.apply(snapshot)
             }
-            self.refreshInFlight = false
         }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Sources/Model/CodexTaskStatusStore.swift` around lines 171 - 183, Update the
Task closure in the refresh flow to reset refreshInFlight with defer immediately
after entering the closure, before any await or early return. Remove the
success-path reset, while preserving the existing weak-self guard and snapshot
application behavior.

297-334: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Consider limiting the day-directory walk between polls.

The loop enumerates 31 day directories on every 15 s poll. The fingerprint gate at line 239 runs after this work, so the enumeration cost is paid even when nothing changed. The 24-hour mtime cutoff at line 297 still admits older day directories, because a resumed session writes to its original start-date directory, so the wide walk is correct. Caching the set of day directories that contained a recent file, and re-scanning the full range less often, would reduce steady-state I/O.

Also consider naming 86400, 30, and 24 as static constants for clarity.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Sources/Model/CodexTaskStatusStore.swift` around lines 297 - 334, Reduce
repeated polling I/O around the day-directory enumeration by caching which day
directories contain recently modified rollout files and re-scanning the full
31-day range only when needed, while preserving discovery of resumed sessions
writing to older start-date directories and the existing 24-hour cutoff
behavior. Name the literals 86400, 30, and 24 as static constants near the
relevant status-store logic, and update the loop, cutoff, and result limit to
use them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@Sources/Model/CodexTaskStatusStore.swift`:
- Around line 105-117: Update the Combine pipeline in CodexTaskStatusStore’s
activityCancellable setup to insert receive(on: DispatchQueue.main) after
removeDuplicates() and before sink. Preserve the existing active mapping and
ensure setPollingActive and its timer/UI-alert work execute on the main thread.

In `@Sources/Views/PanelHeader.swift`:
- Around line 54-76: Update the codexStatusTitle accessibility configuration to
add an explicit localized label containing taskStatus.snapshot.status.label,
ensuring accessibility clients receive the status value even when displayMode is
.icon.

---

Nitpick comments:
In `@Sources/Model/CodexTaskStatusStore.swift`:
- Around line 171-183: Update the Task closure in the refresh flow to reset
refreshInFlight with defer immediately after entering the closure, before any
await or early return. Remove the success-path reset, while preserving the
existing weak-self guard and snapshot application behavior.
- Around line 297-334: Reduce repeated polling I/O around the day-directory
enumeration by caching which day directories contain recently modified rollout
files and re-scanning the full 31-day range only when needed, while preserving
discovery of resumed sessions writing to older start-date directories and the
existing 24-hour cutoff behavior. Name the literals 86400, 30, and 24 as static
constants near the relevant status-store logic, and update the loop, cutoff, and
result limit to use them.

In `@Tests/CodexTaskStatusLogParserTests.swift`:
- Around line 43-51: Update the growth loop in the test around
CodexTaskStatusLogParser.parse to open one writable FileHandle before iterating,
reuse it for each update append, and close it after the loop completes. Preserve
the existing data growth and periodic parser invocation behavior.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: bc1b4470-4fa5-4d88-a26b-acc45f787a4e

📥 Commits

Reviewing files that changed from the base of the PR and between 3b78b69 and cad721c.

📒 Files selected for processing (14)
  • README.md
  • README.zh-CN.md
  • Resources/en.lproj/Localizable.strings
  • Resources/zh-Hans.lproj/Localizable.strings
  • Sources/App.swift
  • Sources/Model/CodexTaskStatusLogParser.swift
  • Sources/Model/CodexTaskStatusStore.swift
  • Sources/Views/CodexTaskStatusView.swift
  • Sources/Views/IslandRootView.swift
  • Sources/Views/PanelHeader.swift
  • Sources/Views/SettingsView.swift
  • Sources/Views/UsageView.swift
  • Tests/CodexTaskStatusLogParserTests.swift
  • scripts/run-tests.sh

Comment thread Sources/Model/CodexTaskStatusStore.swift
Comment thread Sources/Views/PanelHeader.swift

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cad721cf69

ℹ️ 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".

Comment on lines +299 to +301
let calendar = Calendar(identifier: .gregorian)
for dayOffset in 0...30 {
guard let date = calendar.date(byAdding: .day, value: -dayOffset, to: Date()) else {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Scan rollout date partitions in UTC

Codex partitions rollout files by the UTC date, but this calendar inherits the Mac's local time zone and the loop only visits the local current day and earlier days. I verified Codex v0.144.0-alpha.4 under TZ=Pacific/Honolulu: while the local date was August 6, it created the active rollout under sessions/2026/08/07; this scanner therefore misses every active task between UTC midnight and local midnight in western time zones. Set the calendar's time zone to UTC before deriving the directory components.

Useful? React with 👍 / 👎.

Comment on lines +239 to +240
guard fingerprint != previousFingerprint else {
return ScanResult(fingerprint: fingerprint, snapshot: nil)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Recompute time-based terminal-state decay

When a selected error or cancellation crosses the ten-minute decay threshold without another rollout write, the file fingerprint remains unchanged and this early return prevents selectionPriority from being reevaluated. The 15-second timer continues calling scan, but the displayed terminal state can remain stuck until some file metadata changes—potentially for the full 24-hour rollout cutoff—instead of decaying as the policy intends. Include a time bucket in the fingerprint or recompute selection before this shortcut.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
Sources/Model/CodexTaskStatusStore.swift (2)

300-321: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Publish decayed terminal snapshots as idle.

CodexTaskStatusPolicy.priority() returns priority 0 for stale .cancelled or .error states, but the scan still selects and returns the original snapshot unchanged. Views read store.snapshot.status directly, so a terminal file can keep showing error/cancelled after decay. If a decayed terminal file is the most recent relevant state, return .idle from the scan instead.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Sources/Model/CodexTaskStatusStore.swift` around lines 300 - 321, Update the
scan result construction around selected and selectionPriority so a selected
stale terminal .cancelled or .error state with priority 0 is published as an
.idle snapshot. Preserve the selected snapshot’s other fields, and keep
returning the existing unavailable snapshot when no state is selected.

224-232: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Retain the NSSound instance until playback finishes.

NSSound.play() starts playback asynchronously. The local NSSound here has no strong reference after playSound returns, so it can be deallocated while playing and stop playback.

Store the sound in a property, or set an NSSoundDelegate that clears the reference when sound(_:didFinishPlaying:) is called.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Sources/Model/CodexTaskStatusStore.swift` around lines 224 - 232, The
playSound(for:) method must retain the NSSound instance for the duration of
asynchronous playback. Store the successfully created sound in a property owned
by the surrounding type, and clear that property when playback finishes via
NSSoundDelegate; preserve the existing beep fallback when playback cannot start.
🧹 Nitpick comments (3)
Sources/Model/CodexTaskStatusStore.swift (3)

12-14: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Consider aligning the day lookback with the recency cutoff.

recentFileAge is 24 hours, but a full scan enumerates 31 day directories. Only the current and previous UTC day directories can hold files that pass the cutoff. The remaining directory reads always return no eligible files.

A lookback of 1 or 2 days gives the same result with less directory I/O.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Sources/Model/CodexTaskStatusStore.swift` around lines 12 - 14, Update the
maximumDayLookback constant in CodexTaskStatusStore to cover only the current
and previous UTC day directories allowed by recentFileAge, using a lookback of 2
days instead of scanning 30 days. Preserve the existing full-scan behavior and
recency filtering.

214-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider consolidating the Status to CodexTaskLogState mapping.

The same mapping appears in logState(for:) here and in selectionPriority at lines 325-331. The inverse mapping appears in parseState at lines 438-444. A new case in either enum requires three edits.

Add a logState property on Status and an initializer Status(_: CodexTaskLogState), then use them at all three sites.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Sources/Model/CodexTaskStatusStore.swift` around lines 214 - 222, Consolidate
the enum mapping by adding a `logState` computed property to `Status` and an
initializer accepting `CodexTaskLogState`. Replace the switch in
`logState(for:)`, the mapping in `selectionPriority`, and the inverse mapping in
`parseState` with these centralized APIs, preserving existing behavior for every
case.

376-379: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add parentheses to the needsFullScan expression.

?? binds tighter than ||, so this reads as !cacheMatchesRoot || (lastFullDirectoryScan.map { ... } ?? true). That is the intended behavior, but the precedence is not obvious at a glance.

Explicit parentheses, or a small helper let scanExpired = ..., make the intent clear.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Sources/Model/CodexTaskStatusStore.swift` around lines 376 - 379, Clarify the
`needsFullScan` expression by explicitly grouping the `lastFullDirectoryScan.map
{ ... } ?? true` portion, or assign it to a helper such as `scanExpired` before
combining it with `!cacheMatchesRoot`. Preserve the existing precedence and
behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@Sources/Model/CodexTaskStatusStore.swift`:
- Around line 262-269: Update the unavailable branch in scan to return a nil
snapshot when previousFingerprint is already "unavailable", while preserving the
existing unavailable snapshot for the initial transition. Keep the fingerprint
and directory-scan metadata unchanged so refresh/apply can skip redundant
publishes.

---

Outside diff comments:
In `@Sources/Model/CodexTaskStatusStore.swift`:
- Around line 300-321: Update the scan result construction around selected and
selectionPriority so a selected stale terminal .cancelled or .error state with
priority 0 is published as an .idle snapshot. Preserve the selected snapshot’s
other fields, and keep returning the existing unavailable snapshot when no state
is selected.
- Around line 224-232: The playSound(for:) method must retain the NSSound
instance for the duration of asynchronous playback. Store the successfully
created sound in a property owned by the surrounding type, and clear that
property when playback finishes via NSSoundDelegate; preserve the existing beep
fallback when playback cannot start.

---

Nitpick comments:
In `@Sources/Model/CodexTaskStatusStore.swift`:
- Around line 12-14: Update the maximumDayLookback constant in
CodexTaskStatusStore to cover only the current and previous UTC day directories
allowed by recentFileAge, using a lookback of 2 days instead of scanning 30
days. Preserve the existing full-scan behavior and recency filtering.
- Around line 214-222: Consolidate the enum mapping by adding a `logState`
computed property to `Status` and an initializer accepting `CodexTaskLogState`.
Replace the switch in `logState(for:)`, the mapping in `selectionPriority`, and
the inverse mapping in `parseState` with these centralized APIs, preserving
existing behavior for every case.
- Around line 376-379: Clarify the `needsFullScan` expression by explicitly
grouping the `lastFullDirectoryScan.map { ... } ?? true` portion, or assign it
to a helper such as `scanExpired` before combining it with `!cacheMatchesRoot`.
Preserve the existing precedence and behavior.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 171aa480-769d-4dc4-9f72-5434f4aede9c

📥 Commits

Reviewing files that changed from the base of the PR and between cad721c and 089f996.

📒 Files selected for processing (4)
  • Sources/Model/CodexTaskStatusLogParser.swift
  • Sources/Model/CodexTaskStatusStore.swift
  • Sources/Views/PanelHeader.swift
  • Tests/CodexTaskStatusLogParserTests.swift
🚧 Files skipped from review as they are similar to previous changes (3)
  • Sources/Model/CodexTaskStatusLogParser.swift
  • Sources/Views/PanelHeader.swift
  • Tests/CodexTaskStatusLogParserTests.swift

Comment thread Sources/Model/CodexTaskStatusStore.swift

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@README.md`:
- Around line 202-205: Update README.md lines 202-205 to remove “waiting for
approval” and the claim that approval is detected from unresolved
request_permissions calls, while retaining only supported states. Update
README.md lines 209-210 to remove “approval uses Ping” and the separate
cancellation/error sound mappings, documenting only the supported completion and
attention sounds.

In `@Sources/Model/CodexTaskStatusLogParser.swift`:
- Around line 3-17: Remove approval-only handling from
CodexTaskStatusLogParser.swift: delete CodexTaskLogState.waitingApproval and
CodexTaskStatusSoundEvent.approvalRequired, remove approval priority logic, and
stop converting permission request/output items into task status. In
Tests/CodexTaskStatusLogParserTests.swift lines 254-272, remove the
approval-reporting assertions and update affected expectations to cover only
observed task states.
- Around line 140-144: Update the file-reading flow around FileHandle in the
parser so the handle is scheduled for closure immediately after successful
opening, before read(upToCount:) can fail and exit the guard. Preserve the
existing maxBytes validation and false-return behavior while ensuring every
opened handle is closed.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2fa2dca2-673c-4243-8f99-88a63b682c9a

📥 Commits

Reviewing files that changed from the base of the PR and between 1f3fa59 and 3236051.

📒 Files selected for processing (10)
  • README.md
  • README.zh-CN.md
  • Resources/en.lproj/Localizable.strings
  • Resources/zh-Hans.lproj/Localizable.strings
  • Sources/Model/CodexTaskStatusLogParser.swift
  • Sources/Model/CodexTaskStatusStore.swift
  • Sources/Views/CodexTaskStatusView.swift
  • Sources/Views/SettingsView.swift
  • Tests/CodexTaskStatusLogParserTests.swift
  • scripts/run-tests.sh
💤 Files with no reviewable changes (1)
  • scripts/run-tests.sh
🚧 Files skipped from review as they are similar to previous changes (6)
  • Resources/en.lproj/Localizable.strings
  • Resources/zh-Hans.lproj/Localizable.strings
  • Sources/Model/CodexTaskStatusStore.swift
  • README.zh-CN.md
  • Sources/Views/SettingsView.swift
  • Sources/Views/CodexTaskStatusView.swift

Comment thread README.md
Comment on lines +202 to +205
states the current rollout format can support reliably: running, waiting for
approval, idle, cancelled, error, or unavailable. Approval is detected from an
unresolved `request_permissions` call. User-input waits are not claimed because
current rollout files do not expose a reliable event for that state. The

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove unsupported approval behavior from the documentation.

The README claims approval detection and approval-specific sounds, but the current contract supports only running, idle, cancelled, error, and unavailable, with completion and attention sounds.

  • README.md#L202-L205: remove waiting for approval and the request_permissions approval-detection claim.
  • README.md#L209-L210: remove approval uses Ping and the separate cancellation/error mappings.
📍 Affects 1 file
  • README.md#L202-L205 (this comment)
  • README.md#L209-L210
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@README.md` around lines 202 - 205, Update README.md lines 202-205 to remove
“waiting for approval” and the claim that approval is detected from unresolved
request_permissions calls, while retaining only supported states. Update
README.md lines 209-210 to remove “approval uses Ping” and the separate
cancellation/error sound mappings, documenting only the supported completion and
attention sounds.

Comment on lines +3 to +17
enum CodexTaskLogState: Equatable, Sendable {
case running
case waitingApproval
case idle
case cancelled
case error
case unavailable
}

enum CodexTaskStatusSoundEvent: Equatable, Sendable {
case completed
case error
case cancelled
case approvalRequired
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Do not report approval states without observed rollout events.

The PR objective excludes approval detection. This implementation reports .waitingApproval, gives it priority over .running, and can play .approvalRequired. Remove this behavior and its consumer handling, or revise the objective after validating the event schema against observed rollout logs.

  • Sources/Model/CodexTaskStatusLogParser.swift#L3-L17: Remove approval-only state and sound-event cases.
  • Sources/Model/CodexTaskStatusLogParser.swift#L55-L55: Remove approval-state priority.
  • Sources/Model/CodexTaskStatusLogParser.swift#L326-L337: Do not convert permission request/output items into task status.
  • Tests/CodexTaskStatusLogParserTests.swift#L254-L272: Remove approval-reporting assertions.
📍 Affects 2 files
  • Sources/Model/CodexTaskStatusLogParser.swift#L3-L17 (this comment)
  • Sources/Model/CodexTaskStatusLogParser.swift#L55-L55
  • Sources/Model/CodexTaskStatusLogParser.swift#L326-L337
  • Tests/CodexTaskStatusLogParserTests.swift#L254-L272
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Sources/Model/CodexTaskStatusLogParser.swift` around lines 3 - 17, Remove
approval-only handling from CodexTaskStatusLogParser.swift: delete
CodexTaskLogState.waitingApproval and
CodexTaskStatusSoundEvent.approvalRequired, remove approval priority logic, and
stop converting permission request/output items into task status. In
Tests/CodexTaskStatusLogParserTests.swift lines 254-272, remove the
approval-reporting assertions and update affected expectations to cover only
observed task states.

Comment on lines +140 to +144
guard maxBytes > 0,
let handle = try? FileHandle(forReadingFrom: url),
let data = try? handle.read(upToCount: maxBytes)
else { return false }
try? handle.close()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Files matching CodexTaskStatusLogParser.swift:"
fd -a 'CodexTaskStatusLogParser\.swift$' . || true

file="$(fd 'CodexTaskStatusLogParser\.swift$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
  echo "--- wc ---"
  wc -l "$file"
  echo "--- outline ---"
  ast-grep outline "$file" || true
  echo "--- relevant lines ---"
  nl -ba "$file" | sed -n '110,160p'
fi

echo "--- search isSubagentSession occurrences/use ---"
rg -n "isSubagentSession|CodexTaskStatusLogParser" . || true

Repository: ericjypark/codex-island

Length of output: 1057


🏁 Script executed:

#!/bin/bash
set -euo pipefail

file="./Sources/Model/CodexTaskStatusLogParser.swift"
echo "--- relevant lines ---"
awk '{printf "%8d\t%s\n", NR, $0}' "$file" | sed -n '110,160p'

echo "--- method context ---"
awk 'BEGIN{RS=""; FS="\n"} {
  for(i=1;i<=NR;i++) if(/method:\s*isSubagentSession/) print i, $0
}' "$file"

echo "--- isSubagentSession occurrences/use ---"
rg -n "isSubagentSession|CodexTaskStatusLogParser" . || true

Repository: ericjypark/codex-island

Length of output: 6137


Close the handle when the read fails.

If handle.read(upToCount:) fails, the guard returns before closing the opened FileHandle. Move the deferred close to run after opening the handle.

Proposed fix
-        guard maxBytes > 0,
-              let handle = try? FileHandle(forReadingFrom: url),
-              let data = try? handle.read(upToCount: maxBytes)
-        else { return false }
-        try? handle.close()
+        guard maxBytes > 0,
+              let handle = try? FileHandle(forReadingFrom: url)
+        else { return false }
+        defer { try? handle.close() }
+        guard let data = try? handle.read(upToCount: maxBytes) else {
+            return false
+        }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
guard maxBytes > 0,
let handle = try? FileHandle(forReadingFrom: url),
let data = try? handle.read(upToCount: maxBytes)
else { return false }
try? handle.close()
guard maxBytes > 0,
let handle = try? FileHandle(forReadingFrom: url)
else { return false }
defer { try? handle.close() }
guard let data = try? handle.read(upToCount: maxBytes) else {
return false
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Sources/Model/CodexTaskStatusLogParser.swift` around lines 140 - 144, Update
the file-reading flow around FileHandle in the parser so the handle is scheduled
for closure immediately after successful opening, before read(upToCount:) can
fail and exit the guard. Preserve the existing maxBytes validation and
false-return behavior while ensuring every opened handle is closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant