Repository navigation
feat(extension): complete H3 local control center - #837
efraespada wants to merge 71 commits into
Conversation
…ontrol-center-h3.md
…es-and-accessible-configuration: establish H3 profiles and settings migrations
…es-and-accessible-configuration: add settings transactions and fenced center editing
…es-and-accessible-configuration: add typed Svelte review and source boundaries
…es-and-accessible-configuration: mount packaged control center and guarded settings routes
…es-and-accessible-configuration: manage protected named read sessions
…es-and-accessible-configuration: bind profile scopes and repository controls to read identities
…es-and-accessible-configuration: apply verified profiles to runtime and read admission
…es-and-accessible-configuration: add localized feature inventory and verified preferences
…es-and-accessible-configuration: preserve discovery and drafts during configuration recovery
…es-and-accessible-configuration: verify retired clients and queued operations without coverage gaps
…es-and-accessible-configuration: inspect native permissions and privacy scope
…es-and-accessible-configuration: configure protected local navigation shortcuts
…es-and-accessible-configuration: deliver resumable welcome and independent support Reuse reviewed local onboarding progress with transient exploration, selected-profile explanations and separate access guidance. Keep Help, About and Recovery usable without readable settings, with bounded native package observations and factual save/read status. Document the flows, closed links, architecture and recovery; preserve the complete H3/product acceptance gates. Local verification passes 104 extension suites/1,072 tests with every coverage metric at 100%, plus types, specification and documentation contracts. Native H3 and remote review/Codecov acceptance remain pending.
…es-and-accessible-configuration: clarify startup migration and remove unused test import Clarify that independent support adds no automatic preference mutation while the existing supported startup migration remains authoritative. Remove the unused Svelte test import reported by lint. Runtime inputs and coverage-producing assertions are unchanged; rerun the affected mounted suite and specification/documentation gates.
…es-and-accessible-configuration: isolate helper failures and retain bounded closed health reasons Keep valid page configuration separate from helper failures. Current failures quarantine only the affected helper; retired or reentrant failures cannot disable replacements. Contain eligibility, allocation, scope and diagnostic delivery errors while preserving independent controllers. Keep at most 100 immutable closed in-memory health reasons and expose only a native same-extension, source/context-bound readonly snapshot. Invalid configuration does not release quarantine; hidden, protected, changed and disposed contexts cannot disclose stale facts. No network, persistence, grant or generic dispatcher is added. Preserve complete H3 activity/history/privacy/native/human acceptance requirements. Update owning SDD, contributor/runtime documentation and catalog evidence. Full extension verification passes 106 suites/1,109 tests at 100% of all coverage metrics; types, specification/docs, lint and Graphify pass. Native H3 and remote review/Codecov acceptance remain pending.
…es-and-accessible-configuration: add bounded transactional history core
…es-and-accessible-configuration: fence reviewed history clear and verify observed outcomes
…es-and-accessible-configuration: coordinate preferences and gate history with durable consent
…es-and-accessible-configuration: coordinate durable collection restrictions
…es-and-accessible-configuration: review durable collection start and stop
…es-and-accessible-configuration: retain bounded truthful operation history
…es-and-accessible-configuration: share native history ownership and reviewed controls
…es-and-accessible-configuration: compose reviewed local history control center
…es-and-accessible-configuration: capture consented local destinations
…es-and-accessible-configuration: observe preference operations independently
…es-and-accessible-configuration: verify protected session persistence canonically
…es-and-accessible-configuration: observe native session operations independently
…es-and-accessible-configuration: observe compound collection effects independently
…es-and-accessible-configuration: observe reviewed history clear without recording its own deletion
There was a problem hiding this comment.
Current status: All findings originating in this review are resolved.
Last reconciled on1583027. See aggregate Bugbot status.
🤖 Bugbot review snapshot
Bugbot reported 4 potential problems when commit 78ee160 was analyzed. This snapshot is historical; use the status block above for current state. 4 findings are linked to changed code.
Findings
- medium: Concurrent previews can bypass the 16-operation limit —
extension/src/application/settings_operations.ts:79 - medium: Import uses the revision captured when the page first mounted —
extension/src/presentation/DataPage.svelte:8 - medium: Send Keys requests omit the required value array —
extension/tooling/webdriver_page.mts:35 - low: Loading refresh exposes retained settings as current —
extension/src/domain/center_client_view.ts:21
vypbot
added
state:changes-requested
state:awaiting-issue-author
and removed
state:reviewing
state:ai-processing
labels
…ntrol-center-profiles-and-accessible-configuration
Branch synchronized
6 verification checks passed. |
|
/copilot recheck |
Request could not be completed
Action: Inspect the current state and retry the failed step. Retained state: Existing persisted state and completed external effects were preserved. Error code: Retryable: Yes Reference: |
…es-and-accessible-configuration: bound preview work and refresh import fences
vypbot
added
state:ai-processing
state:reviewing
and removed
state:blocked
state:awaiting-maintainer
labels
There was a problem hiding this comment.
Current status: 2 findings originating in this review require attention.
Last reconciled on1583027. See aggregate Bugbot status.
🤖 Bugbot review snapshot
Bugbot reported 3 potential problems when commit 7acba0c was analyzed. This snapshot is historical; use the status block above for current state. 3 findings are linked to changed code.
Findings
- medium: Escaped CSS function names bypass resource checks —
extension/tooling/style_validation.mts:21 - low: The H3 risk-case floor omits later required additions —
specs/github-browser-extension-control-center-h3.md:1492 - low: Unavailable native state accepts a fabricated emergency value —
extension/src/domain/runtime_provenance_validation.ts:24
| if (/(?:url|src|image|image-set|cross-fade|paint|expression)\s*\(|https?:|data:|\/\//iu.test(clean) | ||
| || /^(?:-moz-binding|behavior)$/iu.test(String(value.property))) fail('style-resource-value'); | ||
| } |
There was a problem hiding this comment.
Escaped CSS function names bypass resource checks
Severity: medium
Category: security · Confidence: 89%
Location: extension/tooling/style_validation.mts:21-23
The declaration-value guard only recognizes literal function names such as url(. CSS escapes are decoded by the browser, so a value such as u\\72l(/resource) is interpreted as url(/resource) but contains no matching url(, https:, data:, or // text. Because this validator is used for packaged component and stylesheet CSS, such a declaration can cause an unapproved resource request despite the stated no-fetched-resources policy.
Evidence:
clean removes comments but does not decode CSS escapes before the regex checks. An escaped CSS identifier such as u\\72l(/resource) decodes to url(/resource) in CSS and can load a relative resource when emitted.
Suggested fix:
Decode CSS escapes before checking declaration values, or reject escaped identifiers in resource-bearing values and validate the resulting CSS token stream.
There was a problem hiding this comment.
The reported escape is already rejected before parsing. In 1583027, style_validation.mts line 9 rejects every backslash with style-escape; the declaration regex is a later check. The existing component_styles.test.ts escaped-name case passes. Direct Node24 reproduction of the relative resource form u\72l(/resource), through both validateStyles and componentScript with a used h1 selector, also fails with style-escape. No resource exception was added. Please explicitly re-adjudicate against that earlier admission guard.
| if (value.worker !== 'clear') return ['pending', 'precaution', 'pending-precaution', 'unavailable'].includes(value.worker as string) | ||
| && value.native === 'not-checked' && value.emergency === null && value.tab === null && value.restricted; | ||
| if (value.native === 'future' || value.native === 'corrupt') return value.emergency === null && value.tab === null && value.restricted; | ||
| if (value.native === 'unavailable') return value.tab === null && value.restricted; |
There was a problem hiding this comment.
Unavailable native state accepts a fabricated emergency value
Severity: low
Category: correctness · Confidence: 82%
Location: extension/src/domain/runtime_provenance_validation.ts:24
When the native pause record is unavailable, this branch requires tab to be null and restricted to be true but does not require emergency to be null. The canonical nativePauseProvenance producer sets both emergency and tab to null for an unavailable record. Consequently, decodeRuntimeConfiguration accepts a receipt claiming an emergency pause despite the record being unavailable, and runtimeProvenanceView displays that value as a confirmed yes/no fact instead of unknown.
Evidence:
validNativePauseProvenance accepts native: 'unavailable', tab: null, restricted: true, and either emergency: true or false; the presentation maps native.emergency to a visible yes/no row. The producer emits null for both values when native state is unavailable.
Suggested fix:
Require value.emergency === null in the unavailable-native branch so an unavailable record cannot assert an emergency fact.
Apply this change:
| if (value.native === 'unavailable') return value.tab === null && value.restricted; | |
| if (value.native === 'unavailable') return value.emergency === null && value.tab === null && value.restricted; |
There was a problem hiding this comment.
The canonical producer has another valid branch: a ready pause record with an unavailable native tab ID. temporary_pause_observation.ts returns native=unavailable, the independently observed emergency boolean, tab=null and restricted=true. Both facts are null only when the record itself is unavailable. The pre-existing unknown-tab contract already covers emergency=true; 1583027 also tests false through the actual producer, public factory and wire decoder. Requiring emergency=null breaks the canonical affirmative case and discards known facts. Both cases remain paused/disconnected. Existing unavailable-record tests still require two unknown facts. Please explicitly re-adjudicate this finding against that distinction.
…es-and-accessible-configuration: reconcile risk floors and native observation evidence
There was a problem hiding this comment.
Current status: 2 findings originating in this review require attention.
Last reconciled on1583027. See aggregate Bugbot status.
🤖 Bugbot review snapshot
Bugbot reported 2 potential problems when commit 1583027 was analyzed. This snapshot is historical; use the status block above for current state. 2 findings are linked to changed code.
Findings
- medium: Scoped restoration can activate a session after its request becomes stale —
extension/src/application/protected_session.ts:120 - low: Pending pause writes cause closed-tab cleanup to be lost —
extension/src/application/closed_pause_cleanup.ts:22
| const permitted = await this.ports.permitted(); | ||
| if (epoch !== this.epoch) return; | ||
| if (!permitted) { await this.revoke(); return; } | ||
| if (!permitted || !current()) { await this.revoke(observer, 'cleanup-active'); return; } | ||
| this.connection = connection; this.state = 'connected'; |
There was a problem hiding this comment.
Scoped restoration can activate a session after its request becomes stale
Severity: medium
Category: correctness · Confidence: 94%
Location: extension/src/application/protected_session.ts:120-123
When restore is called with a RestorationScope, the scope is checked through the identity and repository reads and before persistence. After persistence, however, establish awaits ports.permitted() and checks only the service epoch. If the page scope is cancelled or becomes stale during that await, the method still assigns connection and marks the session connected. A stale page request can therefore activate a retained credential after it is no longer eligible to restore it.
Evidence:
restore supplies a current function that includes both the request scope and cancellation state (lines 59–62), and establish checks it after each identity GET. But after the final asynchronous permission check, lines 120–123 test only epoch !== this.epoch; a scope change does not increment that epoch. The stale path then reaches this.connection = connection; this.state = 'connected'.
Suggested fix:
Recheck current() and cancellation.aborted after the permission await and before activating the connection. If the scope is stale, return without setting connection; the existing finally path leaves the session disconnected.
Apply this change:
| const permitted = await this.ports.permitted(); | |
| if (epoch !== this.epoch) return; | |
| if (!permitted) { await this.revoke(); return; } | |
| if (!permitted || !current()) { await this.revoke(observer, 'cleanup-active'); return; } | |
| this.connection = connection; this.state = 'connected'; | |
| const permitted = await this.ports.permitted(); | |
| if (epoch !== this.epoch || !current() || cancellation.aborted) return; | |
| if (!permitted) { await this.revoke(observer, 'cleanup-active'); return; } | |
| this.connection = connection; this.state = 'connected'; |
| const ids = [...this.closed]; this.closed.clear(); | ||
| const inspected = await this.store.inspect(); | ||
| if (this.stopped) return; | ||
| if (inspected.loaded.state !== 'ready' || inspected.pending) continue; |
There was a problem hiding this comment.
Pending pause writes cause closed-tab cleanup to be lost
Severity: low
Category: reliability · Confidence: 94%
Location: extension/src/application/closed_pause_cleanup.ts:22
When a tab closes while any temporary-pause write is pending, inspect() reports pending and this branch skips cleanup. The closed IDs were already removed from this.closed at line 19 and are not requeued. If a closed tab had a saved pause, that stale tab ID remains in storage; a later tab that receives the reused ID can inherit the old pause.
Evidence:
TemporaryPauseStore.inspect() returns pending: this.guard.pending (with writes tracked by pending in TemporaryPauseStore.enqueue). ClosedPauseCleanup.drain() snapshots and clears this.closed before inspection, then continues without restoring those IDs when inspected.pending is true. The persisted tab-scope list is keyed by native tab ID, so skipping this expiration can leave a restriction attached to an ID after its original tab closes.
Suggested fix:
Retain the closed IDs and retry cleanup after pending mutations settle, while keeping the retained queue bounded.
Closes #704.
Adds the packaged local control center for the browser extension. Users can discover the 150-feature inventory, understand profile and repository precedence, and review changes before saving. Twenty routes cover appearance, language, profiles, workspaces, connections, permissions, local data, activity and recovery in four locales. Capabilities delivered by later milestones display their actual availability and dependencies.
Implementation
Verification
Current contribution:
15830271cae1975771ccc5c6fa8f96767f314f28, based on the Action integration ofdevelopat9d3915729c96ec93c6e064d3321ba54c790170eb. Normal build/Jest/lint hooks pass 842 suites / 16,299 tests (35 existing skips). The complete extension campaign passes 5,232 tests with 100% statements, branches, functions and lines, including 8,110/8,110 lines. Fifteen new risk cases cover reserved preview work, import revision capture and admission of current settings; eight reproduced failures before correction. The current follow-up reconciles every additive risk minimum into an 881-case ledger, keeps known emergency facts when only the native tab scope is unknown, and gives macOS 30 minutes after its complete coverage campaign exhausted 20. Current-head CI/platform results, complete semantic review and remote Codecov remain merge gates.The contribution also passed normal build/Jest/lint commit hooks, extension types/Svelte (zero errors or warnings), specification/catalog, documentation and strict full-range whitespace validation. Graphify is updated. The changed preparation/import/current-document paths pass 77 focused cases on Node 24.0.0 as well as Node 24.20.0; the current complete extension campaign ran on 24.20.0.
Historical native evidence below is bound to input SHA-256
5908308a0d7fbebed49c80610f15097787852e08b37624e6d92974e957572bf8:These native executions used the contribution at
78ee16003. After Action integration, all 156 raw target assets and the complete input digest were independently verified identical at129cdecee; no second execution is implied. The current corrected input is927f1e8178b86dd60d0494293e68aaf3fd1249d5a955a6f5b5a9b679dbdaf791; Chrome and Safari require fresh native verification. Firefox executed 41 native assertions across 20 routes at7acba0ccf; all 156 raw target assets and the complete input digest are independently identical at15830271c, without normalization or an invented second execution. Chrome toolbar and browser-options entrypoints have also been verified through the actual isolated browser UI at15830271c; its 41-assertion/20-route component journey and four-case appearance matrix pass with preferences restored and the owned session closed. Native harness receipts and reproducible commands are described in the contributor guide.Remaining acceptance
developbase with 100% modified coverable lines, zero misses and zero partials. Commit currently reproduces the independently tracked progress output-length defect 🐛🧑💻 - Progress partition rejects per file implemented text with schema string long and no file identity #830; that Action fix is being admitted on its own managed issue branch. No failed check or output limit is waived.Copilot / Approvalfor protected paths on the exact final head; no such decision has been received for this PR.The maintainer deferred full manual VoiceOver and final visual/interaction acceptance to whole-product H12 (#713). Automated accessibility and functional checks remain in scope. Local development containers do not establish signed-update or store readiness.