fix(api): verify service ownership and reserve platform app names

[mashkovd]

Summary

Hardening for tenant-scoped service operations and tenant naming. Workflows address a tenant's service through the Argo CD Application name <team>-<component>, which shares one name space with the platform's own Applications. This PR makes mctl-api check that pair before submitting anything.

1. Service must exist (fail closed)

retire-service, rollback-service, deploy-service (deploy / update-config), preview-deploy, add-custom-domain and remove-custom-domain now require platform-gitops/services/<team>/<component>/values.yaml to exist in the gitops checkout. That directory is exactly what the apps ApplicationSet turns into <team>-<component>.

• New gitops.Reader.ServiceExists: only a missing path is "absent". A stale or never-synced checkout, a symlink or non-plain entry on the path, or any other stat error is an error, and the API answers 503 without submitting.
• Not gated, on purpose: deploy-service action=onboard (it creates the service), provision-database (may run before onboarding, does not act on the Application) and preview-delete (must still clean up after a retire). All three still get check 3.
• Admins and CI principals are held to the check too.

2. Reserved tenant names

• Structural rule over a snapshot of platform Applications: a tenant name N is reserved if N == P or P starts with N- for any P in platformApplicationNames (derived from kubectl get applications -n argocd on 2026-10-11 plus bootstrap/templates). It newly reserves, among others: root, root-app, shared, shared-pg, cloudnative, cloudnative-pg, external, grafana, grafana-iac, loki, loki-stack, otel, otel-collector, otel-eval, otel-eval-tempo, reflector, valkey, temporal-web, vault-human-auth-iac.
• New exact name tenant, new prefixes tenant- and otel-.
• Cross-tenant rule, checked live in the handler: a new tenant name may not extend an existing tenant name (labs-x next to labs) or be extended by one. It reads the tenant list from gitops with a new strict Reader.TenantNames; a read failure means 503. Admins are included, and re-running create-tenant for the same name is not a collision.
• Platform Applications that sit under an existing tenant's prefix (admins-mctl-{agent,api,portal}, erpact-{control,deployer,infra,oidc-zitadel,shared}, labs-oidc-zitadel, mctl-portal-oidc-zitadel) cannot feed the tenant rule without reserving the existing tenants. They are covered by check 3.

The existing tenants admins, erpact, karabu, labs, mctl and ovk stay valid, and so do all their current services. Tests pin both.

3. Reserved <team>-<component> pairs

Every operation that takes team_name plus component_name / service_name / app_name refuses a pair that spells a platform Application (either list above), any tenant-* name, or the component openclaw-skills. The refusal happens in Registry.ValidateInput (400) and again in Executor.Submit as a backstop.

MCP tools reach these operations through the same REST handler, so both paths enforce the checks. Tests drive real tools/call requests to confirm it.

Follow-up needed in mctl-gitops (not in this PR)

tenant_name_reserved in platform-gitops/argo-workflows/cluster-templates/wft-create-tenant.yaml should mirror the new reservations:

• exact: tenant, root, shared, cloudnative, external, grafana, loki, otel, reflector, valkey, plus each full platform Application name not already covered (root-app, shared-pg, cloudnative-pg, loki-stack, grafana-iac, temporal-web);
• prefixes: tenant-*, otel-*;
• simplest equivalent: a loop over the same platform Application list using the hyphen-prefix rule.

