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>
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Consensus Code Review — round 2 (7-model panel)
Verdict: REQUEST CHANGES · Critical 0 · Major 4 · Minor 7 · Nits 2
Reviewed independently by Claude Opus 5, GPT-5.4 Codex, Gemini 3.1 Pro, GLM-5.1, Qwen 3.6, MiniMax M2.7, MiMo V2 Pro, then converged over one agreement round (5 APPROVE, 1 CHANGES NEEDED — all three objections incorporated).
Round 1's CRITICAL and the two structural MAJORs are genuinely fixed, and the fixes match what was recommended rather than approximating it. What remains shares one shape: the workflow now reports success in several situations where nothing is promoted.
Major and correctness findings are posted inline. Everything else is below.
Resolution of the 18 round-1 findings
| # | Round-1 finding | Status |
|---|---|---|
| 1 | vars.DOCS_SYNC_APP_ID (CRITICAL) |
FIXED — line 72; GET /actions/variables still returns total_count: 0 |
| 2 | no guard for absent App credentials | FIXED — gate at 26–37, all four downstream steps gated |
| 3 | loop guard defeated by merge commit | FIXED — line 22, plus the marker now rides in the down-sync PR title in help-docs #43 |
| 4 | arc-runner-gke unverified |
FIXED — ubuntu-latest |
| 5 | consumer absent from help-docs default branch | NOT FIXED — see inline |
| 6 | concurrency pending-run eviction | RELOCATED — see inline |
| 7 | no timeout-minutes |
FIXED — line 24 |
| 8 | gh api … .[0] // {} line |
NOT FIXED — line 45 byte-identical |
| 9 | before zero-SHA / force-push |
NOT FIXED here; degraded to a silent skip at the hub |
| 10 | token inherits all installation permissions | NOT FIXED |
| 11 | direct push → empty attribution | NOT FIXED |
| 12 | paths: bounded changed-file list |
NOT FIXED (accepted) |
| 13 | no failure signal | NOT FIXED |
| 14 | inaccurate header comment | FIXED — lines 3–8 rewritten |
| 15 | create-github-app-token v1 vs v2 |
NOT FIXED |
| 16 | no workflow_dispatch |
ADDED, NON-FUNCTIONAL — see inline |
| 17 | github.ref in concurrency group |
MOOT — concurrency removed |
| 18 | heredoc form for $GITHUB_OUTPUT |
NOT A DEFECT — re-rejected |
6 fixed · 1 moot · 1 re-rejected · 1 added-but-broken · 1 relocated · 8 untouched.
Remaining findings not posted inline
MINOR — the PR description still documents the bug that was fixed. The body still reads "Requires … variable DOCS_SYNC_APP_ID" — the exact misconfiguration round-1 finding 1 was about — and still claims "so it never loops" without scoping that to push. Anyone provisioning from the description will create a variable, the config gate will correctly skip, and they will have no idea why.
NIT — workflow_dispatch has no inputs:, so an operator gets no controls at all. Contrast the hub's own workflow_dispatch, which offers direction and product. Resolves itself if the manual-trigger finding is fixed with inputs.
Raised and rejected
| Claim | Why rejected |
|---|---|
Empty before makes the hub run git diff ""..sha, which git resolves as HEAD |
Mechanism is backwards. [ -n "$BEFORE" ] fails first, so git diff never runs; the outcome is run=false → silent skip, not a wrong diff. |
Round-1 nit 18 (heredoc $GITHUB_OUTPUT) |
Re-rejected. All three values are single-line scalars and jq -c emits single-line JSON. |
The vacuous loop guard on workflow_dispatch is a defect |
Intentional — a human-requested retry should bypass the marker guard, and being triggerable via the dispatch API does not itself create a loop. The clarity fix is folded into the job-level if: below. |
Restoring concurrency on the sender partially fixes the dropped-range bug |
It reintroduces the identical one-pending-run eviction one hop earlier. Not a fix. |
Hardcoded product: code is a MAJOR contract risk |
paths: ["docs/docs/**"] can only ever mean the code product, and the hub's case maps it correctly. |
The push payload's commits array is capped, so the loop guard can miss the marker |
The array is capped, but every figure cited for the cap is far above any realistic sync PR (1–5 commits). Operationally irrelevant. |
The hub running on arc-runner-gke |
That is help-docs' own workflow — belongs to help-docs #43's review. |
Positive observations
- The three highest-severity round-1 findings are genuinely and correctly fixed. (all 7 reviewers)
- The config gate is done properly: gate step ungated, every subsequent step gated on its output,
::notice::rather than::warning::, so an unconfigured repo goes green and says why. Mirrorsdispatch-code-review.yml:44-48. - Moving the
[docs-sync]marker into the down-sync PR title in help-docs #43 is the stronger half of the loop fix — it survives all three merge strategies regardless of what this workflow inspects. Verified against this repo's live merge settings (merge_commit_message: PR_TITLE,squash_merge_commit_message: COMMIT_MESSAGES). - The header comment was rewritten to match actual behavior instead of being left stale.
ubuntu-latestmatches every other Linux job in the repo and removes an unverifiable runner-image dependency.- Cross-repo payload contract verified field-by-field by three reviewers independently: all six fields match the hub's reads at
sync-docs.yml:240-245. - Both actions still pinned to full commit SHAs; payload still built with
jq -cn --arg; untrusted values still routed throughenv:.
Missing tests
Nothing here can be exercised before it lands on main, and the one mechanism added for that purpose (workflow_dispatch) does not work.
workflow_dispatchfrommain— does the hub actually promote anything?workflow_dispatchfrom a non-mainbranch.- Three rapid docs pushes, now unserialized, against the hub's single concurrency group.
- A docs merge while help-docs #43 is unmerged — confirm "green" is understood to mean "did nothing".
- Force-push / recreated
main/ all-zerobefore. - Direct push to
mainwith no associated PR. - A commit associated with more than one PR;
ghAPI outage; auth failure. - App credentials absent, or only one of the two configured.
actionlintor any static validation of the workflow at all.
Minimum set before merge
- Merge help-docs #43 first, or land this behind an explicit prerequisite note.
- Fix or remove
workflow_dispatch— as written it is a green no-op — and restrict it tomainin the same change. - Fix the dropped-range risk at the hub, not here.
- One-liners worth taking while the file is open:
permission-contents: write, zero-SHA normalization,# v1→@v2, and correcting the PR description.
| push: | ||
| branches: [main] | ||
| paths: ["docs/docs/**"] | ||
| workflow_dispatch: |
There was a problem hiding this comment.
MAJOR — workflow_dispatch sends an empty range; the hub receives it and does nothing
Flagged by all 7 reviewers. This trigger was added to close round-1 nit 16 ("a failed promotion cannot be manually re-fired, and there is no way to smoke-test"). It can do neither.
The workflow_dispatch payload has no before field, so line 56 evaluates to "" and the dispatch goes out as:
{"product":"code","before":"","after":"<sha>","source_pr":"","source_author":"","source_url":""}At the hub, help-docs/.github/workflows/sync-docs.yml:299-312:
if [ -n "${{ steps.p.outputs.before }}" ] && \
git cat-file -e "${{ steps.p.outputs.before }}^{commit}" 2>/dev/null; then
CH="$(git diff --name-only …)"
else
CH=""
fi
…
if [ "${{ github.event_name }}" = "repository_dispatch" ] && [ -z "$CH" ]; then
echo "run=false" >> "$GITHUB_OUTPUT"The empty before fails the -n test first, so git cat-file and git diff never run: CH="" → run=false → the sync step, the PR step, the source-PR comment and the Slack notify are all skipped. Both workflows report success having promoted nothing.
Fix — make an empty range mean full reconciliation rather than nothing. Either add explicit inputs:
workflow_dispatch:
inputs:
before: { description: "Start SHA (blank = full reconcile)", required: false }
after: { description: "End SHA (blank = main HEAD)", required: false }or send a mode: full flag and teach the hub to sync the whole docs_path when it sees one. Either way the hub half is a merge prerequisite in help-docs #43 — this PR cannot fix it alone. The cheapest correct option is to drop workflow_dispatch here and use the hub's own workflow_dispatch (direction: up, product: code), which already works.
|
|
||
| jobs: | ||
| notify: | ||
| if: ${{ !contains(toJSON(github.event.commits.*.message), '[docs-sync]') }} |
There was a problem hiding this comment.
MAJOR (latent) — workflow_dispatch is not restricted to main; fixing the manual trigger activates this
on.push.branches: [main] constrains only the push trigger. workflow_dispatch can be fired from any branch, and when it is:
github.sha(line 57,AFTER) is that branch's head, not a commit onmain;- the
paths: ["docs/docs/**"]filter does not apply at all; - this loop guard is vacuous —
github.event.commitsdoes not exist.
The hub checks out the product repo at client_payload.after (sync-docs.yml:282), so the promoted content would come from an unmerged branch — violating the hub's premise that it only ever receives content already on the product's default branch.
Today this is inert: the empty before makes the hub set run=false, so it checks out the branch commit and then does nothing. That is the only thing preventing it — and the prescribed fix for the manual trigger (treat an empty range as a full reconciliation) activates it directly. Fix both in the same change:
if: >-
(github.event_name == 'workflow_dispatch' && github.ref == 'refs/heads/main') ||
(github.event_name == 'push' &&
!contains(toJSON(github.event.commits.*.message), '[docs-sync]'))This also makes the trigger/loop-guard interaction explicit instead of incidental.
| pull-requests: read | ||
|
|
||
| jobs: | ||
| notify: |
There was a problem hiding this comment.
MAJOR — deleting concurrency did not fix the dropped-range bug; it moved it to where it fails silently
Round-1 M5 was: GitHub holds one pending run per concurrency group, so three rapid docs pushes evict the middle one and its [before, after] range is never promoted. The concurrency block was deleted. But the hub has its own group covering exactly these runs (sync-docs.yml:58-60):
concurrency:
group: docs-sync-${{ github.event_name }}-${{ github.event.client_payload.product || inputs.product || 'push' }}
cancel-in-progress: falseEvery dispatch from this workflow lands on the single group docs-sync-repository_dispatch-code. Three rapid pushes now produce three near-simultaneous dispatches instead of three serialized ones, so the middle hub run is more likely to be evicted, not less. And the failure is quieter than before: no red check on altimate-code, and the surviving run's before starts after the lost range's after, so those files are simply never promoted.
Fix — this has to be fixed at the hub; there is no sender-side workaround:
queue: maxon the hub's concurrency group if the org's Actions plan supports it, which removes the eviction outright; or- have the hub diff against its own last-successfully-synced SHA (a marker commit or tag it maintains) instead of trusting the
beforeit was handed, so a lost dispatch is recovered by the next one; or - coalesce ranges at the hub.
Restoring concurrency here is not a fix — it reintroduces the identical one-pending-run eviction, just earlier in the chain.
| if: steps.config.outputs.enabled == 'true' | ||
| with: | ||
| token: ${{ steps.tok.outputs.token }} | ||
| repository: AltimateAI/help-docs |
There was a problem hiding this comment.
MAJOR — the consumer is still not on help-docs' default branch, and the credentials are configured, so this fires into the void
Flagged by all 7 reviewers. Carried from round 1, and now sharper rather than softer:
DOCS_SYNC_APP_IDandDOCS_SYNC_APP_PRIVATE_KEYare present as repo secrets, so the new config gate passes, the token mints, and the dispatch is sent.repository_dispatchhandlers must live on the target's default branch.sync-docs.ymlexists only on help-docs PR fix: address remaining code review issues from PR #39 #43, which is still open.POST /dispatchesreturns204whether or not anything is listening.
Merging this first produces a workflow that reports success on every docs merge to main while doing nothing — the worst outcome, because the green check reads as "synced."
Fix: merge help-docs #43 first (or simultaneously) and state the ordering in the PR description. A sender-side preflight — gh api repos/AltimateAI/help-docs/contents/.github/workflows/sync-docs.yml, failing loudly if absent — would make the dependency self-enforcing.
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| run: | | ||
| J="$(gh api repos/${{ github.repository }}/commits/${{ github.sha }}/pulls --jq '.[0] // {}' 2>/dev/null || echo '{}')" |
There was a problem hiding this comment.
MINOR — this line is unchanged in every respect
Flagged by 6 reviewers, and byte-identical to the previous revision:
- Errors are indistinguishable from "no PR."
2>/dev/null || echo '{}'means a missinggh, an auth failure and a rate-limit hit all produce a silent{}with nothing in the log. Direct pushes already return an empty array successfully, so swallowing every error is unnecessary. .[0]is arbitrary. A commit can belong to several PRs; nothing filters onmerged_atorbase.ref, so attribution can land on the wrong author.- No retry against an endpoint that is eventually consistent for seconds after a merge.
${{ }}interpolated into the shell while the very next step routes everything throughenv:. Both values are trusted so there's no injection here, but the split pattern invites the unsafe version to be copied for an untrusted field later.
Consequence at the hub: attribution lands on the wrong author, or on nobody.
J="$(gh api "repos/${GITHUB_REPOSITORY}/commits/${GITHUB_SHA}/pulls" \
--jq 'map(select(.merged_at != null and .base.ref == "main")) | sort_by(.merged_at) | last // {}')" \
|| { echo "::warning::could not resolve merged PR for ${GITHUB_SHA}"; J='{}'; }plus a short retry with backoff while J is {}.
| { | ||
| 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 still promote with no attribution at all
Flagged by 5 reviewers. Every push to main triggers this workflow, not only merges. A direct push yields empty num / login / url, and at the hub that silently disables both notification paths — the source-PR comment (sync-docs.yml:356) and the Slack notify (sync-docs.yml:364) are gated on src_pr != ''. The promotion PR opens with no source link and nobody is told.
Fall back to the pusher rather than gating the dispatch:
[ -z "$NUM" ] && LOGIN="${{ github.event.sender.login }}"Do not gate the dispatch on num != '' — that would suppress legitimate direct docs changes.
| app-id: ${{ secrets.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 minted App token still inherits every installation permission
Flagged by 6 reviewers. owner: AltimateAI / repositories: help-docs is good scoping, but with no permission-* inputs the token carries everything the App installation was granted — per help-docs tools/DOCS_SYNC.md that is Contents:write and PullRequests:write. Creating a repository dispatch needs Contents: write only.
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
Unchanged from round 1. GitHub evaluates paths: filters against a bounded changed-file list, so 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 — which would also give the workflow a natural place to handle the zero-SHA and force-push ranges.
| repositories: help-docs | ||
|
|
||
| - uses: peter-evans/repository-dispatch@ff45666b9427631e3450c54a1bcbee4d9ff4d7c0 # v3 | ||
| if: steps.config.outputs.enabled == 'true' |
There was a problem hiding this comment.
MINOR — no failure signal beyond a red check, and the revision widened the gap
Unchanged from round 1, but there are now three separate paths that converge on "green everywhere, docs never promoted":
- absent configuration → the config gate skips, run is green;
- missing receiver →
POST /dispatchesreturns 204, run is green; workflow_dispatch→ emptybefore, hub setsrun=false, both runs green.
A failed dispatch surfaces only in the Actions tab, and none of the three above surfaces anywhere at all. 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.
| '{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.
NIT — still pinned to create-github-app-token v1
.github/actions/setup-git-committer/action.yml:22 uses @v2. Pick one. Worth bumping here and adding permission-contents: write in the same edit.
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