Pin the one unpinned shebang script, and gate the pin list both ways - #69
Merged
Conversation
`ops/vps-backup-pull` is extensionless, so no `*.sh` or `*.py` rule reached
it, and `* -text` keeps git passive by default. A CRLF checkout would hand
systemd a broken interpreter line for a script it runs unattended.
The reason it was missed is the more interesting half. `.gitattributes`
already pinned `deploy/blog-deploy-shell` and `deploy/authorized_keys`, and
this repository has never carried either file, in any commit: they live on
the server, described in OPERATIONS.md "Server Hardening". A pin for a file
that does not exist binds nothing, and the comment above those two claimed
the extensionless shebang case was handled. That is what made the real one
invisible. Both pins are dropped, with the reason recorded where they were.
`deploy/bootstrap.Caddyfile` gets a pin it never had. It is read by Caddy
from the container's config directory and is the only Caddy file outside
the release bundle, so it is the same daemon-parsed class as
`deploy/Caddyfile` beside it, which was pinned.
`checks/check-eol-pins.py` reads the list back, in both directions, because
a hand-maintained list that nothing verifies is how this got here:
unpinned a tracked file opening with `#!` whose resolved eol is not lf.
The shebang is the test rather than the mode bit, because the
two move independently and it is the interpreter line a CRLF
breaks.
dead a pattern matching no tracked file, which reads as coverage
while binding nothing.
It asks `git check-attr` for the resolved attribute instead of
re-implementing the match rules, so it cannot disagree with what git
actually applies on checkout, and it fails rather than passing if it finds
no shebang files at all.
Verified by reintroducing each defect against the gate: dropping the
`ops/vps-backup-pull` line reports it unpinned, and re-adding
`deploy/blog-deploy-shell` reports it dead. It also caught its own file
before that was staged, which is the third direction working.
`git add --renormalize .` after the change stages nothing beyond these
three files, so the pins bind future checkouts and rewrite no bytes now.
systemd units are deliberately not pinned. Measured rather than assumed:
`systemd-analyze verify` on a CRLF copy of `vps-backup-pull.service`
reports the same clean result as the LF original, and the same tool does
flag an unreachable `ExecStart`, so the negative means something.
Found by Copilot review on #68.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This pull request strengthens the repository’s line-ending enforcement by pinning an extensionless shebang script to LF in .gitattributes and introducing a CI gate that checks .gitattributes coverage in both directions (shebang files must resolve to eol=lf, and .gitattributes patterns must match at least one tracked file).
Changes:
- Add
checks/check-eol-pins.pyto detect (1) unpinned tracked shebang files and (2) “dead”.gitattributespatterns that match no tracked files. - Run the new check in
.github/workflows/validate-task.ymlalongside existing source validation. - Update
.gitattributesto pinops/vps-backup-pull(andchecks/check-eol-pins.py) to LF, remove never-tracked pins, and add an LF pin fordeploy/bootstrap.Caddyfile.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
checks/check-eol-pins.py |
Adds a new validation gate that checks shebang LF pinning and detects dead .gitattributes patterns via git-resolved attributes. |
.github/workflows/validate-task.yml |
Runs the new line-ending pin gate in CI during the validation workflow. |
.gitattributes |
Pins the extensionless shebang script to LF, pins the new checker script, removes dead pins, and pins deploy/bootstrap.Caddyfile. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…false The workflow comment read "a hand-maintained list that nothing read back", which is both ungrammatical and, as of the step it introduces, no longer true. The script's own docstring had the same problem in the present tense: it asserted that nothing reads the list back, which its own existence disproves. Found by Copilot review on #69, which caught the workflow half. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found by Copilot review on #68, which flagged that
ops/vps-backup-pullis an extensionless shebang script with no LF pin. It is, and the reason it was missed turned out to be the larger half.The finding
.gitattributeskeeps git passive with* -textand then names the files whose line endings are load-bearing.ops/vps-backup-pullcarries no extension, so no*.shor*.pyrule reached it. A CRLF checkout hands systemd a broken interpreter line for a script it runs unattended on the backup host.Why it was invisible
Two pins in that file name files this repository has never carried, in any commit:
deploy/blog-deploy-shelldeploy/authorized_keysBoth live on the server, described in
OPERATIONS.md"Server Hardening". A pin binds nothing for a file that does not exist, and the comment above those two said "The deploy shell is an extensionless shebang script that matches no rule above." So the file read as though the extensionless case was covered, while the one real instance sat unpinned twenty lines up. Both are dropped, with the reason recorded in place so neither returns as an oversight.deploy/bootstrap.Caddyfilegets a pin it never had. Caddy reads it from the container's config directory and it is the only Caddy file outside the release bundle, so it is the same daemon-parsed class asdeploy/Caddyfilesitting beside it, which was pinned.The gate
A hand-maintained list that nothing reads back is how both defects got here, so
checks/check-eol-pins.pyreads it in both directions:#!whose resolvedeolis notlf. The shebang is the test rather than the mode bit, because the two move independently and it is the interpreter line that a CRLF breaks.It asks
git check-attrfor the resolved attribute rather than re-implementing the match rules, so it cannot disagree with what git actually applies on checkout, and it fails rather than passing if it finds no shebang files at all. It runs inLint sources jobbesidecheck-env-docs.py.Verification
Each defect was reintroduced and the gate was watched failing on it, rather than the gate being trusted because it passes:
It also caught its own file before that was staged, which is the third direction working.
git add --renormalize .after the change stages nothing beyond the three files here, so the pins bind future checkouts and rewrite no bytes now.The systemd units are deliberately not pinned, measured rather than assumed.
systemd-analyze verifyon a CRLF copy ofvps-backup-pull.servicereports the same clean result as the LF original, and the same tool does flag an unreachableExecStart, so the negative means something. They are not in this class.ops/install.shneeded nothing:*.shalready covers it.