Security hardening, multi-site portability, and a documentation and API-surface sweep

tests/test_api_inventory_parity.py

@@ -0,0 +1,95 @@
+"""Every route the app serves is described in docs/api-inventory.json.
+
+The inventory is hand-maintained, and deliberately so: its value is the prose -
+what each endpoint is FOR, what it accepts, a worked example - and none of that
+can be derived from the code. An audit of all 372 documented operations found
+zero phantom routes and zero wrong parameter names, so the maintenance itself is
+in good order.
+
+What it cannot do is notice a route somebody added. Twenty-two were serving
+traffic undocumented, including an entire plugin's surface and every one of the
+dashboard card endpoints - each of them added after its surface was written.
+
+So the prose stays hand-written and the COVERAGE is enforced here. Adding an
+endpoint now means describing it, in the same commit, which is when the author
+still knows what it is for.
+"""
+
+import json
+import re
+from pathlib import Path
+
+import pytest
+
+from shopdb import create_app
+
+REPO = Path(__file__).resolve().parents[1]
+INVENTORY = REPO / 'docs' / 'api-inventory.json'
+
+pytestmark = pytest.mark.skipif(
+    not INVENTORY.is_file(),
+    reason='docs/ is excluded from publication; nothing to check here')
+
+# Flask's own machinery, not this product's API.
+EXEMPT_ENDPOINTS = {'static'}
+
+# Documented as a family rather than per-file: these serve the frontend bundle
+# and its assets, not JSON.
+EXEMPT_RULE_PREFIXES = ('/static', '/assets')
+
+
+def normalise(rule):
+    """Flask `<int:id>` and inventory `<id>` describe the same path.
+
+    Converters are an implementation detail of the route, and the inventory was
+    written without them. Comparing on the converter would report a difference
+    that is not one.
+    """
+    return re.sub(r'<(?:[^:>]+:)?([^>]+)>', r'<\1>', rule)
+
+
+def served_operations():
+    app = create_app('testing')
+    served = set()
+    for rule in app.url_map.iter_rules():
+        if rule.endpoint in EXEMPT_ENDPOINTS:
+            continue
+        if any(rule.rule.startswith(prefix) for prefix in EXEMPT_RULE_PREFIXES):
+            continue
+        if not rule.rule.startswith('/api'):
+            continue
+        for method in sorted((rule.methods or set()) - {'HEAD', 'OPTIONS'}):
+            served.add((method, normalise(rule.rule)))
+    return served
+
+
+def documented_operations():
+    documented = set()
+    for surface in json.loads(INVENTORY.read_text()):
+        for entry in surface.get('endpoints', []):
+            path = normalise(entry.get('path') or '')
+            # The inventory writes multi-verb routes as 'PUT|PATCH'. Splitting
+            # on comma and slash alone left those undocumented in this check
+            # AND dropped from the generated spec entirely.
+            for method in re.split(r'[/,|]', entry.get('method') or 'GET'):
+                method = method.strip().upper()
+                if method:
+                    documented.add((method, path))
+    return documented
+
+
+def test_every_served_route_is_documented():
+    missing = sorted(served_operations() - documented_operations())
+    assert not missing, (
+        '%d route(s) serve traffic with no entry in docs/api-inventory.json. '
+        'Describe them there (purpose, auth, params, example) and regenerate '
+        'the spec with scripts/gen_openapi.py:\n  %s'
+        % (len(missing), '\n  '.join('%s %s' % row for row in missing)))
+
+
+def test_nothing_documented_has_been_removed():
+    """A documented route that no longer exists sends a reader to a 404."""
+    stale = sorted(documented_operations() - served_operations())
+    assert not stale, (
+        '%d documented route(s) are no longer served:\n  %s'
+        % (len(stale), '\n  '.join('%s %s' % row for row in stale)))

tests/test_core/test_clientip.py

@@ -4,11 +4,11 @@ from shopdb.utils.clientip import strip_port, client_ip
 
 
 def test_strip_ipv4_port():
-    assert strip_port('10.134.48.20:52344') == '10.134.48.20'
+    assert strip_port('192.0.2.20:52344') == '192.0.2.20'
 
 
 def test_ipv4_without_port_untouched():
-    assert strip_port('10.134.48.20') == '10.134.48.20'
+    assert strip_port('192.0.2.20') == '192.0.2.20'
 
 
 def test_bare_ipv6_untouched():

tests/test_core/test_relationship_soft_delete.py

@@ -0,0 +1,76 @@
+"""Deleting a relationship is soft, and everything that reads them must know.
+
+Rows are retired with isactive = False, never removed, and assetrelationships is
+unique on (source, target, type). Anything that looks a relationship up without
+filtering on isactive therefore sees links the UI does not show - and refuses to
+create one it cannot create any other way, or retires the last live row believing
+another one covers it.
+"""
+
+import pytest
+
+from shopdb.core.models import Asset, AssetType
+from shopdb.core.models.relationship import RelationshipType, AssetRelationship
+
+
+@pytest.fixture
+def rig(db):
+    if not AssetType.query.filter_by(assettype='computer').first():
+        db.session.add(AssetType(assettype='computer'))
+    if not AssetType.query.filter_by(assettype='machine').first():
+        db.session.add(AssetType(assettype='machine'))
+    controls = RelationshipType.query.filter_by(relationshiptype='controls').first()
+    if not controls:
+        controls = RelationshipType(relationshiptype='controls', isdirectional=True)
+        db.session.add(controls)
+    controls.isdirectional = True
+    db.session.commit()
+    computer = AssetType.query.filter_by(assettype='computer').first()
+    machine = AssetType.query.filter_by(assettype='machine').first()
+    pc = Asset(assetnumber='PCSOFT01', assettypeid=computer.assettypeid)
+    bay = Asset(assetnumber='BAYSOFT01', assettypeid=machine.assettypeid)
+    db.session.add_all([pc, bay])
+    db.session.commit()
+    return {'pc': pc, 'bay': bay, 'controls': controls}
+
+
+def _create(client, headers, source, target, typeid):
+    return client.post('/api/assets/relationships',
+                       json={'sourceassetid': source, 'targetassetid': target,
+                             'relationshiptypeid': typeid},
+                       headers=headers)
+
+
+def test_a_deleted_relationship_can_be_created_again(client, db, rig, auth_headers):
+    """Soft delete then re-add answered 409 about a link nothing displayed."""
+    created = _create(client, auth_headers, rig['pc'].assetid, rig['bay'].assetid,
+                      rig['controls'].relationshiptypeid)
+    assert created.status_code == 201, created.get_json()
+    relid = created.get_json()['data']['relationshipid']
+
+    assert client.delete('/api/assets/relationships/%d' % relid,
+                         headers=auth_headers).status_code == 200
+
+    again = _create(client, auth_headers, rig['pc'].assetid, rig['bay'].assetid,
+                    rig['controls'].relationshiptypeid)
+    assert again.status_code in (200, 201), again.get_json()
+    row = db.session.get(AssetRelationship, relid)
+    assert row.isactive is True
+    assert AssetRelationship.query.filter_by(
+        sourceassetid=rig['pc'].assetid,
+        targetassetid=rig['bay'].assetid).count() == 1
+
+
+def test_a_deleted_inverse_does_not_block_the_right_direction(
+        client, db, rig, auth_headers):
+    """'Remove the existing one first' is the advice the guard itself gives."""
+    wrong = _create(client, auth_headers, rig['bay'].assetid, rig['pc'].assetid,
+                    rig['controls'].relationshiptypeid)
+    assert wrong.status_code == 201
+    relid = wrong.get_json()['data']['relationshipid']
+    assert client.delete('/api/assets/relationships/%d' % relid,
+                         headers=auth_headers).status_code == 200
+
+    right = _create(client, auth_headers, rig['pc'].assetid, rig['bay'].assetid,
+                    rig['controls'].relationshiptypeid)
+    assert right.status_code in (200, 201), right.get_json()

