feat: run offline demos for all scenarios - #10
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR extends Nullstate to run offline deterministic demonstrations across five infrastructure scenarios (AWS S3, Kubernetes, Docker Compose, on-prem SSH, generic plan review) by implementing scenario-specific finding detection, targeted remediation logic, branded CLI output, and comprehensive test coverage—enabling the hackathon case study to showcase multiple attack paths beyond Azure. ChangesMulti-scenario offline demo execution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/nullstate/report.py (1)
18-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale "Terraform" fallback message missed by the IaC rename.
Lines 24 and 54 were updated, but the fallback text on line 18 still says "Terraform", so the rendered report will read "No Terraform changes were required." even though the rest of the report now uses "IaC".
🔧 Proposed fix
- diff = patch_diff.strip() or "No Terraform changes were required." + diff = patch_diff.strip() or "No IaC changes were required."🤖 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 `@src/nullstate/report.py` at line 18, Update the stale fallback message used when computing the diff: replace the hardcoded string "No Terraform changes were required." with the IaC-consistent text (e.g. "No IaC changes were required.") where the variable diff is assigned (refer to the diff = patch_diff.strip() or ... expression in src/nullstate/report.py) so the report text matches the renamed terminology across the file.src/nullstate/cli.py (1)
92-99:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove plan-only offline coercion before the non-azure offline guard.
Line 92 checks
not offlinebefore Line 97 forces plan-only runs to offline mode. This makes--target plan-onlyfail for non-Azure scenarios unless users manually add--offline, even though the command already auto-coerces plan-only mode.Suggested fix
- if scenario_spec.name != "azure-public-blob" and not offline: - raise typer.BadParameter( - f"Scenario {scenario_spec.name!r} supports offline demo execution only for now. " - "Use --offline until its live sandbox adapter is implemented." - ) if backend.mode == "plan-only": offline = True + if scenario_spec.name != "azure-public-blob" and not offline: + raise typer.BadParameter( + f"Scenario {scenario_spec.name!r} supports offline demo execution only for now. " + "Use --offline until its live sandbox adapter is implemented." + )🤖 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 `@src/nullstate/cli.py` around lines 92 - 99, The guard that disallows non-Azure scenarios when not offline is executed before the code that auto-coerces plan-only runs to offline; move the plan-only coercion (the check of backend.mode == "plan-only" setting offline = True) above the scenario_spec.name != "azure-public-blob" and not offline check so that when backend.mode is "plan-only" the offline flag is set first and the non-azure offline guard sees offline=True; update the block that references backend.mode, offline, and scenario_spec.name accordingly.
🤖 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 `@src/nullstate/remediation.py`:
- Around line 98-104: _remediate_generic_plan_text is always reserializing the
JSON which causes spurious diffs even when _replace_public_cidr made no changes;
change it to compare the parsed payload to the updated result from
_replace_public_cidr and only call json.dumps(...) (and add the trailing
newline) when the updated object differs from the original payload, otherwise
return the original text unchanged so _remediate_files won't report
formatting-only changes.
In `@tests/test_offline_scenario_runs.py`:
- Around line 79-81: The test currently asserts the expected rule ID by indexing
findings[0], which is order-dependent and flaky; update the assertion to check
that any finding in the findings list has rule_id equal to expected_rule_id
(e.g., use an any(...) over findings comparing each finding["rule_id"] to
expected_rule_id) while keeping the existing assertTrue(findings) and
assertIn(expected_rule_id, report); locate the assertion using the symbols
findings and expected_rule_id in the test_offline_scenario_runs.py test and
replace the findings[0] equality check with an existence check across the list.
---
Outside diff comments:
In `@src/nullstate/cli.py`:
- Around line 92-99: The guard that disallows non-Azure scenarios when not
offline is executed before the code that auto-coerces plan-only runs to offline;
move the plan-only coercion (the check of backend.mode == "plan-only" setting
offline = True) above the scenario_spec.name != "azure-public-blob" and not
offline check so that when backend.mode is "plan-only" the offline flag is set
first and the non-azure offline guard sees offline=True; update the block that
references backend.mode, offline, and scenario_spec.name accordingly.
In `@src/nullstate/report.py`:
- Line 18: Update the stale fallback message used when computing the diff:
replace the hardcoded string "No Terraform changes were required." with the
IaC-consistent text (e.g. "No IaC changes were required.") where the variable
diff is assigned (refer to the diff = patch_diff.strip() or ... expression in
src/nullstate/report.py) so the report text matches the renamed terminology
across the file.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07ff74c0-4cae-4de6-8cec-9401369cc243
📒 Files selected for processing (17)
CHANGELOG.mdREADME.mddocs/architecture.mddocs/case-study.mddocs/runbook.mdexamples/generic-plan-review/README.mdexamples/generic-plan-review/tfplan.jsonpyproject.tomlsrc/nullstate/attack.pysrc/nullstate/cli.pysrc/nullstate/demo.pysrc/nullstate/findings.pysrc/nullstate/remediation.pysrc/nullstate/report.pysrc/nullstate/scenarios.pysrc/nullstate/terraform.pytests/test_offline_scenario_runs.py
Summary
Evidence to capture before merge
Closes #9python -m nullstate doctor --offlinebanner outputcompose-exposed-admin, showing success before and blocked afterVerification
python -m unittest discover -s tests -vpython -m ruff check src testspython -m mypy srcpython -m pip_audit . --skip-editablepython -m nullstate run examples/compose-exposed-admin --offline --target docker-compose --scenario compose-exposed-admin --runs-dir $env:TEMP\nullstate-compose-demo-runpython -m nullstate doctor --offlineCloses #9
Summary by CodeRabbit
New Features
Documentation
Tests