Skip to content

chore: restore notify-parent workflow (simplified) - #24

Merged
mateodelnorte merged 1 commit into
mainfrom
fix/restore-notify-parent
Mar 26, 2026
Merged

mateodelnorte merged 1 commit into
mainfrom
fix/restore-notify-parent

Conversation

@mateodelnorte

@mateodelnorte mateodelnorte commented Mar 26, 2026 •

Copy link
Copy Markdown
Contributor

Re-adds notify-parent.yml to trigger parent meta repo's release-please when this child repo merges to main.

Simplified: no commit message parsing, just dispatches repo name and SHA. Companion to gitkb/meta#62.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Chores
    • Added an automated workflow that notifies the parent repository whenever the main branch receives updates, enabling upstream awareness of code changes.

@coderabbitai

coderabbitai Bot commented Mar 26, 2026 •

Copy link
Copy Markdown

Warning

Rate limit exceeded

@mateodelnorte has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 14 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 44de1159-0fcf-4e9e-a26a-6c53adfe9a25

📥 Commits

Reviewing files that changed from the base of the PR and between bce7049 and b793416.

📒 Files selected for processing (1)
  • .github/workflows/notify-parent.yml

Walkthrough

Adds a GitHub Actions workflow that triggers on pushes to the main branch and sends a repository_dispatch event to the harmony-labs/meta repository with repo name, commit SHA, and actor in the payload.

Changes

Cohort / File(s) Summary
GitHub Actions Workflow
\.github/workflows/notify-parent.yml
New workflow added that runs on push to main and uses peter-evans/repository-dispatch@v4 to send a child-repo-updated repository_dispatch to the harmony-labs/meta repo, passing repo_name, sha, and actor via client-payload authenticated with secrets.PARENT_REPO_PAT.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Dev as Dev (push)
    participant GH as GitHub Actions
    participant Action as peter-evans/action
    participant API as GitHub API
    participant Parent as Parent Repo (harmony-labs/meta)

    Dev->>GH: push to main
    GH->>Action: run repository-dispatch step (with payload)
    Action->>API: POST /repos/harmony-labs/meta/dispatches (event + client-payload)
    API->>Parent: deliver repository_dispatch event
    Parent-->>API: acknowledge
    API-->>Action: response
    Action-->>GH: step result
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Poem

🐇 I hopped a push across the wire,

Sent a ping so updates never tire,
Parent heard my tiny thump,
A quiet clap — automation's bump,
I nibble bytes and dance — repose inspired.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: re-adding a simplified notify-parent workflow file as a GitHub Actions configuration.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/restore-notify-parent

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps

greptile-apps Bot commented Mar 26, 2026

Copy link
Copy Markdown

Greptile Summary

This PR restores the notify-parent.yml GitHub Actions workflow that dispatches a repository_dispatch event to harmony-labs/meta whenever this child repo merges to main, and simultaneously replaces the expand_meta_children function in src/commands/worktree/create.rs with the more surgical ensure_intermediate_parents.\n\nKey changes:\n- notify-parent.yml: Triggers peter-evans/repository-dispatch on pushes to main, sending repo_name, short_sha, and actor to the parent meta repo. The payload JSON is valid (YAML >- folding produces a single-line JSON string), but the short_sha field actually carries the full 40-char SHA from github.sha, and the checkout step is unused.\n- ensure_intermediate_parents: Narrows the scope of what gets auto-included in a worktree: instead of eagerly expanding every child of any meta: true repo, it now only adds the minimum set of ancestor repos needed so that git worktree add can succeed for a deeply-nested alias like \"gitkb/core\". The output list is correctly ordered — \".\" first, then ancestors sorted alphabetically, then the originally requested repos.

Confidence Score: 4/5

Safe to merge after clarifying the short_sha field value vs. what gitkb/meta#62 expects.

Both changes are well-scoped. The Rust refactor is logically sound and correctly handles ordering, deduplication, and parent-before-child creation. The workflow has two minor style issues but neither breaks functionality unless the parent explicitly expects a 7-char SHA.

