Skip to content

refactor: extract Browser/Network/Package Manager/System Monitor + close backlog - #169

Merged
AndrewAltimit merged 5 commits into
mainfrom
chore/backlog-cleanup
May 3, 2026
Merged

refactor: extract Browser/Network/Package Manager/System Monitor + close backlog#169
AndrewAltimit merged 5 commits into
mainfrom
chore/backlog-cleanup

Conversation

@AndrewAltimit

@AndrewAltimit AndrewAltimit commented May 3, 2026

Copy link
Copy Markdown
Owner

Summary

Closes the four remaining items from oasis-os-backlog.md AND lands the extraction follow-up that was the resolution of PR1.1's last bullet — all in one branch / one PR.

Commit 1 (60cd05b) — Doc-only cleanup

  • PR1.1 last bullet — record decision to extract Browser/Network/Package Manager/System Monitor from oasis-core into oasis-app-* crates.
  • PR3.5 — skipped. crates/oasis-net/src/tests.rs is genuinely test code, not fixture data. The backlog item was based on a misread; no rename needed.
  • PR4.2 — no code change. All 5 cited panic!() calls verified inside #[cfg(test)] mod tests blocks.
  • PR6 last bullet — audited the 4 ADRs untouched since 2026-02-19. ADR-001 / ADR-002 / ADR-004 and psp-plugin.md already carry Last reviewed: 2026-05-02 — still current annotations from PR docs(crates): subsystem references for js, terminal, net, audio, vector, shader #161. ADR-003 and ADR-005 had stale facts and are updated:
    • ADR-003: 8 extension traits → 9 in the SdiBackend bundle (SdiRenderTarget was missing); 39 optional methods → ~55 default-impl methods; calls out SdiBlendMode and SdiGeometry as additional extensions intentionally outside the bundle.
    • ADR-005: "8 distinct visual styles" → 15; "18 unique skins ship, 18 built-in + 14 external TOML" → 15 built-in / 12 external TOML / 3 built-in-only.

Commit 2 (2fc1bcf) — Apps extraction

The decision recorded in commit 1 is now executed in the same PR.

New crates (each with its own typed App impl + focused unit tests):

  • oasis-app-browser — Browser launcher placeholder (5 tests)
  • oasis-app-network — Network status (loopback / listener / remote connection state) (7 tests)
  • oasis-app-package-manager — Installed packages listing (4 tests)
  • oasis-app-system-monitor — Platform/backend/uptime/CPU placeholder (5 tests)

Each crate defines a struct (BrowserApp, NetworkApp, PackageManagerApp, SystemMonitorApp) with a ContentState field and implements oasis_app_core::App via the impl_content_app_methods!(content) macro — the same shape as oasis-app-settings.

