From b2cb42caf51e8495780e5892b56a64a725bb4d37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=90=D0=BD=D1=82=D0=BE=D0=BD=20=D0=A2=D1=80=D0=BE=D1=88?= =?UTF-8?q?=D0=B8=D0=BD?= Date: Tue, 6 Oct 2026 01:15:20 +0300 Subject: [PATCH] v3.22.0 (review fixes): the ownership reconciliation and the import guards The suite/script ownership invariant could break silently: a suites import with mode "replace" wiped the suites while their scripts stayed behind (suite_id pointing at gone suites - invisible in the UI), a scripts import with mode "replace" left every suite's member list dangling, and a script whose suite_id disagreed with its listing suite stayed inconsistent. - server: reconcile_scripts_to_suites() repairs the invariant - a script a suite lists adopts that suite (the member list is authoritative), a script no suite lists joins "Imported scripts"; migrate_scripts_to_suites() delegates to it (the startup migration is the same pass) and the suites import runs it after loading, so a "replace" adopts the wiped suites' scripts instead of losing them. - the scripts import refuses mode "replace" while any suite still lists scripts (400 naming the dangling references; merge is the normal path). - nits from the review: a legacy numeric `event` is no longer parsed as hex in the step summary/error check (0x0A displayed as 0x10), and an integral float memory quota (a JSON client's 200.0) is accepted. - tests: the reconciliation (missing suite, listed-but-mismatched, idempotent), the scripts-replace guard (refused/allowed), the suites-replace adoption, the float quota, the numeric-event summary. Tests: 838 frontend + 800 Python; sw cache simple-v402. --- docs/api.md | 12 +++- frontend/index.html | 7 ++- frontend/sw.js | 2 +- frontend/tests/testscripts.test.js | 7 ++- pysim_simple_server/server.py | 96 +++++++++++++++++++++++------- tests/test_fetch_routes.py | 39 ++++++++++++ tests/test_ota_helpers.py | 3 + tests/test_test_suites.py | 27 +++++++++ 8 files changed, 166 insertions(+), 27 deletions(-) diff --git a/docs/api.md b/docs/api.md index 25ac96a..d1e0965 100644 --- a/docs/api.md +++ b/docs/api.md @@ -820,7 +820,10 @@ policy) and `POST /api/test/scripts/copy` (`{id, suite_id, name?}`) clones it into the target. `POST /api/test/scripts/import` takes `{scripts: [...], mode: "merge"|"replace", suite_id?}` (the old localStorage export shape; invalid entries are reported in `errors` instead of failing the -whole import; `suite_id` attaches the imported scripts to that suite). +whole import; `suite_id` attaches the imported scripts to that suite). A +`replace` is **refused while any suite still lists scripts** — the wipe would +leave dangling references; delete those scripts first, or import with +`merge`. ### `GET /api/test/suites` and the suite store @@ -852,7 +855,12 @@ removes it - refused while the suite still holds scripts - and `{suites: [...], scripts: [...], mode: "merge"|"replace"}`: the scripts are imported first (ids preserved; a collision gets a fresh uuid and the suites' references are remapped), then the suites; an unresolvable reference is -reported in `errors` and the suite is skipped. +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 +wipes the suites that owned them — is **adopted into "Imported scripts"** +instead of becoming invisible (the same reconciliation runs at startup for a +v3.21.0 store). ### `POST /api/test/run` (single script or suite) diff --git a/frontend/index.html b/frontend/index.html index 4588a23..84688a7 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -7197,7 +7197,8 @@ function testStepSummary(step) { : 'item=' + p.item_id); } else if (step.kind === 'event') { - const code = parseInt(p.event, 16); + // a legacy numeric event is the code itself; the editor stores hex + const code = (typeof p.event === 'number') ? p.event : parseInt(p.event, 16); s = 'EVENT ' + (EVENT_NAMES[code] || ('0x' + String(p.event || '?'))); if (p.src) s += ' src=' + p.src; } @@ -8290,7 +8291,9 @@ function testStepFormError() { if (problem) return problem; } if (step.kind === 'event') { - const code = parseInt(String((step.params || {}).event || ''), 16); + const rawEvent = (step.params || {}).event; + const code = (typeof rawEvent === 'number') ? rawEvent + : parseInt(String(rawEvent || ''), 16); const cfg = EVENT_FORMS[code]; if (!cfg) return t('No form for this event - use the raw Envelope action.'); const bad = eventFormPatternError(cfg, (step.params || {}).fields || {}); diff --git a/frontend/sw.js b/frontend/sw.js index 54545e8..7c31ed6 100644 --- a/frontend/sw.js +++ b/frontend/sw.js @@ -1,4 +1,4 @@ -const CACHE = 'simple-v401'; +const CACHE = 'simple-v402'; const URLS = [ 'index.html', 'help.html', diff --git a/frontend/tests/testscripts.test.js b/frontend/tests/testscripts.test.js index 0cf927e..bf96a57 100644 --- a/frontend/tests/testscripts.test.js +++ b/frontend/tests/testscripts.test.js @@ -465,12 +465,17 @@ test('the event step collects the form values, the built data and the source', ( fakeForm({ 'test-step-kind': 'event', 'test-f-evtype': '00', 'test-f-ev-ti': 'ZZZ', 'test-f-sw': '', 'test-f-cdata': '', 'test-f-fail': 'error' }); assert.match(testStepFormError(), /Invalid/); - // the summary names the event and its source + // the summary names the event and its source; a legacy numeric event is + // the code itself (the editor stores the hex string form) assert.strictEqual(testStepSummary({ type: 'action', kind: 'event', params: { event: '03', fields: {} } }), 'EVENT Location status'); assert.strictEqual(testStepSummary({ type: 'action', kind: 'event', params: { event: '00', data: '1C0101', src: '83' } }), 'EVENT MT call src=83'); + assert.strictEqual(testStepSummary({ type: 'action', kind: 'event', + params: { event: 10 } }), 'EVENT ' + EVENT_NAMES[10]); + assert.strictEqual(testStepSummary({ type: 'action', kind: 'event', + params: { event: '0A' } }), 'EVENT ' + EVENT_NAMES[10]); }); test('the shared event-form helpers render, prefill and collect', () => { diff --git a/pysim_simple_server/server.py b/pysim_simple_server/server.py index 195298d..4e8e01f 100644 --- a/pysim_simple_server/server.py +++ b/pysim_simple_server/server.py @@ -754,9 +754,13 @@ def _single_ef_value(hex_str): def _quota_tlv(tag, value): """A C7/C8 memory-quota TLV: the value is a 2-byte integer up to 32767 and a 4-byte integer above (GP Card Spec 9.7 / Table 11-49; pySim's - ``StripHeaderAdapter(GreedyBytes, 4, steps=[2,4])``).""" + ``StripHeaderAdapter(GreedyBytes, 4, steps=[2,4])``). An integral float + is accepted (a JSON client may send ``200.0``).""" try: - v = int(str(value).strip(), 10) + num = float(str(value).strip()) + if not num.is_integer(): + raise ValueError + v = int(num) except (TypeError, ValueError): raise ValueError('%s quota must be an integer in bytes' % tag) if not 0 <= v <= 0xFFFFFFFF: @@ -5998,21 +6002,49 @@ def _test_script_move(store, suites, cur, target_id): return new, None, 200 -def migrate_scripts_to_suites(script_store, suite_store): - """Attach pre-suite scripts (no ``suite_id``) to an auto-created - "Imported scripts" suite: since v3.22.0 a script belongs to exactly one - suite, and the one-time migration keeps a v3.21.0 store usable - the - operator renames or regroups them with move/copy. Returns the number of - attached scripts.""" +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 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; + - a script a suite lists but whose ``suite_id`` is empty or points + elsewhere adopts the listing suite (the member list is authoritative). + + Idempotent; returns the number of changed scripts. Called at startup + (the v3.21.0 migration) and after a suites import.""" if script_store is None or suite_store is None: return 0 - orphans = script_store.orphans() - if not orphans: + suites = suite_store.list() + owner = {} + for s in suites: + for e in s['scripts']: + owner.setdefault(e['script_id'], s['id']) + orphans = [] + adopted = 0 + for script in script_store.list(): + sid = script.get('suite_id') or None + if sid and owner.get(script['id']) == sid: + continue + listed_by = owner.get(script['id']) + if listed_by: + # the member list is authoritative: adopt the listing suite + try: + script_store.update(script['id'], {'suite_id': listed_by}) + adopted += 1 + except (test_scripts.TestScriptError, OSError) as e: + sys.stderr.write('TESTSUITES: cannot adopt %s: %s\n' + % (script['id'], e)) + continue + orphans.append(script) + if not orphans and not adopted: return 0 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: @@ -6021,16 +6053,26 @@ def migrate_scripts_to_suites(script_store, suite_store): sys.stderr.write('TESTSUITES: cannot attach %s to "%s": %s\n' % (s['id'], suite['name'], e)) continue - entries.append({'script_id': s['id'], 'role': 'member', 'on_fail': 'stop'}) + if s['id'] not in known: + entries.append({'script_id': s['id'], 'role': 'member', 'on_fail': 'stop'}) + known.add(s['id']) attached += 1 if attached: try: 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)) - sys.stderr.write('TESTSUITES: attached %d script(s) to "%s"\n' - % (attached, suite['name'])) - return attached + if attached or adopted: + sys.stderr.write('TESTSUITES: reconciled %d script(s) (attached to "%s": %d, ' + 'adopted by their suite: %d)\n' + % (attached + adopted, suite['name'], attached, adopted)) + return attached + adopted + + +def migrate_scripts_to_suites(script_store, suite_store): + """The v3.21.0 -> v3.22.0 migration: attach the pre-suite scripts to the + "Imported scripts" suite (the general reconciliation, called at startup).""" + return reconcile_scripts_to_suites(script_store, suite_store) def _test_request_blocked(path): @@ -9795,13 +9837,11 @@ class PysimHandler(BaseHTTPRequestHandler): except OSError as e: self._test_suites_store_error(e) return - # The imported scripts carry the *old* suite id: point them at the - # suite that now owns them. - for suite in store.list(): - for e in suite['scripts']: - cur = scripts.get(e['script_id']) - if cur is not None and cur.get('suite_id') != suite['id']: - scripts.update(e['script_id'], {'suite_id': suite['id']}) + # The ownership invariant: the imported scripts carry the *old* + # suite id, and a `replace` wiped the suites that owned the rest - + # the reconciliation re-points the listed scripts and adopts the + # newly unreferenced ones into "Imported scripts". + reconcile_scripts_to_suites(scripts, store) resp['scripts'] = {'added': script_result.get('added', 0), 'skipped': script_result.get('skipped', 0)} resp['skipped'] = resp.get('skipped', 0) + len(ref_errors) @@ -9825,6 +9865,20 @@ class PysimHandler(BaseHTTPRequestHandler): self._log_resp(resp) return mode = body.get('mode') if body.get('mode') in ('merge', 'replace') else 'merge' + if mode == 'replace': + # A wipe would leave every suite's member list dangling: the + # scripts are the references' targets. Merge is the normal + # path; delete the suites' scripts first if a replace is + # really wanted. + holders = [s for s in suites.list() if s['scripts']] + if holders: + resp = {'error': 'replace would leave %d suite(s) with ' + 'dangling script references - delete their ' + 'scripts first, or import with merge' + % len(holders)} + self._send_json(resp, 400) + self._log_resp(resp) + return target = suites.get(body.get('suite_id')) if body.get('suite_id') else None if body.get('suite_id') and target is None: resp = {'error': 'unknown target suite'} diff --git a/tests/test_fetch_routes.py b/tests/test_fetch_routes.py index 0191a81..f047c80 100644 --- a/tests/test_fetch_routes.py +++ b/tests/test_fetch_routes.py @@ -1382,6 +1382,45 @@ class TestScriptStoreHttpTests(unittest.TestCase): self.assertEqual(resp['added'], 0) self.assertIn('unknown script', ' '.join(resp.get('errors') or [])) + def test_scripts_import_replace_is_refused_while_suites_hold_scripts(self): + suite = self._suite() + status, resp = self._post('/api/test/scripts', self._script(suite_id=suite['id'])) + self.assertEqual(status, 200, resp) + status, resp = self._post('/api/test/scripts/import', { + 'mode': 'replace', + 'scripts': [{'name': 'new', 'steps': [ + {'type': 'action', 'kind': 'status', 'params': {'attempts': 1}}]}]}) + self.assertEqual(status, 400, resp) + self.assertIn('dangling script references', resp['error']) + self.assertEqual(len(self.store.list()), 1) # nothing was wiped + # with no suite holding scripts a replace is allowed + status, resp = self._post('/api/test/scripts/delete', + {'id': self.store.list()[0]['id']}) + self.assertEqual(status, 200, resp) + status, resp = self._post('/api/test/scripts/import', { + 'mode': 'replace', + 'scripts': [{'name': 'new', 'steps': [ + {'type': 'action', 'kind': 'status', 'params': {'attempts': 1}}]}]}) + self.assertEqual(status, 200, resp) + self.assertEqual([x['name'] for x in self.store.list()], ['new']) + + def test_suites_import_replace_adopts_the_wiped_suites_scripts(self): + # a suites replace wipes the suite that owned a script: the script is + # not lost - the reconciliation adopts it into "Imported scripts" + suite = self._suite(name='old') + status, resp = self._post('/api/test/scripts', self._script(suite_id=suite['id'])) + self.assertEqual(status, 200, resp) + sid = resp['script']['id'] + status, resp = self._post('/api/test/suites/import', { + 'mode': 'replace', 'suites': [{'name': 'new', 'scripts': []}]}) + self.assertEqual(status, 200, resp) + imported = self.suites.find_by_name('Imported scripts') + self.assertIsNotNone(imported) + self.assertEqual(self.store.get(sid)['suite_id'], imported['id']) + self.assertEqual([e['script_id'] for e in imported['scripts']], [sid]) + self.assertEqual(sorted(s['name'] for s in self.suites.list()), + ['Imported scripts', 'new']) + def test_import_accepts_the_old_localstorage_export(self): status, resp = self._post('/api/test/scripts/import', {'scripts': [ {'name': 'A', 'steps': [{'type': 'action', 'kind': 'status', diff --git a/tests/test_ota_helpers.py b/tests/test_ota_helpers.py index deb375a..a5cf953 100644 --- a/tests/test_ota_helpers.py +++ b/tests/test_ota_helpers.py @@ -1586,6 +1586,9 @@ class CapApduSequenceTest(unittest.TestCase): # a 4-byte quota above 32767 (GP Card Spec 9.7) self.assertEqual(_compose_install_params('', '', volatile_quota=32768), 'C900EF06C70400008000') + # an integral float (a JSON client may send 200.0) is accepted + self.assertEqual(_compose_install_params('', '', nv_quota=200.0), + 'C900EF04C80200C8') # an EA-form STK part is a sibling of EF (the reference TCA form) self.assertEqual(_compose_install_params('', 'EA0480000000', volatile_quota=100), diff --git a/tests/test_test_suites.py b/tests/test_test_suites.py index 5c33114..263da64 100644 --- a/tests/test_test_suites.py +++ b/tests/test_test_suites.py @@ -222,6 +222,33 @@ class MigrationTests(unittest.TestCase): # idempotent: nothing left to attach self.assertEqual(srv.migrate_scripts_to_suites(scripts, suites), 0) + def test_the_reconciliation_adopts_listed_and_missing_suite_scripts(self): + # The general invariant repair: a script whose suite_id points at a + # missing suite (a suites import with mode replace) joins "Imported + # scripts"; a script a suite lists but whose suite_id is empty or + # wrong adopts the listing suite (the member list is authoritative). + 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}} + suite_a = suites.add({'name': 'A'}) + gone = 'f' * 32 + lost = scripts.add({'name': 'lost', 'suite_id': gone, 'steps': [step]}) + listed = scripts.add({'name': 'listed', 'steps': [step]}) + suites.update(suite_a['id'], {'scripts': [ + {'script_id': listed['id'], 'role': 'setup', 'on_fail': 'stop'}]}) + self.assertEqual(srv.reconcile_scripts_to_suites(scripts, suites), 2) + # the listing suite wins (its role is kept) + self.assertEqual(scripts.get(listed['id'])['suite_id'], suite_a['id']) + # the lost one is adopted by "Imported scripts" + imported = suites.find_by_name('Imported scripts') + self.assertEqual(scripts.get(lost['id'])['suite_id'], imported['id']) + self.assertEqual([e['script_id'] for e in imported['scripts']], [lost['id']]) + # idempotent + self.assertEqual(srv.reconcile_scripts_to_suites(scripts, suites), 0) + if __name__ == '__main__': unittest.main()