Skip to content

feat(gitcommitstatus): verify commit signatures over the promotion range - #1836

Open
akselleirv wants to merge 5 commits into
argoproj-labs:mainfrom
akselleirv:feat/gitcommitstatus-signature-verification
Open

akselleirv wants to merge 5 commits into
argoproj-labs:mainfrom
akselleirv:feat/gitcommitstatus-signature-verification

Conversation

@akselleirv

@akselleirv akselleirv commented Jul 29, 2026

Copy link
Copy Markdown

Closes #1801

Open questions

Is Verification.Commits too close to Commit? Whether Commit is in Verification.Commits depends on target — the newest entry under target: proposed, absent entirely under target: active. Alternatives: Verification.PromotedCommits, or a top-level Promotion variable with Promotion.Commits.

Should the scope be explicit in the spec? Verification always covers the range activeHydratedSha..proposedSha and ignores target, so target now only selects which commit's message the expression reads. The alternative is declaring it, as Argo CD did with head vs strict:

verification:
  scope: promotion   # default
  # scope: target

Summary by CodeRabbit

  • New Features
    • Added spec.verification to GitCommitStatus to enable GPG signature verification using trusted armored public keys.
    • Exposes verification status to promotion expressions for range-wide commits.
  • Documentation
    • Documented how verification works, required configuration shape, and expression usage.
  • Bug Fixes
    • Improved proposed branch naming during promotion strategy reconciliation.
  • Tests
    • Added end-to-end and unit coverage for commit verification and range validation.

Adds spec.verification to GitCommitStatus. When set, the expression sees a
top-level Verification variable carrying the signature verdict for every commit
the promotion would add, which is the range activeHydratedSha..proposedSha.

A promotion merges that whole range under a single CommitStatus, so verifying
only the newest commit would let an unsigned commit ride in underneath a signed
one.

Signatures are checked against an ephemeral, offline keyring built per reconcile
from spec.verification.gpg.publicKeys. Those keys are the complete trust anchor:
no keyserver is contacted and the node's keyring is never read.

Refs argoproj-labs#1801

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Aksel Skaar Leirvaag <aksel-skaar.leirvaag@telenor.no>
Copilot AI review requested due to automatic review settings July 29, 2026 07:26
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: dad5e2fe-a69d-4f79-b12b-78edae129391

📥 Commits

Reviewing files that changed from the base of the PR and between ea524f4 and 04f8a95.

📒 Files selected for processing (5)
  • docs/gating-promotions/built-in-gates/git-commit-status.md
  • internal/controller/gitcommitstatus_controller.go
  • internal/controller/gitcommitstatus_controller_test.go
  • internal/git/verification.go
  • internal/git/verification_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
  • internal/controller/gitcommitstatus_controller_test.go
  • internal/git/verification.go
  • docs/gating-promotions/built-in-gates/git-commit-status.md
  • internal/controller/gitcommitstatus_controller.go
  • internal/git/verification_test.go

📝 Walkthrough

Walkthrough

GitCommitStatus now supports optional GPG verification for every commit in a promotion range. The change adds API and CRD schemas, apply configurations, Git verification primitives, controller expression data, RBAC permissions, tests, and documentation.

Changes

Commit signature verification

