Add CI matrix, and expose console + performance monitoring over MCP - #7
Draft
MadTinker wants to merge 7 commits into
Draft
Add CI matrix, and expose console + performance monitoring over MCP#7MadTinker wants to merge 7 commits into
MadTinker wants to merge 7 commits into
Conversation
MadTinker
force-pushed
the
claude/ci-workflow
branch
from
August 22, 2026 18:46
de45802 to
2190459
Compare
MadTinker
changed the base branch from
claude/remove-dead-modules
to
claude/mcp-stdout-logging-fix
August 22, 2026 18:47
MadTinker
force-pushed
the
claude/mcp-stdout-logging-fix
branch
from
August 22, 2026 18:51
cbe0af3 to
28f7cf8
Compare
MadTinker
force-pushed
the
claude/ci-workflow
branch
from
August 22, 2026 18:51
2190459 to
9a2b05b
Compare
The repository had no CI. A passing test suite that nothing runs is a suite that silently stops passing. .github/workflows/ci.yml builds and tests on all three platforms, with fail-fast disabled so one platform's failure doesn't mask the others. Notes on the design: - Uses the runners' preinstalled Rust and only first-party actions (checkout, cache), so no third-party actions enter the supply chain. - The Linux lane installs libgtk-3-dev and libwebkit2gtk-4.1-dev. Cargo cannot declare these, and without them the build fails inside gdk-sys with an error that never names the fix. Keeping the step in the workflow means it stays correct in a way documentation does not. - Builds before testing. tests/mcp_tests.rs spawns `cargo run -- --mcp` and waits a fixed two seconds for startup, which would be tight if the binary still had to compile. - No xvfb or display setup: no surviving test creates a window. Verified by running the full suite with no DISPLAY and no X server. - No fmt or clippy gate. The tree is not rustfmt-clean and the build emits warnings; gating either today would make the workflow red on arrival. Both are recorded in the roadmap as deliberate follow-ups. The Linux lane is verified end-to-end locally: cargo build --all-targets succeeds and cargo test reports 167 passed, 0 failed. The macOS and Windows lanes have never been exercised, so the first run may surface genuine cross-platform breakage — which is the matrix doing its job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pg53xf5xcUV7oc5CkaiCpz
MadTinker
force-pushed
the
claude/ci-workflow
branch
from
August 22, 2026 18:56
9a2b05b to
33fbfc0
Compare
Console capture and the performance suite were both fully implemented and
reachable over REST, but absent from the MCP tool list. An agent driving the
browser could act on a page and never learn whether the page complained --
arguably the most useful question it could ask after an interaction.
Adds nine tools, taking the advertised surface from 16 to 25:
start_console_monitoring start_performance_monitoring
stop_console_monitoring stop_performance_monitoring
get_console_logs get_core_web_vitals
clear_console_logs get_memory_metrics
get_performance_summary
Each maps to a BrowserCommand the engine already handles, so this is a binding
rather than new capability. get_console_logs takes an optional level filter;
omitting it means all levels, and is not an error.
Tests assert the dispatched BrowserCommand rather than only that the call
succeeded, and check that every new tool is advertised in tools/list -- a tool
that dispatches but isn't listed is invisible to an agent, so both halves need
covering.
Verified over the real protocol: spawning the binary and calling tools/list
returns all 25 tools. cargo test reports 172 passed, 0 failed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pg53xf5xcUV7oc5CkaiCpz
MadTinker
changed the title
Recording and replay were reachable over REST only, so an agent could drive the browser but not capture what it did or replay it. Adds eight tools, taking the advertised MCP surface from 25 to 33: start_recording start_playback stop_recording stop_playback save_recording get_playback_state load_recording step_playback step_playback takes a direction rather than exposing StepForward and StepBackward as separate tools -- an agent narrowing down which event causes a failure thinks in terms of stepping. An unrecognised direction is an error rather than a silent default, because stepping the wrong way would mislead exactly the bisect the tool exists to serve. Also fixes two problems in ROADMAP.md: - It listed "CI of any kind. No .github/workflows." under Not started while also reporting the matrix green under M2. Self-contradictory, and precisely the drift the document exists to prevent. - Its test count was stale, and it described the keyboard gap as "expose browser/keyboard.rs". That module maps chrome shortcuts (Ctrl+T, Alt+Left) to commands the API already exposes directly, so binding it would add no capability. Testing tab order and keyboard accessibility needs events dispatched into the page, and synthetic KeyboardEvents from JavaScript cannot do it -- browsers refuse default actions like focus movement for untrusted events. Recorded as needing a design decision, not a binding. Verified over the real protocol: tools/list returns all 33 tools. cargo test reports 177 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pg53xf5xcUV7oc5CkaiCpz
The "Explicitly not doing" section recommended against embedding multiple JS engines, but was written without knowing that feat/js-engine-integration (30 commits, January 2025) had already attempted exactly that. Reviewed it. The conclusion stands, on stronger grounds than the original reasoning. That branch's SpiderMonkeyEngine builds a bare mozjs Runtime with SIMPLE_GLOBAL_CLASS, and JavaScriptCoreEngine a bare javascriptcore_rs Context: standalone interpreters with no DOM, no window, no document. Its Cargo.toml makes them independent of the webview rather than replacements -- webview = ["dep:wry", "dep:tao"] sits alongside v8 = ["dep:v8"] -- so the embedded engines never render the page. Cross-browser bugs live in DOM behavior, layout, CSS, and browser APIs. A bare ECMAScript interpreter cannot observe any of them, so even a working version would have answered a question nobody asked. The branch also never compiled; its own commit message records the build as failing. The CI matrix added in this PR already meets the underlying goal, running real WebKit and real Chromium against real pages. Also notes what is worth salvaging from that branch independently: its Cargo feature reorganization, which is sound practice regardless of the engine work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pg53xf5xcUV7oc5CkaiCpz
While scoping MCP resources, found that every MCP tool is fire-and-forget. handle_tool_call broadcasts a BrowserCommand and returns the string "Command '<name>' sent successfully"; the code carries the comment "in a real implementation, we'd wait for the response". So get_console_logs returns that sentence rather than any logs, and the same holds for get_core_web_vitals, get_page_info, find_element, execute_javascript, take_screenshot, and every other read. This predates the console and performance tools added earlier in this branch, but those commit and PR descriptions overstated what they delivered by saying an agent could now ask whether a click threw a console error. It can trigger the question and never receives the answer. Corrected here. The pieces exist but are not joined: the engine publishes results as BrowserEvents, and McpServer holds an event_rx receiver that nothing ever reads. The open design problem is correlation -- events carry no request id and some are emitted spontaneously by the page, so a time-window collect is racy. Records two candidate approaches and notes that whichever is chosen needs a timeout, since a blocking read without a deadline is exactly what hung CI on all three platforms earlier in this milestone. No behaviour change. Documents the limitation in the readme and the MCP guide so users reach for the REST API when they need a value back, and files the fix as the top item in Track A, blocking MCP resources and structured errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pg53xf5xcUV7oc5CkaiCpz
Implements the direct-accessor approach for the MCP result path. Reads that the engine can answer from its own state now return the value instead of the string "Command '<name>' sent successfully". BrowserState carries clones of the engine's Arc<Mutex<..>> handles for the console monitor, performance monitor, network monitor, and event player. The MCP server runs on its own thread in the same process, so it reads them directly. try_local_read answers these seven tools: get_console_logs get_network_stats get_core_web_vitals export_network_har get_memory_metrics get_playback_state get_performance_summary Everything else, including all actions, falls through to the existing broadcast path unchanged. Without attached state the server behaves exactly as before, so the change is additive. get_playback_state returns position, duration, index and event count rather than the bare PlaybackState enum, since the enum alone does not tell an agent what to do next. Not covered: get_page_info, execute_javascript, find_element and take_screenshot need the WebView itself, which is owned by the thread running the event loop. Those remain fire-and-forget and are still documented as such. Tests populate a monitor and assert the read returns the data, that a level filter applies to it, that actions still acknowledge, and that a server without state falls back cleanly. One of them caught a real defect in its own harness: dropping the command receiver makes broadcast::send fail, so action tools error. Separately, records a pre-existing bug found while chasing an intermittent test failure. --mcp starts the browser engine, which initialises GTK and aborts when no display exists, racing the MCP thread's reply. Sequential runs fail 0/20; three concurrent fail 11/24; --headless narrows but does not close the window at 3/12. This makes tests/mcp_tests.rs a latent CI flake that has passed on luck, and it affects the headless Claude Desktop configuration in the readme. Deferred by request, documented so a future red run is recognised. cargo test: 182 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pg53xf5xcUV7oc5CkaiCpz
main.rs declared api, browser, event, and templates, all of which lib.rs
already exported. The binary therefore compiled the whole crate a second time
and ran every shared test once per target.
mcp moves into the library, since it was declared only in main.rs, and the
binary now links against the library rather than re-declaring modules.
platform stays library-only, as before.
Before After
Incremental rebuild after touching
src/browser/mod.rs 21.5s 4.0s
Warnings from the binary target 91 2
Test executions 182 140
Unique test names 140 140
The fall in executions is duplication disappearing, not lost coverage:
`cargo test -- --list` yields identical sets of 140 names before and after,
with an empty diff.
This also corrects a number that earlier commits in this branch reported
incorrectly. Figures like "182 passed" counted duplicate executions and were
quoted as though they were test counts. The project has 140 tests; it had 140
before this change too.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pg53xf5xcUV7oc5CkaiCpz
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
2 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.
Why
The repository had no CI at all. A passing test suite that nothing runs is a suite that silently stops passing — and with development having been macOS-centric, nothing had ever verified that Tinker builds on Windows or Linux.
What
.github/workflows/ci.yml— build and test acrossubuntu-latest,macos-latest,windows-latest, on push tomain, on every PR, and on manual dispatch.fail-fast: false, so one platform failing doesn't cancel the others. Seeing which platforms differ is the entire point.Results from the first run
All three platforms compiled successfully — Linux 2m06s, macOS 1m20s, Windows 3m07s. That retires the "cross-platform is unproven" caveat for the build.
And the matrix immediately earned its keep by catching a hang. All three platforms then stalled in the test step for 19+ minutes against a suite that finishes in under a second locally.
The cause:
tests/mcp_tests.rsspawnedcargo runfrom insidecargo test. The child contended for cargo's build-directory lock and never started, while the parent blocked on aread_line()with no timeout. Deadlock on every platform.It had passed locally because a fully warm
target/let the child win the race — exactly the kind of environment-dependent flake that only CI exposes.Fixed in #5 by spawning
env!("CARGO_BIN_EXE_tinker"), the binary cargo has already built for integration tests. No nested cargo, no lock contention, and it's dramatically faster:Design notes
No third-party actions. GitHub runners ship a stable Rust toolchain, so there's nothing to install; only
actions/checkoutandactions/cacheare used, both first-party.Linux native deps installed in the workflow.
libgtk-3-devandlibwebkit2gtk-4.1-dev— Cargo can't declare these, and without them the build fails insidegdk-syswith an error that never names the fix. Confirmed correct on a real runner. macOS ships WebKit and Windows runners ship WebView2, so neither needs a step.Build before test, so the test step never waits on compilation.
No display setup needed. No surviving test creates a window — verified by running the full suite with no
DISPLAYand no X server. The only window-creating tests were inbrowser/native_ui.rs, removed in #4 as dead code; had it ever been wired up it would have failed on every runner without a display.timeout-minutes: 30. Added after the hang: a stuck job would otherwise hold a runner until GitHub's six-hour ceiling.No fmt or clippy gate, deliberately. The tree isn't rustfmt-clean (~688 diffs) and the build emits 32 lib + 91 bin warnings. Gating either today would make the workflow red on arrival, which teaches everyone to ignore it. Both are recorded in the roadmap as explicit follow-ups: format once in a single mechanical commit, then gate.
Also flagged in the roadmap
Cargo.lockis gitignored. For a library that's conventional; for an application it means CI builds aren't reproducible and can break when a transitive dependency publishes. It also costs cache precision — the key falls back to hashingCargo.toml. Left as-is since it may be deliberate; recorded in M2 as a decision to make.