fix(codex): persist hook verification status - #28
Merged
Conversation
📝 WalkthroughWalkthroughThe PR adds deterministic hook-definition fingerprints and persistent verification state, records verification only from real Codex events, clears state when hooks change or are removed, and updates the settings UI, styling, and test mocks to use the new runtime status fields. ChangesHook verification integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CodexHook
participant HookServer
participant HookVerification
participant SettingsWindow
CodexHook->>HookServer: emit_event(event)
HookServer->>HookServer: classify real Codex event
HookServer->>HookVerification: record(timestamp)
HookServer->>SettingsWindow: emit codex-event
SettingsWindow->>HookServer: refreshHookStatus()
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src-tauri/src/hook_server.rs`:
- Around line 283-292: Update the HookRuntimeStatus construction in the status
function to call latest_real_event() unconditionally instead of gating it
through hook_verification::verified_at(). Keep verified_at assigned solely to
the verified_at field, while last_real_event_at and last_real_event continue
deriving from the independently retrieved latest real event.
In `@src-tauri/src/hook_verification.rs`:
- Around line 75-105: Update record so a matching persisted
VerificationState.verified_at is reused when initializing or updating
RuntimeVerification memory, instead of retaining the incoming timestamp. Keep
last_real_event as the per-run activity marker, and ensure runtime and persisted
verification timestamps remain consistent across restarts.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f9f8f502-460d-4c7c-89e4-9afc76fdde30
📒 Files selected for processing (8)
e2e/readme-screenshots.screenshot.tse2e/settings.e2e.tssrc-tauri/src/hook_installer.rssrc-tauri/src/hook_server.rssrc-tauri/src/hook_verification.rssrc-tauri/src/lib.rssrc/settings.tssrc/styles.css
Comment on lines
+283
to
+292
| let verified_at = crate::hook_verification::verified_at(); | ||
| let latest_real = verified_at.and_then(|_| latest_real_event()); | ||
| HookRuntimeStatus { | ||
| receiver_running: RECEIVER_RUNNING.load(Ordering::Acquire), | ||
| socket_path: socket_path() | ||
| .map(|path| path.to_string_lossy().to_string()) | ||
| .unwrap_or_default(), | ||
| last_event_at: latest.as_ref().map(|event| event.timestamp), | ||
| last_event: latest.as_ref().map(|event| event.event.clone()), | ||
| last_event_is_test: latest | ||
| .as_ref() | ||
| .map(|event| { | ||
| event.session_id.starts_with("agent-cat-test") | ||
| || event.session_id.starts_with("agent-cat-probe") | ||
| }) | ||
| .unwrap_or(false), | ||
| verified_at, | ||
| last_real_event_at: latest_real.as_ref().map(|event| event.timestamp), | ||
| last_real_event: latest_real.as_ref().map(|event| event.event.clone()), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep current-run real-event state independent from verification.
Line 284 suppresses lastRealEvent* whenever verified_at() is None. A received real event can then render as “尚未收到” even though LATEST_REAL_EVENT has it. Return latest_real_event() unconditionally; retain verifiedAt only for the verification state.
Proposed fix
- let latest_real = verified_at.and_then(|_| latest_real_event());
+ let latest_real = latest_real_event();📝 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
| let verified_at = crate::hook_verification::verified_at(); | |
| let latest_real = verified_at.and_then(|_| latest_real_event()); | |
| HookRuntimeStatus { | |
| receiver_running: RECEIVER_RUNNING.load(Ordering::Acquire), | |
| socket_path: socket_path() | |
| .map(|path| path.to_string_lossy().to_string()) | |
| .unwrap_or_default(), | |
| last_event_at: latest.as_ref().map(|event| event.timestamp), | |
| last_event: latest.as_ref().map(|event| event.event.clone()), | |
| last_event_is_test: latest | |
| .as_ref() | |
| .map(|event| { | |
| event.session_id.starts_with("agent-cat-test") | |
| || event.session_id.starts_with("agent-cat-probe") | |
| }) | |
| .unwrap_or(false), | |
| verified_at, | |
| last_real_event_at: latest_real.as_ref().map(|event| event.timestamp), | |
| last_real_event: latest_real.as_ref().map(|event| event.event.clone()), | |
| let verified_at = crate::hook_verification::verified_at(); | |
| let latest_real = latest_real_event(); | |
| HookRuntimeStatus { | |
| receiver_running: RECEIVER_RUNNING.load(Ordering::Acquire), | |
| socket_path: socket_path() | |
| .map(|path| path.to_string_lossy().to_string()) | |
| .unwrap_or_default(), | |
| verified_at, | |
| last_real_event_at: latest_real.as_ref().map(|event| event.timestamp), | |
| last_real_event: latest_real.as_ref().map(|event| event.event.clone()), |
🤖 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 `@src-tauri/src/hook_server.rs` around lines 283 - 292, Update the
HookRuntimeStatus construction in the status function to call
latest_real_event() unconditionally instead of gating it through
hook_verification::verified_at(). Keep verified_at assigned solely to the
verified_at field, while last_real_event_at and last_real_event continue
deriving from the independently retrieved latest real event.
Comment on lines
+75
to
+105
| pub fn record(timestamp: u64) -> Result<(), String> { | ||
| let Some(fingerprint) = hook_installer::verification_fingerprint()? else { | ||
| return Ok(()); | ||
| }; | ||
| let verified_at = RUNTIME_VERIFICATION | ||
| .get_or_init(|| Mutex::new(RuntimeVerification::Uninitialized)) | ||
| .lock() | ||
| .map(|mut state| { | ||
| let verified_at = match &*state { | ||
| RuntimeVerification::Verified(memory) if memory.fingerprint == fingerprint => { | ||
| memory.verified_at | ||
| } | ||
| _ => timestamp, | ||
| }; | ||
| *state = RuntimeVerification::Verified(MemoryVerification { | ||
| fingerprint: fingerprint.clone(), | ||
| verified_at, | ||
| }); | ||
| verified_at | ||
| }) | ||
| .unwrap_or(timestamp); | ||
| let existing = load().unwrap_or_default(); | ||
| if existing.fingerprint.as_deref() == Some(fingerprint.as_str()) | ||
| && existing.verified_at.is_some() | ||
| { | ||
| return Ok(()); | ||
| } | ||
| save(&VerificationState { | ||
| fingerprint: Some(fingerprint), | ||
| verified_at: Some(verified_at), | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep verified_at consistent with persisted verification.
Line 96 finds an existing matching timestamp, but Lines 79-95 have already cached the incoming event timestamp. After restart, the first new event changes verified_at only until the next restart, when it reverts to the old persisted value. Reuse the matching persisted timestamp when initializing runtime memory (or persist the new one consistently); last_real_event already represents per-run activity.
🤖 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 `@src-tauri/src/hook_verification.rs` around lines 75 - 105, Update record so a
matching persisted VerificationState.verified_at is reused when initializing or
updating RuntimeVerification memory, instead of retaining the incoming
timestamp. Keep last_real_event as the per-run activity marker, and ensure
runtime and persisted verification timestamps remain consistent across restarts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Summary by CodeRabbit
New Features
Bug Fixes