[codex] Add Hermes capability hardening coverage - #23
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)
📝 WalkthroughWalkthroughSmoke tests now stub Harbor imports when Harbor is unavailable, validate Fabric artifact names, expand CLI profile planning checks, add relay-disabled observability assertions, compare SDK and CLI results for Hermes and process adapters, and update CI smoke selection. ChangesSmoke test coverage updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
b624664 to
fc34eba
Compare
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
1de08ef to
f0b4f9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/smoke_hermes_cli.py (1)
67-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract this observability helper into shared test utilities.
This contract is now duplicated here, in
tests/smoke_cli.py, and in the SDK smoke path. Keeping three near-copies in sync will drift quickly; a shared helper with optional process-only assertions would make future hardening changes safer.🤖 Prompt for 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. In `@tests/smoke_hermes_cli.py` around lines 67 - 85, The observability contract asserted in assert_relay_disabled_native_observability is duplicated across multiple smoke tests and will drift; extract the shared checks into a common test utility and reuse it from the existing helper in this file, tests/smoke_cli.py, and the SDK smoke path. Keep the helper focused on the common artifact/event/telemetry assertions, and add an optional flag or separate wrapper for process-only assertions so each caller can preserve its specific stdout/stderr/returncode checks.
🤖 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 `@tests/smoke_cli.py`:
- Around line 78-80: Update the smoke test in the CLI relay matrix to cover both
branches of the relay flag. In the test that inspects telemetry_plan, keep the
existing assertion for relay_enabled using telemetry_plan["relay_output_dir"],
and add a corresponding assertion for the disabled case to ensure that field is
absent or falsy when relay is off. Use the relay_enabled conditional in the test
body to locate the branch and make sure both enabled and disabled profiles are
validated.
---
Nitpick comments:
In `@tests/smoke_hermes_cli.py`:
- Around line 67-85: The observability contract asserted in
assert_relay_disabled_native_observability is duplicated across multiple smoke
tests and will drift; extract the shared checks into a common test utility and
reuse it from the existing helper in this file, tests/smoke_cli.py, and the SDK
smoke path. Keep the helper focused on the common artifact/event/telemetry
assertions, and add an optional flag or separate wrapper for process-only
assertions so each caller can preserve its specific stdout/stderr/returncode
checks.
🪄 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: d8e19f29-9814-43db-b109-87a63bdec8b2
📒 Files selected for processing (4)
python/tests/smoke_harbor_integration.pypython/tests/smoke_sdk.pytests/smoke_cli.pytests/smoke_hermes_cli.py
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
Summary
Closes FABRIC-37
Adds focused coverage for the remaining Hermes capability-demonstration hardening gaps without introducing a standalone closeout smoke:
tests/smoke_cli.pywith the Hermes SDK/CLI profile matrix and Relay on/off plan assertionspython/tests/smoke_sdk.pywith SDK-fallback vs direct CLI normalizedRunResultparity for Python and process adapterspopulate_context_from_resultis exercised with non-empty Fabric artifacts, using lightweight stubs when the real Harbor package is not installedValidation
python3 python/tests/smoke_sdk.pypython3 tests/smoke_cli.pypython3 tests/smoke_hermes_cli.pypython3 python/tests/smoke_harbor_integration.pygit diff --checkSummary by CodeRabbit
Bug Fixes
Tests
Chores