Conversation
MergerNeeds Review PR exceeds the merge-gate context budget (309180 tokens); escalating to a human reviewer. Commit |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| #!/usr/bin/env python3 | ||
| """Offline release-package contract tests for clawsec-ps-fuzz.""" | ||
|
|
There was a problem hiding this comment.
New skill tests are never executed
These four modules use test_*.py and are runnable only through if __name__ == "__main__": unittest.main(), while CI runs Node *.test.mjs files and .github/workflows/skill-release.yml excludes skills/*/test/** from release detection and staged archives, so their required SBOM entries do not execute the tests and regressions can pass silently. Could we convert/move the suite to *.test.mjs, or add a documented reachable Python unittest command and explicitly amend the AGENTS.md convention, since SKILL.md and README.md document no Python test command?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`skills/clawsec-ps-fuzz/test/test_package_contract.py` around lines 1-3, replace this
directly runnable-only Python unittest module with the repository’s documented
`*.test.mjs` test convention, ensuring CI discovers and executes the package contract
coverage. Update the SBOM artifact list and any related `SKILL.md`/`README.md` guidance
to reflect the new test location and command; if Python must be retained, instead add a
reachable documented unittest command and explicitly amend the repository convention and
CI workflow so these tests cannot pass silently.
|
|
||
| ## Safe scope | ||
|
|
||
| - The reviewed runtime is CPython 3.9 through 3.11 with `venv` and `pip` on glibc 2.28+ Linux x86_64/aarch64 or macOS 14+ arm64. Source provisioning also needs `git`. Provision and run fail closed on Windows in v0.1.0 because this standard-library wrapper cannot verify a current-user-private Windows DACL; read-only preflight remains available. |
There was a problem hiding this comment.
Windows preflight documentation promises unavailable interface
On Windows, preflight() calls _runtime_support() first and exits via the unsupported native-wheel platform branch, so it never produces the documented report or runs the venv/pip/configuration checks; test_preflight_reports_windows_runtime_unsupported_without_capability_probes and SKILL.md:58 currently codify that contradiction. Should we implement a Windows read-only result that skips only mutation ACL enforcement while still reporting checks, or update both public guides to say preflight fails on Windows?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`skills/clawsec-ps-fuzz/README.md` around lines 48-48 and
`skills/clawsec-ps-fuzz/SKILL.md` around line 58, fix the contradiction where Windows is
documented as supporting read-only preflight even though `preflight()` exits before
producing its inspection report. Update the runtime/preflight logic so Windows bypasses
only mutation-time ACL enforcement while still reporting venv, pip, and configuration
checks without writes or network calls; keep provision and run fail-closed on Windows.
Adjust the regression tests and documentation to accurately reflect the implemented
behavior.
| python3 scripts/ps_fuzz_runner.py preflight --source wheel \ | ||
| --target-provider open_ai --target-model gpt-4o-mini \ | ||
| --attack-provider open_ai --attack-model gpt-4o-mini \ | ||
| --tests '["system_prompt_stealer"]' --attempts 1 --threads 1 |
There was a problem hiding this comment.
Preflight examples advertise ignored run controls
The published preflight examples pass --attempts 1 --threads 1, but main() neither forwards nor reports them, so argparse silently discards the supplied values and operators receive no validation or report — should we remove these flags from both examples or add explicit preflight validation/reporting?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`skills/clawsec-ps-fuzz/README.md` around lines 72-75, update the preflight example so
it does not pass the run-only `--attempts` and `--threads` options, since `main()` does
not forward them to `preflight()` and the preflight result does not report them. Remove
the same no-op flags from the corresponding preflight example in `SKILL.md`, and keep
those controls only in run examples consistent with the smoke guide and tests.
| valid = ( | ||
| re.fullmatch(r"v[0-9]+(?:\.[0-9]+)*", tag) | ||
| and re.fullmatch(r"[0-9a-f]{40}", commit) | ||
| and re.fullmatch(r"[0-9a-f]{64}", str(release_wheel["sha256"])) | ||
| and re.fullmatch(r"prompt_security_fuzzer-[A-Za-z0-9][A-Za-z0-9_.-]*\.whl", filename) | ||
| and Path(filename).name == filename | ||
| and upstream["clone_url"] == "https://github.com/prompt-security/ps-fuzz.git" | ||
| and artifact_url.scheme == "https" | ||
| and artifact_url.netloc == "github.com" | ||
| and artifact_url.path == f"/prompt-security/ps-fuzz/releases/download/{tag}/{filename}" |
There was a problem hiding this comment.
Capability drift enables mismatched runs
load_capabilities() loads a fixed snapshot independently of the manifest, and provision() calls preflight() without comparing release identity, so run() can combine one ps-fuzz release with another release’s provider flags and attack selectors; at skills/clawsec-ps-fuzz/scripts/ps_fuzz_runner.py:1055-1064, _verify_provision_receipt() records only manifest_sha256, so changing capabilities after provisioning remains undetected. Should we require the manifest tag to match upstream_tag (or a canonical capability fingerprint) before provisioning and record/verify that fingerprint in the receipt?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
skills/clawsec-ps-fuzz/scripts/ps_fuzz_runner.py around lines 166-175 and 1055-1064,
bind the manifest validated by `load_manifest()` to the capability snapshot used by
`preflight()`, `provision()`, and `run()`; currently provisioning silently loads
capabilities independently and the receipt fingerprints only the manifest. Require the
manifest release tag to match `capabilities["upstream_tag"]` (or an equivalent canonical
capability fingerprint), pass the validated capabilities explicitly through
provisioning, and include that fingerprint/version in the provision receipt. Update
`_verify_provision_receipt()` and `run()` to reject receipts or active runs whose
capability snapshot does not match the provision-time binding.
| with urllib.request.urlopen(url) as response: # nosec B310: reviewed manifest URL | ||
| while chunk := response.read(1024 * 1024): | ||
| _write_all(descriptor, chunk) |
There was a problem hiding this comment.
Unbounded wheel download can hang or fill state root
urlopen(url) has no finite timeout, so a stalled server can block provisioning indefinitely, while unbounded response data is written to state_root/downloads before upstream.json’s SHA-256 check, allowing invalid responses to consume arbitrary disk space. Should we add a finite timeout, enforce a cumulative streaming limit regardless of Content-Length, and unlink the destination after any failed or rejected download?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`skills/clawsec-ps-fuzz/scripts/ps_fuzz_runner.py` around lines 310-312, refactor
`_download` to use a finite network timeout and enforce an explicit maximum wheel size
while streaming, validating `Content-Length` when present and tracking cumulative bytes
even when it is absent or incorrect. Ensure every timeout, read, size-limit, or other
download failure closes the descriptor and unlinks the partial destination, while
preserving the existing exclusive/private file protections and post-download hash
verification.
| _validate_provision_destination(state_root) | ||
| selected_python = _trusted_executable(python_executable, "selected Python") |
There was a problem hiding this comment.
Partial failures permanently block reprovisioning
_validate_provision_destination rejects any non-empty state root, so a crash during provisioning leaves partial state that subsequent invocations cannot reconcile or resume, permanently stranding the caller’s directory — should we persist and honor a recoverable FAILED marker, or atomically clean/reconcile partial state before retrying?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`skills/clawsec-ps-fuzz/scripts/ps_fuzz_runner.py` around lines 1209-1210, the
`provision` flow requires an empty state root but then creates virtual environments,
downloads, caches, and receipts incrementally without cleanup or durable failure
tracking, so crashes permanently strand partial state. Refactor provisioning to use a
durable state marker/state machine and reconcile or safely remove incomplete artifacts
on the next invocation before proceeding; preserve and validate completed state for
resumable retries, and ensure failures record a recoverable `FAILED` state.
| def _validate_test_authorization(confirm_authorized_test: bool, authorization_id: str) -> None: | ||
| if not confirm_authorized_test: | ||
| raise ProvisionError("--confirm-authorized-test is required for every active run") | ||
| if ( | ||
| not re.fullmatch(r"[A-Za-z0-9][A-Za-z0-9._:-]{0,63}", authorization_id) | ||
| or _looks_secret_shaped(authorization_id) | ||
| ): | ||
| raise ProvisionError("--authorization-id must be a short non-secret identifier") |
There was a problem hiding this comment.
Reusable authorization ID permits repeated runs
_validate_test_authorization and _validate_authorization accept syntax-valid IDs repeatedly without recording consumption or binding them to approved parameters, so the runner cannot distinguish fresh approval from replay — should we enforce one-time IDs and invocation binding in an authoritative caller or the runner for both test and provisioning paths?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`skills/clawsec-ps-fuzz/scripts/ps_fuzz_runner.py` around lines 1379-1386, refactor
`_validate_test_authorization` and its provisioning counterpart
`_validate_authorization` so authorization IDs are verified as one-time approvals bound
to the exact invocation parameters, rather than merely syntax-checked. Persist and
atomically consume authorization records in the protected state or require an
authoritative caller callback that provides this guarantee; reject replayed IDs and
parameter mismatches on both provision and run paths. Update the call sites and tests so
repeated use of values such as `AUTH-42` cannot authorize another invocation.
| host = parsed.hostname.lower() | ||
| netloc = host if port is None else f"{host}:{port}" | ||
| path = parsed.path.rstrip("/") | ||
| normalized = urlunparse((parsed.scheme.lower(), netloc, path, "", "", "")) |
There was a problem hiding this comment.
Approved endpoints are reconstructed incorrectly
The URL normalizer drops IPv6 brackets and parsed.params when rebuilding endpoints, so IPv6 URLs become malformed and parameterized paths can pass _approved_role_url() as a different URL before reaching the child argv — should we preserve both components or reject URL parameters before approval and launch?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`skills/clawsec-ps-fuzz/scripts/ps_fuzz_runner.py` around lines 1406-1409, fix
`_normalized_base_url` so URL reconstruction preserves endpoint identity and produces
valid child arguments. Format IPv6 hosts with brackets when rebuilding `netloc`, and
preserve `parsed.params` in `urlunparse` (or explicitly reject URL parameters before
approval and launch). Add or update tests covering bracketed IPv6 URLs and semicolon
path parameters, including approval comparisons.
| if len(values) != 4 or not strength or any(not re.fullmatch(r"\d+", value) for value in values): | ||
| return None | ||
| if footer is not None: | ||
| return None | ||
| footer = dict(zip(("broken", "resilient", "errors", "skipped"), (int(value) for value in values))) |
There was a problem hiding this comment.
Oversized footer aborts redacted report
A child-controlled digit field matching \d+ can exceed CPython 3.11’s 4,300-digit limit, so int(value) raises ValueError outside the local try and _aggregate_counts() lets it escape instead of returning None, bypassing invalid-output classification and redacted report generation. Could we bound the digit length and catch conversion failures in _aggregate_counts() by returning None?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`skills/clawsec-ps-fuzz/scripts/ps_fuzz_runner.py` around lines 1580-1584, update
`_aggregate_counts` so child-controlled numeric footer fields cannot trigger an uncaught
`ValueError` during `int()` conversion. Enforce an explicit maximum digit length before
conversion and catch any conversion failure, returning `None` so the active run
classifies the output as `invalid-output` and still writes its redacted report.
| def main(argv: Sequence[str] | None = None, *, command: Command = _run_command) -> int: | ||
| args = _parser().parse_args(argv) | ||
| manifest = load_manifest() | ||
| try: |
There was a problem hiding this comment.
Manifest failures escape stable blocked status
main() calls load_manifest() before the ProvisionError handler, so an unreadable or malformed upstream.json produces an uncaught traceback instead of the documented blocked status — should we move manifest loading inside the handler or add an equivalent outer boundary?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
skills/clawsec-ps-fuzz/scripts/ps_fuzz_runner.py around lines 1891-1894, the main() CLI
loads the manifest before the existing ProvisionError handler, allowing load_manifest()
failures to produce an uncaught traceback. Move manifest loading inside the existing try
block, or add an equivalent outer ProvisionError boundary, so unreadable or malformed
manifests consistently print the blocked message and return exit status 2 for all CLI
commands.
1 participant
User description
Opener Type
Summary
Changes Made
Related Issues
Type of Change
Testing
Checklist
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Introduce clawsec-ps-fuzz, a standalone authorized-testing workflow for provisioning pinned Prompt Security ps-fuzz releases and executing isolated direct-model fuzzing with approved providers, private state, fresh authorization gates, and aggregate-only redacted reporting. The PR also adds signed-release verification with atomic no-replace installation, dependency and capability provenance, an optional pinned loopback Gemma smoke workflow, packaging metadata, operator documentation, third-party notices, and extensive offline security-boundary tests.
Modified files (1)
Latest Contributors(1)
Modified files (6)
Latest Contributors(1)
Modified files (7)
Latest Contributors(1)
Modified files (3)
Latest Contributors(1)
Modified files (1)
Latest Contributors(1)