diff --git a/AGENTS.md b/AGENTS.md index 65e07a7..1a22ac0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,45 +1,13 @@ # AGENTS.md +This file is zfin's source-of-truth for assistant agents. It +holds project-specific architecture, conventions, and gotchas +plus the universal hard rules that apply on top. + +Read the ABSOLUTE PROHIBITIONS section first. + ## ⛔ ABSOLUTE PROHIBITIONS — READ FIRST ⛔ -### Zig 0.16.0 reference — read the release notes - -This codebase is on Zig 0.16.0. The 0.16 release was a major -I/O-as-an-interface refactor that reshaped the standard library. -Before making non-trivial changes (especially anything touching -`std.Io`, `std.fs`, `std.process`, `std.http`, `std.Thread`, -allocators, or `std.time`), **read the release notes** at: - - https://ziglang.org/download/0.16.0/release-notes.html - -Key migrations that bit us repeatedly during the 0.16 upgrade and -will bite future work too: - -- `std.io` → `std.Io` (namespace rename, no deprecation alias). -- `std.fs.cwd()` → `std.Io.Dir.cwd()`. All file ops take `io`. -- `std.process.Child.init(argv, alloc)` → `std.process.spawn(io, .{...})` - or `std.process.run(gpa, io, .{...})`. -- `std.time.timestamp()` → `std.Io.Timestamp.now(io, .real).toSeconds()`. -- `std.Thread.sleep(ns)` → `std.Io.sleep(io, duration, clock)`. -- `pub fn main` gains a `std.process.Init` parameter ("Juicy Main"); - provides pre-built `gpa`, `io`, `arena`, `environ_map`. -- `std.heap.GeneralPurposeAllocator` → `std.heap.DebugAllocator`. -- `std.heap.ThreadSafeAllocator` removed; `ArenaAllocator` is now - lock-free thread-safe, `DebugAllocator` is thread-safe by default. -- `std.mem.trimRight`/`trimLeft` → `trimEnd`/`trimStart`. -- `std.mem.indexOf*` → `find*` (deprecation aliases still present, - so old names work but warn). -- `std.testing.refAllDeclsRecursive` removed. Only `refAllDecls` - remains. We use the bare `std.testing.refAllDecls(@This())` in - `src/main.zig`'s test block — it sema-touches every top-level decl, - which transitively pulls in every imported file's `test` blocks. - No local reimplementation is needed. See "Test discovery" below. -- `std.fs.File.readToEndAlloc(alloc, N)` → two-step: - `file.reader(io, &.{})` then `.interface.allocRemaining(alloc, .limited(N))`. - -For anything not on this list, **read the release notes first.** -The notes are long but they're organized by section; searching for -the specific symbol you're migrating is fast. ### `io` vs `today` / `now_s` — design rule @@ -203,143 +171,15 @@ Add tests in the same file. Money helpers belong next to the other tests in `Money.zig`; date helpers belong next to `yearsBetween`'s tests. -### NEVER invoke ripgrep. EVER. - -**Do not run `rg` in the Bash tool.** Not for open-ended search, not for -counting matches, not for "just this one quick check", not ever. Running -ripgrep on this machine hammers the filesystem badly enough to degrade the -whole system — this is a recurring, reproducible problem, not a hunch. - -**Use instead:** - -- **Grep tool** (built-in) for content search. It handles regex, file - globs, and output shaping without spawning `rg`. -- **Glob tool** (built-in) for finding files by name pattern. -- **Read tool** for reading files (with `offset`/`limit` for large ones). -- Plain `grep` via the Bash tool is acceptable when the built-in Grep - tool can't express what you need — but prefer the built-in first. - -**If you catch yourself typing `rg` in a Bash command:** stop, delete it, -use the Grep tool instead. The fact that `rg` is faster in the abstract -does NOT matter here. This machine's filesystem + ripgrep's parallelism -is a bad combination, full stop. - -**This applies to every variant:** `rg`, `ripgrep`, piping through -`rg`, backgrounded `rg`, `rg --files`, etc. All banned. - -### NEVER delete or modify build caches. EVER. - -**This means:** - -- **NEVER** run `rm -rf .zig-cache` or `rm -rf .zig-cache/*` or any variant. -- **NEVER** run `rm -rf ~/.cache/zig` or touch anything under `~/.cache/zig/`. -- **NEVER** touch `~/.cache/zls/` or any other tool cache. -- **NEVER** suggest deleting the cache as a "fix" — it is not a fix, it is - damage. Deleting `.zig-cache` while ZLS or another `zig build` is running - creates a corrupt state where the build runner's expected cache entry - (`.zig-cache/o//build`) references a path that no longer exists, - producing the error `failed to spawn build runner .zig-cache/o//build: - FileNotFound`. Recovering from this on the affected machine requires - killing every concurrent `zig` process (including ZLS) and waiting for - filesystem state to re-stabilize — it is NOT a simple retry. -- **NEVER** touch the cache "just to force a rebuild". Zig's cache is - content-addressed. It does not get stale in a way that deletion fixes. - If a build result looks wrong, the bug is in the source, not the cache. - Use `touch src/somefile.zig` if you truly need to invalidate one file's - cache line. Do not nuke the whole directory. -- **NEVER** suggest a "different cache directory" as a workaround without - an explicit, specific reason and explicit user approval. `--cache-dir` - and `--global-cache-dir` flags exist; they are not toys. - -**If a test result seems wrong or cached incorrectly:** the answer is -ALWAYS to investigate the source code or build graph, not to delete -cache. See "Test discovery" below — 99% of the time the "cached wrong -result" is actually a test discovery problem, not a cache problem. - -**If you find yourself typing `rm -rf` anywhere near a cache path: STOP. -Ask the user instead.** - -### NEVER run destructive git operations without explicit permission. - -- No `git reset --hard`, `git clean -fdx`, `git push --force`, `git checkout .` - on files with uncommitted work, unless the user asks for that specific - operation by name. - -### NEVER run `git stash`. EVER. - -- **`git stash` is banned outright.** Not `git stash push`, not `git stash --keep-index`, - not `git stash pop`, not `git stash apply`, not even read-only `git stash list`. - No variant. No "just to test something." No "I'll pop it right back." -- The reason: `git stash pop` can conflict on overlapping lines and leave - unresolved conflict markers in the working tree. The recovery requires - hand-resolving the markers, which trashes whatever curated index state - the user had in flight. This has bitten the user before and it's bitten - them again because of me. The rule is absolute now. -- If you think you need `git stash` to verify something (e.g. "does the - staged-only state build cleanly in isolation?"), the answer is: **don't - verify it that way.** Either: - - Read `git diff --cached` and reason about whether the staged hunks - are coherent on their own, OR - - Ask the user to verify after they commit, OR - - If verification is critical, ask the user to do the stash themselves - with their tools — but recommend against it because of this rule. -- **There is no exception clause for `git stash`.** Not even "the user - said it's OK this once" — the previous "one-time exception" for git - staging operations is what led to the stash incident. Direct exceptions - for `git add -p` / `git restore --staged` for staging-management remain - permitted with explicit user approval, but `git stash` is permanently - off-limits regardless of consent. - -### NEVER run `git add`, `git commit`, or `git push`. EVER. - -- **The user commits. You do not.** Do not stage files. Do not create commits. - Do not amend commits. Do not push. Do not suggest running these commands - yourself "to save a step". This includes `git add -p`, `git add .`, - `git add `, `git commit -m ...`, `git commit --amend`, `git push`, - and any `gh pr create` that would auto-stage or auto-commit. -- If you are tempted to run any of these because "the work is done and it - seems logical to commit" — STOP. The user has a review-and-commit workflow. - Your job ends at a clean working tree with the changes ready to review. -- The ONLY exception is when the user says, verbatim in the current turn, - "commit this" / "make a commit" / "push it" / similar direct imperative. - Do not extrapolate from earlier intent, a plan that mentioned milestones, - or any indirect signal. If in doubt, ask — don't commit. -- When a milestone plan says "STOP POINT — user reviews and commits": you - stop. You do not commit. You do not prepare a commit. You hand off the - working tree and wait. - -### NEVER write to `/tmp`. Use `$(cwd)/.tmp` instead. - -- **`/tmp` is off-limits for any temporary file the assistant creates.** - Not for scratch scripts, not for redirected stdout, not for `cp` - backups, not for "just for a second" experiments. The user's workflow - treats `/tmp` as their own space; assistant-created files there - pollute it across sessions and hide from `git status`. -- Use `$(cwd)/.tmp/` (a `.tmp` directory at the repository root) for - every transient file. `mkdir -p .tmp` if it doesn't exist; - `.gitignore` it (or rely on a project-level `.gitignore` rule that - catches it). Files in `.tmp` show up in `ls` next to the work, - surface in `git status` if the ignore rule is wrong, and get - cleaned up at the end of the task with a single `rm -rf .tmp/*`. -- This applies to: - - Bash redirections: `cmd > .tmp/out` not `cmd > /tmp/out`. - - Test fixture scratch files. - - `cp file /tmp/file.bak` for "I'll restore in a sec" backups — - use `cp file .tmp/file.bak` instead. - - Heredoc-created throwaway scripts to test a Zig snippet - (`zig run /tmp/foo.zig` → `zig run .tmp/foo.zig`). -- The ONE exception: temp files created by the system / by other - tools (mise installations, zig's own cache symlinks, etc.) that - legitimately use `/tmp`. Don't try to redirect those. - ### Documentation-file conventions -- **`TODO.md` holds only what's still open.** Git already tracks what was - done and when. Do NOT add "DONE" markers, completion status, strikethrough, - or "shipped in …" blurbs to TODO entries — just delete the section when - the work is finished. If follow-up work came out of the finished task, - add it as a new top-level section; don't leave the parent entry around - as a historical wrapper. +- **`TODO.md` holds only what's still open.** Git already tracks + what was done and when. Do NOT add "DONE" markers, completion + status, strikethrough, or "shipped in ..." blurbs to TODO + entries. Just delete the section when the work is finished. If + follow-up work came out of the finished task, add it as a new + top-level section; don't leave the parent entry around as a + historical wrapper. - **`REPORT.md` is untracked on purpose.** It's a personal workflow doc living in the repo root only until it moves to the user's portfolio directory. Edit it freely when asked; don't treat it as part of the @@ -347,140 +187,84 @@ Ask the user instead.** ### Lint warnings — there are no "pre-existing" warnings -**zlint warnings get fixed, period.** Do NOT excuse a warning by -saying it was "pre-existing in the file" or "inherited from a copy" -or "the same style as elsewhere." That's how lint debt accumulates -to the point where the pre-commit hook (`zlint --deny-warnings`) -becomes a tripwire that everyone routes around. +**Lint warnings get fixed, period.** Do NOT excuse a warning by +saying it was "pre-existing in the file" or "inherited from a +copy" or "the same style as elsewhere." That's how lint debt +accumulates to the point where the deny-warnings flag on the +pre-commit hook becomes a tripwire that everyone routes around. The rule: -1. **Before any commit, run zlint on every file you touched in - the change.** If zlint reports any warnings on those files, - fix them in that change. There is no "I didn't introduce it, - not my problem" — once you've touched the file, the warnings - are yours. +1. **Before any commit, run zlint on every file you touched + in the change.** If zlint reports any warnings on those + files, fix them in that change. There is no "I didn't + introduce it, not my problem." Once you've touched the file, + the warnings are yours. 2. **If you find pre-existing warnings in a file you didn't - intend to touch (e.g. you ran zlint across the whole tree - for a sanity check), fix them in a SEPARATE commit and call - that out to the user explicitly so they can keep the commits - clean.** Do not silently bundle drive-by lint fixes into a - feature commit; the diff becomes harder to review and the - lint-debt origin gets buried. + intend to touch** (e.g. you ran zlint across the whole + tree for a sanity check), fix them in a SEPARATE commit and + call that out to the user explicitly so they can keep the + commits clean. Do not silently bundle drive-by lint fixes + into a feature commit. -3. **Common warning kinds and the right fix:** - - `suppressed-errors` (`catch {}` / `catch "fallback"`): - replace with `try` (propagate to the caller). If the call - genuinely cannot propagate (e.g. an stderr write inside an - error-reporting path where the secondary error doesn't - matter), use `catch |err| std.log.debug(...)` or rewrite to - not need the catch. The `catch {}` form is almost never the - right answer. - - `unsafe-undefined`: add a `// SAFETY: ` comment on - the line with `undefined` explaining why it's safe (e.g. - "buffer immediately overwritten by bufPrint below"). - - `unused-decls`: delete the decl. Don't leave dead imports - or constants around "in case." +3. **Never report a lint result by saying "0 errors, N warnings, + but they're all pre-existing."** Either fix the warnings in + this change OR report "0 errors, 0 warnings on the files I + touched. The wider tree has N warnings I haven't addressed + in this change; flagging for follow-up" — and only after + you've confirmed by file that none of the wider-tree warnings + are in files you modified. -4. **Never report a lint result by saying "0 errors, N warnings, - but they're all pre-existing."** Either: - - Fix the warnings in this change (preferred when N is small - or the file is yours), OR - - Report "0 errors, 0 warnings on the files I touched. The - wider tree has N warnings I haven't addressed in this - change; flagging for follow-up" — and only after you've - confirmed by file that none of the wider-tree warnings - are in files you modified. +zfin uses `zlint --deny-warnings` on the pre-commit hook. The +common zlint warning kinds and the right fix: -The `--deny-warnings` flag is on the pre-commit hook for a -reason: every warning is a real signal that the codebase asked -the linter to flag and someone hasn't dealt with. Treat them as -errors at write-time, not as background noise to ignore. +- `suppressed-errors` (`catch {}` / `catch "fallback"`): + replace with `try` (propagate to the caller). If the call + genuinely cannot propagate (e.g. an stderr write inside an + error-reporting path where the secondary error doesn't + matter), use `catch |err| std.log.debug(...)` or rewrite to + not need the catch. The `catch {}` form is almost never the + right answer. +- `unsafe-undefined`: add a `// SAFETY: ` comment on + the line with `undefined` explaining why it's safe (e.g. + "buffer immediately overwritten by bufPrint below"). +- `unused-decls`: delete the decl. Don't leave dead imports + or constants around "in case." -### Em-dash usage — ASK FIRST - -If you're about to write an em-dash (`—`) anywhere (code, tests, doc -comments, commit messages, AGENTS.md prose), **stop and check whether a -regular ASCII hyphen (`-`) would do.** Most of the time it would. Em-dashes -look nice in prose but they create real problems: - -- **In tabular output / TUI cells**, `—` is 3 bytes / 1 display column. - Zig's `{s:>N}` formatter pads by byte count, so any column containing - em-dashes will be 2 visual columns short per em-dash. We've fixed this - bug at least twice; don't reintroduce it. If you genuinely need a - multibyte sentinel for "no data", use `fmt.padRightToCols` / - `fmt.centerDash` (display-column-aware) — or just hard-code the cell - as a literal const string when the cell width is fixed and the dash - position is static (no point computing what you already know). Add - an alignment test that compares the multibyte row's `displayCols` - against an ASCII row. -- **In code identifiers and string literals**, em-dashes look fine in your - editor and break grep on the user's machine when they're searching with - ASCII `-`. If a future grep for "expected-return" or "as-of" silently - misses your "expected—return" string, that's a bug surface. -- **In commit messages and prose docs**, em-dashes are an AI tell. The - user reads commit messages and won't appreciate the codebase looking - like it was written by ChatGPT. - -**Rule of thumb:** - -- Use `-` (ASCII hyphen) for: ranges (`1-5y`), compound modifiers - (`forecast-vs-actual`), CLI flag names (`--return-backtest`), code - identifiers, and string concatenation in tables. -- Use `—` (em-dash) only when you're displaying it to the user as a - meaningful sentinel (e.g. "no data" cell in a table), AND you've handled - the display-column padding correctly. -- If you find yourself reaching for `—` in prose for a parenthetical - aside, **switch to a regular dash, comma, or parens.** Em-dashes have - become a stylistic AI tic and the user has explicitly asked to keep - them out unless they earn their place. - -When in doubt, **ask**. A one-line "I'm about to use `—` here for X, OK?" -is much cheaper than reverting after the user notices. ### Errors carry information — never throw it away -When you catch an error, the caller's first question is **"why did -this fail?"** A user-facing error message that says only +When you catch an error, the caller's first question is **"why +did this fail?"** A user-facing error message that says only `"FetchFailed"` or `"could not parse portfolio file"` is failing the user — they have to read source code to figure out what happened. Three habits cause this: -**(1) Bare `catch {}` and `catch return error.X`.** The error -value carries the upstream's classification (`RateLimited`, -`Unauthorized`, `NotFound`, `ParseError`, etc.). Discarding it -reduces N distinct conditions to one opaque "something went -wrong." Fix: capture as `catch |err|` and at minimum include -`@errorName(err)` (or `{t}` format) in any user-visible message. -Better: re-raise a more specific error variant from the caller's -error set so the next layer up can decide what to do. +**(1) Bare `catch {}` and `catch return error.X`.** Capture as +`catch |err|` and at minimum include `@errorName(err)` (or `{t}` +format) in any user-visible message. Better: re-raise a more +specific error variant from the caller's error set. -**(2) Generic `error.X` collapse points in service-layer code.** +**(2) Generic `DataError.FetchFailed` collapse in service.zig.** When `service.zig:getX` does `provider.fetchX(...) catch return DataError.FetchFailed`, the provider's specific error (`RateLimited` vs `Unauthorized` vs `NotFound`) is permanently -erased. Fix: either expand the service-layer error set -(`DataError`) to mirror the provider distinctions, OR pass the -inner error through via `catch |err| { log.warn(... -@errorName(err) ...); return ...; }` so the stderr path tells -the user something useful even if the typed return value is -collapsed. +erased. Fix: either expand `DataError` to mirror the provider +distinctions, OR pass the inner error through via `catch |err| { +log.warn(... @errorName(err) ...); return ...; }` so stderr +tells the user something useful even when the typed return value +is collapsed. -**(3) User-facing stderr messages that don't say what the actual -error name was.** A CLI command that catches an error MUST print -`@errorName(err)` (or a deliberate human translation per error -kind) in the stderr message. `"Error: Failed to fetch data for -symbol"` is unhelpful. `"Error fetching AAPL: RateLimited (free -tier 5/min — wait 60s)"` is helpful. The error name alone is -enough for the user to look it up; the human translation is a -nice-to-have on top. +**(3) User-facing stderr messages must name the error.** A CLI +command that catches an error MUST print `@errorName(err)` (or a +deliberate human translation per error kind) in the stderr +message. `"Error: Failed to fetch data for symbol"` is +unhelpful. `"Error fetching AAPL: RateLimited (free tier 5/min, +wait 60s)"` is helpful. -**The grep test:** if you're catching an error in user-facing -code, the message should mention either `@errorName` / `{t}` or -reference a specific error variant by name. `cli.stderrPrint` -calls with hardcoded "Error: failed to ..." strings and no error -context fail this test. +**The grep test:** `cli.stderrPrint` calls with hardcoded "Error: +failed to ..." strings and no error context fail this test. ### NEVER put PII in tests, fixtures, comments, or docs @@ -879,26 +663,27 @@ shape with example signatures. ### `anytype` is almost never the right answer — pause and ask first -Empirically, every time `anytype` looked necessary in this codebase -it turned out not to be. Concrete-typed parameters always worked -once we actually tried them. **Before adding any new `anytype` -parameter, stop and reconsider.** The discussion that surfaces "no, -this can be `*App` after all" is short and worth having every time. +Empirically, every time `anytype` looked necessary in this +codebase it turned out not to be. Concrete-typed parameters +always worked once we actually tried them. **Before adding any +new `anytype` parameter, stop and reconsider.** The discussion +that surfaces "no, this can be `*App` after all" is short and +worth having every time. Common reasons people reach for `anytype` and what to do instead: - **"I want to avoid a circular import."** Test the assumption. Zig resolves `a.zig ↔ b.zig` cycles fine in most cases — file structs are evaluated lazily, and the cycle only fails if - evaluation actually loops. Just write the concrete type and run - `zig build`. If it fails, the answer is usually to extract the - shared type to a third file, not to weaken the contract with - `anytype`. (See: the `tab_framework.zig` ↔ `tui.zig` cycle — - caught me once; turned out Zig handled it cleanly.) + evaluation actually loops. Just write the concrete type and + run `zig build`. If it fails, the answer is usually to extract + the shared type to a third file, not to weaken the contract + with `anytype`. (See: the `tab_framework.zig` ↔ `tui.zig` cycle + — caught me once; turned out Zig handled it cleanly.) - **"This function is genuinely polymorphic over many types."** In Zig, the right shape for runtime polymorphism is usually - `*anyopaque` + an explicit cast at the boundary, paired with - a vtable struct of fn pointers. That's harder to abuse than + `*anyopaque` + an explicit cast at the boundary, paired with a + vtable struct of fn pointers. That's harder to abuse than `anytype` and the cast site documents the type contract unambiguously. Compile-time polymorphism over a known set of types is better expressed with `comptime T: type` parameters. @@ -910,8 +695,8 @@ Common reasons people reach for `anytype` and what to do instead: the method. - **"It's a test helper that takes any struct."** This is the one case where `anytype` is sometimes OK — generic test - utilities like `std.testing.expectEqual`. But check whether - a concrete type would do. + utilities like `std.testing.expectEqual`. But check whether a + concrete type would do. The real cost of `anytype` is that it punches a hole in the type contract: the compiler can't catch wrong-shape arguments at the @@ -977,7 +762,3 @@ command. | [SRF](https://git.lerch.org/lobo/srf) | Cache file format, portfolio/watchlist parsing, serialization | | [libvaxis](https://github.com/rockorager/libvaxis) (v0.6.0) | Terminal UI rendering | | [z2d](https://github.com/vancluever/z2d) (v0.11.0) | Pixel chart rendering (Kitty graphics protocol) | - -## Build system rules - -- **Never use `addAnonymousImport`** in `build.zig`. Always use `b.addModule()` + `addImport()`. Anonymous imports cause "file belongs to multiple modules" errors and make dependency wiring opaque.