docs(styleguide): add AI Agent Checklist section against tech rot
This commit is contained in:
@@ -639,7 +639,149 @@ assert the warning is NOT emitted by `send_result()`.
|
||||
|
||||
---
|
||||
|
||||
## See Also
|
||||
## AI Agent Checklist (Added 2026-06-16)
|
||||
|
||||
This section is for AI agents writing code in this codebase. LLMs are
|
||||
trained on idiomatic Python (`try/except`, `Optional[T]`, `raise
|
||||
Exception`, etc.) which is the OPPOSITE of this convention. The
|
||||
checklist below catches the most common LLM mistakes. **Run this
|
||||
checklist before claiming a task is done.**
|
||||
|
||||
### The 5 MUST-DO rules
|
||||
|
||||
When writing NEW code, you MUST:
|
||||
|
||||
1. **Use `Result[T]` for any function that can fail at runtime.** A
|
||||
function that returns a different value under different runtime
|
||||
conditions (success vs. failure) returns `Result[T]`, not
|
||||
`Optional[T]`, not `T | None`, not a custom exception class. Use the
|
||||
`Result` dataclass from `src/result_types.py`; populate
|
||||
`errors: list[ErrorInfo]` on failure.
|
||||
|
||||
2. **Catch SDK exceptions at the boundary, convert to `ErrorInfo`.** If
|
||||
your code calls `anthropic`, `google.genai`, `openai`, `chromadb`,
|
||||
`requests`, or any other third-party SDK, the catch site
|
||||
converts the exception to `ErrorInfo(kind=..., message=...)` and
|
||||
returns it in `Result.errors`. Do NOT re-raise; do NOT swallow;
|
||||
do NOT let the exception propagate into internal code.
|
||||
|
||||
3. **Use nil-sentinel dataclasses for "no result".** If a function
|
||||
would return `None` in idiomatic Python, return a frozen
|
||||
`NilPath` / `NilRAGState` / etc. singleton from
|
||||
`src/result_types.py` instead. Callers don't need `if x is None:`
|
||||
checks; they can call `x.read_text` and get `""` on the nil path.
|
||||
|
||||
4. **Use `try/finally` (no except) for cleanup.** Bare
|
||||
`try: ...; finally: cleanup()` is the canonical `goto defer`
|
||||
pattern. Use it for resource cleanup, lock release, file handle
|
||||
close. Do NOT use `try/except` + pass for cleanup; the cleanup
|
||||
should run whether or not an exception occurred.
|
||||
|
||||
5. **`raise` is reserved for programmer errors.** `assert` for
|
||||
"this should never happen" invariants. `raise ValueError`,
|
||||
`raise NotImplementedError`, `raise KeyError` in `__init__` for
|
||||
"this object needs X." Do NOT use `raise` for runtime failures
|
||||
(the network is down, the file doesn't exist, the API rate-limited);
|
||||
those are `Result` cases.
|
||||
|
||||
### The 7 MUST-NOT-DO rules
|
||||
|
||||
When writing NEW code, you MUST NOT:
|
||||
|
||||
1. **DO NOT use `Optional[T]` as a return type** (in any file in
|
||||
`src/mcp_client.py`, `src/ai_client.py`, `src/rag_engine.py` —
|
||||
the 3 refactored files). Use `Result[T]` instead. CI fails if
|
||||
you add a new `Optional[T]` to those files (enforced by
|
||||
`scripts/audit_optional_in_3_files.py`).
|
||||
|
||||
2. **DO NOT use `Optional[T]` as a return type** (anywhere else in
|
||||
`src/`). The convention is migrating to `Result[T]`; new code
|
||||
should set the pattern, not perpetuate the old one. Argument
|
||||
types that may be `None` (caller choice) are still OK.
|
||||
|
||||
3. **DO NOT use `None` as a sentinel for "no result".** Use a
|
||||
nil-sentinel dataclass. The data is zero-initialized; the caller
|
||||
doesn't need a None check.
|
||||
|
||||
4. **DO NOT raise a custom exception class for runtime failures.**
|
||||
SDK exceptions caught and converted to `ErrorInfo` is the only
|
||||
legitimate exception path. Internal code uses `Result`.
|
||||
|
||||
5. **DO NOT use `Union[T, E]` (sum type).** Use `Result[T]` with
|
||||
side-channel `errors: list[ErrorInfo]`. The result is the data
|
||||
AND the errors, not a tagged sum.
|
||||
|
||||
6. **DO NOT catch `except Exception` and silently swallow.** Either
|
||||
narrow the exception type, convert to `ErrorInfo` in a `Result`,
|
||||
or document the intentional swallow with a comment-free `assert`
|
||||
for the precondition. The audit script flags this as
|
||||
`INTERNAL_SILENT_SWALLOW`.
|
||||
|
||||
7. **DO NOT catch `except Exception` in non-`*_result` code without
|
||||
conversion to `ErrorInfo`.** If you must catch, convert:
|
||||
`except SomeError as e: return Result(data=NIL_T, errors=[ErrorInfo(kind=INTERNAL, message=..., original=e)])`.
|
||||
The audit script flags this as `INTERNAL_BROAD_CATCH`.
|
||||
|
||||
### The 3 boundary patterns (where `try/except` IS the right answer)
|
||||
|
||||
These are the 3 categories where `try/except` is legitimate. See the
|
||||
"Boundary Types" section above for the full discussion.
|
||||
|
||||
1. **Third-party SDK calls.** Wrapping `anthropic.Anthropic().messages.create(...)`
|
||||
in `try/except anthropic.APIError` is the canonical pattern.
|
||||
Convert to `ErrorInfo`; return in `Result`.
|
||||
|
||||
2. **Stdlib I/O that can raise.** `open()`, `os.path.*`,
|
||||
`json.loads()`, `subprocess.run()`, `socket.*`, `sqlite3.*`,
|
||||
`chromadb.PersistentClient()` can all raise. Catch the specific
|
||||
exception (`OSError`, `FileNotFoundError`, `json.JSONDecodeError`,
|
||||
`subprocess.CalledProcessError`, etc.); convert to `ErrorInfo`.
|
||||
|
||||
3. **FastAPI `HTTPException` in `_api_*` handlers.** `raise
|
||||
HTTPException(status_code=..., detail=...)` in a function named
|
||||
`_api_*` is the FastAPI-idiomatic way to signal HTTP errors.
|
||||
FastAPI converts it to a JSON response at the framework level.
|
||||
This is NOT an exception leak; it's the framework contract.
|
||||
|
||||
### The pre-commit gate
|
||||
|
||||
Before claiming "done," you MUST run:
|
||||
|
||||
```bash
|
||||
uv run python scripts/audit_exception_handling.py
|
||||
```
|
||||
|
||||
If the script reports any `INTERNAL_*` (other than `INTERNAL_COMPLIANT`
|
||||
and `INTERNAL_PROGRAMMER_RAISE`) or `BOUNDARY_*` (other than
|
||||
`BOUNDARY_FASTAPI` in `_api_*` handlers), your code violates the
|
||||
convention. Fix it before committing. For CI use:
|
||||
|
||||
```bash
|
||||
uv run python scripts/audit_exception_handling.py --strict
|
||||
```
|
||||
|
||||
`--strict` exits 1 on any violation; use this in pre-commit hooks and
|
||||
CI to enforce the convention. The 4 enforcement audit scripts are:
|
||||
|
||||
- `scripts/audit_exception_handling.py --strict` (this one)
|
||||
- `scripts/audit_weak_types.py --strict` (the type-strengthening audit)
|
||||
- `scripts/audit_main_thread_imports.py` (always strict; the import graph gate)
|
||||
- `scripts/audit_no_models_config_io.py` (the config-I/O ownership gate)
|
||||
|
||||
All 4 are part of the convention enforcement. See
|
||||
`conductor/product-guidelines.md` "Data-Oriented Error Handling" and
|
||||
`docs/AGENTS.md` §"Convention Enforcement" for the project-level rules.
|
||||
|
||||
### Why this checklist exists
|
||||
|
||||
LLMs are trained on idiomatic Python. Without this checklist, an
|
||||
AI agent writing new code in this codebase will revert to idiomatic
|
||||
patterns (`try/except`, `Optional[T]`, `raise Exception`) — the
|
||||
"tech rot with idiomatic Python" the user is preventing. The
|
||||
checklist is the last line of defense. The audit scripts are the
|
||||
automated check; the checklist is the manual one.
|
||||
|
||||
---
|
||||
|
||||
- `conductor/tracks/data_oriented_error_handling_20260606/spec.md` — the spec
|
||||
that established this convention.
|
||||
|
||||
Reference in New Issue
Block a user