Skip to content

fix: two-round code review fixes for parser, MCP framing, v0.2 serialization - #7

Merged
superops-team merged 1 commit into
mainfrom
fix/two-round-code-review-fixes
Aug 27, 2026
Merged

fix: two-round code review fixes for parser, MCP framing, v0.2 serialization#7
superops-team merged 1 commit into
mainfrom
fix/two-round-code-review-fixes

Conversation

@superops-team

Copy link
Copy Markdown
Owner

Summary

This PR applies fixes identified during a two-round code review of the recent PR #5 (v0.2 upgrade) and PR #6 (MCP server) merges.

Fixes

Parser (boundary bug):

  • findFrontmatterEnd had an off-by-one loop bound (i < len(data)-3i <= len(data)-3) and did not accept closing --- at EOF without trailing newline, causing valid markdown files without trailing newlines to fail frontmatter parsing.

v0.2 spec conformance:

  • ValidateConcept still enforced title as required, contradicting v0.2 spec §4.1 where title is optional. Fixed to derive title from filename when missing (matching parser behavior).

API error handling:

  • LoadBundle now explicitly skips reserved filenames (index.md, log.md) before calling ParseConcept, making the skip logic explicit rather than relying on parse errors.

Data loss in SaveKnowledgeBase (critical):

  • SaveKnowledgeBase only serialized v0.1 fields when writing concepts to disk, dropping all v0.2 fields (sources, generated, verified, status, stale_after, runtime, parameters, computation, executor, attester, filePath). Fixed to serialize the full v0.2 concept structure.

MCP stdio transport protocol (critical):

  • The MCP server used newline-delimited JSON over stdio, but the MCP specification requires LSP-style Content-Length: header framing. Standard MCP clients (Claude Desktop, etc.) could not communicate with the server. Fixed to implement proper Content-Length framing for both reading and writing, with newline-delimited fallback for reading.
  • Added stdout flush after writing responses to prevent buffering delays.

Incremental update / delete (v0.2 compatibility):

  • After fixing v0.2 serialization, removeKnowledgeFile failed to recognize auto-generated concepts because hasTrustedGeneratedMetadata only checked the legacy generated: true boolean in CustomFields, but v0.2 concepts store generated info in the Generated struct. Fixed to accept both formats.

Archive detection consistency:

  • IsArchive was case-sensitive for simple extensions (.zip, .tar) but case-insensitive for compound extensions (.tar.gz), while ExtractArchive internally used case-insensitive checks. Unified to fully case-insensitive detection.
  • Fixed .gz suffix check in extractTarGz to also be case-insensitive.

Tests:

  • Updated TestValidateConcept_MissingTitle to verify filename-derived title instead of expecting error.
  • Updated TestIsArchive/uppercase_ZIP to expect true for case-insensitive detection.
  • Updated test_mcp.py to use Content-Length framing matching the fixed MCP server.

Verification

  • go build ./... passes
  • go vet ./... passes
  • go test ./... all packages pass

…ization

- fix(parser): correct frontmatter boundary detection and support EOF closing marker
- fix(okf): make title optional per v0.2 spec, derive from filename when missing
- fix(api): skip reserved filenames before concept parsing to avoid misleading errors
- fix(git): serialize all v0.2 fields when saving knowledge base to prevent data loss
- fix(git): handle both v0.1 bool and v0.2 struct formats for generated metadata
- fix(mcp): implement standard Content-Length framing for JSON-RPC stdio transport
- fix(mcp): flush stdout after writing messages to prevent buffering delays
- fix(import): make archive extension checks consistently case-insensitive
- test: update test cases to match fixed behavior
@superops-team
superops-team force-pushed the fix/two-round-code-review-fixes branch from f97152f to 2b56bc9 Compare August 27, 2026 13:44
@superops-team
superops-team merged commit cf37765 into main Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant