fix: make skills ask focused questions - #201
Conversation
Astro-Han
added
enhancement
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
🧰 Additional context used📓 Path-based instructions (2)packages/opencode/**/*.ts📄 CodeRabbit inference engine (packages/opencode/AGENTS.md)
Files:
packages/opencode/test/**/*.test.{ts,tsx}📄 CodeRabbit inference engine (packages/opencode/test/AGENTS.md)
Files:
🧠 Learnings (4)📚 Learning: 2026-04-22T08:49:44.563ZApplied to files:
📚 Learning: 2026-04-20T14:36:31.017ZApplied to files:
📚 Learning: 2026-04-20T14:36:04.099ZApplied to files:
📚 Learning: 2026-04-23T08:51:00.819ZApplied to files:
🔇 Additional comments (8)
📝 WalkthroughWalkthroughThis PR updates three productivity skill docs to require structured clarification via a Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant SkillGate as Skill Gate
participant QTool as Question Tool
participant SkillMain as Skill Main Logic
User->>SkillGate: Invoke skill with request
SkillGate->>SkillGate: Evaluate context & Decision use / Success check
alt Mandatory clarification needed
SkillGate->>QTool: Ask 2–4 focused questions
QTool->>User: Present questions
User->>QTool: Provide answers
QTool->>SkillGate: Return clarified context
else Use safe defaults or single-choice prompt
SkillGate->>SkillGate: Apply defaults or ask MCQ
end
SkillGate->>SkillMain: Proceed with clarified context
SkillMain->>User: Deliver output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the clarification guidelines for the Data Analysis, Document Processing, and Writing Assistant skills to standardize the use of the question tool. It introduces a 2-4 question guideline for clarification rounds, provides decision rules for when to ask questions versus using defaults, and updates the test suite to reflect these changes. The review feedback suggests relaxing the rigid question count to typically 2-4 to avoid redundancy and refining the writing assistant's logic to ensure it still checks for additional user details even when the initial input seems sufficient.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/test/skill/skill.test.ts`:
- Around line 502-513: The test strengthens shared assertions for skill docs but
misses checks for the new clarification anchors; update the assertions that
inspect the generated content (the same block that currently checks for
"`question` tool", "2-4 questions", etc.) to also assert content contains the
new anchors "**Success check**" and "**Decision use**" (match the exact bolded
phrases) so the contract test will fail if those prompt sections are removed or
renamed.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 54aacd96-cee6-4394-8ea7-676b65e37ec1
📒 Files selected for processing (4)
packages/opencode/test/skill/skill.test.tsskills/data-analysis/SKILL.mdskills/document-processing/SKILL.mdskills/writing-assistant/SKILL.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: unit-windows-app
- GitHub Check: unit-opencode
- GitHub Check: unit-windows-opencode-session
- GitHub Check: unit-windows-desktop
- GitHub Check: unit-windows-opencode-config-project
- GitHub Check: unit-windows-opencode-server-tools
- GitHub Check: smoke-macos-arm64
- GitHub Check: e2e-artifacts
- GitHub Check: analyze-js-ts
🧰 Additional context used
📓 Path-based instructions (2)
packages/opencode/**/*.ts
📄 CodeRabbit inference engine (packages/opencode/AGENTS.md)
packages/opencode/**/*.ts: UseEffect.gen(function* () { ... })for Effect composition
UseEffect.fn("Domain.method")for named/traced effects andEffect.fnUntracedfor internal helpers; these accept pipeable operators as extra arguments to avoid unnecessary outer.pipe()wrappers
UseEffect.callbackfor callback-based APIs
PreferDateTime.nowAsDateovernew Date(yield* Clock.currentTimeMillis)when you need aDatein Effect code
UseSchema.Classfor multi-field data in Effect schemas
Use branded schemas (Schema.brand) for single-value types in Effect
UseSchema.TaggedErrorClassfor typed errors in Effect schemas
UseSchema.Defectinstead ofunknownfor defect-like causes in Effect code
InEffect.gen/Effect.fn, preferyield* new MyError(...)overyield* Effect.fail(new MyError(...))for direct early-failure branches
UsemakeRuntimefromsrc/effect/run-service.tsfor all services; it returns{ runPromise, runFork, runCallback }backed by a sharedmemoMapthat deduplicates layers
UseInstanceStatefromsrc/effect/instance-state.tsfor per-directory or per-project state that needs per-instance cleanup; do work directly in theInstanceState.makeclosure whereScopedCachehandles run-once semantics
UseEffect.addFinalizerorEffect.acquireReleaseinside theInstanceState.makeclosure for cleanup (subscriptions, process teardown, etc.)
UseEffect.forkScopedinside theInstanceState.makeclosure for background stream consumers — the fiber is interrupted when the instance is disposed
PreferFileSystem.FileSysteminstead of rawfs/promisesfor effectful file I/O in Effect services
PreferChildProcessSpawner.ChildProcessSpawnerwithChildProcess.make(...)instead of custom process wrappers in Effect services
PreferHttpClient.HttpClientinstead of rawfetchin Effect services
PreferPath.Path,Config,Clock, andDateTimeservices when those concerns are already inside Effect code
For backgroun...
Files:
packages/opencode/test/skill/skill.test.ts
packages/opencode/test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (packages/opencode/test/AGENTS.md)
packages/opencode/test/**/*.test.{ts,tsx}: Use thetmpdirfunction fromfixture/fixture.tsto create temporary directories for tests with automatic cleanup. Useawait usingsyntax to ensure automatic cleanup when the variable goes out of scope.
When using thetmpdirfunction with git repository support, pass thegit: trueoption to initialize a git repo with a root commit.
Use theconfigoption intmpdirto write anopencode.jsonconfig file during test setup by passing a partial Config.Info object.
Use theinitoption intmpdirto define custom setup functions that can return extra data accessible viatmp.extra, and use thedisposeoption for custom cleanup logic.
UsetestEffect(...)fromtest/lib/effect.tsfor tests that exercise Effect services or Effect-based workflows.
Useit.effect(...)when the test should run withTestClockandTestConsole. Useit.live(...)when the test depends on real time, filesystem mtimes, child processes, git, locks, or other live OS behavior.
Prefer Effect-aware helpers fromfixture/fixture.tsover building manual runtimes in tests: usetmpdirScoped()for scoped temp directories,provideInstance(dir)(effect)for low-level binding without directory creation,provideTmpdirInstance(...)for single temp instance binding, orprovideTmpdirServer(...)for tests that also need the test LLM server.
Defineconst it = testEffect(...)near the top of the test file and keep the test body insideEffect.gen(function* () { ... }). Yield services directly withyield* MyService.Serviceoryield* MyTool.
Avoid customManagedRuntime,attach(...), or ad hocrun(...)wrappers in Effect tests whentestEffect(...)already provides the runtime.
When a test needs instance-local state, preferprovideTmpdirInstance(...)orprovideInstance(...)over manualInstance.provide(...)inside Promise-style tests.
Files:
packages/opencode/test/skill/skill.test.ts
🧠 Learnings (3)
📚 Learning: 2026-04-22T08:49:44.563Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/desktop-electron/src/main/index-sidecar-source.test.ts:3-11
Timestamp: 2026-04-22T08:49:44.563Z
Learning: In `packages/desktop-electron/src/main/index-sidecar-source.test.ts` (Astro-Han/pawwork), the test intentionally uses `expect(source).toContain` / `expect(source).not.toContain` string matching against the raw `index.ts` source text as a lightweight sidecar contract guard. The maintainer has explicitly chosen not to introduce an AST parser (e.g., `babel/parser` or acorn) for this purpose. Do not flag these string-based assertions as fragile or suggest converting them to AST-based matching.
Applied to files:
packages/opencode/test/skill/skill.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Define `const it = testEffect(...)` near the top of the test file and keep the test body inside `Effect.gen(function* () { ... })`. Yield services directly with `yield* MyService.Service` or `yield* MyTool`.
Applied to files:
packages/opencode/test/skill/skill.test.ts
📚 Learning: 2026-04-23T08:51:00.819Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 186
File: packages/opencode/test/plugin/workspace-adaptor.test.ts:139-144
Timestamp: 2026-04-23T08:51:00.819Z
Learning: For pawwork tests under packages/opencode/test/**, auth.json teardown may intentionally combine `Filesystem.write` (from `packages/opencode/src/util/filesystem.ts`) with `node:fs/promises` `unlink` for cleanup. Do not flag this as inconsistent style; it is the established/intentional pattern because `Filesystem` does not provide a `remove`/`unlink` helper.
Applied to files:
packages/opencode/test/skill/skill.test.ts
🔇 Additional comments (3)
skills/document-processing/SKILL.md (1)
11-37: Clarify-first gate and decision rule are well-structured.Good improvement: this now explicitly binds clarifications to the
questiontool, limits over-asking, and defines clear ask-vs-default behavior before file operations.skills/writing-assistant/SKILL.md (1)
11-38: Gate constraints are clearer and safer for drafting quality.The updated question protocol, decision rule, and stop condition should materially reduce speculative drafting while keeping momentum.
skills/data-analysis/SKILL.md (1)
11-37: Decision-oriented clarification update looks solid.This improves analysis quality by tying question rounds to decision impact and clear stop/default rules.
ecdbb58 to
b269696
Compare
1 participant
Summary
Update the three bundled productivity skills so clarifying questions are routed through the question tool with focused 2 to 4 question rounds, recommended defaults, multiple-choice guidance, and clearer stop conditions. Update the bundled skill test so it protects this intended behavior instead of blocking question tool references.
Why
The document, data, and writing skills all said to ask clarifying questions first, but they did not bind that behavior to the question tool or give weaker models enough structure to decide when to ask, when to use a default, and when to stop. This made proactive questioning unstable and could also make writing over-ask without collecting structured answers.
Related Issue
Part of #188.
How To Verify
Ran targeted Bun tests for bundled skill loading, question tool execution, and pending question handling from packages/opencode. Also ran git diff check.
bun test --timeout 30000 test/skill/skill.test.ts test/tool/question.test.ts test/question/question.test.ts git diff --checkScreenshots or Recordings
Not applicable. This changes skill instructions and tests only.
Checklist
Summary by CodeRabbit
New Features
Documentation
Tests