[CHORE] Migrate PRojection GHA/Workflow here for reusability#20
[CHORE] Migrate PRojection GHA/Workflow here for reusability#20John McCall (lowlydba) merged 17 commits intomainfrom
Conversation
Overture PRojection ReviewThis PR migrates the Overture PRojection GitHub Action/workflow into this repository for centralized, reusable access across Overture projects. It includes documentation, core scripts, and unit tests for utility functions. ✅ Checks Passed
🚩 Flags
❓ Open Questions
Next steps:
No bugs, security issues, or logic errors were found in the reviewed files. Full review of the action's implementation and additional tests will be possible once the PR is split and resubmitted. |
There was a problem hiding this comment.
Pull request overview
Migrates the “Overture PRojection” AI PR review workflow/action into this centralized repo so it can be reused across other repositories, including private repos, with a composite action + reusable workflow entrypoint.
Changes:
- Added a reusable workflow (
workflow_call) and localpull_requesttrigger to run PRojection from this repo. - Added the composite action implementation (scripts + libs) for skill loading, diff fetching, skill selection, context fetching, and review posting.
- Added Node
node:testunit tests covering the helper libraries (diff budgeting, prompt building, models abstraction, etc.).
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/overture-projection.yml | New reusable + PR-triggered workflow wiring PRojection from this repo. |
| .github/actions/overture-projection/action.yml | Composite action definition orchestrating all PRojection steps. |
| .github/actions/overture-projection/README.md | Usage/docs for the composite action and its inputs. |
| .github/actions/overture-projection/scripts/load-skills.js | Loads skills from checked-out omf-devex skills directory. |
| .github/actions/overture-projection/scripts/fetch-diff.js | Fetches PR metadata/diffs and applies ignore + fetch ceiling budgeting. |
| .github/actions/overture-projection/scripts/select-skills.js | Calls a fast model to select which optional skills apply. |
| .github/actions/overture-projection/scripts/fetch-context.js | Fetches and truncates context files for selected skills only. |
| .github/actions/overture-projection/scripts/post-review.js | Builds prompts, dynamically budgets diffs, calls model, posts/upserts comment. |
| .github/actions/overture-projection/scripts/lib/skills.js | Frontmatter parsing + pr-reviewer surface filtering utilities. |
| .github/actions/overture-projection/scripts/lib/prompt.js | Pure prompt assembly utilities (system/user/preamble) + helpers. |
| .github/actions/overture-projection/scripts/lib/models.js | Provider abstraction for GitHub Models + Anthropic APIs. |
| .github/actions/overture-projection/scripts/lib/markdown.js | Markdown compression used for skills/context files. |
| .github/actions/overture-projection/scripts/lib/github.js | Repo/PR-number resolution helpers for cross-repo runs. |
| .github/actions/overture-projection/scripts/lib/diff.js | Ignore-pattern compilation + diff/context budgeting utilities. |
| .github/actions/overture-projection/scripts/lib/defaults.js | Centralized provider defaults for models/token budgets. |
| .github/actions/overture-projection/scripts/lib/context.js | Context ref parsing + per-file truncation/budgeting utilities. |
| .github/actions/overture-projection/scripts/lib/tests/skills.test.js | Unit tests for frontmatter parsing + skill filtering. |
| .github/actions/overture-projection/scripts/lib/tests/prompt.test.js | Unit tests for system/user prompt construction + flags. |
| .github/actions/overture-projection/scripts/lib/tests/models.test.js | Unit tests for provider normalization + rate limit logging. |
| .github/actions/overture-projection/scripts/lib/tests/markdown.test.js | Unit tests for markdown compression behavior. |
| .github/actions/overture-projection/scripts/lib/tests/github.test.js | Unit tests for repo/PR-number resolution. |
| .github/actions/overture-projection/scripts/lib/tests/diff.test.js | Unit tests for ignore patterns + budgeting behavior. |
| .github/actions/overture-projection/scripts/lib/tests/defaults.test.js | Unit tests for default values and provider defaults lookup. |
| .github/actions/overture-projection/scripts/lib/tests/context.test.js | Unit tests for context ref parsing + truncation + grouping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * COMMENT_MODE — 'update' | 'new' (default 'new') | ||
| * COMMENT_TAG — HTML marker for update-mode comment identification | ||
| * DRY_RUN — 'true' to skip posting and print instead | ||
| * PR_NUMBER — PR number override (falls back to event payload) | ||
| * REPOSITORY — target repo in owner/repo format (falls back to context) | ||
| * RUNNER_TEMP — standard Actions temp dir for inter-step artefacts |
There was a problem hiding this comment.
The env-var docs state COMMENT_MODE defaults to new, and the implementation also falls back to 'new', but the composite action input comment-mode defaults to update. Please align the documented/implemented default here with the action default (or make the divergence explicit).
| * COMMENT_MODE — 'update' | 'new' (default 'new') | |
| * COMMENT_TAG — HTML marker for update-mode comment identification | |
| * DRY_RUN — 'true' to skip posting and print instead | |
| * PR_NUMBER — PR number override (falls back to event payload) | |
| * REPOSITORY — target repo in owner/repo format (falls back to context) | |
| * RUNNER_TEMP — standard Actions temp dir for inter-step artefacts | |
| * COMMENT_MODE — 'update' | 'new' (default 'update') | |
| * COMMENT_TAG — HTML marker for update-mode comment identification | |
| * DRY_RUN — 'true' to skip posting and print instead | |
| * PR_NUMBER — PR number override (falls back to event payload) | |
| * REPOSITORY — target repo in owner/repo format (falls back to context) | |
| * RUNNER_TEMP — standard Actions temp dir for inter-step artefacts |
| // ── Post review ────────────────────────────────────────────────────────────── | ||
|
|
||
| const { owner, repo } = resolveRepo(process.env.REPOSITORY, context.repo); | ||
| const prNumber = resolvePrNumber(context.payload.pull_request?.number, process.env.PR_NUMBER); |
There was a problem hiding this comment.
resolvePrNumber(...) can return null; in that case the subsequent GitHub API calls will be made with issue_number/pull_number: null and fail with a less clear error. Add an explicit guard (setFailed + return) when prNumber is null, similar to fetch-diff.js.
| const prNumber = resolvePrNumber(context.payload.pull_request?.number, process.env.PR_NUMBER); | |
| const prNumber = resolvePrNumber(context.payload.pull_request?.number, process.env.PR_NUMBER); | |
| if (prNumber === null) { | |
| core.setFailed('Could not resolve PR number from context or PR_NUMBER'); | |
| return; | |
| } |
| * | ||
| * Env vars consumed: | ||
| * MAX_FILES — max number of changed files to include (default 20) | ||
| * MAX_DIFF_CHARS — fetch ceiling: max chars of patch content to pull from GitHub API (default 100000) |
There was a problem hiding this comment.
JSDoc formatting: the MAX_DIFF_CHARS env-var line is missing the leading * (it’s currently *), which breaks the comment block’s formatting. Fixing it keeps the header readable and consistent.
| * MAX_DIFF_CHARS — fetch ceiling: max chars of patch content to pull from GitHub API (default 100000) | |
| * MAX_DIFF_CHARS — fetch ceiling: max chars of patch content to pull from GitHub API (default 100000) |
| # -- Step 0a: Mask secrets so they never appear in logs -------------------- | ||
| - name: Mask secrets | ||
| shell: bash | ||
| run: | | ||
| if [ -n "${{ inputs['app-private-key'] }}" ]; then | ||
| echo "::add-mask::${{ inputs['app-private-key'] }}" | ||
| fi | ||
| if [ -n "${{ inputs['anthropic-api-key'] }}" ]; then | ||
| echo "::add-mask::${{ inputs['anthropic-api-key'] }}" | ||
| fi | ||
| if [ -n "${{ inputs['github-token'] }}" ]; then | ||
| echo "::add-mask::${{ inputs['github-token'] }}" | ||
| fi |
There was a problem hiding this comment.
::add-mask:: is being called once with the full app-private-key, which is typically a multi-line PEM. GitHub masking is line-oriented, so this may not reliably mask the key if any line is logged independently. Consider splitting on newlines and calling ::add-mask:: for each line (and/or masking a base64-encoded form) to ensure robust redaction.
| it('sends Bearer auth to GitHub Models', async () => { | ||
| let calledHeaders; | ||
| globalThis.fetch = async (_url, opts) => { calledHeaders = JSON.parse(opts.body); return fakeFetchResp(200, GH_SUCCESS); }; | ||
| // headers are in opts, body has model — check Authorization separately | ||
| globalThis.fetch = async (_url, opts) => { | ||
| calledHeaders = opts.headers; | ||
| return fakeFetchResp(200, GH_SUCCESS); | ||
| }; |
There was a problem hiding this comment.
This test assigns globalThis.fetch twice; the first assignment (parsing opts.body) is immediately overwritten and never asserted. Removing the dead assignment will make the intent clearer and avoid confusion when maintaining the test.
| always-skills: "pr-review" | ||
| repository: ${{ inputs.repository || github.event.repository.full_name }} | ||
| pr-number: ${{ inputs.pr_number || github.event.pull_request.number }} | ||
| devex-ref: ${{ inputs.devex_ref || 'main' }} | ||
| model: ${{ inputs.model }} | ||
| selection-model: ${{ inputs.selection_model }} | ||
| model-provider: ${{ inputs.model_provider || 'github-models' }} | ||
| max-input-tokens: ${{ inputs.max_input_tokens }} | ||
| max-output-tokens: ${{ inputs.max_output_tokens }} | ||
| dry-run: ${{ inputs.dry_run == true || inputs.dry_run == 'true' || env.ACT == 'true' }} |
There was a problem hiding this comment.
In pull_request-triggered runs, the inputs context is not available, but this job uses ${{ inputs.* }} in with:. That can cause the workflow to error at evaluation time. Consider deriving repository/pr-number from github.* only for pull_request events (or split into a pull_request wrapper workflow that calls a workflow_call-only reusable workflow).
| # Overture PRojection — AI PR review workflow. | ||
| # | ||
| # Triggers automatically on pull_request events, or manually via workflow_dispatch | ||
| # to review any open PR by number. | ||
| # | ||
| # ── Local testing with act ──────────────────────────────────────────────────── | ||
| # | ||
| # Use workflow_dispatch so --input flags are honoured. Do NOT pass -e with a | ||
| # pull_request event payload — act ignores --input when the event file contains | ||
| # a different event type. All PR targeting is done via --input pr_number and | ||
| # --input repository instead. | ||
| # | ||
| # dry-run is set automatically (ACT=true env var is set by act). | ||
| # | ||
| # GitHub Models — PowerShell: | ||
| # act workflow_dispatch ` | ||
| # -s GITHUB_TOKEN=$(gh auth token) ` | ||
| # -P ubuntu-slim=catthehacker/ubuntu:act-latest ` | ||
| # --input pr_number=<N> ` | ||
| # --input repository=OvertureMaps/<repo> | ||
| # | ||
| # GitHub Models — bash/zsh: | ||
| # act workflow_dispatch \ | ||
| # -s GITHUB_TOKEN=$(gh auth token) \ | ||
| # -P ubuntu-slim=catthehacker/ubuntu:act-latest \ | ||
| # --input pr_number=<N> \ | ||
| # --input repository=OvertureMaps/<repo> | ||
| # | ||
| # Anthropic — PowerShell: | ||
| # act workflow_dispatch ` | ||
| # -s GITHUB_TOKEN=$(gh auth token) ` | ||
| # -s ANTHROPIC_API_KEY=sk-ant-... ` | ||
| # -P ubuntu-slim=catthehacker/ubuntu:act-latest ` | ||
| # --input pr_number=<N> ` | ||
| # --input repository=OvertureMaps/<repo> ` | ||
| # --input model_provider=anthropic | ||
| # # model, max_input_tokens, max_output_tokens, selection_model all default | ||
| # # automatically for anthropic (claude-opus-4-6, 190000, 4096, claude-haiku-4-6) | ||
| # | ||
| # Anthropic — bash/zsh: | ||
| # act workflow_dispatch \ | ||
| # -s GITHUB_TOKEN=$(gh auth token) \ | ||
| # -s ANTHROPIC_API_KEY=sk-ant-... \ | ||
| # -P ubuntu-slim=catthehacker/ubuntu:act-latest \ | ||
| # --input pr_number=<N> \ | ||
| # --input repository=OvertureMaps/<repo> \ | ||
| # --input model_provider=anthropic | ||
| # # model, max_input_tokens, max_output_tokens, selection_model all default | ||
| # # automatically for anthropic (claude-opus-4-6, 190000, 4096, claude-haiku-4-6) | ||
| # | ||
| # ── Manual trigger (no act) ─────────────────────────────────────────────────── | ||
| # | ||
| # gh workflow run overture-projection.yml \ | ||
| # -f pr_number=<N> \ | ||
| # -f dry_run=true | ||
| # | ||
| # ── Required permissions ────────────────────────────────────────────────────── | ||
| # | ||
| # Token scopes: repo (read), pull-requests (write), issues (read), models (read) | ||
| # Org secret OVERTURE_PROJECTION_APP_PEM used for cross-repo context file reads. | ||
| # |
There was a problem hiding this comment.
The header documentation block is duplicated (the entire “Local testing with act / Required permissions” section appears twice). This makes future edits error-prone; please delete the repeated block so there’s a single source of truth.
| # Overture PRojection — AI PR review workflow. | |
| # | |
| # Triggers automatically on pull_request events, or manually via workflow_dispatch | |
| # to review any open PR by number. | |
| # | |
| # ── Local testing with act ──────────────────────────────────────────────────── | |
| # | |
| # Use workflow_dispatch so --input flags are honoured. Do NOT pass -e with a | |
| # pull_request event payload — act ignores --input when the event file contains | |
| # a different event type. All PR targeting is done via --input pr_number and | |
| # --input repository instead. | |
| # | |
| # dry-run is set automatically (ACT=true env var is set by act). | |
| # | |
| # GitHub Models — PowerShell: | |
| # act workflow_dispatch ` | |
| # -s GITHUB_TOKEN=$(gh auth token) ` | |
| # -P ubuntu-slim=catthehacker/ubuntu:act-latest ` | |
| # --input pr_number=<N> ` | |
| # --input repository=OvertureMaps/<repo> | |
| # | |
| # GitHub Models — bash/zsh: | |
| # act workflow_dispatch \ | |
| # -s GITHUB_TOKEN=$(gh auth token) \ | |
| # -P ubuntu-slim=catthehacker/ubuntu:act-latest \ | |
| # --input pr_number=<N> \ | |
| # --input repository=OvertureMaps/<repo> | |
| # | |
| # Anthropic — PowerShell: | |
| # act workflow_dispatch ` | |
| # -s GITHUB_TOKEN=$(gh auth token) ` | |
| # -s ANTHROPIC_API_KEY=sk-ant-... ` | |
| # -P ubuntu-slim=catthehacker/ubuntu:act-latest ` | |
| # --input pr_number=<N> ` | |
| # --input repository=OvertureMaps/<repo> ` | |
| # --input model_provider=anthropic | |
| # # model, max_input_tokens, max_output_tokens, selection_model all default | |
| # # automatically for anthropic (claude-opus-4-6, 190000, 4096, claude-haiku-4-6) | |
| # | |
| # Anthropic — bash/zsh: | |
| # act workflow_dispatch \ | |
| # -s GITHUB_TOKEN=$(gh auth token) \ | |
| # -s ANTHROPIC_API_KEY=sk-ant-... \ | |
| # -P ubuntu-slim=catthehacker/ubuntu:act-latest \ | |
| # --input pr_number=<N> \ | |
| # --input repository=OvertureMaps/<repo> \ | |
| # --input model_provider=anthropic | |
| # # model, max_input_tokens, max_output_tokens, selection_model all default | |
| # # automatically for anthropic (claude-opus-4-6, 190000, 4096, claude-haiku-4-6) | |
| # | |
| # ── Manual trigger (no act) ─────────────────────────────────────────────────── | |
| # | |
| # gh workflow run overture-projection.yml \ | |
| # -f pr_number=<N> \ | |
| # -f dry_run=true | |
| # | |
| # ── Required permissions ────────────────────────────────────────────────────── | |
| # | |
| # Token scopes: repo (read), pull-requests (write), issues (read), models (read) | |
| # Org secret OVERTURE_PROJECTION_APP_PEM used for cross-repo context file reads. | |
| # |
| 1. **Load skills** — sparse-checkouts `omf-devex/skills/`, parses frontmatter, filters to `pr-reviewer` surface. Raw content is stored; nothing is fetched yet. | ||
| 2. **Fetch PR diff** — title, body, branch refs, closing issues (GraphQL), and changed file patches up to `max-diff-chars`. | ||
| 3. **Select skills** — a fast/cheap model reads skill descriptions and changed file paths, picks which optional skills apply, and logs its reasoning. `always-skills` bypass this step entirely. | ||
| 4. **Fetch context files** — only for selected skills; fetched in parallel via the GitHub App token, compressed, and capped at 5,000 chars each. |
There was a problem hiding this comment.
Step 4 says context files are “capped at 5,000 chars each”, but fetch-context.js computes a dynamic per-file cap (10% of max-input-tokens) and only applies a hard cap when max-context-file-chars is set. Please update the README to reflect the actual behavior (dynamic limit + optional override).
| 4. **Fetch context files** — only for selected skills; fetched in parallel via the GitHub App token, compressed, and capped at 5,000 chars each. | |
| 4. **Fetch context files** — only for selected skills; fetched in parallel via the GitHub App token, compressed, and capped per file using a dynamic limit (10% of `max-input-tokens`) unless `max-context-file-chars` is set to override it with a hard cap. |
| if (!fs.existsSync(skillsDir)) { | ||
| core.warning(`⚠️ Skills directory not found: ${skillsDir} — no skills will be loaded.`); |
There was a problem hiding this comment.
fs.existsSync(skillsDir) will throw if SKILLS_DIR is unset/empty (it expects a string/URL/Buffer). Add a guard for a missing SKILLS_DIR (and treat it the same as “directory not found”) so the step fails gracefully with a clear message.
| if (!fs.existsSync(skillsDir)) { | |
| core.warning(`⚠️ Skills directory not found: ${skillsDir} — no skills will be loaded.`); | |
| if (!skillsDir || !skillsDir.trim() || !fs.existsSync(skillsDir)) { | |
| const skillsDirMessage = skillsDir && skillsDir.trim() ? skillsDir : '(SKILLS_DIR not set)'; | |
| core.warning(`⚠️ Skills directory not found: ${skillsDirMessage} — no skills will be loaded.`); |
Signed-off-by: John McCall <john@overturemaps.org>
Signed-off-by: John McCall <john@overturemaps.org>
Signed-off-by: John McCall <john@overturemaps.org>
Introduce a new GitHub Action 'overture-projection' that posts AI-generated PR review comments driven by reusable "skills". Adds action metadata (action.yml), a detailed README, multiple step scripts (load-skills, fetch-diff, select-skills, fetch-context, post-review) and library modules with unit tests for defaults, diff handling, and context processing. The action supports GitHub Models and Anthropic providers, handles skill selection, per-file context fetching/truncation, diff budgeting, and generates/upserts review comments using an installation or workflow token. Signed-off-by: John McCall <john@overturemaps.org>
Signed-off-by: John McCall <john@overturemaps.org>
…ow context Signed-off-by: John McCall <john@overturemaps.org>
…epos Signed-off-by: John McCall <john@overturemaps.org>
Signed-off-by: John McCall <john@overturemaps.org>
4460327 to
030d1cf
Compare
Multiple fixes and workflow improvements for the overture-projection action: - Make frontmatter/markdown parsing robust to CRLF line endings (skills.js, markdown.js). - Improve fetch-diff comment to clarify file-dropping behavior order. - Fix unit test setup that clobbered global fetch in models.test.js. - Validate SKILLS_DIR presence and warn when empty/missing (load-skills.js). - Adjust COMMENT_MODE docs/default wording and fail early if PR number can't be resolved (post-review.js). - Add a dedicated unit test workflow for the action scripts (overture-projection-tests.yml). - Update main workflow to use the reusable-workflows repo, add conditional input handling for dispatch vs PR runs, and simplify header comments (overture-projection.yml). - README updates: clarify per-file context cap default (max-context-file-chars) and reference the reusable workflow commit. These changes improve cross-platform robustness, error handling, and CI ergonomics. Signed-off-by: John McCall <john@overturemaps.org>
Signed-off-by: John McCall <john@overturemaps.org>
Signed-off-by: John McCall <john@overturemaps.org>
Alex Iannicelli (atiannicelli)
left a comment
There was a problem hiding this comment.
I'm gonna just say... ok
Signed-off-by: John McCall <john@overturemaps.org>
Signed-off-by: John McCall <john@overturemaps.org>
Enable single active run per PR with a concurrency group and cancel-in-progress behavior; move write/read permissions for PRs, issues, and models to the job scope and keep only contents: read at top-level. Improve secrets handling in the action: add explanatory masks for template-expanded inputs and set checkout persist-credentials: false to avoid leaking tokens to the checked-out workspace. Add inline justifications for unpinned action usage and org secrets usage for reviewers. Signed-off-by: John McCall <john@overturemaps.org>
Add zizmor lint ignore annotations in the overture-projection action to suppress template-injection/unpinned-uses warnings for intentional literal values and local script usage. Enable concurrency in the test workflow to cancel in-progress test runs for the same PR/run, and set actions/checkout persist-credentials: false in the tests job to avoid leaving credentials in the workspace. Tidy comments in the main workflow's uses/secret entries by moving lint ignores inline. Signed-off-by: John McCall <john@overturemaps.org>
Signed-off-by: John McCall <john@overturemaps.org>
Signed-off-by: John McCall <john@overturemaps.org>
The original location of this AI review PR Action was in a private repo, but that inherently hampers our ability to use it in other private repositories.
This will move it to the centralized place for shared workflows/actions.