optimize: codebase cleanup across 8 dimensions + tools/list_changed - #63
Merged
Conversation
Every check was computed, logged, then discarded; each test function ended in an unconditional 'return true' and runAllTests never set an exit code. CI's Structure tests step could not fail, so deleting the body of background.js kept CI green. Checks now fold into the return value and a failure exits 1. Also collapses the three near-identical per-script test functions into one table-driven check.
Dead bindings, each verified unreferenced repo-wide: - server.js httpServer (assigned, never read) - server.js successIcon in formatPageStyleResult (the one in formatElementFillResult is live) - content.js pageType in detailedAnalysis (dead binding plus a wasted DOM scan every call) - content.js opacity in buildMoodCSS (computed, never interpolated) Dead files: - opendia-extension/manifest.json: build.js copies manifest-<browser>.json, nothing reads this one, and every path inside it (background.js, popup.html, content.js, icon-16.png) is wrong for the current src/ layout so it could not load anyway. - opendia-mcp/.env.example: server.js has zero process.env references and no dotenv dep. - opendia-extension/releases/*.zip: 22MB of tracked 1.0.6 artifacts against a 1.1.x tree, referenced nowhere. Now gitignored; releases ship via GitHub. build-dxt.sh pointed installers at the deleted manifest, and never built the extension it bundles. It now builds first and points at dist/chrome and dist/firefox, and drops node_modules from the copied tree. Note: page_style 'intensity' is now visibly inert (it was already, via the dead opacity binding) while still being echoed back as applied. Left for a product call.
…ent 7)
ConnectionManager.connect() branched on isServiceWorker into two arms that were
byte-identical apart from a console.log string; ensureConnection() then
re-implemented the same readyState guard a third time before calling connect(),
which checks it again. Its comment ('Chrome: Always create fresh connection') had
also become false. Both collapse to the single guard.
The genuine MV3/MV2 differences (heartbeat setup, reconnect scheduling) are
untouched; they still branch on isServiceWorker.
Removed four permissions with zero call sites across background/content/popup:
notifications and webNavigation from both manifests, webRequest and
webRequestBlocking from Firefox. webRequestBlocking in particular is an AMO
review flag on an extension that registers no blocking listener.
madge and dpdm report no module cycles in either package. The real cycles are all in ConnectionManager, and share one root cause: socket lifecycle state lives on the instance while more than one socket can be alive at once, with no handler checking which socket it belongs to. 1. Socket identity guard. Handlers now close over the socket they were bound to and return early if it is no longer current. The server closes the previous socket when the extension reconnects (it already has the mirror-image guard), so a stale close event was clearing the live connection's heartbeat, incrementing the attempt counter on a healthy link, and arming a reconnect interval that nothing could clear - a no-op connect() every 5-30s for the life of the background page. 2. Attempt double-counting. Both onerror and onclose incremented reconnectAttempts, but onclose always follows onerror for a failed socket. With backoff 5000*2^n and a <10 gate, the extension gave up after roughly 5 real attempts instead of 10. Counting now happens only in onclose. 3. One-shot reconnect. setInterval fired again while the previous attempt was still in discoverServerPorts (7 sequential fetches with no timeout), overwriting this.mcpSocket and orphaning the pending socket, whose later close fed the loop. Now setTimeout; createConnection's catch and onclose already re-arm on failure. Field renamed reconnectInterval -> reconnectTimer to match. Not covered by the gate: none of this is reachable from the structural tests. Needs a live server restart under a loaded extension to confirm.
…nt 1) jscpd measured 21 clones over 428/6856 lines. These are the two that were both duplicated and drifting. pingContentScript(tabId, timeoutMs) replaces three copies of the same sendMessage-with-timeout race in background.js. The copies had diverged: two rejected on runtime.lastError, the third silently skipped that check. Call sites keep their original timeouts (2000/3000/3000). getTabContentScriptStatus treats a failed ping as its answer rather than an error, so it catches to null - the same outcome the old code reached by letting an undefined response fall through. resolveTargetTab(tabId) replaces the explicit-tab-else-active-tab block copied across sendToContentScript, getSelectedText, and navigateToUrl. closeTabs keeps its own version: it deliberately tolerates a missing active tab and throws a different message. The rewrapped tabs.get error now keeps the underlying message. It previously reported every failure as 'not found or inaccessible', including bad id types and denied incognito access.
…es (agents 2, 5)
get_page_links advertised link_type and domains; content.js reads include_internal,
include_external, and domain_filter. Params are forwarded verbatim, so every filter
was silently dropped: asking for external links on github.com returned the first 100
links of every type and domain, reported as a successful filtered query. The server's
own fallback schema already declared the correct names, so background.js was the lone
outlier; it now matches, including the real default of 100 rather than 50.
Formatter reads that produced 'undefined' on every call:
- page_extract_content printed 'using undefined' on its default path (summarize:true
emits extraction_method, not method)
- page_scroll with amount:'custom' dropped the distance (scrollPage emits
requested_pixels, not pixels)
- tab_create batches printed 'Chunks processed: undefined' (never produced; now
computed from the clamped chunk size)
- page_wait_for printed 'Condition met: unknown' (waitForCondition now echoes
condition_type)
- page_analyze phase:'detailed' rendered every element as '(undefined) Not ready',
because the detailed branch emits a fingerprint and nests state under meta while
the formatter read the discover phase's top-level type/ready/state. The formatter
now handles both shapes; fixing it there rather than in the producer keeps an
older extension working against a newer server, since the two ship separately.
Input coercion:
- tab_create count only had range checks, and both comparisons coerce. count:'5'
passed, then Array('5').fill(url) produced ONE tab reported as '1/1 succeeded';
count:2.5 threw an opaque 'Invalid array length'. Now requires an integer.
- batch_settings.chunk_size of 0 or negative survived the destructuring default and
left the chunk loop never advancing - an unbounded spin in the service worker
while the server call timed out at 30s. Now clamped.
Substantive:
- build.js had a chrome/firefox post-processing branch whose own comments said it
did nothing. Removed; the manifest copy above already decided everything.
- Removed five debug console.log lines from the extraction path in content.js.
One of them echoed 200 characters of extracted page content to the console on
every page_extract_content call.
- executeDirectBypass already logs the method, then each of the four per-platform
functions logged it again. Dropped the four; fixed the shared line's emoji,
which was a bird for every platform.
Comments:
- The anti-detection comments said what the code did ('THE WORKING FORMULA',
'the magic sequence') but never why. Rewritten to record the actual knowledge:
Draft.js ignores programmatic value writes and synthesized input events, so
execCommand('insertText') is used for its trusted beforeinput. The differing
500/800/600ms settle delays now say what they are waiting for, since that is
the only real difference between the four functions.
- In-motion comments left by the previous pass narrated the change ('used to',
'previously', 'Resetting made the counter oscillate'). Rewritten to state the
invariant a reader needs.
- Dropped 'Enhanced' filler from nine comments; it never distinguished anything.
- Fixed three comments labelling live paths as legacy or original: the single-tab
branch of tab_create, formatToolResult's default arm (which serves
get_bookmarks and add_bookmark), and content.js's summarize:false branch.
- Noted genericDirectBypass as an unreachable-by-construction extension point
rather than leaving it looking like dead code.
…onnects
The extension connects over WebSocket independently of the MCP client, so
tools/list is answered from getFallbackTools() whenever the client lists first -
the likely case on a cold start, since the client spawns the server and lists
immediately while the browser may not even be running yet.
The server never told the client that could change: capabilities advertised
'tools: {}' with no listChanged, and no notification was ever sent. A client had
no signal to re-list and no reason to expect one, so it kept the stale fallback
schemas for the rest of the session - schemas that are missing tab_id on the
tools that support it, and that describe a tab_id parameter on five tools whose
fallback schema never declares it.
Now: capabilities declare tools.listChanged, and a notification is emitted when
the extension registers tools and when it disconnects. Suppressed before the
client initializes, and skipped when the tool set is unchanged, so a reconnecting
extension re-registering the same tools does not spam the client.
Delivery covers both transports: written to stdout on the stdio path, and pushed
to open /sse streams, which required tracking them (the response objects were
previously local to the GET handler with no registry).
Verified end to end against the real server: cold tools/list returns fallback ->
fake extension registers over WebSocket -> exactly one well-formed JSON-RPC
notification arrives (on stdout, and separately on an open SSE stream) ->
re-list returns the extension's schemas with tab_id present -> an identical
re-register sends nothing further -> disconnect notifies again.
pull Bot
pushed a commit
to esinanturan/opendia
that referenced
this pull request
Jul 29, 2026
Runtime tests. CI was structural only - it greps built files and never executes a tool call - so the connection-layer and protocol work merged in aeonfun#63 had no automated coverage. Two suites now run in the MCP job: test-protocol.js drives the real server over stdio and SSE: cold tools/list returns fallback schemas, a fake extension registers over WebSocket, exactly one well-formed tools/list_changed arrives on both transports, re-list returns the extension's schemas, an unchanged re-register stays silent, and a disconnect notifies again. test-connection.js loads the real background.js under stubbed extension globals (simulating Firefox MV2, the heartbeat path) and drives it against the real server: a replaced socket's close event must not clear the live connection's heartbeat, inflate the attempt counter, or arm a reconnect; a failed connect must increment attempts once, not twice; and the reconnect must be one-shot rather than fixed-rate. Both were validated against the pre-fix code at aefab6a, where the connection suite fails 5 of its assertions - so these detect the regressions they describe rather than passing vacuously. They live in opendia-mcp because they need the server and the ws client, which are that package's dependencies; test-connection.js reads the extension source by path and does not import from that package. Neither ships to npm - the package's files list is server.js and README.md. npm test is now these suites plus the syntax check, replacing a stub that always exited 1. Dependency overrides. Two high-severity Dependabot alerts, both development scope and both reached only through web-ext: brace-expansion 1.1.15 -> 1.1.17 (via multimatch -> minimatch), GHSA-3jxr-9vmj-r5cp shell-quote 1.8.4 -> 1.10.0 (via fx-runner), GHSA-395f-4hp3-45gv Both are denial-of-service by input complexity in build tooling. Nothing reaches a user: web-ext is a devDependency and node_modules is not part of the extension bundle. Overrides because web-ext pins the vulnerable ranges transitively. web-ext lint still reports 0 errors and the same 3 pre-existing warnings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Codebase cleanup pass across 8 dimensions, one reviewable commit per domain, plus one behavioural fix that came out of it.
The gate came first
test-extension.jscomputed 12 structural checks, printed them, and discarded every one — each test function ended in an unconditionalreturn true, andrunAllTestsnever set an exit code. CI's "Structure tests" step could not fail; deleting the body ofbackground.jskept it green. That landed first (b735d27), verified to pass on the real tree and exit 1 when broken. Every later "gate green" claim in this PR depends on it.Highest-impact fixes
get_page_linksignored every filter it advertised. The schema declaredlink_type/domains;content.jsreadsinclude_internal/include_external/domain_filter. Params are forwarded verbatim, so asking for external links on a domain returned the first 100 links of every type and domain, reported as a successful filtered query. The server's own fallback schema already had the correct names, sobackground.jswas the lone outlier.Five formatter fields rendered
undefinedon every call, includingpage_analyze phase:"detailed"labelling all elements(undefined) ⚠️ Not ready— the detailed branch emits a fingerprint and nests state undermeta, while the formatter read the discover phase's top-level shape.Stale-socket handling. WebSocket handlers closed over
thiswith no check that the firing socket was still current. The server closes the previous socket on reconnect (it already has the mirror guard), so a stale close event cleared the live connection's heartbeat and armed a reconnect that nothing could clear.tools/list_changed. The extension connects independently of the MCP client, so a cold-start client listed first and got fallback schemas. The server declared nolistChangedcapability and never notified, so the client kept stale schemas for the whole session. Now declared and emitted on both transports.Commits
b735d2766108b0aefab6a0174d6fe57e6a9pingContentScript/resolveTargetTabhelpers847d281e3663ee9eabccenotifications/tools/list_changedNet −127 lines of source, plus 22MB of tracked 1.0.6 release zips removed (now gitignored).
Verification
Gate green and identical to baseline:
node --check, extension build,build.js validate, structure tests,web-ext lintat 0 errors / 3 pre-existing warnings (APPLICATIONS_DEPRECATED,MISSING_DATA_COLLECTION_PERMISSIONS,UNSAFE_VAR_ASSIGNMENT).tools/list_changedwas verified end-to-end against the real server: coldtools/listreturns fallback → fake extension registers over WebSocket → exactly one well-formed JSON-RPC notification (on stdout, and separately on an open SSE stream) → re-list returns the extension's schemas → identical re-register sends nothing → disconnect notifies again.Caveat: the gate is structural only — it greps built files and never executes a tool call. The connection-layer changes are reasoned and syntax-checked, not runtime-verified. They want a live server restart under a loaded extension before being considered confirmed.
Deliberately not included
webextension-polyfill. It no-ops on Firefox (its own guard passes through when nativebrowserexists), so it does real work only in Chrome content scripts. But the finding underneath is bigger:background.jscalls Chrome callback-style APIs against Firefox's promise-onlybrowser.*, includingsendToContentScript— the path every content-script tool uses. Static analysis only, no Firefox available to confirm. Needs a real cross-browser test loop, not a cleanup pass.getFallbackTools()(511 lines, 388 normalized lines diverged). Now safe thanks tolist_changed, but what a client should see before the extension connects is a product call.page_styleremember/intensity;ai_mood's exact-match table (its own documented example misses it);page_scrollwheremediumscrolls 2.5× further thanlarge.