Skip to content

fix(sync): pin the signed-commits engine; record the grants decision - #67

Merged
kyle-sexton merged 1 commit into
mainfrom
fix/sync-pin-signed-commits
Jul 7, 2026
Merged

fix(sync): pin the signed-commits engine; record the grants decision#67
kyle-sexton merged 1 commit into
mainfrom
fix/sync-pin-signed-commits

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Two consequences of the pilot's merge attempt:

  • Pin bump to 7c29641 (ci-workflows#60): the pilot sync PR (github-iac#37) was green on every check but unmergeable — every constellation repo enforces required_signatures, and the engine's CLI-created commits were unsigned. The engine now creates commits via the API (sign-commits: true), signed by GitHub as the sync App's bot. After this merges, a re-run refreshes the pilot PR with a signed commit.
  • Grants decision recorded (plan doc, Auth): AppInstallationRepository is unusable under github-iac's credential model (the grants API rejects App installation tokens; the deploy deliberately stores no PAT). Superseded by "only select repositories" managed in the UI — provider-inexpressible, same class as App registration. github-iac#38 closed with the same rationale; a forgotten grant self-signals as a failed sync leg.

Verification

actionlint + markdownlint (module config) clean locally.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GbDWhcUtduCejgi7mcbMfy


Note

Low Risk
Workflow SHA pin changes how sync commits are created (fixes mergeability); doc-only grant policy clarification with no runtime code in this repo.

Overview
Pins the standards-sync reusable workflow in .github/workflows/sync.yml from 789abbe to 7c29641 so sync PRs use API commits with sign-commits: true, matching constellation repos that require signed commits (pilot PRs were green but unmergeable on unsigned CLI commits).

Updates docs/dedup-program/config-distribution-plan.md Auth and the activation checklist: repo access for the sync App is UI-managed (“only select repositories”), superseding Pulumi AppInstallationRepository (grants API + github-iac’s App-only credential model). Remaining manual steps are called out—narrow org installation, public App, personal-account install.

Reviewed by Cursor Bugbot for commit 859d7a6. Bugbot is set up for automated code reviews on this repo. Configure here.

@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 11s —— View job


PR Review

  • Gather context (CLAUDE.md, REVIEW.md)
  • Read PR diff and changed files
  • Analyze workflow change (sync.yml)
  • Analyze documentation change (config-distribution-plan.md)
  • Post review findings

Two changes, reviewed separately.

.github/workflows/sync.yml — SHA pin bump

Line 36: 789abbe7c29641. The bump follows the existing convention exactly (full 40-char SHA in the uses: line, short-SHA + date comment). Only one token changed; no other inputs, secrets, or structure were touched. Clean and minimal.

No issues here.

docs/dedup-program/config-distribution-plan.md — grants decision record

The Auth section's step 2 is fully replaced with a dated decision record that:

  • Names the root cause (AppInstallationRepository rejects App installation tokens) and the constraint that caused it (deploy model carries no PAT)
  • Cross-references the closed issue (github-iac#38) as evidence
  • States the resulting approach (UI-managed "Only select repositories")
  • Classifies it alongside App registration as provider-inexpressible
  • Documents the failure mode: "A forgotten grant is self-signaling: that target's sync leg fails to mint a token"

That failure-mode line is genuinely useful — it turns a possible silent footgun into a known-loud error, which is worth documenting.

The activation checklist update is consistent: the old "Pulumi-managed per-repo grants" sub-task is replaced with three discrete manual UI steps (flip to select repos, make App public, personal-account install).

One minor observation: The checklist adds "make the App public" without a corresponding explanation in the Auth section body. A reader following the Auth section for rationale won't find why the App needs to be public (the answer is that cross-account installs require it — a GitHub App must be public to be installable on accounts other than the owning org). The cross-account caveat below step 2 covers the what (install on both accounts) but not the why public is required. This is low-severity documentation gap — the checklist item is correct, the constraint is real, it just isn't explained inline.

Fix this →

No correctness or security issues. The SHA bump solves the described problem (unsigned CLI commits blocked by required_signatures) in the minimal way possible, and the documentation accurately captures the architectural decision with enough rationale to reconstruct the reasoning later.

Verdict: ready to merge, pending the one optional doc clarification above.

Re-pin to ci-workflows 7c29641 — the pilot PR passed every check but
was unmergeable because consumer rulesets enforce required_signatures
and the engine's CLI-created commits were unsigned; the engine now
signs via the API as the App bot. Also supersede the "manage grants via
Pulumi" intent in the plan doc: the grants API rejects App installation
tokens (all github-iac's credential model offers), so the installation
stays "only select repositories", UI-managed and recorded as
provider-inexpressible (github-iac#38 closed).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GbDWhcUtduCejgi7mcbMfy

@cursor cursor 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.

Risk: low. Cursor Bugbot completed with no findings; approving this workflow pin bump and documentation update.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@kyle-sexton
kyle-sexton merged commit 6b4542a into main Jul 7, 2026
37 of 38 checks passed
@kyle-sexton
kyle-sexton deleted the fix/sync-pin-signed-commits branch July 7, 2026 02:26
kyle-sexton added a commit that referenced this pull request Jul 15, 2026
…ception, bot composition, reply protocol) (#122)

## Summary

Five reasoning-only convention updates to `engineering/naming.md` and
`conventions/review/`:

- **naming.md** — promotes the org-wide branch-naming grammar
(Conventional Commits type prefix + cloud-agent/bot prefix, kebab-case
slug) into its own section, restated abstractly rather than cited from
any single downstream repo.
- **naming.md** — excepts `github-iac`'s label-governance seam
(`GovernedRepositorySpec.PreviousNames` aliasing, `Labels.cs` taxonomy
applied via `Github.IssueLabels`) from the "Rename across all coupled
edges" rule, since both propagate through a separate reviewed Pulumi
deployment on their own cadence rather than atomically with the
originating rename.
- **conventions/review/reply-protocol.md** (new) — codifies the
threaded, same-surface `Fixed in <sha>` reply as the standard way to
close a review finding once addressed, plus conditional
(bot-thread-only) resolution.
- **conventions/review/ai-review-bot-composition.md** (new) — documents
the expected AI-review-bot composition per repository governance class
(archived / no-CI-surface / governed / high-traffic primary), derived
from `github-iac`'s `GovernedRepositorySpec` registry rather than from
visibility or ownership.
- **conventions/review/code-quality.md** — converts the PR-title prose
bullet to a reference-only pointer at the `pr-title` workflow
(`ci-workflows/.github/workflows/semantic-pr.yml`) and
`enforceability-tiers.md`, removing the duplicated deterministic-rule
prose.

## Decisions closed

- #24 `naming-branch-convention-org-wide`
- #64 `naming-doc-conflict-content-fix`
- #22 `replies-codify-inline-reply-protocol`
- #51 `tooling-gov-review-bot-composition-standard`
- #67 `tooling-gov-standards-catalog-duplicate-prose`

Decisions Log:
https://claude.ai/code/artifact/232ecdce-8316-4880-8c0a-dc3c7dcf3a63
Evidence/rationale:
https://claude.ai/code/artifact/3160ae0e-c02f-4619-8de3-60d73faa1100

## Test plan

- [x] `markdownlint-cli2` — 0 errors on all changed/new files
- [x] `lychee --offline` — 0 broken links/anchors
- [x] `typos` — clean
- [x] Local `lefthook` pre-commit hooks (typos, gitleaks, editorconfig,
markdownlint) — all passed at commit time

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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