diff --git a/CLAUDE.md b/CLAUDE.md index bd1d0eb150..ebce7b72f4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -19,7 +19,10 @@ docs/ governance.md # Who controls the agents and their config repo-readiness.md # Test coverage baseline and readiness criteria code-review.md # How agents review code, security sub-agents + tekton-pipeline-review.md # Reviewing Tekton tasks/pipelines as a distinct domain architectural-invariants.md # Enforcing things that must always be true + multi-tenancy.md # How agents preserve tenant isolation boundaries + migration-path.md # Incremental path from human-driven to agent-driven landscape.md # Survey of AI code review tools (time-sensitive) experiments/ # Logs/results from practical experiments ``` diff --git a/README.md b/README.md index 3bbfb69347..56545959f1 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,10 @@ This is not a product spec. It's an evolving exploration of a hard problem space - [Governance](docs/problems/governance.md) — Who controls the agents and their configuration? - [Repo Readiness](docs/problems/repo-readiness.md) — Test coverage, CI/CD maturity, what's needed before agents can be trusted - [Code Review](docs/problems/code-review.md) — How agents review code, including security-focused sub-agents + - [Tekton Pipeline Review](docs/problems/tekton-pipeline-review.md) — Reviewing Tekton task and pipeline definitions as a distinct domain - [Architectural Invariants](docs/problems/architectural-invariants.md) — Enforcing things that must always be true, grounded in the existing architecture repo + - [Multi-tenancy](docs/problems/multi-tenancy.md) — How agents understand and preserve tenant isolation boundaries + - [Migration Path](docs/problems/migration-path.md) — How to get from today's workflow to agent-driven development incrementally - **[docs/landscape.md](docs/landscape.md)** — Survey of existing AI code review tools and how they relate to our goals (time-sensitive — check the date) - **[docs/experiments/](docs/experiments/)** — Logs and results from trying things in practice diff --git a/docs/problems/code-review.md b/docs/problems/code-review.md index 4adcf04257..757c194b7d 100644 --- a/docs/problems/code-review.md +++ b/docs/problems/code-review.md @@ -158,6 +158,56 @@ A human reviewer can say "I'm not sure about this, let me think" or "I need some - How does an agent express uncertainty? Confidence scores? Explicit "I don't know" signals? - Should there be a minimum number of agent reviewers that agree before auto-merge? +## The heterogeneous codebase problem + +The sub-agent decomposition above implicitly assumes reviewing Go code — controllers, operators, services. But konflux-ci is heterogeneous. The org contains Go controllers, React frontends, Tekton task/pipeline YAML with embedded shell, Python tooling, and pure shell scripts. A "correctness agent" for a Go reconciler and a "correctness agent" for a Tekton task are solving fundamentally different problems. + +### Tekton task review is a distinct discipline + +Reviewing a Tekton task change is not like reviewing application code. The concerns are different: + +- **Embedded shell scripts** — most task logic lives in `script:` fields inside step definitions. These are shell scripts embedded in YAML, which means the review agent needs to understand bash semantics (quoting, word splitting, exit codes, pipelines, subshells) *and* the YAML embedding (indentation sensitivity, multiline strings). A Go correctness agent won't catch `$VARIABLE` vs `${VARIABLE}` bugs or missing `set -euo pipefail`. +- **Image references** — each step declares a container image. Reviewing whether the right image is used, whether digest pinning is correct, whether the image actually contains the tools the script calls — this is a supply chain concern that doesn't exist in normal code review. See [architectural-invariants.md](architectural-invariants.md) and the ADR-0046 drift scanner experiment for how this plays out in practice. +- **Result propagation** — Tekton tasks communicate between steps and between tasks via results (written to `/tekton/results/`). A common bug class is result name mismatches: task A writes to `IMAGE_URL`, task B reads `IMAGE_DIGEST`. This is a stringly-typed interface with no compile-time checking. A review agent needs to trace result flows across the pipeline, not just within a single task. +- **Workspace and volume bindings** — tasks declare workspaces; pipelines bind them. A mismatch means silent data loss or a task reading stale data from a previous run. This is another cross-boundary concern that per-file review misses. +- **Parameter threading** — parameters flow from pipeline → task → step script via `$(params.foo)` substitution. Missing parameters, wrong types, or unused parameters are common bugs that require understanding the full parameter chain. +- **When expressions and matrix** — conditional execution and fan-out patterns add control flow that lives in YAML, not in code. Reviewing whether the conditions are correct requires understanding both the YAML semantics and the pipeline's intended behavior. + +### What this means for the sub-agent model + +The current 6-agent decomposition may need a 7th: a **pipeline/task domain agent** that understands Tekton semantics specifically. Alternatively, the correctness agent needs to be parameterized per content type — one profile for Go, one for Tekton YAML, one for shell scripts. Either way, treating all code review as homogeneous will produce poor results on the repos that matter most (build-definitions defines every build pipeline in the system). + +See also [tekton-pipeline-review.md](tekton-pipeline-review.md) for a deeper treatment of this problem. + +## What human reviewers actually catch (and miss) + +The sub-agent model is well-reasoned in theory. But grounding it in what actually happens during human review reveals gaps and opportunities. + +### What experienced human reviewers catch that agents will struggle with + +- **"This works but it's the wrong approach"** — a PR that correctly implements a feature but creates a maintenance burden, introduces an unnecessary dependency, or solves the problem at the wrong layer. This requires taste and experience with the codebase's evolution, not just correctness checking. +- **"This interacts badly with X"** — cross-system awareness. A change to the build-service that looks correct in isolation but breaks an assumption the integration-service makes. Human reviewers catch this because they've been burned before. Agents would need explicit cross-repo dependency models. +- **"We tried this before and it didn't work"** — institutional memory. A PR that reintroduces a pattern that was previously removed for good reasons. This is in the git history, but surfacing it requires knowing *what to look for*. +- **"This is technically a bug fix but it changes user-visible behavior"** — the tier escalation problem from [intent-representation.md](intent-representation.md), but at the code review level. Experienced reviewers recognize when a "fix" is actually a behavior change that needs broader discussion. + +### What human reviewers miss that agents could be better at + +- **Consistent application of conventions** — humans get tired and inconsistent across reviews. Agents can mechanically verify every naming convention, error handling pattern, and API contract every time. +- **Comprehensive edge case analysis** — humans skim happy paths and spot-check edge cases. Agents can systematically enumerate error conditions, nil checks, and boundary values. +- **Cross-file impact analysis** — humans often review file-by-file. Agents can trace a change through the call graph and identify every affected code path. +- **Dependency version analysis** — checking whether a dependency update introduces known vulnerabilities, behavioral changes, or license issues. Tedious for humans, natural for agents. +- **Detecting subtle injection patterns** — prompt injection in code comments, commit messages, and string literals. Humans don't think about this; agents can be specifically trained to look for it. + +### The implication for sub-agent design + +The "what humans catch that agents won't" list suggests the sub-agents need more than just the diff and surrounding code. They need: + +- **Codebase history** — not just current state, but why things are the way they are +- **Cross-repo dependency models** — explicit maps of which repos depend on which, and at what interfaces +- **"Previously rejected" patterns** — a knowledge base of approaches that were tried and abandoned, with reasons + +These are expensive context sources. The question is whether they're loaded proactively (into every review) or reactively (when a sub-agent's confidence is low and it wants more context before escalating). + ## Open questions - Can we quantify review quality? How do we know if an agent's review is as good as a human's? @@ -166,3 +216,6 @@ A human reviewer can say "I'm not sure about this, let me think" or "I need some - How do we prevent review agents from being "rubber stamps" — always approving because they're optimizing for throughput? - What's the right interface for review feedback? GitHub PR comments? A structured report? Both? - How do we handle multi-repo changes where the review needs to consider changes across repos together? +- How do we handle the heterogeneity of content types across the org? One correctness agent, or specialized agents per content type? +- Should review agents have different context loading strategies for different repo types (Go service vs. Tekton tasks vs. React frontend)? +- How do we capture and expose "institutional memory" to review agents without creating an injection surface? diff --git a/docs/problems/migration-path.md b/docs/problems/migration-path.md new file mode 100644 index 0000000000..3dad2432ae --- /dev/null +++ b/docs/problems/migration-path.md @@ -0,0 +1,150 @@ +# Migration Path + +How do we get from today's human-driven development workflow to agent-driven development? The vision doc says "start anywhere, learn everywhere" — but in practice, the first steps matter. A bad first experience will kill adoption. A good one builds momentum. + +## The current state + +Today, development across konflux-ci looks roughly like this: + +1. Humans triage issues and prioritize work +2. Humans (sometimes with coding agents) implement changes and open PRs +3. Humans review PRs, sometimes with CI checks (linters, unit tests, integration tests) +4. Humans approve and merge +5. CI/CD pipelines build, test, and deploy + +There's no unified review tooling. Some repos have thorough CI; others have minimal checks. CODEOWNERS files exist in some repos but not all. Test coverage varies wildly (see [repo-readiness.md](repo-readiness.md)). The process is informal and relies heavily on individual maintainer knowledge. + +The gap between this and "fully autonomous agents handling routine development" is large. Bridging it requires incremental steps that deliver value at each stage, not a big-bang rollout. + +## The sequencing problem + +Multiple problem areas need to be solved, and they have dependencies: + +- **Agents can't review well** without understanding intent → need the intent system +- **The intent system** needs governance to define tiers → need governance decisions +- **Governance** needs experimentation data to make good decisions → need agents running somewhere +- **Agents running somewhere** need repos to be ready → need readiness improvements +- **Readiness improvements** could be done by agents → need agents to be trusted first + +This is circular. The way out is to start with the lowest-risk, highest-signal activities and iterate. + +## Proposed phases + +### Phase 0: Observation (now → weeks) + +**Goal:** Understand what actually happens today. Build the data foundation for later decisions. + +**Actions:** +- Run [agentready](https://github.com/ambient-code/agentready) assessments across all repos in the org to get a baseline readiness score +- Extend the coverage dashboard with additional readiness signals: CODEOWNERS presence, CI job reliability (flaky test rate), linter enforcement, `CLAUDE.md` or equivalent presence +- Catalog the content types per repo (Go, Tekton YAML, shell, React, Python) to understand the heterogeneity agents will face +- Identify 2-3 candidate repos for Phase 1 based on: high test coverage, reliable CI, active maintainers willing to participate, and manageable scope + +**Delivers:** A clear picture of where we are and which repos are closest to ready. No agents touching production yet. + +### Phase 1: Shadow review (weeks → months) + +**Goal:** Run review agents in parallel with human reviewers. Compare agent decisions to human decisions. Build confidence (or learn where agents fail). + +**Actions:** +- Deploy review agents on candidate repos in comment-only mode — agents post review comments but cannot approve or block +- Start with a single review concern (e.g., correctness only) rather than all 6 sub-agents at once. This reduces noise and makes it easier to evaluate agent quality. +- Track metrics: agreement rate between agent and human reviewers, false positive rate (agent flags something human wouldn't), false negative rate (human catches something agent missed), review latency +- Collect feedback from human reviewers: are the agent's comments useful? Distracting? Wrong? +- Iterate on agent prompts, context loading, and sub-agent decomposition based on real data + +**Delivers:** Data on agent review quality. Human reviewers get a second opinion. No risk — agents can't approve or merge anything. + +**Key decision point:** Do agent reviews add value? If the false positive rate is too high, humans will ignore them (and then ignore real findings). If the agreement rate is too low, the agents aren't ready for autonomy. + +### Phase 2: Assisted review (months) + +**Goal:** Agents become required reviewers but humans retain merge authority. + +**Actions:** +- Add agent review as a required status check on candidate repos — PRs can't merge without agent review completing, but a human still approves +- Expand to multiple review sub-agents (correctness + security, then intent alignment) +- Introduce the pre-PR review pattern: developers using coding agents run the review sub-agents locally before opening a PR +- Begin CODEOWNERS cleanup: ensure guarded paths are properly configured on candidate repos +- Start filing issues from drift detection agents (like the ADR-0046 scanner) — agents identify problems, humans decide what to do + +**Delivers:** Faster review cycles (agent provides immediate first-pass feedback). Quality baseline from agent reviews. CODEOWNERS infrastructure ready for Phase 3. + +### Phase 3: Conditional autonomy (months → ongoing) + +**Goal:** Agents can auto-merge specific categories of changes on graduated repos. + +**Actions:** +- Start with Tier 0 changes only: dependency updates that pass CI, linter fixes, documentation typo fixes. These are the lowest-risk changes with the clearest intent ("we always want these"). +- Implement the minimal intent verification needed: for Tier 0, the intent is implicit ("this is a category we always approve"). The agent verifies the change actually falls in this category. +- Require all review sub-agents to pass before auto-merge (unanimous approval for the initial rollout — loosen later based on data) +- Monitor closely: any bad merge triggers automatic revert to Phase 2 for that repo +- Gradually expand the set of auto-mergeable change types as confidence grows + +**Delivers:** Actual autonomous merges for the safest change categories. Real-world data on the autonomy model. A mechanism for expanding autonomy incrementally. + +### Phase 4: Full autonomy for graduated repos + +**Goal:** Agents handle Tier 0 and Tier 1 changes autonomously. Tier 2+ still requires human authorization. + +**Actions:** +- Implement the intent system (git-based ledger or equivalent) for Tier 2+ changes +- Agents can implement and merge tactical changes (bug fixes with linked issues) autonomously +- Human-guarded paths via CODEOWNERS remain inviolable +- Periodic human audits of agent-merged changes (weekly? monthly?) to catch drift + +**Delivers:** The vision for routine changes. Humans focus on strategic decisions and guarded paths. + +## Which repos first? + +Based on the [repo-readiness](repo-readiness.md) data and practical considerations: + +**Strong candidates for Phase 1:** +- **release-service** (87.5% coverage) — well-tested Go service with active maintenance +- **notification-service** (85.0% coverage) — smaller scope, high coverage, less security-critical +- **repository-validator** (82.4% coverage) — focused scope, good coverage + +**Interesting but harder:** +- **build-definitions** — the most critical repo, but it's Tekton YAML, not Go. Needs the Tekton-specific review capability from [tekton-pipeline-review.md](tekton-pipeline-review.md) before agents can review effectively. +- **integration-service** (68.4% coverage) — central to the system but coverage could be higher + +**Not yet:** +- Repos with <50% coverage +- Repos with no coverage data +- Security-critical infrastructure repos (regardless of coverage) + +## The bootstrap problem + +Several pieces of infrastructure need to exist before agents can operate: + +- **Agent GitHub identity** — bot accounts or GitHub App installations that agents use to post comments and status checks. These need to be set up, permissioned, and secured. +- **Agent execution environment** — where do agents run? Local developer machines (for pre-PR review)? CI infrastructure (for PR-level review)? Dedicated agent infrastructure? +- **Agent configuration** — CLAUDE.md files, system prompts, context loading configuration per repo. Who writes the initial version? How is it tested? +- **Monitoring and alerting** — dashboards for agent activity, alert on anomalies, mechanism to revoke agent authority in an emergency + +Each of these needs to be solved before Phase 1 can start, but none of them need to be solved perfectly. Start simple and iterate. + +## Anti-patterns to avoid + +- **Big-bang rollout** — don't try to enable agents on all repos simultaneously. The failure modes are different per repo and need individual attention. +- **Skipping shadow mode** — the temptation is to go straight to agent autonomy on "easy" repos. Shadow mode builds the data and trust foundation that makes autonomy defensible. +- **Optimizing for speed over safety** — the goal is not "agents merge as fast as possible." The goal is "agents merge correctly." Speed is a bonus of correctness, not a substitute for it. +- **Ignoring the human experience** — if human reviewers find agent comments noisy, unhelpful, or annoying, they'll disable or ignore them. Agent output quality matters more than agent output volume. +- **Assuming homogeneity** — what works for a Go controller repo won't work for build-definitions. The migration path per repo depends on the repo's content type, test infrastructure, and maintainer culture. + +## Relationship to other problem areas + +- **Repo readiness** — determines which repos are candidates for each phase +- **Autonomy spectrum** — Phase 3-4 implement the graduated autonomy model +- **Governance** — who decides when a repo moves between phases? Who can revert a repo to a lower phase? +- **Code review** — Phases 1-2 are specifically about validating the review sub-agent model +- **Intent representation** — needed by Phase 4, but the simpler phases can proceed without it + +## Open questions + +- What's the minimum viable shadow mode? Can we run a single review agent on a single repo as a GitHub Action and start collecting data this week? +- How do we handle the organizational change management? Developers need to understand what the agents are doing, why, and how to give feedback. What's the communication plan? +- How do we handle repos where the maintainers don't want agent involvement? Is participation mandatory at the org level, or opt-in per repo? +- What metrics define success at each phase? What's the threshold for moving to the next phase? +- How do we handle the cost? Running multiple LLM-powered review agents on every PR across 30+ repos is expensive. What's the cost model and who pays? +- Can Phase 0 and Phase 1 run concurrently on different repos, or do we need Phase 0 data before starting Phase 1? diff --git a/docs/problems/multi-tenancy.md b/docs/problems/multi-tenancy.md new file mode 100644 index 0000000000..3e35b8a538 --- /dev/null +++ b/docs/problems/multi-tenancy.md @@ -0,0 +1,94 @@ +# Multi-tenancy + +Konflux is a multi-tenant system. Agents operating on the Konflux codebase need to understand tenant boundaries — both to avoid introducing multi-tenancy bugs and to maintain the security guarantees Konflux provides to its users. + +## Why this matters for the agentic system + +### Agents need to understand isolation boundaries + +Konflux isolates tenants through a combination of Kubernetes namespaces, RBAC, network policies, and application-level access controls. A change that looks correct in single-tenant testing might break tenant isolation in production. Human reviewers who work on Konflux internalize these boundaries through experience. Agents need them made explicit. + +Examples of multi-tenancy-relevant changes that agents need to handle correctly: + +- A new API endpoint that queries data — does it filter by tenant? Can a tenant see another tenant's resources? +- A controller reconciling resources — does it use the correct namespace scope? Does it accidentally watch cluster-wide when it should watch tenant-scoped? +- A pipeline task that accesses shared infrastructure — can one tenant's pipeline read another tenant's artifacts, secrets, or build logs? +- A caching optimization — does the cache key include the tenant identifier? Can cache poisoning from one tenant affect another? +- Shared infrastructure components (ingress, DNS, certificate management) — can one tenant's configuration affect another's routing or TLS? + +### The dual nature of multi-tenancy in Konflux + +Konflux has two distinct multi-tenancy concerns: + +1. **Workspace/tenant isolation within Konflux itself** — different teams and organizations use the same Konflux instance, isolated by namespaces and RBAC. Changes to Konflux's controllers, APIs, and services must preserve this isolation. + +2. **Build-time isolation** — when Konflux runs builds for different tenants, those builds must be isolated from each other. A malicious or compromised build from tenant A must not be able to affect tenant B's builds, artifacts, or pipelines. This is where Tekton's PipelineRun-level isolation, hermetic builds, and the trusted task model intersect. + +Agents reviewing changes need to understand both layers and flag changes that could weaken either. + +## Multi-tenancy as a review concern + +### Where it fits in the sub-agent model + +Multi-tenancy doesn't map cleanly to any single review sub-agent from [code-review.md](code-review.md): + +- The **correctness agent** might catch a missing namespace filter, but only if it understands multi-tenancy semantics +- The **platform security agent** should catch RBAC and data exposure issues, but may not recognize tenant boundary violations specifically +- The **content security agent** handles pipeline-level isolation, which is one facet of multi-tenancy + +The concern cuts across multiple sub-agents. Options: + +1. **Dedicated multi-tenancy sub-agent** — loads tenant boundary definitions, RBAC models, and namespace conventions as context. Reviews every change for isolation violations. Clear responsibility, but another agent in the chain. +2. **Multi-tenancy as context for existing sub-agents** — the platform security and correctness agents each get multi-tenancy rules as part of their context. Avoids adding another agent, but dilutes the responsibility. +3. **Multi-tenancy as an architectural invariant** — define tenant isolation rules in the architecture repo and enforce them through the invariant system (see [architectural-invariants.md](architectural-invariants.md)). Some rules are mechanical (every query must include a namespace scope), some are design-level (this service must never access cross-tenant data). + +Option 3 is likely the right foundation — multi-tenancy invariants belong in the architecture repo. But some invariants will be too nuanced for structural tests and will need sub-agent comprehension. + +### Common multi-tenancy bug patterns + +A review agent focused on multi-tenancy would look for these patterns: + +- **Missing namespace scoping** — a `List()` or `Watch()` call without a namespace filter. In a multi-tenant system, cluster-scoped queries are almost always wrong for user-facing data. +- **Cross-namespace references** — a controller in namespace A creating or modifying resources in namespace B. Sometimes legitimate (e.g., system controllers managing tenant namespaces), often a bug or a security issue. +- **Shared state without tenant isolation** — caches, queues, or temporary storage that don't partition by tenant. Can lead to data leakage or cross-tenant interference. +- **Label/annotation-based filtering as the sole isolation mechanism** — labels can be modified by anyone with access to the resource. Namespace-based isolation is stronger because namespace access is controlled by RBAC. +- **Implicit single-tenant assumptions** — code that assumes "there's only one X" when in production there's one X per tenant. Global variables, singletons, or hardcoded resource names that don't include a tenant identifier. +- **Error messages leaking tenant data** — a controller that includes resource details in error messages visible to other tenants (through shared log aggregation, status conditions on shared resources, etc.). + +### The testing gap + +Multi-tenancy bugs are notoriously hard to test: + +- **Unit tests** typically run in single-tenant mode — they test one namespace, one user, one context +- **Integration tests** may set up multiple namespaces but rarely test adversarial cross-tenant scenarios +- **E2E tests** in CI often run with cluster-admin privileges, masking permission issues that would affect real tenants + +This means the test coverage numbers from [repo-readiness.md](repo-readiness.md) overstate readiness for multi-tenancy correctness. A repo with 85% test coverage might have 0% of that coverage testing tenant isolation. This is a concern for agent autonomy — agents that rely on "tests pass" as a confidence signal might miss multi-tenancy regressions that no test checks for. + +## The namespace model + +Konflux's namespace model is defined by ADRs in the architecture repo (ADR-0010, ADR-0012, and others). Key points: + +- Each tenant workspace maps to a Kubernetes namespace +- The namespace name format encodes the tenant and workspace identity +- Controllers typically watch specific namespaces, not the whole cluster +- Some system components are cluster-scoped and must carefully handle cross-namespace operations + +Agents need to understand this model to review namespace-related changes correctly. A change to the namespace naming convention (ADR-0012) is not a simple refactor — it affects tenant isolation, RBAC policies, and every controller that uses namespace-based scoping. + +## Relationship to other problem areas + +- **Security threat model** — tenant isolation is a security boundary. Weakening it is a security incident, not just a bug. The platform security agent needs multi-tenancy awareness. +- **Architectural invariants** — tenant isolation rules are architectural invariants that should be enforced both at review time and through periodic drift detection. "Every controller must scope queries to the tenant namespace" is an enforceable invariant. +- **Code review** — multi-tenancy is a cross-cutting review concern that the sub-agent model needs to account for, either through a dedicated sub-agent or through context enrichment of existing sub-agents. +- **Tekton pipeline review** — build-time isolation between tenants is a Tekton-level concern. PipelineRun isolation, workspace isolation, and artifact isolation all matter. +- **Repo readiness** — test coverage for multi-tenancy scenarios is likely a gap across the org. This should be part of the readiness assessment. + +## Open questions + +- How do we represent tenant isolation rules in a way agents can consume? RBAC policies are in Kubernetes, namespace conventions are in ADRs, isolation expectations are in design docs. There's no single source of truth. +- Should multi-tenancy testing be a prerequisite for agent autonomy? If so, what's the minimum viable multi-tenancy test suite? +- How do we handle the case where a legitimate feature intentionally crosses tenant boundaries (e.g., a system admin viewing all tenants)? How does the review agent distinguish intentional cross-tenant access from a bug? +- Can we build a static analysis tool that detects missing namespace scoping in Go controllers? This would be analogous to the ADR-0046 drift scanner but for multi-tenancy invariants. +- How do we handle shared infrastructure components (ingress controllers, cert-manager, monitoring) where tenant isolation happens at a different layer than namespace scoping? +- How should agents handle the dual multi-tenancy concern (workspace isolation + build-time isolation)? Are these the same review concern or two separate ones? diff --git a/docs/problems/tekton-pipeline-review.md b/docs/problems/tekton-pipeline-review.md new file mode 100644 index 0000000000..de4f86165b --- /dev/null +++ b/docs/problems/tekton-pipeline-review.md @@ -0,0 +1,101 @@ +# Tekton Pipeline Review + +Reviewing Tekton pipeline and task definitions is a distinct problem domain from reviewing application code. The konflux-ci org's most critical repository — [build-definitions](https://github.com/konflux-ci/build-definitions) — is almost entirely Tekton YAML with embedded shell. If agents can't review this content well, the entire autonomous development vision has a blind spot at the most security-sensitive layer. + +## Why pipeline definitions are special + +### They're infrastructure-as-code for the build system + +Every Konflux build runs through a pipeline defined in build-definitions. Changes to these pipelines affect every user's builds. A bug in a Go controller might break one workflow; a bug in a build pipeline definition breaks every build that uses it. The blast radius is categorically different. + +### They're multi-language by nature + +A single Tekton task YAML file typically contains: + +- **YAML structure** — task metadata, step definitions, parameter declarations, result declarations, workspace declarations +- **Shell scripts** — the actual logic, embedded in `script:` fields, often 50-200 lines of bash +- **OCI image references** — each step's container image, often with digest pinning +- **Tekton parameter substitution** — `$(params.foo)`, `$(results.bar.path)`, `$(workspaces.source.path)` expressions interpolated into the shell scripts +- **Occasional Python or other languages** — some steps embed Python scripts instead of bash + +No single review model handles all of these well. A Go correctness agent doesn't understand bash. A shell linter doesn't understand Tekton parameter substitution. An image vulnerability scanner doesn't understand the YAML structure. + +### The stringly-typed interface problem + +Tekton pipelines compose tasks through string-based interfaces: + +```yaml +# Pipeline passes a result from task A to task B +- name: build + taskRef: + name: buildah + params: + - name: IMAGE + value: "$(tasks.clone.results.IMAGE_URL)" +``` + +If `clone` doesn't produce a result called `IMAGE_URL`, or produces it conditionally, or the name is subtly misspelled (`IMAGE_Url`), this fails at runtime — not at review time, not at admission time, not at any static check. The pipeline YAML is syntactically valid. The parameter reference resolves to an empty string or causes a Tekton resolution error. + +Human reviewers catch these by familiarity with the specific tasks involved. They know which results a task produces because they've worked with it before. An agent needs this knowledge explicitly — either by loading the referenced task definitions or by having a schema of task inputs/outputs. + +### Trusted tasks and the security model + +Konflux's trusted task model (ADR-0053) means that specific tasks are blessed and their provenance is verified. Changes to trusted tasks have security implications beyond normal code changes: + +- Modifying a trusted task's behavior changes what every pipeline using it does +- Adding or removing steps changes the security surface +- Changing image references in trusted tasks can introduce supply chain risks +- The trust boundary between the task definition (controlled by build-definitions maintainers) and the user's pipeline definition (controlled by the user) is a critical security seam + +A review agent that doesn't understand the trusted task model will miss the significance of changes at this boundary. + +## What a pipeline review agent needs to understand + +### 1. Task-level semantics + +- **Step ordering and dependencies** — steps within a task run sequentially. The review agent needs to understand data flow between steps (via workspace mounts, result files, environment variables). +- **Step image correctness** — is the image appropriate for what the script does? Does the image contain the tools the script calls? See the ADR-0046 drift scanner experiment for a mechanical version of this check. +- **Resource declarations** — are workspaces, volumes, and resource requests correct? Over-requesting resources wastes cluster capacity; under-requesting causes OOM kills. +- **Result declarations and writes** — does every declared result get written to in all code paths? A result that's declared but not written (because an error path skipped it) will cause downstream pipeline failures. + +### 2. Pipeline-level semantics + +- **Task graph correctness** — are `runAfter` declarations correct? Are there missing dependencies (task B reads task A's result but doesn't declare `runAfter: [A]`)? +- **Parameter threading** — do pipeline-level parameters correctly flow to task-level parameters? Are types consistent? Are there unused parameters that suggest a wiring error? +- **Result aggregation** — does the pipeline correctly aggregate task results into pipeline results? Missing aggregations mean the pipeline's consumers lose data. +- **When expressions** — conditional task execution. Does the pipeline behave correctly when a task is skipped? Do downstream tasks handle the absence of results from skipped tasks? +- **Matrix/fan-out** — parameterized fan-out. Are the matrix combinations correct? Does the fan-in (result aggregation from matrix runs) work? + +### 3. Embedded shell review + +This is arguably the hardest part. Embedded shell scripts need: + +- **Correctness**: proper quoting (`"$VAR"` not `$VAR`), error handling (`set -euo pipefail`), exit code propagation, handling of special characters in filenames/URLs +- **Security**: command injection risks (especially when parameters come from user input via `$(params.*)`), credential handling, secret exposure in logs +- **Portability**: assumptions about the container environment (available tools, filesystem layout, network access) +- **Tekton-specific patterns**: writing results to `$(results.*.path)`, reading workspaces from `$(workspaces.*.path)`, handling optional workspaces + +The Tekton parameter substitution (`$(params.foo)`) happens *before* the shell script runs, which means it's essentially string interpolation into shell. If `$(params.foo)` contains shell metacharacters, the script breaks or worse — it's a command injection vector. Human reviewers who work on build-definitions know to look for this. A generic code review agent won't. + +### 4. Cross-cutting concerns + +- **Backwards compatibility** — changing a task's parameters, results, or behavior breaks all pipelines that use the current version. Tekton's versioning model (tasks are versioned by directory: `task/buildah/0.1/`, `task/buildah/0.2/`) mitigates this, but only if the reviewer enforces it. Changes to an existing version must be backwards-compatible; breaking changes need a new version. +- **Migration path** — when a new task version is introduced, existing pipelines need migration. A review agent should flag when a new version is created without a corresponding migration plan or pipeline update. +- **Performance** — pipeline execution time matters. Adding steps, pulling larger images, or doing redundant work affects every build. Human reviewers consider this; agents need to be prompted to. + +## Relationship to other problem areas + +- **Code review** — Tekton pipeline review is a specialization of the general code review problem. The sub-agent decomposition in [code-review.md](code-review.md) needs to account for this content type. +- **Architectural invariants** — ADRs like 0046 (common task runner image) and 0053 (trusted task model) define invariants specific to pipeline definitions. The drift scanner experiment enforces one such invariant mechanically. +- **Security threat model** — pipeline definitions are a prime target for supply chain attacks. A compromised task definition affects every build using it. The content security agent from [agent-architecture.md](agent-architecture.md) needs deep Tekton knowledge. +- **Repo readiness** — build-definitions has no coverage data on the dashboard. Its "tests" are primarily integration tests (running pipelines in a cluster). Assessing readiness for agent autonomy requires different criteria than for Go repos. + +## Open questions + +- Should there be a dedicated Tekton review sub-agent, or should the existing sub-agents be parameterized per content type? +- Can we build a static analysis tool for Tekton parameter threading — checking that task A's results match task B's parameter references at PR time rather than runtime? +- How do we handle the embedded shell problem? ShellCheck can lint bash, but it doesn't understand Tekton parameter substitution (`$(params.*)` looks like shell arithmetic to ShellCheck). +- Can we derive a task interface schema (inputs, outputs, side effects) automatically from existing task definitions? This would help review agents verify pipeline wiring without loading full task definitions. +- How do we handle the versioning boundary — detecting when a change to `task/foo/0.1/` should actually be a new `task/foo/0.2/`? +- What does "test coverage" mean for a Tekton task? The task itself isn't tested in isolation — it's tested by running the pipeline in a cluster. How do we assess test adequacy for review purposes? +- How do we handle the `stepTemplate` pattern — where a task sets a default image for all steps? The drift scanner experiment doesn't handle this yet.