fix(monitor): GUI scheduled scan から自律起動を駆動する - #3517
Conversation
📝 WalkthroughWalkthroughThe PR adds asynchronous, authority-fenced Issue Monitor scheduled scans. It adds local fallback leasing, daemon startup retry handling, durable result rebasing, PM wake integration, completion events, shared claim execution, and Windows contract tests. ChangesIssue Monitor scheduling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ScheduledTick
participant AppRuntime
participant IssueMonitorWorker
participant GitHubIssueClient
participant UserEventLoop
ScheduledTick->>AppRuntime: request scheduled scan
AppRuntime->>IssueMonitorWorker: start single-flight worker
IssueMonitorWorker->>GitHubIssueClient: scan candidates and execute effects
GitHubIssueClient-->>IssueMonitorWorker: return scan and mutation results
IssueMonitorWorker-->>UserEventLoop: emit completion event
UserEventLoop->>AppRuntime: process results and PM wake events
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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: 354fb5f772
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/gwt/src/main.rs (1)
7808-7821: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winscheduler 起動失敗時に正常起動を継続しないでください。
この thread は GUI-only topology の scheduled scan を開始する主要な起点です。
thread::spawnが失敗しても GUI を継続すると、アプリは正常起動に見えますが、scheduled scan と autonomous launch は実行されません。起動を失敗させるか、限定回数の再試行と degraded 状態の通知を実装してください。
最小修正案
{ tracing::error!(%error, "failed to start Issue Monitor scheduled tick thread"); + return Err(error); }🤖 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 `@crates/gwt/src/main.rs` around lines 7808 - 7821, Update the scheduled-tick thread startup around IssueMonitorScheduledTick so a std::thread::Builder::spawn failure prevents normal GUI startup from continuing. Propagate the spawn error through the surrounding initialization path and return/abort startup, or implement bounded retries with an explicit degraded-state notification; do not merely log the error and continue.
🧹 Nitpick comments (9)
crates/gwt/src/app_runtime/tests.rs (2)
3000-3038: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider a longer deadline for Windows CI.
The helper waits at most 5 seconds. The scheduled worker in the new Windows job spawns a fake
ghprocess and performs git/prefs I/O. Process spawn onwindows-latestis slow, so this bound can produce flaky failures. A 30-second deadline keeps the failure message useful and removes the timing risk.♻️ Proposed change
- let deadline = Instant::now() + Duration::from_secs(5); + let deadline = Instant::now() + Duration::from_secs(30);🤖 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 `@crates/gwt/src/app_runtime/tests.rs` around lines 3000 - 3038, Increase the timeout in wait_for_scheduled_scan_completion from 5 seconds to 30 seconds so scheduled scan completion remains reliable on slower Windows CI while preserving the existing polling and failure behavior.
43597-43603: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClear the global scan hook when the test ends.
set_scheduled_scan_after_lease_before_commit_test_hookstores the closure in a process-global slot and asserts that the slot is empty (crates/gwt/src/app_runtime/mod.rslines 962-967). The worker consumes the hook only when it reaches the commit point. If the worker fails earlier, the hook stays installed for the rest of the test binary process, and the next test that installs a hook panics on the assert. Add a scope guard that clears the slot on drop, so the contract stays safe when a second test starts using this hook.🤖 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 `@crates/gwt/src/app_runtime/tests.rs` around lines 43597 - 43603, Add a scope guard around the test’s use of set_scheduled_scan_after_lease_before_commit_test_hook that clears the global hook slot on drop, including when the worker exits before consuming it. Ensure the guard is held for the test’s full execution and uses the existing hook-clearing mechanism rather than changing the worker behavior..github/workflows/test.yml (1)
110-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake contract filters fail when no tests match.
The current filters match tests. The three named filters match one test each,
app_runtime_local_driver_matches three binary tests, andlocal_fallback_leasematches three library tests. The--libtarget is correct.If these commands must detect renames, add
--exactto the three single-test commands. Replace the prefix filters with explicit names or assert the expected match count.🤖 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 @.github/workflows/test.yml around lines 110 - 114, Update the targeted cargo test commands so contract filters fail when tests are renamed or removed: add --exact to the three single-test filters, and replace app_runtime_local_driver_ and local_fallback_lease with explicit test names or otherwise validate their expected match counts while preserving the --lib target.crates/gwt/src/app_runtime/mod.rs (3)
1653-1669: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftReplace the string-matched error classification with a typed signal.
Line 1658 selects between
record_scan_errorandrecord_launch_auth_requiredby testing whether the error message contains"deadline". The same pattern exists in the pre-existing Windows driver at line 3921, but this PR makes it part of the primary GUI-only scheduled path.The failure mode is asymmetric. A deadline error whose text stops containing
"deadline"is recorded aslaunch_auth_required, which is a sticky state the operator must clear, instead of a transient scan error that self-heals on the next tick.Return a typed error from
execute_local_issue_monitor_claim_effects— for example an enum distinguishingDeadlineExpiredfromAuthUnavailable— and match on it at both call 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 `@crates/gwt/src/app_runtime/mod.rs` around lines 1653 - 1669, Replace the string-based `"deadline"` check in execute_local_issue_monitor_claim_effects and its callers with a typed error enum distinguishing DeadlineExpired from AuthUnavailable. Return the appropriate variant from execute_local_issue_monitor_claim_effects, then match on that enum at both the GUI scheduled path and the existing Windows driver call site, routing DeadlineExpired to record_scan_error and AuthUnavailable to record_launch_auth_required.
1420-1463: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the shadowed
ownerbinding.The closure parameter list binds the repo owner as
owner. TheAcquireClaimarms then destructure a payload field also namedowner, which is the claim owner. Inside those armsownerrefers to the claim owner.The current code is correct because
issue_client_factory(owner, repo)is evaluated at line 1420, before the match, so it receives the repo owner. The shadowing is still a hazard: moving the factory call into an arm would silently pass the claim owner as the repo owner, and the two values are both plain&str.Rename the destructured field binding to make the distinction explicit.
♻️ Proposed refactor
gwt::IssueMonitorEffectPayload::AcquireClaim { issue_number, claim_id, - owner, + owner: claim_owner, heartbeat_at, expires_at, launched_work_id, } if authority_current => { @@ ClaimComment { comment_id: None, claim_id: claim_id.clone(), - owner: owner.clone(), + owner: claim_owner.clone(), issue_number: *issue_number,Apply the same rename to the revoke arm and the
ReleaseClaimarm.🤖 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 `@crates/gwt/src/app_runtime/mod.rs` around lines 1420 - 1463, Rename the `AcquireClaim` payload’s destructured `owner` binding to a distinct claim-owner name in both match arms, and update the corresponding `ClaimComment` and `release_claim_mutation` arguments. Apply the same rename in the revoke and `ReleaseClaim` arms while keeping the outer repository owner binding used by `issue_client_factory` unchanged.
1519-1522: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDistinguish "monitor disabled" from "deferred to a live daemon".
This early return reports
ScheduledIssueMonitorScanOutcome::DeferredToLiveDaemonwhen the monitor is disabled and no claim cleanup is pending. No daemon is involved in that decision.The completion handler at line 4042 happens to do the right thing for both cases — it re-arms the PM periodic wake — so behavior is correct today. The name misdescribes the cause, which will mislead the next reader of a log or a test assertion.
Add a third variant, for example
NothingToDo, and map it to the same completion behavior.🤖 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 `@crates/gwt/src/app_runtime/mod.rs` around lines 1519 - 1522, Add a distinct ScheduledIssueMonitorScanOutcome variant such as NothingToDo for the disabled-monitor/no-cleanup path in the scheduled scan logic, replacing the misleading DeferredToLiveDaemon return. Update the completion handler around its existing outcome match to treat NothingToDo identically to DeferredToLiveDaemon, preserving the PM periodic wake re-arm behavior.crates/gwt/src/cli/daemon/server.rs (2)
358-367: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the terminal-state destructuring.
The retry loop already guarantees
authority_retry_pending == falseat this point. Thelet ... elsewith afalseliteral pattern plusunreachable!()encodes that invariant in a refutable pattern, which is harder to read than the invariant it protects.Use an irrefutable destructure and a
debug_assert!.♻️ Proposed refactor
let LoadedDaemonIssueMonitorState { mut monitor, recovery_blocked, - authority_retry_pending: false, + authority_retry_pending, authority_fence, authority_lease, - } = loaded - else { - unreachable!("authority retry loop exits only with a terminal load state") - }; + } = loaded; + debug_assert!( + !authority_retry_pending, + "authority retry loop exits only with a terminal load state" + );🤖 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 `@crates/gwt/src/cli/daemon/server.rs` around lines 358 - 367, Update the terminal-state destructuring of LoadedDaemonIssueMonitorState to bind authority_retry_pending instead of matching it against false, making the destructure irrefutable and removing the unreachable! branch. Immediately follow it with a debug_assert! that authority_retry_pending is false, while preserving the existing monitor, recovery_blocked, authority_fence, and authority_lease bindings.
6608-6671: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIsolate the worker test from process-global environment state.
This test spawns the real Issue Monitor worker, which starts a scan that reads the ambient
GWThome andPATH. Neighboring worker tests in this module guard that state — for exampleworker_shutdown_after_ready_preserves_new_corrupt_bytestakescrate::env_test_lock()and installsScopedGwtHome. Without those guards a parallel test that mutatesHOME,GWT_HOME, orPATHcan make this test flaky, and the first scan invokes whateverghbinary is on the ambientPATH.Add the same environment guards used by the other worker tests.
♻️ Proposed fix
#[tokio::test] + #[allow(clippy::await_holding_lock)] // global environment must stay isolated for the worker lifetime async fn worker_stays_starting_until_the_local_fallback_lease_is_released() { + let _env_lock = crate::env_test_lock() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); let temp = TempDir::new().expect("tempdir"); + let home = temp.path().join("home"); + fs::create_dir_all(&home).expect("create gwt home"); + let _home = ScopedGwtHome::set(&home); let repo = temp.path().join("repo");🤖 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 `@crates/gwt/src/cli/daemon/server.rs` around lines 6608 - 6671, Update worker_stays_starting_until_the_local_fallback_lease_is_released to acquire crate::env_test_lock() and install the same ScopedGwtHome and PATH isolation used by neighboring worker tests, especially worker_shutdown_after_ready_preserves_new_corrupt_bytes. Keep these guards active for the entire real-worker lifetime so its initial scan cannot observe concurrent process-global environment changes.crates/gwt/src/issue_monitor.rs (1)
9257-9328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a fallback-lease test for the legacy fence arm.
The three tests cover
Missing, live-lockActive, and stale v2Active. They do not cover theLegacyShutdownRevoke | Active(_)arm oftry_acquire_issue_monitor_local_fallback_lease. That arm is the fail-closed guard against a legacy daemon. Add a test that seeds a v1 fence and assertsWouldBlock, an unchanged fence file, and an unchangedeffect_authority_epoch.The repository targets 90%+ coverage across unit, integration, and E2E tests, so this branch should be pinned. As per coding guidelines, "gwt プロジェクトでは単体テスト、結合テスト、E2E テストを含む全体のテストカバレッジを 90% 以上で維持する".
🤖 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 `@crates/gwt/src/issue_monitor.rs` around lines 9257 - 9328, Add a unit test alongside the existing local fallback lease tests that seeds a legacy v1 fence, invokes try_acquire_issue_monitor_local_fallback_lease, and asserts it returns io::ErrorKind::WouldBlock. Capture and compare the fence state/file contents and effect_authority_epoch before and after the call to verify the fail-closed LegacyShutdownRevoke | Active(_) path leaves both unchanged.Source: Coding guidelines
🤖 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 @.github/workflows/test.yml:
- Around line 108-115: Add shell: bash to both multi-line Windows test steps,
including the step running the Issue Monitor scheduled driver contracts and the
preceding step around line 98, so any failed cargo test causes the workflow step
to fail.
In @.gwt/work/events.jsonl:
- Around line 1522-1536: Remove the manually added Issue `#3505` lifecycle rows
from the projection log, especially the entries ending in the premature
kind:"done" event. Do not edit `.gwt/work/events.jsonl` to record progress or
completion; track remaining commit, verification, push, and PR work in Issue
`#3505` and let the runtime emit events after those actions actually occur.
In `@crates/gwt/src/app_runtime/mod.rs`:
- Around line 4057-4068: Update the `latest` preference match in the scheduled
completion flow to call `pm_periodic_wake_events_at` before returning on both
the disabled `Ok(_)` path and the reload-error `Err(error)` path. Preserve the
existing empty-vector and toast responses while ensuring each path re-arms the
periodic wake.
- Around line 1083-1085: Remove the 250 ms ScopedOperationDeadline applied
inside try_rebase_mutate_and_persist_issue_monitor_state_without_authority_fence
so run_scheduled_issue_monitor_scan’s 60-second deadline remains effective for
background scans; preserve the existing DeferredToLiveDaemon handling.
In `@crates/gwt/src/cli/daemon/server.rs`:
- Around line 341-350: Update the retry loop around
load_issue_monitor_state_for_daemon to execute each state reload via
tokio::task::spawn_blocking, awaiting the blocking task before continuing.
Preserve the existing 25 ms delay, shutdown handling, and loaded-state
assignment while ensuring the blocking file and filesystem work does not run on
Tokio worker threads.
---
Outside diff comments:
In `@crates/gwt/src/main.rs`:
- Around line 7808-7821: Update the scheduled-tick thread startup around
IssueMonitorScheduledTick so a std::thread::Builder::spawn failure prevents
normal GUI startup from continuing. Propagate the spawn error through the
surrounding initialization path and return/abort startup, or implement bounded
retries with an explicit degraded-state notification; do not merely log the
error and continue.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 110-114: Update the targeted cargo test commands so contract
filters fail when tests are renamed or removed: add --exact to the three
single-test filters, and replace app_runtime_local_driver_ and
local_fallback_lease with explicit test names or otherwise validate their
expected match counts while preserving the --lib target.
In `@crates/gwt/src/app_runtime/mod.rs`:
- Around line 1653-1669: Replace the string-based `"deadline"` check in
execute_local_issue_monitor_claim_effects and its callers with a typed error
enum distinguishing DeadlineExpired from AuthUnavailable. Return the appropriate
variant from execute_local_issue_monitor_claim_effects, then match on that enum
at both the GUI scheduled path and the existing Windows driver call site,
routing DeadlineExpired to record_scan_error and AuthUnavailable to
record_launch_auth_required.
- Around line 1420-1463: Rename the `AcquireClaim` payload’s destructured
`owner` binding to a distinct claim-owner name in both match arms, and update
the corresponding `ClaimComment` and `release_claim_mutation` arguments. Apply
the same rename in the revoke and `ReleaseClaim` arms while keeping the outer
repository owner binding used by `issue_client_factory` unchanged.
- Around line 1519-1522: Add a distinct ScheduledIssueMonitorScanOutcome variant
such as NothingToDo for the disabled-monitor/no-cleanup path in the scheduled
scan logic, replacing the misleading DeferredToLiveDaemon return. Update the
completion handler around its existing outcome match to treat NothingToDo
identically to DeferredToLiveDaemon, preserving the PM periodic wake re-arm
behavior.
In `@crates/gwt/src/app_runtime/tests.rs`:
- Around line 3000-3038: Increase the timeout in
wait_for_scheduled_scan_completion from 5 seconds to 30 seconds so scheduled
scan completion remains reliable on slower Windows CI while preserving the
existing polling and failure behavior.
- Around line 43597-43603: Add a scope guard around the test’s use of
set_scheduled_scan_after_lease_before_commit_test_hook that clears the global
hook slot on drop, including when the worker exits before consuming it. Ensure
the guard is held for the test’s full execution and uses the existing
hook-clearing mechanism rather than changing the worker behavior.
In `@crates/gwt/src/cli/daemon/server.rs`:
- Around line 358-367: Update the terminal-state destructuring of
LoadedDaemonIssueMonitorState to bind authority_retry_pending instead of
matching it against false, making the destructure irrefutable and removing the
unreachable! branch. Immediately follow it with a debug_assert! that
authority_retry_pending is false, while preserving the existing monitor,
recovery_blocked, authority_fence, and authority_lease bindings.
- Around line 6608-6671: Update
worker_stays_starting_until_the_local_fallback_lease_is_released to acquire
crate::env_test_lock() and install the same ScopedGwtHome and PATH isolation
used by neighboring worker tests, especially
worker_shutdown_after_ready_preserves_new_corrupt_bytes. Keep these guards
active for the entire real-worker lifetime so its initial scan cannot observe
concurrent process-global environment changes.
In `@crates/gwt/src/issue_monitor.rs`:
- Around line 9257-9328: Add a unit test alongside the existing local fallback
lease tests that seeds a legacy v1 fence, invokes
try_acquire_issue_monitor_local_fallback_lease, and asserts it returns
io::ErrorKind::WouldBlock. Capture and compare the fence state/file contents and
effect_authority_epoch before and after the call to verify the fail-closed
LegacyShutdownRevoke | Active(_) path leaves both unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d197bf20-0b8f-4c02-b4a9-6f7919a94fcf
📒 Files selected for processing (10)
.github/workflows/test.yml.gwt/work/events.jsonlcrates/gwt-github/src/issue_auto_claim.rscrates/gwt/src/app_runtime/mod.rscrates/gwt/src/app_runtime/pm.rscrates/gwt/src/app_runtime/tests.rscrates/gwt/src/cli/daemon/server.rscrates/gwt/src/issue_monitor.rscrates/gwt/src/lib.rscrates/gwt/src/main.rs
1 participant
Summary
Changes
crates/gwt/src/app_runtime/: project-scoped single-flight worker、typed completion、fresh prefs rebase、local claim effect driver、queue/active/needs_human wake を追加しました。crates/gwt/src/issue_monitor.rsとcrates/gwt/src/cli/daemon/server.rs: GUI fallback lease と daemon startup retry を同じ authority lock 上で実装しました。crates/gwt-github/src/issue_auto_claim.rs: runtime の trait-object client から claim mutation を実行できるよう generic 境界を拡張しました。.github/workflows/test.yml: Windows native で no-daemon scheduled launch、disabled cleanup、local driver、lease 契約を明示実行します。crates/gwt/src/app_runtime/tests.rs: no-daemon progress、live fence zero mutation、commit-time race、single-flight、disabled cleanup、completion wake、failure visibility、compatibility driver競合の回帰 matrix を追加しました。Testing
cargo fmt --all -- --check— PASScargo clippy --all-targets --all-features -- -D warnings— PASScargo test -p gwt-core -p gwt --all-features -- --test-threads=1— PASScargo build -p gwt --bin gwt --bin gwtd— PASSbash scripts/check-frontend-bundle.sh— PASSbash scripts/run-frontend-unit-tests.sh— 1197 PASSbash scripts/run-frontend-smoke-tests.sh— 30 PASSbash scripts/run-visual-tests.sh— 214 PASS / 82 skippedvrr-e66ca093b65642fb83dc5b6203c49a69PR Readiness
Closing Issues
Related Issues / Links
Checklist
cargo clippy,cargo fmt)Context
cfg(not(unix))に残り、macOS/Linux GUI-only production では queue refresh から launch へ進みませんでした。本PRは effect boundary を貫通する corrective slice です。Risk / Impact
8467a0311と6559431c1を revert すると PR feat(pm): 常駐ループの穴 3 件と scheduled scan 不在を修正する (SPEC #3431 FR-108〜110 / #3505) #3507 時点の status-only scheduled behavior へ戻ります。永続schema変更はありません。