Add the watcher and the stamper - #14
Conversation
Maximilian-Nesslauer
added
area:infra
There was a problem hiding this comment.
Thanks, this looks like good work.
I am requesting changes for five things, each with an inline comment:
- The write guard in
watch.pyis lexical and the listing id is never validated, so a crafted id defeats the "release files and nothing else" property. - A failed push and a dry run both persist the host ETags, so the next tick sees a 304 and the missed releases are never re-stamped.
- Two exception paths escape the HostError/StampError split: a malformed authored TOML aborts the whole tick, and an HTTPError from an asset download or a pagination request bypasses the reporting entirely.
- The stamper does not yet enforce the release-time rules of RFC 0035:
install.pathcontainment,targeton amod,targetrequired on a loader[install], and[provides].launchexisting in the archive. urljoinlets a SpaceDock-supplied value leave spacedock.info, so a stampeddownload.urlorchangelogcan name any host.
I would also like to shorten most of the comments and simplify them closer to ASD-STE100 Simplified Technical English. I think that some of the (specifically Opus 5 like) output can be a bit hard to understand.
FYI i did use the help of LLMs to help review this PR myself since it is 3000+ LOC, but read through every line myself, changed stuff where i did not agree, discussed etc.
On the decisions you asked for
StarMap cannot have its history stamped. Releases 0.4.0 to 0.4.5 each ship two archives,
StarMapLauncher-x.y.z.zipandStarMapStandalone-x.y.z.zip, 0.0.1 has no asset at all, and therctag does not parse. [...] The way out is either a lookback window on that listing or an asset selector in[releases], which is an RFC 0031 question.
Narrower than that, and I no longer think it needs an RFC.
The ambiguity is only 0.4.0 to 0.4.5. Everything from 0.0.2 to 0.3.6 stamps as it stands, because _asset filters to .zip first and that already resolves the .exe plus .zip pairs those releases ship to exactly one archive. 0.0.1 and rc are correctly rejected.
Taking the standalone for the remaining six does not work either, and I checked the archives rather than the names: 0.4.6 ships StarMap.exe plus StarMap.dll, while the 0.4.5 standalone ships StarMap.Loader.dll and no executable at all, and the launcher ships StarMap.Launcher.dll next to that loader. 0.4.6 is the two-process design collapsing into one binary, not the standalone continued, so "prefer standalone" would stamp archives that are not the thing the listing describes.
[provides].launch from point 4 settles it with no new field: the StarMap listing will say launch = "StarMap.exe", and an archive without it is the release-time rejection RFC 0035 already defines. Worth checking whether that also rejects 0.3.x before it is switched on.
So leave those six unstamped. If the history is ever wanted, the release pull request path (#6) is where a human stamps it deliberately, which the watcher could not do well anyway: stamping freezes the authored facts, and today's authored file is all a backfill has, so a backfilled 0.1.0 would claim game_min = 2026.8.3.5117.
The ETag invariant is "stamped or reported", not "stamped". If a release that can never be stamped, like that
rctag, kept the ETag suppressed, the listing would cost a full uncached release list on every tick forever.
Agreed, that is the right reading, and the reasoning in the one_listing comment is worth keeping exactly as written.
Swap detection is on the asset size and URL. A re-uploaded archive that happens to keep its byte count is not detectable without re-downloading every stamped archive on every tick, which is the whole tick's budget for an answer
download.sha256already gives the client.
Agreed as a trade-off. One addition: SpaceDockHost never sets size, so on a SpaceDock authority the check degrades to the URL alone and a re-upload is never detected. Worth a sentence in the check_for_a_swap docstring.
The sweep depends on a contract that does not exist yet. It needs KSAModding/content-index#4's validation workflow to accept a dispatch carrying the pull request number and to report under a check named
validate.
Fine to depend on it, content-index#3 and #4 will implement both. One consequence to handle first, in the inline comment on _dispatch: until that lands every dispatch is refused, and a refusal currently retires that head commit permanently, so the first sweep would park every open pull request.
Two smaller calls I made: a listing marked
delistedinindex-status.tomlis skipped [...] And theinstallobject is only stamped when it has something to say.
Both right. The delisting skip is the correct reading of whose voice the index status is, and the conditional install object is what makes the StarMap example reproduce byte for byte under RFC 0035's per-type defaults.
One piece of #4 I left out on purpose. The open month re-resolution from RFC 0033. It cannot be done from the repository alone [...] It needs either a marker in the release file or the derived cache, and that felt like your call rather than mine.
This is the one I disagree with: I think the repository is enough, see the inline comment on resolve_bound. If you would rather keep it out of this PR that is fine, but then the docstring has to stop promising it and I will file the follow-up.
The rest are inline too, marked non-blocking, one per site so each can be answered and resolved on its own.
| cannot: release files and nothing else. | ||
| """ | ||
| path = Path(path) | ||
| if self.releases_root not in path.parents: |
There was a problem hiding this comment.
Blocking. The containment check is lexical.
Path.parents never normalizes, so for a listing id like ../x the path releases/../x/1.0.0.json still carries releases in its parents and the guard passes, while the file lands outside releases/.
The id comes straight from the authored TOML with no validation, and this job is the one actor with a branch-protection bypass on main, so this guard is the enforcement the PR body claims it is.
Two changes close it:
- Resolve before comparing:
path.resolve().is_relative_to(self.releases_root.resolve()). - Validate the listing id against the RFC 0031 id rules before using it as a path segment, and report a listing whose id fails them instead of processing it. That also covers
folder()andstamped_versions(), which build paths from the same id.
| exit 1 | ||
|
|
||
| - name: Save the derived cache | ||
| if: always() |
There was a problem hiding this comment.
Blocking. if: always() persists the ETags of a tick whose push failed.
The commits die with the runner, the ETag survives, the next tick gets a 304 and logs "unchanged since the last tick", and the missed releases are not re-stamped until the host's payload happens to change.
That contradicts the push step's own "the next tick rescans and re-stamps" message.
A dry run has the same shape: the Push step is skipped, but the Python side already stored the ETags and the mirror and swap timestamps, and this step uploads them, so a dry run suppresses the next real tick's scan.
Fix: gate this step on success() and on the dry run being off, and ideally watch.py should also skip storing host ETags when --dry-run is set, so the invariant "the ETag stands for stamped or reported" survives local runs too.
Related, one step up: the Push step's explicit dry-run condition drops the implicit success(), so it runs even after a failed tick.
| def tick(self): | ||
| for path in self.listings(): | ||
| with path.open("rb") as handle: | ||
| authored = tomllib.load(handle) |
There was a problem hiding this comment.
Blocking. tomllib.load sits outside the per-listing try, so one malformed authored document aborts the whole tick with a traceback: no other listing is scanned, no issue is opened, and cache.save() is skipped.
That is exactly the property the # noqa comment two lines further down promises ("one listing never fails the tick").
Move the parse inside the guarded block and report a parse failure through issues.report like any other listing error.
Three things to watch while moving it:
delisted()has the same unguarded parse onindex-status.toml, and failing open there would stamp releases a steward delisted.- A
try/finallyaround the listing loop that always callscache.save()would keep the ETags a partial tick already learned. - Once
reportis called from inside anexceptclause, it must not raise: an exception raised in one except clause is not routed to the siblingexcept Exceptionof the same try, so it would leave the loop anyway. Wrapping the API-shaped failures insideIssues.reportcovers it.
| "should verify against" | ||
| ) | ||
| raise StampError("the release carries no archive to download") | ||
| answer = http.get(release.url, api=release.url.startswith(GITHUB_API)) |
There was a problem hiding this comment.
Blocking. Http.get re-raises the raw HTTPError for codes below 500, and only the first page request in releases() is wrapped (line 233).
A 404 on a deleted asset, or on a Link next page, therefore unwinds past stamp_pass and one_listing, which catch only HostError and StampError, into tick()'s broad catch.
The result is a log line unexpected: HTTPError(...) with no URL and no version, no author-facing issue, and no stored ETag, so a permanently deleted asset costs a full uncached scan plus a useless log line every tick, forever.
Apply the same translation the first page already has, at both sites: 404, 410 and 451 become StampError naming the URL, everything else becomes HostError.
The mirror path through mirrors_for shares _download, so fixing it there covers both.
Two neighbours in the same class, worth doing in one pass:
_readraisesHostErrorfor an oversized response, but oversize is permanent, so the watcher downloads and discards 512 MiB every tick and no issue ever opens.GitHubHostalready has the assetsizefrom the release list, so the GitHub case can be rejected before any bytes move.GitHubHost.releasesparses its payloads with a barejson.loadswhileSpaceDockHost.releaseswraps its parse. A 200 carrying proxy HTML is neitherHostErrornorStampErrorand lands in the broad catch.
| ) | ||
|
|
||
|
|
||
| def install_object(handle, listing_id, content_type, authored_install): |
There was a problem hiding this comment.
Blocking. Four release-time rules of RFC 0035 are not enforced here, and since this module is the one place a release file is derived, the release pull request checks will inherit whatever it accepts:
install.pathis copied through unvalidated. Rule 1 makes an absolute path, a~path, or a path whose normalized form escapes its anchor invalid, and the stamped file carries the resolved path to every client.targetis accepted ontype = "mod". RFC 0035 states a mod's install location is not the author's to choose; a stampedtarget = "game-root"on a mod would direct every client to write into the game folder. Reject it, or if you read the RFC differently, let us settle that reading in the design repo first.- A
mod-loaderwith an[install]section but notargetis accepted. Per RFC 0035,targetis required when the section is present and the type has no default. [provides].launchis never checked against the archive. RFC 0035's errors table makes "launch naming a file absent from the release" a release-time rejection reported to the author, which is exactly this module's job, not the authored repo's schema check. Rule 4 also makestarget = "standalone"without alaunchinvalid.
The first and last are the important ones; the middle two could also live in content-index#2's schema, but until that exists the stamper is the only gate.
Separately, while you are in stamp(): the authored spec_version is never read, so a document claiming a future format is silently stamped as 1, and there is no check that game_min_revision <= game_max_revision, so an authored game_max month below the min stamps an empty compatibility range.
| return stamped | ||
|
|
||
|
|
||
| def merge_dependencies(derived, authored): |
There was a problem hiding this comment.
Two authored entries naming the same id both survive the merge, because by_id is built from the derived list only.
Whichever the resolver reads last wins, and the release file carries a contradiction no client can resolve.
Rejecting a duplicate id here makes it the author's error at publish time, which is where every other malformed-authored-document error already lands.
| parser.add_argument("--out", type=Path, help="write here instead of to stdout") | ||
| arguments = parser.parse_args(argv) | ||
|
|
||
| with arguments.listing.open("rb") as handle: |
There was a problem hiding this comment.
The three input reads sit outside the except StampError, so a malformed release JSON, an unreadable listing, or a missing archive gives a traceback instead of the cannot stamp ... message this function promises.
That matters beyond the CLI: content-index-releases#6 has the release checks re-deriving with this module, so the same traceback would be what a contributor sees in a failed check.
| payloads = [json.loads(first.body)] | ||
| following = LINK_NEXT.search(first.headers.get("Link", "") or "") | ||
| pages = 1 | ||
| while following and pages < self.max_pages: |
There was a problem hiding this comment.
Two limits of the scan that are fine as limits but not fine silent.
max_pages=5 caps a listing at 500 releases with no signal crossing the boundary, so the tick settles and stores the ETag as if it had seen everything.
And the stored ETag covers page 1 only, so a change confined to a later page never invalidates it, which quietly weakens the "a tag that reappeared with different bytes is reported" property for a listing that large.
Reporting the truncation as a listing error is the useful form: refusing to settle would only force a full refetch each tick without ever reaching the tail, since GitHub lists newest first.
|
|
||
| document = stamp( | ||
| authored, facts, archive, game_versions, | ||
| mirrors=mirrors, now=datetime.now(timezone.utc), |
There was a problem hiding this comment.
_restamp re-derives with wall-clock now, while stamp() documents that the release checks pass the submission's own time so a re-derivation reaches the same answer as the stamp it is checking.
I think wall clock is actually right here, but for a reason the file does not state: the original stamp time is unrecorded, so neither wall clock nor release_date is always correct for a month game_max, and once the month pass exists the wall-clock answer is the corrected state an example should hold.
Latent either way, since no current example uses a month bound. Worth a comment rather than a change.
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v7 | ||
| # examples/ in the design repository was stamped by hand from the same |
There was a problem hiding this comment.
Three lines of prose for one step, and the last clause ("the one check of this tooling that nobody here wrote") is about how the check came to be rather than what it does.
Our convention is that comments describe the code as it is now, not its development history, so this is the shape I would trim throughout the PR rather than only here. Two other instances: class Sweep's docstring cites "the 2026-08-06 incident", and watch.py opens with a 32-line property table that the code below it already states.
Nothing here is wrong, and the reasoning is genuinely good. It just belongs in the pull request body and the RFC, which already carry it, rather than in the file where it has to be maintained alongside the code.
Authored document plus release archive in, release file out, per RFC 0031. The one place a release file is derived: the watcher stamps with it, and the release pull request checks re-derive with it and compare against what was submitted, so the two paths cannot disagree. It needs no token and touches no network, so it is testable offline against examples/ in the design repository, where every value was produced by this procedure by hand.
A scheduled tick that asks every listing's authority host for its releases and stamps every release with no file under releases/<id>/ yet, commits it, appends a mirror that only appeared later, keeps one error issue per listing current on the authored repository, and sweeps that repository's open pull requests. The tick holds no state. What is stamped in this repository is the state, so a run GitHub delays, drops or cancels costs latency and not data, and a re-run stamps nothing twice. The per-listing ETag store is derived cache written outside the repository, and an ETag is only kept once every release behind it is stamped or reported, so a stale cache can never hide a release.
CLAUDE.md rules them out and tools/update_game_versions.py already goes without them. Prose only, no behavior change.
The stamper is the one place a release file is derived, and the release pull request checks re-derive with it, so what it accepts is what the index accepts. - paths in [install] and [provides] must be relative and stay inside their anchor, and the stamped value is the normalized form - a mod cannot author target or path: the folder name is the identity the game sees, so the location is a default, not a field - a mod-loader [install] needs a target, and standalone needs a launch - [provides].launch has to exist in this release's archive - an authored spec_version other than 1 is refused rather than stamped - game_max below game_min is an empty range and is refused - a duplicate authored dependency id is refused - main() reads all its inputs inside the guard, so a missing archive reports instead of raising
An exception that is neither lands in the tick's broad catch, which logs "unexpected", opens no issue for the author and stores no ETag, so a permanent failure costs a full uncached scan every tick forever. - an asset download translates 404, 410 and 451 to StampError and the rest to HostError, the way the first release-list page already did - a failure on a later page is a HostError instead of a raw HTTPError - a release list that is not JSON is a HostError, as SpaceDock's already was, so a 200 carrying proxy HTML does not escape - an oversized archive is a StampError, and known-oversized from the release list is rejected before any bytes move - a non-numeric SpaceDock id is a StampError, not a ValueError - an unparseable timestamp yields None rather than being stamped raw - a scan that hits max_pages reports the truncation instead of settling silently on a partial list A SpaceDock-supplied path can also leave spacedock.info: urljoin returns an absolute or protocol-relative value unchanged, so download.url, download.mirrors and changelog could name any host. Resolved values are now rejected unless the scheme and host still match the base.
Each of these is a property the tick already claims and did not keep once something went wrong. Release files and nothing else: the guard compared with Path.parents, which never normalizes, so a listing id carrying ".." kept "releases" in the parents while the file landed outside it. Both sides are resolved now, and an id that breaks the RFC 0031 rules, or a file name that does not match the id it declares, is reported instead of becoming a path. One listing never fails the tick: the authored TOML was parsed outside the per-listing guard, so one broken document aborted the whole tick. Reporting can no longer raise either, which matters because it is called from inside an except clause, where a raise leaves the loop. An unreadable index-status.toml now fails closed, and the cache is saved in a finally. The ETag stands for stamped or reported: it was keyed per host, so two listings on one repository blinded each other, and a lookback tick stored it as if the skipped releases were done. A dry run stored it too. One open issue per listing: a degraded issue list no longer creates a duplicate, the label fallback is narrowed to the 422 it was written for, and the issue reads are paginated. A host that stays unreachable is now reported on every tick past the threshold rather than exactly once, and a recovery closes only the issue it opened. Also: the same bytes at a new URL become a mirror rather than a log line, since download.url is immutable; a refused sweep dispatch is retried on a clock instead of retiring the commit; mirrors_for shares the mirror budget; Api.send retries a 5xx and a secondary rate limit; a corrupt stamped file is reported; and the open game_max month is resolved once the month completes, which the stamper's docstring already promised.
The cache step ran on always(), so a tick whose push failed kept its ETags while its commits died with the runner: the next tick took the 304 path over releases it never stamped, which is the opposite of what the push step's own message promises. A dry run stored them too. Both steps now require success() and a real run, and watch.py skips the save entirely under --dry-run. The README gains the month re-resolution, and verify_examples says why it re-derives with the wall clock.
A delisted listing, a deleted document, and a listing whose [releases] section is gone never reach the code that closes an issue, so the issue outlived the listing it was about. The releases-section case closes where the tick notices it. The other two are swept at the end of a tick, guarded three ways because closing is the destructive direction: a narrow dispatch knows nothing about the listings it skipped, an empty listing set is how an unreadable index-status.toml looks, and a failed issue read makes every id look orphaned.
The comment on the examples job, which the review pointed at, was still three lines of development history. It now says what the step does. The same pass over the longest blocks left: the workflow's schedule, cache and argument comments, and the docstring of close_orphans. The ETag comment in one_listing stays as it is, by request.
|
@Maximilian-Nesslauer I left three blocks alone on purpose: the ETag comment in |
2 participants
Closes #4, closes #13.
Two things, because the loop has nothing to call without the derivation:
tools/stamp_release.pyis #13, and everything else is #4.What is here
tools/stamp_release.pytools/hosts.pytools/watch.pytools/verify_examples.pyexamples/from their release hosts and diffs..github/workflows/watcher.yml.github/workflows/checks.ymlEach property from #4 has a place in the code rather than a comment claiming it:
stamped_versions()reads what is stamped off disk, and there is no queue anywherecheck_for_a_swap()never overwrites, and reports the version with both hashesdownload.mirrorsis append only, and only after the other host's archive downloads byte identicalreleases/What I checked
verify_examples.pyre-derives all eight hand-stamped examples from their live release hosts and they come out identical, byte for byte, through the host adapters. With--check-mirrorsit also re-downloads every SpaceDock mirror and confirms the bytes match, so the mirror machinery is verified against the same claim the examples README makes.On top of that, 54 offline tests, and sandbox ticks against the real hosts:
content-indexas it stands today is a clean no-op: no listings yet, no open pull requestsOne piece of #4 I left out on purpose
The open month re-resolution from RFC 0033. It cannot be done from the repository alone: a stamped file with no
game_maxlooks exactly the same whether it was stamped open because the month was not over or the author never set one. Resolving it in the second case would apply an authored change to old stamps, which is the amendment path and not the watcher's. It needs either a marker in the release file or the derived cache, and that felt like your call rather than mine. Everything else about month bounds is in, including stamping an unfinished month as open.Things that want a decision
StarMap cannot have its history stamped. Releases 0.4.0 to 0.4.5 each ship two archives,
StarMapLauncher-x.y.z.zipandStarMapStandalone-x.y.z.zip, 0.0.1 has no asset at all, and therctag does not parse. The watcher reports all of that instead of guessing, because picking the wrong asset stamps a hash clients then verify against the wrong file. The way out is either a lookback window on that listing or an asset selector in[releases], which is an RFC 0031 question. 0.4.6 has exactly one archive and stamps fine.The ETag invariant is "stamped or reported", not "stamped". If a release that can never be stamped, like that
rctag, kept the ETag suppressed, the listing would cost a full uncached release list on every tick forever. A payload that changes changes the ETag, so a renamed tag is still picked up on the next tick.Swap detection is on the asset size and URL. A re-uploaded archive that happens to keep its byte count is not detectable without re-downloading every stamped archive on every tick, which is the whole tick's budget for an answer
download.sha256already gives the client. Stated in the code rather than pretended away.The sweep depends on a contract that does not exist yet. It needs KSAModding/content-index#4's validation workflow to accept a dispatch carrying the pull request number and to report under a check named
validate. Both are passed inwatcher.ymlso they can be changed without touching Python, and a dispatch the workflow refuses is logged once instead of failing the tick.Two smaller calls I made: a listing marked
delistedinindex-status.tomlis skipped, since stamping releases for something that is out of the snapshot would be the watcher arguing with a steward. And theinstallobject is only stamped when it has something to say, which is why the StarMap example reproduces exactly as it stands, with no object at all. The moment that listing authors[install] target = "standalone", new stamps carry it. If you read RFC 0035 as wanting the object on a loader either way, that is a one line change.