Skip to content
Closed
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
105 changes: 105 additions & 0 deletions .github/CODING_GUIDELINES.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
# Coding Guidelines

Standards for all contributors — human and AI — to the `copilot-usage` CLI.

## Type Safety

### Strict Static Typing

- **pyright `strict` mode is mandatory.** Every function parameter and return
value must have an explicit type annotation.
- Use `str | None` union syntax (PEP 604), not `Optional[str]`.
- Use `Final` for module-level constants that should never be reassigned.
- Use `Literal["a", "b"]` for constrained string values — not bare `str`.

### No Duck Typing

- Do not rely on implicit structural compatibility.
- If you need a shared interface, define a `Protocol` with explicit method
signatures.

### No Runtime Type Interrogation

- **No `getattr` / `hasattr`.** Access attributes directly through typed
references.
- **No `isinstance` checks as a substitute for proper typing.** If the type
system cannot express the constraint, refactor so it can.
Comment on lines +25 to +26
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The blanket ban on isinstance checks conflicts with existing boundary-parsing code (e.g. src/copilot_usage/parser.py uses isinstance to validate JSON-derived values). Consider scoping this rule to business logic only, or explicitly allowing runtime checks when converting untyped external data into typed models.

Suggested change
- **No `isinstance` checks as a substitute for proper typing.** If the type
system cannot express the constraint, refactor so it can.
- **Avoid `isinstance` checks in business logic.** Use static typing and data
models instead; reserve `isinstance` for validating untyped external data
at I/O boundaries (e.g. parsing JSON into typed models).

Copilot uses AI. Check for mistakes.
- **No `assert` for type validation.** Assertions are stripped in optimised
builds and are not a control-flow mechanism.

## Data Modelling

### Pydantic at the Boundary, Plain Python Internally

- External data (JSON files, API responses, CLI arguments) is validated with
**Pydantic** models.
- Internal intermediate state uses **frozen `dataclasses`** with `slots=True`.
- Prefer `dataclasses.dataclass(frozen=True, slots=True)` for immutable value
objects that never touch I/O.

### Defaults and Factories

- Use `default_factory=lambda: []` (not `default_factory=list`) for mutable
defaults — this avoids a known pyright false-positive.
Comment on lines +42 to +43
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This recommends default_factory=lambda: [] instead of default_factory=list for mutable defaults, but the codebase currently uses Field(default_factory=list) in Pydantic models (e.g. src/copilot_usage/models.py). If this guidance only applies to dataclasses.field, clarify that to avoid confusion, or align existing usage with the rule.

Suggested change
- Use `default_factory=lambda: []` (not `default_factory=list`) for mutable
defaults — this avoids a known pyright false-positive.
- For `dataclasses.field`, use `default_factory=lambda: []` (not
`default_factory=list`) for mutable defaults — this avoids a known pyright
false-positive.
- For Pydantic models, `Field(default_factory=list)` is acceptable and does
not need the lambda workaround.

Copilot uses AI. Check for mistakes.

## Naming and Structure

### Module-Level Organisation

- Private helpers that serve a single public function live in the same module,
prefixed with `_`.
- When a module grows beyond ~250 lines or serves multiple public consumers,
extract a `_<name>.py` private module.

### Import Conventions

- Standard library → third-party → local, separated by blank lines (enforced
by `ruff` isort rules).
- **`TYPE_CHECKING` is banned.** Do not use `from typing import
TYPE_CHECKING` or `if TYPE_CHECKING:` guards. Every import used in an
annotation must be a real runtime import. Circular imports are a design
bug — fix the module graph, do not hide cycles behind `TYPE_CHECKING`.
Comment on lines +58 to +61
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This guideline bans TYPE_CHECKING, but the current codebase already relies on it (e.g. src/copilot_usage/logging_config.py uses TYPE_CHECKING plus # noqa: F401, and coverage config excludes if TYPE_CHECKING:). Either relax this rule (and document the allowed pattern) or plan a refactor to remove existing TYPE_CHECKING usage and update tooling config accordingly.

Suggested change
- **`TYPE_CHECKING` is banned.** Do not use `from typing import
TYPE_CHECKING` or `if TYPE_CHECKING:` guards. Every import used in an
annotation must be a real runtime import. Circular imports are a design
bug — fix the module graph, do not hide cycles behind `TYPE_CHECKING`.
- **Avoid `TYPE_CHECKING` in new code.** Prefer real runtime imports for all
symbols used in annotations. Circular imports are a design bug — fix the
module graph rather than hiding cycles behind `TYPE_CHECKING`.
- Existing modules that already use `from typing import TYPE_CHECKING` and
`if TYPE_CHECKING:` guards (with `# noqa: F401` and excluded from coverage)
are allowed but should follow that pattern consistently until they can be
refactored to use acyclic runtime imports.

Copilot uses AI. Check for mistakes.

## Error Handling

- Catch **specific** exception types. Never use bare `except:` or
`except Exception:` unless re-raising.
- Prefer early returns to reduce nesting.

## Logging

- Use **loguru**, not stdlib `logging`.
- Log at `warning` for recoverable problems, `error` for failures that affect
output, `debug` for developer-facing tracing.

## Testing

- Tests use **pytest** with `--strict-markers`.
- Unit tests live in `tests/` and mirror the `src/` structure.
- E2E tests live in `tests/e2e/` and are excluded from the default unit run.
- Doctests are collected from `src/` via `--doctest-modules`.
- `assert` is fine inside tests (ruff rule `S101` is disabled for `tests/`).

## Security

- `flake8-bandit` (`S` rules) is enabled in ruff. Subprocess calls must use
absolute paths (resolved via `shutil.which`).
- Hardcoded credentials are allowed only in test fixtures (`S105`/`S106`
suppressed for `tests/`).

## Formatting

- **ruff format** with double quotes, 88-char line length, space indentation.
- Do not fight the formatter — let it own all whitespace decisions.

## Defensive Programming

- Guard clauses at the top of helper functions are acceptable even if currently
unreachable, as long as they make the function self-contained and safe to
call from future call sites.

## Verification Before Merge

- Run `make check` (lint → typecheck → security → test) locally before
pushing. CI mirrors this exactly.
- All existing tests must pass. Coverage must remain ≥ 80 %.
33 changes: 33 additions & 0 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# Copilot Instructions

Instructions for GitHub Copilot and Copilot-powered agents working in this
repository.

## Coding Standards

Read and follow [`.github/CODING_GUIDELINES.md`](CODING_GUIDELINES.md) for
all code changes. Key points:

- **pyright strict** — every parameter and return value must be annotated.
- **No duck typing** — no `getattr`, `hasattr`, or runtime type interrogation.
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This instruction bans runtime type checks, but the existing parser currently uses isinstance checks to safely coerce untyped JSON config/events into typed values. If the intent is to prohibit duck-typing in business logic, consider clarifying that boundary validation/parsing is exempt (or update the codebase to comply).

Suggested change
- **No duck typing**no `getattr`, `hasattr`, or runtime type interrogation.
- **No duck typing in business logic**do not use `getattr`, `hasattr`, or runtime type
interrogation in core logic. Boundary validation/parsing (for example, coercing untyped
JSON/config using `isinstance`) is allowed.

Copilot uses AI. Check for mistakes.
- **No `assert` for validation** — assertions are not control flow.
- **Pydantic at the boundary**, frozen dataclasses internally.
- **loguru** for logging, not stdlib `logging`.
- **ruff** for linting and formatting — do not fight the formatter.

## Workflow Rules

- **Never push to `main`.** Always create a branch and open a PR.
- **Never merge without maintainer approval.**
- **Run `make check` before pushing.** It runs lint, typecheck, security, and
tests — the same checks CI runs.

## Repository Layout

```
src/copilot_usage/ – Main package (CLI, parser, models, reports)
tests/ – Unit tests (mirrors src/ structure)
tests/e2e/ – End-to-end tests (excluded from unit run)
.github/workflows/ – CI and agent pipeline workflows
.github/agents/ – Agentic workflow agent definitions
```
27 changes: 27 additions & 0 deletions src/copilot_usage/_test_violations.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
"""Temporary test file — deliberate coding guideline violations.

This file exists solely to test whether Copilot code review reads
copilot-instructions.md and flags violations. DELETE after testing.
"""

from typing import TYPE_CHECKING

if TYPE_CHECKING:
from loguru import Logger
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from loguru import Logger is imported but never used. This will trip ruff F401 unless suppressed (the existing codebase uses # noqa: F401 for type-only imports) or the symbol is referenced in an annotation.

Suggested change
from loguru import Logger
from loguru import Logger # noqa: F401

Copilot uses AI. Check for mistakes.


Comment on lines +7 to +12
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TYPE_CHECKING blocks are introduced here, but the new CODING_GUIDELINES in this PR explicitly ban from typing import TYPE_CHECKING / if TYPE_CHECKING:. If these guidelines are meant to apply, remove the guard and resolve any import cycles by refactoring, or adjust the guidelines to allow type-only imports.

Suggested change
from typing import TYPE_CHECKING
if TYPE_CHECKING:
from loguru import Logger

Copilot uses AI. Check for mistakes.
def get_value(obj: object, key: str) -> str | None:
"""Get a value using getattr — violates guidelines."""
return getattr(obj, key, None)
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_value is annotated to return str | None, but getattr(obj, key, None) can return a non-str value at runtime and is also disallowed by the new guidelines. Prefer an explicitly typed interface (e.g., a Protocol or Mapping) and direct attribute access, or validate/convert the value before returning.

Suggested change
return getattr(obj, key, None)
value = getattr(obj, key, None)
if value is None:
return None
return str(value)

Copilot uses AI. Check for mistakes.


def check_type(value: object) -> bool:
"""Use hasattr — violates guidelines."""
if hasattr(value, "name"):
return True
return False
Comment on lines +18 to +22
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using hasattr here is both discouraged by the new guidelines and provides only a weak runtime check (it doesn't guarantee type/shape). Prefer passing a properly typed object (or Protocol) so attribute access is statically verified.

Copilot uses AI. Check for mistakes.


def validate(x: object) -> None:
"""Use assert for type validation — violates guidelines."""
assert isinstance(x, str), "must be a string"
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using assert for validation is unsafe because assertions can be stripped with python -O, so this function may perform no validation in production. Raise a real exception (TypeError/ValueError) or use a Pydantic model at the boundary instead.

Suggested change
assert isinstance(x, str), "must be a string"
if not isinstance(x, str):
raise TypeError("must be a string")

Copilot uses AI. Check for mistakes.
Loading