Conversation
`claude -p --output-format json` wraps the agent's reply in
`{"type":"result","result":"<stringified-findings-json>",...}`. 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.
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`.
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/`.
Bundles review-feedback polish for the envelope unwrap and cold-region priority paths: - Make `ClaudeCodeEnvelope.result` an `Option<String>` 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.
📝 WalkthroughWalkthroughThis PR introduces two new Go authorization rules for detecting ABAC patterns and OPA Rego evaluation sites, improves candidate file selection by prioritizing authorization-flavored paths, and adds Claude Code JSON envelope parsing support to the subprocess client for enhanced output format compatibility. ChangesAuthorization Rules: ABAC and OPA Rego Detection
Authorization Path Prioritization in Candidate Selection
Claude Code Envelope Parsing in Subprocess Client
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Review Summary
This PR successfully adds two important features:
-
Claude Code Transport Support: Adds native support for
claude -p --output-format jsonby unwrapping the transport envelope, eliminating the need forjqshell wrappers. -
Enhanced Go Authorization Coverage: Introduces two new structural rules (
go-opa-rego-evalandgo-access-descriptor-builder) plus filename-based prioritization for authz-relevant files.
Code Quality: The implementation is solid with comprehensive test coverage (325 unit + 30 integration tests), proper error handling, and follows Rust best practices. The defensive parsing approach for the Claude Code envelope is well-designed.
No Critical Issues Found: All changes function correctly and are ready to merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/deep/candidate.rs (1)
259-263: ⚡ Quick winNormalize paths to scan-root-relative before priority sorting.
At lines 260–262,
path_priorityand lexicographic comparison operate onfile.path(absolute). If the checkout directory name contains auth-like keywords (e.g.,/tmp/authz-token-abc/src/app.py), spurious prefix matches skew priority and make candidate selection dependent on the local directory structure. Sorting onscan_root-relative paths ensures deterministic, portable results.♻️ Proposed change
- discovered.sort_by(|a, b| { - path_priority(&b.path) - .cmp(&path_priority(&a.path)) - .then_with(|| a.path.cmp(&b.path)) - }); + discovered.sort_by(|a, b| { + let a_rel = a.path.strip_prefix(scan_root).unwrap_or(a.path.as_path()); + let b_rel = b.path.strip_prefix(scan_root).unwrap_or(b.path.as_path()); + path_priority(b_rel) + .cmp(&path_priority(a_rel)) + .then_with(|| a_rel.cmp(b_rel)) + });🤖 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/deep/candidate.rs` around lines 259 - 263, The sorting currently calls path_priority and lexicographic compare on absolute paths (discovered.sort_by closure), which can be skewed by checkout directory names; update the comparator to first compute scan-root-relative paths (use strip_prefix or a helper like make_relative(scan_root, &a.path) and same for b.path) and then call path_priority on those relative paths and compare them lexicographically; if computing the relative path fails, fall back to the original path to preserve behavior. Ensure you update the closure where discovered.sort_by is used and reference path_priority, a.path, b.path and the scan_root variable.
🤖 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.
Nitpick comments:
In `@src/deep/candidate.rs`:
- Around line 259-263: The sorting currently calls path_priority and
lexicographic compare on absolute paths (discovered.sort_by closure), which can
be skewed by checkout directory names; update the comparator to first compute
scan-root-relative paths (use strip_prefix or a helper like
make_relative(scan_root, &a.path) and same for b.path) and then call
path_priority on those relative paths and compare them lexicographically; if
computing the relative path fails, fall back to the original path to preserve
behavior. Ensure you update the closure where discovered.sort_by is used and
reference path_priority, a.path, b.path and the scan_root variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 434d54f3-fef3-4409-a1a5-80b4ced84f76
📒 Files selected for processing (5)
rules/go/access-descriptor-builder.tomlrules/go/opa-rego-eval.tomlsrc/deep/candidate.rssrc/deep/subprocess.rssrc/rules/embedded.rs
Summary
Two themes land together because they both came out of dogfooding zift's deep pass against ocp's
internal/authz/authz.go:--agent-cmddirectly atclaude -p --output-format jsonwithout ajq -r .resultshell wrapper.internal/authz/authz.go,permissions.py,rbac/check.go, …) survive tight--max-candidatescaps in the cold-region pass.Changes
Deep transport (
src/deep/subprocess.rs){"type":"result","result":"<stringified-json>",…}envelope emitted byclaude -p --output-format json. Inner payload is re-stripped of markdown fences before parsing.is_error: true, non-successsubtype, missing/nullresult) asBadResponseso the orchestrator skips that candidate cleanly instead of hard-failing.Option<String>on the innerresultfield — a futureresult: nullwon't slip through to a confusing parse error.Cold-region priority (
src/deep/candidate.rs)AUTHZ_PATH_REGEXrecognises authz-flavoured path tokens (authz,authn,rbac/abac/acl/iam,permissions?,roles?,polic(y|ies),guards?,access[_-]?control,authoriz/sation,authenticat*,authorit*).(priority desc, path asc)so authz-flavoured files always make the budget under tight caps; lex order remains the deterministic tiebreaker.Go rules (
rules/go/)go-opa-rego-eval: matchesrego.New(...)— covers both straightEvaland partial-eval flows. (Aliased imports intentionally not matched; documented in the rule.)go-access-descriptor-builder: matches the fluent ABAC builder shape (.WithPrincipal/Subject/Resource/Permission/Role/Tenant/Action). GenericWithName/WithIDexcluded to keep noise low.Test plan
cargo fmt --checkcargo clippy -- -D warningscargo test— 325 unit + 30 integration pass; new tests cover envelope unwrap (success /is_error/result: null/ missingresult/ unrelated objects), filename priority (positive + negative cases), and cold-region budget tiebreaks.zift scan --deep --agent-cmd "claude -p --output-format json" …against ocp and confirminternal/authz/authz.goproduces findings without ajqwrapper.Summary by CodeRabbit
New Features
Improvements