Skip to content

feat(skills): Phase 2 skill bundle model#1493

Merged
josephfung merged 7 commits into
mainfrom
cursor/phase2-skill-bundle-model-0bc8
Jul 21, 2026
Merged

feat(skills): Phase 2 skill bundle model#1493
josephfung merged 7 commits into
mainfrom
cursor/phase2-skill-bundle-model-0bc8

Conversation

@josephfung

@josephfung josephfung commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #1489 (Phase 2 of epic #1436).

Important

Requires curia-deploy #179 to co-merge and co-deploy. This PR removes the
enable_task_management agent flag, and the agent-config schema is
additionalProperties: false. The social-media custom deploy agent still declares
the flag, so shipping core alone fails startup validation on deploy. curia-deploy
#179 migrates it to pinned_skills: [tasks, documents] — land and deploy the two together.

Introduces the skill (bundle) as a first-class collection of tools + optional instructions — the install / pin / discover unit — on top of Phase 1’s tool vocabulary (ADR-031).

What changed

  • Bundle modelSkillRegistry + SKILL.md parser; nested layout skills/<skill>/SKILL.md + tools/<tool>/
  • pinned_skills retarget — pins expand to member tools; instruction bodies inject into the system prompt
  • enable_task_management retired — replaced by pinning tasks + documents (independent skills)
  • Exemplars nestedcalendar, tasks, documents; remaining flat tools auto-wrap as synthetic singleton skills
  • Per-tool gating preserved — bundles do not flatten action_risk / allowed_callers
  • DB — migration 076_create_skill_registry.sql + RegistryKind: 'skill'
  • taskRepo wiring — enabled when either tasks (heartbeat) or documents is pinned

Open questions (resolved for this phase)

  • Skill-level gating — keep per-tool enforcement; no skill-level action_risk / allowed_callers aggregation
  • Fine-grained pin override — YAGNI (pin skill → all member tools)
  • Discovery of skills — deferred until Phase 3 (activation); toolSearch returns tools only for now

⚠️ Merge / deploy prerequisite (cross-repo)

schemas/agent-config.schema.json is additionalProperties: false. Startup validation (src/startup/validator.ts) rejects unknown agent YAML keys with a hard boot failure (unknown property: …).

curia-deploy/custom/agents/social-media.yaml still sets enable_task_management: true. Before deploying this PR, land a coordinated curia-deploy change that:

  1. Drops enable_task_management
  2. Pins tasks and documents instead

Without that, the next deploy fails agent YAML validation at startup.

Out of scope

Phases 3–5 (Claude-skill import, sandbox, external docs sync). Remaining cluster bundling tracked in #1494.

Test plan

  • pnpm run typecheck + console typecheck
  • pnpm run lint
  • Unit tests for pin-resolution / tasks+documents / calendar gating
  • Migrations apply (076 creates skill_registry; prefix unique vs 075)
  • taskRepo wired for heartbeat-only pin (tasks without documents)
  • Coordinated curia-deploy social-media.yaml migration before prod deploy
Open in Web Open in Cursor 

cursoragent and others added 2 commits July 21, 2026 16:27
Introduce skills as collections of tools + optional instructions. Agents
pin bundles via pinned_skills; bootstrap expands members and injects
SKILL.md bodies. Retire enable_task_management in favor of the
task-management skill. Nested layout for calendar and task-management;
skill_registry lifecycle table; per-tool action_risk preserved.

Closes #1489

Signed-off-by: Joseph Fung <joseph@josephfung.ca>
Signed-off-by: Cursor Agent <cursoragent@cursor.com>

Co-authored-by: Joseph Fung <josephfung@users.noreply.github.com>
Signed-off-by: Joseph Fung <joseph@josephfung.ca>
Signed-off-by: Cursor Agent <cursoragent@cursor.com>

Co-authored-by: Joseph Fung <josephfung@users.noreply.github.com>

@josephfung josephfung left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Strong Phase 2. The core reframe lands correctly: the tool stays the invocation/authorization atom, the skill (bundle) is layered on top as the pin/install/discover unit, and per-tool action_risk is genuinely preserved through bundle expansion. Registry split (ToolRegistry = Layer-1 runtime catalog, new SkillRegistry = Layer-2 lifecycle) matches design §4. Nice transitional touches: synthetic singletons so old atom-name pins keep resolving, lenient SKILL.md parsing that ignores unknown frontmatter for Phase-3 forward-compat.

Requesting a few changes before merge — one is a prod-boot blocker, two are simplifications that make the PR smaller and more correct.

🔴 Blocking

1. Cross-repo: a live custom agent still sets enable_task_management

This PR removes enable_task_management from the core schema, and schemas/agent-config.schema.json is additionalProperties: false. But curia-deploy/custom/agents/social-media.yaml:25 still declares enable_task_management: true. On the next deploy that agent's YAML fails strict validation at startup (unknown property) — a hard boot failure, or at best silent loss of heartbeat eligibility. curia-deploy is a separate repo this PR can't touch, so this needs a coordinated curia-deploy change (migrate social-media.yaml to pinned_skills: [tasks, documents], drop the flag) landed in lockstep. Please call it out as a merge/deploy prerequisite in the PR body, and confirm whether the startup validator rejects vs ignores the unknown key.

2. Decompose task-management into tasks + documents

The flag was two capabilities welded together. The frontmatter already exposes them as independent flags (heartbeat, document_workspace) and resolvePinnedSkills OR-accumulates across pins, so the split is mostly mechanical:

  • skills/tasks/SKILL.mdheartbeat: true, tools task-*, body = the ## Task Management section
  • skills/documents/SKILL.mddocument_workspace: true, tools doc-*, body = the ## Document Workspace section
  • coordinator, ceo-inbox, contacts each pin [tasks, documents] (no behavior regression)
  • update config/registry-defaults.yaml (skills: list) and the CHANGELOG entry

This split exposes a latent wiring coupling that must be fixed as part of it. taskRepo is currently gated only on documentWorkspaceEnabled (src/index.ts:2068), but it's used by both capabilities:

  • src/agents/runtime.ts:279 — documents use it to resolve a doc's project-root task (doc scoping)
  • src/agents/runtime.ts:468 — the task-wake scheduler refresh uses it — a tasks/heartbeat concern

Today both flags are always true together so it never bites. Once tasks can be pinned without documents, a heartbeat-eligible agent would get taskRepo: undefined and the scheduler-wake refresh silently no-ops. Re-gate so taskRepo is wired whenever tasks OR documents is present; keep workingDocsRepo/documentWorkspaceEnabled on documents only.

