Sync 3 Inkeep OSS Changes - #1195
Merged
Merged
Conversation
…(#3440) * fix(ok): keep OK's own skills out of the fullscreen graph (PRD-7959) The link index treats skill bundles as graph nodes deliberately, but a global skill can never link to a document: the index registers it node-only and resolves its refs same-scope. OK force-installs two of its own bundles into every project, so every graph carried four nodes that were structurally incapable of participating in it. Those bundles are now excluded from the fullscreen graph unless another skill links them directly by name. A toggle beside the external-URL control governs the user's own skills, project and global, and shows them by default. The docked local graph is untouched, so a skill's own neighborhood stays inspectable there. Which bundles are OK's own is decided server-side and travels on the payload as a managed flag. The reserved names live in the server package, which core and the app cannot import, and the release version-sync imports that module through a bare tsx eval that cannot resolve workspace packages, so the list cannot move. Flagging server-side leaves exactly one copy of it. Also extracts the graph's yes/no preference helpers into lib/bool-pref.ts with tests, including the backward-compatibility case the external-URL toggle relies on. Storage now holds only the deviation from a preference's default, so a default-on preference survives a storage clear. * fix(ok): enforce graph skill reference invariant (PRD-7959) * docs(ok): qualify the built-in graph exception as same-scope (PRD-7959) R3b and D-K said a built-in surfaces when another skill links it directly by name, without noting that ref resolution never crosses the scope boundary. A live probe against a running server confirmed the narrower behavior: a project-scope skill referencing /open-knowledge-write-skill draws no edge at all, so it cannot surface a user-global built-in. Only another global skill can. Behavior is unchanged; the spec described the exception as broader than it is. * docs(ok): scope bool-pref to its encoding and name the migration barrier (PRD-7959) Review flagged bool-pref.ts as a parallel to follow-file.ts with a different boolean encoding. The concern is real but the framing was narrower than the facts: the split is pre-existing and codebase-wide, not two files. Roughly a dozen prefs already encode 'true'/'false' and about five encode '1'/'0'. bool-pref.ts joined the majority encoding; it did not create the divide. Documents what this helper owns, why adopting it for a '1'/'0' pref would misread stored values, and that migrating one needs a per-key read-both-write-new shim rather than a call-site swap. Steers new prefs here so the codebase does not gain a third encoding. * fix(ok): record skill refs on the cold rebuild (PRD-7959) A project skill's `/skill-name` references drew no graph edges. Two defects compounded. The cold rebuild builds its state inline and never called recordSkillRefs, so refs were recorded only by the live-edit path. And skillRefs was added to the persisted cache as an optional field without bumping SNAPSHOT_VERSION, so v1 caches stayed loadable and deserialized it to empty. With mtimes matching disk the reconcile re-parses nothing, leaving refs permanently absent for every untouched file. Global bundles hid it: ingestGlobalSkillBundles re-reads them on every boot, so their refs looked healthy while project skills had none. One real project shows 7540 indexed docs with 7 skillRefs entries, all global — 156 project skills, zero. Extracts recordSkillRefsInto so the rebuild and the live path share one implementation rather than a second copy of the rule, and bumps the version so existing caches rebuild once. Either fix alone is inert: the rebuild has to record refs, and stale caches have to be discarded for it to run. * fix(ok): index one canonical copy of a skill, not one per editor root (PRD-7959) A skill projected into several editor host dirs reached the content index once per projection, so the graph drew a separate node for each copy of the same skill. The content filter carried its own hardcoded list of editor dotdirs to skip, and that list had drifted from the registry: every host was listed except `.github`. Copilot's projection was therefore not recognized as an editor artifact at all and was swept in as ordinary content, bypassing the canonical election that admits exactly one bundle per skill. Derives the skill roots from knownSkillRootsFor — the same registry the scanner uses — so newly supported hosts and user-configured custom roots are covered without a second list to maintain. A file under a known root is excluded unless the election admitted it. Scoped to the root path (`.github/skills`), never the host dotdir (`.github`): the skip set is segment-matched, so listing the dotdir would also bury real content beside the projection — one project has nine such docs, including `.github/CI_RUNBOOK`. Admission itself is unchanged. Election is per skill, so one skill can be canonical in `.github` while the next is canonical in `.claude`; a skill that exists only under one root is still admitted there. GitOrigin-RevId: ce6e6ba5e50b5cb62a87ed719b18bc880fe2e5b3
* fix(site): avoid architecture guesses in download CTAs * fix(site): preserve download picker context * fix(site): keep share attribution through picker * Use architecture hints for download defaults * Give Playwright CI setup more headroom GitOrigin-RevId: ea66345dad32c69785dacbad92f53f29916fa3cd
…(#3443)
* fix(ok): stop creating agent host folders without consent (PRD-7984)
OpenKnowledge created coding-agent config directories for tools the user
does not have, then read those directories back as evidence the tool was
installed. Project creation, launch-time skill reclaim, store migration,
import, pack seeding and repair all fanned out across the full editor
registry with no detection gate.
The rule is now uniform: never create a harness home, only write into one
that already exists. `.agents` is treated as an ordinary host under the
same rule rather than an always-written central store.
- New projects start with no agent integrations selected
- Omitted CLI skill options install nothing
- Global writes target explicitly selected or already-existing hosts
- Migration, repair, import and pack seeding skip when no host exists
- USER_SKILL_HOSTS addresses Pi and Copilot at their real global roots
* fix(ok): address review on agent-host consent fixes (PRD-7984)
Follow-up to the consent fix, from automated review plus CI.
- `.github` is not a harness home. The first-segment probe resolved any repo
with a `.github/` directory to Copilot's `.github/skills`, reintroducing the
unconsented write this change closes. Both `resolveDefaultSkillHomeRel` and
`skill-migrate`'s target picker now route through `skillRootActivationPath`.
- The default global target set no longer emits an editor id outside the
install-target vocabulary, so a projected copy is always visible in the
picker and removable by a set-exact install.
- The repair gate is evaluated per bundle instead of over a pooled entries
array, so a bundle that lands nowhere reports `failed` rather than
`skip-current`. Applied to both the CLI sweep and the desktop reclaim, which
are documented as applying the identical gate.
- Create-project seeds its editor selection from detection instead of an empty
set. The checkboxes sit inside a collapsed Advanced section, so an empty
default silently wired nothing at all, including for editors the user has.
The default-vs-customized variant is now set-equality against that baseline.
- `resolveInitSkillEnablement` is back to `undefined | true` enabling every
bundle. Enabling is not writing: `installUserSkill` still refuses every
destination whose host root is absent.
- The integration harness seeds a `.claude/skills` host root alongside the
`.ok/config.yml` seed it already writes, since skill create now refuses when
no harness home exists.
* fix(ok): stop the boot watcher conjuring agent homes (PRD-7984)
The managed-artifact watcher `mkdir -p`s every root it is handed, and the
root list was the full vocabulary rather than the roots that exist. So every
server boot fabricated `~/.cursor/skills`, `~/.codex/skills`,
`~/.copilot/skills`, `~/.opencode/skills` and `~/.pi/agent/skills` on a
machine with none of those tools, and detection then read them back as
evidence the tools were installed. The call site already documented this gate
("never CONJURE a vendor tree"); no code implemented it.
`managedArtifactSkillsRoots` now filters to roots whose activation path
exists, at the single chokepoint both the watcher and `getGlobalSkillRoots`
read from. The predicate is `skillRootActivationPath`, not the install-target
set, because this is a watch list: it must still admit `.gemini/skills` and
ledger-recorded custom roots, and it handles Pi's nested `.pi/agent/skills`.
The gate exposed four integration fixtures that only ever resolved a global
skill home because boot had conjured one; they now adopt a host explicitly.
Also from review:
- `applySeed`'s post-install reconciliation turned the deliberate no-host
refusal into one user-facing error per pack skill, so `ok seed` in a project
with no adopted harness reported "skill source could not be authored"
through the CLI, the HTTP route and desktop IPC. Gated on the same
predicate; genuine partial installs still report.
- The changeset text no longer claims new projects start with nothing
selected, which stopped being true when create-project moved to detection.
- The migrate skip assertion pins `target-occupied` rather than matching only
the skill name, so it cannot pass for the wrong reason.
* fix(ok): make seed and the create dialog honest about agent folders (PRD-7984)
Follow-up to the boot-watcher fix, from CI.
`ok seed` claimed work it would never do. The plan marked pack skills pending
whenever they were absent, but apply refuses to author them when the project
has no agent folder, so a re-run reported `applied` every time while writing
nothing. Plan and apply now branch on one resolver, `resolvePackSkillHome`,
which also folds in the symlink-escape case so that path can no longer produce
phantom "skill source could not be authored" errors either. The CLI now says
which folders it would accept and what to do, on the first run as well as
re-runs.
The create-project dialog could discard a user's choice. Its editor selection
is seeded asynchronously from `bridge.integrations.status()` on open, and a
user who expanded Advanced and ticked a box before that resolved had the
selection overwritten. The prior comment argued this was not reachable because
Create stays disabled until the cascade probe settles, but that covers submit,
not checkbox state. A touched flag now lets the visible state win.
Tests updated for the retired all-editors-pre-checked default:
- The three `qa-create-new-extended` smoke tests assert the detection-seeded
baseline, and QA-005 now proves both directions on disk rather than only the
negative half.
- The Playwright fixture seeder adopts a host, since project skill authoring
refuses without one.
- The seed test that deleted every agent folder and still demanded pending
work had an obsolete premise; it now keeps a host and removes only the skill,
and additionally asserts the re-run re-authors it.
* fix(ok): close out review recommendations (PRD-7984)
- The `home-escapes-project` refusal now has coverage through `planSeed` and
`runSeed`. Its symlink-specific guidance was unreachable by any test, so the
two refusal branches could have been swapped without failing anything. Both
layers now pin the verdict, and the CLI test asserts the symlink wording is
present and the no-agent-folder wording is not.
- The skill-import no-host error says what to do. It names the folders it would
accept and states that OpenKnowledge never creates one, matching the seed
refusal note. The list comes from a new scope-aware `skillHomeCandidateFolders`
reading the same root tables `resolveDefaultSkillHomeRel` consults, so it
cannot drift. The urn and detail codes are unchanged.
- `detectedEditors` becomes `detectedEditorIds` on the integrations status. It
is a flat id array and was reusing the name of a pre-existing rich-object
field on the wiring payload. That field and its consumers are untouched.
- The multi-host occupied-slot migration case is pinned: a free lower-precedence
host does not rescue an occupied higher one. The comment records why the
inversion is deliberate, since scattering a skill to whichever host happened
to be free lands it somewhere the user never chose.
GitOrigin-RevId: 56a4092e7d83518e5baa83d40b267412ee5d27ef
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
2 participants
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.
Copybara-translated 3 Inkeep OSS changes. Rebase-merge this PR so the prepared commits land directly on public main.
Commits: