Browse Source

ci(packages): gate generated tree drift with oac build

Runs the build and fails if it changed anything, so a hand-edit to generated
output turns CI red. Proven to fail twice before landing: editing a generated
agent, and deleting a content/ source to catch the orphan. CI runs the same
make target contributors run.

Runs a write build and then asks git, rather than using --check. check()
compares planned files against disk but never compares the manifest itself —
only write() emits it — so a --check gate would have let the committed ledger
drift silently, and a fresh clone with a stale ledger prunes the wrong set.

Snapshots git status --porcelain around the build instead of git diff
--exit-code, which misses untracked new generated files. On a clean CI
checkout the semantics are identical; locally it attributes drift to the
build, so a dirty worktree cannot false-positive.

Gated on has-canonical, not has-packages: a PR that hand-edits
.opencode/agent/** touches no packages/** path, so has-packages would have
skipped the gate on exactly the change it exists to catch.

Also runs detect-pr-changes tests, which no workflow executed — the workflow
triggered on changes to that file but only ever ran packages/cli's suite, so
its tests were decorative.

plugins/claude-code is deliberately excluded: it stages to .tmp/oac-build/
and is compared only, pending the deferred decision on tightening the 4
agents that grant Bash/Edit unscoped. Add it to GENERATED in
check-build-drift.sh when that lands.
darrenhinde 2 weeks ago
parent
commit
33eb6f4c02

+ 66 - 3
.github/workflows/packages-checks.yml

@@ -11,6 +11,13 @@ on:
       - '.github/dependabot.yml'
       - 'scripts/validation/detect-pr-changes.ts'
       - 'scripts/validation/detect-pr-changes.test.ts'
+      # The canonical source, its committed output, and the drift gate's machinery.
+      - 'content/**'
+      - '.opencode/agent/**'
+      - '.oac/**'
+      - 'registry.json'
+      - 'Makefile'
+      - 'scripts/validation/check-build-drift.sh'
 
 permissions:
   contents: read
@@ -21,6 +28,7 @@ jobs:
     runs-on: ubuntu-latest
     outputs:
       has-packages: ${{ steps.filter.outputs.has-packages }}
+      has-canonical: ${{ steps.filter.outputs.has-canonical }}
 
     steps:
       - name: Checkout code
@@ -34,6 +42,13 @@ jobs:
         with:
           bun-version: 1.3.14
 
+      # The detector decides which gates run, so a broken detector silently skips them —
+      # including the drift gate. Its tests also pin the drift job's contract (that it is
+      # blocking, gated on has-canonical, and runs the same make target contributors do).
+      # They must execute somewhere, and nothing else in CI ran them.
+      - name: Test change detection
+        run: bun test scripts/validation/detect-pr-changes.test.ts
+
       - name: Check changed files
         id: filter
         run: |
@@ -124,10 +139,51 @@ jobs:
       - name: Test (vitest)
         run: pnpm --dir packages/compatibility-layer test
 
+  # The structural guarantee the canonical refactor rests on: the generated trees stay
+  # COMMITTED, so drift has to be impossible rather than merely discouraged. Rebuild them from
+  # content/agents/** and fail if the build changed anything — a hand-edit to generated output
+  # turns this red. BLOCKING by design: no continue-on-error.
+  canonical-drift:
+    name: Canonical Build (no drift)
+    runs-on: ubuntu-latest
+    timeout-minutes: 10
+    needs: check-changes
+    if: needs.check-changes.outputs.has-canonical == 'true'
+
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4
+        with:
+          persist-credentials: false
+
+      # `oac build` runs from packages/cli/dist, which is bundled `--target bun`.
+      - name: Setup Bun
+        uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2.2.0
+        with:
+          bun-version: 1.3.14
+
+      - name: Setup pnpm
+        uses: pnpm/action-setup@b0f76dfb45f55f8421693e4803ac7bb65143bd34 # v6
+
+      - name: Setup Node.js
+        uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4
+        with:
+          node-version: '20'
+          cache: 'pnpm'
+          cache-dependency-path: 'pnpm-lock.yaml'
+
+      - name: Install workspace dependencies
+        run: pnpm install --frozen-lockfile
+
+      # The identical target contributors run locally, so a red gate here is reproducible with
+      # one command rather than by reading this file and reassembling it by hand.
+      - name: Build canonical trees and check for drift
+        run: make check-canonical-drift
+
   summary:
     name: Packages Checks Summary
     runs-on: ubuntu-latest
-    needs: [check-changes, cli-checks, compatibility-layer-checks]
+    needs: [check-changes, cli-checks, compatibility-layer-checks, canonical-drift]
     if: always()
 
     steps:
@@ -140,7 +196,7 @@ jobs:
             local label="$1" result="$2"
             case "$result" in
               success) echo "✅ **${label}:** Passed" >> $GITHUB_STEP_SUMMARY ;;
-              skipped) echo "⏭️ **${label}:** Skipped (no packages/** changes)" >> $GITHUB_STEP_SUMMARY ;;
+              skipped) echo "⏭️ **${label}:** Skipped (no relevant changes)" >> $GITHUB_STEP_SUMMARY ;;
               *) echo "❌ **${label}:** ${result}" >> $GITHUB_STEP_SUMMARY ;;
             esac
           }
@@ -148,12 +204,19 @@ jobs:
           report "Change Detection" "${{ needs.check-changes.result }}"
           report "CLI" "${{ needs.cli-checks.result }}"
           report "Compatibility Layer" "${{ needs.compatibility-layer-checks.result }}"
+          report "Canonical Build (no drift)" "${{ needs.canonical-drift.result }}"
 
           echo "" >> $GITHUB_STEP_SUMMARY
 
+          if [ "${{ needs.canonical-drift.result }}" = "failure" ]; then
+            echo "> The generated trees drift from \`content/agents/**\`." >> $GITHUB_STEP_SUMMARY
+            echo "> Run \`make build-canonical\` and commit the result." >> $GITHUB_STEP_SUMMARY
+            echo "" >> $GITHUB_STEP_SUMMARY
+          fi
+
           FAILED=0
           [ "${{ needs.check-changes.result }}" != "success" ] && FAILED=1
-          for result in "${{ needs.cli-checks.result }}" "${{ needs.compatibility-layer-checks.result }}"; do
+          for result in "${{ needs.cli-checks.result }}" "${{ needs.compatibility-layer-checks.result }}" "${{ needs.canonical-drift.result }}"; do
             if [ "$result" != "success" ] && [ "$result" != "skipped" ]; then
               FAILED=1
             fi

+ 16 - 0
Makefile

@@ -7,6 +7,7 @@ OWNER := darrenhinde
 
 .PHONY: help idea ideas board labels project-info issue-view issue-comment issue-close bug feature
 .PHONY: test-evals test-golden test-smoke test-verbose build-evals validate-evals
+.PHONY: build-canonical check-canonical-drift oac-cli
 
 help: ## Show this help message
 	@echo "OpenAgents GitHub Project Management"
@@ -157,6 +158,21 @@ high-priority: ## List all high priority items
 # Evaluation Framework Commands
 # =============================================================================
 
+# The canonical build: content/agents/** is the source, .opencode/agent/**, registry.json and
+# .oac/build-manifest.json are its committed output. `check-canonical-drift` is exactly what CI
+# runs (.github/workflows/packages-checks.yml), so a contributor can reproduce a red gate here.
+oac-cli: ## Build the oac CLI and its compatibility-layer dependency
+	@pnpm --dir packages/compatibility-layer run build
+	@pnpm --dir packages/cli run build
+
+build-canonical: oac-cli ## Regenerate the committed trees from content/agents/**
+	@echo "🔨 Building canonical trees..."
+	@bun packages/cli/dist/index.js build
+
+check-canonical-drift: oac-cli ## Fail if the generated trees drift from content/agents/** (CI gate)
+	@echo "🔍 Checking for generated-tree drift..."
+	@bash scripts/validation/check-build-drift.sh
+
 build-evals: ## Build the evaluation framework
 	@echo "🔨 Building evaluation framework..."
 	@pnpm install --frozen-lockfile

+ 100 - 0
scripts/validation/check-build-drift.sh

@@ -0,0 +1,100 @@
+#!/usr/bin/env bash
+#
+# The drift gate: regenerate the committed trees from content/agents/**, then fail if the
+# build changed anything. This is the structural guarantee the canonical refactor rests on —
+# generated trees stay COMMITTED, so a hand-edit to one must turn CI red rather than merely
+# being discouraged.
+#
+# ── Why a write build and not `oac build --check` ────────────────────────────────────────
+#
+# `check()` (BuildPipeline.ts) compares the PLANNED files against disk and reports orphans
+# from the manifest. It never compares `.oac/build-manifest.json` itself — the manifest is
+# only ever written by `write()`. Since the manifest is committed (it is the ledger orphan
+# pruning reasons from), a `--check` gate would let the manifest drift silently, and a fresh
+# clone with a stale ledger prunes the wrong set. So: real build, then ask git.
+#
+# ── Why before/after and not a bare `git status` ─────────────────────────────────────────
+#
+# In CI the checkout is clean, so "clean after the build" and "unchanged by the build" are
+# the same statement. Locally they are not: `.opencode/agent/eval-runner.md` is a real,
+# shipped, hand-authored agent with no canonical source, and any contributor may have
+# unrelated work in progress under these roots. Comparing the status BEFORE the build to the
+# status AFTER attributes drift to the build rather than to the worktree — pre-existing dirt
+# appears in both snapshots and cancels. The gate stays honest and stays runnable locally,
+# which is the only way contributors ever see it fail before CI does.
+#
+set -euo pipefail
+
+# `oac build` resolves content/ against the CWD — from anywhere else it exits 1 with ENOENT.
+ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)"
+cd "$ROOT"
+
+# The trees `oac build` emits IN PLACE, and therefore everything this gate watches.
+# plugins/claude-code/** is deliberately absent: it stages to .tmp/oac-build/ (gitignored)
+# and is compared only, never emitted. See packages/cli/src/commands/build.ts.
+GENERATED=(.opencode/agent registry.json .oac/build-manifest.json)
+
+if [ ! -f packages/cli/dist/index.js ]; then
+  echo "error: packages/cli/dist/index.js is missing — build the CLI first:" >&2
+  echo "  pnpm --dir packages/compatibility-layer run build" >&2
+  echo "  pnpm --dir packages/cli run build" >&2
+  exit 1
+fi
+
+snapshot() { git status --porcelain -- "${GENERATED[@]}"; }
+
+before="$(snapshot)"
+bun packages/cli/dist/index.js build
+after="$(snapshot)"
+
+if [ "$before" = "$after" ]; then
+  echo ""
+  echo "✅ No drift — the generated trees match content/agents/**."
+  exit 0
+fi
+
+cat >&2 <<'EOF'
+
+❌ The generated trees drift from the canonical source.
+
+Files under .opencode/agent/, registry.json and .oac/build-manifest.json are BUILD OUTPUT.
+They are committed, but they are not hand-edited: `oac build` regenerates them from
+content/agents/**, and this gate fails when the committed bytes disagree with what the
+build produces.
+
+To fix:
+  1. Make your change in content/agents/** (the canonical source), not in the output.
+  2. Run:  make build-canonical
+  3. Commit the regenerated files along with your source change.
+
+Paths the build changed:
+EOF
+
+# Only the paths whose status the BUILD moved. Diffing the whole roots instead would dump
+# every unrelated dirty file in the worktree — including `.opencode/agent/eval-runner.md`,
+# which is hand-authored, has no canonical source, and is none of this gate's business.
+#
+# BOTH sides of the delta matter, and the `<` side is the one that is easy to get wrong.
+# When a contributor hand-edits a generated file, the build REWRITES it back to canonical, so
+# the path is dirty BEFORE and clean AFTER — it leaves the status as a `<` line, never a `>`.
+# Matching only `>` would make the single most likely drift print an empty list.
+# `sed 's/^[<>] ...//'` drops diff's marker, then git's two-char status field and its space.
+drifted=()
+while IFS= read -r line; do
+  [ -n "$line" ] && drifted+=("$line")
+done < <(diff <(printf '%s\n' "$before") <(printf '%s\n' "$after") |
+  sed -n 's/^[<>] ...//p' | sort -u)
+
+if [ ${#drifted[@]} -eq 0 ]; then
+  # Unreachable: the snapshots differ, so some path moved. Fail loudly rather than pretend.
+  echo "  (could not attribute the drift to a path — snapshots differ but no path parsed)" >&2
+  exit 1
+fi
+
+printf '  %s\n' "${drifted[@]}" >&2
+
+echo "" >&2
+echo "Diff:" >&2
+git --no-pager diff -- "${drifted[@]}" >&2
+
+exit 1

+ 115 - 4
scripts/validation/detect-pr-changes.test.ts

@@ -14,11 +14,13 @@ const NO_CHANGES = {
   'has-docs': false,
   'has-workflows': false,
   'has-packages': false,
+  'has-canonical': false,
 }
 
 const SCRIPT_PATH = join(import.meta.dir, 'detect-pr-changes.ts')
 const WORKFLOW_PATH = join(import.meta.dir, '..', '..', '.github', 'workflows', 'pr-checks.yml')
 const PACKAGES_WORKFLOW_PATH = join(import.meta.dir, '..', '..', '.github', 'workflows', 'packages-checks.yml')
+const DRIFT_SCRIPT_PATH = join(import.meta.dir, 'check-build-drift.sh')
 
 type CliResult = {
   exitCode: number
@@ -82,10 +84,12 @@ describe('classifyChangedPaths', () => {
     expect(classifyChangedPaths(['packages/cli/src/index.ts'])).toEqual({
       ...NO_CHANGES,
       'has-packages': true,
+      'has-canonical': true,
     })
     expect(classifyChangedPaths(['packages/compatibility-layer/package.json'])).toEqual({
       ...NO_CHANGES,
       'has-packages': true,
+      'has-canonical': true,
     })
   })
 
@@ -95,6 +99,7 @@ describe('classifyChangedPaths', () => {
         ...NO_CHANGES,
         'has-evals': true,
         'has-packages': true,
+        'has-canonical': true,
       })
     }
   })
@@ -110,10 +115,57 @@ describe('classifyChangedPaths', () => {
         ...NO_CHANGES,
         'has-packages': true,
         ...(path.startsWith('.github/workflows/') ? { 'has-workflows': true } : {}),
+        ...(path === '.github/workflows/packages-checks.yml' ? { 'has-canonical': true } : {}),
       })
     }
   })
 
+  test('routes canonical source changes to the drift gate', () => {
+    for (const path of ['content/agents/core/openagent.md', 'content/profiles/default.yaml']) {
+      expect(classifyChangedPaths([path])).toEqual({
+        ...NO_CHANGES,
+        'has-canonical': true,
+      })
+    }
+  })
+
+  test('routes generated-tree changes to the drift gate', () => {
+    // A PR that hand-edits generated output touches no packages/** path at all. If these did
+    // not set has-canonical, the gate would skip exactly the change it exists to catch.
+    for (const path of [
+      '.opencode/agent/core/openagent.md',
+      '.oac/build-manifest.json',
+      'registry.json',
+    ]) {
+      expect(classifyChangedPaths([path])).toEqual({
+        ...NO_CHANGES,
+        'has-canonical': true,
+      })
+    }
+  })
+
+  test('routes drift-gate machinery changes to the drift gate', () => {
+    for (const path of ['Makefile', 'scripts/validation/check-build-drift.sh']) {
+      expect(classifyChangedPaths([path])).toEqual({
+        ...NO_CHANGES,
+        'has-canonical': true,
+      })
+    }
+  })
+
+  test('rejects near-prefix paths for canonical routing', () => {
+    expect(
+      classifyChangedPaths([
+        'content-old/agents/x.md',
+        'nested/content/agents/x.md',
+        'contents/x.md',
+        '.opencode/command/add-context.md',
+        'registry.json.bak',
+        'Makefile.old',
+      ]),
+    ).toEqual(NO_CHANGES)
+  })
+
   test('detects mixed changes', () => {
     expect(
       classifyChangedPaths([
@@ -128,6 +180,7 @@ describe('classifyChangedPaths', () => {
       'has-docs': true,
       'has-workflows': true,
       'has-packages': true,
+      'has-canonical': true,
     })
   })
 
@@ -171,8 +224,12 @@ describe('formatGitHubOutput', () => {
         'has-docs': false,
         'has-workflows': true,
         'has-packages': false,
+        'has-canonical': false,
       }),
-    ).toBe('has-evals=true\nhas-docs=false\nhas-workflows=true\nhas-packages=false\n')
+    ).toBe(
+      'has-evals=true\nhas-docs=false\nhas-workflows=true\nhas-packages=false\n' +
+        'has-canonical=false\n',
+    )
   })
 })
 
@@ -186,7 +243,8 @@ describe('CLI', () => {
 
       expect(result).toEqual({ exitCode: 0, stderr: '', stdout: '' })
       expect(await readFile(outputPath, 'utf8')).toBe(
-        'existing=value\nhas-evals=true\nhas-docs=false\nhas-workflows=false\nhas-packages=false\n',
+        'existing=value\nhas-evals=true\nhas-docs=false\nhas-workflows=false\n' +
+          'has-packages=false\nhas-canonical=false\n',
       )
     })
   })
@@ -199,7 +257,8 @@ describe('CLI', () => {
 
       expect(result.exitCode).toBe(0)
       expect(await readFile(outputPath, 'utf8')).toBe(
-        'has-evals=false\nhas-docs=false\nhas-workflows=false\nhas-packages=false\n',
+        'has-evals=false\nhas-docs=false\nhas-workflows=false\nhas-packages=false\n' +
+          'has-canonical=false\n',
       )
     })
   })
@@ -237,7 +296,8 @@ describe('CLI', () => {
 
       expect(result.exitCode).toBe(0)
       expect(await readFile(outputPath, 'utf8')).toBe(
-        'has-evals=true\nhas-docs=true\nhas-workflows=true\nhas-packages=true\n',
+        'has-evals=true\nhas-docs=true\nhas-workflows=true\nhas-packages=true\n' +
+          'has-canonical=true\n',
       )
     })
   })
@@ -306,3 +366,54 @@ describe('Packages checks workflow contract', () => {
     expect(workflow).toContain("- 'packages/**'")
   })
 })
+
+describe('Canonical drift gate contract', () => {
+  test('triggers on the canonical source and the generated trees', async () => {
+    const workflow = await readFile(PACKAGES_WORKFLOW_PATH, 'utf8')
+
+    for (const path of [
+      "- 'content/**'",
+      "- '.opencode/agent/**'",
+      "- '.oac/**'",
+      "- 'registry.json'",
+      "- 'Makefile'",
+      "- 'scripts/validation/check-build-drift.sh'",
+    ]) {
+      expect(workflow).toContain(path)
+    }
+  })
+
+  test('gates the drift job on has-canonical and runs the same make target as contributors', async () => {
+    const workflow = await readFile(PACKAGES_WORKFLOW_PATH, 'utf8')
+
+    expect(workflow).toContain('canonical-drift:')
+    expect(workflow).toContain('has-canonical: ${{ steps.filter.outputs.has-canonical }}')
+    expect(workflow).toContain("needs.check-changes.outputs.has-canonical == 'true'")
+    expect(workflow).toContain('run: make check-canonical-drift')
+  })
+
+  test('is blocking — never continue-on-error, and the summary fails on it', async () => {
+    const workflow = await readFile(PACKAGES_WORKFLOW_PATH, 'utf8')
+
+    // A gate that cannot fail the PR reads as coverage while providing none. Match the YAML
+    // key rather than the bare phrase, which also appears in the job's own comment.
+    expect(workflow).not.toMatch(/^\s*continue-on-error:\s*true/m)
+    expect(workflow).toContain('needs: [check-changes, cli-checks, compatibility-layer-checks, canonical-drift]')
+    expect(workflow).toContain('"${{ needs.canonical-drift.result }}"')
+
+    const summary = workflow.slice(workflow.indexOf('  summary:'))
+    expect(summary).toContain('exit 1')
+  })
+
+  test('the drift script rebuilds and compares the committed generated trees', async () => {
+    const script = await readFile(DRIFT_SCRIPT_PATH, 'utf8')
+
+    // A write build, not `--check`: check() never compares the manifest, which is committed.
+    expect(script).toContain('bun packages/cli/dist/index.js build')
+    expect(script).toContain('.oac/build-manifest.json')
+    expect(script).toContain('git status --porcelain')
+    expect(script).toContain('exit 1')
+    // plugins/claude-code/** is staged and compared, never emitted — not this gate's business.
+    expect(script).not.toContain('plugins/claude-code/agents)')
+  })
+})

+ 26 - 0
scripts/validation/detect-pr-changes.ts

@@ -7,6 +7,17 @@ export type ChangeFlags = {
   'has-docs': boolean
   'has-workflows': boolean
   'has-packages': boolean
+  /**
+   * Anything that can move the generated trees, and therefore must re-run the drift gate:
+   * the canonical source, the build that reads it, and the committed output itself.
+   *
+   * Separate from `has-packages` on purpose. A content-only edit must run the drift gate but
+   * has no reason to run the cli/compatibility-layer test suites, and a PR that hand-edits
+   * `.opencode/agent/**` alone touches no `packages/**` path at all — folding this into
+   * `has-packages` would either skip the gate on exactly the change it exists to catch, or
+   * run every package suite on every prose tweak.
+   */
+  'has-canonical': boolean
 }
 
 export function parseChangedPaths(input: string): string[] {
@@ -29,6 +40,20 @@ export function classifyChangedPaths(paths: readonly string[]): ChangeFlags {
       path === 'scripts/validation/detect-pr-changes.ts' ||
       path === 'scripts/validation/detect-pr-changes.test.ts',
     ),
+    'has-canonical': hasWorkspaceChange || paths.some((path) =>
+      // The canonical source.
+      path.startsWith('content/') ||
+      // The build that turns it into output.
+      path.startsWith('packages/') ||
+      // The committed output — a hand-edit here is precisely what the gate exists to catch.
+      path.startsWith('.opencode/agent/') ||
+      path.startsWith('.oac/') ||
+      path === 'registry.json' ||
+      // The gate's own machinery.
+      path === 'Makefile' ||
+      path === 'scripts/validation/check-build-drift.sh' ||
+      path === '.github/workflows/packages-checks.yml',
+    ),
   }
 }
 
@@ -38,6 +63,7 @@ export function formatGitHubOutput(flags: ChangeFlags): string {
     `has-docs=${flags['has-docs']}`,
     `has-workflows=${flags['has-workflows']}`,
     `has-packages=${flags['has-packages']}`,
+    `has-canonical=${flags['has-canonical']}`,
   ].join('\n') + '\n'
 }