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

.github/workflows/auto-pr.yml

Logic looks correct. The early-exit guard prevents creating redundant PRs when dev is already merged into main. One minor note: exit 0 in the middle of a run: block will silently succeed the step but the subsequent steps in the job will still run. Confirm that's the intended behavior — if you want to skip the entire job, you'd need a separate step that sets an output variable and a if: condition on downstream steps. For this use case (just skipping PR creation), exit 0 is fine.

---

node9/__init__.py

Bug: __all__ comment block is broken.

The "evaluate" string is being added inside what appears to be a docstring/comment block, not an actual __all__ list. Looking at the diff:

    # Low-level governance
    "evaluate",     # manual evaluate() without @protect decorator
    # DLP utilities
    "dlp_scan",

If this is inside a multi-line string used as a comment (common pattern for documenting __all__), adding "evaluate" here does nothing for actual exports — it's just a string literal in a comment block. The real __all__ list (if one exists) needs to be updated separately. Verify that evaluate is actually importable as from node9 import evaluate after this change. The from ._client import evaluate line above is correct, but if __all__ is defined elsewhere and not updated, linters and import * consumers won't see it.

Security concern: evaluate exposure.

Exposing evaluate at the top-level public API means developers can call governance checks manually, bypassing @protect entirely. The docstring comment says "without @Protect decorator" — this needs prominent documentation warning that improper use could create a false sense of security. At minimum, the function should enforce the same daemon communication, auth, and error handling as @protect. If evaluate can be called and its return value ignored by the caller, that's a security gap.

Missing: what does evaluate return on daemon timeout/unreachable?

Without seeing _client.evaluate, it's unclear whether a daemon connectivity failure causes a fail-open or fail-closed result. For a security library, fail-open is unacceptable. This should be explicitly tested and documented.

---

node9/_client.py (not shown but implicitly reviewed)

The evaluate function is imported but its implementation isn't in the diff. Key questions that should be answered before merging:

1. Does it raise DaemonNotFoundError on connection failure, or return a default? Fail-open on timeout is a critical security bug.
2. Is there request authentication/signing? If the daemon HTTP endpoint accepts unauthenticated calls, a local process could approve its own actions.
3. Is the run_id/session context passed correctly? Without it, the daemon can't correlate approvals to the right agent session.
4. Is there a timeout configured on the HTTP call? An unbounded requests.get/post with no timeout can hang agent workflows indefinitely.

---

Test Coverage Gaps

The diff adds a new public API surface (evaluate) with no visible test additions. At minimum, tests should cover:

• evaluate returns denial when daemon is unreachable (fail-closed behavior)
• evaluate correctly propagates ActionDeniedException
• evaluate with missing/invalid run_id doesn't silently approve
• The CI workflow change: no test for the "0 commits" branch (acceptable for CI YAML, but worth a manual smoke test)

---

API Design

