chore(review): reconcile protected main into #1629 - #2197
Conversation
…2194) Let a repository declare literal path prefixes in .github/edge-policy-artifact-paths.txt that hold research/data artifacts (SPSS .sav, serialized model objects, numeric arrays, ...) not shaped like documentation, and admit binary content there on the same evidence terms documentation paths already get. Security property: evaluate_pull_request now threads an optional base_ref and resolves the declaration only from that ref, never the pull-request head, so a PR cannot self-authorize admission of its own binary by adding or widening the declaration in the same diff. A new test proves the same-PR case is refused. .github/workflows/opencode- review.yml threads the pull_request_target event's already-available github.event.pull_request.base.sha with no new permissions. Suffix decision: a declared-prefix file whose suffix has no BINARY_DOCUMENT_MAGIC entry (most research formats) is admitted only on "no diff patch + fetched bytes are not valid UTF-8" evidence, so a file that decodes as valid UTF-8 is always still content-scanned. Runtime-named files (_runtime_path_rule) stay rejected inside a declared prefix exactly as inside docs/ today. Declaration parsing is bounded (64 entries, 8-segment depth) and rejects absolute paths, ".." traversal, and globs with a PolicyError naming the offending entry; a missing declaration file behaves identically to before this feature existed. pingora_edge_policy.py stays at 100% branch coverage and 100% interrogate docstring coverage. Refs #2193, #2149, #2116. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced 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 |
db3d648
into
fix/no-heuristic-review-admission-current-main
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| # (e.g. "main", "release/2026.09") or a commit SHA -- whatever the calling | ||
| # workflow already has on the pull_request event without new permissions. | ||
| # Bounded charset/length, no ".." traversal, and no leading/trailing "/". | ||
| BASE_REF_RE = re.compile(r"^[A-Za-z0-9](?:[A-Za-z0-9._/-]{0,253}[A-Za-z0-9])?$") |
There was a problem hiding this comment.
🟡 Valid base branch names fail
A legal branch ending in - or _ makes BASE_REF_RE reject evaluate_pull_request before evidence collection. The CLI promises branch-name support, so those repositories cannot use declared artifact paths.
Learn more
Git branch names allow characters and edge positions excluded by this regular expression. For example, release- and release_ pass git check-ref-format refs/heads/<name> but fail this validator. The current workflow passes a SHA, so the failure affects the newly documented branch-name API and direct CLI callers.
Example: Calling --base-ref release- fails with “Pull-request base ref is malformed” instead of loading the declaration from that valid branch.
Recommended fix: Either validate branch names against Git's ref-format constraints while preserving the existing length bound, or narrow the public contract and CLI help to immutable 40-character SHAs. Add tests for legal punctuation edge cases and illegal Git ref forms.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if isinstance(exc, HTTPError) and exc.code == 404: | ||
| raise ArtifactDeclarationNotFoundError( | ||
| f"GitHub API reported no resource for policy evidence at {url}" | ||
| ) from exc |
There was a problem hiding this comment.
🔍 Global 404 classification obscures evidence failures
_github_open_json labels every API 404 as a missing declaration, including changed-file and head-content requests. These still fail closed today, but the misleading exception contract can cause future callers to suppress unrelated evidence loss.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # The base branch's tip SHA at event time -- already-reviewed, | ||
| # already-merged state. Threaded through so the issue #2193 | ||
| # research/data artifact path declaration can be resolved only | ||
| # from here, never from the untrusted PR head; see | ||
| # `evaluate_pull_request`'s `base_ref` parameter. | ||
| PULL_REQUEST_BASE_SHA: ${{ github.event.pull_request.base.sha || '' }} |
There was a problem hiding this comment.
📝 Info: Base declarations stay snapshot-bound
The workflow passes the event’s immutable base SHA, not a moving branch name. Queued runs therefore evaluate the declaration from their original reviewed base snapshot.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if suffix not in BINARY_DOCUMENT_MAGIC: | ||
| try: | ||
| raw.decode("utf-8") | ||
| except UnicodeDecodeError: | ||
| return True | ||
| return False |
There was a problem hiding this comment.
🟥 Declared executables bypass runtime policy
A declared script with invalid UTF-8 makes _binary_documentation_evidence_confirms admit the entire file. Its valid commands still execute, so prohibited Nginx helpers bypass the required gate.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Ordinary non-force reconciliation of protected
main@91be6442906c7b6b4f600272c953699708394327into canonical review-sidecar ownerfix/no-heuristic-review-admission-current-main. This carries the protected Pingora/OpenCode artifact-path change while preserving #1629's review-admission contract. No predecessor check/review evidence transfers; #1629 remains Draft and must reacquire exact-head acceptance.