notify-parent.yml — verify that gitkb/meta#62 tolerates a full SHA in the short_sha field, or rename/recompute accordingly.

Important Files Changed

Filename Overview
.github/workflows/notify-parent.yml New workflow dispatching a repository event to harmony-labs/meta on every push to main; minor issues: unnecessary checkout step and a misleading short_sha field name that actually carries the full 40-char SHA.
src/commands/worktree/create.rs Replaces expand_meta_children with ensure_intermediate_parents; logic is correct — alphabetical sort guarantees parents precede children, and deduplication against both existing and added sets is properly handled.

Sequence Diagram

sequenceDiagram
    participant Push as Push to main
    participant CI as notify-parent.yml
    participant GH as GitHub API
    participant Meta as harmony-labs/meta
    participant RP as release-please workflow

    Push->>CI: Trigger on push to main
    CI->>GH: repository-dispatch (PARENT_REPO_PAT)<br/>event: child-repo-updated<br/>payload: {repo_name, short_sha, actor}
    GH->>Meta: Deliver repository_dispatch event
    Meta->>RP: Trigger release-please workflow
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: .github/workflows/notify-parent.yml
Line: 23

Comment:
**`short_sha` field contains the full SHA**

`${{ github.sha }}` expands to the full 40-character commit SHA, not a short 7-character one. The field name `short_sha` is misleading and could cause issues if the consumer in `harmony-labs/meta#62` is written to expect the abbreviated form. Either rename the field to `sha`, or add a step to compute the real short SHA:

```yaml
      - name: Compute short SHA
        id: sha
        run: echo "short_sha=$(git rev-parse --short HEAD)" >> "$GITHUB_OUTPUT"
```

Then reference it as `"${{ steps.sha.outputs.short_sha }}"`.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: .github/workflows/notify-parent.yml
Line: 11-12

Comment:
**Unnecessary `checkout` step**

The `peter-evans/repository-dispatch` action only makes an authenticated HTTP call to the GitHub API — it never reads local repository content. Checking out the code adds ~10-20 s of latency with no benefit and can be removed entirely.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "chore: restore notify-parent workflow (s..." | Re-trigger Greptile