Layer / File(s) Summary
Verification API and generated surfaces
api/v1alpha1/..., api/view/v1alpha1/..., applyconfiguration/api/v1alpha1/..., config/crd/...
GitCommitStatus gains validated GPG public-key configuration with generated OpenAPI, deepcopy, model-name, CRD, and apply-configuration support.
Git range verification engine
internal/git/git.go, internal/git/verification.go, internal/git/*_test.go
Git operations can locate and fetch commits, verify ranges using an offline temporary GPG keyring, parse per-commit results, and test signed, unsigned, mismatched, and incomplete ranges.
Controller verification flow
internal/controller/gitcommitstatus_controller.go, internal/controller/gitcommitstatus_controller_test.go, internal/utils/environments.go, internal/controller/promotionstrategy_controller.go
The controller verifies promotion ranges, exposes Verification data to expressions, adds required RBAC, and centralizes proposed branch naming. End-to-end tests cover configured and unconfigured verification.
Verification contract and documentation
docs/gating-promotions/built-in-gates/git-commit-status.md, hack/celcost/report.md, internal/controller/testdata/GitCommitStatus.yaml
Documentation and supporting generated reports describe verification coverage, expression fields, trusted keys, and validation requirements.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested reviewers: copilot

Sequence Diagram(s)

sequenceDiagram
  participant GitCommitStatusReconciler
  participant EnvironmentOperations
  participant GPGKeyring
  participant ExpressionEvaluator
  GitCommitStatusReconciler->>GPGKeyring: Create temporary keyring from configured public keys
  GitCommitStatusReconciler->>EnvironmentOperations: Verify the promotion commit range
  EnvironmentOperations->>GPGKeyring: Use the temporary GNUPGHOME
  EnvironmentOperations-->>GitCommitStatusReconciler: Return per-commit verification results
  GitCommitStatusReconciler->>ExpressionEvaluator: Evaluate the Verification data
Loading
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR verifies the full promotion range and exposes a top-level Verification model instead of the Commit.Verified/Signature* fields requested in #1801. Align the API with #1801 by exposing signature data on Commit, or update the issue/spec to explicitly accept a top-level Verification object and range-wide verification.
Docstring Coverage ⚠️ Warning Docstring coverage is 64.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ 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 is concise and accurately summarizes the main change: commit signature verification for GitCommitStatus promotion processing.
Out of Scope Changes check ✅ Passed The changes stay focused on commit-signature verification support and its supporting schema, docs, tests, and controller plumbing.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@codecov-commenter

Copy link
Copy Markdown

Bundle Report

Bundle size has no change ✅

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the GitCommitStatus gate to optionally verify GPG commit signatures across the entire promotion range (activeHydratedSha..proposedSha) and exposes the verification results to expressions via a new top-level Verification variable, complementing the existing single-commit Commit data.

Changes:

  • Add GPG keyring management and range-based signature verification in the git layer, plus supporting helpers (HasCommit, FetchBranch) to ensure SHAs are present locally.
  • Update the GitCommitStatus controller to (optionally) clone the repo, verify the promotion range signatures, and pass Verification into expression evaluation.
  • Extend CRD/schema/apply-config generation and documentation to describe and validate spec.verification.

Reviewed changes

Copilot reviewed 12 out of 18 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
internal/utils/environments.go Factors proposed-branch naming into a helper used by multiple controllers.
internal/git/verification.go Implements GPG keyring creation and signature parsing for commit ranges.
internal/git/verification_test.go Adds unit tests for signature verification behavior over ranges and edge cases.
internal/git/git.go Adds HasCommit and FetchBranch helpers to support verification flows.
internal/git/git_test.go Adds tests for HasCommit and FetchBranch behavior.
internal/controller/testdata/GitCommitStatus.yaml Updates controller testdata example to include optional spec.verification.
internal/controller/promotionstrategy_controller.go Switches proposed-branch computation to the new shared helper.
internal/controller/gitcommitstatus_controller.go Adds Verification expression data, repo clone setup, and promotion-range verification logic.
internal/controller/gitcommitstatus_controller_test.go Adds integration-style controller tests covering verification enabled/disabled and multi-commit ranges.
docs/gating-promotions/built-in-gates/git-commit-status.md Documents signature verification, trust model, verified range semantics, and expression usage.
config/crd/bases/promoter.argoproj.io_gitcommitstatuses.yaml Adds CRD schema for spec.verification (GPG keys + validation).
applyconfiguration/utils.go Registers apply-configuration types for new verification structs.
applyconfiguration/api/v1alpha1/gitcommitverification.go Generated apply-config for GitCommitVerification.
applyconfiguration/api/v1alpha1/gitcommitverificationgpg.go Generated apply-config for GitCommitVerificationGPG.
applyconfiguration/api/v1alpha1/gitcommitverificationgpgpublickey.go Generated apply-config for GitCommitVerificationGPGPublicKey.
applyconfiguration/api/v1alpha1/gitcommitstatusspec.go Adds Verification to the generated GitCommitStatus spec apply-config.
api/v1alpha1/zz_generated.deepcopy.go Updates deep-copy generation for new verification fields/types.
api/v1alpha1/gitcommitstatus_types.go Introduces API types for spec.verification and GPG public keys with validations.
Files not reviewed (6)
  • api/v1alpha1/zz_generated.deepcopy.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitstatusspec.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverification.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverificationgpg.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverificationgpgpublickey.go: Generated file
  • applyconfiguration/utils.go: Generated file
Comments suppressed due to low confidence (2)

internal/git/verification.go:118

  • NewGPGKeyring replaces the entire environment for the gpg subprocess with only GNUPGHOME and PATH. This can cause brittle behavior in some environments (locale, HOME, proxy/TLS env, etc.). Prefer inheriting os.Environ() and overriding GNUPGHOME.
		cmd := exec.CommandContext(ctx, "gpg", "--homedir", home, "--batch", "--no-tty", "--no-autostart", "--import")
		cmd.Env = []string{"GNUPGHOME=" + home, "PATH=" + os.Getenv("PATH")}
		cmd.Stdin = strings.NewReader(armored)

internal/controller/gitcommitstatus_controller.go:444

  • Follow-up to renaming VerificationCommit.Typ -> Type: update the struct literal here to populate the renamed field.
		result.Commits = append(result.Commits, VerificationCommit{
			SHA:      sig.SHA,
			Verified: sig.Verified,
			Typ:      sig.Type,
			KeyID:    sig.KeyID,
			Signer:   sig.Signer,

Comment thread internal/utils/environments.go
Comment thread internal/controller/gitcommitstatus_controller.go
Comment thread internal/utils/environments.go
Comment thread internal/git/verification.go
@codecov-commenter

codecov-commenter commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 56.83060% with 79 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.08%. Comparing base (6d2d227) to head (04f8a95).

Files with missing lines Patch % Lines
internal/controller/gitcommitstatus_controller.go 59.55% 22 Missing and 14 partials ⚠️
internal/git/verification.go 60.31% 13 Missing and 12 partials ⚠️
internal/git/git.go 18.18% 16 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1836      +/-   ##
==========================================
- Coverage   56.23%   52.08%   -4.15%     
==========================================
  Files         133      129       -4     
  Lines       11089    11145      +56     
  Branches      450      450              
==========================================
- Hits         6236     5805     -431     
- Misses       4204     4623     +419     
- Partials      649      717      +68     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

- Guard verification on gitOperations as well as keyring so nilaway can see
  both are non-nil, rather than inferring it from spec.verification.
- Rename VerificationCommit.Typ to Type, matching the expression field.
- Move `git log -1` before the revision, matching the rest of the repo.
- Derive the signing identity from the key's email in tests instead of parsing
  a fingerprint out of gpg's --with-colons listing, which drops the "fpr"
  record tag the spell checker rejected.
- Run build-installer to regenerate the CEL cost report, OpenAPI definitions,
  model names and dist bundles.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Aksel Skaar Leirvaag <aksel-skaar.leirvaag@telenor.no>
Copilot AI review requested due to automatic review settings July 29, 2026 07:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 24 changed files in this pull request and generated no new comments.

Files not reviewed (7)
  • api/v1alpha1/zz_generated.deepcopy.go: Generated file
  • api/v1alpha1/zz_generated.model_name.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitstatusspec.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverification.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverificationgpg.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverificationgpgpublickey.go: Generated file
  • applyconfiguration/utils.go: Generated file
Comments suppressed due to low confidence (2)

internal/git/verification_test.go:42

  • In this spec, Skip("gpg not installed") can run before remoteDir/signerHome are set, but AfterEach still tries to use them. Depending on os.RemoveAll("") behavior this can fail the skipped spec or attempt to delete the wrong path. Guard cleanup on non-empty paths (and only kill gpgconf when signerHome is set).
	BeforeEach(func() {
		if _, err := exec.LookPath("gpg"); err != nil {
			Skip("gpg not installed")
		}
		var err error
		remoteDir, err = os.MkdirTemp("", "git-verify-remote-*")
		Expect(err).NotTo(HaveOccurred())
		signerHome, err = os.MkdirTemp("", "git-verify-gpg-*")
		Expect(err).NotTo(HaveOccurred())
		Expect(os.Chmod(signerHome, 0o700)).To(Succeed())
	})

	AfterEach(func() {
		_ = exec.CommandContext(context.Background(), "gpgconf", "--homedir", signerHome, "--kill", "all").Run()
		Expect(os.RemoveAll(remoteDir)).To(Succeed())
		Expect(os.RemoveAll(signerHome)).To(Succeed())
	})

internal/controller/gitcommitstatus_controller.go:300

  • Issue #1801’s proposal explicitly scoped the “first implementation” to verifying only the target commit (Argo CD “head” mode) and called verifying full ancestry/range out of scope. This change verifies the full promotion range (activeHydratedSha..proposedSha) and exposes it via Verification, which may be a requirements mismatch for closing #1801 unless the issue scope has been updated/accepted.
		var verification *VerificationData
		if gitOperations != nil && keyring != nil {
			// Verification always covers the promotion range, independent of Target: Target selects
			// whose commit message the expression reads, but a promotion merges the whole range.
			verification, err = verifyPromotionRange(ctx, gitOperations, keyring,
				commitRef{sha: activeHydratedSha, branch: env.Branch},
				commitRef{sha: proposedSha, branch: utils.ProposedBranchName(ps, env)})
			if err != nil {
				return nil, nil, fmt.Errorf("failed to verify signatures for branch %q over range %q..%q: %w", branch, activeHydratedSha, proposedSha, err)
			}
		}

Signed-off-by: Aksel Skaar Leirvaag <aksel-skaar.leirvaag@telenor.no>
Copilot AI review requested due to automatic review settings July 29, 2026 07:59
Signed-off-by: Aksel Skaar Leirvaag <aksel-skaar.leirvaag@telenor.no>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 25 changed files in this pull request and generated no new comments.

Files not reviewed (7)
  • api/v1alpha1/zz_generated.deepcopy.go: Generated file
  • api/v1alpha1/zz_generated.model_name.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitstatusspec.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverification.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverificationgpg.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverificationgpgpublickey.go: Generated file
  • applyconfiguration/utils.go: Generated file
Comments suppressed due to low confidence (4)

internal/controller/gitcommitstatus_controller.go:393

  • setupGitOperations fetches the GitRepository twice (once indirectly via GetScmProviderAndSecretFromRepositoryReference, then again via GetGitRepositoryFromObjectKey). This adds an unnecessary API call on every reconcile when verification is enabled; you can use the existing helper that returns the ScmProvider, Secret, and GitRepository in one GET.
func (r *GitCommitStatusReconciler) setupGitOperations(ctx context.Context, gcs *promoterv1alpha1.GitCommitStatus, ps *promoterv1alpha1.PromotionStrategy) (*git.EnvironmentOperations, error) {
	repoRef := ps.Spec.RepositoryReference

	scmProvider, secret, err := utils.GetScmProviderAndSecretFromRepositoryReference(ctx, r.Client, r.SettingsMgr.GetControllerNamespace(), repoRef, gcs)
	if err != nil {
		return nil, fmt.Errorf("failed to get ScmProvider and secret for repo %q: %w", repoRef.Name, err)
	}

	gitAuthProvider, err := gitauth.CreateGitOperationsProvider(ctx, r.Client, scmProvider, secret, client.ObjectKey{Namespace: gcs.Namespace, Name: repoRef.Name})
	if err != nil {
		return nil, fmt.Errorf("failed to create git auth provider for ScmProvider %q: %w", scmProvider.GetName(), err)
	}

	gitRepo, err := utils.GetGitRepositoryFromObjectKey(ctx, r.Client, client.ObjectKey{Namespace: gcs.Namespace, Name: repoRef.Name})
	if err != nil {
		return nil, fmt.Errorf("failed to get GitRepository %q: %w", repoRef.Name, err)
	}

internal/git/verification_test.go:42

  • AfterEach always runs gpgconf --kill all even when the spec was skipped due to missing gpg (signerHome is still empty). Passing an empty homedir can end up targeting the default user GNUPGHOME, which is risky on developer machines. Guard the gpgconf call so it only runs when signerHome was created.
	AfterEach(func() {
		_ = exec.CommandContext(context.Background(), "gpgconf", "--homedir", signerHome, "--kill", "all").Run()
		Expect(os.RemoveAll(remoteDir)).To(Succeed())
		Expect(os.RemoveAll(signerHome)).To(Succeed())
	})

internal/controller/gitcommitstatus_controller_test.go:361

  • This test configures spec.verification, which triggers git.NewGPGKeyring and requires the gpg binary. Unlike newSigningKey (which Skips when gpg is missing), this spec will hard-fail on environments where gpg isn't installed. Add the same exec.LookPath guard so the suite degrades gracefully on dev machines.
		It("reports an unsigned commit as unverified when verification is configured", func() {
			repo := newPromotionRepo(ctx, gitRepo, nil)
			repo.alignActiveWithProposed(ctx)
			repo.commitToProposedBranches(ctx, false, "unsigned promotion")

internal/utils/environments.go:73

  • ProposedBranchName uses path.Join to build a git branch/ref name. path.Join cleans the path (resolving .., stripping duplicate slashes, and treating a leading / as absolute), so a user-supplied ActivePath like /foo or ../prod can silently change the intended branch name (e.g., dropping the "-next" prefix). This can lead to fetching/pushing the wrong refs. Consider treating ActivePath as an opaque ref component (string concatenation with / after rejecting absolute/.. segments) and adding CRD validation to forbid absolute paths and .. in PromotionStrategy.spec.activePath and Environment.activePath.
	proposedBranch := fmt.Sprintf("%s-%s", env.Branch, "next")
	if activePath != "" {
		proposedBranch = path.Join(proposedBranch, activePath)
	}

Copilot AI review requested due to automatic review settings July 29, 2026 08:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 25 changed files in this pull request and generated no new comments.

Files not reviewed (7)
  • api/v1alpha1/zz_generated.deepcopy.go: Generated file
  • api/v1alpha1/zz_generated.model_name.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitstatusspec.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverification.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverificationgpg.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverificationgpgpublickey.go: Generated file
  • applyconfiguration/utils.go: Generated file
Comments suppressed due to low confidence (2)

internal/utils/environments.go:73

  • path.Join(proposedBranch, activePath) will drop proposedBranch entirely if activePath is absolute (starts with /), producing a proposed branch like /foo instead of <env>-next/foo. Since ActivePath has no validation preventing a leading /, sanitize it before joining so branch naming is stable and can't be accidentally broken by absolute paths.
	proposedBranch := fmt.Sprintf("%s-%s", env.Branch, "next")
	if activePath != "" {
		proposedBranch = path.Join(proposedBranch, activePath)
	}
	return proposedBranch

internal/git/git.go:961

  • HasCommit only treats stderr containing "not a valid object name" as a non-error absence. Depending on git version / failure mode, missing commits can also surface as "bad object" / "unknown revision" / "ambiguous argument"; returning an error in those cases can cause unnecessary requeues (and, for verification, repeated fetch attempts). Consider recognizing these as "not found" as well.
	_, stderr, err := g.runCmd(ctx, gitPath, "cat-file", "-e", sha+"^{commit}")
	if err != nil {
		if strings.Contains(strings.ToLower(stderr), "not a valid object name") {
			return false, nil
		}

@akselleirv
akselleirv marked this pull request as ready for review July 29, 2026 08:14

@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: 2

🧹 Nitpick comments (1)
internal/git/verification_test.go (1)

144-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover VerifyCommitRange’s error contract.

All new cases assert success through this helper. Add a direct nil-keyring test so the explicit failure path remains covered.

🤖 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 `@internal/git/verification_test.go` around lines 144 - 150, Add a direct test
for VerifyCommitRange that passes a nil keyring and asserts the documented
error, rather than routing through verifyRange, which only covers successful
calls. Keep the existing success cases unchanged and use the same repository
setup/context established by the surrounding tests.

Source: Coding guidelines

🤖 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 `@docs/gating-promotions/built-in-gates/git-commit-status.md`:
- Line 225: Wrap the Commit.Trailers type expression in backticks in the
documentation list entry so markdownlint treats map[string][]string as literal
code rather than a reference link.

In `@internal/controller/gitcommitstatus_controller.go`:
- Around line 290-300: Update the verification block around verifyPromotionRange
to handle an empty proposed SHA before invoking verification: when
envStatus.Proposed.Hydrated.Sha is missing, fail closed by returning unverified
or otherwise skipping verification. Only call verifyPromotionRange when both the
active and proposed promotion endpoints are available, preserving the existing
error handling for verification failures.

---

Nitpick comments:
In `@internal/git/verification_test.go`:
- Around line 144-150: Add a direct test for VerifyCommitRange that passes a nil
keyring and asserts the documented error, rather than routing through
verifyRange, which only covers successful calls. Keep the existing success cases
unchanged and use the same repository setup/context established by the
surrounding tests.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d763c687-a431-4633-900e-8c448f308e65

📥 Commits

Reviewing files that changed from the base of the PR and between 6d2d227 and ea524f4.

⛔ Files ignored due to path filters (4)
  • dist/install-with-dashboard-byo-cert.yaml is excluded by !**/dist/**
  • dist/install-with-dashboard-cert-manager.yaml is excluded by !**/dist/**
  • dist/install-without-ui.yaml is excluded by !**/dist/**
  • ui/shared/src/types/generated/view.gen.ts is excluded by !**/generated/**
📒 Files selected for processing (21)
  • api/v1alpha1/gitcommitstatus_types.go
  • api/v1alpha1/zz_generated.deepcopy.go
  • api/v1alpha1/zz_generated.model_name.go
  • api/view/v1alpha1/zz_generated.openapi.go
  • applyconfiguration/api/v1alpha1/gitcommitstatusspec.go
  • applyconfiguration/api/v1alpha1/gitcommitverification.go
  • applyconfiguration/api/v1alpha1/gitcommitverificationgpg.go
  • applyconfiguration/api/v1alpha1/gitcommitverificationgpgpublickey.go
  • applyconfiguration/utils.go
  • config/crd/bases/promoter.argoproj.io_gitcommitstatuses.yaml
  • docs/gating-promotions/built-in-gates/git-commit-status.md
  • hack/celcost/report.md
  • internal/controller/gitcommitstatus_controller.go
  • internal/controller/gitcommitstatus_controller_test.go
  • internal/controller/promotionstrategy_controller.go
  • internal/controller/testdata/GitCommitStatus.yaml
  • internal/git/git.go
  • internal/git/git_test.go
  • internal/git/verification.go
  • internal/git/verification_test.go
  • internal/utils/environments.go

Comment thread docs/gating-promotions/built-in-gates/git-commit-status.md Outdated
Comment thread internal/controller/gitcommitstatus_controller.go
Copilot AI review requested due to automatic review settings July 29, 2026 09:56
git resolves "<from>.." to "<from>..HEAD" and exits 0, so an empty proposed
SHA verified whatever the clone had checked out and could report the range as
verified. Reject it at both layers instead.

Also wraps a doc type in backticks so markdownlint stops reading it as a
reference link.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Aksel Skaar Leirvaag <aksel-skaar.leirvaag@telenor.no>
@akselleirv
akselleirv force-pushed the feat/gitcommitstatus-signature-verification branch from 23efdde to 04f8a95 Compare July 29, 2026 09:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 25 changed files in this pull request and generated no new comments.

Files not reviewed (7)
  • api/v1alpha1/zz_generated.deepcopy.go: Generated file
  • api/v1alpha1/zz_generated.model_name.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitstatusspec.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverification.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverificationgpg.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverificationgpgpublickey.go: Generated file
  • applyconfiguration/utils.go: Generated file
Comments suppressed due to low confidence (1)

internal/git/verification_test.go:42

  • AfterEach always invokes gpgconf --homedir <signerHome> even when the spec is skipped in BeforeEach (e.g. when gpg is not installed), leaving signerHome as an empty string. Guarding this avoids potentially targeting the default GPG home (or otherwise undefined behavior) during cleanup.
	AfterEach(func() {
		_ = exec.CommandContext(context.Background(), "gpgconf", "--homedir", signerHome, "--kill", "all").Run()
		Expect(os.RemoveAll(remoteDir)).To(Succeed())
		Expect(os.RemoveAll(signerHome)).To(Succeed())
	})

Copilot AI review requested due to automatic review settings July 29, 2026 10:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 25 changed files in this pull request and generated no new comments.

Files not reviewed (7)
  • api/v1alpha1/zz_generated.deepcopy.go: Generated file
  • api/v1alpha1/zz_generated.model_name.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitstatusspec.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverification.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverificationgpg.go: Generated file
  • applyconfiguration/api/v1alpha1/gitcommitverificationgpgpublickey.go: Generated file
  • applyconfiguration/utils.go: Generated file
Comments suppressed due to low confidence (2)

internal/utils/environments.go:73

  • ProposedBranchName uses path.Join(proposedBranch, activePath). If ActivePath is misconfigured with a leading '/', path.Join treats it as absolute and can drop the proposedBranch prefix (e.g. "env-next" + "/subdir" => "/subdir"), producing an invalid/unexpected branch name and potentially causing git fetches to target the wrong ref.
	activePath := ps.Spec.ActivePath
	if env.ActivePath != "" {
		activePath = env.ActivePath
	}
	proposedBranch := fmt.Sprintf("%s-%s", env.Branch, "next")
	if activePath != "" {
		proposedBranch = path.Join(proposedBranch, activePath)
	}

internal/controller/gitcommitstatus_controller_test.go:144

  • promotionRepo sets cmd.Env to only GIT_TERMINAL_PROMPT and PATH (plus optional GNUPGHOME). Replacing the whole environment can make git/gpg behavior depend on runner quirks (e.g. missing HOME, LANG, SSH config), which can cause flaky tests across CI images.
	r := &promotionRepo{dir: dir, env: []string{"GIT_TERMINAL_PROMPT=0", "PATH=" + os.Getenv("PATH")}}
	if key != nil {
		r.env = append(r.env, "GNUPGHOME="+key.home)
	}

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.

Commit signature verification via GitCommitStatus

3 participants