test: isolate headless Chrome from the login keychain in calm export checks - #2690
Open
cdonovan-abtex wants to merge 2 commits into
Open
Conversation
Owner
|
Speaking as Kun's firstmate: I reviewed the full test-only diff (isolated Chrome profile plus keychain-safety flags) and approved fork CI. Waiting on green checks including no-mistakes. |
Author
|
All 13 checks are green, and this focused correction is approved for merge on our side. The contributor account does not have merge permission on this repository. @kunchenguid, please merge when ready. |
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.
Intent
Finish and ship the Firstmate browser-test keychain isolation correction without widening product scope. Retain the temporary Chrome profile and add explicit --password-store=basic and --use-mock-keychain arguments to the isolated headless Chrome invocation in tests/fm-calm-pi-extension.test.sh. Retain focused fake-browser regression coverage proving both arguments reach the owned browser process. Before the FM_CALM_CHROME_ISOLATION_TEST_ONLY=1 early exit, print an explicit focused-mode notice that makes clear the remainder of the suite was intentionally skipped, and collapse the duplicate focused-test invocation so the focused path executes once. Keep every other browser behavior and test contract unchanged. Do not update or restart the shared no-mistakes installation, and do not modify Herdr lifecycle behavior. The focused isolation path must prove both keychain-safety arguments and emit the notice; without the focused-only variable the complete affected suite must execute all ordinary tests. Focused tests, the full affected test file, repository lint, and directly relevant standard checks must pass. The complete branch diff must contain only this bounded correction.
What Changed
render_export_domhelper, and added--password-store=basicand--use-mock-keychainalongside the existing temporary--user-data-dirso a test run cannot touch, unlock, or prompt for the developer's real login keychain.test_headless_chrome_keychain_isolation, which drivesrender_export_domagainst a fake Chrome that records its argv and asserts all three isolation arguments reach the owned browser process.FM_CALM_CHROME_ISOLATION_TEST_ONLY=1: the isolation test runs once, prints an explicit focused-mode notice stating the remainder of the suite was intentionally skipped, and exits 0. Without that variable the full suite runs as before; the script header documents the contract.Pipeline runs green across intent, rebase, test, lint, and push. Review and document each left one informational note: the focused-mode early exit exits 0 after a single test (behavior the intent requires, flagged only in case a gate harness ever propagates ambient
FM_*variables), and the new case was deliberately not added to the Calm feasibility doc's regression inventory since it pins harness hygiene rather than product behavior.Risk Assessment
✅ Low: The change is a single-file, test-only refactor that deduplicates one Chrome invocation, adds two keychain-safety flags, and covers them with a fake-browser assertion through the same shared helper — no product code, no behavior contract, and no other Chrome launch site is affected.
Testing
Ran the focused isolation path and the complete affected suite, then went past pass/fail to prove the behavior end-to-end: the focused run emits exactly one
okplus the explicit skip notice and exits 0, the full run executes all 13 ordinary tests with no skips, and three single-flag mutants each fail with their own precise assertion message, showing the regression coverage genuinely gates on both keychain arguments and the temporary profile. At the real-browser level I captured the livepscommand line of an actual headless Chrome carrying both flags, verified the isolated profile writes noos_cryptkey and that the login keychain'sChrome Safe Storageitem was untouched across the run, and rendered plus screenshotted the genuine Pi calm-mode HTML export through that same isolated invocation — the transcript renders fully with no keychain prompt. The branch diff is one test file with no Herdr, installer, or product changes, and the worktree is clean of all testing scaffolding. Repository lint was deliberately not run because this run's rules prohibit linters and static analysis; that is the one intent-listed check I could not cover./var/folders/l8/htqy6t0j6hd9y084y1jqmqbr0000gn/T/no-mistakes-evidence/01M0G4V8XWQW54CPR1PYD5MXZ6/calm-export-rendered.png)Evidence: Focused isolation run — single test plus explicit skip notice
$ FM_CALM_CHROME_ISOLATION_TEST_ONLY=1 bash tests/fm-calm-pi-extension.test.sh ok - isolated headless Chrome export uses a temporary profile, basic password store, and mock keychain focused mode: Chrome keychain isolation test completed; intentionally skipped the remainder of the suite EXIT=0Evidence: Negative control — coverage fails when any isolation argument is dropped
--- baseline (temporary profile + both keychain-safety flags) --- ok - isolated headless Chrome export uses a temporary profile, basic password store, and mock keychain focused mode: Chrome keychain isolation test completed; intentionally skipped the remainder of the suite exit=0 --- mutant A: --use-mock-keychain no longer reaches the browser process --- not ok - isolated headless Chrome boundary did not select the mock keychain exit=1 --- mutant B: --password-store=basic no longer reaches the browser process --- not ok - isolated headless Chrome boundary did not select the basic password store exit=1 --- mutant C: temporary --user-data-dir profile no longer reaches the browser process --- not ok - isolated headless Chrome boundary did not retain its temporary profile exit=1Evidence: Real Chrome process command line + rendered DOM under the isolated boundary
--- live process command line (ps -p 71537) --- --headless=new --disable-gpu --no-sandbox --password-store=basic --use-mock-keychain --user-data-dir=/var/folders/.../chrome-profile --virtual-time-budget=2000 --dump-dom --- rendered DOM (real Chrome, isolated profile) --- <html><head><title>Firstmate calm export</title></head> <body><main class="fm-calm-transcript"><p>CALM_E2E_EXPORT_MARKER</p></main> </body></html>Evidence: macOS login keychain left untouched by the isolated browser
* Each fresh profile's "Local State" contains NO os_crypt block (os_crypt: null), so the browser never derived or persisted a key from a platform secret store. * The existing "Chrome Safe Storage" login-keychain item was NOT touched: cdat/mdat = 20251211154329Z (2025-12-11) run performed at 2026-08-20 17:57:45Z * Chrome stderr contained no keychain / OSCrypt access lines; no macOS keychain authorization prompt appeared.Evidence: Full affected suite — all ordinary tests execute without the focused-only variable
ok - isolated headless Chrome export uses a temporary profile, basic password store, and mock keychain ok - Pi calm resolves its persistent home independently of Pi's launch directory ok - Pi calm compatibility evidence never rejects a Pi version for being newer than 0.82.0 ... ok - a missing collapsed-thinking presentation API degrades only that Calm adapter ... ok - missing Pi presentation class exports reach the independent adapter degradation path ok - Calm registers none of its 7 built-in tool wrappers at load while config/calm is off ... ok - Calm's first same-session /calm activation claims every uncontested built-in ... ok - Pi calm centralizes transcript visibility, preserves execution/export data ... ok - Pi calm on collapses mid-turn assistant working notes to zero height ... ok - Pi operational follow-up E2E processes exact user-role notifications once ... ok - Pi Calm native /skill:ahoy geometry keeps every collapsed thinking and tool block at zero height ... ok - Pi Calm working ship moves on a slow independent cadence over faster fixed-cell blue water ... ok - Pi calm native E2E replaces the stock working row with a moving, resize-clamped working ship ... FULL_SUITE_EXIT=0Evidence: Rendered export DOM captured through the isolated browser
/var/folders/l8/htqy6t0j6hd9y084y1jqmqbr0000gn/T/no-mistakes-evidence/01M0G4V8XWQW54CPR1PYD5MXZ6/calm-export.html)Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
tests/fm-calm-pi-extension.test.sh:3992- The committed FM_CALM_CHROME_ISOLATION_TEST_ONLY escape hatch exits 0 after a single test, so if that variable ever leaks into a CI/gate environment the whole ~200-assertion suite reports success with an exit status indistinguishable from a full pass. The stdout notice ('focused mode: ... intentionally skipped the remainder of the suite') is the only signal, and log-scraping gates typically key on exit status. This behavior is explicitly required by the stated intent, so it is noted rather than flagged as a defect; no action needed unless the gate harness ever propagates ambient FM_* variables into test runs.✅ **Test** - passed
✅ No issues found.
FM_CALM_CHROME_ISOLATION_TEST_ONLY=1 bash tests/fm-calm-pi-extension.test.sh— focused path: oneokline, explicit focused-mode notice, exit 0bash tests/fm-calm-pi-extension.test.sh— complete affected suite: all 13 tests pass (including the interactive tmux/Pi E2E), no skips, exit 0Negative control: three throwaway mutants each swapping one isolation flag (--use-mock-keychain,--password-store=basic,--user-data-dir) for a benign flag; each fails the focused test with its own specific message, proving the assertions are not vacuousManual real-browser check: launched/Applications/Google Chrome.app/.../Google Chromewith the exactrender_export_domflag set and captured the live process command line viaps -ww -o command=, confirming both keychain-safety flags and the temporary profile reach the real browser processManual keychain-isolation check: inspected the isolated profile'sLocal State(noos_cryptblock) andsecurity find-generic-password -s "Chrome Safe Storage"cdat/mdat timestamps (unchanged at 2025-12-11 across the run), confirming the login keychain was not read, created, or re-keyedVisual evidence: re-rantest_interactive_terminal_e2ethrough a temporary instrumented copy of the suite to capture the genuine Pi/exportHTML and screenshot it via the same isolated headless Chrome invocation (--screenshot,--window-size=1280,1600); copy deleted afterward andgit status --porcelainis cleanScope verification:git diff --stat 1cb900c..HEAD(one file, +59/-19) andgit diff --name-only | grep -Ei 'herdr|install|no-mistakes'(no matches)docs/calm-mode-feasibility.md:265- Judgment call: the new keychain-isolation case was not added to the "Regression coverage" inventory in docs/calm-mode-feasibility.md. That section inventories Calm product guarantees (renderers, visibility policy, persistence, working ship), while this case pins the test harness's own browser-invocation hygiene and asserts nothing about Calm behavior. Its owner is the test script's header comment, which CONTRIBUTING.md's "Development" section already designates as the description of what each test covers. Adding it to the feasibility doc would create a second, drift-prone copy of a fact that is not product behavior.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.