fix(sr-2): correct ORDER BY direction in spec, clarify forge param JSDoc, add globalCavemanMode+missing-row test #885

Merged

charles merged 1 commit from dev/870 into main 2026-05-05 19:11:25 +00:00

Conversation 0 Commits 1 Files changed 7 +485 -5

charles commented 2026-05-05 18:15:08 +00:00

Owner

Copy link

Summary

Addresses three review findings from the SR-2 code review (#870):

1. specs/skills-rework.md — Fix ORDER BY direction

The spec incorrectly stated ORDER BY scope DESC and 'instance' > 'type'. The implementation already used ASC (correct, because 'i' = 0x69 < 't' = 0x74). Updated all three occurrences in the spec to match reality.

2. skill-loader.ts — Clarify forge param JSDoc on abortDispatchMissingSkill

Expanded the single-line forge parameter comment to explicitly state that the forge adapter is resolved from repo via createForgeAdapterForRepo — not from the forge argument itself. The non-obvious part was easy to miss.

3. renderPrompt.test.ts — Add missing coverage for globalCavemanMode=true + missing caveman row

The existing tests covered apply_caveman=1 + missing caveman row (graceful skip), but not the globalCavemanMode=true override path hitting the same null-check. New test proves the skip + appendix_missing warning fires correctly on the override branch too.

Tests

All 24 tests in the SR-2 suite pass (23 existing + 1 new).

## Summary Addresses three review findings from the SR-2 code review (#870): ### 1. `specs/skills-rework.md` — Fix `ORDER BY` direction The spec incorrectly stated `ORDER BY scope DESC` and `'instance' > 'type'`. The implementation already used `ASC` (correct, because `'i' = 0x69 < 't' = 0x74`). Updated all three occurrences in the spec to match reality. ### 2. `skill-loader.ts` — Clarify `forge` param JSDoc on `abortDispatchMissingSkill` Expanded the single-line `forge` parameter comment to explicitly state that the forge adapter is resolved from `repo` via `createForgeAdapterForRepo` — not from the `forge` argument itself. The non-obvious part was easy to miss. ### 3. `renderPrompt.test.ts` — Add missing coverage for `globalCavemanMode=true` + missing caveman row The existing tests covered `apply_caveman=1` + missing caveman row (graceful skip), but not the `globalCavemanMode=true` override path hitting the same null-check. New test proves the skip + `appendix_missing` warning fires correctly on the override branch too. ## Tests All 24 tests in the SR-2 suite pass (23 existing + 1 new).

charles added 3 commits 2026-05-05 18:15:08 +00:00

feat(skills): resolveSkillBody + renderPrompt + abortDispatchMissingSkill (SR-2) ... ae6731136c

Closes #870

fix(sr-2): address review findings from PR #884 ... 2ac0e36eb5

- agent-skill-store: replace ne(scope,'builtin') with eq(scope,'global') in
  getAgentTypeAppendixFlags — the negative filter was non-deterministic when
  multiple non-builtin rows existed for the same agent type name
- agent-skill-store: document ORDER BY scope ASC ASCII byte-order assumption
  with an explicit warning about future scope discriminator additions
- skill-loader: remove ne import (now unused after filter fix)
- skill-loader: document resolveSkillBody as intentionally async
- skill-loader: document forge param as log-only in abortDispatchMissingSkill
- skill-loader: use '(none)' for user/agent fields in persistTask call instead
  of agentType, which violates the PersistTaskInput field contract
- abort-missing-skill.test.ts: centralise _abortImpl restore in afterEach so
  a failing assertion can never leave a stub live for subsequent tests; wrap
  per-test warnSpy in try/finally for the same reason

fix(sr-2): correct ORDER BY direction in spec, clarify forge param JSDoc, add globalCavemanMode+missing-row test 36c15bc7b3

charles force-pushed dev/870 from 36c15bc7b3 to 561d08c1fa 2026-05-05 19:03:31 +00:00 Compare

charles merged commit 267f36e8a9 into main 2026-05-05 19:11:25 +00:00

charles deleted branch dev/870 2026-05-05 19:11:26 +00:00

charles referenced this pull request from a commit 2026-05-05 19:11:26 +00:00

fix(sr-2): correct ORDER BY direction in spec, clarify forge param JSDoc, add globalCavemanMode+missing-row test (#885)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

area:agents

Agent types, pool scheduling, per-instance config

  

area:dashboard

Dashboard UI and observability surfaces

  

area:database

DB layer — schema, migrations, ORM, raw SQL

  

area:design

UI/UX mockup work — routes to designer agent

  

area:design-review

Design review dispatch — routes to design-reviewer agent

  

area:flows

Flow runner — YAML loader, executor, op registry, expression eval

  

area:infra

Deployment, isolation, containers, systemd units

  

area:meta

Tracking, scaffolding, project setup

  

area:security

Security — routes to reviewer-security (opus)

  

area:sessions

Session-id store, Claude SDK resume logic

  

area:webhook

Forgejo webhook routing and handlers

  

area:workdir

Clone cache, worktrees, git identity

  

security

Security-sensitive issue

  

type:bug

Bug

  

type:chore

Chore

  

type:meta

Tracking or decisions, not implementation work

  

type:user-story

User story

No labels

area:agents

area:dashboard

area:database

area:design

area:design-review

area:flows

area:infra

area:meta

area:security

area:sessions

area:webhook

area:workdir

security

type:bug

type:chore

type:meta

type:user-story

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

charles/claude-hooks!885

No description provided.