tests/test_core/test_security_headers.py

@@ -0,0 +1,57 @@
+"""Responses carry the baseline headers, and uploads carry the strict ones.
+
+The application image route is PUBLIC and one accepted image type is an XML
+document that can carry a script. Loaded through an <img> tag that script never
+runs, so the tiles were never the risk; opening the file's own URL is. These
+tests pin the headers that make that case inert, because the failure is silent -
+nothing about the page looks different when they are missing.
+"""
+
+import io
+
+import pytest
+
+from shopdb.utils.uploads import UPLOAD_CSP
+
+
+def test_every_response_carries_the_baseline_headers(client):
+    resp = client.get('/api/health')
+    assert resp.headers.get('X-Content-Type-Options') == 'nosniff'
+    assert resp.headers.get('X-Frame-Options') == 'SAMEORIGIN'
+    assert resp.headers.get('Referrer-Policy') == 'strict-origin-when-cross-origin'
+
+
+def test_an_error_response_carries_them_too(client):
+    """after_request runs for error handlers as well, which is where a response
+    built by a different code path would otherwise slip through."""
+    resp = client.get('/api/no-such-route')
+    assert resp.status_code == 404
+    assert resp.headers.get('X-Content-Type-Options') == 'nosniff'
+
+
+def test_a_served_upload_is_sandboxed(client, app, tmp_path):
+    """The uploaded file itself gets a policy that blocks script execution."""
+    from shopdb.core.api.applications import _appimage_dir
+
+    with app.app_context():
+        directory = _appimage_dir()
+    svg = ('<svg xmlns="http://www.w3.org/2000/svg">'
+           '<script>window.top.location="http://evil"</script></svg>')
+    path = directory / 'probe-headers.svg' if hasattr(directory, '__truediv__') \
+        else None
+    if path is None:
+        import os
+        path = os.path.join(directory, 'probe-headers.svg')
+        with open(path, 'w') as handle:
+            handle.write(svg)
+    else:
+        path.write_text(svg)
+
+    try:
+        resp = client.get('/api/applications/image/probe-headers.svg')
+        assert resp.status_code == 200
+        assert resp.headers.get('Content-Security-Policy') == UPLOAD_CSP
+        assert resp.headers.get('X-Content-Type-Options') == 'nosniff'
+    finally:
+        import os
+        os.remove(path)

tests/test_docs_publishable.py

@@ -49,6 +49,29 @@ FORBIDDEN = [
     ('root' + 'password', 'contains a database root password'),
     (r'\b' + 'cla' + 'ude' + r'\b', 'names an LLM assistant'),
     (r'\b' + 'anthro' + 'pic' + r'\b', 'names an LLM vendor'),
+    # ADR-015. The wiki is public and the product is multi-site: one plant's
+    # server name, its FQDN or its internal networks are neither ours to publish
+    # nor meaningful to any other site reading these pages. Assembled from
+    # fragments for the same reason as the terms above.
+    ('tsg' + 'wp00525', 'names a production server'),
+    (r'\b' + r'172\.16\.' + r'\d', 'names an internal network'),
+    # A tool-call fragment reached the end of an ACCEPTED ADR and published to
+    # the wiki. It says nothing to a reader and everything about how the
+    # document was produced, which is not what a decision record is for.
+    ('</' + 'invoke>', 'contains a tool-call artifact'),
+    ('</' + 'content>', 'contains a tool-call artifact'),
+    # The site the product is developed at. Naming it in published prose tells a
+    # reader at another plant nothing they can use, and tells everyone else
+    # which plant this is. Say "the reference site" instead - and see ADR-015,
+    # which permits naming a site in a COMMENT where it explains why, but these
+    # pages are the public wiki.
+    ('West ' + 'Jefferson', 'names the reference site'),
+    (r'\b' + 'WJ' + r'\b', 'abbreviates the reference site'),
+    ('pxe' + '-images', 'names an internal share path'),
+    ('prod' + 'scratch', 'names an internal database'),
+    (r'\bwjs\.' + r'geaerospace\.net\b', 'names a site FQDN'),
+    (r'10\.134\.48\.', 'names an internal network'),
+    (r'10\.48\.249\.', 'names an internal network'),
 ]
 
 # Generated API surface. Not prose, not hand-edited, and regenerated from the

tests/test_docs_versions.py

