Skip to content

fix(skills): attribute a scanner failure to the scan step, not extract - #1280

Merged
edwinyjlim merged 1 commit into
mainfrom
posthog/skill-scan-step-attribution
Sep 22, 2026
Merged

edwinyjlim merged 1 commit into
mainfrom
posthog/skill-scan-step-attribution

Conversation

@Gilbert09

Copy link
Copy Markdown
Member

Problem

downloadSkill runs the YARA-X scan as the last thing inside its try, but
leaves step set to extract while it does. The scan is the only WASM in that
block — unzipSync is fflate, pure JavaScript — so when the engine fails to
instantiate, the failure is filed on skill install failed as an extraction
failure that extraction cannot produce.

Engine-load failures are currently the largest single class of skill-install
failure, all on macOS, and they take the whole run with them: the seeded
data-source step's own skill cannot install, so the task agent is failed rather
than run blind, retried once, and ends without reporting. Every no-report
orchestrator task failure in the window is this, and the exception the run
records suggests checking directory permissions.

The scan value the poisoned-skill branch already sends is unaffected, so a
skill blocked by a rule keeps counting exactly as it did.

Why: the seeded data-source task's telemetry showed a small group of runs
ending in seconds with nothing done. The failing phase was recorded as
extraction, which is why the scanner never came up as the cause.

Changes

  • Widen step to include scan and set it before scanInstalledSkill, so a
    throw from the scanner is attributed to the scan.

Note #1073 renames this property
to install_step to escape a numeric collision; the two are independent and
that PR does not change which value a scanner failure reports. Until it lands,
read the value through JSONExtractString.

Test plan

  • New unit test drives the real downloadSkill over the existing mock HTTP
    server with the warlock mock rejecting, and asserts the captured step. It
    fails on main with extract and passes here.
  • src/lib/__tests__ — 529 tests pass.
  • ESLint clean on both touched files (pre-existing warnings only), Prettier
    clean, tsc --noEmit error count unchanged from main (37, none in the
    touched files).

LLM context

Considered and rejected: making the scanner fail open on an engine error. The
fail-closed behaviour is deliberate and consistent across every scan surface,
and relaxing a security control is not a reporting fix. Also left alone: the
task-level error text that blames permissions, which
#1073 already rewrites per
failure kind.


Created with PostHog Desktop

🤖 Generated with Claude Code

The skill install scan runs the YARA-X WASM engine, but `step` was still
`extract` when it ran, so an engine that fails to instantiate is reported
as an unzip failure — which the pure-JS unzip cannot produce.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: 29067a32-1e8b-4807-aac5-3c13e0497121
@Gilbert09
Gilbert09 requested a review from a team as a code owner September 20, 2026 12:01
Copilot AI lite review requested due to automatic review settings September 20, 2026 12:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown

🧙 Wizard CI

Run the Wizard CI and test your changes against wizard-workbench example apps by replying with a GitHub comment using one of the following commands:

Test all apps:

  • /wizard-ci all

Test all apps in a directory:

  • /wizard-ci ai-observability
  • /wizard-ci basic-integration
  • /wizard-ci mcp-analytics
  • /wizard-ci replay-vision
  • /wizard-ci revenue
  • /wizard-ci self-driving
  • /wizard-ci warehouse
  • /wizard-ci warehouse-seeded

Test an individual app:

  • /wizard-ci ai-observability/anthropic
  • /wizard-ci ai-observability/google-adk
  • /wizard-ci ai-observability/groq
