feat: add live Wire operator dashboard - #374
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a local authenticated Wire operator dashboard with live-session inventory, provenance metadata, topology visualization, linking and group actions, polling, filtering, and accessibility support. It also adds identity resolution, daemon ownership, lease metadata, topology APIs, security checks, and extensive unit, browser, and end-to-end tests. ChangesOperator dashboard and runtime inventory
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant Dashboard
participant WebServer
participant Inventory
participant SessionStore
Operator->>Dashboard: Open local dashboard
Dashboard->>WebServer: Authenticated topology request
WebServer->>Inventory: Start serialized scan
Inventory->>SessionStore: Read live leases, peers, and groups
SessionStore-->>Inventory: Session and relationship data
Inventory-->>WebServer: Sanitized topology report
WebServer-->>Dashboard: JSON snapshot
Dashboard-->>Operator: Render map and session list
Operator->>Dashboard: Confirm link or group action
Dashboard->>WebServer: Authenticated mutation request
WebServer->>Inventory: Validate and execute mutation
Inventory-->>WebServer: Mutation result
WebServer-->>Dashboard: Result and changed sessions
Dashboard->>WebServer: Refresh topology
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying wireup-landing with
|
| Latest commit: |
6d93398
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://097f5ac8.wireup-landing.pages.dev |
| Branch Preview URL: | https://feat-operator-dashboard.wireup-landing.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (11)
src/operator.rs (1)
328-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the group storage accessor instead of a hardcoded path.
This block hardcodes
config/wire/groups/{group_id}.jsonand re-implements the read and decode logic.crate::group::list_groups_atalready resolves the same storage location, andsrc/operator_topology.rsuses it. If the group storage layout changes, this verification loop silently reports success or a false partial write.Call the existing accessor and check for the group id in the returned list.
♻️ Proposed refactor
- let path = member - .home_dir - .join("config/wire/groups") - .join(format!("{group_id}.json")); - let valid = std::fs::read(&path) - .ok() - .and_then(|body| serde_json::from_slice::<crate::group::Group>(&body).ok()) - .is_some_and(|group| group.id == group_id); + let valid = crate::group::list_groups_at(&member.home_dir) + .map(|groups| groups.iter().any(|group| group.id == group_id)) + .unwrap_or(false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/operator.rs` around lines 328 - 348, In the verification loop over ids in the group operation, replace the hardcoded path construction and manual file read/JSON decoding with crate::group::list_groups_at using the member’s home directory. Validate that the returned groups contain group_id, and preserve the existing OperatorError::Partial response for members where the group is absent.src/session_metadata.rs (1)
156-167: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
trim_end_matches(".git")strips the suffix repeatedly.
trim_end_matchesremoves every trailing occurrence of the pattern. A remote ending inrepo.git.gitresolves torepo. Usestrip_suffixso only one suffix is removed.♻️ Proposed refactor
- .map(|value| value.trim_end_matches(".git")) + .map(|value| value.strip_suffix(".git").unwrap_or(value))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session_metadata.rs` around lines 156 - 167, Update repository_name to remove the remote’s .git suffix with a single strip_suffix operation instead of trim_end_matches, preserving all other parsing and fallback behavior.docs/superpowers/specs/2026-08-10-operator-topology-map-design.md (1)
169-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the synthesized
introducedgroup members.
build_topologyinsrc/operator_topology.rsadds a member entry withtier: "introduced"andlive: truefor every live session that holds a copy of the winning group roster but is absent from that roster. The spec describes only rosters read from disk. Add one sentence that states this synthesis rule so operators can interpret the member list.🤖 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 `@docs/superpowers/specs/2026-08-10-operator-topology-map-design.md` around lines 169 - 178, Update the Group construction section describing the response fields to document that build_topology synthesizes a member with tier "introduced" and live true for each live session holding the winning group roster but absent from that roster.assets/operator-dashboard.css (1)
289-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the deprecated
clipproperty in.sr-only.Stylelint reports
clipas deprecated. Useclip-pathand keep the same visually hidden behavior.♻️ Proposed change
-.sr-only { position: absolute; width: 1px; height: 1px; padding: 0; overflow: hidden; clip: rect(0, 0, 0, 0); white-space: nowrap; border: 0; } +.sr-only { position: absolute; width: 1px; height: 1px; padding: 0; overflow: hidden; clip-path: inset(50%); white-space: nowrap; border: 0; }🤖 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 `@assets/operator-dashboard.css` at line 289, Update the `.sr-only` rule by replacing the deprecated `clip` declaration with an equivalent `clip-path` declaration that preserves its visually hidden behavior; leave the other accessibility-related properties unchanged.Source: Linters/SAST tools
src/operator_topology.rs (1)
92-101: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOne unreadable session home fails the whole topology snapshot.
crate::group::list_groups_at(home)?propagates any per-home error out ofcollect_topology. The dashboard then shows a generic failure and keeps stale data, even when only one of many homes is unreadable. The design records this class of problem as an anomaly instead. Consider degrading per home: keep the session, use an empty group list, and push aTopologyAnomalyfor the failed read.♻️ Proposed degradation
- sources.push(TopologySource { - peers: crate::dash::read_peers(home, Some(&session.did), Some(&session.handle)), - groups: crate::group::list_groups_at(home)?, - session, - }); + let groups = crate::group::list_groups_at(home).unwrap_or_default(); + sources.push(TopologySource { + peers: crate::dash::read_peers(home, Some(&session.did), Some(&session.handle)), + groups, + session, + });An anomaly entry needs a small extension to
build_topologyso the operator sees the degraded read.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/operator_topology.rs` around lines 92 - 101, Update the per-session handling in collect_topology so failures from crate::group::list_groups_at(home) do not abort the entire snapshot: retain the session with an empty group list and record a TopologyAnomaly for the failed home read. Extend build_topology as needed to include and expose this anomaly while preserving normal group results for readable homes.assets/operator-dashboard.js (2)
69-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe escaped SVG namespace hides the string from the asset security test.
"http\u003a//www.w3.org/2000/svg"produceshttp://www.w3.org/2000/svgat runtime. The served-asset test insrc/operator_web.rsasserts that/dashboard.jscontains nohttp://. The escape makes the script pass that assertion without changing behavior, so the assertion no longer proves the absence of remote URLs in this file. Prefer an explicit constant plus a narrowed test that forbids only network-capable URLs, and add a comment that states why the namespace literal is present.🤖 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 `@assets/operator-dashboard.js` at line 69, Update the svgNamespace declaration to use an explicit, documented namespace constant explaining its required SVG purpose, and revise the served-asset assertion in operator_web.rs to reject only network-capable URLs rather than any http:// substring. Keep the namespace value and runtime SVG behavior unchanged while ensuring the test still detects remote URLs in dashboard.js.
762-763: 🚀 Performance & Scalability | 🔵 TrivialConsider adapting the poll interval to the observed scan duration.
/api/topologyperforms a full inventory scan of every session home. The plan records a target below two seconds on a 3,000-home machine. A fixed 2,000 ms interval can therefore keep one scan in flight almost continuously. Thestate.busyguard only suppresses polls during mutations, andscan()coalescing prevents overlap but not back-to-back scans. Consider measuring the last scan duration and scheduling the next poll after a proportional delay, or increasing the interval when the tab is hidden.🤖 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 `@assets/operator-dashboard.js` around lines 762 - 763, Update the polling logic around scan() and the setInterval callback to avoid continuously back-to-back topology scans: measure each scan’s duration and schedule the next poll using a delay proportional to that duration, or increase the delay while the tab is hidden. Preserve the state.busy guard and scan coalescing behavior while replacing the fixed 2,000 ms cadence.tests/operator_dashboard_topology.test.mjs (1)
296-311: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPan tests cannot prove that a pointer press on a node does not start a drag.
ElementStubimplements noclosestmethod. Inassets/operator-dashboard.jsthepointerdownhandler usesevent.target.closest?.(".topology-node"), so the guard is always skipped in these tests. Add aclosestimplementation toElementStuband one assertion that apointerdownon a session node leaves the transform unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/operator_dashboard_topology.test.mjs` around lines 296 - 311, Extend ElementStub with a closest implementation that can identify elements matching ".topology-node", then add a test assertion covering pointerdown on a session node and verifying the map transform remains unchanged. Keep the existing zoom-and-pan assertions intact and ensure the node-target path exercises the guard in the pointerdown handler.src/operator_web.rs (2)
186-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the discarded error before returning 500.
The
_arm swallows two distinct failures: aJoinErrorfrom a panic inside the blocking task, and the innerErrfromcollect_live_sessions. Neither is logged. The operator sees onlysession inventory failedand has no way to diagnose the cause.get_topologyat Lines 201-208 andrun_mutationat Lines 218-226 discard error detail the same way.♻️ Proposed change
let _scan = state.scan_lock.lock().await; match tokio::task::spawn_blocking(crate::operator::collect_live_sessions).await { Ok(Ok(report)) => Json(report).into_response(), - _ => error_response( - StatusCode::INTERNAL_SERVER_ERROR, - "session inventory failed", - Vec::new(), - ), + other => { + match other { + Ok(Err(error)) => eprintln!("wire dash: session inventory failed: {error:#}"), + Err(error) => eprintln!("wire dash: session inventory task failed: {error}"), + Ok(Ok(_)) => unreachable!(), + } + error_response( + StatusCode::INTERNAL_SERVER_ERROR, + "session inventory failed", + Vec::new(), + ) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/operator_web.rs` around lines 186 - 193, Update the error handling around collect_live_sessions, get_topology, and run_mutation to match their distinct JoinError and inner operation error cases, log each discarded error with useful context, then preserve the existing 500 error_response behavior.
185-186: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider bounding the serialized scan.
scan_lockis held for the whole duration ofspawn_blocking.collect_live_sessionsandcollect_topologywalk the session registry and probe processes, and they have no timeout. If one scan stalls, every later request on/api/sessions,/api/topology,/api/links, and/api/groupswaits on the same mutex, and the polling dashboard queues requests without bound.Wrapping the awaited task in
tokio::time::timeoutand returning 503 on expiry keeps the dashboard responsive and releases the lock.Also applies to: 200-201
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/operator_web.rs` around lines 185 - 186, Bound the scan operations in the handlers using scan_lock, including the collect_live_sessions and collect_topology paths, by wrapping each awaited spawn_blocking call in tokio::time::timeout. Return HTTP 503 when the timeout expires, while preserving existing success and task-error handling and ensuring the lock is released after the bounded await.tests/cli.rs (1)
64-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd flag names to the clap error assertions.
clap4.6.1 emits both expected substrings. Include--weband--jsonin the conflict assertion, and--no-openand--webin the requirement assertion. This makes failures easier to diagnose after clap 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 `@tests/cli.rs` around lines 64 - 79, Update the assertions in dash_web_rejects_terminal_json_mode and dash_no_open_requires_web_mode to verify the relevant flag names in stderr: include both --web and --json for the conflict case, and --no-open and --web for the requirement case, while preserving the existing error-substring checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/mod.rs`:
- Around line 134-139: Extend the `web` argument’s `conflicts_with_all`
configuration to include the remaining terminal-only flags `all`, `probe`, and
`retired`, alongside `watch`, `json`, and `retire_idle`. Keep `no_open`’s
existing `requires = "web"` behavior unchanged.
In `@src/operator_web.rs`:
- Around line 378-384: Update the authorized GET /api/sessions assertion in the
surrounding test to avoid depending on live session collection or inherited
WIRE_HOME state: either assert only that the response is not rejected by the
token and Host/Origin contract, or configure WIRE_HOME to an isolated temporary
directory for the test before making the request.
In `@src/operator.rs`:
- Around line 90-94: Update validate_group_request to reject trimmed group names
whose first character is '-'; retain the existing empty-name validation and
return the same validation error style. Ensure create_local_group cannot pass
option-like names such as --json or -h to the wire CLI.
- Around line 129-156: Update run_wire_command_at to avoid blocking on
Command::output(): spawn the child process, wait with a bounded deadline, and
kill it if the deadline expires. Map timeout and termination failures to
OperatorError::Internal with an explicit error, while preserving existing
successful-output handling and stderr-based failure reporting.
In `@src/platform.rs`:
- Around line 132-142: Update the PID validation in the surrounding
process-existence check to return false when the converted pid equals zero,
before invoking the unsafe kill function. Preserve the existing conversion
failure handling and kill-based checks for nonzero PIDs.
In `@src/session_lifecycle.rs`:
- Around line 113-136: Make process_snapshot lazy in the heartbeat logic: remove
the unconditional snapshot assignment and invoke
crate::session_metadata::process_snapshot only inside the harness and project
branches when their existing metadata is missing or has Unknown confidence.
Reuse the resulting snapshot for both branches when needed, while leaving the
machine branch independent and preserving all existing metadata assignments.
In `@src/session_metadata.rs`:
- Around line 454-500: Update the process observation loop around observations,
current, and cwd so every requested root PID receives a cwd when its observation
was first created as an ancestor of another root. When a root PID is already
present, backfill its cwd by reading that PID’s proc cwd path if it is still
missing, rather than breaking immediately; preserve ancestor deduplication and
the existing depth-based cwd behavior for newly recorded observations.
- Around line 145-154: Update sanitize_remote to detect errors from both
url.set_username and url.set_password instead of discarding their results; if
either setter fails, return a redacted remote value without credentials, while
preserving the existing URL output for successful sanitization and the original
string for parse failures.
In `@tests/e2e_operator_dashboard.rs`:
- Around line 131-152: Bound the dashboard startup wait in the test setup around
Dashboard(child): create the Dashboard wrapper before reading output so its Drop
implementation can terminate the process on failure, move the blocking
BufReader::read_line and URL parsing into a helper thread, and receive the
result with recv_timeout. On timeout, fail with a diagnostic indicating
dashboard startup did not emit its URL, while preserving the existing URL
validation and token/origin setup on success.
---
Nitpick comments:
In `@assets/operator-dashboard.css`:
- Line 289: Update the `.sr-only` rule by replacing the deprecated `clip`
declaration with an equivalent `clip-path` declaration that preserves its
visually hidden behavior; leave the other accessibility-related properties
unchanged.
In `@assets/operator-dashboard.js`:
- Line 69: Update the svgNamespace declaration to use an explicit, documented
namespace constant explaining its required SVG purpose, and revise the
served-asset assertion in operator_web.rs to reject only network-capable URLs
rather than any http:// substring. Keep the namespace value and runtime SVG
behavior unchanged while ensuring the test still detects remote URLs in
dashboard.js.
- Around line 762-763: Update the polling logic around scan() and the
setInterval callback to avoid continuously back-to-back topology scans: measure
each scan’s duration and schedule the next poll using a delay proportional to
that duration, or increase the delay while the tab is hidden. Preserve the
state.busy guard and scan coalescing behavior while replacing the fixed 2,000 ms
cadence.
In `@docs/superpowers/specs/2026-08-10-operator-topology-map-design.md`:
- Around line 169-178: Update the Group construction section describing the
response fields to document that build_topology synthesizes a member with tier
"introduced" and live true for each live session holding the winning group
roster but absent from that roster.
In `@src/operator_topology.rs`:
- Around line 92-101: Update the per-session handling in collect_topology so
failures from crate::group::list_groups_at(home) do not abort the entire
snapshot: retain the session with an empty group list and record a
TopologyAnomaly for the failed home read. Extend build_topology as needed to
include and expose this anomaly while preserving normal group results for
readable homes.
In `@src/operator_web.rs`:
- Around line 186-193: Update the error handling around collect_live_sessions,
get_topology, and run_mutation to match their distinct JoinError and inner
operation error cases, log each discarded error with useful context, then
preserve the existing 500 error_response behavior.
- Around line 185-186: Bound the scan operations in the handlers using
scan_lock, including the collect_live_sessions and collect_topology paths, by
wrapping each awaited spawn_blocking call in tokio::time::timeout. Return HTTP
503 when the timeout expires, while preserving existing success and task-error
handling and ensuring the lock is released after the bounded await.
In `@src/operator.rs`:
- Around line 328-348: In the verification loop over ids in the group operation,
replace the hardcoded path construction and manual file read/JSON decoding with
crate::group::list_groups_at using the member’s home directory. Validate that
the returned groups contain group_id, and preserve the existing
OperatorError::Partial response for members where the group is absent.
In `@src/session_metadata.rs`:
- Around line 156-167: Update repository_name to remove the remote’s .git suffix
with a single strip_suffix operation instead of trim_end_matches, preserving all
other parsing and fallback behavior.
In `@tests/cli.rs`:
- Around line 64-79: Update the assertions in
dash_web_rejects_terminal_json_mode and dash_no_open_requires_web_mode to verify
the relevant flag names in stderr: include both --web and --json for the
conflict case, and --no-open and --web for the requirement case, while
preserving the existing error-substring checks.
In `@tests/operator_dashboard_topology.test.mjs`:
- Around line 296-311: Extend ElementStub with a closest implementation that can
identify elements matching ".topology-node", then add a test assertion covering
pointerdown on a session node and verifying the map transform remains unchanged.
Keep the existing zoom-and-pan assertions intact and ensure the node-target path
exercises the guard in the pointerdown handler.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7bf672fc-c52f-43d3-a4de-9001f969f407
📒 Files selected for processing (31)
SESSION_LOG_2026_08_10.mdassets/operator-dashboard.cssassets/operator-dashboard.htmlassets/operator-dashboard.jsassets/operator-topology.jsdocs/superpowers/plans/2026-08-10-fleet-session-provenance.mddocs/superpowers/plans/2026-08-10-operator-topology-map.mddocs/superpowers/specs/2026-08-10-fleet-session-provenance-design.mddocs/superpowers/specs/2026-08-10-operator-dashboard-design.mddocs/superpowers/specs/2026-08-10-operator-topology-map-design.mdsrc/cli/dash.rssrc/cli/mod.rssrc/cli/pairing.rssrc/cli/session.rssrc/daemon_supervisor.rssrc/dash.rssrc/ensure_up.rssrc/group.rssrc/lib.rssrc/operator.rssrc/operator_topology.rssrc/operator_web.rssrc/platform.rssrc/session.rssrc/session_lifecycle.rssrc/session_metadata.rstests/cli.rstests/e2e_operator_dashboard.rstests/operator_dashboard_polling.test.mjstests/operator_dashboard_topology.test.mjstests/operator_topology_model.test.mjs
| /// Open the local operator dashboard in a browser. | ||
| #[arg(long, conflicts_with_all = ["watch", "json", "retire_idle"])] | ||
| web: bool, | ||
| /// Start the web dashboard without opening a browser. | ||
| #[arg(long, requires = "web")] | ||
| no_open: bool, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Extend the --web conflict list to the remaining terminal-only flags.
--web conflicts with watch, json, and retire_idle. It does not conflict with --all, --probe, or --retired. Those three flags control the terminal table rendering only. cmd_dash returns into operator_web::serve before it reads them, so wire dash --web --all parses successfully and then ignores --all without any message.
🛠️ Proposed change
/// Open the local operator dashboard in a browser.
- #[arg(long, conflicts_with_all = ["watch", "json", "retire_idle"])]
+ #[arg(long, conflicts_with_all = ["watch", "json", "retire_idle", "all", "probe", "retired"])]
web: bool,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Open the local operator dashboard in a browser. | |
| #[arg(long, conflicts_with_all = ["watch", "json", "retire_idle"])] | |
| web: bool, | |
| /// Start the web dashboard without opening a browser. | |
| #[arg(long, requires = "web")] | |
| no_open: bool, | |
| /// Open the local operator dashboard in a browser. | |
| #[arg(long, conflicts_with_all = ["watch", "json", "retire_idle", "all", "probe", "retired"])] | |
| web: bool, | |
| /// Start the web dashboard without opening a browser. | |
| #[arg(long, requires = "web")] | |
| no_open: bool, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/mod.rs` around lines 134 - 139, Extend the `web` argument’s
`conflicts_with_all` configuration to include the remaining terminal-only flags
`all`, `probe`, and `retired`, alongside `watch`, `json`, and `retire_idle`.
Keep `no_open`’s existing `requires = "web"` behavior unchanged.
| let sessions = client | ||
| .get(format!("http://{address}/api/sessions")) | ||
| .header("X-Wire-Token", "test-token") | ||
| .send() | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(sessions.status(), StatusCode::OK); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This assertion depends on ambient machine state.
The authorized GET /api/sessions call runs the real collect_live_sessions against whatever WIRE_HOME the test process inherits. It scans the session registry and probes processes. If collection fails on a clean CI machine, the handler returns 500 and the test fails for a reason unrelated to authorization.
The surrounding test verifies the token and Host/Origin contract. Assert only that the authorized request is not rejected by that contract, or pin WIRE_HOME to a temporary directory for this test.
💚 Proposed change
assert_eq!(sessions.status(), StatusCode::OK);
+ // Collection may legitimately fail on a machine with no sessions;
+ // this test only asserts that the token gate accepts the request.
+ assert_ne!(sessions.status(), StatusCode::FORBIDDEN);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/operator_web.rs` around lines 378 - 384, Update the authorized GET
/api/sessions assertion in the surrounding test to avoid depending on live
session collection or inherited WIRE_HOME state: either assert only that the
response is not rejected by the token and Host/Origin contract, or configure
WIRE_HOME to an isolated temporary directory for the test before making the
request.
| if request.name.trim().is_empty() { | ||
| return Err(OperatorError::Validation( | ||
| "group name cannot be empty".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Reject group names that begin with -.
validate_group_request only rejects an empty name. create_local_group then passes request.name.trim() as a positional argument to the wire CLI. A name such as --json or -h is parsed as a flag, not as a group name. No shell is involved, so this is argument injection into the child CLI, not command injection. The result is a confusing failure or an unintended CLI behavior.
Add a validation rule for the leading - character.
🛡️ Proposed validation
if request.name.trim().is_empty() {
return Err(OperatorError::Validation(
"group name cannot be empty".to_string(),
));
}
+ if request.name.trim().starts_with('-') {
+ return Err(OperatorError::Validation(
+ "group name cannot start with `-`".to_string(),
+ ));
+ }Also applies to: 270-278
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/operator.rs` around lines 90 - 94, Update validate_group_request to
reject trimmed group names whose first character is '-'; retain the existing
empty-name validation and return the same validation error style. Ensure
create_local_group cannot pass option-like names such as --json or -h to the
wire CLI.
| fn run_wire_command_at(home: &Path, args: &[String]) -> Result<Output, OperatorError> { | ||
| let binary = crate::platform::current_exe_resolved() | ||
| .map_err(|error| OperatorError::Internal(error.into()))?; | ||
| let output = Command::new(binary) | ||
| .args(args) | ||
| .env("WIRE_HOME", home) | ||
| .env("WIRE_HOME_FORCE", "1") | ||
| .env("WIRE_QUIET_AUTOSESSION", "1") | ||
| .env_remove("WIRE_SESSION_ID") | ||
| .env_remove("CLAUDE_CODE_SESSION_ID") | ||
| .env_remove("CODEX_SESSION_ID") | ||
| .env_remove("CODEX_THREAD_ID") | ||
| .env_remove("AGENT") | ||
| .env_remove("AGENT_SESSION_ID") | ||
| .env_remove("COPILOT_AGENT_SESSION_ID") | ||
| .env_remove("VSCODE_GIT_REPOSITORY_ROOT") | ||
| .env_remove("WIRE_LOCAL_PAIR_ONE_WAY") | ||
| .output() | ||
| .map_err(|error| OperatorError::Internal(error.into()))?; | ||
| if !output.status.success() { | ||
| return Err(OperatorError::Internal(anyhow::anyhow!( | ||
| "wire command failed with {}: {}", | ||
| output.status, | ||
| capped_wire_output(&output.stderr).trim() | ||
| ))); | ||
| } | ||
| Ok(output) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout to the child wire process.
Command::output() blocks until the child exits. No timeout exists. If a wire add, group create, group invite, or group join invocation hangs (for example, while it waits on a relay or a lock), the calling dashboard request thread blocks for an unbounded time. Because the dashboard serializes mutations, one hung child can block every later mutation for the life of the process.
Spawn the child, wait with a deadline, and kill it when the deadline passes. Map the timeout to OperatorError::Internal so the operator sees an explicit failure instead of a stalled request.
⏱️ Sketch of a bounded wait
- let output = Command::new(binary)
+ let mut child = Command::new(binary)
.args(args)
.env("WIRE_HOME", home)
@@
- .output()
- .map_err(|error| OperatorError::Internal(error.into()))?;
+ .stdout(std::process::Stdio::piped())
+ .stderr(std::process::Stdio::piped())
+ .spawn()
+ .map_err(|error| OperatorError::Internal(error.into()))?;
+ // Poll `child.try_wait()` until WIRE_OPERATOR_COMMAND_TIMEOUT elapses,
+ // then `child.kill()` and return OperatorError::Internal on timeout.
+ let output = wait_with_timeout(child, WIRE_OPERATOR_COMMAND_TIMEOUT)?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/operator.rs` around lines 129 - 156, Update run_wire_command_at to avoid
blocking on Command::output(): spawn the child process, wait with a bounded
deadline, and kill it if the deadline expires. Map timeout and termination
failures to OperatorError::Internal with an explicit error, while preserving
existing successful-output handling and stderr-based failure reporting.
| let Ok(pid) = i32::try_from(pid) else { | ||
| return false; | ||
| }; | ||
| unsafe extern "C" { | ||
| fn kill(pid: i32, signal: i32) -> i32; | ||
| } | ||
| // SAFETY: signal 0 never delivers a signal; it only asks the kernel | ||
| // whether the process exists and is visible to this user. | ||
| let result = unsafe { kill(pid, 0) }; | ||
| result == 0 | ||
| || std::io::Error::last_os_error().kind() == std::io::ErrorKind::PermissionDenied |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/platform.rs --match process_alive --view expanded
rg -n -C 3 'process_alive\(0\)|kill\(pid, 0\)|pub fn process_alive' srcRepository: SlanchaAI/wire
Length of output: 1143
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/platform.rs structure ---'
ast-grep outline src/platform.rs --match process_alive --view expanded
printf '%s\n' '--- process_alive implementation and nearby callers ---'
sed -n '110,155p' src/platform.rs
rg -n -C 4 '\bprocess_alive\b|pidfile|pid_file|parse::<u32>|parse::<i32>' src
printf '%s\n' '--- PID-zero handling in the repository ---'
rg -n -C 3 '\bpid\s*==\s*0\b|PID 0|pid.*zero|zero.*pid' srcRepository: SlanchaAI/wire
Length of output: 50371
🌐 Web query:
POSIX kill(2) pid 0 signal 0 semantics macOS BSD
💡 Result:
On macOS and other POSIX-compliant systems, the kill(2) system call and the behavior of pid 0 and signal 0 have specific, standardized semantics [1][2]. Signal 0 (the null signal) When the signal number (sig) passed to kill is 0, no signal is actually sent to the target [1][3]. Instead, the system performs error checking, primarily to verify the validity of the process ID (pid) or process group ID and to check for the caller's permission to signal the target [1][4]. This is a standard, portable mechanism used to determine if a process exists and if the caller has sufficient privileges to interact with it [5][6][7]. PID 0 The behavior of pid 0 in the kill(2) system call is distinct from signal 0 and is defined as follows: - If pid is 0, the signal is sent to all processes in the process group of the caller [1][2]. - This functionality is effectively a variant of killpg(2) [2][8]. Combining PID 0 and Signal 0 When you call kill(0, 0) on macOS (or any POSIX-compliant system), you are effectively asking the system to perform a permission and validity check on the entire process group to which the caller belongs [1][8]. If the caller has permission to send signals to all processes in its group, the call will succeed (returning 0) [1][2]. Important Considerations - Shell Confusion: Users often confuse the system call kill(2) with the command-line utility kill(1) [9]. If a user runs the shell built-in command kill -9 0, it interprets pid 0 as the current process group and sends SIGKILL to all processes in that group (including the shell itself), leading to the termination of the shell and its children [9]. - System Call vs. Command: To test for a specific process's existence, one must use a non-zero PID (e.g., kill(target_pid, 0)) [5][10]. Using PID 0 targets the process group, not the caller itself [2][8].
Citations:
- 1: https://pubs.opengroup.org/onlinepubs/009604499/functions/kill.html
- 2: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/kill.2.html
- 3: https://man7.org/linux/man-pages/man2/kill.2.html
- 4: https://man.archlinux.org/man/kill.2.en.txt
- 5: https://unix.stackexchange.com/questions/169898/what-does-kill-0-do
- 6: https://unix.stackexchange.com/questions/722731/what-is-the-meaning-of-kill-0-pid
- 7: https://lists.freebsd.org/pipermail/freebsd-hackers/2017-March/050655.html
- 8: https://github.com/apple/darwin-xnu/blob/master/bsd/kern/kern_sig.c
- 9: https://superuser.com/questions/929702/why-does-kill-9-0-on-a-mac-simply-kill-the-bash-shell
- 10: https://man.openbsd.org/kill.1
Reject PID 0 before calling kill.
On macOS and BSD, kill(0, 0) checks the caller's process group. Return false when pid == 0; otherwise, a PID-zero pidfile can report a live process.
Proposed fix
#[cfg(all(unix, not(target_os = "linux")))]
{
+ if pid == 0 {
+ return false;
+ }
let Ok(pid) = i32::try_from(pid) else {
return false;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let Ok(pid) = i32::try_from(pid) else { | |
| return false; | |
| }; | |
| unsafe extern "C" { | |
| fn kill(pid: i32, signal: i32) -> i32; | |
| } | |
| // SAFETY: signal 0 never delivers a signal; it only asks the kernel | |
| // whether the process exists and is visible to this user. | |
| let result = unsafe { kill(pid, 0) }; | |
| result == 0 | |
| || std::io::Error::last_os_error().kind() == std::io::ErrorKind::PermissionDenied | |
| if pid == 0 { | |
| return false; | |
| } | |
| let Ok(pid) = i32::try_from(pid) else { | |
| return false; | |
| }; | |
| unsafe extern "C" { | |
| fn kill(pid: i32, signal: i32) -> i32; | |
| } | |
| // SAFETY: signal 0 never delivers a signal; it only asks the kernel | |
| // whether the process exists and is visible to this user. | |
| let result = unsafe { kill(pid, 0) }; | |
| result == 0 | |
| || std::io::Error::last_os_error().kind() == std::io::ErrorKind::PermissionDenied |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform.rs` around lines 132 - 142, Update the PID validation in the
surrounding process-existence check to return false when the converted pid
equals zero, before invoking the unsafe kill function. Preserve the existing
conversion failure handling and kill-based checks for nonzero PIDs.
| let snapshot = crate::session_metadata::process_snapshot(&[record.pid]); | ||
| if record.machine.is_none() { | ||
| record.machine = Some(crate::session_metadata::machine_descriptor( | ||
| &record.wire_version, | ||
| )); | ||
| } | ||
| if record.harness.as_ref().is_none_or(|value| { | ||
| value.confidence == crate::session_metadata::MetadataConfidence::Unknown | ||
| }) { | ||
| record.harness = Some(crate::session_metadata::harness_from_snapshot( | ||
| &snapshot, | ||
| record.pid, | ||
| &record.session_source, | ||
| )); | ||
| } | ||
| if record.project.as_ref().is_none_or(|value| { | ||
| value.confidence == crate::session_metadata::MetadataConfidence::Unknown | ||
| }) { | ||
| record.project = Some(crate::session_metadata::project_from_snapshot( | ||
| &snapshot, | ||
| record.pid, | ||
| record.cwd.as_deref().map(Path::new), | ||
| )); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Compute the process snapshot lazily in the heartbeat path.
Line 113 calls process_snapshot on every heartbeat. The three guards below then skip the snapshot when machine, harness, and project are already known. In the common steady state the snapshot result is discarded.
The cost is not free. PROCESS_SNAPSHOT_CACHE is keyed by the sorted PID set. collect_live_from in src/operator.rs calls process_snapshot with all candidate PIDs, and this call passes a single PID. In a process that performs both, each call evicts the other key and triggers a fresh probe. On macOS a probe runs ps -axo and lsof, each with a 5-second timeout, on the heartbeat path.
Move the snapshot behind the branches that need it.
♻️ Proposed fix
- let snapshot = crate::session_metadata::process_snapshot(&[record.pid]);
+ let needs_harness = record.harness.as_ref().is_none_or(|value| {
+ value.confidence == crate::session_metadata::MetadataConfidence::Unknown
+ });
+ let needs_project = record.project.as_ref().is_none_or(|value| {
+ value.confidence == crate::session_metadata::MetadataConfidence::Unknown
+ });
+ let snapshot = (needs_harness || needs_project)
+ .then(|| crate::session_metadata::process_snapshot(&[record.pid]))
+ .unwrap_or_default();
if record.machine.is_none() {
record.machine = Some(crate::session_metadata::machine_descriptor(
&record.wire_version,
));
}
- if record.harness.as_ref().is_none_or(|value| {
- value.confidence == crate::session_metadata::MetadataConfidence::Unknown
- }) {
+ if needs_harness {
record.harness = Some(crate::session_metadata::harness_from_snapshot(
&snapshot,
record.pid,
&record.session_source,
));
}
- if record.project.as_ref().is_none_or(|value| {
- value.confidence == crate::session_metadata::MetadataConfidence::Unknown
- }) {
+ if needs_project {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let snapshot = crate::session_metadata::process_snapshot(&[record.pid]); | |
| if record.machine.is_none() { | |
| record.machine = Some(crate::session_metadata::machine_descriptor( | |
| &record.wire_version, | |
| )); | |
| } | |
| if record.harness.as_ref().is_none_or(|value| { | |
| value.confidence == crate::session_metadata::MetadataConfidence::Unknown | |
| }) { | |
| record.harness = Some(crate::session_metadata::harness_from_snapshot( | |
| &snapshot, | |
| record.pid, | |
| &record.session_source, | |
| )); | |
| } | |
| if record.project.as_ref().is_none_or(|value| { | |
| value.confidence == crate::session_metadata::MetadataConfidence::Unknown | |
| }) { | |
| record.project = Some(crate::session_metadata::project_from_snapshot( | |
| &snapshot, | |
| record.pid, | |
| record.cwd.as_deref().map(Path::new), | |
| )); | |
| } | |
| let needs_harness = record.harness.as_ref().is_none_or(|value| { | |
| value.confidence == crate::session_metadata::MetadataConfidence::Unknown | |
| }); | |
| let needs_project = record.project.as_ref().is_none_or(|value| { | |
| value.confidence == crate::session_metadata::MetadataConfidence::Unknown | |
| }); | |
| let snapshot = (needs_harness || needs_project) | |
| .then(|| crate::session_metadata::process_snapshot(&[record.pid])) | |
| .unwrap_or_default(); | |
| if record.machine.is_none() { | |
| record.machine = Some(crate::session_metadata::machine_descriptor( | |
| &record.wire_version, | |
| )); | |
| } | |
| if needs_harness { | |
| record.harness = Some(crate::session_metadata::harness_from_snapshot( | |
| &snapshot, | |
| record.pid, | |
| &record.session_source, | |
| )); | |
| } | |
| if needs_project { | |
| record.project = Some(crate::session_metadata::project_from_snapshot( | |
| &snapshot, | |
| record.pid, | |
| record.cwd.as_deref().map(Path::new), | |
| )); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/session_lifecycle.rs` around lines 113 - 136, Make process_snapshot lazy
in the heartbeat logic: remove the unconditional snapshot assignment and invoke
crate::session_metadata::process_snapshot only inside the harness and project
branches when their existing metadata is missing or has Unknown confidence.
Reuse the resulting snapshot for both branches when needed, while leaving the
machine branch independent and preserving all existing metadata assignments.
| fn sanitize_remote(remote: &str) -> String { | ||
| let Ok(mut url) = reqwest::Url::parse(remote) else { | ||
| return remote.to_string(); | ||
| }; | ||
| if !url.username().is_empty() || url.password().is_some() { | ||
| let _ = url.set_username(""); | ||
| let _ = url.set_password(None); | ||
| } | ||
| url.to_string() | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Credentials can survive sanitization when the URL setters fail.
Url::set_username and Url::set_password return Err for URLs that cannot be a base. The code discards both results with let _. In that case url.to_string() still contains the credential, and the remote string is persisted in the lease and served through the dashboard API.
Return a redacted value when a setter fails.
🛡️ Proposed fix
fn sanitize_remote(remote: &str) -> String {
let Ok(mut url) = reqwest::Url::parse(remote) else {
return remote.to_string();
};
if !url.username().is_empty() || url.password().is_some() {
- let _ = url.set_username("");
- let _ = url.set_password(None);
+ if url.set_username("").is_err() || url.set_password(None).is_err() {
+ return "redacted-remote".to_string();
+ }
}
url.to_string()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn sanitize_remote(remote: &str) -> String { | |
| let Ok(mut url) = reqwest::Url::parse(remote) else { | |
| return remote.to_string(); | |
| }; | |
| if !url.username().is_empty() || url.password().is_some() { | |
| let _ = url.set_username(""); | |
| let _ = url.set_password(None); | |
| } | |
| url.to_string() | |
| } | |
| fn sanitize_remote(remote: &str) -> String { | |
| let Ok(mut url) = reqwest::Url::parse(remote) else { | |
| return remote.to_string(); | |
| }; | |
| if !url.username().is_empty() || url.password().is_some() { | |
| if url.set_username("").is_err() || url.set_password(None).is_err() { | |
| return "redacted-remote".to_string(); | |
| } | |
| } | |
| url.to_string() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/session_metadata.rs` around lines 145 - 154, Update sanitize_remote to
detect errors from both url.set_username and url.set_password instead of
discarding their results; if either setter fails, return a redacted remote value
without credentials, while preserving the existing URL output for successful
sanitization and the original string for parse failures.
| let mut observations = HashMap::new(); | ||
| for root_pid in pids { | ||
| let mut current = Some(*root_pid); | ||
| for depth in 0..MAX_ANCESTORS { | ||
| let Some(pid) = current else { break }; | ||
| if observations.contains_key(&pid) { | ||
| break; | ||
| } | ||
| let proc_dir = proc_root.join(pid.to_string()); | ||
| let Ok(status) = std::fs::read_to_string(proc_dir.join("status")) else { | ||
| break; | ||
| }; | ||
| let parent_pid = status | ||
| .lines() | ||
| .find_map(|line| line.strip_prefix("PPid:")) | ||
| .and_then(|value| value.trim().parse::<u32>().ok()) | ||
| .filter(|value| *value != 0); | ||
| let executable = std::fs::read_link(proc_dir.join("exe")) | ||
| .ok() | ||
| .and_then(|path| { | ||
| path.file_name() | ||
| .map(|value| value.to_string_lossy().into_owned()) | ||
| }) | ||
| .unwrap_or_else(|| "unknown".to_string()); | ||
| let arguments = std::fs::read(proc_dir.join("cmdline")) | ||
| .unwrap_or_default() | ||
| .split(|byte| *byte == 0) | ||
| .filter(|value| !value.is_empty()) | ||
| .map(|value| String::from_utf8_lossy(value).into_owned()) | ||
| .collect(); | ||
| let cwd = (depth == 0) | ||
| .then(|| std::fs::read_link(proc_dir.join("cwd")).ok()) | ||
| .flatten(); | ||
| observations.insert( | ||
| pid, | ||
| ProcessObservation { | ||
| pid, | ||
| parent_pid, | ||
| executable, | ||
| arguments, | ||
| cwd, | ||
| }, | ||
| ); | ||
| current = parent_pid; | ||
| } | ||
| } | ||
| Ok(ProcessSnapshot { observations }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ancestor deduplication can drop the working directory for a requested PID.
The loop records cwd only at depth == 0. It also breaks when observations already contains the PID. If a requested PID was already recorded as an ancestor of an earlier requested PID, the later iteration breaks immediately and the PID keeps cwd: None.
Example: pids = [10, 20] where PID 20 is the parent of PID 10. Root 10 records PID 10 with cwd, then records PID 20 at depth 1 without cwd. Root 20 then hits contains_key and breaks. The session that owns PID 20 reports an unknown project even though /proc/20/cwd is readable.
The macOS path does not have this defect because lsof fills cwd for every selected PID after the ancestry walk.
Backfill cwd for requested PIDs instead of skipping them.
🐛 Proposed fix
for root_pid in pids {
let mut current = Some(*root_pid);
for depth in 0..MAX_ANCESTORS {
let Some(pid) = current else { break };
- if observations.contains_key(&pid) {
- break;
+ if let Some(existing) = observations.get_mut(&pid) {
+ if depth == 0 && existing.cwd.is_none() {
+ existing.cwd = std::fs::read_link(proc_root.join(pid.to_string()).join("cwd")).ok();
+ }
+ break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut observations = HashMap::new(); | |
| for root_pid in pids { | |
| let mut current = Some(*root_pid); | |
| for depth in 0..MAX_ANCESTORS { | |
| let Some(pid) = current else { break }; | |
| if observations.contains_key(&pid) { | |
| break; | |
| } | |
| let proc_dir = proc_root.join(pid.to_string()); | |
| let Ok(status) = std::fs::read_to_string(proc_dir.join("status")) else { | |
| break; | |
| }; | |
| let parent_pid = status | |
| .lines() | |
| .find_map(|line| line.strip_prefix("PPid:")) | |
| .and_then(|value| value.trim().parse::<u32>().ok()) | |
| .filter(|value| *value != 0); | |
| let executable = std::fs::read_link(proc_dir.join("exe")) | |
| .ok() | |
| .and_then(|path| { | |
| path.file_name() | |
| .map(|value| value.to_string_lossy().into_owned()) | |
| }) | |
| .unwrap_or_else(|| "unknown".to_string()); | |
| let arguments = std::fs::read(proc_dir.join("cmdline")) | |
| .unwrap_or_default() | |
| .split(|byte| *byte == 0) | |
| .filter(|value| !value.is_empty()) | |
| .map(|value| String::from_utf8_lossy(value).into_owned()) | |
| .collect(); | |
| let cwd = (depth == 0) | |
| .then(|| std::fs::read_link(proc_dir.join("cwd")).ok()) | |
| .flatten(); | |
| observations.insert( | |
| pid, | |
| ProcessObservation { | |
| pid, | |
| parent_pid, | |
| executable, | |
| arguments, | |
| cwd, | |
| }, | |
| ); | |
| current = parent_pid; | |
| } | |
| } | |
| Ok(ProcessSnapshot { observations }) | |
| let mut observations = HashMap::new(); | |
| for root_pid in pids { | |
| let mut current = Some(*root_pid); | |
| for depth in 0..MAX_ANCESTORS { | |
| let Some(pid) = current else { break }; | |
| if let Some(existing) = observations.get_mut(&pid) { | |
| if depth == 0 && existing.cwd.is_none() { | |
| existing.cwd = | |
| std::fs::read_link(proc_root.join(pid.to_string()).join("cwd")).ok(); | |
| } | |
| break; | |
| } | |
| let proc_dir = proc_root.join(pid.to_string()); | |
| let Ok(status) = std::fs::read_to_string(proc_dir.join("status")) else { | |
| break; | |
| }; | |
| let parent_pid = status | |
| .lines() | |
| .find_map(|line| line.strip_prefix("PPid:")) | |
| .and_then(|value| value.trim().parse::<u32>().ok()) | |
| .filter(|value| *value != 0); | |
| let executable = std::fs::read_link(proc_dir.join("exe")) | |
| .ok() | |
| .and_then(|path| { | |
| path.file_name() | |
| .map(|value| value.to_string_lossy().into_owned()) | |
| }) | |
| .unwrap_or_else(|| "unknown".to_string()); | |
| let arguments = std::fs::read(proc_dir.join("cmdline")) | |
| .unwrap_or_default() | |
| .split(|byte| *byte == 0) | |
| .filter(|value| !value.is_empty()) | |
| .map(|value| String::from_utf8_lossy(value).into_owned()) | |
| .collect(); | |
| let cwd = (depth == 0) | |
| .then(|| std::fs::read_link(proc_dir.join("cwd")).ok()) | |
| .flatten(); | |
| observations.insert( | |
| pid, | |
| ProcessObservation { | |
| pid, | |
| parent_pid, | |
| executable, | |
| arguments, | |
| cwd, | |
| }, | |
| ); | |
| current = parent_pid; | |
| } | |
| } | |
| Ok(ProcessSnapshot { observations }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/session_metadata.rs` around lines 454 - 500, Update the process
observation loop around observations, current, and cwd so every requested root
PID receives a cwd when its observation was first created as an ancestor of
another root. When a root PID is already present, backfill its cwd by reading
that PID’s proc cwd path if it is still missing, rather than breaking
immediately; preserve ancestor deduplication and the existing depth-based cwd
behavior for newly recorded observations.
| let mut first_line = String::new(); | ||
| BufReader::new(child.stdout.take().unwrap()) | ||
| .read_line(&mut first_line) | ||
| .unwrap(); | ||
| let url = first_line | ||
| .split_once("dashboard: ") | ||
| .map(|(_, url)| url.trim().to_string()) | ||
| .expect("dashboard URL"); | ||
| let parsed_url = reqwest::Url::parse(&url).unwrap(); | ||
| assert_eq!(parsed_url.host_str(), Some("127.0.0.1")); | ||
| let token = parsed_url | ||
| .query_pairs() | ||
| .find(|(key, _)| key == "token") | ||
| .map(|(_, value)| value.into_owned()) | ||
| .unwrap(); | ||
| let origin = format!( | ||
| "{}://{}:{}", | ||
| parsed_url.scheme(), | ||
| parsed_url.host_str().unwrap(), | ||
| parsed_url.port().unwrap() | ||
| ); | ||
| let _dashboard = Dashboard(child); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add a bounded dashboard startup wait.
BufReader::read_line can block forever if the dashboard does not emit its startup URL. This can stall the complete test suite.
Wrap child in Dashboard before waiting. Read the URL on a helper thread. Use recv_timeout to fail with a startup diagnostic and ensure Drop kills the child on failure.
Proposed fix
+use std::sync::mpsc;
use std::time::Duration;
- let mut child = Command::new(wire_bin())
+ let child = Command::new(wire_bin())
.args(["dash", "--web", "--no-open"])
.env("WIRE_HOME", &root)
.env("WIRE_HOME_FORCE", "1")
@@
.spawn()
.unwrap();
- let mut first_line = String::new();
- BufReader::new(child.stdout.take().unwrap())
- .read_line(&mut first_line)
- .unwrap();
+ let mut dashboard = Dashboard(child);
+ let stdout = dashboard.0.stdout.take().unwrap();
+ let (sender, receiver) = mpsc::sync_channel(1);
+ std::thread::spawn(move || {
+ let mut first_line = String::new();
+ let result = BufReader::new(stdout)
+ .read_line(&mut first_line)
+ .map(|_| first_line);
+ let _ = sender.send(result);
+ });
+ let first_line = receiver
+ .recv_timeout(Duration::from_secs(10))
+ .expect("dashboard startup timed out")
+ .expect("read dashboard URL");
@@
- let _dashboard = Dashboard(child);
+ let _dashboard = dashboard;As per coding guidelines, “Before modifying any function, class, or method, run gitnexus_impact with the target symbol and upstream direction, then report the blast radius to the user.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut first_line = String::new(); | |
| BufReader::new(child.stdout.take().unwrap()) | |
| .read_line(&mut first_line) | |
| .unwrap(); | |
| let url = first_line | |
| .split_once("dashboard: ") | |
| .map(|(_, url)| url.trim().to_string()) | |
| .expect("dashboard URL"); | |
| let parsed_url = reqwest::Url::parse(&url).unwrap(); | |
| assert_eq!(parsed_url.host_str(), Some("127.0.0.1")); | |
| let token = parsed_url | |
| .query_pairs() | |
| .find(|(key, _)| key == "token") | |
| .map(|(_, value)| value.into_owned()) | |
| .unwrap(); | |
| let origin = format!( | |
| "{}://{}:{}", | |
| parsed_url.scheme(), | |
| parsed_url.host_str().unwrap(), | |
| parsed_url.port().unwrap() | |
| ); | |
| let _dashboard = Dashboard(child); | |
| let mut dashboard = Dashboard(child); | |
| let stdout = dashboard.0.stdout.take().unwrap(); | |
| let (sender, receiver) = mpsc::sync_channel(1); | |
| std::thread::spawn(move || { | |
| let mut first_line = String::new(); | |
| let result = BufReader::new(stdout) | |
| .read_line(&mut first_line) | |
| .map(|_| first_line); | |
| let _ = sender.send(result); | |
| }); | |
| let first_line = receiver | |
| .recv_timeout(Duration::from_secs(10)) | |
| .expect("dashboard startup timed out") | |
| .expect("read dashboard URL"); | |
| let url = first_line | |
| .split_once("dashboard: ") | |
| .map(|(_, url)| url.trim().to_string()) | |
| .expect("dashboard URL"); | |
| let parsed_url = reqwest::Url::parse(&url).unwrap(); | |
| assert_eq!(parsed_url.host_str(), Some("127.0.0.1")); | |
| let token = parsed_url | |
| .query_pairs() | |
| .find(|(key, _)| key == "token") | |
| .map(|(_, value)| value.into_owned()) | |
| .unwrap(); | |
| let origin = format!( | |
| "{}://{}:{}", | |
| parsed_url.scheme(), | |
| parsed_url.host_str().unwrap(), | |
| parsed_url.port().unwrap() | |
| ); | |
| let _dashboard = dashboard; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e_operator_dashboard.rs` around lines 131 - 152, Bound the dashboard
startup wait in the test setup around Dashboard(child): create the Dashboard
wrapper before reading output so its Drop implementation can terminate the
process on failure, move the blocking BufReader::read_line and URL parsing into
a helper thread, and receive the result with recv_timeout. On timeout, fail with
a diagnostic indicating dashboard startup did not emit its URL, while preserving
the existing URL validation and token/origin setup on success.
Source: Coding guidelines
2 participants
Summary
Safety and boundaries
/api/linksand/api/groupscontractswire-dash-v1and embeds unchangedwire-live-sessions-v2rows insidewire-topology-v1Verification
cargo fmt --checkcargo clippy --all-targets --all-features -- -D warningscargo test --all-targets --all-features: 700 library tests passed, one expected ignore, all CLI/E2E/integration/stress targets passedReview
Deferred
Summary by CodeRabbit
New Features
dash --weband optional--no-openlaunch controls.Bug Fixes