feat(tests,ci): journey tier + per-PR journey-smoke gate (#2335, #2336)

[dolho]

Summary

Rails R1 + R2 of the journey-coverage deck, together: R2 is a gate with nothing to run without R1, and R1 is a tier nothing enforces without R2.

The thing this exists for, concretely: on 2026-08-14 five of six stopped agents could not be started — HTTP 500 on the most fundamental operation the platform has — and it reached users. A human found it by poking around. Nothing automated caught it, because the per-PR gate collects tests/unit/ only and no test in the merge path had ever started a real agent.

R1 — tests/journeys/ + run-full.sh --tier journeys

Reuses the existing harness rather than building a second one, as the issue asks: per-tier verdicts, the timeout, the venv pin and the skip audit all come for free, and run_tier made the wiring literally one line.

Three rules the tier does not bend:

• A journey FAILS; it does not skip. tests/conftest.py::created_agent calls pytest.skip when an agent will not start — reasonable for a fixture whose tests are about something else, and precisely the blind spot here. In this tier, "the agent never reached running" is the finding.
• Preconditions are checked once, loudly. An unreachable stack fails the tier naming the URL it tried, not 40 confusing errors downstream. A tier that cannot run is a failure, never a silent pass — run-full.sh's own stated rule 1.
• Nothing is touched that the tier did not create. Every agent is pytest-ephemeral-journey-<hex>, torn down by that name; teardown is idempotent, so a crashed run leaves the tier re-runnable with no manual cleanup.

poll_until is the only synchronisation primitive — sleep-as-synchronisation pays the worst case every run and still loses the race on a loaded runner. Failures name the broken promise and how long they waited:

agent 'pytest-ephemeral-journey-a1b2c3d4' was created but never reached 'running' — waited 90s of 90s.

never a bare assert 200 == 500.

R2 — .github/workflows/journey-smoke.yml

Every PR to dev. 30-minute bounded job whose timeout fails rather than passes, and which treats "collected nothing" (pytest exit 5) as a failure — the #2029 class a required check exists to prevent.

Two things I want visible rather than glossed

1. AC #2 ("a real chat turn with real output") is credential-bound, and this repo deliberately withholds credentials from PR triggers. pull_request exposes repository secrets to fork PRs while running arbitrary shell from the PR's own tree — integration-nightly.yml sets out the full reasoning and keeps itself credential-free for exactly that reason.

So the J03 first-turn journey ships in the tier (runs on a developer's stack and in the nightly) and skips with an allowlisted, visible reason otherwise. What gates every PR is the lifecycle journey — create → running → stop → start → running — which is where the 08-14 regression actually was.

Resolving AC #2 properly needs a same-repo-only workflow with an environment approval. That is a security decision, not something to slip into this PR.

2. AC #3 is done, and it earned its keep — it failed twice before it passed.

First attempt: reverted ecf1327f exactly as the issue says. Gate went green. Correctly so: ecf1327f (#2092) is the commit that caused the regression, so reverting it removes the bug. The fix to revert is 35a24d0c (#2186). The issue's replay instruction should be corrected.

Second attempt: reverted 35a24d0c. Gate went green again — and this one was my bug. #2186's title states the condition precisely: the 500 happened "when a recreate is needed". A fresh agent stopped and started immediately has no config drift, so start_agent_internal takes the plain-start path and the regression is unreachable. The journey had no teeth, and only a real replay could have shown that.

Third: the journey now changes resource limits while the agent is stopped, forcing check_resource_limits_match false and putting the restart on the recreate path. Red, on the right assertion, with the right message.

I would not have found either problem from a green run. Worth keeping in mind for the rest of the deck: a journey that has never been watched failing is a journey of unknown value.

Test Plan

• pytest tests/journeys/ --collect-only → 3 collected
• run_tier journeys 900 journeys/ wired; marker registered in pyproject.toml
• Skip-audit extended with the credential reason, so the one skip is allowlisted rather than silent
• Live green run — this PR's own journey-smoke: 2 passed, 1 skipped in 36.99s, whole job 5m07s of a 30m budget. Both lifecycle journeys created and drove a real agent.
• AC Feature/vector log retention #3 — the gate goes RED on the regression. Proven in a throwaway PR (DO NOT MERGE — regression replay to prove the journey gate goes red (#2336 AC 3) #2404, closed and deleted):

  test_a_stopped_agent_can_be_started_again FAILED
  E  AssertionError: starting stopped agent 'pytest-ephemeral-journey-e15347a9'
     answered 500: {"detail":"Failed to start agent (ValueError …)"}
     — this is the 2026-08-14 regression: five of six stopped agents
       answered 500 here and it reached users
  

• Make it a required check (repo admin) — after the replay proves it

Related to #2335 · Related to #2336 · Related to #1958 (absorbed)

[AndriiPasternak31]

Review sweep — /review + /validate-pr (agent-assisted, findings only)

/review: needs-changes (all mechanical) · /validate-pr: approve with warnings. The gate itself is sound and the proof discipline is exemplary — RED demonstrated against the real 2026-08-14 regression via throwaway #2404 after two documented false-green attempts, and the gate passed on its own PR (5m0s of the 30m budget). Findings:

1. tests/run-full.sh:182-185 — api tier missing --ignore=journeys (found independently by both passes). The api tier's own contract is "everything not owned by a tier above"; journeys now IS a tier above, so every full run executes each journey twice (double live-agent create/stop/recreate/delete churn), and on a stackless run the api tier goes red collaterally because the journey fixture pytest.fails by design. One line.
2. "J03 runs in the nightly" is false in all four places it's claimed (tests/journeys/test_first_turn_journey.py:29-32 docstring, journey-smoke.yml:24-28, tests/harness/audit_skips.py:38-45, and the PR body). The nightly deliberately sets ANTHROPIC_API_KEY: placeholder (integration-nightly.yml:206,218) and the skipif's own == "placeholder" branch matches exactly that value — so the real-output journey skips in the per-PR gate and the nightly, i.e. feat(ci): per-PR journey-smoke gate — first agent turn + start/stop against a prebuilt stack #2336 AC Feature/gemini runtime support #2's real-output half currently has zero automated executions. Fix the four claims, or wire the environment-approval same-repo path the PR itself names.
3. journey-smoke.yml:116 — --timeout=300 is below the tier's own worst-case legitimate wait (~425s): create call ≤180s + running poll ≤90s (fixture) + stop poll 60s + restart poll 90s (body), and pytest-timeout's default covers setup+call+teardown. On a loaded runner the gate red-flakes an unrelated PR with a pytest-timeout stack dump — which also preempts the named-promise failure messages AC Feature/process engine #6 mandates. Suggest --timeout=900 (run-full.sh already gives this tier 900; the 30m job cap still bounds wall clock).

Lower priority: the exit-5 "no journeys collected" branch is dead code under Actions' bash -e (a non-zero pytest exit terminates the step before rc=$? — fail direction preserved, fix with rc=0; pytest … || rc=$?); requests is an undeclared direct dependency of tests/journeys/conftest.py (verbatim the class the same file's idna comment documents — declare it or use the already-declared httpx); the drift-forcing PUT uses absolute values (2g/1) that can equal a stack's fleet defaults and silently de-tooth the recreate leg — read current values and pick a differing one, or assert the drift took; the workflow never runs audit_skips.py, so a future all-skipped run reads green; tests/README.md:63-65 tier list omits journeys.

Process: no closing keyword — Closes #2335 fits (R1 complete; #2336 stays open for the required-check flip, but then someone must bump its status label manually). Post-merge: the required-check flip is the admin step that makes this gate real — and after flipping, never add paths: filters (a path-skipped required check leaves the PR waiting forever).

Merge-order note: land after #2407 and carry the J03 harness: backfill + regenerated JOURNEYS.md in the rebase (details in the comment on #2407). #2395 interaction: none — disjoint workflows, zero file overlap.

[obasilakis]

Requesting changes on two mechanical findings. The design is right and the AC #3 replay story is the most valuable thing in the PR — a journey that has never been watched failing is exactly the trap this deck exists to avoid, and finding it twice before it had teeth is the evidence that matters.

Nothing below is a security or data finding. Both blockers are one-line fixes.

Blocking

1. tests/run-full.sh:180 — the api tier does not ignore journeys/, so the tier runs twice

run_tier api 900 . \
    --ignore=unit --ignore=integration --ignore=git_sync --ignore=security \
    --ignore=scheduler_tests --ignore=agent_server --ignore=manual --ignore=deploy \
    --ignore=harness

Every other tier directory is in that list; journeys is not. So run_tier journeys runs the tier, and then run_tier api collects journeys/ again from .. A full local run creates and tears down every journey agent twice, pays the container time twice, and reports a journey failure under two different tier names.

This is the new-producer-not-in-the-consumer-allowlist class — the ignore list is a hand-maintained mirror of the tier list, and adding a tier above without adding the ignore below is the failure mode it has. Fix is --ignore=journeys.

2. .github/workflows/journey-smoke.yml — the exit-5 guard is dead code

run: |
  set -uo pipefail
  python -m pytest tests/journeys/ -v -rs --tb=short \
    --timeout=300 --timeout-method=thread
  rc=$?
  if [ "$rc" = "5" ]; then

GitHub's default shell for a run: step is bash --noprofile --norc -e -o pipefail {0}. set -uo pipefail adds -u but does not clear the -e that is already active from the shebang args. So a non-zero pytest exit aborts the step at the pytest line: rc=$? never executes, the if never executes, and the ::error title=No journeys collected annotation never appears.

The job still goes red on exit 5 — via -e, not via the code that documents the guarantee. That is the part worth fixing: the #2029 protection this gate is required to provide is currently held by an implicit shell flag, while the code asserting it is unreachable. A future editor who adds continue-on-error or reshapes the step gets no signal.

Fix is either set +euo pipefail-style explicit disabling, or the shape that does not need it:

rc=0
python -m pytest tests/journeys/ ... || rc=$?

Worth noting no other workflow in the repo uses rc=$?, so this pattern is landing here first and would be copied.

Non-blocking

3. tests/journeys/test_agent_lifecycle_journey.py:75 — the drift-forcing values are hardcoded against defaults the test never reads

drift = journey_client.put(
    f"/api/agents/{name}/resources", json={"memory": "2g", "cpu": "1"},
)

This works today only because settings_service.AGENT_DEFAULT_CPU == "2" and AGENT_DEFAULT_MEMORY == "4g", so the PUT genuinely changes something and check_resource_limits_match goes false. Nothing in the test reads either constant, and an operator can move the fleet default through PUT /api/settings/agent-defaults/resources.

Set the fleet default to 2g/1 — or change the constant — and this journey silently loses its teeth again, in exactly the way the PR body describes it losing them the second time. The gate stays green and nobody learns anything.

Cheapest fix that removes the coupling: GET /api/agents/{name}/resources first, then PUT a provably different value, then assert the GET reflects it. That also gives the failure a name ("could not force a recreate") separate from "the restart 500'd".

4. tests/journeys/conftest.py:131 — a partial create leaks an agent

The create's raise AssertionError(...) on a non-2xx sits before the try: whose finally calls delete_agent_idempotent. A create that provisions the ownership row and then fails (a 500 after the row is written) leaves an agent the teardown never reaches, and the tier's own "re-runnable after a crash with no manual cleanup" rule does not hold for that path. Moving the create inside the try costs nothing, since teardown is already idempotent and safe on an agent that never existed.

5. journey-smoke.yml — no paths-ignore

Every PR to dev — including a docs-only or a .claude-only one — boots the full stack and builds the agent base image. That is 5-12 minutes on the measured run, on a check the body intends to make required. A paths-ignore for docs/** and similar would keep the signal and drop the tax.

What I checked and found fine

• journey_client is scope="session" and api_client is session-scoped too (tests/conftest.py:415), so there is no ScopeMismatch.
• PUT /api/agents/{name}/resources exists (routers/agent_config.py:173) and is owner-gated, so the drift step is a real API call and not a fabricated one.
• The workflow is genuinely credential-free, permissions: contents: read, and the throwaway admin password is generated and ::add-mask::ed. A fork PR gets no secret, which is the reason the J03 split exists and the reasoning holds.
• TRINITY_TEST_CLEANUP_SWEEP: '1' is safe here specifically because the sweep refuses a non-localhost target and the instance dies with the runner.
• The EPHEMERAL_PREFIX teardown-by-name rule is honoured, teardown is idempotent, and poll_until is the only synchronisation primitive — the failure messages name the broken promise and the wait, which is the thing that makes these debuggable at 3am.
• The skip audit entry is allowlisted rather than silent, so the one skip is visible.

Verdict

Requesting changes on 1 and 2 only. Neither touches the design, and 3 is the one I would most like taken alongside them, because it is the same failure mode the PR body already paid for twice.

Note on merge order: #2407 (the catalog) records J03's harness path as null specifically because this branch is unmerged, and #2408 is approved on top of both. This should land first.

[dolho]

Both blockers fixed, plus 3 and 4 — a646de8e

1. The api tier collected journeys twice

Fixed, and fixed as the class rather than the instance. You named it precisely — the ignore list is a hand-maintained mirror of the tier list — so adding --ignore=journeys would have left the next tier to repeat it.

The list is now derived from TIER_DIRS + NON_TIER_DIRS, and checked against the tree: a directory under tests/ in neither set fails the script by name instead of being swept into api. Both halves verified:

IGNORES: --ignore=unit … --ignore=journeys --ignore=manual --ignore=deploy --ignore=harness …

$ mkdir tests/brand_new_tier && bash <fragment>
== tests/run-full.sh: unclassified test directories
   brand_new_tier
   Add each to TIER_DIRS (with its own run_tier line) or to NON_TIER_DIRS.
exit=1

2. The exit-5 guard was unreachable

Confirmed exactly as you described, and simulated under GitHub's real shell flags (bash --noprofile --norc -e -o pipefail):

shape	pytest exits	step exit	annotation
old (rc=$?)	5	5	never printed
new (|| rc=$?)	5	1	printed
new	1	1	correctly absent
new	0	0	—

I took || rc=$? over set +e for the reason you implied: a command in a condition is not subject to -e, so the exit code is captured by construction rather than by remembering to disable a flag. The comment records that this is the first rc= pattern in the repo and why it has this shape, since you noted it would be copied.

3. The drift step no longer depends on constants it never reads

Taken alongside, as you asked. It GETs the current resources, picks a provably different pair, and then asserts the change took — so "could not force a recreate" fails under its own name rather than surfacing later as "the restart 500'd", or as a green run that quietly exercised the plain-start path. That was the third time this journey's teeth depended on a coincidence; it no longer does.

4. Partial create no longer leaks

Create moved inside the try. delete_agent_idempotent was already safe on an agent that never existed, so it costs nothing and restores the tier's re-runnable-after-a-crash rule on the path where cleanup matters most.

5. paths-ignore — deliberately not added, and I'd like your view

The tax is real and I'd normally just add it. I did not, because the obvious fix is a trap here: a filtered-out PR runs no workflow, so the check is never reported — and for a check this PR intends to make required, that is not "skipped", it is a PR that can never merge. That exact failure was diagnosed on #2384 earlier today: three required workflows produced no run and the PR sat green-but-unmergeable until someone pushed again.

The safe shape is the one frontend-e2e.yml already uses — unconditional trigger, cheap changes job, if:-gated heavy job, failing open so "cannot determine" means run rather than skip (#1782). That is ~50 lines with subtle semantics, and that file says of its own sibling implementation that the two "differ ON PURPOSE. Don't 'unify' them" — so it wants its own deliberate change, not a rushed third copy appended here.

I've recorded the hazard as a comment on the on: block so the next person doesn't add the trap. Happy to do it properly as a follow-up, or in this PR if you'd rather it land together.

[dolho]

Ready for re-review — the requested changes are pushed

Flagging state rather than adding findings: the CHANGES_REQUESTED on this PR predates the work that answers it. The review landed 08:39–08:41 UTC; the fix commit is 12:24–12:33 UTC. GitHub does not clear the label on a push, so it reads as outstanding when it is not.

What landed: both blockers — the api tier's ignore list is now derived and tree-checked (an unwired directory fails by name), and the exit-5 guard is reachable, verified under GitHub's real shell flags. Plus findings 3 and 4. Finding 5 (paths-ignore) is deliberately not done and I left the reasoning inline: on a check intended to be required, a filtered-out PR reports nothing and can never merge — the #2384 failure. Happy to do it properly either way; that one wants your call.

The fuller response is in the comment above this one. Nothing further is pending from my side.

(Disclosure: I wrote the fix commit on this PR, so this note is a status flag — not a self-review and not an approval.)

[obasilakis]

/validate-pr re-review — approving. Clearing my earlier CHANGES_REQUESTED.

I re-derived all four findings against the current branch source rather than reading the commit subjects. All four are genuinely fixed, and two of them are fixed better than I asked for.

1. The api tier collecting journeys/ twice — fixed structurally

I asked for --ignore=journeys. What landed is:

api_ignores=()
for _d in "${TIER_DIRS[@]}" "${NON_TIER_DIRS[@]}"; do api_ignores+=("--ignore=${_d}"); done
run_tier api 900 . "${api_ignores[@]}"

That is the better fix. The finding was the new-producer-not-in-the-consumer-allowlist class — a hand-maintained mirror of the tier list — and deriving the ignores from the tier list removes the class rather than the instance. The next tier added cannot reintroduce it.

2. The dead exit-5 guard — fixed, with the reasoning kept

|| rc=$? is there, and the comment explains why it is chosen over set +e (the command becomes part of a condition, which -e does not apply to, so the exit code is captured by construction rather than by remembering to disable a flag). That matters because this pattern is landing in the repo first and will be copied.

3. The hardcoded drift values — fixed, and it now fails with its own name

The journey reads GET /api/agents/{name}/resources, derives a provably different value, PUTs it, and then asserts the change took before starting the agent. The coupling to AGENT_DEFAULT_CPU/AGENT_DEFAULT_MEMORY is gone, so an operator moving the fleet default cannot silently remove this journey's teeth for a third time. The read-back assertion is what I was really after — "could not force a recreate" now fails separately from "the restart 500'd", instead of surfacing as a green run that quietly exercised the plain-start path.

4. The partial-create leak — fixed

The POST /api/agents is inside the try, so the finally reaches an agent whose ownership row was written by a create that then failed or timed out. That was the case where the tier's own re-runnable-after-a-crash rule mattered most.

---

Checklist: base dev, both referenced issues resolve and carry correct labels, security scan clean, the new os.getenv() vars are all under tests/ so the compose-packaging rule does not apply, no schema or backend-module surface. CI green, including the journey-smoke job running against itself.

Two notes for after merge, neither blocking:

The paths-ignore decision is right and the reasoning should survive. No paths-ignore, so every PR to dev pays a 5–12 minute stack boot including docs-only ones. The comment explains why the obvious fix is the wrong one — a filtered-out workflow never reports, and for a required check "never reported" is a PR that can never merge, which is exactly what happened on #2384 on 2026-08-27. Deferring the changes-job version to its own change rather than appending a rushed third copy is the correct call given frontend-e2e.yml says of its own sibling that the two differ on purpose. Worth doing before this becomes a required check, though, or the docs-only tax lands on everyone at once.

No credentials in the job, deliberately. pull_request exposes repository secrets to fork PRs while running the PR's own start.sh, so the credential-bound J03 first-turn journey correctly stays on a developer's stack and in the nightly. The lifecycle journey is where the 2026-08-14 regression actually was, so the split loses nothing that matters here.