diff --git a/AGENTS.md b/AGENTS.md index a830e2de..c2294d02 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,13 +10,14 @@ - **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. - **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. - **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 c9c4566b..d04bf6cb 100644 --- a/self-development/agentconfig.yaml +++ b/self-development/agentconfig.yaml @@ -34,13 +34,14 @@ 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. - 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. @@ -48,4 +49,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 diff --git a/self-development/kelos-workers.yaml b/self-development/kelos-workers.yaml index ec743f5f..9bd02638 100644 --- a/self-development/kelos-workers.yaml +++ b/self-development/kelos-workers.yaml @@ -33,13 +33,14 @@ 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. - 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. @@ -47,6 +48,7 @@ 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 --- apiVersion: kelos.dev/v1alpha1