feat: render plugin hook and SpaceGraph

[mindsocket]

Summary

• Adds a render command that dispatches to plugin-provided render formats (e.g. markdown.bullets, markdown.mermaid)
• Introduces SpaceGraph — a navigable, indexed graph built from SpaceNode[] — used by render and filter consumers
• Standardises plugin hook convention: hook(context: PluginContext, ...data, options) — render hooks now receive PluginContext for consistency with parse/templateSync
• Replaces RenderInput with RenderOptions; plugin-api exports updated accordingly
• Adds lefthook for pre-commit lint/format enforcement
• Flattens src/graph/space-graph.ts → src/space-graph.ts; removes unused categories from SpaceGraph

Base branch: feat/filtering — retarget to main after that PR merges.

Test plan

• bun test tests/render/ — render unit tests pass
• bun test tests/space-graph.test.ts — SpaceGraph tests pass
• ost-tools render <space> markdown.bullets outputs an indented bullet tree
• ost-tools render <space> markdown.mermaid outputs a Mermaid graph TD diagram
• ost-tools render <space> markdown.bullets --filter "opportunity" respects filter

[claude]

Claude finished @mindsocket's task in 5m 16s —— View job

---

Code Review

Tasks:

• Review SpaceGraph implementation
• Review render command and registry
• Review render plugins (bullets, mermaid)
• Review plugin API changes
• Check test coverage
• Verify filter integration
• Post final review

---

Summary

This is a well-structured PR that introduces a clean, extensible render plugin system with the SpaceGraph abstraction. The implementation demonstrates good software engineering practices with comprehensive test coverage (100 tests passing). The architecture successfully unifies the previously scattered diagram and show commands under a single plugin-based render command.

Strengths

1. Excellent Architecture: The SpaceGraph abstraction (src/space-graph.ts) provides a clean, navigable graph structure that consumers can use without building their own indexes. The separation between hierarchyChildren (hierarchy edges only) and children (all edges) is thoughtful.

2. Comprehensive Tests: 67 tests for filter, 33 for space-graph/render — all passing. Tests cover edge cases like orphans, dangling parents, non-hierarchy nodes, and repeated subtrees.

3. Clean Plugin API: The standardized hook convention hook(context: PluginContext, ...data, options) is now consistent across parse, templateSync, and render hooks.

4. Good Type Safety: Strong TypeScript usage throughout with proper exports from plugin-api.ts.

Issues & Suggestions

1. Misleading "DEPRECATED" Comment

File: src/integrations/miro/layout.ts:16-19

The comment marks buildDepthMap as DEPRECATED with a note about "migration to render plugin and SpaceGraph," but this function is actively used by layoutNewCards, which is called from miro/sync.ts.

/**
 * DEPRECATED - likely not needed after migration to render plugin and SpaceGraph
 * Build a depth map from hierarchy levels.
 */
