Merge pull request #595 from NHSDigital/feature/eja-add-custom-review-instructions

Create *.instructions.md for code review

Author: eddalmond1

.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.

.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.

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''']

scripts/config/vale/styles/config/vocabularies/words/accept.txt

@@ -27,6 +27,7 @@ Terraform
 toolchain
 Trufflehog
 Uncomment
+Werkzeug
 Syncytial
 pyenv
 colima