Repository navigation
Decouple nf-benchmark from the live repo: isolation + pinned SUT + self-contained fixtures - #4
Merged
Merged
Conversation
…tate live repo) The serial benchmark path mutated the --project-root (the live nForma/QGSD checkout) in place and relied on a PARTIAL snapshot/restore (.planning/formal + docs + guard dirs only) — so anything nf-solve wrote elsewhere was not restored, and the project's Stop hook could auto-commit a seeded mutation into the working repo. Now each serial challenge runs in its own createIsolatedRoot() temp dir (mirroring --parallel) and is cleaned up after, so the live repo is never touched. Also fix the `test` script (`node --test test/` mis-resolved as a module on Node ≥20) → `node --test test/*.test.cjs`. Note: the suite has pre-existing flaky failures in the results/-dir loadResults/saveResult tests (non-deterministic, 2–4 vary per run), unrelated to this change — a symptom of the committed results/ bloat the redesign removes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
(2) SUT pinning: resolveSut() resolves nf-solve explicitly with provenance —
--sut <path> / NF_SUT (pinned) → installed ~/.claude/nf-bin copy → dev checkout
(with a loud "results NOT version-pinned" warning). The resolved bin+source+version
is printed at startup and recorded on each solve result (sut_source/sut_version), so
benchmark scores become reproducible and comparable across nForma versions. A
non-existent pin throws (no silent fallback). --sut wired into the CLI + propagated
via env so worker threads inherit it.
Isolation write-through fix: createIsolatedRoot() previously deep-copied only
.planning and SYMLINKED everything else — so a mutation/solve touching bin, hooks,
docs, templates or scripts wrote THROUGH the symlink into the real project repo.
Now COPY_DIRS{.planning,bin,hooks,docs,templates,scripts} are deep-copied; only
read-only entries are symlinked; .git is skipped. This is what makes the serial-path
isolation from the previous commit actually safe.
Tests: 5/5 SUT-resolution unit tests; full suite 91/92 (1 pre-existing flaky
results-dir test, unrelated).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The decoupled benchmark core (quorum-confirmed direction). A fixture is a tiny COMPLETE project that embodies one defect plus a `verify` command that is its executable spec — with NO dependency on QGSD or any host repo, so it cannot rot when the product evolves and the SUT can never touch the dev's working tree. - lib/fixture-runner.cjs: copy project/ → temp dir → verify (expect FAIL) → run pinned SUT → verify (expect PASS) → discard. Pure scoring contract (fix_and_verify / no_regression); pluggable/pinnable SUT adapter (defaultSutRun → nf-solve). - fixtures/sort-ascending, fixtures/filter-threshold: reference exemplars (each verify genuinely fails on the seeded defect). - test/fixture-runner.test.cjs: end-to-end with a stub SUT — proves defect→repair discrimination, that a no-op SUT fails, and that the fixture SOURCE is never mutated (isolation). 6/6 green. - fixtures/README.md: format spec + scoring + migration plan (real-SUT adapter, 230 challenge migration, deprecate mutate-QGSD path, keep CLI/CI contract). Foundation only; bulk 230-challenge migration + real-SUT wiring are the documented follow-on. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughAdds a self-contained fixture-based benchmark framework with SUT pinning. ChangesFixture Framework and SUT Pinning
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)package.json┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.19][ERROR]: unable to find a config; path fixtures/orphaned-formal-model/project/package.json┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.21][ERROR]: unable to find a config; path 🔧 markdownlint-cli2 (0.22.1)fixtures/README.mdmarkdownlint-cli2 wrapper config was not available before execution Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Automated commit from nf-solve — includes layer manifests, gate results, evidence snapshots, model registry, and requirements coverage updates.
…n a self-contained fixture (b) Resolves the open design question. nf-solve is residual/formal-driven (no "repair a file until a test passes" mode), so the real self-contained fixture is a MINIMAL nForma project + a seeded layer gap, scored by residual reduction: - measureResidual() runs `nf-solve --report-only --json` (fast, no LLM) and sums the residual_vector; scoring is target_layer-aware (score one seeded gap, not every layer). - fixtures/req-coverage-gap: a minimal nForma project with an uncovered REQ-01. VALIDATED LIVE: nf-solve detects r_to_f residual = 1 on it. - runFixture supports method "residual_reduction"; the repair half (drive residual→0 via a full solve) is gated behind RUN_LIVE_SOLVE / an injected sutRun, since it needs the live quorum toolchain. Detection alone is fast + CI-able (test skips cleanly if no SUT). Tests: 7/7 fixture-runner (incl. live residual detection), 5/5 SUT-resolution. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
fixtures/README.md (1)
17-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifier to fenced code block to silence markdownlint.
Line 18 opens a fenced block without a language tag. Add
textortree:
fixtures/<name>/ fixture.json # manifest project/ # a tiny COMPLETE project embodying ONE defect <code under test> verify.cjs # the executable spec: exit 0 iff fixed, nonzero while broken<details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@fixtures/README.mdaround lines 17 - 24, The fenced code block in the
Fixture format section is missing a language specifier, which triggers
markdownlint. Update the README’s fenced block near the Fixture format example
to use a tagged fence such as text or tree, keeping the existing content
unchanged; the relevant symbol to locate is the Fixture format example block in
fixtures/README.md.</details> <!-- cr-comment:v1:de8087d4b5ea491daceac77b --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>lib/fixture-runner.cjs (2)</summary><blockquote> `57-64`: _🩺 Stability & Availability_ | _🔵 Trivial_ | _⚡ Quick win_ **`defaultSutRun` ignores `spawnSync` return value, masking SUT crashes.** `spawnSync` is called but its status/stdout/stderr are discarded. If `nf-solve` exits non-zero or is killed by signal, the runner has no visibility and will proceed to score as if the SUT ran successfully. The `try/catch` in `runFixture` only catches synchronous throws, not process failures. Consider checking `r.status` and `r.signal` and surfacing them: ```diff function defaultSutRun(tempProjectDir, manifest, opts) { const { resolveSut } = require('./runner.cjs'); const sut = resolveSut(opts.projectRoot || tempProjectDir, opts); const args = ['--json', '--no-timeout']; if (manifest.scoring && manifest.scoring.target_layer) args.push(`--focus=${manifest.scoring.target_layer}`); - spawnSync('node', [sut.bin, ...args], { cwd: tempProjectDir, encoding: 'utf8', timeout: (opts.timeout || 600) * 1000, maxBuffer: 50 * 1024 * 1024 }); - return { source: sut.source, version: sut.version }; + const r = spawnSync('node', [sut.bin, ...args], { cwd: tempProjectDir, encoding: 'utf8', timeout: (opts.timeout || 600) * 1000, maxBuffer: 50 * 1024 * 1024 }); + if (r.status !== 1 && r.status !== 0) { + // nf-solve may exit 1 for residual-detected (expected), but other non-zero is error + throw new Error(`SUT exited ${r.status || r.signal || 'unknown'}: ${(r.stderr || '').slice(0, 500)}`); + } + return { source: sut.source, version: sut.version, exitCode: r.status }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/fixture-runner.cjs` around lines 57 - 64, defaultSutRun currently ignores the result of spawnSync, so SUT process failures are never surfaced. Update defaultSutRun to capture the spawnSync return value, then check its status and signal; if nf-solve exits non-zero or is terminated, throw or otherwise report a failure using the captured stdout/stderr. Keep the fix localized to defaultSutRun and the runFixture flow that depends on it so crashes are not masked as successful runs.
68-87: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
measureResidualtreats SUT crashes the same as "no residual detected".If
nf-solve --report-onlyexits non-zero or is killed,r.stdoutmay be empty or partial. The current error path returns{ total: null, layers: {}, has_residual: false }, whichrunFixtureinterprets as "no gap detected" rather than "measurement failed". This can mislabel a broken SUT as a fixture invalid.Add status validation before parsing:
function measureResidual(projectDir, opts = {}) { const { resolveSut } = require('./runner.cjs'); const sut = resolveSut(opts.projectRoot || projectDir, opts); const args = ['--report-only', '--json', '--fast', '--no-timeout', '--skip-tests', '--skip-proximity', '--skip-heatmap', '--no-coderlm']; const r = spawnSync('node', [sut.bin, ...args], { cwd: projectDir, encoding: 'utf8', timeout: (opts.reportTimeout || 120) * 1000, maxBuffer: 50 * 1024 * 1024 }); + if (r.error || (r.status !== 1 && r.status !== 0)) { + throw new Error(`measureResidual failed: SUT exited ${r.status || r.signal || 'unknown'}: ${(r.stderr || '').slice(0, 500)}`); + } const out = r.stdout || ''; const i = out.indexOf('{');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/fixture-runner.cjs` around lines 68 - 87, measureResidual currently collapses SUT failures into the same “no residual” result, so update it to distinguish crashes or timeouts from a valid empty measurement. In measureResidual, after spawnSync and before parsing stdout, check the child process status/termination info and if nf-solve --report-only exited non-zero, was signaled, or timed out, return a failure indicator that runFixture can treat as measurement error rather than has_residual false. Keep the existing JSON parsing and residual aggregation logic in measureResidual and preserve its use of resolveSut and spawnSync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/nf-benchmark.cjs`:
- Around line 292-303: The dry-run path in nf-benchmark should not depend on SUT
resolution; currently the top-level resolveSut call runs before the --dry-run
branch and can exit early when no SUT is installed. Move the dry-run handling in
bin/nf-benchmark.cjs ahead of the resolveSut/provenance block, so --dry-run only
lists selected challenges and skips SUT lookup entirely. Keep the existing
resolveSut logic only for non-dry-run execution, using the same resolveSut and
SUT logging code path.
- Around line 298-299: The SUT provenance is only being logged in the benchmark
flow, so it is lost from the saved artifacts. Update the benchmark result/report
creation path around resolveSut and the code that builds each saved entry so
sut_source and sut_version are persisted alongside the existing result data.
Make sure the challenge results and report rows written by nf-benchmark.cjs
carry these fields through from the sut object into every stored entry.
- Around line 298-306: The fallback from resolveSut() to the installed binary is
being rejected by the post-resolution validation in nf-benchmark.cjs. Update the
check around resolveSut() and the existing fs.existsSync(path.join(projectRoot,
'bin', 'nf-solve.cjs')) gate so it only errors when neither a local nf-solve.cjs
nor an explicitly selected SUT is available, and does not exit when resolveSut()
has already chosen an installed SUT. Keep the error path tied to the
nf-benchmark startup flow and the resolveSut()/opts.sut/NF_SUT decision points.
In `@lib/fixture-runner.cjs`:
- Line 106: The live-solve toggle in fixture-runner.cjs is using raw truthiness,
so RUN_LIVE_SOLVE values like "0" or "false" still enable solving unexpectedly.
Update the willSolve logic in the fixture runner to explicitly parse
RUN_LIVE_SOLVE as a boolean instead of relying on process.env.RUN_LIVE_SOLVE
truthiness, while preserving the existing opts.sutRun override behavior in the
same code path.
- Around line 49-53: The runVerify helper is splitting verifyCmd on whitespace,
which breaks quoted or escaped arguments and can cause spawnSync to invoke the
wrong command. Update runVerify to handle shell-style parsing properly, or
switch the spawnSync call to use shell execution as suggested, while preserving
cwd, encoding, and timeout behavior. Keep the fix localized to runVerify and the
parts/spawnSync logic so commands like quoted paths with spaces are passed
correctly.
In `@lib/runner.cjs`:
- Around line 203-218: resolveSut currently returns potentially relative SUT
paths for the pinned override and dev-checkout branches, which can point to the
wrong executable when callers spawn with a different cwd. Update resolveSut in
runner.cjs to normalize any returned bin path to an absolute path before
returning, covering the override, installed, and dev paths consistently. Keep
the existing version/source logic intact, but ensure callers of resolveSut
always receive an absolute path from resolveSut itself.
- Around line 238-247: The isolation setup in runner.cjs is symlinking every
non-excluded project entry by default, which allows writes to leak back into the
live repo. Update the project copy logic around the readdirSync/fs.symlinkSync
loop so the default behavior is a deep copy rather than a symlink, and reserve
symlinks only for explicitly immutable or cache-like paths; keep COPY_DIRS
handling intact and ensure the tempDir contents are fully isolated from
mutations.
In `@package.json`:
- Line 10: The npm test script uses a bare glob with node --test, which relies
on shell expansion and can behave inconsistently across platforms. Update the
package.json test entry to point at the test directory target instead, using the
existing test suite location so Node discovers the tests recursively and
portably. Locate the change in the package.json test script.
In `@test/fixture-runner.test.cjs`:
- Around line 58-65: The residual_reduction test is environment-dependent
because runFixture() will switch to the live-solve path when
process.env.RUN_LIVE_SOLVE is set. Update the test around the REQ_FIXTURE call
to temporarily save, clear, and then restore RUN_LIVE_SOLVE so this case always
stays in detection-only mode. Keep the assertions on runFixture,
residual_reduction, and passed === null so the contract remains deterministic.
---
Nitpick comments:
In `@fixtures/README.md`:
- Around line 17-24: The fenced code block in the Fixture format section is
missing a language specifier, which triggers markdownlint. Update the README’s
fenced block near the Fixture format example to use a tagged fence such as text
or tree, keeping the existing content unchanged; the relevant symbol to locate
is the Fixture format example block in fixtures/README.md.
In `@lib/fixture-runner.cjs`:
- Around line 57-64: defaultSutRun currently ignores the result of spawnSync, so
SUT process failures are never surfaced. Update defaultSutRun to capture the
spawnSync return value, then check its status and signal; if nf-solve exits
non-zero or is terminated, throw or otherwise report a failure using the
captured stdout/stderr. Keep the fix localized to defaultSutRun and the
runFixture flow that depends on it so crashes are not masked as successful runs.
- Around line 68-87: measureResidual currently collapses SUT failures into the
same “no residual” result, so update it to distinguish crashes or timeouts from
a valid empty measurement. In measureResidual, after spawnSync and before
parsing stdout, check the child process status/termination info and if nf-solve
--report-only exited non-zero, was signaled, or timed out, return a failure
indicator that runFixture can treat as measurement error rather than
has_residual false. Keep the existing JSON parsing and residual aggregation
logic in measureResidual and preserve its use of resolveSut and spawnSync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e43aff1-22e6-4119-955d-d1953d5dd8f4
📒 Files selected for processing (20)
bin/nf-benchmark.cjsfixtures/README.mdfixtures/filter-threshold/fixture.jsonfixtures/filter-threshold/project/filter.cjsfixtures/filter-threshold/project/verify.cjsfixtures/req-coverage-gap/fixture.jsonfixtures/req-coverage-gap/project/.planning/formal/model-registry.jsonfixtures/req-coverage-gap/project/.planning/formal/requirements.jsonfixtures/req-coverage-gap/project/.planning/formal/solve-sessions/solve-session-2026-06-29T10-41-45Z.mdfixtures/req-coverage-gap/project/.planning/formal/solve-state.jsonfixtures/req-coverage-gap/project/.planning/formal/solve-trend.jsonlfixtures/req-coverage-gap/project/package.jsonfixtures/sort-ascending/fixture.jsonfixtures/sort-ascending/project/sort.cjsfixtures/sort-ascending/project/verify.cjslib/fixture-runner.cjslib/runner.cjspackage.jsontest/fixture-runner.test.cjstest/sut-resolution.test.cjs
Comment on lines
+292
to
+303
| // SUT pinning: --sut <path> (or NF_SUT) pins the nf-solve under test so scores are | ||
| // reproducible and comparable across nForma versions. Propagated via env so worker | ||
| // threads inherit it. Resolve+print provenance once up front. | ||
| if (opts.sut) process.env.NF_SUT = opts.sut; | ||
| const { resolveSut } = require(path.join(__dirname, '..', 'lib', 'runner.cjs')); | ||
| try { | ||
| const sut = resolveSut(projectRoot, {}); | ||
| console.log(`SUT: ${sut.bin} [source=${sut.source}, version=${sut.version}]`); | ||
| } catch (e) { | ||
| console.error(`Error resolving SUT: ${e.message}`); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don’t require SUT resolution for --dry-run.
--dry-run only lists selected challenges, but this now exits if no SUT is installed or pinned. Move the dry-run branch before resolving the SUT.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/nf-benchmark.cjs` around lines 292 - 303, The dry-run path in
nf-benchmark should not depend on SUT resolution; currently the top-level
resolveSut call runs before the --dry-run branch and can exit early when no SUT
is installed. Move the dry-run handling in bin/nf-benchmark.cjs ahead of the
resolveSut/provenance block, so --dry-run only lists selected challenges and
skips SUT lookup entirely. Keep the existing resolveSut logic only for
non-dry-run execution, using the same resolveSut and SUT logging code path.
Comment on lines
+298
to
+299
| const sut = resolveSut(projectRoot, {}); | ||
| console.log(`SUT: ${sut.bin} [source=${sut.source}, version=${sut.version}]`); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist SUT provenance in benchmark artifacts.
Printing source/version to stdout loses the provenance after the run; the returned challenge results and report rows still omit it. Store sut_source and sut_version in each saved result/report entry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/nf-benchmark.cjs` around lines 298 - 299, The SUT provenance is only
being logged in the benchmark flow, so it is lost from the saved artifacts.
Update the benchmark result/report creation path around resolveSut and the code
that builds each saved entry so sut_source and sut_version are persisted
alongside the existing result data. Make sure the challenge results and report
rows written by nf-benchmark.cjs carry these fields through from the sut object
into every stored entry.
Comment on lines
+298
to
+306
| const sut = resolveSut(projectRoot, {}); | ||
| console.log(`SUT: ${sut.bin} [source=${sut.source}, version=${sut.version}]`); | ||
| } catch (e) { | ||
| console.error(`Error resolving SUT: ${e.message}`); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| if (!fs.existsSync(path.join(projectRoot, 'bin', 'nf-solve.cjs')) && !(opts.sut || process.env.NF_SUT)) { | ||
| console.error(`Error: ${projectRoot} does not appear to be a valid nForma project (and no --sut/NF_SUT pin given)`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t reject the installed SUT fallback.
If resolveSut() selects the installed binary without --sut/NF_SUT, Line 305 still exits when projectRoot/bin/nf-solve.cjs is absent, making the installed fallback unusable.
Proposed fix
+ let sut;
try {
- const sut = resolveSut(projectRoot, {});
+ sut = resolveSut(projectRoot, {});
console.log(`SUT: ${sut.bin} [source=${sut.source}, version=${sut.version}]`);
} catch (e) {
console.error(`Error resolving SUT: ${e.message}`);
process.exit(1);
}
- if (!fs.existsSync(path.join(projectRoot, 'bin', 'nf-solve.cjs')) && !(opts.sut || process.env.NF_SUT)) {
- console.error(`Error: ${projectRoot} does not appear to be a valid nForma project (and no --sut/NF_SUT pin given)`);
+ if (!fs.existsSync(projectRoot)) {
+ console.error(`Error: project root not found: ${projectRoot}`);
process.exit(1);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/nf-benchmark.cjs` around lines 298 - 306, The fallback from resolveSut()
to the installed binary is being rejected by the post-resolution validation in
nf-benchmark.cjs. Update the check around resolveSut() and the existing
fs.existsSync(path.join(projectRoot, 'bin', 'nf-solve.cjs')) gate so it only
errors when neither a local nf-solve.cjs nor an explicitly selected SUT is
available, and does not exit when resolveSut() has already chosen an installed
SUT. Keep the error path tied to the nf-benchmark startup flow and the
resolveSut()/opts.sut/NF_SUT decision points.
Comment on lines
+49
to
+53
| function runVerify(cwd, verifyCmd) { | ||
| const parts = verifyCmd.split(/\s+/); | ||
| const r = spawnSync(parts[0], parts.slice(1), { cwd, encoding: 'utf8', timeout: 60000 }); | ||
| return r.status === null ? -1 : r.status; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
runVerify splits on whitespace without shell parsing, breaking quoted arguments.
verifyCmd.split(/\s+/) does not handle quoted strings or escaped spaces. A verify command like node "path with spaces/verify.cjs" would split incorrectly, causing spawnSync to fail.
function runVerify(cwd, verifyCmd) {
- const parts = verifyCmd.split(/\s+/);
+ // Use shell:true for simple commands, or a minimal shell-quote parser
+ // for complex cases. For now, document the limitation or use shell:true:
+ const args = require('child_process').execSync ?
+ { shell: true, command: verifyCmd } :
+ // fallback for spawnSync:
+ { cmd: verifyCmd.split(/\s+/) };
+ // Or more simply, just use shell:true:
- const r = spawnSync(parts[0], parts.slice(1), { cwd, encoding: 'utf8', timeout: 60000 });
+ const r = spawnSync(verifyCmd, [], { cwd, encoding: 'utf8', timeout: 60000, shell: true });
return r.status === null ? -1 : r.status;
}Or keep it simple and just use shell: true:
function runVerify(cwd, verifyCmd) {
- const parts = verifyCmd.split(/\s+/);
- const r = spawnSync(parts[0], parts.slice(1), { cwd, encoding: 'utf8', timeout: 60000 });
+ const r = spawnSync(verifyCmd, [], { cwd, encoding: 'utf8', timeout: 60000, shell: true });
return r.status === null ? -1 : r.status;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function runVerify(cwd, verifyCmd) { | |
| const parts = verifyCmd.split(/\s+/); | |
| const r = spawnSync(parts[0], parts.slice(1), { cwd, encoding: 'utf8', timeout: 60000 }); | |
| return r.status === null ? -1 : r.status; | |
| } | |
| function runVerify(cwd, verifyCmd) { | |
| const r = spawnSync(verifyCmd, [], { cwd, encoding: 'utf8', timeout: 60000, shell: true }); | |
| return r.status === null ? -1 : r.status; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/fixture-runner.cjs` around lines 49 - 53, The runVerify helper is
splitting verifyCmd on whitespace, which breaks quoted or escaped arguments and
can cause spawnSync to invoke the wrong command. Update runVerify to handle
shell-style parsing properly, or switch the spawnSync call to use shell
execution as suggested, while preserving cwd, encoding, and timeout behavior.
Keep the fix localized to runVerify and the parts/spawnSync logic so commands
like quoted paths with spaces are passed correctly.
| const detected = typeof preMetric === 'number' && preMetric > 0; | ||
| // The repair half needs a live quorum solve; gate it behind RUN_LIVE_SOLVE (or | ||
| // an explicit sutRun stub in tests). Detection alone is fast and needs no LLM. | ||
| const willSolve = opts.sutRun || process.env.RUN_LIVE_SOLVE; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
RUN_LIVE_SOLVE env check is boolean-broken: string "0" is truthy.
const willSolve = opts.sutRun || process.env.RUN_LIVE_SOLVE;If RUN_LIVE_SOLVE="0" or RUN_LIVE_SOLVE="false", the string is truthy and the runner will attempt a live solve unexpectedly. Use explicit boolean parsing:
- const willSolve = opts.sutRun || process.env.RUN_LIVE_SOLVE;
+ const willSolve = opts.sutRun || /^(1|true|yes)$/i.test(process.env.RUN_LIVE_SOLVE || '');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/fixture-runner.cjs` at line 106, The live-solve toggle in
fixture-runner.cjs is using raw truthiness, so RUN_LIVE_SOLVE values like "0" or
"false" still enable solving unexpectedly. Update the willSolve logic in the
fixture runner to explicitly parse RUN_LIVE_SOLVE as a boolean instead of
relying on process.env.RUN_LIVE_SOLVE truthiness, while preserving the existing
opts.sutRun override behavior in the same code path.
Comment on lines
238
to
+247
| const entries = fs.readdirSync(projectRoot, { withFileTypes: true }); | ||
| for (const entry of entries) { | ||
| if (entry.name === '.planning' || entry.name === '.git') continue; | ||
| const srcPath = path.join(projectRoot, entry.name); | ||
| const destPath = path.join(tempDir, entry.name); | ||
| fs.symlinkSync(srcPath, destPath); | ||
| if (entry.name === '.git' || COPY_DIRS.has(entry.name)) continue; | ||
| fs.symlinkSync(path.join(projectRoot, entry.name), path.join(tempDir, entry.name)); | ||
| } | ||
|
|
||
| // Deep copy .planning directory | ||
| const planningDir = path.join(projectRoot, '.planning'); | ||
| if (fs.existsSync(planningDir)) { | ||
| fs.cpSync(planningDir, path.join(tempDir, '.planning'), { recursive: true }); | ||
| for (const dirName of COPY_DIRS) { | ||
| const srcDir = path.join(projectRoot, dirName); | ||
| if (fs.existsSync(srcDir)) { | ||
| fs.cpSync(srcDir, path.join(tempDir, dirName), { recursive: true }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Avoid symlink-by-default isolation.
Any mutation or solver write under a symlinked entry outside COPY_DIRS still writes through to the live repo. Deep-copy by default, or only symlink explicitly immutable/cache paths.
Proposed safer default
- // Symlink read-only entries; deep-copy the mutable ones; skip .git entirely.
+ // Deep-copy project entries; skip .git entirely.
const entries = fs.readdirSync(projectRoot, { withFileTypes: true });
for (const entry of entries) {
- if (entry.name === '.git' || COPY_DIRS.has(entry.name)) continue;
- fs.symlinkSync(path.join(projectRoot, entry.name), path.join(tempDir, entry.name));
- }
- for (const dirName of COPY_DIRS) {
- const srcDir = path.join(projectRoot, dirName);
- if (fs.existsSync(srcDir)) {
- fs.cpSync(srcDir, path.join(tempDir, dirName), { recursive: true });
- }
+ if (entry.name === '.git') continue;
+ fs.cpSync(
+ path.join(projectRoot, entry.name),
+ path.join(tempDir, entry.name),
+ { recursive: true }
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const entries = fs.readdirSync(projectRoot, { withFileTypes: true }); | |
| for (const entry of entries) { | |
| if (entry.name === '.planning' || entry.name === '.git') continue; | |
| const srcPath = path.join(projectRoot, entry.name); | |
| const destPath = path.join(tempDir, entry.name); | |
| fs.symlinkSync(srcPath, destPath); | |
| if (entry.name === '.git' || COPY_DIRS.has(entry.name)) continue; | |
| fs.symlinkSync(path.join(projectRoot, entry.name), path.join(tempDir, entry.name)); | |
| } | |
| // Deep copy .planning directory | |
| const planningDir = path.join(projectRoot, '.planning'); | |
| if (fs.existsSync(planningDir)) { | |
| fs.cpSync(planningDir, path.join(tempDir, '.planning'), { recursive: true }); | |
| for (const dirName of COPY_DIRS) { | |
| const srcDir = path.join(projectRoot, dirName); | |
| if (fs.existsSync(srcDir)) { | |
| fs.cpSync(srcDir, path.join(tempDir, dirName), { recursive: true }); | |
| } | |
| // Deep-copy project entries; skip .git entirely. | |
| const entries = fs.readdirSync(projectRoot, { withFileTypes: true }); | |
| for (const entry of entries) { | |
| if (entry.name === '.git') continue; | |
| fs.cpSync( | |
| path.join(projectRoot, entry.name), | |
| path.join(tempDir, entry.name), | |
| { recursive: true } | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/runner.cjs` around lines 238 - 247, The isolation setup in runner.cjs is
symlinking every non-excluded project entry by default, which allows writes to
leak back into the live repo. Update the project copy logic around the
readdirSync/fs.symlinkSync loop so the default behavior is a deep copy rather
than a symlink, and reserve symlinks only for explicitly immutable or cache-like
paths; keep COPY_DIRS handling intact and ensure the tempDir contents are fully
isolated from mutations.
Comment on lines
+58
to
+65
| test('runFixture (residual_reduction): nf-solve detects the seeded r_to_f gap', { skip: !sutAvailable }, () => { | ||
| const r = runFixture(REQ_FIXTURE, {}); // no sutRun + no RUN_LIVE_SOLVE → detection only | ||
| assert.equal(r.method, 'residual_reduction'); | ||
| assert.equal(r.target_layer, 'r_to_f'); | ||
| assert.equal(r.detected, true, 'nf-solve must see a positive r_to_f residual for the uncovered requirement'); | ||
| assert.ok(r.pre_residual > 0); | ||
| assert.equal(r.passed, null, 'repair half is gated behind RUN_LIVE_SOLVE'); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Force detection-only mode in this test.
runFixture() switches to the live-solve path when process.env.RUN_LIVE_SOLVE is set, so this test becomes environment-dependent and can fail whenever that variable is exported. Save/clear it around the assertion so the passed === null contract is deterministic.
Suggested fix
test('runFixture (residual_reduction): nf-solve detects the seeded r_to_f gap', { skip: !sutAvailable }, () => {
- const r = runFixture(REQ_FIXTURE, {}); // no sutRun + no RUN_LIVE_SOLVE → detection only
- assert.equal(r.method, 'residual_reduction');
- assert.equal(r.target_layer, 'r_to_f');
- assert.equal(r.detected, true, 'nf-solve must see a positive r_to_f residual for the uncovered requirement');
- assert.ok(r.pre_residual > 0);
- assert.equal(r.passed, null, 'repair half is gated behind RUN_LIVE_SOLVE');
+ const prev = process.env.RUN_LIVE_SOLVE;
+ delete process.env.RUN_LIVE_SOLVE;
+ try {
+ const r = runFixture(REQ_FIXTURE, {}); // no sutRun + no RUN_LIVE_SOLVE → detection only
+ assert.equal(r.method, 'residual_reduction');
+ assert.equal(r.target_layer, 'r_to_f');
+ assert.equal(r.detected, true, 'nf-solve must see a positive r_to_f residual for the uncovered requirement');
+ assert.ok(r.pre_residual > 0);
+ assert.equal(r.passed, null, 'repair half is gated behind RUN_LIVE_SOLVE');
+ } finally {
+ if (prev === undefined) delete process.env.RUN_LIVE_SOLVE;
+ else process.env.RUN_LIVE_SOLVE = prev;
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('runFixture (residual_reduction): nf-solve detects the seeded r_to_f gap', { skip: !sutAvailable }, () => { | |
| const r = runFixture(REQ_FIXTURE, {}); // no sutRun + no RUN_LIVE_SOLVE → detection only | |
| assert.equal(r.method, 'residual_reduction'); | |
| assert.equal(r.target_layer, 'r_to_f'); | |
| assert.equal(r.detected, true, 'nf-solve must see a positive r_to_f residual for the uncovered requirement'); | |
| assert.ok(r.pre_residual > 0); | |
| assert.equal(r.passed, null, 'repair half is gated behind RUN_LIVE_SOLVE'); | |
| }); | |
| test('runFixture (residual_reduction): nf-solve detects the seeded r_to_f gap', { skip: !sutAvailable }, () => { | |
| const prev = process.env.RUN_LIVE_SOLVE; | |
| delete process.env.RUN_LIVE_SOLVE; | |
| try { | |
| const r = runFixture(REQ_FIXTURE, {}); // no sutRun + no RUN_LIVE_SOLVE → detection only | |
| assert.equal(r.method, 'residual_reduction'); | |
| assert.equal(r.target_layer, 'r_to_f'); | |
| assert.equal(r.detected, true, 'nf-solve must see a positive r_to_f residual for the uncovered requirement'); | |
| assert.ok(r.pre_residual > 0); | |
| assert.equal(r.passed, null, 'repair half is gated behind RUN_LIVE_SOLVE'); | |
| } finally { | |
| if (prev === undefined) delete process.env.RUN_LIVE_SOLVE; | |
| else process.env.RUN_LIVE_SOLVE = prev; | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/fixture-runner.test.cjs` around lines 58 - 65, The residual_reduction
test is environment-dependent because runFixture() will switch to the live-solve
path when process.env.RUN_LIVE_SOLVE is set. Update the test around the
REQ_FIXTURE call to temporarily save, clear, and then restore RUN_LIVE_SOLVE so
this case always stays in detection-only mode. Keep the assertions on
runFixture, residual_reduction, and passed === null so the contract remains
deterministic.
…enchmark green
The previous commit made serial isolation the default, which broke the CI benchmark:
the legacy mutation-challenges are substrate-coupled — they measure residual deltas
against the live repo's exact state, so running them in an isolated COPY inflates
baselines ("no expected layer showed increased residual") and misses runtime-generated
files (ENOENT). That coupling is precisely what the self-contained fixture model
replaces; it cannot be transparently retrofitted onto the old challenges.
So serial isolation is now OPT-IN via --isolate (default = in-place). CI runs against a
throwaway ../nforma checkout where in-place mutation is harmless; --isolate protects a
LOCAL dev run against the developer's real working repo. Self-contained fixtures remain
always-isolated (the forward path). createIsolatedRoot's deep-copy fix is retained.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
bin/nf-benchmark.cjs (3)
293-303: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLet
--dry-runskip SUT resolution.
--dry-runonly lists the selected challenges, but this block can still exit first when no pinned/installed SUT is available. Move the dry-run return ahead of SUT resolution and project validation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/nf-benchmark.cjs` around lines 293 - 303, The SUT resolution block in nf-benchmark.cjs is running before the dry-run path, causing --dry-run to fail early when no pinned or installed SUT is available. Move the dry-run early return so it happens before project validation and before calling resolveSut(), keeping the dry-run flow limited to listing challenges only. Use the existing opts.dryRun handling and the resolveSut / projectRoot logic to place the guard in the correct spot.
293-300: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist the resolved SUT provenance in benchmark artifacts.
This only prints
source/version. The saved result/report rows later inrunBenchmark()still omitsut_sourceandsut_version, so reproducibility metadata is lost once stdout is gone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/nf-benchmark.cjs` around lines 293 - 300, The resolved SUT provenance is only being printed in nf-benchmark.cjs and is not persisted into benchmark artifacts, so runBenchmark() loses reproducibility metadata. Update the result/report row construction in runBenchmark() to carry through the resolved SUT fields from resolveSut, specifically sut_source and sut_version, alongside the existing SUT path/bin information. Use the existing resolveSut and benchmark row-writing logic to ensure these values are saved wherever other benchmark metadata is recorded.
298-307: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t reject
resolveSut()’s installed fallback.If
resolveSut()picks the installed binary, Lines 306-308 still fail unless the repo hasbin/nf-solve.cjsor the user explicitly pinned--sut/NF_SUT. That makes the installed fallback unusable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/nf-benchmark.cjs` around lines 298 - 307, The fallback from resolveSut() to an installed binary is being rejected by the project validation check. Update the nf-benchmark flow around resolveSut() and the subsequent bin/nf-solve.cjs existence guard so that when resolveSut() successfully returns an installed SUT (via its source/version fields), the script does not require a local bin/nf-solve.cjs or explicit --sut/NF_SUT pin. Keep the validation only for cases where no SUT was resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@bin/nf-benchmark.cjs`:
- Around line 293-303: The SUT resolution block in nf-benchmark.cjs is running
before the dry-run path, causing --dry-run to fail early when no pinned or
installed SUT is available. Move the dry-run early return so it happens before
project validation and before calling resolveSut(), keeping the dry-run flow
limited to listing challenges only. Use the existing opts.dryRun handling and
the resolveSut / projectRoot logic to place the guard in the correct spot.
- Around line 293-300: The resolved SUT provenance is only being printed in
nf-benchmark.cjs and is not persisted into benchmark artifacts, so
runBenchmark() loses reproducibility metadata. Update the result/report row
construction in runBenchmark() to carry through the resolved SUT fields from
resolveSut, specifically sut_source and sut_version, alongside the existing SUT
path/bin information. Use the existing resolveSut and benchmark row-writing
logic to ensure these values are saved wherever other benchmark metadata is
recorded.
- Around line 298-307: The fallback from resolveSut() to an installed binary is
being rejected by the project validation check. Update the nf-benchmark flow
around resolveSut() and the subsequent bin/nf-solve.cjs existence guard so that
when resolveSut() successfully returns an installed SUT (via its source/version
fields), the script does not require a local bin/nf-solve.cjs or explicit
--sut/NF_SUT pin. Keep the validation only for cases where no SUT was resolved.
Automated commit from nf-solve — includes layer manifests, gate results, evidence snapshots, model registry, and requirements coverage updates.
Automated commit from nf-solve — includes layer manifests, gate results, evidence snapshots, model registry, and requirements coverage updates.
The published @nforma.ai/nforma ships the whole bin/ (incl. bin/nf-solve.cjs), so the SUT can come entirely from npm. resolveSut now accepts `--sut npm:<pkg>@<ver>`: resolveNpmSut installs the pinned package once into ~/.cache/nf-benchmark-sut/<spec> (--ignore-scripts), and uses its bundled nf-solve.cjs. Combined with self-contained fixtures, the benchmark now runs from nf-benchmark + npm ALONE — it never needs the QGSD source. Proven live: `--sut npm:@nforma.ai/nforma@0.43.1` installs+resolves and detects the seeded r_to_f gap on req-coverage-gap with zero QGSD present. README documents the CI cutover (replace the ../nforma checkout with --sut npm:). Tests: 6/6 SUT (incl. npm: cache-hit), 7/7 fixture-runner. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bin/run-fixtures.cjs runs the self-contained fixture corpus against a pinned SUT: residual fixtures must DETECT their gap (nf-solve report-only), code fixtures must REPRODUCE their defect; --live additionally drives residual gaps to 0 (quorum needed). Adds runFixture reproduceOnly mode (defect-present check, no SUT) for the fast gate. .github/workflows/decoupled-fixtures.yml proves the FULLY decoupled path in CI: checkout nf-benchmark ONLY, install no host repo, run `run-fixtures --sut npm:@nforma.ai/nforma@<ver>`. No ../nforma checkout, version-pinned. This is the decoupled replacement for the legacy benchmark.yml (kept until the 230-challenge fixture migration completes). Proven locally: corpus 3/3 OK against npm:@nforma.ai/nforma@0.43.1 with zero QGSD. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…me pollution; demote legacy gate Three clean changes, no autocommit pollution: 1. orphaned-formal-model fixture (formal_lint layer) — a minimal nForma project whose OrphanSpec.tla has no backing requirement. The pinned npm SUT (@0.43.1) detects it on the formal_lint layer (residual 1), a distinct layer from req-coverage-gap's r_to_f, so the corpus now pins both controllable doc-only layers. README documents the layer-controllability constraint (doc-only minimal projects always carry nonzero l1_to_l3/l3_to_tc, so only r_to_f and formal_lint can discriminate a seeded defect). 2. Purge solve-runtime pollution from fixtures. This repo's own nForma hooks treat fixture project/.planning/formal/ dirs as real artifacts and autocommitted solve-state.json / solve-sessions/ / solve-trend.jsonl into req-coverage-gap. Those are NOT fixture content — a fixture's project/ must hold only the clean seeded-defect state — so they're removed and .gitignore'd to prevent recurrence. Corpus stays 4/4 (fixtures detect structurally, not via solve-state). 3. Demote the legacy coupled benchmark PR gate to continue-on-error (informational). It runs a non-deterministic LLM solver against ../nforma and compares to a cached baseline with a hardcoded zero-tolerance per-category gate; its stability category is structurally flaky (nf-solve non-idempotent on no-op runs, issue #5) and scores 0/15 on main itself. The deterministic decoupled "Fixture corpus vs npm SUT" job is the real PR gate now. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Decouple nf-benchmark from the live QGSD repo (quorum-confirmed direction)
Addresses the "too coupled / it mutates the product repo" concern. Quorum (codex-1 + copilot-1, unanimous APPROVE) confirmed the A+B+C direction: self-contained fixtures + pinned SUT + ephemeral isolation. Debate:
QGSD/.planning/quorum/debates/2026-06-29-nf-benchmark-architecture.md.What was wrong
--project-root(QGSD) in place with a partial snapshot/restore — anythingnf-solvewrote outside.planning/formal+docswasn't restored, and QGSD'snf-stophook could auto-commit a seeded mutation.createIsolatedRootsymlinkedbin/hooks/docs/templates/scripts→ a mutation there wrote through into the real repo.What this PR does
(1) Serial path now runs each challenge in its own ephemeral
createIsolatedRoottemp dir — never mutates the live repo. (+ fixed the brokennpm testinvocation.)(2)
resolveSut()— pin the SUT via--sut/NF_SUT(→ installed → dev-checkout with warning); provenance printed + recorded per result. Fixed the isolation write-through:bin/hooks/docs/templates/scriptsare now deep-copied, not symlinked.(3) Self-contained fixture architecture (
lib/fixture-runner.cjs+fixtures/): a fixture is a tiny complete project + averifyspec, with no QGSD dependency; runner copies it to a temp dir, runs the pinned SUT, checks verify, discards. Two exemplars + an e2e test (stub SUT) proving defect→repair discrimination and that the fixture source is never mutated. Migration plan infixtures/README.md.Tests
11/11 new unit/e2e tests pass (SUT resolution + fixture runner). Full suite green apart from pre-existing flaky
results/-dir tests (unrelated; a symptom of the results bloat the new model removes).Follow-on (documented, not in this PR)
Real-SUT adapter wiring, migrating the 230 mutation-challenges to self-contained fixtures (in waves), deprecating the mutate-QGSD path — all behind the unchanged
bin/nf-benchmark.cjsCLI + CI-gate contract.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation