C4/D11 follow-up - the integration review's seven confirmed findings
An adversarial review (four lenses, every finding independently verified by two skeptics told to refute it) ran over2a5f6b3and8cf8c0f. Seven findings survived; all seven are fixed here. Against the C4 fix: - help.js: the nav hover was renamed onto its own surface token, keeping a no-op T9.9 had introduced (two different grays had been mapped to one name). Hover is now --cds-layer-hover, the token that exists for exactly this. - wp-creation-app.js: the drawer's critical CSS pre-painted --cds-layer-accent while the stylesheet paints --wp-nav-bg; now both paint --wp-nav-bg. Against D11: - wp-sections.js: the Assets toggle note still described the pre-D11 card ('Asset tags and controls.dev links') with a rationale the picker inverts. - runAssetSearch: the result cap counted contains-matches before the exact and prefix tiers finished, so 500 alphabetically-early substring hits could evict the exact match - and Enter then added the wrong asset, ID-locked. The cap now bounds each tier; the scan always sees the whole catalog. - addCatalogAsset: the one mutation in the section with no announced outcome was the successful pick. It now toasts (role=status), matching every sibling path (C1). - assets_db.py: failures are remembered for FAIL_CACHE_SECONDS (default 30s) and a stale catalog is served over an error, so a Micron outage costs one CONNECT_TIMEOUT per window instead of one per page load stacking up in the shared sync threadpool until login itself stalls. - assets_db.py: MICRON_ASSETS_CACHE_SECONDS='5m' no longer crashes the boot - a malformed knob on an OPTIONAL feature degrades to its default, loudly. assets_check grows four regressions for these (27 -> 31): per-tier cap against 600 decoys, the announced pick, boot with a malformed knob, and the stable cached 503. Battery: assets_check 31/31, color_check 5/5, sections_check ALL PASS. Items: C4, D11. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -311,7 +311,7 @@ python tests/color_check.py # C4 - zero literals outside theme-light
|
||||
The August 20 integration adds:
|
||||
|
||||
```bash
|
||||
python tests/assets_check.py # D11 - Micron picker: read-only, degrades 27 checks
|
||||
python tests/assets_check.py # D11 - Micron picker: read-only, degrades 31 checks
|
||||
```
|
||||
|
||||
**Three probes were re-pointed at `T7.1`.** `sections_check.py` 5b drove the live
|
||||
|
||||
@@ -145,7 +145,7 @@
|
||||
.ui-help-nav{ width:230px; flex:none; border-right:1px solid var(--cds-border-subtle); overflow:auto; padding:10px 8px; background:var(--cds-layer-accent); }
|
||||
.ui-help-nav a{ display:block; padding:7px 10px; border-radius:0; color:var(--cds-text-primary); text-decoration:none; font-size:13px;
|
||||
cursor:pointer; margin-bottom:1px; }
|
||||
.ui-help-nav a:hover{ background:var(--cds-layer-accent); }
|
||||
.ui-help-nav a:hover{ background:var(--cds-layer-hover); }
|
||||
.ui-help-nav a.active{ background:var(--cds-highlight); color:var(--cds-link-primary-hover); font-weight:600; }
|
||||
.ui-help-nav a.nohit{ display:none; }
|
||||
.ui-help-content{ flex:1; overflow:auto; padding:22px 28px; scroll-behavior:smooth; }
|
||||
|
||||
@@ -1444,12 +1444,16 @@ function runAssetSearch(q){
|
||||
assetResults = []; renderAssetResults(); openAssetResults(false); return;
|
||||
}
|
||||
const exact=[], prefix=[], other=[];
|
||||
// The cap bounds each TIER, never the scan: breaking on a combined count let
|
||||
// 500 alphabetically-early contains-matches evict an exact or prefix match
|
||||
// that sorted after them - and Enter then added the wrong asset. The scan is
|
||||
// in-memory and cheap; "the box scrolls, not the search" means the search
|
||||
// sees everything.
|
||||
for(const tag of assetCatalog){
|
||||
const t = tag.toLowerCase();
|
||||
if(t === q) exact.push(tag);
|
||||
else if(t.startsWith(q)) prefix.push(tag);
|
||||
else if(t.includes(q)) other.push(tag);
|
||||
if(exact.length + prefix.length + other.length >= ASSET_RESULT_MAX) break;
|
||||
else if(t.startsWith(q)){ if(prefix.length < ASSET_RESULT_MAX) prefix.push(tag); }
|
||||
else if(t.includes(q)){ if(other.length < ASSET_RESULT_MAX) other.push(tag); }
|
||||
}
|
||||
assetResults = exact.concat(prefix, other).slice(0, ASSET_RESULT_MAX);
|
||||
renderAssetResults();
|
||||
@@ -1483,6 +1487,7 @@ function addCatalogAsset(ix){
|
||||
if(assetAlreadyAdded(tag)){ toast('That asset is already on this package'); return; }
|
||||
pkgAssets.push({ tag: tag, desc: '', link: '', source: 'catalog' });
|
||||
buildAssets();
|
||||
toast('Added ' + tag); // announced (role=status) - a keyboard pick is otherwise silent
|
||||
// Mark just this row instead of re-rendering the list: the results stay open
|
||||
// for the next pick, the scroll position holds, and the clicked element is
|
||||
// never detached mid-click (see the composedPath note in initAssetPicker).
|
||||
@@ -2929,7 +2934,7 @@ const WP_NAV_CRITICAL_CSS = `
|
||||
body{--nav-w:288px;}
|
||||
body.wp-nav-collapsed{--nav-w:56px;}
|
||||
.wp-nav{position:fixed;top:var(--rail-top,48px);left:0;bottom:0;width:var(--nav-w);
|
||||
z-index:120;display:flex;flex-direction:column;overflow:hidden;background:var(--cds-layer-accent);
|
||||
z-index:120;display:flex;flex-direction:column;overflow:hidden;background:var(--wp-nav-bg);
|
||||
border-right:1px solid var(--cds-border-subtle);}
|
||||
.wp-nav-list{flex:1 1 auto;overflow-y:auto;overflow-x:hidden;}
|
||||
.wp-nav-item,.wp-nav-link{display:flex;align-items:center;gap:11px;width:100%;
|
||||
|
||||
@@ -32,7 +32,7 @@
|
||||
{ id: 'scope', label: 'Scope of Work',
|
||||
note: 'The ordered steps the crew performs, and the labour estimate.' },
|
||||
{ id: 'assets', label: 'Assets',
|
||||
note: 'Asset tags and controls.dev links. Off for Micron EUV — the content duplicates the database Clinton’s team maintains (CR-016).' },
|
||||
note: 'Asset IDs picked read-only from the Micron DB (D11), with manual entry for anything not listed. Off for Micron EUV — the customer’s own database stays the source of truth; this section only references it (CR-016).' },
|
||||
{ id: 'materials', label: 'Materials',
|
||||
note: 'The bill of materials that feeds kitting.' },
|
||||
{ id: 'kitting', label: 'Kitting',
|
||||
|
||||
@@ -57,8 +57,26 @@ ASSET_QUERY = """
|
||||
ORDER BY a.AssetID
|
||||
"""
|
||||
|
||||
def _env_int(name: str, default: int) -> int:
|
||||
"""A malformed tuning knob degrades to its default; it must never keep the
|
||||
suite from booting. app.py imports this module unconditionally, so a bare
|
||||
int() here would turn "300s" in someone's .env into a crash-looping API -
|
||||
the total-outage switch an OPTIONAL feature is not allowed to own."""
|
||||
raw = os.getenv(name, "")
|
||||
try:
|
||||
return int(raw.strip()) if raw.strip() else default
|
||||
except ValueError:
|
||||
log.warning("%s=%r is not an integer; using the default %d.", name, raw, default)
|
||||
return default
|
||||
|
||||
|
||||
# How long a fetched catalog is reused before the next page load re-queries.
|
||||
CACHE_SECONDS = int(os.getenv("MICRON_ASSETS_CACHE_SECONDS", "300")) # 5 min
|
||||
CACHE_SECONDS = _env_int("MICRON_ASSETS_CACHE_SECONDS", 300) # 5 min
|
||||
# How long a FAILURE is remembered before the next request retries the source.
|
||||
# Without this, every page load during a Micron outage spends CONNECT_TIMEOUT
|
||||
# seconds inside a worker thread; enough concurrent loads exhaust the app's
|
||||
# shared sync threadpool and take unrelated endpoints down with the picker.
|
||||
FAIL_CACHE_SECONDS = _env_int("MICRON_ASSETS_FAIL_CACHE_SECONDS", 30)
|
||||
CONNECT_TIMEOUT = 8
|
||||
|
||||
|
||||
@@ -149,37 +167,66 @@ def _get_engine():
|
||||
# would be one full-table query per person. Held per worker process.
|
||||
_cache: list[dict] | None = None
|
||||
_cached_at = 0.0
|
||||
_error: str | None = None # negative cache: the last failure's user-safe text
|
||||
_error_at = 0.0
|
||||
_cache_lock = threading.Lock()
|
||||
|
||||
|
||||
def load(force: bool = False) -> list[dict]:
|
||||
"""Return the whole catalog as [{'tag': …}, …]. Never writes."""
|
||||
global _cache, _cached_at
|
||||
"""Return the whole catalog as [{'tag': …}, …]. Never writes.
|
||||
|
||||
Failures are handled in two tiers so a Micron outage stays the picker's
|
||||
problem and never the suite's (the module contract above):
|
||||
* a previously fetched catalog is served STALE - it is slow-moving
|
||||
reference data, and old-but-real beats an error;
|
||||
* with nothing to serve, the failure itself is cached for
|
||||
FAIL_CACHE_SECONDS, so an outage costs one CONNECT_TIMEOUT per window
|
||||
instead of one per page load stacking up in the shared threadpool."""
|
||||
global _cache, _cached_at, _error, _error_at
|
||||
with _cache_lock:
|
||||
if _cache is not None and not force and (time.monotonic() - _cached_at) < CACHE_SECONDS:
|
||||
return _cache
|
||||
if (_error is not None and not force
|
||||
and (time.monotonic() - _error_at) < FAIL_CACHE_SECONDS
|
||||
and _cache is None):
|
||||
raise AssetSourceError(_error)
|
||||
|
||||
engine = _get_engine()
|
||||
try:
|
||||
engine = _get_engine()
|
||||
with engine.connect() as conn:
|
||||
result = conn.execute(text(ASSET_QUERY)).mappings().all()
|
||||
except AssetSourceError as exc:
|
||||
# _get_engine already logged and sanitised; remember or stale-serve.
|
||||
with _cache_lock:
|
||||
if _cache is not None:
|
||||
log.warning("Micron asset catalog unavailable; serving the cached "
|
||||
"catalog (%d rows).", len(_cache))
|
||||
return _cache
|
||||
_error, _error_at = str(exc), time.monotonic()
|
||||
raise
|
||||
except SQLAlchemyError as exc:
|
||||
# The driver's message is NOT propagated. AssetSourceError text reaches the
|
||||
# browser, and connection errors quote the host, the login, and — when the
|
||||
# URL is malformed — fragments of the password. Operators get the detail
|
||||
# from the API log, where it belongs; users get a message they can act on.
|
||||
log.error("Micron asset catalog query failed: %s", exc)
|
||||
raise AssetSourceError(
|
||||
"The Micron DB could not be read. Check that the host is "
|
||||
"reachable, that the login has SELECT on the asset table, and that "
|
||||
"ASSET_QUERY matches the real schema — the API log has the driver error."
|
||||
) from exc
|
||||
msg = ("The Micron DB could not be read. Check that the host is "
|
||||
"reachable, that the login has SELECT on the asset table, and that "
|
||||
"ASSET_QUERY matches the real schema — the API log has the driver error.")
|
||||
with _cache_lock:
|
||||
if _cache is not None:
|
||||
log.warning("Micron asset catalog unavailable; serving the cached "
|
||||
"catalog (%d rows).", len(_cache))
|
||||
return _cache
|
||||
_error, _error_at = msg, time.monotonic()
|
||||
raise AssetSourceError(msg) from exc
|
||||
|
||||
# Drop rows with no identifier — an asset with no tag is not selectable and
|
||||
# would render as a blank line in the picker.
|
||||
rows = [{"tag": str(r["tag"])} for r in result if r.get("tag") not in (None, "")]
|
||||
with _cache_lock:
|
||||
_cache, _cached_at = rows, time.monotonic()
|
||||
_error = None
|
||||
return rows
|
||||
|
||||
|
||||
|
||||
@@ -149,6 +149,10 @@ def main():
|
||||
page.eval("(() => { const r=[...document.querySelectorAll('#asset-results button.asset-result')];"
|
||||
" return r.length===2 && r.every(b=>!b.disabled); })()"))
|
||||
page.eval("addCatalogAsset(0)")
|
||||
chk("the pick is announced (role=status toast) - a keyboard pick is otherwise silent",
|
||||
page.eval("(() => { const t=document.getElementById('toast');"
|
||||
" return !!t && t.getAttribute('role')==='status'"
|
||||
" && /Added PUMP-01/.test(t.textContent); })()"))
|
||||
chk("picking adds a catalog row: locked ID (no input), badge, source:'catalog'",
|
||||
page.eval("pkgAssets.length") == 1
|
||||
and page.eval("pkgAssets[0].source") == "catalog"
|
||||
@@ -195,6 +199,20 @@ def main():
|
||||
page.eval("(() => { const b=document.querySelector('#asset-results button');"
|
||||
" return !b.disabled && /add/.test(b.textContent); })()"))
|
||||
|
||||
# The tier-cap regression (review finding, fixed same day): 600
|
||||
# alphabetically-early contains-matches must not evict a prefix match
|
||||
# that sorts after every one of them. Before the fix the scan broke at
|
||||
# a COMBINED 500 and Enter added the wrong asset, ID-locked.
|
||||
got = json.loads(page.eval(
|
||||
"(() => { const c=[];"
|
||||
" for(let i=0;i<600;i++) c.push('A'+String(i).padStart(4,'0')+'-PMP-10');"
|
||||
" c.push('PMP-10-EXTRA');"
|
||||
" assetCatalog=c; assetCatalogIndex=new Map(c.map(t=>[t.toLowerCase(),t]));"
|
||||
" assetCatalogState='ready'; runAssetSearch('pmp-10');"
|
||||
" return JSON.stringify([assetResults[0], assetResults.length]); })()"))
|
||||
chk("a prefix match outranks 600 earlier contains-matches (cap is per tier)",
|
||||
got[0] == "PMP-10-EXTRA" and got[1] == 500, ascii_(got))
|
||||
|
||||
# ── 5. configured-but-broken: a 503 that does not leak ────────────────
|
||||
print("\n5. broken is not a leak")
|
||||
browser.close()
|
||||
@@ -202,10 +220,15 @@ def main():
|
||||
server.terminate()
|
||||
server.wait(timeout=10)
|
||||
os.environ["MICRON_DB_URL"] = "mssql+pymssql://user:S3CRETpw@127.0.0.1:1/MicronDB"
|
||||
# A malformed tuning knob must degrade, not crash the boot (review
|
||||
# finding: int() at import time made "5m" a total-outage switch).
|
||||
os.environ["MICRON_ASSETS_CACHE_SECONDS"] = "5m"
|
||||
try:
|
||||
port2 = cdp.free_port()
|
||||
base2 = "http://127.0.0.1:%d" % port2
|
||||
server = start_server(port2, db_path)
|
||||
chk("the suite boots with MICRON_ASSETS_CACHE_SECONDS='5m' (degrades, no crash)",
|
||||
server is not None and server.poll() is None)
|
||||
st, body = api(base2, "/api/assets", tok["root"])
|
||||
detail = (body or {}).get("detail") or ""
|
||||
chk("a configured-but-unusable catalog answers 503, not 500",
|
||||
@@ -213,8 +236,16 @@ def main():
|
||||
chk("...and the message never echoes the URL, login or password",
|
||||
"S3CRETpw" not in detail and "user" not in detail
|
||||
and "127.0.0.1:1" not in detail, ascii_(detail))
|
||||
# The negative cache (review finding): the second request inside the
|
||||
# failure window must answer from the remembered error - same 503,
|
||||
# same safe text - not stack another connect attempt in a worker.
|
||||
st2, body2 = api(base2, "/api/assets", tok["root"])
|
||||
chk("...and a second request answers the cached failure, stable and safe",
|
||||
st2 == 503 and (body2 or {}).get("detail") == detail,
|
||||
(st2, ascii_(body2)))
|
||||
finally:
|
||||
del os.environ["MICRON_DB_URL"]
|
||||
del os.environ["MICRON_ASSETS_CACHE_SECONDS"]
|
||||
finally:
|
||||
if browser:
|
||||
browser.close()
|
||||
|
||||
Reference in New Issue
Block a user