3. Delete the vestigial instruction/tool constants — SKILL.md is the source of truth

parseSkillMd returns the whole markdown body as instructions, resolvePinnedSkills collects it, and appendSkillInstructions injects it into the system prompt at bootstrap. So the pinned skill's SKILL.md body already is the injected block — the old block-insertion path is dead. TASK_MANAGEMENT_BLOCK / DOCUMENT_WORKSPACE_BLOCK now survive only because unit tests assert on them; that's two sources of truth kept married by a test (false confidence + guaranteed drift).

  • Delete TASK_MANAGEMENT_BLOCK, DOCUMENT_WORKSPACE_BLOCK, and the residual apply* helpers.
  • Rewrite the task/doc unit tests to exercise the real path (pin → resolvePinnedSkills → assert the SKILL.md body reaches the effective prompt), reading the actual SKILL.md, not a duplicated constant.
  • Membership is currently specified three ways (the tools/ dirs, the tools: frontmatter, and TASK_MANAGEMENT_TOOLS). Make the frontmatter tools: list authoritative and warn on drift bidirectionally (frontmatter tool with no dir and dir with no frontmatter entry). Drop the TASK_MANAGEMENT_TOOLS TS constant.

Net effect: ~150 lines of dead code removed and one source of truth per fact.

🟡 Should-fix (design coherence)

4. Discovery surfaces skills that can't yet be activated

toolSearch now returns kind: 'skill' bundle results, but runtime activation of a discovered skill is Phase 3 (tiered lookup + task-state persistence, design §6). A allow_discovery: true agent (coordinator) can now discover calendar/tasks as skills it has no mechanism to invoke or activate — risking a hallucinated call, plus a search for "calendar" returns both the bundle and its member atoms (dup/noise). Recommend either deferring kind: 'skill' results until Phase 3 lands activation, or explicitly instructing the LLM that skill results are informational-only for now.

5. The skillRegistry = toolRegistry local alias re-introduces the confusion the epic exists to kill

src/index.ts:1725 keeps a local skillRegistry pointing at the ToolRegistry while the actual SkillRegistry is named bundleRegistry. That inverted naming is exactly the tool-vs-skill ambiguity ADR-031 is removing. Please finish the local rename here (or a fast-follow noted in the PR). Same smell: effectivePinnedSkills now holds tool names.

🟢 Minor / verify

  • Expanded members that aren't loaded are dropped silently. resolvePinnedSkills validates the pin name but not expanded member tools; with calendar enabled while calendar-* stay off-until-credentialed, unloaded atoms vanish with no diagnostic. Re-add a debug/warn on missing expanded tools to preserve the old per-pin observability.
  • Migration 076 is unique now (075 = Phase 1 rename), but re-run ls src/db/migrations | sort at merge time per the known collision hazard.
  • Doc reconciliation: several enable_task_management references remain in specs (spec 19 §4 is a whole section named after it). Confirm specs 19/02/00/21 and adding-an-agent are fully reconciled to the two-skill model, not just touched.

Follow-up (not this PR)

Only calendar + task-management were bundled; everything else auto-wraps as synthetic singletons, so the §6 payoff ("pin 5 bundles, not 30 atoms") is only fractionally realized. Tracked separately in #1494 to bundle the remaining clusters (contacts, web, memory, scheduler, learning, context-bridge) and settle the synthetic-singleton end-state.

Decompose task-management into independent tasks + documents skills;
wire taskRepo for either pin; delete vestigial BLOCK/apply constants;
make frontmatter.tools authoritative with bidirectional drift warns;
defer kind:skill discovery until Phase 3; rename ToolRegistry alias.

Signed-off-by: Joseph Fung <joseph@josephfung.ca>
Signed-off-by: Cursor Agent <cursoragent@cursor.com>

Co-authored-by: Joseph Fung <josephfung@users.noreply.github.com>
@cursor

cursor Bot commented Jul 21, 2026

Copy link
Copy Markdown

Addressed review feedback in d516636:

Blocking

  1. Cross-repo deploy — Confirmed: startup Ajv validation rejects unknown agent keys (additionalProperties: false → hard boot failure with unknown property: enable_task_management). Called out as a merge/deploy prerequisite in the PR body; curia-deploy social-media.yaml must migrate to pinned_skills: [tasks, documents] in lockstep.
  2. Split task-managementtasks + documents — Done. Coordinator / ceo-inbox / contacts pin both. taskRepo is now wired when heartbeatEligible || documentWorkspaceEnabled; documentWorkspaceEnabled / workingDocs stay documents-only.
  3. Deleted vestigial constants — Removed TASK_MANAGEMENT_BLOCK / DOCUMENT_WORKSPACE_BLOCK / apply* / TASK_MANAGEMENT_TOOLS. SKILL.md is the sole instruction source; tests exercise resolvePinnedSkills against the real files. Frontmatter tools: is authoritative with bidirectional drift warnings.

Should-fix
4. Deferred kind: 'skill' in toolSearch until Phase 3 activation lands (avoids undriveable discovery noise).
5. Renamed the inverted skillRegistry = toolRegistry alias — in-memory catalog is skillRegistry (SkillRegistry); atoms use toolRegistry.

Minor

  • Warn when an expanded member tool isn’t loaded.
  • Specs 19 / 02 / 00 / 21 + adding-an-agent reconciled to the two-skill model.
  • Migration prefix still unique (075 / 076).

@josephfung
josephfung marked this pull request as ready for review July 21, 2026 19:41
@josephfung

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot added governance PII handling, audit integrity, compliance, and security hardening infra Deployment, monitoring, and operational tooling labels Jul 21, 2026
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Introduces first-class skill bundles backed by SKILL.md, separate tool and skill registries, nested tool discovery, bundle pin expansion, instruction injection, and synthetic singleton skills. Agents now pin bundles such as tasks, documents, and calendar; enable_task_management and the previous document-workspace injection APIs are removed. Calendar, task, and document manifests are added or relocated, with registry routes, migrations, configuration, documentation, and tests updated.

Possibly related issues

  • josephfung/curia#1494 — Covers a closely related Phase 2 skill-bundle implementation.
  • josephfung/curia#1436 — Tracks the broader tools-and-skills granularity rework.
  • josephfung/curia#1490 — Covers the Phase 3 foundation for imported Anthropic skill bundles.

Possibly related PRs

  • josephfung/curia#885 — Overlaps with task-management and heartbeat wiring replaced by bundle pinning.
  • josephfung/curia#933 — Relates to the registry infrastructure extended for skill lifecycle management.
  • josephfung/curia#1227 — Relates to document-workspace injection replaced by the documents bundle.

