fix(core): read legacy key=value metadata in .json session files#1738
fix(core): read legacy key=value metadata in .json session files#1738dashitongzhi wants to merge 2 commits into
Conversation
parseMetadataContent() previously only attempted JSON.parse() and returned
null on any failure. When a .json file contains legacy key=value metadata
(as seen in pre-V2 canonical orchestrator sessions), readMetadata() and
readMetadataRaw() treated it as missing, causing ensureOrchestrator() to
fail with a self-contradictory 'session already exists' error.
Fix: fall back to parseKeyValueContent() when JSON.parse fails and the
content does not start with '{' or '['. Content that looks like JSON but
fails to parse is still treated as corrupt.
Closes ComposioHQ#1720
There was a problem hiding this comment.
Pull request overview
This PR fixes a recovery bug where ao start can’t reuse an existing canonical orchestrator if its session metadata file has a .json name but contains legacy key=value content. It updates core metadata parsing to be JSON-first while supporting the legacy format as a fallback, and adds unit tests covering the new behavior.
Changes:
- Extend
parseMetadataContent()to fall back to legacykey=valueparsing when JSON parsing fails and the content doesn’t look like intended JSON ({or[). - Preserve existing “corrupt JSON” behavior by continuing to return
nullfor invalid content that starts like JSON. - Add metadata unit tests covering
readMetadata/readMetadataRawfor legacy content in.jsonfiles.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
packages/core/src/metadata.ts |
Adds legacy key=value fallback parsing to session metadata reads while preserving corrupt-JSON handling. |
packages/core/src/__tests__/metadata.test.ts |
Adds unit tests proving legacy key=value content inside .json files is readable and corrupt/empty cases still return null. |
| /** Parse JSON metadata file content. Returns null on invalid JSON. */ | ||
| /** Parse metadata file content. Returns null on invalid content. | ||
| * Supports JSON format (current) and legacy key=value format. | ||
| * If content starts with '{' but fails JSON parse, it's corrupt — return null. |
There was a problem hiding this comment.
Fixed — updated the doc comment to mention both { and [ in the corrupt-JSON check.
| expect(meta!.runtimeHandle?.runtimeName).toBe("tmux"); | ||
| expect(meta!.status).toBe("working"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Acknowledged — the session-manager regression test for ensureOrchestrator recovery would require deeper mocking of the spawn/reservation path. The existing unit tests now cover the core parsing and round-trip behavior. A higher-level integration test can be added in a follow-up.
Greptile SummaryFixes Confidence Score: 5/5Safe to merge — the change is minimal, well-tested, and the fallback path is only reached for files that already failed JSON parsing. The modification is narrowly scoped to a single private helper function. All callers pre-trim content before passing it in, and the round-trip through flattenToStringRecord and unflattenFromStringRecord correctly handles JSON-stringified nested fields. No files require special attention.
|
| Filename | Overview |
|---|---|
| packages/core/src/metadata.ts | Adds legacy line-oriented fallback in parseMetadataContent(); logic is correct and the round-trip handles JSON-stringified field values correctly. |
| packages/core/src/tests/metadata.test.ts | Six new tests covering readMetadata, readMetadataRaw, blank/comment-only content, mutateMetadata round-trip, and the corrupt-JSON guard. |
Reviews (2): Last reviewed commit: "fix: address bot review feedback on lega..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Hi! I want to acknowledge the 4 inline issues flagged by Copilot and Greptile reviewers before requesting human review. I'll address these feedback items and push an update shortly. Friendly ping for review once the fixes are in 🙏 CI is passing on the current version. Thank you! |
- Update doc comment to mention '[' alongside '{' in corrupt-JSON check
- Add test for comment-only/blank key=value file exercising fallback path
- Add mutateMetadata round-trip test for legacy format content
|
Hi! 👋 This PR looks ready for review. Could a maintainer please take a look when they have a chance? Thank you! |
Problem
ao startfails when a valid canonical orchestrator session is alive but its.jsonmetadata file contains legacykey=valueformat instead of JSON.parseMetadataContent()inmetadata.tsonly attemptsJSON.parse()and returnsnullon non-JSON content, whilereserveSessionId()sees the file as existing — creating a self-contradictory state that blocks recovery.Fixes #1720
Fix
Modified
parseMetadataContent()to fall back toparseKeyValueContent()(an existing helper used elsewhere in the codebase for legacy formats) when:JSON.parse()fails, AND{or[(preserving corrupt-JSON behavior)This is the smallest change that resolves the issue — only
packages/core/src/metadata.tsis modified (plus tests).Key design choices
{or[that fails to parse is still treated as corrupt (not silently parsed as key=value)null(matches existingreserveSessionIdsentinel)writeMetadata/updateMetadatacall will rewrite in JSON formatTests added
4 new tests in
packages/core/src/__tests__/metadata.test.ts:readMetadataparses key=value content from a.jsonfile (withruntimeHandle,tmuxName,status)readMetadataRawparses key=value content from a.jsonfilenullfor empty key=value filenullfor corrupt JSON that starts with{Validation
pnpm --filter @aoagents/ao-core test -- metadata— 47/47 passedpnpm --filter @aoagents/ao-core typecheck— clean