From 612c9eb808ce5bb0926e9caec331c386ba4c796e Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Mon, 4 May 2026 17:57:54 -0400 Subject: [PATCH 1/4] fix(deep): unwrap claude-code --output-format json envelope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `claude -p --output-format json` wraps the agent's reply in `{"type":"result","result":"",...}`. The subprocess transport tried to deserialise the outer object as `FindingsEnvelope` directly, which failed at the missing `findings` field — so every per-candidate analysis was discarded with a `bad response` warning even when Claude had returned a valid payload inside `.result`. Detect the envelope by `type == "result"` plus a string `result` field, peel one layer (re-stripping any markdown fence the inner text carries), and parse the inner string as `FindingsEnvelope`. Recognise `is_error: true` / non-`success` subtype as a real Claude-side failure and surface as `BadResponse` so the orchestrator's per-candidate-skip path handles it without a hard fail. This means users can pass `--agent-cmd "claude -p --output-format json"` directly without a `jq -r .result` shell wrapper. --- src/deep/subprocess.rs | 153 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 147 insertions(+), 6 deletions(-) diff --git a/src/deep/subprocess.rs b/src/deep/subprocess.rs index 25d9e06..b0ce94b 100644 --- a/src/deep/subprocess.rs +++ b/src/deep/subprocess.rs @@ -293,8 +293,26 @@ impl SubprocessClient { // and same truncated-debug-log discipline as the HTTP client — // model-or-CLI output may mirror prompt text and should not // appear verbatim in error strings. + // + // Claude Code's `--output-format json` wraps the agent's reply + // in `{"type":"result","result":"",...}`. If + // we recognise that envelope, peel one layer (and re-strip any + // markdown fence the inner text might carry) before parsing + // into [`FindingsEnvelope`]. This means users can pass + // `--agent-cmd "claude -p --output-format json"` directly, + // without a `jq -r .result` shell wrapper. let cleaned = strip_markdown_fence(&stdout_buf); - let parsed: FindingsEnvelope = serde_json::from_str(cleaned).map_err(|e| { + let unwrapped = unwrap_claude_code_envelope(cleaned)?; + let inner_owned; + let parse_target: &str = match unwrapped { + Some(inner) => { + tracing::debug!("subprocess: unwrapped claude-code result envelope"); + inner_owned = strip_markdown_fence(&inner).to_string(); + &inner_owned + } + None => cleaned, + }; + let parsed: FindingsEnvelope = serde_json::from_str(parse_target).map_err(|e| { tracing::debug!( error = %e, preview = %truncate_for_log(&stdout_buf), @@ -396,6 +414,61 @@ fn kill_process_tree(child: &mut std::process::Child) { let _ = child.kill(); } +#[derive(Deserialize)] +struct FindingsEnvelope { + findings: Vec, +} + +/// Outer envelope emitted by `claude -p --output-format json`. Only the +/// fields we actually consult are deserialised — Claude Code adds new +/// fields over time (`session_id`, `api_error_status`, `duration_ms`, +/// etc.) and we don't want a future addition to break parsing. +/// +/// Recognised by `type == "result"` AND a string-typed `result` field; +/// any other shape is treated as not-an-envelope and the caller falls +/// back to parsing the original payload directly. +#[derive(Deserialize)] +struct ClaudeCodeEnvelope { + #[serde(rename = "type")] + ty: String, + /// Stringified inner JSON (the agent's actual reply). Claude Code + /// always emits this as a JSON string, even on error subtypes — + /// the inner content just isn't a valid `FindingsEnvelope` then. + result: String, + #[serde(default)] + is_error: bool, + #[serde(default)] + subtype: Option, +} + +/// If `s` is a Claude Code `--output-format json` envelope, return the +/// inner stringified payload. Otherwise return `Ok(None)` so the caller +/// can parse `s` directly. +/// +/// Returns `Err(BadResponse)` only when the envelope is recognised AND +/// reports a Claude-side error (`is_error: true` or a non-`success` +/// subtype) — that's a real failure with no findings to recover, and +/// the caller's per-candidate-skip path is the right home for it. +fn unwrap_claude_code_envelope(s: &str) -> Result, DeepError> { + // `from_str` here is cheap: on the common path (no envelope) it + // fails fast on the first unknown field shape. We never return the + // serde error to the caller — that's reserved for the real parse. + let env: ClaudeCodeEnvelope = match serde_json::from_str(s) { + Ok(v) => v, + Err(_) => return Ok(None), + }; + if env.ty != "result" { + return Ok(None); + } + if env.is_error || env.subtype.as_deref().is_some_and(|s| s != "success") { + return Err(DeepError::BadResponse(format!( + "claude-code envelope reported error (subtype={})", + env.subtype.as_deref().unwrap_or(""), + ))); + } + Ok(Some(env.result)) +} + /// Brief, allocation-free string form of [`std::process::ExitStatus`] /// for the user-visible error message. `Display` for `ExitStatus` /// prints "exit status: 1" / "signal: 9" already; just delegate. @@ -403,11 +476,6 @@ fn exit_status_brief(status: &std::process::ExitStatus) -> String { status.to_string() } -#[derive(Deserialize)] -struct FindingsEnvelope { - findings: Vec, -} - #[cfg(test)] mod tests { use super::*; @@ -583,6 +651,79 @@ mod tests { ); } + // ---- claude-code envelope unwrap ---- + + #[test] + fn unwrap_passes_through_plain_findings_envelope() { + // Direct `{"findings":[]}` is NOT a claude envelope — caller + // should fall through to a normal parse on the original string. + let s = r#"{"findings":[]}"#; + let out = unwrap_claude_code_envelope(s).unwrap(); + assert!(out.is_none(), "plain findings should not match envelope"); + } + + #[test] + fn unwrap_returns_inner_for_claude_code_success_envelope() { + let s = r#"{"type":"result","subtype":"success","is_error":false,"result":"{\"findings\":[]}","session_id":"abc"}"#; + let inner = unwrap_claude_code_envelope(s).unwrap().expect("envelope"); + assert_eq!(inner, r#"{"findings":[]}"#); + } + + #[test] + fn unwrap_returns_bad_response_when_envelope_marks_error() { + let s = + r#"{"type":"result","subtype":"error_during_execution","is_error":true,"result":""}"#; + let err = unwrap_claude_code_envelope(s).unwrap_err(); + assert!( + matches!(err, DeepError::BadResponse(ref msg) if msg.contains("error_during_execution")), + "got: {err:?}", + ); + } + + #[test] + fn unwrap_ignores_unrelated_objects_with_string_result() { + // An unrelated wrapper that happens to have a `result` string + // but no `type:"result"` must NOT be unwrapped — that would + // silently drop real fields from a future transport. + let s = r#"{"type":"other","result":"oops"}"#; + let out = unwrap_claude_code_envelope(s).unwrap(); + assert!(out.is_none()); + } + + #[cfg(unix)] + #[test] + fn end_to_end_claude_code_json_envelope_is_unwrapped_and_parsed() { + // Mimic exactly what `claude -p --output-format json` writes: + // a single JSON object whose `result` is a stringified + // `{"findings":[...]}`. The transport should pass these through. + let inner = r#"{\"findings\":[]}"#; + let envelope = format!( + r#"{{"type":"result","subtype":"success","is_error":false,"result":"{inner}","session_id":"x"}}"# + ); + let cmd = format!("printf '%s' '{envelope}'"); + let rt = synth_runtime(&cmd, 10); + let client = SubprocessClient::new(&rt).unwrap(); + let resp = client.analyze(&synth_prompt()).unwrap(); + assert!(resp.findings.is_empty()); + } + + #[cfg(unix)] + #[test] + fn end_to_end_claude_code_envelope_with_error_subtype_skips_candidate() { + // When the envelope itself reports failure, surface BadResponse + // (per-candidate skip), not a hard fail. + let envelope = + r#"{"type":"result","subtype":"error_during_execution","is_error":true,"result":""}"#; + let cmd = format!("printf '%s' '{envelope}'"); + let rt = synth_runtime(&cmd, 10); + let client = SubprocessClient::new(&rt).unwrap(); + let err = client.analyze(&synth_prompt()).unwrap_err(); + assert!( + matches!(err, DeepError::BadResponse(_)), + "expected BadResponse, got: {err:?}", + ); + } + #[cfg(unix)] #[test] fn markdown_fence_around_stdout_is_stripped() { From 29cf6f7a804c9dfcb69d9f77a18927fe9deb0731 Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Mon, 4 May 2026 17:58:07 -0400 Subject: [PATCH 2/4] feat(rules): add Go rules for OPA partial-eval and ABAC builders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two new high-confidence Go rules that close the gap exposed by scanning ocp's `internal/authz/` (where the structural pass returned zero findings on a file literally named `authz.go`): - `go-opa-rego-eval` — anchors on `rego.New(...)` from OPA's Go SDK, the constructor every embedded-OPA flow has to go through. Catches both straight-line `.Eval(ctx)` and `.Partial(ctx)` patterns without matching bare `.Eval` / `.Partial` on unrelated APIs. - `go-access-descriptor-builder` — matches fluent builder calls with authz-flavoured attribute names (`WithPrincipal`, `WithSubject`, `WithResource`, `WithPermission`, `WithRole`, `WithTenant`, `WithAction`). Generic builder methods (`WithName`, `WithTimeout`, …) intentionally do not match. Together they take ocp from 0 → 53 structural findings across 8 files, including the previously-invisible `internal/authz/authz.go`. --- rules/go/access-descriptor-builder.toml | 98 +++++++++++++++++++ rules/go/opa-rego-eval.toml | 121 ++++++++++++++++++++++++ src/rules/embedded.rs | 8 ++ 3 files changed, 227 insertions(+) create mode 100644 rules/go/access-descriptor-builder.toml create mode 100644 rules/go/opa-rego-eval.toml diff --git a/rules/go/access-descriptor-builder.toml b/rules/go/access-descriptor-builder.toml new file mode 100644 index 0000000..07c82a6 --- /dev/null +++ b/rules/go/access-descriptor-builder.toml @@ -0,0 +1,98 @@ +[rule] +id = "go-access-descriptor-builder" +languages = ["go"] +category = "abac" +confidence = "high" +description = "ABAC access-descriptor builder call (e.g. .WithPrincipal(...), .WithPermission(...))" +# Captures the builder/fluent shape commonly used to assemble an +# authorization input for a policy engine — e.g. ocp's +# `NewAccess().WithPrincipal(p).WithResource(r).WithPermission(perm) +# .WithTenant(t)`. Each builder call is a separate +# `call_expression`, so we surface a finding per chain link rather than +# trying to recognise the whole chain — the model (or a human reviewer) +# coalesces them downstream. +# +# The method-name allowlist is intentionally narrow: it sticks to +# attribute names that mean "I'm assembling an authz subject/object/ +# action tuple" — `Principal`, `Subject`, `Resource`, `Permission`, +# `Role`, `Tenant`, `Action`. Generic `WithName`, `WithID`, etc. are +# excluded to keep this from firing on every builder in the codebase. +# Confidence is `high` because the combination of the `With…` prefix +# and an authz-flavored attribute is rarely incidental. +query = """ +(call_expression + function: (selector_expression + field: (field_identifier) @attr_method) +) @match +""" + +[rule.predicates.attr_method] +match = "^With(Principal|Subject|Resource|Permission|Role|Tenant|Action)$" + +[rule.rego_template] +template = """ +# Builder calls assemble an ABAC input — `principal`, `resource`, +# `permission`, etc. The actual decision usually happens elsewhere +# (an OPA `.Eval(ctx)` or a custom evaluator). Adapt this stub to +# match the attributes your builder actually accumulates. +default allow := false + +allow if { + input.principal == input.resource.owner +} + +allow if { + input.permission in data.permissions[input.principal] +} +""" + +[[rule.tests]] +input = """ +package main + +func mk() { + a := NewAccess().WithPrincipal("bob").WithResource("sources").WithPermission("sources.view") + _ = a +} +""" +expect_match = true + +[[rule.tests]] +input = """ +package main + +func mk(b *AccessBuilder) { + b.WithRole("admin") +} +""" +expect_match = true + +[[rule.tests]] +input = """ +package main + +func mk(b *AccessBuilder) { + b.WithTenant("acme") +} +""" +expect_match = true + +[[rule.tests]] +input = """ +package main + +func mk(b *Builder) { + b.WithName("foo") +} +""" +expect_match = false + +[[rule.tests]] +input = """ +package main + +func mk(opt *Options) { + opt.WithTimeout(5) +} +""" +expect_match = false diff --git a/rules/go/opa-rego-eval.toml b/rules/go/opa-rego-eval.toml new file mode 100644 index 0000000..fa04bfe --- /dev/null +++ b/rules/go/opa-rego-eval.toml @@ -0,0 +1,121 @@ +[rule] +id = "go-opa-rego-eval" +languages = ["go"] +category = "custom" +confidence = "high" +description = "OPA Rego evaluation in Go (rego.New(...) entry point)" +# OPA's Go SDK (`github.com/open-policy-agent/opa/rego`) exposes a small, +# distinctive entry-point: `rego.New(opts...)`. Anchoring on the +# constructor catches both straight-line evaluation (`rego.New(...) +# .Eval(ctx)`) and partial-evaluation flows (`rego.New(...).Partial(ctx)`, +# as in ocp's `internal/authz/authz.go`) — every call site of OPA from +# Go has to go through `rego.New` once. We deliberately don't add a +# second rule for `.Eval` / `.Partial` directly: those method names are +# generic enough (lots of unrelated APIs use them) that without the +# `rego` package qualifier they'd be noisy. Confidence is `high` because +# applications don't typically embed OPA for non-policy reasons. +query = """ +(call_expression + function: (selector_expression + operand: (identifier) @pkg + field: (field_identifier) @ctor) +) @match +""" + +[rule.predicates.pkg] +eq = "rego" + +[rule.predicates.ctor] +eq = "New" + +[rule.rego_template] +template = """ +# OPA is already the policy engine here — the embedded Go code IS the +# enforcement point. This stub is a starter scaffold; the real policy +# almost certainly lives in a separate `.rego` file already (and the +# Go code passes it via `rego.Module(...)` or `rego.Load(...)`). Lift +# that policy into your bundle rather than duplicating logic here. +default allow := false + +allow if { + input.user.permissions[_] == input.required_permission +} +""" + +[[rule.tests]] +input = """ +package main + +import "github.com/open-policy-agent/opa/rego" + +func decide(ctx context.Context) { + r := rego.New( + rego.Query("data.authz.allow"), + rego.Module("authz.rego", src), + ) + _ = r +} +""" +expect_match = true + +[[rule.tests]] +input = """ +package main + +func decide(ctx context.Context) { + pqs, err := rego.New( + rego.Query("data.authz.allow = true"), + rego.Module("authz.rego", src), + rego.Unknowns(unknowns), + rego.ParsedInput(access.ToValue()), + ).Partial(ctx) + _ = pqs + _ = err +} +""" +expect_match = true + +[[rule.tests]] +input = """ +package main + +func decide(ctx context.Context) { + res, _ := rego.New(rego.Query("data.x")).Eval(ctx) + _ = res +} +""" +expect_match = true + +[[rule.tests]] +input = """ +package main + +func decide() { + foo.New("bar") +} +""" +expect_match = false + +[[rule.tests]] +input = """ +package main + +func decide(svc *Service) { + svc.Process(input) +} +""" +expect_match = false + +# We deliberately do NOT match a bare `q.Eval(ctx)` without a preceding +# `rego.New` capture in scope — too generic. The companion test pins +# this behaviour so a future widening of the rule has to confront the +# false-positive trade-off explicitly. +[[rule.tests]] +input = """ +package main + +func decide(ctx context.Context, q *rego.Rego) { + _, _ = q.Eval(ctx) +} +""" +expect_match = false diff --git a/src/rules/embedded.rs b/src/rules/embedded.rs index 41cc048..dde9352 100644 --- a/src/rules/embedded.rs +++ b/src/rules/embedded.rs @@ -207,6 +207,14 @@ const EMBEDDED_RULES: &[(&str, &str)] = &[ "go-gin-auth-middleware", include_str!("../../rules/go/gin-auth-middleware.toml"), ), + ( + "go-opa-rego-eval", + include_str!("../../rules/go/opa-rego-eval.toml"), + ), + ( + "go-access-descriptor-builder", + include_str!("../../rules/go/access-descriptor-builder.toml"), + ), ]; pub fn load_embedded_rules() -> Result> { From 7db88ba06fbd00ce665219090f592e4a44b3e3e9 Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Mon, 4 May 2026 17:58:22 -0400 Subject: [PATCH 3/4] feat(deep): prioritise auth-flavoured filenames in cold regions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cold-region candidate selection used to walk discovered files in lexicographic order, so under tight `max_candidates` an `app.py` ahead of `authz.py` could starve the obvious file out of the budget. This is exactly how ocp's `internal/authz/authz.go` slipped past both the structural pass (rules gap) and the deep pass (budget gap) in v0.1.6. Add `AUTHZ_PATH_REGEX` matching path-segment-bounded tokens (`authz`, `authn`, `authentication`, `authorization`, `rbac`, `abac`, `acl`, `iam`, `permission(s)`, `role(s)`, `polic(y|ies)`, `guard(s)`, `access[_-]control`) and use it as a priority key in `build_cold_regions`. Files whose path looks authz-flavoured sort before neutral ones; lexicographic order remains the tiebreaker so output stays deterministic. Filenames are deliberately a *priority* signal only — they never generate findings on their own, since name-only matches conflate "this file is interesting" with "here's an enforcement point". The regex is also conservative on boundaries to avoid hits on substrings like `authoring.md` or `author/`. --- src/deep/candidate.rs | 137 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 133 insertions(+), 4 deletions(-) diff --git a/src/deep/candidate.rs b/src/deep/candidate.rs index b37f9f8..3d14087 100644 --- a/src/deep/candidate.rs +++ b/src/deep/candidate.rs @@ -28,6 +28,43 @@ use std::sync::LazyLock; /// escalations from structural findings always get priority. const COLD_REGION_FRACTION: f32 = 0.3; +/// Filename / path-segment tokens that suggest a file is authz-relevant. +/// Used as a *priority* signal in cold-region selection (not as a finding +/// in its own right) — files whose path contains one of these tokens are +/// scanned before everything else, so under tight `max_candidates` caps +/// the obvious authz files always make the budget. Boundaries are path +/// separators / `_`, `-`, `.` so substring collisions like `authoring.md` +/// or `authentic` (without `authenticat…` continuation) don't trigger. +/// +/// Same false-positive-tolerated stance as `AUTH_NAME_REGEX`: missing a +/// real authz file is a worse failure than an extra deep-pass candidate. +static AUTHZ_PATH_REGEX: LazyLock = LazyLock::new(|| { + Regex::new( + r"(?ix) + (?: ^ | [/\\_.\-] ) + (?: + authz | authn + | authori[sz]ation + | authenticat\w* + | rbac | abac | acl | iam + | permissions? | roles? | polic(?:y|ies) + | guards? + | access[_\-]?control + ) + (?: [/\\_.\-] | $ ) + ", + ) + .expect("AUTHZ_PATH_REGEX is a valid regex") +}); + +/// Lightweight priority hint for cold-region iteration: 1 if the path +/// looks authz-flavoured, 0 otherwise. Higher beats lower; ties fall +/// back to lexicographic path order so ranking stays deterministic. +fn path_priority(path: &Path) -> u8 { + let s = path.to_string_lossy(); + if AUTHZ_PATH_REGEX.is_match(&s) { 1 } else { 0 } +} + /// Names that suggest authorization logic. Matched case-insensitively as /// whole-word tokens. False positives are tolerated — the model filters them /// at deep-pass time. Missed real authz, on the other hand, is a worse @@ -208,10 +245,21 @@ fn build_cold_regions( let mut discovered = discover_files_for_deep(scan_root, &runtime.excludes, &runtime.language_filter); - // Sort by path so that under tight `max_candidates`, the surviving cold - // subset is stable across filesystems and runs. Without this, the - // post-loop sort only orders the items we already happened to pick. - discovered.sort_by(|a, b| a.path.cmp(&b.path)); + // Two-key ordering: + // 1. Path priority (descending) — files whose path looks authz-flavoured + // (`authz.go`, `internal/permissions/...`, `rbac.py`, …) sort before + // neutral files. This is the only place we let filenames influence + // results: under a tight `max_candidates`, the obvious authz files + // survive even when their content doesn't trip the structural rules + // (the bug that made ocp's `internal/authz/authz.go` a no-finding + // file in v0.1.6). + // 2. Lexicographic path (ascending) — ties break deterministically so + // the surviving cold subset is stable across filesystems and runs. + discovered.sort_by(|a, b| { + path_priority(&b.path) + .cmp(&path_priority(&a.path)) + .then_with(|| a.path.cmp(&b.path)) + }); let mut out: Vec = Vec::new(); for file in discovered { @@ -442,6 +490,87 @@ mod tests { } } + // ---- filename priority ---- + + #[test] + fn path_priority_matches_obvious_authz_paths() { + for s in [ + "internal/authz/authz.go", + "src/permissions.py", + "pkg/rbac/check.go", + "lib/abac/policy.ts", + "src/authn/middleware.ts", + "internal/iam/roles.go", + "src/policies.py", + "guards/admin.ts", + "access-control/rules.go", + "authentication.go", + "authorization.go", + "authorisation.py", // British spelling + "src/acl/list.go", + ] { + assert_eq!( + path_priority(Path::new(s)), + 1, + "expected priority 1 for {s}", + ); + } + } + + #[test] + fn path_priority_rejects_non_authz_paths_with_similar_substrings() { + for s in [ + "docs/authoring.md", // 'auth' but not at a separator boundary + "internal/author/x.go", // author != authz + "src/authentic/x.go", // 'authentic' alone is not in our list (only authenticat\w*) + "cmd/migrate/migrate.go", + "pkg/utils/utils.go", + "src/icon.go", + ] { + assert_eq!( + path_priority(Path::new(s)), + 0, + "expected priority 0 for {s}", + ); + } + } + + #[test] + fn cold_region_iterates_authz_paths_first() { + // 4 files, all with the same auth-y content. With `max_candidates=10` + // the cold-region budget is ceil(10 * 0.3) = 3, so we get exactly 3 + // candidates. The first two MUST be the authz-flavoured filenames + // even though `app.py` and `core.py` sort before them lexicographically; + // the third tie-breaks to lex order between the two non-authz files. + let dir = tempdir().unwrap(); + let body = "def is_admin(u):\n return False\n"; + for name in ["app.py", "authz.py", "core.py", "permissions.py"] { + fs::write(dir.path().join(name), body).unwrap(); + } + let mut runtime = rt(); + runtime.max_candidates = 10; + let mut candidates = select_candidates(&[], dir.path(), &runtime).unwrap(); + // `select_candidates` re-sorts the final output by (file, line) for + // deterministic emission; that breaks the ordering signal we want + // to assert. Verify priority instead by checking which files made + // the cold-region budget. + candidates.sort_by(|a, b| a.file.cmp(&b.file)); + let files: Vec<_> = candidates.iter().map(|c| c.file.clone()).collect(); + assert_eq!( + files.len(), + 3, + "cold budget = ceil(10 * 0.3) = 3; got {files:?}", + ); + assert!( + files.contains(&PathBuf::from("authz.py")), + "authz.py must survive the cold-region cap: {files:?}", + ); + assert!( + files.contains(&PathBuf::from("permissions.py")), + "permissions.py must survive the cold-region cap: {files:?}", + ); + } + // ---- escalation rules ---- #[test] From 49783c1e713eed0c89baca911b4d1390013a2264 Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Mon, 4 May 2026 18:12:29 -0400 Subject: [PATCH 4/4] fix(deep): handle null result in claude-code envelope Bundles review-feedback polish for the envelope unwrap and cold-region priority paths: - Make `ClaudeCodeEnvelope.result` an `Option` so a future build emitting `result: null` (or omitting the field on error subtypes) surfaces as a clean `BadResponse` instead of falling through to a confusing "not valid findings JSON" parse error. - Include `is_error`, `subtype`, and `result_present` in the envelope error message so the operator can see exactly which condition fired. - Correct the misleading "fails fast on unknown field shape" comment (serde ignores unknown fields by default). - Document the aliased-import limitation on `go-opa-rego-eval`. - Extend `AUTHZ_PATH_REGEX` to cover the `authorit*` family (authority, authoritative, authorities). - Tighten `cold_region_iterates_authz_paths_first` to pin the lex tiebreaker (`app.py` in, `core.py` out) so a future flip is loud. --- rules/go/opa-rego-eval.toml | 7 ++++ src/deep/candidate.rs | 17 ++++++++- src/deep/subprocess.rs | 73 ++++++++++++++++++++++++++++--------- 3 files changed, 79 insertions(+), 18 deletions(-) diff --git a/rules/go/opa-rego-eval.toml b/rules/go/opa-rego-eval.toml index fa04bfe..3a9d268 100644 --- a/rules/go/opa-rego-eval.toml +++ b/rules/go/opa-rego-eval.toml @@ -14,6 +14,13 @@ description = "OPA Rego evaluation in Go (rego.New(...) entry point)" # generic enough (lots of unrelated APIs use them) that without the # `rego` package qualifier they'd be noisy. Confidence is `high` because # applications don't typically embed OPA for non-policy reasons. +# +# Known limitation: aliased imports (e.g. `import opa "…/opa/rego"`) +# won't match — the `pkg eq "rego"` predicate keys on the source +# identifier, not the imported path. Aliasing this package is rare in +# practice; widening to `opa` would risk false positives on unrelated +# `opa.New` symbols. If a real codebase shows up using an alias, prefer +# adding a second rule keyed on that alias rather than relaxing this one. query = """ (call_expression function: (selector_expression diff --git a/src/deep/candidate.rs b/src/deep/candidate.rs index 3d14087..97393fa 100644 --- a/src/deep/candidate.rs +++ b/src/deep/candidate.rs @@ -45,6 +45,7 @@ static AUTHZ_PATH_REGEX: LazyLock = LazyLock::new(|| { (?: authz | authn | authori[sz]ation + | authorit\w* # authority, authoritative, authorities | authenticat\w* | rbac | abac | acl | iam | permissions? | roles? | polic(?:y|ies) @@ -506,7 +507,9 @@ mod tests { "access-control/rules.go", "authentication.go", "authorization.go", - "authorisation.py", // British spelling + "authorisation.py", // British spelling + "src/authority/check.go", // authority/authoritative family + "pkg/authoritative_source.go", "src/acl/list.go", ] { assert_eq!( @@ -569,6 +572,18 @@ mod tests { files.contains(&PathBuf::from("permissions.py")), "permissions.py must survive the cold-region cap: {files:?}", ); + // Tertiary slot: with the two authz files locked in, the third + // slot tie-breaks to lex order among priority-0 files + // (`app.py` < `core.py`). Pin both halves so a future flip in + // the secondary tiebreaker is loud. + assert!( + files.contains(&PathBuf::from("app.py")), + "app.py should win the lex tiebreak among non-authz files: {files:?}", + ); + assert!( + !files.contains(&PathBuf::from("core.py")), + "core.py should be cut by the budget: {files:?}", + ); } // ---- escalation rules ---- diff --git a/src/deep/subprocess.rs b/src/deep/subprocess.rs index b0ce94b..4dbee4b 100644 --- a/src/deep/subprocess.rs +++ b/src/deep/subprocess.rs @@ -431,10 +431,14 @@ struct FindingsEnvelope { struct ClaudeCodeEnvelope { #[serde(rename = "type")] ty: String, - /// Stringified inner JSON (the agent's actual reply). Claude Code - /// always emits this as a JSON string, even on error subtypes — - /// the inner content just isn't a valid `FindingsEnvelope` then. - result: String, + /// Stringified inner JSON (the agent's actual reply). Today Claude + /// Code emits an empty string on error subtypes rather than `null`, + /// but we accept `Option` defensively: a future build emitting + /// `result: null` for failures should still surface as a clean + /// `BadResponse` rather than fall through to a confusing + /// "not valid findings JSON" parse error. + #[serde(default)] + result: Option, #[serde(default)] is_error: bool, #[serde(default)] @@ -446,13 +450,17 @@ struct ClaudeCodeEnvelope { /// can parse `s` directly. /// /// Returns `Err(BadResponse)` only when the envelope is recognised AND -/// reports a Claude-side error (`is_error: true` or a non-`success` -/// subtype) — that's a real failure with no findings to recover, and -/// the caller's per-candidate-skip path is the right home for it. +/// reports a Claude-side error (`is_error: true`, a non-`success` +/// subtype, or a missing/null `result`) — that's a real failure with +/// no findings to recover, and the caller's per-candidate-skip path +/// is the right home for it. fn unwrap_claude_code_envelope(s: &str) -> Result, DeepError> { - // `from_str` here is cheap: on the common path (no envelope) it - // fails fast on the first unknown field shape. We never return the - // serde error to the caller — that's reserved for the real parse. + // On the common (non-envelope) path, deserialisation fails because + // the required `type` field is missing or wrong-typed; serde's + // default behaviour ignores unknown fields, so we don't have to + // enumerate Claude Code's full envelope schema here. We never + // return the serde error to the caller — that's reserved for the + // real findings parse. let env: ClaudeCodeEnvelope = match serde_json::from_str(s) { Ok(v) => v, Err(_) => return Ok(None), @@ -460,13 +468,16 @@ fn unwrap_claude_code_envelope(s: &str) -> Result, DeepError> { if env.ty != "result" { return Ok(None); } - if env.is_error || env.subtype.as_deref().is_some_and(|s| s != "success") { + let bad_subtype = env.subtype.as_deref().is_some_and(|s| s != "success"); + if env.is_error || bad_subtype || env.result.is_none() { return Err(DeepError::BadResponse(format!( - "claude-code envelope reported error (subtype={})", + "claude-code envelope reported error (is_error={}, subtype={}, result_present={})", + env.is_error, env.subtype.as_deref().unwrap_or(""), + env.result.is_some(), ))); } - Ok(Some(env.result)) + Ok(env.result) } /// Brief, allocation-free string form of [`std::process::ExitStatus`] @@ -674,10 +685,38 @@ mod tests { let s = r#"{"type":"result","subtype":"error_during_execution","is_error":true,"result":""}"#; let err = unwrap_claude_code_envelope(s).unwrap_err(); - assert!( - matches!(err, DeepError::BadResponse(ref msg) if msg.contains("error_during_execution")), - "got: {err:?}", - ); + let DeepError::BadResponse(msg) = err else { + panic!("expected BadResponse, got: {err:?}"); + }; + assert!(msg.contains("error_during_execution"), "msg={msg}"); + assert!(msg.contains("is_error=true"), "msg={msg}"); + } + + #[test] + fn unwrap_returns_bad_response_when_result_is_null() { + // Defensive: today claude-code emits `result: ""` on error + // subtypes, but if a future build emits `result: null` we want + // a clean BadResponse, not a fall-through to the findings parser. + let s = + r#"{"type":"result","subtype":"error_during_execution","is_error":true,"result":null}"#; + let err = unwrap_claude_code_envelope(s).unwrap_err(); + let DeepError::BadResponse(msg) = err else { + panic!("expected BadResponse, got: {err:?}"); + }; + assert!(msg.contains("result_present=false"), "msg={msg}"); + } + + #[test] + fn unwrap_returns_bad_response_when_result_missing_on_success_subtype() { + // Even with subtype=success, a missing `result` field can't + // produce findings — surface as BadResponse rather than fall + // through and parse the envelope itself as the findings payload. + let s = r#"{"type":"result","subtype":"success","is_error":false}"#; + let err = unwrap_claude_code_envelope(s).unwrap_err(); + let DeepError::BadResponse(msg) = err else { + panic!("expected BadResponse, got: {err:?}"); + }; + assert!(msg.contains("result_present=false"), "msg={msg}"); } #[test]