Suggested labels: orchestration, governance

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately captures the Phase 2 skill-bundle refactor.
Description check ✅ Passed The description is relevant and clearly describes the bundle-model changes and deployment caveat.
Linked Issues check ✅ Passed The PR covers bundle pin expansion, nested skill layout, task-flag retirement, discovery, DB changes, and docs as required.
Out of Scope Changes check ✅ Passed The extra file churn supports the skill-bundle refactor; nothing looks like a stray bit of bureaucratic baggage.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install timed out. The project may have too many dependencies for the sandbox.


Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🧹 Nitpick comments (2)
src/skills/skill-registry.ts (1)

12-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

An if-block that does precisely nothing.

Lines 20-23 test a condition and then... comment about it. No branch, no log, no validation. It compiles, it runs, it achieves the void. Either drop the dead conditional or make it earn its keep (e.g., logger?.debug for the zero-tools case), because right now it just sits there being philosophically empty.

♻️ Suggested cleanup
-    if (!manifest.tools || manifest.tools.length === 0) {
-      // Instruction-only skills (Phase 3 imports) may have zero tools; native
-      // Phase 2 skills should list tools. Allow empty for forward compat.
-    }
+    // Instruction-only skills (Phase 3 imports) may have zero tools; native
+    // Phase 2 skills should list tools. Allow empty for forward compat.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/skills/skill-registry.ts` around lines 12 - 32, Remove the no-op
empty-tools conditional from SkillRegistry.register, leaving duplicate-name
validation and skill registration unchanged.
src/index.ts (1)

2045-2059: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Rename pinnedSkills to pinnedTools
This now carries the expanded tool list, not skill names, so the current name is doing a rather poor job of telling the truth. AgentRuntime only forwards it, which makes a clearer field name the sensible option.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/index.ts` around lines 2045 - 2059, Rename the AgentRuntime constructor
option from pinnedSkills to pinnedTools at the call site and update the
corresponding AgentRuntime property, parameter, and forwarding references
consistently. Preserve the existing effectivePinnedTools value and behavior
while using pinnedTools throughout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@skills/calendar/SKILL.md`:
- Line 6: Update the calendar risk summary on line 6 to match the tool
manifests: keep list/find as none, classify ordinary event creation as medium,
distinguish hold placement using its declared risk, and explicitly document any
other calendar tools that are not high risk instead of grouping all creation
under high.

In `@skills/calendar/tools/calendar-check-conflicts/tool.json`:
- Around line 13-16: Add the handler-returned displayTimezone field to the
outputs declaration in skills/calendar/tools/calendar-check-conflicts/tool.json
at lines 13-16. Also add displayTimezone alongside the formatted display value
in skills/calendar/tools/calendar-create-hold/tool.json at lines 17-22, keeping
both manifests aligned with their user-facing timestamp contracts.

In `@skills/documents/tools/doc-search/tool.json`:
- Around line 11-17: Update the outputs schema for the document search tool so
displayTimezone is nullable and matches the sibling doc-write schema, allowing
null when the request context has no timezone. Preserve the existing string
output when a timezone is available.

In `@skills/tasks/tools/task-update/tool.json`:
- Line 16: Update the blocked_by_task_id schema declaration in task-update
tool.json to accept both string values and null, while preserving its
optional/omittable behavior. Ensure the corresponding parsing in the registry
produces a nullable optional field so handler.ts can continue using null to
clear an existing dependency.

In `@src/channels/http/routes/registry.ts`:
- Around line 27-32: Update the action handler’s invalid-kind response to list
skills alongside tools and agents as valid kinds. Keep parseKind’s existing
mapping unchanged and ensure callers receive the complete set of supported URL
segments.

In `@src/registry/registry-repo.ts`:
- Around line 96-99: Update the install query in the registry repository so
conflict handling does not issue an UPDATE or trigger skill_registry_updated_at.
Preserve returning the existing skill_registry row for duplicate names by using
an insert/select or retry-safe DO NOTHING flow, while retaining the current
behavior for new installs.

In `@src/skills/skill-loader.ts`:
- Around line 102-120: Update discoverNestedToolNames to accept an optional
logger and warn when parsing a nested tool.json fails, including the manifest
context and caught error details. Replace the silent catch while preserving
lenient discovery behavior by continuing to the remaining entries after logging.

In `@src/skills/skill-md.ts`:
- Around line 84-93: Update the frontmatter handling for heartbeat and
document_workspace in the skill metadata construction to validate their types
consistently with version and tools. Reject non-boolean values, including
strings such as "true", instead of converting them to undefined; preserve the
existing behavior for omitted fields and valid booleans.

---

Nitpick comments:
In `@src/index.ts`:
- Around line 2045-2059: Rename the AgentRuntime constructor option from
pinnedSkills to pinnedTools at the call site and update the corresponding
AgentRuntime property, parameter, and forwarding references consistently.
Preserve the existing effectivePinnedTools value and behavior while using
pinnedTools throughout.

In `@src/skills/skill-registry.ts`:
- Around line 12-32: Remove the no-op empty-tools conditional from
SkillRegistry.register, leaving duplicate-name validation and skill registration
unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e0f9203d-8160-41b0-adcf-ea884b251a14

📥 Commits

Reviewing files that changed from the base of the PR and between 457d080 and d516636.

