qwen-code/integration-tests/skill-hooks-invocation-parity.test.ts
Tianyuan 7e0beb9d18
fix(skills): re-apply a Skill's side effects when a session is resumed (#11280)
* fix(skills): re-apply a Skill's side effects when a session is resumed

A resumed session restored a Skill's instructions without restoring the
state that enforces them: the restore path only rebuilt the dedup sets,
while session hooks and allowedTools allow rules live in per-process
memory. So --continue / --resume came back with the Skill's body in the
model's context and its PreToolUse gate gone, silently.

Each restored Skill now goes through the same shared side-effect helper
both live invocation paths use. Two refusals carry over from those paths,
so a resume never re-arms what a fresh invocation would refuse today: a
Skill disabled since it was invoked, and a conditional (paths:) Skill
whose activation has not fired in this session. Folder trust is re-checked
inside the helper, and both registrations dedup, so the replay cannot
stack duplicates. Bookkeeping stays unconditional, since the body really
is in the restored context.

The paths that still do not restore now say why in the debug log — at
warn when the Skill declares hooks:, matching the existing rule — instead
of falling through silently. A Skill started with /<skill-name> leaves no
tool-call record to key on and is documented as a limitation.

Fixes #11180

* fix(skills): scope a restored Skill's grants to its session and name what resume declines

Review round 1 on #11280.

A skill's `allowedTools` were granted as process-wide session allow rules
that no session transition clears, while its hooks die with the session
they were registered for. Re-applying them on resume made that gap
reachable from a failed `/resume`: the rollback returns the user to the
old session with the abandoned one's grants still live. Grants made for a
skill now carry the session they were made for, and the permission
manager filters on it alongside the existing folder-trust check, so
`/clear` and a session swap drop them the way they drop the skill body.
The dedup contract widens the same way trust gating does.

Restore also declined more cases than it explained, and explained one it
got wrong: a same-session re-invocation records the dedup message rather
than a body, and every such response was reported as an edited SKILL.md —
a steady-state warning pointing at a file that never changed. An already
restored skill is now a no-op, and the mismatch reason is chosen by
whether the recorded output is a skill body at all. A skill hidden from
the model since it was invoked is no longer re-armed: frontmatter is not
part of the recorded body, so that change is invisible to the match.

Also: collect the cached skills once instead of twice per resume, pin the
two `await`s and the mismatch log with tests that red without them, scope
the integration test's send wait to the request carrying its prompt, and
correct the skills doc — resume re-applies both halves, a frontmatter-only
edit is restored, and the cases that log elsewhere or not at all are named.

* fix(skills): single-source the skill availability rule resume re-checks

Review round 2 on #11280.

Resume decided whether to re-arm a restored skill's `allowedTools` and
`hooks:` from its own copy of the model-facing availability rule, which
the availability filter also hand-rolls. Two copies of one rule drift,
and this pair already had: the filter's visibility condition was missing
from the resume copy until last round. Both now ask
`skillModelInvocationBlock`, which returns which condition failed so the
resume path can still name it in the log — the reason that branch exists.
Every condition is read live off `Config` / `SkillManager` rather than
off the snapshots an unsequenced `refreshSkills()` commits, and
`pendingConditionalSkillNames` keeps its own filter, which is this trio
minus the activation check.

The session scoping added last round was pinned only against a stubbed
`PermissionManagerConfig`, leaving the wiring it depends on — that the id
tagged onto the grant is the one `startNewSession` replaces — untested.
A `config.test.ts` case now drives a real `Config`, a real
`PermissionManager` and `applySkillSideEffects` across a session
boundary, and reds if either the scoping or the id passed through
`applySkillSideEffects` is removed.

Both `PreToolUse`-gate integration suites drove copies of one fixture,
already diverged in the place that decides their assertions: only the
resume suite's gate wrote the hit counter, and nothing at the parity call
site said why. The skill, the gate script (counter always written), the
fake-model env stubs, the launch args, the poll helper and the exit
sequence move to `integration-tests/helpers/gated-skill-fixture.ts`. The
two fake models stay separate: parity dispatches on a request counter,
resume on the last user message, because its second session replays the
first one's turns.

