AddFileWithHash accepted any non-empty bytes as a hash, so the builder could write a manifest that mfer itself refuses to load since #128.
It now decodes the hash with multihash.Decode and refuses one that is not a valid multihash. It also refuses a valid multihash whose digest is shorter than 32 bytes: the reader's limit on decoding cost assumes every hash is at least as long as a SHA-256 multihash, and a manifest of one-character paths whose valid multihashes are 28 bytes or shorter (an empty identity hash, SHA-1) is refused when loaded. The comment on that limit in mfer/constants.go now says the builder enforces it.
What the diff does not show:
An empty or nil hash now fails as "not a valid multihash"; the separate empty-hash error is gone.
Several tests used stand-in hashes that are not valid multihashes (34 zero bytes, or a SHA-256 header with no digest), so they now use a SHA-256 multihash.
mfer freshen carries unchanged files' hashes over through AddFileWithHash, so on a manifest holding a hash this refuses it now stops, whether or not anything changed (it used to report an unchanged manifest and write nothing). The error names the manifest entry and says to regenerate the manifest with mfer generate.
Judgement call: the minimum is SHA-256's 32-byte digest, the size the reader's limit was written for, not the smallest that happens to fit under it (27 bytes).
Judgement call: freshen stops on an unchanged tree too, since it could never update such a manifest; mfer writes only SHA-256.
Model: opus-5-5
`AddFileWithHash` accepted any non-empty bytes as a hash, so the builder could write a manifest that `mfer` itself refuses to load since https://git.eeqj.de/sneak/mfer/pulls/128.
It now decodes the hash with `multihash.Decode` and refuses one that is not a valid multihash. It also refuses a valid multihash whose digest is shorter than 32 bytes: the reader's limit on decoding cost assumes every hash is at least as long as a SHA-256 multihash, and a manifest of one-character paths whose valid multihashes are 28 bytes or shorter (an empty identity hash, SHA-1) is refused when loaded. The comment on that limit in `mfer/constants.go` now says the builder enforces it.
What the diff does not show:
- An empty or nil hash now fails as "not a valid multihash"; the separate empty-hash error is gone.
- Several tests used stand-in hashes that are not valid multihashes (34 zero bytes, or a SHA-256 header with no digest), so they now use a SHA-256 multihash.
- `mfer freshen` carries unchanged files' hashes over through `AddFileWithHash`, so on a manifest holding a hash this refuses it now stops, whether or not anything changed (it used to report an unchanged manifest and write nothing). The error names the manifest entry and says to regenerate the manifest with `mfer generate`.
Judgement call: the minimum is SHA-256's 32-byte digest, the size the reader's limit was written for, not the smallest that happens to fit under it (27 bytes).
Judgement call: freshen stops on an unchanged tree too, since it could never update such a manifest; `mfer` writes only SHA-256.
Model: opus-5-5
mfer/builder_test.go line 127, TestBuilderShortestEntriesLoad: it repeats TestDeserializeLoadsDensestManifest (mfer/deserialize_path_test.go line 206), which already builds a manifest of empty files with modification times at the epoch and SHA-256 multihashes through AddFileWithHash and checks that it loads. Two tests of the same thing leave a reader asking which one counts. Acceptable: one test. Either drop the new one, or give the existing one one-character names if the tighter case is wanted.
PR body, the mfer freshen item: it says freshen now stops "instead of writing one that may not load", but it also stops when nothing in the tree changed, where it used to report the manifest unchanged and write nothing. Acceptable: say it stops in both cases.
Judgement call: a multihash whose function code is not a registered one, with a digest of 32 bytes or more, is accepted. The go-multihash decoder the issue names accepts it and the reader loads it, so I did not count it as a defect.
Model: opus-5-5
Review failed.
1. `mfer/builder_test.go` line 127, `TestBuilderShortestEntriesLoad`: it repeats `TestDeserializeLoadsDensestManifest` (`mfer/deserialize_path_test.go` line 206), which already builds a manifest of empty files with modification times at the epoch and SHA-256 multihashes through `AddFileWithHash` and checks that it loads. Two tests of the same thing leave a reader asking which one counts. Acceptable: one test. Either drop the new one, or give the existing one one-character names if the tighter case is wanted.
2. PR body, the `mfer freshen` item: it says freshen now stops "instead of writing one that may not load", but it also stops when nothing in the tree changed, where it used to report the manifest unchanged and write nothing. Acceptable: say it stops in both cases.
Judgement call: a multihash whose function code is not a registered one, with a digest of 32 bytes or more, is accepted. The go-multihash decoder the issue names accepts it and the reader loads it, so I did not count it as a defect.
Model: opus-5-5
Dropped TestBuilderShortestEntriesLoad; TestDeserializeLoadsDensestManifest remains the one test of a densest manifest loading.
PR body now says mfer freshen stops on such a manifest whether or not anything changed. Kept that stop on an unchanged tree rather than treating it as a defect: freshen could never update such a manifest, and mfer writes only SHA-256 hashes.
Model: opus-5-5
Rework:
1. Dropped `TestBuilderShortestEntriesLoad`; `TestDeserializeLoadsDensestManifest` remains the one test of a densest manifest loading.
2. PR body now says `mfer freshen` stops on such a manifest whether or not anything changed. Kept that stop on an unchanged tree rather than treating it as a defect: freshen could never update such a manifest, and `mfer` writes only SHA-256 hashes.
Model: opus-5-5
internal/cli/freshen.go, addExistingToBuilder (line 618): when mfer freshen carries over an unchanged file whose hash in the existing manifest is shorter than SHA-256, it now stops with failed to add a.txt: hash digest is too short: 20 bytes, at least 32 needed. That reads as a problem with the file, not with the manifest, and does not say what to do. Stopping is acceptable pre-1.0 with no installed base; the message is not. Acceptable: the error names the manifest entry as the cause, as the missing-mtime error beside it does, and tells the user to regenerate the manifest with mfer generate; a test covers it.
internal/cli/freshen_test.go line 184, TestFreshenAddExistingRejectsMissingMtime: the stand-in hash {0x12, 0x20} is a SHA-256 multihash header with no digest, which AddFileWithHash now refuses, so the entry that test means to be valid except for its missing modification time now has two defects. Acceptable: a SHA-256 multihash, like the other fixtures this PR changed.
Judgement call: a multihash whose function code is not a registered one, with a digest of 32 bytes or more, is accepted; the go-multihash decoder the issue names accepts it and the reader loads it, so I did not count it as a defect.
Model: opus-5-5
Review failed.
1. `internal/cli/freshen.go`, `addExistingToBuilder` (line 618): when `mfer freshen` carries over an unchanged file whose hash in the existing manifest is shorter than SHA-256, it now stops with `failed to add a.txt: hash digest is too short: 20 bytes, at least 32 needed`. That reads as a problem with the file, not with the manifest, and does not say what to do. Stopping is acceptable pre-1.0 with no installed base; the message is not. Acceptable: the error names the manifest entry as the cause, as the missing-mtime error beside it does, and tells the user to regenerate the manifest with `mfer generate`; a test covers it.
2. `internal/cli/freshen_test.go` line 184, `TestFreshenAddExistingRejectsMissingMtime`: the stand-in hash `{0x12, 0x20}` is a SHA-256 multihash header with no digest, which `AddFileWithHash` now refuses, so the entry that test means to be valid except for its missing modification time now has two defects. Acceptable: a SHA-256 multihash, like the other fixtures this PR changed.
Judgement call: a multihash whose function code is not a registered one, with a digest of 32 bytes or more, is accepted; the go-multihash decoder the issue names accepts it and the reader loads it, so I did not count it as a defect.
Model: opus-5-5
addExistingToBuilder now reports the refusal as manifest entry a.txt: ... (regenerate the manifest with mfer generate); TestFreshenAddExistingRejectsShortHash covers it.
TestFreshenAddExistingRejectsMissingMtime now uses a SHA-256 multihash.
Model: opus-5-5
Rework:
1. `addExistingToBuilder` now reports the refusal as `manifest entry a.txt: ... (regenerate the manifest with mfer generate)`; `TestFreshenAddExistingRejectsShortHash` covers it.
2. `TestFreshenAddExistingRejectsMissingMtime` now uses a SHA-256 multihash.
Model: opus-5-5
mfer/builder_test.go, TestBuilderAddFileWithHashRejectsBadHashes: the 32-byte minimum is tested only with a 20-byte SHA-1 digest, so the tests still pass if the minimum is lowered far enough (anything from 21 to 26 bytes) for the builder to write manifests the reader refuses, which is the defect this PR fixes. Acceptable: a row with a valid multihash whose digest is 31 bytes, refused with errHashTooShort.
Judgement call: mfer freshen stopping on a manifest with hashes shorter than SHA-256, changed or not, is acceptable pre-1.0 with no installed base; its message names the entry and says to regenerate with mfer generate.
Judgement call: a multihash with an unregistered function code, or with a digest longer than its function produces, is accepted when the digest is 32 bytes or more; the go-multihash decoder the issue names accepts it and the reader loads it.
Model: opus-5-5
Review failed.
1. `mfer/builder_test.go`, `TestBuilderAddFileWithHashRejectsBadHashes`: the 32-byte minimum is tested only with a 20-byte SHA-1 digest, so the tests still pass if the minimum is lowered far enough (anything from 21 to 26 bytes) for the builder to write manifests the reader refuses, which is the defect this PR fixes. Acceptable: a row with a valid multihash whose digest is 31 bytes, refused with `errHashTooShort`.
Judgement call: `mfer freshen` stopping on a manifest with hashes shorter than SHA-256, changed or not, is acceptable pre-1.0 with no installed base; its message names the entry and says to regenerate with `mfer generate`.
Judgement call: a multihash with an unregistered function code, or with a digest longer than its function produces, is accepted when the digest is 32 bytes or more; the go-multihash decoder the issue names accepts it and the reader loads it.
Model: opus-5-5
AddFileWithHash took any non-empty bytes as a hash, so the builder could
write a manifest that mfer refuses to load. It now decodes the hash with
go-multihash and also requires a digest of at least 32 bytes, the SHA-256
length the reader's decoding-cost limit assumes: a valid but shorter
multihash, such as an empty identity hash or SHA-1, still makes a
manifest of one-character paths too costly to load. When mfer freshen
meets such a hash in an existing manifest, its error names the manifest
entry and says to regenerate the manifest with mfer generate. Test
fixtures whose stand-in hashes were not valid multihashes now use a
SHA-256 multihash.
Model: opus-5-5
Rework: TestBuilderAddFileWithHashRejectsBadHashes now also refuses a SHA-256 multihash with a 31-byte digest (errHashTooShort); the 32-byte case is already accepted in TestBuilderAddFileWithHashValidation.
Model: opus-5-5
Rework: `TestBuilderAddFileWithHashRejectsBadHashes` now also refuses a SHA-256 multihash with a 31-byte digest (`errHashTooShort`); the 32-byte case is already accepted in `TestBuilderAddFileWithHashValidation`.
Model: opus-5-5
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.
AddFileWithHashaccepted any non-empty bytes as a hash, so the builder could write a manifest thatmferitself refuses to load since #128.It now decodes the hash with
multihash.Decodeand refuses one that is not a valid multihash. It also refuses a valid multihash whose digest is shorter than 32 bytes: the reader's limit on decoding cost assumes every hash is at least as long as a SHA-256 multihash, and a manifest of one-character paths whose valid multihashes are 28 bytes or shorter (an empty identity hash, SHA-1) is refused when loaded. The comment on that limit inmfer/constants.gonow says the builder enforces it.What the diff does not show:
mfer freshencarries unchanged files' hashes over throughAddFileWithHash, so on a manifest holding a hash this refuses it now stops, whether or not anything changed (it used to report an unchanged manifest and write nothing). The error names the manifest entry and says to regenerate the manifest withmfer generate.Judgement call: the minimum is SHA-256's 32-byte digest, the size the reader's limit was written for, not the smallest that happens to fit under it (27 bytes).
Judgement call: freshen stops on an unchanged tree too, since it could never update such a manifest;
mferwrites only SHA-256.Model: opus-5-5
Review failed.
mfer/builder_test.goline 127,TestBuilderShortestEntriesLoad: it repeatsTestDeserializeLoadsDensestManifest(mfer/deserialize_path_test.goline 206), which already builds a manifest of empty files with modification times at the epoch and SHA-256 multihashes throughAddFileWithHashand checks that it loads. Two tests of the same thing leave a reader asking which one counts. Acceptable: one test. Either drop the new one, or give the existing one one-character names if the tighter case is wanted.mfer freshenitem: it says freshen now stops "instead of writing one that may not load", but it also stops when nothing in the tree changed, where it used to report the manifest unchanged and write nothing. Acceptable: say it stops in both cases.Judgement call: a multihash whose function code is not a registered one, with a digest of 32 bytes or more, is accepted. The go-multihash decoder the issue names accepts it and the reader loads it, so I did not count it as a defect.
Model: opus-5-5
c74318145ato979925dabdRework:
TestBuilderShortestEntriesLoad;TestDeserializeLoadsDensestManifestremains the one test of a densest manifest loading.mfer freshenstops on such a manifest whether or not anything changed. Kept that stop on an unchanged tree rather than treating it as a defect: freshen could never update such a manifest, andmferwrites only SHA-256 hashes.Model: opus-5-5
Review failed.
internal/cli/freshen.go,addExistingToBuilder(line 618): whenmfer freshencarries over an unchanged file whose hash in the existing manifest is shorter than SHA-256, it now stops withfailed to add a.txt: hash digest is too short: 20 bytes, at least 32 needed. That reads as a problem with the file, not with the manifest, and does not say what to do. Stopping is acceptable pre-1.0 with no installed base; the message is not. Acceptable: the error names the manifest entry as the cause, as the missing-mtime error beside it does, and tells the user to regenerate the manifest withmfer generate; a test covers it.internal/cli/freshen_test.goline 184,TestFreshenAddExistingRejectsMissingMtime: the stand-in hash{0x12, 0x20}is a SHA-256 multihash header with no digest, whichAddFileWithHashnow refuses, so the entry that test means to be valid except for its missing modification time now has two defects. Acceptable: a SHA-256 multihash, like the other fixtures this PR changed.Judgement call: a multihash whose function code is not a registered one, with a digest of 32 bytes or more, is accepted; the go-multihash decoder the issue names accepts it and the reader loads it, so I did not count it as a defect.
Model: opus-5-5
979925dabdto0019f9afbfRework:
addExistingToBuildernow reports the refusal asmanifest entry a.txt: ... (regenerate the manifest with mfer generate);TestFreshenAddExistingRejectsShortHashcovers it.TestFreshenAddExistingRejectsMissingMtimenow uses a SHA-256 multihash.Model: opus-5-5
Review failed.
mfer/builder_test.go,TestBuilderAddFileWithHashRejectsBadHashes: the 32-byte minimum is tested only with a 20-byte SHA-1 digest, so the tests still pass if the minimum is lowered far enough (anything from 21 to 26 bytes) for the builder to write manifests the reader refuses, which is the defect this PR fixes. Acceptable: a row with a valid multihash whose digest is 31 bytes, refused witherrHashTooShort.Judgement call:
mfer freshenstopping on a manifest with hashes shorter than SHA-256, changed or not, is acceptable pre-1.0 with no installed base; its message names the entry and says to regenerate withmfer generate.Judgement call: a multihash with an unregistered function code, or with a digest longer than its function produces, is accepted when the digest is 32 bytes or more; the go-multihash decoder the issue names accepts it and the reader loads it.
Model: opus-5-5
0019f9afbfto30907d9b94Rework:
TestBuilderAddFileWithHashRejectsBadHashesnow also refuses a SHA-256 multihash with a 31-byte digest (errHashTooShort); the 32-byte case is already accepted inTestBuilderAddFileWithHashValidation.Model: opus-5-5
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.