@@ -0,0 +1,115 @@
+"""No document types a version or a count that the code already knows.
+
+Nine documents carried a hand-typed contract version. Every one was stale, and
+one of them was load-bearing: PLUGIN-EXTERNAL-REPO.md told an external author to
+pin `>=0.13.0,<0.14.0` against a contract at 0.19.0, so a plugin built by
+following the guide is refused by the loader at startup. The plugin count was
+wrong in six more.
+
+A number that is copied is a number that goes stale, and prose gives no signal
+about which lines are still true. the PROJECT-MAP wiki page is generated for exactly
+this, so a document points at it instead of restating it.
+
+This test is the rule. Where a version genuinely belongs in prose - the contract
+history table, a changelog entry, an ADR recording what was decided when - it is
+recording the PAST, which does not go stale.
+"""
+
+import re
+from pathlib import Path
+
+import pytest
+
+REPO = Path(__file__).resolve().parents[1]
+DOCS = REPO / 'docs'
+
+pytestmark = pytest.mark.skipif(
+    not DOCS.is_dir(),
+    reason='no docs/ in this checkout - it is excluded from publication')
+
+# The map is generated and the changelog records history; both are allowed to
+# carry literal versions. CONTRACT-STABILITY's table is a history of what each
+# contract version DID, which is the past and stays true.
+EXEMPT = {'PROJECT-MAP.md', 'CONTRACT-STABILITY.md'}
+
+ASSIGNMENT = re.compile(r"__(?:contract_)?version__\s*=\s*['\"]([0-9]+\.[0-9]+\.[0-9]+)['\"]")
+NARRATED = re.compile(
+    r"__(?:contract_)?version__[^\n]{0,60}?\b([0-9]+\.[0-9]+\.[0-9]+)\b"
+    r"|\b(?:contract|version) is (?:at|currently)\s+\*{0,2}([0-9]+\.[0-9]+\.[0-9]+)")
+PLUGIN_COUNT = re.compile(r'\b(\d+)\s+bundled plugins\b', re.I)
+
+
+def documentation_files():
+    return sorted(p for p in DOCS.rglob('*.md') if p.name not in EXEMPT)
+
+
+def current_versions():
+    text = (REPO / 'shopdb' / '__init__.py').read_text()
+    return {
+        name: re.search(r"^%s\s*=\s*['\"]([^'\"]+)['\"]" % name, text, re.M).group(1)
+        for name in ('__version__', '__contract_version__')
+    }
+
+
+def bundled_plugin_count():
+    return len(list((REPO / 'plugins').glob('*/manifest.json')))
+
+
+def test_no_document_declares_a_version_literal():
+    """`__contract_version__ = '0.13.0'` in prose is a promise the code breaks."""
+    offenders = []
+    for path in documentation_files():
+        for number, line in enumerate(path.read_text(errors='replace').splitlines(), 1):
+            if ASSIGNMENT.search(line):
+                offenders.append('%s:%d  %s' % (path.relative_to(REPO), number, line.strip()[:90]))
+    assert not offenders, (
+        'These documents declare a version literal. Point at the PROJECT-MAP wiki page, '
+        'which is generated:\n  ' + '\n  '.join(offenders))
+
+
+def test_a_narrated_version_matches_the_code():
+    """Prose that states the CURRENT version has to be right."""
+    versions = set(current_versions().values())
+    offenders = []
+    for path in documentation_files():
+        # An ADR states the version a decision was taken AT. That is a record of
+        # the past, not a claim about today, and rewriting it would falsify the
+        # record this project keeps ADRs for.
+        if path.parent.name == 'adr':
+            continue
+        for number, line in enumerate(path.read_text(errors='replace').splitlines(), 1):
+            for match in NARRATED.finditer(line):
+                found = match.group(1) or match.group(2)
+                if found and found not in versions:
+                    offenders.append('%s:%d  says %s  %s'
+                                     % (path.relative_to(REPO), number, found, line.strip()[:70]))
+    assert not offenders, (
+        'These lines state a current version that no longer matches '
+        'shopdb/__init__.py:\n  ' + '\n  '.join(offenders))
+
+
+def test_a_stated_plugin_count_matches_the_tree():
+    actual = bundled_plugin_count()
+    offenders = []
+    for path in documentation_files():
+        for number, line in enumerate(path.read_text(errors='replace').splitlines(), 1):
+            for match in PLUGIN_COUNT.finditer(line):
+                if int(match.group(1)) != actual:
+                    offenders.append('%s:%d  says %s, tree has %d'
+                                     % (path.relative_to(REPO), number, match.group(1), actual))
+    assert not offenders, (
+        'These documents count plugins by hand. The count is in '
+        'the PROJECT-MAP wiki page:\n  ' + '\n  '.join(offenders))
+
+
+def test_the_map_itself_is_current():
+    """The pointer target has to be right, or every document pointing at it is
+    wrong at one remove."""
+    mapfile = DOCS / 'PROJECT-MAP.md'
+    assert mapfile.is_file(), 'the PROJECT-MAP wiki page is missing; run scripts/gen_project_map.py'
+    text = mapfile.read_text()
+    for name, version in current_versions().items():
+        assert version in text, (
+            '%s is %s in the code but the generated map does not carry it. '
+            'Run: venv/bin/python scripts/gen_project_map.py' % (name, version))
+    assert str(bundled_plugin_count()) in text

tests/test_openapi_spec.py

