From 8a404dbbdc9b9007e23cd92e4e991d17b7191b1c Mon Sep 17 00:00:00 2001 From: haelyra <49814733+haelyra@users.noreply.github.com> Date: Sun, 11 Oct 2026 08:56:29 -0400 Subject: [PATCH] fix: preserve story projections and blank provider defaults --- skills/story-lifecycle/scripts/ledger.js | 18 ++- src/llm/providers/resolver.py | 6 +- .../story-lifecycle-table-projection.test.js | 149 ++++++++++++++++++ ...serve-secret-redaction-environment.test.js | 9 ++ tests/test_resolver.py | 29 ++++ 5 files changed, 205 insertions(+), 6 deletions(-) create mode 100644 tests/docs/story-lifecycle-table-projection.test.js diff --git a/skills/story-lifecycle/scripts/ledger.js b/skills/story-lifecycle/scripts/ledger.js index 0bbba0c71..1b240efed 100644 --- a/skills/story-lifecycle/scripts/ledger.js +++ b/skills/story-lifecycle/scripts/ledger.js @@ -288,10 +288,22 @@ function parseStory(rel, md) { function replaceTable(md, rel, header, rows) { const i = md.indexOf(DERIVED); if (i < 0) throw new LedgerError(`${rel}: missing derived-table marker`); - const start = md.indexOf('\n', i) + 1; + const newline = md.indexOf('\n', i); + const afterMarker = newline < 0 ? md.length : newline + 1; + const skipBlanks = (at) => at + (md.slice(at).match(/^(?:[ \t]*\r?\n)+/) || [''])[0].length; + const hasDerivedHeader = (at) => md.slice(at).split(/\r?\n/, 2).join('\n').replace(/[ \t]/g, '') === header.replace(/[ \t]/g, ''); + const first = skipBlanks(afterMarker); + const start = md[first] === '|' ? first : afterMarker; let end = start; - while (md[end] === '|') { const nl = md.indexOf('\n', end); end = nl < 0 ? md.length : nl + 1; } - return md.slice(0, start) + [header, ...rows].join('\n') + '\n' + md.slice(end); + while (md[end] === '|') { + while (md[end] === '|') { const nl = md.indexOf('\n', end); end = nl < 0 ? md.length : nl + 1; } + // Older projections could insert a second table before a blank-separated + // original. Only adjacent copies of this derived header belong to us. + const next = skipBlanks(end); + if (!hasDerivedHeader(next)) break; + end = next; + } + return md.slice(0, start) + (newline < 0 ? '\n' : '') + [header, ...rows].join('\n') + '\n' + md.slice(end); } // Read the ledger with pending changes overlaid; return regenerated epic/sprint files that differ. diff --git a/src/llm/providers/resolver.py b/src/llm/providers/resolver.py index d4754a288..248e8bf92 100644 --- a/src/llm/providers/resolver.py +++ b/src/llm/providers/resolver.py @@ -69,10 +69,10 @@ def _resolve_provider_config( return _strip_env_value(env_provider).lower(), None saved_config = _read_saved_llm_config() - saved_provider = saved_config.get("LLM_PROVIDER", "claude").lower() + saved_provider = saved_config.get("LLM_PROVIDER", "").lower() # A saved model belongs to its saved provider, never to an override. - saved_model = saved_config.get("LLM_MODEL") if saved_config.get("LLM_PROVIDER") else None - return saved_provider, saved_model + saved_model = saved_config.get("LLM_MODEL") if saved_provider else None + return saved_provider or "claude", saved_model def _resolve_default_model( diff --git a/tests/docs/story-lifecycle-table-projection.test.js b/tests/docs/story-lifecycle-table-projection.test.js new file mode 100644 index 000000000..13c8a09a4 --- /dev/null +++ b/tests/docs/story-lifecycle-table-projection.test.js @@ -0,0 +1,149 @@ +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { execFileSync, spawnSync } = require('child_process'); + +const LEDGER = path.join(__dirname, '..', '..', 'skills', 'story-lifecycle', 'scripts', 'ledger.js'); +const MARKER = ''; +const EPIC_HEADER = '| ID | Title | Status |\n| --- | --- | --- |'; +const SPRINT_HEADER = '| ID | Title | Points | Status |\n| --- | --- | --- | --- |'; +const PREFIX = '# Epic: Auth flow\n\nUser introduction stays here.\n\n'; +const SUFFIX = '\n## Notes\n\nKeep this user prose byte for byte.\n'; +const ownedTemps = new Set(); + +function run(root, args) { + const result = spawnSync(process.execPath, [LEDGER, ...args, '--root', root], { + encoding: 'utf8', timeout: 10000, + }); + assert.strictEqual(result.status, 0, result.stderr || result.error?.message); + return JSON.parse(result.stdout); +} + +function read(root, rel) { + return fs.readFileSync(path.join(root, '.stories', rel), 'utf8'); +} + +function write(root, rel, text) { + fs.writeFileSync(path.join(root, '.stories', rel), text); +} + +function apply(root, changes) { + const input = path.join(root, 'changes.json'); + fs.writeFileSync(input, JSON.stringify(changes)); + return run(root, ['apply', '--input', input]); +} + +function newRepo() { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'story-table-projection-')); + ownedTemps.add(root); + execFileSync('git', ['init', '-q', root]); + run(root, ['init']); + write(root, 'epics/auth-flow.md', `${PREFIX}${MARKER}\n${EPIC_HEADER}\n`); + return root; +} + +function story(id, status = 'todo', sprint = 'unassigned') { + return `# Story: Sign in\n\n**ID:** ${id}\n**Epic:** auth-flow\n**Sprint:** ${sprint}\n**Status:** ${status}\n**Points:** 3\n`; +} + +function assertStable(root, rel, expected) { + assert.strictEqual(read(root, rel), expected); + assert.deepStrictEqual(run(root, ['status']).drift, []); + assert.deepStrictEqual(run(root, ['reconcile']).written, []); + assert.strictEqual(read(root, rel), expected); + assert.ok(!fs.existsSync(path.join(root, '.stories', '.txn'))); + assert.ok(!fs.existsSync(path.join(root, '.stories', '.lock'))); +} + +const tests = [ + ['an EOF marker without a newline appends its table after the marker', () => { + const root = newRepo(); + write(root, 'epics/auth-flow.md', `${PREFIX}${MARKER}`); + const [id] = run(root, ['allocate', '--epic', 'auth-flow']).ids; + apply(root, { create: { [`${id}.md`]: story(id) } }); + assertStable(root, 'epics/auth-flow.md', `${PREFIX}${MARKER}\n${EPIC_HEADER}\n| ${id} | Sign in | todo |\n`); + }], + ['an empty EOF marker with trailing spaces remains after user prose', () => { + const root = newRepo(); + write(root, 'epics/auth-flow.md', `${PREFIX}${MARKER} `); + run(root, ['reconcile']); + assertStable(root, 'epics/auth-flow.md', `${PREFIX}${MARKER} \n${EPIC_HEADER}\n`); + }], + ...['\n', ' \t\n\n', '\r\n \t\r\n'].map(gap => [ + `blank whitespace ${JSON.stringify(gap)} before an epic table is preserved without duplicates`, () => { + const root = newRepo(); + const [id] = run(root, ['allocate', '--epic', 'auth-flow']).ids; + write(root, 'epics/auth-flow.md', `${PREFIX}${MARKER}\n${gap}${EPIC_HEADER}\n| stale-99 | Old row | done |\n${SUFFIX}`); + apply(root, { create: { [`${id}.md`]: story(id) } }); + assertStable(root, 'epics/auth-flow.md', `${PREFIX}${MARKER}\n${gap}${EPIC_HEADER}\n| ${id} | Sign in | todo |\n${SUFFIX}`); + }, + ]), + ['a sprint transition replaces a whitespace-separated table and updates total points', () => { + const root = newRepo(); + const [id] = run(root, ['allocate', '--epic', 'auth-flow']).ids; + const prefix = `# Sprint 1\n\n${MARKER}\n \t\n`; + const suffix = `\n## Total Points: 99\n${SUFFIX}`; + apply(root, { create: { [`${id}.md`]: story(id), + 'sprints/sprint-1.md': `${prefix}${SPRINT_HEADER}\n| stale-99 | Old row | 99 | done |\n${suffix}` } }); + apply(root, { update: { [`${id}.md`]: story(id, 'in-progress', '1') } }); + assertStable(root, 'sprints/sprint-1.md', `${prefix}${SPRINT_HEADER}\n| ${id} | Sign in | 3 | in-progress |\n${suffix.replace('99', '3')}`); + }], + ['reconciliation removes consecutive stale copies of the same derived table', () => { + const root = newRepo(); + const [id] = run(root, ['allocate', '--epic', 'auth-flow']).ids; + apply(root, { create: { [`${id}.md`]: story(id) } }); + const current = `${EPIC_HEADER}\n| ${id} | Sign in | todo |\n`; + const stale = '|ID|Title|Status|\n|---|---|---|\n| stale-99 | Old row | done |\n'; + write(root, 'epics/auth-flow.md', `${PREFIX}${MARKER}\n${current}\n ${'\t'}\n${stale}\n${stale}${SUFFIX}`); + assert.deepStrictEqual(run(root, ['status']).drift, ['epics/auth-flow.md']); + run(root, ['reconcile']); + assertStable(root, 'epics/auth-flow.md', `${PREFIX}${MARKER}\n${current}${SUFFIX}`); + }], + ['unrelated tables and a later table after prose stay byte-identical', () => { + const root = newRepo(); + const [id] = run(root, ['allocate', '--epic', 'auth-flow']).ids; + const userTables = `\n| Owner | Note |\n| --- | --- |\n| User | Keep |\n${SUFFIX}\n${EPIC_HEADER}\n| example-1 | User example | done |\n`; + write(root, 'epics/auth-flow.md', `${PREFIX}${MARKER}\n${EPIC_HEADER}\n${userTables}`); + apply(root, { create: { [`${id}.md`]: story(id) } }); + assertStable(root, 'epics/auth-flow.md', `${PREFIX}${MARKER}\n${EPIC_HEADER}\n| ${id} | Sign in | todo |\n${userTables}`); + }], + ['a stale table ending at EOF is replaced without adding another table', () => { + const root = newRepo(); + write(root, 'epics/auth-flow.md', `${PREFIX}${MARKER}\n\n${EPIC_HEADER}\n| stale-99 | Old row | done |`); + run(root, ['reconcile']); + assertStable(root, 'epics/auth-flow.md', `${PREFIX}${MARKER}\n\n${EPIC_HEADER}\n`); + }], + ['the initial derived table can use compact Markdown header spacing', () => { + const root = newRepo(); + write(root, 'epics/auth-flow.md', `${PREFIX}${MARKER}\n \t\n|ID|Title|Status|\n|---|---|---|\n|stale-99|Old row|done|\n${SUFFIX}`); + run(root, ['reconcile']); + assertStable(root, 'epics/auth-flow.md', `${PREFIX}${MARKER}\n \t\n${EPIC_HEADER}\n${SUFFIX}`); + }], + ['inserting a missing table leaves blank lines and following prose intact', () => { + const root = newRepo(); + const suffix = ` \t\n\n${SUFFIX}`; + write(root, 'epics/auth-flow.md', `${PREFIX}${MARKER}\n${suffix}`); + run(root, ['reconcile']); + assertStable(root, 'epics/auth-flow.md', `${PREFIX}${MARKER}\n${EPIC_HEADER}\n${suffix}`); + }], +]; + +let passed = 0; +let failed = 0; +console.log('\n=== Testing story-lifecycle table projection workflows ===\n'); +for (const [name, test] of tests) { + try { + test(); + console.log(` ✓ ${name}`); + passed++; + } catch (error) { + console.log(` ✗ ${name}\n Error: ${error.message}`); + failed++; + } finally { + for (const dir of ownedTemps) fs.rmSync(dir, { recursive: true, force: true }); + ownedTemps.clear(); + } +} +console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); +process.exitCode = failed > 0 ? 1 : 0; diff --git a/tests/hooks/observe-secret-redaction-environment.test.js b/tests/hooks/observe-secret-redaction-environment.test.js index 5610bc4aa..872d15e81 100644 --- a/tests/hooks/observe-secret-redaction-environment.test.js +++ b/tests/hooks/observe-secret-redaction-environment.test.js @@ -6,6 +6,8 @@ const os = require('node:os'); const path = require('node:path'); const { spawnSync } = require('node:child_process'); +let passed = 0; +let failed = 0; if (process.platform === 'win32') { console.log('Skipping non-Windows Python availability control.'); } else { @@ -19,7 +21,14 @@ if (process.platform === 'win32') { assert.match(result.stdout, /Passed: 0\nFailed: 1/); assert.doesNotMatch(result.stdout, /Skipping shell fixtures/); console.log('PASS missing Python fails the actual non-Windows redaction gate'); + passed += 1; + } catch (error) { + console.log(`FAIL ${error.message}`); + failed += 1; } finally { fs.rmSync(emptyPath, { recursive: true, force: true }); } } +console.log(`Passed: ${passed}`); +console.log(`Failed: ${failed}`); +process.exitCode = failed ? 1 : 0; diff --git a/tests/test_resolver.py b/tests/test_resolver.py index 24c2d80ae..25e6bfcac 100644 --- a/tests/test_resolver.py +++ b/tests/test_resolver.py @@ -258,6 +258,35 @@ class TestSelectedModel: assert get_provider().get_default_model() == "claude-sonnet-4-6" + @pytest.mark.parametrize("saved_provider", [None, "", " ", '""', "' '"]) + def test_missing_or_blank_saved_provider_ignores_unpaired_model( + self, tmp_path: Path, saved_provider: str | None, + ) -> None: + provider_line = "" if saved_provider is None else f"LLM_PROVIDER={saved_provider}\n" + tmp_path.joinpath(".llm.env").write_text(provider_line + "LLM_MODEL=orphan-model\n") + + provider = get_provider() + + assert provider.provider_type == ProviderType.CLAUDE + assert provider.get_default_model() == "claude-sonnet-4-6" + + @pytest.mark.parametrize("explicit", [False, True]) + def test_blank_saved_provider_preserves_provider_and_model_overrides( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path, explicit: bool, + ) -> None: + tmp_path.joinpath(".llm.env").write_text('LLM_PROVIDER=""\nLLM_MODEL=orphan-model\n') + if explicit: + provider = get_provider("ollama", default_model="explicit-model") + expected_model = "explicit-model" + else: + monkeypatch.setenv("LLM_PROVIDER", "ollama") + monkeypatch.setenv("LLM_MODEL", "environment-model") + provider = get_provider() + expected_model = "environment-model" + + assert provider.provider_type == ProviderType.OLLAMA + assert provider.get_default_model() == expected_model + @pytest.mark.parametrize("explicit", [False, True]) def test_provider_override_does_not_read_invalid_saved_file( self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path, explicit: bool,