function buildDepthMap(hierarchyLevels: HierarchyLevel[]): Map<string, number> {

Suggestion: Either remove this function if truly deprecated, or update the comment to clarify its current status. Fix this →

2. Unsafe Type Assertion in Mermaid Renderer

File: src/plugins/markdown/render-mermaid.ts:39

const status = node.schemaData.status as string;

This assumes every node has a status field, which may not be true for all schemas. This would throw at runtime if status is undefined.

Suggestion: Provide a fallback or use optional chaining:

const status = (node.schemaData.status as string | undefined) ?? 'active';

Fix this →

3. Hardcoded Type Styles in Mermaid Output

File: src/plugins/markdown/render-mermaid.ts:17-29

The Mermaid renderer outputs hardcoded styles for specific types (vision, mission, goal, opportunity, solution). This won't work well for schemas with different type hierarchies.

mmd += '  classDef vision fill:#ff9999,stroke:#ff0000,stroke-width:2px\n';
mmd += '  classDef mission fill:#99ccff,stroke:#0066cc,stroke-width:2px\n';
// ...

Suggestion: Consider making this configurable or deriving styles dynamically from the hierarchy levels. For now, this may be acceptable as a Markdown plugin-specific implementation detail.

4. Blocking File Write in Render Command

File: src/commands/render.ts:14

writeFileSync(options.output, result);

The command is async but uses the synchronous writeFileSync. This could block the event loop for large outputs.

Suggestion: Use promises.writeFile for consistency:

await fs.promises.writeFile(options.output, result);

Fix this →

5. Silent Error Continuation in Filter

File: src/filter/filter-nodes.ts:66-70

try {
  const result = await expr.evaluate(input);
  if (result) matched.push(node);
} catch (error) {
  console.warn(`Warning: Error evaluating filter expression for node "${node.title}":`, error);
}

When filter evaluation fails for a node, it's silently skipped. This could lead to unexpected results where some nodes match the filter but fail evaluation and are excluded.

Suggestion: Consider if this should be an error, or at least document this behavior clearly.

Minor Observations

1. Unused Parameter: In src/plugins/markdown/index.ts:46, the _context parameter is prefixed to indicate it's intentionally unused. This is good practice.

2. Lefthook Configuration: The addition of lefthook.yml for pre-commit linting is a good developer experience improvement. The configuration looks appropriate.

3. Filter Integration: The changes to filter-nodes.ts to accept and return SpaceGraph instead of SpaceNode[] are clean and maintain backward compatibility for the filtering logic itself.

Test Plan Verification

• ✅ bun test tests/render/ — render unit tests pass (33 tests)
• ✅ bun test tests/space-graph.test.ts — SpaceGraph tests pass
• ℹ️ Manual testing for ost-tools render <space> markdown.bullets and ost-tools render <space> markdown.mermaid — not verified in this review (requires actual space data)
• ℹ️ Filter integration testing — not verified manually, but unit tests pass

Conclusion

This PR is ready to merge with minor optional improvements. The core architecture is sound, tests are comprehensive, and the implementation follows the project's patterns well. The issues identified above are relatively minor and don't block the merge — they can be addressed in follow-up commits if desired.

[claude]

Claude finished @mindsocket's task in 1m 21s —— View job

---

Updated Code Review

Tasks:

• Review changes since previous review
• Re-examine previously identified issues
• Check for new issues
• Post final review

---

Summary

This updated review examines the changes since commit 27cef32 (the previous review baseline) through the latest commit bd6f472. The most recent commit refactored the SpaceGraph architecture and standardized the render hook convention. Several issues from the previous review have been addressed, though some remain.

✅ Fixed Issues

1. Silent Error Continuation in Filter - FIXED

File: src/filter/filter-nodes.ts

The silent error continuation has been removed. The previous version wrapped expression evaluation in a try-catch with console.warn, which could lead to unexpected results. The new code (lines 65-66) evaluates expressions directly, making filter evaluation errors hard failures as intended.

// Old: try-catch with console.warn
// New: Direct evaluation
const result = await expr.evaluate(input);
if (result) matched.push(node);

This aligns with the commit message from the parent branch: "fix: make filter evaluation errors hard failures".

⚠️ Remaining Issues

1. Misleading "DEPRECATED" Comment (PERSISTS)

File: src/integrations/miro/layout.ts:16-19

The comment still marks buildDepthMap as DEPRECATED with a note about "migration to render plugin and SpaceGraph," but this function is actively used by layoutNewCards (line 43).

/**
 * DEPRECATED - likely not needed after migration to render plugin and SpaceGraph
 * Build a depth map from hierarchy levels.
 */
function buildDepthMap(hierarchyLevels: HierarchyLevel[]): Map<string, number> {

Recommendation: Either remove this function if truly deprecated (refactor layoutNewCards to use SpaceGraph directly), or update the comment to clarify its current status.

2. Unsafe Type Assertion in Mermaid Renderer (PERSISTS)

File: src/plugins/markdown/render-mermaid.ts:39

const status = node.schemaData.status as string;

This assumes every node has a status field, which may not be true for all schemas. This will throw at runtime if status is undefined.

Recommendation: Provide a fallback:

const status = (node.schemaData.status as string | undefined) ?? 'active';

3. Blocking File Write in Render Command (PERSISTS)

File: src/commands/render.ts:14

writeFileSync(options.output, result);

The command is async but uses the synchronous writeFileSync. This could block the event loop for large outputs.

Recommendation: Use promises.writeFile for consistency:

await fs.promises.writeFile(options.output, result);

4. Hardcoded Type Styles in Mermaid Output (ACCEPTABLE FOR NOW)

File: src/plugins/markdown/render-mermaid.ts:17-29

The Mermaid renderer outputs hardcoded styles for specific types. This is plugin-specific implementation and acceptable for now, but may need to be made configurable for schemas with different type hierarchies.

New Observations

1. Clean Architecture: The refactoring to use SpaceGraph directly in filterNodes simplifies the code significantly. The use of graph.nodes and graph.children instead of building indexes manually is cleaner.

2. Consistent API: The render hook signature now follows the same convention as parse/templateSync: render(context: PluginContext, graph: SpaceGraph, options: RenderOptions).

3. Good Test Coverage: 100 tests passing (67 filter, 33 space-graph/render).

Test Plan Verification

• ✅ bun test tests/render/ — render unit tests pass (33 tests)
• ✅ bun test tests/space-graph.test.ts — SpaceGraph tests pass
• ℹ️ Manual testing for CLI commands — not verified (requires actual space data)

Conclusion

The PR has improved since the previous review with the filter error handling fix. The architecture is solid and tests are comprehensive. The remaining issues are minor:

• The DEPRECATED comment should be resolved (either remove the function or update the comment)
• The type assertion and file write issues are minor but worth addressing

This PR is ready to merge with optional follow-up improvements for the remaining issues.

[claude]

Claude finished @mindsocket's task in 1m 8s —— View job

---

Updated Code Review

Tasks:

• Check git history for new commits since last review
• Review new changes in the PR
• Re-examine previously identified issues
• Check for any new issues introduced
• Post final review

---

Summary

This review examines the current state of the PR at commit 89305ed ("refactor: clean up SpaceGraph architecture and standardise render hook convention"). This is the same commit that was reviewed in the previous review cycle — no new commits have been added since then. The core architecture remains solid with all 343 tests passing.

✅ Overall Assessment

This is a well-architected PR that introduces a clean, extensible render plugin system with the SpaceGraph abstraction. The implementation demonstrates excellent software engineering practices with comprehensive test coverage. The standardization of the plugin hook convention across parse, templateSync, and render is a positive improvement.

⚠️ Outstanding Issues from Previous Review

The following issues identified in previous reviews remain unaddressed:

1. Misleading "DEPRECATED" Comment

File: src/integrations/miro/layout.ts:16-19

The comment marks buildDepthMap as DEPRECATED with a note about "migration to render plugin and SpaceGraph," but this function is actively used by layoutNewCards at line 43.

/**
 * DEPRECATED - likely not needed after migration to render plugin and SpaceGraph
 * Build a depth map from hierarchy levels.
 */
function buildDepthMap(hierarchyLevels: HierarchyLevel[]): Map<string, number> {

Recommendation: Either remove this function if truly deprecated (refactor layoutNewCards to use SpaceGraph directly), or update the comment to clarify its current status. Fix this →

2. Unsafe Type Assertion in Mermaid Renderer

File: src/plugins/markdown/render-mermaid.ts:39

const status = node.schemaData.status as string;

This assumes every node has a status field, which may not be true for all schemas. This will throw a runtime error if status is undefined.

Recommendation: Provide a fallback:

const status = (node.schemaData.status as string | undefined) ?? 'active';

Fix this →

3. Blocking File Write in Render Command

File: src/commands/render.ts:14

writeFileSync(options.output, result);

The command is async but uses the synchronous writeFileSync. This could block the event loop for large outputs.

Recommendation: Use promises.writeFile for consistency:

await fs.promises.writeFile(options.output, result);

Fix this →

4. Hardcoded Type Styles in Mermaid Output (ACCEPTABLE FOR NOW)

File: src/plugins/markdown/render-mermaid.ts:17-29

The Mermaid renderer outputs hardcoded styles for specific types (vision, mission, goal, opportunity, solution). This is plugin-specific implementation and acceptable for now, but may need to be made configurable for schemas with different type hierarchies in the future.

Positive Observations

1. Clean Architecture: The SpaceGraph abstraction (src/space-graph.ts) provides a well-designed, navigable graph structure. The separation between hierarchyChildren (hierarchy edges only) and children (all edges) is thoughtful.

2. Consistent Plugin API: The render hook signature now follows the same convention as parse/templateSync: render(context: PluginContext, graph: SpaceGraph, options: RenderOptions), providing API consistency across the codebase.

3. Comprehensive Test Coverage: All 343 tests passing (including 33 render/space-graph tests). The tests cover edge cases like orphans, dangling parents, non-hierarchy nodes, and repeated subtrees.

4. Filter Integration: The changes to filterNodes to accept and return SpaceGraph are clean, and the direct evaluation (lines 65-66) makes filter evaluation errors hard failures as intended.

5. Developer Experience: The addition of lefthook.yml for pre-commit linting is a good improvement for developer workflow.

Test Plan Verification

• ✅ bun test tests/render/ — render unit tests pass (33 tests)
• ✅ bun test tests/space-graph.test.ts — SpaceGraph tests pass
• ✅ bun test — full test suite passes (343 tests)
• ℹ️ Manual testing for CLI commands — not verified (requires actual space data)

Conclusion

The PR is in a stable state with excellent architecture and comprehensive test coverage. The three remaining issues (DEPRECATED comment, unsafe type assertion, blocking file write) are relatively minor and don't block the merge — they can be addressed in follow-up commits if desired. The core functionality is sound and ready for production use.

[mindsocket]

Merged manually