diff --git a/packages/core/src/__tests__/duplicate-guard.test.ts b/packages/core/src/__tests__/duplicate-guard.test.ts index d1f77426e6..6edc4334b5 100644 --- a/packages/core/src/__tests__/duplicate-guard.test.ts +++ b/packages/core/src/__tests__/duplicate-guard.test.ts @@ -3,7 +3,7 @@ import { describe, expect, it, vi } from "vitest"; import type { Column, Task } from "../types.js"; import type { TaskStore } from "../store.js"; import { computeContentFingerprint } from "../duplicate-detection.js"; -import { findRecentTasksByContentFingerprintImpl } from "../task-store/branch-and-pr-entities.js"; +import { resolveFingerprintWindowMs } from "../task-store/branch-and-pr-entities.js"; import { FINGERPRINT_WINDOW_DEFAULT_MS, FINGERPRINT_WINDOW_MAX_MS, @@ -325,50 +325,54 @@ describe("reconcileDeterministicDuplicate", () => { FNXC:TaskCreationDeduplication 2026-07-26-07:40: The store query owns a SECOND clamp on the same window. Code review found that widening only duplicate-guard.ts capped the effective window at the store's own 5-minute ceiling, and no test -caught it because the guard tests stub the query. These assertions pin the real cutoff the SQL -receives, so the two clamps cannot drift apart again. +caught it because the guard tests stub the query. The second clamp now lives in ONE exported policy +function that both the guard and the store query call, so the two cannot drift apart again — and it is +asserted directly rather than recovered from a stubbed query's cutoff string. */ -describe("findRecentTasksByContentFingerprintImpl window", () => { - function stubStore(): { store: TaskStore; cutoffs: string[] } { - const cutoffs: string[] = []; - const store = { - backendMode: false, - getTaskSelectClause: () => "t.*", - rowToTask: (row: unknown) => row as Task, - db: { - prepare: () => ({ - all: (_fingerprint: string, cutoffIso: string) => { - cutoffs.push(cutoffIso); - return []; - }, - }), - }, - } as unknown as TaskStore; - return { store, cutoffs }; - } +describe("duplicate-guard fingerprint window policy", () => { - it("defaults to the shared 10-minute window, not the store's old 60s/5m pair", async () => { - const { store, cutoffs } = stubStore(); - const before = Date.now(); - await findRecentTasksByContentFingerprintImpl(store, "fp"); - const windowMs = before - Date.parse(cutoffs[0]!); - expect(windowMs).toBeGreaterThanOrEqual(FINGERPRINT_WINDOW_DEFAULT_MS - 5_000); - expect(windowMs).toBeLessThanOrEqual(FINGERPRINT_WINDOW_DEFAULT_MS + 5_000); + /* + FNXC:TaskCreationDeduplication 2026-07-30-04:20: + Asserted on the pure policy function instead of through a store fake. These three drove + `findRecentTasksByContentFingerprintImpl` against a fake modelling the DELETED SQLite path + (`db.prepare().all()`) and recovered the window by parsing a captured cutoff string; that fake broke + when the query moved to `asyncLayer` + Drizzle ("Cannot read properties of undefined (reading + 'projectId')"). + + Rebuilding a Drizzle chain to recover a number the policy already returns would be mock-the-world + for no gain. Targeting `resolveFingerprintWindowMs` also removes the +/-5s timing tolerance the old + shape needed, so these now assert exact values. + */ + it("defaults to the shared 10-minute window, not the store's old 60s/5m pair", () => { + expect(resolveFingerprintWindowMs()).toBe(FINGERPRINT_WINDOW_DEFAULT_MS); + expect(FINGERPRINT_WINDOW_DEFAULT_MS).toBeGreaterThan(300_000); }); - it("honors an explicit window above the old 5-minute ceiling", async () => { - const { store, cutoffs } = stubStore(); - const before = Date.now(); - await findRecentTasksByContentFingerprintImpl(store, "fp", { windowMs: 20 * 60_000 }); - const windowMs = before - Date.parse(cutoffs[0]!); - expect(windowMs).toBeGreaterThan(300_000); + it("honors an explicit window above the old 5-minute ceiling", () => { + expect(resolveFingerprintWindowMs(20 * 60_000)).toBe(20 * 60_000); + expect(resolveFingerprintWindowMs(20 * 60_000)).toBeGreaterThan(300_000); }); - it("still clamps to the shared ceiling", async () => { - const { store, cutoffs } = stubStore(); - const before = Date.now(); - await findRecentTasksByContentFingerprintImpl(store, "fp", { windowMs: 24 * 60 * 60_000 }); - const windowMs = before - Date.parse(cutoffs[0]!); - expect(windowMs).toBeLessThanOrEqual(FINGERPRINT_WINDOW_MAX_MS + 5_000); + it("still clamps to the shared ceiling", () => { + expect(resolveFingerprintWindowMs(24 * 60 * 60_000)).toBe(FINGERPRINT_WINDOW_MAX_MS); + }); + + /* + FNXC:TaskCreationDeduplication 2026-07-30-05:40 (coderabbit, major): + Regression for a PRE-EXISTING crash the extraction exposed: `Math.trunc(NaN)` is NaN and both clamps + pass it through, so the caller's `new Date(Date.now() - windowMs).toISOString()` threw "Invalid time + value". Verified by running the old inline expression directly. + */ + it("falls back to the default for a non-finite request instead of propagating NaN", () => { + expect(resolveFingerprintWindowMs(Number.NaN)).toBe(FINGERPRINT_WINDOW_DEFAULT_MS); + expect(resolveFingerprintWindowMs(Number.POSITIVE_INFINITY)).toBe(FINGERPRINT_WINDOW_DEFAULT_MS); + // The point of the guard: the value must be usable as a Date offset. + expect(() => new Date(Date.now() - resolveFingerprintWindowMs(Number.NaN)).toISOString()).not.toThrow(); + }); + + it("floors at 1ms so a zero or negative request cannot produce a future cutoff", () => { + // The `Math.max(1, ...)` half of the policy, which the old cutoff-parsing shape could not see. + expect(resolveFingerprintWindowMs(0)).toBe(1); + expect(resolveFingerprintWindowMs(-5_000)).toBe(1); }); }); diff --git a/packages/core/src/__tests__/settings-parity.test.ts b/packages/core/src/__tests__/settings-parity.test.ts index 24cc10c15b..971edbe77a 100644 --- a/packages/core/src/__tests__/settings-parity.test.ts +++ b/packages/core/src/__tests__/settings-parity.test.ts @@ -529,6 +529,15 @@ describe("settings key parity", () => { "gitlabApiBaseUrl", "gitlabAuthToken", "gitlabAuthTokenType", + /* + FNXC:ToolOutputBudget 2026-07-30-03:40: + Shared ON PURPOSE. settings-schema.ts:462 states it outright: "Project settings participate in + the existing effective-settings merge, allowing a project-specific tool-output cap or explicit + no-limit sentinel to override global policy." So a global default with a per-project override is + the intended shape, and this list is the record of intentional overlap. + Placed in GLOBAL_SETTINGS_KEYS order, as the comment above requires. + */ + "agentToolOutputMaxChars", "mcpServers", "worktrunk", "owningNodeHandoffPolicy", diff --git a/packages/core/src/__tests__/workflow-ir-settings.test.ts b/packages/core/src/__tests__/workflow-ir-settings.test.ts index 7ed2ebf953..c6ce63e43c 100644 --- a/packages/core/src/__tests__/workflow-ir-settings.test.ts +++ b/packages/core/src/__tests__/workflow-ir-settings.test.ts @@ -5,6 +5,7 @@ import { downgradeIrToV1IfPure, WorkflowIrError, } from "../workflow-ir.js"; +import { DEFAULT_MAX_POST_REVIEW_FIXES } from "../builtin-workflow-settings.js"; import { BUILTIN_CODING_WORKFLOW_IR } from "../builtin-coding-workflow-ir.js"; import { BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR } from "../builtin-stepwise-final-review-coding-workflow-ir.js"; import { getBuiltinWorkflow } from "../builtin-workflows.js"; @@ -245,7 +246,15 @@ describe("built-in workflow settings parity anchor (U1, R4)", () => { maxParallelSteps: 2, buildRetryCount: 0, verificationFixRetries: 3, - maxPostReviewFixes: 3, + /* + FNXC:WorkflowOptionalStepCycle 2026-07-30-03:45: + Driven off the exported constant, not a literal. `DEFAULT_MAX_POST_REVIEW_FIXES` + (builtin-workflow-settings.ts:555) exists BECAUSE the declaration default and two inline + literal 3s had drifted apart once; this file is the third place that pinned the stale 3, so a + fourth copy would guarantee a fourth drift. This is the parity anchor — it must follow the + declaration by construction. + */ + maxPostReviewFixes: DEFAULT_MAX_POST_REVIEW_FIXES, requirePrApproval: false, requirePlanApproval: false, reviewHandoffPolicy: "disabled", diff --git a/packages/core/src/task-store/branch-and-pr-entities.ts b/packages/core/src/task-store/branch-and-pr-entities.ts index 3525e0bb02..18b1c499dd 100644 --- a/packages/core/src/task-store/branch-and-pr-entities.ts +++ b/packages/core/src/task-store/branch-and-pr-entities.ts @@ -373,6 +373,30 @@ export async function getActiveMergingTaskImpl(store: TaskStore, excludeTaskId?: return rows[0]?.id; } +/* +FNXC:TaskCreationDeduplication 2026-07-30-04:20: +The duplicate-guard WINDOW POLICY as a pure function, extracted so it can be asserted without a +TaskStore. Byte-identical to the expression that was inlined below. + +Why extracted: the three tests that own this policy drove it through a store fake modelling the +deleted SQLite path (`db.prepare().all()`), and read the window back out of a captured cutoff string. +That fake broke when the query moved to `asyncLayer` + Drizzle (TypeError on `layer.projectId`), and +rebuilding it would have meant reconstructing a Drizzle chain to recover a number this function +already returns. Narrow seam over mock-the-world, per docs/testing.md. +*/ +export function resolveFingerprintWindowMs(requestedWindowMs?: number): number { + const requested = requestedWindowMs ?? FINGERPRINT_WINDOW_DEFAULT_MS; + /* + FNXC:TaskCreationDeduplication 2026-07-30-05:40 (coderabbit, major): + NaN must fall back, not propagate. `Math.trunc(NaN)` is NaN and both clamps pass it through, so the + caller's `new Date(Date.now() - windowMs).toISOString()` threw "Invalid time value" — a crash rather + than a bounded window. This hole is PRE-EXISTING (the inline expression this replaced was + byte-identical); naming the policy is what made it reachable by a test. + */ + if (!Number.isFinite(requested)) return FINGERPRINT_WINDOW_DEFAULT_MS; + return Math.max(1, Math.min(FINGERPRINT_WINDOW_MAX_MS, Math.trunc(requested))); +} + export async function findRecentTasksByContentFingerprintImpl(store: TaskStore, fingerprint: string, options?: { windowMs?: number; includeArchived?: boolean }, @@ -389,8 +413,7 @@ export async function findRecentTasksByContentFingerprintImpl(store: TaskStore, window at five minutes and made its ceiling unreachable — the guard asked for ten minutes and silently got five. One policy, one pair of bounds. */ - const requestedWindowMs = options?.windowMs ?? FINGERPRINT_WINDOW_DEFAULT_MS; - const windowMs = Math.max(1, Math.min(FINGERPRINT_WINDOW_MAX_MS, Math.trunc(requestedWindowMs))); + const windowMs = resolveFingerprintWindowMs(options?.windowMs); const cutoffIso = new Date(Date.now() - windowMs).toISOString(); const includeArchived = options?.includeArchived ?? false; diff --git a/packages/core/src/tool-output-budget.ts b/packages/core/src/tool-output-budget.ts index feb6b43a0d..b0ddba55e0 100644 --- a/packages/core/src/tool-output-budget.ts +++ b/packages/core/src/tool-output-budget.ts @@ -1,3 +1,18 @@ +import { createLogger } from "./logger.js"; + +/* +FNXC:EngineDiagnostics 2026-07-30-04:00: +FN-8603 requires production diagnostics to route through the shared logger rather than bare console +output, so every line carries the shared severity marker and subsystem prefix. This file had a bare +`console.warn` on the invalid-override path, which `log-severity-spam-contract` flags as a contract +violation. + +Kept at `warn`, NOT demoted: an invalid operator-supplied budget is a real misconfiguration, not +routine chatter. Note this path is therefore not FUSION_DEBUG-gated — only `debug` is — so what the +shared logger adds here is the marker and prefix, not suppression. +*/ +const log = createLogger("tool-output-budget"); + /** Default maximum model-visible characters in one engine-injected tool result. */ export const DEFAULT_TOOL_OUTPUT_MAX_CHARS = 16_000; @@ -7,13 +22,13 @@ export const TOOL_OUTPUT_UNLIMITED_SETTING_VALUE = 0; const DEFAULT_TRUNCATION_HINT = "narrow your query or use limit/offset for more"; /** - * FNXC:ToolOutputBudget 2026-08-06-12:00: + * FNXC:ToolOutputBudget 2026-07-30-12:00: * FN-8614 bounds the total text returned by each engine-injected tool result so a * large log, document, or JSON response cannot consume an agent's context window. * 16,000 characters remains the default while operators can use * `agentToolOutputMaxChars` to select a positive cap or the explicit no-limit value. * - * FNXC:ToolOutputBudget 2026-08-06-16:00: + * FNXC:ToolOutputBudget 2026-07-30-16:00: * FN-8616 requires an operator-controlled opt-out without making an unset or invalid * value unbounded. Only the `0` setting sentinel disables this shared wrapper. */ @@ -113,7 +128,7 @@ export function resolveToolOutputBudget( const error = new Error(`Invalid tool output budget for ${toolName}; overrides must be finite positive integers.`); if (process.env.NODE_ENV === "production") { - console.warn(error.message); + log.warn(error.message); return defaultMaxChars; } throw error; diff --git a/packages/engine/src/__tests__/log-severity-manifest.ts b/packages/engine/src/__tests__/log-severity-manifest.ts index 1a7aeaa825..fb9d638e90 100644 --- a/packages/engine/src/__tests__/log-severity-manifest.ts +++ b/packages/engine/src/__tests__/log-severity-manifest.ts @@ -41,7 +41,13 @@ export const logSeverityManifest: SeverityManifestEntry[] = [ { pkg: "engine", file: "pty-native.ts", anchor: "dlopen pre-load failed (continuing)", priorSeverity: "console", severity: "debug" }, { pkg: "engine", file: "goal-anchoring-audit.ts", anchor: "goal retrieval audit emission skipped", priorSeverity: "console", severity: "debug" }, { pkg: "engine", file: "runtimes/child-process-worker.ts", anchor: "Child process worker starting", priorSeverity: "log", severity: "debug" }, - { pkg: "core", file: "central-core.ts", anchor: "local reattached project ${project.id}", priorSeverity: "console", severity: "debug" }, + /* + FNXC:EngineDiagnostics 2026-07-30-04:00: + REMOVED: the `local reattached project ${project.id}` demotion in central-core.ts. Its call site was + deleted by 5ae6332563 ("collapse dead SQLite dual-path code") — verified absent from all of + packages/core/src, not merely moved — so the entry pinned a demotion that no longer exists and the + contract test could only ever fail on it. A manifest row for deleted code cannot ratchet anything. + */ { pkg: "core", file: "docker-provisioning.ts", anchor: "Pulling image ${imageRef}", priorSeverity: "console", severity: "debug" }, { pkg: "core", file: "docker-provisioning.ts", anchor: "provisioned successfully in ${durationMs}ms", priorSeverity: "console", severity: "debug" }, { pkg: "core", file: "docker-provisioning.ts", anchor: "deprovisioned", priorSeverity: "console", severity: "debug" },