self-development: distill recurring conventions from PR review feedback

[kelos-bot]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Distills six recurring conventions from recent PR review feedback and propagates them across AGENTS.md, the shared and worker AgentConfigs, and the reviewer prompts. All changes are backed by specific PR review findings — no speculative rules.

1. Never use os.Getenv() for secrets as Go flag defaults

• Evidence: PR Remove token-refresher sidecar, generate tokens in-process #971 — P1 security findings in cmd/kelos-webhook-server/main.go and cmd/ghproxy/main.go where os.Getenv("GITHUB_TOKEN") was used as flag.StringVar default, leaking the secret in --help output.
• Applied to: AGENTS.md, agentconfig.yaml, kelos-workers.yaml, kelos-reviewer.yaml (new security checklist item).

2. Fail fast on invalid configuration

• Evidence: PR Remove token-refresher sidecar, generate tokens in-process #971 — multiple P1/P2 findings where invalid GitHub App credentials, missing token resolvers, or failed token refreshes caused silent degradation (unauthenticated requests, skipped reporting) instead of erroring out.
• Applied to: AGENTS.md, agentconfig.yaml, kelos-workers.yaml, kelos-reviewer.yaml (new correctness checklist item).

3. Keep API surfaces minimal

• Evidence: PR feat(api): Add generic webhook source for arbitrary event-driven task spawning #902 — repeated maintainer feedback on the generic webhook API: "API is hard to fix, so I'd like to find the best way to design it" / "Can we defer these fields for now?" / "Let's make the minimal API for this PR."
• Applied to: AGENTS.md, agentconfig.yaml, kelos-workers.yaml, kelos-api-reviewer.yaml (new compatibility checklist item).

4. API changes must preserve backward compatibility for existing manifests (new)

• Evidence: PR feat: add bodyPattern and excludeBodyPatterns regex filters for GitHub webhooks #1058 — three distinct findings on the same PR:
  • Greptile P1: "Breaking schema change: BodyContains scalar → array" — existing in-cluster resources with bodyContains: "/kelos" (scalar) would fail Kubernetes structural-schema validation after the CRD update; the repo itself had at least 9 YAMLs in self-development/ and examples/ still using the old scalar form.
  • Maintainer (gjkim42) asked twice: "Is this backward-compatible? Does setting +kubebuilder:validation:MinLength=1 allow empty BodyPattern?" and "I just want to make sure that the existing YAMLs that don't have bodyPattern (because it's a newly introduced field) can be applied. can we just remove minLength here?"
  • Agreed resolution thread: add new fields and mark the old one +deprecated rather than remove it ("Why don't we add BodyPattern and ExcludeBodyPatterns and mark BodyContains deprecated here so that I can remove BodyContains later?").
• This is distinct from "Keep API surfaces minimal" — minimal API is about scope of new additions; backward compatibility is about preserving existing manifests across CRD updates.
• Applied to: AGENTS.md, agentconfig.yaml, kelos-workers.yaml, kelos-api-reviewer.yaml (two new bullets under "API compatibility and evolution": one on validation tightening / kind changes, one on sweeping stale YAMLs).

5. Docs must match implementation, not aspiration

• Evidence:
  • PR docs: Document GenericWebhook TaskSpawner source #1035 (kelos-bot[bot] P1): docs claimed HMAC signature validation per source, but the GenericSource branch in internal/webhook/handler.go never reads <SOURCE>_WEBHOOK_SECRET, inspects X-Hub-Signature-256, or calls validateHMACSignature — giving users a false sense of security.
  • PR API: Add Workspace.spec.setupCommand for pre-agent setup #1056 (kelos-bot[bot] P2): doc note claimed PodOverrides.Env entries reusing reserved names are dropped so the built-in always wins, but the actual filter only drops collisions against names already populated on mainContainer.Env.
• Applied to: AGENTS.md, agentconfig.yaml, kelos-workers.yaml, kelos-reviewer.yaml (new "Documentation accuracy" checklist subsection).

6. TaskSpawner conventions

• Evidence: PR self-development: add kelos-api-reviewer TaskSpawner #965 — three distinct corrections from maintainer (gjkim42): use webhook-based triggers over poll-based; handle empty {{.Branch}} for issue events; design issue_comment prompts for both issue and PR contexts. PR Add API contract validation example (example 12) #974 — P2: "Remove the manual PR branch checkout instruction; Kelos already handles PR branch checkout automatically."
• Applied to: agentconfig.yaml, kelos-workers.yaml. README template-variable table corrected from "Usually PR head branch" to "PR head branch; empty for issue events".

Also fixes the kelos-reviewer TaskSpawner branch field to use the safe {{with index . "Branch"}}{{.}}{{else}}main{{end}} fallback form instead of bare {{.Branch}}. Flagged P2 by Greptile in this PR's prior review (#786): the spawner listens on issue_comment, so a /kelos review triggered on a plain issue would land on whatever default state Kelos falls back to rather than main.

Which issue(s) this PR is related to:

N/A

Special notes for your reviewer:

This PR was rebased onto current main. All changes are documentation/configuration; no runtime code changes.

Does this PR introduce a user-facing change?

NONE

[cubic-dev-ai]

No issues found across 4 files

[gjkim42]

/kelos pick-up
rebase on origin/main

[kelos-bot]

🤖 Kelos Task Status

Task kelos-pr-responder-786 has succeeded. ✅

