fix(codemode): harden saved snippet execution - #682
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate review findings remain in budget enforcement, tool scoping, telemetry, and homelab inventory snippets.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request hardens saved Code Mode snippet execution and adds reusable homelab SSH/Docker inventory snippets.
Changes:
- Raises source and composition limits with parse-only validation.
- Adds exact tool scoping, response-budget handling, and lifecycle telemetry improvements.
- Documents and implements homelab SSH discovery and Docker inventory workflows.
File summaries
| File | Reviewed changes and findings |
|---|---|
plugins/labby/skills/using-labby/references/config-reference.md |
Updates source-limit configuration documentation. |
plugins/labby/skills/using-labby/references/code-mode.md |
Updates Code Mode limits and execution guidance. |
docs/snippets/README.md |
Documents saved-snippet execution and tool declarations. |
docs/snippets/homelab-ssh-targets.md |
Adds SSH discovery. Critical: terminate SSH options before aliases. Moderate: preserve missing-command fields and report traversal truncation. |
docs/snippets/homelab-docker-inventory.md |
Adds multi-host Docker inventory. Critical: redact full ssh_config from artifacts. |
docs/snippets/docker-host-inventory.md |
Adds per-host Docker inventory. Critical: disable SSH agent forwarding. Moderate: preserve optional-command field positions. |
docs/services/SNIPPETS.md |
Updates snippet service documentation. |
docs/dev/CODE_MODE.md |
Updates Code Mode development guidance. |
crates/labby/src/dispatch/snippets/dispatch.rs |
Executes saved snippets with scoped dependencies. Moderate: enforce the effective source limit before execution. |
crates/labby/src/dispatch/setup/settings.rs |
Updates configuration exposure. |
crates/labby/src/config.rs |
Updates Code Mode defaults. |
crates/labby-runtime/src/gateway_config.rs |
Validates source-size configuration. |
crates/labby-gateway/src/upstream/pool/stdio_transport.rs |
Adjusts child-termination telemetry. Moderate: retain warnings for unexpected failures. |
crates/labby-gateway/src/upstream/pool/probe.rs |
Updates upstream probing behavior. |
crates/labby-gateway/src/upstream/pool/lifecycle_compat.rs |
Updates lifecycle compatibility handling. |
crates/labby-gateway/src/upstream/pool/connect.rs |
Updates connection behavior. |
crates/labby-gateway/src/security/spawn_guard.rs |
Updates spawn-guard behavior. |
crates/labby-gateway/src/gateway/manager/tests/code_mode.rs |
Adds Code Mode catalog coverage. |
crates/labby-gateway/src/gateway/manager/code_mode_runtime.rs |
Filters cached catalogs and adjusts telemetry. Moderate: retain an actionable partial-probe failure signal. |
crates/labby-gateway/src/gateway/code_mode/search.rs |
Builds scoped catalogs. Moderate: filter returned tools by the full declaration scope. |
crates/labby-codemode/src/truncate.rs |
Preserves compact responses under trace pressure. |
crates/labby-codemode/src/snippet/tool_declarations.rs |
Validates exact tool declarations. |
crates/labby-codemode/src/snippet/store.rs |
Adds snippet limits and validation. Moderate: enforce configured lower source caps and avoid exposing filesystem paths in errors. |
crates/labby-codemode/src/runner_drive/artifacts.rs |
Resolves nested snippets and enforces execution budgets. |
crates/labby-codemode/src/config.rs |
Defines source and composition budgets. Critical: align the generated sandbox cap with the advertised 1 MiB budget. |
config/config.example.toml |
Updates example configuration. |
Review details
Suppressed comments (7)
crates/labby-codemode/src/snippet/store.rs:24
- The saved-snippet path only forwards
config.max_source_bytesto the broker; unlike direct Code Mode requests, this broker path does not enforce that configurable lower limit (the runner enforces only the separate 1 MiB composed-snippet budget). A host configured with, for example, a 128 KiB source cap can therefore execute a nearly 1 MiB saved snippet, contradicting the comment here and bypassing the operator's resource limit. Enforce the configured cap on the resolved saved-snippet source before wrapping/execution and add a lower-limit test.
/// legacy cap. Hosts may still configure a lower `code_mode.max_source_bytes`,
/// which is enforced when the snippet executes.
const MAX_SNIPPET_CODE_BYTES: usize = crate::config::MAX_SOURCE_BYTES;
crates/labby-codemode/src/snippet/store.rs:353
- This not-found error is serialized to callers of the snippets action and now includes the server's absolute user and built-in snippet directories. That exposes deployment filesystem layout to remote clients for a condition that only needs a stable missing-name error; keep the paths in server-side diagnostics rather than the model-facing message.
message: format!(
"snippet `{name}` not found; searched user snippets at `{}` and built-ins at `{}`",
user_dir.display(),
builtin_dir.display()
),
crates/labby-gateway/src/gateway/code_mode/search.rs:137
- The scoped render path only restricts which upstreams are contacted; it never applies the declaration's exact
scope.allowspredicate to the returned tools. Thus a declaration such asomega::pingstill injects every discovered tool inomegainto the runner catalog: direct calls are denied later, butcodemode.searchand generated helpers can expose undeclared schemas and defeat the exact-declaration/minimal-catalog contract. Filter the returned tools by the full scope before rendering.
) -> Result<ToolsRender, ToolError> {
let raw_tools = if use_cache {
manager
.code_mode_catalog_tools_cached_allowed(Some(owner), oauth_subject, allowed_upstreams)
.await?
} else {
manager
crates/labby-gateway/src/gateway/manager/code_mode_runtime.rs:621
- A failed cold probe is now logged only at DEBUG, and the method still returns a partial catalog when another upstream connects. The failed upstream is not included in the response or an aggregate warning, so its tools can disappear silently in production unless DEBUG logging is enabled; retain one actionable warning/partial-failure signal while avoiding repeated per-upstream startup spam.
Ok(Some((upstream, _, Err(error)))) => {
outstanding.remove(&upstream.name);
tracing::debug!(
surface = "dispatch",
service = "gateway",
docs/snippets/docker-host-inventory.md:112
- Shell quoting protects the Bash command but not OpenSSH's option parser: a caller-provided alias such as
-oProxyCommand=...is still interpreted as an SSH option after quote removal, allowing an unintended local proxy command. Terminate SSH options with--before the alias, and apply the same guard to every alias-based SSH invocation.
command: "ssh " + opts + " " + q(i.alias) + " " + q(r),
docs/snippets/homelab-ssh-targets.md:253
- Shell quoting protects the Bash command but not OpenSSH's option parser: a configured or extra host alias beginning with an option can still alter this SSH invocation after quote removal. Terminate SSH options with
--beforetarget.aliasso input cannot inject client options.
"ssh " + opts + " " + quote(target.alias) + " " + quote(probe),
docs/snippets/homelab-ssh-targets.md:231
- These SSH commands inherit
ForwardAgentfrom the user's configuration. If a selected alias enables agent forwarding, the read-only discovery probe exposes the controller's agent to the remote host; explicitly disable agent forwarding for this workflow.
"-o BatchMode=yes",
- Files reviewed: 26/26 changed files
- Comments generated: 9
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings remain in tool scoping, truncation, source validation, diagnostics, and homelab snippets.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (8)
crates/labby-codemode/src/snippet/store.rs:24
- This now accepts a full
MAX_SOURCE_BYTES, but saved execution wraps every snippet with an input/async-function wrapper beforeCodeModeBrokerenforcesconfig.max_source_bytes. A snippet at the advertised 1 MiB boundary (or with sufficiently large input) passes storage validation and is then rejected before running. Reserve wrapper/input overhead or validate the final wrapped source so the storage and execution contracts agree.
const MAX_SNIPPET_CODE_BYTES: usize = crate::config::MAX_SOURCE_BYTES;
crates/labby-codemode/src/snippet/store.rs:237
read_resolvednow compiles the extracted JavaScript duringvalidate_snippet_body, and callers then invokecode_for_snippet, which compiles the same source again. For example, every gateway snippet resolve followed by execution pays for two Javy parses; with the new 1 MiB limit this is an avoidable per-run cost. Return/reuse the validated extracted source, or separate metadata validation from execution-source validation.
let code = normalize_snippet_code(&code).to_string();
crates/labby-codemode/src/snippet/store.rs:586
- A valid saved snippet ending in a
//comment is rejected here:normalize_snippet_codetrims the final newline, so appending);on the same line lets the comment swallow the validator's closing delimiter. The execution normalizer already accepts this form, so put the generated closing delimiter on a new line (or strip trailing comments consistently).
let source = format!("export default ({code});");
runtime
.compile_to_bytecode("snippet-validation.js", &source)
crates/labby-gateway/src/gateway/manager/code_mode_runtime.rs:624
- This downgrade leaves the all-failed one-shot path without an actionable warning:
code_mode_catalog_tools_cached_allowedreturns earlier when nothing connected, before the summary warning below this block, so this DEBUG event is the only log containing the failed upstream name. Keep a warning summary on that path (or preserve WARN here) while avoiding repeated warnings for recovered/suppressed probes.
tracing::debug!(
surface = "dispatch",
service = "gateway",
action = "code_mode.catalog_cache",
upstream = %upstream.name,
crates/labby-gateway/src/security/spawn_guard.rs:114
WARNED_COMMANDSgrows for every distinct bypassed command and is never evicted or bounded. Becausebypassskips all validation, a long-lived process that accepts unique command strings can grow this process-wideBTreeSetindefinitely; use a bounded dedupe or aggregate warning instead.
static WARNED_COMMANDS: OnceLock<Mutex<BTreeSet<String>>> = OnceLock::new();
let warned = WARNED_COMMANDS.get_or_init(|| Mutex::new(BTreeSet::new()));
let first_for_command = warned
.lock()
.unwrap_or_else(|poisoned| poisoned.into_inner())
.insert(command.to_string());
crates/labby/src/dispatch/snippets/dispatch.rs:305
- Using
ToolScope::default()here drops the caller's route/upstream policy. MCPsnippets.exec/snippets.testreach this helper through generic dispatch, which suppliesTrustedLocal, so a protected route that exposes the snippets service can execute an undeclared snippet—or a declaration naming another upstream—against any configured upstream. Thread the route-derived caller scope through these actions and intersect the declaration with it; otherwise the documented "declaration can only narrow caller authority" invariant is not enforced.
.map(|declared| declared.intersect(&ToolScope::default()))
docs/snippets/homelab-ssh-targets.md:204
- When
ssh_configis supplied, discovery reads and parsesroot, but thisssh -Ginvocation omits-F root; the subsequent probe invocation has the same omission. Aliases can therefore be discovered from a custom file but resolved with the controller's default HostName/User/IdentityFile settings. Pass the selected config path to both SSH commands.
command: "ssh -G -- " + quote(target.alias),
docs/snippets/homelab-ssh-targets.md:133
- Relative
Includeexpansion is wrong when the rootssh_configis a bare filename: with no/,lastIndexOfis-1, so this slice length becomes1and resolvesconfig+included.confascincluded.conf. Use an empty directory prefix whenfromhas no slash so the documented non-glob include expansion works for relative config paths.
: from.slice(0, Math.max(0, from.lastIndexOf("/")) + 1) + p;
- Files reviewed: 32/32 changed files
- Comments generated: 3
- Review effort level: Lite
Summary
Verification
Notes
The active root-owned Labby system service is intentionally not replaced from the unprivileged labby account. Final saved snippets are installed and validated live; binary rollout should use the normal privileged deployment path after merge.