Tags: inkeep/open-knowledge
Tags
Add appliesTo folder picker (PRD-7648) (#3401) * feat(ok): pick appliesTo folders from a checkbox tree + live match count (PRD-7648) Frontmatter schema rows gain a folder picker that authors folder/** globs from the project's folder tree (per-folder doc counts, covered descendants read indeterminate), keeping the raw pill input as the untouched escape hatch. The summary line now reports 'Matches N of M docs right now' live, so a bare folder name like 'blog' reads 0-of-N the moment it's typed instead of only after the server's zero-match warning. * refactor(ok): folder picker as house-style cmdk combobox; okignore-style count Replace the bespoke checkbox-tree popover with the NewItemDialog combobox anatomy: full-width trigger summarizing the selection, trigger-width popover, cmdk search over folder paths, Check-marked multi-select rows that toggle without closing. Inherits the popover-inside-Dialog scroll workaround (radix-ui/primitives#1159) and the cmdk-input focus routing the bespoke popover was missing — wheel scroll inside the folder list was dead inside Settings. The match-count line drops amber for the .okignore preview conventions (polite status, de-emphasized zero) and teaches the /** fix only when a bare folder name is actually authored. * test(ok): e2e for the glob picker's composed write path + scroll pin (PRD-7648) Substrate-adequacy pass: the DOM tier mocks both system boundaries (config binding, page list), so it proves what the UI sends, not that the system accepts it. New Playwright spec drives the real chain — pick a folder -> CRDT patch -> config.yml bytes on disk -> server lint compose -> validation badges scoped to the picked folder — plus the live match count against the real page list, and a wheel-scroll pin for the react-remove-scroll popover-in-dialog class jsdom is blind to (the regression that shipped in the first picker iteration). Added to the CI e2e subset. * fix(ok): satisfy new lint rules post-merge (Trans-wrapped plural forms, ms-2) Main grew two GritQL rules since the branch point: user-facing JSX text must sit inside Trans (the Plural forms now wrap their sentences), and physical direction utilities are banned (ml-2 -> ms-2 on the trigger chevron). Rendering is unchanged — the count-line dom assertions and the e2e pass as-is. * i18n(ok): translate the folder picker's 11 strings across all ten locales The picker-completeness gate requires every picker-promoted locale at 100%: the agent that writes a string writes its ten translations in the same change (locales/REVIEW.md). Terminology follows GLOSSARY.md and the existing catalog (esquema/schéma/架构/結構描述 for schema; patrón/motif/ 模式 for pattern); plural forms carry each locale's own category set (other-only for zh/id, one+many+other for es/fr/pt-BR, the full six for ar). Machine-translated, plainest-literal renderings per translate-ui-strings; no glossary additions needed. * fixup! local-review: baseline (pre-review state) * chore: drop stray baseline artifacts and unrelated MCP config churn The local-review baseline snapshot swept five working-tree files into the branch: a generated browser-skill scratch script, a pico editor backup inside public/open-knowledge, and an ok-mcp-v1 to v2 rewrite of the three harness MCP configs. None of them belong to PRD-7648. Remove the scratch files, restore the configs to main, and ignore both scratch patterns so they cannot be committed again. * test(ok): picker remove-path e2e + coverage gaps (PRD-7648) Review follow-ups for the folder picker: - e2e: unchecking a picked folder now proves the full remove chain, CRDT patch to persistence to config.yml, and pins that emptying the list drops the appliesTo key instead of writing an empty array. - unit: countMatchingDocs with two additive folder patterns pins union semantics. dom: a disabled trigger neither opens nor authors globs, and a no-hit search shows the empty message. - docs: the settings-editor paragraph now names the Pick folders button and the live match count. - selectedFolders returns ReadonlySet, matching coveredByAncestor. - trim design-lineage narration from two comments per the comment discipline. - worktree-service tests: accept both git wordings for an unresolvable base ref (Apple Git 2.39 says not a valid object name, newer gits say invalid reference), so the suite passes on stock macOS git. * fix(ok): address folder picker review findings (PRD-7648) * chore(ok): keep PRD-7648 changes scoped * chore(ok): record folder picker bundle budget (PRD-7648) * test(ok): harden folder glob boundaries (PRD-7648) --------- GitOrigin-RevId: e191b4467d9f490d1d01da1d705e979897348657
feat(ok): remove ok ui command and sibling UI spawn machinery (#3421) * feat(ok): remove ok ui command and sibling UI spawn machinery Wave 5 removal 1. Plain ok start has served the editor, API, and MCP on one port since the single-listener flip, and Desktop attaches via server.lock v2, so the two-process sibling model retires: - drop the ok ui Commander command; ui.ts survives only as the engine behind ok start --only ui --server-url, which now prints a deprecation notice (it was shipped without one, so it gets a notice release before its own removal alongside ui.lock) - strip the start.ts sibling machinery: --ui-port, spawnOkUi, decideUiSpawn, awaitUiSiblingPort, connectUiSibling, the idle-shutdown sibling teardown, and the banner's resolvedUiPort plumbing - bootServer loses attachUiSibling/spawnUiSiblingFn; ui.lock writing is untouched (retires in the follow-up wave) - the clone/local-op flow resolves its redirect from server.lock (url + ui capability) instead of polling ui.lock and spawning ok ui; a live --only server holder is now an explicit no-UI error - operator/agent hints that named ok ui (404 bodies, MCP preview messages, ok open, removed-keys redirect, app connect-banner strings in all locales, skill assets, docs) now point at ok start * feat(ok): add ok ui removed-command tombstone ok ui previously fell through to the default command's arity error (too many arguments), which tells someone with the old command in muscle memory or a stale script nothing actionable. Register a hidden stub that prints the removal pointer (run ok start) and exits 1. Accepts any flags/args so ok ui --port 39847 lands on the same message. Drop at the removed-command horizon. * fix(ok): address ok-ui removal review findings - perf-prod.sh: single-listener model. One ok start serves the shell, API, and collab; Playwright targets the server port; the script also polled the pre-local/ lock path, so fix that to .ok/local/server.lock. - clone redirect: restore the ui.lock fallback resolveUiInfo still honors. New resolveUiRedirectPort (ui-redirect-port.ts, unit-tested) runs the same two-source chain, so a live --only server + --only ui split pair resolves to the sibling port instead of a false no-ui error. Error copy now also names the missing-bundle cause, which writes the same no-ui capability set. - --only ui deprecation notice moves above the refusals (mirrors the --remote ordering) and gains a test; the notice-free refusal path previously made it untestable. - resolveStartShellDir: drop the dead remoteEnabled parameter left behind by the uiPortSet removal; delete the duplicate test whose title referenced the removed opt-out. - stale comments: TOCTOU exited:true reference, desktop-attach-contract reader list, awaitUiBaseUrl sibling wording, mcp-mount shell note. - docs: what-open-knowledge-writes lock rows no longer present the removed --ui-port as a live path; subtree README drops the ok ui command row and auto-spawn claims. * chore(ok): review-pass housekeeping on the ok-ui removal - README: drop the leftover cli.mjs ui example line next to the block the previous commit updated - perf-prod.sh: remove the dead run_start_ts variable and fix the results-selection comment to describe the actual N-most-recent pick - ui-redirect-port: rewrite the step-1 guard via De Morgan so the two admit cases (absent capabilities = optimistic, explicit ui entry) read directly, and pin the draining branches with two new tests (draining no-ui server.lock resolves null, not no-ui; a draining ui.lock is never a redirect target) GitOrigin-RevId: 95bb30b4e8e2aeadaeaac6cdbcd9971860b472bf
chore(ok): skills cleanup deferred from the marketplace PR (PRD-7707)… … (#3323) * chore(ok): skills cleanup deferred from the marketplace PR (PRD-7707) Six deferred items from PR #2923. 1. Fix a `use-skills` staleness bug. `fetchSkillsShared` coalesced on a bare promise, so a refetch triggered by a mutation joined the request that was already in flight before that write. The list settled on pre-write data and never self-healed. Requests now carry the generation they started at, every mutation signal bumps that generation synchronously, and a request from an older generation is never joined. The `.finally` reset is identity-guarded so a superseded request cannot clear the newer one that replaced it. 2. Hoist `.agents/skills` to `AGENTS_SKILLS_ROOT` in core. It was a bare literal at 17 code sites across core, server and app with no shared constant, so moving the hub meant finding every one of them by hand. 3. Trim both AGENTS.md files back under their caps. The Open Knowledge one was at 39,980 of 40,000 bytes, so the next ordinary edit would have been blocked by the pre-commit size guard; the root one was at 38,447 against its 38,500 test limit. Now 37,584 and 36,715. Only prose that the linked canonical docs already carry was cut; every rule, pointer and STOP/WARN entry is intact. 4. Give the skills mirror real leak detection. `check-skills-mirror-faithful` already runs the real Copybara jar, but it only proved the emulator and the jar agree, which says nothing about whether what they both ship is safe to publish. It now runs the same apparatus and content-leak detectors the docs mirror uses against the materialized tree, and asserts the root furniture (README, LICENSE, marketplace manifest, authoring template) survives. That last arm is the one the emulator diff structurally cannot cover: an include narrowing drops a file from both sides at once and reads as clean. The ticket asked to parameterize `check-mirror-export-faithful` over both manifests instead. That was written before the skills gate existed, and doing it now would run the jar over the skills export twice, with a config path that skips the derived skill moves. 5. Extract the install-menu row derivation to `lib/skill-install-rows.ts`, with tests. The component keeps its JSX untouched. 6. Extract the sidebar path builder to `lib/skills-tree-paths.ts`, with tests. `EMPTY_SCOPE_SENTINEL` moves to the builder that emits it and is imported back by `SkillsTree` for its CSS. Verification: OK lint, typecheck and knip clean; core 6501, server 7063 and app integration 1529 tests pass; root `test:scripts` 582 pass, plus boundaries, hook-sync, cross-harness-config, monorepo-traps and mirror-gate-reach. The app `test:dom` suite fails 21 files / 341 tests, byte-identically on the clean base: the known local-only Node 26 `localStorage` cluster, green on CI Node 24. * chore(ok): add changeset for the skills-list staleness fix (PRD-7707) * fix(ok): use a literal for the .agents adoption marker (PRD-7707) The hub-adoption check asks a different question from the hub's install path: "does this project use `.agents/` at all?" versus "where does the hub live?". Deriving the first from the second by splitting AGENTS_SKILLS_ROOT coupled them, so moving the hub to a nested path would have silently broadened the adoption check to that path's first segment. Back to the bare literal the pre-extraction code used, matching the other reader of the same concept in AgentIconCluster. * test(ok): cover the CC1 remote branch and the detected-active path (PRD-7707) Two coverage gaps the review flagged, both in code this PR made testable. The staleness fix invalidates from two subscriptions, the local skills bus and the cross-client CC1 `files` signal, but only the local one was exercised. Dropping the remote call or breaking its channel filter would have regressed cross-window mutations with the suite still green. Both branches now run the same case, plus one pinning that an unrelated channel does NOT invalidate, so a backlink recompute cannot spin a full skills scan. That second case forced the file to stop leaking module state between tests. `useSkills` holds its list, in-flight request and generation at module scope and vitest resets the registry per file, not per test, so a second test would have mounted already-ready on the first one's data and its waitFor would have passed before any fetch resolved. Each test now re-imports a fresh module graph and asserts the loading state first, so the reset itself is pinned. Also covers the detected-skill active path in the tree builder, including that an active managed skill still wins the highlight. Verified non-vacuous: both staleness cases fail against the pre-fix hook, and breaking the detected-active branch fails the new test. * docs(ok): document error-path correctness on the in-flight guard (PRD-7707) The guard is load-bearing on the rejection path too: .finally releases the slot for a failed request so later callers do not join a promise that can only re-reject, and the identity check is what stops a superseded request settling late from nulling out the live one that replaced it. Names the bare .catch reset a maintainer would reach for, since that runs for the same superseded requests without the check. * fix(ok): restore single-flight and complete the mirror survivor list (PRD-7707) Self-review follow-up: two defects this PR introduced, one per file pair. 1. use-skills: the generation counter fixed the staleness this PR set out to fix, but paid for it by dropping the guarantee the code it replaced provided, and wired invalidation in the wrong place. /api/skills is a synchronous walk of every editor skills root, and the old `??=` meant N surfaces reacting to one mutation shared one scan. The generation check made every mid-flight invalidation start ANOTHER concurrent scan, so a multi-editor fan-out install could stack several full walks on the server's event loop and stall CRDT sync. The shared request now RE-RUNS instead of racing: one scan at a time, and if a write lands while it is running its response is discarded and the fetch goes again. Callers still share one promise, and that promise can now only resolve with post-write data, which is why the shared lastKnownSkills seed no longer needs a staleness check of its own; it was previously seeded by whatever resolved, including a pre-write response landing inside the debounce window. The retry is bounded, since it is driven by external writes and would otherwise spin under a sustained mutation stream. Invalidation moved to module scope. It was wired inside the per-instance subscriber, so a write landing while nothing was mounted never invalidated, and a slow request outliving the last unmount is exactly the case this exists for. The next mount would join it and render a deleted skill. Drops the counter pair: with the identity guard on the reset, the two module-level mutables only fed one equality test a plain stale flag expresses directly. 2. skills mirror gate: the survivor list was hand-copied from an export listing and covered four of the seven furniture files the manifest names, so the both-sides-drop hole it was added to close stayed open for the rest, including the PR-bridge workflow whose silent loss strands external contributor PRs rather than failing anything. The missing direction was the test, not the list: it checked entries -> assertions and never furniture-on-disk -> entries, which is why an incomplete list passed. A test now walks the authored tree and fails on anything unnamed. Also scopes the content scan. findContentLeaks was written for a source tree where a leading # or * means a comment; this export is 47 of 52 markdown, where they mean a heading and a bullet, so that arm would have hard-failed a public contributor's PR for writing an internal tool name in a heading with no way to suppress it. Dropped for markdown only; the arms that apply to prose still run everywhere and non-markdown furniture keeps the full check. The DOM tests now assert the concurrency ceiling alongside freshness, since the ceiling is what regressed. Both still fail against the pre-PR hook. * fix(ok): do not require a DOM to import use-skills (PRD-7707) Regression from the previous commit and my own fault for how I verified it. Moving invalidation to module scope wired the listeners at module LOAD, and subscribeToSkillsChanged calls window.addEventListener, so merely importing this module needed a DOM. Every node-env test that transitively pulls it in died at load with "window is not defined" - use-skill-origin, use-skill-directory, use-reconcile-skill-tabs, FileSidebar and the command palette suites all reported "0 test". SSR would have hit the same wall. The listeners now attach on first use behind a typeof-window guard. That keeps what the move was for: once attached they outlive every unmount, so a write landing while nothing is mounted still invalidates, which is the window the stale-join bug lives in. Nothing is cached before the first fetch, so attaching there loses no coverage. Adds a node-environment test that imports the module and asserts no DOM is required, since that is the contract that broke and it is invisible from the dom-config suite where window always exists. I had only run the dom config and the lib tests before pushing; the whole class was one `pnpm test` away. Full turbo test now green across all 13 tasks (core 279, server 448, app 492, desktop 194, cli 128, conformance 115 files). * fix(ok): redirect a preview into the scope the skill landed at (PRD-7707) Installing from the skills browser could leave two tabs open and strand the new one on "Couldn't load document" until the sync timed out. One cause behind both halves. After import the preview tab redirects into the real skill via useOpenSkill, which resolves a skill's doc by (scope, name). The redirect passed the LIVE `scope` selector rather than the scope the bundle actually landed at, and the hook tracked the real one in importedScopeRef but never exposed it. When the two disagree the entry lookup misses, so the replaceActive swap never happens: the preview tab survives and a second tab appears later, while the doc that did get opened belongs to a scope the skill is not in and never resolves. Observed with a skill that imported at global scope while the tab opened ".claude/skills/<name>/SKILL", a project-relative path that did not exist - the bundle was at ~/.agents/skills/<name>/SKILL.md the whole time. The hook now surfaces importedScope and the redirect prefers it, falling back to the selector only for the bulk-install path, which imports at the current scope and reports none of its own. * fix(ok): wait for the skill to be resolvable before leaving the preview (PRD-7707) Installing from the skills browser left you on the preview page even though the skill had landed. The redirect fired the moment the import returned, but useOpenSkill resolves a skill's doc from the skills LIST, which lags a fresh import by the skills-changed refetch. The fire-once ref is set BEFORE the open resolves, so an early fire burns the single attempt on a lookup that cannot succeed and the preview is stranded for good - no retry, no error, just a preview tab for a skill you already own, with its slash-links dead and its INSTALL menu still armed. The redirect now waits until the skill is actually present in the list and opens it at the scope the list reports. It stays gated on landedName, so previewing a marketplace skill whose name already exists locally does not bounce you out of the preview you asked for. Tests: the per-agent install path had NO coverage - the existing suite only exercised the bulk-plugin branch - which is why this shipped. Adds that case plus a non-vacuity test asserting the redirect does NOT fire while the list still lags, since firing early is the whole defect. The bulk test needed its skills list mocked; when it failed on the first attempt that was the test correctly reporting the new contract, not stale. * fix(ok): close the stale preview tab after redirecting to the skill (PRD-7707) Installing from the skills browser left two tabs: the real skill, and the preview still advertising "Install it into your agents" for a skill you already own. replaceActive does not replace THIS tab. It opens with preview DISPOSITION, so it swaps whatever currently holds the preview slot - which is not the same thing. A skill-preview tab opened permanently therefore survives, and the real skill lands beside it. When another surface had already opened and focused the real skill, the swap consumed that tab instead and the preview was orphaned outright. The preview now captures its own tab id before opening and closes it after, so the redirect ends with exactly one tab regardless of which tab held focus. Closing AFTER the open means a failure to resolve can never leave zero tabs. * fix(ok): keep the Skills sidebar pinned across a scope move (PRD-7707) Moving a skill between project and global dropped you into the Files tree instead of following the skill. Every other skill mutation pins the sidebar after it retargets - delete, file delete, rename - but the scope move never did. A project skill's doc is ordinary project CONTENT (.claude/skills/<name>/SKILL), so retargeting onto it lets the surface autofollow to Files. The move now pins Skills the same way its siblings do. * fix(ok): repoint the real doc on a scope move, never a minted store path (PRD-7707) A project-to-global move could leave the open tab pointing at a path the move had just deleted, which then sat on "Couldn't load document" until the sync timed out. An in-place project skill is open at its real content path (.claude/skills/<name>/SKILL). The retarget resolved its FROM doc from the skills list and, when that lookup missed, fell back to skillLiveDocName - the STORE shape, .ok/skills/<name>/SKILL. No tab carries that name, so the retarget matched nothing and returned silently, stranding the tab. The lookup misses routinely rather than rarely: beginOptimisticSkillMove has already dropped the source row by the time this runs, and the list may still be refetching. So the broken fallback was the common path. It now resolves the FROM doc from the open tabs, and refuses to retarget at all when no tab matches, rather than guessing. useOpenSkill already carries the same never-mint-the-store-shape rule; this is the path that was missing it. Global keeps the managed __skill__/global/<name> name, which is real rather than minted. Test drives the move with a deliberately not-ready list - the condition that makes this fire - and pins that the retarget targets the in-place doc and not .ok/skills. It fails against the previous fallback. * fix(ok): restore the scope-move suite and cover the skip-retarget guard (PRD-7707) CI caught a break I introduced: useMoveSkillScope now reads DocumentContext (to pin the Skills sidebar and to find the open tab it must repoint), and SkillScopeMoveDialog renders it inside a suite with no DocumentProvider, so use-skill-scope-move.dom.test.tsx died with "must be used within <DocumentProvider />". Stubbed the context there, matching how that file already stubs its other dependencies. Worth naming the cost: adding a context read to a shared hook makes every consumer's test provide that context. One suite here, but that coupling is real and this is where it showed up. Also covers the guard the reviewer flagged. The suite exercised the case where an open tab IS found, but not its complement - no tab matches, so fromDoc is null and the retarget is skipped. That guard is the safety net against the store-shaped phantom this change removed, so a regression that dropped it or passed null through would have gone unnoticed. It now asserts retarget is not called while the sidebar still pins to Skills, since the move itself still succeeded. * fix(ok): do not close a skill tab during its own scope move (PRD-7707) Moving a skill from project to global closed its tab and dropped you on an empty "New tab" with the sidebar falling back to Files. The reconciler exists to close tabs for skills that no longer exist, and it decides that by looking the skill up in the skills list. A move passes through a window where the skill is in NEITHER scope: the source row is hidden by beginOptimisticSkillMove and the destination row has not landed yet. Absent at both scopes reads as deleted, so the tab is closed - the user asked to move a skill, not to close it. The retarget that should have repointed the tab then has nothing left to act on, which is why fixing the retarget alone changed the symptom from a stale tab to no tab. The optimistic-move bus already tracked the pending move but never exposed it, so the reconciler could not tell "mid-move" from "deleted". It does now, and holds the tab through the window; the move's own refetch retargets it once the skill reappears at its new scope. This is the structural fix rather than a timing one. The window's width varies with how quickly the list settles - my earlier useSkills change widened it, which is why this surfaced here - but any consumer that destroys user state on a transient absence is wrong regardless of how narrow the window gets. Tests pin both directions from identical inputs: in-flight move holds the tab, genuine deletion still closes it, and the retarget still fires once the skill lands at its new scope. * fix(ok): preserve a location's form when installing without a mode (PRD-7707) Re-installing a skill whose locations were SYMLINKS silently rewrote them as full copies, then reported the result as "changed outside". The install fan-out passed input.installMode straight to projectInPlaceSkill, which falls back to `copy` when that is undefined. So an install that named no mode - the common case, since the menu only sends one for an explicit choice - flattened every symlinked location into its own copy. That duplicates bytes into git and detaches those locations from the source, so editing the skill stops propagating to them. Nothing told the user any of it happened. The badge was then correct and blamed the wrong actor: the placement ledger still recorded `link` for paths now holding real directories, so drift detection fired on OK's own conversion. effectiveInstallMode already exists for exactly this and documents the intent - "a skill whose locations are all copies keeps getting copies, so a new one matches its siblings rather than silently mixing forms". skill-import already takes it as a dep; the install path did not. It does now, and only the IMPLICIT default changes: convertLinks still honours an explicit copy request, so deliberate link-to-copy conversion is untouched. linkedHosts rides along on the canonical entry, which already comes straight from the in-place scan that computes it. Worth noting the earlier framing was wrong: the scope move never re-projects at all, so it was not losing the form. The round-trip only made the user re-install, and the install is where it was lost - which means any re-install had this, not just one following a move. * fix(ok): never close a skill tab while a write is in flight (PRD-7707) Installing onto a second editor could close the skill's tab, dropping the user on an empty "New tab" with the sidebar falling back to Files. Same root cause as the scope-move case, wider trigger. A skill write is not atomic on disk - projectInPlaceSkill rm's each destination before materializing it - so a scan taken mid-write reports a skill that plainly exists as absent. The reconciler reads absent-at-both-scopes as deleted and closes the tab. The earlier guard only knew about optimistic MOVES, so an ordinary install walked straight through it. Rather than enumerate mutations, this adds the missing signal: a write-in-flight registry in documents-events, marked by install and by location writes (both of which already bracket their call with setInstallingName, so the pairing is exact). Absence during a marked write is no longer evidence of deletion. Two design points worth keeping: Only the DESTRUCTIVE branch is held. Retarget still follows the skill immediately - repointing a tab is safe and reversible, and my first attempt guarded both, which needlessly deferred the move retarget. The existing test caught that. pendingSkillWritesKey is folded into the dedupe signature. A guarded close is deferred, not cancelled, so clearing the flag has to re-run the effect; without it the list is identical either side of the write, the effect short-circuits, and "close once it settles" becomes "never" - leaking dead tabs. An earlier two-read confirmation approach died on exactly this and was reverted. Also collapses the now-subsumed move-only arm, which had been left as an empty block with its comment lost to a formatter pass. * fix(ok): an install must not restamp a location's existing form (PRD-7707) Installing one more editor rewrote the form of locations the user had already set up: adding .codex converted .agents from a copy into a symlink, after which the placement ledger disagreed with disk and the UI reported the user's own install as "changed outside". The two directions were asymmetric in projectInPlaceSkill: link-to-canonical -> copy required convertLinks (an EXPLICIT choice) same-copy -> link fired on any `mode === 'link'` So an existing symlink survived an implicit install but an existing copy did not. My earlier change made the implicit mode the skill's EFFECTIVE form rather than a hard-coded copy, which is correct for a new location and is what turned that latent asymmetry into a visible bug: for a skill whose effective form was link, every existing copy got restamped. Adds convertCopies as the mirror of convertLinks, set only from an explicit link request. Now neither direction converts without the user asking, and `mode` decides only what a BRAND-NEW location gets - which is what effectiveInstallMode was always documented to be for. Tests cover the repro directly: adding a second editor leaves an existing copy a copy and an existing symlink a symlink, while a new location still follows the preference, and an explicit choice still converts. Fixture note: `same-copy` is decided by a content-hash compare, so the test derives the real hash via parseSkillDir - an empty hash classifies an identical copy as `different` and silently exercises the wrong branch. * fix(ok): keep the surface on Skills when opening a skill (PRD-7707) Opening a skill dropped the user into the Files tree. A PROJECT skill's doc is ordinary project content (.claude/skills/<name>/SKILL), so the surface autofollowed the target out of Skills. Pinned in useOpenSkill, which its own docs establish as the single entry point every open-a-skill surface must route through - sidebar click, create, import, detected-adopt, and the post-install redirect out of a preview tab. Pinning there covers all of them at once; the sidebar's own handlers already did it locally, which is why a sidebar click behaved and the install redirect did not. * fix(ok): admit a freshly installed skill before responding (PRD-7707) Opening a just-installed project skill hung for 30 seconds, flipped the UI through fallback states, then worked. 30s is SYNC_TIMEOUT_MS exactly: the document sync never resolved, timed out, and the retry succeeded. A project skill's SKILL.md is only servable once its dir is in the content-filter's in-place allow-list, and a dir only enters that list on rebuildIgnorePatterns(). The skill-get handler states the trap in its own comment - "which an API write does not otherwise trigger" - and both it and the scope-move handler call it for exactly this reason. The install handler never did, so the client could open a skill the server still judged excluded and wait out the full timeout until some later rebuild admitted it. The install path now refreshes the allow-list before responding, at each of its four success exits, matching what its siblings already do. Why it surfaced now rather than earlier in this branch: the redirect had to work before it could hit this. While the post-install open was going to the wrong scope, firing before the skill was resolvable, or being evicted by the reconciler, it never reached the window where the doc is written but not yet admitted. Fixing those exposed this one - each earlier fix removed a visible consequence and left the hang underneath, which is why the symptom kept changing shape instead of going away. Full server suite green (457 files, 7333 tests). * fix(ok): pin the Skills surface AFTER the open, not before (PRD-7707) Installing a skill still dropped the user into the Files tree: my previous commit pinned the surface BEFORE openTarget, and committing a tab re-arms autofollow, so the pin was immediately overwritten. SkillsSidebarSection already states the rule on its own call - 'Must run AFTER the open - committing a new tab re-arms autofollow, and this is the pin that overrides it.' That is exactly why a sidebar click behaved while the shared opener did not: the sidebar pinned after, useOpenSkill pinned before. Ordering is now openTarget -> pin -> hash. * fix(ok): record the old source's real form on a source move (PRD-7707) Promoting a location to SOURCE left a "changed outside" badge on whichever location used to be the source, and any later install cleared it. promoteInPlaceSource recorded the old source path as mode 'link' unconditionally, on the premise that relocating always leaves a symlink behind. When it does not, the receipt claims a symlink while a real directory sits there, so the next drift read flags OK's own promote as an external edit. That matches the behaviour exactly: the badge always lands on the PREVIOUS source, and it clears the moment an install rewrites and re-records that location with its true form. It now reads the form back with one lstat and records what is actually there. No disk write changes - only what the ledger claims about one. A receipt cannot assert a form the disk disagrees with, which is the property that makes drift detection meaningful. A missing path keeps the previous 'link' record rather than asserting 'copy' for something that no longer exists. Note this does NOT explain why the old source is a real directory rather than the symlink relocate is expected to leave - that question stands, and if the symlink is genuinely not being written there is a second defect underneath. This stops the ledger lying about it either way, which is what produced the false badge. * refactor(ok): route the sidebar skill click through the shared opener (PRD-7707) useOpenSkill documents itself as the single entry point every open-a-skill surface must use, naming "sidebar click" explicitly. The sidebar had drifted: it minted the doc name itself and called openTarget directly. So the same visible action behaved differently depending on where it was invoked from, and every fix landed on one path only - scope resolution, the fresh-entry fallback that refetches when the list lags an install, and pinning the surface to Skills all applied to the redirect but not to a click. That divergence is why these bugs kept reappearing in a slightly different shape after each fix. The click now delegates, preserving preview-tab behaviour by mapping tabBehavior onto replaceActive and passing the entry's real path so the opener can resolve a freshly installed skill without waiting on the list. openSkillDoc stays for bundle FILES, which are ordinary content docs and not skill identities, so they do not belong in the skill opener. * fix(ok): hold the tab guard for the whole scope move (PRD-7707) Moving a skill still closed its tab and dropped the user in Files, and the relocated skill then would not open. The guard was released too early. endOptimisticSkillMove fires as soon as the request returns - BEFORE the retarget runs and before the list carries the skill at its new scope. So the reconciler got a settled read with the skill absent from both scopes and no guard set, and closed the tab. The retarget then had nothing left to repoint, which is why the skill was unopenable afterwards. The move now brackets the whole operation with the write registry: marked before the optimistic hide, released only after the retarget and the surface pin, and on the failure path too. The optimistic row-hide keeps its own lifetime - it is about hiding a row, not about protecting a tab, and conflating the two is what left the gap. Test mocks for the events bus needed the new exports; the failures were the mocks, not the logic. * fix(ok): a source promotion leaves the old location in its siblings' form (PRD-7707) Changing the source left the old location as a symlink while the skill's other locations were copies, so the menu immediately tagged it as divergent and offered to convert it. The app warned about the state it had just created - and guaranteed, not incidentally: a promote leaves exactly one link, so with two or more copies it is always the minority. relocateInPlaceCanonical hard-codes leaveLinkBehind. It never consults effectiveInstallMode, whose contract is exactly this case: "a skill whose locations are all copies keeps getting copies, so a new one matches its siblings rather than silently mixing forms." That helper is the established policy and the established pattern is that the CALLER consults it and passes the result down - skill-import does, and the install fan-out now does. The promote path was the remaining caller that decided for itself. It now asks the same helper and reconciles the left-behind location to match, using the same rm/copy/record shape the install path already uses, so nothing new is introduced. Deliberately not restructured: the primitive keeps its boolean and its signature, and no new abstraction is added. The point was to stop diverging from what is set up, not to invent a fourth way of deciding. The fork `align` branch is the one remaining caller that passes no mode. Left alone here - it resolves a conflict rather than relocating a source, so it wants its own reasoning rather than being swept in. Full server suite green (457 files, 7333 tests). * fix(ok): a cross-scope move leaves the skill openable (PRD-7707) After moving a skill between scopes the destination SKILL.md could not be opened - clicking it fell back to a Files tab, and it only started working roughly thirty seconds later once something else happened to rebuild. handleSkillMoveScope never rebuilt the content filter. The destination scope's skill dir is not servable until the filter's in-place allow-list knows about it, so between the move and the next unrelated rebuild the file the app had just been told to open did not exist as far as the server was concerned. This is the same defect, and the same fix, as the one already applied to handleSkillInstall in this branch: rebuild before the success exit. The install handler does it before all four of its exits; the move handler, immediately above it, did it nowhere. Adding the call matches the established placement rather than introducing anything new. Worth naming for the next reader: the symptoms this produced were blamed on several other things first - a duplicate skill entry, a stale tab, the redirect logic. None of those were it. The file simply was not servable yet. Full server suite green (457 files, 7333 tests). * fix(ok): every skill-writing handler rebuilds the content filter (PRD-7707) A skill dir is not servable until the content filter's in-place allow-list knows about it, and an API write does not otherwise trigger a rebuild. Miss the rebuild and the skill the user just created cannot be opened: the editor falls back to a Files tab until something unrelated rebuilds, which is where the roughly thirty second delay came from. Nine handlers write skill dirs. Four rebuilt. Five did not: duplicate, import, bulk import, restore, reimport. The import gap is the one users hit most, since it backs install-from-preview. This has now been fixed three times one caller at a time - install, then move, and import was going to be the fourth. Patching the caller in front of you is not a fix when the defect is that the obligation is per-caller and nothing enforces it. So this sweeps all of them at once rather than waiting for each to be reported. The rebuild sits before the success exit, matching where handleSkillInstall already puts it. For the import path it goes before the shared respondSkillImport call, which both import entry points route through. Still missing, and worth doing next: nothing stops a tenth handler from shipping without the rebuild. A meta-test over the skill handlers - the same shape as the existing route-registry coverage tests - would close that for good. Named here so it is not rediscovered by another user report. Full server suite green (457 files, 7333 tests); one ACP thread-socket failure under parallel load passed in isolation and is unrelated. * fix(ok): open a symlinked skill at the doc the index actually holds (PRD-7707) A skill dir can be a symlink to a canonical dir elsewhere in the content tree, which is how this repo keeps its bundles in plugins/ and links them into .agents/skills. The document index holds one doc per inode, under the resolved name, so the mounted name is not a page. The sidebar minted the mounted name, the tab had nothing behind it, and the next page-list sync pruned it: the skill flicked open and vanished, and the surface fell back to Files. /api/skills now reports canonicalPath alongside path, and the doc-name builders prefer it. path still answers where the bundle lives, which is what install, reveal and host wiring reason about. Resolved with realpath rather than the watcher's folder-alias index, which is populated asynchronously after boot. Clicking a skill right after launch is exactly when that index is cold, and a list that answers differently depending on how warm the watcher is would have reintroduced the same bug on a timer. On this KB, 17 of 18 symlinked skills now resolve to real pages. * fix(ok): let the skills reconciler own the SKILL tab, not the page list (PRD-7707) A scope move deletes the source bundle before its response lands, so the source doc stops being a page mid-move. The page-list prune closed that tab, and the retarget that runs moments later only acts on a tab that is still open: it matched nothing, did nothing, and the moved skill could not be opened again until the window was reloaded. The prune now leaves the SKILL doc alone. Deletion is still handled, by useReconcileSkillTabs, which decides against the skills list and holds off while a write is in flight. Scoped as narrowly as that reason allows. A references/* tab keeps riding the page list, because an external delete (agent, MCP, another client) has no other closer, and a live provider on a deleted file will rematerialise it on the next keystroke. And only names the reconciler can parse are exempt: exempting a shape it cannot read would leave a tab nothing could ever close. * fix(ok): a scope move leaves nothing behind at the source (PRD-7707) Two leftovers from a project to global move and back. The move reused the uninstall path, which deliberately keeps one copy in the .agents hub and removes only the editor projections. Correct for an uninstall, wrong for a move: the bytes are already at the other scope, so the preserved copy was re-detected as a second skill of the same name at the scope the user had just moved away from. The placement ledger is keyed by name and outlived the move, still describing locations in their pre-move form. Coming back, the re-projection recreated them as copies while the ledger claimed links, and the list rendered that gap as CHANGED OUTSIDE: OK accusing another tool of a rewrite it had done itself. The purge is content-guarded. Two same-named bundles with different bytes are two distinct skills by design (conflictHosts), so symlinks go and a real directory goes only when its hash matches the bundle that moved. A blanket delete here would have destroyed a bundle its owner never touched. Roots come from knownSkillRootsFor plus the legacy store, so a copy parked outside the standard roots cannot survive to be re-detected. * fix(ok): stop two module-level leaks that only a reload could clear (PRD-7707) Both freeze the skills UI until the window is reloaded, which is why a reload kept looking like the cure. The scope move sets a write-pending flag that suppresses the tab reconciler for that skill, and released it after a retarget that can throw. Leak it once and the reconciler is disabled for the rest of the session: the tab is never repointed and the toolbar keeps deriving its level from the stale doc name. Releasing the flag matters more than the retarget succeeding, so the retarget is guarded and the flag is released either way. Every reader of the skills list joins one in-flight promise, released only in its finally. A fetch that never settled froze the list for the session: stale rows, a stale toolbar, and clicks minting doc names from entries that had since moved. The request now carries an abort signal, so a stall becomes one failed refresh that the next files signal retries. A failed refresh also keeps the last good list rather than replacing it with an error screen. * feat(ok): say when a skill is gitignored, and offer the one line that fixes it (PRD-7707) OK will not index a doc the sync engine could never commit, so a gitignored bundle is listed but never admitted. .claude/* is a common rule, which made this read as a random bug: the row is right there, the click produces a tab with nothing behind it, and nothing anywhere says why. The list and the document index disagreed in silence. Entries now carry ignored, and clicking such a skill explains the situation and offers to add one line to .gitignore. The line is shown before anything is written, because this edits the user's repo. The rule names the skills directory, not the bundle: git cannot re-include a file whose parent directory is excluded, so a per-skill negation looks right and does nothing. After writing, the server asks the filter again and reverts if the path is still ignored, so an ineffective rule fails loudly instead of sitting in .gitignore looking like a fix. Built-ins are exempt. OK ships them read-only and a repo that ignores them means it, so offering to track one is an offer the user must not be given. * fix(ok): a skill row click can always retry (PRD-7707) Pierre selects a row on click, and a selected row emits no further selection change. So if the first click is swallowed, no later click on that row can do anything, and the only ways out are selecting a different skill or reloading - which is exactly what people were doing. Two ways it got swallowed. The handler lives behind a ref that starts as a no-op and was assigned in a passive effect, while the tree remounts on every paths change, so a click landing between paint and that effect hit the no-op; it is a layout effect now. And the handler bailed when the clicked row was already the active one, which made a failed open unretryable; re-dispatching is idempotent, so that guard is gone. A row also counts as active only when a tab is genuinely open for it. Matching activeDocName alone was wrong twice over: it survives on the last doc tab when a non-doc target takes over, and an open that never landed leaves it pointing at a doc with no tab. This is the one symptom in this batch I never captured in a log - the renderer log showed the click dispatching and the doc resolving in 301ms - so treat it as the best-supported explanation of the reports rather than a confirmed cure. * test(ok): drop an `as never` that hid the shape it was asserting (PRD-7707) Review finding. The cast disabled structural checking on the whole array, so a change to the Pick this helper takes would compile silently here while every sibling test in the file caught it through contextual typing. Not academic on this branch: the entry shape gained canonicalPath, and a Pick that omitted it was exactly the drift the reviewer is guarding against. * test(ok): stop the admission suite's teardown racing the server's own writes (PRD-7707) CI failure. Every assertion passed - 7204 tests green - and the suite still failed, in afterAll: ENOTEMPTY: directory not empty, rmdir '.../proj-.../.ok/local/cache/main' destroy() stops the server accepting work but does not join its background flushes, so the recursive delete walks a directory that is repopulated under it. Local runs pass; CI hit it on the original run and again on the retry, which is what a slower, more contended filesystem does to this window. Retry the unlink, and let cleanup failure not fail the suite: the assertions have already run and the OS reaps /tmp either way. * fix(ok): pin the surface to Skills even when the retarget throws (PRD-7707) Review finding, and correct. The pin sat inside the guard added for the write flag, so a throwing retarget skipped it — and a project skill's doc is ordinary project content, which means the surface autofollows to Files. The move had already succeeded at that point, so the user got a working move that dumped them into the file tree. Moved outside the guard. The test now asserts it, alongside the flag release. * chore(ok): raise the combined-JS budget to fit the gitignored-skill dialog (PRD-7707) all JS chunks combined (gzipped) Size limit: 4700000 B Size: 4700364 B Over by 364 B — 0.008% — from the new dialog, its store, and five strings across thirteen locale catalogs. The budget had effectively no headroom, so this branch is just the change that happened to cross it. 4.7 -> 4.75 MB buys back room for the next few features rather than landing exactly on the line again. The main-bundle and CSS budgets are untouched and still comfortably under. * fix(ok): opening a skill must not eat the history entry it came from (PRD-7707) navigation-history.e2e.ts, failing on this branch since the sidebar click was routed through the shared opener: Expected: "#/__skills__" Received: "#/__asset__/nav-folder-.../nav-asset-....png" One entry too FEW. The shared opener always replaced the hash, so opening a skill from the Skills home overwrote #/__skills__ and Back skipped straight to whatever preceded it. The last green run on this branch predates that routing commit and the two runs in between were cancelled, so nothing reported it until these commits landed. History gets its own flag. `replaceActive` means "reuse the TAB" - it is the preview-tabs preference, on by default - and says nothing about navigation, so keying the hash off it (my first attempt) changed nothing. `replaceHistory` is set only where an open genuinely supersedes what the user is standing on: the preview tab becoming the real skill after an install. Also reverts two changes from this session aimed at an unconfirmed "first click does nothing" report - the removed same-row click guard, and installing the click handler in a layout effect. Neither was shown to fix that symptom, both touch this navigation path, and the guard is correct again anyway now that activePath requires a genuinely open tab. * style(ok): format the preview-redirect replaceHistory flag (PRD-7707) * fix(ok): clear the five pending review recommendations (PRD-7707) Carried since review rounds 17-19 and never picked up - they sit in review BODIES, and the status script only counts unresolved inline threads, so they never entered the work queue. convertLocations was the actionable one: it rm's each destination before materializing it, the same path install and runLocationWrite guard, but it alone ran unguarded - so a scan taken mid-convert reads the skill as absent and anything treating absence as deletion closes the user's tab. Now guarded. skill-install-ops: dropped the `??` fallback to effectiveInstallMode. installMode is required on that interface and the only call site resolves it beforehand, so the branch could never fire while making the service look like it defaulted. The caller owns resolution; the dep stays wired for the sibling service. Three error paths were documented in comments but pinned by nothing - stale beats blank on a failed refresh, the write flag released on a FAILED move, and the dialog surfacing a failed preview instead of disabling its only button. Tests now cover each. skill-projection: covered the canonical-host arm of convertCopies. Every existing test exercises the non-canonical arm; this one pins that the canonical stays a real directory rather than becoming a link to itself. * test(ok): assert the preview redirect's replaceHistory flag (PRD-7707) Both SkillPreviewTab dom suites pin the exact openSkill(...) arguments, so adding replaceHistory to that call site broke them. The flag is the point of the change - the post-install redirect supersedes the preview rather than stacking a second entry for the same skill - so the assertions gain it. * fix(ok): reconnect a dead pooled provider on open (PRD-7707) Opening a document in a window that had been sitting idle could hang on a blank editor for the full 30 second sync timeout. The provider pool's cache-hit path returned a pooled entry without checking whether its connection was still alive, so a provider whose socket had dropped was handed to syncPromise, which then waited for a synced event that could never arrive. The error boundary recycled the entry once the timeout fired and the same document loaded in about 300ms. Recycle a known-disconnected entry on the hit path instead, reusing the pool's existing recycle machinery. Gated on hasSynced and on the provider holding no unsynced edits, matching the debounced disconnect recycle: an entry still working through its first connect is waiting on a sync its own backoff will deliver, and this path has no buffer-and-replay. Verified against a packaged build by freezing the server long enough for the client to drop the socket, then reopening a warm document: the pool recycles and the document resolves in 315ms. * fix(ok): keep the Skills surface on bundle files and scope moves (PRD-7707) Two ways the Skills sidebar dropped the user into Files. Clicking a markdown file a skill ships beside its SKILL.md switched the sidebar to Files. The surface predicate admitted only SKILL and references/**, so a companion doc read as ordinary content. Bundles are free to ship companion markdown at their root, and skills on skills.sh do: tdd carries tests.md and mocking.md, and its SKILL.md links to both. Right after installing, with the bundle expanded, those rows are the obvious thing to click. Widened to any file inside a bundle, which is what the constant's own note reserves it for; the narrow parsers that feed the link index are untouched. Moving a skill between scopes landed on an unrelated document with the sidebar on Files whenever the only tab open for that skill was one of its bundle files, which is the normal state after clicking a skill and then one of its files, since the file replaces the SKILL preview tab. Three causes, all resting on the same assumption that a SKILL-level tab is always open: the tab reconciler could not see companion docs at all, so a move or delete stranded their tab on a removed doc; it closed a bundle-file tab on a scope move on the grounds that the SKILL tab would carry the move; and the move itself resolved its retarget source from the skills list before the open tabs, aiming at a name no tab carried, so it repointed nothing. The open tab now follows the skill to its new scope, a lone bundle-file tab is promoted rather than closed (one per skill, so no duplicate tabs), and companion docs are recognized by the reconciler. One existing expectation changed: a test asserted that a lone reference tab closes because the SKILL tab carries the move, while passing no SKILL tab. It now covers both arms. Reproduced and verified in a browser against the dev server, base versus fixed, and again in a packaged build. * test(ok): track the track-in-git route in both handler meta-tests (PRD-7707) `handleSkillTrackInGit` landed without an entry in the two meta-tests that enumerate every handler in the static route registry, so both reported it as untracked and integration shard2 has been red since the commit that added the route. Exempt in both. The handler appends a negation to the project `.gitignore` so a gitignored bundle becomes trackable: it rewrites a config file fs-direct, never a Y.Doc body, so the per-doc conflict gate does not apply and there is no authored content to attribute. Same posture as the sibling project-config skill handlers. GitOrigin-RevId: 3523232fc28dc6f8d94b35bb538ab57c53b6a68a
fix(ok): gate destructive MCP tools with ask, not deny (PRD-7891) (#3… …419) * fix(ok): gate destructive MCP tools with ask, not deny (PRD-7891) The docked terminal's auto-approve settings put delete, move, share_link, install, and import on Claude's permissions.deny list, on the assumption that deny outranks allow and would therefore force a prompt. A bare tool-name deny rule instead removes the tool from Claude's context entirely, so agents launched from the built-in terminal saw 16 of the 21 registered OpenKnowledge tools and could not rename or delete a document at all. Verified against a live MCP server: deny yields 16 tools, ask yields 21 and still prompts before the call runs. ask is the rule the original intent called for. It outranks allow the same way and keeps the tool callable. Reported at #1115. * fix(ok): update the remaining pinned deny-list copies The app-side docked-terminal launch test pins the full Claude --settings string, so it still asserted the deny shape and failed test:dom. The configuration reference carried the same "allow/deny list" sentence as editor.mdx and also omitted import from its gated-tool list, which predates this change but sits in the sentence being corrected. GitOrigin-RevId: aa0b2d55d460c3fa4679c9261bc08f9e5efc6bf8
Stabilize desktop theme and Windows chrome smoke (#3424) * fix desktop theme smoke synchronization * address desktop smoke review findings * fix desktop WCO tab reserve accounting GitOrigin-RevId: fe3506dea04bb9fbbaa97e21225f332af188981d
feat(ok): report platform and version on download + update telemetry … …(#3412) * feat(ok): report os and arch on download + update telemetry `app_update_downloaded` carried `artifact_type` but no platform dimension, so the auto-update base could not be split by OS at all and arch was invisible everywhere. Both are derivable from the artifact name the updater requests, so the update proxy now emits `os` (zip→macos, exe→windows, deb/rpm→linux) and `arch`, normalizing each packager's spelling — dpkg's `amd64`, rpm's `x86_64`/`aarch64` — onto the same vocabulary `download-targets.ts` already emits on `dmg_downloaded`. Unrecognized tokens drop out rather than reaching PostHog as new free-form values. `/download/beta` emitted no platform properties at all. It reports macos/arm64/dmg, which is what it actually serves: the beta resolver matches DMG_ASSET_NAME and nothing else, so there is no Windows or Linux beta to attribute. Hardcoded rather than read from `?os=` so a link asking for a platform the route cannot serve is not miscounted as one it did. `/download/stable` and the `/d/` share splash already emit the triple. * feat(ok): attribute Windows and Linux stable updates to a version Only the macOS zip embeds its version in the filename. The Windows and Linux installers are deliberately versionless permalinks that stable resolves through GitHub's `latest` alias, so the proxy had nothing to parse and every Windows/Linux stable update was recorded with a null `to_version` — 15 of 15 exe and 1 of 1 deb over the last 14 days, against 0 of 9548 for the mac zip. Beta escapes this only because it resolves the tag. Release-cohort and version-adoption views were macOS-only in effect. The updater now names the version it is installing in `x-ok-to-version`, and the proxy falls back to that header when it cannot derive the version itself. Server-derived sources still win: a filename version and a resolved beta tag both describe what the redirect actually serves, while the header is only the client's claim, so it cannot overwrite either. The header is validated against the same pattern as `x-ok-from-version` before reaching analytics. `requestHeaders` is read when `downloadUpdate()` runs rather than when the feed is configured, so the value is set in the `update-available` handler once the cross-channel veto has passed. That placement is safe only because `autoDownload = false` puts the download call below the handler instead of racing it, and it is guarded on the proxy feed so the GitHub fallback never grows an OpenKnowledge header. --------- GitOrigin-RevId: c08326f7cd9e24b3bec744cb366964dfb82582e7
feat(ok): re-point desktop attach to server.lock v2 (PRD-7903) (#3413) * feat(ok): attach desktop through the server.lock v2 one-URL contract (PRD-7903) Every desktop dial against a lock holder now derives from the single address the server advertises in server.lock: the keepalive WS and the branch-switch dialog proxies previously rebuilt ws://localhost:<port> and http://localhost:<port> from the port alone, which misses a server bound to a non-default loopback. Both now route through the shared lockApiOrigin/lockWsOrigin helpers (v2 url preferred, port fallback for pre-v2 locks), and the get-info and terminal-window collab URLs derive from the context's apiOrigin instead of rebuilding from the port. The packaged spawn drops the sibling-era --serve-content-assets flag; the bundled CLI serves content assets on every boot path by default and the spawned server is the canonical single-listener advertising url plus capabilities ["ui"]. The explicit --react-shell-dist-dir stays, since resolving the shell from inside app.asar is not guaranteed by the packaging layout. New end-to-end pins in the window-manager attach suite assert the v2 contract all the way through: the WS-upgrade probe and the renderer's injected --ok-api-origin/--ok-collab-url args derive from a ui-capable lock's url (including an ::1 bind), a pre-v2 lock keeps the localhost port fallback, and a non-loopback url is refused by validation. These pins are what make it provably safe for the later removal wave to retire the ok ui sibling and ui.lock: the desktop attach surface consumes only server.lock. Also retires the stale README description of the launch.json open-knowledge-ui rewrite, which no longer exists in the desktop. * fix(ok): address PR #3413 review — one collab-URL helper, wss pin (PRD-7903) Extracts httpOriginToWsOrigin as the single scheme-swap primitive and collabUrlFromApiOrigin as the shared http-origin-to-/collab derivation. lockWsOrigin and lockCollabUrl now delegate to them, and the two sites that inlined the replace on an already-resolved apiOrigin (the ok:project:get-info handler and resolveTerminalWindowProject) call the helper instead, so the untested IPC site inherits the tested behavior. Parity test gains the https-to-wss pin through lockCollabUrl (guards a future narrowing of the regex to /^http:/) and an equivalence pin that an apiOrigin-string consumer derives the same URL as the lock-shaped helper. Also refreshes resolveTerminalWindowProject's stale doc (the collab URL derives from apiOrigin now, not the port) and replaces the CORS injector's stale 'server binds 127.0.0.1 only' claim with the decision record: the filter deliberately stays at localhost/127.0.0.1 because pre-CORS CLI servers, the only servers the injector exists for, could only ever bind those addresses; a server new enough for a non-default loopback bind emits native CORS headers, so widening the filter would guard an unreachable version-skew combination. * fix(ok): drop the unused httpOriginToWsOrigin export (knip) (PRD-7903) The scheme-swap primitive stays as the single internal derivation point; external callers go through lockWsOrigin and collabUrlFromApiOrigin, which are the exported, tested surfaces. Un-exporting satisfies knip's unused-export gate without losing the one-primitive structure. GitOrigin-RevId: db17f4d0c10d393b5007a84ccc3a4ee50abb53e8
feat(ok): make --remote a thin alias over server.* keys (PRD-7902) (#… …3402) * feat(ok): make --remote a thin alias over server.* keys (PRD-7902) ok start --remote <url> now expands in the CLI to the ratified server.* networking keys: server.publicUrl set to the tunnel URL, server.allowExternal consent, and a loopback bind, flowing through the same resolution path as any other exposure route. The dedicated remote topology (resolveRemoteAccess, isRemoteAdmitted, RemoteConfigError, the legacyRemote arm of the ingress policy, BootServerOptions.enableRemote) is deleted; the surviving host-header and forwarding-header helpers move into ingress-policy.ts. Behavior over the tunnel is preserved: same Host and Origin admission, the stable remote port default, idle shutdown stays off, loopback bind coercion, and the https-only rule for the tunnel URL. One deliberate improvement: issued URLs (MCP serverUrl, preview_url) now name the tunnel URL instead of an unreachable loopback address, since the alias sets server.publicUrl through the successor key. The flag is deprecated and prints a notice pointing at --public-url, --bind, and OK_ALLOW_EXTERNAL; a new --public-url <url> flag sets server.publicUrl for a single run. Combining --single-file with --remote is now refused loudly (the legacy flow printed an enabled banner while never arming remote there). The remote.* config keys and the flag keep working through the deprecation window; removal ships separately. Tests: alias-equivalence (the expansion resolves an identical ServerRuntimeConfig to the explicit --public-url plus OK_ALLOW_EXTERNAL spelling), deprecation-notice and exit-code assertions, and the remote-mcp admission matrix now runs against the alias-shaped runtime as behavior-preservation pins. * test(ok): pin --remote alias self-consent and single deprecation notice (PRD-7902) Two verification-review gaps: a boot-level characterization that the alias shape (publicUrl + allowExternal on a loopback bind) boots outright with no OK_ALLOW_EXTERNAL in the environment (the flag historically WAS the consent, so an interlock refusal here would break every existing --remote user), and a strengthened deprecation-notice assertion (fires exactly once and names all three successors: --public-url, --bind, OK_ALLOW_EXTERNAL). * fix(ok): scope --remote self-consent to loopback binds (PRD-7902) Review finding (pullfrog): the alias granted server.allowExternal unconditionally, so ok start --remote <url> on a project with a non-loopback server.bind (a tailnet or LAN address coerceRemoteBindHost does not recognize) booted and served that network with no explicit consent — before the alias, the exposure interlock refused it with exit 78. scopeRemoteAliasConsentToBind now drops allowExternal from the alias overlay unless the effective bind is loopback-only, so the interlock refuses exactly as it did pre-alias, with a warning that names the fix (OK_ALLOW_EXTERNAL=1). Verified end-to-end against the built CLI: the non-loopback form exits 78, the loopback tunnel form still self-consents and serves the same admission matrix. Also from review: deprecation notice now leads with the migration command, the --single-file + --remote refusal explains itself, two stale resolveRemoteAccess comment references in core are updated, and remote-access.mdx gets a deprecation callout naming the successor invocation. * docs(ok): name OK_IDLE_SHUTDOWN=off in the --remote migration path (PRD-7902) Review round 2: the written migration command named only two of the three keys the alias pins. A migrator following it on a loopback bind got the derived 30m idle shutdown, and the idle timer counts editor WS connections only — the server would tear down under a live remote MCP client after 30 minutes. The deprecation notice, the remote-access.mdx callout, and the cli.mdx --public-url row now all carry OK_IDLE_SHUTDOWN=off with the why, and the notice test asserts the token. Also from review: document the server.* networking keys (bind, port, publicUrl, allowExternal, idleShutdown) in the configuration reference and mark remote.* as superseded; de-duplicate the trailing-slash normalization in expandRemoteAlias; move the expandRemoteAlias JSDoc back onto its function; pin the mixed loopback+non-loopback bind case for scopeRemoteAliasConsentToBind. GitOrigin-RevId: 5fdc6e5e629b7d9b0f213df1eb6714af39837d91
fix(ok): give new projects an initial commit (PRD-7310) (#3335) * fix(ok): give new projects an initial commit so worktrees work (PRD-7310) Setting up a project ran `git init` but never committed, which left the repository with an unborn HEAD: `main` pointed at nothing. Every main-relative command then failed. The most visible casualty was New worktree, which runs `git worktree add -b <name> <path> -- main` and died with `fatal: invalid reference: main`. `ensureProjectGit` now creates an empty root commit after it initializes a repo, so the default branch resolves. That function is the shared enforcement boundary for all four project-init paths (create-new-project, the pick-existing consent flow, the utility-process boot, and `ok init`), so no caller can forget it and no fifth caller will have to remember. The commit is empty on purpose. `ensureProjectGit` runs before any scaffolding, and three of its callers can target a folder that already holds the user's un-versioned files, so staging content would put a whole notes tree under version control in a commit nobody asked for. Only a repo this call initialized is touched, so a user's own unborn-HEAD repo is left alone. Several subsystems already carried defensive skips for the unborn-HEAD state (sync-engine push cycles, the server-factory drift check, project-skill-git, project-log). Fresh projects now take the normal path those skips were working around. Worktree create failures are legible now too. A repo with no commits gets its own `empty-repo` reason instead of the generic arm, and both worktree surfaces render git's own stderr, so an unrelated failure like a lock file stops reading as a bad branch name. * fix(ok): repair stranded unborn-HEAD repos, drop locale-bound identity match (PRD-7310) Review round on the initial-commit fix. Gate the root commit on "this repo holds zero refs" rather than "this call ran git init". The old gate was forward-only: every project shipped OK created before the fix kept its unborn HEAD forever, and nothing else backfills it, so the people who reported this bug would have stayed broken. A repo with no refs has nothing to lose and nothing for a root commit to be disjoint from, so adopting one is safe. Stranded projects now get their commit on next open, since ensureProjectGit already runs on the utility-process boot path. The same gate closes a second hole: the old `rev-parse --verify HEAD` probe only answered for the current branch, so a salvaged `.git/` carrying history on another branch with an unborn `main` would have been given a disjoint root. `hasAnyRef` blocks that. A filesystem probe runs first so an already-populated repo still invokes no git and never pays the preflight. Identity detection no longer reads git's message. Git localizes it (a German locale reports "Bitte geben Sie an, wer Sie sind"), so matching English phrases silently skipped the fallback for exactly the non-English users it exists for. `git var GIT_COMMITTER_IDENT` answers by exit code instead. The fallback stays conditional: `-c` overrides configured values rather than being inert, so supplying it unconditionally would misattribute commits for everyone who has an identity set. Tests: the no-identity case now sets `user.useConfigOnly` so git cannot auto-derive `user@host` and the fallback is genuinely exercised, with a companion test pinning that a configured identity is preferred. The old "does not commit into a repo it did not initialize" encoded a promise this change narrows on purpose; it is replaced by one test pinning the repair and one pinning the real bound, that a repo holding any history is never touched. `empty-repo` added to the exhaustive create-refusal enumeration. Two integration helpers globbed all of `refs/wip/`, conflating agent writer refs with the `git-upstream` import ref that exists now that fresh repos have history. Both filter to session writers via `parseWriterId`, mirroring `shadow-repo-stats.ts`. Their comments already said they meant agent writes. Drop `initialCommitCreated`: no caller read it, so its documented `false` state was unobservable. * fix(ok): ask one question about repo emptiness, and answer it the same way twice (PRD-7310) Second review round. `repoHasCommits` in the worktree path still used `rev-parse --verify HEAD`, the probe the server path had already moved off. That answers only for the CURRENT branch, so a repo whose history sits on another branch while the requested base is unborn was reported as `empty-repo` — telling the user "this project has no commits yet" about a repo full of them. It now asks `for-each-ref` like `hasAnyRef` does, and those failures fall to the generic arm, which carries git's own `invalid reference` text. Test added for that shape. `hasAnyRef`'s catch now logs. It is the guard the repair path turns on, so a transient `for-each-ref` failure would silently skip the backfill and strand the user in exactly the state this code exists to clear, with nothing to explain it. `hasRefsOnDisk` documented honestly: it reads only `refs/heads/` and `packed-refs`, so a repo carrying nothing but tags or remotes answers false. That costs one wasted spawn and nothing more, because `hasAnyRef` re-asks over the whole namespace before anything is written. The old comment implied coverage it does not have. Fallback identity is `[email protected]`, matching what the shadow repo already commits under, rather than inventing a second address for OK. GitOrigin-RevId: e572bff8750d1019f4de93444b3cd5ed575a1a08
feat(ok): make destructive confirmations undismissable (PRD-7907) (#3… …389) * feat(ok): make destructive confirmations undismissable (PRD-7907) The delete confirmations were built on Dialog, which dismisses on outside click, dismisses on Escape, and renders a close-X. That gave three ways to walk away from a destructive decision without answering it, one of which is a stray click. The most consequential case is the trash-failure fallback, whose primary action bypasses the OS Trash and unlinks the file outright. Adds components/ui/alert-dialog.tsx and moves DeleteConfirmationDialog and TrashFailureModal onto it, along with the six Dialog roots that render them. Radix AlertDialog gives role="alertdialog", blocks outside pointer dismissal, and moves initial focus to the cancel choice rather than the first focusable child. Escape is deliberately kept: it is the documented alertdialog keyboard contract and reads as Cancel. The accidental-dismissal risk is the pointer, not a keystroke someone chose to press. Hand-authored rather than installed. The registry component resolves here, unlike the spinner, but it omits the Electron titlebar drag opt-out that makes a dialog near the top of the window clickable, omits the reduced-motion opt-outs, has no body slot for the failure list, and styles its title with a token this theme does not define. The class strings therefore mirror dialog.tsx so migrated dialogs keep the proportions they have today. Both confirming actions stay plain Buttons rather than AlertDialogAction. Radix builds Action on Dialog.Close, so it would close the dialog the instant it is pressed, tearing down the in-flight spinner state these two report in the button's own label. The primitive's test pins that distinction so the next migration does not have to rediscover it. The audit scoped this as two call sites, meaning two components. Migrating them actually touches six Dialog roots, because each owner supplies its own root: FileTree owns three, and LintingSection, SkillDeleteDialog, SkillFileDeleteDialog and TemplateDeleteDialog own one each. One of FileTree's three is a convert-to-template confirmation reusing DeleteConfirmationDialog, which is destructive in the same way and comes along with it. Ten Playwright assertions queried these dialogs by role="dialog" and would have gone red on merge; they now query alertdialog. The one in show-ok-folders that presses Escape to dismiss is left as an end-to-end check that Escape still works. * fix(ok): suppress the alert-dialog overlay blur under reduced transparency (PRD-7907) The prefers-reduced-transparency block listed the dialog and sheet overlays but not the alert-dialog overlay, so destructive confirmations kept their backdrop blur for users who had the setting on. The guard test now names every overlay slot that opts into the blur. * test(ok): allowlist the alert-dialog overlay for the safe-area invariant (PRD-7907) The new AlertDialogOverlay is a fixed inset-0 backdrop, which the macOS traffic-light safe-area invariant flags by class pattern. It is a pure dimmer with no children, and the interactive footer lives in the centered AlertDialogContent, so it qualifies for the same exemption the Dialog and Sheet overlays already carry. GitOrigin-RevId: 3dcdd5e8dc8c0a21cc1bcdff2011039785ca80e0
PreviousNext