CampusCore Coding Principles

The canonical home for CampusCore-specific coding principles. Load this before planning, writing, or reviewing any code (the plan and implement skills and the code-reviewer agent do so explicitly). General-engineering failure modes live in robust-code.md; Ousterhout-style design guidance lives in the software_design_philosophy skill - where that skill and this doc overlap, this doc wins.

These principles are the target, not a description of every file

These principles are how we build now, and large parts of the codebase already follow them - but not all of it does. Where you find code that violates these patterns, don't treat it as the template to copy. When the surrounding style and these principles conflict, the principles win for any code you write or change. If a file you're editing deviates (e.g. a service that both produces and persists, a try/except Exception swallowing primary behavior, a feature gate that does nothing), bring your change in line with the principles and flag the deviation to the user with a short note on what's off and what the right shape would be - then refactor it only if they agree or it falls within the Boy Scout rule's "proportional to the task" bound. Don't silently propagate a bad pattern, and don't silently launch a large refactor either.

Design Principles

These principles override convenience. They exist because we've shipped real bugs by violating them - most recently a entity extraction feature that silently never ran for any ingest because the gating logic was buried inside a persistence helper. Audit new features against these before merging.

1. One service, one capability, one typed contract. A service takes one named Pydantic input and returns one named Pydantic output. process_file_content extracts. EntityExtractor types. chunk_markdown chunks. DocumentWriter writes. No service reaches into another's internals or does two unrelated things. If a writer does extraction-as-a-side-effect, it has two responsibilities and zero clear contracts - split it.

2. The orchestrator is the only place that knows step order. Services don't know what runs before or after them. The sequence lives in one file, reads top-to-bottom like prose, and matches the state machine's STEP_ORDER 1:1. If a reader can't open the orchestrator and tell you what each ingest run does in 30 seconds, the orchestrator is wrong.

3. Each pipeline step is its own state-machine transition. No opaque run_step(DONE, …) that swallows seven internal stages. Per-step transitions give free retries on the specific step that failed, free cost attribution per step in DocumentIngestionRunEvent, and free observability (step_started/step_completed rows). The price of admission for being a step is emitting its own pair of events.

4. No silent fallbacks for primary behavior. If feature X is the point of the work, X runs or raises a categorized error (SchemaInvalid → Failed Items Inbox; Transient → retry budget; Fatal → operator intervention). A try/except Exception that catches everything and falls back to a default is how features stay broken for weeks while green tests ship past them. Catch only the exception types you can actually handle.

