Add SDK guardrail smokes - #9
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThree new standalone smoke test scripts are added under ChangesSDK Smoke Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/tests/smoke_consumer_neutral.py`:
- Around line 15-17: The current test in smoke_consumer_neutral.py performs an
AST check on imports but does not directly verify the zero-dependency contract
declared in python/pyproject.toml. Add an assertion that reads the
pyproject.toml file, parses the dependencies field, and explicitly verifies that
dependencies is an empty list. This ensures that the declared packaging contract
matches the actual implementation and prevents accidental addition of
dependencies that could circumvent the static import check.
- Around line 91-93: The subprocess.run call that executes the probe without a
timeout parameter can hang indefinitely if the import blocks. Add a timeout
parameter to the subprocess.run call to enforce a maximum execution time, then
wrap it in a try-except block to catch subprocess.TimeoutExpired exceptions.
When handling both the TimeoutExpired exception and any other failures, include
the captured stderr in the error message to provide debugging context about what
went wrong during the probe execution.
In `@python/tests/smoke_readme_examples.py`:
- Line 62: The README.read_text() call uses the process default encoding instead
of explicitly specifying UTF-8, which can cause the test to fail on systems with
non-UTF-8 locale settings. Modify the read_text() method call to explicitly pass
the encoding parameter set to UTF-8 to ensure deterministic behavior across
different system locales and make the smoke test more reliable.
In `@python/tests/smoke_typed_config.py`:
- Around line 123-131: The test function typed_config_requires_native() only
validates that plan_config() raises FabricNativeUnavailableError when using CLI
fallback, but the WS4 contract requires this behavior for all typed-config
methods including doctor_config() and run_config(). Expand the test to also
invoke doctor_config() and run_config() on the cli_client instance to ensure all
three typed-config entrypoints properly raise FabricNativeUnavailableError and
prevent regressions in the other methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a98be8b7-7457-465a-8540-bd498b051e15
📒 Files selected for processing (3)
python/tests/smoke_consumer_neutral.pypython/tests/smoke_readme_examples.pypython/tests/smoke_typed_config.py
Three dependency-light smokes that pin WS4 contract guarantees: - smoke_typed_config: typed (in-memory) config is first-class. plan_config and doctor_config resolve a maintained adapter with no agent directory; run_config drives an inline run from an adapter-only dir (no agent package); and the *_config methods raise a clear error on the CLI fallback (native-only). - smoke_consumer_neutral: the SDK core imports only the standard library and nemo_fabric (consumer glue stays under nemo_fabric.integrations), and a plain `import nemo_fabric` pulls in no consumer/harness package. Also pins the zero-dependency contract. - smoke_readme_examples: the README "Use Fabric" Python examples execute against the example agent, with a drift guard that the README documents each verbatim. Signed-off-by: Ajay Thorve <athorve@nvidia.com>
The Python CI runs an explicit smoke list; add the new dependency-free guardrail smokes so they are enforced on every PR, not just present in the repo: smoke_typed_config, smoke_consumer_neutral, smoke_readme_examples. Signed-off-by: Ajay Thorve <athorve@nvidia.com>
0604444 to
b4ac6aa
Compare
- consumer_neutral: assert the declared zero-dependency contract directly (pyproject dependencies == []), which the docstring already claimed but the import scan alone could miss; bound the import-probe subprocess with a timeout. - readme_examples: read the README as UTF-8 for locale-independent behavior. - typed_config: extend the native-only check to doctor_config and run_config, not just plan_config (all three *_config methods are native-only). Signed-off-by: Ajay Thorve <athorve@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/tests/smoke_consumer_neutral.py`:
- Around line 84-86: The current code uses chained .get() calls with default
values that silently default to an empty dependency list if either the [project]
section or dependencies key is missing from the pyproject.toml metadata. This
allows metadata regressions to go undetected. Replace the current approach of
using .get("project", {}).get("dependencies", []) with explicit assertions that
verify both the "project" key and the nested "dependencies" key exist in the
pyproject dictionary before accessing them, then extract the dependencies list.
This ensures the assertion properly validates the metadata structure while still
checking that dependencies is empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9944c3ea-40f4-4f28-8046-847f7064acd8
📒 Files selected for processing (3)
python/tests/smoke_consumer_neutral.pypython/tests/smoke_readme_examples.pypython/tests/smoke_typed_config.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/tests/smoke_readme_examples.py
- python/tests/smoke_typed_config.py
A missing [project] table or dependencies key would silently default to an empty list and pass the zero-dependency guard; assert both keys exist so a metadata regression fails loudly rather than going undetected. Signed-off-by: Ajay Thorve <athorve@nvidia.com>
What
Adds three dependency-light SDK smoke tests that pin WS4 contract guarantees (test-only; no runtime code changes):
python/tests/smoke_typed_config.pyplan_config/doctor_configresolve a maintained adapter with no agent directory,run_configdrives an inline run from an adapter-only dir (no agent package), and the*_configmethods raise a clearFabricNativeUnavailableErroron the CLI fallback (native-only).python/tests/smoke_consumer_neutral.pynemo_fabric(consumer glue stays undernemo_fabric.integrations); a plainimport nemo_fabricpulls in no consumer/harness package. Also pins the zero-dependency contract (dependencies = []).python/tests/smoke_readme_examples.pyplan/doctor/plan_config, the source-tree CLI-command form) execute against the example agent, with a drift guard asserting the README documents each invocation verbatim.Why
These are the WS4 "keep ..." guardrails: typed config first-class, a consumer-neutral SDK API, and accurate README examples. They harden the contract a consumer (e.g. Platform) depends on, with near-zero risk.
Tests
No external setup or secrets required (native extension only):
All three print
smoke_<name> ok. The CLI README snippets remain covered bytests/smoke_cli.py.Summary by CodeRabbit
Release Notes