fix: merge latest dev updates into main

[node9ai]

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 PyPI package will be published!

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK PR

🚨 Critical Issues

1. Merge conflict in README.md
The diff contains an unresolved merge conflict marker (<<<<<<< dev / >>>>>>> main). This should never reach a PR review stage. Block this until resolved.

2. NODE9_SKIP bypass is a global, unauthenticated escape hatch
The env var NODE9_SKIP=1 bypasses all checks. There's no mention of integrity verification — any process with env access can silently disable the security library. At minimum, document this prominently as a threat, and consider requiring it to be set at import time only (not dynamically reassignable at runtime). If _config.py reads this lazily on each call, an attacker who can mutate the environment mid-run can disable enforcement retroactively.

3. _dispatch calls internal methods via name lookup — no allowlist
From the README example: agent._dispatch(block.name, block.input). If _dispatch resolves method names by string lookup without strictly filtering to _TOOL_ATTR-marked methods, an LLM could craft a tool_name that maps to an @internal method or even arbitrary class methods, bypassing evaluate() entirely. The diff is truncated so this can't be confirmed — but this is a critical attack surface and must be verified explicitly.

4. shell=True in README and manual_test.py examples
Both run_shell and run_tests examples use subprocess.check_output(command, shell=True, ...). For a security library, shipping shell=True as the canonical example is a red flag. An AI agent passing unsanitized LLM output to these functions is exactly the attack vector Node9 is supposed to protect against. Replace with shell=False and list-form commands in all examples.

---

⚠️ Security / Correctness Issues

5. DLP scan uses or "" — empty string skips scan

filename = call_args.get("filename") or call_args.get("path") or ""
content  = call_args.get("content") or ""

If filename is 0, False, or any falsy non-empty value, the scan is skipped silently. Use is None checks instead. Also, this only checks filename and content keys — tools with parameters named output_path, destination, body, etc. bypass DLP entirely. This heuristic is fragile for a security guarantee.

6. configure() is not thread-safe
configure() mutates module-level globals in _config. If called from multiple threads (plausible in async agent frameworks), there's a race condition. This is a security library — a misconfigured policy window, even briefly, is a problem.

7. safe_path returns a value but the @tool wrapper discards it
The wrapper calls safe_path() for validation but then passes the original filename to the underlying function. If safe_path returns a normalized/resolved path, the function should use that resolved path — otherwise the traversal check can be trivially bypassed depending on implementation.

---

🔧 API / Design Issues

8. _dispatch and _build_tools are public API with underscore names
The README uses agent._dispatch(...) as the primary integration point. Underscore prefix signals "private" in Python. Either rename to dispatch() / build_tools(), or document explicitly why they're underscore-prefixed. This will confuse integrators.

9. tool() decorator introspects only filename/content/path — too narrow
For a general governance library, DLP that only triggers on three hardcoded parameter names will give users false confidence. A tool named write_record(destination, body) gets no DLP. This needs either broader heuristics, opt-in per-parameter annotation, or clearer documentation of the limitation.

10. configure() silently ignores empty strings

if agent_name:
    _config.AGENT_NAME = agent_name

configure(agent_name="") does nothing. This is probably intentional but could mask bugs where callers pass empty strings expecting a reset. Consider raising ValueError on empty string rather than silently no-op-ing.

---

🧪 Test Coverage Gaps

• No test for NODE9_SKIP=1 being set after import (runtime bypass)
• No test for _dispatch with a non-@tool method name (the allowlist attack surface above)
• No test for concurrent configure() calls
• No test for DLP scan with non-standard parameter names (body, destination, output)
• No test for @tool used on an async method (the wrapper is sync — would this deadlock?)
• manual_test.py is committed to the repo root — it should be in tests/ or excluded via .gitignore

---

Minor

• Version jump from 0.1.1 → 2.0.0 is a breaking change signal; ensure CHANGELOG and migration notes exist.
• The truncated diff means _agent.py couldn't be fully reviewed — the _dispatch allowlist concern above must be re-reviewed once the full file is visible.

⚠️ 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

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK PR

🚨 Critical Issues

1. Merge conflict in README.md
The diff contains an unresolved merge conflict (<<<<<<< dev / >>>>>>> main). This should never reach a PR review — reject until resolved.

2. NODE9_SKIP bypass is a global config-level escape hatch with no audit trail
The env var NODE9_SKIP=1 bypasses all checks. There's no indication this is logged anywhere. In a security library, even bypasses should emit a warning or audit entry. A misconfigured CI pipeline silently disabling all protection is a serious risk.

3. _dispatch is a public API in documentation but uses a leading underscore
The README shows agent._dispatch(block.name, block.input) as the primary integration point for LLM loops. This naming signals "private, subject to change" to Python developers but it's the main integration surface. This is an API design problem — it will cause friction and incorrect usage.

4. tool decorator DLP scan is bypassable by argument naming
The DLP check only fires if the parameter is literally named filename, path, or content. A developer writing @tool("write_file") def write_file(self, dest: str, body: str) gets zero DLP protection silently. This is a false sense of security — the decorator advertises DLP but it's heuristic and fragile.

5. configure() mutates module-level globals without thread safety

_config.AGENT_NAME = agent_name
_config.AGENT_POLICY = policy

In async agent frameworks (LangGraph, FastMCP), concurrent requests could race on these globals. A shared agent running multiple sessions simultaneously could have incorrect policy applied to a call.

---

⚠️ Security Concerns

6. run_shell in README uses shell=True
The example code shows subprocess.check_output(command, shell=True, text=True) as a pattern to decorate with @protect. Using @protect does not sanitize command — it only gates on human approval. The README should call this out explicitly, not present it as a safe pattern.

7. safe_path raises ValueError for traversal but @tool wraps it as ActionDeniedException
This is actually reasonable, but the safe_path standalone utility raising ValueError (not ActionDeniedException) creates inconsistency. Callers using safe_path directly get a different exception type than callers using @tool. The README documents both paths.

8. @internal logs locally but "local only" is undefined
The docs say @internal "logs locally only." Where? ~/.node9/audit.log? stdout? If an @internal method exfiltrates data, there's no audit. The contract needs to be explicit.

---

🔧 Correctness & Edge Cases

9. tool decorator with @tool (no-arg) uses fn.__name__ as tool name
This is fine, but the attribute _node9_tool = fn.__name__ means two methods with the same name on different classes will collide if introspected globally. Not a runtime bug, but worth noting for future _build_tools introspection.

10. bound.apply_defaults() before DLP scan
Default argument values will be DLP-scanned even if the caller didn't provide them. If a method has content: str = "", the empty string gets scanned. Low severity, but could cause unexpected behavior if defaults contain template strings resembling secrets.

11. Node9Agent workspace not validated at construction time
workspace="/path/to/repo" is stored but if it doesn't exist, safe_path will fail at call time with an unclear error. Should validate and raise early in __init__.

---

🧪 Test Coverage Gaps

• No test for configure() being called after the first @protect call (race condition / ordering issue)
• No test for tool decorator with non-standard parameter names (the DLP bypass described above)
• No test for NODE9_SKIP=1 to confirm it doesn't log anything (or confirm it does)
• manual_test.py is committed to the repo root — this should be in examples/ or tests/, not the project root
• No test for build_tools_anthropic() vs build_tools_openai() schema shape correctness

---

📐 API Design

• The dual-path API (@protect vs Node9Agent) is a good design, but the README needs a clearer decision tree for which to use. Currently it reads as two equal options; in practice @protect is for retrofitting and Node9Agent is for greenfield.
• _build_tools() returning a "neutral format" with a parameters key alongside Anthropic/OpenAI-specific builders will confuse developers. Document the exact schema or make the neutral format the base for both builders explicitly.

---

Summary

Do not merge. The merge conflict alone is a blocker. The DLP heuristic bypass and _dispatch naming are the most impactful issues to fix before this ships to security-conscious users.