📒 Files selected for processing (106)
  • CHANGELOG.md
  • CLAUDE.md
  • agents/calendar.yaml
  • agents/ceo-inbox.yaml
  • agents/contacts.yaml
  • agents/coordinator.yaml
  • config/registry-defaults.yaml
  • docs/dev/adding-a-tool.md
  • docs/dev/adding-an-agent.md
  • docs/specs/00-overview.md
  • docs/specs/02-agent-system.md
  • docs/specs/19-tasks-and-backlog.md
  • docs/specs/21-agent-document-workspace.md
  • schemas/agent-config.schema.json
  • skills/calendar/SKILL.md
  • skills/calendar/tools/calendar-check-conflicts/handler.test.ts
  • skills/calendar/tools/calendar-check-conflicts/handler.ts
  • skills/calendar/tools/calendar-check-conflicts/tool.json
  • skills/calendar/tools/calendar-create-event/handler.test.ts
  • skills/calendar/tools/calendar-create-event/handler.ts
  • skills/calendar/tools/calendar-create-event/tool.json
  • skills/calendar/tools/calendar-create-hold/handler.test.ts
  • skills/calendar/tools/calendar-create-hold/handler.ts
  • skills/calendar/tools/calendar-create-hold/tool.json
  • skills/calendar/tools/calendar-delete-event/handler.ts
  • skills/calendar/tools/calendar-delete-event/tool.json
  • skills/calendar/tools/calendar-find-free-time/handler.test.ts
  • skills/calendar/tools/calendar-find-free-time/handler.ts
  • skills/calendar/tools/calendar-find-free-time/tool.json
  • skills/calendar/tools/calendar-holds-sweep/handler.test.ts
  • skills/calendar/tools/calendar-holds-sweep/handler.ts
  • skills/calendar/tools/calendar-holds-sweep/tool.json
  • skills/calendar/tools/calendar-list-calendars/handler.ts
  • skills/calendar/tools/calendar-list-calendars/tool.json
  • skills/calendar/tools/calendar-list-events/handler.test.ts
  • skills/calendar/tools/calendar-list-events/handler.ts
  • skills/calendar/tools/calendar-list-events/tool.json
  • skills/calendar/tools/calendar-register/handler.ts
  • skills/calendar/tools/calendar-register/tool.json
  • skills/calendar/tools/calendar-respond-to-invite/handler.test.ts
  • skills/calendar/tools/calendar-respond-to-invite/handler.ts
  • skills/calendar/tools/calendar-respond-to-invite/tool.json
  • skills/calendar/tools/calendar-update-event/handler.ts
  • skills/calendar/tools/calendar-update-event/tool.json
  • skills/documents/SKILL.md
  • skills/documents/tools/doc-list/handler.test.ts
  • skills/documents/tools/doc-list/handler.ts
  • skills/documents/tools/doc-list/tool.json
  • skills/documents/tools/doc-read/handler.test.ts
  • skills/documents/tools/doc-read/handler.ts
  • skills/documents/tools/doc-read/tool.json
  • skills/documents/tools/doc-search/handler.test.ts
  • skills/documents/tools/doc-search/handler.ts
  • skills/documents/tools/doc-search/tool.json
  • skills/documents/tools/doc-write/handler.test.ts
  • skills/documents/tools/doc-write/handler.ts
  • skills/documents/tools/doc-write/tool.json
  • skills/tasks/SKILL.md
  • skills/tasks/tools/task-complete/handler.test.ts
  • skills/tasks/tools/task-complete/handler.ts
  • skills/tasks/tools/task-complete/tool.json
  • skills/tasks/tools/task-create/handler.test.ts
  • skills/tasks/tools/task-create/handler.ts
  • skills/tasks/tools/task-create/tool.json
  • skills/tasks/tools/task-list/handler.test.ts
  • skills/tasks/tools/task-list/handler.ts
  • skills/tasks/tools/task-list/tool.json
  • skills/tasks/tools/task-update/handler.test.ts
  • skills/tasks/tools/task-update/handler.ts
  • skills/tasks/tools/task-update/tool.json
  • src/agents/document-workspace.ts
  • src/agents/loader.ts
  • src/agents/task-management.ts
  • src/channels/calendar/nylas-calendar-client.test.ts
  • src/channels/http/routes/registry.ts
  • src/db/migrations/076_create_skill_registry.sql
  • src/db/queries/tasks.ts
  • src/index.ts
  • src/registry/reconcile.test.ts
  • src/registry/reconcile.ts
  • src/registry/registry-repo.ts
  • src/registry/registry-service.ts
  • src/registry/types.ts
  • src/scheduler/backlog-heartbeat.ts
  • src/skills/execution.ts
  • src/skills/loader.ts
  • src/skills/pin-resolution.ts
  • src/skills/skill-loader.ts
  • src/skills/skill-md.ts
  • src/skills/skill-registry.ts
  • src/skills/skill-types.ts
  • src/skills/types.ts
  • tests/integration/calendar-holds-collaboration.test.ts
  • tests/integration/escalation-ux.test.ts
  • tests/unit/agents/calendar-prompt.test.ts
  • tests/unit/agents/document-workspace.test.ts
  • tests/unit/agents/task-management.test.ts
  • tests/unit/skills/calendar-check-conflicts.test.ts
  • tests/unit/skills/calendar-create-event.test.ts
  • tests/unit/skills/calendar-delete-event.test.ts
  • tests/unit/skills/calendar-find-free-time.test.ts
  • tests/unit/skills/calendar-list-calendars.test.ts
  • tests/unit/skills/calendar-list-events.test.ts
  • tests/unit/skills/calendar-register.test.ts
  • tests/unit/skills/calendar-update-event.test.ts
  • tests/unit/skills/pin-resolution.test.ts
💤 Files with no reviewable changes (6)
  • tests/unit/agents/task-management.test.ts
  • schemas/agent-config.schema.json
  • src/agents/loader.ts
  • src/agents/task-management.ts
  • tests/unit/agents/document-workspace.test.ts
  • src/agents/document-workspace.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 8

🧹 Nitpick comments (2)
src/skills/skill-registry.ts (1)

12-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

An if-block that does precisely nothing.

Lines 20-23 test a condition and then... comment about it. No branch, no log, no validation. It compiles, it runs, it achieves the void. Either drop the dead conditional or make it earn its keep (e.g., logger?.debug for the zero-tools case), because right now it just sits there being philosophically empty.

♻️ Suggested cleanup
-    if (!manifest.tools || manifest.tools.length === 0) {
-      // Instruction-only skills (Phase 3 imports) may have zero tools; native
-      // Phase 2 skills should list tools. Allow empty for forward compat.
-    }
+    // Instruction-only skills (Phase 3 imports) may have zero tools; native
+    // Phase 2 skills should list tools. Allow empty for forward compat.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/skills/skill-registry.ts` around lines 12 - 32, Remove the no-op
empty-tools conditional from SkillRegistry.register, leaving duplicate-name
validation and skill registration unchanged.
src/index.ts (1)

2045-2059: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Rename pinnedSkills to pinnedTools
This now carries the expanded tool list, not skill names, so the current name is doing a rather poor job of telling the truth. AgentRuntime only forwards it, which makes a clearer field name the sensible option.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/index.ts` around lines 2045 - 2059, Rename the AgentRuntime constructor
option from pinnedSkills to pinnedTools at the call site and update the
corresponding AgentRuntime property, parameter, and forwarding references
consistently. Preserve the existing effectivePinnedTools value and behavior
while using pinnedTools throughout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@skills/calendar/SKILL.md`:
- Line 6: Update the calendar risk summary on line 6 to match the tool
manifests: keep list/find as none, classify ordinary event creation as medium,
distinguish hold placement using its declared risk, and explicitly document any
other calendar tools that are not high risk instead of grouping all creation
under high.

