Stop skipping source packages named build - #43
Conversation
`is_excluded()` tested every path segment against `EXCLUDED_DIRS`, which lists Gradle's output directories by name. `build` names both that output directory and an ordinary source package: `io.spine.dependency.build` holds the declarations for build-time tools. Matching the name at any depth dropped those 12 tracked files from every run, so their headers silently kept a stale year — and because the script reports only what it updated, a skipped file is indistinguishable from one that needed no change. The distinction is positional, not by name. A build tool creates these directories beside a build script, never inside a source tree, so a name that follows a `src` segment denotes a package. Split the set in two: tool and VCS directories still match at any depth, since no package is named `.git`, while the output names match only until a source tree is entered. The Spine `.gitignore` draws the same line, pairing `**/build/**` with `!**/src/**/build/**`. Both regression tests fail against the previous script; the third pins the behaviour that must not change, and passes either way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟢 Approval recommended
The behavioral fix is covered by targeted regression tests, and the only noted follow-up is a non-blocking maintainability/performance improvement to the helper implementation.
Pull request overview
This PR fixes an exclusion bug in update_copyright.py where source files could be silently skipped if any path segment matched an output directory name like build, even when that segment was part of a legitimate source package path (e.g., io.spine.dependency.build). It refines the exclusion logic so build output directory names are only treated as output before entering a src subtree, and adds regression tests to prevent the issue from returning.
Changes:
- Split excluded directories into “tool/VCS” dirs (excluded at any depth) vs. build output dirs (excluded only before
src). - Added
in_build_output()helper to implement the positional “beforesrc” rule for output directories. - Added regression tests covering
buildas a package name (explicit path + tracked-files run) and confirming Gradle output remains excluded.
File summaries
| File | Description |
|---|---|
| skills/update-copyright/scripts/update_copyright.py | Refines exclusion logic to avoid skipping source packages named like build output dirs. |
| skills/update-copyright/tests/test_update_copyright.py | Adds regression tests for build as a source package and for Gradle output exclusion. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ac9caa052
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The positional rule was applied to every output-directory name, which admitted a generated tree laid out inside a source root: `src/main/generated/Foo.java` followed a `src` segment, so it read as a package and would have been re-stamped. `guidelines/coding.md` classifies every `**/generated/**` path as generated whatever its depth, and re-stamping such a file churns an edit the next code generation reverts. `generated` is therefore excluded by name, alongside the tool and VCS directories. The positional rule stays for `build`, `out`, and `tmp`, whose names genuinely double as package names — that ambiguity is what this branch set out to resolve. Also walk the segments once instead of re-scanning the prefix for `src` at every step: the first of the two markers to appear decides the answer, so a single pass settles it and drops the quadratic scan. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
One added test (test_gradle_output_directory_is_still_skipped) currently can’t actually detect regressions in the new positional logic because it’s also excluded by the always-excluded generated segment.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| generated = "buildSrc/build/generated/Generated.kt" | ||
| source = root / generated | ||
| self.write_file(source, STALE_BLOCK + "object Generated\n") | ||
|
|
||
| result = self.run_script(root, "--year", "2026", generated) |
update_copyright.pysilently skipped 12 tracked source files in theconfigrepository. Surfaced while re-stamping every header there for a copyright-holder change: the files underio.spine.dependency.buildcame back untouched.The bug
is_excluded()asked whether any path segment is a denied directory name:EXCLUDED_DIRSlists Gradle's output directories by name,buildamong them. Butbuildnames two different things:The second is
io.spine.dependency.build, the package holding declarations for build-time tools (Dokka, ErrorProne, Pmd, CheckStyle, KSP). The unanchored test matched its seventh segment and dropped the file.Two things made this hard to notice. The filtering happens in
expand_requested_paths()before any work, so it applies to explicitly-passed paths too — naming the file on the command line does not get it stamped. And the script reports only what it did update, so a skipped file looks exactly like one that needed no change. In practice the header year just quietly stops advancing, including on thePostToolUsehook run after each edit.The fix
Not every denied name is ambiguous, so they no longer share one rule.
Excluded by name, at any depth — tool and VCS state (
.git,.idea,.gradle,.agents,.kotlin) plusgenerated. No package is called.git, andguidelines/coding.mdclassifies every**/generated/**path as generated whatever its depth, so a generated tree inside a source root stays out of scope.Excluded by position —
build,out, andtmp, whose names genuinely double as package names. A build tool creates these beside a build script and never inside a source tree, so the first marker to appear decides: asrcsegment means package, an output name means build output. This is the same line the Spine.gitignorealready draws, pairing**/build/**with!**/src/**/build/**— the ambiguity was already known there, just not here.Resulting behaviour:
buildSrc/src/main/kotlin/io/spine/dependency/build/Pmd.ktsrc/main/kotlin/out/Api.ktoutbuildSrc/build/generated/Foo.ktmodule/src/main/generated/Foo.javaTests
Four added:
build, stamped via an explicit pathThe first two fail against the previous script (
Updated 0 file(s).); the others pin behaviour that must not change. Suite: 17 tests, green.Note
guidelines/coding.mdnames**/generated-proto/**alongside**/generated/**, butgenerated-protois not in the exclusion set and was not before this change either. Left alone deliberately — adding it would stop stamping files that are stamped today, which is a behaviour change beyond this fix.🤖 Generated with Claude Code