@@ -0,0 +1,123 @@
+"""The generated spec has to be usable by a machine, not just readable.
+
+docs/openapi.json is the only machine-readable description of this API, and it
+is what the MCP server builds its tools from. It was published with no
+`parameters` and no `requestBody` on any operation - invalid OpenAPI 3.1, and
+worse in practice: a tool built from an operation with no parameters has nowhere
+to put the id its caller supplied, so every argument was dropped in silence. A
+call for one asset returned the list.
+
+These tests are the floor. They check the spec's shape, not its prose.
+"""
+
+import json
+import re
+from pathlib import Path
+
+import pytest
+
+REPO = Path(__file__).resolve().parents[1]
+SPEC = REPO / 'docs' / 'openapi.json'
+
+pytestmark = pytest.mark.skipif(
+    not SPEC.is_file(),
+    reason='no generated spec in this checkout')
+
+
+@pytest.fixture(scope='module')
+def spec():
+    return json.loads(SPEC.read_text())
+
+
+def operations(spec):
+    for path, item in spec['paths'].items():
+        for verb, operation in item.items():
+            yield path, verb, operation
+
+
+def test_every_templated_path_declares_its_parameters(spec):
+    """`/api/assets/{id}` with no parameters is not a description of anything."""
+    offenders = [
+        '%s %s' % (verb.upper(), path)
+        for path, verb, operation in operations(spec)
+        if '{' in path and not operation.get('parameters')
+    ]
+    assert not offenders, (
+        'These operations have a templated path and no parameters, so a '
+        'generated client cannot fill it in:\n  ' + '\n  '.join(offenders[:15]))
+
+
+def test_declared_parameters_match_the_template(spec):
+    """A parameter that is not in the path, or a placeholder with no parameter,
+    is the same defect wearing different clothes."""
+    offenders = []
+    for path, verb, operation in operations(spec):
+        intemplate = set(re.findall(r'\{([^}]+)\}', path))
+        declared = {p['name'] for p in operation.get('parameters', [])
+                    if p.get('in') == 'path'}
+        if intemplate != declared:
+            offenders.append('%s %s: template %s, declared %s'
+                             % (verb.upper(), path, sorted(intemplate), sorted(declared)))
+    assert not offenders, '\n  '.join(offenders[:15])
+
+
+def test_write_verbs_can_carry_a_body(spec):
+    offenders = [
+        '%s %s' % (verb.upper(), path)
+        for path, verb, operation in operations(spec)
+        if verb in ('post', 'put') and 'requestBody' not in operation
+    ]
+    assert not offenders, (
+        'These write operations declare no request body, so a client built '
+        'from this spec cannot send one:\n  ' + '\n  '.join(offenders[:15]))
+
+
+def test_optional_auth_is_expressed_as_optional(spec):
+    """An endpoint that works logged out must not be published as requiring a
+    token - that is a lie to every reader, and it is the majority case here."""
+    optional = [operation for _, _, operation in operations(spec)
+                if any(item == {} for item in operation.get('security') or [])]
+    assert len(optional) > 50, (
+        'The inventory documents over a hundred jwt-optional endpoints; the '
+        'spec should express them with an empty security requirement '
+        'alongside the scheme, not as bearer-required.')
+
+
+def test_an_endpoint_that_needs_no_token_cannot_fail_with_401(spec):
+    offenders = [
+        '%s %s' % (verb.upper(), path)
+        for path, verb, operation in operations(spec)
+        if not operation.get('security') and '401' in operation.get('responses', {})
+    ]
+    assert not offenders, '\n  '.join(offenders[:15])
+
+
+def test_every_operation_documents_failure(spec):
+    offenders = [
+        '%s %s' % (verb.upper(), path)
+        for path, verb, operation in operations(spec)
+        if 'default' not in operation.get('responses', {})
+    ]
+    assert not offenders, (
+        'These operations document only success:\n  ' + '\n  '.join(offenders[:15]))
+
+
+def test_the_envelopes_are_defined_and_referenced(spec):
+    schemas = spec.get('components', {}).get('schemas', {})
+    assert 'SuccessEnvelope' in schemas and 'ErrorEnvelope' in schemas
+    # The error shape is the one people get wrong: nested under data.
+    error = schemas['ErrorEnvelope']['properties']['data']['properties']['error']
+    assert set(error['required']) == {'code', 'message'}
+
+
+def test_summaries_are_not_cut_mid_word(spec):
+    """A truncated summary is what a tool picker shows as the whole description
+    of a call."""
+    offenders = []
+    for path, verb, operation in operations(spec):
+        summary = operation.get('summary', '')
+        if summary.endswith('...'):
+            body = summary[:-3]
+            if body and not body[-1].isalnum() and body[-1] not in ')]"\'':
+                offenders.append('%s %s: %r' % (verb.upper(), path, summary[-40:]))
+    assert not offenders, '\n  '.join(offenders[:15])

tests/test_plugin_migrations.py

@@ -52,7 +52,7 @@ EXPECTED_HEAD_REVISION['measuringtools'] = 'measuringtools0001baseline'
 EXPECTED_HEAD_REVISION['backups'] = 'backups0003clearlastseen'
 # geenforce adds the content-addressed blob store (manifestblobs) on top of its
 # baseline.
-EXPECTED_HEAD_REVISION['geenforce'] = 'geenforce0003subtype'
+EXPECTED_HEAD_REVISION['geenforce'] = 'geenforce0004minlib'
 # printers adds the printersupplyalerts crossing-state table on top of its anchor.
 EXPECTED_HEAD_REVISION['printers'] = 'printers0002supplyalerts'
 # machines (renamed from equipment) keeps its original anchor id and adds the
@@ -64,7 +64,7 @@ EXPECTED_HEAD_REVISION['employees'] = 'employees0002photo'
 EXPECTED_HEAD_REVISION['usb'] = 'usb0002dropmachineid'
 # network links a device to a catalog model, which is where its photo comes
 # from - machines, PCs and printers already had that link.
-EXPECTED_HEAD_REVISION['network'] = 'network0002model'
+EXPECTED_HEAD_REVISION['network'] = 'network0003prefix'
 # printedparts is post-cutover: its 0001 really creates its tables; 0004 adds
 # the per-transaction revision column.
 EXPECTED_HEAD_REVISION['printedparts'] = 'printedparts0004txnrev'

tests/test_plugins/test_backups.py

@@ -206,12 +206,19 @@ def test_partmarker_kind_is_share_backed_and_not_renderable():
     assert kind.parse(b'anything') is None
 
 
-def test_partmarker_sharedir_is_under_the_sfld_backups_root():
-    path = registry.getkind('partmarker').sharedir('lathe', '3204')
-    assert path.startswith(registry.DEFAULTSHAREROOT)
+def test_partmarker_sharedir_is_under_the_configured_backups_root():
+    root = r'\\fileserver\shopfloor\backups'
+    path = registry.getkind('partmarker').sharedir('lathe', '3204', shareroot=root)
+    assert path.startswith(root)
     assert path.endswith(r'lathe\3204\partmarker')
 
 
+def test_sharedir_is_empty_when_no_root_is_configured():
+    """No default root ships (ADR-015), so an unconfigured site gets nothing
+    rather than a path composed from another site's file server."""
+    assert registry.getkind('partmarker').sharedir('lathe', '3204') == ''
+
+
 def test_getkind_is_case_insensitive_and_returns_none_when_unknown():
     assert registry.getkind('NTLARS') is not None
     assert registry.getkind('nosuchkind') is None
