[agent] Add template text smoke checks for public query notices (#1111)#1191
[agent] Add template text smoke checks for public query notices (#1111)#1191yanyishuai wants to merge 1 commit into
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds a template text smoke script for public Jinja templates and rendered pages, updates public search notice text to ASCII quotes, aligns tests with the new text, and wires the check into CI and documentation. ChangesTemplate text smoke check
Related Issues: Suggested Labels: testing, ci, documentation, templates Suggested Reviewers: none identified 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
Requesting changes on the current head. The admin runbook additions introduce commands for scripts/check_public_mrwk_links.py and scripts/flag_superseded_review_rounds.py, but this PR does not add those scripts and they are not present on main at the PR base. Since the branch targets main directly, merging it by itself would publish runbook commands that fail with missing files. Please either remove the unrelated runbook sections from this PR or include/land the referenced scripts before documenting them here.
yanyishuai
force-pushed
the
fix/issue-1111-template-text-smoke
branch
from
5747b51 to
def157f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_template_text_smoke.py (1)
1-70: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing test coverage for mojibake/replacement-character detection.
The PR objectives call for "coverage for mojibake/replacement-character cases," but no test here exercises
MOJIBAKE_RESagainstscan_template(a fixture containing\ufffdor theâ€/«sequences). Current tests only cover typographic quotes and placeholder leaks.def test_scan_template_flags_mojibake(tmp_path: Path) -> None: path = tmp_path / "broken.html" path.write_text("<p>Showing wallets matching \ufffd</p>\n", encoding="utf-8") errors = scan_template(path) assert any("mojibake" in item for item in errors)As per path instructions, "Focus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant."
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 334b92c1-af39-4118-8ecd-efaf37d98957
📒 Files selected for processing (10)
.github/workflows/ci.ymlCONTRIBUTING.mdapp/templates/activity.htmlapp/templates/bounties.htmlapp/templates/wallets.htmldocs/admin-runbook.mdscripts/template_text_smoke.pytests/test_activity.pytests/test_bounty_pages.pytests/test_template_text_smoke.py
| def test_rendered_public_pages_do_not_leak_jinja_placeholders(sqlite_url: str) -> None: | ||
| from app.db import create_schema | ||
| from fastapi.testclient import TestClient | ||
|
|
||
| from app.main import create_app | ||
|
|
||
| create_schema(sqlite_url) | ||
| client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) | ||
| for path, params in ( | ||
| ("/activity", {"q": "bob"}), | ||
| ("/wallets", {"q": "alice"}), | ||
| ("/bounties", {"q": "mergework"}), | ||
| ): | ||
| response = client.get(path, params=params) | ||
| assert response.status_code == 200 | ||
| for pattern in LEAKED_PLACEHOLDER_RES: | ||
| assert not pattern.search(response.text), f"{path} leaked {pattern.pattern}" | ||
|
|
||
|
|
||
| def test_scan_rendered_pages_helper(sqlite_url: str) -> None: | ||
| from app.db import create_schema | ||
|
|
||
| create_schema(sqlite_url) | ||
| errors = scan_rendered_pages(sqlite_url) | ||
| assert errors == [] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Duplicate render-check logic between two tests.
test_rendered_public_pages_do_not_leak_jinja_placeholders re-implements the same TestClient/placeholder-scan logic that scan_rendered_pages (tested separately below) already encapsulates. Consider having the first test call scan_rendered_pages directly with its own RENDER_CASES-style params, or drop the manual loop in favor of the helper to avoid duplicated request/scan logic.
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
Reviewed current head def157f53b2ed67de0da7e1fd4a3636b393074f5.
Requesting changes because the hosted quality gate is failing on this head. In run 28492489479, pytest and ruff format --check . complete, but the ruff check . step fails with three lint errors, including I001 [*] Import block is un-sorted or un-formatted.
The PR adds a new CI smoke step and a new scripts/template_text_smoke.py path, so the branch needs to pass the existing lint gate before merge. Sorting the imports with Ruff and rerunning the full quality workflow should clear this blocker.
yanyishuai
force-pushed
the
fix/issue-1111-template-text-smoke
branch
from
def157f to
80a0b11
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 91e44819-6cae-4421-86e3-c883ae48524e
📒 Files selected for processing (10)
.github/workflows/ci.ymlCONTRIBUTING.mdapp/templates/activity.htmlapp/templates/bounties.htmlapp/templates/wallets.htmldocs/admin-runbook.mdscripts/template_text_smoke.pytests/test_activity.pytests/test_bounty_pages.pytests/test_template_text_smoke.py
| <p class="notice">Showing accepted work for <code>{{ account }}</code>{% if account_page_url %} · <a href="{{ account_page_url }}">View account profile</a>{% endif %}</p> | ||
| {% elif query %} | ||
| <p class="notice">Showing accepted work matching “{{ query }}”.</p> | ||
| <p class="notice">Showing accepted work matching "{{ query }}".</p> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'TYPOGRAPHIC_NOTICE_ALLOWLIST' -A 10 scripts/template_text_smoke.pyRepository: ramimbo/mergework
Length of output: 821
Remove the stale allowlist entry for activity.html. app/templates/activity.html now uses ASCII quotes, but scripts/template_text_smoke.py still exempts it from the typographic-quote check, so the smoke test will miss future regressions.
| After deploy or when bounty comments look stale, run the public link health | ||
| check against representative bounty, proposal, proof, and OAuth URLs: | ||
|
|
||
| ```bash | ||
| python scripts/check_public_mrwk_links.py --input fixtures/public_mrwk_links.json --fail-on-issues | ||
| ``` | ||
|
|
||
| The script fails when a published link returns HTTP 4xx/5xx or an Express | ||
| `Cannot GET` shell instead of the expected public detail response. OAuth routes | ||
| use a separate health rule: `422` or `503` from FastAPI means the route is | ||
| registered, while `404` or an Express shell means production is serving the | ||
| wrong app (see issue #1146). | ||
|
|
||
| Post-deploy, also run: | ||
|
|
||
| ```bash | ||
| docker compose run --rm app python scripts/check_deploy_ready.py | ||
| ``` | ||
|
|
||
| That gate now verifies GitHub OAuth login/callback routes are registered in | ||
| the built app before a release goes live. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify check_deploy_ready.py contains OAuth-related checks
rg -n "oauth|github.*callback|/auth" scripts/check_deploy_ready.py -iRepository: ramimbo/mergework
Length of output: 155
🏁 Script executed:
#!/bin/bash
sed -n '1,240p' scripts/check_deploy_ready.py | cat -nRepository: ramimbo/mergework
Length of output: 1051
Remove the OAuth-route claim from the runbook. scripts/check_deploy_ready.py only checks deploy settings and executor config; it does not inspect GitHub OAuth login/callback routes. Either add that check to the script or drop the “now verifies” sentence.
| check against representative bounty, proposal, proof, and OAuth URLs: | ||
|
|
||
| ```bash | ||
| python scripts/check_public_mrwk_links.py --input fixtures/public_mrwk_links.json --fail-on-issues |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for referenced fixtures and scripts
ls -la fixtures/public_mrwk_links.json fixtures/review-rounds.json 2>/dev/null || echo "MISSING"
ls -la scripts/check_public_mrwk_links.py scripts/flag_superseded_review_rounds.py 2>/dev/null || echo "MISSING"Repository: ramimbo/mergework
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n-- referenced paths --\n'
git ls-files 'fixtures/public_mrwk_links.json' 'fixtures/review-rounds.json' \
'scripts/check_public_mrwk_links.py' 'scripts/flag_superseded_review_rounds.py' || true
printf '\n-- runbook context --\n'
sed -n '430,525p' docs/admin-runbook.md
printf '\n-- search for referenced names --\n'
rg -n 'public_mrwk_links\.json|review-rounds\.json|check_public_mrwk_links\.py|flag_superseded_review_rounds\.py' .Repository: ramimbo/mergework
Length of output: 4057
Update the runbook to reference existing assets
fixtures/public_mrwk_links.json, fixtures/review-rounds.json, scripts/check_public_mrwk_links.py, and scripts/flag_superseded_review_rounds.py are not in the repo, so the commands at lines 451 and 511-512 will fail until the docs point at real paths or the files are added.
| body = response.text | ||
| for pattern in LEAKED_PLACEHOLDER_RES: | ||
| if pattern.search(body): | ||
| errors.append(f"{path} {params}: leaked raw placeholder {pattern.pattern}") | ||
| if "\ufffd" in body: | ||
| errors.append(f"{path} {params}: replacement character in rendered HTML") | ||
| return errors |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Rendered-page mojibake check is narrower than the template-source check.
scan_template tests every line against the full MOJIBAKE_RES tuple (replacement char + UTF-8-as-Latin1 byte sequences), but scan_rendered_pages only checks for the bare \ufffd replacement character in rendered HTML. Mojibake sequences like "“" that reach rendered output (e.g. from double-encoded data) would pass --render silently, even though this is exactly the class of bug the PR objectives call out ("coverage for mojibake/replacement-character cases").
Proposed fix
- if "\ufffd" in body:
- errors.append(f"{path} {params}: replacement character in rendered HTML")
+ for pattern in MOJIBAKE_RES:
+ if pattern.search(body):
+ errors.append(f"{path} {params}: mojibake/replacement characters in rendered HTML")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| body = response.text | |
| for pattern in LEAKED_PLACEHOLDER_RES: | |
| if pattern.search(body): | |
| errors.append(f"{path} {params}: leaked raw placeholder {pattern.pattern}") | |
| if "\ufffd" in body: | |
| errors.append(f"{path} {params}: replacement character in rendered HTML") | |
| return errors | |
| body = response.text | |
| for pattern in LEAKED_PLACEHOLDER_RES: | |
| if pattern.search(body): | |
| errors.append(f"{path} {params}: leaked raw placeholder {pattern.pattern}") | |
| for pattern in MOJIBAKE_RES: | |
| if pattern.search(body): | |
| errors.append(f"{path} {params}: mojibake/replacement characters in rendered HTML") | |
| return errors |
| from __future__ import annotations | ||
|
|
||
| from pathlib import Path | ||
|
|
||
|
|
||
| from scripts.template_text_smoke import ( | ||
| LEAKED_PLACEHOLDER_RES, | ||
| scan_rendered_pages, | ||
| scan_template, | ||
| scan_templates, | ||
| ) | ||
|
|
||
|
|
||
| def test_template_text_smoke_passes_current_public_templates() -> None: | ||
| errors = scan_templates() | ||
| assert errors == [] | ||
|
|
||
|
|
||
| def test_scan_template_flags_typographic_query_notice(tmp_path: Path) -> None: | ||
| path = tmp_path / "wallets.html" | ||
| path.write_text( | ||
| '<p class="notice">Showing wallets matching “{{ query_text }}”.</p>\n', | ||
| encoding="utf-8", | ||
| ) | ||
| errors = scan_template(path) | ||
| assert any("typographic quotes" in item for item in errors) | ||
|
|
||
|
|
||
| def test_scan_template_allows_ascii_query_notice(tmp_path: Path) -> None: | ||
| path = tmp_path / "wallets.html" | ||
| path.write_text( | ||
| '<p class="notice">Showing wallets matching "{{ query_text }}".</p>\n', | ||
| encoding="utf-8", | ||
| ) | ||
| assert scan_template(path) == [] | ||
|
|
||
|
|
||
| def test_scan_template_flags_leaked_placeholder_in_fixture(tmp_path: Path) -> None: | ||
| path = tmp_path / "broken.html" | ||
| path.write_text("<p>Showing accepted work matching {{ query }}.</p>\n", encoding="utf-8") | ||
| # Static literal placeholder in template source is fine for Jinja; render check catches leaks. | ||
| assert scan_template(path) == [] | ||
|
|
||
|
|
||
| def test_rendered_public_pages_do_not_leak_jinja_placeholders(sqlite_url: str) -> None: | ||
| from app.db import create_schema | ||
| from fastapi.testclient import TestClient | ||
|
|
||
| from app.main import create_app | ||
|
|
||
| create_schema(sqlite_url) | ||
| client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) | ||
| for path, params in ( | ||
| ("/activity", {"q": "bob"}), | ||
| ("/wallets", {"q": "alice"}), | ||
| ("/bounties", {"q": "mergework"}), | ||
| ): | ||
| response = client.get(path, params=params) | ||
| assert response.status_code == 200 | ||
| for pattern in LEAKED_PLACEHOLDER_RES: | ||
| assert not pattern.search(response.text), f"{path} leaked {pattern.pattern}" | ||
|
|
||
|
|
||
| def test_scan_rendered_pages_helper(sqlite_url: str) -> None: | ||
| from app.db import create_schema | ||
|
|
||
| create_schema(sqlite_url) | ||
| errors = scan_rendered_pages(sqlite_url) | ||
| assert errors == [] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing test coverage for mojibake/replacement-character detection.
The linked issue explicitly asks for "coverage for mojibake/replacement-character cases and typographic quotes around dynamic notices," but this file only tests the typographic-quote and placeholder-leak paths. Neither scan_template's MOJIBAKE_RES branch nor scan_rendered_pages' \ufffd branch has any test exercising a fixture containing a replacement character or mojibake byte sequence. As per path instructions, tests should "include negative, replay, boundary, or regression cases where relevant," and this is a directly relevant, currently-untested code path.
Proposed addition
def test_scan_template_flags_mojibake(tmp_path: Path) -> None:
path = tmp_path / "broken.html"
path.write_text("<p>Showing results for \ufffd query</p>\n", encoding="utf-8")
errors = scan_template(path)
assert any("mojibake" in item for item in errors)Source: Path instructions
| def test_scan_template_flags_leaked_placeholder_in_fixture(tmp_path: Path) -> None: | ||
| path = tmp_path / "broken.html" | ||
| path.write_text("<p>Showing accepted work matching {{ query }}.</p>\n", encoding="utf-8") | ||
| # Static literal placeholder in template source is fine for Jinja; render check catches leaks. | ||
| assert scan_template(path) == [] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Misleading test name.
test_scan_template_flags_leaked_placeholder_in_fixture asserts scan_template(path) == [] (no errors) — the opposite of what "flags" implies. The inline comment clarifies intent, but the name should reflect that scan_template intentionally ignores static literal placeholders.
Proposed rename
-def test_scan_template_flags_leaked_placeholder_in_fixture(tmp_path: Path) -> None:
+def test_scan_template_ignores_static_placeholder_literal(tmp_path: Path) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_scan_template_flags_leaked_placeholder_in_fixture(tmp_path: Path) -> None: | |
| path = tmp_path / "broken.html" | |
| path.write_text("<p>Showing accepted work matching {{ query }}.</p>\n", encoding="utf-8") | |
| # Static literal placeholder in template source is fine for Jinja; render check catches leaks. | |
| assert scan_template(path) == [] | |
| def test_scan_template_ignores_static_placeholder_literal(tmp_path: Path) -> None: | |
| path = tmp_path / "broken.html" | |
| path.write_text("<p>Showing accepted work matching {{ query }}.</p>\n", encoding="utf-8") | |
| # Static literal placeholder in template source is fine for Jinja; render check catches leaks. | |
| assert scan_template(path) == [] |
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
Reviewed current head 80a0b118e9606d1587a4ec96266dc44313391632.
Requesting changes because the hosted quality gate is still failing on this head. Run 28505375578 gets through the test suite (911 passed) and ruff format --check ., then fails ruff check . with three fixable lint issues:
scripts/template_text_smoke.py:F401unusedsysimport.tests/test_template_text_smoke.py:I001import block not sorted/formatted at the module imports.tests/test_template_text_smoke.py:I001import block not sorted/formatted inside the rendered-page test helper.
Since this PR adds a new CI smoke step and test module, the branch should pass the existing quality gate before merge. Running Ruff fixes and rerunning the workflow should clear the blocker.
|
@qingfeng312 — proactive CRLF cleanup on this branch. Normalized LF line endings (no functional changes) in:
Should pass |
yanyishuai
force-pushed
the
fix/issue-1111-template-text-smoke
branch
from
80a0b11 to
a25abbf
Compare
|
@qingfeng312 — removed unused |
yanyishuai
force-pushed
the
fix/issue-1111-template-text-smoke
branch
from
a25abbf to
0e1211a
Compare
|
@qingfeng312 — pushed the missing #1111 scope: ASCII quote fixes in public templates, updated page tests, CI |
yanyishuai
force-pushed
the
fix/issue-1111-template-text-smoke
branch
8 times, most recently
from
65fb40d to
7283939
Compare
yanyishuai
force-pushed
the
fix/issue-1111-template-text-smoke
branch
from
7283939 to
d7961c1
Compare
|
@qingfeng312 — CI fully green on latest head for bounty #1111. Fixes on current head (
Please recheck when convenient. Wallet: |
taherdhanera
left a comment
taherdhanera
left a comment
There was a problem hiding this comment.
Approving current head d7961c17 for the checked #1111 template-text-smoke scope.
The earlier blockers on this PR were against older heads: missing/out-of-scope runbook commands and then hosted lint failures. On the current head, the unrelated runbook changes are gone, CI is green, and the focused template smoke scope is now coherent.
What I checked:
- Diff is limited to CI wiring, CONTRIBUTING guidance, three public template notice text updates,
scripts/template_text_smoke.py, and focused page/smoke tests. scan_templates()covers public templates and flags replacement/mojibake patterns plus typographic quote hazards around query notices.scan_rendered_pages()uses a file-backed sqlite URL, avoiding the prior in-memory TestClient/schema issue called out by the author.- The public notice text changes in activity, wallets, and bounties use ASCII quotes consistently.
- Hosted
Quality, readiness, docs, and image checksis green on current head, including the render smoke step.
Local validation available in this checkout:
python -m pytest tests\test_template_text_smoke.py -k "not rendered" -q
# 4 passed, 2 deselected
python scripts\template_text_smoke.py
# template text smoke ok (templates)
python -m ruff check scripts\template_text_smoke.py tests\test_template_text_smoke.py tests\test_activity.py tests\test_bounty_pages.py
# All checks passed!
python -m ruff format --check scripts\template_text_smoke.py tests\test_template_text_smoke.py tests\test_activity.py tests\test_bounty_pages.py
# 4 files already formatted
git diff --check origin/main...HEAD
# clean
I could not run local render/page tests in this bare checkout because FastAPI is not installed here, so I treated the hosted green workflow as the render-test evidence. I also manually confirmed scan_template() flags a \ufffd replacement-character fixture, so the remaining CodeRabbit note about adding an explicit mojibake unit fixture is useful follow-up coverage, not a merge blocker for this current head.
3 participants
Adds bounded template notice smoke + ASCII quote normalization. Fixes #1111. Wallet: Do4v7foHJvRJLpRRoGaVPWX6DDEjX3yTK7J91gpwUQpE
Summary by CodeRabbit