Skip to content

feat: skill/prompt frontmatter now supports model, model-group, and thinking - #20

Open
ofriw wants to merge 17 commits into
mainfrom
feat/models-frontmatter
Open

feat: skill/prompt frontmatter now supports model, model-group, and thinking#20
ofriw wants to merge 17 commits into
mainfrom
feat/models-frontmatter

Conversation

@ofriw

@ofriw ofriw commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Skill/prompt frontmatter model selection

Skills and prompt templates can now declare model, model-group, and thinking alongside readonly. Interactive invocations apply model selection during input preflight, before slash-command expansion. readonly remains deferred to before_agent_start, where final skill metadata is available.

What changed

  • Generalized the readonly-only cache into frontmatter-cache.ts, which parses and validates readonly, model-group, model, and thinking for skills and prompts.
  • Added input preflight for model selection:
    1. model: provider/model-id selects that configured, authenticated model.
    2. Otherwise model-group: group-name resolves through the existing Model Group router.
    3. thinking: overrides the selected/current model’s level after capability clamping.
  • Explicit model takes precedence over model-group and shows a warning when both are present.
  • Unknown/unusable models or groups visibly block command expansion. Invalid frontmatter shows a warning and leaves valid independent fields usable.
  • Model-selection frontmatter is ignored in headless sessions and blocked during streaming; no model/thinking mutation occurs.
  • Preserved deferred readonly behavior and renamed internal readonly cache state/types to frontmatter cache equivalents.
  • Added integration coverage for prompt/skill preflight, precedence, validation, auth and routing failures, thinking clamping, streaming, headless mode, cache freshness, and state reset.

Documentation

docs/architecture.md now documents the frontmatter fields, resolution order, and lifecycle split.

Verification

  • npm run typecheck
  • npm test (the existing audit-config check currently fails because npm audit reports an additional transitive advisory path; this PR does not change dependencies, the lockfile, or that test)

@ofriw
ofriw requested a review from grzegorznowak August 3, 2026 08:33

@grzegorznowak grzegorznowak left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Requesting changes on four issues, ordered by severity:

  1. High — Block readonly-changing commands during streaming. Pi does not emit a new before_agent_start for queued steer/follow-up messages, so a deferred readonly intent can miss its own command and apply to a later unrelated prompt. Please treat valid readonly: true and readonly: false like model-changing frontmatter during streaming: warn, return handled, enqueue nothing, and leave state unchanged. (index.ts:600, index.ts:607, index.ts:718)

  2. High — Reconcile partial setModel() failures. Pi can commit the new model and clamp thinking before a later model_select hook rejects. The current code then reports failure and blocks expansion even though the new model is active. Please roll back or reconcile the effective state, and cover mutation-then-reject for explicit and group routes. (index.ts:256, index.ts:327, index.ts:372)

  3. Medium — Make the tests match Pi's host lifecycle. Current mocks manually fire before_agent_start after streaming input and throw from setModel() before mutation, masking both issues above. Please add a host-faithful integration seam or equivalent lifecycle/mutation coverage. (tests/unit/model-group-frontmatter.test.ts:380, tests/unit/readonly-frontmatter.test.ts:386, scripts/test-package-host.mjs:36)

  4. Medium — Use Pi's full slash-command token. The extension treats /review/typo as /review, while Pi looks for review/typo. This can change model, thinking, or readonly state for a command Pi never expands. Please align parsing with Pi and add embedded-slash negative tests for prompts and skills. (index.ts:581, index.ts:582)

SUMMARY.md — longer-form explanations

Detailed review summary

1. High: block readonly-changing commands during streaming

Pi intentionally processes steer and follow-up messages inside the current agent run, without emitting another before_agent_start. That is safe for ordinary skill text, but our readonly implementation queues the command during input and waits for before_agent_start to apply its state change.

For a readonly-only command submitted during streaming, the expected hook never runs for that message. The command can therefore run under the old readonly state while its queued intent remains behind and is later consumed by an unrelated prompt.

Example:

readonly is off
→ agent is streaming
→ user queues /research with readonly: true
→ /research continues without readonly being activated
→ user later submits "implement the fix"
→ stale readonly: true is applied to the new prompt

This queue behavior predates the PR, but the PR refactors the shared frontmatter lifecycle and establishes a streaming policy for model-changing fields. Please apply that policy consistently: valid readonly: true and readonly: false frontmatter submitted as either steer or followUp should show a warning, return handled, enqueue nothing, and leave readonly unchanged.

Please test both values under both streaming behaviors and confirm that a subsequent unrelated prompt receives no delayed readonly change.

Sources: index.ts:600, index.ts:607, index.ts:718

2. High: reconcile partial setModel() failures

safeSetModel() assumes that a rejected pi.setModel() means the model was not changed. In Pi 0.82.0, the host commits the model, persists the change, and re-clamps thinking before awaiting model_select hooks. A hook can therefore throw after the requested model is already active.

The current handler then reports a failed switch, blocks command expansion, skips any requested thinking override, and records no frontmatter success entry. The operator sees failure while the session is actually using the new model. This affects both explicit model: and model-group: routes.

Example:

Current model: Claude
/review requests GPT-4o
→ Pi commits GPT-4o and clamps thinking
→ another extension's model_select hook throws
→ user sees "Failed to switch model"
→ /review is blocked
→ actual session model remains GPT-4o

Please either restore the previous model/thinking state or reconcile the effective post-error state and report it accurately. Add a mutation-then-reject test rather than a mock that throws before changing state.

Sources: index.ts:256, index.ts:327, index.ts:372

3. Medium: make the tests match Pi's host lifecycle

The owning tests manually pair streaming input with before_agent_start and use a side-effect-free setModel() mock. Those assumptions are stronger than Pi's real contract and hide both problems above:

  • Pi does not emit before_agent_start for a queued steer/follow-up inside the existing run.
  • Pi can reject setModel() after committing the model and thinking state.

The package-host lane only proves that the packaged extension loads. Please add a small host-faithful integration seam or adjust the lifecycle and mutation mocks to reproduce the locked Pi 0.82.0 behavior.

Sources: tests/unit/model-group-frontmatter.test.ts:380, tests/unit/readonly-frontmatter.test.ts:386, scripts/test-package-host.mjs:36

4. Medium: use Pi's full slash-command token

The extension stops parsing command names at an embedded slash, while Pi uses the complete whitespace-delimited token. For example, the extension treats /review/typo as /review, but Pi looks for a command named review/typo and leaves the input unexpanded if it does not exist.

That means a malformed or different command can still apply /review's model, thinking, or readonly frontmatter even though /review itself never runs. /skill:foo/bar has the same mismatch.

Please use Pi's complete command token and only apply frontmatter when that exact resolved prompt or skill is authoritative. Add negative tests for embedded-slash prompt and skill invocations.

Sources: index.ts:581, index.ts:582

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.

2 participants