feat(cli): verify container image signatures and SLSA provenance on pull#492
feat(cli): verify container image signatures and SLSA provenance on pull#492
Conversation
Add client-side verification of container image cosign signatures and SLSA L3 provenance attestations before pulling images. This closes the gap where CLI binary updates were verified but container images were pulled with zero verification. New verify/ package: - Resolve image tags to digests via go-containerregistry - Verify cosign keyless signatures from Sigstore transparency log - Verify SLSA provenance attestations via OCI referrers API - User-visible progress output during verification Integration: - synthorg start: verify images before pulling - synthorg update: verify images in pullAndPersist flow - --skip-verify flag for air-gapped environments (with warning) - Digest pinning in compose.yml after verification - Verified digests cached in config.json CLI self-update enhancement: - Assert SLSA predicate type in Sigstore bundle verification - Validates DSSE envelope contains slsa.dev/provenance predicate Also updates all Go dependencies to latest versions. Closes #491
Pre-reviewed by 6 agents, 20 findings addressed: Security fixes: - Return error when no Sigstore bundle found in provenance attestation instead of silently accepting unverified structural data - Tighten SAN regex to only accept docker.yml workflow on tags/main - Validate DigestPins values in compose validateParams against digest regex - Preserve cosign bundle verification errors instead of swallowing them Performance: - Build Sigstore verifier and identity policy once per VerifyImages call instead of per-bundle (eliminates 6+ redundant TUF root fetches) Code quality: - Replace custom hexToBytes with encoding/hex.DecodeString - Replace custom contains/searchStr with strings.Contains - Extract verifyAndPinImages helper from runStart (was 83 lines) - Split verifyAttestation into focused helpers (was 81 lines) - Remove silent URL-safe base64 fallback in DSSE payload decoding - Fix DigestRef to return (string, error) instead of documenting a panic - Fix FuzzNewImageRef to test actual invariant (repository prefix) - Use parseDigest helper for consistent digest parsing Documentation: - Add verify/ package to CLAUDE.md Package Structure - Document --skip-verify flag in CLAUDE.md cmd/ description - Update fuzz matrix count from 4 to 5 packages - Add CLI-side verification mention to Docker section - Add image verification to README Security & Trust section
Dependency ReviewThe following issues were found:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🧰 Additional context used📓 Path-based instructions (1)cli/**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: 2026-03-15T11:48:14.867ZApplied to files:
📚 Learning: 2026-03-16T19:51:10.862ZApplied to files:
📚 Learning: 2026-03-16T19:51:10.862ZApplied to files:
🧬 Code graph analysis (2)cli/cmd/update.go (3)
cli/internal/verify/provenance_test.go (2)
🪛 GitHub Check: CodeQLcli/cmd/start.go[failure] 193-193: Uncontrolled data used in path expression 🔇 Additional comments (14)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a verification subsystem to the CLI that resolves image digests, verifies cosign signatures and SLSA provenance before pulling (start/update), pins verified digests into generated compose files and persisted state, adds a persistent Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (start/update)
participant Registry as OCI Registry
participant Sigstore as Sigstore Verifier
CLI->>Registry: Resolve image digest (HEAD/GET manifest)
Registry-->>CLI: Respond with digest
CLI->>Registry: Fetch cosign signature artifact (image.sig tag)
Registry-->>CLI: Signature manifest / layers
CLI->>Sigstore: Verify cosign signature bundle + cert identity
Sigstore-->>CLI: Signature verification result
CLI->>Registry: Discover SLSA attestations (OCI referrers)
Registry-->>CLI: Attestation descriptors
CLI->>Registry: Fetch attestation image
Registry-->>CLI: Attestation manifest / layers
CLI->>Sigstore: Verify provenance bundle (DSSE/in-toto predicate)
Sigstore-->>CLI: Provenance verification result
CLI->>CLI: Pin verified digest into compose file & state
CLI->>Registry: Pull images using pinned digest
Registry-->>CLI: Images pulled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security posture of the CLI by integrating robust supply chain security mechanisms. It introduces client-side verification for all container images used by the application, ensuring their authenticity and integrity before deployment. This measure protects against tampering and unauthorized modifications, providing users with greater confidence in the software's origin and composition. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request introduces a significant security enhancement by implementing client-side verification of container image signatures and SLSA provenance. The design is robust, leveraging sigstore-go and go-containerregistry, and includes appropriate identity policies and error handling. The comprehensive test suite, including unit and fuzz tests, demonstrates a strong commitment to correctness and reliability. The documentation updates clearly communicate the new verification capabilities.
cli/cmd/update.go
Outdated
| rollback() | ||
| return fmt.Errorf("image verification failed: %w\n Use --skip-verify for air-gapped environments", err) | ||
| } | ||
| digestPins = digestPinMap(results) |
There was a problem hiding this comment.
The digestPinMap function is duplicated here and in cli/cmd/start.go. To improve maintainability and adhere to the DRY principle, consider extracting this function into a shared utility package, perhaps within cli/internal/verify or a new cli/internal/util package.
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/cmd/root.go`:
- Around line 31-32: Add support for an environment variable fallback for the
existing skip-verify flag by checking SYNTHORG_SKIP_VERIFY and setting the
skipVerify variable when appropriate; specifically, keep the
rootCmd.PersistentFlags().BoolVar(&skipVerify, "skip-verify", ...) declaration
but after flags are initialized (or before command execution) read
os.Getenv("SYNTHORG_SKIP_VERIFY"), parse it as a boolean (accept
"1"/"true"/"yes"), and if present override skipVerify so CI/air-gapped runs can
use the SYNTHORG_SKIP_VERIFY env var instead of the --skip-verify flag;
reference the skipVerify variable and the "skip-verify" flag
(rootCmd.PersistentFlags()) when implementing this override.
In `@cli/internal/compose/generate.go`:
- Around line 22-24: The duplicate regex digestPattern (defined here and in
verify.go) should be centralized to avoid drift: either export it from the
verify package (e.g., change digestPattern to VerifyDigestPattern or
DigestPattern in package verify and import verify.DigestPattern here) or move it
to a new shared package (e.g., ociutil.DigestPattern) and update usages in both
compose/generate.go and verify.go to reference the single exported symbol;
update imports accordingly and remove the local regex definition in generate.go.
In `@cli/internal/config/state.go`:
- Line 27: The State.validate() method must verify each entry in the
VerifiedDigests map: iterate over s.VerifiedDigests and ensure each value
matches the exact pattern "sha256:<64-hex-chars>" (use a regexp like
^sha256:[a-fA-F0-9]{64}$); if any value is missing or doesn't match, return a
descriptive error that includes the offending map key (the image/reference) and
the invalid digest string. Ensure you import/use regexp in validate(), treat a
nil/empty map as valid, and fail fast on the first invalid entry so callers get
immediate, precise feedback.
In `@cli/internal/selfupdate/sigstore_test.go`:
- Line 18: The test currently ignores the error returned by json.Marshal when
encoding the slsaStatement into payload; update the test to capture and assert
no error (e.g., err := json.Marshal(statement)) and fail the test if err != nil
so unexpected marshal failures are surfaced — locate the line with
json.Marshal(statement) and replace the blank identifier usage with proper error
handling and an assertion that err is nil.
In `@cli/internal/verify/cosign_test.go`:
- Around line 139-143: The test uses an invalid config digest in the
ociDescriptor (Digest set to "sha256:... tried") so manifest parsing fails
before bundle validation; update the Digest fields (both occurrences around the
configJSON fixtures at the earlier and later locations) to a valid sha256 string
(algorithm "sha256" plus a 64-character lowercase hex) so VerifyCosignSignature
proceeds to bundle extraction, and then tighten the test to assert the returned
error is related to an invalid cosign bundle (bundle validation) rather than a
manifest decode error.
In `@cli/internal/verify/cosign.go`:
- Around line 112-124: parseDigest currently accepts any algorithm prefix; add
defensive validation to ensure the algorithm (parts[0]) equals "sha256" (or call
existing IsValidDigest logic) and return a clear error if it does not. In the
parseDigest function, after splitting the digest and before hex.DecodeString,
check that parts[0] == "sha256" (or reuse IsValidDigest/its logic) and return
fmt.Errorf("unsupported digest algorithm %q", parts[0]) when invalid so callers
only get recognized algorithms and malformed inputs are rejected.
In `@cli/internal/verify/digest.go`:
- Around line 20-23: The current call to remote.Head(tagRef,
remote.WithContext(ctx)) can fail on registries that disallow HEAD; modify the
logic in the function that calls remote.Head (the block using variables tagRef,
desc, err) to fall back to remote.Get(tagRef, remote.WithContext(ctx)) when the
initial Head returns an error (i.e., if err != nil then call remote.Get and only
return an error if that also fails), and ensure the final error message still
uses the same fmt.Errorf("fetching manifest for %s: %w", ref, err) pattern; also
add a unit/integration test that simulates a registry returning 405 for HEAD to
verify the fallback path.
In `@cli/internal/verify/fuzz_test.go`:
- Around line 19-23: When result is true the test currently only checks length;
tighten it to also assert the required "sha256:" prefix by checking
strings.HasPrefix(digest, "sha256:") (and calling t.Errorf if it fails) inside
the same branch where len(digest) is validated; add the strings import if
missing and keep the existing length check for the full 71-character invariant
to ensure both prefix and length are enforced for the digest variable used in
this test.
In `@cli/internal/verify/provenance_test.go`:
- Around line 121-124: The test is not isolating the wrong-predicate failure
because the dsseEnvelope (constructed using dsseEnvelope, dssePayloadType and
Payload set from statementJSON) lacks a valid bundle and can be rejected by
VerifyProvenance before predicate checking; either update the fixture to include
a valid bundle (attach a proper bundle field to the dsseEnvelope and ensure the
embedded statementJSON represents a valid in-toto attestation) so
VerifyProvenance reaches the predicate-type check, or change the test assertion
to expect a generic invalid-attestation error (don’t assert on a specific
wrong-predicate message) so the test no longer assumes predicate evaluation;
apply the same change pattern to the other affected cases (lines covering the
other dsseEnvelope fixtures).
In `@cli/internal/verify/provenance.go`:
- Around line 166-188: verifyAttestationBundle currently only checks
attestManifest.Layers[0] for the "dev.sigstore.cosign/bundle" annotation, which
can miss bundles stored on other layers; change verifyAttestationBundle to
iterate all layers in attestManifest.Layers (similar to verifyCosign's for i :=
range layers) and for each layer check
layer.Annotations["dev.sigstore.cosign/bundle"], calling
verifyProvenanceBundleWith([]byte(bundleJSON), digest, sev, certID) and
returning on success; if no layer contains the annotation, return the existing
"no sigstore bundle found..." error.
🪄 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: 53de28c3-da1d-4d74-ab7f-0bde2d8692f4
⛔ Files ignored due to path filters (1)
cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
.github/workflows/cli.ymlCLAUDE.mdREADME.mdcli/cmd/root.gocli/cmd/start.gocli/cmd/update.gocli/go.modcli/internal/compose/compose.yml.tmplcli/internal/compose/generate.gocli/internal/compose/generate_test.gocli/internal/config/state.gocli/internal/selfupdate/sigstore.gocli/internal/selfupdate/sigstore_test.gocli/internal/verify/cosign.gocli/internal/verify/cosign_test.gocli/internal/verify/digest.gocli/internal/verify/digest_test.gocli/internal/verify/fuzz_test.gocli/internal/verify/identity.gocli/internal/verify/identity_test.gocli/internal/verify/provenance.gocli/internal/verify/provenance_test.gocli/internal/verify/verify.gocli/internal/verify/verify_test.gocli/testdata/compose_digest_pins.yml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (2)
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Lint CLI Go code with golangci-lint and go vet; test with go test -race; check vulnerabilities with govulncheck
Files:
cli/internal/verify/digest.gocli/internal/verify/digest_test.gocli/internal/selfupdate/sigstore.gocli/internal/verify/identity_test.gocli/internal/compose/generate_test.gocli/internal/compose/generate.gocli/cmd/start.gocli/internal/verify/cosign_test.gocli/internal/verify/provenance_test.gocli/internal/verify/identity.gocli/internal/config/state.gocli/cmd/root.gocli/internal/verify/verify_test.gocli/internal/verify/fuzz_test.gocli/internal/selfupdate/sigstore_test.gocli/internal/verify/provenance.gocli/cmd/update.gocli/internal/verify/verify.gocli/internal/verify/cosign.go
cli/go.mod
📄 CodeRabbit inference engine (CLAUDE.md)
CLI requires Go 1.26+; dependencies in
cli/go.mod(Cobra, charmbracelet/huh, charmbracelet/lipgloss)
Files:
cli/go.mod
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to .github/workflows/docker.yml : CI Docker: build → scan → push to GHCR + cosign sign + SLSA L3 provenance via attest-build-provenance (images only pushed after Trivy/Grype scans pass).
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to cli/**/*.go : Lint CLI Go code with golangci-lint and go vet; test with go test -race; check vulnerabilities with govulncheck
Applied to files:
cli/internal/verify/identity_test.gocli/cmd/start.gocli/go.modCLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to docker/{Dockerfile*,compose.yml} : Docker: Backend uses 3-stage build (builder → setup → distroless runtime), Chainguard Python, non-root (UID 65532), CIS-hardened. Web uses nginxinc/nginx-unprivileged, Vue 3 SPA with PrimeVue + Tailwind CSS, SPA routing, API/WebSocket proxy to backend.
Applied to files:
cli/testdata/compose_digest_pins.ymlcli/internal/compose/compose.yml.tmplCLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Dependabot: auto-updates Docker image digests and versions daily.
Applied to files:
cli/internal/compose/compose.yml.tmplcli/go.modcli/cmd/update.goCLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).
Applied to files:
cli/cmd/start.gocli/go.modcli/cmd/update.goCLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to .github/workflows/docker.yml : CI Docker: build → scan → push to GHCR + cosign sign + SLSA L3 provenance via attest-build-provenance (images only pushed after Trivy/Grype scans pass).
Applied to files:
README.mdCLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to cli/go.mod : CLI requires Go 1.26+; dependencies in `cli/go.mod` (Cobra, charmbracelet/huh, charmbracelet/lipgloss)
Applied to files:
cli/go.modcli/cmd/update.goCLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Shell commands: for Go CLI work, cd cli is an exception because Go tooling requires working directory to be the module root. Go commands require `cd cli` for other work, never use `cd`.
Applied to files:
cli/go.modCLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to .github/workflows/**/*.yml : Path filtering: dorny/paths-filter detects Python/dashboard/docker changes; jobs only run when their domain is affected. CLI has its own workflow (cli.yml).
Applied to files:
.github/workflows/cli.ymlCLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to docker/** : Docker build context: single root .dockerignore (both images build with context: .). Tags: CI tags images with version from pyproject.toml ([tool.commitizen].version), semver, and SHA.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Lint Python code with ruff (check + format); type-check with mypy strict mode; test with pytest + coverage ≥80%
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Pre-push hooks: mypy type-check + pytest unit tests + golangci-lint + go vet + go test (CLI, conditional on cli/**/*.go) (fast gate before push, skipped in pre-commit.ci — dedicated CI jobs already run these).
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Pre-commit hooks: trailing-whitespace, end-of-file-fixer, check-yaml, check-toml, check-json, check-merge-conflict, check-added-large-files, no-commit-to-branch (main), ruff check+format, gitleaks, hadolint (Dockerfile linting).
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.027Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.027Z
Learning: Pre-push hooks enforce: mypy type-check + pytest unit tests + golangci-lint + go vet + go test (CLI, conditional); skip in pre-commit.ci as dedicated CI jobs run these
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Commits: <type>: <description> — types: feat, fix, refactor, docs, test, chore, perf, ci. Enforced by commitizen (commit-msg hook). Signed commits: required on main via branch protection — all commits must be GPG/SSH signed.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.027Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.027Z
Learning: Pre-commit hooks enforce: trailing-whitespace, end-of-file-fixer, check-yaml, check-toml, check-json, check-merge-conflict, check-added-large-files, no-commit-to-branch (main), ruff check+format, gitleaks, hadolint
Applied to files:
CLAUDE.md
🧬 Code graph analysis (13)
cli/internal/verify/digest.go (1)
cli/internal/verify/verify.go (2)
ImageRef(27-32)IsValidDigest(96-98)
cli/internal/verify/digest_test.go (2)
cli/internal/verify/verify.go (1)
ImageRef(27-32)cli/internal/verify/digest.go (1)
ResolveDigest(14-30)
cli/internal/verify/identity_test.go (1)
cli/internal/verify/identity.go (3)
ExpectedSANRegex(24-24)ImageNames(37-37)BuildIdentityPolicy(66-75)
cli/internal/compose/generate_test.go (1)
cli/internal/compose/generate.go (2)
Params(34-46)Generate(66-85)
cli/cmd/start.go (4)
cli/internal/config/state.go (2)
State(16-28)Save(141-155)cli/internal/verify/verify.go (4)
VerifyImages(110-142)VerifyOptions(66-70)BuildImageRefs(84-93)VerifyResult(59-63)cli/internal/compose/generate.go (2)
ParamsFromState(49-62)Generate(66-85)cli/internal/version/version.go (1)
Version(9-9)
cli/internal/verify/cosign_test.go (2)
cli/internal/verify/verify.go (1)
ImageRef(27-32)cli/internal/verify/cosign.go (1)
VerifyCosignSignature(23-79)
cli/internal/verify/provenance_test.go (2)
cli/internal/verify/verify.go (1)
ImageRef(27-32)cli/internal/verify/provenance.go (1)
VerifyProvenance(33-53)
cli/internal/config/state.go (1)
cli/internal/config/paths.go (1)
DataDir(18-31)
cli/internal/verify/verify_test.go (2)
cli/internal/verify/verify.go (6)
ImageRef(27-32)NewImageRef(74-80)BuildImageRefs(84-93)IsValidDigest(96-98)VerifyImages(110-142)VerifyOptions(66-70)cli/internal/verify/identity.go (1)
RegistryHost(27-27)
cli/internal/verify/fuzz_test.go (2)
cli/internal/verify/verify.go (2)
IsValidDigest(96-98)NewImageRef(74-80)cli/internal/verify/identity.go (2)
RegistryHost(27-27)ImageRepoPrefix(30-30)
cli/internal/verify/provenance.go (1)
cli/internal/verify/verify.go (1)
ImageRef(27-32)
cli/internal/verify/verify.go (4)
cli/internal/verify/identity.go (4)
ImageRepoPrefix(30-30)RegistryHost(27-27)BuildVerifier(42-62)BuildIdentityPolicy(66-75)cli/internal/verify/digest.go (1)
ResolveDigest(14-30)cli/internal/verify/cosign.go (1)
VerifyCosignSignature(23-79)cli/internal/verify/provenance.go (1)
VerifyProvenance(33-53)
cli/internal/verify/cosign.go (1)
cli/internal/verify/verify.go (1)
ImageRef(27-32)
🪛 GitHub Check: CodeQL
cli/cmd/start.go
[failure] 136-136: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
cli/cmd/update.go
[failure] 243-243: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
🔇 Additional comments (29)
cli/internal/selfupdate/sigstore.go (3)
104-130: LGTM!The
assertSLSAProvenancefunction is well-structured with proper defensive checks:
- Gracefully handles non-DSSE bundles as a no-op (backward compatible with older message-signature bundles)
- Validates payload type before JSON parsing
- Uses prefix matching for predicate type, correctly accommodating SLSA provenance versions (v0.2, v1, etc.)
- Clear error messages include both actual and expected values for debugging
84-90: LGTM!The SLSA provenance check is correctly integrated after the core bundle verification, ensuring the signature and identity are validated before checking provenance predicates. Error wrapping provides clear context.
24-24: Regex pattern is correct; no issues found.The SAN regex pattern at line 24 is properly formatted and works as intended. All test cases pass:
- Valid version tags (v1.0.0, v1.2.3-beta.1, v0.1.0+build.123) correctly match
- Invalid workflows (other.yml) and refs (refs/heads) are correctly rejected
The escaped dots (
\.) in the pattern are necessary and correct—in regex, an unescaped dot matches any character, so escaping it to\.is the proper way to match a literal dot. This is standard regex practice, not a potential confusion.cli/internal/selfupdate/sigstore_test.go (2)
14-32: LGTM!Good test coverage for the happy path with a valid SLSA provenance v1 predicate type.
34-110: LGTM!Comprehensive test coverage for error paths:
- Wrong predicate type correctly rejected
- Wrong payload type correctly rejected
- Non-DSSE bundles (message signatures) accepted silently as expected
- Invalid JSON payloads produce errors
The tests correctly validate both the error conditions and error message content where appropriate.
cli/go.mod (2)
5-14: LGTM!The direct dependencies are well-chosen for the verification feature:
go-containerregistryfor OCI registry interactions (tag→digest resolution, referrers API)sigstore-goandsigstore/protobuf-specsfor cosign signature and SLSA provenance verificationgo-tuffor Sigstore root of trust
8-9: Dependencies are valid and current.The direct dependencies
go-containerregistry v0.21.2andgo-runewidth v0.0.21both exist and are the current latest versions for their respective packages..github/workflows/cli.yml (1)
174-174: LGTM!Adding
./internal/verify/to the fuzz test matrix appropriately extends coverage to the new verification package, which handles untrusted input from container registries (digests, signatures, provenance attestations).README.md (1)
49-49: LGTM!The documentation accurately describes the new security capability: cosign-signed images with SLSA L3 provenance verified by the CLI at pull time. This aligns with the CI workflow that signs images and attests provenance.
cli/cmd/root.go (1)
13-16: LGTM!The
skipVerifypackage-private variable and corresponding--skip-verifypersistent flag follow the existing pattern fordataDir. The flag description appropriately warns users that skipping verification is not recommended.cli/internal/config/state.go (1)
16-28: LGTM on the struct field addition.The
VerifiedDigestsfield is appropriately typed asmap[string]stringwithomitemptyto avoid serializing when empty. The zero value (nil map) is safe for JSON unmarshaling.cli/internal/verify/digest.go (1)
14-29: LGTM! Consider documenting authentication assumptions.The implementation correctly resolves tags to digests using OCI registry HEAD requests and validates the returned digest. Context propagation and error wrapping are handled properly.
Consider adding a comment noting whether this function assumes public registries or if authentication is handled at a different layer (e.g., Docker config credentials).
cli/internal/compose/compose.yml.tmpl (2)
6-6: LGTM!The
digestPintemplate function correctly wraps image references to enable digest pinning when verified digests are available, falling back to tag-based references otherwise. This approach maintains backward compatibility while enabling deterministic, verified deployments.
54-54: Consistent digest pinning across all services.The
digestPinpattern is applied consistently to backend, web, and sandbox services, ensuring all images benefit from digest pinning after verification.Also applies to: 85-85
cli/cmd/start.go (4)
64-67: LGTM!The verification step is correctly integrated before the pull operation, ensuring images are verified before being pulled. The flow is clear and follows a secure-by-default approach.
96-122: Well-structured verification flow.The
verifyAndPinImagesfunction:
- Correctly short-circuits with a warning when
--skip-verifyis set- Builds image refs dynamically based on config (tag, sandbox enabled)
- Provides clear error messaging with guidance for air-gapped environments
- Persists verified digests with graceful degradation (warns but continues on save failure)
135-138: CodeQL path injection concern is mitigated bySecurePathvalidation.The static analysis tool flags this path as user-controlled, but
safeDiris derived fromsafeStateDir(state)which callsconfig.SecurePath(). TheSecurePathfunction validates the path is absolute and resolves symlinks to prevent traversal attacks.Consider adding a brief comment to document this security control for future reviewers:
📝 Optional: Add clarifying comment
func pinDigestsInCompose(state config.State, results []verify.VerifyResult, safeDir string) error { params := compose.ParamsFromState(state) params.CLIVersion = version.Version params.DigestPins = digestPinMap(results) composeYAML, err := compose.Generate(params) if err != nil { return fmt.Errorf("generating compose file: %w", err) } composePath := filepath.Join(safeDir, "compose.yml") + // safeDir is validated by SecurePath in the caller, preventing path traversal. if err := os.WriteFile(composePath, composeYAML, 0o600); err != nil { return fmt.Errorf("writing compose file: %w", err) } return nil }
142-152: LGTM!The
digestPinMaphelper correctly transforms verification results into the map format expected by compose generation, safely handling empty digests.cli/cmd/update.go (1)
215-249: This concern is invalid; no action required.
ParamsFromStatedoes not mapstate.VerifiedDigeststoDigestPins—it explicitly omits it. The firstregenerateCompose(state, tag, safeDir, effectiveVersion)call uses the newtagand renders withDigestPins: nil, which falls back to the tag. Both the--skip-verifyand normal paths render correctly: skip-verify keeps the new tag, and the normal path overlays verified digests on a re-render. No stale pin risk exists.> Likely an incorrect or invalid review comment.cli/internal/verify/verify_test.go (1)
1-150: LGTM! Well-structured unit tests for the verify package public API.The tests cover the essential public API surface including
ImageRefmethods,BuildImageRefs,IsValidDigest, and early-exit paths inVerifyImages. Table-driven tests are used appropriately forTestImageRefNameandTestIsValidDigest.One observation: the tests for
VerifyImagesonly cover the early-exit paths (SkipVerify=trueandImages=nil). The actual verification flow with real images would require registry mocking, which may be covered by integration tests elsewhere.cli/internal/compose/generate.go (1)
118-135: LGTM! Digest pin validation and template helper are correctly implemented.The
validateParamsextension properly validates each digest inDigestPins, and thedigestPinhelper correctly handles nil maps and empty digest values with a clean fallback to tag-based references.cli/internal/verify/cosign.go (1)
23-79: LGTM! Cosign signature verification flow is well-structured.The implementation correctly:
- Validates digest presence before proceeding
- Constructs the deterministic cosign signature tag
- Fetches and iterates signature layers
- Tries each bundle until one verifies successfully
- Provides clear error messages distinguishing "no bundle found" from "verification failed"
cli/internal/verify/identity.go (2)
16-34: LGTM! Identity constants are well-defined with appropriate security constraints.The
ExpectedSANRegexcorrectly restricts signatures to:
- Only the
docker.ymlworkflow (not arbitrary workflows)- Only version tags (
v*.*.*) or themainbranch (not feature branches)- Includes proper escaping for regex metacharacters
This aligns with the PR objective to "restrict SAN regex to specific workflow/refs."
42-75: LGTM! Verifier and identity policy construction is correct.The implementation correctly:
- Configures a custom HTTP timeout for TUF metadata fetches
- Requires SCTs, transparency log entries, and integrated timestamps (appropriate for keyless verification)
- Builds the certificate identity with the expected OIDC issuer and SAN regex
The verifier is built once per
VerifyImagescall and reused across all images, as noted in the PR objectives.CLAUDE.md (1)
108-109: LGTM! Documentation accurately reflects the new verification feature.The additions clearly document:
- CLI verification of cosign signatures and SLSA provenance at pull time
- The
--skip-verifyflag for air-gapped environments- The new
internal/verify/package and its purposecli/internal/verify/verify.go (2)
110-142: LGTM! VerifyImages correctly implements the verification flow.The implementation:
- Short-circuits early when
SkipVerifyis true or no images are provided- Builds the verifier and identity policy once and reuses across all images (efficient)
- Provides clear error context by wrapping errors with the image reference
- Handles nil
Outputwriter gracefully
144-178: LGTM! Per-image verification with appropriate warn-only handling for provenance.The
verifyOneImagefunction correctly:
- Resolves the digest first (required for signature verification)
- Fails hard on cosign signature verification failure (security-critical)
- Treats SLSA provenance as warn-only (continues with
ProvenanceVerified=falseon error)- Provides clear progress output for each verification step
This matches the PR objective for "warn-only if missing" provenance.
cli/internal/verify/provenance.go (2)
28-53: LGTM! Provenance verification entry point is well-structured.The
VerifyProvenancefunction correctly:
- Validates digest presence before proceeding
- Finds attestations via OCI referrers API
- Tries each attestation until one verifies successfully
- Provides clear error messages on failure
The error message at line 52 safely handles
lastErrsincefindAttestationsreturns an error when no attestations are found, ensuring this path is only reached with at least one failed verification attempt.
144-164: LGTM! SLSA predicate validation is correct and version-flexible.The
validateSLSAPredicatefunction:
- Validates the DSSE payload type is
application/vnd.in-toto+json- Properly base64 decodes and parses the in-toto statement
- Uses prefix matching for the predicate type, allowing any SLSA provenance version (e.g.,
v0.2,v1.0)
… and CodeQL - Remove dead SkipVerify field from VerifyOptions (callers handle skip logic) - Remove redundant CosignVerified field (always true on success) - Deduplicate constants (SLSAProvenancePredicatePrefix, DSSEPayloadType, InTotoStatement) between verify and selfupdate packages - Deduplicate digestPattern regex: compose uses verify.IsValidDigest - Extract verifyAndPinForUpdate to bring pullAndPersist under 50 lines - Remove embedded \n from error strings (non-idiomatic Go) - Fix garbled digest in cosign_test.go fixture - Iterate all manifest layers for Sigstore bundle (not just first) - Validate sha256 algorithm in parseDigest - Add HEAD-to-GET fallback in ResolveDigest for registry compatibility - Convert ImageNames to function to prevent slice mutation - Add VerifiedDigests validation in config state.go - Add SYNTHORG_SKIP_VERIFY env var for CI/air-gapped environments - Add 120s context timeout for OCI registry calls - Distinguish provenance "missing" from "crypto failure" (missing=warn, crypto=hard error) - Add sha256 prefix assertion in FuzzIsValidDigest - Fix json.Marshal error handling in sigstore_test.go - Update CLAUDE.md: selfupdate description, CLI deps - Update docs/security.md: client-side verification section - Update docs/user_guide.md: verification and --skip-verify info
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
cli/internal/verify/provenance_test.go (1)
113-189: 🧹 Nitpick | 🔵 TrivialTest assertion may not isolate the intended failure path.
Per the earlier review comment, this test constructs a DSSE envelope without a valid Sigstore bundle. With the "bundle required" rule,
VerifyProvenancemay reject this before evaluating the predicate type, so theerr != nilassertion doesn't prove the specific path the test name claims.Consider either:
- Attaching a valid bundle so the test reaches predicate evaluation, or
- Renaming the test and assertion message to reflect that it's testing generic invalid-attestation behavior
♻️ Suggested fix: Update test name and assertion
-func TestVerifyProvenanceWithInvalidAttestation(t *testing.T) { +func TestVerifyProvenanceRejectsInvalidAttestation(t *testing.T) { // ... existing setup ... err := VerifyProvenance(context.Background(), ref, nil, sigverify.CertificateIdentity{}) if err == nil { - t.Fatal("expected error for wrong predicate type") + t.Fatal("expected error for invalid attestation") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/internal/verify/provenance_test.go` around lines 113 - 189, The test TestVerifyProvenanceWithInvalidAttestation currently may fail before predicate checking because VerifyProvenance enforces a bundle requirement; rename the test and update its failure message to reflect generic invalid-attestation behavior (e.g., TestVerifyProvenance_InvalidAttestation or TestVerifyProvenanceWithoutBundle) and change the t.Fatal string from "expected error for wrong predicate type" to something like "expected error for invalid attestation"; update the function name and any references to it so the test intent matches the constructed DSSE envelope rather than claiming predicate-specific failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/cmd/start.go`:
- Around line 128-156: pinDigestsInCompose duplicates compose regeneration logic
also present in verifyAndPinForUpdate; extract the shared behavior into a single
helper (e.g., writeDigestPinnedCompose) that takes (state config.State,
digestPins map[string]string, safeDir, cliVersion string) and performs
ParamsFromState, sets CLIVersion and DigestPins, calls compose.Generate and
writes compose.yml with 0o600, then replace the body of pinDigestsInCompose and
the identical block in verifyAndPinForUpdate to call this helper; ensure the
helper is placed in a common package/file accessible to both start.go and
update.go so both call the same function.
In `@cli/internal/verify/digest_test.go`:
- Around line 108-148: The test TestResolveDigestHEADFallbackToGET currently
only checks IsValidDigest(digest) but should assert the exact digest computed
from the returned manifest body; update the test to compute the expected digest
for manifestBody (the canonical sha256 digest used by go-containerregistry for
the JSON payload returned when Docker-Content-Digest is absent) and replace or
augment the IsValidDigest assertion with an equality check (e.g., expectedDigest
== digest) so ResolveDigest (called in the test) is validated against the known
correct digest for the `manifestBody` used in the mock server.
In `@cli/internal/verify/digest.go`:
- Around line 26-29: The fallback GET in the manifest fetch is wrapping the
wrong error variable; inside the remote.Get fallback block (where rd, getErr :=
remote.Get(tagRef, ...)), replace the wrapped error variable from err to getErr
so the returned error uses the actual GET failure (use getErr in fmt.Errorf)
while keeping the same contextual message referencing ref and tagRef.
In `@cli/internal/verify/fuzz_test.go`:
- Around line 48-51: Replace the manual slice check for the ".sig" suffix in the
cosignSigTag test with strings.HasSuffix for clarity and consistency with
FuzzIsValidDigest; update imports to include "strings" if necessary and use
strings.HasSuffix(tag, ".sig") in the condition while preserving the existing
t.Errorf call and message and the surrounding logic in the test that computes
tag (cosignSigTag).
In `@cli/internal/verify/provenance.go`:
- Around line 225-228: Rename the exported type InTotoStatement to an unexported
inTotoStatement to match dsseEnvelope's visibility, keep the json tag
`json:"predicateType"` unchanged, and update all references/usages of
InTotoStatement (including the parsing/decoding site that currently constructs
or type-asserts this struct) to use inTotoStatement so compilation and JSON
decoding continue to work.
- Around line 47-57: The current loop only preserves the final verification
error in lastErr, losing earlier failures; change verify loop in the function
that iterates over attestationDescs to collect all verification errors (e.g.,
create a slice like var errs []error and for each failed verifyAttestation(ctx,
ref, desc, sev, certID) append a contextualized error including the attestation
identifier or index), continue on failure and return nil on success, and when
none succeed return a combined error (use errors.Join(errs...) or fmt.Errorf("no
valid SLSA provenance attestation for %s: %v", ref, errs) and add errors to
imports if needed) so callers see all failure reasons rather than only lastErr.
In `@cli/internal/verify/verify_test.go`:
- Around line 64-75: The test TestNewImageRef hardcodes the expected repository
string; update the assertion to build the expected repository using the
ImageRepoPrefix constant instead of the literal "aureliolo/synthorg-backend".
Locate TestNewImageRef and replace the hardcoded repository expectation with
ImageRepoPrefix + "/synthorg-backend" (or equivalent concatenation) when
comparing against ref.Repository so the test stays correct if ImageRepoPrefix
changes.
In `@cli/internal/verify/verify.go`:
- Around line 187-191: Replace fragile substring checks in isProvenanceMissing
with a sentinel error and errors.Is: define a package-level sentinel (e.g.
ErrNoProvenanceAttestations) in the provenance-related file, update
findAttestations to return that sentinel wrapped with fmt.Errorf("%w: %s",
ErrNoProvenanceAttestations, ref) when no attestations are found, and change
isProvenanceMissing to use errors.Is(err, ErrNoProvenanceAttestations) instead
of string matching; this makes provenance detection robust to message changes.
- Around line 34-37: ImageRef.String currently always formats "%s/%s:%s" which
yields a trailing colon when ImageRef.Tag is empty; update ImageRef.String to
check Tag and, if empty, return the registry/repository without the ":" (e.g.,
"%s/%s"), otherwise return "%s/%s:%s"; reference the ImageRef.String method and
the ImageRef fields Registry, Repository, and Tag when making the change so the
output never produces a malformed "registry/repo:" string.
---
Duplicate comments:
In `@cli/internal/verify/provenance_test.go`:
- Around line 113-189: The test TestVerifyProvenanceWithInvalidAttestation
currently may fail before predicate checking because VerifyProvenance enforces a
bundle requirement; rename the test and update its failure message to reflect
generic invalid-attestation behavior (e.g.,
TestVerifyProvenance_InvalidAttestation or TestVerifyProvenanceWithoutBundle)
and change the t.Fatal string from "expected error for wrong predicate type" to
something like "expected error for invalid attestation"; update the function
name and any references to it so the test intent matches the constructed DSSE
envelope rather than claiming predicate-specific failure.
🪄 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: 185b874c-70c3-4662-bd91-00198056733f
📒 Files selected for processing (21)
CLAUDE.mdcli/cmd/root.gocli/cmd/start.gocli/cmd/update.gocli/internal/compose/generate.gocli/internal/config/state.gocli/internal/selfupdate/sigstore.gocli/internal/selfupdate/sigstore_test.gocli/internal/verify/cosign.gocli/internal/verify/cosign_test.gocli/internal/verify/digest.gocli/internal/verify/digest_test.gocli/internal/verify/fuzz_test.gocli/internal/verify/identity.gocli/internal/verify/identity_test.gocli/internal/verify/provenance.gocli/internal/verify/provenance_test.gocli/internal/verify/verify.gocli/internal/verify/verify_test.godocs/security.mddocs/user_guide.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (1)
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Lint CLI Go code with golangci-lint and go vet; test with go test -race; check vulnerabilities with govulncheck
Files:
cli/internal/selfupdate/sigstore.gocli/cmd/root.gocli/cmd/start.gocli/internal/verify/digest_test.gocli/cmd/update.gocli/internal/verify/verify_test.gocli/internal/verify/identity_test.gocli/internal/verify/identity.gocli/internal/verify/provenance_test.gocli/internal/verify/digest.gocli/internal/verify/fuzz_test.gocli/internal/selfupdate/sigstore_test.gocli/internal/verify/provenance.gocli/internal/verify/cosign.gocli/internal/verify/verify.gocli/internal/config/state.gocli/internal/verify/cosign_test.gocli/internal/compose/generate.go
🧠 Learnings (19)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to .github/workflows/docker.yml : CI Docker: build → scan → push to GHCR + cosign sign + SLSA L3 provenance via attest-build-provenance (images only pushed after Trivy/Grype scans pass).
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).
Applied to files:
cli/cmd/root.gocli/cmd/start.gocli/cmd/update.goCLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to .github/workflows/docker.yml : CI Docker: build → scan → push to GHCR + cosign sign + SLSA L3 provenance via attest-build-provenance (images only pushed after Trivy/Grype scans pass).
Applied to files:
docs/security.mdCLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to docker/{Dockerfile*,compose.yml} : Docker: Backend uses 3-stage build (builder → setup → distroless runtime), Chainguard Python, non-root (UID 65532), CIS-hardened. Web uses nginxinc/nginx-unprivileged, Vue 3 SPA with PrimeVue + Tailwind CSS, SPA routing, API/WebSocket proxy to backend.
Applied to files:
docs/security.mdCLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to cli/**/*.go : Lint CLI Go code with golangci-lint and go vet; test with go test -race; check vulnerabilities with govulncheck
Applied to files:
cli/cmd/start.gocli/internal/verify/identity_test.gocli/internal/verify/provenance_test.goCLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Dependabot: auto-updates Docker image digests and versions daily.
Applied to files:
cli/cmd/update.goCLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to cli/go.mod : CLI requires Go 1.26+; dependencies in `cli/go.mod` (Cobra, charmbracelet/huh, charmbracelet/lipgloss)
Applied to files:
cli/cmd/update.goCLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to docker/** : Docker build context: single root .dockerignore (both images build with context: .). Tags: CI tags images with version from pyproject.toml ([tool.commitizen].version), semver, and SHA.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Lint Python code with ruff (check + format); type-check with mypy strict mode; test with pytest + coverage ≥80%
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Shell commands: for Go CLI work, cd cli is an exception because Go tooling requires working directory to be the module root. Go commands require `cd cli` for other work, never use `cd`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Pre-push hooks: mypy type-check + pytest unit tests + golangci-lint + go vet + go test (CLI, conditional on cli/**/*.go) (fast gate before push, skipped in pre-commit.ci — dedicated CI jobs already run these).
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to .github/workflows/**/*.yml : Path filtering: dorny/paths-filter detects Python/dashboard/docker changes; jobs only run when their domain is affected. CLI has its own workflow (cli.yml).
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Pre-commit hooks: trailing-whitespace, end-of-file-fixer, check-yaml, check-toml, check-json, check-merge-conflict, check-added-large-files, no-commit-to-branch (main), ruff check+format, gitleaks, hadolint (Dockerfile linting).
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.027Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.027Z
Learning: Pre-push hooks enforce: mypy type-check + pytest unit tests + golangci-lint + go vet + go test (CLI, conditional); skip in pre-commit.ci as dedicated CI jobs run these
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Commits: <type>: <description> — types: feat, fix, refactor, docs, test, chore, perf, ci. Enforced by commitizen (commit-msg hook). Signed commits: required on main via branch protection — all commits must be GPG/SSH signed.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.027Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.027Z
Learning: Pre-commit hooks enforce: trailing-whitespace, end-of-file-fixer, check-yaml, check-toml, check-json, check-merge-conflict, check-added-large-files, no-commit-to-branch (main), ruff check+format, gitleaks, hadolint
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to pyproject.toml : Dependencies: all versions use == in pyproject.toml. Groups: test (pytest + plugins, hypothesis), dev (includes test + ruff, mypy, pre-commit, commitizen, pip-audit). Required: mem0ai (Mem0 memory backend — the default and currently only backend). Install: uv sync installs everything (dev group is default).
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to web/package.json : Web dashboard requires Node.js 20+; dependencies in `web/package.json` (Vue 3, PrimeVue, Tailwind CSS, Pinia, VueFlow, ECharts, Axios, vue-draggable-plus, Vitest, fast-check, ESLint, vue-tsc)
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to web/** : Web dashboard: Node.js 20+, dependencies in web/package.json (Vue 3, PrimeVue, Tailwind CSS, Pinia, VueFlow, ECharts, Axios, vue-draggable-plus, Vitest, fast-check, ESLint, vue-tsc).
Applied to files:
CLAUDE.md
🧬 Code graph analysis (13)
cli/internal/selfupdate/sigstore.go (1)
cli/internal/verify/provenance.go (3)
DSSEPayloadType(23-23)InTotoStatement(226-228)SLSAProvenancePredicatePrefix(19-19)
cli/cmd/start.go (4)
cli/internal/config/state.go (2)
State(16-28)Save(160-174)cli/internal/verify/verify.go (4)
VerifyImages(111-140)VerifyOptions(67-70)BuildImageRefs(84-93)VerifyResult(61-64)cli/internal/compose/generate.go (2)
ParamsFromState(49-62)Generate(66-85)cli/internal/version/version.go (1)
Version(9-9)
cli/internal/verify/digest_test.go (2)
cli/internal/verify/verify.go (2)
ImageRef(27-32)IsValidDigest(96-98)cli/internal/verify/digest.go (1)
ResolveDigest(17-38)
cli/cmd/update.go (4)
cli/internal/ui/ui.go (2)
NewUI(58-70)UI(44-53)cli/internal/config/state.go (1)
State(16-28)cli/internal/verify/verify.go (3)
VerifyImages(111-140)VerifyOptions(67-70)BuildImageRefs(84-93)cli/internal/compose/generate.go (2)
ParamsFromState(49-62)Generate(66-85)
cli/internal/verify/verify_test.go (2)
cli/internal/verify/verify.go (6)
ImageRef(27-32)NewImageRef(74-80)BuildImageRefs(84-93)IsValidDigest(96-98)VerifyImages(111-140)VerifyOptions(67-70)cli/internal/verify/identity.go (1)
RegistryHost(27-27)
cli/internal/verify/identity_test.go (1)
cli/internal/verify/identity.go (3)
ExpectedSANRegex(24-24)ImageNames(38-38)BuildIdentityPolicy(67-76)
cli/internal/verify/digest.go (1)
cli/internal/verify/verify.go (2)
ImageRef(27-32)IsValidDigest(96-98)
cli/internal/verify/fuzz_test.go (2)
cli/internal/verify/verify.go (2)
IsValidDigest(96-98)NewImageRef(74-80)cli/internal/verify/identity.go (2)
RegistryHost(27-27)ImageRepoPrefix(30-30)
cli/internal/selfupdate/sigstore_test.go (1)
cli/internal/verify/provenance.go (1)
InTotoStatement(226-228)
cli/internal/verify/provenance.go (1)
cli/internal/verify/verify.go (1)
ImageRef(27-32)
cli/internal/verify/cosign.go (1)
cli/internal/verify/verify.go (1)
ImageRef(27-32)
cli/internal/verify/cosign_test.go (2)
cli/internal/verify/verify.go (1)
ImageRef(27-32)cli/internal/verify/cosign.go (1)
VerifyCosignSignature(23-79)
cli/internal/compose/generate.go (1)
cli/internal/verify/verify.go (1)
IsValidDigest(96-98)
🪛 GitHub Check: CodeQL
cli/cmd/update.go
[failure] 276-276: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
🔇 Additional comments (48)
docs/user_guide.md (1)
19-24: LGTM!Documentation clearly explains the new verification behavior and provides appropriate guidance for users in air-gapped environments who need to bypass verification.
docs/security.md (1)
126-126: LGTM!Security documentation accurately describes the new client-side verification workflow, digest pinning for tag mutation protection, and bypass options with appropriate warnings. This aligns well with the CI pipeline that generates cosign signatures and SLSA L3 provenance attestations.
cli/internal/verify/digest.go (1)
33-37: LGTM!Digest validation using
IsValidDigestbefore returning ensures malformed registry responses are rejected.cli/internal/selfupdate/sigstore.go (2)
86-91: LGTM!The SLSA provenance assertion is correctly integrated after standard bundle verification, ensuring bundles with DSSE envelopes contain valid SLSA predicates while maintaining backward compatibility with plain message signatures.
102-124: LGTM!The
assertSLSAProvenancefunction correctly:
- Allows non-DSSE bundles to pass (backward compatibility)
- Validates DSSE payload type before parsing
- Uses prefix matching for SLSA predicate type (supports multiple SLSA versions)
- Reuses constants from the verify package to avoid duplication
cli/internal/verify/cosign.go (3)
23-79: LGTM!The cosign signature verification flow is well-structured:
- Validates digest presence before proceeding
- Uses deterministic signature tag derivation
- Iterates all layers to find the bundle annotation (handles multi-layer signature images)
- Properly distinguishes between "no bundle found" and "verification failed" errors
112-128: LGTM!The
parseDigestfunction now validates that onlysha256algorithm is supported, addressing the previous review feedback about rejecting unsupported digest algorithms.
130-137: BothnewBundleandsigstoreBundleare properly defined in the verify package (incli/internal/verify/verify.go), withsigstoreBundlebeing a type alias forbundle.Bundle. No issues found.cli/internal/verify/cosign_test.go (3)
16-65: LGTM!Tests for
cosignSigTagandparseDigestprovide good coverage of the helper functions, including edge cases like missing colons and invalid hex.
67-105: LGTM!Tests for empty digest and missing signature artifact correctly verify error handling paths using a mock HTTP server.
128-191: LGTM!The test fixture now uses a valid 64-character sha256 hex digest (line 141), addressing the previous review feedback. The mock registry setup correctly simulates an OCI manifest with an invalid cosign bundle to test the bundle parsing error path. The
testDigestconstant is properly defined in the package.cli/internal/selfupdate/sigstore_test.go (2)
16-37: LGTM!Test correctly validates the happy path for SLSA provenance assertion with a valid predicate type. The
json.Marshalerror is now properly checked (addressing previous feedback).
39-118: LGTM!Comprehensive test coverage for error paths:
- Wrong predicate type detection
- Wrong DSSE payload type detection
- Non-DSSE bundles pass silently (backward compatibility)
- Invalid JSON payload handling
All tests properly verify specific error messages where applicable.
cli/internal/verify/identity.go (2)
16-34: LGTM!Constants are well-defined:
ExpectedSANRegexcorrectly restricts signatures to thedocker.ymlworkflow on version tags ormainbranch, preventing acceptance of signatures from arbitrary workflows or feature branchesTUFFetchTimeoutprovides reasonable bounds (30s) for TUF metadata fetches
36-76: LGTM!The verification infrastructure is well-structured:
ImageNames()returns a new slice on each call to prevent callers from mutating the canonical listBuildVerifier()correctly configures the Sigstore verifier with SCTs, transparency log entries, and integrated timestamps (aligning with sigstore-go v1.1+ requirements)BuildIdentityPolicy()creates the certificate identity with proper issuer and SAN regex constraints- Both builders properly wrap errors with context
cli/internal/verify/identity_test.go (3)
8-22: LGTM!Good coverage of valid SAN references including semver tags, pre-release versions (
-rc.1), build metadata (+build.456), and themainbranch. The test structure is clear and follows Go testing conventions.
24-41: LGTM!Comprehensive negative test cases covering security-relevant scenarios: wrong owner, wrong repo, wrong host, wrong workflow file, unauthorized branch patterns, and edge cases (empty string, random string).
43-63: LGTM!The
ImageNamestest correctly validates bidirectional completeness (no extras, no missing). TheBuildIdentityPolicysmoke test ensures the policy construction doesn't fail.CLAUDE.md (3)
108-108: LGTM!Documentation accurately describes the new verification behavior and clearly notes the
--skip-verifybypass for air-gapped environments.
148-159: LGTM!Package structure documentation correctly reflects the new
--skip-verifyroot flag, the enhanced selfupdate verification, and the newverify/package.
269-269: LGTM!CI documentation and dependency list accurately reflect the fuzz testing expansion and new Sigstore-related dependencies.
Also applies to: 292-292
cli/cmd/root.go (2)
14-17: LGTM!Clean refactor grouping related state variables into a single
varblock.
32-41: LGTM!The
--skip-verifyflag with environment variable fallback is well-implemented:
- Clear warning in the flag description
- Case-insensitive env var parsing accepting common truthy values
- Env var sets the initial default, allowing explicit flag values to override
cli/internal/config/state.go (2)
17-27: LGTM!The
VerifiedDigestsfield withomitemptyis appropriate for optional digest caching. The JSON tag follows existing naming conventions.
136-156: LGTM!The digest validation is correctly implemented:
- Validates exact length (71 = 7 prefix + 64 hex chars)
- Enforces lowercase hex per OCI spec
- Clear comment explaining the circular dependency avoidance rationale
cli/cmd/start.go (2)
64-67: LGTM!Good integration point - verification happens before pulling, ensuring only verified images are pulled.
96-126: LGTM!The verification flow is well-structured:
- Early return with warning when
--skip-verifyis set- 120s timeout prevents indefinite hangs on registry issues
- Helpful hint for air-gapped environments on failure
- Digest pins are persisted to state with graceful degradation on save failure
cli/internal/compose/generate.go (4)
13-13: LGTM!Good deduplication - importing
verify.IsValidDigestinstead of duplicating the digest pattern regex. The comment clearly explains this design choice.Also applies to: 23-24
45-45: LGTM!Clear documentation of the
DigestPinsfield's purpose with the example mapping format.
71-74: LGTM!Template function registration is clean with proper closure capture for
DigestPins.
118-135: LGTM!The digest pin validation and template function are well-implemented:
- Validates all pins before template execution (fail-fast)
digestPinclosure defensively checks both map existence and non-empty value- Returns appropriate format:
repo@digestfor pinned,repo:tagfor unpinnedcli/internal/verify/provenance_test.go (3)
16-29: LGTM!Good test for the empty digest edge case - verifies the function correctly validates that digest resolution happened before provenance verification.
31-63: LGTM!Well-structured test using
httptestto mock OCI registry responses. Correctly tests the "no SLSA provenance attestations found" error path.
65-111: LGTM!Good unit tests for
validateSLSAPredicatecovering:
- Valid SLSA predicate type acceptance
- Wrong predicate type rejection
- Wrong payload type rejection
These directly test the predicate validation logic in isolation.
cli/cmd/update.go (3)
216-223: LGTM!Good integration - verification runs after initial compose regeneration but before image pull. Rollback is correctly triggered on verification failure.
243-280: CodeQL path-injection finding is a false positive.The static analysis flags Line 276 for "uncontrolled data used in path expression." However,
safeDiroriginates fromsafeStateDir()which callsconfig.SecurePath()to validate and canonicalize the path. The path is already validated upstream.The verification flow logic is correct:
- Skip with warning when
--skip-verifyis set- 120s timeout bounds registry operations
- Helpful hint on failure for air-gapped environments
- Compose regenerated with digest pins before returning
Note: This function has significant overlap with
pinDigestsInComposeinstart.go(Lines 128-144). Consider the refactoring suggestion from thestart.goreview.
233-235: LGTM!The
VerifiedDigestsare correctly persisted alongside the updatedImageTagafter successful image pull.cli/internal/verify/fuzz_test.go (2)
10-30: LGTM - sha256 prefix assertion now included.The fuzz test correctly validates both the
sha256:prefix (line 22) and the 71-character length (line 25) for valid digests, addressing the prior review feedback.
57-79: LGTM - solid fuzz test for ImageRef construction.The test appropriately guards against excessive input sizes (line 64-66), validates all relevant fields against expected values, and uses the exported constants (
RegistryHost,ImageRepoPrefix) for assertions.cli/internal/verify/digest_test.go (2)
16-34: LGTM - clean mock registry implementation.The helper correctly simulates OCI distribution spec behavior:
/v2/version check, manifest endpoint for HEAD/GET with proper headers (Docker-Content-Digest,Content-Type), and 404 for unmatched paths.
150-168: LGTM - context cancellation test.Correctly tests that
ResolveDigestrespects context cancellation by immediately canceling before the call.cli/internal/verify/verify_test.go (2)
106-123: LGTM - comprehensive digest validation coverage.The table-driven test covers key edge cases: valid lowercase hex, all-zeros, truncated hash, wrong algorithm prefix, empty string, and uppercase rejection. This aligns well with the
^sha256:[a-f0-9]{64}$pattern.
125-136: LGTM - correctly tests nil return for empty images.The test verifies that
VerifyImagesreturnsnil, nilwhen no images are provided, which matches the early-return behavior inverify.goline 112-114.cli/internal/verify/verify.go (2)
15-16: LGTM - digest pattern correctly enforces lowercase hex.The regex
^sha256:[a-f0-9]{64}$properly validates OCI content digests per the specification, rejecting uppercase characters and non-sha256 algorithms.
111-140: LGTM - efficient verifier reuse across images.The implementation correctly builds the Sigstore verifier and identity policy once (lines 122-129) and reuses them for all images, avoiding redundant TUF root fetches as documented in the PR objectives.
cli/internal/verify/provenance.go (3)
16-30: LGTM - well-documented constants with appropriate scoping.The exported constants (
SLSAProvenancePredicatePrefix,DSSEPayloadType) are documented for reuse by selfupdate, whileslsaReferrerArtifactTypeis correctly kept unexported. The comment on lines 26-29 clarifies the intentional match between artifact type and DSSE payload type.
184-190: LGTM - now iterates all layers for Sigstore bundle.The implementation correctly checks all layers' annotations for the Sigstore bundle, addressing the prior review feedback about only checking the first layer.
197-217: BothloadBundleandparseDigestare properly defined in the package.These functions are defined in
cli/internal/verify/cosign.goand are correctly accessible fromprovenance.goas they share the same package. No action needed.
| func TestNewImageRef(t *testing.T) { | ||
| ref := NewImageRef("backend", "0.3.0") | ||
| if ref.Registry != RegistryHost { | ||
| t.Errorf("Registry = %q, want %q", ref.Registry, RegistryHost) | ||
| } | ||
| if ref.Repository != "aureliolo/synthorg-backend" { | ||
| t.Errorf("Repository = %q, want aureliolo/synthorg-backend", ref.Repository) | ||
| } | ||
| if ref.Tag != "0.3.0" { | ||
| t.Errorf("Tag = %q, want 0.3.0", ref.Tag) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using ImageRepoPrefix constant for maintainability.
The test hardcodes "aureliolo/synthorg-backend" instead of using the ImageRepoPrefix constant. If the prefix changes, this test would need manual updates.
♻️ Use constant for repository assertion
func TestNewImageRef(t *testing.T) {
ref := NewImageRef("backend", "0.3.0")
if ref.Registry != RegistryHost {
t.Errorf("Registry = %q, want %q", ref.Registry, RegistryHost)
}
- if ref.Repository != "aureliolo/synthorg-backend" {
- t.Errorf("Repository = %q, want aureliolo/synthorg-backend", ref.Repository)
+ wantRepo := ImageRepoPrefix + "backend"
+ if ref.Repository != wantRepo {
+ t.Errorf("Repository = %q, want %q", ref.Repository, wantRepo)
}
if ref.Tag != "0.3.0" {
t.Errorf("Tag = %q, want 0.3.0", ref.Tag)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/verify/verify_test.go` around lines 64 - 75, The test
TestNewImageRef hardcodes the expected repository string; update the assertion
to build the expected repository using the ImageRepoPrefix constant instead of
the literal "aureliolo/synthorg-backend". Locate TestNewImageRef and replace the
hardcoded repository expectation with ImageRepoPrefix + "/synthorg-backend" (or
equivalent concatenation) when comparing against ref.Repository so the test
stays correct if ImageRepoPrefix changes.
| func isProvenanceMissing(err error) bool { | ||
| msg := err.Error() | ||
| return strings.Contains(msg, "no SLSA provenance attestations found") || | ||
| strings.Contains(msg, "querying referrers") | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
String-based error classification is fragile.
isProvenanceMissing relies on substring matching of error messages. If the error text in findAttestations or upstream code changes, this classification could break silently.
Consider defining sentinel errors or using errors.Is for more robust error classification.
♻️ Use sentinel errors for robustness
// Define in provenance.go or errors.go
var ErrNoProvenanceAttestations = errors.New("no SLSA provenance attestations found")
// In findAttestations:
if len(descs) == 0 {
return nil, fmt.Errorf("%w for %s", ErrNoProvenanceAttestations, ref)
}
// In isProvenanceMissing:
func isProvenanceMissing(err error) bool {
return errors.Is(err, ErrNoProvenanceAttestations)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/verify/verify.go` around lines 187 - 191, Replace fragile
substring checks in isProvenanceMissing with a sentinel error and errors.Is:
define a package-level sentinel (e.g. ErrNoProvenanceAttestations) in the
provenance-related file, update findAttestations to return that sentinel wrapped
with fmt.Errorf("%w: %s", ErrNoProvenanceAttestations, ref) when no attestations
are found, and change isProvenanceMissing to use errors.Is(err,
ErrNoProvenanceAttestations) instead of string matching; this makes provenance
detection robust to message changes.
- Extract writeDigestPinnedCompose shared helper (DRY: start.go + update.go) - Fix digest.go: wrap getErr (not HEAD err) in GET fallback error path - Use strings.HasSuffix in FuzzCosignSigTag for clarity - Unexport InTotoStatement back to inTotoStatement (match dsseEnvelope visibility) - Restore local slsaStatement struct in selfupdate (trivial, no export needed) - Collect all attestation errors in VerifyProvenance loop (errors.Join) - Add ErrNoProvenanceAttestations sentinel for robust missing-provenance detection - Use ImageRepoPrefix constant in TestNewImageRef (avoid hardcoded string) - Handle empty Tag in ImageRef.String (no trailing colon) - Rename TestVerifyProvenanceWithInvalidAttestation to match actual test intent - Assert exact digest in TestResolveDigestHEADFallbackToGET
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cli/internal/verify/provenance_test.go (1)
113-189:⚠️ Potential issue | 🟡 MinorTest may not isolate the intended failure path.
This test constructs an attestation with a non-SLSA predicate type (
https://example.com/not-slsa) but lacks a valid Sigstore bundle. WhenVerifyProvenanceis called, it may fail atverifyAttestationBundle(which requires a bundle) before evaluating the predicate type viavalidateSLSAPredicate.The current assertion
err != nilpasses but doesn't confirm that the error is specifically due to the wrong predicate type. Consider either:
- Adding a valid bundle to test the predicate-type rejection path specifically, or
- Renaming the test and updating comments to reflect it tests general invalid-attestation handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/internal/verify/provenance_test.go` around lines 113 - 189, The test TestVerifyProvenanceInvalidAttestation currently builds an attestation with a non-SLSA predicate but no Sigstore bundle so VerifyProvenance may fail in verifyAttestationBundle before validateSLSAPredicate; either (A) add a valid bundle to the constructed envelope/manifest (so VerifyProvenance reaches validateSLSAPredicate and fails specifically for PredicateType) — update the envelope payload to include a valid Sigstore bundle and any required signatures/fields used by verifyAttestationBundle — or (B) if you intend to test a general invalid-attestation path, rename the test (and adjust its comment) to reflect it does not assert the predicate-type rejection; reference TestVerifyProvenanceInvalidAttestation, VerifyProvenance, verifyAttestationBundle, and validateSLSAPredicate when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/cmd/start.go`:
- Around line 154-161: The helper digestPinMap currently drops results with
empty Ref.Digest; change its signature to return (map[string]string, error) and
make it fail closed: in digestPinMap iterate results and if any r.Ref.Digest ==
"" return a nil map and an error (include r.Ref.Name() in the message for
context), otherwise build and return the pins map and nil error; then update the
callers (the start and update code paths that call digestPinMap) to handle the
error by aborting and propagating/logging it instead of proceeding with a
partial pin map.
In `@cli/cmd/update.go`:
- Around line 216-223: The code currently rewrites compose.yml to the new tag
before calling verifyAndPinForUpdate, which can leave the working file pointing
at an unverified tag if the process exits; change the ordering so you do NOT
persist the new compose.yml until verifyAndPinForUpdate returns successfully
(keep the original compose file in place while verifying), and only
write/replace compose.yml immediately after a successful verifyAndPinForUpdate;
preserve the existing rollback() behavior if verification fails, and make an
explicit exception to persist the new tag only when the --skip-verify flag is
set so that skipping verification still writes the file without calling
verifyAndPinForUpdate.
---
Duplicate comments:
In `@cli/internal/verify/provenance_test.go`:
- Around line 113-189: The test TestVerifyProvenanceInvalidAttestation currently
builds an attestation with a non-SLSA predicate but no Sigstore bundle so
VerifyProvenance may fail in verifyAttestationBundle before
validateSLSAPredicate; either (A) add a valid bundle to the constructed
envelope/manifest (so VerifyProvenance reaches validateSLSAPredicate and fails
specifically for PredicateType) — update the envelope payload to include a valid
Sigstore bundle and any required signatures/fields used by
verifyAttestationBundle — or (B) if you intend to test a general
invalid-attestation path, rename the test (and adjust its comment) to reflect it
does not assert the predicate-type rejection; reference
TestVerifyProvenanceInvalidAttestation, VerifyProvenance,
verifyAttestationBundle, and validateSLSAPredicate when making the change.
🪄 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: 250254fd-4504-4c13-a295-1546ab9685d1
📒 Files selected for processing (11)
cli/cmd/start.gocli/cmd/update.gocli/internal/selfupdate/sigstore.gocli/internal/selfupdate/sigstore_test.gocli/internal/verify/digest.gocli/internal/verify/digest_test.gocli/internal/verify/fuzz_test.gocli/internal/verify/provenance.gocli/internal/verify/provenance_test.gocli/internal/verify/verify.gocli/internal/verify/verify_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (1)
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Lint CLI Go code with golangci-lint and go vet; test with go test -race; check vulnerabilities with govulncheck
Files:
cli/internal/verify/provenance_test.gocli/internal/selfupdate/sigstore.gocli/internal/verify/digest_test.gocli/internal/selfupdate/sigstore_test.gocli/internal/verify/digest.gocli/internal/verify/fuzz_test.gocli/cmd/update.gocli/internal/verify/verify_test.gocli/cmd/start.gocli/internal/verify/verify.gocli/internal/verify/provenance.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to .github/workflows/docker.yml : CI Docker: build → scan → push to GHCR + cosign sign + SLSA L3 provenance via attest-build-provenance (images only pushed after Trivy/Grype scans pass).
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).
Applied to files:
cli/cmd/update.gocli/cmd/start.go
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to cli/go.mod : CLI requires Go 1.26+; dependencies in `cli/go.mod` (Cobra, charmbracelet/huh, charmbracelet/lipgloss)
Applied to files:
cli/cmd/update.go
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to cli/**/*.go : Lint CLI Go code with golangci-lint and go vet; test with go test -race; check vulnerabilities with govulncheck
Applied to files:
cli/internal/verify/verify_test.gocli/cmd/start.go
🧬 Code graph analysis (9)
cli/internal/verify/provenance_test.go (2)
cli/internal/verify/verify.go (1)
ImageRef(28-33)cli/internal/verify/provenance.go (2)
VerifyProvenance(43-63)DSSEPayloadType(24-24)
cli/internal/selfupdate/sigstore.go (1)
cli/internal/verify/provenance.go (2)
DSSEPayloadType(24-24)SLSAProvenancePredicatePrefix(20-20)
cli/internal/verify/digest_test.go (2)
cli/internal/verify/verify.go (1)
ImageRef(28-33)cli/internal/verify/digest.go (1)
ResolveDigest(17-38)
cli/internal/verify/digest.go (1)
cli/internal/verify/verify.go (2)
ImageRef(28-33)IsValidDigest(101-103)
cli/internal/verify/fuzz_test.go (2)
cli/internal/verify/verify.go (2)
IsValidDigest(101-103)NewImageRef(79-85)cli/internal/verify/identity.go (2)
RegistryHost(27-27)ImageRepoPrefix(30-30)
cli/cmd/update.go (3)
cli/internal/ui/ui.go (2)
NewUI(58-70)UI(44-53)cli/internal/config/state.go (2)
Save(160-174)State(16-28)cli/internal/verify/verify.go (3)
VerifyImages(116-145)VerifyOptions(72-75)BuildImageRefs(89-98)
cli/internal/verify/verify_test.go (2)
cli/internal/verify/verify.go (6)
ImageRef(28-33)NewImageRef(79-85)BuildImageRefs(89-98)IsValidDigest(101-103)VerifyImages(116-145)VerifyOptions(72-75)cli/internal/verify/identity.go (2)
RegistryHost(27-27)ImageRepoPrefix(30-30)
cli/internal/verify/verify.go (4)
cli/internal/verify/identity.go (4)
ImageRepoPrefix(30-30)RegistryHost(27-27)BuildVerifier(43-63)BuildIdentityPolicy(67-76)cli/internal/verify/digest.go (1)
ResolveDigest(17-38)cli/internal/verify/cosign.go (1)
VerifyCosignSignature(23-79)cli/internal/verify/provenance.go (2)
VerifyProvenance(43-63)ErrNoProvenanceAttestations(36-36)
cli/internal/verify/provenance.go (2)
cli/internal/verify/verify.go (1)
ImageRef(28-33)src/synthorg/core/enums.py (1)
ArtifactType(256-261)
🔇 Additional comments (8)
cli/internal/verify/digest.go (1)
1-38: LGTM!The implementation correctly handles HEAD-to-GET fallback for registries that don't support HEAD requests, properly propagates the GET error (
getErr) on fallback failure, and validates the digest format before returning.cli/internal/verify/provenance_test.go (1)
65-99: LGTM!The
validateSLSAPredicatehelper tests correctly isolate the predicate validation logic, testing both valid SLSA predicates and rejection of non-SLSA predicate types.cli/internal/verify/digest_test.go (1)
1-171: LGTM!Comprehensive test coverage for
ResolveDigestincluding success, error, HEAD-to-GET fallback, and context cancellation scenarios. The fallback test now correctly asserts the expected computed digest value.cli/internal/verify/provenance.go (1)
1-234: LGTM!Well-structured provenance verification implementation. The code correctly:
- Uses a sentinel error (
ErrNoProvenanceAttestations) for robust error classification- Aggregates all attestation verification errors for debuggability
- Iterates all layers when searching for Sigstore bundles in annotations
- Keeps internal types (
dsseEnvelope,inTotoStatement) unexported for encapsulationcli/internal/selfupdate/sigstore.go (1)
86-129: LGTM!The SLSA provenance assertion correctly handles both DSSE and non-DSSE bundles, validates the payload type, and checks the predicate type prefix. The local
slsaStatementtype is appropriate given it's a trivial struct used only here.cli/internal/selfupdate/sigstore_test.go (1)
1-116: LGTM!Comprehensive test coverage for
assertSLSAProvenancecovering all code paths: valid predicates, invalid predicate types, invalid payload types, non-DSSE bundles, and malformed JSON. Error handling forjson.Marshalhas been added.cli/internal/verify/verify.go (1)
1-194: LGTM!Well-designed verification orchestration:
ImageRef.String()correctly handles empty tags (no trailing colon)isProvenanceMissing()useserrors.Is()with the sentinel error for robust classification- Single verifier/identity built once and reused across all images
- Clear separation between fatal errors (cosign failure, provenance verification failure) and warnings (missing provenance)
cli/internal/verify/fuzz_test.go (1)
1-79: LGTM!Well-structured fuzz tests with appropriate seed values and invariant checks:
FuzzIsValidDigestvalidates both prefix and length for valid digestsFuzzCosignSigTaguses idiomaticstrings.HasSuffixFuzzNewImageRefcaps input size to prevent excessive memory usage
- digestPinMap now returns error on empty digest (fail-closed) - Reorder compose write in pullAndPersist: compose.yml is only written after verification succeeds (or when --skip-verify is set), preventing a crash from leaving an unverified tag on disk - Remove unused regenerateCompose (absorbed into verifyAndPinForUpdate) - Add clarifying comment on TestVerifyProvenanceInvalidAttestation
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/cmd/start.go`:
- Around line 135-146: In writeDigestPinnedCompose, avoid directly using
os.WriteFile for compose.yml; instead write composeYAML to a temporary file
inside safeDir (use a unique name), fsync the file (or at least close it) to
ensure contents are flushed, set the file mode to 0600, and then atomically
rename the temp file to "compose.yml" (os.Rename) to replace the target; ensure
you remove the temp file on error and return wrapped errors from
compose.Generate and the write/rename operations so callers of
writeDigestPinnedCompose see the failure details.
In `@cli/cmd/update.go`:
- Around line 262-264: When handling errors from verify.VerifyImages, only show
the "Use --skip-verify" hint for transport/offline errors rather than all
verification failures; change the error branch that currently calls
errOut.Hint(...) to first detect transport/offline conditions (e.g., via an
isOfflineError/isTransportError helper that checks for network/transport-related
errors or wraps errors.Is for known transport error types) and only call
errOut.Hint when that check returns true. Apply the same conditional hint logic
in the start flow where image verification errors are handled so the bypass flag
is suggested consistently only for offline/transport failures.
In `@cli/internal/verify/provenance_test.go`:
- Around line 65-111: Consolidate the three tests into one table-driven test
that iterates cases for valid, wrong predicate type, and wrong payload type;
create a test table with fields like name, predicateType, payloadType, wantErr,
errContains, and inside the loop build the inTotoStatement, marshal it, create
the dsseEnvelope (using DSSEPayloadType where appropriate) and call
validateSLSAPredicate, then assert whether an error occurred matches wantErr and
that err.Error() contains errContains when provided; replace
TestValidateSLSAPredicate, TestValidateSLSAPredicateWrongType, and
TestValidateSLSAPredicateWrongPayloadType with this single
TestValidateSLSAPredicate table-driven implementation.
- Around line 101-111: The test TestValidateSLSAPredicateWrongPayloadType only
checks for a non-nil error; update it to assert the specific error returned by
validateSLSAPredicate to ensure the payload-type validation path is exercised.
After calling validateSLSAPredicate(envelope), compare err.Error() (or use
errors.Is if the function returns a sentinel error) against the exact message or
sentinel used in validateSLSAPredicate for the wrong payload type (match the
same string/variable used in that function) and fail the test if it does not
match.
🪄 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: d918d266-e7e5-43b4-82ea-936a7e092826
📒 Files selected for processing (3)
cli/cmd/start.gocli/cmd/update.gocli/internal/verify/provenance_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (1)
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Lint CLI Go code with golangci-lint and go vet; test with go test -race; check vulnerabilities with govulncheck
Files:
cli/cmd/start.gocli/cmd/update.gocli/internal/verify/provenance_test.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to .github/workflows/docker.yml : CI Docker: build → scan → push to GHCR + cosign sign + SLSA L3 provenance via attest-build-provenance (images only pushed after Trivy/Grype scans pass).
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).
Applied to files:
cli/cmd/start.gocli/cmd/update.go
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to cli/**/*.go : Lint CLI Go code with golangci-lint and go vet; test with go test -race; check vulnerabilities with govulncheck
Applied to files:
cli/cmd/start.gocli/cmd/update.go
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Shell commands: for Go CLI work, cd cli is an exception because Go tooling requires working directory to be the module root. Go commands require `cd cli` for other work, never use `cd`.
Applied to files:
cli/cmd/update.go
🧬 Code graph analysis (2)
cli/cmd/start.go (4)
cli/internal/config/state.go (2)
State(16-28)Save(160-174)cli/internal/verify/verify.go (4)
VerifyImages(116-145)VerifyOptions(72-75)BuildImageRefs(89-98)VerifyResult(66-69)cli/internal/version/version.go (1)
Version(9-9)cli/internal/compose/generate.go (2)
ParamsFromState(49-62)Generate(66-85)
cli/internal/verify/provenance_test.go (2)
cli/internal/verify/verify.go (1)
ImageRef(28-33)cli/internal/verify/provenance.go (2)
VerifyProvenance(43-63)DSSEPayloadType(24-24)
🔇 Additional comments (10)
cli/internal/verify/provenance_test.go (8)
1-14: LGTM!Package declaration and imports are appropriate for the test file. The
sigverifyalias for the Sigstore verify package avoids collision with the localverifypackage name.
16-29: LGTM!Test correctly validates the early-exit error path when
Digestis empty. Thenilverifier is safe here sinceVerifyProvenancereturns before reaching verification logic.
65-79: LGTM!Test correctly validates the happy path for
validateSLSAPredicatewith a valid SLSA v1 predicate type. The use ofDSSEPayloadTypeconstant ensures consistency with the production code.
81-99: LGTM!Test correctly validates that
validateSLSAPredicaterejects non-SLSA predicate types with the expected error message.
113-117: Good clarification addressing the test isolation concern.The comment clearly documents that this test exercises the general invalid-attestation error path rather than a specific validation step. This addresses the previous review concern about test isolation.
161-180: LGTM!The mock HTTP server correctly simulates OCI registry endpoints needed for the test: version check, referrers API, manifest retrieval, and blob downloads. The default 404 response handles unexpected paths appropriately.
53-54: No action needed —testDigestis properly defined incli/internal/verify/digest_test.goas a package-level constant, so the code will compile and run correctly.
190-193: Current code order is correct; nil verifier concern is hypothetical.The code already calls
validateSLSAPredicatebeforeverifyAttestationBundle(validated at lines 108 and 112 ofprovenance.go). The test correctly expects an error from the invalid predicate type, not from a nil pointer dereference. The concern about reordering is valid as a preventative measure, but the current implementation poses no risk.cli/cmd/update.go (1)
212-219: Good fail-closed ordering.
verifyAndPinForUpdatenow runs before the new compose is kept on disk, so a verification failure no longer leavescompose.ymlpointing at an unverified tag.cli/cmd/start.go (1)
154-163: Good hard failure on missing digests.Rejecting empty digests here preserves the “pull exactly what was verified” guarantee instead of silently falling back to tag-based pulls.
| if err != nil { | ||
| return fmt.Errorf("generating compose file: %w", err) | ||
| errOut.Hint("Use --skip-verify for air-gapped environments") | ||
| return nil, fmt.Errorf("image verification failed: %w", err) |
There was a problem hiding this comment.
Only suggest --skip-verify for offline failures.
This hint fires for every verify.VerifyImages error, including real signature or provenance mismatches. That makes the bypass flag look like the recovery path for a possible tampering event. Gate the hint behind transport/offline errors only, and reuse the same check in the start flow.
Possible shape
if err != nil {
- errOut.Hint("Use --skip-verify for air-gapped environments")
+ if shouldSuggestSkipVerify(err) {
+ errOut.Hint("Use --skip-verify for air-gapped environments")
+ }
return nil, fmt.Errorf("image verification failed: %w", err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/cmd/update.go` around lines 262 - 264, When handling errors from
verify.VerifyImages, only show the "Use --skip-verify" hint for
transport/offline errors rather than all verification failures; change the error
branch that currently calls errOut.Hint(...) to first detect transport/offline
conditions (e.g., via an isOfflineError/isTransportError helper that checks for
network/transport-related errors or wraps errors.Is for known transport error
types) and only call errOut.Hint when that check returns true. Apply the same
conditional hint logic in the start flow where image verification errors are
handled so the bypass flag is suggested consistently only for offline/transport
failures.
- Atomic compose write: writeDigestPinnedCompose now writes to a temp file, fsyncs, sets permissions, and renames — prevents partial writes on crash - Conditional --skip-verify hint: only suggest bypass for transport/offline errors (DNS, timeout, connection refused), not for verification failures (tampered signature, wrong identity) - Consolidate validateSLSAPredicate tests into a single table-driven test with specific error message assertions for each case
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
cli/cmd/root.go (1)
81-99:⚠️ Potential issue | 🟠 MajorRestrict the skip hint to genuine transport failures.
context.Canceledis usually a user abort, and the raw"fetching sigstore trusted root"substring will also match trust-root bootstrap integrity failures. Both cases still satisfy this helper and surface the--skip-verifyhint, which is exactly the bypass suggestion this flow is trying not to show on non-network verification errors. Limit this to timeout/DNS/socket failures and inspect wrapped causes instead of wrapper text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/cmd/root.go` around lines 81 - 99, The helper currently treats context.Canceled and loose substring matches (including "fetching sigstore trusted root") as transport failures and thus surfaces a --skip-verify hint; remove context.Canceled and the raw "fetching sigstore trusted root" check and instead classify only genuine transport failures: keep errors.Is(err, context.DeadlineExceeded); walk wrapped errors (using errors.As/unwrap loop) and return true for net.Error values with Timeout() == true, for *net.OpError and *net.DNSError via errors.As, and for explicit syscall/net errors like connection refused only when errors.Is reports those concrete causes; drop the broad substring checks for wrapper text (or limit them to "i/o timeout" and "TLS handshake timeout" only if you must keep any message checks) so the hint appears only for real DNS/timeout/socket failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/cmd/start.go`:
- Around line 153-190: The atomicWriteFile helper currently fsyncs the temp file
but not the parent directory after os.Rename, so add a directory sync after the
rename: open the parent directory of targetPath (e.g. via
os.Open(filepath.Dir(targetPath))), call Fsync (file.Sync()) on that directory
handle, close it, and return any error; keep the existing tmp.Sync() and
tmp.Close() logic and retain the tmpPath = "" after successful rename to avoid
deferred removal. Ensure errors from opening/syncing/closing the directory are
wrapped with descriptive messages (e.g. "syncing parent directory") and returned
from atomicWriteFile.
In `@cli/cmd/update.go`:
- Around line 254-255: Replace the raw fmt.Fprintln call that prints "Verifying
container image signatures..." with the CLI UI step so the output uses the
lipgloss-backed styling: either instantiate a UI with ui.NewUI(out) and call its
Step(...) method at the point where fmt.Fprintln(out, "...") is used, or change
the helper signature to accept a *ui.UI and call ui.Step from the provided UI;
update the call sites accordingly so update uses ui.NewUI(out).Step("Verifying
container image signatures...") (or ui.Step on the injected *ui.UI) instead of
fmt.Fprintln to keep styling consistent.
In `@cli/internal/verify/provenance_test.go`:
- Around line 129-137: The test currently discards errors from json.Marshal for
inTotoStatement and dsseEnvelope (variables statementJSON and envelopeJSON),
which can produce invalid fixtures; update the two json.Marshal calls to check
their returned errors and fail the test on error (e.g., t.Fatalf or
require.NoError) so marshaling failures are surfaced immediately; apply the same
change to the other occurrences mentioned (around lines 155-156) to ensure no
marshal errors are ignored.
- Around line 197-200: Change the test that calls
VerifyProvenance(context.Background(), ref, nil,
sigverify.CertificateIdentity{}) so it asserts the specific invalid-attestation
error rather than any non-nil error: after calling VerifyProvenance, replace the
loose nil check with a targeted assertion using errors.Is(err, <expected
sentinel error>) if the sigverify package exposes a sentinel like
sigverify.ErrInvalidAttestation, or otherwise assert the error message contains
a stable fragment (e.g., strings.Contains(err.Error(), "invalid attestation")).
Keep the call to VerifyProvenance and the sigverify.CertificateIdentity usage
but tighten the assertion to check the specific error class/message to avoid
false positives.
---
Duplicate comments:
In `@cli/cmd/root.go`:
- Around line 81-99: The helper currently treats context.Canceled and loose
substring matches (including "fetching sigstore trusted root") as transport
failures and thus surfaces a --skip-verify hint; remove context.Canceled and the
raw "fetching sigstore trusted root" check and instead classify only genuine
transport failures: keep errors.Is(err, context.DeadlineExceeded); walk wrapped
errors (using errors.As/unwrap loop) and return true for net.Error values with
Timeout() == true, for *net.OpError and *net.DNSError via errors.As, and for
explicit syscall/net errors like connection refused only when errors.Is reports
those concrete causes; drop the broad substring checks for wrapper text (or
limit them to "i/o timeout" and "TLS handshake timeout" only if you must keep
any message checks) so the hint appears only for real DNS/timeout/socket
failures.
🪄 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: 6c3a02a5-c3b1-4839-9563-2869de917037
📒 Files selected for processing (4)
cli/cmd/root.gocli/cmd/start.gocli/cmd/update.gocli/internal/verify/provenance_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CLI Test (macos-latest)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (1)
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
cli/**/*.go: Use native Gotesting.Ffuzz functions (Fuzz*) for property-based testing
Use Cobra for CLI command structure, charmbracelet/huh for interactive prompts, and charmbracelet/lipgloss for styled output
Files:
cli/internal/verify/provenance_test.gocli/cmd/root.gocli/cmd/start.gocli/cmd/update.go
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to .github/workflows/docker.yml : CI Docker: build → scan → push to GHCR + cosign sign + SLSA L3 provenance via attest-build-provenance (images only pushed after Trivy/Grype scans pass).
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).
Applied to files:
cli/cmd/root.gocli/cmd/start.gocli/cmd/update.go
📚 Learning: 2026-03-16T19:51:10.862Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.862Z
Learning: Applies to cli/**/*.go : Use Cobra for CLI command structure, charmbracelet/huh for interactive prompts, and charmbracelet/lipgloss for styled output
Applied to files:
cli/cmd/root.gocli/cmd/start.gocli/cmd/update.go
📚 Learning: 2026-03-16T19:51:10.862Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T19:51:10.862Z
Learning: Applies to cli/go.mod : CLI dependencies: Go 1.26+, Cobra, charmbracelet/huh, charmbracelet/lipgloss
Applied to files:
cli/cmd/start.gocli/cmd/update.go
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Shell commands: for Go CLI work, cd cli is an exception because Go tooling requires working directory to be the module root. Go commands require `cd cli` for other work, never use `cd`.
Applied to files:
cli/cmd/update.go
🧬 Code graph analysis (2)
cli/internal/verify/provenance_test.go (2)
cli/internal/verify/verify.go (1)
ImageRef(28-33)cli/internal/verify/provenance.go (2)
VerifyProvenance(43-63)DSSEPayloadType(24-24)
cli/cmd/update.go (3)
cli/internal/ui/ui.go (2)
NewUI(58-70)UI(44-53)cli/internal/config/state.go (1)
State(16-28)cli/internal/verify/verify.go (3)
VerifyImages(116-145)VerifyOptions(72-75)BuildImageRefs(89-98)
🪛 GitHub Check: CodeQL
cli/cmd/start.go
[failure] 156-156: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
[failure] 186-186: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
🔇 Additional comments (2)
cli/internal/verify/provenance_test.go (2)
16-63: Solid negative-path coverage for provenance preconditions.
TestVerifyProvenanceEmptyDigestandTestVerifyProvenanceNoReferrersare scoped well and assert meaningful error fragments rather than exact full strings.
65-118: Table-driven predicate validation is a strong improvement.The consolidated
TestValidateSLSAPredicatecases and per-caseerrContainschecks make this suite easier to extend and less repetitive.
| // atomicWriteFile writes data to targetPath via a temp file + rename to prevent | ||
| // partial writes on crash. tmpDir must be on the same filesystem as targetPath. | ||
| func atomicWriteFile(targetPath string, data []byte, tmpDir string) error { | ||
| tmp, err := os.CreateTemp(tmpDir, ".compose-*.yml.tmp") | ||
| if err != nil { | ||
| return fmt.Errorf("creating temp file: %w", err) | ||
| } | ||
| tmpPath := tmp.Name() | ||
|
|
||
| // Clean up temp file on any error path. | ||
| defer func() { | ||
| if tmpPath != "" { | ||
| _ = os.Remove(tmpPath) | ||
| } | ||
| }() | ||
|
|
||
| if _, err := tmp.Write(data); err != nil { | ||
| _ = tmp.Close() | ||
| return fmt.Errorf("writing compose file: %w", err) | ||
| } | ||
| if err := tmp.Sync(); err != nil { | ||
| _ = tmp.Close() | ||
| return fmt.Errorf("syncing compose file: %w", err) | ||
| } | ||
| if err := tmp.Close(); err != nil { | ||
| return fmt.Errorf("closing compose file: %w", err) | ||
| } | ||
|
|
||
| // Set permissions before rename so the target is never world-readable. | ||
| if err := os.Chmod(tmpPath, 0o600); err != nil { | ||
| return fmt.Errorf("setting compose file permissions: %w", err) | ||
| } | ||
|
|
||
| if err := os.Rename(tmpPath, targetPath); err != nil { | ||
| return fmt.Errorf("replacing compose file: %w", err) | ||
| } | ||
| tmpPath = "" // prevent deferred removal of the now-renamed file | ||
| return nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all usages of atomicWriteFile in the codebase
rg "atomicWriteFile" --type goRepository: Aureliolo/synthorg
Length of output: 354
🏁 Script executed:
# Check if the function is defined in start.go and examine the context
grep -n "func atomicWriteFile" cli/cmd/start.goRepository: Aureliolo/synthorg
Length of output: 141
🏁 Script executed:
# Look at the full start.go file to understand the context around atomicWriteFile usage
git ls-files | grep -E "cli/cmd/(start|update)\.go"Repository: Aureliolo/synthorg
Length of output: 96
🏁 Script executed:
# Find usages of atomicWriteFile across the codebase
rg "atomicWriteFile\(" --type go -B 2 -A 2Repository: Aureliolo/synthorg
Length of output: 635
🏁 Script executed:
# Check if atomicWriteFile is also in update.go
grep -n "atomicWriteFile" cli/cmd/update.goRepository: Aureliolo/synthorg
Length of output: 44
🏁 Script executed:
# Let's also examine the full context of where it's called to understand the flow
grep -B 10 -A 2 "return atomicWriteFile" cli/cmd/start.goRepository: Aureliolo/synthorg
Length of output: 495
🏁 Script executed:
# Check if update.go uses atomicWriteFile or has similar logic
cat -n cli/cmd/update.go | head -100Repository: Aureliolo/synthorg
Length of output: 3792
🏁 Script executed:
# Check if update.go uses atomicWriteFile anywhere
rg "atomicWriteFile" cli/cmd/update.goRepository: Aureliolo/synthorg
Length of output: 44
🏁 Script executed:
# Let's see the full update.go file to check for similar file replacement logic
wc -l cli/cmd/update.goRepository: Aureliolo/synthorg
Length of output: 83
🏁 Script executed:
# Examine the key parts of update.go to see if it has similar functionality
grep -n "os.Rename\|WriteFile\|Chmod" cli/cmd/update.goRepository: Aureliolo/synthorg
Length of output: 113
🏁 Script executed:
# Check the actual size and scan for any file writing operations in update.go
sed -n '1,300p' cli/cmd/update.go | tail -100Repository: Aureliolo/synthorg
Length of output: 3396
🌐 Web query:
Go os.Rename crash safety directory sync durability
💡 Result:
os.Renameatomicity (Go): On Unix,os.Renamemaps to the OS rename semantics (atomic within a filesystem). On non-Unix platforms, Go explicitly documents thatos.Renameis not atomic even within the same directory. [1]- Crash safety / durability: Atomic rename (what other processes observe) is not the same as durability after a crash/power loss. To make an “atomic write via temp-file + rename” crash-durable on POSIX filesystems, you generally must:
- Write the temp file.
f.Sync()(Go) to flush file data/metadata. [2]Close()the temp file.rename(temp, final).fsyncthe containing directory so the directory entry update (the rename) is durably recorded. If the rename crosses directories, sync both directories. [3][4]
Minimal POSIX pattern (Go-ish pseudocode):
write temp
temp.Sync()
temp.Close()
os.Rename(tempName, finalName)
fsync(dirOf(finalName)) // and also dirOf(tempName) if differentSources: Go os.Rename docs [1], Go (*os.File).Sync docs [2], durable rename requires syncing directories [3][4].
[1] (pkg.go.dev)
[2] (pkg.go.dev)
[3] (stackoverflow.com)
[4] (calvin.loncaric.us)
Citations:
- 1: https://pkg.go.dev/os
- 2: https://pkg.go.dev/os
- 3: https://stackoverflow.com/questions/3764822/how-to-durably-rename-a-file-in-posix?utm_source=openai
- 4: https://calvin.loncaric.us/articles/CreateFile.html?utm_source=openai
🏁 Script executed:
# Verify all callers of writeDigestPinnedCompose
rg "writeDigestPinnedCompose\(" --type go -B 2 -A 2Repository: Aureliolo/synthorg
Length of output: 1329
Add directory sync after rename for crash-safe durability.
tmp.Sync() only makes the temp file durable. After os.Rename(), a power loss or system crash can still lose the new directory entry unless the parent directory is synced too. Both start and update rely on this shared helper for compose.yml replacement, so add an os.Fsync() call on the directory after the rename to achieve full crash-safety.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 156-156: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
[failure] 186-186: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/cmd/start.go` around lines 153 - 190, The atomicWriteFile helper
currently fsyncs the temp file but not the parent directory after os.Rename, so
add a directory sync after the rename: open the parent directory of targetPath
(e.g. via os.Open(filepath.Dir(targetPath))), call Fsync (file.Sync()) on that
directory handle, close it, and return any error; keep the existing tmp.Sync()
and tmp.Close() logic and retain the tmpPath = "" after successful rename to
avoid deferred removal. Ensure errors from opening/syncing/closing the directory
are wrapped with descriptive messages (e.g. "syncing parent directory") and
returned from atomicWriteFile.
- Add best-effort directory fsync after atomic rename in atomicWriteFile - Use ui.Step for styled output in verifyAndPinForUpdate (consistent with start.go flow) - Handle json.Marshal errors in TestVerifyProvenanceInvalidAttestation - Assert specific "no valid SLSA provenance attestation" error message - Tighten isTransportError: remove context.Canceled and "fetching sigstore trusted root" string match, add net.Error timeout check, keep only genuine transport failure detection
🤖 I have created a release *beep* *boop* --- ## [0.3.1](v0.3.0...v0.3.1) (2026-03-17) ### Features * **api:** RFC 9457 Phase 2 — ProblemDetail and content negotiation ([#496](#496)) ([30f7c49](30f7c49)) * **cli:** verify container image signatures and SLSA provenance on pull ([#492](#492)) ([bef272d](bef272d)), closes [#491](#491) * **engine:** implement context budget management in execution loops ([#520](#520)) ([181eb8a](181eb8a)), closes [#416](#416) * implement settings persistence layer (DB-backed config) ([#495](#495)) ([4bd99f7](4bd99f7)), closes [#450](#450) * **memory:** implement dual-mode archival in memory consolidation ([#524](#524)) ([4603c9e](4603c9e)), closes [#418](#418) * migrate config consumers to read through SettingsService ([#510](#510)) ([32f553d](32f553d)) * **settings:** implement settings change subscriptions for service hot-reload ([#526](#526)) ([53f908e](53f908e)), closes [#503](#503) * **settings:** register API config in SettingsService with 2-phase init ([#518](#518)) ([29f7481](29f7481)) * **tools:** add SSRF prevention for git clone URLs ([#505](#505)) ([492dd0d](492dd0d)) * **tools:** wire RootConfig.git_clone to GitCloneTool instantiation ([#519](#519)) ([b7d8172](b7d8172)) ### Bug Fixes * **api:** replace JWT query parameter with one-time ticket for WebSocket auth ([#493](#493)) ([22a25f6](22a25f6)), closes [#343](#343) ### Documentation * add uv cache lock contention handling to worktree skill ([#500](#500)) ([bd85a8d](bd85a8d)) * document RFC 9457 dual response formats in OpenAPI schema ([#506](#506)) ([8dd2524](8dd2524)) ### Maintenance * upgrade jsdom from 28 to 29 ([#499](#499)) ([1ea2249](1ea2249)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
synthorg startandsynthorg updateimage: repo@sha256:...)--skip-verifyflag for air-gapped environments with clear warningcli/internal/verify/package with pure Go verification (no cosign binary required)How it works
compose.ymlsodocker compose pullfetches exactly what was verifiedconfig.jsonUser-visible output during verification:
Security decisions
docker.ymlworkflow on version tags andmainbranch (same tightness as CLI binary verification)VerifyImagescall (not per-bundle) — eliminates redundant TUF root fetchesDigestPinsvalues validated against digest regex in composevalidateParamsDependencies
go-containerregistrypromoted from indirect to direct dependency (v0.21.2)Test plan
httptest.NewServergo test ./...— 11 packages)go vet ./...cleangolangci-lint runcleanReview coverage
Pre-reviewed by 6 agents, 20 findings addressed:
Closes #491