test(lifecycle): make --mode api a real, free Golden Path suite (#1068) - #1147
Conversation
) #948 deleted `tests/lifecycle/test_api_lifecycle.py` because it was green theatre: a `@pytest.mark.skip` class whose methods raised NotImplementedError, so `scripts/lifecycle --mode api` collected only skips and exited 0 while CLAUDE.md advertised it as the pre-PR gate. This is the actual work. The new suite drives the Golden Path over HTTP against the real FastAPI app on `CODEFRAME_LLM_PROVIDER=mock`: workspace init, PRD upload, task generation, approval, batch execution, terminal-state polling, and the event trail. It costs nothing and needs no ANTHROPIC_API_KEY, so it is deliberately NOT marked `lifecycle` — the default CI gate collects it on every PR, which is the point the issue makes about the mock provider. The plan preserved in the issue named endpoints that do not exist as written; the issue says to verify them, and it was right. Checked against the live OpenAPI schema: plan actual POST /api/v2/workspace/init POST /api/v2/workspaces POST /api/v2/tasks/generate POST /api/v2/discovery/generate-tasks POST /api/v2/batches/run POST /api/v2/tasks/execute `scripts/lifecycle` now resolves per-mode paths, markers and key requirements rather than passing `-m lifecycle` for everything — with the api suite unmarked, that combination selects zero tests and exits 0, which is the same silent success #948 removed the stubs for. A gate test measures that. `tests/test_lifecycle_gates_948.py` flips as the issue requires: `api` is now asserted to work (and to work with no API key present), while `web` keeps every must-fail-loudly assertion. Two further gates assert AC2 rather than assuming it — the CI step must not `--ignore=tests/lifecycle`, and the gate's own selector must actually collect the api suite while leaving the paid cli suite behind.
…1068) Codex review, and correct. `ApproveTasksRequest` is exclusion-shaped — it has `excluded_task_ids` and no `task_ids` — and Pydantic drops unknown fields, so the driver's `{"task_ids": [...]}` returned 200 while approving every BACKLOG task. The existing assertion (`the chosen task is READY`) held either way, so the suite looked like it covered scoped approval and did not. The helper now computes the exclusions from the live task list, and a new test asserts the tasks it did not choose stayed BACKLOG. Verified the guard bites: restoring the old one-line body fails it.
|
Claude finished @frankbria's task in 6m 12s —— View job GLM Precision Review — PR #1147Hunting concrete defects only (logic errors, null/error paths, security, data loss, races, contract breaks, resource leaks). Style/coverage/architecture are covered by a separate review.
Verified against source (not just the diff):
✅ GLM review: no defects found.No concrete failure scenario could be constructed for any hunk — the test's contracts all match the live source, and the bash/AST/test logic is internally consistent. |
ReviewI independently traced every endpoint and response model this PR's tests rely on against the live code (not just the PR's own claims) — Two minor, non-blocking things:
No prior formal review exists on this PR (only a placeholder "working" comment), so this is a fresh pass. Code quality, test structure, and self-documentation (docstrings explaining why, not just what) are strong throughout — the SSE-hang investigation and the honest "known limitations" section in both the PR body and the test file are a good example of documenting a real constraint instead of hiding it. 🤖 Generated with Claude Code |
Post-PR review flagged that a hang in this suite would stall the default backend gate rather than report anything, since it is deliberately unmarked. The reported reproduction does not occur here — the file runs in ~7s locally and passed inside the full 6450-test gate run — but the consequence is real enough to bound: pytest-timeout is already a dependency, so a marker turns any hypothetical block into a failure with a traceback. Same reasoning that kept the SSE test out.
Post-PR third-party review (
|
|
Claude finished @frankbria's task in 6m 7s —— View job GLM Precision Review — PR #1147 (synchronize)Hunting concrete defects only (logic errors, null/error paths, security, data loss, races, contract breaks, resource leaks). Style/coverage/architecture are covered by a separate review.
Verified against source (focused on the two new commits since the prior clean pass):
✅ GLM review: no defects found.No concrete failure scenario could be constructed for any hunk — the approve-fix logic, the timeout marker, and the bash/endpoint contracts are all internally consistent and match the live source. |
ReviewI read the current diff directly (not just the PR description) and re-checked the two non-blocking items from my prior pass, since the task asked me to be consistent with earlier reviews rather than repeat them wholesale. Endpoints, contracts, and the approve-bug fix all check out.
The Two minor items from my earlier pass are still present, unchanged, still non-blocking:
Neither is a regression from this update and neither blocks merge. New in this update (the 74d3ced commit) — the AST-based gate test. Nothing else stood out — test coverage for the stated scope (Golden Path over HTTP, mode-resolution gates, AC1/AC2 assertions) is thorough, the "known limitations" sections (SSE, web mode) are honest about what's not covered rather than papering over it, and the CLAUDE.md/script doc updates match the actual behavior. 🤖 Generated with Claude Code |
GLM Precision Review — PR #1147 (re-review on synchronize)Re-reviewed the two commits pushed since the last clean pass: Verified against source, not just the diff:
✅ GLM review: no defects found.No concrete failure scenario could be constructed for any hunk — the new commits and the surrounding endpoint/bash contracts are internally consistent and match the live source.
|
Demo — every acceptance criterion, with outcome evidence
The CI run is the evidence, not the green tickThe post-PR review's P1 said this suite would make the backend gate non-terminating. Here is that job's own log: Seven dots, 1.62s for the full Golden Path, on a clean runner with no API key. That is simultaneously the disproof of the P1 and the proof of AC2 — the suite is genuinely in the default gate, not merely eligible for it. Full check statusAll 13 checks pass ( What I am explicitly not claiming
|
…1153) * test(lifecycle): drive the Golden Path through the browser (#1068) 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. * fix(lifecycle): only build pytest args when there is a pytest leg 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.
1 participant
Closes #1068 (API half — see Scope below).
What
#948deletedtests/lifecycle/test_api_lifecycle.pybecause it was green theatre: a@pytest.mark.skipclass whose methods raisedNotImplementedError, soscripts/lifecycle --mode apicollected only skips and exited 0 whileCLAUDE.mdadvertised it as the pre-PR gate. This is the work #948 spun off.The new suite drives the Golden Path over HTTP against the real FastAPI app on
CODEFRAME_LLM_PROVIDER=mock: workspace init → PRD upload → task generation → approval → batch execution → terminal-state polling → event trail. It costs nothing and needs noANTHROPIC_API_KEY, so it is deliberately not markedlifecycle— the default CI gate collects it on every PR. That is the point the issue makes about the mock provider: it covers the server-layer seam no unit test reaches, for free, rather than being a paid duplicate of the CLI test.7 tests, ~7s.
The plan's endpoints were wrong, as the issue warned
The issue preserves the deleted stub's plan and says to verify each endpoint rather than trust it. Checked against the live OpenAPI schema:
POST /api/v2/workspace/initPOST /api/v2/workspacesPOST /api/v2/tasks/generatePOST /api/v2/discovery/generate-tasksPOST /api/v2/batches/runPOST /api/v2/tasks/executeuse_llm=falseon generation is deliberate: it makes the task list deterministic, so a failure here is a server-layer failure rather than the mock returning prose the decomposer cannot parse (#1115 turned that into a hard error, correctly).scripts/lifecycleresolves per-mode, not one-size-fits-allIt used to pass
-m lifecyclefor every mode. With the api suite unmarked, that selects zero tests and exits 0 — the same silent success #948 removed the stubs for. Now each mode carries its own path, marker and key requirement:clitest_cli_lifecycle.py-m lifecycleapitest_api_lifecycle.py-m "not lifecycle"alltests/lifecycle/-m "lifecycle or not lifecycle"webexit 3, unchangedA gate test measures that selector rather than trusting it.
The gates flip, as the issue's last AC requires
tests/test_lifecycle_gates_948.pyasserted thatapiandwebare rejected. Nowapiis asserted to work — and to work with noANTHROPIC_API_KEYpresent at all — whilewebkeeps every must-fail-loudly assertion. Two further gates assert AC2 instead of assuming it: the CI step must not--ignore=tests/lifecycle, and the gate's own selector must actually collect the api suite while leaving the paid cli suite behind.One of these is AST-parsed rather than grepped: the new file's docstring quotes both
NotImplementedErrorandpytest.mark.skipwhile describing the stub it replaces, so a substring check would report the stub it is documenting.Review finding, fixed and verified
Codex caught a real one.
ApproveTasksRequestis exclusion-shaped (excluded_task_ids, notask_ids) and Pydantic drops unknown fields — so the driver's{"task_ids": [...]}returned 200 while approving the entire backlog. The existing assertion ("the chosen task is READY") held either way, so the suite looked like it covered scoped approval and did not. The helper now computes exclusions from the live task list, and a new test asserts the tasks it did not choose stayedBACKLOG. Verified the guard bites: restoring the old one-line body fails it.The endpoint's own footgun — 200 + inverted semantics for the intuitive payload — is filed as #1146; no production caller hits that route today.
Known limitations
test_sse_streaming_delivers_eventsis only partially met.TestClient.stream()on an event-stream endpoint blocks: the response never completes, and the block happens inside anyio's portal where an httpx read timeout does not interrupt it. Both a plain context manager and a boundediter_lines()with a 5s read timeout hung until an outer timeout killed the run. A test that can hang CI indefinitely is strictly worse than none, so there is no test that opens the stream — with that reasoning recorded in the file. What the stream carries is asserted via the event log instead, and the real browser-to-server SSE path is covered by the Playwright harness ([P6.8.5] Rewrite Playwright browser E2E suite against current Phase-3+ UI #684/feat(e2e): rewrite Playwright browser suite for Phase-3+ UI (#684) #703). An in-process SSE test needs a live uvicorn on a real socket rather thanTestClient.--mode webstill exits 3 with its pointer, and every gate asserting that still passes. I'll assess it separately — [P2.23] Implement the web lifecycle test (Playwright; the API half shipped in #1147) #1068 should stay open for it.Testing
uv run pytest tests/ --ignore=tests/e2e -m "not lifecycle"— 6450 passed, 49 skipped, 421suv run ruff check .— cleanscripts/lifecyclemodes verified by--dry-run, including with the key unsetReview
Third-party pre-PR review:
codex review(opencode skipped — it mutates the tree and returns zero bytes on this repo, per prior sessions). One P2 finding, fixed above.