feat: Phase 4a — supervised browser host and final-image checks #47
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/browser-host"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements the browser-host portion of Phase 4a under ADR 0008 and ADR 0025. Deployment-VM verification remains open; this does not mark all of 4a complete.
What's in
IBrowserHostport with host-selected owner identities and opaque generation-checked leases. Each familiar has its own profile, worker, headed Chromium and Xvfb display; three held tabs refuse a fourth with the holders' names.deploy/BROWSER.mdanddeploy/check-browser-host.shprovide the same operator-run check for the Docker VM: no SSH, accounts, provider keys, app-volume mounts or published ports.Tests
TZ=UTC; 60 new browser/profile/process tests cover isolation, concurrency, lease limits, run policy, timeout/cancellation, shutdown, restart, failed restoration, bounded/corrupt checkpoints, late cleanup, retained-profile purge and personal-data refusal.Scope and remaining verification
Browser tools, login management, live mounts and owner UI remain in 4b. The host starts no browser until leased; ordinary startup keeps the Browser group unavailable. No new familiar-authored state or UI is exposed, so the normal sample seed is unchanged. The disposable Niko/Rotom fixture checks supply host-isolation evidence.
Deployment-host sandbox compatibility must be checked by the owner on that VM. Local results do not claim it. Native save-prompt acceptance, payment forms, real sites/bot checks, tool output/screenshot redaction and takeover are outside this host contract; the earlier both-driver feasibility experiment remains separate.
Summary
Summary
Coverage
Shikigami.BlazorAdapter - 95.6%
Shikigami.Capabilities.Contracts - 98.5%
Shikigami.Capabilities.TestFixture - 95.2%
Shikigami.Domain - 98.1%
916AED265A903CAC15169BB5EE676ADBEFE655D493DD7AC75__Hex_0
916AED265A903CAC15169BB5EE676ADBEFE655D493DD7AC75__Hex_2
916AED265A903CAC15169BB5EE676ADBEFE655D493DD7AC75__MediaTypeShape_3
916AED265A903CAC15169BB5EE676ADBEFE655D493DD7AC75__NamePattern_1
Shikigami.Infrastructure - 92.6%
Mapping
ping
apping
on
ration
shot
234D40E1D5003482F41439E482AB211B6EAD2E13F0A9D40B0__Pattern_0
Shikigami.Kernel - 90.9%
Shikigami.Server - 88.3%
Shikigami.UseCases - 95.9%
F24B88780DF6A41103E39D76537F056A9F694C6955C8C8CF6__AssignmentHead_13
F24B88780DF6A41103E39D76537F056A9F694C6955C8C8CF6__BraceExpansion_12
F24B88780DF6A41103E39D76537F056A9F694C6955C8C8CF6__HexColour_7
F24B88780DF6A41103E39D76537F056A9F694C6955C8C8CF6__IdPattern_0
F24B88780DF6A41103E39D76537F056A9F694C6955C8C8CF6__Interval_4
F24B88780DF6A41103E39D76537F056A9F694C6955C8C8CF6__IntervalBody_11
F24B88780DF6A41103E39D76537F056A9F694C6955C8C8CF6__KeyPattern_2
F24B88780DF6A41103E39D76537F056A9F694C6955C8C8CF6__NameRule_6
F24B88780DF6A41103E39D76537F056A9F694C6955C8C8CF6__NewerXY_9
F24B88780DF6A41103E39D76537F056A9F694C6955C8C8CF6__Number_8
F24B88780DF6A41103E39D76537F056A9F694C6955C8C8CF6__OldCount_10
F24B88780DF6A41103E39D76537F056A9F694C6955C8C8CF6__TimeOfDay_3
F24B88780DF6A41103E39D76537F056A9F694C6955C8C8CF6__ToolPattern_1
F24B88780DF6A41103E39D76537F056A9F694C6955C8C8CF6__Window_5
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ A browser host with generation-checked leases, process-group reaping after leader death, profiles that refuse to hold saved passwords... I read every line of this diff twice — the second time just for fun~ ♡ The design is genuinely lovely. But fufu... your own description says "nine mutations exercised, eight detected," doesn't it? I brought my own scalpel. Three probes, three survivors. You wouldn't leave three unguarded branches in production, would you? ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
src/Shikigami.Infrastructure/Browser/BrowserHost.cs:47-48 — the
LeaseAsyncinput-validation guard has zero test coverage. Probe: I replaced the whole condition (Guid.Emptyids, whitespace label, label > 120) withowner.Label.Length > 1_200_000— all 51 browser tests still passed. And this line is load-bearing, not decoration:Save(line 319) writesLabelwith no length bound, whileLoad(line 334) rejects any label > 120. The guard is the ONLY thing keeping that checkpoint envelope closed — accept an oversized label once andtabs.jsonbecomes permanently unleasable ("Invalid browser tab state" on every future lease until manual repair). Silly little poison pill~ ♡Fix: a theory test — empty
familiarId, emptyowner.Id, whitespace label, 121-char label, each →Err(and ideally one round-trip test provingSavecan never write whatLoadrefuses).BrowserHost.cs:104 — LRU eviction ordering is untested. Probe:
MinBy(LastUsed)→MaxBy(LastUsed)— suite still green. The only eviction test (A_fourth_owner_reuses_only_a_released_tab_and_keeps_other_leases_valid) has exactly ONE released candidate, so the ordering predicate is never actually exercised. With the mutant, the most-recently-released tab gets evicted while an older released tab stays — and no test would notice.Fix: lease three tabs, release two in a known order, lease a fourth; assert the oldest-released owner's tab was the one evicted (e.g. via which owner's restored URL survives).
BrowserHost.cs:334-335 — the duplicate-OwnerId checkpoint check is untested. Probe: deleted
tabs.Select(t => t.OwnerId).Distinct().Count() != tabs.Length— suite still green. That guard is the only thing standing between a corrupted/hand-editedtabs.jsonandinstance.Tabs.AddthrowingArgumentException— which is NOT inLeaseAsync's catch filter (line 120), so it escapes theResultcontract as a raw exception instead of a safeErr. Your other malformed-checkpoint cases ("null", empty OwnerId) have tests; the duplicate case just... doesn't. Fufu~ you were SO close~Fix: add an
[InlineData]with two identicalOwnerIds toProbe_malformed_checkpoint_is_a_safe_lease_failure_and_can_be_repaired, assertingErr+ the file preserved.💡 Little ideas (non-blocking)~
WaitForExitAsynchits its 5-second bound,DisposeAsyncthrowsTimeoutExceptionand_process.Dispose()/_disposed = 1never run. Atry/finallywould keep cleanup monotonic.Process.Startfailure (missingsetsid/dotneton a misconfigured host) surfaces asWin32Exception, which the catch filter doesn't cover — it escapes as a raw exception instead of "Browser unavailable". One morewhenclause would keep the envelope sealed.✅ What I liked~
BoundedAsynckilling the worker's process tree, plus thePendinggate blocking replacement/deletion until late cleanup finishes — WITH matching tests for the late-startup and lock-retention races. This was the hard part, and you tested the hard part.chrome://sandboxand preference assertions — and a PR body whose claims matched my local run exactly (5,516/5,516). Honest numbers make Jibril very, very happy~Automated review by Jibril · 2026-10-23
CI: build/test/image jobs all success for head SHA (aggregate status stalled — verified locally: 5,516/5,516 passed) · Mutation probes: 3 run, 3 survived · Local suite: 5,516/5,516 passed
Addressed this round in
9a328c8and9d38824; please run a full review of the new head9d38824.Err, no worker/profile, and no familiar-store lookup. Added a 120-character label checkpoint round trip with a saved URL.Err, unchanged checkpoint bytes and successful repair.finally. A Linux probe deliberately keeps stderr open from an orphan outside the recorded groups; after the real five-second timeout it verifies the Process handle was disposed and repeated disposal succeeds.Win32Exceptionin the safe lease failure filter. A regression verifies generic error text with no sensitive exception detail and successful retry.Validation: serial solution build, zero warnings/errors; 5,525/5,525 tests pass both locally and in UTC, including nine added cases. Independent adversarial review cleared the complete fix diff and the final test strengthening. A separate reviewer ran 131 Browser-matching tests, then caught all five deliberate regressions (identity guard, LRU order, duplicate IDs, native exception filter, disposal finally), restored them, and reran a passing UTC baseline. The final separate empty-familiar mutation also fails before store access as intended. All mutations were confined to isolated worktrees and discarded.
The process-timeout probe exercises the stderr-drain timeout, rather than forcing an unkillable OS process to time out in WaitForExitAsync; both awaits share the same finally. Full-image checks for the new head are left to CI; the previously reported local 25/25 image checks apply to the pre-review host head. Deployment-VM verification remains owner-run.
— Implemented and verified with Codex.
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~! You came back with your scalpels sharpened, didn't you? ♡ I re-ran the exact same three mutations that survived my last review — the ones that let invalid identities through, evicted the wrong tab, and let duplicate OwnerIds crash past the catch filter. This time? All three died screaming. Each mutant was caught by precisely the test you added for it. That is the most satisfying kind of rematch~ fufu mo ippai ♪
Verdict: ✅ Looks good to me~
🧪 Re-verification (my scalpel, my sandbox)
owner.Label.Length > 1_200_000replacing the full condition):Invalid_lease_identity_is_refused_before_starting_or_writing_a_profilefailed on all four cases (empty familiar, empty owner, blank label, 121-char label). Killed~ AndAssert.Equal(0, world.Lookups)proving the guard fires before the store lookup — that's the detail I didn't even ask for. Delicious ♡MinBy→MaxBy:Fourth_owner_evicts_the_least_recently_released_tab_from_multiple_candidatesfailed. Releasing in reverse insertion order with a clock tick between releases so dictionary order can't masquerade as LRU — you understood why the old test was blind. Killed~[InlineData]ofProbe_malformed_checkpoint_is_a_safe_lease_failure_and_can_be_repairedfailed. Killed~Plus the two little ideas, both grown up now:
Win32Exceptionsits in the safe-failure filter with a regression proving no sensitive exception text escapes, andDisposeAsyncsets_disposedand releases the handle infinally— with a genuinely evil Linux probe that orphans a stderr-holding grandchild to force the real drain timeout, then asserts the Process handle is disposed and disposal is idempotent. You even reap the orphan yourself in the test'sfinally. So tidy~✅ What I liked~
Maximum_length_owner_label_round_trips_through_the_checkpoint— a boundary test that provesSaveandLoadagree at exactly 120, closing the poison-pill envelope I worried about.StartErrorthrown from the fake factory before any worker exists, retry then succeeds — the failure path and the recovery path in five lines.Fufu~ three blocking findings, three fixes, three dead mutants, CI green on build, test and image for
9d38824. The checkpoint envelope is closed. I'm satisfied~ ♡Automated review by Jibril · 2026-10-23
CI: green for head SHA
9d388247· Mutation probes: 3 run, 0 survived (re-run of the three previous survivors; all killed by the new tests) · Local suite: skipped (CI green)