@@ -981,3 +988,21 @@ def test_the_panel_shows_verified_beside_captured(bk_app, bk_plugin):
         listpanel = next(p for p in panels if p.get('render') == 'list')
         labels = [m['label'] for m in listpanel['map']['meta']]
         assert 'Captured' in labels and 'Verified' in labels
+
+
+def test_retention_reads_the_key_the_settings_page_writes(app, db):
+    """The plugin helper namespaces to `plugin.backups.<key>`; the declared key
+    is bare. Reading through the helper meant retention was configurable and
+    inert - it always fell back to 0, which means keep everything."""
+    from shopdb.core.models import Setting
+    from plugins.backups.plugin import BackupsPlugin
+
+    declared = {entry['key'] for entry in BackupsPlugin().get_settings_defaults()}
+    assert 'backups_retentioncount' in declared
+
+    Setting.set('backups_retentioncount', 5)
+    db.session.commit()
+    assert int(Setting.get('backups_retentioncount', 0)) == 5
+    # The helper cannot see it, which is the whole defect: it looks under
+    # plugin.backups.backups_retentioncount, which nothing writes.
+    assert int(BackupsPlugin().get_setting('backups_retentioncount', 0) or 0) == 0

tests/test_plugins/test_collector_measuringtool_adoption.py

@@ -0,0 +1,232 @@
+"""The collector ADOPTS an existing instrument instead of minting a twin.
+
+Minting derived the asset number from the HOSTNAME, so a permanent instrument
+inherited the identity of whichever PC drove it that week. And because the
+collector keyed idempotency on its OWN label, it could not see a tool it had not
+created - on prod that left 43 legacy MT-#### tools shadowed by minted
+<HOST>-CMM twins, three records deep for some CMMs.
+"""
+
+import pytest
+
+from shopdb.core.models import Asset, AssetType
+from shopdb.core.models.relationship import RelationshipType, AssetRelationship
+from plugins.measuringtools.models import MeasuringTool
+
+KEY = 'collector-adoption-key'
+
+
+@pytest.fixture
+def collector_key(app):
+    old = app.config.get('COLLECTOR_API_KEY')
+    app.config['COLLECTOR_API_KEY'] = KEY
+    yield KEY
+    app.config['COLLECTOR_API_KEY'] = old
+
+
+@pytest.fixture
+def rig(db):
+    """Asset types, a controls relationship type, and nothing else."""
+    for name in ('computer', 'measuring_tool'):
+        if not AssetType.query.filter_by(assettype=name).first():
+            db.session.add(AssetType(assettype=name))
+    if not RelationshipType.query.filter_by(relationshiptype='controls').first():
+        db.session.add(RelationshipType(relationshiptype='controls'))
+    db.session.commit()
+
+
+def _tool(db, assetnumber):
+    """A measuring tool asset the collector did NOT create."""
+    assettype = AssetType.query.filter_by(assettype='measuring_tool').first()
+    asset = Asset(assetnumber=assetnumber, assettypeid=assettype.assettypeid)
+    db.session.add(asset)
+    db.session.flush()
+    db.session.add(MeasuringTool(assetid=asset.assetid))
+    db.session.commit()
+    return asset
+
+
+def _report(client, key, hostname, **extra):
+    payload = {'hostname': hostname, 'pctype': 'gea-shopfloor-cmm'}
+    payload.update(extra)
+    return client.post('/api/collector/computers', json=payload,
+                       headers={'X-API-Key': key})
+
+
+def _tools_controlled(hostname):
+    pc = Asset.query.filter_by(assetnumber=hostname).first()
+    controls = RelationshipType.query.filter_by(relationshiptype='controls').first()
+    toolids = {r.assetid for r in MeasuringTool.query.with_entities(MeasuringTool.assetid)}
+    rows = AssetRelationship.query.filter(
+        AssetRelationship.sourceassetid == pc.assetid,
+        AssetRelationship.relationshiptypeid == controls.relationshiptypeid,
+        AssetRelationship.isactive.is_(True)).all()
+    return [Asset.query.get(r.targetassetid).assetnumber
+            for r in rows if r.targetassetid in toolids]
+
+
+def test_a_named_instrument_is_adopted_not_minted(client, db, rig, collector_key):
+    """measuringtool-id.txt names the instrument; nothing new is created."""
+    _tool(db, 'MT-6227')
+    assert _report(client, collector_key, 'FB7FB1V3',
+                   measuringtoolid='MT-6227').status_code in (200, 201)
+
+    assert _tools_controlled('FB7FB1V3') == ['MT-6227']
+    assert Asset.query.filter_by(assetnumber='FB7FB1V3-CMM').first() is None
+
+
+def test_a_tool_already_controlled_is_adopted(client, db, rig, collector_key):
+    """The legacy MT-#### case: linked by the import, invisible to the collector.
+
+    The link must pre-date the first report - that is the real sequence, and it
+    is the whole point. Reporting first would mint the twin before adoption ever
+    had a chance, which is exactly the bug.
+    """
+    from plugins.computers.models import Computer
+    tool = _tool(db, 'MT-6224')
+    computertype = AssetType.query.filter_by(assettype='computer').first()
+    pc = Asset(assetnumber='FJPX1GT3', assettypeid=computertype.assettypeid)
+    db.session.add(pc)
+    db.session.flush()
+    db.session.add(Computer(assetid=pc.assetid, hostname='FJPX1GT3'))
+    controls = RelationshipType.query.filter_by(relationshiptype='controls').first()
+    db.session.add(AssetRelationship(
+        sourceassetid=pc.assetid, targetassetid=tool.assetid,
+        relationshiptypeid=controls.relationshiptypeid, label=None))
+    db.session.commit()
+
+    assert _report(client, collector_key, 'FJPX1GT3').status_code in (200, 201)
+    assert _tools_controlled('FJPX1GT3') == ['MT-6224']
+    assert Asset.query.filter_by(assetnumber='FJPX1GT3-CMM').first() is None
+
+
+def test_the_reported_bay_id_is_adopted(client, db, rig, collector_key):
+    """The CMM case: cmmid.txt already reports CMM4, which is a real asset."""
+    _tool(db, 'CMM4')
+    assert _report(client, collector_key, 'FB7FB1V3',
+                   machinenumber='CMM4').status_code in (200, 201)
+
+    assert 'CMM4' in _tools_controlled('FB7FB1V3')
+    assert Asset.query.filter_by(assetnumber='FB7FB1V3-CMM').first() is None
+
+
+def test_minting_still_happens_when_nothing_exists(client, db, rig, collector_key):
+    """A genuinely new instrument has no identity to adopt - last resort."""
+    assert _report(client, collector_key, 'FNEWPC01').status_code in (200, 201)
+    assert _tools_controlled('FNEWPC01') == ['FNEWPC01-CMM']
+
+
+def test_a_named_instrument_that_does_not_exist_warns_and_mints_nothing_wrong(
+        client, db, rig, collector_key):
+    """A typo must not mint a phantom instrument nobody can account for."""
+    resp = _report(client, collector_key, 'FTYPO001', measuringtoolid='MT-NOPE')
+    assert resp.status_code in (200, 201)
+    body = resp.get_json()['data']
+    assert any('MT-NOPE' in w for w in body.get('warnings', []))
+
+
+def test_adoption_is_idempotent(client, db, rig, collector_key):
+    _tool(db, 'MT-6331')
+    for _ in range(3):
+        _report(client, collector_key, 'F9LQSDB4', measuringtoolid='MT-6331')
+    assert _tools_controlled('F9LQSDB4') == ['MT-6331']
+
+
+def test_a_named_instrument_supersedes_a_minted_twin(
+        client, db, rig, collector_key):
+    """The migration case: the twin already exists when the file names the real
+    one.
+
+    This is the state prod is actually in - 43 minted twins already linked - so
+    adoption that only works on a clean PC fixes nothing. The twin's link has to
+    be archived, or the PC reads as controlling two instruments.
+    """
+    _tool(db, 'MT-6227')
+    _report(client, collector_key, 'FB7FB1V3')
+    assert _tools_controlled('FB7FB1V3') == ['FB7FB1V3-CMM']
+
+    _report(client, collector_key, 'FB7FB1V3', measuringtoolid='MT-6227')
+    assert _tools_controlled('FB7FB1V3') == ['MT-6227']
+
+
+def test_a_named_id_that_is_not_a_measuring_tool_is_refused(
+        client, db, rig, collector_key):
+    """A machine number in measuringtool-id.txt must not link the machine.
+
+    The name resolving is not enough: linked under the measuring-tool label it
+    would read as an instrument everywhere downstream, on a row the machine sync
+    also owns.
+    """
+    machinetype = AssetType.query.filter_by(assettype='machine').first()
+    if machinetype is None:
+        machinetype = AssetType(assettype='machine')
+        db.session.add(machinetype)
+        db.session.flush()
+    db.session.add(Asset(assetnumber='2335', assettypeid=machinetype.assettypeid))
+    db.session.commit()
+
+    resp = _report(client, collector_key, 'FTYPO777', measuringtoolid='2335')
+    assert resp.status_code in (200, 201)
+    body = resp.get_json()['data']
+    assert any('2335' in w for w in body.get('warnings', []))
+    assert '2335' not in _all_controlled('FTYPO777')
+
+
+def test_a_cmm_reporting_its_bay_survives_a_second_cycle(
+        client, db, rig, collector_key):
+    """The instrument IS the reported bay, so both syncs want the same triple.
+
+    Adoption used to relabel the machine sync's row. The machine sync finds its
+    rows BY that label, so cycle 2 built a second row for the same
+    (pc, tool, controls) triple and MySQL rejected it on the unique constraint:
+    200, then 500 on every report after. The machine link must survive as a
+    machine link.
+    """
+    _tool(db, 'CMM4')
+    for _ in range(3):
+        resp = _report(client, collector_key, 'FB7FB1V3', machinenumber='CMM4')
+        assert resp.status_code in (200, 201)
+
+    assert _all_controlled('FB7FB1V3') == ['CMM4']
+    assert Asset.query.filter_by(assetnumber='FB7FB1V3-CMM').first() is None
+
+    pc = Asset.query.filter_by(assetnumber='FB7FB1V3').first()
+    tool = Asset.query.filter_by(assetnumber='CMM4').first()
+    rows = AssetRelationship.query.filter_by(
+        sourceassetid=pc.assetid, targetassetid=tool.assetid).all()
+    assert len(rows) == 1
+    assert rows[0].label == 'collector:machine'
+
+
+def test_a_reported_identifier_is_not_a_wildcard(client, db, rig, collector_key):
+    """`_` and `%` in a reported identifier must match themselves, nothing else.
+
+    These values come from a text file on a shopfloor PC. Resolved with ilike,
+    'MT-600_' matched MT-6001 and a bare '%' matched whatever active asset came
+    first - an identifier resolving to an asset it does not name, then linked to
+    as though it did.
+    """
+    _tool(db, 'MT-6001')
+    resp = _report(client, collector_key, 'FWILD001', measuringtoolid='MT-600_')
+    assert resp.status_code in (200, 201)
+    assert 'MT-6001' not in _tools_controlled('FWILD001')
+
+    resp = _report(client, collector_key, 'FWILD002', measuringtoolid='%')
+    assert resp.status_code in (200, 201)
+    assert 'MT-6001' not in _tools_controlled('FWILD002')
+
+
+def _all_controlled(hostname):
+    """Every active controls target, tool or not - the refusal cases need to see
+    a link the tool-only helper would filter out."""
+    pc = Asset.query.filter_by(assetnumber=hostname).first()
+    controls = RelationshipType.query.filter_by(relationshiptype='controls').first()
+    rows = AssetRelationship.query.filter(
+        AssetRelationship.sourceassetid == pc.assetid,
+        AssetRelationship.relationshiptypeid == controls.relationshiptypeid,
+        AssetRelationship.isactive.is_(True)).all()
+    targets = [r.targetassetid for r in rows]
+    if not targets:
+        return []
+    return sorted(a.assetnumber for a in
+                  Asset.query.filter(Asset.assetid.in_(targets)).all())

tests/test_plugins/test_collector_partmarker_links.py

@@ -0,0 +1,97 @@
+"""A part-marker PC keeps reporting after the first cycle.
+
+Both links a part-marker PC produces are rows in assetrelationships, which is
+unique on (source, target, type). Both were looked up by LABEL, so a row made by
+hand or by the legacy import was invisible and the insert violated the
+constraint; and the minted marker's asset number is derived from the PC, so it
+could already be taken. Either way the PC reported 200 on the cycle that minted
+the marker and 500 on every cycle after - the same shape as the CMM adoption bug
+in test_collector_measuringtool_adoption.py.
+"""
+
+import pytest
+
+from shopdb.core.models import Asset, AssetType
+from shopdb.core.models.relationship import RelationshipType, AssetRelationship
+
+KEY = 'partmarker-links-key'
+
+
+@pytest.fixture
+def collector_key(app):
+    old = app.config.get('COLLECTOR_API_KEY')
+    app.config['COLLECTOR_API_KEY'] = KEY
+    yield KEY
+    app.config['COLLECTOR_API_KEY'] = old
+
+
+@pytest.fixture
+def rig(db):
+    for name in ('computer', 'machine'):
+        if not AssetType.query.filter_by(assettype=name).first():
+            db.session.add(AssetType(assettype=name))
+    for name in ('controls', 'partof'):
+        if not RelationshipType.query.filter_by(relationshiptype=name).first():
+            db.session.add(RelationshipType(relationshiptype=name))
+    db.session.commit()
+
+
+def _report(client, key, hostname, **extra):
+    payload = {'hostname': hostname, 'pctype': 'gea-shopfloor-partmarker'}
+    payload.update(extra)
+    return client.post('/api/collector/computers', json=payload,
+                       headers={'X-API-Key': key})
+
+
+def _operation(db, assetnumber):
+    machinetype = AssetType.query.filter_by(assettype='machine').first()
+    asset = Asset(assetnumber=assetnumber, assettypeid=machinetype.assettypeid)
+    db.session.add(asset)
+    db.session.commit()
+    return asset
+
+
+def test_repeat_cycles_do_not_500(client, db, rig, collector_key):
+    _operation(db, '0613')
+    codes = [_report(client, collector_key, 'FMARK001', machinenumber='0613').status_code
+             for _ in range(3)]
+    assert codes == [codes[0]] * 3, codes
+    assert codes[0] in (200, 201), codes
+
+
+def test_a_partof_row_under_another_label_is_not_duplicated(
+        client, db, rig, collector_key):
+    """A partof row made by hand or by the legacy import carries no label."""
+    operation = _operation(db, '0615')
+    # First cycle mints the marker and links it.
+    assert _report(client, collector_key, 'FMARK002',
+                   machinenumber='0615').status_code in (200, 201)
+    marker = Asset.query.filter_by(assetnumber='FMARK002-PARTMARKER').first()
+    assert marker is not None
+    partof = RelationshipType.query.filter_by(relationshiptype='partof').first()
+    row = AssetRelationship.query.filter_by(
+        sourceassetid=marker.assetid, targetassetid=operation.assetid,
+        relationshiptypeid=partof.relationshiptypeid).first()
+    assert row is not None
+    # Somebody relabels it (or it came from the import unlabelled).
+    row.label = None
+    db.session.commit()
+
+    resp = _report(client, collector_key, 'FMARK002', machinenumber='0615')
+    assert resp.status_code in (200, 201), resp.status_code
+    rows = AssetRelationship.query.filter_by(
+        sourceassetid=marker.assetid, targetassetid=operation.assetid,
+        relationshiptypeid=partof.relationshiptypeid).all()
+    assert len(rows) == 1
+
+
+def test_a_taken_marker_asset_number_does_not_500(client, db, rig, collector_key):
+    """The minted number is derived from the PC, so it can already exist."""
+    machinetype = AssetType.query.filter_by(assettype='machine').first()
+    db.session.add(Asset(assetnumber='FMARK003-PARTMARKER',
+                         assettypeid=machinetype.assettypeid))
+    db.session.commit()
+    _operation(db, '0617')
+
+    resp = _report(client, collector_key, 'FMARK003', machinenumber='0617')
+    assert resp.status_code in (200, 201), resp.status_code

tests/test_plugins/test_geenforce_manifest.py

@@ -125,7 +125,7 @@ def _set_allowlist(app, value):
 def test_ip_allowlist_allows_without_token(client, db, app):
     # An allowlisted caller reaches the manifest with NO token (vault trust).
     _seed_and_publish(app)
-    _set_allowlist(app, '127.0.0.0/8, 10.134.48.0/23')  # test client is 127.0.0.1
+    _set_allowlist(app, '127.0.0.0/8, 192.0.2.0/24')  # test client is 127.0.0.1
     resp = client.get('/api/geenforce/manifest?pctype=gea-shopfloor-cmm')
     assert resp.status_code == 200, resp.data
     assert b'Alpha' in resp.data
@@ -152,9 +152,9 @@ def test_spoofed_forwarded_for_does_not_bypass_allowlist(client, db, app):
     # whose real IP (127.0.0.1) is NOT allowlisted must NOT gain token-less access
     # by forging X-Forwarded-For to an allowlisted address.
     _seed_and_publish(app)
-    _set_allowlist(app, '10.134.48.0/23')  # test client 127.0.0.1 is NOT in it
+    _set_allowlist(app, '192.0.2.0/24')  # test client 127.0.0.1 is NOT in it
     resp = client.get('/api/geenforce/manifest?pctype=gea-shopfloor-cmm',
-                      headers={'X-Forwarded-For': '10.134.48.10'})
+                      headers={'X-Forwarded-For': '192.0.2.10'})
     assert resp.status_code == 401, 'spoofed X-Forwarded-For bypassed the allowlist'
 
 

tests/test_plugins/test_geenforce_publish_gate.py

@@ -263,3 +263,42 @@ def test_reported_subtype_keeps_an_unknown_value_verbatim(db):
     stored = ManifestEnforcementReport.query.filter_by(
         hostname='WJKIOSKODD', iscurrent=True).one()
     assert stored.subtype == 'Lobbby'
+
+
+# -- what the gate compares against ------------------------------------------
+#
+# The gate used manifestversion as the library floor. That is the manifest's own
+# 'Version' field: for a share-imported manifest it happens to share the
+# library's numbering, but a code-authored scope sets it freely -
+# seed_display_scope writes '2.0' - so the fleet was compared against a number
+# that says nothing about library features, and the gate passed on the very
+# scope that most needed it.
+
+def test_a_declared_minlibversion_is_what_the_fleet_is_judged_against(db):
+    scope = _scope(manifestversion='2.0')
+    scope.minlibversion = '2.6'
+    db.session.flush()
+    assert service.required_libversion(scope) == '2.6'
+
+    _report('KIOSK01', '2.4')
+    with pytest.raises(service.LibVersionTooOldError):
+        service.publish_scope('gea-shopfloor-cmm', 'runtime')
+
+
+def test_the_floor_is_derived_from_the_features_the_manifest_uses(db):
+    """A scope written before minlibversion existed is still gated on what it
+    contains: a version-gated entry needs lib 2.6, whatever its own Version
+    field claims."""
+    from plugins.geenforce.models import ManifestEntry
+
+    scope = _scope(manifestversion='2.0')
+    scope.entries.append(ManifestEntry(
+        name='PC-DMIS 2023.2', entrytype='MSI', sortorder=0,
+        cmmversion='2023'))
+    db.session.flush()
+    assert service.required_libversion(scope) == '2.6'
+
+
+def test_a_scope_using_nothing_special_falls_back_to_its_own_version(db):
+    scope = _scope(manifestversion='2.4')
+    assert service.required_libversion(scope) == '2.4'

tests/test_plugins/test_network_assetnumber_generation.py

@@ -0,0 +1,108 @@
+"""Asset numbers generated from the device type's prefix.
+
+Every network device on this fleet already followed one convention applied by
+hand - AP-<name>, SW-<name>, SVR-<name>, IDF-<name>, 45 records, no exceptions.
+Generation codifies it so the same value is not typed twice, and must reproduce
+those records exactly rather than inventing a new scheme.
+"""
+
+import pytest
+
+from shopdb.extensions import db
+from shopdb.core.models import AssetType
+from plugins.network.models import NetworkDeviceType
+
+
+def _devicetype(db, name, prefix):
+    """A device type, with the core asset type the create path needs present."""
+    if not AssetType.query.filter_by(assettype='network_device').first():
+        db.session.add(AssetType(assettype='network_device'))
+    devicetype = NetworkDeviceType(networkdevicetype=name, prefix=prefix)
+    db.session.add(devicetype)
+    db.session.commit()
+    return devicetype
+from plugins.network.services.assetnumbers import generate, sanitize
+
+
+@pytest.mark.parametrize('prefix,name,expected', [
+    ('AP', 'AIRwdMUSwestj02', 'AP-AIRwdMUSwestj02'),
+    ('SW', 'AIRsdMUSwestj01', 'SW-AIRsdMUSwestj01'),
+    ('SVR', 'AVEWP4546V01', 'SVR-AVEWP4546V01'),
+])
+def test_reproduces_the_existing_convention(prefix, name, expected):
+    assert generate(prefix, name) == expected
+
+
+def test_a_type_with_no_prefix_gives_the_bare_name():
+    """Nullable on purpose - nobody invents a prefix for a type that wants none."""
+    assert generate(None, 'Bare Name') == 'Bare-Name'
+    assert generate('', 'Bare Name') == 'Bare-Name'
+
+
+def test_an_already_prefixed_name_is_not_stacked():
+    """IDF-03 under type IDF is IDF-03, never IDF-IDF-03. Real row on this data."""
+    assert generate('IDF', 'IDF-03') == 'IDF-03'
+    assert generate('idf', 'idf-03') == 'idf-03'
+
+
+def test_characters_that_break_a_business_key_are_stripped():
+    """assetnumber lands in URLs and in joins. The existing IDF-Telco-Demarc-#1
+    row shows a '#' reaching a unique key, which is what this prevents."""
+    assert generate('IDF', 'Telco Demarc #1') == 'IDF-Telco-Demarc-1'
+    assert generate('APNL', 'Door 4 / West') == 'APNL-Door-4-West'
+    assert sanitize('  spaced  out  ') == 'spaced-out'
+
+
+def test_nothing_to_build_from_returns_empty():
+    """So the caller demands an explicit value instead of inventing one."""
+    assert generate('AP', '') == ''
+    assert generate('AP', None) == ''
+
+
+def test_create_generates_when_assetnumber_is_blank(client, db, auth_headers):
+    devicetype = _devicetype(db, 'Access Panel', 'APNL')
+
+    resp = client.post('/api/network', json={
+        'name': 'Dock Door 4',
+        'networkdevicetypeid': devicetype.networkdevicetypeid,
+    }, headers=auth_headers)
+    assert resp.status_code == 201, resp.get_json()
+    assert resp.get_json()['data']['assetnumber'] == 'APNL-Dock-Door-4'
+
+
+def test_an_explicit_assetnumber_always_wins(client, db, auth_headers):
+    """Adopt an external identifier where one exists - a vendor tag beats a
+    generated value, which is why generation only fills a BLANK field."""
+    devicetype = _devicetype(db, 'Access Panel 2', 'APNL')
+
+    resp = client.post('/api/network', json={
+        'assetnumber': 'VENDORTAG-99',
+        'name': 'Dock Door 5',
+        'networkdevicetypeid': devicetype.networkdevicetypeid,
+    }, headers=auth_headers)
+    assert resp.status_code == 201, resp.get_json()
+    assert resp.get_json()['data']['assetnumber'] == 'VENDORTAG-99'
+
+
+def test_no_name_and_no_assetnumber_is_refused(client, db, auth_headers):
+    devicetype = _devicetype(db, 'Access Panel 3', 'APNL')
+
+    resp = client.post('/api/network', json={
+        'networkdevicetypeid': devicetype.networkdevicetypeid,
+    }, headers=auth_headers)
+    assert resp.status_code == 400
+    assert 'assetnumber is required' in resp.get_json()['data']['error']['message']
+
+
+def test_prefix_round_trips_through_the_type_api(client, db, auth_headers):
+    created = client.post('/api/network/types', json={
+        'networkdevicetype': 'Door Controller', 'prefix': 'DOOR',
+    }, headers=auth_headers)
+    assert created.status_code == 201, created.get_json()
+    assert created.get_json()['data']['prefix'] == 'DOOR'
+
+    typeid = created.get_json()['data']['networkdevicetypeid']
+    updated = client.put(f'/api/network/types/{typeid}', json={'prefix': 'DR'},
+                         headers=auth_headers)
+    assert updated.status_code == 200, updated.get_json()
+    assert updated.get_json()['data']['prefix'] == 'DR'