Skip to content

Litmus: agentic PR testing inside AO (phase 2) - #6568

Draft
Rishet11 wants to merge 32 commits into
mainfrom
feat/litmus
Draft

Rishet11 wants to merge 32 commits into
mainfrom
feat/litmus

Conversation

@Rishet11

@Rishet11 Rishet11 commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator

Lines

Part Files + -
Code 43 12,951 18
Tests 40 12,936 0
Others 19 11,603 6,905
Total 102 37,490 6,923

Others: generated API files +10,734 / -6,905, test fixtures 741, skills 48, recipe, window script and icon 80.

What it does

Litmus tests a PR by running it. It builds the PR and the main commit the PR branched from, opens both as AO windows, clicks through them, and records screenshots and video. A bug that shows on the PR but not on main is the PR's bug. Each bug gets its own PR comment with a picture and a clip.

flowchart LR
  A["'test PR #N'<br/>chat or CLI"] --> B[Fetch PR,<br/>pin commits]
  B --> C[Build PR]
  B --> D[Build main]
  C --> E[Tester clicks<br/>through both apps]
  D --> E
  E --> F[Screenshots and<br/>video per check]
  F --> G[Verdict]
  G --> H{Post?}
  H -->|yes| I[One comment<br/>per bug]
  H -->|no| J[Discard]
Loading

How to use

  • Tell any AO agent "test PR #N". It starts the run, shows the result, and asks before posting.
  • Terminal: ao testing start --project <id> --pr <url>. Add --no-comment --no-evidence to post nothing.
  • Per project: ao testing settings --harness codex|claude-code --model <id> --effort <level>.

Guards

  • A tester session can't post, prune, start or rerun. The daemon refuses those calls.
  • Testers never use your real browser profiles, cookies or keychains.
  • Two runs never build in the same folder at once. A run you stop by hand closes as "stopped" with no verdict.

Known gap: the test app uses your real agent CLIs, so a test that presses Update in Harness settings updates them.

Review

