Skip to main content

Coding standards

These conventions are enforced by code review, CI lint, and (where possible) automated checks. Read them before your first PR — fixing them late costs cycles.

Audience

All contributors. Apply on every PR, including chore and docs PRs that touch code blocks.

TypeScript — strict mode, no any​

apps/frontend/tsconfig.json runs with "strict": true and "noImplicitAny": true. Concretely:

  • No any casts. If you reach for any, you are usually missing a type guard or a generic. Use unknown and narrow it deliberately.
  • No non-null assertions (!) unless the value is provably non-null at that point and a comment explains why.
  • Discriminated unions over enums. Enums leak transitive imports; literal unions (type Status = "pending" | "running" | "succeeded") are tree-shakable.

Justified narrowing example:

// `data` comes from the server as JSON; runtime-validate before use.
function isProject(value: unknown): value is Project {
  return (
    typeof value === "object" &&
    value !== null &&
    "id" in value &&
    typeof (value as { id: unknown }).id === "string"
  );
}

If any is genuinely unavoidable (e.g. interop with an untyped third-party callback), wrap it in a typed boundary function and add a // eslint-disable-next-line @typescript-eslint/no-explicit-any with a one-line justification.

Pydantic v2 — BaseModel + Field(...)​

The backend is on Pydantic v2. Schemas live in apps/backend/schemas/.

  • Always declare types. Field(...) for required fields, Field(default=...) or Field(default_factory=...) for defaults.
  • Use model_validator for cross-field invariants. Avoid raising in __init__; the validator runs at the right time and produces structured errors.
  • Avoid arbitrary_types_allowed. It bypasses validation. Wrap third-party types in a custom validator instead.
from pydantic import BaseModel, Field, model_validator

class ProjectCreate(BaseModel):
    name: str = Field(min_length=1, max_length=200)
    repository_url: str = Field(pattern=r"^(https?|ssh)://")
    visibility: Literal["team-only", "org-wide"] = "team-only"

    @model_validator(mode="after")
    def visibility_requires_team_admin(self) -> "ProjectCreate":
        # cross-field check goes here; raise ValueError on violation
        return self

Alembic — forward-only​

The migration policy is forward-only. downgrade() is not implemented:

def upgrade() -> None:
    op.add_column("projects", sa.Column("watch_list", sa.ARRAY(sa.String())))
    op.execute("UPDATE projects SET watch_list = ARRAY[]::text[]")

def downgrade() -> None:
    raise NotImplementedError("forward-only migrations")

Three consequences:

  • Breaking column changes are 3-stage. Adding NOT NULL, dropping a column, or renaming requires expand (add new column nullable) → migrate data (a separate revision or one-shot Celery task) → contract (drop old column / set NOT NULL). Never combine in one revision.
  • Schema and data migrations are separate revisions. A schema revision should not embed a bulk_insert more than a few rows. For larger data shifts, write a one-shot Celery task that is idempotent and queue it from a separate data_xxxx_* revision.
  • New table? Decide the runtime grant. If the trustedoss_app runtime role needs to UPDATE or DELETE rows in the table, add an explicit GRANT UPDATE, DELETE ON <table> TO trustedoss_app to the same migration's upgrade(). If the table is append-only (INSERT only), no grant is needed, but update apps/backend/tests/fixtures/app_role_privileges.json to declare it. Otherwise test_app_role_grant_matrix.py fails and tells you which of the two to do.

Adding an index to a large table - CREATE INDEX CONCURRENTLY​

alembic/env.py's run_migrations_online() wraps every migration in one pg_advisory_xact_lock-guarded transaction, so two alembic upgrade head runs racing during a rollout serialise instead of corrupting the schema. A plain op.create_index(...) inherits that transaction, which is correct and free almost always: Postgres holds a lock for the duration, but the DDL itself is fast on any table small enough that nobody would call it "large."

CREATE INDEX CONCURRENTLY is the exception, and it does not fit inside a transaction at all (Postgres refuses it: CREATE INDEX CONCURRENTLY cannot run inside a transaction block). If a table has grown to where an ordinary index build's lock would stall production traffic for the build's duration (audit_logs is the one large enough to matter today), the index migration needs Alembic's own escape hatch:

def upgrade() -> None:
    with op.get_context().autocommit_block():
        op.create_index(
            "ix_audit_logs_actor_user_id",
            "audit_logs",
            ["actor_user_id"],
            postgresql_concurrently=True,
        )

autocommit_block() commits the ambient transaction, runs the enclosed DDL in Postgres autocommit mode, then opens a fresh transaction for whatever follows in the same revision. That has one consequence worth being deliberate about: committing the ambient transaction releases the advisory lock pg_advisory_xact_lock was holding, before the CONCURRENTLY build starts. The concurrency guard that makes a K8s rollout's N replicas race-safe does not cover this migration once it reaches the block: a second alembic upgrade head starting at the same moment would not wait for it.

So a revision containing an autocommit_block() is not "just another migration" to run alongside a normal rollout. Run it by hand, alone, during a maintenance window:

  1. Confirm nothing else will invoke alembic upgrade head concurrently for the duration (pause the deploy pipeline / scale the rollout to one replica running migrations).
  2. alembic upgrade <this revision> targeting exactly this revision, not head, so its blast radius is visible in the command itself.
  3. Watch pg_stat_progress_create_index while it runs; a CONCURRENTLY build that is interrupted (connection drop, statement timeout) can leave an INVALID index behind, which Postgres will not use and will not silently rebuild, so DROP INDEX CONCURRENTLY it and re-run.
  4. Only then resume the normal rollout for the rest of the deploy.

Python dependencies - edit the .txt, regenerate the .lock​

requirements.txt and requirements-dev.txt (direct dependencies, each pin carrying a comment explaining why) are what a person edits. The Dockerfiles install from requirements.lock / requirements-dev.lock instead (pip install --require-hashes -r ...): every direct AND transitive dependency pinned to an exact version and wheel/sdist hash, generated by pip-compile so a build cannot silently pick up a newer transitive release and an image's SBOM / SLSA provenance claims are actually reproducible from source.

After changing either .txt file, regenerate its .lock in the same commit:

cd apps/backend
pip install pip-tools
pip-compile --generate-hashes --output-file=requirements.lock requirements.txt
pip-compile --generate-hashes --output-file=requirements-dev.lock requirements-dev.txt

lint (backend) regenerates both and diffs them against what you committed; a stale .lock fails the PR with the exact command to fix it. Regenerate to the SAME --output-file name shown above: pip-compile stamps its own invocation into the file's header comment, and a different output path would make the diff "stale" on that line alone, regardless of whether the actual pins changed.

RFC 7807 — application/problem+json​

Every 4xx and 5xx response uses application/problem+json. The base shape:

{
  "type": "https://trustedoss.dev/problems/forbidden",
  "title": "Forbidden",
  "status": 403,
  "detail": "API key 'tos_a1b2c3d4_…' lacks the scan:trigger permission.",
  "instance": "/v1/projects/42/scans"
}
  • type is a stable URI, even if the URI does not resolve. We treat it as a machine-readable error code.
  • title is a short human-readable summary; do not interpolate user input.
  • detail may interpolate user input but never leak secrets.
  • instance is the URL path that produced the error (request-id is in the X-Request-ID header, not the body).

Domain-specific extensions are snake_case and registered as Pydantic models so they appear in OpenAPI:

class GateFailedProblem(Problem):
    type: Literal["https://trustedoss.dev/problems/gate-failed"]
    failing_components: list[str]
    license_findings: int
    cve_findings: int

The conversion from exception to Problem happens in a single FastAPI exception_handler. Do not return raw HTTPException from routes.

structlog — JSON lines, context-propagated​

Logs are JSON, one event per line. structlog is configured in apps/backend/core/logging.py. Every log line carries:

  • request_id — X-Request-ID from the inbound header, or a UUIDv7 minted by the middleware.
  • user_id, team_id — set by the auth middleware, None for unauthenticated calls.
  • task_name, task_id: bound in the worker for every task. Before, 22 task modules bound task_name by hand and 12 bound task_id, so the ten that bound neither had no way to say what had run.

In a worker, request_id is the id of the request that dispatched the task. It is copied onto the message at dispatch and bound again on the other side by the signal handlers in apps/backend/tasks/log_context.py, so the HTTP request and the scan it started share one id. Tasks that beat dispatches have no request behind them and carry no request_id; that absence is meaningful and is not filled in with a substitute.

