From e6450ba39c4708570cf267c0f967c107eaf99141 Mon Sep 17 00:00:00 2001 From: Gunju Kim Date: Sat, 30 May 2026 18:06:20 +0000 Subject: [PATCH 1/5] self-development: broaden substring-assertion rule to forwarding tests The existing "vacuous substring assertions" convention was scoped only to printer/formatter tests, leaving stream/forwarding tests uncovered. In #1189 both the Kelos reviewer and cubic independently flagged a forwarder test that used strings.Contains per line, which passes even when lines are dropped, duplicated, or reordered. Extend the rule to require full byte-equality comparison for writer/pipe forwarding tests, mirroring the assertForwarded fix landed in that PR. Co-Authored-By: Claude Opus 4.8 --- AGENTS.md | 2 +- self-development/agentconfig.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9e6ac2a86..31d621c5b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -19,7 +19,7 @@ - **Do not use Gomega's global `Expect()` inside `Eventually` polling blocks.** Gomega does not retry on `Expect` failures — a transient API error short-circuits the poller and fails the test on the first blip instead of retrying. In e2e `WaitFor*` helpers, either inline the call and return a zero-value on error, or use the `Eventually(func(g Gomega) { ... })` form so failed assertions are caught and retried. - **CLI error messages must name the resource.** When a CLI command fails for a named resource (task, spawner, workspace, etc.), include the resource name in the returned error so operators piping or batching invocations get an actionable signal. `fmt.Errorf("task %s failed", name)`, not `errors.New("task failed")`. - **Test the happy path, not only the early-return guards.** When a handler has both early-return guards and a primary action (post a message, create a resource, emit a metric), unit tests must include at least one positive case verifying the primary action runs with the right arguments — not just the no-op branches. If the production code lacks a seam, add one (interface, function field, fake client) so the happy path is covered. -- **Avoid vacuous substring assertions in printer/formatter tests.** When asserting a `label: value` line is emitted, match against the full `"label: value"` string (or a regex), not the bare value — bare values often collide with the resource's `Name` or other surrounding context in the fixture and pass even when the line is missing. +- **Avoid vacuous substring assertions in printer/formatter and stream-forwarding tests.** When asserting a `label: value` line is emitted, match against the full `"label: value"` string (or a regex), not the bare value — bare values often collide with the resource's `Name` or other surrounding context in the fixture and pass even when the line is missing. When asserting that a writer or pipe forwards its input, compare the full normalized output for byte equality, not `strings.Contains` per line — a per-line substring check still passes when lines are dropped, duplicated, or reordered. - **Keep CRD enum docstrings consistent with the `+kubebuilder:validation:Enum` marker.** If the godoc says "empty matches both", either include `""` in the enum list so `field: ""` is accepted, or rephrase to "Omit to match both" so no one writes the explicit empty form. A docstring that invites a value the API server then rejects is a worse contract than either alternative. - **Qualify cross-CRD field references with the owning kind in docs.** In a CRD reference section, write `Task.spec.podOverrides.env` rather than bare `podOverrides.env` when describing a field that lives on a sibling CRD. A reader of one CRD's reference page should be able to locate the cited field without already knowing the layout of the others. diff --git a/self-development/agentconfig.yaml b/self-development/agentconfig.yaml index 6f93c08c2..6aab66b6d 100644 --- a/self-development/agentconfig.yaml +++ b/self-development/agentconfig.yaml @@ -40,7 +40,7 @@ spec: - Do not use Gomega's global `Expect()` inside `Eventually` polling blocks: Gomega does not retry on `Expect` failures, so a transient API error short-circuits the poller. In e2e `WaitFor*` helpers, either inline the call and return a zero-value on error, or use the `Eventually(func(g Gomega) { ... })` form so failed assertions are caught and retried. - CLI error messages must name the resource: when a CLI command fails for a named resource (task, spawner, workspace, etc.), include the name in the returned error so operators piping or batching invocations get an actionable signal — `fmt.Errorf("task %s failed", name)`, not `errors.New("task failed")`. - Test the happy path, not only early-return guards: when a handler has both guard branches and a primary action (post a message, create a resource, emit a metric), include at least one positive test verifying the primary action runs with the right arguments. If the production code lacks a seam, add one (interface, function field, fake client) so the happy path is testable. - - Avoid vacuous substring assertions in printer/formatter tests: when asserting a `label: value` line is emitted, match the full `"label: value"` string (or a regex), not the bare value — bare values frequently collide with the fixture's `Name` or surrounding context and pass even when the line is missing. + - Avoid vacuous substring assertions in printer/formatter and stream-forwarding tests: when asserting a `label: value` line is emitted, match the full `"label: value"` string (or a regex), not the bare value — bare values frequently collide with the fixture's `Name` or surrounding context and pass even when the line is missing. When asserting that a writer or pipe forwards its input, compare the full normalized output for byte equality, not `strings.Contains` per line — a substring check still passes when lines are dropped, duplicated, or reordered. - Keep CRD enum docstrings consistent with the `+kubebuilder:validation:Enum` marker: if the godoc says "empty matches both", either include `""` in the enum list so `field: ""` is accepted, or rephrase to "Omit to match both" so no one writes the explicit empty form. A docstring that invites a value the API server then rejects is a worse contract than either alternative. - Qualify cross-CRD field references with the owning kind in docs: in a CRD reference section, write `Task.spec.podOverrides.env` rather than bare `podOverrides.env` when describing a field that lives on a sibling CRD. A reader of one CRD's reference page should be able to locate the cited field without already knowing the layout of the others. - PRs that only modify files under `self-development/` are internal agent improvements: use `/kind cleanup` and write "NONE" in the `release-note` block, even when the change fixes a bug or adds a feature in agent behavior. Classify by file location, not by problem nature. From 15c74e09f184fcbe412748ac6fdccd5cc386bf43 Mon Sep 17 00:00:00 2001 From: Gunju Kim Date: Sun, 31 May 2026 18:03:29 +0000 Subject: [PATCH 2/5] self-development: require author/subject filters on autonomous-run triggers PR #1241 (Add kanon-development TaskSpawners) shipped event-driven triggers without author scoping. The Kelos/cubic review flagged this twice and both fixes were applied in that PR: - P1 (confidence 9) on kanon-pr-responder.yaml: add an `author` filter to the `pull_request_review` trigger; without it, unapproved review comments can trigger autonomous task runs. - P2 on kanon-reviewer.yaml: the `issue_comment` trigger was not scoped to pull requests, so `/kelos review` on a regular issue could start the reviewer and make the PR-only command fail. Add a TaskSpawner convention so future spawners ship the `author` allowlist and `commentOn: PullRequest` scoping up front instead of being corrected in review. Existing conventions only said issue_comment fires for both subjects; they did not state the baseline that every autonomous-run trigger must be restricted to a trusted author and the intended subject. Co-Authored-By: Claude Opus 4.8 --- self-development/agentconfig.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/self-development/agentconfig.yaml b/self-development/agentconfig.yaml index 6aab66b6d..0a54640f2 100644 --- a/self-development/agentconfig.yaml +++ b/self-development/agentconfig.yaml @@ -48,4 +48,5 @@ spec: - Prefer webhook-based triggers (`githubWebhook`) over poll-based (`githubPullRequests`) for real-time event-driven tasks - The `{{.Branch}}` template variable is empty for issue-only events; use `{{with index . "Branch"}}{{.}}{{else}}main{{end}}` when it may be empty - The `issue_comment` webhook event fires for both issues and pull requests; design prompts to detect and handle both contexts + - Restrict every trigger that can launch an autonomous run to a trusted author and the intended subject: give `issue_comment` and `pull_request_review` filters an `author` allowlist (e.g. `author: gjkim42`) so an unapproved comment or review cannot start a task and spend tokens, and add `commentOn: PullRequest` to filters whose command is PR-only (e.g. `/kelos review`) so the same command on a plain issue does not start a PR-only task. An unfiltered event trigger is reachable by anyone who can comment - Do not include manual PR branch checkout instructions in prompts — Kelos already checks out the PR branch automatically From 272004576962028ff84365b74a34d8054cd5a2c8 Mon Sep 17 00:00:00 2001 From: Gunju Kim Date: Mon, 1 Jun 2026 18:04:08 +0000 Subject: [PATCH 3/5] self-development: distinguish "not found" from "lookup failed" in handlers PR #1238 (Add WebhookGateway CRD) shipped request handlers that conflated API lookup errors with empty results. The review flagged this repeatedly and the maintainer fixed every instance: - A TaskSpawner List error was treated as an empty match set, so a transient API failure silently dropped the webhook with a 200; the fix returns the error so the handler responds 5xx and the sender retries. - The gateway Get returned 404 unconditionally; the fix returns 404 only on IsNotFound and 5xx on RBAC/transient errors. Add a coding convention so handlers and reconcilers propagate lookup errors as 5xx (sender redelivers) instead of answering 200 with an empty result. This is distinct from the existing "fail fast on invalid configuration" rule, which covers startup config/secrets rather than request-time API reads. Co-Authored-By: Claude Opus 4.8 --- AGENTS.md | 1 + self-development/agentconfig.yaml | 1 + 2 files changed, 2 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 31d621c5b..7b10c55e9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,6 +10,7 @@ - **Kubernetes resource comparison.** Use semantic `.Equal()` or `.Cmp()` methods for `resource.Quantity` comparisons, not `reflect.DeepEqual` — structurally different Quantity values can be semantically identical (e.g., `1000m` vs `1` CPU). - **Never use `os.Getenv()` for secrets as Go `flag` defaults.** Go's `flag` package prints default values in usage/help output, which leaks secret values. Instead, use an empty default and read the env var after `flag.Parse()`. - **Fail fast on invalid configuration.** Do not silently fall back to degraded behavior (e.g., unauthenticated requests) when configuration or credentials are invalid or missing. Return an error or exit immediately instead of returning nil or empty values that mask the failure. +- **Distinguish "not found" from "lookup failed" in request handlers.** In a webhook/HTTP handler or reconciler, a `List`/`Get` that errors is not the same as an empty result. Do not treat a failed lookup as "no match" and answer `200` — that silently drops the event with no redelivery. Propagate the error so the handler returns `5xx` and the sender retries; return `404` only on `IsNotFound`, and `5xx` on RBAC or transient errors. - **Keep API surfaces minimal.** When adding new API fields, types, or CRD changes, include only what is immediately needed. Do not add speculative fields — API is hard to change once shipped. Start with the minimum viable API and extend in follow-up PRs. - **API changes must preserve backward compatibility for existing manifests.** Existing in-cluster resources must continue to apply after a CRD update. Do not change a field's kind (scalar ↔ array, string ↔ object) on an existing field; do not add `MinLength`, `Required`, or other tightening validation to a field that previously accepted absence/empty values; when replacing a field, mark the old one `+deprecated` and keep it functional rather than removing it. When the schema must change, sweep `examples/`, `self-development/`, and any in-tree YAMLs that use the old form and update them in the same PR. - **Maintain API changes only in the latest Kelos API version.** Add new CRD fields, validation, enum values, constants, and user-facing API behavior only to the latest served/storage API version. Older served versions are compatibility surfaces: keep them served, convertible, and backward-compatible, but do not add new capabilities there unless required to preserve existing manifests or implement conversion/migration compatibility. diff --git a/self-development/agentconfig.yaml b/self-development/agentconfig.yaml index 0a54640f2..a71884624 100644 --- a/self-development/agentconfig.yaml +++ b/self-development/agentconfig.yaml @@ -34,6 +34,7 @@ spec: - Kubernetes resource comparison: use semantic `.Equal()` or `.Cmp()` methods for `resource.Quantity` comparisons, not `reflect.DeepEqual` - Never use `os.Getenv()` for secrets as Go `flag` defaults: Go's `flag` package prints default values in usage/help output, which leaks secret values; use an empty default and read the env var after `flag.Parse()` - Fail fast on invalid configuration: do not silently fall back to degraded behavior (e.g., unauthenticated requests) when configuration or credentials are invalid or missing; return an error or exit immediately + - Distinguish "not found" from "lookup failed" in request handlers: in a webhook/HTTP handler or reconciler, a `List`/`Get` that errors is not the same as an empty result; do not treat a failed lookup as "no match" and answer `200` (which silently drops the event with no redelivery), but propagate the error so the handler returns `5xx` and the sender retries — return `404` only on `IsNotFound`, and `5xx` on RBAC or transient errors - Keep API surfaces minimal: when adding new API fields, types, or CRD changes, include only what is immediately needed; do not add speculative fields — API is hard to change once shipped - API changes must preserve backward compatibility for existing manifests: existing in-cluster resources must continue to apply after a CRD update. Do not change a field's kind (scalar ↔ array) on an existing field; do not add `MinLength`/`Required` to a field that previously accepted absence/empty values; replace by deprecating (mark `+deprecated`, keep functional) rather than removing. When the schema must change, sweep `examples/` and `self-development/` for YAMLs using the old form and update them in the same PR. - Docs must match implementation, not aspiration: describe only what the code actually does; do not document unimplemented behavior, overstate guarantees, or describe security checks (e.g., HMAC validation) that aren't enforced. Verify the code enforces a contract before documenting it. From 8b5a1ce4c09de17608ebb827e083623273884b8c Mon Sep 17 00:00:00 2001 From: Gunju Kim Date: Wed, 15 Jul 2026 18:05:54 +0000 Subject: [PATCH 4/5] self-development: honor reduced-motion preferences --- AGENTS.md | 1 + self-development/agentconfig.yaml | 1 + 2 files changed, 2 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 7b10c55e9..5371db017 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,6 +17,7 @@ - **Import versioned Kelos APIs with aliases.** Prefer aliasing the current versioned API import as `kelos` when a file uses only one API version. Use explicit aliases like `kelosv1alpha1` only when a file genuinely needs multiple versions. - **Docs must match implementation, not aspiration.** When writing or updating docs, READMEs, or comments, describe only what the code actually does. Do not document unimplemented behavior, overstate guarantees, or describe security checks (e.g., HMAC validation) that aren't enforced. Before describing a contract ("X is filtered", "Y is validated"), verify the code enforces it — partial enforcement should be documented as partial. - **Keep `docs/reference.md` user-facing.** Document API fields, configuration, validation, defaults, and observable operational behavior. Do not describe controller or runtime implementation details unless users need them to configure, operate, or troubleshoot Kelos. Prefer outcomes over mechanisms, and remove nonessential internal detail instead of moving it to a separate architecture document. +- **Honor reduced-motion preferences in web UI.** Do not add continuous or non-essential animation without a `prefers-reduced-motion` fallback. Keep the static state distinguishable when animation is disabled; prefer gating animation under `@media (prefers-reduced-motion: no-preference)`. - **Do not use Gomega's global `Expect()` inside `Eventually` polling blocks.** Gomega does not retry on `Expect` failures — a transient API error short-circuits the poller and fails the test on the first blip instead of retrying. In e2e `WaitFor*` helpers, either inline the call and return a zero-value on error, or use the `Eventually(func(g Gomega) { ... })` form so failed assertions are caught and retried. - **CLI error messages must name the resource.** When a CLI command fails for a named resource (task, spawner, workspace, etc.), include the resource name in the returned error so operators piping or batching invocations get an actionable signal. `fmt.Errorf("task %s failed", name)`, not `errors.New("task failed")`. - **Test the happy path, not only the early-return guards.** When a handler has both early-return guards and a primary action (post a message, create a resource, emit a metric), unit tests must include at least one positive case verifying the primary action runs with the right arguments — not just the no-op branches. If the production code lacks a seam, add one (interface, function field, fake client) so the happy path is covered. diff --git a/self-development/agentconfig.yaml b/self-development/agentconfig.yaml index a71884624..eacc75437 100644 --- a/self-development/agentconfig.yaml +++ b/self-development/agentconfig.yaml @@ -38,6 +38,7 @@ spec: - Keep API surfaces minimal: when adding new API fields, types, or CRD changes, include only what is immediately needed; do not add speculative fields — API is hard to change once shipped - API changes must preserve backward compatibility for existing manifests: existing in-cluster resources must continue to apply after a CRD update. Do not change a field's kind (scalar ↔ array) on an existing field; do not add `MinLength`/`Required` to a field that previously accepted absence/empty values; replace by deprecating (mark `+deprecated`, keep functional) rather than removing. When the schema must change, sweep `examples/` and `self-development/` for YAMLs using the old form and update them in the same PR. - Docs must match implementation, not aspiration: describe only what the code actually does; do not document unimplemented behavior, overstate guarantees, or describe security checks (e.g., HMAC validation) that aren't enforced. Verify the code enforces a contract before documenting it. + - Honor reduced-motion preferences in web UI: do not add continuous or non-essential animation without a `prefers-reduced-motion` fallback. Keep the static state distinguishable when animation is disabled; prefer gating animation under `@media (prefers-reduced-motion: no-preference)`. - Do not use Gomega's global `Expect()` inside `Eventually` polling blocks: Gomega does not retry on `Expect` failures, so a transient API error short-circuits the poller. In e2e `WaitFor*` helpers, either inline the call and return a zero-value on error, or use the `Eventually(func(g Gomega) { ... })` form so failed assertions are caught and retried. - CLI error messages must name the resource: when a CLI command fails for a named resource (task, spawner, workspace, etc.), include the name in the returned error so operators piping or batching invocations get an actionable signal — `fmt.Errorf("task %s failed", name)`, not `errors.New("task failed")`. - Test the happy path, not only early-return guards: when a handler has both guard branches and a primary action (post a message, create a resource, emit a metric), include at least one positive test verifying the primary action runs with the right arguments. If the production code lacks a seam, add one (interface, function field, fake client) so the happy path is testable. From 446a4b302611744a1a48e1d34f5bee3d34631581 Mon Sep 17 00:00:00 2001 From: Gunju Kim Date: Mon, 27 Jul 2026 18:10:25 +0000 Subject: [PATCH 5/5] self-development: add security, state, and behavior-test conventions --- AGENTS.md | 3 +++ self-development/agentconfig.yaml | 3 +++ 2 files changed, 6 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 5371db017..bacd7a391 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,7 +10,9 @@ - **Kubernetes resource comparison.** Use semantic `.Equal()` or `.Cmp()` methods for `resource.Quantity` comparisons, not `reflect.DeepEqual` — structurally different Quantity values can be semantically identical (e.g., `1000m` vs `1` CPU). - **Never use `os.Getenv()` for secrets as Go `flag` defaults.** Go's `flag` package prints default values in usage/help output, which leaks secret values. Instead, use an empty default and read the env var after `flag.Parse()`. - **Fail fast on invalid configuration.** Do not silently fall back to degraded behavior (e.g., unauthenticated requests) when configuration or credentials are invalid or missing. Return an error or exit immediately instead of returning nil or empty values that mask the failure. +- **Protect credentials in configuration and transport.** Values that may contain tokens, authorization headers, or other credentials must come from Kubernetes Secret references, not plaintext Helm values or rendered manifests. Require HTTPS for configurable endpoints that receive credentials unless the API has an explicit insecure opt-in. - **Distinguish "not found" from "lookup failed" in request handlers.** In a webhook/HTTP handler or reconciler, a `List`/`Get` that errors is not the same as an empty result. Do not treat a failed lookup as "no match" and answer `200` — that silently drops the event with no redelivery. Propagate the error so the handler returns `5xx` and the sender retries; return `404` only on `IsNotFound`, and `5xx` on RBAC or transient errors. +- **Protect asynchronous state transitions from stale work.** Before an asynchronous completion mutates shared state, verify it still belongs to the current resource, Session, dialog, or request generation, or prevent that state from being replaced while the operation is pending. Before a destructive lifecycle transition based on observed status, coordinate or revalidate so newly accepted work cannot race with the transition. - **Keep API surfaces minimal.** When adding new API fields, types, or CRD changes, include only what is immediately needed. Do not add speculative fields — API is hard to change once shipped. Start with the minimum viable API and extend in follow-up PRs. - **API changes must preserve backward compatibility for existing manifests.** Existing in-cluster resources must continue to apply after a CRD update. Do not change a field's kind (scalar ↔ array, string ↔ object) on an existing field; do not add `MinLength`, `Required`, or other tightening validation to a field that previously accepted absence/empty values; when replacing a field, mark the old one `+deprecated` and keep it functional rather than removing it. When the schema must change, sweep `examples/`, `self-development/`, and any in-tree YAMLs that use the old form and update them in the same PR. - **Maintain API changes only in the latest Kelos API version.** Add new CRD fields, validation, enum values, constants, and user-facing API behavior only to the latest served/storage API version. Older served versions are compatibility surfaces: keep them served, convertible, and backward-compatible, but do not add new capabilities there unless required to preserve existing manifests or implement conversion/migration compatibility. @@ -21,6 +23,7 @@ - **Do not use Gomega's global `Expect()` inside `Eventually` polling blocks.** Gomega does not retry on `Expect` failures — a transient API error short-circuits the poller and fails the test on the first blip instead of retrying. In e2e `WaitFor*` helpers, either inline the call and return a zero-value on error, or use the `Eventually(func(g Gomega) { ... })` form so failed assertions are caught and retried. - **CLI error messages must name the resource.** When a CLI command fails for a named resource (task, spawner, workspace, etc.), include the resource name in the returned error so operators piping or batching invocations get an actionable signal. `fmt.Errorf("task %s failed", name)`, not `errors.New("task failed")`. - **Test the happy path, not only the early-return guards.** When a handler has both early-return guards and a primary action (post a message, create a resource, emit a metric), unit tests must include at least one positive case verifying the primary action runs with the right arguments — not just the no-op branches. If the production code lacks a seam, add one (interface, function field, fake client) so the happy path is covered. +- **Test behavior through the layer that owns it.** Do not rely only on helper tests or source-fragment assertions when behavior depends on controller scheduling, rendered configuration, or stateful UI interactions. Exercise the behavior through `Reconcile`, rendered output, or executable UI tests, and cover non-default configuration branches that change the result. - **Avoid vacuous substring assertions in printer/formatter and stream-forwarding tests.** When asserting a `label: value` line is emitted, match against the full `"label: value"` string (or a regex), not the bare value — bare values often collide with the resource's `Name` or other surrounding context in the fixture and pass even when the line is missing. When asserting that a writer or pipe forwards its input, compare the full normalized output for byte equality, not `strings.Contains` per line — a per-line substring check still passes when lines are dropped, duplicated, or reordered. - **Keep CRD enum docstrings consistent with the `+kubebuilder:validation:Enum` marker.** If the godoc says "empty matches both", either include `""` in the enum list so `field: ""` is accepted, or rephrase to "Omit to match both" so no one writes the explicit empty form. A docstring that invites a value the API server then rejects is a worse contract than either alternative. - **Qualify cross-CRD field references with the owning kind in docs.** In a CRD reference section, write `Task.spec.podOverrides.env` rather than bare `podOverrides.env` when describing a field that lives on a sibling CRD. A reader of one CRD's reference page should be able to locate the cited field without already knowing the layout of the others. diff --git a/self-development/agentconfig.yaml b/self-development/agentconfig.yaml index eacc75437..3b08b5374 100644 --- a/self-development/agentconfig.yaml +++ b/self-development/agentconfig.yaml @@ -34,7 +34,9 @@ spec: - Kubernetes resource comparison: use semantic `.Equal()` or `.Cmp()` methods for `resource.Quantity` comparisons, not `reflect.DeepEqual` - Never use `os.Getenv()` for secrets as Go `flag` defaults: Go's `flag` package prints default values in usage/help output, which leaks secret values; use an empty default and read the env var after `flag.Parse()` - Fail fast on invalid configuration: do not silently fall back to degraded behavior (e.g., unauthenticated requests) when configuration or credentials are invalid or missing; return an error or exit immediately + - Protect credentials in configuration and transport: values that may contain tokens, authorization headers, or other credentials must come from Kubernetes Secret references, not plaintext Helm values or rendered manifests. Require HTTPS for configurable endpoints that receive credentials unless the API has an explicit insecure opt-in. - Distinguish "not found" from "lookup failed" in request handlers: in a webhook/HTTP handler or reconciler, a `List`/`Get` that errors is not the same as an empty result; do not treat a failed lookup as "no match" and answer `200` (which silently drops the event with no redelivery), but propagate the error so the handler returns `5xx` and the sender retries — return `404` only on `IsNotFound`, and `5xx` on RBAC or transient errors + - Protect asynchronous state transitions from stale work: before an asynchronous completion mutates shared state, verify it still belongs to the current resource, Session, dialog, or request generation, or prevent that state from being replaced while the operation is pending. Before a destructive lifecycle transition based on observed status, coordinate or revalidate so newly accepted work cannot race with the transition. - Keep API surfaces minimal: when adding new API fields, types, or CRD changes, include only what is immediately needed; do not add speculative fields — API is hard to change once shipped - API changes must preserve backward compatibility for existing manifests: existing in-cluster resources must continue to apply after a CRD update. Do not change a field's kind (scalar ↔ array) on an existing field; do not add `MinLength`/`Required` to a field that previously accepted absence/empty values; replace by deprecating (mark `+deprecated`, keep functional) rather than removing. When the schema must change, sweep `examples/` and `self-development/` for YAMLs using the old form and update them in the same PR. - Docs must match implementation, not aspiration: describe only what the code actually does; do not document unimplemented behavior, overstate guarantees, or describe security checks (e.g., HMAC validation) that aren't enforced. Verify the code enforces a contract before documenting it. @@ -42,6 +44,7 @@ spec: - Do not use Gomega's global `Expect()` inside `Eventually` polling blocks: Gomega does not retry on `Expect` failures, so a transient API error short-circuits the poller. In e2e `WaitFor*` helpers, either inline the call and return a zero-value on error, or use the `Eventually(func(g Gomega) { ... })` form so failed assertions are caught and retried. - CLI error messages must name the resource: when a CLI command fails for a named resource (task, spawner, workspace, etc.), include the name in the returned error so operators piping or batching invocations get an actionable signal — `fmt.Errorf("task %s failed", name)`, not `errors.New("task failed")`. - Test the happy path, not only early-return guards: when a handler has both guard branches and a primary action (post a message, create a resource, emit a metric), include at least one positive test verifying the primary action runs with the right arguments. If the production code lacks a seam, add one (interface, function field, fake client) so the happy path is testable. + - Test behavior through the layer that owns it: do not rely only on helper tests or source-fragment assertions when behavior depends on controller scheduling, rendered configuration, or stateful UI interactions. Exercise the behavior through `Reconcile`, rendered output, or executable UI tests, and cover non-default configuration branches that change the result. - Avoid vacuous substring assertions in printer/formatter and stream-forwarding tests: when asserting a `label: value` line is emitted, match the full `"label: value"` string (or a regex), not the bare value — bare values frequently collide with the fixture's `Name` or surrounding context and pass even when the line is missing. When asserting that a writer or pipe forwards its input, compare the full normalized output for byte equality, not `strings.Contains` per line — a substring check still passes when lines are dropped, duplicated, or reordered. - Keep CRD enum docstrings consistent with the `+kubebuilder:validation:Enum` marker: if the godoc says "empty matches both", either include `""` in the enum list so `field: ""` is accepted, or rephrase to "Omit to match both" so no one writes the explicit empty form. A docstring that invites a value the API server then rejects is a worse contract than either alternative. - Qualify cross-CRD field references with the owning kind in docs: in a CRD reference section, write `Task.spec.podOverrides.env` rather than bare `podOverrides.env` when describing a field that lives on a sibling CRD. A reader of one CRD's reference page should be able to locate the cited field without already knowing the layout of the others.