Start with backend/internal/litmus/types.go, then verdict.go, service.go, and skillassets/litmus/SKILL.md (the tester's instructions).

cd backend && go test ./internal/litmus/ ./internal/cli/ ./internal/httpd/... ./internal/skillassets/ ./internal/session_manager/ ./internal/daemon/

One tester session takes a pull request, builds the PR commit and the
main commit it branched from, opens each as a private AO window, drives
it over Chromium's debug port with the bundled agent-browser, records
screenshots and video, compares the two builds, and posts one comment on
the PR with the verdict and the evidence.

- `ao testing` verbs: intake, prepare, launch, drive (snapshot, click,
  fill, type, press, wait), check add/start/end, test (a package's tests
  inside a checkout, with the PR's test files optionally copied onto
  main), compare, submit, cleanup, prune.
- Verdict rule: fail if anything fails on the PR build; pass if all
  passes there and at least one check passes on the PR build while
  failing on main; otherwise partial. Same failure on main is
  pre-existing and excluded. Submit refuses a run that skipped main
  without a reason it can verify.
- One comment per PR, upserted by a hidden marker, written in plain
  words from the tester's notes: side-by-side pictures, playable videos
  of both builds, GIFs and step shots in a details block, the areas the
  PR touches.
- Tester skill as a role description, plus a context pack per PR: PR
  text, diff by area, suggested checks, checkout paths, how the touched
  features are gated, how the author tested.
- Guards: no skipping main without reason, no posting without evidence,
  an allow-listed environment for PR test code, no overlapping runs, a
  reaper for testers that never reach intake, retention after every run.

Tried on #6403, #6266 and #6387 with one unattended run each on this
build; all three pass with evidence in the comment.
@Rishet11

Copy link
Copy Markdown
Collaborator Author

Check status on the author's Mac (8 GB): the repo-wide pre-commit gate (go test ./... for the whole backend) was stopped at 119 packages; its failures (claudecode, crush, goose, modelcatalog, opencode, opencodev2, pi, codexappserver, opencodev2acp, reviewer/opencodev2) fail identically on an untouched main checkout because those tests probe binaries on the machine. golangci-lint on every package this PR touches reports 0 issues, gofmt is clean, and the scoped suite (litmus, cli, httpd, controllers, apispec, skillassets, session_manager, daemon) passes: 3,078 tests. The commit therefore bypassed the local hook; CI is the real gate.

@github-actions

Copy link
Copy Markdown
Contributor

🏆 Review leaderboard

Oct 5–12, 2026 · UTC

Rank Reviewer PRs reviewed Review rounds PR comments
🥇 @illegalcall @illegalcall 32 52 8
🥈 @ronishrohan @ronishrohan 17 27 11
🥉 @nikhilachale @nikhilachale 13 32 4
4 @Prasad-D-Ware @Prasad-D-Ware 12 21 0
5 @Vaibhaav-Tiwari @Vaibhaav-Tiwari 8 11 1
6 @axisrow @axisrow 8 10 0
7 @codebanditssss @codebanditssss 6 10 2
8 @mohakchakraborty2004 @mohakchakraborty2004 6 6 1
9 @Rishet11 @Rishet11 4 10 2
10 @Pritom14 @Pritom14 4 4 3

Ranked by distinct external PRs reviewed, then review rounds, then PR comments. Self-activity and bot activity are excluded.

Show 5 more reviewers
Rank Reviewer PRs reviewed Review rounds PR comments
11 @Annieeeee11 @Annieeeee11 3 6 1
12 @neversettle17-101 @neversettle17-101 3 4 0
13 @harshitsinghbhandari @harshitsinghbhandari 2 4 0
14 @whoisasx @whoisasx 2 2 0
15 @AgentWrapper @AgentWrapper 1 3 0

- GET /testing/runs and /testing/runs/{id}: a project's runs with verdict,
  duration, comment link, tokens, and the full run for a detail view.
- POST /testing/rerun: a new run on the same PR for the same project.
- Ask before posting: a per-project flag; when on, submit composes and
  writes the comment and stops at awaiting-approval; /runs/{id}/post
  publishes the evidence and posts it through the same code path as
  submit, /runs/{id}/discard cleans up without posting. The sweep and
  retention leave awaiting runs alone.
- Prune: a run dir younger than five minutes with a spawn marker and no
  run.json is a starting run, not an unreadable one.
- CLI: ao testing runs, rerun, post, discard, settings.
…rt 2)

The comment's first line reads "Litmus tried #6403 (for #6390) on the PR
build and on main." when the PR closes an issue, with the issue linked.
Issues come from Closes/Fixes/Resolves phrases in the body, else the
first issue link, else GitHub's own closing references; at most three.
When the PR title is vaguer than the issue title, the suggested
headline check uses the issue title.
…t 3)

A new /projects/$projectId/litmus route, reached from the project's menu
in the sidebar: a "Test a PR" input, an "Ask before posting" switch,
the runs table (PR, title, verdict, duration, started, comment, tokens,
Rerun) polling while a run is live, and a detail drawer with the
rendered comment, the checks, and Post / Discard for a run awaiting
approval. Hidden for the standalone workspace and remote-host projects,
whose daemon is not this one. Strings in all eight locales; the
non-English ones are machine translations awaiting a native review.
…t 4)

In a project's orchestrator chat, "test PR #N", "test PR N", "test <PR
url>" or "litmus <N or url>" starts a Litmus run for that project
instead of going to the agent, and a strip in the chat says which PR
and session started, with a link to the Litmus view. Worker chats,
remote hosts and messages with attachments pass through untouched.
… 2, part 5)

- Runs are listed only for their own project; a bare PR number resolves
  against the project's own repository, never another run's.
- A discard that cannot clean up leaves the run as it was, and a
  discarded comment can never be posted.
