test: isolate integration harness runtime#125
Conversation
📝 WalkthroughWalkthroughThe integration harness now defaults to isolated ports, rejects unsafe production-like port usage without explicit opt-in, validates sandbox home handling, sanitizes Reflexio environment state, and uses a configurable embedding health timeout. Tests and documentation were updated to cover the new local validation paths. ChangesIsolated integration harness
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
0c129df to
87c8385
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_install_scripts.py (1)
51-57: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
_isolated_envdoesn't scrubCLAUDE_SMART_INTEGRATION_ALLOW_PROD_PORTS, breaking test isolation.The production port rejection tests depend on this env var being unset, but
_isolated_envonly popsBASH_ENVandENV. If a developer hasCLAUDE_SMART_INTEGRATION_ALLOW_PROD_PORTS=1in their shell environment,os.environ.copy()will inherit it and the script won't refuse production ports — causingtest_integration_harness_rejects_production_ports_without_opt_into fail or silently pass incorrectly.🔧 Proposed fix
def _isolated_env(tmp_path: Path) -> dict[str, str]: env = os.environ.copy() env["HOME"] = str(tmp_path) env["SHELL"] = "/bin/sh" env.pop("BASH_ENV", None) env.pop("ENV", None) + env.pop("CLAUDE_SMART_INTEGRATION_ALLOW_PROD_PORTS", None) return envAlso applies to: 1716-1747
🤖 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/test_install_scripts.py` around lines 51 - 57, The test helper _isolated_env is not fully isolating the shell environment because it leaves CLAUDE_SMART_INTEGRATION_ALLOW_PROD_PORTS inherited from os.environ. Update _isolated_env in test_install_scripts.py to scrub that variable alongside BASH_ENV and ENV so the production-port rejection tests stay deterministic; use the existing _isolated_env helper and the tests that call it, including test_integration_harness_rejects_production_ports_without_opt_in.
🤖 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.
Outside diff comments:
In `@tests/test_install_scripts.py`:
- Around line 51-57: The test helper _isolated_env is not fully isolating the
shell environment because it leaves CLAUDE_SMART_INTEGRATION_ALLOW_PROD_PORTS
inherited from os.environ. Update _isolated_env in test_install_scripts.py to
scrub that variable alongside BASH_ENV and ENV so the production-port rejection
tests stay deterministic; use the existing _isolated_env helper and the tests
that call it, including
test_integration_harness_rejects_production_ports_without_opt_in.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 66379ec8-db0c-4979-a6e7-e87a77b38a5e
📒 Files selected for processing (3)
DEVELOPER.mdtests/integration/integration.shtests/test_install_scripts.py
✅ Files skipped from review due to trivial changes (1)
- DEVELOPER.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/integration.sh
What's broken / annoying
A developer machine can have the installed claude-smart plugin running while the same developer is also debugging this checkout.
Before this PR, the source integration test used the same default ports as the installed plugin: backend
8071, embedding8072, and dashboard3001. The test also runs the normal start/stop scripts. That meant a local e2e run could accidentally stop, replace, or confuse the claude-smart services the developer depends on day to day.There was also a quieter risk when reusing a test home: an old
.claude-smart/.envcould keep managed Reflexio connection keys and point the source test at hosted Reflexio instead of the local test backend.What's changing
18071, embedding18072, dashboard13001.CLAUDE_SMART_INTEGRATION_ALLOW_PROD_PORTS=1.HOMEplus explicit local Reflexio storage/config paths, so test data stays separate from the installed plugin's data.REFLEXIO_URL,REFLEXIO_API_KEY, andREFLEXIO_USER_ID..envis written as private (0600) because it may contain local settings or secrets.EMBEDDING_HEALTH_TIMEOUT_SECONDS; the default is now long enough for slower local model warmup.DEVELOPER.mdnow separates safe source e2e from production-like private tarball validation.Local machine flows
How the product behaves afterwards
Installed claude-smart behaves the same as before. The backend, embedding daemon, and dashboard still use
8071,8072, and3001; Claude Code, Codex, and OpenCode still share the same local services and data.Only the developer integration harness changes its defaults. Developers can now debug source changes without disturbing their daily installed plugin. When they need to test exactly like production, they should build and install a private tarball instead of running source e2e on production ports.
How it was verified
bash -n tests/integration/integration.shgit diff --checkuv run --project plugin --with pytest pytest tests/test_install_scripts.py::test_integration_harness_prepares_vendored_reflexio tests/test_install_scripts.py::test_integration_harness_defaults_to_isolated_runtime tests/test_install_scripts.py::test_integration_harness_rejects_production_ports_without_opt_in tests/test_install_scripts.py::test_integration_harness_rejects_non_numeric_ports tests/test_install_scripts.py::test_integration_harness_sanitizes_reused_managed_env tests/test_host_integration_e2e.py -quv run --project plugin --with pytest pytest tests/test_install_scripts.py -qbash tests/integration/integration.sh56668at8071, embedding stayed on PID56666at8072, and dashboard stayed on PID82939at3001.18071,18072, or13001.Reviewer notes
.envsanitizer test now proves comment preservation and the intentional0600privacy behavior.Summary by CodeRabbit
EMBEDDING_HEALTH_TIMEOUT_SECONDS.