Comment thread .github/workflows/notify-parent.yml Outdated
client-payload: >-
{
"repo_name": ${{ toJSON(github.event.repository.name) }},
"short_sha": "${{ github.sha }}",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 short_sha field contains the full SHA

${{ github.sha }} expands to the full 40-character commit SHA, not a short 7-character one. The field name short_sha is misleading and could cause issues if the consumer in harmony-labs/meta#62 is written to expect the abbreviated form. Either rename the field to sha, or add a step to compute the real short SHA:

      - name: Compute short SHA
        id: sha
        run: echo "short_sha=$(git rev-parse --short HEAD)" >> "$GITHUB_OUTPUT"

Then reference it as "${{ steps.sha.outputs.short_sha }}".

Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/notify-parent.yml
Line: 23

Comment:
**`short_sha` field contains the full SHA**

`${{ github.sha }}` expands to the full 40-character commit SHA, not a short 7-character one. The field name `short_sha` is misleading and could cause issues if the consumer in `harmony-labs/meta#62` is written to expect the abbreviated form. Either rename the field to `sha`, or add a step to compute the real short SHA:

```yaml
      - name: Compute short SHA
        id: sha
        run: echo "short_sha=$(git rev-parse --short HEAD)" >> "$GITHUB_OUTPUT"
```

Then reference it as `"${{ steps.sha.outputs.short_sha }}"`.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in af351ea.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — renamed short_sha to sha (sender sends full SHA, parent truncates to 7 chars), removed checkout step, added permissions: {} block.

Comment thread .github/workflows/notify-parent.yml Outdated
Comment on lines +11 to +12
- name: Checkout
uses: actions/checkout@v4

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Unnecessary checkout step

The peter-evans/repository-dispatch action only makes an authenticated HTTP call to the GitHub API — it never reads local repository content. Checking out the code adds ~10-20 s of latency with no benefit and can be removed entirely.

Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/notify-parent.yml
Line: 11-12

Comment:
**Unnecessary `checkout` step**

The `peter-evans/repository-dispatch` action only makes an authenticated HTTP call to the GitHub API — it never reads local repository content. Checking out the code adds ~10-20 s of latency with no benefit and can be removed entirely.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in af351ea.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — renamed short_sha to sha (sender sends full SHA, parent truncates to 7 chars), removed checkout step, added permissions: {} block.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/notify-parent.yml:
- Around line 7-10: Add an explicit least-privilege permissions block for the
workflow (instead of relying on defaults) so the notify job uses a scoped
GITHUB_TOKEN; add a top-level permissions: map (or a job-level permissions:
under the notify job) specifying only the required scopes (for example, limit to
contents: read, issues: write, checks: write, or whichever minimal scopes the
notify job's steps need) and remove broad or default permissions to harden the
notify job and its steps.
- Line 23: The workflow currently assigns the full commit SHA (${ { github.sha }
}) to the key short_sha which mismatches the name; either rename the key
short_sha to commit_sha (to keep the full SHA) or compute a true short SHA in a
prior step and use that output for short_sha (e.g., create a step that uses the
GITHUB_SHA substring ${GITHUB_SHA::7} and exposes it via an output, then
reference that output instead of ${ { github.sha } }). Ensure you update the
reference to the unique symbol "short_sha" in the workflow and/or add the new
step that emits the short SHA.
- Around line 3-5: Replace the broad push trigger under on: with a pull_request
trigger scoped to closed PRs targeting main (use on: pull_request with types:
[closed] and branches: [main]) and add an explicit merged check (if:
github.event.pull_request.merged == true) to the workflow's jobs so the workflow
only runs when a PR is merged into main; update the existing on: and add the
job-level if condition in notify-parent.yml accordingly.

In `@src/commands/worktree/create.rs`:
- Around line 570-573: Add a debug/info log when an intermediate parent
directory is skipped due to a missing .git: inside the branch that checks
parent_source.join(".git").exists() (the block that currently does continue),
emit a concise debug-level message that includes the parent_source path and note
that it was skipped because .git was not present, using the crate's existing
logging facility (e.g., tracing::debug! or the project's logger) so maintainers
can see why a parent worktree was not created.
- Around line 564-568: The existence check for intermediate parents uses
meta_dir.join(&parent_alias) which ignores .meta path mappings; replace that
logic by calling lookup_nested_project(&meta_dir, &parent_alias) (or the
appropriate signature used elsewhere) to resolve parent_alias to its actual
source path, assign its result to parent_source, and only continue if
lookup_nested_project returns None; ensure parent_source's type and error
handling match surrounding code (e.g., handle Option/Result the same way other
uses at lines 121/431 do).
🪄 Autofix (Beta)

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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: bf0a7c10-985c-49fa-a186-ecd59a9480c7

📥 Commits

Reviewing files that changed from the base of the PR and between f694a31 and 7ba7a37.

📒 Files selected for processing (2)
  • .github/workflows/notify-parent.yml
  • src/commands/worktree/create.rs

Comment on lines +3 to +5
on:
push:
branches: [main]

@coderabbitai coderabbitai Bot Mar 26, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n .github/workflows/notify-parent.yml

Repository: harmony-labs/meta_git_cli

Length of output: 868


🏁 Script executed:

# Check for branch protection configuration
git ls-files | grep -E "(branch|protection|rules)" | head -20

Repository: harmony-labs/meta_git_cli

Length of output: 51


🏁 Script executed:

# Look for any references to the parent repo or related documentation
rg "harmony-labs/meta|parent.?repo|PARENT_REPO" -i --max-count=20

Repository: harmony-labs/meta_git_cli

Length of output: 184


🏁 Script executed:

# Check if there are other workflows that might show the intended pattern
fd "\.yml$|\.yaml$" .github/workflows/ --type f

Repository: harmony-labs/meta_git_cli

Length of output: 163


🏁 Script executed:

# Look for any documentation about the workflow intent
fd "README|CONTRIBUTING|docs" -type f | head -10

Repository: harmony-labs/meta_git_cli

Length of output: 239


🏁 Script executed:

# Check the other workflows for comparison
cat -n .github/workflows/ci.yml | head -30

Repository: harmony-labs/meta_git_cli

Length of output: 793


🏁 Script executed:

# Check for README documentation
cat README.md 2>/dev/null | head -50

Repository: harmony-labs/meta_git_cli

Length of output: 51


🏁 Script executed:

# Look at git history for when notify-parent was added
git log --oneline .github/workflows/notify-parent.yml 2>/dev/null | head -5

Repository: harmony-labs/meta_git_cli

Length of output: 127


🏁 Script executed:

# Search for any comments or context about the workflow intent
git log -p --reverse .github/workflows/notify-parent.yml 2>/dev/null | head -100

Repository: harmony-labs/meta_git_cli

Length of output: 51


🏁 Script executed:

# Check if there's a branch protection config or CODEOWNERS
git ls-files | grep -iE "(codeowners|branch|protection)"

Repository: harmony-labs/meta_git_cli

Length of output: 51


🏁 Script executed:

# Search for any GitHub organization/settings files
find . -name ".github" -type d -exec ls -la {} \;

Repository: harmony-labs/meta_git_cli

Length of output: 235


🏁 Script executed:

# Look for any issues or discussions about this workflow
git log --all --grep="notify-parent" -i --oneline 2>/dev/null

Repository: harmony-labs/meta_git_cli

Length of output: 316


🏁 Script executed:

# Check the full commit that restored the workflow
git show 7ba7a37 2>/dev/null

Repository: harmony-labs/meta_git_cli

Length of output: 50382


🏁 Script executed:

# Check the previous version of notify-parent to understand what changed
git show 2b2ab14:.github/workflows/notify-parent.yml 2>/dev/null | head -50

Repository: harmony-labs/meta_git_cli

Length of output: 51


🏁 Script executed:

# Check if there's any GitHub organization-level config
ls -la .github/

Repository: harmony-labs/meta_git_cli

Length of output: 235


🏁 Script executed:

# Look for any references to branch protection or main branch policy
rg "branch|protection|main" -i .github/ -l

Repository: harmony-labs/meta_git_cli

Length of output: 163


🏁 Script executed:

# Check the parent repo to understand what it expects
# (if accessible, though likely a private repo)
rg "child-repo-updated|notify-parent" -i 2>/dev/null | head -10

Repository: harmony-labs/meta_git_cli

Length of output: 51


Align trigger with intent: only notify on merged PRs, not all pushes to main.

Commit message states this should trigger "when this child repo merges to main", but the current push trigger fires on every push, including direct pushes if branch protection allows them. This risks unintended parent release notifications.

🔧 Suggested fix
-on:
-  push:
-    branches: [main]
+on:
+  pull_request:
+    types: [closed]
+    branches: [main]

 jobs:
   notify:
     runs-on: ubuntu-latest
+    if: ${{ github.event.pull_request.merged == true }}
     steps:
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
on:
push:
branches: [main]
on:
pull_request:
types: [closed]
branches: [main]
jobs:
notify:
runs-on: ubuntu-latest
if: ${{ github.event.pull_request.merged == true }}
steps:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/notify-parent.yml around lines 3 - 5, Replace the broad
push trigger under on: with a pull_request trigger scoped to closed PRs
targeting main (use on: pull_request with types: [closed] and branches: [main])
and add an explicit merged check (if: github.event.pull_request.merged == true)
to the workflow's jobs so the workflow only runs when a PR is merged into main;
update the existing on: and add the job-level if condition in notify-parent.yml
accordingly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in af351ea.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
git show af351ea -- .github/workflows/notify-parent.yml 2>/dev/null

Length of output: 1347


🏁 Script executed:

#!/bin/bash
# Check the current state of the file on the PR branch
cat -n .github/workflows/notify-parent.yml

# Also check if there are any commits after af351ea on this branch
git log --oneline -5

Length of output: 1057


@mateodelnorte, it looks like commit af351ea made some improvements (removed the checkout step and fixed toJSON quoting), but the push trigger is still in place — the file currently reads:

on:
  push:
    branches: [main]

The suggested change to scope it to merged PRs only (using pull_request: types: [closed] with if: github.event.pull_request.merged == true) wasn't applied. If you'd like to address this concern, here's the fix:

on:
  pull_request:
    types: [closed]
    branches: [main]

jobs:
  notify:
    runs-on: ubuntu-latest
    if: ${{ github.event.pull_request.merged == true }}
    steps:
      - name: Notify parent repo
        uses: peter-evans/repository-dispatch@v4
        with:
          token: ${{ secrets.PARENT_REPO_PAT }}
          repository: harmony-labs/meta
          event-type: child-repo-updated
          client-payload: >-
            {
              "repo_name": ${{ toJSON(github.event.pull_request.head.repo.name) }},
              "short_sha": ${{ toJSON(github.event.pull_request.merge_commit_sha) }},
              "actor": ${{ toJSON(github.actor) }}
            }

Note that with the pull_request trigger, github.sha and github.event.repository.name still work, but github.event.pull_request.merge_commit_sha gives you the actual merge commit SHA which is more precise for the merged PR case.

If you intentionally want to keep the push trigger (e.g., because direct pushes to main should also notify the parent), please let me know and I'll dismiss this comment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — renamed short_sha to sha (sender sends full SHA, parent truncates to 7 chars), removed checkout step, added permissions: {} block.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
cat -n .github/workflows/notify-parent.yml
git log --oneline -5

Length of output: 1090


@mateodelnorte, confirmed — permissions: {}, the sha rename, and the removed checkout step are all in place. Thanks for those improvements!

However, the original concern is still present: the trigger remains push on main (Lines 3–5), not scoped to merged PRs only. If direct pushes to main are prevented by branch protection, this is functionally equivalent — but if branch protection is not enforced, direct pushes would also fire the parent notification.

Could you confirm whether this is intentional (relying on branch protection to guarantee only merges reach main)? If so, I'm happy to mark this resolved.

Comment thread .github/workflows/notify-parent.yml
Comment thread .github/workflows/notify-parent.yml Outdated
Comment thread src/commands/worktree/create.rs Outdated
Comment thread src/commands/worktree/create.rs Outdated
@mateodelnorte
mateodelnorte force-pushed the fix/restore-notify-parent branch from 7ba7a37 to af351ea Compare March 26, 2026 12:36

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/commands/worktree/create.rs`:
- Around line 527-535: The docstring claims intermediate parents are created
only for `meta: true` repos, but the fallback branch that creates parent
worktrees doesn't verify the project's meta flag; update the code to enforce
that behavior by querying the project via lookup_nested_project (used elsewhere)
and checking its `.meta` (or equivalent) boolean before treating a parent as
eligible, or if you prefer simpler change, amend the docstring to remove the
`meta: true` claim so it matches the current behavior of
repos_to_create/fallback. Ensure you reference the same lookup_nested_project
path and only proceed with worktree creation when the project's meta flag is
true.
🪄 Autofix (Beta)

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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: d141e754-f582-4166-842c-1240878f0c3a

📥 Commits

Reviewing files that changed from the base of the PR and between 7ba7a37 and af351ea.

📒 Files selected for processing (2)
  • .github/workflows/notify-parent.yml
  • src/commands/worktree/create.rs

Comment thread src/commands/worktree/create.rs
@mateodelnorte
mateodelnorte force-pushed the fix/restore-notify-parent branch from af351ea to 63d775b Compare March 26, 2026 12:43

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (3)
.github/workflows/notify-parent.yml (3)

3-5: ⚠️ Potential issue | 🟠 Major

Still using broad push trigger instead of merged PR trigger.

Per past review discussion, the PR description states this should trigger "when this child repo merges to main", but the push trigger fires on every push to main (including direct pushes if branch protection allows). Despite earlier discussion, this hasn't been changed to scope it to merged PRs only.

If you want to restrict to merged PRs as originally intended, apply:

🔧 Suggested fix
 on:
-  push:
+  pull_request:
+    types: [closed]
     branches: [main]

 jobs:
   notify:
     runs-on: ubuntu-latest
+    if: ${{ github.event.pull_request.merged == true }}
     steps:

Note: With pull_request trigger, use github.event.pull_request.merge_commit_sha instead of github.sha in the payload.

If direct pushes to main should also notify the parent, the current trigger is correct and this comment can be dismissed.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/notify-parent.yml around lines 3 - 5, The workflow
currently uses a broad "push" trigger (the on: push block) which fires for any
push to main; to restrict notifications to only merged PRs change the trigger to
use the pull_request event (e.g., on: pull_request with types: [closed]) and add
a guard that only proceeds when github.event.pull_request.merged == true, and
update any payload reference that used github.sha to use
github.event.pull_request.merge_commit_sha instead; modify the trigger and the
job conditionals in notify-parent.yml accordingly so only merged PRs to main
cause the notification.

7-9: ⚠️ Potential issue | 🟠 Major

Missing explicit least-privilege permissions block.

Despite earlier discussion about adding explicit permissions, the current code still lacks a permissions: block. Without it, the workflow relies on default GITHUB_TOKEN permissions which may be broader than necessary.

🔒 Suggested hardening
 jobs:
   notify:
+    permissions:
+      contents: read
     runs-on: ubuntu-latest

Even though this workflow uses a custom PAT for the dispatch action, explicitly scoping the default GITHUB_TOKEN is a security best practice.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/notify-parent.yml around lines 7 - 9, Add an explicit
permissions: block to the workflow (top-level or for the notify job) to scope
the default GITHUB_TOKEN to least privilege instead of relying on defaults;
update the workflow containing the notify job to include a permissions block
that only grants the exact scopes needed (for example: permissions: contents:
read and actions: read) so the notify job and any steps using GITHUB_TOKEN are
limited, and ensure the notify job references the GITHUB_TOKEN with the newly
scoped permissions.

17-22: ⚠️ Potential issue | 🟡 Minor

Misleading short_sha key name carries full SHA.

Line 20 assigns the full 40-character commit SHA (github.sha) to a key named short_sha. While toJSON() provides proper escaping, the naming mismatch could mislead downstream consumers in harmony-labs/meta who might expect a 7-character identifier.

🛠️ Two options to fix

Option 1: Rename to reflect actual content

-              "short_sha": ${{ toJSON(github.sha) }},
+              "commit_sha": ${{ toJSON(github.sha) }},

Option 2: Compute actual short SHA

     steps:
+      - name: Compute short SHA
+        id: vars
+        run: echo "short_sha=${GITHUB_SHA:0:7}" >> "$GITHUB_OUTPUT"
+
       - name: Notify parent repo
         uses: peter-evans/repository-dispatch@v4
         with:
           ...
           client-payload: >-
             {
               "repo_name": ${{ toJSON(github.event.repository.name) }},
-              "short_sha": ${{ toJSON(github.sha) }},
+              "short_sha": ${{ toJSON(steps.vars.outputs.short_sha) }},
               "actor": ${{ toJSON(github.actor) }}
             }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/notify-parent.yml around lines 17 - 22, The payload key
"short_sha" currently contains the full commit SHA (github.sha); either rename
the key to accurately reflect its content (e.g., "full_sha") in the
client-payload JSON, or compute a true short SHA and reference that instead: add
a step that derives a 7-char short SHA (e.g., using GITHUB_SHA substring and
set-output or outputs in a step id) and then use that step's output
(steps.<id>.outputs.short_sha) in the client-payload; update references to
"short_sha" or "full_sha" accordingly so downstream consumers see the correct
name/value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/notify-parent.yml:
- Around line 11-16: Add documentation for the PARENT_REPO_PAT used in
.github/workflows/notify-parent.yml: state that the secret named PARENT_REPO_PAT
is required, must be stored in the repository Secrets (Settings → Secrets), and
must be a PAT with permission to dispatch events to the target repo
(harmony-labs/meta) — typically repo scope or at least contents:write and
workflow/dispatch privileges for the target repository; note it must be created
by an account with access to harmony-labs/meta. Update either the repository
README with a short “CI secrets” section describing these requirements and how
to add the secret, and/or add an inline comment above the uses:
peter-evans/repository-dispatch@v4 step documenting the same details.

---

Duplicate comments:
In @.github/workflows/notify-parent.yml:
- Around line 3-5: The workflow currently uses a broad "push" trigger (the on:
push block) which fires for any push to main; to restrict notifications to only
merged PRs change the trigger to use the pull_request event (e.g., on:
pull_request with types: [closed]) and add a guard that only proceeds when
github.event.pull_request.merged == true, and update any payload reference that
used github.sha to use github.event.pull_request.merge_commit_sha instead;
modify the trigger and the job conditionals in notify-parent.yml accordingly so
only merged PRs to main cause the notification.
- Around line 7-9: Add an explicit permissions: block to the workflow (top-level
or for the notify job) to scope the default GITHUB_TOKEN to least privilege
instead of relying on defaults; update the workflow containing the notify job to
include a permissions block that only grants the exact scopes needed (for
example: permissions: contents: read and actions: read) so the notify job and
any steps using GITHUB_TOKEN are limited, and ensure the notify job references
the GITHUB_TOKEN with the newly scoped permissions.
- Around line 17-22: The payload key "short_sha" currently contains the full
commit SHA (github.sha); either rename the key to accurately reflect its content
(e.g., "full_sha") in the client-payload JSON, or compute a true short SHA and
reference that instead: add a step that derives a 7-char short SHA (e.g., using
GITHUB_SHA substring and set-output or outputs in a step id) and then use that
step's output (steps.<id>.outputs.short_sha) in the client-payload; update
references to "short_sha" or "full_sha" accordingly so downstream consumers see
the correct name/value.
🪄 Autofix (Beta)

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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9345b164-6998-4f0a-bf35-bb73607478af

📥 Commits

Reviewing files that changed from the base of the PR and between af351ea and 63d775b.

📒 Files selected for processing (1)
  • .github/workflows/notify-parent.yml

Comment thread .github/workflows/notify-parent.yml
@mateodelnorte
mateodelnorte force-pushed the fix/restore-notify-parent branch from 63d775b to bce7049 Compare March 26, 2026 13:07

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/notify-parent.yml:
- Around line 14-16: Document the required secret used by the workflow: add a
note (either as an inline comment in the .github/workflows/notify-parent.yml
near the uses: peter-evans/repository-dispatch@v4 block or as a README section)
that secrets.PARENT_REPO_PAT must be created in Repository Settings → Secrets
and variables → Actions, must be a Personal Access Token with appropriate
permissions (e.g., repo scope or a fine‑grained token with contents:write for
harmony-labs/meta), and the PAT owner must have write access to the
harmony-labs/meta repository; include an example description for the secret name
PARENT_REPO_PAT and the minimum permission requirements and where to configure
it.
🪄 Autofix (Beta)

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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2e725464-426e-4b60-bd3f-529694c0c817

📥 Commits

Reviewing files that changed from the base of the PR and between 63d775b and bce7049.

📒 Files selected for processing (1)
  • .github/workflows/notify-parent.yml

Comment thread .github/workflows/notify-parent.yml Outdated
Re-adds notify-parent.yml to trigger parent meta repo's release-please
when this child repo merges to main. Simplified: no checkout needed,
all payload fields properly quoted with toJSON.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mateodelnorte
mateodelnorte force-pushed the fix/restore-notify-parent branch from bce7049 to b793416 Compare March 26, 2026 13:21
@mateodelnorte
mateodelnorte merged commit 66ccd5a into main Mar 26, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant