From 83be9d113e8310789c281f7c8a00ed4fad191dd5 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Tue, 11 Aug 2026 15:53:06 -0500 Subject: [PATCH] feat(validate): add --archived to lint task completion of archived changes (#1604) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(validate): add --archived to lint task completion of archived changes `openspec validate --archived` scans every change under changes/archive/ and fails (exit 1) if any has unchecked tasks in tasks.md. This catches changes archived with unfinished work — which the normal validate flow never sees, since it only looks at active changes — and is meant for a pre-commit or CI hook. It is a standalone, opt-in scope: it returns before any existing bulk path, so no current `validate` invocation changes behavior, and it does not re-validate already-applied spec deltas. Reuses getTaskProgressForChange (the same counter status/list/archive use) so task counting never forks, and reads root.archiveDir so it is store-aware. Closes #205 Co-Authored-By: Claude Opus 4.8 * fix(validate): fail loudly on archive read errors and unreadable task files Address adversarial + CodeRabbit review of `validate --archived`: - listArchivedChangeIds now returns [] only for ENOENT (missing archive dir) and rethrows permission/I/O/ENOTDIR errors, so a real archive-read failure exits 1 instead of silently reading as "no archived changes". - Add getTaskProgressDetailForChange, which reports task files that exist but cannot be read; --archived turns those into an ERROR (naming the file) rather than silently counting them as zero tasks. The shared getTaskProgressForChange now wraps it and drops the detail, so status/list/archive totals are byte-identical. - Start the spinner after listing so a thrown listing error never leaves a spinner running. Adds regression tests (archive path is a file; archived tasks.md is unreadable) and unit tests for the new detail variant. Docs: align the --archived table verb and add a troubleshooting one-liner. Co-Authored-By: Claude Opus 4.8 * test(validate): address CodeRabbit nits on archived-task tests - Build the unreadable-fixture path from separate path.join components instead of a hard-coded Unix-separator string. - Assert the reported unreadable path (canonicalized with realpathSync.native), not just the count, so a wrong path can't pass. Co-Authored-By: Claude Opus 4.8 * refactor(validate): address round-3 review of --archived From three fresh adversarial reviews (scale/perf, flag/output-shape, filesystem/security): - perf: memoize schema→glob resolution across archived changes via a run-scoped SchemaGlobCache, so the same schema.yaml isn't re-parsed once per change (the archive is append-only and can hold thousands). Threaded as an optional arg; existing callers are unchanged. Loop stays sequential by design (per-change work is synchronous) — now documented. - output shape: issue `path` now follows validate's convention — 'tasks.md' for incomplete tasks, and the POSIX root-relative file path for an unreadable file (one issue per file) instead of the bare 'tasks'. - plain output: print `change/` (matching the JSON `type` and bulk validation) instead of `archived/`. - docs: correct the "Never throws" docstrings (glob resolution can throw on a malformed/unsafe schema; the caller guards it) and note the load-bearing projectRoot override for the archive path depth. Store-mode resolution confirmed correct by review. Tests updated + a memo regression test added. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- .changeset/validate-archived-tasks.md | 5 + docs/cli.md | 6 + docs/troubleshooting.md | 1 + src/cli/index.ts | 3 +- src/commands/validate.ts | 140 ++++++++++++++++ src/core/completions/command-registry.ts | 4 + src/utils/task-progress.ts | 127 +++++++++++---- test/cli-e2e/validate-archived-tasks.test.ts | 160 +++++++++++++++++++ test/utils/task-progress.test.ts | 75 ++++++++- 9 files changed, 485 insertions(+), 36 deletions(-) create mode 100644 .changeset/validate-archived-tasks.md create mode 100644 test/cli-e2e/validate-archived-tasks.test.ts diff --git a/.changeset/validate-archived-tasks.md b/.changeset/validate-archived-tasks.md new file mode 100644 index 00000000..f857c892 --- /dev/null +++ b/.changeset/validate-archived-tasks.md @@ -0,0 +1,5 @@ +--- +"@fission-ai/openspec": minor +--- + +Add `openspec validate --archived`: an opt-in check that every change under `changes/archive/` has all of its `tasks.md` checkboxes ticked, exiting non-zero if any are unchecked. This surfaces changes that were archived with unfinished work — which the normal validate flow never catches, because it only looks at active changes — and is meant for a pre-commit or CI hook (#205). It is a standalone scope: it does not alter any existing `validate` invocation and does not re-validate already-applied spec deltas. diff --git a/docs/cli.md b/docs/cli.md index c294aa32..d00d3038 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -553,12 +553,15 @@ A change with zero spec deltas fails validation unless its `.openspec.yaml` decl | `--all` | Validate all changes and specs | | `--changes` | Validate all changes | | `--specs` | Validate all specs | +| `--archived` | Validate that archived changes have all tasks completed (for pre-commit linting) | | `--type ` | Specify type when name is ambiguous: `change` or `spec` | | `--strict` | Enable strict validation mode | | `--json` | Output as JSON | | `--concurrency ` | Max parallel validations (default: 6, or `OPENSPEC_CONCURRENCY` env) | | `--no-interactive` | Disable prompts | +`--archived` is its own scope: it does not validate spec deltas (already applied at archive time), it verifies that every change under `changes/archive/` has all of its `tasks.md` checkboxes ticked, exiting non-zero if any are unchecked. This catches changes that were archived with unfinished work — handy in a pre-commit hook. + **Examples:** ```bash @@ -576,6 +579,9 @@ openspec validate --all --json # Strict validation with increased parallelism openspec validate --all --strict --concurrency 12 + +# Fail if any archived change still has unchecked tasks +openspec validate --archived ``` **Output (text):** diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index b6e65eec..62f6be04 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -92,6 +92,7 @@ Validation checks your specs and changes for structural problems. Read the messa openspec validate # validate one item openspec validate --all # validate everything openspec validate --all --strict # stricter checks, good for CI +openspec validate --archived # fail if archived changes have unchecked tasks ``` Common causes are a missing required section (like a spec with no scenarios) or a malformed delta header. Fix the file and re-run. The [CLI reference](cli.md#openspec-validate) documents the output format. diff --git a/src/cli/index.ts b/src/cli/index.ts index 619f958e..6772643c 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -439,6 +439,7 @@ program .option('--all', 'Validate all changes and specs') .option('--changes', 'Validate all changes') .option('--specs', 'Validate all specs') + .option('--archived', 'Validate that archived changes have all tasks completed (for pre-commit linting)') .option('--type ', 'Specify item type when ambiguous: change|spec') .option('--strict', 'Enable strict validation mode') .option('--json', 'Output validation results as JSON') @@ -446,7 +447,7 @@ program .option('--no-interactive', 'Disable interactive prompts') .option('--store ', STORE_OPTION_DESCRIPTION) .addOption(hiddenStorePathOption()) - .action(async (itemName?: string, options?: { all?: boolean; changes?: boolean; specs?: boolean; type?: string; strict?: boolean; json?: boolean; noInteractive?: boolean; concurrency?: string; store?: string; storePath?: string }) => { + .action(async (itemName?: string, options?: { all?: boolean; changes?: boolean; specs?: boolean; archived?: boolean; type?: string; strict?: boolean; json?: boolean; noInteractive?: boolean; concurrency?: string; store?: string; storePath?: string }) => { try { const validateCommand = new ValidateCommand(); await validateCommand.execute(itemName, options); diff --git a/src/commands/validate.ts b/src/commands/validate.ts index 7c474cd0..69cd033a 100644 --- a/src/commands/validate.ts +++ b/src/commands/validate.ts @@ -13,6 +13,9 @@ import { isInteractive, resolveNoInteractive } from '../utils/interactive.js'; import { getSpecIds } from '../utils/item-discovery.js'; import { getAvailableChanges } from './workflow/shared.js'; import { nearestMatches } from '../utils/match.js'; +import { promises as fs } from 'fs'; +import { getTaskProgressDetailForChange, type SchemaGlobCache } from '../utils/task-progress.js'; +import { FileSystemUtils } from '../utils/file-system.js'; type ItemType = 'change' | 'spec'; @@ -20,6 +23,7 @@ interface ExecuteOptions { all?: boolean; changes?: boolean; specs?: boolean; + archived?: boolean; type?: string; strict?: boolean; json?: boolean; @@ -47,6 +51,18 @@ export class ValidateCommand { const interactive = isInteractive(options); + // Archived-task linting is its own scope: it checks task completion of + // already-archived changes, not delta specs (whose operations are already + // applied). Handled before the other bulk flags so `--archived` is explicit + // and never alters an existing invocation's behavior (#205). + if (options.archived) { + await this.runArchivedTaskValidation(root, { + json: !!options.json, + noInteractive: resolveNoInteractive(options), + }); + return; + } + // Handle bulk flags first if (options.all || options.changes || options.specs) { await this.runBulkValidation(root, { @@ -387,6 +403,130 @@ export class ValidateCommand { process.exitCode = failed > 0 ? 1 : 0; } + + /** + * Lists archived change ids from the resolved root's archive directory, + * mirroring `getArchivedChangeIds` but store-aware (uses `root.archiveDir` + * rather than a cwd-relative path). Directories only, hidden entries skipped. + * + * Only a missing archive directory (ENOENT) is an empty list; a permission + * error, an I/O error, or an `archive` path that is a file (ENOTDIR) is a real + * failure and must not read as "no archived changes" — that would let a + * pre-commit lint pass without inspecting anything (#205). + */ + private async listArchivedChangeIds(root: ResolvedOpenSpecRoot): Promise { + try { + const entries = await fs.readdir(root.archiveDir, { withFileTypes: true }); + return entries + .filter((entry) => entry.isDirectory() && !entry.name.startsWith('.')) + .map((entry) => entry.name) + .sort(); + } catch (error: any) { + if (error?.code === 'ENOENT') return []; + throw error; + } + } + + /** + * Validates that every archived change has all of its tasks completed. + * + * An archived change is expected to be finished; an archived change with + * unchecked tasks is a real integrity problem the normal validate flow never + * surfaces, because active-change discovery excludes the archive directory + * (#205). Reuses the same task-progress counting `status`, `list`, and + * `archive` rely on, so what counts as a task never forks. Changes with no + * tasks pass (nothing to complete). + */ + private async runArchivedTaskValidation( + root: ResolvedOpenSpecRoot, + opts: { json: boolean; noInteractive?: boolean } + ): Promise { + // List first (may throw on a real archive-read failure), then start the + // spinner so a thrown error never leaves a spinner spinning. + const ids = await this.listArchivedChangeIds(root); + const spinner = !opts.json && !opts.noInteractive ? ora('Validating archived changes...').start() : undefined; + + // The archive is append-only and can hold thousands of changes; a single + // run resolves them all under one constant projectRoot (root.path), so + // memoize the schema→glob lookup to avoid re-parsing the same schema.yaml + // once per change. The loop is intentionally sequential: the per-change work + // is dominated by synchronous schema/config resolution, which a promise pool + // cannot overlap on Node's single thread — a pool would add complexity for + // no real gain here. + const schemaGlobCache: SchemaGlobCache = new Map(); + const results: BulkItemResult[] = []; + let passed = 0; + let failed = 0; + for (const id of ids) { + const start = Date.now(); + const issues: BulkItemResult['issues'] = []; + try { + // The explicit root.path override is load-bearing: an archived change + // lives one directory deeper (changes/archive/), so the default + // "../../.." projectRoot derivation would be wrong without it. + const progress = await getTaskProgressDetailForChange(root.archiveDir, id, root.path, schemaGlobCache); + // A tasks file that exists but cannot be read must fail loudly, not be + // silently counted as "no tasks" and pass. Report one issue per file, + // pathed like every other validate issue (POSIX, root-relative). + for (const file of progress.unreadable) { + issues.push({ + level: 'ERROR', + path: FileSystemUtils.toPosixPath(path.relative(root.path, file)), + message: 'could not read task file', + }); + } + const incomplete = Math.max(progress.total - progress.completed, 0); + if (incomplete > 0) { + issues.push({ + level: 'ERROR', + path: 'tasks.md', + message: `${incomplete} incomplete task${incomplete === 1 ? '' : 's'} (${progress.completed}/${progress.total} completed)`, + }); + } + } catch (error: any) { + issues.push({ level: 'ERROR', path: 'tasks.md', message: error?.message || 'Unknown error' }); + } + const valid = issues.length === 0; + if (valid) passed++; else failed++; + results.push({ id, type: 'change', valid, issues, durationMs: Date.now() - start }); + } + + spinner?.stop(); + + const summary = { + totals: { items: results.length, passed, failed }, + byType: { change: summarizeType(results, 'change') }, + } as const; + + if (opts.json) { + const out = { items: results, summary, version: '1.0', root: toRootOutput(root) }; + console.log(JSON.stringify(out, null, 2)); + process.exitCode = failed > 0 ? 1 : 0; + return; + } + + if (results.length === 0) { + console.log('No archived changes found.'); + process.exitCode = 0; + return; + } + + // Use the same `/` prefix bulk validation prints, so the plain + // output maps to the JSON `type` ('change') and stays greppable the same way. + for (const res of results) { + if (res.valid) { + console.log(`✓ change/${res.id}`); + } else { + console.error(`✗ change/${res.id}`); + for (const issue of res.issues) { + const prefix = issue.level === 'ERROR' ? '✗' : issue.level === 'WARNING' ? '⚠' : 'ℹ'; + console.error(` ${prefix} ${issue.message}`); + } + } + } + console.log(`Totals: ${summary.totals.passed} passed, ${summary.totals.failed} failed (${summary.totals.items} items)`); + process.exitCode = failed > 0 ? 1 : 0; + } } function summarizeType(results: BulkItemResult[], type: ItemType) { diff --git a/src/core/completions/command-registry.ts b/src/core/completions/command-registry.ts index 2d139b30..0fd3c02b 100644 --- a/src/core/completions/command-registry.ts +++ b/src/core/completions/command-registry.ts @@ -98,6 +98,10 @@ export const COMMAND_REGISTRY: CommandDefinition[] = [ name: 'specs', description: 'Validate all specs', }, + { + name: 'archived', + description: 'Validate that archived changes have all tasks completed (for pre-commit linting)', + }, COMMON_FLAGS.type, COMMON_FLAGS.strict, COMMON_FLAGS.jsonValidation, diff --git a/src/utils/task-progress.ts b/src/utils/task-progress.ts index a9d75548..e3ebf56a 100644 --- a/src/utils/task-progress.ts +++ b/src/utils/task-progress.ts @@ -79,36 +79,77 @@ function findTrackedTasksArtifact(schema: SchemaYaml): Artifact | undefined { return schema.artifacts.find((a) => a.id === 'tasks'); } +/** + * Run-scoped memo mapping a schema name to its tracked-tasks `generates` glob. + * When one command resolves progress for many changes under a constant + * `projectRoot` — e.g. `validate --archived` over an append-only archive — this + * avoids re-reading and re-parsing (YAML + Zod) the same `schema.yaml` once per + * change. Keyed by schema name alone, which is safe *only* because a single run + * holds `projectRoot` constant; never reuse one cache across differing roots. + */ +export type SchemaGlobCache = Map; + /** * Resolves the tracked-tasks artifact's output glob for a change, or undefined * when the schema cannot be resolved or no tracked-tasks artifact exists. * `resolveSchema` throws on an unresolvable/misnamed schema; we swallow that so * the caller falls back to a single top-level `tasks.md` and never crashes. + * A `schemaGlobCache`, when supplied, memoizes the schema-name → glob lookup for + * the duration of one run. */ -function resolveTrackedTasksGlob(changeDir: string, projectRoot: string): string | undefined { +function resolveTrackedTasksGlob( + changeDir: string, + projectRoot: string, + schemaGlobCache?: SchemaGlobCache +): string | undefined { try { const schemaName = resolveSchemaForChange(changeDir, undefined, projectRoot); + if (schemaGlobCache?.has(schemaName)) return schemaGlobCache.get(schemaName); const schema = resolveSchema(schemaName, projectRoot); - return findTrackedTasksArtifact(schema)?.generates; + const generates = findTrackedTasksArtifact(schema)?.generates; + schemaGlobCache?.set(schemaName, generates); + return generates; } catch { return undefined; } } -async function countSingleTopLevelTasksFile(changeDir: string): Promise { - const tasksPath = path.join(changeDir, 'tasks.md'); - try { - const content = await fs.readFile(tasksPath, 'utf-8'); - return countTasksFromContent(content); - } catch { - return { total: 0, completed: 0 }; - } +/** Resolves the task files selected by the schema's apply tracking rule. */ +export function resolveTaskFilesForChange( + changeDir: string, + projectRoot: string, + schemaGlobCache?: SchemaGlobCache +): string[] { + const generates = resolveTrackedTasksGlob(changeDir, projectRoot, schemaGlobCache); + return generates ? resolveArtifactOutputs(changeDir, generates) : []; } -/** Resolves the task files selected by the schema's apply tracking rule. */ -export function resolveTaskFilesForChange(changeDir: string, projectRoot: string): string[] { - const generates = resolveTrackedTasksGlob(changeDir, projectRoot); - return generates ? resolveArtifactOutputs(changeDir, generates) : []; +export interface TaskProgressDetail extends TaskProgress { + /** + * Task files that exist but could not be read (any error other than ENOENT). + * `getTaskProgressForChange` discards this list to preserve its behavior; + * callers that must fail loudly on an unreadable tasks file — e.g. + * `openspec validate --archived` — read it so an unreadable file is never + * silently counted as "no tasks" (#205). + */ + unreadable: string[]; +} + +/** + * Reads one task file and counts its checkboxes. ENOENT (a glob file that + * vanished between resolve and read, or the absent single top-level `tasks.md`) + * means zero tasks, exactly as before. Any other error (permissions, I/O, + * ENOTDIR) is recorded in `unreadable` so a caller can surface it; the count + * still contributes zero, so existing callers see no change. + */ +async function countTaskFile(file: string, unreadable: string[]): Promise { + try { + const content = await fs.readFile(file, 'utf-8'); + return countTasksFromContent(content); + } catch (error: any) { + if (error?.code !== 'ENOENT') unreadable.push(file); + return { total: 0, completed: 0 }; + } } /** @@ -118,32 +159,50 @@ export function resolveTaskFilesForChange(changeDir: string, projectRoot: string * artifact (`resolveArtifactOutputs`) — so progress is no longer blind to nested * `tasks.md` files (#1202). Falls back to a single top-level `tasks.md` (exactly * as before) when the schema is unresolvable, no tracked-tasks artifact is found, - * or the glob matches no file. Never throws. + * or the glob matches no file. Also reports task files that exist but could not + * be read. Per-file read errors are captured (never thrown); the only throw path + * is a malformed/unsafe schema whose glob resolution rejects (path traversal or + * a linked-directory cycle in `resolveArtifactOutputs`). Pass `schemaGlobCache` + * to memoize schema→glob resolution across many changes in one run. + */ +export async function getTaskProgressDetailForChange( + changesDir: string, + changeName: string, + projectRoot: string, + schemaGlobCache?: SchemaGlobCache +): Promise { + const changeDir = path.join(changesDir, changeName); + const files = resolveTaskFilesForChange(changeDir, projectRoot, schemaGlobCache); + const targets = files.length > 0 ? files : [path.join(changeDir, 'tasks.md')]; + const unreadable: string[] = []; + let total = 0; + let completed = 0; + for (const file of targets) { + const progress = await countTaskFile(file, unreadable); + total += progress.total; + completed += progress.completed; + } + return { total, completed, unreadable }; +} + +/** + * The task-completion counter `status`, `list`, and `archive` share. Delegates + * to `getTaskProgressDetailForChange` and drops the `unreadable` detail, so its + * returned totals are unchanged. Throws only on the same malformed/unsafe-schema + * glob-resolution path as that function (existing behavior; callers guard it as + * they did before). */ export async function getTaskProgressForChange( changesDir: string, changeName: string, projectRoot: string ): Promise { - const changeDir = path.join(changesDir, changeName); - const files = resolveTaskFilesForChange(changeDir, projectRoot); - if (files.length > 0) { - let total = 0; - let completed = 0; - for (const file of files) { - try { - const content = await fs.readFile(file, 'utf-8'); - const progress = countTasksFromContent(content); - total += progress.total; - completed += progress.completed; - } catch { - // Swallow files that vanish between glob and read, as before. - } - } - return { total, completed }; - } - - return countSingleTopLevelTasksFile(changeDir); + const { total, completed } = await getTaskProgressDetailForChange( + changesDir, + changeName, + projectRoot + ); + return { total, completed }; } export function formatTaskStatus(progress: TaskProgress): string { diff --git a/test/cli-e2e/validate-archived-tasks.test.ts b/test/cli-e2e/validate-archived-tasks.test.ts new file mode 100644 index 00000000..7ea5ae84 --- /dev/null +++ b/test/cli-e2e/validate-archived-tasks.test.ts @@ -0,0 +1,160 @@ +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { promises as fs } from 'fs'; +import path from 'path'; +import { tmpdir } from 'os'; +import { runCLI } from '../helpers/run-cli.js'; + +describe('openspec validate --archived checks archived task completion (#205)', () => { + let projectDir: string; + + const write = async (relative: string, content: string) => { + const file = path.join(projectDir, relative); + await fs.mkdir(path.dirname(file), { recursive: true }); + await fs.writeFile(file, content, 'utf-8'); + }; + + beforeAll(async () => { + projectDir = await fs.mkdtemp(path.join(tmpdir(), 'openspec-archived-tasks-e2e-')); + + // Fully completed archived change. + await write( + 'openspec/changes/archive/2026-01-01-done-change/tasks.md', + ['# Tasks', '', '- [x] 1.1 do a', '- [x] 1.2 do b', ''].join('\n') + ); + + // Archived change with an unchecked nested sub-task. + await write( + 'openspec/changes/archive/2026-01-02-incomplete-change/tasks.md', + [ + '# Tasks', + '', + '- [x] 1.1 do a', + '- [ ] 1.2 do b', + ' - [ ] 1.2.1 nested unfinished work', + '', + ].join('\n') + ); + + // An active change with unchecked tasks must NOT be scanned by --archived. + await write( + 'openspec/changes/active-change/tasks.md', + ['# Tasks', '', '- [ ] 1.1 still in progress', ''].join('\n') + ); + }); + + afterAll(async () => { + await fs.rm(projectDir, { recursive: true, force: true }); + }); + + it('fails when an archived change has unchecked tasks and passes the complete one', async () => { + const result = await runCLI(['validate', '--archived', '--json'], { + cwd: projectDir, + }); + + expect(result.exitCode).toBe(1); + const report = JSON.parse(result.stdout); + const byId = Object.fromEntries( + report.items.map((item: { id: string; valid: boolean }) => [item.id, item.valid]) + ); + + // Only archived changes are considered; the active change is absent. + expect(byId['active-change']).toBeUndefined(); + expect(byId['2026-01-01-done-change']).toBe(true); + expect(byId['2026-01-02-incomplete-change']).toBe(false); + + const incomplete = report.items.find( + (item: { id: string }) => item.id === '2026-01-02-incomplete-change' + ); + // The unchecked nested sub-task counts, so 2 of 3 tasks are open. + expect(incomplete.issues[0]).toEqual( + expect.objectContaining({ + level: 'ERROR', + path: 'tasks.md', + message: expect.stringContaining('2 incomplete tasks (1/3 completed)'), + }) + ); + }); + + it('exits 0 when every archived change is complete', async () => { + await fs.rm( + path.join( + projectDir, + 'openspec/changes/archive/2026-01-02-incomplete-change' + ), + { recursive: true, force: true } + ); + + const result = await runCLI(['validate', '--archived'], { cwd: projectDir }); + + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain('✓ change/2026-01-01-done-change'); + }); + + it('exits 0 with a friendly message when there is no archive directory', async () => { + const emptyDir = await fs.mkdtemp(path.join(tmpdir(), 'openspec-no-archive-e2e-')); + await fs.mkdir(path.join(emptyDir, 'openspec', 'changes'), { recursive: true }); + await fs.mkdir(path.join(emptyDir, 'openspec', 'specs'), { recursive: true }); + + const result = await runCLI(['validate', '--archived'], { cwd: emptyDir }); + + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain('No archived changes found.'); + await fs.rm(emptyDir, { recursive: true, force: true }); + }); + + it('fails instead of passing silently when the archive path is not a directory', async () => { + const dir = await fs.mkdtemp(path.join(tmpdir(), 'openspec-archive-notdir-e2e-')); + await fs.mkdir(path.join(dir, 'openspec', 'changes'), { recursive: true }); + await fs.mkdir(path.join(dir, 'openspec', 'specs'), { recursive: true }); + // A real read failure (ENOTDIR) must not read as "no archived changes". + await fs.writeFile( + path.join(dir, 'openspec', 'changes', 'archive'), + 'not a directory\n' + ); + + const result = await runCLI(['validate', '--archived'], { cwd: dir }); + + expect(result.exitCode).toBe(1); + expect(result.stdout).not.toContain('No archived changes found.'); + await fs.rm(dir, { recursive: true, force: true }); + }); + + it('fails when an archived tasks file exists but cannot be read', async () => { + const dir = await fs.mkdtemp(path.join(tmpdir(), 'openspec-archive-unreadable-e2e-')); + await fs.mkdir(path.join(dir, 'openspec', 'specs'), { recursive: true }); + // A tasks.md that is a directory triggers a non-ENOENT read error (EISDIR) + // on every platform, standing in for a genuinely unreadable file. It must + // be reported, not silently counted as "no tasks". + await fs.mkdir( + path.join( + dir, + 'openspec', + 'changes', + 'archive', + 'unreadable-change', + 'tasks.md' + ), + { recursive: true } + ); + + const result = await runCLI(['validate', '--archived', '--json'], { + cwd: dir, + }); + + expect(result.exitCode).toBe(1); + const report = JSON.parse(result.stdout); + const item = report.items.find( + (i: { id: string }) => i.id === 'unreadable-change' + ); + expect(item.valid).toBe(false); + expect(item.issues[0]).toEqual( + expect.objectContaining({ + level: 'ERROR', + // Pathed like every other validate issue: POSIX, root-relative. + path: 'openspec/changes/archive/unreadable-change/tasks.md', + message: 'could not read task file', + }) + ); + await fs.rm(dir, { recursive: true, force: true }); + }); +}); diff --git a/test/utils/task-progress.test.ts b/test/utils/task-progress.test.ts index 501f9b94..c94021b0 100644 --- a/test/utils/task-progress.test.ts +++ b/test/utils/task-progress.test.ts @@ -1,10 +1,11 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { promises as fs } from 'fs'; +import { promises as fs, realpathSync } from 'fs'; import path from 'path'; import os from 'os'; import { countTasksFromContent, getTaskProgressForChange, + getTaskProgressDetailForChange, parseTaskLines, } from '../../src/utils/task-progress.js'; import { resolveArtifactOutputs } from '../../src/core/artifact-graph/index.js'; @@ -115,6 +116,25 @@ describe('getTaskProgressForChange (#1202 tracked-tasks resolution)', () => { expect(progress).toEqual({ total: 2, completed: 1 }); }); + it('memoizes schema→glob resolution across changes via the shared cache (#205)', async () => { + await writeGlobSchema(); + await writeChange('c1', { 'backend/tasks.md': '- [x] a\n' }); + await writeChange('c2', { 'backend/tasks.md': '- [ ] b\n' }); + + const cache = new Map(); + const d1 = await getTaskProgressDetailForChange(changesDir, 'c1', projectRoot, cache); + // The schema→glob lookup is now cached under the resolved schema name. + expect(cache.get('glob-tasks')).toBe('**/tasks.md'); + expect(cache.size).toBe(1); + + // A second change on the same schema reuses the entry (no new key added), + // and results are still correct. + const d2 = await getTaskProgressDetailForChange(changesDir, 'c2', projectRoot, cache); + expect(cache.size).toBe(1); + expect(d1).toEqual({ total: 1, completed: 1, unreadable: [] }); + expect(d2).toEqual({ total: 1, completed: 0, unreadable: [] }); + }); + it('identifies the tracked artifact by apply.tracks even when it is not named "tasks"', async () => { const schemaDir = path.join(projectRoot, 'openspec', 'schemas', 'custom-track'); await fs.mkdir(schemaDir, { recursive: true }); @@ -305,3 +325,56 @@ describe('countTasksFromContent', () => { expect(countTasksFromContent(content)).toEqual({ total: 6, completed: 3 }); }); }); + +/** + * #205 — `getTaskProgressDetailForChange` mirrors `getTaskProgressForChange` + * but also reports task files that exist yet cannot be read, so a lint can fail + * loudly instead of silently counting an unreadable file as "no tasks". + */ +describe('getTaskProgressDetailForChange (#205 unreadable reporting)', () => { + let root: string; + let changesDir: string; + + beforeEach(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-taskdetail-')); + changesDir = path.join(root, 'openspec', 'changes'); + await fs.mkdir(changesDir, { recursive: true }); + }); + + afterEach(async () => { + await fs.rm(root, { recursive: true, force: true }); + }); + + it('reports no unreadable files for a normal tasks.md and counts as before', async () => { + const changeDir = path.join(changesDir, 'ok'); + await fs.mkdir(changeDir, { recursive: true }); + await fs.writeFile(path.join(changeDir, 'tasks.md'), '- [x] a\n- [ ] b\n', 'utf-8'); + + const detail = await getTaskProgressDetailForChange(changesDir, 'ok', root); + expect(detail).toEqual({ total: 2, completed: 1, unreadable: [] }); + }); + + it('records a task file that exists but cannot be read (EISDIR)', async () => { + // A tasks.md that is a directory yields a non-ENOENT read error on every + // platform, standing in for a genuinely unreadable file. + const badTasks = path.join(changesDir, 'bad', 'tasks.md'); + await fs.mkdir(badTasks, { recursive: true }); + + const detail = await getTaskProgressDetailForChange(changesDir, 'bad', root); + expect(detail.total).toBe(0); + expect(detail.completed).toBe(0); + expect(detail.unreadable).toHaveLength(1); + // The reported path is the file that could not be read (both canonicalized + // so a /var vs /private/var symlink difference does not fail the identity). + expect(realpathSync.native(detail.unreadable[0])).toBe( + realpathSync.native(badTasks) + ); + }); + + it('treats a missing tasks.md as zero tasks, not unreadable', async () => { + await fs.mkdir(path.join(changesDir, 'empty'), { recursive: true }); + + const detail = await getTaskProgressDetailForChange(changesDir, 'empty', root); + expect(detail).toEqual({ total: 0, completed: 0, unreadable: [] }); + }); +});