Skip to content

fix(managed-skills): unsupported rows on every path; failed discovery is degraded - #1037

Merged
Juliusolsson05 merged 4 commits into
mainfrom
fix/conventions-unsupported-minors
Sep 19, 2026
Merged

Juliusolsson05 merged 4 commits into
mainfrom
fix/conventions-unsupported-minors

Conversation

@Juliusolsson05

Copy link
Copy Markdown
Owner

Follow-up to #1017's internal review (ledger T10).

Why

  1. The unsupported rows vanish. bug(managed-skills): provider declaring personalAgentSkills unsupported blocks every session spawn #1014 made unsupported providers informational: the unsupported:<provider> rows show in Settings instead of blocking. Those rows were appended by only two of the roughly fifteen code paths that set target statuses. The startup audit, a target conflict on enable, a disable, and the custom and installed reconciles all replaced the list without them. So Settings stopped naming the unsupported provider as soon as anything else happened.
  2. A failed discovery reads "unsupported". When provider target discovery fails, resolveTargetsSafely marked conventions and custom skills with the resolution error, but not installed skills. They kept stale rows, and installedHealth checked "zero targets → unsupported" before any error. The skill therefore said "no provider can take this" when the truth was "we could not look".

Change

  • withUnsupportedRows adds the rows when a snapshot is built, derived from this.targets, so every path agrees.
    • It replaces the three near-identical builders (conventions, custom, installed).
    • Health still reads the stored statuses, which never contain these rows.
  • Installed skills get the resolution error row on a discovery failure.
  • installedHealth checks error before "unsupported", as health() and customHealth() already do.
  • Two Grok nits from ledger T6: an import spacing fix, and a doubled word.

Tests (the real service, fail-first)

  • AgentCodeConventionsService.system.test.ts:
    • the first snapshot after startup names unsupported:grok;
    • a target conflict on enable keeps that row beside the conflict row.
  • AgentCodeInstalledSkillsService.system.test.ts: after an install, a discovery failure, then audit(), the skill is degraded with the provider-target-resolution error row.

All three fail on main. The managed-skills system and unit suites pass (128), the settings renderer suites pass (18), and npx tsc -b is clean.

🤖 Generated with Claude Code

Juliusolsson05 and others added 4 commits September 19, 2026 01:10
…led discovery

#1017 review minors, fail-first on the real service:
- the first snapshot after startup must already name the unsupported
  provider, not only after a save;
- a target conflict on enable must keep that row beside the conflict;
- an installed skill whose provider target discovery fails must read
  degraded with the resolution error, not unsupported with stale rows.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… discovery is degraded

From the #1017 internal review (ledger T10):
- The informational unsupported:* rows were appended by two of the
  roughly fifteen paths that set target statuses. The startup audit, a
  target conflict, a disable and the custom/installed reconciles dropped
  them. They are now added when a snapshot is built, from this.targets, by
  one builder that replaces three near-identical ones. Health reads the
  stored statuses, which never contain these rows.
- resolveTargetsSafely invalidated conventions and custom skills on a
  discovery failure, but not installed skills. Those kept stale rows, and
  installedHealth then read unsupported, because it checked the
  zero-targets case before the error. Installed skills now get the error
  row, and installedHealth orders error first, as the other two do.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…bled skill

From the #1037 review (CHANGES REQUESTED):
- MEDIUM: the new provider-target-resolution error row counted as a
  delete blocker. It has no fingerprint and so could never be approved,
  and deleting a disabled installed skill then dead-ended with
  "External changes must be reviewed" until discovery recovered. It is
  no longer a blocker. The stillOwnedKeys journal check still refuses to
  forget a skill whose files are on disk. Fail-first test.
- LOW: the custom-skills list shows the provider on an unsupported row,
  where it used to show a generic path.
- LOW: three comments still described the removed append mechanism.
- NIT: an installed skill's error row shows its message.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Juliusolsson05

Copy link
Copy Markdown
Owner Author

Review (CHANGES REQUESTED) addressed in c1dd4882:

  1. MEDIUM: the discovery-error row is no longer a delete blocker. It names no file and has no fingerprint, so it could never be approved. The stillOwnedKeys journal check still prevents forgetting a skill whose files are on disk. The fail-first test (install, disable, discovery fails, audit, delete) fails with the old blocker filter.
  2. LOW: an unsupported row in the custom-skills list shows its provider.
  3. LOW: the stale append comments are rewritten.
  4. NIT: an installed skill's error row shows its message.

Managed-skills and settings: 147/147, and tsc is clean.

@Juliusolsson05
Juliusolsson05 merged commit aee2c0e into main Sep 19, 2026
2 checks passed
@Juliusolsson05
Juliusolsson05 deleted the fix/conventions-unsupported-minors branch September 19, 2026 09:09
Juliusolsson05 added a commit that referenced this pull request Sep 19, 2026
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

1 participant