diff --git a/scripts/check-gates-ran.sh b/scripts/check-gates-ran.sh index 501e56ab..98ec1dea 100755 --- a/scripts/check-gates-ran.sh +++ b/scripts/check-gates-ran.sh @@ -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 " and "ERROR " 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 " 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 diff --git a/scripts/check-naming-and-style.sh b/scripts/check-naming-and-style.sh index 0ecaeea4..fc897aaf 100755 --- a/scripts/check-naming-and-style.sh +++ b/scripts/check-naming-and-style.sh @@ -78,10 +78,18 @@ fi # DO NOT ADD --exclude= 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() 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) diff --git a/tests/test_docs_publishable.py b/tests/test_docs_publishable.py index c4256873..5cd42c11 100644 --- a/tests/test_docs_publishable.py +++ b/tests/test_docs_publishable.py @@ -34,6 +34,14 @@ DOCS = REPO / 'docs' # published mirror - keying off the directory made this module run there against # a prose-less tree and fail. A published checkout is one with no authored # prose, matching the canonical definition in scripts/check-gates-ran.sh. +# KNOWN GAP, left deliberately for a decision rather than closed here: this +# keys on docs/*.md, so prose that lives only in docs/wiki-drafts/ does NOT +# turn the module on, and on gh-wj that is every page there is. Pointed at the +# drafts on 2026-09-18 the scrub fails on 27 lines - site name, internal git +# server, developer home paths - which is a question about what those drafts +# are for (internal wiki, or publication) and not a bug to fix by rewriting +# them. scripts/check-gates-ran.sh matches this definition exactly; change both +# together or the gate runner reports a skip it was told to expect. pytestmark = pytest.mark.skipif( not any(DOCS.glob('*.md')), reason='no docs/ prose in this checkout - it is excluded from publication and lives in the wiki') diff --git a/tests/test_naming_gate_migrations.py b/tests/test_naming_gate_migrations.py new file mode 100644 index 00000000..e02cfeee --- /dev/null +++ b/tests/test_naming_gate_migrations.py @@ -0,0 +1,295 @@ +"""Holes in `scripts/check-naming-and-style.sh`, each held open by a test. + +The naming gate must read the column names migrations CREATE, not just models. + +`scripts/check-naming-and-style.sh` checked `db.Column` attribute names on +models and nothing else, while `migrations/versions/` is filtered out of the +file list every other check in that script uses. A column is born in a +migration, so from the day the gate was written no created column name was ever +looked at: a model attribute reading fine says nothing about what the database +actually got. + +Two tests, and the second is the one that matters. A gate that prints nothing +looks identical whether it is clean or broken, and this one was silently +passing everything for its whole life, so the test plants a violation and +requires the gate to fail on it. Without that, a future edit that breaks the +regex would leave every test here green. +""" + +import re +import shutil +import subprocess +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parent.parent +GATE = REPO_ROOT / 'scripts' / 'check-naming-and-style.sh' + +# Same shapes the gate matches: the name literal of a created column, positional +# or keyword, on the sa.Column( line or on the line after it, plus the target of +# a rename. +COLUMN_LITERAL = re.compile(r"""sa\.Column\(\s*['"]([^'"]+)['"]""", re.S) +COLUMN_KEYWORD = re.compile( + r"""sa\.Column\(\s*name\s*=\s*['"]([^'"]+)['"]""", re.S) +RENAME_LITERAL = re.compile(r"""new_column_name\s*=\s*['"]([^'"]+)['"]""") + +# THE RULE, not a proxy for it. The first version of this file asked whether the +# name contained an underscore, which is the same mistake the gate's own regex +# made: 'Monitor_Name' has one but was missed for its leading capital, and a +# CamelCase name with no underscore at all is just as wrong and has nothing to +# find. Lowercase concatenated means lowercase concatenated. +LOWERCASE_CONCATENATED = re.compile(r'[a-z][a-z0-9]*\Z') + +PLANTED = '''"""Planted by the test suite. Never applied.""" +import sqlalchemy as sa +from alembic import op + +revision = 'plantedprobe' +down_revision = None + + +def upgrade(): + op.add_column('widgets', sa.Column('planted_column', sa.Integer(), + nullable=True)) + + +def downgrade(): + op.drop_column('widgets', 'planted_column') +''' + +# ASSEMBLED FROM FRAGMENTS, like the gate's own patterns and +# tests/test_docs_publishable.py: written literally, a test for the site-literal +# rule is itself a site literal, and both the gate and the publication scrub +# would find it here. +PLANTED_SITE_HOST = 'tsg' 'wp00525' '.geaero' 'space.net' +PLANTED_AGENT = ( + '// Planted by the test suite.\n' + 'internal static class Probe\n' + '{\n' + ' public const string DefaultBaseUrl = "https://' + + PLANTED_SITE_HOST + '/shopdb";\n' + '}\n' +) + + +def _migration_files(): + found = list((REPO_ROOT / 'migrations' / 'versions').glob('*.py')) + found += list(REPO_ROOT.glob('plugins/*/migrations/versions/*.py')) + return sorted(found) + + +def test_there_are_migrations_to_check(): + """Guard the guard: a glob that matches nothing passes everything below.""" + assert _migration_files(), 'no migrations found - has the layout moved?' + + +def test_no_migration_creates_a_badly_named_column(): + offenders = [] + for path in _migration_files(): + text = path.read_text(encoding='utf-8') + for pattern in (COLUMN_LITERAL, COLUMN_KEYWORD, RENAME_LITERAL): + for name in pattern.findall(text): + if not LOWERCASE_CONCATENATED.match(name): + offenders.append('{0}: {1}'.format( + path.relative_to(REPO_ROOT), name)) + assert not offenders, ( + 'column names are lowercase concatenated (CONTRIBUTING.md):\n ' + + '\n '.join(offenders)) + + +@pytest.mark.skipif(shutil.which('bash') is None, reason='needs bash') +def test_the_gate_fails_on_a_planted_migration_column(tmp_path): + """Plant one, and require the gate to say so. + + Run against a throwaway tree rather than the repository, so the planted + file cannot be left behind by a failing assert. tmp_path is outside any + checkout, so the script's git file list is unavailable and it takes its + own find fallback - which is the branch a developer without git metadata + gets, and so is worth exercising. + """ + versions = tmp_path / 'migrations' / 'versions' + versions.mkdir(parents=True) + gate = tmp_path / 'scripts' / 'check-naming-and-style.sh' + gate.parent.mkdir(parents=True) + shutil.copy(GATE, gate) + + planted = versions / 'zz01_planted.py' + planted.write_text(PLANTED, encoding='utf-8') + dirty = subprocess.run(['bash', str(gate)], cwd=str(tmp_path), + capture_output=True, text=True) + assert dirty.returncode == 1, ( + 'the gate passed a snake_case migration column:\n' + dirty.stdout) + assert 'planted_column' in dirty.stdout + + planted.unlink() + clean = subprocess.run(['bash', str(gate)], cwd=str(tmp_path), + capture_output=True, text=True) + assert clean.returncode == 0, ( + 'the gate fails with nothing planted:\n' + clean.stdout) + + +@pytest.mark.skipif(shutil.which('bash') is None, reason='needs bash') +def test_the_gate_fails_on_a_site_literal_in_the_agent(tmp_path): + """agent/ is in the ADR-015 scan, and has to stay in it. + + The C# agent is compiled once and installed on every bay at every site. A + site hostname, base URL or pin baked into a .cs file there ships to the next + site inside a binary, where no reviewer reads it - and agent/ was not one of + the directories the site-literal scan looked at. + """ + gate = tmp_path / 'scripts' / 'check-naming-and-style.sh' + gate.parent.mkdir(parents=True) + shutil.copy(GATE, gate) + agent = tmp_path / 'agent' / 'Probe' + agent.mkdir(parents=True) + + planted = agent / 'Probe.cs' + planted.write_text(PLANTED_AGENT, encoding='utf-8') + dirty = subprocess.run(['bash', str(gate)], cwd=str(tmp_path), + capture_output=True, text=True) + assert dirty.returncode == 1, ( + 'the gate passed a site hostname in the agent:\n' + dirty.stdout) + assert 'Probe.cs' in dirty.stdout + + planted.unlink() + clean = subprocess.run(['bash', str(gate)], cwd=str(tmp_path), + capture_output=True, text=True) + assert clean.returncode == 0, ( + 'the gate fails with nothing planted:\n' + clean.stdout) + + +# Every shape a migration writes a column name in. The gate's first regex asked +# for ['"][a-z0-9]*_ inside the quotes, so only the first of these was ever +# caught: a leading capital, a keyword argument or a CamelCase name with no +# underscore all walked past it. One case per shape, named so a failure says +# which shape regressed. +PLANTED_COLUMN_SHAPES = { + 'positional_snake': ( + " op.add_column('widgets', sa.Column('planted_column',\n" + " sa.Integer()))\n", 'planted_column'), + 'positional_leading_capital': ( + " op.add_column('widgets', sa.Column('Monitor_Name',\n" + " sa.String(50)))\n", 'Monitor_Name'), + 'positional_camelcase_no_underscore': ( + " op.add_column('widgets', sa.Column('MonitorName', sa.String(50)))\n", + 'MonitorName'), + 'keyword_name': ( + " op.add_column('widgets', sa.Column(name='kw_form',\n" + " type_=sa.String()))\n", 'kw_form'), + 'wrapped_after_open_paren': ( + " op.add_column('widgets', sa.Column(\n" + " 'wrapped_name', sa.Integer()))\n", 'wrapped_name'), + 'alter_column_rename': ( + " op.alter_column('widgets', 'oldname',\n" + " new_column_name='New_Name')\n", 'New_Name'), + 'create_table_second_column': ( + " op.create_table('t2', sa.Column('goodone', sa.Integer()),\n" + " sa.Column('Bad_Two', sa.Integer()))\n", 'Bad_Two'), + 'batch_alter_table_add': ( + " with op.batch_alter_table('widgets') as batch_op:\n" + " batch_op.add_column(sa.Column('batchCol', sa.Integer()))\n", + 'batchCol'), + 'batch_alter_table_rename': ( + " with op.batch_alter_table('widgets') as batch_op:\n" + " batch_op.alter_column('a', new_column_name='b_c')\n", 'b_c'), +} + +PLANTED_CLEAN_BODY = ( + " op.add_column('widgets', sa.Column('goodname', sa.Integer()))\n" + " op.add_column('widgets', sa.Column(name='alsogood', type_=sa.String()))\n" + " op.alter_column('widgets', 'oldname', new_column_name='newname')\n" + # drop_column names a column that already exists, so a bad name here is + # deliberately NOT a finding: it was wrong where it was created, and + # reporting it again makes one rename look like two. + " op.drop_column('widgets', 'legacy_old')\n" +) + + +def _migration_module(body): + return ('"""Planted by the test suite. Never applied."""\n' + 'import sqlalchemy as sa\n' + 'from alembic import op\n\n' + "revision = 'plantedprobe'\n" + 'down_revision = None\n\n\n' + 'def upgrade():\n' + body + '\n\n' + 'def downgrade():\n' + ' pass\n') + + +def _gate_tree(tmp_path): + gate = tmp_path / 'scripts' / 'check-naming-and-style.sh' + gate.parent.mkdir(parents=True, exist_ok=True) + shutil.copy(GATE, gate) + return gate + + +@pytest.mark.skipif(shutil.which('bash') is None, reason='needs bash') +@pytest.mark.parametrize('shape', sorted(PLANTED_COLUMN_SHAPES)) +def test_the_gate_catches_every_column_shape(tmp_path, shape): + """Plant one shape at a time and require the gate to name it. + + Parametrised rather than one file holding all nine, so a regression says + WHICH shape stopped being seen. A single file would fail on the first one + and hide the rest. + """ + body, expected = PLANTED_COLUMN_SHAPES[shape] + versions = tmp_path / 'migrations' / 'versions' + versions.mkdir(parents=True) + gate = _gate_tree(tmp_path) + + planted = versions / 'zz01_planted.py' + planted.write_text(_migration_module(body), encoding='utf-8') + dirty = subprocess.run(['bash', str(gate)], cwd=str(tmp_path), + capture_output=True, text=True) + assert dirty.returncode == 1, ( + 'the gate passed a {0} column:\n{1}'.format(shape, dirty.stdout)) + assert expected in dirty.stdout, ( + 'the gate failed but never named {0}:\n{1}'.format( + expected, dirty.stdout)) + + +@pytest.mark.skipif(shutil.which('bash') is None, reason='needs bash') +def test_the_gate_passes_well_named_columns(tmp_path): + """The other half: a gate that fails on everything is no better. + + Includes a drop_column with a bad name, which is out of scope on purpose. + """ + versions = tmp_path / 'migrations' / 'versions' + versions.mkdir(parents=True) + gate = _gate_tree(tmp_path) + (versions / 'zz01_clean.py').write_text( + _migration_module(PLANTED_CLEAN_BODY), encoding='utf-8') + clean = subprocess.run(['bash', str(gate)], cwd=str(tmp_path), + capture_output=True, text=True) + assert clean.returncode == 0, ( + 'the gate failed on lowercase concatenated names:\n' + clean.stdout) + + +@pytest.mark.skipif(shutil.which('bash') is None, reason='needs bash') +def test_the_site_literal_scan_honours_its_include_list(tmp_path): + """The ADR-015 scan must read the file types it declares, and no others. + + The scan carried --exclude=README.md AHEAD of its --include list, and on GNU + grep a file-level exclude in that position cancels every include on the + command. So the scan was reading EVERY file under agent/ and the other + scanned trees, and --include='*.cs' was decorative - adding agent/ did not + mean the C# agent was covered for the reason the flags claimed, and any + later narrowing of the include list would have been silently inert. + + The .cs half is covered by the test above. This is the other half: a file + type the scan does not declare must not fail it, which is only true when the + include list is actually in force. Before the fix this run came back red on + notes.txt. + """ + gate = _gate_tree(tmp_path) + agent = tmp_path / 'agent' / 'Probe' + agent.mkdir(parents=True) + for name in ('notes.txt', 'Probe.csproj', 'README.md'): + (agent / name).write_text( + 'shipped to ' + PLANTED_SITE_HOST + '\n', encoding='utf-8') + + result = subprocess.run(['bash', str(gate)], cwd=str(tmp_path), + capture_output=True, text=True) + assert result.returncode == 0, ( + 'the scan read a file type it does not declare:\n' + result.stdout) diff --git a/tests/test_plugin_migrations.py b/tests/test_plugin_migrations.py index 2cb69a24..c9de7065 100644 --- a/tests/test_plugin_migrations.py +++ b/tests/test_plugin_migrations.py @@ -15,6 +15,7 @@ These tests pin that contract: * `flask plugin upgrade-all` runs clean on a fresh DB and is idempotent. """ import ast +import re from pathlib import Path import pytest @@ -487,3 +488,634 @@ def test_upgrade_all_seeds_permissions_and_links_them_to_scopes( (plugin_manager._app, plugin_manager._db, plugin_manager.registry, plugin_manager.loader, plugin_manager.migration_manager, plugin_manager._registered_prefixes) = saved + + +# --------------------------------------------------------------------------- +# ADR-008 lane guard: a plugin migration may only do DDL on its OWN tables. +# +# The endpoints chain broke this and nothing noticed. 0021 (shipped, since +# rewritten to a no-op stub) added five client-certificate columns plus +# idx_assets_cert_fingerprint to the CORE assets table from inside a plugin +# chain, and 0023 drops them again. Both directions are guarded and idempotent +# now, so the breach self-heals on every database - but self-healing is not +# prevention, and the only thing that stopped the next one was that somebody +# happened to read the file. That is what this guard replaces. +# +# The rule, stated as ADR-008 states it: the core Alembic chain owns core +# tables, each plugin chain owns the tables listed for it in +# PLUGIN_TABLE_OWNERS, and no chain reaches across. `assets` is not in +# PLUGIN_TABLE_OWNERS['endpoints'] and cannot be, so the per-plugin MetaData +# filter never saw the columns either: the ownership map was right and the +# migration simply ignored it. +# --------------------------------------------------------------------------- + +CORE_MODELS_DIR = Path(__file__).resolve().parent.parent / 'shopdb' / 'core' / 'models' + +# Alembic op name -> position of the table name in its positional arguments. +# create_index/drop_index take the INDEX name first and the table second, and +# every constraint op takes the CONSTRAINT name first, which is the whole +# reason this is a map and not "args[0]". +# +# The constraint ops are here because they are DDL on somebody's table just as +# much as add_column is: create_foreign_key / drop_constraint / +# create_unique_constraint / create_check_constraint / create_primary_key can +# all add or drop a constraint on core `assets` from inside a plugin chain. +# Leaving them out let that pass silently. +# +# create_foreign_key's REFERENT table is deliberately not recorded. The DDL +# runs against the source table only; naming another lane's table as a foreign +# key target is a read of its shape, not a write to it, and plugins legitimately +# point at core parents (network 0002 references `models`). +_TABLE_ARG_POSITION = { + 'create_table': 0, + 'drop_table': 0, + 'add_column': 0, + 'drop_column': 0, + 'alter_column': 0, + 'rename_table': 0, + 'create_index': 1, + 'drop_index': 1, + 'create_foreign_key': 1, + 'drop_constraint': 1, + 'create_unique_constraint': 1, + 'create_check_constraint': 1, + 'create_primary_key': 1, +} + +# Keywords that carry the table when it is not passed positionally. 0023 uses +# `table_name='assets'`, which an args-only scan walks straight past. `source` +# and `source_table` are the two spellings Alembic has used for +# create_foreign_key's own table; `referent`/`referent_table` are absent on +# purpose, for the reason given above. +_TABLE_KEYWORDS = ('table_name', 'source', 'source_table', 'old_table_name') + +# Op name -> position of the COLUMN name in its positional arguments, for the +# ops that name exactly one column. The allowlist keys on this as well as the +# table, so an entry excusing a shipped drop_column cannot also excuse a NEW +# drop_column on the same table in the same file. +_COLUMN_ARG_POSITION = { + 'add_column': 1, + 'drop_column': 1, + 'alter_column': 1, +} + +# Same, for the batch_alter_table forms, where the table moved onto the context +# manager and every positional argument shifted left by one. +_BATCH_COLUMN_ARG_POSITION = { + 'add_column': 0, + 'drop_column': 0, + 'alter_column': 0, +} + +# --- raw SQL ----------------------------------------------------------------- +# op.execute() and Connection.exec_driver_sql() hand a string straight to the +# database, so an AST scan that only knows the op.* helpers sees zero ops for +# `op.execute('ALTER TABLE assets DROP COLUMN assetnumber')`. That is the +# cheapest possible way around every other guard in this file, so the statement +# text is parsed too. +# +# Only DDL against a named table counts. UPDATE/DELETE/INSERT/SELECT are data, +# not schema: three plugin revisions already run cross-checked DML +# (backups 0002, backups 0003, computers 0006) and none of them is a lane +# crossing. +# +# Unresolvable text is rendered as a sentinel character rather than dropped, so +# the difference between "interpolates the constraint name" and "interpolates +# the table name" survives into the regex. machines 0002 does the first +# (`f"ALTER TABLE equipment DROP FOREIGN KEY {name}"` - the table is literal +# text, the FK name is not) and stays resolvable; the dodge this guards against +# is the second (`f'ALTER TABLE {t} DROP COLUMN x'`), where the sentinel lands +# inside the captured table name and the op reports UNRESOLVED, which +# test_plugin_migration_ops_all_resolve_to_a_table treats as a failure. +_SQL_HOLE = '\x00' + +# Identifier characters plus the sentinel, so an interpolated name is captured +# rather than silently truncated to the literal prefix beside it. +_SQL_IDENT = r'[A-Za-z0-9_$`"\[\]\.' + _SQL_HOLE + r']+' + +# (op name, compiled pattern, number of table groups). Ordered only for +# readability; every pattern is tried against every statement. +_SQL_DDL_PATTERNS = ( + ('execute:alter_table', + re.compile(r'\bALTER\s+TABLE\s+(?:IF\s+EXISTS\s+)?(' + _SQL_IDENT + r')', re.I)), + ('execute:drop_table', + re.compile(r'\bDROP\s+TABLE\s+(?:IF\s+EXISTS\s+)?(' + _SQL_IDENT + r')', re.I)), + ('execute:create_table', + re.compile(r'\bCREATE\s+(?:TEMPORARY\s+)?TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?(' + + _SQL_IDENT + r')', re.I)), + ('execute:rename_table', + re.compile(r'\bRENAME\s+TABLE\s+(' + _SQL_IDENT + r')\s+TO\s+(' + + _SQL_IDENT + r')', re.I)), + ('execute:create_index', + re.compile(r'\bCREATE\s+(?:UNIQUE\s+|FULLTEXT\s+|SPATIAL\s+)?INDEX\s+' + + _SQL_IDENT + r'\s+ON\s+(' + _SQL_IDENT + r')', re.I)), + ('execute:drop_index', + re.compile(r'\bDROP\s+INDEX\s+' + _SQL_IDENT + r'\s+ON\s+(' + + _SQL_IDENT + r')', re.I)), + ('execute:truncate_table', + re.compile(r'\bTRUNCATE\s+(?:TABLE\s+)?(' + _SQL_IDENT + r')', re.I)), +) + +# Statement verbs that are data, not schema. A statement starting with one of +# these is skipped outright, even if its body happens to contain a word like +# "table"; anything starting with something unreadable is recorded UNRESOLVED, +# because `op.execute(sql)` with sql built elsewhere is the same dodge as an +# f-string. +_SQL_DML_VERBS = ('select', 'insert', 'update', 'delete', 'replace', 'with', + 'set', 'commit', 'rollback', 'analyze', 'optimize', 'call', + 'pragma', 'flush', 'use', 'show', 'start') + +# Historical lane crossings that have already shipped to production. Every entry +# is closed - listed so the guard is green today and red for anything new, NOT +# as a precedent to extend. Adding a line here means a plugin chain wrote to +# somebody else's table, which needs an ADR amendment, not an allowlist edit. +# +# The key is (plugin, filename, op, table, column). The column is part of it on +# purpose: keyed on the first four alone, the endpoints 0023 drop_column entry +# would also excuse a brand new `op.drop_column('assets', 'assetnumber')` +# appended to that same shipped file, which is exactly the breach this guard +# exists to stop. `None` means the op names no single column (drop_index) or +# names one the scan cannot resolve. +# +# endpoints 0023 is the CLEANUP of the 0021/0022 client-certificate breach: it +# drops the five cert columns and any index over them from the core assets +# table. It is applied on WJ production, so it cannot be edited - the fix is +# that no 0025 may do the same thing. +# +# 0021 and 0022 themselves are NOT listed, and must not be: they were rewritten +# to stamp-only no-ops, so they present no DDL to scan. Their damage lives on +# only as the 0023 cleanup below. +# +# The drop_index half of that cleanup (0023:71-73) reads alarmingly: it drops +# ANY index on assets whose column set INTERSECTS the cert columns, and never +# recreates one. An index over (assetid, client_cert_fingerprint) is dead once +# the column goes, but an index that merely happened to include a cert column +# alongside columns that still matter would be silently lost. Settled here so +# the next reader does not re-derive it - only these things ever create an index +# on assets: +# * core 68b3947ae14f baseline: idx_asset_active, idx_asset_location, +# idx_asset_status, idx_asset_type_bu, ix_assets_assetnumber, +# ix_assets_serialnumber +# * core 7b02: ix_assets_gaugelabreference, ix_assets_maintenancereference +# * core 7d33: idx_assets_levelid +# * the shipped endpoints 0021: idx_assets_cert_fingerprint over +# (client_cert_fingerprint) alone. The shipped 0022 added two more cert +# columns and no index at all. +# * db.create_all off shopdb/core/models/asset.py. Between 18e5fdc00 +# (2026-09-14) and 7480e4fe3 (2026-09-15) that model DID carry the five cert +# columns, with client_cert_fingerprint declared index=True, so create_all +# built ix_assets_client_cert_fingerprint on every database created in that +# window. That is a second cert index, not just the one 0021 named, and the +# inventory above is wrong without it. +# It changes nothing for this loop. ix_assets_client_cert_fingerprint is +# single-column over client_cert_fingerprint, which 0023 drops in the very next +# statement, so the index carries no column that outlives it and there is +# nothing to recreate. The conclusion holds: the only indexes the loop can drop +# are idx_assets_cert_fingerprint and ix_assets_client_cert_fingerprint, both +# dead by construction. No index was lost and no repair migration is owed. +# +# The filter is also unreachable by accident: every name in _CERT_COLUMNS is +# underscore-spelled, which this repo's column convention forbids, so no real +# assets column can collide with it. +ALLOWED_CROSS_LANE_OPS = { + ('endpoints', '0023_endpoint_credentials.py', 'drop_index', 'assets', None), + ('endpoints', '0023_endpoint_credentials.py', 'drop_column', 'assets', + 'client_cert_fingerprint'), + ('endpoints', '0023_endpoint_credentials.py', 'drop_column', 'assets', + 'cert_issued_at'), + ('endpoints', '0023_endpoint_credentials.py', 'drop_column', 'assets', + 'cert_expires_at'), + ('endpoints', '0023_endpoint_credentials.py', 'drop_column', 'assets', + 'cert_revoked_at'), + ('endpoints', '0023_endpoint_credentials.py', 'drop_column', 'assets', + 'cert_revocation_reason'), +} + + +def _core_tablenames() -> set: + """Every table the CORE chain owns, read statically off shopdb/core/models. + + Picks up both `__tablename__` on a model and the bare `db.Table(...)` + association tables (userroles, rolepermissions, userpermissions), which have + no model class and would otherwise read as unowned. + """ + names = set() + for source in CORE_MODELS_DIR.glob('*.py'): + tree = ast.parse(source.read_text()) + for node in ast.walk(tree): + if isinstance(node, ast.Assign): + targets = [t.id for t in node.targets if isinstance(t, ast.Name)] + if '__tablename__' in targets and isinstance(node.value, ast.Constant): + names.add(node.value.value) + if isinstance(node, ast.Call): + func = node.func + name = func.attr if isinstance(func, ast.Attribute) else getattr(func, 'id', None) + if name == 'Table' and node.args: + first = node.args[0] + if isinstance(first, ast.Constant) and isinstance(first.value, str): + names.add(first.value) + return names + + +def _table_owner_map() -> dict: + """table name -> owning lane ('core' or a plugin name).""" + owners = {} + for plugin, tables in PLUGIN_TABLE_OWNERS.items(): + for tablename in tables: + owners[tablename] = plugin + for tablename in _core_tablenames(): + # A plugin entry wins: pre-cutover the core chain CREATED the plugin + # tables, so a name can look core-shaped and still belong to a plugin. + owners.setdefault(tablename, 'core') + return owners + + +def _module_constants(tree) -> dict: + """Module-level `NAME = 'sometable'` bindings. + + Half the chains name their tables through constants (CREDENTIALS, + REQUESTS, _APPS), so a scan that only understands string literals sees + almost nothing. + """ + consts = {} + for node in tree.body: + if isinstance(node, ast.Assign) and isinstance(node.value, ast.Constant) \ + and isinstance(node.value.value, str): + for target in node.targets: + if isinstance(target, ast.Name): + consts[target.id] = node.value.value + return consts + + +def _module_sequences(tree) -> dict: + """Module-level `NAME = ('a', 'b')` bindings of all-string sequences. + + Needed for the column half of the allowlist key: endpoints 0023 drops its + five columns with `for column in _CERT_COLUMNS`, and without this the column + resolves to nothing and the allowlist entry degrades back to a blanket + "drop_column on assets from this file is fine". + """ + seqs = {} + for node in tree.body: + if not isinstance(node, ast.Assign): + continue + if not isinstance(node.value, (ast.Tuple, ast.List, ast.Set)): + continue + values = set() + for element in node.value.elts: + if not (isinstance(element, ast.Constant) and isinstance(element.value, str)): + values = set() + break + values.add(element.value) + if not values: + continue + for target in node.targets: + if isinstance(target, ast.Name): + seqs[target.id] = frozenset(values) + return seqs + + +def _literal(node, consts): + if isinstance(node, ast.Constant) and isinstance(node.value, str): + return node.value + if isinstance(node, ast.Name): + return consts.get(node.id) + return None + + +def _loop_bindings(tree, consts, seqs) -> dict: + """Map each Call node to the loop variables in scope over a literal sequence. + + `for table in (IMPACTS, TRANSITIONS, HEALTHSTATES): op.drop_table(table)` + (endpoints 0017) and the same shape in guides 0001 are real DDL over real + tables. Without this they resolve to nothing, and "resolves to nothing" has + to be a failure or the guard is dodgeable by writing a loop. + + A bare module constant as the iterable (`for column in _CERT_COLUMNS`) is + resolved through `seqs` for the same reason. + """ + bindings = {} + for node in ast.walk(tree): + if not isinstance(node, ast.For) or not isinstance(node.target, ast.Name): + continue + values = set() + if isinstance(node.iter, (ast.Tuple, ast.List, ast.Set)): + for element in node.iter.elts: + value = _literal(element, consts) + if value is None: + values = set() + break + values.add(value) + elif isinstance(node.iter, ast.Name): + values = set(seqs.get(node.iter.id, ())) + if not values: + continue + # ast.walk is breadth-first from the top, so an inner loop rebinding the + # same name is visited after the outer one and correctly shadows it. + for sub in ast.walk(node): + if isinstance(sub, ast.Call): + bindings.setdefault(id(sub), {})[node.target.id] = frozenset(values) + return bindings + + +def _resolve_tables(node, consts, loopvars) -> frozenset: + """Every table name an argument could be. Empty means unresolvable.""" + if isinstance(node, ast.Constant) and isinstance(node.value, str): + return frozenset({node.value}) + if isinstance(node, ast.Name): + if node.id in loopvars: + return loopvars[node.id] + if node.id in consts: + return frozenset({consts[node.id]}) + return frozenset() + + +def _resolve_columns(opname, node, args, consts, loopvars, positions) -> frozenset: + """The column name(s) an op names, or empty when it names none. + + Empty is NOT a failure here the way it is for a table: most ops name no + column at all. It only widens the allowlist key to None. + """ + position = positions.get(opname) + if position is None or len(args) <= position: + return frozenset() + argument = args[position] + if opname == 'add_column': + # add_column takes sa.Column('name', ...), not a bare string. + if isinstance(argument, ast.Call) and argument.args: + argument = argument.args[0] + else: + return frozenset() + return _resolve_tables(argument, consts, loopvars) + + +def _flatten_sql(node, consts) -> str: + """Statement text with every unresolvable fragment replaced by a sentinel. + + Keeping the hole in place (instead of giving up on the whole string) is what + lets `f"ALTER TABLE equipment DROP FOREIGN KEY {name}"` resolve to + `equipment` while `f'ALTER TABLE {t} DROP COLUMN x'` resolves to nothing. + """ + if isinstance(node, ast.Constant): + return node.value if isinstance(node.value, str) else _SQL_HOLE + if isinstance(node, ast.JoinedStr): + return ''.join(_flatten_sql(value, consts) for value in node.values) + if isinstance(node, ast.FormattedValue): + return _SQL_HOLE + if isinstance(node, ast.BinOp) and isinstance(node.op, ast.Add): + return _flatten_sql(node.left, consts) + _flatten_sql(node.right, consts) + if isinstance(node, ast.Name): + return consts.get(node.id, _SQL_HOLE) + if isinstance(node, ast.Call): + # sa.text('...') / text('...') is a wrapper, not a hole: computers 0006 + # passes its statement that way. + func = node.func + name = func.attr if isinstance(func, ast.Attribute) else getattr(func, 'id', None) + if name == 'text' and len(node.args) == 1: + return _flatten_sql(node.args[0], consts) + return _SQL_HOLE + + +def _clean_sql_identifier(raw: str) -> str: + """`db`.`assets` -> assets. Keeps the sentinel if one landed in the name.""" + name = raw.strip().rstrip('(;,') + if '.' in name: + name = name.rsplit('.', 1)[-1] + return name.strip('`"[]') + + +def _scan_raw_sql(node, consts) -> list: + """(opname, frozenset-of-tables) for the DDL in one execute-style call. + + An empty table set is UNRESOLVED and the caller fails on it. A statement + that is plainly DML returns nothing at all. + """ + statement = _flatten_sql(node, consts) + found = [] + # Classify PER STATEMENT, not per call. Reading the first verb and bailing + # let a leading DML verb hide trailing DDL: op.execute("SET @x = 1; ALTER + # TABLE assets DROP COLUMN serialnumber") was skipped whole, because the + # scan saw SET, called it DML and returned nothing. + for part in statement.split(';'): + stripped = part.lstrip().lstrip('(') + verb = stripped.split(None, 1)[0].lower() if stripped.split() else '' + if not verb: + continue + if verb.startswith(_SQL_HOLE): + # The verb itself is interpolated, so nothing about this statement + # can be checked. Unresolved op rather than no op. + found.append(('execute:unknown', frozenset())) + continue + if verb in _SQL_DML_VERBS: + continue + for opname, pattern in _SQL_DDL_PATTERNS: + for match in pattern.finditer(part): + for group in match.groups(): + name = _clean_sql_identifier(group) + if not name or _SQL_HOLE in name: + found.append((opname, frozenset())) + else: + found.append((opname, frozenset({name}))) + return found + + +def _scan_plugin_migration_ops() -> list: + """Every schema op in every plugin chain, as + (plugin, filename, lineno, opname, frozenset-of-tables, + frozenset-of-columns). + + An empty table set means the argument could not be resolved statically; + the caller treats that as a failure rather than a pass. An empty column set + only means the op names no resolvable single column. + """ + found = [] + for script in sorted(PLUGINS_DIR.glob('*/migrations/versions/*.py')): + plugin = script.parts[-4] + tree = ast.parse(script.read_text()) + consts = _module_constants(tree) + seqs = _module_sequences(tree) + loops = _loop_bindings(tree, consts, seqs) + + # batch_alter_table('assets') moves the table onto the context manager, + # so batch_op.drop_column('x') names a column where op.drop_column names + # a table. Resolve those against the `with` instead. + batched = {} + for node in ast.walk(tree): + if not isinstance(node, ast.With): + continue + for item in node.items: + call = item.context_expr + if not (isinstance(call, ast.Call) and isinstance(call.func, ast.Attribute) + and call.func.attr == 'batch_alter_table' and call.args): + continue + tables = _resolve_tables(call.args[0], consts, loops.get(id(call), {})) + alias = item.optional_vars.id if isinstance(item.optional_vars, ast.Name) else None + for sub in ast.walk(node): + if isinstance(sub, ast.Call) and isinstance(sub.func, ast.Attribute) \ + and sub.func.attr in _TABLE_ARG_POSITION \ + and isinstance(sub.func.value, ast.Name) \ + and sub.func.value.id == alias: + batched[id(sub)] = tables + + for node in ast.walk(tree): + if not (isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute)): + continue + opname = node.func.attr + base = node.func.value + + # Raw SQL: op.execute(...), bind.execute(...) and any + # .exec_driver_sql(...). + # + # Both names are matched on the method name ALONE, without looking at + # what they are called on. Requiring the receiver to be literally `op` + # missed the repo's DOMINANT idiom: 24 call sites across endpoints + # 0006/0007/0010/0019, network 0003 and notifications 0005 all do + # `bind = op.get_bind()` and then `bind.execute(...)`, so a plugin + # could drop a column off the core assets table through a bind and + # the scan recorded nothing at all. Every shipped bind.execute passes + # a literal SELECT or UPDATE, which _SQL_DML_VERBS skips, so widening + # this costs no false positives. + if opname in ('exec_driver_sql', 'execute'): + if not node.args: + continue + for sqlop, tables in _scan_raw_sql(node.args[0], consts): + found.append((plugin, script.name, node.lineno, sqlop, + tables, frozenset())) + continue + + if opname not in _TABLE_ARG_POSITION: + continue + loopvars = loops.get(id(node), {}) + if id(node) in batched: + columns = _resolve_columns(opname, node, node.args, consts, loopvars, + _BATCH_COLUMN_ARG_POSITION) + found.append((plugin, script.name, node.lineno, + 'batch_' + opname, batched[id(node)], columns)) + continue + if not (isinstance(base, ast.Name) and base.id == 'op'): + continue + position = _TABLE_ARG_POSITION[opname] + tables = frozenset() + if len(node.args) > position: + tables = _resolve_tables(node.args[position], consts, loopvars) + for keyword in node.keywords: + if keyword.arg in _TABLE_KEYWORDS: + tables = tables | _resolve_tables(keyword.value, consts, loopvars) + columns = _resolve_columns(opname, node, node.args, consts, loopvars, + _COLUMN_ARG_POSITION) + found.append((plugin, script.name, node.lineno, opname, tables, columns)) + if opname == 'rename_table' and len(node.args) > 1: + # Both ends of a rename are DDL on a table. + found.append((plugin, script.name, node.lineno, 'rename_table', + _resolve_tables(node.args[1], consts, loopvars), + frozenset())) + return found + + +def _cross_lane_ops() -> list: + """Scanned ops whose table belongs to core or to a different plugin. + + A table that is in neither the core models nor PLUGIN_TABLE_OWNERS is NOT a + crossing: that is a pre-rename name (machines 0002 renames equipment -> + machines) or a table since dropped, and neither reaches into another lane. + Ownership drift of a table that DOES still exist is already covered by + test_owned_tables_match_declared_models. + """ + owners = _table_owner_map() + crossings = [] + for plugin, filename, lineno, opname, tables, columns in _scan_plugin_migration_ops(): + for tablename in sorted(tables): + owner = owners.get(tablename) + if owner is None or owner == plugin: + continue + for column in (sorted(columns) or [None]): + crossings.append((plugin, filename, lineno, opname, tablename, + owner, column)) + return crossings + + +def _crossing_keys() -> set: + return {(plugin, filename, opname, tablename, column) + for plugin, filename, _, opname, tablename, _, column in _cross_lane_ops()} + + +def test_plugin_migration_ops_all_resolve_to_a_table(): + """Every scanned op names a table the scan can actually resolve. + + Without this the guard is dodgeable by accident: a table name built at + runtime resolves to nothing, and "nothing" would otherwise read as "touches + no foreign table" and pass. It is also what makes the raw-SQL scan real - + `op.execute(f'ALTER TABLE {t} ...')` lands here rather than going quietly + green. + """ + unresolved = [(plugin, filename, lineno, opname) + for plugin, filename, lineno, opname, tables, _ + in _scan_plugin_migration_ops() if not tables] + assert not unresolved, ( + 'plugin migration ops whose table name could not be resolved ' + f'statically: {unresolved}') + + +def test_scan_sees_the_known_lane_crossing(): + """The scanner is not vacuous: it still finds the 0023 assets cleanup. + + If a refactor breaks the AST walk, every other assertion here goes green on + an empty result set. This one goes red. + """ + scanned = _scan_plugin_migration_ops() + assert len(scanned) > 100, f'scan found only {len(scanned)} ops; it is broken' + keys = _crossing_keys() + assert ALLOWED_CROSS_LANE_OPS <= keys, ( + 'the allowlisted historical crossings are no longer detected: ' + f'{ALLOWED_CROSS_LANE_OPS - keys}') + + +def test_scan_reads_raw_sql_ddl(): + """The raw-SQL half is not vacuous either. + + machines 0002 does its whole MySQL rename through exec_driver_sql. If the + statement parser stops working, those ops vanish and nothing else in this + file notices. + """ + raw = [(plugin, filename, opname, tuple(sorted(tables))) + for plugin, filename, _, opname, tables, _ + in _scan_plugin_migration_ops() if opname.startswith('execute:')] + assert ('machines', '0002_rename_from_equipment.py', 'execute:rename_table', + ('equipmenttypes',)) in raw, raw + assert ('machines', '0002_rename_from_equipment.py', 'execute:alter_table', + ('machines',)) in raw, raw + # DML stays out of it, or the three shipped UPDATE statements would read as + # schema changes on tables their own plugin owns. + assert not [entry for entry in raw + if entry[0] == 'backups' or entry[1].startswith('0006_policy')], raw + + +def test_no_plugin_migration_touches_another_lanes_table(): + """ADR-008: a plugin chain does DDL only on the tables it owns. + + Fails on create_table/drop_table/add_column/drop_column/alter_column/ + create_index/drop_index/rename_table, the five constraint ops, the + batch_alter_table forms, and DDL inside op.execute/exec_driver_sql, against + a core table or another plugin's table. The allowlist above holds exactly + the entries that already shipped. + """ + offenders = [crossing for crossing in _cross_lane_ops() + if (crossing[0], crossing[1], crossing[3], crossing[4], crossing[6]) + not in ALLOWED_CROSS_LANE_OPS] + assert not offenders, '\n'.join( + f'{plugin} chain: {filename}:{lineno} {opname} on {tablename!r}' + + (f' column {column!r}' if column else '') + + f', owned by {owner} (ADR-008)' + for plugin, filename, lineno, opname, tablename, owner, column in offenders) + + +def test_cross_lane_allowlist_has_no_dead_entries(): + """The allowlist does not outlive what it excuses. + + An entry left behind after its migration is squashed or rewritten is a + standing licence to write to a core table from that file again. + """ + keys = _crossing_keys() + assert not (ALLOWED_CROSS_LANE_OPS - keys), ( + 'allowlist entries that match nothing in the tree: ' + f'{ALLOWED_CROSS_LANE_OPS - keys}')