- Rerun posts only when the original run posted.
- A failed post is shown in the drawer with its reason.
- Tokens are counted only for closed runs from a bounded cache; the PR
  title is stored on the run at intake instead of re-read on every poll.
- A run awaiting approval for seven days is discarded by the sweep.
- Post runs on a detached context, so a closed tab cannot leave a
  half-posted comment.
- The settings switch settles on the server value after quick toggles.
- The checks table says "Build" and "PR build"; plain hyphens replace
  em-dash placeholders.
@i-trytoohard i-trytoohard added this to the Agents & orchestration milestone Oct 12, 2026
@i-trytoohard i-trytoohard added comp/daemon Go daemon, process lifecycle, and backend control plane. enhancement New feature or request labels Oct 12, 2026
…part 6)

A cold build of both legs ran for six minutes on #6463 while the CLI cut
the prepare call at the two minute command timeout; every retry was refused
as busy, the tester gave up nine seconds before the build finished and the
run idled to its deadline.

- cli: prepare gets its own 11 minute bound, outlasting the daemon's cap on
  one prepare, the way test already outlasts its cap.
- service: a prepare that finds one in flight for the same run waits for it
  and then runs itself (quick once the clones are warm) instead of failing;
  a waiter that gives up gets its own context error.
- status: lists the long verbs still running for the run with their elapsed
  time, so a poller can tell a long build from a stuck one; the CLI prints
  "prepare in flight for 3m10s".
- skill: says prepare returns when both builds are ready and a cold build
  takes minutes.
Six checkouts of 1.2 GB each were left on disk as <sha>.pruning, some
renamed four times. The retention pass moved a checkout aside without
looking at its context and then skipped the delete silently once the
context was cancelled; the next pass saw the aside dir as a checkout,
could not match it to a pin, and moved it again.

- A pass never moves aside what it will not delete: the context is
  checked before the move, and a dir already moved aside is deleted
  whatever the context says.
- A dir left aside by an earlier pass (<sha>.pruning, any depth) is
  deleted directly, pinned commit or not: no run can use it. Aside dirs
  go first, so a move never lands on one.
- The retention pass after a submit runs on a context that cannot be
  cancelled: the client that closed the run may hang up.
…ase 2, part 8)

A reviewer read seven comments against their screenshots: "That is the
fix, and it holds." repeated after every compared check, the file
inventory said "covered above" for areas whose check was skipped, and
"Took N minutes." stood as its own paragraph.

- A check's paragraph ends on its main-side observation; the result
  line already carries the verdict.
- An area counts as covered only when a check naming it ran on the PR
  build; the block is now one sentence listing what was not checked,
  or nothing.
- The duration sits in the first line: "(4 minutes)".
…ase 2, part 9)

- A git, install or build failure in prepare reached the tester as a
  bare "Internal server error". It is now TESTING_PREPARE_FAILED with
  the leg, the commit and the command's last lines, and is logged.
- A run blocked before any check ran rendered "On the PR build, ." for
  each planned check. A blocked run keeps the first line, the result
  line, what was not checked and the tester's reason; a check that
  never ran on the PR build gets one sentence saying so.
… (phase 2, part 10)

The PR build and the main build opened as two bare Electron windows that
looked like the user's own dev AO. Litmus now passes LITMUS_PR with the
leg, and an unpackaged desktop that finds LITMUS_LEG names itself
"Litmus: PR build #6572" or "Litmus: main for #6572" (app name, window
title) and sets a Litmus Dock icon. A window Litmus did not launch is
untouched, so the dev-window rule from #3642 still holds.
…e under test (phase 2, part 11)

Part 10 put the Dock icon and window name into the desktop main process,
which the builds under test never run: they run the PR's own checkout.
The label now ships with Litmus as a small script and icon written into
the leg dir at launch and loaded by the unpackaged Electron through
NODE_OPTIONS=--require, so it applies to any checkout. The script waits
for Electron to register its module (a preload runs before that), names
the app and every window "Litmus: PR build #N" or "Litmus: main for #N"
and sets the Litmus Dock icon; plain Node processes in the dev chain load
it and do nothing. The desktop-side change from part 10 is reverted.

