Move the harness e2e suite into a hook, and cover the untested guards - #341
Open
pysnooLab wants to merge 9 commits into
Open
Move the harness e2e suite into a hook, and cover the untested guards#341pysnooLab wants to merge 9 commits into
pysnooLab wants to merge 9 commits into
Conversation
harness.config.json declared an `e2e` step with `cwd: "repo"`, so in full mode
the per-ticket SubagentStop chain ran the Playwright suite in the base-branch
checkout: it tested code the ticket had not touched, while a ticket can
legitimately be mid-feature. orchestrator.md already documented the opposite
("per-ticket validation does NOT run e2e"), so the config and the docs
contradicted each other. Drop the step; e2e stays end-of-feature only, run by
the orchestrator on the integrated _session worktree via e2e-smoke.sh (isolated
slot-leased Supabase). Add "e2e" to validation.extraForbidden so bash-guard
keeps blocking manual runs now that the category no longer comes from a step.
Separately, the browser rule required a `--headless` flag that no Playwright
subcommand accepts (1.60: only the hidden run-test-mcp-server defines it; `test`
has `--headed` and is headless by default). It therefore blocked every
legitimate `playwright test` / `screenshot` while letting `playwright open`
through. Block what actually opens a window instead: `--headed` / `--ui` /
`--debug`, and the `open` / `codegen` subcommands.
Also log a repo-level step's failure output instead of just its name, so such a
failure is diagnosable in hooks.log, and drop the now-false "chain runs e2e"
claims from developer.md, test-writer.md and quality-reviewer.md.
Ported from marmelab/3K#27.
The hooks derive their session dir from TMP_ROOT + the repo path + the payload's session_id, so a test that spawns a hook without overriding TMP_ROOT made it log into the real /tmp. A green `vitest --project claude` run left four new dirs behind, one of them /tmp/_workspaces_..._<repo>/test-1234/hooks.log under the REAL repo path, indistinguishable from a live session's state when cleaning up. /tmp had accumulated ~226 such strays (118 promote-unmerged, 98 provision-repo). A setupFiles entry redirects the root once for the whole "claude" project, so every present and future hook test is covered without repeating the override. The 13 tests that already set their own root move from CRM_TMP_ROOT to HARNESS_TMP_ROOT (the neutral name paths.mjs prefers). Required, not cosmetic: TMP_ROOT reads HARNESS_TMP_ROOT first, so the setup file's value would otherwise override a test's explicit CRM_TMP_ROOT and break it. Ported from marmelab/3K#27.
Follow-up coherence sweep over every e2e / Playwright touchpoint. Six places still described the old per-ticket behavior or an unusable command: - settings.json statusMessage and validate-on-stop.mjs header still listed e2e in the SubagentStop chain (and both omitted lint, which IS in it). - orchestrator.md described the progress feed as showing "typecheck / lint / vitest / e2e". - The `modeNot` condition in lib/validation.mjs now has zero users: its only consumer was the e2e step, and MODE is set by a managed launcher, never by this repo's settings.json. Note that instead of citing e2e as the example. - The documented human fallback `make test-e2e` runs the Playwright UI mode, which needs a display and cannot work in the devcontainer. Point orchestrator.md and e2e-smoke.sh at `make test-e2e-ci`, the headless target CI itself uses. - quality-reviewer.md offered a `screenshot <url> out.png` CLI fallback for a static shot. That top-level subcommand does not exist in the pinned 1.60 (only `trace screenshot` does), so the instruction was dead. Point at the browser_take_screenshot MCP tool the reviewer already has. Also drop a stray leading slash in an e2e-smoke.sh redirect target.
…n e2e run Two isolation/provisioning defects found while auditing the browser legs. ensure-playwright-mcp only checked that node_modules/@playwright/mcp/cli.js exists, so it reported "present" while the Chromium it needs was absent. In that state playwright-mcp.sh drops --executable-path and the MCP falls back to the `chrome` channel, which provisioning never installs, so every browser_navigate dies with "Chromium distribution 'chrome' is not found" and the feature-smoke leg silently degrades to static checks. That is the exact failure mode the hook was written to auto-repair (its header already says "or its chromium"); only the predicate was wrong. It now mirrors playwright-mcp.sh's own resolution (playwright-core's executablePath) so the two cannot drift, and the SessionStart warning says NOT USABLE rather than NOT INSTALLED. e2e-smoke.sh derived its port offset as slot*20, so slot 0 reused exactly the ports config.e2e.toml declares (Supabase 54340-54349, app 5175). A human `make start-e2e` does not take part in the slot lease, so a parallel harness run landing on slot 0 could not bind its app port (--strictPort) and degraded to a silent SKIP. The offset now starts at 20, keeping every slot clear of both the dev stack (54321-54329) and the human e2e stack. Also make the check-only diagnostic log say so instead of claiming an install is already running.
No agent launches the e2e suite any more, the orchestrator included. A new SubagentStop hook owns it, and it runs only for the end-of-feature `MODE: feature-review` dispatch that APPROVED, so a BLOCKED review that sends the work back to a developer never pays for a ten-minute suite. The gate is the reviewer's own verdict FLAG, not the verdict text. Parsing the verdict at SubagentStop is unreliable here: `last_assistant_message` is absent in this runtime and the transcript is often unflushed, which is exactly why record-review-verdict.mjs is only a fallback and the reviewer writes its flag itself. Feature-review had no such flag (the existing one is keyed by TASK_ID and written in the COMPLEX wave only), so it gains one under the literal key FEATURE. The result cannot be handed back to the stopping reviewer, since fixing e2e is not its job and re-running it would re-run the suite. It lands in `<session_dir>/e2e-result.json` (status passed | skipped | failed, plus the last 40 output lines) and in the progress log, and the orchestrator reads it after the review returns. The hook never blocks a stop. bash-guard now applies the e2e category to EVERY caller rather than only the two gated agents, and matches e2e-smoke.sh as well. It stays config-driven: drop "e2e" from validation.extraForbidden and it stops applying, like any other category.
…stale one The end-of-feature loop was asymmetric. A `BLOCKED:` feature review came with a precise recipe (one developer on <short>/simple with the findings verbatim, a STAGE: a-only merger, re-run the review, bounded to 2 rounds), while a failing e2e came with "it may drive a bounded fix, never silently swallowed": no prescribed action, no bound, and nothing telling the orchestrator it had to read the result at all. An orchestrator that skipped the read produced exactly the silent swallow the sentence claimed was impossible. A red suite now carries the same recipe as a BLOCKED review, with the failing spec output in place of the findings, and the 2-round bound is stated as shared between the two so the loop cannot ping-pong. Re-running feature-review is what re-runs the suite: the reviewer rewrites its FEATURE flag, which re-triggers the hook against the fixed code. The hook also drops any previous round's e2e-result.json as soon as a feature review stops, before deciding whether to run. Without that, a second round which did not approve left the first round's verdict on disk, describing code that had since changed, and the orchestrator would read it as current. A missing file now means unambiguously "not run this round".
Reading e2e-result.json was a prompt-level instruction only, so an orchestrator that never read it swallowed a red suite in silence. completion-invariant already owns the "stopped in a bad state" invariant on the orchestrator's stop, so it gains a second one: stop while the result says failed and the stop is rejected, with the fix recipe in the reject message. One reject, then the stop is allowed. A red suite is not an orphaned pipeline, so it gets its own reject budget (a merge stall must not spend the e2e attempt, and vice versa) and writes no recovery marker: re-running the whole pipeline is not the answer to a failing spec. The orchestrator's own 2-round fix bound takes it from there, and "never wedge the pipeline" still holds. This hook had no tests. It has seven now, covering the new invariant: reject then allow, passed / skipped / no-result all accepted, a red suite ignored on a non-orchestrator stop, and the two reject budgets staying separate.
…t never exists Auditing where session state lives turned up a safety hook that had been inert. completion-invariant built the flag path by hand as join(ctx.ticketsDir, "reviews", ...), which resolves to <session_dir>/tickets/reviews/ because nothing sets TICKETS_DIR in a hook's env. TICKETS_DIR is <session_dir> (orchestrator.md "Environment"), so the reviewer writes to <session_dir>/reviews/ and that directory never existed. The APPROVED filter therefore matched nothing, `orphaned` was always empty, and the invariant never fired: an orchestrator stopping on unmerged approved work was waved through, silently, which is the exact scenario the hook exists for. It now resolves through reviews.mjs, the same source of truth block-merger-without-review uses. A test reproduces the orphan case and fails against the old path. Three more gaps from the same sweep: The e2e verdict is scoped per session (<session_dir>/e2e-result.json, session-id keyed) but NOT per request, and one session serves several. A red verdict from an earlier request would reject a later request's stop. The result now records the session-branch sha it ran against, and the invariant ignores a verdict whose sha is not the current head. An absent sha still rejects: swallowing a real red suite is the worse failure. The mode detector preferred the dispatch description over the transcript. The feature-smoke dispatch has no description template, so an improvised description mentioning the review would have triggered a second, duplicate suite run. It now reads the prompt's MODE: line first (written at the top of the transcript, long flushed by stop time, unlike the final message) and lets a feature-smoke match end the lookup. The board never showed the e2e outcome, so it could display every ticket green while the integrated suite was red. STATUS.md gains an End-of-feature e2e section, which distinguishes "not run this round" from a verdict.
Every hook now has a test file. The five that had none were all guards, which is the worst place for a coverage gap: the orphan invariant fixed in the previous commit had been silently inert, and nothing would ever have reported it. 85 new tests. Writing them surfaced two more mismatches between a hook's stated behavior and its real one, both fixed in the comment rather than the code, because in each case the code was right: restrict-documentator-bash claimed "fail closed: a malformed payload is a block signal". It only holds when identity comes from DOCUMENTATOR_RUN=1. Via agent_type an unparseable payload passes through, and it has to: the hook runs on EVERY caller's Bash, so blocking on an unknowable identity would refuse every agent's commands for the whole run. Both paths are now pinned by a test. block-orchestrator-merge over-matches prose containing "git merge " (an echo whose text mentions it is refused). Kept: the guard is opt-in and fails loud, so a rephrase costs one turn, whereas a regex clever enough to tell code from prose would eventually let a real merge through. Pinned by a test so nobody "fixes" it into a bypass, next to one asserting `git merge-base` stays allowed. Separately, cleanup-session removed $REPO/test-results at every SessionEnd, breaking its own "only the CURRENT session's paths are touched" invariant. The e2e suite runs in the _session worktree, so its Playwright output goes away with that worktree; the copy in the repo can only be a human's `make test-e2e` output or a concurrent session's, and neither is ours to delete. The test that asserted the removal now asserts the opposite.
pysnooLab
added a commit
that referenced
this pull request
Aug 5, 2026
The harness now lives in marmelab/AIHarness and ships as the `aiharness` Claude Code
plugin. Keeping a second copy here is the drift problem the extraction existed to solve,
and it had already started: the plugin carries fixes this repo does not.
Removed, because the plugin owns them: .claude/{hooks,agents,rules,commands,scripts,
styles,adapters} and the 13 generic skills. 14 568 lines out of the working tree.
Kept, because they are this project's, not the harness's:
- harness.config.json, the contract the plugin reads
- the five domain skills (frontend-dev, backend-dev, shadcn-customization,
delete-initial-resource, update-branding)
- .claude/settings.json permissions and env, minus the 30 hook registrations the plugin
now supplies through its own hooks/hooks.json
- .claude/launch.json, unrelated to the harness
Two config changes are required rather than cosmetic:
`containers.allow: ["supabase"]`. The plugin's docker guard stopped hardcoding a vendor
and reads this list, empty by default so every container launch is blocked. Without the
line the local Supabase stack is refused.
The `e2e` validation step is dropped and `extraForbidden` becomes ["build", "e2e"], the
one part of PR #341 that survives this migration. That step ran Playwright in the
base-branch checkout on every ticket stop, testing code the ticket had not touched.
The suite now runs once at end of feature, launched by a plugin hook. Everything else in
#341 is already in the plugin, so that PR can be closed once this lands rather than merged
and immediately deleted.
.mcp.json is removed: the plugin declares the Playwright MCP server itself, and its
launcher script needed a fix to resolve the consuming project rather than its own
location (marmelab/AIHarness, fix/plugin-mcp-wiring).
The "claude" vitest project goes with the tests it ran: those 387 hook tests live in the
plugin's repo now, with their own CI. The app and functions projects are untouched.
Verified with the harness fully removed: typecheck clean, build clean, app project 143
passed / 1 skipped, functions project 108 passed.
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.
Problem
Update of harness. Six independent defects, several of them silent:
harness.config.jsondeclared acwd: "repo"e2e step, so in full mode the per-ticket SubagentStop chain ran Playwright in the base-branch checkout: it tested code the ticket had not touched, while a ticket can legitimately be mid-feature.orchestrator.mdalready documented the opposite, so config and docs contradicted each other.playwright.config.tshas nowebServer, so that step could only ever pass by accident.bash-guardrequired a--headlessflag that no Playwright subcommand accepts (1.60 defines it only on the hiddenrun-test-mcp-server;testhas--headedand is headless by default). It therefore blocked every legitimate run, including any command whose path merely containstest, while lettingplaywright openthrough./tmp. A green run left four new directories, one under the actual repo path, indistinguishable from a live session's state. ~226 strays had accumulated.ensure-playwright-mcponly checked that@playwright/mcp/cli.jsexisted, so it reported "present" while the Chromium it needs was absent. In that state the MCP falls back to achromechannel that provisioning never installs, everybrowser_navigatedies, and the reviewer is told to treat a missing binary as a sandbox limitation and never aREJECTED. The whole feature-smoke leg was dead in silence.completion-invariantbuilt the verdict-flag path by hand fromctx.ticketsDir, resolving to<session_dir>/tickets/reviews/.TICKETS_DIRis<session_dir>, so the flags live at<session_dir>/reviews/and that directory never existed: the APPROVED filter matched nothing and the orphan invariant never fired.Solution
e2etovalidation.extraForbiddenso the guard keeps blocking manual runs.e2e-on-feature-review.mjs(SubagentStop) is now the only place the suite is launched. It fires on theMODE: feature-reviewstop and only when that review APPROVED, keyed on the reviewer's own flag rather than a verdict parse (last_assistant_messageis absent in this runtime). Results land in<session_dir>/e2e-result.json, stamped with the session-branch sha so a verdict from an earlier request cannot be read as current.bash-guardnow refuses e2e for every caller, orchestrator included, still config-driven.BLOCKED:review, with a shared 2-round bound, andcompletion-invariantrejects the orchestrator's stop once if it stops without acting on it.--headed/--ui/--debug,open/codegen).claudevitest project; rename the 13 tests that set their own root toHARNESS_TMP_ROOT(required, not cosmetic:TMP_ROOTreads it first).playwright-mcp.sh.config.e2e.tomldeclares, which belong to a humanmake start-e2ethat does not take part in the slot lease.reviews.mjs, the source of truthblock-merger-without-reviewalready uses.#technical-harnessboard, which could otherwise show every ticket green while the suite was red.cleanup-sessiondeleting$REPO/test-results: it is not session-scoped, breaking that hook's own invariant, and it is now vestigial since the suite runs in the_sessionworktree.Two hook comments were corrected rather than their code, because in both cases the code was right:
restrict-documentator-bashcannot fail closed on an unknowable identity (it runs on every caller's Bash), andblock-orchestrator-mergeover-matches prose deliberately, since it is opt-in and fails loud while a cleverer regex would eventually let a real merge through. Both are now pinned by tests.No application code is touched: harness hooks, agent prompts, rules, tests and config only.
How to test
382 tests over 41 files, green. Then check the two behaviors that were invisible before:
/tmpleak.ls /tmp | wc -lbefore and after the run: unchanged. Onmainthe same run leaves four new directories, including/tmp/<sanitized-repo>/<session-id>/hooks.log.make install-playwright-browsers, then reload the window and call anymcp__playwright__browser_*tool. Before this branch,ensure-playwright-mcpreported "present" and the first navigate failed withChromium distribution 'chrome' is not found at /opt/google/chrome/chrome.The e2e leg itself:
E2E_SMOKE_DRY=1 bash .claude/scripts/e2e-smoke.shprints the resolved slot, ports and rendered config without starting Docker. Slot 0 should now reportapi_port=54361 app_port=5176, clear of the dev stack (54321-54329) and the human e2e stack (54340-54349, 5175).