Compare commits
3
Commits
afcef4245b
..
next
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
9f1e9f5253 | ||
|
|
2c90144e7b | ||
|
|
26b5160aa8 |
@@ -1,75 +1,76 @@
|
||||
# Repository Rules
|
||||
|
||||
Last Updated 2026-01-08
|
||||
Last Updated 2026-10-27
|
||||
|
||||
These rules MUST be followed at all times, it is very important.
|
||||
These rules MUST be followed at all times. This is very important.
|
||||
|
||||
- Never use `git add -A` - add specific changes to a deliberate commit. A commit
|
||||
should contain one change. After each change, make a commit with a good
|
||||
one-line summary.
|
||||
- Never use `git add -A`. Stage specific changes for a deliberate commit. A
|
||||
commit holds one change. After each change, make a commit with a good one-line
|
||||
summary.
|
||||
|
||||
- NEVER modify the linter config without asking first.
|
||||
|
||||
- NEVER modify tests to exclude special cases or otherwise get them to pass
|
||||
without asking first. In almost all cases, the code should be changed, NOT the
|
||||
tests. If you think the test needs to be changed, make your case for that and
|
||||
ask for permission to proceed, then stop. You need explicit user approval to
|
||||
modify existing tests. A bug fix may change an existing test without asking
|
||||
when that test pins exactly the behavior the filed bug calls wrong, and the
|
||||
change moves it to the fixed behavior and nothing else; any other change to an
|
||||
existing test still needs explicit user approval. (You do not need user
|
||||
approval for writing NEW tests.)
|
||||
- NEVER modify a test to exclude special cases or otherwise make it pass without
|
||||
asking first. In almost all cases, change the code, NOT the test. You need
|
||||
explicit user approval to modify an existing test. If you think a test needs
|
||||
to change, make your case and ask for permission to proceed. Then stop. A bug
|
||||
fix may change an existing test without asking only when that test pins
|
||||
exactly the behavior the filed bug calls wrong. The change must then move the
|
||||
test to the fixed behavior and nothing else. Any other change to an existing
|
||||
test needs explicit user approval. Writing NEW tests needs no approval.
|
||||
|
||||
- When linting, assume the linter config is CORRECT, and that each item output
|
||||
by the linter is something that legitimately needs fixing in the code.
|
||||
- When linting, assume the linter config is CORRECT. Treat each item the linter
|
||||
reports as something that legitimately needs fixing in the code.
|
||||
|
||||
- When running tests, use `make test`.
|
||||
|
||||
- Before commits, run `make check`. This runs `make lint` and `make test` and
|
||||
`make check-fmt`. Any issues discovered MUST be resolved before committing
|
||||
unless explicitly told otherwise.
|
||||
- Before committing, run `make check`. It runs `make test`, `make lint` and
|
||||
`make fmt-check`. Any issue it finds MUST be resolved before committing,
|
||||
unless you are explicitly told otherwise.
|
||||
|
||||
- When fixing a bug, write a failing test for the bug FIRST. Add appropriate
|
||||
logging to the test to ensure it is written correctly. Commit that. Then go
|
||||
about fixing the bug until the test passes (without modifying the test
|
||||
further). Then commit that.
|
||||
logging to the test, to make sure the test is written correctly. Commit that.
|
||||
Then work on the fix until the test passes, without changing the test further.
|
||||
Then commit the fix.
|
||||
|
||||
- When adding a new feature, do the same - implement a test first (TDD). It
|
||||
doesn't have to be super complex. Commit the test, then commit the feature.
|
||||
- When adding a new feature, do the same: write a test first (test-driven
|
||||
development, TDD). The test does not need to be very complex. Commit the test,
|
||||
then commit the feature.
|
||||
|
||||
- When adding a new feature, use a feature branch. When the feature is
|
||||
completely finished and the code is up to standards (passes `make check`) then
|
||||
and only then can the feature branch be merged into `main` and the branch
|
||||
deleted.
|
||||
- When adding a new feature, use a feature branch. The feature branch reaches
|
||||
`next` through a PR that passes review. It is merged only when the feature is
|
||||
completely finished and the code is up to standards. The code is up to
|
||||
standards when it passes `make check`. The branch is deleted after the merge.
|
||||
Only the user merges `next` into `main`.
|
||||
|
||||
- Write godoc documentation comments for all exported types and functions as you
|
||||
go along.
|
||||
- Write `godoc` comments for all exported types and functions as you go along.
|
||||
|
||||
- ALWAYS be consistent in naming. If you name something one thing in one place,
|
||||
name it the EXACT SAME THING in another place.
|
||||
- ALWAYS be consistent in naming. If you name something one way in one place,
|
||||
use the EXACT SAME name in every other place.
|
||||
|
||||
- Be descriptive and specific in naming. `wl` is bad; `SourceHostWhitelist` is
|
||||
- Be descriptive and specific in naming. `al` is bad; `SourceHostAllowlist` is
|
||||
good. `ConnsPerHost` is bad; `MaxConnectionsPerHost` is good.
|
||||
|
||||
- This is not prototype or teaching code - this is designed for production. Any
|
||||
security issues (such as denial of service) or other web vulnerabilities are
|
||||
P1 bugs and must be added to TODO.md at the top.
|
||||
- This is not prototype or teaching code. It is designed for production. Any
|
||||
security issue (such as denial of service) or other web vulnerability is a P1
|
||||
bug. It must be added at the top of `TODO.md`.
|
||||
|
||||
- As this is production code, no stubbing of implementations unless specifically
|
||||
instructed. We need working implementations.
|
||||
- Do not stub an implementation unless you are specifically instructed to. This
|
||||
is production code, and we need working implementations.
|
||||
|
||||
- NEVER silently fall back to a different setting when a user's parameter
|
||||
explicitly specifies a value. If a user requests format=webp and WebP encoding
|
||||
is not supported, return an error - do NOT silently output PNG instead. If a
|
||||
user specifies fit=invalid and that fit mode doesn't exist, return an error -
|
||||
do NOT silently default to "cover". Silent fallbacks violate the principle of
|
||||
least surprise and mask bugs. The only acceptable defaults are for OMITTED
|
||||
parameters, never for INVALID explicit values.
|
||||
explicitly specifies a value. For example, if a user requests `format=webp`
|
||||
and WebP encoding is not supported, return an error. Do NOT silently output
|
||||
PNG instead. If a user specifies `fit=invalid` and that fit mode does not
|
||||
exist, return an error. Do NOT silently default to `cover`. A silent fallback
|
||||
violates the principle of least surprise and masks bugs. Defaults are
|
||||
acceptable only for OMITTED parameters, never for INVALID explicit values.
|
||||
|
||||
- Avoid vendoring deps unless specifically instructed to. NEVER commit the
|
||||
vendor directory, NEVER commit compiled binaries. If these directories or
|
||||
files exist, add them to .gitignore (and commit the .gitignore) if they are
|
||||
not already in there. Keep the entire git repository (with history) small -
|
||||
under 20MiB, unless you specifically must commit larger files (e.g. test
|
||||
fixture example media files). Only OUR source code and immediately supporting
|
||||
files (such as test examples) goes into the repo/history.
|
||||
- Avoid vendoring dependencies unless specifically instructed to. NEVER commit
|
||||
the `vendor/` directory. NEVER commit compiled binaries. If such a directory
|
||||
or file exists and `.gitignore` does not list it, add it to `.gitignore`.
|
||||
Commit that `.gitignore` change. Keep the entire git repository, history
|
||||
included, under 20 MiB. Go over 20 MiB only when you specifically must commit
|
||||
a large file, such as a media file used as a test fixture. Only OUR source
|
||||
code and its immediately supporting files, such as test examples, go into the
|
||||
repository and its history.
|
||||
|
||||
@@ -31,8 +31,9 @@ P2: security: per-origin rate limiting
|
||||
# Completed Steps
|
||||
|
||||
- 2026-10-27 the reconciliation pass skips a directory it cannot list and a file
|
||||
whose info it cannot read, and goes on (closes #343). It returned the error,
|
||||
so one such entry stopped every pass before its later checks.
|
||||
whose info it cannot read, and goes on (closes #343). It stopped before its
|
||||
later checks at a directory it could not list, or at a source or variant file
|
||||
whose info it could not read.
|
||||
- It logs the warning "skipped unreadable entry in cache directory", with
|
||||
the path and the error.
|
||||
- A file renamed or removed after its directory was listed gets no warning.
|
||||
|
||||
@@ -819,10 +819,11 @@ func openDirectoryNearPathLimit(t *testing.T, dir string) *os.Root {
|
||||
return root
|
||||
}
|
||||
|
||||
// makeEntriesTooLongToRead makes a directory and a file in the directory
|
||||
// that openDirectoryNearPathLimit opens inside dir. Even the root user can
|
||||
// then neither list that directory nor read the file's info. It returns
|
||||
// their two paths.
|
||||
// makeEntriesTooLongToRead makes a new directory and a new file in the
|
||||
// deepest directory of the chain that openDirectoryNearPathLimit makes
|
||||
// inside dir. Even the root user can then neither list the new directory
|
||||
// nor read the new file's info. It returns the paths of the new directory
|
||||
// and the new file.
|
||||
func makeEntriesTooLongToRead(t *testing.T, dir string) []string {
|
||||
t.Helper()
|
||||
|
||||
@@ -847,9 +848,10 @@ func makeEntriesTooLongToRead(t *testing.T, dir string) []string {
|
||||
}
|
||||
}
|
||||
|
||||
// makeTempFileTooLongToRead makes a temp file in the directory that
|
||||
// openDirectoryNearPathLimit opens inside dir. Even the root user then
|
||||
// cannot read its info. It returns its path.
|
||||
// makeTempFileTooLongToRead makes a temp file in the deepest directory of
|
||||
// the chain that openDirectoryNearPathLimit makes inside dir. Even the
|
||||
// root user then cannot read the temp file's info. It returns the temp
|
||||
// file's path.
|
||||
func makeTempFileTooLongToRead(t *testing.T, dir string) string {
|
||||
t.Helper()
|
||||
|
||||
@@ -864,6 +866,25 @@ func makeTempFileTooLongToRead(t *testing.T, dir string) string {
|
||||
return filepath.Join(deepest.Name(), name)
|
||||
}
|
||||
|
||||
// assertSkippedEntryWarning checks that logs hold the warning "skipped
|
||||
// unreadable entry in cache directory" naming path, with the error
|
||||
// ENAMETOOLONG for that path.
|
||||
func assertSkippedEntryWarning(t *testing.T, logs, path string) {
|
||||
t.Helper()
|
||||
|
||||
for line := range strings.Lines(logs) {
|
||||
if strings.Contains(line, `"level":"WARN"`) &&
|
||||
strings.Contains(line, `"msg":"skipped unreadable entry in cache directory"`) &&
|
||||
strings.Contains(line, `"path":"`+path+`"`) &&
|
||||
strings.Contains(line, path+": "+syscall.ENAMETOOLONG.Error()) {
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
t.Errorf("no warning %q names %s with the error %q",
|
||||
"skipped unreadable entry in cache directory", path, syscall.ENAMETOOLONG)
|
||||
}
|
||||
|
||||
// TestReconciliationGoesOnPastEntriesItCannotRead puts, in each storage
|
||||
// directory, a directory the pass cannot list and a file whose info it
|
||||
// cannot read. The pass walks each directory in name order, so it reaches
|
||||
@@ -915,11 +936,7 @@ func TestReconciliationGoesOnPastEntriesItCannotRead(t *testing.T) {
|
||||
logs := reconcileAsAtStartup(t, cache)
|
||||
|
||||
for _, path := range unreadable {
|
||||
assertWarningNames(t, logs, path)
|
||||
|
||||
if !strings.Contains(logs, path+": "+syscall.ENAMETOOLONG.Error()) {
|
||||
t.Errorf("no log line gives the error reading %s", path)
|
||||
}
|
||||
assertSkippedEntryWarning(t, logs, path)
|
||||
}
|
||||
|
||||
rowQuery := `SELECT COUNT(*) FROM variant_content WHERE cache_key = ?`
|
||||
@@ -965,11 +982,7 @@ func TestReconciliationWarnsOfTempFilesWhoseInfoItCannotRead(t *testing.T) {
|
||||
logs := reconcileAsAtStartup(t, cache)
|
||||
|
||||
for _, path := range unreadable {
|
||||
assertWarningNames(t, logs, path)
|
||||
|
||||
if !strings.Contains(logs, path+": "+syscall.ENAMETOOLONG.Error()) {
|
||||
t.Errorf("no log line gives the error reading %s", path)
|
||||
}
|
||||
assertSkippedEntryWarning(t, logs, path)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user