Seen live on #6576: the PR build's log says
  litmus-window: labelled "Litmus: PR build #6576".
…gh effort (phase 2, part 12)

The tester could only run `ao testing` and read files, and it ran at the
default medium effort. On a theme PR it wrote a note that its own
screenshots contradicted.

- Open toolset: any shell command, file edits, the web, and gh reads on
  the user's login.
- The deny list names what must never happen: git push, gh commands that
  write, and ao commands that reach other sessions, the daemon, GitHub or
  the desktop app.
- git push is also blocked through the session's git config env for the
  common remote forms. Git settings already in the env are kept.
- Model sonnet at effort high on spawn, relaunch and native resume.
- The spawn prompt and SKILL.md describe the open shell and its limits:
  scratch files in scratch/, Litmus's own run files and the checkouts
  left alone.
…et high only as the fallback (phase 2, part 13)

A tester now keeps the model and reasoning effort the AO project sets for its
workers. Litmus fills in sonnet / high only where the project names none, on
both a fresh launch and a native resume.
…del (phase 2, part 14)

task or chat it started inside the app under test, and those run on the
owner's account. SKILL.md now says to use Haiku 5.5 or Codex's smallest
model with a one-line prompt, unless the PR is about a specific agent or
model.

To stay under the skill's 900-word cap, three sentences that repeat other
parts of the skill are removed: "Litmus writes the comment from your notes"
(the Notes paragraph says it), "Every action leaves an after-screenshot and
the verb reply names it" (the Judge paragraph says to read the after-shot
the verb named), and the repeat of what is out of reach at the end of the
Explore paragraph.
…x (phase 2, part 15)

"test PR #N", "test pr 6588 please", "please run litmus on PR 6588" or a pull
URL typed in any local project session, or as a new task's prompt, starts a
Litmus run instead of reaching the agent. Only the orchestrator chat did this
before, and only for the exact "test PR #N" form. Sentences that mention a PR
for another reason ("fix the failing test in PR 6588", "the unit test is flaky
since #123") still go to the agent.

The using-ao skill now tells every agent to start Litmus when asked to test a
pull request, and never to check the PR out and test it by hand.
…, tester settings (phase 2, part 16)

One commit because these parts share types.go, service.go, verdict.go,
comment.go and intake.go; split by hunk they would not build on their own.

Intake and builds
- Litmus keeps its own bare mirror per repo under <data dir>/litmus/mirrors
  and never borrows the user's checkout. Build copies are shared clones of
  it, every commit pinned under refs/litmus/keep/<sha>; prune drops pins
  nothing needs and the daemon sweep runs gc when no run is in flight.
- The PR's file list, counts and patch come from git (merge-base, three-dot
  diff, patch streamed to a 5 MB cap); GitHub supplies the words. Only "PR
  not found", "cannot get both commits" and "repo not registered" stop a run;
  everything else is a Run.Gaps line shown in the pack, verdict.md, the
  comment's Limits block and the run drawer. #6588 (298 files, refused by
  GitHub's diff limits) now intakes in under a minute.
- "main" is built at the commit the PR branched from, with a gap line saying
  how many commits behind; a merged PR keeps the base tip.
- The 25-minute budget starts when both builds are ready; prepare is capped
  at 20 minutes. Big PRs (over 30 files or 2,000 lines) get a per-area table,
  the 25 most changed files, UI changes first and git commands for the rest.

Verdict and comment
- Test receipts record exit code, did-not-build and how many tests ran.
  check end refuses a label its receipts contradict; a fail that only shows
  main failing to compile is stored as skipped. A main-side fail counts only
  when a test ran and failed or a screenshot backs it; "pre-existing" needs
  the same state on both builds.
- Checks are pairs: the cap counts pairs and a main twin never uses a slot;
  submit --skip-base is accepted at any time; the comment says how many
  checks were compared on main.
