feat: platform abstraction seam — github backend, zero behavior change (#221) - #236
Conversation
#221) Every host-forge API call now routes through scripts/platform_api.sh (platform_* functions) or pr_reviewer/platform.py. The github backend reproduces the exact pre-seam gh invocations — the existing test suites, which stub gh via PATH, pass unmodified apart from two static source-grep assertions mechanically pointed at the seam file. - New `platform` input: auto (default, detects from GITHUB_SERVER_URL), github, forgejo. PLATFORM is wired into all six script-running steps. - Forgejo mode delegates implemented operations (core PR I/O from #222) to pr_reviewer/forgejo_backend.py and fails loudly on everything else (reviews/#224, CI status/#225, graphql, collaborator permission) instead of silently misbehaving; missing FORGEJO_API_URL is a hard error. - Linked-source enrichment calls are deliberately a separate class (github_enrich_api): they target github.com-hosted upstreams regardless of the host platform. #227 gates their degradation. - resolve_finding_threads.py builds gh argv via the Python seam; GraphQL thread management degrades to its existing best-effort no-op on forgejo. - The #190 stdout-error-body discipline is preserved: wrappers are thin execs, never capture-and-echo; call-site rc-check semantics unchanged. Tests: 32-case seam suite asserts every github-backend command line byte-for-byte plus resolution logic and loud forgejo failures; pytest suite covers the Python mirror. Full regression: all bash suites + 660 python tests pass. Refs #221 (Forgejo umbrella #229). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic) — routed smart (risk match: path_handling_changes)
Review Summary
Recommendation: Approve.
This PR implements the platform abstraction seam (issue PR 221) — a clean architectural refactor that routes all host-forge API interactions through platform_* functions in scripts/platform_api.sh and pr_reviewer/platform.py. The github backend reproduces the exact pre-seam gh invocations byte-for-byte, ensuring zero behavior change. The Forgejo backend stubs fail loudly with operation names rather than silently misbehaving, which is the correct design for an in-progress migration.
Change-by-Change Findings
New platform seam files
scripts/platform_api.sh — The central seam file (~260 lines). All ~40 call sites across the action's scripts now route through these wrapper functions. The #190 pitfall (gh printing JSON error bodies to stdout on HTTP errors) is explicitly documented and avoided throughout: wrappers are thin execs that never capture-and-echo, preserving existing rc-check/stdout-discard semantics at every call site.
pr_reviewer/platform.py — Python mirror for resolve_platform() and gh_argv(). Exposes PlatformUnsupported for forgejo mode on consumers not yet migrated.
Call site migrations
action.yml, scripts/check_review_needed.sh, scripts/parse_review_command.sh, scripts/publish_helpers.sh, scripts/resolve_finding_threads.py, scripts/run_review.sh, scripts/wait_for_ci.sh — each updated to source platform_api.sh and replace bare gh invocations with platform-wrapped equivalents. The migration is consistent and complete.
Test coverage
tests/test_platform_api.sh — 32 test cases asserting byte-for-byte argv equality on the github backend, forgejo mode loud-failure behavior, auto-resolution logic, and case-insensitivity. This is the right regression harness.
tests/test_platform.py — Python mirror coverage (6 cases).
Updated existing tests (test_approval_guardrails.sh, test_ci_status_check.sh) — mechanically updated to point at the seam file for static source assertions, as the issue allows.
Required Checks
Review for path traversal vulnerabilities
Verified: No path traversal vulnerabilities introduced.
- All file-system operations are either (a) controlled reads from GitHub Actions environment variables (
$REPO,$PR_NUMBER) or (b) reading body files passed byaction.ymlstep contexts — none are derived from untrusted PR content. platform_comment_stickyreads$3(a body file path) via$(cat "$3"), but this is written by the action's own publish step, not exposed to PR content.- No file write operations that could be exploited for traversal.
- The forgejo backend (
_forgejo_py) is not yet implemented — every forgejo operation either delegates to a stub or calls_forgejo_unimplemented(), which fails loudly. The stub code passes repo/PR-number as arguments to the python module (not as filesystem paths), and the actual Forgejo backend module does not exist yet.
Test with edge-case paths (null bytes, symlinks)
Verified: Not applicable to this PR's scope.
- The platform seam operates on API identifiers (repo names, PR numbers, issue numbers) and file paths internal to the action (body files in
action.yml's${{ steps.*.outputs.* }}chain). None of these inputs are derived from PR diff content or external user input where null bytes or symlinks would be relevant attack vectors. - Repo names like
owner/repoare validated by the gh CLI and API layer, not by this seam. - The test suite (
test_platform_api.sh) validates correct argument passing with standard identifiers; the forgejo stubs are not yet executable.
Standards Compliance
✅ Conventions from AGENTS.md followed:
- The
scripts/platform_api.shfile is documented in AGENTS.md's key files list. - The platform seam is sourced (not executed) by all consumer scripts.
- Double-sourcing guard (
_PLATFORM_API_SOURCED) prevents issues whenpublish_helpers.shand callers both source it. #190 discipline preserved: wrappers are thin execs, never capture-and-echo.- Evidence/tool sections confirm no external command execution or URL fetching was triggered in this review.
Linked Issue Fit
No linked issues were found in the PR body for direct comparison. However, the PR's own description references PR 221 as the parent issue, and the implementation fully matches the described design: github backend reproduces exact pre-seam invocations, forgejo operations fail loudly, two function classes (platform_* vs github_enrich_*), and migration coverage of all ~40 call sites.
Evidence Provider Findings
No evidence providers are configured for this repository.
Tool Harness Findings
No tool harness was invoked (planning response was empty/default).
Unknowns / Needs Verification
None identified. The forgejo backend module (pr_reviewer/forgejo_backend.py) does not yet exist — this is by design, as the PR description states forgejo support is "rolling out across the 1.4.x line." The loud-failure stubs correctly enforce this incremental rollout without silent misbehavior.
Implements #221 (Forgejo umbrella #229, 1.4.x line): every host-forge API interaction now goes through one seam, with the gh CLI as the default backend and byte-identical output in github mode.
Design
scripts/platform_api.sh— sourced shell seam.platform_*functions cover the repo-under-review operations (PR get/diff/files, issue get/comments, compare, sticky comment, reviews create/dismiss/native, graphql, check-runs/commit-status, collaborator permission). The github branch of each wrapper is the exact pre-seamghinvocation.pr_reviewer/platform.py— Python mirror;resolve_finding_threads.pybuilds its gh argv through it. (run_tool_harness.py'sgh_apiuses urllib against api.github.com directly — that's Forgejo support 6/8: tool harness gh_api over the platform seam #226's seam point.)platforminput (auto|github|forgejo, defaultauto— detects fromGITHUB_SERVER_URL), wired asPLATFORMinto all six script-running steps.pr_reviewer/forgejo_backend.py; everything not yet implemented fails loudly with the operation name (reviews → Forgejo support 4/8: native reviews and inline-comment anchoring backend #224, CI status → Forgejo support 5/8: CI wait via commit statuses #225) rather than silently misbehaving. MissingFORGEJO_API_URLis a hard error.github_enrich_apiexists separately because linked-source enrichment (releases/tags/compares of upstream dependencies) targets github.com-hosted repos regardless of where the action runs. Forgejo support 7/8: degradation gating and platform support matrix docs #227 owns their degradation story.Migration coverage
All ~40 call sites:
run_review.sh(14),action.ymlpublish steps (7),check_review_needed.sh(5),publish_helpers.sh(5),wait_for_ci.sh(3),resolve_finding_threads.py,parse_review_command.sh. A final sweep confirms zero baregh pr|apiinvocations outside the seam.Regression evidence
ghvia PATH; since the github backend execs identical commands, they pass unmodified — except two static source-grep assertions (approval guardrails, CI-status) mechanically pointed at the seam file, as the issue allows.tests/test_platform_api.sh: 32 cases asserting every github-backend command line byte-for-byte, resolution logic (autodetection), and loud forgejo failures.tests/test_platform.pyfor the Python mirror.