diff --git a/.github/workflows/brain-seal-refresh.yml b/.github/workflows/brain-seal-refresh.yml index ce30eecf1e..ee99d76dcd 100644 --- a/.github/workflows/brain-seal-refresh.yml +++ b/.github/workflows/brain-seal-refresh.yml @@ -47,6 +47,8 @@ jobs: name: brain-seals path: .trinity/seals/brain_*.json +# tri:cause-removed the `git push` step was removed in #3324; this workflow has +# not run since, so its zero success count is history # The `Commit brain seals` step was removed here. It ran `git push` to master, # and the branch ruleset refuses that: every run since this workflow was added # ends `remote: error: GH013: Repository rule violations found` and diff --git a/cli/tri/src/fpga.rs b/cli/tri/src/fpga.rs index 2bc987026f..6acf3c683c 100644 --- a/cli/tri/src/fpga.rs +++ b/cli/tri/src/fpga.rs @@ -1316,6 +1316,8 @@ fn flash_id_debug() -> Result<()> { id[0], id[1], id[2] ); match id[0] { + // 0x20ba18 is the part fpga/HARDWARE_SSOT.md:323 refuses the quad flags + // for; `program_flash` reads that refusal from SSOT_PART. 0x20 => eprintln!(" → Micron (N25Q / MT25Q family)"), 0xC2 => eprintln!(" → Macronix (MX25 family)"), 0xEF => eprintln!(" → Winbond (W25Q family)"), @@ -6836,6 +6838,21 @@ fn yosys_available() -> bool { .unwrap_or(false) } +/// The board `fpga/HARDWARE_SSOT.md` is about, and whose flash refuses the quad +/// flags. CLAUDE.md names that file the single source of truth for this bench; +/// the refusal was measured in W396 (experiment E4) and written into Markdown +/// only, so the CLI went on advertising `--enable-quad` for this exact default. +const SSOT_PART: &str = "xc7a200tfgg676"; + +/// Whether the quad flags must be refused for this part. +/// +/// A predicate rather than an inline condition, so it can be tested without +/// reaching the code that SPAWNS `openFPGALoader`. A test that could spawn the +/// programmer is a test that writes to a board when it regresses. +pub fn quad_refused(part: &str, enable_quad: bool, disable_quad: bool) -> bool { + (enable_quad || disable_quad) && part == SSOT_PART +} + fn program_flash( bit: &PathBuf, cable: &str, @@ -6856,6 +6873,17 @@ fn program_flash( if enable_quad && disable_quad { bail!("--enable-quad and --disable-quad are mutually exclusive"); } + if quad_refused(part, enable_quad, disable_quad) { + bail!( + "refusing --enable-quad/--disable-quad on {part}.\n\ + fpga/HARDWARE_SSOT.md:323 records this as a MEASURED result (W396, E4):\n\ + the Micron N25Q128_3V (JEDEC 0x20ba18) on this board has no separate QE\n\ + status bit -- the N25Q family supports quad natively -- so openFPGALoader\n\ + v1.1.0 answers \"SPI Flash has no Quad bit\" and ABORTS the command.\n\ + The canonical write is x1, with verify and no quad flags.\n\ + If your bench is a different board or flash, pass --part." + ); + } let bit_str = bit .to_str() @@ -6895,7 +6923,11 @@ fn program_flash( } args.push(bit_str.to_string()); - if let Some(w) = spi_buswidth { + // Silent on x1 -- the width the SSOT's own canonical command uses and that all + // three internal callers hardcode. Printing "ensure the flash QE bit matches" + // there sent a reader to align a bit the SSOT records as not existing on this + // part, in the middle of the experiment the SSOT itself prescribes. + if let Some(w) = spi_buswidth.filter(|w| *w != "1") { eprintln!( "[program-flash] bitstream expects SPI x{}; ensure the flash QE bit and board straps match", w @@ -10817,3 +10849,54 @@ mod tests { assert_eq!(pvt.process_corner, ProcessCorner::Ss); } } + +#[cfg(test)] +mod quad_refusal_tests { + use super::{quad_refused, SSOT_PART}; + + #[test] + fn both_quad_flags_are_refused_on_the_ssot_board() { + assert!(quad_refused(SSOT_PART, true, false)); + assert!(quad_refused(SSOT_PART, false, true)); + } + + #[test] + fn without_a_quad_flag_nothing_is_refused() { + // The canonical x1 write must stay reachable. + assert!(!quad_refused(SSOT_PART, false, false)); + } + + #[test] + fn another_board_is_not_this_bench() { + // The SSOT speaks about one board; refusing everywhere would be a rule + // wider than its measurement. + assert!(!quad_refused("xc7a35tcsg324", true, false)); + assert!(!quad_refused("xc7a35tcsg324", false, true)); + } + + /// `--part` defaults to the very board the SSOT refuses, which is why the + /// advertised flag was reachable with no argument at all. + #[test] + fn the_default_part_is_the_refused_one() { + let src = include_str!("fpga.rs"); + let d = concat!("default_value = \"", "xc7a200tfgg676\""); + assert!( + src.contains(d), + "the default part moved; re-check SSOT_PART" + ); + assert_eq!(SSOT_PART, "xc7a200tfgg676"); + } + + /// The refusal must sit before anything spawns the programmer. + #[test] + fn the_refusal_precedes_the_spawn() { + let src = include_str!("fpga.rs"); + let guard = src + .find("if quad_refused(part, enable_quad, disable_quad) {") + .expect("the guard is gone"); + let spawn = src[guard..] + .find("openfpgaloader") + .expect("no spawn after the guard -- anchor moved"); + assert!(spawn > 0, "the guard must precede the invocation"); + } +} diff --git a/cli/tri/src/gates.rs b/cli/tri/src/gates.rs index b246f5e440..afd2384d55 100644 --- a/cli/tri/src/gates.rs +++ b/cli/tri/src/gates.rs @@ -4069,6 +4069,36 @@ fn workflow_file_present(path: &str) -> bool { } /// Where a never-succeeded workflow belongs. +/// The reason a never-green workflow records for its own redness, when the +/// cause has been diagnosed and removed and it simply has not run since. +/// +/// `dead` had a fourth state in the world and three in its vocabulary. It +/// printed, unconditionally, that a suppressed workflow's "last step is +/// forbidden by this repository's own ruleset" -- true of +/// `brain-seal-refresh.yml` until #3324 removed that step. On master its last +/// step is `Upload brain seals` and the only `git push` left in the file is +/// inside the comment explaining the removal, which this tool cannot read. +/// +/// So the refusal goes where the tool looks, the way `# tri:no-dispatch` did. +pub const CAUSE_REMOVED_MARKER: &str = "tri:cause-removed"; + +/// The reason recorded on a workflow whose diagnosed cause is gone. +pub fn cause_removed(text: &str) -> Option { + text.lines().find_map(|l| { + let l = l.trim(); + if !l.starts_with('#') { + return None; + } + let i = l.find(CAUSE_REMOVED_MARKER)?; + let rest = l[i + CAUSE_REMOVED_MARKER.len()..].trim(); + if rest.is_empty() { + None + } else { + Some(rest.to_string()) + } + }) +} + #[derive(Debug, PartialEq, Eq)] pub enum Bucket { /// Has a file and enough runs to judge: a dead gate, reported. @@ -4095,13 +4125,14 @@ pub fn classify(on_disk: bool, total: u64, min_runs: u64) -> Bucket { } fn dead(repos: &[String], min_runs: u64) -> Result<()> { - let mut rows: Vec<(String, String, u64, String)> = Vec::new(); - let mut deleted: Vec<(String, String, u64, String)> = Vec::new(); + let mut rows: Vec<(String, String, u64, String, Option)> = Vec::new(); + let mut deleted: Vec<(String, String, u64, String, Option)> = Vec::new(); // Every workflow the threshold hid, so a bounded report never reads as a - // complete one. `brain-seal-refresh.yml` fails structurally -- its last step - // is a `git push` this repository's own ruleset rejects -- and has 8 lifetime - // runs, so the shipped floor of 50 suppresses it entirely. - let mut suppressed: Vec<(String, String, u64, String)> = Vec::new(); + // complete one. `brain-seal-refresh.yml` has 8 lifetime runs, so the shipped + // floor of 50 suppresses it entirely. Its cause -- a `git push` to master the + // ruleset rejects -- was REMOVED by #3324, and the workflow has not run since; + // it now records that in itself with `# tri:cause-removed`. + let mut suppressed: Vec<(String, String, u64, String, Option)> = Vec::new(); let single_repo = repos.len() == 1; for repo in repos { let listing = workflow_listing( @@ -4122,7 +4153,16 @@ fn dead(repos: &[String], min_runs: u64) -> Result<()> { // The path check only means anything for the repository we are standing // in; for any other repo in the list, take the API at its word. let on_disk = !single_repo || workflow_file_present(path); - let row = (repo.clone(), name.to_string(), total, id.to_string()); + // Read from the file the classifier already consults, so a refusal + // recorded in the workflow reaches the tool rather than only a reader. + let why = if single_repo { + std::fs::read_to_string(path) + .ok() + .and_then(|t| cause_removed(&t)) + } else { + None + }; + let row = (repo.clone(), name.to_string(), total, id.to_string(), why); match classify(on_disk, total, min_runs) { Bucket::Deleted => deleted.push(row), Bucket::Suppressed => suppressed.push(row), @@ -4147,7 +4187,7 @@ fn dead(repos: &[String], min_runs: u64) -> Result<()> { total ); let mut never_ran = 0usize; - for (repo, name, runs, id) in &rows { + for (repo, name, runs, id, _) in &rows { let short: String = name.chars().take(38).collect(); // Ask the newest runs whether a job was ever allocated. A run with zero jobs // is a startup failure: recorded as failed, never executed a line. @@ -4180,11 +4220,12 @@ fn dead(repos: &[String], min_runs: u64) -> Result<()> { } /// Say what was left out. A bounded report that does not name its bound reads as -/// a complete one, and the four workflows below the shipped floor include the two -/// whose failure is structural rather than situational. +/// a complete one. A row that records why it is red -- `# tri:cause-removed` in +/// the workflow -- is listed with that reason instead of under the blanket +/// footnote, which had gone on diagnosing a cause #3324 had already removed. fn report_suppressed_and_deleted( - suppressed: &[(String, String, u64, String)], - deleted: &[(String, String, u64, String)], + suppressed: &[(String, String, u64, String, Option)], + deleted: &[(String, String, u64, String, Option)], min_runs: u64, ) { if !suppressed.is_empty() { @@ -4193,13 +4234,23 @@ fn report_suppressed_and_deleted( "{} more have never succeeded but fall under --min-runs {min_runs}:", suppressed.len() ); - for (repo, name, runs, _) in suppressed { + for (repo, name, runs, _, why) in suppressed { let short: String = name.chars().take(44).collect(); - println!(" {runs:>6} {repo:<22} {short}"); + match why { + Some(r) => println!(" {runs:>6} {repo:<22} {short:<46}cause removed: {r}"), + None => println!(" {runs:>6} {repo:<22} {short}"), + } + } + // Only about the rows that carry no reason. The blanket version of this + // sentence outlived its single referent: #3324 removed the `git push` + // from `brain-seal-refresh.yml`, and the footnote went on diagnosing it. + let unexplained = suppressed.iter().filter(|r| r.4.is_none()).count(); + if unexplained > 0 { + println!("Few runs is not few enough to be safe: a workflow that fails every"); + println!("time it runs, runs rarely. {unexplained} of the above say nothing about"); + println!("why; a workflow whose cause is fixed records it with"); + println!("`# {CAUSE_REMOVED_MARKER} ` and is listed with that reason."); } - println!("Few runs is not few enough to be safe: a workflow whose last step is"); - println!("forbidden by this repository's own ruleset fails every time it runs,"); - println!("and runs rarely."); } if !deleted.is_empty() { println!(); @@ -4208,7 +4259,7 @@ fn report_suppressed_and_deleted( deleted.len() ); println!("history, not a gate, and nothing to fix:"); - for (repo, name, runs, _) in deleted { + for (repo, name, runs, _, _) in deleted { let short: String = name.chars().take(44).collect(); println!(" {runs:>6} {repo:<22} {short}"); } @@ -4298,10 +4349,13 @@ mod tests { /// Below the floor is named, not dropped. /// - /// `brain-seal-refresh.yml` has 8 lifetime runs across five months and fails - /// every one: its last step is a `git push` to master, which the ruleset - /// answers with GH013. The shipped floor of 50 hid it completely, and a - /// bounded report that does not name its bound reads as a complete one. + /// `brain-seal-refresh.yml` has 8 lifetime runs across five months and failed + /// every one: its last step WAS a `git push` to master, which the ruleset + /// answers with GH013. #3324 removed that step -- the last step on master is + /// `Upload brain seals`, and the only `git push` left in the file is inside + /// the comment explaining the removal. The shipped floor of 50 hid the row + /// completely, and a bounded report that does not name its bound reads as a + /// complete one. #[test] fn under_the_floor_is_a_bucket_and_not_a_silence() { assert_eq!(classify(true, 8, 50), Bucket::Suppressed); @@ -8699,3 +8753,87 @@ mod dispatch_refusal_tests { ); } } + +#[cfg(test)] +mod cause_removed_tests { + use super::{cause_removed, CAUSE_REMOVED_MARKER}; + + #[test] + fn a_marker_comment_yields_its_reason() { + let y = "name: x\n# tri:cause-removed the push step was removed in #3324\non:\n"; + assert_eq!( + cause_removed(y).as_deref(), + Some("the push step was removed in #3324") + ); + } + + #[test] + fn a_workflow_with_no_marker_says_nothing() { + assert_eq!(cause_removed("name: x\non:\n push:\n"), None); + assert_eq!(cause_removed(""), None); + } + + #[test] + fn a_marker_with_no_reason_is_not_a_reason() { + // A bare marker would print "cause removed:" and explain nothing, which + // is the silence this whole change exists to remove. + assert_eq!(cause_removed("# tri:cause-removed\n"), None); + assert_eq!(cause_removed("# tri:cause-removed \n"), None); + } + + #[test] + fn the_marker_must_be_a_comment_not_a_run_line() { + let y = "jobs:\n a:\n steps:\n - run: echo tri:cause-removed nope\n"; + assert_eq!(cause_removed(y), None); + } + + /// The defect was an unconditional footnote, so the guard reads the print + /// SITE. A blanket sentence about every suppressed row is what outlived its + /// single referent when #3324 removed the cause it diagnosed. + #[test] + fn the_blanket_footnote_is_scoped_to_rows_that_explain_nothing() { + let src = include_str!("gates.rs"); + let old = concat!( + "forbidden by this repository's own ruleset ", + "fails every time it runs," + ); + assert_eq!( + src.matches(old).count(), + 0, + "the unconditional footnote is back" + ); + let guard = concat!( + "let unexplained = suppressed.iter()", + ".filter(|r| r.4.is_none()).count();" + ); + assert_eq!( + src.matches(guard).count(), + 1, + "the footnote must count only unexplained rows" + ); + assert!( + src.contains("if unexplained > 0 {"), + "the footnote must be conditional" + ); + assert_eq!(CAUSE_REMOVED_MARKER, "tri:cause-removed"); + } + + /// The workflow that motivated this must actually carry the marker, or the + /// mechanism is untested against the only case it was built for. + #[test] + fn the_motivating_workflow_records_its_own_reason() { + let y = include_str!("../../../.github/workflows/brain-seal-refresh.yml"); + let why = cause_removed(y).expect("brain-seal-refresh.yml must record why it is red"); + assert!( + why.contains("#3324"), + "the reason must name the repair: {why}" + ); + // And the cause really is gone: the only `git push` left is in prose. + for line in y.lines().filter(|l| l.contains("git push")) { + assert!( + line.trim_start().starts_with('#'), + "a live `git push` is back in this workflow: {line}" + ); + } + } +} diff --git a/docs/now/2026-09-06-two-tools-that-advised-against-their-own-bench.md b/docs/now/2026-09-06-two-tools-that-advised-against-their-own-bench.md new file mode 100644 index 0000000000..41bf0561d2 --- /dev/null +++ b/docs/now/2026-09-06-two-tools-that-advised-against-their-own-bench.md @@ -0,0 +1,8 @@ +# NOW -- Two tools that advised against their own bench (2026-09-06) + +## Two tools that advised against their own bench (Closes #3371) + +- `tri fpga program-flash` advertised `--enable-quad` with `--part` defaulting to the exact board whose SSOT records the flag ABORTS the command (measured, W396/E4). Zero guards near the flag. `quad_refused()` now refuses both flags on that part, quoting the SSOT. +- Worse than the flag: on `--spi-buswidth 1` -- the SSOT-canonical path all three internal callers hardcode -- it printed 'ensure the flash QE bit matches' about a bit the SSOT says does not exist on this part. Now silent on x1. +- `tri gates dead` printed unconditionally that a suppressed workflow's last step is 'forbidden by this repository's own ruleset'. #3324 removed that step; the only `git push` left in `brain-seal-refresh.yml` is inside the comment explaining the removal. Three further assertions in gates.rs stated the old cause. +- `# tri:cause-removed ` gives the classifier its fourth state; the row is listed with its reason and the blanket footnote is scoped to the rows that explain nothing -- 3 of 4 today.