- New check state "absent": main does not have the feature. Head pass +
  main absent is a new feature; head fail + main absent is the PR's own
  problem and never pre-existing.
- Wording: "tests only, the app was not opened" when no window opened;
  "could not start" when nothing was built; builds and testing time shown
  separately; gap text scrubbed of local paths.

Tester
- ao testing settings --harness --model --effort choose the tester per
  project (Litmus-owned file), before the project's worker config; the Codex
  harness is supported. A tester session cannot post, prune or start runs
  (daemon refusal), has no gh login inside the run, and the Claude deny list
  gains gh auth, ao testing post/prune/start and other runs' results.
- SKILL.md: a check is done only with both sides, done back to back; UI
  checks before package tests; a feature main lacks is absent, never
  skipped; never touch the user's real browser profiles, cookies, keychain
  or accounts; use the whole budget.

Also: evidence commit messages say "PR N" so GitHub stops listing them on
the PR timeline; forAuthor names what tripped; zero-test runs do not count
toward the test cap; openapi.yaml and schema.ts regenerated.

@axisrow axisrow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes. The core design is impressive and the security fundamentals hold: no PR-controlled string reaches sh -c, prune deletions are withinRoot-gated, CDP ports bind loopback, comment upsert cannot double-post, and the intake fork-point logic is correct. But the tester guard has three blind spots that defeat ask-before-posting, the publish pipeline dies with the CLI call, and PR-controlled install/build runs with the daemon environment. Inline findings carry the details.

Optional, no change required: (1) the partial-verdict headline at comment.go:339 prints "the PR build works, but nothing differed" when the only failure is pre-existing and excluded from the tally — technically true, but it sits above a table row reading fails; a mention of the pre-existing failure would read better. (2) useLitmusChatCommand discards the repo-lookup error, so an API failure while typing a bare number reports needsUrl even though the input was fine. (3) LitmusRunDrawer keeps post.error visible after a successful discard (post.error ?? discard.error is never reset). (4) RefuseTester trusting a caller-supplied sessionId is documented as stopping mistakes rather than a determined agent — consistent stance, not counted against the PR; the inline gate findings are about verbs outside the documented scope.

r.Get("/testing/projects/{id}/settings", func(w http.ResponseWriter, r *http.Request) {
out, err := s.ProjectSettings(r.Context(), chi.URLParam(r, "id"))
writeTesting(w, r, out, err)
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No RefuseTester gate on this route while the sibling user-only verbs (start, post, prune) all have one, and ao testing settings is absent from the deny list in policy.go. A tester session can rewrite its own project settings: with --ask-before-posting=false (mustAsk reads the settings live at submit time) the verdict comment auto-posts to GitHub, defeating the approval gate this design is built around; the same door switches the tester harness to codex. Add s.RefuseTester(in.SessionID, "settings") and the verb to the deny list.

return litmus.Run{}, err
}
return s.Post(r.Context(), chi.URLParam(r, "id"))
}))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This route lacks the RefuseTester gate its sibling post has six lines above, and ao testing discard is not on the deny list. A tester can discard any run by id (ids are enumerable via GET /testing/runs), and a discarded awaiting-approval run permanently refuses a later post — silently voiding the pending user comment, the same outcome the post refusal exists to prevent. The CLI already sends the caller session for this verb (withCaller), so the gate was clearly intended; add it.

}))
r.Post("/testing/runs/{id}/discard", testingPost(func(r *http.Request, _ TestingRequest) (litmus.Run, error) {
return s.Discard(r.Context(), chi.URLParam(r, "id"))
}))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No RefuseTester gate, and the rerun CLI verb sends no caller attribution at all (the body is built without withCaller), so a daemon-side check could not see the caller even once added. ao testing rerun is the same operation as the forbidden ao testing start — spawn a tester session, unbounded — and is a natural move for a tester. Gate it here and send withCaller from the CLI.


