Check the things the gates were reporting green without checking
Four gates that were not gating. NO MIGRATION COLUMN NAME HAD EVER BEEN CHECKED. The naming gate matched db.Column attribute assignments only, so every column a migration creates has bypassed the lowercase-concatenated rule since the gate was written. It now reads migrations too, in the shapes they are actually written: sa.Column in create_table and add_column, alter_column's new_column_name, the batch equivalents, the keyword form, and a literal with a leading capital or a string prefix. Run against the tree it finds nothing, which is now a fact rather than an artefact of the pattern. THE ADR-015 SITE-LITERAL SCAN DID NOT REACH THE C# AGENT, twice over: agent/ was not in the directory list, and a file-level --exclude alongside the --include list changes which files grep matches at all, so the .cs include was not doing what it looked like it was doing. A hostname or base URL compiled into the agent would have shipped to another site with no CI signal. NO PLUGIN MIGRATION WAS CHECKED FOR TOUCHING SOMEBODY ELSE'S TABLES. ADR-008 says plugin chains own plugin schema and the core chain is reserved for core tables; the endpoints chain broke that, added and then dropped columns on the core assets table, and nothing would have stopped the next one. The scan walks every plugin chain, resolves each DDL op to its table, and fails when that table belongs to core or to another plugin - including through op.execute and through a bind, which is this repo's dominant idiom and the shape a check keyed on the receiver name would have missed entirely. Statements are classified one at a time, so a leading DML verb cannot hide trailing DDL. The two historical crossings are allowlisted by plugin, file, op, table AND column, so the shipped history stays green and anything new goes red. AND THE GATE RUNNER ITSELF PASSED WITHOUT RUNNING ANYTHING. In a checkout with no venv, `python` is often not on PATH; the pytest line ends in `|| true`, so the run died 127, the report was one command-not-found line, every gate matched nothing, and it still printed "All gates ran." and exited 0. That exit code is what everything else trusts. It now refuses to run without an interpreter that can import pytest. Recorded rather than fixed, with the reasoning in both files: the publishability scrub keys on docs/*.md, so prose that lives only in docs/wiki-drafts/ does not turn it on. Whether those drafts are internal or headed for publication is a question about what they are for, not a bug to close by rewriting them.
This commit is contained in:
cproudlock
@@ -6,7 +6,7 @@
|
||||
#
|
||||
# tests/test_api_inventory_parity.py skipif docs/api-inventory.json missing
|
||||
# tests/test_openapi_spec.py skipif docs/openapi.json missing
|
||||
# tests/test_docs_publishable.py skipif docs/ missing
|
||||
# tests/test_docs_publishable.py skipif docs/*.md missing
|
||||
#
|
||||
# That is correct in a published checkout, where docs/ becomes the wiki and
|
||||
# there is genuinely nothing to check. It was silently wrong on GitHub, which
|
||||
@@ -30,21 +30,33 @@ cd "$(dirname "$0")/.."
|
||||
# always a fault.
|
||||
GATES_EVERYWHERE=(
|
||||
"tests/test_openapi_spec.py"
|
||||
"tests/test_api_inventory_parity.py"
|
||||
"tests/test_core/test_public_read_surface.py"
|
||||
"tests/test_core/test_authz.py"
|
||||
"tests/test_plugin_migrations.py"
|
||||
)
|
||||
|
||||
# Gates that need files the publication export deliberately leaves behind -
|
||||
# docs/ becomes the wiki, so a published checkout genuinely has nothing to check
|
||||
# and skipping is correct there. In THIS repo, skipping means something broke.
|
||||
# docs/*.md becomes the wiki, so a published checkout genuinely has nothing to
|
||||
# check and skipping is correct there. In THIS repo, skipping means something
|
||||
# broke.
|
||||
#
|
||||
# The inventory parity gate used to be here, and that was the hole it exists to
|
||||
# close. docs/api-inventory.json is not wiki prose: it is a generated artifact,
|
||||
# tracked exactly like docs/openapi.json beside it, because the app publishes the
|
||||
# spec built from it. Keying the gate to prose meant a checkout with no docs/*.md
|
||||
# - which is every fresh clone and every worktree - called itself published and
|
||||
# ran the route check nowhere. That is where 24 undocumented routes came in.
|
||||
GATES_INTERNAL_ONLY=(
|
||||
"tests/test_api_inventory_parity.py"
|
||||
"tests/test_docs_publishable.py"
|
||||
)
|
||||
|
||||
# A published checkout is one with no docs/ prose. openapi.json alone does not
|
||||
# make it internal: that file ships because the app serves it at runtime.
|
||||
#
|
||||
# NOTE: docs/wiki-drafts/*.md does NOT count here, and that is a standing
|
||||
# question rather than an oversight - see the comment on the skipif in
|
||||
# tests/test_docs_publishable.py. Keep the two definitions identical.
|
||||
if compgen -G "docs/*.md" > /dev/null; then
|
||||
GATES=("${GATES_EVERYWHERE[@]}" "${GATES_INTERNAL_ONLY[@]}")
|
||||
echo "==> Internal checkout: every gate must run"
|
||||
@@ -56,11 +68,26 @@ fi
|
||||
PYTHON="${PYTHON:-python}"
|
||||
[ -x venv/bin/python ] && PYTHON=venv/bin/python
|
||||
|
||||
# REFUSE TO RUN WITHOUT A REAL INTERPRETER. In a git worktree there is no venv,
|
||||
# `python` is often not on PATH at all, and the pytest line below ends in
|
||||
# `|| true` - so the run died 127, the report was a single "command not found"
|
||||
# line, every gate matched nothing, and the script still printed "All gates ran."
|
||||
# and exited 0. A gate-checker that passes when it ran nothing is worse than no
|
||||
# gate-checker, because its exit code is what everything else trusts.
|
||||
if [ -z "${1:-}" ] && ! "$PYTHON" -c 'import pytest' >/dev/null 2>&1; then
|
||||
echo "FAIL: '$PYTHON' cannot import pytest, so no gate can run here." >&2
|
||||
echo " Set PYTHON=/path/to/python (a venv that has pytest) and re-run." >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
report="${1:-}"
|
||||
if [ -z "$report" ]; then
|
||||
report="$(mktemp)"
|
||||
# -rs lists every skip with its reason, which is the thing being checked.
|
||||
"$PYTHON" -m pytest "${GATES[@]}" -q -rs > "$report" 2>&1 || true
|
||||
# -rf and -rE add the "FAILED <nodeid>" and "ERROR <nodeid>" lines, which -rs
|
||||
# alone does NOT emit: this script grepped for them anyway, so on a real
|
||||
# failure it printed its header and then nothing at all.
|
||||
"$PYTHON" -m pytest "${GATES[@]}" -q -rsfE > "$report" 2>&1 || true
|
||||
fi
|
||||
|
||||
echo "==> Checking that every gate ran"
|
||||
@@ -82,10 +109,34 @@ for gate in "${GATES[@]}"; do
|
||||
echo " ran $gate"
|
||||
done
|
||||
|
||||
if grep -qE "^[0-9]+ failed|error" "$report"; then
|
||||
# Name what failed, from whatever the report actually contains. This script can
|
||||
# be handed a report somebody else produced, and that one may have been run
|
||||
# without -rf, so the "FAILED <nodeid>" lines are not guaranteed to be there.
|
||||
# The FAILURES section underlines are: pytest prints "____ test_name ____" above
|
||||
# every traceback whether the short summary was asked for or not.
|
||||
list_failures() {
|
||||
if grep -qE '^(FAILED|ERROR) ' "$1"; then
|
||||
grep -E '^(FAILED|ERROR) ' "$1"
|
||||
return
|
||||
fi
|
||||
if grep -qE '^_{3,} .+ _{3,}$' "$1"; then
|
||||
sed -n 's/^_\{3,\} \(.*[^_]\) _\{3,\}$/\1/p' "$1"
|
||||
return
|
||||
fi
|
||||
# Nothing named. Hand over the count line rather than an empty list, so the
|
||||
# reader knows to open the report instead of believing there is nothing here.
|
||||
grep -E '[0-9]+ (failed|error)' "$1" | tail -1
|
||||
echo "(no test names in this report; run without an argument to get them)"
|
||||
}
|
||||
|
||||
# The old condition was "^[0-9]+ failed" or the bare word "error" anywhere,
|
||||
# which matches a test name, a warning or a docstring. Anchor on the shapes
|
||||
# pytest actually prints: a short-summary line, a count in the tally, or an
|
||||
# interrupted collection.
|
||||
if grep -qE '^(FAILED|ERROR) |(^|, )[0-9]+ (failed|error)|^!+ Interrupted' "$report"; then
|
||||
echo
|
||||
echo "Gate tests FAILED (not merely skipped):"
|
||||
grep -E "^FAILED|^ERROR" "$report" | sed 's/^/ /'
|
||||
list_failures "$report" | sed 's/^/ /'
|
||||
failed=1
|
||||
fi
|
||||
|
||||
|
||||
@@ -78,10 +78,18 @@ fi
|
||||
# DO NOT ADD --exclude=<file> TO THE ARRAY ABOVE.
|
||||
#
|
||||
# In this grep build a single file-level --exclude silently CANCELS EVERY
|
||||
# --include filter on the same command. Bisected: with only --exclude-dir flags
|
||||
# a shorthand scan restricted to --include='*.py' returns 0 markdown hits; add
|
||||
# any one --exclude and the same scan returns 1142. Directory excludes are
|
||||
# unaffected - it is specifically the file form.
|
||||
# --include filter that comes AFTER it on the same command. Bisected: with only
|
||||
# --exclude-dir flags a shorthand scan restricted to --include='*.py' returns 0
|
||||
# markdown hits; add any one --exclude ahead of the includes and the same scan
|
||||
# returns 1142. Re-proved on GNU grep 3.11 with six files of one literal:
|
||||
# --include='*.py' --include='*.cs' -> 2 hits, as written
|
||||
# --exclude=README.md --include='*.py' --include='*.cs' -> all 6 hits
|
||||
# --include='*.py' --include='*.cs' --exclude=README.md -> 2 hits
|
||||
# Directory excludes are unaffected in any position - it is specifically the
|
||||
# file form. DO NOT rely on writing the exclude last: the position rule is not
|
||||
# documented behaviour, the array above is expanded ahead of every include list
|
||||
# in this file, and a later edit that reorders the flags would silently widen
|
||||
# the scan again with nothing failing. Post-filter instead.
|
||||
#
|
||||
# This is not theoretical, it was live: --exclude=findings.sarif and
|
||||
# --exclude=report.md sat in this array and had been quietly disabling the
|
||||
@@ -246,6 +254,107 @@ if [ -n "$SNAKE_COLS" ]; then
|
||||
VIOLATIONS=$((VIOLATIONS + 1))
|
||||
fi
|
||||
|
||||
# The check above reads MODEL attributes, and a model attribute is not where a
|
||||
# column is born - a migration is. migrations/versions/ is filtered out of
|
||||
# GIT_FILES_FILTER and out of EXCLUDES for every other check in this file, so
|
||||
# from the day this gate was written not one created column name was ever
|
||||
# looked at. A model whose attribute reads fine still ships whatever the
|
||||
# migration wrote, because the migration is what the database got.
|
||||
#
|
||||
# CREATION SITES ONLY: sa.Column(<name>) and an alter_column rename. drop_column
|
||||
# and an ordinary alter_column name a column that already exists, so a bad name
|
||||
# there was already wrong where it was created and flagging it twice only makes
|
||||
# the same rename look like two findings.
|
||||
#
|
||||
# awk, not grep, because sa.Column( is routinely followed by a newline before
|
||||
# the name (op.add_column('applications', sa.Column(\n 'isrequired', ...)) and a
|
||||
# line-at-a-time grep sees neither half: the first line has no name and the
|
||||
# second has no sa.Column. One line of lookahead joins them - that shape is 9
|
||||
# of the ~1180 call sites here, so dropping it is not acceptable.
|
||||
#
|
||||
# KNOWN LIMIT, shared with every text scan in this file: a name held in a module
|
||||
# constant or walked out of a list (_SNAPSHOT_COLUMNS and the COLUMN_NAME
|
||||
# constants in the endpoints and notifications chains) is not a literal and is
|
||||
# not seen. Those are all clean today; a text scan cannot promise they stay so.
|
||||
echo "==> Checking for snake_case DB column names in migrations..."
|
||||
# NUL-delimited end to end, and the list is PIPED rather than held in a
|
||||
# variable: command substitution drops null bytes, which would silently rejoin
|
||||
# a path containing a space into two half-paths.
|
||||
SNAKE_MIG_COLS=$(
|
||||
if [ "$USE_GIT_LS_FILES" = "1" ] && git rev-parse --git-dir >/dev/null 2>&1; then
|
||||
git ls-files -z -- '*migrations/versions/*.py' 2>/dev/null
|
||||
else
|
||||
find . -path '*/migrations/versions/*.py' -not -path '*/venv/*' \
|
||||
-not -path '*/node_modules/*' -not -path '*/.plugins-staged/*' -print0 2>/dev/null
|
||||
fi | xargs -0 -r awk -v Q="'" '
|
||||
# THE RULE IS LOWERCASE CONCATENATED, so the test is on the NAME, not on
|
||||
# the shape of the name. The first version of this matched
|
||||
# [a-z0-9]*_[a-z0-9_]* inside the quotes, which asks "does a lowercase
|
||||
# name contain an underscore" - so sa.Column("Monitor_Name", ...) passed
|
||||
# because of the leading capital, and so did every CamelCase name with no
|
||||
# underscore at all. Extract whatever literal the call names and test it
|
||||
# against ^[a-z][a-z0-9]*$ instead: anything else is a finding.
|
||||
#
|
||||
# SHAPES a migration writes a column name in, all of which land here:
|
||||
# sa.Column("x", ...) positional
|
||||
# sa.Column(name="x", type_=sa.String()) keyword, first or later
|
||||
# op.add_column("t", sa.Column("x", ...)) the sa.Column( inside it
|
||||
# with op.batch_alter_table(...) as batch_op:
|
||||
# batch_op.add_column(sa.Column("x", ...)) likewise
|
||||
# op.alter_column("t", "old", new_column_name="x") the rename target
|
||||
# drop_column and a plain alter_column name a column that already exists,
|
||||
# so they stay out: a bad name there was already wrong where it was
|
||||
# created, and reporting it twice makes one rename look like two.
|
||||
function nameat(s, re, quote, rest, stop) {
|
||||
# the literal that follows re, where re ENDS on the opening quote
|
||||
if (!match(s, re)) return ""
|
||||
quote = substr(s, RSTART + RLENGTH - 1, 1)
|
||||
rest = substr(s, RSTART + RLENGTH)
|
||||
stop = index(rest, quote)
|
||||
if (stop == 0) return ""
|
||||
return substr(rest, 1, stop - 1)
|
||||
}
|
||||
BEGIN {
|
||||
QQ = Q "\""
|
||||
callre = "sa\\.Column\\("
|
||||
posre = "^[ \t]*[" QQ "]"
|
||||
kwre = "(^|[^A-Za-z0-9_])name[ \t]*=[ \t]*[" QQ "]"
|
||||
renre = "new_column_name[ \t]*=[ \t]*[" QQ "]"
|
||||
contre = "sa\\.Column\\([ \t]*$"
|
||||
okre = "^[a-z][a-z0-9]*$"
|
||||
}
|
||||
FNR == 1 { held = ""; heldline = 0 }
|
||||
{
|
||||
line = $0; lineno = FNR
|
||||
if (held != "") { line = held line; lineno = heldline; held = "" }
|
||||
if (line ~ contre) { held = line; heldline = lineno; next }
|
||||
|
||||
# walk every sa.Column( on the line: op.create_table() writes several
|
||||
# on one line and each names a column of its own.
|
||||
rest = line
|
||||
while (match(rest, callre)) {
|
||||
rest = substr(rest, RSTART + RLENGTH)
|
||||
# bound this call at the next sa.Column( so a later call cannot
|
||||
# lend this one its name= argument
|
||||
if (match(rest, callre)) chunk = substr(rest, 1, RSTART - 1)
|
||||
else chunk = rest
|
||||
colname = nameat(chunk, posre)
|
||||
if (colname == "") colname = nameat(chunk, kwre)
|
||||
if (colname != "" && colname !~ okre)
|
||||
printf "%s:%d: %s\n", FILENAME, lineno, colname
|
||||
}
|
||||
colname = nameat(line, renre)
|
||||
if (colname != "" && colname !~ okre)
|
||||
printf "%s:%d: %s\n", FILENAME, lineno, colname
|
||||
}' || true)
|
||||
if [ -n "$SNAKE_MIG_COLS" ]; then
|
||||
echo "FAIL: column name created in a migration is not lowercase concatenated"
|
||||
echo " (see CONTRIBUTING.md):"
|
||||
echo "$SNAKE_MIG_COLS"
|
||||
echo
|
||||
VIOLATIONS=$((VIOLATIONS + 1))
|
||||
fi
|
||||
|
||||
echo "==> Checking for snake_case ForeignKey targets..."
|
||||
if [ "$USE_GIT_LS_FILES" = "1" ] && git rev-parse --git-dir >/dev/null 2>&1; then
|
||||
SNAKE_FK=$(git ls-files -z '*.py' | eval "$GIT_FILES_FILTER" \
|
||||
@@ -402,6 +511,43 @@ echo "==> Checking for site-specific literals in product code (ADR-015)..."
|
||||
# only worth having where
|
||||
# the leaks are.
|
||||
#
|
||||
# agent/ arrived late and is the reason this note now names it. The C# endpoint
|
||||
# agent is compiled once and installed on every bay at every site, and it holds
|
||||
# exactly the values this rule is about - a default base URL, a server hostname,
|
||||
# an SPKI pin. A site literal baked into a .cs file there ships to the next site
|
||||
# INSIDE A BINARY, where no reviewer reads it and no grep over the deployed tree
|
||||
# finds it. agent/appsettings.json and the agent's .ps1 utilities come in free,
|
||||
# since *.json and *.ps1 were already in the include list.
|
||||
#
|
||||
# --exclude=README.md IS GONE, and nothing replaces it. That flag used to sit on
|
||||
# this command ABOVE the --include list, and on GNU grep 3.11 a file-level
|
||||
# --exclude placed before the includes cancels all of them: proven
|
||||
# here by planting the same literal in x.py, x.cs, x.txt, x.md, x.csproj and
|
||||
# x.gitignore, then running the scan with and without the flag. Without it, two
|
||||
# files match (.py and .cs, as the flags say). With it, all six match. So this
|
||||
# check was reading every file under every directory listed below, and
|
||||
# --include='*.cs' was decorative - adding agent/ to the scan did NOT mean the
|
||||
# C# agent was being scanned for the reason the include list claims, and any
|
||||
# future narrowing of the include list would have been silently inert.
|
||||
#
|
||||
# No post-filter took its place because it never had anything to skip: *.md is
|
||||
# not in the include list below, so README.md cannot match this scan at all. The
|
||||
# flag's only effect was the one above.
|
||||
#
|
||||
# The .csproj post-filter is kept even though *.csproj is no longer in scope. The
|
||||
# only site names under agent/ sit in the version-history comment of one project
|
||||
# file, recording which sites a past build went to; that is a record of what
|
||||
# happened, not a value a bay receives, and a .csproj is a build input that is
|
||||
# never deployed. Keeping the filter means adding *.csproj to the include list
|
||||
# later cannot quietly turn that comment into a gate failure.
|
||||
#
|
||||
# bin/, obj/, publish/ and .vs/ are excluded ON THIS COMMAND rather than left to
|
||||
# EXCLUDES, which is empty whenever USE_GIT_LS_FILES is 1. A developer machine
|
||||
# with Visual Studio has all four under agent/, holding vendored runtime DLLs
|
||||
# and per-edit source snapshots, and walking them is what would make this scan
|
||||
# slow enough to stop being run. Directory excludes are safe here; a file-level
|
||||
# --exclude is not (see the warning above the EXCLUDES array).
|
||||
#
|
||||
# Fleet-wide vocabulary (gea-shopfloor-*) is NOT matched: it is overridable
|
||||
# through the pctypemap settings and is not one site's data.
|
||||
#
|
||||
@@ -432,11 +578,11 @@ SITE_TENANTS='geaero''spaceqa'
|
||||
SITE_PATTERNS="${SITE_HOST}|${SITE_DOMAIN}|\bwjs\b|${SITE_NAME}|${SITE_NETS}|${SITE_TENANTS}"
|
||||
SITE_HITS=$(grep -rPni --binary-files=without-match \
|
||||
"$SITE_PATTERNS" "${EXCLUDES[@]}" \
|
||||
--exclude=README.md \
|
||||
--include='*.py' --include='*.vue' --include='*.js' \
|
||||
--include='*.ps1' --include='*.psm1' --include='*.sh' --include='*.iss' \
|
||||
--include='*.json' --include='*.html' \
|
||||
plugins/ shopdb/ scripts/ deploy/ frontend/src/ tools/ 2>/dev/null \
|
||||
--include='*.json' --include='*.html' --include='*.cs' \
|
||||
--exclude-dir=bin --exclude-dir=obj --exclude-dir=publish --exclude-dir=.vs \
|
||||
plugins/ shopdb/ scripts/ deploy/ frontend/src/ tools/ agent/ 2>/dev/null \
|
||||
| grep -v '/tests\?/' \
|
||||
| grep -v '\.spec\.js:' \
|
||||
| grep -v 'site_imports/' \
|
||||
@@ -445,6 +591,7 @@ SITE_HITS=$(grep -rPni --binary-files=without-match \
|
||||
| grep -v '\.plugins-staged/' \
|
||||
| grep -v 'check-naming-and-style\.sh' \
|
||||
| grep -v 'export-github\.sh' \
|
||||
| grep -v '\.csproj:' \
|
||||
| grep -v 'ADR-015-OK' || true)
|
||||
if [ -n "$SITE_HITS" ]; then
|
||||
COUNT=$(echo "$SITE_HITS" | wc -l)
|
||||
|
||||
Reference in New Issue
Block a user