[kelos-bot]

🤖 Kelos Agent @gjkim42

/kelos needs-input

Rebased on origin/main (was already up to date). Added kind/cleanup label to fix check-pr-labels CI check. Updated PR body formatting.

The only failing CI check is comment-label — this is a repo-level permissions issue (kelos-bot[bot] lacks write access), not related to this PR. All core checks (build, test, verify, e2e, integration) passed on the current commit.

Ready for re-review.

[greptile-apps]

Greptile Summary

This PR propagates five conventions distilled from recent PR review feedback — secrets in flag defaults, fail-fast configuration, minimal API surfaces, backward-compatible manifests, and documentation accuracy — across AGENTS.md, the shared and worker AgentConfigs, and the reviewer prompts. It also corrects the kelos-reviewer TaskSpawner branch template to the safe {{with index . "Branch"}}{{.}}{{else}}main{{end}} fallback form, resolving the prior P2 finding.

Confidence Score: 5/5

Safe to merge — documentation/configuration only, no runtime code changes, with one minor P2 style inconsistency.

All findings are P2 style suggestions; no P0/P1 issues found. The changes are purely documentation and configuration with a well-scoped, evidence-backed set of convention additions.

self-development/agentconfig.yaml and self-development/kelos-workers.yaml have a minor mismatch between the documented branch-fallback example and the canonical form used in the spawner YAMLs.

Important Files Changed

Filename	Overview
AGENTS.md	Adds five new coding-convention bullets (secrets in flags, fail-fast config, minimal API, backward-compat manifests, docs accuracy); straightforward documentation update.
self-development/README.md	Single-line fix: corrects the {{.Branch}} description from "Usually PR head branch or push branch" to "PR head branch; empty for issue events".
self-development/agentconfig.yaml	Propagates all five conventions to the shared dev-agent config; documented branch-fallback form ({{if .Branch}}) is inconsistent with the canonical form ({{with index . "Branch"}}) used in the actual spawner YAMLs.
self-development/kelos-workers.yaml	Adds all five conventions to the worker agent config; same branch-fallback form inconsistency as agentconfig.yaml; also adds previously-missing Kubernetes resource-comparison rule.
self-development/kelos-reviewer.yaml	Adds correctness/security/doc-accuracy checklist items to the reviewer prompt; branch field updated to the safe {{with index . "Branch"}}{{.}}{{else}}main{{end}} fallback form, resolving the prior P2 finding.
self-development/kelos-api-reviewer.yaml	Adds three new API-design checklist bullets (minimal surface, backward-compat, stale manifests); clean addition with no issues.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[PR Review Feedback\nPRs #965 #971 #902 #974 #1035 #1056] --> B[Five Conventions Distilled]

    B --> C1[1. No os.Getenv secrets\nas flag defaults]
    B --> C2[2. Fail fast on\ninvalid config]
    B --> C3[3. Minimal API surfaces]
    B --> C4[4. Docs match\nimplementation]
    B --> C5[5. TaskSpawner\nconventions]

    C1 --> D1[AGENTS.md]
    C2 --> D1
    C3 --> D1
    C4 --> D1

    C1 --> D2[agentconfig.yaml]
    C2 --> D2
    C3 --> D2
    C4 --> D2
    C5 --> D2

    C1 --> D3[kelos-workers.yaml]
    C2 --> D3
    C3 --> D3
    C4 --> D3
    C5 --> D3

    C3 --> D4[kelos-api-reviewer.yaml\nnew compatibility checklist]

    C1 --> D5[kelos-reviewer.yaml\nsecurity + doc-accuracy checklist]
    C2 --> D5
    C4 --> D5

    C5 --> D6[kelos-reviewer.yaml\nbranch: with index Branch main]

    D6 --> E[Resolves prior P2 finding\nfrom PR #786 review]

Loading

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
self-development/agentconfig.yaml:41
**Documented convention form differs from canonical implementation**

The convention here prescribes `{{if .Branch}}{{.Branch}}{{else}}main{{end}}`, but `kelos-reviewer.yaml` and `kelos-api-reviewer.yaml` both use `{{with index . "Branch"}}{{.}}{{else}}main{{end}}`. Both are functionally equivalent for map access in Go templates, but the same inconsistency exists in `kelos-workers.yaml` line 40. Given that this PR introduces the "Docs must match implementation" rule, the documented example should match the canonical form actually deployed.

```suggestion
      - The `{{.Branch}}` template variable is empty for issue-only events; use `{{with index . "Branch"}}{{.}}{{else}}main{{end}}` when it may be empty
```

### Issue 2 of 2
self-development/kelos-workers.yaml:40
**Documented convention form differs from canonical implementation**

Same issue as `agentconfig.yaml` line 41 — the prescribed form `{{if .Branch}}{{.Branch}}{{else}}main{{end}}` does not match the `{{with index . "Branch"}}{{.}}{{else}}main{{end}}` form used in `kelos-reviewer.yaml` and `kelos-api-reviewer.yaml`. The convention text and the deployed examples should agree.

```suggestion
      - The `{{.Branch}}` template variable is empty for issue-only events; use `{{with index . "Branch"}}{{.}}{{else}}main{{end}}` when it may be empty
```

_{Reviews (3): Last reviewed commit: "self-development: add API backward-compa..." | Re-trigger Greptile}

[cubic-dev-ai]

You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment @cubic-dev-ai review.