fix(fpga): record the dry-run sweep's success in the flag its verdict reads (Closes #2304) - #2305
Merged
Merged
Conversation
… reads `dry_run_sweep_ok` had two mentions in cli/tri/src/fpga.rs: declared `false` at 6098, read in the `passed` conjunction at 6333, never assigned `true`. The dry-run CCLK sweep succeeds and prints its OK line, but nothing recorded it, so smoke_gate() returned passed=false for every possible input. Its siblings survive this by being read guarded -- (!run_verify_lean || verify_lean_ok) is vacuously true when the phase is not requested. The `&& dry_run_sweep_ok` term is unguarded, so it failed the gate unconditionally. This is the second occurrence of the shape documented at line 6167 for verify_lean_ok, restored from 494e659 after the #2228 batch merge dropped it. dry_run_sweep_ok went out in the same merge and was missed. The fix mirrors the restored code: at the point where success is established -- after the variant-count bail, before the OK line -- set the flag and write the report entry, in that order. The report entry is part of the fix, not extra scope: report["dry_run_sweep"] was written only on failure, so on success it kept the null from its initializer, and the test requires that key to be an object with status "ok". Not compiled: the authoring machine had ~148 MB free, too little to check out the repo or run cargo. Verified textually -- three mentions instead of two, placement mirrors verify_lean_ok, dry_report/variant_count in scope, and dry_run_sweep is an existing field on the deny_unknown_fields report schema. The verdict conjunction is unchanged; no phase was dropped to make it pass. Closes #2304
Contributor
gHashTag
enabled auto-merge (squash)
August 20, 2026 09:22
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
gHashTag
added a commit
that referenced
this pull request
Aug 20, 2026
Third sequential cause in the cli-tri `build` job. #2303 fixed the yosys ordering; #2305 assigned dry_run_sweep_ok. With both landed the tests report 156 passed / 0 failed and the job reached a step that had never executed once in the workflow's history: ./target/debug/tri rtl check chips/phi --json Error: No such file or directory (os error 2) verdict lines: 0 chips/phi is a gitlink (mode 160000) to gHashTag/tt-trinity-phi. The job's checkout was a bare `uses: actions/checkout@v4` with no `with:` block, so submodules took its default of false and the directory was empty on the runner. The binary was never broken; it had nothing to read. The error is unattributed because top_from_info reads info.yaml with a plain `?` while its sibling declared_sources wraps the identical failure in .with_context() naming the path. submodules: true under the default GITHUB_TOKEN only works for a public submodule -- the token is scoped to this repo, and a private one would have converted a failing step into a failing checkout. All three chips/* submodules are public (phi 941 KB, euler 3,954 KB, gamma 5,365 KB, ~10.2 MB total), and none has a nested .gitmodules, so `true` is the minimal setting and `recursive` would buy nothing. No other workflow in the repo checks out submodules, so the setting comes from the constraint rather than from copied precedent. Not compiled: the authoring machine had ~230 MB free, too little to check out the repo or run cargo. Verified against the pinned gitlink f5456685c3593665153fe2765c85bb1f46ec14c2 via the GitHub contents API -- the commit is reachable in tt-trinity-phi, info.yaml is present and sets top_module "tt_um_trinity_nano", and all 49 declared source_files resolve against the 51 .v files in src/ at that exact commit. So check 1 "sources resolve" passes rather than merely emitting a FAIL line, and the remaining four come from one yosys pass already on PATH from #2303. The N<5 assertion is byte-identical and every other step is unchanged; build was not added to the required contexts. Closes #2307
gHashTag
added a commit
that referenced
this pull request
Aug 20, 2026
…ds (#2308) Third sequential cause in the cli-tri `build` job. #2303 fixed the yosys ordering; #2305 assigned dry_run_sweep_ok. With both landed the tests report 156 passed / 0 failed and the job reached a step that had never executed once in the workflow's history: ./target/debug/tri rtl check chips/phi --json Error: No such file or directory (os error 2) verdict lines: 0 chips/phi is a gitlink (mode 160000) to gHashTag/tt-trinity-phi. The job's checkout was a bare `uses: actions/checkout@v4` with no `with:` block, so submodules took its default of false and the directory was empty on the runner. The binary was never broken; it had nothing to read. The error is unattributed because top_from_info reads info.yaml with a plain `?` while its sibling declared_sources wraps the identical failure in .with_context() naming the path. submodules: true under the default GITHUB_TOKEN only works for a public submodule -- the token is scoped to this repo, and a private one would have converted a failing step into a failing checkout. All three chips/* submodules are public (phi 941 KB, euler 3,954 KB, gamma 5,365 KB, ~10.2 MB total), and none has a nested .gitmodules, so `true` is the minimal setting and `recursive` would buy nothing. No other workflow in the repo checks out submodules, so the setting comes from the constraint rather than from copied precedent. Not compiled: the authoring machine had ~230 MB free, too little to check out the repo or run cargo. Verified against the pinned gitlink f5456685c3593665153fe2765c85bb1f46ec14c2 via the GitHub contents API -- the commit is reachable in tt-trinity-phi, info.yaml is present and sets top_module "tt_um_trinity_nano", and all 49 declared source_files resolve against the 51 .v files in src/ at that exact commit. So check 1 "sources resolve" passes rather than merely emitting a FAIL line, and the remaining four come from one yosys pass already on PATH from #2303. The N<5 assertion is byte-identical and every other step is unchanged; build was not added to the required contexts. Closes #2307
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
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.
Closes #2304.
The
buildcheck had two causes; #2303 fixed one#2303 correctly fixed the yosys ordering bug and the
cli-tribuildcheck stayed red. That is not a regression and not a second ordering bug — the job had two independent causes, and only one of them lived in the YAML.In
cli/tri/src/fpga.rs,dry_run_sweep_okhad exactly two mentions in the whole file:Declared
false, read in thepassedconjunction, never assignedtrue. The dry-run CCLK sweep runs, succeeds, and prints its OK line — but nothing recorded that it had, sosmoke_gate()returnedpassed: falsefor every possible input.The sibling flags survive this by being read guarded:
(!run_verify_lean || verify_lean_ok)is vacuously true when the caller does not request that phase.&& dry_run_sweep_okis unguarded and has no such escape, so it failed the gate unconditionally.Evidence — the post-#2303 master run names it by elimination
Run 32352361519, job 96374085002: every phase reports success, verdict still false.
yosys synthesis OKis #2303 working as intended. Withbit_config_resultok,verify_lean_oktrue,theorem_matrix_oka hardcodedtrue, and thevalidate_lean_standaloneconjunct guarded off,dry_run_sweep_okis the only remaining false term — and that one test is the only failure in the job.The compiler had been printing the signature for all 12 runs:
Warnings are not denied in this job, so it never turned the build red on its own.
The fix mirrors
verify_lean_ok; it is not a new patternLine 6167 already documents this exact defect for
verify_lean_ok— it "stayed a declaration nothing ever set", restored from494e659d8after the #2228 batch merge dropped seven definitions.dry_run_sweep_okwent out in that same merge and was missed on that pass.verify_lean_okestablishes success like this (6252-6267): bail on failure, then set the flag, then write the report entry, then print the OK line. This change places the identical three-step sequence at the sweep's own success point — after the variant-count bail, before its OK line, insideif bit_path.is_file():The report entry is part of the fix, not extra scope.
report["dry_run_sweep"]was written only on failure (6148); on success it kept thenullfrom its initializer at 5967. Setting the flag alone would have moved the failure down two assertions instead of fixing it — the test walks["bit_config", "dry_run_sweep", "verify_lean"]and requires each to be an object withstatus: "ok". Mirroringverify_leansupplies both halves becauseverify_leansets flag and report together.muton 6098 stays, because the variable is now genuinely mutated.Not compiled — and exactly what that leaves unverified
This was not compiled and no test was run locally. The authoring machine had ~148 MB of disk free, which is not enough to check out the repository or run
cargo. The change was written and reviewed againstorigin/masterthrough the GitHub contents API. Nothing below should be read as a claim that it was tested — CI is the first real compile, and it may find something this review could not.What was verified, and how:
dry_run_sweep_oknow appears at 6098 (declaration), 6160 (assignment), 6340 (the unchanged conjunction).verify_lean_ok— both sit after their phase's bail and before their OK line, in flag-then-report order, inside the block that established success. The assignment stays insideif bit_path.is_file(), so a missing bitstream still yieldsdry_run_sweep: nullandpassed: false, matching the hand-builtmissing_bitstream_snapshotfixture.dry_reportis bound at 6139,variant_countat 6143.dry_run_sweepis an existingOption<serde_json::Value>field onSmokeGateReport;deny_unknown_fieldsapplies to top-level keys only, and the nested object is free-form.check_smoke_gate_snapshotdoes a strictassert_eq!but has exactly one caller, fed a hand-built literal rather than live gate output; the other comparison,assert_report_superset, tolerates added fields by construction.fpga.rs, zero deletions.What was deliberately not done
Deleting
&& dry_run_sweep_okwould have turned this check green in one keystroke by making the verdict verify less. That is the absence-is-not-a-value failure closed in #2285, #2287, and again in #2302's own note that a gate may not treat "I could not look" as "I looked and it was fine". The conjunction is unchanged and no phase was dropped.buildwas not added to the required contexts either.Known follow-up, filed not fixed
validate_lean_standalone_ok(6100) has the same two-mention shape, and worse: the entirevalidate_lean_standalonephase body is absent fromsmoke_gate(), so the flag has no success point to attach to and cannot be repaired by mirroring. It is masked today only because its conjunct is guarded and both of its tests return early whenlakeis not on PATH. It will bite the moment a runner haslake. Out of scope here.Note that
buildis not a required context, so this PR can merge beforebuildfinishes — as happened with #2303. Thebuildrun on the resulting master push is the actual verdict on this change, and will be reported.