[slice-P1c] refactor: stop naming Studio inside core - #273
Merged
Conversation
…egister The shared knowledge store's three read paths each imported the studio capture module directly and emitted a literal source:'studio'. The discriminator in a shared store was a product name, so a second product could not register without editing core's query paths, response types and research-type allowlist. Core now holds a name, a key predicate, a search, a hydrate and an optional research policy. The persisted studio://<type>|<id> scheme is unchanged and stays the provider's business: it is already the key in the shared vector store and index_jobs for every artifact on disk.
…gistry cache.ts no longer imports the studio capture module and no longer emits a literal source:'studio' — the owning provider supplies both the key routing and the source label. CacheResultItem.source / FindSimilarResult.source become open: the closed 'cache'|'studio' union put a product name in core's response contract, so a second surface could not appear in a result without core being edited. Behaviour is unchanged: cache-studio-union.test.ts passes untouched.
…tifact registry find-similar.ts no longer imports the studio capture module. Both the provider-FTS list and the per-row embedding hydration take the source label from the owning provider, so the fourth ranked list is a generic artifact lane rather than a studio-shaped one. find-similar-studio-fts and find-similar-studio-leak pass untouched.
…the artifact provider
pipeline.ts had a literal STUDIO_RESEARCH_TYPES = {clip,qa,note} allowlist and
emitted engines:['studio']. Which artifact types carry citable prose is provider
policy, not a core-side allowlist — the literal set silently dropped any other
surface's types from research, and the engines label an agent reads named the
product rather than the surface.
The file now contains no occurrence of 'studio' at all. The three research
studio-source suites pass untouched.
Scoped to the three files whose CONTRACTS were leaking, not a repo-wide sweep: a blanket /studio/i rule over src/ cannot tell a contract leak from the legitimate cases, and the guard asserts that explicitly — src/cli/index.ts's subcommand and src/security/ssrf.ts's shared-infrastructure comments still carry the name and are deliberately outside the guarded set, as are the com.visualstudio.code and 'LM Studio' false positives. The behavioural half drives handleCache with a provider core has never heard of and asserts its rows surface under ITS id in both FTS and hybrid mode. Falsifiability probes: source:'studio' back in cache.ts reds 2 (one textual, one behavioural); a static studio import in find-similar.ts reds 2; STUDIO_RESEARCH_TYPES back in pipeline.ts reds 1.
…qliteBusyTimeoutMs The field is read by cache/db.ts on every connection to set SQLite busy_timeout: generic core behaviour carrying a product prefix because a concurrent host writer was the first thing to need it. WIGOLO_SQLITE_BUSY_TIMEOUT_MS is unchanged — env names are a user-facing surface. The persisted-settings key takes the new spelling and still reads the old one, so a ~/.wigolo/config.json that already carries studioBusyTimeoutMs is not silently reverted to the 5000 default.
… shared data dir Five modules independently spelled join(dataDir, 'studio', …) beneath ~/.wigolo, the shared data dir. With the segment repeated per call site nothing decides who owns what — the shape of the bug where a second product writes into the first product's directory. The path is UNCHANGED. Nothing on disk moves: an existing profile store, handoff ledger, escalation ledger, snapshot spill and session handle all keep resolving, and the tests assert the literal paths that were already in use.
'studio' was a hardcoded member of the Module union, so core had to know the
product existed for createLogger('studio') to typecheck. Core's own subsystems stay
enumerated — the labels still autocomplete and typos are still catchable — and the
type is open past that list so a surface core does not own can log without editing
core.
Behaviour is unchanged: createLogger('studio') still emits module:'studio'.
…med-table leak auto-launch spelled the sibling workspace two ways — a path-segment list for the existence probe and an npm -w argument for the spawn — which can drift apart silently. One const, next to the installedSubstrateExists seam the file already names as where distribution work lands. runner.ts records the product-named-table leak and why it stays: D15 locks migration names, and tool_audit already IS the bill for it (a session-less record could not reuse studio_audit's NOT-NULL FK, so D10 paid for a second audit table). Renaming a shipped table is a data-integrity bug on every machine that ran it.
…-set check-typecheck-gate.mjs requires any test importing a safety-critical studio module to be in tsconfig.test.json, so a removed or changed safety API fails the gate. paths.test.ts imports the handle path.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…eral
On Windows join() normalises separators, so `\tmp\...\studio`.startsWith('/tmp/...')
is false and the containment assertion became an assertion about path spelling.
Caught by lint + build + unit (windows-latest) on PR #273.
…every artifact The bootstrap held its specifier as an array element and iterated it into `await import(path)`. esbuild — which packaging/binary/bundle.mjs runs with bundle:true, format:'cjs' — cannot follow a non-literal specifier: it emits the call verbatim, does not inline the module, and does not warn. In the binary that resolved against dist/cli/agents/, so ../studio/… pointed at dist/cli/studio/… while the file sits at dist/studio/…. The bootstrap caught ENOENT, logged one warn, registered nothing, and every captured clip, qa and note silently vanished from cache, find_similar and research with no error reaching the agent. Binary-only: tsup sets bundle:false, so npm and source installs resolved fine and nothing in the normal matrix could see it. The loader list now holds thunks wrapping LITERAL specifiers, which esbuild does inline under the same flags, preserving the laziness. Coverage: bundle-provider-inlining.test.ts runs esbuild with the packaging flags and asserts the provider's code, its id and its type policy land in the output, and that no dynamic import or bare specifier survives. It asserts the property, not the current fix shape, and reds on both regression shapes. Also corrected three inaccurate claims: logger's union does not catch typos (it is open, and that is fine for a pure label); the nested envInt for sqliteBusyTimeoutMs read the env var twice with a dead branch; and the laziness does NOT defer better-sqlite3 on stdio, where server.ts -> session-target.ts -> capture/artifacts.ts -> cache/db.ts is a pre-existing static edge.
…ostic
The catch logged {error} alone after the move to thunks. That warn is the ENTIRE
signal for the silent-degradation mode this PR exists to fix, and it stayed
diagnosable only by luck: the old failure was a module-resolution error whose
message happened to embed the path. A provider that throws during module
evaluation produces an error naming nothing at all.
Logs loaderIndex + loaderCount rather than the specifier, because putting the path
back as a data string would defeat the bundling guard (which asserts no bare
specifier survives) and core's neutrality pin.
Covered by artifact-bootstrap-diagnostic.test.ts, which mocks a provider whose
export throws on read — an error mentioning nothing about the module — and asserts
the identifier is present, that the underlying error is still carried, and that the
bootstrap still degrades to [] rather than throwing. Removing the identifier reds 2
of its 3 tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stops core from naming Studio inside its own contracts. Base
fb16fb01.The target is the shape of bug BrowserOS shipped: a product name hardcoded in a shared layer, so the
second product scatters output into the first product's namespace with the wrong name visible to
callers. This slice closes that for the shared knowledge store, the shared data dir, core's response
types and core's logger union — and deliberately leaves the cases where a rename would cost data
integrity or would be pure cosmetics.
What changed
1. The
studio://scheme +source:'studio'/engines:['studio']in three core query paths.New
src/cache/artifact-registry.ts: anArtifactProvideris a name, anowns(key)predicate, asearchKeys, ahydrate, and an optionalisResearchablepolicy.src/studio/artifact-provider.tsregisters the studio side through a lazy in-tree bootstrap.
src/tools/cache.ts,src/search/find-similar.tsandsrc/research/pipeline.tsnow contain zero occurrences of theproduct name and import nothing from
src/studio/.The URI scheme is the provider's business — core asks
owns(key)rather than matching a prefix ithardcodes, because those keys are already persisted.
research/pipeline.tslost its literalSTUDIO_RESEARCH_TYPES = {clip,qa,note}allowlist; which types carry citable prose is now providerpolicy, so a second surface's types are no longer silently dropped from research.
CacheResultItem.source/FindSimilarResult.sourcewere closed unions containing'studio'— aproduct name in core's response contract. They are now open, documented as "any other value is a
registered provider id".
2.
studioBusyTimeoutMs->sqliteBusyTimeoutMs. Read bycache/db.tson every connection to setSQLite
busy_timeout: generic core behaviour wearing a product prefix.WIGOLO_SQLITE_BUSY_TIMEOUT_MSis unchanged (env names are a user-facing surface); thepersisted-settings key takes the new spelling and still reads the old one, both spellings tested.
3.
~/.wigolo/studio/— five inlinedjoin(dataDir, 'studio', …)call sites -> onestudioStateDir(). The path is unchanged; nothing on disk moves.5.
src/logger.ts—'studio'was a hardcoded member of theModuleunion, so core had to knowthe product existed for
createLogger('studio')to typecheck. Core's own subsystems stay enumerated asCoreModule(autocomplete and typo-catching preserved); the type is open past that list.6.
auto-launch.tsspelled the sibling workspace two ways (a path-segment list and annpm -wargument) — now one const, beside the
installedSubstrateExistsseam the file already names as wheredistribution work lands.
7. Product-named tables — documented, NOT renamed (D15).
runner.tsnow records the leak and thattool_auditalready is the bill for it: a session-less record could not reusestudio_audit'sNOT-NULL FK, so D10 paid for a second audit table. Renaming a shipped table is a data-integrity bug on
every machine that ran the migration.
Deferred, with reasons
host. Relocating them changes persisted-config keys (a user-facing breaking change) for no contract
benefit while the env names — the actual user surface — stay either way. Separate slice.
config.ts:4importing the origin-budget defaults from./studio/. Not independently fixable:the consts are legitimately owned by
src/studio/origin-budget.ts(its own fallback, the barrel,tests, the Electron host), so the options are duplicate the numbers (drift) or move a product policy
constant into core (wrong direction). A symptom of the 19-key ownership question; should move with it.
studio_open,studio_observe, …), locked by D15 and the conformance contract. Rewording "Requires an active studiosession (the human runs
wigolo studio)" removes nothing whilestudio_*sits in the same sentence,and
wigolo studiois a real command the agent must be able to name. The descriptions already usecapability language for implementation ("shared browser workspace", never Playwright/CDP/Electron).
Cosmetic, no seam consequence — skipped per the scope-discipline instruction.
Data integrity
studio://keys: nothing migrates, nothing is orphaned. The persisted scheme is byte-identical —the key already written into
studio_artifacts, the vector store andindex_jobsfor every artifact ondisk. What moved is ownership: core asks
provider.owns(key)instead of matching a prefix, so thestring lives in
src/studio/artifact-provider.tsand core contains neither the prefix nor the label.The scheme was NOT renamed even with a dual-read, for a concrete reason: a mixed corpus reintroduces a
dedup bug. One artifact indexed in the vector store as
studio://clip|42but returned by FTS under anew scheme fuses as two RRF entries for one row. Correcting that needs canonicalisation at every key
comparison — real bug risk for a value an LLM reads as prose.
source/engineswas left equal to theprovider id for the same coherence reason: emitting
source: 'capture'besideurl: 'studio://clip|42'is worse than either consistent choice. Yours to call — a neutral scheme needs a key-canonicalisation
plan attached, not a prefix swap.
~/.wigolo/studio/files: nothing moves.studioStateDir()resolves to the identical path;tests/unit/studio/paths.test.tsasserts the literal pre-existing paths, so an accidental relocationreds. Existing profile store, auth-origin ledger, escalation counters, snapshot spill and the 0600
handle all keep resolving.
Instructions byte budget
Untouched, so it still holds: 3496 / 3900, headroom 404. Not raised.
UNTRUSTED_STUDIO_NOTICE— rename request for P2src/security/untrusted.tsis P2's file, so this is a request, not an edit. The const name isproduct-scoped while its value is already generic and names no product — so no wire change.
"Perception" is what it actually scopes: the notice accompanies structured page-perception results
(elements / diff / marks) which cannot be string-fenced without breaking the agent's structured reads.
Call sites:
src/studio/observe.ts(x4),src/studio/index.ts:160(barrel re-export),src/cli/studio.ts(x4).Verification
CI: 19 pass / 2 cancel. The two non-passing jobs (
full test suite (ubuntu),lint + build + unit (ubuntu-latest)) are the same two cancelled on basefb16fb01— the bounded ubuntu hang that basecommit is about; their logs end in
Terminate orphan process: npm run test:unit, not an assertion.lint + build + unit (windows-latest)failed once on a path-separator bug in my own new test and isgreen after
d314b88a.tsc --noEmitgate:studiopaths.test.ts, required bycheck-typecheck-gate.mjs)typecheck:debtnpm testHonest caveat on the local core suite: two full runs disagreed (1 failed, then 19 failed) on code
differing only by a Windows path assertion in a studio test. All 19 pass standalone AND as a 9-file
group. This worktree sits inside the shared checkout, so a dotenv parent-walk reaches the real key in
/…/wigolo/.env(the known pre-existing LLM contamination), plus port and model-cache contention fromother agents on the machine. CI runs the same suite clean on macOS and Windows and passes. No green
local full-suite number is claimed.
Measured, not inferred
ensureArtifactProviders()costs 15.62ms first call, 0.001ms cached, 1.8MB RSS — measured in aprocess with
cache/store.jsandembedding/embed.jsalready loaded, i.e. the marginal cost againstthe warm graph a real
find_similarrequest runs in. ~4000x too small to explain the 60srest-toolstimeout seen once, which classifies that as load flake.
Correction to an earlier claim of mine: I said
better-sqlite3was type-only and erased on thischain. Only line 1 of
capture/artifacts.tsis. Line 5 pullsgetDatabasefromcache/db.js, whoseline 3 is a VALUE import of
better-sqlite3; ESM evaluates eagerly, and the probe confirmedbetter_sqlite3_is_value=true. The Electron 43 constraint is unchanged and the DB broker is notretirable on this evidence — the probe ran on plain Node and establishes nothing about Electron.
Falsifiability probes
Every guard proven able to fail, then reverted by reverse-edit:
source: 'studio'back incache.ts../studio/capture/artifacts.jsimport infind-similar.tsSTUDIO_RESEARCH_TYPESliteral back inpipeline.tsconfig.tsstudio'studio'back in the loggerCoreModuleunionThe neutrality guard is scoped to the three files whose contracts leaked and asserts its own scoping:
one negative test checks that
src/cli/index.ts's subcommand andsrc/security/ssrf.ts'sshared-infrastructure comments do still contain the name and are deliberately outside the guarded
set; another checks that
com.visualstudio.codeand "LM Studio" / "Google AI Studio" are not policed.A blanket
/studio/isweep oversrc/cannot make those distinctions — which is why it isn't one.Found, not asked about
src/studio/synthesize.ts:45emits a second, incompatible shape under the same scheme:studio://artifact/<id>, where the capture pipeline's isstudio://<type>|<id>.owns()claims it byprefix, but
hydrate()returns null (no|separator), so such a key is silently skipped rather thantreated as a non-artifact url. Pre-existing and fail-safe — unchanged by this PR — but the scheme has
two producers that disagree, worth fixing before anything relies on round-tripping a brief citation back
into the store.
apps/studio/src/main/cdp-fence.tswas left alone, as instructed.