* fix(skills): scope the AUTO stash, key resume declines on the record

Round 2 review follow-ups.

R1-1 (Critical): `addSessionAllowRule`'s AUTO-mode stash branch deduped on
`raw` and returned before the live re-point, so a dangerous `allowedTools`
grant stashed in session A kept A's id; leaving AUTO re-attached that same
object and the session filter dropped it for good. It now updates the stashed
entry in place, never pushing it into `sessionRules.allow` while AUTO is on.

R2-3: `activeSessionAllowRules()` was the only reader applying the session
predicate. `getAllowRawStrings()` (the `StartSessionEvent` at a session
boundary) now goes through it, and the AUTO strip candidates go through the
new session-only half — not the full predicate, so a mid-AUTO trust grant
cannot activate an unstripped dangerous rule.

R1-3: `skillModelInvocationBlock` checks `disableModelInvocation` before
`isSkillActive`. Such a skill never enters the activation registry, so
`inactive` is permanently unsatisfiable and masked the cause that explains
the decline.

R2-4: the decline log is deferred to after the history scan and emitted only
for skills nothing later restored, so a stale first pair no longer warns
"your gate is gone" about a gate the same loop arms.

R2-7: `logSkillNotRestored` keys on the record rather than the declaration.
A non-body record (a recorded refusal) logs at debug saying nothing was ever
armed; the warn level now covers `allowedTools` as well as `hooks:` (nine
bundled skills declare the former and none the latter); and each reason names
the route that works — `/<skill-name>` for inactive and hidden, `/skills`
first for disabled.

R1-5, R2-1, R2-6: the docs bullet, the `sessionId` JSDoc and the
`SkillCommandLoader` assertion comment now describe what the code does — a
recorded-response mismatch covers a moved skill directory as well as an
edited body, no in-tree caller produces an unscoped session grant, and the
"Exactly `{ trustGated: true }`" pin covers both keys.

R2-2, R2-8: the shared fixture derives `SKILL_DESCRIPTION` from its prefix,
names each generated on-disk path once, is imported by the parity suite
instead of re-spelling `gated-skill`, and emits the description as a quoted
YAML scalar — a bare one with a colon-space flattens the `hooks:` block into
`{"PreToolUse":"","- matcher":"Shell"}`, which `parseHooksConfig` discards,
leaving the gate silently absent.

* fix(skills): hold the AUTO and trust invariants at the read side on resume

Round-3 review findings on #11280, fixed at the shared cause rather than
per instance.

`sessionRules.allow` keeps a previous session's grants as inert entries and
nothing ever removes one, so every path that reads the array had to
re-derive "is this rule mine?" — and each derived it differently:

- The AUTO strip takes candidates from the session-scoped half, so a grant
  tagged with another session was neither stripped nor stashed, and the
  strip's latch never re-scans. Re-entering that session id under a latched
  AUTO made the rule live again with the classifier bypassed. Enforce the
  AUTO half in `activeSessionAllowRules()`, the predicate `evaluate()`
  decides on, where session provenance cannot defeat it.
- `restoreDangerousRules()` re-attached the stash by concatenation, seating
  two entries with the same `raw` once a cycle spanned a session swap. Merge
  by `raw`, keeping the stashed object so the next strip can still remove it
  by identity.
- The live dedup re-pointed a kept entry across a session boundary while
  keeping the dead session's `trustGated`, so a previous session's ungated
  grant de-gated a later project skill's repository-controlled one and the
  folder-trust gate failed open. Take the arrival's gating on a cross-scope
  re-point, widen only within one scope, and route the AUTO stash branch
  through the same rule instead of re-pointing the session alone.

On the resume path:

