firewall: preserve unfinished interactive follow-up work#352
Conversation
|
Checked the current CI signal on this draft. The failing |
|
Scheduled follow-up check complete. There are still no review submissions, requested changes, or unresolved review threads on this draft. CI remains the only concrete blocker: the |
|
Applied the formatting-only fix that This was limited to the two rustfmt rewrites called out by the failing CI log:
I’m leaving the PR as a draft because it is still a preservation branch that is substantially behind |
|
Checked this draft again from the current head. There are still no review submissions or unresolved review threads, but I found a concrete code-level blocker that keeps it in draft territory.
I’m leaving the PR open as the preservation path for the remaining firewall work, but the next useful step is to reconcile the inspector action enum with the handler paths before spending time on broader review or merge readiness. |
|
Checked the draft against the current branch head and confirmed a concrete integration blocker in the firewall UI follow-up. This branch intentionally removes I’m leaving the PR open as the preservation path for the remaining firewall work, but the next useful step is to reconcile the inspector action enum and the stale handler arms in |
|
Follow-up check on the current draft head: the inspector mismatch is narrower and more mechanical than the earlier note suggested, but it is still a real compile/integration blocker.
That leaves the branch head internally inconsistent even before broader review against current |
|
Applied a narrow follow-up fix on Changed:
Why this shape: on the current draft head, Validation used here:
There is still no fresh GitHub Actions run or commit status attached to this new head from this environment, so compile/test confirmation is still pending the next CI run. |
YMRYMR
left a comment
There was a problem hiding this comment.
A couple of blocking issues stood out on review, so I’d keep this in draft for now.
-
The new firewall row-to-inspector interaction looks non-functional as written. In
src/ui/firewall.rs, the selection path hangs offresp.response.clicked()fromui.horizontal(...)andresp.clicked()fromui.label(...), but those widgets/layout responses are hover-only by default in egui. That means the advertised “click any rule for details and actions” flow likely never fires for isolation, blocked IPs, blocked processes, blocked domains, or suspended processes. I’d switch each row to an explicitly clickable widget (selectable_label,Button, or a customSense::click()interaction on the row rect) before this moves forward. -
refresh_inspector_snapshotinsrc/ui/mod.rsnow throttles snapshot fetches even when the selection key changes. Afterkey_changedis detected, the new code only starts a worker whenself.inspector_snapshot_rx.is_none() && self.inspector_snapshot_last_started.elapsed() >= INSPECTOR_SNAPSHOT_REFRESH. That means if the user changes selections inside the refresh window, the inspector can sit blank/stale until the timer expires. Currentmasterbypasses the refresh throttle on key changes; this branch should preserve that behavior.
There are still no review threads or CI signals on the PR, so I’m leaving it as a draft preservation branch rather than trying to merge it yet.
|
Applied a narrow follow-up fix on Changed:
Validation used here:
I did not apply the |
|
Checked the current draft head again. There are still no workflow runs or commit-status checks attached to The issue is the same one called out in review point 2: after The next useful fix is still to let a changed selection bypass that throttle, matching current |
|
Applied the remaining concrete review follow-up on Changed:
Why this shape: the prior branch logic cleared the inspector on Validation used here:
I did not move the PR out of draft. It remains a preservation branch that is substantially behind current |
|
Checked the current draft head ( The earlier That means rapid selection changes should no longer sit behind the refresh window in the way the earlier review comment described. I’m still keeping this as a draft preservation PR and not moving it toward merge from this run because:
So the queue status here is: the old snapshot-throttle blocker appears addressed on the current head, but the PR is still not in a merge-ready state. |
|
Scheduled follow-up on the current draft head ( The earlier blocking review points around clickable firewall rows and
There are still no unresolved review threads or requested changes on the PR. I’m still leaving this in draft and not moving it toward merge from this run because the concrete remaining blockers are repository-state blockers rather than the earlier code-review ones:
So the next useful step is not another small UI follow-up. It is to rebase or otherwise reconcile this preservation branch against current |
|
Checked the current draft head again with the branch-vs- The earlier code-review blockers around clickable firewall rows and the inspector snapshot throttle appear addressed on the current head, and there are still no unresolved review threads or requested changes. The remaining blocker is now branch reconciliation rather than another small follow-up inside the draft. Since merge-base
So the current state is:
I’m keeping this in draft. The next useful step is to rebase or otherwise reconcile |
|
Scheduled follow-up on the current draft head ( The earlier code-review blockers around clickable firewall rows and the inspector snapshot throttle still appear addressed on the current branch state, and there are still no unresolved review threads or requested changes. The remaining blocker is still branch reconciliation rather than another small follow-up inside the draft:
I’m keeping this in draft. The next useful step is still to rebase or otherwise reconcile |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Aligned the PR state with its current readiness and moved it back to draft. Current branch readback still shows the earlier code-review blockers addressed:
I’m not merging from this run because the remaining blockers are branch-state blockers rather than another small code follow-up:
Next useful step: rebase or otherwise reconcile |
|
Scheduled follow-up on the current draft head ( I checked for new review submissions, unresolved review threads, requested changes, commit statuses, and GitHub Actions runs. There are still no unresolved threads or requested changes, and GitHub is still showing no attached commit-status checks or workflow runs for this head from the available signals here. I did not make a code change in this run because there is no new narrow in-scope fix to apply safely. The remaining blocker is still branch-state reconciliation rather than another isolated follow-up inside the draft:
Validation used here:
Next useful step remains to rebase or otherwise reconcile |
|
Checked the current draft head The earlier I’m still leaving this as a draft. There are no unresolved review threads or requested changes at the moment, but GitHub still shows no workflow runs or commit-status checks attached to the current head from this environment, and the PR body still describes this as a preservation branch that is substantially behind |
|
Scheduled follow-up on the current draft head ( I checked for new PR comments, review submissions, unresolved review threads, requested changes, commit-status checks, and GitHub Actions runs. There are still no unresolved threads or requested changes, and GitHub is still showing no attached commit-status checks or workflow runs for this head from the available signals here. I did not make a code change in this run because there is no new narrow in-scope fix to apply safely. The remaining blocker is still broader branch-state reconciliation rather than another isolated follow-up inside the draft:
Validation used here:
I’m keeping this in draft. A rebase or other reconciliation against current |
|
Checked the current draft head again. The earlier inspector refresh blocker from review point 2 is addressed on the branch now. Commit I’m keeping the PR in draft because it is still a preservation branch with no attached CI or commit-status signal on the current head from this environment, and the branch remains substantially behind |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3681710f4
ℹ️ 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".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Checked the current head The two open blockers both still reproduce on the live branch state:
There are also still no attached commit-status checks or workflow runs on this head from the available signals here, and the branch remains diverged from |
|
Scheduled follow-up on the current draft head There are two new unresolved review threads, both in
I did not apply another code change from this scheduled pass. The draft is still substantially behind Validation used here:
|
|
Scheduled maintenance check on the current head
The concrete next step is a focused compatibility pass across the rewritten UI flow and current APIs. The failing log points at the known drift areas: scheduled-lockdown schedule shape, I’m leaving this draft unmerged. |
|
Scheduled maintainer pass complete. Current state from head
The concrete compile blockers I verified from the failing job/code include:
I’m leaving the PR as draft and unmerged. The next useful action is a focused compile-compatibility pass in |
|
Maintainer pass: #352 is still correctly blocked as a draft. The latest head ( I could inspect the PR and CI state through the GitHub connector, but this execution environment cannot clone the repo ( |
|
Maintenance pass update:
Keeping this draft open; it is not merge-ready yet. |
|
Scheduled maintenance pass complete on the current head ( Current state:
The next useful action is still a focused compatibility pass against current |
|
Maintenance pass update:
|
|
Scheduled maintainer check on current head
I’m leaving this as a draft and not merging. The next useful change is a focused compile repair in |
|
Scheduled follow-up check complete. Current head I did not push a fix from this scheduled run because the failure is no longer a narrow formatter issue; the clippy/build log reports a broad compile/API compatibility cluster, and the code confirms multiple in-scope but cross-cutting errors in |
|
Maintenance pass update: this should remain draft for now. Latest CI on I did not merge or mark this ready because the build is not green and the PR is intentionally preserving unfinished work. |
|
Maintainer pass update: leaving this as a draft and not merge-ready. The latest head still has a failing CI
All review threads I saw are resolved or outdated, so the next useful step is a compile-fix pass followed by rerunning CI before this is marked ready for review. |
|
Scheduled follow-up update:
Validation on the latest commit
|
|
Scheduled maintenance check complete. Current head I inspected the remaining failure path and this should stay draft. The next useful compatibility pass is focused in
I did not merge or mark this ready for review. There are no unresolved review threads, but CI still has compile errors and the PR is correctly still a preservation draft. |
|
Scheduled maintenance check complete. Current status on head
The next useful compatibility pass remains focused in I did not push a direct code rewrite from this scheduled run because this environment cannot clone the branch or run local Rust verification, and applying a full-file GitHub contents replacement to |
|
Scheduled maintenance check complete. Current status on head
I rechecked the failure shape against the branch and current I did not push a direct code update from this scheduled run because the environment still cannot clone the repository for a local compile/rustfmt pass, and replacing large files through the GitHub contents API without verification would be too risky for this preservation draft. Next useful action remains a local rebase/compatibility pass with |
|
Scheduled maintenance check on the latest head
I’m leaving this as a draft preservation PR. The next useful action is a focused compatibility pass against current |
|
Maintainer pass update: keeping this PR in draft and unmerged. The latest CI failure is still the right blocker. Review threads are resolved/outdated, but the branch still has compile/API drift to repair before it should be reviewed for merge. Confirmed remaining areas from the current branch/code state:
I could not safely push the compatibility patch from this scheduled run because the runtime cannot clone/download GitHub raw content, and the connector edit path requires whole-file replacement. No merge attempted. |
|
Scheduled maintenance check on the current head ( Two concrete current-head issues are visible from the source and failing log:
I did not push a branch update from this run because this environment cannot materialize a clean checkout or raw file download for a safe full-file update, and the connector returns this large UI file truncated in the chat view. I’m leaving the PR as a draft and not merging; the next useful pass is a small compatibility patch in |
|
Scheduled maintenance check complete on current head Current state:
The compile blocker is broader than the last source-only note captured. The current failing areas are:
I did not push a branch update from this run because this environment still cannot clone or fetch raw GitHub content, and the available connector edit path would require unverified whole-file replacement for large UI files. Next useful action remains a local compatibility patch against the PR merge ref followed by |
|
Maintainer pass update: I rechecked the open PR queue and this is still the only open PR. It should remain draft. Current blocker is still the
I did not merge or mark ready. The next useful fix is a focused compatibility pass that unifies the schedule type usage, updates the closed-connection handler to the current event shape, and wires the remaining inspector actions before rerunning CI. |
|
CI is still blocked on compile/API compatibility rather than a flaky check. The failing workflow is The current errors I verified from the log are concentrated in
I could not safely push a fix from this scheduled run because this environment cannot clone or download the repository source from GitHub, so I could not run local verification after editing. The next merge-moving step is a focused compatibility pass on those two files, then rerun |
|
Maintainer pass update: leaving this as draft and not merge-ready. The current blocker is still the
I did not merge or mark this ready. The next useful change is a narrow compatibility patch in |
|
Maintenance pass update:
I did not merge this PR. The next useful step is still a focused compile/API compatibility patch, followed by rerunning CI and then rechecking the earlier review-body concerns before marking it ready for review. |
…lockdown logic (#362) * Respect scheduled lockdown days * Handle overnight scheduled lockdown days * Clear Linux tray alert target on reset
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c629a49573
ℹ️ 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".
| #[path = "active_response_platform.rs"] | ||
| mod platform; |
There was a problem hiding this comment.
Keep the platform module at the same parent depth
On Linux, moving the implementation behind active_response::imp changes this #[path] module's parent chain: active_response_platform.rs still imports super::super::linux_command_plan/linux_firewall_backend/linux_firewall_executor, which now resolves under security::active_response instead of security. I checked cargo check --all-targets on this commit and it fails before building the app with unresolved linux_* imports.
Useful? React with 👍 / 👎.
| pub use imp::{ | ||
| block_domain, block_process, block_remote, can_apply_quarantine_profile, can_block_domain, | ||
| can_isolate_network, can_kill_connection, can_modify_firewall, can_suspend_process, | ||
| extract_domain_target, extract_remote_target, freeze_autoruns, has_frozen_autoruns, is_blocked, | ||
| is_domain_blocked, is_process_blocked, is_process_suspended, isolate_machine, kill_connection, |
There was a problem hiding this comment.
Preserve the public active-response API exports
The new facade omits public functions that existing callers still use: src/security/response_rules.rs:598 calls active_response::apply_quarantine_profile, and src/autoruns.rs:13 calls active_response::revert_frozen_autoruns. Since the functions only exist inside private imp, cargo check --all-targets fails with unresolved function errors until these are re-exported or all callers are updated.
Useful? React with 👍 / 👎.
| ConnEvent::New(info) => { | ||
| self.add_conn_to_activity(info); | ||
| self.trim_history_buffers(); | ||
| self.cached_activity_process_count = process_list::process_count(&self.activity); |
There was a problem hiding this comment.
Remove calls to missing process-list helpers
This event path now calls process_list::process_count, but src/ui/process_list.rs does not define or export such a function (only a CachedGroupView::process_count field exists). The same unresolved call is repeated for alerts and closed events, so any normal build of the UI fails at compile time before these handlers can run.
Useful? React with 👍 / 👎.
| ConnEvent::Closed { | ||
| pid, | ||
| proc_name, | ||
| local_addr, | ||
| remote_addr, |
There was a problem hiding this comment.
Match the current ConnEvent::Closed fields
ConnEvent::Closed is defined with { pid, local, remote, snapshot }, but this new handler destructures proc_name, local_addr, and remote_addr. I checked the enum in src/types.rs, and this pattern does not compile, so closed-connection handling currently blocks the entire UI build.
Useful? React with 👍 / 👎.
| r"\\Temp\\", | ||
| r"\\AppData\\Local\\Temp\\", | ||
| r"\\AppData\\Roaming\\", | ||
| r"\\Downloads\\", | ||
| r"\\Public\\", |
There was a problem hiding this comment.
Restore single-backslash Windows path fragments
These defaults now contain doubled backslashes, but score.rs compares them with path_upper.contains(&frag.to_uppercase()) against normal Windows paths such as C:\Users\...\AppData\Local\Temp\..., which contain single separators. As a result, the default suspicious Windows drop locations no longer match and lose the intended +3 scoring signal for processes launched from Temp/AppData/Downloads/Public.
Useful? React with 👍 / 👎.
Summary
phase19-p5-ep2firewall follow-up work in a reviewable pathWhy
This branch still contains meaningful unmerged work, but it is not merge-ready. Keeping it as a draft PR preserves the work in a reviewable path without implying the branch is ready to land as-is.
Current branch scope
Based on comparison against current
master, this branch changes:src/autoruns.rssrc/config.rssrc/main.rssrc/platform/tray.rssrc/security/active_response.rssrc/security/active_response_impl.rssrc/security/firewall/mod.rssrc/ui/firewall.rssrc/ui/inspector.rssrc/ui/mod.rssrc/ui/settings/draft.rssrc/ui/uninstall.rssrc/yara_rules.rssrc/yara_scan.rsCurrent status
cargo fmt --checkpasses on the attached CI runcargo clippyfails on the current PR merge ref with compile/API compatibility errors, so build/test jobs are skippedConnEvent::Closeddestructuring, a missingactive_response::suspend_processargument, a non-exhaustivehandle_inspector_actionmatch, and ownedStringpayload mismatches insrc/ui/inspector.rsNext useful action
Run a focused compatibility pass between the rewritten UI flow and the current
Config,ConnEvent,inspector::Action, andactive_responseAPIs, then rerun CI before considering this for review or merge.