// baseEnv is the daemon's environment minus anything that could steer a target
// at the real AO (AO_*) or make Electron behave as plain node, with nodeDir first on PATH.
func baseEnv(nodeDir string) []string {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

baseEnv is the whole daemon environment (only AO_*/ELECTRON_RUN_AS_NODE stripped) and it feeds the install and build steps (prepare.go:279-281) and the app under test via legEnv (target.go:173) — all PR-controlled code: npm postinstall scripts run with it. A malicious PR reads the daemon GH_TOKEN/GITHUB_TOKEN, SSH agent, or any cloud key in the environment. testrun.go deliberately allowlists the env for test scripts and its comment says no tokens reach PR-controlled code; the install/build path breaks that promise. Build these environments from an allowlist like testEnvKeep plus the toolchain and recipe env.

func GuardEnv(env map[string]string, runDir string) {
ghDir := filepath.Join(runDir, "gh-config")
_ = os.MkdirAll(ghDir, 0o700) // best effort: gh treats a missing config dir as empty too
env["GH_CONFIG_DIR"], env["GH_TOKEN"] = ghDir, ""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only GH_TOKEN is emptied; GITHUB_TOKEN and GH_ENTERPRISE_TOKEN inherited from the daemon environment still authenticate gh inside the run (gh reads those variables regardless of GH_CONFIG_DIR). The Codex tester gets no deny list precisely because GuardEnv and the skill are what hold it back — with GITHUB_TOKEN set it can post to the PR directly. Empty the standard gh token variables here too.

// copied from a sibling clone with the same lock hash, or a fresh install.
func (s *Service) install(ctx context.Context, r Recipe, dir, hash string, env []string, m *Manifest) error {
modules := filepath.Join(dir, r.Install.Dir, "node_modules")
if _, err := os.Stat(modules); err == nil && m.LockHash == hash && !m.InstalledAt.IsZero() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shared per-SHA clone under repos/ has no cross-run serialization: cloneLeg releases the mirror lock before install, busy tracking is per run, and the warm check needs Manifest.InstalledAt, written only after install finishes. Two runs preparing the same commit (two testers on one PR, or two PRs sharing a base SHA) both see a cold manifest and interleave npm ci or the donor cp into the same node_modules — corrupted builds that flake with no hint of the cause. Hold a lock keyed on the clone dir across install, or install into a temp dir and rename atomically.

}
_ = os.RemoveAll(modules)
}
if _, err := s.sh(ctx, filepath.Join(dir, r.Install.Dir), env, r.Install.Cmd); err != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Steps execute only sh -c Cmd; Step.Args is documented in recipe.go as the direct-exec form and ParseRecipe accepts it, but it is silently dropped here and for the build step below. A stored recipe with cmd: npm, args: [run, build] runs bare npm on every prepare and hangs or fails with no validation error. Honor Args (direct exec when present) or reject recipes that set it.

id="litmus-ask-before-posting"
checked={settings.data?.askBeforePosting ?? false}
disabled={settings.isLoading || setSettings.isPending}
onCheckedChange={(askBeforePosting) => setSettings.mutate({ askBeforePosting })}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Switch PUTs only { askBeforePosting }, but SetProjectSettings replaces the whole record — the CLI settings command does read-modify-write for exactly this reason. Toggling the switch after ao testing settings --model/--harness/--effort silently wipes those fields, and later testers spawn with defaults. Merge with settings.data before the mutate.

onRerun: () => void;
}) {
const { t, i18n } = useTranslation();
const commentState = run.commentState?.startsWith("failed") ? t("litmus.comment.failed") : run.commentState;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment column renders the raw commentState enum (awaiting-approval, discarded, off) as user-facing text; only the failed prefix is translated. With ask-before-posting on, every row shows the literal kebab-case enum in every locale. Add keys under litmus.commentState.* like the drawer handles its states.

if cur.State == RunIntake { // the tester's time starts when the builds are ready; a repeat prepare does not extend it
cur.Deadline = s.clock().Add(RunBudget)
}
cur.Legs, cur.State, cur.Error = legs, RunPrepared, ""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This update is unconditional, without the Closed() guard its siblings have (target.go:493 and 622), and markBusy pairs only launch against submit/cleanup — not prepare. ao testing submit --blocked accepted while a long prepare is in flight closes the run; when prepare then finishes, this line flips it back to open RunPrepared with the error cleared: an open run on a killed session whose verdict says blocked. Skip the update when cur.State.Closed(), or pair prepare against submit in markBusy.

… recording parts (phase 2, part 17)

Comments: a failing run posts one comment per failing check ("Bug k of N"),
each with up to four sentences, a cropped main-vs-PR image around the element
and that check's clip. A passing run posts one short summary. Reruns edit the
same comment by check name, mark a fixed bug "Fixed at <sha>", never delete.
With ask-before-posting the comments wait in comments/N.md and post as edited.

Tester turns: each action returns a compact snapshot (interactive elements
only), one receipt line and console errors only. Click receipts keep the
element box; a click refused because the element is covered still writes a
receipt. New `ao testing replay <check> --leg base` redoes on main a check
already walked on the PR build, matching each step by role and name and
stopping at the first missing or covered element.

Recordings: a failed stop no longer deletes the file; a recorder that stops
growing is restarted into a part file and parts are joined at the end;
per-check clips are cut from the PR recording, slideshow where it has gaps.

Also since part 16: the click guard is removed (tester instructions protect
the owner's logins, owner-login hash line in the run), page eval goes through
CDP, the tester cannot discard, prune, post or start runs, and the build
readiness limit is 5 minutes.
@Rishet11 Rishet11 changed the title Litmus: agentic PR testing inside AO (phase 1) Litmus: agentic PR testing inside AO (phase 2) Oct 14, 2026

@axisrow axisrow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes - not on the code, but on merge-readiness. I read the core surfaces end to end (policy.go, mirror.go, prune.go, verdict.go, comment.go, the testing controller) and found no concrete correctness or security defect in roughly 4.5k of the 31.7k lines: the git work is exec-argv only with no shell, mirror names are slash-free, prune confines every deletion behind a symlink-resolved withinRoot plus a move-aside re-check under the lock and keeps unreadable run dirs, the comment path sanitizes tester prose (no HTML, links, mentions or engine refs), posts via --input file rather than argv, upserts only the token-owning login marker comment, and skips REQUEST_CHANGES on the author PR. The tester policy states its own limits honestly (sh -c bypass, Codex without a deny list) and GuardEnv plus the emptied gh config are sensible second layers. What blocks merge today: (1) the branch is a draft and no CI check has ever run on it - flip it from draft so the full suite gates it, and confirm the three timing-sensitive tests the description names are stable under CI load, not just locally; (2) the branch conflicts with current main and needs a rebase (the session-manager and TaskComposer areas it touches are exactly the ones main has been moving). (3) Process, not code: at 120 files and +31.7k this is beyond a reviewable unit even with the excellent line-distribution table in the description; the phase numbering implies a phased plan, so if a third split is feasible along the bodies own area rows (service/API/CLI and frontend are the natural cut lines), it would let each half actually be read rather than sampled. Non-blocking notes: the sweep/reaper interplay (SweepExpired killing sessions, mirror GC gated on anyInFlight) deserves a focused look from someone who owns lifecycle - the code reads correct but it composes with #6205s reaper gating and #6612s reconcile-kill takeover; and the Codex tester running with no deny list is a documented gap worth an issue so it is not forgotten.

…at stop (phase 2, part 18)

agent-browser's live recorder stopped receiving frames about two minutes
into a run whenever screenshots were taken, and its file was often
unplayable. Plain Chrome keeps streaming CDP screencast frames with
screenshots in between, so Litmus now holds its own screencast per leg,
writes JPEG frames and a frames.jsonl index, and builds <leg>.mp4 with
ffmpeg at stop using each frame's real duration. No live encoder.

Removed: agent-browser record start/restart, dead-recorder detection,
part files and joining, and the slideshow fallback. Kept: per-check
clips, cut video and captions, same output paths.

Recorder details: everyNthFrame 6 with immediate acks and a 180 ms cap,
monotonic frame offsets, reconnect with backoff that respects
cancellation and stays off the Drive lock, frames recovered after a
daemon restart, closed-run frames pruned, encode timeout scales with
frame count. A playable recording adds no run gap; trouble goes to
engine.log.

Also fixes a blocked submit dropping a recorded failure from the PR
comment, and a launch test whose stand-in target exited at once.

@axisrow axisrow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes again - same three merge-readiness items, none resolved at head a231ddc. For the record, what did move: the branch is now 22 phase-labeled commits instead of one squashed drop, which genuinely helps partial review, and the new screencast.go replaces testrun.go with a clean single-writer CDP capture (bounded frame index, manifest with reconnects/problems, O_EXCL index, atomic manifest write) that stays within the feature local-only posture - I have no new code finding there. What still blocks merge: (1) the PR is still a draft and the branch has never had a single CI check run - flip it out of draft so the full suite gates it; with the size now at +37.5k/-5.9k across 133 files, the remote race shards are also the only real gate for the sweep/lock code, as discussed last round; (2) mergeable is still CONFLICTING with current main and the drift is growing (the branch gained ~5.8k added lines since the last review while main moved on), so rebase soon - every day this waits the conflict resolution gets riskier; (3) the split suggestion stands, more so now: the size grew instead of shrinking, and the phase commits show the natural cut lines already exist (the runs API + view + chat command are separable from the engine/verdict core along those phase boundaries). The implementation quality I sampled last round and this round remains high; this verdict is purely about landing a 37k-line change with zero CI runs on top of a conflicting base.

…t 20)

- check end --shot <N> names the head receipt that shows a failure seen earlier; the bug picture uses it, and crops main to the same region.
- Without --shot, the bug picture uses each side's check-end theme shot (full frame), else the last after-shot as before. Run 188's error appeared after the last click, so the old picture showed no bug.
- Bug text drops clauses that name receipts, and a sentence ending in ";" ends with "." (entity semicolons kept).
- SKILL: notes omit receipt numbers; --shot documented.
…e errors in bug comments (phase 2, part 21b)
# Conflicts:
#	backend/internal/httpd/controllers/dto.go
The merge with main gave buildSystemPrompt a requestedOutput argument;
the tester prompt test still used the old call and did not compile.

DraftComments now stats the comments path first. On Windows, ReadDir on
a regular file does not report an error, so a broken path read as no
drafts and TestTestingShow failed there.
golangci-lint findings in litmus: close websocket handshake bodies,
explicit ignores for cleanup errors, process.CommandContext for the
keychain probe, a bounds check before indexing a regex match, dead
params and assignments removed (publishComment's videos, existingVideos),
import grouping.

using-ao skill: after showing a run's verdict and drafted comments, the
agent asks the user whether to post them. A live check showed it gave
the result but never asked.
A tester session killed before its run finished went unnoticed until the
deadline, where closeAtDeadline marked unfinished checks "ran out of time"
and closed the run through its comment mode (a false draft with
ask-before-posting). The sweep now expires a run whose session reads
exited with ReasonStopped and no verdict or comment, closeAtDeadline
refuses such runs on later ticks, and ao testing show/runs print the reason.
…m testers

Checkout folders are keyed by commit, so two runs of the same commit shared
one folder while the mirror lock covered only the git work: both could
install and build there at once. A context-aware per-folder lock now spans
checkout, install, build and the manifest write; different folders (head
and base) still prepare in parallel, and a waiter reuses the finished work.

The rerun endpoint lacked the tester refusal that start, post, discard and
prune have, so a tester could start another tester through rerun. It now
refuses tester callers, and the CLI sends the caller session id.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/daemon Go daemon, process lifecycle, and backend control plane. enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants