sandcastle: populate CODING_STANDARDS.md (Rust, evie-specific) #12

New issue

Closed

opened 2026-07-05 03:21:27 +08:00 by weiwen · 0 comments

weiwen commented

2026-07-05 03:21:27 +08:00

Owner

Blocked by: #9, #10

Why

.sandcastle/CODING_STANDARDS.md is loaded by the reviewer agent (@.sandcastle/CODING_STANDARDS.md) but is still the empty template — and its examples are JS/TS (camelCase, "named exports"), actively misleading for a Rust codebase.

Design decisions (settled)

Role: supplement. review-prompt.md already carries generic craft guidance (nesting, ternaries, redundancy, security). This file must NOT repeat it — only evie-specific, opinionated rules grounded in src/.
Rust-only. The reviewer only sees Rust (issues → Rust features). Delete the JS/TS template examples.
Defers to tooling, with teeth. Don't restate what clippy/rustfmt enforce.
Descriptive (codify existing conventions) with a tiny prescriptive tail.

What to write

Rewrite the file with these sections:

Preamble / teeth: Before hand-review, run nix develop .#ci -c just check; fmt/clippy/test failures are findings. Don't hand-review anything a linter owns.
Error handling: anyhow::Result + .context() on app/binary paths; thiserror enums (SessionError, ApiError) only at interface surfaces callers match on. No new error enum where anyhow suffices.
Domain language: use the terms defined in CONTEXT.md (Session, Chat ID, Stream Buffer, …) and avoid the listed synonyms. Link to CONTEXT.md; do not restate it.
Config changes are cross-cutting: a new config field must update default-config generation, validation, README.md, and CONTEXT.md in the same change.
Module placement: land code in the module CONTEXT.md's architecture map assigns to that concern; keep pure logic (e.g. markdown.rs) pure and pub(super).
Testing: pure synchronous logic (markdown paginate/render, config parsing/validation) must have unit/totality tests. Async orchestration / I-O glue (session, telegram, http, pi spawning) is exempt from unit tests — BUT when logic is buried in glue and can't be tested cleanly, extract the pure core into a deep, synchronous unit and test that (cf. 2bacb57 refactor: extract RPC parser for testability). The exemption is not a license to leave untestable logic in handlers.
Footgun (prescriptive): never hold a lock across .await in SessionManager.

Keep it short and high-signal. Every rule must be checkable against a diff.

Acceptance criteria

No JS/TS references remain.
File contains the 7 sections above, phrased as concrete, diff-checkable rules.
No duplication of review-prompt.md's generic guidance.

Note

Depends on #10 (so just check exists) and #9; only fully real once #11 rebuilds the image. The doc can be authored ahead of the image rebuild.

Blocked by: #9, #10 ## Why `.sandcastle/CODING_STANDARDS.md` is loaded by the reviewer agent (`@.sandcastle/CODING_STANDARDS.md`) but is still the empty template — and its examples are JS/TS (`camelCase`, "named exports"), actively misleading for a Rust codebase. ## Design decisions (settled) - **Role: supplement.** `review-prompt.md` already carries generic craft guidance (nesting, ternaries, redundancy, security). This file must NOT repeat it — only evie-specific, opinionated rules grounded in `src/`. - **Rust-only.** The reviewer only sees Rust (issues → Rust features). Delete the JS/TS template examples. - **Defers to tooling, with teeth.** Don't restate what `clippy`/`rustfmt` enforce. - **Descriptive** (codify existing conventions) with a tiny prescriptive tail. ## What to write Rewrite the file with these sections: 1. **Preamble / teeth:** Before hand-review, run `nix develop .#ci -c just check`; fmt/clippy/test failures are findings. Don't hand-review anything a linter owns. 2. **Error handling:** `anyhow::Result` + `.context()` on app/binary paths; `thiserror` enums (`SessionError`, `ApiError`) only at interface surfaces callers match on. No new error enum where `anyhow` suffices. 3. **Domain language:** use the terms defined in `CONTEXT.md` (Session, Chat ID, Stream Buffer, …) and avoid the listed synonyms. Link to `CONTEXT.md`; do not restate it. 4. **Config changes are cross-cutting:** a new config field must update default-config generation, validation, `README.md`, and `CONTEXT.md` in the same change. 5. **Module placement:** land code in the module `CONTEXT.md`'s architecture map assigns to that concern; keep pure logic (e.g. `markdown.rs`) pure and `pub(super)`. 6. **Testing:** pure synchronous logic (markdown paginate/render, config parsing/validation) must have unit/totality tests. Async orchestration / I-O glue (session, telegram, http, pi spawning) is exempt from unit tests — BUT when logic is buried in glue and can't be tested cleanly, extract the pure core into a deep, synchronous unit and test that (cf. `2bacb57 refactor: extract RPC parser for testability`). The exemption is not a license to leave untestable logic in handlers. 7. **Footgun (prescriptive):** never hold a lock across `.await` in `SessionManager`. Keep it short and high-signal. Every rule must be checkable against a diff. ## Acceptance criteria - No JS/TS references remain. - File contains the 7 sections above, phrased as concrete, diff-checkable rules. - No duplication of `review-prompt.md`'s generic guidance. ## Note Depends on #10 (so `just check` exists) and #9; only fully real once #11 rebuilds the image. The doc can be authored ahead of the image rebuild.

weiwen added the

ready-for-agent

label

2026-07-05 03:40:25 +08:00

weiwen referenced this issue from a commit

2026-07-05 06:22:40 +08:00

RALPH: populate CODING_STANDARDS.md (Rust, evie-specific) — closes #12

weiwen referenced this issue from a pull request that will close it,

2026-07-05 06:22:51 +08:00

sandcastle: populate CODING_STANDARDS.md (Rust, evie-specific) #16

weiwen

2026-07-05 06:22:55 +08:00