feat(rust): enforce hot-path authority and remove PostToolUse Python fallback - #2598
feat(rust): enforce hot-path authority and remove PostToolUse Python fallback#2598kantorcodes wants to merge 10 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
PR Summary by QodoEnforce Rust PostToolUse authority and fail-safe native routing
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| if [[ "${{ needs.ownership.outputs.rust }}" == "true" || "${{ needs.ownership.outputs.resident_integration }}" == "true" ]]; then | ||
| test "${{ needs.rust-local-integration.result }}" = "success" | ||
| fi |
There was a problem hiding this comment.
1. Required authority jobs ignored 🐞 Bug ≡ Correctness
The classifier selects all manifest required_jobs, but the workflow only runs and validates rust-local-integration when rust or resident_integration is selected. Changes requiring all-harness, pretool-integration, command-differential, transport-faults, performance, native-release, or installed-wheel can therefore pass authority-summary without those declared gates running.
Agent Prompt
## Issue description
The authority summary ignores most jobs selected from the ownership manifest, allowing protected changes to pass without their declared validation.
## Issue Context
Every `required_jobs` identifier must map to an actual workflow job and its selected result must be enforced by the summary job.
## Fix Focus Areas
- .github/workflows/rust-hotpath-authority.yml[17-114]
- ci/rust-hotpath-ownership.toml[12-102]
- scripts/ci/rust_hotpath_ownership.py[181-198]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| def _git_paths(root: Path, base: str, head: str) -> list[str]: | ||
| completed = subprocess.run( | ||
| ["git", "diff", "--name-only", "--find-renames", f"{base}...{head}"], | ||
| cwd=root, |
There was a problem hiding this comment.
2. Renames bypass path authority 🐞 Bug ⛨ Security
git diff --name-only --find-renames does not independently classify both sides of a detected rename, while classification only examines the returned names. Renaming a protected hot-path file to a path outside HOT_PATH_PREFIXES can therefore omit the old protected path and avoid its required jobs and unmapped-path rejection.
Agent Prompt
## Issue description
Detected renames can hide the protected source path from ownership classification.
## Issue Context
Parse a name-status or raw diff format and add both old and new paths for renames and copies before classifying them; add self-tests for rename-out, rename-in, and deletion cases.
## Fix Focus Areas
- scripts/ci/rust_hotpath_ownership.py[133-174]
- scripts/ci/rust_hotpath_ownership.py[201-235]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if: needs.ownership.outputs.rust == 'true' || needs.ownership.outputs.resident_integration == 'true' | ||
| runs-on: [self-hosted, guard] | ||
| timeout-minutes: 25 |
There was a problem hiding this comment.
3. Pr code reaches self-hosted runner 🐞 Bug ⛨ Security
The workflow runs for pull requests and checks out, builds, and executes the proposed revision on a persistent self-hosted, guard runner. A malicious PR can execute arbitrary Cargo build scripts or modify the invoked Python integration script, exposing or persisting on the runner despite the workflow's read-only GitHub token.
Agent Prompt
## Issue description
Untrusted pull-request code is executed directly on a self-hosted runner.
## Issue Context
Use GitHub-hosted or genuinely ephemeral isolated runners for pull-request events, or restrict the self-hosted integration to trusted post-merge/manual revisions while retaining a required safe PR gate.
## Fix Focus Areas
- .github/workflows/rust-hotpath-authority.yml[3-8]
- .github/workflows/rust-hotpath-authority.yml[66-95]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| response = attach_native_decision_receipt(response, receipt) | ||
| record_native_decision(event_name, harness, receipt) |
There was a problem hiding this comment.
4. Route evidence is unreachable 🐞 Bug ◔ Observability
The worker stores the receipt only in HookReviewResponse.metrics, which to_harness_json() omits, and the new aggregate snapshot function has no callers. As a result, the advertised backend receipts and aggregate route counters never reach any response, telemetry sink, report, or artifact.
Agent Prompt
## Issue description
New route evidence is collected only in process memory and is never exposed to an observability consumer.
## Issue Context
Wire privacy-safe receipts and aggregate snapshots into an existing metrics/health/reporting sink without adding them to user-visible content fields.
## Fix Focus Areas
- src/codex_plugin_scanner/guard/daemon/hook_worker.py[109-135]
- src/codex_plugin_scanner/guard/native_route_metrics.py[95-117]
- src/codex_plugin_scanner/guard/runtime/hook_review_types.py[106-130]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if receipt.decision_backend.startswith("rust_"): | ||
| _RUST += 1 | ||
| else: | ||
| _PYTHON += 1 |
There was a problem hiding this comment.
5. Failures inflate rust decisions 🐞 Bug ◔ Observability
When native review returns no result, Python constructs the fail-safe block and labels it rust_fail_safe, while the counter treats every rust_ backend as a Rust decision. Native outages therefore increase rust_decisions and can keep rust_decision_share at 100% even when no Rust decision was produced.
Agent Prompt
## Issue description
Python-generated fail-safe blocks are counted as successful Rust decisions.
## Issue Context
Separate native decisions, native-unavailable fail-safe outcomes, and Python compatibility decisions in totals and shares so availability failures cannot satisfy Rust decision-share thresholds.
## Fix Focus Areas
- src/codex_plugin_scanner/guard/daemon/hook_worker.py[92-106]
- src/codex_plugin_scanner/guard/native_route_metrics.py[81-117]
- ci/rust-hotpath-ownership.toml[6-10]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Local integration snapshot source: release/3.0 source archive. This comment will be replaced with the final integration evidence after review remediation. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| raise RuntimeError(f"{harness} secret-bearing content was not blocked by Rust") | ||
| elif result.get("policy_action") != "block" or result.get("model_output_action") != "block": | ||
| raise RuntimeError(f"{harness} secret-bearing content was not blocked by Rust") | ||
| if "reviewed_excerpt" in result or _SECRET_TEXT in json.dumps(result): |
There was a problem hiding this comment.
WARNING: JSON-escaped secret text check is ineffective
json.dumps(result) escapes newlines as \n (two characters), but _SECRET_TEXT contains raw newline bytes. The in check will never match, so secret-bearing content leakage in harness responses goes undetected.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if report.get("rust_decisions") != expected or report.get("python_decisions") != 0: | ||
| raise RuntimeError("persisted native route report did not match all-harness outcomes") | ||
| if _SAFE_TEXT in report_text or _SECRET_TEXT in report_text or str(root) in report_text: | ||
| raise RuntimeError("aggregate native route report contained request-derived content") |
There was a problem hiding this comment.
WARNING: JSON-escaped content leak check is ineffective
report_text is JSON with escaped newlines, but _SAFE_TEXT and _SECRET_TEXT contain raw newlines. The in checks will never match, so raw content leakage in aggregate reports goes undetected.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| observe_mode=config.mode == "observe", | ||
| observe_mode = self._observe_mode(guard_home=guard_home, workspace=workspace) | ||
| try: | ||
| response = review_post_tool_native(request, observe_mode=observe_mode) |
There was a problem hiding this comment.
WARNING: Native emergency rollback removed
native_mode() is no longer checked before calling review_post_tool_native. When HOL_GUARD_NATIVE=off, native_runtime_status() returns unavailable, so the worker now returns a deterministic fail-safe block instead of falling back to Python. This silently changes behavior for users who relied on the documented off emergency rollback.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if not isinstance(surface_id, str) or not surface_id.strip() or surface_id in seen: | ||
| raise ValueError(f"surface[{index}].id is missing or duplicated") | ||
| if raw.get("authority") not in VALID_AUTHORITY or raw.get("target_authority") not in VALID_AUTHORITY: | ||
| raise ValueError(f"surface {surface_id} has invalid authority") |
There was a problem hiding this comment.
SUGGESTION: Misleading error message for target_authority validation
The condition validates both authority and target_authority, but the error message only mentions "invalid authority". If only target_authority is invalid, the message is misleading.
| raise ValueError(f"surface {surface_id} has invalid authority") | |
| raise ValueError(f"surface {surface_id} has invalid authority or target_authority") |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous Review Summaries (3 snapshots, latest commit 4a6cdde)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 4a6cdde)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous review (commit ea94f26)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (14 files)
Fix these issues in Kilo Cloud Previous review (commit 4c41178)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (14 files)
Reviewed by free · Input: 46.1K · Output: 10.9K · Cached: 86.9K |
660c21b to
73247f9
Compare
Remove Python semantic fallback from the supported PostToolUse decision path, preserve observe mode without weakening enforcement, add privacy-safe route evidence, harden mixed target-path handling, and require every manifest-selected native authority gate on GitHub-hosted runners. Signed-off-by: Michael Kantor <6068672+kantorcodes@users.noreply.github.com>
73247f9 to
ea94f26
Compare
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| branches: | ||
| - rust/hotpath-authority-t001-t100 | ||
| paths: | ||
| - .github/workflows/finalize-pr-2598.yml |
There was a problem hiding this comment.
High: Ownership gate rejects this PR because finalize-pr-2598.yml is an unmapped hot-path file.
scripts/ci/rust_hotpath_ownership.py treats any changed path under .github/workflows/ as a hot path and fails the ownership job when it matches no [[surface]] glob. This PR adds .github/workflows/finalize-pr-2598.yml, which is not listed under migration-governance (or any other surface). Classifying the PR file set yields unknown_hot_paths=['.github/workflows/finalize-pr-2598.yml'] and exit code 1, so the authority workflow cannot pass on this branch. Map the finalizer under migration-governance, exclude it from HOT_PATH_PREFIXES, or remove it after in-tree fixes instead of leaving a self-rejecting workflow file.
| notice="warning", | ||
| reason_code="native_post_tool_unavailable", | ||
| policy_action="block", | ||
| ) |
There was a problem hiding this comment.
Medium: PostToolUse native fail-safe ignores observe mode and hard-blocks where the engine and daemon convert to allow.
When Guard config mode is observe and review_post_tool_native returns None or raises (HOL_GUARD_NATIVE=off, missing binary, overload, timeout, invalid response), HookWorker builds a deny/block HookReviewResponse with reason_code native_post_tool_unavailable and never applies the observe conversion that HookReviewEngine.review and server.runtime_hook_fail_safe_response still perform (allow with observed_policy_action/observe* reason). Observe-mode users therefore get a hard PostToolUse block on any native outage after this cutover, instead of allow-and-record. After building the fail-safe response, apply the same observe-only rewrite the engine uses when _observe_mode is true, or route fail-safe through that helper before harness mapping.
| fi | ||
| if [[ -z "$BASE" || "$BASE" == "0000000000000000000000000000000000000000" ]]; then | ||
| BASE="${{ github.sha }}^" | ||
| fi |
There was a problem hiding this comment.
Medium: Authority workflow still uses fragile BASE resolution; the hardening only exists in an unapplied finalizer patch.
rust-hotpath-authority.yml currently sets BASE to github.sha^ when before is empty or all-zeros, without ensuring HEAD/BASE commits are present in the shallow checkout. The finalizer would replace that with cat-file checks and a fetch of HEAD, but that edit is not in the tree—only in finalize-pr-2598.yml string surgery—and the finalizer itself is blocked by the ownership unmapped-path failure above. On workflow_dispatch or first-push edge cases where before is unavailable and parent history is missing, ownership classification can fail or classify the wrong range until the patch is committed in-tree. Put the BASE/HEAD reachability fix directly in rust-hotpath-authority.yml in this PR.
| runner_lines = runner_path.read_text(encoding="utf-8").splitlines() | ||
| runner_anchor = " adaptive_capacity.observe_load(queue_p95_ms=queue_p95_ms, queued=queued)" | ||
| runner_index = runner_lines.index(runner_anchor) | ||
| if runner_lines[runner_index + 1] != " self._refresh_capacity_policy()": |
There was a problem hiding this comment.
[WARNING]: Unchecked array access can crash the runner patch
runner_lines[runner_index + 1] is accessed without verifying runner_index + 1 < len(runner_lines). If the anchor is the last line in hook_process_runner.py, this raises IndexError and aborts the remediation workflow.
| if runner_lines[runner_index + 1] != " self._refresh_capacity_policy()": | |
| if runner_index + 1 >= len(runner_lines) or runner_lines[runner_index + 1] != " self._refresh_capacity_policy()": | |
| raise SystemExit("hook runner patch anchor changed") |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| with: | ||
| ref: rust/hotpath-authority-t001-t100 | ||
| fetch-depth: 0 | ||
| persist-credentials: true |
There was a problem hiding this comment.
[WARNING]: persist-credentials: true with auto-push exposes the GITHUB_TOKEN
The checkout step persists the GITHUB_TOKEN, and the workflow later pushes directly to the branch without human review. If the runner environment is compromised, the token can be exfiltrated and used to push arbitrary code.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
Unique-diff vs current Supported PreToolUse/PostToolUse fail-closed |
1 participant
Summary
Implements the T001-T100 Rust hot-path stability tranche for
release/3.0.PostToolUsepathHOL_GUARD_NATIVE=off, into a deterministic fail-safe block instead of changing decision enginesOwnership impact
PostToolUseis Rust-authoritative after this change. Python remains a bounded transport and harness-mapping layer and cannot substituteHookReviewEngine,ContentScanner, orHookDecisionCachewhen native evaluation fails.PreToolUseis explicitly staged for the immediately following delivery. The manifest carries one short, hard-expiring waiver through 2026-09-02. It does not allow Python fallback on a Rust-owned surface.There is no
strictmode. Rust is the product default.Validation
The authority workflow runs entirely on GitHub-hosted runners for pull requests and enforces every selected manifest job:
PostToolUseintegration through the production worker routePreToolUseThe all-harness proof reads measured aggregate counters from the production routing layer and requires zero Python decisions and zero native fail-safe outcomes. Evidence and reports reject both raw and JSON-escaped request content.
Targets
release/3.0only. Never merge intomain.