test: ban asserting comment text, and fix the prompt it was hiding
Tests must assert behavior, not source text. A test that pins an FNXC block, a
date stamp, or comment prose guards documentation — and AGENTS.md tells authors
to keep those comments current, so the two rules fight and the test loses in the
worst way.
Measured today: grok-runtime-bootstrap.test.ts asserted runTaskMerge's body
contained "FNXC:GrokCliRouting 2026-07-15-10:17". FN-9167 legitimately rewrote
that function and dropped the block while leaving behavior intact; the test went
red, and the fix applied earlier in this sweep was to RE-ADD THE COMMENT to
packages/cli/src/commands/task.ts. A comment returned to shipped source not
because it documented anything true, but to appease a test. Four more such
assertions sat in dashboard CSS tests, each beside a real assertion, each adding
nothing.
- Drops the two prose pins from grok-runtime-bootstrap; its real structural
guard (`not.toContain("mergePluginRunner")`) stays. The product comment stays
too — it is accurate documentation, it was simply never a test's business.
- Adds scripts/check-no-comment-assertions-in-tests.mjs, wired into pretest,
pretest:full, and test:gate:static. It flags the unambiguous case; an earlier
draft that also matched `/*` produced 24 false positives and zero true ones,
because a regex cannot separate comment prose from a path glob.
- Adds the standing rule to AGENTS.md, with an explicit boundary: prose,
comments, and date stamps are never a test subject, while code-construct and
call-site-allowlist guards (no-blocking-shellout, vi-mock resolution, durable
write and emit-surface inventories, legacy tombstones) are a different
category and stay.
Also carries a product fix that the agent-generation tests surfaced: the
system prompt exists in two copies, and `resolvePrompt` returns core's catalog
default, so FN-021 adding the xhigh/max thinking levels to the dashboard copy
alone left every real generation run advertising levels that stop at "high".
Core's copy is synced and both literals now say they must move together.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
gsxdsm
17
AGENTS.md
17
AGENTS.md
@@ -210,6 +210,23 @@ FNXC:TestFlakeRegister 2026-08-01-07:00: Issue #2862 recorded three suite-only P
|
||||
- Motivating incident for UI affordances: the workflow-row drop-down arrow removal took three tasks (FN-6115 → FN-6118 → FN-6123) because the affordance rendered in two components and mobile kept an empty 36×36 `btn-icon` button shell.
|
||||
- If a regression test only proves the exact reported case, it is incomplete; extend it until the invariant holds across all known surfaces.
|
||||
|
||||
### Standing Rule: Tests Assert Behavior, Never Source Text Or Comments
|
||||
|
||||
**A test may never assert that a comment exists.** Not an `FNXC:` block, not a date stamp, not prose lifted from a source or CSS file. Comments are documentation; asserting them tests nothing a user, caller, or operator can observe.
|
||||
|
||||
This is not a style preference — it is a rule this repo's own conventions make self-defeating. The FNXC convention above tells authors to *keep these comments updated as requirements change*. A test pinning one guarantees a future false failure, and the cheapest way to make that failure go away is the worst possible action: **re-adding a comment to shipped source purely to satisfy a test.**
|
||||
|
||||
**Measured 2026-08-23.** `grok-runtime-bootstrap.test.ts` asserted `runTaskMerge`'s body contained the literal `FNXC:GrokCliRouting 2026-07-15-10:17`. FN-9167 legitimately rewrote that function and dropped the block while leaving the behavior intact. The test went red, and the fix applied was to restore the comment in `packages/cli/src/commands/task.ts` — a comment returned to the product not because it documented anything true, but to appease a test. Four more such assertions sat in the dashboard's CSS tests (`NewTaskModal`, `FloatingWindow`, `TaskDetailModal.responsive-and-dependencies`, `CommandCenter.tablet-layout`), each beside a real assertion and each adding nothing.
|
||||
|
||||
What to do instead, in order of preference:
|
||||
1. **Assert the behavior** the comment describes: the rendered style, the computed value, the observable outcome, the state after the action.
|
||||
2. **Assert the code construct** if the invariant is genuinely structural — `expect(body).not.toContain("mergePluginRunner")` is a real guard; `expect(body).toContain("FNXC:…")` is not.
|
||||
3. **Delete the assertion.** If nothing behavioral or structural is behind it, it was never guarding anything.
|
||||
|
||||
**Boundary — this rule does not touch code-construct guards.** Source scans that enforce call-site allowlists and architectural ratchets (`engine-no-blocking-shellout`, `user-configured-command-no-execsync`, `vi-mock-specifiers-resolve`, the durable-write and emit-surface inventories, `legacy-tombstones`, `lazy-loaded-views-docs`) assert code structure, are mandated elsewhere in this document, and stay. The line is: **prose, comments, and date stamps are never a test subject; code constructs may be.**
|
||||
|
||||
Enforced by `scripts/check-no-comment-assertions-in-tests.mjs`, which runs in `pretest`, `pretest:full`, and `test:gate:static`. It flags the unambiguous case (an `FNXC:` stamp inside an assertion matcher); an earlier draft that also matched `/*` produced 24 false positives and zero true ones, because a regex cannot tell comment prose from a path glob. The rest of this rule is enforced by review.
|
||||
|
||||
### Standing Rule: A Behavior Change Owns Every Test That Asserts The Old Behavior
|
||||
|
||||
**Changing behavior is not done until the tests that encoded the old behavior are updated or deleted — in the same change.** This is not the same rule as "keep the tests green": targeted verification runs the tests for the files you touched, and the stale assertions are almost always in files you did not touch, so a green targeted run is not evidence that no test still encodes the behavior you just changed.
|
||||
|
||||
@@ -14,8 +14,8 @@
|
||||
"type": "module",
|
||||
"packageManager": "pnpm@10.33.0",
|
||||
"scripts": {
|
||||
"pretest": "node scripts/check-no-nohup.mjs && node scripts/check-no-cwd-relative-dashboard-test-reads.mjs && node scripts/check-no-kill-4040.mjs && node scripts/check-no-getdatabase.mjs && node scripts/check-prerebase-inert.mjs && node scripts/check-capacity-pool-id.mjs && node scripts/check-cli-runtime-routing.mjs && node scripts/check-no-node-only-core-imports-in-dashboard.mjs && node scripts/check-pi-versions-pinned.mjs && node scripts/check-workspace-package-graph.mjs && node scripts/check-no-test-timeout-appeasement.mjs && node scripts/check-changeset-format.mjs && node scripts/check-pre-json-anchor.mjs && node scripts/check-routes-modular.mjs && node scripts/check-runtime-skill-loader-drift.mjs",
|
||||
"pretest:full": "node scripts/check-no-nohup.mjs && node scripts/check-no-cwd-relative-dashboard-test-reads.mjs && node scripts/check-no-kill-4040.mjs && node scripts/check-no-getdatabase.mjs && node scripts/check-prerebase-inert.mjs && node scripts/check-capacity-pool-id.mjs && node scripts/check-cli-runtime-routing.mjs && node scripts/check-no-node-only-core-imports-in-dashboard.mjs && node scripts/check-pi-versions-pinned.mjs && node scripts/check-workspace-package-graph.mjs && node scripts/check-no-test-timeout-appeasement.mjs && node scripts/check-changeset-format.mjs && node scripts/check-routes-modular.mjs && node scripts/check-runtime-skill-loader-drift.mjs",
|
||||
"pretest": "node scripts/check-no-nohup.mjs && node scripts/check-no-cwd-relative-dashboard-test-reads.mjs && node scripts/check-no-kill-4040.mjs && node scripts/check-no-getdatabase.mjs && node scripts/check-prerebase-inert.mjs && node scripts/check-capacity-pool-id.mjs && node scripts/check-cli-runtime-routing.mjs && node scripts/check-no-node-only-core-imports-in-dashboard.mjs && node scripts/check-pi-versions-pinned.mjs && node scripts/check-workspace-package-graph.mjs && node scripts/check-no-test-timeout-appeasement.mjs && node scripts/check-no-comment-assertions-in-tests.mjs && node scripts/check-changeset-format.mjs && node scripts/check-pre-json-anchor.mjs && node scripts/check-routes-modular.mjs && node scripts/check-runtime-skill-loader-drift.mjs",
|
||||
"pretest:full": "node scripts/check-no-nohup.mjs && node scripts/check-no-cwd-relative-dashboard-test-reads.mjs && node scripts/check-no-kill-4040.mjs && node scripts/check-no-getdatabase.mjs && node scripts/check-prerebase-inert.mjs && node scripts/check-capacity-pool-id.mjs && node scripts/check-cli-runtime-routing.mjs && node scripts/check-no-node-only-core-imports-in-dashboard.mjs && node scripts/check-pi-versions-pinned.mjs && node scripts/check-workspace-package-graph.mjs && node scripts/check-no-test-timeout-appeasement.mjs && node scripts/check-no-comment-assertions-in-tests.mjs && node scripts/check-changeset-format.mjs && node scripts/check-routes-modular.mjs && node scripts/check-runtime-skill-loader-drift.mjs",
|
||||
"check:line-count": "node scripts/check-file-line-count.mjs",
|
||||
"check:routes-modular": "node scripts/check-routes-modular.mjs",
|
||||
"check:changesets": "node scripts/check-changeset-format.mjs",
|
||||
@@ -32,7 +32,7 @@
|
||||
"census:lifecycle-columns": "node scripts/lifecycle-column-census.mjs",
|
||||
"check:quarantine-ledger": "node scripts/check-quarantine-ledger.mjs --strict",
|
||||
"check:mock-completeness": "node scripts/check-mock-completeness.mjs",
|
||||
"test:gate:static": "node scripts/check-no-nohup.mjs && node scripts/check-no-cwd-relative-dashboard-test-reads.mjs && node scripts/check-no-kill-4040.mjs && node scripts/check-no-getdatabase.mjs && node scripts/check-prerebase-inert.mjs && node scripts/check-capacity-pool-id.mjs && node scripts/check-cli-runtime-routing.mjs && node scripts/check-no-node-only-core-imports-in-dashboard.mjs && node scripts/check-pi-versions-pinned.mjs && node scripts/check-workspace-package-graph.mjs && node scripts/check-no-test-timeout-appeasement.mjs && node scripts/check-changeset-format.mjs && node scripts/check-mock-completeness.mjs && node scripts/check-inert-sync-lane-conversions.mjs && node scripts/check-runtime-skill-loader-drift.mjs",
|
||||
"test:gate:static": "node scripts/check-no-nohup.mjs && node scripts/check-no-cwd-relative-dashboard-test-reads.mjs && node scripts/check-no-kill-4040.mjs && node scripts/check-no-getdatabase.mjs && node scripts/check-prerebase-inert.mjs && node scripts/check-capacity-pool-id.mjs && node scripts/check-cli-runtime-routing.mjs && node scripts/check-no-node-only-core-imports-in-dashboard.mjs && node scripts/check-pi-versions-pinned.mjs && node scripts/check-workspace-package-graph.mjs && node scripts/check-no-test-timeout-appeasement.mjs && node scripts/check-no-comment-assertions-in-tests.mjs && node scripts/check-changeset-format.mjs && node scripts/check-mock-completeness.mjs && node scripts/check-inert-sync-lane-conversions.mjs && node scripts/check-runtime-skill-loader-drift.mjs",
|
||||
"test:gate": "node scripts/run-static-gate-checks.mjs && sh -c 'pnpm --filter @fusion/engine test:core & engine_pid=$!; pnpm --filter @fusion/core test:pg-gate & pg_pid=$!; pnpm --filter @fusion/core test:unit-gate & unit_pid=$!; status=0; wait $engine_pid || status=1; wait $pg_pid || status=1; wait $unit_pid || status=1; exit $status' && pnpm --filter @runfusion/fusion test:ci-shape",
|
||||
"pg:test:up": "node scripts/pg-test-server.mjs up",
|
||||
"pg:test:status": "node scripts/pg-test-server.mjs status",
|
||||
|
||||
@@ -97,8 +97,6 @@ describe("Grok CLI PluginRunner host wiring", () => {
|
||||
expect(mergeFnIndex).toBeGreaterThanOrEqual(0);
|
||||
const nextExport = source.indexOf("export async function runTaskAttach", mergeFnIndex);
|
||||
const mergeFnBody = source.slice(mergeFnIndex, nextExport > 0 ? nextExport : undefined);
|
||||
expect(mergeFnBody).toContain("FNXC:GrokCliRouting 2026-07-15-10:17");
|
||||
expect(mergeFnBody).toContain("Do not invent a full PluginRunner bootstrap");
|
||||
expect(mergeFnBody).not.toContain("mergePluginRunner");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -178,6 +178,10 @@ If there are merge conflicts:
|
||||
5. Run \`git add <file>\` for each resolved file
|
||||
6. Do NOT change anything beyond what's needed to resolve the conflict`,
|
||||
},
|
||||
/*
|
||||
FNXC:PromptOverrides 2026-08-23-23:25:
|
||||
THIS defaultContent is the prompt agent generation actually ships: `resolvePrompt` returns it whenever no operator override exists, so the identical literal exported as `AGENT_GENERATION_SYSTEM_PROMPT` in the dashboard is only the fallback-of-a-fallback. FN-021 added the xhigh/max thinking levels to that copy alone, which left every real generation run advertising levels that stop at "high". Any edit to one copy must be made to both.
|
||||
*/
|
||||
"agent-generation-system": {
|
||||
key: "agent-generation-system",
|
||||
name: "Agent Generation System",
|
||||
@@ -202,7 +206,7 @@ You MUST respond with ONLY valid JSON (no markdown, no explanation):
|
||||
"role": "The most appropriate capability: triage | executor | reviewer | merger | scheduler | engineer | custom",
|
||||
"description": "A brief 1-2 sentence description of the agent's purpose and expertise",
|
||||
"systemPrompt": "A detailed markdown system prompt for the agent. This should be comprehensive and include:\\n- Role definition\\n- Core responsibilities\\n- Specific areas of expertise\\n- Behavioral guidelines\\n- Output format expectations\\n- Edge case handling instructions",
|
||||
"thinkingLevel": "off | minimal | low | medium | high",
|
||||
"thinkingLevel": "off | minimal | low | medium | high | xhigh | max",
|
||||
"maxTurns": 1000
|
||||
}
|
||||
|
||||
@@ -220,6 +224,7 @@ You MUST respond with ONLY valid JSON (no markdown, no explanation):
|
||||
- "low": For moderate complexity tasks
|
||||
- "medium": For complex analysis, code review, architecture decisions
|
||||
- "high": For critical decisions, security analysis, complex debugging
|
||||
- "xhigh" or "max": For the hardest problems when the selected model advertises that level
|
||||
|
||||
## Max Turns Guidelines
|
||||
- 5-10: Simple, focused tasks (quick reviews, status checks)
|
||||
|
||||
@@ -6300,6 +6300,8 @@
|
||||
"usedAutomaticallyIfThePrimaryDefaultModelHits": "Used automatically if the primary default model hits a retryable provider error like rate limiting or overload. No default — unset.",
|
||||
"useDefault": "Use default",
|
||||
"whenEnabledStartupFetchesTheLatestAvailableModels": " When enabled, startup fetches the latest available models from the OpenRouter API so model pickers always include the newest catalog. Default: enabled. ",
|
||||
"syncOrcaRouterModelListAtStartup": " Sync OrcaRouter model list at startup ",
|
||||
"whenEnabledStartupFetchesTheLatestOrcaRouterModels": " When enabled, startup fetches the latest available models from the OrcaRouter API so model pickers include the OrcaRouter catalog. Default: enabled. ",
|
||||
"whenEnabledStartupRefreshesModelsThroughTheLocal": " When enabled, startup refreshes models through the local ",
|
||||
"commaSeparatedValuesSentToOpenRouterModelSyncOutputModalities": "Comma-separated values sent to OpenRouter model sync. No default — unset (unfiltered).",
|
||||
"openRouterRoutingOrderHint": "No default — unset (OpenRouter's own default routing order applies).",
|
||||
|
||||
93
scripts/check-no-comment-assertions-in-tests.mjs
Normal file
93
scripts/check-no-comment-assertions-in-tests.mjs
Normal file
@@ -0,0 +1,93 @@
|
||||
#!/usr/bin/env node
|
||||
/*
|
||||
FNXC:TestHygiene 2026-08-23-23:50:
|
||||
TESTS ASSERT BEHAVIOR, NEVER COMMENT TEXT. A test that pins an `FNXC:` block, a date stamp, or any
|
||||
comment prose from a source/CSS file guards documentation instead of behavior — and this repo's own
|
||||
convention tells authors to keep those comments updated as requirements change, so the two rules
|
||||
fight and the test loses in the worst possible way.
|
||||
|
||||
Measured 2026-08-23: a commit legitimately rewrote `runTaskMerge` and dropped its FNXC block;
|
||||
`grok-runtime-bootstrap.test.ts` went red because it asserted that comment existed, and the "fix"
|
||||
was to RE-ADD THE COMMENT to the product file. A comment returned to the shipped source not because
|
||||
it documented anything true, but to appease a test. Four more such assertions existed across the
|
||||
dashboard's CSS tests, each sitting beside a real assertion and adding nothing.
|
||||
|
||||
Scope is deliberately narrow. This bans asserting PROSE. Guards that scan source for CODE CONSTRUCTS
|
||||
or call-site allowlists — engine-no-blocking-shellout, vi-mock specifier resolution, durable-write
|
||||
and emit-surface inventories, legacy tombstones — are a different category, are mandated elsewhere
|
||||
in AGENTS.md, and are untouched here.
|
||||
|
||||
If a pinned comment was standing in for a real invariant, assert the invariant: the rendered style,
|
||||
the observable outcome, the absent construct. If nothing behavioral is behind it, delete the line.
|
||||
*/
|
||||
import { readFileSync } from "node:fs";
|
||||
import { spawnSync } from "node:child_process";
|
||||
import { fileURLToPath } from "node:url";
|
||||
|
||||
const SCAN_ROOTS = ["packages", "plugins"];
|
||||
const TEST_FILE_PATTERN = /\.test\.(?:ts|tsx|mts|cts|mjs|cjs|js|jsx)$/;
|
||||
/*
|
||||
An assertion matcher applied to a string carrying an FNXC stamp.
|
||||
|
||||
DELIBERATELY NARROW: an earlier draft also flagged `/*` and `*/` as comment delimiters and produced
|
||||
24 false positives and zero true ones - every hit was a path glob or a legitimate pattern
|
||||
assertion, which a regex cannot tell apart from comment prose. Comment prose is not reliably distinguishable from
|
||||
data by regex, so this check enforces the one unambiguous, observed case and the standing rule in
|
||||
AGENTS.md covers the rest for human and agent reviewers.
|
||||
*/
|
||||
const ASSERTION_PATTERN =
|
||||
/\.(?:toContain|toMatch|toContainEqual|toHaveTextContent)\s*\(\s*[`"'][^`"']*FNXC:/;
|
||||
|
||||
function isTestFile(filePath) {
|
||||
return TEST_FILE_PATTERN.test(filePath);
|
||||
}
|
||||
|
||||
function listTrackedTestFiles() {
|
||||
const result = spawnSync("git", ["ls-files", "--", ...SCAN_ROOTS], {
|
||||
encoding: "utf8",
|
||||
stdio: ["ignore", "pipe", "pipe"],
|
||||
});
|
||||
if (result.status !== 0) throw new Error(result.stderr?.trim() || "git ls-files failed");
|
||||
return result.stdout.split("\n").map((line) => line.trim()).filter(isTestFile);
|
||||
}
|
||||
|
||||
export function scanTrackedFiles(files = listTrackedTestFiles()) {
|
||||
const matches = [];
|
||||
for (const filePath of files) {
|
||||
let source;
|
||||
try {
|
||||
source = readFileSync(filePath, "utf8");
|
||||
} catch {
|
||||
continue;
|
||||
}
|
||||
if (!source.includes("FNXC:")) continue;
|
||||
source.split("\n").forEach((line, index) => {
|
||||
if (ASSERTION_PATTERN.test(line)) {
|
||||
matches.push({ filePath, lineNumber: index + 1, line });
|
||||
}
|
||||
});
|
||||
}
|
||||
return matches;
|
||||
}
|
||||
|
||||
export function formatFailureMessage(matches) {
|
||||
return [
|
||||
"[check-no-comment-assertions-in-tests] a test asserts COMMENT TEXT instead of behavior.",
|
||||
"Comments are documentation: AGENTS.md asks authors to keep FNXC blocks current, so pinning one guarantees a future false failure —",
|
||||
"and the tempting 'fix' is to re-add a comment to product source purely to satisfy the test (observed 2026-08-23).",
|
||||
"Assert the behavior instead — the rendered style, the observable outcome, the absent code construct — or delete the assertion if nothing behavioral is behind it.",
|
||||
"Code-construct and call-site-allowlist guards are a different category and are not affected by this check.",
|
||||
...matches.map(({ filePath, lineNumber, line }) => `${filePath}:${lineNumber}: ${line.trim()}`),
|
||||
].join("\n");
|
||||
}
|
||||
|
||||
export function main() {
|
||||
const matches = scanTrackedFiles();
|
||||
if (matches.length === 0) return 0;
|
||||
console.error(formatFailureMessage(matches));
|
||||
return 1;
|
||||
}
|
||||
|
||||
if (process.argv[1] && fileURLToPath(import.meta.url) === process.argv[1]) {
|
||||
process.exitCode = main();
|
||||
}
|
||||
Reference in New Issue
Block a user