Refs #406: Add public discovery routes#552
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds three SEO discovery endpoints ( ChangesPublic discovery routes
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f12fc1fa-de83-4ee4-8420-87ea4d8ec7a8
📒 Files selected for processing (3)
app/main.pyapp/templates/base.htmltests/test_api_mcp.py
Baijack-star
left a comment
Baijack-star
left a comment
There was a problem hiding this comment.
Requesting changes for a narrow test-coverage gap before merge.
The route implementation itself looks correct in the slice I checked: /robots.txt, /sitemap.xml, and /favicon.ico return bounded 200 responses; the sitemap uses MERGEWORK_PUBLIC_BASE_URL rather than the test/request host; the base template links /favicon.ico; and the routes are currently absent from /openapi.json because they are registered with include_in_schema=False.
The missing piece is regression coverage for that last contract. Since these are public browser/crawler discovery routes and intentionally hidden from the API schema, please add assertions to test_public_discovery_routes_are_bounded_and_use_public_origin (or a focused companion test) that /robots.txt, /sitemap.xml, and /favicon.ico are not present in client.get("/openapi.json").json()["paths"]. This matches the CodeRabbit pre-merge warning and prevents a future refactor from accidentally exposing these non-API routes in OpenAPI.
Validation I ran on head 33f56be6bffbaa16110d3af5b9b7cec51c537a62:
- focused discovery/HEAD tests -> 2 passed
tests/test_api_mcp.py tests/test_hub.py-> 81 passed- route smoke: all three discovery routes returned 200 with expected content types and
/rendered the favicon link - OpenAPI probe:
/robots.txt,/sitemap.xml, and/favicon.icoare currently absent frompaths - scoped Ruff check/format on Python files passed
mypy app/main.pypassed- docs smoke passed
git diff --check origin/main...HEADclean
|
Addressed in Validation after the update:
|
yunrongy424-oss
left a comment
yunrongy424-oss
left a comment
There was a problem hiding this comment.
Requesting changes for one small URL-normalization edge case.
/sitemap.xml already normalizes settings.public_base_url with rstrip(/), but /robots.txt builds the sitemap URL from the raw setting. Deploy validation allows MERGEWORK_PUBLIC_BASE_URL with path /, so https://mrwk.example.test/ is a valid origin-style setting. With that setting, the current route returns:
Sitemap: https://mrwk.example.test//sitemap.xml
That is avoidable crawler/discovery noise and inconsistent with the sitemap route's normalized origin. Please reuse a stripped base URL in robots_txt() and add a regression assertion for the trailing-slash setting.
Evidence on head 33f56be6bffbaa16110d3af5b9b7cec51c537a62:
- focused discovery/HEAD tests -> 2 passed
- scoped Ruff check/format on
app/main.pyandtests/test_api_mcp.py-> passed mypy app/main.py-> passed- docs smoke -> ok
git diff --check origin/main...HEAD-> clean- extra probe with
MERGEWORK_PUBLIC_BASE_URL=https://mrwk.example.test/reproduced the doubled sitemap slash above
No secrets, wallet material, private deployment values, private vulnerability details, live mutation, price claims, liquidity claims, or off-ramp claims were used.
|
Addressed the trailing-slash base URL edge in Validation after this update:
|
yunrongy424-oss
left a comment
yunrongy424-oss
left a comment
There was a problem hiding this comment.
Re-reviewed current head d405c15ab50b962fa4c76346bab5da87ef8bf13d after the follow-up fix.
The trailing-slash edge I flagged is resolved: robots_txt() now normalizes settings.public_base_url before appending /sitemap.xml, and the new regression test covers MERGEWORK_PUBLIC_BASE_URL=https://mrwk.example.test/ without emitting doubled slashes in either robots or sitemap output.
Validation on the current head:
- focused discovery/normalization/HEAD tests -> 3 passed
- Ruff check/format on
app/main.pyandtests/test_api_mcp.py-> passed - docs smoke -> ok
No remaining blocker in my reviewed slice.
Baijack-star
left a comment
Baijack-star
left a comment
There was a problem hiding this comment.
Re-reviewed current head d405c15ab50b962fa4c76346bab5da87ef8bf13d after the two follow-up commits.
My previous blocker is resolved: test_public_discovery_routes_are_bounded_and_use_public_origin now asserts /robots.txt, /sitemap.xml, and /favicon.ico stay absent from /openapi.json, so these public browser/crawler routes remain outside the API schema contract.
I also checked the later trailing-slash fix from this head. robots_txt() now normalizes settings.public_base_url before appending /sitemap.xml, matching the sitemap route, and the new regression covers MERGEWORK_PUBLIC_BASE_URL=https://mrwk.example.test/ without doubled slashes in robots or sitemap output.
Validation run locally on current head:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ./.venv/bin/python -m pytest tests/test_api_mcp.py::test_public_discovery_routes_are_bounded_and_use_public_origin tests/test_api_mcp.py::test_public_discovery_routes_normalize_public_base_url tests/test_api_mcp.py::test_head_requests_match_get_routes_without_body -q-> 3 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ./.venv/bin/python -m pytest tests/test_api_mcp.py tests/test_hub.py -q-> 82 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ./.venv/bin/python scripts/docs_smoke.py-> docs smoke ok./.venv/bin/python -m mypy app/main.py-> success./.venv/bin/python -m ruff check app/main.py tests/test_api_mcp.py-> passed./.venv/bin/python -m ruff format --check app/main.py tests/test_api_mcp.py-> already formattedgit diff --check origin/main...HEAD-> clean- ad hoc TestClient smoke with
MERGEWORK_PUBLIC_BASE_URL=https://mrwk.example.test/confirmed all three discovery routes return 200, no doubled sitemap URL is emitted, and all three remain absent from OpenAPI.
No remaining blocker in my reviewed slice.
|
Reviewed PR #552 at Evidence checked:
Validation:
Assessment: no blocker found. The change is bounded to browser/crawler discovery surfaces and keeps the API schema unchanged. |
4 participants
Refs #406
Summary
/robots.txt,/sitemap.xml, and/favicon.ico./favicon.icoso browsers stop probing a missing icon path.MERGEWORK_PUBLIC_BASE_URLrather than the request/test host.Evidence
Live production smoke before the fix, using unauthenticated public requests only:
GET https://mrwk.ltclab.site/robots.txt-> HTTP 404 JSON{"detail":"Not Found"}GET https://mrwk.ltclab.site/sitemap.xml-> HTTP 404 JSON{"detail":"Not Found"}GET https://mrwk.ltclab.site/favicon.ico-> HTTP 404 JSON{"detail":"Not Found"}This is a small browser/crawler polish fix: standard discovery URLs should either exist intentionally or fail closed. The current behavior is bounded, but it creates routine browser 404 noise and gives crawlers/agents no sitemap entry point for public docs, bounties, ledger, wallet, and status pages.
Live bounty preflight for #406 / internal bounty 66 showed
status=open,awards_remaining=15, and no active attempts.Validation
.\.venv\Scripts\python.exe -m pytest tests\test_api_mcp.py::test_public_discovery_routes_are_bounded_and_use_public_origin tests\test_api_mcp.py::test_head_requests_match_get_routes_without_body -q-> 2 passed.\.venv\Scripts\python.exe -m pytest tests\test_api_mcp.py tests\test_hub.py -q-> 81 passed.\.venv\Scripts\python.exe -m pytest -q-> 415 passed.\.venv\Scripts\python.exe -m ruff check .-> passed.\.venv\Scripts\python.exe -m ruff format --check .-> 79 files already formatted.\.venv\Scripts\python.exe -m mypy app-> success.\.venv\Scripts\python.exe scripts\docs_smoke.py-> docs smoke okgit diff --check-> cleanNo secrets, wallet material, private keys, tokens, cookies, OAuth state, private data, production mutation, price claims, liquidity claims, exchange claims, bridge promises, or private security details are included.
Summary by CodeRabbit
New Features
Tests