[lihongyuan99]

Why

Self-found; no issue.

rollback_custom_skill constructs the user-scoped storage, probes whether the
skill exists and parses custom/.history/<name>.jsonl inline, while
get_custom_skill_history two handlers above already offloads those exact three
steps and documents why (backend/app/gateway/routers/skills.py:597-606):

        def _read_history() -> list[dict] | None:
            # Worker thread: storage construction, the existence probes, and the
            # history-file read are blocking filesystem IO that must stay off the
            # event loop. None signals 404 to the caller.

So this is not a new policy proposal — it is one handler breaking a rule the
adjacent handler states verbatim, and the rest of the same handler obeys it
(append_history and write_custom_skill both go through
asyncio.to_thread).

The cost is not one stat(): read_history reads the whole file and
json.loads every line (skills/storage/local_skill_storage.py:260-270), and
each history entry carries the full prev_content and new_content of the
skill, so the file grows with every edit. A rollback request therefore parses
that skill's entire edit history on the Gateway event loop, stalling every other
run and stream in the worker for its duration.

The storage construction is blocking too, which the strict gate confirms
directly rather than by reading — see Bug fix verification.

Why the gate never caught it: scripts/detect_blocking_io_static.py reports this
file's existence probe and the post-scan read_text, but cannot see
read_history at all, because its async reachability is same-file only
(ASYNC_REACHABLE_SAME_FILE) and read_history lives in the storage module. The
SOP in .agent/skills/blocking-io-guard calls out exactly this blind spot, and
that cross-file rule is how the call was found.

What changed

• POST /api/skills/custom/{skill_name}/rollback now performs storage
  construction, the existence probes and the history-file read on a worker
  thread, in one closure mirroring get_custom_skill_history.
• Both pre-scan branches are anchored under tests/blocking_io/, so
  make test-blocking-io guards the route from now on.

Status codes and their precedence are unchanged (missing skill → 404, empty
history → 400, out-of-range index → 400), the response shape is unchanged, and no
user-visible default changes, so no README/AGENTS.md text is affected.

Deliberately not included: the post-scan skill_file.read_text(...) in the
same handler, which #5645 already offloads on its own line — see Collision audit.

Surface area

• Frontend UI — page / component / setting / interaction under frontend/
• Backend API — endpoint / SSE event / request-response shape under backend/app
• Agents / LangGraph — agent node, graph wiring, langgraph.json, or prompt change
• Sandbox — docker/ or sandboxed execution
• Skills — change under skills/ (the custom-skill rollback route)
• Dependencies — new/upgraded entry in backend/pyproject.toml or frontend/package.json (say what it buys us)
• Default behavior change — changes existing behavior without the user opting in (default model, default setting, data shape)
• Docs / tests / CI only — no runtime behavior change

Backend API is left unchecked because the request/response shape and every status
code are identical; only the thread the blocking work runs on changes.

Screenshots / Recording

Not applicable — no UI surface.

Bug fix verification

• Test path that reproduces the bug:
  backend/tests/blocking_io/test_skills_custom_router.py
  (test_rollback_missing_skill_does_not_block_event_loop,
  test_rollback_history_read_does_not_block_event_loop)

• Red on main: yes, on 9ae1e58 with only the test file added and
  routers/skills.py untouched:

  blockbuster.blockbuster.BlockingError: Blocking call to os.getcwd
    app/gateway/routers/skills.py:620  rollback_custom_skill
    -> _get_user_skill_storage -> get_or_new_user_skill_storage
    -> UserScopedSkillStorage.__init__ -> LocalSkillStorage.__init__
    -> config.skills.get_skills_path() -> runtime_paths.project_root() -> Path.cwd().resolve()
  2 failed in 2.30s
  

• Green on this branch: yes — 2 passed.

• The anchor drives the real handler and real local filesystem; the only stubbed
  collaborator is require_admin_user, because this is a direct handler call with
  no HTTP layer to run auth through. Both branches return before the awaited
  security scan, so no scanner or model stub is needed.

• Test-side setup IO (_install_skill, _write_history) is itself dispatched via
  asyncio.to_thread, matching test_agents_router, so only production work runs
  on the loop.

Collision audit

Method: all 392 open PRs enumerated with
pulls?state=open&per_page=100&page=1..4, then each PR's file list and the @@
hunk headers of the candidate files compared against the lines this change
touches.

• Six open PRs touch backend/app/gateway/routers/skills.py: feat(skills): add guarded evolution host APIs #5645, feat(skills): install local archives from settings #4739,
  Add App Key control plane #4646, fix(skills): preserve extension config on toggle #3777, feat(skills): per-user custom skill isolation  #3388, chroe(PMC): improvements of MCP and Skills management  #1091. Five of them have no hunk below line 353, and Add App Key control plane #4646
  only edits the body of _get_user_skill_storage (not its signature or call
  sites), so none of them can conflict here.
• The one real neighbour is feat(skills): add guarded evolution host APIs #5645, which has hunks inside
  rollback_custom_skill and already offloads the post-scan
  current_content = skill_file.read_text(...) via
  await asyncio.to_thread(read_optional_text, storage, skill_file). That is why
  this PR stops at the three pre-scan steps: the remaining blocking read in this
  handler is theirs, and this change does not touch that line.
