From 0a425c68796bcc469951b516b68e879eb76ffe9c Mon Sep 17 00:00:00 2001 From: Sasa Junuzovic Date: Sat, 28 Mar 2026 21:04:49 -0700 Subject: [PATCH] docs: add coding guidelines and copilot instructions (#110) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add .github/CODING_GUIDELINES.md with coding standards for all contributors (human and AI): type safety, data modelling, naming, error handling, testing, security, formatting. Add .github/copilot-instructions.md as a pointer — Copilot code review automatically reads this file and follows the link to the guidelines. Update all 6 agent workflow .md files (issue-implementer, review-responder, ci-fixer, quality-gate, code-health, test-analysis) to read and follow the guidelines. Remove TYPE_CHECKING from logging_config.py — replaced with runtime import loguru + string annotation, consistent with the new ban. Remove stale TYPE_CHECKING coverage exclusion from pyproject.toml. Update architecture.md and changelog.md to reflect the change. Closes #110 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/CODING_GUIDELINES.md | 113 +++++++++++++++++++++++++ .github/copilot-instructions.md | 14 +++ .github/workflows/ci-fixer.md | 2 + .github/workflows/code-health.md | 2 + .github/workflows/issue-implementer.md | 2 + .github/workflows/quality-gate.md | 2 + .github/workflows/review-responder.md | 2 + .github/workflows/test-analysis.md | 2 + pyproject.toml | 1 - src/copilot_usage/docs/architecture.md | 2 +- src/copilot_usage/docs/changelog.md | 2 +- src/copilot_usage/logging_config.py | 5 +- 12 files changed, 142 insertions(+), 7 deletions(-) create mode 100644 .github/CODING_GUIDELINES.md create mode 100644 .github/copilot-instructions.md diff --git a/.github/CODING_GUIDELINES.md b/.github/CODING_GUIDELINES.md new file mode 100644 index 00000000..2839408e --- /dev/null +++ b/.github/CODING_GUIDELINES.md @@ -0,0 +1,113 @@ +# 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 in business logic.** Use static typing and data + models instead. `isinstance` is allowed at I/O boundaries when coercing + untyped external data (e.g., JSON values) into typed models. +- **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) is validated with + **Pydantic** models. CLI arguments are validated by **Click** at the + boundary. +- 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 + +- In `dataclasses.field`, use `default_factory=lambda: []` (not + `default_factory=list`) for mutable defaults — this avoids a known pyright + false-positive. +- In Pydantic `Field`, `default_factory=list` is fine — Pydantic handles + the typing correctly. + +## 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 `_.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`. + +## Error Handling + +- Catch **specific** exception types. Never use bare `except:` or + `except Exception:` unless re-raising. +- **Exception:** Top-level event loops (e.g., TUI render loops) may catch + `Exception` without re-raising when crash recovery is intentional, provided + `KeyboardInterrupt` is handled separately. +- 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 runs these checks plus diff-coverage on changed lines. +- All existing tests must pass. Coverage must remain ≥ 80 %. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 00000000..74ecb92a --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,14 @@ +# Copilot Instructions + +Read and follow [`.github/CODING_GUIDELINES.md`](CODING_GUIDELINES.md) for +all code changes. + +## 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 +``` diff --git a/.github/workflows/ci-fixer.md b/.github/workflows/ci-fixer.md index bcb7fa9e..f7fa2522 100644 --- a/.github/workflows/ci-fixer.md +++ b/.github/workflows/ci-fixer.md @@ -50,6 +50,8 @@ Fix CI failures on pull request #${{ github.event.inputs.pr_number }}. ## Instructions +Read and follow the coding standards in `.github/CODING_GUIDELINES.md` for all code changes. + 1. First, check if PR #${{ github.event.inputs.pr_number }} has the label `aw-ci-fix-attempted`. If it does, add a comment saying "CI fix already attempted once — stopping to prevent loops. Manual intervention needed." and stop. Do NOT attempt another fix. 2. Add the label `aw-ci-fix-attempted` to the PR. diff --git a/.github/workflows/code-health.md b/.github/workflows/code-health.md index 7e9c5080..c8f58ecb 100644 --- a/.github/workflows/code-health.md +++ b/.github/workflows/code-health.md @@ -32,6 +32,8 @@ Analyze the entire codebase for cleanup opportunities and open issues for anythi ## Instructions +Read `.github/CODING_GUIDELINES.md`. Flag any violations of those standards in existing code as cleanup opportunities. + Read all files in the repository. Read all open issues in the repository. Identify genuine cleanup opportunities — refactoring, dead code, inconsistencies, stale docs, dependency hygiene, or anything else that would make the codebase meaningfully better. For each finding, open an issue with root cause analysis and a clear spec for resolving it. Each issue must include a testing requirement — regression tests for bugs, coverage for new functionality. Prefix each issue title with `[aw][code health]` and label each issue with `aw` and `code-health`. diff --git a/.github/workflows/issue-implementer.md b/.github/workflows/issue-implementer.md index ca3e4579..810c3d35 100644 --- a/.github/workflows/issue-implementer.md +++ b/.github/workflows/issue-implementer.md @@ -45,6 +45,8 @@ Read the issue specified by the input, understand the problem, implement the sol ## Instructions +Read and follow the coding standards in `.github/CODING_GUIDELINES.md` for all code you write. + Read all files in the repository. Read issue #${{ github.event.inputs.issue_number }} to understand what needs to be fixed. Implement the fix following the spec in the issue, including any testing requirements. Before committing, run the full CI check suite locally: diff --git a/.github/workflows/quality-gate.md b/.github/workflows/quality-gate.md index cf48ae71..2c72a051 100644 --- a/.github/workflows/quality-gate.md +++ b/.github/workflows/quality-gate.md @@ -50,6 +50,8 @@ Evaluate pull request #${{ inputs.pr_number }} for autonomous merge eligibility. ## Instructions +Read `.github/CODING_GUIDELINES.md` and verify that the PR's code changes comply with those standards. + This workflow is dispatched by the pipeline orchestrator when a PR has CI green and all review threads resolved. 1. Fetch the PR details for PR #${{ inputs.pr_number }}. Verify it has the `aw` label. If not, stop immediately. diff --git a/.github/workflows/review-responder.md b/.github/workflows/review-responder.md index 3c40f864..73d35c87 100644 --- a/.github/workflows/review-responder.md +++ b/.github/workflows/review-responder.md @@ -54,6 +54,8 @@ Address review comments on pull request #${{ inputs.pr_number }}. ## Instructions +Read and follow the coding standards in `.github/CODING_GUIDELINES.md` for all code changes. + This workflow addresses unresolved review comments on a pull request. 1. Check if the PR already has the label `aw-review-response-attempted`. If it does, add a comment to the PR saying "Review response already attempted — stopping to prevent loops. Manual intervention needed." and stop. diff --git a/.github/workflows/test-analysis.md b/.github/workflows/test-analysis.md index 0770209d..5e16610d 100644 --- a/.github/workflows/test-analysis.md +++ b/.github/workflows/test-analysis.md @@ -32,6 +32,8 @@ Analyze the test suite for coverage gaps and suggest new tests. ## Instructions +Read `.github/CODING_GUIDELINES.md` for context on the project's coding standards. + Read all files in the repository. Read all open issues in the repository. Identify meaningful test gaps across unit tests, e2e tests, and integration tests — untested code paths, missing scenarios, weak assertions, or anything else that would improve confidence in the code. For each area with gaps, open an issue with root cause analysis, repro steps where applicable, and a clear spec for what tests to add. Each issue must specify the expected behavior to assert and any regression scenarios to cover. Prefix each issue title with `[aw][test audit]` and label each issue with `aw` and `test-audit`. diff --git a/pyproject.toml b/pyproject.toml index b991b83f..48fd3517 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -86,5 +86,4 @@ fail_under = 80 show_missing = true exclude_lines = [ "pragma: no cover", - "if TYPE_CHECKING:", ] diff --git a/src/copilot_usage/docs/architecture.md b/src/copilot_usage/docs/architecture.md index fabf6c28..d349929a 100644 --- a/src/copilot_usage/docs/architecture.md +++ b/src/copilot_usage/docs/architecture.md @@ -40,7 +40,7 @@ Monorepo containing Python CLI utilities that share tooling, CI, and common depe | `render_detail.py` | Session detail rendering — extracted from report.py. Displays event timeline, per-event metadata, and session-level aggregates. | | `_formatting.py` | Shared formatting utilities — `format_duration()` and `format_tokens()` with doctest-verified examples. Used by report.py and render_detail.py. | | `pricing.py` | Model pricing registry — multiplier lookup, tier categorization. Multipliers are used for `~`-prefixed cost estimates in live/active views (`render_live_sessions`, `render_cost_view`); historical post-shutdown views use exact API-provided numbers exclusively. | -| `logging_config.py` | Loguru setup — stderr warnings only, no file output. `loguru` module import guarded behind `TYPE_CHECKING` for the `"loguru.Record"` type annotation. Called once from CLI entry point. | +| `logging_config.py` | Loguru setup — stderr warnings only, no file output. Runtime `import loguru` for pyright to resolve the `"loguru.Record"` string annotation. Called once from CLI entry point. | ### Event Processing Pipeline diff --git a/src/copilot_usage/docs/changelog.md b/src/copilot_usage/docs/changelog.md index 3e34d714..509b568d 100644 --- a/src/copilot_usage/docs/changelog.md +++ b/src/copilot_usage/docs/changelog.md @@ -27,7 +27,7 @@ Manual work only — autonomous agent pipeline PRs are not tracked here. **Plan**: Fix three `aw-protected-files` issues that autonomous agents cannot touch. **Done**: -- **#291**: Wrapped bare `import loguru` in `TYPE_CHECKING` guard in `logging_config.py` +- **#291**: Replaced `TYPE_CHECKING` guard with runtime `import loguru` in `logging_config.py` - **#330**: Added `exclude = ["src/copilot_usage/docs"]` to hatchling wheel config; created `tests/test_packaging.py` using `shutil.which("uv")` for S607 compliance - **#352**: Restored doctest examples in `_formatting.py`, enabled `--doctest-modules` with `testpaths = ["tests", "src"]` - Updated Makefile: unit test targets use `--ignore=tests/e2e` instead of explicit paths, letting `pyproject.toml` `testpaths` guide collection diff --git a/src/copilot_usage/logging_config.py b/src/copilot_usage/logging_config.py index 7487b257..1ffe7c65 100644 --- a/src/copilot_usage/logging_config.py +++ b/src/copilot_usage/logging_config.py @@ -1,13 +1,10 @@ """Logging configuration — console-only for CLI tool.""" import sys -from typing import TYPE_CHECKING +import loguru from loguru import logger -if TYPE_CHECKING: - import loguru # noqa: F401 — for pyright to resolve "loguru.Record" - LEVEL_EMOJI: dict[str, str] = { "TRACE": "🔍", "DEBUG": "🐛",