Twigpine/zero

0 Fork 0

mirror of https://github.com/Twigpine/zero.git synced 2026-11-10 17:57:54 -10:00

Files

T

euxaristiaandeuxaristia 8dd33ec39e docs(agents): Record four recurring review blockers (#899 )

* docs(agents): Record four recurring review blockers

These classes come up across unrelated pull requests, are not caught by CI,
and nothing in AGENTS.md currently prevents them, so each is rediscovered by a
reviewer at the cost of a round trip.

Host tool version floors is the clearest case. Zero shells out to git and the
project declares no minimum version anywhere, so adding a modern flag raises
that floor silently and an older host fails with a usage error instead of a
clear message. Prove-the-test-fails addresses a recurring shape where the
problem is not a missing test but one that passes either way, because an
earlier guard rejects the input before the behavior under test runs. Hermetic
tests covers isolation helpers applied to some tests but not others reaching
the same storage. Platform error codes extends the existing platform-truth
rule, which covers path canonicalization but not errno and NTSTATUS values.

Advisory lint findings are deliberately not included: deadcode and lint-static
both run with continue-on-error in ci.yml, and section 3 already requires
fixing findings the change introduces.

Refs #898

* docs(agents): Tighten version-floor, isolation, and errno guidance

Three corrections from review, two of them to claims that were too strong.

Git version floors are per subcommand, not per flag. --end-of-options reached
the shared parser in 2.24 but git rev-parse only accepted it from 2.30, so
recording the flag's general debut would still let a caller ship something
that fails on 2.24 through 2.29.

The hermetic-tests bullet named config, cache, and state but gave a procedure
covering only config. It now covers each root, including %LocalAppData% for
the cache root, which os.UserCacheDir resolves without consulting XDG.

The errno bullet asserted ENOTDIR on Linux and Darwin. Only Linux was actually
measured; Darwin was assumed, in a bullet whose whole point is not to assume.
It now says the call may report either code and both should be accepted, which
is what the guidance was trying to teach.

Refs #898

* docs(agents): Clarify macOS and Windows roots for hermetic test isolation.

os.UserConfigDir and os.UserCacheDir derive from HOME on macOS and AppData/LocalAppData on Windows rather than reading XDG variables.

Refs #899

---------

Co-authored-by: euxaristia <[email protected]>

2026-08-20 20:23:31 +08:00

9.2 KiB

Raw Permalink Blame History

Repository Guidelines for Zero

These instructions apply to all work in this repository. For the user-facing guide to extending Zero with specialists, hooks, plugins, MCP, and skills, see docs/EXTENDING.md.

1. Contribution and Pull Request Rules

Before opening any pull request, all contributors—including maintainers, community contributors, and coding agents—must read and follow CONTRIBUTING.md.
Community pull requests require an existing parent issue with the issue-approved label. Team members may open pull requests through the internal development process described in CONTRIBUTING.md.
Keep each change focused on the approved or assigned scope. Do not include unrelated fixes, refactors, formatting churn, generated output, or existing local changes in the same commit or pull request.
Discuss new implementation languages, runtimes, major dependency changes, and broad architectural rewrites with maintainers before implementation.
Pull request descriptions must explain what changed and why, link the parent issue when required, and list the tests or verification performed. Include screenshots or a short recording for user-visible UI changes when practical.

2. Repository and Implementation Conventions

Use the Go version declared in go.mod. Do not hardcode a different local toolchain version in scripts or documentation.
Use the repository build and release commands (make and go run ./cmd/zero-release ...) instead of inventing parallel build flows.
Keep tests beside their source files (foo_test.go next to foo.go). Add a regression test for behavior changes and run affected concurrent code under the race detector.
Never edit files under third_party/; they are vendored.
Prefer one cross-platform function with small conditional checks over duplicated platform-specific helpers when the behavior can remain unified.
Do not commit generated benchmark reports from internal/perfbench/reports/*.json; reports are configuration-specific evidence, not repository state.
Preserve the user's working tree. Do not overwrite, delete, stage, or commit unrelated tracked or untracked files.

3. Required Validation

Run validation from the repository root before committing, opening a pull request, or completing an implementation task:

Formatting check: make fmt-check. If it fails, format with go fmt ./... (or make fmt) and run the check again.
Vet: go vet ./... (or make vet).
Tests: go test ./.... Use make test for the full race-enabled suite, or run focused tests with -race, when concurrency is affected.
Build: go run ./cmd/zero-release build.
Smoke test: go run ./cmd/zero-release smoke.
Advisory lint: make lint-static.
Security: make vulncheck.
Diff hygiene: git diff HEAD --check (covers staged and unstaged tracked changes).

make lint currently runs the formatting check and go vet; it does not run golangci-lint. The pinned golangci-lint job is advisory in CI while the existing repository-wide backlog is cleaned up. Fix findings introduced by or related to the current change. Unrelated pre-existing advisory findings do not justify expanding a focused pull request; report them separately.

Formatting, vet, tests, build, smoke, diff hygiene, and govulncheck are hard requirements. If a related check fails, fix it. If a required check cannot run because of the environment or fails for an unrelated external reason, report the exact failure and obtain maintainer direction rather than silently ignoring it.

4. Common Review Blockers

These classes drive multi-round reviews. Fix them before requesting review:

Fresh base: Rebase onto the current PR base (main or stacked target) before review. A stale head that rolls back mainline commits is a hard blocker, not a merge-time detail. Resolve conflicts by keeping upstream behavior unless the PR intentionally changes it.
Platform truth: Code and tests must pass on Linux, macOS, and Windows. Compare paths after canonicalization (physical/EvalSymlinks form); never assert raw t.TempDir() spellings (/var vs /private/var on macOS, short paths on Windows). Prefer one cross-platform function with small GOOS checks over duplicated helpers that drift.
Security edges: Keep secrets out of argv, env dumps, and logs. Redact success and error paths (including stderr). Fail closed on ownership, lease, and permission checks. Do not rely on pre-open path resolution (EvalSymlinks then open) for containment: that is a check-to-use race. Bind containment at open/use time with rooted or handle-relative, traversal-resistant APIs. If a no-follow API is used, apply it to every traversed component and enforce the platform's reparse-point protections; final-component-only no-follow is insufficient. On multi-step setup, roll back only what this run created; never destroy pre-existing resources you did not create; never report success when cleanup or unlock failed.
Normalize before matching: Any transform that REMOVES bytes without leaving a gap is also a reassembler, so it has to run before whatever matches on the result. Redacting by shape and then stripping control bytes lets a credential split by a NUL, an ESC or a C1 byte pass the patterns as two fragments and be rejoined on the way out; comparing a path against a form produced by the same resolver the kernel just used makes a redirect agree with itself. Strip, decode, or canonicalize first, then match. The unsplit value passing is not evidence, so the test needs a split case.
Atomic shared state: Write a complete temporary file, then atomically replace the destination so concurrent readers never see a partial write. Exclusive create or a write lock alone is not enough for readers. Serialize the full read-modify-write sequence for lockfiles and shared stores.
Tests match the claim: Every behavior or security-boundary change needs a regression test, including the failure path. Path-sensitive logic needs at least one non-Linux path case (or a hermetic fake that exercises the same normalization).
Honest scope: PR description, help text, and comments must match what shipped. Wire advertised entry points or shrink the claim. Do not bundle unrelated fixes. Do not widen security allowlists by name alone; check classification and side effects too.
Host tool version floors: Zero shells out to git and other host tools, and the project does not declare a minimum version for any of them. A flag, subcommand, or config key that is not long-established therefore raises that floor silently, and the failure on an older host is an unhelpful usage error rather than a clear message. Before adding one, check when it landed and say so where a caller would look. Record the floor for the exact subcommand you call, not for the flag in general: --end-of-options reached Git's shared parser in 2.24, but git rev-parse only accepted it from 2.30, and git checkout and git reset later still. Likewise --show-current is git branch 2.22, and branch.autoSetupMerge=inherit is 2.35. Then either keep a fallback for older versions or gate the call. A test that depends on a newer feature must skip below its floor instead of failing, and say which version it needs.
Prove the test fails without the change: A regression test earns its place only if it fails on the unfixed code, for the reason it claims. Run it both ways and quote the failure in the PR. The recurring miss is not a missing test but a test that never reaches the behavior it names, because an earlier guard rejects the input first, so it passes with and without the fix. When a test targets a specific layer, call that layer directly rather than relying on a public entry point that may refuse earlier.
Hermetic tests: Tests must not read or write the developer's real config, cache, or state directories. Redirect every root the code under test resolves, not just config. os.UserConfigDir and os.UserCacheDir read XDG_CONFIG_HOME and XDG_CACHE_HOME only on Linux and the BSDs. On macOS they derive from HOME, and on Windows from %AppData% and %LocalAppData%. Setting the XDG variables alone still leaves a macOS run writing to the real home, so set HOME as well, or take the directory as a parameter. Apply it to every test that reaches the same storage, not only the one where it was first needed; a test that passes on a clean machine and pollutes a real one is a blocker.
Error codes vary by platform and by flag combination: A check written against the code one target returns can be silently inert on another while still looking correct. openat(O_DIRECTORY|O_NOFOLLOW) on a symlink may report ENOTDIR or ELOOP depending on the platform, because the kernel can reject the directory mismatch before it ever resolves the link; accept both rather than assuming the one POSIX names. On Windows, \\?\ and \\.\ prefixes are not UNC paths, and an NtCreateFile information class must match the struct actually passed. Confirm the code each supported target returns instead of reasoning from the man page.