Hypeman build follow-up items

[hiroTamada]

Follow-up items from @rgarcia's review of PR #53 (hypeman build system).

High Priority

• Add size limit to multipart upload (cmd/api/api/builds.go:63)

  The multipart parsing loop has no size limit on sourceData—could be a DoS vector if someone uploads a huge file.

• Fix ErrBuildInProgress naming/semantics (lib/builds/errors.go:34, cmd/api/api/builds.go:200)

  The comment says "returned when trying to cancel a build that's already complete" but the error name is ErrBuildInProgress—these are contradictory. Should this be ErrBuildAlreadyComplete or similar?

• Secrets timeout should be a hard fail (lib/builds/builder_agent/main.go:401)

  If secrets are required for the build, proceeding without them will just cause a more confusing error later.

• Queue goroutine cancellation on shutdown (lib/builds/queue.go:73)

  Enqueue launches goroutines with go wrappedFn(). If the queue is stopped/shutdown, there's no way to cancel these goroutines. Should startFn accept a context for cancellation?

• Race condition in build cancellation (lib/builds/manager.go:683)

  When cancelling a building/pushing job, this deletes the instance but doesn't wait for confirmation. The deferred cleanup in executeBuild might race with this.

Medium Priority

• Use context.WithoutCancel(ctx) for trace propagation (lib/builds/manager.go:227)

  This uses context.Background() instead of the parent context. If intentional, use context.WithoutCancel(ctx) so context values (like OpenTelemetry trace context) are still propagated.

• Reduce/eliminate 3-second sleep in hot path (lib/builds/manager.go:423)

  Is there some way to avoid this or make it more precise / less fuzzy?

• Implement AllowedDomains filtering or remove field (lib/builds/types.go:75)

  AllowedDomains is defined but doesn't appear in BuildConfig that gets passed to the VM. Is domain filtering implemented, or is this a placeholder?

• Handle partial execution on queue recovery (lib/builds/queue.go:24)

  The queue is purely in-memory. Recovery via listPendingBuilds() could cause issues if a startFn had already partially executed before the restart.

Low Priority (Logging & Observability)

• Log errors in listAllBuilds (lib/builds/storage.go:111)

  Silently skips invalid entries—a corrupted metadata file would be ignored.

• Log json.Marshal failures in SSE events (cmd/api/api/builds.go:246)

  If json.Marshal(event) fails, it silently continues.

• Return error or log for invalid secret IDs (lib/builds/file_secret_provider.go:35)

  The path traversal check skips invalid IDs silently. If someone requests my/token they'd get no feedback.

Code Clarity

• Add sync comments for duplicated types (lib/builds/builder_agent/main.go:38, lib/middleware/oapi_auth.go:20)

  Note that these types must be kept in sync with lib/builds/types.go and lib/builds/registry_token.go.

• Replace bubble sort with sort.Strings (lib/builds/cache.go:154)

  Would be clearer and no slower for small slices.

• Consider removing build-builders Makefile alias (Makefile:217)

  Seems premature if there's only one builder image.

Questions to Address

• Provenance timestamp redundancy (openapi.yaml:732)

  Maybe completed_at would be a better name? Or remove if redundant with Build.completed_at?

• Registry URL configurability (cmd/api/config/config.go:110)

  This has to be the hypeman registry URL since it relies on custom auth—maybe it should not be configurable?

• Plan/timeline to remove deprecated IP fallback (lib/middleware/oapi_auth.go:302)

  This IP fallback is marked as deprecated—is there a plan to remove it?

---

Reference: #53