From 5da9a1f4a76061c60d93a2b464b06cc7d5f71313 Mon Sep 17 00:00:00 2001 From: haelyra <49814733+haelyra@users.noreply.github.com> Date: Sun, 11 Oct 2026 05:33:44 -0400 Subject: [PATCH] fix: preflight ledger storage and normalize saved model padding --- skills/story-lifecycle/SKILL.md | 18 +++++- skills/story-lifecycle/scripts/ledger.js | 52 ++++++++++++++++- src/llm/providers/resolver.py | 2 +- tests/docs/story-lifecycle-ledger.test.js | 71 +++++++++++++++++++++++ tests/test_model_config.py | 62 +++++++++++++++++++- 5 files changed, 200 insertions(+), 5 deletions(-) diff --git a/skills/story-lifecycle/SKILL.md b/skills/story-lifecycle/SKILL.md index 3371c26cc..4965a136f 100644 --- a/skills/story-lifecycle/SKILL.md +++ b/skills/story-lifecycle/SKILL.md @@ -98,6 +98,22 @@ files directly with editor or shell tools: direct writes bypass the lock, ID all and rollback. The helper is plain Node.js with no dependencies and runs on Windows, macOS, and Linux. If `node` is not available, use the ledger read-only and tell the user. +**Storage requirement:** `.stories/` and its subdirectories must stay on the same +writable, hardlink-capable filesystem. FAT32 and exFAT do not support hardlinks. +Before any write command, the helper probes `.stories/`, or its nearest existing +ancestor before initialization, and cleans up only its own probe files. If the +probe fails, initialization, ID allocation, locking, publication and recovery are +refused before ledger mutation. There is no copy fallback: it would weaken +exclusive ownership and crash recovery. A successful probe cannot guarantee that +storage remains available or unchanged during a later write. + +`status` stays read-only, including on unsupported storage. An existing interrupted +transaction still reports `recoveryPending` and suppresses projection; this is +pending work, not a promise that automatic recovery can run on the current storage. +If storage changed while `.txn/` was pending, preserve the journal and files and +use reviewed manual recovery. Do not discard the transaction or retry writes on +unsupported storage. Automatic rollback requires unchanged hardlink-capable storage. + | Command | Effect | | --- | --- | | `init` | Create `.stories/`, `epics/`, `sprints/` and `.stories/.gitignore` | @@ -126,7 +142,7 @@ Guarantees the helper enforces: batch. If a process dies mid-batch, the next write command (or `recover`) restores every backed-up file and deletes every file the batch created. Until then, `status` reports `recoveryPending: true`, and the next write command restores a consistent ledger - before it does anything else. + before it does anything else, after its storage capability check succeeds. - **Stale locks.** A lock whose process is dead (same host) is removed only under a second exclusive lock, `.stories/.lock.break`, after a re-read, so a live lock is never deleted. If a process dies while holding `.lock.break`, writers time out with code `3`; diff --git a/skills/story-lifecycle/scripts/ledger.js b/skills/story-lifecycle/scripts/ledger.js index 6431276ab..0bbba0c71 100644 --- a/skills/story-lifecycle/scripts/ledger.js +++ b/skills/story-lifecycle/scripts/ledger.js @@ -71,6 +71,52 @@ function removeOwned(file, owner) { if (sameFile(file, owner)) fs.unlinkSync(file); } +// Probe the intended ledger volume, before creating directories, locks or journals. +// Hardlinks preserve exclusive publication and rollback inode ownership; copying +// is not an equivalent fallback. Status deliberately remains entirely read-only. +function requireHardlinks(dir) { + let ancestor = dir; + for (;;) { + try { + if (!fs.statSync(ancestor).isDirectory()) throw new LedgerError(`not a directory: ${ancestor}`); + break; + } catch (err) { + if (err.code !== 'ENOENT') throw err; + const parent = path.dirname(ancestor); + if (parent === ancestor) throw err; + ancestor = parent; + } + } + let probe, probeOwner, source, target, owner, fd; + try { + probe = fs.mkdtempSync(path.join(ancestor, '.hardlink-probe-')); + probeOwner = identity(fs.lstatSync(probe)); + source = path.join(probe, 'source'); + target = path.join(probe, 'linked'); + fd = fs.openSync(source, fs.constants.O_WRONLY | fs.constants.O_CREAT | + fs.constants.O_EXCL | fs.constants.O_NOFOLLOW, 0o600); + owner = identity(fs.fstatSync(fd)); + fs.writeFileSync(fd, 'story-ledger hardlink capability probe'); + fs.linkSync(source, target); + if (!sameFile(target, owner)) throw new LedgerError('hardlink probe did not preserve file identity'); + } catch (err) { + throw new LedgerError(`hardlink-capable writable storage required at ${ancestor} (${err.code || err.message}); ` + + 'no ledger write or recovery attempted. If storage changed with a pending transaction, use reviewed manual recovery.'); + } finally { + try { + if (owner) { removeOwned(target, owner); removeOwned(source, owner); } + } finally { + if (fd !== undefined) fs.closeSync(fd); + // Never recursively delete a probe directory: unexpected files belong to + // somebody else and must survive, even when cleanup cannot finish. + if (probe && probeOwner) { + const stat = fs.lstatSync(probe); + if (stat.isDirectory() && stat.dev === probeOwner.dev && stat.ino === probeOwner.ino) fs.rmdirSync(probe); + } + } + } +} + function pidAlive(pid) { try { process.kill(pid, 0); return true; } catch (err) { return err.code === 'EPERM'; } } @@ -388,13 +434,14 @@ function main() { const root = path.resolve(opts.root || '.'); const dir = path.join(root, '.stories'); assertNoSymlinkDirs(dir); - if (cmd === 'init') return init(dir); + if (cmd === 'init') { requireHardlinks(dir); return init(dir); } if (!fs.existsSync(path.join(dir, 'epics'))) throw new LedgerError('no .stories/ ledger; run `init` first'); if (cmd === 'status') { const holder = readLock(dir); const transactionPending = fs.existsSync(path.join(dir, '.txn')); const recoveryPending = transactionPending && (!holder || isStale(holder)); - if (transactionPending) return { stories: [], drift: [], recoveryPending, projectionAvailable: false }; + if (transactionPending) return { stories: [], drift: [], recoveryPending, projectionAvailable: false, + recoveryAdvice: 'Recovery requires unchanged hardlink-capable storage; storage changes require reviewed manual recovery.' }; const { stories, derived } = project(dir); return { stories, drift: Object.keys(derived), recoveryPending }; } @@ -402,6 +449,7 @@ function main() { if (!Number.isSafeInteger(waitMs) || waitMs < 0 || waitMs > 60000) { throw new LedgerError('--wait-ms must be an integer in 0..60000'); } + requireHardlinks(dir); const release = acquireLock(dir, waitMs); try { const rolledBack = recover(dir); diff --git a/src/llm/providers/resolver.py b/src/llm/providers/resolver.py index 5bcb6eb13..d4754a288 100644 --- a/src/llm/providers/resolver.py +++ b/src/llm/providers/resolver.py @@ -39,7 +39,7 @@ LLM_ENV_FILE = ".llm.env" def _strip_env_value(value: str) -> str: value = value.strip() if len(value) >= 2 and value[0] == value[-1] and value[0] in {"'", '"'}: - return value[1:-1] + return value[1:-1].strip() return value diff --git a/tests/docs/story-lifecycle-ledger.test.js b/tests/docs/story-lifecycle-ledger.test.js index 34036abaf..2f9e074bf 100644 --- a/tests/docs/story-lifecycle-ledger.test.js +++ b/tests/docs/story-lifecycle-ledger.test.js @@ -79,6 +79,28 @@ function symlink(target, file, type) { const read = (root, rel) => fs.readFileSync(path.join(root, '.stories', rel), 'utf8'); const storyFiles = (root) => fs.readdirSync(path.join(root, '.stories')).filter((f) => /-\d+\.md$/.test(f)).sort(); +function snapshot(dir) { + if (!fs.existsSync(dir)) return null; + return Object.fromEntries(fs.readdirSync(dir).sort().map((name) => { + const file = path.join(dir, name); + return [name, fs.lstatSync(file).isDirectory() ? snapshot(file) : fs.readFileSync(file, 'base64')]; + })); +} + +function withoutHardlinks(root, args, { expectedAncestor = null, code = 'ENOTSUP' } = {}) { + const preload = path.join(root, 'unsupported-links.js'); + fs.writeFileSync(preload, `const fs = require('fs'); const path = require('path'); +fs.linkSync = function (source) { + const ancestor = ${JSON.stringify(expectedAncestor)}; + if (ancestor && path.dirname(path.dirname(source)) !== ancestor) { + throw new Error('probe used wrong filesystem: ' + source); + } + throw Object.assign(new Error('injected unsupported hardlinks'), { code: ${JSON.stringify(code)} }); +};`); + const r = spawnSync(process.execPath, ['-r', preload, LEDGER, ...args], { encoding: 'utf8' }); + return { code: r.status, out: r.stdout ? JSON.parse(r.stdout) : null, err: r.stderr }; +} + // One full create-stories run (allocate + apply) in a separate process. const WRITER = ` const { execFileSync } = require('child_process'); @@ -93,6 +115,55 @@ run('apply', '--input', root + '/' + tag + '.json'); `; const tests = [ + ['unsupported storage refuses init on the nearest existing ancestor without creating the ledger', () => { + const root = tempDir('story-storage-'); + const intended = path.join(root, 'missing', 'repo'); + const result = withoutHardlinks(root, ['init', '--root', intended], { expectedAncestor: root }); + assert.strictEqual(result.code, 2, result.err); + assert.match(result.err, /hardlink.*ENOTSUP/); + assert.deepStrictEqual(fs.readdirSync(root), ['unsupported-links.js']); + assert.ok(!fs.existsSync(path.join(root, 'missing'))); + }], + + ['unsupported storage refuses every write before touching ledger files or ID reservations', () => { + const root = newRepo(); + const { input } = createStories(root, ['Unpublished']); + fs.writeFileSync(path.join(root, '.stories', '.hardlink-probe-user'), 'keep user file'); + const ledgerBefore = snapshot(path.join(root, '.stories')); + const idsBefore = snapshot(path.join(root, '.git', 'story-lifecycle')); + for (const args of [['init'], ['allocate', '--epic', 'auth-flow'], + ['apply', '--input', input], ['reconcile'], ['recover']]) { + const result = withoutHardlinks(root, [...args, '--root', root], { + expectedAncestor: path.join(root, '.stories'), code: 'EPERM', + }); + assert.strictEqual(result.code, 2, `${args}: ${result.err}`); + assert.match(result.err, /hardlink.*EPERM/); + assert.deepStrictEqual(snapshot(path.join(root, '.stories')), ledgerBefore); + assert.deepStrictEqual(snapshot(path.join(root, '.git', 'story-lifecycle')), idsBefore); + } + }], + + ['storage change leaves pending recovery untouched and status truthful until supported recovery', () => { + const root = newRepo(); + const { ids, input } = createStories(root, ['Partially published']); + assert.strictEqual(run(root, ['apply', '--input', input], { STORY_LEDGER_CRASH_AFTER: '1' }).code, 70); + const before = snapshot(path.join(root, '.stories')); + const result = withoutHardlinks(root, ['recover', '--root', root]); + assert.strictEqual(result.code, 2, result.err); + assert.match(result.err, /hardlink.*ENOTSUP/); + assert.deepStrictEqual(snapshot(path.join(root, '.stories')), before, 'no partial rollback or lock cleanup'); + const status = withoutHardlinks(root, ['status', '--root', root]); + assert.strictEqual(status.code, 0, status.err); + assert.strictEqual(status.out.recoveryPending, true); + assert.strictEqual(status.out.projectionAvailable, false); + assert.match(status.out.recoveryAdvice, /storage.*manual/i); + assert.deepStrictEqual(snapshot(path.join(root, '.stories')), before, 'status must be read-only'); + assert.ok(fs.existsSync(path.join(root, '.stories', `${ids[0]}.md`))); + assert.strictEqual(run(root, ['recover']).code, 0); + assert.strictEqual(run(root, ['apply', '--input', input]).code, 0); + assert.deepStrictEqual(run(root, ['status']).out.drift, []); + }], + ['create-stories writes story files and regenerates the epic table', () => { const root = newRepo(); const { ids, input } = createStories(root, ['Login', 'Logout']); diff --git a/tests/test_model_config.py b/tests/test_model_config.py index 13eb2a5a6..c40820d0f 100644 --- a/tests/test_model_config.py +++ b/tests/test_model_config.py @@ -1,13 +1,16 @@ +import json from collections.abc import Callable from dataclasses import dataclass +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer from pathlib import Path +from threading import Thread from typing import Literal import pytest from llm.cli.selector import save_config from llm.core.interface import LLMProvider -from llm.core.types import ProviderType +from llm.core.types import LLMInput, Message, ProviderType, Role from llm.providers.astraflow import AstraflowCNProvider, AstraflowProvider from llm.providers.atlas import AtlasProvider from llm.providers.ollama import OllamaProvider @@ -127,3 +130,60 @@ def test_unset_constructor_model_uses_selected_model( monkeypatch.setenv("LLM_MODEL", "Selected-Case-Model") provider = get_provider("openai", api_key="test-key", default_model=default_model) assert provider.get_default_model() == "Selected-Case-Model" + + +@pytest.mark.parametrize("quote", ('"', "'")) +@pytest.mark.parametrize("source", ("saved", "environment")) +def test_padded_quoted_selection_is_normalized( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, quote: str, source: str, +) -> None: + provider_value = f" {quote} OLLAMA {quote} " + model_value = f" {quote} Selected/MixedCase-model {quote} " + if source == "saved": + tmp_path.joinpath(".llm.env").write_text( + f"LLM_PROVIDER={provider_value}\nLLM_MODEL={model_value}\n" + ) + else: + monkeypatch.setenv("LLM_PROVIDER", provider_value) + monkeypatch.setenv("LLM_MODEL", model_value) + + assert get_provider().get_default_model() == "Selected/MixedCase-model" + + +def test_saved_quoted_blank_model_retains_builtin_default(tmp_path: Path) -> None: + tmp_path.joinpath(".llm.env").write_text('LLM_PROVIDER=ollama\nLLM_MODEL=" \t "\n') + assert get_provider().get_default_model() == "llama3.2" + + +@pytest.mark.parametrize("quote", ('"', "'")) +def test_saved_padded_model_reaches_real_http_request(tmp_path: Path, quote: str) -> None: + requests = [] + + class Handler(BaseHTTPRequestHandler): + def do_POST(self) -> None: + requests.append((self.path, json.loads(self.rfile.read(int(self.headers["Content-Length"]))))) + self.send_response(200) + self.end_headers() + self.wfile.write(b'{"message":{"content":"ok"},"model":"Selected/MixedCase-model"}') + + def log_message(self, format: str, *args: object) -> None: + pass + + tmp_path.joinpath(".llm.env").write_text( + f"LLM_PROVIDER=ollama\nLLM_MODEL={quote} Selected/MixedCase-model {quote}\n" + ) + server = ThreadingHTTPServer(("127.0.0.1", 0), Handler) + thread = Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + provider = get_provider(base_url=f"http://127.0.0.1:{server.server_port}") + response = provider.generate(LLMInput(messages=[Message(Role.USER, "hello")])) + assert response.content == "ok" + assert requests[0][0] == "/api/chat" + assert requests[0][1]["model"] == "Selected/MixedCase-model" + provider.generate(LLMInput(messages=[], model="explicit-request-model")) + assert requests[1][1]["model"] == "explicit-request-model" + finally: + server.shutdown() + thread.join(timeout=5) + server.server_close()