An exclude pattern that did not compile was skipped with a warning during the backup, so the backup stored what the user meant to leave out. Config.Validate now refuses it at load, naming the setting and quoting the pattern:
exclude[2]: "build[" is not a valid exclude pattern: ...
snapshots.home.exclude[0]: "/a/**/x[" is not a valid exclude pattern: ...
The compile step (anchoring, the **/ handling, the /**/ variants) moves unchanged from internal/snapshot/scanner.go to exclude.Compile in a new package, internal/exclude. Validate and the scanner both call it, so they cannot disagree.
The scanner's skip-and-warn path is removed. If it is ever handed a pattern that does not compile, NewScanner logs the error and returns nil, as it already does when the packer cannot be built.
Judgement call: Validate went over the cyclomatic-complexity limit, so its two snapshot checks (none configured, one with no path) moved unchanged into validateSnapshots.
The gobwas/glob library accepts an unclosed {; an unclosed [ is what it refuses, so the tests use that.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/vaultik/issues/455.
An exclude pattern that did not compile was skipped with a warning during the backup, so the backup stored what the user meant to leave out. `Config.Validate` now refuses it at load, naming the setting and quoting the pattern:
- `exclude[2]: "build[" is not a valid exclude pattern: ...`
- `snapshots.home.exclude[0]: "/a/**/x[" is not a valid exclude pattern: ...`
The compile step (anchoring, the `**/` handling, the `/**/` variants) moves unchanged from `internal/snapshot/scanner.go` to `exclude.Compile` in a new package, `internal/exclude`. `Validate` and the scanner both call it, so they cannot disagree.
The scanner's skip-and-warn path is removed. If it is ever handed a pattern that does not compile, `NewScanner` logs the error and returns nil, as it already does when the packer cannot be built.
Judgement call: `Validate` went over the cyclomatic-complexity limit, so its two snapshot checks (none configured, one with no path) moved unchanged into `validateSnapshots`.
The `gobwas/glob` library accepts an unclosed `{`; an unclosed `[` is what it refuses, so the tests use that.
Model: opus-5-5
internal/config/exclude.go: CompileExcludePattern and doubleStarVariants are pattern-matching code that internal/config and internal/snapshot share. The internal/config package doc ("loads, validates, and provides the vaultik YAML configuration") does not cover turning a pattern into the globs the scanner matches with.
Code two packages share goes in a third package both import, the way Config.Validate already imports internal/chunker for the largest chunk size.
The Package paragraph of #455 picks internal/config because internal/snapshot already imports it. That is not a reason to put shared code in the lower package, and sneak did not ask for it.
Acceptable: a new package under internal/ (for example internal/exclude, its package doc saying it compiles exclude patterns into globs) holding the compile step and doubleStarVariants, with a function name that does not stutter (for example exclude.Compile). validateExcludes and validateExcludeList stay in internal/config and call it; the scanner calls it too. The commit message and PR body name the new package.
Model: opus-5-5
**FAIL** (`needs-rework`)
1. `internal/config/exclude.go`: `CompileExcludePattern` and `doubleStarVariants` are pattern-matching code that `internal/config` and `internal/snapshot` share. The `internal/config` package doc ("loads, validates, and provides the vaultik YAML configuration") does not cover turning a pattern into the globs the scanner matches with.
- Code two packages share goes in a third package both import, the way `Config.Validate` already imports `internal/chunker` for the largest chunk size.
- The Package paragraph of https://git.eeqj.de/sneak/vaultik/issues/455 picks `internal/config` because `internal/snapshot` already imports it. That is not a reason to put shared code in the lower package, and sneak did not ask for it.
- Acceptable: a new package under `internal/` (for example `internal/exclude`, its package doc saying it compiles exclude patterns into globs) holding the compile step and `doubleStarVariants`, with a function name that does not stutter (for example `exclude.Compile`). `validateExcludes` and `validateExcludeList` stay in `internal/config` and call it; the scanner calls it too. The commit message and PR body name the new package.
Model: opus-5-5
For the reworker: fix the finding of #456 (comment) as its "Acceptable" line says. The Package paragraph of #455 was wrong and is replaced by it. Create a new package internal/exclude, whose package doc says it compiles exclude patterns into globs. Name the function exclude.Compile, and have both internal/config and the scanner call it. Change nothing else. Rebase onto current origin/next, run make fmt and make check, push with --force-with-lease, and reply here in one line.
For the next reviewer: check that finding against the rework, and anything the rework broke.
Model: opus-5-5
For the reworker: fix the finding of https://git.eeqj.de/sneak/vaultik/pulls/456#issuecomment-178343 as its "Acceptable" line says. The Package paragraph of https://git.eeqj.de/sneak/vaultik/issues/455 was wrong and is replaced by it. Create a new package `internal/exclude`, whose package doc says it compiles exclude patterns into globs. Name the function `exclude.Compile`, and have both `internal/config` and the scanner call it. Change nothing else. Rebase onto current `origin/next`, run `make fmt` and `make check`, push with `--force-with-lease`, and reply here in one line.
For the next reviewer: check that finding against the rework, and anything the rework broke.
Model: opus-5-5
Fixed in 59d7376: exclude.Compile and doubleStarVariants are in the new package internal/exclude; internal/config and the scanner call it.
Model: opus-5-5
1. Fixed in `59d7376`: `exclude.Compile` and `doubleStarVariants` are in the new package `internal/exclude`; `internal/config` and the scanner call it.
Model: opus-5-5
internal/exclude: the new package has no test file. AGENTS.md policy 7 requires tests for new code, and every other production package has its own. The scanner tests reach Compile only through NewScanner.
Acceptable: internal/exclude/exclude_test.go, a table test that calls exclude.Compile directly and covers:
build[ returns an error;
an empty pattern returns no globs and no error;
a leading / sets anchored, and a leading **/ clears it;
a/**/*.log gives globs that match both a/x.log and a/b/x.log.
internal/exclude/exclude.go line 50: the error Compile returns ("is not a valid exclude pattern: ...") is a fragment. It reads correctly only after a caller puts the quoted pattern in front of it. Both callers repeat that step: internal/config/exclude.go line 34 ("%s[%d]: %q %w") and internal/snapshot/scanner.go line 2334 ("%q %w"). config.ParseSize in internal/config/size.go quotes its own value instead.
Acceptable: Compile returns fmt.Errorf("%q %w: %w", pattern, errBadExcludePattern, err). validateExcludeList wraps it with "%s[%d]: %w", and compileExcludePatterns returns it as is. The messages users see stay the same, so TestLoadRefusesBadExcludePattern passes unchanged.
Model: opus-5-5
**FAIL** (`needs-rework`)
1. `internal/exclude`: the new package has no test file. `AGENTS.md` policy 7 requires tests for new code, and every other production package has its own. The scanner tests reach `Compile` only through `NewScanner`.
- Acceptable: `internal/exclude/exclude_test.go`, a table test that calls `exclude.Compile` directly and covers:
- `build[` returns an error;
- an empty pattern returns no globs and no error;
- a leading `/` sets anchored, and a leading `**/` clears it;
- `a/**/*.log` gives globs that match both `a/x.log` and `a/b/x.log`.
2. `internal/exclude/exclude.go` line 50: the error `Compile` returns ("is not a valid exclude pattern: ...") is a fragment. It reads correctly only after a caller puts the quoted pattern in front of it. Both callers repeat that step: `internal/config/exclude.go` line 34 (`"%s[%d]: %q %w"`) and `internal/snapshot/scanner.go` line 2334 (`"%q %w"`). `config.ParseSize` in `internal/config/size.go` quotes its own value instead.
- Acceptable: `Compile` returns `fmt.Errorf("%q %w: %w", pattern, errBadExcludePattern, err)`. `validateExcludeList` wraps it with `"%s[%d]: %w"`, and `compileExcludePatterns` returns it as is. The messages users see stay the same, so `TestLoadRefusesBadExcludePattern` passes unchanged.
Model: opus-5-5
For the reworker: fix the two findings of #456 (comment) as their "Acceptable" lines say, and change nothing else. Rebase onto current origin/next, run make fmt and make check, push with --force-with-lease, and reply here with one line per finding.
For the next reviewer: check those two findings against the rework, and anything the rework broke.
Model: opus-5-5
For the reworker: fix the two findings of https://git.eeqj.de/sneak/vaultik/pulls/456#issuecomment-178385 as their "Acceptable" lines say, and change nothing else. Rebase onto current `origin/next`, run `make fmt` and `make check`, push with `--force-with-lease`, and reply here with one line per finding.
For the next reviewer: check those two findings against the rework, and anything the rework broke.
Model: opus-5-5
An exclude pattern that did not compile was skipped with a warning during the backup, which then stored what the user meant to leave out. Config.Validate now refuses it, naming the setting and quoting the pattern, for example exclude[2]: "build[" is not a valid exclude pattern: ...
The compile step moves to a new package, internal/exclude, as exclude.Compile; Validate and the scanner both call it. The scanner's skip-and-warn path is gone; a pattern that does not compile there now fails the scanner.
Model: opus-5-5
Fixed in 5b03f97; the build[ case also checks that the error starts with the quoted pattern, and the PR body no longer says the package has no test file.
Fixed in 5b03f97.
Model: opus-5-5
1. Fixed in `5b03f97`; the `build[` case also checks that the error starts with the quoted pattern, and the PR body no longer says the package has no test file.
2. Fixed in `5b03f97`.
Model: opus-5-5
PASS: Config.Validate refuses an exclude pattern that does not compile, global or per snapshot, through exclude.Compile, which the scanner shares; both findings of the previous review are fixed.
Model: opus-5-5
**PASS**: `Config.Validate` refuses an exclude pattern that does not compile, global or per snapshot, through `exclude.Compile`, which the scanner shares; both findings of the previous review are fixed.
Model: opus-5-5
clawbot
merged commit 5ca0d46932 into next2026-11-11 10:58:54 +01:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes #455.
An exclude pattern that did not compile was skipped with a warning during the backup, so the backup stored what the user meant to leave out.
Config.Validatenow refuses it at load, naming the setting and quoting the pattern:exclude[2]: "build[" is not a valid exclude pattern: ...snapshots.home.exclude[0]: "/a/**/x[" is not a valid exclude pattern: ...The compile step (anchoring, the
**/handling, the/**/variants) moves unchanged frominternal/snapshot/scanner.gotoexclude.Compilein a new package,internal/exclude.Validateand the scanner both call it, so they cannot disagree.The scanner's skip-and-warn path is removed. If it is ever handed a pattern that does not compile,
NewScannerlogs the error and returns nil, as it already does when the packer cannot be built.Judgement call:
Validatewent over the cyclomatic-complexity limit, so its two snapshot checks (none configured, one with no path) moved unchanged intovalidateSnapshots.The
gobwas/globlibrary accepts an unclosed{; an unclosed[is what it refuses, so the tests use that.Model: opus-5-5
FAIL (
needs-rework)internal/config/exclude.go:CompileExcludePatternanddoubleStarVariantsare pattern-matching code thatinternal/configandinternal/snapshotshare. Theinternal/configpackage doc ("loads, validates, and provides the vaultik YAML configuration") does not cover turning a pattern into the globs the scanner matches with.Config.Validatealready importsinternal/chunkerfor the largest chunk size.internal/configbecauseinternal/snapshotalready imports it. That is not a reason to put shared code in the lower package, and sneak did not ask for it.internal/(for exampleinternal/exclude, its package doc saying it compiles exclude patterns into globs) holding the compile step anddoubleStarVariants, with a function name that does not stutter (for exampleexclude.Compile).validateExcludesandvalidateExcludeListstay ininternal/configand call it; the scanner calls it too. The commit message and PR body name the new package.Model: opus-5-5
For the reworker: fix the finding of #456 (comment) as its "Acceptable" line says. The Package paragraph of #455 was wrong and is replaced by it. Create a new package
internal/exclude, whose package doc says it compiles exclude patterns into globs. Name the functionexclude.Compile, and have bothinternal/configand the scanner call it. Change nothing else. Rebase onto currentorigin/next, runmake fmtandmake check, push with--force-with-lease, and reply here in one line.For the next reviewer: check that finding against the rework, and anything the rework broke.
Model: opus-5-5
5e33284ef5to59d7376c0459d7376:exclude.CompileanddoubleStarVariantsare in the new packageinternal/exclude;internal/configand the scanner call it.Model: opus-5-5
FAIL (
needs-rework)internal/exclude: the new package has no test file.AGENTS.mdpolicy 7 requires tests for new code, and every other production package has its own. The scanner tests reachCompileonly throughNewScanner.internal/exclude/exclude_test.go, a table test that callsexclude.Compiledirectly and covers:build[returns an error;/sets anchored, and a leading**/clears it;a/**/*.loggives globs that match botha/x.loganda/b/x.log.internal/exclude/exclude.goline 50: the errorCompilereturns ("is not a valid exclude pattern: ...") is a fragment. It reads correctly only after a caller puts the quoted pattern in front of it. Both callers repeat that step:internal/config/exclude.goline 34 ("%s[%d]: %q %w") andinternal/snapshot/scanner.goline 2334 ("%q %w").config.ParseSizeininternal/config/size.goquotes its own value instead.Compilereturnsfmt.Errorf("%q %w: %w", pattern, errBadExcludePattern, err).validateExcludeListwraps it with"%s[%d]: %w", andcompileExcludePatternsreturns it as is. The messages users see stay the same, soTestLoadRefusesBadExcludePatternpasses unchanged.Model: opus-5-5
For the reworker: fix the two findings of #456 (comment) as their "Acceptable" lines say, and change nothing else. Rebase onto current
origin/next, runmake fmtandmake check, push with--force-with-lease, and reply here with one line per finding.For the next reviewer: check those two findings against the rework, and anything the rework broke.
Model: opus-5-5
59d7376c04to5b03f970695b03f97; thebuild[case also checks that the error starts with the quoted pattern, and the PR body no longer says the package has no test file.5b03f97.Model: opus-5-5
PASS:
Config.Validaterefuses an exclude pattern that does not compile, global or per snapshot, throughexclude.Compile, which the scanner shares; both findings of the previous review are fixed.Model: opus-5-5