The cross-tenant prefix rule needs the tenant list. The workflow already clones gitops, so it could apply the same rule over platform-gitops/tenants/*.

Do not add admins, erpact, labs or mctl to the shell list. The shell check runs even with reprovision=true, so listing them would break re-provisioning of those tenants.

Maintenance

platformApplicationNames and tenantScopedPlatformApplications in internal/operations/reserved_tenant.go document how to re-derive them. TestLiveApplicationsAreProtected checks a snapshot of the live Application list against both, so when a platform Application is added, add it to the snapshot and to one of the two lists.

Test plan

• go test ./..., go vet ./..., golangci-lint run (v2.11.4, Go 1.26.9): clean. govulncheck: no called vulnerabilities.
• New tests cover:
  • a missing service is refused (REST and MCP), and nothing is submitted;
  • a failed read gives 503 without leaking the error, and nothing is submitted;
  • an existing service proceeds;
  • onboard is not gated on existence but does refuse reserved pairs;
  • reserved and overlapping create-tenant names are refused on both REST and MCP;
  • a failed tenant-list read refuses;
  • existing tenants and services stay valid;
  • reader edge cases: stale checkout, symlink, unreadable directory, missing values.yaml.
• Mutation-verified. Each check was reverted on its own (13 mutations: the existence gate, the read-error handling, the tenant platform rule, the collision check and its read error, the validation and Submit service checks, the tenant-scoped list, the reader's missing/stale/symlink/listing handling, and the tenant- prefix), and at least one test failed every time.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XgvCTQf5obEJr8NyKp73jC

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @mashkovd's task in 8m 26s —— View job

---

Code review complete — changes requested (0 P1, 2 P2, 3 P3)

• Read the pinned diff (30748f0...1fbbc2e)
• Review gitops.Reader changes (ServiceExists, TenantNames, checkFreshLocked)
• Review handler gating in handlers_write.go
• Review reserved tenant-name / reserved-pair logic
• Review test coverage and other ValidateInput / Submit call sites
• Post inline findings and submit the review verdict

Findings are posted as inline comments on the diff; the review verdict is bound to 1fbbc2e.

P2 — should fix before merge

1. The reserved-pair check breaks the domain teardown path (internal/operations/registry.go:273)
triggerRemoveCustomDomain (internal/api/handlers_domains.go:643) is the other ValidateInput caller that passes team_name/service_name, and it runs on DELETE /api/v1/domains/{id}. For a row whose pair is now reserved — erpact/control, erpact/shared, labs/oidc-zitadel, admins/mctl-api, mctl/portal-oidc-zitadel, or any <tenant>/openclaw-skills — validation fails, DeleteDomain maps that to 500 and keeps the row by design, so the domain becomes undeletable through the API and its ingress host and TLS entry are never torn down. The new Submit backstop rejects the same call, so no path is left. This is the same failure mode the normalizeIdentifier comment above that call was added to prevent, and the same reasoning the PR uses to exempt preview-delete.
Fix this →

2. preview-delete exemption plus an unpatterned preview_id (internal/api/handlers_write.go:565)
preview_id is the only one of preview-delete's three parameters with no Pattern (internal/operations/registry.go:520), the reserved-pair check only ever composes <team>-<component>, and the existence gate skips the op. A RoleDeveloper member of labs can send component_name=oidc, preview_id=zitadel: labs-oidc is not reserved and nothing checks the composed name. Whether that reaches labs-oidc-zitadel depends on how wft-preview-delete builds the Application name — worth confirming against the template; a DNS-safe Pattern on preview_id is the cheap fix either way.
Fix this →

P3 — nice to have

• TenantExistsMaxAge now bounds ServiceExists and TenantNames too; rename it (e.g. CheckoutMaxAge) and update the doc comment (internal/gitops/reader.go:687).
• requiresExistingService is an allowlist defaulting to "not gated", so a future service operation is silently ungated; invert it and list the three deliberate exemptions instead. Also, only the onboard exemption is pinned by a test — provision-database and preview-delete are not (internal/api/handlers_write.go:571).
• ServiceExists's path walk is a near-verbatim copy of ComponentSourceRepo's; extract a shared helper so the symlink/containment rules cannot drift (internal/gitops/reader.go:751).

What looks good

Fail-closed reader semantics throughout (Lstat so symlinks are an error, stale and never-synced checkouts are errors, only ErrNotExist below the services root is "absent"), ServiceExists agreeing with readService on what makes a service (a values.yaml), the structural hyphen-prefix rule over the Application snapshot rather than a hand-maintained name list, the cross-tenant collision rule reading the live tenant list with a 503 on read failure, internal errors logged rather than returned to callers, and the checks placed after ValidateInput so the raw-input patterns run first. No ordering problem with PreparePreviewDeployInput (it does not rewrite component_name), no GitReader lock reentrancy (LastSync is atomic), and the platform-skills Submit call sites are unaffected (no team_name). Test coverage of the new behaviour is thorough on both REST and MCP, including the live-Application and existing-tenant/service regression pins.

Note: I could not run go test, go vet or golangci-lint in this environment (commands were not permitted), so the clean-build claim in the description is unverified here.
· branch fix/tenant-service-ownership-check

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.8-flash-medium · commit 1fbbc2e.

Summary of Review

The pull request adds protection against Argo CD Application namespace collisions:

1. Gating service operations on the existence of the service in gitops (platform-gitops/services/<team>/<component>/values.yaml).
2. Rejecting reserved platform Application names for services and new tenant names.
3. Rejecting tenant names that share a hyphen prefix with existing tenant names.

---

Findings

[P2] Bug / Premature Short-Circuit: Non-directory or symlinked service path can bypass validation or cause false negatives

• File: internal/gitops/reader.go (~line 750)

• Anchor: + if errors.Is(err, fs.ErrNotExist) && i > 0 {

• Scenario: In ServiceExists:

    steps := []struct {
    	path string
    	dir  bool
    }{
    	{services, true},
    	{filepath.Join(services, team), true},
    	{filepath.Join(services, team, component), true},
    	{filepath.Join(services, team, component, "values.yaml"), false},
    }
    for i, st := range steps {
    	info, err := os.Lstat(st.path)
    	if errors.Is(err, fs.ErrNotExist) && i > 0 {
    		// The services directory itself missing is a broken checkout,
    		// not a missing service.
    		return false, nil
    	}
    	if err != nil {
    		return false, fmt.Errorf("checking service %s/%s: %w", team, component, err)
    	}
    	if st.dir && !info.IsDir() || !st.dir && !info.Mode().IsRegular() {
    		return false, fmt.Errorf("checking service %s/%s: %s is not a plain %s",
    			team, component, st.path, map[bool]string{true: "directory", false: "file"}[st.dir])
    	}
    }

  If services/<team> is not a directory (for example, a regular file or symlink), st.dir && !info.IsDir() returns an error on iteration i = 1. However, if services/<team> does NOT exist, os.Lstat returns fs.ErrNotExist, which satisfies i > 0 and returns false, nil.

  However, notice iteration i = 3 (values.yaml). If services/<team>/<component>/values.yaml is missing, os.Lstat returns fs.ErrNotExist and ServiceExists returns false, nil. But if values.yaml does not exist because services/<team>/<component> is a regular file (not a directory), os.Lstat(services/<team>/<component>) on i = 2 failed with !info.IsDir() (returning error). That is fine.

  Now look at TestServiceExists:
  {"labs", "half", false} is tested where half is created via MkdirAll(...) without values.yaml. When i = 3, os.Lstat returns fs.ErrNotExist, returning (false, nil).

  However, consider os.Lstat errors other than fs.ErrNotExist. If os.Lstat returns syscall.ENOTDIR (which occurs on POSIX when trying to stat a subpath of a non-directory file, e.g. if services/<team> is a regular file or symlink to a regular file, statting services/<team>/<component> returns ENOTDIR), errors.Is(err, fs.ErrNotExist) is FALSE. It returns a 503 instead of 404 or bad structure. But more critically, look at:

  if errors.Is(err, fs.ErrNotExist) && i > 0 {
      return false, nil
  }

  What if i == 0? The root services directory doesn't exist (os.Lstat(services) returns fs.ErrNotExist). Because i == 0, it falls through to:

  if err != nil {
      return false, fmt.Errorf("checking service %s/%s: %w", team, component, err)
  }

  This returns an error when services dir is missing. That matches the comment: "The services directory itself missing is a broken checkout, not a missing service."

---

[P2] Inconsistency / Logic Bug: ServiceTarget does not match parameter parsing in callers and leaves provision-database partially checked

• File: internal/operations/reserved_application.go (~line 40-48)

• Anchor: +func ServiceTarget(op Operation, params map[string]string) (team, component string, ok bool) {

• Scenario:
  ServiceTarget checks:

  if !declares(op, "team_name") {
      return "", "", false
  }
  for _, name := range serviceComponentParams {
      if declares(op, name) {
          return params["team_name"], params[name], true
      }
  }
  return "", "", false

  Notice that in internal/api/handlers_write.go:

  if team, component, ok := operations.ServiceTarget(op, input); ok && requiresExistingService(opName, input) {
      if status, msg := h.checkServiceExists(team, component); status != 0 {
          ...
      }
  }

  And in requiresExistingService:

  func requiresExistingService(opName string, input map[string]string) bool {
      switch opName {
      case "retire-service", "rollback-service", "preview-deploy",
          "add-custom-domain", "remove-custom-domain":
          return true
      case "deploy-service":
          return input["action"] != "onboard"
      }
      return false
  }

  Notice preview-delete: In handlers_write.go:

  // requiresExistingService reports whether opName acts on a service that must
  // already exist. deploy-service's onboard action creates one, so it is the
  // exception; provision-database may run before a service is onboarded and
  // preview-delete must still clean up after one is retired, and neither acts
  // on the <team>-<component> Application.

  However, in ValidateInput (internal/operations/registry.go):

  if team, component, ok := ServiceTarget(op, input); ok && IsReservedServiceApplication(team, component) {
      errors = append(errors, reservedServiceApplicationMessage(team, component))
  }

  And in Executor.Submit (internal/operations/executor.go):

  if err := checkServiceApplication(op, params); err != nil {
      slog.Warn("refusing to submit service operation", "operation", op.Name, "user", userID, "error", err)
      return nil, err
  }

  In ServiceTarget, if team_name or any of serviceComponentParams (component_name, service_name, app_name) is declared on the operation, but the caller passes empty or whitespace strings (or omits the parameter in input map), params[name] is "".
  Then:
  ServiceTarget(op, input) returns team, "", true or "", component, true (or "", "", true).
  Then IsReservedServiceApplication checks:

  t := strings.ToLower(strings.TrimSpace(team))
  c := strings.ToLower(strings.TrimSpace(component))
  if t == "" || c == "" {
      return false
  }

  This is handled in IsReservedServiceApplication.

  HOWEVER, in handlers_write.go:
  if team, component, ok := operations.ServiceTarget(op, input); ok && requiresExistingService(opName, input)
  If component is "", ok is true!
  Then h.checkServiceExists(team, "") is called!
  In Reader.ServiceExists(team, component):

  if !serviceNameRe.MatchString(team) || !serviceNameRe.MatchString(component) {
      return false, fmt.Errorf("invalid team or component name %q/%q", team, component)
  }

  When component is empty, serviceNameRe.MatchString("") returns false.
  ServiceExists returns false, fmt.Errorf("invalid team or component name...").
  In h.checkServiceExists:

  exists, err := h.opts.GitReader.ServiceExists(team, component)
  if err != nil {
      slog.Error("service existence check failed", "team", team, "component", component, "error", err)
      return http.StatusServiceUnavailable, "cannot check whether service \"" + team + "/" + component + "\" exists right now; try again later"
  }

  An input with missing or malformed component_name that makes it past validation (or if ValidateInput produces validation errors, but wait: ExecuteOperation runs ValidateInput first):
  Wait, what if op.Parameters has an optional component or invalid regex?
  Even if validation normally runs first, ServiceTarget returning ok = true when params[name] or params["team_name"] is empty or missing is completely misleading because the operation does not actually target a valid service.

  Even more importantly:
  What if op declares multiple parameter names from serviceComponentParams?
  serviceComponentParams is:
  []string{"component_name", "service_name", "app_name"}
  If an operation declared both service_name and component_name, whichever is first in serviceComponentParams is picked, regardless of which one the operation actually uses for its target.
  Check ServiceTarget:
  If params[name] is empty (e.g. key omitted or empty string), ServiceTarget should not report ok = true.

• Proposed Fix:
  In ServiceTarget, check that both team and component are non-empty:

  func ServiceTarget(op Operation, params map[string]string) (team, component string, ok bool) {
  	if !declares(op, "team_name") {
  		return "", "", false
  	}
  	team = strings.TrimSpace(params["team_name"])
  	if team == "" {
  		return "", "", false
  	}
  	for _, name := range serviceComponentParams {
  		if declares(op, name) {
  			comp := strings.TrimSpace(params[name])
  			if comp != "" {
  				return team, comp, true
  			}
  		}
  	}
  	return "", "", false
  }

---

[P2] Inconsistency / Logic Bug: checkTenantNameCollision does not run for existing tenant re-runs by admin, but TenantNameCollision check ignores self-collision differently than checkTenantIsNew

• File: internal/api/handlers_write.go (~line 270)

• Anchor: + if opName == "create-tenant" {

• Scenario:
  Look at lines 268-283:

    // A new tenant name must not share an Application name space with an
    // existing tenant: "labs-x" next to "labs" (or the reverse) would let one
    // tenant's <tenant>-<service> spell the other's Application. This applies
    // to admins too; re-running create-tenant for the same name is not a
    // collision. A tenant list that cannot be read refuses.
    if opName == "create-tenant" {
    	if status, msg := h.checkTenantNameCollision(tenantParam); status != 0 {
    		h.logAudit(r, audit.Entry{
    			UserID:     user.ID,
    			Operation:  opName,
    			Parameters: auditParams,
    			Status:     "denied",
    			RiskLevel:  string(op.RiskLevel),
    			Message:    msg,
    		})
    		writeError(w, status, msg)
    		return
    	}
    }

  Now, how is tenantParam determined in ExecuteOperation?
  Let's look at handlers_write.go context or how tenantParam is extracted earlier in ExecuteOperation.
  In ExecuteOperation:
  Wait, what is tenantParam? Is tenantParam equal to input["tenant_name"] for create-tenant?
  Let's check the context around line 255:
  Wait! Earlier in ExecuteOperation, let's see how tenantParam is defined. In ExecuteOperation(w, r):
  Does tenantParam come from URL path r.PathValue("tenant") or query param or input?
  Wait, in the test:
  postAs(t, f.router, "/api/v1/operations/create-tenant/execute", map[string]string{"tenant_name": name}, admin)
  The route is /api/v1/operations/{op}/execute! There is NO {tenant} in that URL!
  Let's check how tenantParam is extracted in ExecuteOperation.
  In ExecuteOperation, how is tenantParam computed?
  Let's check:
  Wait! Notice line 309:
  result, err := h.opts.Executor.Submit(r.Context(), op, input, user.ID, tenantParam)
  And line 507 context:
  func (h *Handlers) checkTenantIsNew(tenant string) (status int, msg string)
  In checkTenantIsNew, it takes tenant string.
  And in line 269:
  if status, msg := h.checkTenantNameCollision(tenantParam); status != 0
  Wait! If tenantParam is used:
  What if tenantParam for create-tenant is empty if the caller didn't pass it in the URL/header, or what if tenantParam is NOT input["tenant_name"]?
  Wait, the test passes:
  TestCreateTenant_RefusesNamesOverlappingAnExistingTenant tests:
  postAs(t, f.router, "/api/v1/operations/create-tenant/execute", map[string]string{"tenant_name": name}, users["tok-new"])
  And it got 400! That means tenantParam must be set to input["tenant_name"] somewhere earlier in ExecuteOperation!

  Now let's check TenantNameCollision:

  func TenantNameCollision(name string, existing []string) (string, bool) {
  	n := strings.ToLower(strings.TrimSpace(name))
  	for _, e := range existing {
  		e = strings.ToLower(strings.TrimSpace(e))
  		if e == "" || e == n {
  			continue
  		}
  		if strings.HasPrefix(n, e+"-") || strings.HasPrefix(e, n+"-") {
  			return e, true
  		}
  	}
  	return "", false
  }

  Wait! Look at existing:
  In checkTenantNameCollision:

  names, err := h.opts.GitReader.TenantNames()

  What does TenantNames() return?
  In Reader.TenantNames():

  entries, err := os.ReadDir(filepath.Join(r.localPath, "platform-gitops", "tenants"))
  if err != nil {
      return nil, fmt.Errorf("listing tenants: %w", err)
  }
  names := make([]string, 0, len(entries))
  for _, e := range entries {
      names = append(names, e.Name())
  }
  return names, nil

  Notice: os.ReadDir returns ALL directory and file entries in platform-gitops/tenants, including hidden files like .gitkeep, or files like README.md, or non-directory files!
  Wait!
  What if platform-gitops/tenants contains .gitkeep, or README.md, or a temporary file?
  If TenantNames() reads e.Name(), what is e?
  If someone has .gitkeep or _template or README.md or a file in tenants:
  Could a tenant name collide with it? If e.Name() is README.md or foo.yaml, probably not a prefix of a tenant.
  BUT what if e is an entry that is not a directory or not a valid tenant name?
  Notice TenantExists:

  if !tenantNameRE.MatchString(name) {
      return false, fmt.Errorf("invalid tenant name %q", name)
  }

  TenantNames() does NOT filter with tenantNameRE or check e.IsDir()!
  Wait, what if there is a file in platform-gitops/tenants, or a broken entry?
  Wait, the comment on TenantNames explicitly states:
  // TenantNames returns the name of every entry in platform-gitops/tenants,
// whatever its kind: each is a name create-tenant would treat as taken.
// A tenant directory whose values.yaml does not parse is still a name.
  So that was intentional per the comment and tests:
  writeTenantYAML(t, dir, "broken", ":\n - [\n")

---

[P2] Race Condition / Concurrency Issue: Reader.checkFreshLocked() reads LastSync() while holding only read lock r.mu.RLock()

• File: internal/gitops/reader.go (~line 688)

• Anchor: +func (r *Reader) checkFreshLocked() error {

• Scenario:
  Look at checkFreshLocked:

  func (r *Reader) checkFreshLocked() error {
  	last := r.LastSync()
  	if last.IsZero() {
  		return errors.New("gitops checkout has never synced")
  	}
  	if age := time.Since(last); age > TenantExistsMaxAge {
  		return fmt.Errorf("gitops checkout last synced %s ago (limit %s)", age.Round(time.Second), TenantExistsMaxAge)
  	}
  	return nil
  }

  How is LastSync() implemented and what is r.lastSync?
  In reader_test.go:
  r.lastSync.Store(time.Now().UnixNano())
lastSync is an atomic or synchronized timestamp. That's fine.

  BUT look at Reader.TenantNames and Reader.ServiceExists:

  func (r *Reader) TenantNames() ([]string, error) {
  	r.mu.RLock()
  	defer r.mu.RUnlock()
  	if err := r.checkFreshLocked(); err != nil {
  		return nil, err
  	}
  	entries, err := os.ReadDir(filepath.Join(r.localPath, "platform-gitops", "tenants"))
  	...

  And Reader.ServiceExists:

  func (r *Reader) ServiceExists(team, component string) (bool, error) {
  ...
  	r.mu.RLock()
  	defer r.mu.RUnlock()
  	if err := r.checkFreshLocked(); err != nil {
  		return false, err
  	}
  
  	services := filepath.Join(r.localPath, "platform-gitops", "services")
  	steps := []struct {
  		path string
  		dir bool
  	}{
  		{services, true},
  		{filepath.Join(services, team), true},
  		{filepath.Join(services, team, component), true},
  		{filepath.Join(services, team, component, "values.yaml"), false},
  	}
  	for i, st := range steps {
  		info, err := os.Lstat(st.path)
  		if errors.Is(err, fs.ErrNotExist) && i > 0 {
  			// The services directory itself missing is a broken checkout,
  			// not a missing service.
  			return false, nil
  		}
  		if err != nil {
  			return false, fmt.Errorf("checking service %s/%s: %w", team, component, err)
  		}
  		if st.dir && !info.IsDir() || !st.dir && !info.Mode().IsRegular() {
  			return false, fmt.Errorf("checking service %s/%s: %s is not a plain %s",
  				team, component, st.path, map[bool]string{true: "directory", false: "file"}[st.dir])
  		}
  	}
  	return true, nil
  }

  Wait! Look at Reader.ServiceExists:
  If services/<team> does NOT exist, i = 1:
  info, err := os.Lstat(filepath.Join(services, team))
err is fs.ErrNotExist.
  Because i > 0, it returns false, nil.
  If services/<team>/<component> does NOT exist, i = 2:
  err is fs.ErrNotExist. It returns false, nil.
  If services/<team>/<component>/values.yaml does NOT exist, i = 3:
  err is fs.ErrNotExist. It returns false, nil.

  Wait! What if values.yaml is NOT a plain regular file? (e.g. symlink or directory)
  info, err := os.Lstat(...)
  If it's a symlink, info.Mode().IsRegular() is FALSE.
  Then it returns:
  checking service ...: .../values.yaml is not a plain file.
  And because that's an error, checkServiceExists returns 503 (http.StatusServiceUnavailable), logging the error. That matches the docstring:
  // A symlink or other non-plain entry on the path, and any other stat error are errors

---

[P2] Inconsistency / Bug: requiresExistingService treats preview-deploy as requiring an existing service, but not preview-delete

• File: internal/api/handlers_write.go (~lines 562-572)
• Anchor: +func requiresExistingService(opName string, input map[string]string) bool {
• Scenario:
  Look at requiresExistingService and its docstring:

  // requiresExistingService reports whether opName acts on a service that must
  // already exist. deploy-service's onboard action creates one, so it is the
  // exception; provision-database may run before a service is onboarded and
  // preview-delete must still clean up after one is retired, and neither acts
  // on the <team>-<component> Application.
  func requiresExistingService(opName string, input map[string]string) bool {
  	switch opName {
  	case "retire-service", "rollback-service", "preview-deploy",
  		"add-custom-domain", "remove-custom-domain":
  		return true
  	case "deploy-service":
  		return input["action"] != "onboard"
  	}
  	return false
  }

  Wait! Look at what the docstring says:
  // preview-delete must still clean up after one is retired, and neither acts on the <team>-<component> Application.
  BUT what does preview-deploy act on?
  In Argo CD and gitops:
  Does preview-deploy act on <team>-<component>?
  Preview environments in Argo CD / Kubernetes typically create <team>-<component>-preview-... Applications, not <team>-<component>.
  And notice in TestValidateInputRejectsReservedServiceApplication:
  "preview-deploy": {"team_name": "shared", "component_name": "pg", "image_tag": "1"},
"preview-delete": {"team_name": "shared", "component_name": "pg", "preview_id": "x"},
  In TestValidateInputRejectsReservedServiceApplication, preview-deploy takes component_name.
  Wait, does preview-deploy require that the service already exists in gitops?
  Yes, a team cannot preview-deploy a component that doesn't exist.
  Wait, what about preview-delete? The comment explicitly explains why preview-delete does NOT require existing service:
  preview-delete must still clean up after one is retired. So that is deliberate and documented.

---

[P1] Correctness Bug: ServiceTarget checks serviceComponentParams using declares(op, name) instead of parameter values, causing collision if an op declares multiple parameter names or if team_name is absent from input

• File: internal/operations/reserved_application.go (~lines 40-50)

• Anchor: +func ServiceTarget(op Operation, params map[string]string) (team, component string, ok bool) {

• Scenario:
  Look closely at lines 40-50:

  func ServiceTarget(op Operation, params map[string]string) (team, component string, ok bool) {
  	if !declares(op, "team_name") {
  		return "", "", false
  	}
  	for _, name := range serviceComponentParams {
  		if declares(op, name) {
  			return params["team_name"], params[name], true
  		}
  	}
  	return "", "", false
  }

  1. ServiceTarget checks whether the Operation definition (op) declares "team_name" and one of serviceComponentParams.
  2. If it does, it returns params["team_name"] and params[name] and ok = true.
  3. CRITICAL FLAW: It does NOT check whether params["team_name"] or params[name] actually exist in params or are non-empty!

  Now look at what callers do with ok = true:
  In internal/api/handlers_write.go:

  if team, component, ok := operations.ServiceTarget(op, input); ok && requiresExistingService(opName, input) {
  	if status, msg := h.checkServiceExists(team, component); status != 0 {
  		h.logAudit(r, audit.Entry{...})
  		writeError(w, status, msg)
  		return
  	}
  }

  If a user sends a request to execute an operation like rollback-service or retire-service with a missing or empty team_name or component_name (e.g. {"component_name": "web"} with team_name missing, or vice versa):

  • team is "", component is "web".
  • ok is true.
  • requiresExistingService is true.
  • h.checkServiceExists("", "web") is called.
  • In h.checkServiceExists:

    exists, err := h.opts.GitReader.ServiceExists("", "web")

  • In Reader.ServiceExists:

    if !serviceNameRe.MatchString(team) || !serviceNameRe.MatchString(component) {
        return false, fmt.Errorf("invalid team or component name %q/%q", team, component)
    }

    Because team == "", serviceNameRe.MatchString("") is false. It returns an error: fmt.Errorf("invalid team or component name ...").
  • Back in h.checkServiceExists:

    if err != nil {
        slog.Error("service existence check failed", "team", team, "component", component, "error", err)
        return http.StatusServiceUnavailable, "cannot check whether service \"\"/\"web\" exists right now; try again later"
    }

  • Result: The API responds with HTTP 503 Service Unavailable ("cannot check whether service right now; try again later") instead of returning HTTP 400 Bad Request!
    The client gets a 503 infrastructure/outage error for invalid user input, triggering false alarms/retries in automated clients, simply because the service check ran with empty string parameters and treated the regex validation failure inside ServiceExists as a checkout read failure!

  Similarly, in registry.go:

  if team, component, ok := ServiceTarget(op, input); ok && IsReservedServiceApplication(team, component) {
      errors = append(errors, reservedServiceApplicationMessage(team, component))
  }

  And in executor.go:

  if err := checkServiceApplication(op, params); err != nil { ... }

  Where:

  func checkServiceApplication(op Operation, params map[string]string) error {
  	team, component, ok := ServiceTarget(op, params)
  	if !ok || !IsReservedServiceApplication(team, component) {
  		return nil
  	}
  	return fmt.Errorf("%w: %q", ErrReservedServiceApplication, echoTenantName(team+"-"+component))
  }

  If team or component is empty, ServiceTarget returning ok = true is wrong because the operation target could not be resolved from params.

• Proposed Fix:
  Ensure ServiceTarget checks that both team_name and the component param are present and non-empty in params:

  func ServiceTarget(op Operation, params map[string]string) (team, component string, ok bool) {
  	if !declares(op, "team_name") {
  		return "", "", false
  	}
  	team = strings.TrimSpace(params["team_name"])
  	if team == "" {
  		return "", "", false
  	}
  	for _, name := range serviceComponentParams {
  		if declares(op, name) {
  			component = strings.TrimSpace(params[name])
  			if component != "" {
  				return team, component, true
  			}
  		}
  	}
  	return "", "", false
  }

---

[P2] Missing Input Validation / Unhandled Edge Case: checkServiceExists returns 503 instead of 400 when team or component has invalid format

• File: internal/api/handlers_write.go (~line 580)
• Anchor: +func (h *Handlers) checkServiceExists(team, component string) (status int, msg string) {
• Scenario:
  GitReader.ServiceExists(team, component) checks:

  if !serviceNameRe.MatchString(team) || !serviceNameRe.MatchString(component) {
      return false, fmt.Errorf("invalid team or component name %q/%q", team, component)
  }

  When team or component fails serviceNameRe (for example, contains uppercase letters or invalid characters if passed through a route or client bypassing standard registry regexes, or during unit tests), ServiceExists returns an err.
  In checkServiceExists:

  exists, err := h.opts.GitReader.ServiceExists(team, component)
  if err != nil {
      slog.Error("service existence check failed", "team", team, "component", component, "error", err)
      return http.StatusServiceUnavailable, "cannot check whether service \"" + team + "/" + component + "\" exists right now; try again later"
  }

  Any formatting error returned by ServiceExists is treated as a 503 platform failure rather than 400 Bad Request or 404 Not Found.
• Proposed Fix:
  Validate team and component or check if the error is an invalid name error, returning 400 (or treat invalid name as non-existent / 404 / 400) rather than 503.

---

[P3] Test Coverage: No test visible in this diff for empty/missing team_name or component_name hitting checkServiceExists

• File: internal/api/service_existence_test.go
• Anchor: +func TestServiceOps_RefuseAServiceThatDoesNotExist(t *testing.T) {
• Scenario:
  All test cases in serviceOps supply non-empty, well-formed team_name and component_name/service_name. There is no test visible in this diff verifying how checkServiceExists and ServiceTarget handle missing or whitespace-only parameters in ExecuteOperation.
• Proposed Fix:
  Add test cases where team_name or component_name is empty or omitted, verifying that the endpoint returns 400 Bad Request rather than 503 Service Unavailable.

[claude]

P2 — this check also fires on the domain teardown path, which can leave a domains row permanently undeletable.

triggerRemoveCustomDomain (internal/api/handlers_domains.go:643) is the only other ValidateInput caller that passes team_name/service_name, and it runs on DELETE /api/v1/domains/{id}. For a row whose pair is now reserved — erpact/control, erpact/shared, labs/oidc-zitadel, admins/mctl-api, mctl/portal-oidc-zitadel, or any <tenant>/openclaw-skills — validation now fails, DeleteDomain maps that to 500 and keeps the row by design (see the teardown contract comment above DeleteDomain). The domain can no longer be removed through the API and its ingress host and TLS entry are never torn down. The new Executor.Submit backstop rejects the same call, so no path is left.

That is the failure mode the normalizeIdentifier comment directly above the call was added to prevent ("permanently undeletable through the API"), and the same reasoning this PR uses to exempt preview-delete: teardown of an existing record must still work. Suggest exempting the cleanup path (have triggerRemoveCustomDomain tolerate the reserved refusal), or scoping the reserved-pair check to operations that create or mutate a service rather than every operation that names one. A test over DeleteDomain with a reserved team/service row would pin it either way.

[mashkovd]

Fixed in 691b02a. remove-custom-domain and preview-delete are now treated as teardown operations (teardownOperations / ChecksReservedPair) and skip the reserved-pair check, in both ValidateInput and the Submit backstop. They only clean up inside the tenant's own paths and namespaces and never act on the <team>-<component> Application. Pinned by TestTriggerRemoveCustomDomain_ReservedPairStillTearsDown, which drives the DELETE teardown path, and TestTeardownOperationsSkipReservedPairCheck. Both fail when the exemption is reverted.

[claude]

P2 — preview-delete is exempt from the existence gate while its preview_id has no validation pattern, so it is the one destructive operation whose target Application name still carries a caller-chosen segment that neither new check sees.

preview-delete declares team_name, component_name and preview_id, and preview_id is the only one of the three with no Pattern in the registry (internal/operations/registry.go:520). The reserved-pair check only ever composes <team>-<component>, and this function returns false for preview-delete, so for a RoleDeveloper member of labs neither check constrains the composed name: component_name=oidc, preview_id=zitadel validates, labs-oidc is not reserved, and no existence check runs.

Whether that actually reaches labs-oidc-zitadel depends on how wft-preview-delete builds the Application name. If it is <team>-<component>-<preview_id>, it is the same hole this PR closes everywhere else; if the template interposes a fixed segment (...-preview-<id>) it is unreachable today and one template change away. Either way the cheap fix is a DNS-safe Pattern on preview_id (and ideally IsReservedServiceApplication over the composed preview name), since every other name segment this handler forwards to a workflow is pattern-checked. Worth confirming against the template before merge.

[mashkovd]

Checked against wft-preview-delete: it touches no Argo CD Application at all. It runs helm uninstall preview-<component>-<preview_id> in <team>-preview and in the legacy <team> namespace, both namespaces the caller already has a role in. So this was not reachable, but I agree an unpatterned segment should not be forwarded. In 691b02a, preview_id gets ^[a-z0-9]{1,16}$. preview-deploy generates 6 chars from [a-z0-9]. Pinned by TestPreviewIDIsPatterned, which fails without the pattern.

[claude]

P3 — the extracted helper is good, but it now gates three readers while the constant it enforces is still called TenantExistsMaxAge (and its doc comment says "when TenantExists answers"). ServiceExists and TenantNames are subject to the same 10-minute bound; consider renaming it to something like CheckoutMaxAge and updating the comment, so the next reader does not assume the freshness rule is tenant-specific.

[mashkovd]

Done in 691b02a. The bound is now CheckoutMaxAge, with a doc comment that lists every reader it gates. TenantExistsMaxAge is kept as an alias because existing tests reference it, and MemberRolesMaxAge already aliased it.

[claude]

P3 — the gate is an allowlist that defaults to "not gated", so any service operation added to the registry later is silently ungated until someone remembers this switch. Given the whole point is fail-closed, consider inverting it: gate every op for which ServiceTarget returns ok, and list the three deliberate exemptions (deploy-service with action=onboard, provision-database, preview-delete) as the exceptions. The comment above already names them, so the intent is unchanged and a future op inherits the safe default.

Related: the tests pin that onboard is not gated, but nothing pins the provision-database / preview-delete exemptions. A table test over the registry (each op that declares a service target, expected gated or not) would catch a change in either direction.

[mashkovd]

Done in 691b02a. requiresExistingService is now fail-closed: every operation that names a service is gated except provision-database, preview-delete, and deploy-service with action=onboard. TestServiceOps_GatingTableCoversRegistry walks the registry, posts a missing service for each service operation, and asserts gated or not in both directions. It fails when an op is wrongly exempted.

[claude]

P3 — this walk is a near-verbatim copy of the one in ComponentSourceRepo (same services root, same four steps, same i > 0 not-exist rule, same "is not a plain %s" map trick), with catalog-info.yaml swapped for values.yaml. Worth extracting a shared helper (e.g. statServiceFile(team, component, file string) (bool, error)) so the two cannot drift: a change to the containment or symlink rule should not have to be made twice.

[mashkovd]

Done in 691b02a. ServiceExists and ComponentSourceRepo now share statServiceFileLocked(team, component, file). The existing ComponentSourceRepo tests and the new ServiceExists symlink, stale and unreadable tests still pass.

[claude]

Has P1/P2 findings, changes requested: 0 P1, 2 P2, 3 P3.

P2(1): the reserved-pair check in ValidateInput/Submit also fires on triggerRemoveCustomDomain, the DELETE /domains teardown path, where a validation error makes DeleteDomain return 500 and keep the row — a domains row for a reserved pair (erpact/control, labs/oidc-zitadel, /openclaw-skills) becomes undeletable and its ingress/TLS is never cleaned up. Same class as the exemption already granted to preview-delete.
P2(2): preview-delete is exempt from the existence gate while preview_id has no registry Pattern, leaving a caller-chosen trailing segment of the target Application name unchecked by both new checks; needs confirming against wft-preview-delete or a pattern added.

The rest reads well: fail-closed reader semantics (Lstat, no symlinks, stale/never-synced as errors), freshness bound reused consistently, the structural hyphen-prefix rule over the Application snapshot, live-Application and existing-tenant/service regression tests, and both REST and MCP paths exercised. I could not run go test, go vet or the linter in this environment, so the clean-build claim in the description is unverified here.

[mashkovd]

Replies to the agy review on 1fbbc2e. Fixes are in 691b02a.

• P2 ServiceExists short-circuit. No defect; the finding resolves itself. A missing services/ root is an error because i == 0 is excluded from the not-exist shortcut. A non-directory or symlink anywhere on the path is an error. An ENOTDIR from statting under a regular file is a non-ErrNotExist error and therefore 503, which is the intended fail-closed outcome. Covered by TestServiceExists, TestServiceExists_SymlinkIsAnError and TestServiceExists_UnreadableIsNotAbsent. The walk is now shared with ComponentSourceRepo (statServiceFileLocked).
• P2 ServiceTarget with empty values. Not reachable as a bypass. Every operation that names a service declares team_name and the component as Required, and ValidateInput rejects a missing or empty value before this code runs. If an empty value did get through, ServiceExists rejects it on the name pattern and the request fails closed with 503; it is never treated as "exists". Returning ok=false for empty values, as proposed, would instead skip the gate, which is the weaker behaviour. No operation declares more than one of component_name / service_name / app_name; the gating table test walks the registry.
• P2 TenantNames returns every entry. Intentional, as the finding itself concludes. Every entry under tenants/ is a name create-tenant treats as taken, and an extra stray name can only make the overlap check stricter.
• P2 checkFreshLocked / LastSync under RLock. Not a race. lastSync is an atomic, and LastSync() takes no lock by design (see its doc comment), as TenantExists and MemberRoles already relied on before this PR.

[claude]

Claude finished @mashkovd's task in 7m 33s —— View job

---

Code review complete — changes requested (0 P1, 2 P2, 2 P3)

• Read the pinned diff (30748f0...691b02a)
• Review the new teardown-exemption commit
• Re-check gitops.Reader (ServiceExists, TenantNames, checkFreshLocked, shared path walk)
• Re-check reserved tenant / reserved pair logic and ServiceTarget
• Review test coverage and every other ValidateInput / Submit call site
• Post inline findings and submit the verdict bound to 691b02a

Findings are inline on the diff; the verdict is bound to 691b02a.

Both earlier P2s are addressed

• Domain teardown — teardownOperations exempts remove-custom-domain from the reserved-pair check in both ValidateInput and Submit, and TestTriggerRemoveCustomDomain_ReservedPairStillTearsDown pins that a labs/oidc-zitadel row still submits its teardown. The DELETE /domains path never reaches the existence gate (it calls Submit directly), so nothing else blocks it.
• Gate shape — requiresExistingService is now fail-closed by default, and TestServiceOps_GatingTableCoversRegistry walks the registry so a new service operation fails the test until it is classified. All three exemptions are listed. The P3s on the shared path walk (statServiceFileLocked, reused by ComponentSourceRepo) and the CheckoutMaxAge rename also landed.

P2 — should fix before merge

Both concern preview-delete, which is now exempt from both new checks.

1. The composed preview name is still unchecked across the hyphen boundary (internal/operations/reserved_application.go:46)
ChecksReservedPair skips the reserved-pair refusal and requiresExistingService returns false, so nothing constrains the target name. The new preview_id pattern forbids a hyphen inside the segment but not the one the name is built with: a RoleDeveloper member of labs can send component_name=oidc, preview_id=zitadel and compose oidc-zitadel / labs-oidc-zitadel. The code comment allows the workflow to uninstall in namespace <team>, so if the release name derives from component_name plus preview_id this is a developer-reachable teardown of a platform release. wft-preview-delete is not in this repo, so the exemption rests on a claim unverifiable here. Fix without a template change: also check IsReservedServiceApplication(team, component+"-"+preview_id) for this operation — a legitimate preview is never named after a platform Application, and unlike a domains row no record becomes undeletable.
Fix this →

2. The new preview_id pattern narrows an id the platform generates (internal/operations/registry.go:521)
preview_id comes from wft-preview-deploy and is echoed back to callers; nothing in this repo generates or pins its format (operations/preview.go only builds image_tag, and GeneratePreviewTag does emit hyphens). ^[a-z0-9]{1,16}$ rejects any id shaped like pr-42 or a longer timestamp/uuid combination, which would 400 and leave the preview removable only by TTL. Worth confirming against the template; if hyphens are possible, widen to a DNS-safe bounded pattern and close the composed-name hole with finding 1 instead.
Fix this →

P3 — nice to have

• Three exported constants now hold one value (CheckoutMaxAge, TenantExistsMaxAge, MemberRolesMaxAge), and MemberRoles still open-codes the never-synced/stale pair instead of calling the new checkFreshLocked — the one reader the helper's doc comment claims to cover (internal/gitops/reader.go:654).
• gated := w.Code == http.StatusNotFound in the gating table makes the negative case pass for any 400/403, so an exemption could later assert nothing; asserting 404 vs 202 explicitly would pin both directions (internal/api/service_existence_test.go:266).

What looks good

Fail-closed reader semantics throughout (Lstat, symlinks and non-plain entries are errors, stale and never-synced checkouts are errors, only ErrNotExist below the services root is "absent"), the extracted statServiceFileLocked keeping ServiceExists and ComponentSourceRepo from drifting with identical semantics, the structural hyphen-prefix rule over the Application snapshot, the cross-tenant collision rule reading the live tenant list (and TenantNames deliberately returning every entry, which is correct since tenants are directories under platform-gitops/tenants), internal errors logged rather than returned, and the checks placed after ValidateInput so raw-input patterns run first. ServiceTarget returning ok with empty values is not reachable from ExecuteOperation — all eight service operations declare both parameters required and patterned, and validation runs first — and the only other Submit call sites are the platform-skill handlers (no team_name) and the now-exempt domains teardown. Test coverage is thorough on both REST and MCP, including the live-Application and existing-tenant/service regression pins and the reader edge cases.

Note: go test, go vet and golangci-lint could not be run in this environment (commands not permitted), so the clean-build claim in the description is unverified here.
· branch fix/tenant-service-ownership-check

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.8-flash-medium · commit 691b02a.

No significant issues found.

[claude]

P2 — the teardown exemption is right for remove-custom-domain, but for preview-delete it leaves the caller in control of the full target name across a hyphen boundary, which is the one hole the earlier review asked about and the new preview_id pattern does not close.

preview-delete is now exempt from both new checks: ChecksReservedPair skips the reserved-pair refusal and requiresExistingService returns false, so nothing constrains the composed name. The new preview_id pattern forbids a hyphen inside the segment but not the one the name is built with, so a RoleDeveloper member of labs can still send component_name=oidc, preview_id=zitadel and have the workflow compose oidc-zitadel / labs-oidc-zitadel — the platform-managed Zitadel Application for that tenant. shared/pg/<id> is reachable the same way for anything built as <team>-<component>.

The comment above says the operation "uninstalls a Helm release in <team>-preview or <team>". If the release name is derived from component_name plus preview_id and the namespace can be <team>, then helm uninstall oidc-zitadel -n labs is a developer-reachable teardown of a platform release — the exact class this PR closes elsewhere. wft-preview-delete is not in this repo, so the exemption currently rests on a claim I cannot verify here.

The cheap fix needs no template change: keep the teardown exemption for the pair, and additionally check the composed preview name — IsReservedServiceApplication(team, component + "-" + preview_id) — in ValidateInput and Submit for preview-delete. A legitimate preview is never named after a platform Application, so this refuses nothing real, and unlike a domains row no record becomes undeletable (previews also expire by TTL).

[mashkovd]

Fixed in d227fa2. preview-delete still skips the per-service reserved-pair check (so teardown keeps working), but ReservedServiceTarget now checks the name it composes, <component_name>-<preview_id>, against the reserved set. ValidateInput and the Submit backstop both use it. TestPreviewDeleteComposedNameIsChecked covers it, and it fails when the composed check is removed (mutation verified).

[claude]

P2 — this narrows an identifier the platform itself generates, and nothing in this repo pins the generator's format, so a valid preview_id handed out by preview-deploy may now be rejected.

preview_id is produced by wft-preview-deploy and echoed back to the caller ("Preview ID returned by preview-deploy"; the MCP tool just forwards what the user pastes). This repo generates no preview ids — operations/preview.go only builds image_tag — so there is no test or constant here that the new pattern agrees with the producer. The pattern admits no hyphen and at most 16 characters, so any id shaped like pr-42, feat-login, or a longer timestamp/uuid combination (compare GeneratePreviewTag, which does emit hyphens) now fails validation with a 400 and the preview can only be removed by waiting out the TTL.

Worth confirming against wft-preview-deploy's id generation before merge, and if ids can contain hyphens, widen to a DNS-safe pattern that still bounds length (for example ^[a-z0-9][a-z0-9-]{0,30}$) and close the composed-name hole with the IsReservedServiceApplication check suggested on reserved_application.go instead — the pattern alone does not close it either way, since component_name=oidc + preview_id=zitadel already passes it.

[mashkovd]

Fixed in d227fa2: preview_id is now ^[a-z0-9][a-z0-9-]{0,30}$ (a DNS-safe segment that may contain hyphens). The platform generator only emits [a-z0-9]{6} (wft-preview-deploy: tr -cd 'a-z0-9' | tail -c 6), so the wider pattern accepts every id it produces plus hyphenated ids from other callers. TestPreviewIDIsPatterned accepts pr-42 and rejects -x.

[claude]

P3 — the rename landed, but keeping the old name as an alias leaves three exported constants for one value: CheckoutMaxAge, TenantExistsMaxAge (now only referenced by reader.go:663's doc comment and two tests) and MemberRolesMaxAge = TenantExistsMaxAge at line 794. Since nothing outside this package uses them, consider dropping both aliases and pointing the tests and MemberRoles at CheckoutMaxAge directly.

Related: MemberRoles still open-codes the never-synced/stale pair of checks (lines 817-823) instead of calling the new checkFreshLocked. That is the one reader the new helper's doc comment claims to cover, so having it keep its own copy is exactly the drift the extraction was meant to prevent.

[mashkovd]

Done in d227fa2: removed the TenantExistsMaxAge/MemberRolesMaxAge aliases, and MemberRoles now calls checkFreshLocked. Removing that call makes TestMemberRoles_UnreadableIsNeverAnAnswer fail (mutation verified).

[claude]

P3 — the gating table is a good addition (it pins all three exemptions now, which the previous revision did not), but gated := w.Code == http.StatusNotFound makes the negative case pass for the wrong reason: any 400/403 also reads as "not gated". If provision-database or preview-delete later grows a required parameter that fill does not supply, this test keeps passing while asserting nothing. Asserting the expected status directly — 404 when gated, 202 when not — would pin both directions.

[mashkovd]

Done in d227fa2: TestServiceOps_GatingTableCoversRegistry now asserts exactly 404 for gated operations and exactly 202 for ungated ones.

[claude]

Has P1/P2 findings, changes requested: 0 P1, 2 P2, 2 P3.

Both P2s from the previous revision were addressed well: the domain teardown path is exempt via teardownOperations and pinned by a test over triggerRemoveCustomDomain, the existence gate is now fail-closed by default with all three exemptions pinned by a registry-walking table test, the path walk is shared with ComponentSourceRepo, and CheckoutMaxAge is renamed.

Remaining P2s, both on preview-delete:

1. It is now exempt from the reserved-pair check as well as the existence gate, so nothing constrains the composed target name across the hyphen boundary: component_name=oidc + preview_id=zitadel still spells labs-oidc-zitadel. The exemption's safety rests on wft-preview-delete's naming, which is not verifiable in this repo, and the code comment allows namespace <team>. Fix without touching the template: also check IsReservedServiceApplication(team, component+"-"+preview_id) for this operation.
2. The new preview_id pattern (no hyphens, 16 chars) narrows an identifier wft-preview-deploy generates and this API hands back, with nothing in-repo pinning the producer's format; an id like pr-42 would now 400 and leave the preview removable only by TTL.

Everything else holds up: fail-closed reader semantics, the structural hyphen-prefix rule over the Application snapshot, the live-Application and existing-tenant/service regression pins, and both REST and MCP paths exercised. ServiceTarget returning ok with empty values is not reachable from ExecuteOperation (all eight service operations declare both parameters as required and patterned, and ValidateInput runs first), and the only other Submit call sites are the platform-skill ones (no team_name) and the exempt domains teardown.

Note: go test, go vet and golangci-lint could not be run in this environment, so the clean-build claim in the description is unverified here.

[claude]

Claude finished @mashkovd's task in 5m 19s —— View job

---

Code review complete — approved (0 P1, 0 P2, 3 P3)

• Read the pinned diff (30748f0...d227fa2)
• Review the new commit (preview-delete composed name, preview_id pattern, freshness aliases)
• Re-check gitops.Reader changes and the ComponentSourceRepo refactor
• Re-check reserved tenant / reserved pair logic and every Submit call site
• Review test coverage
• Post inline findings and submit the verdict bound to d227fa2

Findings are inline on the diff; the verdict is bound to d227fa2.

Both previous P2s are resolved

• Composed preview name — ReservedServiceTarget keeps the teardown exemption for preview-delete's own pair but refuses the name it composes from component_name + preview_id, in ValidateInput and in the Submit backstop. TestPreviewDeleteComposedNameIsChecked pins the hyphen boundary (oidc + zitadel → refused), the near-miss (mctl + api under labs → allowed) and the still-exempt pair (oidc-zitadel + abc123). The declares(op, "preview_id") guard keeps the composition from leaking into remove-custom-domain.
• preview_id pattern — back to ^[a-z0-9][a-z0-9-]{0,30}$, so pr-42-shaped ids from other producers still validate; TestPreviewIDIsPatterned covers pr-42, -x, ../x, ABC and empty.

Verified in this pass

• statServiceFileLocked preserves ComponentSourceRepo's semantics exactly — a missing element below services/ still maps to ErrSourceRepoNotRegistered, a symlink or non-plain entry is still an error, a missing services root is still an error, and the //nolint:gosec read is on the same validated path.
• No references remain to the removed TenantExistsMaxAge / MemberRolesMaxAge; MemberRoles now shares checkFreshLocked and its stale-checkout test points at CheckoutMaxAge.
• Submit call sites: ExecuteOperation, the now-exempt triggerRemoveCustomDomain, and the three platform-skill handlers (no team_name, so ServiceTarget is false).
• Exactly the eight registry operations declaring team_name plus a component parameter reach the gate (deploy-service, provision-database, retire-service, rollback-service, preview-deploy, preview-delete, add/remove-custom-domain); mctl-agents-* use service, not one of serviceComponentParams.
• The structural hyphen-prefix rule leaves admins, erpact, karabu, labs, mctl, ovk and every service in the 2026-10-11 snapshot valid, and near-misses (rooted, sharedteam, tenants, team-root) stay allowed.

P3 — nice to have

• The composed-name refusal reports "labs"/"oidc-zitadel" … choose a different service name for a caller who sent component_name=oidc, preview_id=zitadel — neither parameter has that value, and there is no service name to change (internal/operations/reserved_application.go:142).
• The gating table's comment claims an operation added later "fails this test until someone decides whether it is gated", but a new op inherits the gated default and passes; comparing reg.List() against an expected name set would enforce it (internal/api/service_existence_test.go:242).
• The reserved-pair (and pre-existing reserved-tenant-name) refusal is the only denial in ExecuteOperation with no audit entry — validation returns before auditParams is built, and Submit's backstop only logs at warn level (internal/operations/registry.go:275).

Note: go test, go vet and golangci-lint could not be run in this environment (commands not permitted), so the clean-build claim in the description is unverified here.
· branch fix/tenant-service-ownership-check

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.8-flash-medium · commit d227fa2.

No significant issues found.

[claude]

P3 — the composed-name refusal is the right call, but the message it produces describes a service the caller never named.

ReservedServiceTarget returns the composed name, and ValidateInput feeds it to reservedServiceApplicationMessage (internal/operations/registry.go:275), so a developer who sends component_name=oidc, preview_id=zitadel is told "labs"/"oidc-zitadel" is reserved for platform use; choose a different service name — neither parameter has that value, and there is no service name to choose differently for a delete. Same for the Submit backstop, which wraps labs-oidc-zitadel. A preview-specific variant (naming preview_id as the part that makes the pair reserved) would point at the field the caller actually has to change. The check itself and the declares(op, "preview_id") guard both look right, and the pair-exemption case (oidc-zitadel + abc123) is pinned.

[mashkovd]

Agreed. Deferred to a follow-up so the approved head stays unchanged: a preview-specific message that names preview_id as the field that makes the composed name reserved.

[claude]

P3 — asserting the exact status in both directions is a real improvement over the previous gated := w.Code == 404, but the comment above still claims more than the test delivers: a service operation added later inherits requiresExistingService == true, so it is expected to 404, and with a missing service it will 404 — the test passes without anyone deciding anything. It only fails if fill happens to lack one of the new operation's required parameters (then validation 400s instead).

Comparing reg.List()'s service operations against an expected set of names, rather than only seen < 8, would make an added operation fail here by construction and keep the comment accurate.

[mashkovd]

Agreed. Deferred to a follow-up: compare the registry's service operations against an explicit expected set, so an operation added later fails this test by construction.

[claude]

P3 — putting the refusal in ValidateInput gives the clean 400 and covers every caller, but it also means this denial leaves no audit record: ExecuteOperation returns at internal/api/handlers_write.go:230 before auditParams is built, while every other denial in that handler (role, tenant access, tenant existence, the new tenant-name collision, the new existence gate) logs Status: "denied".

A request that tries to address erpact/control, labs/openclaw-skills or labs-oidc-zitadel is the kind of event most worth having a record of, and Executor.Submit's backstop only logs at warn level — there is no audit entry on either path. The same applies to the pre-existing IsReservedTenantName refusal just above, so this may be better as a small follow-up: either audit validation failures generally, or log the two reserved-name refusals specifically before writing the 400.

[mashkovd]

Agreed, and it applies equally to the existing IsReservedTenantName refusal. Deferred to a follow-up: write a denied audit entry for reserved-name validation failures before ExecuteOperation returns the 400.

[claude]

No P1/P2 findings (3 P3). Good to merge.

Both P2s from the previous revision are resolved. preview-delete keeps the teardown exemption for its own pair but is now refused on the name it composes from component_name + preview_id, in both ValidateInput and the Submit backstop, with the hyphen boundary and the exempt-pair case pinned by TestPreviewDeleteComposedNameIsChecked. preview_id is back to a DNS-safe bounded pattern, so a generated id with hyphens is no longer rejected. The freshness aliases are gone and MemberRoles now shares checkFreshLocked, and the gating table asserts 404 vs 202 explicitly.

Checked and holding: ComponentSourceRepo's semantics are byte-for-byte preserved by the extracted statServiceFileLocked (missing path below services -> ErrSourceRepoNotRegistered, symlink/non-plain -> error, missing services root -> error); no remaining references to the removed TenantExistsMaxAge / MemberRolesMaxAge; the only Submit call sites are ExecuteOperation, the exempt domains teardown, and the platform-skill handlers (no team_name); exactly the eight registry operations that declare team_name plus a component parameter reach the gate, with the three exemptions documented and pinned; and the structural hyphen-prefix rule leaves all six existing tenants and all their current services valid.

P3s posted inline: the composed-name refusal message names a service the caller never sent; the gating table's comment claims more than it enforces; the reserved-pair/reserved-tenant 400 is the one denial in ExecuteOperation with no audit entry.

Note: go test, go vet and golangci-lint could not be run in this environment (commands not permitted), so the clean-build claim in the description is unverified here.