feat: dynamic skill loading with fingerprint filtering and message rescue#105
feat: dynamic skill loading with fingerprint filtering and message rescue#105
Conversation
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| // altimate_change start - side channel for per-turn user message text | ||
| // Follows the same pattern as Fingerprint (module-level cached state, get/set/clear) | ||
| export namespace MessageContext { | ||
| let current: string | undefined | ||
|
|
There was a problem hiding this comment.
Bug: The global singletons for MessageContext and Fingerprint cause a race condition in concurrent sessions, leading to incorrect data being used when one session's data overwrites another's.
Severity: MEDIUM
Suggested Fix
Refactor MessageContext and Fingerprint to use a session-scoped or directory-scoped state management system, such as Instance.state(), which is used elsewhere in the codebase. This will ensure that context and fingerprint data are isolated per session, preventing race conditions.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/opencode/src/altimate/context/message-context.ts#L1-L5
Potential issue: The `MessageContext` and `Fingerprint` modules both use a module-level
singleton pattern for caching data (`MessageContext.set`/`get` and a `cached` variable
in `Fingerprint`). In a multi-session environment, when two sessions execute
concurrently, one session can overwrite the cached data of another during an `await`
operation. This race condition leads to sessions processing incorrect data, such as
using the wrong user message for skill selection or the wrong project fingerprint. This
behavior only occurs when the `experimental.dynamic_skills` feature flag is enabled.
Did we get this right? 👍 / 👎 to inform future reviews.
- Environment fingerprint partitions skills into included/excluded pools - Per-turn message rescue: excluded skills rescued when user message contains matching tag words (set intersection) - `MessageContext` side channel passes latest user message text - Config-gated via `experimental.dynamic_skills` (off by default) - Add 39 official skills from dbt-labs, Astronomer, Databricks, Snowflake - Tag all 50 skills with user-facing terms (dbt, airflow, snowflake, etc.)
kulvirgit
force-pushed
the
feat/dynamic-skill-loading
branch
from
624a4b1 to
3788010
Compare
| if (!skill) { | ||
| const available = await Skill.all().then((x) => Object.keys(x).join(", ")) | ||
| const available = await Skill.all().then((s) => s.map((x) => x.name).join(", ")) | ||
| throw new Error(`Skill "${params.name}" not found. Available skills: ${available || "none"}`) | ||
| } |
There was a problem hiding this comment.
Bug: The SkillTool's execute function bypasses dynamic skill filtering by looking up skills from the complete unfiltered list, allowing execution of disallowed skills.
Severity: MEDIUM
Suggested Fix
The execute function should validate that the requested skill params.name is present in the allAllowed list that is generated during the tool's initialization. This will enforce the filtering at execution time, not just at the display layer. The error message should also list skills from allAllowed instead of Skill.all().
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/opencode/src/tool/skill.ts#L95-L98
Potential issue: In `packages/opencode/src/tool/skill.ts`, the `SkillTool` filters
skills based on project context (`dynamic_skills`) to generate a list of allowed skills
(`displaySkills`) for the LLM. However, the `execute` function at line 95 retrieves the
skill using `Skill.get(params.name)`, which queries the complete, unfiltered list of all
skills. This bypasses the intended filtering, allowing an LLM to execute a skill that
was meant to be unavailable if it knows its name (e.g., from prior context or the error
message). This undermines the logic of the dynamic skill partitioning feature.
|
This pull request has been automatically closed because it was not updated to meet our contributing guidelines within the 2-hour window. Feel free to open a new pull request that follows our guidelines. |
1 participant
Summary
MessageContextside channel — passes latest user message text per-turn for rescue matchingexperimental.dynamic_skillsconfig flag, off by default (zero behavioral change without opt-in)Files changed
src/altimate/fingerprint/index.tssrc/altimate/context/message-context.tssrc/config/config.tsexperimental.dynamic_skillsbooleansrc/session/prompt.tssrc/skill/skill.tstagsfield to skill schema, remove unusedtriggerfieldsrc/tool/skill.tsfilterByFingerprintwithpartitionByFingerprint+rescueByMessagetest/altimate/fingerprint.test.tstest/altimate/skill-filtering.test.tsTest plan
bun test ./test/altimate/— 47 tests passbunx tsc --noEmit— no new type errorsdynamic_skills: true, verify dbt skills shown and react skills hidden