Anything beyond those fields belongs to the task and is the task's to bind and unbind: scan_id in the scan pipeline, dry_run in the sweeps. The handlers clear only what they bound.

Use the binder pattern:

log = structlog.get_logger().bind(component="scan-pipeline")

async def run_scan(scan_id: str) -> None:
    bound = log.bind(scan_id=scan_id)
    bound.info("scan.start")
    ...
    bound.info("scan.finish", duration_ms=duration_ms)

PII masking​

Never log PII in plaintext. Pass values through mask_pii():

from core.security import mask_pii

log.info("auth.login.success", email=mask_pii(email))
# emits: { ..., "email": "h****@n****.com" }

The masked categories are: email, full names, raw tokens, API key full secrets (the prefix is fine), webhook URLs, and OAuth code query parameters.

i18n keys — <feature>.<screen>.<element> kebab-case​

Every UI string lives in apps/frontend/src/locales/{en,ko}/<namespace>.json. Keys follow:

notifications.inbox.empty-state-title
notifications.inbox.empty-state-body
notifications.preferences.toggle-email-tooltip

Rules:

  • Three segments minimum: feature → screen → element. Rare edge case (a global helper) may use two.
  • Kebab-case within a segment. Avoid camelCase or snake_case.
  • EN and KO mirror each other. Adding a key in EN without the corresponding KO key fails the i18next-parser drift gate in CI.
  • No string concatenation. Use ICU placeholders: "badge.unread-count": "{{count}} unread".

When you remove a key from a component, the parser drift gate also catches the orphan in EN/KO files; remove it from both.

Server-generated output is English-only​

The rule above covers apps/frontend's UI strings, which is the ENTIRE localization surface. Everything the backend generates on its own, notification titles/bodies (notifications/email.py, notifications/slack.py, notifications/teams.py), Excel report headers (services/report_xlsx_service.py), PDF report text, and RFC 7807 title/detail text on every error response, is hardcoded English and stays that way. There is no backend i18n framework: no gettext/babel, and nothing in the request path reads Accept-Language.

This is a decision, not a gap someone forgot to close: translating generated documents and outbound messages means shipping and maintaining a second translation surface with its own extraction tooling, review process, and drift gate, for output most of it (audit exports, webhook payloads, admin-triggered reports) is read by an operator rather than the person who set their UI language to Korean in the first place. If a real demand for localized reports or notifications shows up, revisit this rather than translating ad hoc; a half-translated report (some sheets EN, some KO, numbers and dates still formatted for whichever locale the code happened to hardcode) is worse than a consistently English one.

# nosec and # nosemgrep — justify in line​

Static analysis runs bandit (Python) and semgrep (multi-language) in CI. SAST is hard-fail on High+. Suppress only when the finding is provably a false positive, and justify on the line:

data = pickle.loads(blob)  # nosec B301: blob comes from the local backup volume, owned by root, never user-supplied
expression = re.compile(user_input)  # nosemgrep: regex-from-user-input: validated upstream by `is_safe_regex()`

The format is:

# nosem grep: <rule-id>: <one-sentence justification>

Reviewers reject suppressions that lack a justification or whose justification does not address the rule. If multiple lines need the same suppression, lift the offending logic into a single function and suppress once.

secret-scan reads history, not your files​

gitleaks detect scans the commits reachable from your branch's HEAD. It does not scan the checked-out tree. That distinction is invisible until the check fails, and then it explains a failure that otherwise looks impossible.

Deleting the value does not clear the finding. The commit that introduced it is still on the branch, so the scan still sees it. If you remove the value, commit, push, and the check fails again with the same finding, nothing went wrong with your fix: it was never the problem being reported. Somebody has already lost two rounds of CI to this, editing the working tree a second and third time.

Another contributor's branch cannot fail your pull request. fetch-depth: 0 reads as "fetch everything", but it means all of this ref's history: actions/checkout fetches only the pull-request ref, so no other branch is in the checkout to be read. The run logs bear it out, with each pull request scanning main's commit count plus its own rather than the whole repository.

To clear a finding, the commit carrying it has to go. From a clean tree, with nobody else building on your branch:

git fetch origin
git rebase origin/main       # FIRST, before collapsing anything
git reset --soft origin/main # your whole diff becomes one staged change
git commit                   # one commit, without the value
git push --force-with-lease

