Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions .github/workflows/merge-train.yml
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,25 @@ jobs:
# be signed. The bot can't sign, but it isn't adding commits
# here — these are the author's. Verify upfront so a quiet
# branch-protection rejection downstream doesn't confuse.
unsigned=$(git log --format='%H %G?' "origin/main..origin/${HEAD_REF}" | awk '$2!="G" && $2!="U" {print $1}' || true)
#
# Use GitHub's verification API rather than `git log --format=%G?`:
# %G? requires local `gpg.ssh.allowedSignersFile` to recognise
# SSH signatures, which actions/checkout does not set. GitHub's
# API is the same source of truth `required_signatures` branch
# protection uses downstream, so any commit it reports as
# verified will also pass the push gate.
unsigned=""
while IFS= read -r sha; do
[ -z "${sha}" ] && continue
verified=$(gh api "repos/${REPO}/commits/${sha}" \
--jq '.commit.verification.verified // false' 2>/dev/null || echo "false")
if [ "${verified}" != "true" ]; then
unsigned="${unsigned}${sha}"$'\n'
fi
done < <(git log --format='%H' "origin/main..origin/${HEAD_REF}")
Comment on lines +114 to +121

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

issue (bug_risk): Consider failure/availability handling for gh api and authentication

Currently, any gh api failure (network, auth, missing CLI, rate limits, etc.) is treated as verified=false, causing the merge train to fail and making outages indistinguishable from genuinely unsigned commits. To keep the check strict but clearer, consider: (a) explicitly checking for gh before the loop and failing with a targeted error, and/or (b) handling non-zero gh exit codes separately and emitting a distinct message like "could not verify signatures via GitHub API".

unsigned=${unsigned%$'\n'}
if [ -n "${unsigned}" ]; then
fail_and_unlabel "one or more commits between main and \`${HEAD_REF}\` are not GPG/SSH-signed:\n\n\`\`\`\n${unsigned}\n\`\`\`\n\nSign them locally and re-add the label. The bot cannot sign on your behalf."
fail_and_unlabel "one or more commits between main and \`${HEAD_REF}\` are not signed (per GitHub verification API):\n\n\`\`\`\n${unsigned}\n\`\`\`\n\nSign them locally and re-add the label. The bot cannot sign on your behalf."
fi

echo "[4/5] waiting for required checks to complete..."
Expand Down
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ installable release; see the roadmap in [README.md](README.md).

## [Unreleased]

### Fixed

- **`merge-train` signature check rejected every signed PR** ([#619](https://github.com/robotrocketscience/aelfrice/issues/619)). The check shipped in #602 used `git log --format='%G?'`, which only registers SSH signatures as `G` when the runner has `gpg.format=ssh` and `gpg.ssh.allowedSignersFile` configured — `actions/checkout` sets neither, so every SSH-signed commit fell through to GPG parsing and `%G?` returned `N`. Every PR labeled `ready-to-merge` since #602 shipped was unlabeled with a spurious "not signed" comment, and `main` only advanced via manual operator FF pushes. Replaced with `gh api repos/.../commits/<sha> --jq '.commit.verification.verified'`, the same source of truth the `required_signatures` branch protection rule uses downstream. Any commit the workflow accepts will also pass the push gate.

### Added

- **HRR persistence — split-format save/load on `HRRStructIndex`** ([#553](https://github.com/robotrocketscience/aelfrice/issues/553)). `HRRStructIndex.save(path)` now writes a per-store directory containing `struct.npy` (the `(N, dim)` float64 matrix, mmap-able) plus `meta.npz` (the small metadata blob: belief ids, role/id vectors, dim, seed, layout version) instead of a single bundled `.npz`. Writes are atomic via temp-file + `os.replace` so a reader process never observes a partial write. `HRRStructIndex.load(path)` auto-detects the layout: a directory with the two split-format files loads the new layout; a file path falls through to the v1.7 bundled `.npz` reader and emits a one-shot module-logger deprecation warning (legacy stores still load — this is a substrate landing, not a breaking change). New `mmap=False` keyword on `load()` requests `np.load(mmap_mode='r')` on the struct matrix; default off keeps existing callers byte-stable. The split layout is the substrate that makes persistence-default-ON viable per `docs/feature-hrr-integration.md` — `np.load(.npz, mmap_mode='r')` is silently ignored per the numpy docs, so mmap requires this format change. Sub-pieces (`HRRStructIndexCache` mmap read path, ephemeral-path auto-disable, `aelf doctor` rows, `AELFRICE_HRR_PERSIST` opt-out flag) ship as separate atomic PRs per the spec's "Coordination" section.
Expand Down
Loading