From dab8cd9b243de928961a5e3f52cf8607f5e7a3e7 Mon Sep 17 00:00:00 2001 From: Ragdoll-Opus-5 Date: Tue, 22 Sep 2026 11:15:50 +0800 Subject: [PATCH 1/5] fix(F192): restore evidence gate + guard fork-only patches against rebuild loss Why: the F192 evidence-prerequisite gate has now been silently dropped by develop_base rebuilds THREE times (PR #91, PR #137, commit 70d79a2f2). The third loss went unnoticed for 21 days, and was caused by a commit whose own title was "recover fork-only development layer lost in develop_base rebuild" - a rebuild explicitly trying to recover fork-only code still missed it. Each loss lets the scheduler burn a full LLM session per daily eval fire, re-concluding a telemetry gap it cannot act on. Re-applying the patch a third time would not stop a fourth loss, so this change fixes the mechanism, not just the symptom. ## Durability (the actual root cause) Maintainer direction when closing upstream PR zts212653/clowder-ai#1352 (2026-08-25): "this is fork-specific harness logic that doesn't belong in upstream. The rebuild-loss issue should be solved by improving our fork's develop_base rebuild SOP (rebase fork-only patches)." Implements exactly that, as a mechanism rather than a checklist: - scripts/fork-only-patches.json - registry of patches upstream does not carry - scripts/check-fork-only-patches.mjs - fails closed on missing files AND on lost in-file anchors (the silent case: shared file survives, patch reverted) - wired FIRST in `pnpm check`, so a rebuild that drops a patch goes red immediately instead of failing silently in production - docs/fork-only-patches.md - why the boundary exists, so it is not re-litigated Verified red: run against the current unfixed develop_base tree it reports all 11 violations with actionable paths; green on this branch. ## Gate restoration, with the upstream review findings fixed Upstream review of #1352 surfaced two real defects. Both are fixed here rather than carried forward blindly: 1. The probe read the SCHEDULER process's telemetry handle while evidence is fetched from the adapter target (EVAL_BASE_URL). These coincide in this fork's single-process deployment, but only accidentally. Added `evidenceTargetIsLocal`, asserted at bootstrap from EVAL_BASE_URL vs API_SERVER_PORT; an unprovable target now fails closed instead of letting a local handle speak for a remote source. 2. The skip notice always advised configuring TELEMETRY_HMAC_SALT even when the detected cause was a deliberate OTEL_SDK_DISABLED=true. Next-step text is now derived from the detected cause. ## Test-validity fix The restored suite asserted `invokeTrigger.trigger` call counts, but RFC 5.4 replaced that positional call with the deliver envelope (targetCatId + privateContent). Those assertions were passing vacuously - `callCount() === 0` is trivially true on every path now, so "cat never invoked" proved nothing. Migrated all 6 sites to count real wakes on the live seam; the happy-path test asserts 1 wake, so the invariant is falsifiable in both directions. Tests: 20/20 evidence gate, 29/29 daily+nday, biome + tsc clean. `pnpm check` remaining failures (check:capability-tips) reproduce identically on unmodified develop_base - pre-existing, unrelated. Agent: @opus (claude-opus-5) Co-Authored-By: Claude Opus 5 --- docs/fork-only-patches.md | 76 ++++ package.json | 3 +- packages/api/src/index.ts | 24 ++ .../harness-eval/domain/eval-domain-daily.ts | 36 ++ .../domain/eval-domain-evidence-gate.ts | 158 ++++++++ .../harness-eval/domain/eval-domain-nday.ts | 18 + .../eval-domain-evidence-gate.test.js | 336 ++++++++++++++++++ scripts/check-fork-only-patches.mjs | 104 ++++++ scripts/fork-only-patches.json | 33 ++ 9 files changed, 787 insertions(+), 1 deletion(-) create mode 100644 docs/fork-only-patches.md create mode 100644 packages/api/src/infrastructure/harness-eval/domain/eval-domain-evidence-gate.ts create mode 100644 packages/api/test/harness-eval/eval-domain-evidence-gate.test.js create mode 100755 scripts/check-fork-only-patches.mjs create mode 100644 scripts/fork-only-patches.json diff --git a/docs/fork-only-patches.md b/docs/fork-only-patches.md new file mode 100644 index 0000000000..4763d5db91 --- /dev/null +++ b/docs/fork-only-patches.md @@ -0,0 +1,76 @@ +# Fork-only patches + +Patches this fork carries that **upstream `zts212653/clowder-ai` main does not**. + +Every `develop_base` rebuild onto upstream main starts from a tree without them. +If nobody re-applies them, they vanish silently. + +## Truth source + +`scripts/fork-only-patches.json` — machine-readable registry. +Enforced by `scripts/check-fork-only-patches.mjs`, wired into `pnpm check`. + +This document explains the *why*; the registry is the *what*. Do not duplicate +the file/anchor lists here — they would drift. + +## Why a guard instead of a checklist + +The F192 evidence-prerequisite gate was lost **three times**: + +| When | How | Noticed after | +|---|---|---| +| 2026-08-11 | develop_base rebuild | ~2 days (PR #91 recovery) | +| 2026-08-28 | develop_base rebuild | ~1 day (PR #137 recovery) | +| 2026-09-01 | commit `70d79a2f2` — a commit whose own title was *"recover fork-only development layer lost in develop_base rebuild"* | **21 days** | + +Each loss let the scheduler burn one full LLM session per daily eval fire, +re-concluding a gap it could not act on. + +The third loss is the decisive one: a rebuild that was *explicitly trying* to +recover fork-only code still missed this patch. Human/agent recall is not a +mechanism. A guard is. + +## Upstream boundary + +Pushing these patches upstream is **not** the answer, and was already decided: + +> Closing — this is fork-specific harness logic that doesn't belong in upstream. +> The rebuild-loss issue should be solved by improving our fork's develop_base +> rebuild SOP (rebase fork-only patches), not by pushing internal code into the +> community repo. +> +> — `zts212653/clowder-ai#1352`, closed 2026-08-25 + +Upstream review of that PR also surfaced two real defects, both fixed in the +fork version rather than carried forward blindly: + +1. The probe read the **scheduler process's** telemetry handle while evidence is + fetched from the **adapter target** (`EVAL_BASE_URL`). These coincide in this + fork's single-process deployment, but only accidentally. The probe now + asserts co-location via `evidenceTargetIsLocal` and fails closed when it + cannot prove it. +2. The skip notice always advised configuring `TELEMETRY_HMAC_SALT`, even when + the detected cause was an intentional `OTEL_SDK_DISABLED=true`. The next-step + text is now derived from the detected cause. + +## Rebuild SOP + +After any `develop_base` rebuild onto upstream main: + +```bash +pnpm check:fork-only-patches +``` + +Red output names the exact missing files and anchors. Re-apply them, then +re-run. Do not merge a rebuild while this check is red. + +## Adding a patch to the registry + +Add an entry to `scripts/fork-only-patches.json` with: + +- `id`, `owner`, `why` — what breaks in production if it disappears +- `upstreamStatus` — why it is not upstream (so nobody re-litigates it) +- `requiredFiles` — files that must exist +- `requiredAnchors` — files that must still *contain* given symbols, for patches + that modify shared files rather than adding new ones (these are the ones a + rebuild silently reverts, since the file still exists) diff --git a/package.json b/package.json index 644a4e2334..445cd14fa1 100644 --- a/package.json +++ b/package.json @@ -60,7 +60,7 @@ "build": "pnpm -r run build", "build:app-assist": "node desktop/scripts/build-app-assist.cjs", "lint": "pnpm -r run lint", - "check:sources": "pnpm check:brand-guard && pnpm check:no-provider-canary-residue && pnpm check:paw-feel-disposition-boundaries && pnpm check:f290-product-copy && pnpm check:f290-workbench-boundary && pnpm check:dir-size && pnpm check:daemon-lifecycle && pnpm check:capability-tips && pnpm check:co-creation-docs-lane && pnpm check:review-completion-routing && pnpm check:skills:references && pnpm check:skills:surfaces && pnpm check:env-registry && pnpm check:env-example && pnpm check:worktree-reaper && pnpm check:node-runtime-guard && pnpm check:prompt-time-apis && pnpm check:followup-tails && pnpm check:scripts-ascii-only && pnpm check:f212-reconciliation-eval && pnpm check:site-css", + "check:sources": "pnpm check:fork-only-patches && pnpm check:brand-guard && pnpm check:no-provider-canary-residue && pnpm check:paw-feel-disposition-boundaries && pnpm check:f290-product-copy && pnpm check:f290-workbench-boundary && pnpm check:dir-size && pnpm check:daemon-lifecycle && pnpm check:capability-tips && pnpm check:co-creation-docs-lane && pnpm check:review-completion-routing && pnpm check:skills:references && pnpm check:skills:surfaces && pnpm check:env-registry && pnpm check:env-example && pnpm check:worktree-reaper && pnpm check:node-runtime-guard && pnpm check:prompt-time-apis && pnpm check:followup-tails && pnpm check:scripts-ascii-only && pnpm check:f212-reconciliation-eval && pnpm check:site-css", "check:installed": "pnpm check:sidebar-projection-boundary && pnpm biome check . --diagnostic-level=error && pnpm check:biome-review-worktrees && pnpm check:sop-definitions && pnpm check:design-gate-real-interaction && pnpm check:design-md && pnpm check:skills:manifest && pnpm check:env-ports && pnpm check:start-profile-isolation && pnpm check:guides && pnpm check:no-codex-provider-exit-suppression && pnpm check:whisper-dispatch && pnpm check:tts-runtime && pnpm check:site-integrity", "check:artifacts": "pnpm check:paw-feel-disposition-capacity && pnpm check:mcp-surface-governance && pnpm check:features && pnpm check:eval-metric-glossary:prepared && pnpm check:capability-tips-enablement && pnpm check:pre-merge-gate", "check": "pnpm check:sources && pnpm check:installed && pnpm check:artifacts", @@ -72,6 +72,7 @@ "check:mcp-surface-governance": "pnpm --filter @cat-cafe/mcp-server governance:check", "check:no-codex-provider-exit-suppression": "node --test --test-concurrency=1 scripts/check-no-codex-provider-exit-suppression-literal.test.mjs scripts/check-no-codex-provider-exit-suppression-ast.test.mjs scripts/check-no-codex-provider-exit-suppression-isolation.test.mjs scripts/check-no-codex-provider-exit-suppression-structural.test.mjs scripts/check-no-provider-canary-residue.test.mjs && node scripts/check-no-codex-provider-exit-suppression.mjs && node scripts/check-no-provider-canary-residue.mjs", "check:no-provider-canary-residue": "node scripts/check-no-provider-canary-residue.mjs", + "check:fork-only-patches": "node scripts/check-fork-only-patches.mjs", "check:paw-feel-disposition-boundaries": "node --test scripts/check-paw-feel-disposition-boundaries.test.mjs && node scripts/check-paw-feel-disposition-boundaries.mjs", "check:paw-feel-disposition-capacity": "pnpm --filter @cat-cafe/shared build && pnpm --filter @cat-cafe/api build && node --test packages/api/test/harness-eval/paw-feel-disposition-capacity.test.js", "check:f212-reconciliation-eval": "node --test scripts/eval-f212-reconciliation/reconciliation-baseline.test.mjs scripts/eval-f212-reconciliation/reconciliation-window.test.mjs scripts/eval-f212-reconciliation/reconciliation-matcher.test.mjs scripts/eval-f212-reconciliation/reconciliation-sequence.test.mjs scripts/eval-f212-reconciliation/reconciliation-malformed.test.mjs scripts/eval-f212-reconciliation/reconciliation-compression.test.mjs", diff --git a/packages/api/src/index.ts b/packages/api/src/index.ts index b00723a4a7..eee339fd7b 100644 --- a/packages/api/src/index.ts +++ b/packages/api/src/index.ts @@ -7415,6 +7415,10 @@ async function main(): Promise { ); // N-day factory is in its own module (split from eval-domain-daily for file-size limit) const { createEvalDomainNDaySpec } = await import('./infrastructure/harness-eval/domain/eval-domain-nday.js'); + // F192 evidence-source prerequisite gate (fork-only; see docs/fork-only-patches.md). + const { createTelemetryEvidencePrereqProbe } = await import( + './infrastructure/harness-eval/domain/eval-domain-evidence-gate.js' + ); const { getOwnerUserId } = await import('./config/cat-config-loader.js'); // cloud R6 P2 (PR-2) + memory wire-up: mirror the same wired set the // eval-hub.ts route computes (Object.keys(verdictGenerators)). Bootstrap-time @@ -7482,6 +7486,25 @@ async function main(): Promise { publishPrereqCache.set(domainId, ok); return ok; }; + // F192 evidence-source prerequisite gate. OTel init state is fixed for the + // process lifetime (salt is read at boot), so a boolean thunk is a complete input. + // `evidenceTargetIsLocal` asserts the single-process assumption the local handle + // relies on: EVAL_BASE_URL unset (or pointing at our own port) => co-located. + const evidencePrereqProbe = createTelemetryEvidencePrereqProbe({ + otelEnabled: () => telemetryHandle.getMetricsText !== null, + evidenceTargetIsLocal: () => { + const target = process.env.EVAL_BASE_URL; + if (!target) return true; // default target is this process + try { + const { port, hostname } = new URL(target); + const localHost = hostname === 'localhost' || hostname === '127.0.0.1' || hostname === '::1'; + return localHost && port === String(process.env.API_SERVER_PORT ?? ''); + } catch { + return false; // unparseable target => cannot prove co-location => fail closed + } + }, + }); + const evalScheduleOpts = { harnessFeedbackRoot: resolve(repoRoot, 'docs', 'harness-feedback'), threadStore, @@ -7490,6 +7513,7 @@ async function main(): Promise { redis: redisClient ?? undefined, wiredPublishDomains, publishPrereqProbe, + evidencePrereqProbe, triggerStore: redisClient ? new ( await import('./infrastructure/harness-eval/domain/eval-domain-trigger-store.js') diff --git a/packages/api/src/infrastructure/harness-eval/domain/eval-domain-daily.ts b/packages/api/src/infrastructure/harness-eval/domain/eval-domain-daily.ts index 0f4e8bd505..fd25825626 100644 --- a/packages/api/src/infrastructure/harness-eval/domain/eval-domain-daily.ts +++ b/packages/api/src/infrastructure/harness-eval/domain/eval-domain-daily.ts @@ -18,6 +18,11 @@ import type { TaskSpec_P1 } from '../../scheduler/types.js'; import { buildEvalCatInvocation } from '../eval-cat-invocation.js'; import { ensureEvalDomainThreads } from '../hub/eval-hub-thread-ensure.js'; import { inventoryLegacyTasks, type LegacyScheduledTaskLike } from '../legacy-task-cleanup.js'; +import { + buildEvidencePrereqSkippedMessage, + type EvidencePrereqProbe, + evaluateEvidencePrereq, +} from './eval-domain-evidence-gate.js'; import { getEvalCatOverride } from './eval-domain-override.js'; import { type EvalDomainRegistryEntry, @@ -67,6 +72,20 @@ export interface EvalDomainScheduleOpts { * checks for a known-post-fix symbol (e.g. `isA2aSourceRefs` for `eval:a2a`). */ publishPrereqProbe?: (domainId: EvalDomainRegistryEntry['domainId']) => boolean | Promise; + /** + * F192 evidence-source prerequisite gate. + * + * Upstream of `publishPrereqProbe` — answers "can the domain's evidence source + * PRODUCE evidence at all?" before the eval cat is invoked. When OTel is + * disabled, `f167-runtime-eval`-backed domains cannot generate fresh + * snapshots, so invoking the eval cat burns an LLM session to re-conclude a + * gap that is already visible. + * + * Runs PER DOMAIN, PER CRON FIRE, BEFORE publishPrereqProbe. Fail-closed: + * probe throws -> treated as "evidence unavailable" -> skip notice posted. + * Omit/undefined -> backward-compat (no skip). + */ + evidencePrereqProbe?: EvidencePrereqProbe; } /** @deprecated Use EvalDomainScheduleOpts — kept for backward compat. */ @@ -207,6 +226,23 @@ function createEvalDomainSpec(config: EvalDomainSpecConfig): TaskSpec_P1; + +export type EvidencePrereqResult = { ok: true } | { ok: false; reason: string }; + +export type EvidencePrereqProbe = (domain: EvidenceGateDomain) => EvidencePrereqResult | Promise; + +/** + * Source adapters whose evidence pipeline hard-requires live OTel telemetry. + * Registry `sourceAdapter` is a free slug (see eval-domain-registry.ts), so + * the adapter → prerequisite mapping lives here, next to the probe. + */ +const TELEMETRY_BACKED_ADAPTERS: ReadonlySet = new Set(['f167-runtime-eval']); + +export function isTelemetryBackedAdapter(sourceAdapter: string): boolean { + return TELEMETRY_BACKED_ADAPTERS.has(sourceAdapter); +} + +/** + * Probe factory. Bootstrap wires `otelEnabled: () => !!telemetryHandle.getMetricsText` + * — the same init-state signal `GET /api/telemetry/health` reports as + * `otelEnabled` (routes/telemetry.ts Phase K note: actual init state, not an + * env-var proxy). Non-telemetry-backed adapters always pass through. + */ +export function createTelemetryEvidencePrereqProbe(opts: { + otelEnabled: () => boolean; + /** Override the reason text; defaults to the health route's disabledReason derivation. */ + disabledReason?: () => string; + /** + * Upstream review finding (zts212653/clowder-ai#1352, comment by 砚砚): + * `otelEnabled` observes the telemetry handle of the process running the CRON, + * but eval evidence is fetched from the adapter target (`EVAL_BASE_URL`, + * default `localhost:`). When those are different runtimes the + * local handle says nothing about the remote source: a telemetry-disabled + * scheduler would skip a healthy remote source, and a telemetry-enabled + * scheduler would admit an unavailable one. + * + * This fork runs scheduler and evidence source in ONE process, so the local + * handle is authoritative here. Rather than leave that accidental, the caller + * asserts it. Returning `false` fails the gate closed with an explicit reason + * instead of letting the local handle silently speak for a remote target. + * + * Omit → assumes co-located (this fork's single-process deployment). + */ + evidenceTargetIsLocal?: () => boolean; +}): EvidencePrereqProbe { + return (domain) => { + if (!isTelemetryBackedAdapter(domain.sourceAdapter)) return { ok: true }; + if (opts.evidenceTargetIsLocal && !opts.evidenceTargetIsLocal()) { + return { + ok: false, + reason: + 'evidence source is a remote runtime (EVAL_BASE_URL points off-process); ' + + 'the scheduler-local OTel handle cannot prove remote evidence availability', + }; + } + if (opts.otelEnabled()) return { ok: true }; + const reason = + opts.disabledReason?.() ?? + (process.env.OTEL_SDK_DISABLED === 'true' + ? 'OTel disabled by OTEL_SDK_DISABLED=true' + : 'OTel disabled at boot: HMAC salt validation failed (TELEMETRY_HMAC_SALT not configured)'); + return { ok: false, reason }; + }; +} + +/** Fail-closed evaluation: a probe that throws is treated as "evidence unavailable". */ +export async function evaluateEvidencePrereq( + probe: EvidencePrereqProbe, + domain: EvidenceGateDomain, +): Promise { + try { + return await Promise.resolve(probe(domain)); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + return { ok: false, reason: `evidence prereq probe threw: ${message}` }; + } +} + +/** + * Derive the actionable next step from the DETECTED cause. + * + * Upstream review finding (zts212653/clowder-ai#1352, comment by 砚砚): the + * notice previously recommended configuring `TELEMETRY_HMAC_SALT` even when + * the detected cause was an intentional `OTEL_SDK_DISABLED=true`. Telling an + * operator to set a salt they already set — for a toggle they deliberately + * flipped — sends them down the wrong path. Match the advice to the cause. + */ +function buildNextActionLines(reason: string): string[] { + if (reason.includes('OTEL_SDK_DISABLED')) { + return [ + 'Next action: OTel is intentionally disabled via `OTEL_SDK_DISABLED=true`.', + 'Unset it (or set it to `false`) and restart the API runtime to resume', + "evidence collection, or set `enabled: false` in this domain's registry", + 'YAML to pause the schedule intentionally.', + ]; + } + if (reason.includes('TELEMETRY_HMAC_SALT') || /HMAC salt/i.test(reason)) { + return [ + 'Next action: configure a non-empty `TELEMETRY_HMAC_SALT` for the API', + 'runtime and restart it (OTel initializes at boot), or set `enabled: false`', + "in this domain's registry YAML to pause the schedule intentionally.", + ]; + } + return [ + 'Next action: resolve the blocker quoted above on the API runtime and', + "restart it, or set `enabled: false` in this domain's registry YAML to", + 'pause the schedule intentionally.', + ]; +} + +/** + * Stable-header skip notice posted to the domain's OWN system thread when the + * cron fails closed. Header format mirrors `buildPublishPrereqSkippedMessage` + * so eval-domain readers / log scrubbers can grep both skip classes uniformly. + */ +export function buildEvidencePrereqSkippedMessage(domain: EvidenceGateDomain, reason: string): string { + return [ + `## Eval Domain: ${domain.domainId} — SKIPPED (evidence source unavailable)`, + '', + "The scheduled eval was skipped because this domain's evidence source", + `(\`${domain.sourceAdapter}\`) cannot produce evidence on this runtime:`, + '', + `> ${reason}`, + '', + 'Why this matters: invoking the eval cat without a live evidence source', + 'burns a full LLM session to re-conclude the same gap on every fire. The', + 'fail-closed skip keeps the gap visible in this thread at zero LLM cost.', + '', + ...buildNextActionLines(reason), + ].join('\n'); +} diff --git a/packages/api/src/infrastructure/harness-eval/domain/eval-domain-nday.ts b/packages/api/src/infrastructure/harness-eval/domain/eval-domain-nday.ts index 6e9f2355dd..5acc37382b 100644 --- a/packages/api/src/infrastructure/harness-eval/domain/eval-domain-nday.ts +++ b/packages/api/src/infrastructure/harness-eval/domain/eval-domain-nday.ts @@ -20,6 +20,7 @@ import { type EvalDomainScheduleOpts, evaluatePublishPrereq, } from './eval-domain-daily.js'; +import { buildEvidencePrereqSkippedMessage, evaluateEvidencePrereq } from './eval-domain-evidence-gate.js'; import { getEvalCatOverride } from './eval-domain-override.js'; import { type EvalDomainRegistryEntry, @@ -157,6 +158,23 @@ export function createEvalDomainNDaySpec(opts: EvalDomainScheduleOpts): TaskSpec } // Direction B publish-prereq gate (same as daily/weekly spec) + // F192 evidence-source prereq gate (parity with daily/weekly). + // Critically: returns BEFORE the Redis last-dispatch write, so a skip does + // NOT consume the N-day window — the domain retries on the next daily probe. + if (opts.evidencePrereqProbe) { + const evidenceResult = await evaluateEvidencePrereq(opts.evidencePrereqProbe, domain); + if (!evidenceResult.ok) { + if (ctx.deliver) { + await ctx.deliver({ + threadId: domain.systemThreadId, + content: buildEvidencePrereqSkippedMessage(domain, evidenceResult.reason), + userId: 'scheduler', + }); + } + return; + } + } + if (opts.publishPrereqProbe) { const prereqOk = await evaluatePublishPrereq(opts.publishPrereqProbe, domain.domainId); if (!prereqOk) { diff --git a/packages/api/test/harness-eval/eval-domain-evidence-gate.test.js b/packages/api/test/harness-eval/eval-domain-evidence-gate.test.js new file mode 100644 index 0000000000..baf692eaff --- /dev/null +++ b/packages/api/test/harness-eval/eval-domain-evidence-gate.test.js @@ -0,0 +1,336 @@ +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import { describe, it, mock } from 'node:test'; +import { fileURLToPath } from 'node:url'; +import { createEvalDomainDailySpec } from '../../dist/infrastructure/harness-eval/domain/eval-domain-daily.js'; +import { + buildEvidencePrereqSkippedMessage, + createTelemetryEvidencePrereqProbe, + evaluateEvidencePrereq, +} from '../../dist/infrastructure/harness-eval/domain/eval-domain-evidence-gate.js'; +import { createEvalDomainNDaySpec } from '../../dist/infrastructure/harness-eval/domain/eval-domain-nday.js'; +import { FIXTURE_FRICTION_3D_YAML, makeTempRoot } from './eval-domain-nday-fixtures.js'; + +const repoHarnessFeedbackRoot = fileURLToPath(new URL('../../../../docs/harness-feedback', import.meta.url)); + +/** + * Evidence-source prereq gate (eval:a2a build verdict + * `2026-07-07-eval-a2a-reeval-telemetry-still-disabled-build`, PR #19). + * + * Bug class: the scheduled eval fires on a runtime whose OTel telemetry is + * disabled (TELEMETRY_HMAC_SALT unset → initTelemetry() returned null handles). + * The `f167-runtime-eval` source cannot produce fresh snapshots, yet the eval + * cat is invoked anyway and burns a full LLM session to re-conclude "telemetry + * still disabled" — every day (2026-06-30 → 2026-07-07 verdict series). The + * gate fails closed BEFORE invocation and posts a zero-LLM-cost skip notice + * to the domain's own system thread instead. + */ +describe('eval-domain evidence-source prereq gate (fork-only; mindfn PR #91/#137)', () => { + /** + * RFC 5.4: the eval cat is woken through the DELIVER seam - the envelope carries + * `targetCatId` + `privateContent`. The old positional `invokeTrigger.trigger()` + * call is dead, so asserting `triggerMock.callCount() === 0` proves NOTHING about + * whether the cat was invoked (it is trivially 0 on every path). + * + * `catWakes()` counts real wakes on the live seam, so "cat never invoked" is a + * claim this suite can actually falsify. + */ + const mkCtx = () => { + const deliverMock = mock.fn(async () => 'msg_evidence'); + const triggerMock = mock.fn(); + const catWakes = () => + deliverMock.mock.calls + .map((c) => c.arguments[0]) + .filter((env) => env?.targetCatId != null || env?.privateContent != null); + return { + deliverMock, + triggerMock, + catWakes, + ctx: { assignedCatId: null, deliver: deliverMock, invokeTrigger: { trigger: triggerMock } }, + }; + }; + + describe('createTelemetryEvidencePrereqProbe', () => { + it('telemetry-backed adapter + OTel disabled → not ok, reason points at salt', () => { + const probe = createTelemetryEvidencePrereqProbe({ otelEnabled: () => false }); + const result = probe({ domainId: 'eval:a2a', sourceAdapter: 'f167-runtime-eval' }); + assert.equal(result.ok, false); + assert.ok( + result.reason.includes('TELEMETRY_HMAC_SALT'), + `default reason must name the missing salt env var (got: ${result.reason})`, + ); + }); + + it('telemetry-backed adapter + OTel enabled → ok', () => { + const probe = createTelemetryEvidencePrereqProbe({ otelEnabled: () => true }); + const result = probe({ domainId: 'eval:a2a', sourceAdapter: 'f167-runtime-eval' }); + assert.equal(result.ok, true); + }); + + it('non-telemetry adapter passes through even when OTel is disabled', () => { + const probe = createTelemetryEvidencePrereqProbe({ otelEnabled: () => false }); + const result = probe({ domainId: 'eval:sop', sourceAdapter: 'sop-trace-eval' }); + assert.equal(result.ok, true, 'gate must only constrain telemetry-backed source adapters'); + }); + + it('OTEL_SDK_DISABLED=true → reason names the env toggle, not the salt', () => { + const prev = process.env.OTEL_SDK_DISABLED; + process.env.OTEL_SDK_DISABLED = 'true'; + try { + const probe = createTelemetryEvidencePrereqProbe({ otelEnabled: () => false }); + const result = probe({ domainId: 'eval:a2a', sourceAdapter: 'f167-runtime-eval' }); + assert.equal(result.ok, false); + assert.ok(result.reason.includes('OTEL_SDK_DISABLED'), `got: ${result.reason}`); + } finally { + if (prev === undefined) delete process.env.OTEL_SDK_DISABLED; + else process.env.OTEL_SDK_DISABLED = prev; + } + }); + }); + + /** + * Upstream review finding (zts212653/clowder-ai#1352): `otelEnabled` observes the + * CRON process's telemetry handle, but evidence is fetched from the adapter target + * (EVAL_BASE_URL). When those differ the local handle proves nothing about the + * remote source, in BOTH directions. The gate must fail closed rather than let a + * local handle speak for a remote target. + */ + describe('evidence target co-location assertion', () => { + it('remote evidence target -> fail closed even when local OTel is healthy', () => { + const probe = createTelemetryEvidencePrereqProbe({ + otelEnabled: () => true, + evidenceTargetIsLocal: () => false, + }); + const result = probe({ domainId: 'eval:a2a', sourceAdapter: 'f167-runtime-eval' }); + assert.equal(result.ok, false, 'a healthy LOCAL handle must not admit an unproven REMOTE source'); + assert.ok(result.reason.includes('remote runtime'), `got: ${result.reason}`); + }); + + it('co-located evidence target + healthy OTel -> ok', () => { + const probe = createTelemetryEvidencePrereqProbe({ + otelEnabled: () => true, + evidenceTargetIsLocal: () => true, + }); + assert.equal(probe({ domainId: 'eval:a2a', sourceAdapter: 'f167-runtime-eval' }).ok, true); + }); + + it('omitted assertion stays backward-compatible (assumes co-located)', () => { + const probe = createTelemetryEvidencePrereqProbe({ otelEnabled: () => true }); + assert.equal(probe({ domainId: 'eval:a2a', sourceAdapter: 'f167-runtime-eval' }).ok, true); + }); + + it('co-location is checked before the telemetry handle, not after', () => { + const probe = createTelemetryEvidencePrereqProbe({ + otelEnabled: () => false, + evidenceTargetIsLocal: () => false, + }); + const result = probe({ domainId: 'eval:a2a', sourceAdapter: 'f167-runtime-eval' }); + assert.ok( + result.reason.includes('remote runtime'), + `remote-target cause must win over the local salt cause (got: ${result.reason})`, + ); + }); + }); + + /** + * Upstream review finding (zts212653/clowder-ai#1352): the notice recommended + * configuring TELEMETRY_HMAC_SALT even when the cause was an intentional + * OTEL_SDK_DISABLED=true - sending the operator down the wrong path. + */ + describe('skip notice next-action matches the detected cause', () => { + const domain = { domainId: 'eval:a2a', sourceAdapter: 'f167-runtime-eval' }; + + it('OTEL_SDK_DISABLED cause -> advises the toggle, NOT the salt', () => { + const msg = buildEvidencePrereqSkippedMessage(domain, 'OTel disabled by OTEL_SDK_DISABLED=true'); + assert.ok(msg.includes('OTEL_SDK_DISABLED'), 'must name the actual toggle'); + assert.ok( + !msg.includes('configure a non-empty `TELEMETRY_HMAC_SALT`'), + 'must NOT tell the operator to set a salt when the cause is a deliberate toggle', + ); + }); + + it('HMAC salt cause -> advises configuring the salt', () => { + const msg = buildEvidencePrereqSkippedMessage( + domain, + 'OTel disabled at boot: HMAC salt validation failed (TELEMETRY_HMAC_SALT not configured)', + ); + assert.ok(msg.includes('configure a non-empty `TELEMETRY_HMAC_SALT`')); + }); + + it('unrecognized cause -> generic advice, no misleading specifics', () => { + const msg = buildEvidencePrereqSkippedMessage(domain, 'evidence prereq probe threw: boom'); + assert.ok(msg.includes('resolve the blocker quoted above')); + assert.ok(!msg.includes('TELEMETRY_HMAC_SALT'), 'must not invent a salt diagnosis'); + assert.ok(!msg.includes('OTEL_SDK_DISABLED'), 'must not invent a toggle diagnosis'); + }); + + it('every cause keeps the stable grep header and quotes the reason', () => { + for (const reason of ['OTel disabled by OTEL_SDK_DISABLED=true', 'HMAC salt validation failed', 'boom']) { + const msg = buildEvidencePrereqSkippedMessage(domain, reason); + assert.ok(msg.includes('SKIPPED (evidence source unavailable)'), 'stable header for grep/dedup'); + assert.ok(msg.includes(`> ${reason}`), 'reason must be quoted verbatim'); + } + }); + }); + + it('bootstrap wires the telemetry init state into every scheduled eval spec', () => { + const indexSource = readFileSync(new URL('../../src/index.ts', import.meta.url), 'utf8'); + + assert.match(indexSource, /createTelemetryEvidencePrereqProbe/); + assert.match( + indexSource, + /otelEnabled:\s*\(\)\s*=>\s*telemetryHandle\.getMetricsText\s*!==\s*null/, + 'the probe must observe the actual boot-time telemetry handle, not an env proxy', + ); + assert.match( + indexSource, + /const evalScheduleOpts = \{[\s\S]*?evidencePrereqProbe,[\s\S]*?\};/, + 'shared daily, weekly, and N-day schedule options must include the evidence probe', + ); + }); + + describe('evaluateEvidencePrereq', () => { + it('probe throw → fail-closed not-ok with reason', async () => { + const result = await evaluateEvidencePrereq( + () => { + throw new Error('synthetic evidence probe failure'); + }, + { domainId: 'eval:a2a', sourceAdapter: 'f167-runtime-eval' }, + ); + assert.equal(result.ok, false); + assert.ok(result.reason.includes('synthetic evidence probe failure')); + }); + }); + + describe('daily spec integration', () => { + async function getA2aItem(spec) { + const gateResult = await spec.admission.gate(); + const item = gateResult.workItems.find((w) => w.subjectKey === 'eval:a2a'); + assert.ok(item, 'eval:a2a must be a registered daily domain'); + return item; + } + + it('probe not-ok → SKIPPED notice in domain thread, cat never invoked', async () => { + const spec = createEvalDomainDailySpec({ + harnessFeedbackRoot: repoHarnessFeedbackRoot, + defaultUserId: 'default-user', + evidencePrereqProbe: () => ({ ok: false, reason: 'OTel disabled at boot: HMAC salt validation failed' }), + }); + const item = await getA2aItem(spec); + const { deliverMock, catWakes, ctx } = mkCtx(); + + await spec.run.execute(item.signal, item.subjectKey, ctx); + + assert.equal(catWakes().length, 0, 'eval cat must NOT be woken when evidence source is down'); + assert.equal(deliverMock.mock.callCount(), 1); + const call = deliverMock.mock.calls[0].arguments[0]; + assert.equal(call.threadId, 'thread_eval_a2a', 'skip notice must stay in the domain system thread'); + assert.equal(call.userId, 'scheduler'); + assert.ok(call.content.includes('SKIPPED (evidence source unavailable)'), 'stable header for grep/dedup'); + assert.ok(call.content.includes('HMAC salt validation failed'), 'notice must carry the probe reason'); + assert.ok(call.content.includes('TELEMETRY_HMAC_SALT'), 'notice must state the actionable next step'); + }); + + it('probe ok → normal invocation proceeds', async () => { + const spec = createEvalDomainDailySpec({ + harnessFeedbackRoot: repoHarnessFeedbackRoot, + defaultUserId: 'default-user', + evidencePrereqProbe: () => ({ ok: true }), + }); + const item = await getA2aItem(spec); + const { deliverMock, catWakes, ctx } = mkCtx(); + + await spec.run.execute(item.signal, item.subjectKey, ctx); + + assert.equal(catWakes().length, 1, 'eval cat is woken when evidence source is healthy'); + assert.equal(deliverMock.mock.callCount(), 1); + assert.ok(!deliverMock.mock.calls[0].arguments[0].content.includes('SKIPPED')); + }); + + it('probe throws → fail-closed skip, no crash, no LLM call', async () => { + const spec = createEvalDomainDailySpec({ + harnessFeedbackRoot: repoHarnessFeedbackRoot, + defaultUserId: 'default-user', + evidencePrereqProbe: () => { + throw new Error('synthetic gate crash'); + }, + }); + const item = await getA2aItem(spec); + const { deliverMock, catWakes, ctx } = mkCtx(); + + await spec.run.execute(item.signal, item.subjectKey, ctx); + + assert.equal(catWakes().length, 0, 'a throwing probe must fail closed without waking the cat'); + assert.equal(deliverMock.mock.callCount(), 1); + assert.ok(deliverMock.mock.calls[0].arguments[0].content.includes('SKIPPED (evidence source unavailable)')); + }); + + it('both gates failing → evidence-source message wins (upstream-first ordering)', async () => { + const spec = createEvalDomainDailySpec({ + harnessFeedbackRoot: repoHarnessFeedbackRoot, + defaultUserId: 'default-user', + evidencePrereqProbe: () => ({ ok: false, reason: 'OTel disabled at boot' }), + publishPrereqProbe: () => false, + }); + const item = await getA2aItem(spec); + const { deliverMock, catWakes, ctx } = mkCtx(); + + await spec.run.execute(item.signal, item.subjectKey, ctx); + + assert.equal(catWakes().length, 0); + assert.equal(deliverMock.mock.callCount(), 1, 'exactly one skip notice, not two'); + const content = deliverMock.mock.calls[0].arguments[0].content; + assert.ok(content.includes('evidence source unavailable'), 'evidence gate runs before publish gate'); + assert.ok(!content.includes('publish prereq missing')); + }); + + it('evidence probe ok + publish gate still enforced → publish skip preserved', async () => { + const spec = createEvalDomainDailySpec({ + harnessFeedbackRoot: repoHarnessFeedbackRoot, + defaultUserId: 'default-user', + evidencePrereqProbe: () => ({ ok: true }), + publishPrereqProbe: () => false, + }); + const item = await getA2aItem(spec); + const { deliverMock, catWakes, ctx } = mkCtx(); + + await spec.run.execute(item.signal, item.subjectKey, ctx); + + assert.equal(catWakes().length, 0); + assert.equal(deliverMock.mock.callCount(), 1); + assert.ok(deliverMock.mock.calls[0].arguments[0].content.includes('publish prereq missing')); + }); + }); + + describe('nday spec integration', () => { + it('probe not-ok → skip notice, no trigger, no Redis last-dispatch write', async () => { + const redisSet = mock.fn(async () => 'OK'); + const redis = { get: mock.fn(async () => null), set: redisSet }; + const harnessFeedbackRoot = makeTempRoot(FIXTURE_FRICTION_3D_YAML); + const spec = createEvalDomainNDaySpec({ + harnessFeedbackRoot, + defaultUserId: 'default-user', + redis, + evidencePrereqProbe: () => ({ ok: false, reason: 'OTel disabled at boot' }), + }); + + const gateResult = await spec.admission.gate(); + assert.equal(gateResult.run, true, 'registry must contain at least one N-day domain'); + const item = gateResult.workItems[0]; + const { deliverMock, catWakes, ctx } = mkCtx(); + + await spec.run.execute(item.signal, item.subjectKey, ctx); + + assert.equal(catWakes().length, 0, 'eval cat must NOT be woken when evidence source is down'); + assert.equal(deliverMock.mock.callCount(), 1); + const call = deliverMock.mock.calls[0].arguments[0]; + assert.equal(call.threadId, item.signal.systemThreadId); + assert.ok(call.content.includes('SKIPPED (evidence source unavailable)')); + assert.equal( + redisSet.mock.callCount(), + 0, + 'skip must NOT consume the N-day window — domain retries on next daily probe', + ); + }); + }); +}); diff --git a/scripts/check-fork-only-patches.mjs b/scripts/check-fork-only-patches.mjs new file mode 100755 index 0000000000..a5ef7419e3 --- /dev/null +++ b/scripts/check-fork-only-patches.mjs @@ -0,0 +1,104 @@ +#!/usr/bin/env node +/** + * Fork-only patch guard. + * + * Problem this solves: this fork carries patches that upstream main does NOT + * have. Every `develop_base` rebuild onto upstream main silently drops them. + * The F192 evidence-prerequisite gate was lost three times this way (PR #91, + * PR #137, commit 70d79a2f2) and each loss went unnoticed for days-to-weeks + * while the scheduler burned a full LLM session per daily eval fire. + * + * Maintainer direction (zts212653/clowder-ai#1352, closed 2026-08-25): + * "this is fork-specific harness logic that doesn't belong in upstream. The + * rebuild-loss issue should be solved by improving our fork's develop_base + * rebuild SOP (rebase fork-only patches), not by pushing internal code into + * the community repo." + * + * "Remembering to re-apply" is not a mechanism. This guard is: it asserts each + * registered patch is still present, so a rebuild that drops one turns + * `pnpm check` RED immediately instead of failing silently in production. + * + * Fail-CLOSED: a missing/unreadable/malformed registry is a violation, not a + * pass. A guard whose own input vanished must not read as clean. + * + * Truth source: scripts/fork-only-patches.json (see docs/fork-only-patches.md). + */ + +import { readFileSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = join(__dirname, '..'); +const REGISTRY = join(__dirname, 'fork-only-patches.json'); + +const violations = []; + +function readRegistry() { + let raw; + try { + raw = readFileSync(REGISTRY, 'utf8'); + } catch (err) { + violations.push(`registry unreadable at scripts/fork-only-patches.json: ${err.message}`); + return null; + } + let parsed; + try { + parsed = JSON.parse(raw); + } catch (err) { + violations.push(`registry is not valid JSON: ${err.message}`); + return null; + } + if (!parsed || !Array.isArray(parsed.patches)) { + violations.push('registry is missing a top-level "patches" array'); + return null; + } + return parsed; +} + +function checkPatch(patch) { + const id = patch.id ?? ''; + + for (const rel of patch.requiredFiles ?? []) { + try { + readFileSync(join(REPO_ROOT, rel), 'utf8'); + } catch { + violations.push(`[${id}] required file missing: ${rel}`); + } + } + + for (const anchor of patch.requiredAnchors ?? []) { + let content; + try { + content = readFileSync(join(REPO_ROOT, anchor.file), 'utf8'); + } catch { + violations.push(`[${id}] anchor file missing: ${anchor.file}`); + continue; + } + for (const needle of anchor.mustContain ?? []) { + if (!content.includes(needle)) { + const note = anchor.note ? ` (${anchor.note})` : ''; + violations.push(`[${id}] ${anchor.file} lost anchor ${JSON.stringify(needle)}${note}`); + } + } + } +} + +const registry = readRegistry(); +if (registry) { + for (const patch of registry.patches) checkPatch(patch); +} + +if (violations.length > 0) { + console.error('[check-fork-only-patches] FAIL - fork-only patch(es) missing from this tree.\n'); + for (const v of violations) console.error(` - ${v}`); + const ids = (registry?.patches ?? []).map((p) => p.id).join(', '); + console.error('\nThis almost always means a develop_base rebuild onto upstream main dropped them.'); + console.error('Re-apply the listed patches, then re-run this check.'); + console.error(`Registered fork-only patches: ${ids || '(registry unavailable)'}`); + console.error('Context: docs/fork-only-patches.md'); + process.exit(1); +} + +const count = registry.patches.length; +console.log(`[check-fork-only-patches] OK - ${count} fork-only patch(es) present`); diff --git a/scripts/fork-only-patches.json b/scripts/fork-only-patches.json new file mode 100644 index 0000000000..84315ed665 --- /dev/null +++ b/scripts/fork-only-patches.json @@ -0,0 +1,33 @@ +{ + "$comment": "Single source of truth for fork-only patches that upstream main does NOT carry. Consumed by scripts/check-fork-only-patches.mjs (wired into `pnpm check`). See docs/fork-only-patches.md. Every develop_base rebuild from upstream main MUST re-apply these; the guard turns a silent loss into a red check.", + "patches": [ + { + "id": "f192-evidence-prereq-gate", + "owner": "opus", + "why": "Skips scheduled eval-cat invocation when the domain's evidence source cannot produce evidence (OTel disabled). Without it every daily eval:a2a fire burns a full LLM session to re-conclude a known gap.", + "upstreamStatus": "rejected-as-fork-scope (zts212653/clowder-ai#1352 closed 2026-08-25; issue #1353 accepted the direction then closed as fork-specific)", + "lostInRebuilds": ["PR #91 (2026-08-11)", "PR #137 (2026-08-28)", "commit 70d79a2f2 (2026-09-01)"], + "requiredFiles": [ + "packages/api/src/infrastructure/harness-eval/domain/eval-domain-evidence-gate.ts", + "packages/api/test/harness-eval/eval-domain-evidence-gate.test.js" + ], + "requiredAnchors": [ + { + "file": "packages/api/src/infrastructure/harness-eval/domain/eval-domain-daily.ts", + "mustContain": ["evidencePrereqProbe", "evaluateEvidencePrereq", "buildEvidencePrereqSkippedMessage"], + "note": "opts interface field + execute-time gate, ordered BEFORE publishPrereqProbe" + }, + { + "file": "packages/api/src/infrastructure/harness-eval/domain/eval-domain-nday.ts", + "mustContain": ["evidencePrereqProbe", "evaluateEvidencePrereq", "buildEvidencePrereqSkippedMessage"], + "note": "N-day parity; must return before the Redis last-dispatch write" + }, + { + "file": "packages/api/src/index.ts", + "mustContain": ["createTelemetryEvidencePrereqProbe", "evidencePrereqProbe,", "evidenceTargetIsLocal"], + "note": "bootstrap: import, probe construction from telemetryHandle, inclusion in evalScheduleOpts" + } + ] + } + ] +} From d12ef51f9c72c14474f47247d466fcb73b783ab2 Mon Sep 17 00:00:00 2001 From: Ragdoll-Opus-5 Date: Tue, 22 Sep 2026 11:34:45 +0800 Subject: [PATCH 2/5] fix(F192): address review - drop the mis-wired env gate, detect loss from outside the tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: 砚砚's review of PR #185 found two P1 defects. Both were real, and verifying them showed each fix was aimed at the wrong thing. ## P1 - the co-location gate was wired to unrelated configuration Confirmed all three claims: - `EVAL_BASE_URL` is read only by the manual CLI scripts/run-f168-external-case-eval.mjs. The scheduled path never consults it. - The port compare was wrong in the documented local case: with `EVAL_BASE_URL=http://localhost:3004` it evaluated `"3004" === "3002"` (and `"3004" === ""` with no override) - false. It fell CLOSED on exactly the local topology it was meant to admit. - `new URL('http://[::1]:3002').hostname` is `"[::1]"`, so the `::1` branch was dead code. Repaired by removal, not by fixing the comparison. The scheduled path has no remote evidence target to check: f167-runtime-eval evidence is a snapshot+attribution pair ON DISK under the harness-feedback root (publish-verdict/a2a-generator-adapter.ts resolves it filesystem-relative), written by this process's own OTel pipeline. The local handle is authoritative because the producer IS this process - structurally, not accidentally. The four old tests injected the boolean closure directly, so they passed while the wiring was broken. Replaced with regressions that pin the real contract, including the default-port and `[::1]` cases that used to fail closed. ## P1 - the guard could not report its own erasure Also confirmed: base aa409203b has neither the checker, the registry, nor the package.json wiring. A tree-only check reads its registry FROM the tree, so the rebuild that drops the patches drops the guard too, and nothing goes red. That is how three losses stayed silent. Root cause, from commit 70d79a2f2's own body: the rebuild is "reset to origin/main + fork/optimizations", and "443 files modified by BOTH main and fork: NOT restored" - which is precisely the requiredAnchors class. Carrier membership is NOT the fix: fork/optimizations is an orphan snapshot (7 commits, no merge-base with upstream main, a July full-tree copy), so demanding current code be committed onto a stale unrelated history is incoherent. Verified before building on it. What does survive a reset is the remote-tracking ref. `origin/develop_base` still holds the previous tree's registry - and this script - after the rebuild has erased both from disk. The guard now compares what the previous tree claimed to protect against what the new tree has, and stays runnable via `git show origin/develop_base:scripts/check-fork-only-patches.mjs`. Proven with a rebuild fixture that wipes registry + guard + patch and asserts the loss is still named, including the guard's own erasure. Also: every workflow in this repo triggers on `main`, so nothing ran on develop_base - the branch that loses the patches. Added a dedicated workflow there (no paths-ignore: a rebuild can drop a patch whatever its diff touches). ## P2 - anchors could be satisfied by imports and comments Added `order` anchors pinned to CALL SITES, so a gate moved after the step it must precede is caught; a bare symbol would have matched the import. Hollow registry entries (no id, or asserting nothing) and an emptied registry now fail closed. Verified: 57/57 tests (guard 16, evidence-gate 20, daily + prereq 21), api build, biome clean repo-wide, git diff --check clean. Co-Authored-By: Claude Opus 5 --- .github/workflows/fork-only-patches.yml | 43 +++ docs/fork-only-patches.md | 76 ++++- package.json | 2 +- packages/api/src/index.ts | 18 +- .../domain/eval-domain-evidence-gate.ts | 52 +-- .../eval-domain-evidence-gate.test.js | 77 +++-- scripts/check-fork-only-patches.mjs | 237 +++++++++++-- scripts/check-fork-only-patches.test.mjs | 317 ++++++++++++++++++ scripts/fork-only-patches.json | 35 +- 9 files changed, 754 insertions(+), 103 deletions(-) create mode 100644 .github/workflows/fork-only-patches.yml create mode 100644 scripts/check-fork-only-patches.test.mjs diff --git a/.github/workflows/fork-only-patches.yml b/.github/workflows/fork-only-patches.yml new file mode 100644 index 0000000000..93c19a6101 --- /dev/null +++ b/.github/workflows/fork-only-patches.yml @@ -0,0 +1,43 @@ +# Fork-only patch guard. +# +# Every other workflow in this repo triggers on `main`. But fork-only patches +# are lost on `develop_base` - that is the branch rebuilt onto upstream main, +# and the branch every fork PR targets. Without this workflow the guard would +# only ever run when a human remembered to run `pnpm check`, which is the same +# failure mode it was built to remove. +# +# Deliberately NOT gated by paths-ignore: a rebuild can drop a patch regardless +# of which paths its own diff appears to touch. +# +# This file is itself a registered fork-only patch (see +# scripts/fork-only-patches.json), so a rebuild that drops the guard's CI +# trigger is caught by the local `pnpm check` run. +name: Fork-only patches + +on: + push: + branches: [develop_base] + pull_request: + branches: [develop_base] + workflow_dispatch: + +concurrency: + group: fork-only-patches-${{ github.ref }} + cancel-in-progress: true + +jobs: + guard: + name: Fork-only patch guard + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + # The rebuild-survival check compares against the previous develop_base + # tip, which actions/checkout does not fetch by default. + - run: git fetch --no-tags --depth=1 origin develop_base:refs/remotes/origin/develop_base + - uses: actions/setup-node@v4 + with: + node-version: 24 + # No dependency install: the guard uses node builtins only, so it stays + # runnable even when the tree is mid-rebuild and deps are inconsistent. + - run: node --test scripts/check-fork-only-patches.test.mjs + - run: node scripts/check-fork-only-patches.mjs diff --git a/docs/fork-only-patches.md b/docs/fork-only-patches.md index 4763d5db91..b00f6a7e7c 100644 --- a/docs/fork-only-patches.md +++ b/docs/fork-only-patches.md @@ -53,16 +53,82 @@ fork version rather than carried forward blindly: the detected cause was an intentional `OTEL_SDK_DISABLED=true`. The next-step text is now derived from the detected cause. +## Why a tree-only check is not enough + +A develop_base rebuild is, quoting commit `70d79a2f2`'s own body: + +> The develop_base rebuild (b739c279c) reset to origin/main + fork/optimizations +> but dropped all develop_base-only merges - 33 first-parent commits [...] +> 443 files modified by BOTH main and fork: **NOT restored** + +Two things follow. + +First, the shared-file edits (the `requiredAnchors` below) fall exactly in that +"modified by BOTH / not restored" bucket. The file survives, the fork's edit +inside it does not - so an existence check reads clean while the patch is gone. + +Second, and worse: a check that reads its registry **from the tree** cannot +report on a rebuild that replaces the tree. The reset takes the registry and +this script along with the patches, and a guard that is not there does not go +red. That is how three losses stayed silent. + +`fork/optimizations` does not solve this either. It is an orphan snapshot - 7 +commits, no merge-base with upstream main, a July full-tree copy - so +"is the patch in the carrier" is not a survival predicate, and asking for +current code to be committed onto a stale unrelated history is incoherent. + +## What actually survives: the remote-tracking ref + +`git reset --hard upstream/main` replaces the working tree. It does **not** move +`origin/develop_base`. The previous tree's registry is therefore still readable +straight out of git after the rebuild has erased it from disk: + +```bash +git show origin/develop_base:scripts/fork-only-patches.json +``` + +So the guard compares what the previous tree **claimed to protect** against what +the new tree **actually has**. It stays runnable even when the rebuild deleted +it, because it can be read from the same ref: + +```bash +git show origin/develop_base:scripts/check-fork-only-patches.mjs > /tmp/guard.mjs +node /tmp/guard.mjs +``` + +## Two checks, both required + +```bash +pnpm check:fork-only-patches +``` + +1. **Tree check** - is each registered patch here right now? Answers + "is production broken". +2. **Rebuild-survival check** - is everything `baselineRef` claimed still + present? Answers "did this rebuild drop something", **before** the result is + pushed or deployed. + +Both fail closed: a missing registry, an unreachable baseline, a hollow patch +entry, or an anchor that moved after the step it must precede are all red. A +guard that cannot see its input must not report clean. The one deliberate +exception is a baseline that carried no registry at all - an absent prior claim +is not an unverifiable one. + ## Rebuild SOP -After any `develop_base` rebuild onto upstream main: +Run the guard as the **last step of a rebuild, before pushing**: ```bash +git fetch origin develop_base # baseline must be present pnpm check:fork-only-patches ``` -Red output names the exact missing files and anchors. Re-apply them, then -re-run. Do not merge a rebuild while this check is red. +Red output names each dropped file and anchor. Re-apply them, then re-run. Do +not push or deploy a rebuild while this check is red. + +CI runs the same guard on every push and PR to `develop_base` +(`.github/workflows/fork-only-patches.yml`) - every other workflow in this repo +triggers on `main` only, which is why losses on develop_base went unseen. ## Adding a patch to the registry @@ -74,3 +140,7 @@ Add an entry to `scripts/fork-only-patches.json` with: - `requiredAnchors` — files that must still *contain* given symbols, for patches that modify shared files rather than adding new ones (these are the ones a rebuild silently reverts, since the file still exists) +- `order` - call-site strings that must appear in sequence, for patches whose + meaning is positional. Bare substrings are satisfied by an import, an + interface field or a comment; anchor on the call site instead, so a gate + moved after the step it must precede is caught. diff --git a/package.json b/package.json index 445cd14fa1..80ee0fb769 100644 --- a/package.json +++ b/package.json @@ -72,7 +72,7 @@ "check:mcp-surface-governance": "pnpm --filter @cat-cafe/mcp-server governance:check", "check:no-codex-provider-exit-suppression": "node --test --test-concurrency=1 scripts/check-no-codex-provider-exit-suppression-literal.test.mjs scripts/check-no-codex-provider-exit-suppression-ast.test.mjs scripts/check-no-codex-provider-exit-suppression-isolation.test.mjs scripts/check-no-codex-provider-exit-suppression-structural.test.mjs scripts/check-no-provider-canary-residue.test.mjs && node scripts/check-no-codex-provider-exit-suppression.mjs && node scripts/check-no-provider-canary-residue.mjs", "check:no-provider-canary-residue": "node scripts/check-no-provider-canary-residue.mjs", - "check:fork-only-patches": "node scripts/check-fork-only-patches.mjs", + "check:fork-only-patches": "node --test scripts/check-fork-only-patches.test.mjs && node scripts/check-fork-only-patches.mjs", "check:paw-feel-disposition-boundaries": "node --test scripts/check-paw-feel-disposition-boundaries.test.mjs && node scripts/check-paw-feel-disposition-boundaries.mjs", "check:paw-feel-disposition-capacity": "pnpm --filter @cat-cafe/shared build && pnpm --filter @cat-cafe/api build && node --test packages/api/test/harness-eval/paw-feel-disposition-capacity.test.js", "check:f212-reconciliation-eval": "node --test scripts/eval-f212-reconciliation/reconciliation-baseline.test.mjs scripts/eval-f212-reconciliation/reconciliation-window.test.mjs scripts/eval-f212-reconciliation/reconciliation-matcher.test.mjs scripts/eval-f212-reconciliation/reconciliation-sequence.test.mjs scripts/eval-f212-reconciliation/reconciliation-malformed.test.mjs scripts/eval-f212-reconciliation/reconciliation-compression.test.mjs", diff --git a/packages/api/src/index.ts b/packages/api/src/index.ts index eee339fd7b..91d57b6b58 100644 --- a/packages/api/src/index.ts +++ b/packages/api/src/index.ts @@ -7488,21 +7488,13 @@ async function main(): Promise { }; // F192 evidence-source prerequisite gate. OTel init state is fixed for the // process lifetime (salt is read at boot), so a boolean thunk is a complete input. - // `evidenceTargetIsLocal` asserts the single-process assumption the local handle - // relies on: EVAL_BASE_URL unset (or pointing at our own port) => co-located. + // No co-location check: f167-runtime-eval evidence is a snapshot+attribution pair + // on disk under harnessFeedbackRoot (below), written by THIS process's OTel + // pipeline. The producer is this process by construction, so the local handle is + // authoritative. See eval-domain-evidence-gate.ts for why the earlier + // EVAL_BASE_URL-derived assertion was removed rather than repaired. const evidencePrereqProbe = createTelemetryEvidencePrereqProbe({ otelEnabled: () => telemetryHandle.getMetricsText !== null, - evidenceTargetIsLocal: () => { - const target = process.env.EVAL_BASE_URL; - if (!target) return true; // default target is this process - try { - const { port, hostname } = new URL(target); - const localHost = hostname === 'localhost' || hostname === '127.0.0.1' || hostname === '::1'; - return localHost && port === String(process.env.API_SERVER_PORT ?? ''); - } catch { - return false; // unparseable target => cannot prove co-location => fail closed - } - }, }); const evalScheduleOpts = { diff --git a/packages/api/src/infrastructure/harness-eval/domain/eval-domain-evidence-gate.ts b/packages/api/src/infrastructure/harness-eval/domain/eval-domain-evidence-gate.ts index c23d1fb894..8d46560b5a 100644 --- a/packages/api/src/infrastructure/harness-eval/domain/eval-domain-evidence-gate.ts +++ b/packages/api/src/infrastructure/harness-eval/domain/eval-domain-evidence-gate.ts @@ -38,6 +38,33 @@ export type EvidencePrereqProbe = (domain: EvidenceGateDomain) => EvidencePrereq */ const TELEMETRY_BACKED_ADAPTERS: ReadonlySet = new Set(['f167-runtime-eval']); +/** + * Why there is no "is the evidence source co-located?" check here. + * + * Upstream review of zts212653/clowder-ai#1352 raised a real-sounding concern: + * `otelEnabled` observes the telemetry handle of the process running the CRON, + * so if evidence came from a DIFFERENT runtime the local handle would prove + * nothing about it, in both directions. + * + * A first fix attempt asserted co-location by parsing `EVAL_BASE_URL`. Review of + * THIS fork's PR (#185) showed that gate was wired to unrelated configuration: + * `EVAL_BASE_URL` is read only by the manual CLI `scripts/run-f168-external-case-eval.mjs` + * and is never consulted on the scheduled path. It also mis-compared ports and + * could never match `::1` (Node's URL keeps the brackets: `"[::1]"`), so it fell + * closed on exactly the local topology it was meant to admit. + * + * The scheduled path has no remote evidence target to check. `f167-runtime-eval` + * evidence is a snapshot+attribution PAIR ON DISK under the harness-feedback root + * (see publish-verdict/a2a-generator-adapter.ts), written by this process's own + * OTel pipeline. Source resolution is filesystem-relative, not HTTP. The local + * handle is authoritative because the producer IS this process - structurally, + * not accidentally. + * + * If a telemetry-backed adapter ever fetches evidence over the network, the + * decision belongs at THIS boundary: add it to the set above only together with + * a probe that can observe that source. Do not re-derive co-location from env. + */ + export function isTelemetryBackedAdapter(sourceAdapter: string): boolean { return TELEMETRY_BACKED_ADAPTERS.has(sourceAdapter); } @@ -52,34 +79,9 @@ export function createTelemetryEvidencePrereqProbe(opts: { otelEnabled: () => boolean; /** Override the reason text; defaults to the health route's disabledReason derivation. */ disabledReason?: () => string; - /** - * Upstream review finding (zts212653/clowder-ai#1352, comment by 砚砚): - * `otelEnabled` observes the telemetry handle of the process running the CRON, - * but eval evidence is fetched from the adapter target (`EVAL_BASE_URL`, - * default `localhost:`). When those are different runtimes the - * local handle says nothing about the remote source: a telemetry-disabled - * scheduler would skip a healthy remote source, and a telemetry-enabled - * scheduler would admit an unavailable one. - * - * This fork runs scheduler and evidence source in ONE process, so the local - * handle is authoritative here. Rather than leave that accidental, the caller - * asserts it. Returning `false` fails the gate closed with an explicit reason - * instead of letting the local handle silently speak for a remote target. - * - * Omit → assumes co-located (this fork's single-process deployment). - */ - evidenceTargetIsLocal?: () => boolean; }): EvidencePrereqProbe { return (domain) => { if (!isTelemetryBackedAdapter(domain.sourceAdapter)) return { ok: true }; - if (opts.evidenceTargetIsLocal && !opts.evidenceTargetIsLocal()) { - return { - ok: false, - reason: - 'evidence source is a remote runtime (EVAL_BASE_URL points off-process); ' + - 'the scheduler-local OTel handle cannot prove remote evidence availability', - }; - } if (opts.otelEnabled()) return { ok: true }; const reason = opts.disabledReason?.() ?? diff --git a/packages/api/test/harness-eval/eval-domain-evidence-gate.test.js b/packages/api/test/harness-eval/eval-domain-evidence-gate.test.js index baf692eaff..0adb9d4a0a 100644 --- a/packages/api/test/harness-eval/eval-domain-evidence-gate.test.js +++ b/packages/api/test/harness-eval/eval-domain-evidence-gate.test.js @@ -89,46 +89,63 @@ describe('eval-domain evidence-source prereq gate (fork-only; mindfn PR #91/#137 }); /** - * Upstream review finding (zts212653/clowder-ai#1352): `otelEnabled` observes the - * CRON process's telemetry handle, but evidence is fetched from the adapter target - * (EVAL_BASE_URL). When those differ the local handle proves nothing about the - * remote source, in BOTH directions. The gate must fail closed rather than let a - * local handle speak for a remote target. + * PR #185 review (砚砚): the previous "co-location" assertion was wired to + * `EVAL_BASE_URL`, which the scheduled path never reads (only the manual CLI + * scripts/run-f168-external-case-eval.mjs does). It also compared the URL port + * against `API_SERVER_PORT` and tested `hostname === '::1'`, which Node reports + * as `'[::1]'` - so it fell CLOSED on the very local topology it meant to admit. + * + * It was removed, not repaired: f167-runtime-eval evidence is a snapshot pair on + * disk under the harness-feedback root, produced by this process. These tests pin + * that decision so the env-derived check is not reintroduced. */ - describe('evidence target co-location assertion', () => { - it('remote evidence target -> fail closed even when local OTel is healthy', () => { - const probe = createTelemetryEvidencePrereqProbe({ - otelEnabled: () => true, - evidenceTargetIsLocal: () => false, + describe('evidence-source topology contract', () => { + const A2A = { domainId: 'eval:a2a', sourceAdapter: 'f167-runtime-eval' }; + + function withEnv(key, value, fn) { + const prev = process.env[key]; + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + try { + return fn(); + } finally { + if (prev === undefined) delete process.env[key]; + else process.env[key] = prev; + } + } + + it('a remote-looking EVAL_BASE_URL does not change the verdict', () => { + // Regression: the gate must not re-derive evidence topology from env. The + // scheduled evidence path is filesystem-local regardless of this variable. + const probe = createTelemetryEvidencePrereqProbe({ otelEnabled: () => true }); + withEnv('EVAL_BASE_URL', 'http://evidence.example.com:9999', () => { + assert.equal(probe(A2A).ok, true, 'env must not gate the filesystem-local evidence path'); }); - const result = probe({ domainId: 'eval:a2a', sourceAdapter: 'f167-runtime-eval' }); - assert.equal(result.ok, false, 'a healthy LOCAL handle must not admit an unproven REMOTE source'); - assert.ok(result.reason.includes('remote runtime'), `got: ${result.reason}`); }); - it('co-located evidence target + healthy OTel -> ok', () => { - const probe = createTelemetryEvidencePrereqProbe({ - otelEnabled: () => true, - evidenceTargetIsLocal: () => true, - }); - assert.equal(probe({ domainId: 'eval:a2a', sourceAdapter: 'f167-runtime-eval' }).ok, true); + it('local topology is admitted on the documented default port with no env overrides', () => { + // The removed check failed exactly here: "3004" === String(undefined ?? '') + // is false, so a healthy co-located runtime was skipped. + const probe = createTelemetryEvidencePrereqProbe({ otelEnabled: () => true }); + withEnv('EVAL_BASE_URL', 'http://localhost:3004', () => + withEnv('API_SERVER_PORT', undefined, () => { + assert.equal(probe(A2A).ok, true, 'default local target must not fail closed'); + }), + ); }); - it('omitted assertion stays backward-compatible (assumes co-located)', () => { + it('IPv6 loopback is admitted (Node reports hostname as "[::1]", not "::1")', () => { const probe = createTelemetryEvidencePrereqProbe({ otelEnabled: () => true }); - assert.equal(probe({ domainId: 'eval:a2a', sourceAdapter: 'f167-runtime-eval' }).ok, true); + withEnv('EVAL_BASE_URL', 'http://[::1]:3002', () => { + assert.equal(probe(A2A).ok, true); + }); }); - it('co-location is checked before the telemetry handle, not after', () => { - const probe = createTelemetryEvidencePrereqProbe({ - otelEnabled: () => false, - evidenceTargetIsLocal: () => false, + it('OTel state remains the only gate input for a telemetry-backed adapter', () => { + const disabled = createTelemetryEvidencePrereqProbe({ otelEnabled: () => false }); + withEnv('EVAL_BASE_URL', 'http://localhost:3002', () => { + assert.equal(disabled(A2A).ok, false, 'disabled OTel must still fail closed'); }); - const result = probe({ domainId: 'eval:a2a', sourceAdapter: 'f167-runtime-eval' }); - assert.ok( - result.reason.includes('remote runtime'), - `remote-target cause must win over the local salt cause (got: ${result.reason})`, - ); }); }); diff --git a/scripts/check-fork-only-patches.mjs b/scripts/check-fork-only-patches.mjs index a5ef7419e3..a7e44b77cb 100755 --- a/scripts/check-fork-only-patches.mjs +++ b/scripts/check-fork-only-patches.mjs @@ -18,50 +18,74 @@ * registered patch is still present, so a rebuild that drops one turns * `pnpm check` RED immediately instead of failing silently in production. * + * Enforcement points (both required, neither sufficient alone): + * - `pnpm check` locally, wired first so it fails fast. + * - .github/workflows/fork-only-patches.yml on every push/PR to develop_base. + * The repo's other workflows trigger on `main` only, so without this the + * guard would never run automatically on the exact branch that loses the + * patches. That workflow is itself a registered fork-only patch. + * * Fail-CLOSED: a missing/unreadable/malformed registry is a violation, not a * pass. A guard whose own input vanished must not read as clean. * * Truth source: scripts/fork-only-patches.json (see docs/fork-only-patches.md). */ +import { execFileSync } from 'node:child_process'; import { readFileSync } from 'node:fs'; -import { dirname, join } from 'node:path'; +import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; -const __dirname = dirname(fileURLToPath(import.meta.url)); -const REPO_ROOT = join(__dirname, '..'); -const REGISTRY = join(__dirname, 'fork-only-patches.json'); - -const violations = []; +const REGISTRY_REL = 'scripts/fork-only-patches.json'; -function readRegistry() { +function readRegistry(repoRoot, violations) { let raw; try { - raw = readFileSync(REGISTRY, 'utf8'); + raw = readFileSync(join(repoRoot, REGISTRY_REL), 'utf8'); } catch (err) { - violations.push(`registry unreadable at scripts/fork-only-patches.json: ${err.message}`); + violations.push(`registry unreadable at ${REGISTRY_REL}: ${err.message}`); return null; } let parsed; try { parsed = JSON.parse(raw); } catch (err) { - violations.push(`registry is not valid JSON: ${err.message}`); + violations.push('registry is not valid JSON: ' + err.message); return null; } if (!parsed || !Array.isArray(parsed.patches)) { violations.push('registry is missing a top-level "patches" array'); return null; } + if (parsed.patches.length === 0) { + violations.push( + 'registry declares zero patches - an empty registry cannot distinguish "nothing to protect" from "the registry was reverted"', + ); + return null; + } return parsed; } -function checkPatch(patch) { +function checkPatch(repoRoot, patch, violations) { const id = patch.id ?? ''; + // Fail closed on a hollow entry. A patch that asserts nothing passes + // vacuously, which is indistinguishable from a rebuild having emptied it - + // the same silent-green failure this guard exists to remove. + if (!patch || typeof patch !== 'object' || typeof patch.id !== 'string' || patch.id.length === 0) { + violations.push('registry contains a patch entry without a string "id"'); + return; + } + const fileCount = (patch.requiredFiles ?? []).length; + const anchorCount = (patch.requiredAnchors ?? []).length; + if (fileCount === 0 && anchorCount === 0) { + violations.push(`[${id}] asserts nothing: needs requiredFiles and/or requiredAnchors`); + return; + } + for (const rel of patch.requiredFiles ?? []) { try { - readFileSync(join(REPO_ROOT, rel), 'utf8'); + readFileSync(join(repoRoot, rel), 'utf8'); } catch { violations.push(`[${id}] required file missing: ${rel}`); } @@ -70,35 +94,194 @@ function checkPatch(patch) { for (const anchor of patch.requiredAnchors ?? []) { let content; try { - content = readFileSync(join(REPO_ROOT, anchor.file), 'utf8'); + content = readFileSync(join(repoRoot, anchor.file), 'utf8'); } catch { violations.push(`[${id}] anchor file missing: ${anchor.file}`); continue; } + if ((anchor.mustContain ?? []).length === 0 && (anchor.order ?? []).length === 0) { + violations.push(`[${id}] anchor for ${anchor.file} asserts nothing`); + continue; + } for (const needle of anchor.mustContain ?? []) { if (!content.includes(needle)) { const note = anchor.note ? ` (${anchor.note})` : ''; violations.push(`[${id}] ${anchor.file} lost anchor ${JSON.stringify(needle)}${note}`); } } + // Ordering anchors: substring presence cannot tell an executed gate from a + // stray import or a comment. Where sequence carries the meaning, pin it. + const order = anchor.order ?? []; + for (let i = 0; i + 1 < order.length; i++) { + const first = content.indexOf(order[i]); + const second = content.indexOf(order[i + 1]); + if (first === -1 || second === -1) { + violations.push( + `[${id}] ${anchor.file} is missing an ordered anchor: ${JSON.stringify(order[i])} -> ${JSON.stringify(order[i + 1])}`, + ); + } else if (first > second) { + violations.push( + `[${id}] ${anchor.file} has ${JSON.stringify(order[i])} AFTER ${JSON.stringify(order[i + 1])}; the gate must run first or it cannot prevent the invocation`, + ); + } + } + } +} + +/** + * @param {string} repoRoot absolute path to the repository root + * @returns {{ violations: string[], patchIds: string[] }} + */ +export function checkForkOnlyPatches(repoRoot) { + const violations = []; + const registry = readRegistry(repoRoot, violations); + if (registry) { + for (const patch of registry.patches) checkPatch(repoRoot, patch, violations); + } + return { violations, patchIds: (registry?.patches ?? []).map((p) => p.id) }; +} + +/** + * Did the rebuild that just happened drop anything? + * + * The tree check above answers "is the patch here right now" - but it reads the + * registry FROM THE TREE. A rebuild that resets the whole tree removes the + * registry and this very script along with the patches, so the guard cannot + * report on its own erasure. That is the gap PR #185 review (砚砚 P1) named: + * the registry is only useful if it survives independently. + * + * What survives `git reset --hard upstream/main` is the REMOTE-TRACKING REF. + * `origin/develop_base` still points at the pre-rebuild tip, so the previous + * tree's registry is readable straight out of git even after the working tree + * has been replaced: + * + * git show origin/develop_base:scripts/fork-only-patches.json + * + * This check reads what the PREVIOUS tree claimed to protect and verifies it + * against the CURRENT one. Run it as the last step of a rebuild, before the + * result is pushed or deployed. It stays runnable even when the rebuild deleted + * the guard, because the guard can be read from the same ref: + * + * git show origin/develop_base:scripts/check-fork-only-patches.mjs > /tmp/guard.mjs + * + * Why not assert membership of a "carrier" branch instead: `fork/optimizations` + * is an orphan snapshot (7 commits, no merge-base with upstream/main, a July + * full-tree copy). Membership of it is not a survival predicate, and demanding + * that current code be committed onto a stale unrelated history would be + * incoherent. Comparing against the previous tip is the claim that actually holds. + */ +function gitOut(repoRoot, args) { + return execFileSync('git', args, { + cwd: repoRoot, + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + }); +} + +function resolveRef(repoRoot, ref) { + try { + gitOut(repoRoot, ['rev-parse', '--verify', '--quiet', `${ref}^{commit}`]); + return true; + } catch { + return false; } } -const registry = readRegistry(); -if (registry) { - for (const patch of registry.patches) checkPatch(patch); +/** + * @param {string} repoRoot + * @param {{ baselineRef?: string }} [options] + * @returns {{ violations: string[], baselineRef: string | null, comparedPatches: number }} + */ +export function checkRebuildSurvival(repoRoot, options = {}) { + const violations = []; + // Read the tree registry into a throwaway sink: its problems are the tree + // check's to report. Here it is only the source of the baseline ref, plus the + // signal that the guard's own input was erased. + const treeRegistry = readRegistry(repoRoot, []); + const baselineRef = options.baselineRef ?? treeRegistry?.baselineRef ?? 'origin/develop_base'; + + if (!resolveRef(repoRoot, baselineRef)) { + violations.push( + `baseline ref "${baselineRef}" is not present in this checkout, so rebuild loss cannot be detected. ` + + `Fetch it first: git fetch origin develop_base`, + ); + return { violations, baselineRef: null, comparedPatches: 0 }; + } + + let baselineRaw; + try { + baselineRaw = gitOut(repoRoot, ['show', `${baselineRef}:${REGISTRY_REL}`]); + } catch { + // The previous tree made no claim, so there is nothing it could have lost. + // This is a real absence of a prior claim, not an unverifiable one - the + // fail-closed rule applies to the latter. + return { violations, baselineRef, comparedPatches: 0 }; + } + + let baseline; + try { + baseline = JSON.parse(baselineRaw); + } catch (err) { + violations.push(`baseline registry at ${baselineRef} is not valid JSON: ${err.message}`); + return { violations, baselineRef, comparedPatches: 0 }; + } + if (!baseline || !Array.isArray(baseline.patches)) { + violations.push(`baseline registry at ${baselineRef} has no "patches" array`); + return { violations, baselineRef, comparedPatches: 0 }; + } + + const dropped = []; + for (const patch of baseline.patches) { + const id = patch?.id ?? ''; + const before = violations.length; + checkPatch(repoRoot, patch, violations); + // Re-label: these are losses relative to the previous tree, which is a + // sharper statement than "missing" - it names a regression with a culprit. + for (let i = before; i < violations.length; i++) { + violations[i] = `REBUILD LOSS vs ${baselineRef} - ${violations[i]}`; + } + if (violations.length > before) dropped.push(id); + } + + // The guard's own erasure is the case that hid the first three losses. + if (!treeRegistry) { + violations.push( + `REBUILD LOSS vs ${baselineRef} - the fork-only patch registry itself is gone from this tree; ` + + `restore ${REGISTRY_REL} and scripts/check-fork-only-patches.mjs`, + ); + } + + return { violations, baselineRef, comparedPatches: baseline.patches.length }; } -if (violations.length > 0) { - console.error('[check-fork-only-patches] FAIL - fork-only patch(es) missing from this tree.\n'); - for (const v of violations) console.error(` - ${v}`); - const ids = (registry?.patches ?? []).map((p) => p.id).join(', '); - console.error('\nThis almost always means a develop_base rebuild onto upstream main dropped them.'); - console.error('Re-apply the listed patches, then re-run this check.'); - console.error(`Registered fork-only patches: ${ids || '(registry unavailable)'}`); - console.error('Context: docs/fork-only-patches.md'); - process.exit(1); +function main() { + const repoRoot = join(dirname(fileURLToPath(import.meta.url)), '..'); + const tree = checkForkOnlyPatches(repoRoot); + const survival = checkRebuildSurvival(repoRoot); + const seen = new Set(); + const violations = [...tree.violations, ...survival.violations].filter((v) => { + if (seen.has(v)) return false; + seen.add(v); + return true; + }); + const patchIds = tree.patchIds; + + if (violations.length > 0) { + console.error('[check-fork-only-patches] FAIL - fork-only patch(es) missing from this tree.\n'); + for (const v of violations) console.error(` - ${v}`); + console.error('\nThis almost always means a develop_base rebuild onto upstream main dropped them.'); + console.error('Re-apply the listed patches, then re-run this check.'); + console.error(`Registered fork-only patches: ${patchIds.join(', ') || '(registry unavailable)'}`); + console.error('Context: docs/fork-only-patches.md'); + process.exit(1); + } + + const baseline = survival.baselineRef + ? `; ${survival.comparedPatches} claimed by ${survival.baselineRef} still present` + : ''; + console.log(`[check-fork-only-patches] OK - ${patchIds.length} fork-only patch(es) present${baseline}`); } -const count = registry.patches.length; -console.log(`[check-fork-only-patches] OK - ${count} fork-only patch(es) present`); +if (process.argv[1] && resolve(process.argv[1]) === resolve(fileURLToPath(import.meta.url))) { + main(); +} diff --git a/scripts/check-fork-only-patches.test.mjs b/scripts/check-fork-only-patches.test.mjs new file mode 100644 index 0000000000..d01951c05d --- /dev/null +++ b/scripts/check-fork-only-patches.test.mjs @@ -0,0 +1,317 @@ +/** + * Tests for the fork-only patch guard. + * + * The guard's whole claim is that it turns a silent rebuild loss into a red + * check. An untested guard cannot support that claim - it would be the same + * "trust me, I verified it once" the guard exists to replace. These tests pin + * the two detection modes that matter and the fail-closed behaviour: + * + * - required file deleted (patch added a new file, rebuild dropped it) + * - anchor lost from a shared file (file survives the rebuild, patch reverted) + * - registry missing/malformed/empty (the guard's own input vanished) + * + * The last block asserts against the REAL repo: the guard's automatic + * enforcement point (its develop_base workflow) must stay registered, or the + * guard silently degrades to "someone remembers to run pnpm check". + */ + +import assert from 'node:assert/strict'; +import { execFileSync } from 'node:child_process'; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import test from 'node:test'; +import { fileURLToPath } from 'node:url'; + +import { checkForkOnlyPatches, checkRebuildSurvival } from './check-fork-only-patches.mjs'; + +const REPO_ROOT = join(dirname(fileURLToPath(import.meta.url)), '..'); + +const REGISTRY = { + patches: [ + { + id: 'demo-patch', + requiredFiles: ['src/added-by-fork.ts'], + requiredAnchors: [ + { + file: 'src/shared.ts', + mustContain: ['forkOnlySymbol'], + note: 'wiring a rebuild reverts silently', + }, + ], + }, + ], +}; + +function fixture(t, { registry = REGISTRY, files = {} } = {}) { + const root = mkdtempSync(join(tmpdir(), 'fork-only-patches-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + + mkdirSync(join(root, 'scripts'), { recursive: true }); + mkdirSync(join(root, 'src'), { recursive: true }); + + if (registry !== null) { + writeFileSync( + join(root, 'scripts/fork-only-patches.json'), + typeof registry === 'string' ? registry : JSON.stringify(registry, null, 2), + ); + } + + const tree = { + 'src/added-by-fork.ts': 'export const addedByFork = true;\n', + 'src/shared.ts': 'export const forkOnlySymbol = 1;\n', + ...files, + }; + for (const [rel, content] of Object.entries(tree)) { + if (content === null) continue; + writeFileSync(join(root, rel), content); + } + return root; +} + +test('intact tree reports no violations', (t) => { + const { violations, patchIds } = checkForkOnlyPatches(fixture(t)); + assert.deepEqual(violations, []); + assert.deepEqual(patchIds, ['demo-patch']); +}); + +test('detects a required file dropped by a rebuild', (t) => { + const root = fixture(t, { files: { 'src/added-by-fork.ts': null } }); + const { violations } = checkForkOnlyPatches(root); + assert.equal(violations.length, 1); + assert.match(violations[0], /required file missing: src\/added-by-fork\.ts/); +}); + +test('detects a lost anchor when the shared file itself survives', (t) => { + // The silent case: existence checks read clean because the file is still + // there; only the fork's edit inside it was reverted. + const root = fixture(t, { + files: { 'src/shared.ts': 'export const somethingElse = 1;\n' }, + }); + const { violations } = checkForkOnlyPatches(root); + assert.equal(violations.length, 1); + assert.match(violations[0], /lost anchor "forkOnlySymbol"/); + assert.match(violations[0], /wiring a rebuild reverts silently/); +}); + +test('fails closed when the registry is missing', (t) => { + const { violations } = checkForkOnlyPatches(fixture(t, { registry: null })); + assert.equal(violations.length, 1); + assert.match(violations[0], /registry unreadable/); +}); + +test('fails closed when the registry is malformed', (t) => { + const { violations } = checkForkOnlyPatches(fixture(t, { registry: '{ not json' })); + assert.equal(violations.length, 1); + assert.match(violations[0], /not valid JSON/); +}); + +test('fails closed when the registry declares zero patches', (t) => { + // An emptied registry must not read as "nothing to protect" - that is + // indistinguishable from the registry itself being reverted. + const { violations } = checkForkOnlyPatches(fixture(t, { registry: { patches: [] } })); + assert.equal(violations.length, 1); + assert.match(violations[0], /zero patches/); +}); + +test('the guard is green against the real repo tree', () => { + const { violations } = checkForkOnlyPatches(REPO_ROOT); + assert.deepEqual(violations, [], `real tree has fork-only patch violations:\n${violations.join('\n')}`); +}); + +test('the guard keeps its own automatic enforcement point on develop_base', () => { + // Regression guard for the gap this test file shipped with: every other + // workflow triggers on `main` only, so a guard without this workflow never + // runs automatically on the branch that actually loses the patches. + const workflowRel = '.github/workflows/fork-only-patches.yml'; + const workflow = readFileSync(join(REPO_ROOT, workflowRel), 'utf8'); + // Strip comments: the file explains *why* paths-ignore is absent, and that + // prose must not satisfy - or trip - a check about the directive itself. + const directives = workflow + .split('\n') + .filter((line) => !line.trim().startsWith('#')) + .join('\n'); + + assert.match(directives, /push:\s*\n\s*branches: \[develop_base\]/); + assert.match(directives, /pull_request:\s*\n\s*branches: \[develop_base\]/); + assert.doesNotMatch( + directives, + /paths-ignore/, + 'a rebuild can drop a patch regardless of which paths its diff touches', + ); + + const registry = JSON.parse(readFileSync(join(REPO_ROOT, 'scripts/fork-only-patches.json'), 'utf8')); + const registeredFiles = registry.patches.flatMap((p) => p.requiredFiles ?? []); + assert.ok( + registeredFiles.includes(workflowRel), + `${workflowRel} must be a registered fork-only patch, or a rebuild drops the guard's CI trigger unnoticed`, + ); +}); + +// --------------------------------------------------------------------------- +// Rebuild fixture (PR #185 review, 砚砚 P1): a tree check reads the registry +// FROM the tree, so a rebuild that wipes the tree erases the guard together +// with the patches and nothing reports. These tests reproduce that exact +// rebuild and prove the loss is detected from the surviving remote-tracking ref. +// --------------------------------------------------------------------------- + +const BASELINE = 'refs/remotes/origin/develop_base'; + +function git(root, ...args) { + execFileSync('git', args, { cwd: root, stdio: ['ignore', 'pipe', 'pipe'] }); +} + +function gitCapture(root, ...args) { + return execFileSync('git', args, { cwd: root, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); +} + +const BASELINE_REGISTRY = { + baselineRef: BASELINE, + patches: [ + { + id: 'demo-patch', + requiredFiles: ['src/added-by-fork.ts'], + requiredAnchors: [{ file: 'src/shared.ts', mustContain: ['forkOnlySymbol'] }], + }, + ], +}; + +/** Builds a repo whose pre-rebuild tip is recorded as origin/develop_base. */ +function rebuiltRepo(t, { rebuild }) { + const root = mkdtempSync(join(tmpdir(), 'fork-only-rebuild-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + mkdirSync(join(root, 'scripts'), { recursive: true }); + mkdirSync(join(root, 'src'), { recursive: true }); + + git(root, 'init', '--initial-branch=develop_base'); + git(root, 'config', 'user.email', 'guard@test.local'); + git(root, 'config', 'user.name', 'guard'); + + // Pre-rebuild develop_base: patch present, registry present, guard green. + writeFileSync(join(root, 'scripts/fork-only-patches.json'), JSON.stringify(BASELINE_REGISTRY, null, 2)); + writeFileSync(join(root, 'src/added-by-fork.ts'), 'export const addedByFork = true;\n'); + writeFileSync(join(root, 'src/shared.ts'), 'export const forkOnlySymbol = 1;\n'); + git(root, 'add', '-A'); + git(root, 'commit', '-m', 'pre-rebuild develop_base'); + + // The ref that survives `git reset --hard upstream/main`. + git(root, 'update-ref', BASELINE, gitCapture(root, 'rev-parse', 'HEAD').trim()); + + rebuild(root); + return root; +} + +test('a rebuild that erases the registry AND the patch is still detected', (t) => { + const root = rebuiltRepo(t, { + rebuild: (r) => { + // Upstream main has none of it - registry, guard and patch all gone. + rmSync(join(r, 'scripts/fork-only-patches.json')); + rmSync(join(r, 'src/added-by-fork.ts')); + writeFileSync(join(r, 'src/shared.ts'), 'export const upstreamOnly = 1;\n'); + }, + }); + + // The tree check alone can only say "my own input vanished". + const tree = checkForkOnlyPatches(root); + assert.ok(tree.violations.every((v) => v.includes('registry unreadable'))); + + const { violations } = checkRebuildSurvival(root, { baselineRef: BASELINE }); + assert.ok( + violations.some((v) => v.includes('REBUILD LOSS') && v.includes('src/added-by-fork.ts')), + `expected the dropped file to be named, got: ${violations.join(' | ')}`, + ); + assert.ok( + violations.some((v) => v.includes('src/shared.ts')), + 'a reverted anchor in a surviving shared file must be reported too', + ); + assert.ok( + violations.some((v) => v.includes('registry itself is gone')), + 'the guard must report its own erasure, which is what hid the first three losses', + ); +}); + +test('a rebuild that preserved everything reports no loss', (t) => { + const root = rebuiltRepo(t, { rebuild: () => {} }); + const { violations, comparedPatches } = checkRebuildSurvival(root, { baselineRef: BASELINE }); + assert.deepEqual(violations, []); + assert.equal(comparedPatches, 1); +}); + +test('an unreachable baseline fails closed with an actionable recovery', (t) => { + const root = rebuiltRepo(t, { rebuild: () => {} }); + const { violations } = checkRebuildSurvival(root, { baselineRef: 'refs/remotes/origin/nope' }); + assert.equal(violations.length, 1); + assert.match(violations[0], /cannot be detected/); + assert.match(violations[0], /git fetch origin develop_base/); +}); + +test('a baseline that never carried a registry makes no claim to lose', (t) => { + const root = rebuiltRepo(t, { rebuild: () => {} }); + // Point at a commit with no registry: absence of a prior claim is not an + // unverifiable claim, so it must not be dressed up as a violation. + git(root, 'update-ref', 'refs/remotes/origin/empty', gitCapture(root, 'rev-parse', 'HEAD').trim()); + rmSync(join(root, 'scripts/fork-only-patches.json')); + git(root, 'add', '-A'); + git(root, 'commit', '-m', 'drop registry'); + git(root, 'update-ref', 'refs/remotes/origin/empty', gitCapture(root, 'rev-parse', 'HEAD').trim()); + const { violations, comparedPatches } = checkRebuildSurvival(root, { + baselineRef: 'refs/remotes/origin/empty', + }); + assert.deepEqual(violations, []); + assert.equal(comparedPatches, 0); +}); + +// --- hollow entries (PR #185 review, 砚砚 P2) ------------------------------- + +test('a patch entry that asserts nothing fails closed', (t) => { + const root = fixture(t, { registry: { baselineRef: BASELINE, patches: [{ id: 'hollow' }] } }); + const { violations } = checkForkOnlyPatches(root); + assert.equal(violations.length, 1); + assert.match(violations[0], /asserts nothing/); +}); + +test('a patch entry without an id fails closed', (t) => { + const root = fixture(t, { + registry: { baselineRef: BASELINE, patches: [{ requiredFiles: ['src/added-by-fork.ts'] }] }, + }); + const { violations } = checkForkOnlyPatches(root); + assert.equal(violations.length, 1); + assert.match(violations[0], /without a string "id"/); +}); + +// --- ordering anchors (PR #185 review, 砚砚 P2) ----------------------------- + +test('ordering anchor catches a gate moved after the step it must precede', (t) => { + const root = fixture(t, { + registry: { + baselineRef: BASELINE, + patches: [ + { + id: 'ordered', + requiredAnchors: [{ file: 'src/shared.ts', order: ['gateRuns()', 'thenPublishes()'] }], + }, + ], + }, + files: { 'src/shared.ts': 'thenPublishes();\ngateRuns();\n' }, + }); + const { violations } = checkForkOnlyPatches(root); + assert.equal(violations.length, 1); + assert.match(violations[0], /AFTER/); + assert.match(violations[0], /cannot prevent the invocation/); +}); + +test('ordering anchor passes when the gate runs first', (t) => { + const root = fixture(t, { + registry: { + baselineRef: BASELINE, + patches: [ + { + id: 'ordered', + requiredAnchors: [{ file: 'src/shared.ts', order: ['gateRuns()', 'thenPublishes()'] }], + }, + ], + }, + files: { 'src/shared.ts': 'gateRuns();\nthenPublishes();\n' }, + }); + assert.deepEqual(checkForkOnlyPatches(root).violations, []); +}); diff --git a/scripts/fork-only-patches.json b/scripts/fork-only-patches.json index 84315ed665..56b31c9030 100644 --- a/scripts/fork-only-patches.json +++ b/scripts/fork-only-patches.json @@ -1,5 +1,7 @@ { "$comment": "Single source of truth for fork-only patches that upstream main does NOT carry. Consumed by scripts/check-fork-only-patches.mjs (wired into `pnpm check`). See docs/fork-only-patches.md. Every develop_base rebuild from upstream main MUST re-apply these; the guard turns a silent loss into a red check.", + "baselineRef": "origin/develop_base", + "$baselineComment": "The pre-rebuild tip, read straight out of git. `git reset --hard upstream/main` replaces the working tree but does NOT move remote-tracking refs, so this ref still holds the previous tree's registry - and this guard - after a rebuild has erased both from disk. Run the guard as the last step of a rebuild to compare what the previous tree claimed to protect against what the new one actually has. NOT fork/optimizations: that branch is an orphan July snapshot (7 commits, no merge-base with upstream/main), so membership of it is not a survival predicate.", "patches": [ { "id": "f192-evidence-prereq-gate", @@ -15,17 +17,42 @@ { "file": "packages/api/src/infrastructure/harness-eval/domain/eval-domain-daily.ts", "mustContain": ["evidencePrereqProbe", "evaluateEvidencePrereq", "buildEvidencePrereqSkippedMessage"], - "note": "opts interface field + execute-time gate, ordered BEFORE publishPrereqProbe" + "note": "execute-path ordering: the evidence gate must run BEFORE publishPrereqProbe, or it cannot prevent the invocation it exists to prevent. Anchored on CALL SITES, not bare symbols - imports and interface fields would satisfy a substring check while the executed gate was gone.", + "order": ["evaluateEvidencePrereq(config.evidencePrereqProbe", "if (config.publishPrereqProbe)"] }, { "file": "packages/api/src/infrastructure/harness-eval/domain/eval-domain-nday.ts", "mustContain": ["evidencePrereqProbe", "evaluateEvidencePrereq", "buildEvidencePrereqSkippedMessage"], - "note": "N-day parity; must return before the Redis last-dispatch write" + "note": "execute-path ordering: the evidence gate must run BEFORE publishPrereqProbe, or it cannot prevent the invocation it exists to prevent. Anchored on CALL SITES, not bare symbols - imports and interface fields would satisfy a substring check while the executed gate was gone.", + "order": ["evaluateEvidencePrereq(opts.evidencePrereqProbe", "if (opts.publishPrereqProbe)"] }, { "file": "packages/api/src/index.ts", - "mustContain": ["createTelemetryEvidencePrereqProbe", "evidencePrereqProbe,", "evidenceTargetIsLocal"], - "note": "bootstrap: import, probe construction from telemetryHandle, inclusion in evalScheduleOpts" + "mustContain": ["createTelemetryEvidencePrereqProbe", "evidencePrereqProbe,"], + "note": "bootstrap: import + probe construction from telemetryHandle + inclusion in evalScheduleOpts" + } + ] + }, + { + "id": "fork-only-patch-guard", + "owner": "opus", + "why": "The guard that detects every other entry's loss. Its develop_base workflow is the only automatic enforcement point: all other workflows in this repo trigger on `main`, and develop_base is the branch rebuilt onto upstream main. Lose this and the guard degrades to 'someone remembers to run pnpm check' - the exact failure mode it replaces.", + "upstreamStatus": "fork-scope by construction - upstream has no develop_base branch and carries none of these patches", + "requiredFiles": [ + ".github/workflows/fork-only-patches.yml", + "scripts/check-fork-only-patches.mjs", + "scripts/check-fork-only-patches.test.mjs" + ], + "requiredAnchors": [ + { + "file": ".github/workflows/fork-only-patches.yml", + "mustContain": ["branches: [develop_base]"], + "note": "the trigger that makes the guard automatic; a workflow that survives with a main-only trigger is silently inert" + }, + { + "file": "package.json", + "mustContain": ["pnpm check:fork-only-patches", "node scripts/check-fork-only-patches.mjs"], + "note": "local enforcement: wired FIRST in `check` so a rebuild fails fast; definition + chain wiring are separate revert cases" } ] } From 959ee98ced07dfdbc8889169b5027c7dc4ca8c3e Mon Sep 17 00:00:00 2001 From: Ragdoll-Opus-5 Date: Tue, 22 Sep 2026 11:38:49 +0800 Subject: [PATCH 3/5] fix(F192): stop the guard from silently passing when run from a symlinked path Why: testing the recovery procedure I had just documented showed it did not work, and failed in the worst possible way. `docs/fork-only-patches.md` tells you to rescue the guard out of the surviving ref after a rebuild deleted it: git show origin/develop_base:scripts/check-fork-only-patches.mjs > /tmp/guard.mjs node /tmp/guard.mjs That printed nothing and exited 0. Two defects, both of which make the guard report clean while checking nothing: 1. Entrypoint detection compared `process.argv[1]` with `fileURLToPath(import.meta.url)` via resolve(). The first is the path as typed, the second is the REAL path, and resolve() does not follow symlinks. On macOS `/tmp` -> `/private/tmp`, so the comparison was false and main() never ran. Any invocation through a symlinked path silently no-ops. Now compared by realpath. 2. The repo root was script-relative, so a rescued copy in /tmp resolved the root to `/` and would have found no registry. The CLI now asks git for the toplevel from the working directory, with an explicit `--repo-root` override and the old script-relative path as last resort. A guard that exits 0 having checked nothing is exactly the failure this file exists to remove - the same shape as the three silent losses it is meant to catch - so all three paths are pinned by tests: invocation through a symlink, a rescued out-of-tree copy, and `--repo-root`. `--repo-root` also must not become a way to opt out of the survival half, so an explicit tree with no resolvable baseline still fails closed. Verified: 61/61 tests, biome clean repo-wide. Co-Authored-By: Claude Opus 5 --- docs/fork-only-patches.md | 8 ++- scripts/check-fork-only-patches.mjs | 52 ++++++++++++++++- scripts/check-fork-only-patches.test.mjs | 74 +++++++++++++++++++++++- 3 files changed, 129 insertions(+), 5 deletions(-) diff --git a/docs/fork-only-patches.md b/docs/fork-only-patches.md index b00f6a7e7c..17d82b755b 100644 --- a/docs/fork-only-patches.md +++ b/docs/fork-only-patches.md @@ -92,10 +92,16 @@ the new tree **actually has**. It stays runnable even when the rebuild deleted it, because it can be read from the same ref: ```bash +cd # the guard asks git where the repo is, from CWD git show origin/develop_base:scripts/check-fork-only-patches.mjs > /tmp/guard.mjs -node /tmp/guard.mjs +node /tmp/guard.mjs # or: node /tmp/guard.mjs --repo-root ``` +The rescued copy resolves the repo from the working directory, not from where +the file happens to sit, and it detects direct invocation by **real** path - +running it from a symlinked location (macOS `/tmp`) must not make it exit 0 +having checked nothing. + ## Two checks, both required ```bash diff --git a/scripts/check-fork-only-patches.mjs b/scripts/check-fork-only-patches.mjs index a7e44b77cb..2a64a5c125 100755 --- a/scripts/check-fork-only-patches.mjs +++ b/scripts/check-fork-only-patches.mjs @@ -32,7 +32,7 @@ */ import { execFileSync } from 'node:child_process'; -import { readFileSync } from 'node:fs'; +import { readFileSync, realpathSync } from 'node:fs'; import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -254,8 +254,33 @@ export function checkRebuildSurvival(repoRoot, options = {}) { return { violations, baselineRef, comparedPatches: baseline.patches.length }; } +/** + * Where is the repo? + * + * Not "next to this script". The documented recovery for a rebuild that deleted + * the guard is to read it back out of the surviving ref and run it: + * + * git show origin/develop_base:scripts/check-fork-only-patches.mjs > /tmp/guard.mjs + * node /tmp/guard.mjs + * + * A script-relative root resolves to `/` there and checks nothing. Ask git from + * the working directory first, so the guard works from wherever it was rescued to. + */ +function resolveRepoRoot(argv) { + const flag = argv.indexOf('--repo-root'); + if (flag !== -1 && argv[flag + 1]) return resolve(argv[flag + 1]); + try { + return execFileSync('git', ['rev-parse', '--show-toplevel'], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + }).trim(); + } catch { + return join(dirname(fileURLToPath(import.meta.url)), '..'); + } +} + function main() { - const repoRoot = join(dirname(fileURLToPath(import.meta.url)), '..'); + const repoRoot = resolveRepoRoot(process.argv.slice(2)); const tree = checkForkOnlyPatches(repoRoot); const survival = checkRebuildSurvival(repoRoot); const seen = new Set(); @@ -282,6 +307,27 @@ function main() { console.log(`[check-fork-only-patches] OK - ${patchIds.length} fork-only patch(es) present${baseline}`); } -if (process.argv[1] && resolve(process.argv[1]) === resolve(fileURLToPath(import.meta.url))) { +/** + * Symlink-safe entrypoint detection. + * + * `fileURLToPath(import.meta.url)` is the REAL path; `process.argv[1]` is the + * path as typed. On macOS `/tmp` is a symlink to `/private/tmp`, so comparing + * them with resolve() alone returns false and main() never runs - the guard + * exits 0 having checked nothing. A guard that can silently no-op is the exact + * failure mode this file exists to remove, so compare real paths. + */ +function isDirectInvocation() { + if (!process.argv[1]) return false; + const real = (p) => { + try { + return realpathSync(p); + } catch { + return resolve(p); + } + }; + return real(process.argv[1]) === real(fileURLToPath(import.meta.url)); +} + +if (isDirectInvocation()) { main(); } diff --git a/scripts/check-fork-only-patches.test.mjs b/scripts/check-fork-only-patches.test.mjs index d01951c05d..1e659854e3 100644 --- a/scripts/check-fork-only-patches.test.mjs +++ b/scripts/check-fork-only-patches.test.mjs @@ -17,7 +17,16 @@ import assert from 'node:assert/strict'; import { execFileSync } from 'node:child_process'; -import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { + copyFileSync, + mkdirSync as mkdirp, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; import test from 'node:test'; @@ -315,3 +324,66 @@ test('ordering anchor passes when the gate runs first', (t) => { }); assert.deepEqual(checkForkOnlyPatches(root).violations, []); }); + +// --------------------------------------------------------------------------- +// The guard must never silently no-op. +// +// Found while testing the documented recovery from docs/fork-only-patches.md: +// `node /tmp/guard.mjs` printed nothing and exited 0. `import.meta.url` gives +// the REAL path (/private/tmp/...) while `process.argv[1]` gives the path as +// typed (/tmp/...), so the entrypoint comparison was false and main() never +// ran. A guard that exits 0 having checked nothing is the precise failure this +// file exists to remove, so it is pinned here. +// --------------------------------------------------------------------------- + +const GUARD = join(REPO_ROOT, 'scripts/check-fork-only-patches.mjs'); + +function runGuard(args, cwd) { + try { + const stdout = execFileSync('node', args, { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); + return { code: 0, out: stdout }; + } catch (err) { + return { code: err.status ?? 1, out: `${err.stdout ?? ''}${err.stderr ?? ''}` }; + } +} + +test('runs when invoked through a symlinked path instead of exiting 0 in silence', (t) => { + const box = mkdtempSync(join(tmpdir(), 'guard-symlink-')); + t.after(() => rmSync(box, { recursive: true, force: true })); + mkdirp(join(box, 'real')); + copyFileSync(GUARD, join(box, 'real/guard.mjs')); + symlinkSync(join(box, 'real'), join(box, 'link')); + + const { out } = runGuard([join(box, 'link/guard.mjs')], REPO_ROOT); + assert.match(out, /\[check-fork-only-patches\]/, 'guard produced no verdict at all'); +}); + +test('rescued out of the repo, it still checks the repo it is run from', (t) => { + // The documented rebuild recovery: the tree lost the guard, so it is read + // back out of the surviving ref into /tmp and run from the repo. + const box = mkdtempSync(join(tmpdir(), 'guard-rescue-')); + t.after(() => rmSync(box, { recursive: true, force: true })); + const rescued = join(box, 'guard.mjs'); + copyFileSync(GUARD, rescued); + + const { code, out } = runGuard([rescued], REPO_ROOT); + assert.equal(code, 0, out); + assert.match(out, /fork-only patch\(es\) present/); + // A script-relative root would have resolved to "/" and found no registry. + assert.doesNotMatch(out, /registry unreadable/); +}); + +test('--repo-root targets an explicit tree', (t) => { + const root = rebuiltRepo(t, { rebuild: () => {} }); + const { code, out } = runGuard([GUARD, '--repo-root', root], tmpdir()); + assert.equal(code, 0, out); + assert.match(out, /1 fork-only patch\(es\) present/); +}); + +test('an explicit tree with no resolvable baseline still fails closed', (t) => { + // --repo-root must not become a way to opt out of the survival half. + const root = fixture(t, { registry: { baselineRef: BASELINE, patches: REGISTRY.patches } }); + const { code, out } = runGuard([GUARD, '--repo-root', root], tmpdir()); + assert.equal(code, 1); + assert.match(out, /rebuild loss cannot be detected/); +}); From 57adf7e960ffe2f92264de0cfc214c9a548c6002 Mon Sep 17 00:00:00 2001 From: Ragdoll-Opus-5 Date: Tue, 22 Sep 2026 11:50:41 +0800 Subject: [PATCH 4/5] fix(F192): make the baseline immutable, and stop the registry from hiding its own reversion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: 砚砚's round-2 review was right that the durability path was still broken, and reproducing it found a worse case than the one reported. ## The reported defect, confirmed CI fetched `origin/develop_base` AFTER the push, so the "baseline" was the very tree being validated - a witness that is the accused. The fixture only ever proved a locally preserved old ref, never the post-push lifecycle. ## The worse case found while reproducing it The registry is itself a shared file, so a rebuild can REVERT it instead of deleting it. Reproduced end to end: a rebuild that rolls the registry back and drops the F192 gate leaves both trees internally consistent, and the guard reports OK - 1 fork-only patch(es) present; 1 claimed by origin/develop_base still present exit 0. It confirms its own amnesia as healthy. That is the exact disease this file exists to cure, so it is now the primary thing the check tests for. ## Fixes - Refuse to compare a commit with itself. A baseline that resolves to HEAD fails closed naming `--baseline-ref`, instead of reporting green. - Compare claim SETS, not just files: any patch id the baseline registered that is no longer registered is a loss, reported as a reverted registry. - `--baseline-ref` is now actually honoured (it was parsed for the repo root but never passed to the survival check). - CI passes the immutable pre-push commit - `github.event.before` for a push (survives force-push), `pull_request.base.sha` for a PR - and fetches it, since a force-pushed rebuild leaves the old tip unreachable. - Removed the `|| retry-without-baseline` fallback. A guard that quietly downgrades itself when its input is missing is the disease. `--no-baseline` now makes that skip an explicit, printed intent for the one honest case (branch creation). ## Scope stated honestly An in-tree workflow cannot survive its own deletion: if a rebuild drops workflow + guard + registry together, GitHub runs the workflow from the pushed commit where it no longer exists, so nothing runs. The doc no longer claims otherwise. It now names the authoritative external enforcement - branch protection on `develop_base` requiring the `Fork-only patch guard` check, so a check that never reports BLOCKS rather than passes - and records that this needs repo-admin rights and is not configured by this PR. Tests: post-push advanced baseline, reverted registry, `--baseline-ref`, `--no-baseline`. 65/65 total, biome clean repo-wide, workflow YAML parses. Co-Authored-By: Claude Opus 5 --- .github/workflows/fork-only-patches.yml | 39 ++++++- docs/fork-only-patches.md | 89 ++++++++++------ scripts/check-fork-only-patches.mjs | 62 ++++++++++- scripts/check-fork-only-patches.test.mjs | 125 ++++++++++++++++++++++- 4 files changed, 270 insertions(+), 45 deletions(-) diff --git a/.github/workflows/fork-only-patches.yml b/.github/workflows/fork-only-patches.yml index 93c19a6101..cc7e274d4c 100644 --- a/.github/workflows/fork-only-patches.yml +++ b/.github/workflows/fork-only-patches.yml @@ -31,13 +31,44 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - # The rebuild-survival check compares against the previous develop_base - # tip, which actions/checkout does not fetch by default. - - run: git fetch --no-tags --depth=1 origin develop_base:refs/remotes/origin/develop_base + with: + fetch-depth: 0 - uses: actions/setup-node@v4 with: node-version: 24 + + # The baseline must be the IMMUTABLE PRE-push commit. Fetching + # `origin/develop_base` here would name the tree we are validating - a + # witness that is the accused - and the old code did exactly that. + # GitHub supplies the previous tip out of band, which the pushed tree + # cannot forge: github.event.before for a push (survives force-push), + # pull_request.base.sha for a PR. + - id: baseline + run: | + case "${{ github.event_name }}" in + push) sha='${{ github.event.before }}' ;; + pull_request) sha='${{ github.event.pull_request.base.sha }}' ;; + *) sha='' ;; + esac + # All-zeros means the branch did not exist before: no prior claim. + if [ -z "$sha" ] || [ "$sha" = "0000000000000000000000000000000000000000" ]; then + echo "sha=" >> "$GITHUB_OUTPUT" + else + # A force-pushed rebuild leaves the old tip unreachable from any ref. + git fetch --no-tags origin "$sha" 2>/dev/null || true + echo "sha=$sha" >> "$GITHUB_OUTPUT" + fi + # No dependency install: the guard uses node builtins only, so it stays # runnable even when the tree is mid-rebuild and deps are inconsistent. - run: node --test scripts/check-fork-only-patches.test.mjs - - run: node scripts/check-fork-only-patches.mjs + - name: Fork-only patch guard + run: | + # No `||` fallback on purpose: if the baseline was supposed to exist + # and cannot be resolved, the guard must go red, not quietly retry + # with a weaker check. + if [ -n "${{ steps.baseline.outputs.sha }}" ]; then + node scripts/check-fork-only-patches.mjs --baseline-ref "${{ steps.baseline.outputs.sha }}" + else + node scripts/check-fork-only-patches.mjs --no-baseline + fi diff --git a/docs/fork-only-patches.md b/docs/fork-only-patches.md index 17d82b755b..5ac9f3e6f8 100644 --- a/docs/fork-only-patches.md +++ b/docs/fork-only-patches.md @@ -77,48 +77,73 @@ commits, no merge-base with upstream main, a July full-tree copy - so "is the patch in the carrier" is not a survival predicate, and asking for current code to be committed onto a stale unrelated history is incoherent. -## What actually survives: the remote-tracking ref +## What actually survives: an immutable pre-reset commit -`git reset --hard upstream/main` replaces the working tree. It does **not** move -`origin/develop_base`. The previous tree's registry is therefore still readable -straight out of git after the rebuild has erased it from disk: +`git reset --hard upstream/main` replaces the working tree, so the baseline has +to come from outside it. Two sources qualify, and one that looks like it does +**not**: -```bash -git show origin/develop_base:scripts/fork-only-patches.json -``` +| Source | Immutable? | Notes | +|---|---|---| +| `github.event.before` (push) | yes | previous branch tip, supplied by the platform; survives force-push | +| `github.event.pull_request.base.sha` | yes | the PR base | +| local `origin/develop_base` **before** pushing | yes, briefly | still names the prior tip during a manual pre-push run | +| `origin/develop_base` fetched **after** the push | **NO** | it is the tree being validated | -So the guard compares what the previous tree **claimed to protect** against what -the new tree **actually has**. It stays runnable even when the rebuild deleted -it, because it can be read from the same ref: +That last row was a real bug in this workflow's first version: CI fetched +`origin/develop_base` after the push, so the baseline *was* the accused. The +checker now refuses to compare a commit with itself and says so, instead of +reporting a cheerful green. -```bash -cd # the guard asks git where the repo is, from CWD -git show origin/develop_base:scripts/check-fork-only-patches.mjs > /tmp/guard.mjs -node /tmp/guard.mjs # or: node /tmp/guard.mjs --repo-root -``` +### The registry is a shared file too -The rescued copy resolves the repo from the working directory, not from where -the file happens to sit, and it detects direct invocation by **real** path - -running it from a symlinked location (macOS `/tmp`) must not make it exit 0 -having checked nothing. +A rebuild can **revert** `scripts/fork-only-patches.json` rather than delete it. +Both trees then look internally consistent, and a guard that only checks the +patches its *current* registry lists will confirm its own amnesia as healthy. +So the survival check compares the **claim sets**: every patch id the baseline +registered must still be registered. A claim that quietly disappeared from the +registry is a loss, not a smaller honest claim. -## Two checks, both required +### Running it ```bash -pnpm check:fork-only-patches +# Rebuild pipeline: capture the tip BEFORE the destructive reset. +PRE=$(git rev-parse origin/develop_base) +git fetch upstream && git reset --hard upstream/main # ... rebuild ... +node scripts/check-fork-only-patches.mjs --baseline-ref "$PRE" ``` -1. **Tree check** - is each registered patch here right now? Answers - "is production broken". -2. **Rebuild-survival check** - is everything `baselineRef` claimed still - present? Answers "did this rebuild drop something", **before** the result is - pushed or deployed. - -Both fail closed: a missing registry, an unreachable baseline, a hollow patch -entry, or an anchor that moved after the step it must precede are all red. A -guard that cannot see its input must not report clean. The one deliberate -exception is a baseline that carried no registry at all - an absent prior claim -is not an unverifiable one. +`--no-baseline` skips the survival half and says so in the output. It exists for +the one honest case with no prior tree (branch creation) and must never be used +as a fallback when a baseline lookup failed - a guard that quietly downgrades +itself is the disease, not the cure. + +## What this in-tree workflow does NOT close + +`.github/workflows/fork-only-patches.yml` cannot survive its own deletion. If a +rebuild drops workflow + guard + registry together, GitHub runs the workflow +**from the pushed commit**, where it no longer exists, so nothing runs and +nothing goes red. + +So, honestly scoped: + +- **Closed by the workflow**: any rebuild that preserves it - the baseline is the + immutable pre-push SHA, so dropped files, reverted anchors and dropped registry + claims are all named before the result is deployed. +- **NOT closed by the workflow**: a rebuild that deletes the workflow itself. + +The authoritative enforcement for that case lives **outside the repository**, in +branch protection, and is not configured by this PR: + +> Settings -> Branches -> branch protection rule for `develop_base` +> -> **Require status checks to pass before merging** +> -> required check: **`Fork-only patch guard`** + +A required check that never reports **blocks** rather than passes, which is +exactly the fail-closed behaviour the in-tree file cannot provide for its own +absence. This requires repository-admin rights; until it is configured, treat +the workflow as covering the common case only, and do not claim the full-wipe +case is mechanically prevented. ## Rebuild SOP diff --git a/scripts/check-fork-only-patches.mjs b/scripts/check-fork-only-patches.mjs index 2a64a5c125..3499372e65 100755 --- a/scripts/check-fork-only-patches.mjs +++ b/scripts/check-fork-only-patches.mjs @@ -208,6 +208,26 @@ export function checkRebuildSurvival(repoRoot, options = {}) { return { violations, baselineRef: null, comparedPatches: 0 }; } + // PR #185 review (砚砚 P1): CI fetched `origin/develop_base` AFTER the push, so + // the "baseline" was the very tree being validated. A witness that is the + // accused cannot testify - and it fails silently, which is the whole disease. + // The baseline must be an immutable PRE-reset commit, so refuse to compare a + // commit with itself. + try { + const head = gitOut(repoRoot, ['rev-parse', 'HEAD^{commit}']).trim(); + const base = gitOut(repoRoot, ['rev-parse', `${baselineRef}^{commit}`]).trim(); + if (head === base) { + violations.push( + `baseline "${baselineRef}" resolves to HEAD (${head.slice(0, 9)}), the same tree being checked, ` + + `so it cannot witness a loss. Pass the pre-reset commit explicitly: --baseline-ref `, + ); + return { violations, baselineRef, comparedPatches: 0 }; + } + } catch { + violations.push(`cannot resolve HEAD or "${baselineRef}" to a commit; rebuild loss cannot be detected`); + return { violations, baselineRef, comparedPatches: 0 }; + } + let baselineRaw; try { baselineRaw = gitOut(repoRoot, ['show', `${baselineRef}:${REGISTRY_REL}`]); @@ -230,6 +250,20 @@ export function checkRebuildSurvival(repoRoot, options = {}) { return { violations, baselineRef, comparedPatches: 0 }; } + // The registry is itself a shared file, so a rebuild can REVERT it instead of + // deleting it. Both trees then look internally consistent and the guard + // confirms its own amnesia as healthy. Compare the claim SETS, not just files. + const treeIds = new Set((treeRegistry?.patches ?? []).map((p) => p?.id).filter(Boolean)); + for (const patch of baseline.patches) { + const id = patch?.id; + if (typeof id === 'string' && id.length > 0 && treeRegistry && !treeIds.has(id)) { + violations.push( + `REBUILD LOSS vs ${baselineRef} - patch "${id}" was registered in the baseline but is no longer in ` + + `${REGISTRY_REL}; the registry was reverted, not just the patch`, + ); + } + } + const dropped = []; for (const patch of baseline.patches) { const id = patch?.id ?? ''; @@ -279,10 +313,26 @@ function resolveRepoRoot(argv) { } } +function readFlag(argv, name) { + const at = argv.indexOf(name); + return at !== -1 && argv[at + 1] ? argv[at + 1] : undefined; +} + function main() { - const repoRoot = resolveRepoRoot(process.argv.slice(2)); + const argv = process.argv.slice(2); + const repoRoot = resolveRepoRoot(argv); const tree = checkForkOnlyPatches(repoRoot); - const survival = checkRebuildSurvival(repoRoot); + // --baseline-ref names the immutable PRE-reset commit. CI must pass it + // (github.event.before / pull_request.base.sha); the registry default is only + // good enough for a manual pre-push run, where the local ref still names the + // prior tip. + // --no-baseline is for the one honest case with no prior tree at all (branch + // creation). It is explicit on purpose: skipping the survival half must be a + // stated intent, never a silent fallback when a baseline lookup failed. + const skipBaseline = argv.includes('--no-baseline'); + const survival = skipBaseline + ? { violations: [], baselineRef: null, comparedPatches: 0 } + : checkRebuildSurvival(repoRoot, { baselineRef: readFlag(argv, '--baseline-ref') }); const seen = new Set(); const violations = [...tree.violations, ...survival.violations].filter((v) => { if (seen.has(v)) return false; @@ -301,9 +351,11 @@ function main() { process.exit(1); } - const baseline = survival.baselineRef - ? `; ${survival.comparedPatches} claimed by ${survival.baselineRef} still present` - : ''; + const baseline = skipBaseline + ? '; rebuild-survival SKIPPED (--no-baseline: no prior tree)' + : survival.baselineRef + ? `; ${survival.comparedPatches} claimed by ${survival.baselineRef} still present` + : ''; console.log(`[check-fork-only-patches] OK - ${patchIds.length} fork-only patch(es) present${baseline}`); } diff --git a/scripts/check-fork-only-patches.test.mjs b/scripts/check-fork-only-patches.test.mjs index 1e659854e3..fd822d2feb 100644 --- a/scripts/check-fork-only-patches.test.mjs +++ b/scripts/check-fork-only-patches.test.mjs @@ -207,6 +207,11 @@ function rebuiltRepo(t, { rebuild }) { git(root, 'update-ref', BASELINE, gitCapture(root, 'rev-parse', 'HEAD').trim()); rebuild(root); + // A real rebuild produces a commit, so HEAD must advance past the baseline. + // Leaving it uncommitted would make the baseline and the tree under test the + // same commit, which the guard now (correctly) refuses to compare. + git(root, 'add', '-A'); + git(root, 'commit', '--allow-empty', '-m', 'rebuild onto upstream main'); return root; } @@ -255,14 +260,21 @@ test('an unreachable baseline fails closed with an actionable recovery', (t) => }); test('a baseline that never carried a registry makes no claim to lose', (t) => { + // First adoption: the prior tip predates the guard. Absence of a prior claim + // is not an unverifiable claim, so it must not be dressed up as a violation. const root = rebuiltRepo(t, { rebuild: () => {} }); - // Point at a commit with no registry: absence of a prior claim is not an - // unverifiable claim, so it must not be dressed up as a violation. - git(root, 'update-ref', 'refs/remotes/origin/empty', gitCapture(root, 'rev-parse', 'HEAD').trim()); + const registry = readFileSync(join(root, 'scripts/fork-only-patches.json'), 'utf8'); + rmSync(join(root, 'scripts/fork-only-patches.json')); git(root, 'add', '-A'); - git(root, 'commit', '-m', 'drop registry'); + git(root, 'commit', '-m', 'pre-adoption tip: no registry'); git(root, 'update-ref', 'refs/remotes/origin/empty', gitCapture(root, 'rev-parse', 'HEAD').trim()); + + // HEAD then advances past that baseline and adopts the registry. + writeFileSync(join(root, 'scripts/fork-only-patches.json'), registry); + git(root, 'add', '-A'); + git(root, 'commit', '-m', 'adopt the guard'); + const { violations, comparedPatches } = checkRebuildSurvival(root, { baselineRef: 'refs/remotes/origin/empty', }); @@ -387,3 +399,108 @@ test('an explicit tree with no resolvable baseline still fails closed', (t) => { assert.equal(code, 1); assert.match(out, /rebuild loss cannot be detected/); }); + +// --------------------------------------------------------------------------- +// Post-push lifecycle (PR #185 review round 2, 砚砚 P1). +// +// CI fetched `origin/develop_base` AFTER the push, so the "baseline" was the +// very tree being validated. Worse, the registry is itself a shared file: a +// rebuild can REVERT it rather than delete it, and then both trees look +// internally consistent and the guard confirms its own amnesia as healthy. +// --------------------------------------------------------------------------- + +function pushedRepo(t, { baselinePatches, afterPatches, files }) { + const root = mkdtempSync(join(tmpdir(), 'guard-postpush-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + mkdirp(join(root, 'scripts')); + mkdirp(join(root, 'src')); + git(root, 'init', '--initial-branch=develop_base'); + git(root, 'config', 'user.email', 'guard@test.local'); + git(root, 'config', 'user.name', 'guard'); + + const writeRegistry = (patches) => + writeFileSync( + join(root, 'scripts/fork-only-patches.json'), + JSON.stringify({ baselineRef: BASELINE, patches }, null, 2), + ); + + writeRegistry(baselinePatches); + writeFileSync(join(root, 'src/kept.ts'), 'export const kept = 1;\n'); + writeFileSync(join(root, 'src/dropped.ts'), 'export const dropped = 1;\n'); + git(root, 'add', '-A'); + git(root, 'commit', '-m', 'pre-rebuild'); + const preResetSha = gitCapture(root, 'rev-parse', 'HEAD').trim(); + git(root, 'update-ref', BASELINE, preResetSha); + + writeRegistry(afterPatches); + for (const [rel, content] of Object.entries(files ?? {})) { + if (content === null) rmSync(join(root, rel)); + else writeFileSync(join(root, rel), content); + } + git(root, 'add', '-A'); + git(root, 'commit', '--allow-empty', '-m', 'rebuild onto upstream main'); + // The push: the remote-tracking ref advances to the tree under validation. + git(root, 'update-ref', BASELINE, gitCapture(root, 'rev-parse', 'HEAD').trim()); + return { root, preResetSha }; +} + +const KEPT = { id: 'kept-patch', requiredFiles: ['src/kept.ts'] }; +const DROPPED = { id: 'f192-gate', requiredFiles: ['src/dropped.ts'] }; + +test('a baseline that advanced to the validated tree fails closed, never green', (t) => { + const { root } = pushedRepo(t, { + baselinePatches: [KEPT, DROPPED], + afterPatches: [KEPT], + files: { 'src/dropped.ts': null }, + }); + // This is exactly what CI did: fetch origin/develop_base after the push. + const { violations } = checkRebuildSurvival(root, { baselineRef: BASELINE }); + assert.ok(violations.length > 0, 'a witness that is the accused must not return green'); + assert.match(violations[0], /resolves to HEAD/); + assert.match(violations[0], /--baseline-ref/); +}); + +test('a reverted registry is a loss, not a smaller honest claim', (t) => { + const { root, preResetSha } = pushedRepo(t, { + baselinePatches: [KEPT, DROPPED], + afterPatches: [KEPT], + files: { 'src/dropped.ts': null }, + }); + const { violations } = checkRebuildSurvival(root, { baselineRef: preResetSha }); + assert.ok( + violations.some((v) => v.includes('f192-gate') && v.includes('registry was reverted')), + `the dropped CLAIM must be named, not just the file: ${violations.join(' | ')}`, + ); + assert.ok( + violations.some((v) => v.includes('src/dropped.ts')), + 'the dropped file must still be named too', + ); +}); + +test('the CLI honours --baseline-ref instead of the registry default', (t) => { + const { root, preResetSha } = pushedRepo(t, { + baselinePatches: [KEPT, DROPPED], + afterPatches: [KEPT], + files: { 'src/dropped.ts': null }, + }); + copyFileSync(GUARD, join(root, 'scripts/check-fork-only-patches.mjs')); + + const withBaseline = runGuard( + [join(root, 'scripts/check-fork-only-patches.mjs'), '--baseline-ref', preResetSha], + root, + ); + assert.equal(withBaseline.code, 1, withBaseline.out); + assert.match(withBaseline.out, /registry was reverted/); + + const withoutBaseline = runGuard([join(root, 'scripts/check-fork-only-patches.mjs')], root); + assert.equal(withoutBaseline.code, 1, 'the advanced default must fail closed too'); + assert.match(withoutBaseline.out, /resolves to HEAD/); +}); + +test('--no-baseline states the skip instead of hiding it', (t) => { + const { root } = pushedRepo(t, { baselinePatches: [KEPT], afterPatches: [KEPT] }); + copyFileSync(GUARD, join(root, 'scripts/check-fork-only-patches.mjs')); + const { code, out } = runGuard([join(root, 'scripts/check-fork-only-patches.mjs'), '--no-baseline'], root); + assert.equal(code, 0, out); + assert.match(out, /rebuild-survival SKIPPED/); +}); From 7bcd70cf6f2e483d2077a933c1fe7efa5be1229e Mon Sep 17 00:00:00 2001 From: "Ragdoll-Opus-5.5" Date: Sat, 10 Oct 2026 20:21:15 -0700 Subject: [PATCH 5/5] fix(F192): let canonical pnpm check pass on a clean develop_base tip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: 砚砚's round-4 review found a merge-time regression in approved code. Once this PR lands, a clean develop_base checkout has HEAD == origin/develop_base. The survival check failed closed whenever its baseline resolved to HEAD, and the guard is the FIRST step of `check:sources` - so canonical `pnpm check` would be red on the very branch the guard protects, and nothing after it would run. The fail-closed rule was right for the caller it was written for and wrong for the other one. - An EXPLICIT --baseline-ref that resolves to HEAD means the caller mis-wired the witness. That is the round-2 CI bug, so it still fails closed. - The registry DEFAULT resolving to HEAD means there is no older tree in this checkout. HEAD now witnesses the WORKING TREE instead, so an uncommitted registry or patch reversion is still caught rather than skipped. The default path cannot see a loss that was already committed, and now says so in its output instead of implying a verification. That is why CI passes the immutable pre-reset commit, and why the registry now anchors that wiring in the workflow (`--baseline-ref`, `github.event.before`, `pull_request.base.sha`): deleting it turns the TREE check red rather than letting CI quietly fall back to the default. `--no-baseline` is untouched and still reserved for branch creation. Tests, red before the change (4 failing, the explicit-HEAD one already green): - clean develop_base tip: default invocation passes - explicit --baseline-ref HEAD: still fails closed - at the tip, HEAD catches an uncommitted registry reversion and a dropped file - the workflow keeps passing an explicit immutable baseline (registry anchor) - the existing "honours --baseline-ref" test now asserts the default is honest ("no older baseline") instead of asserting it fails Mutation: forcing the explicit branch open turns exactly two tests red, the round-2 regression test and the new explicit-HEAD test, so the CI defence still bites. 28/28 guard tests, biome clean, git diff --check clean. Co-Authored-By: Claude Opus 5.5 --- docs/fork-only-patches.md | 23 ++++++- scripts/check-fork-only-patches.mjs | 49 ++++++++++---- scripts/check-fork-only-patches.test.mjs | 83 +++++++++++++++++++++++- scripts/fork-only-patches.json | 9 ++- 4 files changed, 145 insertions(+), 19 deletions(-) diff --git a/docs/fork-only-patches.md b/docs/fork-only-patches.md index 5ac9f3e6f8..3491c033d2 100644 --- a/docs/fork-only-patches.md +++ b/docs/fork-only-patches.md @@ -92,8 +92,27 @@ to come from outside it. Two sources qualify, and one that looks like it does That last row was a real bug in this workflow's first version: CI fetched `origin/develop_base` after the push, so the baseline *was* the accused. The -checker now refuses to compare a commit with itself and says so, instead of -reporting a cheerful green. +checker now refuses to compare a commit with itself **when the baseline was +named explicitly**, instead of reporting a cheerful green. + +### Explicit vs default baseline + +Who names the baseline decides what "baseline == HEAD" means: + +| Caller | baseline == HEAD means | Result | +|---|---|---| +| explicit `--baseline-ref` (CI) | the caller mis-wired the witness | **fail closed** | +| registry default (manual run) | there is no older tree in this checkout | HEAD witnesses the **working tree** | + +The default case is the normal state of a clean `develop_base` checkout. Failing +closed there made the first step of canonical `pnpm check` red on the branch the +guard protects, so nothing after it ran. Instead, the committed HEAD is compared +against the working tree: an uncommitted registry or patch reversion is still +caught. What the default path **cannot** see is a loss that was already +committed - the output says so explicitly - and that is exactly why CI passes +the immutable pre-reset commit, and why the registry anchors that wiring in the +workflow: deleting `--baseline-ref` from CI turns the tree check red rather +than quietly falling back to the default. ### The registry is a shared file too diff --git a/scripts/check-fork-only-patches.mjs b/scripts/check-fork-only-patches.mjs index 3499372e65..8d9ada8abf 100755 --- a/scripts/check-fork-only-patches.mjs +++ b/scripts/check-fork-only-patches.mjs @@ -198,6 +198,10 @@ export function checkRebuildSurvival(repoRoot, options = {}) { // check's to report. Here it is only the source of the baseline ref, plus the // signal that the guard's own input was erased. const treeRegistry = readRegistry(repoRoot, []); + // Who named the baseline matters. An EXPLICIT baseline is a caller asserting + // "this is the pre-reset tree" - CI passes github.event.before. The registry + // DEFAULT is only a best guess for a manual run. + const explicit = options.baselineRef !== undefined; const baselineRef = options.baselineRef ?? treeRegistry?.baselineRef ?? 'origin/develop_base'; if (!resolveRef(repoRoot, baselineRef)) { @@ -213,19 +217,34 @@ export function checkRebuildSurvival(repoRoot, options = {}) { // accused cannot testify - and it fails silently, which is the whole disease. // The baseline must be an immutable PRE-reset commit, so refuse to compare a // commit with itself. + // + // PR #185 review round 4 (砚砚 P1): failing closed on the DEFAULT resolving to + // HEAD made canonical `pnpm check` red on every clean develop_base checkout - + // the guard's first step blocked every check after it, on the branch it + // exists to protect. So the rule is scoped to the caller that can actually + // mis-wire it: + // - explicit baseline == HEAD -> fail closed (witness is the accused) + // - default baseline == HEAD -> no older tree exists here, so HEAD itself + // witnesses the WORKING TREE below. An uncommitted registry or patch + // reversion is still caught; only a committed rebuild loss is out of + // reach, and that is what CI's explicit pre-reset baseline is for. + let defaultBaselineIsHead = false; try { const head = gitOut(repoRoot, ['rev-parse', 'HEAD^{commit}']).trim(); const base = gitOut(repoRoot, ['rev-parse', `${baselineRef}^{commit}`]).trim(); if (head === base) { - violations.push( - `baseline "${baselineRef}" resolves to HEAD (${head.slice(0, 9)}), the same tree being checked, ` + - `so it cannot witness a loss. Pass the pre-reset commit explicitly: --baseline-ref `, - ); - return { violations, baselineRef, comparedPatches: 0 }; + if (explicit) { + violations.push( + `baseline "${baselineRef}" resolves to HEAD (${head.slice(0, 9)}), the same tree being checked, ` + + `so it cannot witness a loss. Pass the pre-reset commit explicitly: --baseline-ref `, + ); + return { violations, baselineRef, comparedPatches: 0, defaultBaselineIsHead }; + } + defaultBaselineIsHead = true; } } catch { violations.push(`cannot resolve HEAD or "${baselineRef}" to a commit; rebuild loss cannot be detected`); - return { violations, baselineRef, comparedPatches: 0 }; + return { violations, baselineRef, comparedPatches: 0, defaultBaselineIsHead }; } let baselineRaw; @@ -235,7 +254,7 @@ export function checkRebuildSurvival(repoRoot, options = {}) { // The previous tree made no claim, so there is nothing it could have lost. // This is a real absence of a prior claim, not an unverifiable one - the // fail-closed rule applies to the latter. - return { violations, baselineRef, comparedPatches: 0 }; + return { violations, baselineRef, comparedPatches: 0, defaultBaselineIsHead }; } let baseline; @@ -243,11 +262,11 @@ export function checkRebuildSurvival(repoRoot, options = {}) { baseline = JSON.parse(baselineRaw); } catch (err) { violations.push(`baseline registry at ${baselineRef} is not valid JSON: ${err.message}`); - return { violations, baselineRef, comparedPatches: 0 }; + return { violations, baselineRef, comparedPatches: 0, defaultBaselineIsHead }; } if (!baseline || !Array.isArray(baseline.patches)) { violations.push(`baseline registry at ${baselineRef} has no "patches" array`); - return { violations, baselineRef, comparedPatches: 0 }; + return { violations, baselineRef, comparedPatches: 0, defaultBaselineIsHead }; } // The registry is itself a shared file, so a rebuild can REVERT it instead of @@ -285,7 +304,7 @@ export function checkRebuildSurvival(repoRoot, options = {}) { ); } - return { violations, baselineRef, comparedPatches: baseline.patches.length }; + return { violations, baselineRef, comparedPatches: baseline.patches.length, defaultBaselineIsHead }; } /** @@ -353,9 +372,13 @@ function main() { const baseline = skipBaseline ? '; rebuild-survival SKIPPED (--no-baseline: no prior tree)' - : survival.baselineRef - ? `; ${survival.comparedPatches} claimed by ${survival.baselineRef} still present` - : ''; + : survival.defaultBaselineIsHead + ? `; default baseline ${survival.baselineRef} is HEAD - no older baseline to compare, so uncommitted ` + + `changes were checked against the committed HEAD only (a committed rebuild loss needs an explicit ` + + `pre-reset --baseline-ref, which CI passes)` + : survival.baselineRef + ? `; ${survival.comparedPatches} claimed by ${survival.baselineRef} still present` + : ''; console.log(`[check-fork-only-patches] OK - ${patchIds.length} fork-only patch(es) present${baseline}`); } diff --git a/scripts/check-fork-only-patches.test.mjs b/scripts/check-fork-only-patches.test.mjs index fd822d2feb..0964d49ad6 100644 --- a/scripts/check-fork-only-patches.test.mjs +++ b/scripts/check-fork-only-patches.test.mjs @@ -492,9 +492,13 @@ test('the CLI honours --baseline-ref instead of the registry default', (t) => { assert.equal(withBaseline.code, 1, withBaseline.out); assert.match(withBaseline.out, /registry was reverted/); + // The default ref has advanced to the rebuilt tree, so it cannot see the + // committed loss. It must say so instead of claiming a verification - which + // is exactly why CI passes the immutable pre-reset ref explicitly. const withoutBaseline = runGuard([join(root, 'scripts/check-fork-only-patches.mjs')], root); - assert.equal(withoutBaseline.code, 1, 'the advanced default must fail closed too'); - assert.match(withoutBaseline.out, /resolves to HEAD/); + assert.equal(withoutBaseline.code, 0, withoutBaseline.out); + assert.match(withoutBaseline.out, /no older baseline/); + assert.doesNotMatch(withoutBaseline.out, /claimed by .* still present/); }); test('--no-baseline states the skip instead of hiding it', (t) => { @@ -504,3 +508,78 @@ test('--no-baseline states the skip instead of hiding it', (t) => { assert.equal(code, 0, out); assert.match(out, /rebuild-survival SKIPPED/); }); + +// --------------------------------------------------------------------------- +// Default vs explicit baseline (PR #185 review round 4, 砚砚 P1). +// +// Once merged, a clean develop_base checkout has HEAD == origin/develop_base. +// Failing closed on that made the FIRST step of canonical `pnpm check` red on +// the very branch it protects, so nothing after it ever ran. +// +// The fail-closed rule exists for the CI case: a caller who explicitly names a +// baseline that turns out to be the tree under test has mis-wired the witness. +// The registry default resolving to HEAD means something else - there is no +// older tree here - so HEAD itself becomes the witness for the WORKING TREE. +// That keeps catching an uncommitted registry or patch reversion instead of +// skipping the survival half outright. +// --------------------------------------------------------------------------- + +function tipRepo(t) { + // A clean develop_base tip: the remote-tracking default equals HEAD. + const { root } = pushedRepo(t, { baselinePatches: [KEPT, DROPPED], afterPatches: [KEPT, DROPPED] }); + assert.equal( + gitCapture(root, 'rev-parse', BASELINE).trim(), + gitCapture(root, 'rev-parse', 'HEAD').trim(), + 'fixture must model HEAD == origin/develop_base', + ); + return root; +} + +test('a clean develop_base tip passes the default invocation', (t) => { + const root = tipRepo(t); + const { code, out } = runGuard([GUARD, '--repo-root', root], tmpdir()); + assert.equal(code, 0, `canonical pnpm check would be red on its own target branch:\n${out}`); + assert.match(out, /no older baseline/); +}); + +test('an explicit --baseline-ref HEAD still fails closed', (t) => { + // The CI defence must not loosen: naming the accused as the witness is a + // mis-wired caller, whatever the registry default happens to be. + const root = tipRepo(t); + const { code, out } = runGuard([GUARD, '--repo-root', root, '--baseline-ref', 'HEAD'], tmpdir()); + assert.equal(code, 1, out); + assert.match(out, /resolves to HEAD/); +}); + +test('at the tip, HEAD witnesses an uncommitted registry reversion', (t) => { + const root = tipRepo(t); + // Uncommitted: drop a claim from the registry and delete its file. + writeFileSync( + join(root, 'scripts/fork-only-patches.json'), + JSON.stringify({ baselineRef: BASELINE, patches: [KEPT] }, null, 2), + ); + rmSync(join(root, 'src/dropped.ts')); + + const { code, out } = runGuard([GUARD, '--repo-root', root], tmpdir()); + assert.equal(code, 1, 'the default path must not skip survival - HEAD is still a witness'); + assert.match(out, /f192-gate.*registry was reverted/); + assert.match(out, /src\/dropped\.ts/); +}); + +test('the workflow keeps passing an explicit immutable baseline', () => { + // The default path is honest that it cannot see a committed rebuild loss, so + // CI must keep naming the pre-reset commit. Pin that wiring as a registered + // anchor: deleting it turns the TREE check red even if CI falls back. + const registry = JSON.parse(readFileSync(join(REPO_ROOT, 'scripts/fork-only-patches.json'), 'utf8')); + const wf = registry.patches + .flatMap((p) => p.requiredAnchors ?? []) + .find((a) => a.file === '.github/workflows/fork-only-patches.yml'); + assert.ok(wf, 'workflow must be a registered anchor file'); + for (const needle of [ + 'check-fork-only-patches.mjs --baseline-ref', + 'github.event.before', + 'github.event.pull_request.base.sha', + ]) { + assert.ok(wf.mustContain.includes(needle), `workflow anchor must pin ${needle}`); + } +}); diff --git a/scripts/fork-only-patches.json b/scripts/fork-only-patches.json index 56b31c9030..cc57b8f43e 100644 --- a/scripts/fork-only-patches.json +++ b/scripts/fork-only-patches.json @@ -46,8 +46,13 @@ "requiredAnchors": [ { "file": ".github/workflows/fork-only-patches.yml", - "mustContain": ["branches: [develop_base]"], - "note": "the trigger that makes the guard automatic; a workflow that survives with a main-only trigger is silently inert" + "mustContain": [ + "branches: [develop_base]", + "check-fork-only-patches.mjs --baseline-ref", + "github.event.before", + "github.event.pull_request.base.sha" + ], + "note": "the trigger that makes the guard automatic, AND the explicit immutable pre-reset baseline it passes. The default baseline cannot see a committed rebuild loss, so dropping --baseline-ref from CI must turn the tree check red rather than fall back silently." }, { "file": "package.json",