Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions .github/instructions/code-review.instructions.md
Original file line number Diff line number Diff line change
@@ -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.
93 changes: 93 additions & 0 deletions .github/instructions/python.instructions.md
Original file line number Diff line number Diff line change
@@ -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.
2 changes: 1 addition & 1 deletion scripts/config/gitleaks.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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''']
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ Terraform
toolchain
Trufflehog
Uncomment
Werkzeug
Syncytial
pyenv
colima
Expand Down
Loading