Add AGENTS.md, CLAUDE.md, and PR browser smoke test - #5947
Conversation
12b3284 to
c62e4c6
Compare
🔵 Review postedLast updated: 2026-07-17 19:04 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #5947 adds agent guidance docs (AGENTS.md/CLAUDE.md) plus a WhiteNoise-wrapped, prod-shaped browser smoke test and its CI job. Changes are additive and CI-only; spot-checks of the executable claims held up (custom create_superuser signature and is_active=False flip verified against models.py). No blocking issues — the findings are hardening suggestions on the smoke test's failure sensitivity.
- suggestion —
smoke_test.py:129:status >= 400fails on any same-origin 4xx, including benign ones (favicon, empty-state/unauth probes). See inline. - suggestion —
smoke_test.py:195:walk_pageshas no positive assertion that the SPA mounted — a blank-white mount passes. See inline. - suggestion —
deploytest.yml:160:whitenoiseinstalled unpinned. See inline. - nitpick —
smoke_test.py:173,201:networkidlewaits depend on the 5s sync poller staying quiet; if a walked page enqueues a change this can time out.domcontentloaded+ an explicit content assertion would be more robust. Not observed in CI — noting as the first thing to check if this job ever goes flaky.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| ) | ||
|
|
||
| def on_response(resp): | ||
| if _same_host(resp.url) and resp.status >= 400: |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: This records every same-host response with status >= 400 as a failure, coupling pass/fail to every subresource any walked page touches — a missing /favicon.ico or an endpoint that legitimately returns 404/403 for an empty-state or unauthenticated probe would turn the smoke test red for reasons unrelated to a broken build. CI is green today, but as routes evolve this is a likely source of confusing false failures. Consider narrowing to >= 500 plus an explicit allowlist for known-benign 4xx, or scoping the check to document/navigation responses rather than every subresource.
| return f"{base}#{frag}" if frag else base | ||
|
|
||
|
|
||
| def walk_pages(page, buffer, failures): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: walk_pages only navigates, waits for networkidle, screenshots, and drains error collectors — a page that mounts to a blank white screen with no console error and no failed request passes. Given the stated goal (catch broken SPA mount over broken route logic), add a minimal positive check per page — e.g. page.locator("#app, [data-app]").wait_for(state="visible") or asserting a non-empty body — so a silently-blank mount is caught rather than screenshotted-and-passed.
| # WhiteNoise lets gunicorn serve /static/ without nginx (see | ||
| # integration_testing/smoke_wsgi.py). Smoke-test-only, so not in | ||
| # requirements.txt. | ||
| uv pip install whitenoise |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Everything else in this job flows through pinned uv pip sync requirements.txt, but WhiteNoise is installed unpinned. A future major that changes the WhiteNoise(application, root=..., prefix=...) constructor used in smoke_wsgi.py would break CI with no lockfile change to point at. Consider a loose upper bound (e.g. whitenoise<7), mirroring how smoke_test.py pins playwright<2.
cb21381 to
30de4c1
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #5947 — all four prior findings resolved, but the PR's own Browser smoke test job is now failing on every run (4/4). The frontend bundle round-trips to the wrong path, so every page 500s and the smoke test times out on the login form. One blocking fix — see inline.
CI: failing (the new smoke-test job itself). A smoke-test PR whose smoke test is red can't land.
Prior-finding status
RESOLVED — integration_testing/smoke_test.py:129 — status>=400 fails on benign 4xx
RESOLVED — integration_testing/smoke_test.py:207 — walk_pages has no positive SPA-mount assertion
RESOLVED — .github/workflows/deploytest.yml:160 — whitenoise installed unpinned
ACKNOWLEDGED — integration_testing/smoke_test.py:173,201 — networkidle waits (nitpick; backstopped by new content assertion)
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| - name: Download frontend bundle | ||
| uses: actions/download-artifact@v8 | ||
| with: | ||
| name: studio-frontend-bundle |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Downloaded bundle lands one directory too shallow → 500 on every page, smoke test times out. upload-artifact strips the least-common-ancestor (contentcuration/), so the artifact holds build/webpack-stats.json + contentcuration/static/studio/…. This download has no path:, so it extracts into the workspace root: webpack-stats.json ends up at ./build/… but WEBPACK_LOADER["STATS_FILE"] resolves to <repo>/contentcuration/build/webpack-stats.json, and the static bundle lands at ./contentcuration/static/studio/ instead of the contentcuration/contentcuration/static/studio/ dir collectstatic reads. gunicorn logs OSError: Error reading .../contentcuration/build/webpack-stats.json.
Fix — extract under the stripped prefix to restore the original layout:
- name: Download frontend bundle
uses: actions/download-artifact@v8
with:
name: studio-frontend-bundle
path: contentcurationVerify the job goes green (and that collectstatic then finds the bundle) before merge.
30de4c1 to
2c414dd
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #5947 — all 5 prior findings resolved; no new findings. The blocking bundle-path fix (deploytest.yml:168, path: contentcuration) is verified and the Browser smoke test job is green (2m14s). Non-blocking CI note: Python unit tests is still pending (unrelated to this delta).
Prior-finding status
RESOLVED — integration_testing/smoke_test.py:138 — status>=400 fails on benign 4xx
RESOLVED — integration_testing/smoke_test.py:235 — walk_pages has no positive SPA-mount assertion
RESOLVED — .github/workflows/deploytest.yml:159 — whitenoise installed unpinned
RESOLVED — .github/workflows/deploytest.yml:168 — downloaded bundle landed one directory too shallow → 500 on every page
ACKNOWLEDGED — integration_testing/smoke_test.py:187,215 — networkidle waits (nitpick; backstopped by body-text assertion)
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
|
|
||
| ### ⚠️ Use Vuetify's Grid for Responsive Layout | ||
|
|
||
| `v-container` / `v-row` / `v-col` with Vuetify breakpoints (`xs`/`sm`/`md`/`lg`/`xl`). Studio does not have a `responsive-window` equivalent. |
There was a problem hiding this comment.
This might be the best approach for right now, but I feel as though this is a bit at odds with the "no vuetify" guidance (for both people and LLMs)
There was a problem hiding this comment.
Yes, happy to change it, just wasn't sure what to suggest.
There was a problem hiding this comment.
maybe, thinking about the css grid conversation we have been talking about, try usingKResponsiveWindow and/or plain <div>s where possible? but otherwise if working within existing components/pages, you do not have to refactor them, and if only v-container makes it work, that is okay.
|
|
||
| ## Docs Reference | ||
|
|
||
| The Kolibri-dev docs site serves an LLM-friendly Markdown variant of every page — append `.md` to any URL below. The whole index is at https://kolibri-dev.readthedocs.io/en/latest/llms.txt. |
There was a problem hiding this comment.
I think the kolibri docs here are generally applicable but confirming that this is an intentional repurposing and not copypasta
There was a problem hiding this comment.
Yes, purely because it contains lots of general info.
Adds AGENTS.md + CLAUDE.md at the repo root with Studio-specific guidance for AI coding agents, and a PR-time browser smoke test that boots gunicorn behind nginx, logs in, and walks a curated set of in-app URLs collecting console errors and same-origin HTTP failures. The smoke test job consumes a built frontend artifact from the existing build_assets job (upload step added), mirroring Kolibri's whl -> browser_smoke_test pattern. nginx fronts gunicorn so the request path matches prod.
2c414dd to
c301634
Compare
3 participants
Summary
AGENTS.md+CLAUDE.mdwith Studio-specific guidance for AI coding agents (KDS-not-Vuetify, Composition-API-for-new-state,ValuesViewset, sync framework).build_assets, runs gunicorn with WhiteNoise serving/static/(CI-only; prod fronts gunicorn with nginx), drives Playwright through login + a curated set of in-app URLs collecting JS errors and same-origin HTTP failures.References
learningequality/kolibri(itsdevelopbranch).learningequality/kolibri-data-portal.Reviewer guidance
Browser smoke testjob on this PR — it should pass and upload asmoke_test_screenshotsartifact (00_login.png,01_post_login.png, and 8 walked pages).AI usage
I used Claude Code to draft the AGENTS.md / CLAUDE.md content (forking Kolibri's equivalents and adapting to Studio) and to write the smoke-test script + CI job. I drove the brainstorm and the spec, reviewed every section before commit, and ran the smoke test end-to-end locally against gunicorn + WhiteNoise before push.