Improve process page data handling and coverage - #1261
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe process model now tracks identity, RSS, structured parsing issues, and direction-aware sorting. Unix and Windows scripts emit updated metrics, framed output, and safely quoted paths. Server refreshes reject stale results and copy mutable state. The process page now uses a responsive sortable table with refresh, formatting, and termination controls. Tests and localization cover the updated behavior. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/view/page/process.dart (2)
103-108: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSet
_hasLoadedon the disconnected path.When
_canRunProcessCmdreturns false, the method returns without setting_hasLoaded._buildProcessContentthen keeps returningUIs.centerLoadingbecause_hasLoadedis false and_result.procsis empty. A user who opens the page while the server is disconnected sees an endless spinner and no explanation.Set
_hasLoaded = truebefore the return so the empty state appears.🐛 Proposed fix
if (!_canRunProcessCmd(serverState)) { + _hasLoaded = true; if (userTriggered && mounted) { context.showSnackBar(libL10n.disconnected); } return; }🤖 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 `@lib/view/page/process.dart` around lines 103 - 108, Set _hasLoaded to true in the !_canRunProcessCmd branch before returning, while preserving the existing userTriggered snackbar behavior, so _buildProcessContent renders the disconnected empty state instead of an endless loading indicator.
109-117: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNo deadline on the remote command calls. Both remote calls in this file await
client.runwith no timeout, and both hold the shared_isRefreshingflag while they wait. If the SSH channel stalls, the flag staystrue, the refresh button and every row stop button stay disabled, and the periodic timer cannot start a new refresh.
lib/view/page/process.dart#L109-L117: bound the process-listing call with a timeout so thefinallyblock always clears_isRefreshing.lib/view/page/process.dart#L632-L646: bound the kill call at line 635 with a timeout. This path is worse, because_confirmKillwraps it incontext.showLoadingDialog, so a stalled call leaves a modal spinner that the user cannot dismiss.Use one shared constant for the duration and handle
TimeoutExceptionin the existingcatchblocks.🤖 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 `@lib/view/page/process.dart` around lines 109 - 117, In lib/view/page/process.dart lines 109-117, add a shared timeout-duration constant and apply it to the process-listing client.run call, handling TimeoutException in the existing catch block while preserving finally cleanup of _isRefreshing. Apply the same shared constant and timeout handling to the kill client.run call at lines 632-646 within _confirmKill, ensuring the loading dialog is dismissed through the existing error path.
🧹 Nitpick comments (4)
test/proc_test.dart (1)
246-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a test for the PID tie-breaker.
_sortfalls back toa.pid.compareTo(b.pid)when the primary keys are equal. No test covers that path. A test with two processes that share the samecpuvalue would lock in the stable order and prevent a regression when the comparator changes.🤖 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 `@test/proc_test.dart` around lines 246 - 266, Add a test in the existing process sorting tests for ProcSortMode.cpu using two processes with equal CPU values and different PIDs, asserting the result is ordered by PID through the comparator tie-breaker. Keep the test focused on _sort’s equal-primary-key behavior.lib/view/page/process.dart (3)
284-382: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe column layout is declared twice.
_buildHeaderand_buildProcessRowat lines 400-506 each repeat the same sequence of columns, widths, gaps, and visibility conditions. The two lists currently agree. Any future change must be applied in both places, and a mismatch misaligns the header from the rows without any compile-time error.Define the columns once as a list of descriptors and build the header and the rows from it.
♻️ Sketch
class _ProcColumn { const _ProcColumn({ required this.width, required this.label, required this.mode, required this.visible, required this.value, this.alignEnd = false, }); final double width; final String label; final ProcSortMode mode; final bool visible; final String Function(Proc) value; final bool alignEnd; }Build one
List<_ProcColumn>from_ProcessLayout, then map it to header cells and to row cells.🤖 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 `@lib/view/page/process.dart` around lines 284 - 382, The process column layout is duplicated between _buildHeader and _buildProcessRow, allowing widths, visibility, and ordering to diverge. Introduce a shared _ProcColumn descriptor and build one column list from _ProcessLayout, including each column’s width, label, sort mode, visibility, value accessor, and alignment; use that list to generate both header cells and row cells while preserving the existing order and conditional visibility.
127-131: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe result is sorted twice on every refresh.
PsResult.parsesorts internally using its defaultsortofProcSortMode.cpu. Line 129 then sorts the same list again with the selected mode and direction. Each refresh performs two O(n log n) sorts on a list that can hold hundreds of processes, and the timer repeats this on every interval.Pass the selected mode and direction to
parseand drop the second sort.♻️ Proposed refactor
- var parsed = PsResult.parse(result, previous: _result); - _updateCapabilities(parsed); - parsed = parsed.sortedBy(_procSortMode, ascending: _sortAscending); - _result = parsed; + var parsed = PsResult.parse( + result, + previous: _result, + sort: _procSortMode, + ascending: _sortAscending, + ); + _updateCapabilities(parsed); + // `_updateCapabilities` can change `_procSortMode` when the current mode + // is unsupported, so re-sort only in that case. + if (parsed.procs.isNotEmpty) { + parsed = parsed.sortedBy(_procSortMode, ascending: _sortAscending); + } + _result = parsed;A cleaner variant is to derive the capabilities from the raw parse before sorting, then sort once.
🤖 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 `@lib/view/page/process.dart` around lines 127 - 131, Update the refresh flow around PsResult.parse to pass _procSortMode and _sortAscending into parsing, then remove the subsequent parsed.sortedBy call so each refresh sorts only once. Preserve _updateCapabilities, _result assignment, and loading-state behavior; derive capabilities from the parsed result as currently required.
539-549: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Btnx.cancelOkand handle confirmation after the dialog closes. AwaitshowRoundDialog<bool>and call_killAndRefresh(proc.pid)after it returnstrue;Btnx.cancelOkdoes not accept the current customonTapcallback.🤖 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 `@lib/view/page/process.dart` around lines 539 - 549, Replace the custom Btn.cancel/Btn.ok actions in the process-kill dialog with Btnx.cancelOk, and await showRoundDialog<bool> for the user’s result. After the dialog closes, call _killAndRefresh(proc.pid) only when the returned value is true, preserving the loading-dialog behavior around the refresh.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 `@lib/view/page/process.dart`:
- Around line 590-626: Split _ProcessPageState into separate extension on blocks
according to responsibility: place build and all _build* widget methods in
_ProcessPageStateWidget, move _confirmKill and _killAndRefresh into an actions
extension, and move _formatPercent, _formatCpu, _formatRss,
_formatNullableSpeed, _formatSpeed, _canRunProcessCmd, and _killProcessCmd into
a utils extension. Keep the existing state fields on _ProcessPageState and
preserve method behavior.
- Around line 248-250: Update the empty-state branch in the process-list build
flow around _result.procs.isEmpty to wrap the CenterGreyTitle content in a
RefreshIndicator backed by an always-scrollable view, reusing the existing
refresh callback. Preserve the current empty-state styling and horizontal
padding while enabling pull-to-refresh when no processes are present.
- Around line 419-473: Update the metric Text widgets in the CPU, memory, RSS,
read-speed, and write-speed cells to match the existing overflow handling used
by the user and command cells: constrain each to one line and apply the
established overflow behavior. Preserve their fixed widths, formatting,
alignment, and styles.
- Around line 566-567: Update the VSZ detail rendering near _formatRss so
proc.vsz is displayed with an explicit, human-readable unit consistent with RSS.
Add and use a _formatVsz(Proc proc) helper that parses the KiB value, converts
it to bytes for formatting, and preserves the original value or an em dash when
parsing fails or is unavailable.
- Around line 221-227: Add a count-aware localized process-count key to the app
ARB localization resources, then update the process-count Text in the
surrounding page widget to format _result.procs.length through that key instead
of concatenating it with libL10n.process. Preserve the existing styling and use
the generated localization accessor consistently.
- Around line 780-808: Update _SortHeader’s Semantics label so active sort
headers include the localized l10n.ascending or l10n.descending text based on
ascending; keep inactive labels unchanged and retain the decorative arrow Icon
without adding a separate semantic label.
- Around line 628-631: Update _killAndRefresh so a confirmed kill request is not
discarded when _isRefreshing is already true: wait for the in-flight _refresh()
to complete, then execute _killProcessCmd and continue the refresh flow.
Preserve the mounted check and existing loading-dialog feedback, without relying
on a nonexistent libL10n.busy key.
---
Outside diff comments:
In `@lib/view/page/process.dart`:
- Around line 103-108: Set _hasLoaded to true in the !_canRunProcessCmd branch
before returning, while preserving the existing userTriggered snackbar behavior,
so _buildProcessContent renders the disconnected empty state instead of an
endless loading indicator.
- Around line 109-117: In lib/view/page/process.dart lines 109-117, add a shared
timeout-duration constant and apply it to the process-listing client.run call,
handling TimeoutException in the existing catch block while preserving finally
cleanup of _isRefreshing. Apply the same shared constant and timeout handling to
the kill client.run call at lines 632-646 within _confirmKill, ensuring the
loading dialog is dismissed through the existing error path.
---
Nitpick comments:
In `@lib/view/page/process.dart`:
- Around line 284-382: The process column layout is duplicated between
_buildHeader and _buildProcessRow, allowing widths, visibility, and ordering to
diverge. Introduce a shared _ProcColumn descriptor and build one column list
from _ProcessLayout, including each column’s width, label, sort mode,
visibility, value accessor, and alignment; use that list to generate both header
cells and row cells while preserving the existing order and conditional
visibility.
- Around line 127-131: Update the refresh flow around PsResult.parse to pass
_procSortMode and _sortAscending into parsing, then remove the subsequent
parsed.sortedBy call so each refresh sorts only once. Preserve
_updateCapabilities, _result assignment, and loading-state behavior; derive
capabilities from the parsed result as currently required.
- Around line 539-549: Replace the custom Btn.cancel/Btn.ok actions in the
process-kill dialog with Btnx.cancelOk, and await showRoundDialog<bool> for the
user’s result. After the dialog closes, call _killAndRefresh(proc.pid) only when
the returned value is true, preserving the loading-dialog behavior around the
refresh.
In `@test/proc_test.dart`:
- Around line 246-266: Add a test in the existing process sorting tests for
ProcSortMode.cpu using two processes with equal CPU values and different PIDs,
asserting the result is ordered by PID through the comparator tie-breaker. Keep
the test focused on _sort’s equal-primary-key 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b8a23ad0-ab5e-42f4-9730-b32091a90ce9
📒 Files selected for processing (3)
lib/data/model/server/proc.dartlib/view/page/process.darttest/proc_test.dart
![winnowl[bot]](https://avatars.githubusercontent.com/in/4312382?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🛠️ To have the bot fix these findings, comment @winnowl fix.
🔎 Confirmed findings (2)
- 🟡 Medium Windows working-set bytes are converted to RSS KiB by rounding up, which does not preserve the byte value in the declared common unit and collapses distinct values into the same RSS. A process with WorkingSet=1 byte is reported as rssKb=1 (and displayed as 1024 bytes), while Unix RSS=1 denotes 1 KiB; values 512 and 1024 bytes both sort as 1 KiB. This can misstate and misorder small working sets; conversion should use a documented truncation/precision policy rather than unconditional ceiling. The claim would be false if the product explicitly defines RSS KiB as a nonzero ceiling bucket for Windows rather than a unit conversion. (inline)
- 🟡 Medium The confirmation action trusts the PID captured from the row without verifying that it still denotes the same current process. A user can open confirmation for PID X, wait for an auto-refresh (or for PID X to exit and be reused), then confirm;
_killAndRefresh(proc.pid)unconditionally executeskill X/taskkill /PID X, potentially terminating a different process than the one displayed. There is no snapshot/generation check or relookup of the current row/process identity before the destructive command. This is disproven only if the supported servers guarantee PID uniqueness for the entire lifetime of the dialog, which normal PID reuse does not. (inline)
📋 Additional findings from this change (not shown inline) (13)
- 🟡 Medium Incomplete or malformed Windows JSON items with a missing/unparseable PID are silently discarded instead of contributing row-level diagnostics. For example, a JSON array containing a valid process and
{ "ProcessName": "broken" }returns the valid process witherror == null, so callers cannot distinguish a complete sample from one whose item was dropped. This violates the obligation to retain useful diagnostics while returning partial results. The claim would be false if the upstream contract guarantees every JSON item always has a valid Id/ProcessId (including all malformed/incomplete cases). (lib/data/model/server/proc.dart) — anchor-outside-diff - 🟡 Medium A refresh that starts while the server is disconnected can leave the page permanently in its initial loading state.
_refreshsets_isRefreshingbut returns from the_canRunProcessCmdbranch without setting_hasLoaded, and_buildProcessContentrendersUIs.centerLoadingwhenever_hasLoadedis false and the process list is empty. Repro: open the page while the provider has a null/closed client or a disconnected connection; the initial refresh returns and the page remains a spinner rather than showing empty/disconnected state (unless a later lifecycle/timer refresh succeeds). This is disproven only if every initial disconnected state is guaranteed to be followed by a successful refresh before the page is rendered. (lib/view/page/process.dart) — anchor-outside-diff - 🟡 Medium The destructive kill path does not revalidate the client/connection state before issuing the command.
_killAndRefreshreads the current state, derives the system, and callsserverState.client?.run(...)directly; unlike refresh, it never checks_canRunProcessCmd. Thus a stale confirmation dialog can be accepted after disconnect/reconnect, and a still-open client can receivetaskkill/killeven when the provider connection is no longer in an allowed state. The nullable call can also silently do nothing when the client is null, after which the method proceeds to refresh and may show a misleading empty-success flow. This is disproven only if client state cannot change between row confirmation and command execution, which conflicts with the page's refresh/disconnect lifecycle. (lib/view/page/process.dart) — anchor-outside-diff - 🟡 Medium Unknown RSS values are displayed as a literal hyphen instead of the page's unknown marker.
_parseRssKbreturns null forrss == '-', but_formatRssthen returnsproc.rsswhenever the parsed value is null, so a valid UnixRSSplaceholder-renders-in both rows and details rather than—. This is misleading and inconsistent with CPU/MEM/IO unknown formatting. The claim is false only if the process command can never emit-for RSS, although the parser explicitly handles that value and partial platform formats are documented. (lib/view/page/process.dart) — per-file-budget - 🟡 Medium Process command failures are treated as successful empty snapshots, and kill command failures are treated as successful kills. The refresh path reads only
.stringand never checks the command result'sexitCode; a failedSbProcesswith no stdout enters the empty-result branch and replaces the list with an empty state (and may showempty), while_killAndRefreshawaitsrun(...)without inspecting its exit code and then refreshes. Consequently permission-denied/unsupported-command failures can be presented as empty or successful rather than an error/retry state. This is disproven only if the client throws for every nonzero remote exit code, but its result type explicitly exposes nullableexitCodeand other callers inspect it. (lib/view/page/process.dart) — per-file-budget - 🟡 Medium The malformed-input test asserts only an unsupported header and never asserts partial-result plus diagnostics for malformed rows, nor mixed valid/invalid Windows JSON items. Consequently, a regression that silently drops a bad Unix row or Windows item (instead of retaining valid processes and exposing
PsResult.error) would pass this suite, despite partial-result/error behavior being an explicit obligation. This is false if another test file covers malformed rows and mixed JSON items; no such coverage is present in the provided process tests. (test/proc_test.dart) — anchor-outside-diff - 🟡 Medium Malformed or structurally invalid entries in an otherwise valid JSON snapshot are silently discarded instead of being reported in
PsResult.error: non-map items and maps without a parseableId/ProcessIdhitcontinuebefore the per-row error collection. A mixed snapshot such as[validProcess, {"ProcessName":"broken"}, 17]therefore renders as a clean partial result, hiding that process data was lost. (lib/data/model/server/proc.dart) — anchor-outside-diff - 🟡 Medium I/O speed is matched by PID alone, so a PID-reused process can inherit the old process's counters and be reported with a false read/write rate. For example, if PID 1's first snapshot has start=10:00 and readBytes=1000, then after PID 1 exits and is reused with start=10:05 and readBytes=2000, the second parse uses the old Proc from
previousByPid[pid]and reports 1000 bytes/sec rather than null. The existing Unix rows expose START when available, but that field is never compared before_calculateSpeeds; the tests cover rollback and new PIDs, not PID reuse. This would be disproven if the process command guarantees that a PID cannot be reused between snapshots, or if callers always discard snapshots spanning process replacement. (lib/data/model/server/proc.dart) — per-file-budget - 🟡 Medium Unix command parsing cannot preserve argument boundaries or command text containing whitespace/quotes/backslashes. Every data row is split on whitespace and reconstructed with
join(' '), so an args string such as/bin/sh -c 'echo hi'becomes/bin/sh -c 'echo hi'; quoted spaces are treated as ordinary separators and repeated spaces are lost. ConsequentlyProc.command,binary, andargsare not the actual command line despite the tests asserting command/args display semantics. This would be disproven if the process-output contract intentionally defines COMMAND as normalized whitespace tokens rather than the original command line. (lib/data/model/server/proc.dart) — per-file-budget - 🟡 Medium Optional numeric Unix columns are not actually nullable when present: a row containing the conventional
-placeholder in%CPUor%MEMthrows and is dropped._parseusesdouble.parse(parts[map.cpu!])anddouble.parse(parts[map.mem!]), whereas the model and parser otherwise use nullable placeholders (-for I/O and RSS) and the tests specifically exercise optional layouts. For example,PID USER %CPU %MEM COMMAND\n1 root - - fooreturns no Proc and an error instead of one process with null CPU/MEM. This would be disproven if every supported Unixpsoutput guarantees numeric CPU/MEM whenever those headers are present. (lib/data/model/server/proc.dart) — per-file-budget - 🟡 Medium Windows field fallbacks are not applied when the preferred property is present but unusable. The expressions
raw['IOReadBytes'] ?? raw['ReadTransferCount'],raw['IOWriteBytes'] ?? raw['WriteTransferCount'],raw['WorkingSet'] ?? raw['WorkingSetSize'], andraw['Id'] ?? raw['ProcessId']select the first non-null value before parsing; a PowerShell/compatibility payload such as{ "Id": "-", "ProcessId": 42, "WorkingSet": "-", "WorkingSetSize": 2048 }is discarded (or gets null RSS) despite valid fallback fields. The claim would be false only if the Windows JSON contract guarantees preferred keys are absent, never null/placeholder, whenever fallback keys are supplied. (lib/data/model/server/proc.dart) — anchor-unreliable - 🔵 Low The speed tests do not establish that the denominator is the current-minus-previous sample time, or that non-positive elapsed intervals yield null speeds. They also do not distinguish per-PID predecessor matching from accidental global/row-position matching (the fixture keeps the same PID order). A regression using a fixed interval, accepting zero/negative elapsed time, or pairing counters by position would therefore pass. This is false if other tests exercise reordered snapshots and zero/negative timestamps; the provided suite has no such cases. (test/proc_test.dart) — anchor-outside-diff
- 🔵 Low The Windows JSON test does not verify the documented alternate field names (
ProcessId,Name/Path,WorkingSetSize,ReadTransferCount, andWriteTransferCount). A regression in any alias path would leave the suite green even though a supported PowerShell snapshot would lose PID, command, RSS, or I/O data (and PID parsing can make the item disappear). This is false if another test outside this scope supplies an equivalent alias-only snapshot, buttest/proc_test.dartcontains no such case. (test/proc_test.dart) — anchor-outside-diff
🤖 Prompt for AI agents — all findings (15)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Findings on this change (also posted as inline comments) (2)
In lib/data/model/server/proc.dart around line 185, address this finding:
Windows working-set bytes are converted to RSS KiB by rounding up, which does not preserve the byte value in the declared common unit and collapses distinct values into the same RSS. A process with WorkingSet=1 byte is reported as rssKb=1 (and displayed as 1024 bytes), while Unix RSS=1 denotes 1 KiB; values 512 and 1024 bytes both sort as 1 KiB. This can misstate and misorder small working sets; conversion should use a documented truncation/precision policy rather than unconditional ceiling. The claim would be false if the product explicitly defines RSS KiB as a nonzero ceiling bucket for Windows rather than a unit conversion.
In lib/view/page/process.dart around line 542, address this finding:
The confirmation action trusts the PID captured from the row without verifying that it still denotes the same current process. A user can open confirmation for PID X, wait for an auto-refresh (or for PID X to exit and be reused), then confirm; `_killAndRefresh(proc.pid)` unconditionally executes `kill X`/`taskkill /PID X`, potentially terminating a different process than the one displayed. There is no snapshot/generation check or relookup of the current row/process identity before the destructive command. This is disproven only if the supported servers guarantee PID uniqueness for the entire lifetime of the dialog, which normal PID reuse does not.
## Additional findings on this change (not posted inline) (13)
In lib/data/model/server/proc.dart around line 333, address this finding:
Incomplete or malformed Windows JSON items with a missing/unparseable PID are silently discarded instead of contributing row-level diagnostics. For example, a JSON array containing a valid process and `{ "ProcessName": "broken" }` returns the valid process with `error == null`, so callers cannot distinguish a complete sample from one whose item was dropped. This violates the obligation to retain useful diagnostics while returning partial results. The claim would be false if the upstream contract guarantees every JSON item always has a valid Id/ProcessId (including all malformed/incomplete cases).
In lib/view/page/process.dart around line 103, address this finding:
A refresh that starts while the server is disconnected can leave the page permanently in its initial loading state. `_refresh` sets `_isRefreshing` but returns from the `_canRunProcessCmd` branch without setting `_hasLoaded`, and `_buildProcessContent` renders `UIs.centerLoading` whenever `_hasLoaded` is false and the process list is empty. Repro: open the page while the provider has a null/closed client or a disconnected connection; the initial refresh returns and the page remains a spinner rather than showing empty/disconnected state (unless a later lifecycle/timer refresh succeeds). This is disproven only if every initial disconnected state is guaranteed to be followed by a successful refresh before the page is rendered.
In lib/view/page/process.dart around line 635, address this finding:
The destructive kill path does not revalidate the client/connection state before issuing the command. `_killAndRefresh` reads the current state, derives the system, and calls `serverState.client?.run(...)` directly; unlike refresh, it never checks `_canRunProcessCmd`. Thus a stale confirmation dialog can be accepted after disconnect/reconnect, and a still-open client can receive `taskkill`/`kill` even when the provider connection is no longer in an allowed state. The nullable call can also silently do nothing when the client is null, after which the method proceeds to refresh and may show a misleading empty-success flow. This is disproven only if client state cannot change between row confirmation and command execution, which conflicts with the page's refresh/disconnect lifecycle.
In lib/view/page/process.dart around line 606, address this finding:
Unknown RSS values are displayed as a literal hyphen instead of the page's unknown marker. `_parseRssKb` returns null for `rss == '-'`, but `_formatRss` then returns `proc.rss` whenever the parsed value is null, so a valid Unix `RSS` placeholder `-` renders `-` in both rows and details rather than `—`. This is misleading and inconsistent with CPU/MEM/IO unknown formatting. The claim is false only if the process command can never emit `-` for RSS, although the parser explicitly handles that value and partial platform formats are documented.
In lib/view/page/process.dart around line 109, address this finding:
Process command failures are treated as successful empty snapshots, and kill command failures are treated as successful kills. The refresh path reads only `.string` and never checks the command result's `exitCode`; a failed `SbProcess` with no stdout enters the empty-result branch and replaces the list with an empty state (and may show `empty`), while `_killAndRefresh` awaits `run(...)` without inspecting its exit code and then refreshes. Consequently permission-denied/unsupported-command failures can be presented as empty or successful rather than an error/retry state. This is disproven only if the client throws for every nonzero remote exit code, but its result type explicitly exposes nullable `exitCode` and other callers inspect it.
In test/proc_test.dart around line 274, address this finding:
The malformed-input test asserts only an unsupported header and never asserts partial-result plus diagnostics for malformed rows, nor mixed valid/invalid Windows JSON items. Consequently, a regression that silently drops a bad Unix row or Windows item (instead of retaining valid processes and exposing `PsResult.error`) would pass this suite, despite partial-result/error behavior being an explicit obligation. This is false if another test file covers malformed rows and mixed JSON items; no such coverage is present in the provided process tests.
In lib/data/model/server/proc.dart around line 329, address this finding:
Malformed or structurally invalid entries in an otherwise valid JSON snapshot are silently discarded instead of being reported in `PsResult.error`: non-map items and maps without a parseable `Id`/`ProcessId` hit `continue` before the per-row error collection. A mixed snapshot such as `[validProcess, {"ProcessName":"broken"}, 17]` therefore renders as a clean partial result, hiding that process data was lost.
In lib/data/model/server/proc.dart around line 231, address this finding:
I/O speed is matched by PID alone, so a PID-reused process can inherit the old process's counters and be reported with a false read/write rate. For example, if PID 1's first snapshot has start=10:00 and readBytes=1000, then after PID 1 exits and is reused with start=10:05 and readBytes=2000, the second parse uses the old Proc from `previousByPid[pid]` and reports 1000 bytes/sec rather than null. The existing Unix rows expose START when available, but that field is never compared before `_calculateSpeeds`; the tests cover rollback and new PIDs, not PID reuse. This would be disproven if the process command guarantees that a PID cannot be reused between snapshots, or if callers always discard snapshots spanning process replacement.
In lib/data/model/server/proc.dart around line 152, address this finding:
Unix command parsing cannot preserve argument boundaries or command text containing whitespace/quotes/backslashes. Every data row is split on whitespace and reconstructed with `join(' ')`, so an args string such as `/bin/sh -c 'echo hi'` becomes `/bin/sh -c 'echo hi'`; quoted spaces are treated as ordinary separators and repeated spaces are lost. Consequently `Proc.command`, `binary`, and `args` are not the actual command line despite the tests asserting command/args display semantics. This would be disproven if the process-output contract intentionally defines COMMAND as normalized whitespace tokens rather than the original command line.
In lib/data/model/server/proc.dart around line 140, address this finding:
Optional numeric Unix columns are not actually nullable when present: a row containing the conventional `-` placeholder in `%CPU` or `%MEM` throws and is dropped. `_parse` uses `double.parse(parts[map.cpu!])` and `double.parse(parts[map.mem!])`, whereas the model and parser otherwise use nullable placeholders (`-` for I/O and RSS) and the tests specifically exercise optional layouts. For example, `PID USER %CPU %MEM COMMAND\n1 root - - foo` returns no Proc and an error instead of one process with null CPU/MEM. This would be disproven if every supported Unix `ps` output guarantees numeric CPU/MEM whenever those headers are present.
In lib/data/model/server/proc.dart, address this finding:
Windows field fallbacks are not applied when the preferred property is present but unusable. The expressions `raw['IOReadBytes'] ?? raw['ReadTransferCount']`, `raw['IOWriteBytes'] ?? raw['WriteTransferCount']`, `raw['WorkingSet'] ?? raw['WorkingSetSize']`, and `raw['Id'] ?? raw['ProcessId']` select the first non-null value before parsing; a PowerShell/compatibility payload such as `{ "Id": "-", "ProcessId": 42, "WorkingSet": "-", "WorkingSetSize": 2048 }` is discarded (or gets null RSS) despite valid fallback fields. The claim would be false only if the Windows JSON contract guarantees preferred keys are absent, never null/placeholder, whenever fallback keys are supplied.
In test/proc_test.dart around line 65, address this finding:
The speed tests do not establish that the denominator is the current-minus-previous sample time, or that non-positive elapsed intervals yield null speeds. They also do not distinguish per-PID predecessor matching from accidental global/row-position matching (the fixture keeps the same PID order). A regression using a fixed interval, accepting zero/negative elapsed time, or pairing counters by position would therefore pass. This is false if other tests exercise reordered snapshots and zero/negative timestamps; the provided suite has no such cases.
In test/proc_test.dart around line 141, address this finding:
The Windows JSON test does not verify the documented alternate field names (`ProcessId`, `Name`/`Path`, `WorkingSetSize`, `ReadTransferCount`, and `WriteTransferCount`). A regression in any alias path would leave the suite green even though a supported PowerShell snapshot would lose PID, command, RSS, or I/O data (and PID parsing can make the item disappear). This is false if another test outside this scope supplies an equivalent alias-only snapshot, but `test/proc_test.dart` contains no such case.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- 3 of 3 areas reviewed
| // same unit so sorting and display stay consistent across platforms. | ||
| rss: workingSetBytes == null | ||
| ? null | ||
| : ((workingSetBytes + 1023) ~/ 1024).toString(), |
There was a problem hiding this comment.
🔍 Data Integrity | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
⚠️ The repository does not state whether Windows RSS is intentionally defined as a ceiling bucket; the implementation comment only says it is normalized to the Unix KiB unit, and the existing test confirms the ceiling behavior rather than documenting that policy.
🤖 Prompt for AI agents
In lib/data/model/server/proc.dart, address this finding:
Windows working-set bytes are converted to RSS KiB by rounding up, which does not preserve the byte value in the declared common unit and collapses distinct values into the same RSS. A process with WorkingSet=1 byte is reported as rssKb=1 (and displayed as 1024 bytes), while Unix RSS=1 denotes 1 KiB; values 512 and 1024 bytes both sort as 1 KiB. This can misstate and misorder small working sets; conversion should use a documented truncation/precision policy rather than unconditional ceiling. The claim would be false if the product explicitly defines RSS KiB as a nonzero ceiling bucket for Windows rather than a unit conversion.
Deploying sbmd with
|
| Latest commit: |
1ad8e95
|
| Status: | 🚫 Build failed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🛠️ To have the bot fix these findings, comment @winnowl fix.
🔎 Confirmed findings (1)
- 🟡 Medium I/O capability detection is based on sampled speeds rather than the presence of I/O counters. On the first valid sample, all readSpeed/writeSpeed values are necessarily null, so the process UI hides the read/write columns and rejects those sort modes even though the input provides supported counters and the parser has populated readBytes/writeBytes. (inline)
📋 Additional findings from this change (not shown inline) (2)
- 🟡 Medium Snapshot correlation uses PID alone, so a restarted process that reuses a PID can inherit the previous process's I/O counters and report a false positive speed. (lib/data/model/server/proc.dart) — anchor-outside-diff
- 🟡 Medium The Windows process command emits
Get-Process'sCPUproperty, which is cumulative CPU time in seconds, but the process model exposes it ascpuand the page treats it as the CPU metric used for the default CPU sort and column. As a result, Windows processes are ranked by lifetime CPU seconds rather than current CPU usage, so the default view can surface old CPU-heavy processes instead of currently busy ones. The UI'sssuffix makes the unit explicit but does not fix the sorting/metric contract. (lib/data/model/app/scripts/script_builders.dart) — anchor-outside-diff
♻️ Previously reported (still present) (1)
- 🟡 Medium Windows JSON rows with malformed or missing process IDs are silently discarded, and malformed JSON falls through as Unix text instead of producing a displayable parse error; consequently invalid Windows input can appear as a successful empty/unsupported result with no error metadata. (lib/data/model/server/proc.dart) — previously-reported
🤖 Prompt for AI agents — all findings (4)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Findings on this change (also posted as inline comments) (1)
In lib/view/page/process.dart around line 715, address this finding:
I/O capability detection is based on sampled speeds rather than the presence of I/O counters. On the first valid sample, all readSpeed/writeSpeed values are necessarily null, so the process UI hides the read/write columns and rejects those sort modes even though the input provides supported counters and the parser has populated readBytes/writeBytes.
## Additional findings on this change (not posted inline) (2)
In lib/data/model/server/proc.dart around line 231, address this finding:
Snapshot correlation uses PID alone, so a restarted process that reuses a PID can inherit the previous process's I/O counters and report a false positive speed.
In lib/data/model/app/scripts/script_builders.dart around line 115, address this finding:
The Windows process command emits `Get-Process`'s `CPU` property, which is cumulative CPU time in seconds, but the process model exposes it as `cpu` and the page treats it as the CPU metric used for the default CPU sort and column. As a result, Windows processes are ranked by lifetime CPU seconds rather than current CPU usage, so the default view can surface old CPU-heavy processes instead of currently busy ones. The UI's `s` suffix makes the unit explicit but does not fix the sorting/metric contract.
## Previously reported and still present (1)
In lib/data/model/server/proc.dart around line 333, address this finding:
Windows JSON rows with malformed or missing process IDs are silently discarded, and malformed JSON falls through as Unix text instead of producing a displayable parse error; consequently invalid Windows input can appear as a successful empty/unsupported result with no error metadata.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- 3 of 3 areas reviewed
GT-610
dismissed stale reviews from winnowl[bot] and coderabbitai[bot]
via
August 9, 2026 03:59
c93271e
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/data/model/server/proc.dart (1)
355-405: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSeparate parser diagnostics from localized display text.
ProcessPagedisplaysPsResult.errordirectly. Keep row and exception details for diagnosis, but add a stable parse-failure code or type and map it tolibL10norl10nin the display layer. Update the parser tests and thePsResult.errorcontract.🤖 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 `@lib/data/model/server/proc.dart` around lines 355 - 405, Separate diagnostic details from user-facing text in the Windows process parsing flow around PsResult and ProcessPage: add a stable parse-failure code or typed error to PsResult while retaining row and exception details for diagnostics, then map that code to libL10n or l10n in ProcessPage instead of displaying PsResult.error directly. Update the PsResult contract and parser tests to verify both the stable failure classification and preserved diagnostic details.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.
Outside diff comments:
In `@lib/data/model/server/proc.dart`:
- Around line 355-405: Separate diagnostic details from user-facing text in the
Windows process parsing flow around PsResult and ProcessPage: add a stable
parse-failure code or typed error to PsResult while retaining row and exception
details for diagnostics, then map that code to libL10n or l10n in ProcessPage
instead of displaying PsResult.error directly. Update the PsResult contract and
parser tests to verify both the stable failure classification and preserved
diagnostic details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a9440f23-1673-4daa-b64c-3df4c155e5c4
📒 Files selected for processing (5)
lib/data/model/app/scripts/script_builders.dartlib/data/model/server/proc.dartlib/view/page/process.darttest/proc_test.darttest/script_builder_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/view/page/process.dart
There was a problem hiding this comment.
Actionable comments posted: 4
🛠️ To have the bot fix these findings, comment @winnowl fix.
🔎 Confirmed findings (4)
- 🟠 High The confirmed termination targets only the numeric PID captured at confirmation time and does not revalidate that the same process is still present or has the same start identity before issuing the kill command. (inline)
- 🟡 Medium The Windows process script emits I/O fields that standard
Get-Processdoes not provide, so the new Windows process metadata silently loses read/write counters.Get-Processexposes properties such asIOReadBytes/IOWriteBytesneither as standard process properties nor under these names; with$ErrorActionPreference = "SilentlyContinue", those expressions become null. The parser then receives null forIOReadBytesandIOWriteBytes, and the process page cannot calculate R/s or W/s even though the script claims to emit I/O counters. This is disproven if the target PowerShell environment is guaranteed to add those properties to everyGet-Processobject, or if a supported provider populates them before serialization. (inline) - 🟡 Medium When no start identity is available, a stable process whose command text changes between samples fails to inherit its prior counters, because
_matchingPreviousrequirescommand == previous.commandafter the start checks. SincepreviousByPidis already keyed by PID and absent start fields provide no stronger identity, a PID-stable process that updates argv (or has command formatting changed byps) gets null rates rather than deltas. This violates the obligation that stable processes match the intended previous sample when start fields are absent. The claim would be false if command text is guaranteed immutable for every supported process while start fields are absent. (inline) - 🟡 Medium The Windows producer emits
ProcessName,Id,CPUPercent,WorkingSet,IOReadBytes,IOWriteBytes, andStartId, but the parser falls back toCommandLine/Path/Nameand otherwise uses only the process name. This means the generated script does not provide a real command line on Windows, and rows for multiple processes with the same executable name are indistinguishable to the previous-snapshot matching logic (especially if StartId is absent/null), causing IO speeds to be missing or inherited incorrectly. The producer should emit a stable command identity/command line or the consumer should use a stronger identity contract. (inline)
📋 Additional findings from this change (not shown inline) (5)
- 🟠 High Custom script directories are interpolated directly into remote shell commands without quoting. For example, a Unix
customDirof/tmp/x; touch /tmp/pwned; #producesmkdir -p /tmp/x; touch /tmp/pwned; #in the install command, and the path is also interpolated into the redirection and chmod commands. A configured directory containing shell metacharacters can therefore execute unintended commands during script installation (and similarly affect the generated exec command). This is disproven only if customDir is guaranteed by validation upstream to be a safe path containing no shell metacharacters, but this code performs no such validation or escaping. (lib/data/model/app/scripts/script_builders.dart) — anchor-outside-diff - 🟠 High Default Windows script paths use
%USERPROFILE%and%TEMP%, but the generated PowerShell commands quote/use those strings literally rather than expanding them. For example, the default install path becomesNew-Item ... -Path '%TEMP%/server_box'andSet-Content -Path '%TEMP%/server_box/...ps1'; PowerShell does not perform cmd-style%VAR%expansion, and single quotes explicitly prevent PowerShell expansion. The script is therefore installed under a literal%TEMP%path (or fails) and subsequent-File "%TEMP%/..."execution does not target the installed script. This is disproven if the Windows SSH execution layer expands%TEMP%before PowerShell receives the command, or if those directories are normalized to$env:TEMP/expanded paths before builder invocation. (lib/data/model/app/scripts/script_consts.dart) — anchor-outside-diff - 🟡 Medium Unix rows with a missing/placeholder/malformed numeric metric are discarded instead of retaining the process with that metric nullable.
Proc._parseusesdouble.parsedirectly for%CPUand%MEM; therefore values such as-,N/A, or a malformed token throw, andPsResult.parsecatches the exception at the row boundary and omits the row. This violates the requirement to tolerate partial values without losing valid rows (and can make an otherwise valid process disappear). The claim would be false if the supported Unix producers were guaranteed never to emit any non-numeric CPU/MEM token. (lib/data/model/server/proc.dart) — anchor-outside-diff - 🟡 Medium The compact layout has a hard minimum width of 148 logical pixels, but no guard or horizontal scrolling for narrower constraints, so both the header and every process row overflow horizontally in a sufficiently narrow window. For example, with a 120 px
LayoutBuilderwidth, the compact row requires 12+68 (PID)+12 (gap)+44 (action)+12 = 148 px before the command'sExpandedchild; Flutter'sRowreports a 28 px overflow and clips the stop button/content. The same fixed children are used by_buildHeader, so its header overflows too. This is false only if the page is guaranteed never to receive a width below 148 logical pixels or an ancestor provides horizontal scrolling/clipping behavior intended for this page. (lib/view/page/process.dart) — anchor-unreliable - 🟡 Medium The Unix process script emits a BusyBox-specific
ps wresult without the stable START_ID/READ_BYTES/WRITE_BYTES schema, while the consumer assumes a header-driven process format and treats the presence of the resulting fields as capabilities. On BusyBox hosts this can make process rows fail to parse or silently lose the IO/start-identity data, so the process page cannot reliably display or calculate the metrics that the producer is intended to provide. The BusyBox branch needs either the same normalized schema as the main branch or an explicitly supported parser format. (lib/data/model/app/scripts/script_builders.dart) — anchor-unreliable
🤖 Prompt for AI agents — all findings (9)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Findings on this change (also posted as inline comments) (4)
In lib/view/page/process.dart around line 618, address this finding:
The confirmed termination targets only the numeric PID captured at confirmation time and does not revalidate that the same process is still present or has the same start identity before issuing the kill command.
In lib/data/model/app/scripts/script_builders.dart around line 130, address this finding:
The Windows process script emits I/O fields that standard `Get-Process` does not provide, so the new Windows process metadata silently loses read/write counters. `Get-Process` exposes properties such as `IOReadBytes`/`IOWriteBytes` neither as standard process properties nor under these names; with `$ErrorActionPreference = "SilentlyContinue"`, those expressions become null. The parser then receives null for `IOReadBytes` and `IOWriteBytes`, and the process page cannot calculate R/s or W/s even though the script claims to emit I/O counters. This is disproven if the target PowerShell environment is guaranteed to add those properties to every `Get-Process` object, or if a supported provider populates them before serialization.
In lib/data/model/server/proc.dart around line 554, address this finding:
When no start identity is available, a stable process whose command text changes between samples fails to inherit its prior counters, because `_matchingPrevious` requires `command == previous.command` after the start checks. Since `previousByPid` is already keyed by PID and absent start fields provide no stronger identity, a PID-stable process that updates argv (or has command formatting changed by `ps`) gets null rates rather than deltas. This violates the obligation that stable processes match the intended previous sample when start fields are absent. The claim would be false if command text is guaranteed immutable for every supported process while start fields are absent.
In lib/data/model/app/scripts/script_builders.dart around line 125, address this finding:
The Windows producer emits `ProcessName`, `Id`, `CPUPercent`, `WorkingSet`, `IOReadBytes`, `IOWriteBytes`, and `StartId`, but the parser falls back to `CommandLine`/`Path`/`Name` and otherwise uses only the process name. This means the generated script does not provide a real command line on Windows, and rows for multiple processes with the same executable name are indistinguishable to the previous-snapshot matching logic (especially if StartId is absent/null), causing IO speeds to be missing or inherited incorrectly. The producer should emit a stable command identity/command line or the consumer should use a stronger identity contract.
## Additional findings on this change (not posted inline) (5)
In lib/data/model/app/scripts/script_builders.dart around line 166, address this finding:
Custom script directories are interpolated directly into remote shell commands without quoting. For example, a Unix `customDir` of `/tmp/x; touch /tmp/pwned; #` produces `mkdir -p /tmp/x; touch /tmp/pwned; #` in the install command, and the path is also interpolated into the redirection and chmod commands. A configured directory containing shell metacharacters can therefore execute unintended commands during script installation (and similarly affect the generated exec command). This is disproven only if customDir is guaranteed by validation upstream to be a safe path containing no shell metacharacters, but this code performs no such validation or escaping.
In lib/data/model/app/scripts/script_consts.dart around line 15, address this finding:
Default Windows script paths use `%USERPROFILE%` and `%TEMP%`, but the generated PowerShell commands quote/use those strings literally rather than expanding them. For example, the default install path becomes `New-Item ... -Path '%TEMP%/server_box'` and `Set-Content -Path '%TEMP%/server_box/...ps1'`; PowerShell does not perform cmd-style `%VAR%` expansion, and single quotes explicitly prevent PowerShell expansion. The script is therefore installed under a literal `%TEMP%` path (or fails) and subsequent `-File "%TEMP%/..."` execution does not target the installed script. This is disproven if the Windows SSH execution layer expands `%TEMP%` before PowerShell receives the command, or if those directories are normalized to `$env:TEMP`/expanded paths before builder invocation.
In lib/data/model/server/proc.dart around line 158, address this finding:
Unix rows with a missing/placeholder/malformed numeric metric are discarded instead of retaining the process with that metric nullable. `Proc._parse` uses `double.parse` directly for `%CPU` and `%MEM`; therefore values such as `-`, `N/A`, or a malformed token throw, and `PsResult.parse` catches the exception at the row boundary and omits the row. This violates the requirement to tolerate partial values without losing valid rows (and can make an otherwise valid process disappear). The claim would be false if the supported Unix producers were guaranteed never to emit any non-numeric CPU/MEM token.
In lib/view/page/process.dart, address this finding:
The compact layout has a hard minimum width of 148 logical pixels, but no guard or horizontal scrolling for narrower constraints, so both the header and every process row overflow horizontally in a sufficiently narrow window. For example, with a 120 px `LayoutBuilder` width, the compact row requires 12+68 (PID)+12 (gap)+44 (action)+12 = 148 px before the command's `Expanded` child; Flutter's `Row` reports a 28 px overflow and clips the stop button/content. The same fixed children are used by `_buildHeader`, so its header overflows too. This is false only if the page is guaranteed never to receive a width below 148 logical pixels or an ancestor provides horizontal scrolling/clipping behavior intended for this page.
In lib/data/model/app/scripts/script_builders.dart, address this finding:
The Unix process script emits a BusyBox-specific `ps w` result without the stable START_ID/READ_BYTES/WRITE_BYTES schema, while the consumer assumes a header-driven process format and treats the presence of the resulting fields as capabilities. On BusyBox hosts this can make process rows fail to parse or silently lose the IO/start-identity data, so the process page cannot reliably display or calculate the metrics that the producer is intended to provide. The BusyBox branch needs either the same normalized schema as the main branch or an explicitly supported parser format.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- 4 of 4 areas reviewed
CI failure root-cause analysisJob Attribution Not determinable from the supplied diagnostics. The only supported observation is that the Verifiable fix Obtain the failed assertion and stack trace, then inspect the test fixture and the code that classifies and filters Windows status commands. Verify the configured disabled command types reach that code path and that the actual command list excludes the expected Windows status commands; add or adjust the implementation or test only after the assertion identifies the mismatch, and rerun the targeted test followed by the full suite. Incremental value: root cause, attributed to this change, verifiable fix; confidence 8%. Passing CI ≠ absence of defects (§29.4). |
There was a problem hiding this comment.
Actionable comments posted: 2
🛠️ To have the bot fix these findings, comment @winnowl fix.
🔎 Confirmed findings (2)
- 🟡 Medium Windows rows with non-positive PIDs are accepted as valid processes, despite the Windows producer explicitly filtering to IDs greater than zero and process identity requiring a real PID. (inline)
- 🟡 Medium Malformed VSZ/RSS values are rendered as raw strings instead of the required placeholder, producing incorrect units and exposing parser input in process details. (inline)
⛔ Unresolved from previous review (1) — not approved until fixed
- lib/view/page/process.dart: The confirmed termination targets only the numeric PID captured at confirmation time and does not revalidate that the same process is still present or has the same start identity before issuing the kill command. — The current flow now refreshes the process list immediately before killing and compares PID plus startId/start (when available), but
_isSameProcessfalls back to command equality when both identity fields are absent. On those platforms/output paths, a PID-reused process with the same command passes validation and is killed, so the reported wrong-process consequence remains possible.
📋 Additional findings from this change (not shown inline) (3)
- 🟡 Medium Script directory caching is keyed only by server id, so a server whose platform changes can reuse the previous platform's directory and then combine it with the new platform's separator and filename. For example, resolving id
xon Unix caches/tmp/server_box; resolvingxon Windows returns that same directory, andgetScriptPathproduces/tmp/server_box\srvboxm_v....ps1instead of a Windows%TEMP%/$env:TEMPpath. The reverse transition similarly yields a Unix.shpath under the Windows environment-variable directory. Installation and execution for the transitioned server therefore target a mixed, potentially invalid location. (lib/data/model/app/scripts/script_consts.dart) — anchor-outside-diff - 🟡 Medium The output protocol is not injective for custom command names or command output: any custom command whose output contains a line beginning
SrvBoxSep.orSrvBoxCusCmdSep.is interpreted as a new section marker, truncating/splitting that command's output. Likewise, a custom-command key containing a newline is inserted directly into the generated marker, creating extra marker lines and changing the parsed key. For example, custom commandecho SrvBoxSep.cpuproduces a parsedcpusection instead of retaining that ordinary output line. This violates compatibility for arbitrary custom-command output and names. (lib/data/model/app/scripts/script_consts.dart) — anchor-outside-diff - 🟡 Medium Windows separator parsing retains carriage returns in command names because
parseScriptOutputsplits only on\nand takes the marker suffix verbatim. PowerShellWrite-Hostemits CRLF, so a marker such asSrvBoxSep.echo\r\nis stored underecho\r, whileShellCmdType.findInMaplooks upecho; Windows status/custom-command results consequently appear missing. This is proven whenever the SSH/output transport preserves PowerShell's native CRLF line endings (and would be false only if a lower layer always normalizes\r\nbefore this parser). (lib/data/model/app/scripts/script_consts.dart) — anchor-outside-diff
♻️ Previously reported (still present) (2)
- 🟠 High The PID identity check is separated from the destructive command: after
_isSameProcesssucceeds, the remote process may exit and the PID may be reused beforekill $pid/taskkill /PID $pidruns, causing the command to terminate the replacement process. The implementation has no remote-side identity-qualified kill or post-check that can prevent this TOCTOU race. (lib/view/page/process.dart) — previously-reported - 🟡 Medium The kill path does not enforce the same server connection-state guard as refresh, so it can issue a process query and kill command while the provider reports the server is disconnected or still connecting. (lib/view/page/process.dart) — previously-reported
🤖 Prompt for AI agents — all findings (8)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Unresolved from the previous review — these block approval, fix them first (1)
In lib/view/page/process.dart, address this finding:
The confirmed termination targets only the numeric PID captured at confirmation time and does not revalidate that the same process is still present or has the same start identity before issuing the kill command.
## Findings on this change (also posted as inline comments) (2)
In lib/data/model/server/proc.dart around line 548, address this finding:
Windows rows with non-positive PIDs are accepted as valid processes, despite the Windows producer explicitly filtering to IDs greater than zero and process identity requiring a real PID.
In lib/view/page/process.dart around line 596, address this finding:
Malformed VSZ/RSS values are rendered as raw strings instead of the required placeholder, producing incorrect units and exposing parser input in process details.
## Additional findings on this change (not posted inline) (3)
In lib/data/model/app/scripts/script_consts.dart around line 121, address this finding:
Script directory caching is keyed only by server id, so a server whose platform changes can reuse the previous platform's directory and then combine it with the new platform's separator and filename. For example, resolving id `x` on Unix caches `/tmp/server_box`; resolving `x` on Windows returns that same directory, and `getScriptPath` produces `/tmp/server_box\srvboxm_v....ps1` instead of a Windows `%TEMP%`/`$env:TEMP` path. The reverse transition similarly yields a Unix `.sh` path under the Windows environment-variable directory. Installation and execution for the transitioned server therefore target a mixed, potentially invalid location.
In lib/data/model/app/scripts/script_consts.dart around line 51, address this finding:
The output protocol is not injective for custom command names or command output: any custom command whose output contains a line beginning `SrvBoxSep.` or `SrvBoxCusCmdSep.` is interpreted as a new section marker, truncating/splitting that command's output. Likewise, a custom-command key containing a newline is inserted directly into the generated marker, creating extra marker lines and changing the parsed key. For example, custom command `echo SrvBoxSep.cpu` produces a parsed `cpu` section instead of retaining that ordinary output line. This violates compatibility for arbitrary custom-command output and names.
In lib/data/model/app/scripts/script_consts.dart around line 46, address this finding:
Windows separator parsing retains carriage returns in command names because `parseScriptOutput` splits only on `\n` and takes the marker suffix verbatim. PowerShell `Write-Host` emits CRLF, so a marker such as `SrvBoxSep.echo\r\n` is stored under `echo\r`, while `ShellCmdType.findInMap` looks up `echo`; Windows status/custom-command results consequently appear missing. This is proven whenever the SSH/output transport preserves PowerShell's native CRLF line endings (and would be false only if a lower layer always normalizes `\r\n` before this parser).
## Previously reported and still present (2)
In lib/view/page/process.dart around line 704, address this finding:
The PID identity check is separated from the destructive command: after `_isSameProcess` succeeds, the remote process may exit and the PID may be reused before `kill $pid`/`taskkill /PID $pid` runs, causing the command to terminate the replacement process. The implementation has no remote-side identity-qualified kill or post-check that can prevent this TOCTOU race.
In lib/view/page/process.dart around line 665, address this finding:
The kill path does not enforce the same server connection-state guard as refresh, so it can issue a process query and kill command while the provider reports the server is disconnected or still connecting.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- 4 of 4 areas reviewed
| return value.toInt(); | ||
| } | ||
| return int.tryParse(value?.toString() ?? ''); | ||
| } |
There was a problem hiding this comment.
🔍 Error Handling | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/data/model/server/proc.dart, address this finding:
Windows rows with non-positive PIDs are accepted as valid processes, despite the Windows producer explicitly filtering to IDs greater than zero and process identity requiring a real PID.
📝 Committable suggestion — review it before committing; it is generated, not proven.
Suggested change
| } | |
| int? _parseProcessId(Object? value) { | |
| if (value is int) return value > 0 ? value : null; | |
| if (value is num) { | |
| if (!value.isFinite || value != value.truncateToDouble()) return null; | |
| final pid = value.toInt(); | |
| return pid > 0 ? pid : null; | |
| } | |
| final pid = int.tryParse(value?.toString() ?? ''); | |
| return pid != null && pid > 0 ? pid : null; | |
| } |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/data/model/app/scripts/script_builders.dart (1)
366-370: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep I/O placeholders when
/proc/<pid>/iocannot be read.Lines 367-368 can overwrite
'-'with an empty value when a process exits after the readability check. Line 370 then emits missing positional fields. This can shiftWRITE_BYTESandCOMMANDduring process parsing.Proposed fix
read_bytes=$(awk '/^read_bytes:/ {print $2}' "/proc/$pid/io") write_bytes=$(awk '/^write_bytes:/ {print $2}' "/proc/$pid/io") + [ -n "$read_bytes" ] || read_bytes='-' + [ -n "$write_bytes" ] || write_bytes='-' fi🤖 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 `@lib/data/model/app/scripts/script_builders.dart` around lines 366 - 370, Update the I/O collection block in the generated script to preserve the `read_bytes` and `write_bytes` placeholder values when `/proc/<pid>/io` becomes unavailable or the awk lookup returns empty. Only replace each placeholder after confirming a non-empty value, so the printf fields emitted by the surrounding process-reporting logic remain positionally stable.
🤖 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.
Outside diff comments:
In `@lib/data/model/app/scripts/script_builders.dart`:
- Around line 366-370: Update the I/O collection block in the generated script
to preserve the `read_bytes` and `write_bytes` placeholder values when
`/proc/<pid>/io` becomes unavailable or the awk lookup returns empty. Only
replace each placeholder after confirming a non-empty value, so the printf
fields emitted by the surrounding process-reporting logic remain positionally
stable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 49738ad8-c42c-445e-8e58-2d88b04576e3
📒 Files selected for processing (7)
lib/data/model/app/scripts/script_builders.dartlib/data/model/app/scripts/script_consts.dartlib/data/model/server/proc.dartlib/view/page/process.darttest/disabled_cmd_types_test.darttest/proc_test.darttest/script_builder_test.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- test/proc_test.dart
- test/script_builder_test.dart
- lib/view/page/process.dart
- lib/data/model/server/proc.dart
There was a problem hiding this comment.
Actionable comments posted: 8
🛠️ To have the bot fix these findings, comment @winnowl fix.
🔎 Confirmed findings (8)
- 🟡 Medium An empty custom command produces an invalid Unix script:
_unixFramedCommandemits a brace group with no command between{and}, so adding a custom command whose value is empty makesstatus()fail shell parsing. This violates the requirement that generated scripts remain executable for arbitrary custom-command values. (inline) - 🟡 Medium The Windows process command only catches failures while collecting CPU percentages; the subsequent
Get-CimInstance Win32_Process | ...is unguarded. If CIM/WMI is unavailable or access is denied, the command emits no JSON (and can emit an error before the global preference suppresses it), so the process consumer receives an empty/non-contract result instead of a parseable process header or fallback output. (inline) - 🟡 Medium Custom Unix directories containing an environment-variable reference are treated as literal directory names rather than expanded paths. For example, configuring
customDiras$HOME/.config/server_boxgeneratesmkdir -p '$HOME/.config/server_box'and redirects to'$HOME/...sh', so installation writes under a directory literally named$HOME(or fails) and the subsequent execution command targets that same literal path, not the user's home directory. (inline) - 🟡 Medium When both samples omit START/START_ID,
_matchingPreviousfalls through to PID-only matching. A PID reused between refreshes with no available identity therefore inherits the old process's IO counters and produces a plausible but false speed instead of null. The same fallback is used for Unix rows without identity and Windows JSON without StartId. (inline) - 🟡 Medium Negative RSS values are treated as valid measurements. Unix
rss: '-1'makesrssKbequal -1, and a negative Windows WorkingSet can be rounded by((workingSetBytes + 1023) ~/ 1024)into a positive RSS value (for example -1 bytes becomes 0 KiB). These rows advertise RSS capability, sort ahead of valid values under descending order, and can display misleading memory sizes rather than being absent/invalid. (inline) - 🟡 Medium BSD process rows can be displayed but can never be terminated:
_killProcessCmddispatchesSystemType.bsdto_bsdKillProcessCmd, which is hard-coded to return null, causing every confirmed BSD kill to shownotAvailablewithout attempting the guarded platform-specific signal. (inline) - 🟡 Medium The new process count and parse-error UI is untranslated for most supported locales. For example, in the German generated localization,
processCountreturns'1 process'/'$count processes'andprocessParseInvalidRowsreturns'Some process entries could not be read.'; the same English implementations are present in other non-Chinese locale files. A German user viewing the process count or a parse-error dialog will therefore see English instead of German. (inline) - 🟡 Medium A refresh that returns a parse issue is still installed as
_resultand then used as theprevioussnapshot for the next refresh, so I/O rates can be computed against a partial or invalid sample rather than the last valid sample. (inline)
⛔ Unresolved from previous review (1) — not approved until fixed
- lib/view/page/process.dart: The speed tests do not establish that the denominator is the current-minus-previous sample time, or that non-positive elapsed intervals yield null speeds. They also do not distinguish per-PID predecessor matching from accidental global/row-position matching (the fixture keeps the same PID order). A regression using a fixed interval, accepting zero/negative elapsed time, or pairing counters by position would therefore pass. This is false if other tests exercise reordered snapshots and zero/negative timestamps; the provided suite has no such cases. — Still-present: the current implementation in lib/data/model/server/proc.dart does calculate elapsedSeconds from currentSampledAtMillis minus previous.sampledAtMillis, rejects elapsedSeconds <= 0, and looks up predecessors by PID; however, test/proc_test.dart only exercises positive intervals with the snapshots in the same PID order and has no zero/negative-interval or reordered-snapshot assertions. Thus the reported test-suite gap remains, even though the current production implementation itself follows the intended behavior.
📋 Additional findings from this change (not shown inline) (17)
- 🟡 Medium When every command in a Unix status set is disabled, the generated
status()function contains an empty branch body (thenfollowed directly byelse, and likewise for the BSD branch), which is a shell syntax error rather than a valid no-op. Executing the generated script with all Unix status command types disabled therefore fails to parse instead of returning an empty status result. (lib/data/model/app/scripts/script_builders.dart) — anchor-unreliable - 🟡 Medium
parseScriptOutputtrims every section withbuffer.toString().trim(), so framed command output is not lossless: a custom command that intentionally prints leading/trailing spaces (or blank leading/trailing lines) is changed before consumers receive it. This violates the arbitrary command-output/line-ending integrity requirement even though marker-like lines are protected by the data prefix. (lib/data/model/app/scripts/script_consts.dart) — anchor-unreliable - 🟡 Medium A Windows script-install failure never switches the cached script directory or retries installation in the home directory, even though
ScriptPathsdefines Windows temp/home alternatives and documents fallback behavior. On a Windows host where$env:TEMP\server_boxcannot be created or written (redirected/locked temp, restrictive ACL, or an invalid TEMP value), the provider enters the failure branch, skipsswitchScriptDirbecause ofdetectedSystemType != SystemType.windows, and subsequent reconnects keep retrying the same inaccessible temp path. (lib/data/provider/server/single.dart) — anchor-outside-diff - 🟡 Medium An empty Unix custom command makes the generated script syntactically invalid. For an entry such as
{'label': ''},_unixFramedCommandproduces{ } | sed ...; POSIXshdoes not allow an empty brace group (it needs a command such as:), so the entire script fails to parse before any status command runs. The persisted custom-command editor permits string values and there is no filtering/validation before this builder. (lib/data/model/app/scripts/script_builders.dart) — per-file-budget - 🟡 Medium A custom command whose key matches a built-in command name overwrites that built-in's parsed output. Both built-ins and custom commands are decoded into the same
Map<String,String>keyed only bymarker.name;parseScriptOutputassignsresult[currentCmd] = ...each time. WithcustomCmds: {'cpu': 'echo custom'}, the generated status script emits the built-inSrvBoxSep...cpuand then the custom marker, so the final map containscpu: 'custom'; the status parser then receives custom text where it expects/proc/statand CPU parsing fails or reports missing CPU data. This is false only if custom-command keys are guaranteed never to equal any built-in command name, but the editor/model currently accepts arbitrary map keys. (lib/data/model/app/scripts/script_consts.dart) — anchor-unreliable - 🟡 Medium Duplicate PIDs in one sample are accepted without a diagnostic, while previous samples are indexed by PID in a map. If a malformed/duplicated Unix or Windows listing contains two rows for PID 42, both are emitted and a later refresh's counter baseline is ambiguous (the map comprehension silently keeps only the last prior row), so IO rates and row identity are not deterministic or unambiguous. (lib/data/model/server/proc.dart) — anchor-outside-diff
- 🟡 Medium Windows PID parsing accepts fractional numeric IDs by truncating them:
_parseProcessIdusesvalue == value.truncateToDouble()only for thenumpattern, but JSON decoding a value such as 1.5 produces a double and correctly rejects; however integer-like floating values such as 1.0 are accepted, which is fine. The concrete failure is the separate_parseDynamicIntconversion: fractional metric counters such as 1.5 are silently truncated to 1, creating fabricated IO deltas rather than a diagnostic/absent value. (lib/data/model/server/proc.dart) — per-file-budget - 🟡 Medium Numeric parsing accepts non-finite floating-point values (for example Unix
%CPU/%MEMor WindowsCPUPercentequal toNaN/Infinity) becausedouble.tryParseis returned without anisFinitecheck. Such a row is treated as having a metric, enables the corresponding process-page sort/capability, and displaysNaN%/Infinity%instead of being null or diagnosed; comparisons against these values are not a meaningful numeric ordering. (lib/data/model/server/proc.dart) — per-file-budget - 🟡 Medium Windows field fallbacks only apply when the preferred property is null, not when it is present but empty or unparsable. For example,
{"Id":7,"CommandLine":"","Path":"C:\\app.exe","Name":"app"}yields an empty command, and{"Id":7,"CPUPercent":"","PercentProcessorTime":12.5}yields null CPU; similarly an emptyIOReadBytessuppresses a validReadTransferCount. This makes valid fallback data disappear and can incorrectly disable columns/sorting. (lib/data/model/server/proc.dart) — per-file-budget - 🟡 Medium When both snapshots expose an identity field but its value is missing (
START_IDis-),_matchingPreviousfalls through and matches by PID. Thus a PID-reused process whose old and new rows both have unavailable start IDs inherits the old IO counters and can report a false speed. The same happens for Windows rows with noStartId: two unrelated lifetimes with the same PID are accepted when both identities are null. This would be false only if a missing identity is guaranteed to mean the PID is stable for the lifetime represented by both samples; the parser explicitly normalizes-/empty identities to null, so that guarantee is not established. (lib/data/model/server/proc.dart) — per-file-budget - 🟡 Medium Unix parsing accepts PID 0 and negative PIDs as valid processes. For example, a
PID USER COMMANDrow-1 root /badreachesProc._parse,int.parsesucceeds, and the result contains pid -1 instead of reporting an invalid row. This violates the positive-process-ID invariant already enforced by_parseProcessIdfor Windows and can expose a non-process row to the process UI/actions. This would be disproven if Unix process output is guaranteed upstream to contain only positive PIDs and invalid rows are intentionally allowed, but the parser's typed invalid-row handling and Windows validation indicate the opposite. (lib/data/model/server/proc.dart) — per-file-budget - 🟡 Medium A malformed/partially parseable refresh replaces the last good snapshot with only the rows that happened to parse. On the following refresh,
previous: _resulttherefore lacks the dropped processes and uses the partial sample's timestamp, causing surviving rows' I/O rates to be calculated across a parse-failure interval (and potentially from the wrong baseline) instead of from the last trustworthy snapshot; the same partial result is also rendered as the complete process population. (lib/data/model/server/proc.dart) — anchor-unreliable - 🟡 Medium The BSD/macOS process branch still emits the old nine-field contract and omits
START_ID,READ_BYTES, andWRITE_BYTES, while the Linux and BusyBox branches now advertise and emit those columns. This means supported BSD/macOS hosts cannot provide the newly required start identity and I/O counters, and the process parser necessarily leaves those fields null on those platforms. (lib/data/model/app/scripts/script_builders.dart) — per-file-budget - 🟡 Medium A custom command name can collide with a built-in status command name, and
parseScriptOutputstores both sections under the same string key. When the later marker is processed it overwrites the earlier result, so the status consumer can parse custom output as the built-in command (or vice versa), losing one section. For example, custom keynetcollides with the built-innetmarker. (lib/data/model/app/scripts/script_consts.dart) — anchor-unreliable - 🟡 Medium Unix rows with PID 0 or a negative PID are accepted as valid processes, even though process identity and the Windows branch require a strictly positive PID. For example, a header
PID COMMANDfollowed by0 idleyields a Proc with pid 0 and can enter the process page, be used as a previous-sample key, and participate in kill/sort logic instead of being reported as an invalid row. (lib/data/model/server/proc.dart) — per-file-budget - 🟡 Medium Disabling every Unix/BSD status command generates a syntactically invalid shell script.
_getUnixStatusCommandstill emitsif [ "$macSign" = "" ] && [ "$bsdSign" = "" ]; thenfollowed immediately byelsewhen both filtered command lists are empty, producingthen elsewith no command (POSIX shells require a command or:). The script installation can succeed, but every subsequentsh ... -sinvocation fails with a syntax error and returns no status. (lib/data/model/app/scripts/script_builders.dart) — per-file-budget - 🔵 Low Unix command text is not preserved verbatim: every row is split on one-or-more whitespace and reconstructed with single spaces (
parts.sublist(map.command).join(' ')). A process command such as/bin/tool --name 'a b'is exposed as/bin/tool --name 'a b', changing argument text and the command used for display/identity fallback. Header-driven parsing can preserve the command tail directly instead of normalizing it. (lib/data/model/server/proc.dart) — per-file-budget
♻️ Previously reported (still present) (1)
- 🟡 Medium An empty command response is treated as a successful empty process snapshot:
_resultis replaced with an empty result and the prior sample/timestamp is discarded. If the SSH command returns empty because the script is unavailable, output is truncated, or the connection is tearing down, the page loses valid rows and the next non-empty refresh cannot derive I/O deltas from the last valid sample. (lib/view/page/process.dart) — previously-reported
🤖 Prompt for AI agents — all findings (27)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Unresolved from the previous review — these block approval, fix them first (1)
In lib/view/page/process.dart, address this finding:
The speed tests do not establish that the denominator is the current-minus-previous sample time, or that non-positive elapsed intervals yield null speeds. They also do not distinguish per-PID predecessor matching from accidental global/row-position matching (the fixture keeps the same PID order). A regression using a fixed interval, accepting zero/negative elapsed time, or pairing counters by position would therefore pass. This is false if other tests exercise reordered snapshots and zero/negative timestamps; the provided suite has no such cases.
## Findings on this change (also posted as inline comments) (8)
In lib/data/model/app/scripts/script_builders.dart around line 34, address this finding:
An empty custom command produces an invalid Unix script: `_unixFramedCommand` emits a brace group with no command between `{` and `}`, so adding a custom command whose value is empty makes `status()` fail shell parsing. This violates the requirement that generated scripts remain executable for arbitrary custom-command values.
In lib/data/model/app/scripts/script_builders.dart around line 170, address this finding:
The Windows process command only catches failures while collecting CPU percentages; the subsequent `Get-CimInstance Win32_Process | ...` is unguarded. If CIM/WMI is unavailable or access is denied, the command emits no JSON (and can emit an error before the global preference suppresses it), so the process consumer receives an empty/non-contract result instead of a parseable process header or fallback output.
In lib/data/model/app/scripts/script_builders.dart around line 26, address this finding:
Custom Unix directories containing an environment-variable reference are treated as literal directory names rather than expanded paths. For example, configuring `customDir` as `$HOME/.config/server_box` generates `mkdir -p '$HOME/.config/server_box'` and redirects to `'$HOME/...sh'`, so installation writes under a directory literally named `$HOME` (or fails) and the subsequent execution command targets that same literal path, not the user's home directory.
In lib/data/model/server/proc.dart around line 584, address this finding:
When both samples omit START/START_ID, `_matchingPrevious` falls through to PID-only matching. A PID reused between refreshes with no available identity therefore inherits the old process's IO counters and produces a plausible but false speed instead of null. The same fallback is used for Unix rows without identity and Windows JSON without StartId.
In lib/data/model/server/proc.dart around line 234, address this finding:
Negative RSS values are treated as valid measurements. Unix `rss: '-1'` makes `rssKb` equal -1, and a negative Windows WorkingSet can be rounded by `((workingSetBytes + 1023) ~/ 1024)` into a positive RSS value (for example -1 bytes becomes 0 KiB). These rows advertise RSS capability, sort ahead of valid values under descending order, and can display misleading memory sizes rather than being absent/invalid.
In lib/view/page/process.dart around line 671, address this finding:
BSD process rows can be displayed but can never be terminated: `_killProcessCmd` dispatches `SystemType.bsd` to `_bsdKillProcessCmd`, which is hard-coded to return null, causing every confirmed BSD kill to show `notAvailable` without attempting the guarded platform-specific signal.
In lib/generated/l10n/l10n_de.dart around line 1127, address this finding:
The new process count and parse-error UI is untranslated for most supported locales. For example, in the German generated localization, `processCount` returns `'1 process'`/`'$count processes'` and `processParseInvalidRows` returns `'Some process entries could not be read.'`; the same English implementations are present in other non-Chinese locale files. A German user viewing the process count or a parse-error dialog will therefore see English instead of German.
In lib/view/page/process.dart around line 740, address this finding:
A refresh that returns a parse issue is still installed as `_result` and then used as the `previous` snapshot for the next refresh, so I/O rates can be computed against a partial or invalid sample rather than the last valid sample.
## Additional findings on this change (not posted inline) (17)
In lib/data/model/app/scripts/script_builders.dart, address this finding:
When every command in a Unix status set is disabled, the generated `status()` function contains an empty branch body (`then` followed directly by `else`, and likewise for the BSD branch), which is a shell syntax error rather than a valid no-op. Executing the generated script with all Unix status command types disabled therefore fails to parse instead of returning an empty status result.
In lib/data/model/app/scripts/script_consts.dart, address this finding:
`parseScriptOutput` trims every section with `buffer.toString().trim()`, so framed command output is not lossless: a custom command that intentionally prints leading/trailing spaces (or blank leading/trailing lines) is changed before consumers receive it. This violates the arbitrary command-output/line-ending integrity requirement even though marker-like lines are protected by the data prefix.
In lib/data/provider/server/single.dart around line 344, address this finding:
A Windows script-install failure never switches the cached script directory or retries installation in the home directory, even though `ScriptPaths` defines Windows temp/home alternatives and documents fallback behavior. On a Windows host where `$env:TEMP\server_box` cannot be created or written (redirected/locked temp, restrictive ACL, or an invalid TEMP value), the provider enters the failure branch, skips `switchScriptDir` because of `detectedSystemType != SystemType.windows`, and subsequent reconnects keep retrying the same inaccessible temp path.
In lib/data/model/app/scripts/script_builders.dart around line 29, address this finding:
An empty Unix custom command makes the generated script syntactically invalid. For an entry such as `{'label': ''}`, `_unixFramedCommand` produces `{
} | sed ...`; POSIX `sh` does not allow an empty brace group (it needs a command such as `:`), so the entire script fails to parse before any status command runs. The persisted custom-command editor permits string values and there is no filtering/validation before this builder.
In lib/data/model/app/scripts/script_consts.dart, address this finding:
A custom command whose key matches a built-in command name overwrites that built-in's parsed output. Both built-ins and custom commands are decoded into the same `Map<String,String>` keyed only by `marker.name`; `parseScriptOutput` assigns `result[currentCmd] = ...` each time. With `customCmds: {'cpu': 'echo custom'}`, the generated status script emits the built-in `SrvBoxSep...cpu` and then the custom marker, so the final map contains `cpu: 'custom'`; the status parser then receives custom text where it expects `/proc/stat` and CPU parsing fails or reports missing CPU data. This is false only if custom-command keys are guaranteed never to equal any built-in command name, but the editor/model currently accepts arbitrary map keys.
In lib/data/model/server/proc.dart around line 269, address this finding:
Duplicate PIDs in one sample are accepted without a diagnostic, while previous samples are indexed by PID in a map. If a malformed/duplicated Unix or Windows listing contains two rows for PID 42, both are emitted and a later refresh's counter baseline is ambiguous (the map comprehension silently keeps only the last prior row), so IO rates and row identity are not deterministic or unambiguous.
In lib/data/model/server/proc.dart around line 542, address this finding:
Windows PID parsing accepts fractional numeric IDs by truncating them: `_parseProcessId` uses `value == value.truncateToDouble()` only for the `num` pattern, but JSON decoding a value such as 1.5 produces a double and correctly rejects; however integer-like floating values such as 1.0 are accepted, which is fine. The concrete failure is the separate `_parseDynamicInt` conversion: fractional metric counters such as 1.5 are silently truncated to 1, creating fabricated IO deltas rather than a diagnostic/absent value.
In lib/data/model/server/proc.dart around line 564, address this finding:
Numeric parsing accepts non-finite floating-point values (for example Unix `%CPU`/`%MEM` or Windows `CPUPercent` equal to `NaN`/`Infinity`) because `double.tryParse` is returned without an `isFinite` check. Such a row is treated as having a metric, enables the corresponding process-page sort/capability, and displays `NaN%`/`Infinity%` instead of being null or diagnosed; comparisons against these values are not a meaningful numeric ordering.
In lib/data/model/server/proc.dart around line 181, address this finding:
Windows field fallbacks only apply when the preferred property is null, not when it is present but empty or unparsable. For example, `{"Id":7,"CommandLine":"","Path":"C:\\app.exe","Name":"app"}` yields an empty command, and `{"Id":7,"CPUPercent":"","PercentProcessorTime":12.5}` yields null CPU; similarly an empty `IOReadBytes` suppresses a valid `ReadTransferCount`. This makes valid fallback data disappear and can incorrectly disable columns/sorting.
In lib/data/model/server/proc.dart around line 575, address this finding:
When both snapshots expose an identity field but its value is missing (`START_ID` is `-`), `_matchingPrevious` falls through and matches by PID. Thus a PID-reused process whose old and new rows both have unavailable start IDs inherits the old IO counters and can report a false speed. The same happens for Windows rows with no `StartId`: two unrelated lifetimes with the same PID are accepted when both identities are null. This would be false only if a missing identity is guaranteed to mean the PID is stable for the lifetime represented by both samples; the parser explicitly normalizes `-`/empty identities to null, so that guarantee is not established.
In lib/data/model/server/proc.dart around line 135, address this finding:
Unix parsing accepts PID 0 and negative PIDs as valid processes. For example, a `PID USER COMMAND` row `-1 root /bad` reaches `Proc._parse`, `int.parse` succeeds, and the result contains pid -1 instead of reporting an invalid row. This violates the positive-process-ID invariant already enforced by `_parseProcessId` for Windows and can expose a non-process row to the process UI/actions. This would be disproven if Unix process output is guaranteed upstream to contain only positive PIDs and invalid rows are intentionally allowed, but the parser's typed invalid-row handling and Windows validation indicate the opposite.
In lib/data/model/server/proc.dart, address this finding:
A malformed/partially parseable refresh replaces the last good snapshot with only the rows that happened to parse. On the following refresh, `previous: _result` therefore lacks the dropped processes and uses the partial sample's timestamp, causing surviving rows' I/O rates to be calculated across a parse-failure interval (and potentially from the wrong baseline) instead of from the last trustworthy snapshot; the same partial result is also rendered as the complete process population.
In lib/data/model/app/scripts/script_builders.dart around line 374, address this finding:
The BSD/macOS process branch still emits the old nine-field contract and omits `START_ID`, `READ_BYTES`, and `WRITE_BYTES`, while the Linux and BusyBox branches now advertise and emit those columns. This means supported BSD/macOS hosts cannot provide the newly required start identity and I/O counters, and the process parser necessarily leaves those fields null on those platforms.
In lib/data/model/app/scripts/script_consts.dart, address this finding:
A custom command name can collide with a built-in status command name, and `parseScriptOutput` stores both sections under the same string key. When the later marker is processed it overwrites the earlier result, so the status consumer can parse custom output as the built-in command (or vice versa), losing one section. For example, custom key `net` collides with the built-in `net` marker.
In lib/data/model/server/proc.dart around line 333, address this finding:
Unix rows with PID 0 or a negative PID are accepted as valid processes, even though process identity and the Windows branch require a strictly positive PID. For example, a header `PID COMMAND` followed by `0 idle` yields a Proc with pid 0 and can enter the process page, be used as a previous-sample key, and participate in kill/sort logic instead of being reported as an invalid row.
In lib/data/model/app/scripts/script_builders.dart around line 312, address this finding:
Disabling every Unix/BSD status command generates a syntactically invalid shell script. `_getUnixStatusCommand` still emits `if [ "$macSign" = "" ] && [ "$bsdSign" = "" ]; then` followed immediately by `else` when both filtered command lists are empty, producing `then
else` with no command (POSIX shells require a command or `:`). The script installation can succeed, but every subsequent `sh ... -s` invocation fails with a syntax error and returns no status.
In lib/data/model/server/proc.dart around line 140, address this finding:
Unix command text is not preserved verbatim: every row is split on one-or-more whitespace and reconstructed with single spaces (`parts.sublist(map.command).join(' ')`). A process command such as `/bin/tool --name 'a b'` is exposed as `/bin/tool --name 'a b'`, changing argument text and the command used for display/identity fallback. Header-driven parsing can preserve the command tail directly instead of normalizing it.
## Previously reported and still present (1)
In lib/view/page/process.dart around line 129, address this finding:
An empty command response is treated as a successful empty process snapshot: `_result` is replaced with an empty result and the prior sample/timestamp is discarded. If the SSH command returns empty because the script is unavailable, output is truncated, or the connection is tearing down, the page loses valid rows and the next non-empty refresh cannot derive I/O deltas from the last valid sample.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- 4 of 4 areas reviewed
| printf '%s\\n' ${_quoteUnixLiteral(marker)} | ||
| { | ||
| $command | ||
| } | sed 's/^/${ScriptConstants.dataPrefix}/' |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/data/model/app/scripts/script_builders.dart, address this finding:
An empty custom command produces an invalid Unix script: `_unixFramedCommand` emits a brace group with no command between `{` and `}`, so adding a custom command whose value is empty makes `status()` fail shell parsing. This violates the requirement that generated scripts remain executable for arbitrary custom-command values.
📝 Committable suggestion — review it before committing; it is generated, not proven.
Suggested change
| } | sed 's/^/${ScriptConstants.dataPrefix}/' | |
| ${command.trim().isEmpty ? ':' : command} | |
| } | sed 's/^/${ScriptConstants.dataPrefix}/' |
| if (path.startsWith('~/')) { | ||
| return r'"$HOME"/' + _quoteUnixLiteral(path.substring(2)); | ||
| } | ||
| return _quoteUnixLiteral(path); |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/data/model/app/scripts/script_builders.dart, address this finding:
Custom Unix directories containing an environment-variable reference are treated as literal directory names rather than expanded paths. For example, configuring `customDir` as `$HOME/.config/server_box` generates `mkdir -p '$HOME/.config/server_box'` and redirects to `'$HOME/...sh'`, so installation writes under a directory literally named `$HOME` (or fails) and the subsequent execution command targets that same literal path, not the user's home directory.
| String get systemd => 'Systemd'; | ||
|
|
||
| @override | ||
| String processCount(int count) { |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/generated/l10n/l10n_de.dart, address this finding:
The new process count and parse-error UI is untranslated for most supported locales. For example, in the German generated localization, `processCount` returns `'1 process'`/`'$count processes'` and `processParseInvalidRows` returns `'Some process entries could not be read.'`; the same English implementations are present in other non-Chinese locale files. A German user viewing the process count or a parse-error dialog will therefore see English instead of German.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@lib/l10n/app_fr.arb`:
- Line 240: Update the processCount ICU plural message so only an exact count of
1 renders “1 processus,” while zero and other counts preserve the actual count;
then regenerate localization output with flutter gen-l10n.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 00bac05c-06a1-40bf-aebb-3ad9317cee5f
⛔ Files ignored due to path filters (12)
lib/generated/l10n/l10n_de.dartis excluded by!**/generated/**lib/generated/l10n/l10n_es.dartis excluded by!**/generated/**lib/generated/l10n/l10n_fr.dartis excluded by!**/generated/**lib/generated/l10n/l10n_id.dartis excluded by!**/generated/**lib/generated/l10n/l10n_it.dartis excluded by!**/generated/**lib/generated/l10n/l10n_ja.dartis excluded by!**/generated/**lib/generated/l10n/l10n_ko.dartis excluded by!**/generated/**lib/generated/l10n/l10n_nl.dartis excluded by!**/generated/**lib/generated/l10n/l10n_pt.dartis excluded by!**/generated/**lib/generated/l10n/l10n_ru.dartis excluded by!**/generated/**lib/generated/l10n/l10n_tr.dartis excluded by!**/generated/**lib/generated/l10n/l10n_uk.dartis excluded by!**/generated/**
📒 Files selected for processing (22)
lib/data/model/app/scripts/script_builders.dartlib/data/model/app/scripts/script_consts.dartlib/data/model/server/proc.dartlib/data/model/server/server_status_update_req.dartlib/data/model/server/windows_parser.dartlib/data/provider/server/single.dartlib/l10n/app_de.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_id.arblib/l10n/app_it.arblib/l10n/app_ja.arblib/l10n/app_ko.arblib/l10n/app_nl.arblib/l10n/app_pt.arblib/l10n/app_ru.arblib/l10n/app_tr.arblib/l10n/app_uk.arblib/view/page/process.darttest/disabled_cmd_types_test.darttest/proc_test.darttest/script_builder_test.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/data/model/app/scripts/script_consts.dart
- test/proc_test.dart
- lib/data/model/app/scripts/script_builders.dart
- lib/view/page/process.dart
- lib/data/model/server/proc.dart
There was a problem hiding this comment.
Actionable comments posted: 5
🛠️ To have the bot fix these findings, comment @winnowl fix.
🔎 Confirmed findings (5)
- 🟠 High The Windows termination path sends a PowerShell program directly to the SSH client's generic
runmethod instead of invokingpowershellexplicitly, so on Windows SSH servers whose default shell iscmd.exethe command is not interpreted as PowerShell and no process is killed. (inline) - 🟠 High Process refresh and kill re-fetches ignore the server's configured custom script directory, so custom-script servers can return non-process output and the page may classify it as a parse diagnostic or fail to validate/kill the selected target. (inline)
- 🟠 High The BSD kill path can kill an unrelated process after PID reuse: it runs
ps -p <pid>to validate the saved start value, then executes a separatekill <pid>. If the original process exits and the same PID is assigned beforekill, the validation has already succeeded but the signal is delivered to the replacement process. This violates the changed/exited target invariant that the process selected in the UI must be the one terminated. The current tests cover parser validation and script generation, but have no kill-path test or implementation that closes this race. (inline) - 🟡 Medium Unix path quoting expands every syntactically valid
$NAME/${NAME}prefix, not just the intended HOME/TEMP paths. A custom script directory such as$PATH/server_boxis emitted as"${PATH}"/'server_box', causing the remote shell to resolve the path from attacker- or user-controlled PATH rather than treating the supplied path literally. This violates the path contract and can redirect installation/execution; only the supported HOME/TEMP environment forms should be expanded. (inline) - 🟡 Medium The legacy-marker fallback accepts any line beginning with
SrvBoxSep.orSrvBoxCusCmdSep.whose suffix is notb64., including marker-like payload lines from legacy command output. For example, legacy outputSrvBoxSep.echo\nSrvBoxSep.cpu\nvalueis parsed as two sections and overwrites/createscpu, rather than preservingSrvBoxSep.cpuas payload. Because the parser has no framing/version distinction, malformed or marker-like legacy data is not handled safely. (inline)
📋 Additional findings from this change (not shown inline) (33)
- 🟠 High A manual disconnect (or client replacement) can be undone by an in-flight refresh because the guard only prevents a second refresh; it does not invalidate the current operation. For example, while
genClientor_runStatusCommandis awaiting,closeConnection()clears the client and setsdisconnected, but the old refresh can later callupdateClient(client)and then publishconnected/finishedand status. This resurrects a closed SSH client and leaves the UI showing a server that the user explicitly disconnected. The claim is false only if all callers guarantee that close/update operations cannot occur while refresh awaits, or if the underlying notifier drops all state mutations after close (neither is present here;ServersNotifier.closeOneServerdirectly callscloseConnection). (lib/data/provider/server/single.dart) — anchor-outside-diff - 🟠 High The BSD listing uses
ps ... start=asSTART_ID, and the kill command rechecks only that human-formatted start value before issuing an independentkill PID. This value is coarse/display-oriented rather than a unique birth identity, and the check-then-kill gap permits a recycled PID (or a process replacement between check and kill) to be terminated while still passing the guard. (lib/view/page/process.dart) — anchor-unreliable - 🟠 High The BSD kill command's identity check is neither stable nor atomic: it compares the selected row's
START_ID(populated fromps ... start=) with a freshps -p ... -o start=value and then performs a separatekill PID. If the original process exits and the PID is reused beforekill, the replacement can be terminated; additionally, BSDstartvalues are commonly only minute-granularity, so a reuse within the same minute can pass the check even without a scheduling race. (lib/view/page/process.dart) — anchor-unreliable - 🟠 High The BSD path treats the human-formatted
ps start=field as a process identity, but the listing itself emitsstart=asSTART_ID; on BSD/macOS this format is not a unique creation identity (often a clock time or abbreviated date). Consequently, a legitimate process whose PID is reused during the same displayed time can pass the guard and be killed, violating the target-identity invariant. (lib/data/model/app/scripts/script_builders.dart) — anchor-unreliable - 🟠 High BSD PID recycling is not reliably prevented. The BSD script emits
ps ... start=...asSTART_ID, and_bsdKillProcessCmdauthorizes a kill when the current process has the same string. BSDstartis a display-formatted time (typically only a clock time for recent processes, or a calendar date for older ones), not a unique process start token; a recycled PID can therefore have the samestartvalue (for example two processes created in the same minute, or the same displayed date). In that case the refresh check and the shell kill target the recycled process rather than reporting that the target changed. (lib/view/page/process.dart) — anchor-unreliable - 🟠 High The BSD kill path has a PID-recycling TOCTOU even when
START_IDmatches. It runsps -p <pid>to compare the start value, then executes a separatekill <pid>; if the validated process exits and the PID is recycled in that interval, the signal is delivered to the new process. Thus the check does not establish the identity of the process actually killed. (lib/view/page/process.dart) — anchor-unreliable - 🟠 High Windows PID recycling can still defeat the identity check in a narrow TOCTOU window. The command obtains a process by PID, compares its
StartTime, and only afterward calls$p.Kill(). If that process exits and the PID is reused between the comparison and the kill operation, the kill operation can resolve the recycled PID and terminate the wrong process; the StartId check is not atomic with signaling. (lib/view/page/process.dart) — anchor-unreliable - 🟠 High The BSD kill command checks the PID's start value in one
psinvocation and then callskill <pid>separately; if the process exits and the PID is reused between those commands, the reused process can be terminated despite the target having changed, violating the changed/exited target safety invariant. (lib/view/page/process.dart) — anchor-unreliable - 🟡 Medium On Windows, disabling
WindowsStatusCmdType.cpu(while leavingcpuBrandenabled) also preserves the previous CPU brand indefinitely._parseWindowsCpuDatareturns before readingcpuBrandwhen the CPU section is absent, while_createWorkingStatusdeliberately reuses the mutable CPU object, so the UI shows a brand from an earlier refresh even though that section is no longer available. (lib/data/model/server/server_status_update_req.dart) — anchor-outside-diff - 🟡 Medium Disabling every status command makes the generated status function emit no bytes, so every refresh is classified as a transport/segment failure rather than a valid status with missing sections.
UnixScriptBuilder/WindowsScriptBuilderintentionally produce only:when all sections are filtered, butServerNotifier._getDatarejectsraw.isEmptybeforeparseScriptOutput; the connection is marked failed and no empty/partialServerStatuscan be consumed. The same occurs when a host has no enabled section due to configuration. (lib/data/provider/server/single.dart) — anchor-outside-diff - 🟡 Medium Unix process I/O counters are accepted even when negative, so malformed/invalid counters become real metrics and can later produce misleading rates and enable I/O capabilities. For example, a row containing
READ_BYTES=-1parses withreadBytes == -1instead of null/diagnostic, and a subsequent nonnegative sample can calculate a rate from that invalid baseline. The Windows path already requests nonnegative parsing, butProc._parsecalls_parseNullableIntwithout the same validation. (lib/data/model/server/proc.dart) — anchor-outside-diff - 🟡 Medium I/O capability detection is based on cumulative
readBytes/writeBytes, but the displayed and sortable columns are derived rates. On the first sample (and after PID identity changes), counters are present whilereadSpeed/writeSpeedare null; the page consequently exposes R/s/W/s columns and permits sorting by them even though every displayed value is null and the preferred sort can select an unusable mode. (lib/view/page/process.dart) — per-file-budget - 🟡 Medium The process page's compact layout hides the active metric sort controls while leaving the underlying metric sort mode unchanged. On a narrow screen, a user can be left with CPU/MEM/RSS ordering but no visible way to inspect or change that ordering, reducing usable PID/name operation and making the active sort inaccessible. (lib/view/page/process.dart) — per-file-budget
- 🟡 Medium A failed/empty refresh leaves the previous process snapshot in
_resultwhile marking the page loaded, so the page continues to display processes that may no longer exist (and generic command failures do not mark the data as stale). For example, after a successful load, disconnect the SSH client and tap refresh:_canRunProcessCmdreturns false, but the old rows remain visible; the same occurs when the command returns an empty string. This violates the refresh error behavior invariant that an unsuccessful sample must not be presented as current. The claim would be false if the product explicitly requires retaining stale rows without any stale/error state on connection or empty-command failures. (lib/view/page/process.dart) — per-file-budget - 🟡 Medium A pull-to-refresh started while the periodic/initial refresh is still running does not await the in-flight operation:
_refreshimmediately returns when_isRefreshingis true, andRefreshIndicator.onRefreshtherefore completes before the actual refresh (including its error handling) finishes. On a slow connection, the spinner can dismiss while the old rows are still being fetched, and a user pull provides no completion/error signal for that fetch. The claim would be false if the refresh indicator is deliberately allowed to complete independently of the in-flight refresh. (lib/view/page/process.dart) — per-file-budget - 🟡 Medium The Windows kill payload is emitted as raw PowerShell syntax and passed directly to
client.run(killCommand), unlike normal Windows script execution which explicitly prefixespowershell -ExecutionPolicy Bypass -File. On an SSH connection whose configured default shell is cmd.exe (or another non-PowerShell shell),$p,Get-Process, and the PowerShell conditionals are not interpreted, so termination fails (and can produce no success marker); the workflow is not platform-independent. (lib/view/page/process.dart) — per-file-budget - 🟡 Medium Windows status refreshes always discard SMART and sensor data, even though the Windows script emits both sections.
_createWorkingStatusinitializesdiskSmartto an empty list andsensorsis likewise a fresh empty collection, but_getWindowsStatusnever calls a parser forWindowsStatusCmdType.diskSmartorWindowsStatusCmdType.sensors(it only invokes the listed helpers through GPU and custom commands). Therefore every successful Windows refresh publishes empty SMART/sensor state and loses any prior values. (lib/data/model/server/server_status_update_req.dart) — anchor-outside-diff - 🟡 Medium Windows temperatures are converted to Celsius in the PowerShell command, then converted again by the parser when the normal Fahrenheit setting is used. The command computes
(CurrentTemperature - 2732) / 10, which is already Celsius, while_parseWindowsTemperaturesmultiplies it by 1000 and callsTemperatures.parsewithreq.tempDivisor; the default divisor is 1000, so a valid 45.0°C reading becomes 0.045°C (and the Celsius preference divisor 1.0 makes it 45,000°C). The Windows parser must use Windows' already-Celsius values independently of the Linux temperature divisor. (lib/data/model/server/server_status_update_req.dart) — anchor-unreliable - 🟡 Medium A valid full Windows volume is silently omitted because
FreeSpace == 0is treated as a missing required field.parseDisksrequiresfreeSpace != BigInt.zero, but a full filesystem legitimately reports zero free bytes; when that is the only volume, the refresh publishes no disk anddiskUsagebecomes null. This also violates the parser's required-field validation intent: zero is valid for FreeSpace, whereas negative free space orFreeSpace > Sizeshould be rejected. (lib/data/model/server/windows_parser.dart) — anchor-outside-diff - 🟡 Medium Windows memory parsing accepts malformed or physically impossible values as a successful result.
parseMemorydefaults missing fields to zero and returnsMemory(total: totalKB, free: freeKB, avail: freeKB)without checking that fields are present, nonnegative, orfreeKB <= totalKB;_parseWindowsMemoryDatathen replaces the fresh working snapshot's memory with this result. For example, a WMI error-shaped JSON object or{TotalVisibleMemorySize: 0, FreePhysicalMemory: 0}yields a zero-memory status instead of leaving the prior good state untouched, and{total: 100, free: 200}publishes impossible memory. (lib/data/model/server/windows_parser.dart) — anchor-outside-diff - 🟡 Medium Windows batteries with a valid
BatteryStatusof 3 (Fully Charged) are reported as discharging._parseWindowsBatteriesconsiders only statuses 6–8 charging and maps every other status, including documented status 3, toBatteryStatus.discharging. Thus a fully charged laptop is shown as actively discharging on every refresh. (lib/data/model/server/server_status_update_req.dart) — anchor-outside-diff - 🟡 Medium Windows CPU parsing does not validate the WMI numeric range before synthesizing cumulative counters. A malformed or out-of-range
LoadPercentage(for example 150 or -10) is accepted, producing negative idle or an invalid usage ratio inSingleCpuCore; invalid/zero core counts are also accepted and can produce no cores or inconsistent counts. Because_parseWindowsCpuDataupdates the rolling CPU state whenever any cores are returned, malformed WMI data can replace the refresh's CPU state instead of being rejected. (lib/data/model/server/windows_parser.dart) — anchor-unreliable - 🟡 Medium The BSD status section is not portable to FreeBSD/BSD despite being selected for every
uname -acontainingBSD:BSDStatusCmdType.memrunstop -l 1 | grep PhysMem, but BSDtopimplementations (notably FreeBSD) do not accept macOS's-loption or emitPhysMem. On FreeBSD this section returns no memory data (and may emit an error that is hidden by the script header), so BSD status monitoring loses memory/swap values. (lib/data/model/app/scripts/cmd_types.dart) — anchor-outside-diff - 🟡 Medium The status snapshot is not actually immutable:
_copyStatusand_createWorkingStatusretain the same mutableCpus,NetSpeed, andDiskIOinstances as the currently published state, andgetStatusupdates those objects in place. After the earlyupdateStatus(newStatus)in_getData, parsing can mutate the already-publishedstate.status.cpu/netSpeed/diskIO(including their rollingpre/nowlists and cached fields) without a corresponding Riverpod state assignment; widgets can therefore observe half-updated rolling data, and a parse failure can leave deltas/history advanced despite the failed refresh. This is disproven only ifComputer.shared.startalways deep-copies these objects before invokinggetStatusand never runs the parser against the passed instance; the source-side construction explicitly passesstate.statusand the working snapshot shares these references. (lib/data/model/server/server_status_update_req.dart) — inline-budget - 🟡 Medium Editing a server while its refresh is awaiting can publish status computed with the obsolete configuration after the edit.
_getDatacapturesfinal spi = state.spiat entry, then awaits connection/system detection/script execution/status parsing, but the finalupdateStatus(newStatus)andupdateConnection(finished)never verify thatstate.spiis still that SPI (or that the client/session is still current).updateSpionly replaces the state field. Thus changing custom commands, script directory, temperature units, or system-affecting SSH settings during a refresh can result in old output and oldsystembeing installed into the newly edited server state. This is false only if server edits are externally serialized against every await inrefresh;ServersNotifier.updateServerupdates the notifier and may invoke refresh independently, with no such synchronization. (lib/data/provider/server/single.dart) — anchor-unreliable - 🟡 Medium Scalar JSON Windows responses are not classified as invalid Windows JSON:
_parseWindowsJsonResultonly runs when trimmed output starts with{or[, so valid JSON such asnullor123falls through to Unix-header parsing and producesprocessParseUnsupportedOutputinstead of the required Windows JSON diagnostic. (lib/data/model/server/proc.dart) — inline-budget - 🟡 Medium The BSD/macOS process format is whitespace-tokenized even though
ps ... start=can contain an internal space (for exampleJun 5for processes older than a day). The generated loop doesset -- $lineand assignsstart_id=$10, so it captures onlyJun; the command is also shifted. The kill builder then compares that truncated value with the freshps -o start=output (Jun 5), so such processes cannot be killed and their displayed command/fields are malformed. (lib/data/model/app/scripts/script_builders.dart) — inline-budget - 🟡 Medium When the window shrinks below 700px,
_ProcessLayouthides all metric/user columns but leaves the previously selected sort mode unchanged. Thus a user sorted by CPU (or another metric) can resize to compact mode and see no active sort control/indicator while rows remain ordered by the hidden metric; the UI no longer exposes the control that describes the current ordering. The claim would be false if hidden-column sorting is explicitly intended and the compact view is not required to communicate its ordering. (lib/view/page/process.dart) — inline-budget - 🟡 Medium On Windows, the producer already converts WMI's tenths-of-Kelvin value to Celsius, but
_parseWindowsTemperaturesthen always multiplies it by 1000 before callingtemps.parse. When the per-servertempIsCelsiussetting is enabled,getStatuspassestempDivisor: 1.0, so the same value is interpreted as Celsius even though it was converted to fake millicelsius, displaying temperatures roughly 1000x too high. The Windows path needs to honor the divisor (or avoid the unconditional conversion) consistently with the setting. (lib/data/model/server/server_status_update_req.dart) — inline-budget - 🟡 Medium When a refresh parses rows but reports a partial parse issue, the process page keeps the old
PsResultincluding its oldsampledAtMillisinstead of retaining the newly sampled timestamp. After a transient bad row, the next successful refresh compares counters against the stale snapshot and divides by an interval spanning multiple samples, producing incorrect I/O rates (and potentially matching against stale process data). (lib/view/page/process.dart) — anchor-unreliable - 🟡 Medium BSD process rows are generated with
ps ... start=and then split on whitespace, so starts containing a date/time such asJun 10 12:34are stored only asJun. The kill command compares this truncated value to the full normalizedps -o start=result, causing every such target to be classified as changed and never killed; the workflow cannot fulfill successful termination on affected BSD/macOS systems. (lib/data/model/app/scripts/script_builders.dart) — anchor-unreliable - 🟡 Medium The process page cannot reliably parse or kill processes when a server has a custom script directory configured. Both process refresh and the pre-kill re-fetch call
ShellFunc.process.exec(..., customDir: null), so they execute the default cached script path, whereassingle.dartinstalls and runs the status script withspi.custom?.scriptDir. On a server configured with (for example)/opt/serverbox, the default script is absent or stale, yielding an empty/old process list and causing the new target-identity check to reportprocessKillTargetChangedor never reach the kill command. (lib/view/page/process.dart) — anchor-unreliable - 🔵 Low
binarynormalization only splits on the literal space character, while the parser otherwise treats all whitespace as separators. A valid process command beginning with a tab (or containing tab/newline whitespace), such as\t/usr/bin/worker\t--job, producesbinaryequal to the entire command rather than/usr/bin/worker, andargsis consequently empty instead of--job. This breaks the documented command/binary/args normalization for valid whitespace-separated command text. (lib/data/model/server/proc.dart) — inline-budget
♻️ Previously reported (still present) (2)
- 🟡 Medium A legacy command output containing a line beginning
SrvBoxSep.orSrvBoxCusCmdSep.is no longer preserved as command data. For example, raw legacy outputSrvBoxSep.echo\nhello\nSrvBoxSep.cpu\nworldis parsed as two sections (echo=hello,cpu=world) rather than theechopayload containing the literal second line. This affects existing scripts/custom commands whose output prints these marker prefixes and can silently feedworldto a different status parser. (lib/data/model/app/scripts/script_consts.dart) — previously-reported - 🟡 Medium The Unix process command reconstructs fields with shell
set -- $line, which splits on whitespace and then joins the remainder with single spaces. Although Dart preserves the whitespace it receives, the producer has already collapsed repeated spaces and cannot preserve command arguments containing intentional spacing; shell metacharacter/quoting in command text is also not treated as opaque. This violates the obligation that command text and arguments be preserved across Unix formats. (lib/data/model/app/scripts/script_builders.dart) — anchor-unreliable
❓ Low-evidence leads (not confirmed — verify before acting) (1)
- When a refresh parses rows but reports a partial parse issue, the page keeps the old
sampledAtMilliswhile replacing only the issue:_resultretains_result.procsand_result.sampledAtMillis. Subsequent successful parsing uses that stale timestamp as the previous sample interval, so I/O rates are divided by the entire time since the last fully successful refresh rather than the actual elapsed interval between samples. (lib/view/page/process.dart)
🤖 Prompt for AI agents — all findings (40)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Findings on this change (also posted as inline comments) (5)
In lib/view/page/process.dart around line 796, address this finding:
The Windows termination path sends a PowerShell program directly to the SSH client's generic `run` method instead of invoking `powershell` explicitly, so on Windows SSH servers whose default shell is `cmd.exe` the command is not interpreted as PowerShell and no process is killed.
In lib/view/page/process.dart around line 742, address this finding:
Process refresh and kill re-fetches ignore the server's configured custom script directory, so custom-script servers can return non-process output and the page may classify it as a parse diagnostic or fail to validate/kill the selected target.
In lib/view/page/process.dart around line 682, address this finding:
The BSD kill path can kill an unrelated process after PID reuse: it runs `ps -p <pid>` to validate the saved start value, then executes a separate `kill <pid>`. If the original process exits and the same PID is assigned before `kill`, the validation has already succeeded but the signal is delivered to the replacement process. This violates the changed/exited target invariant that the process selected in the UI must be the one terminated. The current tests cover parser validation and script generation, but have no kill-path test or implementation that closes this race.
In lib/data/model/app/scripts/script_builders.dart around line 26, address this finding:
Unix path quoting expands every syntactically valid `$NAME`/`${NAME}` prefix, not just the intended HOME/TEMP paths. A custom script directory such as `$PATH/server_box` is emitted as `"${PATH}"/'server_box'`, causing the remote shell to resolve the path from attacker- or user-controlled PATH rather than treating the supplied path literally. This violates the path contract and can redirect installation/execution; only the supported HOME/TEMP environment forms should be expanded.
In lib/data/model/app/scripts/script_consts.dart around line 109, address this finding:
The legacy-marker fallback accepts any line beginning with `SrvBoxSep.` or `SrvBoxCusCmdSep.` whose suffix is not `b64.`, including marker-like payload lines from legacy command output. For example, legacy output `SrvBoxSep.echo\nSrvBoxSep.cpu\nvalue` is parsed as two sections and overwrites/creates `cpu`, rather than preserving `SrvBoxSep.cpu` as payload. Because the parser has no framing/version distinction, malformed or marker-like legacy data is not handled safely.
## Additional findings on this change (not posted inline) (33)
In lib/data/provider/server/single.dart around line 195, address this finding:
A manual disconnect (or client replacement) can be undone by an in-flight refresh because the guard only prevents a second refresh; it does not invalidate the current operation. For example, while `genClient` or `_runStatusCommand` is awaiting, `closeConnection()` clears the client and sets `disconnected`, but the old refresh can later call `updateClient(client)` and then publish `connected`/`finished` and status. This resurrects a closed SSH client and leaves the UI showing a server that the user explicitly disconnected. The claim is false only if all callers guarantee that close/update operations cannot occur while refresh awaits, or if the underlying notifier drops all state mutations after close (neither is present here; `ServersNotifier.closeOneServer` directly calls `closeConnection`).
In lib/view/page/process.dart, address this finding:
The BSD listing uses `ps ... start=` as `START_ID`, and the kill command rechecks only that human-formatted start value before issuing an independent `kill PID`. This value is coarse/display-oriented rather than a unique birth identity, and the check-then-kill gap permits a recycled PID (or a process replacement between check and kill) to be terminated while still passing the guard.
In lib/view/page/process.dart, address this finding:
The BSD kill command's identity check is neither stable nor atomic: it compares the selected row's `START_ID` (populated from `ps ... start=`) with a fresh `ps -p ... -o start=` value and then performs a separate `kill PID`. If the original process exits and the PID is reused before `kill`, the replacement can be terminated; additionally, BSD `start` values are commonly only minute-granularity, so a reuse within the same minute can pass the check even without a scheduling race.
In lib/data/model/app/scripts/script_builders.dart, address this finding:
The BSD path treats the human-formatted `ps start=` field as a process identity, but the listing itself emits `start=` as `START_ID`; on BSD/macOS this format is not a unique creation identity (often a clock time or abbreviated date). Consequently, a legitimate process whose PID is reused during the same displayed time can pass the guard and be killed, violating the target-identity invariant.
In lib/view/page/process.dart, address this finding:
BSD PID recycling is not reliably prevented. The BSD script emits `ps ... start=...` as `START_ID`, and `_bsdKillProcessCmd` authorizes a kill when the current process has the same string. BSD `start` is a display-formatted time (typically only a clock time for recent processes, or a calendar date for older ones), not a unique process start token; a recycled PID can therefore have the same `start` value (for example two processes created in the same minute, or the same displayed date). In that case the refresh check and the shell kill target the recycled process rather than reporting that the target changed.
In lib/view/page/process.dart, address this finding:
The BSD kill path has a PID-recycling TOCTOU even when `START_ID` matches. It runs `ps -p <pid>` to compare the start value, then executes a separate `kill <pid>`; if the validated process exits and the PID is recycled in that interval, the signal is delivered to the new process. Thus the check does not establish the identity of the process actually killed.
In lib/view/page/process.dart, address this finding:
Windows PID recycling can still defeat the identity check in a narrow TOCTOU window. The command obtains a process by PID, compares its `StartTime`, and only afterward calls `$p.Kill()`. If that process exits and the PID is reused between the comparison and the kill operation, the kill operation can resolve the recycled PID and terminate the wrong process; the StartId check is not atomic with signaling.
In lib/view/page/process.dart, address this finding:
The BSD kill command checks the PID's start value in one `ps` invocation and then calls `kill <pid>` separately; if the process exits and the PID is reused between those commands, the reused process can be terminated despite the target having changed, violating the changed/exited target safety invariant.
In lib/data/model/server/server_status_update_req.dart around line 459, address this finding:
On Windows, disabling `WindowsStatusCmdType.cpu` (while leaving `cpuBrand` enabled) also preserves the previous CPU brand indefinitely. `_parseWindowsCpuData` returns before reading `cpuBrand` when the CPU section is absent, while `_createWorkingStatus` deliberately reuses the mutable CPU object, so the UI shows a brand from an earlier refresh even though that section is no longer available.
In lib/data/provider/server/single.dart around line 418, address this finding:
Disabling every status command makes the generated status function emit no bytes, so every refresh is classified as a transport/segment failure rather than a valid status with missing sections. `UnixScriptBuilder`/`WindowsScriptBuilder` intentionally produce only `:` when all sections are filtered, but `ServerNotifier._getData` rejects `raw.isEmpty` before `parseScriptOutput`; the connection is marked failed and no empty/partial `ServerStatus` can be consumed. The same occurs when a host has no enabled section due to configuration.
In lib/data/model/server/proc.dart around line 148, address this finding:
Unix process I/O counters are accepted even when negative, so malformed/invalid counters become real metrics and can later produce misleading rates and enable I/O capabilities. For example, a row containing `READ_BYTES=-1` parses with `readBytes == -1` instead of null/diagnostic, and a subsequent nonnegative sample can calculate a rate from that invalid baseline. The Windows path already requests nonnegative parsing, but `Proc._parse` calls `_parseNullableInt` without the same validation.
In lib/view/page/process.dart around line 878, address this finding:
I/O capability detection is based on cumulative `readBytes`/`writeBytes`, but the displayed and sortable columns are derived rates. On the first sample (and after PID identity changes), counters are present while `readSpeed`/`writeSpeed` are null; the page consequently exposes R/s/W/s columns and permits sorting by them even though every displayed value is null and the preferred sort can select an unusable mode.
In lib/view/page/process.dart around line 365, address this finding:
The process page's compact layout hides the active metric sort controls while leaving the underlying metric sort mode unchanged. On a narrow screen, a user can be left with CPU/MEM/RSS ordering but no visible way to inspect or change that ordering, reducing usable PID/name operation and making the active sort inaccessible.
In lib/view/page/process.dart around line 111, address this finding:
A failed/empty refresh leaves the previous process snapshot in `_result` while marking the page loaded, so the page continues to display processes that may no longer exist (and generic command failures do not mark the data as stale). For example, after a successful load, disconnect the SSH client and tap refresh: `_canRunProcessCmd` returns false, but the old rows remain visible; the same occurs when the command returns an empty string. This violates the refresh error behavior invariant that an unsuccessful sample must not be presented as current. The claim would be false if the product explicitly requires retaining stale rows without any stale/error state on connection or empty-command failures.
In lib/view/page/process.dart around line 102, address this finding:
A pull-to-refresh started while the periodic/initial refresh is still running does not await the in-flight operation: `_refresh` immediately returns when `_isRefreshing` is true, and `RefreshIndicator.onRefresh` therefore completes before the actual refresh (including its error handling) finishes. On a slow connection, the spinner can dismiss while the old rows are still being fetched, and a user pull provides no completion/error signal for that fetch. The claim would be false if the refresh indicator is deliberately allowed to complete independently of the in-flight refresh.
In lib/view/page/process.dart around line 641, address this finding:
The Windows kill payload is emitted as raw PowerShell syntax and passed directly to `client.run(killCommand)`, unlike normal Windows script execution which explicitly prefixes `powershell -ExecutionPolicy Bypass -File`. On an SSH connection whose configured default shell is cmd.exe (or another non-PowerShell shell), `$p`, `Get-Process`, and the PowerShell conditionals are not interpreted, so termination fails (and can produce no success marker); the workflow is not platform-independent.
In lib/data/model/server/server_status_update_req.dart around line 390, address this finding:
Windows status refreshes always discard SMART and sensor data, even though the Windows script emits both sections. `_createWorkingStatus` initializes `diskSmart` to an empty list and `sensors` is likewise a fresh empty collection, but `_getWindowsStatus` never calls a parser for `WindowsStatusCmdType.diskSmart` or `WindowsStatusCmdType.sensors` (it only invokes the listed helpers through GPU and custom commands). Therefore every successful Windows refresh publishes empty SMART/sensor state and loses any prior values.
In lib/data/model/server/server_status_update_req.dart, address this finding:
Windows temperatures are converted to Celsius in the PowerShell command, then converted again by the parser when the normal Fahrenheit setting is used. The command computes `(CurrentTemperature - 2732) / 10`, which is already Celsius, while `_parseWindowsTemperatures` multiplies it by 1000 and calls `Temperatures.parse` with `req.tempDivisor`; the default divisor is 1000, so a valid 45.0°C reading becomes 0.045°C (and the Celsius preference divisor 1.0 makes it 45,000°C). The Windows parser must use Windows' already-Celsius values independently of the Linux temperature divisor.
In lib/data/model/server/windows_parser.dart around line 213, address this finding:
A valid full Windows volume is silently omitted because `FreeSpace == 0` is treated as a missing required field. `parseDisks` requires `freeSpace != BigInt.zero`, but a full filesystem legitimately reports zero free bytes; when that is the only volume, the refresh publishes no disk and `diskUsage` becomes null. This also violates the parser's required-field validation intent: zero is valid for FreeSpace, whereas negative free space or `FreeSpace > Size` should be rejected.
In lib/data/model/server/windows_parser.dart around line 179, address this finding:
Windows memory parsing accepts malformed or physically impossible values as a successful result. `parseMemory` defaults missing fields to zero and returns `Memory(total: totalKB, free: freeKB, avail: freeKB)` without checking that fields are present, nonnegative, or `freeKB <= totalKB`; `_parseWindowsMemoryData` then replaces the fresh working snapshot's memory with this result. For example, a WMI error-shaped JSON object or `{TotalVisibleMemorySize: 0, FreePhysicalMemory: 0}` yields a zero-memory status instead of leaving the prior good state untouched, and `{total: 100, free: 200}` publishes impossible memory.
In lib/data/model/server/server_status_update_req.dart around line 660, address this finding:
Windows batteries with a valid `BatteryStatus` of 3 (Fully Charged) are reported as discharging. `_parseWindowsBatteries` considers only statuses 6–8 charging and maps every other status, including documented status 3, to `BatteryStatus.discharging`. Thus a fully charged laptop is shown as actively discharging on every refresh.
In lib/data/model/server/windows_parser.dart, address this finding:
Windows CPU parsing does not validate the WMI numeric range before synthesizing cumulative counters. A malformed or out-of-range `LoadPercentage` (for example 150 or -10) is accepted, producing negative idle or an invalid usage ratio in `SingleCpuCore`; invalid/zero core counts are also accepted and can produce no cores or inconsistent counts. Because `_parseWindowsCpuData` updates the rolling CPU state whenever any cores are returned, malformed WMI data can replace the refresh's CPU state instead of being rejected.
In lib/data/model/app/scripts/cmd_types.dart around line 156, address this finding:
The BSD status section is not portable to FreeBSD/BSD despite being selected for every `uname -a` containing `BSD`: `BSDStatusCmdType.mem` runs `top -l 1 | grep PhysMem`, but BSD `top` implementations (notably FreeBSD) do not accept macOS's `-l` option or emit `PhysMem`. On FreeBSD this section returns no memory data (and may emit an error that is hidden by the script header), so BSD status monitoring loses memory/swap values.
In lib/data/model/server/server_status_update_req.dart around line 62, address this finding:
The status snapshot is not actually immutable: `_copyStatus` and `_createWorkingStatus` retain the same mutable `Cpus`, `NetSpeed`, and `DiskIO` instances as the currently published state, and `getStatus` updates those objects in place. After the early `updateStatus(newStatus)` in `_getData`, parsing can mutate the already-published `state.status.cpu/netSpeed/diskIO` (including their rolling `pre/now` lists and cached fields) without a corresponding Riverpod state assignment; widgets can therefore observe half-updated rolling data, and a parse failure can leave deltas/history advanced despite the failed refresh. This is disproven only if `Computer.shared.start` always deep-copies these objects before invoking `getStatus` and never runs the parser against the passed instance; the source-side construction explicitly passes `state.status` and the working snapshot shares these references.
In lib/data/provider/server/single.dart, address this finding:
Editing a server while its refresh is awaiting can publish status computed with the obsolete configuration after the edit. `_getData` captures `final spi = state.spi` at entry, then awaits connection/system detection/script execution/status parsing, but the final `updateStatus(newStatus)` and `updateConnection(finished)` never verify that `state.spi` is still that SPI (or that the client/session is still current). `updateSpi` only replaces the state field. Thus changing custom commands, script directory, temperature units, or system-affecting SSH settings during a refresh can result in old output and old `system` being installed into the newly edited server state. This is false only if server edits are externally serialized against every await in `refresh`; `ServersNotifier.updateServer` updates the notifier and may invoke refresh independently, with no such synchronization.
In lib/data/model/server/proc.dart around line 378, address this finding:
Scalar JSON Windows responses are not classified as invalid Windows JSON: `_parseWindowsJsonResult` only runs when trimmed output starts with `{` or `[`, so valid JSON such as `null` or `123` falls through to Unix-header parsing and produces `processParseUnsupportedOutput` instead of the required Windows JSON diagnostic.
In lib/data/model/app/scripts/script_builders.dart around line 395, address this finding:
The BSD/macOS process format is whitespace-tokenized even though `ps ... start=` can contain an internal space (for example `Jun 5` for processes older than a day). The generated loop does `set -- $line` and assigns `start_id=$10`, so it captures only `Jun`; the command is also shifted. The kill builder then compares that truncated value with the fresh `ps -o start=` output (`Jun 5`), so such processes cannot be killed and their displayed command/fields are malformed.
In lib/view/page/process.dart around line 936, address this finding:
When the window shrinks below 700px, `_ProcessLayout` hides all metric/user columns but leaves the previously selected sort mode unchanged. Thus a user sorted by CPU (or another metric) can resize to compact mode and see no active sort control/indicator while rows remain ordered by the hidden metric; the UI no longer exposes the control that describes the current ordering. The claim would be false if hidden-column sorting is explicitly intended and the compact view is not required to communicate its ordering.
In lib/data/model/server/server_status_update_req.dart around line 782, address this finding:
On Windows, the producer already converts WMI's tenths-of-Kelvin value to Celsius, but `_parseWindowsTemperatures` then always multiplies it by 1000 before calling `temps.parse`. When the per-server `tempIsCelsius` setting is enabled, `getStatus` passes `tempDivisor: 1.0`, so the same value is interpreted as Celsius even though it was converted to fake millicelsius, displaying temperatures roughly 1000x too high. The Windows path needs to honor the divisor (or avoid the unconditional conversion) consistently with the setting.
In lib/view/page/process.dart, address this finding:
When a refresh parses rows but reports a partial parse issue, the process page keeps the old `PsResult` including its old `sampledAtMillis` instead of retaining the newly sampled timestamp. After a transient bad row, the next successful refresh compares counters against the stale snapshot and divides by an interval spanning multiple samples, producing incorrect I/O rates (and potentially matching against stale process data).
In lib/data/model/app/scripts/script_builders.dart, address this finding:
BSD process rows are generated with `ps ... start=` and then split on whitespace, so starts containing a date/time such as `Jun 10 12:34` are stored only as `Jun`. The kill command compares this truncated value to the full normalized `ps -o start=` result, causing every such target to be classified as changed and never killed; the workflow cannot fulfill successful termination on affected BSD/macOS systems.
In lib/view/page/process.dart, address this finding:
The process page cannot reliably parse or kill processes when a server has a custom script directory configured. Both process refresh and the pre-kill re-fetch call `ShellFunc.process.exec(..., customDir: null)`, so they execute the default cached script path, whereas `single.dart` installs and runs the status script with `spi.custom?.scriptDir`. On a server configured with (for example) `/opt/serverbox`, the default script is absent or stale, yielding an empty/old process list and causing the new target-identity check to report `processKillTargetChanged` or never reach the kill command.
In lib/data/model/server/proc.dart around line 223, address this finding:
`binary` normalization only splits on the literal space character, while the parser otherwise treats all whitespace as separators. A valid process command beginning with a tab (or containing tab/newline whitespace), such as `\t/usr/bin/worker\t--job`, produces `binary` equal to the entire command rather than `/usr/bin/worker`, and `args` is consequently empty instead of `--job`. This breaks the documented command/binary/args normalization for valid whitespace-separated command text.
## Previously reported and still present (2)
In lib/data/model/app/scripts/script_consts.dart around line 79, address this finding:
A legacy command output containing a line beginning `SrvBoxSep.` or `SrvBoxCusCmdSep.` is no longer preserved as command data. For example, raw legacy output `SrvBoxSep.echo\nhello\nSrvBoxSep.cpu\nworld` is parsed as two sections (`echo=hello`, `cpu=world`) rather than the `echo` payload containing the literal second line. This affects existing scripts/custom commands whose output prints these marker prefixes and can silently feed `world` to a different status parser.
In lib/data/model/app/scripts/script_builders.dart, address this finding:
The Unix process command reconstructs fields with shell `set -- $line`, which splits on whitespace and then joins the remainder with single spaces. Although Dart preserves the whitespace it receives, the producer has already collapsed repeated spaces and cannot preserve command arguments containing intentional spacing; shell metacharacter/quoting in command text is also not treated as opaque. This violates the obligation that command text and arguments be preserved across Unix formats.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- 5 of 5 areas reviewed
| context.showSnackBar(libL10n.notAvailable); | ||
| return; | ||
| } | ||
| final killOutput = await latestServerState.client! |
There was a problem hiding this comment.
🎯 Correctness | 🟠 High
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/view/page/process.dart, address this finding:
The Windows termination path sends a PowerShell program directly to the SSH client's generic `run` method instead of invoking `powershell` explicitly, so on Windows SSH servers whose default shell is `cmd.exe` the command is not interpreted as PowerShell and no process is killed.
Contributor
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 `@lib/data/model/server/server_status_update_req.dart`:
- Around line 468-480: Update the CPU brand count logic near
WindowsStatusCmdType.cpuBrand to use cpuResult.totalCoreCount when it is
positive, falling back to 1 otherwise, instead of req.ss.cpu.coresCount.
Preserve the existing brand parsing and assignment behavior.
In `@lib/data/provider/server/single.dart`:
- Around line 122-125: Update updateSpi to close the existing state.client and
dispose the persistent shell before replacing state. Publish a disconnected
state that retains the new Spi while clearing the old client and
connection-related state, ensuring the next refresh establishes a new SSH
connection.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1b8b22e3-f04a-4887-a8fb-006c8760e53d
⛔ Files ignored due to path filters (1)
lib/generated/l10n/l10n_fr.dartis excluded by!**/generated/**
📒 Files selected for processing (19)
lib/data/model/app/scripts/cmd_types.dartlib/data/model/app/scripts/script_builders.dartlib/data/model/app/scripts/script_consts.dartlib/data/model/server/cpu.dartlib/data/model/server/disk.dartlib/data/model/server/net_speed.dartlib/data/model/server/proc.dartlib/data/model/server/server_status_update_req.dartlib/data/model/server/temp.dartlib/data/model/server/time_seq.dartlib/data/model/server/windows_parser.dartlib/data/provider/server/single.dartlib/l10n/app_fr.arblib/view/page/process.darttest/l10n_fr_test.darttest/proc_test.darttest/script_builder_test.darttest/server_status_update_req_test.darttest/windows_test.dart
🚧 Files skipped from review as they are similar to previous changes (7)
- lib/l10n/app_fr.arb
- test/script_builder_test.dart
- lib/data/model/app/scripts/script_builders.dart
- test/proc_test.dart
- lib/data/model/app/scripts/script_consts.dart
- lib/view/page/process.dart
- lib/data/model/server/proc.dart
There was a problem hiding this comment.
Actionable comments posted: 3
🛠️ To have the bot fix these findings, comment @winnowl fix.
🔎 Confirmed findings (3)
- 🟡 Medium Unix custom-command framing changes the execution context by piping the entire body through
sed; shell bodies that rely on their exit status or surrounding control flow do not retain normal command semantics, and a body can terminate/close the wrapper because it is interpolated as raw shell source inside{ ... }. For example, a custom body containingexit 7exits only the wrapper subshell and the generated status function continues, while a body containing an unmatched}makes the generated script syntactically invalid rather than being safely framed. (inline) - 🟡 Medium The page discards valid partial rows whenever one malformed row is present.
PsResult.parseintentionally returns successfully parsed processes together withissue, but_refreshreplaces that withPsResult(procs: const [], issue: parsed.issue). Thus a Unix output containing 99 valid rows and one malformed row (or Windows JSON with partial rows) displays count 0 and no rows, while only exposing the diagnostic icon; this loses the parser's partial result and makes localized count/sort state misleading. This would be false only if the intended UI contract is to hide all valid rows whenever any optional row is malformed. (inline) - 🟡 Medium Termination success is accepted if the raw remote output merely contains
SrvBoxKill.Succeeded, rather than requiring an exact, unambiguous result from the kill script. Any unrelated output from the remote shell/session (for example a profile/banner or command output containing that marker) can make_killAndRefreshsetkilled = trueeven when the termination script failed or emitted a failure marker, causing the UI to report success and refresh as though the target was stopped. This would be false only if the SSH execution layer guarantees that output is exclusively the kill script's marker and cannot contain attacker- or server-controlled text. (inline)
⛔ Unresolved from previous review (10) — not approved until fixed
- The BSD kill command checks the PID's start value in one
psinvocation and then callskill <pid>separately; if the process exits and the PID is reused between those commands, the reused process can be terminated despite the target having changed, violating the changed/exited target safety invariant. - Windows PID recycling can still defeat the identity check in a narrow TOCTOU window. The command obtains a process by PID, compares its
StartTime, and only afterward calls$p.Kill(). If that process exits and the PID is reused between the comparison and the kill operation, the kill operation can resolve the recycled PID and terminate the wrong process; the StartId check is not atomic with signaling. - The BSD kill path has a PID-recycling TOCTOU even when
START_IDmatches. It runsps -p <pid>to compare the start value, then executes a separatekill <pid>; if the validated process exits and the PID is recycled in that interval, the signal is delivered to the new process. Thus the check does not establish the identity of the process actually killed. - BSD PID recycling is not reliably prevented. The BSD script emits
ps ... start=...asSTART_ID, and_bsdKillProcessCmdauthorizes a kill when the current process has the same string. BSDstartis a display-formatted time (typically only a clock time for recent processes, or a calendar date for older ones), not a unique process start token; a recycled PID can therefore have the samestartvalue (for example two processes created in the same minute, or the same displayed date). In that case the refresh check and the shell kill target the recycled process rather than reporting that the target changed. - The BSD path treats the human-formatted
ps start=field as a process identity, but the listing itself emitsstart=asSTART_ID; on BSD/macOS this format is not a unique creation identity (often a clock time or abbreviated date). Consequently, a legitimate process whose PID is reused during the same displayed time can pass the guard and be killed, violating the target-identity invariant. - The BSD kill command's identity check is neither stable nor atomic: it compares the selected row's
START_ID(populated fromps ... start=) with a freshps -p ... -o start=value and then performs a separatekill PID. If the original process exits and the PID is reused beforekill, the replacement can be terminated; additionally, BSDstartvalues are commonly only minute-granularity, so a reuse within the same minute can pass the check even without a scheduling race. - The BSD listing uses
ps ... start=asSTART_ID, and the kill command rechecks only that human-formatted start value before issuing an independentkill PID. This value is coarse/display-oriented rather than a unique birth identity, and the check-then-kill gap permits a recycled PID (or a process replacement between check and kill) to be terminated while still passing the guard. - lib/view/page/process.dart: Process refresh and kill re-fetches ignore the server's configured custom script directory, so custom-script servers can return non-process output and the page may classify it as a parse diagnostic or fail to validate/kill the selected target.
- lib/data/provider/server/single.dart: A manual disconnect (or client replacement) can be undone by an in-flight refresh because the guard only prevents a second refresh; it does not invalidate the current operation. For example, while
genClientor_runStatusCommandis awaiting,closeConnection()clears the client and setsdisconnected, but the old refresh can later callupdateClient(client)and then publishconnected/finishedand status. This resurrects a closed SSH client and leaves the UI showing a server that the user explicitly disconnected. The claim is false only if all callers guarantee that close/update operations cannot occur while refresh awaits, or if the underlying notifier drops all state mutations after close (neither is present here;ServersNotifier.closeOneServerdirectly callscloseConnection). - lib/view/page/process.dart: The Windows termination path sends a PowerShell program directly to the SSH client's generic
runmethod instead of invokingpowershellexplicitly, so on Windows SSH servers whose default shell iscmd.exethe command is not interpreted as PowerShell and no process is killed.
⚠️ Unverified risks (1)
- Disk I/O speed calculation divides by a non-positive interval and accepts counter rollbacks. When two samples have equal timestamps,
_getSpeedperforms division by zero; when a kernel counter resets, it returns negative bytes/sec. These values are then formatted and cached byonUpdate, violating the requirement to avoid invalid rates. (lib/data/model/server/disk.dart)
📋 Additional findings from this change (not shown inline) (18)
- 🟠 High Same-length samples are never aligned by identity, so reordering records silently pairs counters from different devices/cores. For example, a two-interface sample [eth0=100, eth1=100] followed by [eth1=200, eth0=300] has equal lengths, skips the
same()matching block, and reports eth0's delta as 100 (using old eth0 at index 0) instead of 200 and eth1's as 200 instead of 100; the same error affects CPU core rows. (lib/data/model/server/time_seq.dart) — anchor-outside-diff - 🟠 High Changing a server's SPI does not invalidate its existing SSH client:
updateSpiincrements the operation generation but only copies the new SPI, while_refreshimmediately reads the newspitogether with the oldclientand executes the process command through that client. A process-page refresh after host/user/port changes can therefore query the prior connection while labeling it as the new server/configuration; the kill flow's SPI equality check compares the same already-mismatched state and cannot detect this. This would be false only if all SPI updates are guaranteed to close/replace the client elsewhere before any process-page refresh. (lib/data/provider/server/single.dart) — anchor-outside-diff - 🟡 Medium Network rates can become negative after a counter reset/wrap.
speedInBytesandspeedOutBytessubtract the previous counter without checking that the new counter is at least as large; a reset from 1000 to 10 with a positive interval publishes a negative speed, which can render as an invalid rate and corrupt aggregates. (lib/data/model/server/net_speed.dart) — anchor-outside-diff - 🟡 Medium Windows disk parsing aborts the entire disk batch when a scalar/null row is present.
parseDisksnormalizes a JSON array but then immediately indexes eachdiskDataas a map; a mixed PowerShell result such as[validDisk, null, validDisk]throws, the outer catch returns[], and valid disks are lost instead of being retained independently. (lib/data/model/server/windows_parser.dart) — anchor-outside-diff - 🟡 Medium Windows disk sizes are truncated independently before computing used space:
usedKB = (size ~/ 1024) - (freeSpace ~/ 1024). For byte values whose remainder crosses a KiB boundary (for example size=2049, free=1025), this reports used=1 KiB although the actual used bytes are 1024 (and other combinations can make the displayed fields inconsistent). (lib/data/model/server/windows_parser.dart) — anchor-outside-diff - 🟡 Medium Windows battery rows with numeric-string WMI values are silently discarded along with all valid rows.
_parseWindowsBatteriesusesas int?for both fields; PowerShell/WMI can serialize these values as strings, causing a cast exception, the outer catch returns an empty list, and the caller clears the existing battery collection. (lib/data/model/server/server_status_update_req.dart) — anchor-outside-diff - 🟡 Medium A malformed/non-object row aborts Windows temperature parsing for the entire batch. The loop indexes every
itemas a map, so a scalar/null mixed into[validTemperature, null, validTemperature]throws and the catch prevents even the valid rows from being added. (lib/data/model/server/server_status_update_req.dart) — anchor-outside-diff - 🟡 Medium CPU percentages become negative or exceed 100% after cumulative counters reset/wrap.
usedPercentand_getUser/_getSys/_getIowaitdivide raw counter differences without rejecting negative total or component deltas; a reset can therefore publish a negative usage (or invalid component percentages) instead of treating the sample as unusable. (lib/data/model/server/cpu.dart) — anchor-outside-diff - 🟡 Medium A scalar/null row in a Windows battery JSON array aborts the whole batch, because each row is indexed as a map inside one outer try/catch. Valid batteries before or after the malformed row are lost, and the caller clears the existing collection when the resulting list is empty. (lib/data/model/server/server_status_update_req.dart) — anchor-outside-diff
- 🟡 Medium A malformed/scalar element in a PowerShell disk array discards every otherwise valid disk in the same response.
parseDisksputs the whole decode-and-loop in one try, but indexes eachdiskDataas a map without checking its type; for input such as[{"DeviceID":"C:","Size":1024,"FreeSpace":0,"FileSystem":"NTFS"},null], the second element throws and the outer catch returns[]instead of retaining C:. A single bad WMI row can therefore blank the disk status. (lib/data/model/server/windows_parser.dart) — anchor-outside-diff - 🟡 Medium CPU counter resets are treated as ordinary deltas. When the kernel counters reset (e.g. reboot or hotplug) and
now.total < pre.total,onUpdatepasses a negative_totalDeltainto_getUser/_getSys/_getIowait, producing negative component percentages andusedPercentcan exceed 100%; no reset sample is discarded or clamped. (lib/data/model/server/cpu.dart) — anchor-outside-diff - 🟡 Medium An in-flight process refresh is not invalidated when the server connection or configuration changes.
_refreshcapturesserverState.client,systemType, and SPI, awaits the command, then unconditionally assigns the parsed result (the only guard ismounted). If the user switches/reconnects the server or changes its SPI while that command is running, the old connection's process list can overwrite the new configuration's state and become_lastValidResult, so subsequent I/O-rate matching and termination decisions use a prior connection's snapshot. This would be false only if provider changes are guaranteed to dispose/recreate the page before every in-flight command completes. (lib/view/page/process.dart) — anchor-outside-diff - 🟡 Medium Unix process rows accept negative CPU and memory percentages instead of treating those metrics as invalid. For input
PID %CPU %MEM COMMAND\n1 -2.5 -1 /bad,_parseNullableDoublereturns both negative values, so the UI displays negative resource usage and CPU/MEM sorting ranks this row as a real metric. CPU and memory percentages are bounded non-negative metrics, so malformed/sentinel values should be omitted like the existing negative-RSS handling. (lib/data/model/server/proc.dart) — inline-budget - 🟡 Medium French runtime localization regresses to English for a substantial set of keys that are exposed by the generated French implementation. For example,
pveLoadingForwarding,pvePassword,tmuxAutoAttach, andportForward_startPromptreturn English strings inAppLocalizationsFr, and the corresponding French ARB entries are absent, so selecting French still shows English for these runtime-facing controls instead of a French translation. (lib/generated/l10n/l10n_fr.dart) — inline-budget - 🟡 Medium The French source/output contains a wrong-language value:
fallbackSshDestis Spanish (Destino SSH alternativo) in bothapp_fr.arbandAppLocalizationsFr. The server edit page consumes this key as the label for the fallback SSH destination, so French users see Spanish UI text. (lib/l10n/app_fr.arb) — inline-budget - 🟡 Medium Several newly generated French entries are stale untranslated English rather than French, including
askAiEndpointTip,icloudBackupStatusTitle/status messages,configured, GitHub Gist labels, invalid-host validation, jump-server errors, PVE login/password errors, temperature guidance, SSH connection-mode labels, sync settings, port-forward labels, and the entire tmux UI. These are runtime strings (for exampleaskAiEndpointTipis passed as a setting description), so French users see English instead of French across these features. (lib/generated/l10n/l10n_fr.dart) — inline-budget - 🟡 Medium app_fr.arb is missing multiple keys present in the English source, including
askAiEndpointTip, all iCloud-backup status/state keys (icloudBackupStatusTitle,icloudBackupStatusSummary, etc.),invalidUrl,jumpServersNotFoundFmt, the PVE login/OTP messages (pveServerClientMissingthroughpvePasswordHint), and the tmux messages (tmuxAutoAttachthroughtmuxNotAvailable). French users therefore fall back to English for those labels instead of receiving a complete French locale. (lib/l10n/app_fr.arb) — inline-budget - 🔵 Low Windows RSS is rounded up to the next KiB, overstating memory and changing RSS ordering at sub-KiB boundaries. For
WorkingSet: 1025, the parser stores RSS as2KiB via((workingSetBytes + 1023) ~/ 1024), and the UI consequently displays 2 KiB even though the process uses only 1.0009 KiB; a process at 2048 bytes also stores 2 KiB and ties it. Unix RSS is already an integer KiB value and byte-to-KiB normalization should use the same floor/truncation convention. (lib/data/model/server/proc.dart) — inline-budget
❓ Low-evidence leads (not confirmed — verify before acting) (1)
- Refresh failures and timeouts discard the last valid process snapshot instead of retaining it. In both catch branches
_resultis replaced with an empty result, even though_lastValidResultexists and is only updated after successful parsing. During a transient SSH timeout/error after a successful load, the page therefore loses all rows and shows an error/empty state; repeated periodic failures can make the page unusable until a later success, contrary to the last-valid-snapshot obligation. This would be false only if the intended behavior is explicitly to hide valid process data on every transient refresh failure. (lib/view/page/process.dart)
🤖 Prompt for AI agents — all findings (31)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Unresolved from the previous review — these block approval, fix them first (10)
Somewhere in the code under review, address this finding:
The BSD kill command checks the PID's start value in one `ps` invocation and then calls `kill <pid>` separately; if the process exits and the PID is reused between those commands, the reused process can be terminated despite the target having changed, violating the changed/exited target safety invariant.
Somewhere in the code under review, address this finding:
Windows PID recycling can still defeat the identity check in a narrow TOCTOU window. The command obtains a process by PID, compares its `StartTime`, and only afterward calls `$p.Kill()`. If that process exits and the PID is reused between the comparison and the kill operation, the kill operation can resolve the recycled PID and terminate the wrong process; the StartId check is not atomic with signaling.
Somewhere in the code under review, address this finding:
The BSD kill path has a PID-recycling TOCTOU even when `START_ID` matches. It runs `ps -p <pid>` to compare the start value, then executes a separate `kill <pid>`; if the validated process exits and the PID is recycled in that interval, the signal is delivered to the new process. Thus the check does not establish the identity of the process actually killed.
Somewhere in the code under review, address this finding:
BSD PID recycling is not reliably prevented. The BSD script emits `ps ... start=...` as `START_ID`, and `_bsdKillProcessCmd` authorizes a kill when the current process has the same string. BSD `start` is a display-formatted time (typically only a clock time for recent processes, or a calendar date for older ones), not a unique process start token; a recycled PID can therefore have the same `start` value (for example two processes created in the same minute, or the same displayed date). In that case the refresh check and the shell kill target the recycled process rather than reporting that the target changed.
Somewhere in the code under review, address this finding:
The BSD path treats the human-formatted `ps start=` field as a process identity, but the listing itself emits `start=` as `START_ID`; on BSD/macOS this format is not a unique creation identity (often a clock time or abbreviated date). Consequently, a legitimate process whose PID is reused during the same displayed time can pass the guard and be killed, violating the target-identity invariant.
Somewhere in the code under review, address this finding:
The BSD kill command's identity check is neither stable nor atomic: it compares the selected row's `START_ID` (populated from `ps ... start=`) with a fresh `ps -p ... -o start=` value and then performs a separate `kill PID`. If the original process exits and the PID is reused before `kill`, the replacement can be terminated; additionally, BSD `start` values are commonly only minute-granularity, so a reuse within the same minute can pass the check even without a scheduling race.
Somewhere in the code under review, address this finding:
The BSD listing uses `ps ... start=` as `START_ID`, and the kill command rechecks only that human-formatted start value before issuing an independent `kill PID`. This value is coarse/display-oriented rather than a unique birth identity, and the check-then-kill gap permits a recycled PID (or a process replacement between check and kill) to be terminated while still passing the guard.
In lib/view/page/process.dart, address this finding:
Process refresh and kill re-fetches ignore the server's configured custom script directory, so custom-script servers can return non-process output and the page may classify it as a parse diagnostic or fail to validate/kill the selected target.
In lib/data/provider/server/single.dart, address this finding:
A manual disconnect (or client replacement) can be undone by an in-flight refresh because the guard only prevents a second refresh; it does not invalidate the current operation. For example, while `genClient` or `_runStatusCommand` is awaiting, `closeConnection()` clears the client and sets `disconnected`, but the old refresh can later call `updateClient(client)` and then publish `connected`/`finished` and status. This resurrects a closed SSH client and leaves the UI showing a server that the user explicitly disconnected. The claim is false only if all callers guarantee that close/update operations cannot occur while refresh awaits, or if the underlying notifier drops all state mutations after close (neither is present here; `ServersNotifier.closeOneServer` directly calls `closeConnection`).
In lib/view/page/process.dart, address this finding:
The Windows termination path sends a PowerShell program directly to the SSH client's generic `run` method instead of invoking `powershell` explicitly, so on Windows SSH servers whose default shell is `cmd.exe` the command is not interpreted as PowerShell and no process is killed.
## Findings on this change (also posted as inline comments) (3)
In lib/data/model/app/scripts/script_builders.dart around line 48, address this finding:
Unix custom-command framing changes the execution context by piping the entire body through `sed`; shell bodies that rely on their exit status or surrounding control flow do not retain normal command semantics, and a body can terminate/close the wrapper because it is interpolated as raw shell source inside `{ ... }`. For example, a custom body containing `exit 7` exits only the wrapper subshell and the generated status function continues, while a body containing an unmatched `}` makes the generated script syntactically invalid rather than being safely framed.
In lib/view/page/process.dart around line 152, address this finding:
The page discards valid partial rows whenever one malformed row is present. `PsResult.parse` intentionally returns successfully parsed processes together with `issue`, but `_refresh` replaces that with `PsResult(procs: const [], issue: parsed.issue)`. Thus a Unix output containing 99 valid rows and one malformed row (or Windows JSON with partial rows) displays count 0 and no rows, while only exposing the diagnostic icon; this loses the parser's partial result and makes localized count/sort state misleading. This would be false only if the intended UI contract is to hide all valid rows whenever any optional row is malformed.
In lib/view/page/process.dart around line 889, address this finding:
Termination success is accepted if the raw remote output merely contains `SrvBoxKill.Succeeded`, rather than requiring an exact, unambiguous result from the kill script. Any unrelated output from the remote shell/session (for example a profile/banner or command output containing that marker) can make `_killAndRefresh` set `killed = true` even when the termination script failed or emitted a failure marker, causing the UI to report success and refresh as though the target was stopped. This would be false only if the SSH execution layer guarantees that output is exclusively the kill script's marker and cannot contain attacker- or server-controlled text.
## Additional findings on this change (not posted inline) (18)
In lib/data/model/server/time_seq.dart around line 64, address this finding:
Same-length samples are never aligned by identity, so reordering records silently pairs counters from different devices/cores. For example, a two-interface sample [eth0=100, eth1=100] followed by [eth1=200, eth0=300] has equal lengths, skips the `same()` matching block, and reports eth0's delta as 100 (using old eth0 at index 0) instead of 200 and eth1's as 200 instead of 100; the same error affects CPU core rows.
In lib/data/provider/server/single.dart around line 123, address this finding:
Changing a server's SPI does not invalidate its existing SSH client: `updateSpi` increments the operation generation but only copies the new SPI, while `_refresh` immediately reads the new `spi` together with the old `client` and executes the process command through that client. A process-page refresh after host/user/port changes can therefore query the prior connection while labeling it as the new server/configuration; the kill flow's SPI equality check compares the same already-mismatched state and cannot detect this. This would be false only if all SPI updates are guaranteed to close/replace the client elsewhere before any process-page refresh.
In lib/data/model/server/net_speed.dart around line 96, address this finding:
Network rates can become negative after a counter reset/wrap. `speedInBytes` and `speedOutBytes` subtract the previous counter without checking that the new counter is at least as large; a reset from 1000 to 10 with a positive interval publishes a negative speed, which can render as an invalid rate and corrupt aggregates.
In lib/data/model/server/windows_parser.dart around line 223, address this finding:
Windows disk parsing aborts the entire disk batch when a scalar/null row is present. `parseDisks` normalizes a JSON array but then immediately indexes each `diskData` as a map; a mixed PowerShell result such as `[validDisk, null, validDisk]` throws, the outer catch returns `[]`, and valid disks are lost instead of being retained independently.
In lib/data/model/server/windows_parser.dart around line 250, address this finding:
Windows disk sizes are truncated independently before computing used space: `usedKB = (size ~/ 1024) - (freeSpace ~/ 1024)`. For byte values whose remainder crosses a KiB boundary (for example size=2049, free=1025), this reports used=1 KiB although the actual used bytes are 1024 (and other combinations can make the displayed fields inconsistent).
In lib/data/model/server/server_status_update_req.dart around line 667, address this finding:
Windows battery rows with numeric-string WMI values are silently discarded along with all valid rows. `_parseWindowsBatteries` uses `as int?` for both fields; PowerShell/WMI can serialize these values as strings, causing a cast exception, the outer catch returns an empty list, and the caller clears the existing battery collection.
In lib/data/model/server/server_status_update_req.dart around line 789, address this finding:
A malformed/non-object row aborts Windows temperature parsing for the entire batch. The loop indexes every `item` as a map, so a scalar/null mixed into `[validTemperature, null, validTemperature]` throws and the catch prevents even the valid rows from being added.
In lib/data/model/server/cpu.dart around line 56, address this finding:
CPU percentages become negative or exceed 100% after cumulative counters reset/wrap. `usedPercent` and `_getUser/_getSys/_getIowait` divide raw counter differences without rejecting negative total or component deltas; a reset can therefore publish a negative usage (or invalid component percentages) instead of treating the sample as unusable.
In lib/data/model/server/server_status_update_req.dart around line 669, address this finding:
A scalar/null row in a Windows battery JSON array aborts the whole batch, because each row is indexed as a map inside one outer try/catch. Valid batteries before or after the malformed row are lost, and the caller clears the existing collection when the resulting list is empty.
In lib/data/model/server/windows_parser.dart around line 222, address this finding:
A malformed/scalar element in a PowerShell disk array discards every otherwise valid disk in the same response. `parseDisks` puts the whole decode-and-loop in one try, but indexes each `diskData` as a map without checking its type; for input such as `[{"DeviceID":"C:","Size":1024,"FreeSpace":0,"FileSystem":"NTFS"},null]`, the second element throws and the outer catch returns `[]` instead of retaining C:. A single bad WMI row can therefore blank the disk status.
In lib/data/model/server/cpu.dart around line 40, address this finding:
CPU counter resets are treated as ordinary deltas. When the kernel counters reset (e.g. reboot or hotplug) and `now.total < pre.total`, `onUpdate` passes a negative `_totalDelta` into `_getUser/_getSys/_getIowait`, producing negative component percentages and `usedPercent` can exceed 100%; no reset sample is discarded or clamped.
In lib/view/page/process.dart around line 136, address this finding:
An in-flight process refresh is not invalidated when the server connection or configuration changes. `_refresh` captures `serverState.client`, `systemType`, and SPI, awaits the command, then unconditionally assigns the parsed result (the only guard is `mounted`). If the user switches/reconnects the server or changes its SPI while that command is running, the old connection's process list can overwrite the new configuration's state and become `_lastValidResult`, so subsequent I/O-rate matching and termination decisions use a prior connection's snapshot. This would be false only if provider changes are guaranteed to dispose/recreate the page before every in-flight command completes.
In lib/data/model/server/proc.dart around line 167, address this finding:
Unix process rows accept negative CPU and memory percentages instead of treating those metrics as invalid. For input `PID %CPU %MEM COMMAND\n1 -2.5 -1 /bad`, `_parseNullableDouble` returns both negative values, so the UI displays negative resource usage and CPU/MEM sorting ranks this row as a real metric. CPU and memory percentages are bounded non-negative metrics, so malformed/sentinel values should be omitted like the existing negative-RSS handling.
In lib/generated/l10n/l10n_fr.dart around line 641, address this finding:
French runtime localization regresses to English for a substantial set of keys that are exposed by the generated French implementation. For example, `pveLoadingForwarding`, `pvePassword`, `tmuxAutoAttach`, and `portForward_startPrompt` return English strings in `AppLocalizationsFr`, and the corresponding French ARB entries are absent, so selecting French still shows English for these runtime-facing controls instead of a French translation.
In lib/l10n/app_fr.arb around line 81, address this finding:
The French source/output contains a wrong-language value: `fallbackSshDest` is Spanish (`Destino SSH alternativo`) in both `app_fr.arb` and `AppLocalizationsFr`. The server edit page consumes this key as the label for the fallback SSH destination, so French users see Spanish UI text.
In lib/generated/l10n/l10n_fr.dart around line 40, address this finding:
Several newly generated French entries are stale untranslated English rather than French, including `askAiEndpointTip`, `icloudBackupStatusTitle`/status messages, `configured`, GitHub Gist labels, invalid-host validation, jump-server errors, PVE login/password errors, temperature guidance, SSH connection-mode labels, sync settings, port-forward labels, and the entire tmux UI. These are runtime strings (for example `askAiEndpointTip` is passed as a setting description), so French users see English instead of French across these features.
In lib/l10n/app_fr.arb around line 240, address this finding:
app_fr.arb is missing multiple keys present in the English source, including `askAiEndpointTip`, all iCloud-backup status/state keys (`icloudBackupStatusTitle`, `icloudBackupStatusSummary`, etc.), `invalidUrl`, `jumpServersNotFoundFmt`, the PVE login/OTP messages (`pveServerClientMissing` through `pvePasswordHint`), and the tmux messages (`tmuxAutoAttach` through `tmuxNotAvailable`). French users therefore fall back to English for those labels instead of receiving a complete French locale.
In lib/data/model/server/proc.dart around line 220, address this finding:
Windows RSS is rounded up to the next KiB, overstating memory and changing RSS ordering at sub-KiB boundaries. For `WorkingSet: 1025`, the parser stores RSS as `2` KiB via `((workingSetBytes + 1023) ~/ 1024)`, and the UI consequently displays 2 KiB even though the process uses only 1.0009 KiB; a process at 2048 bytes also stores 2 KiB and ties it. Unix RSS is already an integer KiB value and byte-to-KiB normalization should use the same floor/truncation convention.
📜 Review details
Model
- gpt-5.6-luna, glm-5.2
Coverage
- 4 of 4 areas reviewed
| printf '%s\\n' ${_quoteUnixLiteral(marker)} | ||
| { | ||
| $body | ||
| } | sed 's/^/${ScriptConstants.dataPrefix}/' |
There was a problem hiding this comment.
🔒 Security | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/data/model/app/scripts/script_builders.dart, address this finding:
Unix custom-command framing changes the execution context by piping the entire body through `sed`; shell bodies that rely on their exit status or surrounding control flow do not retain normal command semantics, and a body can terminate/close the wrapper because it is interpolated as raw shell source inside `{ ... }`. For example, a custom body containing `exit 7` exits only the wrapper subshell and the generated status function continues, while a body containing an unmatched `}` makes the generated script syntactically invalid rather than being safely framed.
| _procSortMode = _sortModes.first; | ||
| } | ||
| _checkedIncompleteData = true; | ||
| if (parsed.issue != null) { |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In lib/view/page/process.dart, address this finding:
The page discards valid partial rows whenever one malformed row is present. `PsResult.parse` intentionally returns successfully parsed processes together with `issue`, but `_refresh` replaces that with `PsResult(procs: const [], issue: parsed.issue)`. Thus a Unix output containing 99 valid rows and one malformed row (or Windows JSON with partial rows) displays count 0 and no rows, while only exposing the diagnostic icon; this loses the parser's partial result and makes localized count/sort state misleading. This would be false only if the intended UI contract is to hide all valid rows whenever any optional row is malformed.
| context.showSnackBar(libL10n.error); | ||
| return; | ||
| } | ||
| killed = true; |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
⚠️ The external SSH client's exact handling of remote shell startup output is not present in this repository; the defect applies whenever that layer or a forced/non-interactive shell emits additional output.
🤖 Prompt for AI agents
In lib/view/page/process.dart, address this finding:
Termination success is accepted if the raw remote output merely contains `SrvBoxKill.Succeeded`, rather than requiring an exact, unambiguous result from the kill script. Any unrelated output from the remote shell/session (for example a profile/banner or command output containing that marker) can make `_killAndRefresh` set `killed = true` even when the termination script failed or emitted a failure marker, causing the UI to report success and refresh as though the target was stopped. This would be false only if the SSH execution layer guarantees that output is exclusively the kill script's marker and cannot contain attacker- or server-controlled text.
📝 Committable suggestion — review it before committing; it is generated, not proven.
Suggested change
| killed = true; | |
| if (killOutput.trim() != _killSucceededMarker) { | |
| context.showSnackBar(libL10n.error); | |
| return; | |
| } | |
| killed = true; |
lollipopkit
added a commit
that referenced
this pull request
Aug 10, 2026
Conflicts sat where this branch had already moved parsing/script generation into sbm_parser, so upstream's Dart-side edits to cmd_types/script_consts/server_status_update_req had no counterpart here. Resolved by keeping the Rust implementation and taking only the behaviour upstream's changes encode. Adopted from upstream: - Win32_Battery status 3 is "fully charged", not a discharge state. sbm_parser folded everything outside 6-8 into Discharging, so a laptop sitting at 100% on mains reported as draining. Now maps the full enumeration (3 full, 6-9 charging, 2/10 unknown), with the spec ported to dart_compat.rs. - Temperatures.copy / NetSpeed.copy, matching the copy constructors this branch already added to Cpus and Disks. Diverged deliberately (see CLAUDE.md "test as spec"): - Time series no longer start from a synthetic seed sample. The seed made the first window divide by a zero counter delta, which surfaced as -Infinity% user and NaN in the charts. Series now start empty and derived values are null until a real window exists. - Windows CPU brand comes from the Name field of the same Win32_Processor record that already carries LoadPercentage and the core counts, instead of a second plain-text cpuBrand command. One record means the brand and the count it describes cannot disagree. The three tests encoding those two contracts were rewritten rather than the code changed.
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
Testing
test/proc_test.dartSummary by CodeRabbit
Summary
Changes