Wiring:

  • Workspace Cargo.toml: 4 new members + workspace.dependencies entries.
  • oasis-core/Cargo.toml: depends on the 4 new crates.
  • crates/oasis-core/src/apps/registry.rs: title dispatch routes to the new types instead of SimpleApp::* constructors.
  • SimpleApp::network / ::package_manager / ::browser / ::system_monitor and their tests removed (dead post-extraction). SimpleApp itself stays for the in-core Terminal app and the plugin system's dynamic app placeholder.
  • runner.rs scroll test switched from "Network" to "Terminal" since "Network" no longer downcasts to SimpleApp.
  • System Monitor backend label upgraded from "SDL2" to "SDL3" while the constructor was being moved (pre-existing drift; SDL3 migration landed in PR feat: migrate SDL2 → SDL3 (3.4.2) #53).

Docs (CLAUDE.md / AGENTS.md):

  • App-crate count 9 → 13.
  • Crate-directory count 45 → 49.
  • Workspace member count 35 → 39.
  • oasis-core line rewritten — no app implementations remain in core beyond the Terminal delegate and the plugin SimpleApp placeholder.

Test plan

  • cargo fmt --all -- --check clean
  • cargo clippy --workspace -- -D warnings clean (the --all-targets form fails on pre-existing unwrap_used denies in oasis-ui test code — same on main)
  • cargo build --workspace clean
  • Targeted tests pass: oasis-app-browser (5), oasis-app-network (7), oasis-app-package-manager (4), oasis-app-system-monitor (5), oasis-core (613)
  • Pre-commit hooks (fmt, clippy, doc-build, markdown link check, cargo-deny) all pass
  • Pre-existing rmp3 UB precondition panic in oasis-backend-sdl::sdl_audio::tests::streaming_buffer_is_bounded reproduces on main and is unrelated to this PR (third-party decoder issue)

Generated with Claude Code

AI Agent Bot and others added 2 commits May 3, 2026 06:54
Closes the four remaining backlog items from oasis-os-backlog.md:

- PR1.1 last bullet — record decision to extract Browser/Network/
  Package Manager/System Monitor from oasis-core to oasis-app-*
  crates as a follow-up. CLAUDE.md updated in both the crate
  dependency graph and the oasis-core bullet.
- PR3.5 — skipped: crates/oasis-net/src/tests.rs is genuinely test
  code (free_port helpers, #[test] blocks, listener lifecycle), not
  fixture data. No rename needed.
- PR4.2 — no code change: all 5 cited panic!() calls verified inside
  #[cfg(test)] mod tests blocks (text-editor:572 in mod from L426,
  skin/builtin:347-350 in mod from L107, audio/radio/source:1028 in
  mod from L536).
- PR6 last bullet — audited the 4 ADRs untouched since 2026-02-19.
  ADR-001/002/004 and psp-plugin.md already carry "Last reviewed:
  2026-05-02 — still current" from PR #161. ADR-003 and ADR-005 had
  stale facts and are updated:
    * ADR-003: 8 extension traits → 9 in the SdiBackend bundle
      (SdiRenderTarget was missing); 39 optional methods → ~55
      default-impl methods; mention SdiBlendMode + SdiGeometry as
      additional extension traits outside the bundle.
    * ADR-005: 8 distinct visual styles → 15; "18 unique skins ship,
      18 built-in + 14 external TOML" → 15 built-in / 12 external
      TOML / 3 built-in-only (corrupted, desktop, modern).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Completes the extraction decision recorded in commit 60cd05b. The four
remaining in-core apps now live in their own oasis-app-* crates,
matching the pattern already used by Settings, File Manager, etc.

New crates:
- oasis-app-browser           — Browser launcher placeholder
- oasis-app-network           — Network status app (loopback, listener,
                                remote-connection state)
- oasis-app-package-manager   — Installed packages listing
- oasis-app-system-monitor    — Platform/backend/uptime/CPU placeholder

Each crate defines a typed App struct (BrowserApp, NetworkApp,
PackageManagerApp, SystemMonitorApp) with a ContentState field,
implements oasis_app_core::App via impl_content_app_methods!(content),
and ships its own focused unit tests (5 + 7 + 4 + 5 = 21 tests).
Behavioural parity with the previous SimpleApp::* constructors —
title, lines, navigation, downcast — is covered.

Wiring:
- Workspace Cargo.toml: 4 new members + workspace.dependencies entries
- oasis-core/Cargo.toml: depends on the 4 new crates
- crates/oasis-core/src/apps/registry.rs: dispatch routes the four
  app titles to the new types instead of SimpleApp constructors
- SimpleApp constructors network/package_manager/browser/system_monitor
  + their tests removed (dead code post-extraction). SimpleApp itself
  stays for the in-core Terminal app and the plugin system's dynamic
  app placeholder.
- runner.rs scroll test switched from "Network" to "Terminal" since
  Network no longer downcasts to SimpleApp.
- System Monitor backend label upgraded from "SDL2" to "SDL3" while
  the constructor was being moved (pre-existing drift; SDL3 migration
  landed in PR #53).

Docs:
- CLAUDE.md / AGENTS.md: app-crate count 9 → 13, crate-directory
  count 45 → 49, workspace member count 35 → 39, oasis-core line
  rewritten to reflect that no app implementations remain in core
  beyond the Terminal delegate and the plugin SimpleApp placeholder.

CI: cargo build --workspace, cargo fmt --all -- --check, cargo clippy
--workspace -- -D warnings, and targeted tests (oasis-app-browser /
network / package-manager / system-monitor / oasis-core) all pass.
The pre-existing rmp3 UB precondition panic in
oasis-backend-sdl::sdl_audio::tests::streaming_buffer_is_bounded
reproduces on main and is unrelated.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@AndrewAltimit AndrewAltimit changed the title docs: close out backlog items — extraction decision + ADR audit refactor: extract Browser/Network/Package Manager/System Monitor + close backlog May 3, 2026
@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown

Claude AI Architecture & Quality Review

Issues (if any)

  • [WARNING] crates/oasis-core/src/apps/registry.rs:30-31NetworkApp always instantiated with hardcoded defaults (listener_active: false, listener_port: 9000, remote_connected: false)

    • The AppFactory type is fn(&str, &dyn Vfs) -> Box<dyn App> — no channel to runtime network state. The Network status screen will permanently show "Not running" / "Not connected" regardless of actual system state.
    • Either the factory closure needs access to a shared state handle (e.g. Arc<NetworkState>), or NetworkApp needs a refresh(&mut self, ...) method called on each render tick. As-is, the extracted crate is more misleading than the previous SimpleApp version, which had the same problem but was at least clearly a placeholder.
  • [WARNING] crates/oasis-app-system-monitor/src/lib.rs:37"VFS: MemoryVfs" is hardcoded unconditionally

    • On UE5 (GameAssetVfs) or desktop with RealVfs this is wrong. The VFS type string should be passed as a constructor parameter alongside platform and backend, or omitted until oasis-platform exposes it.

Previous Issues (for incremental reviews)

(none)

Suggestions (if any)

  • crates/oasis-app-browser/Cargo.toml, oasis-app-network/Cargo.toml, oasis-app-package-manager/Cargo.toml, oasis-app-system-monitor/Cargo.tomloasis-sdi and oasis-skin are listed as direct dependencies in all four new crates but are not directly imported in any lib.rs. They are needed only transitively via impl_content_app_methods! expanding to oasis_sdi::SdiRegistry / oasis_skin::ActiveTheme references inside oasis-app-core. This follows the same pattern as oasis-app-settings, so it's consistent, but the pattern causes all four new crates to appear heavier than they are. Centralising the re-exports in oasis-app-core would let downstream crates drop the two explicit entries.

  • crates/oasis-app-package-manager/src/lib.rs:28-30 — hardcoded entries show version 0.1.0 [UNVERIFIED - File does not exist and is not in PR] while the workspace version is 1.3.0 [UNVERIFIED - File does not exist and is not in PR]. Fine for a placeholder, but the version mismatch will confuse anyone who reads the screen on a shipped binary.

Notes

  • 2 claim(s) were automatically filtered as potential hallucinations (file:line content didn't match claims)

  • Extraction is structurally correct and consistent with the prior oasis-app-settings / oasis-app-file-manager pattern.

  • runner.rs test pivot from "Network""Terminal" is the right fix; the comment explaining why is helpful.

  • SDL2 → SDL3 label correction in the SystemMonitorApp constructor call is a valid cleanup caught opportunistically.

  • ADR updates (003, 005) are accurate and well-scoped.

Reaction


Generated by Claude AI (sonnet). Supplementary to human reviews.

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown

Claude AI Incremental Security & Correctness Review

This is an incremental review focusing on changes since the last review.

Issues (if any)

(none — no new security or correctness issues in the new files)

Previous Issues (for incremental reviews)

  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:30NetworkApp always instantiated with hardcoded defaults (false, 9000, false)

    • The factory closure |path, _vfs| still has no access to runtime network state; the screen permanently shows "Not running" / "Not connected"
  • [STILL UNRESOLVED] crates/oasis-app-system-monitor/src/lib.rs:37"VFS: MemoryVfs" hardcoded unconditionally

    • The string is now in the new crate's lib.rs (migrated verbatim from SimpleApp::system_monitor); still wrong on UE5 (GameAssetVfs) and desktop (RealVfs)

Suggestions (if any)

  • crates/oasis-app-package-manager/src/lib.rs:26-28 — displayed version strings 0.1.0 [UNVERIFIED - File does not exist and is not in PR] don't match workspace version 1.3.0 [UNVERIFIED - File does not exist and is not in PR]; a deployed binary will show obviously wrong metadata (pre-existing issue, migrated verbatim)

Notes

  • 2 claim(s) were automatically filtered as potential hallucinations (file:line content didn't match claims)

  • All four new crates are structurally sound: correct App trait impl, no unsafe code, no panics outside #[cfg(test)], no injection surface

  • runner.rs pivot from "Network""Terminal" for the SimpleApp downcast test is correct and the comment explaining why is clear

  • ADR updates are accurate: 9 extensions in SdiBackend bundle, 15 built-in skins / 12 external TOML / 3 built-in-only

Reaction


Generated by Claude AI (sonnet). Supplementary to human reviews.

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown

Openrouter AI General Review

Issues (if any)

  • [WARNING] crates/oasis-app-system-monitor/src/lib.rs:36 - "VFS: MemoryVfs" is hardcoded unconditionally

    • Displays incorrect VFS type on UE5 (GameAssetVfs) or desktop (RealVfs) backends
    • Pass vfs_type: &str as a constructor parameter or query it from the Vfs trait before instantiation
  • [WARNING] crates/oasis-core/src/apps/registry.rs:29 - NetworkApp instantiated with hardcoded defaults (false, 9000, false)

    • UI permanently shows "Not running" / "Not connected" regardless of actual network state
    • Inject runtime state via Arc<NetworkState> into the factory closure, or add a refresh() method called per render tick

Previous Issues (for incremental reviews)

(none)

Suggestions (if any)

  • crates/oasis-app-package-manager/src/lib.rs:28 - Hardcoded version 0.1.0 [UNVERIFIED - File does not exist and is not in PR] for oasis-core and oasis-sdl mismatches workspace 1.3.0 [UNVERIFIED - File does not exist and is not in PR]

    • Update placeholder strings or derive from env!("CARGO_PKG_VERSION") to avoid confusing users on shipped binaries
  • crates/oasis-app-browser/Cargo.toml (and 3 others) - oasis-sdi and oasis-skin are direct dependencies but only used transitively via impl_content_app_methods!

    • Consistent with existing oasis-app-* crates, but centralizing re-exports in oasis-app-core would allow dropping these explicit entries

Notes

  • 2 claim(s) were automatically filtered as potential hallucinations (file:line content didn't match claims)

  • Extraction is structurally sound and strictly follows the established oasis-app-settings pattern

  • runner.rs test pivot to "Terminal" correctly prevents SimpleApp downcast failures on the new concrete types

  • ADR updates accurately reflect current codebase state (9 SDI extensions, 15 built-in skins)

  • All new crates include comprehensive unit tests covering title, content rendering, input handling, and type downcasting

Reaction


Generated by Openrouter AI (qwen/qwen3.6-plus). Supplementary to human reviews.

Automated fix by Claude in response to AI review feedback.

Iteration: 1/5

Co-Authored-By: AI Review Agent <noreply@anthropic.com>
@AndrewAltimit

Copy link
Copy Markdown
Owner Author

Review Response Agent (Iteration 1)

Status: Changes committed, pushing...

Commit: e7a17db

Fixed Issues

  • crates/oasis-app-system-monitor/src/lib.rs — VFS row was hardcoded "MemoryVfs" and wrong on UE5/desktop. Added vfs_type(&self) -> &'static str to the Vfs trait (default "MemoryVfs"), overrode it in RealVfs ("RealVfs") and GameAssetVfs ("GameAssetVfs"), added a vfs_type parameter to SystemMonitorApp::new(), and updated the registry to pass vfs.vfs_type() so each backend renders its actual VFS implementation.
  • crates/oasis-app-package-manager/src/lib.rsoasis-core and oasis-sdl rows showed stale 0.1.0 instead of the workspace version. Switched to env!("CARGO_PKG_VERSION") so the placeholder rows track the workspace version (1.3.0) automatically.

Ignored Issues

  • (none)

Deferred to Human

  • crates/oasis-core/src/apps/registry.rs NetworkApp factory — listener_active / listener_port / remote_connected are hardcoded false, 9000, false. Fixing it properly requires changing the AppFactory signature (fn(&str, &dyn Vfs) -> Box<dyn App>) to thread a runtime state handle (Arc<NetworkState>) into the closure, or adding a refresh() method on apps invoked per render tick. Both are architectural changes outside the scope of this extraction PR; reviewers explicitly note the prior SimpleApp placeholder had the same limitation.

Notes

  • Verified with cargo build --workspace, cargo test -p oasis-vfs -p oasis-app-system-monitor -p oasis-app-package-manager -p oasis-core, cargo clippy --workspace -- -D warnings, and cargo fmt --all -- --check. All green.
  • Added one new test lines_show_passed_vfs_type in oasis-app-system-monitor to cover the new parameter; all six existing tests in that crate were updated to the new constructor signature.
  • The "UNVERIFIED - File does not exist and is not in PR" markers in the reviewer output were spurious — the package-manager file is in the PR — so I treated those suggestions as legitimate and fixed them.

Automated summary of agent fixes.

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown

Openrouter AI Incremental General Review

This is an incremental review focusing on changes since the last review.

Issues (if any)

  • [WARNING] crates/oasis-core/src/apps/registry.rs:40-41 — Platform and backend strings hardcoded to "Desktop (SDL3)" / "SDL3"
    • Will display incorrect backend on UE5 (GameAssetVfs), WASM, or PSP targets.
    • Requires threading platform/backend identifiers through AppFactory or querying them from a shared state handle.

Previous Issues (for incremental reviews)

  • [RESOLVED] crates/oasis-app-system-monitor/src/lib.rs:36-37"VFS: MemoryVfs" hardcoded unconditionally
  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:29NetworkApp instantiated with hardcoded defaults (false, 9000, false)

Suggestions (if any)

  • crates/oasis-app-package-manager/src/lib.rs:26-27env!("CARGO_PKG_VERSION") is used for oasis-core and oasis-sdl versions. This works while workspace versions are synchronized, but will mislead if crate versions diverge. Consider deriving from actual dependency metadata if a real package manager is implemented later.

Notes

  • Extraction is structurally sound and strictly follows the established oasis-app-* pattern.
  • runner.rs test pivot to "Terminal" correctly prevents SimpleApp downcast failures on the new concrete types.
  • ADR updates accurately reflect current codebase state (9 SDI extensions, 15 built-in skins).
  • All new crates include comprehensive unit tests covering title, content rendering, input handling, and type downcasting.
  • The vfs_type() trait addition cleanly resolves the hardcoded VFS string without breaking existing MemoryVfs usage.

Reaction


Generated by Openrouter AI (qwen/qwen3.6-plus). Supplementary to human reviews.

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown

Claude AI Incremental Architecture & Quality Review

This is an incremental review focusing on changes since the last review.

Issues (if any)

  • [WARNING] crates/oasis-vfs/src/lib.rs:210-213vfs_type() default implementation returns "MemoryVfs"
    • Any future Vfs implementor that forgets to override will silently identify as MemoryVfs regardless of what it actually is. The wrong name in a diagnostic screen is confusing but not catastrophic; the silent nature is the real hazard.
    • Either remove the default (force all impls to declare their type) or return a neutral sentinel like "UnknownVfs". The latter is non-breaking and makes misidentification visible.

Previous Issues (for incremental reviews)

  • [RESOLVED] crates/oasis-app-system-monitor/src/lib.rs:36-37"VFS: MemoryVfs" hardcoded unconditionally. Fixed: vfs_type is now a constructor parameter, populated via vfs.vfs_type() in the registry.
  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:29NetworkApp instantiated with hardcoded defaults (false, 9000, false). The factory closure still has no access to runtime network state; the screen permanently shows "Not running" / "Not connected".
  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:40-41"Desktop (SDL3)" and "SDL3" remain hardcoded. The AppFactory signature fn(&str, &dyn Vfs) -> Box<dyn App> carries no channel for platform/backend identity, so WASM and UE5 targets will display incorrect values.

Suggestions (if any)

  • crates/oasis-app-package-manager/src/lib.rs:24env!("CARGO_PKG_VERSION") resolves to oasis-app-package-manager's own version, used here as a proxy for oasis-core and oasis-sdl. Accurate only while workspace versions stay synchronized. Fine for a placeholder, but worth a comment so a future reader doesn't assume these are independently queried.

Notes

  • 1 claim(s) were automatically filtered as potential hallucinations (file:line content didn't match claims)

  • vfs_type() trait addition in oasis-vfs is a minor layer concern (a display label on a storage abstraction), but it's contained and follows a precedent set by similar identification methods in the codebase.

  • The two still-unresolved hardcoded-defaults issues both stem from the same root: AppFactory has no parameter for runtime state beyond VFS. Fixing either properly requires either widening the factory signature or introducing a refresh(&mut self, ctx: &AppContext) on the App trait — both are architectural changes intentionally deferred per the PR description.

  • Package manager env! fix is correct and an improvement over the prior 0.1.0 [UNVERIFIED - File does not exist and is not in PR] placeholder.

Reaction


Generated by Claude AI (sonnet). Supplementary to human reviews.

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown

Claude AI Incremental Security & Correctness Review

This is an incremental review focusing on changes since the last review.

Issues (if any)

(none — no new security or correctness issues in the new files)

Previous Issues (for incremental reviews)

  • [RESOLVED] crates/oasis-app-system-monitor/src/lib.rs:37"VFS: MemoryVfs" hardcoded unconditionally. Fixed: vfs_type is now a constructor parameter, populated via vfs.vfs_type() in the registry, with RealVfs and GameAssetVfs overriding the default.

  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:30NetworkApp instantiated with hardcoded defaults (false, 9000, false). The factory closure |path, _vfs| still has no access to runtime network state; the screen permanently shows "Not running" / "Not connected".

  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:40-41"Desktop (SDL3)" and "SDL3" remain hardcoded. WASM and UE5 targets will display incorrect platform/backend strings. The AppFactory signature fn(&str, &dyn Vfs) -> Box<dyn App> carries no channel for platform or backend identity.

  • [STILL UNRESOLVED] crates/oasis-vfs/src/lib.rs:210-213vfs_type() default returns "MemoryVfs". Any future Vfs implementor that omits an override will silently self-identify as MemoryVfs.

Suggestions (if any)

(none)

Notes

  • 1 claim(s) were automatically filtered as potential hallucinations (file:line content didn't match claims)

  • All four new crates are structurally sound: correct App trait impl, no unsafe code, no panics outside #[cfg(test)], no injection surface.

  • env!("CARGO_PKG_VERSION") in oasis-app-package-manager correctly reflects the workspace version now instead of the stale 0.1.0 [UNVERIFIED - File does not exist and is not in PR] — a valid improvement even if semantically approximate.

  • The three still-unresolved issues all share the same root cause: the AppFactory signature lacks a channel for runtime state beyond VFS. Deferred as architectural work per the PR description; the deferral is clearly documented.

Reaction


Generated by Claude AI (sonnet). Supplementary to human reviews.

Automated fix by Claude in response to AI review feedback.

Iteration: 2/5

Co-Authored-By: AI Review Agent <noreply@anthropic.com>
@AndrewAltimit

Copy link
Copy Markdown
Owner Author

Review Response Agent (Iteration 2)

Status: Changes committed, pushing...

Commit: b0fcf75

Fixed Issues

  • crates/oasis-vfs/src/lib.rsvfs_type() default returned "MemoryVfs", which silently mislabels any future Vfs implementor that forgets to override it. Changed default to "UnknownVfs" (a neutral sentinel that surfaces missing overrides) and updated the doc comment to call this out.
  • crates/oasis-vfs/src/memory.rs — Added explicit vfs_type() override on MemoryVfs returning "MemoryVfs" to preserve the existing System Monitor display now that the default is "UnknownVfs".

Ignored Issues

  • crates/oasis-app-package-manager/src/lib.rs — Reviewer suggestion to add a comment noting that env!("CARGO_PKG_VERSION") works as a proxy for oasis-core / oasis-sdl versions only while workspace versions stay synchronized. The existing doc comment ("reflect the workspace version this crate was built against") already conveys this; further commentary would be redundant.

Deferred to Human

  • crates/oasis-core/src/apps/registry.rs NetworkApp factory — listener_active/listener_port/remote_connected hardcoded to false, 9000, false. Already deferred in iteration 1; same architectural change required (widen AppFactory signature or add App::refresh()).
  • crates/oasis-core/src/apps/registry.rs:42-43"Desktop (SDL3)" and "SDL3" hardcoded for SystemMonitorApp. Will display incorrectly on WASM/UE5/PSP. Same root cause as the NetworkApp issue: AppFactory carries no channel for platform/backend identity. Fixing it independently would diverge from the iteration-1 deferral; bundling with that architectural pass is the right call.

Notes

  • Verified with cargo build -p oasis-vfs -p oasis-app-system-monitor -p oasis-core, cargo test -p oasis-vfs -p oasis-app-system-monitor, cargo clippy --workspace -- -D warnings, and cargo fmt --all -- --check. All green.
  • All existing System Monitor tests still pass — MemoryVfs::vfs_type() continues to return "MemoryVfs", so the diagnostic display is unchanged for the common case.
  • The RealVfs and GameAssetVfs overrides were already in place from iteration 1; only MemoryVfs needed to be made explicit now that the default no longer matches it.

Automated summary of agent fixes.

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown

Claude AI Incremental Architecture & Quality Review

This is an incremental review focusing on changes since the last review.

Issues (if any)

(none — no new issues in the two new files)

Previous Issues (for incremental reviews)

  • [RESOLVED] crates/oasis-vfs/src/lib.rs:210-213vfs_type() default returned "MemoryVfs". Fixed: default is now "UnknownVfs"; MemoryVfs gained an explicit override returning "MemoryVfs", making missing overrides visible rather than silently mislabelled.
  • [RESOLVED] crates/oasis-app-system-monitor/src/lib.rs:36-37"VFS: MemoryVfs" hardcoded unconditionally. Resolved in iteration 1 (constructor takes vfs_type parameter; registry passes vfs.vfs_type()).
  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:29-31NetworkApp instantiated with hardcoded defaults (false, 9000, false). Factory closure |path, _vfs| has no channel to runtime network state; screen permanently shows "Not running" / "Not connected". Explicitly deferred per PR description.
  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:40-41"Desktop (SDL3)" and "SDL3" hardcoded. Will display incorrectly on WASM/UE5/PSP. Same root cause as NetworkAppAppFactory signature carries no platform/backend context. Explicitly deferred.

Suggestions (if any)

(none)

Notes

  • The two new files (lib.rs, memory.rs) are minimal and correct — the sentinel default plus explicit MemoryVfs override is the right pattern.
  • Both remaining unresolved items share the same root: AppFactory is fn(&str, &dyn Vfs) -> Box<dyn App> with no slot for runtime state. Widening that signature or adding App::refresh(&mut self, ctx: &AppContext) would close both in one pass.

Reaction


Generated by Claude AI (sonnet). Supplementary to human reviews.

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown

Claude AI Incremental Security & Correctness Review

This is an incremental review focusing on changes since the last review.

Issues (if any)

(none — no new security or correctness issues in the new files)

Previous Issues (for incremental reviews)

  • [RESOLVED] crates/oasis-vfs/src/lib.rs:210-213vfs_type() default returned "MemoryVfs". Fixed: default is now "UnknownVfs"; MemoryVfs gained an explicit override, making missing overrides visible.
  • [RESOLVED] crates/oasis-app-system-monitor/src/lib.rs:36-37"VFS: MemoryVfs" hardcoded unconditionally. Fixed: constructor now takes vfs_type: &str; registry passes vfs.vfs_type().
  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:29NetworkApp instantiated with hardcoded defaults (false, 9000, false). The factory closure |path, _vfs| carries no runtime network state; the screen permanently shows "Not running" / "Not connected". Explicitly deferred per PR description.
  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:40-41"Desktop (SDL3)" and "SDL3" hardcoded. Will display incorrectly on WASM/UE5/PSP. Same root cause as NetworkAppAppFactory has no channel for platform/backend identity. Explicitly deferred.

Suggestions (if any)

(none)

Notes

  • All four new crates are structurally sound: correct App trait impl, no unsafe code, no panics outside #[cfg(test)], no injection surface.
  • Both remaining unresolved items share the same root cause: AppFactory is fn(&str, &dyn Vfs) -> Box<dyn App> with no slot for runtime state. One architectural pass (widen signature or add App::refresh()) would close both.
  • MemoryVfs explicit override and "UnknownVfs" sentinel default is the correct pattern — future Vfs implementors will surface a missing override immediately rather than silently mislabelling.

Reaction


Generated by Claude AI (sonnet). Supplementary to human reviews.

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown

Openrouter AI Incremental General Review

This is an incremental review focusing on changes since the last review.

Issues (if any)

(none)

Previous Issues (for incremental reviews)

  • [RESOLVED] crates/oasis-vfs/src/lib.rs:210-213vfs_type() default returned "MemoryVfs". Fixed: default changed to "UnknownVfs" sentinel; MemoryVfs, RealVfs, and GameAssetVfs now explicitly override it.
  • [RESOLVED] crates/oasis-app-system-monitor/src/lib.rs:36-37"VFS: MemoryVfs" hardcoded unconditionally. Fixed: constructor now accepts vfs_type: &str; registry passes vfs.vfs_type().
  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:29-31NetworkApp instantiated with hardcoded defaults (false, 9000, false). Factory closure lacks runtime state channel; explicitly deferred per PR scope.
  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:40-41 — Platform/backend strings hardcoded to "Desktop (SDL3)" / "SDL3". Will misrepresent WASM/UE5/PSP targets; explicitly deferred per PR scope.

Suggestions (if any)

  • crates/oasis-app-package-manager/src/lib.rs:26 — Display name oasis-sdl mismatches the actual workspace crate name oasis-backend-sdl. Aligning the string prevents future confusion when a real package manager queries metadata.
  • crates/oasis-app-system-monitor/src/lib.rs:34-36 — Uptime hours are unpadded ({h}). If uptime exceeds 9 hours, column alignment shifts. Consider {h:>2} or {h:02} if strict table formatting is desired.

Notes

  • Extraction strictly follows the established oasis-app-* pattern; all four new crates implement App correctly with comprehensive unit tests.
  • runner.rs test pivot from "Network" to "Terminal" correctly prevents SimpleApp downcast failures on the new concrete types.
  • The "UnknownVfs" sentinel default is a robust improvement that surfaces missing trait overrides immediately rather than silently mislabeling.
  • ADR updates accurately reflect current counts (9 SDI extensions, 15 built-in skins, 13 app crates).

Reaction


Generated by Openrouter AI (qwen/qwen3.6-plus). Supplementary to human reviews.

Automated fix by Claude in response to AI review feedback.

Iteration: 3/5

Co-Authored-By: AI Review Agent <noreply@anthropic.com>
@AndrewAltimit

Copy link
Copy Markdown
Owner Author

Review Response Agent (Iteration 3)

Status: Changes committed, pushing...

Commit: 4933a97

Fixed Issues

  • crates/oasis-app-package-manager/src/lib.rs:29 — Display name oasis-sdl mismatched the actual workspace crate name oasis-backend-sdl. Renamed to oasis-backend-sdl so the listed name matches the real crate (and aligns with the dependency graph in CLAUDE.md). Spacing in the row tightened by one space to keep the column from drifting; tests still pass.

Ignored Issues

  • crates/oasis-app-system-monitor/src/lib.rs:38 — Reviewer suggestion to pad uptime hours ({h}{h:>2} or {h:02}). The H:MM:SS format is the standard Unix uptime convention; padding hours produces awkward output for short uptimes (00:01:01 or 1:01:01) without solving an actual alignment problem (no surrounding column system depends on it). Per project guidance ("Don't add features... beyond what the task requires"), skipping this purely cosmetic suggestion.
  • crates/oasis-vfs/src/lib.rs:210-213 — already fixed in iteration 2 (default is now "UnknownVfs"; reviewers confirmed RESOLVED).
  • crates/oasis-app-system-monitor/src/lib.rs:36-37 — already fixed in iteration 1 (vfs_type parameter; reviewers confirmed RESOLVED).

Deferred to Human

  • crates/oasis-core/src/apps/registry.rs:29-31 NetworkApp factory hardcoded false, 9000, false. Same architectural deferral as iterations 1 and 2: AppFactory signature (fn(&str, &dyn Vfs) -> Box<dyn App>) carries no slot for runtime network state. Closing it requires either widening the factory signature to accept an AppContext carrying Arc<NetworkState>, or adding App::refresh(&mut self, ctx: &AppContext) invoked per render tick.
  • crates/oasis-core/src/apps/registry.rs:40-41 "Desktop (SDL3)" / "SDL3" hardcoded for SystemMonitorApp. Same root cause as NetworkAppAppFactory carries no platform/backend identity. Both deferred items would close in one architectural pass on AppFactory.

Notes

  • Verified with cargo test -p oasis-app-package-manager (4/4 passed), cargo clippy -p oasis-app-package-manager -- -D warnings (clean), and cargo fmt --all -- --check (clean).
  • All three reviewers (Claude Quality, Claude Security, OpenRouter) reported "(none)" for new issues on b0fcf75. The only actionable suggestion across all three reviews was the oasis-sdl rename, which is now applied.

Automated summary of agent fixes.

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown

Claude AI Incremental Security & Correctness Review

This is an incremental review focusing on changes since the last review.

Issues (if any)

(none — no new security or correctness issues in the new file)

Previous Issues (for incremental reviews)

  • [RESOLVED] crates/oasis-app-system-monitor/src/lib.rs:37"VFS: MemoryVfs" hardcoded unconditionally. Fixed: constructor takes vfs_type parameter; registry passes vfs.vfs_type().
  • [RESOLVED] crates/oasis-vfs/src/lib.rs:210-213vfs_type() default returned "MemoryVfs". Fixed: default is now "UnknownVfs"; MemoryVfs gained an explicit override.
  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:30NetworkApp instantiated with hardcoded defaults (false, 9000, false). Factory closure |path, _vfs| still has no access to runtime network state; screen permanently shows "Not running" / "Not connected". Explicitly deferred per PR description.
  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:40-41"Desktop (SDL3)" and "SDL3" hardcoded for SystemMonitorApp. Will display incorrectly on WASM/UE5/PSP. Same root cause as NetworkAppAppFactory carries no channel for platform/backend identity. Explicitly deferred.

Suggestions (if any)

(none)

Notes

  • 1 claim(s) were automatically filtered as potential hallucinations (file:line content didn't match claims)

  • crates/oasis-app-package-manager/src/lib.rs is clean: no unsafe code, no panics outside #[cfg(test)], no injection surface. The env!("CARGO_PKG_VERSION") fix correctly replaces the stale 0.1.0 [UNVERIFIED - File does not exist and is not in PR] placeholder.

  • The oasis-sdloasis-backend-sdl rename in the display string matches the actual workspace crate name.

  • Both remaining unresolved items share the same root: AppFactory is fn(&str, &dyn Vfs) -> Box<dyn App> with no slot for runtime state. One architectural pass would close both.

Reaction


Generated by Claude AI (sonnet). Supplementary to human reviews.

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown

Openrouter AI Incremental General Review

This is an incremental review focusing on changes since the last review.

Issues (if any)

(none)

Previous Issues (for incremental reviews)

  • [RESOLVED] crates/oasis-app-system-monitor/src/lib.rs:36-37"VFS: MemoryVfs" hardcoded unconditionally. Fixed: constructor now accepts vfs_type parameter; registry passes vfs.vfs_type().
  • [RESOLVED] crates/oasis-vfs/src/lib.rs:210-213vfs_type() default returned "MemoryVfs". Fixed: default changed to "UnknownVfs" sentinel; MemoryVfs, RealVfs, and GameAssetVfs explicitly override it.
  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:29-31NetworkApp instantiated with hardcoded defaults (false, 9000, false). Factory closure lacks runtime state channel; explicitly deferred per PR scope.
  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:40-41 — Platform/backend strings hardcoded to "Desktop (SDL3)" / "SDL3". Will misrepresent WASM/UE5/PSP targets; explicitly deferred per PR scope.

Suggestions (if any)

  • crates/oasis-app-package-manager/src/lib.rs:24env!("CARGO_PKG_VERSION") is used as a proxy for oasis-core and oasis-backend-sdl versions. While accurate under current workspace synchronization, consider adding a // TODO comment to clarify this assumption for future maintainers if crate versions ever diverge.

Notes

  • Extraction strictly follows the established oasis-app-* pattern; all four new crates implement App correctly with comprehensive unit tests.
  • runner.rs test pivot from "Network" to "Terminal" correctly prevents SimpleApp downcast failures on the new concrete types.
  • The "UnknownVfs" sentinel default is a robust improvement that surfaces missing trait overrides immediately rather than silently mislabeling.
  • ADR updates accurately reflect current counts (9 SDI extensions, 15 built-in skins, 13 app crates).

Reaction


Generated by Openrouter AI (qwen/qwen3.6-plus). Supplementary to human reviews.

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown

Claude AI Incremental Architecture & Quality Review

This is an incremental review focusing on changes since the last review.

Issues (if any)

(none — no new architectural or quality issues in the new files)

Previous Issues (for incremental reviews)

  • [RESOLVED] crates/oasis-app-system-monitor/src/lib.rs:36-37"VFS: MemoryVfs" hardcoded unconditionally. Fixed: constructor now takes vfs_type: &str; registry passes vfs.vfs_type().
  • [RESOLVED] crates/oasis-vfs/src/lib.rs:210-213vfs_type() default returned "MemoryVfs". Fixed: default is now "UnknownVfs"; MemoryVfs gained an explicit override. RealVfs and GameAssetVfs overrides also confirmed present.
  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:30NetworkApp::new(path, false, 9000, false) still hardcoded. Factory closure |path, _vfs| carries no runtime network state; screen permanently shows "Not running" / "Not connected". Explicitly deferred per PR.
  • [STILL UNRESOLVED] crates/oasis-core/src/apps/registry.rs:40-43"Desktop (SDL3)", "SDL3", and 0 (uptime) all hardcoded for SystemMonitorApp. AppFactory carries no channel for platform/backend identity or live uptime. Explicitly deferred per PR.

Suggestions (if any)

(none — previous suggestions have been addressed or are deferred with documented rationale)

Notes

  • Both remaining unresolved items share the same root: AppFactory is fn(&str, &dyn Vfs) -> Box<dyn App> with no slot for runtime state beyond VFS. One pass widening to an AppContext parameter or adding App::refresh(&mut self, ctx: &AppContext) closes both.
  • All four new crates are structurally sound: correct App trait impl pattern, no unsafe code, no panics outside #[cfg(test)], consistent with the established oasis-app-settings shape.
  • SimpleApp cleanup (removal of network, package_manager, browser, system_monitor constructors and their tests) is correct and complete.

Reaction


Generated by Claude AI (sonnet). Supplementary to human reviews.

@AndrewAltimit

Copy link
Copy Markdown
Owner Author

Review Response Agent (Iteration 4)

Status: Hallucination detected — no commit

Detection: The agent's summary below claims to have applied fixes, > but Claude made zero file-mutating tool calls (Edit / Write / MultiEdit / NotebookEdit) > during the run. The retry was skipped because it uses the same prompting > strategy Claude already ignored.

Stream log: /home/mikunpc/Documents/repos/actions-runner-oasis-os/_work/_temp/review-agent-logs/claude-stream-iter3-1777813850114.jsonl
Uploaded as the review-agent-claude-logs workflow artifact.

Fixed Issues

  • (none — no Edit calls made this iteration)

Ignored Issues

  • crates/oasis-app-system-monitor/src/lib.rs:36-37 — "VFS: MemoryVfs" hardcoded. Already fixed in prior iteration: constructor now takes vfs_type: &str parameter (line 25), registry passes vfs.vfs_type(). All three reviewers marked this RESOLVED.
  • crates/oasis-vfs/src/lib.rs:210-213vfs_type() default returned "MemoryVfs". Already fixed in prior iteration: default is now "UnknownVfs" sentinel (line 216), with explicit overrides for MemoryVfs/RealVfs/GameAssetVfs. All three reviewers marked this RESOLVED.
  • crates/oasis-app-package-manager/src/lib.rs:29oasis-sdloasis-backend-sdl rename. Already fixed in prior iteration (HEAD commit 4933a97).
  • OpenRouter suggestion to add a // TODO comment in crates/oasis-app-package-manager/src/lib.rs:24 clarifying that env!("CARGO_PKG_VERSION") is a proxy for oasis-core/oasis-backend-sdl versions. Skipped: the existing doc comment on lines 19-20 already states "The oasis-core and backend rows reflect the workspace version this crate was built against." Adding a redundant TODO violates the project's "default to no comments" guidance, and OpenRouter framed this as a "consider" suggestion, not an issue.

Deferred to Human

  • crates/oasis-core/src/apps/registry.rs:30NetworkApp instantiated with hardcoded defaults (false, 9000, false). All three reviewers explicitly flagged this as deferred per PR scope. Root cause: AppFactory = fn(&str, &dyn Vfs) -> Box<dyn App> has no slot for runtime network state. Fix requires widening to an AppContext parameter — out of scope for this extraction PR.
  • crates/oasis-core/src/apps/registry.rs:40-43SystemMonitorApp instantiated with hardcoded "Desktop (SDL3)", "SDL3", and uptime 0. Same AppFactory root cause as above; will misrepresent WASM/UE5/PSP targets. Explicitly deferred per PR scope by all three reviewers.

Notes

  • The HEAD commit (4933a97 by AI Review Agent) already addressed the oasis-sdloasis-backend-sdl cosmetic fix. The PR is in a clean state with respect to the in-scope review feedback.
  • Both deferred items share a single architectural fix (widen AppFactory signature), which the reviewers themselves note: "One pass widening to an AppContext parameter or adding App::refresh(&mut self, ctx: &AppContext) closes both." This is a follow-up PR, not a bug fix.
  • Filtered claim from Claude Security review (re: oasis-app-package-manager/src/lib.rs not existing) was a tooling false positive — the file exists and is part of the PR.

No file modifications were performed; this iteration produced no commit.

@AndrewAltimit
AndrewAltimit merged commit 3697a43 into main May 3, 2026
10 checks passed
@AndrewAltimit
AndrewAltimit deleted the chore/backlog-cleanup branch May 3, 2026 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant