v3.22.3: the mismatch on_fail, the self-healing move, the pruning reconciliation

Three test-management issues:

- an expectation whose command type did not match the pending command
  hard-coded level 'error', so on_fail "warning" tolerated "no command
  pending" but not "a different command is pending".  The check now honours
  the step's on_fail - the tolerant form for "the applet either asks X or
  does Y" (the mismatched command is still fetched and answered).
- moving a script between suites could answer 500 and leave two owners:
  _test_script_move detached only from the suite named by the script's
  suite_id and appended to the target unconditionally, so a duplicate
  listing or a stale suite_id (a hand edit, an interrupted move) collided
  on the attach.  It now detaches from every suite that lists the script,
  takes the on_fail from the explicit owner (suite_id if that suite lists
  it, else the first lister), updates the script first (a refused
  validation changes nothing) and re-attaches with a prune; the same-suite
  no-op only applies when the target really is the only lister.
- reconcile_scripts_to_suites() now prunes duplicate listings to the
  explicit owner (it only adopted before), so those states self-heal at
  startup and after a bundle import; the "Imported scripts" suite is only
  created when there are orphans.

Docs: the bundle-import note (importing into the source store duplicates
the suite and its scripts - the round-trip is not idempotent) and the
expectation on_fail clarification.
Tests: 839 frontend + 818 Python; sw cache simple-v407.

docs/api.md

@@ -864,10 +864,17 @@ collision gets a fresh uuid and the suites' references are remapped; then the
 suites are imported (`replace` wipes the suite store first); an unresolvable
 reference is reported in `errors` and the suite is skipped.  After the import the
 ownership invariant is reconciled: a script the imported suites list adopts
-its listing suite, and a script no suite lists any more — the `replace` mode
+its listing suite, a script listed by several suites keeps its explicit owner
-wipes the suites that owned them — is **adopted into "Imported scripts"**
+and the other listings are pruned, and a script no suite lists any more — the
-instead of becoming invisible (the same reconciliation runs at startup for a
+`replace` mode wipes the suites that owned them — is **adopted into "Imported
-v3.21.0 store).
+scripts"** instead of becoming invisible (the same reconciliation runs at
+startup for a v3.21.0 store).
+
+The import always **merges**: an id already in the store gets a fresh uuid
+(scripts) / a fresh suite id, so importing a bundle **into the store it came
+from duplicates the suite and its scripts** — the round-trip is not
+idempotent.  Restore a bundle into a fresh store, or wipe the target first
+(`mode: "replace"` for the suites).
 
 ### `POST /api/test/run` (single script or suite)
 

docs/test-scripts.md

@@ -28,7 +28,10 @@ suite run's report (the PWA's report view, exportable as JSON or Markdown)
 carries every member's step table, failing notes and run-log excerpt, plus the
 summary (pass/warn/fail/skipped, wall time, the SCP80 counters before/after
 per keyset and the card-session flag).  The suites live in
-`~/.pysim-simple-server/test_suites.json` (`--test-suites`).
+`~/.pysim-simple-server/test_suites.json` (`--test-suites`).  A bundle
+import always merges: importing a suite export **into the store it came
+from duplicates the suite and its scripts** (an id already present gets a
+fresh uuid) - restore into a fresh store, or wipe the target first.
 
 **Format**: server ≥ 3.15.0 (`kvn`, `por` and `files` checks), scripts stored
 server-side since 3.16.0, `menu-select` by text and the spec-order TERMINAL
@@ -385,9 +388,11 @@ runner never polls by itself.  `command` is a name (as shown in the proactive
 log) or a hex type; `ANY`/`*` skips the type check.  `qualifier` is the
 command-details byte 3 (one byte, exact/mask).
 
-When nothing is pending the step fails at its own `on_fail` level (the
+When nothing is pending - or **a different command** is pending - the step
-default stops the run; `on_fail: "warning"` warns and continues - the
+fails at its own `on_fail` level (the default stops the run;
-tolerant consume-if-pending form, v3.21.0).  "Add a `status` action" applies
+`on_fail: "warning"` warns and continues: the tolerant consume-if-pending
+form, and the way to express "the applet either asks X or does Y" - the
+mismatched command is still fetched and answered with the step's `respond`).  "Add a `status` action" applies
 when the card delivers on poll: `status` with `attempts > 1` polls until a
 command appears; it cannot make one appear when the card has none.
 

frontend/index.html

@@ -1903,7 +1903,7 @@
 // ===== Version =====
 // Single source of truth for the PWA version: shown in the header and used
 // by the server version check in pysimConnect().
-const SIMPLE_VERSION = '3.22.2';
+const SIMPLE_VERSION = '3.22.3';
 document.getElementById('app-version').textContent = 'v' + SIMPLE_VERSION;
 
 // ===== Tab switching =====

frontend/sw.js

@@ -1,4 +1,4 @@
-const CACHE = 'simple-v406';
+const CACHE = 'simple-v407';
 const URLS = [
   'index.html',
   'help.html',

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "pysim-simple-server"
-version = "3.22.2"
+version = "3.22.3"
 description = "HTTP REST server wrapping pysim for the SIMple PWA"
 requires-python = ">=3.8"
 # pysim is a git-only dependency installed explicitly by setup.bat/setup.sh.

pysim_simple_server/server.py

@@ -36,7 +36,7 @@ from osmocom.tlv import BER_TLV_IE
 from osmocom.utils import rpad
 
 
-VERSION = '3.22.2'
+VERSION = '3.22.3'
 
 MAX_ENVELOPE_SEGMENTS = 5  # max SMS segments for outgoing C-APDU in ENVELOPE
 
@@ -5987,30 +5987,52 @@ def _test_script_detach(suites, cur):
 
 
 def _test_script_move(store, suites, cur, target_id):
-    """Move a script to another suite: the source detaches, the target appends
+    """Move a script to another suite: the target appends it as a member
-    it as a member (keeping the on_fail policy it had).  Both stores are
+    (keeping the on_fail policy it had) and **every** suite that lists it
-    updated in the one request.  Returns ``(entry, error, status)``."""
+    detaches.  The script's own ``suite_id`` names the owner when that suite
+    lists it, otherwise the first lister does.  A duplicate listing or a
+    stale suite_id (a hand edit, an interrupted move) is repaired by the
+    move instead of answering 500 and leaving two owners (v3.22.3 review).
+    Both stores are updated in the one request; returns
+    ``(entry, error, status)``."""
     target = suites.get(target_id)
     if target is None:
         return None, 'unknown target suite', 400
-    if target['id'] == (cur.get('suite_id') or ''):
+    listings = [s for s in suites.list()
+                if any(e['script_id'] == cur['id'] for e in s['scripts'])]
+    if target['id'] == (cur.get('suite_id') or '') and [s['id'] for s in listings] == [target['id']]:
+        # the exact no-op: the script already belongs to the target and only
+        # the target lists it
         return cur, None, 200
-    source = suites.get(cur.get('suite_id')) if cur.get('suite_id') else None
+    owner = next((s for s in listings if s['id'] == (cur.get('suite_id') or '')), None)
+    if owner is None and listings:
+        owner = listings[0]
     on_fail = 'stop'
-    if source is not None:
+    if owner is not None:
-        entry = next((e for e in source['scripts'] if e['script_id'] == cur['id']), None)
+        entry = next((e for e in owner['scripts'] if e['script_id'] == cur['id']), None)
         if entry is not None:
             on_fail = entry['on_fail']
-        _test_script_detach(suites, cur)
+    # The script first: its validation refuses before any listing changes, so
+    # a failed move never leaves the script unlisted.
     try:
         new = store.update(cur['id'], {'suite_id': target['id']})
     except (test_scripts.TestScriptError, OSError) as e:
         return None, 'cannot move the script: %s' % e, 500
     if new is None:
         return None, 'unknown test script id', 404
+    for s in listings:
+        entries = [e for e in s['scripts'] if e['script_id'] != cur['id']]
+        if entries == s['scripts']:
+            continue
         try:
-        entries = list(target['scripts']) + [
+            suites.update(s['id'], {'scripts': entries})
-            {'script_id': cur['id'], 'role': 'member', 'on_fail': on_fail}]
+        except (test_suites.TestSuiteError, OSError) as e:
+            sys.stderr.write('TESTSUITES: cannot detach %s from "%s": %s\n'
+                             % (cur['id'], s['name'], e))
+    try:
+        fresh = suites.get(target['id']) or target
+        entries = [e for e in fresh['scripts'] if e['script_id'] != cur['id']]
+        entries.append({'script_id': cur['id'], 'role': 'member', 'on_fail': on_fail})
         suites.update(target['id'], {'scripts': entries})
     except (test_suites.TestSuiteError, OSError) as e:
         return None, 'cannot attach to the target suite: %s' % e, 500
@@ -6021,6 +6043,10 @@ def reconcile_scripts_to_suites(script_store, suite_store):
     """Make the ownership invariant hold again: every script belongs to
     exactly one suite and that suite lists it.
 
+    - a script **listed by several suites** (a hand edit or an interrupted
+      move) stays with its explicit owner - the suite its ``suite_id`` names
+      when that suite lists it, else the first lister - and the other
+      listings are pruned;
     - a script no suite lists (a v3.21.0 store, a suites import with
       ``mode: "replace"``, a hand edit or an interrupted move) is attached to
       the auto-created "Imported scripts" suite;
@@ -6032,6 +6058,31 @@ def reconcile_scripts_to_suites(script_store, suite_store):
     if script_store is None or suite_store is None:
         return 0
     suites = suite_store.list()
+    listings = {}
+    for s in suites:
+        for e in s['scripts']:
+            listings.setdefault(e['script_id'], []).append(s['id'])
+    by_id = {s['id']: s for s in script_store.list()}
+    pruned = 0
+    for sid, owners in listings.items():
+        if len(owners) < 2:
+            continue
+        explicit = (by_id.get(sid) or {}).get('suite_id') or None
+        keep = explicit if explicit in owners else owners[0]
+        for s in suites:
+            if s['id'] == keep or s['id'] not in owners:
+                continue
+            entries = [e for e in s['scripts'] if e['script_id'] != sid]
+            try:
+                suite_store.update(s['id'], {'scripts': entries})
+                s['scripts'] = entries
+                pruned += 1
+                sys.stderr.write('TESTSUITES: pruned %s from "%s" (owner "%s")\n'
+                                 % (sid, s['name'], keep))
+            except (test_suites.TestSuiteError, OSError) as e:
+                sys.stderr.write('TESTSUITES: cannot prune %s from "%s": %s\n'
+                                 % (sid, s['name'], e))
+    # The owner map after the pruning (the member list is authoritative).
     owner = {}
     for s in suites:
         for e in s['scripts']:
@@ -6053,23 +6104,25 @@ def reconcile_scripts_to_suites(script_store, suite_store):
                                  % (script['id'], e))
             continue
         orphans.append(script)
-    if not orphans and not adopted:
+    if not orphans and not adopted and not pruned:
         return 0
+    attached = 0
+    if orphans:
         suite = suite_store.find_by_name('Imported scripts')
         if suite is None:
             suite = suite_store.add({'name': 'Imported scripts'})
         entries = list(suite['scripts'])
         known = {e['script_id'] for e in entries}
-    attached = 0
         for s in orphans:
             try:
                 script_store.update(s['id'], {'suite_id': suite['id']})
             except (test_scripts.TestScriptError, OSError) as e:
-            # The member list is authoritative: list the script anyway.  The
+                # The member list is authoritative: list the script anyway.
-            # store keeps a script that no longer validates (a tightened rule
+                # The store keeps a script that no longer validates (a
-            # must not destroy it) and its suite_id write is refused by the
+                # tightened rule must not destroy it) and its suite_id write
-            # same validator - without the listing the script would be
+                # is refused by the same validator - without the listing the
-            # invisible in the PWA and could never be fixed in the editor.
+                # script would be invisible in the PWA and could never be
+                # fixed in the editor.
                 sys.stderr.write('TESTSUITES: cannot set the suite id of %s '
                                  '(listed in "%s" so the editor can fix it): %s\n'
                                  % (s['id'], suite['name'], e))
@@ -6082,11 +6135,12 @@ def reconcile_scripts_to_suites(script_store, suite_store):
                 suite_store.update(suite['id'], {'scripts': entries})
             except (test_suites.TestSuiteError, OSError) as e:
                 sys.stderr.write('TESTSUITES: cannot fill "%s": %s\n' % (suite['name'], e))
-    if attached or adopted:
+    if attached or adopted or pruned:
-        sys.stderr.write('TESTSUITES: reconciled %d script(s) (attached to "%s": %d, '
+        sys.stderr.write('TESTSUITES: reconciled %d script(s) (attached to '
-                         'adopted by their suite: %d)\n'
+                         '"Imported scripts": %d, adopted by their suite: %d, '
-                         % (attached + adopted, suite['name'], attached, adopted))
+                         'pruned duplicate listings: %d)\n'
-    return attached + adopted
+                         % (attached + adopted + pruned, attached, adopted, pruned))
+    return attached + adopted + pruned
 
 
 def migrate_scripts_to_suites(script_store, suite_store):
@@ -6849,9 +6903,12 @@ def _test_run_expect(server, step, pending, ctx=None):
         checks.append(_test_check_result('Command', True, step['command'].get('name') or type_name,
                                          '%s (0x%02X)' % (type_name, cmd_type), 'ok'))
     else:
+        # A different command pending is a check failure like any other: it
+        # honours the step's on_fail, so on_fail "warning" expresses "the
+        # applet either asks X or Y" (the command is still fetched/answered).
         checks.append(_test_check_result(
             'Command', False, step['command'].get('name') or '0x%02X' % want_type,
-            '%s (0x%02X)' % (type_name, cmd_type), 'error'))
+            '%s (0x%02X)' % (type_name, cmd_type), step['on_fail']))
     if step.get('qualifier'):
         actual_q = '%02X' % cmd_qual if cmd_qual is not None else None
         checks.append(_test_check_result('Qualifier',

tests/test_fetch_routes.py

@@ -1360,6 +1360,63 @@ class TestScriptStoreHttpTests(unittest.TestCase):
         self.assertEqual(status, 400, resp)
         self.assertIn('member set changes', resp['error'])
 
+    def test_move_repairs_a_duplicate_listing(self):
+        # v3.22.3: the move detaches the script from EVERY suite that lists
+        # it and re-attaches to the target - an inconsistent state (a partial
+        # move, a stale suite_id) must not answer 500 or leave two owners.
+        a = self._suite(name='a')
+        b = self._suite(name='b')
+        status, resp = self._post('/api/test/scripts', self._script(suite_id=a['id']))
+        self.assertEqual(status, 200, resp)
+        sid = resp['script']['id']
+        # b lists it too: the state a partial move leaves behind
+        self.suites.update(b['id'], {'scripts': [
+            {'script_id': sid, 'role': 'member', 'on_fail': 'continue'}]})
+        status, resp = self._post('/api/test/scripts/update', {
+            'id': sid, 'script': {'name': 'demo', 'suite_id': b['id']}})
+        self.assertEqual(status, 200, resp)
+        self.assertTrue(resp.get('moved'))
+        self.assertEqual(self.suites.get(a['id'])['scripts'], [])
+        self.assertEqual([e['script_id'] for e in self.suites.get(b['id'])['scripts']],
+                         [sid])
+        self.assertEqual(self.store.get(sid)['suite_id'], b['id'])
+        # the on_fail comes from the explicit owner (the suite_id named a)
+        self.assertEqual(self.suites.get(b['id'])['scripts'][0]['on_fail'], 'stop')
+
+    def test_move_from_a_stale_owner_repairs_the_listing(self):
+        a = self._suite(name='a')
+        b = self._suite(name='b')
+        status, resp = self._post('/api/test/scripts', self._script(suite_id=a['id']))
+        self.assertEqual(status, 200, resp)
+        sid = resp['script']['id']
+        # the suite_id points at a missing suite while a still lists it
+        self.store.update(sid, {'suite_id': 'f' * 32})
+        status, resp = self._post('/api/test/scripts/update', {
+            'id': sid, 'script': {'name': 'demo', 'suite_id': b['id']}})
+        self.assertEqual(status, 200, resp)
+        self.assertEqual(self.suites.get(a['id'])['scripts'], [])
+        self.assertEqual([e['script_id'] for e in self.suites.get(b['id'])['scripts']],
+                         [sid])
+        self.assertEqual(self.store.get(sid)['suite_id'], b['id'])
+
+    def test_move_to_the_same_suite_prunes_a_duplicate(self):
+        a = self._suite(name='a')
+        b = self._suite(name='b')
+        status, resp = self._post('/api/test/scripts', self._script(suite_id=b['id']))
+        self.assertEqual(status, 200, resp)
+        sid = resp['script']['id']
+        # a also lists it; the explicit suite_id names b, whose on_fail stays
+        self.suites.update(a['id'], {'scripts': [
+            {'script_id': sid, 'role': 'member', 'on_fail': 'stop'}]})
+        self.suites.update(b['id'], {'scripts': [
+            {'script_id': sid, 'role': 'member', 'on_fail': 'continue'}]})
+        status, resp = self._post('/api/test/scripts/move', {'id': sid, 'suite_id': b['id']})
+        self.assertEqual(status, 200, resp)
+        self.assertEqual(self.suites.get(a['id'])['scripts'], [])
+        self.assertEqual([e['script_id'] for e in self.suites.get(b['id'])['scripts']],
+                         [sid])
+        self.assertEqual(self.suites.get(b['id'])['scripts'][0]['on_fail'], 'continue')
+
     def test_move_and_copy_between_suites(self):
         a = self._suite(name='a')
         b = self._suite(name='b')

tests/test_test_suites.py

@@ -222,6 +222,50 @@ class MigrationTests(unittest.TestCase):
             # idempotent: nothing left to attach
             self.assertEqual(srv.migrate_scripts_to_suites(scripts, suites), 0)
 
+    def test_a_duplicate_listing_is_pruned_to_the_explicit_owner(self):
+        # v3.22.3: the reconciliation keeps a script in the suite its
+        # suite_id names and prunes the other listings (a hand edit or an
+        # interrupted move must not leave two owners).
+        from pysim_simple_server import server as srv
+        from pysim_simple_server import test_scripts
+        with tempfile.TemporaryDirectory() as tmp:
+            scripts = test_scripts.TestScriptStore(pathlib.Path(tmp) / 's.json')
+            suites = test_suites.TestSuiteStore(pathlib.Path(tmp) / 'u.json')
+            step = {'type': 'action', 'kind': 'status', 'params': {'attempts': 1}}
+            a = suites.add({'name': 'A'})
+            b = suites.add({'name': 'B'})
+            s = scripts.add({'name': 'x', 'suite_id': b['id'], 'steps': [step]})
+            for su in (a, b):
+                suites.update(su['id'], {'scripts': [
+                    {'script_id': s['id'], 'role': 'member', 'on_fail': 'stop'}]})
+            self.assertEqual(srv.reconcile_scripts_to_suites(scripts, suites), 1)
+            self.assertEqual(suites.get(a['id'])['scripts'], [])
+            self.assertEqual([e['script_id'] for e in suites.get(b['id'])['scripts']],
+                             [s['id']])
+            self.assertEqual(scripts.get(s['id'])['suite_id'], b['id'])
+            # idempotent
+            self.assertEqual(srv.reconcile_scripts_to_suites(scripts, suites), 0)
+
+    def test_a_duplicate_listing_with_a_stale_owner_keeps_the_first_lister(self):
+        from pysim_simple_server import server as srv
+        from pysim_simple_server import test_scripts
+        with tempfile.TemporaryDirectory() as tmp:
+            scripts = test_scripts.TestScriptStore(pathlib.Path(tmp) / 's.json')
+            suites = test_suites.TestSuiteStore(pathlib.Path(tmp) / 'u.json')
+            step = {'type': 'action', 'kind': 'status', 'params': {'attempts': 1}}
+            a = suites.add({'name': 'A'})
+            b = suites.add({'name': 'B'})
+            s = scripts.add({'name': 'x', 'suite_id': 'f' * 32, 'steps': [step]})
+            for su in (a, b):
+                suites.update(su['id'], {'scripts': [
+                    {'script_id': s['id'], 'role': 'member', 'on_fail': 'stop'}]})
+            # the prune of B plus the adoption of the first lister A
+            self.assertEqual(srv.reconcile_scripts_to_suites(scripts, suites), 2)
+            self.assertEqual([e['script_id'] for e in suites.get(a['id'])['scripts']],
+                             [s['id']])
+            self.assertEqual(suites.get(b['id'])['scripts'], [])
+            self.assertEqual(scripts.get(s['id'])['suite_id'], a['id'])
+
     def test_an_invalid_script_is_still_listed_by_the_reconciliation(self):
         # The script store keeps an entry that no longer validates (a
         # tightened rule must not destroy it).  Its suite_id write is refused

tests/test_testscript.py

@@ -563,6 +563,39 @@ class TestRunnerDialogue(RunnerTestCase):
         self.assertIn('no proactive command pending',
                       run['steps'][0]['checks'][0]['detail'])
 
+    def test_a_command_mismatch_honours_on_fail(self):
+        # v3.22.3: a different command pending is a check failure at the
+        # step's on_fail level, like every other expectation check - so
+        # on_fail "warning" expresses "the applet either asks X or does Y"
+        # (the mismatched command is still fetched and answered).
+        scc = FakeScc()
+        scc.push('80F2', '', '9102')          # STATUS: DISPLAY TEXT pending
+        scc.push('8012', DISPLAY_TEXT_CMD, '9000')
+        scc.push('8014', '', '9000')
+        run = self.run_script(FakeServer(scc), [
+            {'type': 'action', 'kind': 'status', 'params': {'attempts': 2}},
+            {'type': 'expect', 'command': 'SEND SHORT MESSAGE',
+             'respond': {'result': 'ok'}, 'on_fail': 'warning'},
+        ])
+        self.assertEqual(run['status'], 'warning', run['steps'])
+        check = run['steps'][1]['checks'][0]
+        self.assertEqual(check['label'], 'Command')
+        self.assertFalse(check['ok'])
+        self.assertEqual(check['level'], 'warning')
+        self.assertIn('DISPLAY TEXT', check['actual'])
+        self.assertTrue(run['steps'][1]['sent'].startswith('TR '))
+        # the default (error) still stops the run
+        scc2 = FakeScc()
+        scc2.push('80F2', '', '9102')
+        scc2.push('8012', DISPLAY_TEXT_CMD, '9000')
+        scc2.push('8014', '', '9000')
+        run2 = self.run_script(FakeServer(scc2), [
+            {'type': 'action', 'kind': 'status', 'params': {'attempts': 2}},
+            {'type': 'expect', 'command': 'SEND SHORT MESSAGE',
+             'respond': {'result': 'ok'}},
+        ])
+        self.assertEqual(run2['status'], 'error')
+
     def test_text_mismatch_fails_the_expectation(self):
         scc = FakeScc()
         scc.push('80C2', '', '9105')