[Tvisha-A]

📌 Description

• Adds two new fields, version and environment, to health endpoint.
  • defines an explicit environment variable PESU_AUTH_ENVIRONMENT
• Makes /health answerable to HEAD requests
• Turns off caching in health , so that the response is not stale

ℹ️ Fixes / Related Issues
Closes: #168

🧱 Type of Change

Please indicate the type of changes introduced in your PR. Anything left unchecked will be assumed to be non-relevant

• 🐛 Bug fix – Non-breaking fix for a functional/logic error
• ✨ New feature – Adds functionality without breaking existing APIs
• ⚠️ Breaking change – Introduces backward-incompatible changes (API, schema, etc.)
• 📝 Documentation update – README, docstrings, OpenAPI tags, etc.
• 🧪 Test suite change – Adds/updates unit, functional, or integration tests
• ⚙️ CI/CD pipeline update – Modifies GitHub Actions, pre-commit, or Docker build
• 🧹 Code quality / Refactor – Improves structure, readability, or style (no functional changes)
• 🐢 Performance improvement – Speeds up auth, profile retrieval, or reduces I/O
• 🕵️ Debug/logging enhancement – Adds or improves logging/debug support
• 🔧 Developer tooling – Scripts, benchmarks, local testing improvements
• 🔒 Security fix – Addresses auth/session/data validation vulnerabilities
• 🧰 Dependency update – Updates libraries in pyproject.toml, uv.lock

🧪 How Has This Been Tested?

Please indicate how you tested your changes. Completing all the relevant items on this list is mandatory

• Unit Tests (tests/unit/)
• Functional Tests (tests/functional/)
• Integration Tests (tests/integration/)
• Manual Testing

⚙️ Test Configuration:

• OS: (macos)
• Python: (3.14 via uv)
• Docker build tested

✅ Checklist

Please indicate the work items you have carried out. Completing all the relevant items on this list is mandatory. Anything left unchecked will be assumed to be non-relevant

• My code follows the CONTRIBUTING.md guidelines
• I've performed a self-review of my changes
• I've added/updated necessary comments and docstrings
• I've updated relevant docs (README or endpoint docs)
• No new warnings introduced
• I've added tests to cover my changes
• All tests pass locally (scripts/run_tests.py)
• I've run linting and formatting (pre-commit run --all-files)
• Docker image builds and runs correctly
• Changes are backwards compatible (if applicable)
• Feature flags or .env vars updated (if applicable)
• I've tested across multiple environments (if applicable)
• Benchmarks still meet expected performance (scripts/benchmark/benchmark_requests.py)
  uv run --group benchmark python benchmark_requests.py --num-requests 100 --parallel --tag baseline

🛠️ Affected API Behaviour

Please indicate the areas affected by changes introduced in your PR

• app/app.py – Modified /authenticate route logic
• app/pesu.py – Updated PESU Academy login or profile handling
• app/exceptions/ – Error types, their status codes or messages
• app/metrics/ – Metrics collection, exposition or the /metrics token
• app/docs/ – OpenAPI (Swagger) examples and descriptions

🧩 Models

• app/models/request.py – Input validation or request schema changes
• app/models/response.py – Authentication response formatting
• app/models/profile.py – Profile response schema
• app/models/upstream.py – Parsing of PESU Academy's login and profile responses

🐳 DevOps & Config

• Dockerfile – Changes to base image or build process
• .github/workflows/*.yml – CI/CD pipeline or deployment updates
• pyproject.toml / uv.lock – Dependency version changes
• .pre-commit-config.yaml – Linting or formatting hook changes
• .github/scripts/check_version_bump.py – The CI version check

📊 Benchmarks & Analysis

• scripts/benchmark/benchmark_requests.py – Performance or latency measurement changes
• scripts/benchmark/analyze_benchmark.py – Benchmark result analysis changes
• scripts/run_tests.py – Custom test runner logic or behavior updates

📸 Screenshots / API Demos (if applicable)

🧠 Additional Notes (if applicable)

yet to pass all tests due to invalid pesu credentials

[aditeyabaral]

Thanks for picking this up, @Tvisha-A! The runtime behaviour is right: /health returns version and environment for all three values and when the variable is unset, a bad value fails startup loudly, and the Swagger 200/500 models are the right way round. The comments below are what's left before this meets the issue's "Done when" list.

CI is red on every job because uv.lock wasn't regenerated after the version bump (see the comment on pyproject.toml). That stops CI at uv sync --locked, so it never actually linted or tested this PR.

A few things that don't belong to a single line:

• Commit metadata: the commit has no body. Conventional Commits here should explain why in the body. Its author email is tvishaagrawal.12@ggmail.com (note ggmail), which doesn't match a GitHub account, so the commit isn't credited to your profile. Fix it with git config user.email and git commit --amend --reset-author.
• PR description: /health already answered HEAD on dev before this PR; this PR adds tests for it, so the description should say that. There's a stray uv run --group benchmark ... line inside the checklist, and "Changes are backwards compatible" can be ticked, since the change only adds fields.
• Live tests: this doesn't touch the login path, so not running them is low risk. Please still say in the description that they weren't run, because CI doesn't run them on PRs from forks either.

[aditeyabaral]

The bump to 5.5.0 is right, but uv.lock still records 5.4.0. The version check fails with pyproject.toml says 5.5.0 but uv.lock says 5.4.0, and every other job stops at uv sync --locked. Run uv lock and commit uv.lock along with this.

[aditeyabaral]

This test never starts the app: it sets the variable and then checks that os.environ holds what was just set, so it passes even if the lifespan ignores PESU_AUTH_ENVIRONMENT entirely. The issue asks for each valid value to be tested, so start the app and check what /health reports:

Suggested change

	@pytest.mark.parametrize("valid_cases", ("development","production","staging"))
	def test_valid_cases_for_PESU_AUTH_ENVIRONMENT(monkeypatch, valid_cases):
	monkeypatch.setenv("PESU_AUTH_ENVIRONMENT",valid_cases)
	assert os.environ.get("PESU_AUTH_ENVIRONMENT") == valid_cases
	@pytest.mark.parametrize("valid_cases", ("development", "production", "staging"))
	def test_valid_cases_for_PESU_AUTH_ENVIRONMENT(monkeypatch, valid_cases):
	monkeypatch.setenv("PESU_AUTH_ENVIRONMENT", valid_cases)
	with TestClient(app) as client:
	response = client.get("/health")
	assert response.json()["environment"] == valid_cases

[aditeyabaral]

This only passes because an earlier test's lifespan has already set app.state.environment. Run on its own (pytest tests/unit/test_health.py::test_default_fallback_PESU_AUTH_ENVIRONMENT) it fails, and it also fails if the variable is set in the developer's shell. Clear the variable, start the app, and check the response:

Suggested change

	def test_default_fallback_PESU_AUTH_ENVIRONMENT():
	assert app.state.environment == "development"
	def test_default_fallback_PESU_AUTH_ENVIRONMENT(monkeypatch):
	monkeypatch.delenv("PESU_AUTH_ENVIRONMENT", raising=False)
	with TestClient(app) as client:
	response = client.get("/health")
	assert response.json()["environment"] == "development"

[aditeyabaral]

A few problems with this field:

• default="development" takes environment out of required in the published schema, though every response includes it. Use ... like the other fields; the default belongs in the lifespan, which already has one.
• The example "developement" is misspelled, and isn't one of the field's own allowed values.
• "an environment variable" doesn't tell a reader anything. Say what it means and where it comes from.

Suggested change

	default="development",
	title="environment",
	description="an environment variable",
	json_schema_extra={"example": "developement"},
	...,
	title="Environment",
	description="The deployment this instance is running as, set by `PESU_AUTH_ENVIRONMENT`.",
	json_schema_extra={"example": "development"},

[aditeyabaral]

#254 put all of the header logic in _security_headers (line 64) so that it applies to every response, including the 500 that unhandled_exception_handler renders. Because the header is set here instead, a 500 from /health goes out without Cache-Control. Drop it here:

Suggested change

headers={"Cache-Control": "no-store"},

and add /health to the check in _security_headers. That function isn't part of this diff, so it can't be a suggestion:

    Returns:
        dict[str, str]: The general security headers, plus `Cache-Control: no-store` for /authenticate and /health.
    """
    headers = dict(SECURITY_HEADERS)
    if request.url.path in ("/authenticate", "/health"):
        headers["Cache-Control"] = "no-store"

Also update the security_headers_middleware docstring ("prevent caching for /authenticate") and the README's "Response headers" section, which still says only /authenticate gets no-store.

[aditeyabaral]

The issue asks for a commented-out line. It also shouldn't go at the end of the TEST_* profile block. That file's header says the application never reads it, so give it its own section that says the app reads it from its environment:

Suggested change

	# Environment variable PESU_AUTH_ENVIRONMENT can take on values in ("development", "staging", "production"). defaults to "development" when not set
	# Not a test variable: the app reads it from its environment and reports it on /health. One of
	# development, staging or production, and development when unset; any other value fails startup.
	# PESU_AUTH_ENVIRONMENT=development

[aditeyabaral]

A few things here:

• The variable is read twice. Read it once into a local.
• Fix the typos ("Invaild enivronment") and include the value that was rejected, so whoever deployed it can see what's wrong.
• A comment saying why this is checked at startup helps the next reader.

Suggested change

	if os.environ.get("PESU_AUTH_ENVIRONMENT", "development") not in ("development", "staging", "production"):
	raise ValueError("Invaild enivronment value. Value must be development, staging or production")
	app.state.environment = os.environ.get("PESU_AUTH_ENVIRONMENT", "development")
	# Read once, so that a bad deploy fails here instead of /health reporting a wrong value
	environment = os.environ.get("PESU_AUTH_ENVIRONMENT", "development")
	if environment not in ("development", "staging", "production"):
	raise ValueError(f"PESU_AUTH_ENVIRONMENT must be development, staging or production, not {environment!r}")
	app.state.environment = environment

The test on lines 48-51 of tests/unit/test_health.py checks the old message; there's a suggestion there to match this one, so apply both together (Add suggestion to batch). Optionally, the three values are now listed here and in HealthModel's Literal; a shared Literal alias, checked with typing.get_args, would keep the two from drifting apart.

[aditeyabaral]

version() reads the package metadata from disk on every call, and /health is called a lot. app.version already holds the same value, computed once at import:

Suggested change

	"version": version("pesu-auth"),
	"version": app.version,

[aditeyabaral]

If /health returns anything other than 200, this test asserts nothing and passes. Assert the status first:

Suggested change

	if response.status_code == 200:
	json_data = response.json()
	actual_version = json_data["version"]
	assert actual_version == version("pesu-auth")
	assert response.status_code == 200
	assert response.json()["version"] == version("pesu-auth")

[aditeyabaral]

Titles and descriptions here are cased inconsistently ("Health status" vs "timestamp" and "version"), and this one reads oddly. Match ResponseModel:

Suggested change

	title="timestamp",
	description="returns the timestamp of when /health was used",
	title="Health Check Timestamp",
	description="Time of the health check, in IST (UTC+05:30).",

The same goes for the other fields: e.g. "Version" for version's title, and capitalised descriptions for status and message.

[aditeyabaral]

Follow-up to my review above: I've added suggestions to the comments, so you can apply the fixes from this page. Some comments moved to the lines their suggestion replaces. Where a comment says to apply suggestions together, use Add suggestion to batch and commit them at once.

[aditeyabaral]

fastapi.response doesn't exist (import fastapi.response raises ModuleNotFoundError). This only goes unnoticed because it's under TYPE_CHECKING. Request is also imported twice (lines 18 and 25), and Response is imported at runtime though it's only used in annotations.

health() doesn't need the request parameter: app is already in module scope, so it can read app.state.environment directly. Then these imports can go back to what they were:

Suggested change

	from fastapi.requests import Request
	from fastapi.responses import FileResponse, JSONResponse, PlainTextResponse, RedirectResponse, Response
	from fastapi.routing import APIRoute
	if TYPE_CHECKING:
	from collections.abc import AsyncGenerator
	from fastapi.requests import Request
	from fastapi.responses import Response
	from fastapi.response import Response
	from fastapi.responses import FileResponse, JSONResponse, PlainTextResponse, RedirectResponse
	from fastapi.routing import APIRoute
	if TYPE_CHECKING:
	from collections.abc import AsyncGenerator
	from fastapi.requests import Request
	from fastapi.responses import Response

Apply this together with the suggestions on lines 338 and 349 (Add suggestion to batch). If only this one is applied, FastAPI can no longer resolve request: Request and treats it as a required query parameter, so /health answers 400.

[aditeyabaral]

Part of the import fix on lines 18-26: app is in module scope, so health() needs no request.

Suggested change

	async def health(request: Request) -> JSONResponse:
	async def health() -> JSONResponse:

Apply this together with the suggestions on lines 18-26 and 349 (Add suggestion to batch).

[aditeyabaral]

Part of the import fix on lines 18-26:

Suggested change

	"environment": request.app.state.environment,
	"environment": app.state.environment,

Apply this together with the suggestions on lines 18-26 and 338 (Add suggestion to batch).

[aditeyabaral]

Goes with the suggestion on lines 89-91 of app/app.py, which fixes the typos in this message; apply both together. match= is the usual way to check an exception's message:

Suggested change

	with pytest.raises(ValueError) as exc_info:
	with TestClient(app):
	pass
	assert "Invaild enivronment value. Value must be development, staging or production" in str(exc_info.value)
	with pytest.raises(ValueError, match="PESU_AUTH_ENVIRONMENT must be development, staging or production"):
	with TestClient(app):
	pass

[aditeyabaral]

Removing /health from the test below means nothing checks any more that /health sends the standard security headers. Since it now sends no-store, it needs its own test, for both GET and HEAD:

Suggested change

	def test_integration_security_headers_on_general_endpoints(client):
	def test_integration_health_security_headers(client):
	"""/health must carry the standard security headers and Cache-Control: no-store, on GET and HEAD."""
	for response in (client.get("/health"), client.head("/health")):
	assert response.status_code == 200
	assert_security_headers(response)
	assert response.headers["Cache-Control"] == "no-store"
	def test_integration_security_headers_on_general_endpoints(client):

[aditeyabaral]

Ruff doesn't check tests/, but please match the style of the other test files. Add a module docstring, and drop the imports nothing uses (lifespan and HealthModel, and os once the environment tests no longer need it):

Suggested change

	import os
	from importlib.metadata import version
	import pytest
	from fastapi.testclient import TestClient
	from app.app import app, lifespan
	from app.models import HealthModel
	"""Tests for /health: the version and environment fields, HEAD, and Cache-Control."""
	from importlib.metadata import version
	import pytest
	from fastapi.testclient import TestClient
	from app.app import app

For the rest, run uv run ruff format tests/unit/test_health.py once. It adds the two blank lines between top-level functions, removes the blank lines between @pytest.mark.parametrize and its function (lines 36 and 45), and adds spaces after commas.

[aditeyabaral]

This blank line deletion isn't related to this issue. Please put it back:

Suggested change

	return check
	return check

[jois-code]

I left an inline comment about the outdated 5.0.0 response example. CI currently stops before running tests: uv sync --locked --all-groups fails because pyproject.toml is at 5.5.0 but uv.lock is out of date.

[jois-code]

this PR bumps the package to 5.5.0, while /health returns the installed package version. Please update the example to match the current version