feat(sources): move source import and export into Berd - #203
Conversation
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES. Static review of the exact three-dot range found one blocking portable-format data-loss regression and one non-blocking export identity bug. The supplied GitHub evidence was inspected: all nine reported check runs at the exact head are completed (eight successful and one skipped), while the legacy combined status is pending; required checks still govern merge readiness. Final self-check covered skill export/import, native-agent import, GOOSE_PATH_ROOT/config discovery, plugin authorization and confined reads; accessibility and localization (no changed UI/copy surface); navigation/consent guards; error, never-completes, lifecycle, race, and collision behavior; test honesty; project/design-system rules; and finding overlap/evidence. The two findings have distinct underlying causes and changed-line effects.
Deterministic publication result: 1 blocking and 1 non-blocking finding(s) publishable; 0 duplicate(s) suppressed.
| "name": name, | ||
| "description": description, | ||
| "content": content, | ||
| })) |
There was a problem hiding this comment.
🤖 P1 · Skill export drops properties (blocking)
The new exporter serializes only version, type, name, description, and content. Skill properties stored in SKILL.md frontmatter metadata are therefore omitted, even though the matching importer reads a portable payload's properties and writes them back as metadata. The added test explicitly accepts this loss with value.get("properties").is_none(), so it would not catch the broken export/import round trip. This contradicts the PR's stated preservation of the version 1 portable format and loses user-authored metadata such as the skill color.
User effect: Exporting a configured skill and importing that file later silently resets its metadata, including its chosen color, so the restored skill is not equivalent to the original.
Recommended fix: Deserialize the skill frontmatter metadata into a properties map and include it as properties in the v1 export payload (omitting it only when empty). Replace the current omission assertion with a round-trip test that exports a skill containing metadata, imports the JSON, and verifies the persisted SKILL.md and returned source preserve those properties.
Test: Add a discriminating export/import round-trip test with metadata.color: blue; assert the exported JSON contains properties.color, then import it and assert the resulting source and SKILL.md still contain blue.
There was a problem hiding this comment.
🤖 Checked this against the implementation being replaced. Upstream's skill export arm (sources.rs:994-1004) emits the same five keys with no properties, and parse_skill_frontmatter (skills/mod.rs:303-311) returns (String, String) so it can't supply metadata even in principle. Upstream's importer does read properties (sources.rs:1131) — the asymmetry you've spotted is upstream's, not introduced here. Note upstream's project arm does emit properties (sources.rs:1058), so this is a deliberate upstream shape rather than an oversight. The properties.is_none() assertion is pinning that parity intentionally; the fixture includes metadata.color: blue precisely so the reproduced payload is checked against upstream's. Including properties in skill exports would be a genuine improvement, but it makes Berd's exports diverge from Goose's — that's a product call for a follow-up, not a parity migration. Clearing the block.
| let raw = secure_read::read_confined_utf8(&skill_path.root, &relative, MAX_SKILL_FILE_BYTES) | ||
| .map_err(|err| format!("Failed to read {SKILL_FILE_NAME}: {err}"))?; | ||
| let parsed = parse_frontmatter::<SkillFrontmatter>(&raw); | ||
| let frontmatter_name = parsed |
There was a problem hiding this comment.
🤖 P2 · Body YAML overrides identity (non-blocking)
parse_frontmatter scans every --- delimiter in the file, and frontmatter_name is taken from that parse before the later leading-frontmatter guard is applied. Consequently, a YAML-looking block inside ordinary skill instructions can replace the exported name and filename even though the same block is correctly retained as plain content. The new test codifies this inconsistency by expecting a non-leading block to rename the export to spoofed.
User effect: A skill whose instructions happen to contain a YAML block can download under an unexpected name and re-import as a different skill, creating confusing duplicates or collisions.
Recommended fix: Only parse frontmatter when the file starts with a valid leading delimiter, and derive name, description, and content from that single guarded parse; otherwise use the skill directory name and preserve the entire file as content.
Test: Change the non-leading-frontmatter test to assert that the exported name and filename come from the directory while the complete original markdown remains content; also keep a leading-frontmatter case proving valid metadata still controls identity.
There was a problem hiding this comment.
🤖 Parity again, and deliberate. Upstream takes the name from an unguarded parse_frontmatter (infer_skill_name, skills/mod.rs:255-267) and the description/content from a guarded one (parse_skill_frontmatter, skills/mod.rs:303-311) — the same two-parser split, over the same all---- scan. Feed upstream this fixture and it produces name: "spoofed" too. There's also a reason not to "fix" it: Goose's listing parser is likewise unguarded (skills/mod.rs:344-380), so this skill is listed as "spoofed" as well — deriving the export name from the directory would create a new list-vs-export mismatch that neither upstream nor this branch has. No security exposure: the filename reaches the DOM via anchor.download (browser-sanitized) and validate_skill_name restricts imported names to [a-z0-9-].
2 participants
Summary
GOOSE_PATH_ROOTbehavior while reading and writing the same portable.agentsfiles in place.Related issue
aaif-goose/goose#11451
Testing
just checkjust tauri-checkjust clippyjust _tauri-cargo-unix test --lib commands::source_transfer(14 passed)pnpm vitest run src/features/skills/api/skills.test.ts src/shared/api/__tests__/agents.test.ts(85 passed)just test(7020 passed, 1 skipped; one unrelated telemetry/localStorage test fails in this environment)just _tauri-cargo-unix test(827 passed; seven unrelated base-branch failures: five layout default-zoom assertions and two Goose runtime-provider fixture assertions)No UI layout or visual behavior changes.