[lhw2002426]

Description:

Add three new workspace management subcommands to rbnx CLI and enhance the build command:

rbnx init — Scaffold a new robonix workspace with robonix_workspace.yaml, packages/, contracts/, generated/, and deploy/ directories.
rbnx package-new — Create a new package under packages/ with robonix_manifest.yaml and src/ boilerplate.
rbnx deploy — Deploy a full stack (runtime + services + packages) from a config file.
rbnx build -c — Add --config flag to batch-build all packages defined in a config file.

Changes:

New modules: init.rs, package_new.rs, deploy.rs, build.rs
Updated mod.rs to register and route new commands
Added dependency in Cargo.toml

[enkerewpo]

Code Review: rbnx init / package-new / deploy / build --config

Thanks for the substantial feature addition! I've reviewed the full diff (~1500 lines). There are a few critical issues that should be addressed before merge, along with several medium/low suggestions.

---

🔴 HIGH — Must fix before merge

1. Command injection via unescaped YAML values in deploy.rs

The deploy command builds a shell string by interpolating params_file paths and inline params values directly into a bash -c invocation:

full_cmd.push_str(&format!("export RBNX_PARAMS_FILE={}; ", params_abs.display()));
full_cmd.push_str(&format!("export {}={}; ", env_key, val_str));

A malicious config.yaml with params_file: "foo; rm -rf /" or params containing $(evil_command) will execute arbitrary shell commands.

Fix: Pass params as environment variables via Command::envs() instead of constructing shell export statements. The spawn_background function already accepts an env map — use it.

2. std::mem::forget(child) leaks process handles — zombie processes

std::mem::forget(child);

The child handle is never reaped, creating zombie processes. There is no way to gracefully shut down individual children on error or Ctrl+C.

Fix: Store child handles in a Vec<Child>. On shutdown, iterate and send SIGTERM/wait.

3. Shutdown kills own process group — kills CLI itself and parent shell

let pgid = getpgrp();
let _ = killpg(pgid, Signal::SIGTERM);

getpgrp() returns the calling process's group. This kills the CLI itself and potentially the parent shell or CI runner. This is dangerous and unreliable.

Fix: Track spawned child PIDs explicitly and kill them individually, or use pre_exec to place children in a dedicated process group and kill that group instead.

4. start_cmd from manifest passed directly to bash -c

The node.start field from robonix_manifest.yaml is executed via bash -c with no validation. In a supply-chain scenario, a compromised package manifest could execute arbitrary commands.

Fix: At minimum, validate or log the command before execution. Consider restricting to a known set of entry point patterns.

---

🟡 MEDIUM — Should fix

5. Topological sort uses pop() (LIFO) instead of FIFO — reversed order

All three topo sort implementations sort the queue alphabetically, then call .pop() which takes from the end — producing reverse-alphabetical order among nodes at the same level. This contradicts the "stable ordering" intent.

Fix: Use VecDeque with pop_front(), or reverse the sort so pop() returns the smallest name.

6. Three near-identical topological sort implementations

topo_sort_by_name_deps() in build.rs, topo_sort_packages() and topo_sort_runtime() in deploy.rs are copy-pasted Kahn's algorithm. This violates DRY and makes maintenance error-prone.

Fix: Extract a generic topo_sort<T> that takes name/deps extractors as closures, put in a shared module.

7. depends_on referencing a non-existent name is silently ignored

if let Some(&dep_idx) = name_to_idx.get(dep.as_str()) {
    // ...
}
// else: silently ignored

If a package lists a dependency that doesn't exist in the config, it's silently skipped. This can cause incorrect build/deploy ordering and hard-to-debug failures.

Fix: Emit a warning or return an error for unknown dependency names.

8. init does not validate workspace name for path safety

let workspace_dir = Path::new(name);

The name argument is used directly as a directory path. Values like ../../../etc or /tmp/evil create directories in unexpected locations. Unlike package_new which validates characters, init has no validation at all.

Fix: Apply the same validation as package_new — restrict to alphanumeric, hyphens, and underscores. Reject path separators and ...

9. make_executable permissions too broad

perms.set_mode(perms.mode() | 0o755);

This ORs with 0o755, which also adds read/write for group and others. The intent is to add execute bits only.

Fix: perms.set_mode(perms.mode() | 0o111);

10. --config and -p/--path are not mutually exclusive

When both are provided, -p is silently ignored. This is confusing.

Fix: Add conflicts_with = "path" on the config arg in the clap derive.

11. Duplicate config schema structs

SystemEntry, UpstreamWorkspace/UpstreamConfig are nearly identical across build.rs and deploy.rs. If the schema evolves, they will drift.

Fix: Define shared config schema structs in a common module (e.g., src/config_schema.rs).

12. Contract validation uses prefix matching — overly permissive

consumed.id.starts_with(cid)

robonix/sys/model would match robonix/sys/model_evil.

Fix: Use exact matching, or at least a path-segment-aware prefix check: consumed.id == cid || consumed.id.starts_with(&format!("{cid}/")).

---

🟢 LOW — Nice to have

#	Issue	Suggestion
13	Config env overrides process env (precedence inversion)	Process env should take priority, or at least skip overriding system vars like PATH
14	Adds toml = "0.8" but workspace already has toml 1.1 — dual versions in lockfile	Use the workspace version, or remove if unused (I don't see toml used in the new code)
15	YAML template system: [] followed by commented examples — un-commenting produces invalid YAML	Use system: without [], or place comments before the key
16	detect_rust_root walks up at most 10 directories — magic number	Walk until filesystem root, or document the limit
17	find_service_package hardcodes service name mappings ("vlm" → "vlm_service")	Make the service package path configurable in the YAML schema
18	No --dry-run flag for deploy	Would help debug deployment configs safely
19	canonicalize failure silently falls back to uncanonicalized path	Propagate the error or log a warning
20	package_new resolves packages/ relative to cwd, not workspace root	Detect workspace root by looking for robonix_workspace.yaml in parent dirs

---

Summary

Severity	Count	Key themes
🔴 High	4	Command injection (×2), zombie processes, dangerous process group kill
🟡 Medium	8	Topo sort bugs, code duplication, missing validation, silent failures
🟢 Low	8	Env precedence, unused dep, YAML templates, hardcoded mappings

The command injection in deploy.rs (issues #1 and #4) and process lifecycle problems (issues #2 and #3) are blockers. The rest can be addressed incrementally.

---

Generated by Claude Code

[enkerewpo]

@copilot resolve the merge conflicts in this pull request