PR Review#

Consolidated review workflow for dakota pull requests.

When to Use#

Use when asked to review any Dakota PR, including feature PRs, dep-update PRs, or agent-authored maintenance changes.

When NOT to Use#

• Queue cleanup or stale-branch repair without substantive review → merge-queue.md
• General issue triage before any PR exists → actionadon.md

Core Process#

1. Read the workflow and checklist docs first.
2. Check branch hygiene and diff scope.
3. Verify the PR category-specific checklist items.
4. Check required CI status and mergeability.
5. Review correctness only after the workflow hygiene is sound.

Before you review#

1. Read docs/workflow.md — issue lifecycle, labels, branch flow
2. Read docs/pr-checklist.md — per-category checklist

Review priorities (in order)#

1. Branch hygiene — PR must branch from upstream/main, not a fork's local main. Verify with git diff upstream/main...HEAD --stat — it should be minimal and contain only the PR's changes.
2. Checklist compliance — verify the relevant items from pr-checklist.md for the type of change (junction bump, patch, OCI, element, etc.).
3. CI gate status — validate is a required status check on main (via merge queue). PRs targeting testing have no required status checks — testing is a loose integration branch; the quality gate is at main. If CI hasn't run on a main-targeting PR, note it. If e2e was skipped (non-image paths), that counts as passing.
4. Scope discipline — one logical change per PR. Junction bumps must not include patch modifications in the same commit.
5. Correctness — element syntax, layer kind (compose not stack), cargo sources generated not hand-written, systemd units enabled via BST install commands.

Common rejection reasons#

How to give feedback#

• Guide, don't just reject. Point contributors toward the correct pattern in docs/workflow.md or docs/pr-checklist.md.
• One comment per review event. Combine all findings into a single review comment. Never post follow-ups for new observations — edit the existing comment.
• Do not duplicate GitHub UI. Don't restate approval counts, merge queue status, or CI summaries that GitHub already shows.
• Minimal test reports. What ran, pass/fail, blockers. No diff summaries.

Ghost detection#

Agent-assisted PRs are identified by the checked template checkbox:
[x] I am using an agent and I take responsibility for this PR

Hold these to the same standard as human PRs. The operator is accountable.

Mergeraptor pre-approval#

Junction-only bumps from mergeraptor[bot] are pre-approved once validate and e2e pass (or e2e is skipped for non-image paths). No human review required for those.

Dep-update PR review#

Auto-generated dep-update PRs (auto/track-*, renovate/*) always target
main by default. Before reviewing, check:

1. Is it redundant? — Compare the ref: field in the element against
   upstream/testing. If identical, close the PR; testing already has it.
2. Cherry-pick, don't merge as-is — The branch is based on main and
   carries 4–6 unrelated CI/docs commits not in testing. Cherry-pick only
   the top dep-update commit onto a clean testing base.
3. Skip e2e if infrastructure is broken — If the same e2e failure appears
   on every dep PR simultaneously ("SSH never became ready"), it's a stale
   :testing issue, not a PR issue. Validate passing + correct diff is enough.

See merge-queue.md for the full retarget/cherry-pick/merge flow.

Common Rationalizations#

Red Flags#

• Reviewing code details before checking branch base and scope
• Ignoring checklist category mismatches
• Accepting junction bumps mixed with unrelated patch changes
• Treating skipped/absent CI as equivalent without understanding why

Verification#

• Branch hygiene was checked first
• Relevant checklist items were reviewed
• Required CI/check state was examined
• Scope is one logical change
• Review comments align with Dakota workflow, not just generic code review taste

Lessons Learned#

Rubber duck CI changes against bluefin/bluefin-lts before merging (2026-06-07)#

When making changes to .github/workflows/ that touch the publish, promote,
or release pipeline, run a rubber duck comparison against the equivalent
workflows in projectbluefin/bluefin and projectbluefin/bluefin-lts before
opening a PR. This surfaces inconsistencies and cross-repo bugs that pure
code review misses.

What to compare:

• cert-identity-regexp anchoring — must be ^...$ end-anchored (dakoa is correct; bluefin/lts are not)
• cosign cert-identity-regexp anchoring — must be ^...$ end-anchored
• SBOM handling — artifact-first with Syft fallback is stronger than always regenerating
• TOCTOU patterns — digest-pinned promotions, single skopeo inspect calls

Invocation:

rubber duck the release pipeline for consistency with projectbluefin/bluefin and bluefin-lts, built on projectbluefin/actions

Then pipe the findings directly into fixes before the PR lands. This session
caught 4 issues from rubber duck + 1 interaction bug (TOCTOU guard) from
doublecheck — all fixed before merging.

Check new service/file additions are wired into BST install-commands (2026-06-10)#

Dropping a file into files/firstboot/ (or any source directory) does not make
it land in the OCI image. elements/bluefin/firstboot-services.bst only installs
files explicitly listed in install-commands. A PR that adds a new file but no
corresponding install -Dm644 ... "%{install-root}/..." line is silently incomplete
— the file exists in the source tree but never reaches the image.

Review checklist for PRs adding new files:

1. Find the .bst element that owns the directory (grep path: files/<dir> in elements/)
2. Confirm the element's install-commands block has an entry for the new file
3. If the file should be enabled at boot, also check for a ln -sf symlink step

PR 750 added files/firstboot/keyring-unlock.service with no install-commands
entry — the service would never land in the image.