Repository navigation
fix(security): don't let one undecryptable row abort the secrets listing - #621
Closed
Rushikeshiitb wants to merge 31 commits into
Closed
Rushikeshiitb wants to merge 31 commits into
Rushikeshiitb wants to merge 31 commits into
Conversation
Contributor
|
@claude review the current head against dev for correctness, security, compatibility and regression coverage. Please distinguish PR defects from shared-base or infrastructure blockers. |
|
Claude finished @kshivang's task in 6m 17s —— View job Review: resilient secret listing
The server-side change is correct and the restatements are faithful - I diffed all three function bodies against their stated sources and the only differences are the two intended decrypt calls (plus duplicated PR defects1. Blocking: a NULL password still destroys the whole page, now in the client
val password: String, // Decrypted on clientnon-nullable and with no default.
So after this migration a single corrupt row returns This is a PR defect rather than a pre-existing one: Minimum fix in this repo: make 2.
|
This was referenced Sep 13, 2026
Contributor
|
@Rushikeshiitb Review handoff: validation infrastructure is the remaining blocker, not an identified unresolved author defect. Your per-row decryption recovery addresses a real whole-page outage. The original helper/RPC work remains credited to you. I pushed a fix for the existing desktop non-null password contract, sanitized diagnostics, internal-helper grants/audit coverage and additional SQL/client regressions. Validated repair head: The current-head build https://github.com/risa-labs-inc/BossConsole/actions/runs/34736948893 remains queued with no runner steps after the bounded wait. Earlier green checks were for the original head and do not validate these repairs. Required local Gradle validation was also blocked waiting for the shared build lock (zero Gradle output); I canceled only my queued commands after the wait. A final gate attempt under the required dev-merge lock also could not acquire that lock and was canceled without executing a merge. No required check was bypassed, and no live database or app instance was touched. Next steps for maintainers/author: allow or restore CI runner capacity, run the current-head platform/compatibility/database/quality checks, and run the focused desktop tests where applicable plus Per the user's final disposition policy, this unresolved validation handoff is being marked draft with all pushed fixes and contributor credit preserved. No merge or auto-merge was performed. |
kshivang
marked this pull request as draft
September 13, 2026 04:14
…c#547 three times (risa-labs-inc#596) * test(db): gate the two client-grant shapes that produced risa-labs-inc#547 three times `ALTER DEFAULT PRIVILEGES` in 20251023000014_grants.sql hands every later table client grants, and three tables then shipped with a permissive write policy on top of that: plugin_downloads (risa-labs-inc#488), completed_authentications (risa-labs-inc#536) and secret_access_log / plugin_api_key_logs (risa-labs-inc#538). Each was caught by a person reading a migration. identity-disclosure audit. That audit asks who can READ identity. Neither it nor the default-privileges change reaches the shape above: a default-privilege revoke is not retroactive, CHECK 3 filters `cmd in ('SELECT','ALL')` so an INSERT policy is invisible to it, and CHECK 3b filters `not relrowsecurity` while both risa-labs-inc#538 tables had RLS enabled with a permissive policy. Adds supabase/audit/client_grant_audit.sql with two exact catalog checks: * G1 names any public table a client role can reach without RLS. No allowlist, because all 34 tables in the schema enable RLS today. * G2 names any INSERT/UPDATE/DELETE/ALL policy reachable by anon, authenticated or PUBLIC whose predicate is the literal `true`. G2 keys on the predicate, not on a missing `TO` clause. The missing `TO` is how all three slipped past review, but 101 of the 128 policies in this schema omit it, so gating on that would report most of the schema. Predicate plus write plus client role matches two policies in the whole schema, and both are the ones risa-labs-inc#538 removes. G2 is asserted as a subset rather than as HEALTHY while those two are still on dev, so the gate passes before and after risa-labs-inc#538 and still fails on anything new. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(db): exclude restrictive policies from write lint * docs(db): state audit limits and cross-suite regression coverage * docs(db): name the exact companion policy assertion --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Shivang <shivang.iitk@gmail.com>
…sa-labs-inc#484) * fix(browser): actually warn before downloading an executable file warnForExecutables defaults to true and its KDoc promises a warning before a recognized executable finishes downloading, but the branch that was supposed to show it was an empty TODO block ("for now, just proceed") - so the download always proceeded silently regardless of the setting, for every user, with no way to turn the (nonexistent) warning off since there is no Settings UI for it either. confirmExecutableDownload mirrors pickSaveFile's own pattern exactly, since both are called from the same synchronous StartDownloadCallback and both need a blocking answer before it returns: a Swing confirm dialog run via SwingUtilities.invokeAndWait, failing closed (treated as cancel) if the dialog itself cannot be shown, matching how a null save path from pickSaveFile already cancels the same callback. Declining releases the claimed save path and cancels the download the same way the existing parent-directory-failure bail does, just above it in the same callback. * fix(browser): close executable consent and cancellation gaps * fix(browser): bind download consent to a usable window and explicit choice * feat(browser): add a user-facing toggle for the executable download warning Settings > Browser > Downloads > "Warn before downloading executable files", on by default. BrowserSettings.warnForExecutables is persisted through the existing BrowserSettingsManager JSON store alongside the other browser toggles, and FluckEngine reads it live at download time instead of the unpersisted DownloadSettings copy, so a change applies to the next download without restarting BOSS. Disabling it skips only the executable warning; filename checks, save-path handling and cancellation cleanup are unchanged. Addresses the maintainer's follow-up on risa-labs-inc#484. * fix(settings): index the executable-download-warning toggle for search SettingsSearchIndexDriftTest failed CI on all three platforms: the new "Warn before downloading executable files" toggle (Settings > Browser > Downloads) had no entry in SettingsSearchIndex, so Settings search could not find it. Added a Downloads group under the FLUCK section, extracted into its own downloadsEntries() helper alongside the existing fileAndLinkEntries() to keep browserEntries() under detekt's LongMethod threshold. * fix(browser): pin the warning toggle's persistence and volatility - BrowserSettingsManager: settingsFile is now an internal var with a reloadForTest() seam (same pattern as DefaultAppsSettingsManager), so the persisted-value tests exercise the real saveSettings/load path instead of re-stating the assignment; covers save+reload round-trips, a file predating the key, and a corrupt file - ExecutableDownloadConsentTest: the live-read test re-implemented the call site, so it now pins the StartDownloadCallback argument textually (SettingsSearchIndexDriftTest idiom) - a revert to the unpersisted downloadSettings copy now fails the build - BrowserSettings.warnForExecutables is @volatile: written on the Compose UI thread, read on the JxBrowser download callback thread - DownloadSettings.warnForExecutables removed: no reader left it; the live flag is BrowserSettings.warnForExecutables * test(browser): make the persistence tests measure the reload, not the assignment Addressing the second Claude review of 70a4874: - default test asserted process-global state that the test's own @beforeeach causes to be loaded from the developer's real settings file - it would pass in CI and fail on a machine with the toggle off. It now pins the persisted shape's default; the in-memory direction is carried by the predating-key reload test. - the round-trip test moved the in-memory value away between save and reload, so deleting the load-apply in BrowserSettingsManager now fails it. - the whole persisted shape (all eleven fields, including the four system-property mirrors) is snapshotted in @beforeeach - after the first access that may trigger the manager's real-file load - and restored in @AfterEach, since reloadForTest() rewrites all of them. - reloadForTest KDoc states the deliberate no-reset divergence from DefaultAppsSettingsManager.resetForTest. * fix(browser): test the download dialog's owner resolution and surface it R1: the dialog-owner resolution (active window, else a fallback, else refuse) was the only new decision in the executable-download warning with no test - the branch that silently cancels a user's download had nothing pinning it. Extracted pickDialogOwner(activeWindow, candidates), mirroring resolveActionableWindowIdFrom's own pure-function split, and added cases covering active-window-wins, non-displayable-active falling back, and no-usable-owner refusing. R2: the fallback resolved exactly one window id and refused outright if it wasn't displayable, even when another registered window was open - reachable in the interval where a window has been disposed but its EDT unregister hasn't run yet. pickDialogOwner now takes an ordered list of candidates and scans all of them; WindowFocusManager. candidateWindowsForDialogOwner() supplies that list (the resolved fallback window first, then every other registered window), covered by a test where the first candidate is stale and a later one is usable. R3: nothing called toFront()/requestFocus() on the resolved owner, so a background BOSS could still show this modal somewhere the operator doesn't see, with the JxBrowser callback thread waiting behind it. Already running on the EDT inside confirmExecutableDownloadOnEdt's own Runnable, so this calls AWT directly rather than needing invokeLater. * fix(review): preserve current release metadata in download consent PR * fix(review): tighten executable consent cleanup and default coverage --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>
Co-authored-by: sgoel2be24-cyber <223222024+sgoel2be24-cyber@users.noreply.github.com>
… plugin's own id (risa-labs-inc#548) * fix(plugins): scope PluginStorageFactory.createStorage to the calling plugin's own id TrackingPluginContext (the only PluginContext a plugin's own code ever holds - every registration path wraps createSandboxedContext in one before calling register()) exposed the raw, unscoped PluginStorageFactory straight through from its delegate. That factory takes an arbitrary pluginId per call and the desktop implementation persists purely off that string (~/.boss/plugin-data/{pluginId}/storage.properties), with no check anywhere that it names the caller. Any installed plugin could reach another plugin's persisted data with nothing more than: context.pluginStorageFactory?.createStorage("victim-plugin-id") - read, overwrite, or clear() it. Added ScopedPluginStorageFactory, which binds createStorage to the context's own pluginId (a construction-time value the plugin does not control) and ignores whatever id the caller passes in - the same pattern DownloadCenterProviderImpl.forPlugin already uses to keep one plugin's transfer ids from colliding with or addressing another's. SandboxedPluginContext is untouched: it already passed downloadCenterProvider through unscoped in the same way, since TrackingPluginContext wraps it and is the only context a plugin ever sees - matching that existing, working precedent rather than adding a second, redundant scoping layer. Added tests/TrackingPluginContextStorageScopeTest.kt: the underlying factory is only ever asked for the context's own plugin id regardless of what a caller requests, and a write made through one plugin's storage reference never lands in another plugin's real storage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(plugins): model shared storage in isolation regression * test(plugins): assert scoped provider identity and document boundary --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Shivang <shivang.iitk@gmail.com>
…o JS (risa-labs-inc#549) Validate remote co-browse control objects before embedding them in page JavaScript. Reject unquoted expressions recursively and preserve valid control messages and escaped string data. Original diagnosis, object guard and six regression tests by Antriksh. Prior review repair adds recursive token validation and four regressions; contributor replayed all three commits cleanly onto dev. All ten hosted checks and ten focused tests pass. Co-authored-by: Antriksh <antrikshsingh850@gmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…and Download bridges (risa-labs-inc#550) * fix(kernel): require verified caller identity for the Supabase proxy and Download bridges SupabaseServiceBridge is a generic, unrestricted Postgrest proxy backed by the signed-in user's own RLS-scoped Supabase session - its rpc() reaches the same create_secret/update_secret RPCs SecretServiceBridge exists to protect, and select() can read any table RLS exposes. Neither select nor rpc checked caller identity (BossConsole#53), so any local process reaching the kernel's IPC socket had the same reach as a legitimately installed plugin, attributable to nobody. Both now require a verified process identity, matching SecretServiceBridge's established pattern, with audit logging on every call. DownloadServiceBridge.openFile/revealInFolder shell out to the OS's own "open with default application" command for any path that exists on disk, with no check that the path has anything to do with a download - before this, an unauthenticated caller on the IPC bridge could make BOSS execute any file on disk via the OS default-handler mechanism, completely bypassing BossConsole#484's executable-download-warning consent dialog (that path never goes through it at all). Every RPC on the bridge now requires a verified caller identity, and openFile/revealInFolder are additionally confined to a path this provider's own downloads list is currently tracking - canonical-path compared, so a symlink or a `..` cannot walk outside it - turning "open any file that exists" into "open a file BOSS itself downloaded". Both bridges get real gRPC-server-and-client tests (matching PluginUIServiceBridgeTest's pattern): an anonymous caller is refused with PERMISSION_DENIED on every RPC, the underlying provider is asserted never invoked on a refusal, and DownloadServiceBridge additionally covers an authenticated caller naming a path outside the tracked set. * fix(kernel): revalidate download streams and use checked paths * fix(kernel): capture stream identity and reject empty download paths * style(tests): align download regression expression wrapping * fix(kernel): clear code-quality on the download/supabase caller-identity branch detekt's default MaxLineLength (120) rejected a 128-char assertEquals in DownloadServiceBridgeTest that ktlint's wider 140-char .editorconfig limit had let through - wrap it. Also finishes the finding-6 KDoc reword the bot review flagged as ungrammatical ("this is the path does not provide" -> "this path provides no executable-consent step") and reflows that paragraph. No behavior change; addresses the last remaining code-quality blocker noted in the PR's review thread. --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>
…ry mode (risa-labs-inc#551) (risa-labs-inc#552) * fix(plugins): give a plugin's file panel an owner and its own directory mode (risa-labs-inc#551) FilePickerProviderFactory created every panel as FileDialog(null as Frame?, ...) and showed it with a bare isVisible = true. Two consequences, and they are different failures. No owner. The panel has no window to be modal to and none to come forward over, so it can open behind the Compose window. onResult(null) then reaches the plugin looking exactly like a cancel. No directory mode. apple.awt.fileDialogForDirectories is process-wide and read by the native peer at creation, and a modal panel runs a nested event loop on the EDT that keeps dispatching. NativeFileDialogs already recorded this file as one of three that create a LOAD dialog without stating the flag, and the consequence it named: such a panel opened inside the browser's folder panel comes up as a directory chooser. That part is a defect independent of the reported symptom. NativeFileDialogs had already solved both for the browser's own panels, and its comment says the durable fix is to route every FileDialog in the tree through one helper. This extracts that helper rather than adding a third copy of it. NativeFileDialogs now delegates to it and loses its private duplicates, so the two cannot drift. The property discipline is split out as withDirectoryMode so it is testable without a display: an absent property is cleared rather than written back as "false", a present one is restored verbatim, nesting unwinds in reverse, and a throwing body still restores. Seven tests cover it. The owner selection and the panel itself are not covered and cannot be from a headless test. The catch blocks now log at error rather than warn and say that the plugin will see the failure as a user cancel, since onResult(null) is the only channel this API has and the log is the only place the difference exists. 570 tests in the browser package pass, including the existing NativeFileDialogs suite. ktlint and detekt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(plugins): preserve save filters from PR 611 Consolidate Jaspreet Singh’s save-filter addition into arjun28115’s shared owned-dialog implementation. Remove comments left behind by the helper extraction. Co-authored-by: Jaspreet Singh <jaspreetsinghupkar@gmail.com> * test(pickers): preserve pre-existing directory flag in teardown * fix(pickers): deliver one result per pick, and accept a dotted extension The two shared-provider defects handed back on risa-labs-inc#552. Neither came from the ownership change; both live in the picker this PR touches, and both are silent. 1. `onResult` was called inside the same `try` that catches picker failures, so a plugin whose callback threw received its path and then a `null` immediately after. `null` is indistinguishable from a cancel, so one pick produced two answers and the second one was wrong. Delivery now goes through `deliverOnce`, which sets its flag before invoking, so a throw from the plugin's callback cannot be retried by the catch. The two throws are told apart by when they happen: a picker failure arrives before delivery and still reports the cancel. 2. The API takes `List<String>` and says nothing about the spelling, so plugins pass both `csv` and `.csv`, and every consumer appended its own dot. The macOS filter tested `name.endsWith(".$ext")` and Swing's FileNameExtensionFilter adds the dot itself, so `.csv` was matched as `..csv`. No file is named that, so the filter hid every valid choice instead of narrowing to them. `normalizedExtensions` strips a leading dot and drops blanks, and both platforms and both methods now route through it. Normalising rather than declaring a bare-extension contract, because the contract is not checkable at the plugin boundary and the failure it produces is a panel that lists nothing, which reads as "no matching files here". Both fixes are in Open and Save. The Save path also gains a comment saying what its filter does NOT do: a native SAVE panel narrows what is listed and does not constrain what the user types, so the returned path may still carry any extension or none. The three helpers are `internal` and pure so the rules are testable without an AWT panel on the EDT. The provider calls exactly these, so reverting a call site is a compile error rather than a silently unexercised test. 11 tests cover one delivery per pick, a throwing callback, a failure before delivery, csv/.csv, mixed case, whitespace, blank-only and empty filter lists. ktlint 1.8.0 clean; no line over 120 columns. No local test run is claimed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(pickers): repair what CI caught on the rebased head Two failures, both mine, neither caused by the rebase. `compileTestKotlinDesktop` failed at FilePickerDeliveryTest:34. `assertEquals` takes one type parameter for both arguments, and `listOf("...")` against a `MutableList<String?>` gives it nothing to unify, so inference failed outright. The expectation is now spelled `listOf<String?>(...)`, matching the two assertions further down that already were. The `emptyList()` expectations are spelled `emptyList<String>()` for the same reason, before they hit it too. `detekt` failed on SpreadOperator at both FileNameExtensionFilter constructions. The rule is not new and the code is not either: renaming `filters` to `extensions` moved the element out of whatever suppressed it before. Suppressed at the two functions with the reasoning written out, matching what NativeFileDialogs.onOpenFiles in this same PR already does. The spread is over a plugin's declared filter list, a handful of entries, and FileNameExtensionFilter is Java varargs with no collection overload to prefer. Worth saying plainly: a type error is something a compile would have caught and I shipped it without one. There is no working Gradle path on this machine, so ktlint is the only gate I can run locally, and it does not type-check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Shivang <shivang.iitk@gmail.com> Co-authored-by: Jaspreet Singh <jaspreetsinghupkar@gmail.com>
…isa-labs-inc#542) * feat(mcp): trust a whole plugin's tools at once, not one at a time Adds a "Trust This Plugin" option to the MCP tool approval dialog - a persisted, provider-wide ALLOW covering every tool a plugin contributes, not just the one call being approved. Answers the actual friction: a plugin with several mutating tools currently means approving each one individually, in the same session, one at a time. McpToolPolicyConfig gains a second rule map, providerRules, keyed by providerId rather than tool name. policyFor(toolName, providerId) now resolves both, with DENY - at either level - always winning first and unconditionally: a provider-wide DENY is not "most specific wins" and must not be overridable by a narrower per-tool ALLOW, the same posture DENY already has against session trust everywhere else in this engine. Below that: session trust, then an explicit tool-specific rule (ALLOW or ASK), then the provider's own ALLOW, then the risk-based default. providerId is an optional parameter so an existing caller that only ever checked a tool name keeps compiling. setToolPolicy, the new setProviderPolicy and revokeProviderPolicy now share one persistAndPublish helper for the persist-and-fault-report shape, so a future change to how a write failure is logged or reported cannot update one path and silently miss the others - the mistake a similar policy engine made once already by not sharing this logic. revokeProviderPolicy removes the key rather than rewriting it to some neutral value, for the same reason. McpApprovalDecision.Approved gains a trustProvider flag alongside the existing trustForSession one. Persisting it is a synchronous disk write, so it runs under withContext(Dispatchers.IO) rather than on whatever dispatcher delivered the MCP call - and a failed persist doesn't block the call the operator already approved past; it falls back to session trust for that one tool instead; a new PROVIDER_TRUST_PERSIST_FAILED ledger disposition records that the durable grant did not save. The new button sits in its own row below the main actions, not next to "Approve Once" - it is the broadest grant this dialog can make, so it should take a deliberate reach rather than risk a fast click landing on it by mistake. A caption names the scope. Revocation is a new McpProviderTrustDialog (list + per-row revoke, mirroring the shape an equivalent per-tool dialog would need) reachable from a "Trusted plugins (n)" button in the bottom bar, next to the existing session- trust revoke button - only shown once a provider is actually trusted. Test plan: McpProviderPolicyTest pins policyFor's full precedence table (provider ALLOW/DENY against tool rules, session trust, the policy-unreadable fault) and persistence (round-trips through a real file; revoke removes the key, survives reload; revoking one provider leaves another's trust untouched). McpGovernedInvocationTest adds two integration cases through the real registry: approving one tool with trustProvider lets a sibling tool from the same provider through with no second prompt and no session-trust call, and revoking provider trust makes its tools ask again. No Compose-level test for the dialogs themselves - no test infrastructure exists yet for this UI layer on this branch (no ScrimmedModalSemanticsTest-style harness), noted here rather than left unstated. Verified: full :composeApp:allTests passes except the two pre-existing, environment-specific Windows symlink-privilege failures already known from earlier this week (DesktopFileScannerTest, DiscardDownloadContainmentTest). ktlintCheck/detekt clean. * test(mcp): pin the provider-trust persist-failure fallback A failed provider-wide persist must still run the already-approved call, record PROVIDER_TRUST_PERSIST_FAILED, session-trust only that one tool, and leave the provider's other tools asking. Mirrors the blocked-write setup the per-tool failure case in McpPersistentApprovalTest uses. Agent-side regression coverage added during sweep BOSS_SWEEP_20260911_mcp-policy; not part of the author's submission. * fix(mcp): guard provider-wide trust writes against a stale approval dialog trustProvider ("Trust This Plugin") called setProviderPolicy directly, with none of the revocation/DENY/availability guards validateApproval already applies before setToolPolicy's persisted write. A revoke or DENY landing while the dialog was open (e.g. an operator resetting the tool from "Persisted MCP policies") could be silently overwritten: the call in hand was correctly withheld, but the provider-wide ALLOW still persisted, leaving every other tool that provider contributes AUTO_ALLOWED from an authorization the reset was supposed to invalidate. setProviderPolicy now takes the same preserveDeny/expectedRevocation guards setToolPolicy has, checked under the same lock, plus an optional toolName since the revocation check is inherently per-tool. The trustProvider branch in approvedAuthorization pre-checks isAvailable/revocationVersion/policyFor before writing (mirroring validateApproval exactly) and, on a false return, disambiguates a guard refusal (POLICY_DENIED, call does not run) from a genuine disk failure (PROVIDER_TRUST_PERSIST_FAILED, call still runs - the existing, deliberate fallback for that case). New regression test in McpGovernedInvocationTest reproduces the review's exact scenario (a revokePersistedPolicy landing while a prompt is open) and confirms it fails without the fix. AGENTS.md's governed-MCP section now documents provider-wide trust as its own scope, including the deliberate asymmetry between a stale-dialog refusal and a disk-failure fallback. * fix(mcp): invalidate queued grants when provider trust is reset * fix(mcp): clear code-quality on the provider-scoped-policy branch detekt flagged setProviderPolicy's preserveDeny guard as ComplexCondition (4 boolean/comparison operators inline) and a 121-char assertEquals in McpProviderRevocationTest as MaxLineLength (over detekt's default 120, though under ktlint's wider 140-char .editorconfig limit). Extract the DENY check into isProviderOrToolDenied() - same behavior, named and testable on its own - and wrap the long assertEquals. No behavior change. --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>
…macOS (risa-labs-inc#553) (risa-labs-inc#559) Make explicit Ctrl bindings fire on Control off macOS, with shared capture/matching rules and platform-aware chord identity for conflict detection and migration. Preserve distinct macOS modifiers and reject unsupported Super capture. Original implementation and contributor follow-up by @arjun28115. Earlier review-agent work integrated dev, rejected unsupported Super captures, and corrected comments; those repairs are separately credited in the PR description. Closes risa-labs-inc#553.
…-inc#560) (risa-labs-inc#561) Restore iconified frames before focusing through either WindowFocusManager entry point, preserving maximized state. Original shared helper and headless mask tests by @arjun28115 (risa-labs-inc#561). Independent equivalent implementation and missing-target regression by @Sahvendra7 (risa-labs-inc#580), retained through consolidation. Earlier review-agent work integrated that regression and clarified restore ordering and documentation. Closes risa-labs-inc#560. Co-authored-by: Sahvendra <shyamajanidhaka@gmail.com>
* fix(cli): prevent readiness queue lost commands * fix(cli): close initialization queue race and guard deferred logging --------- Co-authored-by: ayush <workside@gamil.com> Co-authored-by: Shivang <shivang.iitk@gmail.com>
Rushikeshiitb
force-pushed
the
fix/secret-listing-resilient-decrypt
branch
from
September 14, 2026 08:15
2ab3f4c to
77ad33f
Compare
Contributor
Author
* Stop anon reaching schema public by inheritance, and scope identity to real orgs
The BOSS Supabase anon key is compiled into this repo, which is public. Anything
`anon` can execute is therefore executable by anyone. Verified on 2026-09-08:
* get_encryption_key() returned the Vault master encryption key, unauthenticated.
decrypt_text() was anon-callable beside it, making a decryption oracle over
everything encrypt_text protects - user secrets and the QBO/Google broker
credentials.
* find_user_by_email() confirmed any address and returned its uuid.
* list_shareable_recipients() returned 152 users WITH full email addresses to
any self-registered account, and 82 @risalabs.ai addresses on a search.
* arcade_leaderboard() and arcade_bs_standings() published the roster of
everyone who had opened the Arcade, unauthenticated.
None of it was granted on purpose. PostgreSQL hardwires EXECUTE to PUBLIC on
every new function, PUBLIC includes anon, and this project's default privileges
add anon on top - so a SECURITY DEFINER function is internet-callable from the
moment it is created. Both revokes are traps in mirror image: `from public`
leaves the explicit anon grant, `from anon` leaves the hardwired PUBLIC one, and
ALTER DEFAULT PRIVILEGES cannot remove the latter at all (pg_default_acl loses
the entry and new functions still get `=X`).
20260908000000 event trigger: revoke PUBLIC+anon on every function created in
schema public. An explicit `grant ... to anon` after the create
still wins - anon access must be stated, never inherited.
20260908010000 org_visible_users / org_is_vetted / user_display_name: ONE
definition of "may this account learn who that account is",
shared by the Arcade and poker rather than copied. Vetted means
a human approved the join, which excludes the catch-all `boss`
org every account joins on signup (153 members, 20 domains).
20260908020000 route list_shareable_recipients through both. It was already
org-scoped, which is why it looked right; it accepted ANY
shared org.
20260908030000 one-time sweep of the ~30 functions that already had the
inherited grant. The RBAC mutators fail closed
(is_user_admin(NULL) is false), so this is defence in depth,
not a patched escalation.
The rule is a SET, not a per-target predicate: as a per-row qual it is pushed
below a DISTINCT ON and evaluated once per underlying row - 2.7s over 29k score
rows for 45 players, against 37ms.
supabase/audit/identity_disclosure_audit.sql is the standing check, because "we
fixed the leak" is not a durable claim. All four checks report HEALTHY.
Left deliberately anon-callable, each documented where it is kept: the plugin
store browse API, the three helpers called from RLS policies on anon-readable
tables, and custom_access_token_hook. Verified after the sweep that anonymous
plugin-store browse and signed-in RBAC reads both still work.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Add the master-key rotation procedure, and rotate
get_encryption_key() was anon-callable, so the Vault master_encryption_key was
retrievable by anyone holding the project anon key - which ships compiled into
this public repo. Revoking access does not un-disclose a key, so it was rotated
on 2026-09-09 and the 184 rows encrypted under it re-encrypted.
Scope of the original exposure, stated precisely because the capability was
worse than the reach: decrypt_text() was anon-callable beside it, so an attacker
holding ciphertext from ANY channel - a backup, an export, a screenshot - could
decrypt it through the API without even needing the key. But no bulk read path
to that ciphertext existed: qbo_token_state and google_token_state have no
client grants at all, and all 178 secrets rows have org_id NULL, so the secrets
policy reduces to owner-only. This was a serious latent vulnerability - one
org-shared secret, one RLS slip, or anyone with backup access would have had
plaintext - rather than a confirmed bulk disclosure.
The script is kept because rotation is not a one-off. It is atomic (one DO
block), self-verifying (md5 of every row's plaintext captured before, re-derived
after through decrypt_text so the check covers both the swap and the data), and
a mismatch or row-count drift raises and rolls back, so a broken rotation cannot
commit. Verified after the run: the live key differs from the disclosed one,
184/184 rows decrypt under it, and the disclosed key opens 0 rows.
The old key is retained in the Vault under an explicit name. Do NOT delete it
while pre-rotation backups exist - they are encrypted under it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(db): close signed-in crypto access and verify visibility boundaries
* test(db): exercise repeated key rotation and exact anonymous ACLs
* test(db): grant fixture role membership on Supabase Postgres
* test(db): inline rotation source for isolated pgTAP mounts
* test(db): keep generated rotation SQL out of source control
* fix(db): reserve identity-taking store mutators for the edge service
* fix(db): serialize rotations without requiring direct Vault writes
* fix(db): address review gaps in system-org and rotation coverage
* test(db): verify core secrets across hex and broker-free rotations
* test(db): execute standing audit and missing-signature regression
* fix(db): enforce revoke postconditions and harden rotation verification
* test(db): enable recovery metadata in the authenticated fixture
* test(db): use the schema-supported authenticator type
* Address remaining database review quality findings
* Fix invoker view audit boolean parsing and test timestamp bounds
* Rotate the TOTP envelope, exempt extension routines, and close the repo/prod drift
Addresses the handoff's database-integration item and the substantiated findings
from the current-head review. Everything below was executed against the PR's own
Supabase preview branch, not asserted from reading.
**The blocker: rotation refused to run once #417 landed.** The guard was a
blanket "TOTP is installed, refuse everything". It is now an adapter. The stored
form is 'v1:' || encrypt_text(...) - same cipher, same key, only the framing
differs - so the column map gained an envelope prefix and rotates like the rest
once the prefix is stripped and re-applied. Three things this needed that reading
the diff would not have shown:
* #417's trigger RAISES on a v1: value ('TOTP input must be plaintext, not a
storage envelope'), so the re-encryption update is rejected outright. The
reviewed trigger is now disabled for the update and restored after, inside
the same transaction.
* safe_decrypt_recovery_codes returns jsonb, not text, so verification through
the real read path needed a cast. Both fingerprints now go through each
column's OWN application read path rather than decrypt_text, which also
removes the step-1/step-5 asymmetry raised as U5.
* a step 0 pre-check reports rows that are already unreadable, instead of
letting a safe_decrypt_* NULL surface at the end as "a row was missed".
The rejection contract is preserved, not removed: an unknown envelope version,
an unmapped safe_decrypt_* wrapper, or an unreviewed BEFORE trigger on a mapped
table each still refuse. All three are now regression-tested, along with the
recovery-code column that was previously never exercised (its fixture was
vacuous - create_secret writes no secret_metadata row without p_twofa_enabled).
Verified: three consecutive rotations, TOTP/recovery/password all intact, v1
envelope preserved, trigger re-enabled and still enforcing its own contract.
**CREATE EXTENSION was impossible.** The fail-closed guard aborted the first
routine of any extension installed into public: `create extension pgtap` failed
with "Anonymous EXECUTE remains on public.pg_version()". A guard that forces
operators to disable it in order to install an extension is a guard that ends up
disabled. Extension-owned routines are now exempted with a warning, and the
exposure stays visible as advisory CHECK 1x rather than vanishing - it reports
pgtap (1079) on the preview. The sweep skips them too: pgcrypto-style helpers
are called from column DEFAULTs and CHECK constraints, which are evaluated with
the DML role's privileges. No extension owns a function in public on a clean
database, so no deployed behaviour changes.
**The repo was weaker than production.** find_user_by_email, get_session_status
and update_api_key_last_used were locked down on the live project on 2026-09-08
but never captured as a migration, so a fresh deploy re-opened them.
find_user_by_email is a user-enumeration oracle over auth.users and signup is
open. The standing audit caught this itself on the preview - CHECK 2 named it -
which is the drift that check exists for. Both remaining callers use
SUPABASE_SERVICE_ROLE_KEY, verified on this branch, so service_role keeps
EXECUTE and neither path changes.
Also: the guard no longer hard-fails on a database without the Supabase roles (a
plain restore), and the vault fixtures no longer assume the key is absent (U3) -
both suites failed outright on any database that already had one. Retained-key
assertions measure this run's delta instead of an absolute count.
Suites on the preview: rotation 16/16, db access audit, and visibility 54/54 all
clean; the standing audit reports no failing gated check.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Exercise the anonymous read path, not just the grant
The keep-list assertions only checked has_function_privilege for the seven
allowlisted signatures. That proves the grant, not the path: an RLS policy
expression is evaluated as the QUERYING role, so revoking a policy helper turns
an anonymous SELECT into 'permission denied for function ...' rather than an
empty result, and no privilege assertion can see it.
The seeding is the part that matters. A policy expression runs PER ROW, so on
an empty table it is never evaluated - and plugins and user_roles are both empty
on a fresh database, which made the first version of this probe pass no matter
what had been revoked. Caught by trying to make it fail.
Sensitivity is stated from measurement, not assumption: with a row present,
revoking can_view_plugin_row from anon does fail the probe and revoking
authorize does not, because permissive policies are ORed and short-circuit. A
helper in a policy that never has to be evaluated cannot be detected this way by
any test, so the comment says so rather than implying broader coverage.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Make the gated audit checks and the test generator provable
Both halves of this are the same failure: a suite that is green while proving
nothing. I hit that twice writing the previous commits - a recovery-code fixture
over a secret_metadata row create_secret never wrote, and an anonymous-read
probe over an empty table whose RLS policy therefore never evaluated - so these
are not hypothetical.
Finding 6. The four CI-GATED checks (1, 1b, 4, 5) only ever asserted HEALTHY,
while every advisory check already had a fixture-based detection test. An
inverted `not exists`, a wrong role literal or a typo in the evtenabled test
would have left the gate permanently green. Each is now broken deliberately and
asserted to report it: a routine granted to anon AFTER creation (which is how
one gets past the event trigger), a revoked deliberate anonymous grant, a
disabled event trigger, and a crypto routine exposed to authenticated. They have
teeth by construction - the assertion is that the finding appears, so a broken
audit query fails them.
Finding 8. The generator gained --check, which regenerates in memory and
compares without writing, and orphan removal for a generated suite whose .sql.in
has been renamed or deleted - that file otherwise keeps being executed forever
with stale content. Orphans are identified by a provenance banner rather than a
hard-coded list, so a hand-written suite is never touched; verified that
explicit_anon_and_org_visibility_test.sql is untouched. Each guard was proven by
making it fire: a hand edit and a planted orphan are both reported by --check,
and generate removes the orphan.
CI now runs generate and --check as their own step, so a preparation failure is
visible in the log instead of being attributed to the test run.
Preview: rotation 16/16, db access audit 15/15, visibility 56/56.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Announce every project change, not only the plugin-initiated ones
Panels could come up empty and stay empty, and the reason was that the
startup restore told no one.
ProjectChangeEvent was published from ProjectDataProviderImpl.selectProject,
which is the path a PLUGIN takes. The two callers that matter most bypass
it: WorkspaceApplier.applyWorkspace calls windowProjectState.selectProject
directly when it restores the workspace, and BossTopBar's picker goes
through selectProjectInWindow. Neither published anything.
The ordering that produced the empty panel: DefaultPlugin is constructed in
a DisposableEffect keyed on registries and window state, not on
selectedProject, and it kicks off the plugin scan asynchronously while
BossAppScaffold composes and the sidebar builds panels immediately. The
restore meanwhile waits on WorkspaceManager's sequential Dispatchers.IO JSON
reads. When those reads are slow the panel is built first, reads the ""
that WindowProjectState seeds itself with, and renders its no-project state.
The restore then lands, silently, and nothing remounts the cached component.
Publishing from the state itself rather than from one caller catches all
three. The bus is MutableSharedFlow(replay = 0), so a publish that never
happens cannot be recovered by a later subscriber - which is why this could
not stay per-caller.
The publish is removed from selectProject rather than added alongside the
collector; keeping both would double-fire on the plugin path.
previousPath is seeded from the current value so the StateFlow's replay of
it is not announced as a change. That seed is "" at startup, and telling
every plugin the project just became "" moments before the real restore
arrives is precisely the clear-yourself signal worth avoiding.
One collector per window: projectDataProvider is `by lazy` on DefaultPlugin,
which is per-window, so this cannot stack.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Publish from the state's own callback, not from a lazily-built provider
The review's finding 1: observing `selectedProject` from `ProjectDataProviderImpl`
put the fix behind two `by lazy` initializers. `projectDataProvider` and
`applicationEventBus` are both lazy on `DefaultPlugin`, and the second is the only
host path that creates the bus at all. So the collector started only if some plugin
touched both before the workspace JSON reads finished - and "the reads are slow" is
the premise of the bug. Touched after the restore, the collector seeds to the
restored path and announces nothing; never touched, no caller is announced at all.
The announcement now hangs off the `ProjectSelectionCallback` that
`WindowProjectStateRegistry` installs when it builds the window's state, which
`BossAppState` does eagerly in composition. `WindowProjectState.selectProject` is
the sole mutator of the selection and invokes that callback synchronously, so every
caller is covered with no coroutine, no scope and nothing lazy in the path. Being
synchronous also makes `previousProjectPath` a faithful history rather than
best-effort: there is no StateFlow conflation to collapse A -> B -> C into A -> C.
The two registry entry points carried a copy of the wiring each; they now share one
`newState`, so the announcement cannot be installed on some windows and not others.
Finding 1's third mitigation, which is a hole of its own: `publishSystemEvent` was a
no-op whenever the bus had not been created, so on a build where no installed plugin
had touched `applicationEventBus` yet, no host event existed - not `ProjectChangeEvent`,
not `AuthEvent`, not `TabEvent` - and with `replay = 0` none could be recovered.
It now creates the bus instead of dropping. `getInstance` also re-checks the registry
on every call rather than only at creation, so an instance that exists while the
registry is empty can no longer strand the host's publisher.
Findings 2 and 3: `ProjectDataProviderImpl` implements `DisposableProvider` and
`DefaultPlugin.dispose()` releases it through a named delegate, alongside
`logDataProviderDelegate` and `gitDataProviderDelegate`; its scope gets a
`SupervisorJob`, matching `pluginScope`.
Tests: `ProjectChangeAnnouncementTest` (6) pins a direct `selectProject` being
announced, the previous-path chain, the seeding rule, the same-path suppression,
both registry entry points, and the bus being created rather than dropping the event.
Full `:composeApp:desktopTest` green: 3695 tests, 0 failures. `detekt ktlintCheck` clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Round 2: honest bus wording, atomic previousPath, three more tests
Review (1/2) and (2/2).
1. `publishSystemEvent` does not save the event it creates the bus for. Correct - no
registered publisher means nobody holds the bus, so it has no subscribers and that
first event still reaches no one. The comment, the test name and the PR body all
claimed otherwise. Reworded to what it actually buys: the host stops being
permanently silent while it waits for a plugin to touch `applicationEventBus`.
3. `previousPath` was check-then-act on a plain field, and
`ProjectDataServiceBridge.selectProject` is a suspend gRPC handler with no hop to
Main (verified: `KernelBootstrap` registers it, the bridge calls
`provider.selectProject` directly). Now an `AtomicReference.getAndSet`, so the read
and the write are one operation.
5. The registry read-modify-write moved back inside `synchronized(this)`. Both fields
are in fact `@Volatile` in boss-plugin-api 1.0.87 (checked the pinned jar), but the
lock is uncontended after the first call and removes the question. The
bus-before-publisher ordering is now written down, including why a publish landing
in that window loses nothing.
6. `initialPath` KDoc no longer describes a production scenario it cannot reach; it is
documented as defensive and test-reachable, and defaulted to "".
7. Three tests added:
- a plugin-initiated selection is announced exactly once (catches both a silent
regression to zero and an accidentally re-added publish);
- the registry test now asserts the recent-projects half of the callback ran -
deleting it from `newState` previously left every test green;
- `dispose()` stops the recent-projects collector, which is the whole reason
`projectDataProviderDelegate` became a named lazy.
`Dispatchers.setMain(UnconfinedTestDispatcher())` for the class: the provider
collects on `Dispatchers.Main`, which has no implementation in a plain test JVM, so
its collector silently never ran. Unconfined also makes the dispose test
deterministic rather than a sleep.
Smaller: `SupervisorJob` comment describes it as future-proofing (there is one
collector); `systemEventBusScope` is `by lazy`; the KERNEL-mode consequence of
disposing the provider is documented on `dispose()`.
Not taken: `replay = 1` for `ProjectChangeEvent`. It would close the mirror race, but
it changes `events()` semantics for every subscriber and every event type on a bus
whose consumers are out of tree. The body now says the race is narrowed rather than
closed, and names the api-side `selectedProject` StateFlow as what closes it.
`:composeApp:desktopTest` 3697 tests, 0 failures. `detekt ktlintCheck` clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Round 3: unconditional announcement, ordered publish, drop dead register
Review 3.
2. AGENTS.md: a bullet in the canonical "what a third-party plugin can observe"
paragraph, since this widens *when* a filesystem path reaches every installed
plugin.
3. The `expect` KDoc still said "a no-op if the bus has not been created yet". Only
the `actual` had been corrected. Fixed, and it now states the part that IS still
best-effort: replay = 0 means an unsubscribed event is gone either way.
4. The announcement sat downstream of `ProjectState.updateRecentProjects`, so a throw
there skipped it AND left `previousPath` stale - every later selection reporting a
previous path one step behind, silently, for the session. Now `try { recents }
finally { announce }`: order preserved for anyone reading recents off the event,
the announcement unconditional, the throw still propagating.
5. `getAndSet` closed the read-modify-write but not the publish order, so two
concurrent selections could emit out of order. `previousPath` and the publish are
now in one `synchronized` block (the AtomicReference is gone - the lock subsumes
it). The KDoc says plainly what this still does not settle: `WindowProjectState`
writes `_selectedProject.value` then calls the callback with no atomicity, so state
and last event can still disagree. Closing that means locking upstream.
6. `ProjectState`'s async `loadRecentProjects()` assigns `_recentProjects.value`
wholesale and could land mid-test, dropping a just-added path - only where
`~/.boss/recent-projects.json` exists, so a developer-machine-only flake. A
once-per-JVM settle in `@BeforeTest` puts that single file read before any test.
The test paths stay deliberately non-existent, now with a comment saying why: the
fire-and-forget saves are unordered, and `loadRecentProjects` reclaims entries whose
directory is gone, so a real temp directory would leak where these self-heal.
7. `register` deleted. No production caller (`BossAppState:299` uses `getOrCreate`),
and it overwrote an existing entry - handing back a state with a fresh announcer
seeded to "" for a window that already had a project. `newState` stays; it is what
keeps the wiring from drifting.
8. `@Volatile` dropped from `ApplicationEventBusImpl.instance` - every access is inside
the lock now, and leaving it reads as if a fast path survives.
Also: a test for the actual regression path, `applyWorkspace` announcing the project it
restores, rather than a comment claiming to imitate it.
Answering the grep you could not run: the only `projectChanges()` consumers in
boss_plugins are the two fluck-agent panels (`FluckAgentViewModel`), and both assign
`_bossProject.value` and re-sweep - idempotent on a repeat. Nothing used the repeat
publish as a reload nudge. Recorded in the announcer comment.
Item 1 was already in the PR body from round 2 ("The startup race is narrowed, not
closed"), naming the mirror case and the api-side StateFlow that closes it.
`:composeApp:desktopTest` 3698 tests / 368 classes, 0 failures. `detekt ktlintCheck` clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Round 4: split the dispose out, stop tests writing the real ~/.boss
Review 4. Items 1 and 3 were the before-merge ones.
1. Verified: ProjectDataServiceBridge.watchRecentProjects collects
provider.recentProjects, a StateFlow, so cancelling the provider's scope on
window close leaves that gRPC stream open and silently frozen for every
out-of-process plugin. Taking the first option and SPLITTING the dispose out.
Note this reverses what round 1 asked for, and the justification changed rather
than the opinion: once the announcement moved to the registry callback,
ProjectDataProviderImpl went back to being exactly what it was before this PR, so
the leak is pre-existing and unrelated to the subject. Closing it properly means
the bridge reading ProjectState directly instead of a per-window provider, which
is its own change. The reasoning is on the class so it is not re-fixed by accident.
3. Verified and worse than described: MAX_RECENT_PROJECTS = 10, so test entries evict
real ones from the developer's picker and no cleanup restores them.
systemProperty("user.home", <build>/test-home) on the Test task. One run with it in
place shows the existing suite was writing far more than recents to the real home -
window-appearance-settings.json, keymap-settings.json, scrollbar-settings.json,
recent-browser-pages.json, dashboard-stats.json and a ~/BossProjects/ directory all
land in test-home now. The settle is gone with it: no recent-projects.json in
test-home means the wholesale reload never happens.
2. publishSystemEvent now refuses the bus != null && systemPublisher == null state
instead of falling through into a bus that may not be the registry's, with one
warning rather than one per event. The comment at the fallback is true as written.
4. Both stale comments fixed (one construction path, not two).
5. hostProjectCallback(updateRecents, announcer) extracted so the try/finally claim is
testable - ProjectState is an object, so a hard-coded call cannot be made to fail.
6. The publish-inside-the-lock trade is spelled out in the KDoc: what it buys
(ordering), what it costs (an inline subscriber runs holding the lock inside
selectProject), and that the contended path is untested.
7. The serial-execution dependency is recorded in the test class KDoc.
Plus: multi-window isolation, and a test that makes the recents update throw.
One thing this round caused and fixed: removing dispose() left two
ProjectDataProviderImpl collectors alive on the test dispatcher. On a
TestCoroutineScheduler a live coroutine is something every later runTest waits on, and
the git classes timed out at 60s in full-suite runs while passing in isolation.
Dispatchers.setMain(Dispatchers.Unconfined) rather than UnconfinedTestDispatcher():
same inline behaviour, not enrolled in a scheduler anyone else observes. It is also a
real argument for the follow-up - without dispose() the provider cannot be constructed
in a test without leaking.
:composeApp:desktopTest 3699 tests / 368 classes, 0 failures. detekt ktlintCheck clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Round 5: reset test-home per run, stop getInstance stealing a publisher
Review 5.
1. Confirmed reproducibly - build/test-home/.boss/recent-projects.json was sitting
there from the previous run. doFirst now deleteRecursively() before mkdirs(), so
the home is fresh per run rather than merely private. Without it the redirect only
helped the first run and ProjectState.init's load reintroduced the same race from
the second. Verified by running the full suite twice back to back.
2. Right, and sharper than it looks: the guard tested `bus` but wrote both fields, and
`systemPublisher != null && bus == null` is exactly what ProjectChangeAnnouncementTest,
BrowserAnalyticsEmissionTest and BossTabsComponentMoveTest install. Guard now tests
both, so getInstance cannot take a publisher away from whoever set it.
3. Test added for the warn-and-drop branch, asserting the DROP rather than the warning -
partialRegistryWarned is a one-shot process global, so "it warns" is unassertable
after the first test to trip it. Uses a stub bus so the half-registry state is real.
4. The re-entrancy hazard is now documented as SAFE and why (synchronized is reentrant,
previousPath advances before the publish), so it is not "fixed" later. The
lock-widens-a-UI-hang-to-a-cross-thread-one point is in the same block.
5. Taken, including the optional ones:
- hostProjectCallback keeps both failures (addSuppressed) instead of letting finally
discard the first. Needs @Suppress("TooGenericExceptionCaught") - catching
Throwable is the contract, and both are rethrown.
- ProjectDataProviderImpl takes an injectable dispatcher (default Dispatchers.Main).
This removes the global setMain AND the round-4 collector leak at the source
rather than working around them, without reopening the DisposableProvider split.
- Three more pins: a null window state announces nothing, applyWorkspace
(restoreProject = false) announces nothing, unregister + getOrCreate restarts the
chain at "".
Not taken, with reasons: jvmArgumentProviders (the current form works, is
config-cache-serialisable and passed CI on three OSes); widening the user.home redirect
to plugin-path-utils (its test only mkdirs a directory every BOSS user already has).
There is no CHANGELOG in this repo, so the release-notes line for the same-path change
is an action item at release time, not a file change. Flagged in the body.
Two consecutive full runs: 3703 tests / 368 classes, 0 failures both times.
detekt ktlintCheck clean - it caught two TooGenericExceptionCaught and a long line in
this round's own code first.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Address project announcement review feedback
* fix(plugin-store): restrict plugin_downloads RLS to the server-side pipeline (#488)
* Batch validation: promote latest reviewed dev changes to main (#463)
* Fix #30: Resolve duplicate context menu label collisions
* Cleanup: Remove dead code for #91 and #93
* chore: Sanitize SubmitResult.Error at construction rather than at one render site
* Fix panel component lifecycle disposal
* fixes
* Guard panel lifecycle destruction failures
* fix(logging): sanitize secret RPC failures and crash-report errors
Three related gaps in what gets logged and shown when something fails:
- SecretService.kt had no BossLogger in any of its ten catch blocks (#145).
When the organisation migration broke all four secret RPCs (#144), the
only WARN in the log came from the calling plugin - the code that
actually failed said nothing. Every catch now logs via
logger.warn(LogCategory.NETWORK, ...), reusing the already-sanitized
failure for both the log and the returned Result rather than sanitizing
twice. RoleService and RoleCreationService were checked and already log
correctly - SecretService was the only silent one.
- LogSanitizer.sanitizeExceptionMessage redacted a hostname only when it
appeared inside a URL - a bare hostname (exactly what
UnknownHostException.getMessage() produces, i.e. every DNS failure, and
what a proxy-connect failure looks like) passed through untouched (#109).
Added a narrow hostname pattern: lowercase-only labels ending in a short
explicit TLD/`.internal`/`.local` list, with a negative lookahead so a
package path that happens to end in a real TLD word mid-FQN
(kotlinx.coroutines.internal.ScopeCoroutine, kotlinx.io.EOFException)
is not mistaken for one - caught by the existing realistic-stack-trace
test after the first version of the pattern redacted a live Kotlin
package name out of it.
- CrashReportService.SubmitResult.Error held a raw exception string,
sanitized at exactly one render call site in CrashReportDialog (#110).
The constructor is now private; SubmitResult.Error.of(...) is the only
way to build one, and it sanitizes before the raw string can reach
.message - a property of the type now, not something every future
consumer (a copy button, a toast, a log line) has to remember on its own.
Both construction sites (the service's two catch blocks, and the one in
CrashReportDialog the issue names directly) now route through it.
Tests: new coverage for the ten now-logging SecretService paths (via
SupabaseWiringTest's existing sanitization-wiring guard, updated to
recognize the "sanitize once into a local, reuse for log and return" shape
SupabaseDataProviderImpl already used), six new LogSanitizerTest cases for
the hostname fix (including the FQN/version-number false-positive guards),
and a new CrashReportServiceTest for the sanitize-at-construction guarantee.
* test(crash): extract construction tests from #404
Selected unchanged test file from Antriksh1984 original commit 99d2a5228023c541e825fe2714619c8228140369 (PR #404). Maintainer extraction only; subsequent API/scope adaptations are recorded separately.
* chore(ipc): clarify advisory menu ID scope and fix formatting
* chore(ui): finish obsolete toolbar comment cleanup
* test(crash): adapt consolidated coverage and guard factory visibility
Maintainer consolidation: use #311 companion invoke and a URL fixture instead of depending on #404 hostname redaction. Add private constructor/copy bytecode assertions. Preserve original contributor tests and implementation credit separately.
* refactor(crash): consolidate construction sanitization in #311
Maintainer scope change: remove only the duplicate #110 implementation, retaining Antriksh1984 original #109/#145 work and commit history. Standalone construction tests are retained with attribution in #311; its Aditya8369 implementation supersedes Error.of. Keep render-time sanitization here until #311 merges and describe the remaining hostname coverage conservatively.
* test(panels): retain sibling lifecycle isolation coverage from #313
Adapt the original #313 test to the #317 constructor and supply the missing panelInfo overrides. Original test scenario and assertions are by @rehannayeem0786; these compile adaptations are maintainer integration work.
* fix(panels): complete lifecycle cleanup across failure and window teardown
Destroy partially constructed panels and replacements that fail to resume. Continue downward lifecycle transitions after a failing pause or stop callback. Dispose panels before plugin teardown in the same callback rather than relying on sibling Compose effect order. Add regression coverage and adapt the retained #313 test bodies for detekt. These changes are maintainer follow-up to the original #313/#317 contributions.
* docs(ui): remove stale disabled top-bar feature list
* test(ipc): protect advisory menu item ID uniqueness
* fix(panels): keep store registration scoped to its window effect
Remove the redundant unregister from plugin teardown. Document independent context services and synchronous Essenty onCreate replay, with tests for registration ownership and exactly-once create callbacks. Keep tab teardown hardening separate. Maintainer follow-up to the review of #317; original #313/#317 contributor credit is unchanged.
* fix(crash): address consolidated sanitization review notes
Maintainer follow-up to Claude N1-N4: document factory/copy invariant, correct stale comments, move Aditya original construction assertions into the service suite while retaining Antriksh blank/diagnostic cases, and log unexpected dialog submission failures. Original contribution credit remains separate.
* fix(logging): keep server failure payloads out of secret RPC logs
Maintainer follow-up to Claude review: log operation and exception type only, preserve sanitized return semantics, and check each catch for safe return/log wiring. Document measured hostname limits without broadening the matcher; add module-local redaction and diagnostic-preservation coverage. Antriksh original implementation remains credited separately.
* ci: build PR batches on dev
* Allow Claude diff reviews after approved fork builds (#413)
* fix(browser): preserve newer clipboard copies during plain-text paste (#316)
* fix(browser): paste-without-formatting no longer clobbers a copy made in its restore window (#205)
* fix(browser): make paste-without-formatting's restore identity-based, not text-based
Review of this PR caught that the text-equality guard cannot distinguish 'our write is still current' from 'a previous press already restored the rich original' - whose string projection is by construction the same text - so two presses inside the 200ms window permanently downgraded a rich clipboard to plain text. A PasteWithoutFormattingSession now tracks the exact Transferable each press installed and restores the pre-window original once, while a user copy in the window still wins. Adds five tests to BrowserClipboardCommandsTest, including the double-press regression; wraps the two over-limit lines; e.message -> e::class.simpleName in the touched handlers.
* fix(browser): fold tryRestore's early exits so it stays within detekt's ReturnCount
* fix(browser): blank lines between FakeTransferable overrides per ktlint
* test(browser): retain clipboard restore scenarios from #408
* fix(browser): track clipboard ownership through AWT wrappers
---------
Co-authored-by: rehannayeem0786 <rehannayeem0786@github.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
Co-authored-by: Antriksh1984 <antrikshsingh850@gmail.com>
* fix(plugins): serialize concurrent first loads per class name (#324)
* fix(plugins): prevent duplicate class definitions during concurrent loads
* fix(plugins): make class loading parallel-safe
* test(plugins): pin concurrent loading lifecycle boundaries
Maintainer consolidation follow-up: retain #324 synchronization and its tests; add concurrent shared-name, superclass, and unload-marker coverage. Document caller-sensitive registration based on arjun28115 review feedback and clarify that marking unload does not drain an already admitted parent lookup. No global lifecycle lock or resource-policy changes.
---------
Co-authored-by: sgoel2be24-cyber <223222024+sgoel2be24-cyber@users.noreply.github.com>
Co-authored-by: Procs <168113425+ProportanilityConstant@users.noreply.github.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(browser): defer native disposal until admitted calls drain (#312 + #409) (#412)
* fix: make BrowserHandle JavaScript execution cancellable
the previous synchronous JxBrowser call blocked the Main/EDT thread;
coroutine cancellation could not take effect while that call was blocking;
the implementation now uses JxBrowser’s asynchronous JavaScript callback API;
suspendCancellableCoroutine makes the Kotlin waiting side cancellable;
late callbacks after cancellation are safely ignored;
this does NOT terminate JavaScript already executing inside Chromium.
* fix: add robust native call lifecycle tracking for disposal
* test: verify native operation cleanup on synchronous failure
* fix: linearize browser disposal and pending operation tracking
* fix: resolve detekt violations in browser native tracking
* test: satisfy detekt failure simulation rule
* fix: close native operation disposal race
* fix(browser): drain an in-flight call before closing the browser
Issue #300 reports that executeJavaScript is a synchronous, non-
cancellation-aware native call, and a plugin's own withTimeoutOrNull
around it can only abandon the caller's wait - not the call itself -
which stays running on Main indefinitely and can race a concurrent
handle.dispose(), touching the same native object from two threads at
once.
The severe half of this was already fixed by BoundedBrowserCall: this
class confines every blocking round trip to one dedicated daemon thread
instead of Dispatchers.Main, so a wedged renderer no longer freezes the
app. What was not yet closed is the residual window this issue's core
report is actually about: shutdown() stopped new work but did not wait
for whatever was already running, so a caller could proceed straight to
browser.close() while a call from just before shutdown was still
finishing on the dedicated thread.
shutdown() now waits, bounded, for that in-flight call to drain before
returning. Costs nothing in the common case - awaitTermination returns
immediately once the one worker thread and its queue are both idle,
which is where an instance sits between calls - and only the bound in
the rare case something was genuinely still running.
Known, and disclosed in the KDoc rather than claimed away: this narrows
the race for the common case (a call that was always going to finish
quickly) rather than closing it. It cannot help when the in-flight call
is itself the wedge this class's own deadline exists for - waiting
unboundedly for a genuinely stuck call would reintroduce the exact freeze
BoundedBrowserCall exists to prevent, just moved from a plugin's await
into every caller's teardown path. There is no JxBrowser API on this
version able to interrupt a blocking round trip already inside the
native call, which is the whole reason the call is confined to its own
thread instead of cancelled - closing that window completely needs
JxBrowser's own cooperation, which the issue's own analysis already
concluded is unavailable.
Tests: shutdown waits for a fast in-flight call to finish before
returning, and does not wait past its own drain timeout for a wedged one
- the same before/after pair the rest of this test file already uses for
BoundedBrowserCall's deadline.
* fix(browser): defer native close until admitted calls drain
Consolidates the lifecycle work from #312 (johncybersage) and #409 (Antriksh1984), retaining both original histories. Replaces success-only callback accounting and timeout-then-close with executor termination signals and host-owned native/profile cleanup. These corrections and replacement tests are maintainer/agent work, separate from contributor scoring.
* fix(browser): bound profile waits and clarify deferred cleanup ownership
Address consolidation review: schedule both service disposal paths without awaiting native close, diagnose pending drains, centralize executor ownership, and bound profile lease acquisition without permitting unsafe reuse or eviction. Document process-exit abandonment and verify wiring and cleanup ordering. Maintainer/agent follow-up, separate from original #312 and #409 contributor credit.
* fix(browser): schedule disposal after early UI teardown failure
* ci: create launcher directory before Claude native installation
* Revert "ci: create launcher directory before Claude native installation"
This reverts commit 5184908e6c3f133502508513b4c5e17651b13bc0.
* test: keep warmup registry ordering check on one event loop
* test(browser): normalize CRLF in disposal source wiring guards
---------
Co-authored-by: john k <johnk@johns-MacBook-Pro.local>
Co-authored-by: john k <johnk@syn-172-100-137-112.res.spectrum.com>
Co-authored-by: Antriksh1984 <antrikshsingh850@gmail.com>
* feat(tabs): show a speaker glyph on tabs that are playing audio (#314)
* feat(tabs): show a speaker glyph on tabs that are playing audio (#308)
* fix(tabs): review round - compile, registry lifetime, UI-thread marshalling, owner flush
1. graphicsLayer import dropped (does not exist at that package); the fade now uses .alpha(audioAlpha), already imported. 2. TabAudioRegistry no longer outlives its owner: disposeAllTabsBlocking unregisters everything this component registered, and DetachedTab.destroy drops the entry for a tab destroyed without adoption - the handler captures the BossTabsComponent, so stale entries retained it. 3. The registry update is marshalled to the EDT via SwingUtilities.invokeLater, so the tab-model mutation happens on the UI thread as the KDoc claims. 4. setFullscreenHandler flushes the current playback state once ownerTabId becomes known, closing the starts-before-registration race the review could not verify. 5. TabAudioRegistryTest added: delivery, last-writer-wins, ownership-checked unregister.
* fix(tabs): CI round 2 - DetachedTab receiver error, long line, and baseline-preserving signatures
- BossMainWindowPanel.kt:2098: DetachedTab is a non-inner nested class and cannot use the outer component as receiver; the audio-handler drop moves to detachTab instead, which is where adoption-vs-destroy actually diverges. - TabAudioRegistry.kt:9 wrapped under detekt's 120-char limit. - FluckTabInfo.isPlayingAudio and BossTabButton's glyph read no longer change any signature: the flag is a mutable property (copied in copy()'s body) and the button reads it off tabInfo, so the constructor, copy() and BossTabButton keep their exact baseline-frozen detekt IDs and the resurfaced LongMethod/LongParameterList/CyclomaticComplexMethod findings retire.
* fix(tabs): fold handleAudioUpdate within ReturnCount; add missing final newline in the test file
* fix(tabs): publish audio state by browser ownership and keep icon layout stable
---------
Co-authored-by: rehannayeem0786 <rehannayeem0786@github.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* feat: connect chrome density settings and keyboard controls (#305)
* feat: wire the chrome density scale into settings and small-screen defaults
ChromeDensity / ChromeDimens.of() / LocalChromeDimens already existed on
main and every bar (BossTitleBar, BossTopBar, the main tab bar,
BossBottomBar) already read its height from BossChrome.dimens - but
LocalChromeDimens was never provided anywhere, so it silently resolved to
its staticCompositionLocalOf default (Comfortable) always. The scale was
built and unreachable.
This closes that gap:
- WindowAppearanceSettings gets a `density` field (default COMFORTABLE, so
an existing install's settings file - which has never heard of this key
- decodes to exactly the chrome it already had; no settings-version bump
needed).
- BossApp.kt now provides LocalChromeDimens from that field, host-only and
outside BossAppCompositionLocals (plugins never draw host chrome).
- A "Density" dropdown in Settings > Bars (Compact/Comfortable/Spacious),
indexed in Settings search.
- A fresh install on a small screen (< 1000dp logical height, clearing the
13" MacBook Air's ~931-956pt from #239's own measurements) now defaults
to Compact and starts with the bottom bar off - the one bar this manager
can still reclaim itself, since the side strips are already off by
class default. Toolkit.getScreenSize() is read once, wrapped in
runCatching (HeadlessException off a display must not break a fresh
install), with a pure defaultDensityFor(screenHeightDp) so the decision
is unit-testable without a display.
Addresses #239, scoped down from the full issue. Out of scope in this
pass, deliberately:
- The app does not yet report its own chrome budget as a measured
percentage - the issue's own "state it, don't argue it" ask. Reachable
later from the same ChromeDimens source, but needs a spot to render it.
- The title bar's 27dp "Boss Console" label row is untouched - collapsing
or merging it with the tab bar is a separate, riskier layout change than
a density scale.
- No regression intended to the existing per-bar show/hide switches or
focus-mode edges: density only changes how much room a bar that is
already on screen takes, never whether it is shown.
* fix: drop the Compact-density bottom-bar auto-off, wrap an overlong line
Address review feedback on #305:
- Removed showBottomBar = density != ChromeDensity.COMPACT from
getDefaultSettings(). ChromeMetricsTest already shows the shipped
macOS defaults reach 93.3% of a 931dp window at Comfortable, so
#239's >=90% bar is met without touching the status bar - and that
bar's own KDoc argues it must stay on ("the only always-on readout").
It was also reachable from the settings-load catch fallback, so a
transient read error could silently drop the bar and have the next
save persist the loss. Compact now only changes density, nothing else.
- Wrapped primaryScreenHeightDp() - was 121 chars, one over detekt's
default MaxLineLength (120), which applies here since .editorconfig's
140 is a ktlint-only setting and this line wasn't in the baseline.
* feat: add chrome density setting
* feat: add small-screen chrome defaults
* feat: add chrome density controls and small-screen defaults
* fix(ui): preserve lean density defaults and test density controls
* style: format multiline screen-height lookup
* fix(ui): separate density recovery and make editor chords opt-in
* style: wrap density default documentation
---------
Co-authored-by: AdityaK-iiita <adityakolate0070@gmail.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(plugins): stop host resource fallback after unload (#325)
* fix(plugins): stop host resource fallback after unload
* fix(plugins): preserve resource warning for a missing result
---------
Co-authored-by: sgoel2be24-cyber <223222024+sgoel2be24-cyber@users.noreply.github.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* Handle closed browser frames during navigation callbacks (#400)
* Handle closed browser frames during navigation callbacks
* test(browser): retain closed-event guard after navigation integration
* fix(browser): preserve helpers on PID failure and recognize closed transports
* style(browser): wrap review diagnostics for detekt
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* refactor(window): pin the panel content alignment width (#398)
* fix(window): restore strict width constraints for nested panels
Fixes #278. Commit c15660c5 inadvertently dropped the explicit width constraint for nested panels in the right dock by wrapping them inside a PanelColumn with a Box(weight(1f)) that lacked fillMaxWidth(). This loose constraint caused a known issue with SubcomposeLayout caching stale bounds when resized, resulting in Row content disappearing after weighted Spacers. This commit restores the .fillMaxWidth() propagation.
* Cover panel content alignment across width changes
* Keep panel layout regression in the existing app test package
* Document panel alignment contract and pin filling content width
* Wrap the panel width assertion to satisfy detekt
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* #320 - Fix quick actions vanishing in short tab rail (#328)
* #320 - Fix quick actions vanishing in short tab rail
* Fix: detekt & ktlint checks
* Fix rail action budgets and reversible fallback wiring
* Fix resize test assertion import and update rail coverage note
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(crash): refine scroll boundary and bound error sanitization (#350)
* fix(crash): refine scroll boundary and bound error sanitization
* Avoid exposing a partial token at the crash message limit
* Document sanitizer input bound and verify expansion ordering
---------
Co-authored-by: Aishwary Anand <aishwary.cd22@bmsce.ac.in>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(logging): redact private hostnames (#346)
* fix(logging): redact private hostnames
* Handle private hostname punctuation and preserve diagnostic ports
* Format hostname punctuation regression assertion
---------
Co-authored-by: ayush <workside@gamil.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* feat: Opening an .html file should ask open as a file or webpage (#333)
* feat: Opening an .html file should ask open as a file or webpage
* Fix HTML prompt delivery and persisted preference ordering
* Cover HTML routing and correct review test fixtures
* Honor HTML navigation intent and release pending prompts on failure
* Isolate HTML prompt handling from the general dialog host
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(plugin-store): parse timestamps consistently in list and detail metadata (#379)
* Fix: Implement proper ISO timestamp parsing in PluginStoreClient
* fix(repository): implement robust ISO timestamp parsing for Supabase payloads
* style(repository): resolve detekt MaxLineLength and TooGenericExceptionCaught
* fix(plugin-store): implement plugin metadata timestamp parsing
PluginStoreClient.parseTimestamp was a stub that always returned 0L, so
every plugin fetched from the store showed "Last Updated"/"Published" as
the Unix epoch in the Toolbox UI regardless of what the server actually
sent (#337).
Parses the ISO-8601 timestamp (the shape the store sends, e.g.
2024-05-12T14:30:00Z, or with a numeric offset) via java.time.Instant,
falling back to 0L for a blank, missing, or malformed string rather than
throwing - matching the field's own empty-string default for a response
that omits it. java.time rather than adding kotlinx-datetime as a new
dependency: this module already uses java.util.* directly in the same
commonMain source set (it has one real target, jvm("desktop")), so nothing
is gained by introducing a second time library for one function.
Tests exercise the real PluginDetailResponse.toPluginInfo(), matching the
convention already established next to it in
PluginStoreResponseDecodingTest: a Z-suffixed timestamp, a numeric-offset
timestamp, a blank string, and a malformed string.
* Fix timestamp normalization edge cases and verify both response mappings
* Use release publication dates and pin timezone-independent fallback
---------
Co-authored-by: Sanjay <sanjaysaini4423@gmail.com>
Co-authored-by: Antriksh1984 <antrikshsingh850@gmail.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* feat(remote-ui): authenticate and expose remote UI surfaces (#348)
* feat(remote-ui): authenticate remote UI process identity
* feat(remote-ui): place authenticated remote UI surfaces
* fix(remote-ui): close ownership and placement lifecycle races
* test(remote-ui): expose placement recovery cases to JUnit
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* feat(dashboard): add shared read-only What's New release feed (#381)
* Add What's New release feed to dashboard
* Serialize update settings persistence
* Remove unused update settings import
* fix(updater): retain realtime refreshes during shared release fetches
* style(updater): format shared release fetch regression tests
* fix(updater): coalesce release refresh bursts and preserve visible history
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(language): consolidate host and out-of-process language-id tables (#358)
* fix(language): consolidate host and out-of-process language-id tables
EditorLanguages (composeApp) and EditorServiceImpl (modules/boss-app-editor)
each hand-maintained their own extension-to-language-id table because
boss-app-editor - a plain JVM module compiled to a GraalVM native image -
could not depend on anything in composeApp. They disagreed: EditorServiceImpl
named .sh/.bash/.zsh "shell", EditorLanguages named them "bash", and
EditorServiceImpl was missing more than forty ids the other table had
(fortran, delphi, latex, lisp, tcl, clojure, batch, diff, ...) (#75).
Moves the table into a new plugin-platform/plugin-language-types module -
dependency-free by design, so it carries nothing extra into boss-app-editor's
native-image build - and has both EditorLanguages and EditorServiceImpl read
from it. This is the first time anything under modules/ depends on
plugin-platform/; verified compiling and passing tests before committing to
the approach.
EditorLanguages keeps its existing public API unchanged (same values for
every extension), so FileTypeCategoriesTest and EditorLanguageDetectionTest
needed no changes. EditorServiceImpl keeps its own "proto"->"protobuf" entry
and "plaintext" fallback as local additions on top of the shared table,
rather than folding them into it: proto isn't part of the boss-file-types.json
default-app-association surface the shared table backs, and "plaintext" is
this service's own gRPC default, not a value composeApp reads.
Adds LanguageIdsTest (the new module) and EditorServiceImplTest (previously
no tests existed for this class at all) - the latter pins the actual bug fix,
that detectLanguage("sh") now returns "bash" instead of "shell".
Out of scope, and disclosed rather than silently dropped: two of the five
duplicate tables the issue names - the editor-tab plugin's own
LanguageDetection and BossEditor's lexer registry - live in separate
repositories this build cannot reach. FileIcons.forSpecialFileName/forFile in
plugin-icons is also not consolidated: it keys icon selection off the raw
filename to preserve distinctions a language id can't carry (package.json,
yarn.lock and pnpm-lock.yaml are all valid JSON/YAML but get three different
brand icons) and never computes a language id in the first place, so
routing it through this table would need a separate icon-selection redesign.
* fix(language): keep file-association drift checks on the shared table
* fix(language): apply shared filename rules to editor file opens
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(editor): contain write-local stack overflow and preserve fatal errors (#396)
* Make writeFileContentSafe actually safe: catch the Errors it was named for
`writeFileContentSafe` exists to return false rather than throw. It did not hold
that contract:
} catch (e: Exception) {
fileIoLogger.warn(LogCategory.EDITOR, "Error writing file", error = e)
false
}
StackOverflowError is an Error, not an Exception, so it went straight through -
out of writeFileContent, out of the editor_write_file MCP handler, and to the
caller as a bare StackOverflowError instead of the handler's own
"Write failed for <path>". The warn that would have named the file never ran
either.
That is not hypothetical. risa-labs-inc/boss-plugin-editor-tab#18 and #27 are two
independent reports of it, on unrelated content, and #27's complaint is precisely
the missing diagnosis: the error "gives no indication of which input caused it".
StackOverflowError and OutOfMemoryError are caught explicitly rather than
widening to Throwable. A stack overflow unwinds and leaves the JVM usable, and
readFileContentSafe in this same file already treats OutOfMemoryError as a
reportable outcome. Throwable would also absorb LinkageError and ThreadDeath,
which are not this function's to swallow - there is a test asserting a
NoClassDefFoundError still propagates.
The failure log now names the path and the character count. Not the content:
these writes carry whatever the user is editing.
**This does not fix the write.** Whatever recurses is upstream of this function,
which is mkdirs plus writeText - I ruled out the plugin's handler, this write,
and the host's MCP argument parsing, and said so on the issue. What changes is
that the failure is reported honestly instead of escaping, which is where a
diagnosis can start.
The body moves to `guardedWrite` with an injectable write, because a test JVM
cannot be made to overflow the stack inside writeText on demand.
Verified: 3729 tests, 0 failures, plus ktlintCheck and detekt.
Refs risa-labs-inc/boss-plugin-editor-tab#18, #27.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(editor): clarify write guard scope and prior recursion fix
* fix(editor): preserve fatal heap errors in write guard
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(state): apply version-checked JSON merge patches (#361)
* Fix: Implement proper ISO timestamp parsing in PluginStoreClient
* Feature: Implement JSON Merge Patch for plugin delta state synchronization
* fix(state): validate delta bases and exclude duplicate timestamp scope
* fix(state): reject malformed and deeply nested patches
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* :arrow_up: deps:(deps): Bump grpc from 1.83.1 to 1.84.0 (#363)
Bumps `grpc` from 1.83.1 to 1.84.0.
Updates `io.grpc:grpc-netty` from 1.83.1 to 1.84.0
- [Release notes](https://github.com/grpc/grpc-java/releases)
- [Commits](https://github.com/grpc/grpc-java/compare/v1.83.1...v1.84.0)
Updates `io.grpc:grpc-protobuf` from 1.83.1 to 1.84.0
- [Release notes](https://github.com/grpc/grpc-java/releases)
- [Commits](https://github.com/grpc/grpc-java/compare/v1.83.1...v1.84.0)
Updates `io.grpc:grpc-stub` from 1.83.1 to 1.84.0
- [Release notes](https://github.com/grpc/grpc-java/releases)
- [Commits](https://github.com/grpc/grpc-java/compare/v1.83.1...v1.84.0)
Updates `io.grpc:grpc-services` from 1.83.1 to 1.84.0
- [Release notes](https://github.com/grpc/grpc-java/releases)
- [Commits](https://github.com/grpc/grpc-java/compare/v1.83.1...v1.84.0)
Updates `io.grpc:grpc-util` from 1.83.1 to 1.84.0
- [Release notes](https://github.com/grpc/grpc-java/releases)
- [Commits](https://github.com/grpc/grpc-java/compare/v1.83.1...v1.84.0)
---
updated-dependencies:
- dependency-name: io.grpc:grpc-netty
dependency-version: 1.84.0
dependency-type: direct:production
update-type: version-update:semver-minor
- dependency-name: io.grpc:grpc-protobuf
dependency-version: 1.84.0
dependency-type: direct:production
update-type: version-update:semver-minor
- dependency-name: io.grpc:grpc-stub
dependency-version: 1.84.0
dependency-type: direct:production
update-type: version-update:semver-minor
- dependency-name: io.grpc:grpc-services
dependency-version: 1.84.0
dependency-type: direct:production
update-type: version-update:semver-minor
- dependency-name: io.grpc:grpc-util
dependency-version: 1.84.0
dependency-type: direct:production
update-type: version-update:semver-minor
...
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* :arrow_up: deps:(deps): Bump protobuf from 4.36.0 to 4.36.1 (#364)
Bumps `protobuf` from 4.36.0 to 4.36.1.
Updates `com.google.protobuf:protobuf-kotlin` from 4.36.0 to 4.36.1
Updates `com.google.protobuf:protobuf-java` from 4.36.0 to 4.36.1
- [Release notes](https://github.com/protocolbuffers/protobuf/releases)
- [Commits](https://github.com/protocolbuffers/protobuf/commits)
---
updated-dependencies:
- dependency-name: com.google.protobuf:protobuf-kotlin
dependency-version: 4.36.1
dependency-type: direct:production
update-type: version-update:semver-patch
- dependency-name: com.google.protobuf:protobuf-java
dependency-version: 4.36.1
dependency-type: direct:production
update-type: version-update:semver-patch
...
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(plugins): defer stale jar cleanup until restart (#340)
* fix(plugins): defer stale jar cleanup until restart
* fix(plugins): satisfy quality checks
* Preserve staged plugin artifacts when another plugin updates
* Require cleanup intent and retain artifacts with unordered manifest versions
---------
Co-authored-by: ayush <workside@gamil.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(plugins): defer fluck-browser hot-reload to a restart (#352)
* fix(plugins): defer stale jar cleanup until restart
* fix(plugins): defer fluck-browser hot-reload to a restart
Hot-reloading fluck-browser force-unloads the classloader that created its
JxBrowser native views. Every open tab - and every tab opened afterward,
since the factory that would recreate the view is gone too - draws an empty
box, and a second hot-reload does not recover it; only a full restart does.
HotReloadPolicy names plugins that must never be swapped in place, matching
the existing NOT_USER_INSTALLABLE/RetiredPlugins pattern. It's checked at
every path that can force-unload a live plugin to apply an update: the
resetPluginInstances/doReloadPlugin path that actually reproduced this bug,
the menu-driven Reload action, the Toolbox update flow, and the store
installer. For a not-hot-reloadable plugin, the new jar is staged into
installed.json without touching the running instance or its open tabs, and
a status message tells the user to restart to apply it - the next cold
start loads the update with a fresh classloader.
Also fixes two call sites that asked "restart dependent plugins?" before
checking whether an unload was even going to happen, which produced a
confusing prompt for an unload that a deferred update was never going to
do.
* fix(plugins): satisfy quality checks
* Preserve staged plugin artifacts when another plugin updates
* Validate deferred browser artifacts and reject ineffective downgrades
* Require cleanup intent and retain artifacts with unordered manifest versions
* Preserve unloaded-browser recovery and unify safe deferred selection
* Format scoped update cleanup integration
---------
Co-authored-by: ayush <workside@gamil.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* Harden OOP plugin reaping: hook-race flag, registry cleanup, spawn guard (#135) (#365)
* Harden OOP plugin reaping: hook-race flag, registry cleanup, spawn guard (#135)
reapDepth (AtomicInteger) replaces the boolean reaping flag, so the two JVM shutdown hooks (main.kt and KernelBootstrap) cannot clear the in-progress signal while the other is still reaping (item 2). reapChildren now unregisters reaped handles from the process-wide ProcessRegistry, so stale dead entries do not persist across an in-process mode switch (item 4). OutOfProcessPluginSpawnerImpl.spawn() stands down when isReaping(), so a plugin load racing host exit cannot register a child after the reap snapshot (item 3). Covered by ReapChildrenTest.
* Serialize plugin spawn registration with reaping and preserve replacement handles
* Fence late process creation without blocking shutdown and cover recovery
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(plugins): complete loader cleanup after disposal linkage errors (#302)
* fix: API hot swap during startup
* fix: workflows
* resolved comments by maintainer
* fix CI and buil…
…a-labs-inc#635) * fix(kernel): require verified caller identity for the Git bridge GitServiceBridge had no caller-identity check on any of its fourteen RPCs. Any process able to open a connection to the kernel IPC server - not only the plugins the host itself loaded - could read the open project's full commit history and working-tree status, and could discardChanges (destroying uncommitted work), checkout, cherryPick or revert against the project's real git repository with no credential at all. Gate every RPC behind authenticatedCallerOrRefuse(), the same PERMISSION_DENIED-on-no-identity pattern PluginUIServiceBridge established for BossConsole#53. The four watch streams (file status, commit log, is-repo, is-loading) capture identity synchronously before returning and re-check it on every emission, so a revoked token stops a live stream rather than only new calls - matching the shape PR risa-labs-inc#550's review settled on for this bridge's siblings. * style(tests): wrap chained builder calls in GitServiceBridgeTest Real CI's ktlintDesktopTestSourceSetCheck flagged chain-method-continuation violations (a local run had missed this) - wrap the affected builder chains onto their own lines, per ktlintFormat. No behavior change. * test(kernel): cover revoked callers and log stream refusals --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>
…a-labs-inc#645) * fix(kernel): require verified caller identity for the Log bridge LogServiceBridge had no caller-identity check on any of its nine RPCs. Any process able to open a connection to the kernel IPC server - not only the plugins the host itself loaded - could watchLogs to see the live stdout/stderr stream of the running BOSS process, or call exportLogs to pull its full history, with no credential at all. Unlike a bridge whose values are deliberately routed through LogSanitizer at the call site, nothing about this stream is scrubbed for IPC exposure - it is whatever every logger.info/warn/error call site across this large and growing codebase happened to write, which is not a guarantee that none of it is a stack trace, a file path, or something else nobody meant to expose to an arbitrary local process. Gate every RPC behind authenticatedCallerOrRefuse(), the same PERMISSION_DENIED-on-no-identity pattern PluginUIServiceBridge established for BossConsole#53. The four watch streams capture identity synchronously before returning and re-check it on every emission, matching the shape PR risa-labs-inc#550's review settled on for this bridge's siblings. * test(kernel): verify revocation and diagnose stream refusals --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>
…abs-inc#647) * fix(security): randomize the IV for encrypt_text/decrypt_text encrypt_text called the 3-arg extensions.encrypt(plaintext, key, 'aes'), which pgcrypto documents as using an all-zero IV, so encryption was a pure function of (plaintext, key) alone: two equal passwords produced byte-identical ciphertext. Because create_secret encrypts server-side and RLS lets a user read their own password_encrypted back, any signed-in user was an encryption oracle - grouping a dump by ciphertext names every set of users sharing a password, with no key required. The same defect applied to recovery_codes_encrypted and twofa_secret. encrypt_text now draws a fresh random 16-byte IV per call and stores 'v2:' || base64(iv || ciphertext); decrypt_text dispatches on that prefix and falls back to the original zero-IV path for anything not yet migrated, so this deploys without a flag day. A backfill converts existing rows in all three columns, guarded by NOT LIKE 'v2:%' so a partial or repeated run is a no-op. Deliberately scoped to this one defect (BossConsole#618 names three). Defect 2 (the encryption key truncated to 32 string characters instead of 32 bytes) is not touched here: ops/rotate_master_encryption_key.sql records that a deployed key may be hex or base64, and this migration cannot tell which is live in any given project's Vault - guessing wrong would make get_encryption_key()'s result unusable and break encryption for every row on deploy. Defect 3 (no MAC/AEAD) needs its own wire-format review. ops/rotate_master_encryption_key.sql itself will need a follow-up before it can run again, since it manipulates raw ciphertext bytes assuming the old no-IV, no-prefix layout - both gaps are documented in the migration's own header. This could not be run against a real Postgres/pgTAP instance in this environment (no supabase/docker CLI available here) - the added test was written and reviewed by hand, and hosted CI's Database (pgTAP) check is the actual verification gate. * fix(security): un-qualify substring(... from ...) so it parses CI's Database (pgTAP) check caught what local review couldn't (no Postgres available in this sandbox): pg_catalog.substring(x from y) is a syntax error. substring's SQL-standard FROM/FOR trailing syntax is recognized by the parser only on the bare function name - schema-qualifying it makes PostgreSQL treat it as an ordinary call, where FROM is not a valid argument separator. 20260909000000_encrypt_totp.sql's own decrypt_text already calls substring unqualified for exactly this reason; this migration's three call sites (envelope/iv/body extraction in decrypt_text, and the twofa_secret backfill) now match that convention. Both functions' search_path already resolves the unqualified name to pg_catalog's substring, so nothing else about name resolution changes. * fix(security): make key rotation understand the v2 IV envelope CI's Database (pgTAP) check caught what local review couldn't (no Postgres available in this sandbox): the pre-existing master_key_rotation_test.sql runs ops/rotate_master_encryption_key.sql against real fixture data, and that script's re-encrypt loop calls extensions.decrypt/encrypt directly on raw ciphertext bytes, assuming the old zero-IV, no-prefix layout. The 'v2:' prefix the previous commit's encrypt_text now writes is not valid base64, so decoding it raised - exactly the gap this migration's own header comment already named as a follow-up, just sooner than expected. Added a pg_temp helper, rotate_ciphertext, that re-encrypts a mapped column's value under the new key while preserving whichever envelope it already carries: a 'v2:' row is decrypted with decrypt_iv under the old key and re-encrypted with encrypt_iv under the new key AND a fresh IV (a function rather than an inline SQL expression, specifically so gen_random_bytes(16) is evaluated once and reused for both the encrypt_iv call and the stored prefix - two calls would produce two different values and corrupt the row). A legacy row - every column before this pair of migrations, and still qbo_token_state / google_token_state going forward, since those are untouched by the IV migration - stays zero-IV, matching what it already was. Rotation's contract stays "same shape, new key"; upgrading a row's shape is the encryption migration's job, not rotation's. Updated the IV migration's own header comment, which had flagged this as a still-open follow-up, to point at the fix now landing alongside it instead. * fix(security): preserve plaintext bytes during IV upgrade * fix(security): make IV backfill atomic and diagnose unreadable rows --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>
* Stop anon reaching schema public by inheritance, and scope identity to real orgs
The BOSS Supabase anon key is compiled into this repo, which is public. Anything
`anon` can execute is therefore executable by anyone. Verified on 2026-09-08:
* get_encryption_key() returned the Vault master encryption key, unauthenticated.
decrypt_text() was anon-callable beside it, making a decryption oracle over
everything encrypt_text protects - user secrets and the QBO/Google broker
credentials.
* find_user_by_email() confirmed any address and returned its uuid.
* list_shareable_recipients() returned 152 users WITH full email addresses to
any self-registered account, and 82 @risalabs.ai addresses on a search.
* arcade_leaderboard() and arcade_bs_standings() published the roster of
everyone who had opened the Arcade, unauthenticated.
None of it was granted on purpose. PostgreSQL hardwires EXECUTE to PUBLIC on
every new function, PUBLIC includes anon, and this project's default privileges
add anon on top - so a SECURITY DEFINER function is internet-callable from the
moment it is created. Both revokes are traps in mirror image: `from public`
leaves the explicit anon grant, `from anon` leaves the hardwired PUBLIC one, and
ALTER DEFAULT PRIVILEGES cannot remove the latter at all (pg_default_acl loses
the entry and new functions still get `=X`).
20260908000000 event trigger: revoke PUBLIC+anon on every function created in
schema public. An explicit `grant ... to anon` after the create
still wins - anon access must be stated, never inherited.
20260908010000 org_visible_users / org_is_vetted / user_display_name: ONE
definition of "may this account learn who that account is",
shared by the Arcade and poker rather than copied. Vetted means
a human approved the join, which excludes the catch-all `boss`
org every account joins on signup (153 members, 20 domains).
20260908020000 route list_shareable_recipients through both. It was already
org-scoped, which is why it looked right; it accepted ANY
shared org.
20260908030000 one-time sweep of the ~30 functions that already had the
inherited grant. The RBAC mutators fail closed
(is_user_admin(NULL) is false), so this is defence in depth,
not a patched escalation.
The rule is a SET, not a per-target predicate: as a per-row qual it is pushed
below a DISTINCT ON and evaluated once per underlying row - 2.7s over 29k score
rows for 45 players, against 37ms.
supabase/audit/identity_disclosure_audit.sql is the standing check, because "we
fixed the leak" is not a durable claim. All four checks report HEALTHY.
Left deliberately anon-callable, each documented where it is kept: the plugin
store browse API, the three helpers called from RLS policies on anon-readable
tables, and custom_access_token_hook. Verified after the sweep that anonymous
plugin-store browse and signed-in RBAC reads both still work.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Add the master-key rotation procedure, and rotate
get_encryption_key() was anon-callable, so the Vault master_encryption_key was
retrievable by anyone holding the project anon key - which ships compiled into
this public repo. Revoking access does not un-disclose a key, so it was rotated
on 2026-09-09 and the 184 rows encrypted under it re-encrypted.
Scope of the original exposure, stated precisely because the capability was
worse than the reach: decrypt_text() was anon-callable beside it, so an attacker
holding ciphertext from ANY channel - a backup, an export, a screenshot - could
decrypt it through the API without even needing the key. But no bulk read path
to that ciphertext existed: qbo_token_state and google_token_state have no
client grants at all, and all 178 secrets rows have org_id NULL, so the secrets
policy reduces to owner-only. This was a serious latent vulnerability - one
org-shared secret, one RLS slip, or anyone with backup access would have had
plaintext - rather than a confirmed bulk disclosure.
The script is kept because rotation is not a one-off. It is atomic (one DO
block), self-verifying (md5 of every row's plaintext captured before, re-derived
after through decrypt_text so the check covers both the swap and the data), and
a mismatch or row-count drift raises and rolls back, so a broken rotation cannot
commit. Verified after the run: the live key differs from the disclosed one,
184/184 rows decrypt under it, and the disclosed key opens 0 rows.
The old key is retained in the Vault under an explicit name. Do NOT delete it
while pre-rotation backups exist - they are encrypted under it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(db): close signed-in crypto access and verify visibility boundaries
* test(db): exercise repeated key rotation and exact anonymous ACLs
* test(db): grant fixture role membership on Supabase Postgres
* test(db): inline rotation source for isolated pgTAP mounts
* test(db): keep generated rotation SQL out of source control
* fix(db): reserve identity-taking store mutators for the edge service
* fix(db): serialize rotations without requiring direct Vault writes
* fix(db): address review gaps in system-org and rotation coverage
* test(db): verify core secrets across hex and broker-free rotations
* test(db): execute standing audit and missing-signature regression
* fix(db): enforce revoke postconditions and harden rotation verification
* test(db): enable recovery metadata in the authenticated fixture
* test(db): use the schema-supported authenticator type
* Address remaining database review quality findings
* Fix invoker view audit boolean parsing and test timestamp bounds
* Rotate the TOTP envelope, exempt extension routines, and close the repo/prod drift
Addresses the handoff's database-integration item and the substantiated findings
from the current-head review. Everything below was executed against the PR's own
Supabase preview branch, not asserted from reading.
**The blocker: rotation refused to run once #417 landed.** The guard was a
blanket "TOTP is installed, refuse everything". It is now an adapter. The stored
form is 'v1:' || encrypt_text(...) - same cipher, same key, only the framing
differs - so the column map gained an envelope prefix and rotates like the rest
once the prefix is stripped and re-applied. Three things this needed that reading
the diff would not have shown:
* #417's trigger RAISES on a v1: value ('TOTP input must be plaintext, not a
storage envelope'), so the re-encryption update is rejected outright. The
reviewed trigger is now disabled for the update and restored after, inside
the same transaction.
* safe_decrypt_recovery_codes returns jsonb, not text, so verification through
the real read path needed a cast. Both fingerprints now go through each
column's OWN application read path rather than decrypt_text, which also
removes the step-1/step-5 asymmetry raised as U5.
* a step 0 pre-check reports rows that are already unreadable, instead of
letting a safe_decrypt_* NULL surface at the end as "a row was missed".
The rejection contract is preserved, not removed: an unknown envelope version,
an unmapped safe_decrypt_* wrapper, or an unreviewed BEFORE trigger on a mapped
table each still refuse. All three are now regression-tested, along with the
recovery-code column that was previously never exercised (its fixture was
vacuous - create_secret writes no secret_metadata row without p_twofa_enabled).
Verified: three consecutive rotations, TOTP/recovery/password all intact, v1
envelope preserved, trigger re-enabled and still enforcing its own contract.
**CREATE EXTENSION was impossible.** The fail-closed guard aborted the first
routine of any extension installed into public: `create extension pgtap` failed
with "Anonymous EXECUTE remains on public.pg_version()". A guard that forces
operators to disable it in order to install an extension is a guard that ends up
disabled. Extension-owned routines are now exempted with a warning, and the
exposure stays visible as advisory CHECK 1x rather than vanishing - it reports
pgtap (1079) on the preview. The sweep skips them too: pgcrypto-style helpers
are called from column DEFAULTs and CHECK constraints, which are evaluated with
the DML role's privileges. No extension owns a function in public on a clean
database, so no deployed behaviour changes.
**The repo was weaker than production.** find_user_by_email, get_session_status
and update_api_key_last_used were locked down on the live project on 2026-09-08
but never captured as a migration, so a fresh deploy re-opened them.
find_user_by_email is a user-enumeration oracle over auth.users and signup is
open. The standing audit caught this itself on the preview - CHECK 2 named it -
which is the drift that check exists for. Both remaining callers use
SUPABASE_SERVICE_ROLE_KEY, verified on this branch, so service_role keeps
EXECUTE and neither path changes.
Also: the guard no longer hard-fails on a database without the Supabase roles (a
plain restore), and the vault fixtures no longer assume the key is absent (U3) -
both suites failed outright on any database that already had one. Retained-key
assertions measure this run's delta instead of an absolute count.
Suites on the preview: rotation 16/16, db access audit, and visibility 54/54 all
clean; the standing audit reports no failing gated check.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Exercise the anonymous read path, not just the grant
The keep-list assertions only checked has_function_privilege for the seven
allowlisted signatures. That proves the grant, not the path: an RLS policy
expression is evaluated as the QUERYING role, so revoking a policy helper turns
an anonymous SELECT into 'permission denied for function ...' rather than an
empty result, and no privilege assertion can see it.
The seeding is the part that matters. A policy expression runs PER ROW, so on
an empty table it is never evaluated - and plugins and user_roles are both empty
on a fresh database, which made the first version of this probe pass no matter
what had been revoked. Caught by trying to make it fail.
Sensitivity is stated from measurement, not assumption: with a row present,
revoking can_view_plugin_row from anon does fail the probe and revoking
authorize does not, because permissive policies are ORed and short-circuit. A
helper in a policy that never has to be evaluated cannot be detected this way by
any test, so the comment says so rather than implying broader coverage.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Make the gated audit checks and the test generator provable
Both halves of this are the same failure: a suite that is green while proving
nothing. I hit that twice writing the previous commits - a recovery-code fixture
over a secret_metadata row create_secret never wrote, and an anonymous-read
probe over an empty table whose RLS policy therefore never evaluated - so these
are not hypothetical.
Finding 6. The four CI-GATED checks (1, 1b, 4, 5) only ever asserted HEALTHY,
while every advisory check already had a fixture-based detection test. An
inverted `not exists`, a wrong role literal or a typo in the evtenabled test
would have left the gate permanently green. Each is now broken deliberately and
asserted to report it: a routine granted to anon AFTER creation (which is how
one gets past the event trigger), a revoked deliberate anonymous grant, a
disabled event trigger, and a crypto routine exposed to authenticated. They have
teeth by construction - the assertion is that the finding appears, so a broken
audit query fails them.
Finding 8. The generator gained --check, which regenerates in memory and
compares without writing, and orphan removal for a generated suite whose .sql.in
has been renamed or deleted - that file otherwise keeps being executed forever
with stale content. Orphans are identified by a provenance banner rather than a
hard-coded list, so a hand-written suite is never touched; verified that
explicit_anon_and_org_visibility_test.sql is untouched. Each guard was proven by
making it fire: a hand edit and a planted orphan are both reported by --check,
and generate removes the orphan.
CI now runs generate and --check as their own step, so a preparation failure is
visible in the log instead of being attributed to the test run.
Preview: rotation 16/16, db access audit 15/15, visibility 56/56.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Announce every project change, not only the plugin-initiated ones
Panels could come up empty and stay empty, and the reason was that the
startup restore told no one.
ProjectChangeEvent was published from ProjectDataProviderImpl.selectProject,
which is the path a PLUGIN takes. The two callers that matter most bypass
it: WorkspaceApplier.applyWorkspace calls windowProjectState.selectProject
directly when it restores the workspace, and BossTopBar's picker goes
through selectProjectInWindow. Neither published anything.
The ordering that produced the empty panel: DefaultPlugin is constructed in
a DisposableEffect keyed on registries and window state, not on
selectedProject, and it kicks off the plugin scan asynchronously while
BossAppScaffold composes and the sidebar builds panels immediately. The
restore meanwhile waits on WorkspaceManager's sequential Dispatchers.IO JSON
reads. When those reads are slow the panel is built first, reads the ""
that WindowProjectState seeds itself with, and renders its no-project state.
The restore then lands, silently, and nothing remounts the cached component.
Publishing from the state itself rather than from one caller catches all
three. The bus is MutableSharedFlow(replay = 0), so a publish that never
happens cannot be recovered by a later subscriber - which is why this could
not stay per-caller.
The publish is removed from selectProject rather than added alongside the
collector; keeping both would double-fire on the plugin path.
previousPath is seeded from the current value so the StateFlow's replay of
it is not announced as a change. That seed is "" at startup, and telling
every plugin the project just became "" moments before the real restore
arrives is precisely the clear-yourself signal worth avoiding.
One collector per window: projectDataProvider is `by lazy` on DefaultPlugin,
which is per-window, so this cannot stack.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Publish from the state's own callback, not from a lazily-built provider
The review's finding 1: observing `selectedProject` from `ProjectDataProviderImpl`
put the fix behind two `by lazy` initializers. `projectDataProvider` and
`applicationEventBus` are both lazy on `DefaultPlugin`, and the second is the only
host path that creates the bus at all. So the collector started only if some plugin
touched both before the workspace JSON reads finished - and "the reads are slow" is
the premise of the bug. Touched after the restore, the collector seeds to the
restored path and announces nothing; never touched, no caller is announced at all.
The announcement now hangs off the `ProjectSelectionCallback` that
`WindowProjectStateRegistry` installs when it builds the window's state, which
`BossAppState` does eagerly in composition. `WindowProjectState.selectProject` is
the sole mutator of the selection and invokes that callback synchronously, so every
caller is covered with no coroutine, no scope and nothing lazy in the path. Being
synchronous also makes `previousProjectPath` a faithful history rather than
best-effort: there is no StateFlow conflation to collapse A -> B -> C into A -> C.
The two registry entry points carried a copy of the wiring each; they now share one
`newState`, so the announcement cannot be installed on some windows and not others.
Finding 1's third mitigation, which is a hole of its own: `publishSystemEvent` was a
no-op whenever the bus had not been created, so on a build where no installed plugin
had touched `applicationEventBus` yet, no host event existed - not `ProjectChangeEvent`,
not `AuthEvent`, not `TabEvent` - and with `replay = 0` none could be recovered.
It now creates the bus instead of dropping. `getInstance` also re-checks the registry
on every call rather than only at creation, so an instance that exists while the
registry is empty can no longer strand the host's publisher.
Findings 2 and 3: `ProjectDataProviderImpl` implements `DisposableProvider` and
`DefaultPlugin.dispose()` releases it through a named delegate, alongside
`logDataProviderDelegate` and `gitDataProviderDelegate`; its scope gets a
`SupervisorJob`, matching `pluginScope`.
Tests: `ProjectChangeAnnouncementTest` (6) pins a direct `selectProject` being
announced, the previous-path chain, the seeding rule, the same-path suppression,
both registry entry points, and the bus being created rather than dropping the event.
Full `:composeApp:desktopTest` green: 3695 tests, 0 failures. `detekt ktlintCheck` clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Round 2: honest bus wording, atomic previousPath, three more tests
Review (1/2) and (2/2).
1. `publishSystemEvent` does not save the event it creates the bus for. Correct - no
registered publisher means nobody holds the bus, so it has no subscribers and that
first event still reaches no one. The comment, the test name and the PR body all
claimed otherwise. Reworded to what it actually buys: the host stops being
permanently silent while it waits for a plugin to touch `applicationEventBus`.
3. `previousPath` was check-then-act on a plain field, and
`ProjectDataServiceBridge.selectProject` is a suspend gRPC handler with no hop to
Main (verified: `KernelBootstrap` registers it, the bridge calls
`provider.selectProject` directly). Now an `AtomicReference.getAndSet`, so the read
and the write are one operation.
5. The registry read-modify-write moved back inside `synchronized(this)`. Both fields
are in fact `@Volatile` in boss-plugin-api 1.0.87 (checked the pinned jar), but the
lock is uncontended after the first call and removes the question. The
bus-before-publisher ordering is now written down, including why a publish landing
in that window loses nothing.
6. `initialPath` KDoc no longer describes a production scenario it cannot reach; it is
documented as defensive and test-reachable, and defaulted to "".
7. Three tests added:
- a plugin-initiated selection is announced exactly once (catches both a silent
regression to zero and an accidentally re-added publish);
- the registry test now asserts the recent-projects half of the callback ran -
deleting it from `newState` previously left every test green;
- `dispose()` stops the recent-projects collector, which is the whole reason
`projectDataProviderDelegate` became a named lazy.
`Dispatchers.setMain(UnconfinedTestDispatcher())` for the class: the provider
collects on `Dispatchers.Main`, which has no implementation in a plain test JVM, so
its collector silently never ran. Unconfined also makes the dispose test
deterministic rather than a sleep.
Smaller: `SupervisorJob` comment describes it as future-proofing (there is one
collector); `systemEventBusScope` is `by lazy`; the KERNEL-mode consequence of
disposing the provider is documented on `dispose()`.
Not taken: `replay = 1` for `ProjectChangeEvent`. It would close the mirror race, but
it changes `events()` semantics for every subscriber and every event type on a bus
whose consumers are out of tree. The body now says the race is narrowed rather than
closed, and names the api-side `selectedProject` StateFlow as what closes it.
`:composeApp:desktopTest` 3697 tests, 0 failures. `detekt ktlintCheck` clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Round 3: unconditional announcement, ordered publish, drop dead register
Review 3.
2. AGENTS.md: a bullet in the canonical "what a third-party plugin can observe"
paragraph, since this widens *when* a filesystem path reaches every installed
plugin.
3. The `expect` KDoc still said "a no-op if the bus has not been created yet". Only
the `actual` had been corrected. Fixed, and it now states the part that IS still
best-effort: replay = 0 means an unsubscribed event is gone either way.
4. The announcement sat downstream of `ProjectState.updateRecentProjects`, so a throw
there skipped it AND left `previousPath` stale - every later selection reporting a
previous path one step behind, silently, for the session. Now `try { recents }
finally { announce }`: order preserved for anyone reading recents off the event,
the announcement unconditional, the throw still propagating.
5. `getAndSet` closed the read-modify-write but not the publish order, so two
concurrent selections could emit out of order. `previousPath` and the publish are
now in one `synchronized` block (the AtomicReference is gone - the lock subsumes
it). The KDoc says plainly what this still does not settle: `WindowProjectState`
writes `_selectedProject.value` then calls the callback with no atomicity, so state
and last event can still disagree. Closing that means locking upstream.
6. `ProjectState`'s async `loadRecentProjects()` assigns `_recentProjects.value`
wholesale and could land mid-test, dropping a just-added path - only where
`~/.boss/recent-projects.json` exists, so a developer-machine-only flake. A
once-per-JVM settle in `@BeforeTest` puts that single file read before any test.
The test paths stay deliberately non-existent, now with a comment saying why: the
fire-and-forget saves are unordered, and `loadRecentProjects` reclaims entries whose
directory is gone, so a real temp directory would leak where these self-heal.
7. `register` deleted. No production caller (`BossAppState:299` uses `getOrCreate`),
and it overwrote an existing entry - handing back a state with a fresh announcer
seeded to "" for a window that already had a project. `newState` stays; it is what
keeps the wiring from drifting.
8. `@Volatile` dropped from `ApplicationEventBusImpl.instance` - every access is inside
the lock now, and leaving it reads as if a fast path survives.
Also: a test for the actual regression path, `applyWorkspace` announcing the project it
restores, rather than a comment claiming to imitate it.
Answering the grep you could not run: the only `projectChanges()` consumers in
boss_plugins are the two fluck-agent panels (`FluckAgentViewModel`), and both assign
`_bossProject.value` and re-sweep - idempotent on a repeat. Nothing used the repeat
publish as a reload nudge. Recorded in the announcer comment.
Item 1 was already in the PR body from round 2 ("The startup race is narrowed, not
closed"), naming the mirror case and the api-side StateFlow that closes it.
`:composeApp:desktopTest` 3698 tests / 368 classes, 0 failures. `detekt ktlintCheck` clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Round 4: split the dispose out, stop tests writing the real ~/.boss
Review 4. Items 1 and 3 were the before-merge ones.
1. Verified: ProjectDataServiceBridge.watchRecentProjects collects
provider.recentProjects, a StateFlow, so cancelling the provider's scope on
window close leaves that gRPC stream open and silently frozen for every
out-of-process plugin. Taking the first option and SPLITTING the dispose out.
Note this reverses what round 1 asked for, and the justification changed rather
than the opinion: once the announcement moved to the registry callback,
ProjectDataProviderImpl went back to being exactly what it was before this PR, so
the leak is pre-existing and unrelated to the subject. Closing it properly means
the bridge reading ProjectState directly instead of a per-window provider, which
is its own change. The reasoning is on the class so it is not re-fixed by accident.
3. Verified and worse than described: MAX_RECENT_PROJECTS = 10, so test entries evict
real ones from the developer's picker and no cleanup restores them.
systemProperty("user.home", <build>/test-home) on the Test task. One run with it in
place shows the existing suite was writing far more than recents to the real home -
window-appearance-settings.json, keymap-settings.json, scrollbar-settings.json,
recent-browser-pages.json, dashboard-stats.json and a ~/BossProjects/ directory all
land in test-home now. The settle is gone with it: no recent-projects.json in
test-home means the wholesale reload never happens.
2. publishSystemEvent now refuses the bus != null && systemPublisher == null state
instead of falling through into a bus that may not be the registry's, with one
warning rather than one per event. The comment at the fallback is true as written.
4. Both stale comments fixed (one construction path, not two).
5. hostProjectCallback(updateRecents, announcer) extracted so the try/finally claim is
testable - ProjectState is an object, so a hard-coded call cannot be made to fail.
6. The publish-inside-the-lock trade is spelled out in the KDoc: what it buys
(ordering), what it costs (an inline subscriber runs holding the lock inside
selectProject), and that the contended path is untested.
7. The serial-execution dependency is recorded in the test class KDoc.
Plus: multi-window isolation, and a test that makes the recents update throw.
One thing this round caused and fixed: removing dispose() left two
ProjectDataProviderImpl collectors alive on the test dispatcher. On a
TestCoroutineScheduler a live coroutine is something every later runTest waits on, and
the git classes timed out at 60s in full-suite runs while passing in isolation.
Dispatchers.setMain(Dispatchers.Unconfined) rather than UnconfinedTestDispatcher():
same inline behaviour, not enrolled in a scheduler anyone else observes. It is also a
real argument for the follow-up - without dispose() the provider cannot be constructed
in a test without leaking.
:composeApp:desktopTest 3699 tests / 368 classes, 0 failures. detekt ktlintCheck clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Round 5: reset test-home per run, stop getInstance stealing a publisher
Review 5.
1. Confirmed reproducibly - build/test-home/.boss/recent-projects.json was sitting
there from the previous run. doFirst now deleteRecursively() before mkdirs(), so
the home is fresh per run rather than merely private. Without it the redirect only
helped the first run and ProjectState.init's load reintroduced the same race from
the second. Verified by running the full suite twice back to back.
2. Right, and sharper than it looks: the guard tested `bus` but wrote both fields, and
`systemPublisher != null && bus == null` is exactly what ProjectChangeAnnouncementTest,
BrowserAnalyticsEmissionTest and BossTabsComponentMoveTest install. Guard now tests
both, so getInstance cannot take a publisher away from whoever set it.
3. Test added for the warn-and-drop branch, asserting the DROP rather than the warning -
partialRegistryWarned is a one-shot process global, so "it warns" is unassertable
after the first test to trip it. Uses a stub bus so the half-registry state is real.
4. The re-entrancy hazard is now documented as SAFE and why (synchronized is reentrant,
previousPath advances before the publish), so it is not "fixed" later. The
lock-widens-a-UI-hang-to-a-cross-thread-one point is in the same block.
5. Taken, including the optional ones:
- hostProjectCallback keeps both failures (addSuppressed) instead of letting finally
discard the first. Needs @Suppress("TooGenericExceptionCaught") - catching
Throwable is the contract, and both are rethrown.
- ProjectDataProviderImpl takes an injectable dispatcher (default Dispatchers.Main).
This removes the global setMain AND the round-4 collector leak at the source
rather than working around them, without reopening the DisposableProvider split.
- Three more pins: a null window state announces nothing, applyWorkspace
(restoreProject = false) announces nothing, unregister + getOrCreate restarts the
chain at "".
Not taken, with reasons: jvmArgumentProviders (the current form works, is
config-cache-serialisable and passed CI on three OSes); widening the user.home redirect
to plugin-path-utils (its test only mkdirs a directory every BOSS user already has).
There is no CHANGELOG in this repo, so the release-notes line for the same-path change
is an action item at release time, not a file change. Flagged in the body.
Two consecutive full runs: 3703 tests / 368 classes, 0 failures both times.
detekt ktlintCheck clean - it caught two TooGenericExceptionCaught and a long line in
this round's own code first.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Address project announcement review feedback
* fix(plugin-store): restrict plugin_downloads RLS to the server-side pipeline (#488)
* Batch validation: promote latest reviewed dev changes to main (#463)
* Fix #30: Resolve duplicate context menu label collisions
* Cleanup: Remove dead code for #91 and #93
* chore: Sanitize SubmitResult.Error at construction rather than at one render site
* Fix panel component lifecycle disposal
* fixes
* Guard panel lifecycle destruction failures
* fix(logging): sanitize secret RPC failures and crash-report errors
Three related gaps in what gets logged and shown when something fails:
- SecretService.kt had no BossLogger in any of its ten catch blocks (#145).
When the organisation migration broke all four secret RPCs (#144), the
only WARN in the log came from the calling plugin - the code that
actually failed said nothing. Every catch now logs via
logger.warn(LogCategory.NETWORK, ...), reusing the already-sanitized
failure for both the log and the returned Result rather than sanitizing
twice. RoleService and RoleCreationService were checked and already log
correctly - SecretService was the only silent one.
- LogSanitizer.sanitizeExceptionMessage redacted a hostname only when it
appeared inside a URL - a bare hostname (exactly what
UnknownHostException.getMessage() produces, i.e. every DNS failure, and
what a proxy-connect failure looks like) passed through untouched (#109).
Added a narrow hostname pattern: lowercase-only labels ending in a short
explicit TLD/`.internal`/`.local` list, with a negative lookahead so a
package path that happens to end in a real TLD word mid-FQN
(kotlinx.coroutines.internal.ScopeCoroutine, kotlinx.io.EOFException)
is not mistaken for one - caught by the existing realistic-stack-trace
test after the first version of the pattern redacted a live Kotlin
package name out of it.
- CrashReportService.SubmitResult.Error held a raw exception string,
sanitized at exactly one render call site in CrashReportDialog (#110).
The constructor is now private; SubmitResult.Error.of(...) is the only
way to build one, and it sanitizes before the raw string can reach
.message - a property of the type now, not something every future
consumer (a copy button, a toast, a log line) has to remember on its own.
Both construction sites (the service's two catch blocks, and the one in
CrashReportDialog the issue names directly) now route through it.
Tests: new coverage for the ten now-logging SecretService paths (via
SupabaseWiringTest's existing sanitization-wiring guard, updated to
recognize the "sanitize once into a local, reuse for log and return" shape
SupabaseDataProviderImpl already used), six new LogSanitizerTest cases for
the hostname fix (including the FQN/version-number false-positive guards),
and a new CrashReportServiceTest for the sanitize-at-construction guarantee.
* test(crash): extract construction tests from #404
Selected unchanged test file from Antriksh1984 original commit 99d2a5228023c541e825fe2714619c8228140369 (PR #404). Maintainer extraction only; subsequent API/scope adaptations are recorded separately.
* chore(ipc): clarify advisory menu ID scope and fix formatting
* chore(ui): finish obsolete toolbar comment cleanup
* test(crash): adapt consolidated coverage and guard factory visibility
Maintainer consolidation: use #311 companion invoke and a URL fixture instead of depending on #404 hostname redaction. Add private constructor/copy bytecode assertions. Preserve original contributor tests and implementation credit separately.
* refactor(crash): consolidate construction sanitization in #311
Maintainer scope change: remove only the duplicate #110 implementation, retaining Antriksh1984 original #109/#145 work and commit history. Standalone construction tests are retained with attribution in #311; its Aditya8369 implementation supersedes Error.of. Keep render-time sanitization here until #311 merges and describe the remaining hostname coverage conservatively.
* test(panels): retain sibling lifecycle isolation coverage from #313
Adapt the original #313 test to the #317 constructor and supply the missing panelInfo overrides. Original test scenario and assertions are by @rehannayeem0786; these compile adaptations are maintainer integration work.
* fix(panels): complete lifecycle cleanup across failure and window teardown
Destroy partially constructed panels and replacements that fail to resume. Continue downward lifecycle transitions after a failing pause or stop callback. Dispose panels before plugin teardown in the same callback rather than relying on sibling Compose effect order. Add regression coverage and adapt the retained #313 test bodies for detekt. These changes are maintainer follow-up to the original #313/#317 contributions.
* docs(ui): remove stale disabled top-bar feature list
* test(ipc): protect advisory menu item ID uniqueness
* fix(panels): keep store registration scoped to its window effect
Remove the redundant unregister from plugin teardown. Document independent context services and synchronous Essenty onCreate replay, with tests for registration ownership and exactly-once create callbacks. Keep tab teardown hardening separate. Maintainer follow-up to the review of #317; original #313/#317 contributor credit is unchanged.
* fix(crash): address consolidated sanitization review notes
Maintainer follow-up to Claude N1-N4: document factory/copy invariant, correct stale comments, move Aditya original construction assertions into the service suite while retaining Antriksh blank/diagnostic cases, and log unexpected dialog submission failures. Original contribution credit remains separate.
* fix(logging): keep server failure payloads out of secret RPC logs
Maintainer follow-up to Claude review: log operation and exception type only, preserve sanitized return semantics, and check each catch for safe return/log wiring. Document measured hostname limits without broadening the matcher; add module-local redaction and diagnostic-preservation coverage. Antriksh original implementation remains credited separately.
* ci: build PR batches on dev
* Allow Claude diff reviews after approved fork builds (#413)
* fix(browser): preserve newer clipboard copies during plain-text paste (#316)
* fix(browser): paste-without-formatting no longer clobbers a copy made in its restore window (#205)
* fix(browser): make paste-without-formatting's restore identity-based, not text-based
Review of this PR caught that the text-equality guard cannot distinguish 'our write is still current' from 'a previous press already restored the rich original' - whose string projection is by construction the same text - so two presses inside the 200ms window permanently downgraded a rich clipboard to plain text. A PasteWithoutFormattingSession now tracks the exact Transferable each press installed and restores the pre-window original once, while a user copy in the window still wins. Adds five tests to BrowserClipboardCommandsTest, including the double-press regression; wraps the two over-limit lines; e.message -> e::class.simpleName in the touched handlers.
* fix(browser): fold tryRestore's early exits so it stays within detekt's ReturnCount
* fix(browser): blank lines between FakeTransferable overrides per ktlint
* test(browser): retain clipboard restore scenarios from #408
* fix(browser): track clipboard ownership through AWT wrappers
---------
Co-authored-by: rehannayeem0786 <rehannayeem0786@github.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
Co-authored-by: Antriksh1984 <antrikshsingh850@gmail.com>
* fix(plugins): serialize concurrent first loads per class name (#324)
* fix(plugins): prevent duplicate class definitions during concurrent loads
* fix(plugins): make class loading parallel-safe
* test(plugins): pin concurrent loading lifecycle boundaries
Maintainer consolidation follow-up: retain #324 synchronization and its tests; add concurrent shared-name, superclass, and unload-marker coverage. Document caller-sensitive registration based on arjun28115 review feedback and clarify that marking unload does not drain an already admitted parent lookup. No global lifecycle lock or resource-policy changes.
---------
Co-authored-by: sgoel2be24-cyber <223222024+sgoel2be24-cyber@users.noreply.github.com>
Co-authored-by: Procs <168113425+ProportanilityConstant@users.noreply.github.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(browser): defer native disposal until admitted calls drain (#312 + #409) (#412)
* fix: make BrowserHandle JavaScript execution cancellable
the previous synchronous JxBrowser call blocked the Main/EDT thread;
coroutine cancellation could not take effect while that call was blocking;
the implementation now uses JxBrowser’s asynchronous JavaScript callback API;
suspendCancellableCoroutine makes the Kotlin waiting side cancellable;
late callbacks after cancellation are safely ignored;
this does NOT terminate JavaScript already executing inside Chromium.
* fix: add robust native call lifecycle tracking for disposal
* test: verify native operation cleanup on synchronous failure
* fix: linearize browser disposal and pending operation tracking
* fix: resolve detekt violations in browser native tracking
* test: satisfy detekt failure simulation rule
* fix: close native operation disposal race
* fix(browser): drain an in-flight call before closing the browser
Issue #300 reports that executeJavaScript is a synchronous, non-
cancellation-aware native call, and a plugin's own withTimeoutOrNull
around it can only abandon the caller's wait - not the call itself -
which stays running on Main indefinitely and can race a concurrent
handle.dispose(), touching the same native object from two threads at
once.
The severe half of this was already fixed by BoundedBrowserCall: this
class confines every blocking round trip to one dedicated daemon thread
instead of Dispatchers.Main, so a wedged renderer no longer freezes the
app. What was not yet closed is the residual window this issue's core
report is actually about: shutdown() stopped new work but did not wait
for whatever was already running, so a caller could proceed straight to
browser.close() while a call from just before shutdown was still
finishing on the dedicated thread.
shutdown() now waits, bounded, for that in-flight call to drain before
returning. Costs nothing in the common case - awaitTermination returns
immediately once the one worker thread and its queue are both idle,
which is where an instance sits between calls - and only the bound in
the rare case something was genuinely still running.
Known, and disclosed in the KDoc rather than claimed away: this narrows
the race for the common case (a call that was always going to finish
quickly) rather than closing it. It cannot help when the in-flight call
is itself the wedge this class's own deadline exists for - waiting
unboundedly for a genuinely stuck call would reintroduce the exact freeze
BoundedBrowserCall exists to prevent, just moved from a plugin's await
into every caller's teardown path. There is no JxBrowser API on this
version able to interrupt a blocking round trip already inside the
native call, which is the whole reason the call is confined to its own
thread instead of cancelled - closing that window completely needs
JxBrowser's own cooperation, which the issue's own analysis already
concluded is unavailable.
Tests: shutdown waits for a fast in-flight call to finish before
returning, and does not wait past its own drain timeout for a wedged one
- the same before/after pair the rest of this test file already uses for
BoundedBrowserCall's deadline.
* fix(browser): defer native close until admitted calls drain
Consolidates the lifecycle work from #312 (johncybersage) and #409 (Antriksh1984), retaining both original histories. Replaces success-only callback accounting and timeout-then-close with executor termination signals and host-owned native/profile cleanup. These corrections and replacement tests are maintainer/agent work, separate from contributor scoring.
* fix(browser): bound profile waits and clarify deferred cleanup ownership
Address consolidation review: schedule both service disposal paths without awaiting native close, diagnose pending drains, centralize executor ownership, and bound profile lease acquisition without permitting unsafe reuse or eviction. Document process-exit abandonment and verify wiring and cleanup ordering. Maintainer/agent follow-up, separate from original #312 and #409 contributor credit.
* fix(browser): schedule disposal after early UI teardown failure
* ci: create launcher directory before Claude native installation
* Revert "ci: create launcher directory before Claude native installation"
This reverts commit 5184908e6c3f133502508513b4c5e17651b13bc0.
* test: keep warmup registry ordering check on one event loop
* test(browser): normalize CRLF in disposal source wiring guards
---------
Co-authored-by: john k <johnk@johns-MacBook-Pro.local>
Co-authored-by: john k <johnk@syn-172-100-137-112.res.spectrum.com>
Co-authored-by: Antriksh1984 <antrikshsingh850@gmail.com>
* feat(tabs): show a speaker glyph on tabs that are playing audio (#314)
* feat(tabs): show a speaker glyph on tabs that are playing audio (#308)
* fix(tabs): review round - compile, registry lifetime, UI-thread marshalling, owner flush
1. graphicsLayer import dropped (does not exist at that package); the fade now uses .alpha(audioAlpha), already imported. 2. TabAudioRegistry no longer outlives its owner: disposeAllTabsBlocking unregisters everything this component registered, and DetachedTab.destroy drops the entry for a tab destroyed without adoption - the handler captures the BossTabsComponent, so stale entries retained it. 3. The registry update is marshalled to the EDT via SwingUtilities.invokeLater, so the tab-model mutation happens on the UI thread as the KDoc claims. 4. setFullscreenHandler flushes the current playback state once ownerTabId becomes known, closing the starts-before-registration race the review could not verify. 5. TabAudioRegistryTest added: delivery, last-writer-wins, ownership-checked unregister.
* fix(tabs): CI round 2 - DetachedTab receiver error, long line, and baseline-preserving signatures
- BossMainWindowPanel.kt:2098: DetachedTab is a non-inner nested class and cannot use the outer component as receiver; the audio-handler drop moves to detachTab instead, which is where adoption-vs-destroy actually diverges. - TabAudioRegistry.kt:9 wrapped under detekt's 120-char limit. - FluckTabInfo.isPlayingAudio and BossTabButton's glyph read no longer change any signature: the flag is a mutable property (copied in copy()'s body) and the button reads it off tabInfo, so the constructor, copy() and BossTabButton keep their exact baseline-frozen detekt IDs and the resurfaced LongMethod/LongParameterList/CyclomaticComplexMethod findings retire.
* fix(tabs): fold handleAudioUpdate within ReturnCount; add missing final newline in the test file
* fix(tabs): publish audio state by browser ownership and keep icon layout stable
---------
Co-authored-by: rehannayeem0786 <rehannayeem0786@github.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* feat: connect chrome density settings and keyboard controls (#305)
* feat: wire the chrome density scale into settings and small-screen defaults
ChromeDensity / ChromeDimens.of() / LocalChromeDimens already existed on
main and every bar (BossTitleBar, BossTopBar, the main tab bar,
BossBottomBar) already read its height from BossChrome.dimens - but
LocalChromeDimens was never provided anywhere, so it silently resolved to
its staticCompositionLocalOf default (Comfortable) always. The scale was
built and unreachable.
This closes that gap:
- WindowAppearanceSettings gets a `density` field (default COMFORTABLE, so
an existing install's settings file - which has never heard of this key
- decodes to exactly the chrome it already had; no settings-version bump
needed).
- BossApp.kt now provides LocalChromeDimens from that field, host-only and
outside BossAppCompositionLocals (plugins never draw host chrome).
- A "Density" dropdown in Settings > Bars (Compact/Comfortable/Spacious),
indexed in Settings search.
- A fresh install on a small screen (< 1000dp logical height, clearing the
13" MacBook Air's ~931-956pt from #239's own measurements) now defaults
to Compact and starts with the bottom bar off - the one bar this manager
can still reclaim itself, since the side strips are already off by
class default. Toolkit.getScreenSize() is read once, wrapped in
runCatching (HeadlessException off a display must not break a fresh
install), with a pure defaultDensityFor(screenHeightDp) so the decision
is unit-testable without a display.
Addresses #239, scoped down from the full issue. Out of scope in this
pass, deliberately:
- The app does not yet report its own chrome budget as a measured
percentage - the issue's own "state it, don't argue it" ask. Reachable
later from the same ChromeDimens source, but needs a spot to render it.
- The title bar's 27dp "Boss Console" label row is untouched - collapsing
or merging it with the tab bar is a separate, riskier layout change than
a density scale.
- No regression intended to the existing per-bar show/hide switches or
focus-mode edges: density only changes how much room a bar that is
already on screen takes, never whether it is shown.
* fix: drop the Compact-density bottom-bar auto-off, wrap an overlong line
Address review feedback on #305:
- Removed showBottomBar = density != ChromeDensity.COMPACT from
getDefaultSettings(). ChromeMetricsTest already shows the shipped
macOS defaults reach 93.3% of a 931dp window at Comfortable, so
#239's >=90% bar is met without touching the status bar - and that
bar's own KDoc argues it must stay on ("the only always-on readout").
It was also reachable from the settings-load catch fallback, so a
transient read error could silently drop the bar and have the next
save persist the loss. Compact now only changes density, nothing else.
- Wrapped primaryScreenHeightDp() - was 121 chars, one over detekt's
default MaxLineLength (120), which applies here since .editorconfig's
140 is a ktlint-only setting and this line wasn't in the baseline.
* feat: add chrome density setting
* feat: add small-screen chrome defaults
* feat: add chrome density controls and small-screen defaults
* fix(ui): preserve lean density defaults and test density controls
* style: format multiline screen-height lookup
* fix(ui): separate density recovery and make editor chords opt-in
* style: wrap density default documentation
---------
Co-authored-by: AdityaK-iiita <adityakolate0070@gmail.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(plugins): stop host resource fallback after unload (#325)
* fix(plugins): stop host resource fallback after unload
* fix(plugins): preserve resource warning for a missing result
---------
Co-authored-by: sgoel2be24-cyber <223222024+sgoel2be24-cyber@users.noreply.github.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* Handle closed browser frames during navigation callbacks (#400)
* Handle closed browser frames during navigation callbacks
* test(browser): retain closed-event guard after navigation integration
* fix(browser): preserve helpers on PID failure and recognize closed transports
* style(browser): wrap review diagnostics for detekt
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* refactor(window): pin the panel content alignment width (#398)
* fix(window): restore strict width constraints for nested panels
Fixes #278. Commit c15660c5 inadvertently dropped the explicit width constraint for nested panels in the right dock by wrapping them inside a PanelColumn with a Box(weight(1f)) that lacked fillMaxWidth(). This loose constraint caused a known issue with SubcomposeLayout caching stale bounds when resized, resulting in Row content disappearing after weighted Spacers. This commit restores the .fillMaxWidth() propagation.
* Cover panel content alignment across width changes
* Keep panel layout regression in the existing app test package
* Document panel alignment contract and pin filling content width
* Wrap the panel width assertion to satisfy detekt
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* #320 - Fix quick actions vanishing in short tab rail (#328)
* #320 - Fix quick actions vanishing in short tab rail
* Fix: detekt & ktlint checks
* Fix rail action budgets and reversible fallback wiring
* Fix resize test assertion import and update rail coverage note
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(crash): refine scroll boundary and bound error sanitization (#350)
* fix(crash): refine scroll boundary and bound error sanitization
* Avoid exposing a partial token at the crash message limit
* Document sanitizer input bound and verify expansion ordering
---------
Co-authored-by: Aishwary Anand <aishwary.cd22@bmsce.ac.in>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(logging): redact private hostnames (#346)
* fix(logging): redact private hostnames
* Handle private hostname punctuation and preserve diagnostic ports
* Format hostname punctuation regression assertion
---------
Co-authored-by: ayush <workside@gamil.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* feat: Opening an .html file should ask open as a file or webpage (#333)
* feat: Opening an .html file should ask open as a file or webpage
* Fix HTML prompt delivery and persisted preference ordering
* Cover HTML routing and correct review test fixtures
* Honor HTML navigation intent and release pending prompts on failure
* Isolate HTML prompt handling from the general dialog host
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(plugin-store): parse timestamps consistently in list and detail metadata (#379)
* Fix: Implement proper ISO timestamp parsing in PluginStoreClient
* fix(repository): implement robust ISO timestamp parsing for Supabase payloads
* style(repository): resolve detekt MaxLineLength and TooGenericExceptionCaught
* fix(plugin-store): implement plugin metadata timestamp parsing
PluginStoreClient.parseTimestamp was a stub that always returned 0L, so
every plugin fetched from the store showed "Last Updated"/"Published" as
the Unix epoch in the Toolbox UI regardless of what the server actually
sent (#337).
Parses the ISO-8601 timestamp (the shape the store sends, e.g.
2024-05-12T14:30:00Z, or with a numeric offset) via java.time.Instant,
falling back to 0L for a blank, missing, or malformed string rather than
throwing - matching the field's own empty-string default for a response
that omits it. java.time rather than adding kotlinx-datetime as a new
dependency: this module already uses java.util.* directly in the same
commonMain source set (it has one real target, jvm("desktop")), so nothing
is gained by introducing a second time library for one function.
Tests exercise the real PluginDetailResponse.toPluginInfo(), matching the
convention already established next to it in
PluginStoreResponseDecodingTest: a Z-suffixed timestamp, a numeric-offset
timestamp, a blank string, and a malformed string.
* Fix timestamp normalization edge cases and verify both response mappings
* Use release publication dates and pin timezone-independent fallback
---------
Co-authored-by: Sanjay <sanjaysaini4423@gmail.com>
Co-authored-by: Antriksh1984 <antrikshsingh850@gmail.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* feat(remote-ui): authenticate and expose remote UI surfaces (#348)
* feat(remote-ui): authenticate remote UI process identity
* feat(remote-ui): place authenticated remote UI surfaces
* fix(remote-ui): close ownership and placement lifecycle races
* test(remote-ui): expose placement recovery cases to JUnit
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* feat(dashboard): add shared read-only What's New release feed (#381)
* Add What's New release feed to dashboard
* Serialize update settings persistence
* Remove unused update settings import
* fix(updater): retain realtime refreshes during shared release fetches
* style(updater): format shared release fetch regression tests
* fix(updater): coalesce release refresh bursts and preserve visible history
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(language): consolidate host and out-of-process language-id tables (#358)
* fix(language): consolidate host and out-of-process language-id tables
EditorLanguages (composeApp) and EditorServiceImpl (modules/boss-app-editor)
each hand-maintained their own extension-to-language-id table because
boss-app-editor - a plain JVM module compiled to a GraalVM native image -
could not depend on anything in composeApp. They disagreed: EditorServiceImpl
named .sh/.bash/.zsh "shell", EditorLanguages named them "bash", and
EditorServiceImpl was missing more than forty ids the other table had
(fortran, delphi, latex, lisp, tcl, clojure, batch, diff, ...) (#75).
Moves the table into a new plugin-platform/plugin-language-types module -
dependency-free by design, so it carries nothing extra into boss-app-editor's
native-image build - and has both EditorLanguages and EditorServiceImpl read
from it. This is the first time anything under modules/ depends on
plugin-platform/; verified compiling and passing tests before committing to
the approach.
EditorLanguages keeps its existing public API unchanged (same values for
every extension), so FileTypeCategoriesTest and EditorLanguageDetectionTest
needed no changes. EditorServiceImpl keeps its own "proto"->"protobuf" entry
and "plaintext" fallback as local additions on top of the shared table,
rather than folding them into it: proto isn't part of the boss-file-types.json
default-app-association surface the shared table backs, and "plaintext" is
this service's own gRPC default, not a value composeApp reads.
Adds LanguageIdsTest (the new module) and EditorServiceImplTest (previously
no tests existed for this class at all) - the latter pins the actual bug fix,
that detectLanguage("sh") now returns "bash" instead of "shell".
Out of scope, and disclosed rather than silently dropped: two of the five
duplicate tables the issue names - the editor-tab plugin's own
LanguageDetection and BossEditor's lexer registry - live in separate
repositories this build cannot reach. FileIcons.forSpecialFileName/forFile in
plugin-icons is also not consolidated: it keys icon selection off the raw
filename to preserve distinctions a language id can't carry (package.json,
yarn.lock and pnpm-lock.yaml are all valid JSON/YAML but get three different
brand icons) and never computes a language id in the first place, so
routing it through this table would need a separate icon-selection redesign.
* fix(language): keep file-association drift checks on the shared table
* fix(language): apply shared filename rules to editor file opens
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(editor): contain write-local stack overflow and preserve fatal errors (#396)
* Make writeFileContentSafe actually safe: catch the Errors it was named for
`writeFileContentSafe` exists to return false rather than throw. It did not hold
that contract:
} catch (e: Exception) {
fileIoLogger.warn(LogCategory.EDITOR, "Error writing file", error = e)
false
}
StackOverflowError is an Error, not an Exception, so it went straight through -
out of writeFileContent, out of the editor_write_file MCP handler, and to the
caller as a bare StackOverflowError instead of the handler's own
"Write failed for <path>". The warn that would have named the file never ran
either.
That is not hypothetical. risa-labs-inc/boss-plugin-editor-tab#18 and #27 are two
independent reports of it, on unrelated content, and #27's complaint is precisely
the missing diagnosis: the error "gives no indication of which input caused it".
StackOverflowError and OutOfMemoryError are caught explicitly rather than
widening to Throwable. A stack overflow unwinds and leaves the JVM usable, and
readFileContentSafe in this same file already treats OutOfMemoryError as a
reportable outcome. Throwable would also absorb LinkageError and ThreadDeath,
which are not this function's to swallow - there is a test asserting a
NoClassDefFoundError still propagates.
The failure log now names the path and the character count. Not the content:
these writes carry whatever the user is editing.
**This does not fix the write.** Whatever recurses is upstream of this function,
which is mkdirs plus writeText - I ruled out the plugin's handler, this write,
and the host's MCP argument parsing, and said so on the issue. What changes is
that the failure is reported honestly instead of escaping, which is where a
diagnosis can start.
The body moves to `guardedWrite` with an injectable write, because a test JVM
cannot be made to overflow the stack inside writeText on demand.
Verified: 3729 tests, 0 failures, plus ktlintCheck and detekt.
Refs risa-labs-inc/boss-plugin-editor-tab#18, #27.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(editor): clarify write guard scope and prior recursion fix
* fix(editor): preserve fatal heap errors in write guard
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(state): apply version-checked JSON merge patches (#361)
* Fix: Implement proper ISO timestamp parsing in PluginStoreClient
* Feature: Implement JSON Merge Patch for plugin delta state synchronization
* fix(state): validate delta bases and exclude duplicate timestamp scope
* fix(state): reject malformed and deeply nested patches
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* :arrow_up: deps:(deps): Bump grpc from 1.83.1 to 1.84.0 (#363)
Bumps `grpc` from 1.83.1 to 1.84.0.
Updates `io.grpc:grpc-netty` from 1.83.1 to 1.84.0
- [Release notes](https://github.com/grpc/grpc-java/releases)
- [Commits](https://github.com/grpc/grpc-java/compare/v1.83.1...v1.84.0)
Updates `io.grpc:grpc-protobuf` from 1.83.1 to 1.84.0
- [Release notes](https://github.com/grpc/grpc-java/releases)
- [Commits](https://github.com/grpc/grpc-java/compare/v1.83.1...v1.84.0)
Updates `io.grpc:grpc-stub` from 1.83.1 to 1.84.0
- [Release notes](https://github.com/grpc/grpc-java/releases)
- [Commits](https://github.com/grpc/grpc-java/compare/v1.83.1...v1.84.0)
Updates `io.grpc:grpc-services` from 1.83.1 to 1.84.0
- [Release notes](https://github.com/grpc/grpc-java/releases)
- [Commits](https://github.com/grpc/grpc-java/compare/v1.83.1...v1.84.0)
Updates `io.grpc:grpc-util` from 1.83.1 to 1.84.0
- [Release notes](https://github.com/grpc/grpc-java/releases)
- [Commits](https://github.com/grpc/grpc-java/compare/v1.83.1...v1.84.0)
---
updated-dependencies:
- dependency-name: io.grpc:grpc-netty
dependency-version: 1.84.0
dependency-type: direct:production
update-type: version-update:semver-minor
- dependency-name: io.grpc:grpc-protobuf
dependency-version: 1.84.0
dependency-type: direct:production
update-type: version-update:semver-minor
- dependency-name: io.grpc:grpc-stub
dependency-version: 1.84.0
dependency-type: direct:production
update-type: version-update:semver-minor
- dependency-name: io.grpc:grpc-services
dependency-version: 1.84.0
dependency-type: direct:production
update-type: version-update:semver-minor
- dependency-name: io.grpc:grpc-util
dependency-version: 1.84.0
dependency-type: direct:production
update-type: version-update:semver-minor
...
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* :arrow_up: deps:(deps): Bump protobuf from 4.36.0 to 4.36.1 (#364)
Bumps `protobuf` from 4.36.0 to 4.36.1.
Updates `com.google.protobuf:protobuf-kotlin` from 4.36.0 to 4.36.1
Updates `com.google.protobuf:protobuf-java` from 4.36.0 to 4.36.1
- [Release notes](https://github.com/protocolbuffers/protobuf/releases)
- [Commits](https://github.com/protocolbuffers/protobuf/commits)
---
updated-dependencies:
- dependency-name: com.google.protobuf:protobuf-kotlin
dependency-version: 4.36.1
dependency-type: direct:production
update-type: version-update:semver-patch
- dependency-name: com.google.protobuf:protobuf-java
dependency-version: 4.36.1
dependency-type: direct:production
update-type: version-update:semver-patch
...
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(plugins): defer stale jar cleanup until restart (#340)
* fix(plugins): defer stale jar cleanup until restart
* fix(plugins): satisfy quality checks
* Preserve staged plugin artifacts when another plugin updates
* Require cleanup intent and retain artifacts with unordered manifest versions
---------
Co-authored-by: ayush <workside@gamil.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(plugins): defer fluck-browser hot-reload to a restart (#352)
* fix(plugins): defer stale jar cleanup until restart
* fix(plugins): defer fluck-browser hot-reload to a restart
Hot-reloading fluck-browser force-unloads the classloader that created its
JxBrowser native views. Every open tab - and every tab opened afterward,
since the factory that would recreate the view is gone too - draws an empty
box, and a second hot-reload does not recover it; only a full restart does.
HotReloadPolicy names plugins that must never be swapped in place, matching
the existing NOT_USER_INSTALLABLE/RetiredPlugins pattern. It's checked at
every path that can force-unload a live plugin to apply an update: the
resetPluginInstances/doReloadPlugin path that actually reproduced this bug,
the menu-driven Reload action, the Toolbox update flow, and the store
installer. For a not-hot-reloadable plugin, the new jar is staged into
installed.json without touching the running instance or its open tabs, and
a status message tells the user to restart to apply it - the next cold
start loads the update with a fresh classloader.
Also fixes two call sites that asked "restart dependent plugins?" before
checking whether an unload was even going to happen, which produced a
confusing prompt for an unload that a deferred update was never going to
do.
* fix(plugins): satisfy quality checks
* Preserve staged plugin artifacts when another plugin updates
* Validate deferred browser artifacts and reject ineffective downgrades
* Require cleanup intent and retain artifacts with unordered manifest versions
* Preserve unloaded-browser recovery and unify safe deferred selection
* Format scoped update cleanup integration
---------
Co-authored-by: ayush <workside@gamil.com>
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* Harden OOP plugin reaping: hook-race flag, registry cleanup, spawn guard (#135) (#365)
* Harden OOP plugin reaping: hook-race flag, registry cleanup, spawn guard (#135)
reapDepth (AtomicInteger) replaces the boolean reaping flag, so the two JVM shutdown hooks (main.kt and KernelBootstrap) cannot clear the in-progress signal while the other is still reaping (item 2). reapChildren now unregisters reaped handles from the process-wide ProcessRegistry, so stale dead entries do not persist across an in-process mode switch (item 4). OutOfProcessPluginSpawnerImpl.spawn() stands down when isReaping(), so a plugin load racing host exit cannot register a child after the reap snapshot (item 3). Covered by ReapChildrenTest.
* Serialize plugin spawn registration with reaping and preserve replacement handles
* Fence late process creation without blocking shutdown and cover recovery
---------
Co-authored-by: Shivang <shivang.iitk@gmail.com>
* fix(plugins): complete loader cleanup after disposal linkage errors (#302)
* fix: API hot swap during startup
* fix: workflows
* resolved comments by maintainer
* fix CI and buil…
…a-labs-inc#635) * fix(kernel): require verified caller identity for the Git bridge GitServiceBridge had no caller-identity check on any of its fourteen RPCs. Any process able to open a connection to the kernel IPC server - not only the plugins the host itself loaded - could read the open project's full commit history and working-tree status, and could discardChanges (destroying uncommitted work), checkout, cherryPick or revert against the project's real git repository with no credential at all. Gate every RPC behind authenticatedCallerOrRefuse(), the same PERMISSION_DENIED-on-no-identity pattern PluginUIServiceBridge established for BossConsole#53. The four watch streams (file status, commit log, is-repo, is-loading) capture identity synchronously before returning and re-check it on every emission, so a revoked token stops a live stream rather than only new calls - matching the shape PR risa-labs-inc#550's review settled on for this bridge's siblings. * style(tests): wrap chained builder calls in GitServiceBridgeTest Real CI's ktlintDesktopTestSourceSetCheck flagged chain-method-continuation violations (a local run had missed this) - wrap the affected builder chains onto their own lines, per ktlintFormat. No behavior change. * test(kernel): cover revoked callers and log stream refusals --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>
…a-labs-inc#645) * fix(kernel): require verified caller identity for the Log bridge LogServiceBridge had no caller-identity check on any of its nine RPCs. Any process able to open a connection to the kernel IPC server - not only the plugins the host itself loaded - could watchLogs to see the live stdout/stderr stream of the running BOSS process, or call exportLogs to pull its full history, with no credential at all. Unlike a bridge whose values are deliberately routed through LogSanitizer at the call site, nothing about this stream is scrubbed for IPC exposure - it is whatever every logger.info/warn/error call site across this large and growing codebase happened to write, which is not a guarantee that none of it is a stack trace, a file path, or something else nobody meant to expose to an arbitrary local process. Gate every RPC behind authenticatedCallerOrRefuse(), the same PERMISSION_DENIED-on-no-identity pattern PluginUIServiceBridge established for BossConsole#53. The four watch streams capture identity synchronously before returning and re-check it on every emission, matching the shape PR risa-labs-inc#550's review settled on for this bridge's siblings. * test(kernel): verify revocation and diagnose stream refusals --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>
…abs-inc#647) * fix(security): randomize the IV for encrypt_text/decrypt_text encrypt_text called the 3-arg extensions.encrypt(plaintext, key, 'aes'), which pgcrypto documents as using an all-zero IV, so encryption was a pure function of (plaintext, key) alone: two equal passwords produced byte-identical ciphertext. Because create_secret encrypts server-side and RLS lets a user read their own password_encrypted back, any signed-in user was an encryption oracle - grouping a dump by ciphertext names every set of users sharing a password, with no key required. The same defect applied to recovery_codes_encrypted and twofa_secret. encrypt_text now draws a fresh random 16-byte IV per call and stores 'v2:' || base64(iv || ciphertext); decrypt_text dispatches on that prefix and falls back to the original zero-IV path for anything not yet migrated, so this deploys without a flag day. A backfill converts existing rows in all three columns, guarded by NOT LIKE 'v2:%' so a partial or repeated run is a no-op. Deliberately scoped to this one defect (BossConsole#618 names three). Defect 2 (the encryption key truncated to 32 string characters instead of 32 bytes) is not touched here: ops/rotate_master_encryption_key.sql records that a deployed key may be hex or base64, and this migration cannot tell which is live in any given project's Vault - guessing wrong would make get_encryption_key()'s result unusable and break encryption for every row on deploy. Defect 3 (no MAC/AEAD) needs its own wire-format review. ops/rotate_master_encryption_key.sql itself will need a follow-up before it can run again, since it manipulates raw ciphertext bytes assuming the old no-IV, no-prefix layout - both gaps are documented in the migration's own header. This could not be run against a real Postgres/pgTAP instance in this environment (no supabase/docker CLI available here) - the added test was written and reviewed by hand, and hosted CI's Database (pgTAP) check is the actual verification gate. * fix(security): un-qualify substring(... from ...) so it parses CI's Database (pgTAP) check caught what local review couldn't (no Postgres available in this sandbox): pg_catalog.substring(x from y) is a syntax error. substring's SQL-standard FROM/FOR trailing syntax is recognized by the parser only on the bare function name - schema-qualifying it makes PostgreSQL treat it as an ordinary call, where FROM is not a valid argument separator. 20260909000000_encrypt_totp.sql's own decrypt_text already calls substring unqualified for exactly this reason; this migration's three call sites (envelope/iv/body extraction in decrypt_text, and the twofa_secret backfill) now match that convention. Both functions' search_path already resolves the unqualified name to pg_catalog's substring, so nothing else about name resolution changes. * fix(security): make key rotation understand the v2 IV envelope CI's Database (pgTAP) check caught what local review couldn't (no Postgres available in this sandbox): the pre-existing master_key_rotation_test.sql runs ops/rotate_master_encryption_key.sql against real fixture data, and that script's re-encrypt loop calls extensions.decrypt/encrypt directly on raw ciphertext bytes, assuming the old zero-IV, no-prefix layout. The 'v2:' prefix the previous commit's encrypt_text now writes is not valid base64, so decoding it raised - exactly the gap this migration's own header comment already named as a follow-up, just sooner than expected. Added a pg_temp helper, rotate_ciphertext, that re-encrypts a mapped column's value under the new key while preserving whichever envelope it already carries: a 'v2:' row is decrypted with decrypt_iv under the old key and re-encrypted with encrypt_iv under the new key AND a fresh IV (a function rather than an inline SQL expression, specifically so gen_random_bytes(16) is evaluated once and reused for both the encrypt_iv call and the stored prefix - two calls would produce two different values and corrupt the row). A legacy row - every column before this pair of migrations, and still qbo_token_state / google_token_state going forward, since those are untouched by the IV migration - stays zero-IV, matching what it already was. Rotation's contract stays "same shape, new key"; upgrading a row's shape is the encryption migration's job, not rotation's. Updated the IV migration's own header comment, which had flagged this as a still-open follow-up, to point at the fix now landing alongside it instead. * fix(security): preserve plaintext bytes during IV upgrade * fix(security): make IV backfill atomic and diagnose unreadable rows --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>
Rushikeshiitb
force-pushed
the
fix/secret-listing-resilient-decrypt
branch
from
September 14, 2026 08:44
77ad33f to
7e72f12
Compare
…ression coverage Preserve the additional SQL listing cases from PR risa-labs-inc#652 by Akash Jadon. Original resilient listing implementation by Rushikeshiitb remains in preceding commits. Co-authored-by: Akash Jadon <contact@evolnex.digital>
Contributor
|
@claude Please review the current head against current dev, focusing on the narrowed corrupt-row exception boundary versus systemic key/configuration failures, unchanged authorization, and migration compatibility with merged #647/#651. The older null-client finding is resolved by SQL COALESCE for installed clients. Additional SQL coverage from duplicate #652 is retained with contributor credit; synthetic local validation is running. |
|
Claude finished @kshivang's task in 8m 55s —— View job Review: narrowed corrupt-row boundary, authorization, #647/#651 compatibility
The previously blocking finding is genuinely resolved, and resolved at the right layer. The exception boundary is now narrowed and, more importantly, tested. Hoisting I could not run Gradle or pgTAP in this job (no database, no runner), so everything below is static analysis plus reading the suites. Findings - none blockingA.
|
Contributor
|
@Rushikeshiitb Your contributor update was reassessed: the rebased 7e72f12 head had all ten checks green, so the previous queue-only conclusion was not carried forward. The per-field listing recovery and original tests remain credited to you. @akashjadon04's equivalent #652 is now consolidated here with their complete SQL suite and explicit description/coauthor credit. I pushed current-dev integration and repair head fd3d903 to your original branch. The substantive remaining catch-all issue is repaired: key lookup/encoding/empty-key checks happen outside the row fallback, and privilege, missing-function and internal errors propagate. Invalid ciphertext/base64/UTF-8 and pgcrypto decrypt failures still blank only the field. A valid-but-wrong key cannot be identified reliably with #647's unauthenticated formats; this limitation is explicitly documented. The migration retains its unique 20260915000000 version and current dev's #647/#651 migration fixes. Validation: 872 assertions across 36 disposable PostgreSQL suites passed, plus three focused probes for truncated v2, invalid padding and malformed key encoding. Substituting the previous catch-all helper fails all five new systemic-error assertions. Fixture generation/check and whitespace checks pass. Current-head hosted database, code-quality, compatibility, shell, version and AI-contract checks pass. No live database/app/secret operation occurred. The remaining blocker is incomplete validation/review, not an identified unresolved author defect: current-head Linux/Windows builds are in progress and macOS is queued; build-summary is not complete (https://github.com/risa-labs-inc/BossConsole/actions/runs/34830779841). The local focused desktop plus ktlintCheck/detekt invocation waited 180 seconds at the shared build lock with zero Gradle output, then only that invocation was canceled. Its tests are not claimed executed. The single current Claude review request is still running (https://github.com/risa-labs-inc/BossConsole/actions/runs/34830756252); earlier findings were addressed, but this unfinished review is not a clean approval. Next steps: complete current-head platform CI and summary, inspect the requested Claude result and resolve any valid findings with evidence, then refresh dev and re-evaluate integration. Run the focused SecretDecodingTest through with-build-lock.py if it is not covered by completed current-head CI. Before merging, use with-dev-merge-lock.py, refresh PR/dev state under the lock, require open/non-draft/dev plus passing current-head gates, and squash with --match-head-commit for the final reviewed SHA. No bypass/admin merge or main merge. Manual synthetic visual checks remain list/search/pagination, blank corrupt password, read-only direct share/source and disabled blank-password Save; no visual pass is claimed. Per the requested bounded-review disposition policy, I am converting this to draft until these gates complete. Please treat this as a temporary CI/review handoff, not a contributor correctness downgrade. No repeated Claude request or prolonged polling loop was started. |
kshivang
marked this pull request as draft
September 14, 2026 10:03
kshivang
force-pushed
the
dev
branch
2 times, most recently
from
September 14, 2026 11:16
cd88490 to
5a2e818
Compare
- Widen try_decrypt_text handler to the data_exception / external_routine_invocation_exception condition names (explicit class match, covers 22P05); systemic faults still propagate. - Correct the migration header runbook: signal is an empty password plus the SQLSTATE-only server warning, never a NULL after the RPC COALESCE. - Note password_encrypted stays mapped to strict decrypt_text in the rotation script on purpose (rotation must fail closed). - Cross-reference the two overlapping pgTAP suites; credit Akash Jadon (risa-labs-inc#652). - Add a SecretDecodingTest case asserting a null password fails to decode, which is why the listing RPCs COALESCE a failed decryption to an empty string.
…esilient-decrypt # Conflicts: # modules/boss-ipc/src/main/kotlin/ai/rever/boss/ipc/auth/ProcessIdentityInterceptor.kt
kshivang
added a commit
to Antriksh1984/BossConsole
that referenced
this pull request
Sep 15, 2026
…ailures Includes desktop regression coverage from risa-labs-inc#621 by Rushikeshiitb and review repairs by kshivang; related independent risa-labs-inc#652 work by akashjadon04 informed the review.
kshivang
added a commit
that referenced
this pull request
Sep 15, 2026
…668) * fix(secrets): make the secret listing RPCs fail soft on one bad row BossConsole#620: get_user_secrets, search_user_secrets and get_user_secrets_with_shared each decrypt password_encrypted and recovery_codes_encrypted with a bare call to decrypt_text inside a set-returning query. decrypt_text raises on an undecryptable value - a wrong key after a partial key rotation, storage corruption, or an HMAC/padding mismatch - and one raised row aborts the whole query, so the caller sees every one of their secrets fail to load rather than just the bad one. twofa_secret already reads through safe_decrypt_twofa_secret (NULL on failure). recovery_codes_encrypted already had a safe wrapper too - safe_decrypt_recovery_codes, whose own doc comment says "used internally by get_user_secrets functions" - but whichever later migration touched these RPCs stopped calling it and fell back to raw decrypt_text, so it sat unused. password_encrypted never had a safe wrapper at all. Adds try_decrypt_text(text), mirroring safe_decrypt_recovery_codes's BEGIN/EXCEPTION WHEN OTHERS THEN RETURN NULL shape for a scalar result, locked down the same way safe_decrypt_twofa_secret is (no client role gets EXECUTE). Routes the password read in all three RPCs through it and the recovery-codes read back through the existing safe wrapper. One corrupt row now surfaces as a blanked field - password NULL, recovery_codes [] - while every other secret in the page still loads. Deliberately not named safe_decrypt_*: ops/rotate_master_encryption_key.sql's re-encrypt loop treats every function matching that name pattern as a mapped bespoke-format adapter and refuses to run if one is unmapped. try_decrypt_text wraps the same plain envelope that script already re-encrypts directly, so it must stay outside that sweep. Scoped to the three user-facing listing RPCs only; the operator-run backfill/rotation paths keep using raw decrypt_text and should still fail closed. * Fix installed-client compatibility and preserve systemic decryption failures Includes desktop regression coverage from #621 by Rushikeshiitb and review repairs by kshivang; related independent #652 work by akashjadon04 informed the review. --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>
kshivang
added a commit
that referenced
this pull request
Sep 15, 2026
…668) * fix(secrets): make the secret listing RPCs fail soft on one bad row BossConsole#620: get_user_secrets, search_user_secrets and get_user_secrets_with_shared each decrypt password_encrypted and recovery_codes_encrypted with a bare call to decrypt_text inside a set-returning query. decrypt_text raises on an undecryptable value - a wrong key after a partial key rotation, storage corruption, or an HMAC/padding mismatch - and one raised row aborts the whole query, so the caller sees every one of their secrets fail to load rather than just the bad one. twofa_secret already reads through safe_decrypt_twofa_secret (NULL on failure). recovery_codes_encrypted already had a safe wrapper too - safe_decrypt_recovery_codes, whose own doc comment says "used internally by get_user_secrets functions" - but whichever later migration touched these RPCs stopped calling it and fell back to raw decrypt_text, so it sat unused. password_encrypted never had a safe wrapper at all. Adds try_decrypt_text(text), mirroring safe_decrypt_recovery_codes's BEGIN/EXCEPTION WHEN OTHERS THEN RETURN NULL shape for a scalar result, locked down the same way safe_decrypt_twofa_secret is (no client role gets EXECUTE). Routes the password read in all three RPCs through it and the recovery-codes read back through the existing safe wrapper. One corrupt row now surfaces as a blanked field - password NULL, recovery_codes [] - while every other secret in the page still loads. Deliberately not named safe_decrypt_*: ops/rotate_master_encryption_key.sql's re-encrypt loop treats every function matching that name pattern as a mapped bespoke-format adapter and refuses to run if one is unmapped. try_decrypt_text wraps the same plain envelope that script already re-encrypts directly, so it must stay outside that sweep. Scoped to the three user-facing listing RPCs only; the operator-run backfill/rotation paths keep using raw decrypt_text and should still fail closed. * Fix installed-client compatibility and preserve systemic decryption failures Includes desktop regression coverage from #621 by Rushikeshiitb and review repairs by kshivang; related independent #652 work by akashjadon04 informed the review. --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>
kshivang
added a commit
that referenced
this pull request
Sep 15, 2026
…668) * fix(secrets): make the secret listing RPCs fail soft on one bad row BossConsole#620: get_user_secrets, search_user_secrets and get_user_secrets_with_shared each decrypt password_encrypted and recovery_codes_encrypted with a bare call to decrypt_text inside a set-returning query. decrypt_text raises on an undecryptable value - a wrong key after a partial key rotation, storage corruption, or an HMAC/padding mismatch - and one raised row aborts the whole query, so the caller sees every one of their secrets fail to load rather than just the bad one. twofa_secret already reads through safe_decrypt_twofa_secret (NULL on failure). recovery_codes_encrypted already had a safe wrapper too - safe_decrypt_recovery_codes, whose own doc comment says "used internally by get_user_secrets functions" - but whichever later migration touched these RPCs stopped calling it and fell back to raw decrypt_text, so it sat unused. password_encrypted never had a safe wrapper at all. Adds try_decrypt_text(text), mirroring safe_decrypt_recovery_codes's BEGIN/EXCEPTION WHEN OTHERS THEN RETURN NULL shape for a scalar result, locked down the same way safe_decrypt_twofa_secret is (no client role gets EXECUTE). Routes the password read in all three RPCs through it and the recovery-codes read back through the existing safe wrapper. One corrupt row now surfaces as a blanked field - password NULL, recovery_codes [] - while every other secret in the page still loads. Deliberately not named safe_decrypt_*: ops/rotate_master_encryption_key.sql's re-encrypt loop treats every function matching that name pattern as a mapped bespoke-format adapter and refuses to run if one is unmapped. try_decrypt_text wraps the same plain envelope that script already re-encrypts directly, so it must stay outside that sweep. Scoped to the three user-facing listing RPCs only; the operator-run backfill/rotation paths keep using raw decrypt_text and should still fail closed. * Fix installed-client compatibility and preserve systemic decryption failures Includes desktop regression coverage from #621 by Rushikeshiitb and review repairs by kshivang; related independent #652 work by akashjadon04 informed the review. --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>
Rushikeshiitb
marked this pull request as ready for review
September 15, 2026 03:40
Contributor
Author
|
Current head ( |
…esilient-decrypt # Conflicts: # composeApp/src/desktopTest/kotlin/ai/rever/boss/services/supabase/SecretDecodingTest.kt
Contributor
Author
|
Resolved the merge conflict with |
Contributor
|
@Rushikeshiitb Thank you for the resilient listing implementation and follow-up fixes. I verified current head The surviving #668 description already explicitly credits your #621 work, @akashjadon04's #652 implementation and @kshivang's separate review repairs, alongside @Antriksh1984's contribution. Current dev retains the decoder regression and SQL tests for corrupted/valid values, systemic failure propagation and helper permission denial. Your additional SQL suites and strict-rotation explanatory prose remain available in this PR's history; I am not claiming every test file was copied into #668. Closing as already covered by #668, with contributor provenance preserved, rather than merging a second migration. All ten checks on this head are green; this closure is not a CI or contributor-quality failure. The completed Claude review and subsequent repairs were inspected. No application or live database action was performed. |
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
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #620.
One corrupt password or recovery-code field previously aborted an entire authorized listing. The three RPCs now preserve rows and sharing metadata, return a non-null blank password for a corrupt field, and retain recovery codes as JSON arrays. This remains a host database/client contract fix; no plugin UI or live migration is included.
Original implementation: @Rushikeshiitb, including the resilient helper, three listing RPCs and initial SQL suite. @akashjadon04 independently supplied the equivalent implementation in #652 and additional SQL coverage now preserved verbatim here as
secret_listing_decryption_resilience_test.sql(including valid values and recovery codes in search/shared listings and direct helper permission denial). Their desktop mixed-row decoding case is already covered by the equivalent test here. Both contributors retain explicit credit; #652 is the duplicate consolidation source, not a distinct encryption-format change.Review repairs by @kshivang: installed-client non-null compatibility, internal-helper ACL/audit coverage, SQLSTATE-only diagnostics, sharing/array/desktop regressions; then current-dev integration retaining all #647/#651 migration repairs, and a narrowed corrupt-row error boundary. Missing/malformed/empty key configuration fails outside the handler; privilege, missing-function and internal errors propagate. Only invalid parameter/base64, invalid UTF-8 and pgcrypto decryption errors receive field fallback. Six additional synthetic fault assertions cover systemic propagation. A valid-but-wrong key remains indistinguishable from damaged ciphertext under the existing unauthenticated legacy/v2 format; this PR does not claim authenticated encryption or reliable key identity verification. Key rotation retains its strict decoder.
The migration is uniquely versioned
20260915000000, after merged #647's IV migration. #651 is confirmed merged; its atomic-backfill/trigger and migration replay repairs are retained from current dev. #619 is separate historical encryption-format work, not a dependency or consolidation target.Validation on repair head
fd3d90390467b3260a91dff9c4bf0f46504f811e:No app was launched and no visual pass is claimed. Manual synthetic UI checks remain: list/search pagination, blank corrupt password, intact direct-share read-only attribution, and disabled blank-password Save. No live database changes, secret reads, rotation or deployment. Merge only to dev after current-head validation and review pass.
Final review disposition: DRAFT pending current-head platform CI/summary and the single requested Claude review. Database, quality and compatibility checks pass. Local Gradle timed out waiting for the shared build lock without starting. Detailed handoff: #621 (comment) . #652 closed after contributor credit and verified test consolidation. No merge performed.