docs(api): document versioned REST API endpoints, schemas, error formats, and add contract tests (closes #20)

[ghzhost]

Summary

Comprehensive documentation of the versioned REST API for Lux under docs/API.md, addressing all requirements in #20.

Key Changes

• Detailed API Reference (docs/API.md):
  • Documents all existing /api/v1 and health routes (GET /health, GET /ready, GET /api/v1/levels, GET /api/v1/level/<id>, GET /api/v1/achievements, GET /api/v1/contributors, GET /api/v1/progress, GET /api/v1/profile, GET /api/v1/leaderboard, GET /api/v1/learning-path, GET /api/v1/game/adventure, GET /api/v1/game/daily, POST /api/v1/submit, POST /api/v1/puzzles/generate).
  • Includes realistic JSON request/response bodies and status codes (200 OK, 400 Bad Request, 404 Not Found).
  • Outlines local development setup and curl validation instructions.
  • Clearly delineates future proposed endpoints (/api/v1/auth/* and POST /api/v1/sync).
• README Link (README.md):
  • Adds direct link to docs/API.md in the API routes section.
• Contract & Error Test Suite (tests/test_web_api.py):
  • Adds unit test coverage (test_api_documentation_contract_and_errors) verifying status codes, success flags, error shapes (missing level_id, invalid level, not found), and service health status.

Validation

• pytest -q: 27 passed in 0.70s
• ruff check .: All checks passed!

/claim #20

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[johnny603]

Pull Request #28 Review: API Documentation & Contract Tests

Thanks for this comprehensive PR — it adds much-needed API documentation and contract tests. Below is a detailed review with actionable feedback organized by file.

---

Summary

Area	Status	Notes
README.md	✅ Good	Simple, appropriate link added
docs/API.md	⚠️ Needs work	Comprehensive but has inaccuracies and missing details
tests/test_web_api.py	⚠️ Needs expansion	Good start, but coverage is incomplete

---

README.md (L2-L5)

L2-L5: The link to docs/API.md is appropriately placed.

Suggestion: Consider adding a one-line description of what the API docs contain (e.g., "covering all versioned endpoints, request/response schemas, and error formats") to give users more context before they click through.

---

docs/API.md — Major Issues

1. Endpoint paths are incorrectly documented

L39-L41: The endpoint is documented as GET /api/v1/level/, but based on the repository README, the actual endpoint appears to be GET /api/v1/level/<id> (path parameter). The documentation mentions URL parameters but doesn't show the correct path format with a placeholder.

Fix: Change to GET /api/v1/level/{id} and clarify that the id is part of the path, not a query parameter.

---

2. Inconsistent error response format

L8-L11: The standard error format is documented as { "ok": false, "error": "<message>" }. However:

• The 404 Not Found response for GET /api/v1/level/ returns { "ok": false, "error": "not found" } — ✅ consistent.
• The 400 Bad Request for missing level_id returns { "ok": false, "error": "missing level_id" } — ✅ consistent.
• The 404 Not Found for invalid level returns { "ok": false, "error": "invalid level" } — ✅ consistent.

However, the incorrect attempt response returns { "ok": true, "correct": false, "expected": "-a" }. This uses "ok": true even though the submission was not successful (it was incorrect). This is semantically confusing — ok typically indicates the request was processed successfully, not that the answer was correct. The current behavior is acceptable if documented clearly, but the documentation should explicitly call out this nuance.

Suggestion: Add a note: "Note: ok: true indicates the request was valid and processed; correct: false means the submitted answer was wrong."

---

3. Missing endpoint: GET /api/v1/contributors response format inconsistency

L51-L55: The response for GET /api/v1/contributors is shown as an object with a contributors array. However, the repository README lists /contributors (without the /api/v1/ prefix) and doesn't specify the response format. This should be verified against the actual server implementation.

Action required: Confirm the actual response format and update the documentation accordingly.

---

4. POST /api/v1/submit — missing error cases

L87-L99: The documentation covers:

• Missing level_id → 400
• Invalid level → 404

Missing documented error cases:

• What happens when both attempt and files are missing?
• What happens when files contains an unsupported filename?
• What happens when the submission times out (e.g., Docker container hangs)?

Suggestion: Add these error scenarios to the documentation.

---

5. POST /api/v1/puzzles/generate — incomplete response

L100-L106: The response shows "answer": "...", but the generation may fail (e.g., Ollama not running). There's no documented error response for this scenario.

Suggestion: Add a 500/503 error response example for AI service unavailability.

---

6. Authentication section — needs clarity

L107-L130: The authentication section correctly notes that auth is not yet enabled. However, the proposed endpoints are documented in a way that might confuse users into thinking they're already implemented.

Suggestion: Add a clear visual indicator (e.g., a "🚧 Planned" badge) before each proposed endpoint, and move them to a separate "Future Endpoints" section to avoid confusion.

---

7. Missing schema definitions

The documentation would benefit from a shared schema section at the top defining common structures:

• Level object (id, category, title, description, hint, difficulty, tags)
• ErrorResponse object (ok, error)
• ProgressSummary object

This would reduce repetition and make the documentation more maintainable.

---

tests/test_web_api.py — Coverage Gaps

8. Test only covers a subset of documented endpoints

L143-L159: The test test_api_documentation_contract_and_errors covers:

• GET /health and GET /ready ✅
• GET /api/v1/level/99999 (404) ✅
• POST /api/v1/submit missing level_id (400) ✅
• POST /api/v1/submit invalid level (404) ✅

Missing test coverage for documented endpoints:

• GET /api/v1/levels
• GET /api/v1/achievements
• GET /api/v1/contributors
• GET /api/v1/progress
• GET /api/v1/profile
• GET /api/v1/leaderboard
• GET /api/v1/learning-path
• GET /api/v1/game/adventure
• GET /api/v1/game/daily
• POST /api/v1/puzzles/generate

Suggestion: Either expand this test to cover all documented endpoints, or create separate test functions for each endpoint group. At minimum, each endpoint should have a smoke test verifying it returns the expected status code and response shape.

---

9. Test doesn't verify response schemas

The test only checks status codes and the presence of ok/error fields. It doesn't validate that:

• Response bodies match the documented schemas
• All expected fields are present
• Field types are correct

Suggestion: Use a schema validation library (e.g., pydantic or jsonschema) to validate responses against the documented schemas.

---

10. No test for successful submission

The test covers error cases for POST /api/v1/submit but doesn't test a successful submission (both string attempts and script submissions).

Suggestion: Add test cases for:

• POST /api/v1/submit with a valid attempt → 200, correct: true
• POST /api/v1/submit with a valid files object → 200

---

Nitpicks & Minor Improvements

Location	Issue	Suggestion
docs/API.md L16-L23	Setup instructions duplicate content from README	Consider linking back to README instead of duplicating to reduce maintenance burden
docs/API.md L8	Base URL is http://127.0.0.1:5050	Also mention that the port can be configured via environment variable
docs/API.md L26-L30	Health endpoints return { "ok": true, "service": "ok" }	Consider adding a note about what "ok" vs "ready" means in terms of server state
tests/test_web_api.py L143	Test function name is very long	Consider breaking into smaller, named test functions (e.g., test_health_endpoints, test_level_not_found, test_submission_errors)

---

Overall Assessment

This is a valuable PR that significantly improves the project's documentation and test coverage. The documentation is comprehensive in scope but has some inaccuracies and omissions that should be addressed before merging. The tests are a good start but need expansion to fully validate the documented API contract.