Rebase before the reset. git reset --soft origin/main moves your branch to wherever origin/main points now while keeping your working tree, so if main advanced after you branched, the single commit you create reverts everything merged in between. It happens silently, because your own files are all still correct. This has happened in this repository: 125 lines from two merged pull requests were undone that way, and the only clue was a diffstat showing far more deletions than the change could account for. Before committing, read git diff --stat origin/main, and confirm git diff --name-only origin/main lists nothing you did not touch.

If the value was a real credential, rotate it. It reached a remote, so treat it as disclosed regardless of what the history looks like afterwards. Rewriting history hides it from the next scan; it does not un-publish it.

Only add a value to .gitleaks.toml when you can prove it is not a secret, and record the proof in the comment beside it. "It is only used in CI" is context, not proof.

AI review comments — advisory, never a gate​

A pull request may pick up a comment headed AI security review (findings-driven). It comes from .github/workflows/ai-review.yml, which re-runs semgrep over the files you changed — without the severity filter and the --error flag the SAST gate uses — and asks a model to sort what it flagged into true and false positives.

Treat it as a reading order, not a verdict. It does not block the merge and it must never be added to branch protection; secret-scan and sast are the checks that gate. A verdict of "false positive" is not grounds to skip a fix, and a verdict of "true positive" is not grounds to merge a suppression without the in-line justification described above.

Two things follow from where its input comes from. The comment quotes code and tool messages taken from the pull request, so on a pull request from a fork it is a stranger's text: read the verdicts, not any instructions embedded in them. And the workflow is dormant unless the repository has an ANTHROPIC_API_KEY secret — with no key every step skips, the job passes, and nothing is called. If you see no comment, that is the expected state today.

The parsing, caps, and rendering behind it live in tools/ai-review/; python tools/ai-review/selftest.py exercises them offline and runs on every PR as part of lint (backend).

MCP servers and agent tools​

Much of this repository is written with an AI coding agent, and an agent's tool calls never pass through a pull request. A Model Context Protocol (MCP) server hands its tool descriptions to the agent as context and can reach services outside the repository, so a server added quietly in a local configuration is an input to the build that nobody reviewed.

.mcp.json at the repository root declares the approved servers. It is committed and it is currently empty. Adding a server means editing that file in a pull request rather than only a local setting, so the addition arrives as a diff someone can read.

Establish four things before a server lands, and record the outcome in the pull request:

  • Origin. Publisher, the repository it is built from, recent commit activity. Listing in a marketplace or a registry is not a review.
  • Tool descriptions. Read them. They enter the agent's context as instructions, so a description carrying hidden directives has the effect of an edited system prompt.
  • Data egress. Which external endpoints the server talks to, and whether anything from this repository can leave through them.
  • Version. Pin it. A package that was clean when it was approved can change in a later release, which is how published MCP servers have turned malicious.

Read the tool descriptions again when you move the pin, because a server update can rewrite them.

Scan a candidate before adopting it (Snyk's agent-scan, or Cisco's mcp-scanner). Both execute the server in order to inspect it, so run them in a container or a throwaway environment, not on the machine you work from.

Agent skills and IDE extensions get the same treatment. They run with your permissions, they appear in no lockfile, and no SCA run in this repository will see them. Re-check what you have installed once a quarter.

Frontend UI — use the shared primitives​

The frontend has a single design system; the Design system reference is the source of truth for tokens, components, and motion. Do not hand-roll what a primitive already covers:

  • Page headers — render every route header through PageHeader (stacked / bar). Never write a bare <header><h1> (the breadcrumb detail pages are the one documented exception).
  • Typography — use the typography.tsx primitives (PageTitle / SectionTitle / Subtitle / Body / Caption / Eyebrow); reach for a raw text-* utility only for one-off inline spans.
  • Feedback — useToast() for success / non-blocking notices; keep form-validation errors inline (RFC 7807 detail). Toasts carry the data-toast-key e2e contract — pass a stable key.
  • Empty / loading — EmptyState (layered medallion) and the composite skeletons in skeletons.tsx, not ad-hoc bars.
  • Colour — reference tokens / Tailwind utilities (bg-card, text-risk-high), never a hex literal in a component.

Eyeball every primitive at /dev/design-preview (the living reference); new shared primitives should be added there.

See also​