- A skill body the scheduler truncated before recording starts with the
  truncation sentinel, not the body prefix, so it was classified as "not
  this skill's body" and declined at `debug` under a sentence asserting
  nothing was ever armed — false for a record whose `execute()` had already
  applied the side effects. Classify it as the truncated body it is. The
  decline stands: the record can no longer be compared against SKILL.md.
- Declines were keyed last-write-wins, so a later refusal pair overwrote a
  body-shaped decline and discarded the one actionable signal. Rank them.
- The body-mismatch arm hardcoded `reinvoke`, which `validateToolParams`
  refuses whenever a block condition co-occurs — and it does by
  construction for a `paths:` skill. Consult the block for the remedy.
- Remedies named `/<skill-name>` unconditionally, but `user-invocable:
  false` drops the command from every dispatch accessor. Branch on it.

Also corrects the in-code rationale for the warn/debug split: `warn` and
`debug` share one sink gated only on `QWEN_DEBUG_LOG_FILE`, so the level
buys grep and triage inside the debug log, not operator visibility.

Every behavioural fix is pinned by a test verified to red when the fix is
removed.

* fix(skills): read unmatched resume records as uncorroborated bodies

Addresses the round-4 review of the resume re-arming path.

- An unmatched recorded response is now reported as a declined body
  (warn, with a route) unless it is one of the strings execute() returns
  in place of a body. The persisted-output stub, the budget-exhausted
  stub and the spill-failure fallback no longer land in the arm that
  claims nothing was armed; none of them is re-armed.
- The restore lookup keys on the exact skill name, as the live path
  does, so two skills whose names differ only by case no longer collapse
  into one entry.
- A declined project skill in an untrusted folder names folder trust as
  the precondition, and a disabled user-invocable: false skill is no
  longer sent to /skills.
- Pin the stash path's cross-session trust narrowing and the merge's
  object replacement in permission-manager tests, and correct the merge
  comment's stated reason.

* refactor(skills): simplify resume re-arming

- Drop session-scoped allow rules. Config.startNewSession now clears
  session allow rules (including the AUTO stash) when the session
  changes, and a resumed session re-arms its own skills during
  initialize(). This removes the sessionId tag and its AUTO-mode merge
  and trust re-pointing logic.
- Replace the decline classification, remedy text and warn/debug ranking
  with one factual warn line for a skipped skill that declares hooks or
  allowedTools.
- Trim comments and the docs bullet.

* fix(skills): do not re-arm Skills recorded inside exec output

An exec script can print the nested-results line itself, so it now restores
the Skill body only. The docs now state which skips log and that the match
covers the body only.

---------

Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
2026-09-14 01:02:42 +00:00

170 lines
5.6 KiB
TypeScript

