Test multi-sub-agent Review Agent

.github/workflows/pat-cleanup.yml

@@ -0,0 +1,45 @@
+# Delete expired classic PATs from the e2e test account (botsend).
+# Each e2e run creates a PAT and tries to clean it up, but crashed or
+# timed-out runs leave orphaned tokens that accumulate over time.
+
+name: Clean up expired PATs
+
+on:
+  schedule:
+    - cron: "0 4 * * 0" # Weekly on Sundays at 4am UTC
+  workflow_dispatch:
+
+concurrency:
+  group: pat-cleanup
+  cancel-in-progress: false
+
+permissions:
+  contents: read
+
+jobs:
+  cleanup:
+    runs-on: ubuntu-latest
+    timeout-minutes: 30
+    steps:
+      - uses: actions/checkout@v4
+
+      - uses: actions/setup-go@v5
+        with:
+          go-version-file: go.mod
+
+      - name: Install Playwright browser and system dependencies
+        run: |
+          go run github.com/playwright-community/playwright-go/cmd/playwright install chromium
+          npx playwright install-deps chromium
+
+      - name: Decode session
+        run: |
+          SESSION_FILE="${RUNNER_TEMP}/github-session.json"
+          printf '%s' "$E2E_GITHUB_SESSION_B64" | base64 -d > "$SESSION_FILE"
+          chmod 600 "$SESSION_FILE"
+          echo "E2E_GITHUB_SESSION_FILE=${SESSION_FILE}" >> "$GITHUB_ENV"
+        env:
+          E2E_GITHUB_SESSION_B64: ${{ secrets.E2E_GITHUB_SESSION }}
+
+      - name: Delete expired PATs
+        run: go run hack/cleanup-pats.go

docs/plans/universal-harness-access-implementation.md

@@ -142,14 +142,14 @@ PRs 1, 2, 4, and 6 have no dependencies and can be developed/merged in parallel.
 **Scope:** New package that orchestrates fetch + cache + validation + audit for URL-referenced resources. This is the core logic.
 
 **Create `internal/resolve/resolve.go`:**
-- `ResolvedHarness` struct: wraps `*harness.Harness` + resolved paths (AgentPath, PolicyPath, SkillPaths, Dependencies)
-- `Dependency` struct: URL, LocalPath (cache path), SHA256, FetchedAt
-- `ResolveOpts` struct: WorkspaceRoot, FetchPolicy, OrgAllowlist, TraceID, AuditLogPath
-- `ResolveHarness(ctx, h *harness.Harness, opts) (*ResolvedHarness, error)`:
+- `Dependency` struct: URL, LocalPath (cache path), SHA256, FetchedAt, CacheHit
+- `ResolveOpts` struct: WorkspaceRoot, FetchPolicy, TraceID, AuditLogPath
+- `ResolveHarness(ctx, h *harness.Harness, opts) ([]Dependency, error)`:
+  - Modifies the harness in place, replacing URL fields with local cache paths
   - For each declarative field (Agent, Policy, Skills):
     - Local path: return as-is
-    - URL: validate against `AllowedRemoteResources` → extract/require integrity hash → check cache (with re-verification) → if miss and not offline: `fetch.FetchURL` → verify hash → security scan (InputPipeline, remote threshold) → `CachePut` → `AppendFetchAudit` → return cache content path
-  - Phase 1: single-level only (no transitive deps)
+    - URL: extract/require integrity hash → validate against `AllowedRemoteResources` → check cache (with re-verification) → if miss and not offline: `fetch.FetchURL` → verify hash → `CachePut` → `AppendFetchAudit` → return cache content path
+  - Phase 1: single-level only (no transitive deps), security scanning deferred
 
 **Create `internal/resolve/resolve_test.go`:**
 - Tests using `httptest.NewTLSServer`: local pass-through, URL fetch+cache, cache hit, hash mismatch, URL not in allowlist, missing hash, offline+miss, offline+hit, security scan failure, mixed harness, audit entries
@@ -169,9 +169,8 @@ PRs 1, 2, 4, and 6 have no dependencies and can be developed/merged in parallel.
 - In `runAgent()`, **between** `h.ResolveRelativeTo(absFullsendDir)` and `h.ValidateFilesExist()`:
   1. `h.ValidateResourceTypes()` — reject URLs in script fields, require hashes (no-op for local-only harnesses)
   2. If harness has any URL references: load org config, call `h.ValidateAllowedRemoteResources(orgCfg.AllowedRemoteResources)`
