Repository navigation
feat(mcp): let operators set tool policy proactively - #636
Merged
kshivang merged 9 commits intoSep 15, 2026
Merged
Conversation
Contributor
|
@claude Please review the current head against dev for correctness, security, regressions and tests. For #542, verify the provider-reset/optional-guard repairs and author quality fixes against the prior review; avoid repeating resolved findings. For history UI, check ledger redaction, bounded file reads, statistics, overlay placement and policy changes. Related #624 and #636 overlap in history UI but proactive policies and reliability statistics are distinct. Review only; do not push changes. |
18 of 29 tasks
|
Claude finished @kshivang's task in 6m 10s —— View job Review: #636 — MCP activity log + proactive tool policyReviewed
What checks out
Findings1. Proactive The approval dialog's "Always Allow" shows the risk level and reason ( Worse, this same file already demands a confirmation tap to reset a DENY, on the stated reasoning that "resetting a DENY raises it, removing the one rule that beats session trust" ( 2. The row shows
3. "N errors" in alert red conflates governance denials with reliability failures.
The ledger carries 4. The feature's safety invariant lives in a composable and has no test.
The PR's "no other MCP dialog has a unit test" argument is about composables and I accept it; this is a pure list transform. Extract it (e.g. 5. Kill-switch-disabled tools are offered a policy that does nothing.
6.
Nits
Scope notes#542 — not in this head. #624 overlap — agreed the proactive-policy half is distinct, but both PRs add a reader over Tests — none added, and #4 is the one place I'd insist. I could not run Gradle in this environment (no dependency network), so I'm relying on the author's stated Net: the direction is right and the engine-side reuse is genuinely correct — the queued-approval and session-trust interactions I expected to be broken are not. The blocking concerns are all about a privilege-granting button presented more casually than the identical grant one dialog over (#1, #2) and a summary statistic that reads governance successes as failures (#3). |
Contributor
|
@Antriksh1984 The proactive policy entry point is useful: operators can configure a tool before it triggers an approval, including when no saved rules exist. The detailed policy/disposition/provider activity rows are also useful. I reviewed the three-file change and current-head hosted checks, which all pass. This is not a CI-failure handoff. I will mark this PR draft because the remaining work is substantive:
Current head Current Claude review has completed. It agrees the engine's final DENY recheck protects the reactive queued approval; that does not fix the distinct stale proactive unconditional write described above. Extracting/testing the candidate-list filter alone, as suggested, is insufficient across the IO suspension: enforce absence/reset guards under the write lock. Claude also correctly calls for an explicit durable/name-wide scope warning and deliberate confirmation for proactive Allow, matching the existing remove-denial control; provider identity in a row must not imply provider-scoped storage. Explain disabled tools rather than implying an ALLOW re-enables them. Relabel lifetime “errors” as denied/failed/cancelled outcomes or provide a visible-window disposition breakdown, since the ledger counter includes operator denials. Use theme signalText for textual success markers (warn for the durable grant), Locale.ROOT for enum labels, and update normative AGENTS.md copy. These valid findings remain open in this draft handoff. I do not accept “redaction is correct by construction” as a blanket guarantee: the snippet path is bounded and sanitized, but arbitrary secret formats are outside the sanitizer's guarantee. The viewer does not render arguments. Likewise #624/#636 should coordinate one eventual history surface, but distinct policy/statistics contributions must not be discarded; no full duplicate consolidation is authorized by code coverage here. |
kshivang
marked this pull request as draft
September 14, 2026 04:11
Antriksh1984
added a commit
to Antriksh1984/BossConsole
that referenced
this pull request
Sep 14, 2026
…nventions Addresses the review on risa-labs-inc#636: - Proactive Allow now writes through setToolPolicy(preserveDeny = true), so a DENY landing between the dialog rendering "no rule yet" and the write crossing to Dispatchers.IO cannot be silently overwritten - the engine's own lock closes the race, not the UI filter that produced the candidate list. - Proactive Allow requires a second confirming tap, shows the tool's risk level/reason and the same tool-name/all-agents/all-restarts scope sentence McpApprovalDialog shows before its own "Always Allow" - it is the same durable, standing-privilege grant and was presented far more casually than the escalation it actually is. - The candidate list (excluding tools with a rule, or withheld by the kill switch) is extracted to mcpProactivePolicyCandidates(), a pure function with its own tests - previously a filter inside a composable, reachable by nothing but eyeballing the UI. - The saved-rules and proactive sections now share one bounded, scrollable region instead of two independent ones stacked under fixed header/footer content, so Close can no longer be pushed off a short window. - The activity log's lifetime "N errors" is relabeled "N unsuccessful" (it counts an operator's own Deny and a cancelled approval, not just tool faults) and a denied/cancelled/failed breakdown over the visible window replaces the single alarming count. - colors.signal -> colors.signalText/colors.warn on text per BossDesignSystem's own rule (signal is for rects; the durable-grant labels use warn, matching McpApprovalDialog's Always Allow/Always Deny buttons). - Locale.ROOT for the enum-token lowercase, provider id/tool name overflow handling, and the AGENTS.md section documenting this surface.
Contributor
Author
|
Addressed the review in `c7f56031`, then merged `origin/dev` (`#542` had landed, conflicting with this branch's own additions in `BossBottomBar.kt` - reconciled by hand, not auto-resolved, since the two sides restructured the same function differently) in `dfa3ca4f`. Point by point:
Not done, deliberately: extracting the activity/policy UI into a dynamic plugin per #416/#432. That's a real architecture decision (host `AGENTS.md` conventions, plugin API surface for the ledger/engine, migration of the two dialogs) I don't think a bounded fix-up on this PR should make unilaterally - flagged in the new AGENTS.md paragraph as a candidate for that move rather than committed to it here. Happy to open that as its own follow-up if that's the right next step. Current head `dfa3ca4f` - compiled, detekt clean, and the full `ai.rever.boss.mcp.*` + new test suite green locally; hosted CI running now. |
Antriksh1984
marked this pull request as ready for review
September 14, 2026 06:05
Contributor
|
@claude Please review current head dfa3ca4 after the contributor repairs in c7f5603. In particular, assess whether preserveDeny alone protects a proactive Allow across a newer reset/revocation, and check the revised dialog sizing and #416 placement. Prior findings fixed by these commits should be credited as resolved. |
|
Claude finished @kshivang's task in 5m 3s —— View job Review: #636 at
|
| Prior finding | Status |
|---|---|
| #1 Proactive Allow granted standing privilege casually | Fixed. confirmingAllow second tap (McpPolicyManagerDialog.kt:255, :321-329), DefaultMcpRiskEvaluator level/reason and the approval dialog's own scope sentence (:340-359), rendered colors.warn. Deny correctly still fires on the first tap — it only narrows. |
| #2 Provider id implied provider-scoped storage | Fixed. :206-208 now says "A tool name, not a specific plugin: another plugin that later registers the same name inherits this rule too." |
| #3 "N errors" conflated denials with faults | Fixed, both halves. Lifetime counter relabelled "N unsuccessful" (McpActivityLogDialog.kt:101) and a real per-disposition breakdown over the visible window (:275-284). DENIED_DISPOSITIONS/CANCELLED_DISPOSITIONS match the enum. |
| #4 Safety invariant in a composable, untested | Fixed. mcpProactivePolicyCandidates (BossBottomBar.kt:437-449) is pure, and McpProactivePolicyCandidatesTest pins all five cases. |
| #5 Kill-switch-disabled tools offered a dead rule | Fixed. disabledToolNames filter, with its own test. |
#6 colors.signal as a text color |
Fixed. signalText on the ✓, warn on the durable-grant labels. |
Nits: Locale.ROOT, dead distinctBy, sorting out of the composable, "last 100" copy, provider/tool overflow, AGENTS.md |
All fixed. |
The merge dfa3ca4f reconciles #542 cleanly: McpProviderTrustControls and the two new status items share one policyEngine.config collection (BossBottomBar.kt:251-257) rather than each subscribing.
The question asked: does preserveDeny alone protect a proactive Allow across a newer reset/revocation?
No. preserveDeny covers exactly one thing — a DENY that is currently in effect at tool scope — and a reset is specifically the case where no DENY remains. The engine's own test says so in its name:
McpPolicyRevocationTest.kt:170—stale persistent decisions cannot write after reset **even when no DENY remains**
That test's protection comes entirely from expectedRevocation, and revokePersistedPolicy deliberately publishes the version bump after the write (McpPolicyEngine.kt:379-381) for that reason. The proactive call site passes neither expectedRevocation nor providerId:
// BossBottomBar.kt:410
McpToolRegistryImpl.policyEngine.setToolPolicy(toolName, action, preserveDeny = true)Compare the reactive path, which after #542 passes all four — captured generation, preserveDeny, expectedRevocation, providerId — at McpToolRegistryImpl.kt:838-845, :884-891, :926-933.
Two concrete consequences:
(a) A provider-wide DENY is invisible to the guard. preserveDeny evaluates policyFor(toolName, null), and policyFor's provider-DENY branch (McpPolicyEngine.kt:149-152) only runs when providerId is supplied. If another window writes providerRules[p] = DENY while the write is crossing Dispatchers.IO, the guard sees the default and persists rules[tool] = ALLOW. Resolution is still DENY today (provider DENY at step 2 outranks a tool ALLOW at step 4), so no immediate privilege leak — but a latent standing ALLOW now sits on disk and activates the moment that provider DENY is lifted, with nothing recording that the operator ever meant it. McpToolIdentity already carries providerId and the dialog even renders it; only the onSetPolicy signature drops it.
(b) A revoke landing in the window does not refuse the write. revokeProviderPolicy bumps providerRevocations[p] and removes the rule; revokePersistedPolicy does the same for a tool. Either leaves the effective policy at the default (ASK), so preserveDeny passes and the stale ALLOW lands. The reachable sequence is the provider one: tool x from provider p, p provider-trusted, x has no tool rule so it is a proactive candidate (the filter checks rules, not providerRules) → operator arms and confirms Allow → operator opens "Trusted plugins" and revokes p → the stale write persists a tool-scoped ALLOW for one of p's tools, which is durable in a way the provider grant they just withdrew was not. That is the guarantee AGENTS.md states as "each reset invalidates older authorizations before their final approval boundary".
In fairness: a proactive rule is the operator authoring policy de novo rather than answering a prompt about a specific call, so one can argue the two intents are independent. I don't think that survives contact with case (a), and the guard costs one captured Long and one parameter on a callback that already has the provider id in hand.
Related, still open from the maintainer's point 2: the row's failure text is "This tool already has a rule, or the write failed. See the host log." (McpPolicyManagerDialog.kt:314) — still one message for both. setToolPolicy returns false for a stale refusal and for a disk fault, and the reactive path already disambiguates by re-reading revocationVersion/policyFor after the false (McpToolRegistryImpl.kt:849-856, :897-903). Only a disk fault raises McpPolicyFault.PolicyPersistFailed, so the signal is already there.
AGENTS.md overstates this. The new paragraph says the preserveDeny write means "a DENY that lands between the dialog rendering 'no rule yet' and the write landing is not overwritten." True for a tool-scoped DENY only, and silent on reset. Since this repo treats that file as normative, it should either gain the guards or lose the sentence.
Dialog sizing
The defect that was reported is genuinely fixed. Two independently-scrolling regions (320dp + 200dp) under fixed header/footer are now one shared heightIn(max = 440.dp).verticalScroll (McpPolicyManagerDialog.kt:119-124), with the Close row outside it. Total scroller height dropped 520dp → 440dp and, with few saved rules, the proactive section gets the whole budget instead of a fixed 200dp.
Two residuals, neither blocking:
- Nothing still ties the card to the window height. 440dp of scroller plus ~185dp of header prose, section spacers and the Close row means the card wants ~625dp before it can render fully, and
BossDialog's own KDoc says "the card is intrinsically sized" on the heavyweight path withusePlatformDefaultWidthand friends ignored. On a short window the footer can still be clipped. This is the house pattern rather than something this PR introduced (CommitDialogandShortcutHelpDialogboth cap at 700dp), so I'd only note it — but afillMaxHeight(0.8f)-style bound on the outerColumnwould make the claim in the code comment unconditional instead of approximately true. - Sharing the region costs the proactive section its discoverability. With a long saved-rules list the "Set a rule proactively" header is now inside the scroll, below everything — with ~30 rules an operator scrolls past ~1200dp to reach it. Putting the proactive section first, or giving that header a sticky position, gets the fix without the cost.
dialogScrollFence (DialogScrollFence.kt) exists for the intrinsics leak that bites AlertDialog; this is a Surface inside BossDialog, so it does not apply — noting it only so the next reader doesn't re-derive that.
Still not present, and the maintainer asked for them explicitly: rendered tests with both sections populated, long tool/provider names, a short viewport, the failure-feedback state and the zero-saved-rules entry point. McpProactivePolicyCandidatesTest covers the list transform, which was my #4 and is a real improvement, but it cannot see layout.
#416 placement
The AGENTS.md paragraph records this as "Both are host UI for now; #416 is where history/activity UI and its ownership move into a dynamic plugin, and this pair is a candidate for that move rather than a permanent host surface."
That is an accurate flag, and I think declining to make the extraction unilaterally on a bounded fix-up PR was the right call. But it is a note, not the "explicit architecture decision for this narrower host view" the maintainer offered as the alternative — it says candidate, which leaves the question exactly where it was. That's a maintainer decision, not something the diff can settle, and it's the one item from the draft handoff that this head does not move.
Worth noting on the way: the proactive-policy half has a clean architectural argument for staying in the host regardless — it writes host-owned policy through the host's own engine lock, and a plugin-side writer would need the engine surface exposed across the plugin API. The activity viewer is the half with a natural home in #416.
Smaller items on this head
McpActivityLogDialog.kt:278—POLICY_PERSIST_FAILEDandPROVIDER_TRUST_PERSIST_FAILEDfall outsideGOVERNANCE_DISPOSITIONSand so count as "failed", i.e. a tool fault. They are host disk faults. Low stakes, but the breakdown exists precisely to stop that kind of attribution.McpActivityLogDialog.kt:245— the KDoc example lost its middle:"Cancelled ıight"should read"Cancelled ın flight". The fix itself is right.McpPolicyManagerDialog.kt:249—failedSetis a single slot across all rows (a success on tool B clears tool A's message), same accepted trade-offfailedRevokedocuments at:76-79, but without the comment that makes it read as deliberate.- Redaction is unchanged and still correct for what this surface renders: only
errorSnippet, alreadysanitizeMessage(...).take(4096)at record time, andsanitizedArgsis deliberately never rendered. I'll restate the maintainer's qualification rather than my earlier phrasing — the sanitizer is bounded and best-effort, so this is "the viewer shows nothing the ledger has not already scrubbed", not a guarantee about arbitrary secret formats. A KDoc line saying the args omission is deliberate would still stop the next contributor adding that row. - I could not run Gradle here (no dependency network), so compile/detekt/test status is the author's and hosted CI's, not mine.
Net: all six prior findings and every nit are genuinely closed, and the risk/scope/confirmation work on proactive Allow is a real improvement rather than a cosmetic one. The remaining blocker is narrow and specific: preserveDeny is the weakest of the four guards the reactive path uses, and the two it omits — expectedRevocation and providerId — are the ones that cover reset and provider-scope, which is exactly what was asked about. Sizing is improved and no longer has the reported defect; #416 remains an open maintainer decision the PR flags honestly.
· branch feat/mcp-governance-visibility
Contributor
|
@Antriksh1984 I reassessed I am returning this PR to draft for two remaining substantive items:
Visual follow-up remains necessary: one shared 440dp scroll region is a useful improvement, but a fixed-height cap is not evidence that header + content + footer fit a short viewport. Test both dialogs in short/narrow viewports and light/dark themes, with long identifiers and failures, and keep Close reachable. The activity metadata provider Text is still unbounded before policy/disposition, and the disk-history wording still implies older calls remain available despite finite rotation and best-effort writes. Scope the viewer to registry-governed records and describe retention accurately. No live app was launched, stopped or visually exercised during this review. Validation: all ten hosted checks pass on #624 overlaps history access but adds disk loading and per-tool percentiles that #636 does not cover; #636 adds proactive policy controls it does not cover. #434/#342/#432 and policy/context work #497/#498/#597 remain read-only cross-group context. No complete duplicate was established and no other PR was modified, closed or absorbed. Your branch, commits and authorship remain unchanged. Claude status: the earlier review was assessed point by point against your repairs. I requested one current-head review at #636 (comment); run 34821029850 is still in progress at handoff. No repeat request or extended polling. Pending independent review is not an author defect; the demonstrated reset race and unresolved architecture scope are the reasons for draft. The earlier blanket suggestion that sanitization guarantees secret-free output remains rejected: this sanitizer is bounded and best effort. |
kshivang
marked this pull request as draft
September 14, 2026 08:10
Antriksh1984
force-pushed
the
feat/mcp-governance-visibility
branch
from
September 14, 2026 09:19
dfa3ca4 to
58030cf
Compare
Antriksh1984
marked this pull request as ready for review
September 14, 2026 09:19
Contributor
Author
|
Rebuilt this PR's branch cleanly onto current dev (the prior head had accumulated unrelated drift from dev advancing under it - GitServiceBridge/LogServiceBridge, the now-merged encryption migration, etc. - none of which belonged in this diff) and closed the remaining substantive finding from the last review: The proactive-Allow reset/provider race (point 1). Updated the AGENTS.md paragraph, which had described Point 2 (#416 placement) is unchanged from the last response: still a maintainer architecture call, not something this PR should decide unilaterally. The AGENTS.md paragraph still flags the pair as a candidate for that move. Current head |
Contributor
|
@claude Please review current head 58030cf, crediting the newly threaded expectedRevocation/providerId guards. Check candidate capture and confirmation freshness, add-only-if-absent behavior for intervening ASK/ALLOW, and distinguish remaining architecture/UI issues from previously repaired findings. One review request for this head. |
|
Claude finished @kshivang's task in 4m 48s —— View job Review: #636 at
|
| Interleaving after the candidate is offered | Outcome |
|---|---|
| tool-scoped DENY written (reactive "Always Deny") | refused — preserveDeny → policyFor = DENY |
| provider-wide DENY written | refused — providerId is now supplied, so policyFor's provider branch (McpPolicyEngine.kt:149-152) runs. This was gap (a) last review |
revokePersistedPolicy (reset to ASK) |
refused — revocations[tool] bumped, stamp mismatch at :242. This was gap (b) |
revokeProviderPolicy |
refused — providerRevocations[p] bumped, and revocationVersion sums both scopes (:90), so the token moves |
The engine side is already covered by tests that predate this PR and assert exactly these shapes — McpProviderRevocationTest.kt:26-33 (provider reset refuses setToolPolicy(expectedRevocation, providerId)) and McpPolicyRevocationTest.kt:170-188 ("stale persistent decisions cannot write after reset even when no DENY remains"). So the fix is threading, not new engine behaviour, which is the right shape. The new sixth case in McpProactivePolicyCandidatesTest pins that each candidate carries its own tool-and-provider generation, which is the part that could silently regress into a single shared token.
revocationVersion being a sum is safe here: both counters are monotonically increasing, so no combination of later bumps can reproduce an earlier total.
Prior findings #1-#6 and every nit from the c7f56031 round remain fixed on this head; I re-checked rather than assumed, and am not restating them.
Capture freshness — one reachable defect, and it is the refusal path, not the capture
The capture itself is placed well. remember(allTools, persistedPolicyConfig.rules, disabledToolNames) deliberately does not key on the revocation counters, so a reset cannot silently refresh the token out from under an armed confirmation. I checked the two ways that could have leaked:
revokePersistedPolicy(x)for an unruledx(which a candidate always is) produces anequals-equalMcpToolPolicyConfig, so theStateFlowconflates it and no recompute happens — the stamp stays stale, which is the correct direction.revokeProviderPolicy(p)changesproviderRules, notrules, so therememberkey is unchanged — again no refresh.
That correctness has a consequence nobody has traced yet:
A refused proactive write leaves the row permanently unusable for the life of the dialog. ProactivePolicySectionContent (McpPolicyManagerDialog.kt:288-290) records only failedSet = tool.toolName; nothing re-reads revocationVersion and nothing re-stamps. So when the bump came from a source that does not change rules — revokeProviderPolicy from the "Trusted plugins" control in the same bottom bar, or another window's reset of an unruled tool — every subsequent attempt on that tool, and on every other tool from that provider, fails identically until the operator closes and reopens the dialog. The only feedback is "This tool already has a rule, or the write failed. See the host log.", which names neither the real cause nor the remedy.
The sharper half: this also silently blocks a narrowing action. An operator who opens the dialog, has a provider revoke land, and then clicks Deny to lock a tool down is told the write failed, with no way to make it succeed from that dialog. Failing closed on ALLOW is right; failing closed on DENY with no retry affordance means the governance UI refuses the safe action.
The reactive path already has the disambiguation this needs — it re-reads revocationVersion/policyFor after a false and distinguishes a stale refusal (POLICY_DENIED) from a disk fault (POLICY_PERSIST_FAILED) at McpToolRegistryImpl.kt:849-856 and :923-932. The proactive site collapses both into one string. Re-stamping the candidate on a stale refusal and saying "the policy for this tool changed while the dialog was open — press again to apply to the current state" would close both halves.
Add-only-if-absent for an intervening ASK / ALLOW
Still not an engine contract — setToolPolicy remains an unconditional rules + (toolName to action) (McpPolicyEngine.kt:249). What protects the advertised "for a tool nothing has asked about yet" is a composition of two other guards, not the absence check itself. That composition holds today, and I enumerated why rather than taking it on trust: the only production writers of a tool rule are McpToolRegistryImpl.kt:839 / :928 and this new call site, so an intervening rule can only be ALLOW or DENY. DENY is caught by preserveDeny; an intervening ALLOW overwritten by a proactive ALLOW is a no-op, and by a proactive DENY is a narrowing the operator just asked for. Nothing escalates.
Two things follow that are worth writing down rather than leaving to be rediscovered:
- It is load-bearing that no production path writes ASK.
setToolPolicy(_, ASK)exists in tests only, andrevokePersistedPolicyis deliberately a key removal for the reasons its own KDoc gives. The day something writes a durable ASK without bumping a revocation counter, a stale proactive ALLOW overwrites it silently and neither guard notices. AnexpectAbsentflag under the same lock would make the contract the UI states true in the engine, instead of true by coincidence of who calls it. - Do not "fix" the proactive DENY refusal by dropping
preserveDeny. It looks redundant for a narrowing action — and the reactive denial path at:928indeed omits it — but here it is doing a second job. UnderMcpPolicyFault.PersistedPolicyUnreadable,policyForreturns DENY for everything (:144), andapplyConfigclears_faulton a successful write (:224). WithpreserveDenyoff, a proactive DENY during a damaged-policy fault would persist the fail-closed defaults over the damaged file and clear the fault — i.e. this dialog would become the first reachable way to discard a policy file AGENTS.md says to back up before manual recovery. The reactive path cannot reach that state (during a faultpolicyForis DENY, so no approval prompt ever opens).
Remaining UI items — all pre-existing on this head, none newly introduced
Separating these from the repaired set, since several were named in the draft handoff and are still open:
op.providerIdis still unbounded (McpActivityLogDialog.kt:221-225) — nomaxLines/overflow, unlike the tool name and the snippet. A long provider id pushes the policy and disposition chips off the row, which are the two fields the governance view exists to show. This one was explicitly called out and is a one-line change.POLICY_PERSIST_FAILED/PROVIDER_TRUST_PERSIST_FAILEDcount as "failed" (:278) — they fall outsideGOVERNANCE_DISPOSITIONS, so a host disk fault is attributed to the tool. The breakdown exists precisely to stop that class of misattribution.- Retention copy still overstates (
:82-85): "Older calls roll off this list, but are still in the rotated~/.boss/mcp-calls.jsonlon disk." Rotation keeps the active file plus five backups and drops the rest, andrecordswallows I/O failures — so older calls are sometimes there. "may still be in" would be exact. - Scope copy: "The most recent tool calls this session" is narrower than before but still wider than the data.
invokereturns atMcpToolRegistryImpl.kt:744for an unknown, disabled or permission-denied tool, before the ledger'stry. The ledger's own KDoc says so; the dialog is the surface where a reader would infer otherwise. - KDoc example lost its middle (
:245):"Cancelled ıight"should read"Cancelled ın flight". The code it documents is right. - Sizing: one shared 440dp region with Close outside it is the real fix and the reported defect is gone. There is still no window-relative bound — ~440dp of scroller plus ~185dp of header prose and footer wants ~625dp, and
BossDialogintrinsically sizes the card on the heavyweight path. House pattern, not this PR's invention. - Discoverability cost of sharing the region: with a long saved-rules list, "Set a rule proactively" is now below everything inside the scroll. Ordering the proactive section first would keep the sizing fix without it.
- No rendered/interaction tests for either dialog (populated both-sections, long identifiers, short viewport, failure state, zero-saved-rules entry).
McpProactivePolicyCandidatesTestcovers the list transform well and cannot see layout; nothing pins thatonSetPolicyactually forwardsexpectedRevocation/providerId, which is the wiring this round is about. - Redaction is unchanged: only
errorSnippetis rendered, alreadysanitizeMessage(...).take(4096)at record time, andsanitizedArgsis deliberately never rendered. Stated as "the viewer shows nothing the ledger has not already scrubbed" — not a guarantee over arbitrary secret formats, per the maintainer's standing qualification. A KDoc line marking the args omission as deliberate would keep it that way.
AGENTS.md now describes the write accurately (preserveDeny and the captured generation, both scopes) — the overstatement from the last round is gone. It does not mention that the captured generation refreshes whenever rules/allTools/disabledToolNames change, which is the subtlety behind the refusal defect above; worth a clause if that paragraph is touched again.
Architecture (#416)
Unchanged from the previous head, and correctly labelled as such by the author: the AGENTS.md paragraph records the pair as host UI for now and a candidate for the #416 move. That remains a maintainer decision the diff cannot settle. The asymmetry I noted before still holds and may help make it: the proactive-policy half has an independent argument for staying in the host (it writes host-owned policy under the host engine's own lock, and a plugin-side writer would need that surface exposed across the plugin API), while the activity viewer is the half with a natural home in #416.
Verification
I could not run Gradle in this environment (no dependency network), so compile/detekt/test status is the author's and hosted CI's, not mine. Everything above is from reading the code on this head.
Net: the specific gap from the last review is closed correctly and at the right layer — expectedRevocation + providerId threaded through to the engine's own lock, matching the reactive path and covered by engine tests that already existed. What the threading exposed is the other side of the same contract: a refused write has no recovery and no disambiguation, which can strand a proactive Deny. That is the one thing I would fix before merge; the rest are the UI residuals listed above and the open #416 call.
· branch feat/mcp-governance-visibility
Contributor
|
@Antriksh1984 I reassessed your rebuilt Two remaining items prevent merging this head into dev:
Visual/source follow-up: bound the outer dialogs to the viewport and verify Close with short windows, long provider/tool identifiers, failures and both themes. The activity metadata provider Text remains unbounded before policy/disposition, and “older calls ... are still in ... on disk” overpromises finite, best-effort retention. Scope the description to registry-governed records. No changed app was run or visually exercised in this review. All ten current-head hosted checks pass in run 34827315958, including three-platform builds/tests and code-quality; I checked the latest completed Claude review against your repairs and requested one review of this head at #636 (comment); no repeat requests or prolonged polling. Pending review is not the reason for draft. Its earlier reset/provider finding is credited as repaired; its broad provider-overflow conclusion does not match the remaining unbounded activity metadata Text, and sanitizer guarantees remain bounded/best effort. #651 is now confirmed merged; current main/dev were fetched and inspected, and the relevant policy engine is equivalent, though the branches differ elsewhere. #624's disk reader/statistics and #636's proactive policies remain distinct; #434/#342/#432/#497/#498/#597 are read-only context. No duplicate closure or contributor-credit transfer is justified. Returning this PR to draft with your production code, head and authorship unchanged. Please request review after the atomic-write and architecture work, with interleaving/reload and rendered checks plus required CI on the resulting head. |
kshivang
marked this pull request as draft
September 14, 2026 09:56
…nc#606) (risa-labs-inc#648) Global Search matched an open tab only against its title, and TabResult hardcoded url = null / filePath = null on every hit. A browser tab whose title never mentions its domain, or an editor tab someone wants to find by path rather than filename, could not be found at all. searchTabs now also scores a browser tab's FluckTabInfo.currentUrl and an editor tab's EditorTabInfo.filePath, through the same substring- containment gate (proseScore) searchRecentPages already uses for a page's URL, since a fuzzy subsequence match over a whole URL or path is close to meaningless. TabResult carries the real url/filePath instead of null, and the tab row's subtitle now leads with the workspace name (disambiguating identical URLs open in different workspaces, or across windows) followed by the url/filePath/tabType, in place of the old "tabType • workspaceName" line. 11 tests cover: URL-only and file-path-only matches, both fields populated for the right tab type and left null for the other/a blank path, a short query not matching a URL by fuzzy accident, title+URL both matching taking the higher (title) score with title highlight ranges preserved, live navigation changing which URL matches (and the old URL no longer matching), directory-path matching without a subsequence false-positive, and no-open-tabs contributing nothing rather than failing.
* 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…
…dge (risa-labs-inc#646) Every ActiveTabsService RPC was reachable by any process able to open a connection to the kernel IPC server, not only the plugins the host itself loaded. watchActiveTabs disclosed every open tab's title and URL across every window with no credential at all, and createBrowserTab could force the browser to navigate to an arbitrary URL; selectTab/closeTab could manipulate what the user is looking at. All seven RPCs now require a verified caller identity, matching the pattern PR risa-labs-inc#505 established for the Secret Service: unary RPCs call authenticatedCallerOrRefuse, and the watchActiveTabs stream captures ProcessIdentityInterceptor.CURRENT_IDENTITY synchronously in the method body (before the flow is collected, on the calling thread the interceptor's context is current on) and re-checks it before every emission, so a revoked caller's stream is refused before an already-held handle could receive a value it should no longer see. Refusals are logged with a bind/revocation stage tag, and the three mutating RPCs (selectTab, createBrowserTab, closeTab) now record an attributed audit line - caller and RPC name only, no URLs or titles. Tested with a real gRPC server plus ProcessIdentityInterceptor: every RPC is refused with no credential and never reaches the provider, an authenticated caller can watch/read/select/create/close tabs, and a revoked caller's live watch stream terminates with PERMISSION_DENIED before receiving a value that arrived after revocation.
Bumps `essenty` from 2.5.0 to 2.6.0. Updates `com.arkivanov.essenty:lifecycle` from 2.5.0 to 2.6.0 - [Release notes](https://github.com/arkivanov/Essenty/releases) - [Commits](arkivanov/Essenty@2.5.0...2.6.0) Updates `com.arkivanov.essenty:state-keeper` from 2.5.0 to 2.6.0 - [Release notes](https://github.com/arkivanov/Essenty/releases) - [Commits](arkivanov/Essenty@2.5.0...2.6.0) --- updated-dependencies: - dependency-name: com.arkivanov.essenty:lifecycle dependency-version: 2.6.0 dependency-type: direct:production update-type: version-update:semver-minor - dependency-name: com.arkivanov.essenty:state-keeper dependency-version: 2.6.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>
Bumps [org.graalvm.buildtools.native](https://github.com/graalvm/native-build-tools) from 1.1.11 to 1.1.12. - [Release notes](https://github.com/graalvm/native-build-tools/releases) - [Commits](graalvm/native-build-tools@1.1.11...1.1.12) --- updated-dependencies: - dependency-name: org.graalvm.buildtools.native dependency-version: 1.1.12 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>
Reactive approval was the only way a persistent tool policy came to exist: a plugin had to invoke the tool and trigger the approval dialog before "Always Allow"/"Always Deny" could be clicked. An operator who already knew they never wanted a tool run had no way to say so ahead of time, short of hand-editing ~/.boss/mcp-tool-policy.json. "Persisted MCP policies" (present even with zero saved rules, then labeled "Set MCP tool policies") now also offers a rule for any registered, not-yet-ruled, not-disabled tool - Allow requires a second confirming tap and shows the tool's risk assessment first, same as the approval dialog's own Always Allow. The write goes through a new McpPolicyEngine.setToolPolicyIfAbsent, not the reactive path's setToolPolicy: the proactive contract is "add only while this tool still has none of its own," and the existing expectedRevocation guard alone can't enforce that - revocationVersion only moves on a revoke, so an intervening explicit ASK or ALLOW made through the reactive approval dialog for the same tool never trips it, and a candidate captured while the tool had no rule could otherwise silently overwrite a decision made in between. setToolPolicyIfAbsent re-checks the rule map under the same lock the write itself takes, so any rule present at write time - not only a DENY - refuses the write, and returns a result distinguishing that refusal from a genuine disk failure rather than a bare boolean. mcpProactivePolicyCandidates filters the offered list to tools with no rule and not kill-switch-disabled, stamping each with its own revocation generation for the write to pass back.
Antriksh1984
force-pushed
the
feat/mcp-governance-visibility
branch
from
September 14, 2026 11:07
58030cf to
7b9e8f4
Compare
Contributor
Author
|
Pushed changes addressing both open items: 1. Atomic add-only-if-absent guard. Added
New test file 2. #416 / history viewer split. Narrowed this PR to the proactive policy controls only. The activity log dialog and its bottom-bar hook are now #662, which references #416 for the eventual host-vs-plugin decision and links back here. Compiles clean, all tests pass (including the new add-only-if-absent suite), ktlint/detekt clean. |
Antriksh1984
marked this pull request as ready for review
September 14, 2026 11:20
Antriksh1984
added a commit
to Antriksh1984/BossConsole
that referenced
this pull request
Sep 14, 2026
…lassification Rebuilt standalone against dev (was stacked on risa-labs-inc#636's branch, which made this PR's real diff 11 files instead of the 4 it claimed). Replaced the setOf-based unsuccessful-call classification with an exhaustive when over McpApprovalDisposition: the enum has grown repeatedly across risa-labs-inc#336/risa-labs-inc#371/risa-labs-inc#636, and a setOf membership check silently counts a new disposition as a tool fault by falling through to the default. POLICY_PERSIST_FAILED gets its own "withheld" bucket instead of counting as a tool fault - the call never ran; a host disk fault stopped it, which is a different failure than the tool itself breaking. Made the classification and the disposition-name formatter internal and added a test suite pinning every disposition's bucket. The status item is now reachable even with no MCP activity yet, so an operator can open the log before the first call rather than only after one. Added hover cursor, a tooltip, and Role.Button semantics to the clickable status line, which previously looked like a bare label next to buttons that visibly were clickable. Resolved the ledger path through BossDirectories instead of hardcoding ~/.boss/mcp-calls.jsonl, which is wrong under BOSS_DATA_DIR. Softened the retention description to finite and best-effort (active file plus up to 5 rotated backups, write failures logged not retried) and scoped the "most recent calls" description to what actually reaches the ledger - an unregistered, unpermitted or kill-switch-disabled tool call never gets there. Bounded the provider-id text in each row so a long identifier can no longer push the policy/disposition chips off it.
kshivang
changed the title
Contributor
|
@claude Please review this PR's current head, including the actual diff against dev. This is the one review request for this sweep. Credit @Antriksh1984's atomic add-only-if-absent implementation and split of #662; prior absence-check findings are fixed. Check the review-agent restored effective-DENY/unreadable-policy guard, typed refusal/storage feedback, refreshed candidates and fresh confirmation identity, and window-bound dialog/interaction tests. The proactive controls remain with host enforcement; the independent viewer placement is tracked separately in #662. Please assess current code rather than repeat the removed history findings. |
|
Claude finished @kshivang's task in 6m 47s —— View job Review: #636 at
|
| After the candidate is offered… | Result |
|---|---|
| reactive ASK written for the same tool | Refused — toolName in rules |
| reactive ALLOW written | Refused — same |
| reactive DENY written | Refused — twice over (rules, and policyFor = DENY) |
revokePersistedPolicy (tool reset to ASK) |
Refused — revocations[tool] bumped, stamp mismatch |
revokeProviderPolicy |
Refused — providerRevocations[p] bumped, and revocationVersion sums both scopes |
| provider-wide DENY written | Refused — providerId is passed, so policyFor's provider branch runs; no latent tool ALLOW left on disk |
policy file damaged (PersistedPolicyUnreadable) |
Refused for ALLOW and DENY, file left byte-identical, fault preserved |
Each of those has a named regression in McpPolicyEngineAddOnlyIfAbsentTest (ten cases), including the reload assertions that were asked for — assertEquals(ASK, McpPolicyEngine(policyFile = file).policyFor(...)) after the stale write — plus a stale revocation is refused without touching the fault channel, which is the right distinction to pin: a guard refusal is not a disk event.
Typed outcome, wired end to end. McpProactivePolicyOutcome.Saved/Refused/Failed(error) (:49-61) replaces the bare Boolean, and the two cases reach the operator as different sentences (McpPolicyManagerDialog.kt:389-400) — refusal says candidates were refreshed and to review current policy, failure says check the host log and storage. Failed is produced only by a real persistConfig error, and a genuine disk failure is distinguishable from a guard refusal pins that with the directory-as-file trick the existing persist-failure test uses.
Refused refreshes candidates, and a refreshed candidate is a different candidate. onRefreshCandidates bumps candidateRefresh, which is in the remember key that recomputes mcpProactivePolicyCandidates (BossBottomBar.kt:366-374), so the stamps are re-read from revocationVersion. That is what makes retry-after-refusal possible — the engine half is pinned by fresh candidate can retry after provider reset without reusing stale approval. Deliberately keeping the counters out of the remember key is also right: a reset cannot silently refresh the token under an already-armed confirmation.
Confirmation identity is the whole candidate, not the tool name. confirmingAllow is remember(availableTools) and compared with confirmingAllow == tool on a data class carrying expectedRevocation, so a re-stamped row drops its armed state. replacing a candidate invalidates the armed confirmation in light theme asserts the important half — writes == 1 after arm → invalidate → re-arm → confirm, i.e. the stale arm never wrote.
The dialog is now bounded by the originating window, and the bound is real on the production path. LocalWindowInfo.current.containerSize inside a heavyweight modal is the overlay window's container, and HeavyweightModal sizes that window to rememberOverlayParentBounds(parent) — the parent window's bounds — so maxHeight = windowHeight - 32.dp (:86) is the main window's height, not a self-referential measure of the card. The ordering also gets the Row-weight lesson from this repo's own SpaceRow right: the scrolling body is weight(1f, fill = false) and the Close row is unweighted and outside it, so Close is measured first and survives a short viewport. short window keeps close visible with saved and proactive rows drives 30 saved rows plus an 640-character provider id at 700×360 and asserts Close's unclipped bounds, which is the assertion that can actually fail.
Candidate filter is unchanged and still pinned (six cases); worth recording that its disabledToolNames filter is truthful even in the kill-switch fail-closed state, because applyExposed folds every registered tool into _disabled there (McpToolRegistryImpl.kt:417-427) — so the list empties rather than offering inert rules.
#662 split is done: no activity-log dialog, no ledger reader, and the AGENTS.md addition is scoped to the policy write. The new paragraph now describes what the code checks (absence under the lock, both revocation scopes, provider DENY, the fault), and drops the earlier preserveDeny-is-the-whole-guard overstatement. No em-dashes in the added prose, and **Both**-style claims are backed by tests.
The one item I would still look at
Refused is four causes with one sentence, and two of them are self-correcting while two are permanent. McpPolicyEngine.kt:311-314 returns the same Refused for a stale generation, a rule that now exists, an effective DENY, and PersistedPolicyUnreadable. The first two resolve themselves — the refresh re-stamps, or the row moves to the saved-rules list. The last two are stable states, so the copy at :390-392 ("Candidates refreshed; review the current policy before trying again") sends the operator round a loop that refuses identically every time:
- Damaged policy file is the sharp case. Every proactive Allow and Deny refuses, forever, and the real remedy is file recovery — which AGENTS.md says to do only after preserving a backup. The host does surface
fault.messagein the bottom bar (BossBottomBar.kt:257-267), but this modal is a parent-sized always-on-top window, so while the dialog is open that line is behind it. The engine already knows:fault.value is PersistedPolicyUnreadableis exactly this case, and saying so would turn a dead retry into an actionable message. - An effective DENY refuses a narrowing write too.
policyFor(...) == DENYis the right guard for ALLOW; for DENY the operator is asking for something strictly safer than the state the guard is protecting, and a tool-scoped DENY is durable in a way a provider DENY or a non-ASK default is not — remove the provider rule later and the tool falls back to its default rather than to the DENY the operator tried to pin. Reachable today only through a hand-edited provider DENY or hand-edited defaults (there is no UI that writes provider DENY), so this is low severity, not a hole. The fault branch must keep refusing both actions — a successful write clears_fault(:256) and would overwrite the damaged file, which is the one thing this guard cannot give up.
Nits
- Only the height is window-bounded.
Modifier.width(480.dp)(:106) is fixed, so a narrow window clips horizontally — and Close is right-aligned, so it is the thing that goes. The rendered test uses a 700dp-wide container, which does not exercise it.widthIn(max = windowWidth - 32.dp)would make the bound symmetric with the height fix. - No mutation warning in the proactive confirmation.
ProactiveAllowConfirmationreproduces the approval dialog's risk line and scope sentence exactly, but not its⚠ This tool performs mutations or external execution.row (McpApprovalDialog.kt:210-218);McpMutatingToolCatalog.isMutatingis reachable from here, and for a durable grant that is the line most worth repeating. failedSetis a single slot across all rows (:274) — a success on tool B clears tool A's message. Same accepted trade-offfailedRevokedocuments at:92-95, and it deserves the same comment so it reads as deliberate rather than copied. Related: after aRefusedthat came from "a rule now exists", the refresh removes the row and the message goes with it, so the only feedback is the row silently relocating to the saved-rules list.- The rendered test writes PNGs unconditionally (
McpProactivePolicyDialogTest.kt:78-85) intobuild/reports/mcp-review, with an inlinejavax.imageio.ImageIOcall, while the assertion is bounds-only. Useful while building the layout; as committed it is scaffolding every CI run pays for. The hardcoded360.dpin the same helper would also be better read off the container size the test provides. providerIddefaults tonullonsetToolPolicyIfAbsent. Consistent withpolicyFor/confirmInvocation/setToolPolicy, so I would not change it on its own — but it is the parameter that carries the provider-scope half of the guard, and a future call site that omits it loses that silently. This repo's own convention for exactly that shape (blockingDependentsOf'sisDisabled) is "no default, so a new call site has to answer."
Verification
No Gradle here (no dependency network), so compile/ktlint/detekt/test status is the author's and hosted CI's, not mine. Everything above is from reading this head. Shallow-clone caveat on the diff is noted at the top.
Net: the item the last round left open is closed at the right layer — the absence check is atomic, inside the engine's own lock, and it is the check that actually protects an intervening explicit ASK or ALLOW, which no generation token could. Both guards the review asked to see restored are present and tested (provider DENY through providerId, damaged policy through the fault branch, with the file asserted byte-identical), the refusal/storage distinction is a real type rather than one string, refusal re-stamps the candidate so retry works, confirmation is bound to the full candidate, and the dialog is bounded by the originating window with a rendered test that asserts Close's unclipped bounds at 700×360. What remains is a messaging split for the two refusal causes that cannot self-correct, plus the nits above.
· branch feat/mcp-governance-visibility
Contributor
|
@Antriksh1984 Your atomic absence check and standalone policy scope are retained and credited. The latest Claude review confirms the stale-write, confirmation and layout repairs. I addressed its remaining actionable feedback:
I am retaining the existing conservative effective-DENY guard for both proactive actions. The review explicitly describes a narrowing exception as low severity/optional; the UI now accurately says the current policy already denies the tool, rather than implying a retry will save it. This preserves the reviewed no-DENY-replacement contract. Optional providerId stays aligned with the existing engine APIs; the sole production caller supplies providerId, and both candidate/engine tests exercise it. Locked local validation on this follow-up: 147 tests passed, 0 failures/errors, including six rendered tests and ten atomic-write tests; detekt and formatting passed. Current-head CI must complete before merge. Source-only Claude review is distinguished from these executed tests. No new review request, draft cycle or database deployment. |
kshivang
added a commit
that referenced
this pull request
Sep 15, 2026
* feat(search): index open tabs by their URL and file path (#606) (#648)
Global Search matched an open tab only against its title, and TabResult
hardcoded url = null / filePath = null on every hit. A browser tab whose
title never mentions its domain, or an editor tab someone wants to find
by path rather than filename, could not be found at all.
searchTabs now also scores a browser tab's FluckTabInfo.currentUrl and
an editor tab's EditorTabInfo.filePath, through the same substring-
containment gate (proseScore) searchRecentPages already uses for a
page's URL, since a fuzzy subsequence match over a whole URL or path is
close to meaningless. TabResult carries the real url/filePath instead
of null, and the tab row's subtitle now leads with the workspace name
(disambiguating identical URLs open in different workspaces, or across
windows) followed by the url/filePath/tabType, in place of the old
"tabType • workspaceName" line.
11 tests cover: URL-only and file-path-only matches, both fields
populated for the right tab type and left null for the other/a blank
path, a short query not matching a URL by fuzzy accident, title+URL
both matching taking the higher (title) score with title highlight
ranges preserved, live navigation changing which URL matches (and the
old URL no longer matching), directory-path matching without a
subsequence false-positive, and no-open-tabs contributing nothing
rather than failing.
* fix(browser): replace superseded capture requests (#509)
* 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 d…
kshivang
added a commit
that referenced
this pull request
Sep 15, 2026
* feat(search): index open tabs by their URL and file path (#606) (#648)
Global Search matched an open tab only against its title, and TabResult
hardcoded url = null / filePath = null on every hit. A browser tab whose
title never mentions its domain, or an editor tab someone wants to find
by path rather than filename, could not be found at all.
searchTabs now also scores a browser tab's FluckTabInfo.currentUrl and
an editor tab's EditorTabInfo.filePath, through the same substring-
containment gate (proseScore) searchRecentPages already uses for a
page's URL, since a fuzzy subsequence match over a whole URL or path is
close to meaningless. TabResult carries the real url/filePath instead
of null, and the tab row's subtitle now leads with the workspace name
(disambiguating identical URLs open in different workspaces, or across
windows) followed by the url/filePath/tabType, in place of the old
"tabType • workspaceName" line.
11 tests cover: URL-only and file-path-only matches, both fields
populated for the right tab type and left null for the other/a blank
path, a short query not matching a URL by fuzzy accident, title+URL
both matching taking the higher (title) score with title highlight
ranges preserved, live navigation changing which URL matches (and the
old URL no longer matching), directory-path matching without a
subsequence false-positive, and no-open-tabs contributing nothing
rather than failing.
* fix(browser): replace superseded capture requests (#509)
* 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 d…
21 tasks done
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
3 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.
Change
Operators can set a persistent Allow/Deny rule for a registered MCP tool before an approval prompt occurs. The existing policy dialog remains reachable with zero saved rules, describes the tool-name-wide scope and risk, and requires a second click for Allow.
The host engine atomically requires no existing tool rule, the captured tool/provider revocation generation, and no effective DENY or unreadable-policy fault. A stale write refreshes candidates and requires a new decision. Stable DENY, unreadable-policy recovery and storage failures each get separate feedback. Confirmation is tied to the full candidate snapshot. The dialog follows the originating window width/height, with a fixed fallback before measurement, and scrolls with Close outside the scrolling body.
This PR contains privileged host policy controls only. The independent read-only history viewer is #662; #416 tracks observation/plugin architecture. No plugin policy-write surface is introduced.
Credit
Original implementation and subsequent contributor fixes: @Antriksh1984 (Antriksh), including proactive policy UI, candidate filtering/generation capture, atomic add-only-if-absent engine outcome, and candidate/engine tests. Review-agent repairs restore effective-DENY/damaged-policy guards, carry typed outcomes into the UI, refresh refused candidates, bind confirmation to candidate identity, add viewport bounds and rendered regression tests, and integrate current dev. Contributor authorship/history is preserved.
Validation
Local locked desktopTest: 147 tests passed, including six rendered interaction/short-window/recovery cases and ten add-only-if-absent engine regressions. Detekt passed and commonMain/desktopTest sources were formatted. Captured short-window layout inspected; light-theme confirmation replacement is exercised by the interaction test. Current-head hosted CI and Claude review are checked before merge. No live database changes or second application launch.