⚠️ 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

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK v2.0.0

Critical Security Issues

1. shell=True in documentation examples (README + manual_test.py)
The README shows subprocess.check_output(command, shell=True, text=True) with only a comment warning. For a security library, this is dangerous documentation. The SDK's own CiAgent example in _agent.py (docstring) has the same pattern. The note about shlex.split() is buried and easily missed. The canonical example should use shell=False.

2. NODE9_SKIP bypass has no integrity check

NODE9_SKIP=1

Any process with env access can silently disable all governance. There's no audit log entry when NODE9_SKIP is active, no warning emitted, and no way for the SaaS backend to know it was bypassed. At minimum, skip-mode should emit a loud warning and ideally should be restricted (e.g., only honored in test environments via a separate mechanism).

3. Path traversal check only fires if _workspace is set

if hasattr(self, "_workspace") and self._workspace:

If workspace is not passed to Node9Agent.__init__, the path safety check is silently skipped. An agent with write_file or similar tools is unprotected. This should either be a hard requirement or fail closed (block the call) rather than silently passing.

4. DLP path_arg extraction is fragile

path_arg = call_args.get("filename") or call_args.get("path") or ""

This only checks two parameter names. A method named write_config(destination: str, ...) bypasses the path-based DLP check entirely. The comment says "all string args" but that only applies to content scanning — the sensitive-path check only fires on filename/path. This is a correctness bug with security implications.

5. configure() locking is incomplete
The lock protects writes but not reads. _config.AGENT_NAME is a module-level mutable variable read without any lock throughout the codebase. Under concurrent asyncio + threading (LangGraph), this is a data race. Either use a threading.RLock around reads too, or make the config immutable after first use.

---

Correctness / Edge Cases

6. dispatch vs _dispatch inconsistency
The README example uses agent.dispatch(block.name, block.input) but _agent.py's docstring uses agent._dispatch(...). If the public API is dispatch, _dispatch should be an alias or vice versa — this will cause AttributeError for users following one of the examples.

7. tool decorator dual-mode type annotation is wrong

def tool(tool_name: str | Callable):

str | Callable uses the union shorthand (Python 3.10+). If users are on 3.9, this raises TypeError at import time. Use Union[str, Callable] or typing.Union for the declared Python support range.

8. run_id scope per agent instance
A single Node9Agent instance reuses the same run_id across its entire lifetime. If an agent handles multiple user sessions (common in server deployments), all sessions share one run ID in the dashboard. This seems architecturally wrong — run ID should be per-invocation or per-session, not per-instance.

9. No timeout on evaluate() calls
The blocking HTTP call to the daemon has no documented timeout. If the daemon hangs, @protect blocks the calling thread indefinitely. Async callers run this in a thread executor — an unbounded thread pool could exhaust resources under load.

---

API Design

10. write_file path in Node9Agent example ignores workspace

def write_code(self, filename: str, content: str) -> str:
    with open(filename, "w") as f:  # raw open — not workspace-relative

The path safety check validates filename against _workspace, but the actual open() uses the raw value. If filename is "output.py" (no slash), the path check passes but the file is written to CWD, not the workspace. The canonical example should use safe_path explicitly in the function body.

11. build_tools_anthropic / build_tools_openai not in __all__
These are the primary integration points for the two dominant LLM providers, yet they're undocumented in __all__ and not shown in the README's env var table or API surface summary.

---

Test Coverage Gaps

• No test for NODE9_SKIP=1 behavior (does it log? does it warn?)
• No test for concurrent configure() calls
• No test for @tool on a method with no string args (DLP edge case)
• No test for workspace=None path safety bypass
• manual_test.py is committed to the repo root — this should live in tests/ or be gitignored; shipping smoke tests as top-level files is noisy and suggests test infrastructure is incomplete

---

Minor

• The manual_test.py comment "Note: @Protect gates on human approval but does NOT sanitize command." has broken indentation in the CiAgent example — return is at the wrong level.
• __version__ = "2.0.0" with no changelog or migration guide is a significant jump from 0.1.1.

⚠️ 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

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK v2.0.0

Security Issues (Critical)

1. NODE9_SKIP bypass is trivially exploitable

# _client.py (implied by README)
if os.environ.get("NODE9_SKIP"):
    return  # auto-approve

Any code running in the same process can set os.environ["NODE9_SKIP"] = "1" at runtime. For a security library, this bypass should be a startup-time constant (read once at import, stored as a module-level bool, never re-evaluated). Otherwise a compromised dependency could silently disable all governance.

2. DLP scan on @tool only scans string args — structured inputs escape it
If an LLM passes {"content": {"nested": "AKIA1234..."}}, the DLP scan on string args won't catch it. The README lists this as a security guarantee. Either document the limitation explicitly or scan JSON-serialized representations of non-string args.

3. shell=True in README examples despite the inline warning
The inline comment # Use shlex.split() + shell=False directly contradicts subprocess.check_output(command, shell=True, text=True) in the same code block. This is a security library — the example code should model correct behavior, not document the wrong behavior and hope developers read the comment.

4. configure() race is only partially solved
The lock protects the write, but reads in _client.py/_agent.py are unsynchronized. If a thread calls @protect concurrently with configure(), it may read a partially-updated AGENT_NAME or AGENT_POLICY. Use a RLock or make config immutable after first use.

---

Correctness / Edge Cases

5. dispatch() error handling is unspecified
The README shows agent.dispatch(block.name, block.input) in a production LLM loop but there's no indication of what happens when block.name doesn't match any @tool method. Does it raise KeyError? A descriptive ToolNotFoundError? The LLM loop will crash silently in an unclear way.

6. new_session() concurrency
If Node9Agent is used in a server context (FastAPI, FastMCP), multiple requests sharing an agent instance will race on _run_id. The README mentions new_session() for this but it's not shown — confirm it's thread/task-safe and document whether agents should be per-request or shared.

7. safe_path return type inconsistency
safe_path raises ValueError in manual_test.py but the @tool wrapper catches it and re-raises as ActionDeniedException. The public standalone safe_path utility raises ValueError. These are different exception types for the same logical failure depending on call site — this will confuse callers who use safe_path directly.

8. _build_tools() parameter introspection on *args/**kwargs
If a @tool method accepts **kwargs (e.g., for forward compatibility), the introspection will produce a broken tool spec. This needs a guard or documented restriction.

---

API Design

9. @tool("name") vs @tool dual-mode decorator
The implementation def tool(tool_name: Union[str, Callable]) is a common pattern but fragile — if someone accidentally passes a non-string non-callable, the error message will be confusing. Add an explicit type check with a clear error.

10. policy as a class attribute is surprising
Node9Agent subclasses set policy = "audit" as a class variable, but configure(policy=...) sets a module-level variable. These can silently conflict. Document clearly which takes precedence and consider raising if both are set to different values.

11. @internal is undiscoverable
There's no enforcement that @internal methods must start with _. The convention is stated in docs but not enforced. A developer could accidentally mark a public method @internal and bypass all governance with no warning.

---

Test Coverage Gaps

12. manual_test.py is committed to the repo root
This should be in tests/ or examples/, not the project root. More importantly, the automated test suite should cover: DLP scan on nested/non-string args, dispatch() with unknown tool name, configure() called after first @protect fires, and safe_path symlink traversal (a symlink inside the workspace pointing outside it).

13. No test for cloud routing fallback ordering
The routing table (API key → daemon → offline) is a core security property. There should be a unit test that mocks each condition and verifies the correct backend is selected — especially that a misconfigured NODE9_API_KEY (set but invalid) doesn't silently fall through to offline/auto-approve mode.

---

Minor

• The indentation in the README CiAgent example is wrong — return subprocess.check_output(...) is outdented relative to the method body.
• __version__ = "2.0.0" is a major version bump with no migration guide or changelog referenced.
• The # Node9Agent methods (documented here for IDE discoverability) comment block in __init__.py is a code smell — these should be actual imports or omitted.

⚠️ 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

[node9ai]

🤖 Claude Code Review

Node9 Python SDK — Code Review

Security Issues (High Priority)

1. _agent.py — dispatch() trusts LLM-controlled tool names without sanitisation
The dispatch() method routes by tool_name from the LLM response. If an attacker can influence the tool name (prompt injection), they may not reach a malicious method since only @tool-decorated methods are registered — but confirm _tool_registry is built at class definition time and is not mutable at runtime. If new_session() or any other method rebuilds the registry, verify it can't be poisoned.

2. _agent.py — @tool DLP scan is explicitly documented as incomplete
The docstring acknowledges that secrets nested inside dicts or lists are not caught. For a security library, this is a meaningful gap. dispatch() receives tool_input as a dict directly from the LLM — all values should be recursively scanned before they reach the DLP layer, not just top-level strings. The current design means structured tool calls (which are standard in OpenAI/Anthropic function calling) silently bypass DLP.

3. safe_path — no symlink resolution
If safe_path uses os.path.abspath without os.path.realpath, a symlink inside the workspace pointing outside it will pass the traversal check. Must call os.path.realpath() on the resolved path before comparing against the workspace prefix.

4. manual_test.py — sensitive path check uses a hardcoded absolute path

dlp_scan("/home/user/.ssh/id_rsa", "content")

This is fine as a smoke test, but if DLP matching is prefix/substring-based on the full path string, a relative path like id_rsa or keys/id_rsa may evade it. The test should also cover relative sensitive paths.

5. README — NODE9_SKIP=1 is documented at top level
Documenting a global bypass flag prominently in the README is a risk. At minimum, add a clear warning that this env var must never be set in production and should be guarded in CI configurations.

---

Correctness and Edge Cases

6. _agent.py — tool decorator dual-use signature is fragile
tool accepts either a string or a callable (bare @tool vs @tool("name")). This pattern is error-prone: if someone passes a non-string, non-callable (e.g. @tool(None) or @tool(42)), the failure mode is unclear. Add an explicit type guard with a descriptive error.

7. _agent.py — inspect.signature on wrapped methods
@functools.wraps preserves __wrapped__. If a method is decorated multiple times (e.g. @tool + a framework decorator applied later), sig.bind() may bind against the wrong signature. Consider using inspect.signature(fn, follow_wrapped=False) explicitly.

8. configure() thread safety claim vs implementation
The docstring says "Thread-safe" but this depends entirely on _config.set_identity(). If that function does a read-modify-write on module-level globals without a lock, the claim is false. This is worth verifying given LangGraph's async/threaded execution model.

9. @internal bypasses all governance but still logs locally
The README states it "Logs locally only" — verify the implementation actually does this. If a bug causes @internal to silently swallow errors without logging, infrastructure failures will be invisible.

---

API Design

10. dispatch() error handling for unknown tool names
What happens when the LLM hallucinates a tool name not in _tool_registry? It should raise a clear, catchable exception (not a KeyError). This is a common LLM failure mode and callers need to handle it gracefully in their loop.

11. new_session() semantics are underdocumented
The README mentions it for "server/multi-session deployments" but doesn't explain whether it's safe to call mid-session, whether it affects in-flight tool calls, or whether it's goroutine/thread-safe.

12. Version bump to 2.0.0 without a CHANGELOG entry
This is a major version bump introducing a new base class, new decorators, and new routing behaviour. There should be a migration note for users on 0.1.x.

---

Test Coverage Gaps

• No test for dispatch() with an unknown tool name
• No test for @tool on an async method within Node9Agent
• No test for DLP on nested dict values (the known gap)
• No test for safe_path with symlinks
• No test verifying configure() is respected when called after module import but before first @protect invocation (race condition in lazy initialisation)
• manual_test.py is a smoke test, not a regression suite — it has no assertions, only print statements. A failed check prints "(BUG)" but exits with code 0, meaning CI won't catch regressions.

---

Minor

• build_tools_anthropic() / build_tools_openai() are mentioned in __init__.py comments but not exported — this is correct since they're instance methods, but the comment placement is slightly misleading.
• The README example uses claude-opus-4-6 which is not a real model name — fix before publishing.

⚠️ 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

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK PR

Security Issues (Critical)

safe_path called with positional argument in write_code docstring example:
The module-level docstring shows safe_path(filename, self._workspace) but the README shows safe_path("src/main.py", workspace="/tmp/repo") as a keyword arg. If the actual signature is safe_path(filename, workspace) positionally, this is fine — but the inconsistency suggests the API isn't settled and one of these callers could silently pass args in the wrong order. Pin down the signature and make it keyword-only for workspace.

run_tests in the docstring example uses shlex.split(command) with user-controlled input:
This is better than shell=True, but shlex.split on agent-controlled input still allows arbitrary command construction. The README example has the same pattern. For a security library, the docs should at minimum warn that command here is agent-controlled and that callers should validate/allowlist commands before passing them to subprocess. As-is, this is a demonstration of how to escape shell injection while still allowing full command execution — that's a meaningful distinction worth calling out.

NODE9_SKIP=1 bypass:
The env var that disables all checks is documented and ships in the SDK. There's no warning that this should never be set in production environments, only "for unit tests only" in a table footnote. Consider making the behavior louder — e.g., emitting a warnings.warn at import time if NODE9_SKIP is set, so it's harder to accidentally leave enabled.

---

Correctness and Edge Cases

new_session() thread safety:
The docstring says new_session() is for "server/multi-session deployments" but if two concurrent requests call new_session() on the same agent instance, the _run_id assignment is a race. A shared Node9Agent instance in a web server would have this problem. Either document "one instance per request" explicitly or use a threading.local.

dispatch() with unknown tool name:
What happens when dispatch(name, input) is called with a name that doesn't match any @tool method? If it raises a KeyError or AttributeError, LLM loops will get an unhandled exception instead of a clean error. It should raise a descriptive, catchable error (ideally ActionDeniedException or a new UnknownToolError).

_build_tools() type inference:
Auto-generating schemas from annotations is convenient, but Union types, Optional, List[str], and unannotated parameters all need handling. If any @tool method has a parameter typed as dict or list, the generated schema may be wrong or empty, silently producing bad tool specs sent to the LLM.

safe_path with symlinks:
Path traversal via symlinks is a classic bypass — os.path.realpath or Path.resolve() should be used, not just checking for ../ in the string. Confirm the implementation uses resolve() and checks the resolved path is within workspace, not just the input string.

---

API Design

@tool("name") vs @tool without argument:
The decorator takes a string name, but the method already has a name. This is redundant boilerplate for the common case. Consider making the name optional and defaulting to the method name, like @protect does.

configure() is not idempotent-safe:
If a framework calls configure() twice (e.g., during hot reload), the second call silently overwrites identity. There's no warning. This could cause audit logs to misattribute tool calls mid-session.

build_tools_anthropic() / build_tools_openai() as separate methods:
This will need a new method every time a new LLM provider appears. A build_tools(format="anthropic") pattern would be more maintainable and easier to document.

---

Test Coverage Gaps

• No tests for dispatch() with an unknown tool name
• No tests for concurrent new_session() calls
• No tests for @tool methods with unannotated or complex-typed parameters in _build_tools()
• No tests verifying NODE9_SKIP=1 actually bypasses evaluation (important to confirm the bypass works predictably in CI, even if the behavior is dangerous in prod)
• No tests for symlink-based path traversal in safe_path
• configure() called twice — no test verifying the second call wins and logs/warns appropriately

---

Minor

The version jump from 0.1.1 to 2.0.0 in __init__.py is a breaking change signal — make sure CHANGELOG and PyPI release notes reflect this, especially since DaemonNotFoundError and existing @protect behavior may have changed semantics with the new routing table.

The manual_test.py in examples/ printing (BUG) on bad paths is a nice pattern, but it's not part of the automated test suite — it should either be converted to a pytest integration test or clearly marked as manual-only in CI configuration so it's never accidentally run as a test.

⚠️ 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

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK PR

Security Issues (High Priority)

1. safe_path not enforced for all file-touching @tool methods
The DLP scan in _agent.py flags sensitive paths, but path traversal protection only triggers when filename is in DLP_PATH_PARAMS. If a developer names their parameter filepath, dest, or output, traversal protection silently doesn't apply. This is a security library — path safety should not depend on parameter naming conventions the user might not know about.

2. @tool dispatch trusts arbitrary string input from LLM
dispatch() looks up the method by name from _tools. If there's any way for tool_name to be influenced without going through the registry (e.g., future refactors, monkey-patching), this becomes a problem. Verify the lookup is strictly against the decorated registry and not getattr-based.

3. shlex.split in README example doesn't prevent all injection
The README shows subprocess.check_output(shlex.split(command), text=True) as a safe pattern. This is better than shell=True, but shlex.split of arbitrary LLM-generated strings can still produce valid but dangerous argument lists (e.g., --upload-pack, path arguments to git). This example will be cargo-culted by users — it needs a stronger warning or a safer pattern.

4. NODE9_SKIP=1 bypass is in the env var table
Documenting NODE9_SKIP in the README tells adversaries exactly how to disable the security layer. If an attacker can set env vars (e.g., via a compromised .env file, a prompt injection writing to the environment), they can bypass all governance. At minimum, add a warning that this must never be set in production and consider whether it should be documented at all.

---

Correctness and Edge Cases

5. configure() is documented as "thread-safe" but the implementation needs verification
The docstring says "Thread-safe — safe to call from concurrent async frameworks." If _config.set_identity() is doing a simple module-level variable assignment, it is not guaranteed thread-safe under PyPy or with certain interpreter implementations. This claim needs to be backed by a lock or the claim should be removed.

6. new_session() behavior in concurrent deployments
The README mentions .new_session() for server/multi-session deployments but the method isn't demonstrated in any test or example. If two concurrent requests call dispatch() between a new_session() call, the run_id could be wrong for one of them. This needs a documented threading model or per-request agent instantiation guidance.

7. _build_tools() introspection of @tool methods
Auto-generating schemas from annotations is convenient but fragile. *args, **kwargs, parameters with Union types, or missing annotations will either silently produce a bad schema or raise at runtime during an LLM session. There's no validation at class definition time — errors will surface only when build_tools_* is called, potentially mid-session.

---

Test Coverage Gaps

8. No tests for Node9Agent
The entire new Node9Agent, @tool, and @internal surface has no automated tests in this diff. The manual_test.py is a smoke test, not a test suite. Missing critical coverage:

• dispatch() with an unknown tool name
• @tool DLP block raises ActionDeniedException (not a generic exception)
• build_tools_anthropic() vs build_tools_openai() schema correctness
• new_session() produces a fresh run_id
• @internal confirms evaluate() is never called

9. No test for configure() interaction with env vars
If NODE9_AGENT_POLICY is set in the environment and configure(policy="audit") is called, which wins? This precedence is undocumented and untested.

---

API Design

10. _build_tools() vs build_tools_anthropic() naming inconsistency
Public methods use build_tools_anthropic() / build_tools_openai() (no underscore). The neutral format is _build_tools() (private). If _build_tools() is meant to be used by framework integrators, it should be public. If it's truly internal, it shouldn't appear in the README or __init__.py comments.

11. Node9Agent is a base class with no abstract methods
Nothing stops a developer from instantiating Node9Agent() directly with no tools and calling dispatch(), which would succeed vacuously. Consider a __init_subclass__ check or at minimum document the expected subclassing pattern more explicitly in the class docstring.

---

Minor

• The version jump from 0.1.1 → 2.0.0 with no migration guide or changelog in this diff is a breaking change signal — if there are existing users of the 0.1.x API, they need a migration path documented before this merges.
• The pre-commit hook uses python3 hardcoded; in virtualenv-based CI this may resolve to the wrong interpreter. Consider $(which python) or checking VIRTUAL_ENV.

⚠️ 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

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK

Security Issues (High Priority)

_agent.py — dispatch() accepts arbitrary string keys
The dispatch() method routes by tool name from LLM output directly into self._tools dict lookup. If _tools is ever populated with unexpected keys (e.g., via subclass overrides of __init__), an LLM could potentially invoke unintended methods. At minimum, validate that the resolved callable has the _node9_tool = True sentinel before calling it, not just that the key exists in _tools.

_agent.py — @internal bypass is invisible to callers
@internal methods skip evaluate() entirely with only a local log. There's no mechanism preventing a subclass from accidentally marking a genuinely dangerous method as @internal. Consider documenting this more aggressively in the decorator itself (a runtime warning if the method name doesn't start with _ would catch the obvious case).

README run_shell example — shlex.split improvement is good but incomplete
The fix from shell=True to shlex.split is a real improvement. However, the warning comment says it "prevents shell injection" — shlex.split does not prevent shell injection in the sense that shell=True enables; it just avoids invoking a shell interpreter. The comment is misleading. The actual risk (arbitrary executable invocation) is mentioned, but the framing is inaccurate and could give integrators false confidence.

NODE9_SKIP warning on import
The docs say a warning is emitted at import time when NODE9_SKIP=1. Verify this is actually enforced in _config.py — the diff doesn't show that module, so it can't be confirmed. If it's missing, NODE9_SKIP silently disables all governance with no signal, which is a significant risk in misconfigured prod environments.

---

Correctness and Edge Cases

_agent.py — build_tools_anthropic() / build_tools_openai() not in diff
These methods are referenced extensively in the README and __all__ comments but aren't visible in the truncated diff. Can't verify their correctness. The schema generation from type annotations and docstrings needs careful review — particularly handling of Optional, Union, default values, and missing annotations, which are common in real agent code.

_agent.py — new_session() thread safety
The README mentions new_session() for "server/multi-session deployments." If _run_id is an instance variable this is fine, but if it's class-level (shared across instances), concurrent agents would clobber each other's run IDs, breaking audit trail grouping. This needs verification.

configure() race condition
configure() is documented as "thread-safe" but calls _config.set_identity() which isn't in the diff. If set_identity does a non-atomic read-modify-write on module globals, this claim is false. In async frameworks like LangGraph this matters.

safe_path — symlink escape
safe_path blocks ../ traversal but the manual test only checks string-based traversal. If the workspace directory contains symlinks, os.path.realpath (or Path.resolve()) is needed after joining to catch symlink-based escapes. The diff doesn't show the _dlp.py implementation so this can't be confirmed.

---

API Design

dispatch() error UX
When an unknown tool name is passed (e.g., LLM hallucinated a tool name), what happens? A KeyError from a dict lookup is a poor error for integrators. Should raise ActionDeniedException or a descriptive ValueError with the tool name and list of available tools.

configure() call ordering
The docstring says "Call before the first evaluate()." If called after, behavior is undefined. Consider raising a RuntimeError if configure() is called after any evaluate() has run, or at least document what happens (does it take effect immediately, or only for subsequent calls?).

Version bump to 2.0.0
Jumping from 0.1.1 to 2.0.0 is a significant signal. Confirm this is intentional and that PyPI release notes will document the breaking changes clearly, especially since Node9Agent, tool, and internal are all new public API surface.

---

Test Coverage Gaps

• No tests visible for Node9Agent.dispatch() with unknown tool names
• No tests for build_tools_anthropic() / build_tools_openai() schema correctness
• No concurrency test for configure() thread-safety claim
• No test verifying NODE9_SKIP=1 emits a warning
• new_session() not tested for isolation between instances

---

Minor

The pre-commit and pre-push hooks are functionally identical. Consider a shared script they both source to avoid drift. The 2>&1 redirect on the pytest invocation suppresses stderr to stdout — fine for display, but means test output ordering may interleave unexpectedly in some terminals.

⚠️ 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

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK PR

Security Issues (High Priority)

run_shell example still dangerous. The README example with shlex.split(command) and subprocess.check_output is better than shell=True, but the comment says "does NOT prevent the LLM from running arbitrary executables." This is still a genuinely dangerous example in a security library's README. Consider replacing it with something that doesn't demonstrate arbitrary command execution at all, or gate it behind an explicit allowlist example. New users will copy this pattern.

NODE9_SKIP warning at import time is good — but verify it actually fires. If _config is imported lazily or the warning is only emitted when protect is first called, a developer could set NODE9_SKIP=1 and not see it until runtime. The PR doesn't show the warning implementation.

dispatch() unknown tool handling — the diff is truncated, but dispatch(name, input) must raise a clear, non-swallowable error for unknown tool names. If it silently returns None or a falsy value, LLM tool loops will get confused results without any governance firing. This is a correctness and security issue.

@internal trust model needs documentation. The README says @internal "never calls evaluate()." That's fine for infra, but there's no mechanism preventing a subclass author from accidentally (or maliciously) marking a dangerous method as @internal. Consider at least a lint-level warning or naming convention enforcement.

---

API Design Issues

configure() threading claim is unverified. The docstring says "Thread-safe — safe to call from concurrent async frameworks." But if _config.set_identity just does module-level variable assignment without a lock, this claim is false. GIL protects simple assignments in CPython, but this is not guaranteed across implementations and doesn't hold if set_identity does a read-modify-write. Remove the thread-safety claim or back it with a lock.

new_session() is undocumented in the diff. It's mentioned in __all__ comments but never shown. For server/multi-session deployments this is critical — if callers share a run_id across sessions, audit logs are corrupted. This needs a test and documentation.

safe_path raises ValueError but write_file in Node9Agent raises ActionDeniedException. The manual_test.py confirms this inconsistency — safe_path standalone raises ValueError, but through @tool it becomes ActionDeniedException. Callers wrapping safe_path directly get a different exception type. Standardize on ActionDeniedException.

build_tools_anthropic() / build_tools_openai() are listed in comments but not shown. These are high-visibility integration points. Any mismatch with actual provider schemas (e.g., Anthropic requires input_schema, not parameters) will silently produce broken tool calls. These need tests against the actual schema shapes.

---

Test Coverage Gaps

• No test for configure() racing with @protect being called concurrently.
• No test for dispatch() with an unknown tool name.
• No test for new_session() generating distinct run_id values.
• No test that NODE9_SKIP=1 emits a warning (not just skips governance).
• No test for @tool on an async def method — the README claims async works, but Node9Agent async dispatch isn't shown.
• DLP tests for the full pattern list (AWS keys, GCP SA, PEM keys) aren't visible. If dlp_scan uses regex, false-negative edge cases (e.g., keys with slight formatting variation) need coverage.
• No test that build_tools_anthropic() output is rejected vs. accepted by the Anthropic client schema validator.

---

Daemon / HTTP Communication

Version bump to 2.0.0 without a migration section is a red flag. If the daemon protocol changed between 0.1.1 and 2.0.0, existing users running the old @node9/proxy daemon will get silent failures or incorrect routing. Document the protocol compatibility guarantee.

Offline fallback to ~/.node9/audit.log — what happens if that path isn't writable (containerized environments, read-only filesystems)? If it swallows the write error, you get silent audit loss with no indication to the developer.

---

Minor / Positive Notes

• Git hooks setup is clean and the run-tests.sh abstraction is correct.
• Routing table in the README is a meaningful usability improvement.
• shlex.split over shell=True is the right direction even with the caveats noted above.
• @internal local-only logging is a good separation of concerns.
• The manual_test.py explicitly printing (BUG) on unexpected paths is a nice pattern.

⚠️ 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

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK

Security Issues (Critical)

1. safe_path raises ValueError, not ActionDeniedException
The README and manual test both expect ActionDeniedException for path traversal when called via @tool. But safe_path itself raises ValueError. If someone calls safe_path directly outside a @tool context, they get the wrong exception type. The manual test explicitly checks except ValueError for the standalone case but except ActionDeniedException via dispatch — make sure the @tool wrapper actually catches and re-raises as ActionDeniedException, and document that the raw utility throws ValueError.

2. NODE9_SKIP bypass is dangerous with weak guardrails
The README says "a warning is emitted at import time" but this needs to be verified in _config.py. If the warning is only a print() and not a warnings.warn(..., stacklevel=2), it won't surface in environments that suppress stdout. Given this is a security library, a skipped check should be loud. Confirm the warning goes through the warnings module.

3. dispatch() method — no input sanitization before DLP
The diff shows agent.dispatch(block.name, block.input) where block.input is a raw dict from the LLM. If the @tool decorator only scans known parameter names (filename, content), a tool with differently-named parameters carrying secrets could bypass DLP. Verify DLP scans all string values in the input dict, not just fields named filename/content.

4. shlex.split() on LLM-controlled input is still dangerous
The CiAgent example in the README and _agent.py docstring shows shlex.split(command) where command comes from the LLM. shlex.split handles quoting but the resulting list is still passed to subprocess.check_output. An allowlist approach (like the _ALLOWED_SUITES set) is the right pattern — but the docstring example without an allowlist is a footgun that developers will copy. Remove or explicitly warn against the bare shlex.split example.

Correctness / Edge Cases

5. Node9Agent._build_tools() introspection on inherited methods
If a subclass inherits from another subclass of Node9Agent, @tool-decorated methods on intermediate classes may or may not appear in _build_tools() depending on how __dict__ vs inspect.getmembers is used. This needs a test covering multi-level inheritance.

6. new_session() thread-safety
The README mentions new_session() for "server/multi-session deployments." If _run_id is an instance attribute and Node9Agent instances are shared across threads (common in async frameworks), there's a TOCTOU race on session rotation. Clarify whether Node9Agent instances are expected to be per-request or long-lived.

7. configure() called after first @protect use
The docstring says "Call before the first evaluate()." But there's no enforcement — if called mid-session, AGENT_NAME/AGENT_POLICY changes silently. At minimum, log a warning if called after any tool has already been evaluated.

API Design

8. build_tools_anthropic() vs build_tools_openai() vs _build_tools()
The underscore-prefixed _build_tools() is exposed in __all__ comments and the manual test. Either make it public (no underscore) or remove it from examples. Leaking "private" methods as the canonical neutral format is confusing.

9. @internal logging only
The README says @internal logs: [node9 internal] _git_push(branch='main'). If this goes to stdout/print, it will break LLM parsers that consume agent stdout. Confirm it uses logging.getLogger("node9") rather than print.

10. Version bump to 2.0.0 with no migration guide
The diff goes from 0.1.1 to 2.0.0. This is a breaking public API change (Node9Agent, new configure(), changed env vars). The README doesn't include a migration section for existing 0.1.x users. Add one, or at minimum a CHANGELOG.md.

Test Coverage Gaps

• No test for configure() called concurrently from multiple threads
• No test for dispatch() with an unknown tool name (should raise, not silently fail)
• No test for @tool on an async def method in Node9Agent
• No test that NODE9_SKIP=1 actually emits a warning (not just skips)
• No test for build_tools_anthropic() / build_tools_openai() schema shape validation

Git Hooks

The pre-commit hook uses source run-tests.sh which means failures in the script call exit 1 in the current shell — correct. But if a developer isn't using python3 by name (e.g., python on Windows with WSL, or a Conda env without VIRTUAL_ENV set), command -v python3 may resolve to the wrong interpreter. Consider adding a python3 --version sanity check with a clear error message.

Summary

The main concerns in priority order: DLP scope in dispatch(), the shlex.split footgun in documentation, NODE9_SKIP warning reliability, and the missing dispatch() unknown-tool-name test. The API surface is clean and the routing table is a good UX improvement.

⚠️ 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

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK

Security Issues (High Priority)

_agent.py — write_code example writes to arbitrary paths
The README's Node9Agent example shows write_code opening filename directly with open(filename, "w") before DLP/path-safety runs. If DLP is checked inside @tool but after the file handle is opened, there's a window. More critically, the README example doesn't use safe_path at all — it will write to absolute paths. This is a documentation-level security hole that developers will copy verbatim.

_agent.py — dispatch() unknown tool name behavior
If an LLM hallucinates a tool name not in @tool-decorated methods, dispatch() presumably raises a KeyError or similar. That raw exception should not propagate to the LLM loop — it should return a structured error or raise ActionDeniedException. A misbehaving/jailbroken LLM could use unknown tool names to probe the agent's internals via error messages.

NODE9_SKIP warning is emitted at import time, not at bypass time
The README says "a warning is emitted at import time" when NODE9_SKIP=1. Import-time warnings are easy to miss in test output. The warning should also fire at each skipped evaluate() call, so audit logs are never silently empty in a misconfigured production deployment.

run_tests.sh — sourceing a script that calls exit 1
When run-tests.sh is sourced (not executed) by the hooks, exit 1 will terminate the parent shell, not just the script. This works for git hooks but is fragile — if someone sources it interactively to debug, it kills their shell session. Use return 1 in a sourced script.

---

Correctness & Edge Cases

safe_path with symlinks
safe_path likely uses os.path.abspath or Path.resolve() before the file exists. If an attacker pre-creates a symlink inside the workspace pointing outside it, resolve() will follow the symlink and the check passes — then the tool writes outside the workspace. You need to check the resolved path of the parent directory that exists, or stat after write.

dlp_scan return type inconsistency
manual_test.py shows dlp_scan returning None for clean content and a string reason for hits. If the return type is Optional[str], callers must always check for None. Consider returning a typed result object or raising directly — silent None returns are easy to forget to check, which is catastrophic for a security library.

configure() thread safety claim
The docstring says "Thread-safe" but the implementation presumably just calls _config.set_identity() which does module-level attribute assignment. In CPython this is GIL-protected for simple assignments, but the claim is fragile across implementations and should either be backed by a threading.Lock or the claim removed.

Node9Agent with new_session()
If a server reuses one Node9Agent instance across concurrent requests (common in FastAPI/LangGraph deployments), new_session() replaces _run_id on the shared instance. Concurrent requests will share or race on _run_id. This needs to be documented explicitly as "not safe for concurrent use on a shared instance" or the session ID needs to be thread-local/contextvar-scoped.

---

API Design

build_tools_anthropic() vs build_tools_openai() vs _build_tools()
Having a private _build_tools() as the "neutral" format while the others are public is inconsistent. If _build_tools() is documented in __init__.py comments and shown in manual_test.py, it should be public. Underscored names signal "don't use this" to integrators.

@tool("echo") requires repeating the method name as a string
This is a footgun — @tool("writ_file") typo on a method named write_file silently misregisters the tool. Consider making the name argument optional and defaulting to func.__name__, matching the pattern from @protect.

ActionDeniedException fields in manual_test.py
e.tool_name and e.reason are accessed but these fields aren't in the public API surface documented in __init__.py. If these are stable public fields, add them to __all__ or document them explicitly.

---

Test Coverage Gaps

• No test for concurrent dispatch() calls on a shared agent instance
• No test for dispatch() with an unknown/hallucinated tool name
• No test for dlp_scan with a file that has a clean path but sensitive content and a sensitive path (both triggers simultaneously)
• No test for safe_path with symlinks inside the workspace
• No test verifying NODE9_SKIP=1 still emits a warning
• No test for configure() called after the first @protect call (race/ordering issue)
• The pre-commit hook blocks on test failures but there's no test that the hook itself is wired correctly (trivial, but worth a smoke test in CI)

---

Minor

The version jump from 0.1.1 → 2.0.0 in __init__.py is a breaking change signal — make sure CHANGELOG and PyPI release notes reflect the removed env vars (NODE9_AUTO_START behavior change) and the new Node9Agent API surface, since existing @protect-only users will hit this on pip upgrade.

⚠️ 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

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK

Security Issues (Critical)

1. NODE9_SKIP bypass is a single env var away from disabling all governance
The README warns "Never set in production" but there's no defense-in-depth. A compromised environment or misconfigured CI pipeline silently disables everything. Consider requiring a cryptographic token or a specific non-guessable value rather than NODE9_SKIP=1.

2. dispatch() has no input size limit
An LLM could pass arbitrarily large tool_input dicts to dispatch(). Before DLP scanning or routing to the daemon, there's no cap on payload size. A malicious or runaway agent could cause memory exhaustion or inflate audit logs.

3. _build_tools() introspection trusts docstrings as security-relevant descriptions
Tool descriptions sent to the LLM come directly from __doc__. If a developer writes a misleading docstring, the LLM gets a false picture of what the tool does. This is a social-engineering vector within the agent loop. At minimum, flag this risk in documentation.

4. run_shell example in README was correctly fixed, but the old pattern lingers in muscle memory
The README improvement (shell=True → allowlist) is good. Ensure the examples in examples/ directory (not shown) don't reintroduce shell=True.

---

Correctness & Edge Cases

5. safe_path raises ValueError but @tool catches ActionDeniedException
In manual_test.py line ~75, the traversal test catches ValueError. But when traversal goes through a @tool-decorated method, the @tool wrapper should be converting that ValueError to ActionDeniedException. Verify the wrapping logic handles this consistently — if it doesn't, traversal failures inside @tool methods will propagate as uncaught ValueError to the LLM loop, not as catchable ActionDeniedException.

6. _ALLOWED_SUITES uses string matching for subprocess commands
In CiAgent, shlex.split(suite) is called on the full string "pytest --tb=short". This is safe only because the set is hardcoded. But the pattern teaches users that splitting trusted strings into subprocess args is acceptable, which is dangerous to generalize. The README example deserves a comment clarifying this is safe only because the strings are compile-time constants.

7. configure() is not idempotent-safe in concurrent scenarios
The docstring says "Thread-safe" but _config.set_identity() isn't shown. If it's a simple attribute assignment, concurrent calls from async frameworks (LangGraph with multiple coroutines) could race. Needs a lock or should document that it must be called once before any concurrency starts.

8. new_session() on Node9Agent — not shown but referenced
The __all__ comment references .new_session() for "server/multi-session deployments." If _run_id is a class-level or instance-level UUID, server deployments sharing one agent instance across requests will cross-contaminate audit logs. This pattern needs explicit documentation or an explicit guard.

---

Daemon HTTP Communication

9. No timeout shown on daemon HTTP calls
The diff doesn't show the HTTP client code, but the original README described blocking HTTP calls run in a thread. If the daemon is slow or unresponsive, @protect will hang indefinitely. There should be a documented (and enforced) timeout, and the fallback behavior when timeout occurs needs to be explicit — does it fail open or fail closed? For a security library, fail-closed is the only acceptable default.

10. Offline fallback is auto-approve — this is dangerous and under-documented
The routing table says "offline → auto-approve, never blocks." This means a misconfigured production environment (API key accidentally unset) silently degrades to no governance. The warning at import time for NODE9_SKIP=1 is good — the same warning should fire when running in offline mode, especially if NODE9_AGENT_POLICY is require_approval.

---

API Design

11. Version jump from 0.1.1 → 2.0.0 without migration guide
This is a breaking change (new Node9Agent, configure(), changed routing). The README doesn't include a migration section for existing @protect-only users. Add one.

12. _build_tools() is public-ish via __all__ comments but prefixed with _
The comment in __all__ documents .build_tools_anthropic() and .build_tools_openai() as the public API, but also shows ._build_tools(). This inconsistency will confuse integrators. Either expose it without underscore or remove it from the documentation.

---

Test Coverage Gaps

• No test for offline fallback when both daemon and API key are absent — especially that it emits a warning under require_approval policy
• No test for configure() called after the first @protect has already fired (late configuration)
• No test for dispatch() with an unknown tool name (should it raise KeyError, ValueError, or ActionDeniedException?)
• No test for new_session() behavior in the concurrent/server case

---

Minor

• examples/manual_test.py uses sys.path.insert(0, ...) — fine for a manual script, but it shouldn't be picked up by pytest. Confirm it's excluded in pytest.ini / pyproject.toml.
• The run-tests.sh set -euo pipefail with source is correct and the return vs exit comment is a good callout. No issues there.

⚠️ 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

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK

Security Issues (High Priority)

crewai_agent.py — Decorator ordering bug

@tool("run_shell")
_ALLOWED_COMMANDS = {"pytest", "ruff check .", "mypy src/"}  # ← assignment between decorators
@protect("bash")
def run_shell(command: str) -> str:

This is broken Python. The @tool decorator is applied to the set literal _ALLOWED_COMMANDS, not the function. The @protect decorator then wraps run_shell alone, meaning CrewAI never sees run_shell as a registered tool. This silently defeats the whole integration. Move _ALLOWED_COMMANDS above the decorators.

safe_path — Missing enforcement in Node9Agent.write_file example
The README and manual_test.py show @tool("write_file") using pathlib directly without calling safe_path, while the docstring claims path safety is automatic for @tool. If path traversal checks aren't enforced by the @tool decorator itself (i.e., they only happen when safe_path is called manually), the README's claim that @tool provides "path safety — rejects ../ traversal" is misleading and dangerous. This needs to be either enforced in the decorator or the documentation corrected.

NODE9_SKIP bypass — no test coverage
The README warns NODE9_SKIP=1 disables all governance, but there's no visible test asserting that a warning is actually emitted at import time when it's set. Given this is a security library, that behaviour must be tested.

---

Correctness Issues

run-tests.sh — Version comment mismatch

PYVER=$("$PYTHON" -c 'import sys; print(sys.version_info >= (3,10))' 2>/dev/null)
if [[ "$PYVER" != "True" ]]; then
  echo "❌ node9: Python 3.10+ required ...

The script header says # Sanity-check: require Python 3.9+ but the check enforces 3.10+. Pick one and be consistent — this will silently reject 3.9 environments if the library claims 3.9+ support.

shlex.split on allowlisted multi-word commands
Commands like "git log --oneline" and "ruff check ." are allowlisted as full strings but then split with shlex.split. This works, but it means the allowlist check and the executed command are in different representations. An LLM sending "git log --oneline" (double space) passes the allowlist check but splits identically — low risk here but worth a comment explaining the design intent.

---

API Design Issues

build_tools_anthropic() / build_tools_openai() not shown being tested
The README documents three build_tools_* methods but the manual test and (presumably) unit tests don't exercise them. If the introspection-based schema generation breaks for methods with default arguments, *args, or no type annotations, developers will hit runtime errors when integrating.

dispatch() error handling is undocumented
What does agent.dispatch("nonexistent_tool", {}) return or raise? The README shows the happy path only. For LLM feedback loops, this matters — if an LLM hallucinates a tool name, the error needs to be catchable and feedable back as a tool_result.

@internal log output goes to... where?
The README says @internal "logs locally only" but doesn't say where — stdout, a file, the audit log? Developers integrating this need to know if internal calls will pollute their agent's stdout in production.

---

Test Coverage Gaps

• No test for the crewai_agent.py decorator ordering (the bug above goes undetected)
• No test that NODE9_SKIP=1 emits a warning
• No test for build_tools_anthropic() / build_tools_openai() schema correctness
• No test for dispatch() with an unknown tool name
• No test for @tool + @protect interaction when the daemon is unreachable (timeout behaviour)

---

Minor

The manual_test.py file uses sys.path.insert(0, ...) to import from source — fine for a manual script, but it shouldn't be committed to examples/ where users might copy it and get confused why it works from the repo root but not after pip install node9.

Summary: The crewai_agent.py decorator ordering is a functional bug that silently breaks the CrewAI integration. The @tool path safety claim vs. actual enforcement needs clarification — if it's not automatic, that's a security documentation issue. Everything else is addressable but lower priority.

⚠️ 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

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK

Security Issues

Critical: shlex.split on allowlisted strings is still risky
In examples/basic.py and examples/crewai_agent.py, strings like "git log --oneline" pass the allowlist then get split via shlex.split and passed to subprocess.check_output. This is fine today because the allowlist is hardcoded, but the pattern teaches users that allowlist + shlex.split = safe subprocess. It's a footgun if someone adapts this with a less strict allowlist. A comment clarifying "the allowlist is the safety net, shlex.split is not" would help.

NODE9_SKIP warning at import time
The README says "a warning is emitted at import time" when NODE9_SKIP=1. This is a security-critical guarantee — verify this is actually implemented in the SDK source. If it's doc-only and not enforced in code, that's a documentation lie in a security library.

safe_path raises ValueError, not ActionDeniedException
The manual test catches ValueError for traversal attempts. For a security library, path traversal should raise ActionDeniedException (or a subclass of it) so callers have a single exception contract. Using ValueError means an agent catching only ActionDeniedException silently swallows path traversal and proceeds.

DLP in manual_test.py passes /home/user/.ssh/id_rsa as the filename argument
dlp_scan("/home/user/.ssh/id_rsa", "content") — if DLP only scans content and uses the filename for pattern matching separately, this test doesn't verify content scanning. Make sure the test covers actual secrets in the content string, not just sensitive filenames.

---

Correctness and Edge Cases

Node9Agent.dispatch error handling is unspecified
The README shows agent.dispatch(block.name, block.input) in the LLM loop with no error handling around it. If dispatch raises ActionDeniedException, the tool result is never appended to messages, and the LLM loop continues with a missing tool result — likely causing a protocol error with Anthropic's API. The example should show catching this and returning the denial as a tool result.

build_tools_anthropic() vs _build_tools() naming inconsistency
build_tools_anthropic() and build_tools_openai() are public; _build_tools() is private (underscore prefix). If _build_tools() is the framework-neutral format and meant for users, it should be public. If it's internal, don't surface it in README examples.

@tool docstring → description pipeline is untested
The README claims tool specs are auto-generated from docstrings. There's no test covering what happens with no docstring, a multiline docstring, or a docstring with special characters. This is part of the public API contract.

---

Git Hooks

source + set -euo pipefail interaction
run-tests.sh uses set -euo pipefail and is sourced. If pytest is not installed, python -m pytest exits non-zero and set -e triggers return 1 — but this also means any unrelated error in the sourced script (e.g., command -v python3 failing) will silently exit the hook without a clear message. The existing checks partially mitigate this, but it's fragile.

Hook setup is manual
git config core.hooksPath .githooks is not automated. New contributors will miss it. Consider a Makefile target or a setup-dev.sh that runs this automatically.

---

API Design

configure() + class attributes + env vars = three config sources with unclear precedence
The README shows configure(agent_name=..., policy=...), Node9Agent.agent_name = ... as a class attribute, and NODE9_AGENT_NAME env var. The precedence order is never documented. For a security library, ambiguous policy resolution is a correctness risk.

@internal is under-specified
The README says @internal "never calls evaluate()" and "logs locally only." It's unclear whether @internal methods are discoverable via dispatch(). If they are, that's a bypass vector. If they're not, document it explicitly.

---

Test Coverage Gaps

• No test for configure() precedence (env var vs. kwarg vs. class attribute)
• No test for dispatch() with an unknown tool name
• No test for @internal methods being inaccessible via dispatch()
• No test for async @protect under actual concurrent calls
• No test for dlp_scan with secrets embedded in content (only filename patterns appear covered in manual test)

---

Minor

The manual_test.py diff is truncated — agent.write_file for path traversal is cut off. Ensure that test completes and actually asserts the right exception type.

Overall the security posture improvements (removing shell=True, adding allowlists, DLP, path safety) are directionally correct. The main concerns are the safe_path exception type, the dispatch() error handling gap in the LLM loop example, and the undocumented config precedence.

⚠️ 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

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK

Security Issues (High Priority)

run_shell allowlist bypass in examples
shlex.split("git log --oneline") produces ["git", "log", "--oneline"] which is fine, but shlex.split("git status") produces ["git", "status"]. The allowlist check happens on the raw input string, so this is correct — but it's fragile. If someone adds "git log" to the allowlist, "git log --oneline" would be blocked (correct) but "git log; rm -rf /" would also be blocked (correct by accident). The real risk: these are examples that developers will copy. The inline comment says "never pass LLM-controlled strings to shell=True" but the command string itself still comes from the LLM. The allowlist protects against unknown commands, but an LLM can still control which allowlisted command runs. This is fine for the examples as demonstration, but worth a stronger comment clarifying that allowlist entries must be fully static and not parameterized.

NODE9_SKIP warning at import time
The README says "a warning is emitted at import time" when NODE9_SKIP=1 is set. This must be verified in the actual implementation (not shown in the diff). If the warning goes to stdout instead of stderr, it could be silently swallowed in CI pipelines. Verify it uses warnings.warn(..., stacklevel=2) or writes to stderr.

safe_path error type
The manual test catches ValueError for traversal attempts. A security library should raise a dedicated exception (e.g., PathTraversalException or ActionDeniedException) so callers can distinguish security violations from programmer errors. Using ValueError is too generic and easy to accidentally swallow.

Git Hooks

Sourcing run-tests.sh with set -euo pipefail
The script uses set -euo pipefail and is sourced into the parent shell. This means set -e becomes active in the calling shell for the duration. If the parent shell had set +e, this silently changes its behavior. Consider using a subshell or subprocess instead of sourcing, or explicitly restoring shell options after the script completes.

PYTHON variable construction

PYTHON="${VIRTUAL_ENV:+$VIRTUAL_ENV/bin/python}"
PYTHON="${PYTHON:-$(command -v python3 2>/dev/null)}"

If VIRTUAL_ENV is set but the venv doesn't have python (only python3), the first line sets PYTHON to a nonexistent path and the second line never runs because PYTHON is non-empty. This would produce a confusing "python3 not found" error — actually a misleading "python not found" failure. Consider checking for python3 inside the venv explicitly.

API Design

build_tools_anthropic() / build_tools_openai() naming
Coupling method names to specific providers in a "framework-agnostic" SDK creates maintenance debt. When Anthropic or OpenAI change their schema format, these method names either become misleading or require a breaking change. Consider build_tools(format="anthropic") or a separate node9.adapters module.

dispatch() return type
The README shows result = agent.dispatch(block.name, block.input) but the return type isn't documented. Tool methods return str in examples, but what happens if a tool returns a dict or raises? Callers integrating with LLM frameworks need to know if dispatch always returns str, can return Any, and whether it re-raises or wraps exceptions.

@internal on public-looking methods
The _git_push example uses a leading underscore, which is good Python convention. But nothing in the docs prevents @internal on def git_push(self) (no underscore). Consider whether @internal should enforce or at least warn about the naming convention.

Test Coverage Gaps

• No test shown for NODE9_SKIP=1 warning emission behavior
• No test for dispatch() with an unknown tool name (should this raise KeyError, AttributeError, or a custom exception?)
• No test for safe_path with symlinks pointing outside the workspace — a classic traversal bypass
• No test for dlp_scan on binary content or very large payloads
• No test for the @protect + @tool decorator stacking order (the CrewAI example stacks them — order matters)

Minor Issues

• model="claude-opus-4-6" in the README example looks like a typo (claude-opus-4-5 or claude-opus-4 are real model names)
• The routing table in README says "offline audit log at ~/.node9/audit.log" — this path should be documented as configurable, otherwise it will cause issues in read-only filesystems (containers, CI)
• run-tests.sh runs pytest tests/ hardcoded — if the repo root changes or tests move, the hook silently passes if pytest finds no tests. Add --no-header and check for collected test count, or use --import-mode=importlib

What Looks Good

• Removing shell=True from all subprocess calls is the right call
• The three-tier routing (SaaS → local daemon → offline) is clean and well-documented
• @internal vs @tool separation is a good API distinction
• Pre-commit/pre-push hook structure is solid and the return vs exit comment is exactly right

⚠️ 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

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK

Security

shlex.split() on allowlisted strings is still risky. In examples/basic.py, commands like "git log --oneline" are split by shlex and passed directly to subprocess. While allowlisting helps, splitting a multi-word string means the LLM only needs to get a string into the allowlist to control arguments. Prefer storing commands as pre-split tuples ["git", "log", "--oneline"] and passing them directly — no splitting needed, no ambiguity.

NODE9_SKIP warning timing is unverified. The README says a warning is emitted at import time when NODE9_SKIP=1. This is good, but the diff doesn't show that code. Reviewers can't confirm it actually happens. If that warning is missing or suppressible, a developer could silently disable all governance in production. This needs to be visible in the diff.

safe_path error type inconsistency. The manual test catches ValueError for traversal attempts, but @tool docs say it raises ActionDeniedException. These should be the same type throughout — mixing them forces callers to catch two exception types for the same class of error, and in a security library that's a footgun.

@internal bypass is opt-in with no enforcement. The @internal decorator skips evaluate() entirely. There's nothing stopping a developer from accidentally decorating a genuinely dangerous method with @internal instead of @tool. At minimum, the decorator should log a warning if the method name doesn't start with _, since the docs imply it's for private infrastructure methods.

---

Correctness & Edge Cases

Node9Agent.dispatch() error handling is opaque. If a tool raises ValueError (e.g., the allowlist check), does dispatch() propagate it, wrap it, or swallow it? The diff doesn't show dispatch() implementation. For LLM loops, the distinction matters — a ValueError from an allowlist check should probably surface differently than an ActionDeniedException from policy.

build_tools_anthropic() / build_tools_openai() schema generation from docstrings is fragile. Auto-generating parameter schemas from type annotations works for simple types, but Optional, Union, list[str], and Literal types aren't mentioned. If introspection silently drops unsupported annotations, the tool schema sent to the LLM will be incomplete and hard to debug.

_ALLOWED_COMMANDS as a class variable in RunShellTool (langchain example) is a set on the class, not an instance attribute. LangChain's Pydantic-based BaseTool may not handle arbitrary class attributes cleanly. This could cause a validation error depending on the LangChain version.

---

Test Coverage Gaps

• No test for dlp_scan detecting each individual pattern (AWS keys, PEM headers, etc.). One regex typo silently breaks detection for that entire secret class.
• No test for safe_path with symlinks — a symlink inside the workspace pointing outside it can bypass startswith-style traversal checks.
• No test that NODE9_SKIP=1 actually emits a warning (if it does).
• No test for dispatch() with an unknown tool name — should raise clearly, not KeyError.
• No test for @internal on a non-underscore method name (if the suggested guard is added).
• The manual_test.py smoke test is useful but shouldn't live in examples/ — it's a developer tool and will confuse users treating the examples directory as reference implementations.

---

API Design

configure() vs environment variables create a precedence ambiguity. If NODE9_AGENT_POLICY is set in the environment and configure(policy="audit") is called in code, which wins? The README doesn't say. In a security library this precedence must be explicit and documented.

build_tools_anthropic() naming. Tying method names to specific providers (_anthropic, _openai) means adding a new provider requires a new method. A build_tools(format="anthropic") signature would be more extensible.

---

Minor

• The pre-commit hook uses source to run run-tests.sh, which means set -euo pipefail in the sourced script applies to the parent shell. Any subsequent command failing in the developer's shell session after a test run could behave unexpectedly. This is called out in the comments but is still a real footgun — consider using a subshell instead.
• examples/manual_test.py has trailing whitespace and an incomplete diff (visible truncation). Confirm the file is complete 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