-  3. `resolve.ResolveHarness(ctx, h, opts)` — fetch/cache URLs (no-op if all local)
-  4. Replace harness fields with resolved paths: `h.Agent = resolved.AgentPath`, etc.
-  5. `h.ValidateFilesExist()` then validates resolved paths (cache files or local files)
+  3. `resolve.ResolveHarness(ctx, h, opts)` — fetch/cache URLs, replace harness fields with cache paths in place (no-op if all local)
+  4. `h.ValidateFilesExist()` then validates resolved paths (cache files or local files)
 
 **Key design:** For local-only harnesses, steps 1-3 are no-ops (no URLs detected, no fetches). Zero behavioral change for existing users.
 

docs/plans/universal-harness-access.md

@@ -305,9 +305,9 @@ Resolution algorithm:
 5. Detect cycles (if skill A references skill B, and skill B references skill A, reject)
 6. Fail if any resource cannot be fetched or validated
 
-**Output:** A `ResolvedHarness` struct containing absolute paths or cache paths for all resources.
+**Output:** The harness is modified in place, replacing URL fields with local cache paths. Returns `([]Dependency, error)` listing the resolved resources.
 
-**Implementation:** New package `internal/resolve/` provides `ResolveHarness(h *harness.Harness) (*ResolvedHarness, error)`.
+**Implementation:** New package `internal/resolve/` provides `ResolveHarness(ctx, h *harness.Harness, opts ResolveOpts) ([]Dependency, error)`.
 
 ### Runtime Dependency Loading (Future)
 
@@ -934,60 +934,41 @@ import (
 
     "github.com/fullsend-ai/fullsend/internal/fetch"
     "github.com/fullsend-ai/fullsend/internal/harness"
-    "github.com/fullsend-ai/fullsend/internal/security"
 )
 
-type ResolvedHarness struct {
-    Harness      *harness.Harness
-    AgentPath    string   // absolute path or cache path
-    PolicyPath   string
-    SkillPaths   []string
-    Dependencies []Dependency
-}
-
 type Dependency struct {
-    URL        string
-    LocalPath  string // cache path
-    SHA256     string
-    FetchedAt  time.Time
+    URL       string
+    LocalPath string
+    SHA256    string
+    FetchedAt time.Time
+    CacheHit  bool
 }
 
