fix: remediate full-review findings - #70
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThis PR adds CI/release checks and scripts, rebrands docs to cortex, implements SshOptions/SshContext and wires it into inventory collectors and deploy, refactors inventory projection into plan/apply, binds SQL LIMITs and splits patterns fetch/clustering, centralizes MCP typed request parsing and schema tightening, tightens request-model deserialization, extends runtime observability, and updates/adds tests. ChangesRelease & CI
Documentation & Policy
Models & CLI
SSH & Deploy
Inventory & Orchestration
Graph Projection
DB Queries & Patterns
MCP & Schemas
Observability & Runtime
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/mcp/DEPLOY.md (1)
173-173:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale syslog-named SWAG config reference.
Line 173 still points to
docs/syslog.subdomain.conf, which appears inconsistent with the cortex rebrand and can mislead operators looking for current deployment docs.🤖 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/DEPLOY.md` at line 173, Update the stale reference in DEPLOY.md that points to docs/syslog.subdomain.conf and the example URL https://cortex.tootie.tv/mcp: locate the string "docs/syslog.subdomain.conf" and replace it with the current SWAG/nginx config filename used by the project (e.g., the repo's current SWAG config file), and update or remove the "https://cortex.tootie.tv/mcp" example to the correct, non-rebranded domain or a neutral placeholder; ensure the text around the reference still directs operators to the working nginx/SWAG config (adjust the filename in the same sentence where docs/syslog.subdomain.conf appears).src/inventory/orchestrator.rs (1)
171-182:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPer-collector timeouts are never classified as
skipped.
collector_task()emits the timeout marker as"timeout", butrun_collector()only recognizes"collection_timeout". Any collector that hits its individual deadline will therefore fall through to"partial"instead of"skipped".Proposed fix
-type NamedOutput = (&'static str, String, String, u128, CollectorOutput); +type NamedOutput = (&'static str, String, String, u128, bool, CollectorOutput); - ( - name, - now.clone(), - now, - config.collection_deadline.as_millis(), - output, - ) + ( + name, + now.clone(), + now, + config.collection_deadline.as_millis(), + true, + output, + ) - let out = match tokio::time::timeout(deadline, future).await { - Ok(output) => output, - Err(_) => timeout_output(name, deadline), - }; + let (timed_out, out) = match tokio::time::timeout(deadline, future).await { + Ok(output) => (false, output), + Err(_) => (true, timeout_output(name, deadline)), + }; ( name, started, Utc::now().to_rfc3339(), t.elapsed().as_millis(), + timed_out, out, ) - let (name, started_at, finished_at, elapsed_ms, output) = result; + let (name, started_at, finished_at, elapsed_ms, timed_out, output) = result; warnings.extend(output.warnings.iter().cloned()); - let status = if output - .errors - .iter() - .any(|e| e.phase == "collection_timeout") - { + let status = if timed_out { "skipped" } else if output.errors.iter().any(|e| e.severity == "error") { "failed"Also applies to: 271-274, 300-306
🤖 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/orchestrator.rs` around lines 171 - 182, The per-collector timeout marker mismatch causes timed collectors to be classified as "partial" because collector_task emits "timeout" while run_collector expects "collection_timeout"; update run_collector (the code paths that inspect CollectorOutput warnings/markers) to treat either "timeout" or "collection_timeout" as the timeout/skipped case (or normalize incoming warnings by mapping "timeout" -> "collection_timeout") so that COLLECTOR_NAMES iteration and CollectorOutput::warn("collection_timeout", ...) semantics correctly classify those collectors as skipped; apply the same change in the other similar branches that inspect collector warnings (the other occurrences noted around where run_collector handles per-collector results).
🤖 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 `@deny.toml`:
- Around line 55-58: The policy currently sets the cargo-deny wildcards policy
to "allow" via the wildcards setting; change this to "warn" so wildcard
dependency requirements are detected and surfaced (i.e., update the wildcards =
"allow" entry to wildcards = "warn"), keeping the git dep pin behavior intact
while restoring warning-level detection for broad/wildcard specs.
In `@docs/plugin/HOOKS.md`:
- Line 24: Update the inconsistent file path references in HOOKS.md so they
match: decide whether the hooks file lives at "plugins/cortex/hooks/hooks.json"
or "plugins/hooks/hooks.json", then change the other occurrence to the chosen
path (update the sentence "Hooks are registered in
`plugins/cortex/hooks/hooks.json`" or the "File location" tree entries
referencing `plugins/hooks/hooks.json`) so both references are identical and
correct.
In `@docs/plugin/SKILLS.md`:
- Line 41: Update the "Directory structure" section in the SKILLS.md
documentation (around lines 17-18) to align with the new path structure. Replace
references to `plugins/skills/syslog/` with the new `plugins/cortex/skills/`
path pattern to maintain consistency with the updated examples shown elsewhere
in the document at lines 41, 52, and 59. Ensure all directory examples in the
structure block reflect the current plugin directory organization.
In `@scripts/check-plugin-manifest-versions.sh`:
- Around line 25-36: Invert the Python exit logic inside the heredoc so the
script returns non-zero when a top-level "version" key is present (and zero
otherwise); specifically change the current sys.exit(0 if "version" in payload
else 1) to sys.exit(1 if "version" in payload else 0) inside the Python block of
the check-plugin-manifest-versions.sh heredoc so the shell if ... then condition
becomes direct (if python fails then print the FAIL message and set status=1).
In `@src/app/services/analytics.rs`:
- Around line 123-137: The clustering work is happening after awaiting
self.run_db which can block executor threads; move the db::cluster_pattern_rows
call and construction of PatternsResponse into the closure passed to run_db so
clustering runs on the DB worker thread. Specifically, inside the closure given
to run_db("patterns_fetch", ...) perform db::fetch_pattern_rows then call
db::cluster_pattern_rows(rows, top_n) and return the PatternsResponse (or the
tuple needed) from the closure so the await receives the already-clustered
result instead of running cluster_pattern_rows after the await.
In `@src/db/graph_inventory.rs`:
- Around line 596-603: The current token filtering in the prefix extraction uses
starts_with("http") which incorrectly excludes tokens like "http-api"; change
the predicate in the closure used after upstream.split(...) so it only rejects
exact URL scheme tokens ("http" or "https") instead of anything starting with
"http" — e.g. replace part.starts_with("http") with a check like part == "http"
|| part == "https". This affects the block that computes prefix and calls
canonical_or_raw and match_service_name_key (variables/functions: upstream,
prefix, canonical_or_raw, match_service_name_key, normalized, source, services).
In `@src/deploy.rs`:
- Around line 643-666: Move the inline unit tests
(remote_deploy_rejects_option_like_hosts_before_running_ssh and
remote_deploy_accepts_safe_hosts) out of the deploy module into a sidecar test
file named deploy_tests.rs and keep only the test-module hook in the source;
update references so the tests still use FakeRemoteRunner and
run_remote_deploy_with_runner from the deploy module, and replace the inline
tests block with the single hook: #[cfg(test)] #[path = "deploy_tests.rs"] mod
tests; ensuring the new deploy_tests.rs contains the original test functions and
any necessary use/imports.
In `@src/runtime/inventory_refresh.rs`:
- Around line 206-209: spawn_remote_docker_event_tasks /
run_remote_docker_events_once currently acquire_owned() a semaphore permit and
hold it for the entire lifetime of the remote `docker events` process, which
starves other hosts; change the logic so the semaphore is only held while
initiating the SSH/command (e.g., acquire_owned() in
run_remote_docker_events_once just long enough to open the SSH session and spawn
the remote `docker events` reader) and then drop the owned permit before
awaiting the long-lived stream; specifically modify
run_remote_docker_events_once and the task started by
spawn_remote_docker_event_tasks to release the permit (drop the
OwnedSemaphorePermit) immediately after the child/process is started (or use
try_acquire with timeout if startup may block), so SshContext/SSH setup uses the
permit briefly but the long-lived event loop does not hold it.
---
Outside diff comments:
In `@docs/mcp/DEPLOY.md`:
- Line 173: Update the stale reference in DEPLOY.md that points to
docs/syslog.subdomain.conf and the example URL https://cortex.tootie.tv/mcp:
locate the string "docs/syslog.subdomain.conf" and replace it with the current
SWAG/nginx config filename used by the project (e.g., the repo's current SWAG
config file), and update or remove the "https://cortex.tootie.tv/mcp" example to
the correct, non-rebranded domain or a neutral placeholder; ensure the text
around the reference still directs operators to the working nginx/SWAG config
(adjust the filename in the same sentence where docs/syslog.subdomain.conf
appears).
In `@src/inventory/orchestrator.rs`:
- Around line 171-182: The per-collector timeout marker mismatch causes timed
collectors to be classified as "partial" because collector_task emits "timeout"
while run_collector expects "collection_timeout"; update run_collector (the code
paths that inspect CollectorOutput warnings/markers) to treat either "timeout"
or "collection_timeout" as the timeout/skipped case (or normalize incoming
warnings by mapping "timeout" -> "collection_timeout") so that COLLECTOR_NAMES
iteration and CollectorOutput::warn("collection_timeout", ...) semantics
correctly classify those collectors as skipped; apply the same change in the
other similar branches that inspect collector warnings (the other occurrences
noted around where run_collector handles per-collector results).
🪄 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: 6e0f21cd-e4d0-4301-8df4-aea2e8dca3e0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lockand included by**/*
📒 Files selected for processing (74)
.github/workflows/ci.ymlAGENTS.mdAGENTS.mdCHANGELOG.mdCLAUDE.mdCargo.tomlREADME.mdconfig/mcporter.jsondeny.tomldocs/CLI.mddocs/CONFIG.mddocs/INVENTORY.mddocs/OAUTH.mddocs/README.mddocs/RELEASE.mddocs/RUST.mddocs/SECURITY.mddocs/contracts/mcp-actions-current.mddocs/mcp/CONNECT.mddocs/mcp/DEPLOY.mddocs/mcp/DEV.mddocs/mcp/ELICITATION.mddocs/mcp/MCPORTER.mddocs/mcp/PATTERNS.mddocs/mcp/PUBLISH.mddocs/mcp/SCHEMA.mddocs/mcp/TESTS.mddocs/mcp/TOOLS.mddocs/mcp/TRANSPORT.mddocs/mcp/WEBMCP.mddocs/plugin/AGENTS.mddocs/plugin/AGENTS.mddocs/plugin/CLAUDE.mddocs/plugin/HOOKS.mddocs/plugin/MARKETPLACES.mddocs/plugin/PLUGINS.mddocs/plugin/SKILLS.mddocs/repo/SCRIPTS.mddocs/runbooks/deploy.mdmcpb/manifest.jsonscripts/check-agent-memory-symlinks.shscripts/check-plugin-manifest-versions.shscripts/check-public-identity.shserver.jsonsrc/app/models/ai_incidents.rssrc/app/models/ai_sessions.rssrc/app/models/context.rssrc/app/models/ops.rssrc/app/models/stats.rssrc/app/services/analytics.rssrc/db.rssrc/db/analytics.rssrc/db/graph_inventory.rssrc/db/graph_inventory/sql.rssrc/db/graph_inventory_tests.rssrc/db/queries.rssrc/db/queries_tests.rssrc/deploy.rssrc/inventory.rssrc/inventory/orchestrator.rssrc/inventory/orchestrator_tests.rssrc/inventory/raw_configs.rssrc/inventory/remote_configs.rssrc/inventory/remote_device.rssrc/inventory/remote_docker.rssrc/inventory/ssh.rssrc/inventory/ssh_tests.rssrc/mcp/actions.rssrc/mcp/rmcp_server.rssrc/mcp/rmcp_server_tests.rssrc/mcp/tools.rssrc/mcp/tools_tests.rssrc/runtime/inventory_refresh.rssrc/runtime/inventory_refresh_tests.rs
Collaborator
Author
|
Review Toolkit follow-up pushed to Commits:
Findings addressed:
Local verification on final head:
PR status after push:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
CHANGELOG.md (1)
10-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate release compare links to include 1.14.2.
Line 10 adds
1.14.2, but the footer still points[Unreleased]tov1.14.0...HEADand is missing1.14.2/1.14.1link definitions, so release navigation is stale.♻️ Suggested update
-[Unreleased]: https://github.com/jmagar/cortex/compare/v1.14.0...HEAD +[Unreleased]: https://github.com/jmagar/cortex/compare/v1.14.2...HEAD +[1.14.2]: https://github.com/jmagar/cortex/compare/v1.14.1...v1.14.2 +[1.14.1]: https://github.com/jmagar/cortex/compare/v1.14.0...v1.14.1 [1.14.0]: https://github.com/jmagar/cortex/compare/v1.13.3...v1.14.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` at line 10, The footer link definitions are stale after adding the header "## [1.14.2] - 2026-06-07": update the release compare links so [Unreleased] points to v1.14.2...HEAD (not v1.14.0...HEAD) and add link definitions for [1.14.2] and [1.14.1] with the correct GitHub compare ranges (e.g. v1.14.1...v1.14.2 and v1.14.0...v1.14.1 or your repo's equivalent tags), ensuring the labels in the footer match the header "[1.14.2] - 2026-06-07" and previous release names.docs/plugin/CONFIG.md (1)
34-34: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winClarify the duplicated
CORTEX_*/CORTEX_*reference.The phrase "exports current Claude Code
userConfigvalues asCORTEX_*/CORTEX_*environment variables" contains unclear duplication. Line 41 refers only to "CORTEX_*overrides" without the slash-separated repetition. If both fragments refer to the same variable pattern, remove the duplication; if they represent distinct sets, clarify the distinction.🤖 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/plugin/CONFIG.md` at line 34, The sentence that says "exports current Claude Code `userConfig` values as `CORTEX_*` / `CORTEX_*` environment variables" is ambiguous due to the duplicated `CORTEX_*` reference; update the docs text so it clearly states whether a single pattern (`CORTEX_*`) is used or two distinct patterns are meant. Locate the phrase referencing `userConfig` and `CORTEX_*` in CONFIG.md and either remove the duplicate slash form to read "exports current Claude Code `userConfig` values as `CORTEX_*` environment variables" or replace one side of the slash with the correct distinct pattern (and ensure consistency with the later "CORTEX_* overrides" mention). Ensure the final wording matches the actual variable pattern used by the codebase.docs/plugin/HOOKS.md (1)
34-34: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winClarify the duplicated
CORTEX_*/CORTEX_*reference.The phrase "exports current Claude Code
userConfigvalues asCORTEX_*/CORTEX_*environment variables" contains the same unclear duplication found in CONFIG.md. Consider removing the repetition or clarifying if the slash represents two distinct variable sets.🤖 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/plugin/HOOKS.md` at line 34, The sentence in HOOKS.md that reads "exports current Claude Code `userConfig` values as `CORTEX_*` / `CORTEX_*` environment variables" duplicates the same token and is unclear; edit that line to either remove the repeated `CORTEX_*` so it reads "as `CORTEX_*` environment variables" or explicitly clarify what the slash means (e.g., "`CORTEX_*` (build-time) / `CORTEX_*` (runtime)" or list the two distinct variable prefixes), updating the phrase that references `userConfig` to match the chosen wording so the intent is unambiguous.src/mcp/tools_tests.rs (1)
594-611: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider tightening the assertion to verify action name in error.
The test verifies that type mismatches produce errors containing
"invalid "and either"invalid type"or"invalid value", but doesn't verify that the action name appears in the error message. Sinceaction_payloadat line 431 intools.rsproduces errors in the format"invalid {action} arguments: {err}", you could strengthen this assertion to also check for the action name.♻️ Optional: stricter assertion
+ let action = args["action"].as_str().unwrap(); let err = execute_tool(&h.state, "cortex", args, None) .await .unwrap_err(); assert!( - err.to_string().contains("invalid ") + err.to_string().contains(&format!("invalid {action} arguments")) && (err.to_string().contains("invalid type") || err.to_string().contains("invalid value")) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp/tools_tests.rs` around lines 594 - 611, The test numeric_args_reject_wrong_type_values should also assert that the error message includes the tool action name emitted by action_payload; update the loop so after calling execute_tool and obtaining err you extract the action string from args (e.g. args["action"]) and assert err.to_string() contains format!("invalid {} arguments", action) in addition to the existing invalid/type checks, ensuring the error includes the specific action name produced by action_payload.
♻️ Duplicate comments (2)
src/runtime/inventory_refresh.rs (1)
293-346:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLong-lived event streams still exhaust the SSH semaphore.
The permit acquired at Line 293 is held until the function returns (Line 346), which doesn't happen until the
docker eventsstream exits (Line 333). For stable fleets, long-running event streams hold permits indefinitely, exhausting themax_concurrentlimit and silently blocking additional hosts from starting their event streams.🔧 Recommended fix
Drop the permit immediately after spawning the SSH child, before entering the line-reading loop:
async fn run_remote_docker_events_once( host: &str, ssh_context: &crate::inventory::ssh::SshContext, trigger: mpsc::Sender<()>, token: CancellationToken, ) -> anyhow::Result<()> { - let Some(_permit) = ssh_context.acquire_owned_cancellable(&token).await? else { + let Some(permit) = ssh_context.acquire_owned_cancellable(&token).await? else { return Ok(()); }; let args = remote_docker_events_ssh_args(ssh_context, host)?; let mut child = Command::new("ssh") .args(args) .stdin(Stdio::null()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) .kill_on_drop(true) .spawn()?; + drop(permit); // Release concurrency slot now that SSH is started let stdout = child🤖 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/runtime/inventory_refresh.rs` around lines 293 - 346, The SSH permit acquired via ssh_context.acquire_owned_cancellable (bound to _permit) is held for the whole function and blocks other hosts; after spawning the SSH child (Command::new("ssh")...spawn()) and starting the stderr_task, explicitly drop the permit (drop(_permit) or move acquisition into a smaller scope) so the semaphore is released before entering the line-reading loop, while keeping the rest of the logic (lines loop, token.cancelled handling, child.kill, stderr_task abort) unchanged.src/deploy.rs (1)
474-669: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMove all inline deploy tests to a sidecar
deploy_tests.rsmodule.The inline test module (lines 474-669) violates the repository coding guideline requiring unit tests to live in sidecar
*_tests.rsfiles with only a#[cfg(test)] #[path = "deploy_tests.rs"] mod tests;hook remaining in the source file. This pattern is established in other modules (ssh.rs,remote_configs.rs,remote_device.rs,remote_docker.rs). All test functions—including the newly addedremote_deploy_rejects_option_like_hosts_before_running_sshandremote_deploy_accepts_safe_hosts—should be moved todeploy_tests.rswithuse super::*;to access the module under test.As per coding guidelines,
**/*_tests.rs: Unit tests live in sidecar files beside their source modules with source files containing only#[cfg(test)] #[path = "..._tests.rs"] mod tests;hook.🤖 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/deploy.rs` around lines 474 - 669, Move the entire #[cfg(test)] mod tests block out of src/deploy.rs into a new sidecar file named deploy_tests.rs and leave only a hook in deploy.rs: #[cfg(test)] #[path = "deploy_tests.rs"] mod tests;; in the new deploy_tests.rs keep use super::*; and preserve all test items (FakeRemoteRunner, its impl RemoteRunner, and tests like remote_dry_run_only_checks_ssh_and_docker, remote_repair_writes_assets_before_compose_up, remote_env_uses_remote_uid_and_gid, remote_deploy_skips_mutations_after_identity_failure, remote_deploy_reports_identity_spawn_error_as_phase_failure, remote_deploy_does_not_source_env_as_shell, remote_deploy_rejects_option_like_hosts_before_running_ssh, remote_deploy_accepts_safe_hosts) unchanged so they continue to reference run_remote_deploy_with_runner and other symbols from the parent module.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/plugin/HOOKS.md`:
- Around line 64-66: The docs show a literal command `plugins/cortex/bin/cortex
setup plugin-hook` that can mismatch the hook definition using
`${CLAUDE_PLUGIN_ROOT}/bin/cortex setup plugin-hook`; update the example to use
the environment variable consistently (i.e. `${CLAUDE_PLUGIN_ROOT}/bin/cortex
setup plugin-hook`) or add a short note instructing users to export
`CLAUDE_PLUGIN_ROOT` to the correct path before running the manual command;
reference the symbols CLAUDE_PLUGIN_ROOT and the setup command to make the
change clear.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Line 10: The footer link definitions are stale after adding the header "##
[1.14.2] - 2026-06-07": update the release compare links so [Unreleased] points
to v1.14.2...HEAD (not v1.14.0...HEAD) and add link definitions for [1.14.2] and
[1.14.1] with the correct GitHub compare ranges (e.g. v1.14.1...v1.14.2 and
v1.14.0...v1.14.1 or your repo's equivalent tags), ensuring the labels in the
footer match the header "[1.14.2] - 2026-06-07" and previous release names.
In `@docs/plugin/CONFIG.md`:
- Line 34: The sentence that says "exports current Claude Code `userConfig`
values as `CORTEX_*` / `CORTEX_*` environment variables" is ambiguous due to the
duplicated `CORTEX_*` reference; update the docs text so it clearly states
whether a single pattern (`CORTEX_*`) is used or two distinct patterns are
meant. Locate the phrase referencing `userConfig` and `CORTEX_*` in CONFIG.md
and either remove the duplicate slash form to read "exports current Claude Code
`userConfig` values as `CORTEX_*` environment variables" or replace one side of
the slash with the correct distinct pattern (and ensure consistency with the
later "CORTEX_* overrides" mention). Ensure the final wording matches the actual
variable pattern used by the codebase.
In `@docs/plugin/HOOKS.md`:
- Line 34: The sentence in HOOKS.md that reads "exports current Claude Code
`userConfig` values as `CORTEX_*` / `CORTEX_*` environment variables" duplicates
the same token and is unclear; edit that line to either remove the repeated
`CORTEX_*` so it reads "as `CORTEX_*` environment variables" or explicitly
clarify what the slash means (e.g., "`CORTEX_*` (build-time) / `CORTEX_*`
(runtime)" or list the two distinct variable prefixes), updating the phrase that
references `userConfig` to match the chosen wording so the intent is
unambiguous.
In `@src/mcp/tools_tests.rs`:
- Around line 594-611: The test numeric_args_reject_wrong_type_values should
also assert that the error message includes the tool action name emitted by
action_payload; update the loop so after calling execute_tool and obtaining err
you extract the action string from args (e.g. args["action"]) and assert
err.to_string() contains format!("invalid {} arguments", action) in addition to
the existing invalid/type checks, ensuring the error includes the specific
action name produced by action_payload.
---
Duplicate comments:
In `@src/deploy.rs`:
- Around line 474-669: Move the entire #[cfg(test)] mod tests block out of
src/deploy.rs into a new sidecar file named deploy_tests.rs and leave only a
hook in deploy.rs: #[cfg(test)] #[path = "deploy_tests.rs"] mod tests;; in the
new deploy_tests.rs keep use super::*; and preserve all test items
(FakeRemoteRunner, its impl RemoteRunner, and tests like
remote_dry_run_only_checks_ssh_and_docker,
remote_repair_writes_assets_before_compose_up,
remote_env_uses_remote_uid_and_gid,
remote_deploy_skips_mutations_after_identity_failure,
remote_deploy_reports_identity_spawn_error_as_phase_failure,
remote_deploy_does_not_source_env_as_shell,
remote_deploy_rejects_option_like_hosts_before_running_ssh,
remote_deploy_accepts_safe_hosts) unchanged so they continue to reference
run_remote_deploy_with_runner and other symbols from the parent module.
In `@src/runtime/inventory_refresh.rs`:
- Around line 293-346: The SSH permit acquired via
ssh_context.acquire_owned_cancellable (bound to _permit) is held for the whole
function and blocks other hosts; after spawning the SSH child
(Command::new("ssh")...spawn()) and starting the stderr_task, explicitly drop
the permit (drop(_permit) or move acquisition into a smaller scope) so the
semaphore is released before entering the line-reading loop, while keeping the
rest of the logic (lines loop, token.cancelled handling, child.kill, stderr_task
abort) unchanged.
🪄 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: 30446e3a-da7c-43ff-be3c-0c115309e168
📒 Files selected for processing (50)
CHANGELOG.mdCLAUDE.mdREADME.mddocs/CHECKLIST.mddocs/CONFIG.mddocs/INVENTORY.mddocs/README.mddocs/RELEASE.mddocs/SECURITY.mddocs/SETUP.mddocs/mcp/SCHEMA.mddocs/plugin/CLAUDE.mddocs/plugin/CONFIG.mddocs/plugin/HOOKS.mddocs/plugin/SKILLS.mddocs/repo/SCRIPTS.mdplugins/cortex/skills/cortex-redeploy/SKILL.mdscripts/check-plugin-manifest-versions.shscripts/check-public-identity.shscripts/smoke-test.shscripts/validate-marketplace.shsrc/app/models/graph.rssrc/app/service_tests.rssrc/app/services/analytics.rssrc/app/services/graph.rssrc/app/services/graph_limits.rssrc/cli/dispatch_surface_gap.rssrc/cli/dispatch_tests.rssrc/db.rssrc/db/analytics.rssrc/db/analytics_tests.rssrc/db/queries.rssrc/db/queries_tests.rssrc/deploy.rssrc/inventory/remote_configs.rssrc/inventory/remote_device.rssrc/inventory/remote_docker.rssrc/inventory/ssh.rssrc/inventory/ssh_tests.rssrc/mcp/actions.rssrc/mcp/rmcp_server.rssrc/mcp/schemas.rssrc/mcp/schemas_tests.rssrc/mcp/tools.rssrc/mcp/tools_tests.rssrc/observability.rssrc/runtime.rssrc/runtime/inventory_refresh.rssrc/runtime/inventory_refresh_tests.rstests/test_live.sh
💤 Files with no reviewable changes (1)
- src/mcp/rmcp_server.rs
Collaborator
Author
|
Final lavra-review fixes pushed in Addressed findings:
Local verification passed:
Push verification:
|
Resolves review thread PRRT_kwDORy0Fc86HsTI8 Resolves review thread PRRT_kwDORy0Fc86HsjPT Resolves review thread PRRT_kwDORy0Fc86HsTI_ Resolves review thread PRRT_kwDORy0Fc86HsTJB Resolves review thread PRRT_kwDORy0Fc86HsTJE Resolves review thread PRRT_kwDORy0Fc86HsTJF
This was referenced Jun 12, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #69.
This PR completes the comprehensive remediation pass for all 24 findings from the full-review issue. It includes runtime/code fixes, regression coverage, CI/policy gates, docs rebrand/security/release updates, and agent-memory source-of-truth repair.
Issue #69 Checklist
CORTEX_INVENTORY_SSH_TRUST_ON_FIRST_USE=truebootstrap opt-in and known-hosts support.--before the host.syslogidentity tocortextool names, scopes, plugin paths, install examples, and schema metadata where appropriate.CLAUDE.mdas the source of truth and converted siblingAGENTS.mdfiles to symlinks after merging deltas.patternsinto DB row fetch plus CPU clustering outside the DB closure.cargo deny checkis clean.cargo clippy --all-targets -- -D warnings.invalid {action} arguments.rmcp = "1.6.0"as the supported lower bound while the lockfile may resolve newer compatible 1.x releases.Verification
cargo fmt -- --checkpassed.cargo testpassed: lib1074 passed, 0 failed, 1 ignored; main328 passed, 0 failed, 1 ignored; integration/doc tests passed.cargo clippy --all-targets -- -D warningspassed.cargo deny checkpassed:advisories ok, bans ok, licenses ok, sources ok.bash scripts/check-version-sync.sh --require-changelogpassed atv1.14.2.bash scripts/check-agent-memory-symlinks.shpassed.bash scripts/check-plugin-manifest-versions.shpassed.bash scripts/check-public-identity.shpassed.git diff --checkpassed.Notes
No subpoints were intentionally deferred. Live fleet smoke gates are documented in
docs/RELEASE.mdand remain operator-intent gates, not hermetic CI gates.Summary by cubic
Completes remediation of all 24 full‑review findings. Tightens request/schema validation, adds graph lookup by id with stricter target rules, and hardens SSH inventory/remote Docker/deploy flows with safer defaults and new telemetry.
deny_unknown_fieldsacross HTTP/MCP models; MCP tool schema setsadditionalProperties: falsewith numeric mins; standardized “invalid {action} arguments”; enforced array shape forabuse.terms.entity_idlookup; reject mixed/partial targets;aroundfixed to depth=1; inventory projection planned outside the write lock and removed redundant counts; bound SQLLIMITparameters; split pattern row fetch from CPU clustering.SshContextwith strict host key checking, safe arg building (reject option-like hosts, insert--), bounded concurrency, and retry/backoff; remote Docker event streams use safer defaults; added counters and last-error snapshots.license = "MIT"; documentedrmcp = "1.6.0"lower bound; bumped release metadata to 1.14.2.Written for commit 80f977d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
CI
Chores