Show more apps
  • /wizard-ci ai-observability/manual-capture
  • /wizard-ci ai-observability/openai
  • /wizard-ci ai-observability/openai-agents
  • /wizard-ci ai-observability/opentelemetry
  • /wizard-ci ai-observability/vercel-ai
  • /wizard-ci basic-integration/android
  • /wizard-ci basic-integration/angular
  • /wizard-ci basic-integration/astro
  • /wizard-ci basic-integration/django
  • /wizard-ci basic-integration/fastapi
  • /wizard-ci basic-integration/flask
  • /wizard-ci basic-integration/flutter
  • /wizard-ci basic-integration/javascript-node
  • /wizard-ci basic-integration/javascript-web
  • /wizard-ci basic-integration/laravel
  • /wizard-ci basic-integration/next-js
  • /wizard-ci basic-integration/nuxt
  • /wizard-ci basic-integration/python
  • /wizard-ci basic-integration/rails
  • /wizard-ci basic-integration/react-native
  • /wizard-ci basic-integration/react-router
  • /wizard-ci basic-integration/sveltekit
  • /wizard-ci basic-integration/swift
  • /wizard-ci basic-integration/tanstack-router
  • /wizard-ci basic-integration/tanstack-start
  • /wizard-ci basic-integration/vue
  • /wizard-ci mcp-analytics/custom-dispatcher
  • /wizard-ci mcp-analytics/typescript-sdk
  • /wizard-ci replay-vision/javascript-node
  • /wizard-ci replay-vision/next-js
  • /wizard-ci replay-vision/react-vite
  • /wizard-ci revenue/stripe
  • /wizard-ci self-driving/astro
  • /wizard-ci self-driving/fastapi
  • /wizard-ci self-driving/nuxt
  • /wizard-ci self-driving/react-router
  • /wizard-ci self-driving/sveltekit
  • /wizard-ci warehouse/monorepo-env
  • /wizard-ci warehouse/multi-source-next
  • /wizard-ci warehouse/stripe-node
  • /wizard-ci warehouse/zero-source
  • /wizard-ci warehouse-seeded/next-stripe
  • /wizard-ci warehouse-seeded/next-stripe-declined

Test against a Context Mill branch:

  • /wizard-ci all context-mill:my-branch

Add context-mill:<branch> to any command above to pin the Context Mill branch. It defaults to main.

Results will be posted here when complete.

@Gilbert09 Gilbert09 added the stamphog Auto-merge stamp for automated PRs label Sep 20, 2026 — with PostHog
@edwinyjlim
edwinyjlim merged commit 0c72cd2 into main Sep 22, 2026
21 checks passed
@edwinyjlim
edwinyjlim deleted the posthog/skill-scan-step-attribution branch September 22, 2026 17:32
gewenyu99 added a commit that referenced this pull request Sep 23, 2026
Brings in the eight main fixes since beccc79 (#1275, #1280, #1284, #1294,
#1295, #1301, #1302, #1305). One conflict: agentic detection keeps main's
two-attempt retry loop from #1305 and adds A1's progress emitter to the
agent it initializes.

Generated-By: PostHog Desktop
Task-Id: d14e92bb-6ee1-49b5-8502-39cb80079589
gewenyu99 added a commit that referenced this pull request Sep 23, 2026
Brings the final Release A, through the new B1, into the callable program
host. B2's structure stays; A's agent contract holds throughout:
- asks and task notices take { signal }, and cancelAsk, cancelTaskNotice
  and cancelQuestion are gone;
- the linear sequence owns a run controller;
- the orchestrator's fatal path reports blocked tasks;
- the agent never sends terminal analytics.

The adapter calls wizardAbort with an explicit status: 'cancelled' only when
the host cancelled, 'error' otherwise. After a non-composed success it sends
'success' behind the flush guard.

main's fixes land where B2 moved the code:
- #1284's warehouse seed cap goes into posthog-integration/run.ts;
- #1280's scan step goes into B2's downloadSkill;
- #1305's two detection attempts share one inference-auth provider;
- the #1305 timer and B2's host-abort check sit side by side in
  agent-interface.

B2's skill-cleanup test now expects an agent [ABORT] to end as failed, and
its host-abort test checks that an unmarked user skill survives.

Generated-By: PostHog Desktop
Task-Id: d14e92bb-6ee1-49b5-8502-39cb80079589
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stamphog Auto-merge stamp for automated PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants