ci: notify help-docs hub on docs changes - #1084
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
📝 WalkthroughWalkthroughA new GitHub Actions workflow monitors documentation changes pushed to ChangesDocumentation synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant GitHubAPI
participant HelpDocs
GitHubActions->>GitHubAPI: Resolve source pull request metadata
GitHubActions->>GitHubActions: Build commit and pull request payload
GitHubActions->>GitHubAPI: Create repository-scoped GitHub App token
GitHubActions->>HelpDocs: Dispatch promote-from-product event
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.github/workflows/notify-help-docs.yml (1)
21-21: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin both workflow actions to full commit SHAs.
actions/create-github-app-token@v1produces the App token, andpeter-evans/repository-dispatch@v3passes it toAltimateAI/help-docs. Mutable tags can move, so pin bothuses:entries to verified full commit SHAs.🤖 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 @.github/workflows/notify-help-docs.yml at line 21, Pin both workflow actions—the actions/create-github-app-token and peter-evans/repository-dispatch uses entries—to verified full commit SHAs instead of mutable version tags, preserving their existing action versions and workflow behavior.Source: MCP tools
🤖 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.
Inline comments:
In @.github/workflows/notify-help-docs.yml:
- Line 18: Update the workflow guard to inspect all messages in
github.event.commits rather than only github.event.head_commit.message, and
prevent dispatch when any pushed commit contains the [docs-sync] marker.
- Around line 3-11: Change the workflow trigger from the current main-branch
push in the workflow’s on configuration to a merged-only pull request event,
filtering for docs/docs/** and requiring github.event.pull_request.merged ==
true. Update any client-payload before/after references to use the corresponding
pull_request event fields while preserving the existing docs-sync notification
behavior.
- Around line 13-15: Update the token-generation step in the workflow to pass
permission-contents: write while retaining the audited commit-SHA pin for
actions/create-github-app-token@v1 and the AltimateAI/help-docs repository
restriction. Keep the job-level permissions unchanged; the App installation
token must be scoped to Contents: write only.
- Around line 21-25: Update the workflow’s token and dispatch steps to run only
when both docs-sync credentials are configured. Add a preflight configuration
step that exposes a boolean output, using a step-local environment variable for
the secret-dependent private-key check, then gate both
actions/create-github-app-token and peter-evans/repository-dispatch with that
output.
---
Nitpick comments:
In @.github/workflows/notify-help-docs.yml:
- Line 21: Pin both workflow actions—the actions/create-github-app-token and
peter-evans/repository-dispatch uses entries—to verified full commit SHAs
instead of mutable version tags, preserving their existing action versions and
workflow behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 29f7383e-f911-4b4d-8cf5-1644cf55834a
📒 Files selected for processing (1)
.github/workflows/notify-help-docs.yml
| # On a merge that touches docs on the default branch, tell the help-docs hub to | ||
| # promote the change up (product -> help-docs). Requires the shared docs-sync | ||
| # GitHub App: set vars.DOCS_SYNC_APP_ID and secrets.DOCS_SYNC_APP_PRIVATE_KEY. | ||
| # Sync-generated merges carry a [docs-sync] marker and are skipped (no loop). | ||
|
|
||
| on: | ||
| push: | ||
| branches: [main] | ||
| paths: ["docs/docs/**"] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 \
'push:|pull_request(_target)?:|types:|closed|merged|github\.event\.(before|after)|merge_commit_sha' \
.github/workflows/notify-help-docs.ymlRepository: AltimateAI/altimate-code
Length of output: 694
🌐 Web query:
GitHub Actions push event runs for merged pull requests direct pushes pull_request_target merged condition
💡 Result:
GitHub Actions workflows behave differently depending on the trigger event used [1][2]. When a pull request is merged, it results in a push event to the target branch, but this push event does not contain the pull request metadata required to check status conditions like github.event.pull_request.merged [3][4][5]. Key points regarding these events: 1. Push Event Limitations: The push event payload does not include information about the pull request that initiated the merge [4][5]. Consequently, attempting to use github.event.pull_request.merged within a push-triggered workflow will fail because that object is empty or undefined [3][5]. To detect merges within a push workflow, you must rely on workarounds, such as checking the commit message for standard merge patterns (e.g., 'Merge pull request...') or using the GitHub API to check if the commit has multiple parents [5]. 2. Pull Request Event: To trigger a workflow specifically when a pull request is merged, you should use the pull_request event with the closed activity type [1][2][6]. You can then verify the merge status using a conditional expression at the job level [6]: on: pull_request: types: [closed] jobs: if_merged: if: github.event.pull_request.merged == true runs-on: ubuntu-latest steps: - run: echo The PR was merged 3. Pull Request Target Event: The pull_request_target event also supports the closed activity type and the merged conditional [6]. However, it is fundamentally different in security context [7]. Unlike pull_request, which runs using the code from the pull request's merge commit, pull_request_target runs in the context of the base repository and is granted elevated permissions (such as access to secrets) [7]. It is intended for scenarios where you need to perform actions on pull requests from forks while maintaining access to repository secrets, but it requires careful security considerations to prevent malicious code execution [7]. In summary, use pull_request with the closed activity type to reliably detect merges [6]. Avoid using pull_request_target unless you specifically require its elevated security context, and avoid relying on push events for merge-specific logic as they lack the necessary pull request context [3][7][5].
Citations:
- 1: https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows
- 2: https://docs.github.com/actions/using-workflows/events-that-trigger-workflows
- 3: https://stackoverflow.com/questions/76443801/github-actions-getting-push-event-when-pull-request-merged
- 4: https://stackoverflow.com/questions/71382892/github-actions-on-a-push-to-master-workflow-how-can-i-get-a-link-to-the-pull-r
- 5: https://stackoverflow.com/questions/79105435/how-to-only-trigger-push-on-push-and-not-on-branch-merge-after-pull-request
- 6: https://stackoverflow.com/questions/60710209/trigger-github-actions-only-when-pr-is-merged
- 7: https://docs.github.com/en/actions/reference/security/securely-using-pull_request_target
Run this workflow only for merged pull requests.
push also matches direct pushes to main when docs/docs/** changes, but this workflow is meant to notify after merging. Use a closed pull_request_target trigger with github.event.pull_request.merged == true, or another merged-only flow, and update the before/after client-payload fields for the chosen event.
🤖 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 @.github/workflows/notify-help-docs.yml around lines 3 - 11, Change the
workflow trigger from the current main-branch push in the workflow’s on
configuration to a merged-only pull request event, filtering for docs/docs/**
and requiring github.event.pull_request.merged == true. Update any
client-payload before/after references to use the corresponding pull_request
event fields while preserving the existing docs-sync notification behavior.
Source: MCP tools
| permissions: | ||
| contents: read | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'permissions:|create-github-app-token|repositories:|permission-' .github
gh api /orgs/AltimateAI/installation --jq '.permissions'Repository: AltimateAI/altimate-code
Length of output: 11267
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow =="
cat -n .github/workflows/notify-help-docs.yml
echo
echo "== create-github-app-token action v1 metadata =="
git ls-files .github
tmpdir="$(mktemp -d)"
cd "$tmpdir"
git init >/dev/null 2>&1
git remote add origin https://github.com/actions/create-github-app-token.git >/dev/null 2>&1
git fetch --no-tags --depth 1 origin 9626c2827b2773687c9ceab62c3cf66ef99169 >/dev/null 2>&1 || true
git checkout --detach FETCH_HEAD >/dev/null 2>&1 || true
find . -maxdepth 3 -type f \( -name 'action.yml' -o -name 'README.md' \) -print
if [ -f action.yml ]; then cat -n action.yml; fi
echo
echo "== repository-dispatch metadata =="
git init >/dev/null 2>&1
git remote add origin https://github.com/peter-evans/repository-dispatch.git >/dev/null 2>&1
git fetch --no-tags --depth 1 origin v3 >/dev/null 2>&1 || true
git checkout --detach FETCH_HEAD >/dev/null 2>&1 || true
if [ -f action.yml ]; then cat -n action.yml; fiRepository: AltimateAI/altimate-code
Length of output: 2587
🌐 Web query:
actions/create-github-app-token v1 permissions input permission-contents default README
💡 Result:
The actions/create-github-app-token action does not have a default value for the permission-contents input [1]. By default, if you do not specify any permission- inputs (including permission-contents), the generated GitHub App installation access token automatically inherits all of the permissions granted to the GitHub App installation itself [2][3][4]. When you choose to use the permission-contents input, you must explicitly set it to either read or write [1]. The action recommends explicitly listing the permissions required for your specific use case to follow the principle of least privilege [2][3]. If you set any permission- input (such as permission-contents), the token's permissions are restricted to only those explicitly defined in your workflow configuration [5][6]. Ensure that any permission you specify is also granted to the GitHub App installation, otherwise the action will return an error [2][3].
Citations:
- 1: https://github.com/actions/create-github-app-token/blob/29824e69f54612133e76f7eaac726eef6c875baf/action.yml
- 2: https://github.com/actions/create-github-app-token
- 3: https://github.com/actions/create-github-app-token/blob/main/README.md
- 4: https://github.com/actions/create-github-app-token/tree/v1.9.2
- 5: https://deepwiki.com/actions/create-github-app-token/4.2-permission-system
- 6: https://deepwiki.com/actions/create-github-app-token/4.1-action-inputs
Restrict the generated App token to the contents permission used by the dispatch.
repositories: help-docs scopes the repository, but actions/create-github-app-token@v1 still issues an installation token with all App installation permissions unless a permission-* input is provided. The workflow permissions: contents: read controls the job’s GITHUB_TOKEN, not steps.tok.outputs.token. Configure the App installation with Contents: write on AltimateAI/help-docs only; then pass permission-contents: write after keeping the action pinned to an audited commit SHA.
🤖 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 @.github/workflows/notify-help-docs.yml around lines 13 - 15, Update the
token-generation step in the workflow to pass permission-contents: write while
retaining the audited commit-SHA pin for actions/create-github-app-token@v1 and
the AltimateAI/help-docs repository restriction. Keep the job-level permissions
unchanged; the App installation token must be scoped to Contents: write only.
Sources: MCP tools, Linters/SAST tools
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/notify-help-docs.yml">
<violation number="1" location=".github/workflows/notify-help-docs.yml:8">
P2: The `push` trigger on `main` fires for any push that touches `docs/docs/**`, including direct pushes to the branch, not just merged pull requests. This is broader than the stated intent of notifying only when a docs PR merges. Consider a merged-only trigger (e.g., a closed `pull_request` event filtered on `github.event.pull_request.merged == true`) if direct pushes to main should not trigger the dispatch.</violation>
<violation number="2" location=".github/workflows/notify-help-docs.yml:26">
P2: `actions/create-github-app-token` issues a token that inherits all of the GitHub App installation's permissions by default; `repositories: help-docs` only scopes which repo the token can touch, not what it can do there. The job-level `permissions: contents: read` doesn't apply to this generated token either, since that only governs `GITHUB_TOKEN`. Add an explicit `permission-contents: write` (or the minimal set actually required by the dispatch) to avoid granting this token broader access than needed.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| with: | ||
| app-id: ${{ vars.DOCS_SYNC_APP_ID }} | ||
| private-key: ${{ secrets.DOCS_SYNC_APP_PRIVATE_KEY }} | ||
| owner: AltimateAI |
There was a problem hiding this comment.
P2: actions/create-github-app-token issues a token that inherits all of the GitHub App installation's permissions by default; repositories: help-docs only scopes which repo the token can touch, not what it can do there. The job-level permissions: contents: read doesn't apply to this generated token either, since that only governs GITHUB_TOKEN. Add an explicit permission-contents: write (or the minimal set actually required by the dispatch) to avoid granting this token broader access than needed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/notify-help-docs.yml, line 26:
<comment>`actions/create-github-app-token` issues a token that inherits all of the GitHub App installation's permissions by default; `repositories: help-docs` only scopes which repo the token can touch, not what it can do there. The job-level `permissions: contents: read` doesn't apply to this generated token either, since that only governs `GITHUB_TOKEN`. Add an explicit `permission-contents: write` (or the minimal set actually required by the dispatch) to avoid granting this token broader access than needed.</comment>
<file context>
@@ -0,0 +1,34 @@
+ with:
+ app-id: ${{ vars.DOCS_SYNC_APP_ID }}
+ private-key: ${{ secrets.DOCS_SYNC_APP_PRIVATE_KEY }}
+ owner: AltimateAI
+ repositories: help-docs
+ - uses: peter-evans/repository-dispatch@v3
</file context>
| # GitHub App: set vars.DOCS_SYNC_APP_ID and secrets.DOCS_SYNC_APP_PRIVATE_KEY. | ||
| # Sync-generated merges carry a [docs-sync] marker and are skipped (no loop). | ||
|
|
||
| on: |
There was a problem hiding this comment.
P2: The push trigger on main fires for any push that touches docs/docs/**, including direct pushes to the branch, not just merged pull requests. This is broader than the stated intent of notifying only when a docs PR merges. Consider a merged-only trigger (e.g., a closed pull_request event filtered on github.event.pull_request.merged == true) if direct pushes to main should not trigger the dispatch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/notify-help-docs.yml, line 8:
<comment>The `push` trigger on `main` fires for any push that touches `docs/docs/**`, including direct pushes to the branch, not just merged pull requests. This is broader than the stated intent of notifying only when a docs PR merges. Consider a merged-only trigger (e.g., a closed `pull_request` event filtered on `github.event.pull_request.merged == true`) if direct pushes to main should not trigger the dispatch.</comment>
<file context>
@@ -0,0 +1,34 @@
+# GitHub App: set vars.DOCS_SYNC_APP_ID and secrets.DOCS_SYNC_APP_PRIVATE_KEY.
+# Sync-generated merges carry a [docs-sync] marker and are skipped (no loop).
+
+on:
+ push:
+ branches: [main]
</file context>
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| run: | | ||
| J="$(gh api repos/${{ github.repository }}/commits/${{ github.sha }}/pulls --jq '.[0] // {}')" |
There was a problem hiding this comment.
WARNING: A transient API failure here silently skips the docs-sync dispatch.
GitHub Actions runs run: blocks under set -e (and pipefail) by default. This gh api .../commits/.../pulls call has no fallback, so a transient failure (rate limit, 5xx, network) makes the command substitution exit non-zero, failing the step. Because the downstream repository-dispatch step uses the default if: success(), the whole notification to help-docs is then skipped and the docs change is never promoted. The users/$LOGIN call below already guards with || echo ''; this one does not. Add the same fallback so a transient error degrades to empty metadata instead of aborting the job.
| J="$(gh api repos/${{ github.repository }}/commits/${{ github.sha }}/pulls --jq '.[0] // {}')" | |
| J="$(gh api repos/${{ github.repository }}/commits/${{ github.sha }}/pulls --jq '.[0] // {}' || echo '{}')" |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous Review Summaries (3 snapshots, latest commit e897998)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit e897998)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous review (commit 1ceb638)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous review (commit f3242ad)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file)
Reviewed by glm-5.2 · Input: 32.7K · Output: 9.2K · Cached: 294.5K Review guidance: REVIEW.md from base branch |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/notify-help-docs.yml (1)
36-36: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the action references to full commit SHAs.
Both actions handle credentials or a write-capable token but use mutable major tags. Pin each action to a reviewed full-length commit SHA. GitHub identifies full-length SHAs as the immutable release reference. (docs.github.com)
Also applies to: 44-44
🤖 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 @.github/workflows/notify-help-docs.yml at line 36, Pin the action references at the credential and write-token steps, including actions/create-github-app-token and the action at the other referenced location, to reviewed full-length commit SHAs instead of mutable version tags.
🤖 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 @.github/workflows/notify-help-docs.yml:
- Line 36: Pin the action references at the credential and write-token steps,
including actions/create-github-app-token and the action at the other referenced
location, to reviewed full-length commit SHAs instead of mutable version tags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a43fd051-2015-4f4e-9513-4100b953c550
📒 Files selected for processing (1)
.github/workflows/notify-help-docs.yml
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Consensus Code Review — 7-model panel
Verdict: REQUEST CHANGES · Critical 1 · Major 5 · Minor 7 · Nits 5
Reviewed independently by Claude Opus 5, GPT-5.4 Codex, Kimi K2.5, GLM-5.1, Qwen 3.6, MiniMax M2.7, MiMo V2 Pro, then converged over one agreement round (2 APPROVE, 3 CHANGES NEEDED — all valid objections incorporated).
Critical, major and minor findings are posted as inline comments. Nits and everything not attributable to a single line are in the follow-up comment below.
Blocking
- C1 (line 57) —
vars.DOCS_SYNC_APP_IDis empty; the App ID is stored as a secret. Every docs push tomainwill fail red and nothing will ever dispatch. - M2 (line 24) — the
[docs-sync]loop guard doesn't survive a merge-commit merge, which this repo allows. - M4 (line 65) — the help-docs consumer (PR #43) is still open, so the dispatch is a silent no-op today.
What's done well
- Both third-party actions pinned to full commit SHAs with version comments — matches
docs.ymland blocks tag-hijack attacks. (noted by all 7 reviewers) - App token scoped to a single repository and auto-revoked at job end.
- Minimal
permissions:—contents: read,pull-requests: read, no write scopes. - Payload built with
jq -cn --argrather than string concatenation, so quotes and backslashes can't break the JSON. - Untrusted PR-derived values flow through step outputs and
env:, never into shell source. This is why the panel's injection findings were rejected rather than reported. paths: ["docs/docs/**"]correctly narrows to the published subtree and excludesdocs/internal/— tighter and more correct thandocs.yml'sdocs/**.- The cross-repo payload contract matches the consumer field-for-field.
- Lean job: no checkout, no unnecessary tooling.
| - uses: actions/create-github-app-token@d72941d797fd3113feb6b93fd0dec494b13a2547 # v1 | ||
| id: tok | ||
| with: | ||
| app-id: ${{ vars.DOCS_SYNC_APP_ID }} |
There was a problem hiding this comment.
CRITICAL — vars.DOCS_SYNC_APP_ID is wrong: the App ID is stored as a secret
Verified against this repo via the API:
| Call | Result |
|---|---|
GET /repos/AltimateAI/altimate-code/actions/variables |
{"variables":[],"total_count":0} — zero Actions variables |
GET /repos/AltimateAI/altimate-code/actions/secrets |
includes DOCS_SYNC_APP_ID and DOCS_SYNC_APP_PRIVATE_KEY |
So ${{ vars.DOCS_SYNC_APP_ID }} resolves to the empty string. actions/create-github-app-token declares app-id as a required input and fails the step on an empty value.
Net effect: every docs push to main produces a red workflow run, and no dispatch is ever sent. The PR description's claim that this is "inert until those are set" is wrong on both counts — the credentials are configured (as secrets), and an unset credential fails loudly rather than sitting inert.
| app-id: ${{ vars.DOCS_SYNC_APP_ID }} | |
| app-id: ${{ secrets.DOCS_SYNC_APP_ID }} |
Line 5's comment needs the same correction (vars.DOCS_SYNC_APP_ID → secrets.DOCS_SYNC_APP_ID).
Caveat: org-level Actions variables couldn't be enumerated (403). If an org-level DOCS_SYNC_APP_ID variable also exists this is an ambiguity rather than a hard break — but the identically-named repo secret makes the secret the intended source.
| '{product:$product,before:$before,after:$after,source_pr:$source_pr,source_author:$source_author,source_url:$source_url}')" | ||
| echo "json=$JSON" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - uses: actions/create-github-app-token@d72941d797fd3113feb6b93fd0dec494b13a2547 # v1 |
There was a problem hiding this comment.
MAJOR — no guard for absent App credentials; the job hard-fails instead of skipping
Flagged independently by all 7 reviewers. Even with the credential reference fixed, there is no if: gate here. On a fork, a repo where the org secret isn't visible, or after a key rotation, create-github-app-token errors and an unrelated docs merge shows a failed check on main.
The repo's own sibling workflow already handles this — dispatch-code-review.yml:44-48:
if [ -z "$GH_TOKEN" ]; then
echo "AUTOPILOT_DISPATCH_TOKEN not available — skipping centralized dispatch."
exit 0
fisecrets isn't available in a job-level if, so gate at step level — and gate every subsequent step, not just the token and dispatch, or a jq/runner failure still reddens an unconfigured workflow:
- name: Check docs-sync configuration
id: config
env:
APP_ID: ${{ secrets.DOCS_SYNC_APP_ID }}
APP_KEY: ${{ secrets.DOCS_SYNC_APP_PRIVATE_KEY }}
run: |
if [[ -n "$APP_ID" && -n "$APP_KEY" ]]; then
echo "enabled=true" >> "$GITHUB_OUTPUT"
else
echo "enabled=false" >> "$GITHUB_OUTPUT"
echo "::notice::docs-sync App not configured; skipping dispatch."
fithen if: steps.config.outputs.enabled == 'true' on all remaining steps.
|
|
||
| jobs: | ||
| notify: | ||
| if: ${{ !contains(github.event.head_commit.message, '[docs-sync]') }} |
There was a problem hiding this comment.
MAJOR — loop prevention is defeated by the merge-commit strategy
The counterpart down-sync (help-docs PR #43, sync-docs.yml) opens its PR into this repo with:
commit-message: "docs: sync from help-docs (source of truth) [docs-sync]"← marker presenttitle: "docs: sync from help-docs"← marker absent
This repo's merge settings, read from the API:
allow_merge_commit: true
merge_commit_title: MERGE_MESSAGE → "Merge pull request #N from AltimateAI/docs/sync-from-help-docs"
merge_commit_message: PR_TITLE → "docs: sync from help-docs"
On a merge-commit merge, head_commit is the merge commit, whose message carries no [docs-sync] marker. The marked branch commit is still in the push payload's commits[] array — but this guard never looks there. The condition passes and the sync bounces back at help-docs.
Squash (squash_merge_commit_message: COMMIT_MESSAGES) and rebase both preserve the marker and are safe — but merge commits are enabled and nothing enforces the strategy. Blast radius is bounded (help-docs's semantic diffing should find no change and open no PR), but the header comment's "so this never loops" doesn't hold as written.
Fix — check every commit in the push, which is exactly where the marker survives a merge commit:
| if: ${{ !contains(github.event.head_commit.message, '[docs-sync]') }} | |
| if: ${{ !contains(toJSON(github.event.commits.*.message), '[docs-sync]') }} |
Complement it by adding [docs-sync] to the down-sync PR title in help-docs #43 so every merge strategy propagates it, and by making the receiver reject already-imported SHAs.
Note: !endsWith(github.actor, '[bot]') is not a safe alternative — it suppresses every bot-authored docs update, and a human merging the sync PR is still the actor.
| jobs: | ||
| notify: | ||
| if: ${{ !contains(github.event.head_commit.message, '[docs-sync]') }} | ||
| runs-on: arc-runner-gke |
There was a problem hiding this comment.
MAJOR — arc-runner-gke is an unverified prerequisite, and the image contract is unstated
Flagged by all 7 reviewers. This is the only occurrence of arc-runner-gke in .github/ — the other 22 Linux jobs use ubuntu-latest, 2 use windows-latest.
A repo-wide grep can't disprove an org-level ARC scale set, and help-docs's sync-docs.yml uses the same label, so treat this as a rollout prerequisite to verify rather than a proven defect. But if the scale set is not assigned to this repository, the job sits queued with no failure signal — and timeout-minutes does not help there; it bounds execution after scheduling, not queue time.
The image contract is the harder problem. These steps need bash, gh, jq, and a Node 20 runtime for the two external actions, with no actions/checkout and no setup step. If gh is missing, line 32's || echo '{}' swallows it and the dispatch goes out with empty author metadata; if jq is missing, line 48 fails the job.
Either confirm the scale set is assigned to this repo before merge, or use ubuntu-latest (which dispatch-code-review.yml already does for the same kind of cross-repo dispatch). If the ARC runner stays, add a preflight:
- name: Verify runner tooling
run: |
command -v gh >/dev/null || { echo "::error::gh not on runner"; exit 1; }
command -v jq >/dev/null || { echo "::error::jq not on runner"; exit 1; }| - uses: peter-evans/repository-dispatch@ff45666b9427631e3450c54a1bcbee4d9ff4d7c0 # v3 | ||
| with: | ||
| token: ${{ steps.tok.outputs.token }} | ||
| repository: AltimateAI/help-docs |
There was a problem hiding this comment.
MAJOR — merge-order prerequisite: the consumer doesn't exist yet
AltimateAI/help-docs has no repository_dispatch handler for promote-from-product on its default branch. The handler lives in help-docs PR #43 (feat/sync-docs-to-oss), which is still open. POST /dispatches returns 204 whether or not anything is listening, so this will "succeed" and do nothing, with no signal that the far end is missing.
The payload contract itself is correct — {product, before, after, source_pr, source_author, source_url} matches the consumer's reads at sync-docs.yml:232-237, product: code matches the code: key in tools/oss_sources.yml, and that entry's docs_path: docs/docs matches this workflow's paths: ["docs/docs/**"] filter. Good contract, wrong merge order.
Merge help-docs #43 first, or land this behind the credential guard so it's genuinely inert until both ends are live. If the ordering is already coordinated, state it as a merge prerequisite in the PR description.
| { | ||
| echo "num=$(echo "$J" | jq -r '.number // ""')" | ||
| echo "login=$(echo "$J" | jq -r '.user.login // ""')" | ||
| echo "url=$(echo "$J" | jq -r '.html_url // ""')" |
There was a problem hiding this comment.
MINOR — direct pushes to main dispatch with all source fields empty
Every push to main triggers this workflow, not only merges. A direct push produces empty num/login/url, and the receiver gets a promotion request it can't attribute to anyone.
Fall back to the commit author:
[ -z "$NUM" ] && LOGIN="$(gh api "repos/$GITHUB_REPOSITORY/commits/$GITHUB_SHA" --jq '.author.login // ""')"Do not gate the dispatch on num != '' — that would suppress legitimate direct docs changes.
| - name: Build dispatch payload | ||
| id: payload | ||
| env: | ||
| BEFORE: ${{ github.event.before }} |
There was a problem hiding this comment.
MINOR — github.event.before is forwarded without a recovery mode
Flagged by all 7 reviewers. On branch creation before is the all-zero SHA; after a force-push it can name a commit no longer reachable. The consumer does git diff $BEFORE..$AFTER, which breaks in both cases.
This repo already has the guard pattern — ci.yml:554:
if [[ "${{ github.event.before }}" == "0000000000000000000000000000000000000000" ]]; thenNormalize the zero-SHA to "" and forward github.event.created / github.event.forced in the payload — without them the receiver can't tell a new branch from a force-push, and can't decide when to fall back to a full reconciliation.
| app-id: ${{ vars.DOCS_SYNC_APP_ID }} | ||
| private-key: ${{ secrets.DOCS_SYNC_APP_PRIVATE_KEY }} | ||
| owner: AltimateAI | ||
| repositories: help-docs |
There was a problem hiding this comment.
MINOR — the App token inherits every installation permission
Scoping to owner: AltimateAI / repositories: help-docs is good, but no permission-* inputs are supplied, so the minted token carries every permission granted to the App installation. repository_dispatch needs only Contents: write on the target.
| repositories: help-docs | |
| repositories: help-docs | |
| permission-contents: write |
| on: | ||
| push: | ||
| branches: [main] | ||
| paths: ["docs/docs/**"] |
There was a problem hiding this comment.
MINOR — path filtering is bounded on very large pushes
GitHub evaluates paths: filters against a bounded changed-file list (3,000 files). A push whose diff exceeds that cap may not start this workflow even though docs/docs/** changed.
Acceptable if very large pushes are operationally prohibited; otherwise trigger on every push to main and detect docs changes inside the job.
Separately: the docs/docs/** scope itself is correct and deliberate — it matches docs_path: docs/docs for the code: entry in help-docs tools/oss_sources.yml and excludes docs/internal/. Four reviewers flagged it as a mismatch with docs.yml's docs/**; that was rejected.
| owner: AltimateAI | ||
| repositories: help-docs | ||
|
|
||
| - uses: peter-evans/repository-dispatch@ff45666b9427631e3450c54a1bcbee4d9ff4d7c0 # v3 |
There was a problem hiding this comment.
MINOR — no failure signal beyond a red check
A failed dispatch surfaces only in the Actions tab. help-docs already ships tools/notify_slack.py; an if: failure() notify step here would close the loop on a sync that has silently stopped working.
Consensus review — nits and non-line-attributable findingsFollow-up to the inline review. Everything below either spans the whole file, concerns the review process itself, or is a nit. Nits
Missing test coverageNothing in this workflow can be exercised before it lands on
A Findings raised by individual models and rejectedRecorded so they don't get re-litigated on the next pass. Each was checked against the code before being dropped.
Where the panel disagreedTwo objections were raised during convergence and rejected, recorded here for transparency:
Objections that were incorporated, and changed the review:
Reviewed by 7 models: Claude Opus 5, GPT-5.4 Codex, Kimi K2.5, GLM-5.1, Qwen 3.6, MiniMax M2.7, MiMo V2 Pro. Convergence: 1 round — 2 APPROVE, 3 CHANGES NEEDED. |
…loop guard, config gate, drop concurrency, ubuntu-latest - CRITICAL: App ID is stored as a repo *secret*, not a variable — use secrets.DOCS_SYNC_APP_ID (vars.* resolved empty and failed every run). - loop guard checks every commit (toJSON(github.event.commits.*.message)) so the [docs-sync] marker survives merge-commit merges, not just squash/rebase. - add a config-check step: skip (notice) instead of red-failing when the App is not configured (forks, key rotation). - drop the concurrency group (a 3rd rapid push cancelled the pending run and dropped that range permanently); dispatch is cheap. - run on ubuntu-latest (arc-runner-gke is unverified on this repo and would queue with no signal; matches the repo\ s other jobs + dispatch-code-review). - add timeout-minutes + workflow_dispatch (manual re-fire / smoke test). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011fcN8kPwzjDeBysYUzKfJU
|
Thanks for the exceptionally thorough review — addressed in Blocking
Major
Minor
Rejected findingsAgreed with the panel's dispositions — no changes. The injection ( Follow-ups (noted, non-blocking)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/notify-help-docs.yml (1)
68-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the pinned GitHub App token action to
v2.
.github/workflows/notify-help-docs.ymlstill installs av1commit ofactions/create-github-app-token, while.github/actions/setup-git-committer/action.ymluses thev2release. Keep the pinned commit-SHA pin but align it with thev2major version so both authentication paths use the same supported action contract.🤖 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 @.github/workflows/notify-help-docs.yml at line 68, Update the actions/create-github-app-token step in the notify-help-docs workflow to a commit SHA corresponding to the v2 release, retaining SHA pinning and updating the version comment; do not change the surrounding authentication configuration.
🤖 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.
Inline comments:
In @.github/workflows/notify-help-docs.yml:
- Line 14: Update the workflow_dispatch configuration and its job guard to avoid
relying on unavailable push-event fields: either remove manual dispatch or
restrict it to main with required before and after inputs, inspect that explicit
range for [docs-sync], and pass those inputs as BEFORE and AFTER instead of
deriving them from github.event.
---
Nitpick comments:
In @.github/workflows/notify-help-docs.yml:
- Line 68: Update the actions/create-github-app-token step in the
notify-help-docs workflow to a commit SHA corresponding to the v2 release,
retaining SHA pinning and updating the version comment; do not change the
surrounding authentication configuration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b47a8b4-ea27-43b7-9e08-7ad79a6b7b68
📒 Files selected for processing (1)
.github/workflows/notify-help-docs.yml
| push: | ||
| branches: [main] | ||
| paths: ["docs/docs/**"] | ||
| workflow_dispatch: |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
workflow=".github/workflows/notify-help-docs.yml"
if rg -q '^ workflow_dispatch:$' "$workflow" &&
! rg -q 'inputs\.(before|after)' "$workflow"; then
echo "::error::workflow_dispatch has no explicit synchronization range"
exit 1
fi
rg -n -C 3 \
'workflow_dispatch|github\.event\.(before|commits)|github\.ref|BEFORE|AFTER|inputs\.(before|after)' \
"$workflow"Repository: AltimateAI/altimate-code
Length of output: 227
🏁 Script executed:
#!/bin/bash
set -euo pipefail
workflow=".github/workflows/notify-help-docs.yml"
echo "== workflow exists =="
git ls-files "$workflow" || true
echo "== excerpt =="
cat -n "$workflow"Repository: AltimateAI/altimate-code
Length of output: 4089
🌐 Web query:
GitHub Actions manually running workflow workflow_dispatch github.event before commits selected branch ref after commit manual run
💡 Result:
When you manually trigger a workflow using the workflow_dispatch event, you can specify a target branch or tag via the ref parameter [1]. Within the workflow run, the following properties provide information about the execution context: - github.ref: This context variable contains the fully-formed reference of the branch or tag that was used to trigger the workflow (e.g., refs/heads/main) [2]. - github.sha: This contains the commit SHA that the workflow is running against [2]. For a manual workflow_dispatch run, this corresponds to the HEAD commit of the ref you specified [3]. - github.event.ref: When triggered via workflow_dispatch, this property also contains the reference (e.g., refs/heads/main) that was targeted by the manual run [4]. To access the commit SHA specifically chosen for the run, you can rely on the standard github.sha context, which is automatically set to the commit at the tip of the chosen ref at the time of the trigger [3]. If you need to perform actions relative to a specific commit or branch, you can use the github.ref or github.event.ref context to identify the source [4]. If your workflow requires handling a commit other than the current HEAD of a branch (e.g., a specific SHA), the workflow_dispatch event natively supports specifying a ref, but it does not support passing a specific SHA directly as a "ref" to the workflow trigger itself in the same way it handles branches or tags [1]. If you must target a specific arbitrary SHA, a common workaround is to use custom inputs (e.g., input: sha) in your workflow_dispatch configuration and then use those inputs within your workflow to check out or interact with that specific commit [5][6].
Citations:
- 1: https://docs.github.com/en/actions/how-tos/manage-workflow-runs/manually-run-a-workflow?tool=webui
- 2: https://docs.github.com/en/actions/reference/workflows-and-actions/contexts
- 3: https://www.codestudy.net/blog/get-run-id-after-triggering-a-github-workflow-dispatch-event/
- 4: https://github.com/process-analytics/github-actions-playground/blob/master/.github/workflows/workflow-dispatch.yml
- 5: https://github.com/tenstorrent/pytorch2.0_ttnn/blob/7702784d91d5d1c84fa5656cacfa0ca29f59bd0a/.github/workflows/pull-request.yaml
- 6: https://github.com/vercel/repository-dispatch/blob/main/actions/status/src/utils/set-commit-status.ts
🏁 Script executed:
#!/bin/bash
set -euo pipefail
workflow=".github/workflows/notify-help-docs.yml"
if [ -f "$workflow" ]; then
echo "== workflow excerpt =="
cat -n "$workflow"
else
echo "Workflow file not found: $workflow"
echo "Matching notify-help-docs files:"
git ls-files | grep -F "notify-help-docs" || true
fiRepository: AltimateAI/altimate-code
Length of output: 4037
🌐 Web query:
GitHub push event github.event.commits workflow_dispatch before property not present manual runs
💡 Result:
The github.event.before property is not present in workflow runs triggered by the workflow_dispatch event because this event is manually triggered rather than being tied to a specific git commit history or push activity [1][2]. While events like push provide a commit-based history that includes before and after properties to represent changes [3], a workflow_dispatch event is an ad-hoc trigger that does not inherently know about previous states or commit deltas [1]. Consequently, if a GitHub Action (such as a path-filtering or diff-based tool) relies on github.event.before to determine what changed in the repository, it will fail or skip its logic because that information is missing in the manual event payload [1][2]. Common workarounds for this limitation include: - Removing strict dependency on base commit properties in your action configuration [1]. - Using Git commands within your workflow steps to manually calculate the relevant diff or SHA, such as finding the merge-base between the current branch and the default branch [1][2]. - Relying on GitHub-provided environment variables (like GITHUB_SHA) and branch references rather than event payload properties when performing comparisons [1].
Citations:
- 1: Changes not detected on a manual trigger (workflow_dispatch) dorny/paths-filter#235
- 2: https://core.trac.wordpress.org/ticket/61699
- 3: https://github.com/github/gh-aw/blob/main/.github/aw/context.md
Provide explicit sync ranges for workflow_dispatch.
workflow_dispatch can run from a selected ref, but it does not provide github.event.before or github.event.commits. The job guard checks github.event.commits.*.message, while lines 56-57 send an empty BEFORE and use only the selected HEAD as AFTER. Restrict manual runs to main, require before and after inputs, inspect that range for [docs-sync], then use those inputs for before and after; otherwise remove workflow_dispatch.
🤖 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 @.github/workflows/notify-help-docs.yml at line 14, Update the
workflow_dispatch configuration and its job guard to avoid relying on
unavailable push-event fields: either remove manual dispatch or restrict it to
main with required before and after inputs, inspect that explicit range for
[docs-sync], and pass those inputs as BEFORE and AFTER instead of deriving them
from github.event.
Source: MCP tools
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/notify-help-docs.yml">
<violation number="1" location=".github/workflows/notify-help-docs.yml:14">
P2: Manual re-fires dispatch an empty `before` revision, so the hub cannot form the source range it receives for normal promotions. Add an explicit base-SHA input (and use it for `BEFORE`) or prevent manual dispatch from sending a promotion.</violation>
<violation number="2" location=".github/workflows/notify-help-docs.yml:22">
P1: A mixed push permanently drops ordinary docs updates whenever its commit list also contains a `[docs-sync]` commit. Narrow the suppression to pushes that are wholly sync-generated, or otherwise dispatch the non-sync docs range.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| jobs: | ||
| notify: | ||
| if: ${{ !contains(toJSON(github.event.commits.*.message), '[docs-sync]') }} |
There was a problem hiding this comment.
P1: A mixed push permanently drops ordinary docs updates whenever its commit list also contains a [docs-sync] commit. Narrow the suppression to pushes that are wholly sync-generated, or otherwise dispatch the non-sync docs range.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/notify-help-docs.yml, line 22:
<comment>A mixed push permanently drops ordinary docs updates whenever its commit list also contains a `[docs-sync]` commit. Narrow the suppression to pushes that are wholly sync-generated, or otherwise dispatch the non-sync docs range.</comment>
<file context>
@@ -1,31 +1,44 @@
notify:
- if: ${{ !contains(github.event.head_commit.message, '[docs-sync]') }}
- runs-on: arc-runner-gke
+ if: ${{ !contains(toJSON(github.event.commits.*.message), '[docs-sync]') }}
+ runs-on: ubuntu-latest
+ timeout-minutes: 5
</file context>
| push: | ||
| branches: [main] | ||
| paths: ["docs/docs/**"] | ||
| workflow_dispatch: |
There was a problem hiding this comment.
P2: Manual re-fires dispatch an empty before revision, so the hub cannot form the source range it receives for normal promotions. Add an explicit base-SHA input (and use it for BEFORE) or prevent manual dispatch from sending a promotion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/notify-help-docs.yml, line 14:
<comment>Manual re-fires dispatch an empty `before` revision, so the hub cannot form the source range it receives for normal promotions. Add an explicit base-SHA input (and use it for `BEFORE`) or prevent manual dispatch from sending a promotion.</comment>
<file context>
@@ -1,31 +1,44 @@
push:
branches: [main]
paths: ["docs/docs/**"]
+ workflow_dispatch:
permissions:
</file context>
2 participants
Adds a tiny workflow that, when a PR touching docs merges to the default branch, notifies the help-docs hub (
repository_dispatch) so the change is promoted up into help-docs as a review PR.Part of the bidirectional docs-sync (help-docs is the source-of-truth hub). See
tools/DOCS_SYNC.mdin help-docs.Requires the shared docs-sync GitHub App and these Actions settings on this repo (or org-level):
DOCS_SYNC_APP_IDDOCS_SYNC_APP_PRIVATE_KEYInert until those are set. Sync-generated merges (marker
[docs-sync]) are skipped, so it never loops.🤖 Generated with Claude Code
Summary by cubic
Add a workflow that notifies the
help-docshub when docs change onmain, promoting a review PR. It sends before/after SHAs and source PR info; skips[docs-sync]commits (checks all commit messages); adds a config gate; uses pinned actions, a jq-built payload,ubuntu-latest, a short timeout, and supports manualworkflow_dispatch.DOCS_SYNC_APP_IDandDOCS_SYNC_APP_PRIVATE_KEYas repo secrets.Written for commit cba8741. Summary will update on new commits.
Summary by CodeRabbit