diff --git a/.github/instructions/code-review.instructions.md b/.github/instructions/code-review.instructions.md new file mode 100644 index 000000000..5bf9ebb4c --- /dev/null +++ b/.github/instructions/code-review.instructions.md @@ -0,0 +1,65 @@ +--- +description: "Code review instructions for the eligibility-signposting-api project" +applyTo: "**" +excludeAgent: ["coding-agent"] +--- + +# Code Review Instructions + +Guidelines for the eligibility-signposting-api project — a serverless AWS Lambda + Flask eligibility rules engine. + +## Review Priorities + +### Critical (block merge) + +- **Security**: Exposed secrets, PII leakage (especially NHS Numbers), missing header validation +- **Correctness**: Logic errors in rules engine evaluation, incorrect operator behaviour, data corruption in DynamoDB +- **Breaking Changes**: API contract changes to FHIR response models or request validation + +### Important (requires discussion) + +- **Code Quality**: SOLID violations, excessive duplication +- **Test Coverage**: Missing tests for critical paths, new rules/operators, or edge cases +- **Performance**: Unnecessary DynamoDB scans, missing caching, Lambda cold start regressions +- **Architecture**: Deviations from established patterns (wireup DI, chain of responsibility, operator registry) + +### Suggestion (non-blocking) + +- **Readability**: Naming, simplification of complex logic +- **Best Practices**: Minor convention deviations +- **Documentation**: Missing or incomplete docstrings + +## Security + +- **PII handling**: NHS Numbers must never appear in logs or error messages. `TokenError` messages must be redacted. Verify new log statements do not leak person data. +- **Secrets**: No API keys, tokens, or secrets in code. Use environment variables or AWS Secrets Manager. +- **NHS Number hashing**: Lookups use HMAC-SHA512 via `HashingService` with secret rotation (AWSCURRENT → AWSPREVIOUS fallback). +- **Header validation**: `NHSE-Product-ID` must be present (403 if missing). `nhs-login-nhs-number` must match path parameter. +- **Security headers**: Responses must include `Cache-Control: no-store, private`, `Strict-Transport-Security`, `X-Content-Type-Options: nosniff`. + +## Architecture + +- **Dependency injection**: Use wireup `@service` for all services, repos, and factories. Inject via `Injected[T]`, `Inject(qualifier=...)`, or `Inject(param=...)`. Never instantiate services manually. +- **Chain of responsibility**: Processing follows `CohortEligibilityHandler → BaseEligibilityHandler → FilterRuleHandler → SuppressionRuleHandler`. Extend this chain for new steps. +- **Operator registry**: New operators must extend `hamcrest.BaseMatcher` and register via the decorator-based `OperatorRegistry`. +- **Pydantic models**: Use `Field(alias=...)` for JSON mapping, `field_validator`/`model_validator` for validation. Response models use camelCase aliases. +- **FHIR compliance**: Error responses must use `OperationOutcome` models with `application/fhir+json` content type. +- **Lambda reuse**: The Flask app is cached in `CacheManager` across invocations. Changes to app initialization must not break container reuse. + +## Performance + +- **DynamoDB**: Use `query()` with `KeyConditionExpression`, never `scan()`. Partition key is `NHS_NUMBER`, sort key discriminator is `ATTRIBUTE_TYPE`. +- **S3 configuration loading**: Campaign configs load from S3 per request. Avoid unnecessary `list_objects` or `get_object` calls. +- **Caching**: Feature toggles use `TTLCache` (300s). New caching should follow the same pattern with appropriate TTLs. +- **Lambda cold starts**: Avoid heavy imports at module level. Keep wireup service graph lean. + +## Audit Trail + +- **Completeness**: New eligibility logic must call `AuditContext.append_audit_condition()` to record evaluation details. +- **Firehose delivery**: Audit events use Pydantic `AuditEvent` models sent to Kinesis Firehose. Preserve the full audit data model. + +## Terraform + +- **Encryption**: All AWS resources (DynamoDB, S3, Lambda, Firehose, Secrets Manager) must use KMS CMK encryption. +- **Environment parity**: Verify deletion protection and PITR are enabled for production/pre-production DynamoDB tables. +- **Safety**: Terraform changes must not destroy or replace stateful resources (DynamoDB tables, S3 buckets) unintentionally. diff --git a/.github/instructions/python.instructions.md b/.github/instructions/python.instructions.md new file mode 100644 index 000000000..d5dd89b48 --- /dev/null +++ b/.github/instructions/python.instructions.md @@ -0,0 +1,93 @@ +--- +description: "Python coding standards for the eligibility-signposting-api project" +applyTo: "**/*.py" +--- + +# Python Coding Standards + +## Naming Conventions + +- `snake_case` for functions, variables, and module names. +- `PascalCase` for class names. +- `UPPER_SNAKE_CASE` for constants. +- Prefix private methods and attributes with `_`. + +## Code Style + +- Line length limit: 120 characters (enforced by ruff). +- Use type hints for all function signatures and return types. +- Prefer `dataclass` for simple domain objects, Pydantic `BaseModel` for validated/serialized models. +- Use `StrEnum` for string enumerations. +- Avoid bare `except:` — catch specific exceptions. + +## Error Handling + +```python +# Bad: silent failure +try: + person = repo.get(nhs_number) +except Exception: + pass + +# Good: specific exceptions with context +try: + person = repo.get(nhs_number) +except PersonNotFoundError: + raise +except ClientError as e: + raise RepositoryError(f"Failed to query person table: {e}") from e +``` + +## Dependency Injection (wireup) + +- Decorate services with `@service`. Do not instantiate services manually. +- Use `Inject(qualifier=...)` for AWS client disambiguation. +- Use `Inject(param=...)` for configuration values. +- Register factory functions with `@service` for boto3 clients. + +```python +# Good +@service +class MyService: + def __init__(self, repo: Injected[MyRepo]) -> None: + self._repo = repo + +# Bad: manual instantiation +class MyService: + def __init__(self) -> None: + self._repo = MyRepo() +``` + +## Pydantic Models + +- Use `Field(alias=...)` for JSON key mapping. +- Use `field_validator` / `model_validator` for custom validation. +- Response models must use camelCase aliases (`alias_generator=to_camel` or explicit aliases). +- Use `model_dump(by_alias=True)` when serializing for API responses. + +## Testing + +- Use `pytest` with pyHamcrest assertions (`assert_that`, `is_`, `has_entries`, `contains_exactly`, etc.). +- Use `brunns-matchers` for Werkzeug response assertions. +- Use project auto-matchers (`BaseAutoMatcher`) for dataclass/Pydantic model assertions. +- Use `polyfactory` (`DataclassFactory` / `ModelFactory`) for test data builders. +- Mock AWS services with `moto`, not manual stubs. +- Use `@pytest.mark.parametrize` for rule/operator test cases. + +```python +# Good: pyHamcrest with specific matchers +def test_eligible_person_returns_eligible_status(): + result = evaluate(person, campaign) + assert_that(result, is_(has_property("status", equal_to(Status.ELIGIBLE)))) + +# Bad: generic assert +def test_eligible(): + result = evaluate(person, campaign) + assert result is not None +``` + +## Logging + +- Use structured JSON logging via `python-json-logger`. +- Never log NHS Numbers or other PII. +- Include `request_id` via the `ContextVar` pattern for request tracing. diff --git a/scripts/config/gitleaks.toml b/scripts/config/gitleaks.toml index 66a3d7e94..adf24874f 100644 --- a/scripts/config/gitleaks.toml +++ b/scripts/config/gitleaks.toml @@ -16,5 +16,5 @@ regexes = [ ] [allowlist] -paths = ['''.terraform.lock.hcl''', '''poetry.lock''', '''yarn.lock'''] +paths = ['''.terraform.lock.hcl''', '''poetry.lock''', '''yarn.lock''', '''.github/instructions/\*.instructions.md'''] stopwords = ['''dummy_key''', '''dummy_secret''', '''192.0.0.1''', '''prance = "^25.4.8.0"''', '''25.4.8.0'''] diff --git a/scripts/config/vale/styles/config/vocabularies/words/accept.txt b/scripts/config/vale/styles/config/vocabularies/words/accept.txt index fc7d8ef23..dc1443bea 100644 --- a/scripts/config/vale/styles/config/vocabularies/words/accept.txt +++ b/scripts/config/vale/styles/config/vocabularies/words/accept.txt @@ -27,6 +27,7 @@ Terraform toolchain Trufflehog Uncomment +Werkzeug Syncytial pyenv colima