You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Scanning an accumulated vendor-delivery directory could overwrite unrelated or newer managed bundles, reuse input passwords for output encryption, and silently skip expected output. Managed refreshes now show a complete plan before writing, with explicit scope, replacement checks, and separate output credentials.
scan --bundle-path previews by default; --write applies the plan. Repeat --bundle-name / --only to limit output, and use --require-bundle / --fail-on-skip for automation. Empty selections and invalid configuration fail explicitly.
Compare the selected leaf with existing output and block expiration downgrades, equal-expiry conflicts, and ambiguous replacements unless --force is provided. Pin each plan to an absolute resolved output destination and check directory identity. Preflight every bundle, recheck scope, existing contents, candidate validity, and the planned trusted chain after staging and immediately before replacement, serialize refreshes with a lock, and retain staged directory replacement/rollback.
Keep scan input passwords separate from output encryption. --output-password-file encrypts PEM keys and sets the P12 password. Without it, P12 preserves the intentional changeit default and PEM keys remain unencrypted. Default artifacts include PEM certificates/key, a P12 archive, and JSON metadata. --formats selects additional or fewer artifacts.
Select certificates by expiry, issuance time, and SHA-256 fingerprint. Report the selected source, key source, config rule, identity, validity, trust, files, unselected candidates with tie-break reasons, and created/replaced/skipped decisions in text/JSON and each saved manifest.json.
Reserve all selected primary directories before candidate lookup, including missing candidates; reject colliding CN-derived names within a rule while allowing explicit grouping. Reject sanitized, case, and Unicode aliases, protect unselected rules and existing manifest identities, and reject reserved names and control characters in directories and artifacts.
Keep declared config, password, and database files outside managed output. Exclude output/password/database paths and their symlink, case, and Unicode aliases from raw ingestion, preserve explicit database imports, and honor explicitly chosen scan roots named vendor. Document the complete workflow in top-level help, README, examples, and architecture notes.
Breaking changes: Existing managed-export scripts must add --write, choose extra artifacts explicitly, and use --output-password-file for output encryption. bundle and convert retain their existing password behavior. --force still permits untrusted certificates and now also overrides replacement conflicts. Expired leaves separately require --allow-expired; with verification enabled, their chains are checked at the leaf issuance time, disclosed in chain warnings. Future-dated leaves remain ineligible even with force or the expired-certificate override.
Test Plan
gopls check passed for all changed Go files.
Full pre-commit run --all-files passed, including Go race tests, lint/vulnerability checks, generated docs, WASM/Cloudflare builds, and 129 web tests.
Behavioral regressions for scope, deterministic ties, downgrade/conflict protection, explicit overrides, missing/required bundles, stale plans and validity, directory/artifact aliases, control-file protection, symlinks, concurrent writers, public-only exports, artifact selection, and provenance.
CLI tests with encrypted P12 and ZIP deliveries, distinct input/output passwords (including spaces), default P12 password round-trips, expiry policy, preview/write behavior, JSON manifests, and invalid options.
Built CLI smoke test: preview creates no output; explicit write creates selected files; a downgrade exits 2 and leaves existing output unchanged.
Real certificate-management setup: previews reported 15 planned and 24 skipped bundles; an isolated copy replaced the same 15 and preserved skipped directories. Required stale exports failed, expiration downgrades stayed blocked with --allow-expired, the original tree was unchanged, and temporary private artifacts were removed.
Checklist
Conventional Commit messages and signed commits.
Changelog includes compatibility changes.
CLI docs regenerated and examples updated.
Limits
Each bundle uses staged directory replacement with rollback; the refresh is not a transaction across all directories. A later write-time error may leave earlier bundles applied; their statuses remain visible in the manifest. A replacement committed before backup cleanup fails retains its replaced status; the error identifies the retained backup path and later writes stop. A killed process can leave the refresh lock requiring removal after confirming no writer remains.
Preview scoped bundle changes before writing, protect existing leaves from unsafe replacement, and separate input decryption from output encryption. Report deterministic selection and provenance, support artifact selection and required bundles, and cover the workflow with behavioral regressions.
Adds a preview-first managed bundle refresh workflow with scoped selection, replacement safety, separate credentials, selectable artifacts, manifests, and documentation.
Adds separate input/output password handling and format-aware artifact generation.
Updates CLI integration, tests, documentation, changelog, architecture notes, and dependencies.
File
Reviewed changes and findings
web/package-lock.json
Updates locked web dependencies.
README.md
Documents the managed refresh workflow and artifacts.
internal/scanwalk.go
Adds scan exclusions and vendor-root handling. Findings: Critical (3 votes): use canonical/evaluated paths to prevent symlink bypasses. Nit (1 vote): wrap exclusion errors with context while preserving the cause.
internal/scanwalk_test.go
Tests scan exclusions.
internal/certstore/memstore.go
Adds deterministic certificate ordering.
internal/certstore/export.go
Adds artifact selection and key validation. Findings: Nit (2 votes): wrap key/certificate mismatch errors with context while preserving errors.Is. Nit (1 vote): add table-driven coverage for all supported formats and key/password rules.
internal/certstore/export_formats.go
Defines supported and default formats.
internal/bundleplan.go
Implements planning, validation, and writes. Findings: Moderate (2 votes): pass expiry policy independently from trust verification. Moderate (2 votes): represent skipped duplicate candidates in the plan. Moderate (1 vote): record Forced whenever force overrides verification.
internal/bundleplan_test.go
Tests planning and replacement behavior.
internal/bundleplan_existing.go
Inspects existing bundle contents and detects stale changes.
EXAMPLES.md
Updates managed refresh examples. Finding: Nit (1 vote): clarify that missing keys fail only for key-dependent formats.
Recognize selected CA certificates in manifests, retain valid configured names for duplicate Kubernetes Secrets, and scope required-bundle checks to the primary directory so optional historical skips do not block it.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Moderate findings remain around default-password warning visibility and artifact-selection test coverage.
Review effort: Lite Findings: None
Files not reviewed (1)
web/package-lock.json: Generated file
Previously missed (1)
In code that hasn't changed since last review
Always emit changeit security warning
internal/bundleplan.go:146
The managed path emits the changeit fallback warning through slog.Warn, so --log-level error suppresses it even though the command still creates a P12 protected by the well-known default. This differs from bundle/convert, which write the warning directly to stderr; emit this security warning independently of the configured log level (or return a plan flag for the CLI to report).
CSR metadata is not an existing leaf certificate. Ignore it during replacement checks while keeping malformed certificate protection and identifying the offending artifact. Keep the intentional changeit fallback warning visible even at error log level.
Fixed in 3cb05ec. Addressed the default-password warning feedback: managed scans now write the warning directly to stderr, independently of the configured log level. The intentional changeit fallback is unchanged. CLI regression coverage runs with error-level logging and checks both warning visibility and suppression when an explicit output password is supplied or P12 is omitted.
Functional testing against an existing certificate-management setup also found that legacy .csr.json files were being treated as certificate metadata. Refresh now ignores CSR metadata when identifying the installed leaf; malformed certificate artifacts still block replacement and identify the offending filename. Synthetic regressions cover complete legacy and managed bundles.
Validation: scoped previews and isolated replacement writes with real inputs; deterministic selection and archive provenance; exact selected artifact lists; unselected-directory preservation; leaf/key matching and chain verification with OpenSSL; default and explicit P12 passwords; explicit PEM encryption; private-file permissions; and downgrade protection. The source setup was unchanged, and temporary private-key copies were removed. All repository pre-commit checks and Go language-server diagnostics pass.
ERR-4: this new validation error contains uppercase Kubernetes, but project error strings must be lowercase (CLAUDE.md:89). Use kubernetes in the wrapped error text so this path follows the repository's error contract.
Skip candidates that cannot be exported before comparing installed leaves. Exclude dump outputs from ingestion and abort before managed writes if the default-password warning cannot be displayed.
Fixed in 4d41ab9. Addressed the latest review feedback, including the review-summary request to lowercase kubernetes in the validation error. Optional expired/keyless candidates now skip before replacement checks; existing dump paths are excluded from ingestion; and a failed default-password diagnostic aborts before any managed writes. The intentional changeit fallback remains unchanged.
Regression tests cover optional versus required/scoped skips, preserved downgrade protection, stale key/certificate dumps, and closed stderr. Functional checks against the real certificate-management setup confirmed successful unscoped previews and isolated writes while preserving skipped directories. The original setup was unchanged, and the temporary private-key copies were removed. Full pre-commit checks and Go language-server diagnostics passed.
Optional untrusted deliveries cannot replace anything and must not block unrelated exports. Verify trust before replacement checks, and reject collisions with the managed manifest and refresh lock before writing.
Fixed the two concrete review-summary points in d5adf4a. Managed writes now recheck cancellation, directory scope, candidate validity/trust, and existing contents after artifact staging, immediately before replacement. The final manifest is generated after those checks. Regressions simulate edits to the current bundle and to a later bundle while the first bundle is staged; the edits survive, the affected entry is blocked, earlier commits retain their status, and staged artifacts are removed.
Malformed certificate and manifest diagnostics also retain the offending filename when a directory has zero or multiple identifiable leaves, rather than being overwritten by a generic ambiguity message.
The diff-thread fixes for case/Unicode scan exclusions and control characters in artifact names are included in the same commit. Focused reproductions failed before those fixes and pass now. Full pre-commit checks, Go language-server diagnostics, and real-data previews/isolated writes pass. The original certificate-management tree remains unchanged; temporary private artifacts were removed. The intentional changeit fallback is preserved.
Keep output targets stable across working-directory and symlink changes, reject derived-name collisions within one rule, and retain bundle names in early write failures.
Assign configured bundle names after AIA ingestion so fetched CAs are eligible for export. Block malformed existing JSON/YAML metadata even beside a valid leaf, and preserve managed-export error context.
Fixed the three concrete points in the latest review overview in 19d2bad:
Bundle names are assigned after AIA resolution, so fetched CA certificates can match configured exports. A local HTTP AIA regression exercises both preview and write, including selected bundle identity and source provenance.
Malformed JSON/YAML certificate metadata now blocks replacement even when a neighboring PEM contains a valid leaf, and the plan identifies the offending filename. Regressions reproduce the previous unsafe acceptance and verify that blocked writes preserve the original files. CSR JSON and Kubernetes YAML remain excluded from leaf inspection; explicit force overrides retain their documented behavior.
Managed-export, text-plan output, and existing-artifact failures now carry contextual error wrappers using %w. The closed-stdout regression verifies the original error remains available through errors.Is.
README, examples, architecture notes, and changelog match these changes. Full pre-commit checks (including race tests), generated docs, and Go language-server diagnostics pass. The required dependency hook also refreshed two transitive development dependencies.
Functional testing against the real certificate-management setup passed previews and isolated replacement writes: 15 replacements and 24 skips. The original tree and skipped bundle contents were unchanged; temporary private artifacts were removed. The intentional changeit default remains unchanged.
The existing Go 1.27 embedded-field comment still has the technical explanation and build evidence in its thread and remains open pending reviewer agreement, per the repository disagreement policy.
Validate metadata identity and filename lengths before replacement, including mixed-case artifacts. Preserve review decisions when scope changes and report lock cleanup failures without removing another refresh marker.
Fixed the latest three diff findings and the review-summary lock/scope concerns in e577951.
Planning enforces a 255-byte UTF-8 component limit for directories and generated artifacts. Short staging/backup prefixes allow valid long directory names to be written.
Existing certificate metadata requires nonempty certificate fields, and manifests require bundle identity plus selected leaf PEM. Empty/null/incomplete metadata blocks ordinary replacement, with the filename in the plan. Artifact extensions and CSR/Kubernetes exclusions are case-insensitive.
Refresh-lock cleanup now returns errors while preserving committed statuses. Lock ownership is checked before commit; cleanup uses the original open directory handle and preserves replacement markers, including if the output directory moves.
A scope conflict during writing marks pending entries blocked and invalidates the plan. Previously committed bundles retain their status, including when the conflict occurs while staging a later bundle.
Regression coverage includes the reported pre-fix failures, byte-length boundaries, preserved malformed/conflicting artifacts, lock cleanup/interference, stale-plan reuse, and partial-write reporting. README, examples, architecture notes, and changelog match the final behavior. Full pre-commit checks, race tests, generated docs, and Go language-server diagnostics pass. Required hooks refreshed the indirect Go dependency and web development lockfile.
Real-data checks against the certificate-management setup passed: 15 replacements and 24 skips in an isolated copy, original files unchanged, skipped directories preserved, and temporary private artifacts removed. The intentional changeit default is unchanged.
The existing Go 1.27 syntax disagreement remains open with its prior technical explanation and build evidence, pending reviewer agreement.
Keep staging, rename, and cleanup operations on open directory handles so output-root relocation cannot strand private keys. Treat failed reinspection as a blocked plan and canonicalize symlink-aware exclusions before scanning.
Fixed the follow-up review in dd84c2e, including the symlink-aware exclusion concern from the latest review overview.
The exclusion reproductions showed both directions of a working-directory alias bypass: relative exclusions with an absolute scan root, and absolute exclusions with a relative scan root. They also showed a declared future output under a symlinked ancestor being ingested when it appeared during the scan. Exclusions now resolve those aliases and missing suffixes, and ordinary in-root symlink inputs retain matching canonical boundaries.
The two new diff findings are also fixed: staging/writes/renames/rollback/cleanup use opened directory handles, and failed existing-bundle reinspection records a blocked validation result while preserving the underlying error. The relocated-output regression includes a staged synthetic private key and verifies cleanup cannot remove a replacement staging path or lock.
Full pre-commit checks, race tests, generated docs, and Go language-server diagnostics pass. The real certificate-management setup passed again: 15 isolated replacements and 24 skips, original tree and skipped bundles unchanged, temporary private artifacts removed. README, examples, architecture notes, and changelog describe the final behavior. The intentional changeit default remains unchanged.
The previously explained Go 1.27 syntax disagreement remains open pending reviewer agreement.
The reason will be displayed to describe this comment to others. Learn more.
I could not reproduce this finding: append(slices.Clone(names), input.RequireBundles...) is used only to validate rule existence and never assigned back to names. The selected scope is unchanged, and the following out-of-scope check remains reachable. Commit 138394e adds TestBundlePlan_RequiredBundlesRespectScope covering rejection of required-but-unselected bundles without any output, successful required names inside the scope, unscoped requirements, and unknown rules. The scope behavior already passed before the production fixes. I added a clarifying comment and am leaving this disagreement open for reviewer confirmation.
Recheck the reviewed directory and lock after staging the manifest and immediately before replacement. Preserve blocked and committed states when safety checks fail, and cover required bundles outside an explicit scope.
Added a final commit guard after manifest staging, immediately before the first rename, checking the opened parent against the reviewed destination and rechecking lock ownership and the other safety conditions.
Destination and lock failures now mark pending bundles blocked and invalidate the plan, while preserving already committed statuses and underlying errors.
Added regression coverage for artifact/manifest interference, partial replacements, and required bundles outside an explicit scope. The scoped-export finding is a disagreement: required-rule validation appends to a clone and does not expand the export scope.
Validation: pre-commit run --all-files and gopls check passed. The real-certificate functional check replaced 15 bundles in an isolated copy and left 24 skipped bundles unchanged; the original input/setup tree was verified unchanged and temporary private artifacts were removed. The required dependency hook also refreshed one transitive web development dependency.
The two fixed threads are resolved. The scoped-export and Go-version disagreements remain open with explanations. CI passed for this commit. Automated rereview is still running.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Description
Scanning an accumulated vendor-delivery directory could overwrite unrelated or newer managed bundles, reuse input passwords for output encryption, and silently skip expected output. Managed refreshes now show a complete plan before writing, with explicit scope, replacement checks, and separate output credentials.
scan --bundle-pathpreviews by default;--writeapplies the plan. Repeat--bundle-name/--onlyto limit output, and use--require-bundle/--fail-on-skipfor automation. Empty selections and invalid configuration fail explicitly.--forceis provided. Pin each plan to an absolute resolved output destination and check directory identity. Preflight every bundle, recheck scope, existing contents, candidate validity, and the planned trusted chain after staging and immediately before replacement, serialize refreshes with a lock, and retain staged directory replacement/rollback.--output-password-fileencrypts PEM keys and sets the P12 password. Without it, P12 preserves the intentionalchangeitdefault and PEM keys remain unencrypted. Default artifacts include PEM certificates/key, a P12 archive, and JSON metadata.--formatsselects additional or fewer artifacts.manifest.json.vendor. Document the complete workflow in top-level help, README, examples, and architecture notes.Breaking changes: Existing managed-export scripts must add
--write, choose extra artifacts explicitly, and use--output-password-filefor output encryption.bundleandconvertretain their existing password behavior.--forcestill permits untrusted certificates and now also overrides replacement conflicts. Expired leaves separately require--allow-expired; with verification enabled, their chains are checked at the leaf issuance time, disclosed in chain warnings. Future-dated leaves remain ineligible even with force or the expired-certificate override.Test Plan
gopls checkpassed for all changed Go files.pre-commit run --all-filespassed, including Go race tests, lint/vulnerability checks, generated docs, WASM/Cloudflare builds, and 129 web tests.--allow-expired, the original tree was unchanged, and temporary private artifacts were removed.Checklist
Limits
Each bundle uses staged directory replacement with rollback; the refresh is not a transaction across all directories. A later write-time error may leave earlier bundles applied; their statuses remain visible in the manifest. A replacement committed before backup cleanup fails retains its replaced status; the error identifies the retained backup path and later writes stop. A killed process can leave the refresh lock requiring removal after confirming no writer remains.