In `@skills/calendar/tools/calendar-check-conflicts/tool.json`:
- Around line 13-16: Add the handler-returned displayTimezone field to the
outputs declaration in skills/calendar/tools/calendar-check-conflicts/tool.json
at lines 13-16. Also add displayTimezone alongside the formatted display value
in skills/calendar/tools/calendar-create-hold/tool.json at lines 17-22, keeping
both manifests aligned with their user-facing timestamp contracts.

In `@skills/documents/tools/doc-search/tool.json`:
- Around line 11-17: Update the outputs schema for the document search tool so
displayTimezone is nullable and matches the sibling doc-write schema, allowing
null when the request context has no timezone. Preserve the existing string
output when a timezone is available.

In `@skills/tasks/tools/task-update/tool.json`:
- Line 16: Update the blocked_by_task_id schema declaration in task-update
tool.json to accept both string values and null, while preserving its
optional/omittable behavior. Ensure the corresponding parsing in the registry
produces a nullable optional field so handler.ts can continue using null to
clear an existing dependency.

In `@src/channels/http/routes/registry.ts`:
- Around line 27-32: Update the action handler’s invalid-kind response to list
skills alongside tools and agents as valid kinds. Keep parseKind’s existing
mapping unchanged and ensure callers receive the complete set of supported URL
segments.

In `@src/registry/registry-repo.ts`:
- Around line 96-99: Update the install query in the registry repository so
conflict handling does not issue an UPDATE or trigger skill_registry_updated_at.
Preserve returning the existing skill_registry row for duplicate names by using
an insert/select or retry-safe DO NOTHING flow, while retaining the current
behavior for new installs.

In `@src/skills/skill-loader.ts`:
- Around line 102-120: Update discoverNestedToolNames to accept an optional
logger and warn when parsing a nested tool.json fails, including the manifest
context and caught error details. Replace the silent catch while preserving
lenient discovery behavior by continuing to the remaining entries after logging.

In `@src/skills/skill-md.ts`:
- Around line 84-93: Update the frontmatter handling for heartbeat and
document_workspace in the skill metadata construction to validate their types
consistently with version and tools. Reject non-boolean values, including
strings such as "true", instead of converting them to undefined; preserve the
existing behavior for omitted fields and valid booleans.

---

Nitpick comments:
In `@src/index.ts`:
- Around line 2045-2059: Rename the AgentRuntime constructor option from
pinnedSkills to pinnedTools at the call site and update the corresponding
AgentRuntime property, parameter, and forwarding references consistently.
Preserve the existing effectivePinnedTools value and behavior while using
pinnedTools throughout.

In `@src/skills/skill-registry.ts`:
- Around line 12-32: Remove the no-op empty-tools conditional from
SkillRegistry.register, leaving duplicate-name validation and skill registration
unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e0f9203d-8160-41b0-adcf-ea884b251a14

📥 Commits

Reviewing files that changed from the base of the PR and between 457d080 and d516636.

📒 Files selected for processing (106)
  • CHANGELOG.md
  • CLAUDE.md
  • agents/calendar.yaml
  • agents/ceo-inbox.yaml
  • agents/contacts.yaml
  • agents/coordinator.yaml
  • config/registry-defaults.yaml
  • docs/dev/adding-a-tool.md
  • docs/dev/adding-an-agent.md
  • docs/specs/00-overview.md
  • docs/specs/02-agent-system.md
  • docs/specs/19-tasks-and-backlog.md
  • docs/specs/21-agent-document-workspace.md
  • schemas/agent-config.schema.json
  • skills/calendar/SKILL.md
  • skills/calendar/tools/calendar-check-conflicts/handler.test.ts
  • skills/calendar/tools/calendar-check-conflicts/handler.ts
  • skills/calendar/tools/calendar-check-conflicts/tool.json
  • skills/calendar/tools/calendar-create-event/handler.test.ts
  • skills/calendar/tools/calendar-create-event/handler.ts
  • skills/calendar/tools/calendar-create-event/tool.json
  • skills/calendar/tools/calendar-create-hold/handler.test.ts
  • skills/calendar/tools/calendar-create-hold/handler.ts
  • skills/calendar/tools/calendar-create-hold/tool.json
  • skills/calendar/tools/calendar-delete-event/handler.ts
  • skills/calendar/tools/calendar-delete-event/tool.json
  • skills/calendar/tools/calendar-find-free-time/handler.test.ts
  • skills/calendar/tools/calendar-find-free-time/handler.ts
  • skills/calendar/tools/calendar-find-free-time/tool.json
  • skills/calendar/tools/calendar-holds-sweep/handler.test.ts
  • skills/calendar/tools/calendar-holds-sweep/handler.ts
  • skills/calendar/tools/calendar-holds-sweep/tool.json
  • skills/calendar/tools/calendar-list-calendars/handler.ts
  • skills/calendar/tools/calendar-list-calendars/tool.json
  • skills/calendar/tools/calendar-list-events/handler.test.ts
  • skills/calendar/tools/calendar-list-events/handler.ts
  • skills/calendar/tools/calendar-list-events/tool.json
  • skills/calendar/tools/calendar-register/handler.ts
  • skills/calendar/tools/calendar-register/tool.json
  • skills/calendar/tools/calendar-respond-to-invite/handler.test.ts
  • skills/calendar/tools/calendar-respond-to-invite/handler.ts
  • skills/calendar/tools/calendar-respond-to-invite/tool.json
  • skills/calendar/tools/calendar-update-event/handler.ts
  • skills/calendar/tools/calendar-update-event/tool.json
  • skills/documents/SKILL.md
  • skills/documents/tools/doc-list/handler.test.ts
  • skills/documents/tools/doc-list/handler.ts
  • skills/documents/tools/doc-list/tool.json
  • skills/documents/tools/doc-read/handler.test.ts
  • skills/documents/tools/doc-read/handler.ts
  • skills/documents/tools/doc-read/tool.json
  • skills/documents/tools/doc-search/handler.test.ts
  • skills/documents/tools/doc-search/handler.ts
  • skills/documents/tools/doc-search/tool.json
  • skills/documents/tools/doc-write/handler.test.ts
  • skills/documents/tools/doc-write/handler.ts
  • skills/documents/tools/doc-write/tool.json
  • skills/tasks/SKILL.md
  • skills/tasks/tools/task-complete/handler.test.ts
  • skills/tasks/tools/task-complete/handler.ts
  • skills/tasks/tools/task-complete/tool.json
  • skills/tasks/tools/task-create/handler.test.ts
  • skills/tasks/tools/task-create/handler.ts
  • skills/tasks/tools/task-create/tool.json
  • skills/tasks/tools/task-list/handler.test.ts
  • skills/tasks/tools/task-list/handler.ts
  • skills/tasks/tools/task-list/tool.json
  • skills/tasks/tools/task-update/handler.test.ts
  • skills/tasks/tools/task-update/handler.ts
  • skills/tasks/tools/task-update/tool.json
  • src/agents/document-workspace.ts
  • src/agents/loader.ts
  • src/agents/task-management.ts
  • src/channels/calendar/nylas-calendar-client.test.ts
  • src/channels/http/routes/registry.ts
  • src/db/migrations/076_create_skill_registry.sql
  • src/db/queries/tasks.ts
  • src/index.ts
  • src/registry/reconcile.test.ts
  • src/registry/reconcile.ts
  • src/registry/registry-repo.ts
  • src/registry/registry-service.ts
  • src/registry/types.ts
  • src/scheduler/backlog-heartbeat.ts
  • src/skills/execution.ts
  • src/skills/loader.ts
  • src/skills/pin-resolution.ts
  • src/skills/skill-loader.ts
  • src/skills/skill-md.ts
  • src/skills/skill-registry.ts
  • src/skills/skill-types.ts
  • src/skills/types.ts
  • tests/integration/calendar-holds-collaboration.test.ts
  • tests/integration/escalation-ux.test.ts
  • tests/unit/agents/calendar-prompt.test.ts
  • tests/unit/agents/document-workspace.test.ts
  • tests/unit/agents/task-management.test.ts
  • tests/unit/skills/calendar-check-conflicts.test.ts
  • tests/unit/skills/calendar-create-event.test.ts
  • tests/unit/skills/calendar-delete-event.test.ts
  • tests/unit/skills/calendar-find-free-time.test.ts
  • tests/unit/skills/calendar-list-calendars.test.ts
  • tests/unit/skills/calendar-list-events.test.ts
  • tests/unit/skills/calendar-register.test.ts
  • tests/unit/skills/calendar-update-event.test.ts
  • tests/unit/skills/pin-resolution.test.ts
💤 Files with no reviewable changes (6)
  • tests/unit/agents/task-management.test.ts
  • schemas/agent-config.schema.json
  • src/agents/loader.ts
  • src/agents/task-management.ts
  • tests/unit/agents/document-workspace.test.ts
  • src/agents/document-workspace.ts
🛑 Comments failed to post (8)
skills/calendar/SKILL.md (1)

6-6: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Make the calendar risk summary match the tool manifests.

Line 6 groups all creation as high risk, but calendar-create-hold/tool.json declares action_risk: "medium". The universe has somehow produced two risk matrices for one calendar; distinguish ordinary event creation from hold placement and document the other non-high-risk tools explicitly.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@skills/calendar/SKILL.md` at line 6, Update the calendar risk summary on line
6 to match the tool manifests: keep list/find as none, classify ordinary event
creation as medium, distinguish hold placement using its declared risk, and
explicitly document any other calendar tools that are not high risk instead of
grouping all creation under high.
skills/calendar/tools/calendar-check-conflicts/tool.json (1)

13-16: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Standardise calendar timestamp outputs.

Calendar skills expose user-facing timestamps, but their manifests do not consistently declare the required displayTimezone field. Because these manifests define discovery and output contracts, align both locations before this paperwork grows teeth.

  • skills/calendar/tools/calendar-check-conflicts/tool.json#L13-L16: add the displayTimezone field returned by handler.ts.
  • skills/calendar/tools/calendar-create-hold/tool.json#L17-L22: add displayTimezone alongside the formatted display value.
📍 Affects 2 files
  • skills/calendar/tools/calendar-check-conflicts/tool.json#L13-L16 (this comment)
  • skills/calendar/tools/calendar-create-hold/tool.json#L17-L22
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@skills/calendar/tools/calendar-check-conflicts/tool.json` around lines 13 -
16, Add the handler-returned displayTimezone field to the outputs declaration in
skills/calendar/tools/calendar-check-conflicts/tool.json at lines 13-16. Also
add displayTimezone alongside the formatted display value in
skills/calendar/tools/calendar-create-hold/tool.json at lines 17-22, keeping
both manifests aligned with their user-facing timestamp contracts.

Source: Coding guidelines

skills/documents/tools/doc-search/tool.json (1)

11-17: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

displayTimezone should probably be string?, not string.

Marginally thrilling news: this field is documented as always present, but the sibling doc-write/tool.json marks the same field "string?", and the codebase convention (see calendar-find-free-time/handler.ts in this very PR) sets it to null when ctx.timezone is undefined. Might as well be consistent — not that consistency has ever cheered anyone up.

