Skip to content

feat: convention-based skill queue registration via frontmatter#98

Merged
lukstafi merged 2 commits intomainfrom
ludics/task-b5a884c0-s5/root
Mar 28, 2026
Merged

feat: convention-based skill queue registration via frontmatter#98
lukstafi merged 2 commits intomainfrom
ludics/task-b5a884c0-s5/root

Conversation

@lukstafi
Copy link
Copy Markdown
Owner

Summary

  • Add queue-action frontmatter field to all 14 queueable skill templates
  • New src/skill-queue-registry.ts module scans frontmatter and builds action→command map
  • Refactored resolveQueueRequestCommand() to use registry with hardcoded fallbacks for special cases

Test plan

  • Verify all 14 skill queue actions resolve correctly
  • Verify special cases (briefing pre-hook, adopt-sessions pre-hook, message, adapter-followup, complete-task) still work
  • bun run typecheck passes
  • bun test passes

Generated with Claude Code

lukstafi and others added 2 commits March 28, 2026 17:35
Each skills/*.md file now declares its queue-action, queue-args, and
optional queue-args-defaults/queue-required-args fields so the
convention-based registry can discover the mapping automatically.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add src/skill-queue-registry.ts: lazy-loaded registry that scans
  skills/*.md frontmatter to build action→command mappings. Exports
  resolveSkillCommand(), hasRegisteredAction(), clearSkillQueueCache().
- Refactor resolveQueueRequestCommand() in src/mag.ts to three-tier
  dispatch: pre-hooks (briefing/adopt-sessions), auto-discovered skill
  commands via registry, programmatic-only actions (message/adapter-
  followup/complete-task) in a reduced switch.
- Add src/skill-queue-registry.test.ts: 26 tests covering all 14
  existing actions, required-arg null guard, defaults, and cache.

Adding a new queueable skill now requires only the template file.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lukstafi lukstafi force-pushed the ludics/task-b5a884c0-s5/root branch from f6cfb3f to 011d714 Compare March 28, 2026 16:35
@lukstafi lukstafi merged commit 17ee2f7 into main Mar 28, 2026
1 check passed
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f6cfb3fd11

ℹ️ 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".

Comment on lines +1087 to +1089
const skillCommand = resolveSkillCommand(action, request);
if (skillCommand !== null) {
return skillCommand;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restore fallback dispatch for known queue skill actions

This now hard-depends on resolveSkillCommand() succeeding for every non-programmatic queue action, but buildRegistry() returns an empty map whenever skills/ is missing or unreadable, so actions like briefing, draft-proposal, and verify-completion fall through and get dropped as unknown. That is a regression from the previous hardcoded switch behavior and can happen in binary-only or partial installs where skills is not colocated with the executable. Please keep a fallback mapping (or explicit fallbacks for existing actions) so queue dispatch still works when registry loading fails.

Useful? React with 👍 / 👎.


/** Parse YAML frontmatter block from a Markdown file, or return null. */
function parseFrontmatter(content: string): Record<string, unknown> | null {
const match = content.match(/^---\n([\s\S]*?)\n---/);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Parse frontmatter with CRLF-compatible delimiters

The frontmatter regex only matches LF line endings, so any skill file saved with CRLF (for example with core.autocrlf or Windows editors) will fail frontmatter parsing and never register its queue-action. In that environment, those queue requests are treated as unregistered and are dropped. Using a \r?\n-tolerant pattern avoids this platform-dependent dispatch failure.

Useful? React with 👍 / 👎.

@lukstafi
Copy link
Copy Markdown
Owner Author

Suggest Refactor — task-b5a884c0 (coder retrospective)

What I'd do differently next time

1. Validate the registry at startup, not silently at first use

The current design builds the registry lazily on the first resolveSkillCommand call.
If a skill file has malformed frontmatter (bad YAML, missing name field) the entry is
silently skipped. A better approach: add a validateSkillRegistry() function that
scans all files and logs a warning for each malformed entry, then call it once at
process start (e.g. in the mag queue-pop command handler). This surfaces authoring
errors early instead of causing a confusing "unknown action" failure at runtime.

2. Use a typed frontmatter schema for skill files

Right now parseFrontmatter() returns Record<string, unknown> and every field access
does manual type narrowing. The skill frontmatter schema is small and stable enough to
define a SkillFrontmatter interface and a parseSkillFrontmatter() function (similar
to parseTaskFrontmatter in src/tasks/markdown.ts). This would make the registry
code more readable, catch typos like queue-action vs queueAction at the parse site,
and make the schema self-documenting.

3. Co-locate the queue-pre-hook declaration in frontmatter rather than as inline if guards

The two pre-hooks (briefingPrecomputeContext, adoptSessionsPrecomputeContext) are
currently checked with plain if (action === "briefing") guards at the top of
resolveQueueRequestCommand. This works, but it means the connection between a skill
and its pre-hook is split across two files. A queue-pre-hook: briefingPrecomputeContext
frontmatter field (option 2 from the proposal) combined with a small hook registry map
(Record<string, () => Promise<void>>) would keep the definition fully in the skill
file and make resolveQueueRequestCommand a pure dispatcher with no hard-coded action
names at all.

4. Avoid String(request.task ?? "") pattern in favour of a typed request helper

Both the old switch and the new tier-3 switch use String(request.x ?? "") inline.
A tiny helper — requestString(request, key): string — would reduce boilerplate and
make it easy to add a debug-mode log ("missing expected field x on action y") in one
place rather than scattered across cases.

5. Consider a test fixture for the registry instead of reading live skill files

The registry test currently reads real skills/*.md files, which means it is sensitive
to the working directory and would break if a skill file is deleted or renamed. A thin
abstraction — buildRegistryFromDir(dir: string) exposed for testing — would allow
tests to supply a temporary directory with controlled fixture files, making them fully
hermetic. The live-file integration test can still exist as one test, but most unit
assertions would use fixtures.

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