feat(wiki): scalable wiki build with recursive topic indexes - #174
feat(wiki): scalable wiki build with recursive topic indexes#174zhuzhaoyun wants to merge 31 commits into
Conversation
- Stream large JSON via createReadStream state machine, recording only
top-level key + value type without retaining values
- Track string escapes and roles so nested strings no longer confuse
depth tracking
- Require explicit { approved: true, fields: [] } fieldPolicy whitelist
before extracting large JSON values
- Validate batch id and attemptToken against safe path segments before
writing prepared/<id>-<token>.json
- Keep fallback chunks on UTF-8/UTF-16 boundaries and track mixed
CRLF/LF/CR byte ranges while streaming JSONL
- Chunk external normalized Markdown by its normalized extension
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
- Treat a prepared or applied journal as a crash-replay authorized by
its payloadHash, skipping the activeBatchId/attemptToken guards so a
crash between state-write and journal-completion can still complete
instead of throwing BATCH_NOT_ACTIVE
- Add CHECKPOINT_CONFLICT guard for differing payloads on replay
- Make the mutation lock self-healing: store {pid, startedAt}, reclaim
when the holder pid is dead or past the 10m stale TTL, and surface a
live holder as LOCK_BUSY instead of leaking EEXIST
- Share acquireMutationLock between sync withMutationLock and the async
prepare/checkpoint path so both recover from a stuck lock
- Resolve staged pages via realpath so a staging symlink cannot
materialize arbitrary vault files
- Short-circuit recoverRunning when nothing is running
- Document the retry-batch id collision-resilience intent
Co-Authored-By: Claude <noreply@anthropic.com>
- Add indexes.mjs with buildIndexModel, writeIndexes, verifyCoverage, finalizeBuild, reindexTopicAndAncestors - Register finalize and reindex CLI commands in wiki-build.mjs - Add --summaries and --topic-id flags to parseArgs - Add test helpers: writeCompletedBuildState, completedThreeLevelBuild, completedLeafBuild, completedBuildWithMixedResults - 12 tests covering: three-level hierarchy, deterministic sharding, source page missing, reindex chain isolation, pending batch rejection, completed_with_errors phase, verifyCoverage, token-based sharding, estimateIndexTokens, TOPIC_SUMMARY_MISSING validation Co-Authored-By: Claude <noreply@anthropic.com>
- Remove unlinkSync before renameSync in writeIndexes (crash-safety gap) - Clean up stale index-shards/ directories when topic transitions from sharded to inline (writeIndexes + reindexTopicAndAncestors) - Remove dead code in verifyCoverage (model.missing check was redundant) - Add comment about conservative token estimation at sharding boundary - Add test for sharded-to-inline transition Co-Authored-By: Claude <noreply@anthropic.com>
Add E2E workflow test exercising all CLI commands from scan to finalize: - Full happy path with simulated crash mid-build (recover + re-claim) - Partial failure + skip + finalize with completed_with_errors - Failed file retry via retry batch + finalize with completed phase 3 new tests, 73/73 total wiki-build tests passing, no source changes needed. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const versions = readApprovedVersions(paths); | ||
| const latest = versions.length ? Math.max(...versions) : 0; | ||
| if (candidate.planVersion <= latest) { |
There was a problem hiding this comment.
approvePlan (lines 262-263) calls readApprovedVersions which reads the current plan file and history directory to determine the latest approved version. Between this read and the version check (lines 264-272), a concurrent process could approve the same version, causing the version-increment check to pass incorrectly. The acquireMutationLock from workspace.mjs is not invoked here, leaving a TOCTOU race window. Wrap the version-check-and-write block with acquireMutationLock / withMutationLock to serialize plan approvals.
Suggestion:
| const versions = readApprovedVersions(paths); | |
| const latest = versions.length ? Math.max(...versions) : 0; | |
| if (candidate.planVersion <= latest) { | |
| const result = withMutationLock(paths, () => { | |
| const versions = readApprovedVersions(paths); | |
| const latest = versions.length ? Math.max(...versions) : 0; | |
| if (candidate.planVersion <= latest) { |
| return; | ||
| } | ||
| if (!inventoryIds.has(fileId)) issue(errors, 'FILE_ID_UNKNOWN', { fileId, source }); | ||
| if (classifications.has(fileId)) issue(errors, 'FILE_CLASSIFICATION_DUPLICATE', { fileId }); |
There was a problem hiding this comment.
The FILE_CLASSIFICATION_DUPLICATE error (line 159) does not include the source parameter, making it hard to determine which parts of the plan (assignments, excluded, or undecided) caused the conflict. Include source and the existing classification's source in the error details for better debuggability.
Suggestion:
| if (classifications.has(fileId)) issue(errors, 'FILE_CLASSIFICATION_DUPLICATE', { fileId }); | |
| if (classifications.has(fileId)) issue(errors, 'FILE_CLASSIFICATION_DUPLICATE', { fileId, source, existingSource: classifications.get(fileId) }); |
| if (!Number.isInteger(policy.contextWindowTokens) || policy.contextWindowTokens < 1 | ||
| || policy.maxInputTokens !== Math.floor(policy.contextWindowTokens * policy.maxInputFraction)) { | ||
| issue(errors, 'BATCH_INPUT_TOKENS_INVALID'); | ||
| } |
There was a problem hiding this comment.
The batchPolicy.maxInputTokens validation (line 200) uses a strict equality check against Math.floor(policy.contextWindowTokens * policy.maxInputFraction). When maxInputTokens is undefined (not provided in the candidate), the inequality check undefined !== computedValue fails with BATCH_INPUT_TOKENS_INVALID, but the error message doesn't distinguish between a missing value and an incorrect one. Consider adding a separate check for policy.maxInputTokens being undefined/not an integer before the equality check, or providing a more specific error code.
Suggestion:
| if (!Number.isInteger(policy.contextWindowTokens) || policy.contextWindowTokens < 1 | |
| || policy.maxInputTokens !== Math.floor(policy.contextWindowTokens * policy.maxInputFraction)) { | |
| issue(errors, 'BATCH_INPUT_TOKENS_INVALID'); | |
| } | |
| if (!Number.isInteger(policy.contextWindowTokens) || policy.contextWindowTokens < 1) { | |
| issue(errors, 'BATCH_CONTEXT_WINDOW_INVALID'); | |
| } | |
| if (!Number.isInteger(policy.maxInputTokens) || policy.maxInputTokens < 1) { | |
| issue(errors, 'BATCH_MAX_INPUT_TOKENS_INVALID'); | |
| } else if (policy.maxInputTokens !== Math.floor(policy.contextWindowTokens * policy.maxInputFraction)) { | |
| issue(errors, 'BATCH_INPUT_TOKENS_MISMATCH'); | |
| } |
| // Crash replay: a prepared/applied journal authorizes the replay via its own | ||
| // payloadHash — the activeBatchId/attemptToken guards must be skipped because | ||
| // the state may already have been advanced past step 11 (activeBatchId cleared). | ||
| const resuming = existing?.phase === 'prepared' || existing?.phase === 'applied'; | ||
| if (resuming && existing.payloadHash !== payloadHash) { | ||
| throw codedError('CHECKPOINT_CONFLICT', `Batch ${result.batchId} already recorded a different payload`, { batchId: result.batchId }); | ||
| } |
There was a problem hiding this comment.
In checkpointBatch(), when resuming after a crash (resuming=true), the code skips validation of activeBatchId and attemptToken entirely. This means a stale journal from a previous attempt could be replayed even after the batch has been reclaimed with a new attemptToken. The journal's attemptToken should be compared against the current state's attemptToken when resuming to detect this stale replay scenario.
Suggestion:
| // Crash replay: a prepared/applied journal authorizes the replay via its own | |
| // payloadHash — the activeBatchId/attemptToken guards must be skipped because | |
| // the state may already have been advanced past step 11 (activeBatchId cleared). | |
| const resuming = existing?.phase === 'prepared' || existing?.phase === 'applied'; | |
| if (resuming && existing.payloadHash !== payloadHash) { | |
| throw codedError('CHECKPOINT_CONFLICT', `Batch ${result.batchId} already recorded a different payload`, { batchId: result.batchId }); | |
| } | |
| // Crash replay: a prepared/applied journal authorizes the replay via its own | |
| // payloadHash — the activeBatchId/attemptToken guards must be skipped because | |
| // the state may already have been advanced past step 11 (activeBatchId cleared). | |
| const resuming = existing?.phase === 'prepared' || existing?.phase === 'applied'; | |
| if (resuming && existing.payloadHash !== payloadHash) { | |
| throw codedError('CHECKPOINT_CONFLICT', `Batch ${result.batchId} already recorded a different payload`, { batchId: result.batchId }); | |
| } | |
| if (resuming && existing.attemptToken !== result.attemptToken) { | |
| throw codedError('STALE_ATTEMPT', `Journal attempt token ${existing.attemptToken} does not match payload ${result.attemptToken}`, { batchId: result.batchId }); | |
| } |
| for (const fileId of fileIdsForBatch) { | ||
| const fileResult = fileResults.get(fileId); | ||
| if (!fileResult) continue; | ||
| const fileState = next.files[fileId]; | ||
| if (fileResult.status === 'succeeded') { | ||
| fileState.status = 'succeeded'; | ||
| fileState.contentHash = fileResult.contentHash ?? fileState.contentHash; | ||
| anySucceeded = true; | ||
| } else if (fileResult.status === 'failed') { | ||
| fileState.status = 'failed'; | ||
| fileState.lastError = fileResult.error ?? null; | ||
| anyFailed = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
In checkpointBatch(), when a batch contains files not reported in the result payload (result.files), those files remain in 'running' status. The code only iterates over fileIdsForBatch and skips files not in fileResults. These orphaned files would stay as 'running' indefinitely, preventing the batch from being considered complete. Consider reverting unreported files to 'pending' or 'failed' with a descriptive error.
Suggestion:
| for (const fileId of fileIdsForBatch) { | |
| const fileResult = fileResults.get(fileId); | |
| if (!fileResult) continue; | |
| const fileState = next.files[fileId]; | |
| if (fileResult.status === 'succeeded') { | |
| fileState.status = 'succeeded'; | |
| fileState.contentHash = fileResult.contentHash ?? fileState.contentHash; | |
| anySucceeded = true; | |
| } else if (fileResult.status === 'failed') { | |
| fileState.status = 'failed'; | |
| fileState.lastError = fileResult.error ?? null; | |
| anyFailed = true; | |
| } | |
| } | |
| for (const fileId of fileIdsForBatch) { | |
| const fileResult = fileResults.get(fileId); | |
| if (!fileResult) { | |
| // File missing from result payload — mark as failed so it doesn't stay running forever. | |
| const fileState = next.files[fileId]; | |
| fileState.status = 'failed'; | |
| fileState.lastError = { code: 'MISSING_RESULT', message: `File ${fileId} not reported in checkpoint result` }; | |
| anyFailed = true; | |
| continue; | |
| } | |
| const fileState = next.files[fileId]; | |
| if (fileResult.status === 'succeeded') { | |
| fileState.status = 'succeeded'; | |
| fileState.contentHash = fileResult.contentHash ?? fileState.contentHash; | |
| anySucceeded = true; | |
| } else if (fileResult.status === 'failed') { | |
| fileState.status = 'failed'; | |
| fileState.lastError = fileResult.error ?? null; | |
| anyFailed = true; | |
| } | |
| } |
| export function makeVault() { | ||
| const path = mkdtempSync(join(tmpdir(), 'molio-wiki-build-')); | ||
| return { | ||
| path, | ||
| cleanup: () => rmSync(path, { recursive: true, force: true }), | ||
| }; | ||
| } |
There was a problem hiding this comment.
The makeVault function creates a temporary directory but requires manual cleanup via the returned cleanup() callback. If a test assertion fails or an exception is thrown between makeVault() and cleanup(), the temporary directory leaks. Over many test runs, this can accumulate and cause disk space issues.
Consider using node:test's built-in after hook or a disposable pattern (e.g., returning a [vault, cleanup] tuple and wrapping test bodies in try/finally).
Suggestion:
| export function makeVault() { | |
| const path = mkdtempSync(join(tmpdir(), 'molio-wiki-build-')); | |
| return { | |
| path, | |
| cleanup: () => rmSync(path, { recursive: true, force: true }), | |
| }; | |
| } | |
| export function makeVault() { | |
| const path = mkdtempSync(join(tmpdir(), 'molio-wiki-build-')); | |
| return { | |
| path, | |
| cleanup: () => rmSync(path, { recursive: true, force: true }), | |
| }; | |
| } | |
| /** Wraps a vault fixture so cleanup runs even if the callback throws. */ | |
| export function withVault<T>(fn: (vault: { path: string }) => T): T { | |
| const vault = makeVault(); | |
| try { | |
| return fn(vault); | |
| } finally { | |
| vault.cleanup(); | |
| } | |
| } |
| export function stagePage(vaultPath: string, stagingDir: string, relPath: string, content: string) { | ||
| const target = join(vaultPath, stagingDir, relPath); | ||
| mkdirSync(join(target, '..'), { recursive: true }); | ||
| writeFileSync(target, content, 'utf8'); | ||
| } |
There was a problem hiding this comment.
The stagePage function constructs a file path using join(vaultPath, stagingDir, relPath) without validating that the resolved path stays within the vault. While stagingDir comes from the CLI's controlled output in tests, a defense-in-depth check would prevent accidental writes outside the vault directory if the function is ever reused with untrusted inputs.
Consider resolving the final path and asserting it is within vaultPath, similar to how assertPathWithinVault works in the production code.
Suggestion:
| export function stagePage(vaultPath: string, stagingDir: string, relPath: string, content: string) { | |
| const target = join(vaultPath, stagingDir, relPath); | |
| mkdirSync(join(target, '..'), { recursive: true }); | |
| writeFileSync(target, content, 'utf8'); | |
| } | |
| import { resolve } from 'node:path'; | |
| export function stagePage(vaultPath: string, stagingDir: string, relPath: string, content: string) { | |
| const target = resolve(vaultPath, stagingDir, relPath); | |
| if (!target.startsWith(resolve(vaultPath) + '/') && target !== resolve(vaultPath)) { | |
| throw new Error(`Path escapes vault: ${target}`); | |
| } | |
| mkdirSync(dirname(target), { recursive: true }); | |
| writeFileSync(target, content, 'utf8'); | |
| } |
| }; | ||
| } | ||
|
|
||
| export function makePlanFixture(inventoryDigest: string): any { |
There was a problem hiding this comment.
Multiple functions return any type (e.g., makePlanFixture, makePlanFixtureForFiles, readState, runWikiBuildCli). While this is a test helper file, using any can hide type errors in test code that consumes these fixtures. The plan.mjs module already defines JSDoc typedefs (TopicNode, FileAssignment, Batch) that could be mirrored as TypeScript interfaces in a shared types file.
Suggestion:
| export function makePlanFixture(inventoryDigest: string): any { | |
| // Consider defining these interfaces in a shared types file: | |
| // interface PlanFixture { schemaVersion: number; planVersion: number; ... } | |
| export function makePlanFixture(inventoryDigest: string): any { |
| for (const record of records) { | ||
| const match = files.find((file) => file.filename === record.path); | ||
| if (match) record.id = match.id; | ||
| } |
There was a problem hiding this comment.
In the for...of loop, if a record's path doesn't match any file in the files array, match will be undefined and record.id will be set to undefined. While this is unlikely in normal test usage (the vault only contains the files that were just written), it could silently produce confusing failures if the vault somehow contains unexpected files (e.g., from a previous test run that wasn't cleaned up).
Consider adding a guard or at least logging a warning when a record has no matching file.
Suggestion:
| for (const record of records) { | |
| const match = files.find((file) => file.filename === record.path); | |
| if (match) record.id = match.id; | |
| } | |
| for (const record of records) { | |
| const match = files.find((file) => file.filename === record.path); | |
| if (match) { | |
| record.id = match.id; | |
| } else { | |
| throw new Error(`Unexpected inventory record with no matching file: ${record.path}`); | |
| } | |
| } |
| const pagesMap: Record<string, any> = {}; | ||
| for (const page of pages) { | ||
| pagesMap[page.path] = { | ||
| sha256: createHash('sha256').update(page.content).digest('hex'), | ||
| batchId: plan.batches[0]?.id ?? 'batch-001', |
There was a problem hiding this comment.
In writeCompletedBuildState, the pagesMap loop uses plan.batches[0]?.id ?? 'batch-001' as the batchId for every page, regardless of which batch actually produced that page. This means all pages are assigned to the first batch, which is incorrect for multi-batch plans. In completedLeafBuild, for example, there are multiple batches but all pages get batchId from batches[0].
This may cause issues in tests that verify batch-to-page relationships. Consider looking up the correct batch for each page based on the page's topicId and the batch assignments.
Suggestion:
| const pagesMap: Record<string, any> = {}; | |
| for (const page of pages) { | |
| pagesMap[page.path] = { | |
| sha256: createHash('sha256').update(page.content).digest('hex'), | |
| batchId: plan.batches[0]?.id ?? 'batch-001', | |
| const pagesMap: Record<string, any> = {}; | |
| for (const page of pages) { | |
| const matchingBatch = plan.batches.find((b: any) => b.topicId === page.topicId); | |
| pagesMap[page.path] = { | |
| sha256: createHash('sha256').update(page.content).digest('hex'), | |
| batchId: matchingBatch?.id ?? plan.batches[0]?.id ?? 'batch-001', |
- Extract filterAffectedIndexes from reindexTopicAndAncestors for reuse - Add buildIndexModelLenient (no summary requirement for incremental builds) - Add appendBuildLog (prepend to wiki/log.md after each checkpoint) - Add writeHotCache (rewrite wiki/hot.md with current build status) - Add rebuildAfterCheckpoint (incremental index rebuild per batch) - Slim finalizeBuild: coverage check now optional via --verify flag - Add parseFrontmatter and assembleAutoPayload to state.mjs - Integrate rebuildAfterCheckpoint into checkpointBatch (steps 9-10) - Demote prepareWorkItems to file-level by default, chunk only with --chunk - Add checkpoint --auto mode (frontmatter-driven, no hand-written JSON) - Add run --resume command (recover + claim in one step) - Add --auto, --failed-file, --resume, --chunk, --verify CLI flags - Update SKILL.md workflow to 2-command loop (next + checkpoint --auto) - Update all 4 test files with new coverage (90 tests, all green) Co-Authored-By: Claude <noreply@anthropic.com>
- renderEntry: derive wikilink target from page.path (actual flat location) instead of assembling topicPath/type/title (which doesn't match how pages are stored on disk, causing all INDEX links to break) - topicDisplayName: fall back to slug then id when topic.name is missing (plans only require slug/id, name is optional — rendering "undefined" when absent) - state.mjs log entry: same name→slug→id fallback for topic names Co-Authored-By: Claude <noreply@anthropic.com>
Affected Areas & Test SelectionNo area matched. Only P0 specs will run as baseline. Changed files: 29
Top changed symbols (by caller count):
E2E specs that will run on this PR:
Generated by Impact Analysis workflow. This is informational, not a gate. |
There was a problem hiding this comment.
🔍 OpenCodeReview found 36 issue(s) in this PR.
- ✅ 31 posted as inline comment(s)
- 📝 5 posted as summary
📄 apps/daemon/src/tools/skills/wiki-build/scripts/lib/workspace.mjs
The missing array (pages with unknown topicIds) is populated in buildIndexModel but never checked by finalizeBuild. When verifyCoverage is not enabled (the default path), pages with unknown topicIds are silently excluded from indexes, causing undetected data loss. The missing field from buildIndexModel should be checked and thrown as an error here, similar to how missingPages (pages missing from disk) is handled just above.
💡 Suggested Change
Before:
// Build index model
const model = buildIndexModel(plan, state.pages, summaries);
// Verify coverage (optional, only when --verify is passed)
After:
// Build index model
const model = buildIndexModel(plan, state.pages, summaries);
if (model.missing.length > 0) {
throw codedError('UNKNOWN_TOPIC_ID',
`${model.missing.length} page(s) reference unknown topicIds`,
{ codes: ['UNKNOWN_TOPIC_ID'], missingPages: model.missing });
}
// Verify coverage (optional, only when --verify is passed)
📄 apps/daemon/src/tools/skills/wiki-build/scripts/lib/workspace.mjs
The estimatedIndexTokens function uses Buffer.byteLength(markdown, 'utf8') / 3 to estimate tokens. For CJK characters, UTF-8 encoding is 3 bytes per character, but GPT tokenizers typically count each CJK character as 1-2 tokens (not 1 as bytes/3 would suggest). This means the token count is underestimated for CJK-heavy content, potentially causing shards to exceed maxLeafIndexTokens. Consider using a more conservative ratio (e.g., bytes/2) or a CJK-aware heuristic.
💡 Suggested Change
Before:
export function estimateIndexTokens(markdown) {
return Math.ceil(Buffer.byteLength(markdown, 'utf8') / 3);
}
After:
export function estimateIndexTokens(markdown) {
// CJK characters typically consume 2-3 tokens each in GPT tokenizers.
// Using bytes/2 provides a safer upper bound, especially for CJK-heavy content.
return Math.ceil(Buffer.byteLength(markdown, 'utf8') / 2);
}
📄 apps/daemon/src/tools/skills/wiki-build/scripts/lib/workspace.mjs
Inconsistent semicolons: buildIndexModelLenient omits semicolons on variable declarations (e.g., const capacity = ..., const topics = ..., const missing = []), while buildIndexModel and the rest of the file consistently use semicolons. This creates a style mismatch within the same module.
💡 Suggested Change
Before:
export function buildIndexModelLenient(plan, pages, summaries = {}) {
const capacity = plan.capacity ?? DEFAULT_CAPACITY
const topics = plan.topics ?? []
const { topicById, topicPathMap } = buildTopicMaps(topics)
// Group pages by topicId
const pagesByTopic = new Map()
const missing = []
After:
export function buildIndexModelLenient(plan, pages, summaries = {}) {
const capacity = plan.capacity ?? DEFAULT_CAPACITY;
const topics = plan.topics ?? [];
const { topicById, topicPathMap } = buildTopicMaps(topics);
// Group pages by topicId
const pagesByTopic = new Map();
const missing = [];
📄 apps/daemon/src/tools/skills/wiki-build/scripts/lib/workspace.mjs
The stale index-shards/ cleanup logic (lines 492-510) only iterates over model.indexes entries. If a leaf topic is completely removed from the plan, there is no corresponding INDEX.md in the model, and the old index-shards/ directory for that topic will never be cleaned up. This orphans disk space. reindexTopicAndAncestors has its own cleanup loop (lines 845-861) that handles topics in the chain, but finalizeBuild → writeIndexes has no equivalent cleanup for removed topics. Consider adding a scan of existing wiki/*/index-shards/ directories on disk and removing those not referenced in the current model.
💡 Suggested Change
Before:
// Clean up stale index-shards/ directories that are no longer in the model.
// This handles the sharded→inline transition: when a topic previously had
// shards but now produces an inline index, the old index-shards/ directory
// must be removed.
for (const [relPath] of Object.entries(model.indexes)) {
if (relPath.endsWith('/INDEX.md') && !relPath.includes('/index-shards/')) {
const topicDir = dirname(join(vault, relPath));
const shardDir = join(topicDir, 'index-shards');
if (existsSync(shardDir)) {
// Check whether the model includes any shards for this topic
const topicRelDir = dirname(relPath);
const hasModelShards = Object.keys(model.indexes).some(
(p) => p.startsWith(`${topicRelDir}/index-shards/`),
);
if (!hasModelShards) {
for (const file of readdirSync(shardDir)) {
try { unlinkSync(join(shardDir, file)); } catch { /* best effort */ }
}
try { rmdirSync(shardDir); } catch { /* best effort */ }
}
}
}
}
After:
// Clean up stale index-shards/ directories that are no longer in the model.
// This handles the sharded→inline transition: when a topic previously had
// shards but now produces an inline index, the old index-shards/ directory
// must be removed.
for (const [relPath] of Object.entries(model.indexes)) {
if (relPath.endsWith('/INDEX.md') && !relPath.includes('/index-shards/')) {
const topicDir = dirname(join(vault, relPath));
const shardDir = join(topicDir, 'index-shards');
if (existsSync(shardDir)) {
// Check whether the model includes any shards for this topic
const topicRelDir = dirname(relPath);
const hasModelShards = Object.keys(model.indexes).some(
(p) => p.startsWith(`${topicRelDir}/index-shards/`),
);
if (!hasModelShards) {
for (const file of readdirSync(shardDir)) {
try { unlinkSync(join(shardDir, file)); } catch { /* best effort */ }
}
try { rmdirSync(shardDir); } catch { /* best effort */ }
}
}
}
}
// Clean up orphaned index-shards/ directories for topics removed from the plan.
// When a topic is removed, its INDEX.md disappears from the model but its
// shard directory may remain on disk.
const wikiDir = join(vault, 'wiki');
if (existsSync(wikiDir)) {
const knownShardDirs = new Set(
Object.keys(model.indexes)
.filter((p) => p.includes('/index-shards/'))
.map((p) => dirname(join(vault, p))),
);
const walkAndClean = (dir) => {
for (const entry of readdirSync(dir, { withFileTypes: true })) {
const full = join(dir, entry.name);
if (entry.isDirectory() && entry.name === 'index-shards' && !knownShardDirs.has(full)) {
for (const file of readdirSync(full)) {
try { unlinkSync(join(full, file)); } catch { /* best effort */ }
}
try { rmdirSync(full); } catch { /* best effort */ }
} else if (entry.isDirectory()) {
walkAndClean(full);
}
}
};
walkAndClean(wikiDir);
}
📄 apps/daemon/src/tools/skills/wiki-build/scripts/lib/workspace.mjs
appendBuildLog uses writeFileSync directly without the atomic write-then-rename pattern used elsewhere in the codebase (e.g., writeIndexes, atomicWrite in workspace.mjs). If the process crashes mid-write, the log file will be corrupted. While log corruption is less critical than data corruption, using the atomic pattern would be more consistent and robust.
💡 Suggested Change
Before:
mkdirSync(dirname(logPath), { recursive: true })
if (existsSync(logPath)) {
const existing = readFileSync(logPath, 'utf8')
// Insert after the "# 构建日志\n" header line
const headerEnd = existing.indexOf('\n')
if (headerEnd >= 0 && existing.startsWith('# ')) {
writeFileSync(logPath, `${existing.slice(0, headerEnd + 1)}\n${block}${existing.slice(headerEnd + 1)}`, 'utf8')
} else {
writeFileSync(logPath, `${block}${existing}`, 'utf8')
}
} else {
writeFileSync(logPath, `# 构建日志\n\n${block}`, 'utf8')
}
After:
mkdirSync(dirname(logPath), { recursive: true })
let content = '';
if (existsSync(logPath)) {
const existing = readFileSync(logPath, 'utf8');
// Insert after the "# 构建日志\n" header line
const headerEnd = existing.indexOf('\n');
if (headerEnd >= 0 && existing.startsWith('# ')) {
content = `${existing.slice(0, headerEnd + 1)}\n${block}${existing.slice(headerEnd + 1)}`;
} else {
content = `${block}${existing}`;
}
} else {
content = `# 构建日志\n\n${block}`;
}
// Use atomic write pattern to avoid corruption on crash
const tmpPath = `${logPath}.tmp`;
const fd = openSync(tmpPath, 'w');
try {
writeFileSync(fd, content, 'utf8');
fsyncSync(fd);
} finally {
closeSync(fd);
}
renameSync(tmpPath, logPath);
| pathToKey.set(relPath, key); | ||
| pathToKey.set(relPath.replace(/\\/g, '/').toLowerCase(), key); |
There was a problem hiding this comment.
pathToKey collision risk: relPath from scanTree always uses / separators, so the replace(/\\/g, '/') is a no-op. But .toLowerCase() means two files differing only by case (e.g., Foo.md and foo.md) will overwrite each other in pathToKey. On case-insensitive filesystems (macOS/Windows) this is a non-issue since the filesystem prevents such files from coexisting, but on Linux it silently loses one file's mapping. Consider using the original-case path as the canonical key and only storing the lowercase variant as a secondary lookup.
Suggestion:
| pathToKey.set(relPath, key); | |
| pathToKey.set(relPath.replace(/\\/g, '/').toLowerCase(), key); | |
| pathToKey.set(relPath, key); | |
| // Case-insensitive lookup; on case-sensitive filesystems two files | |
| // differing only in case would collide, but that's a pathological edge case. | |
| pathToKey.set(relPath.replace(/\\/g, '/').toLowerCase(), key); |
| const exactCandidates = [`${normalizedTarget}.md`]; | ||
| if (sourcePath.startsWith('wiki/') && !normalizedTarget.startsWith('wiki/')) { | ||
| exactCandidates.push(`wiki/${normalizedTarget}.md`); | ||
| } |
There was a problem hiding this comment.
The exactCandidates construction in resolveLink strips leading / and ./ from the normalized target, but pathToKey stores relative paths like wiki/concepts/foo.md (never with leading /). This is consistent. However, the wiki/ prefix prepend logic on line 218-219 only applies when sourcePath.startsWith('wiki/'). If a link [[entities/SomeEntity]] is written from a non-wiki source file, the wiki/ prefix won't be prepended, and the link won't resolve even though the target exists at wiki/entities/SomeEntity.md. This is likely intentional for the wiki build context, but worth confirming the expected behavior for non-wiki source files linking to wiki content.
| export const FILE_STATUSES = Object.freeze([ | ||
| 'pending', 'running', 'succeeded', 'failed', 'skipped', | ||
| ]); | ||
| export const BATCH_STATUSES = FILE_STATUSES; | ||
| export const BUILD_PHASES = Object.freeze([ | ||
| 'not_started', 'scanned', 'draft', 'approved', 'running', | ||
| 'paused', 'completed', 'completed_with_errors', | ||
| ]); |
There was a problem hiding this comment.
FILE_STATUSES, BATCH_STATUSES, and BUILD_PHASES are exported but never imported by any other module. These are dead code. If they are intended for future use, consider adding them when the consuming code is ready. If they represent the canonical status/phase values used by the build system, they should be consumed by the modules that use string literals like 'pending', 'running', 'succeeded', 'failed', 'skipped' throughout the codebase (e.g., state.mjs, indexes.mjs).
Suggestion:
| export const FILE_STATUSES = Object.freeze([ | |
| 'pending', 'running', 'succeeded', 'failed', 'skipped', | |
| ]); | |
| export const BATCH_STATUSES = FILE_STATUSES; | |
| export const BUILD_PHASES = Object.freeze([ | |
| 'not_started', 'scanned', 'draft', 'approved', 'running', | |
| 'paused', 'completed', 'completed_with_errors', | |
| ]); | |
| export const FILE_STATUSES = Object.freeze([ | |
| 'pending', 'running', 'succeeded', 'failed', 'skipped', | |
| ]); | |
| export const BATCH_STATUSES = FILE_STATUSES; | |
| export const BUILD_PHASES = Object.freeze([ | |
| 'not_started', 'scanned', 'draft', 'approved', 'running', | |
| 'paused', 'completed', 'completed_with_errors', | |
| ]); |
| export const FILE_STATUSES = Object.freeze([ | ||
| 'pending', 'running', 'succeeded', 'failed', 'skipped', | ||
| ]); | ||
| export const BATCH_STATUSES = FILE_STATUSES; |
There was a problem hiding this comment.
BATCH_STATUSES = FILE_STATUSES creates an alias, not a copy. If batch and file statuses diverge in the future, modifying one would silently affect the other. Consider using a separate array or a spread copy: export const BATCH_STATUSES = Object.freeze([...FILE_STATUSES]).
Suggestion:
| export const BATCH_STATUSES = FILE_STATUSES; | |
| export const BATCH_STATUSES = Object.freeze([...FILE_STATUSES]); |
| if (existsSync(targetPath)) { | ||
| try { unlinkSync(targetPath); } catch { /* rename will overwrite */ } | ||
| } | ||
| renameSync(temporary, targetPath); |
There was a problem hiding this comment.
atomicReplaceFile uses renameSync after attempting unlinkSync. On Windows, if unlinkSync fails (e.g., another process holds the file, permission denied), the catch block silently swallows the error, and renameSync will then fail with a different error — but the original unlinkSync failure is lost. The function should throw a descriptive error instead of catching and ignoring the unlink failure. Additionally, the catch block catches ALL errors (including unexpected ones like ENOENT), not just EBUSY/EPERM. Consider using renameSync with a try/catch, and if it fails with EPERM/EACCES (Windows-specific), retry with copyFileSync + unlinkSync as a fallback.
Suggestion:
| if (existsSync(targetPath)) { | |
| try { unlinkSync(targetPath); } catch { /* rename will overwrite */ } | |
| } | |
| renameSync(temporary, targetPath); | |
| if (existsSync(targetPath)) { | |
| try { | |
| unlinkSync(targetPath); | |
| } catch (err) { | |
| if (err.code !== 'ENOENT') { | |
| throw codedError('ATOMIC_REPLACE_FAILED', | |
| `Failed to remove target ${targetPath}: ${err.message}`, { code: err.code }); | |
| } | |
| } | |
| } | |
| try { | |
| renameSync(temporary, targetPath); | |
| } catch (err) { | |
| // On Windows, renameSync may fail if the target exists despite unlink. | |
| // Fall back to copy + unlink. | |
| if (err.code === 'EPERM' || err.code === 'EACCES') { | |
| const { copyFileSync } = require('node:fs'); | |
| copyFileSync(temporary, targetPath); | |
| unlinkSync(temporary); | |
| } else { | |
| throw err; | |
| } | |
| } |
| const colonIndex = line.indexOf(':') | ||
| if (colonIndex < 1) continue | ||
| const key = line.slice(0, colonIndex).trim() | ||
| let value = line.slice(colonIndex + 1).trim() |
There was a problem hiding this comment.
parseFrontmatter uses line.indexOf(':') to split key-value pairs. This fails on values containing colons — e.g., summary: Foo: bar is parsed as summary: "Foo" instead of summary: "Foo: bar". Wiki page frontmatter generated by the tool can contain colons in summaries and titles. This can cause assembleAutoPayload to reject valid staged pages as missing required frontmatter fields, or silently produce incorrect metadata. Fix by splitting only on the first colon.
Suggestion:
| const colonIndex = line.indexOf(':') | |
| if (colonIndex < 1) continue | |
| const key = line.slice(0, colonIndex).trim() | |
| let value = line.slice(colonIndex + 1).trim() | |
| // Split only on the first colon to allow colons in values | |
| const colonIndex = line.indexOf(':') | |
| if (colonIndex < 1) continue | |
| const key = line.slice(0, colonIndex).trim() | |
| let value = line.slice(colonIndex + 1).trim() |
| if (existsSync(paths.plan)) { | ||
| readJson(paths.plan); | ||
| return { phase: 'approved' }; | ||
| } | ||
| if (existsSync(paths.planDraft)) { | ||
| readJson(paths.planDraft); | ||
| return { phase: 'draft' }; | ||
| } |
There was a problem hiding this comment.
The status function reads plan and planDraft files using readJson without try-catch. If these files are corrupted or contain malformed JSON, JSON.parse will throw an unhandled exception, crashing the process instead of returning a graceful error status (e.g., { phase: 'error', message: 'Corrupted file' }).
Suggestion:
| if (existsSync(paths.plan)) { | |
| readJson(paths.plan); | |
| return { phase: 'approved' }; | |
| } | |
| if (existsSync(paths.planDraft)) { | |
| readJson(paths.planDraft); | |
| return { phase: 'draft' }; | |
| } | |
| if (existsSync(paths.plan)) { | |
| try { | |
| readJson(paths.plan); | |
| return { phase: 'approved' }; | |
| } catch { | |
| return { phase: 'error', message: 'Corrupted plan file' }; | |
| } | |
| } | |
| if (existsSync(paths.planDraft)) { | |
| try { | |
| readJson(paths.planDraft); | |
| return { phase: 'draft' }; | |
| } catch { | |
| return { phase: 'error', message: 'Corrupted plan draft file' }; | |
| } | |
| } |
| const payload = withMutationLock(paths, () => assembleAutoPayload(paths, batchId, attemptToken, failedFiles)); | ||
| withAsyncMutationLock(paths, () => checkpointBatch(paths, payload)) |
There was a problem hiding this comment.
The checkpoint --auto path calls assembleAutoPayload inside a synchronous withMutationLock and checkpointBatch inside a separate withAsyncMutationLock. Between the release of the sync lock and acquisition of the async lock, another process could modify the state (e.g., claim a different batch, modify file statuses). While checkpointBatch re-validates activeBatchId and attemptToken, the assembleAutoPayload results (which read state/plan) could become stale. Consider using a single lock scope that covers both the payload assembly and the checkpoint, or accept that checkpointBatch's own re-validation is the authoritative check.
Suggestion:
| const payload = withMutationLock(paths, () => assembleAutoPayload(paths, batchId, attemptToken, failedFiles)); | |
| withAsyncMutationLock(paths, () => checkpointBatch(paths, payload)) | |
| withAsyncMutationLock(paths, async () => { | |
| const payload = assembleAutoPayload(paths, batchId, attemptToken, failedFiles); | |
| return checkpointBatch(paths, payload); | |
| }) |
| const plan = readJson(paths.plan); | ||
| const state = readJson(paths.state); |
There was a problem hiding this comment.
The reindex command reads paths.plan and paths.state with readJson but does not check if the files exist first. If the build has not completed the plan phase, readFileSync will throw an ENOENT error, crashing the process with an unhelpful error message instead of returning a friendly CLI error.
Suggestion:
| const plan = readJson(paths.plan); | |
| const state = readJson(paths.state); | |
| if (!existsSync(paths.plan)) cliError(command, 'PLAN_NOT_FOUND', 'Plan file not found. Run "plan approve" first.'); | |
| if (!existsSync(paths.state)) cliError(command, 'STATE_NOT_FOUND', 'State file not found. Run "next" and batch processing first.'); | |
| const plan = readJson(paths.plan); | |
| const state = readJson(paths.state); |
| const summariesPath = assertPathWithinVault(vault, resolve(vault, options.summaries)); | ||
| const summaries = readJson(summariesPath); |
There was a problem hiding this comment.
The finalize command calls assertPathWithinVault which only ensures the path is within the vault but does not guarantee the file exists (it resolves to the nearest existing parent directory). The subsequent readJson call will throw ENOENT if the summaries file doesn't exist, producing an unhelpful error. Add an explicit existsSync check before readJson.
Suggestion:
| const summariesPath = assertPathWithinVault(vault, resolve(vault, options.summaries)); | |
| const summaries = readJson(summariesPath); | |
| const summariesPath = assertPathWithinVault(vault, resolve(vault, options.summaries)); | |
| if (!existsSync(summariesPath)) cliError(command, 'FILE_NOT_FOUND', `Summaries file not found: ${summariesPath}`); | |
| const summaries = readJson(summariesPath); |
1 participant
Summary
Implements a deterministic, resumable wiki build system that can scale to large vaults (50,000+ files) with recursive topic hierarchies and crash recovery.
Key Features
1. CLI Toolchain
scan→ vault inventory with path safety and symlink protectionplan→ semantic topic hierarchy with version control and user approval gatenext→ batch claiming with attempt tokens and source change detectionprepare→ work item preparation with JSON streaming and Unicode boundariescheckpoint→ idempotent checkpoints with crash recovery journalfinalize→ recursive index generation with deterministic shardingreindex→ incremental index updates for specific topic chains2. Resumability & Crash Recovery
CHECKPOINT_CONFLICTpreparedandappliedjournal phases3. Recursive Topic Indexes
<type>-NNNN.mdwhen leaf exceeds 200 pages or 12000 tokens[[topic/subtopic/page|display]]4. User Approval Workflow
wiki/writes until approved)Files Changed
New Files
apps/daemon/src/tools/skills/wiki-build/scripts/lib/{contracts,workspace,inventory,plan,preprocess,state,indexes}.mjs— core modulesapps/daemon/src/tools/skills/wiki-build/scripts/wiki-build.mjs— CLI entry pointapps/daemon/test/tools/wiki-build-{cli,scan,plan,preprocess,state,indexes,workflow}.test.ts— 73 test casesapps/desktop/test/wiki-build-resources.test.js— packaging verificationdocs/wiki-build-acceptance.md— acceptance test runbookModified Files
apps/daemon/src/tools/skills/wiki-build/SKILL.md— v2.0.0 with CLI workflowapps/daemon/src/tools/skills/wiki-ingest/SKILL.md— v2.0.0 for recursive wikiapps/daemon/src/core/wiki-prompts.ts— recursive index navigation promptsapps/daemon/src/routes/graph.ts— path-qualified wikilink resolutionapps/daemon/test/tools/builtin-skills.test.ts— v2.0.0 assertionsapps/daemon/test/core/skill-installer.test.ts— version migration testsTest Coverage
Test Highlights
preparedandappliedjournal replaystatus --recoverCHECKPOINT_CONFLICTCommits (11 tasks)
feat(wiki): add deterministic build cli— CLI + workspacefeat(wiki): add bounded vault inventory— scan + inventoryfeat(wiki): freeze validated topic plans— plan validation + versioningfeat(wiki): prepare bounded build inputs— preprocessingfix(wiki): stream large json and harden prepared paths— task 4 review fixesfeat(wiki): add resumable batch checkpoints— state machine + checkpointsfix(wiki): complete applied-journal replay and self-heal lock— task 5 review fixesfeat(wiki): generate recursive topic indexes— indexes + shardingfix(wiki): fix atomic write and clean stale shards— task 6 review fixestest(wiki): cover resumable build workflow— E2E workflow testfeat(wiki): make build and ingest resumable— SKILL.md v2.0.0feat(wiki): navigate recursive topic indexes— prompts + graphtest(wiki): verify build tool packaging— desktop packaging testdocs(wiki): add acceptance test runbook— acceptance docsBreaking Changes
None. The legacy flat wiki (
wiki/INDEX.mdwith type directories) remains readable and queryable. The new recursive wiki coexists with legacy wikis.Next Steps
articles-1vault (seedocs/wiki-build-acceptance.md)hot.mdandlog.mdto recursive wiki structure (currently only in prompts)