From 953401a43175f1d65dd4b1d345bb1a6aebe9466c Mon Sep 17 00:00:00 2001 From: Julian lechner Date: Thu, 8 Oct 2026 11:55:47 +0200 Subject: [PATCH] fix(qmd): reject unreliable npm listings and restore non-mapping models verbatim Co-Authored-By: Claude Opus 5.5 --- scripts/lib/qmd.py | 8 +++++--- scripts/tests/test-qmd-lib.mjs | 2 ++ scripts/tests/test-qmd-setup.py | 21 +++++++++++++++++++-- shared/qmd/qmd-config.mjs | 2 +- 4 files changed, 27 insertions(+), 6 deletions(-) diff --git a/scripts/lib/qmd.py b/scripts/lib/qmd.py index f2d6350..54e7cf3 100644 --- a/scripts/lib/qmd.py +++ b/scripts/lib/qmd.py @@ -74,12 +74,14 @@ class Setup: def package_installed(self): # npm list exits non-zero on tree problems (ELSPROBLEMS) but still prints valid JSON. - output = self.run(['npm', 'list', '--global', '--depth=0', '--json'], check=False) or '{}' + output = self.run(['npm', 'list', '--global', '--depth=0', '--json'], check=False) try: listing = json.loads(output) except json.JSONDecodeError: raise ValueError('npm list did not return JSON.') - return PACKAGE in (listing.get('dependencies') or {}) + if not isinstance(listing, dict) or not isinstance(listing.get('dependencies'), dict): + raise ValueError('npm list did not report the global packages.') + return PACKAGE in listing['dependencies'] def require_node(self): if not node_version_ok(self.run(['node', '--version'])): @@ -135,7 +137,7 @@ class Setup: previous = json.loads(output.strip().splitlines()[-1])['previous'] except (IndexError, KeyError, TypeError, json.JSONDecodeError): raise ValueError('qmd-config set-models returned no previous models.') - state['previous_models'] = previous if isinstance(previous, dict) else None + state['previous_models'] = previous state['models_block'] = True atomic_json(self.state_path, state) before = {p.name for p in self.models.iterdir()} if self.models.is_dir() else set() diff --git a/scripts/tests/test-qmd-lib.mjs b/scripts/tests/test-qmd-lib.mjs index 997d2ce..e360742 100644 --- a/scripts/tests/test-qmd-lib.mjs +++ b/scripts/tests/test-qmd-lib.mjs @@ -78,6 +78,8 @@ test('runQmdCli runs the CLI entry of QMD_PACKAGE_DIR without a shell', () => { test('previousModels captures a user models block, null when absent', () => { assert.deepEqual(previousModels('models:\n embed: mine\ncollections: {}\n', YAML), { embed: 'mine' }); + assert.equal(previousModels('models: custom\n', YAML), 'custom'); + assert.deepEqual(previousModels('models: [a, b]\n', YAML), ['a', 'b']); assert.equal(previousModels('collections: {}\n', YAML), null); assert.equal(previousModels('', YAML), null); }); diff --git a/scripts/tests/test-qmd-setup.py b/scripts/tests/test-qmd-setup.py index fd743e0..50f0eb9 100644 --- a/scripts/tests/test-qmd-setup.py +++ b/scripts/tests/test-qmd-setup.py @@ -13,9 +13,9 @@ spec.loader.exec_module(qmd) class FakeRunner: - def __init__(self, installed=False, gpu='cuda', gpu_result=None, cpu_result=None, previous=None, fail_pull=False): + def __init__(self, installed=False, gpu='cuda', gpu_result=None, cpu_result=None, previous=None, fail_pull=False, list_output=None): self.calls, self.installed, self.gpu = [], installed, gpu - self.previous, self.fail_pull = previous, fail_pull + self.previous, self.fail_pull, self.list_output = previous, fail_pull, list_output self.results = {'gpu': gpu_result, 'cpu': cpu_result} def __call__(self, arguments, env=None, timeout=None, check=True): @@ -27,6 +27,8 @@ class FakeRunner: return json.dumps({'previous': self.previous}) + chr(10) if self.fail_pull and arguments[:2] == ['qmd', 'pull']: raise ValueError('pull failed') + if 'npm list' in joined and self.list_output is not None: + return self.list_output if 'npm list' in joined: return json.dumps({'dependencies': {'@tobilu/qmd': {}} if self.installed else {}}) if 'npm install' in joined: @@ -198,6 +200,21 @@ class SetupTest(unittest.TestCase): self.assertTrue(self.setup(FakeRunner(installed=True)).package_installed()) self.assertFalse(self.setup(FakeRunner()).package_installed()) + def test_package_installed_rejects_unreliable_listings(self): + for output in ('', ' ', 'not json', '{"error": {"code": "ELSPROBLEMS"}}', '{}', '[]'): + with self.assertRaises(ValueError, msg=repr(output)): + self.setup(FakeRunner(list_output=output)).package_installed() + self.assertTrue(self.setup(FakeRunner(list_output='{"dependencies": {"@tobilu/qmd": {}}, "error": {}}')).package_installed()) + self.assertFalse(self.setup(FakeRunner(list_output='{"dependencies": {}}')).package_installed()) + + def test_non_mapping_previous_models_are_restored_verbatim(self): + runner = FakeRunner(previous='custom.gguf') + setup = self.setup(runner) + setup.enable(['claude']) + self.assertEqual(self.state()['previous_models'], 'custom.gguf') + setup.disable(['claude']) + self.assertTrue(any(c[-2:] == ['restore-models', '"custom.gguf"'] for c in runner.calls)) + def test_dry_run_changes_nothing(self): runner = FakeRunner() self.setup(runner, dry_run=True).enable(['claude']) diff --git a/shared/qmd/qmd-config.mjs b/shared/qmd/qmd-config.mjs index f4f9d5d..e1a8a9a 100644 --- a/shared/qmd/qmd-config.mjs +++ b/shared/qmd/qmd-config.mjs @@ -46,7 +46,7 @@ async function main(action, argument) { next = setModels(current, YAML); } else if (action === 'restore-models') { const mapping = JSON.parse(argument ?? ''); - if (mapping === null || typeof mapping !== 'object' || Array.isArray(mapping)) throw new Error('restore-models needs a JSON object'); + if (mapping === undefined) throw new Error('restore-models needs a JSON value'); next = restoreModels(current, YAML, mapping); } else next = unsetModels(current, YAML); mkdirSync(dirname(path), { recursive: true });