/**
* @license
* Copyright 2026 Qwen Team
* SPDX-License-Identifier: Apache-2.0
*/
/**
* A project skill's frontmatter `hooks:` must fire regardless of who invoked
* the skill. Regression for #11067, where the `/<skill-name>` slash-command
* path injected the skill body and granted its allowedTools but never
* registered its hooks — so a `PreToolUse` gate silently failed open exactly
* when the user started the skill by hand.
*/
import { describe, it, expect, afterEach } from 'vitest';
import { fakeServerHostOptions, TestRig } from './test-helper.js';
import { fakeToolCall, startFakeOpenAIServer } from './fake-openai-server.js';
import type { FakeOpenAIServer } from './fake-openai-server.js';
import {
EXECUTED_FLAG,
GATE_MARKER,
SKILL_DESCRIPTION_PREFIX,
SKILL_NAME,
exitInteractive,
fakeModelLaunchArgs,
installGatedSkill,
makeWaitFor,
stubFakeModelEnv,
} from './helpers/gated-skill-fixture.js';
import { join } from 'node:path';
import { existsSync } from 'node:fs';
describe('skill hooks fire on both invocation paths', () => {
let rig: TestRig;
let fakeServer: FakeOpenAIServer;
let restoreNoProxy: (() => void) | undefined;
afterEach(async () => {
restoreNoProxy?.();
restoreNoProxy = undefined;
await fakeServer?.close();
await rig?.cleanup();
});
/**
* Drives one interactive session. `modelInvokesSkill` controls whether the
* model calls the Skill tool itself (control) or the user typed the slash
* command first (repro).
*/
async function driveSession(options: {
testName: string;
modelInvokesSkill: boolean;
}): Promise<{ output: string; executed: boolean }> {
rig = new TestRig();
await rig.setup(options.testName);
installGatedSkill(rig.testDir!);
let streamingRequestIndex = 0;
fakeServer = await startFakeOpenAIServer(({ body }) => {
if (body['stream'] !== true) {
return { content: '{"selected_memories":[]}' };
}
const i = streamingRequestIndex++;
if (options.modelInvokesSkill && i === 0) {
return {
toolCalls: [
fakeToolCall('skill', { skill: SKILL_NAME }, 'call-skill'),
],
};
}
const shellTurn = options.modelInvokesSkill ? 1 : 0;
if (i === shellTurn) {
return {
toolCalls: [
fakeToolCall(
'run_shell_command',
{ command: `touch ${EXECUTED_FLAG}` },
'call-shell',
),
],
};
}
return { content: 'done' };
}, fakeServerHostOptions());
restoreNoProxy = stubFakeModelEnv(rig, fakeServer);
const { ptyProcess } = rig.runInteractive(
...fakeModelLaunchArgs(fakeServer),
);
let output = '';
ptyProcess.onData((d) => {
output += d;
});
const ready = await rig.waitForText('Type your message', 30000);
expect(ready, `CLI did not start. Output:\n${output}`).toBe(true);
const waitFor = makeWaitFor(rig, () => output);
if (!options.modelInvokesSkill) {
// USER path: start the skill by hand. Wait for the command to exist,
// not merely for the typed text to come back: the echo lands long
// before the skill command registry does, and submitting into that gap
// gets `Unknown command: /<skill>` instead of the skill. That failure
// is undetectable downstream — the error text contains the typed
// command, so any inclusion check on it matches the error too. The
// completion menu rendering the skill's own description is a signal
// only a registered command can produce.
ptyProcess.write(`/${SKILL_NAME}`);
await waitFor('the skill command to be registered', () =>
output.includes(SKILL_DESCRIPTION_PREFIX),
);
// The body is submitted as a prompt rather than echoed, so the
// observable signal is the model call it produces.
const before = fakeServer.requests.length;
ptyProcess.write('\r');
await waitFor(
'the skill body to reach the model',
() => fakeServer.requests.length > before,
);
// Guards the wait above rather than the CLI: the command was never
// submitted before it existed, so this string must never appear.
expect(
output,
'the slash command was submitted before it was registered',
).not.toContain(`Unknown command: /${SKILL_NAME}`);
}
const prompt = 'run the downstream command';
ptyProcess.write(prompt);
await waitFor('the prompt to echo', () => output.includes(prompt));
ptyProcess.write('\r');
const flagPath = join(rig.testDir!, EXECUTED_FLAG);
await waitFor(
'the shell call to be gated or to run',
() => existsSync(flagPath) || output.includes(GATE_MARKER),
);
await exitInteractive(ptyProcess, waitFor, () => output);
return { output, executed: existsSync(flagPath) };
}
it('model-invoked skill: the gate fires and blocks the shell call', async () => {
const { output, executed } = await driveSession({
testName: 'skill hooks model invoked',
modelInvokesSkill: true,
});
expect(
output,
`gate did not fire. Output tail:\n${output.slice(-1500)}`,
).toContain(GATE_MARKER);
expect(executed).toBe(false);
}, 120000);
it('user-invoked skill (/<skill-name>): the gate fires and blocks the shell call', async () => {
const { output, executed } = await driveSession({
testName: 'skill hooks slash invoked',
modelInvokesSkill: false,
});
expect(
output,
`gate did not fire. Output tail:\n${output.slice(-1500)}`,
).toContain(GATE_MARKER);
expect(executed).toBe(false);
}, 120000);
});