test(lifecycle): drive the Golden Path through the browser (#1068) - #1153
Conversation
The web half of #1068. #948 deleted `tests/lifecycle/test_web_lifecycle.py` because it was green theatre — a `@pytest.mark.skip` class raising NotImplementedError, so `--mode web` collected only skips and exited 0 while CLAUDE.md advertised it as the pre-PR gate. #1147 replaced the API half; this replaces the web one, and every mode the script offers is now real. `tests/e2e/lifecycle.spec.ts` starts from an EMPTY directory and drives the product through the UI: PRD upload, task generation, approval, execution. Every other spec in the harness asserts how a pre-seeded workspace renders, which cannot catch a broken write path. 3 tests, ~13s including server startup, free. Three things had to change for it to be possible: 1. MockProvider's default is "Mock response", which no parser can read. #1115 correctly made an unparseable decomposition a hard error, so CODEFRAME_LLM_PROVIDER=mock could not drive the THINK step at all — the API lifecycle test had to pass use_llm=false to avoid the LLM branch entirely. The mock now answers exactly one prompt with a valid shape: the task decomposition, matched on its full closing sentence rather than a keyword (the classifier mistake #1113/#1116/#1064 each made). The canned answer has a real dependency edge, because the prompt itself says an empty graph means the decomposition is wrong. Everything else is untouched, and a queued response or handler still wins. 2. WORKSPACE_ROOT now points at the PARENT of the seeded workspace, so the spec can create its own sibling and still sit inside a real allowlist (#896). Not unrestricted — the guard stays meaningful, and a third test asserts the seeded workspace was not touched. 3. scripts/lifecycle grew a Playwright leg. `web` runs no pytest at all rather than running one against a path with no tests, and `all` runs both engines. Assertions are outcome-based. The run's result is polled from the API as a task status, not matched as page text: `/done|completed|failed|blocked/` would have matched the sidebar's "Blockers" link and passed before the run finished. The approved task reaches DONE and the unapproved one stays BACKLOG — exact, so it would fail if approval leaked to the whole backlog (the #1146 bug). The gates in tests/test_lifecycle_gates_948.py invert as that issue requires: they asserted `api` and `web` FAIL, and now assert every advertised mode resolves to a suite while a typo is still rejected. Plus: the spec exists and carries its tag, and a CI step actually selects it — the tag is how both the script and CI find it, so an untagged spec would silently run in neither. CI runs it on every PR (`--grep "@smoke|@lifecycle"` in e2e-browser-smoke). Full backend suite 6502 passed; all 42 chromium specs pass.
|
Claude finished @frankbria's task in 9m 58s —— View job GLM precision review — PR #1153Scope: concrete defects only (logic errors, null derefs, security, data loss, races, breaking changes, resource leaks). Style/architecture/coverage are handled by a separate review.
Verdict: 1 minor finding (no critical/major defects).
What I verified and ruled out
|
ReviewSolid, well-scoped change — it closes the actual gap (#948/#1068) rather than papering over it, and the PR description's own "review finding" callout plus the extensive
One gap worth considering: No direct unit test for the new
Nothing else stood out — no security, performance, or correctness concerns in the diff itself. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The spec runs in CI, not just locally
On a clean runner, with no Outcome evidence for each acceptance criterion
The pipeline genuinely ran, rather than the page merely rendering — read straight off the workspace afterwards: $ cf tasks list # the workspace the browser built
DONE | Implement the core function
BACKLOG | Add a test for the core function
Checks
|
Bot review. `web` sets MARKER_ARGS=(), and the PYTEST_ARGS array was built
unconditionally — under `set -u`, bash < 4.4 (macOS still ships 3.2) errors on
"${empty[@]}". So the one mode that never uses those args was the one that
would have failed to start, on the shell a contributor is most likely to have.
Bash 5.2 here, which is why it passed locally and in CI. All four modes
re-verified by --dry-run; shellcheck clean.
|
Claude finished @frankbria's task in 6m 0s —— View job GLM precision review — PR #1153 (synchronize)Scope: concrete defects only (logic errors, null derefs, security, data loss, races, breaking changes, resource leaks). Style/architecture/coverage are handled by a separate review.
Verdict: ✅ GLM review: no defects found. The prior-review finding (empty What I verified
So
|
ReviewSolid follow-up. I focused on verifying the two things flagged in prior reviews on this PR and re-reading the diff end to end. The GLM review's bash bug is correctly fixed. My earlier suggestion (isolated unit tests for Beyond re-verifying those two items, I checked a few things not previously called out:
Nothing else stood out — the |
1 participant
Closes #1068. With this, every mode
scripts/lifecycleadvertises is real — the thing #948 spun the issue off to fix.What
tests/e2e/lifecycle.spec.tsstarts from an empty directory and drives the product through the UI: PRD upload → task generation → approval → execution. Every other spec in the harness asserts how a pre-seeded workspace renders, which cannot catch a broken write path. 3 tests, ~13s including server startup, free.Three things had to change to make it possible
1.
MockProvidercould not drive the THINK step at all. Its default is the literal string"Mock response", which no parser can read, and #1115 correctly turned an unparseable decomposition into a hard error:So
CODEFRAME_LLM_PROVIDER=mockfailed the Generate Tasks button, and the API lifecycle test had to passuse_llm=falseto skip the LLM branch entirely. The mock now answers exactly one prompt with a valid shape — the task decomposition, matched on its full closing sentence rather than a keyword, because a substring classifier is the mistake #1113, #1116 and #1064 each made in turn. The canned answer carries a real dependency edge, since the prompt itself says an empty graph means the decomposition is wrong. Everything else is untouched, and a queued response or handler still wins.2.
WORKSPACE_ROOTnow points at the parent of the seeded workspace, so the spec can create its own sibling and still sit inside a real allowlist (#896). Not unrestricted — the guard stays meaningful, and a third test asserts the seeded workspace was not touched, because this spec writes for real and every other spec asserts against that directory.3.
scripts/lifecyclegrew a Playwright leg.webruns no pytest at all rather than running one against a path with no tests — that is how a mode reports success for nothing.allruns both engines.cli-m lifecycleapi-m "not lifecycle"web--grep @lifecycleallThe assertions are outcomes, not page text
The run's result is polled from the API as a task status:
My first version matched
/done|completed|failed|blocked/ion the page — which matches the sidebar's Blockers link and would have passed before the run even started. That is the exact class of assertion this suite exists to replace.The persistence test is exact rather than "something moved": the approved task is
DONEand the unapproved one isBACKLOG. A looser check would pass if approval had leaked to the whole backlog — the #1146 bug, fixed two PRs ago.The gates invert, as #1068 requires
tests/test_lifecycle_gates_948.pyassertedapiandwebfail. They now assert every advertised mode resolves to a suite, that a typo is still rejected (different failures, and a wrapping script has to tell them apart), that the two free modes work with noANTHROPIC_API_KEYpresent, and thatallruns both engines. Two more: the spec exists and carries its tag, and a CI step actually selects it — the tag is how both the script and CI find it, so an untagged spec would silently run in neither.Review finding
codex reviewflagged that the gate test readstests/e2e/lifecycle.spec.tswhile the file was untracked, so a clean checkout wouldFileNotFoundError. Right about the consequence — the file was staged in the same commit, but "the assertion depends on a file being committed" is worth having been checked.Testing
WORKSPACE_ROOTor mock changesweb-uiunit testsruff,actionlint--modevalues--dry-run, including with the key unsetKnown limitation
Workspace creation is the one step still done API-side in
beforeAll: the UI has no "create a workspace from nothing" screen — it reads a path out oflocalStorage. Every pipeline step after that goes through the browser. Worth noting as a product gap rather than a test shortcut.