Repository navigation
Three format-converge defects that only a live spreadsheet could find - #11581
Conversation
The format converge landed in gunbc#11485 with witnesses green and CI green. Run against the real sheet it failed three times in a row, each for a different reason, and none of the three was reachable by any witness. 1. A GENERIC THAT TYPECHECKED AND DID NOT EVALUATE. first_of<T> folded over List<T> with `none` as the initial accumulator; nothing in the expression fixes T, so the return type could not be resolved where it was used and the run died with an error type cascade. This is the `as Nat` shape -- an expression the compiler accepts and the evaluator refuses -- and the repair is the same: say the type. Four monomorphic folds replace it. 2. THE READBACK COULD NOT SEE ITS OWN WORK, and this is the serious one. The read was bounded to A1:Q1 to keep grid data cheap. Sheets filters conditionalFormats to the rules INTERSECTING the requested ranges, and these rules start at row index 1 while that range is row 0 -- so the rules came back absent, the fold counted zero, and the converge reported UNVERIFIED over a sheet it had just formatted correctly. Not a cosmetic false negative. format_is_converged compares the rule COUNT, so the next run would have applied five more rules, and the next five more again -- the exact accumulation the readback exists to prevent, caused by the readback. Invisible to witnesses by construction: every witness supplies its own observation, so none can discover that a request does not return what it asked for. Split now along what each question needs -- grid data stays bounded to the header row, metadata is unranged. 3. AN EMPTY RANGE IS NOT AN ABSENT ONE. Passing "" for ranges answers 400 "Unable to parse range: ", and a declared query parameter cannot be left out, so the metadata read needed an operation that never names a range. GetSpreadsheetMetadata is that operation and its annotation says why it exists, because "why is there a second get" is the question a reader will ask. EVIDENCE, and it is execution rather than typecheck. Against the live sheet: the converge adopted the existing spreadsheet by its marker (no create), applied all seven format steps, and the readback then confirmed them. A SECOND run wrote NOTHING -- permissions read, file found, two format reads, zero batchUpdate calls -- and the sheet still carries exactly 5 conditional rules and frozenRowCount=1, which is the idempotence claim verified against the API rather than asserted. STILL OWED, and stated rather than left: the marker migration. gunbc#11485 changed the identity field from appProperties to properties for a credential-visibility reason that was correct, and orphaned the sheet already carrying the old marker -- which is how a duplicate got minted tonight. The operator's sheet was migrated BY HAND; a fresh environment would repeat it. The converge must find a sheet carrying the old marker and adopt it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… annotation THE REVIEW WAS RIGHT AND THE FINDING IS THE SERIOUS KIND. Splitting the readback left header_format_of answering `none` for "the grid response contained no sheets", which is the same value it answers for "the header cell carries no userEnteredFormat". The prior revision refused there -- FormatUnreadable, "nothing may be adopted from it" -- and folding the two together meant header_matches false, format_is_converged false, and the converge proceeding to issue batchUpdates against a spreadsheet whose grid response it never read. A PR whose whole argument is that a readback must be trustworthy before acting on it had moved one input of that readback the wrong way: the rostered class absent_reads_identically_to_never_looked, and a widening failure arm where DESIGN section 5 requires a refusal. The repair is a type rather than a guard, because `SheetsCellFormatRead?` is simply not capable of carrying the distinction: HeaderFormatReading has an arm for a reading that did not happen, and read_format_meta carries it through as FormatUnreadable. The two states can no longer be spelled the same way. THREE CLASSES DISCOVERED, TWO ROWS FILED, AND THE THIRD IS DELIBERATELY NOT A ROW. DESIGN section 4b obliges every newly discovered error class to file one, and the receipts were living in `//` prose, which no Accepted program can read. generic_type_parameter_fixed_only_by_an_untyped_literal -- a type parameter whose only occurrence is an accumulator seeded by a literal that fixes no type. Every occurrence agrees with every other, which is why acceptance succeeds: consistency is not determination. Ceiling 3, and the trigger names the capability. The row carries the detector gap as its point: a witness SUPPLIES the list, which fixes T at the call site, so it exercises a resolvable instantiation of a function whose production instantiation is unresolvable -- green, honest, and evidence of nothing. readback_scoped_to_exclude_the_effect_it_verifies -- a converge readback whose request scope excludes the effect it exists to confirm. The harm is unbounded rather than merely wrong: the mechanism installed to make an actuation idempotent becomes the ratchet that repeats it. Ceiling 2, and the reason it stops there is stated rather than softened -- whether a request's scope contains an effect's extent is a fact about an external API's filtering semantics. The empty-range 400 is a RECEIPT INSIDE the second row, not a third file. An empty parameter is not an absent one, and a declared query parameter has no absent encoding -- but that fact was discovered in the repair of the readback split, shares its repair (an operation that names no range), and has no life apart from it. Minting a row for it would be the re-invention DESIGN section 2 names as a failed decomposition. AND THE PARSE REFUSAL, WHICH IS MINE AND IS EXACTLY THE RULE I WAS QUOTING. required-witnesses-floor refused with six parse errors, and downstream of those the namespace wave had no head index and the floor could not plan its arm set -- three blockers, one cause. The six were six lines of one annotation block I authored INSIDE the sheets.Spreadsheets service body. Section 4c admits standalone leading `//` attached to MODULE-SCOPE declarations and refuses body forms, so an operation nested in a service cannot carry one. The reasoning is not deleted; it moved to the module-scope block that already sits above the service and now says why there are two get operations rather than one with a different argument. Worth recording that `gunbc compile` over the whole corpus accepted this: the annotation grain is adjudicated by the required-CI parse sweep and by nothing the ordinary compile path runs. Locally re-derived after the move: parse OK, 6152 files parse-clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both findings were real and both are fixed in f0acbcd. Thank you — the first one is the serious kind and I had introduced it in the same change whose argument is that a readback must be trustworthy before you act on it. 1. The grid-side refusal is restored, as a type rather than a guard. You read it exactly right: The repair is 2. Two rows filed; the third is deliberately a receipt rather than a file.
3. The CI refusal was mine, and it was the rule I was quoting. Six parse errors, and downstream of them the namespace wave had no head index and the floor could not plan its arm set — three blockers, one cause. The six were six lines of one annotation block I authored inside the One thing worth leaving here: Still owed and unchanged: the — sent from zesty-crane-846 |
briansrls
left a comment
There was a problem hiding this comment.
APPROVE — source-reviewed exact head f0acbcda5788ac5d0649308930731875aa45a2ec.
No blocking source findings.
The repair is correctly cut at the external boundary the live run exposed:
- grid data stays bounded to
A1:Q1; - conditional-format and frozen-row metadata move to a distinct unranged operation, so the readback can see every rule it decides on;
- an empty
rangesvalue is not used as a counterfeit omission; HeaderFormatReadingkeeps “the header has no format” distinct from “no sheet was read”;- the undetermined generic is replaced with monomorphic folds whose return types are explicit.
The decisive evidence is the live two-pass receipt: the first run applied the seven intended steps and the second performed the reads with zero formatting writes while observing five rules and one frozen row. That establishes the route, not merely the folds.
GitHub currently reports this branch mergeable: false, so this is source approval rather than permission to merge an unresolved head. Reconcile/update the branch, rerun the required gates, and preserve the exact two-read construction. #11596 remains a separate follow-up and does not block this bounded repair.
briansrls
left a comment
There was a problem hiding this comment.
Correction to my source approval — one required source-cleanup push on the same head.
The implementation remains sound, but the annotation immediately above classify_format_step now states the opposite of the production route this PR lands:
“formatting is NOT idempotent-by-observation: re-running re-applies rather than adopting. Conditional rules in particular accumulate”
That was true of the pre-readback actuator. It is false at this head: converge_review_sheet_format performs independent reads, skips writes when converged, and the live second run observed five rules/one frozen row with zero mutations.
Narrow the annotation to what the function actually establishes: an individual mutation response is not its own readback; the enclosing converge supplies the independent pre/post observation. Delete the stale claims that reruns necessarily reapply and rules accumulate.
This is not a code HOLD beyond the already-required branch reconciliation, but I would not merge a mechanism claim that directly contradicts the new route and its live receipt. With that sentence corrected and the branch mergeable/gates green, my disposition is APPROVE.
DESIGN 4b requires every discovered error class -- a review finding among them -- to file or UPDATE one row under recurring_failure_mode. This updates rather than adds: readback_scoped_to_exclude_the_effect_it_verifies already names a field mask among the bounds it covers, and its existing specimen is this very module from #11581, so a new row would be a second authority for one concept. The new specimen carries the discriminator that is not a restatement. In #11581 the bound was always too small for one of the facts it gathered. Here the bound was correct when authored and the DECODE grew a field dependency the REQUEST was never widened for -- drift between two edits rather than a scope that never contained its subject. That changes the remedy, so the row now says so. Naming the scope beside the extent is a discipline a reader performs, and it failed on the next edit to the same module. Composing every mask from the same named field-path row its decode keys on removes the relationship's two ends instead: construction where the rule was validation. It also corrects something the row asserted too broadly. It concluded no witness could hold this wall because a witness never issues a request. True of the API's filtering semantics; not true of this specimen, where the missing claim was local and cheap -- whether the mask names the field the decode keys on is decidable with no service involved, and eighteen claims, eight mutations and a green required floor all passed over a production path that refused every workbook. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The format converge landed in #11485 with witnesses green and CI green. Run against the real spreadsheet it failed three times in a row, each for a different reason, and none of the three was reachable by any witness.
1. A generic that typechecked and did not evaluate
first_of<T>folded overList<T>withnoneas the initial accumulator. Nothing in the expression fixesT, so the return type could not be resolved at the use site and the run died with an error type cascade. Four monomorphic folds (first_grid,first_row,first_cell,first_sheet_of) replace it — saying the type is what the evaluator needed and what the generic had no way to supply.2. The readback could not see its own work
The read was bounded to
A1:Q1to keep grid data cheap. Sheets filtersconditionalFormatsto the rules intersecting the requested ranges, and these rules start at row index 1 while that range is row 0 — so the rules came back absent, the fold counted zero, and the converge reported UNVERIFIED over a sheet it had just formatted correctly.Not a cosmetic false negative.
format_is_convergedcompares the rule count, so the next run would have applied five more rules, and the next five more again — the exact accumulation the readback exists to prevent, caused by the readback. Split now along what each question needs: grid data stays bounded to the header row, metadata is unranged.3. An empty range is not an absent one
ranges=""answers400 Unable to parse range:, and a declared query parameter cannot be left out, so the metadata read needed an operation that never names a range.GetSpreadsheetMetadatais that operation.Then review 67514 found a fail-open I introduced while fixing these, and it was right
Splitting the readback left
header_format_ofmapping "the grid response contained no sheets" tonone— the same value it answers for "the header cell carries nouserEnteredFormat". The prior revision refused there; folding the two together meantheader_matchesfalse,format_is_convergedfalse, and the converge proceeding to issue batchUpdates against a spreadsheet whose grid response it never read. A change whose whole argument is that a readback must be trustworthy before you act on it had moved one input of that readback the wrong way — the rostered classabsent_reads_identically_to_never_looked.The repair is a type rather than a guard:
HeaderFormatReading = HeaderFormatRead { format } | HeaderFormatUnreadable { cause }.SheetsCellFormatRead?simply has nowhere to put "no reading happened", which is how the two states could be spelled the same way at all.And the CI refusal was mine, and was the rule I was quoting
Six parse errors, and downstream of them the namespace wave had no head index and the floor could not plan its arm set — three blockers, one cause. The six were six lines of one annotation block authored inside the
sheets.Spreadsheetsservice body. §4c admits standalone leading//attached to module-scope declarations and refuses body forms. The reasoning moved to the module-scope block already above the service.Worth recording:
gunbc compileover the whole corpus accepted it. That grain is adjudicated by the required-CI parse sweep and bygunbc run, not by the ordinary compile path.Two failure-mode rows filed
The receipts were living in
//prose, which noAcceptedprogram can read (§4b).generic_type_parameter_fixed_only_by_an_untyped_literal— every occurrence ofTagrees with every other, which is why acceptance succeeds: consistency is not determination. The row's point is the detector gap — a witness supplies the list, which fixesTat the call site, so it exercises a resolvable instantiation of a function whose production instantiation is unresolvable. Green, honest, and evidence of nothing.readback_scoped_to_exclude_the_effect_it_verifies— the harm is unbounded rather than merely wrong: the mechanism installed to make an actuation idempotent becomes the ratchet that repeats it. Ceiling 2, because whether a request's scope contains an effect's extent is a fact about an external API's filtering semantics.The empty-
ranges400 is a receipt inside the second row rather than a third file — it was discovered in that repair, shares it, and has no life apart from it.Evidence — execution, not typecheck
Against the live spreadsheet: the converge adopted the existing sheet by its marker (no create), applied all seven format steps, and the readback confirmed them. A second run wrote nothing — permissions read, file found, two format reads, zero batchUpdate calls — and the sheet still carries exactly 5 conditional rules and
frozenRowCount=1. That is idempotence verified against the API rather than asserted.Locally re-derived after the annotation move:
required-ci: parse OK 6152 file(s) parse-clean.Follow-up, now open
The
appProperties→propertiesmarker migration is #11596.🤖 Generated with Claude Code