5. A green test must mean the feature works, not just that a helper ran. The evidence a change is done is a test of its public contract - "after ingest(folder_upload), typed Documents exist for the uploaded files." Test private helpers where they hold tricky logic worth pinning against regression - reading a private in a test is fine, but wiring test state by setting or patching one goes through a real seam instead (Design Principle #11). But a passing helper test never substitutes for the contract test: green on _get_or_create_parent_document proves the helper runs, not that the feature works or that this change didn't break something downstream. If the public API has no assertable contract for a feature, the feature is not done.

6. No plan/phase references anywhere in code or data. No Phase 2, v1, WIP, Stage 1, phase2-typed-v1 in comments, names, fields, enum values, string literals, or stamped data. The codebase is the artifact; planning docs live in PRs and docs/. Stamped fields describe what the row is ("entity", "generic"), not which planning doc produced it. To know when something was added, use git log. Outside Python, naming a system's own runtime stages is not a plan reference: a Docker build stage, a deploy workflow's role phases, or a pipeline stage a user sees in the UI keeps the name the running system uses, and the checker's mechanical ban is correspondingly Python-only (Comments & Docstrings rule 3). In Python, name a stage by its domain word (discovered, crawled, builder) - there the numbered form reads as plan vocabulary and the checker blocks it.

7. Feature gates are decisions, not hiding spots. A USE_TYPED_EXTRACTION=True setting silently overridden by a missing kwarg is worse than no setting. If a flag exists, flipping it must change behavior end-to-end and a test must assert that. If a flag stops doing anything, delete it the same PR you make it inert.

8. One service, one front door. A reader landing on a service module must tell from the import line - without opening the file - how to use it. Each service exposes exactly one of: a class with a constructor plus one or two public methods, OR a single named module-level function. Everything else is _underscored and absent from __all__. Two co-equal top-level functions side by side means two services have been fused - split them.

9. Shared infrastructure lives in exactly one place. Cross-cutting primitives - LLM client construction, AWS client construction (S3 / SQS / Lambda), content-type resolution, token counting, tempfile lifecycle - are themselves services living in dedicated modules (apps/main_app/clients/, apps/main_app/utils/), not private helpers inside every consumer. When a provider's SDK changes its constructor, that change touches one file. Any def _llm(): or def _s3_client(): inside a service file is a duplicated recipe waiting to drift.

10. A service is defined by its output, not by what callers do next. The contract is the single typed value it produces. Persistence, telemetry, downstream notifications, queue dispatch are what some caller does with the value, not part of the producing service. EntityExtractor.extract(markdown) → EntityBatch is one service; DocumentWriter.write_batch(batch, source_file) → list[Document] is a different one. Orchestrators compose services; services don't compose each other.

11. Leading underscore means genuinely internal. A _name is internal to its own class or its own .py file - the file is the boundary, not the package. Same-class instance-to-instance access is internal in principle, but express clone/builder wiring through the constructor - a with_* builder passes state to __init__ (or a _replace helper that does) - so the writes land on self and ruff's SLF001 stays silent by construction; the rare unavoidable cross-instance write carries a # noqa: SLF001 with its reason on the line. The moment a second file needs a _name, one of three things happens: the member is promoted to a public name, it gets a deliberate seam (a constructor parameter, a typed Settings field), or the caller moves into the file. Importing another module's _name is forbidden in production code - lint cannot detect this class of access, so review owns it. Third-party privates that are de-facto public API (Django's Model._meta; an httpx Response._content write that mirrors aread()) are the one sanctioned exception: exempted in config or suppressed at the site, with the rationale alongside. Tests may read privates to pin tricky logic (the per-file-ignore for **/tests/** encodes this), but wiring test state by setting or patching a private requires a real seam - or, where a seam would be pure scaffolding, a comment marked Justified exemption under #35.

12. Exceptions live where their contract lives. Exception types and error-handling helpers that cross a subsystem boundary live in apps/main_app/services/error_handling/ - today that is the categorized pipeline taxonomy and chat-facing sanitization. The typed-API family in campus_core/api/exceptions.py owns HTTP error rendering and stays with the framework. An exception raised and handled within one subsystem stays at its raise site: a module-local signal is _-private, and one caught by a sibling module is public in its own module. The test for moving one is "does a second subsystem need to catch it?" - centralizing an exception nobody shares couples subsystems for no benefit.

13. A name is a promise of exactly what the thing does. A caller must be able to trust a name without opening the body. A function named reindex that re-embeds rows, a validate_* that mutates state, a get_* that creates - each is a defect even when the behavior is correct, because every call site now reads as doing something it doesn't. When a name and its behavior disagree, fix the name to match the behavior - or narrow the behavior to match the name - in the same change that notices the gap; prose explaining the mismatch is not a fix. This applies to modules, classes, functions, admin action labels, URL names, and CLI commands alike. Renaming a user-visible or wire-visible name is a behavior change and is called out in the plan and PR like any other.

Type-Driven Development

Treat Pydantic models as executable documentation and the primary guardrails of the system.

1. Types First, Logic Second - Define the Pydantic input/output models, then the function signature, then the logic. Never start with the implementation.

2. Eliminate Primitive Obsession - Don't use raw str/int/bool for domain concepts. Use semantic types or value objects (email: EmailStr, age: int = Field(..., gt=0, lt=120)). A status, or any other field with a closed value set - a kind, a protocol, a scope, a match mode, a category, an outcome - is its model's TextChoices class on a wire model, never str; see "Enumerated fields" in spa-and-api-framework.md.

3. Make Illegal States Unrepresentable - Use Literal + discriminated unions instead of optional fields whose combinations can be invalid:

class Success(BaseModel):
    status: Literal['success']
    data: dict
class Error(BaseModel):
    status: Literal['error']
    message: str
Response = Union[Success, Error]

4. Parse, Don't Validate - Validate untrusted data once, at the boundary (HTTP, DB rows, LLM output, queue messages), and emit a narrower type that proves it happened. Functions downstream take the parsed model, not the raw dict - so re-checking structural invariants deep in the logic is impossible by construction, not just discouraged. "Trust it" covers what the type guarantees (field present, right type, in range); facts the type can't encode - a user_id still resolves to a live row, an action is inside its allowed window - are business checks you still make at the point of use.

5. Explicit Method Inputs - Every argument has a type hint. Group 3+ arguments into a Pydantic input model (validation lives in the model, not the function body).

6. Explicit Return Types - No raw dict returns. def get_user(id: UUID) -> UserProfile | None:, not -> dict.

7. Review the representation, new and touched - The review form of items 3 and 4 above and the data form of "One canonical home per invariant" below: before the code, ask three questions of every model, Pydantic shape, queue message or TypeScript state type the change adds, edits, writes, or branches on, and ask them of the structure as it stands today, not only of the fields being added:

1. Impossible combinations - Which combinations of these fields are impossible, and what in the type or the database stops them? Two booleans, a nullable pair, or a status with "companion" columns usually mean one enum or one variant is missing.
2. Second home - Where else is this fact stored, and which single writer updates the other copy? A denormalized column, a cached count, or a list that mirrors a foreign key is fine only when one named writer owns it.
3. Silent rename - What breaks if one of these values is renamed, and does the compiler or the test suite see it? A str status, a metadata key read in six files, or a dict[str, Any] the SPA parses by hand breaks silently.

A change touches a structure when it creates or edits the definition, writes a field, or branches on a field (a status check, a flag test, a null check on a scope key); reading a field only to pass it through or display it does not count. A "no" that predates the change is still a finding. Fix it now when the fix is proportional to the task, and write the fix into plan.md before any review sees the diff; otherwise defer it, citing the GitHub issue that tracks it, or naming a candidate issue in the plan or the PR for the human to file - never file it yourself. "The tests are green" answers none of the three: a shape that allows an invalid state passes every test that never constructs that state. Reference shapes: WebPage.host in apps/main_app/models/scraping.py is a database-computed column with no second writer. The AgentEvent union in apps/main_app/services/agent/events.py gives each event kind only its own fields.

Comments & Docstrings

Prose in code is held to the same bar as the code. These rules govern every comment in production code, whatever the language - Python docstrings and # comments, TypeScript/TSX, Terraform, shell, workflow YAML, Django templates, CSS, and config files alike. scripts/check_comment_hygiene.py enforces the mechanically-detectable subset on changed files of every comment-bearing type (via gate.sh and pre-commit), and the code-reviewer agent checks the rest. In templates the checker also enforces the rendering rule that a {# #} comment closes on its own line (template-comment-across-lines); multi-line prose belongs in {% comment %}. The plain-dash rule is part of that subset: an em dash in a comment is a hit (em-dash) in every language, while one in a string literal or rendered text is left alone, because there the character can be content the user reads.

1. A comment states a contract, an invariant, or a why the code cannot express - otherwise it does not exist. A comment that restates the adjacent code, or a docstring that rewords the def line ("Lazy initialization of retrieval service"), is deleted on sight. A missing comment on a subtle decision is obscurity; the judgment is the same in both directions: write exactly what the code cannot say, and nothing it already says.

2. No Args:/Returns:/Attributes: sections that restate typed signatures. The signature already says query: str and -> ToolResult. A parameter earns prose only when it carries a constraint the type cannot express: units, valid ranges beyond the validator, ownership or lifetime, cross-parameter coupling.

3. Present tense only. Code describes what is, never what was: no history voice ("Previously...", "used to be", "was renamed from", "This replaces the old X"), no PR or commit references, no provenance citations ("adapted from X's implementation"). Git history owns the past. Design Principle #6 already bans plan/phase references; this generalizes it to all archaeology. The mechanical Phase/Stage N ban applies to Python only - outside Python those words are domain vocabulary (a Docker build stage, a deploy workflow's role phases, a pipeline stage shown in the UI), and the checker leaves them alone.

4. One canonical home per invariant. State an invariant once, at its strongest enforcement point; every other site gets at most a one-line pointer to that home. Restating a rule at six sites means six copies drifting independently until they disagree.

5. Narration comments are an extraction signal. A long method segmented by # --- step 2: rerank --- banners is asking to become named methods. Extract, or file a refactor issue - don't keep the banner as a substitute for structure.

6. A behavior change includes its docstring in the diff. A stale docstring is a bug: it sends the next reader - human or model - confidently in the wrong direction. If a diff changes what a function does, the same diff updates the prose that describes it.

7. Runtime-consumed prose is exempt - edit it as contract content, never delete it as documentation. Field(description=...) strings are LLM-facing schema content. Connector @action method docstrings become the agent-facing tool description, and the connector_assistant skill's conventions govern them. Class docstrings of Pydantic models whose model_json_schema() reaches an LLM are prompt content - today the metadata-extraction schemas and agent tool args_schema models. API view docstrings and registered API model class docstrings are serialized into the generated OpenAPI/TS artifacts, so editing them requires re-running scripts/sync_openapi_types.sh. The non-Python analogs follow the same rule: Terraform description fields are operator-facing string values (the comment checker never sees them), and the leading comment block a shell script prints from its usage() function is runtime output - edit both as contract content. Template {% comment %} blocks are ordinary scanned prose, not an exemption.

Where shared code lives: campus_core/shared_utils/

Reusable cross-cutting utilities that don't belong to a single Django app live here. The bar for adding a new module: at least two real call sites, no Django-app dependencies (only django.conf + stdlib + third-party), and import-safe at startup (no DB access at import time).

When adding a new notification call site, use notify_workflow_event(channel_key=…, header=…, body=…, fields=…) from slack_utils.py. Channel keys resolve through settings.NOTIFICATION_CHANNELS. The helper never raises - Slack outages can't break a workflow.

Testing: prove behavior, not coverage

A change is done when a test asserts the observable contract it affects - "after ingest(folder_upload), typed Documents exist" - not when every new function has a matching test file. Helper-level tests are welcome where a helper has tricky logic worth guarding, but a passing helper (or unit) test is not evidence the application is healthy, the feature works, or the change didn't break something else - only a contract/seam test gives you that (see Design Principle #5). Don't over-mock the very collaborator whose contract you just changed; it'll stay green while production breaks. Tests live in campuscore_app/apps/<app_name>/tests/ (pytest for agent/service tests, Django TestCase for view/model tests; @pytest.mark.asyncio for async).