fix(wizard): report skill-install failures accurately, and report the ones we were missing - #1073
Draft
gewenyu99 wants to merge 5 commits into
Draft
fix(wizard): report skill-install failures accurately, and report the ones we were missing#1073gewenyu99 wants to merge 5 commits into
gewenyu99 wants to merge 5 commits into
Conversation
The orchestrator fetches the full skill menu once at run start and keeps it in `menuSkillEntries`, but `installSkillById` re-fetched `skill-menu.json` from GitHub for every single skill it installed. A run installs a skill per task, so one run made a double-digit number of redundant menu requests, and any one of them hitting a transient GitHub blip killed the task — and with it the rest of the drain, because a task that cannot load its instructions must not run blind. `menu-fetch-failed` was the single most common skill-install failure. Every occurrence in the orchestrator path was a refetch of data already in memory. - `SkillInstallOptions` gains `menuEntries`; when supplied, `installSkillById` skips the menu round trip entirely. The orchestrator passes the entries it already has at both call sites, so `menu-fetch-failed` is now unreachable there and only a real per-asset download can fail. - The orchestrator's thrown error hardcoded "If this is a permissions error, check that the project directory is writable" for every failure kind — wrong and misleading for a menu fetch, which never touches the project directory. The accurate per-kind vocabulary already existed in `abortOnInstallFailure` for the linear path; it moves to a shared `describeInstallFailure` so both paths report the same true cause, including the underlying error text for a failed download. - Widen the fetch retry defaults from 3 attempts over ~1.5s to 4 over ~7s. The old window expired inside a typical GitHub release-download blip. Generated-By: PostHog Code Task-Id: 7ffe8598-04c1-41c6-ba1d-9faab4656e8b
🧙 Wizard CIRun 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:
Test all apps in a directory:
Test an individual app:
Show more apps
Results will be posted here when complete. |
…closed silently The dominant real-world skill install failure is not a download — it is the scanner deleting a first-party skill it flagged as a prompt-injection attack. Those failures far outnumber every network cause put together. `triageFilter` fails closed in two places: no provider, and the triage call throwing. Failing closed is correct, but both paths only wrote to the local log, so an enforced match looked identical in telemetry whether triage had confirmed it or never ran at all. `yara triage overruled` can only fire when triage worked, so a rule's real false-positive rate is unmeasurable by construction — and gateway auth failures are common enough that some share of enforcement is very likely running blind. - Emit `yara triage unavailable` on both fail-closed paths, carrying the rule metadata, the scan context, whether the cause was `no-provider` or `error`, and the error's class. Deliberately not the error message or any scanned content, matching the discipline already applied to the triage reason. - `scanInstalledSkill` marks its reason string when the deciding match carries no triage verdict, so a skill deleted because the gateway was unreachable stops being reported to the user, and to error tracking, as a confirmed attack. No change to enforcement: a flagged skill is still deleted and the install still fails. This only makes the difference between "triage confirmed it" and "triage never ran" visible, which is the prerequisite for tuning the rule in warlock. Generated-By: PostHog Code Task-Id: 7ffe8598-04c1-41c6-ba1d-9faab4656e8b
… gaps Drops the menu-reuse plumbing and the retry-default widening from this branch. Both were guesses at a fix aimed at the download path, which the data does not support as the main problem — and neither was measurable before or after. What stays is the part that makes the failures legible, plus the reporting holes that made them hard to read in the first place: - `installSkillById` now reports `skill install failed` for `menu-fetch-failed` and `skill-not-found`. Both resolve before `downloadSkill` is called, and `downloadSkill` owned the only capture, so neither kind produced an event at all — a menu that would not load was indistinguishable in analytics from a skill nobody tried to install. - Rename the captured `step` to `install_step`. A numeric `step` is already defined project-wide; the collision types this one numeric too, so every `download` / `extract` / `scan` value reads back null and the failing stage is invisible in queries. The values were being sent correctly the whole time, only the name was unusable. Generated-By: PostHog Code Task-Id: 7ffe8598-04c1-41c6-ba1d-9faab4656e8b
gewenyu99
changed the title
`reportTriageUnavailable` took `ScanMatch[]`. A `ScanMatch` carries `matchedStrings` — the text that tripped the rule, lifted verbatim out of whatever was scanned, which is the user's source on the Write/Edit path, their Bash commands on the PreToolUse path, and their secrets whenever a `hardcoded_secret` or `posthog_pii` rule is what matched. The emitted payload picked fields individually, so nothing leaked in practice. But handing a content-carrying object to a function whose whole job is to call `wizardCapture` is the wrong shape: it puts the user's code one `...m` away from being shipped, in a file where the same mistake has already had to be guarded against by hand twice for the free-text triage reason. Project first instead. `ruleIdentity()` reduces a match to rule, severity, and category, and telemetry takes `RuleIdentity` — so the content is gone before analytics is in scope and there is no field left to spread. Also fixes a signal bug found while tracing the call sites: PreToolUse Bash passes no provider deliberately, because a flagged command is blocked whatever triage would say, so the LLM call would be wasted. The untriaged report fired there too, meaning every blocked Bash command would have been counted as a triage outage — swamping the gateway failures the event exists to surface. Callers now state whether triage was expected, and only `expected` reports. Tests pin both: the payload has exactly the six rule-identity keys and contains none of the matched content even when warlock hands back a match carrying it, and a blocked Bash command emits nothing while still returning `block`. Generated-By: PostHog Code Task-Id: 7ffe8598-04c1-41c6-ba1d-9faab4656e8b
The rationale lives in the commit messages and the PR description; it did not need repeating as 8-12 line blocks above every new function and type. Generated-By: PostHog Code Task-Id: 7ffe8598-04c1-41c6-ba1d-9faab4656e8b
Collaborator
Author
|
@sarahxsanders For you to look at during your Warlock sprint. I'm not super sure this makes sense tbh, but do take a looksies for what bit of the analytics is useful. It does seem we're still killing some first party skills and we don't have the fullest docs around this |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Over the last 7 days, external prod orchestrator runs: 2,658 total, 58 genuinely incomplete (2.2%), 164 stranded steps.
Skill installs are a small slice of that — 21 install failures across ~12 runs, and only 4 of the 35 partial drains involve one at all. At those volumes nothing is diagnosable unless each failure is attributed correctly, and three of the reporting paths were not.
1. The error blamed the wrong thing. The orchestrator hardcoded "If this is a permissions error, check that the project directory is writable" onto every failure kind, so users hitting a network failure were sent to check
chmod. A menu fetch never touches the project directory. The accurate per-kind wording already existed inabortOnInstallFailure— but only the linear path used it.2. Two failure kinds emitted no event at all.
menu-fetch-failedandskill-not-foundboth resolve insideinstallSkillByIdbeforedownloadSkillis called, anddownloadSkillowned the onlyskill install failedcapture. So a menu that wouldn't load looked, in analytics, exactly like a skill nobody attempted.3. The failing stage was invisible. The capture sends
step, but a numericstepis already defined project-wide, and the collision types this one numeric too — so everydownload/extract/scanvalue reads back null. The values were being sent correctly all along; the property name was just unusable.Recovering the stage via
JSONExtractStringis what makes the last week readable, and it does not look like the 30-day total:downloadscanextractThe 30-day
scancolumn is one bounded incident (Jul 24–28) that is already over — zero since Aug 5. The 30-dayextractcolumn is a single day (Jul 30). Reading the 30-day total as a standing problem points at the scanner; reading the last week points at downloads, and at no single dominant cause: GitHub 5xx (6 events / 3 runs), local filesystem —ENOTDIR,ENOSPC,EPERMonC:\Windows\System32(5 / 5), fetch timeouts (4 / 2), scanner (6 / 2).4. Enforcement can run blind, invisibly.
triageFilterfails closed in two places: no provider, and the triage call throwing. Failing closed is correct. But both paths only wrote to the local log, so an enforced match is indistinguishable from one triage actually confirmed.yara triage overruledcan only fire when triage worked, which makes any rule's true false-positive rate unmeasurable by construction — including during a spike like Jul 24–28.Changes
No behaviour changes to scanning or fetching. A flagged skill is still deleted, the install still fails, retries are untouched.
describeInstallFailureas the one place that turns anInstallSkillResultinto user-facing text, used by both the orchestrator and the linear path — including the underlying download error, which the orchestrator previously discarded.skill install failedformenu-fetch-failedandskill-not-found, so every install failure kind is countable in one place.steptoinstall_step, escaping the numeric collision so the failing stage is queryable without reaching forJSONExtractString.yara triage unavailablewhen a match is enforced without ever being triaged, carrying rule identity, scan context,no-providervserror, and the error's class.scanInstalledSkillmarks its reason string when the deciding match carries no triage verdict, so a skill deleted because the gateway was unreachable stops being reported to the user, and to error tracking, as a confirmed attack.Keeping scanned content out of telemetry
A
ScanMatchcarriesmatchedStrings— the text that tripped the rule, lifted verbatim from whatever was scanned: the user's source on the Write/Edit path, their Bash commands on PreToolUse, their secrets whenever ahardcoded_secretorposthog_piirule is what matched.The new event never contained any of it, but the first cut passed whole
ScanMatchobjects into the reporting function, which put user code one...maway from shipping — in a file that has already had to hand-guard the same mistake twice for the free-text triage reason. Telemetry now takes aRuleIdentity(rule, severity, category) produced byruleIdentity(), so the content is gone before analytics is in scope and there is no field left to spread.Tracing those call sites also turned up a signal bug: PreToolUse Bash passes no provider on purpose, since a flagged command is blocked whatever triage would say. The untriaged report fired there too, so every blocked Bash command would have counted as a triage outage and buried the real gateway failures. Callers now state whether triage was expected, and only
expectedreports.Test plan
pnpm build && pnpm test— 1758 tests pass.wizard-tools.test.ts:menu-fetch-failedandskill-not-foundeach emit a capture carrying the rightinstall_step;describeInstallFailurenever mentions permissions or writability for a menu fetch, and surfaces the underlying error for a failed download.yara-hooks.test.ts: the payload has exactly the six rule-identity keys and contains none of the matched content even when warlock returns a match carrying it; a blocked Bash command emits nothing while still returningblock; the event fires withreason: 'error'plus the error class when the gateway throws, withreason: 'no-provider'where triage was expected, and stays silent when triage actually ran.Follow-ups, deliberately not here
downloadWithRetrycallsfetchWithRetry, then readsresp.arrayBuffer()outside the retry loop, still bound to the 60s abort signal. A body read that stalls or drops mid-transfer is therefore never retried — which matches theThe operation was aborted due to timeoutfailures, the most recent on Aug 10.prompt_injection_posthog_integration_attackrule lives in warlock. The one sentence that tripped it fordata-warehouse-source-setupis fixed in posthog.com#19364; the rule still treats anyPostHog-<word>compound as a package name.Created with PostHog from a Slack thread