Skip to content

feat(skills): spec-2.3 skill invocation#95

Merged
sqlrush merged 3 commits into
mainfrom
spec-2.3-skill-invocation
Jun 6, 2026
Merged

feat(skills): spec-2.3 skill invocation#95
sqlrush merged 3 commits into
mainfrom
spec-2.3-skill-invocation

Conversation

@sqlrush

@sqlrush sqlrush commented Jun 5, 2026

Copy link
Copy Markdown
Owner

What

spec-2.3-skill-invocation implementation (design: opendbrb PR #80, APPROVED @ R2 2026-06-05, 三路 pre-impl review + 用户 path 4/4).

Third leg of the Skills chain: SkillTool + description injection + body injection + allowed-tools enforcement.

Deliverables

D What Where
D-1 invoke.SkillTool — wire name Skill (Q3 CC parity); args 5-shape validation; semantic failures recoverable / ctx fatal (FROZEN 1.21 two-track); Q7 model notice internal/app/skills/invoke/
D-2 skills.PromptSection — name-sorted; pinned first-line extraction; leaf-pure internal/app/skills/prompt.go
D-3 ToolOutput.ToolFilter (additive) + Loop enforcement: per-turn advertise filter (nil = slice identity), dispatch guard after lookup / before dedup with full results+emit pairing, scope update reads out (never !cached-gated), intra-turn immediate (Q12), Skill implicitly retained (Q13) diagnose/{tool,loop,scope}.go
D-4 Skill(<name>) per-tool adapter + args summary block/adapter/skill.go
D-5 bootstrap skillsForChat: log+skip, never panic (user mandate); real seam llmapp.Options.SystemPrompt (production's first system prompt — empty→non-empty, Q10); R-10 plugin provenance + R-7 64KiB warnings bootstrap/{skills,tui_launcher}.go
D-6 3 errcodes + templates; SCOPE_TOOL_DENIED registered in diagnose (loop composes denial text; invoke→diagnose import is one-way — cycle-free placement, TOOL_TIMEOUT precedent) invoke/errors.go + diagnose/errors.go
D-7 integration: golden code-reviewer.md full chain (invoke→body→narrow→denied paired→implicit retention→replace→real exec) + cached replay keeps scope tests/integration/skillinvoke/
D-8 imports allowlist tests: app/skills leaf + directional parent-not-child rule + invoke allowlist skills/imports_test.go

User-mandated regressions (approve note 2026-06-05) ✅

  • D-3 regression first: TestRun_ScopeDenied_PairedAndNotStuck — denied tool emits EventToolCall+EventToolResult (UI never stuck Running), transcript paired-commit intact
  • D-5 no panic: TestSkillsForChat_ContractViolation_LogSkipNoPanic — SkillTool construction error → log + skip, interact continues

规则 21 全调用方验证(ToolOutput 加字段 + loop.go 改动)

  • ToolOutput callers: tool.go(定义) / tool_clock.go / tool_echo.go(零值 nil 语义不变) / dedup.go(存整结构,重放测试) / loop.go(classifyToolErr 只拷 Content/IsError — wire 隔离,测) / 全部测试字面量 named-field(grep 证实 0 positional)→ 既有 diagnose 测试 0 修改全绿
  • loop.go: 9-arm FinishReason switch 结构不动;FinishToolUse 臂内 additive(guard + scope update + applyFilter 包 :180);race ×2 clean;unknown-tool terminal 回归测试保留

Quality

  • Coverage: skills 96.4% / invoke 94.3% / diagnose 93.4% / adapter 92.1% / bootstrap 89.9%(全 ≥85%)
  • make gate Layer-2 PASSED;go mod tidy 0-diff(0 新依赖);import-rules-check 0 violation;errcode manifest +3(sorted append)
  • gen-docs 再生 opendbrb error-codes.md(68 码,设计仓分支同步提交)

偏离点(实现层,post-impl review 注意)

  1. SCOPE_TOOL_DENIED 注册位置 = diagnose/errors.go(spec D-6 表写 invoke/errors.go)— invoke→diagnose 单向 import 强制;code 字符串/三件套/用途均按 spec。三路 pre-impl review 未覆盖此拓扑细节;建议 FROZEN 时 spec § D-6 加一行 errata 注。
  2. R-10/R-11 警告在 startup(bootstrap)而非 invoke 时 — invoke 包 import 集不含 logger(D-8 allowlist 即 spec 自身锁定);v1 body/source 启动后静态,启动期检查严格更早。

UI review(规则 20)

D-4 是新增 per-tool adapter(共用组件 registry 追加条目,不改既有 adapter):Layer 1 invariants + 既有 block golden/PTY 全绿。repo 无 area:render label——Layer 4 触发待 spec-0.11.5 infra;Layer 5 截图随 post-impl 手验一并。

Next

  • CI 8 job 绿
  • 三路 post-impl review
  • FROZEN: registry append (ordinal) + 双仓 tag via tag-spec.sh

Spec: spec-2.3-skill-invocation.md (opendbrb PR #80)

sqlrush added 2 commits June 6, 2026 00:23
Why: third leg of the Skills chain — let the model see, select, and
enter skill scopes inside the diagnose loop.

- invoke.SkillTool (wire name "Skill", CC parity Q3): body-as-tool_result
  (explicit deviation Q11), args five-shape validation, Q7 model notice,
  semantic failures recoverable / ctx fatal (two-track, FROZEN 1.21)
- skills.PromptSection: name-sorted available-skills section; injected
  via llmapp.Options.SystemPrompt (production's first system prompt,
  empty→non-empty cache transition, Q10)
- diagnose.ToolOutput.ToolFilter (additive) + Loop scope enforcement:
  applyFilter per turn (nil keeps slice identity), dispatch guard after
  lookup / before dedup with full results+emit pairing (denied tools
  never stuck Running; transcript stays paired — user regression
  mandate), scope update reads out (never gated on !cached), intra-turn
  immediate effect (Q12), "Skill" implicitly retained (Q13)
- block adapter "Skill(<name>)" header + args summary
- bootstrap skillsForChat: log+skip on contract violation (never
  panics — user mandate), R-10 plugin provenance + R-7 64KiB body
  warnings at startup
- 3 errcodes (NOT_FOUND/INVOKE_INVALID in invoke; SCOPE_TOOL_DENIED in
  diagnose — loop composes the denial text, cycle-free placement) +
  central manifest + gen-error-codes blank-import
- imports_test: app/skills leaf allowlist + directional parent-not-
  child rule; invoke allowlist
- integration: golden code-reviewer.md full chain + cached replay

Coverage: skills 96.4% / invoke 94.3% / diagnose 93.4% / adapter 92.1%
/ bootstrap 89.9%. Zero new deps; existing diagnose tests unmodified.

Spec: spec-2.3-skill-invocation.md
Why: claude post-impl 双路 review (code-reviewer + go-reviewer 均
APPROVE-WITH-FIXES, 0 CRIT/0 HIGH) findings absorb:

- cr MED-1: scopeDeniedContent → colon-style template per spec D-6
  pinned contract (was bracket style; invisible to Contains-based tests)
- cr MED-2: Layer-2 Skill adapter cases (running / args summary /
  no-skill placeholder) + parked fixture dirs + ParkedFixtures list
- go MED-2: errcode.Newf sentinel references (ErrInvokeInvalid.Code() /
  skills.ErrNamespaceConflict.Code()) replace string literals — a code
  rename now breaks the build, not first runtime call
- cr LOW-2 / go MED-1 (dual-hit): new logger.InfoForceFile (mirrors
  WarnForceFile); 'skills active' success line demoted to info band
- cr LOW-1: skill discovery gated on perr == nil — no wasted fs scan
  on the provider-error path
- go LOW-2: scope-update comment notes nil ToolFilter = inherit (never
  clears) on both fresh and cached paths
- go LOW-3: applyFilter identity test tightened (len/cap guard before
  element-0 address compare)
- cr NIT-1: PromptSection defensively dedupes duplicate keys (first
  occurrence wins; unreachable in production via 2.2 Active contract)

make gate Layer-2 PASSED; existing tests still 0-modification green.

Spec: spec-2.3-skill-invocation.md
@sqlrush

sqlrush commented Jun 6, 2026

Copy link
Copy Markdown
Owner Author

Post-impl review trace — Route 1 (claude code-reviewer)

Reviewer: claude-code-reviewer (independent run)
Scope: full diff main...spec-2.3-skill-invocation @ 2bf2387 (21 files) + surrounding FROZEN
       loop.go/dedup.go/schema.go/validate.go/llm.go(ValidateRequest)/tui_launcher.go
Artifact: this comment (adversarial Q&A 全文见 trace 归档)
Findings:
  CRIT: 0
  HIGH: 0
  MED:  2  (scopeDeniedContent bracket-style 偏离 spec D-6 colon 模板;
            D-4 Layer-2 PTY golden 缺 Skill 用例 — DoD 项未闭)
  LOW:  2  (provider-error 路径仍跑 discovery 浪费 I/O; 'skills active' 用 Warn 级)
  NIT:  1  (PromptSection 重复 name 防御缺口 — 生产不可达)
Verdict: APPROVE-WITH-FIXES
关键裁定: dispatch guard 位置/配对 ✓; cached-hit scope 重放 ✓; timeout 不更新 scope ✓;
  ' alpha' 前导空格 → NOT_FOUND 判 spec-conformant; 嵌套 args key-sorted ✓;
  security: 错误模板回显面不超出 spec R-10 已接受范围; imports_test 启发式 sound

Post-impl review trace — Route 2 (claude go-reviewer)

Reviewer: claude-go-reviewer (independent run; 与 Route 1 不同 agent 入口)
Scope: 同 diff @ 2bf2387; go vet/staticcheck/golangci-lint clean; race clean
Artifact: this comment
Findings:
  CRIT: 0
  HIGH: 0
  MED:  2  (errcode.Newf 裸字符串码 → 应引 sentinel.Code() 防 rename 漂移;
            'skills active' Warn 级污染告警带 — 与 Route 1 LOW-2 收敛)
  LOW:  3  (aliasing 链 AllowedToolsList→store→replay 追踪后判 clean — 备案;
            scope 更新处 nil=inherit 注释缺口; applyFilter identity 测试脆弱)
  NIT:  2  (jsonTypeName 死分支防御性 OK; adapter init() 注册符合规则 12 例外)
Verdict: APPROVE-WITH-FIXES
关键裁定: SkillTool 构造后真不可变 ✓; activeFilter 无 goroutine 逃逸 ✓;
  双轨契约全程正确 ✓; classifyToolErr 只拷 Content/IsError = wire 隔离 ✓

R-fix (af55de6) — 全部 findings absorb

Finding Fix
cr MED-1 模板 bracket→colon scopeDeniedContent 改 spec D-6 pinned colon 形态
cr MED-2 Layer-2 Skill 用例缺 +3 cases (running / args summary / no-skill) + parked dirs + ParkedFixtures 清单
go MED-2 裸字符串码 ErrInvokeInvalid.Code() + skills.ErrNamespaceConflict.Code() sentinel 引用
双路收敛 log level logger.InfoForceFile(镜像 WarnForceFile); 成功行降 info 带
cr LOW-1 error-path I/O discovery gated on perr == nil
go LOW-2/LOW-3 + cr NIT-1 注释补 nil=inherit / identity 测试加 len/cap guard / PromptSection 防御 dedupe

make gate Layer-2 PASSED;既有测试仍 0 修改全绿。

待 Route 3 (codex) post-impl review + 用户 review → FROZEN(registry append + 双仓 tag)。

@sqlrush

sqlrush commented Jun 6, 2026

Copy link
Copy Markdown
Owner Author

Codex Route 3/3 — Post-Impl Review

Verdict: APPROVE-WITH-FIXES

Findings

MED-1 — InfoForceFile still logs as WARN, so normal skill startup telemetry pollutes the warning band.
internal/bootstrap/skills.go:163 explicitly uses logger.InfoForceFile for the normal path ("skills active for this session") and the comment says it must be info-band, not warn. But internal/platform/logger/slog_handler.go:33-34 calls forceFile(LevelInfo, ...), and forceFileAttrs clamps every level below LevelWarn up to WARN at internal/platform/logger/slog_handler.go:187-191. Result: the normal successful skill activation record is written as [WARN], contradicting the R-fix contract and making startup warnings noisy. Existing slog handler tests cover slog Info routing, but not direct InfoForceFile, so this slipped through.

Recommended fix: route InfoForceFile directly to fileOnlyAttrs(LevelInfo, ...) (or add a no-clamp helper), keep WarnForceFile/ErrorForceFile clamped, and add a direct test asserting InfoForceFile("skills active...") writes [INFO] and still never writes stderr under DebugToStderr.

NIT-1 — New source comments still contain Chinese rule references.
The PR adds Chinese text in comments at internal/app/diagnose/scope.go:33, internal/bootstrap/skills.go:163, and internal/bootstrap/skills.go:171. This is small, but the repo has repeatedly enforced English-only source comments under Rule 13. Suggested sweep: translate these to Rule 12, post-impl dual-route finding, and Rule 7 wording.

Verified

  • Required PR checks are green on af55de6.
  • Targeted local tests passed: go test ./internal/app/diagnose ./internal/app/skills/... ./internal/bootstrap ./tests/integration/skillinvoke.
  • Core spec-2.3 contracts look sound: system-prompt seam is real, scope denial emits paired tool results and does not leave UI Running, ctx errors stay fatal, Skill is retained as the scope-control tool, same-turn scope changes are covered, and dedup replay preserves ToolFilter.

Why: codex post-impl Route 3 (APPROVE-WITH-FIXES) absorb:

- MED: InfoForceFile was routed through forceFile, whose
  forceFileAttrs clamps any level below WARN up to WARN — the normal
  'skills active' startup line landed in the warning band as [WARN].
  Now calls fileOnlyAttrs(LevelInfo, ...) directly (same
  bypass-debug-gate, no-stderr, no-op-before-Init semantics) + direct
  test asserting [INFO] band, file reached with debug disabled, no
  stderr, and explicitly NOT [WARN].
- NIT: Rule-13 sweep — Chinese rule references in NEW comments
  translated to English (scope.go / invoke errors.go+skilltool.go /
  bootstrap skills.go / 3 test files). Pre-existing FROZEN-era Chinese
  comments in diagnose errors.go/loop.go left untouched (out of scope).

make gate Layer-2 PASSED.

Spec: spec-2.3-skill-invocation.md
@sqlrush

sqlrush commented Jun 6, 2026

Copy link
Copy Markdown
Owner Author

codex R-fix (64ba25a) — Route 3 两项闭合

Finding Fix
MED: InfoForceFile 被 clamp 成 [WARN] 根因确认:forceFileAttrs< LevelWarn 一律抬到 WARN(warn/error 带归一器)。改为直走 fileOnlyAttrs(LevelInfo, ...)(同样绕过 debug gate、不进 stderr、Init 前 no-op)。Direct test TestInfoForceFileKeepsInfoBand:断言 [INFO] 落盘(DebugEnabled=false)、stderr 零写入、且显式断言 [WARN]
NIT: Rule-13 中文注释扫尾 新增注释中 9 处中文规则引用全部英化(scope.go / invoke errors.go + skilltool.go / bootstrap skills.go / scope_test + invoke 两个 test)。diff 新增行中文计数 = 0。FROZEN 既有中文注释(diagnose errors.go/loop.go 的 1.21 时代文本)不在本 spec 范围,未动。

make gate Layer-2 PASSED + CI 三次绿(run 27048700756)。

三路 post-impl 全部闭合(cr + go + codex 均 APPROVE-WITH-FIXES → R-fix ×2 absorb)。待用户 review/merge → FROZEN(registry append ordinal 53 + tag-spec.sh 双仓 v0.53.0-stage2.53)。

@sqlrush

sqlrush commented Jun 6, 2026

Copy link
Copy Markdown
Owner Author

Codex Route 3 — Final R-fix Verification

Verdict: APPROVE (recorded as a comment because GitHub does not allow the current authenticated user to formally approve their own PR).

Verified head: 64ba25a

Closed findings:

  • MED-1 fixed: InfoForceFile now bypasses the warn/error clamp and writes LevelInfo directly. New TestInfoForceFileKeepsInfoBand asserts [INFO], no [WARN], file write with debug disabled, and no stderr write.
  • NIT-1 fixed: Rule-13 sweep translated the new Chinese rule/comment references in the spec-2.3 touched files. Existing FROZEN-era Chinese comments were left out of scope, correctly.

Validation:

  • PR feat(skills): spec-2.3 skill invocation #95 required checks are green and merge state is CLEAN.
  • Targeted local tests passed: go test ./internal/platform/logger ./internal/app/diagnose ./internal/app/skills/... ./internal/bootstrap ./tests/integration/skillinvoke.
  • Core spec-2.3 contracts remain sound: paired scope-deny tool result, ctx fatal path, Skill retained as scope-control tool, same-turn ToolFilter, and dedup replay preserving ToolFilter.

This closes codex Route 3 for spec-2.3. Ready for merge/FROZEN/tag sequence.

@sqlrush sqlrush merged commit 0cf1005 into main Jun 6, 2026
12 checks passed
@sqlrush sqlrush deleted the spec-2.3-skill-invocation branch June 6, 2026 03:47
sqlrush added a commit that referenced this pull request Jun 6, 2026
Why: PR #95 (spec-2.3 impl) omitted the git-hooks/spec-registry.txt
fallback-copy row that prior spec PRs carried (e.g. #93 for 2.2). The
pre-push hook prefers the sibling opendbrb registry (already updated @
opendbrb 5bb091e) so tagging was unblocked; this restores the fallback
copy's sync. Tag v0.53.0-stage2.53 already pushed dual-repo.

Spec: spec-2.3-skill-invocation.md

Co-authored-by: sqlrush <sqlrush@sqlrushdeMacBook-Pro.local>
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