chore: steer the no-mistakes Test step to diff-focused local validation - #2
Merged
Merged
Conversation
Add a trusted test.instructions policy to .no-mistakes.yaml directing the Test step to select and run only tests relevant to the branch diff via bin/fm-test-run.sh --changed, and to never walk the entire tests/ suite locally. GitHub CI already runs the complete deterministic regression suite on every push, so a redundant local full-suite walk only burns the local fix-round time budget. commands.test stays absent per the existing firstmate-coding-guidelines policy. Extend tests/fm-nm-test-contract.test.sh to parse the real config and assert test.instructions is set and steers toward the changed-file selector instead of a full suite walk, and cross-reference the sanctioned test.instructions surface from firstmate-coding-guidelines.
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
1 participant
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.
Intent
Approve configuring Firstmate's local validation to run only the relevant tests, leaving the complete test suite to GitHub, so the already-coded customization fixes do not repeatedly hit the 30-minute limit. Run the later customization validation without another heavy local validation competing for this machine.
What Changed
.no-mistakes.yamlnow carries atest.instructionspolicy that directs the Test step to judge and run only the tests exercising the branch's changed behavior - preferring the narrowest selection such as the single subject script the diff touches, treatingbin/fm-test-run.sh --changedas an optional upper bound rather than a required floor, and never walking the wholetests/suite locally.commands.teststays absent; full deterministic regression coverage remains with GitHub CI.tests/fm-nm-test-contract.test.shadds a contract asserting the parsed config exposes a non-emptytest.instructionsstring, and the file now skips both contracts with a message whenrubyis unavailable instead of failing on the missing parser.docs/configuration.mdand thefirstmate-coding-guidelinesskill document the new field, including thattest.instructionssteers an agent that executes code and so is honored only from the default-branch copy of.no-mistakes.yaml- the same trust propertycommands.testalready has.Risk Assessment
✅ Low: The change is a well-bounded config, documentation, and contract-test edit whose central claims (that
test.instructionsis a consumed, default-branch-trusted field, and that CI owns full regression coverage) I verified directly against the no-mistakes binary and the CI workflow, with no correctness defect found in the only executable logic it adds.Testing
I drove this change through firstmate's own test runner rather than reading it. The narrowest selection the new policy prescribes (
tests/fm-nm-test-contract.test.shplustests/fm-documentation-audiences.test.sh) completes in 0.45s against a 206-script inventory, with--changedresolving to 54 scripts as a genuine upper bound to narrow from - which is exactly the local time-budget relief the intent asks for. I reproduced the ruby regression end-to-end: the pre-fix version of the contract test hard-fails on this ruby-less host while the shipped version emitsskip:and the runner recordsgate_skip=true, exit 0. Because ruby is absent here, I provisioned a Debian container with ruby 3.3.8 and drove the ruby-present path adversarially - removing, emptying, and mistypingtest.instructionseach fail with the intended message, and re-pinningcommands.testto a full-suite walk fails the pre-existing guard, so the new assertion is not vacuous. The documentation-audiences test passes over the changed docs prose, and--check-coverageconfirms the CI lanes equal the full inventory, so leaving broad regression to GitHub is real. The change has no rendered UI surface - it is a YAML config field, a bash test, and prose - so the reviewer-visible evidence is CLI transcripts of the runner's actual output rather than screenshots. One scenario stayed untested: the no-mistakes Test step rendering the new field as its trusted live-validation runbook, which needs a default branch that already carries it. While driving the coverage proof I hit a pre-existing locale-dependent failure inbin/fm-test-run.sh --check-coverageunder en_US.UTF-8; I reported it instead of fixing it, since the fix is in a file this change does not touch.time bin/fm-test-run.sh tests/fm-nm-test-contract.test.sh tests/fm-documentation-audiences.test.sh-> FM_TEST_SUMMARY total=2 failed=0, 0.448s wall, vs--list --all= 206 scripts; evidence file 06…bin/fm-test-run.sh tests/fm-nm-test-contract.test.shon this ruby-less host ->skip: ruby not installed...,gate_skip=true, exit 0; evidence file 01-targeted-run-ruby-absent.txtnot ok - ruby is required to parse .no-mistakes.yaml for this contract, exit=1; shipped version -> exit=0; evi…bin/fm-test-run.sh tests/fm-nm-test-contract.test.sh-> each `not ok - test.instructions must be a non-empty str…commands.test: 'bin/fm-test-run.sh --all'injected ->not ok - commands.test must be absent or empty so Test stays intent-targeted; got: "bin/fm-test-run.sh --all", exit=1;…bin/fm-test-run.sh tests/fm-documentation-audiences.test.sh-> 4 ok assertions including local link resolution, exit 0; evidence file 05-docs-audience-behavior.txtLC_ALL=C bin/fm-test-run.sh --check-coverage(the exact command .github/workflows/ci.yml:61 runs) -> FM_TEST_COVERAGE ok total=206 across the 9 CI lanes; evidence file 04-ci-owns-full-coverage.txtEvidence: Targeted run of the changed contract test on this ruby-less host (gate skip, exit 0, 17ms)
Source: Targeted run of the changed contract test on this ruby-less host (gate skip, exit 0, 17ms)
Evidence: Regression reproduction: pre-fix version hard-fails, shipped version gate-skips
Source: Regression reproduction: pre-fix version hard-fails, shipped version gate-skips
Evidence: Adversarial contract enforcement with ruby 3.3.8 present (6 config states)
Source: Adversarial contract enforcement with ruby 3.3.8 present (6 config states)
Evidence: CI owns the full 206-script inventory, plus the pre-existing en_US.UTF-8 coverage-guard failure
Source: CI owns the full 206-script inventory, plus the pre-existing en_US.UTF-8 coverage-guard failure
Evidence: Documentation audiences behavior test over the changed docs/configuration.md prose
Source: Documentation audiences behavior test over the changed docs/configuration.md prose
Evidence: Selection narrowing and wall clock: 206 full / 54 changed-upper-bound / 2 scripts in 0.448s
Source: Selection narrowing and wall clock: 206 full / 54 changed-upper-bound / 2 scripts in 0.448s
Evidence: no-mistakes v1.72.0 carries the test.instructions consumer; origin/main does not yet set the field
Source: no-mistakes v1.72.0 carries the test.instructions consumer; origin/main does not yet set the field
Evidence: Adversarial proof the new assertion is not vacuous (excerpt)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
.no-mistakes.yaml:45- The instruction mandatesbin/fm-test-run.sh --changedas a floor, which is the same "changed tests" deterministic suite walk the guideline this change edits forbids (.agents/skills/firstmate-coding-guidelines/SKILL.md:117: "whether it selects the full suite, changed tests, a family, or a fixed script list"), just expressed as prose instead ofcommands.test. SKILL.md:120 justifies the new field by saying it "directs the agent's judgment rather than pinning a command", but the text pins one. Concrete over-selection on this very branch:.no-mistakes.yamlmaps topure-contract-unit+real-herdr-gated(bin/fm-test-run.sh:1540-1542), sobin/fm-test-run.sh --list --changedselects 54 of 206 scripts, including all 15 real-lab Herdr e2e scripts - a serial lane CI measures at ~7 minutes, withfm-backend-herdr-focus-flash-e2ealone at ~2 minutes locally (docs/fm-test-portable-shards.md:113) - even though the behavior this branch changes is covered bytests/fm-nm-test-contract.test.shalone. Because the text also forbids running less, the agent cannot narrow to the relevant script, which is the 3.6-minute posture SKILL.md:119 credits. This works against the intent's "run only the relevant tests" and "without another heavy local validation competing for this machine". Narrower form that satisfies the intent: direct judgment ("select the tests that exercise the changed behavior;bin/fm-test-run.sh --changedis available as an upper bound when the mapping is unclear") instead of commanding the walk.tests/fm-nm-test-contract.test.sh:39- The two newcaseblocks (lines 39-46) assert that a natural-language instruction string contains particular phrases. The repo test-quality rule names this exactly: parsing the YAML into a semantic model is right, but "a natural-language prompt or instruction is not proven effective because its source contains a sentence." The guard gives a wrong PASS on a config that mandates the opposite: instructions reading "Runbin/fm-test-run.sh --all; it covers every changed area. Walking the entire tests/ suite locally is acceptable." match*fm-test-run.sh*changed*(via "every changed area") and*"entire tests/ suite"*, so the test goes green while the config now pins the forbidden full walk. Symmetrically, a behavior-preserving reword ("stay scoped to the branch diff") fails the test with the policy unchanged. Smallest honest remedy is to remove the two phrase-matching blocks; the only semantically checkable property left is the typed "test.instructions is present and a non-empty string" assertion at lines 37-38, matching the siblingcommands.testcheck that asserts a typed value rather than prose.tests/fm-nm-test-contract.test.sh:44- The*"complete deterministic regression"*alternative never matches the configured value:.no-mistakes.yaml:51says "full deterministic regression coverage", and "complete deterministic regression" appears only in the YAML comment at line 37, which is not part of the parsedinstructionsstring. It is a second acceptance spelling that matches nothing today and only widens what the guard would accept tomorrow. No intent requirement needs two spellings. It lives inside the phrase-matching component flagged above, so removing that component removes this too; if the phrase matching is kept, the narrower form is a single exact requirement rather than an alternation..no-mistakes.yaml:44- CONTRIBUTING.md:72 declaresdocs/configuration.md§"Gate defaults (.no-mistakes.yaml)" the owner of the tracked gate defaults, and that section (docs/configuration.md:217-222) enumerates them:test.evidence.store_in_repo: trueand thecommands.lintpin. This change adds a third tracked default,test.instructions, and a new trust property (honored only from the default-branch copy, likecommands.testat docs/configuration.md:221), but the owning section is not updated. A reader following the declared owner pointer gets an incomplete list of what the tracked config sets.tests/fm-nm-test-contract.test.sh:28- The new function hard-fails whenrubyis missing. On this machinecommand -v rubyexits 1, so the whole suite exits 1 with "not ok - ruby is required to parse .no-mistakes.yaml for this contract" before any contract is evaluated - and this suite is inpure-contract-unit, which the change's own mandated--changedselection pulls in for a.no-mistakes.yamldiff. The repo already owns the right convention for a host-missing prerequisite: emitskip: <reason>as the first output line (tests/lib.sh:249-251), which bin/fm-test-run.sh:1669-1693 records as a successful gate-skip rather than a failure. Flagging as ask-user because the honest remedy is not local to the new function - the pre-existing sibling at line 12 fails identically, as do tests/fm-ci-workflow.test.sh:21 and tests/fm-test-run.test.sh:1667 - so fixing it means either converting that whole pattern toskip:or declaring ruby a required local toolchain, both of which extend past this change's stated scope..no-mistakes.yaml:48- "Record the exact command run and its pass/fail output as evidence before reporting the step done." is a parallel copy of a rule no-mistakes already owns: the Test step captures step evidence itself, and this same file already configures its destination at lines 52-53 (evidence.store_in_repo: true). The intent is about test scope ("run only the relevant tests, leaving the complete test suite to GitHub") and requires nothing about evidence handling. Recommend removing the sentence rather than keeping a second, drift-prone statement of the evidence contract.🔧 Fix applied.
3 infos still open:
tests/fm-nm-test-contract.test.sh:11- The ruby guard is now file-level, so on any host without ruby the whole file exits 0 with a gate skip - including the pre-existing commands.test contract, which previously hard-failed.command -v rubyexits 1 on this machine, so this change ships with zero local evidence for its own new contract, and a local regression that re-pinnedcommands.testwould not be caught locally either. This is not a defect and needs no change: the siblingtest_nm_has_no_deterministic_test_commandis invoked first at line 44, so a function-scoped skip inside the new case could never have been reached, making the file-level guard the only coherent form of the requested skip. Enforcement is intact where it counts - GitHub's runner image ships ruby, so both contracts still execute in CI, and tests/fm-ci-workflow.test.sh:21 and tests/fm-test-run.test.sh:1667 still hard-fail if ruby ever disappears from that image, so the skip cannot silently become permanent..no-mistakes.yaml:44- Verified in the consumer:internal/pipeline/steps.trustedTestInstructionsSectionrenders this field under the header "Repository live-validation runbook (trusted, from the default branch):", confirming the docs' default-branch-only trust claim. The practical consequence worth knowing is that this run's own Test step still reads main's copy, which has notest.instructions- so the new steering takes effect for validations that start after this merges, which matches the intent's framing about "the already-coded customization fixes" and the "later customization validation". Nothing to change..no-mistakes.yaml:53- The no-mistakes Test step already ships the rule "Do NOT run the complete repository test suite. Local Test is targeted validation of the requested intent; remote CI owns broad regression and remains mandatory before a PR is ready" whenevercommands.testis absent, which is firstmate's configured state. The new "Never run--allor otherwise walk the entire tests/ suite locally" sentence therefore restates a rule the tool already enforces by default; the config's incremental value is the firstmate-specific guidance (thebin/fm-test-run.sh tests/<subject>.test.shnarrowest-selection form and the optional--changedupper bound). Noting the overlap only, not recommending removal: the intent explicitly approves configuring this policy, and an explicit trusted statement is a deliberate belt-and-braces choice after the PR perf: accelerate local validation with bounded concurrency kunchenguid/firstmate#3644 32.7-minute regression.bin/fm-test-run.sh:978- Pre-existing and unrelated to this change:bin/fm-test-run.sh --check-coverage- the guard .github/workflows/ci.yml:61 runs to prove the CI lanes equal the full 206-script inventory - fails on any host whose locale is en_US.UTF-8, printingcomm: file 2 is not in sorted orderand exiting 1. Everysortin that guard is prefixedLC_ALL=C(lines 965-1053), but thecommcalls that consume those files are not, so comm validates C-sorted input against en_US collation. It passes underLC_ALL=C, which is why GitHub runners (C.UTF-8) are green. Fix is mechanical - prefix the comm calls the same way - but it touches bin/fm-test-run.sh, which this change does not, so widening this already-reviewed diff is your scope call rather than mine.time bin/fm-test-run.sh tests/fm-nm-test-contract.test.sh tests/fm-documentation-audiences.test.sh-> FM_TEST_SUMMARY total=2 failed=0, 0.448s wall, vs--list --all= 206 scripts; evidence file 06…bin/fm-test-run.sh tests/fm-nm-test-contract.test.shon this ruby-less host ->skip: ruby not installed...,gate_skip=true, exit 0; evidence file 01-targeted-run-ruby-absent.txtnot ok - ruby is required to parse .no-mistakes.yaml for this contract, exit=1; shipped version -> exit=0; evi…bin/fm-test-run.sh tests/fm-nm-test-contract.test.sh-> each `not ok - test.instructions must be a non-empty str…commands.test: 'bin/fm-test-run.sh --all'injected ->not ok - commands.test must be absent or empty so Test stays intent-targeted; got: "bin/fm-test-run.sh --all", exit=1;…bin/fm-test-run.sh tests/fm-documentation-audiences.test.sh-> 4 ok assertions including local link resolution, exit 0; evidence file 05-docs-audience-behavior.txtLC_ALL=C bin/fm-test-run.sh --check-coverage(the exact command .github/workflows/ci.yml:61 runs) -> FM_TEST_COVERAGE ok total=206 across the 9 CI lanes; evidence file 04-ci-owns-full-coverage.txtbin/fm-test-run.sh tests/fm-nm-test-contract.test.sh --json /tmp/nm-contract-timing.json- targeted run on this ruby-less host; gate_skip=true, exit 0, 17msbin/fm-test-run.sh tests/fm-nm-test-contract.test.shagainst the pre-fix (commit 26d647c) version of the script in a scratch copy - reproducednot ok - ruby is required to parse .no-mistakes.yaml for this contract, exit 1podman run --rm -v <repo-copy>:/repo debian:trixie-slimwith ruby 3.3.8 installed, drivingbin/fm-test-run.sh tests/fm-nm-test-contract.test.shacross 6 config states: shipped, test.instructions removed, emptied, set to a list, commands.test re-pinned tobin/fm-test-run.sh --all, and restoredbin/fm-test-run.sh tests/fm-documentation-audiences.test.sh- docs classification, owner pointers, and local link resolution for the changed docs/configuration.mdbin/fm-test-run.sh tests/fm-nm-test-contract.test.sh tests/fm-documentation-audiences.test.shundertime- narrowest correct selection, 0.448s wallbin/fm-test-run.sh --list --all | wc -l(206) vsbin/fm-test-run.sh --list --changed --base 8ff3a80 | wc -l(54) - selection narrowingLC_ALL=C bin/fm-test-run.sh --check-coverage- CI lane coverage proof, FM_TEST_COVERAGE ok total=206LANG=en_US.UTF-8 bin/fm-test-run.sh --check-coverage- reproduced the pre-existing locale-dependent failureno-mistakes --versionand symbol/string inspection of ~/.no-mistakes/bin/no-mistakes fortrustedTestInstructionsSectionand its rendered prompt section headerdocs/configuration.md:217- Pre-existing and out of scope for this change. The "Gate defaults (.no-mistakes.yaml)" section - which CONTRIBUTING.md:72 names as the owner of the tracked gate defaults - now enumerates test.evidence.store_in_repo, test.instructions, and the commands.lint pin, but the tracked file also sets document.instructions (.no-mistakes.yaml:15-23, the trusted Document-step placement policy) and disable_project_settings: true (.no-mistakes.yaml:10). The latter is documented at docs/architecture.md:238 under the gate authority boundary, but the owner section carries no pointer to it, and document.instructions is documented nowhere. Both omissions predate this change, which only added test.instructions, so completing the inventory here would widen an already-reviewed diff. Proposed follow-up: extend that section with document.instructions and a one-line pointer to docs/architecture.md for disable_project_settings, so the declared owner is a complete inventory of what the tracked config sets.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.