📝 Proposed fix
-    "displayTimezone": "string"
+    "displayTimezone": "string?"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

  "outputs": {
    "query": "string",
    "path_prefix": "string",
    "match_count": "number",
    "matches": "array of {path, line_number, line}",
    "displayTimezone": "string?"
  },
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@skills/documents/tools/doc-search/tool.json` around lines 11 - 17, Update the
outputs schema for the document search tool so displayTimezone is nullable and
matches the sibling doc-write schema, allowing null when the request context has
no timezone. Preserve the existing string output when a timezone is available.
skills/tasks/tools/task-update/tool.json (1)

16-16: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the files and inspect the relevant sections.
git ls-files 'skills/tasks/tools/task-update/*' 'skills/tasks/tools/*' | sed -n '1,120p'

printf '\n--- tool.json ---\n'
cat -n skills/tasks/tools/task-update/tool.json | sed -n '1,120p'

printf '\n--- handler references ---\n'
rg -n "blocked_by_task_id|updateTask|null" skills/tasks/tools/task-update -S

Repository: josephfung/curia

Length of output: 5377


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n--- comparable tool manifests ---\n'
for f in skills/tasks/tools/task-create/tool.json skills/tasks/tools/task-complete/tool.json skills/tasks/tools/task-list/tool.json; do
  echo "### $f"
  cat -n "$f" | sed -n '1,120p'
  echo
done

printf '\n--- task-update handler ---\n'
cat -n skills/tasks/tools/task-update/handler.ts | sed -n '1,220p'

Repository: josephfung/curia

Length of output: 10758


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n--- schema/docs for tool input types ---\n'
rg -n "\"string\\?|\"number\\?|\"boolean\\?|\"timestamp\\?|\\?\"|nullable|optional" src skills README.md docs -S

printf '\n--- parser/validator for tool manifests ---\n'
rg -n "tool.json|inputs|parse.*schema|schema.*tool|nullable|optional" src skills -S

Repository: josephfung/curia

Length of output: 50373


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n--- tool schema parser / type syntax ---\n'
rg -n "string\\?|number\\?|boolean\\?|timestamp\\?|object\\?|\\[\\]\\?|nullable|optional|undefined|null" src skills -S | sed -n '1,220p'

printf '\n--- likely schema/type implementation files ---\n'
fd -a ".*(skill|tool|schema|input|manifest).*\\.(ts|js|json)$" src skills | sed -n '1,200p'

Repository: josephfung/curia

Length of output: 29007


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n--- skill type definitions ---\n'
cat -n src/skills/skill-types.ts | sed -n '1,240p'

printf '\n--- skill loader / manifest parsing ---\n'
cat -n src/skills/skill-loader.ts | sed -n '1,260p'

printf '\n--- skill registry ---\n'
cat -n src/skills/skill-registry.ts | sed -n '1,260p'

Repository: josephfung/curia

Length of output: 12946


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n--- parser candidates ---\n'
rg -n "parse.*tool|tool manifest|ToolManifest|inputs.*string\\?|endsWith\\('\\?'\\)|endsWith\\(\"\\?\"\\)|nullable|optional" src/skills skills -S

printf '\n--- skill / tool related source files ---\n'
fd -a ".*(tool|skill|manifest|registry).*\\.(ts|js)$" src | sed -n '1,200p'

Repository: josephfung/curia

Length of output: 28480


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n--- src/skills/registry.ts relevant section ---\n'
cat -n src/skills/registry.ts | sed -n '120,190p'

printf '\n--- src/skills/types.ts relevant ToolManifest section ---\n'
cat -n src/skills/types.ts | sed -n '31,120p'

Repository: josephfung/curia

Length of output: 10112


Allow blocked_by_task_id to be nullable. handler.ts treats null as the way to clear the dependency, but string? is parsed as optional string in src/skills/registry.ts, so the generated schema still rejects null. Callers can omit the field, but they cannot clear an existing blocker — an irritating little contract mismatch.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@skills/tasks/tools/task-update/tool.json` at line 16, Update the
blocked_by_task_id schema declaration in task-update tool.json to accept both
string values and null, while preserving its optional/omittable behavior. Ensure
the corresponding parsing in the registry produces a nullable optional field so
handler.ts can continue using null to clear an existing dependency.
src/channels/http/routes/registry.ts (1)

27-32: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Include skills in the invalid-kind response.

Line 31 accepts skills, but the action handler still tells callers that only tools or agents are valid. The UI will receive misleading guidance, which is about as useful as a tube map in a power cut.

Proposed fix
-          error: `Unknown kind '${rawKind}' (expected 'tools' or 'agents')`,
+          error: `Unknown kind '${rawKind}' (expected 'tools', 'agents', or 'skills')`,
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/channels/http/routes/registry.ts` around lines 27 - 32, Update the action
handler’s invalid-kind response to list skills alongside tools and agents as
valid kinds. Keep parseKind’s existing mapping unchanged and ensure callers
receive the complete set of supported URL segments.
src/registry/registry-repo.ts (1)

96-99: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Keep an idempotent install from rewriting updated_at.

The conflict update fires skill_registry_updated_at, so reinstalling an existing skill changes its audit timestamp despite the comment promising it is left untouched. A rather elaborate way for time to become fiction.

Use an insert/select or retry-safe DO NOTHING flow that returns the existing row without issuing an UPDATE.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/registry/registry-repo.ts` around lines 96 - 99, Update the install query
in the registry repository so conflict handling does not issue an UPDATE or
trigger skill_registry_updated_at. Preserve returning the existing
skill_registry row for duplicate names by using an insert/select or retry-safe
DO NOTHING flow, while retaining the current behavior for new installs.
src/skills/skill-loader.ts (1)

102-120: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Empty catch {} swallows nested tool.json failures without a peep.

Lines 115-117 catch and discard, no log, no propagate. Guidelines here are unambiguous: every catch must log, audit, and propagate. The comment claims it's "reported when tool discovery runs," but that's a different function on a different call path — this one, right here, says nothing to anyone. Truly the sound of one hand clapping.

