feat(eval): add isolated ROCm sanitizer plugins - #78
Conversation
amd-vivekag
left a comment
amd-vivekag
left a comment
There was a problem hiding this comment.
Review summary
Verdict: fix-before-ship
The architecture is thoughtful: immutable image-ID launch, image-owned worker provenance, narrow mounts, explicit plugin registration, bounded logs, and separate execution/finding states are all strong choices. A deeper full-diff pass nevertheless found several reproducible fail-closed violations where a failed or incomplete analysis can become clean, plus control-plane and provenance gaps that should be fixed before merge. I did not re-raise the PR's explicitly documented phase-isolation, capsule-origin, or top-level resume limitations as new findings.
Findings: 7 High · 7 Medium · 0 Low
High items:
- Detached descendants can escape process-group timeout cleanup and survive or wedge the sequential worker.
- FpSan can return
cleanafter a failed harness. - Multiple FpSan records are last-wins, so a later clean record can hide an earlier mismatch.
- A configured rocJITsu
race_reportcan suppress a real current stderr race. - Misspelled config keys can silently weaken
requiredto the defaultadvisorypolicy. - Reused artifact directories allow stale attestations to satisfy a later invocation.
- Replay launch geometry accepts lossy/out-of-range values that can reduce the dispatched workload.
Verification performed locally at 844761c: tests/test_docker_benchmark.sh passed; Python compile checks passed; 15 quality-loop tests passed; focused reproductions confirmed the fail-clean, detached-child, symlink-fingerprint, timeout, and replay-marker cases. The full pytest suite could not be rerun on the host because pytest is not installed; Docker image rebuilds and MI355X hardware qualification were not repeated.
| else: | ||
| # A launcher may exit after spawning a daemon that inherited our pipes. | ||
| # Evaluation commands are not allowed to leave background descendants. | ||
| if _group_alive(process_group): |
There was a problem hiding this comment.
[High] A detached child can escape timeout cleanup.
Cleanup only targets the leader's original process group. A child using setsid()/start_new_session=True survives; reproduced with termination=none and a live child, while inherited pipes can also wedge the sequential worker past its timeout.
Fix: run each invocation in a dedicated cgroup/PID namespace and kill that containment unit; add an escaped-descendant regression test.
There was a problem hiding this comment.
Fixed in 25c3ddcd. The sidecar worker now enables Linux PR_SET_CHILD_SUBREAPER, snapshots descendant PID/start-time identities before each sequential request, and cleans every newly created/adopted descendant with TERM/KILL plus reaping, including setsid() and double-fork escapees. Any such cleanup sets cleanup_required, so the typed client forces returncode=None; if survivors cannot be removed, PID 1 exits to tear down the container PID namespace. Added test_detached_descendant_is_killed_by_worker_containment and test_typed_client_never_maps_containment_cleanup_to_success for the reproduced detached-child case.
| for line in combined.splitlines(): | ||
| if line.startswith(_FPSAN_PREFIX): | ||
| try: | ||
| payload = json.loads(line[len(_FPSAN_PREFIX) :]) |
There was a problem hiding this comment.
[High] Multiple FpSan records are silently last-wins.
Each marker overwrites payload; a mismatch record followed by an equal record reproduced PASS, despite the documented one-record contract.
Fix: collect records and return TOOL_ERROR unless exactly one valid AKA_FPSAN_RESULT is present.
There was a problem hiding this comment.
Fixed in 25c3ddcd. parse_fpsan_comparison now collects every AKA_FPSAN_RESULT record instead of overwriting a single payload, rejects invalid/non-finite JSON, and returns TOOL_ERROR with fpsan_multiple_results unless the one-record contract is satisfied. Added test_fpsan_parser_rejects_multiple_result_records, including mismatch-then-equal ordering.
| reason_code="fpsan_instrumentation_not_attested", | ||
| details="FPSan outputs are meaningful only when both compared kernels were instrumented.", | ||
| ) | ||
| if returncode != 0 and payload is None: |
There was a problem hiding this comment.
[High] A failed FpSan harness can be reported clean.
The nonzero guard applies only when no payload exists; matching digests plus return code 139 reproduced PASS and would satisfy a required gate.
Fix: require returncode == 0 before returning PASS (while preserving a genuine mismatch finding if desired), and test nonzero/None return codes with valid payloads.
There was a problem hiding this comment.
Fixed in 25c3ddcd. FpSan now checks returncode != 0 before any digest comparison can return PASS; this also treats None as TOOL_ERROR. Added test_fpsan_parser_never_reports_clean_after_process_failure, covering valid matching payloads with both return code 139 and None. The HIP-FpSan positive control now also requires both probe processes to exit zero.
| # rocJITsu is configured with both stderr and file sinks. Once the | ||
| # evaluator-owned report exists it is the authoritative race stream; | ||
| # parsing stderr as well would duplicate every structured finding. | ||
| "" if report else execution.stderr, |
There was a problem hiding this comment.
[High] A configured race_report can hide a current stderr race.
Any non-empty report makes parsing discard stderr. A stale clean report plus a real current stderr race reproduced completed/clean.
Fix: always parse both current sinks with deduplication, or remove/constrain race_report to the evaluator-owned sink and truncate it before every invocation.
There was a problem hiding this comment.
Fixed in 25c3ddcd. rocJITsu now uses an evaluator-owned race.log below the fresh invocation artifact directory, truncates it before every launch, enables both stderr and file sinks, and parses both sinks together with finding deduplication. AOT replay rejects a configurable report sink. Added test_rocjitsu_current_stderr_race_is_not_hidden_by_clean_file and test_rocjitsu_aot_does_not_duplicate_file_and_stderr_race.
| return cls.disabled() | ||
| if not isinstance(config, Mapping): | ||
| raise ValueError("evaluation tools config must be a mapping") | ||
| section: Any = config.get("evaluation_tools", config) |
There was a problem hiding this comment.
[High] Misspelled keys can silently weaken required policy.
Unknown keys are ignored, so polciy: required parses as the default advisory and permits performance after tool failure.
Fix: validate run-level and per-tool mappings against explicit key allowlists before applying defaults; add typo tests for policy and identity fields.
There was a problem hiding this comment.
Fixed in 25c3ddcd. Run-level and per-tool mappings are now validated against explicit allowlists before defaults are applied, so polciy and other unknown fields raise instead of silently falling back to advisory behavior. The parser also rejects unknown enabled tools, conflicting runtime identity fields, and duplicate normalized tool names. Added test_unknown_run_and_tool_fields_fail_closed plus related identity/normalization coverage.
| --socket "${EVAL_TOOL_SOCKET_CONTAINER_DIR}/${tool}.sock" | ||
| --input-root "$EVAL_TOOL_INPUT_CONTAINER_DIR" | ||
| --scratch-root "$EVAL_TOOL_SCRATCH_CONTAINER_DIR" | ||
| --artifact-root "$EVAL_TOOL_ARTIFACT_CONTAINER_DIR" |
There was a problem hiding this comment.
[Medium] Valid configuration can exceed the worker's hidden timeout ceiling.
EvalToolsConfig accepts timeout_s=7200, but the runner never passes --max-timeout-s, so workers keep the 3600-second default and reject every such request.
Fix: pass a validated configured maximum when starting each worker, or reject values above 3600 in config and document the ceiling.
There was a problem hiding this comment.
Fixed in 25c3ddcd using the second suggested option: configuration now shares the worker ceiling of 3600 seconds. Run and per-tool timeouts must be exact integers in [1, 3600], and task/tool overrides cannot exceed the run timeout, so a config accepted by the evaluator cannot exceed the hidden worker cap. Added test_run_timeout_must_match_worker_contract and documented the limit.
| number = float(value) | ||
| except (TypeError, ValueError) as error: | ||
| raise RequestValidationError(f"{field} must be a positive number") from error | ||
| if number <= 0 or number > maximum: |
There was a problem hiding this comment.
[Medium] NaN bypasses timing validation and can remove the deadline.
Python JSON accepts NaN, and all range comparisons here are false for it. Popen.wait(timeout=NaN) waits for process completion, so an unbounded child can wedge the sequential worker.
Fix: reject non-finite values with math.isfinite, reject non-finite JSON constants, and add NaN tests for timeout and grace fields.
There was a problem hiding this comment.
Fixed in 25c3ddcd. The worker recursively rejects non-finite JSON values and _positive_number/_nonnegative_number explicitly require math.isfinite for timeout and grace fields. --max-timeout-s is validated the same way at startup. Added parameterized test_non_finite_timing_values_are_rejected for timeout_s, term_grace_s, and kill_grace_s.
| return False, "attestation_tool_mismatch" | ||
| if not self.instrumented: | ||
| return False, "artifact_not_instrumented" | ||
| command_text = " ".join(self.build_command) |
There was a problem hiding this comment.
[Medium] Required compiler flags are validated by substring.
Arguments such as -DNOTE=-fsanitize=address or --not-shared-libsan satisfy these checks without enabling instrumentation.
Fix: validate normalized argv tokens structurally and exactly, with explicit handling only for legitimate joined forms such as -I/path; add near-match negative tests.
There was a problem hiding this comment.
Fixed in 25c3ddcd. Required build flags are now matched as exact argv tokens, with narrowly supported structural equivalents only for legitimate split -I/-L and --option value forms. Substrings such as -DNOTE=-fsanitize=address and --not-shared-libsan no longer attest instrumentation. Added near-match negative tests and a split-include positive test.
| "gpu_asan_flydsl_no_device_instrumentation", | ||
| "FlyDSL 0.2.x does not insert the AMDGPU AddressSanitizer pass.", | ||
| ) | ||
| elif profile.framework in {"aiter", "rocblas", "rccl"} or profile.artifact_kind == ArtifactKind.HSACO_PRECOMPILED: |
There was a problem hiding this comment.
[Medium] rocBLAS/RCCL profiles can become GPU-ASan ready despite the strict matrix.
Setting HIP source available plus rebuilt_from_source=true takes the ready branch for rocblas and rccl, although the docs say their internal kernels are out of scope.
Fix: keep rocBLAS/RCCL unconditionally unsupported until dedicated qualified adapters exist; scope the rebuild exception only to explicitly supported lanes.
There was a problem hiding this comment.
Fixed in 25c3ddcd. GPU-ASan capability assessment now rejects rocblas and rccl up front as gpu_asan_library_kernel_out_of_scope, regardless of rebuilt_from_source or generic HIP recompilation evidence. Added parameterized test_library_kernel_gpu_asan_stays_unsupported_after_source_rebuild for both frameworks.
| str(hip_runtime), | ||
| inherited=env.get("LD_PRELOAD", ""), | ||
| ) | ||
| env["AKA_BUILD_ATTESTATION_PATH"] = str( |
There was a problem hiding this comment.
[Medium] The documented attestation_path option is disconnected from where the command writes.
The invocation always injects the default path, while parsing honors a configured path. An adapter following AKA_BUILD_ATTESTATION_PATH therefore writes one file and the parser reads another; the same split exists in both FpSan plugins.
Fix: resolve one artifact-contained path and reuse it for environment, metadata, and parsing, or remove the configurable option.
There was a problem hiding this comment.
Fixed in 25c3ddcd. GPU-ASan, Triton-FpSan, and HIP-FpSan now resolve one artifact-contained attestation path and reuse that exact value for AKA_BUILD_ATTESTATION_PATH, invocation metadata, and parsing; the parent is created before launch and path escape is rejected. Added parameterized test_configured_attestation_path_is_shared_by_invocation_and_parser plus test_attestation_path_cannot_escape_invocation_artifacts.
|
Implemented and pushed the Waitcheck/ConSan follow-up in e8d594a and 7ba42fe. What changed
Important scope boundary These are evaluator plugins implemented as isolated sidecars over the existing Unix-socket RPC. They are not skills or MCP servers, and they do not automatically inspect every optimized kernel. A task must explicitly provide the candidate-specific code object and adapter options. Waitcheck and ConSan are currently qualified only for advisory pilots; broad AITER, rocBLAS, and RCCL ConSan launch paths remain unsupported until dedicated adapters are qualified. Validation
GitHub CI and Read the Docs were queued/pending when this comment was posted. |
amd-vivekag
left a comment
amd-vivekag
left a comment
There was a problem hiding this comment.
Review summary
Verdict: fix-before-ship
All 14 findings from the previous review are fixed correctly. A full current-diff and gap-sweep pass found one remaining false-clean path in native HIP rocJITsu plus two startup-control gaps in the new Waitcheck/ConSan lanes. No bot reviews or comments were present.
Findings: 1 High · 2 Medium · 0 Low
High items:
- Native HIP rocJITsu accepts candidate-spoofable, unstructured dispatch text as a clean simulated run.
Verification: 186 eval-tools tests passed; tests/test_docker_benchmark.sh passed; git diff --check passed; a focused reproduction confirmed the spoofed rocjitsu_clean result. The full host suite could not collect under the host's older Python, while this repository documents Docker as the supported test path.
| else: | ||
| expected_kernel = context.options.get("expected_kernel") | ||
| if expected_kernel: | ||
| dispatch_seen = f'Kernel dispatch: "{expected_kernel}"' in combined |
There was a problem hiding this comment.
[High] Candidate output can spoof dispatch attestation and produce a false clean.
This unanchored substring is taken from the wrapped launcher's combined output; Kernel dispatch: "hip_kernel" plus any rocjitsu text reaches rocjitsu_clean without a structured [rocjitsu] Kernel dispatch record.
Fix: derive attestation from _KERNEL_RE, require the configured kernel among those matches, and make the parser require at least one structured dispatch; add a spoofed-launcher-output regression test.
There was a problem hiding this comment.
Fixed in ea0d0812.
- Added
rocjitsu_dispatch_kernels()as the single canonical_KERNEL_RE-based dispatch extractor. - Native HIP clean attestation now comes only from structured records in the rocJITsu report sink and requires the configured
expected_kernel; candidate stdout/stderr cannot attest a clean run. parse_rocjitsu()now returnsrocjitsu_no_dispatch_observedwhen no structured dispatch exists.- Added regressions for both the original unanchored spoof and a candidate printing the complete
[rocjitsu] Kernel dispatch: ...line, while preserving genuine report-sink and AOT clean behavior.
The focused spoof/preservation tests pass, as do the full eval-tools suite (192 tests), full repository suite (273 passed, 2 skipped, 3 subtests), and the six-sidecar gfx950 smoke. The native lane remains documented as advisory rather than a secure reward boundary.
| ): | ||
| steps["safe"] = _run_probe_step( | ||
| "safe", | ||
| [binary, str(safe), "--target", "gfx950"], |
There was a problem hiding this comment.
[Medium] Waitcheck's required positive control skips the production C API path.
A broken aka-waitcheck-capi or entrypoint can still report a ready sidecar here, then make every real evaluation fail closed.
Fix: run the production entrypoint/C-API chain on the safe fixture and validate one clean AKA_WAITCHECK_RESULT; keep the direct hazard check as a second lane.
There was a problem hiding this comment.
Fixed in ea0d0812.
Waitcheck startup now:
- compiles raw safe and hazardous gfx950 HSACOs and inventories the exact kernel entry;
- runs the image-owned production
waitcheck_entrypoint.pyandaka-waitcheck-capichain for both fixtures with exact SHA/kernel/entry binding; - validates the emitted
AKA_WAITCHECK_RESULTthrough the production parser (PASSfor safe,FINDINGfor hazard); and - retains the direct CLI hazard check as an independent second lane.
I also added a regression proving a broken production entrypoint makes the positive control fail/degrade. The physical gfx950 startup smoke exercised the real C API path and reported the sidecar ready.
| ): | ||
| steps["safe"] = _run_probe_step( | ||
| "safe", | ||
| [str(safe)], |
There was a problem hiding this comment.
[Medium] ConSan's startup control bypasses the production entrypoint and oracle split.
This proves the hook works, but not argument forwarding, oracle environment scrubbing, or the AKA_CONSAN_RUN contract used by real evaluations.
Fix: run consan_entrypoint.py for the startup fixtures with separate instrumented/oracle argv and validate its structured clean/finding outcomes.
There was a problem hiding this comment.
Fixed in ea0d0812.
ConSan startup now compiles raw safe/racy HSACOs plus an image-owned HIP module launcher, then invokes the real consan_entrypoint.py with separate instrumented --command-arg and clean --oracle-arg vectors. The launcher requires the ConSan hook environment in instrumented mode and explicitly rejects HSA_TOOLS_*/RJ_CONSAN_* leakage in oracle mode. Startup also binds exact SHA-256/FNV identities, requires the unique AKA_CONSAN_RUN contract, and validates the structured outcomes through the production parser (PASS for safe, FINDING for racy).
The unit orchestration test and physical gfx950 smoke both passed; the smoke observed CONSAN_INSTRUMENTED_LAUNCH_COMPLETED and CONSAN_ORACLE_ENV_CLEAN for both fixtures.
2 participants
Summary
This is a stacked Draft PR on #76 (
codex/quality-loop-agent). It adds an opt-in evaluation-tool stage after compilation/correctness and before performance measurement, while retaining the existing MI355X scoring runtime unchanged.lmsysorg/sglang-rocm:v0.5.14-rocm720-mi35x-20260705, FlyDSL 0.2.2, and AITER0.1.17.dev110+g9127c94a1unchanged.gfx950rocJITsu replay capsules for Triton and FlyDSL HSACO. Whole-Python JIT wrapping remains unsupported.Isolation and runtime identity
network=none, read-only root filesystems, host UID,cap-drop=ALL,no-new-privileges, bounded scratch/logs, read-only repository input, one writable per-tool socket directory, and one narrow per-worker artifact directory..eval-tool-artifactsnamespace read-only and only the current worker child read/write, preventing cross-worker writable aliases through the broad repository mount./opt/aka-eval-tools; runtime health attests the worker/probe hashes and startup positive-control result before publishing the socket.Current support boundary
triton_aotcapsule replay.gfx950:xnack+recompilation and attestation; rocJITsu ready with a native launcher; HIP-FpSan requires an explicit source port and comparison adapter.flydsl_aotcapsule replay.gfx942No bundled task is claimed as production-qualified. Automatic evaluator-owned AOT capsule capture and binding to the exact correctness dispatch, post-agent phase separation/authenticated RPC, and top-level resume fingerprint enforcement remain follow-up work; keep rollout advisory until each task/tool pair is qualified.
Real MI355X (
gfx950) validationFinal sidecar image IDs:
sha256:f28991cbfc462c2fb4d08ac6d92b8e23170f0d79a37dae1abb4ec2c1437f9c7csha256:1b48d325d5601581d378059c0bd3c2339efc4a52640d4ca94a2887b292cd1ed4sha256:e2b71a80fc3ec998e277e86ad4705234e30fc9b0c465f98bf20496a38fb24261sha256:6e6e7a834f43936a0a494be0b9acf24e7f858f57b302da80e6ea7a16e9d1fb52All four final images passed their integrated hardware startup controls. Final manager-to-sidecar candidate fixtures produced:
cleanfoundcleanfoundcleanfoundcleanfoundclean; dispatch, replay, capsule attestedfound; dispatch, replay, capsule attestedAdditional negative qualification confirmed that runtime preload alone does not cover an uninstrumented HSACO, FlyDSL does not currently receive GPU ASan instrumentation, and the AITER Python rocJITsu path fails in the current runtime. Those paths remain fail-closed in the matrix.
Tests
214 passed, 2 skipped, 3 subtests passedin the pinned scoring image (pytest -q tests).tests/test_docker_benchmark.shpasses, including immutable-image, socket, artifact containment, and cross-worker mount assertions.