• No open PR adds backend/tests/blocking_io/test_skills_custom_router.py.
• Issue side: no open issue reports this route (the current skill-related reports
  [bug] describe_skill prints "Allowed tools: (all)" for a skill that declared an empty allowed-tools list #5590/[bug] Skill export renders empty allowed-tools and required-secrets as blank #5658/[bug] Assembly fingerprint collapses undeclared and empty Skill allowed-tools #5665 are about describe_skill rendering, export requirements and
  assembly fingerprints, and each already has its own PR).

Validation

Branch fix/rollback-history-offloop @ edb34c2, rebased onto upstream main
9ae1e58. Every command below was run on that combination.

• Anchor, red (production file reverted to 9ae1e58): 2 failed,
  BlockingError: Blocking call to os.getcwd with the frame chain quoted above.
• Anchor, green (this branch): 2 passed.
• Full strict gate, make test-blocking-io equivalent
  (pytest tests/blocking_io -q): 151 passed.
• Behaviour contracts for the touched route
  (pytest tests/test_skills_custom_router.py tests/test_skill_storage_lifecycle.py -q):
  36 passed, including the existing 404 / 400-no-history /
  scanner-blocked-400 and out-of-range cases.
• Step-2 re-scan python3 scripts/detect_blocking_io_static.py: total findings
  63 → 62; routers/skills.py goes from 3 findings (lines 622, 635, 635) to 2
  (the skill_file.read_text / .exists pair at 644, which is feat(skills): add guarded evolution host APIs #5645's line,
  shifted by this diff). read_history never appeared in either listing, per the
  same-file limitation above.
• ruff check . and ruff format --check . over the whole backend: clean
  (1574 files already formatted).
• python3 scripts/check_agent_guidance.py: 31 AGENTS.md, 0 errors, 0 warnings.
• git diff --check: clean.
• Full offline suite (make test equivalent,
  pytest -m "not live" --ignore=tests/blocking_io tests/):
  18161 passed, 205 skipped, 3 deselected, 10 failed in 8:22. Those 10 are
  pre-existing in this environment, not caused by this change: the same 10 node
  ids fail on the untouched base 9ae1e58 (4 in
  tests/test_extension_manager.py, which install real git/requirement sources
  through uv; 3 in tests/test_lark_broker.py, 1 in
  tests/test_openviking_mcp_integration.py and 2 in
  tests/test_parallel_mcp_user_agent.py, which need a live local broker /
  lark-cli shim — test_health_endpoint fails with urllib.error). This branch
  adds no failure beyond those.

AI assistance

Tool(s) used: Qoder CLI (AI coding agent)

How you used it: the agent ran the repository's blocking-IO scanner and the
blocking-io-guard SOP, traced the same-file reachability gap that hides
read_history, drafted the anchor against the strict Blockbuster gate, and
applied the offload. It also produced the collision audit by paging all 392 open
PRs and diffing hunk headers. A human read every line of the resulting diff,
verified the 404/400 precedence claim against the existing route tests, and
reviewed and re-ran the red/green, scan, lint and gate numbers above (all of
which are actual command output on 9ae1e58, not summaries).

• I've read and understand every line of this change and take responsibility for it — it's not unreviewed AI output.

[willem-bd]

Reviewed against head edb34c2. The production change is correct and minimal: I verified that LocalSkillStorage.read_history returns [] (never None) when the history file is missing, so the history is None 404 sentinel cannot collide with the existing 400 has no history path, and status-code precedence (404 missing → 400 empty → 400 out-of-range) is unchanged. SkillStorage is imported at module top so the nested-def annotation resolves, and asyncio.to_thread copies the current context so get_effective_user_id() inside the worker behaves as on the loop — matching the established get_custom_skill_history pattern. Two suggestions and a nit on the new anchor file below.

[willem-bd]

This docstring overclaims: "every other filesystem phase in the same handler already runs through asyncio.to_thread" is not true at this head — rollback_custom_skill still executes current_content = skill_file.read_text(encoding="utf-8") if skill_file.exists() else None inline on the event loop (backend/app/gateway/routers/skills.py:644), which this PR's own collision audit deliberately leaves to #5645. Worth rewording (e.g. "every other pre-scan filesystem phase ...") so a future reader of this anchor doesn't conclude the handler is already fully off-loop and mis-attribute line 644's blocking read to a regression.

[willem-bd]

Consider the sibling pattern instead of stubbing a security predicate: test_skills_router.py in this same directory builds a real starlette Request with an admin in request.state.user (_admin_request()) and lets the real require_admin_user → is_admin_user path run — no DB or extra fixtures needed. Reusing that here would drop the require_admin_user monkeypatch (so the gate also keeps covering anything authz itself might do) and keep the anchor closer to production. Optional, since the PR documents the stub choice.

[willem-bd]

Nit: the monkeypatch parameter is unused — path isolation comes from the autouse _isolate_paths fixture. The parameter can be dropped.