From 3829bf69ae28eedcf30122dd496c0e88d9c419d5 Mon Sep 17 00:00:00 2001 From: Dmitry Kropachev Date: Thu, 22 Oct 2026 10:40:27 -0400 Subject: [PATCH 1/2] Fix frozen Git evidence and updater lifecycle edge cases Keep submodule and subdirectory inventories complete and accept SHA-256 review pins. Tie deferred updates to their installation and project, retain lifecycle tracking through optional failures, and verify installed results. Validate release version consistency and stable version syntax. Record ten independent review passes and synchronize plugin manifests at 1.0.7. --- .claude-plugin/marketplace.json | 2 +- .claude-plugin/plugin.json | 2 +- .codex-plugin/plugin.json | 2 +- PROVENANCE.json | 12 + README.md | 2 +- REVIEW_REPORT.md | 35 +++ common/codex/scripts/aspect-driver.py | 2 +- common/codex/scripts/prepare-review.js | 21 +- scripts/check.py | 4 +- scripts/update.py | 110 +++++++-- scripts/version.py | 2 +- tests/test_aspect_driver.py | 21 +- tests/test_inventory.py | 70 ++++++ tests/test_release_tools.py | 76 ++++++ tests/test_update.py | 312 +++++++++++++++++++++++++ 15 files changed, 636 insertions(+), 37 deletions(-) create mode 100644 REVIEW_REPORT.md create mode 100644 tests/test_release_tools.py diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 892a0e8..5a47fb3 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -8,7 +8,7 @@ "name": "code-review-ad", "source": "./", "description": "code-review-ad", - "version": "1.0.6" + "version": "1.0.7" } ] } diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index a19da9c..30d8fd5 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "code-review-ad", - "version": "1.0.6", + "version": "1.0.7", "description": "Independent Toolkit, Standards, Spec, and conditional Performance review.", "author": { "name": "Dmitry Kropachev" diff --git a/.codex-plugin/plugin.json b/.codex-plugin/plugin.json index 21eb4eb..53be180 100644 --- a/.codex-plugin/plugin.json +++ b/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "code-review-ad", - "version": "1.0.6", + "version": "1.0.7", "description": "Independent Toolkit, Standards, Spec, and conditional Performance review.", "author": { "name": "Dmitry Kropachev" diff --git a/PROVENANCE.json b/PROVENANCE.json index 56cad3c..fa8f326 100644 --- a/PROVENANCE.json +++ b/PROVENANCE.json @@ -40,6 +40,18 @@ "tests/test_aspect_driver.py" ], "description": "Codex issue discovery from the first prompt and one role-specific deeper-look driver call in the same session, without intermediate observation or finding-collection payloads; read-only pins and parent verification retained." + }, + { + "version": "1.0.7", + "paths": [ + "common/codex/scripts", + "scripts/update.py", + "scripts/check.py", + "scripts/version.py", + "tests", + "REVIEW_REPORT.md" + ], + "description": "Ten separate-agent correctness review passes: hardened frozen Git evidence, updater installation identity and lifecycle failure handling, and release validation; added focused regression coverage and a review report." } ] } diff --git a/README.md b/README.md index e5be187..d07f362 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # code-review-ad -Independent MIT plugin for Claude Code and Codex. Version 1.0.6. +Independent MIT plugin for Claude Code and Codex. Version 1.0.7. ## Install diff --git a/REVIEW_REPORT.md b/REVIEW_REPORT.md new file mode 100644 index 0000000..7dae566 --- /dev/null +++ b/REVIEW_REPORT.md @@ -0,0 +1,35 @@ +# Ten-pass correctness review + +Ten sequential review-and-fix iterations, each performed by a separate agent +(`review_01` through `review_10`). Later passes worked against the accumulated +fixes, and the final pass reviewed the combined diff. Findings were fixed only +when supported by a concrete failure or contract mismatch. + +| Pass | Area reviewed | Result | +| --- | --- | --- | +| 1 | Frozen Git inventory | Fixed added and modified submodules crashing source extraction when gitlink commits are unavailable locally. | +| 2 | Deeper-look driver | Accepted full SHA-256 commit IDs already supported by the inventory helper. | +| 3 | Updater installation identity | Prevented local checkouts from borrowing another Codex installation's identity, and excluded local marketplaces from automatic updates. | +| 4 | Pending update lifecycle | Rechecked full installation identity at completion and preserved requests for distinct installations, including legacy state records. | +| 5 | Release validation | Rejected stale Claude marketplace versions and invalid stable versions with leading zeroes. | +| 6 | Updater failure paths | Preserved lifecycle tracking across host/configuration/version errors, allowed completion with invalid settings, and refreshed invalid or future-dated release caches. | +| 7 | Inventory configuration and paths | Included gitlinks despite submodule-ignore settings and retained full source evidence when invoked from repository subdirectories. | +| 8 | Scoped updates and locking | Ran and verified Claude project/local updates in their registered project; retried Windows lock contention beyond the native API's short retry window. | +| 9 | Codex update verification | Required installed status and the original scope before reporting success, and ignored uninstalled duplicate rows. | +| 10 | Combined diff and runtime contracts | Aligned updater stable-version validation with release tools; invalid leading-zero and Unicode-digit tags cannot schedule installations. | + +The submodule regressions and SHA-256 integration regression were observed failing +before their fixes. Other focused regressions cover identity changes, distinct +installations, configuration and host failures, cache corruption, project scope, +Git settings, and invalid release identities. + +Final validation: + +- `python3 scripts/check.py`: valid, synchronized version 1.0.7. +- `python3 scripts/test.py`: 77 passed (baseline: 23 passed). +- `git diff --check`: passed. + +Runtime Python remains standard-library-only. Windows lock contention was tested +with a simulated `msvcrt` adapter on Linux; native Windows execution remains +unverified. Host commands and GitHub release responses use controlled fixtures; +no live plugin installation was performed. diff --git a/common/codex/scripts/aspect-driver.py b/common/codex/scripts/aspect-driver.py index d321e97..19b8033 100644 --- a/common/codex/scripts/aspect-driver.py +++ b/common/codex/scripts/aspect-driver.py @@ -19,7 +19,7 @@ def git(*args): def drive(args): - if not all(re.fullmatch(r"[a-f0-9]{40}", pin) for pin in (args.base, args.head)): + if not all(re.fullmatch(r"(?:[a-f0-9]{40}|[a-f0-9]{64})", pin) for pin in (args.base, args.head)): raise ValueError("full base/head OIDs are required") if git("rev-parse", "HEAD") != args.head: raise ValueError("checkout HEAD differs from the pinned head") diff --git a/common/codex/scripts/prepare-review.js b/common/codex/scripts/prepare-review.js index c1882ea..75eb3b4 100644 --- a/common/codex/scripts/prepare-review.js +++ b/common/codex/scripts/prepare-review.js @@ -54,26 +54,33 @@ function prepareReview({ repo = '.', base, head }) { } } repo = path.resolve(repo) + // Git's tree/pathspec commands otherwise interpret root-relative inventory + // paths from the caller's subdirectory and silently miss their blob entries. + // Remove only Git's terminator: a repository directory may end in whitespace. + repo = git(repo, 'rev-parse', '--show-toplevel').replace(/\n$/, '') base = git(repo, 'rev-parse', '--verify', `${base}^{commit}`).trim() head = git(repo, 'rev-parse', '--verify', `${head}^{commit}`).trim() if (git(repo, 'rev-parse', 'HEAD').trim() !== head) throw new Error('checkout HEAD does not match the frozen head') const mergeBase = git(repo, 'merge-base', base, head).trim() const range = `${mergeBase}...${head}` - const files = changedFiles(git(repo, 'diff', '--no-ext-diff', '--no-textconv', '--find-renames', '--name-status', '-z', range)) + const files = changedFiles(git(repo, 'diff', '--no-ext-diff', '--no-textconv', '--ignore-submodules=none', '--find-renames', '--name-status', '-z', range)) const comments = [], nearby = [], risks = [] for (const file of files) { if (file.status === 'D') continue + // Gitlinks refer to commits in a different object database. Inspect the + // tree entry rather than dereferencing their possibly unavailable objects. + const entry = git(repo, '--literal-pathspecs', 'ls-tree', '-z', head, '--', file.path) + if (!/^[0-7]+ blob /.test(entry)) continue // The old pathname may now be a directory containing unrelated new files. // Compare renamed blobs directly so its descendants cannot enter this diff. const scope = file.oldPath ? [`${mergeBase}:${file.oldPath}`, `${head}:${file.path}`] : [range, '--', file.path] const diff = git(repo, '--literal-pathspecs', 'diff', '--no-ext-diff', '--no-textconv', '--no-color', - '--find-renames', '--unified=0', ...scope) + '--ignore-submodules=none', '--find-renames', '--unified=0', ...scope) const added = addedLines(diff) - if (!added.length) continue // Binary files, submodules and pure renames stay in the file list. + if (!added.length) continue // Binary files and pure renames stay in the file list. const object = `${head}:${file.path}` - if (git(repo, 'cat-file', '-t', object).trim() !== 'blob') continue const lines = git(repo, 'cat-file', 'blob', object).split('\n') const addedNumbers = new Set(added.map((row) => row.line)) const nearbyNumbers = new Set() @@ -101,8 +108,8 @@ function prepareReview({ repo = '.', base, head }) { const commits = git(repo, 'rev-list', '--reverse', `${mergeBase}..${head}`).trim().split('\n').filter(Boolean).map((oid) => { const parent = git(repo, 'show', '-s', '--format=%P', oid).trim().split(' ')[0] || null const changed = parent - ? git(repo, 'diff', '--no-ext-diff', '--no-textconv', '--name-only', '-z', parent, oid) - : git(repo, 'diff-tree', '--root', '--no-commit-id', '--name-only', '-r', '-z', oid) + ? git(repo, 'diff', '--no-ext-diff', '--no-textconv', '--ignore-submodules=none', '--name-only', '-z', parent, oid) + : git(repo, 'diff-tree', '--ignore-submodules=none', '--root', '--no-commit-id', '--name-only', '-r', '-z', oid) return { oid, filesComparedWith: parent, message: git(repo, 'show', '-s', '--format=%B', oid).trimEnd(), @@ -112,7 +119,7 @@ function prepareReview({ repo = '.', base, head }) { return { schemaVersion: 1, base, head, mergeBase, workingTreeStatus: git(repo, 'status', '--porcelain=v1'), - diffCommand: `git diff --no-ext-diff --no-textconv --find-renames ${range}`, + diffCommand: `git diff --no-ext-diff --no-textconv --ignore-submodules=none --find-renames ${range}`, files, commits, comments, nearby, risks, } } diff --git a/scripts/check.py b/scripts/check.py index 591c13e..49d3162 100644 --- a/scripts/check.py +++ b/scripts/check.py @@ -5,7 +5,7 @@ ROOT=Path(__file__).resolve().parent.parent def version(v): - if not re.fullmatch(r"[0-9]+\.[0-9]+\.[0-9]+",v):raise ValueError('Stable semantic version required') + if not re.fullmatch(r"(?:0|[1-9][0-9]*)\.(?:0|[1-9][0-9]*)\.(?:0|[1-9][0-9]*)",v):raise ValueError('Stable semantic version required') return tuple(map(int,v.split('.'))) def check(base=None): @@ -28,6 +28,8 @@ def check(base=None): for host,filename in [('codex','.agents/plugins/marketplace.json'),('claude','.claude-plugin/marketplace.json')]: catalog=json.loads((ROOT/filename).read_text());assert catalog['name']==manifests[0]['name'] assert [p['name'] for p in catalog['plugins']]==[manifests[0]['name']] + if host=='claude': + assert catalog['plugins'][0]['version']==manifests[0]['version'],'Claude marketplace version must match host manifests' for p in ROOT.rglob('*'): if '.git' in p.parts or '__pycache__' in p.parts or '.pytest_cache' in p.parts:continue if p.is_symlink():assert p.resolve().is_relative_to(ROOT),str(p) diff --git a/scripts/update.py b/scripts/update.py index 6e37453..0c83d31 100644 --- a/scripts/update.py +++ b/scripts/update.py @@ -2,6 +2,7 @@ """Self-contained plugin lifecycle and release policy (Python standard library).""" import argparse from contextlib import contextmanager +import errno import json import os from pathlib import Path @@ -39,8 +40,18 @@ def locked(path): with path.open('a+b') as stream: if os.name == 'nt': import msvcrt - stream.write(b'0'); stream.flush(); stream.seek(0) - msvcrt.locking(stream.fileno(), msvcrt.LK_LOCK, 1) + stream.seek(0, 2) + if stream.tell() == 0: + stream.write(b'0'); stream.flush() + stream.seek(0) + while True: + try: + msvcrt.locking(stream.fileno(), msvcrt.LK_NBLCK, 1) + break + except OSError as exc: + if exc.errno not in (errno.EACCES, errno.EAGAIN, errno.EDEADLK): + raise + time.sleep(0.05) else: import fcntl fcntl.flock(stream, fcntl.LOCK_EX) @@ -54,23 +65,34 @@ def locked(path): def version(value): - match = re.fullmatch(r'v?(\d+)\.(\d+)\.(\d+)', value or '') + match = re.fullmatch(r'v?(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)', value or '') return tuple(map(int, match.groups())) if match else None -def run(argv): - proc = subprocess.run(argv, text=True, capture_output=True, timeout=90) +def run(argv, *, cwd=None): + proc = subprocess.run(argv, text=True, capture_output=True, timeout=90, cwd=cwd) if proc.returncode: raise RuntimeError(proc.stderr.strip() or proc.stdout.strip() or 'Host command failed') return proc.stdout +def project_path(record): + if record.get('scope') not in ('project', 'local'): + return None + value = record.get('projectPath') + if isinstance(value, str) and Path(value).is_absolute(): + return str(Path(value).resolve()) + return None + + def installed(root, host): + root = root.resolve() manifest = read(root / f'.{host}-plugin/plugin.json') name = manifest['name'] marketplace = None pinned = False scope = 'user' + project = None records = [] if host == 'claude': home = Path(os.environ.get('CLAUDE_CONFIG_DIR', Path.home() / '.claude')) @@ -81,7 +103,23 @@ def installed(root, host): known = read(home / 'plugins/known_marketplaces.json') else: data = json.loads(run(['codex', 'plugin', 'list', '--json'])) - records = [(r['pluginId'], r) for r in data.get('installed', []) if r.get('name') == name] + home = Path(os.environ.get('CODEX_HOME', Path.home() / '.codex')) + for record in data.get('installed', []): + if record.get('name') != name or record.get('installed') is False: + continue + identity = record.get('pluginId', '') + plugin_name, separator, catalog_name = identity.partition('@') + if not separator or plugin_name != name or not catalog_name: + continue + # Listing a plugin with the same name does not register this script's + # checkout. Current Codex lists omit installPath, so also recognize + # the host's versioned cache directory (including an older loaded + # version after the registry has advanced). + install_path = record.get('installPath') + registered_root = (Path(install_path) if install_path else + home / 'plugins/cache' / catalog_name / name / manifest['version']) + if registered_root.resolve() == root: + records.append((identity, record)) known = {} if len(records) > 1: raise ValueError('Ambiguous installed plugin registry identity; no automatic update') @@ -90,13 +128,17 @@ def installed(root, host): manifest['version'] = record.get('version', manifest['version']) marketplace = identity.split('@', 1)[1] scope = record.get('scope', 'user') + if host == 'claude': + project = project_path(record) + pinned |= scope in ('project', 'local') and project is None source = record.get('source') or known.get(marketplace, {}).get('source', {}) if isinstance(source, str): source = {'source': source} marketplace_source = record.get('marketplaceSource') or {} remote_marketplace = marketplace_source.get('sourceType') == 'git' - pinned = bool(record.get('pinned') or source.get('sha') or source.get('ref') not in (None, 'main') + pinned |= bool(record.get('pinned') or source.get('sha') or source.get('ref') not in (None, 'main') or marketplace_source.get('sha') or marketplace_source.get('ref') not in (None, 'main') + or marketplace_source.get('sourceType') in ('local', 'directory', 'file') or (source.get('source') == 'local' and not remote_marketplace) or source.get('sourceType') in ('local', 'directory') or source.get('source') in ('directory', 'file')) @@ -113,7 +155,7 @@ def installed(root, host): pinned = True # Direct checkout, plugin-dir, or unregistered installation. return {'name': name, 'host': host, 'marketplace': marketplace, 'version': manifest['version'], 'repository': 'dk-code-factory/' + name, 'pinned': pinned, 'scope': scope, - 'root': str(root)} + 'root': str(root), 'project_path': project} def latest(repository): @@ -150,9 +192,16 @@ def mode(settings, name): return value +def installation_key(identity): + return json.dumps([identity[key] for key in ('host', 'name', 'marketplace', 'scope', 'root')] + + [identity.get('project_path')]) + + def update(identity, target): name, marketplace, host = (identity[k] for k in ('name', 'marketplace', 'host')) - if identity['pinned'] or not marketplace: + if (identity['pinned'] or not marketplace + or host == 'claude' and identity['scope'] in ('project', 'local') + and not identity.get('project_path')): return {'status': 'pinned', 'message': 'Pinned or unregistered installation; update manually.'} if version(identity['version']) and version(identity['version']) >= version(target): return {'status': 'current', 'version': identity['version'], 'reload_required': True, @@ -165,14 +214,17 @@ def update(identity, target): ['codex', 'plugin', 'add', name + '@' + marketplace, '--json']] try: for command in commands: - run(command) + run(command, cwd=identity.get('project_path') if host == 'claude' else None) if host == 'codex': rows = json.loads(run(['codex', 'plugin', 'list', '--json'])).get('installed', []) - versions = [r['version'] for r in rows if r['pluginId'] == name + '@' + marketplace] + versions = [r['version'] for r in rows if r['pluginId'] == name + '@' + marketplace + and r.get('installed') is not False + and r.get('scope', 'user') == identity['scope']] else: home = Path(os.environ.get('CLAUDE_CONFIG_DIR', Path.home() / '.claude')) rows = read(home / 'plugins/installed_plugins.json').get('plugins', {}).get(name + '@' + marketplace, []) - versions = [r['version'] for r in rows if r.get('scope') == identity['scope']] + versions = [r['version'] for r in rows if r.get('scope') == identity['scope'] + and project_path(r) == identity.get('project_path')] if len(versions) != 1 or not version(versions[0]) or version(versions[0]) < version(target): raise RuntimeError('Host did not confirm installation of the released version') return {'status': 'updated', 'version': versions[0], 'reload_required': True, @@ -184,12 +236,9 @@ def update(identity, target): def lifecycle(root, host, phase, *, token=None, parent=None, now=None): now = time.time() if now is None else now config_path, state = paths(root, Path.cwd()) - settings = read(config_path) with locked(state / '.lock'): data = read(state / 'state.json', {'checks': {}, 'notices': [], 'invocations': {}, 'sessions': {}}) if phase == 'entry': - identity = installed(root, host) - policy = mode(settings, identity['name']) token = uuid.uuid4().hex if parent: if parent not in data['invocations'] or data['invocations'][parent]['complete']: @@ -201,11 +250,27 @@ def lifecycle(root, host, phase, *, token=None, parent=None, now=None): session = data['sessions'][session_id] session['active'].append(token) data['invocations'][token] = {'session': session_id, 'complete': False} + # Host/configuration failures must not hide a live workflow from + # other invocations that are waiting to install an update. + try: + identity = installed(root, host) + if not version(identity['version']): + raise ValueError('Installed plugin version is not stable semantic versioning') + policy = mode(read(config_path), identity['name']) + except Exception as exc: + write(state / 'state.json', data) + return {'status': 'unavailable', 'action': 'continue', 'message': str(exc), + 'non_blocking': True, 'token': token} result = {'status': 'current', 'action': 'continue', 'message': '', 'token': token, 'identity': identity, 'mode': policy} if policy != 'off': cache = data['checks'].get(identity['repository'], {}) - if now - cache.get('checked', -DAY) >= DAY: + checked = cache.get('checked') if isinstance(cache, dict) else None + valid_cache = (isinstance(checked, (int, float)) and not isinstance(checked, bool) + and 0 <= now - checked < DAY + and ('error' in cache or isinstance(cache.get('version'), str) + and version(cache['version']))) + if not valid_cache: try: cache = {'checked': now, 'version': latest(identity['repository'])} except Exception as exc: @@ -220,7 +285,7 @@ def lifecycle(root, host, phase, *, token=None, parent=None, now=None): status, message = 'pinned', f'{identity["name"]} {target} is available; this installation is pinned or unregistered.' elif policy == 'auto': status, message = 'deferred', f'{identity["name"]} {target} will update after the outer workflow completes.' - session['pending'][host + ':' + identity['name']] = {'identity': identity, 'target': target} + session['pending'][installation_key(identity)] = {'identity': identity, 'target': target} else: status, message = 'available', f'{identity["name"]} {target} is available (installed {identity["version"]}).' result['status'] = status @@ -243,15 +308,18 @@ def lifecycle(root, host, phase, *, token=None, parent=None, now=None): # pending work only when all recorded invocations have finished. pending_updates = {} for row in data['sessions'].values(): - pending_updates.update(row['pending']) + # Derive keys from saved identities so older host:name keys + # remain readable without merging distinct installations. + for pending in row['pending'].values(): + pending_updates[installation_key(pending['identity'])] = pending for pending in pending_updates.values(): identity = pending['identity'] # Recheck policy and pins at completion; changing settings must cancel a pending auto-update. - if mode(settings, identity['name']) != 'auto': - continue try: + if mode(read(config_path), identity['name']) != 'auto': + continue current = installed(Path(identity['root']), identity['host']) - if current['marketplace'] != identity['marketplace']: + if installation_key(current) != installation_key(identity): raise ValueError('Installation identity changed during workflow') outcome = update(current, pending['target']) except Exception as exc: diff --git a/scripts/version.py b/scripts/version.py index 9c1a40b..c743402 100644 --- a/scripts/version.py +++ b/scripts/version.py @@ -3,7 +3,7 @@ import argparse,json,re from pathlib import Path p=argparse.ArgumentParser();p.add_argument('version');a=p.parse_args() -if not re.fullmatch(r'[0-9]+\.[0-9]+\.[0-9]+',a.version):p.error('Use X.Y.Z') +if not re.fullmatch(r'(?:0|[1-9][0-9]*)\.(?:0|[1-9][0-9]*)\.(?:0|[1-9][0-9]*)',a.version):p.error('Use X.Y.Z without leading zeroes') r=Path(__file__).resolve().parent.parent for path in [r/'.codex-plugin/plugin.json',r/'.claude-plugin/plugin.json']: d=json.loads(path.read_text());d['version']=a.version;path.write_text(json.dumps(d,indent=2)+'\n') diff --git a/tests/test_aspect_driver.py b/tests/test_aspect_driver.py index 2947c10..67a97a2 100644 --- a/tests/test_aspect_driver.py +++ b/tests/test_aspect_driver.py @@ -1,5 +1,6 @@ """Exercise deeper-look prompts against real pinned Git objects.""" from pathlib import Path +import json import subprocess import sys @@ -10,10 +11,11 @@ @pytest.fixture -def repo(tmp_path): +def repo(tmp_path, request): def git(*args): return subprocess.check_output(["git", "-C", str(tmp_path), *args], text=True).strip() - git("init", "-q") + object_format = getattr(request, "param", "sha1") + git("init", "-q", f"--object-format={object_format}") git("config", "user.email", "test@example.invalid") git("config", "user.name", "Test") (tmp_path / "code.py").write_text("value = 1\n") @@ -43,6 +45,21 @@ def test_returns_distinct_role_prompts_without_editing_checkout(repo): assert subprocess.check_output(["git", "status", "--porcelain"], cwd=repo[0]) == b"" +@pytest.mark.parametrize("repo", ["sha256"], indirect=True) +def test_accepts_sha256_pins_from_inventory(repo): + directory, base, head = repo + inventory = subprocess.run( + ["node", str(DRIVER.with_name("prepare-review.js")), "--base", base, + "--head", head, "--json"], + cwd=directory, capture_output=True, text=True, check=True, + ) + pins = json.loads(inventory.stdout) + assert len(pins["mergeBase"]) == len(pins["head"]) == 64 + result = run((directory, pins["mergeBase"], pins["head"])) + assert result.returncode == 0, result.stderr + assert "Look deeper" in result.stdout + + @pytest.mark.parametrize("pin", ["HEAD", "abc", "f" * 40]) def test_rejects_invalid_or_missing_base(repo, pin): directory, base, head = repo diff --git a/tests/test_inventory.py b/tests/test_inventory.py index f5abf75..6a4c1a2 100644 --- a/tests/test_inventory.py +++ b/tests/test_inventory.py @@ -23,3 +23,73 @@ def git(*args):return subprocess.check_output(['git',*args],cwd=tmp_path,text=Tr assert git('status','--porcelain')==before and (tmp_path/'x.py').read_text()=='local work\n' failed=subprocess.run(['node',str(HELPER),'--repo',str(tmp_path),'--base','HEAD~1','--head',head],text=True,capture_output=True) assert failed.returncode and 'full commit' in failed.stderr + + +@pytest.mark.parametrize("initial_submodule", [False, True]) +@pytest.mark.parametrize("ignore_submodules", [False, True]) +def test_inventory_keeps_submodules_with_unavailable_commit_objects(tmp_path, initial_submodule, ignore_submodules): + def git(*args): + return subprocess.check_output(['git', *args], cwd=tmp_path, text=True).strip() + + git('init', '-q') + git('config', 'user.name', 'Test') + git('config', 'user.email', 'test@example.test') + (tmp_path / 'source.py').write_text('value = 1\n') + git('add', '.') + if initial_submodule: + git('update-index', '--add', '--cacheinfo', '160000', '1' * 40, 'dependency') + git('commit', '-qm', 'Initial') + base = git('rev-parse', 'HEAD') + git('update-index', '--add', '--cacheinfo', '160000', '2' * 40, 'dependency') + git('commit', '-qm', 'Update dependency') + head = git('rev-parse', 'HEAD') + if ignore_submodules: + git('config', 'diff.ignoreSubmodules', 'all') + + result = subprocess.run( + ['node', str(HELPER), '--repo', str(tmp_path), '--base', base, '--head', head, '--json'], + text=True, capture_output=True, check=True, + ) + data = json.loads(result.stdout) + assert data['files'] == [{'status': 'M' if initial_submodule else 'A', 'path': 'dependency'}] + assert data['commits'][0]['files'] == ['dependency'] + assert data['comments'] == [] + assert data['nearby'] == [] + assert data['risks'] == [] + + +def test_inventory_from_subdirectory_uses_repository_relative_paths(tmp_path): + # Preserve trailing whitespace in the root returned by Git as well. + repo = tmp_path / 'repository \n' + subdir = repo / 'nested' + subdir.mkdir(parents=True) + + def git(*args): + return subprocess.check_output(['git', *args], cwd=repo, text=True).strip() + + git('init', '-q') + git('config', 'user.name', 'Test') + git('config', 'user.email', 'test@example.test') + for source in [repo / 'outside.py', subdir / 'inside.py']: + source.write_text('value = 1\n') + git('add', '.') + git('commit', '-qm', 'Initial') + base = git('rev-parse', 'HEAD') + for source in [repo / 'outside.py', subdir / 'inside.py']: + source.write_text('# Retain the value\nsleep(1)\n') + git('add', '.') + git('commit', '-qm', 'Change both files') + head = git('rev-parse', 'HEAD') + git('config', 'diff.relative', 'true') + + def inventory(directory): + result = subprocess.run( + ['node', str(HELPER), '--repo', str(directory), '--base', base, '--head', head, '--json'], + text=True, capture_output=True, check=True, + ) + return json.loads(result.stdout) + + data = inventory(subdir) + assert data == inventory(repo) + assert {row['path'] for row in data['comments']} == {'outside.py', 'nested/inside.py'} + assert {row['path'] for row in data['risks']} == {'outside.py', 'nested/inside.py'} diff --git a/tests/test_release_tools.py b/tests/test_release_tools.py new file mode 100644 index 0000000..b1fd0e8 --- /dev/null +++ b/tests/test_release_tools.py @@ -0,0 +1,76 @@ +"""Validate release identities before CI and publication consume them.""" +import json +from pathlib import Path +import shutil +import subprocess +import sys + +import pytest + + +ROOT = Path(__file__).resolve().parents[1] + + +@pytest.fixture +def release_tree(tmp_path): + def write(path, value): + target = tmp_path / path + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(json.dumps(value)) + + for host in ("codex", "claude"): + write(f".{host}-plugin/plugin.json", { + "name": "example", "version": "1.2.3", "skills": f"./{host}/skills/", + }) + skill = tmp_path / host / "skills/example/SKILL.md" + skill.parent.mkdir(parents=True) + skill.write_text("name: example\ndescription: lifecycle test\n") + for path in (".agents/plugins/marketplace.json", ".claude-plugin/marketplace.json"): + write(path, {"name": "example", "plugins": [{"name": "example", "version": "1.2.3"}]}) + (tmp_path / "scripts").mkdir() + for name in ("check.py", "version.py"): + shutil.copyfile(ROOT / "scripts" / name, tmp_path / "scripts" / name) + return tmp_path + + +def run(tree, script, *args): + return subprocess.run([sys.executable, str(tree / "scripts" / script), *args], + capture_output=True, text=True) + + +def test_checker_rejects_stale_marketplace_version(release_tree): + assert run(release_tree, "check.py").returncode == 0 + catalog_path = release_tree / ".claude-plugin/marketplace.json" + catalog = json.loads(catalog_path.read_text()) + catalog["plugins"][0]["version"] = "1.2.2" + catalog_path.write_text(json.dumps(catalog)) + result = run(release_tree, "check.py") + assert result.returncode == 1 + assert "marketplace version must match" in result.stderr + + +@pytest.mark.parametrize("version", ["01.2.3", "1.02.3", "1.2.03"]) +def test_release_tools_reject_leading_zeroes(release_tree, version): + before = {path: path.read_bytes() for path in release_tree.rglob("*.json")} + result = run(release_tree, "version.py", version) + assert result.returncode != 0 + assert {path: path.read_bytes() for path in before} == before + for path in before: + value = json.loads(path.read_text()) + if "version" in value: + value["version"] = version + else: + value["plugins"][0]["version"] = version + path.write_text(json.dumps(value)) + result = run(release_tree, "check.py") + assert result.returncode == 1 + assert "Stable semantic version required" in result.stderr + + +@pytest.mark.parametrize("version", ["0.0.0", "1.10.0", "20.0.31"]) +def test_version_command_updates_all_release_versions(release_tree, version): + result = run(release_tree, "version.py", version) + assert result.returncode == 0, result.stderr + result = run(release_tree, "check.py") + assert result.returncode == 0, result.stderr + assert json.loads(result.stdout)["version"] == version diff --git a/tests/test_update.py b/tests/test_update.py index a72f7d5..426ac8e 100644 --- a/tests/test_update.py +++ b/tests/test_update.py @@ -1,4 +1,5 @@ import json +import shutil import pytest from update_fixtures import Fixture @@ -55,6 +56,90 @@ def offline(*a,**kw):f.network.append('offline');raise OSError('offline') assert f.invoke(phase='complete',token=one['token'])['status']=='deferred' +@pytest.mark.parametrize('tag', ['v01.2.3', 'v1.02.3', 'v1.2.03', 'v١.2.3']) +def test_non_semver_release_is_unavailable_and_never_auto_installed(tmp_path, tag): + import io + + f = Fixture(tmp_path, 'invalid-release', 'auto') + + def invalid_release(request, **kwargs): + f.network.append(request.full_url) + return io.StringIO(json.dumps({'tag_name': tag, 'draft': False, 'prerelease': False})) + + f.urlopen = invalid_release + first = f.invoke() + second = f.invoke() + assert first['status'] == second['status'] == 'offline' + assert len(f.network) == 1 + assert f.invoke(phase='complete', token=first['token'])['status'] == 'deferred' + assert f.invoke(phase='complete', token=second['token'])['updates'] == [] + assert not [call for call in f.calls if call[1:3] == ['plugin', 'add']] + + +@pytest.mark.parametrize('failure', ['host-offline', 'host-invalid', 'config-invalid', 'version-invalid']) +def test_optional_entry_failure_still_defers_updates_until_completion(tmp_path, failure): + import subprocess + + f = Fixture(tmp_path, 'entry-failure', 'auto') + outer = f.invoke() + original = f.subrun + config = f.base / 'config/dk-code-factory/updates.json' + + def unavailable(argv, **kwargs): + if argv[:3] == ['codex', 'plugin', 'list']: + if failure == 'host-offline': + raise OSError('Host unavailable') + return subprocess.CompletedProcess(argv, 0, 'null', '') + return original(argv, **kwargs) + + if failure.startswith('host-'): + f.subrun = unavailable + elif failure == 'config-invalid': + config.write_text('{') + else: + f.records[0]['version'] = 'unknown' + inner = f.invoke() + assert inner['status'] == 'unavailable' + assert inner['non_blocking'] is True + assert inner['token'] in f.state()['invocations'] + f.subrun = original + config.write_text('{"mode":"auto"}') + f.records[0]['version'] = '1.0.0' + assert f.invoke(phase='complete', token=outer['token'])['status'] == 'deferred' + assert not [call for call in f.calls if call[1:3] == ['plugin', 'add']] + done = f.invoke(phase='complete', token=inner['token']) + assert done['updates'][0]['status'] == 'updated' + + +@pytest.mark.parametrize('config', ['{', '{"mode":"invalid"}', '{"plugins":null}']) +def test_invalid_configuration_does_not_strand_completion_or_install(tmp_path, config): + f = Fixture(tmp_path, 'completion-config', 'auto') + entry = f.invoke() + (f.base / 'config/dk-code-factory/updates.json').write_text(config) + done = f.invoke(phase='complete', token=entry['token']) + assert done['status'] == 'completed' + assert done['updates'][0]['status'] == 'update_failed' + assert f.state()['invocations'][entry['token']]['complete'] is True + assert not [call for call in f.calls if call[1:3] == ['plugin', 'add']] + + +@pytest.mark.parametrize('cache', [None, {'checked': 'invalid', 'version': '1.1.0'}, + {'checked': 1e100, 'version': '1.1.0'}, + {}, {'version': 'invalid'}, {'version': None}]) +def test_invalid_release_cache_is_refreshed_without_losing_entry_token(tmp_path, cache): + f = Fixture(tmp_path, 'invalid-cache', 'auto') + entry = f.invoke() + state = f.state() + if isinstance(cache, dict) and 'checked' not in cache: + cache['checked'] = state['checks']['dk-code-factory/dk-workflows']['checked'] + state['checks']['dk-code-factory/dk-workflows'] = cache + (f.base / 'state/dk-code-factory/updates/state.json').write_text(json.dumps(state)) + other = f.invoke() + assert other['status'] == 'deferred' + assert other['token'] in f.state()['invocations'] + assert len(f.network) == 2 + + def test_pinned_version_does_not_update(tmp_path): f=Fixture(tmp_path,'pin','auto');f.records[0]['source']['ref']='v1.0.0' one=f.invoke();assert one['status']=='pinned' @@ -87,6 +172,40 @@ def unchanged(argv,**kwargs): assert done['updates'][0]['status']=='update_failed' +@pytest.mark.parametrize('changed', [{'installed': False}, {'scope': 'project'}]) +def test_codex_verification_requires_installed_record_in_requested_scope(tmp_path, changed): + f = Fixture(tmp_path, 'final-verification', 'auto') + entry = f.invoke() + original = f.subrun + + def changed_record(argv, **kwargs): + result = original(argv, **kwargs) + if argv[:3] == ['codex', 'plugin', 'add']: + f.records[0].update(changed) + return result + + f.subrun = changed_record + done = f.invoke(phase='complete', token=entry['token']) + assert done['updates'][0]['status'] == 'update_failed' + assert 'did not confirm' in done['updates'][0]['message'] + + +def test_codex_verification_ignores_uninstalled_duplicate(tmp_path): + f = Fixture(tmp_path, 'uninstalled-duplicate', 'auto') + entry = f.invoke() + original = f.subrun + + def duplicate_record(argv, **kwargs): + result = original(argv, **kwargs) + if argv[:3] == ['codex', 'plugin', 'add']: + f.records.append(dict(f.records[0], installed=False)) + return result + + f.subrun = duplicate_record + done = f.invoke(phase='complete', token=entry['token']) + assert done['updates'][0]['status'] == 'updated' + + def test_already_newer_installation_is_not_reinstalled(tmp_path): f=Fixture(tmp_path,'newer','auto');one=f.invoke();f.records[0]['version']='2.0.0' done=f.invoke(phase='complete',token=one['token']) @@ -100,3 +219,196 @@ def test_local_entry_in_remote_marketplace_can_auto_update(tmp_path): f.records[0]['marketplaceSource']={'sourceType':'git','source':'https://github.com/dk-code-factory/dk-workflows.git','ref':'main'} one=f.invoke();assert one['status']=='deferred' assert f.invoke(phase='complete',token=one['token'])['updates'][0]['status']=='updated' + + +@pytest.mark.parametrize('has_install_path', [True, False]) +def test_codex_local_checkout_does_not_borrow_registered_identity(tmp_path, has_install_path): + f = Fixture(tmp_path, 'checkout', 'auto') + if has_install_path: + f.records[0]['installPath'] = str(f.base / 'another-installed-copy') + else: + del f.records[0]['installPath'] + f.env['CODEX_HOME'] = str(f.base / 'codex') + one = f.invoke() + assert one['status'] == 'pinned' + assert one['identity']['marketplace'] is None + assert not f.invoke(phase='complete', token=one['token'])['updates'] + assert not [call for call in f.calls if call[1:3] == ['plugin', 'add']] + + +@pytest.mark.parametrize('registered_version', ['1.0.0', '1.1.0']) +def test_codex_cache_identity_without_install_path(tmp_path, monkeypatch, registered_version): + f = Fixture(tmp_path, 'cache-identity', 'auto') + mod = f.modules['dk-workflows'] + home = f.base / 'codex' + root = home / 'plugins/cache/factory/dk-workflows/1.0.0' + shutil.copytree(f.base / 'installed/dk-workflows', root) + del f.records[0]['installPath'] + f.records[0]['version'] = registered_version + monkeypatch.setenv('CODEX_HOME', str(home)) + monkeypatch.setattr(mod.subprocess, 'run', f.subrun) + identity = mod.installed(root, 'codex') + assert identity['marketplace'] == 'factory' + assert identity['version'] == registered_version + assert identity['pinned'] is False + + +@pytest.mark.parametrize('source_type', ['local', 'directory', 'file']) +def test_codex_local_marketplace_does_not_auto_update_remote_plugin(tmp_path, source_type): + f = Fixture(tmp_path, 'local-marketplace', 'auto') + f.records[0]['source'] = {'source': 'git', 'url': 'https://example.com/plugin.git', 'ref': 'main'} + f.records[0]['marketplaceSource'] = {'sourceType': source_type, 'source': str(f.base / 'catalog')} + one = f.invoke() + assert one['status'] == 'pinned' + assert not f.invoke(phase='complete', token=one['token'])['updates'] + + +@pytest.mark.parametrize('host', ['codex', 'claude']) +def test_changed_installation_scope_cancels_pending_update(tmp_path, host): + f = Fixture(tmp_path, 'changed-scope', 'auto', host) + one = f.invoke() + f.records[0]['scope'] = 'project' + f.write_claude() + done = f.invoke(phase='complete', token=one['token']) + assert done['updates'][0]['status'] == 'update_failed' + assert 'identity changed' in done['updates'][0]['message'] + assert not [call for call in f.calls if call[1:3] in (['plugin', 'add'], ['plugin', 'update'])] + + +@pytest.mark.parametrize('nested,legacy_keys', [(False, False), (True, False), (False, True)]) +def test_parallel_scoped_installations_are_both_updated(tmp_path, nested, legacy_keys): + import subprocess + + first = Fixture(tmp_path, 'first-scope', 'auto', 'claude') + second = Fixture(tmp_path, 'second-scope', 'auto', 'claude') + second.env = first.env + records = [first.records[0], second.records[0]] + records[1]['scope'] = 'project' + records[1]['projectPath'] = str(second.base) + registry = first.base / 'claude/plugins/installed_plugins.json' + + def write_registry(): + registry.write_text(json.dumps({'plugins': {'dk-workflows@factory': records}})) + + original = first.subrun + + def scoped_run(argv, **kwargs): + if argv[:3] == ['claude', 'plugin', 'update']: + first.calls.append(argv) + for record in records: + if record['scope'] == argv[-1]: + record['version'] = '1.1.0' + write_registry() + return subprocess.CompletedProcess(argv, 0, '', '') + return original(argv, **kwargs) + + write_registry() + first.subrun = second.subrun = scoped_run + one = first.invoke() + two = second.invoke(**({'parent': one['token']} if nested else {})) + if legacy_keys: + state = first.state() + for session in state['sessions'].values(): + session['pending'] = {'claude:dk-workflows': next(iter(session['pending'].values()))} + (first.base / 'state/dk-code-factory/updates/state.json').write_text(json.dumps(state)) + assert second.invoke(phase='complete', token=two['token'])['status'] == 'deferred' + done = first.invoke(phase='complete', token=one['token']) + assert [update['status'] for update in done['updates']] == ['updated', 'updated'] + assert [record['version'] for record in records] == ['1.1.0', '1.1.0'] + assert {call[-1] for call in first.calls if call[:3] == ['claude', 'plugin', 'update']} == {'user', 'project'} + + +@pytest.mark.parametrize('scope', ['project', 'local']) +def test_claude_scoped_update_uses_registered_project_and_verifies_it(tmp_path, scope): + f = Fixture(tmp_path, 'project-context', 'auto', 'claude') + project = f.base / 'project' + project.mkdir() + f.records[0].update(scope=scope, projectPath=str(project)) + f.write_claude() + original = f.subrun + contexts = [] + + def scoped_run(argv, **kwargs): + if argv[0] == 'claude': + contexts.append(kwargs.get('cwd')) + return original(argv, **kwargs) + + f.subrun = scoped_run + entry = f.invoke() + done = f.invoke(phase='complete', token=entry['token']) + assert done['updates'][0]['status'] == 'updated' + assert contexts == [str(project), str(project)] + + +@pytest.mark.parametrize('project', [None, '', 'relative/project']) +def test_claude_project_scope_without_absolute_context_is_pinned(tmp_path, project): + f = Fixture(tmp_path, 'missing-context', 'auto', 'claude') + f.records[0].update(scope='project', projectPath=project) + f.write_claude() + entry = f.invoke() + assert entry['status'] == 'pinned' + assert not f.invoke(phase='complete', token=entry['token'])['updates'] + + +def test_claude_verification_does_not_accept_another_project(tmp_path): + f = Fixture(tmp_path, 'project-verification', 'auto', 'claude') + project = f.base / 'project' + project.mkdir() + f.records[0].update(scope='project', projectPath=str(project)) + f.write_claude() + entry = f.invoke() + original = f.subrun + + def moved_record(argv, **kwargs): + result = original(argv, **kwargs) + if argv[:3] == ['claude', 'plugin', 'update']: + f.records[0]['projectPath'] = str(f.base / 'different-project') + f.write_claude() + return result + + f.subrun = moved_record + done = f.invoke(phase='complete', token=entry['token']) + assert done['updates'][0]['status'] == 'update_failed' + assert 'did not confirm' in done['updates'][0]['message'] + + +@pytest.mark.parametrize('scope', ['user', 'project']) +def test_legacy_pending_context_is_safe(tmp_path, scope): + f = Fixture(tmp_path, 'legacy-context', 'auto', 'claude') + f.records[0].update(scope=scope, projectPath=str(f.base)) + f.write_claude() + entry = f.invoke() + state = f.state() + for session in state['sessions'].values(): + for pending in session['pending'].values(): + pending['identity'].pop('project_path') + (f.base / 'state/dk-code-factory/updates/state.json').write_text(json.dumps(state)) + done = f.invoke(phase='complete', token=entry['token']) + assert done['updates'][0]['status'] == ('updated' if scope == 'user' else 'update_failed') + if scope == 'project': + assert not [call for call in f.calls if call[:3] == ['claude', 'plugin', 'update']] + + +def test_windows_lock_retries_contention_without_growing_lock_file(tmp_path, monkeypatch): + import errno + import sys + from types import SimpleNamespace + + f = Fixture(tmp_path, 'windows-lock') + mod = f.modules['dk-workflows'] + lock = f.base / '.lock' + attempts = [] + + def locking(fd, operation, size): + attempts.append(operation) + if operation == 1 and attempts.count(1) <= 12: + raise OSError(errno.EACCES, 'Lock held by another workflow') + + monkeypatch.setattr(mod, 'os', SimpleNamespace(name='nt')) + monkeypatch.setitem(sys.modules, 'msvcrt', SimpleNamespace(LK_NBLCK=1, LK_UNLCK=2, locking=locking)) + monkeypatch.setattr(mod.time, 'sleep', lambda delay: None) + for _ in range(2): + with mod.locked(lock): + assert lock.read_bytes() == b'0' + assert attempts.count(1) == 14 + assert attempts.count(2) == 2 From 2b54d537a47247d55137282b980d96b4b6af1b08 Mon Sep 17 00:00:00 2001 From: Dmitry Kropachev Date: Thu, 22 Oct 2026 10:44:56 -0400 Subject: [PATCH 2/2] Remove the review report from the plugin --- PROVENANCE.json | 5 ++--- REVIEW_REPORT.md | 35 ----------------------------------- 2 files changed, 2 insertions(+), 38 deletions(-) delete mode 100644 REVIEW_REPORT.md diff --git a/PROVENANCE.json b/PROVENANCE.json index fa8f326..35cceda 100644 --- a/PROVENANCE.json +++ b/PROVENANCE.json @@ -48,10 +48,9 @@ "scripts/update.py", "scripts/check.py", "scripts/version.py", - "tests", - "REVIEW_REPORT.md" + "tests" ], - "description": "Ten separate-agent correctness review passes: hardened frozen Git evidence, updater installation identity and lifecycle failure handling, and release validation; added focused regression coverage and a review report." + "description": "Ten separate-agent correctness review passes: hardened frozen Git evidence, updater installation identity and lifecycle failure handling, and release validation; added focused regression coverage." } ] } diff --git a/REVIEW_REPORT.md b/REVIEW_REPORT.md deleted file mode 100644 index 7dae566..0000000 --- a/REVIEW_REPORT.md +++ /dev/null @@ -1,35 +0,0 @@ -# Ten-pass correctness review - -Ten sequential review-and-fix iterations, each performed by a separate agent -(`review_01` through `review_10`). Later passes worked against the accumulated -fixes, and the final pass reviewed the combined diff. Findings were fixed only -when supported by a concrete failure or contract mismatch. - -| Pass | Area reviewed | Result | -| --- | --- | --- | -| 1 | Frozen Git inventory | Fixed added and modified submodules crashing source extraction when gitlink commits are unavailable locally. | -| 2 | Deeper-look driver | Accepted full SHA-256 commit IDs already supported by the inventory helper. | -| 3 | Updater installation identity | Prevented local checkouts from borrowing another Codex installation's identity, and excluded local marketplaces from automatic updates. | -| 4 | Pending update lifecycle | Rechecked full installation identity at completion and preserved requests for distinct installations, including legacy state records. | -| 5 | Release validation | Rejected stale Claude marketplace versions and invalid stable versions with leading zeroes. | -| 6 | Updater failure paths | Preserved lifecycle tracking across host/configuration/version errors, allowed completion with invalid settings, and refreshed invalid or future-dated release caches. | -| 7 | Inventory configuration and paths | Included gitlinks despite submodule-ignore settings and retained full source evidence when invoked from repository subdirectories. | -| 8 | Scoped updates and locking | Ran and verified Claude project/local updates in their registered project; retried Windows lock contention beyond the native API's short retry window. | -| 9 | Codex update verification | Required installed status and the original scope before reporting success, and ignored uninstalled duplicate rows. | -| 10 | Combined diff and runtime contracts | Aligned updater stable-version validation with release tools; invalid leading-zero and Unicode-digit tags cannot schedule installations. | - -The submodule regressions and SHA-256 integration regression were observed failing -before their fixes. Other focused regressions cover identity changes, distinct -installations, configuration and host failures, cache corruption, project scope, -Git settings, and invalid release identities. - -Final validation: - -- `python3 scripts/check.py`: valid, synchronized version 1.0.7. -- `python3 scripts/test.py`: 77 passed (baseline: 23 passed). -- `git diff --check`: passed. - -Runtime Python remains standard-library-only. Windows lock contention was tested -with a simulated `msvcrt` adapter on Linux; native Windows execution remains -unverified. Host commands and GitHub release responses use controlled fixtures; -no live plugin installation was performed.