fix: prevent command injection and fix cross-platform compatibility

[VivanRajath]

fix: prevent command injection and fix cross-platform compatibility

Problem

Multiple files use execSync() with string interpolation to run git commands. User-controlled inputs (branch names, commit messages, URLs) are interpolated directly into shell strings, enabling arbitrary command execution:

// Before — vulnerable to injection via branch name
function git(args: string, cwd: string): string {
    return execSync(`git ${args}`, { cwd, stdio: "pipe", encoding: "utf-8" }).trim();
}
git(`checkout -b ${branch}`, dir);  // branch = "; rm -rf /" → executes arbitrary command

Additionally:

• Shell tools (cli, hooks, tool-loader) use sh which doesn't exist on Windows, breaking core command execution on that platform.
• The hook path traversal guard uses hardcoded / instead of path.sep, completely bypassing the security check on Windows (which uses \).

Solution

1. Command Injection → Safe Argument Arrays

Replaced every execSync with string interpolation with execFileSync using argument arrays. Inputs are passed as separate OS-level arguments and are never interpreted by a shell:

// After — safe, inputs cannot be interpreted as shell commands
function git(args: string[], cwd: string): string {
    return execFileSync("git", args, { cwd, stdio: "pipe", encoding: "utf-8" }).trim();
}
git(["checkout", "-b", branch], dir);  // branch is always treated as a literal string

For compound git operations (e.g., git add && git commit), the single shell command was split into separate execFileSync calls:

// Before — single shell command with string interpolation
execSync(`git add "${memoryPath}" && git commit -m "${commitMsg.replace(/"/g, '\\"')}"`, ...);

// After — separate safe calls, no quote escaping needed
execFileSync("git", ["add", memoryPath], { cwd, stdio: "pipe" });
execFileSync("git", ["commit", "-m", commitMsg], { cwd, stdio: "pipe" });

2. Cross-Platform Shell Execution

Shell-spawning tools now detect the platform and use the appropriate shell:

const isWin = process.platform === "win32";
const child = spawn(isWin ? "cmd" : "sh", isWin ? ["/c", command] : ["-c", command], ...);

3. Path Traversal Guard Fix

-if (!resolvedScript.startsWith(allowedBase + "/") && resolvedScript !== allowedBase) {
+if (!resolvedScript.startsWith(allowedBase + sep) && resolvedScript !== allowedBase) {

Files Changed (12)

File	Change
src/session.ts	git() helper rewritten + all 15 call sites
src/loader.ts	git clone in resolveInheritance() and resolveDependencies()
src/index.ts	isGitRepo(), ensureRepo() git init/add/commit
src/tools/memory.ts	Memory save git add + commit
src/sandbox.ts	git remote get-url origin
src/tools/skill-learner.ts	gitCommit() helper + delete action
src/tools/capture-photo.ts	Photo commit git add + commit
src/plugin-cli.ts	Removed unused execSync import
src/tools/cli.ts	Cross-platform shell (sh/cmd)
src/hooks.ts	Path traversal guard (path.sep) + cross-platform shell
src/tool-loader.ts	Declarative tool scripts cross-platform shell

Not Included

src/voice/server.ts still uses execSync — it's a 113KB monolith that needs a refactor before these changes can be applied safely. Planned for a follow-up PR.

Testing

-tsc --noEmit passes with zero compilation errors
-npm test passes with zero compilation errors

• No behavioral changes — all functions maintain identical semantics
• The only difference is that inputs can no longer escape their argument boundaries

[shreyas-lyzr]

Good intent — command injection prevention and cross-platform compat are important. However, there's a breaking change:

Build script change breaks macOS/Linux:
```diff
-"build": "tsc && cp src/voice/ui.html dist/voice/"
+"build": "tsc && copy src\voice\ui.html dist\voice\"
```
`copy` and backslash paths are Windows-only. This breaks the build on macOS and Linux (where all current contributors develop). Use a cross-platform approach:
```json
"build": "tsc && node -e "require('fs').cpSync('src/voice/ui.html','dist/voice/ui.html')""
```

The `execSync` → `execFileSync` changes and `path.sep` usage are correct and welcome. The `package-lock.json` diff includes unrelated dependency bumps — ideally separate those. Please fix the build script and I'll approve.

[VivanRajath]

Thanks for the detailed review.

I've updated the build script to use a cross-platform Node-based approach (using fs.cpSync) and ensured it works across macOS, Linux, and Windows.

Also reverted the unrelated dependency changes in package-lock.json to keep this PR focused.

tsc --noEmit passes with zero compilation errors
-npm test passes with zero compilation errors

[shreyas-lyzr]

Build script fix is exactly right — cross-platform node -e approach. The execSync → execFileSync changes and Windows shell detection all look good.

One issue remaining: the PR includes a my-project/ directory with test scaffolding files (agent.yaml, SOUL.md, memory/MEMORY.md). The agent.yaml even contains a local Windows path (C:\Users\Vivan Rajath\Desktop\...) as the agent name. These shouldn't be committed to the repo.

Please remove the my-project/ directory from the PR (add it to .gitignore or just git rm -r my-project/) and this is good to merge.

[VivanRajath]

Hey Shreyas,

I’ve removed the my-project/ directory from the repository and added it to .gitignore to prevent it from being committed again.

I’ve pushed the updated changes to the PR. Please take another look , It should be good to merge now.

[shreyas-lyzr]

Apologies for the very long wait on this — March was busy and this should not have sat for almost two months.

Brutal honesty: the underlying changes here are exactly right.

• path.sep instead of hardcoded / in the hook escape check — correct cross-platform fix; the current main is still broken on Windows for the same reason.
• cmd /c vs sh shell selection in executeHook — correct, hooks were never going to work on Windows otherwise.
• cp src/voice/ui.html → node -e cpSync(...) in build script — same story, Windows.
• execSync → execFileSync — main still has execSync in src/session.ts, src/sandbox.ts, src/index.ts, src/tools/memory.ts, and src/tools/capture-photo.ts. Several of those use template-literal arg interpolation, which is a real command-injection risk (e.g., tools/capture-photo.ts runs git add "${photoRelPath}" "${INDEX_FILE}" && git commit -m "${commitMsg.replace(/"/g, '\\"')}" — a path with a " would break out).
• Surface assistant-end errors in red — also reasonable; main has partial coverage now via stopReason === "error", but the on_error hook fire + audit log path from this PR is still relevant.

