From 901c2c99effe876ec7b37d68276cc5c931f33a36 Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 13:18:23 -0700 Subject: [PATCH 01/19] feat(drive-by): make `ResourceSync` spec immutable --- manifests/crd.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/manifests/crd.yml b/manifests/crd.yml index d14470b..aa291cc 100644 --- a/manifests/crd.yml +++ b/manifests/crd.yml @@ -139,6 +139,9 @@ spec: - source - target type: object + x-kubernetes-validations: + - message: spec is immutable + rule: "self == oldSelf" status: nullable: true properties: From b31247145c3832b4c0cc92b7313d6104f58b7411 Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 13:23:02 -0700 Subject: [PATCH 02/19] docs: readme skill --- .agents/skills/create-readme/SKILL.md | 66 +++++++++++++++++++ .../skills/create-readme/agents/openai.yaml | 4 ++ 2 files changed, 70 insertions(+) create mode 100644 .agents/skills/create-readme/SKILL.md create mode 100644 .agents/skills/create-readme/agents/openai.yaml diff --git a/.agents/skills/create-readme/SKILL.md b/.agents/skills/create-readme/SKILL.md new file mode 100644 index 0000000..565704a --- /dev/null +++ b/.agents/skills/create-readme/SKILL.md @@ -0,0 +1,66 @@ +--- +name: create-readme +description: Create or update a README for a user-specified repository module, package, or directory by thoroughly reading the code and documenting its purpose, architecture, APIs, behavior, maintenance guidance, and control flow. Use when the user asks to write module documentation, create a README, document a directory, explain how a package works for users and developers, or add a Mermaid diagram to documentation. +--- + +> **After completing tasks with this skill:** Invoke `improving-skills` to capture feedback and lessons learned. + +# Create README + +Write detailed README documentation for a module or directory based on the actual code, tests, manifests, and existing docs. The README should serve both users who need to understand how to use the module and developers who need to maintain it. + +## Workflow + +1. Identify the requested module or directory. If the target is ambiguous, inspect likely paths before asking for clarification. +2. For requests scoped to changes between refs, inspect `git diff --name-status ...` and the relevant hunks first, then update only documentation affected by behavior changes while validating against referenced code, API types, manifests, and tests. +3. When changes alter component registration, operational status, supported combinations, or an inventory of modules, controllers, commands, or resources, audit ancestor module READMEs and applicable `AGENTS.md` files for affected summaries or status tables. Update only statements made stale by the scoped changes. +4. For requests scoped to one or more explicit commits, inspect each commit with `git show --name-status --patch ` or `git diff ^!` before choosing README updates. +5. For requests scoped to changes since the target README was last updated, find the last README commit with `git log -1 --format=%H -- `, inspect `git diff ..HEAD -- `, then validate affected changes against current code before editing. +6. After inspecting a commit or diff scope, extract changed identifiers, fields, configuration keys, and behaviors from the hunks. Search the complete target README and applicable ancestor documentation for every occurrence so distant lifecycle, operations, and maintenance statements are included in the audit. +7. Read the code thoroughly enough to explain purpose, responsibilities, public interfaces, data flow, operational behavior, and maintenance concerns. +8. Inspect adjacent tests, examples, generated manifests, existing docs, package metadata, and callers/importers when they clarify real usage. +9. Draft or update the README in the target directory unless the user specifies another output path. +10. Verify that the documentation matches the code and does not invent behavior, commands, APIs, configuration, or dependencies. + +## Investigation Guidance + +Use repository-native tools first: + +- Use `rg --files ` to map the target directory. +- Use `rg` to find callers, type definitions, configuration keys, CRD fields, CLI commands, and tests. +- Read package files such as `go.mod`, `Makefile`, `README.md`, `PROJECT`, `config/`, `api/`, `internal/`, and `cmd/` when relevant. +- For Go modules, inspect exported types/functions, controllers, reconcilers, tests, generated API types, and package comments. +- When documenting Go test or validation commands for a package tree, use a recursive package pattern such as `./path/to/package/...` by default so nested subpackages are included. Use a single-package path only when the sample intentionally excludes subpackages, and explain that narrower scope when it is not obvious. +- For Kubernetes controllers or operators, inspect CRDs/API types, reconcile loops, RBAC markers, owned resources, and manifests. + +Trace behavior from entrypoints to side effects. Prefer concrete file references and observed code paths over inferred intent. + +## README Content + +Include sections that fit the module. Do not force every section if it would add empty or speculative content. + +- Purpose: what the module does and why it exists. +- Scope: what is inside the directory and what is deliberately handled elsewhere. +- Architecture: main packages, components, controllers, commands, data types, or resources. +- Control flow: how requests, reconciliation, generation, or execution moves through the module. +- Usage: public APIs, CLI commands, configuration fields, examples, or integration points. +- Operations: deployment behavior, runtime assumptions, observability, failure modes, and dependencies. +- Development: how to test, regenerate, validate, or safely extend the module. +- Maintenance notes: invariants, common mistakes, important source-of-truth files, and coupling to other packages. + +## Mermaid Diagrams + +Include an embedded Mermaid diagram when it clarifies non-trivial control flow, reconciliation, build/generation pipelines, data movement, or resource ownership. Keep diagrams simple enough to maintain. + +Use `flowchart TD` for most control flow and resource relationship diagrams. Use `sequenceDiagram` only when the ordering between actors is central to understanding behavior. + +## Writing Standards + +- Be detailed but concise. Prefer precise explanations over broad marketing language. +- Write for both users and developers; separate usage from implementation details when helpful. +- Name real files, packages, types, commands, CRDs, and configuration fields. +- Mark unknowns explicitly if the code does not answer them. +- Preserve existing README content when updating unless it is obsolete or contradicted by the code. +- Avoid documenting internal guesses as facts. + +Before finishing, reread the README against the implementation and report any validation performed. diff --git a/.agents/skills/create-readme/agents/openai.yaml b/.agents/skills/create-readme/agents/openai.yaml new file mode 100644 index 0000000..3b1e2ca --- /dev/null +++ b/.agents/skills/create-readme/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "Create README" + short_description: "Write detailed module README files" + default_prompt: "Use $create-readme to document this module with a clear README." From 8a8a55e2cf6652c9065c04e506f702b331ed9239 Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 13:32:49 -0700 Subject: [PATCH 03/19] docs(1): readme skill --- .agents/skills/create-readme/SKILL.md | 148 ++++++++++++++++++-------- 1 file changed, 103 insertions(+), 45 deletions(-) diff --git a/.agents/skills/create-readme/SKILL.md b/.agents/skills/create-readme/SKILL.md index 565704a..0141665 100644 --- a/.agents/skills/create-readme/SKILL.md +++ b/.agents/skills/create-readme/SKILL.md @@ -1,66 +1,124 @@ --- name: create-readme -description: Create or update a README for a user-specified repository module, package, or directory by thoroughly reading the code and documenting its purpose, architecture, APIs, behavior, maintenance guidance, and control flow. Use when the user asks to write module documentation, create a README, document a directory, explain how a package works for users and developers, or add a Mermaid diagram to documentation. +description: Create or update repository, module, package, or directory READMEs grounded in code, tests, and configuration. Use when the user asks to write README documentation for users and maintainers, including architecture and control-flow explanations with Mermaid diagrams where useful. --- > **After completing tasks with this skill:** Invoke `improving-skills` to capture feedback and lessons learned. # Create README -Write detailed README documentation for a module or directory based on the actual code, tests, manifests, and existing docs. The README should serve both users who need to understand how to use the module and developers who need to maintain it. +Write README documentation based on the actual implementation, tests, configuration, and existing docs. Explain how to +use and maintain the target, with detail proportional to its complexity and the user's requested scope. ## Workflow -1. Identify the requested module or directory. If the target is ambiguous, inspect likely paths before asking for clarification. -2. For requests scoped to changes between refs, inspect `git diff --name-status ...` and the relevant hunks first, then update only documentation affected by behavior changes while validating against referenced code, API types, manifests, and tests. -3. When changes alter component registration, operational status, supported combinations, or an inventory of modules, controllers, commands, or resources, audit ancestor module READMEs and applicable `AGENTS.md` files for affected summaries or status tables. Update only statements made stale by the scoped changes. -4. For requests scoped to one or more explicit commits, inspect each commit with `git show --name-status --patch ` or `git diff ^!` before choosing README updates. -5. For requests scoped to changes since the target README was last updated, find the last README commit with `git log -1 --format=%H -- `, inspect `git diff ..HEAD -- `, then validate affected changes against current code before editing. -6. After inspecting a commit or diff scope, extract changed identifiers, fields, configuration keys, and behaviors from the hunks. Search the complete target README and applicable ancestor documentation for every occurrence so distant lifecycle, operations, and maintenance statements are included in the audit. -7. Read the code thoroughly enough to explain purpose, responsibilities, public interfaces, data flow, operational behavior, and maintenance concerns. -8. Inspect adjacent tests, examples, generated manifests, existing docs, package metadata, and callers/importers when they clarify real usage. -9. Draft or update the README in the target directory unless the user specifies another output path. -10. Verify that the documentation matches the code and does not invent behavior, commands, APIs, configuration, or dependencies. +1. Identify the requested target, output path, and applicable `AGENTS.md` instructions. If the target is ambiguous, + inspect likely paths before asking for clarification. Resolve module names to actual files; a Rust module may be a + single `.rs` file rather than a directory. Use the containing directory's README or existing documentation coverage + unless the user specifies another location. +2. Establish any requested change scope before choosing edits: + - Between refs: inspect `git diff --name-status ...` and the relevant hunks for branch changes since the + merge base; use `..` for a direct comparison of the two trees. + - Explicit commits: inspect each with `git show --name-status --patch `. + - Since the README was last updated: find its last commit with `git log -1 --format=%H -- `, then inspect + `git diff ..HEAD -- `. If it has no history, inspect the current implementation directly. +3. Read the implementation thoroughly enough to explain responsibilities, public interfaces, data flow, side effects, + and maintenance concerns. Follow callers, related modules, tests, examples, and build or deployment files where they + clarify behavior. For historical documentation, validate against the requested revision; otherwise use current code. +4. For scoped updates, extract changed identifiers, configuration fields, and behaviors from the diff. Search the + complete target README and relevant ancestor or linked documentation for affected usage examples, lifecycle + descriptions, and summaries. Include related files outside the target when needed to verify behavior, but keep + documentation edits tied to the requested scope. +5. Draft or update the README, preserving accurate existing content and the user's chosen structure. +6. Verify claims, examples, commands, and links against their sources. Report validation performed and any unresolved + discrepancies. ## Investigation Guidance -Use repository-native tools first: - -- Use `rg --files ` to map the target directory. -- Use `rg` to find callers, type definitions, configuration keys, CRD fields, CLI commands, and tests. -- Read package files such as `go.mod`, `Makefile`, `README.md`, `PROJECT`, `config/`, `api/`, `internal/`, and `cmd/` when relevant. -- For Go modules, inspect exported types/functions, controllers, reconcilers, tests, generated API types, and package comments. -- When documenting Go test or validation commands for a package tree, use a recursive package pattern such as `./path/to/package/...` by default so nested subpackages are included. Use a single-package path only when the sample intentionally excludes subpackages, and explain that narrower scope when it is not obvious. -- For Kubernetes controllers or operators, inspect CRDs/API types, reconcile loops, RBAC markers, owned resources, and manifests. - -Trace behavior from entrypoints to side effects. Prefer concrete file references and observed code paths over inferred intent. +- Use `rg --files ` to map the directory and `rg` to find definitions, callers, configuration keys, and tests. + Include hidden paths explicitly when inspecting CI or repository instructions. +- Read package metadata, toolchain configuration, build scripts, and CI before documenting development commands. For + Rust, inspect `Cargo.toml`, `Cargo.lock`, `rust-toolchain.toml`, module declarations, public items and re-exports, + serialization attributes, and inline `#[cfg(test)]` modules. +- Trace behavior from entrypoints through transformations to side effects, including error and cleanup paths. + Distinguish public interfaces from internal helpers and implemented behavior from comments or TODOs. +- For Kubernetes behavior, inspect API types, schemas, reconciliation, watches, finalizers, ownership, RBAC manifests, + and deployment configuration as relevant. Distinguish controller logic from API-server validation and permissions + required from permissions actually supplied by the deployment. +- Compare generated artifacts with their generator and checked-in versions when they affect the documentation. If they + disagree, explain which behavior each source establishes and report the discrepancy; do not silently choose one as + authoritative or regenerate tracked files merely to write a README. +- Verify dependency-provided flags and runtime behavior against the resolved dependency source or executable help. Do + not turn a build-time API feature selection or pinned toolchain into a claim about minimum supported runtime versions + without supporting evidence. + +### Sinker Source Pointers + +Sinker is a single Rust package with a controller binary and library modules. Use these pointers only when relevant to +the requested documentation; they are starting points for investigation, not a required README outline. Paths are +relative to the repository root. + +- `src/main.rs` and `src/lib.rs`: CLI entrypoint, runtime setup, module visibility, and shared errors. Client and admin + arguments are flattened from `kubert`, so their full interface is not declared locally. +- `src/resources.rs` and `manifests/crd.yml`: `ResourceSync` and `SinkerContainer` schemas, serialized field names, + defaults, and validation. `SinkerContainer` has a manually supplied schema; inspect that as well as the Rust types. + Check examples in `example.yaml` against these sources and runtime handling. +- `src/controller.rs`, `src/remote_watcher.rs`, `src/remote_watcher_manager.rs`, and `src/filters.rs`: reconciliation + triggers, target application, status, deletion, watcher lifecycle, and filtering of self-generated events. Follow both + reconciliation and watch paths before describing retries, drift correction, or cleanup guarantees. +- `src/resource_extensions.rs`: client selection, resource discovery, namespace resolution, and access checks for + kubeconfig Secrets. Distinguish the namespace holding credentials from the source or target resource namespace, and + check local, remote, and cluster-scoped cases when documenting references. +- `src/mapping.rs` and its tests: whole-resource copying, field selection, target construction, and metadata handling. + Source selectors and destination paths use different parsing logic; verify their syntax and missing-value behavior + separately. +- `manifests/`, `Dockerfile`, and `.github/workflows/rust.yml`: deployment, RBAC, container packaging, and build or + publication commands. Derive operational examples from these files and identify placeholders or environment-specific + values. + +### Development and Generation Commands + +Use commands appropriate to the documented target and verify them against the current CI workflow. This repository uses +`cargo build`, `cargo fmt`, `cargo test`, and `cargo clippy --all-targets --all-features`. Tests are inline in the Rust +modules. Cargo test filters match test names, not filesystem paths; if documenting a narrower command, check the +selected tests with `cargo test -- --list`. + +The `manifests` subcommand in `src/main.rs` emits CRDs. CI runs `cargo run -- manifests > manifests/crd.yml` and checks +for drift. When verifying documentation, direct generated output to a temporary file for comparison so checked-in schema +changes are preserved. Keep CRD generation distinct from rendering the complete deployment through +`manifests/kustomization.yaml`. ## README Content -Include sections that fit the module. Do not force every section if it would add empty or speculative content. +Include sections that fit the target. Do not force every section or expand a focused module README into a full +deployment guide. -- Purpose: what the module does and why it exists. -- Scope: what is inside the directory and what is deliberately handled elsewhere. -- Architecture: main packages, components, controllers, commands, data types, or resources. -- Control flow: how requests, reconciliation, generation, or execution moves through the module. -- Usage: public APIs, CLI commands, configuration fields, examples, or integration points. -- Operations: deployment behavior, runtime assumptions, observability, failure modes, and dependencies. -- Development: how to test, regenerate, validate, or safely extend the module. -- Maintenance notes: invariants, common mistakes, important source-of-truth files, and coupling to other packages. +- Purpose and scope: what the target does, who uses it, and what is handled elsewhere. +- Architecture and control flow: the main components, interfaces, and paths through execution. +- Usage: public APIs, commands, configuration, examples, and integration points. +- Operations: runtime assumptions, observability, failure modes, lifecycle behavior, and dependencies. +- Development and maintenance: how to test, generate, validate, or extend the target; invariants, source-of-truth files, + and coupling to other modules. ## Mermaid Diagrams -Include an embedded Mermaid diagram when it clarifies non-trivial control flow, reconciliation, build/generation pipelines, data movement, or resource ownership. Keep diagrams simple enough to maintain. - -Use `flowchart TD` for most control flow and resource relationship diagrams. Use `sequenceDiagram` only when the ordering between actors is central to understanding behavior. - -## Writing Standards - -- Be detailed but concise. Prefer precise explanations over broad marketing language. -- Write for both users and developers; separate usage from implementation details when helpful. -- Name real files, packages, types, commands, CRDs, and configuration fields. -- Mark unknowns explicitly if the code does not answer them. -- Preserve existing README content when updating unless it is obsolete or contradicted by the code. -- Avoid documenting internal guesses as facts. - -Before finishing, reread the README against the implementation and report any validation performed. +Include an embedded Mermaid diagram when it clarifies non-trivial control flow, reconciliation, build or generation +pipelines, data movement, or resource ownership. Keep diagrams simple enough to maintain and include only relationships +supported by the implementation. + +Use `flowchart TD` for most control flow and resource relationships. Use `sequenceDiagram` when ordering between actors +is central to understanding behavior. + +## Writing and Verification + +- Be detailed but concise. Separate usage from implementation details when that helps the reader. +- Name real files, types, commands, resources, and configuration fields. Use serialized names in configuration examples + and Rust identifiers when discussing code. +- Link to source files and related documentation using paths relative to the README's location. +- Treat existing docs and example fixtures as evidence to check, not proof that a behavior or deployment is supported. + Mark consequential unknowns explicitly. +- Reread prose, tables, examples, and diagrams together against the implementation so a scoped update leaves no + contradictory descriptions. +- Run checks that substantiate the documentation changes; distinguish commands verified by inspection from checks + actually executed. Report implementation or manifest discrepancies without expanding a documentation task into + unrelated repairs. From 924cddcee2df5b80ddd7646621273c756623de96 Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 13:38:01 -0700 Subject: [PATCH 04/19] docs: improving skills --- .agents/skills/improving-skills/SKILL.md | 105 +++++++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 .agents/skills/improving-skills/SKILL.md diff --git a/.agents/skills/improving-skills/SKILL.md b/.agents/skills/improving-skills/SKILL.md new file mode 100644 index 0000000..9141f14 --- /dev/null +++ b/.agents/skills/improving-skills/SKILL.md @@ -0,0 +1,105 @@ +--- +name: improving-skills +description: Use when finishing a task that used a project skill from .agents/skills/, or when a skill was unclear, missing information, or could be improved. Triggers feedback collection for continuous skill improvement. +--- + +# Improving Skills + +## Overview + +After using any skill from this project's `.agents/skills/` directory, collect feedback and propose improvements. Skills improve through use — gaps found today become fixes tomorrow. + +## When This Applies + +**Trigger after using ANY skill in `.agents/skills/`** (this repo's skills, not superpowers). + +How to know: If you invoked a skill for this starfleet repo and completed the task, invoke this skill next. + +## Feedback Collection + +After completing the task that used the skill, ask yourself: + +| Question | Why It Matters | +|----------|----------------| +| What was missing? | Gaps cause future agents to repeat workarounds | +| What was unclear? | Confusing sections slow everyone down | +| What was most useful? | Confirms what to keep/expand | +| What was wrong? | Errors propagate if not fixed | + +## Workflow + +```dot +digraph feedback { + "Task using skill completed" -> "Urgent follow-up task?"; + "Urgent follow-up task?" -> "Do urgent task FIRST" [label="yes"]; + "Urgent follow-up task?" -> "Any issues or feedback?" [label="no"]; + "Do urgent task FIRST" -> "Any issues or feedback?"; + "Any issues or feedback?" -> "Draft improvement" [label="yes"]; + "Any issues or feedback?" -> "Done" [label="no, skill was perfect"]; + "Draft improvement" -> "Show user proposed changes"; + "Show user proposed changes" -> "User approves?"; + "User approves?" -> "Edit skill file" [label="yes"]; + "User approves?" -> "Done" [label="no"]; + "Edit skill file" -> "Done"; +} +``` + +**Key point:** If there's an urgent follow-up task, handle it first — but you MUST still provide skill feedback before the session ends. "Later" in an ephemeral session means "never." + +## Proposing Changes + +**Always propose before editing.** Format: + +```markdown +## Skill Improvement Proposal + +**Skill:** [skill-name] +**Issue:** [gap/unclear/wrong/enhancement] + +**Current content:** +[quote relevant section or "missing"] + +**Proposed change:** +[new or revised content] + +**Rationale:** +[why this helps future agents] +``` + +After user approves, edit the skill file directly. + +This takes 30 seconds, not 30 minutes. A quick proposal with a one-sentence rationale is enough — don't over-formalize it. + +## Red Flags - You're Skipping Feedback + +| Thought | Reality | +|---------|---------| +| "The skill worked, nothing to report" | Positive confirmation helps too — what worked well? | +| "Reporting feels like extra work" | 30 seconds of feedback saves hours of repeated workarounds | +| "User didn't ask for feedback" | This skill IS asking for feedback — you have permission | +| "It's not my job to improve docs" | Every agent using skills should improve them | +| "I'll come back to this later" | You won't. Sessions are ephemeral. Later = never. Write it now. | +| "I'll invoke this skill later" | Invoke NOW while context is fresh. Later = never. | +| "This is too minor to report" | Minor issues compound. Report it. | +| "That skill isn't my domain" | You used it. You found gaps. You're the best person to report them right now. | +| "The skill owner should fix it" | There is no single owner. Every user is a maintainer. | +| "I need to prioritize the user's next request" | Handle urgent work first, then provide feedback. Both matter. | +| "Feedback would create context-switching friction" | A 30-second proposal isn't a context switch — it's a note. | + +## What NOT to Report + +- Typos (fix silently if obvious) +- Style preferences (skills have varied styles, that's fine) +- Hypothetical improvements ("someday we might need...") +- Changes to superpowers skills (those have their own process) + +## Example Improvements + +**Gap found:** +> Backlog skill didn't cover how to handle issues that span both starfleet and tubernetes repos. Added cross-repo issue linking guidance. + +**Unclear section:** +> The `gh project item-edit` section didn't clarify which fields can be set via CLI vs require the GitHub UI. Added a table. + +**Worked well:** +> The agent team structure in prioritizing-backlog saved significant time on data gathering. Consider similar patterns for other data-heavy skills. From 462d88d01d0067a073aeea04245c29e380b28231 Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 13:43:50 -0700 Subject: [PATCH 05/19] docs(1): improving skills --- .agents/skills/improving-skills/SKILL.md | 132 ++++++++--------------- 1 file changed, 47 insertions(+), 85 deletions(-) diff --git a/.agents/skills/improving-skills/SKILL.md b/.agents/skills/improving-skills/SKILL.md index 9141f14..a1f8685 100644 --- a/.agents/skills/improving-skills/SKILL.md +++ b/.agents/skills/improving-skills/SKILL.md @@ -1,105 +1,67 @@ --- name: improving-skills -description: Use when finishing a task that used a project skill from .agents/skills/, or when a skill was unclear, missing information, or could be improved. Triggers feedback collection for continuous skill improvement. +description: Collect feedback after using a project skill from .agents/skills/, when a used skill is unclear, incomplete, or incorrect, or when the user requests a skill review. Propose or apply scoped improvements grounded in the task just completed. --- -# Improving Skills +> **After completing tasks with this skill:** Invoke `improving-skills` to capture feedback and lessons learned. +> For this skill, include one self-review in the current feedback pass; do not recursively invoke it. -## Overview +# Improving Skills -After using any skill from this project's `.agents/skills/` directory, collect feedback and propose improvements. Skills improve through use — gaps found today become fixes tomorrow. +Use lessons from completed work to improve skill instructions and supporting resources. Keep feedback concrete and +proportional to the task, preserving guidance that worked well. ## When This Applies -**Trigger after using ANY skill in `.agents/skills/`** (this repo's skills, not superpowers). - -How to know: If you invoked a skill for this starfleet repo and completed the task, invoke this skill next. - -## Feedback Collection - -After completing the task that used the skill, ask yourself: +- After completing a task that used a skill from the current project's `.agents/skills/` directory. +- When a skill used during the task was unclear, incomplete, or incorrect. +- When the user explicitly requests feedback on a skill, including one stored outside the repository. -| Question | Why It Matters | -|----------|----------------| -| What was missing? | Gaps cause future agents to repeat workarounds | -| What was unclear? | Confusing sections slow everyone down | -| What was most useful? | Confirms what to keep/expand | -| What was wrong? | Errors propagate if not fixed | +Resolve skills to their actual locations and read applicable `AGENTS.md` instructions. Follow references relevant to the +observed issue. Do not assume that skills from another repository or skill framework are installed here. ## Workflow -```dot -digraph feedback { - "Task using skill completed" -> "Urgent follow-up task?"; - "Urgent follow-up task?" -> "Do urgent task FIRST" [label="yes"]; - "Urgent follow-up task?" -> "Any issues or feedback?" [label="no"]; - "Do urgent task FIRST" -> "Any issues or feedback?"; - "Any issues or feedback?" -> "Draft improvement" [label="yes"]; - "Any issues or feedback?" -> "Done" [label="no, skill was perfect"]; - "Draft improvement" -> "Show user proposed changes"; - "Show user proposed changes" -> "User approves?"; - "User approves?" -> "Edit skill file" [label="yes"]; - "User approves?" -> "Done" [label="no"]; - "Edit skill file" -> "Done"; -} -``` - -**Key point:** If there's an urgent follow-up task, handle it first — but you MUST still provide skill feedback before the session ends. "Later" in an ephemeral session means "never." - -## Proposing Changes - -**Always propose before editing.** Format: +1. Finish the user's task and any urgent follow-up first. Capture feedback while the evidence is fresh and include it + in the task's final response. +2. Review the skills used and the decisions or workarounds they caused. Identify missing guidance, unclear instructions, + incorrect assumptions, and useful guidance to retain. Ground findings in the completed work and verify proposed paths, + commands, and behavior against the destination repository or available tools. +3. Check whether the reviewed skills include the feedback hook below. Recommend it for every project skill, including + this one. Keep edits within the authorized scope; a missing hook elsewhere is a recommendation, not a reason to edit + every skill in the repository. +4. Apply improvements already covered by the user's request or earlier authorization without asking again. Otherwise, + prepare a concrete proposal before requesting approval for substantive edits outside that scope. Feedback collection + alone does not authorize changes to unrelated skills or externally managed bundles. +5. Update affected supporting resources together with `SKILL.md`, preserving unrelated edits and invocation policy. + Check frontmatter, references, and consistency across the changed bundle. Run focused checks for changed executable + helpers; distinguish checks actually run from behavior verified by inspection. +6. Report meaningful changes, remaining proposals, and validation briefly. If no change is warranted, mention what worked + well without inventing an improvement. Include one review of `improving-skills` itself in this pass. That review + satisfies its own feedback hook; finish without starting another feedback cycle solely because this skill ran. + +## Feedback Hook for Project Skills + +Recommend that all project skills, including `improving-skills` itself, include this instruction after the frontmatter +or in an equivalent completion section: ```markdown -## Skill Improvement Proposal - -**Skill:** [skill-name] -**Issue:** [gap/unclear/wrong/enhancement] - -**Current content:** -[quote relevant section or "missing"] - -**Proposed change:** -[new or revised content] - -**Rationale:** -[why this helps future agents] +> **After completing tasks with this skill:** Invoke `improving-skills` to capture feedback and lessons learned. ``` -After user approves, edit the skill file directly. - -This takes 30 seconds, not 30 minutes. A quick proposal with a one-sentence rationale is enough — don't over-formalize it. - -## Red Flags - You're Skipping Feedback - -| Thought | Reality | -|---------|---------| -| "The skill worked, nothing to report" | Positive confirmation helps too — what worked well? | -| "Reporting feels like extra work" | 30 seconds of feedback saves hours of repeated workarounds | -| "User didn't ask for feedback" | This skill IS asking for feedback — you have permission | -| "It's not my job to improve docs" | Every agent using skills should improve them | -| "I'll come back to this later" | You won't. Sessions are ephemeral. Later = never. Write it now. | -| "I'll invoke this skill later" | Invoke NOW while context is fresh. Later = never. | -| "This is too minor to report" | Minor issues compound. Report it. | -| "That skill isn't my domain" | You used it. You found gaps. You're the best person to report them right now. | -| "The skill owner should fix it" | There is no single owner. Every user is a maintainer. | -| "I need to prioritize the user's next request" | Handle urgent work first, then provide feedback. Both matter. | -| "Feedback would create context-switching friction" | A 30-second proposal isn't a context switch — it's a note. | - -## What NOT to Report - -- Typos (fix silently if obvious) -- Style preferences (skills have varied styles, that's fine) -- Hypothetical improvements ("someday we might need...") -- Changes to superpowers skills (those have their own process) - -## Example Improvements +Preserve equivalent existing instructions rather than duplicating them. When creating, migrating, or updating a project +skill within the user's requested scope, add the hook if missing. For `improving-skills`, keep the single-pass self-review +qualification shown above so the hook terminates. -**Gap found:** -> Backlog skill didn't cover how to handle issues that span both starfleet and tubernetes repos. Added cross-repo issue linking guidance. +## Useful Feedback -**Unclear section:** -> The `gh project item-edit` section didn't clarify which fields can be set via CLI vs require the GitHub UI. Added a table. +For each material finding, give the skill name and file, the observed issue or successful guidance, the proposed or applied +change, and a brief rationale. Quote existing text only when needed to make the change understandable. A short paragraph +or small diff is usually enough; combine related findings. -**Worked well:** -> The agent team structure in prioritizing-backlog saved significant time on data gathering. Consider similar patterns for other data-heavy skills. +- Report concrete gaps, contradictory instructions, stale dependencies, and verified path or command errors. +- Keep guidance that helped the task, especially constraints tied to an observed failure mode. +- Fix obvious typos within an authorized edit without a separate proposal. +- Skip stylistic preferences and hypothetical future needs. Do not turn one task's details into universal requirements. +- For skills maintained outside the project, respect their ownership and update process. Propose changes unless the + user's request or existing authorization covers editing that bundle. From 548845da89f440262c65ada8b834bbe0fc44a4c2 Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 14:00:13 -0700 Subject: [PATCH 06/19] docs: agent facing docs guide --- .agents/skills/create-readme/SKILL.md | 73 +++++++------------ .../skills/create-readme/agents/openai.yaml | 4 +- .../references/agent-facing-documentation.md | 72 ++++++++++++++++++ .../references/sinker-source-guide.md | 39 ++++++++++ .agents/skills/improving-skills/SKILL.md | 65 ++++++++++++----- 5 files changed, 186 insertions(+), 67 deletions(-) create mode 100644 .agents/skills/create-readme/references/agent-facing-documentation.md create mode 100644 .agents/skills/create-readme/references/sinker-source-guide.md diff --git a/.agents/skills/create-readme/SKILL.md b/.agents/skills/create-readme/SKILL.md index 0141665..6eb96f0 100644 --- a/.agents/skills/create-readme/SKILL.md +++ b/.agents/skills/create-readme/SKILL.md @@ -1,20 +1,31 @@ --- name: create-readme -description: Create or update repository, module, package, or directory READMEs grounded in code, tests, and configuration. Use when the user asks to write README documentation for users and maintainers, including architecture and control-flow explanations with Mermaid diagrams where useful. +description: Create or update repository, module, package, or directory READMEs and agent-facing documentation such as skills and AGENTS.md files. Ground guidance in sources, keep primary documents concise, and link to task-specific details. --- > **After completing tasks with this skill:** Invoke `improving-skills` to capture feedback and lessons learned. # Create README -Write README documentation based on the actual implementation, tests, configuration, and existing docs. Explain how to -use and maintain the target, with detail proportional to its complexity and the user's requested scope. +Write documentation based on the actual implementation, tests, configuration, and existing docs. Tailor it to the +intended audience, with detail proportional to the target's complexity and the user's requested scope. + +## Agent-Facing Documentation and Context + +For all documentation, including general `README.md` files, keep primary documents concise: summarize essentials, +link to existing sources, and separate substantial task-specific detail into references that readers can load when needed. +Preserve the information needed by the intended audience and the constraints needed to act correctly. + +When authoring or improving skills or `AGENTS.md` files, read +[agent-facing documentation guidance](references/agent-facing-documentation.md) for audience selection, reference +structure, avoiding duplication, and self-improvement hooks. For README restructuring, use its guidance on summaries +and references while preserving human-facing usage information. ## Workflow -1. Identify the requested target, output path, and applicable `AGENTS.md` instructions. If the target is ambiguous, +1. Identify the requested target, audience, output path, and applicable `AGENTS.md` instructions. If the target is ambiguous, inspect likely paths before asking for clarification. Resolve module names to actual files; a Rust module may be a - single `.rs` file rather than a directory. Use the containing directory's README or existing documentation coverage + single `.rs` file rather than a directory. For README tasks, use the containing directory's README or existing coverage unless the user specifies another location. 2. Establish any requested change scope before choosing edits: - Between refs: inspect `git diff --name-status ...` and the relevant hunks for branch changes since the @@ -22,14 +33,16 @@ use and maintain the target, with detail proportional to its complexity and the - Explicit commits: inspect each with `git show --name-status --patch `. - Since the README was last updated: find its last commit with `git log -1 --format=%H -- `, then inspect `git diff ..HEAD -- `. If it has no history, inspect the current implementation directly. -3. Read the implementation thoroughly enough to explain responsibilities, public interfaces, data flow, side effects, - and maintenance concerns. Follow callers, related modules, tests, examples, and build or deployment files where they - clarify behavior. For historical documentation, validate against the requested revision; otherwise use current code. +3. Read the sources needed to substantiate the requested documentation. For implementation docs, trace responsibilities, + public interfaces, data flow, side effects, and maintenance concerns through relevant code, tests, and configuration. + For agent instructions, verify workflows, commands, and constraints against applicable instructions and available tools. + Follow references when relevant to the task. For historical docs, validate against the requested revision. 4. For scoped updates, extract changed identifiers, configuration fields, and behaviors from the diff. Search the - complete target README and relevant ancestor or linked documentation for affected usage examples, lifecycle + complete target document and relevant ancestor or linked documentation for affected usage examples, lifecycle descriptions, and summaries. Include related files outside the target when needed to verify behavior, but keep documentation edits tied to the requested scope. -5. Draft or update the README, preserving accurate existing content and the user's chosen structure. +5. Draft or update the document, preserving accurate existing content and the user's chosen structure. Summarize and + link to existing coverage before creating new references; keep each detailed topic in one maintained location. 6. Verify claims, examples, commands, and links against their sources. Report validation performed and any unresolved discrepancies. @@ -52,41 +65,8 @@ use and maintain the target, with detail proportional to its complexity and the not turn a build-time API feature selection or pinned toolchain into a claim about minimum supported runtime versions without supporting evidence. -### Sinker Source Pointers - -Sinker is a single Rust package with a controller binary and library modules. Use these pointers only when relevant to -the requested documentation; they are starting points for investigation, not a required README outline. Paths are -relative to the repository root. - -- `src/main.rs` and `src/lib.rs`: CLI entrypoint, runtime setup, module visibility, and shared errors. Client and admin - arguments are flattened from `kubert`, so their full interface is not declared locally. -- `src/resources.rs` and `manifests/crd.yml`: `ResourceSync` and `SinkerContainer` schemas, serialized field names, - defaults, and validation. `SinkerContainer` has a manually supplied schema; inspect that as well as the Rust types. - Check examples in `example.yaml` against these sources and runtime handling. -- `src/controller.rs`, `src/remote_watcher.rs`, `src/remote_watcher_manager.rs`, and `src/filters.rs`: reconciliation - triggers, target application, status, deletion, watcher lifecycle, and filtering of self-generated events. Follow both - reconciliation and watch paths before describing retries, drift correction, or cleanup guarantees. -- `src/resource_extensions.rs`: client selection, resource discovery, namespace resolution, and access checks for - kubeconfig Secrets. Distinguish the namespace holding credentials from the source or target resource namespace, and - check local, remote, and cluster-scoped cases when documenting references. -- `src/mapping.rs` and its tests: whole-resource copying, field selection, target construction, and metadata handling. - Source selectors and destination paths use different parsing logic; verify their syntax and missing-value behavior - separately. -- `manifests/`, `Dockerfile`, and `.github/workflows/rust.yml`: deployment, RBAC, container packaging, and build or - publication commands. Derive operational examples from these files and identify placeholders or environment-specific - values. - -### Development and Generation Commands - -Use commands appropriate to the documented target and verify them against the current CI workflow. This repository uses -`cargo build`, `cargo fmt`, `cargo test`, and `cargo clippy --all-targets --all-features`. Tests are inline in the Rust -modules. Cargo test filters match test names, not filesystem paths; if documenting a narrower command, check the -selected tests with `cargo test -- --list`. - -The `manifests` subcommand in `src/main.rs` emits CRDs. CI runs `cargo run -- manifests > manifests/crd.yml` and checks -for drift. When verifying documentation, direct generated output to a temporary file for comparison so checked-in schema -changes are preserved. Keep CRD generation distinct from rendering the complete deployment through -`manifests/kustomization.yaml`. +For Sinker implementation, development commands, or CRD generation, read the relevant parts of the +[Sinker source guide](references/sinker-source-guide.md). ## README Content @@ -114,7 +94,8 @@ is central to understanding behavior. - Be detailed but concise. Separate usage from implementation details when that helps the reader. - Name real files, types, commands, resources, and configuration fields. Use serialized names in configuration examples and Rust identifiers when discussing code. -- Link to source files and related documentation using paths relative to the README's location. +- Link to source files and related documentation using paths relative to the document's location. Explain when each + reference is useful so agents can select relevant context without reading every linked document. - Treat existing docs and example fixtures as evidence to check, not proof that a behavior or deployment is supported. Mark consequential unknowns explicitly. - Reread prose, tables, examples, and diagrams together against the implementation so a scoped update leaves no diff --git a/.agents/skills/create-readme/agents/openai.yaml b/.agents/skills/create-readme/agents/openai.yaml index 3b1e2ca..4ecc982 100644 --- a/.agents/skills/create-readme/agents/openai.yaml +++ b/.agents/skills/create-readme/agents/openai.yaml @@ -1,4 +1,4 @@ interface: display_name: "Create README" - short_description: "Write detailed module README files" - default_prompt: "Use $create-readme to document this module with a clear README." + short_description: "Write READMEs and agent-facing docs" + default_prompt: "Use $create-readme to write or update this documentation with concise summaries and links to relevant details." diff --git a/.agents/skills/create-readme/references/agent-facing-documentation.md b/.agents/skills/create-readme/references/agent-facing-documentation.md new file mode 100644 index 0000000..6da9180 --- /dev/null +++ b/.agents/skills/create-readme/references/agent-facing-documentation.md @@ -0,0 +1,72 @@ +# Agent-Facing Documentation + +Use this guidance when authoring or improving skills, `AGENTS.md` files, and other instructions for agents. Apply the +summary, reference, and reuse patterns to general READMEs too, while retaining what human users and maintainers need. + +## Write for the Agent's Task + +Include information that changes how an agent selects, performs, or verifies work: scope, actionable instructions, +non-obvious constraints, source locations, relevant commands, and completion criteria. Assume the agent already has +general coding and reasoning abilities. Omit generic tutorials, repeated background, and human-oriented material that +does not help it complete the task; link to that material when it provides useful optional context. + +State the condition under which an instruction applies and distinguish requirements from recommendations. Preserve +existing authorization boundaries and instruction scope. Summarizing a requirement must not weaken it or make it appear +optional. Keep essential constraints in the primary document, or require the relevant reference before the affected +action, so an agent cannot reasonably miss them. + +## Keep Primary Documents Brief + +Make the primary document an entry point: purpose and scope, essential instructions, a short workflow or orientation, +and links that explain when more detail is needed. Keep enough context to choose the next action without opening every +reference. Do not optimize for an arbitrary line limit or remove necessary instructions just to shorten the document. + +- In `SKILL.md`, keep selection guidance, the common workflow, and shared constraints. Move substantial mode-specific + procedures, schemas, examples, and troubleshooting into focused `references/` documents. +- In `AGENTS.md`, keep instructions that apply throughout its scope and links to task-specific guidance. Put detailed + build, release, architecture, or subsystem procedures in maintained references. Preserve directory-specific scope when + reorganizing instructions; moving a rule into a reference must not change where it applies. +- In `README.md`, retain a useful overview and common getting-started information. Summarize architecture, operations, + and specialized workflows, linking to detail as needed. Preserve human usability and the user's requested depth. + +Separate substantial topics when agents commonly need them for different tasks. Keep short, tightly related instructions +together when splitting would add navigation without saving meaningful context. The primary document and references +should form a usable path through the task, not require readers to reconstruct instructions from scattered fragments. + +## Reuse Existing Sources + +Before adding an explanation, search the target, relevant ancestor documents, and existing linked docs for coverage. +Prefer a brief summary and a link to the maintained source over copying instructions, command catalogs, schemas, or +background. Create a new reference only when the information has no suitable home, or move existing detail into one +and replace the original with a summary and link. + +Keep each detailed topic in one authoritative location where reasonably possible. A short reminder or prerequisite may +be repeated when necessary to apply an instruction correctly, but avoid parallel versions of the same procedure. If +existing sources disagree, verify the underlying behavior and resolve the discrepancy within scope or report it; do not +silently choose a convenient version. Update affected links when moving content. + +## Make References Selective and Discoverable + +Link references directly from the primary document or the workflow step that needs them. Use descriptive link text and +state the trigger for reading, such as changing resource mapping, generating CRDs, or preparing a release. Avoid +instructions to read every reference before starting an unrelated task. + +Keep each reference focused on a coherent topic, with its own scope and necessary prerequisites. Use paths relative to +the linking document and stable headings for links to specific sections. For long references, provide a short contents +list or useful search terms so agents can locate the needed section. Avoid deep chains of index documents. A reference +may link back for orientation, but following links must not create a mandatory reading or feedback loop. + +## Include Self-Improvement Guidance + +When creating or updating a skill or `AGENTS.md` within the authorized scope, use the hook guidance in +[improving-skills](../../improving-skills/SKILL.md): its +[skill feedback hook](../../improving-skills/SKILL.md#feedback-hook-for-project-skills) or +[AGENTS.md self-improvement section](../../improving-skills/SKILL.md#self-improvement-section-for-agentsmd). +Preserve equivalent hooks already present. Link to the feedback workflow instead of copying it into each document. + +## Review the Result + +Check that an agent can identify the applicable instructions and select the needed references from the primary +document alone. Verify relative links and section anchors, retained requirements and scope, and summaries against their +sources. Look for duplicated procedures and references that would force unrelated context into routine tasks. Preserve +accurate, useful information during extraction, and keep edits within the requested documentation scope. diff --git a/.agents/skills/create-readme/references/sinker-source-guide.md b/.agents/skills/create-readme/references/sinker-source-guide.md new file mode 100644 index 0000000..f430d76 --- /dev/null +++ b/.agents/skills/create-readme/references/sinker-source-guide.md @@ -0,0 +1,39 @@ +# Sinker Source Guide + +Read this reference when documenting Sinker implementation, development, or CRD generation. + +## Source Pointers + +Sinker is a single Rust package with a controller binary and library modules. Use these pointers only when relevant to +the requested documentation; they are starting points for investigation, not a required README outline. Paths are +relative to the repository root. + +- `src/main.rs` and `src/lib.rs`: CLI entrypoint, runtime setup, module visibility, and shared errors. Client and admin + arguments are flattened from `kubert`, so their full interface is not declared locally. +- `src/resources.rs` and `manifests/crd.yml`: `ResourceSync` and `SinkerContainer` schemas, serialized field names, + defaults, and validation. `SinkerContainer` has a manually supplied schema; inspect that as well as the Rust types. + Check examples in `example.yaml` against these sources and runtime handling. +- `src/controller.rs`, `src/remote_watcher.rs`, `src/remote_watcher_manager.rs`, and `src/filters.rs`: reconciliation + triggers, target application, status, deletion, watcher lifecycle, and filtering of self-generated events. Follow both + reconciliation and watch paths before describing retries, drift correction, or cleanup guarantees. +- `src/resource_extensions.rs`: client selection, resource discovery, namespace resolution, and access checks for + kubeconfig Secrets. Distinguish the namespace holding credentials from the source or target resource namespace, and + check local, remote, and cluster-scoped cases when documenting references. +- `src/mapping.rs` and its tests: whole-resource copying, field selection, target construction, and metadata handling. + Source selectors and destination paths use different parsing logic; verify their syntax and missing-value behavior + separately. +- `manifests/`, `Dockerfile`, and `.github/workflows/rust.yml`: deployment, RBAC, container packaging, and build or + publication commands. Derive operational examples from these files and identify placeholders or environment-specific + values. + +## Development and Generation Commands + +Use commands appropriate to the documented target and verify them against the current CI workflow. This repository uses +`cargo build`, `cargo fmt`, `cargo test`, and `cargo clippy --all-targets --all-features`. Tests are inline in the Rust +modules. Cargo test filters match test names, not filesystem paths; if documenting a narrower command, check the +selected tests with `cargo test -- --list`. + +The `manifests` subcommand in `src/main.rs` emits CRDs. CI runs `cargo run -- manifests > manifests/crd.yml` and checks +for drift. When verifying documentation, direct generated output to a temporary file for comparison so checked-in schema +changes are preserved. Keep CRD generation distinct from rendering the complete deployment through +`manifests/kustomization.yaml`. diff --git a/.agents/skills/improving-skills/SKILL.md b/.agents/skills/improving-skills/SKILL.md index a1f8685..6d07e9a 100644 --- a/.agents/skills/improving-skills/SKILL.md +++ b/.agents/skills/improving-skills/SKILL.md @@ -1,6 +1,6 @@ --- name: improving-skills -description: Collect feedback after using a project skill from .agents/skills/, when a used skill is unclear, incomplete, or incorrect, or when the user requests a skill review. Propose or apply scoped improvements grounded in the task just completed. +description: Review and improve skills and AGENTS.md instructions using task feedback. Use after a project skill or AGENTS.md feedback hook, when instructions caused an issue, or when the user requests a review or improvement. Keep changes scoped and grounded in observed work. --- > **After completing tasks with this skill:** Invoke `improving-skills` to capture feedback and lessons learned. @@ -8,34 +8,43 @@ description: Collect feedback after using a project skill from .agents/skills/, # Improving Skills -Use lessons from completed work to improve skill instructions and supporting resources. Keep feedback concrete and -proportional to the task, preserving guidance that worked well. +Use lessons from completed work to improve skills, `AGENTS.md` instructions, and supporting resources. Keep feedback +concrete and proportional to the task, preserving guidance that worked well. ## When This Applies - After completing a task that used a skill from the current project's `.agents/skills/` directory. -- When a skill used during the task was unclear, incomplete, or incorrect. -- When the user explicitly requests feedback on a skill, including one stored outside the repository. +- When an applicable `AGENTS.md` self-improvement section calls for feedback after a task. +- When a skill or `AGENTS.md` instruction used during the task was unclear, incomplete, or incorrect. +- When the user explicitly requests review or improvement of a skill or `AGENTS.md`, including one outside the repository. Resolve skills to their actual locations and read applicable `AGENTS.md` instructions. Follow references relevant to the observed issue. Do not assume that skills from another repository or skill framework are installed here. +## Documentation Guidance + +When improving skills or `AGENTS.md` files, use [create-readme](../create-readme/SKILL.md) for writing and verification +guidance and its [agent-facing documentation reference](../create-readme/references/agent-facing-documentation.md) +for audience, concise primary documents, selective references, and reuse of existing sources. Read only the supporting +topics relevant to the improvement. Using this guidance belongs to the current feedback pass; it does not start another +pass through the skills' completion hooks. + ## Workflow 1. Finish the user's task and any urgent follow-up first. Capture feedback while the evidence is fresh and include it - in the task's final response. -2. Review the skills used and the decisions or workarounds they caused. Identify missing guidance, unclear instructions, - incorrect assumptions, and useful guidance to retain. Ground findings in the completed work and verify proposed paths, - commands, and behavior against the destination repository or available tools. -3. Check whether the reviewed skills include the feedback hook below. Recommend it for every project skill, including - this one. Keep edits within the authorized scope; a missing hook elsewhere is a recommendation, not a reason to edit - every skill in the repository. + in the task's final response. An explicitly requested documentation improvement is itself the task to complete. +2. Review the skills and applicable `AGENTS.md` instructions used and the decisions or workarounds they caused. Identify + missing guidance, unclear instructions, incorrect assumptions, and useful guidance to retain. Ground findings in the + completed work and verify proposed paths, commands, and behavior against the destination repository or available tools. +3. Check the reviewed documents for the appropriate feedback hook below. Recommend it for project skills and + `AGENTS.md` files. Keep edits within the authorized scope; a missing hook elsewhere is a recommendation, not a reason + to edit every instruction file in the repository. 4. Apply improvements already covered by the user's request or earlier authorization without asking again. Otherwise, prepare a concrete proposal before requesting approval for substantive edits outside that scope. Feedback collection - alone does not authorize changes to unrelated skills or externally managed bundles. -5. Update affected supporting resources together with `SKILL.md`, preserving unrelated edits and invocation policy. - Check frontmatter, references, and consistency across the changed bundle. Run focused checks for changed executable - helpers; distinguish checks actually run from behavior verified by inspection. + alone does not authorize changes to unrelated instruction files or externally managed bundles. +5. Update affected supporting resources together with `SKILL.md` or `AGENTS.md`, preserving unrelated edits and invocation + policy. Check frontmatter, references, and consistency across the changed bundle. Run focused checks for changed + executable helpers; distinguish checks actually run from behavior verified by inspection. 6. Report meaningful changes, remaining proposals, and validation briefly. If no change is warranted, mention what worked well without inventing an improvement. Include one review of `improving-skills` itself in this pass. That review satisfies its own feedback hook; finish without starting another feedback cycle solely because this skill ran. @@ -53,11 +62,29 @@ Preserve equivalent existing instructions rather than duplicating them. When cre skill within the user's requested scope, add the hook if missing. For `improving-skills`, keep the single-pass self-review qualification shown above so the hook terminates. +## Self-Improvement Section for AGENTS.md + +When creating or updating an `AGENTS.md` file within the user's requested scope, add a self-improvement section if no +equivalent instruction already applies. Use the following pattern, adjusting the link relative to that `AGENTS.md`: + +```markdown +## Self-Improvement + +After completing a task governed by this file, use +[improving-skills](.agents/skills/improving-skills/SKILL.md) to review the skills and AGENTS.md instructions used and +capture concrete feedback. Apply improvements within the authorized scope; propose changes outside it. Combine feedback +into one pass, including improving-skills' self-review, without recursively invoking completion hooks. +``` + +The example path is for a repository-root `AGENTS.md` with this project skill installed. Verify the actual location +before adding the link; do not create a broken dependency when the skill is unavailable. Preserve equivalent inherited +guidance without duplicating it in nested files. A missing section outside the requested scope is a recommendation. + ## Useful Feedback -For each material finding, give the skill name and file, the observed issue or successful guidance, the proposed or applied -change, and a brief rationale. Quote existing text only when needed to make the change understandable. A short paragraph -or small diff is usually enough; combine related findings. +For each material finding, identify the skill or `AGENTS.md` file, the observed issue or successful guidance, the proposed +or applied change, and a brief rationale. Quote existing text only when needed to make the change understandable. A short +paragraph or small diff is usually enough; combine related findings. - Report concrete gaps, contradictory instructions, stale dependencies, and verified path or command errors. - Keep guidance that helped the task, especially constraints tied to an observed failure mode. From bc2a861d36f58567a5089025db374502cbf7117d Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 14:12:50 -0700 Subject: [PATCH 07/19] docs: README.md update --- README.md | 450 ++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 320 insertions(+), 130 deletions(-) diff --git a/README.md b/README.md index 2ffdda2..b6437a6 100644 --- a/README.md +++ b/README.md @@ -1,139 +1,120 @@ # Sinker -Sinker is a Kubernetes controller that keeps resources in sync across clusters. It watches `ResourceSync` custom -resources (CRs), reads a source object, then projects it onto a target object—optionally in a different cluster—while -preserving the fields you care about. Sinker keeps watching both ends so that drifts introduced by other actors are -reconciled automatically. +Sinker is a Kubernetes controller that copies resources, or selected fields, from a source object to a target object. +Each `ResourceSync` defines a one-way synchronization within one cluster or between clusters. Sinker watches both +objects and reapplies the desired target when they change. ## Features -- Cross-cluster or in-cluster synchronization for any Kubernetes API resource using a single declarative CR. -- JSONPath-based field mappings that let you clone entire objects or copy specific subtrees into the target. -- Safe lifecycle management through finalizers plus explicit per-resource annotations to control deletion behavior - during outages. -- Remote watchers that observe both source and target objects and trigger reconciliations whenever something changes. -- Admin HTTP server exposing readiness and liveness probes (and, via the `kubert` runtime, metrics) so the controller - integrates cleanly with cluster operations tooling. +- Copy whole resources or select fields with JSONPath source expressions and dotted destination paths. +- Connect either end to another cluster using a kubeconfig stored in a Kubernetes Secret. +- Share kubeconfig Secrets across namespaces with an explicit namespace access annotation. +- Store arbitrary structured data in `SinkerContainer` resources for use as sources or targets. +- Manage target cleanup through a finalizer and report reconciliation results in `ResourceSync` status. -## Getting Started +## Getting started ### Prerequisites -- Rust toolchain 1.85 or later (see `rust-toolchain.toml`) if you plan to build from source. -- Docker or another OCI-compatible builder to produce a controller image. -- Access to at least one Kubernetes cluster (1.33+) with `kubectl` and the ability to create CRDs, roles, and service - accounts. -- Optional: access credentials for any remote clusters you want Sinker to read from or write to. These must be stored as - Kubernetes secrets containing kubeconfigs such as those created by [CAPI](https://cluster-api.sigs.k8s.io/). +- A Kubernetes cluster for the controller and its CRDs, with permission to install CRDs, RBAC, and a Deployment. + The bundled schema uses CEL validation, and target writes use server-side apply. The repository selects Kubernetes + 1.33 API bindings at build time; this is not a tested minimum cluster version. +- `kubectl` with Kustomize support. +- The Rust toolchain selected by [rust-toolchain.toml](rust-toolchain.toml), currently `1.85.0`, when building locally. +- An OCI image builder and a registry accessible to the cluster when building your own container image. +- For remote clusters, network access from the controller and a usable kubeconfig for each connection. ### Build the controller ```bash git clone https://github.com/influxdata/sinker.git cd sinker -cargo build --release +cargo build --locked --release ``` -The compiled binary lives at `target/release/sinker`. To build a container image: +The binary is `target/release/sinker`. To build and publish your own image, replace the registry and tag below: ```bash -docker build -t //sinker: . -docker push //sinker: +docker build -t /sinker: . +docker push /sinker: ``` +The [Dockerfile](Dockerfile) uses cargo-chef for build caching and a distroless Debian 12 runtime running as UID/GID +`65532:65532`. CI also publishes images; see [Development](#development) for the publication workflow. + ### Deploy to Kubernetes -1. **Create an image pull secret** (if needed) in the namespace where Sinker will run, for example: - ```bash - kubectl create secret docker-registry gar-auth-sinker \ - --docker-server= \ - --docker-username= \ - --docker-password= \ - --namespace sinker - ``` -2. **Update the controller image** in `manifests/deployment.yml` (or overlay with Kustomize) to point at the image you - pushed. -3. **Apply the bundled manifests** (CRDs, RBAC, Deployment, ServiceAccount, etc.): - ```bash - kubectl apply -k manifests - ``` -4. **Verify deployment**: - ```bash - kubectl -n sinker get pods - kubectl -n sinker logs deploy/sinker - ``` -5. When the pod is ready, the controller listens on port `8080` for `/live`, `/ready`, and `/metrics` (Prometheus - format) endpoints exposed by the admin server. +The [bundled manifests](manifests/kustomization.yaml) install both CRDs, a namespace, RBAC, a ServiceAccount, and a +single-replica Deployment. Customize [deployment.yml](manifests/deployment.yml), directly or through an overlay: -### Running locally against a cluster +- Replace the `sinker:replace_me` image with an image you can pull. +- Configure the `gar-auth-sinker` image pull Secret, or remove/change `imagePullSecrets` for your registry. +- Adjust resource requests and limits: the defaults request **2 CPUs and 3G memory**, with an **8 CPU** limit. +- Extend [clusterrole.yml](manifests/clusterrole.yml) if you need additional resource kinds; see [Permissions](#permissions). -You can also run the controller directly from your workstation: +If the registry requires a pull Secret, create the namespace first: ```bash -cargo run -- --kubeconfig /path/to/kubeconfig --context my-context +kubectl apply -f manifests/namespace.yaml +kubectl -n sinker create secret docker-registry gar-auth-sinker \ + --docker-server= \ + --docker-username= \ + --docker-password= ``` -Key CLI flags: +After customizing the manifests, render, apply, and check the Deployment: -- `--log-level` (or `SINKER_LOG`) controls tracing filters (default `sinker=info,warn`). -- `--log-format` selects `plain` or `json`. -- `--kubeconfig`, `--context`, `--cluster`, and `--user` mirror the standard `kubectl` flags for choosing credentials. -- `--as` and `--as-group` let you impersonate another Kubernetes user or group. -- `--kube-api-response-headers-timeout` configures the Kubernetes client timeout (default `9s`). -- `--admin-addr` sets the admin HTTP server bind address (default `0.0.0.0:8080`). +```bash +kubectl kustomize manifests +kubectl apply -k manifests +kubectl -n sinker rollout status deployment/sinker +kubectl -n sinker logs deployment/sinker +``` -## Defining resource syncs +Sinker watches `ResourceSync` objects in **all namespaces**. Keep one active controller per cluster: the implementation +has no leader election. The admin server listens on port `8080`; see [Status and observability](#status-and-observability). -Sinker ships two custom resources: +### Running locally against a cluster -### ResourceSync +Install the CRDs in the cluster selected by your kubeconfig, then run: -`ResourceSync` objects describe how to mirror a Kubernetes resource. +```bash +kubectl --kubeconfig /path/to/kubeconfig --context my-context apply -f manifests/crd.yml +kubectl --kubeconfig /path/to/kubeconfig --context my-context wait --for=condition=Established \ + --timeout=60s crd/resourcesyncs.sinker.influxdata.io crd/sinkercontainers.sinker.influxdata.io +cargo run --locked -- --kubeconfig /path/to/kubeconfig --context my-context +``` -| Field | Required | Description | -|---------------------------|----------|-------------------------------------------------------------------------------------------------------------------------------------| -| `spec.source.resourceRef` | ✓ | API reference (`apiVersion`, `kind`, `name`) pointing to the object you want to copy. | -| `spec.source.cluster` | | Optional remote cluster reference. When omitted, Sinker reads the source from the same cluster and namespace as the `ResourceSync`. | -| `spec.target.resourceRef` | ✓ | API reference for the target object. | -| `spec.target.cluster` | | Optional remote cluster reference. When omitted, the target lives in the same cluster as the `ResourceSync`. | -| `spec.mappings[]` | | Optional list of field mapping rules. See below. | +Use a cluster without another active Sinker controller. Local execution needs the same API permissions as a deployed +controller. Credentials are loaded from `--kubeconfig`, then `KUBECONFIG` or `~/.kube/config`; without explicit client +selection, a failed local configuration falls back to in-cluster credentials. -#### Cluster references +Run `cargo run --locked -- --help` for the full CLI. These options configure the controller's local connection and runtime: -When you provide `spec.{source|target}.cluster`, Sinker reads a kubeconfig from a secret to talk to the remote cluster: +| Option | Purpose / default | +| --- | --- | +| `--log-level` / `SINKER_LOG` | Tracing filter; `sinker=info,warn`. | +| `--log-format` | `plain` or `json`; defaults to `plain`. | +| `--kubeconfig`, `--context`, `--cluster`, `--user` | Select the local kubeconfig and its entries. | +| `--as`, `--as-group` | Set kubeconfig user/group impersonation. | +| `--kube-api-response-headers-timeout` | Local Kubernetes client response-header timeout; `9s`. | +| `--admin-addr` | Admin HTTP bind address; `0.0.0.0:8080`. | -```yaml -cluster: - namespace: other-namespace # optional override; defaults to the Remote kubeconfig's namespace - kubeConfig: - secretRef: - name: remote-kubeconfig - key: value -``` +Remote clients use the kubeconfigs referenced in each `ResourceSync`; local client flags do not override those connections. -Create the secret by embedding a standard kubeconfig: +## Defining resource syncs -```bash -kubectl -n create secret generic remote-kubeconfig \ - --from-file=value=/path/to/kubeconfig -``` +Both CRDs use `apiVersion: sinker.influxdata.io/v1alpha1` and are namespaced. -Within the remote cluster, RBAC for the kubeconfig user limits what Sinker can access. For local clusters, Sinker uses -its in-cluster credentials. +### ResourceSync -#### Mappings +For a simple in-cluster sync, create a source ConfigMap: -- When `spec.mappings` is empty, Sinker clones the entire source object, copying annotations and labels while cleaning - the `kubectl.kubernetes.io/last-applied-configuration` annotation. -- When mappings are supplied, each mapping entry has `fromFieldPath` and/or `toFieldPath`: - - `fromFieldPath` is a JSONPath evaluated against the source. Use `spec.data.someField` to copy a nested field, or - leave it blank/omit it to select the entire object. - - `toFieldPath` is a JSONPath-like dotted path inside the target (`metadata.*` and `spec|status|data` are - supported). Omit `toFieldPath` to replace the entire target with the selected subtree. -- Sinker enforces that at least one of the fields is present per mapping. If `toFieldPath` targets `metadata`, the - controller keeps metadata in sync using server-side apply. +```bash +kubectl -n default create configmap sinker-source --from-literal=message=hello +``` -Example `ResourceSync`: +Save this as `resource-sync.yaml`, then run `kubectl apply -f resource-sync.yaml`: ```yaml apiVersion: sinker.influxdata.io/v1alpha1 @@ -146,64 +127,273 @@ spec: resourceRef: apiVersion: v1 kind: ConfigMap - name: remote-demo - cluster: - namespace: default - kubeConfig: - secretRef: - name: k3-test-27-kubeconfig - key: value + name: sinker-source target: resourceRef: apiVersion: v1 kind: ConfigMap - name: demo - mappings: - - fromFieldPath: data.remote - toFieldPath: data.remote - - fromFieldPath: data.foo - toFieldPath: data.bar + name: sinker-target +``` + +Inspect the result with `kubectl -n default get configmap sinker-target -o yaml` and +`kubectl -n default get resourcesync demo -o yaml`. Changes to the source are copied to the target; external changes to +fields Sinker manages on the target trigger another apply. Deleting the source causes reconciliation errors and leaves +an existing target in place. Deleting the `ResourceSync` initiates [target cleanup](#annotations-and-finalizers). + +| Field | Required | Meaning | +| --- | --- | --- | +| `spec.source.resourceRef` | Yes | Source `apiVersion`, `kind`, and `name`. | +| `spec.target.resourceRef` | Yes | Target `apiVersion`, `kind`, and `name`. | +| `spec.source.cluster` | No | Source kubeconfig reference and optional resource namespace override. | +| `spec.target.cluster` | No | Target kubeconfig reference and optional resource namespace override. | +| `spec.mappings` | No | Ordered field mappings. Omitted or `[]` copies the source's content, labels, and annotations. | + +**The checked-in CRD makes the entire `spec` immutable**, including mappings. Replace the `ResourceSync` to change its +spec, accounting for target deletion when removing the old sync. This restriction is enforced by the installed schema; +the generator currently omits the rule. See [Generating CRDs](#generating-crds) before updating or packaging schemas. + +### Cluster references and namespaces + +Without `cluster`, a namespaced source or target is resolved in the `ResourceSync`'s namespace using the controller's +local client. `resourceRef` has no namespace field. To use a kubeconfig, add this under either `source` or `target`: + +```yaml +cluster: + namespace: workloads + kubeConfig: + secretRef: + name: remote-kubeconfig + namespace: cluster-credentials + key: value ``` +The two namespace fields have different meanings: + +| Field | Location | Default when omitted | +| --- | --- | --- | +| `cluster.kubeConfig.secretRef.namespace` | Namespace of the Secret in the **controller's cluster**. | `ResourceSync` namespace. | +| `cluster.namespace` | Namespace of the source/target in the **referenced cluster**. | Selected kubeconfig context's namespace, or `default`. | + +Discovery determines whether a kind is namespaced or cluster-scoped. Cluster-scoped API requests use no resource +namespace; see the [local target ownership limitation](#annotations-and-finalizers) before using such a target. + +Create the kubeconfig Secret in an existing namespace. This example authorizes syncs in `default` to use credentials +stored in `cluster-credentials`: + +```bash +kubectl -n cluster-credentials create secret generic remote-kubeconfig \ + --from-file=value=/path/to/remote-kubeconfig +kubectl -n cluster-credentials annotate secret remote-kubeconfig \ + 'sinker.influxdata.io/allowed-namespaces=^default$' +``` + +The `sinker.influxdata.io/allowed-namespaces` annotation belongs to the **kubeconfig Secret**. Its value is a Rust regular +expression matched against the **ResourceSync namespace**. Use anchors for exact matches, for example +`^(team-a|team-b)$`; matching is otherwise not anchored. Cross-namespace access is denied when the annotation is absent, +invalid, or does not match. Same-namespace Secret references do not require this annotation. A failed cross-namespace +Secret read also reports the generic namespace-restriction error. + +The Secret key must contain a UTF-8 kubeconfig with a usable current context. Any referenced files or credential helper +executables must be available inside the controller container; the supplied distroless image includes no cloud CLI +helpers. A self-contained kubeconfig avoids dependencies on workstation files. + +[example.yaml](example.yaml) shows remote-to-local field mapping. It requires the Secret `k3-test-27-kubeconfig` in +`default` and a remote `ConfigMap/default/remote-demo` containing `data.remote` and `data.foo`. + +### Permissions + +Sinker uses the controller's credentials for local resources and kubeconfig Secret reads, and the referenced kubeconfig's +identity for remote resources. It does not impersonate the creator of a `ResourceSync`. Grant the ability to create syncs +and authorize shared credentials according to the resources those identities can access. + +The bundled ClusterRole supplies access to `ResourceSync`, `SinkerContainer`, ConfigMaps, and Secrets, plus read access to +Namespaces. Dynamic resource discovery does not grant access to other kinds: add RBAC for them on the relevant cluster. +Source operations require `get` and `watch`; targets require `get`, `watch`, `patch` for server-side apply, and `delete` +for cleanup. API discovery and remote server-version requests must also be allowed. The controller additionally needs +cluster-wide list/watch access to `ResourceSync`, status updates, and finalizer management, as provided by the bundle. + +### Mappings + +With no mappings, Sinker copies the source's non-metadata fields, labels, and annotations into a target with the requested +name and discovered type. It drops `kubectl.kubernetes.io/last-applied-configuration` and does not copy source UIDs, +resource versions, owner references, or finalizers. + +With mappings, Sinker starts with an empty target and processes entries in order: + +| Mapping | Behavior | +| --- | --- | +| Both `fromFieldPath` and `toFieldPath` | Select a source value and place it at the destination. | +| Only `toFieldPath` | Place the entire source object at the destination, such as `spec` in a `SinkerContainer`. | +| Only `fromFieldPath` | Treat the selected subtree as a Kubernetes object and replace the target template with it. The subtree must contain `apiVersion` and `kind` matching the target reference. | +| Neither field | Reconciliation error; use an empty mappings list for a whole-resource copy. The CRD does not reject an empty mapping entry. | + +Source paths are JSONPath expressions **without a leading `$` or `$.`**: Sinker prepends `$.` to nonempty paths. +For example, `data.foo`, `spec.items[0]`, or `metadata.annotations['example.com/key']`. An omitted or empty +`fromFieldPath` selects the whole source. No match produces JSON `null`; multiple matches cause a reconciliation error. + +Destination paths are dot-separated object keys, such as `data.bar` or `metadata.labels`. They create missing objects +but cannot traverse a scalar, index an array, or escape a dot within a key. To copy keys containing dots, map their whole +parent object, such as `data` or `metadata.annotations`. An empty `toFieldPath` is not a root replacement; omit it for that. + +For example, to copy only the demo ConfigMap's `data.message` to `data.copiedMessage`, add this to the sync's initial spec: + +```yaml +mappings: + - fromFieldPath: data.message + toFieldPath: data.copiedMessage +``` + +All target writes use **forced server-side apply** with field manager `sinker.influxdata.io`. Sinker can take ownership of +conflicting fields on an existing target. Other fields follow Kubernetes apply and schema rules; a sync is not an exact +byte-for-byte clone. Sinker writes to the main resource endpoint, so mapping `status.*` does not update a separate +[`/status` subresource](https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#status-subresource). + ### SinkerContainer -`SinkerContainer` is a lightweight CRD that stores arbitrary structured data under `.spec`. Use it as either a source or -target when you want Sinker to materialize generated configuration into a typed CR (for example, to copy an inner spec -from one object into another). +`SinkerContainer` holds an arbitrary JSON object under `.spec`, with unknown fields preserved by its manual schema. It +has no separate reconciliation loop. Use it to collect selected data or hold an embedded Kubernetes object: + +```yaml +apiVersion: sinker.influxdata.io/v1alpha1 +kind: SinkerContainer +metadata: + name: config-template + namespace: default +spec: + apiVersion: v1 + kind: ConfigMap + data: + message: hello +``` + +To materialize this embedded ConfigMap, reference `config-template` as the source, a `v1` ConfigMap as the target, and +use `mappings: [{fromFieldPath: spec}]`. Conversely, `mappings: [{toFieldPath: spec}]` stores an entire source object in +a target `SinkerContainer`. ## Annotations and finalizers -Sinker adds a finalizer (`sinker.influxdata.io/target`) to each `ResourceSync` so it can clean up target objects when -the CR is deleted. Two optional annotations modify that behavior: +Sinker adds `sinker.influxdata.io/target` to the `ResourceSync` before applying the target. On deletion, it requests target +deletion and waits until the target is absent before stopping the watches and removing its finalizer. It uses background +deletion when the target already has finalizers, and foreground deletion otherwise. A target's own finalizers can keep +cleanup pending. Existing targets adopted by a sync are subject to the same cleanup. + +These annotations belong to the **ResourceSync** and are enabled only by the string `"true"`: + +| Annotation | Effect | +| --- | --- | +| `sinker.influxdata.io/disable-target-deletion: "true"` | Skip Sinker issuing the target delete request, stop watches, and remove Sinker's finalizer. | +| `sinker.influxdata.io/force-delete: "true"` | During deletion, remove Sinker's finalizer if constructing either source or target API fails, for example because credentials or a remote API are unavailable. It does not bypass errors from subsequent target GET/DELETE requests or a target's finalizers. | -- `sinker.influxdata.io/force-delete: "true"` – if set and the controller cannot contact a remote cluster while - deleting, Sinker removes its finalizer so the `ResourceSync` can be garbage-collected. -- `sinker.influxdata.io/disable-target-deletion: "true"` – skip deleting the target object when the `ResourceSync` is - removed. Useful when you want to manage the target lifecycle manually. +Both source and target APIs are resolved before normal cleanup, even with `disable-target-deletion`. An unavailable +source cluster can therefore block cleanup too. Force deletion can leave a target behind for manual cleanup. + +Local targets (`target.cluster` omitted) also receive an owner reference to the namespaced `ResourceSync`. +**Disabling target deletion alone does not retain a local target**: Kubernetes garbage collection can still delete it. +Retention must also account for Kubernetes [orphaning behavior](https://kubernetes.io/docs/concepts/architecture/garbage-collection/#orphaned-dependents). +The controller currently adds this owner reference even to a local cluster-scoped target, which conflicts with Kubernetes +[owner scope rules](https://kubernetes.io/docs/concepts/architecture/garbage-collection/#owners-and-dependents). +Targets configured through a kubeconfig receive no Sinker owner reference, even if that kubeconfig points at the local cluster. ## Status and observability -- Sinker publishes a `ResourceSyncFailing` condition in `.status.conditions[]` with timestamps, reasons, and messages if - reconciliation fails. -- Logs use the standard `tracing` crate; adjust verbosity with `SINKER_LOG`. -- The admin server exposes `/live`, `/ready`, and `/metrics` on port 8080. Wire these into your cluster’s probes and - monitoring. -- Remote watchers reconcile whenever the source or target resource changes, even on remote clusters, which keeps - synchronized objects fresh with minimal polling. +`status.conditions` contains `ResourceSyncFailing`: `"True"` with an error message after failure, or `"False"` with reason +`ResourceSyncSucceeded` after successful reconciliation. It records `observedGeneration` and preserves `lastTransitionTime` +while the condition value stays the same, using the live status rather than the watch cache. A successful finalizer-only +initialization also reports `"False"`, so inspect the target when verifying the first sync. Successful deletion cleanup +does not write status. + +```bash +kubectl get resourcesyncs -A +kubectl -n default get resourcesync demo -o yaml +kubectl -n sinker logs deployment/sinker --tail=100 +kubectl -n sinker port-forward deployment/sinker 8080:8080 +# In another terminal: +curl http://localhost:8080/live +curl http://localhost:8080/ready +``` + +The admin server provides `/live` and `/ready`. Readiness reflects runtime initialization and shutdown, not the health +of individual syncs or remote clusters. **There is currently no registered `/metrics` endpoint**; enabling the dependency's +Prometheus feature does not configure an exporter in [main.rs](src/main.rs). + +Reconciliation errors retry after five seconds; object watches reconnect with backoff. A healthy sync waits for events +rather than polling on a fixed interval. Metadata-only edits to `ResourceSync` and changes to kubeconfig Secrets are not +explicit reconciliation triggers. Existing watches keep their clients until they reconnect. A retry or subsequent object +event can pick up updated settings; restart the controller when credential or access changes need to take effect promptly. + +For failures, check the condition message, referenced object names, resource and Secret namespaces, the Secret's selected +key and access annotation, RBAC, and connectivity to both API servers. If startup logs say the CRD is not queryable, +check CRD installation and list permissions. Debug logging includes source objects and mapped values, which can include +Secret contents. + +## How it works + +```mermaid +flowchart TD + RS[ResourceSync watch] --> R[Reconcile] + K[Kubeconfig Secrets in controller cluster] --> C[Resolve clients and discover resource APIs] + R --> C + C --> S[Read source] + S --> M[Clone content or apply mappings] + M --> T[Server-side apply target] + T --> W[Watch source and target] + W -->|External changes, deletion, or watch errors| R + R --> ST[Write ResourceSync condition] + C -->|ResourceSync is deleting| D[Clean up target and remove finalizer] +``` + +The controller filters `ResourceSync` events by generation to avoid loops from its own status writes. The watcher manager +keeps a watcher per resource reference and owning sync, for local and remote endpoints. Object watches use +`metadata.name` selectors and managed-field timestamps to suppress changes last attributed to `sinker.influxdata.io`; +events with unknown ownership trigger reconciliation. Shutdown cancels and joins the watchers. -## Generating CRDs programmatically +## Generating CRDs -To print the latest CRDs (e.g., for Helm packaging), use the built-in command: +`sinker manifests` emits only the two CRDs; it needs no cluster connection. Compare generated output in a temporary file +before replacing the checked-in schema: ```bash -cargo run -- manifests > out.yml +cargo run --locked -- manifests > /tmp/sinker-crds.yaml +diff -u manifests/crd.yml /tmp/sinker-crds.yaml ``` -The output includes both `ResourceSync` and `SinkerContainer` definitions. +**Known drift:** [manifests/crd.yml](manifests/crd.yml) includes the `self == oldSelf` validation on `ResourceSync.spec`; +[resources.rs](src/resources.rs) does not generate it. The comparison currently reports that rule missing. Replacing the +checked-in file with generated output would remove spec immutability. CI regenerates `manifests/crd.yml` and checks for a +clean diff, so this mismatch also affects the generation check. Preserve the rule when packaging until its generation is +reconciled with the manifest. -## Development notes +Use `kubectl kustomize manifests` to render the complete deployment bundle. + +## Development + +Sinker is a single Rust package with a binary and library. The [CI workflow](.github/workflows/rust.yml) builds, checks +formatting and CRD drift, runs tests, and runs Clippy. Equivalent local checks, with formatting kept read-only, are: + +```bash +cargo build --locked +cargo fmt --check +cargo test --locked +cargo clippy --locked --all-targets --all-features +``` -- Format and lint with `cargo fmt` and `cargo clippy --all-targets --all-features`. -- Run tests with `cargo test`. -- When developing new features, update `manifests` or regenerate CRDs via `sinker manifests` before deploying. -- The project uses MIT licensing; see `LICENSE` for details. +Tests are inline unit tests for mappings, status transitions, annotations, namespace access checks, and event filtering. +They do not establish end-to-end behavior against a live Kubernetes API. Use the [CRD comparison](#generating-crds) when +changing resource definitions. + +| Source | Read when changing | +| --- | --- | +| [main.rs](src/main.rs), [lib.rs](src/lib.rs) | CLI, runtime wiring, public modules, and shared errors. | +| [resources.rs](src/resources.rs) | Serialized API fields, annotations, and the manual `SinkerContainer` schema. | +| [controller.rs](src/controller.rs) | Reconciliation, server-side apply, status, ownership, and cleanup. | +| [resource_extensions.rs](src/resource_extensions.rs) | Client creation, namespace resolution, discovery, and kubeconfig Secret authorization. | +| [mapping.rs](src/mapping.rs) | Source selection, destination construction, and metadata handling. | +| [remote_watcher.rs](src/remote_watcher.rs), [remote_watcher_manager.rs](src/remote_watcher_manager.rs), [filters.rs](src/filters.rs) | Watch lifecycle, retries, and filtering of Sinker-generated events. | +| [manifests/](manifests/), [Dockerfile](Dockerfile), [rust.yml](.github/workflows/rust.yml) | Deployment defaults, RBAC, packaging, and publication. | + +On successful pushes to `main`, CI builds `linux/amd64` and `linux/arm64` images at +`us-docker.pkg.dev/influxdb2-artifacts/tubernetes/sinker:`, updates the manifest image for publication, and +publishes a Flux OCI artifact under `sinker-manifests` in the same registry. This workflow requires the configured +registry credentials and Depot project; building your own image does not require access to that infrastructure. + +Sinker is licensed under the [MIT License](LICENSE). From 7d0ae2c4924128ec01a8206cbca223da7568bb74 Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 14:18:42 -0700 Subject: [PATCH 08/19] docs: AGENTS.md --- AGENTS.md | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..0956be2 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,107 @@ +# Working on Sinker + +This file applies throughout the repository. Sinker is a single Rust package with a binary and library, implementing +one-way Kubernetes resource synchronization within or between clusters. `ResourceSync` drives reconciliation; +`SinkerContainer` supplies an arbitrary-object schema and has no separate controller. + +Start with [README.md](README.md) for project context. Use its [source map](README.md#development) to locate the relevant +modules and its [control-flow diagram](README.md#how-it-works) when changing reconciliation or watches. Read the linked +task-specific sections below as needed. + +## Development and verification + +Use the toolchain in [rust-toolchain.toml](rust-toolchain.toml) and the dependency versions resolved in +[Cargo.lock](Cargo.lock). [Cargo.toml](Cargo.toml) and [rustfmt.toml](rustfmt.toml) select Rust 2021. When changing the +toolchain or dependencies, also check the Rust builder image in [Dockerfile](Dockerfile). The `k8s-openapi` feature +selects build-time API bindings; it does not establish a tested minimum Kubernetes server version. + +Run these checks from the repository root for Rust changes, matching [CI](.github/workflows/rust.yml) while keeping +format validation read-only and dependency resolution locked: + +```bash +cargo build --locked +cargo fmt --check +cargo test --locked +cargo clippy --locked --all-targets --all-features +``` + +Tests live in inline `#[cfg(test)]` modules and use ordinary Rust tests, `rstest`, and Tokio tests. Add focused regression +coverage for behavior changes beside the affected implementation. Cargo test filters match test names, not file paths; +use `cargo test --locked -- --list` to check selection. These tests exercise local logic, not live Kubernetes +reconciliation, authorization, garbage collection, or server-side apply. + +For API/schema changes, also perform the CRD comparison described below. For deployment changes, render with +`kubectl kustomize manifests`. For documentation-only changes, verify claims, relative links, and section anchors; +execute examples only when needed to substantiate them. Report checks actually run and distinguish existing failures +from regressions introduced by the change. + +## Implementation constraints + +- **API and serialization:** Update [resources.rs](src/resources.rs), [manifests/crd.yml](manifests/crd.yml), and affected + [README examples](README.md#defining-resource-syncs) together when changing the public API. Serialized fields use + `camelCase`. `SinkerContainer` uses `crd_with_manual_schema()` to preserve arbitrary fields under `.spec`; its empty + Rust spec type does not describe the stored payload. Check schema validation separately from runtime validation. +- **Mappings:** In [mapping.rs](src/mapping.rs), source selection uses JSONPath, while destination construction uses + dotted object keys. Preserve the distinction between an empty mappings list (whole-resource copy) and an empty + mapping entry (error), ordered mappings, and special handling of `DynamicObject` metadata. Read + [Mappings](README.md#mappings) before changing path parsing, subtree replacement, or cloning behavior. +- **Client and namespace resolution:** In [resource_extensions.rs](src/resource_extensions.rs), kubeconfig Secrets are + read from the controller's cluster. Their namespace is distinct from the source/target resource namespace. Preserve + cross-namespace Secret authorization through `sinker.influxdata.io/allowed-namespaces`: absent, invalid, or + nonmatching expressions deny access. Same-namespace references bypass this annotation check. Read + [namespace resolution](README.md#cluster-references-and-namespaces) and [permissions](README.md#permissions) when + changing this path; local operations use the controller's identity, not the `ResourceSync` creator's permissions. +- **Target writes and event filtering:** [controller.rs](src/controller.rs) uses forced server-side apply with field + manager `sinker.influxdata.io`. [filters.rs](src/filters.rs) and [remote_watcher.rs](src/remote_watcher.rs) use that same + manager to suppress self-generated events; unknown ownership triggers reconciliation. Review these paths together + when changing field ownership or event handling. +- **Watch lifecycle:** [remote_watcher_manager.rs](src/remote_watcher_manager.rs) keys watches by resource reference + and owning sync, for both local and remote resources. Preserve watcher cancellation and joining during cleanup and + shutdown. The main `ResourceSync` stream filters by generation; metadata-only edits and kubeconfig Secret changes + are not explicit triggers. Read [event and retry behavior](README.md#status-and-observability) before changing + configuration refresh or reconciliation scheduling. +- **Status:** In [controller.rs](src/controller.rs), compute `ResourceSyncFailing` from live status, not the reflector + cache. Success clears failure, unchanged condition values retain `lastTransitionTime`, and successful deletion + cleanup skips the status write because the object may already be gone. Preserve the regression coverage for these + transitions when changing reconciliation results or status updates. +- **Deletion and ownership:** Read [Annotations and finalizers](README.md#annotations-and-finalizers) before changing + cleanup. Preserve unrelated finalizers. Normal cleanup waits for target absence before stopping watches and removing + Sinker's finalizer. Both APIs are resolved before cleanup; `force-delete` only bypasses failures at that resolution + step. Local targets also have an owner reference, so `disable-target-deletion` alone does not retain them. Account for + the documented local cluster-scoped ownership limitation when changing target scope or retention behavior. + +## CRD generation + +Follow [Generating CRDs](README.md#generating-crds): generate into a temporary file and compare before replacing +[manifests/crd.yml](manifests/crd.yml). The `manifests` subcommand emits only the two CRDs and needs no cluster connection; +Kustomize renders the complete deployment bundle. + +**Known drift:** the checked-in `ResourceSync.spec` schema contains `self == oldSelf` validation, but the generator in +[resources.rs](src/resources.rs) omits it. Blind regeneration removes spec immutability and the current CI generation +check detects this mismatch. Preserve that rule unless changing immutability is part of the task. Report the existing +drift rather than regenerating tracked artifacts during an unrelated change. + +## Runtime and deployment changes + +Use [main.rs](src/main.rs) for runtime wiring and `cargo run --locked -- --help` to verify CLI flags; client and admin +arguments come from `kubert`. Consult [observability](README.md#status-and-observability) when changing admin endpoints +or logging. Source objects and mapped values can contain Secret data; avoid adding payloads to routine logs. + +For live testing, follow [Running locally against a cluster](README.md#running-locally-against-a-cluster) with an explicit +kubeconfig/context and a cluster without another active Sinker controller. Running the binary without a subcommand +starts reconciliation. The controller watches all namespaces and has no leader election. + +When changing supported operations or kinds, review [RBAC](manifests/clusterrole.yml) alongside the code; discovery does +not grant permissions. For packaging, review [deployment defaults](README.md#deploy-to-kubernetes), +[Dockerfile](Dockerfile), and the [publication workflow](.github/workflows/rust.yml). The bundled image tag and pull +Secret require environment-specific configuration; [example.yaml](example.yaml) also requires external resources. + +## Documentation and self-improvement + +Use [create-readme](.agents/skills/create-readme/SKILL.md) for README and agent-documentation changes. Keep this file +focused on actionable project guidance and link to existing README sections for detailed usage and operations. + +After completing a task governed by this file, use +[improving-skills](.agents/skills/improving-skills/SKILL.md) to review the skills and AGENTS.md instructions used and +capture concrete feedback. Apply improvements within the authorized scope; propose changes outside it. Combine feedback +into one pass, including improving-skills' self-review, without recursively invoking completion hooks. From bd1fe5062b9e9e94e4da0f0abc39e5feca62009e Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 14:19:44 -0700 Subject: [PATCH 09/19] docs: rust-unit-tests --- .agents/skills/rust-unit-tests/SKILL.md | 143 ++++++++++++++++++++++++ 1 file changed, 143 insertions(+) create mode 100644 .agents/skills/rust-unit-tests/SKILL.md diff --git a/.agents/skills/rust-unit-tests/SKILL.md b/.agents/skills/rust-unit-tests/SKILL.md new file mode 100644 index 0000000..edd69e9 --- /dev/null +++ b/.agents/skills/rust-unit-tests/SKILL.md @@ -0,0 +1,143 @@ +--- +name: rust-unit-tests +description: write comprehensive rust unit tests for a user-specified file, module, function, or code path. use when the user asks to add, improve, review, or generate rust tests, especially for table-driven testing with rstest, branch coverage, tokio async tests, randomized fixtures, temporary filesystem behavior, precise assertions, and explicit verification of success and error results. +--- + +# Rust Unit Tests + +Write comprehensive Rust tests for the file, module, function, or code path specified by the user. + +## Core workflow + +1. Inspect the target code before writing tests. +2. Identify all public behavior, logical branches, edge cases, parameter combinations, and error paths. +3. Prefer table-driven tests using `rstest` wherever practical. +4. Aim for 100% code coverage as much as reasonably possible. +5. Keep tests readable, focused, deterministic, and DRY. +6. Do not ignore `Result` values, error branches, or cleanup failures. + +## Test style + +Use `rstest` for table-based tests whenever reasonable. + +Prefer named and parameterized cases, for example: + +```rust +#[rstest] +#[case::empty_input("", Expected::Empty)] +#[case::valid_input("abc", Expected::Parsed)] +#[case::invalid_input("!", Expected::Error)] +fn parses_input(#[case] input: &str, #[case] expected: Expected) { + // ... +} +``` + +Use `#[should_panic]` only when the behavior being tested is intentionally panic-based and cannot be more precisely verified with `Result` assertions. + +When table-driven tests need shared randomized fixtures, define those fixtures before constructing the test case table using the `#[fixture]` attribute. + +## Assertions + +Use standard Rust assertions when they are clear and sufficient. + +You may also use `https://github.com/google/assertor` when it improves readability or precision. + +Keep assertions: + +- precise +- focused +- readable +- tied directly to the expected behavior of the scenario + +Avoid broad assertions that only prove the function “does something.” + +## Error handling requirements + +If any function used in a test returns a `Result`, explicitly verify both success and error outcomes for relevant branches. + +Do not ignore errors. + +For expected errors: + +1. Assert the error type when the type is meaningful. +2. Assert the error value or error contents when the value is meaningful. +3. Use `expect_err`, pattern matching, `err.to_string().contains("...")`, or an equivalent assertion helper to verify that the error message includes a substring indicating the cause of the error. + +Do not merely assert that an error exists unless no stronger assertion is possible. + +## Fixtures and randomized values + +For fixtures, use randomized non-`None`, non-empty, and non-default values as much as reasonably possible. + +Randomized values must still produce deterministic and reliable tests. Prefer seeded randomness or helper functions that generate valid randomized values without introducing flakiness. + +Use meaningful defaults only when the specific default value is part of the behavior under test. + +## Async tests + +Use `#[tokio::test]` as much as reasonably possible so tests can run concurrently. + +Do not use concurrent async tests when concurrency would cause issues, such as: + +- shared mutable global state +- shared filesystem paths +- process-wide environment variables +- timing-sensitive behavior +- external services or ports +- tests that intentionally mutate common resources + +In those cases, isolate the state, use serial execution, or use a regular test where appropriate. + +## Filesystem tests + +If a test creates files or directories: + +1. Create them only inside the system temporary directory. +2. Use unique paths for each test case. +3. Attempt cleanup when the test finishes. +4. Verify cleanup errors when cleanup is part of the behavior being tested. +5. Avoid relying on repository-relative paths unless the target code explicitly requires them. + +Prefer temporary directory helpers where available. + +## Coverage expectations + +Ensure tests cover all logical branches as much as reasonably possible, including: + +- valid inputs +- invalid inputs +- empty inputs +- boundary values +- default values +- non-default values +- optional values present and absent +- all relevant combinations of input parameters +- branching paths +- internal conditions that affect observable behavior +- success results +- expected error results +- panic behavior, only when intentional +- filesystem success and failure paths when applicable +- async success, failure, cancellation, or ordering behavior when applicable + +Explicitly test all logical combinations of input parameters, branching paths, internal conditions, and expected outputs as much as reasonably possible. + +## DRYness + +Be DRY as much as reasonably possible. + +Prefer helpers, fixtures, builders, and table-driven cases over repeated setup code. + +Do not over-abstract tests if doing so makes the behavior harder to understand. + +## Output expectations + +When writing tests: + +1. Add or update the appropriate test module or test file. +2. Include any required imports, dev-dependencies, or feature flags. +3. Explain any assumptions made about the code under test. +4. Call out branches that could not reasonably be tested and why. +5. Ensure the resulting tests are idiomatic Rust and should compile in the project context. + +When modifying dependency files, add only the dependencies needed for the tests. From 943b63084628d0ce4c079d20ef3bf4ad4c6e8c3c Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 14:24:29 -0700 Subject: [PATCH 10/19] docs(1): rust-unit-tests --- .agents/skills/rust-unit-tests/SKILL.md | 240 ++++++++++-------------- 1 file changed, 103 insertions(+), 137 deletions(-) diff --git a/.agents/skills/rust-unit-tests/SKILL.md b/.agents/skills/rust-unit-tests/SKILL.md index edd69e9..245370e 100644 --- a/.agents/skills/rust-unit-tests/SKILL.md +++ b/.agents/skills/rust-unit-tests/SKILL.md @@ -1,143 +1,109 @@ --- name: rust-unit-tests -description: write comprehensive rust unit tests for a user-specified file, module, function, or code path. use when the user asks to add, improve, review, or generate rust tests, especially for table-driven testing with rstest, branch coverage, tokio async tests, randomized fixtures, temporary filesystem behavior, precise assertions, and explicit verification of success and error results. +description: Write comprehensive Rust unit tests for a user-specified file, module, function, or code path. Use when the user asks to add, improve, review, or generate Rust tests, including table-driven testing with rstest, branch coverage, Tokio async tests, randomized fixtures, temporary filesystem behavior, and precise success and error assertions. --- -# Rust Unit Tests - -Write comprehensive Rust tests for the file, module, function, or code path specified by the user. - -## Core workflow - -1. Inspect the target code before writing tests. -2. Identify all public behavior, logical branches, edge cases, parameter combinations, and error paths. -3. Prefer table-driven tests using `rstest` wherever practical. -4. Aim for 100% code coverage as much as reasonably possible. -5. Keep tests readable, focused, deterministic, and DRY. -6. Do not ignore `Result` values, error branches, or cleanup failures. - -## Test style - -Use `rstest` for table-based tests whenever reasonable. - -Prefer named and parameterized cases, for example: - -```rust -#[rstest] -#[case::empty_input("", Expected::Empty)] -#[case::valid_input("abc", Expected::Parsed)] -#[case::invalid_input("!", Expected::Error)] -fn parses_input(#[case] input: &str, #[case] expected: Expected) { - // ... -} -``` - -Use `#[should_panic]` only when the behavior being tested is intentionally panic-based and cannot be more precisely verified with `Result` assertions. - -When table-driven tests need shared randomized fixtures, define those fixtures before constructing the test case table using the `#[fixture]` attribute. - -## Assertions - -Use standard Rust assertions when they are clear and sufficient. - -You may also use `https://github.com/google/assertor` when it improves readability or precision. - -Keep assertions: - -- precise -- focused -- readable -- tied directly to the expected behavior of the scenario - -Avoid broad assertions that only prove the function “does something.” - -## Error handling requirements - -If any function used in a test returns a `Result`, explicitly verify both success and error outcomes for relevant branches. - -Do not ignore errors. - -For expected errors: - -1. Assert the error type when the type is meaningful. -2. Assert the error value or error contents when the value is meaningful. -3. Use `expect_err`, pattern matching, `err.to_string().contains("...")`, or an equivalent assertion helper to verify that the error message includes a substring indicating the cause of the error. - -Do not merely assert that an error exists unless no stronger assertion is possible. - -## Fixtures and randomized values +> **After completing tasks with this skill:** Invoke [improving-skills](../improving-skills/SKILL.md) to capture feedback +> and lessons learned. Combine this with the repository's required feedback pass. -For fixtures, use randomized non-`None`, non-empty, and non-default values as much as reasonably possible. - -Randomized values must still produce deterministic and reliable tests. Prefer seeded randomness or helper functions that generate valid randomized values without introducing flakiness. - -Use meaningful defaults only when the specific default value is part of the behavior under test. - -## Async tests - -Use `#[tokio::test]` as much as reasonably possible so tests can run concurrently. - -Do not use concurrent async tests when concurrency would cause issues, such as: - -- shared mutable global state -- shared filesystem paths -- process-wide environment variables -- timing-sensitive behavior -- external services or ports -- tests that intentionally mutate common resources - -In those cases, isolate the state, use serial execution, or use a regular test where appropriate. - -## Filesystem tests - -If a test creates files or directories: - -1. Create them only inside the system temporary directory. -2. Use unique paths for each test case. -3. Attempt cleanup when the test finishes. -4. Verify cleanup errors when cleanup is part of the behavior being tested. -5. Avoid relying on repository-relative paths unless the target code explicitly requires them. - -Prefer temporary directory helpers where available. - -## Coverage expectations - -Ensure tests cover all logical branches as much as reasonably possible, including: - -- valid inputs -- invalid inputs -- empty inputs -- boundary values -- default values -- non-default values -- optional values present and absent -- all relevant combinations of input parameters -- branching paths -- internal conditions that affect observable behavior -- success results -- expected error results -- panic behavior, only when intentional -- filesystem success and failure paths when applicable -- async success, failure, cancellation, or ordering behavior when applicable - -Explicitly test all logical combinations of input parameters, branching paths, internal conditions, and expected outputs as much as reasonably possible. - -## DRYness - -Be DRY as much as reasonably possible. - -Prefer helpers, fixtures, builders, and table-driven cases over repeated setup code. - -Do not over-abstract tests if doing so makes the behavior harder to understand. - -## Output expectations - -When writing tests: - -1. Add or update the appropriate test module or test file. -2. Include any required imports, dev-dependencies, or feature flags. -3. Explain any assumptions made about the code under test. -4. Call out branches that could not reasonably be tested and why. -5. Ensure the resulting tests are idiomatic Rust and should compile in the project context. +# Rust Unit Tests -When modifying dependency files, add only the dependencies needed for the tests. +Write focused tests that comprehensively exercise the requested behavior. Preserve existing regression coverage and +keep changes within the requested code path. + +## Workflow + +1. Read the repository's [AGENTS.md](../../../AGENTS.md) and use the [README source map](../../../README.md#development) + to locate the target. Inspect its implementation, callers, existing tests, and relevant documented invariants before + choosing cases. File links in this skill are relative to this file. +2. Map reachable branches, boundary values, relevant parameter combinations, and success and error outcomes. Include + absent, empty, default, and non-default inputs where they produce distinct behavior. Aim for complete coverage of the + requested code path; explain gaps that require an external system or cannot reasonably be exercised. +3. Add or extend an inline `#[cfg(test)]` module beside the implementation, following Sinker's existing layout. Reuse setup + helpers where useful, but keep each case's inputs and expected behavior visible. Avoid widening production visibility + solely to test private helpers. +4. Run the focused tests, then the required repository checks described under [Verification](#verification). Report + assumptions, remaining gaps, and checks actually run; do not claim a coverage percentage without measurement. + +## Dependencies and test style + +Use [Cargo.toml](../../../Cargo.toml), [Cargo.lock](../../../Cargo.lock), and +[rust-toolchain.toml](../../../rust-toolchain.toml) for available dependencies, resolved APIs, and the toolchain. +Sinker already has `rstest`, `rand`, `once_cell`, and `chrono` as dev-dependencies, and Tokio as a runtime dependency. +Prefer standard assertions and existing helpers. Add a dev-dependency or feature only when the requested tests need it; +the project does not currently include an assertion library or temporary-directory helper. + +- Prefer `#[rstest]` with named `#[case::scenario(...)]` cases for comparable inputs and outcomes. Use `#[test]` for + synchronous tests that do not benefit from a table, and `#[tokio::test]` when the test needs an async runtime. +- Use helpers, builders, or `#[fixture]` for shared setup when they make the cases clearer. Share immutable fixture data + or construct fresh state per case; avoid abstractions that obscure the behavior being asserted. +- Test observable behavior and meaningful invariants, including preservation of unrelated data when relevant. Derive + expected values independently of the implementation so tests can detect regressions. +- Use `#[should_panic(expected = "...")]` only for intentional panic contracts. Report accidental panics encountered + while designing cases instead of treating them as required behavior. + +## Assertions and errors + +Assert concrete return values, resulting state, or error variants and payloads. For JSON and Kubernetes objects, compare +the relevant structure rather than serialized key order or a broad string match. + +Handle every `Result` from the test and its setup: use `expect`, `?` in a test returning `Result`, or explicit matching. +Cover relevant success and failure branches of the code under test; setup helpers do not each need their own error +matrix. Some functions return `Result` without a reachable error branch, so do not invent one solely to satisfy coverage. + +For expected errors, prefer `expect_err` and a match on [Sinker's error variants](../../../src/lib.rs) or the relevant +module's error type, checking meaningful payloads. A cause-specific variant can be sufficient. Check a stable message +substring when text carries additional meaning or the error is opaque; avoid coupling to full dependency error wording. +An `is_err()` assertion alone is insufficient when a more precise check is possible. For `Result<()>`, successful +completion may be the whole return contract; also check side effects where applicable. + +## Fixtures and isolation + +Use explicit, distinct, non-empty and non-default values for fields relevant to the scenario; retain defaults for +irrelevant scaffolding. Include separate cases for defaults, empty values, and `None` when they affect behavior. + +Use randomized fixtures when variation strengthens the test. Seed a local RNG with `StdRng::seed_from_u64`, use the +resolved `rand` API, and generate values that satisfy the intended domain. For example, alphanumeric sampling can +produce digits, so it is unsuitable without filtering for a namespace suffix meant to match `[a-z]`. Report the input +and seed on failure. Fixed seeds make failures reproducible; they do not make invalid fixture generation correct. + +Use fixed timestamps for ordering or retained-time assertions. When the code reads the current time internally, bound +the expected time around the call instead of relying on sleeps or an exact independently sampled timestamp. + +For filesystem behavior, create files and directories under a unique system-temporary directory per case. Arrange +cleanup even when assertions fail, handle explicit cleanup results, and assert cleanup failures when they are part of +the behavior under test. Prefer a temporary-directory guard when available. Use repository files only when the target +requires them, and keep test output out of the checkout. + +## Async behavior and Kubernetes boundaries + +The Rust test harness already runs tests in parallel; an async annotation is not needed for test-level concurrency. +Isolate global state, environment variables, filesystem paths, and ports for both sync and async tests. Changing a test +to `#[test]` does not serialize it. When isolation is impossible, use explicit coordination or a serial test invocation. + +For async behavior, test success, failure, cancellation, and ordering where applicable. Coordinate with channels or +barriers rather than sleeps, bound waits, and await spawned tasks while checking both join errors and returned results. +Cancel or otherwise stop background tasks during cleanup. Tokio's configured `full` feature does not include +`test-util`; check feature availability before using paused-time utilities. + +Sinker's existing tests exercise local logic. For API-dependent code, inspect the boundary and use a controlled client +or a narrowly scoped test seam when needed. Tests must not rely on ambient kubeconfig or a live cluster. Keep assertions +about local decisions distinct from claims about API-server authorization, validation, server-side apply, or garbage +collection. Read the relevant [implementation constraints](../../../AGENTS.md#implementation-constraints) when testing +mapping, status, access checks, or watch cleanup. If the requested behavior requires live verification, report that gap +and follow the repository's live-testing instructions only when that work is in scope. + +## Verification + +Run commands from the repository root. Cargo filters match module or test names, not file paths: replace `` in +`cargo test --locked -- --list` with a name found in the target's tests and confirm the intended cases are +selected before running `cargo test --locked `. A successful command that selects zero tests does not verify +the change. + +For Rust changes, complete all checks in [AGENTS.md's development instructions](../../../AGENTS.md#development-and-verification), +including the full locked test suite. Use its additional checks if the task also changes schemas or deployment files. +For skill-only edits, validate frontmatter, relative links, and claims against the sources; execute examples only when +needed to substantiate them. + +Summarize the behavior covered, verification results, and any untested paths or existing failures. Distinguish local +unit-test results from live behavior and commands inspected from commands executed. From b97a4f27ca689199ae8c014f5738da660e6fa433 Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 14:27:48 -0700 Subject: [PATCH 11/19] docs: style guidance --- AGENTS.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 0956be2..64c4033 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -35,6 +35,16 @@ For API/schema changes, also perform the CRD comparison described below. For dep execute examples only when needed to substantiate them. Report checks actually run and distinguish existing failures from regressions introduced by the change. +## Code style + +Keep project code as DRY (Don't Repeat Yourself) as reasonably possible. Reuse existing helpers and consolidate repeated +logic when doing so improves clarity and maintainability. Avoid abstractions that obscure meaningful differences or add +unnecessary complexity, and keep refactoring focused on the requested task. + +Preserve existing behaviors and contracts when changing code, including during refactoring and deduplication. Change +them only when the user explicitly requests it or there is no other reasonable way to complete the requested task. In +the latter case, keep the change minimal and explain why it is necessary and which behaviors or contracts it affects. + ## Implementation constraints - **API and serialization:** Update [resources.rs](src/resources.rs), [manifests/crd.yml](manifests/crd.yml), and affected From 06a8a14c3edce487f7dc7de7ff90f5e3a1f1b83d Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 14:28:27 -0700 Subject: [PATCH 12/19] docs: fix-rust-lint --- .agents/skills/fix-rust-lint/SKILL.md | 121 ++++++++++++++++++ .../skills/fix-rust-lint/agents/openai.yaml | 4 + 2 files changed, 125 insertions(+) create mode 100644 .agents/skills/fix-rust-lint/SKILL.md create mode 100644 .agents/skills/fix-rust-lint/agents/openai.yaml diff --git a/.agents/skills/fix-rust-lint/SKILL.md b/.agents/skills/fix-rust-lint/SKILL.md new file mode 100644 index 0000000..2451ab5 --- /dev/null +++ b/.agents/skills/fix-rust-lint/SKILL.md @@ -0,0 +1,121 @@ +--- +name: fix-rust-lint +description: Fix Rust clippy and rustfmt issues in a specific crate or Cargo workspace in the Tubernetes monorepo using crate/workspace-scoped `cargo clippy --fix --allow-dirty --allow-staged --all-targets --all-features -- -D warnings`. Use when the agent is asked to run, diagnose, or fix Clippy lint findings for Rust code such as tubectl, especially when fixes must preserve behavior and iterate until `cargo clippy` reports no warnings. +--- + +> **After completing tasks with this skill:** Invoke `improving-skills` to capture feedback and lessons learned. + +# Fix Rust Lint + +## Overview + +Fix crate-scoped Rust `clippy` and `rustfmt` findings in the Tubernetes monorepo without running Cargo from an unrelated repository root. Preserve existing function behavior, public contracts, error semantics, async/concurrency behavior, and serialization formats while making lint-compliant, idiomatic changes. + +## Workflow + +1. Identify the target Rust crate or Cargo workspace from the user's request. Use the directory that contains the relevant `Cargo.toml`, such as `tubectl`. Do not run Cargo from `` unless `/Cargo.toml` is the relevant crate or workspace manifest. +2. Inspect local state before edits with `git status --short` from the repository root. Do not revert or overwrite unrelated user changes. +3. Inspect the crate's lint and build configuration before choosing command flags: `Cargo.toml`, `Cargo.lock`, `.cargo/config*`, `rust-toolchain*`, `clippy.toml`, Makefile targets, and nearby CI scripts when present. +4. Run Clippy from the crate or workspace root. For a single-crate root, start with: + +```bash +cargo clippy --fix --allow-dirty --allow-staged --all-targets --all-features -- -D warnings +``` + +For a Cargo workspace, add `--workspace` when the user asked for the whole workspace, or use `-p ` to keep the run scoped to the affected package. + +5. Read the Clippy output and inspect any files changed automatically by `--fix`. Treat auto-fixes as edits that still require review. If `--fix` changes files adjacent to, but not directly part of, the feature or bug fix you were working on, keep only behavior-preserving fixes required for the final warning-free Clippy run and call those incidental edits out in the final response. +6. Fix each reported issue as much as reasonably possible. Prefer behavior-preserving code changes over suppression. +7. If a finding is a false positive or cannot reasonably be fixed, use the narrowest targeted lint expectation or suppression that the crate's MSRV and local style support. Prefer `#[expect(..., reason = "...")]` when supported; otherwise use `#[allow(...)]` with a nearby concise explanation: + +```rust +#[expect(clippy::lint_name, reason = "concise explanation of why this lint finding is intentionally accepted")] +``` + +```rust +#[allow(clippy::lint_name)] // concise explanation of why this lint finding is intentionally ignored +``` + +8. Run `cargo fmt --all` from the same crate or workspace root unless tooling already formatted the edited files. +9. Run relevant `cargo test` commands from the same crate or workspace root for packages whose behavior or tests changed. +10. Re-run a non-fixing Clippy command from the same crate or workspace root. Iterate on fixes and validation until it reports no warnings. + +## Fixing Rules + +- Maintain the pre-existing behavior and contract of every modified item. Do not change public signatures, trait implementations, return semantics, error variants, error text, serialization/deserialization behavior, CLI flags, side effects, locking, ordering, or async/concurrency behavior unless the lint issue cannot be fixed otherwise and the user has agreed. +- Keep fixes as small and local as practical. Avoid unrelated refactors. +- Keep code DRY where it materially improves clarity or removes repeated lint-prone logic. Do not introduce broad abstractions only to satisfy a single finding. +- Prefer the Rust standard library, well-established crate APIs already in use, and existing local helper APIs over ad hoc parsing, cloning, allocation, reflection-like patterns, or string manipulation. +- Treat Clippy machine suggestions as proposals, not proof of correctness. Re-check ownership, borrowing, lifetimes, drop order, iterator laziness, allocation behavior, and side effects after accepting a suggestion. +- Preserve panic behavior deliberately. Do not replace `unwrap`, `expect`, indexing, or panics with fallible behavior unless that is already part of the intended contract or the user agrees. +- Be careful with lints that can alter API shape or data layout, such as `large_enum_variant`, `large_error_err`, `boxed_local`, `ptr_arg`, `too_many_arguments`, `new_without_default`, `derive_*`, and lifetime elision suggestions. Verify downstream call sites, trait bounds, serde formats, and error handling before keeping the change. +- For iterator and collection lints, preserve ordering, duplicate handling, short-circuiting, mutation, and error accumulation behavior. Avoid "simplifying" loops when explicit control flow makes error handling or side effects clearer. +- Explain every lint expectation or suppression thoroughly but concisely. Name only the specific lint being suppressed or expected, and place the attribute on the narrowest applicable expression, item, module, or test. +- Never use broad suppressions such as `#[allow(warnings)]`, `#![allow(warnings)]`, `#[allow(clippy::all)]`, `#![allow(clippy::all)]`, or unexplained allow attributes. +- If `--fix` changes generated, vendored, lockfile, or config-derived files, inspect repository conventions before keeping the changes. Regenerate from the source tool when that is the established pattern. +- If `--fix` changes unrelated source files in the same crate or workspace, do not reflexively revert them. First determine whether they are behavior-preserving lint fixes needed to make the final non-fixing Clippy command pass. Keep those required fixes, avoid broad cleanup beyond what Clippy reported, and explicitly summarize the incidental files changed when reporting back to the user. + +## Test Code + +When lint findings or edits touch Rust test code, or when a lint fix changes behavior that needs regression coverage, also use `rust-unit-tests`. Apply that skill's style guidance while fixing lint issues so tests remain idiomatic for this repository. + +Convert Go unit-test patterns into Rust test work as follows: + +- Read the target file and nearby existing tests before adding or rewriting tests. +- When the user asks for tests based on a commit or commit range, inspect that exact scope first with `git show --name-only `, `git diff --name-only ..`, or `git log --stat `, then focus coverage on changed behavior while still reading nearby code and tests. +- Identify public behavior, private helpers worth testing from the same module, success paths, expected error paths, `None` or empty inputs, default and populated values, logical branches, loops over caller-provided collections, and meaningful combinations of independent inputs. +- Prefer table-driven tests using `rstest` wherever practical, with named cases that make the scenario obvious. +- Define shared fixtures before table cases. Use `#[fixture]` for `rstest` fixtures when it improves clarity. +- Use deterministic randomized fixtures for non-`None`, non-empty, and non-default values as much as reasonably possible. Seed randomness or use helper functions so tests do not become flaky. +- Implement focused helper builders only when they remove meaningful repetition. Keep table entries readable and explicit. +- Do not ignore `Result` values returned by setup, cleanup, or functions under test. Use test functions that return `Result` where that keeps success paths clear, and use `expect_err`, pattern matching, or precise assertions for expected failures. +- For expected errors, assert the error type when meaningful and assert the error value or message contents with a substring that identifies the actual cause. Do not merely assert that an error exists unless no stronger assertion is possible. +- Preemptively add explicit cases for complicated combinations of input parameters, even when they are redundant under the current implementation. +- When code loops over a slice, array, iterator, or map provided as input, include explicit multi-item cases as much as reasonably possible. +- When a loop body has multiple logical branches, include mixed-item cases that prove ordering, accumulation, mutation, and error handling across repeated iterations. +- For CLI, command, or process-level tests, identify global/static state, environment variables, current directory changes, output capture, and function hooks before relying on Rust's default parallel test execution. +- Reset process-wide or global state before the assertion path and again with cleanup guards so test order cannot leak credentials, env vars, flags, mocked collaborators, or tracing/logging subscribers between tests. +- Prefer fresh command or object instances per test case. Avoid concurrent tests when the code under test mutates shared global state, shared filesystem paths, process-wide environment variables, ports, timing-sensitive resources, or external services. +- For async code, use `#[tokio::test]` when it matches the crate's async runtime. Test success, failure, cancellation, ordering, and shared-state behavior where those paths affect observable behavior. +- For filesystem tests, use system temporary directories with unique paths per test case. Prefer existing temporary-directory helpers or `tempfile` if adding a dev-dependency is justified. Attempt cleanup when the test finishes and verify cleanup errors when cleanup is part of the behavior under test. +- Aim for 100% branch coverage where reasonable. Use the crate's established Rust coverage tool when available, such as `cargo llvm-cov` or `cargo tarpaulin`; otherwise run targeted `cargo test` commands and clearly report that coverage tooling was unavailable. + +## Validation Commands + +Run commands from the crate or workspace root. + +For a single crate: + +```bash +cargo clippy --fix --allow-dirty --allow-staged --all-targets --all-features -- -D warnings +cargo fmt --all +cargo test --all-targets --all-features +cargo clippy --all-targets --all-features -- -D warnings +cargo fmt --all -- --check +``` + +For a whole workspace: + +```bash +cargo clippy --fix --allow-dirty --allow-staged --workspace --all-targets --all-features -- -D warnings +cargo fmt --all +cargo test --workspace --all-targets --all-features +cargo clippy --workspace --all-targets --all-features -- -D warnings +cargo fmt --all -- --check +``` + +For a specific workspace package: + +```bash +cargo clippy --fix --allow-dirty --allow-staged -p --all-targets --all-features -- -D warnings +cargo fmt --all +cargo test -p --all-targets --all-features +cargo clippy -p --all-targets --all-features -- -D warnings +cargo fmt --all -- --check +``` + +If `--all-features` is invalid because the crate has mutually exclusive features or feature-gated platform behavior, use the crate's established CI or Makefile feature set and the narrowest additional feature set needed for the changed code. Report the deviation. + +If the default Cargo target directory or cache is not writable in the sandbox, rerun with a writable target directory such as `CARGO_TARGET_DIR=/tmp/-target`. If dependency download, missing toolchain components, or network access blocks a necessary command, rerun with approval outside the sandbox rather than treating it as a code failure. + +If package names or targets are unclear after a lint finding, derive the narrow package with `cargo metadata` from the crate or workspace root before running tests. Finish only after the final Clippy run reports no warnings and formatting checks pass, or clearly report any blocker that prevents reaching that state. diff --git a/.agents/skills/fix-rust-lint/agents/openai.yaml b/.agents/skills/fix-rust-lint/agents/openai.yaml new file mode 100644 index 0000000..223eb0c --- /dev/null +++ b/.agents/skills/fix-rust-lint/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "Fix Rust Lint" + short_description: "Fix module-scoped Rust clippy issues" + default_prompt: "Use $fix-rust-lint to fix clippy issues in this Rust crate or workspace." From f807178fd1cf336c353133f4874e337d0e0f32d4 Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 14:34:59 -0700 Subject: [PATCH 13/19] docs(1): fix-rust-lint --- .agents/skills/fix-rust-lint/SKILL.md | 190 ++++++++---------- .../skills/fix-rust-lint/agents/openai.yaml | 4 +- 2 files changed, 91 insertions(+), 103 deletions(-) diff --git a/.agents/skills/fix-rust-lint/SKILL.md b/.agents/skills/fix-rust-lint/SKILL.md index 2451ab5..59cc866 100644 --- a/.agents/skills/fix-rust-lint/SKILL.md +++ b/.agents/skills/fix-rust-lint/SKILL.md @@ -1,121 +1,109 @@ --- name: fix-rust-lint -description: Fix Rust clippy and rustfmt issues in a specific crate or Cargo workspace in the Tubernetes monorepo using crate/workspace-scoped `cargo clippy --fix --allow-dirty --allow-staged --all-targets --all-features -- -D warnings`. Use when the agent is asked to run, diagnose, or fix Clippy lint findings for Rust code such as tubectl, especially when fixes must preserve behavior and iterate until `cargo clippy` reports no warnings. +description: Fix Rust Clippy and rustfmt issues in Sinker while preserving behavior and public contracts. Use when asked to run, diagnose, or fix Rust lint findings, review Clippy suggestions, or iterate until formatting passes and Clippy reports no warnings. --- -> **After completing tasks with this skill:** Invoke `improving-skills` to capture feedback and lessons learned. +> **After completing tasks with this skill:** Invoke [improving-skills](../improving-skills/SKILL.md) to capture feedback +> and lessons learned. Combine this with the repository's required feedback pass. # Fix Rust Lint -## Overview - -Fix crate-scoped Rust `clippy` and `rustfmt` findings in the Tubernetes monorepo without running Cargo from an unrelated repository root. Preserve existing function behavior, public contracts, error semantics, async/concurrency behavior, and serialization formats while making lint-compliant, idiomatic changes. +Fix Rust lint and formatting findings with small, behavior-preserving changes. Sinker is a single Cargo package with +a binary and library; run Cargo commands from the repository root. File links in this skill are relative to this file. ## Workflow -1. Identify the target Rust crate or Cargo workspace from the user's request. Use the directory that contains the relevant `Cargo.toml`, such as `tubectl`. Do not run Cargo from `` unless `/Cargo.toml` is the relevant crate or workspace manifest. -2. Inspect local state before edits with `git status --short` from the repository root. Do not revert or overwrite unrelated user changes. -3. Inspect the crate's lint and build configuration before choosing command flags: `Cargo.toml`, `Cargo.lock`, `.cargo/config*`, `rust-toolchain*`, `clippy.toml`, Makefile targets, and nearby CI scripts when present. -4. Run Clippy from the crate or workspace root. For a single-crate root, start with: - -```bash -cargo clippy --fix --allow-dirty --allow-staged --all-targets --all-features -- -D warnings -``` - -For a Cargo workspace, add `--workspace` when the user asked for the whole workspace, or use `-p ` to keep the run scoped to the affected package. - -5. Read the Clippy output and inspect any files changed automatically by `--fix`. Treat auto-fixes as edits that still require review. If `--fix` changes files adjacent to, but not directly part of, the feature or bug fix you were working on, keep only behavior-preserving fixes required for the final warning-free Clippy run and call those incidental edits out in the final response. -6. Fix each reported issue as much as reasonably possible. Prefer behavior-preserving code changes over suppression. -7. If a finding is a false positive or cannot reasonably be fixed, use the narrowest targeted lint expectation or suppression that the crate's MSRV and local style support. Prefer `#[expect(..., reason = "...")]` when supported; otherwise use `#[allow(...)]` with a nearby concise explanation: - -```rust -#[expect(clippy::lint_name, reason = "concise explanation of why this lint finding is intentionally accepted")] -``` - -```rust -#[allow(clippy::lint_name)] // concise explanation of why this lint finding is intentionally ignored -``` - -8. Run `cargo fmt --all` from the same crate or workspace root unless tooling already formatted the edited files. -9. Run relevant `cargo test` commands from the same crate or workspace root for packages whose behavior or tests changed. -10. Re-run a non-fixing Clippy command from the same crate or workspace root. Iterate on fixes and validation until it reports no warnings. - -## Fixing Rules - -- Maintain the pre-existing behavior and contract of every modified item. Do not change public signatures, trait implementations, return semantics, error variants, error text, serialization/deserialization behavior, CLI flags, side effects, locking, ordering, or async/concurrency behavior unless the lint issue cannot be fixed otherwise and the user has agreed. -- Keep fixes as small and local as practical. Avoid unrelated refactors. -- Keep code DRY where it materially improves clarity or removes repeated lint-prone logic. Do not introduce broad abstractions only to satisfy a single finding. -- Prefer the Rust standard library, well-established crate APIs already in use, and existing local helper APIs over ad hoc parsing, cloning, allocation, reflection-like patterns, or string manipulation. -- Treat Clippy machine suggestions as proposals, not proof of correctness. Re-check ownership, borrowing, lifetimes, drop order, iterator laziness, allocation behavior, and side effects after accepting a suggestion. -- Preserve panic behavior deliberately. Do not replace `unwrap`, `expect`, indexing, or panics with fallible behavior unless that is already part of the intended contract or the user agrees. -- Be careful with lints that can alter API shape or data layout, such as `large_enum_variant`, `large_error_err`, `boxed_local`, `ptr_arg`, `too_many_arguments`, `new_without_default`, `derive_*`, and lifetime elision suggestions. Verify downstream call sites, trait bounds, serde formats, and error handling before keeping the change. -- For iterator and collection lints, preserve ordering, duplicate handling, short-circuiting, mutation, and error accumulation behavior. Avoid "simplifying" loops when explicit control flow makes error handling or side effects clearer. -- Explain every lint expectation or suppression thoroughly but concisely. Name only the specific lint being suppressed or expected, and place the attribute on the narrowest applicable expression, item, module, or test. -- Never use broad suppressions such as `#[allow(warnings)]`, `#![allow(warnings)]`, `#[allow(clippy::all)]`, `#![allow(clippy::all)]`, or unexplained allow attributes. -- If `--fix` changes generated, vendored, lockfile, or config-derived files, inspect repository conventions before keeping the changes. Regenerate from the source tool when that is the established pattern. -- If `--fix` changes unrelated source files in the same crate or workspace, do not reflexively revert them. First determine whether they are behavior-preserving lint fixes needed to make the final non-fixing Clippy command pass. Keep those required fixes, avoid broad cleanup beyond what Clippy reported, and explicitly summarize the incidental files changed when reporting back to the user. - -## Test Code - -When lint findings or edits touch Rust test code, or when a lint fix changes behavior that needs regression coverage, also use `rust-unit-tests`. Apply that skill's style guidance while fixing lint issues so tests remain idiomatic for this repository. - -Convert Go unit-test patterns into Rust test work as follows: - -- Read the target file and nearby existing tests before adding or rewriting tests. -- When the user asks for tests based on a commit or commit range, inspect that exact scope first with `git show --name-only `, `git diff --name-only ..`, or `git log --stat `, then focus coverage on changed behavior while still reading nearby code and tests. -- Identify public behavior, private helpers worth testing from the same module, success paths, expected error paths, `None` or empty inputs, default and populated values, logical branches, loops over caller-provided collections, and meaningful combinations of independent inputs. -- Prefer table-driven tests using `rstest` wherever practical, with named cases that make the scenario obvious. -- Define shared fixtures before table cases. Use `#[fixture]` for `rstest` fixtures when it improves clarity. -- Use deterministic randomized fixtures for non-`None`, non-empty, and non-default values as much as reasonably possible. Seed randomness or use helper functions so tests do not become flaky. -- Implement focused helper builders only when they remove meaningful repetition. Keep table entries readable and explicit. -- Do not ignore `Result` values returned by setup, cleanup, or functions under test. Use test functions that return `Result` where that keeps success paths clear, and use `expect_err`, pattern matching, or precise assertions for expected failures. -- For expected errors, assert the error type when meaningful and assert the error value or message contents with a substring that identifies the actual cause. Do not merely assert that an error exists unless no stronger assertion is possible. -- Preemptively add explicit cases for complicated combinations of input parameters, even when they are redundant under the current implementation. -- When code loops over a slice, array, iterator, or map provided as input, include explicit multi-item cases as much as reasonably possible. -- When a loop body has multiple logical branches, include mixed-item cases that prove ordering, accumulation, mutation, and error handling across repeated iterations. -- For CLI, command, or process-level tests, identify global/static state, environment variables, current directory changes, output capture, and function hooks before relying on Rust's default parallel test execution. -- Reset process-wide or global state before the assertion path and again with cleanup guards so test order cannot leak credentials, env vars, flags, mocked collaborators, or tracing/logging subscribers between tests. -- Prefer fresh command or object instances per test case. Avoid concurrent tests when the code under test mutates shared global state, shared filesystem paths, process-wide environment variables, ports, timing-sensitive resources, or external services. -- For async code, use `#[tokio::test]` when it matches the crate's async runtime. Test success, failure, cancellation, ordering, and shared-state behavior where those paths affect observable behavior. -- For filesystem tests, use system temporary directories with unique paths per test case. Prefer existing temporary-directory helpers or `tempfile` if adding a dev-dependency is justified. Attempt cleanup when the test finishes and verify cleanup errors when cleanup is part of the behavior under test. -- Aim for 100% branch coverage where reasonable. Use the crate's established Rust coverage tool when available, such as `cargo llvm-cov` or `cargo tarpaulin`; otherwise run targeted `cargo test` commands and clearly report that coverage tooling was unavailable. - -## Validation Commands - -Run commands from the crate or workspace root. - -For a single crate: - -```bash -cargo clippy --fix --allow-dirty --allow-staged --all-targets --all-features -- -D warnings -cargo fmt --all -cargo test --all-targets --all-features -cargo clippy --all-targets --all-features -- -D warnings -cargo fmt --all -- --check -``` - -For a whole workspace: +1. Read [AGENTS.md](../../../AGENTS.md) and use the [README source map](../../../README.md#development) to locate the + requested code. Inspect `git status --short` before edits and preserve unrelated user changes. For a diagnosis-only + request, use non-fixing commands and report findings without applying fixes. +2. Inspect [Cargo.toml](../../../Cargo.toml), [Cargo.lock](../../../Cargo.lock), + [rust-toolchain.toml](../../../rust-toolchain.toml), [rustfmt.toml](../../../rustfmt.toml), and + [CI](../../../.github/workflows/rust.yml). Check for additional Cargo or Clippy configuration if present. Use the + pinned toolchain and locked dependencies; do not upgrade them merely to resolve lint findings. Inspect crate-level + lint attributes and the affected implementation, callers, and tests before choosing a fix. +3. For a fix request, run the fixing command below. Review every automatic edit against the initial working tree. + Keep incidental fixes in other source files only when they preserve behavior and are needed for the final Clippy + check; identify those files in the final response. Do not overwrite pre-existing edits while removing unwanted fixes. +4. Resolve remaining findings using the [fixing rules](#fixing-rules). Prefer a behavior-preserving code change over a + suppression. If a finding cannot reasonably be fixed, use the narrowest justified lint expectation or allowance. +5. Run `cargo fmt` to apply formatting, then review the diff. Treat formatting as an edit; use `cargo fmt --check` for + read-only validation. Use [rust-unit-tests](../rust-unit-tests/SKILL.md) when fixing test code or adding regression + coverage, as described under [Test code](#test-code). +6. Complete the [validation commands](#validation) after Rust edits. If validation reveals another finding, repeat the + affected fixes and checks until Clippy reports no warnings and formatting passes, or report the concrete blocker. +7. Summarize fixes, incidental files changed, any lint expectations or suppressions, and checks actually run. Distinguish + existing failures from regressions and describe any unverified behavior. + +## Fixing rules + +- Preserve function behavior, public signatures, trait implementations, return values, error variants and text, + serialization, CLI flags, side effects, ordering, locking, and async/concurrency behavior. Follow + [AGENTS.md's behavior-preservation rule](../../../AGENTS.md#code-style) if a contract change is unavoidable; explain + the necessity and effect. A lint suggestion alone does not justify a behavior change. +- Keep fixes local and reuse existing helpers or established dependency APIs. Consolidate repeated logic when it + improves clarity, without introducing broad abstractions or unrelated cleanup. +- Treat machine suggestions as proposals. Re-check ownership, borrowing, lifetimes, drop order, iterator laziness, + allocations, and side effects. For async changes, inspect lock scope, cancellation, task joining, and ordering. +- Preserve panic behavior deliberately. Do not replace `unwrap`, `expect`, indexing, or panics with fallible behavior + merely to satisfy a lint; that changes the contract. +- For API or layout suggestions such as `large_enum_variant`, `large_error_err`, `boxed_local`, `ptr_arg`, + `too_many_arguments`, `new_without_default`, derives, or lifetime elision, inspect callers, trait bounds, serde + behavior, and error handling before accepting the change. +- For iterator and collection fixes, preserve ordering, duplicates, short-circuiting, mutation, and error accumulation. + Keep explicit control flow when it makes side effects or error handling clearer. +- Read the relevant [implementation constraints](../../../AGENTS.md#implementation-constraints) before accepting fixes + that affect mappings, client resolution, status, ownership, or watches. These paths have contracts beyond what local + unit tests establish. +- Place lint expectations or allowances on the narrowest applicable expression or item and name only the specific + lint. Prefer `#[expect(..., reason = "...")]` when the pinned toolchain supports it; otherwise use `#[allow(...)]` + with a concise explanation. Do not add broad suppressions such as `allow(warnings)` or `allow(clippy::all)`, or + unexplained attributes. For example: + + ```rust + #[expect(clippy::too_many_arguments, reason = "Signature must match the existing public API")] + ``` + +- Review any generated, vendored, lockfile, or configuration changes before keeping them. Follow the established + generator workflow where applicable. For API/schema changes, follow the + [CRD comparison instructions](../../../README.md#generating-crds), generating into a temporary file. Preserve the + documented spec-immutability rule and report existing drift; do not regenerate tracked CRDs as routine lint cleanup. + +## Test code + +Use [rust-unit-tests](../rust-unit-tests/SKILL.md) for test edits and regression coverage instead of duplicating its +fixture, assertion, isolation, and async guidance here. Read nearby inline tests and preserve their existing coverage. +Add focused cases when a fix affects behavior or carries a meaningful regression risk; mechanical lint and formatting +edits do not by themselves require new tests or a coverage campaign. + +Sinker uses ordinary Rust tests, `rstest`, and Tokio tests. Verify a focused test selection with +`cargo test --locked -- --list` before running `cargo test --locked `; filters match test names, not +file paths. Focused tests supplement the required full suite. Local unit tests do not verify live Kubernetes behavior. + +## Validation + +For lint fixes, run this from the repository root, then review the edits: ```bash -cargo clippy --fix --allow-dirty --allow-staged --workspace --all-targets --all-features -- -D warnings -cargo fmt --all -cargo test --workspace --all-targets --all-features -cargo clippy --workspace --all-targets --all-features -- -D warnings -cargo fmt --all -- --check +cargo clippy --locked --fix --allow-dirty --allow-staged --all-targets --all-features -- -D warnings +cargo fmt ``` -For a specific workspace package: +After Rust changes, complete the repository's build, formatting, test, and Clippy checks: ```bash -cargo clippy --fix --allow-dirty --allow-staged -p --all-targets --all-features -- -D warnings -cargo fmt --all -cargo test -p --all-targets --all-features -cargo clippy -p --all-targets --all-features -- -D warnings -cargo fmt --all -- --check +cargo build --locked +cargo fmt --check +cargo test --locked +cargo clippy --locked --all-targets --all-features -- -D warnings ``` -If `--all-features` is invalid because the crate has mutually exclusive features or feature-gated platform behavior, use the crate's established CI or Makefile feature set and the narrowest additional feature set needed for the changed code. Report the deviation. +The final Clippy command adds `-D warnings` to the repository's required invocation so warnings fail this skill's +completion check; CI itself does not set that flag. Run additional schema or deployment checks only when the change +calls for them under [AGENTS.md](../../../AGENTS.md#development-and-verification). -If the default Cargo target directory or cache is not writable in the sandbox, rerun with a writable target directory such as `CARGO_TARGET_DIR=/tmp/-target`. If dependency download, missing toolchain components, or network access blocks a necessary command, rerun with approval outside the sandbox rather than treating it as a code failure. +If build artifacts cannot be written, use a writable target directory such as `CARGO_TARGET_DIR=/tmp/sinker-target`. +This does not relocate the dependency cache. If sandbox restrictions on cache writes, downloads, or toolchain components +block a necessary command, request the required execution approval; distinguish environment failures from code failures. -If package names or targets are unclear after a lint finding, derive the narrow package with `cargo metadata` from the crate or workspace root before running tests. Finish only after the final Clippy run reports no warnings and formatting checks pass, or clearly report any blocker that prevents reaching that state. +For skill-only edits, validate frontmatter, supporting metadata, relative links, and command claims against the local +sources. Execute examples only when needed to substantiate them. Report commands inspected separately from checks run. diff --git a/.agents/skills/fix-rust-lint/agents/openai.yaml b/.agents/skills/fix-rust-lint/agents/openai.yaml index 223eb0c..bdf4bb8 100644 --- a/.agents/skills/fix-rust-lint/agents/openai.yaml +++ b/.agents/skills/fix-rust-lint/agents/openai.yaml @@ -1,4 +1,4 @@ interface: display_name: "Fix Rust Lint" - short_description: "Fix module-scoped Rust clippy issues" - default_prompt: "Use $fix-rust-lint to fix clippy issues in this Rust crate or workspace." + short_description: "Fix Sinker Rust lint and formatting issues" + default_prompt: "Use $fix-rust-lint to fix Sinker Clippy and rustfmt findings while preserving behavior and running the required locked Cargo checks." From 3741a5f31968331e9693238ec4c0fa575639f1d4 Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 14:35:49 -0700 Subject: [PATCH 14/19] docs: comment-code-diff --- .agents/skills/comment-code-diff/SKILL.md | 105 ++++++++++++++++++ .../comment-code-diff/agents/openai.yaml | 4 + .../references/rollingupdate-notes.md | 80 +++++++++++++ 3 files changed, 189 insertions(+) create mode 100644 .agents/skills/comment-code-diff/SKILL.md create mode 100644 .agents/skills/comment-code-diff/agents/openai.yaml create mode 100644 .agents/skills/comment-code-diff/references/rollingupdate-notes.md diff --git a/.agents/skills/comment-code-diff/SKILL.md b/.agents/skills/comment-code-diff/SKILL.md new file mode 100644 index 0000000..6622ae1 --- /dev/null +++ b/.agents/skills/comment-code-diff/SKILL.md @@ -0,0 +1,105 @@ +--- +name: comment-code-diff +description: Add thorough, explicit, concise comments to Go code and canonical YAML manifests, plus operator-facing spec.notes for applicable RollingUpdate resources, in a requested scope such as a branch diff, file, directory, package, Go module, or manifest tree. Use for documentation-focused passes that should explain behavior and operational intent while avoiding generated files and behavior changes outside that narrow notes exception. +--- + +> **After completing tasks with this skill:** Invoke `improving-skills` to capture feedback and lessons learned. + +# Comment Code Diff + +## Overview + +Use this skill to make a documentation-focused pass over Go code and canonical YAML manifests in the user's requested scope. Starfleet is a multi-module Go monorepo for Kubernetes controllers, operators, CLI tooling, shared APIs, and shipyard manifest generation. Add comments appropriate for code maintainers, code users, and PR reviewers where they clarify behavior, API contracts, operational assumptions, or reviewer-relevant intent. When a supported third-party dependency or its committed `RollingUpdate` is in scope, also add or update applicable operator-facing `.spec.notes`. This field is the skill's sole standing exception to a comment-only edit. Be thorough and explicit about non-obvious context while keeping documentation concise and avoiding narration that simply repeats the code or manifest. + +## Workflow + +1. Inspect repository guidance and working tree state. + - Read the root `AGENTS.md` and any component-level `AGENTS.md` that applies to the requested scope, such as `starfleet-controller/AGENTS.md`, `squadron-operator/AGENTS.md`, or `fleetctl/AGENTS.md`. + - Run `git status --short`. + - Identify the requested scope before reading code: + - For a request naming one commit, including "the most recent commit," resolve the revision to its immutable commit hash and inspect only that commit with `git show ` or `^..`. Do not substitute a branch comparison or include neighboring commits; when using the range form, preserve that exact comparison in every range command. + - For branch or commit-range requests, preserve the complete named comparison in every range command, including whether it uses `..` or `...` (for example, `73c4a096..HEAD` or `origin/main...HEAD`). + - Inspect current working-tree changes separately with `git status` and an unqualified `git diff`. Do not fold them into a requested commit range unless the user explicitly includes them in scope. + - For requests that name multiple commits without an explicit range, inspect the union of those commits' changed files with `git show` or `git diff-tree` for each commit, and do not implicitly include intervening commits unless the user names a range. + - For "this branch", "the diff", or unspecified branch comparison, default to `origin/main...HEAD`. + - For explicit file requests, use only the named files unless the user asks to include related files. + - For package, directory, or module requests, use the named directory/package/module scope. In this repo, module roots are directories with their own `go.mod`, including `starfleet-controller`, `squadron-operator`, `fleetctl`, `starfleet-kit`, `release-manager`, `cluster-test-probes`, `instance-creation-test`, and `region-classifier`. + - When the user combines scopes, such as "changes in `squadron-operator/internal/controller` since main", intersect them: use the diff range filtered to that path. + +2. Scope the review to relevant changed files. + - For diff-range scopes, start with `git diff --stat ` and `git diff --name-only `, where `` is the complete requested range rather than only its base revision. + - For explicit files, read those files directly, and use `git diff -- ` or `git diff -- ` only if the user asked for comments based on changes. + - For package, directory, or module scopes, enumerate Go files and canonical YAML manifests with `rg --files ` and, when a diff comparison is relevant, filter with `git diff --name-only -- `. + - Skip generated files, vendored files, dependency metadata, and unrelated docs unless the user explicitly asks for them. In Starfleet, this includes `zz_generated.deepcopy.go`, generated mocks such as `*_mock_test.go` when their header says they are generated, and controller-gen output such as generated CRD or RBAC YAML. + - Read related changed documentation as supporting context when it describes eligible code or manifest behavior. Do not edit it during a comment-only pass unless the user separately requests documentation changes; report any verified documentation drift in the final response. + - Treat repo-owned canonical YAML as eligible for comments when the requested scope includes it. Known canonical YAML trees include `manifests/release/**`, `**/dist/**`, and most `**/config/**` YAML; these are source manifests, so useful `#` comments may be appropriate when they clarify non-obvious operational intent. + - Treat `starfleet-controller/dist/manifests/management-cluster/config/rollingupdates/**` as canonical YAML. When branch, multi-commit range, directory, module, or manifest-tree changes add, modify, or partially remove a supported third-party dependency, its corresponding `RollingUpdate` is an eligible synchronization file for `.spec.notes` even if that file was not already changed. Do not use this exception to audit unrelated dependencies. Keep an exact single-commit request limited to files changed by that commit and an explicit-file request limited to the named files; report an applicable out-of-scope note instead. + - Treat generated YAML snapshots as read-only output. In particular, skip `**/rendered_manifests/**`: those files are snapshot-test outputs produced from fixed inputs, canonical manifests, and the applicable generator or transformer code, so hand-written comments there will be overwritten. + - Treat generated YAML under `**/config/**` as read-only too. In `starfleet-controller` and `squadron-operator`, this includes CRD and RBAC YAML generated from Go API types, kubebuilder markers, or controller-gen configuration, even though other config YAML is generally canonical. + - For large diffs, prioritize handwritten production code before tests, docs, or tooling. + +3. Read changed code before editing. + - Use `git diff --unified=80 -- ` for changed context when a diff comparison applies. + - For explicit files or module scopes without a diff comparison, read the whole target file plus nearby tests or callers as needed. + - Read surrounding files and tests when needed to understand contracts, Kubernetes reconciliation semantics, shipyard generator or transformer behavior, CLI command behavior, status propagation, or edge cases. + - When a supported third-party dependency or a committed `RollingUpdate` is in scope, read [RollingUpdate Notes](references/rollingupdate-notes.md) before editing. It defines the update-system sources of truth, applicability threshold, dependency-specific research, content boundaries, and verification for `.spec.notes`. + - Prefer `rg` for finding related exported identifiers, callers, and existing comment style. + +4. Add comments only where they carry useful intent. + - Follow GoDoc conventions for exported packages, types, funcs, vars, consts, interface methods, and struct fields that are part of a public or cross-package contract. + - Add package docs for new public packages when the package purpose is not already documented. + - Write for the relevant audience: maintainers need invariants, lifecycle ordering, ownership, and maintenance hazards; code users need API contracts, defaults, nil and zero-value behavior, authorization, and compatibility expectations; PR reviewers need intent behind changed behavior, tradeoffs, and risk-sensitive decisions. + - Add inline comments for non-obvious behavior: reconciliation ordering, finalizers, ownership, pruning, Sinker sync boundaries, status and condition transitions, shipyard manifest filtering or emission, cloud-provider assumptions, multi-tenant safety, security posture, nil semantics, retries, matching rules, compatibility behavior, data sensitivity, or deliberately skipped work. + - In Starfleet API type packages, treat comments on CRD types and fields as externally visible API documentation because kubebuilder can copy them into CRD schema descriptions. Preserve `+kubebuilder`, RBAC, deepcopy, and other code-generation markers exactly unless the user explicitly asked to edit them. + - In `fleetctl`, comment command behavior where it clarifies flag/config/env precedence, interactive prompts, generated equivalent commands, watch selectors, or compatibility with existing automation. + - In shipyard generators and transformers, explain why resources are emitted, dropped, merged, or cloud-specialized when the reason is not obvious from the manifest shape. + - In canonical YAML manifests, use YAML `#` comments sparingly for operational intent that is not obvious from resource kind, name, labels, or field values. Avoid comments that merely restate Kubernetes field names or duplicate adjacent Go transformer comments. + - In a canonical `RollingUpdate`, put durable guidance needed by the person approving a discovered dependency version in `.spec.notes`, not only in YAML `#` comments. Keep maintainer-only rendering and identity invariants as YAML comments. Add, revise, or remove notes only according to [RollingUpdate Notes](references/rollingupdate-notes.md). + - Replace mechanical comments like "construct request", "set header", or "return error" with comments that explain why the code does that work, or remove them if no extra context is needed. + - Phrase reviewer-oriented context as maintainer-facing code intent. Do not mention PRs, reviewers, the comment-writing task, or other review process details in committed code comments. + - Be thorough and explicit enough to capture the needed reason, contract, or operational implication, but keep each comment close to the code it explains and as short as accuracy allows. + +5. Preserve behavior. + - Do not change exported signatures, error behavior, metric names, condition types, logging of sensitive values, resource names, labels, annotations, owner references, CLI output, or Kubernetes apply/prune semantics while adding comments. + - Treat string literals, raw string contents, struct tags, identifiers, executable statements, kubebuilder markers, and generated CRD schema descriptions as behavior. Restore any non-comment changes unless the user explicitly requested them or the change is an applicable canonical `RollingUpdate.spec.notes` edit made under this skill. + - A `.spec.notes` edit changes notifier-visible API data even though it does not select a version or target. Keep it limited to operator documentation; do not change discovery, update type, targets, concurrency, names, or any other manifest value during the notes pass. Never introduce committed `.spec.selectedVersion`, which is runtime approval state owned by `fleetctl dependency approve`. If a resource already commits that field, do not modify it under documentation-only authority; report the blocking contract violation and the need for a separately authorized behavior change. + - Do not hand-edit generated files. Regenerate them from the owning component's tooling only when the user's requested comment pass intentionally changes source comments that drive generated output. + - Avoid broad refactors, even if comments reveal cleanup opportunities. + +## Go Comment Guidance + +- Start GoDoc comments for exported identifiers with the identifier name. +- Make package comments begin with `Package ...`. +- Follow GoDoc conventions where they apply; do not force GoDoc-style wording onto ordinary inline comments. +- When working in `starfleet-kit` or other library-like code that may be imported from multiple Go modules, treat exported functions and methods as external APIs whose GoDoc should include usage instructions and examples where applicable. +- Document nil, zero-value, timeout, authorization, and ownership semantics when callers must know them. +- For interfaces, explain the contract and any important method-level behavior. +- For config structs, comment fields whose JSON meaning, defaults, secrecy, or operational impact would not be obvious from the field name. +- For Kubernetes API structs, make field comments accurate for CRD users, not just Go callers. +- Keep inline comments close to the decision they justify. + +## YAML Comment Guidance + +- Only add comments to canonical YAML manifests, such as `manifests/release/**`, `**/dist/**`, most `**/config/**` YAML, or another YAML file that local guidance or file context clearly identifies as handwritten source. +- Do not add comments to generated YAML output, including `**/rendered_manifests/**`, generated CRDs, generated RBAC manifests, generated portions of `starfleet-controller/config/**` or `squadron-operator/config/**`, or other YAML with generated-file headers. +- When a generated YAML snapshot lacks an important explanation, add the comment to the canonical manifest or to the Go generator or transformer that produces the snapshot. +- Keep YAML comments close to the field, list item, or resource they explain, and focus on operational assumptions, ownership, ordering, security posture, cloud-provider differences, or why a value must not drift. +- For `RollingUpdate` manifests, distinguish YAML comments from `.spec.notes`: comments explain the resource to maintainers, while notes are Markdown delivered to operators when approval is needed. Do not duplicate the same prose in both places. +- Preserve YAML semantics exactly: do not reorder keys, normalize formatting, change anchors, alter document separators, or move comments in a way that changes parser behavior. + +## Verification + +After edits: + +1. Run `gofmt` on touched Go files. For `fleetctl`, `just fmt` is also acceptable from the module root. +2. Run focused tests from the owning Go module, not the repository root. Use the nearest `go.mod` to choose the module root and prefer the component guidance from its `AGENTS.md`. +3. If comments changed Kubernetes API type docs or code-generation markers in `starfleet-controller/api` or `squadron-operator/api`, run the component's appropriate generation target, usually `make manifests` and, when type generation is affected, `make generate`. +4. If canonical YAML comments or `RollingUpdate.spec.notes` changed manifests that feed snapshot tests, run the owning generator, transformer, or snapshot test workflow when it is reasonably discoverable from the surrounding package. For `starfleet-controller/**/rendered_manifests/**`, regenerate from `starfleet-controller/**/dist/**` with the starfleet-controller `make render` target. For `release-manager/**/rendered_manifests/**` and `squadron-operator/**/rendered_manifests/**`, regenerate from `manifests/release/**` with the release-manager `just render` target. +5. If linting, use `$fix-go-lint` guidance and run the narrowest useful module-scoped `golangci-lint run` scope. +6. Run `git diff --check`. +7. Review `git diff` for changed literals, struct tags, identifiers, statements, raw string contents, code-generation markers, generated output, YAML values or ordering, redundant comments, inaccurate comments, or accidental non-comment behavior changes. For a `RollingUpdate` notes pass, confirm `.spec.notes` is the only intentionally changed YAML value and no modified resource commits `.spec.selectedVersion`. +8. Restore incidental formatting-only changes introduced by editing tools, such as adding or removing a final newline from a file that was otherwise intentionally unchanged except for comments. + +## Final Response + +Summarize the documentation pass by naming the requested scope, the main files or areas touched, any companion `RollingUpdate.spec.notes` changes, the verification commands run, and any warnings or skipped generated files, including generated YAML snapshots such as `**/rendered_manifests/**`. diff --git a/.agents/skills/comment-code-diff/agents/openai.yaml b/.agents/skills/comment-code-diff/agents/openai.yaml new file mode 100644 index 0000000..422a6a1 --- /dev/null +++ b/.agents/skills/comment-code-diff/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "Comment Code Diff" + short_description: "Document scoped Go, YAML, and rollout notes" + default_prompt: "Use $comment-code-diff to add concise, useful comments and applicable RollingUpdate operator notes to this branch, package, file, or manifest scope." diff --git a/.agents/skills/comment-code-diff/references/rollingupdate-notes.md b/.agents/skills/comment-code-diff/references/rollingupdate-notes.md new file mode 100644 index 0000000..31732fa --- /dev/null +++ b/.agents/skills/comment-code-diff/references/rollingupdate-notes.md @@ -0,0 +1,80 @@ +# RollingUpdate Notes + +Read this reference when the requested scope contains a supported third-party dependency deployed to a `ManagementCluster`, `ObservabilityCluster`, or `RegionalCluster`, or contains its committed `RollingUpdate` under `starfleet-controller/dist/manifests/management-cluster/config/rollingupdates/`. + +## Scope and update-system contract + +Begin from dependencies or `RollingUpdate` resources in the user's requested scope. Do not turn the task into a fleet-wide notes audit. For branch, multi-commit range, directory, module, or manifest-tree scopes, the corresponding canonical `RollingUpdate` is an eligible synchronization file even when it was not already changed. A request for one exact commit remains limited to files changed by that commit, and an explicit-file request remains limited to the named files; report an applicable note outside either scope instead of editing it. + +Before writing notes, read: + +- [Third-Party Dependency RollingUpdate Coverage](../../code-review/references/repository-review-contracts.md#third-party-dependency-rollingupdate-coverage) for the complete resource-coverage contract; +- `starfleet-controller/api/v1alpha1/rollingupdate_types.go` for admitted update, discovery, and target combinations and field semantics; and +- `starfleet-controller/internal/controller/rollingupdate/README.md` for discovery identities, approval behavior, notifier delivery, target mutation, and progress semantics. + +Use the coverage contract to identify the applicable committed resource; do not create or repair a `RollingUpdate` as part of a documentation-only pass. Report non-note synchronization defects unless the user separately authorizes their repair. In particular: + +- `RollingUpdate` manages supported dependencies maintained outside InfluxData, not internal dependencies maintained by InfluxData. +- A dependency normally has one resource selecting the complete union of applicable target types. Provider-specific regional dependencies use the admission-owned provider label; dependencies deployed to every provider use an unfiltered `RegionalCluster` target. +- Thanos is the sole one-resource-per-deployed-HelmRelease exception. Each document in `thanos.yaml` maps to one rendered `thanos-` HelmRelease. Notes must be accurate for the particular component; repeat shared guidance only when every sentence applies to every document. +- Resource names and update types can be operational identities: Flux package and progress lookup use the `RollingUpdate` name, CAPI providers have fixed identities, k0smotron represents paired bootstrap and control-plane providers, and Kubernetes must be named `kubernetes`. +- Never introduce committed `.spec.selectedVersion`. It is runtime approval state changed by `fleetctl dependency approve`, not documentation or a manifest default. If the field is already committed, modifying the resource would trigger the review contract's requirement to remove it; do not make that behavior change under documentation-only authority. Report the blocker and request separate authorization before changing the resource. + +## When notes are applicable + +`.spec.notes` is optional Markdown passed to every notifier. The GitHub notifier appends it beneath its own `## Notes` heading in the approval issue; Slack links to that issue rather than copying the text. + +Add notes when an operator needs dependency-specific information beyond the notifier's ordinary version, discovery, target, and concurrency context to decide whether or how to approve safely. Typical triggers include: + +- required update ordering, prerequisites, supported version or skew constraints, or intermediate upgrades; +- CRD, API, configuration, storage, schema, or data migrations; +- breaking changes, removed features, irreversible steps, or a constrained rollback path; +- a reason to approve only an exact version or a narrower automatic-approval range; +- manual validation, soak time, or data-plane checks not represented by the configured progress adapter; or +- a documented incident, known incompatibility, temporary prerelease exception, security tradeoff, or exit condition that materially affects approval. + +Update existing notes whenever scoped code, manifests, targets, update mechanics, incidents, or verified upstream requirements make them incomplete, stale, misleading, or applicable to the wrong dependency. Remove an obsolete statement when evidence shows it no longer applies. Omit `.spec.notes` when there is no durable, actionable guidance; generic filler is less useful than no note. + +## Research the actual dependency + +Trace the dependency from its `RollingUpdate` to every canonical deployment source and provider variant selected by its targets. Inspect relevant values, transformers, controllers, tests, component documentation, and `AGENTS.md` files. Search `docs/incidents/` by dependency name, resource name, alert, and failure signature; established contributing factors, rejected mitigations, recovery steps, and open follow-ups take precedence over generic advice. + +Consult the dependency's official release notes, upgrade guide, compatibility or version-skew policy, and security notices when repository evidence does not fully establish the approval requirements. Prefer primary upstream sources, link directly to stable authoritative guidance when the link will help the operator, and do not turn an unverified assumption into an instruction. Verify every copied or shared sentence against the named dependency; nearby resources may have different upgrade ordering, version limits, targets, or failure modes. + +Consider the dependency family without substituting family-wide boilerplate for specific evidence: + +- For Kubernetes and k0s, check control-plane and worker skew, adjacent-version requirements, + k0s revision handling, and the repository's comparable `X.Y.Z-k0s.N` approval identity. Trace + rollout ordering through downstream controllers and their readiness gates, not only the + `RollingUpdate` target mutation and progress adapters. Before asserting that sequencing is + absent or blocking approval, distinguish what `RollingUpdate` orchestrates from what CAPI, + k0smotron, or another target subsystem enforces, and corroborate the conclusion with tests, + authoritative documentation, or user-provided operational evidence. Revision-sensitive + back-pins should use an exact version because range discovery can over-select tags whose k0s + revision is build metadata upstream. +- For CAPI Operator, core Cluster API, CAPA, CAPZ, and k0smotron, establish the supported compatibility matrix and required update order. State which resource the note governs. Remember that the k0smotron bootstrap and control-plane providers are one logical dependency updated together; do not paste core-provider instructions into an infrastructure provider or an unrelated chart without proving they apply. +- For networking, ingress, DNS, certificate, identity, and storage dependencies, examine availability blast radius, CRD or controller/node-agent sequencing, migration requirements, rollback behavior, and the health signal that proves the data plane still works. A `FluxHelmRelease` can report completion before a chart's data-plane rollout finishes, so document a separate check or soak only when the dependency's implementation or an incident establishes that need. +- For observability dependencies, determine whether the update affects collection, remote write, querying, alerting, or retained data and whether losing the component can hide the rollout's own failure. Keep per-component Thanos guidance aligned with the rendered HelmRelease it controls. +- For a temporary prerelease, pin, workaround, or known-bad-version exclusion, record why it exists, the evidence-backed condition for removing it, and what must change when that condition is met. Avoid time-relative wording that will silently become false. + +## Write for the approver + +Use a YAML block scalar and concise Markdown. The API limit is 32,768 characters, but useful notes should normally be much shorter. Do not add a `## Notes` heading because the GitHub notifier supplies it. + +Make each instruction self-contained and actionable: + +- name the affected dependency, target, or related component rather than relying on pronouns; +- state the required order or compatibility boundary and what evidence the operator should check; +- use the current command name, `fleetctl dependency approve `, when approval mechanics are relevant; +- link a repository checklist, incident, or official upstream guide instead of copying a long procedure; and +- distinguish a mandatory safety gate from a recommendation or observation. + +Do not merely restate `.spec.type`, discovery URLs or ranges, target selectors, or `maxConcurrentUpdates`; the approval issue already has that context. Do not mention the PR, reviewer, or comment-writing task. Do not include credentials, internal tokens, or other sensitive data. Keep maintainer-only explanations of rendering, naming, or target selection in nearby YAML `#` comments rather than notifier-facing notes. + +## Verification + +After changing canonical notes: + +1. Run `make render` from `starfleet-controller/` and inspect the corresponding generated `RollingUpdate` snapshots; never hand-edit those snapshots. +2. Run `VALIDATE_YAML_CONCURRENCY=4 ./scripts/validate-yaml.sh --changed-only` from the repository root. +3. Run `git diff --check` and inspect the diff. Confirm the Markdown remains readable after YAML rendering, every factual instruction is supported, `.spec.notes` is the only intentional canonical YAML value changed by the documentation pass, and no modified resource commits `.spec.selectedVersion`. From 34f920274b42cb7041bbe762d5694a34c733fe28 Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 14:43:27 -0700 Subject: [PATCH 15/19] docs(1): comment-code-diff --- .agents/skills/comment-code-diff/SKILL.md | 224 +++++++++++------- .../comment-code-diff/agents/openai.yaml | 4 +- .../references/rollingupdate-notes.md | 80 ------- 3 files changed, 141 insertions(+), 167 deletions(-) delete mode 100644 .agents/skills/comment-code-diff/references/rollingupdate-notes.md diff --git a/.agents/skills/comment-code-diff/SKILL.md b/.agents/skills/comment-code-diff/SKILL.md index 6622ae1..128bd7c 100644 --- a/.agents/skills/comment-code-diff/SKILL.md +++ b/.agents/skills/comment-code-diff/SKILL.md @@ -1,105 +1,159 @@ --- name: comment-code-diff -description: Add thorough, explicit, concise comments to Go code and canonical YAML manifests, plus operator-facing spec.notes for applicable RollingUpdate resources, in a requested scope such as a branch diff, file, directory, package, Go module, or manifest tree. Use for documentation-focused passes that should explain behavior and operational intent while avoiding generated files and behavior changes outside that narrow notes exception. +description: Add thorough, explicit, concise Rust documentation and inline comments, plus comments in canonical YAML manifests, for a requested commit, branch diff, file, Rust module, directory, crate, or manifest tree. Use for documentation-focused passes that explain behavior, API contracts, and operational intent while preserving runtime behavior and respecting generated-file boundaries. --- -> **After completing tasks with this skill:** Invoke `improving-skills` to capture feedback and lessons learned. +> **After completing tasks with this skill:** Invoke [improving-skills](../improving-skills/SKILL.md) to capture feedback and lessons learned. # Comment Code Diff -## Overview +Make a documentation-focused pass over Rust code and handwritten YAML manifests in the requested scope. Explain +non-obvious behavior, contracts, and operational intent for maintainers and code users. Be thorough where context matters, +but keep comments concise and close to the code they explain; avoid narration that repeats the implementation. -Use this skill to make a documentation-focused pass over Go code and canonical YAML manifests in the user's requested scope. Starfleet is a multi-module Go monorepo for Kubernetes controllers, operators, CLI tooling, shared APIs, and shipyard manifest generation. Add comments appropriate for code maintainers, code users, and PR reviewers where they clarify behavior, API contracts, operational assumptions, or reviewer-relevant intent. When a supported third-party dependency or its committed `RollingUpdate` is in scope, also add or update applicable operator-facing `.spec.notes`. This field is the skill's sole standing exception to a comment-only edit. Be thorough and explicit about non-obvious context while keeping documentation concise and avoiding narration that simply repeats the code or manifest. +Run commands and resolve unlinked source paths from the repository root. Markdown links are relative to this skill file. ## Workflow -1. Inspect repository guidance and working tree state. - - Read the root `AGENTS.md` and any component-level `AGENTS.md` that applies to the requested scope, such as `starfleet-controller/AGENTS.md`, `squadron-operator/AGENTS.md`, or `fleetctl/AGENTS.md`. - - Run `git status --short`. - - Identify the requested scope before reading code: - - For a request naming one commit, including "the most recent commit," resolve the revision to its immutable commit hash and inspect only that commit with `git show ` or `^..`. Do not substitute a branch comparison or include neighboring commits; when using the range form, preserve that exact comparison in every range command. - - For branch or commit-range requests, preserve the complete named comparison in every range command, including whether it uses `..` or `...` (for example, `73c4a096..HEAD` or `origin/main...HEAD`). - - Inspect current working-tree changes separately with `git status` and an unqualified `git diff`. Do not fold them into a requested commit range unless the user explicitly includes them in scope. - - For requests that name multiple commits without an explicit range, inspect the union of those commits' changed files with `git show` or `git diff-tree` for each commit, and do not implicitly include intervening commits unless the user names a range. - - For "this branch", "the diff", or unspecified branch comparison, default to `origin/main...HEAD`. - - For explicit file requests, use only the named files unless the user asks to include related files. - - For package, directory, or module requests, use the named directory/package/module scope. In this repo, module roots are directories with their own `go.mod`, including `starfleet-controller`, `squadron-operator`, `fleetctl`, `starfleet-kit`, `release-manager`, `cluster-test-probes`, `instance-creation-test`, and `region-classifier`. - - When the user combines scopes, such as "changes in `squadron-operator/internal/controller` since main", intersect them: use the diff range filtered to that path. - -2. Scope the review to relevant changed files. - - For diff-range scopes, start with `git diff --stat ` and `git diff --name-only `, where `` is the complete requested range rather than only its base revision. - - For explicit files, read those files directly, and use `git diff -- ` or `git diff -- ` only if the user asked for comments based on changes. - - For package, directory, or module scopes, enumerate Go files and canonical YAML manifests with `rg --files ` and, when a diff comparison is relevant, filter with `git diff --name-only -- `. - - Skip generated files, vendored files, dependency metadata, and unrelated docs unless the user explicitly asks for them. In Starfleet, this includes `zz_generated.deepcopy.go`, generated mocks such as `*_mock_test.go` when their header says they are generated, and controller-gen output such as generated CRD or RBAC YAML. - - Read related changed documentation as supporting context when it describes eligible code or manifest behavior. Do not edit it during a comment-only pass unless the user separately requests documentation changes; report any verified documentation drift in the final response. - - Treat repo-owned canonical YAML as eligible for comments when the requested scope includes it. Known canonical YAML trees include `manifests/release/**`, `**/dist/**`, and most `**/config/**` YAML; these are source manifests, so useful `#` comments may be appropriate when they clarify non-obvious operational intent. - - Treat `starfleet-controller/dist/manifests/management-cluster/config/rollingupdates/**` as canonical YAML. When branch, multi-commit range, directory, module, or manifest-tree changes add, modify, or partially remove a supported third-party dependency, its corresponding `RollingUpdate` is an eligible synchronization file for `.spec.notes` even if that file was not already changed. Do not use this exception to audit unrelated dependencies. Keep an exact single-commit request limited to files changed by that commit and an explicit-file request limited to the named files; report an applicable out-of-scope note instead. - - Treat generated YAML snapshots as read-only output. In particular, skip `**/rendered_manifests/**`: those files are snapshot-test outputs produced from fixed inputs, canonical manifests, and the applicable generator or transformer code, so hand-written comments there will be overwritten. - - Treat generated YAML under `**/config/**` as read-only too. In `starfleet-controller` and `squadron-operator`, this includes CRD and RBAC YAML generated from Go API types, kubebuilder markers, or controller-gen configuration, even though other config YAML is generally canonical. - - For large diffs, prioritize handwritten production code before tests, docs, or tooling. - -3. Read changed code before editing. - - Use `git diff --unified=80 -- ` for changed context when a diff comparison applies. - - For explicit files or module scopes without a diff comparison, read the whole target file plus nearby tests or callers as needed. - - Read surrounding files and tests when needed to understand contracts, Kubernetes reconciliation semantics, shipyard generator or transformer behavior, CLI command behavior, status propagation, or edge cases. - - When a supported third-party dependency or a committed `RollingUpdate` is in scope, read [RollingUpdate Notes](references/rollingupdate-notes.md) before editing. It defines the update-system sources of truth, applicability threshold, dependency-specific research, content boundaries, and verification for `.spec.notes`. - - Prefer `rg` for finding related exported identifiers, callers, and existing comment style. - -4. Add comments only where they carry useful intent. - - Follow GoDoc conventions for exported packages, types, funcs, vars, consts, interface methods, and struct fields that are part of a public or cross-package contract. - - Add package docs for new public packages when the package purpose is not already documented. - - Write for the relevant audience: maintainers need invariants, lifecycle ordering, ownership, and maintenance hazards; code users need API contracts, defaults, nil and zero-value behavior, authorization, and compatibility expectations; PR reviewers need intent behind changed behavior, tradeoffs, and risk-sensitive decisions. - - Add inline comments for non-obvious behavior: reconciliation ordering, finalizers, ownership, pruning, Sinker sync boundaries, status and condition transitions, shipyard manifest filtering or emission, cloud-provider assumptions, multi-tenant safety, security posture, nil semantics, retries, matching rules, compatibility behavior, data sensitivity, or deliberately skipped work. - - In Starfleet API type packages, treat comments on CRD types and fields as externally visible API documentation because kubebuilder can copy them into CRD schema descriptions. Preserve `+kubebuilder`, RBAC, deepcopy, and other code-generation markers exactly unless the user explicitly asked to edit them. - - In `fleetctl`, comment command behavior where it clarifies flag/config/env precedence, interactive prompts, generated equivalent commands, watch selectors, or compatibility with existing automation. - - In shipyard generators and transformers, explain why resources are emitted, dropped, merged, or cloud-specialized when the reason is not obvious from the manifest shape. - - In canonical YAML manifests, use YAML `#` comments sparingly for operational intent that is not obvious from resource kind, name, labels, or field values. Avoid comments that merely restate Kubernetes field names or duplicate adjacent Go transformer comments. - - In a canonical `RollingUpdate`, put durable guidance needed by the person approving a discovered dependency version in `.spec.notes`, not only in YAML `#` comments. Keep maintainer-only rendering and identity invariants as YAML comments. Add, revise, or remove notes only according to [RollingUpdate Notes](references/rollingupdate-notes.md). - - Replace mechanical comments like "construct request", "set header", or "return error" with comments that explain why the code does that work, or remove them if no extra context is needed. - - Phrase reviewer-oriented context as maintainer-facing code intent. Do not mention PRs, reviewers, the comment-writing task, or other review process details in committed code comments. - - Be thorough and explicit enough to capture the needed reason, contract, or operational implication, but keep each comment close to the code it explains and as short as accuracy allows. - -5. Preserve behavior. - - Do not change exported signatures, error behavior, metric names, condition types, logging of sensitive values, resource names, labels, annotations, owner references, CLI output, or Kubernetes apply/prune semantics while adding comments. - - Treat string literals, raw string contents, struct tags, identifiers, executable statements, kubebuilder markers, and generated CRD schema descriptions as behavior. Restore any non-comment changes unless the user explicitly requested them or the change is an applicable canonical `RollingUpdate.spec.notes` edit made under this skill. - - A `.spec.notes` edit changes notifier-visible API data even though it does not select a version or target. Keep it limited to operator documentation; do not change discovery, update type, targets, concurrency, names, or any other manifest value during the notes pass. Never introduce committed `.spec.selectedVersion`, which is runtime approval state owned by `fleetctl dependency approve`. If a resource already commits that field, do not modify it under documentation-only authority; report the blocking contract violation and the need for a separately authorized behavior change. - - Do not hand-edit generated files. Regenerate them from the owning component's tooling only when the user's requested comment pass intentionally changes source comments that drive generated output. - - Avoid broad refactors, even if comments reveal cleanup opportunities. - -## Go Comment Guidance - -- Start GoDoc comments for exported identifiers with the identifier name. -- Make package comments begin with `Package ...`. -- Follow GoDoc conventions where they apply; do not force GoDoc-style wording onto ordinary inline comments. -- When working in `starfleet-kit` or other library-like code that may be imported from multiple Go modules, treat exported functions and methods as external APIs whose GoDoc should include usage instructions and examples where applicable. -- Document nil, zero-value, timeout, authorization, and ownership semantics when callers must know them. -- For interfaces, explain the contract and any important method-level behavior. -- For config structs, comment fields whose JSON meaning, defaults, secrecy, or operational impact would not be obvious from the field name. -- For Kubernetes API structs, make field comments accurate for CRD users, not just Go callers. -- Keep inline comments close to the decision they justify. +1. Read applicable repository guidance and establish the working tree state. + - Start with the root [AGENTS.md](../../../AGENTS.md) and [README.md](../../../README.md), then any nested instructions + that apply to the requested files. Use the README's [source map](../../../README.md#development) to locate related + implementation and its task-specific sections when documenting reconciliation, mappings, credentials, or watches. + - Run `git status --short`, `git diff`, and `git diff --cached`. Preserve existing edits and inspect relevant untracked + files separately. Keep working-tree changes distinct from a requested historical comparison. + +2. Resolve the requested scope before choosing files to edit. + - For one commit, including "the most recent commit," resolve its immutable hash and inspect only that commit with + `git show `. Do not substitute a branch comparison or include neighboring commits. + - For a branch or commit range, preserve the complete comparison in every range command, including `..` versus `...` + (for example, `origin/main...HEAD`). For "this branch," "the diff," or an unspecified branch comparison, default to + `origin/main...HEAD` unless the conversation identifies working-tree changes. Verify the base exists; do not silently + substitute another base if it is unavailable. + - For multiple named commits without an explicit range, inspect each with `git show` or `git diff-tree` and use the + union of their changed files. Do not implicitly include intervening commits. + - For explicit files, edit only the named files. Related callers, tests, and docs may be read to verify intent. + - For a module, directory, crate, or manifest tree, resolve its actual files before enumerating them. Sinker has one + Cargo package at the repository root, with binary and library entrypoints. A Rust module may be one `.rs` file; + follow declarations in [lib.rs](../../../src/lib.rs) and the named module rather than assuming a module directory. + - Intersect combined scopes: "changes in `src/controller.rs` since main" means the requested comparison filtered to + that path. Do not add working-tree changes to a commit scope unless the user includes them. + +3. Identify eligible files and read the relevant implementation. + - For comparisons, start with `git diff --stat ` and `git diff --name-only `, adding a path + filter when requested. Read hunks with `git diff --unified=80 -- `. + - For file scopes, read the named files directly. For directory or crate scopes, use `rg --files ` to enumerate + Rust files and canonical YAML. Include hidden directories explicitly when they are requested. + - Skip generated output, vendored files, dependency metadata, and unrelated docs as edit targets unless explicitly + requested. In Sinker, `manifests/crd.yml` is generated from Rust resource definitions with a checked-in validation + addition; it is not an ordinary YAML comment target. Follow [Derived documentation](#derived-documentation) for + intentional source documentation changes that affect it. + - Handwritten deployment and RBAC files under [manifests/](../../../manifests/) and [example.yaml](../../../example.yaml) + are eligible when in scope. Establish provenance from generators, CI, and local guidance; a YAML extension or missing + generated header alone does not establish that a file is handwritten. + - Read callers, inline `#[cfg(test)]` modules, and related documentation as needed to verify contracts and edge cases. + Trace implementation through error and cleanup paths; do not treat existing comments, TODOs, or fixtures as proof. + Report verified drift in documentation outside the edit scope. + - For large diffs, prioritize handwritten production code before tests and tooling. Use `rg` to find related symbols + and existing comment style. + +4. Write comments that add useful context. + - Use [Rust comment guidance](#rust-comment-guidance) for API contracts and implementation intent, and + [YAML comment guidance](#yaml-comment-guidance) for operational explanations. + - Explain invariants, lifecycle ordering, ownership, authorization, defaults, retries, data sensitivity, and deliberate + omissions when they matter to the scoped code. Follow the relevant constraints in + [AGENTS.md](../../../AGENTS.md#implementation-constraints) instead of reproducing the entire controller design. + - Express review-relevant context as durable maintainer-facing intent. Do not mention PRs, reviewers, or the + comment-writing task in committed comments. + - Correct misleading comments when behavior is verified. Remove mechanical comments such as "construct request" or + "return error" if there is no additional intent to explain. + +5. Preserve behavior and scope, then verify the pass. + - Keep signatures, visibility, executable statements, identifiers, error messages, logging, serialization, resource + identities, CLI parsing, and reconciliation semantics unchanged. Preserve non-documentation attributes, including + `serde`, `schemars`, `kube`, `clap`/`command`/`arg`, `cfg`, derive, and lint attributes. + - String and raw-string contents are data even when they contain YAML or look like comments. Do not edit the + `MANUAL_SCHEMA` string or manifest values under comment-only authority. + - Rust doc comments can affect generated public output. Apply [Derived documentation](#derived-documentation) before + editing comments consumed by macros; ordinary `//` comments are suitable for maintainer-only context. + - Avoid refactors, test additions unrelated to documentation, and incidental formatting changes. Restore only changes + introduced by the pass that fall outside its scope; preserve pre-existing work. + +## Rust Comment Guidance + +- Use `///` for item documentation and `//!` for crate or module documentation. Describe the purpose in a short opening + sentence, then add detail needed by callers. Rustdoc prose need not start with the identifier's name. +- Document public types, functions, methods, traits, variants, and fields where their contract needs explanation. Add + crate or module docs when their purpose or relationship to other modules is unclear and the file is in scope. +- Describe meaningful `Option`/`None`, empty-collection, `Default`, borrowing, ownership, error, and panic semantics. + Explain asynchronous cancellation, locking, or task shutdown when callers or maintainers rely on those guarantees. + Use `# Errors`, `# Panics`, `# Safety`, or `# Examples` only where relevant and supported by the implementation. +- For serialized API types, use the actual configuration field names and distinguish runtime checks from schema + validation. Verify defaults and missing-value behavior through serialization attributes and callers, rather than + inferring them from Rust field types alone. Follow the derived documentation rules below for schema-facing prose. +- Use `//` near non-obvious decisions, such as why a status read must be live, why a watcher is cancelled before joining, + or why mapping source selectors and destination paths differ. Explain the applicable invariant rather than narrating + each statement or duplicating nearby API documentation. +- Use resolvable intra-doc links for Rust symbols and Markdown links for URLs. The crate denies broken intra-doc links + and bare URLs. Keep examples self-contained; Rust code fences are doctests by default. Mark examples requiring a live + cluster as `no_run` and non-Rust snippets with their correct language, rather than using `ignore` to hide invalid code. +- Use synthetic values in examples. Source objects and mapped values can contain Secret data; do not copy credentials + into comments or add payload logging while documenting a path. ## YAML Comment Guidance -- Only add comments to canonical YAML manifests, such as `manifests/release/**`, `**/dist/**`, most `**/config/**` YAML, or another YAML file that local guidance or file context clearly identifies as handwritten source. -- Do not add comments to generated YAML output, including `**/rendered_manifests/**`, generated CRDs, generated RBAC manifests, generated portions of `starfleet-controller/config/**` or `squadron-operator/config/**`, or other YAML with generated-file headers. -- When a generated YAML snapshot lacks an important explanation, add the comment to the canonical manifest or to the Go generator or transformer that produces the snapshot. -- Keep YAML comments close to the field, list item, or resource they explain, and focus on operational assumptions, ownership, ordering, security posture, cloud-provider differences, or why a value must not drift. -- For `RollingUpdate` manifests, distinguish YAML comments from `.spec.notes`: comments explain the resource to maintainers, while notes are Markdown delivered to operators when approval is needed. Do not duplicate the same prose in both places. -- Preserve YAML semantics exactly: do not reorder keys, normalize formatting, change anchors, alter document separators, or move comments in a way that changes parser behavior. +- Add `#` comments only to canonical YAML within scope. Explain non-obvious operational assumptions, ownership, ordering, + permissions, or why a setting must remain consistent with other resources. Avoid restating field names or values. +- Preserve parsed values and structure exactly, including key ordering, anchors, document separators, quoting, and block + scalars. A `#` inside a block scalar or quoted value is payload, not a YAML comment. +- Put explanations for generated artifacts in their owning source when eligible, rather than hand-authoring comments + in generated output. For CRDs, see the source documentation and schema constraints below. -## Verification +## Derived Documentation + +Before editing doc comments consumed by macros, inspect their downstream output. In Sinker, `JsonSchema` documentation +in [resources.rs](../../../src/resources.rs) can become CRD titles or descriptions, and `clap` documentation in +[main.rs](../../../src/main.rs) can become CLI help. These are public output changes even though the input is a comment. + +Change derived descriptions or help only when that output is included in the requested documentation scope. Preserve +validation and runtime behavior. If the necessary companion output is outside an exact-file or single-commit scope, +keep the pass to maintainer comments and report the out-of-scope documentation need instead of expanding the edit set. + +For an intentional CRD documentation update, follow [Generating CRDs](../../../README.md#generating-crds) and the API +instructions in [AGENTS.md](../../../AGENTS.md#implementation-constraints). Generate to temporary files before and after +the source edit to distinguish its effects from existing drift. Carry only intended, in-scope documentation changes +from generated output into the checked-in CRDs; preserve schema constraints, defaults, and serialized fields. Keep +affected examples accurate when they are included in the requested scope. -After edits: +The checked-in `ResourceSync.spec` has `self == oldSelf` validation that the generator omits. Preserve that rule and report +the inherited drift; do not replace the tracked file wholesale or repair generation during an unrelated comment pass. +`SinkerContainer` uses `crd_with_manual_schema()` to preserve arbitrary `.spec` content, so documentation on its empty +Rust spec type does not describe the stored payload. Read the manual schema when explaining that API. + +For intentional CLI help changes, compare `cargo run --locked -- --help` and the affected subcommand's help before and +afterward. Preserve flags, defaults, environment bindings, and parsing behavior. Running without a subcommand starts +reconciliation; use explicit help or `manifests` invocations for local documentation checks. + +## Verification -1. Run `gofmt` on touched Go files. For `fleetctl`, `just fmt` is also acceptable from the module root. -2. Run focused tests from the owning Go module, not the repository root. Use the nearest `go.mod` to choose the module root and prefer the component guidance from its `AGENTS.md`. -3. If comments changed Kubernetes API type docs or code-generation markers in `starfleet-controller/api` or `squadron-operator/api`, run the component's appropriate generation target, usually `make manifests` and, when type generation is affected, `make generate`. -4. If canonical YAML comments or `RollingUpdate.spec.notes` changed manifests that feed snapshot tests, run the owning generator, transformer, or snapshot test workflow when it is reasonably discoverable from the surrounding package. For `starfleet-controller/**/rendered_manifests/**`, regenerate from `starfleet-controller/**/dist/**` with the starfleet-controller `make render` target. For `release-manager/**/rendered_manifests/**` and `squadron-operator/**/rendered_manifests/**`, regenerate from `manifests/release/**` with the release-manager `just render` target. -5. If linting, use `$fix-go-lint` guidance and run the narrowest useful module-scoped `golangci-lint run` scope. -6. Run `git diff --check`. -7. Review `git diff` for changed literals, struct tags, identifiers, statements, raw string contents, code-generation markers, generated output, YAML values or ordering, redundant comments, inaccurate comments, or accidental non-comment behavior changes. For a `RollingUpdate` notes pass, confirm `.spec.notes` is the only intentionally changed YAML value and no modified resource commits `.spec.selectedVersion`. -8. Restore incidental formatting-only changes introduced by editing tools, such as adding or removing a final newline from a file that was otherwise intentionally unchanged except for comments. +1. For Rust edits, run the root [development checks](../../../AGENTS.md#development-and-verification) from the repository + root, using the selected toolchain, locked dependency resolution, and read-only `cargo fmt --check`. Rust comment + edits still require the prescribed build, format, test, and Clippy checks. Fix formatting only within the edited scope. +2. For changed Rustdoc, also run `cargo doc --locked --no-deps --document-private-items` to check links and rendering. + Select `--lib` or `--bin sinker` as needed to cover the edited target. Inspect the relevant generated pages; + the prescribed `cargo test --locked` includes library doctests. If using a focused test + filter while investigating, confirm selection with `cargo test --locked -- --list`; filters match test names, + not source paths. Local checks do not establish live Kubernetes behavior. +3. For YAML comment edits, compare parsed values with the pre-edit version, including examples outside the deployment + bundle. When deployment manifests change, run `kubectl kustomize manifests` and compare rendered output before and + after the pass. Rendering is local and does not deploy resources. +4. For source comments that feed CRDs or CLI help, perform the comparisons in + [Derived documentation](#derived-documentation). Treat any unexpected public output change as a scope or correctness + issue, and distinguish pre-existing CRD drift from changes introduced by the pass. +5. Run `git diff --check` and review the final diff against the initial working-tree state for scope, redundant or + inaccurate comments, accidental literal or attribute changes, generated output, and YAML values or formatting. + Restore incidental changes introduced by editing tools, including unrelated final-newline changes. ## Final Response -Summarize the documentation pass by naming the requested scope, the main files or areas touched, any companion `RollingUpdate.spec.notes` changes, the verification commands run, and any warnings or skipped generated files, including generated YAML snapshots such as `**/rendered_manifests/**`. +Summarize the requested scope, main files or areas documented, intentional changes to derived documentation, and checks +actually run. Note relevant skipped generated files, verified documentation drift, and any checks that could not run. diff --git a/.agents/skills/comment-code-diff/agents/openai.yaml b/.agents/skills/comment-code-diff/agents/openai.yaml index 422a6a1..8cc43cd 100644 --- a/.agents/skills/comment-code-diff/agents/openai.yaml +++ b/.agents/skills/comment-code-diff/agents/openai.yaml @@ -1,4 +1,4 @@ interface: display_name: "Comment Code Diff" - short_description: "Document scoped Go, YAML, and rollout notes" - default_prompt: "Use $comment-code-diff to add concise, useful comments and applicable RollingUpdate operator notes to this branch, package, file, or manifest scope." + short_description: "Document scoped Rust code and YAML manifests" + default_prompt: "Use $comment-code-diff to add concise, useful Rust documentation and YAML comments to this commit, branch, module, file, or manifest scope while preserving behavior." diff --git a/.agents/skills/comment-code-diff/references/rollingupdate-notes.md b/.agents/skills/comment-code-diff/references/rollingupdate-notes.md deleted file mode 100644 index 31732fa..0000000 --- a/.agents/skills/comment-code-diff/references/rollingupdate-notes.md +++ /dev/null @@ -1,80 +0,0 @@ -# RollingUpdate Notes - -Read this reference when the requested scope contains a supported third-party dependency deployed to a `ManagementCluster`, `ObservabilityCluster`, or `RegionalCluster`, or contains its committed `RollingUpdate` under `starfleet-controller/dist/manifests/management-cluster/config/rollingupdates/`. - -## Scope and update-system contract - -Begin from dependencies or `RollingUpdate` resources in the user's requested scope. Do not turn the task into a fleet-wide notes audit. For branch, multi-commit range, directory, module, or manifest-tree scopes, the corresponding canonical `RollingUpdate` is an eligible synchronization file even when it was not already changed. A request for one exact commit remains limited to files changed by that commit, and an explicit-file request remains limited to the named files; report an applicable note outside either scope instead of editing it. - -Before writing notes, read: - -- [Third-Party Dependency RollingUpdate Coverage](../../code-review/references/repository-review-contracts.md#third-party-dependency-rollingupdate-coverage) for the complete resource-coverage contract; -- `starfleet-controller/api/v1alpha1/rollingupdate_types.go` for admitted update, discovery, and target combinations and field semantics; and -- `starfleet-controller/internal/controller/rollingupdate/README.md` for discovery identities, approval behavior, notifier delivery, target mutation, and progress semantics. - -Use the coverage contract to identify the applicable committed resource; do not create or repair a `RollingUpdate` as part of a documentation-only pass. Report non-note synchronization defects unless the user separately authorizes their repair. In particular: - -- `RollingUpdate` manages supported dependencies maintained outside InfluxData, not internal dependencies maintained by InfluxData. -- A dependency normally has one resource selecting the complete union of applicable target types. Provider-specific regional dependencies use the admission-owned provider label; dependencies deployed to every provider use an unfiltered `RegionalCluster` target. -- Thanos is the sole one-resource-per-deployed-HelmRelease exception. Each document in `thanos.yaml` maps to one rendered `thanos-` HelmRelease. Notes must be accurate for the particular component; repeat shared guidance only when every sentence applies to every document. -- Resource names and update types can be operational identities: Flux package and progress lookup use the `RollingUpdate` name, CAPI providers have fixed identities, k0smotron represents paired bootstrap and control-plane providers, and Kubernetes must be named `kubernetes`. -- Never introduce committed `.spec.selectedVersion`. It is runtime approval state changed by `fleetctl dependency approve`, not documentation or a manifest default. If the field is already committed, modifying the resource would trigger the review contract's requirement to remove it; do not make that behavior change under documentation-only authority. Report the blocker and request separate authorization before changing the resource. - -## When notes are applicable - -`.spec.notes` is optional Markdown passed to every notifier. The GitHub notifier appends it beneath its own `## Notes` heading in the approval issue; Slack links to that issue rather than copying the text. - -Add notes when an operator needs dependency-specific information beyond the notifier's ordinary version, discovery, target, and concurrency context to decide whether or how to approve safely. Typical triggers include: - -- required update ordering, prerequisites, supported version or skew constraints, or intermediate upgrades; -- CRD, API, configuration, storage, schema, or data migrations; -- breaking changes, removed features, irreversible steps, or a constrained rollback path; -- a reason to approve only an exact version or a narrower automatic-approval range; -- manual validation, soak time, or data-plane checks not represented by the configured progress adapter; or -- a documented incident, known incompatibility, temporary prerelease exception, security tradeoff, or exit condition that materially affects approval. - -Update existing notes whenever scoped code, manifests, targets, update mechanics, incidents, or verified upstream requirements make them incomplete, stale, misleading, or applicable to the wrong dependency. Remove an obsolete statement when evidence shows it no longer applies. Omit `.spec.notes` when there is no durable, actionable guidance; generic filler is less useful than no note. - -## Research the actual dependency - -Trace the dependency from its `RollingUpdate` to every canonical deployment source and provider variant selected by its targets. Inspect relevant values, transformers, controllers, tests, component documentation, and `AGENTS.md` files. Search `docs/incidents/` by dependency name, resource name, alert, and failure signature; established contributing factors, rejected mitigations, recovery steps, and open follow-ups take precedence over generic advice. - -Consult the dependency's official release notes, upgrade guide, compatibility or version-skew policy, and security notices when repository evidence does not fully establish the approval requirements. Prefer primary upstream sources, link directly to stable authoritative guidance when the link will help the operator, and do not turn an unverified assumption into an instruction. Verify every copied or shared sentence against the named dependency; nearby resources may have different upgrade ordering, version limits, targets, or failure modes. - -Consider the dependency family without substituting family-wide boilerplate for specific evidence: - -- For Kubernetes and k0s, check control-plane and worker skew, adjacent-version requirements, - k0s revision handling, and the repository's comparable `X.Y.Z-k0s.N` approval identity. Trace - rollout ordering through downstream controllers and their readiness gates, not only the - `RollingUpdate` target mutation and progress adapters. Before asserting that sequencing is - absent or blocking approval, distinguish what `RollingUpdate` orchestrates from what CAPI, - k0smotron, or another target subsystem enforces, and corroborate the conclusion with tests, - authoritative documentation, or user-provided operational evidence. Revision-sensitive - back-pins should use an exact version because range discovery can over-select tags whose k0s - revision is build metadata upstream. -- For CAPI Operator, core Cluster API, CAPA, CAPZ, and k0smotron, establish the supported compatibility matrix and required update order. State which resource the note governs. Remember that the k0smotron bootstrap and control-plane providers are one logical dependency updated together; do not paste core-provider instructions into an infrastructure provider or an unrelated chart without proving they apply. -- For networking, ingress, DNS, certificate, identity, and storage dependencies, examine availability blast radius, CRD or controller/node-agent sequencing, migration requirements, rollback behavior, and the health signal that proves the data plane still works. A `FluxHelmRelease` can report completion before a chart's data-plane rollout finishes, so document a separate check or soak only when the dependency's implementation or an incident establishes that need. -- For observability dependencies, determine whether the update affects collection, remote write, querying, alerting, or retained data and whether losing the component can hide the rollout's own failure. Keep per-component Thanos guidance aligned with the rendered HelmRelease it controls. -- For a temporary prerelease, pin, workaround, or known-bad-version exclusion, record why it exists, the evidence-backed condition for removing it, and what must change when that condition is met. Avoid time-relative wording that will silently become false. - -## Write for the approver - -Use a YAML block scalar and concise Markdown. The API limit is 32,768 characters, but useful notes should normally be much shorter. Do not add a `## Notes` heading because the GitHub notifier supplies it. - -Make each instruction self-contained and actionable: - -- name the affected dependency, target, or related component rather than relying on pronouns; -- state the required order or compatibility boundary and what evidence the operator should check; -- use the current command name, `fleetctl dependency approve `, when approval mechanics are relevant; -- link a repository checklist, incident, or official upstream guide instead of copying a long procedure; and -- distinguish a mandatory safety gate from a recommendation or observation. - -Do not merely restate `.spec.type`, discovery URLs or ranges, target selectors, or `maxConcurrentUpdates`; the approval issue already has that context. Do not mention the PR, reviewer, or comment-writing task. Do not include credentials, internal tokens, or other sensitive data. Keep maintainer-only explanations of rendering, naming, or target selection in nearby YAML `#` comments rather than notifier-facing notes. - -## Verification - -After changing canonical notes: - -1. Run `make render` from `starfleet-controller/` and inspect the corresponding generated `RollingUpdate` snapshots; never hand-edit those snapshots. -2. Run `VALIDATE_YAML_CONCURRENCY=4 ./scripts/validate-yaml.sh --changed-only` from the repository root. -3. Run `git diff --check` and inspect the diff. Confirm the Markdown remains readable after YAML rendering, every factual instruction is supported, `.spec.notes` is the only intentional canonical YAML value changed by the documentation pass, and no modified resource commits `.spec.selectedVersion`. From 0dc25268ba27ce3cb7d5c05eb4a755256807d97d Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 14:50:01 -0700 Subject: [PATCH 16/19] chore: claude symlinks --- .claude | 1 + CLAUDE.md | 1 + 2 files changed, 2 insertions(+) create mode 120000 .claude create mode 120000 CLAUDE.md diff --git a/.claude b/.claude new file mode 120000 index 0000000..c0ca468 --- /dev/null +++ b/.claude @@ -0,0 +1 @@ +.agents \ No newline at end of file diff --git a/CLAUDE.md b/CLAUDE.md new file mode 120000 index 0000000..47dc3e3 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +AGENTS.md \ No newline at end of file From a30d2c472a0ad9107d6a0119b7944a5395580a69 Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 14:52:14 -0700 Subject: [PATCH 17/19] chore: regen --- manifests/crd.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/manifests/crd.yml b/manifests/crd.yml index aa291cc..d14470b 100644 --- a/manifests/crd.yml +++ b/manifests/crd.yml @@ -139,9 +139,6 @@ spec: - source - target type: object - x-kubernetes-validations: - - message: spec is immutable - rule: "self == oldSelf" status: nullable: true properties: From 9c685ef8dc2931a5321b7d6d02b45ebb0cc1aec4 Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Thu, 10 Sep 2026 15:13:12 -0700 Subject: [PATCH 18/19] feat: add test coverage --- Cargo.lock | 2 + Cargo.toml | 2 + src/controller.rs | 611 +++++++++++++++++++++++++++++++++- src/filters.rs | 34 ++ src/lib.rs | 129 +++++++ src/mapping.rs | 200 +++++++++++ src/remote_watcher.rs | 310 +++++++++++++++++ src/remote_watcher_manager.rs | 178 ++++++++++ src/resource_extensions.rs | 232 ++++++++++++- src/resources.rs | 92 +++++ src/util.rs | 50 +++ 11 files changed, 1820 insertions(+), 20 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6028582..2c743ee 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1992,6 +1992,7 @@ dependencies = [ "chrono", "clap", "futures", + "http", "k8s-openapi", "kube", "kubert", @@ -2008,6 +2009,7 @@ dependencies = [ "tokio", "tokio-context", "tokio-stream", + "tower", "tracing", ] diff --git a/Cargo.toml b/Cargo.toml index 78e6c38..3eb8915 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,6 +34,8 @@ tokio-stream = "^0.1.18" regex = "^1.12.3" [dev-dependencies] +http = "1.4.0" +tower = { version = "0.5.2", features = ["util"] } rstest = "^0.26.1" once_cell = "^1.21.3" chrono = "^0.4.42" diff --git a/src/controller.rs b/src/controller.rs index ed03ea4..906613b 100644 --- a/src/controller.rs +++ b/src/controller.rs @@ -432,21 +432,599 @@ pub async fn run(client: Client) -> Result<()> { #[cfg(test)] mod tests { + use super::{ + reconcile, reconcile_deleted_resource, reconcile_helper, reconcile_normally, Context, + RemoteWatcherManager, + }; use super::{ reconcile_status, sync_failing_transition_time, RESOURCE_SYNC_FAILING_CONDITION, RESOURCE_SYNC_SUCCEEDED_REASON, }; use crate::resources::{ResourceSync, ResourceSyncStatus}; + use crate::test_support::{ + api_error, discovery_response, resource_sync as sync_fixture, response, MockApi, + }; + use crate::FINALIZER; use crate::{Error, Result}; - use chrono::{TimeDelta, TimeZone}; + use chrono::TimeZone; use k8s_openapi::apimachinery::pkg::apis::meta::v1::Condition; use k8s_openapi::apimachinery::pkg::apis::meta::v1::ObjectMeta; use k8s_openapi::apimachinery::pkg::apis::meta::v1::Time; use kube::runtime::controller::Action; use once_cell::sync::Lazy; use rstest::rstest; + use serde_json::json; + use std::{sync::Arc, time::Duration}; + + fn context(client: kube::Client) -> Arc { + let (remote_watcher_manager, _events) = RemoteWatcherManager::new(client.clone()); + Arc::new(Context { + client, + remote_watcher_manager, + }) + } + + async fn stop_watches(ctx: &Context) { + tokio::time::timeout( + Duration::from_secs(2), + ctx.remote_watcher_manager.stop_all(), + ) + .await + .expect("watchers cancel and join"); + } + + const SYNC_PATH: &str = + "/apis/sinker.influxdata.io/v1alpha1/namespaces/team-a/resourcesyncs/copy-config"; + const STATUS_PATH: &str = + "/apis/sinker.influxdata.io/v1alpha1/namespaces/team-a/resourcesyncs/copy-config/status"; + const SOURCE_PATH: &str = "/api/v1/namespaces/team-a/configmaps/source-config"; + const TARGET_PATH: &str = "/api/v1/namespaces/team-a/configmaps/target-config"; + + #[tokio::test] + async fn initialization_preserves_finalizers_and_writes_live_success_status() { + let mut sync = sync_fixture(); + sync.metadata.finalizers = Some(vec!["example.com/other".into()]); + sync.status = status_with_condition("True"); + let mut live = sync.clone(); + live.status = status_with_condition("False"); + let mock = MockApi::new(vec![ + discovery_response("ConfigMap", "configmaps", true), + discovery_response("ConfigMap", "configmaps", true), + response(200, json!(sync)), + response(200, json!(live)), + response(200, json!(live)), + ]); + let result = reconcile(Arc::new(sync), context(mock.client.clone())) + .await + .expect("initialize sync"); + assert_eq!(result, Action::requeue(Duration::from_millis(500))); + let requests = mock.finish(&[ + ("GET", "/api/v1"), + ("GET", "/api/v1"), + ("PATCH", SYNC_PATH), + ("GET", STATUS_PATH), + ("PATCH", STATUS_PATH), + ]); + assert_eq!( + requests[2].body(), + &json!({"metadata": {"finalizers": ["example.com/other", FINALIZER]}}) + ); + assert_eq!( + requests[2].headers()["content-type"], + "application/merge-patch+json" + ); + let status: ResourceSyncStatus = + serde_json::from_value(requests[4].body()["status"].clone()).expect("status patch"); + let condition = single_condition(Some(status)); + assert_eq!(condition.last_transition_time, *EPOCH); + assert_eq!(condition.observed_generation, Some(7)); + assert_eq!(condition.status, "False"); + assert_eq!(condition.message, "Sync succeeded"); + } + + #[tokio::test] + async fn deleted_missing_target_removes_only_our_finalizer_and_skips_status() { + let mut sync = sync_fixture(); + sync.metadata.deletion_timestamp = Some(EPOCH.clone()); + sync.metadata.finalizers = Some(vec![ + FINALIZER.into(), + "example.com/keep".into(), + FINALIZER.into(), + ]); + let mock = MockApi::new(vec![ + discovery_response("ConfigMap", "configmaps", true), + discovery_response("ConfigMap", "configmaps", true), + api_error(404), + response(200, json!(sync)), + ]); + assert_eq!( + reconcile(Arc::new(sync), context(mock.client.clone())) + .await + .expect("cleanup"), + Action::await_change() + ); + let requests = mock.finish(&[ + ("GET", "/api/v1"), + ("GET", "/api/v1"), + ("GET", TARGET_PATH), + ("PATCH", SYNC_PATH), + ]); + assert_eq!( + requests[3].body(), + &json!({"metadata": {"finalizers": ["example.com/keep"]}}) + ); + } + + #[tokio::test] + async fn source_failure_is_returned_and_written_to_status() { + let mut sync = sync_fixture(); + sync.metadata.finalizers = Some(vec![FINALIZER.into()]); + let mut live = sync.clone(); + live.status = status_with_condition("True"); + let mock = MockApi::new(vec![ + discovery_response("ConfigMap", "configmaps", true), + discovery_response("ConfigMap", "configmaps", true), + api_error(404), + response(200, json!(live)), + response(200, json!(live)), + ]); + let error = reconcile(Arc::new(sync), context(mock.client.clone())) + .await + .expect_err("source absent"); + let message = error.to_string(); + assert!( + matches!(error, Error::ResourceNotFoundError(name, kind, kube::Error::Api(error)) + if name == "source-config" && kind == "ConfigMap" && error.code == 404) + ); + let requests = mock.finish(&[ + ("GET", "/api/v1"), + ("GET", "/api/v1"), + ("GET", SOURCE_PATH), + ("GET", STATUS_PATH), + ("PATCH", STATUS_PATH), + ]); + let condition = &requests[4].body()["status"]["conditions"][0]; + assert_eq!(condition["status"], "True"); + assert_eq!(condition["message"], message); + assert_eq!(condition["observedGeneration"], 7); + assert_eq!(condition["lastTransitionTime"], json!(*EPOCH)); + } + + #[rstest] + #[case::deleting_target_api_failure(true, true, false, true)] + #[case::deleting_source_api_failure(true, true, true, true)] + #[case::disabled_force_delete(true, false, false, false)] + #[case::active_sync(false, true, false, false)] + #[tokio::test] + async fn force_delete_only_bypasses_api_resolution_for_deleting_syncs( + #[case] deleted: bool, + #[case] force: bool, + #[case] source_failure: bool, + #[case] removed: bool, + ) { + let mut sync = sync_fixture(); + sync.metadata.deletion_timestamp = deleted.then(|| EPOCH.clone()); + sync.metadata.finalizers = Some(vec![FINALIZER.into(), "example.com/keep".into()]); + sync.metadata.annotations = Some(std::collections::BTreeMap::from([( + crate::resources::FORCE_DELETE_ANNOTATION.into(), + force.to_string(), + )])); + let mut responses = vec![]; + let mut expected = vec![]; + if source_failure { + responses.push(discovery_response("ConfigMap", "configmaps", true)); + expected.push(("GET", "/api/v1")); + } + responses.push(api_error(403)); + expected.push(("GET", "/api/v1")); + if removed { + responses.push(response(200, json!(sync))); + expected.push(("PATCH", SYNC_PATH)); + } + let mock = MockApi::new(responses); + let parent_api = sync.api(mock.client.clone()); + let result = reconcile_helper( + Arc::new(sync), + context(mock.client.clone()), + &"copy-config".into(), + &parent_api, + ) + .await; + if removed { + assert_eq!(result.expect("force cleanup"), Action::await_change()); + } else { + assert!( + matches!(result.expect_err("API resolution failure"), Error::KubeError(kube::Error::Api(error)) if error.code == 403) + ); + } + let requests = mock.finish(&expected); + if removed { + assert_eq!( + requests.last().expect("finalizer patch").body(), + &json!({"metadata": {"finalizers": ["example.com/keep"]}}) + ); + } + } + + #[rstest] + #[case::no_finalizers(None, false, Some("Foreground"))] + #[case::empty_finalizers(Some(vec![]), false, Some("Foreground"))] + #[case::target_finalizer(Some(vec!["example.com/target"]), false, Some("Background"))] + #[case::already_deleting(Some(vec!["example.com/target"]), true, None)] + #[tokio::test] + async fn target_deletion_waits_for_absence( + #[case] finalizers: Option>, + #[case] deleting: bool, + #[case] propagation: Option<&str>, + ) { + let mut sync = sync_fixture(); + sync.metadata.finalizers = Some(vec![FINALIZER.into()]); + sync.metadata.deletion_timestamp = Some(EPOCH.clone()); + let target = json!({"apiVersion": "v1", "kind": "ConfigMap", "metadata": { + "name": "target-config", "finalizers": finalizers, "deletionTimestamp": deleting.then(|| EPOCH.clone())}}); + let mut responses = vec![ + discovery_response("ConfigMap", "configmaps", true), + response(200, target.clone()), + ]; + let mut expected = vec![("GET", "/api/v1"), ("GET", TARGET_PATH)]; + if propagation.is_some() { + responses.push(response(200, target)); + expected.push(("DELETE", TARGET_PATH)); + } + let mock = MockApi::new(responses); + let ctx = context(mock.client.clone()); + let _cancelled = + crate::remote_watcher_manager::tests::park_watchers(&ctx.remote_watcher_manager, &sync) + .await; + let parent = sync.api(mock.client.clone()); + let target_api = sync + .spec + .target + .api_for(mock.client.clone(), "team-a") + .await + .expect("target API"); + let result = reconcile_deleted_resource( + Arc::new(sync), + "copy-config", + target_api, + &parent, + Arc::clone(&ctx), + ) + .await; + stop_watches(&ctx).await; + assert_eq!(result.expect("request deletion"), Action::await_change()); + let requests = mock.finish(&expected); + if let Some(propagation) = propagation { + assert_eq!(requests[2].body()["propagationPolicy"], propagation); + } + } + + #[rstest] + #[case::without_our_finalizer(false, false)] + #[case::deletion_disabled(true, true)] + #[tokio::test] + async fn cleanup_can_skip_target_requests(#[case] finalizer: bool, #[case] disabled: bool) { + let mut sync = sync_fixture(); + sync.metadata.finalizers = Some(if finalizer { + vec![FINALIZER.into()] + } else { + vec!["example.com/other".into()] + }); + sync.metadata.annotations = Some(std::collections::BTreeMap::from([( + crate::resources::DISABLE_TARGET_DELETION_ANNOTATION.into(), + disabled.to_string(), + )])); + let mut responses = vec![discovery_response("ConfigMap", "configmaps", true)]; + let mut expected = vec![("GET", "/api/v1")]; + if disabled { + responses.push(response(200, json!(sync))); + expected.push(("PATCH", SYNC_PATH)); + } + let mock = MockApi::new(responses); + let parent = sync.api(mock.client.clone()); + let target = sync + .spec + .target + .api_for(mock.client.clone(), "team-a") + .await + .expect("target API"); + assert_eq!( + reconcile_deleted_resource( + Arc::new(sync), + "copy-config", + target, + &parent, + context(mock.client.clone()) + ) + .await + .expect("cleanup"), + Action::await_change() + ); + let requests = mock.finish(&expected); + if disabled { + assert_eq!(requests[1].body(), &json!({"metadata": {"finalizers": []}})); + } + } + + #[rstest] + #[case::whole_resource(false, false)] + #[case::mapped_resource(true, false)] + #[case::remote_target(false, true)] + #[tokio::test] + async fn target_apply_uses_forced_field_manager_and_local_ownership( + #[case] mapped: bool, + #[case] remote: bool, + ) { + let mut sync = sync_fixture(); + if mapped { + sync.spec.mappings = vec![crate::resources::Mapping { + from_field_path: Some("data.original".into()), + to_field_path: Some("data.copied".into()), + }]; + } + let source = json!({"apiVersion": "v1", "kind": "ConfigMap", "metadata": {"name": "source-config"}, "data": {"original": "value"}}); + let mock = MockApi::new(vec![ + discovery_response("ConfigMap", "configmaps", true), + discovery_response("ConfigMap", "configmaps", true), + response(200, source.clone()), + response(200, source), + ]); + let source_api = sync + .spec + .source + .api_for(mock.client.clone(), "team-a") + .await + .expect("source API"); + let target_api = sync + .spec + .target + .api_for(mock.client.clone(), "team-a") + .await + .expect("target API"); + // API resolution is tested separately; this flag determines target ownership. + if remote { + sync.spec.target.cluster = Some(Default::default()); + } + let ctx = context(mock.client.clone()); + let _cancelled = + crate::remote_watcher_manager::tests::park_watchers(&ctx.remote_watcher_manager, &sync) + .await; + let result = reconcile_normally( + Arc::new(sync), + "copy-config", + source_api, + target_api, + Arc::clone(&ctx), + ) + .await; + stop_watches(&ctx).await; + assert_eq!(result.expect("apply target"), Action::await_change()); + let requests = mock.finish(&[ + ("GET", "/api/v1"), + ("GET", "/api/v1"), + ("GET", SOURCE_PATH), + ("PATCH", TARGET_PATH), + ]); + let patch = &requests[3]; + let query = patch.uri().query().expect("apply parameters"); + assert!(query.split('&').any(|part| part == "force=true")); + assert!(query + .split('&') + .any(|part| part == "fieldManager=sinker.influxdata.io")); + assert_eq!( + patch.headers()["content-type"], + "application/apply-patch+yaml" + ); + assert_eq!(patch.body()["metadata"]["name"], "target-config"); + assert_eq!(patch.body()["metadata"]["namespace"], "team-a"); + assert_eq!( + patch.body()["data"], + if mapped { + json!({"copied": "value"}) + } else { + json!({"original": "value"}) + } + ); + if remote { + assert!(patch.body()["metadata"].get("ownerReferences").is_none()); + } else { + assert_eq!( + patch.body()["metadata"]["ownerReferences"], + json!([{ + "apiVersion": "sinker.influxdata.io/v1alpha1", "kind": "ResourceSync", "name": "copy-config", + "uid": "sync-uid", "controller": false, "blockOwnerDeletion": true}]) + ); + } + } + + #[test] + fn deletion_errors_still_update_status_and_ignore_unrelated_conditions() { + let mut live = status_with_condition("True").expect("status fixture"); + live.conditions.as_mut().expect("conditions")[0].type_ = "OtherCondition".into(); + let mut sync = resource_sync(true, None); + sync.metadata.generation = Some(9); + let before = chrono::Utc::now(); + let condition = single_condition(reconcile_status( + &sync, + &Some(live), + &Err(Error::NamespaceRequired), + )); + let after = chrono::Utc::now(); + assert_eq!(condition.status, "True"); + assert_eq!(condition.observed_generation, Some(9)); + assert_eq!(condition.message, "Namespace is required"); + assert!((before..=after).contains(&condition.last_transition_time.0)); + } + + #[rstest] + #[case::missing_name(true)] + #[case::missing_namespace(false)] + #[tokio::test] + async fn reconciliation_requires_identity_before_api_access(#[case] missing_name: bool) { + let mut sync = sync_fixture(); + let mock = MockApi::new(vec![]); + let ctx = context(mock.client.clone()); + if missing_name { + sync.metadata.name = None; + assert!(matches!( + reconcile(Arc::new(sync), ctx) + .await + .expect_err("name required"), + Error::NameRequired + )); + } else { + sync.metadata.namespace = None; + let parent = sync.api(mock.client.clone()); + assert!(matches!( + reconcile_helper(Arc::new(sync), ctx, &"copy-config".into(), &parent) + .await + .expect_err("namespace required"), + Error::NamespaceRequired + )); + } + mock.finish(&[]); + } + + #[rstest] + #[case::get_target(false)] + #[case::delete_target(true)] + #[tokio::test] + async fn force_delete_does_not_bypass_target_request_errors(#[case] delete: bool) { + let mut sync = sync_fixture(); + sync.metadata.deletion_timestamp = Some(EPOCH.clone()); + sync.metadata.finalizers = Some(vec![FINALIZER.into()]); + sync.metadata.annotations = Some(std::collections::BTreeMap::from([( + crate::resources::FORCE_DELETE_ANNOTATION.into(), + "true".into(), + )])); + let mut responses = vec![discovery_response("ConfigMap", "configmaps", true)]; + let mut expected = vec![("GET", "/api/v1"), ("GET", TARGET_PATH)]; + if delete { + responses.push(response( + 200, + json!({"metadata": {"name": "target-config"}}), + )); + expected.push(("DELETE", TARGET_PATH)); + } + responses.push(api_error(403)); + let mock = MockApi::new(responses); + let target = sync + .spec + .target + .api_for(mock.client.clone(), "team-a") + .await + .expect("target API"); + let parent = sync.api(mock.client.clone()); + let error = reconcile_deleted_resource( + Arc::new(sync), + "copy-config", + target, + &parent, + context(mock.client.clone()), + ) + .await + .expect_err("target request denied"); + assert!(matches!(error, Error::KubeError(kube::Error::Api(error)) if error.code == 403)); + mock.finish(&expected); + } + + #[rstest] + #[case::missing_owner_uid(false)] + #[case::apply_denied(true)] + #[tokio::test] + async fn target_write_failures_are_returned(#[case] uid_present: bool) { + let mut sync = sync_fixture(); + if !uid_present { + sync.metadata.uid = None; + } + let mut responses = vec![ + discovery_response("ConfigMap", "configmaps", true), + discovery_response("ConfigMap", "configmaps", true), + response(200, json!({"data": {"key": "value"}})), + ]; + let mut expected = vec![("GET", "/api/v1"), ("GET", "/api/v1"), ("GET", SOURCE_PATH)]; + if uid_present { + responses.push(api_error(409)); + expected.push(("PATCH", TARGET_PATH)); + } + let mock = MockApi::new(responses); + let ctx = context(mock.client.clone()); + let source = sync + .spec + .source + .api_for(mock.client.clone(), "team-a") + .await + .expect("source API"); + let target = sync + .spec + .target + .api_for(mock.client.clone(), "team-a") + .await + .expect("target API"); + let result = reconcile_normally( + Arc::new(sync), + "copy-config", + source, + target, + Arc::clone(&ctx), + ) + .await; + stop_watches(&ctx).await; + let error = result.expect_err("target write failure"); + if uid_present { + assert!( + matches!(error, Error::KubeError(kube::Error::Api(error)) if error.code == 409) + ); + } else { + assert!(matches!(error, Error::UIDRequired)); + } + mock.finish(&expected); + } + + #[rstest] + #[case::status_read(false)] + #[case::status_write(true)] + #[tokio::test] + async fn status_api_errors_are_returned(#[case] write: bool) { + let sync = sync_fixture(); + let mut responses = vec![ + discovery_response("ConfigMap", "configmaps", true), + discovery_response("ConfigMap", "configmaps", true), + response(200, json!(sync)), + ]; + let mut expected = vec![ + ("GET", "/api/v1"), + ("GET", "/api/v1"), + ("PATCH", SYNC_PATH), + ("GET", STATUS_PATH), + ]; + if write { + responses.push(response(200, json!(sync))); + expected.push(("PATCH", STATUS_PATH)); + } + responses.push(api_error(403)); + let mock = MockApi::new(responses); + let error = reconcile(Arc::new(sync), context(mock.client.clone())) + .await + .expect_err("status request denied"); + assert!(matches!(error, Error::KubeError(kube::Error::Api(error)) if error.code == 403)); + mock.finish(&expected); + } + + #[tokio::test] + async fn reconciliation_errors_retry_after_five_seconds() { + let mock = MockApi::new(vec![]); + assert_eq!( + super::error_policy( + Arc::new(sync_fixture()), + &Error::NamespaceRequired, + context(mock.client.clone()) + ), + Action::requeue(Duration::from_secs(5)) + ); + mock.finish(&[]); + } - static NOW: Lazy