fix(fix): preserve an existing document's file mode on rewrite - #138
Merged
Conversation
…e internal/fix permission-widening defect, then fixed it under strict TDD by consolidating internal/fix onto internal/fsx, with all three repo gates passing.
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.
Closes #137.
The defect
internal/fixcarried its own atomic-write helper (internal/fix/writefile.go) that never stat'ed the target. It applied the caller'spermunconditionally:All 13 non-test call sites (8 in
internal/fix/fix.go, 5 ininternal/fix/repair.go) passed a hardcoded0o644, sorootline fixandrootline repair applywidened a0600or0400governed document to0644on every rewrite. Rewriting frontmatter is a content operation and carries no instruction to change who may read the file, so this is a privacy regression, not a cosmetic one.internal/fsx/writeFileAtomicwas never affected: it stats the target and overridespermwith the existing mode, which makes its0o644arguments a create-new default only. The real defect was the divergence — two atomic writers with different mode semantics, andinternal/fixwas the unsafe one.The fix
Deleted
internal/fix/writefile.goand routed all 13 call sites throughfsx.WriteFileAtomic, rather than patching mode logic into a second copy that can diverge again.Failure-path cleanup is preserved and slightly strengthened:
internal/fixrelied on five explicitcleanup()calls, whilefsxuses adeferguarded by a committed flag, so staging files are also removed on panic.TestApplyRepairLeavesNoStagingFilespins that half of the contract at the path level.fix.newFileModereplaces the bare0o644literal, with a doc comment stating that the value is a create-new default and is ignored when the target already exists.Regression evidence
internal/fix/filemode_test.godrives both write paths (ApplyRepairandApplyProposals) against a seeded document and asserts the mode is unchanged. Each test first asserts the document actually changed, so a mode check cannot pass vacuously.Against the pre-fix code, all three fail:
With the fix they pass.
Behavior changes this consolidation introduces
Both were named in #137 before landing:
fsxreturns an error whenos.Staton the target fails for a reason other thanNotExist— a failure modeinternal/fixdid not have.Verification
just check,just testandjust coverage-checkall pass.internal/fixcoverage moved 87.3% → 89.0%: 79 well-covered production lines were deleted along with the five helper-level tests that duplicatedinternal/fsx/atomic_test.go.