feat(aegisctl): pedestal helm get/set/verify + dry-run by Lincyaw · Pull Request #67 · OperationsPAI/AegisLab

Lincyaw · 2026-04-18T12:09:57Z

Closes OperationsPAI/aegis#19

Implements OperationsPAI/aegis#19. Adds CRUD and verification commands for helm_configs rows, plumbed through 3 new backend handlers under /api/v2/pedestal/helm/:container_version_id. verify dry-runs helm repo add + helm pull without triggering a real restart_pedestal task. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Adds a new “pedestal helm” management surface spanning API + CLI to read/update helm_configs rows by container_version_id and to dry-run verification (helm repo add/update/pull + values YAML parse) without triggering a real restart_pedestal task.

Changes:

Introduces /api/v2/pedestal/helm/:container_version_id GET/PUT and /verify POST endpoints plus DTOs for config + verify results.
Adds a unit-testable verification pipeline (pedestalhelm package) with fakeable helm runner and unit tests.
Extends aegisctl with pedestal helm get|set|verify commands and documents the workflow.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/router/v2.go	Registers the new Pedestal Helm route group and endpoints.
src/handlers/v2/pedestal_helm.go	Implements the GET/PUT/verify handlers wiring DB ↔ DTO ↔ verify pipeline.
src/handlers/v2/pedestalhelm/verify.go	Implements the dry-run verification pipeline and real helm CLI runner.
src/handlers/v2/pedestalhelm/verify_test.go	Unit tests for the verification pipeline behavior and value-file parsing.
src/dto/container.go	Adds API response/request DTOs for pedestal helm config and verification.
src/cmd/aegisctl/cmd/root.go	Registers the new `pedestal` command group.
src/cmd/aegisctl/cmd/pedestal.go	Implements `aegisctl pedestal helm get
docs/aegisctl-cli-spec.md	Documents the new CLI commands, flags, and API mappings.

Copilot · 2026-04-18T12:14:47Z

+func VerifyValueFile(path string) error {
+	abs := path
+	if !filepath.IsAbs(abs) {
+		abs, _ = filepath.Abs(abs)


VerifyValueFile ignores the error from filepath.Abs. If Abs fails, abs may be empty/incorrect and the subsequent open error becomes misleading. Handle the Abs error and return a wrapped error so callers get a clear failure reason.

Suggested change

abs, _ = filepath.Abs(abs)

var err error

abs, err = filepath.Abs(abs)

if err != nil {

return fmt.Errorf("resolve value file %q to absolute path: %w", path, err)

}

Copilot · 2026-04-18T12:14:47Z

+//
+// It is intentionally separated from the handlers/v2 package so the pure
+// pipeline can be unit-tested without dragging in the full server build
+// graph (which currently has an unrelated compile break in injections.go).


This package comment references an “unrelated compile break in injections.go”. That’s likely to become stale and is surprising to ship in production code/docs. Consider removing this reference and keeping the comment focused on the architectural reason (unit-testable pipeline / reduced deps) without calling out transient build state.

Suggested change

// graph (which currently has an unrelated compile break in injections.go).

// graph and its broader dependencies.

Copilot · 2026-04-18T12:14:47Z

+func (RealRunner) RepoAdd(name, url string) (string, error) {
+	cmd := exec.Command("helm", "repo", "add", name, url, "--force-update")
+	out, err := cmd.CombinedOutput()
+	return string(out), err
+}
+
+func (RealRunner) RepoUpdate() (string, error) {
+	cmd := exec.Command("helm", "repo", "update")
+	out, err := cmd.CombinedOutput()
+	return string(out), err
+}
+
+func (RealRunner) Pull(repo, chart, version, destDir string) (string, error) {
+	cmd := exec.Command("helm", "pull", fmt.Sprintf("%s/%s", repo, chart),
+		"--version", version, "--destination", destDir)
+	out, err := cmd.CombinedOutput()
+	return string(out), err
+}


RealRunner uses helm repo add / helm repo update without isolating Helm’s repository config/cache. By default this mutates shared state under the server user’s home directory, which can cause cross-request interference and races, and helm repo update refreshes all repos (potentially slow). Consider running helm with an isolated repo config/cache (tmp dir) and updating only the relevant repo name to keep requests independent and bounded.

Copilot · 2026-04-18T12:14:47Z

+4. If `value_file` is set: open and `yaml.Unmarshal` to assert it parses,
+   plus a shallow check that `image.repository` / `image.tag` are scalar
+   when present. Image reachability is **not** checked (TODO: add
+   `skopeo inspect` once the round-trip is fast enough).
+
+The command exits **0** on success, **1** on any failed check. On
+failure, each check's `detail` includes the helm CLI stderr — the CLI
+output is never hidden.


The verify pipeline description says the server will yaml.Unmarshal the values file, but the implementation uses yaml.NewDecoder(...).Decode. Also, the text says the CLI surfaces “helm CLI stderr”, but the server captures CombinedOutput (stdout+stderr). Please update the wording to match the actual behavior to avoid confusing users when stdout is included in detail.

Suggested change

4. If `value_file` is set: open and `yaml.Unmarshal` to assert it parses,

plus a shallow check that `image.repository` / `image.tag` are scalar

when present. Image reachability is **not** checked (TODO: add

`skopeo inspect` once the round-trip is fast enough).

The command exits **0** on success, **1** on any failed check. On

failure, each check's `detail` includes the helm CLI stderr — the CLI

output is never hidden.

4. If `value_file` is set: open and decode it with

`yaml.NewDecoder(...).Decode` to assert it parses, plus a shallow check

that `image.repository` / `image.tag` are scalar when present. Image

reachability is **not** checked (TODO: add `skopeo inspect` once the

round-trip is fast enough).

The command exits **0** on success, **1** on any failed check. On

failure, each check's `detail` includes the Helm CLI combined output

(stdout+stderr) — the CLI output is never hidden.

Copilot · 2026-04-18T12:14:48Z

+			helm.GET("/:container_version_id", v2handlers.GetPedestalHelmConfig)
+			helm.POST("/:container_version_id/verify", v2handlers.VerifyPedestalHelmConfig)


The new /api/v2/pedestal/helm endpoints are only protected by JWTAuth; unlike other container-version/helm routes, GET and verify don’t require any container-version read/upload permission middleware. This allows any authenticated user to read helm_configs rows and trigger server-side helm executions for arbitrary container_version_id values. Consider applying RequireContainerVersionRead (and likely the same upload/admin permission for verify if intended) at the route level to align with existing authorization patterns.

Suggested change

helm.GET("/:container_version_id", v2handlers.GetPedestalHelmConfig)

helm.POST("/:container_version_id/verify", v2handlers.VerifyPedestalHelmConfig)

helm.GET("/:container_version_id", middleware.RequireContainerVersionRead, v2handlers.GetPedestalHelmConfig)

helm.POST("/:container_version_id/verify", middleware.RequireContainerVersionUpload, v2handlers.VerifyPedestalHelmConfig)

Copilot · 2026-04-18T12:14:48Z

+	cfg, err := repository.GetHelmConfigByContainerVersionID(database.DB, versionID)
+	if err != nil {
+		if errors.Is(err, gorm.ErrRecordNotFound) {
+			dto.ErrorResponse(c, http.StatusNotFound, "helm config not found for container_version_id")
+			return
+		}
+		dto.ErrorResponse(c, http.StatusInternalServerError, "failed to load helm config: "+err.Error())
+		return
+	}
+
+	dto.SuccessResponse(c, toHelmConfigResp(cfg))


These handlers treat any container_version_id as a “pedestal” version, but there’s no validation that the referenced ContainerVersion belongs to a pedestal container type. Given the route naming/docs, it would be safer to load the ContainerVersion and verify Container.Type == pedestal (or rename/move the endpoint if it’s meant to be generic).

Copilot AI review requested due to automatic review settings April 18, 2026 12:09

Lincyaw had a problem deploying to test-server April 18, 2026 12:10 — with GitHub Actions Failure

Copilot started reviewing on behalf of Lincyaw April 18, 2026 12:10 View session

Copilot AI reviewed Apr 18, 2026

View reviewed changes

Lincyaw closed this Apr 19, 2026

Lincyaw deleted the issue-19/pedestal-helm branch April 19, 2026 03:47

Navigation Menu

feat(aegisctl): pedestal helm get/set/verify + dry-run - #67

feat(aegisctl): pedestal helm get/set/verify + dry-run#67
Lincyaw wants to merge 1 commit into
mainfrom
issue-19/pedestal-helm

Lincyaw commented Apr 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

This branch had an error being deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-		abs, _ = filepath.Abs(abs)
+		var err error
+		abs, err = filepath.Abs(abs)
+		if err != nil {
+			return fmt.Errorf("resolve value file %q to absolute path: %w", path, err)
+		}

	// graph (which currently has an unrelated compile break in injections.go).
	// graph and its broader dependencies.

		helm.GET("/:container_version_id", v2handlers.GetPedestalHelmConfig)
		helm.POST("/:container_version_id/verify", v2handlers.VerifyPedestalHelmConfig)

Conversation

Lincyaw commented Apr 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

This branch had an error being deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants