Add BGSTM external-results smoke workflow pinned to reporter SHA ab5d7c1 with main-branch, audit-log, and artifact-path compatibility hardening - #314
Conversation
Agent-Logs-Url: https://github.com/bg-playground/BGSTM/sessions/1b41d391-8b03-4161-9c69-e2a89debf6af Co-authored-by: bg-playground <259109604+bg-playground@users.noreply.github.com>
Agent-Logs-Url: https://github.com/bg-playground/BGSTM/sessions/1b41d391-8b03-4161-9c69-e2a89debf6af Co-authored-by: bg-playground <259109604+bg-playground@users.noreply.github.com>
Agent-Logs-Url: https://github.com/bg-playground/BGSTM/sessions/1b41d391-8b03-4161-9c69-e2a89debf6af Co-authored-by: bg-playground <259109604+bg-playground@users.noreply.github.com>
Agent-Logs-Url: https://github.com/bg-playground/BGSTM/sessions/1b41d391-8b03-4161-9c69-e2a89debf6af Co-authored-by: bg-playground <259109604+bg-playground@users.noreply.github.com>
|
@copilot please address the review feedback below: The pin SHA, scope hygiene, regression test coverage, spec doc bump, and 1 (blocker) — Bootstrap crashes with 500 on
|
bg-playground
left a comment
There was a problem hiding this comment.
@copilot please address the review feedback below:
The pin SHA, scope hygiene, regression test coverage, spec doc bump, and if: always() cleanup all look right. The smoke run surfaced two real issues plus one polish item.
1 (blocker) — Bootstrap crashes with 500 on POST /auth/runner-tokens
Smoke job log (commit 3257dee):
Project creation endpoint not available; using generated project_id for external-results smoke run.
Traceback (most recent call last):
File ".../scripts/smoke/bootstrap.py", line 70, in <module>
main()
File ".../scripts/smoke/bootstrap.py", line 57, in main
runner_token = _api(client, "POST", "/api/v1/auth/runner-tokens", ...)
File ".../scripts/smoke/bootstrap.py", line 13, in _api
response.raise_for_status()
httpx.HTTPStatusError: Server error '500 Internal Server Error' for url 'http://localhost:8001/api/v1/auth/runner-tokens'
Login succeeded (JWT was captured). The project fallback fired as designed. The 500 happens inside the backend on runner-token issuance — not surfaceable from the bootstrap script.
Diagnostic step required
Add a workflow step that dumps backend logs when bootstrap fails, so the next run gives us the actual stack trace:
- name: Bootstrap project and runner token
id: bootstrap
run: python bgstm/scripts/smoke/bootstrap.py
- name: Dump backend logs on bootstrap failure
if: failure() && steps.bootstrap.outcome == 'failure'
run: docker compose -f bgstm/docker-compose.test.yml logs backendAdd the same if: failure() log dump after the assertion step too — same reasoning.
Most likely root cause
POST /auth/runner-tokens writes both a runner_tokens row (added by #296) and an audit-log entry via create_audit_entry (the shim added by #297). Two concrete hypotheses:
docker-compose.test.ymlstack doesn't run Alembic migrations. If the test backend only runsinit_db()(SQLite-style auto-create) and skipsalembic upgrade head, then the test Postgres is missing post-#296/#297 schema (norunner_tokenstable, noactor_kind/actor_token_idcolumns onaudit_log). Worth checkingbackend/entrypoint.sh— does it runalembic upgrade headbefore uvicorn?- Audit shim CHECK-constraint violation. #297 added a CHECK (
exactly one of user_id / actor_token_id must be non-null, matching actor_kind). If the shim path fromcreate_audit_entrydoesn't passactor_kind="user"correctly, or the migration's server default didn't propagate to the test DB, the constraint trips on insert.
Once we have the backend stack trace from the log-dump step, the fix path will be obvious. My money is on #1 — adding alembic upgrade head to the test stack startup is also a hardening win regardless.
2 (please add a comment) — /api/v1/projects 404 fallback
bootstrap.py::_get_or_generate_project_id falls back to a random UUID when POST /api/v1/projects returns 404. There is no projects API on main, so this fallback fires every run. The smoke is therefore validating "session row was created with the project_id we passed in" rather than "session belongs to a real project." Acceptable for v0.1, but please add a comment explaining the fallback so the next person doesn't think the 404 path is a bug:
def _get_or_generate_project_id(client: httpx.Client, headers: dict[str, str]) -> str:
# NOTE: /api/v1/projects does not exist on main as of v0.1. The 404 fallback
# is intentional and will trigger every run. Tracked as v0.2 follow-up #315.
# The synthetic UUID is sufficient because the session endpoint does not
# currently FK-validate project_id.
response = client.post("/api/v1/projects", headers=headers, json={"name": "smoke-project"})
...3 (downstream of #1) — Assertion script calls GET /external-results/case/{id}, which doesn't exist on main
Once #1 is fixed and bootstrap clears, the assertion phase will likely 404 on _fetch_snapshot's per-case fetch loop. There is no GET /external-results/case/{id} endpoint on main; only GET /session/{id} exists.
Easiest fix: reconstruct case rows from audit-log details rather than fetching them. The external_results.case.create audit entry already captures enough to validate the contract (resource_id → case id, plus whatever's in details). This matches how artifacts are already validated (audit-log only, no fetch). If audit details for case.create doesn't include external_id and outcome directly, fall back to a docker compose exec db psql direct-DB read in the assert step — keeps the change script-local without widening the v0.1 contract surface.
Don't add a new GET /case/{id} endpoint here — that's a v0.2 surface conversation if we want it.
4 (minor, optional) — restore the four-step reporter call sequence in the spec doc
The previous version of docs/specs/external_results_v1.md's "Reference implementation" section had a numbered list of the four reporter calls (POST /session → POST /case × N → POST /artifact on failure → PATCH /session/{id}). The new version drops it. Up to you whether to restore — not a merge blocker.
Suggested order for the agent
- Add the
if: failure()log-dump steps (item #1, diagnostic infra). - Push and let CI re-run. Share the backend log here.
- From the stack trace, fix the root cause (likely Alembic migrations on the test stack, or audit shim).
- Switch
assert.pyto audit-only case validation (item #3). - Add the bootstrap fallback comment (item #2, references #315).
Don't do these all in one push — items 1+2 first to get visibility, then 3, then 4+5. We've been bitten too many times in this chain by "fix multiple things at once and one fix masks another."
Thanks!
Agent-Logs-Url: https://github.com/bg-playground/BGSTM/sessions/1123ad8f-4de3-4e0a-bcb3-d52176bfe2f6 Co-authored-by: bg-playground <259109604+bg-playground@users.noreply.github.com>
Addressed in I reproduced the 500 and confirmed it was caused by a Postgres schema mismatch ( |
Agent-Logs-Url: https://github.com/bg-playground/BGSTM/sessions/3ef84186-557c-465a-ada3-1b031de06c56 Co-authored-by: bg-playground <259109604+bg-playground@users.noreply.github.com>
Agent-Logs-Url: https://github.com/bg-playground/BGSTM/sessions/3ef84186-557c-465a-ada3-1b031de06c56 Co-authored-by: bg-playground <259109604+bg-playground@users.noreply.github.com>
Agent-Logs-Url: https://github.com/bg-playground/BGSTM/sessions/3ef84186-557c-465a-ada3-1b031de06c56 Co-authored-by: bg-playground <259109604+bg-playground@users.noreply.github.com>
Implemented in
Validation run:
|
bg-playground
left a comment
There was a problem hiding this comment.
The pin SHA, scope hygiene, regression test coverage, spec doc bump, and if: always() cleanup all look right. The smoke run surfaced two real issues plus one polish item.
1 (blocker) — Bootstrap crashes with 500 on POST /auth/runner-tokens
Smoke job log (commit 3257dee):
Project creation endpoint not available; using generated project_id for external-results smoke run.
Traceback (most recent call last):
File ".../scripts/smoke/bootstrap.py", line 70, in <module>
main()
File ".../scripts/smoke/bootstrap.py", line 57, in main
runner_token = _api(client, "POST", "/api/v1/auth/runner-tokens", ...)
File ".../scripts/smoke/bootstrap.py", line 13, in _api
response.raise_for_status()
httpx.HTTPStatusError: Server error '500 Internal Server Error' for url 'http://localhost:8001/api/v1/auth/runner-tokens'
Login succeeded (JWT was captured). The project fallback fired as designed. The 500 happens inside the backend on runner-token issuance — not surfaceable from the bootstrap script.
Diagnostic step required
Add a workflow step that dumps backend logs when bootstrap fails, so the next run gives us the actual stack trace:
- name: Bootstrap project and runner token
id: bootstrap
run: python bgstm/scripts/smoke/bootstrap.py
- name: Dump backend logs on bootstrap failure
if: failure() && steps.bootstrap.outcome == 'failure'
run: docker compose -f bgstm/docker-compose.test.yml logs backendAdd the same if: failure() log dump after the assertion step too — same reasoning.
Most likely root cause
POST /auth/runner-tokens writes both a runner_tokens row (added by #296) and an audit-log entry via create_audit_entry (the shim added by #297). Two concrete hypotheses:
docker-compose.test.ymlstack doesn't run Alembic migrations. If the test backend only runsinit_db()(SQLite-style auto-create) and skipsalembic upgrade head, then the test Postgres is missing post-#296/#297 schema (norunner_tokenstable, noactor_kind/actor_token_idcolumns onaudit_log). Worth checkingbackend/entrypoint.sh— does it runalembic upgrade headbefore uvicorn?- Audit shim CHECK-constraint violation. #297 added a CHECK (
exactly one of user_id / actor_token_id must be non-null, matching actor_kind). If the shim path fromcreate_audit_entrydoesn't passactor_kind="user"correctly, or the migration's server default didn't propagate to the test DB, the constraint trips on insert.
Once we have the backend stack trace from the log-dump step, the fix path will be obvious. My money is on #1 — adding alembic upgrade head to the test stack startup is also a hardening win regardless.
2 (please add a comment) — /api/v1/projects 404 fallback
bootstrap.py::_get_or_generate_project_id falls back to a random UUID when POST /api/v1/projects returns 404. There is no projects API on main, so this fallback fires every run. The smoke is therefore validating "session row was created with the project_id we passed in" rather than "session belongs to a real project." Acceptable for v0.1, but please add a comment explaining the fallback so the next person doesn't think the 404 path is a bug:
def _get_or_generate_project_id(client: httpx.Client, headers: dict[str, str]) -> str:
# NOTE: /api/v1/projects does not exist on main as of v0.1. The 404 fallback
# is intentional and will trigger every run. Tracked as v0.2 follow-up #315.
# The synthetic UUID is sufficient because the session endpoint does not
# currently FK-validate project_id.
response = client.post("/api/v1/projects", headers=headers, json={"name": "smoke-project"})
...3 (downstream of #1) — Assertion script calls GET /external-results/case/{id}, which doesn't exist on main
Once #1 is fixed and bootstrap clears, the assertion phase will likely 404 on _fetch_snapshot's per-case fetch loop. There is no GET /external-results/case/{id} endpoint on main; only GET /session/{id} exists.
Easiest fix: reconstruct case rows from audit-log details rather than fetching them. The external_results.case.create audit entry already captures enough to validate the contract (resource_id → case id, plus whatever's in details). This matches how artifacts are already validated (audit-log only, no fetch). If audit details for case.create doesn't include external_id and outcome directly, fall back to a docker compose exec db psql direct-DB read in the assert step — keeps the change script-local without widening the v0.1 contract surface.
Don't add a new GET /case/{id} endpoint here — that's a v0.2 surface conversation if we want it.
4 (minor, optional) — restore the four-step reporter call sequence in the spec doc
The previous version of docs/specs/external_results_v1.md's "Reference implementation" section had a numbered list of the four reporter calls (POST /session → POST /case × N → POST /artifact on failure → PATCH /session/{id}). The new version drops it. Up to you whether to restore — not a merge blocker.
Suggested order for the agent
- Add the
if: failure()log-dump steps (item #1, diagnostic infra). - Push and let CI re-run. Share the backend log here.
- From the stack trace, fix the root cause (likely Alembic migrations on the test stack, or audit shim).
- Switch
assert.pyto audit-only case validation (item #3). - Add the bootstrap fallback comment (item #2, references #315).
Don't do these all in one push — items 1+2 first to get visibility, then 3, then 4+5. We've been bitten too many times in this chain by "fix multiple things at once and one fix masks another."
Thanks!
|
@copilot please address the review feedback in the review comment |
Agent-Logs-Url: https://github.com/bg-playground/BGSTM/sessions/5c293b39-4619-4432-b5ac-5f928ca1b000 Co-authored-by: bg-playground <259109604+bg-playground@users.noreply.github.com>
Addressed in I bumped the smoke frameworks pin to Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
Marking this PR as blocked. New finding from the latest run: the case-result and artifact endpoints don't exist on EvidenceBackend log from this run: Confirmed by """API router for External Results — session endpoints (BGSTM#300).
Implements:
POST /external-results/session – start a run (201 Created)
PATCH /external-results/session/{id} – finish a run
GET /external-results/session/{id} – read a session
Case-result endpoints → BGSTM#303 ← still open
Artifact endpoints → BGSTM#298 ← still open
Audit-log integration → BGSTM#297 ← merged
"""Both #303 and #298 are Why we can't ship this PR with further relaxationEven if we relax the smoke to "session-only," the gate becomes a permanent placeholder that can't validate the actual v0.1 contract. We've been bitten by this exact pattern earlier in the chain (mocks/fixtures masking schema drift in PR #7). Putting a weak gate into PlanHold this PR until #303 and #298 are merged, then come back. Order:
What's already useful about the work in this PR
Action right now
Tracking the dependency:
Will revisit once #303 and #298 are in. Thanks for the diagnostic infrastructure on this round — it's what surfaced the real gap. |
bg-playground
left a comment
There was a problem hiding this comment.
Marking this PR as blocked. New finding from the latest run: the case-result and artifact endpoints don't exist on main at all, so the smoke gate can't validate the External Results contract no matter how we tune the assertions.
Evidence
Backend log from this run:
POST /api/v1/external-results/session 201 Created ← works
POST /api/v1/external-results/case 404 Not Found ← endpoint not implemented
Confirmed by backend/app/api/external_results.py on main:
"""API router for External Results — session endpoints (BGSTM#300).
Implements:
POST /external-results/session – start a run (201 Created)
PATCH /external-results/session/{id} – finish a run
GET /external-results/session/{id} – read a session
Case-result endpoints → BGSTM#303 ← still open
Artifact endpoints → BGSTM#298 ← still open
Audit-log integration → BGSTM#297 ← merged
"""Both #303 and #298 are state: open. The reporter (correctly fixed in bgstm-playwright-frameworks PR #8 → merge SHA ab5d7c1) is making the right calls; BGSTM 404s on the second one and the rest of the lifecycle never happens. That's why every relaxation we've shipped on this PR has only revealed a deeper layer of the same problem.
Why we can't ship this PR with further relaxation
Even if we relax the smoke to "session-only," the gate becomes a permanent placeholder that can't validate the actual v0.1 contract. We've been bitten by this exact pattern earlier in the chain (mocks/fixtures masking schema drift in PR #7). Putting a weak gate into main invites the same class of regressions while the real endpoints are being built.
Plan
Hold this PR until #303 and #298 are merged, then come back. Order:
- #303 (case-result endpoints + auto-upsert + traceability auto-link) — dispatching now.
- #298 (artifacts + pluggable storage) — dispatch after #303 merges (artifact rows FK to case results, and we want to learn from #303 review before opening #298).
- Return to this PR.
- Revert the assertion relaxations from rounds 2 and 3 — restore exact-count assertions on case results, artifacts, audit entries.
- Bump the frameworks pin if any reporter drift since
ab5d7c1. (Likely none — reporter is feature-complete for v0.1 modulo annotations, which #316 covers.) - Re-run the smoke; the full v0.1 assertion table should pass without further loosening.
What's already useful about the work in this PR
- The diagnostic infrastructure (
if: failure()log dump, schema-normalize step) — keep. - The bootstrap flow (login → runner token issue) — keep.
- The pinned frameworks checkout pattern — keep.
- The pluggable assertion script structure — keep, but assertions get re-tightened post-#303/#298.
Action right now
- This PR stays in draft. No more pushes against it until #303 and #298 are merged.
- Don't merge the relaxed assertions; we don't want them on
maineven temporarily. - Re-targeting this PR after #303/#298 merge will likely require a rebase, not a fresh PR.
Tracking the dependency:
- depends on: #303 (case-result endpoints)
- depends on: #298 (artifacts + storage)
- already follows: #316 (v0.2 reporter annotation gap, separate)
- already follows: #315 (v0.2 projects API, separate)
Will revisit once #303 and #298 are in. Thanks for the diagnostic infrastructure on this round — it's what surfaced the real gap.
|
@copilot 1. Rebase onto
|
…-smoke-workflow Co-authored-by: bg-playground <259109604+bg-playground@users.noreply.github.com>
Agent-Logs-Url: https://github.com/bg-playground/BGSTM/sessions/83452736-cfa9-4dfc-94bc-eec486a77274 Co-authored-by: bg-playground <259109604+bg-playground@users.noreply.github.com>
... Addressed in I updated this branch with current Latest pushed SHA is |
|
@copilot Both migrations point at the same
Pre-rebase, Please change -down_revision: Union[str, None] = "i8j9k0l1m2n3"
+down_revision: Union[str, None] = "l1m2n3o4p5q6"That linearizes the graph to No other changes needed — the |
Agent-Logs-Url: https://github.com/bg-playground/BGSTM/sessions/01aa7118-af85-41d6-b9b4-d874ac8f6a75 Co-authored-by: bg-playground <259109604+bg-playground@users.noreply.github.com>
Agent-Logs-Url: https://github.com/bg-playground/BGSTM/sessions/01aa7118-af85-41d6-b9b4-d874ac8f6a75 Co-authored-by: bg-playground <259109604+bg-playground@users.noreply.github.com>
... Fixed in I added an Alembic merge revision ( Validation locally now shows a single head: UI screenshot: N/A (no UI changes in this fix). |
|
LGTM ✅ — smoke is green and the contract is locked end-to-end. Nice call on the merge migration ( Verified on
Ready to merge once you drop draft. Closes #295. 🚀 |
Original prompt
Add the BGSTM-side end-to-end smoke workflow that exercises the External Results v1 contract against a real
BGSTMReporterfrombgstm-playwright-frameworks. Closes BGSTM#295 (parent: #291).This is step 2 of the cross-repo wrap-up. Step 1 (the reporter half) merged to
bgstm-playwright-frameworks/mainas commit98027ad2126cde2a87392564828f10709612142e— pin to that SHA exactly. Do not chasemainof the frameworks repo.This is the closer for v0.1 of the External Results contract chain (#299 spec, #296 auth, #300 sessions, #303 case results, #298 artifacts, #297 audit are all merged). After this lands, the chain is done.
What "smoke" means here
A new GitHub Actions workflow that:
bgstm-playwright-frameworksat the pinned commit, installs deps, installs Chromiumpnpm --filter crm-example smokeagainst the live BGSTM instance with a runner tokentolerateOfflinehereThe smoke fixture in step 1 has
tolerateOffline: falseset explicitly, so any reporter network error already fails the Playwright run. The BGSTM-side assertions in this workflow catch the inverse: cases where the reporter ran "successfully" but BGSTM didn't actually persist the expected rows.Files to add/modify
Add —
.github/workflows/external-results-smoke.ymlA new top-level workflow. Do NOT modify any existing workflow (
ci.yml,e2e-tests.yml, etc.) — this is a separate, additive file. Mirror the shape ofe2e-tests.ymlfor consistency (service containers, healthchecks, etc.) but keep it focused on the smoke scenario.Triggers:
pull_requestpaths-filtered to:backend/app/api/external_results.py,backend/app/schemas/external_*.py,backend/app/models/external_*.py,backend/app/crud/external_*.py,backend/alembic/versions/**,docs/specs/external_results_v1.md,.github/workflows/external-results-smoke.ymlitselfpushtomain(full coverage on every merge)workflow_dispatch(manual re-runs)Job:
smokeonubuntu-latest, single job, single matrix entry. Total runtime budget: < 3 minutes (per #295's acceptance criteria).Required steps in order:
Checkout BGSTM (
actions/checkout@v4withpath: bgstm)Checkout pinned
bgstm-playwright-frameworksat the merge SHA above:Set up Python 3.11 via
actions/setup-python@v5Set up Node 20 via
actions/setup-node@v4Set up pnpm 9 via
pnpm/action-setup@v4Boot BGSTM stack. Two acceptable approaches — pick whichever yields a cleaner diff:
docker-compose.test.ymlfrom the BGSTM checkout (recommended — already exposes backend on port 8001 withadmin@test.com/password123). Rundocker compose -f docker-compose.test.yml up -d backend db. Wait for/healthto return 200. Skip thefrontendservice — not needed for smoke.docker-compose.smoke.ymlif (a) has constraints (e.g. seed.sql conflicts, frontend coupling).Default to (a) unless you find a real blocker. Document the choice in the PR description.
Bootstrap project + runner token. Add a small helper script
scripts/smoke/bootstrap.sh(bash, ~40 lines) orscripts/smoke/bootstrap.py(Python, more readable) that:POST /api/v1/auth/loginwithadmin@test.com/password123→ capture JWTPOST /api/v1/projects(or whatever the projects-create endpoint is onmain— discover frombackend/app/api/) with name"smoke-project"→ captureproject_idPOST /api/v1/auth/runner-tokens(admin JWT) with{"label":"smoke","scopes":["external_results:write","external_results:read"]}→ capture plaintext token$GITHUB_ENV:BGSTM_API_URL=http://localhost:8001,BGSTM_API_TOKEN=…,BGSTM_PROJECT_ID=…,BGSTM_ADMIN_JWT=…(the JWT is needed for the assertion phase)Important — note the
readscope on the runner token. The reporter only needs:write, but reusing the same token for the assertion phase below requires:readtoo. If the spec disallows multi-scope tokens (verify), use a separate admin JWT for assertions and only:writefor the reporter.Install frameworks deps:
pnpm -C frameworks install --frozen-lockfile,pnpm -C frameworks build,pnpm -C frameworks/examples/crm-example exec playwright install --with-deps chromiumRun the smoke fixture: `pnpm --filter crm-ex...
This pull request was created from Copilot chat.