-// ResolveHarness resolves all resources (local and remote) and returns paths.
-func ResolveHarness(ctx context.Context, workspaceRoot string, h *harness.Harness, policy fetch.FetchPolicy) (*ResolvedHarness, error) {
-    resolved := &ResolvedHarness{Harness: h}
-    resourceCount := 0
-
-    // Resolve agent
-    var err error
-    resolved.AgentPath, err = resolveResourceWithLimits(ctx, workspaceRoot, h.Agent, h.AllowedRemoteResources, policy, 0, &resourceCount, "")
-    if err != nil {
-        return nil, fmt.Errorf("resolving agent: %w", err)
-    }
+type ResolveOpts struct {
+    WorkspaceRoot string
+    FetchPolicy   fetch.FetchPolicy
+    TraceID       string
+    AuditLogPath  string
+}
 
-    // Resolve policy
-    if h.Policy != "" {
-        resolved.PolicyPath, err = resolveResourceWithLimits(ctx, workspaceRoot, h.Policy, h.AllowedRemoteResources, policy, 0, &resourceCount, "")
-        if err != nil {
-            return nil, fmt.Errorf("resolving policy: %w", err)
-        }
-    }
+// ResolveHarness resolves URL-referenced declarative fields (Agent, Policy,
+// Skills) in the harness to local cache paths. Local paths are left unchanged.
+// The harness is modified in place.
+// Phase 1: single-level resolution only (no transitive deps).
+func ResolveHarness(ctx context.Context, h *harness.Harness, opts ResolveOpts) ([]Dependency, error) {
+    var deps []Dependency
 
-    // Resolve skills
-    // Phase 1: Single-level only (skills themselves cannot reference URLs)
-    // Phase 2+: Each skill may have transitive dependencies (code below)
-    for _, skill := range h.Skills {
-        skillPath, err := resolveResourceWithLimits(ctx, workspaceRoot, skill, h.AllowedRemoteResources, policy, 0, &resourceCount, "")
+    if h.Agent != "" && harness.IsURL(h.Agent) {
+        dep, localPath, err := resolveURL(ctx, "agent", h.Agent, h, opts)
         if err != nil {
-            return nil, fmt.Errorf("resolving skill %s: %w", skill, err)
+            return nil, fmt.Errorf("resolving agent: %w", err)
         }
-        resolved.SkillPaths = append(resolved.SkillPaths, skillPath)
-
-        // Phase 2+: Parse skill to extract transitive dependencies
-        // (skill format TBD — may have a dependencies: field in frontmatter)
-        // Recursively resolve those dependencies
+        h.Agent = localPath
+        deps = append(deps, dep)
     }
 
-    return resolved, nil
+    // Similar for h.Policy and h.Skills...
+    return deps, nil
 }
 
 // resolveResourceWithLimits resolves a single resource with depth and count limits.
@@ -1135,12 +1116,14 @@ if err := h.ResolveRelativeTo(absFullsendDir); err != nil {
 // NEW: Resolve remote resources
 fetchPolicy := fetch.DefaultPolicy
 // TODO: Load allowed domains from config.yaml
-resolved, err := resolve.ResolveHarness(ctx, workspaceRoot, h, fetchPolicy)
+deps, err := resolve.ResolveHarness(ctx, h, resolve.ResolveOpts{
+    WorkspaceRoot: workspaceRoot,
+    FetchPolicy:   fetchPolicy,
+})
 if err != nil {
     return fmt.Errorf("resolving remote resources: %w", err)
 }
-
-// Use resolved.AgentPath, resolved.PolicyPath, etc. instead of h.Agent, h.Policy
+// h.Agent, h.Policy, h.Skills are now local cache paths
 ```
 
 ### 7. Security Scanner Integration
@@ -1238,7 +1221,10 @@ fetchPolicy := fetch.DefaultPolicy
 if offline {
     fetchPolicy.Offline = true
 }
-resolved, err := resolve.ResolveHarness(ctx, workspaceRoot, h, fetchPolicy)
+deps, err := resolve.ResolveHarness(ctx, h, resolve.ResolveOpts{
+    WorkspaceRoot: workspaceRoot,
+    FetchPolicy:   fetchPolicy,
+})
 ```
 
 ## Migration Path

docs/problems/agent-architecture.md

@@ -57,14 +57,7 @@ Writes code to address an issue. This is the most mature capability of current A
 
 Code review is decomposed into multiple specialized sub-agents rather than handled by a single monolithic reviewer. This is an architectural necessity, not an optimization — see [code-review.md](code-review.md) for the full argument (context window limits, defense in depth, specialization).
 
-The current decomposition:
-
-- **Correctness agent** — logic errors, edge cases, test adequacy
-- **Intent alignment agent** — does the change match authorized intent, is it correctly tiered
-- **Platform security agent** — threats to Konflux itself (RBAC, auth, data exposure)
-- **Content security agent** — threats to Konflux users via CI/CD content
-- **Injection defense agent** — prompt injection patterns targeting other agents
-- **Style/conventions agent** — repo-specific patterns (may be folded into pre-PR self-review)
+The list showing current decomposition is maintained in [code-review.md](code-review.md).
 
 Each sub-agent operates under zero trust — they don't rely on other sub-agents' judgments. See [code-review.md](code-review.md) for how sub-agent findings compose into a merge decision.
 

docs/problems/applied/konflux-ci/README.md

@@ -90,11 +90,11 @@ See BOOKMARKS.md for architectural context and external standards.
 
 ### Code review
 
-The platform security and content security review sub-agents have Konflux-specific concerns:
+The Security review sub-agent covers two Konflux-specific concerns within a single dimension:
 
-**Platform security agent** — Reviews changes for threats to Konflux itself: RBAC and authorization changes, authentication flows, data exposure risks, privilege escalation paths, injection vulnerabilities.
+**Platform security** — Reviews changes for threats to Konflux itself: RBAC and authorization changes, authentication flows, data exposure risks, privilege escalation paths, injection vulnerabilities.
 
-**Content security agent** — Reviews changes that affect the CI/CD content passing through Konflux — protecting Konflux's users:
+**Content security** — Reviews changes that affect the CI/CD content passing through Konflux — protecting Konflux's users:
 - Pipeline definition handling — can a user's pipeline definition escape its sandbox?
 - Build configuration — can build parameters be manipulated?
 - Release policy — can release gates be bypassed?