feat: add constrained red attack runner - #24
Merged
Merged
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGenerates and executes allowlisted run-directory ChangesConstrained Attack Script Execution and Reporting
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ 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: 3
🤖 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 `@README.md`:
- Line 50: Confirm whether the "stage" field is actually emitted to events.jsonl
by the red tool runner (the generator that writes events.jsonl alongside
generated attack.py runs) and if so, update the README text to include "stage"
in the list of recorded fields; if it is not emitted, update the PR description
to remove "stage" from the objectives and/or add a note in the README explaining
why it is omitted. While editing the README sentence, consider splitting the
long sentence into 2–3 shorter sentences and ensure the links to Security Model
and Threat Model remain intact.
In `@src/nullstate/attack_runner.py`:
- Around line 53-60: The subprocess.run call in attack_runner.py (the block that
sets completed = subprocess.run(...)) can raise subprocess.TimeoutExpired; catch
subprocess.TimeoutExpired around that call and convert it into an
AttackToolResult instead of letting it propagate: import or reference
subprocess.TimeoutExpired, set a timed_out/timeout flag in the AttackToolResult,
populate stdout and stderr using the exception's output and stderr attributes
(or empty strings if missing), include the timeout_seconds and
command/resolved_run_dir in the result metadata, and return that
AttackToolResult; ensure existing success/failure code paths (which use the
completed variable) remain unchanged for non-timeout cases.
In `@src/nullstate/attack.py`:
- Around line 15-27: Duplicate probe_target logic found in multiple script
templates (probe_target in src/nullstate/attack.py and the same function in the
azure-public-blob and aws-public-s3 templates); extract it into a shared
template helper (e.g., a template fragment named probe_target_helper) and update
each script template to include or import that helper instead of embedding the
function inline. Ensure the shared helper preserves the same signature
(probe_target(target_url: str, stage: str) -> int), same behavior around URL
validation, health_url construction, urllib.request.urlopen usage and error
handling (return codes 0/2), and update the templates to reference the helper
symbol so changes are centralized.
🪄 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: 415be843-8957-4d70-b2c3-415454300e1f
📒 Files selected for processing (9)
README.mddocs/case-study.mddocs/demo-script.mddocs/security-model.mdsrc/nullstate/attack.pysrc/nullstate/attack_runner.pysrc/nullstate/cli.pytests/test_attack_runner.pytests/test_cli.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/nullstate/attack_runner.py (1)
57-64:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUnhandled
subprocess.TimeoutExpiredstill propagates to callers.
subprocess.run(..., timeout=timeout_seconds)raisessubprocess.TimeoutExpiredwhen the timeout is hit, but it is not caught. Callers incli.run(Line 295 and Line 375) expect anAttackToolResultand have no handler, so a slow/hungattack.pywould crash the entire run instead of producing a recorded result.🛡️ Proposed fix to convert timeout into a result
started_at = datetime.now(UTC).isoformat() started = time.monotonic() - completed = subprocess.run( - command, - cwd=resolved_run_dir, - text=True, - capture_output=True, - check=False, - timeout=timeout_seconds, - ) + try: + completed = subprocess.run( + command, + cwd=resolved_run_dir, + text=True, + capture_output=True, + check=False, + timeout=timeout_seconds, + ) + except subprocess.TimeoutExpired as exc: + return AttackToolResult( + command=command, + target_url=target_url, + stage=stage, + returncode=-1, + stdout=exc.stdout or "", + stderr=exc.stderr or "", + started_at=started_at, + ended_at=datetime.now(UTC).isoformat(), + duration_seconds=round(time.monotonic() - started, 3), + ) ended_at = datetime.now(UTC).isoformat()🤖 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/attack_runner.py` around lines 57 - 64, The subprocess.run call in src/nullstate/attack_runner.py can raise subprocess.TimeoutExpired and currently propagates; catch subprocess.TimeoutExpired around the subprocess.run(...) invocation and convert it into an AttackToolResult indicating a timeout (populate any available output via the exception's output/stderr attributes, set an appropriate non-zero return code or timeout flag, and include the timeout_seconds context). Update the error path that constructs the AttackToolResult so callers of cli.run receive a result object instead of an exception; reference the subprocess.run call and the AttackToolResult type when making the change.
🤖 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 `@docs/case-study.md`:
- Around line 86-89: The README uses the vaguer term "timestamps" while
docs/case-study.md explicitly states the runner records "start time, end time,
and duration" for each run; update the README to match this precise terminology
so both places describe the runner/nullstate behavior consistently (referencing
the runner that executes attack.py in the run directory and the events.jsonl
artifact) — replace "timestamps" with "start time, end time, and duration" and
ensure the phrasing matches the case-study wording.
- Around line 39-40: The docs/case-study.md step describing logging to
events.jsonl omits the "stage" field; if the implementation emits a "stage"
property it must be documented here for consistency with the PR objectives and
README—update the events.jsonl bullet to list the "stage" field (e.g.,
pre-attack, post-remediation) and briefly describe its purpose and possible
values so readers know to expect and interpret the "stage" attribute in
events.jsonl.
In `@docs/security-model.md`:
- Line 52: The event log documentation omits the "--stage" parameter from the
recorded fields; update the event log description to include "stage" as a
recorded field so that the event log (which already lists command, stdout,
stderr, return code, target URL, start time, end time, and duration) also
records the stage value provided by the --stage dynamic input to the attack
script for complete traceability.
---
Duplicate comments:
In `@src/nullstate/attack_runner.py`:
- Around line 57-64: The subprocess.run call in src/nullstate/attack_runner.py
can raise subprocess.TimeoutExpired and currently propagates; catch
subprocess.TimeoutExpired around the subprocess.run(...) invocation and convert
it into an AttackToolResult indicating a timeout (populate any available output
via the exception's output/stderr attributes, set an appropriate non-zero return
code or timeout flag, and include the timeout_seconds context). Update the error
path that constructs the AttackToolResult so callers of cli.run receive a result
object instead of an exception; reference the subprocess.run call and the
AttackToolResult type when making the change.
🪄 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: fb1e066c-47e9-4cca-8780-d64077c93452
📒 Files selected for processing (27)
README.mddocs/architecture.mddocs/case-study.mddocs/demo-script.mddocs/enterprise-roadmap.mddocs/handoff.mddocs/plans/2026-06-01-real-sandbox-red-team-commands.mddocs/security-model.mddocs/technical-walkthrough.mdexamples/aws-public-s3/main.tfsrc/nullstate/attack.pysrc/nullstate/attack_manifest.pysrc/nullstate/attack_runner.pysrc/nullstate/bundle.pysrc/nullstate/cli.pysrc/nullstate/dashboard.pysrc/nullstate/demo.pysrc/nullstate/remediation.pysrc/nullstate/report.pysrc/nullstate/scenarios.pytests/test_attack_manifest.pytests/test_attack_runner.pytests/test_bundle_dashboard.pytests/test_cli.pytests/test_remediation.pytests/test_report.pytests/test_scenarios.py
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/nullstate/attack_runner.py (1)
67-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHash evidence is captured after execution, not for the executed artifact.
Line 95 and Line 96 hash files only after the process exits. If
attack.py(or manifest) mutates during execution, the recorded digest no longer proves what was actually executed.Proposed fix
started_at = datetime.now(UTC).isoformat() started = time.monotonic() + attack_script_sha256 = _sha256_file(resolved_script) + manifest_sha256 = _sha256_file(resolved_manifest) if resolved_manifest is not None else None completed = subprocess.run( command, cwd=resolved_run_dir, text=True, capture_output=True, check=False, timeout=timeout_seconds, ) @@ - attack_script_sha256=_sha256_file(resolved_script), - manifest_sha256=_sha256_file(resolved_manifest) if resolved_manifest is not None else None, + attack_script_sha256=attack_script_sha256, + manifest_sha256=manifest_sha256, )🤖 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/attack_runner.py` around lines 67 - 97, The recorded SHA256 hashes for the executed artifacts are computed after subprocess.run, which risks capturing mutated files; compute the hashes before execution instead: call _sha256_file(resolved_script) and, if resolved_manifest is not None, _sha256_file(resolved_manifest) prior to invoking subprocess.run in the function that builds the command (the scope containing started_at, started, completed, etc.), store those values (e.g., attack_script_sha256 and manifest_sha256), then use those stored variables when constructing the AttackToolResult so the recorded digests reflect the exact files executed.src/nullstate/cli.py (1)
698-706:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEndpoint detail text can contradict configured provider state.
When only
NULLSTATE_RED_LLM_PROVIDER/NULLSTATE_BLUE_LLM_PROVIDERare set,_llm_configured()returns true, but_endpoint_status_detail()can still print the “Set …” fallback message.Proposed fix
def _endpoint_status_detail() -> str: @@ if shared: return f"shared={_redact_url(shared)}" - provider = os.getenv("NULLSTATE_LLM_PROVIDER") - if provider: - return f"provider={provider}" + provider = os.getenv("NULLSTATE_LLM_PROVIDER") + red_provider = os.getenv("NULLSTATE_RED_LLM_PROVIDER") + blue_provider = os.getenv("NULLSTATE_BLUE_LLM_PROVIDER") + if red_provider or blue_provider: + return f"red_provider={red_provider or 'missing'}, blue_provider={blue_provider or 'missing'}" + if provider: + return f"provider={provider}" return "Set NULLSTATE_LLM_BASE_URL, role-specific endpoints, or NULLSTATE_LLM_PROVIDER=google or claude."Also applies to: 709-720
🤖 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 698 - 706, _llm_configured() can return True when only provider env vars (NULLSTATE_RED_LLM_PROVIDER/NULLSTATE_BLUE_LLM_PROVIDER) are set while _endpoint_status_detail() still prints the “Set …” fallback for base URL; update _endpoint_status_detail() to determine the message based on the same configuration logic as _llm_configured(): specifically check provider-specific env vars (NULLSTATE_RED_LLM_PROVIDER, NULLSTATE_BLUE_LLM_PROVIDER, NULLSTATE_LLM_PROVIDER) as well as the corresponding base URL env vars (NULLSTATE_RED_LLM_BASE_URL, NULLSTATE_BLUE_LLM_BASE_URL, NULLSTATE_LLM_BASE_URL) and only show the “Set …” fallback when neither a provider nor a base URL is configured for that endpoint; apply the same fix to the other endpoint-status helper logic referenced around the 709-720 region so messages and _llm_configured() remain consistent.
🤖 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/artifact_scrubber.py`:
- Around line 46-62: The TEXT_SUFFIXES set in artifact_scrubber.py currently
omits Terraform state filenames, allowing files like "terraform.tfstate" and
"terraform.tfstate.backup" to bypass text-based scrubbing; update the scrubbing
logic by adding ".tfstate" and ".tfstate.backup" (or explicitly include the
exact names "terraform.tfstate" and "terraform.tfstate.backup" if the code
treats full filenames) to TEXT_SUFFIXES (and similarly update any other location
referencing TEXT_SUFFIXES around lines 109-110) so Terraform state files are
treated as text and go through the same scrubbing pipeline used by functions
that consult TEXT_SUFFIXES.
- Around line 15-31: The secret-scrubbing regexes (the tuples named
"model_api_key", "azure_client_secret", "bearer_token" and similar) only match
env-style KEY=value or KEY: value and miss quoted JSON/YAML forms like "KEY":
"value"; update those regex patterns in src/nullstate/artifact_scrubber.py to
also match optional surrounding quotes for keys and values and the JSON-style
separator (":") with optional whitespace and optional quotes around the secret
value (e.g., allow patterns like ["']?KEY["']?\s*[:=]\s*["']?VALUE["']?), so the
replacements redact both env-style and quoted JSON/YAML key/value pairs without
changing the replacement strings.
In `@src/nullstate/metrics.py`:
- Around line 84-87: The substring check using "provider in host" is too
permissive; replace it with a precise host/domain match (use host == provider or
host.endswith('.' + provider)) and account for optional port by stripping any
":port" part before comparison so managed-host detection (the host variable and
the provider tuple ("fireworks.ai", "together.ai", "openai.com",
"anthropic.com", "generativelanguage.googleapis.com")) only matches exact
domains or proper subdomains rather than arbitrary substrings.
---
Outside diff comments:
In `@src/nullstate/attack_runner.py`:
- Around line 67-97: The recorded SHA256 hashes for the executed artifacts are
computed after subprocess.run, which risks capturing mutated files; compute the
hashes before execution instead: call _sha256_file(resolved_script) and, if
resolved_manifest is not None, _sha256_file(resolved_manifest) prior to invoking
subprocess.run in the function that builds the command (the scope containing
started_at, started, completed, etc.), store those values (e.g.,
attack_script_sha256 and manifest_sha256), then use those stored variables when
constructing the AttackToolResult so the recorded digests reflect the exact
files executed.
In `@src/nullstate/cli.py`:
- Around line 698-706: _llm_configured() can return True when only provider env
vars (NULLSTATE_RED_LLM_PROVIDER/NULLSTATE_BLUE_LLM_PROVIDER) are set while
_endpoint_status_detail() still prints the “Set …” fallback for base URL; update
_endpoint_status_detail() to determine the message based on the same
configuration logic as _llm_configured(): specifically check provider-specific
env vars (NULLSTATE_RED_LLM_PROVIDER, NULLSTATE_BLUE_LLM_PROVIDER,
NULLSTATE_LLM_PROVIDER) as well as the corresponding base URL env vars
(NULLSTATE_RED_LLM_BASE_URL, NULLSTATE_BLUE_LLM_BASE_URL,
NULLSTATE_LLM_BASE_URL) and only show the “Set …” fallback when neither a
provider nor a base URL is configured for that endpoint; apply the same fix to
the other endpoint-status helper logic referenced around the 709-720 region so
messages and _llm_configured() remain consistent.
🪄 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: 85f9b76d-5f70-41ac-bb9f-aba17f6a8312
📒 Files selected for processing (30)
.env.example.gitignoreREADME.mddocs/case-study.mddocs/enterprise-readiness.mddocs/failure-modes.mddocs/handoff.mddocs/model-serving.mddocs/plans/2026-06-01-real-sandbox-red-team-commands.mddocs/runbook.mddocs/security-model.mddocs/technical-walkthrough.mdexamples/azure-public-blob/main.tfsrc/nullstate/agents.pysrc/nullstate/artifact_scrubber.pysrc/nullstate/attack.pysrc/nullstate/attack_manifest.pysrc/nullstate/attack_runner.pysrc/nullstate/cli.pysrc/nullstate/demo.pysrc/nullstate/llm_providers.pysrc/nullstate/metrics.pysrc/nullstate/report.pytests/test_attack_manifest.pytests/test_attack_runner.pytests/test_bundle_dashboard.pytests/test_cli_model_endpoint.pytests/test_llm_providers.pytests/test_report.pytests/test_sandbox.py
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
README.md (1)
73-73:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the "stage" field to the documented events.jsonl fields.
The PR objectives state that events.jsonl records "command, stdout, stderr, return code, target URL, stage, and timestamps," but the current README text omits "stage." The technical-walkthrough.md correctly documents the
stagefield in thered-toolevent structure (before/after stages at lines 288, 358). Update this line to include "stage" in the documented list to match the implementation and PR objectives.📝 Suggested fix
-The red tool runner is constrained to generated `attack.py` scripts inside the run directory and records command, stdout, stderr, return code, target URL, and timestamps in `events.jsonl`. +The red tool runner is constrained to generated `attack.py` scripts inside the run directory and records command, stdout, stderr, return code, target URL, stage, and timestamps in `events.jsonl`.🤖 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 `@README.md` at line 73, Update the README sentence describing events.jsonl to include the "stage" field so it matches the implementation and PR objectives: modify the sentence that lists recorded fields for the red tool runner's events.jsonl (currently enumerating "command, stdout, stderr, return code, target URL, and timestamps") to add "stage" (e.g., "command, stdout, stderr, return code, target URL, stage, and timestamps") so the README aligns with the documented red-tool event structure and technical-walkthrough.docs/technical-walkthrough.md (1)
381-400: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRefactor sentence structure to reduce repetition in the artifact descriptions.
Lines 381–400 describe multiple artifact outputs with four consecutive sentences beginning with
`nullstatecommands (lines 392, 394, 396, 398). While the content is clear, the repetitive sentence structure reduces readability. Consider varying the sentence structure—for example, combining related items or using a list format.✍️ Suggested refactoring for improved readability
Instead of repeating
`nullstate [command]` writes...:-`nullstate sarif` writes `nullstate.sarif`, a SARIF 2.1.0 export with one result per finding for CI and code-scanning upload. +The following commands produce machine-readable outputs for CI and integration: + +- `nullstate sarif` writes `nullstate.sarif` (SARIF 2.1.0, one result per finding) for CI and code-scanning upload. +- `nullstate run --ci` writes `ci-summary.json` and exits with code `2` when findings meet or exceed `--fail-on-severity`. +- `nullstate baseline` writes a baseline JSON of finding identities. When combined with `--baseline-file`, the CI failure threshold evaluates only new findings. +- `nullstate upload --dry-run` writes `upload-plan.json`, refreshes `run-bundle.json`, and records endpoint intent and token environment presence without storing values.This structure reduces repetition while preserving clarity and detail.
🤖 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 `@docs/technical-walkthrough.md` around lines 381 - 400, Refactor the repeated sentences that start with `nullstate report`, `nullstate bundle`, `nullstate dashboard`, `nullstate sarif`, `nullstate run --ci`, `nullstate baseline`, `nullstate upload --dry-run`, and `nullstate scrub` so the artifact descriptions read more smoothly: either combine related items into a single sentence or convert them into a concise bullet/list form that states the command and its produced artifact(s) once per line, preserving each artifact name and behavior (e.g., "opens", "writes", "creates", "writes and exits with code 2", "writes baseline", "writes upload plan and confirms token", "creates scrubbed copy and report") while removing the repetitive "nullstate [command] writes..." phrasing.src/nullstate/attack_runner.py (1)
78-85:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winHandle
subprocess.TimeoutExpiredexception to prevent runner crashes.The
subprocess.runcall can raisesubprocess.TimeoutExpiredwhen the timeout is exceeded, but this exception is not caught. The runner should catch this exception and convert it to anAttackToolResultwith appropriate metadata (such asreturncode=-1or a timeout-specific flag) to ensure callers always receive a structured result rather than an uncaught exception.🛡️ Proposed fix to handle timeout
started_at = datetime.now(UTC).isoformat() started = time.monotonic() -completed = subprocess.run( - command, - cwd=resolved_run_dir, - text=True, - capture_output=True, - check=False, - timeout=timeout_seconds, -) +try: + completed = subprocess.run( + command, + cwd=resolved_run_dir, + text=True, + capture_output=True, + check=False, + timeout=timeout_seconds, + ) +except subprocess.TimeoutExpired as exc: + ended_at = datetime.now(UTC).isoformat() + stdout, stdout_truncated = _truncate_text(exc.stdout or "", max_output_bytes) + stderr, stderr_truncated = _truncate_text(exc.stderr or "", max_output_bytes) + return AttackToolResult( + schema_version=1, + command_policy_id=command_policy_id, + command=command, + target_url=target_url, + target_classification=target_classification, + stage=stage, + returncode=-1, + stdout=stdout, + stderr=stderr, + stdout_truncated=stdout_truncated, + stderr_truncated=stderr_truncated, + started_at=started_at, + ended_at=ended_at, + duration_seconds=round(time.monotonic() - started, 3), + attack_script_sha256=_sha256_file(resolved_script), + manifest_sha256=_sha256_file(resolved_manifest) if resolved_manifest is not None else None, + ) ended_at = datetime.now(UTC).isoformat()🤖 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/attack_runner.py` around lines 78 - 85, Wrap the subprocess.run call that assigns to completed in a try/except catching subprocess.TimeoutExpired in attack_runner.py (the block where completed = subprocess.run(...)); on TimeoutExpired, construct and return an AttackToolResult (or populate the same result path used for completed) with a distinct indicator for timeout (e.g., returncode = -1 or a timed_out/timeout flag in metadata), include available stdout/stderr from the exception (e.output / e.stderr or empty strings) and any relevant timing info so callers always receive a structured AttackToolResult instead of an uncaught exception.
🤖 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 @.github/workflows/nullstate-sarif.yml:
- Around line 15-16: The workflow currently pins third-party actions using
mutable tags (actions/checkout@v6, actions/setup-python@v6,
github/codeql-action/upload-sarif@v4, actions/upload-artifact@v6); replace each
tag with the corresponding full commit SHA for that release (e.g.,
actions/checkout@<full-sha>, actions/setup-python@<full-sha>,
github/codeql-action/upload-sarif@<full-sha>,
actions/upload-artifact@<full-sha>) so the workflow uses immutable references;
fetch the exact commit SHAs from each action's GitHub repo release/tag page and
update the references in .github/workflows/nullstate-sarif.yml accordingly.
In `@docs/handoff.md`:
- Around line 10-15: Consolidate the duplicate freeze rules that say "Do not
merge anything into `main`." and "Do not push or merge updates to `main`." into
a single clear sentence (e.g., a unified freeze rule referencing `main`) and
immediately follow it with the existing allowance for feature-branch checkpoint
pushes (keep the phrase "feature branches" or "checkpoint commits" so intent is
preserved); update the lines containing those two statements so only one unified
freeze rule appears and ensure the subsequent bullet about feature branch pushes
remains unchanged.
In `@src/nullstate/policy.py`:
- Around line 37-44: Wrap the file read and JSON parse in load_attack_policy in
a try/except that catches json.JSONDecodeError (and optionally OSError from
path.read_text), and re-raise a clearer exception (e.g., ValueError or a custom
error) that includes the policy path and the original exception message; update
the return path in load_attack_policy to only proceed to construct AttackPolicy
when parsing succeeds and include the original exception as context (e.g., "from
err") so callers can inspect the underlying error.
In `@tests/test_cli_ci.py`:
- Around line 14-30: The subprocess.run calls that assign to the variable
completed can hang; add an explicit timeout parameter (e.g., timeout=300) to
each subprocess.run invocation in tests/test_cli_ci.py (the call building the
args list and any other similar run at the later block around lines 48-66) so
the test fails fast on hangs; ensure you choose a sensible timeout value and
handle subprocess.TimeoutExpired if the test needs to capture output on timeout.
In `@tests/test_github_workflows.py`:
- Around line 12-16: The test uses brittle exact-string assertions for workflow
commands; update the assertions in tests/test_github_workflows.py to check
smaller invariants instead: assert the step names or base command (e.g., that
the rendered step contains "nullstate run" and separately assert presence of
critical flags like "--offline", "--mock-agents", "--ci", "--fail-on-severity
none", and "--runs-dir runs/ci"), and for the SARIF step assert it contains
"nullstate sarif" plus "--runs-dir" and "--output artifacts/nullstate.sarif".
Locate the two assertIn calls shown and replace each long literal check with
multiple assertIn checks for these substrings so formatting/line-wrapping
changes won’t break the test.
In `@tests/test_upload.py`:
- Around line 77-79: The test currently checks that "super-secret-token" is not
present in plan_text and completed.stdout but misses stderr; update the test in
tests/test_upload.py to also assert that the secret is redacted from the
subprocess stderr by adding a check like self.assertNotIn("super-secret-token",
completed.stderr) alongside the existing assertions for plan_text and
completed.stdout so all outputs (plan JSON, stdout, stderr) are covered.
---
Outside diff comments:
In `@docs/technical-walkthrough.md`:
- Around line 381-400: Refactor the repeated sentences that start with
`nullstate report`, `nullstate bundle`, `nullstate dashboard`, `nullstate
sarif`, `nullstate run --ci`, `nullstate baseline`, `nullstate upload
--dry-run`, and `nullstate scrub` so the artifact descriptions read more
smoothly: either combine related items into a single sentence or convert them
into a concise bullet/list form that states the command and its produced
artifact(s) once per line, preserving each artifact name and behavior (e.g.,
"opens", "writes", "creates", "writes and exits with code 2", "writes baseline",
"writes upload plan and confirms token", "creates scrubbed copy and report")
while removing the repetitive "nullstate [command] writes..." phrasing.
In `@README.md`:
- Line 73: Update the README sentence describing events.jsonl to include the
"stage" field so it matches the implementation and PR objectives: modify the
sentence that lists recorded fields for the red tool runner's events.jsonl
(currently enumerating "command, stdout, stderr, return code, target URL, and
timestamps") to add "stage" (e.g., "command, stdout, stderr, return code, target
URL, stage, and timestamps") so the README aligns with the documented red-tool
event structure and technical-walkthrough.
In `@src/nullstate/attack_runner.py`:
- Around line 78-85: Wrap the subprocess.run call that assigns to completed in a
try/except catching subprocess.TimeoutExpired in attack_runner.py (the block
where completed = subprocess.run(...)); on TimeoutExpired, construct and return
an AttackToolResult (or populate the same result path used for completed) with a
distinct indicator for timeout (e.g., returncode = -1 or a timed_out/timeout
flag in metadata), include available stdout/stderr from the exception (e.output
/ e.stderr or empty strings) and any relevant timing info so callers always
receive a structured AttackToolResult instead of an uncaught exception.
🪄 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: 0a09c9fc-02cb-4af4-9a2d-6fde7acd2336
📒 Files selected for processing (21)
.github/workflows/nullstate-sarif.ymlREADME.mddocs/ci-cd.mddocs/enterprise-readiness.mddocs/enterprise-roadmap.mddocs/handoff.mddocs/progress.mddocs/technical-walkthrough.mdsrc/nullstate/attack_runner.pysrc/nullstate/baseline.pysrc/nullstate/ci.pysrc/nullstate/cli.pysrc/nullstate/policy.pysrc/nullstate/sarif.pysrc/nullstate/upload.pytests/test_attack_policy.pytests/test_baseline.pytests/test_cli_ci.pytests/test_github_workflows.pytests/test_sarif_export.pytests/test_upload.py
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Ker102
marked this pull request as ready for review
June 18, 2026 14:44
Owner
Author
|
@coderabbitai skip\n\nMaintainer requested no additional CodeRabbit review loop on this already-reviewed PR. This push only clears verified merge-blocking review threads before merging. |
|
✅ Action performedReviews paused. |
Merged
Ker102
added a commit
that referenced
this pull request
Jun 18, 2026
Record that PR #24 was merged after the hackathon freeze was lifted and refresh handoff/progress docs for future sessions.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
feature/red-agent-runnerwithout touchingmain.Safety / Merge Policy
mainwithout explicit user approval.Verification
C:\Users\ivo\AppData\Local\Programs\Python\Python312\python.exe -m ruff check src testspassed.C:\Users\ivo\AppData\Local\Programs\Python\Python312\python.exe -m mypy srcpassed.C:\Users\ivo\AppData\Local\Programs\Python\Python312\python.exe -m unittest discover -s tests -vpassed: 112 tests OK.Notes For Reviewers
attack.pycommand path; they are not a general shell permission system.