🔧 Proposed fix — thread an optional logger and warn
-export function discoverNestedToolNames(skillDir: string): string[] {
+export function discoverNestedToolNames(skillDir: string, logger?: Logger): string[] {
   const toolsDir = path.join(skillDir, 'tools');
   if (!fs.existsSync(toolsDir)) return [];
   const names: string[] = [];
   for (const entry of fs.readdirSync(toolsDir, { withFileTypes: true })) {
     if (!entry.isDirectory()) continue;
     const manifestPath = path.join(toolsDir, entry.name, 'tool.json');
     if (!fs.existsSync(manifestPath)) continue;
     try {
       const raw = JSON.parse(fs.readFileSync(manifestPath, 'utf-8')) as { name?: unknown };
       if (typeof raw.name === 'string' && raw.name.trim()) {
         names.push(raw.name.trim());
       }
-    } catch {
-      // Lenient: broken nested tool is reported when tool discovery runs.
+    } catch (err) {
+      logger?.warn(
+        { manifestPath, error: err instanceof Error ? err.message : String(err) },
+        'skill discovery: failed to read nested tool.json',
+      );
     }
   }
   return names;
 }
🧰 Tools
🪛 ast-grep (0.44.1)

[warning] 110-110: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(manifestPath, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').

(detect-non-literal-fs-filename-typescript)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/skills/skill-loader.ts` around lines 102 - 120, Update
discoverNestedToolNames to accept an optional logger and warn when parsing a
nested tool.json fails, including the manifest context and caught error details.
Replace the silent catch while preserving lenient discovery behavior by
continuing to the remaining entries after logging.

Source: Coding guidelines

src/skills/skill-md.ts (1)

84-93: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Invalid heartbeat/document_workspace vanish into the void, silently.

Rows 84-85 turn anything other than a literal boolean true into undefined — no throw, no warning, nothing. Meanwhile version and tools two blocks up correctly throw when given the wrong type. A skill author who writes heartbeat: "true" (a string, easy typo) gets a skill that quietly isn't heartbeat-eligible, and no signal as to why. Riveting way to lose an afternoon debugging.

🔧 Proposed fix — validate instead of silently discarding
-  const heartbeat = fm.heartbeat === true ? true : undefined;
-  const document_workspace = fm.document_workspace === true ? true : undefined;
+  if (fm.heartbeat !== undefined && typeof fm.heartbeat !== 'boolean') {
+    throw new Error(`${sourceLabel}: frontmatter.heartbeat must be a boolean when set`);
+  }
+  if (fm.document_workspace !== undefined && typeof fm.document_workspace !== 'boolean') {
+    throw new Error(`${sourceLabel}: frontmatter.document_workspace must be a boolean when set`);
+  }
+  const heartbeat = fm.heartbeat === true ? true : undefined;
+  const document_workspace = fm.document_workspace === true ? true : undefined;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

  if (fm.heartbeat !== undefined && typeof fm.heartbeat !== 'boolean') {
    throw new Error(`${sourceLabel}: frontmatter.heartbeat must be a boolean when set`);
  }
  if (fm.document_workspace !== undefined && typeof fm.document_workspace !== 'boolean') {
    throw new Error(`${sourceLabel}: frontmatter.document_workspace must be a boolean when set`);
  }
  const heartbeat = fm.heartbeat === true ? true : undefined;
  const document_workspace = fm.document_workspace === true ? true : undefined;

  return {
    name: name.trim(),
    description: description.trim(),
    version,
    tools,
    heartbeat,
    document_workspace,
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/skills/skill-md.ts` around lines 84 - 93, Update the frontmatter handling
for heartbeat and document_workspace in the skill metadata construction to
validate their types consistently with version and tools. Reject non-boolean
values, including strings such as "true", instead of converting them to
undefined; preserve the existing behavior for omitted fields and valid booleans.

Align calendar risk docs and displayTimezone manifests, make skill
registry install conflict-safe, tighten skill-md boolean parsing, and
rename AgentRuntime pinnedSkills to pinnedTools.

Signed-off-by: Cursor Agent <cursoragent@cursor.com>

Co-authored-by: Joseph Fung <josephfung@users.noreply.github.com>
Comment thread src/channels/http/routes/registry.ts
Comment thread skills/documents/tools/doc-search/tool.json
Comment thread skills/calendar/tools/calendar-check-conflicts/tool.json
Comment thread skills/tasks/tools/task-update/tool.json
Comment thread src/skills/skill-registry.ts
Comment thread src/skills/skill-md.ts
Comment thread skills/calendar/SKILL.md
Comment thread src/registry/registry-repo.ts
Comment thread src/index.ts
Comment thread src/skills/skill-loader.ts
@cursor

cursor Bot commented Jul 21, 2026

Copy link
Copy Markdown

CodeRabbit’s 8 actionable + 2 nitpick findings are addressed in f6b2758. Their inline review comments failed to post on GitHub (platform limit/error), so replies are filed as path/line notes on each site rather than in_reply_to threads.

Summary: calendar risk/docs + displayTimezone manifests, doc-search/task-update schema fixes, registry invalid-kind + skill install DO NOTHING, skill-loader warn, skill-md boolean validation, empty-if cleanup, pinnedSkillspinnedTools.

Comment thread skills/calendar/tools/calendar-create-hold/tool.json
CI failed because task-update's blocked_by_task_id used string|null?, which
the manifest parser rejected. Parse T|null unions into JSON Schema type arrays.

Signed-off-by: Cursor Agent <cursoragent@cursor.com>

Co-authored-by: Joseph Fung <josephfung@users.noreply.github.com>
Comment thread skills/tasks/tools/task-update/tool.json
@josephfung

Copy link
Copy Markdown
Owner Author

@CodeRabbit review

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot added orchestration Multi-agent coordination — delegation, specialist agents, skill routing and removed infra Deployment, monitoring, and operational tooling labels Jul 21, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/smoke/harness.ts`:
- Around line 203-204: Update the harness configuration assignment around
pinnedTools to use the resolved values from pinResolution: pass
pinResolution.toolNames instead of agentPinnedSkills, and pass its generated
tool definitions instead of agentToolDefs. Mirror the resolution flow used in
src/index.ts so skill bundles expand into member tools and preserve their
associated instructions and capability flags.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4ca42bd5-dffe-4c1d-ae87-34ade061b47a

📥 Commits

Reviewing files that changed from the base of the PR and between d516636 and 1c974b0.

📒 Files selected for processing (24)
  • CHANGELOG.md
  • skills/calendar/SKILL.md
  • skills/calendar/tools/calendar-check-conflicts/tool.json
  • skills/calendar/tools/calendar-create-hold/tool.json
  • skills/documents/tools/doc-search/tool.json
  • skills/tasks/tools/task-update/handler.ts
  • skills/tasks/tools/task-update/tool.json
  • src/agents/runtime.ts
  • src/channels/http/routes/registry.ts
  • src/index.ts
  • src/registry/registry-repo.ts
  • src/skills/registry.ts
  • src/skills/skill-loader.ts
  • src/skills/skill-md.ts
  • src/skills/skill-registry.ts
  • tests/integration/multi-agent-delegation.test.ts
  • tests/integration/research-analyst-multi-turn.test.ts
  • tests/integration/setup-wizard-delegate.test.ts
  • tests/integration/skill-invocation.test.ts
  • tests/smoke/harness.ts
  • tests/unit/agents/runtime.test.ts
  • tests/unit/skills/calendar-check-conflicts.test.ts
  • tests/unit/skills/pin-resolution.test.ts
  • tests/unit/skills/registry.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
  • skills/tasks/tools/task-update/handler.ts
  • skills/calendar/tools/calendar-check-conflicts/tool.json
  • tests/unit/skills/calendar-check-conflicts.test.ts
  • skills/calendar/SKILL.md
  • src/skills/skill-md.ts
  • src/skills/skill-loader.ts
  • skills/tasks/tools/task-update/tool.json
  • tests/unit/skills/pin-resolution.test.ts
  • skills/calendar/tools/calendar-create-hold/tool.json
  • src/skills/skill-registry.ts
  • src/index.ts

Comment thread tests/smoke/harness.ts Outdated
Mirror src/index.ts: load SkillRegistry, resolvePinnedSkills, append
instruction blocks, and pass expanded tool names/defs to AgentRuntime.

Signed-off-by: Cursor Agent <cursoragent@cursor.com>

Co-authored-by: Joseph Fung <josephfung@users.noreply.github.com>
EmailAdapter.poll now guards non-array list results, and the watermark
persistence tests keep returning [] on interval ticks after Once mocks.

Signed-off-by: Cursor Agent <cursoragent@cursor.com>

Co-authored-by: Joseph Fung <josephfung@users.noreply.github.com>
@josephfung
josephfung merged commit 81d0d3b into main Jul 21, 2026
12 checks passed
@cursor
cursor Bot deleted the cursor/phase2-skill-bundle-model-0bc8 branch July 21, 2026 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

governance PII handling, audit integrity, compliance, and security hardening orchestration Multi-agent coordination — delegation, specialist agents, skill routing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Phase 2: bundle (skill) model

2 participants