The blocker is just the rebase. Two months of code drift means the conflict footprint is large. A few options:

1. Rebase this PR on main. If it's a small effort for you, that's the cleanest path. I'll review same-day after rebase.
2. Split into smaller PRs. The four concerns are independent: (a) cross-platform hook executor, (b) cross-platform build script, (c) execSync→execFileSync migration across the codebase, (d) error surfacing on agent_end. Each could land separately.
3. If you're out of time, comment here and I'll pick up the rebase + split myself with you credited as co-author.

Whichever path works for you. The work is genuinely valuable — the cross-platform and command-injection pieces are blockers for production deployment on regulated stacks.

Real apologies again.

[VivanRajath]

Hi @shreyas-lyzr — rebased on current main and force-pushed. Branch is now 3 clean commits on top of 8a6959f:

1. fix: prevent command injection and fix cross-platform compatibility — the execSync → execFileSync migration, path.sep guard, and cmd/sh shell selection

2. fix: surface assistant errors in red on agent_end — the on_error path you mentioned in bullet 4

3. fix: cross-platform build script for voice UI — the node -e cpSync(...) build fix, isolated

Conflict resolution notes:

• src/tools/memory.ts and src/tools/skill-learner.ts — main added a type Static import alongside; kept both that and the execFileSync switch. Call sites auto-merged.

• package-lock.json — regenerated via npm install rather than hand-merged, so no unrelated dep bumps.

• All other migrated files (session.ts, sandbox.ts, index.ts, hooks.ts, loader.ts, etc.) auto-merged cleanly despite the drift.

tsc and npm run build both pass.

Known gap, not a blocker: In src/tool-loader.ts the Windows branch uses cmd /c <script>, which relies on file-extension association and ignores the declarative tool's runtime field. For the default sh runtime this is fine; a tool explicitly declaring runtime: python with a non-.py extension wouldn't pick up Python on Windows. Worth a follow-up if/when that path matters — but it was completely broken on Windows before, so this isn't a regression.

Out of scope, as in the original PR:

• src/voice/server.ts — still excluded per the original PR description. Main has added more execSync call sites there during the drift (including the same template-literal pattern from capture-photo.ts), so the follow-up is more justified now. Happy to file a tracking issue.

• src/plugins.ts pathToFileURL (raised by @huangrichao2020 in Architecture-quality audit notes from hermescheck #30 and referenced by you in that thread) — confirmed the bug at line 245. Will open as a separate small PR to keep this one scoped, unless you'd rather I add it as a 4th commit here.
  also
  tsc, npm run build, and npm test all pass (27/27 tests).
  Ready for re-review.