CHANGELOG.md
Rama main · réplica de lectura del repositorio original.
# Changelog
All notable changes to Massing. Releases are signed, auto-updating desktop builds
(Windows / macOS / Linux); the updater always serves the latest. Format loosely follows
[Keep a Changelog](https://keepachangelog.com/).
## Unreleased
### ROUTE-SHADOW — four routes were registered on a URL another route already owned
Starlette matches the **first** route whose path and method fit, so a second registration on the same
pair is not an override — it is dead code that looks live. FastAPI then builds the OpenAPI document
into a dict keyed by path and method, so the **last** registration is the one that gets described.
The server ran one handler and `/docs`, the OpenAPI JSON and `apps/web/src/api/schema.d.ts` all
described the other.
| URL | served | never ran |
|---|---|---|
| `GET …/drawings/plan.svg` | `routers/drawings.py::plan` | `routers/authoring_docs.py::plan_svg` |
| `GET …/drawings/sheet.svg` | `routers/drawings.py::sheet_svg` | `routers/authoring_docs.py::sheet_svg` |
| `GET …/drawings/sheet.pdf` | `routers/drawings.py::sheet_pdf` | `routers/authoring_docs.py::sheet_pdf` |
| `GET …/mep` | `routers/analysis.py::mep` | `routers/authoring_analysis.py::mep_summary` |
**Fixed — the MEP systems browser has never worked.** `apps/web/src/api/mep.ts` declared two shapes
for `/projects/{pid}/mep`: `mepSummary()` with `systems` as a **list** of per-system counts, and
`mep()` with `systems` as a **Record**. The inventory handler served, so
`apps/web/src/viewer/tools/mepSection.ts` read `s.systems.length` off a Record, got `undefined`, and
printed *"No distribution systems yet — add duct/pipe runs + fittings."* on every model that has ever
had one — then returned, hiding the by-discipline rollup, the fire-protection check and the sizing
checks below it. `/projects/{pid}/mep` now resolves to `mep_summary`, which is what the panel, the
client method and the published contract all already expected.
**Also fixed: the sheet parameters `apps/web/src/viewer/sheetSpecs.ts` was written to work around.**
That module records, from a measurement on 2026-08-09, that the rail sent `number`, `title` and
`scale` and the route dropped all three. It fixed the caller and diagnosed the cause as a caller
speaking a dialect the callee does not. The dialect was real and published: `number`, `title` and
`scale` are exactly the shadowed handler's parameters, which is what `/docs` describes for that URL.
**The decisions ran in opposite directions, and consumers is what decides.** The three drawing
handlers in `authoring_docs.py` went, because `drawings.py` serves a strict superset and the whole
client is built on it. The **serving** `/mep` handler went, because `energy.mep_inventory` had one
caller in the tree — that route — and its client method `api.mep()` had none at all. The engines stay
in `aec_data`, still tested, so publishing the ARCH-D titleblock sheet under its own URL remains
available as a product decision rather than a gap.
**Gated: `services/api/test_route_shadow.py`.** Derives every `(path, method)` the app holds by
walking FastAPI's deferred router placeholders; fails closed on any route object it cannot classify;
floors the walk's size and requires the four routers that carried the collisions to be present, so a
broken walk cannot report a clean tree in the words of a clean tree. It replays the four shipped
registrations (built here, not fetched from git, which CI's shallow checkout cannot do) and must find
**all four and nothing else**, and a walk narrowed to top-level routes must **miss** all four.
**The gate asks Starlette, not `==`.** The four shipped instances were identical path strings, so an
equality test would have found all four — but equality is a *precondition*, not the rule. Starlette
matches by regex, so `/projects/{pid}/drawings/{name}` registered before `/projects/{pid}/drawings/sheet.svg`
swallows it just as completely. Measured before widening: the live tree holds **0** of that
non-identical form, so using Starlette's own matcher costs no exemptions and no noise — it removes a
precondition rather than chasing a finding. What it buys is visible under mutation: a single misplaced
`/projects/{pid}/mep/{probe}` silently kills **eleven** MEP endpoints, and the widened rule names every
one while the equality version reports that tree clean. Its soundness rests on no route declaring a
`:path` converter, which is now asserted rather than assumed.
**FastAPI's own warning saw two of the four.** It keys on a duplicate *operationId*, built from the
function name plus the path, so it fired for `sheet_svg` and `sheet_pdf` — two handlers spelled the
same — and stayed silent for `plan`/`plan_svg` and `mep`/`mep_summary`. *A warning keyed on the name
cannot see a collision that is about the path*, and half a population reported confidently reads
exactly like all of it.
**Review found two ways the gate could still report clean, and both were preconditions I had not
written down.** It probed `{param}` with a plain segment, so an `:int`, `:float` or `:uuid` route
rejected its own probe and a duplicate on one came back clean — the first draft asserted only that no
`:path` converter existed, which is *asserting the one case you thought of, not the property*. And it
read a `prefix` attribute off FastAPI's deferred-inclusion placeholder; **this FastAPI has no such
attribute**, so it silently walked the un-prefixed router and two handlers colliding at a prefixed URL
passed. Every replay fixture used an empty prefix, so **no test in the file could have caught it**.
Both are now fixed at the source rather than patched: the walk takes FastAPI's own effective route
contexts, whose `path_regex` already carries the prefix, and probe validity is *self-checking* — every
probe must match the route it was built from, so an unhandled converter reds the build instead of
quietly shrinking the population. Two fixtures added (`:int` collision, prefixed collision), each
mutation-verified to red the right check.
**Why nothing went red.** Every MEP assertion in `services/api/test_mep_systems.py` calls the engine
`mep.mep_summary(m)` directly; not one goes through the route. *A test of the engine is not a test of
the door.*
**And the gate that would have named it misdiagnosed instead.** `apps/web/src/api/clientCallers.test.ts`
already held `mep` in `UNCALLED` — it had measured the method as callerless and frozen that. Removing
the method made its check fail with *"1 method(s) in UNCALLED now HAVE a caller: mep"*. An entry
leaves that set either because it gained a caller or because it stopped existing, and the message
asserted the first for both. The named repair happened to be right and the stated reason would have
sent a reader looking for a screen that does not exist; the two are now separate assertions, each
mutation-checked.
Everything below ships in the next release. **These were 34 separate `## Unreleased` headings**
until 2026-09-07 — one per change, each a peer of a real version — so the word had stopped
meaning anything as a heading and the file read as 34 pending releases rather than one. The
titles are unchanged and now sit at `###` beneath this, in the same order; no text was edited,
added or dropped in the fold.
### A wire type declared twice is a field no audit can see
`ClashResult.truncated` was returned by the server and read by nobody for months, and the reason was
never that anyone decided to ignore it: the clash panel had re-spelled the response shape, so no
property access resolved to the declared field and no type error, lint or unread-field audit could
reach it. Two more shapes were written out twice the same way — `Vital` and `LogisticsResource`, both
character-for-character copies of `api/types.ts`, one of them under a comment reading "as the API
returns it". Both now import the type they were copying.
A new check refuses the next **exact** copy: same type name and same member set. That is narrower
than the case above — a local shape that drops a field, as the clash panel's inline cast did, has a
different member set and is not caught. The narrowness is deliberate: a subset rule flags twenty
shapes here, eighteen of them deliberate narrowings, and gating it would need an exemption list.
### The Discipline Spine says how many spec sections it is NOT counting
The chain-coverage bars are computed over the *enforced* spec population — sections a withdrawal has
taken out of the chain are excluded, and correctly so: a withdrawn section is not an unpackaged one.
The card printed the enforced number and nothing else, so on a job with a void section its "18
specs" silently disagreed with the 21 in the spec register, with nothing on the page accounting for
the difference. It now reads "21 spec sections on the job · **3 withdrawn (excluded)**, so every bar
above is taken over 18", with the sections themselves in a disclosure below it. The server has always
returned both numbers for exactly this reason, and the submittal-log card has always rendered the
same caveat.
Two of the four bars, not all four: `sheets_specced_pct` divides by the drawing count and
`packages_costed_pct` by the package count, neither of which a withdrawn spec changes. The caveat
names the two it applies to.
### Sources & Uses shows the loan fees
The PDF's Sources & Uses itemises six rows; the on-screen table showed five. The missing one is the
origination points — real money, and invisible because its absence balances: the fee is folded into
Total uses and funded out of equity, so uses still equalled debt plus equity and every visible number
reconciled. On two points against a $13m loan that is $260k of the equity cheque with no line of its
own.
### The coordination matrix now counts the whole clash run, not the first page of it
A federated clash run returns two numbers: `count`, every interference it found, and `clashes`, the
first 200 of them. The Clash panel printed the first beside the list and used the second to build
the discipline-pair matrix — the screen a coordination meeting reads to decide which trades still
have to talk to each other.
That matrix is the one place this product makes a claim about what was *checked*, and it has a
third state for exactly this: a pair nobody ran is `untested`, never `clean`. Building it from the
page defeated that from outside. Every returned clash on a large model tends to belong to one or two
busy pairs, so a pair whose interferences all sat past the 200th arrived with no evidence at all —
and with every pair declared tested, it was reported **clean**. Measured on a 1,438-clash fixture
with 918 of them between architecture and MEP, none on the first page: the matrix said 100% pair
coverage, nothing untested, and called that pair clean.
Both federated paths now tally clashes per discipline pair over the *whole* run, where the full list
still exists, and the tally travels beside the page: the list is what a person clicks, the tally is
what a report may claim. Against an older server that truncates and sends no tally, the panel
declares nothing tested rather than guessing — those pairs read `untested`, which is what the third
state is for.
Two smaller things on the same screen. The list header counted the page while the summary above it
counted the run, so two different numbers sat under the word "clashes" with nothing saying a cap was
in play; it now reads "200 of 1438 clashes — click to inspect (list capped)". And a single-model run
that creates issues says when the cap is why fewer issues exist than clashes.
`truncated` was a field the server had always returned and nothing had ever read, which is how this
survived: the panel re-spelled the response shape inline rather than using the declared one, so no
type, lint or audit could see the omission. The shapes are now declared once in
`apps/web/src/api/clash.ts` and the audit pins them as read.
Review then found the same premise one level further down. The panel declared *every* pair of
discipline models tested, including each model against itself — and a federated run excludes
same-model overlaps by construction, because a beam meeting its own column is a joint, not a
coordination finding. So the diagonal was reported clean by a run that could not have looked at it.
It is now filtered on the model key, which keeps two distinct models that happen to share a
discipline label correctly counted as tested.
### Clicking a register pin on the model now opens its record
A pin on the 3D model is a register record's marker — an RFI, a punch item, an observation. Clicking
one selected the element it sits on and wrote a line to the status bar, and that was all: the record
the pin stood for had no way in. It now selects, reports, and opens the record.
The behaviour was already written down as shipped. `apps/web/src/pins/pins.ts` branches
topic-versus-record on click and explains why — *"a topic restores a saved viewpoint, a record opens
its register row"*. Only the first half was true.
Nothing new was designed for it. `apps/web/src/main.ts` has jumped to a record since the command
palette shipped, whenever a search hit is chosen; the pin now goes through that same
`jumpToRecord`, so there is one answer to "where does a record open" rather than two that drift.
The item had been filed as needing a UX decision on the strength of a grep over the viewer directory
finding no opener — there is none *there*, and the opener was one directory over.
Selection stays in the viewer rather than following the user across: coming back to the Model
workspace finds the element still highlighted.
The handler lives in `apps/web/src/viewer/pinOpen.ts` so the viewer's `app.ts` did not grow, and the
opener reaches the viewer as a required callback rather than an import — required so a dropped wire
is a compile error instead of a dead pin, injected so the viewer never reaches into the portal.
Three things review found, all real. A pin marker's click also reached the viewport, whose raycast
misses a DOM overlay and then clears the selection a few frames later — so the highlight the click
had just set was being undone; both marker kinds now stop the event. A rejected `selectByGuid` took
the record jump with it, although the record id came from the pin and not from the scene; selection
is best effort now, and says so rather than failing silently. And the jump waited on a fixed 500 ms
instead of the portal's own init — **there were three copies of that guess**, and all three now share
one wait that resolves when the portal is actually ready and reports when it cannot open at all.
That last fix created a fourth defect, which the next review round found. Making the portal's init
retryable turned every partial mutation on the way to a failure into reachable state: `init()`
reassigns its own root to the content pane and registers a window listener before it awaits the
first render, which can reject — so a retry would have nested a second shell inside the first and
added a duplicate listener. Making a failure recoverable makes every partial mutation on the way to
it reachable.
### The project's parcel boundary now draws on the model
Load a cadastral boundary on Feasibility → Massing and the lot line is available in the 3D viewer,
under Open ▾ → Site context. It is the same ring the building was sized against, not a second copy
loaded separately — the two cannot drift.
The roadmap carried this as "parcel overlays", which sounds like a drawing task, and both halves of
the drawing already shipped: the viewer draws OSM land-use parcels, and the server parses a real
cadastral boundary that sizes the building on the lot's true outline. What was missing sat one layer
down — nothing persisted the ring. The feasibility tab held it in a local variable and sent it
straight to the massing engine; a tab re-render dropped it, and no other screen could ask for it.
The ring is stored beside the parcel and tax keys it belongs with, through the merging write that
already serialises the property form against the feasibility tab. It is drawn in metres rather than
being re-projected through a lon/lat anchor, which is how a lot line ends up a continent from its
building.
Review then found the frame was wrong in a smaller, quieter way. A saved ring is shifted to its own
bounding-box *minimum*, so it lies wholly in the positive quadrant, while a generated massing model
is built around the origin — the lot line sat off the corner of its own building. It is now
recentred on the bounding-box **centre**, which is the anchor the massing generator already uses to
put the parcel in the building's frame. The test that claimed to pin the frame passed before and
after that fix, because its only fixture was a square centred on the origin and the translation was
a no-op on it.
And the save is now serialised per project. Selecting one parcel and then another fired two
independent writes; the project lock decides which lands last by arrival, not by which was chosen,
so the viewer could end up drawing a parcel the tab was not showing — and a select-then-clear pair
could resurrect a parcel the user had removed. Selecting a parcel with no project open now says so
instead of skipping the save silently.
The queue outlives one render of the tab, which is what makes it a queue: the feasibility tab
re-renders whenever the proforma does, including immediately after "Generate IFC model", and a queue
rebuilt at that moment cannot order the save that was already in flight across it.
**Saving the property no longer deletes the rest of it.** `PUT /projects/{pid}/property` preserved
one key across a write — the saved appraisal — and let the request body replace everything else. That
read as a merge for as long as the only caller was the property form, which reads the whole record,
edits it and sends it back. This change added a caller that sends the parcel boundary alone, and that
write took the address, the block and lot, the purchase price, the areas and every tax line with it.
The route now merges the body over the stored record: the body still wins where they overlap, so the
form can still edit, and an explicit null still clears, because what matters is a key omitted versus
a key sent as null.
The viewer also waits for a pending boundary save before reading the property back, so opening the
overlay straight after selecting a parcel no longer reports that none is saved.
### Four records called concurrency gaps open that the sweep had already closed
`services/api/test_rmw_sweep.py` reports *"43 ORM sites: 43 reasoned exempt or locked, 0 named
open"*. `docs/roadmap.md` still carried RMW-LOCKGAP and RMW-TOKEN as 🟡 **OPEN**, and
`docs/security/threat-model.md` still carried G-10 and G-12 as open gaps with a summary line reading
"**Four** sites remain open". All four are corrected against the code, each naming the pull request
that closed it and the date.
A stale OPEN is a work item, not a typo: the roadmap's ranking sends the next reader at it, the
threat model presents a reviewer with a live exposure, and RMW-TOKEN's entry had priced its fix as
"a migration (add the column, backfill, route both writers through a CAS)" — work nobody ever did,
because both of its sites closed with an advisory lock already in the tree and already serving the
gap next door. Half of that entry was wrong on the day it was written: `realestate.save_appraisal`
took its lock the day before.
`services/api/test_gap_records.py` now fails the build on the class. It derives the sites from the
sweep's own ledgers by AST and the records from both documents by indentation, and holds any record
that cites the sweep and reads as open to naming at least one still-open site. It replays the four
records as they shipped at `53cfa71a` — frozen under `services/api/tests/fixtures/gap_records/` —
and must re-find all four, and nothing else, before it may report a clean tree.
The first draft read those records out of git history instead of copying them. That works on a
full clone and not in CI, whose checkout is shallow, so the gate failed closed on every build. A
proof that an analyser finds anything cannot depend on the depth of somebody's clone.
Review found two more. The replay was evaluating those frozen records against today's ledger, so a
site that legitimately reopened would have red the build blaming the analyser for being right; the
statuses are frozen alongside the records now. And the gate's roadmap-marker list knew `✅ ◧ 🟡 ⭐`
but not `⛔`, `❌` or a struck-through code — 12 of 90 roadmap items were outside its population
entirely, reported as a clean tree in exactly the same words as a clean tree. A loose-versus-strict
parity check now reds the build on a marker it does not know, and found one of the three itself.
`services/api/test_roadmap_status.py` gained the self-tests it was missing. Its loop reads
`if not marked_open(code): PASS; continue`, so a `marked_open` that can never say yes sent every
registered item down the passing branch — measured by mutation: breaking its bullet regex with one
literal left every line green, the exit status 0, and the verdict *"every item with a measurement
agrees with its marker"* printed over nothing.
### The desktop IFC converter can now be interrupted — it runs in a child process the server kills
`from_ifc.convert`'s deadline is cooperative: checked between elements and every 4,096 entities.
That bounds a model that overran because it had a lot in it, and cannot reach the two costs outside
every checkpoint — `ifcopenshell.open`, one call whose time scales with file size and is paid in
full before the first check, and any single `create_shape` that never returns. On the desktop app,
where there is no Node runtime and the Python path is the only one, `edit_preview` passes
`timeout=120` on a synchronous route: a model that hit either cost held a request worker
indefinitely instead of returning the 503 the caller is written to expect.
The conversion now runs in a child process, and the parent kills it. The entry point re-enters
itself through an argv sentinel, so `sys.executable` is the bundle when frozen and the interpreter
when not — no `multiprocessing`, and therefore no `freeze_support()` to wire, which is what the
roadmap entry had recorded as the blocker for a fortnight.
Behaviour where nothing is wrong is unchanged: the child gets the caller's full budget and an
ordinary overrun still comes back from its own checkpoint, naming the phase it was in. The kill
fires five seconds later, so the outer bound is `timeout + 5s` rather than `timeout` — stated
plainly, because shortening the child's deadline to keep the bound exact would make the two
branches disagree about what the parameter means.
There is no in-process fallback. Spawning failures are not something anyone reports, so a fallback
would mean the conversion quietly went back to being uninterruptible while the reader still believed
it was bounded.
### The reachability gate was reading 708,650 characters of somebody else's product as evidence about ours
`_web_source()` — the blob both reachability gates ask "does any client call this route?" — included
`apps/web/src/vendor/**`: verbatim, unedited copies of `MassingCloud/massingifc` and
`MassingCloud/massing-pdf`, 83 files. A route leaf occurring there is a fact about another
repository. The blob already excluded the generated OpenAPI types for exactly that reason, and the
sentence simply had not been finished.
**It was not a theory; the consumer had written the defect down twice without naming it.** The
gate's own notes blame `/projects/{pid}/cost/calibration` on "85 hits of PDF-takeoff SCALE
calibration under `vendor/massingpdf/`, a different trade entirely", and its deterministic
regression fixture is a line lifted out of `vendor/massingifc/project-schema/coordination.ts`. The
diagnosis stopped at "a leaf that is also a common domain noun" — true, and one layer short, because
a vendored tree is a bulk supplier of exactly that noun. *Two symptoms written up separately are how
one cause stays unnamed.*
Measured: uncalled 57 → 59, nothing lost. Both routes that move —
`/projects/{pid}/project-package/contents` and `/projects/{pid}/workflow/{key}` — were already
frozen as **invisible** in `LEAF_COLLISION_BLIND`, so this finds no new debt; it converts two routes
the rule could not see into two it reports. **That is the direction the ratchet punishes**, which is
why nothing was going to drift into it unprompted, and it is also a third way out of the blind-spot
list that the list did not anticipate: not "it gained a caller" and not "it was renamed", but *the
text that vouched for it was deleted*.
The exclusion is asserted the same two ways the generated-types one is — the vendored text is
genuinely absent from the blob (probed per package, since the two were vendored on different dates
from different repositories), and putting it back changes the answer — plus a third the older check
does not have: the differential is **named**, not just counted. Two is small enough that a different
pair silently swapping in would leave every other check green.
### The budget form put back the $9,000,000 the GMP sync had just replaced, and both calls said 200
Two findings raised in review on the lock-boundary work and declined there as pre-existing rather
than regressions of that diff. They are one mistake in two shapes: **a lock orders writers; it does
not tell the second one that what it read is out of date.**
**`PUT /projects/{pid}/dev-budget` replaces the whole blob.** An editor whose screen loaded before a
"Sync GMP" or "Sync from model" click saves the hard lines that sync had just rebuilt. Serialising
the two changes nothing — the stale body lands *after* the sync, in perfect order, and the sync is
gone with a 200 on both calls. The route now carries a content-derived `rev`
(`dev_budget.budget_rev`), captured under the same lock that does the write, and answers **409** when
the budget has moved. `projects` has no `modified_at` to swap on, so the token is the blob's own
canonical-JSON hash: two callers holding the same bytes hold the same revision, and no migration is
needed.
**The comment over that lock justified it by an argument belonging to a different route.** It said
the two edits touch different line categories, so serialising lets both survive — which is
`connections.update_connection`'s reasoning, and true *there* because that route merges keys into the
blob. This one assigns over it. *A justification copied from the route that inspired the control
describes that route, not this one,* and it reads as settled precisely because it is true somewhere.
**`POST …/dev-budget/sync-from-model` committed a hard cost derived from a model the project no
longer had.** It reads the IFC and runs a full geometry takeoff outside the lock — correctly, since
holding the project lock across a parse would block every publication for as long as it runs — and a
publication landing in between leaves the committed number describing the old model under a 200
saying "synced". *The expensive read belongs outside the lock; what belongs inside is the proof that
it is still the right read.*
**And the obvious check could not have worked.** `publish_source_ifc` writes to a unique staged path
and `os.replace`s it onto the **same** final path, so `Project.source_ifc` is re-assigned the string
it already held — comparing it passes on exactly the interleaving it would exist to catch. *A
staleness check against a field the writer does not move always passes, and reads as though it were
guarding something.* `authoring_shared.ifc_identity` compares `(st_ino, st_size, st_mtime_ns)`
instead, and `os.replace` always changes the inode. It lives beside `publish_source_ifc` on purpose:
whoever changes how a model is promoted should see what detects the promotion.
`services/api/test_budget_rev.py` asserts behaviour and mutation-checks both guards — neutering the
`rev` comparison brings back the 200 that loses the synced hard line, and swapping the file identity
for the path string (what a check written against `p.source_ifc` compares) lets the mid-takeoff
republish commit. It also pins the one caller that actually races: the budget panel must send `rev`,
re-read the one the server returns, and handle the 409 by **reloading** rather than retrying — a
retry carrying the fresh token would re-send exactly the body the server refused.
**`rev` is optional, and that is a bounded decision rather than a fail-open default.** Requiring it
would refuse `build_demo_data.py`, `e2e_dome.py` and `e2e_vertfarm.py`, which write a whole budget
onto a project they have just created and race nothing. What bounds the decision is the web-source
check above, not the sentence in the route saying so.
### Two LPs granted access to one scenario, and one of the grants was never there
`proforma.share_scenario` unions a grantee into `Scenario.shared_with` over a value it has already
read, so two LPs granted access at the same moment leave one grant behind. **Both callers get a 201
naming their own person** — the response echoes the winner's list — so nothing surfaces until the
loser's LP cannot open the scenario and somebody goes looking for a grant that was acknowledged and
does not exist. `drawingset.revise_sheet` is the same shape on a sheet's `revisions` list: an
accepted revision is counted in its own response and is absent from the sheet and from the
cross-sheet register. The scenario site is gap G-10's last band-2 entry, and with it **the ORM
read-modify-write sweep closes outright: 43 sites, 43 reasoned exempt or locked, 0 named open.**
*That number was rewritten at the moment it became true, not before.* On the base this work was
built against it read "four to two, and both survivors are the `dev_budget` pair" — accurate then,
and false the instant the lock-boundary change landed and locked that pair. A count carried across a
rebase is a measurement of a tree that no longer exists, so it was re-read from `test_rmw_sweep`
rather than carried forward. The same failure this file records about narrative copies of a pinned
number, arriving by the other door: not prose drifting away from a gate, but prose staying still
while the gate moved under it.
**A lock, not a compare-and-swap**, for the reason `connections.update_connection` already states in
this file: two grants are *independent* claims, so serialising lets both survive where a 409 would
refuse an edit that does not conflict.
**`db.refresh` inside the lock is half the fix, and the half a lock alone cannot supply.**
`_scenario_for` loads the row *before* the wait, so without the refresh SQLAlchemy's identity map
hands the loser the list it read on the far side of the lock and the lock serialises two writes of
the same stale value — perfectly ordered, and still losing a grant. *A lock makes writers take
turns; it does not make the second one re-read.* The key is the scenario, not `Scenario.project_id`,
which is nullable.
**The ledger keyed the drawing site on `m.data`, and that is the milder half of what is wrong there.**
The markup tag it names is guarded by `"carried_from" not in d2` and has no other read-modify-writer;
the `revisions` list beside it loses an accepted revision outright. *A derivation keyed by attribute
names the site it matched, not the worst thing happening inside it* — so a finding's title is a
starting point for reading the function, never a summary of it.
**The first fix was wrong in a way that looked like tidying.** Hoisting the body into a
`_revise_sheet_locked` helper moved the `with` into the *caller*, and `test_rmw_sweep`'s
`under_pid_lock` went red on exactly that: a lock it cannot see in the function it is asked about is
a lock it cannot verify. Inlined instead. *A refactor that relocates a guard out of the scope a
checker examines turns a verified claim into an asserted one.*
`services/api/test_share_revise_lock.py` asserts BEHAVIOUR — both grants survive, both revisions
survive, and with the lock neutered the same forced interleaving loses one of each (`shared_with`
comes back holding only `lp-a`, `revisions` only `C`). A test that grepped for `pid_lock` would pass
on a lock wrapped around the assignment alone.
### LOCK-XPROC — the cross-process lock was proven by two threads in one process
**CodeQL flagged this file HIGH on its first push, and it was right.** The connect-failure message
printed the DSN verbatim, so a wrong port or an unreachable service container would have written
`PGPASSWORD` into the CI log in clear text. *A diagnostic is an output like any other* — and the
string most worth printing when a connection fails is exactly the one carrying the credential. **Two rounds of redaction were the wrong fix, and the alert said so by not clearing.** `_safe` and
`_scrub` were both correct and neither closed it: CodeQL traces the *flow* from the environment read
to the printer, and `str.replace` changes the value without removing the edge. The DSN no longer
embeds the password at all — libpq reads `PGPASSWORD` from the environment the children already
inherit, so putting it in the URL bought nothing and put the credential into a string that is handed
to subprocesses, set as `DATABASE_URL`, and printed on failure. *Sanitising a sink answers "is this
output safe"; removing the source answers "is this value sensitive at all", and only the second makes
the question stop existing.* The scrubbers stay as defence in depth, and review narrowed what they
actually claim: `_safe` redacts userinfo **and** a `password=` query parameter — libpq accepts the
secret there, so the `@` split alone never saw it — while `_scrub` removes only `PGPASSWORD`'s value
from child output and derives nothing from a caller-supplied `AEC_TEST_PG_URL`. The first wording
here said they "cover a caller-supplied URL", which was wider than the code. *A claim about a guard's
coverage is a claim like any other, and it is checkable the same way* — so both the function and the
sentence were corrected, not just the sentence.
**And fixing the sink it named exposed a second one.** The children are handed the DSN through
`DATABASE_URL` in their environment and their stdout and stderr are reported in a failure detail, so
CodeQL flagged one HIGH after the other was cleared. *Fixing the sink that was reported closes that
sink* — the same lesson this branch's lock work keeps arriving at, met again in the security rules.
Captured output is scrubbed **where it is captured**, not at each print: a sanitiser applied at every
sink is a list that has to stay complete, one applied at the boundary is a property of the value.
That call is pinned by reading this file's own source, because `_run_pair` never runs on a machine
with no PostgreSQL server — so the guard with the widest blast radius was the one no behavioural
check here could reach. *A check that exercises a helper is not a check that the helper is called.*
The first draft of the scrub assertion read `… if os.environ.get("PGPASSWORD") == "sekret" else True`
— **vacuous on every ordinary run**, the same shape as the `web_files()` sortedness check two entries
below. Caught before it was pushed, by the entry about it. The password is a parameter now. It is
redacted, splitting on the **last** `@` rather than the first, because a password may contain one
and a first-`@` split leaves the tail of the secret in the log. Three assertions cover it, since the
only place it runs is a failure path a green run never takes: *a guard that runs only when something
has already gone wrong needs a test that runs always.*
`test_pid_lock_xproc.py` is named for cross-process serialisation and asserts a great deal about it.
Its one exclusion check is **two threads, in one process, on SQLite** — the suite's default database,
where `_advisory()` takes nothing and yields `False`, so the exclusion it observes is entirely the
in-process `RLock`. `pg_advisory_lock`, the whole of R35-PIDLOCK-XPROC, was therefore exercised by no
line of this tree and could have shipped acquiring nothing. **A test named for a property can assert
every neighbouring property and never the one in its name.** What it does prove across processes is
that two workers derive the same KEY: necessary, and silent on whether taking it excludes anybody.
`services/api/test_pid_lock_pgxproc.py` asks the question. Two real OS processes contend for one
project's lock against a real server; their held intervals must not overlap. Two mutations, because
either alone passes a lock that has stopped locking: two DIFFERENT projects must overlap — otherwise
the positive check is measuring the order the harness starts things in, and a lock that excluded
everything would look identical — and a per-process advisory key must overlap, which is note 1's
`hash()` defect reinstated across real processes.
Verified by deleting the acquisition outright rather than by reasoning: the behavioural check reds
independently of the static one. One check *stayed green* under that mutation and had its label
narrowed rather than left standing — `cross_process_status()` reports the DIALECT, so it says the
advisory path was available, never that a lock was taken.
It also pins the acquisition as the BLOCKING form. `_advisory` sets `acquired = True` without reading
a result, because `pg_advisory_lock` returns void; a `pg_try_advisory_lock` "don't block the request"
change would leave the code reporting a lock it does not hold. That static pin is redundant wherever a
server exists — with the swap applied the behavioural check reds too, deterministically, since B starts
only once A is demonstrably inside. It earns its place on the run with NO server, where the exclusion
arm cannot run at all.
With no server it does not pass quietly: it asserts that `.github/workflows/db-migrations.yml` still
runs it with `AEC_PG_REQUIRED`, so deleting that step reds the ordinary suite instead of silently
removing the only place the check is real — the construction `services/api/test_pin_pgnull.py` uses,
for the same reason.
### RMW-LOCKBOUND — a bounded wait, and the correctness bug that bounding it nearly introduced
`pid_lock.mutating` serialises sidecar read-modify-write per project with `pg_advisory_lock`, which
blocks until the holder releases — **with no ceiling**. All 30 call sites are plain `def`, so FastAPI
runs them in the threadpool: a stuck holder does not wedge the event loop, it pins threadpool
workers, and Starlette's default pool is 40. *The entry on record described it as blocking the
server, and named 11 call sites. Both were wrong, and the mechanism mattered more than the count —
"it blocks the event loop" and "it exhausts the threadpool" send a reader to different fixes.*
`SET LOCAL lock_timeout` now bounds the wait; a timeout raises `LockTimeout(TimeoutError)`.
**Three premises, each probed against PostgreSQL 16 rather than recalled.** That `lock_timeout`
bounds an ADVISORY lock is not stated in so many words anywhere, so it was measured: SQLSTATE
`55P03`. That a plain `SET` is connection-scoped and rides the pool back out was measured too —
1500 ms still in force on the next checkout of the same backend — which is why the statement is
`SET LOCAL`, and why the advisory lock surviving that `SET LOCAL`'s commit had to be measured as
well. Any one of the three being false sinks the shape.
**The third premise is the dangerous one.** A lock timeout and an unreachable server raise the *same*
SQLAlchemy class; only the sqlstate separates them. `_advisory` already catches broadly and degrades
to in-process, loudly — correct for "there is no Postgres here". Caught as `except OperationalError`,
a timeout lands in that same branch and the contended write **proceeds having taken no cross-process
lock at all**. Bounding the wait would have converted an availability problem into a silent
correctness one, strictly worse than what it replaced. *A bound is only safe if it cannot be mistaken
for the absence of the thing it bounds.*
**Two bugs in the first draft, both found by the gate rather than by reading.**
`SET LOCAL lock_timeout = :ms` is a **syntax error** — `SET` takes no bind parameter — so it did not
merely fail to apply the timeout: it raised, fell into the degrade branch, and let the write proceed
unserialised. The exact failure above, reached by a door it did not anticipate, in the change written
to prevent it. And the static check read its own **prose**: `"SET lock_timeout" not in src` failed on
*correct* code, because the comment explaining why a plain `SET` is wrong contains that string.
*A check that greps a source file is reading whatever a future author writes ABOUT the code alongside
the code — and prose is exactly where the forbidden form gets named.* Comments and docstrings are now
stripped, with a self-test in both directions.
**A measured limit, stated rather than hidden.** The degrade-branch mutation **survives** a
static-only run: the sqlstate constant stays in the source, unreachable. Only the two-process arm
sees it, and that needs a server the API gate's job does not have — so it runs in
`.github/workflows/db-migrations.yml`, and `services/api/test_pid_lock_bound.py` asserts that step is
still there. *A gate whose real arm runs in exactly one place is one deletion away from decoration,
and the deletion is in a different file from the gate.*
Four mutations: removing the `SET LOCAL` reds five checks (B waits out the full hold); folding the
timeout into the degrade branch reds two (B enters the critical section having timed out and written
anyway); deleting the CI step reds two; dropping `AEC_PG_REQUIRED` from it reds one.
**Review round 2 found the lock's lifetime was wrong in BOTH directions, and the fix is the shape
that satisfies neither constraint alone.** A session-level advisory lock lives on a connection, so
its lifetime IS that connection's lifetime — and two requirements pull opposite ways:
* **End the transaction and the connection goes back to the pool.** Measured under contention: the
backend changed between acquisition and unlock, `pg_advisory_unlock` returned `False`, and the real
lock stayed stranded until recycling while another caller held that connection.
* **Leave it open across the critical section and `idle_in_transaction_session_timeout` kills it.**
Measured with that timeout at 1 s and a 3 s section: the acquiring session was terminated, the lock
died with it, and a rival writer walked in. **A lost update** — precisely what the lock exists to
prevent, arriving through the fix for the other half.
A `Session` can satisfy neither cleanly. A checked-out `Connection` whose acquisition transaction is
*committed* satisfies both: the connection is held to `close()`, nothing is left idle in a
transaction, and `SET LOCAL` still reverts before the connection is released.
*Three of this change's verifications were themselves wrong before they were right, each in the same
shape.* The connection probe cleared the pooling bug because an idle pool hands the same connection
straight back — five rival Sessions were needed to tell "retained" from "released and re-issued". The
static check for it matched a substring that tokenising makes unmatchable. And the idle-timeout check
set the GUC on **its own** connection while `pid_lock` opens another, so the Session-shape mutation
sailed through it; it now sets it on the DATABASE, which is also how a managed PostgreSQL imposes it.
**Each one reported success while measuring nothing, and only a mutation distinguished them from a
real check.**
### DESKTOP-CONVERT-TIMEOUT — the parameter reached one of the two branches
`fragconvert.convert_ifc` has always taken a `timeout`. It reached `subprocess.run` on the Node
path, where an overrunning child can be killed, and reached **nothing at all** on the Python path,
which calls IfcOpenShell in the caller's own process. `_publish` runs on a background worker and
does not care. **`edit_preview` is a synchronous route passing `timeout=120`**, so a slow model held
a request worker past its own deadline instead of returning the 503 that route is written to give —
and the parameter's *presence* is what made the gap invisible.
It was filed rather than fixed because the obvious repair is a killable child process, which is
net-new spawning machinery in a PyInstaller bundle. **That was the right call about that repair and
the wrong conclusion about the problem**: `from_ifc.convert` already had a per-element loop with a
progress hook, so a *cooperative* deadline fits with no process isolation at all. It now checks the
clock after the parse, between elements, and every 4,096 entities in the index loop; `convert_ifc`
computes the deadline from the caller's `timeout` (not the duration — the callee would restart the
clock, and `edit_preview` authors a one-element IFC before it calls). Both paths raise
`TimeoutError`: Node's `subprocess.TimeoutExpired` is a `SubprocessError`, so telling "too slow"
from "broke" meant knowing which converter ran, which is the fact this function exists to hide.
**The entry stays OPEN, deliberately — and the first draft of this paragraph overstated what the
fix does.** It said the change "bounds the cost that scales". It does not: `ifcopenshell.open` is a
single call whose time scales with file size, and the checkpoint is *after* it, so a large file is
parsed in full and only then refused. The accurate claim is **enforced at phase boundaries and
within the two loops** — the geometry loop per element, the entity index every 4,096 — with the
parse and any one hanging `create_shape` still outside every checkpoint. *Caught by re-reading my
own claim against the code before review did, which is the only reason it is not in the shipped
prose.* *A partial fix described as a fix is how the original asymmetry became invisible in the
first place.*
**Two defects in the gate, both found by its own mutations rather than by review.** Reinstating the
pre-fix state red it, but deleting the parse and geometry checkpoints while leaving the entity-index
one **passed** — that tick fires at entity 0, so the deadline was still detected, after all the
geometry work was done. The timing assertion meant to catch that was vacuous: `elapsed < max(0.25,
baseline * 0.9)` against a 33 ms fixture is satisfied by doing the whole conversion. *A threshold
generous enough not to flake on a slow runner is generous enough to measure nothing.* The check now
asserts the **stage name** in the raised message — a deadline already past must be caught at
`parse`, before any geometry — and a second case crosses the deadline mid-loop through a deliberately
slow `progress` hook, so both checkpoints are load-bearing. And the always-raise mutation used to
kill the run with a traceback instead of naming a contract; the positive control now catches it.
The first draft of that positive control also failed on *correct* code, asserting byte equality
between two conversions — `metadata` embeds `"created": datetime.now(...)`. It compares the parsed
model instead. *An assertion that fails on correct code costs a round exactly like one that passes on
broken code.*
**And forcing one branch to reach the code under test stopped testing the other.** Every check above
sets `have_converter()` false to drive the Python path, so the Node half of "both paths raise
`TimeoutError`" was asserted by nothing — a regression letting `subprocess.TimeoutExpired` escape
would have passed the gate that exists for exactly that promise. Raised in review. *The act of
forcing a branch is itself an act of un-testing its alternative, and it leaves no trace in the
output.*
Two arms, because neither covers the other. **Injected**: `subprocess.run` raises `TimeoutExpired`
directly, proving the handler converts it; runs everywhere. **Real**: a Node process that does not
finish, under a real budget, proving the whole chain including the argument being passed. Measured
rather than argued — with `timeout=timeout` deleted from the call, the injected arm still **passes**,
because its exception came from the stub and not from the runtime. The real arm needs a `node` the
API gate's job does not install, so it is conditional, and a conditional arm that skips everywhere is
a check reporting good news: the arms that ran are named in the output and at least one is required.
**The real arm's first draft turned that mutation into a hang rather than a failure.** Its script only
blocked, so dropping `timeout=` left `subprocess.run` waiting for ever — caught, in the sense that the
gate never went green, but reported as nothing at all and paid for with a CI job's whole budget. The
script now exits on its own timer too, so the same mutation returns normally and the arm fails **by
name**, and a margin check separates "gave up at the budget" from "waited for the script". *Make a
check fail; do not settle for it not passing.* The lane table's row for this item was also still
carrying the pre-fix text two paragraphs of narrowing later — and its stated REASON, "the fix is
process/packaging shape", is why the item read as unactionable for thirteen days: that is the repair
the residue needs, recorded as the repair the whole item needed. *A correct decision with a wrong
reason attached is worse than no reason, because the reason is what the next reader plans against.*
### An exclusion whose own differential is blind to what it wrongly excludes
The vendored-tree exclusion below shipped as `"/src/vendor/" in q` — a match on the *parent*
directory rather than on the packages. Review named the hole, and it is the same shape the exclusion
exists to close, one level out.
A first-party caller added under `apps/web/src/vendor/` would be dropped from the blob. **And the
differential beside the exclusion cannot see that**, because it asks what restoring the vendored text
*vouches for*: if the new caller calls `/projects/{pid}/workflow/{key}`, the vendored copies already
vouch for that route, the difference is unchanged, and the route stays frozen as uncalled while the
client calls it. *A differential over what an exclusion REMOVES says nothing about what it removes
WRONGLY* — a file wrongly excluded is absent from **both** sides of that subtraction, so it moves
nothing and is invisible.
The excluded set is now named — `vendor/massingifc/`, `vendor/massingpdf/`, matched on the path
relative to the web root. Two checks, because the first alone would rot:
- The tuple must equal the directories under `vendor/` carrying a `VENDOR.md`, **in both
directions**, so the list cannot drift from the tree.
- A synthetic sibling package the tree does not contain must still be READ. Every real directory
under `vendor/` is vendored, so the tree *cannot* distinguish a named exclusion from a
parent-directory one — the same empty-population trap that made the `.tsx` sort check vacuous.
The list is literal rather than derived from the `VENDOR.md` markers, deliberately: a blob that
decides what to read from a marker file changes meaning when somebody deletes one. The marker checks
the list instead. *Hardcode what is load-bearing; derive the proof that it is right.*
Reinstating the parent-directory match reds the sibling probe **and nothing else**, which is the
finding restated as a measurement: no check that existed could catch it. Dropping a package from the
list reds five, the derivation check among them. And the fixture itself was wrong on its first run —
it restored `glob` from a name bound two hundred lines further down the file, so it crashed instead
of asserting. *A fixture that restores from a name defined later restores nothing.*
### Looking one way down the list is not reading the list
The round below stopped asking "is it written down" and started asking "can it fail the job", and
then answered it by looking at the separator that **follows** the invocation. A review pass named two
shapes that pass with the status masked by nothing at all:
- `true || python <file>` — nothing follows the invocation, so nothing is masked, and bash never
reaches it. Reading one token forward is not reading the command list.
- `python <file> && echo passed` on one line, `echo done` on the next. `bash -e` suspends itself for
every command of a `&&`/`||` list except the last, so a failing test does not end the step; the
list's own status is then discarded, because a later list runs and the step exits with `echo
done`'s zero. **The single-line form of that same text is correct** — there the failing list is the
last one and its status *is* the step's — and the check added in the round below tested only the
single-line form. *A shape and the same shape one line longer are different programs.*
`bash -e` reasons about command **lists**, not about commands, so the guard now does too: a `run:`
block is cut into lists at `;` and at line ends, each list into segments at `&&`/`||`/`|`/`&`, and an
invocation counts only when its status can reach the runner — nothing masking after it, no `||`
before it in the same list, and, if the list is conditional at all, that list must be the last in the
block. A `&&` *before* the invocation is deliberately still accepted: `cd services/api && python
<file>` is the ordinary idiom, and the same limit `_parked` already states applies — a constant
written to be false is caught, a condition that merely evaluates false is not.
Three new self-tests, and the command test is split out from the control-flow test so each can be
mutated alone. Deleting either guard reds exactly its own self-test and nothing else; widening the
conditional-list rule to reject *every* conditional list reds four positive checks, including the
shipped step's own shape. Three further mutations against the real `db-migrations.yml` — the step
behind `true ||`, the step as a two-line `&&` block, and the status masked with `|| true` — each take
the found-step count to zero.
### "Does it run" and "can it fail the job" are different questions
I had asked review whether `_invokes` could ever fail **open**. It can, three ways, none of them the
way I asked about — and all three are the same class the last four rounds were about, one level
further out than any of them reached.
**The step can be skipped, its failure can be ignored, or its exit status can be thrown away.**
`if: false` is the ordinary way to park a step; `continue-on-error: true` runs it and shrugs;
`python <file> || true` discards the status, and `python <file> | tee log` hands it to `tee`, because
GitHub runs `bash -e {0}` with no `pipefail` unless `shell:` says otherwise. In the last two the
file *is* executed — which is exactly what the previous rounds established — and in the first it is
not executed at all; what all three share is that the `run:` text, the `AEC_PG_REQUIRED` value and
the step selection are identical to a live step, so every check added so far stays green while CI
asserts nothing. (The first draft of this paragraph claimed execution in all three, which `if: false`
plainly contradicts — found in review. *A sentence generalising over cases it lists is worth
re-reading against each of them.*) *Each round asked a sharper version of "is it written
down" and none asked "can it fail".*
The separator that **follows** a command segment now decides whether it counts (`;` and `&&` keep the
status; `||`, `|` and `&` do not), and a job or step that is parked or cannot fail is excluded.
Explicit `continue-on-error: false` still counts, however it is spelled — `bool("false")` is `True`,
so the obvious predicate would have red a correct workflow, and a gate that cries wolf gets deleted.
Only the *constant* false is treated as parked: a real condition is not evaluated here and is not
assessed, which is stated rather than implied.
**And the budget panel's reload was still racing the editor.** Holding the request slot across the
conflict reload stops a save from *leaving*; it does nothing about the keystroke. `lines.splice`
replaces the array and `paint` rebuilds the inputs from it, so an edit typed in that window is gone
from both the model and the screen, and the queued follow-up then saves the server's copy back.
Editing is now closed while the reload runs and restored by `paint` — which is also what the reload
calls when it finishes, so a disable applied anywhere else would be undone by the next repaint.
Reconciling a keystroke against a list replaced wholesale needs a rule nobody has chosen, and
guessing one silently is how a budget acquires a number no one typed.
Seven new self-tests. Four mutations, three of them against the real workflow: the step's failure
masked with `|| true`, the step parked with `if: false`, and `continue-on-error: true` — each takes
the found-step count to zero.
### Three fixes that each narrowed the right thing and stopped one question short
A second review pass over the round above found one more defect behind each of its three fixes. The
shape repeats, which is the interesting part: every one of them narrowed the correct subject and
then asked the old question about it.
**Serialising the requests was not serialising the state.** One save in flight stops two sends
carrying one `rev` — but `again` only covers a send *that flow* queued, and says nothing about a
`setTimeout` the debounce armed while the save was on the wire. That timer fires during the reload
carrying the pre-conflict lines and the stale rev (another self-inflicted 409) or after it, carrying
the server's own copy straight back. And the reload is asynchronous: releasing the in-flight slot
when the *save* settles lets a keystroke start a request against a `lines` that is mid-replacement.
The 409 path now cancels the pending timer and holds the slot until the reload resolves. *The reload
mutates the state every request is built from.*
**Extracting the right workflow step was not asking whether that step runs anything.** The guard
matched a step whose `run:` merely *contained* the filename — so commenting the invocation out, the
ordinary way somebody parks a slow step, leaves the substring in place, the step is still found, its
`AEC_PG_REQUIRED` is still set, and the guard reports that CI runs this file while CI runs nothing.
Comments are stripped per line and the name is compared as a whole basename. The `#` split is crude
and fails in the safe direction: a `#` inside a quoted string can only *lose* an invocation, which
reds the caller, because the caller requires exactly one step.
**And that first fix was still wrong, for the reason it was written.** It rejected a commented-out
invocation and a longer filename and went on accepting `echo test_pid_lock_pgxproc.py` — review
declined to close the finding on exactly that ground. *Excluding the shapes you were shown is not
answering the question those shapes were examples of.* The head word of the command segment holding
the name must now be a Python runner, or be the script itself. The runner list can go stale, and it
does so in the direction that gets noticed: a missing runner makes the step invisible, the caller
requires exactly one step, the build reds, somebody adds the name — while the opposite error passes
quietly. Emptying the list is a mutation and it reds three self-tests. *When a list must be
incomplete, put the incompleteness where it fails loudly.*
**The positive control earned its place the same hour.** The first version of that narrowing matched
the filename as a whole token before asking what the command was, and the token pattern excludes a
preceding `/` on purpose — so a direct exec `./test_pid_lock_pgxproc.py` never reached the command
test. Comparing basenames answers both questions at once with no lookbehind to get wrong. Without a
check that real invocations are still *accepted*, that would have shipped as a stricter-looking gate
that had quietly stopped recognising a legitimate way to run the file.
**And then the claim I put to review turned out to be false.** Asking whether the command split
could ever *invent* an invocation, I asserted that every parsing error loses one instead — and
measured it before the answer came back. `echo "a; python test_pid_lock_pgxproc.py ; true"` returned
true: a regex split cannot see quotes, so a `;` inside a string ended the `echo` early and handed the
rest back as its own command. *A claim about which way a heuristic fails is a claim like any other,
and this file's entire subject is that an unchecked one reads as settled.* The line is now tokenised
with `shlex` in POSIX mode with `punctuation_chars`, which respects quoting for the comment marker
**and** the separators, so one change closes both — including a case the character-level strip got
wrong in the other direction, where a quoted `#` swallowed a real invocation after it. Per line,
because a `run: |` block is a sequence of commands and newlines are whitespace to `shlex`. An
unbalanced quote raises and that line is skipped: unreadable is not "runs it", and the caller
requires exactly one step, so skipping reds rather than vouches.
**Merging the environment in force was not reading the value.** `AEC_PG_REQUIRED: ""` is exported by
GitHub as an empty string, which the consumer reads as opted *out* — so the key-presence check passed
a workflow whose step would take the no-server branch and exit 0. The predicate is now derived from
the consumer rather than restated, which is also why `"false"` and `"0"` count: a workflow `env:`
value is exported as a string, and any non-empty string is truthy there. That is a fact about GitHub
Actions, not a choice.
Four mutations, two of them against the real workflow — commenting out the invocation, and setting
the flag to `""` — plus two on the panel. The cross-process gate was re-run against a live
PostgreSQL 16 after the change: 24 checks, all green.
### The budget panel manufactured its own 409, and two of this PR's own checks were measuring the runner
Three findings from a full review pass on the head that added the `rev` precondition. All three are
inside work landed earlier in this same branch, and the first is a product defect the precondition
itself created.
**A debounced save that races itself supplies its own stale write.** `clearTimeout` cancels a save
that has not left yet; it does nothing to one already on the wire. On a connection slower than the
500 ms debounce, two saves go out carrying the same `rev` — the server commits the first, refuses the
second, and `reloadAfterConflict` replaces the user's newest keystroke with the server's copy while
telling them "the development budget changed elsewhere (a GMP or model sync, or another editor)".
*A staleness token turns a lost write into a refused one; it does not decide WHOSE write is stale.*
One send is now in flight at a time, the body is built when the request starts, and a keystroke
during a save sets a flag rather than queueing a second request — so one follow-up send carries every
edit made while the first was in flight, instead of N sends of the same final state.
**The workflow guard searched the whole file for a flag its neighbour also sets.** With no PostgreSQL
server the cross-process gate cannot run its assertions, so it proves instead that
`db-migrations.yml` still runs it *with* `AEC_PG_REQUIRED`. It asked whether that string occurred
anywhere in the workflow — and the pin-sweep step sets it too. Measured: deleting the flag from this
file's own step leaves three occurrences and the old check green, so the CI step would have taken the
no-server branch and exited 0 with nothing exercising the advisory lock anywhere. The workflow is now
parsed, the step that runs this file is extracted, and the flag is looked for in the environment
actually in force for *that* step — workflow, job and step `env` merged, so hoisting the flag to the
job stays valid. *A check scoped to the FILE cannot speak about a STEP, and a neighbour's correct
configuration is what makes the difference invisible.*
**And two timing checks had a window that was a constant.** Child A held the lock for a fixed 1.5 s
while the parent started child B; if B's interpreter, import, status call and connection took longer
than that, B entered after A left, and the two MUTATION checks — which *require* an overlap — failed
on a lock working perfectly. A now waits for B's own log line before starting its hold. Measured on
a real server: with the hold shrunk to 50 ms, the old shape fails both mutations and the new one
passes all three. *A timing check whose window is a constant is measuring the runner.*
The extractor's three self-tests run on **every** invocation, including the one with a server —
otherwise the only branch that exercises it is the branch a broken extractor would make vacuous.
### Round 16: the RVT translation was still on the event loop, and the sort check could not fail
A full review pass over the current head found two, and both were inside this PR's own earlier fixes.
**The APS translation in `import_rvt` still ran on the event loop.** Round 12 moved `staged.write_bytes`
off it, with a comment explaining that a converted IFC is hundreds of megabytes and blocking the
coroutine freezes every other request in the process. The call that *produces* those bytes —
`aps.translate_rvt_to_ifc`, which `convert.py` documents as running "for many minutes" and already
sends to the threadpool for that reason — sat directly above it, unwrapped. *Fixing the blocking call
you are looking at is not the same as fixing the blocking call that dominates the route*, and the
smaller one was the one that already had a comment pointing at it.
**And the new determinism check could not fail.** `web_files()` sorted inside its glob loop, yielding
`[every .ts sorted] + [every .tsx sorted]` — deterministic, but not sorted. The assertion that it is
sorted passed anyway, because the tree holds **0 `.tsx` files against 658 `.ts`**: the fix was
accidentally right and the check was accidentally green. The day somebody added a `.tsx` the check
would have gone red with a message blaming the glob — *a check whose failure message can misdiagnose
is worse than one that stays silent, because somebody acts on it.* The sort is now one pass over the
whole list, and the contract is probed by feeding a synthetic `.tsx` through the real function, since
the tree cannot supply one.
*A check that passes because its population is empty has the same shape as one that passes because
the code is correct.* Reverting either fix reds exactly one assertion, naming the right cause.
### The "CI flake" in the reachability gate was a backtick in a comment, and the answer depended on directory order
`test_route_reachability` went red on 2026-09-23, passed on a re-run of a byte-identical tree, and
went red again on the next push. It was filed as a transient of unknown cause. It is neither
transient nor unknown: **one call in the file handed `leaf_is_called` the RAW web source instead of
the comment-stripped one**, and that one argument made the gate's answer depend on the order
`glob.glob` happened to return ~400 files in.
`string_blob`'s template-literal alternative spans newlines, because a real template literal does.
This tree quotes identifiers in backticks throughout its **comments**, so an unpaired backtick in one
comment opens a phantom template that closes at the next comment's backtick — and everything between
is emitted as a single string body **with its inner quotes intact**. A leaf whose only vouching
occurrence is a quoted word then sits in that body preceded by `"` rather than by the NUL that
anchors "opens a string", and reads as *uncalled*. Which comments pair with which depends entirely on
the concatenation order. Measured: **1 of 25 shuffled orders flips `workflow`**, and
`string_blob(BLOB)` ranges from **933k to 2,014k characters** across orders; read through the stripped
source, the derived verdicts are identical in every order.
*A gate whose answer depends on directory enumeration order cannot be reproduced from its own
failure* — which is exactly why the first instance was dismissed. "Flake" was a description of the
symptom standing in for a cause, and it bought a second red build.
**Fixed four ways, because the one-word fix is the one that drifts back.** The call is stripped;
`web_source.web_files()` is split out and **sorted**, so the blob is at least the same blob twice;
and both properties are asserted — a four-line fixture that reproduces the backtick swallow without
depending on anything in the tree, and an order-invariance check that rebuilds the real blob reversed
and requires the same uncalled set. The fourth is the one that matters most: **the file now reads its
own source** and fails if any call passes the raw blob, because neither behavioural check reds when
somebody writes `BLOB` there again, and *for a 1-in-25 defect "run it again" is not an alternative to
a check.*
All four are mutation-checked — reverting the call, inverting the fixture, blinding the AST scan, and
dropping `sorted` each red exactly one assertion, with a message that names the right cause. The
`sorted` assertion exists precisely because dropping it changes no verdict today: *the change nobody
can measure is the change nobody notices reverting.*
### LOCK-BOUNDARY round 15 — the answer collected and then discarded, and a test sharing a directory with its own failures
Two findings. The first is inside round 14's fix; the second is a test-hygiene defect that would have
broken the suite permanently for anyone whose run died at the wrong moment.
**1. `pid_lock_names` collected the bare-lock names and returned a boolean.** The names were gathered
into `bare_names`, the shadowing was subtracted correctly — and then the function answered yes/no, so
`is_the_lock` fell back to the literal spelling `mutating`. Both directions are wrong at once:
```python
from aec_api.pid_lock import mutating as held
def writer(db, pid, mutating):
with mutating(pid): # the PARAMETER certifies
p.source_ifc = "x"
```
`bare_names` is `{"held"}`, `shadowed` holds `"mutating"`, the intersection is empty, the flag says
yes — about a name nobody bound to the lock. And the genuine `held(pid)` is **never** certified, so
the docstring promising the bare form was false for the one spelling that actually reaches it.
*Asking the right question and then throwing the answer away* — the lesson
`services/api/test_pin_pgnull.py` already records, arriving here by a different door. `lock_spans`
now checks membership in the names the import bound. Probes `aliased_bare` (must certify) and
`bare_alias_shadow` (must not); restoring the boolean reds them by name.
**2. `test_ifc_publish_atomic` ran in a fixed `./_ifc_atomic` and never removed it.** That file
deliberately *keeps* one `.rollback-*` backup and then asserts there is exactly one and that no other
rollback scratch is left behind. In a shared directory those two assertions are armed by any run that
dies between creating the kept backup and the cleanup at the end — a crash, a timeout, a cancelled CI
job — and then **every later run fails**, for a reason unrelated to the code under test, until
somebody deletes the file by hand. `run_tests.py` reports `_ifc_*` as residue it does not own, so the
runner never clears it either.
**Demonstrated, not argued**: with the old fixed directory and a single planted
`.rollback-stale-from-a-crashed-run`, the run fails on *both* named assertions. The directory is now
`tempfile.mkdtemp()` per run, removed only on a pass — a failed run's staged files, backups and
published bytes are the evidence somebody will want. *A test that asserts "nothing was left behind"
must not share a directory with its own previous failures.*
### LOCK-BOUNDARY round 14 — the same fail-open, in the two spellings the fix did not look at
One finding, and it is inside round 13's own fix.
Round 13 closed a fail-open where `_lock_import_names` accepted `pid_lock` imported from **any**
module, so `from .fakes import pid_lock` certified as the project lock. The repair was to recognise
the lock by its **source module** — and the docstring said exactly that. The code under it went on
matching a **suffix**:
- `(node.module or "").endswith("pid_lock")` accepts `from .fake_pid_lock import mutating`,
`from .fakes.pid_lock import mutating`, and `from tests.pid_lock import mutating`. Each returns
`mutating`, `pid_lock_names` returns `bare=True`, and `lock_spans` certifies `with mutating(pid):`.
- the `ast.Import` branch matched `a.name.split(".")[-1]`, so `import fakes.pid_lock as pid_lock` is
certified too — and for `import x.pid_lock` **without** `as`, Python binds `x`, not `pid_lock`, so
the analyser was recording a binding the language does not make.
*A fix aimed at the spelling that was reported closes that spelling.* The rule was right and stated
plainly one paragraph above the code that ignored it, which is the part worth keeping: **the prose
was true and the test was not**, and prose agreeing with itself is not evidence. Recognition is now
by exact module path — relative `pid_lock` or absolute `aec_api.pid_lock`, with `level` checked so a
top-level module of that name cannot stand in — and the `import` form is accepted only when aliased,
the one shape that binds a usable name.
Two probes added, `fake_bare_import` and `fake_module_import`; restoring either the suffix test or
the last-segment match reds the violation list by name.
### LOCK-BOUNDARY round 13 — a live unlocked writer the analyser could not see, by construction
Four findings, all real. One is a **product defect**, not a gate defect, and it is the kind this
whole pull request exists to close.
**1. `bim.patch_project` swapped `Project.source_ifc` outside the lock.** `ProjectPatch` declares
`source_ifc`, so an admin PATCH can move the pointer that `bake_layers`, `edit` and every authoring
route read, spend a recipe on, and write back — the same race the five writers in this PR's title
had. It survived twelve rounds of a gate built to find exactly this, **for a structural reason
rather than an oversight**: the write is `setattr(p, k, v)`, which carries no `Store` context and no
`Attribute` node, so an analyser that asks the grammar for attribute writes is blind to it however
carefully it reads.
`_stored_attrs`' docstring claimed `ctx=Store` was *"the complete, exact marking of an attribute
write in EVERY spelling."* It is every **syntactic** spelling — a different and smaller claim. Python
spells attribute writes two ways, syntactically and through the reflection API, and *a derivation
that asks the grammar is only as complete as the grammar's own account of what it is looking for.*
`dynamic_writes` now covers `setattr`, judged by the same `lock_spans` traversal keyed on the call
node; the route takes `pid_lock.mutating(pid)` around the lookup, the refresh and the commit.
**The gate found the defect on its own once taught the spelling** — it reported
`patch_project writes Project.<setattr> with mentions outside the lock at [759]` before the fix.
The dynamic rule is coarse in the fail-closed direction — an unlocked `setattr` on a model with
*any* protected attribute is a violation, because the analyser cannot prove it wrote a different one
— and that is affordable because the population is seven sites tree-wide, measured. Three receivers
do not resolve (two ifcopenshell entities, one `Viewpoint` reached through a generator) and are
declared in the exemption ledger with stated reasons rather than inferred away.
**2. The shadowing list was short again — a fifth family.** `case pid_lock:` binds through
`MatchAs.name`, with no `Name` node anywhere, so a captured subject certified as the project lock.
`MatchAs`, `MatchStar` and `MatchMapping.rest` are now handled. The sting is where the claim was
made: round 12's docstring said the list of string-field binding forms was **"closed by the
language"** while naming four of seven. *A completeness claim about a hand-written list is a claim
the list cannot check* — asserted in the very sentence explaining why enumerations fail. The
docstring now says what is true: as complete as the last reading of the grammar.
**3. Lock recognition disagreed with itself in both directions at once.** `_lock_import_names` and
`pid_lock_names` each matched `a.name == "pid_lock"` from *any* module, so
`from .fakes import pid_lock` certified — the `import_shadow` probe missed it because that probe
renames and this substitution does not. *A probe written against one spelling of a substitution does
not cover the substitution.* And in the other direction, round 12's own shadowing fix made
`from ..pid_lock import mutating` **unreachable**: the import bound no lock name, so `"mutating"`
entered the shadow set and `bare` could never be true. The form the docstring promised to accept had
been switched off by a fix, invisibly, because nothing in this tree spells it that way. Recognition
is now by **source module**, in one place both callers read.
**4. `_rel` emitted backslashes on Windows**, which is where this repository is developed, so the
`/`-separated keys in the exemption ledger never matched, `stamp_conformance`'s receiver fell through
to UNRESOLVED and the real-tree verdict red — **there and only there**, because the ledger self-test
normalises separators before comparing. *A check that normalises in one place and not the other is
green where it looks and red where it runs.* The self-test uses `PureWindowsPath`, which is
platform-independent, so it reproduces the Windows string on Linux instead of asserting something
that can only hold here.
Each fix is mutation-checked: dropping the match forms, matching the lock import by name alone,
returning `str()` from `_rel`, removing the `setattr` collector, and unlocking `patch_project` each
red a specific named self-test.
**One of those five did not, at first, and the gap was in the check rather than the fix.** The
Windows self-test asserted the property against `PureWindowsPath` directly and never called `_rel`,
so reverting `_rel` to `str()` red nothing — a check written beside the guard instead of through it.
The separable half is now `_posix_rel(p, root)`, which the self-test calls, and the mutation reaches
it. *A static check that still passes with the guard removed is not testing the guard* — this file
states that rule about `_system_field` and then broke it about itself one round later, which is the
argument for running every mutation rather than the ones expected to bite.
### LOCK-BOUNDARY round 12 — the third statement-type list, and a sentinel spent as a model
Four findings, all in-diff, all real. Three are in `services/api/test_lock_boundary.py`; one is a
route that blocks the event loop.
**1. The scope merge inherited the enclosing model for a name the inner scope had taken over.**
`_bindings` marked a name meaning two models `CONFLICT` and then *dropped the entry at its own
return* — so `collect`'s `dict.update` chain found nothing for that name in the inner scope and the
**outer** function's model flowed in. An unlocked `Project.source_ifc` write inside a helper that
rebinds `p` was therefore classified as the outer `SavedView`, and reached neither the violations nor
the unknowns. Two sentinels now survive the merge — `CONFLICT` for a name meaning two models and a
new `UNRESOLVED` for one this analyser cannot pin down at all — and `_resolved` removes them **after
the last merge**. *A sentinel filtered before the merge cannot shadow anything*, which is the whole
mechanism by which an inner binding is supposed to beat an outer one. The probe runs through
`collect` in a temp directory, because nothing calling `_bindings` alone can see a defect in the
merge.
**2. The shadowing check in `pid_lock_names` was a list of statement types, and the list was short.**
Third time in this analyser, third time short. It named five forms and still certified an arbitrary
object as the project lock for `pid_lock, _x = other, 1`, `from .fakes import stub as pid_lock`,
`except Exception as pid_lock:`, a nested `def pid_lock(...)` — and a **module-level** rebinding
beside the import, which a per-function scan cannot see at all although it is the one rebinding whose
blast radius is the entire file. **The fix for an incomplete enumeration is not a longer
enumeration**: the new `_bound_names` asks the grammar — `ctx=Store` on `ast.Name` is the exact,
complete marking of a name assignment in every spelling, exactly as `_stored_attrs` learned one node
type over — and names explicitly only the four forms that carry their name as a plain string field
rather than as a node (`except ... as`, `def`/`class`, import aliases, `global`/`nonlocal`). That
list is closed by the language, not by what this tree happens to contain. Module scope is now in the
chain the check scans.
**3. `NOT_A_MODEL` was spendable as a model.** The sentinel meaning *"proved not to be a mapped
instance"* is a non-empty string, so `if m and ok` accepted it and a locked write through `self` in
any non-mapped class seeded a protected pair keyed on the sentinel. Nothing ever resolves *to* it, so
no violation could match — but its **attribute** entered `prot_attrs`, the set that decides whether
an unresolved receiver is worth reporting, so unrelated unlocked writes were raised as UNKNOWN on the
strength of a pair protecting nothing.
**4. `import_rvt` wrote a whole converted IFC on the event loop** — `staged.write_bytes(ifc)` on the
coroutine, one line above a `run_in_threadpool` the same route already used for the publish. A
converted model is routinely hundreds of MB; the loop stalls for every other request while it
lands. This is the v0.3.703 SSE failure in miniature.
Deriving that shape over the tree found **three more instances, all in one other route plus the
converter**, and fixing only the line review pointed at would have left the *largest* blocker two
lines above it: `raise_plan_to_bim` ran the DXF parse, the ifcopenshell raise, a whole-file read, a
whole-file write and a `storage.put` network round trip on the loop; `convert` read its fragments
output back on the loop having just handed the translation *off* it. All are now awaited through the
threadpool. **No gate is claimed for this** — a check matching `read_bytes`/`write_bytes` would be a
completeness claim with a filter under it, since the honest population of "blocking work in an
`async def`" also includes `storage`, `subprocess` and every synchronous `db` call. That axis is
filed as its own task rather than half-gated here.
Each of the three analyser fixes is mutation-checked: filtering the sentinels inside `_bindings`
reds the scope-merge probe by name; narrowing `_bound_names` back to statement types loses all four
shadow probes from the violation list; taking module scope back out reds the module-rebinding probe;
letting `NOT_A_MODEL` seed again brings back both the phantom pair and the UNKNOWN it manufactured.
### LOCK-BOUNDARY round 11 — the third traversal, and a correction to what round 10 claimed
Two High findings, both real, both fail-opens, both in `test_lock_boundary.py`.
**1. `lock_spans` crossed nested function boundaries — and round 10's commit message said this could
not happen.** That commit asserted *"there are now exactly two function-taking traversals and both
use `_own_nodes`."* **That claim was false.** `lock_spans` is a third, and it was missed because the
search that produced the claim was `grep 'ast.walk(fn)'` — and `lock_spans` does not call `ast.walk`,
it recurses through a hand-written closure. **Searching for a spelling is not searching for the
property**, and the property was "descends into a nested scope". The same lesson this PR has been
prosecuting for four rounds, applied to my own verification of it and not caught by me.
The consequence is round 10's shape exactly: a nested helper's *unlocked* mention of an attribute made
the **enclosing** function's locked write report as unlocked; that write is the SEED; losing it means
an unlocked writer of the pair elsewhere is no longer a violation. All three function-taking
traversals now stop at `FunctionDef`, `AsyncFunctionDef` and `ClassDef`.
**2. `pid_lock_names` accepted shadowed names.** `def writer(p, pid_lock):` binds a *parameter*, and
Python resolves `with pid_lock.mutating(...)` inside that function to the parameter, not the imported
module — as does `pid_lock = other`. The analyser kept `"pid_lock"` in `lock_names` and certified an
arbitrary object as the project lock. The `impostor_alias` probe added in round 9 could not reach
this: it binds the name in a *different* function. **The dangerous case is the same scope, where the
name is right and the binding is wrong.** Names bound by a parameter, assignment, `for` target or
`with ... as` in any scope on the chain are now subtracted — removed rather than resolved, because
this is a certifier and refusing what it cannot prove is the fail-closed direction.
**And the reviewer confirmed, unprompted, the thing this round was asked to check:** *"The stable
432 sites / 6 pairs count does not detect either defect."* It was right to say so. Finding 1 can
remove one seed while a second locked writer keeps the pair alive; finding 2 can add a **false** seed
without changing the count at all. That number has been cited in five consecutive entries as evidence
the tightenings lost nothing — **it is evidence of much less than that**, and it is now retired as an
argument. The probes are the evidence; the count is a description.
Sites unchanged at **432**, six protected pairs. Each fix mutation-checked: restoring the nested
descent drops `(Project, prop_layers)` and loses `span_victim`; restoring the unshadowed names loses
`param_shadow` and `local_shadow`.
### LOCK-BOUNDARY round 10 — two defensible rules that were a hole together
One finding, and it is the one I asked for: *is there another place where the two traversals still
disagree about where a function ends?* Yes — `_bindings` was the last `ast.walk(fn)` in the analyser.
Round 9 fixed `_stored_attrs` to use `_own_nodes` (writes belong to their innermost function) and
added `_bind` (a name meaning two models resolves to neither). Each is right on its own. **Together
they opened a fail-open neither could cause alone**, because `_bindings` still descended into nested
bodies: a name rebound inside a *helper* polluted the enclosing function's map, `_bind` marked it
CONFLICT, and the enclosing function's **locked** write lost its receiver. That write is what SEEDS
the `(model, attribute)` pair. Lose the seed and an unlocked writer of the same pair elsewhere is no
longer a violation — because nothing is protecting it.
**This is a fail-open that works by REMOVING A SEED rather than by excusing a write**, which is why
it survived every self-test aimed at the judgement. All of those ask "is this unlocked write
reported?"; none asked "did the thing that makes it reportable survive?" The new probe is built the
other way round: a correct locked writer whose nested helper rebinds the same name, plus a genuine
unlocked writer of that pair. Reverting `_bindings` to `ast.walk` drops `(Connection, config)` out of
the protected set entirely and the unlocked writer silently leaves the violations — exactly the shape
described, reproduced.
Two lessons, and the second is the one worth carrying:
- **Every traversal in this analyser must agree on where a function ends.** Third instance in two
rounds: round 9's first attempt attributed a nested write to its enclosing function while the scope
maps stopped at the boundary, and this is the same disagreement in the remaining direction.
- **A rule can be correct in isolation and unsafe in composition.** `_own_nodes` and `_bind` were each
argued for on their own merits and reviewed on their own merits. The hole existed only in the
interaction, which is precisely what a diff-shaped review cannot see and why every round of this PR
has been requested as a full pass over the whole state.
Sites unchanged at **432**, six protected pairs, no new violations — the fix corrects *which scope*
a binding belongs to, not how many there are.
### LOCK-BOUNDARY round 9 — three fail-opens in the analyser, and a completeness claim with a filter under it
Three findings from the round-9 review, all inside the diff, all in `test_lock_boundary.py`, all
real. Each mutation-checked.
**1. `_stored_attrs` claimed completeness and then filtered.** Round 8's docstring argued that an
attribute write *is* `ast.Attribute` carrying `ctx=Store` — the grammar's own marking, complete by
construction — and the very next line required `isinstance(n.value, ast.Name)`. So
`ctx.project.source_ifc = v`, whose outer node carries `Store` exactly like any other write, never
entered the site list and the gate **accepted** it. *A completeness claim with a filter under it is
not a completeness claim* — and this is the third round running where the fix for an over-narrow
population was itself over-narrow, now at the level of a single `and` clause. Receivers are no longer
filtered here: resolving one is `collect`'s job, and an unresolvable receiver is reported UNKNOWN,
which reds. **The narrow version traded a fail-CLOSED report for silence**, which is the worse of the
two by the same argument this whole PR rests on.
**2. Imports were collected file-wide, not lexically.** `_alias_map` and `pid_lock_names` walked the
whole module, so a function-local `from .. import pid_lock` — how most of this tree spells it —
became a binding for *every* function in the file. One function's `import pid_lock as lock` would
certify an unrelated `lock.mutating(...)` elsewhere, and a reused model alias could resolve another
function's receiver to the wrong mapped model. Bindings now resolve along the real scope chain:
module level, then each enclosing function, then the function itself, with normal shadowing.
**3. One binding per name, applied to every write in the function.** `_bindings` kept the last
binding and `collect` applied it everywhere, so in `p = Project(...)` → `p.source_ifc = v` →
`p = SavedView(...)` the unlocked `Project.source_ifc` write was reclassified by a later line and
left the violations entirely. Resolving per source position needs real flow analysis; refusing to
resolve a name that means two different things does not, and errs the only safe way — UNKNOWN, which
reds. Every real site in this tree has one consistent meaning, so nothing legitimate is affected.
**The first version of fix 2 produced two false positives, and reading the code is what caught it.**
It reported `authoring.py::import_families` and `::content_import` as unlocked writers of
`Project.source_ifc`. Both are in fact correct — the write sits inside a nested helper that holds
`pid_lock.mutating(pid)` and imports it locally, the `run_in_threadpool` shape every `async def`
route here uses. The bug was mine: `_stored_attrs` walked *into* nested functions while the new scope
maps stopped *at* them, so a write was attributed to the enclosing function while its lock and its
import were not. **Two traversals over one tree must agree on where a function ends.** Writes are now
attributed to the innermost enclosing function — which `collect` visits in its own right, so nothing
is lost — while bindings accumulate down the chain, because a nested helper closes over the receiver
its caller fetched. *Had this been reported instead of read, it would have been two invented defects
in working concurrency code.*
Sites scanned **375 → 432**. Fifty-seven more real attribute writes, no new violations, and the six
protected pairs are unchanged before and after — which is the evidence that the scoping tightened
nothing legitimate. Three self-tests are planted and each fix is mutation-checked: restoring the
`Name` filter loses the chained receiver, going back to file-wide imports makes the impostor alias
*vanish from the violations*, and keeping the last binding loses the rebound site.
One implementation note worth keeping: the first scope-chain build was quadratic (a parent map per
call, three calls per function) and the gate stopped finishing. It is memoised per module now.
### LOCK-BOUNDARY round 8 (review) — a certifier that matches on a method NAME, and an inference wearing the shape of a proof
Four findings from the round-8 review, all inside the diff, all real, all fixed.
**1. `lock_spans` certified on the method name alone.** It matched any `<anything>.mutating(...)`, so
`with session.mutating(pid):` — an unrelated object exposing a method of that name — marked every
write in its body as locked. *A certifier that matches on a method NAME is trusting a string anybody
may define*, and it fails in the one direction a certifier must not: silently, toward "yes". The
receiver is now resolved against the file's own `pid_lock` imports (`pid_lock_names`, per file,
because most call sites import it function-locally), so a module with no such import certifies
nothing. An impostor is planted in the self-test population; loosening back to name-matching reds it.
Six protected pairs before and after — the tightening lost no real lock.
**2. Form 7 was an inference wearing the shape of a proof.** Round 6 reasoned: "a variable bound from
an attribute chain touching no mapped attribute name cannot be a mapped instance, because reaching one
by attribute access means traversing a mapped relationship." That holds for chains *through the ORM*
and says nothing about a chain through anything else — `p = ctx.row` on a plain container holding a
`Project` touches no mapped attribute name and is a mapped instance. **It was a tree-wide fail-open,
the same defect as the module predicate it replaced, one layer smaller and better argued.**
The repair is the `test_court_primary` move: *a premise that cannot be checked has already drifted;
stop inferring it and DECLARE it.* What made that affordable is that the population turned out to be
**one site** — measured by deleting the rule and re-deriving, not assumed. `NON_MODEL_RECEIVERS` now
carries that one entry with its reason, everything else falls through to UNKNOWN (fail-closed), and
the ledger is checked in **both** directions: emptying it must bring every declared site back as an
unresolvable unlocked write (so no entry is decoration, and none can rot into an exemption for its
next occupant), and nothing undeclared may need one (the direction round 6 failed in).
`MAPPED_ATTRS` and `_chain_attrs` went with the inference. Its self-test went too — a check on a
variable nothing reads is precisely the thing this PR is about — and the lesson it carried moved to
`MODELS`, which *is* load-bearing: an unconfigured registry resolves no receiver and leaves an empty
seed. **That repointed check immediately failed on a threshold copied from the old one** (34 models,
not the 183 attributes), which is the argument for pointing a check at something live.
**3. `test_ifc_publish_atomic` asserted SOURCE TEXT.** One check read `authoring_shared.py` and looked
for the literal `"os.link(final, backup)"`. Same defect `test_pin_pgnull` exists to record: *asking the
right question and then asserting something adjacent to the answer.* It passes on a spelling and on
dead code alike, and a faithful rewrite (`os.link(src=…, dst=…)`, or the call moved into a helper) reds
it for nothing. It now wraps `authoring_shared.os.link` and runs a real publication through it;
reverting the helper to `os.replace` reds it with the call list empty.
**4. A stale `LOCK-BOUNDARY` comment in `generate.py`.** Two paragraphs had stacked: the first still
claimed generation runs inside the lock and writes the fixed published path, which round 1 stopped
being true. The second, correct one stays. *The same failure as round 6's "`mutating` is reentrant, so
this nests safely" — a comment asserting the property the code no longer has, which reads as reasoning
rather than as the claim it is.*
### LOCK-BOUNDARY round 8 — the fix for an incomplete enumeration is not a longer enumeration
Round 7 replaced `test_lock_boundary`'s `ast.Assign`-only matcher with a three-statement one
(`+AnnAssign`, `+AugAssign`) and called the hole closed. It was the same mistake one size larger.
**Five further spellings of an attribute write were still invisible, and the gate ACCEPTED each:**
```python
p.source_ifc, _rest = a, b # a Tuple target -- the top-level target is not an Attribute
p.source_ifc, *_tail = xs
(_a, (p.source_ifc, _c)) = v # nested arbitrarily deep
for p.source_ifc in rows: ... # `For.target` -- reached by no assignment statement at all
with ctx as p.source_ifc: ... # `withitem.optional_vars` -- likewise
```
All legal Python, all writes, none of them a target of the three statements the matcher knew.
The repair is not a sixth case. **An attribute write is not a list of statement types — it is
`ast.Attribute` carrying `ctx=Store`, which is how Python itself marks one, in every form it has.**
`_stored_attrs` now yields exactly those, and admits nothing else: a read (`v = p.x`), a call
argument (`f(p.x)`) and `del p.x` carry `Load`, `Load` and `Del`. There is no seventh form to miss,
because the set is no longer a list somebody maintains. Sites scanned went **346 → 375** — twenty-nine
more real attribute writes in the tree, none of them a new violation.
*This is the third round in a row where the previous round's fix was the next round's defect, and
the first where the shape repeated at the same layer rather than a deeper one.* Round 6 removed a
module-level predicate for excluding what it had not proved; round 7 widened a statement matcher
from one spelling to three; round 8 stopped spelling. The lesson the first two did not quite reach:
**widening an enumeration leaves an enumeration, and its remaining blind spot is invisible in exactly
the same way** — the count looked more complete each time, and 331, 346 and 375 are all confident
numbers. Only the last one is derived from something that cannot silently shrink.
Seven non-plain spellings are now planted unlocked in the self-test population, and narrowing
`_stored_attrs` back to the three statement forms reds two checks, naming the five it stopped seeing.
Worth stating because it is the *reason* only this one site needed fixing: the analyser's other two
`ast.Assign`-only matchers — Form 7's `NOT_A_MODEL` exclusion and the model-resolving pass — fail
**closed**. A form they miss leaves the receiver unresolved, which the gate reports as UNKNOWN and
reds. `collect` was the only one where a missed form meant a site was never looked at.
### LOCK-BOUNDARY round 7 — a `finally` that deletes the only copy, and two spellings of a write
Three fixed, two declined. The first is the most serious defect this PR has produced, and round 6
made it worse.
**1. The rollback backup was deleted even when the restore FAILED.** The `finally` unlinked it
unconditionally. If `os.replace(backup, final)` raises, the published path is left holding bytes the
system refused **and the backup — the user's previous model — is destroyed.** Round 6's `os.link`
sharpens it: backup and `final` begin as one inode, so after the promotion the backup holds the
**last** reference to the old bytes. *Cleanup in a `finally` runs on the branch where cleanup is
exactly the wrong thing to do* — the one case that still needs the file is the case that got there by
failing. The backup is now removed only when publication succeeded **or** restoration succeeded; a
surviving `.rollback-*` is evidence that a publication failed and could not be undone, not litter.
**2. `test_lock_boundary` matched `ast.Assign` only.** `p.source_ifc: str = v` is an `AnnAssign` and
`p.source_ifc += v` an `AugAssign` — both writes, and the augmented one is *literally* a
read-modify-write, the shape the gate exists for. Neither ever entered the site list, so the gate
**accepted** them. Same defect as round 6's module predicate one layer down: *a population derived by
matching one spelling of a thing silently excludes the others, and the exclusion never appears in the
output.* Sites scanned went **331 → 346** — fifteen attribute writes the gate had never seen. Both
forms are now planted unlocked in the self-test population, and narrowing the matcher back to
`ast.Assign` reds two checks.
**3. The hard-link continuity claim was unconditional.** It is a property of the hard-link path; on a
filesystem without hard links the helper falls back to the rename and the window returns. Qualified.
**Declined, both real, both about the proforma module rather than this diff:** a full-budget `PUT`
can still overwrite a concurrent sync's hard lines (needs a revision token, i.e. a client contract
change); and `source_ifc_path` / `takeoff_file` / `open_model` run before the lock in the model-sync
route, so a publication can swap the IFC between the read and the commit. Both predate this PR, both
need their own change, and both are tracked.
### LOCK-BOUNDARY round 6 — the gate's own look-at predicate, and a fix that cost an unrelated property
Three real findings from a full re-review of the head. Two are defects **inside earlier rounds of this
PR**, which is now the pattern rather than the exception.
**1. The published path was briefly ABSENT, and that is not the same property as "never partial".**
Round 4 took its rollback backup with `os.replace(final, backup)`, so `source.ifc` did not exist
between that rename and the promotion. `bake_layers` calls `project_with_source` **before** it takes
the lock, and that helper raises 409 *"project has no accessible source IFC"* on a missing file — so a
concurrent read could be refused during a publication that then **succeeded**. The backup is now an
`os.link` — the same inode under two names, so the path stays present and `os.replace` still swaps it
atomically. **That continuity is a property of the hard-link path, not an unconditional guarantee:**
on a filesystem without hard links (FAT, some network mounts) the helper falls back to the rename and
the window returns, because copying what may be hundreds of MB on every publish is the worse trade.
The fallback costs a spurious 409 on a concurrent read, never data. *A fix for one property can cost an unrelated one that nothing was asserting* —
the bare rename had given continuity for free since the beginning, and round 4 spent it without
noticing.
**And the first test written for this was vacuous.** It asserted no `<missing>` observation during a
normal publish; reverting the fix left it **passing**, because two back-to-back renames take
microseconds and the reader polls every millisecond. *A check that cannot observe the interval it
describes reports on nothing.* It now tests the primitive with a deliberately wide window — with a
rename the reader must catch the gap, with a link there is no gap at any width — plus an assertion
that the helper is what uses the link.
**2. `test_lock_boundary`'s module filter was the defect its own docstring warned about.**
`touches_orm` skipped any module importing neither `models` nor `Session`, justified as *"provably
incapable of the defect"*. That is **false**: `def stamp(p): p.source_ifc = v` holds a mapped instance
its caller passed in, with neither import. Such a file was skipped whole, so the write never reached
`verdicts` and could not be reported UNKNOWN — **the fail-closed rule was defeated one layer above the
code enforcing it.** The docstring quotes CLAUDE.md's lesson that *a predicate deciding what to LOOK
at is more dangerous than one deciding what to report* two lines before violating it.
The module skip is gone. Two per-receiver rules replace it, both derived and provable in both
directions: `self` inside a class resolves to that class when it is mapped and to a proven non-model
when it is not; and a receiver bound from an attribute chain touching **no** mapped attribute name
cannot be a mapped instance, because reaching one by attribute access means traversing a mapped
relationship. Deliberately not *"an attribute chain is not a model"* — `project.owner` would be — so
the test is against the registry, not the shape.
`MAPPED_ATTRS` needs `configure_mappers()`: the first probe returned **0 of 183**, because mappers
configure lazily. An empty set would mark every chain "provably not a model" — a fail-open across the
tree — so a self-test asserts it is populated before any verdict prints. The removal itself has a
regression test with a mutation: reinstating the predicate makes the planted site disappear.
**3. Wording corrected in three places.** "`os.replace` undoes itself for free" overstates it: a rename
is cheap to undo — one more rename, nothing copied — but nothing undoes it automatically.
**Declined, with reasons:** the object-storage arm of a failed commit is the residual already recorded
in the helper's docstring and filed as **#558**, blocked on a retention decision that is the
maintainer's; and `pid_lock._advisory` degrading to in-process on an acquisition error is a
pre-existing, documented property of that module affecting all eleven call sites, not something this
PR introduced — it is tracked separately rather than changed inside a PR already six rounds deep.
### LOCK-BOUNDARY round 5 — the rollback had a branch that existed doing nothing
Round 4 made publication failure-atomic by moving the previous `source.ifc` aside and putting it back
on any exception. Review found the half that does not exist: **when there is no previous file, there
is no backup, and the `except` branch did nothing at all** — while `os.replace(staged, final)` had
already put the refused bytes at the published path. That is every FIRST publication: blank create,
first upload, first generate, first `ensure_model`.
*The inverse of a rename is a rename OR a delete, depending on what was displaced, and a rollback
written only for the first case is a no-op in the second.*
The orphan is the smaller half. The larger one is what it does to the NEXT publication: that one finds
`final.exists()` true, adopts the refused bytes as its backup, and on its own failure **restores them**
— promoting a model the system refused into "the previous model worth keeping".
**Why the round-4 test could not see it.** `test_ifc_publish_atomic`'s rollback arm calls `reset()`,
which writes the old model to the published path every time, so `final.exists()` was always true and
the no-backup branch was never entered. *A rollback test that always has something to roll back never
exercises the branch where there is nothing.* The new arm deletes the published file first, asserts the
failure leaves nothing behind, and then asserts the follow-on — a later successful publish has no
refused model to inherit. Deleting the fix reds it with `source.ifc survived holding 4144 refused
bytes`, so the branch is exercised rather than merely present.
### LOCK-BOUNDARY round 4 — a comment that asserted the safety property the code lacked
Two Major review findings, both real.
**The publication and the budget seed were two lock intervals, not one.** `publish_source_ifc` took
the lock and RELEASED it; a second `with` then re-took it — and the comment beside it said
"`mutating` is reentrant, so this nests safely", describing a nesting that did not exist because the
publisher had already returned. **A comment can assert the exact safety property the code lacks, and
it reads as the reasoning rather than as the claim it is.** The gap lets generations A and B publish
in order A, B — so the model is B's — and then A, still to seed, finds `dev_budget` empty and seeds
it from A's metrics: model B priced by budget A, reported by nothing. Now one outer interval, with
the publisher re-entering it — which is what the comment had claimed all along.
**Publication was not failure-atomic.** A rename is CHEAP to undo — one more rename, no data
copied — but nothing undoes it automatically, and `put_stream` and
`commit` do not. A failure after the rename left the published local path — the file `bake_layers`
and the converter open — holding the new bytes while nothing else had been published. *Atomic at
each step is not atomic across the sequence.* The previous file is now kept aside and restored on
any failure.
**What that does NOT cover is stated in the helper rather than implied:** if `put_stream` succeeds
and the commit then fails, object storage holds newer bytes than the local file and the row. Storage
is the durable copy rather than what readers open, and the next successful publish overwrites it —
but it is a real window, and closing it means publishing immutable versioned artifacts and moving
the pointer last, which is a design change rather than a guard.
`test_ifc_publish_atomic` gains three checks: a midway failure raises rather than reporting success,
the previous model is restored byte-for-byte, and no rollback scratch is left. Removing the restore
reds the second one (`final is 4144 bytes, expected the old 57`) — the guard is exercised, not
merely present.
### LOCK-BOUNDARY round 3 — the gate's own seed was failing open, and it hid three live defects
Round 1's gate reported nothing about `Project.dev_budget`, and the reason was in the gate, not the
code. `generate._finalize_generated` wrote it entirely inside the lock — but took `p` as an
**un-annotated parameter**, so the receiver was UNKNOWN, the pair never entered the protected seed,
and the three unlocked writers in `routers/proforma.py` were never compared against anything.
**UNKNOWN was fail-CLOSED where it could excuse an unlocked write and fail-OPEN where it could
establish that a field is protected at all.** *A predicate that decides what to LOOK at is more
dangerous than one that decides what to report* — and a seed is exactly such a predicate, so the gate
was not exempt from the rule it enforces. Found by running the analyser and reading its output, not
by any test: nothing was red.
Closed in both halves, because either alone is useless. A fifth binding form (a parameter's type
annotation) put the pair in the seed, which immediately named `proforma.put_dev_budget`,
`sync_gmp_to_hard` and `sync_model_to_hard`; all three now hold `pid_lock.mutating(pid)`. **Note
which half did the finding: a one-line signature annotation turned a silent gate into one that named
three live defects.**
The defect itself is the `connections` shape again on money. `put_dev_budget` replaces the whole
budget blob from the editor's form; the two `sync_*_to_hard` routes rebuild only the `hard` lines and
preserve soft/acquisition/contingency. An editor saving the budget while someone clicks "sync GMP"
lost one of the two silently — each caller's 200 was true of its own write. A lock and not a
compare-and-swap, for the same reason `connections` chose one: the edits are claims on different line
categories of one blob, so serialising lets both survive where a 409 would refuse an edit that
conflicts with nothing.
`test_rmw_sweep`'s ledger is updated from OPEN to LOCKED for the two sync routes, with the reason
recorded: that sweep could only ever see two of the four writers, because the third is a plain
replace and the fourth a conditional seed. **A pair is only as closed as its widest member, and that
derivation could not see the widest.** Sweep open count 4 → 2; the two remaining are
`drawingset.revise_sheet` and `share_scenario`.
### LOCK-BOUNDARY round 2 — the lock covered the pointer and not the bytes
Review on #557 caught that the first pass locked the `source_ifc` POINTER and left the file
PRODUCTION outside. Every producer wrote the published `source.ifc` directly: `import_rvt` with a
bare `write_bytes`, both massing generators and the blank create onto the fixed path, and the upload
through `stream_to_path` — which does write `<dest>.part` and rename, but **the part name is derived
from the destination**, so two concurrent uploads to one project share `source.ifc.part` and
interleave into it before either renames. *A staging name that is a function of the destination is
not staging, it is a second shared path.*
`bake_layers` and `/edit` take the project lock and then open `p.source_ifc`, so a reader could open
a file being rewritten underneath it. Locking the assignment alone moved the race rather than
closing it — the same correction PR #552 forced on `under_pid_lock`, one layer down: *a lock proves
something about the interval it spans, and the interval has to contain the bytes as well as the
pointer.*
Now every producer writes to a unique staged path and `authoring_shared.publish_source_ifc` promotes
it — `os.replace`, storage publish, pointer, commit — inside one critical section. One helper rather
than the same critical section at five producers, for the reason `connections._lock_key` is one.
`staged_ifc` is a context manager so a producer that fails before promoting cannot leak its staged
file; this tree has already run a disk out on orphaned scratch once.
`services/api/test_ifc_publish_atomic.py` is the behavioural half, and it exists because
`test_lock_boundary` structurally cannot see this: an AST analyser can confirm every
`p.source_ifc = ...` sits inside the lock and says nothing about which path the bytes went to — the
whole defect passes it. The test polls the published path while a producer runs and asserts the
reader only ever sees a complete old or complete new model; the mutation writes straight to the
published path and the reader catches partial files, so it measures the staging and not the timing.
### LOCK-BOUNDARY — G-11 was closed yesterday and five writers of the same column were still unlocked
`test_rmw_sweep` derives **read-modify-writes**. `p.source_ifc = str(ifc_path)` reads nothing — it
installs a new pointer — so it was never in that population, and neither was the conditional-create
`if not p.source_ifc: ...`. G-11 was closed on 2026-09-13 by locking every `source_ifc`
read-modify-write in `routers/authoring.py`; **five writers of that same column were left open, two
of them inside that file.** A pointer swap that reads nothing still orphans the version a concurrent
`bake_layers` is deriving from.
Now locked: `authoring.upload_source_ifc`, `authoring.import_rvt` (both `async def`, so the lock is
taken off the event loop via `run_in_threadpool` — the v0.3.703 SSE failure otherwise),
`generate.create_blank_model`, `generate.ensure_model`, `generate._finalize_generated`.
`generate.py` had no `pid_lock` import at all.
**`ensure_model` was the sharpest.** It is a CHECK-THEN-ACT, not a read-modify-write: it reads
`source_ifc`, concludes from its emptiness that no model exists, and installs a blank one. Unlocked,
two concurrent calls both read empty, both generate over the same fixed `source.ifc` path, and both
answer `created: true`. The lock spans the DECISION — the early returns are inside it — because a
lock taken after the branch protects nothing this route does.
**The first diagnosis was wrong, and the correction is the point.** It looked like a FILE boundary:
`generate.py` contained no lock at all, so the closure seemed to have stopped at the edge of the file
being worked in. It had not — two of the five are in the "closed" file. What bounds the coverage is
the SHAPE. *A coincidence that corroborates a wrong theory is the expensive kind, because it ends the
search.*
`services/api/test_lock_boundary.py` derives the rule from the code rather than a ledger: a
`(model, attribute)` pair locked in **any** writer must be locked in **every** writer. It resolves
each receiver to a mapped model — four binding forms, every one added because a probe reported
UNKNOWN rather than because it was anticipated — and fails closed, because a resolver that guesses
turns each of those into a confident wrong answer. Receiver resolution is load-bearing, not tidy:
`modules.save_view` writes `.config` and matches `Connection.config` by NAME, but it is a `SavedView`
and not a violation at all.
**It states its own limit rather than hiding it:** this finds INCONSISTENT protection, not ABSENT
protection. `Project.dev_budget` has four writers and no lock anywhere, so it never enters the seed
and this gate is silent on it — the sweep names two of those four instead. Each derivation is blind
where the other sees, which is why both exist.
### Two admins saving one connection: one of the two saves vanished
`Connection.config` had **two** writers and neither took a lock. `update_connection` merges the
body's keys into the blob — keeping a stored secret where the form sent it blank — and
`put_mappings` rewrites the whole blob to replace its `mappings` key. So an admin saving credentials
while another saved field mappings lost one of the two, silently: each caller's 200 is true of its
own merge. Gap G-12's `connections` pair is closed; the sweep's open count drops six to four.
**A lock, not a compare-and-swap — deliberately the opposite call from the register rows.** There
the whole row is one logical edit, so a stale write should be REFUSED with a 409. Here the two
writers touch near-disjoint keys, credentials against `mappings`, so serialising lets the second
merge onto the first's committed blob and **both survive**, where a 409 would refuse an edit that
does not actually conflict. *The right control depends on whether the concurrent edits are rival
claims on one value or independent claims on different ones.* `connections` carries no token to swap
on in any case — only `created_at`.
**Two checks, and the measurement showing neither is sufficient alone.**
`services/api/test_connection_lock.py` asserts both edits survive and that neutering the lock loses
one. Then two mutations were run:
| mutation | sweep | behavioural test |
|---|---|---|
| one route's lock deleted | ❌ red | ❌ red |
| the two routes lock on **different keys** | ✅ **green** | ❌ red |
*Static analysis can see that a lock is present; only behaviour can see that it is the SAME lock.*
`under_pid_lock` asks whether each mention sits inside *a* `pid_lock.mutating(...)`, and under the
second mutation both still do — "a lock one side does not take protects nothing" wearing the shape
of a lock. Hence `_lock_key` is a shared helper rather than an f-string at each call site.
**And the first mutation found a hole in the gate itself.** `test_rmw_sweep`'s "every site this
ledger calls locked IS lexically under `pid_lock.mutating`" derived from `IFC_PIPELINE` only — the
one ledger that had LOCKED rows when the check was written. The two new LOCKED rows landed in
`CROSS_FIELD`, where **nothing asserted them**: the status was prose again, and deleting either lock
left the sweep green. The derivation now spans every ledger, taking verified locked sites from 6 to
18. *A gate scoped to the ledger that existed when it was written silently stops covering the
ledgers that arrive later* — and only mutating the shipped fix surfaced it.
### Two people authoring at once: one of them lost the whole model version
Six routes in `services/api/src/aec_api/routers/authoring.py` — `bake_layers`, `import_families`,
`import_family_pack`, `place_family`, `content_import` and the shared `_restore_version` behind
undo/redo — read `Project.source_ifc`, produced a NEW IFC version from it, and wrote the pointer
back, all outside the per-project lock. **Five sibling routes in the same file do the identical
thing inside `pid_lock.mutating(pid)`**, and `edit` carries a comment describing exactly this race.
So two concurrent authoring operations each derived from version N, each wrote their own N+1, and
the second commit orphaned the first user's work — **silently, because each caller's response is
true of its own run**. What is lost is a baked override set, an imported family pack, a placed
family, a content import, or a restored version: the model the product exists to author.
All six now hold the lock across the whole read → derive → apply → pointer-swap. Gap G-11 is closed
and the sweep's open count drops from twelve to six.
**The lock is proved by behaviour, not by its presence.** `services/api/test_authoring_lock.py`
runs two concurrent bakes against a stubbed recipe and asserts the second reads the FIRST's output;
neutering `pid_lock.mutating` makes both read the same base. A test that grepped for the lock would
pass on a lock spanning only the assignment — the defect review caught one level up on PR #552.
**Three things the uniform fix got wrong**, each corrected before it shipped:
- **Two of the six are `async def`.** `pid_lock.mutating` is synchronous and blocks on a Postgres
advisory lock, so taking it inline would park the event loop behind somebody else's upload —
*a rare lost model version traded for a routine server-wide stall*, which is the v0.3.703 SSE
failure arriving by a different door. Both now run the locked section through `run_in_threadpool`.
- **`import_family_pack` first had `db.commit()` outside the lock.** An uncommitted pointer swap is
invisible to the next lock-holder, which reads the old version and derives from it — the same race
moved rather than closed. *A lock proves something about the interval it spans, and that interval
has to end after the write is durable.*
- **`_restore_version` re-read the column once more after the lock released.** Harmless today, and
the sweep refused to certify it anyway — *a certifier that reasons about which stragglers are
harmless is one bad reading away from blessing one.*
The undo/redo lock sits in the shared helper rather than in `edit_undo` and `edit_redo`: the
narrowest place that can carry the fact, so both routes inherit it instead of two call sites having
to remember.
### The re-key voided one of the gate's own regression checks, silently
Found in review, and it is the same defect the pull request exists to fix, committed by the fix.
The gate carried a regression check that the share-token view counter is no longer an ORM
read-modify-write, written as `("…client_portal.py", "model_fragment") not in ORM`. That was true
while `ORM` held `(path, function)` pairs — and it stayed true, permanently, the instant this
change re-keyed `ORM` to `(path, function, attribute)`: **a 2-tuple is not a member of a set of
3-tuples no matter what is in it.** Reinstating the original `row.view_count += 1` would have left
those three lines printing PASS.
*A membership test written against a key SHAPE fails OPEN the moment the shape changes*, and
nothing about the diff looks like it touched that check. The projection to `(path, function)` is
now derived by a named function rather than written out at the call site, and the gate plants a
synthetic site and requires the projection to find one that raw membership cannot — so the shape
bug has to come back through an assertion that is watching for it.
The canary's first draft was itself conditioned on the real population, and reinstating the genuine
regression made it report *"the projection did not distinguish the planted site"* — blaming the
analyser for a defect in its subject. It now runs on a two-element synthetic population and touches
the tree not at all. *A check whose failure message can misdiagnose is worse than one that stays
silent, because somebody acts on it.*
### The concurrency ledger was keyed by function, and reasoned about one field at a time
`services/api/test_rmw_sweep.py` keys its read-modify-write ledgers by `(path, function)`. **Six
functions write more than one field**, so a single status and a single sentence covered several
distinct races — and the sentence names whichever field the author happened to be looking at. The
rest are exempted by reasoning about a different line.
That is this file's own subject one level up: *an exemption that reasons about part of its subject
and reports on all of it.* And it had already hidden something. `connections.update_connection` was
EXEMPT because "`body.name or c.name` is a PATCH keeping an unsent field" — true of `name`, and
silent about `merged = dict(c.config or {})` … `c.config = merged`, an accumulating merge that keeps
a stored secret where the form sent it blank. `connections` carries no version column at all, only
`created_at`.
Worse, it is the **second** writer of the blob `connections.put_mappings` writes, which was already
listed OPEN. One admin saving connection settings while another saves field mappings loses a write,
and whatever control that table eventually gets must cover both — *a lock one side does not take
protects nothing*, the lesson the `dev_property` pair paid for a day earlier.
The key is now `(path, function, attribute)`. The derived population went **32 → 41** and the open
count **11 → 12**; the count going up is the sweep working rather than failing. Two side effects
worth naming: `under_pid_lock(*k)` now takes the column straight from the key instead of being told
it a second time, and the `dev_property` pair is derived by `k[2] == "dev_property"` rather than by
searching for that string **inside the reason prose** — *a derivation over prose, in the file whose
own lesson is that a count derived by grepping prose is prose.*
The other five multi-field functions were re-read and their exemptions hold for every field, now
said per field rather than inherited: `bim._with_kind` sets non-column serialisation fields,
`verification.set_status` and the three `cloud` sites take whole values or fallbacks from a provider
response. `cloud_refresh_profile`'s `cloud_roles` and `cloud_tier` get their own sentences despite
reaching the same verdict, because they are authorisation-bearing and inheriting a reason about a
display name is how that stops being examined.
A check now asserts that at least one function's fields carry **different** statuses — the thing the
old key could not express, so the extra column is load-bearing rather than decorative. Its failure
message names both causes, because today `update_connection` is the only such function and closing
it legitimately would red the check for the opposite reason: *a check whose failure message can
misdiagnose is worse than one that stays silent, because somebody acts on it.*
### Two people pressing "promote" on one markup got two RFIs, and one of them vanished
`bim.promote_markup` checked `if m.topic_id: 409` and then claimed the back-link with a plain
`m.topic_id = t.id`. The check reads the session's own copy, and `SessionLocal` is
`expire_on_commit=False`, so a request that loaded the markup before a concurrent promote committed
still sees `None` however long it holds it. Both requests therefore passed the guard, both minted an
RFI Topic, and the later write won — leaving **two Topics for one markup, the first orphaned**
because nothing points at it any more, with both callers told 201. A double-click is enough; no load
is required.
It now claims the link with a conditional `UPDATE ... WHERE topic_id IS NULL` and rolls back the
flushed Topic on a loss, so the loser leaves nothing behind. That is not a new idea here — it is
exactly what `modules.promote_comment` was given when the same defect was fixed there, one module
over. **This instance was found by the read-modify-write sweep's derivation, not by anyone reading
the route**, which is the argument for deriving a population rather than listing one: *an
already-solved defect living on elsewhere is the cheapest kind to close and the easiest to never
look for.*
Two smaller things the fix exposed. The Core `UPDATE` bypasses the ORM, so the session's `m` kept
its null `topic_id` and the response would have reported the markup unpromoted **in the very call
that promoted it** — a `db.refresh` before the response fixes that. And closing the site made its
own ledger entry stale, which `services/api/test_rmw_sweep.py` reported before the ledger was
touched: *closing a gap has to force the record to move, or the tree ends up describing a defect
nobody can still find.* Open gaps 12 → 11; G-12 five → four.
### Two people editing one record both saved; only one of the edits survived
RMW-SWEEP. `transition` was one instance of a class, and PR #551 fixed only that instance. The shape
is: **read a column, compute a new value FROM what you read, write it back under `where(id == rid)`
alone.** Nothing holds the row still in between, so two callers both read the old value and the
second write erases the first — and *both* callers are told they succeeded. Reproduced against the
real functions before anything was written:
```
set_element_guids: A reads ['GUID-A'] · B commits ['GUID-A','GUID-B'] · A commits ['GUID-A','GUID-C']
FINAL ['GUID-A','GUID-C'] GUID-B survived? False
link_record: B commits a link · A commits a link · FINAL holds one of the two
```
*A lost update that refuses one caller is nearly fixed; one that tells both they succeeded has to be
found twice* — the same severity inversion the seeding sweep recorded, where a primary key refuses
the loser and a non-unique index refuses nothing.
**Five sat on the register row**, and `update_record` is the one that matters. It is the path the
register's inline text and select cells use; it writes `{**what we read, **what you sent}`; and the
control `docs/security/threat-model.md` names for exactly this threat — `expected_modified_at` — is
**opt-in**, passed by one of five web call sites. So two people editing *different fields* of one
record both merged onto the same pre-image and the second erased the first person's field. The
documented control was real and it was not on the path people use.
All five now route through `modules._cas_row_edit`, which re-reads, recomputes, and writes under
`WHERE id = ? AND modified_at = ?` — a compare-and-swap in SQL rather than a check in Python.
**Three decisions in it are worth naming, because each has a wrong-looking alternative.**
- **The swap is on `modified_at`, not on the collection.** `element_guids` is a JSON column and JSON
equality is not comparable the same way on SQLite and Postgres — the exact trap `with_for_update()`
is, which Postgres honours and SQLite silently ignores. `modified_at` is a plain timestamp on both.
And **every register-row writer sets it**, which is now asserted rather than assumed: the dynamic
register table declares the column with **no `onupdate=`**, so that coverage is a property of the
call sites, and one writer forgetting it would make this predicate match a row somebody else had
just changed — *a lock that looks present.*
- **It retries rather than refusing.** A lost `transition` race means the action genuinely is no
longer available, so a 409 is honest. "Add GUID-C" is still the caller's intent after somebody else
adds GUID-B, so re-reading and re-applying gives the user what they asked for.
- **…except where the caller does not own the transaction.** A retry rolls back, and
`update_record(commit=False)` has callers (`responsibility`, renaming a role column across every
row) that have already staged rows in that transaction. Rolling those back and then succeeding here
would commit a partial edit and report it as whole — *the very shape of bug this exists to remove.*
So `commit=False` gets one attempt and a 409, which aborts the caller's whole edit.
**`revise` was hiding a second defect behind the first.** Its "record already revised" check was
itself a read-then-write: two concurrent revises both passed it, both minted a revision row, and the
second supersede write erased the first's `superseded_by` — leaving two live revisions of one record,
each believing it is the only one, and only one findable from the source. The CAS refuses the second.
**Two off-register instances were found, and one of them was not a race at all.**
`client_portal`'s share-token view counter was `row.view_count = (row.view_count or 0) + 1` — two
people opening the same link both read 7 and both wrote 8, so the count drifts *down* under exactly
the load that makes it interesting. It moved into SQL (`view_count + 1` evaluated by the database),
with no CAS and no retry, because there is then nothing read in this process for anyone to
invalidate. The other was the opposite of what the sweep was looking for: `proforma.save_property`
wrote `p.dev_property = body` **wholesale**, and `realestate.save_appraisal` keeps `appraisal` inside
that same blob — so every ordinary re-save of the property tab deleted the saved appraisal overrides.
No concurrency required. *A write that reads nothing cannot lose an update — it just deletes one.*
*That sentence is about the code as it WAS, and review pointed out it stops being true of the fix:
carrying `appraisal` across means reading `dev_property` first, so the repaired route can now lose a
concurrent appraisal save — a smaller fault than the one it removes, on a table with no concurrency
token, and it is filed as open rather than implied away.* The same round found the response still
answering with `body` while the merged dict was what got stored — persisting one value and returning
another, which is the kind of mismatch that gets discovered from two screens away.
**What the sweep could NOT reach is named rather than left silent**, because that is precisely how
the seeding sweep lost four sites. `Scenario.shared_with` (a share grant) and `Project.dev_property`
(the appraisal merge) still lose a concurrent write: neither table carries a `modified_at`, so the
register row's token does not exist for them. They are frozen in `test_rmw_sweep.py`'s `BAND_2`
ledger and filed as gap **G-10** — the set cannot grow silently, because a new instance reds the
build.
**The gate**, `services/api/test_rmw_sweep.py`, derives the population by AST with transitive taint
from `get_record`, fails **closed** on an `update()` whose target it cannot resolve, and runs itself
against the shipped pre-fix bodies of `set_element_guids` *and* `update_record` before it may print
any verdict — the second because `update_record` splats `**vals`, which is the blind spot that cost
PR #551 a review round. The behavioural half asserts the **outcome** through the real functions on a
hand-built interleaving, and a mutation removing the swap predicate must lose the GUID again.
**The review round found three more things, and two of them were holes in this gate rather than in
the code it guards.** The swap-predicate test asked only whether the `WHERE` named a non-identity
column — so `WHERE deleted_at IS NULL` read as a compare-and-swap though it compares against a
constant and swaps on nothing: *a fail-OPEN verdict inside a gate whose whole design is failing
closed.* Tightening it then flagged two correct writers, which taught the distinction the first two
drafts had collapsed: a constant predicate on the column being **written** (`promote_comment`'s
`topic_id IS NULL`) is a real guard, and one on a column the statement does not write is a filter.
And the token-coverage check searched the function's text for `"modified_at"` — which a **docstring**
satisfies, and every function here has a long one. It now reads the assignment with docstrings
stripped and demands `_next_stamp`; strengthening it immediately found `transition`, whose stamp sits
in a dict literal the detector could not see. *Three shapes write that column and the first draft of
the detector knew two.*
**The third was a real hole in the lock, not the token.** `_now()` can equal the row's existing
`modified_at` — same microsecond, or an NTP step backwards — and then the winning write leaves its
own predicate reusable by the writer it just excluded. Every register-row writer now stamps through
`modules._next_stamp`, which is strictly monotonic per row. *Rare is not impossible, and a
concurrency control that is correct except under contention is wrong exactly where it is
load-bearing.*
**And widening the gate's ORM derivation caught this pull request hiding a site from itself.** The
`save_property` fix hoisted its read into a local (`prior = p.dev_property or {}`), and the analyser
only looked for the object named directly on the right-hand side — so the commit that created an RMW
site removed it from the count in the same edit, and nothing said anything. *Hoisting a read into a
variable is the cheapest way to make a pattern-matching check stop matching, and it is what a tidy-up
commit looks like.* With taint propagated one hop the population went **12 → 33**, and the 21 new
sites are read and classified rather than counted.
**One of them is the biggest thing this round turned up, and no reviewer asked for it.** Twelve routes
read `Project.source_ifc`, derive a new IFC version from it, and write the pointer back. **Six wrap
that in `pid_lock.mutating(pid)` and six do not** — and one of the six that does carries the comment
*"same RMW race as /edit — serialize per project"*, so the control exists, is named in the threat
model, and covers half its population. Two concurrent `place_family` calls lose one placement with
both callers answered 200. It is **filed, not fixed** (gap G-11, roadmap RMW-LOCKGAP): wrapping six
routes that do long IFC I/O is a different blast radius from the register row, in a lane this change
did not otherwise touch. The set is frozen instead — the gate asserts the split in *both* directions,
so a new unlocked writer reds the build and closing one of the six forces the ledger to be updated
rather than leaving a recorded gap that no longer exists.
**A second review round found four more, and three were the gate reporting safety it had not
established.** Each is the same failure wearing a different mask, and each was introduced by the fix
for the one before it:
- **The conditional-write exemption protected the wrong column.** The first tightening made a
constant predicate count as a swap when the statement *writes* that column — correct for
`promote_comment`. But `.where(status == "open").values(status=…, element_guids=<from the read>)`
also satisfied it, and a concurrent `element_guids` change does not move `status`, so the stale
write still lands. The guard now has to cover **every** column carrying a read-derived value, and
a splatted `**vals` — which hides the column-to-expression mapping — makes coverage unprovable
rather than assumed. *Fixing an over-broad rule by narrowing one clause can leave the same hole one
column to the left.*
- **The stamp check asked "somewhere", not "every".** It returned on the first valid
`modified_at=_next_stamp(...)` anywhere in the function, so a writer with two updates passed while
the second omitted the stamp. Now validated per update — and doing so immediately failed
`_cas_row_edit`, whose stamp is a *subscript assignment* into a dict built earlier. That is the
third shape, and **every draft of this detector has known one fewer than exist**.
- **The lock check measured the write, not the interval.** `under_pid_lock` asked only whether the
`source_ifc` assignment sat inside `pid_lock.mutating`. A route can read the pointer, spend a
minute deriving a new IFC from it, and *then* enter the lock to assign — passing a check that
looks at the write alone while two callers derive from the same old version. Every mention of the
column, read or write, must now be inside. *A lock proves nothing about the statement it contains;
it proves something about the interval it spans.* All six certified routes survive the stricter
test, which is the answer that makes the certification worth having.
- **And the open-gap count was computed by grepping its own prose.** `_open` matched the substring
`"OPEN"`; one entry had been written `"Open"`; the gate reported **13** open sites where there are
**14**, and the threat model inherited the wrong number. Status is now a structured field with a
declared value set, asserted, so a typo reds the build instead of deleting a gap from the tally.
The six non-IFC sites that tally had been under-reporting are now their own named gap (**G-12**),
`bim.promote_markup` among them — which is `promote_comment`'s pre-fix shape, a live instance of a
defect already solved one module over.
**And the monotonic-stamp branch was itself untested until it was asked to be.** `_next_stamp`'s
`prev + 1µs` path runs only when the clock fails to advance, so no ordinary test reaches it — *a
branch that only runs under contention is a branch only production runs.* It is now exercised against
the real table with the clock frozen, which also settles a question the fix quietly raised: that path
returns a **tz-aware** datetime while SQLite hands `modified_at` back **naive**, and if the two
rendered differently the next `WHERE modified_at = <what we just read>` would match nothing and every
edit would 409.
**A third review round found two more, and both were the gate accepting a SHAPE where it meant to
check a FACT.** Neither is a live defect in this tree — the population and every count are unchanged
— which is precisely what makes them worth recording: each one would have gone quiet on the *next*
ordinary edit, with every number still printing.
- **Naming the guard counted as calling it.** The stamp check asked whether the name `_next_stamp`
occurred anywhere in the `modified_at` expression, so `_next_stamp(None)` satisfied it in all three
shapes. That argument is not a near-miss: `None` skips the `now <= prev` comparison inside the
helper and it returns a bare `_now()` — the exact unguarded clock the helper exists to replace.
Under a frozen clock the writer re-emits the token it just read, the loser's CAS predicate still
matches, and the swap protects nothing while the gate reports it protected. The argument is now
traced back to the row's prior `modified_at`, through a local if it was hoisted into one, and all
three shapes route through one function so a fourth inherits the rule. *Naming the guard is not
calling it, and calling it is not feeding it.*
- **Both taint analyses walked `ast.Assign` and not `ast.AnnAssign`.** An annotated
`rec: dict = get_record(...)` breaks the Core chain and the write behind it classifies as PLAIN;
an annotated `prior: dict = p.dev_property` drops the ORM site out of the population *entirely*,
taking its ledger entry with it — at which point the staleness check would have blamed the ledger
entry and pointed the next reader at the wrong file. **The blind spot is one `mypy --strict` pass
wide.** Adding annotations changes no behaviour, is the most ordinary edit in the language, and
would have silently emptied both derivations. *A gate whose reach depends on the syntax somebody
happened to use is measuring the author, not the code.*
The ORM analyser was also split out per-file so a **fixture** can be run through it. It could
previously only be fed the repository — and the shape review found missing does not occur here, so no
amount of reading the source would have produced a failing case. *An analyser that can only be fed
the tree cannot be given the input it gets wrong.*
**A fourth review round found three more, and the first two are the third round's own fixes failing
one level down.** The pattern is worth naming on its own: *a fix can recreate, at the next level of
detail, exactly the defect it just closed — and it reads as the same code both times.*
- **Data dependence is not identity.** Tracing the `_next_stamp` argument accepted a `"modified_at"`
read off *any* object, and carried names through *any* expression mentioning an accepted one — so
`_next_stamp(metadata.get("modified_at"))` and `_next_stamp(stamp + timedelta(days=1))` both
passed. Neither is the value the CAS predicate is compared against, so a writer feeding either
would swap on a token no row ever held and be certified correct on the way past. Row *aliases* are
now tracked separately (`rec = get_record(...)`, `other = rec`), the read must be off one of them,
and prior-token names propagate through a bare name only. *An alias is an identity claim;
provenance is a different question, and this file has now been wrong in both directions by
conflating them.*
- **"Yes, somewhere" came back for the dicts.** Round two fixed it for updates — each is validated
against its own `.values(...)`. That fix then pooled every `modified_at` assignment in the
function into one list, so one correctly-stamped dict authorised a second `.values(**other)` whose
dict had no stamp at all. Verdicts are now keyed by dict name, and each splat resolves its own.
- **And the `dev_property` merge was the class this sweep is about, created by this sweep's own
fix.** Preserving `appraisal` turned a wholesale write into a read-modify-write, with
`realestate.save_appraisal` merging into the same blob from the other side. Both now take
`pid_lock.mutating(pid)` — a lock rather than a swap, because that table has no token to swap on —
and **both** is the load-bearing word: *a lock one side does not take protects nothing*, so
locking only the route review named would have left the pair exactly as racy. `db.refresh` matters
as much as the lock, since `db.get` can answer from the session identity map and the waiter would
otherwise merge into the pre-image it read before the winner committed. Open gaps 14 → 12.
*The first draft of that check passed under its own mutation.* It asserted that a frozen-clock edit
LANDS and that the next edit matches — both still true with the branch deleted, because SQLite
serialises writers and each session sees its own write. It is now the losing interleaving run with
the clock frozen, and removing the branch reproduces the loss exactly: `['GUID-A', 'GUID-C']`, GUID-B
gone. **Two checks that pass under the mutation are two checks measuring the round-trip, not the
guard.**
*Its own self-tests earned their keep within the hour.* Teaching the analyser about mapped model
classes — so two correctly-guarded writes stopped reporting as UNKNOWN — made it read the `t.c`
accessor as a column name, so every write looked swapped-on and both pre-fix bodies came back clean.
The population check would have printed "0 blind" over a tree it had not examined. *A widening and a
rule change are different edits, and widening one can break the other.* And the sibling gate did the
same for its neighbour: routing `revise` through the helper removed `update(` from its body, so
`test_transition_cas`'s exemption for it went stale and redded the build the hour the helper landed.
*An exemption that outlives its site is a hole nobody opened on purpose.*
### A second "send RFQ" minted a second solicitation, and a test called that idempotent
`POST /projects/{pid}/procurement/packages/{rid}/send-rfq` called `create_record` — which commits —
and only THEN looked at `workflow_state`, under an `if … == "draft"` that took the transition and an
`else` that skipped it and reported the unchanged state as a success. So a second send persisted
**another** Bid Solicitation while making no move. Measured on the old code: three sends produced
**ITB-001, ITB-002 and ITB-003**, every response 200 / `rfq_sent`. The same three sends now produce
**one ITB and two 409s**. *(That sentence read "one ITB and a 409" — singular — against a three-send
example, which is the kind of count a reader checks against the numbers directly above it. Caught in
review.)*
**The roadmap filed this as needing a contract decision** between a 409, an idempotent no-op, and a
deliberate re-solicitation with its own round number. **It was already decided, in
`services/api/modules/procurement_package/module.json`.** That workflow is linear — draft → rfq_sent
→ quotes_in → awarded — with no path back to draft and no re-solicit action, so `modules.transition`
answers a second `send_rfq` with a 409 of its own accord; the probe prints
`action 'send_rfq' not allowed from state 'rfq_sent'`. Deliberate re-solicitation would need a new
declared transition, which is a product change and not this fix. *The only thing that made the
refusal invisible was a branch written to route around it* — so there was no decision to take, only
a declaration to stop overriding. The route now moves first and mints only if the move landed.
**The existing test asserted the bug as the contract, and could not have noticed.** It read *"a
second send on the already-sent package doesn't error and reports the current state"*, asserted a 200
and an unchanged `package_state`, and its summary line called the route *"idempotent on re-send"*. It
asserted the two things that stay the same and **never counted the solicitations** — the one thing
that changed. *A word like "idempotent" in a summary is a claim, and this one had a test shaped
around it.* It now asserts the 409 **and** that the refused send minted nothing.
### And reversing the order installed a worse bug than the one it fixed
Raised in review on PR #551, before this shipped anywhere. The fix above moved the package FIRST and
minted second — correct about the duplicate, and **wrong about a failure in between**. `transition`
commits, so a solicitation refused after the move left the package in `rfq_sent` with no ITB — and
this workflow declares **no path back to `draft`**, so the package was stranded where a retry earns
only another 409. The old bug was recoverable by retrying (with a duplicate); the new one was not
recoverable at all.
**It needed no bug to reach.** `create_record` consults `fin_gov.locked_reason`, which raises 409 on
a locked accounting period — an ordinary business rule, on an ordinary day.
The fix is to STAGE rather than reorder: `create_record(..., commit=False)` writes the solicitation
into the session without committing, and `transition`'s own commit then carries both writes as one
transaction. Every refusal now leaves nothing behind — a refused mint never reaches the move, and a
refused move (sequential 409 or a lost CAS race) discards the staged ITB with the transaction.
Nothing in `transition` had to change; `create_record` already took `commit=False`.
*A fix that reverses an order can install a worse fault than the one it removes, because the second
write's failure is the one with nothing left to roll back.* The test drives the real
`fin_gov.locked_reason` path and asserts the package is still `draft`; reinstating move-first fails
it with **"refused mint left the package moved"**, so the ordering cannot quietly come back.
### `transition` was a read-then-write race — in all 139 modules at once
Found while fixing the above, and the more severe half. `modules.transition` read the record, proved
the move legal **from the state it read**, and then wrote with `update(t).where(t.c.id == rid)` —
keyed on the id ALONE, with no state predicate and no row lock. Two callers could both read `draft`,
both pass the check, and both commit. Two concurrent `award` calls both succeeded.
The reach is what makes it worth a release note rather than a line: `transition` is the **only** place
a record's `workflow_state` moves after creation (`revise` inserts a new row; every other mention of
the column reads it), so that one statement backed **139 modules and 342 declared transitions**. It is
now a compare-and-swap — the decided-from state is a predicate, and `rowcount != 1` is a 409 naming
the race, deliberately worded differently from the ordinary "not allowed from state" refusal so the
two are distinguishable by something other than a status code.
Not `with_for_update()`: Postgres honours a row lock and **SQLite treats it as a no-op**, so it would
read as protected on the backend the suite runs on while protecting nothing — the identical trap
`next_counter` had to be dug out of in August. `services/api/test_race_conditions.py` gated the job
claim and the ref counter; transitions were never in its population.
**The gate is `services/api/test_transition_cas.py`**, and two of its twelve checks exist because of
mistakes made writing it. It reinstates the **pre-fix statement shape** against an already-moved row
and requires the blind write to land, so it cannot pass with the guard removed. And its derivation of
"who writes this column" is taken at FUNCTION scope rather than from the text inside `.values(...)`:
the first attempt read only the call's own source, and because `transition` builds a dict and splats
it as `**vals`, **it found one writer and missed the one under repair** — reporting a tree that looked
clean. The gate now asserts it reaches `transition` before it may print any verdict. *A predicate that
decides what to LOOK at is more dangerous than one that decides what to report.*
`apps/web/src/portal/panels/buyoutKeep.ts`'s `rfqSummary` was written against the old rule and told
the user *"a solicitation has been minted anyway, so check whether this package already had one"*.
After the fix that is the one thing such a response has failed to establish, and the duplicates it
sends them hunting for can no longer be created. The branch is kept — a response that is not
`rfq_sent` now contradicts the contract — but it says that instead. *A defensive branch that outlives
its cause keeps giving the advice that fitted the bug.*
### The 3D pin overlay made two calls, and the route meant to replace them had no caller
`apps/web/src/pins/pins.ts` fetched BCF topics through `api.pins()` and register records through
`api.modulePins()`, building a marker from each of two row shapes. It now makes **one** request to
`/pins/all` and runs one loop; the only remaining branch is the CLICK, which is genuinely different —
a topic restores a saved viewpoint, a record opens its register row.
**The roadmap entry understated this by one whole half.** It said the remaining gap was *"what each
row CARRIES"*. That was true and incomplete: **`/pins/all` had ZERO frontend consumers** — no client
method, no call site anywhere in `apps/web`. The route was built, server-tested and dark, so the
overlay made two calls not only because the envelope lacked fields but because nothing had ever been
wired to the route that replaces them. *An item scoped from what a file lacks will miss what nothing
does.*
**CORRECTION, made before this shipped rather than left standing.** The paragraph above originally
ended *"Route-to-client reachability is a different question, and no gate here asks it."* **That is
false.** `services/api/test_route_reachability.py` asks exactly that question, and it is the gate
that caught this change. `services/api/test_reachable.py` — the one I checked — measures whether a
MODULE is reachable, and `pins.py` is imported by two routers, so it passes honestly; I concluded
from one gate's silence that no gate was listening.
**And the real finding is worse than the one I claimed.** The route-reachability gate did not miss
`/pins/all` — it VOUCHED for it. Measured both ways: on `origin/main` its uncalled set has 56
entries and `/pins/all` is not among them, while a grep of `apps/web` finds no reference to it at
all. Its leaf is the single common word `all`, and `leaf_is_called` is the rule that file's own
comments already describe as a standing cost (*"a shared leaf decides nothing"*; *"an English word
containing a route leaf, which no naming convention prevents"*). *A false negative in a reachability
gate is worse than no gate, because the question looks asked.* I did not isolate which string
vouched for it, and say so rather than guess.
PIN-ONE-CALL corrects the gate in both directions at once: `/pins/all` becomes genuinely called, and
`/module-pins` becomes genuinely uncalled — which the gate immediately reported as a new unreachable
route. It is recorded in `KNOWN_UNCALLED` with a reason and an expiry condition (delete the entry
and the route together at the next public-API revision), not silently exempted.
**The design question the entry left open is settled by measurement.** It asked whether the envelope
should carry the viewer's presentation fields or whether the viewer should look them up from
`/modules`. The viewer does not fetch `/modules` anywhere, so the lookup trades one request for
another and defeats the item. The envelope carries `icon` and `source_name` for BOTH kinds: a
register's `source_name` and `icon` from its own `module.json` — which is where the old
`/module-pins` row already read them — and a topic's from a glyph table lifted out of `pins.ts`,
where it was a literal the server could not see, so no server could name a topic pin.
*That last clause replaces a claim that was simply false, and it is worth leaving the correction
visible.* This read "which is precisely why the overlay needed a branch per topic type and a second
loop for register records". Neither half survives looking: the glyph was a **map lookup**, not a
branch per type, and the register loop **already took `icon` off the server row**, so the literal
cannot have been what forced it. What forced the second loop was the two ROW SHAPES — `Topic`
(`anchor`/`title`/`type`) against `ModulePin` (`anchor`/`ref`/`module_name`/`icon`) — which one
envelope collapses. The click branch stays, because restoring a viewpoint and opening a register row
are different acts. *A causal sentence is the easiest kind to write without checking, because it
reads as an explanation whether or not anything caused anything.* Found in review, not by me.
`api.modulePins()` is deleted as the dead client method it became; the `/module-pins` route stays.
**The gate already existed and was asserting the wrong number.**
`apps/web/src/kernel/markupPlugin.test.ts` pinned `["load:proj-1", "modulePins:proj-1"]` — the
two-call behaviour, recorded as a fact. It now pins one entry and a length, so a second request
fails a build. Mutation-verified: reinstating a second `pins.load` reports
`expected [ 'load:proj-1', 'load:proj-1' ] to deeply equal [ 'load:proj-1' ]`.
One test changed meaning rather than being deleted. *"A failure part-way through is still contained"*
asserted that a failure in the SECOND call left the first one's pins drawn — "it got that far". That
half-loaded overlay is unreachable now, so the test asserts its **absence**; deleting it would have
removed the record that the state was ever possible.
*And `test_file_sizes` caught `app.ts` at 2,445 against its 2,442 cap — three lines of comment, the
same shape as the `register.ts` growth one PR earlier. The explanation belongs on the `ResolvedPin`
type, not inside a file under a shrink ratchet. Learning a lesson and applying it are apparently
still two events.*
### Ball-in-court was computed twice, by two rules, and 10 states disagreed
The register screen and every server-side report answered "whose move is it" differently.
`modules_query.court_party` took the primary outgoing transition; `register.ts` unioned the parties
of **every** outgoing transition. Derived over all 139 modules and 267 workflow states, **10 states
in 9 modules** produced different answers. On an open RFI the screen showed Consultant, OwnersRep
*and* GC, while `changeorders.py`, `closeout.py`, `quality.py` and every `ball_in_court` rollup said
Consultant/OwnersRep — the GC appearing only because it can `void`. *An escape hatch is not a move
somebody owes.*
The server's reading survives, because it is what the money reports are built on. But it rested on a
premise nothing could check: *"module authors list the primary forward action first."* **That premise
is false in two places, and they are the ones that matter:**
| | listed first | actually owed |
|---|---|---|
| `pull_plan_task.made_ready` | `reconstrain` (back to `pulled`) | `commit`, by the Subcontractor |
| `permit.applied` | `issue` | `start_review` — nothing is issued before review |
**And it cannot be rescued by deriving "forward" from the declared state order**, which was the
obvious fix and is measurably wrong: `action_item.states` is `['done', 'open']` — alphabetical, not a
progression. Under that reading 45 states have no forward transition at all and 39 have several.
*A convention nothing can check is a convention that has already drifted.*
So the primary is **declared**: `"primary": true` on one transition, or the state is listed as
`resting` — reachable, but owing nobody anything. **Only 17 of 267 states need to declare anything**,
because 199 have a single outgoing transition and 51 have several that all carry the same parties; for
250 of them the choice cannot change the answer. That bound is the design — a rule demanding 267 edits
would have been abandoned halfway and left the tree half-declared.
Five answers change. Two are the corrections above. Three states become **resting** —
`decision.decided`, `information_container.published`, `spec_section.issued` — and stop naming a
party at all, which is what had been putting closed records into ball-in-court rollups as though
someone were sitting on them. Transitioning into a resting state leaves `party_owner` as it was,
exactly as a terminal state already does.
The register now **reads** the value instead of deriving it, and deliberately has no local fallback:
recomputing would be the second implementation again. Both the list and the single-record route send
it, resolved through `workflow_config.effective` — the project's overridden workflow, which is what
`transition` uses, so the screen and the engine cannot disagree by that door either. Enriched at the
two **routes** rather than in `modules_query.list_records`, which has **274 call sites** including a
BCF export built from the row's keys: *the narrowest place that can carry a fact is where it belongs.*
Gated by `services/api/test_court_primary.py`, which derives the ambiguous states rather than listing
them, strips a declaration and requires that to be caught, and mutates `court_party` back to the
first-transition rule and requires **both** wrong answers to return — so the assertions test the
reader and not merely the config.
Two gates went red on that change and both were right, so both are recorded rather than quietly
re-pinned:
**`test_json_null_filter` exempted its sites by LINE NUMBER, and went stale twice in two days.** Not
because the code it describes changed — because unrelated edits further up `modules_query.py` pushed
those sites down the file. *A line number is a coordinate every edit to the file moves, so an
exemption keyed on one measures when the file was last touched rather than whether the site is still
there.* Re-pinning it a second time would have been a fix already known to fail, so the key is now
`(file, enclosing function, subject)`, which moves only when the code genuinely moves.
Collapsing the line into the function costs the ability to tell two sites in one function apart —
`_apply_filters` has two — so each entry now carries **how many sites it covers**, and the gate
deletes one of that pair and requires the count to catch it. Otherwise this would have traded a
noisy failure for a silent one: an exemption still covering the survivor after its twin was deleted.
Three mutations pin it: claiming a wrong count, restoring the line-number key, and dropping an entry
outright are each reported.
**`test_file_sizes` caught `register.ts` growing past its ratchet — by six lines of COMMENT.** The
code shrank five lines (the local ball-in-court union went away); a seventeen-line doc comment
explaining why replaced it. The ratchet was right: the substance of that comment is about the
SERVER's rule and belongs with `court_party` and its gate, not restated in a 2,458-line file that is
under a shrink ratchet precisely because it accumulates. Trimmed to six lines and the now-dead
`ModuleDef` parameter dropped from `ballCell`; **2,458 → 2,447**.
*Both were found by running the suite rather than by reading the diff, and CI's API gate failed on
the same two — which is the argument for running it before claiming a PR is green.*
**A review round on that fix found three more, and two of them were the same defect I had just
fixed one file over.** `test_court_primary`'s list lookup unwrapped `{"items": ...}` and then
iterated whatever was left — a 4xx body is a dict with no `items`, so the unwrap hands the ERROR
OBJECT back, iteration yields its string keys, and `x.get` raises before `check` can report the
route failure. And the new self-tests in `test_json_null_filter` opened with a bare `next(...)`,
which raises `StopIteration` and kills the gate if the site they are built on is ever deleted.
*Both are "dies instead of reporting", written hours after fixing that exact shape — knowing a
defect class by name is not the same as recognising it in your own next paragraph.* Both now report
through `check` and reach the summary; each is mutation-proved by breaking its precondition.
The third was in the rekey itself. `_sites()` mapped each node to its enclosing function with
`ast.walk` + `setdefault`, which binds to the **outermost** enclosing function, not the nearest — so
a NULL test inside a nested `def` resolves against the wrong locals. That was already true when the
owner map only fed `_resolve`; making the function name part of the site's **identity** is what
turned it into a misattributed key no exemption could match. Replaced with a scope-stack descent.
**Measured on today's tree the two walkers agree on every one of the 21 sites** — which is precisely
why a synthetic nested case is asserted: *a fix nothing can distinguish from the bug is a fix nobody
can keep.*
**And the first draft of THAT self-test tested a copy of the fix.** It mirrored the traversal into a
local `_probe` instead of calling `_owners`, so `_owners` could regress to outer-function
attribution and the check would still pass — *a self-test written to stop a fix being silently
reverted, unable to observe the fix.* Worse, the mutation run to verify it was aimed at the copy, so
it proved the copy behaved as advertised and said nothing about the gate. **Mutate the thing under
test, and prove the test can reach it.** It now calls `_owners` directly, and regressing `_owners`
itself is what reds the build. The fixture was also shadowing an unrelated `_NESTED` used by the
resolver self-tests, harmless only because those happen to run first; renamed.
### The PostgreSQL pin gate could be handed a dead server and still exit 0
Follow-up to the pin-sweep work, and a finding that survived two review rounds because it sat in the
review **body** rather than an inline thread — every inline finding was fixed, and this one was not
seen until after the PR merged. *Where a finding is posted decides whether it gets read.*
`test_pin_pgnull.py` connects to the PostgreSQL a run asks for, and failed the build on an
unreachable one only `if _REQUIRED` — that is, only when `AEC_PG_REQUIRED` was set. But `_url()`
returns a URL under **two** opt-ins: that one, and `AEC_TEST_PG_URL`. So a run that set only
`AEC_TEST_PG_URL`, pointing at a dead port, took the connect-failure branch, recorded nothing, fell
through to the no-server path and exited **0** — printing *"no opt-in"* at a caller who had just
opted in.
Measured before the fix, not reasoned about:
AEC_TEST_PG_URL=postgresql+psycopg://postgres@127.0.0.1:1/nope (AEC_PG_REQUIRED unset)
-> test_pin_pgnull OK (no opt-in; ...) exit 0
This is the same shape as the defect the file's own comment already records one flag earlier —
*"a flag that guards one branch of two guards nothing"* — and it recurred because the fix at the
time asked *is this run required to reach a server*, when the question the branch needed was *did
anything ask for this server at all*. By the time that `except` runs, something always has: the
failure is now unconditional. **A message that names the wrong cause is worse than a bare failure,
because somebody acts on it** — the reader would have gone hunting for the opt-in they had supplied.
Verified in four directions: the dead explicit URL now exits 1; `AEC_PG_REQUIRED` with a dead
`PGHOST` still exits 1; no opt-in at all still passes through the genuine no-server path; and the
live-PostgreSQL run still passes. CI was never exposed — it sets `AEC_PG_REQUIRED` — which is
exactly why nothing went red.
### Sorting a register by `updated_at` was a 500; `SYSTEM_COLUMNS` named two columns that do not exist
`modules_query.SYSTEM_COLUMNS` is the allowlist of row columns a caller may filter and sort a
register by. Two of its six names were not columns at all: **`updated_at`** — the register column is
`modified_at` — and **`ball_in_court`**, which is derived from `workflow_state` by `court_party` and
never stored. `_resolve_field` returned `{"_system": True}` for both, so `_field_expr` and
`_display_expr` went straight to `t.c[name]` and raised `KeyError` inside the request handler.
Measured over HTTP against the pre-fix code, on a live register:
| request | before | after |
|---|---|---|
| `?sort=not_a_field` | 400 | 400 |
| `?sort=workflow_state` | 200 | 200 |
| `?sort=updated_at` | **500** | **400** |
| `?sort=ball_in_court` | **500** | **400** |
| `?sort=modified_at` | 400 | **200** |
| save a view with `{"sort": "updated_at"}` | **201, accepted** | **400, refused** |
That last row is the one worth reading twice. `validate_view_config` resolves `sort`, `group_by`,
`agg_field`, `columns` and `filters` through the same `_resolve_field`, with no table in scope at
all — so the phantom could be **stored**. The view saved, the view list returned 200, and the 500
arrived only when somebody applied it. *The stored form is not a louder version of the transient
one; it is a quieter one, and its cause sits two screens from its symptom.*
The fix is both halves, because either alone passes while the other is broken. `_system_field`
validates the name against the register table before the system stub may be returned, so the next
phantom is the 400 an unknown field was always supposed to get; and the list is corrected —
`updated_at` renamed to the column it meant (checked against all 139 modules: none declares a field
by that name, so it shadows nothing), `ball_in_court` dropped. **`?sort=modified_at` now works**,
which is the capability the wrong name was reaching for.
`updated_at` is the **third** instance of that typo on a module row. `quality.py` and `rfi.register`
both read it and got a permanent `None` for `avg_days_to_close`, fixed in August. Those two failed
silently and this one is reachable from a URL — *the same wrong name is not the same severity in two
places, and the loud one was found last.*
Gated by `services/api/test_system_columns.py`, which asserts the allowlist against the live
`TABLES` metadata (139 tables), requires its own derivation to re-find both shipped phantoms before
it may report anything, and then **deletes the runtime guard and requires the `KeyError` to come
back** — a static check that passes with the guard gone is not testing the guard.
### The pin sweep skipped exactly the rows it was written to convert, on PostgreSQL only
`b3c9e42d18a5` — the migration that clears empty pin values so `/pins/all` stops counting them as
candidates — selects every row where either pin column is non-NULL and then decides per column in
Python, beginning `if v is None: continue`. A `json` column holding the JSON scalar `null` is **not
SQL NULL**, and psycopg decodes it, so the driver hands back Python `None`: by the time that line
runs, a row that was never pinned and a row holding `null` are indistinguishable. It walked past
every one of them.
Those are precisely the legacy rows the sweep exists for. Every `None` written to `anchor` or
`element_guids` before that same change added `JSON(none_as_null=True)` went in as the scalar
`null`; they satisfy the SQL half of the pin predicate, fail the exact test in `pins.pin_fields`,
and are what puts the `~` on the plan sheet's project total. Measured against PostgreSQL 16 on a
four-row fixture before anything was written: **the `null` row came out exactly as it went in, and
`SKIPPED` was empty** — so the run neither converted it nor said anything about having seen it.
*(An earlier draft of this entry led with "`changed=1` → `changed=4`", and review was right that the
comparison is unsound: the corrected sweep issues one UPDATE per column where the old one grouped a
row's columns into one, so the number moved from ROWS to COLUMN VALUES in the same change. Two
counts in different units read as a measurement and are not one. The row states are the evidence;
the unit change is now pinned by a both-columns fixture in `services/api/test_pin_empty.py`, which
nothing caught before because every earlier fixture seeded a single column.)*
**SQLite hid it, and not by accident.** There a JSON column is TEXT, the scalar arrives as the four
characters `null`, and the decode happens *after* the guard has already let the row through — so the
conversion works and every local test passes. This is the same `null`-is-not-`NULL` distinction that
migration was itself about, reappearing one layer up in its own reader. *A migration that fixes a
representation problem has to be careful not to inherit it.*
`d7f1a5c3e094` re-sweeps with the question asked in SQL instead: one SELECT per column, each
narrowed to that column being non-NULL, so everything it returns is non-NULL in SQL and a Python
`None` can only be the scalar. It is a new revision rather than an edit, because `b3c9e42d18a5` has
run — editing it would change nothing on any database that ran it while making the file disagree
with what was executed there. Verified on the same fixture: the `null` rows are now SQL NULL, the
already-NULL row is untouched, and a real anchor survives.
**The dialect was never the mechanism — the driver decoding the column is**, and that is why the
gate does not need a PostgreSQL to be real. `test_pin_empty.py` registers a sqlite3 converter so its
driver decodes too, and asserts the corrected sweep converts the row while the migration it corrects
walks past it. The first draft of that section checked only the emitted SQL, and **reinstating the
exact defect on top of the corrected SELECT passed it** — asking the right question and then
throwing the answer away are two different failures, and only one is visible in the statement.
`test_pin_pgnull.py` runs the same shapes against a live PostgreSQL in `db-migrations.yml`; with no
server it does not pass quietly, it asserts that the CI step which runs it is still there.
### A NULL test on a JSON column is a filter that filters nothing — now a gate
Second half of the same defect, as a class rather than an instance. SQLAlchemy persists a Python
`None` in a `JSON` column as the scalar `null` unless the column declares `none_as_null=True`, so
`WHERE <json col> IS NOT NULL` matches every row ever written and `IS NULL` matches none.
`services/api/test_json_null_filter.py` derives every NULL test in `services/api/src/aec_api` by AST
— **21 sites**, re-derived rather than taken from the roadmap's 20 — and resolves each subject
against the model metadata. Three answers are safe: not a JSON column, a JSON column with
`none_as_null=True`, or a JSON *extraction* (`data ->> 'x'`), which is a text scalar and yields SQL
NULL on both backends for a missing key and for a stored `null` alike. One is not. Result: 15 safe,
5 unresolvable subjects each read and exempted with what the subject actually is, and **one live
member of the class** — `Topic.labels` in `client_portal.py`, which narrows on `type == "info"`
first and applies the real test in Python, so it over-fetches and does not answer wrongly. It is
recorded rather than fixed: making it right needs a sweep of the existing `'null'` rows.
It fails closed — an unresolved subject reds the build rather than being assumed safe — and it runs
itself against the known instance before it is allowed to report anything, because a derivation that
reports a clean tree is indistinguishable from one that reports nothing.
**What it deliberately does not assert** is that every JSON column declares `none_as_null=True`.
Measured: 588 JSON columns, 308 without it. Turning those on is a write-behaviour change on 308
columns, not a gate, and almost none of them is ever the subject of a NULL test — which is what
makes the reader the right place to stand.
### @thatopen/ui dropped — the guard was demanding a browser verification of a package that cannot reach the browser
`@thatopen/ui` was a declared dependency that **nothing imported**: zero source references tree-wide,
absent from the built bundle, and a peer dependency of nothing here — it appears only as a
*devDependency* of the upstream `components` packages, which imposes nothing on us. Yet
`scripts/check-fragments-version.mjs` pinned it in `KNOWN_GOOD`, and that guard's failure message
sets a specific contract: re-verify the viewer — load a model, author, section — and move the pin in
the same commit.
So every dependabot patch bump demanded a full live viewer pass for a library that **cannot affect
the running app**. That bill was paid twice: #416 was closed on exactly this cost/benefit, and #545
paid it again — the live pass was done, and its most useful finding was that there was nothing to
verify.
Removed from `apps/web/package.json`, the README pinned-version table, `KNOWN_GOOD`, and
`docs/credits.md` — the last of those mattering because credits name what is *shipped*, and it no
longer is. The npm lock was regenerated with `--package-lock-only` rather than hand-edited: the
removal orphans fourteen packages in total (`lit` and its three internals, `chart.js`,
`chartjs-plugin-datalabels`, `iconify-icon`, the three `@floating-ui` packages, `@iconify/types`,
`@kurkle/color`), and computing a transitive orphan closure by hand is how a lockfile stops matching
what `npm ci` will install. **Zero packages added, zero versions changed** — verified by diffing the
two lockfiles rather than by reading npm's summary, which matters because twenty-one dependencies
carry `^` ranges that a careless regeneration could have floated.
The general point is worth keeping: **a guard that names something unreachable does not protect it,
it only charges for it.** The pin looked like coverage and behaved like a toll.
### numpy 2.5.3 and manifold3d 3.5.3 — two dependabot PRs that could never have gone green alone
#516 (`numpy>=2.5.3`) and #517 (`manifold3d>=3.5.3`) both failed the API test gate, and the reason is
written in the tree they were editing. `services/api/requirements.in` carries it on the numpy line
itself: *"floor raised WITH services/data/requirements.txt: pip-compile reads only this file, so a
floor raised only there cannot move the lock."* Dependabot edits `services/data/requirements.txt`,
which pip-compile never reads — so the lock kept pinning 2.5.2 and 3.5.2, and
`test_lock_satisfies_requirements`, which deliberately reads **both** requirements files, kept
failing. **Structural, not incidental:** no amount of rebasing could have fixed either PR.
Raising both floors lets the recompile follow, and it moves **exactly two pins** — nothing added,
nothing removed. That was predicted from a measurement before the change was made, then confirmed by
CI's `Verify` step on the committed bytes.
**This lock was compiled locally, which is the opposite of the previous entry's conclusion — and the
reversal is the point.** There the bytes were transcribed out of CI because a local resolve had
produced an unrecognisable answer. Once the cause was understood (a fresh output path preserves
nothing; an in-place one constrains to the existing pins), the local tool became the same tool CI
runs, and `Verify` confirmed byte-identity on a lock carrying hundreds of hash lines rather than
alembic's two.
**The clock said it before the diff did.** The fresh-path compile took over forty minutes because it
re-resolved and re-hashed the whole graph; the in-place compile took **thirty-nine seconds** because
it only fetched what moved. A forty-minute run and a forty-second run were never doing the same work,
and nobody had to read a single package name to see that. *When two runs of "the same" command differ
by sixty-fold in cost, they are not the same command, and the cheap explanation for their differing
output is probably not the true one.*
Transcription would not have worked here in any case: the `Verify` step prints `head -60` of its
diff, which alembic's two hashes fit inside and `manifold3d`'s dozens of wheel hashes do not, and the
artifact holding the full file sits behind an egress block. Either route ends at the same gate, which
is what makes either safe.
### alembic 1.18.5 → 1.20.0, and a local resolver that confidently reported the wrong answer
Dependabot's #519 carried this floor bump with a recompiled lock and went fully green, including the
Postgres `alembic upgrade + check + runtime-parity` job. It then became unmergeable: three later
bumps each recompiled `services/api/requirements.lock`, so the branch conflicted in a generated file
that must never be merged by hand. Handing it back to dependabot was not available either — comment
commands posted from this session are rewritten in transit, with U+00B7 injected around the `@` and
inside the command word, so `recreate` and `rebase` both read correctly in the timeline and did
nothing. **A command that looks delivered and is inert is worse than one that visibly fails.**
Redone here, with the lock taken from `lockfile.yml`'s own compile in `python:3.12-slim` rather than
from a local one — and that distinction turned out to matter enormously.
**The local resolve said 34 packages would move.** `anthropic` 1.2.0 → 1.5.0, `websockets` 16.1 →
17.1, `cryptography`, `sqlalchemy`, `certifi`, and thirty more: a one-line floor bump becoming a
tree-wide sweep, which is exactly the kind of scope change worth stopping for. It was wrong. CI's
`Verify` step prints its own diff, and that diff carries `annotated-doc==0.0.4` as an unchanged
**context** line — the very next package after alembic — while the local resolve moved it. `Verify`
then passed on the transcribed lock, proving the real change is alembic **and nothing else**.
**The cause was the invocation, not the environment** — and the first explanation written here was
wrong in a way worth preserving. It blamed the sandbox's package index for serving something
different from the prod container. That was a guess dressed as a diagnosis, and it would have
libelled perfectly good local tooling. `pip-compile` treats an EXISTING output file as constraints
and moves only what the inputs force; the local runs wrote to fresh scratch paths, where there were
no pins to preserve, so everything resolved to latest. Compiling into a copy of the real lock
instead moves **zero** packages — which is also why only alembic moved in CI: its floor was the only
one that changed.
So the reproduction rule is concrete: **compile in place, to the lock you intend to replace.** A
lockfile regeneration aimed at a new path is not the same operation and does not answer the same
question.
*A number with a package list attached reads as evidence.* The list was specific, ordered and
plausible — it was very nearly reported as a finding, and its wrong explanation very nearly outlived
it. What exposed the number was a context line in someone else's diff; what exposed the explanation
was checking whether the two versions even existed upstream, and finding that they did, with cp312
wheels, which left no room for an index story. **When a measurement and a gate disagree, the gate is
the one that graded the artifact — and the first story that explains the gap is not automatically
the true one.**
Worth recording that the floor and the pin differ on purpose. `requirements.in` asks `>=1.19.2`,
what dependabot requested; the lock resolves to the latest satisfying release, 1.20.0. So the
behaviour under test is 1.18.5 → 1.20.0, wider than the floor names, with alembic 1.19's
autogenerate CHECK-constraint change inside that span.
### A digest pin freezes the base image, and the CRITICAL gate blocks on it going stale
The API container build went red on three fixed CRITICAL CVEs in `perl-base` — CVE-2026-13221,
CVE-2026-42496 and CVE-2026-8376, all `Status: fixed` at `5.40.1-6+deb13u1` — with nothing in the
diff that touched a Dockerfile. `services/api/Dockerfile` is byte-identical to main, the base images
are pinned by digest, and the two other images in the same matrix (`node:24-slim`,
`nginx:1.31-alpine`) passed. Nothing about the change caused it; the pinned `python:3.12-slim`
simply predates a Debian security update, and the Trivy CRITICAL gate is doing exactly its job.
**The obvious fix is a bet, not a fix.** Bumping to a newer `python:3.12-slim` digest only helps if
upstream happened to rebuild after the DSA landed — and the tag's head at the time was built
2026-09-01, with no way from here to establish which packages that rebuild picked up. The egress
policy in this environment blocks both the Docker blob CDN and `deb.debian.org`, so the candidate
digest could not be opened and read either; a digest chosen and pushed on the assumption it was
newer-and-therefore-fixed would have been a guess wearing a checksum.
So the runtime stage applies Debian security updates at build time instead. That is not a weaker
supply-chain position than what the stage already did — the same `apt`, the same signed archive, the
same build-time resolution as the `apt-get install` one line below it — and the digest pin is
untouched, so the base layers are still the reviewed ones. **A pin fixes the starting point; it does
not stop the starting point going stale**, and a gate that blocks on staleness needs a fix aimed at
staleness.
**And the first draft of that fix was stale by construction — review caught it.** This entry claimed
the upgrade "keeps working: the next DSA is picked up by the next build". It would not have. Both
container jobs build with `cache-from: type=gha`, and the cache key for that `RUN` is its parent
(a pinned digest, which never moves) plus its command string (which also never moves). The layer
would have been restored from cache on every later build, so the upgrade would have run **exactly
once** — on the build that introduced it — and then frozen at whatever the archive held that day.
`ci.yml` now passes `no-cache-filters: runtime` for the api image, and the stage is named `AS
runtime` to be addressable; the expensive `pybuild` and `converter` stages stay cached.
The shape is the one this repo keeps meeting: **a mitigation whose self-maintaining property was
asserted rather than checked.** It is the same failure as a gate that answers instead of failing,
moved one layer out — the remediation would have been real on the day it was written and inert
every day after, with a green build both times. *A fix that depends on re-running needs the
re-running proved, not assumed.* Worth noting where it was caught: not by a test, because nothing
here can execute a container build — no docker daemon in the authoring environment, and the egress
policy blocks the registry's blob CDN. The check that could fail was CI, and the reader who spotted
the gap ahead of it was a review bot.
### The escaping gate could not see a helper's parameters, so 108 sinks sat outside it
The identity ratchet shipped in the previous entry freezes `file :: expression`. That constrains a
sink whose expression NAMES its source — `${a.filename}` cannot start carrying something else
without the identity moving. It does nothing for a bare value binding:
```ts
const card = (label: string, value: string) => {
c.innerHTML = `<div>${value}</div><div class="meta">${label}</div>`; // identity: "value"
};
```
`value` is the identity whatever the caller passes. Every call site could switch from a literal to
a user-controlled field and the baseline would not move — **the sink goes live while the gate stays
green**. That is the same defect the ratchet was built to fix, one level up: there the frozen key
was a count with no identity, here it is an identity with no source.
**108 interpolations across 27 files**, all now escaped. The population was derived by AST — bare
identifiers reaching `innerHTML` that resolve to a function parameter or a destructuring binding —
and the fix is verifiable in a way most sweeps are not: escaping a sink can only ever REMOVE entries
from the ratchet's baseline, never add one. It went **119 → 74 with zero additions**.
Three things this found that the existing gate could not:
- **The HOT term list was hiding 53 of them.** `budget.ts` interpolated a bare `val` and `evm.ts` a
bare `sub`; neither word matches any term in the list, so both were invisible to the ratchet while
being exactly the shape the new rule exists to catch. Under the term filter the population read
32; under the shape rule it read 85. *A keyword list derived from what someone happened to be
looking at cannot bound a population — a shape can.*
- **The first draft of the new rule tested only for parameters, and reported the tree clean at 85
fixed sites.** 23 more were live in `for (const [label, val] of cards)` — a destructuring binding
is a value handed in from elsewhere exactly as a parameter is, and the name says nothing about the
source either way. *A predicate that decides what to LOOK at hides everything it excludes from its
own count*, which is why the count looked complete both times.
- **`register.ts` carried a comment reading "textContent/esc throughout: ref+title are user data
(stored-XSS guard)"** directly above the one interpolation on that line that was not escaped.
The new rule takes **no baseline and admits no exemption** — the fix is always available and always
correct, so there is nothing to grandfather, and grandfathering one would freeze a sink open by
definition. Its boundary is stated rather than implied: a plain `const x = …` local is out of scope,
because 157 of those reach `innerHTML` in this tree and most hold markup assembled a few lines
earlier (`rows`, `thead`, `bars`, `cells`) where escaping would destroy it. A local assigned from
user data and interpolated bare is caught by neither rule in this file, and the test says so.
**The excluded class was then triaged, not assumed** — an exclusion nobody measures is a blind spot
with a justification attached. All 134 bare locals were classified by their declaration's initialiser
(41 build markup, 39 literal, 15 numeric, 1 already escaped, 36 suspect, 2 unresolvable) and every
suspect and unresolvable one was read. Exactly **one was a real sink**: the CPM panel's critical-path
line, where the server builds `critical_path` as `[r["ref"] or r["id"] …]` — stored, user-entered
activity refs — and the panel joined and interpolated them raw into `innerHTML`. Anyone who can name
a schedule activity could put markup in it and have it run for everyone who opens that panel. Fixed.
Two things about that triage are worth keeping. Its first draft resolved declarations **file-wide and
kept the last match**, so one panel's `gap` (a number) was attributed to an unrelated `el("div")`
hundreds of lines away and reported suspect — the lookup is scope-aware now, and the bug surfaced
only because the suspects were *read* rather than counted. And the single real finding was a **local**,
i.e. in the class the new rule deliberately excludes: *the documented gap was live, not theoretical,*
which is the whole argument for measuring an exclusion instead of asserting one.
**A review round then found the detector's real boundary, and it was hiding a live sink.** The
scanner walked only the interpolations lexically inside an `innerHTML =` expression. But markup is
usually assembled into a local FIRST and only then interpolated — `const rows = deals.map(d =>
\`<tr><th>${d.name}</th>\`)`, then `panel.innerHTML = \`…${rows}…\``. Every span inside that builder
was invisible to **both** rules: not baselined, not flagged, live. The reviewer found one symptom
(`d.name`, a saved scenario name that any user can set); behind that one hop sat **15 hot
interpolations across 8 files**. *The scanner was measuring syntax where the question is
reachability.* It now follows one hop, and stops there deliberately — a local built from another
local is still uncovered, and the test asserts that limit rather than leaving it to be discovered.
Nine of the fifteen were real and are escaped: a scenario name, vendor names in a bid-levelling
matrix, carbon hotspot element names, a takt trade name, project-health domain labels, sources-uses
line labels, forecast line names, and test-fit scheme names. The remaining six are one shape — a
ternary whose branches are literal markup or a glyph (`s.name === r.best ? " ★" : ""`) — which match
the term list on their *condition*, never their value. Escaping any of them would render markup as
visible text, so they are baselined with that reason: **the fix would be the bug.**
Three more findings from the same round, all real. The value-binding rule ignored **constructor and
setter parameters** — a coverage hole in a security gate. It resolved bindings **file-wide**, so a
destructured `rows` in one function made an unrelated plain local `rows` in a sibling fail the
plain-local exclusion; the rationale written beside that code ("a false positive only costs one
`esc()` that was already correct") was wrong, because for *this* exclusion the suggested fix escapes
assembled HTML and breaks the page — *a false positive is only cheap when the fix it suggests is
safe.* Bindings resolve lexically now. And a milestone `<option>` escaped its **text but not its
value**: `esc` turns `&` into `&` and the old `.replace(/"/g, …)` did not, so a milestone named
`A & B` parsed back out as `A & B` and the next request queried the wrong one — asserted now
through a real DOM parse rather than by reading the template.
One test had to be repaired as a consequence, and the repair is the point. The substitution check
hardcoded `portal/panels/evm.ts` and the identity `"value"` as its fixture; this change escaped
every sink in that file, so a legitimate improvement red the test with a message about a missing
baseline entry rather than about the substitution it guards. The fixture is derived now. *A failure
message that can misdiagnose is worse than one that stays silent, because somebody acts on it.*
### A stored XSS the security ratchet had been counting for months
`apps/web/src/ui/innerHtmlGuard.test.ts` froze a per-file **number** of unescaped `innerHTML`
interpolations. A number has no identity, so a file sitting inside its allowance was never looked at
again -- you could delete a safe sink and add a live one in the same file and the gate stayed green
both times.
It was doing exactly that. `register.ts` was allowed 6 and had 4, and two of the four were
`${a.filename}`:
| step | where |
|---|---|
| raw multipart filename, unsanitised | `services/api/src/aec_api/routers/modules.py` (single + bulk upload) |
| persisted verbatim | `modules.py` -> `record_attachments.filename` |
| returned verbatim | `modules.py` list_attachments |
| rendered **unescaped** into `innerHTML` | `apps/web/src/portal/register/register.ts`, both branches |
A user with the `reviewer` role uploads an attachment named `<img src=x onerror=...>.pdf`; everyone
who opens that record's attachment list runs it. The default CSP is framing-only, so inline handlers
execute; `AEC_CSP=1` blocks the script but not the content injection. `esc` was already imported in
that file and used 200 lines away -- the inconsistency the gate's own docstring diagnoses, on the
exact vector it names ("an upload filename").
**There was headroom at the top, too.** `BASELINE_TOTAL` was 85 against 77 actual, and SEVEN files
the baseline had never heard of were sitting in that slack, passing silently. All seven read benign;
the defect is that nothing had assessed them.
**And it was blind to half its own population**, which is the worse of the two defects. The rule
needed `innerHTML` and `${` on the SAME LINE, so any statement wrapped across lines -- the normal
shape for a non-trivial row -- was scanned only as far as its first chunk. Measured while converting
it: **under the OLD same-line rule**, 62 sinks visible and **58 more on continuation lines it could
not see**, among them `v.vendor`, `s.company`, `r.text`, `t.title` and `cert.ref`. *Scope is a
separate question from rule, and a green ratchet is what makes a scope error invisible* -- the same
lesson `services/api/test_ruff_scope.py` paid for.
**Those 62/58 figures are a diagnostic of the old detector, not a remediation count.** They are not
comparable with the numbers below, which come from the new whole-statement detector with widened
terms: the two rules see different populations, so 62 + 58 and 146 do not describe the same set and
no arithmetic relates them. The scan now accumulates the whole statement AND parses interpolations
by brace depth.
**The baseline is now keyed on IDENTITY -- `file :: expression` -- and the count is derived, never
asserted.** An expression the list has not seen fails even when the total is flat or under. A
vanished entry still never fails, deliberately: improving the code must not red the build.
Fixed in the same change: **33 sinks escaped, 30 identities removed** -- measured under ONE
rule against `origin/main` (146 sinks / 125 identities) and this head (113 / 95), because the first
attempt at that figure ADDED two counts taken under the narrow and wide detectors and got 29. *An
arithmetic total across two different scopes is not a measurement.*
Escaped: the filename pair, kanban `title`/`assignee`/`ref`, BCF topic fields in `viewer/app.ts`,
imported schedule activity + trade, an `<option value>` attribute pair, vendor/company/trade across
the analytics panels, a saved-search name, an AI-topic code/section/title row, a certificate ref, a
field-capture label + subject, and a portal comment body. Two PARTIAL escapes were completed -- they
escaped `&`/`<` only, or `"` only, which is worse than none because it reads as handled.
**Two escapers had different coverage.** `ui/feedback.escapeHtml` covers `& < > " '`; `ui/charts.esc`
omitted `'`, and `esc(x)` reads identically whichever you imported. There is now one.
Server side, `serving.stored_filename` normalises the persisted value (basename, control characters,
length) at both upload doors. It deliberately does NOT strip `&` or `<`: those are legal in a
filename, and mangling them would corrupt real names while giving false assurance. **Escaping stays
the renderer's job** -- input filtering fixes an XSS only until the next sink.
The decisive test is a SUBSTITUTION: delete one baselined sink, add a different one in the same
file, count identical. The old rule passed it; the new one names the identity. Verified by mutating
a real source file and watching the gate go red, not by reading it.
### The API client could not take another mixin, and now it can
`apps/web/src/api/client.ts` composed its 49 mixins in one nested `extends` expression. Adding a
50th made `tsc` fail outright — TS2589, *"Type instantiation is excessively deep and possibly
infinite"* — which is how BSDD-LOOKUP ended up lodging two `/bsdd` methods in `ids.ts` against the
route-seam rule ~20 SCALE-SEAM PRs have followed. **A ceiling nobody had measured was silently
setting architecture.**
Measured in both directions, and two plausible fixes ruled out before the working one:
| composition | mixins | `tsc` |
|---|---|---|
| one nested `extends` | 49 | passes |
| one nested `extends` | 50 | **TS2589** |
| `const Stage = withA(withB(…))` split | 50 | **TS2589** |
| `class _StageA extends …` declarations | 50 | **passes** |
- **It is depth, not accumulated members.** A 50th mixin declaring *no methods at all* fails
identically, so trimming the API surface would have bought nothing.
- **A `const` split does not work** — it infers the same nested anonymous type, so the depth is
unchanged. Only a `class` *declaration* names the type nominally, letting the second stage start
from a resolved base instead of re-walking the first.
`client.ts` now stages 25 + 24 through two class declarations, and `api/mixinStaging.test.ts` holds
the shape: a tidy refactor that inlines the stages compiles fine *today* at 49 and strands the next
person at 50, with an error naming neither the cause nor the limit.
**Two corrections this forced.** The BSDD-LOOKUP entry above said the chain broke at a *51st* mixin;
it held **49**, so it was the 50th. That number was asserted, never counted — `git grep -c` would
have settled it in a second, and it shipped. *An unmeasured number reads exactly like a measured one.*
And while mutation-testing the new gate, one mutation **silently failed to apply** and the suite
reported green — indistinguishable from a correct implementation. The mutation now asserts the file
changed before running. *A mutation that does not apply is a test that was never exercised.*
### The reachability gate's blind spot was 70 routes wide, and only two were written down
`LEAF_COLLISION_DARK` is a **triaged** list — routes somebody read, confirmed dark, and recorded. It
was never the population, and the note above it described it as though it were. The population is
every route the reachability rule cannot see *at all*: leaf long enough to check, leaf vouched by an
ordinary English or HTML word (so the rule stays silent), and no caller that builds the path.
Deriving that instead of listing it gives **70 of 947 routes**.
`services/api/test_route_reachability.py` now derives and freezes that set as `LEAF_COLLISION_BLIND`,
the way `KNOWN_UNCALLED` already freezes the routes the rule *can* see. It is a **record, not an
allowlist**: 70 is not a target, and most of the set is probably correct. `/scim/v2/Users` is for an
identity provider and `/bcf/2.1/projects` for external BIM tools — neither should ever have a web
caller. Some are false positives of the probe itself: `openDrawing("plan.dxf?…")` builds
`/projects/{pid}/drawings/plan.dxf` through a helper, so the literal never appears.
**The point is not the number, it is that the number exists.** Before this, the blind spot had no
measured size and nothing could report a new member; now it can only shrink, and route 71 arrives
with a name. Triage is deliberately *not* done here — no syntactic probe can classify these, because
the leaf being vouched is the whole problem (a leaf-based search returns 685 hits for `projects` and
44 for `status`). Each needs a read, and that is its own work.
**Two review findings, both real, both applied.** The derivation matched its probe against all
code; `leaf_is_called` directly above it matches inside STRING LITERALS, for the documented reason
that a bare substring test cannot tell a route from a token containing it — so a probe appearing in
an identifier could drop a route from the set, and since the ratchet fails only on *additions*, that
removal would never be reported. *This file's own recurring defect, one level down, inside the
derivation written to measure it.* The differential was **measured, not assumed: 0 routes today**,
both controls unmoved — so it is preventive rather than corrective, and applied because it closes the
silent-shrink path for nothing. Second: a route *leaving* the frozen set only printed. It now fails,
on `KNOWN_UNCALLED`'s stated reasoning — an entry leaves either because it gained a caller, or
because it was renamed, and then the frozen name is a stale string pre-authorising whatever reuses
that path. *"Both need a human; neither should print and pass."* The weaker precedent in the same
file was the wrong one to copy.
**This took four measurements to get right** — 377 and 35 in a previous gate, then 26 and 70 here —
and the pattern is identical each time: *the checker did not fail, it answered.* So the derivation
carries two live controls it must separate: `/bsdd/search`, wired an hour earlier, must fall out;
`/scim/v2/Users`, callerless by design, must stay in. Mutating the derivation to return nothing reds
the controls rather than silently reporting a clean tree.
### You can now look up a buildingSMART class, not just be told how few you have
The standards panel has always scored *what fraction* of the model carries a bSDD classification
URI -- "bSDD / classification coverage, 38%" -- and offered no way whatsoever to look one up. **A
coverage number with no means of closing the gap it reports.** The engine was already complete:
`services/api/src/aec_api/bsdd.py` is a read-only buildingSMART Data Dictionary client, and
`GET /bsdd/search` and `GET /bsdd/class` had been exposed for months with **no client caller at
all**. There is now a lookup card beside that figure: search the dictionary, open a class, read the
properties it defines.
**An outage must not render as an empty result.** bSDD is a remote service, so three states look
identical as a blank list and mean opposite things -- the dictionary is unreachable (try again), the
dictionary has no such class (it does not exist), your search matched nothing (your query was
wrong). `routers/standards.py` already separated the first two into 502 and 404 for exactly this
reason; `apps/web/src/portal/panels/bsddLookup.ts` is the pure rule that spends them, tested without
a DOM. 500 and 503 deliberately do **not** fold into "unavailable": those are our own bug and our own
deployment, not evidence about buildingSMART, and saying otherwise sends the reader to wait for a
recovery that is not coming.
#### A slower answer could overwrite a newer one
Review (CodeRabbit, PR #540) found a stale-render race, and it was real: the card's two actions —
search, and open a class — both write their result into the **same** element after an await, so the
slower request won regardless of which was asked for last. Search "wall", search "door", and if
"wall" resolved second you read wall results under the door query; clicking a class and then
"← results" raced the same way.
`requestGate()` in `apps/web/src/portal/panels/bsddLookup.ts` is one shared generation counter for
both actions. **One gate, not one per action** — two independent counters would each be internally
consistent and still let a stale class detail overwrite a newer search, because what is contended is
the output element, not the endpoint. Its own test then caught an off-by-one in it: generations count
from 1, so `0` — the value a caller holds before claiming anything — compared equal to the initial
state and read as the live request.
And asserting the gate's behaviour is not the same as asserting the panel uses it that way: giving
each action its own gate **typechecks cleanly** and leaves every behavioural test green. So the tests
also read `standards.ts` and assert one `requestGate()` construction with a guard on both writers.
*Asserting an outcome without asserting the path to it ran is vacuous.*
#### Two corrections to the reachability gate, both about the list rather than the routes
`/bsdd/class` was one of three routes frozen in `LEAF_COLLISION_DARK` in
`services/api/test_route_reachability.py` -- routes the reachability rule cannot see because their
last segment is also an ordinary English or HTML word (`class` is vouched for by ~1,756
`class="..."` attributes). Wiring it turned up two problems with the *list*:
**It was never complete.** `/bsdd/search` is the identical case -- same route group, same absent
`/bsdd/` fragment, leaf vouched by the English word -- and was not in it. The list describes itself
as the output of screening "every route whose leaf this rule reads as called but which never appears
path-shaped anywhere in the web source"; re-running exactly that query returns **twenty-six** routes
against three entries. Most of the remainder are legitimately web-callerless (`/scim/v2/Users` is for
an identity provider, `/bcf/2.1/projects` for external BIM tools), so twenty-six is an upper bound on
candidates and not a defect count
*(Superseded within the hour: **the real figure is 70 of 947 routes.** Twenty-six was measured with a
looser probe that asked whether the route's PARENT fragment appeared, which any sibling route
satisfies — so a called `/lod/assessment` vouched for an uncalled `/lod/matrix`, the same collision
one segment up. DARK-LIST-DERIVE below carries the corrected derivation and freezes it. Recorded
rather than edited away: a number that drifts toward the conclusion it supports is the hardest kind
to catch, and this one drifted in the flattering direction.)* -- but *a hand-curated list that describes itself as derived reads
exactly like a complete one.* The remaining candidates are filed as their own axis rather than
smuggled into a wiring change.
**And nothing could have noticed the fix.** All three per-entry checks assert the blind spot
*persists* -- not flagged, still a live route, leaf still vouched -- and every one of them stays green
after the route is wired, because the blindness is a fact about the leaf collision and has nothing to
do with whether a caller exists. That was measured, not assumed: `/bsdd/class` was wired and the file
re-run unchanged, all green. So the list could only ever decay in one direction, toward naming routes
that are actually reachable. A fourth check now probes for a real caller and reds when one appears,
with a positive control (`/bsdd/search`, wired by this change) so it cannot pass by measuring nothing.
#### `client.ts` has hit a hard compiler ceiling, and it changes where new routes can live
`/bsdd` is its own route group, so by the rule the API client has followed for ~20 SCALE-SEAM PRs --
*the seam is the ROUTE* -- these two methods should have been their own `withBsdd` mixin. They are
not. **Adding one more mixin to the chain in `apps/web/src/api/client.ts` makes `tsc` fail outright**
*(Corrected by MIXIN-CEILING below: this said "a 51st" and the chain held **49**, so it was the 50th.
The count was asserted, never counted. And the 50-mixin ceiling is now BYPASSED, not gone: staging
clears the measured failure, but TS2589 is a property of instantiation depth and a deep enough
composition still hits it. Where the staged limit actually sits has not been measured — saying
"gone" would repeat, in the correction itself, the exact error it corrects. See that entry.)*
with TS2589, *"Type instantiation is excessively deep and possibly infinite"*, on the `extends`
clause; removing it makes the error vanish, so the count is the cause and not anything in the new
code. New route groups can no longer get their own mixin -- they must lodge with a neighbour -- until
the chain is composed in stages. Recorded in `apps/web/src/api/ids.ts`, which took them, because the
next person to reach for `withX()` will hit the same wall and the error message names neither the
cause nor the limit.
### Test scratch directories no longer make `git status` read as uncommitted work
A stop hook flagged `services/api/_shelf_famgeom/` as work somebody had forgotten to commit. It was
not work -- `test_family_geometry.py` creates and removes it -- but nothing ignored it, so for the
seconds it existed it was indistinguishable from the real thing. **That is the entire cost, and it is
not cosmetic: a dirty `git status` from a concurrent or crashed suite run looks exactly like
uncommitted changes, to a hook and to a person.** Thirteen scratch directories were uncovered. The
suite already counted the residue on every run -- "N dir(s) this runner does not own" -- so it was
visible to the tooling and invisible to `.gitignore`.
`services/api/test_scratch_ignored.py` derives the population (every `"./<name>"` directory literal
in `services/api/*.py`) and delegates the verdict to **git itself** rather than reimplementing its
matcher. **Its first two drafts were both wrong in the way this repository keeps re-learning: the
checker did not fail, it answered.** Asked without a trailing slash, git cannot tell an absent path
is a directory, so every directory-only pattern missed and it reported 377 uncovered. Asked from
`services/` instead of the repo root, the paths resolved nowhere, the per-directory
`services/api/.gitignore` was never consulted, and it reported 35 -- which would have had nineteen
redundant patterns added to fix nothing. *A wrong question returns a confident number, and a number
with a list attached reads as evidence.*
So the gate now carries three self-tests, one per way of asking wrongly, and they run **before**
anything else -- a failed classifier prints no verdicts at all. The per-directory canary is
**derived** rather than named: an earlier draft hardcoded `test_ifc*/`, which made removing that
pattern for a legitimate reason red the self-test with a message confidently blaming the wrong cause.
*A check whose failure message can misdiagnose is worse than one that stays silent, because somebody
acts on it.*
### A saved scenario's name could run script in the Finance home
`ScenarioIn.name` is an unconstrained `str` that `create_scenario` persists verbatim, and the Finance
home interpolated it into `innerHTML` unescaped — stored XSS, one `escapeHtml()` away, in a line the
SCENARIO-SOURCES change happened to touch. Now escaped, like every other interpolation in that file.
**The interesting part is why nothing caught it.** `apps/web/src/ui/innerHtmlGuard.test.ts` exists
precisely for this class and was green over it for months, because that file's BASELINE listed
`"main.ts": 2` — and this was one of the two. **The ratchet is one-directional by design (a count may
fall, never rise), so a frozen sink is invisible for as long as nobody adds another.** The entry
means "a future reader should check this"; the check never happened, and a review bot found it by
reading the code the baseline had frozen. The baseline is now `1`, verified by counting rather than
assumed. *A number that licenses a defect is indistinguishable from a number that merely records
one — only reading the site tells them apart.*
### Which deal assumptions have a document behind them, and which came from nowhere
`GET /proforma/scenarios/{sid}/provenance` walks a saved scenario's material numeric drivers — the
numbers under timing, debt, equity, operations, exit and waterfall that produce the IRR an investment
committee acts on — and reports, per dotted path, whether a document citation is recorded against it.
It is R22-PROVENANCE ②, and **it had no client caller**: its leaf `provenance` was read as reachable
because `ProformaResult.provenance` is a *field* of that name on a different reply. **🔎 Sources**, in
Finance → home beside the returns it qualifies, is the first way to see it.
**The engine's own docstring sets the rule the screen has to honour:** *"A coverage figure with no
named gaps is the shape that gets quoted in a memo; the named list is the part that gets fixed."* So
the percentage is never the deliverable — the named paths are, and a `To fix` list renders every one
of them rather than a sample.
**Two ways a true number reads as a stronger claim than it is, and the screen refuses both.**
A scenario can report **100% coverage while every citation points at a superseded revision**. The
engine marks a path `cited` whenever any readable citation exists, regardless of the revision it
names, so coverage and currency are different axes and the headline carries only one. Currency is now
its own line, always.
And **`stale_citation_count: 0` means two different things.** Staleness is computed only when the
caller supplies `?revision=`, so with no revision the count is zero *because nothing was compared* —
indistinguishable from zero because everything is current. The response echoes `current_revision`,
which is the only thing separating them; the screen reads it and says **not checked** rather than
printing a check that never ran. It also offers the input that would actually run it.
**Two distinctions the screen keeps that a summary would collapse.** "No source recorded" and "a
source is recorded but is not a readable citation" are different fixes — find one, versus repair the
one you made — and the engine allows a path to be both. And this is **not** the provenance
`proforma/provenanceLine.ts` already renders: that one is declared-vs-defaulted, this one is
cited-vs-uncited. A number can be declared by the underwriter and have nothing behind it, which is the
ordinary and dangerous case.
**A third instance of the leaf-collision class, created by this change rather than found by it.** The
rules module declared `type Headline = … | "stale" | …`, and a bare `"stale"` string literal is a
whole path segment — so `/projects/{pid}/coordination/stale`, frozen as callerless, read as called and
`services/api/test_route_reachability.py`'s rot check fired within a minute. Renamed to
`"superseded-sources"`, which matches its sibling `"unreadable-sources"` and is the better word anyway.
*Three instances, three syntactic categories — a route path, an identifier, now a string-literal union
member. That the categories cannot be enumerated is the argument for the rot check rather than for a
naming rule.*
### The estimate can be calibrated against what this job actually cost
`GET /projects/{pid}/cost/calibration` compares the model's takeoff estimate with what the project has
**committed** (awarded subcontracts) and **spent** (posted direct costs) and returns the ratio as a
factor. It is the "learns from historical cost data" half of COST-AGENT, it has worked since v0.3.475,
`docs/roadmap-completed.md` records it under "P2 ring (shipped through 492)" — and **no client has
ever called it.** *A completed-roadmap entry is a claim about an engine; it was read as a claim about
reach, and nothing distinguishes the two.* **Calibrate from this job**, in the Estimate card of
Finance → Budget, is the first way to see it.
**Two things the screen refuses to do, and they are the reason it is more than a button.**
The engine clamps the factor to 0.5–2.0. One $500 mobilisation invoice against a $10M estimate is a
raw ratio of 0.00005, which comes back as a confident-looking **0.5** — read plainly, a finding that
the model over-prices by half. *The clamp does not make an unreliable ratio safe; it makes it look
reliable.* All three totals are returned, so `apps/web/src/portal/panels/costCalibration.ts` recovers
the raw ratio and says whether the figure was measured or is a boundary.
It also names what the basis IS. `cost.py` picks actuals over commitments whenever any direct cost has
been posted — right at completion, where actuals are the truth, and wrong at the start, where they are
a fraction of the job. *The preference that is correct at the end is wrong at the beginning, and
nothing in the route knows which end it is at.* Detected rather than guessed: while a job runs, more is
committed than posted, so `actual < committed` is "still running".
**The route's own advice was pointing at the wrong parameter.** Its `apply_hint` and docstring said to
apply the factor by passing `benchmark_factor` to `estimate_from_takeoff`. No HTTP surface accepts a
caller-supplied one — every call site derives it from the project's cost vintage — and the parameter is
already spoken for: it puts the GFA benchmark in the same **dollar-year** as the model total. Feeding
it a calibration ratio raises the benchmark, and since the engine trusts the model only while
`total >= 0.4 * benchmark`, that pushes `recommended` *away* from the model and reports an inflated
benchmark instead. `services/api/test_cost_calibration.py` asserts that direction on the real function
rather than arguing it, and refuses a hint that names the parameter without an explicit negation —
checking itself against the pre-fix wording first.
**Reachability.** The route was invisible to `services/api/test_route_reachability.py` because its leaf
`calibration` is vouched for by 85 uses of PDF-takeoff *scale* calibration. Screening that axis found
four more dark routes hidden the same way — `/bsdd/class` behind ~1,756 `class="…"` attributes,
`/proforma/provenance` and its scenario sibling behind a response *field* of that name, and
`/projects/{pid}/workflow/{key}` behind the plain English noun. They are now recorded there as a named
blind spot that asserts the collision is real, not merely that nothing was flagged. **A route leaf that
is also a common domain noun will be vouched for by the noun** — the rule matches a string without
matching its role, and unlike the earlier one-off collisions this one is a class, not an accident.
### The pay application can be read before it is signed
A GC could **freeze** a pay application and **download** it as a PDF, but never *look* at it. The nine
G702 certificate lines and the G703 continuation sheet existed on screen nowhere. **Review pay app**
in Finance → Budget now renders both from data, between seeding the SOV and creating the owner
invoice — which is the order the work actually happens in.
**This is the direct sequel to the over-billing bug below, and the reason it is worth doing.** That
defect lived in line 7, "less previous certificates for payment". Line 7 could only be seen inside a
PDF blob, so a wrong zero there was invisible until the next draw asked for the whole job again.
*A number a person cannot see is a number nothing checks.*
The screen does more than print. It verifies the certificate's own arithmetic — line 3 = 1 + 2,
line 6 = 4 − 5, line 8 = 6 − 7, line 9 = 3 − 6, and lines 4 and 5 against the continuation sheet's
totals — and says so above the numbers, because a certificate that does not add up is what a reviewer
most needs told. Comparisons are in integer cents: this codebase has already had HALF_EVEN and
HALF_UP disagree by a penny inside one G702, and `0.1 + 0.2 !== 0.3`. Line 5 is *skipped* rather than
failed on a final application, where `release_retainage` zeroes it by design — a check that reports a
correct certificate as broken teaches the reviewer to ignore the checks.
It also names **where line 7 came from**: deducted from what a submitted application actually
certified, or equal to the schedule of values re-added. Those are not the same fact — the first is
historical and does not move when the SOV is edited; the second is a reconstruction that does. Saying
which keeps a reconstruction from reading as a signature.
`GET …/cost/g702` and `GET …/cost/g703` had answered all of this since they were built and had no
client caller. They sat in the reachability gate's `CONFIRMED_DARK_SHORT_LEAF` with a note saying a
screen rendering the application from data would use them and none existed — and that note was the
whole entry, naming the missing work and waiting. Short-leaf ratchet **5 → 3**, and the three that
remain are all *correctly* dark: the SAML assertion-consumer endpoint the IdP POSTs to, and two
scheduler polls the server-side runner reads. **The ratchet no longer records any parked work.**
### A submitted pay application no longer makes the next one re-bill the whole job
**This is an over-billing bug that shipped**, and it is fixed. On a $100k line at 10% retainage with
$18,000 genuinely due, creating an owner invoice with the button in Finance → Budget and submitting
it moved G702 **line 7 from $45,000 to $0 and line 8 from $18,000 to $63,000**. The next draw asked
for everything earned to date, again.
**Why.** There were two builders for the same document. `cost.build_application` freezes the whole
application — the G703 continuation sheet, all nine certificate lines, `sov_snapshot_at` — and sat
behind `POST …/cost/pay-application` with **no client caller ever**. The route the button actually
called, `…/cost/pay-app/invoice`, assembled its own record from `g702()` **line 8 alone** and stored
`{number, amount, period, status, prime_contract}`.
The thin record was not merely smaller. `cost._certified_to_date` fills line 7 by reading
`current_payment_due` off submitted applications — a key the thin record does not have. So the sum
came out `0.0`, and **`0.0` is not `None`**. `None` is the sentinel meaning *fall back to
reconstructing from `completed_prev`*, which would have been right. A real number suppresses the
fallback. Creating the invoice therefore replaced a correct answer with a wrong zero — *the failure
needed the feature to be used*, which is why nothing caught it.
**The fix, and it is a consolidation rather than a patch.** `…/cost/pay-app/invoice` now calls
`build_application`, so there is exactly one builder and the record it writes carries its own
arithmetic. `…/cost/pay-application` is retired. The `prime_contract` link, the one field the thin
route set and the builder did not, is carried into the builder so consolidating trades no fact away.
Two more things fell out of reading it:
- **Every application was called "App 1."** `app_no` defaulted to `1` server-side *and* the client
always sent `1`. It is optional now in both places, and the server allocates the number from the
same atomic `ref_counters` row the human refs use. The button is relabelled **"Owner application
from draw"**, because that is what it makes.
- **The period roll had two implementations too**, and they disagreed. `…/cost/pay-app/advance` was
dark and counted *every* SOV row; the live `…/cost/advance-period` counts only rows with work
billed this period. The dark one also returned a `next_application_no` derived from an
`application_no` field on SOV lines that nothing sets. It is retired, and `test_closeout.py` — which
had been written against the dark route's response shape — now exercises the live one.
**Review found that the first draft of that numbering was itself a defect, and named the wrong half
of it.** It read `app_no = 1 + len(records)`. `RefCounter`'s own docstring, in `services/api/src/aec_api/models.py`,
says that counter exists *because* a COUNT(*) scheme let a later create reuse a number after a
delete — so the fix for a money bug had reintroduced, on a money register, the exact scheme this
codebase retired for refs. The review described it as a race between concurrent requests, which is
real: no unique index on `data.number` refuses either writer, so two applications both become
"App 3". But the reuse half needs no concurrency at all — delete application 3 and the next one is
"App 3" again, every time — and that is the half that leaves a register unable to say which
application was paid. *A race-shaped description hides the half that reproduces deterministically,
and the deterministic half is the worse one.*
The allocation now goes through `modules.next_counter`, extracted from `_next_ref` so the primitive
is shared rather than private to its first caller. Its seed is a callable, consulted only when the
counter row does not yet exist, so the ordinary path stays one atomic `UPDATE … RETURNING`.
**And a second review round found the counter half-wired.** Only the *allocating* path moved the
mark, so a number chosen explicitly by a caller did not. Measured rather than argued: applications
1, explicit 7, then omitted produced **[1, 7, 2]** — no duplicate yet, which is exactly why the
first round missed it. The collision is deferred to whichever allocation later climbs back to 7 and
names an application that already exists. `modules.raise_counter` now lifts the mark to any explicit
number, atomically and portably (`CASE WHEN n < :v THEN :v ELSE n END` — Postgres spells the scalar
maximum `GREATEST`, SQLite has no such function and its `MAX` is the aggregate). *An off-by-one you
can see is safer than a collision you have to wait for.*
Two more from the same review: a non-positive `app_no` stored `"App -2"` while the route's
trailing-digit parse reported `2` — refused now at the builder, so a direct caller cannot bypass it —
and the "Owner application from draw" button is disabled for its own round trip, since creating an
application is not idempotent and server-side numbering labels duplicates rather than preventing
them.
Six regression assertions pin the money, including that the retired builder stays retired: its
response shape differed, so a caller written against it would silently receive a record where the
survivor returns an envelope. Routes 949 → 947, uncalled 58 → 56, short-leaf ratchet 5 → 3.
*A record that keeps the answer and discards the arithmetic is not a smaller record — it is one that
something downstream was already trying to read.*
### Every trade partner's record with your firm, and what it does not cover
`GET /benchmarks/vendors` has reported each subcontractor's cross-project commercial and compliance
history since R22-PROCURE-DEPTH ③ — subcontracts, commitments, invoices, COIs, lien waivers and
warranties, from six registers — and had no client caller the whole time. Its five `/benchmarks/*`
siblings all had one. It now renders in the Benchmarks panel, worst first.
**It is the route the entry below blinded the reachability gate to, wired in the same pull request.**
LEDGER-BROWSE's entity list contains `vendors`, so `/connections/{cid}/erp/vendors` became a URL this
client builds and the leaf `vendors` read as called — by something else. Wiring this removes the
blind spot's *subject* rather than leaving a record of it. The gate's two assertions pass unchanged
either way, which is exactly why they stay: **it could not see this route dark and cannot see it
wired.** Its verdict was, and remains, no verdict.
Three rules in `apps/web/src/portal/panels/vendorScorecard.ts` (20 tests), and all three are the
server's own — stated there in prose and enforced here, because a caveat that lives only in a JSON
field nobody renders is a caveat nobody reads:
- **`no_history` is NOT `clear`.** A sub who has never worked for you is an unknown. The engine calls
a clean-looking record for an unexamined vendor "the single most expensive mistake this module
could make" — and it is a *rendering* mistake as much as an engine one: three verdicts that reach
the same pixel are one verdict. An unrecognised fourth verdict does not inherit green either.
- **A clean record here is silence about quality, not a pass.** `ncr` and `inspection` carry no vendor
field, so nothing in this scorecard has looked at quality or schedule performance. The attribution
line is stated even when the payload omits it — the silence is a property of the route, and a
missing caveat reads as no caveat.
- **A certificate with no expiry is not cover.** "Expired", "expiry unknown" and "none recorded" are
three states; printing "0 expired" for a vendor with no certificate at all is the `no_history`
error one field down.
The headline states how many vendors have no record at all beside the watch count, so "3 on watch"
cannot be read as "and the rest are fine".
### An accounting connection's books can be read from the app
`GET /connections/{cid}/quickbooks/{entity}` and `GET /connections/{cid}/erp/{entity}` return a
QuickBooks or Sage/Viewpoint chart of accounts, vendor list or bill list. Both were finished on the
server and callable by nothing. A **Books** action now sits on every QuickBooks, Sage and Viewpoint
connection in the Data-connections modal.
**The two halves were dark for two different reasons, and only one of them was recorded.**
`erp` is three characters, under the reachability gate's `MIN_SEGMENT`, so the short-leaf ratchet
held it and named it as the next wiring candidate. `quickbooks` is ten characters, IS assessed by
the main rule, and the rule believed it called — off the string `"quickbooks"` in the connection-TYPE
`<select>`. A product vocabulary word, not a URL. The listed one was the one anybody could see.
Three rules in `apps/web/src/connections/ledgerBrowse.ts`, each because the server's answer and the
obvious rendering of it say different things:
- **A vendor failure is HTTP 200 with `{"error": …}`.** The route catches the vendor exception and
puts the message in the body, so `res.ok` is true and the row list is absent. Rendering that as an
empty table tells an accountant their books are EMPTY when the truth is that we could not read
them. `outcome()` refuses to collapse the two, and checks `error` first and on its own — testing
the rows first would classify every failure as "no records".
- **The QuickBooks count is capped at 50 and nothing pages past it**, so `count: 50` there is a floor
and a company with 200 accounts sees 50 with no indication. The ERP read is uncapped. The same
field means different things on the two routes, so it is never printed bare.
- **Row keys are vendor-cased** — Intuit's `Name`/`Id` against a generic ERP's lowercase — which is
the COL-PAIR defect class, and is why the server's own `_info_erp` already hedges.
**Wiring a route blinded the gate to a different one, in the same commit.** The entity list contains
`vendors`, so `/connections/{cid}/erp/vendors` is now a URL this client builds, and
`/benchmarks/vendors` — dark since the gate was written, and frozen in `KNOWN_UNCALLED` ever since —
immediately read as called. It is not; its five `/benchmarks/*` siblings have client methods and it
still has none. Reachability work is not monotone: a new URL adds a segment to the corpus, and any
dark route sharing that last segment leaves the gate's sight. It is recorded now as a blind spot in
neither set, beside `/asset-rights/verify` and the two jurisdiction routes. The rot check earned its
keep — it was a bare `print` until 2026-08-20, and under that version this route would have dropped
out of the frozen list with no output at all.
### Two type-aware gates now see the tree on Windows
`deadFieldTyped.test.ts` and `responseUndeclared.test.ts` failed 9 tests on Windows and passed on
CI. Both compared a root built with `path.resolve` — backslashed on Windows — against TypeScript's
`sf.fileName`, which is forward-slashed on every OS, so `startsWith` rejected every source file and
both derivations walked an empty tree. Linux has no backslashes, which is why CI never saw it.
(`ts.sys.resolvePath` is not the fix: it hands the backslashed path back unchanged.)
They failed closed, which is the point of their vacuity guards — with one exception worth naming:
*"the tree's undeclared keys are exactly the known ones"* **passed** on the empty tree, because
`KNOWN_GAPS` is empty and so is a walk that looked at nothing. Only the guard beside it went red.
The fix had a trap in it. `deadFieldTyped`'s two filesystem scans compared their walked paths
against the same `API` prefix, and matched on Windows only because both sides were backslashed.
Normalising the prefix alone lets `api/` into both scans and every assertion still passes —
measured: the sound candidate set fell from 386 to 348, all green. So the two identical walks are
now one normalised walk, and a new assertion requires every declaring file the checker found to be
one the scans' `api/` exclusion removes; the un-normalised walk now fails it. `extract()` normalises
its own root, and a new self-test hands it a backslashed one, so the regression is caught on Linux
CI too.
### An npm override can no longer disagree with the dependency it overrides
`eslint` moves to **10.10.0**, and the engineering standards say so — `toolchainDocs.test.ts`
refused the bump until the prose caught up, which is exactly what it is for.
The new part is `apps/web/src/tooling/npmOverrides.test.ts`. The root manifest pins some packages
through `overrides`, and when such a pin drifts from the range a workspace declares, npm refuses
the whole install with `EOVERRIDE`. That already happened here once, on this same pair. The rule
has lived only in a changelog entry since — and a changelog does not run.
The population is derived rather than listed: every overridden package that is also a declared
dependency. Three of the four current overrides pin transitive packages nothing here declares, so
there is no pair to disagree and they are outside the population rather than exempted. The
comparator returns `null`, not `false`, for a shape it cannot read, and `null` reds the build — a
checker that quietly approves what it cannot parse goes green exactly when it stops understanding
its subject.
**npm applies two different rules, and the gate now applies each in its own position.** In the
root manifest — the same file as the `overrides` block — the two specs must match as RAW STRINGS;
`">=10.9.0"` beside an override of `"10.10.0"` is semver-compatible and npm still refuses it. In a
workspace manifest the raw rule does not reach, and npm resolves the declared range and rewrites
it to the override; `package-lock.json` records `apps/web` resolving `^10.10.0` to `10.10.0`,
which is npm's own proof. A single rule applied to both positions is wrong in one direction or the
other: too loose at the root, and red on a configuration that demonstrably installs in the
workspace. `$name`, npm's own escape hatch, is accepted in both.
### Extension-of-time claims, with the method that produced them
**Schedule** now assembles an **extension of time**. Pick an analysis method, and it grades each
delay event against the activity's float, states what the claim is worth, and shows its working.
It will not compute one without a method. The published taxonomies exist because the same facts give
different answers under different methods, so an EOT figure without its method cannot be weighed by
whoever reads it — and this one ends up in a claim. The method list comes from the engine itself, and
choosing one shows that method's own published weakness before you run it.
Two ways to run it, and the default is the auditable one: **from the captured baseline**, measuring
slip against a named, dated artefact and taking causes from the project's own detected events, so
both can be re-derived later. Typed dates still work for a claim argued before a baseline exists, and
are labelled as what they are — the baseline is the single most contested input in a delay claim, and
typed it cannot be checked.
Four things it refuses to round off, each because rounding it off would favour one party:
- **A delay absorbed by float is reported as absorbed, not as zero.** It earns no extension and it
still happened; only the second reading says nothing occurred.
- **Slip with no matching cause is unattributed, never non-excusable.** Defaulting unexplained
overrun to contractor risk hands somebody a finding nobody demonstrated.
- **Concurrent delays are named, never split.** Whether overlapping employer-risk and
contractor-risk delay gives time, money, both or neither is a contract question the protocols
themselves disagree on; a silent 50/50 would present the most argued number in the discipline as
arithmetic.
- **A detected event with no stated duration is excluded and counted.** Detection establishes that
something happened, never what it cost.
### Pedestrian wind comfort, screened from the massing
**📐 Design Metrics** now carries a **pedestrian wind comfort** screen. Give it a height, width and
depth — or leave them blank and they come off the model's bounding box — plus the site's mean
pedestrian-level wind, and it grades corner acceleration, downwash and channelling against the
**Lawson comfort categories**, lists the zone speeds, and names the standard mitigations: corner
chamfers, a podium, an entrance canopy, porous screens in a passage.
It answers in a second, which is the point: massing decides most pedestrian wind outcomes, and a
wind-tunnel study arrives long after the massing is fixed. **It is a screening heuristic, not CFD
and not a wind study** — no external service, no simulation licence, no weeks of turnaround — and it
says so on every result.
Three things it will not do. It will not call a result acceptable when a check did not run —
channelling is only looked at if you give it the gap to the neighbouring building, and without that
the screen says plainly that it is partial. It will not let the default site wind pass as a
measurement, because every speed on the screen scales directly with it. And when the dimensions came
off the model it says so, since a bounding box spans the site and context geometry as well as the
building.
### Buyout packages can be kept, and their RFQs sent
**Budget → Buyout packages** has always grouped the model's priced quantities into packages, each
with an RFQ scope. There was no way to keep them: the grouping vanished when you navigated away, and
the buyout workflow it feeds — draft → RFQ sent → quotes in → awarded — could not be reached from the
screen that produces its input.
Keeping them now creates one **Buyout Packages** record per group, in draft, visible to the whole
project and quotable by ref; each one then offers **Send RFQ**, which mints a Bid Solicitation
carrying the package's name, trade and due date and moves the package to RFQ sent. The confirmation
says what is being created before it is created, and re-grouping and keeping again is described as
adding a second set rather than replacing the first, because that is what it does.
Two refusals are reported rather than hidden. A grouping that produced nothing is reported as nothing
kept, not as a save of zero. And because the server mints a solicitation whether or not the package
was still in draft, a send that does **not** move the package says so and names the state it is
actually in — so a second send cannot look like a first one.
### An authority's data requirements can now be imported, applied and checked
A **requirement pack** is a set of data requirements published by an authority for a jurisdiction —
what a submitted model has to carry before it will be accepted. The whole mechanism was built and
none of it could be reached from the app: not the shared library, not the import, not the delete,
and not the two reads that apply a pack to a project and check the model against it.
All of it is now under **CDE / Standards → Data requirements (jurisdiction)**. Packs resolve from
the project's jurisdiction; a project with none is told so rather than given a default, because
requirements from the wrong authority are a different answer that looks exactly like the right one.
Every pack must carry its **authority, edition and source** before it can be stored, and every
result says whose rules produced it — a compliance number detached from that is the kind that gets
quoted at somebody. The built-in demonstration pack is labelled as one wherever its results appear,
so it can never be mistaken for a finding. An import that is refused shows the server's own reason,
which names the missing citation or the exact selector that would have silently matched nothing.
### The triangle census offered a saving you could not take
Model QA → Standards has always measured where a model's triangle budget goes and told you what a
coarse stand-in would save — *"proxy would save 47%"*. There was no way to build it.
There is now, in the same place the saving is quoted. It writes a **separate** file beside the
model; the model itself is untouched, and every box in the proxy is stamped as a stand-in that must
not be measured, scheduled or priced — a coarse shape mistaken for real geometry is worse than no
coarse shape at all. When a model has nothing worth replacing, the button is absent and says why
instead, and a build that stores nothing reports that plainly rather than as a success.
### Prefab kits are on the screen, and a released kit's scope can finally be written down
The prefab-kit register was built and had no way in. Every kit, worst first, now appears under
**Work → Prefab Kits**: its scope, its bill of materials, and the specific things blocking it, in
the order the shop cares about — a kit whose released scope has *drifted* from the model outranks
one that is merely late, because a late kit is a known problem and a drifted one is an unknown
wrong one.
The write that matters is **writing the scope**. Until a kit's scope is written down it is a
selector — a live query against the model — and a released kit in that state hands the shop
something that can change under it without anyone being told. Writing it resolves the selector once
and records the exact list of elements, after which any divergence is reported here instead of
silently changing what is being fabricated. Re-writing a scope says so plainly, because it can
change what a fabricator is already building.
The register refuses to write a scope that is empty, that comes from a selector which does not
parse, or that was cut short by a result cap — the last being the one nobody would notice, since
the count looks plausible and the list is quietly short. When no model is loaded the panel says the
figures cannot be checked rather than showing zeroes, which would be a wrong answer in the shape of
a right one.
### You can share a saved view — and delete now tells the truth about it
Saved views on a register could already be shared with the whole project. The app just never offered
it, so every view anyone made here was private whether they wanted it or not. Saving one now asks.
Three related things were wrong and are fixed together. Shared views from colleagues looked exactly
like your own private filters in the dropdown — they now say whose they are. The delete list offered
every view on screen, including other people's, which the server refuses; picking one reported *"was
already gone"* and then it was still there. It now offers only views you can actually delete. And the
confirmation claimed a view was *"yours alone"* even when deleting it would remove it for everyone
using it — it now says which.
### A lender draw package shows the schedule of values it was computed from
Generating a G702 draw package reported the payment due and how many schedule-of-values lines were
created, and stopped there. Two things it had already worked out went unseen: the G703 totals the
certificate is computed from — completed to date, retainage held, balance to finish — and the deal's
returns re-forecast with your actuals folded in.
That second one is the entire point of running the draw off the underwriting model: it answers "is
this still the deal we signed?" Both are now on screen beside the payment due.
### A pro forma now says how much of it you actually supplied
Solve a deal and Massing reports an equity IRR, a yield on cost, a development spread. Some of the
inputs behind those are yours; the rest the engine filled in with defaults. Until now the screen
could not tell you which, even though it had the answer — the server works it out on every single
solve and the app was discarding it.
Under the underwriting guardrails you'll now see a line like *"4 inputs behind these 6 figures were
defaulted by the engine: exit.cap_rate, debt.rate, ..."*, or confirmation that every input was
declared. It doesn't change any number. It tells you which numbers are worth arguing about, which is
the difference between a pro forma you can review and one you can only read.
If your server is older and doesn't send the breakdown, the line is omitted rather than claiming
everything was declared.
### A mistyped spec section is now caught while you can still fix it
Attaching a MasterFormat or UniFormat code to an element used to be typing into an empty box with an
example beside it. `80 51 00` instead of `08 51 00` — two digits swapped, into a division that does
not exist — was accepted without comment and became a permanent classification on that element.
Massing already knew the answer: the server publishes the full MasterFormat division master and the
UniFormat-to-MasterFormat crosswalk, and the app was throwing both away as they arrived. It now
reads your code back to you as you name it: *"Division 08 · Openings (Architectural)"* for a section
it recognises, and a warning for one it does not. UniFormat codes get the crosswalk — *"B20 ·
Exterior Enclosure → MasterFormat 04, 07, 08"* — so you can see what an element will be bought
through.
It warns, it never blocks: MasterFormat reserves whole ranges for divisions you define yourself, and
those are legitimate codes. If the reference can't be reached, classifying works exactly as before.
### Sealing a project now tells you the key people need to check it
When you save a project as a sealed `.mass` file, Massing signs it so that anyone else can confirm
the file is authentic and unaltered. The dialog said exactly that — and then could not tell you
which verification key to publish, because the app was never given it. It now names the issuer and
prints the key alongside, so you can hand it to whoever needs to check your releases. On a server
with sealing switched on but no signing key configured, nothing is shown, because there would be
nothing to verify against.
### The offline cost baseline can now actually be installed
The budget panel has been telling cost managers that an offline cost baseline is *"installable — no
subscription"*, and the app had no way to install one. The server has always had the importer; no
screen ever called it.
It is now in Profile & settings → Administration, under Server. It is deliberately not on the budget
card: installing a vintage reprices **every project that has not pinned one**, which is a server-wide
change and not something to offer beside one project's numbers. The budget card now names who can do
it instead of implying you can. The confirmation spells out the consequence, and projects with a
pinned vintage are unaffected.
### SAML single sign-on now works, and has a button
If your organisation runs its own identity provider — Okta, Entra ID, Ping, Google Workspace as a
SAML IdP, anything that speaks SAML 2.0 — Massing could already be configured to accept it, and the
sign-in screen still had nowhere to click. Worse, a server with SAML configured and no OAuth
provider showed a note advising you to set up Google or Microsoft sign-in instead, which was the
opposite of the truth. The login window now leads with **Continue with single sign-on** whenever
your server has an IdP configured and your plan includes SSO.
Two things had to be true for that button to be worth adding, and only one of them was. Signing in
through SAML had never actually worked: the server verified the assertion, created or found the
account, issued a valid session — and then stored it under a name the rest of the server does not
read, so the browser came back to Massing still signed out. The automated test covering that flow
checked that *a* session cookie was written, using the same wrong name the code did, so it reported
success for a sign-in that had never once let anybody in. Both are fixed; the test now asks the
server who it thinks you are, which is a question no spelling mistake can answer correctly.
If you had SAML configured and gave up on it, it is worth another try.
### A check that looks for unreachable features stopped being fooled by coincidences
Massing has an automated check whose job is to notice when part of the server ships with nothing in
the app able to reach it — a feature that exists, is tested, and that no user can get to. It decided
whether something was reachable by searching the app's source for the feature's name, and it searched
for that name *anywhere*, including in the middle of longer words.
So it could be satisfied by an accident. Adding a field called `available_public` to the cost screen
was enough to convince it that an entirely unrelated public-listing feature had been reached, because
the letters matched. The check then reported all clear — for something nobody had ever wired up. It
had been fooled this way twice before, each time noticed only by chance.
It now requires a whole-word match. That change found five more features the app cannot currently
reach, which are recorded for a later pass; it was verified not to lose any of the ones that really
are reachable. The first attempt at the fix was stricter still and would have wrongly declared 23
working features unreachable — the drawing exports among them — so the check now carries examples of
both what it must reject and what it must accept.
### Clearing an imported schedule now tells you whether it cleared anything
The "Clear import" button in the schedule panel undoes a Primavera P6 or MS-Project import by
deleting the activities that import created, leaving anything hand-entered alone. It reported
success the same way whether it removed four hundred activities or found no import at all — the
server always answers "cleared", and the count that distinguishes the two was never read. It now
says how many activities were removed, or that there was no import on record to remove — and when
it is talking to an older server that does not send the count, it keeps the previous wording rather
than guessing which of the two happened.
That was one of nine places where the app was throwing away something the server had already told
it. All nine now read the field; five of them show it:
- **Project health** reports what the incidents *were*, not just how many. The safety figures are
broken down by OSHA classification — the first thing the server works out, and the one figure the
card could not display — so five incidents no longer read the same whether they were near-misses
or recordables.
- **Waiting for a model to convert** shows how long it has been going. A large model legitimately
takes minutes, and the status said "running" from the first second to the last, which is also what
a stuck job looks like. The elapsed time comes from the server, so it stays right across a page
reload — and it is left out entirely rather than guessed at if the two clocks disagree.
- **Saving a pro forma scenario** reports the returns it solved to. The server solves the scenario
as it saves it and sends the result back; the app discarded it, so a scenario that solved to
nothing looked exactly like one that solved well until you opened the Portfolio.
- **The cost-database card** says what can still be installed. With nothing installed it read "0
vintages installed", which sounds like there is no cost data to be had — when an offline public
baseline needs no subscription at all.
The remaining four are values that repeat something the app already knew, or belong to screens that
do not exist yet; they are declared so the compiler and the audits can see them, and noted as such.
### The app now checks that it declares every field the server sends it
A field the server returns but the web app never declares is invisible — not just on screen, but to
the compiler, the linter, and every automated audit, all of which start from the declarations. So it
does not look like a bug. It looks like nothing.
Fifteen such gaps are now named and held by a check, and six have been read closely so far. Three of
them are not missing text on a page but a whole capability that cannot be reached: single sign-on is
fully built on the server and has no button anywhere in the app, so an organisation that configures
it still cannot use it; a signed release publishes the key needed to verify it and the app never
shows that key, which leaves a verifier trusting the document's own copy rather than an independent
one; and a saved view can be shared by the server but the app can only ever create a private one.
Another drops the payment-application schedule of values from a draw package; one fetches four
classification vocabularies over the network and keeps one; and one works out where an underwriting
result's assumptions came from and then never shows it.
None of this was reachable by searching for names. A name can mean two different things in two
places, and two different fields can share a name — so a text search answers confidently and wrongly
in both directions. The check reads the real types instead, and refuses to report at all on anything
it cannot resolve, because "found nothing" and "did not look" must never produce the same result.
### The desktop app is now started and checked before it is published
The installers for Windows, macOS and Linux were built, signed and put in front of users without
anything ever launching them. That is how the Linux app shipped in a state where it could not start
at all: every automated check reads the source code, and the fault only existed in the packaged
copy. The checks were green and the app was broken, and those two facts had nothing to do with each
other.
Every build now starts the server the app runs on and uses it before the release is allowed to
proceed — under the exact conditions the crash needed, since the same build started fine under
slightly different ones. It has to answer, reach its database, list every one of the 139 registers,
and serve the interface. On Linux that happens twice, and the second time is the one that counts:
the server is taken back out of the finished installer, the file a user would actually download, and
started from there. On Windows and macOS only the first check runs — the installers themselves are
still built and published without being opened.
The check is deliberately unable to pass quietly. If the app it is supposed to test is missing, that
is a failure rather than a skip — "nothing was wrong" and "nothing was looked at" are the two
outcomes that must never be confused. It compares the registers it gets against the registers the
project defines — by name, one for one — rather than against a round number, because an app that had
silently lost thirty of them would clear a round number, and one that lost a register while
double-loading another would clear a count. And it was itself tested against three deliberately
broken builds before being trusted, including a reproduction of the crash that shipped.
It also builds a model and opens it, which is the part that matters most and the part a startup
check cannot reach. The machinery that turns a building model into something the 3D view can draw is
only loaded at the moment someone publishes — so an installer that lost it along the way starts
perfectly, passes every other check, and disappoints the first person who tries to use it. That is
exactly what happened once already. Each build now creates a small model inside the packaged app,
publishes it, and requires the finished geometry back before the release may continue.
It insists on the right machinery, too. The desktop app has no copy of the tooling the server
version uses, so it has to fall back to its own — and every machine that assembles these builds
happens to have the server tooling installed. A check that accepted either would pass without ever
testing what a user actually runs, so this one names which is required and fails if the other
answers.
Proven both ways before being trusted: a deliberately broken build fails in one second and says
exactly what is missing, and a correctly built one passes and produces real geometry — because a
test that only ever fails could be failing for the wrong reason.
### The desktop app can show you the model it just built
Install the desktop app, create a project, upload an IFC and publish it, and everything worked
except the part people install it for: the 3D view stayed empty, permanently, and the app reported
success while doing it. Converting a model into something the viewer can draw was a step that only
existed on the server — the desktop download never contained it, so the app quietly skipped the
conversion and then had nothing to show. The web and hosted versions were never affected.
The conversion now happens inside the desktop app itself, using the model library it already ships
with. Publish a model and the 3D view fills in, offline, on a fresh install, with nothing else to
install.
It is a second implementation of the same file format, which is a real risk: two converters that
quietly disagree would be worse than one that plainly does nothing. So the two are run against the
same model on every build and checked against each other — same elements, same identifiers — and
both are opened with the viewer's own reader rather than with anything we wrote. Where they are
allowed to differ, they say so out loud instead of being made to match.
### An issue pinned on the model shows in the same place on the sheet and in 3D
An RFI, NCR or clash tied to the model can reach it two ways: somebody drops a marker at a point, or
the record names the element it is about. The plan sheet only ever read the second, and the 3D
viewer only ever read the first — so a marker dropped in the viewer appeared on no printed plan, and
an issue tied to a wall appeared nowhere in 3D. One issue, two surfaces, two different answers.
Both now read both, and the sheet is the full set.
A record placed at a point also draws immediately, with no model loaded — its position is where
somebody put it, so nothing has to be resolved first. Records tied to an element still need the
model, and the list continues to say how many it could not place rather than quietly returning
fewer.
**Underneath that was a bigger one: "no pin here" had never actually been stored as empty.** The
database recorded an absent marker as the *word* "null" rather than as nothing at all, so the query
that was supposed to fetch "only the records with a pin" matched every record in the register. The
pin overlay has a ceiling of 2,000; it was spending that ceiling on ordinary records, and the "N
pins on this project" count included them. Nothing was ever drawn in the wrong place — a second
check in the application caught them — but on a busy project the count was inflated and the overlay
could fill up with records that have no pin at all. Absent is now stored as absent, existing rows
are converted on upgrade, and the create form no longer writes an empty marker where it means none.
One more thing it fixes, found because a test started failing: the viewer's pin list was returning
**every issue on the project**, not the pinned ones. It has a ceiling of 2,000 and keeps the newest,
so on a project with more than that many issues the viewer could show almost no pins while having
plenty — the ceiling was being spent on issues that were never pins. It now returns exactly the
pinned ones. Note that this means the number it returns gets smaller, and correctly so: an issue
recorded against the whole model rather than a specific element has nowhere to be drawn, and is
still in the issue list.
**The gate written for this could not detect the bug it was written for, and only deliberately
re-breaking the code found that out.** The test passed with the fix removed, because the "null"
problem underneath made the broken and fixed versions behave identically.
### The Linux desktop app could not start
The published AppImage and .deb quit before they drew a window. The bundled server looked for the
web build by walking a fixed number of directories up from its own file — a number that is right in
a source checkout and wrong inside a packaged app, where the code is unpacked into a temporary
directory. On Linux the default temporary path is exactly one level too shallow, so the walk ran off
the top of the filesystem and the process died with `IndexError` before it ever reached the copy of
the web build packaged beside it.
The same binary starts normally when the temporary directory is deeper, which is why macOS and
Windows were unaffected: their temporary paths are twice as deep. It was found by downloading the
released build and running it, not by reading code.
**Fixing that one file would not have fixed the app.** Three modules counted directories the same
way, and two of them did it while being imported rather than inside a function — so the app would
have stopped one line later, in a different file, with the same kind of error. Every place that
looks for the program's own files now asks one shared question that recognises a packaged build and
says "there is no source checkout here", instead of counting directories and hoping.
Twelve more places were counting correctly enough not to crash and still pointing outside the
packaged app — two of them at directories that have never existed in the source either. Because each of them checks whether the directory is there before using it, none
of that ever surfaced: a wrong answer and a missing folder look identical, and something else quietly
answered instead. Those are corrected too, and a build-time check now refuses any new one.
Nothing in the release pipeline launches the application it builds, which is why every check was
green. That gap is now recorded as work of its own.
### The guard against a test writing into your database was itself half blind
An earlier change added a rule: a test that can create a schema must decide which database it creates
it in, rather than inheriting whatever the shell is carrying. That rule was real and the check for it
was real. What the check *looked at* was not — it decided whether a test could create a schema by
searching for a literal call to the table-creating function.
Starting the application creates the schema too, and a test that starts the application contains no
such call. **355 test files were in that blind spot, and four of them named no database at all.** One
of them, run the way its own instructions describe with the variable set, was measured creating 173
tables in the wrong database and reporting success — the same number that motivated the original
rule, in the part of the codebase the rule could not see.
The four are fixed, and the check now recognises a test that starts the application. The rule was
being enforced on 32 files; it is now enforced on 383.
A follow-up widened it again, after review pointed out that the check recognised only one of the
three ways a test can start the application. It now also recognises a client that is stored in a
variable before being started, and one imported under a different name. **No file in the suite was
being missed** — every file written the first way is also written the second — but a check that is
narrower than the rule it stands for is a gap with nobody in it yet, and the tree agreeing with it
today is exactly what makes the gap invisible.
### An issue tied to a wall shows on the model, whatever register it lives in
Binding a record to an element by GlobalId is how you say *this problem is here*. It worked for RFIs
and punch items. For a coordination issue, an NCR, a deficiency, an incident — and thirty-two other
registers — the record saved, reported that it was attached, and then appeared on no drawing and in
no viewer.
The pin resolver decided which registers hold pins from a list written by hand, and six of the eleven
names on it were not registers at all: they were the *words* for those ideas. Every clash imported
from Solibri or Navisworks becomes a "coordination issue", and the list said "clash" — so the clashes
the resolver exists to place were the ones it could not see.
Which records can carry a pin is now read from the registers themselves, from the same setting the
3D viewer has always used. The plan sheet and the model now show the same set, which is what the
sheet already claimed to do.
### Field-verification coverage says how much of the work is actually photographed
The coverage card showed how much of the model is verified and installed, and how many deviations
were logged. It did not say how much of that is backed by a photo — even though the server has been
computing exactly that, including the count of deviations with no photo at all, which it describes as
the handover number: a deviation with no photo is an assertion with nothing behind it.
The card now shows the share of tracked work that is photographed, names the denominator it is
measured against, and warns when deviations are unevidenced. Twelve deviations and twelve deviations
with nine unphotographed are different things to hand a client at closeout.
### A valuation that could not be computed no longer reads as a valuation of zero
When a project has no proforma and no comparables, none of the three appraisal approaches produces a
value. The valuation screen used to print **$0** as the opinion of value, in the same large type it
uses for a real one — so a property that could not be appraised looked like a property worth nothing.
It now shows a dash and says what is missing and what to add.
The same screen was quietly showing less than the valuation report built from the identical data. It
now names which of the three approaches reconciled into the value and says when the others were
excluded, shows the implied cap rate the comparables carry, shows the median that matches the basis in
force (a per-unit valuation used to display an empty "$/SF" row), shows the depreciation percentage
beside the amount deducted, and shows a missing cap rate as absent rather than as 0.00%.
It also names comparables the appraiser excluded. The server has been reporting those for some time,
precisely so a valuation whose sample shrank reads as a decision somebody made rather than as a
thinner market — but the web app had no field for them at all, so no one ever saw it.
### The pin count and the pins it describes are proven to come from one read
The pin overlay fetches a page of pins and the project-wide total in a single database read, so the
number beside the list always describes the list. That was implemented two releases ago and never
verified — the code could have been reverted to two separate reads and every one of the project's
tests would still have passed.
It is now checked directly, and the check fails if the single read is broken apart.
The concurrency scenario itself — another user filing an issue in the instant between two reads —
turned out not to be reproducible in the project's local test database, for reasons recorded next to
the test. Rather than write a check that would pass whether or not the code was correct, the test
states the limit plainly and verifies the property a different way.
### Pin totals on older projects stop being approximate
A pin total on a capped project was reported as an upper bound, and the printed plan sheet said so
with a `~`, because records saved before the previous release could hold an *empty but not absent*
value where a pin's location goes. Those records looked like pins to the fast database filter and
were then rejected, so they inflated the count without ever appearing.
A migration clears them. New records have not been able to carry these values since the previous
release, so this is a one-time cleanup of history rather than a patch over an ongoing problem — the
eleven other places that write a pin location were read and none of them can produce one.
Records at the project origin are explicitly preserved: a location of 0,0,0 is a place, not a
missing value, and treating it as empty would have deleted real pins.
### A printed plan sheet now says when its pin list was capped
The pin overlay was fixed to report a cap; the **printed sheet stayed silent**. A superintendent
walks the building with paper, so the half that reaches the person at risk was the half still
missing — a capped sheet printed a confident subset and looked complete.
A capped sheet now carries a note: *"Pin list capped at 2000 of ~2400 project pins — this sheet may
not show every issue on this level."* It is deliberately **qualitative about the sheet and
quantitative only about the project**. The cap is spent before pins are filtered to a storey, so how
many of the dropped pins belonged to *this* level cannot be known at either end, and printing a
per-sheet number would repeat the original defect in a new place. The `~` marks the total as an
upper bound while legacy records with empty-but-not-null pin fields still exist.
A sheet with **no** pins left after a cap still warns — that case is the blank overlay reaching
paper, and it is exactly when "this level looks clean" is most wrong.
The blocker recorded for this in the roadmap was false: it said the drawing engine takes positions
and labels, not notes, when the engine had been printing *"N pin(s) not located on this sheet"* all
along. Nobody had checked the claim against the code.
### The pin overlay went blank on busy projects, and nothing said so
Pins are how an issue gets a place in the building — an RFI on *that* wall, a punch item at *that*
door. The engine that resolves them took the project's 2,000 oldest issues and then discarded the
ones that are not pins.
Most issues are not pins. On a project with more than 2,000 of them — which is an ordinary
mid-size job, not a stress case — the 2,000 oldest were mostly ordinary un-pinned RFIs, so the
overlay came back with **nothing on it**, and a pin you placed this morning never showed up, because
the window never moved past those same 2,000 rows. It did not fade as a project got busier. It went
blank and stayed blank, on the model and on every plan sheet, with no message.
The test for "is this a pin" now runs in the database, before the limit, so the limit is spent on
actual pins. The eleven registers that can also carry pins used to get 2,000 each and then have the
combined list chopped back to 2,000. They now share one budget — which does not change which pins
you see, but does mean the ones that did not fit are counted rather than quietly discarded, so the
number beside them is the project's.
And the count that comes back is the project's, not the screen's. Where the overlay has more pins
than it can show it says how many there are — and where that number is an estimate rather than an
exact count, it says that too, instead of quietly rounding in its own favour.
Two things this does not yet do, said plainly rather than left to be discovered: a printed plan sheet
still cannot show that its pins were capped, and the total is an upper bound rather than an exact
figure on projects carrying older records.
### Three screens that showed you part of the answer and called it the answer
The sibling of the fix below, and the milder half. There the rows were wrong; here the rows are
right and the **number beside them** describes only what fitted on screen while being named for the
whole.
**The equipment schedule was the one that costs money.** It groups a model's procurable equipment
into RFQ line items, and it caps the list — reasonably, since a schedule is a document rather than a
data dump. But it reported the capped figure as the line count *and* summed the quantities of only
the lines that survived, so on a large model the RFQ you sent out was short in both the item list
and the numbers, in the direction that under-buys. It now says how many lines there are in total and
marks itself truncated; the lines are ordered largest-first, so a capped schedule drops the smallest
items rather than arbitrary ones.
**A topic's timeline** reported the number of events it was showing, not the number the topic has —
and the ones it drops are the oldest, which is where a decision's origin sits. **Viewer load
timings** reported the capped row count as the period's total, with every percentile beside it
computed from the newest slice while labelled as the period's.
Nothing was hidden deliberately in any of the three; each simply had one name doing duty for two
different numbers. All three now carry a total beside the count and say when they truncated.
### Two more screens that said "nothing here" while something was there
Fixing the agent trail (below) raised an obvious question nobody had asked: *how many other screens
answer a question by taking the most recent N of something and then narrowing?* Two more did, and
both were reporting an empty answer to a person who had every reason to trust it.
**Import history.** Finance ▸ where these numbers came from listed the newest hundred import
batches across the whole installation and *then* kept the ones belonging to the project you were
looking at. On a system where another project has been importing more recently, a project's import
history came back **completely empty while imports existed** — measured with one import on a quiet
project and 150 newer on a busy one. An empty lineage is the worst possible shape for that
particular answer: somebody reading it is trying to find out where a figure came from, and "nothing
was imported here" is a conclusion they will act on.
**The notification bell.** The feed took the project's two hundred most recent events and then
worked out which of them concerned *you*. On a busy project that window can be a single day, so a
person whose assigned work was older than it saw **an empty bell while holding open records
assigned to them** — measured going from one item to zero behind 250 events by other people. The
"my work" list directly beside it had always done this correctly; the two now work the same way.
Both now narrow first and cap second, so what you see is the newest N *of your things* rather than
your things among the newest N. A capped import history also now says how many batches there are in
total, so a hundred-of-a-hundred-and-fifty view stops reading as the whole file — the ledger's
**Import lineage** heading reads "newest 100 of 150" when there is more than it is showing.
### The agent trail said "nothing ran here" while runs existed
Every AI agent run against a project is recorded — which tool, through which named pack, by whom,
and whether it failed. That trail is the thing an organisation checks before letting an agent near
its projects, and the thing it reads first after something goes wrong.
It was quietly dropping runs. The console asked the database for the most recent activity and only
*then* narrowed it to the project you were looking at — so on a deployment where one project is
busier than the rest, a quieter project's console could report **no agent activity at all while
activity existed**. Measured: one run on a quiet project, a few hundred newer runs elsewhere, and
the quiet project's console showed nothing. The busy project lost older rows the same way, without
saying so.
For this screen that is not a blank, it is a wrong answer — "no agent touched this project" is
exactly the claim someone acts on. The narrowing now happens in the query, and a capped view says
how many runs there are in total beside how many it is showing.
**Settings ▸ Diagnostics ▸ Agent runs** is new: every agent run across every project, who ran it,
which pack it came through, and which failed — instead of opening each project's console in turn and
adding them up. It is admin-only because these are audit records, and the audit log is already
restricted to administrators. A run nobody could attribute is listed as unattributed rather than
hidden, since those are the ones worth looking at.
### Resourcing "by department" — the axis your firm defines, not one we invent
Portfolio resourcing already showed weekly demand per trade summed across every project, which is
the only place a crew promised to three jobs in the same week is visible at all. It could not group
those trades into the units a firm actually staffs by, because nothing in the schema is called a
department — and rather than invent a column, that was left open as a product question.
Checking how the industry's own tools model it settled it: the dominant AEC ERPs ship **no
department dimension either.** They give a configurable organisation breakdown whose levels and
labels the firm chooses. The axis is firm-defined, and the two readings the question was stuck
between turn out to be different *populations* rather than different labels — a general
contractor's departments are office functions (preconstruction, estimating, operations, safety)
whose people are salaried staff, not the field trades on a resource plan, while a design firm's
axis is discipline, which is already exactly what the field holds.
So no field was added. **Settings ▸ Portfolio resource grouping** takes a declaration
(`Structure:ironworker,concrete; MEP:electrician,plumber`) and the portfolio panel rolls the whole
book up by it, beneath the trades it is made of. A group cap flags a department over-committed
across projects where no single trade in it is. `Test` in that Settings group validates the
grouping on the spot instead of leaving you to discover a typo on someone else's screen.
The roll-up refuses two shapes outright, because a group total that is quietly wrong looks exactly
like one that is right: a trade claimed by two groups (the totals would then sum to more than the
book, and neither group is the right one to charge it to) and a group with no trades (which would
report a confident zero). It also names what it did *not* cover in both directions — demand no
group claimed, and a group naming a trade the book does not have — and a group whose trades are all
absent is shown as carrying no demand rather than zero demand.
### The dead-field sweep was asking a question its method cannot answer
An internal audit axis checks whether every field the API declares is actually *read* by the web
app — a field with no reader is a candidate for "a caveat that never reaches the screen", which is
how a number gets shown without the warning that qualifies it. The roadmap recorded that axis as
blocked on reading ~35 remaining fields one by one.
Re-deriving it found a different blocker. **The scan matches a bare identifier, not a type.** So
`SpatialNode.elevation` — referenced in dozens of files and by none of them as a `SpatialNode`,
because storeys, drawing grids and GIS overlays all happen to name a field `elevation` — is counted
as read while nothing renders it. Across the tree, **256 of the 719 fields counted as read, 35%, have
no reader that even names the declaring interface.** It fails hardest on `name`, `key`, `id`,
`title`, `status` and `count`, each referenced in 200+ files — which are the names most likely to be
displayed. A freeze list built on this would be guaranteed wrong rather than merely unverified.
The population is now re-run by a test rather than remembered, with the collision pinned as a fact:
identifier readers present, typed readers zero. Making the scan sound needs a type-aware pass over
the TypeScript symbol table, and that is now what the entry says. Two bugs in the derivation itself
turned up on the way — matching only top-level fields cannot see a *nested* one, and this axis's one
real defect was nested; and including the generated OpenAPI types reports 730 instead of 32.
Seven more fields were read while doing it and none was a defect, which is consistent with the
earlier rate. One is worth naming: a cost-variance figure that looked unread turns out to be a
documented refusal — comparing a realised overrun against an entered contingency is a claim about
what an underwriting asserts, not a unit conversion.
### Renaming a responsibility column moves its cells with it, or moves nothing
The RACI/DACI matrix stores two halves of one fact in two places: the **role columns** in a config
record, and the **letters keyed by those column names** on every activity row. The panel changed
them separately — one `PATCH` per row from the browser, then the config update — so a failure
anywhere in that sequence committed the rows it had already reached and abandoned the rest.
A half-applied **rename** was the bad one. Some rows carried `GC / PM`, some carried
`Project Manager`, and the column list still said `GC / PM` — one logical role under two names, one
of which was no longer a column, so its cells rendered as blank and counted toward nothing. That is
indistinguishable on screen from an orphaned column, and re-running the rename did not repair it:
the rows that had already moved no longer matched the name being renamed. Reproduced at five rows
splitting three-to-two.
The column edit and the cell migration are now one server-side transaction.
`PUT /projects/{pid}/responsibility/config` takes an optional `rename` map and a `drop` list
alongside `roles` and `mode`; the rename moves cells with their column, `drop` clears a removed
column's cells, and a mode change remaps the doer letter (R↔D) on every existing row. Nothing is
written unless all of it can be, and a request whose `roles`, `rename` and `drop` contradict each
other is refused with a 400 before anything is touched — renaming one live column onto another was
a silent merge that ate a column's cells. The panel's four row loops are now four single calls.
**A second, quieter instance of the same split, fixed with it.** Loading a starter template in the
other mode moved the matrix to DACI without migrating the rows already on it. Those rows kept `R`,
which is not a DACI letter — and the grid hides any letter invalid in the current mode, so they did
not render *wrong*, they rendered **empty**, and five activities reported having nobody Responsible.
**And a third, found by touching the transaction boundary rather than by looking for it.** Both
responsibility write routes recorded an audit entry *after* the engine had already committed, into a
session nothing commits again — so every change to the role columns, the one edit that can orphan an
entire matrix, left no trace in the audit log. `audit.record` only `db.add()`s, and `get_db` closes
without committing. The route now owns the only commit in the request, so the change and its trail
land together or not at all.
The class is a gate — `services/api/test_audit_commit.py`, which fails closed on any of the 106
`audit.record` call sites it cannot resolve. **Its first version asked the wrong question**, and
review caught it: "is there a `.commit()` at a later line in this function?" is not the same as "is
a commit reached on every path out of here". `run_clash_federated` was a live third instance the
whole time — its coordination branch records an audit row, and the only commit sat inside
`if create_topics and not coordinate`, a branch that is mutually exclusive with it. Later in the
file, never on the same path. The classifier now climbs out through enclosing blocks and counts only
commits that always execute, and it is proved against that branch shape before it may report.
### Feasibility sizes the building on the real parcel, not on its bounding rectangle
The "Generate from zoning" tab could describe a lot only as **lot width × lot depth**. For any parcel
that is not a rectangle — a corner lot, a flag lot, anything with a cut corner — that describes a box
*around* the lot, and a box's area is always larger. The error is not cosmetic: lot area sets max
GFA, which sets floors, which sets the unit count, which sets rent and the acquisition proforma's
IRR. Every one of those came out optimistic and none of them said so. On an L-shaped 1,600 m² lot
inside a 50 × 50 m box, the rectangle yields **5,000 m² of buildable GFA against the parcel's true
3,200** — 56% more building, on the number a deal gets priced from.
Three pieces existed and had never been connected. The parcel reader parsed a real GeoJSON/WKT
boundary and projected it to metres — then returned the area and threw the projected outline away.
The massing engine had always accepted a `lot_polygon` and offset it inward for a true buildable
footprint, and nothing could reach it: the request type had no such field. And the parcel reader
itself had no caller on any screen.
Now: paste a boundary (or load a `.geojson` / `.wkt` file) on the feasibility tab — one outer ring,
so a WKT `MULTIPOLYGON` is refused by name rather than sent to an endpoint that would reject it, and
a ring that encloses no area (repeated or collinear points) is refused with the reason rather than
priced as a lot of zero square metres. It reports the
parcel's area, the rectangle it replaces, and the percentage between them, and the width/depth boxes
become the parcel's own bounding extents, disabled — so it is clear which figure the building is
being sized on. The result summary says the same thing again beside the GFA. Nothing is substituted
silently; a user who prefers the rectangle can clear the boundary and still have it.
**A gate was wrong in the same direction.** `test_body_param_reach.py` decides whether the web app
sends a given request field by looking for it in the source — and only recognised object-literal
syntax (`key:`). A body assembled field by field (`p.dome_radius = …`, which is how this tab has
always sent its optionals) was invisible to it, so `dome_radius` sat on the "nothing sends this" list
while a control on that very tab had been sending it all along. Two false positives out of 27, in the
direction its own docstring calls the expensive one. Assignments now count; comparisons and arrow
parameters still do not, and a self-test pins the distinction.
### Parcel boundaries and T&M reports: four corrections from review
- **A line with no rate but an amount already entered was reported as excluded.** Only a line with a
*typed rate* was counted as being in the ticket total, but an amount entered directly never reaches
the rate-extension step at all — it survives untouched and is summed into the total regardless. The
same contradiction the previous entry fixed, one path over, and the fixture there could not see it
because it had a rate.
- **A parcel boundary with a repeated point produced a wrong buildable footprint.** Stripping the
duplicated closing point is not enough; an interior repeat survives it. The setback offset divides
by each edge's length and guards against division by zero, so a zero-length edge does not fail —
it leaves that corner un-inset while its neighbours move, and returns a plausible polygon with the
wrong area. Repeated points are now removed before anything is measured, and a boundary with fewer
than three distinct points is refused.
- **Clearing a parcel while it was still loading could bring it back.** Press "Use this parcel", then
Clear before the response arrives, and the late response republished the parcel just dismissed —
after which "Generate IFC model + apply" would save a model and a proforma for a lot the panel no
longer showed.
- **A WKT `MULTIPOLYGON` was accepted by the panel and rejected by the server.** Refused locally now,
by name, with the export that works. Supporting it properly is a design question rather than a
cleanup: the buildable footprint comes from offsetting one outer ring, and a lot split by a
right-of-way has no single ring to offset.
### T&M pricing: an unpriced line that was in the total after all
A follow-up review found the pricing report contradicting the figure printed beside it. An equipment
line with a rate the user typed, a blank amount, and a register rate quoted per Day/Week/Month was
listed as unpriced — *"8 hours are NOT in the figures above"* — directly beneath the $800 the engine
had just added to that line.
Both halves were doing something defensible. The register genuinely cannot price the line: converting
a weekly rate against hours needs a working day nobody stated. But `equipment_lines.rate` is declared
in `$/hr`, so when a rate is typed, `rate × hours` is the **line's own** arithmetic and is computed
like any other line — the register's unit is a fact about the register, not about the line, and
refusing the sum would drop a line the superintendent priced out of the ticket total.
So the sum was right and the sentence was wrong. A pricing run now says which of the two happened:
a quantity nothing could price still reads "NOT in the figures above", while one the line priced
itself reads "in the figures above at the rate you entered, not the register's". Overtime and idle
hours are unchanged — nothing multiplies those, here or anywhere, because a 1.5× premium is a
contract term rather than a default.
The fixture that missed it gave the line no rate of its own, so the old sentence was true by
accident. That is the fourth time in this item that the case distinguishing two behaviours was the
case nobody wrote.
### T&M pricing: five corrections to the first cut
Review of the merged TM-RATES change found the pricing engine wrong in five ways. All are fixed, and
none of them was reachable from the original tests:
- **Material never priced.** The line table's item name lives in `description`; the code looked for a
`material` column, which `material_lines` does not have. Labour and equipment happened to use the
same name on both sides, so one conflated field looked right in two cases out of three.
- **A hand-entered amount was overwritten.** 8 hours plus 2 overtime written up at $1,045 became
$760, and a lump-sum line with no hours became $0. A typed amount is evidence for the same reason
a typed rate is — it may carry a premium or an allowance both parties agreed. It is now reported
when it disagrees with rate × quantity, never replaced.
- **A register row with no rate priced the line at $0** and reported it as a success.
- **A rate quoted per Day or Week was extended against hours** — a $1,200/week crane read as $9,600
for an eight-hour day. It is now reported unpriced, for the same reason overtime is: converting
needs a working day nobody stated.
- **A closed (superseded) rate beat its live replacement** and was snapshotted onto the line.
Review of those fixes found three more, also fixed here: an unpriced line labelled its quantity with
the rate column (*"8 rate are NOT in the figures above"*), the rate comparison still rounded
half-even while the amount beside it rounded half-up — which can report a rate variance over a
figure that agrees to the cent — and one test sequenced its setup so a fixture failure would have
skipped the pricing rather than failing.
### T&M tickets price themselves from the project rate registers
**TM-RATES.** The eTicket register carries three line tables — labour, material, equipment — and the
project carries three rate registers to price them from. Nothing connected the two: a superintendent
typed the rate onto every line by hand, which on a T&M ticket is the number that gets argued about
when the change order is negotiated. **Price from rate tables** on the ticket now fills each blank
rate from `labor_rate` / `material_rate` / `equipment_rate` and recomputes the amounts, and the
totals follow from the lines.
It answers for what it did NOT price, which is the point:
- a rate somebody **typed** is kept as typed, and its disagreement with the register is shown with
both numbers — a T&M rate that diverges from the contract rate table is what a GC needs to see
before signing, not something to correct silently;
- a trade or material no register knows is left untouched and named, with the register that would
fix it;
- **overtime and idle hours come back unpriced.** Neither register holds an OT or idle rate, and a
1.5x nobody agreed to is a contract term, not a default.
Re-pricing after a register changes does not re-rate work already done — the rate is a snapshot —
but it does report that the register has moved since.
**The route existed since June and nothing called it, and worse than that.** It wrote the totals
straight onto the record; `apply_table_totals` — six weeks younger — recomputes every total from its
table inside the same write, so the priced figure was replaced by the sum of the untouched `amount`
cells while the response returned the correct number. Measured: `200 {"labor_total": 760.0}` over a
record holding `0.0`. It also wrote `tm_lines`, a field the module does not declare and nothing
reads. `test_cost.py` asserted the stored total and passed, because its fixture ticket had no line
tables at all — the one shape in which the write-back survives.
Found by a new derivation: **which required request parameter no web client sends** — the mirror of the
response-field sweep. `services/api/test_body_param_reach.py` now gates it, and reaches inside
`test_route_reachability.py`'s documented blind spot: that gate skips any path segment under five
characters, and this route's leaf is `tm`.
### the review-cycles card says how many rounds it could not measure
"Whose court did the time sit in" is the question that card exists to settle, and it was answering
over fewer rounds than the register holds. A review round with no submitted date cannot start a
clock, so it is left out of the split entirely — correctly — but the round count printed beside the
days was the *scored* count, with nothing saying so. Two dated rounds out of five recorded read as a
complete answer.
It now says it: **"3 more round(s) carry no submitted date and are NOT in the figures above — the
split covers 2 of 5 recorded rounds"**, with a nudge to add the dates. The card already handled the
sibling case, rounds still open, with exactly this care; this is the harder one, because an undated
round is not counted at all.
The days figures also now name their basis — **calendar days**, because a statutory review clock
does not pause for a weekend, while durations elsewhere in this system are working days. The engine
had always returned that axis and nothing printed it.
### the responsibility matrix stops telling you it is complete when it is not
Load a second starter template into a RACI matrix that already has rows and the columns changed
under them. The earlier rows survived — the dialog promises they will — but every letter on them was
now keyed to a role column that no longer existed, so those rows rendered as blank lines. And the
validation banner still said **"Every activity has exactly one Accountable and at least one
Responsible"**, because it counted assignments the grid had no cell to draw.
Validity is now counted over the columns you can actually see, so a row whose letters have been
orphaned reads as what it is: no Accountable, no Responsible. The letters are not lost — the banner
names the columns they are stranded on, and offers to **restore those columns** (nothing was
deleted, so bringing them back makes the assignments visible again) or to **clear them** if they are
genuinely finished with.
Applying a template no longer replaces your columns either: when the matrix already has rows, the
template's roles are merged in after yours instead of taking their place. If that would push past
the sixteen-column limit the whole thing is refused before anything is written, rather than
truncating and orphaning the very rows it was about to create.
### you can now see which property values this element overrides — and put them back
The viewer has always let you set a property on a selected element. It has never shown you that a
value you were looking at was **this element's own**, written over what its type says — and once you
had done it, there was no way back. Both halves are here now.
Properties that override their type are marked, and the panel prints the value the type carries
underneath, because a badge saying "overridden" without saying *what it replaced* is half an answer.
Next to it is **Reset to type**, which drops this element's value so the type's shows through again.
It appears only on properties that actually override something, since that is exactly when it can
work.
If the panel cannot find out which values are overridden — a request that fails, an index still
rebuilding — it **says so** rather than showing an unmarked list. An unmarked list would look
identical to "nothing here is overridden", which is a different and much more reassuring claim than
the one we can actually make.
### a project can change its units, and move a far-flung model back to the origin
Two things a modelling tool is expected to do had no control anywhere in the app. Both engines were
built, tested, and reachable through the edit API the whole time; the coverage matrix listed them
under *"a capability no user can reach"*, which is that file's own words for a defect.
**Project units.** A project can now be converted between metres, millimetres and centimetres. The
building does not move or change size — only the numbers do — and the panel names the unit the
project reads in *now* before offering to change it. That last part is why th