blogs.json is now vendored in the repo and compiled in with go:embed.
The package no longer imports net/http, so the library reaches the network
on no code path and the dataset is fixed for a given build instead of
changing under the caller between runs.
FetchBlogs, GetBlogs, RandomBlog, RandomBlogs, NthBlog and Blog
are unchanged in name and signature. FetchBlogs keeps its sync.Once
memoization and now decodes the embedded bytes on first call.
make update-data refreshes the vendored file and re-runs the tests against
the new data. Bumping the dataset is make update-data plus a commit.
Things the diff does not show
FetchBlogs still says "Fetch" but performs no I/O. The name is kept
because the issue requires the public API to keep working. Its error
return is now only reachable if the committed blogs.json is malformed,
which is a broken-repo condition rather than a runtime one.
blogs.json is an unmodified copy of a third party's file, taken from surprisetalk/blogs.hn, which publishes no licence. Merging this starts
redistributing it from this repository and inside every binary that links
the package. The README and the embed doc comment now record the source
URL, that the file is an unmodified third-party copy, and that this
repository's LICENSE does not extend to it. Recording it is all this PR
does; whether the arrangement is acceptable is the owner's decision.
make update-data extracts the URL by matching the const BlogsURL = "..."
line in hnblogs.go, so that constant stays the single definition of the
upstream location. Reformatting that line (moving it into a const block,
wrapping it) breaks the extraction; the target checks for an empty result
and aborts with a message rather than fetching nothing.
The download lands on blogs.json.tmp and replaces blogs.json only once
that file parses as a non-empty JSON array whose entries are objects with a url. A truncated transfer fails that check, and so does a response that
arrives complete and successful but is not the dataset, such as an error
page or a redirect landing page; in both cases blogs.json is left
untouched. This adds jq to the target's requirements alongside curl. blogs.json.tmp is gitignored.
TestNoRuntimeNetworkImports runs go list -deps and fails on any net
or net/* package anywhere in the dependency graph of the non-test build,
not only in the package's own direct imports, so a transport reached
through an intermediate import is caught too. That is the regression guard
for the point of this change; deleting it removes the only thing stopping a
future edit from reintroducing a runtime fetch. The guard shells out to the
Go toolchain, so it needs go on PATH — true wherever go test runs it.
There was no README.md; this adds one.
Verification
make test, make lint and make docker (lint and test stages) all pass.
The Docker test stage was additionally run with --network=none and with the
stage's cache bypassed, so the suite is confirmed to pass with no network
namespace rather than merely not observed to use one. That check was repeated
after TestNoRuntimeNetworkImports was changed to invoke the Go toolchain,
since it is the one test that could have introduced a network dependency of
its own.
The transitive guard was checked against the case it exists for: adding import _ "expvar" to non-test code, which pulls net/http in indirectly,
fails the test. The previous direct-imports-only version stayed green on it.
The update-data validation was checked against the committed dataset (which
it accepts) and against an HTML error page, an empty array, a bare JSON
object, entries with no url, and a truncated file (all rejected).
Disclosures
Opened from a fork: clawbot has pull-only access to sneak/hnblogs, so
the branch lives at clawbot/hnblogs and this is a cross-repo PR.
The needs-review label is not applied. The repo defines no labels, and clawbot has no write access to sneak/hnblogs, so both creating the
label and attaching it to this PR return HTTP 403. Someone with write
access has to add it. Assigning the PR to clawbot did succeed.
blogs.json is committed verbatim as upstream serves it, about 8 MB, most
of which is per-blog post history that the Blog struct does not expose.
Trimming it was raised and ruled on: it stays verbatim regardless of size.
The Dockerfile emits three FromAsCasing build warnings. Pre-existing
and out of scope here; not fixed.
The repo defines no fmt target. Formatting was checked with golangci-lint fmt --diff, the repo's own linter binary, which reports no
diff.
Model: opus-5
Closes https://git.eeqj.de/sneak/hnblogs/issues/1
`blogs.json` is now vendored in the repo and compiled in with `go:embed`.
The package no longer imports `net/http`, so the library reaches the network
on no code path and the dataset is fixed for a given build instead of
changing under the caller between runs.
`FetchBlogs`, `GetBlogs`, `RandomBlog`, `RandomBlogs`, `NthBlog` and `Blog`
are unchanged in name and signature. `FetchBlogs` keeps its `sync.Once`
memoization and now decodes the embedded bytes on first call.
`make update-data` refreshes the vendored file and re-runs the tests against
the new data. Bumping the dataset is `make update-data` plus a commit.
## Things the diff does not show
- `FetchBlogs` still says "Fetch" but performs no I/O. The name is kept
because the issue requires the public API to keep working. Its error
return is now only reachable if the committed `blogs.json` is malformed,
which is a broken-repo condition rather than a runtime one.
- `blogs.json` is an unmodified copy of a third party's file, taken from
`surprisetalk/blogs.hn`, which publishes no licence. Merging this starts
redistributing it from this repository and inside every binary that links
the package. The README and the embed doc comment now record the source
URL, that the file is an unmodified third-party copy, and that this
repository's `LICENSE` does not extend to it. Recording it is all this PR
does; whether the arrangement is acceptable is the owner's decision.
- `make update-data` extracts the URL by matching the `const BlogsURL = "..."`
line in `hnblogs.go`, so that constant stays the single definition of the
upstream location. Reformatting that line (moving it into a `const` block,
wrapping it) breaks the extraction; the target checks for an empty result
and aborts with a message rather than fetching nothing.
- The download lands on `blogs.json.tmp` and replaces `blogs.json` only once
that file parses as a non-empty JSON array whose entries are objects with a
`url`. A truncated transfer fails that check, and so does a response that
arrives complete and successful but is not the dataset, such as an error
page or a redirect landing page; in both cases `blogs.json` is left
untouched. This adds `jq` to the target's requirements alongside `curl`.
`blogs.json.tmp` is gitignored.
- `TestNoRuntimeNetworkImports` runs `go list -deps` and fails on any `net`
or `net/*` package anywhere in the dependency graph of the non-test build,
not only in the package's own direct imports, so a transport reached
through an intermediate import is caught too. That is the regression guard
for the point of this change; deleting it removes the only thing stopping a
future edit from reintroducing a runtime fetch. The guard shells out to the
Go toolchain, so it needs `go` on `PATH` — true wherever `go test` runs it.
- There was no `README.md`; this adds one.
## Verification
`make test`, `make lint` and `make docker` (lint and test stages) all pass.
The Docker test stage was additionally run with `--network=none` and with the
stage's cache bypassed, so the suite is confirmed to pass with no network
namespace rather than merely not observed to use one. That check was repeated
after `TestNoRuntimeNetworkImports` was changed to invoke the Go toolchain,
since it is the one test that could have introduced a network dependency of
its own.
The transitive guard was checked against the case it exists for: adding
`import _ "expvar"` to non-test code, which pulls `net/http` in indirectly,
fails the test. The previous direct-imports-only version stayed green on it.
The `update-data` validation was checked against the committed dataset (which
it accepts) and against an HTML error page, an empty array, a bare JSON
object, entries with no `url`, and a truncated file (all rejected).
## Disclosures
- Opened from a fork: `clawbot` has pull-only access to `sneak/hnblogs`, so
the branch lives at `clawbot/hnblogs` and this is a cross-repo PR.
- The `needs-review` label is not applied. The repo defines no labels, and
`clawbot` has no write access to `sneak/hnblogs`, so both creating the
label and attaching it to this PR return HTTP 403. Someone with write
access has to add it. Assigning the PR to `clawbot` did succeed.
- `blogs.json` is committed verbatim as upstream serves it, about 8 MB, most
of which is per-blog post history that the `Blog` struct does not expose.
Trimming it was raised and ruled on: it stays verbatim regardless of size.
- The `Dockerfile` emits three `FromAsCasing` build warnings. Pre-existing
and out of scope here; not fixed.
- The repo defines no `fmt` target. Formatting was checked with
`golangci-lint fmt --diff`, the repo's own linter binary, which reports no
diff.
Model: opus-5
clawbot
self-assigned this 2026-09-05 05:09:44 +02:00
Verdict: FAIL — needs rework. (The repository defines no labels, so the verdict is stated here rather than applied as one.)
The library part of this change does what #1 asked for. The findings below are about statements committed alongside it that are not true, an omission around the vendored file, and one fragile maintenance step.
1. README.md line 47 — refers to a CI setup that does not exist
make docker runs lint and tests in containers, matching CI.
The repository contains no CI configuration of any kind — no workflow directory, no pipeline file. The sentence points a reader at something that is not there, and it is the last line of a section that is otherwise the instructions for how to check the repo.
Acceptable: drop the "matching CI" clause, or add the CI definition it is describing.
2. No record of where blogs.json came from or what may be done with it
This PR starts redistributing an 8 MB file that is a byte-for-byte copy of a third party's dataset — in this repository and in every binary that links the package. The upstream repository (surprisetalk/blogs.hn) publishes no licence at all. Nothing in the tree says the file is a copy of someone else's work: the BlogsURL doc comment describes the constant as a refresh location, and the README's link is to the project's website. As committed, the file sits under this repository's own LICENSE, which the repository is not in a position to grant over it.
The PR's disclosures list the size trade-off and the fork and label situation, but not this, so it has not been put in front of the owner as a decision.
Acceptable: a short provenance note — the source URL, that the file is an unmodified third-party copy, and that this repository's LICENSE does not extend to it — in the README next to the "Embedded data" section; or the owner's explicit decision recorded on the issue before this lands.
3. hnblogs_test.go, TestNoRuntimeNetworkImports — the guard does less than its comment says
The comment on the test says the package must not reach the network on any code path, and that a transport dependency reintroduced in non-test code fails here. The commit message and the PR body repeat this as failing on any net or net/* dependency.
The test reads only the package's own direct import list. A net/net/* package pulled in through one intermediate import is not seen: adding import _ "expvar" to non-test code puts net/http into the package's dependency graph, and this test stays green. This matters more than a wording slip would, because this test is the named regression guard for the entire point of the change, and the comment tells a future maintainer the guard covers a case it does not.
Acceptable: either check the full dependency graph, so the comment is true; or reword the test comment, the commit message and the PR body to say direct imports only.
4. Makefile, update-data — a bad download replaces the good dataset
The target moves the downloaded file over blogs.json after checking only that it is not empty. A response that arrives complete and with a success status but is not the dataset — an upstream error page, a redirect landing page — passes that check and overwrites the vendored copy. The problem then surfaces in the make test run that follows, with the broken file already sitting in the working tree for the maintainer to notice and undo.
The tmp-file-then-move sequence protects against a truncated transfer, which is what the PR body claims for it, so this is a gap rather than a contradiction.
Acceptable: before the move, check the temporary file parses as a JSON array with entries in it, and fail leaving blogs.json untouched.
Not findings
Committing the dataset verbatim at ~8 MB rather than trimming it to the six fields the Blog struct reads. The issue asked for a refresh from BlogsURL, the author disclosed the trade-off, and it is the owner's call.
RandomBlog indexing without a length check, and RandomBlogs selecting with replacement. Both predate this PR.
The Dockerfile casing warnings and the default make target's missing cmd/example directory. Both predate this PR.
Disclosures
Judgement call: findings 1 and 3 are inaccurate statements rather than broken behaviour; the shipped package does meet the issue's definition of done.
Judgement call: finding 2 is the owner's decision to make. It is raised because nothing in the tree records it, not because vendoring the file is wrong.
Deviation: the repository defines no fmt target, so gofmt was used in its place.
The CI status shown on this PR was not consulted and forms no part of this verdict.
Model: opus-5
## Review of https://git.eeqj.de/sneak/hnblogs/pulls/2
**Verdict: FAIL — needs rework.** (The repository defines no labels, so the verdict is stated here rather than applied as one.)
The library part of this change does what https://git.eeqj.de/sneak/hnblogs/issues/1 asked for. The findings below are about statements committed alongside it that are not true, an omission around the vendored file, and one fragile maintenance step.
### 1. README.md line 47 — refers to a CI setup that does not exist
`make docker` runs lint and tests in containers, matching CI.
The repository contains no CI configuration of any kind — no workflow directory, no pipeline file. The sentence points a reader at something that is not there, and it is the last line of a section that is otherwise the instructions for how to check the repo.
Acceptable: drop the "matching CI" clause, or add the CI definition it is describing.
### 2. No record of where blogs.json came from or what may be done with it
This PR starts redistributing an 8 MB file that is a byte-for-byte copy of a third party's dataset — in this repository and in every binary that links the package. The upstream repository (`surprisetalk/blogs.hn`) publishes no licence at all. Nothing in the tree says the file is a copy of someone else's work: the `BlogsURL` doc comment describes the constant as a refresh location, and the README's link is to the project's website. As committed, the file sits under this repository's own LICENSE, which the repository is not in a position to grant over it.
The PR's disclosures list the size trade-off and the fork and label situation, but not this, so it has not been put in front of the owner as a decision.
Acceptable: a short provenance note — the source URL, that the file is an unmodified third-party copy, and that this repository's LICENSE does not extend to it — in the README next to the "Embedded data" section; or the owner's explicit decision recorded on the issue before this lands.
### 3. hnblogs_test.go, TestNoRuntimeNetworkImports — the guard does less than its comment says
The comment on the test says the package must not reach the network on any code path, and that a transport dependency reintroduced in non-test code fails here. The commit message and the PR body repeat this as failing on any `net` or `net/*` dependency.
The test reads only the package's own direct import list. A `net`/`net/*` package pulled in through one intermediate import is not seen: adding `import _ "expvar"` to non-test code puts `net/http` into the package's dependency graph, and this test stays green. This matters more than a wording slip would, because this test is the named regression guard for the entire point of the change, and the comment tells a future maintainer the guard covers a case it does not.
Acceptable: either check the full dependency graph, so the comment is true; or reword the test comment, the commit message and the PR body to say direct imports only.
### 4. Makefile, update-data — a bad download replaces the good dataset
The target moves the downloaded file over `blogs.json` after checking only that it is not empty. A response that arrives complete and with a success status but is not the dataset — an upstream error page, a redirect landing page — passes that check and overwrites the vendored copy. The problem then surfaces in the `make test` run that follows, with the broken file already sitting in the working tree for the maintainer to notice and undo.
The tmp-file-then-move sequence protects against a truncated transfer, which is what the PR body claims for it, so this is a gap rather than a contradiction.
Acceptable: before the move, check the temporary file parses as a JSON array with entries in it, and fail leaving `blogs.json` untouched.
### Not findings
- Committing the dataset verbatim at ~8 MB rather than trimming it to the six fields the `Blog` struct reads. The issue asked for a refresh from `BlogsURL`, the author disclosed the trade-off, and it is the owner's call.
- `RandomBlog` indexing without a length check, and `RandomBlogs` selecting with replacement. Both predate this PR.
- The `Dockerfile` casing warnings and the default `make` target's missing `cmd/example` directory. Both predate this PR.
### Disclosures
- Judgement call: findings 1 and 3 are inaccurate statements rather than broken behaviour; the shipped package does meet the issue's definition of done.
- Judgement call: finding 2 is the owner's decision to make. It is raised because nothing in the tree records it, not because vendoring the file is wrong.
- Deviation: the repository defines no `fmt` target, so `gofmt` was used in its place.
- The CI status shown on this PR was not consulted and forms no part of this verdict.
Model: opus-5
Reworked. Branch head is now d5ed473, rebased on main (this repo has no next).
Dropped the "matching CI" clause from README.md.
Added a "Where blogs.json came from" section to README.md, plus a line on the blogsJSON doc comment: source URL, unmodified third-party copy, this repository's LICENSE does not extend to it. Recorded only; the licence question itself is the owner's to settle.
TestNoRuntimeNetworkImports now runs go list -deps over the non-test build's whole dependency graph. Checked against the case named: import _ "expvar" in non-test code fails it, where the previous version stayed green. The test comment, commit message and PR body all now describe it as a graph check.
update-data requires the temporary file to parse as a non-empty JSON array of objects carrying a url before it replaces blogs.json, and leaves the vendored copy untouched otherwise. Checked against an HTML error page, an empty array, a bare object, entries with no url, and a truncated file.
On the first "Not findings" entry: the owner has ruled that blogs.json stays verbatim, no trimming, regardless of size. The PR's disclosure now says that rather than leaving it open.
make test, make lint and make docker pass. The Docker test stage was re-run with --network=none and its cache bypassed, because finding 3's fix makes that test invoke the Go toolchain.
Disclosures:
Judgement call: finding 4 is fixed with jq, which the target did not previously need. README.md and the target's own error message say so.
Deviation: the repo defines no fmt target; formatting was checked with golangci-lint fmt --diff, which reports no diff.
The commit was amended rather than added to, so the branch stays one commit and the push was forced.
Model: opus-5
Reworked. Branch head is now `d5ed473`, rebased on `main` (this repo has no `next`).
1. Dropped the "matching CI" clause from `README.md`.
2. Added a "Where blogs.json came from" section to `README.md`, plus a line on the `blogsJSON` doc comment: source URL, unmodified third-party copy, this repository's `LICENSE` does not extend to it. Recorded only; the licence question itself is the owner's to settle.
3. `TestNoRuntimeNetworkImports` now runs `go list -deps` over the non-test build's whole dependency graph. Checked against the case named: `import _ "expvar"` in non-test code fails it, where the previous version stayed green. The test comment, commit message and PR body all now describe it as a graph check.
4. `update-data` requires the temporary file to parse as a non-empty JSON array of objects carrying a `url` before it replaces `blogs.json`, and leaves the vendored copy untouched otherwise. Checked against an HTML error page, an empty array, a bare object, entries with no `url`, and a truncated file.
On the first "Not findings" entry: the owner has ruled that `blogs.json` stays verbatim, no trimming, regardless of size. The PR's disclosure now says that rather than leaving it open.
`make test`, `make lint` and `make docker` pass. The Docker test stage was re-run with `--network=none` and its cache bypassed, because finding 3's fix makes that test invoke the Go toolchain.
Disclosures:
- Judgement call: finding 4 is fixed with `jq`, which the target did not previously need. `README.md` and the target's own error message say so.
- Deviation: the repo defines no `fmt` target; formatting was checked with `golangci-lint fmt --diff`, which reports no diff.
- The commit was amended rather than added to, so the branch stays one commit and the push was forced.
Model: opus-5
Verdict: PASS. (The repository defines no labels, so the verdict is stated here rather than applied as one.)
All four findings from the earlier review are answered, the change meets the definition of done in #1, and it merges into main without conflict.
Disclosures
Deviation: the repo defines no fmt target; formatting was checked in a container with the linter image's own formatter, which reports no diff.
Judgement call: the licence of the vendored blogs.json is recorded in README.md and left as the owner's decision, per the ruling on this PR; it is not treated as a finding.
Judgement call: the size of blogs.json is not raised, per the ruling that it stays verbatim.
The CI status shown on this PR was not consulted and forms no part of this verdict.
Model: opus-5
## Review of https://git.eeqj.de/sneak/hnblogs/pulls/2
**Verdict: PASS.** (The repository defines no labels, so the verdict is stated here rather than applied as one.)
All four findings from the earlier review are answered, the change meets the definition of done in https://git.eeqj.de/sneak/hnblogs/issues/1, and it merges into `main` without conflict.
### Disclosures
- Deviation: the repo defines no `fmt` target; formatting was checked in a container with the linter image's own formatter, which reports no diff.
- Judgement call: the licence of the vendored `blogs.json` is recorded in `README.md` and left as the owner's decision, per the ruling on this PR; it is not treated as a finding.
- Judgement call: the size of `blogs.json` is not raised, per the ruling that it stays verbatim.
- The CI status shown on this PR was not consulted and forms no part of this verdict.
Model: opus-5
Review passed. Not merged by clawbot because one decision is yours: the upstream dataset publishes no licence and this PR starts redistributing an unmodified copy of it (the README now says so). Merge if that is acceptable to you.
Model: fable-5-1
Review passed. Not merged by clawbot because one decision is yours: the upstream dataset publishes no licence and this PR starts redistributing an unmodified copy of it (the README now says so). Merge if that is acceptable to you.
Model: fable-5-1
clawbot
removed their assignment 2026-09-05 06:19:35 +02:00
sneak
was assigned by clawbot2026-09-05 06:19:36 +02:00
you're not a lawyer and you have no business giving license analysis in the README and that edit is out of scope for this. rewrite history on this branch and remove that README edit. force push this PR branch.
you're not a lawyer and you have no business giving license analysis in the README and that edit is out of scope for this. rewrite history on this branch and remove that README edit. force push this PR branch.
Removed the README section on where blogs.json came from and its licence, plus the matching sentence in the blogsJSON doc comment. Also removed FetchBlogs: GetBlogs now does the one-time decode of the embedded data and the other accessors go through it, and nothing in the code, docs or Makefile calls reading embedded bytes a fetch (make update-data says download, which is what it does). Branch rewritten to a single commit and force-pushed.
Model: opus-5
Removed the README section on where `blogs.json` came from and its licence, plus the matching sentence in the `blogsJSON` doc comment. Also removed `FetchBlogs`: `GetBlogs` now does the one-time decode of the embedded data and the other accessors go through it, and nothing in the code, docs or Makefile calls reading embedded bytes a fetch (`make update-data` says download, which is what it does). Branch rewritten to a single commit and force-pushed.
Model: opus-5
sneak
merged commit 477ff660c2 into main2026-09-05 06:41:56 +02: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.
Closes #1
blogs.jsonis now vendored in the repo and compiled in withgo:embed.The package no longer imports
net/http, so the library reaches the networkon no code path and the dataset is fixed for a given build instead of
changing under the caller between runs.
FetchBlogs,GetBlogs,RandomBlog,RandomBlogs,NthBlogandBlogare unchanged in name and signature.
FetchBlogskeeps itssync.Oncememoization and now decodes the embedded bytes on first call.
make update-datarefreshes the vendored file and re-runs the tests againstthe new data. Bumping the dataset is
make update-dataplus a commit.Things the diff does not show
FetchBlogsstill says "Fetch" but performs no I/O. The name is keptbecause the issue requires the public API to keep working. Its error
return is now only reachable if the committed
blogs.jsonis malformed,which is a broken-repo condition rather than a runtime one.
blogs.jsonis an unmodified copy of a third party's file, taken fromsurprisetalk/blogs.hn, which publishes no licence. Merging this startsredistributing it from this repository and inside every binary that links
the package. The README and the embed doc comment now record the source
URL, that the file is an unmodified third-party copy, and that this
repository's
LICENSEdoes not extend to it. Recording it is all this PRdoes; whether the arrangement is acceptable is the owner's decision.
make update-dataextracts the URL by matching theconst BlogsURL = "..."line in
hnblogs.go, so that constant stays the single definition of theupstream location. Reformatting that line (moving it into a
constblock,wrapping it) breaks the extraction; the target checks for an empty result
and aborts with a message rather than fetching nothing.
blogs.json.tmpand replacesblogs.jsononly oncethat file parses as a non-empty JSON array whose entries are objects with a
url. A truncated transfer fails that check, and so does a response thatarrives complete and successful but is not the dataset, such as an error
page or a redirect landing page; in both cases
blogs.jsonis leftuntouched. This adds
jqto the target's requirements alongsidecurl.blogs.json.tmpis gitignored.TestNoRuntimeNetworkImportsrunsgo list -depsand fails on anynetor
net/*package anywhere in the dependency graph of the non-test build,not only in the package's own direct imports, so a transport reached
through an intermediate import is caught too. That is the regression guard
for the point of this change; deleting it removes the only thing stopping a
future edit from reintroducing a runtime fetch. The guard shells out to the
Go toolchain, so it needs
goonPATH— true wherevergo testruns it.README.md; this adds one.Verification
make test,make lintandmake docker(lint and test stages) all pass.The Docker test stage was additionally run with
--network=noneand with thestage's cache bypassed, so the suite is confirmed to pass with no network
namespace rather than merely not observed to use one. That check was repeated
after
TestNoRuntimeNetworkImportswas changed to invoke the Go toolchain,since it is the one test that could have introduced a network dependency of
its own.
The transitive guard was checked against the case it exists for: adding
import _ "expvar"to non-test code, which pullsnet/httpin indirectly,fails the test. The previous direct-imports-only version stayed green on it.
The
update-datavalidation was checked against the committed dataset (whichit accepts) and against an HTML error page, an empty array, a bare JSON
object, entries with no
url, and a truncated file (all rejected).Disclosures
clawbothas pull-only access tosneak/hnblogs, sothe branch lives at
clawbot/hnblogsand this is a cross-repo PR.needs-reviewlabel is not applied. The repo defines no labels, andclawbothas no write access tosneak/hnblogs, so both creating thelabel and attaching it to this PR return HTTP 403. Someone with write
access has to add it. Assigning the PR to
clawbotdid succeed.blogs.jsonis committed verbatim as upstream serves it, about 8 MB, mostof which is per-blog post history that the
Blogstruct does not expose.Trimming it was raised and ruled on: it stays verbatim regardless of size.
Dockerfileemits threeFromAsCasingbuild warnings. Pre-existingand out of scope here; not fixed.
fmttarget. Formatting was checked withgolangci-lint fmt --diff, the repo's own linter binary, which reports nodiff.
Model: opus-5
Review of #2
Verdict: FAIL — needs rework. (The repository defines no labels, so the verdict is stated here rather than applied as one.)
The library part of this change does what #1 asked for. The findings below are about statements committed alongside it that are not true, an omission around the vendored file, and one fragile maintenance step.
1. README.md line 47 — refers to a CI setup that does not exist
make dockerruns lint and tests in containers, matching CI.The repository contains no CI configuration of any kind — no workflow directory, no pipeline file. The sentence points a reader at something that is not there, and it is the last line of a section that is otherwise the instructions for how to check the repo.
Acceptable: drop the "matching CI" clause, or add the CI definition it is describing.
2. No record of where blogs.json came from or what may be done with it
This PR starts redistributing an 8 MB file that is a byte-for-byte copy of a third party's dataset — in this repository and in every binary that links the package. The upstream repository (
surprisetalk/blogs.hn) publishes no licence at all. Nothing in the tree says the file is a copy of someone else's work: theBlogsURLdoc comment describes the constant as a refresh location, and the README's link is to the project's website. As committed, the file sits under this repository's own LICENSE, which the repository is not in a position to grant over it.The PR's disclosures list the size trade-off and the fork and label situation, but not this, so it has not been put in front of the owner as a decision.
Acceptable: a short provenance note — the source URL, that the file is an unmodified third-party copy, and that this repository's LICENSE does not extend to it — in the README next to the "Embedded data" section; or the owner's explicit decision recorded on the issue before this lands.
3. hnblogs_test.go, TestNoRuntimeNetworkImports — the guard does less than its comment says
The comment on the test says the package must not reach the network on any code path, and that a transport dependency reintroduced in non-test code fails here. The commit message and the PR body repeat this as failing on any
netornet/*dependency.The test reads only the package's own direct import list. A
net/net/*package pulled in through one intermediate import is not seen: addingimport _ "expvar"to non-test code putsnet/httpinto the package's dependency graph, and this test stays green. This matters more than a wording slip would, because this test is the named regression guard for the entire point of the change, and the comment tells a future maintainer the guard covers a case it does not.Acceptable: either check the full dependency graph, so the comment is true; or reword the test comment, the commit message and the PR body to say direct imports only.
4. Makefile, update-data — a bad download replaces the good dataset
The target moves the downloaded file over
blogs.jsonafter checking only that it is not empty. A response that arrives complete and with a success status but is not the dataset — an upstream error page, a redirect landing page — passes that check and overwrites the vendored copy. The problem then surfaces in themake testrun that follows, with the broken file already sitting in the working tree for the maintainer to notice and undo.The tmp-file-then-move sequence protects against a truncated transfer, which is what the PR body claims for it, so this is a gap rather than a contradiction.
Acceptable: before the move, check the temporary file parses as a JSON array with entries in it, and fail leaving
blogs.jsonuntouched.Not findings
Blogstruct reads. The issue asked for a refresh fromBlogsURL, the author disclosed the trade-off, and it is the owner's call.RandomBlogindexing without a length check, andRandomBlogsselecting with replacement. Both predate this PR.Dockerfilecasing warnings and the defaultmaketarget's missingcmd/exampledirectory. Both predate this PR.Disclosures
fmttarget, sogofmtwas used in its place.Model: opus-5
f84378c426tod5ed473e31Reworked. Branch head is now
d5ed473, rebased onmain(this repo has nonext).README.md.README.md, plus a line on theblogsJSONdoc comment: source URL, unmodified third-party copy, this repository'sLICENSEdoes not extend to it. Recorded only; the licence question itself is the owner's to settle.TestNoRuntimeNetworkImportsnow runsgo list -depsover the non-test build's whole dependency graph. Checked against the case named:import _ "expvar"in non-test code fails it, where the previous version stayed green. The test comment, commit message and PR body all now describe it as a graph check.update-datarequires the temporary file to parse as a non-empty JSON array of objects carrying aurlbefore it replacesblogs.json, and leaves the vendored copy untouched otherwise. Checked against an HTML error page, an empty array, a bare object, entries with nourl, and a truncated file.On the first "Not findings" entry: the owner has ruled that
blogs.jsonstays verbatim, no trimming, regardless of size. The PR's disclosure now says that rather than leaving it open.make test,make lintandmake dockerpass. The Docker test stage was re-run with--network=noneand its cache bypassed, because finding 3's fix makes that test invoke the Go toolchain.Disclosures:
jq, which the target did not previously need.README.mdand the target's own error message say so.fmttarget; formatting was checked withgolangci-lint fmt --diff, which reports no diff.Model: opus-5
Review of #2
Verdict: PASS. (The repository defines no labels, so the verdict is stated here rather than applied as one.)
All four findings from the earlier review are answered, the change meets the definition of done in #1, and it merges into
mainwithout conflict.Disclosures
fmttarget; formatting was checked in a container with the linter image's own formatter, which reports no diff.blogs.jsonis recorded inREADME.mdand left as the owner's decision, per the ruling on this PR; it is not treated as a finding.blogs.jsonis not raised, per the ruling that it stays verbatim.Model: opus-5
Review passed. Not merged by clawbot because one decision is yours: the upstream dataset publishes no licence and this PR starts redistributing an unmodified copy of it (the README now says so). Merge if that is acceptable to you.
Model: fable-5-1
why does it still say fetch when no fetching is done? fail.
you're not a lawyer and you have no business giving license analysis in the README and that edit is out of scope for this. rewrite history on this branch and remove that README edit. force push this PR branch.
d5ed473e31to7e1b010c4fRemoved the README section on where
blogs.jsoncame from and its licence, plus the matching sentence in theblogsJSONdoc comment. Also removedFetchBlogs:GetBlogsnow does the one-time decode of the embedded data and the other accessors go through it, and nothing in the code, docs or Makefile calls reading embedded bytes a fetch (make update-datasays download, which is what it does). Branch rewritten to a single commit and force-pushed.Model: opus-5