feat: add native homelab inventory map - #67
Merged
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
📝 WalkthroughWalkthroughAdds a homelab inventory subsystem (collectors, schema, redaction, storage, orchestration), new ChangesHomelab Inventory Subsystem
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e79ba7fad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| schema: MAP_SCHEMA.to_string(), | ||
| generated_at: rfc3339_z(Utc::now()), | ||
| cache_status: cache_status.status.clone(), | ||
| freshness: inventory.as_ref().map(|i| i.freshness.clone()), |
There was a problem hiding this comment.
When a cached inventory is older than stale_after_secs, inventory_status() correctly computes is_stale, but this response returns the serialized inventory.freshness from the cache file. Since refresh writes that field with is_stale: false, action=map will keep reporting stale caches as fresh indefinitely; use the status age/staleness calculation to update the returned freshness.
Useful? React with 👍 / 👎.
Comment on lines
+96
to
+98
| let total_hosts = log_hosts | ||
| .max(nodes.len()) | ||
| .max(inventory.as_ref().map(|i| i.nodes.len()).unwrap_or(0)); |
There was a problem hiding this comment.
When host_limit is reached by log hosts, additional heartbeat-only or inventory-only hosts are skipped in merge_heartbeat/merge_inventory_nodes, but total_hosts is only the max of the individual source counts. For example, with one log host and one distinct inventory host and host_limit=1, this reports hosts=1 and truncated_hosts=false even though a host was omitted; compute the distinct union or at least include skipped source totals so truncation is visible.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adds a native (Rust) homelab inventory subsystem (private on-disk cache + collectors + redaction) and upgrades the MCP map action to return a cached inventory snapshot with a bounded live Cortex overlay (hosts + heartbeat), along with CLI support for refreshing/inspecting inventory.
Changes:
- Added a new
src/inventory/module with collectors (device, Docker, Tailscale, UniFi, Unraid, media stack, raw config parsing, projects) plus private cache storage + redaction. - Introduced
cortex inventory refresh|statusCLI commands and made them explicitly local-only (reject HTTP flags). - Upgraded MCP
action=mapoutput schema tocortex.homelab_map.v2, updated smoke tests/docs, and bumped version to1.10.0.
Reviewed changes
Copilot reviewed 56 out of 57 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mcp/tools_tests.rs | Updates map-action tests for cortex.homelab_map.v2 response shape. |
| src/mcp/actions.rs | Updates map action description to reflect cached inventory + overlay. |
| src/main.rs | Routes inventory CLI command and rejects HTTP flags for it. |
| src/main_tests.rs | Updates mode parsing test message to include inventory. |
| src/lib.rs | Exposes new inventory module from the library. |
| src/inventory/mod.rs | Adds inventory module wiring and re-exports cache/orchestrator APIs. |
| src/inventory/limits.rs | Defines inventory/map schema constants and collection/cap limits. |
| src/inventory/schema.rs | Introduces typed inventory/cache schema structures. |
| src/inventory/config.rs | Adds env-driven inventory configuration (paths, endpoints, deadlines). |
| src/inventory/collectors.rs | Defines CollectorOutput and warning/error recording shape. |
| src/inventory/process.rs | Adds capped command execution helper with redacted stderr. |
| src/inventory/process_tests.rs | Tests command timeout behavior and shell word splitting. |
| src/inventory/redaction.rs | Adds text/JSON redaction and artifact truncation helpers. |
| src/inventory/redaction_tests.rs | Tests redaction for common secret shapes and truncation reporting. |
| src/inventory/storage.rs | Adds private atomic writes, symlink rejection, and refresh lock. |
| src/inventory/storage_tests.rs | Tests private permissions and symlink rejection behavior. |
| src/inventory/http.rs | Adds HTTP probing with redaction and body-size capping. |
| src/inventory/cache.rs | Implements inventory cache read + freshness/status reporting. |
| src/inventory/cache_tests.rs | Tests missing-cache status behavior. |
| src/inventory/orchestrator.rs | Orchestrates collector runs, caps sections, writes cache/state reports. |
| src/inventory/orchestrator_tests.rs | Tests that refresh writes normalized cache and state files. |
| src/inventory/device.rs | Collects local host facts (hostname/OS/IPs/listeners/storage/CPU/mem). |
| src/inventory/docker.rs | Collects Docker container/service/network inventory from Docker HTTP API. |
| src/inventory/docker_tests.rs | Tests container normalization (ports/labels/domains/networks). |
| src/inventory/tailscale.rs | Collects local Tailscale identity from tailscale status --json. |
| src/inventory/tailscale_tests.rs | Tests parsing local Tailscale identity. |
| src/inventory/unifi.rs | Collects UniFi sites/devices via controller proxy endpoints. |
| src/inventory/unifi_tests.rs | Tests device normalization with optional/missing fields. |
| src/inventory/unraid.rs | Collects Unraid sections via GraphQL and normalizes system/array info. |
| src/inventory/unraid_tests.rs | Tests GraphQL error-to-warning behavior and system normalization. |
| src/inventory/media_stack.rs | Probes media service endpoints and normalizes versions/topology. |
| src/inventory/media_stack_tests.rs | Tests version extraction/normalization for media services. |
| src/inventory/raw_configs.rs | Collects and redacts raw compose/proxy configs; parses domains/ports. |
| src/inventory/raw_configs_tests.rs | Tests compose/proxy parsing outputs. |
| src/inventory/projects.rs | Discovers git repos and captures branch/head/dirty/ahead/behind/worktrees. |
| src/inventory/projects_tests.rs | Tests ahead/behind parsing and ignored-dir behavior. |
| src/cli/run.rs | Adds internal guard for inventory command dispatching. |
| src/cli/parse.rs | Adds inventory command parsing (refresh/status + --json). |
| src/cli/help.rs | Documents inventory in CLI help catalog and sectioning. |
| src/cli/help_tests.rs | Ensures help parser tokens include inventory. |
| src/cli/args.rs | Adds InventoryCommand and InventoryArgs types. |
| src/cli.rs | Implements run_inventory CLI behavior for refresh/status (json + text). |
| src/app/services/map.rs | Reworks map action to read cached inventory sections + live overlay. |
| src/app/services.rs | Updates imports to match revised map response/overlay types. |
| src/app/models/log_query.rs | Updates map request/response models for v2 schema + sections. |
| scripts/smoke-test.sh | Updates smoke assertions for map v2 fields. |
| README.md | Documents map action and new inventory refresh/status flows. |
| docs/mcp/TOOLS.md | Updates map tool documentation for cached inventory + overlay semantics. |
| docs/mcp/SCHEMA.md | Updates action listing + argument documentation for map v2. |
| docs/INVENTORY.md | Updates action descriptions and adds inventory CLI/env var docs. |
| docs/CLI.md | Adds CLI documentation for `cortex inventory refresh |
| server.json | Bumps published server version and OCI identifier to v1.10.0. |
| mcpb/manifest.json | Bumps MCP bundle version to 1.10.0. |
| CHANGELOG.md | Adds 1.10.0 release notes for native inventory + map v2. |
| Cargo.toml | Bumps crate version to 1.10.0. |
| Cargo.lock | Updates locked crate version to 1.10.0. |
| .claude-plugin/plugin.json | Bumps Claude plugin version to 1.10.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+71
to
+77
| let bytes = response.bytes().await.map_err(redacted_reqwest_error)?; | ||
| let truncated = bytes.len() > MAX_HTTP_BODY_BYTES; | ||
| let slice = if truncated { | ||
| &bytes[..MAX_HTTP_BODY_BYTES] | ||
| } else { | ||
| bytes.as_ref() | ||
| }; |
Comment on lines
+96
to
+98
| let total_hosts = log_hosts | ||
| .max(nodes.len()) | ||
| .max(inventory.as_ref().map(|i| i.nodes.len()).unwrap_or(0)); |
Comment on lines
+105
to
+115
| "--help" | "-h" => { | ||
| return Ok(CliCommand::Inventory(match command.as_str() { | ||
| "refresh" => { | ||
| super::InventoryCommand::Refresh(super::InventoryArgs { json: true }) | ||
| } | ||
| "status" => { | ||
| super::InventoryCommand::Status(super::InventoryArgs { json: true }) | ||
| } | ||
| _ => bail!("unknown inventory subcommand: {command}"), | ||
| })); | ||
| } |
| @@ -136,25 +140,46 @@ pub struct HomelabMapRequest { | |||
| pub host_limit: Option<u32>, | |||
| /// Maximum source IPs and apps attached to each host. Default 10, max 25. | |||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/mcp/SCHEMA.md (1)
22-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the action count in the schema docs.
This intro says the tool exposes 44 actions, but
src/mcp/actions.rsnow defines 45. The count now disagrees with both the registry and the table below it.🤖 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/mcp/SCHEMA.md` around lines 22 - 33, Update the schema docs intro to reflect the correct number of actions (change "44 actions" to "45 actions") so it matches the current definition in src/mcp/actions.rs (the Action enum/registry) and the table below; ensure any other mentions of the total action count in this document are updated to 45 to keep the doc consistent with the code.README.md (1)
38-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe tools list dropped three still-supported actions.
This updated section adds
map, but it no longer mentionshost_state,fleet_state, orcorrelate_statein either the prose list or the table.src/mcp/actions.rsstill registers all three, so the README now under-documents the public MCP surface.🤖 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 `@README.md` around lines 38 - 50, The README's action list and table were updated to include `map` but accidentally removed three still-registered actions (`host_state`, `fleet_state`, `correlate_state`); update the prose list and the Action table to re‑include `host_state`, `fleet_state`, and `correlate_state` so the docs match the registered actions (see the MCP action registry that registers `host_state`, `fleet_state`, and `correlate_state` for authoritative names), ensuring their short Purpose strings mirror the style of the other entries.
🤖 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 `@CHANGELOG.md`:
- Around line 10-22: The changelog footer needs updating so the new release link
is defined and Unreleased compares from the new tag: add a `[1.10.0]` footer
link that compares `v1.9.0...v1.10.0` and change the `[Unreleased]` footer to
compare `v1.10.0...HEAD`; update the existing footer references for
`[Unreleased]`, `[1.10.0]`, and the prior tag `v1.9.0` accordingly so the
rendered links point to the correct diffs.
In `@docs/CLI.md`:
- Around line 122-126: Update the CLI.md paragraph that summarizes the `refresh`
command (the "cortex inventory refresh" description) to include the raw-config
(raw config files) collector in the enumerated list of collectors; edit the
sentence that currently lists "native Rust collectors for local host facts,
Docker endpoints, redacted Compose/proxy artifacts, Unraid, Tailscale, UniFi,
media services, and configured local project roots" to also mention the
raw-config collector and preserve the existing note that missing provider
credentials are warnings and `status` reads only cache metadata.
In `@docs/mcp/SCHEMA.md`:
- Line 135: The docs entry removed per_host_limit but the model
HomelabMapRequest still exposes per_host_limit; restore per_host_limit to the
`map` arguments list in the schema docs so the documented parameters match the
implementation (include a brief description like "per-node source/app result
cap" alongside `host_limit`, `section_limit`, and `include_sections`), ensuring
the schema reference aligns with the HomelabMapRequest struct and its fields.
In `@src/inventory/device.rs`:
- Around line 1-41: Add the sidecar test module hook to this file by declaring
the test module using the cfg test/path pattern; specifically add a bottom-level
module declaration such as #[cfg(test)] #[path = "device_tests.rs"] mod tests;
so the unit tests for functions like collect, collect_ips, collect_listeners,
etc. live in the sidecar device_tests.rs file per project guidelines.
In `@src/inventory/media_stack.rs`:
- Around line 78-93: version_from_body(body) is being called twice; compute it
once into a local variable (e.g. let version = version_from_body(body)) before
building topology and the MediaService, then use that variable for both
inserting into topology and for the MediaService.version field; update
references in the block that builds topology, the MediaService instantiation
(service, base_url, status, version, topology, provenance) to use that single
computed variable so the JSON is only walked once.
- Around line 60-64: The match in headers_for is redundant: both the "overseerr"
arm and the default return "X-Api-Key". Update the match on
service.kind.as_str() (used to set header) to only special-case "plex" ->
"X-Plex-Token" and use a single default arm returning "X-Api-Key", removing the
explicit "overseerr" arm.
In `@src/inventory/orchestrator.rs`:
- Around line 129-138: The current status assignment (the status variable
computed from inventory.nodes/services/compose_projects/projects) is too
optimistic; update the logic in the status assignment block to also inspect
inventory.collectors and treat any collector with status == "failed" as a
non-success outcome. Concretely: within the same block that computes status,
first check if inventory.collectors.iter().any(|c| c.status == "failed") and if
so set status to "failed" (or "partial" per policy); otherwise keep the existing
empty-check: if all main sections are empty set "partial" else "success". Modify
the status computation around the existing inventory usage so it references
inventory.collectors and the status variable accordingly.
In `@src/inventory/raw_configs.rs`:
- Around line 227-228: Remove the dead helper function
_keep_value_and_trust_imports which exists only to silence unused-import
warnings; delete the fn _keep_value_and_trust_imports(_: BTreeMap<String,
Value>, _: TrustLevel) {} declaration and its #[allow(dead_code)] attribute,
then either remove the now-unused imports (BTreeMap, Value, TrustLevel) from the
module or, if they are actually needed by tests, move those imports into the
test module so production code has no unused imports.
- Around line 99-147: The parse_compose_project function currently uses a
fragile, indentation-based line heuristic to identify services, domains, and
ports (e.g., expects the "services:" line then service keys at exactly two-space
indentation, stops on next non-indented top-level key) and this
subset/limitations aren’t documented; update src/inventory/raw_configs.rs by
adding a clear doc comment above parse_compose_project that states the exact
supported YAML subset and assumptions (how indentation is interpreted, that
multi-line values, anchors, nested maps, flow style, and other docker-compose
features are unsupported), and mention the helper routines relied on
(extract_domainish and parse_port_line) and that a full YAML parser
(serde_yaml/yaml-rust) should be used if broader correctness is required; only
add documentation (no functional changes) unless you decide to replace the
heuristic with a YAML parser, in which case implement parsing via serde_yaml and
map service names/ports/domains from the parsed structure.
In `@src/inventory/redaction_tests.rs`:
- Line 6: The test fixture string assigned to variable input in
redaction_tests.rs contains a realistic-looking Bearer token that triggers
GitGuardian; replace the token portion with an obvious non-secret placeholder
(e.g., use repeated dummy characters like "Bearer DUMMY.DUMMY.DUMMY" or "Bearer
xxxxx.xxxxx.xxxxx") so the regex still exercises the matcher but no real-looking
secret remains, and add a GitGuardian ignore pragma comment on that line (e.g.,
a scanner-ignore/GG comment) to suppress the alert if your CI requires it;
update the string literal in the test (the input variable) and add the ignore
comment immediately above or inline with that line.
- Line 10: The assertion is ineffective because it checks for
"abcdefghijklmnopqrstuvwxyz123" which cannot appear in the input
"abc.def.ghijklmnopqrstuvwxyz123"; update the negative containment check to a
substring that actually exists in the input so the test can fail when redaction
is missing — for example replace
assert!(!out.contains("abcdefghijklmnopqrstuvwxyz123")) with
assert!(!out.contains("def.ghijklmnopqrstuvwxyz123")) (or
assert!(!out.contains("ghijklmnopqrstuvwxyz123"))) in
src/inventory/redaction_tests.rs, referencing the same out.contains(...) call so
the test validates real output redaction.
In `@src/inventory/redaction.rs`:
- Around line 90-100: The array truncation marker is never added because
take(MAX_ARRAY_ENTRIES) guarantees out.len() <= MAX_ARRAY_ENTRIES so
cap_vec(&mut out, MAX_ARRAY_ENTRIES) always returns false; change the truncation
decision to inspect the original items length (e.g., if items.len() >
MAX_ARRAY_ENTRIES) and only then push
Value::String("[TRUNCATED_ARRAY]".to_string()) after collecting redacted items
from redact_json_inner, and remove the cap_vec import/usages if they become
unused; keep using take(MAX_ARRAY_ENTRIES) to limit work but base the marker on
items.len() instead of cap_vec.
In `@src/inventory/storage.rs`:
- Around line 50-76: RefreshLock::acquire currently fails if a stale
refresh.lock remains; modify acquire to detect and recover stale locks by: when
open(create_new) fails because file exists, read the existing lock file at path
to parse an owning PID and a timestamp (write these into the lock when
creating), check if the PID is dead (on Unix use an existence/kill-0 check or
platform-appropriate API) or if the file mtime/timestamp exceeds a configurable
threshold, and if stale remove the lock and retry the create_new open
atomically; update the creation path to write PID/timestamp into the lock file
and keep the Drop impl (remove_file in Drop) unchanged, and ensure robust error
handling and logging around parse/OS checks in RefreshLock::acquire and during
the retry path.
In `@src/inventory/unifi.rs`:
- Line 28: The code currently does let headers = api_key_header("x-api-key",
api_key).unwrap_or_else(|_| HeaderMap::new()), which silently falls back to
empty headers on header-creation errors; change this to surface the failure:
when api_key_header(...) returns Err, log an explicit warning (using the
module's logger/tracing) indicating the API key/header is invalid, and then skip
making the request by returning an Err (or otherwise short-circuiting) from the
surrounding function instead of proceeding with an empty HeaderMap; locate the
header creation site (the let headers = ... expression) and the surrounding
function that returns Result to propagate the error.
In `@src/inventory/unraid.rs`:
- Around line 11-16: SECTIONS includes "docker" and "config" queries but
normalize_section only handles "system" and "array", so responses for "docker"
and "config" are fetched and ignored; either implement normalization branches
for those keys in normalize_section (e.g., handle "docker" to extract containers
-> names/image/status into your normalized model and "config" to extract valid
flag into the config model) or remove those entries from SECTIONS to avoid
unnecessary GraphQL calls; update any tests or callers that expect normalized
output from normalize_section to match the new behavior.
- Line 12: The system GraphQL query currently only requests os, version and
machineId (tuple ("system", "{ info { os version machineId } }")), so the code
path that looks up info.get("host") (used when building hostname/id in the logic
that produces unraid:{hostname}) is unreachable and you end up using machineId
as the node hostname; update the query to include a human-readable host/hostname
field (e.g., request host or hostname alongside machineId) and change the
mapping logic that builds the node id and provenance to prefer
info.get("host")/info.get("hostname") when present, falling back to machineId
only if the host field is missing (adjust the code around the info.get("host")
usage and the creation of the unraid:{hostname} id).
- Around line 95-105: In the StorageSummary creation in src/inventory/unraid.rs
(the block that builds StorageSummary from disk), fix the field mappings and
harden size decoding: set mount to the disk's mountpoint (try
disk.get("mountpoint").and_then(Value::as_str).map(ToString::to_string) and fall
back to the existing name) and set fs_type from a filesystem field (try
disk.get("filesystem") / disk.get("fstype") / disk.get("fsType") and fall back
to disk.get("status") only if none exist) instead of swapping them, and change
total_bytes to accept both numeric and string BigInt encodings by first
attempting disk.get("size").and_then(Value::as_u64) and if None then
disk.get("size").and_then(Value::as_str).and_then(|s| s.parse::<u64>().ok());
keep provenance(url, section) as-is.
---
Outside diff comments:
In `@docs/mcp/SCHEMA.md`:
- Around line 22-33: Update the schema docs intro to reflect the correct number
of actions (change "44 actions" to "45 actions") so it matches the current
definition in src/mcp/actions.rs (the Action enum/registry) and the table below;
ensure any other mentions of the total action count in this document are updated
to 45 to keep the doc consistent with the code.
In `@README.md`:
- Around line 38-50: The README's action list and table were updated to include
`map` but accidentally removed three still-registered actions (`host_state`,
`fleet_state`, `correlate_state`); update the prose list and the Action table to
re‑include `host_state`, `fleet_state`, and `correlate_state` so the docs match
the registered actions (see the MCP action registry that registers `host_state`,
`fleet_state`, and `correlate_state` for authoritative names), ensuring their
short Purpose strings mirror the style of the other entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4285e022-ac70-4448-8a8c-1bcd73f7774c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lockand included by**/*
📒 Files selected for processing (56)
.claude-plugin/plugin.jsonCHANGELOG.mdCargo.tomlREADME.mddocs/CLI.mddocs/INVENTORY.mddocs/mcp/SCHEMA.mddocs/mcp/TOOLS.mdmcpb/manifest.jsonscripts/smoke-test.shserver.jsonsrc/app/models/log_query.rssrc/app/services.rssrc/app/services/map.rssrc/cli.rssrc/cli/args.rssrc/cli/help.rssrc/cli/help_tests.rssrc/cli/parse.rssrc/cli/run.rssrc/inventory/cache.rssrc/inventory/cache_tests.rssrc/inventory/collectors.rssrc/inventory/config.rssrc/inventory/device.rssrc/inventory/docker.rssrc/inventory/docker_tests.rssrc/inventory/http.rssrc/inventory/limits.rssrc/inventory/media_stack.rssrc/inventory/media_stack_tests.rssrc/inventory/mod.rssrc/inventory/orchestrator.rssrc/inventory/orchestrator_tests.rssrc/inventory/process.rssrc/inventory/process_tests.rssrc/inventory/projects.rssrc/inventory/projects_tests.rssrc/inventory/raw_configs.rssrc/inventory/raw_configs_tests.rssrc/inventory/redaction.rssrc/inventory/redaction_tests.rssrc/inventory/schema.rssrc/inventory/storage.rssrc/inventory/storage_tests.rssrc/inventory/tailscale.rssrc/inventory/tailscale_tests.rssrc/inventory/unifi.rssrc/inventory/unifi_tests.rssrc/inventory/unraid.rssrc/inventory/unraid_tests.rssrc/lib.rssrc/main.rssrc/main_tests.rssrc/mcp/actions.rssrc/mcp/tools_tests.rs
Comment on lines
+122
to
+126
| `refresh` runs native Rust collectors for local host facts, Docker endpoints, | ||
| redacted Compose/proxy artifacts, Unraid, Tailscale, UniFi, media services, and | ||
| configured local project roots. Missing provider credentials are warnings, not | ||
| fatal errors for unrelated collectors. `status` reads only the cache metadata | ||
| and does not open SQLite. |
There was a problem hiding this comment.
Include the raw-config collector in this command summary.
This paragraph lists the new inventory collectors, but it omits the raw config files collector called out in the PR scope. That makes cortex inventory refresh sound narrower than the feature actually is.
🤖 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/CLI.md` around lines 122 - 126, Update the CLI.md paragraph that
summarizes the `refresh` command (the "cortex inventory refresh" description) to
include the raw-config (raw config files) collector in the enumerated list of
collectors; edit the sentence that currently lists "native Rust collectors for
local host facts, Docker endpoints, redacted Compose/proxy artifacts, Unraid,
Tailscale, UniFi, media services, and configured local project roots" to also
mention the raw-config collector and preserve the existing note that missing
provider credentials are warnings and `status` reads only cache metadata.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
22 issues found across 57 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/app/services/map.rs">
<violation number="1" location="src/app/services/map.rs:96">
P1: `summary.hosts` uses `max(...)` instead of host-union semantics, which can underreport total hosts and misstate truncation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ); | ||
| let collection_errors = merge_cache_warnings(cache_status.clone(), collection_errors); | ||
|
|
||
| let total_hosts = log_hosts |
There was a problem hiding this comment.
P1: summary.hosts uses max(...) instead of host-union semantics, which can underreport total hosts and misstate truncation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/app/services/map.rs, line 96:
<comment>`summary.hosts` uses `max(...)` instead of host-union semantics, which can underreport total hosts and misstate truncation.</comment>
<file context>
@@ -1,293 +1,286 @@
+ );
+ let collection_errors = merge_cache_warnings(cache_status.clone(), collection_errors);
+
+ let total_hosts = log_hosts
+ .max(nodes.len())
+ .max(inventory.as_ref().map(|i| i.nodes.len()).unwrap_or(0));
</file context>
jmagar
force-pushed
the
feat/homelab-inventory-map
branch
6 times, most recently
from
June 3, 2026 23:18
095c484 to
09d4ff6
Compare
There was a problem hiding this comment.
Actionable comments posted: 20
♻️ Duplicate comments (1)
src/inventory/unraid.rs (1)
10-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequest the disk fields that
normalize_sectionactually consumes.The
arrayquery only asks forname,status, andsize, but the normalizer readsmountpoint/mountandfilesystem/fstype/fsType. As written, every disk falls back tomount = nameandfs_type = status, so the cached storage metadata is wrong.🩹 Minimal fix
const SECTIONS: &[(&str, &str)] = &[ ("system", "{ info { host os version machineId } }"), - ("array", "{ array { state disks { name status size } } }"), + ( + "array", + "{ array { state disks { name status size mount mountpoint filesystem fstype fsType } } }", + ), ];🤖 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/inventory/unraid.rs` around lines 10 - 13, The GraphQL SECTIONS constant's "array" query is missing disk fields that normalize_section expects, causing mount and fs_type to be populated from the wrong fields; update the tuple for "array" in SECTIONS to request the disk fields normalize_section reads (mountpoint and/or mount, and filesystem and/or fstype/fsType) in addition to name, status, and size so normalize_section (and any helpers it calls) receive real mount and filesystem data rather than falling back to name/status.
🤖 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 `@docs/mcp/TOOLS.md`:
- Around line 98-105: Update the action summary table entry for the "map" action
so it matches the detailed description: state that "map" returns a bounded
cached inventory snapshot from ~/.cortex/inventory overlaid with live
host/heartbeat data (read-only, never triggers refresh), omits raw Compose/proxy
artifact bodies by default, requires action = "map", and supports optional
host_limit (default 100, max 500), section_limit (default 100, max 250) and
include_sections; replace the old host/source/app/heartbeat wording in the
overview row so the summary and the detailed paragraph are consistent.
In `@README.md`:
- Around line 38-40: Update README.md to use the correct MCP tool name "cortex"
everywhere the examples reference a tool; specifically replace occurrences where
examples set the JSON "name" field to "syslog" (or any other outdated tool name)
so they use "cortex" instead, and ensure any verification or example payloads
and accompanying text (e.g., the verification examples later in the file) are
consistent with the list of valid actions and the single exposed tool "cortex".
- Around line 87-99: Add a short note in the "Homelab Inventory" section
explaining cold-start behavior: when the normalized cache doesn't exist the MCP
`map` action and `cortex inventory status` will report cache_status: "missing"
until the first `cortex inventory refresh` completes; mention that running
`cortex inventory refresh --json` seeds `~/.cortex/inventory` (producing
normalized/homelab.json, collection-state.json, raw/<run_id>/...) and that users
should run that command on first run to clear the "missing" status.
In `@src/app/services/map.rs`:
- Around line 35-117: The code eagerly clones full cached vectors when calling
inventory.as_ref().map(|i| i.<section>.clone()) for each section (e.g.,
services, compose_projects, networks, etc.), causing unnecessary allocations
before section_limit is applied; change the calls so section() receives an
Option<&[T]> or a lazily-cloned iterator instead of owned Vecs (e.g., pass
inventory.as_ref().map(|i| i.services.as_slice()) or a closure that clones up to
section_limit inside section()), update the section() signature/usage to cap and
clone only the needed items (apply section_limit before cloning), and likewise
adjust merge_cache_warnings/collection_errors handling so cache entries are
sliced or lazily cloned before cap_vec and truncation checks to avoid
full-vector copies.
In `@src/cli/help.rs`:
- Around line 288-295: The inventory namespace only defines a top-level
CommandDoc, so classify_help can't resolve subcommands; add explicit nested help
entries for the two subcommands (e.g., CommandDoc entries with names "inventory
refresh" and "inventory status" or entries keyed into NESTED_CATALOG) so
classify_help/NESTED_CATALOG can return the exact help for "cortex inventory
refresh --help" and "cortex inventory status --help"; ensure each nested
CommandDoc includes the proper summary and usage arrays matching the examples
already present.
In `@src/inventory/cache.rs`:
- Around line 43-58: The status path currently calls
read_json::<HomelabInventory>(&paths.normalized_json) which deserializes the
entire inventory; change it to deserialize a tiny metadata-only view (e.g.,
define a small struct like InventoryMetadata with generated_at: String and
freshness: { stale_after_secs: usize } or equivalent) and call
read_json::<InventoryMetadata>(&paths.normalized_json) in the inventory_status
logic, then compute generated_at, age_seconds and is_stale from those metadata
fields (keep the same DateTime::parse_from_rfc3339, Utc::now and Duration logic
and the stale_after conversion/unwrap behavior). Update any variable
names/references (generated_at, age_seconds, is_stale, status) to use the
metadata struct instead of HomelabInventory so status no longer depends on full
inventory shape.
In `@src/inventory/config.rs`:
- Around line 9-33: Remove automatic Debug derives that can leak secrets: delete
#[derive(Debug, Clone)] from MediaServiceConfig (and from InventoryConfig if you
want to avoid showing its unraid_api_key/unifi_api_key) and either leave Debug
off or implement a manual Debug impl that redacts sensitive fields (api_key,
password, unraid_api_key, unifi_api_key). Locate the structs InventoryConfig and
MediaServiceConfig and ensure any logging or formatting using {:?} will not
include secret fields; if debug output is required, provide a custom Debug impl
for MediaServiceConfig that prints non-sensitive fields and replaces
api_key/password with redacted placeholders.
In `@src/inventory/docker.rs`:
- Around line 73-80: The loop currently pushes a new NetworkSegment for every
(network, container) pair causing duplicates; change it to aggregate members by
network name: build a map keyed by network (using the existing networks
variable) that collects members (the name variable) and provenance(host) once,
then after the loop convert each map entry into a single NetworkSegment with
kind "docker" and push into out.networks; update any existing code that
constructs NetworkSegment so you only create one per network and append members
to that segment instead of pushing inside the per-container loop.
In `@src/inventory/http.rs`:
- Around line 24-114: Add the missing sidecar test hook by declaring the
standard test module at the bottom of this file (e.g. add #[cfg(test)] #[path =
"http_tests.rs"] mod tests;), then create the sidecar http_tests.rs that does
use super::*; and implements unit tests covering HttpProbe
(get_json/post_json/read_json truncation), api_key_header, and
redacted_reqwest_error error-paths and redaction behavior so private items are
testable.
In `@src/inventory/orchestrator.rs`:
- Around line 144-153: The refresh status logic only considers nodes, services,
compose_projects, and projects when computing has_output, so inventories that
only populate media_services, networks, storage, or reverse_proxies are
misclassified as "partial"; update the has_output expression in orchestrator.rs
to include inventory.media_services, inventory.networks, inventory.storage, and
inventory.reverse_proxies (so has_output becomes true when any of those
collections is non-empty) while leaving has_collection_errors/status logic
unchanged, and add a regression test that runs a refresh which only populates
media_services (or another single section) to assert the resulting status is
"success" when collection_errors is empty.
In `@src/inventory/process.rs`:
- Around line 85-86: shell_words currently uses split_whitespace which breaks
quoted and escaped args; replace it with a real shell-style parser: change the
signature of shell_words(input: &str) -> Vec<&str> to shell_words(input: &str)
-> Vec<String> and implement a small state machine that iterates the input,
supports single quotes (literal), double quotes (allow backslash escapes for \"
\\ etc.), backslash escapes outside quotes, accumulates characters into the
current token, pushes tokens at unquoted whitespace, and returns owned strings
(preserving spaces inside quotes and removing quote characters); update any
callers to accept Vec<String>. Alternatively, if you prefer not to implement
parsing, rename the function to indicate whitespace-only splitting so callers
don’t assume shell semantics.
In `@src/inventory/projects_tests.rs`:
- Around line 24-40: The test unconditionally expects "real" in the repo list
though that directory is only created inside the #[cfg(unix)] block; update the
assertion logic in the test (around discover_repos, repos, names) to account for
non-Unix platforms by computing the expected vector conditionally—e.g., build
expected_names based on whether the symlink/real setup ran (check cfg!(unix) or
a boolean set inside the #[cfg(unix)] block) and then assert_eq!(names,
expected_names) and assert!(!names.contains(&"link-repo")) as before; ensure you
reference the same variables discover_repos, repos, names, and the
"real"/"link-repo" identifiers.
In `@src/inventory/raw_configs.rs`:
- Around line 153-168: The bug is that parse_port_line(trimmed) runs for every
line and picks up numeric-looking scalars; restrict port parsing to actual YAML
ports lists by tracking when we're inside a ports block (e.g., maintain an
in_ports boolean set when encountering a "ports:" key at the current indentation
and cleared when indentation decreases or a new sibling section starts) and only
call parse_port_line for lines that are list items under that block (e.g., lines
starting with "-" or the expected indentation for ports). Update the logic
around the existing in_services check and the parse_port_line call so
ports.push(...) is executed only when in_ports is true; keep references to
parse_port_line, extract_domainish, services, domains, and ports to locate and
modify the block.
In `@src/inventory/redaction.rs`:
- Around line 154-155: The two Regex::new(...) patterns in
src/inventory/redaction.rs currently only match unquoted secret values and
therefore miss YAML/TOML/JSON-style quoted forms; update those two regexes to
also accept and redact values wrapped in single or double quotes (e.g. allow
optional surrounding quotes around the captured secret token) so patterns like
token: "abc", password='abc', or api_key = "abc123" are covered, and add
regression tests that assert redaction for examples such as `token:
"quoted-secret"` and `password='quoted-secret'` to prevent future regressions.
In `@src/inventory/storage_tests.rs`:
- Around line 43-53: The test uses a GNU-only external command
(Command::new("touch").args(["-d","`@1`"])) which breaks non-Linux Unixes; replace
the external touch invocation with a portable API call to set the file's
modification time (e.g. use the filetime crate). Concretely, after creating the
file at path, call filetime::set_file_mtime(&path,
filetime::FileTime::from_unix_time(1, 0)) (or equivalent
filetime::set_file_times) before acquiring RefreshLock::acquire(&path) so the
test is portable across macOS/BSD/Linux; add filetime to dev-dependencies if not
already present.
In `@src/inventory/storage.rs`:
- Around line 123-146: The temp file is created with default permissions then
written to, allowing a window where bytes may be world-readable; in
write_private_atomic you should create the temp file with 0600 immediately (use
OpenOptions::new().write(true).create_new(true).mode(0o600).open(&tmp) on Unix
via std::os::unix::fs::OpenOptionsExt or the platform-appropriate equivalent) so
permissions are restrictive before write_all; keep the subsequent
chmod_private_file(path) and error-cleanup logic unchanged and ensure
compilation guards for non-Unix platforms if needed.
In `@src/inventory/unifi.rs`:
- Around line 77-85: The current fallback unwrap_or("unifi-device") causes many
distinct records to get the same synthetic ID because id is built with
format!("unifi:{hostname}"); change the logic in the block that builds
InventoryNode (the hostname variable and the id creation) to prefer a stable
unique field (e.g., a raw "mac" or "id" value from item via
item.get("mac").and_then(Value::as_str) or
item.get("id").and_then(Value::as_str)) and only fall back to a synthetic value
if you can incorporate a per-record unique piece (e.g.,
"unifi:unknown-{index-or-uuid}"); if no unique field exists, skip pushing the
node and emit a warning (use the crate's logger such as log::warn or
tracing::warn) mentioning the missing keys so downstream consumers won't receive
duplicate "unifi:unifi-device" IDs.
- Around line 57-63: Before you call items.iter().take(200) check the full
collection size (the Vec produced by
body.get("data").and_then(Value::as_array).or_else(||
body.as_array()).cloned().unwrap_or_default()) and if items.len() > 200 emit a
clear warning/error (e.g. a logger.warn or logger.error) that includes the total
count and that the response will be truncated, and also set a "truncated"
signal/flag on the inventory record being persisted so downstream consumers know
it was clipped; keep the existing slicing (items.iter().take(200)) but only
after logging and setting the truncated indicator.
In `@src/mcp/tools_tests.rs`:
- Around line 190-198: The test is flaky because homelab_map() reads
InventoryConfig::from_env() and may see a real ~/.cortex/inventory; update
test_state_with_token() to create an isolated temp inventory directory (e.g. via
tempfile::tempdir()) and set CORTEX_INVENTORY_DIR to that path
(std::env::set_var) before calling homelab_map()/starting the service so the
assertions on value["cache_status"] == "missing" and the cache-warning in
collection_errors are deterministic; ensure the TempDir lives for the duration
of the test so it is cleaned up afterward.
In `@tests/test_live.sh`:
- Around line 526-532: Add assertions in the test block using the existing
assert_jq helper and the same "${map_result}" variable to validate that the
remaining required top-level fields in HomelabMapResponse are present and are
arrays: 'services', 'compose_projects', 'reverse_proxies', 'networks',
'storage', 'media_services', and 'projects'. For each field call assert_jq with
a descriptive message like "cortex map — <field> field is array" and test the
JSON path '.<field> | type' equals "array", mirroring the other checks (e.g.,
the existing '.nodes | type' "array" assertion).
---
Duplicate comments:
In `@src/inventory/unraid.rs`:
- Around line 10-13: The GraphQL SECTIONS constant's "array" query is missing
disk fields that normalize_section expects, causing mount and fs_type to be
populated from the wrong fields; update the tuple for "array" in SECTIONS to
request the disk fields normalize_section reads (mountpoint and/or mount, and
filesystem and/or fstype/fsType) in addition to name, status, and size so
normalize_section (and any helpers it calls) receive real mount and filesystem
data rather than falling back to name/status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 83161620-abd6-4fa7-a4a8-e9ea75174180
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lockand included by**/*
📒 Files selected for processing (62)
.claude-plugin/plugin.jsonCHANGELOG.mdCargo.tomlREADME.mddocs/CLI.mddocs/INVENTORY.mddocs/mcp/SCHEMA.mddocs/mcp/TOOLS.mdmcpb/manifest.jsonscripts/smoke-test.shserver.jsonsrc/app/models/log_query.rssrc/app/services.rssrc/app/services/map.rssrc/app/services/map_tests.rssrc/cli.rssrc/cli/args.rssrc/cli/help.rssrc/cli/help_tests.rssrc/cli/parse.rssrc/cli/parse_tests.rssrc/cli/run.rssrc/inventory/cache.rssrc/inventory/cache_tests.rssrc/inventory/collectors.rssrc/inventory/config.rssrc/inventory/device.rssrc/inventory/device_tests.rssrc/inventory/docker.rssrc/inventory/docker_tests.rssrc/inventory/http.rssrc/inventory/limits.rssrc/inventory/limits_tests.rssrc/inventory/media_stack.rssrc/inventory/media_stack_tests.rssrc/inventory/mod.rssrc/inventory/orchestrator.rssrc/inventory/orchestrator_tests.rssrc/inventory/process.rssrc/inventory/process_tests.rssrc/inventory/projects.rssrc/inventory/projects_tests.rssrc/inventory/raw_configs.rssrc/inventory/raw_configs_tests.rssrc/inventory/redaction.rssrc/inventory/redaction_tests.rssrc/inventory/schema.rssrc/inventory/storage.rssrc/inventory/storage_tests.rssrc/inventory/tailscale.rssrc/inventory/tailscale_tests.rssrc/inventory/unifi.rssrc/inventory/unifi_tests.rssrc/inventory/unraid.rssrc/inventory/unraid_tests.rssrc/lib.rssrc/main.rssrc/main_tests.rssrc/mcp/actions.rssrc/mcp/tools_tests.rstests/mcporter/test-tools.shtests/test_live.sh
jmagar
force-pushed
the
feat/homelab-inventory-map
branch
from
June 3, 2026 23:43
09d4ff6 to
be45951
Compare
jmagar
force-pushed
the
feat/homelab-inventory-map
branch
from
June 3, 2026 23:57
be45951 to
f80a950
Compare
This was referenced Jun 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Beads
Validation
Summary by cubic
Adds a native homelab inventory with a private cache and a local-only
cortex inventoryCLI. Upgrades MCPaction=maptocortex.homelab_map.v2, which reads the cache and overlays bounded live host/heartbeat data without triggering refresh; ships inv1.10.0.New Features
~/.cortex/inventorywith normalized JSON (cortex.homelab_inventory.v1), collection state, and redacted raw artifacts; CLI:cortex inventory refreshandcortex inventory status(local-only; rejects--http/--server/--token;statusreads cache metadata only).mapnow returnscortex.homelab_map.v2, reads the cache, and adds a bounded live Cortex overlay; supportshost_limit,section_limit, andinclude_sections; deprecatesper_host_limit(ignored; surfaced as a request warning); removesinventory_sources; omits raw bodies by default; new fields:cache_status,artifact_refs,collection_errors,cortex_overlay.reqwest), stricter redaction, API key header validation, symlink-safe private writes, and a refresh concurrency lock.Migration
cortex inventory refreshonce to seed the cache;mapreportscache_status: "missing"until then.cortex.homelab_map.v2and new fields; stop relying onper_host_limitandinventory_sources.Written for commit f80a950. Summary will update on new commits.
Summary by CodeRabbit
New Features
cortex inventory refreshandcortex inventory statusCLI commands that write/read a private local inventory cache (~/.cortex/inventory).mapaction to v2: returns a cached homelab inventory overlaid with bounded live host/heartbeat data; supportssection_limitandinclude_sectionsfiltering.Documentation
mapv2 semantics.