Exposing evaluate as a first-class public function is reasonable for advanced integrations (e.g., custom LangChain middleware that can't use decorators), but the naming is generic. Consider whether request_approval or check_approval better communicates the security intent to developers who are new to the SDK — evaluate could be confused with LLM evaluation tooling.

---

Summary

The CI fix is clean. The evaluate export has a potential __all__ bug that could silently fail to export, and the security properties of the underlying function (fail-closed behavior, auth, timeouts) need verification before this lands. No blocking issues in what's shown, but the missing _client.py diff is the real risk surface here.

---

Automated review by Claude Sonnet

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK PR

node9/__init__.py — Bug: String literal in wrong context

The __all__ comment block has a critical issue:

# .new_session()           — fresh run_id for server/multi-session deployments
# Low-level governance
"evaluate",     # manual evaluate() without @protect decorator
# DLP utilities

A bare string literal ("evaluate") is sitting inside what appears to be a comment block used as documentation, but if this is actually inside __all__ or a list structure, the indentation and surrounding context matter enormously. From the diff, it's unclear whether "evaluate" is properly inserted into __all__ or is orphaned as a no-op string expression between comments. Verify the final file renders "evaluate" as a list element in __all__, not a floating string literal. This is the most significant correctness issue in the diff.

---

node9/_client.py — Docstring Updates

The updated docstring is a clear improvement. A few concerns:

Fail-open behavior is dangerous for a security library. The docstring now explicitly documents that an unreachable daemon causes auto-approval. This design decision should be reconsidered or at minimum heavily scrutinized:

• An attacker who can disrupt the daemon (process kill, port block) causes all tool calls to auto-approve silently.
• The RuntimeWarning mitigation for policy == "require_approval" is insufficient — warnings can be suppressed, swallowed by logging configs, or missed entirely in production.
• Consider whether fail-open should instead be fail-closed when a policy is explicitly set, regardless of warning emission.

_CHECK_TIMEOUT vs _WAIT_TIMEOUT asymmetry: The docstring documents "daemon unreachable at call time → offline mode" vs "daemon timeout/connection closed → ActionDeniedException." The distinction between "unreachable during initial check" and "connection closed mid-wait" needs to be verified as actually implemented — this is an easy place for a race condition or inconsistent state to exist silently.

---

node9/_agent.py — tool and internal exports

Not changed in this diff, but evaluate is now public API alongside @protect. Ensure the public evaluate raises ActionDeniedException consistently with the decorator path. Any divergence here is a correctness and security issue.

---

.github/workflows/auto-pr.yml

The no-commit guard is correct and the logic is sound. One minor note: exit 0 in a run: step won't stop subsequent steps in the job — it only exits the current shell. If there are downstream steps that should also be skipped, consider using $GITHUB_OUTPUT with a condition flag or actions/github-script to set job-level outputs. For this specific case (single-step job body), it's likely fine.

---

tests/test_client.py — Test Coverage

The new test is good but insufficient for a security library release.

What's tested: evaluate is importable and callable. ✓

Missing coverage that should accompany this PR:

1. evaluate is in __all__ — the import test passes even if __all__ is broken, because from node9 import evaluate resolves via __init__.py regardless of __all__ contents. Add assert "evaluate" in node9.__all__.

2. Fail-open path — no test that when the daemon is unreachable, evaluate auto-approves (no exception raised). This is security-critical behavior.

3. NODE9_SKIP=1 warning — the updated docstring says a warning is emitted; no test verifies it.

4. policy == "require_approval" + offline mode — the RuntimeWarning path has zero test coverage.

5. The double-blank line before _make_response is a trivial style issue (PEP 8: two blank lines between top-level definitions, not three).

---

Summary

Area	Severity
"evaluate" placement in __all__	🔴 High — verify correctness
Fail-open daemon behavior	🟠 Medium — design risk for a security library
Missing __all__ assertion in test	🟡 Low
Missing coverage for fail-open/warning paths	🟡 Low
CI exit 0 scope	⚪ Negligible

The docstring improvements are good. The main blocker is confirming "evaluate" is actually in __all__, and the fail-open design deserves explicit team sign-off given the security context.

---

Automated review by Claude Sonnet

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK PR

CI / Workflow (auto-pr.yml)

Early exit bug: Using exit 0 inside a run: step only exits the shell subprocess — it does not stop the job. Subsequent steps will still execute, meaning the PR creation/update logic runs anyway. Use a job-level output variable and a if: condition on downstream steps, or use the $GITHUB_OUTPUT / step conclusion pattern.

# The exit 0 here does NOT prevent the next step from running

This is a correctness failure, not a cosmetic issue.

---

node9/__init__.py

Malformed __all__ comment block: The string "evaluate" appears to be placed inside a comment block as if it's part of __all__, but based on context it looks like it's injected mid-comment rather than into the actual list. Verify that evaluate is properly appended to __all__ = [...] — a string literal floating in module scope does nothing and won't make it into __all__. If __all__ is defined elsewhere and this is a doc-comment convention, that's non-obvious and fragile.

---

node9/_client.py (docstring only)

The docstring update is accurate and well-structured. A few notes:

Fail-open concern (security-critical): The docstring now clearly documents that an unreachable daemon falls back to offline/auto-approve mode. For a security library, this is the most dangerous path. The docstring mentions a RuntimeWarning for policy == "require_approval", but:

• Is this warning guaranteed to surface in all logging configurations? warnings.warn is suppressible.
• Consider whether this path should raise rather than warn, or at minimum be documented as a known security trade-off requiring explicit opt-in.

Timeout gap: The 10-second gap between daemon auto-deny (~55 s) and _WAIT_TIMEOUT (65 s) is documented, but it's not clear what happens if the daemon auto-denies silently and the client times out first — does the client get an ActionDeniedException from the timeout, or does it fall into offline mode? The docstring implies timeout → denied, but the behavior should be verified in tests.

NODE9_SKIP warning: Docstring says it "emits a warning" — confirm this is implemented; it's not visible in this diff.

---

tests/test_client.py

Test is mostly a smoke test: test_evaluate_importable_from_public_api validates import machinery and __all__ membership — useful for regression, but the docstring oversells it. The comment "Fail-open / offline-mode coverage lives in TestOfflineMode (4 tests)" is doing real work here: none of those tests are visible in this diff, so their existence can't be verified in this review. If TestOfflineMode was not modified, that's fine, but the newly documented behavior (the require_approval warning path, the fail-open degradation) should have test coverage that's either shown or explicitly called out as pre-existing.

Missing test cases for new public API surface:

• evaluate() called directly (not via @protect) with daemon unreachable — does it actually warn for require_approval?
• evaluate() with NODE9_SKIP=1 — does it warn as documented?
• evaluate() with a SaaS HTTP error — does it propagate RuntimeError?

These are the behaviors the updated docstring promises. They should be tested.

---

Security Summary

Risk	Severity
Daemon unreachable → silent auto-approve (fail-open)	High — acceptable only with explicit operator awareness
RuntimeWarning suppressible by caller	Medium
NODE9_SKIP bypass emitting only a warning	Medium — should be a hard error in non-test environments
exit 0 in CI not actually skipping downstream steps	Low (CI correctness, not runtime security)

---

Summary

The docstring improvements are genuinely useful. The __all__ export and the smoke test are reasonable additions. However: the CI early-exit bug is a real defect, the __all__ mutation needs to be verified as syntactically correct, and the fail-open degradation path is the highest-risk behavior in this diff — it needs explicit test coverage and consideration of whether a warning is a strong enough signal for a security library. Do not merge until the CI step exit behavior is fixed.

---

Automated review by Claude Sonnet

[node9ai]

🤖 Claude Code Review

Code Review

node9/__init__.py — __all__ Mutation Bug (High Severity)

The "evaluate" string and its comment are inserted inside what appears to be a docstring or comment block, but looking at the diff closely, it's being added as a bare string literal inside __all__ in an unusual position. More critically:

# .new_session()           — fresh run_id for server/multi-session deployments
# Low-level governance
"evaluate",     # manual evaluate() without @protect decorator
# DLP utilities

If this is inside a list literal for __all__, this is fine syntactically. But mixing comment lines with list entries in the middle of __all__ is fragile and visually misleading — it looks like the comments are part of the list. Confirm the full __all__ definition renders correctly as a valid Python list; a future maintainer could accidentally delete the closing bracket thinking the comments are the end of the block.

The from ._client import evaluate import is added but evaluate was already callable in internal code — verify this doesn't shadow anything in _agent.py or elsewhere that might also export an evaluate symbol.

---

node9/_client.py — Docstring / Behavior Inconsistency (Medium Severity)

The updated docstring states:

Daemon unreachable at call time → offline mode (fail-open, auto-approve)

But also:

Daemon timeout / connection closed → ActionDeniedException (fail-closed)

These two cases need to be clearly distinguished in the docstring and — more importantly — in the actual code. "Unreachable at call time" vs "timeout during approval wait" is a meaningful security boundary. Reviewers cannot verify correctness from this diff alone since the routing logic itself isn't shown. This is a security library — the fail-open path should be audited carefully. Does the daemon-reachability check happen before or after acquiring a run_id? If after, a TOCTOU window exists where the daemon disappears mid-request and falls through to auto-approve.

The 5s / 65s timeout values are documented here but defined as constants elsewhere — ensure they're kept in sync. Magic numbers in docstrings are a maintenance liability.

---

tests/test_client.py — Test Coverage (Medium Severity)

The new test only validates importability and __all__ membership. The docstring comment says "Fail-open / offline-mode coverage lives in TestOfflineMode (4 tests)" — but this diff doesn't show those tests. If they're pre-existing, fine, but:

• There's no test for the require_approval + daemon-unreachable → RuntimeWarning path introduced in this PR.
• There's no test that NODE9_SKIP=1 emits a warning (the docstring says it does, but nothing enforces it).
• There's no test that SaaS HTTP errors propagate as RuntimeError rather than silently degrading to offline mode.

These are all new behaviors documented in this PR and should have corresponding tests.

---

.github/workflows/auto-pr.yml — Acceptable

The explanatory comment is accurate and the exit 0 behavior is correct. No issues.

---

API Design Concern

Exposing evaluate as a public top-level API is reasonable for power users, but the docstring should explicitly warn that calling evaluate directly bypasses any @protect decorator composition guarantees (e.g., DLP scanning, agent context injection). Without that callout, an integrator could use evaluate thinking they're getting full protection when they're only getting approval routing.

---

Summary

Issue	Severity
__all__ structural ambiguity	Low
Fail-open TOCTOU risk (not verifiable from diff)	Needs audit
Missing tests for new warning/error paths	Medium
Docstring doesn't warn about bypassing @protect guarantees	Low

The importability test is well-commented and honest about its limitations, which is good. The main gap is test coverage for the new failure modes documented in the evaluate docstring.

---

Automated review by Claude Sonnet

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK PR

Overall Assessment

This is a documentation/export PR — adding evaluate() to the public API, improving docstrings, adding tests, and clarifying CI logic. The changes are mostly solid but have a few issues worth addressing.

---

node9/__init__.py — Malformed __all__ Entry

Bug: The string "evaluate" is placed inside the docstring comment block, not inside the __all__ list literal. It appears to be a misplaced list element formatted as a comment string:

# Low-level governance
"evaluate",     # manual evaluate() without @protect decorator
# DLP utilities

If this is inside __all__, the surrounding context needs verification. If __all__ is a list, embedding it between comment lines within the list body is valid Python — but the diff doesn't show enough context to confirm the list brackets surround this entry. If this is in a comment/docstring block (as the indentation and style suggest), "evaluate" is a dangling string expression, not an __all__ entry. This needs confirmation — the test test_evaluate_importable_from_public_api could pass even if __all__ is broken (as the test itself notes), making the bug invisible.

---

node9/_client.py — Docstring Accuracy Concerns

1. Fail-open security risk not prominently warned: The docstring describes daemon-unreachable → offline → auto-approve as normal degradation. For a security library, this deserves a ⚠️ warning equivalent to the NODE9_SKIP bypass. Fail-open is arguably the more dangerous path because it's silent in production unless require_approval is set.

2. "Daemon dies mid-request → fail-closed" claim needs verification: The diff doesn't show the underlying implementation, so this claim can't be validated here. If the polling loop has any exception handler that falls through to auto-approve, this claim is incorrect and misleading.

3. Timeout values (5s, 65s) hardcoded in docstring: If _CHECK_TIMEOUT / _WAIT_TIMEOUT constants change, the docstring will silently go stale. Consider referencing the constants by name only, without repeating the numeric values.

---

tests/test_client.py — Test Coverage Gaps

1. test_saas_route_taken_when_api_key_set doesn't assert the request content: The test confirms routing but doesn't verify the Bearer token is actually sent in the Authorization header. For a security library, this is a meaningful gap — the API key could be silently dropped.

2. test_saas_http_error_raises_runtime_error uses hdrs=None: Passing None for headers to urllib.error.HTTPError can cause AttributeError in some Python versions when the error is processed. Use http.client.HTTPMessage() or email.message.Message() instead.

3. No test for evaluate() called with run_id explicitly set: The run_id parameter exists on the public signature. No test verifies it's forwarded correctly to either the daemon or SaaS path.

4. No test for args containing non-serializable types: Since args: dict[str, Any] is the public contract, passing something like {"fn": lambda x: x} should fail fast with a clear error, not silently corrupt the audit log or crash inside json.dumps deep in the stack.

5. Missing test: NODE9_SKIP=1 emits a warning: The docstring now explicitly documents that NODE9_SKIP emits a warning. There's no test verifying this.

---

.github/workflows/auto-pr.yml

The comment added is accurate and helpful. No issues.

---

API Design Note

Exposing evaluate() as a public primitive is reasonable, but the docstring warning about missing DLP and agent context should probably also appear in the node9 module-level docstring or README. Developers skimming help(node9) won't see the _client.py docstring. A short # noqa-style note in __init__.py near the export would help.

---

Summary of Required Actions

Priority	Issue
🔴 High	Verify "evaluate" is actually inside __all__ list, not a floating string
🟡 Medium	Add test asserting Bearer token is present in SaaS requests
🟡 Medium	Fix hdrs=None in HTTPError mock
🟡 Medium	Add test for NODE9_SKIP warning
🟢 Low	Remove hardcoded timeout values from docstring
🟢 Low	Add run_id forwarding test

---

Automated review by Claude Sonnet

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK PR

Overall Assessment

This is a documentation/export PR — adding evaluate() to the public API, improving docstrings, and adding test coverage for the SaaS routing path. The changes are generally solid, but there are several issues worth addressing.

---

node9/__init__.py — Malformed __all__ Entry

Bug (medium severity): The string "evaluate" is inserted as a comment-style annotation inside what appears to be a docstring or comment block, not as an actual list element in __all__. Based on the diff context, the __all__ list likely looks like:

# Low-level governance
"evaluate",     # manual evaluate() without @protect decorator
# DLP utilities

If __all__ is defined as a tuple/list elsewhere and this is injected into a comment block, the export won't work correctly. The test test_evaluate_importable_from_public_api would still pass because evaluate is directly imported in __init__.py — the test explicitly calls this out but then proceeds to assert __all__ membership. Verify that "evaluate" is actually appended to the __all__ list, not just placed in surrounding comments.

---

node9/_client.py — Docstring Issues

Minor — misleading fail-open documentation: The docstring states:

"Daemon unreachable at call time → offline mode (fail-open, auto-approve)"

This is a significant security behavior that deserves more than a bullet point. For a security library, fail-open should be conspicuously warned in the docstring with a ⚠️ marker similar to the one used for the low-level primitive warning. Developers skimming docs may miss this.

Minor — timeout constant references: Referencing _CHECK_TIMEOUT and _WAIT_TIMEOUT by name is helpful, but if these are renamed in future refactors, the docstring silently becomes wrong. Consider including the actual default values inline (e.g., (default: 2s / 30s)).

---

tests/test_client.py — Test Coverage Gaps

Missing: NODE9_SKIP=1 warning test. The updated docstring explicitly states NODE9_SKIP=1 "emits a warning." This behavior should be tested. If the warning was added as part of this PR's intent (the docstring implies it), there's no test confirming it fires.

Missing: Denial path for SaaS route. TestSaaSRoute only tests approval and error cases. There's no test for {"approved": False} from the SaaS endpoint — confirming ActionDeniedException is raised. This is the core security assertion for the SaaS path.

test_saas_route_taken_and_bearer_token_sent — patching concern:

with patch("node9._client._daemon_reachable") as mock_check:
    evaluate("bash", {"command": "ls"})
mock_check.assert_not_called()

The _daemon_reachable patch is correct in intent (proving SaaS route skips daemon check), but the assert_not_called() happens outside the patch context manager. By the time the assertion runs, the mock is already uninstalled. Move the assertion inside the with block, or restructure. This is a test correctness bug.

test_saas_run_id_forwarded — fragile key assumption: The test asserts runId (camelCase). If the serialization format changes to run_id, the test fails without a clear error message. Consider asserting both that the key exists and logging the full payload on failure.

test_evaluate_importable_from_public_api — redundant assertion noted but kept: The comment correctly explains why the import test is insufficient alone. Good discipline. No issue here.

---

.github/workflows/auto-pr.yml

The comment added is accurate and useful. No issues.

---

Security Concerns

No new security regressions introduced. The evaluate() exposure is explicitly documented as a low-level primitive, with a clear warning about missing DLP and context injection. This is acceptable, though consider whether exposing it without a mandatory agent_name parameter creates an audit log gap — unauthenticated calls with no agent context are harder to investigate post-incident.

---

Summary of Required Fixes

Priority	Issue
High	Verify "evaluate" is actually in the __all__ list, not a comment
High	Add SaaS denial path test (approved: False → ActionDeniedException)
Medium	Fix mock_check.assert_not_called() placement outside with block
Low	Add NODE9_SKIP warning emission test
Low	Emphasize fail-open behavior more prominently in docstring

---

Automated review by Claude Sonnet

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK PR

Documentation (_client.py)

The docstring additions are genuinely useful and accurate. One concern: the fail-open behaviour description needs scrutiny from a security standpoint.

Security: Fail-Open is Documented But Not Hardened

The docstring accurately describes that daemon-unreachable → offline mode → auto-approve. For a security library, this is the most dangerous path. The docstring now makes it visible, but the require_approval policy only warns rather than blocks. There's a gap between what users will expect ("require approval" sounds like it should fail-closed) and what actually happens. This should be either fixed or prominently flagged as a known limitation with a rationale, not just a RuntimeWarning. At minimum, the docstring should explicitly state this is a design trade-off, not just describe the behaviour.

Docstring Accuracy

"Daemon dies mid-request → DaemonNotFoundError or ActionDeniedException (fail-closed: no auto-approve path exists once a request_id is acquired)"

This is good security documentation, but it's worth confirming the implementation actually guarantees this. If there's any exception-swallowing in the polling loop that could fall through to offline mode, this claim would be incorrect and dangerous. The review can't verify this without seeing the polling code, but it should be validated against the actual implementation.

Public API Export (__init__.py)

The comment inside __all__ is structurally broken:

# Low-level governance
"evaluate",     # manual evaluate() without @protect decorator
# DLP utilities

The comment lines inside __all__ are fine syntactically (they're not strings), but mixing inline comments with list entries is fragile and inconsistent with how the rest of __all__ appears to be structured. More importantly, evaluate appearing in __all__ without also having a corresponding entry in the module-level docstring or README is a discoverability gap for integrators.

Tests (test_client.py)

test_evaluate_importable_from_public_api: The docstring explains the subtlety well. The test is correct and the separate __all__ assertion is the right approach.

test_saas_route_taken_and_bearer_token_sent: The assertion mock_check.assert_not_called() is inside the with patch(...) block but after evaluate() returns — this is fine functionally, but the comment "Assert inside the with block so mock is still active" is misleading. The mock is active throughout the with block; the call ordering doesn't matter here. Remove or correct the comment to avoid confusing future maintainers.

test_saas_run_id_forwarded: Uses hasattr(req, "data") and req.data to capture the payload. This silently passes if urlopen is called without a body (e.g., a GET). If the implementation changes to a two-step request (submit + poll), only the first call's payload is captured. The test should assert len(sent) >= 1 and explicitly validate it's the submit call, or assert len(sent) == 1 if exactly one request is expected.

test_saas_denial_raises_action_denied_exception: Good test. However, assert "Blocked by policy" in exc.value.reason assumes ActionDeniedException has a .reason attribute. If the attribute name ever changes, this fails with AttributeError rather than a clear assertion error. Consider str(exc.value) as a fallback check, or at least confirm .reason is part of the public exception contract.

Missing Test Cases:

• No test for NODE9_SKIP=1 emitting a warning (the docstring says it does; this should be verified).
• No test for the SaaS path when NODE9_API_URL is not set — what's the default URL? Does it fall back correctly?
• No test for run_id defaulting to empty string in the SaaS payload when not provided.
• No test for malformed SaaS response (e.g., missing approved key) — this is a security-relevant edge case.

CI Workflow (.github/workflows/auto-pr.yml)

The comment is accurate and the exit 0 behaviour is correct. No issues here.

Summary

• Block: require_approval policy silently degrades to auto-approve — either fix it or document the deliberate trade-off more explicitly than a warning.
• Minor: Misleading comment in bearer token test; __all__ comment formatting; missing run_id-absent and malformed-response test cases.
• Verify: Confirm the polling loop cannot fall through to offline mode after acquiring a request_id, given the docstring's fail-closed claim.

---

Automated review by Claude Sonnet

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK PR

Summary

This PR makes three changes: adds evaluate to the public API, hardens require_approval to fail-closed (raise instead of warn), and adds a TestSaaSRoute test class. The intent and direction are correct, but there are issues worth addressing.

---

Security Issues

1. Fail-closed change is correct but incomplete

The _offline_audit change (warn → raise) is the right call. However, _offline_audit is only reached when the daemon is unreachable at check time. The comment in evaluate's docstring correctly notes the daemon-dies-mid-request case is already fail-closed — but it's worth confirming _offline_audit is the only auto-approve path. If there's any exception handler upstream that catches DaemonNotFoundError and falls through to audit, that's a hidden bypass. The review can't confirm this without seeing the full evaluate call graph, and this warrants a targeted audit.

2. NODE9_SKIP=1 emits a warning — verify it does

The docstring says NODE9_SKIP=1 "emits a warning." There's no test confirming this, and the previous code may not have added one. For a security library, silent skip-bypass is dangerous. A test asserting the warning is emitted (not just that the call succeeds) should exist.

3. test_saas_malformed_response_raises is too weak

with pytest.raises((ActionDeniedException, RuntimeError)):

Accepting either exception obscures whether the implementation is actually fail-closed or just coincidentally raises. If approved key is missing and the code does response.get("approved", True) (auto-approve), this test would pass if a downstream check raises for unrelated reasons. Pin the expected exception type.

---

Test Coverage Gaps

4. No test for require_approval + API key set but SaaS fails

The fail-closed docstring says SaaS errors propagate as RuntimeError. But what if NODE9_API_KEY is set, SaaS returns 500, and the code falls back to offline audit? This combination isn't tested. Given the security implications, it should be explicit.

5. test_saas_route_taken_and_bearer_token_sent has a structural issue

def capturing_urlopen(req, timeout):
    captured_headers.append(dict(req.headers))
    return approve_resp

urllib.request.Request headers are stored case-normalized (capitalized first letter). The assertion captured_headers[0].get("Authorization", "") may return "" even when the header is present as "Authorization" vs "authorization" depending on Python version. Use a case-insensitive lookup or assert on the raw request object.

6. No test for _offline_audit still writing the audit log when policy is NOT require_approval

The audit log path after the raise was preserved, but there's no regression test confirming the log write still happens in the non-require_approval offline case after this refactor.

---

API Design

7. evaluate docstring warning placement

The ⚠️ evaluate() is a low-level primitive warning is at the bottom of the docstring. Developers scanning quickly will miss it. Move it to the top, before the routing table. This is user-facing and the library's main safety concern.

8. __all__ comment block is misformatted

    # Low-level governance
    "evaluate",     # manual evaluate() without @protect decorator
    # DLP utilities

This string literal sits inside a comment block, which is confusing. The structure of __all__ in __init__.py should be checked — if this is inside a docstring or comment rather than the actual __all__ list, evaluate isn't exported correctly. The test test_evaluate_importable_from_public_api would catch a broken import but the __all__ assertion depends on the list being correctly structured.

---

Minor Issues

9. test_evaluate_importable_from_public_api comment is stale

"Fail-open / offline-mode coverage lives in TestOfflineMode (4 tests)"

After this PR renames test_offline_with_require_approval_policy_warns → test_offline_with_require_approval_policy_raises, the count and description may be inaccurate. Minor, but misleading in a test file.

---

What's Good

• The fail-closed direction is correct and well-justified.
• The expanded docstring on evaluate is genuinely useful for integrators.
• SaaS route tests cover the happy path, denial, HTTP errors, and run_id forwarding — good baseline.
• The CI workflow comment is accurate and harmless.

---

Bottom line: The security direction is right, but the malformed-response test is too permissive, the Bearer header assertion has a reliability issue, and the NODE9_SKIP warning path is untested. For a security library, these need to be tight before merging.

---

Automated review by Claude Sonnet

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK PR

Overall Assessment

This is a solid, security-focused PR. The fail-closed change for require_approval is the right call, and the new tests add meaningful coverage. A few issues worth addressing.

---

Security

✅ Fail-closed change is correct. Replacing a RuntimeWarning with DaemonNotFoundError when policy == "require_approval" but no daemon/API key is available is the right security decision. Silent auto-approval under an explicit approval policy is a false sense of security.

⚠️ evaluate() is now public but strips DLP and agent context — docstring says so, but there's no runtime guard. An integrator who calls evaluate() thinking it's equivalent to @protect will silently skip DLP scanning. Consider at minimum emitting a warnings.warn if dlp_scan would have caught something, or adding a _warn_no_dlp flag. At least the docstring is explicit — but this is a security library and discoverability of that footgun matters.

⚠️ NODE9_SKIP=1 path in the routing docstring says it "emits a warning" — verify that's actually implemented. If _SKIP is truthy and the code silently no-ops without any warning, the docstring is misleading and operators won't notice the bypass is active in production.

---

Correctness & Edge Cases

🐛 _offline_audit raises before writing the audit log. After the raise DaemonNotFoundError(DAEMON_PORT) for require_approval, the audit log write below it is unreachable. This is likely intentional (you don't want to log an auto-approved action that was actually blocked), but it's worth a comment clarifying that the raise is the intended terminal path and the audit log is only for the auto-approve branch.

⚠️ test_saas_route_taken_and_bearer_token_sent uses a shared mutable list captured_headers: list[dict] = [] that's never reset between potential parametrize runs. Not a bug here since there's no parametrize, but fragile if the test is ever extended.

⚠️ capturing_urlopen in the SaaS tests doesn't consume a timeout argument consistently. Some stubs accept (req, timeout), others may not match the actual call signature if urlopen is called with keyword args. Double-check the actual call site in _evaluate_cloud — if it uses urlopen(req, timeout=...), keyword-only stubs will fail.

⚠️ test_saas_malformed_response_raises — the comment says "approved missing → falsy; pending missing → falsy → raises ActionDeniedException." This is testing an implicit behavior that depends on falsy dict-key lookup. This is fragile if _evaluate_cloud ever adds explicit key validation with a different exception type. The test is valuable but the behavior being tested should arguably raise a more specific error (e.g., ValueError: malformed response) rather than silently treating missing keys as denial.

---

API Design

⚠️ evaluate appearing in __all__ via a comment block that's not actually a list literal. Looking at the diff, the "evaluate" string is added inside a docstring/comment block in __init__.py, not inside the actual __all__ list. If __all__ is defined elsewhere and this comment block is the canonical list, that's fine — but it's worth confirming the test_evaluate_importable_from_public_api test actually catches this. The test does check "evaluate" in node9.__all__ separately, so it would catch a broken __all__ — good.

Minor: The evaluate() docstring is now quite long. Consider splitting the "Fail behaviour" table into a separate internal _ROUTING_NOTES constant or trimming it. For a public-facing SDK, shorter docstrings tend to get read; longer ones get skipped.

---

Test Coverage Gaps

• No test for NODE9_SKIP=1 behavior — does it no-op, does it warn, does it log? This bypass path has zero test coverage in this diff.
• No test for evaluate() called with args containing sensitive data — verifying DLP is not run (since that's the documented behavior) would make the footgun explicit in tests.
• No test for the require_approval raise path leaving the audit log unwritten — if operators rely on the audit log for forensics, confirming it's empty on a blocked call is worth asserting.

---

CI/Workflow

The exit 0 comment in auto-pr.yml is accurate and useful for future maintainers. No issues.

---

Summary

One real bug to confirm (audit log unreachability intentional vs. accidental), one potentially misleading docstring (NODE9_SKIP warning), and a few missing test cases for the bypass path and DLP omission. Nothing blocking, but the NODE9_SKIP warning claim should be verified before merge.

---

Automated review by Claude Sonnet

[node9ai]

🤖 Claude Code Review

Code Review: Node9 Python SDK PR

Overall Assessment

This is a solid, well-reasoned PR. The fail-closed change for require_approval is the right security call, and the new tests are thorough. A few issues worth addressing:

---

Security Issues

1. NODE9_SKIP bypass with only a warning — not audited
The docstring now explicitly labels NODE9_SKIP=1 as "unsafe bypass — testing only, emits a warning," but the skip path doesn't appear to write to the audit log either. An operator enabling NODE9_SKIP in a misconfigured prod environment gets no forensic trail. At minimum, the warning should be loud (consider warnings.warn(..., stacklevel=2) with a distinct category), and ideally a skip event should be logged to the audit file.

2. Offline audit log race condition (pre-existing, but now more visible)
os.makedirs + open/append has a TOCTOU window. Low risk in practice but worth noting given this is a security library. File locking or atomic writes would be more appropriate.

3. test_saas_route_taken_and_bearer_token_sent — approve_resp is a single mock object
_make_response likely returns a context manager or file-like object that can only be read once. If _evaluate_cloud reads the response body twice (or if urlopen is called more than once), the second read returns empty bytes, potentially masking a bug where a second call silently auto-approves. Use side_effect with a factory, not a single reused object.

---

Correctness & Edge Cases

4. _offline_audit raises DaemonNotFoundError before checking NODE9_API_KEY
The routing in evaluate() should mean _offline_audit is only reached when neither SaaS nor daemon is available — but this assumption should be validated. If the routing logic ever changes and _offline_audit is called while an API key is set (e.g., after a SaaS timeout fallback), the error message citing DAEMON_PORT will be misleading and confusing to operators. The error message should reflect the actual missing resource.

5. DaemonNotFoundError(DAEMON_PORT) — message clarity
When require_approval raises, the error message will say something like "daemon not found on port 7473." But the user may have NODE9_API_KEY unset too — the error should mention both remediation paths (start daemon or set NODE9_API_KEY), as the docstring correctly documents but the exception message may not.

6. test_saas_malformed_response_raises — comment is implementation-coupling
The test comment explains internal _evaluate_cloud logic (approved missing → falsy). If the implementation changes to raise KeyError or RuntimeError instead, the test still passes but the comment is wrong. Assert the specific exception type/message you actually want as the public contract, and drop the internal logic explanation.

---

API Design

7. evaluate() docstring — "first match wins" order doesn't match actual routing
The docstring lists NODE9_SKIP → NODE9_API_KEY → daemon → offline. Confirm this exactly matches the if/elif chain in evaluate(). If daemon-reachability is checked before the API key (or vice versa), the docstring is a misleading security contract.

8. __init__.py — comment placement in __all__
The "evaluate" entry is added with a comment inside what appears to be a docstring-style comment block, not inside the actual __all__ list. Verify this is syntactically inside __all__ = [...] and not just a comment in the module body — the test assert "evaluate" in node9.__all__ would catch this at runtime, but it's worth a visual check.

---

Test Coverage Gaps

9. No test for NODE9_SKIP=1 behavior
Given the explicit "unsafe bypass" warning in the docstring, there should be a test confirming: (a) it returns None without calling urlopen or daemon, and (b) it emits the expected warning. Without this, the bypass path is untested.

10. No test for evaluate() called with args={} (empty dict)
Edge case: some frameworks pass empty args for zero-argument tools. Ensure the payload serialization handles this without raising.

11. Missing test: daemon dies mid-request (the "fail-closed once request_id acquired" claim)
The docstring documents this important behavior but there's no test for it. This is the most subtle failure mode and deserves at least one test with a mock that raises mid-poll.

---

Minor

• The auto-pr.yml comment is accurate and helpful — no issues there.
• Renaming test_offline_with_require_approval_policy_warns → test_offline_with_require_approval_policy_raises is correct and improves clarity.

---

Automated review by Claude Sonnet