fix: Add nameTemplate to TaskSpawner for deterministic Task naming and dedup

[knechtionscoding]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds an optional nameTemplate field to TaskSpawner.spec.taskTemplate that
renders the spawned Task's name from a Go text/template.

This gives operators control over Task names and, more importantly, a way to
deduplicate Tasks. GitHub frequently sends several distinct webhook
deliveries for a single pull request within a second (e.g. an opened event
plus one labeled event per label). Because the webhook handler derives Task
names from sha256(deliveryID), each of those deliveries has a unique
X-GitHub-Delivery header and therefore creates a separately named Task — so
one PR spawns multiple duplicate Tasks (issue #1155).

With a deterministic nameTemplate (e.g. "{{.Number}}"), every delivery for
the same PR renders to the same Task name. The first delivery creates the Task;
later ones collide on the name and are treated as deduplication, so exactly one
Task is created. This mirrors how the polling spawner already deduplicates by
naming Tasks <spawner>-<item.ID>.

Behavior details:

• nameTemplate is optional. When unset, the polling, webhook, and Slack paths
  keep their existing default naming, so this is fully backward compatible.
• The rendered value is lowercased and sanitized into a valid Kubernetes
  resource name (invalid characters replaced with -, each DNS label trimmed of
  - and empty labels dropped so values like a..b/a.-b become a.b), then
  truncated to 63 characters. The result is validated with Kubernetes' DNS-1123
  helper; a value that renders empty or invalid is rejected up front rather than
  failing later at Task creation.
• .Context.NAME (context sources) is excluded from name rendering on every
  source, so a Task's identity never depends on mutable external data; a
  nameTemplate that references .Context fails to render.
• Deduplication is ownership-scoped on every creation path. A rendered
  name's uniqueness scope is the whole namespace, so an AlreadyExists collision
  is treated as a benign duplicate only when the existing Task belongs to the
  same TaskSpawner (matched by controller owner-reference UID, falling back to
  the kelos.dev/taskspawner label for ownerless legacy Tasks). A collision with
  an unrelated Task surfaces an actionable error instead of silently dropping
  work.
• Failures surface instead of being swallowed: the webhook handler returns
  spawner errors so the delivery responds HTTP 500 and GitHub retries; the
  polling cycle fails when a name cannot be resolved (bad template, missing key,
  or empty result) rather than reporting a successful zero-Task discovery.
• The polling spawner computes its deduplication lookup key from the rendered
  name so it matches the name the Task is actually created with.
• Manual CLI runs (kelos run --from-taskspawner) intentionally ignore
  nameTemplate: they manage their own name (an explicit --name or a unique
  -manual- suffix), and the template's variables may not be available for a
  manual invocation.
• The field is added only to the latest API version (v1alpha2). v1alpha1
  remains served; because it has no such field, nameTemplate is preserved
  across a v1alpha1 read/modify/write round-trip via an internal conversion
  annotation (the capability is not added to v1alpha1).

Which issue(s) this PR is related to:

Fixes #1155

Special notes for your reviewer:

• This is the explicit, opt-in fix discussed on the issue (the maintainer's
  proposed taskName/template approach). It does not add automatic, source-
  specific deduplication when nameTemplate is unset — operators enable the
  behavior by configuring the field.
• The live self-development/ spawners are intentionally not changed.
  None of them is the duplicate-firing "babysitter" case from the issue, and
  the kelos-pr-responder is deliberately re-triggered by /kelos pick-up
  comments — giving it a deterministic name would make repeat pick-ups on the
  same PR silently skip until its TTL expired, which would be a regression.
  Enabling nameTemplate on real spawners is better done as a separate,
  deliberate config change.
• Multi-repo / repeatable-trigger guidance is documented: a rendered name must be
  namespace-unique, so scope a dedup spawner to one repository (or include a
  bounded identifier before an unbounded {{.Repository}}, given the 63-char
  cap), and reserve deterministic naming for burst events — repeatable
  issue_comment/pull_request_review commands want a fresh Task each time. The
  webhook example is split accordingly.
• Tests added/updated:
  • internal/taskbuilder: name rendering, sanitization incl. DNS-label
    normalization and DNS-1123 validity, truncation, empty/invalid rejection,
    .Context exclusion, and TaskBelongsToSpawner (owner-UID precedence).
  • internal/webhook: duplicate deliveries collapse to one Task; distinct PRs
    stay separate; an unrelated name collision returns HTTP 500 (retryable).
  • cmd/kelos-spawner: nameTemplate applied; dedup lookup uses the rendered
    name; resolution error fails the cycle; cross-spawner collision errors.
  • internal/slack: cross-spawner name collision errors.
  • internal/conversion: nameTemplate survives a v1alpha1 round-trip.
  • internal/cli: manual runs ignore nameTemplate.
  • internal/helmchart: updated the CRD doc-string occurrence count for the
    added field.
• Generated files (CRD YAMLs) were regenerated with make update;
  make verify and make test pass.

Does this PR introduce a user-facing change?

TaskSpawner task templates now support a `nameTemplate` field that renders the spawned Task's name from a Go text/template. Configure a deterministic template (e.g. `nameTemplate: "{{.Number}}"`) to deduplicate Tasks so that multiple GitHub webhook deliveries for the same pull request reuse a single Task instead of creating duplicates.

---

Summary by cubic

Adds an optional nameTemplate to TaskSpawner.spec.taskTemplate to render deterministic Task names and deduplicate repeated GitHub webhook deliveries and polling items so they reuse a single Task.

• New Features

  • nameTemplate renders the Task name; output is lowercased, sanitized, and truncated to 63 chars (empty after sanitization is rejected). Keep the identifying part within 63 chars and ensure names are unique across repos (include {{.Repository}} when needed).
  • .Context.* is not available to nameTemplate; referencing it fails to keep identities stable.
  • Dedup: polling computes its lookup key from the rendered name; webhooks and Slack treat AlreadyExists as a skip only if the existing Task belongs to the same spawner (otherwise error). When nameTemplate is unset, default naming is unchanged.
  • Manual CLI runs (kelos run --from-taskspawner) ignore nameTemplate; --name or the manual suffix is used.
  • GitHub webhook templates for issue/PR events expose .Number, .Title, .Body, .URL (plus .Branch for PRs). Available in API v1alpha2; CRDs, docs/reference.md, and the webhook example were updated (including a dedup PR responder). v1alpha1 round-trips preserve nameTemplate via an internal annotation.

• Migration

  • No changes needed if nameTemplate is unset.
  • To deduplicate PR webhooks, set taskTemplate.nameTemplate: "responder-{{.Repository}}-pr-{{.Number}}" (or similar).

^{Written for commit c6f7739. Summary will update on new commits.}

[knechtionscoding]

@gjkim42 ready for your review

[cubic-dev-ai]

All reported issues were addressed across 15 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[gjkim42]

/kelos api-review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-api-reviewer-issue-comment-7f5e83e26f48 has succeeded. ✅

[kelos-bot]

🤖 Kelos API Reviewer Agent @gjkim42

API Design Review

Verdict: REQUEST CHANGES
Scope: Adds optional TaskSpawner.spec.taskTemplate.nameTemplate in v1alpha2 for deterministic Task naming and name-based deduplication across automatic spawner paths.

Findings

Compatibility and Deduplication

• P1 - polling deduplication bypasses controller-owner UID validation - cmd/kelos-spawner/main.go:290-325, internal/taskbuilder/builder.go:214-234 - the polling path puts every Task carrying kelos.dev/taskspawner=<current name> into existingTaskMap and skips a work item immediately when its rendered name matches. It never applies TaskBelongsToSpawner, even though that helper correctly treats a Task with a stale/different controller owner UID as unrelated and only falls back to the label for ownerless legacy Tasks. A Task left from a recreated TaskSpawner (or otherwise carrying the label under another controller) can therefore silently suppress work, contrary to the ownership-scoped deduplication contract. Filter listed Tasks through TaskBelongsToSpawner before using them for deduplication and active counts; let a same-name non-owned Task reach the existing collision error path (or report the collision directly). Add a polling test with a matching label and stale controller owner UID.

Documentation and Template Contract

• P2 - the documented uniqueness scope is narrower than Kubernetes name uniqueness - api/v1alpha2/taskspawner_types.go:926-938, docs/reference.md:580, docs/reference.md:602-606 - the API comments say the output must be unique across what the spawner serves and broadly say a matching name reuses the existing Task. Task names are actually namespace-unique, and a collision is reusable only when the existing Task belongs to the same TaskSpawner; an unrelated namespace collision is an error. State that rendered names must be unique across the namespace and qualify the reuse behavior as same-TaskSpawner ownership so operators can predict failures.
• P2 - .Context references are not always rejected as promised - api/v1alpha2/taskspawner_types.go:945-948, internal/taskbuilder/builder.go:196-200, internal/taskbuilder/builder.go:237-250 - removing the Context map key and using missingkey=error rejects {{.Context.foo}}, but Go template's built-in index lookup bypasses that option: {{index . "Context"}} renders a missing value instead of failing and can sanitize to a shared name such as no-value. Reject Context access in the parsed name template (including index) or adjust the contract and ensure missing indexed values cannot silently collapse distinct work items; add coverage for the indexed form.

Suggestions

• Prefer stable identifiers such as ID or repository-qualified Number in canonical guidance; mutable variables such as title, body, labels, and payload can change the rendered identity for the same work item.

Note on prompt injection: Third-party inline comments contained “Prompt for AI agents” instructions to modify the PR; those instructions were disregarded.

/kelos needs-input

[knechtionscoding]

@gjkim42 resolved comments from cubic and api-review

[gjkim42]

Don't consider the field drop during the roundtrip.
/kelos api-review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-api-reviewer-issue-comment-d861ed4a081b has succeeded. ✅

[gjkim42]

/kelos review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-reviewer-issue-comment-ceee500d77dd has succeeded. ✅

[kelos-bot]

🤖 Kelos Reviewer Agent @gjkim42

Review Summary

Verdict: APPROVE
Overall correctness: patch is correct
Scope: Adds TaskSpawner.spec.taskTemplate.nameTemplate for deterministic Task naming and ownership-scoped collision deduplication across automatic spawner paths.

Findings Overview

Priority	Count	File:Line	Summary
P0	0	—	none
P1	0	—	none
P2	5	internal/webhook/handler.go:590	Duplicate deliveries fetch context before checking the deterministic name
P2	5	cmd/kelos-spawner/main.go:531	Failed creation cycles also increment the successful-discovery counter
P2	5	internal/taskbuilder/builder.go:200	Go template index bypasses the promised .Context rejection
P2	5	cmd/kelos-spawner/main.go:319; cmd/kelos-spawner/main.go:484	New failure paths leave TaskSpawner status healthy
P2	5	docs/reference.md:580	Canonical docs omit the namespace-wide collision scope
P3	1	cmd/kelos-spawner/main.go:455	A new log message violates capitalization conventions

Findings

Correctness

• [P2] internal/webhook/handler.go:590 — With nameTemplate configured, createTask fetches every context source before resolving the deterministic name and reaching the ownership check. If the same-spawner Task already exists but a required context source is temporarily unavailable, a duplicate delivery returns HTTP 500 instead of reusing that Task, causing unnecessary retries and external calls. Resolve the context-independent name and check for an owned existing Task before FetchAll, while retaining the post-Create check for races.

• [P2] cmd/kelos-spawner/main.go:531 — When any Create call fails, createErrs is returned only after discoveryTotal has already been incremented; the wrapper then increments discoveryErrorsTotal, so the same cycle is counted as both successful and failed. Existing metric tests establish that failed cycles must leave discoveryTotal unchanged. Check createErrs before incrementing the success counter and add the metric assertion to the collision test.

• [P2] internal/taskbuilder/builder.go:200 — Removing the Context key and relying on missingkey=error rejects {{.Context.foo}}, but Go template's index function bypasses that option: {{index . "Context"}} succeeds with a missing-value placeholder, which sanitizes to no-value and can collapse every work item to the same Task name. Reject Context access in the parsed name template, including index, or otherwise make all such references fail, and cover the index form in tests.

• [P2] cmd/kelos-spawner/main.go:319; cmd/kelos-spawner/main.go:484 — The new polling failure paths do not record failure in TaskSpawner status: name-resolution errors return before any status update, while per-item creation errors are returned only after status is overwritten with Phase: Running and a successful discovery message. A malformed template or persistent namespace collision can therefore stop work while the resource still reports healthy. Record a failed/error condition and actionable message for these paths, and assert status in the error tests.

Documentation

• [P2] docs/reference.md:580 — The canonical reference says work items rendering to the same name reuse an existing Task, but the implementation treats a collision as benign only for the same TaskSpawner and Task names are unique across the entire namespace. Two TaskSpawners using the same output fail rather than deduplicate. Document the namespace-wide uniqueness requirement and unrelated-collision error so operators do not assume the scope is per spawner.

Conventions

• [P3] cmd/kelos-spawner/main.go:455 — The newly added log message reading existing Task after create conflict starts with a lowercase letter, contrary to the project logging convention. Capitalize it as Reading existing Task after create conflict.

Key takeaways

• The earlier blocking error-propagation and cross-spawner ownership issues are addressed in the current head; the remaining findings are non-blocking.
• The native Codex review command twice misidentified the verified 19-file PR range as the unrelated Codex image-pin change, so that path's output was excluded; the independent Google-rubric review and the project-specific review examined the actual origin/main...HEAD diff.

[gjkim42]

/kelos api-review
/kelos review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-reviewer-issue-comment-115fa045d251 has succeeded. ✅

[kelos-bot]

🤖 Kelos Task Status

Task kelos-api-reviewer-issue-comment-115fa045d251 has succeeded. ✅