docs: remove the boxel-development skill; factory loads the boxel skill#5571
docs: remove the boxel-development skill; factory loads the boxel skill#5571jurgenwerk wants to merge 1 commit into
Conversation
…-12269) .agents/skills/boxel-development was a fork of the boxel references that ship in the boxel-cli plugin (built from boxel-skills). Its additive content is merged upstream via boxel-workspaces; the fork is deleted and every consumer repointed: - factory-skill-loader resolves the boxel skill from the plugin skills dir; REFERENCE_KEYWORD_MAP / ALWAYS_LOAD_REFERENCES use the upstream reference names, with keyword rows for the references that had no dev-* twin; file/media issues now pull in the boxel-file-def skill - operations/bootstrap skill pointers, the orchestrator twin, the realm Wiki copy, software-factory CLAUDE.md, and boxel-api's cross-pointer name the boxel skill - tests and smoke tests updated to match Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9fb5f3280
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * in the `boxel-file-def` skill (FileDef, ImageDef, MarkdownDef, and the | ||
| * binary-persistence rules). | ||
| */ | ||
| const FILE_DEF_KEYWORDS = ['file', 'asset', 'FileDef', 'upload', 'image']; |
There was a problem hiding this comment.
Match all file-media issue terms
Expand this predicate to cover the file types named by the skill (for example MarkdownDef, PngDef, CsvFileDef, document, and media). As written, a non-bootstrap issue such as “add a MarkdownDef preview” or “attach a PDF document” does not match any of these five terms, so the resolver omits boxel-file-def even though that skill contains the required FileDef subtype, import, and binary-persistence guidance.
Useful? React with 👍 / 👎.
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖]
Lens. This is a mechanical fork-removal + repoint, so the review focused on the two ways that class of change breaks silently: (1) does every reference filename the loader now curates actually resolve against the built boxel skill, and (2) was any consumer or reference left pointing at the deleted fork? I checked out the head branch and verified the curated names against packages/boxel-cli/plugin/skills/boxel/references/ on disk.
Bottom line: no blocking issues. The repoint is thorough and internally consistent. Two things are worth a conscious decision before merge — both non-blocking: one reference (technical-rules.md) silently became unreachable on the issue path, and the file/media keyword set is narrower than the skill it gates. Detail is in the two inline threads.
What lands right (verified, not assumed):
- All 27 filenames in
REFERENCE_KEYWORD_MAPexist in the builtboxel/references/directory. The "no dev-* twin" additions the description mentions (container-query-fitted-layout,base-field-catalog,date-math,formatters,icons, …) are all present and correctly keyed. - Scoping is clean:
grepfinds no dangling reference to the deleted.agents/skills/boxel-development/directory. The remainingboxel-developmentstrings (runtime-common/constants.tsdevSkillLocalPath, the experiments-realmEnvironmentJSON, the matrix skills spec) all name the separate legacy realmSkill/boxel-developmentcard, which this PR correctly leaves alone. - Every doc pointer moved together:
boxel-api, the operations/bootstrap skills, the orchestrator twin, the realm Wiki copy, and.claude/CLAUDE.mdall nameboxel, andCLAUDE.md's fallback-dir listing now matches reality (root.agents/skills/contains onlyember-best-practices, confirmed on disk). - Tests, smoke tests, and the
filterBoxelDevelopmentRefs→filterBoxelRefsrename are coherent; no staledev-*fixture names remain insrc/,tests/, orscripts/.
Recommendations:
- Decide whether
technical-rules.mdshould stay reachable on the issue path — it's in neither list now. (thread onALWAYS_LOAD_REFERENCES; non-blocking) - Consider a validation assertion that curated reference filenames resolve on disk, now that they're owned by boxel-skills. (same thread; follow-up)
- Widen
FILE_DEF_KEYWORDSto the termsboxel-file-defadvertises. (thread onFILE_DEF_KEYWORDS; follow-up — the automated reviewer flagged the same)
Adjacent, out of scope: packages/software-factory/docs/phase-1-plan.md and phase-2-plan.md still describe the old boxel-development skill and its dev-*.md reference layout; they read as frozen design records, so I'd leave them, just noting it for whoever next edits those docs. Separately, matchesAnyKeyword's mixed-case branch (lowerKw !== lowerKw.toLowerCase()) is dead code — lowerKw is already lowercased — so mixed-case tokens like FileDef/CardDef fall through to word-boundary matching rather than the intended substring matching. Pre-existing, harmless for standalone tokens, and not this PR's to fix.
Generated by Claude Code
| 'core-concept.md', | ||
| 'quick-reference.md', | ||
| 'common-imports.md', | ||
| 'qunit-testing.md', | ||
| 'spec-usage.md', |
There was a problem hiding this comment.
[Claude Code 🤖]
technical-rules.md is now unreachable whenever an issue is present — likely unintended. In the old fork dev-technical-rules.md was in ALWAYS_LOAD_REFERENCES, so its detailed cardinal-rule / validation-checklist content was injected on every issue. In the new mapping technical-rules.md — which does exist on disk in the built boxel skill's references/ — is in neither ALWAYS_LOAD_REFERENCES nor REFERENCE_KEYWORD_MAP. filterBoxelRefs therefore drops it on every issue-driven load (the normal factory path): it isn't always-loaded, and with no keyword row nothing can ever match it.
This one stands out because every other on-disk reference got at least one keyword row in this change — the description says rows were added "so issue-filtering doesn't silently drop them." technical-rules.md is the lone reference that fell through: it lost its always-load slot (swapped for common-imports.md) and gained no keyword row.
It may be deliberate — the boxel SKILL.md body now carries a condensed Cardinal Rules table (rules 1–14) inline, and core-concept.md is always-loaded — but unlike the qunit-testing.md gap you called out in the description, this one isn't noted anywhere. Recommend a conscious decision:
- add
technical-rules.mdback toALWAYS_LOAD_REFERENCES(restores prior behavior), or - give it a keyword row (e.g.
['rule', 'validation', 'checklist']), or - if the inline table is meant to supersede it, say so in a code comment.
Scope: behavioral change introduced by this PR. Non-blocking, but worth an explicit call.
Secondary point on this block — qunit-testing.md and the absence of filename validation. As the description notes, qunit-testing.md isn't in the built boxel/references/ yet (confirmed — it's absent on disk), so this always-load entry is a silent no-op until boxel-skills regenerates. That's fine as documented, because filterBoxelRefs filters the refs actually read from disk, so a not-yet-present name simply never appears — no error.
The broader observation: nothing validates that the curated names in ALWAYS_LOAD_REFERENCES / REFERENCE_KEYWORD_MAP resolve against the built skill. Now that these filenames are owned by a separate repo (boxel-skills, pinned via BOXEL_SKILLS_VERSION), an upstream rename would silently drop a reference with no failure — the same mechanism that makes the qunit-testing.md gap harmless also makes drift invisible. A small unit/smoke assertion that each curated name exists in the boxel skill's references/ would catch this class (and would flag qunit-testing.md as a reminder to drop the transitional note once it lands). Scope: follow-up, non-blocking.
Generated by Claude Code
| * in the `boxel-file-def` skill (FileDef, ImageDef, MarkdownDef, and the | ||
| * binary-persistence rules). | ||
| */ | ||
| const FILE_DEF_KEYWORDS = ['file', 'asset', 'FileDef', 'upload', 'image']; |
There was a problem hiding this comment.
[Claude Code 🤖]
FILE_DEF_KEYWORDS is narrower than the skill it gates (the automated reviewer flagged this too — confirming and expanding with the mechanism). boxel-file-def's own SKILL.md description reads: "Use when adding or working with file-typed fields (FileDef, ImageDef, MarkdownDef, PngDef, CsvFileDef) … reference an image, document, or other file asset." The gate here is only ['file', 'asset', 'FileDef', 'upload', 'image'].
Verified against matchesAnyKeyword (below in this file): plain words match on word boundaries, so an issue titled "add a MarkdownDef preview" or "attach a PDF document" matches none of the five terms, and the resolver omits boxel-file-def — the one skill written for that work.
Two mitigating facts so this reads accurately: this PR already improved coverage (it added image, which the old dev-file-def.md keyword row lacked), so this is incremental, not a regression; and the main boxel skill's Cardinal Rule 11 covers binary-persistence at a high level even when boxel-file-def isn't pulled in.
Suggest widening the set toward the skill's advertised trigger surface (term selection is a judgment call, so leaving this as a snippet rather than a suggestion block):
const FILE_DEF_KEYWORDS = [
'file',
'asset',
'FileDef',
'ImageDef',
'MarkdownDef',
'PngDef',
'CsvFileDef',
'upload',
'image',
'document',
'media',
];The mixed-case tokens (ImageDef, MarkdownDef, …) still match via the word-boundary path, which is correct for standalone occurrences. Scope: follow-up, non-blocking.
Generated by Claude Code
.agents/skills/boxel-developmentwas a fork of theboxelskill's references that ship in the boxel-cli plugin (built from the boxel-skills repo). The fork's only consumers were the software factory's two skill paths; its genuinely additive content (four items out of twenty files) is merged upstream via the boxel-workspaces sources, so the fork is deleted here and every consumer repointed at theboxelskill.factory-skill-loader.ts: the resolver loadsboxel;REFERENCE_KEYWORD_MAP/ALWAYS_LOAD_REFERENCESuse the upstream reference filenames, with keyword rows added for upstream references that had no dev-* twin (container-query-fitted-layout, defensive-link-traversal, base-field-catalog, date-math, formatters, icons, …) so issue-filtering doesn't silently drop them. File/media-flavored issues now also pull in theboxel-file-defskill, replacing the fork's file-def reference.boxel. The boxel-cli change is that one doc line.smoke:skillexercises the real plugin skill end-to-end with reference filtering.One transitional note:
qunit-testing.mdsits in the always-load list but only reaches the plugin'sboxelcopy after boxel-skills regenerates from the upstream merge and the pinnedBOXEL_SKILLS_VERSIONbumps past it. Until then the filter simply doesn't match it (no error), and the operations skill's own QUnit section covers the gap.🤖 Generated with Claude Code