Skip to content

ci(models): mirror pinned models to S3 and publish from CI (ATR-218) - #33

Merged
luanlorenzo merged 6 commits into
mainfrom
luan/atr-218-mirror-pinned-models-to-an-s3-bucket
Aug 17, 2026
Merged

ci(models): mirror pinned models to S3 and publish from CI (ATR-218)#33
luanlorenzo merged 6 commits into
mainfrom
luan/atr-218-mirror-pinned-models-to-an-s3-bucket

Conversation

@luanlorenzo

Copy link
Copy Markdown
Contributor

Mirrors the pinned NER models to an S3 bucket so a deployment does not depend
on reaching huggingface.co, and publishes them from a workflow rather than from
someone's laptop.

What lands

alcatraz models pins prints the pin table's entry for a model as JSON —
revision, origin, licence, and every file with its digest, size and the key the
downloader requests. -list prints every pinned model id, one per line, so a
loop does not have to carry its own idea of what is in the table.

hack/mirror-model.sh drives the aws CLI from that output. It downloads
through the normal path — so every file is verified against its pinned digest
on the way in, and nothing unverified can reach the bucket — uploads write-once,
then round-trips back through --origin to prove the mirror serves what the
downloader will accept.

Mirror model is the publish job. Manual: a model is mirrored when its pin
lands, a handful of times a year. Credentials are assumed via OIDC and last
minutes, so this repository — which is public — never holds a key with write
access to the bucket. The role's trust policy is scoped to the models-publish
environment rather than to the repo, so a run that skipped the reviewer gate
cannot assume it.

Mirror check is the half that earns its keep on an ordinary day. Pinning a
model nobody mirrored otherwise surfaces in a production image build, far from
the PR that caused it. It needs no credentials, since the mirror is served
publicly, and it compares content-length rather than just status, because a
half-uploaded object answers a HEAD perfectly well and only fails later, on its
digest.

Why there is no staging bucket

The artifact is content-addressed — the key carries an immutable commit sha and
every file is checked against a pinned digest on load, including cache hits. So
the bytes a mirror serves during review are the bytes production reads, or the
downloader rejects them. Promotion is the pin reaching main, and then a
version bump in whatever image consumes it. A second bucket would hold a second
copy of identical bytes and add a step that can fail.

For the same reason a mirror is a base-URL swap, not a second code path: an
origin is trusted for availability, never for content.

Deploying this

The workflows no-op until the infrastructure exists. Mirror check warns and
exits 0 while MODELS_ORIGIN is unset, which is deliberate — it must not fail
PRs against an empty bucket, including this one.

Bucket hoop-alcatraz-models (us-east-1, versioned, private) is served through
CloudFront with an OAC, and the publisher role is assumed via OIDC. The
models-publish environment carries MODELS_PUBLISH_ROLE as a secret — the arn
is the one value here holding the account id — plus MODELS_BUCKET,
MODELS_ORIGIN and MODELS_REGION as variables, so a failed run shows which
bucket and origin it used.

Order after merge: run Mirror model as a dry run, then for real, then add
MODELS_ORIGIN at the repository level to arm the PR gate.

Part of ATR-189. Closes ATR-218.

A mirror is a base-URL swap, not a new code path, but filling one still
needs the file list, digests and keys. "alcatraz models pins" emits them
as JSON so a publisher reads the same pin table the downloader verifies
against, instead of keeping a second list that drifts.

Each file carries a precomputed key, {model}/resolve/{revision}/{file},
so the layout lives in one place. PinnedFile gains Path alongside Name:
Name is what lands on disk, Path is what the fetch URL is built from,
and the two only agree because every model pinned today is flat.

hack/mirror-model.sh drives the aws CLI from that output. It verifies
locally before uploading, is write-once by default since a pinned
revision that changes bytes is a mistake, and round-trips through the
public origin at the end -- the upload arriving and the downloader
accepting it are different questions.

No credentials in the repo; the script uses the ambient AWS config.

Refs ATR-218.
The bucket is usually filled before anything serves it, so requiring the
round-trip verify up front blocks the upload it is supposed to follow.
Mirroring a model needed write access to the bucket, which meant a
long-lived key on someone's machine. This repository is public, so that
is the one credential shape worth avoiding: the publish job assumes a
role via OIDC instead, scoped in its trust policy to the models-publish
environment rather than to the repo, so a run that skipped the reviewer
gate cannot assume it.

There is no staging bucket, because there is nothing to stage. The
artifact is content-addressed — the key carries an immutable commit sha
and every file is checked against a pinned digest on load — so the bytes
a mirror serves during review are the bytes production reads, or the
downloader rejects them. Promotion is the pin reaching main, and then a
version bump in whatever image consumes it. A second bucket would hold a
second copy of identical bytes and add a step that can fail.

Mirror check is the half that earns its keep on an ordinary day: pinning
a model nobody mirrored otherwise surfaces in a production image build,
far from the PR that caused it. It needs no credentials, since the
mirror is served publicly, and it compares content-length rather than
just status, because a half-uploaded object answers a HEAD perfectly
well and only fails later, on its digest.

pins grew -list so that loop does not have to carry its own idea of
which models are in the table.
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Mirror pinned models to S3 and publish from GitHub Actions

✨ Enhancement ⚙️ Configuration changes 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Add alcatraz models pins JSON output for consuming the pinned model table.
• Add S3 mirroring script and GitHub Actions workflow to publish pinned model artifacts.
• Add PR-time mirror verification workflow plus tests/docs to prevent unmirrored pins.
Diagram

graph TD
  pr["Pull request"] --> check["Mirror check (GHA)"] --> cli(["alcatraz CLI"]) --> mirror{{"Public model origin"}}
  dispatch["Workflow dispatch"] --> publish["Mirror model (GHA)"] --> script[/"hack/mirror-model.sh"/] --> hub{{"HuggingFace hub"}} --> s3[("S3 bucket")] --> mirror
  subgraph Legend
    direction LR
    _wf["Workflow/job"] ~~~ _cli(["CLI"]) ~~~ _script[/"Script"/] ~~~ _db[("Storage")] ~~~ _ext{{"External"}}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Implement mirroring as a Go subcommand (AWS SDK)
  • ➕ Single self-contained tool (no bash/jq/aws-cli assumptions)
  • ➕ Richer error handling and structured logs
  • ➕ Easier portability to non-Linux environments
  • ➖ Adds AWS SDK dependency footprint and maintenance burden
  • ➖ More code to secure and review than a thin script wrapper
  • ➖ Harder to keep the CLI dependency graph minimal if not carefully isolated
2. Use `aws s3 sync`/artifact packaging instead of per-key uploads
  • ➕ Potentially simpler upload step and fewer API calls
  • ➕ Can be faster for bulk uploads
  • ➖ Harder to keep strict write-once semantics per pinned key
  • ➖ More room for accidental extra files or layout drift
  • ➖ Still needs the pin table to avoid mismatched content

Recommendation: Current approach is strong for correctness: it reuses the verified download path, uploads only the exact pinned keys, and can round-trip through the public origin to prove consumers will accept the mirror. Keep the bash script for now; consider a Go-based publisher only if the workflow grows beyond simple mirroring/verification or if jq/aws-cli availability becomes a recurring operational issue.

Files changed (10) +599 / -4

Enhancement (2) +104 / -3
models.goAdd 'models pins' subcommand emitting a stable JSON pin manifest +99/-2

Add 'models pins' subcommand emitting a stable JSON pin manifest

• Implements 'alcatraz models pins' and '-list', emitting a stable JSON schema decoupled from internal types. The output includes model metadata, license provenance, and per-file key/path/name/digest/size so external tooling can mirror without duplicating the pin table layout.

cmd/alcatraz/models.go

models.goExtend 'PinnedFile' with repository 'Path' for origin key construction +5/-1

Extend 'PinnedFile' with repository 'Path' for origin key construction

• Adds a 'Path' field to 'PinnedFile' so mirrors can upload to the exact paths the downloader requests, independent of flattened on-disk filenames. Updates 'PinnedFiles' to populate 'Path' from the pin table entry.

models/models.go

Tests (2) +117 / -1
models_test.goAdd tests for 'models pins' JSON schema and loopable '-list' output +74/-0

Add tests for 'models pins' JSON schema and loopable '-list' output

• Adds tests validating 'models pins' matches the pin table (including computed key format), rejects unpinned models without emitting output, and ensures 'pins -list' emits one id per line that can be round-tripped back into 'pins'.

cmd/alcatraz/models_test.go

models_test.goTest that 'PinnedFile.Path' matches requested hub-layout keys +43/-1

Test that 'PinnedFile.Path' matches requested hub-layout keys

• Adds a test fixture with a nested file path to ensure the downloader requests keys built from 'PinnedFile.Path' while still flattening to 'Name' on disk. Updates existing 'PinnedFiles' test expectations to include 'Path'.

models/models_test.go

Documentation (3) +82 / -0
doc.goDocument the new 'alcatraz models pins' command +14/-0

Document the new 'alcatraz models pins' command

• Extends CLI package docs with usage and behavior for 'models pins', including its JSON output contract and relationship to the mirroring script.

cmd/alcatraz/doc.go

cli.mdDocument 'models pins' flags and example JSON output +46/-0

Document 'models pins' flags and example JSON output

• Adds a CLI reference section describing 'models pins' flags, the JSON manifest shape, and how it powers mirroring. Also documents the two new workflows ('Mirror model' and 'Mirror check') and their purpose.

docs/cli.md

ner-offline.mdAdd guidance for using a custom model mirror via '--origin' +22/-0

Add guidance for using a custom model mirror via '--origin'

• Documents pointing 'alcatraz models download --origin' at an internal/public mirror with unchanged hub-style layout. Includes instructions for filling the mirror using 'hack/mirror-model.sh'.

docs/ner-offline.md

Other (3) +296 / -0
mirror-check.ymlAdd PR workflow to verify all pinned files are mirrored +69/-0

Add PR workflow to verify all pinned files are mirrored

• Introduces a pull_request-triggered workflow that builds 'alcatraz', enumerates pinned models, and HEAD-checks every pinned key against 'MODELS_ORIGIN'. It validates 'content-length' (not just 200 OK) and fails the PR if any pinned file is missing or truncated; it warns and exits 0 if the origin variable is unset.

.github/workflows/mirror-check.yml

mirror-model.ymlAdd manual publish workflow to mirror a pinned model into S3 +79/-0

Add manual publish workflow to mirror a pinned model into S3

• Adds a workflow_dispatch job that optionally assumes an AWS role via OIDC (models-publish environment) and runs 'hack/mirror-model.sh'. Supports dry-run listing, force overwrite, and optional origin round-trip verification; requires 'MODELS_BUCKET' to be set.

.github/workflows/mirror-model.yml

mirror-model.shAdd write-once S3 mirroring script driven by 'models pins' +148/-0

Add write-once S3 mirroring script driven by 'models pins'

• Introduces a bash script that builds 'alcatraz', reads the pin manifest, downloads and verifies the model locally, then uploads pinned files to S3 with immutable caching headers. By default it skips existing keys (write-once), supports '--force' and '--dry-run', and can round-trip verify by downloading back through a provided public origin.

hack/mirror-model.sh

The first real run caught it: an origin pasted with a trailing slash asks
S3 for a key that starts with one, and every file reports missing. The
downloader has trimmed these since it learned to take an --origin at
all, so an origin that works there has to work here — the check being
stricter than the thing it checks is the wrong way round.
@qodo-code-review

qodo-code-review Bot commented Aug 17, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Origin slash breaks check ✓ Resolved 🐞 Bug ≡ Correctness
Description
mirror-check.yml builds URLs as $ORIGIN/$key without trimming trailing slashes, so an origin
ending in / can produce a // path segment and request the wrong S3-style key. This can cause
Mirror check to fail PRs as “missing” even though the downloader normalizes origins specifically to
avoid this case.
Code

.github/workflows/mirror-check.yml[R54-55]

+              got=$(curl -fsSIL --max-time 30 "$ORIGIN/$key" \
+                | awk 'tolower($1) == "content-length:" { print $2 }' | tail -1 | tr -d '\r') || got=""
Relevance

●●● Strong

PR #30 accepted trailing-slash normalization for S3-style mirror URLs; this workflow repeats the
same unsafe concatenation.

PR-#30

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Mirror-check directly concatenates $ORIGIN/$key for its HEAD requests, but the downloader
explicitly trims all trailing slashes from origins to prevent S3-style 404s due to literal keys;
this mismatch can make CI check the wrong URL path.

.github/workflows/mirror-check.yml[34-61]
models/models.go[184-212]
PR-#30

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Mirror check` concatenates `$ORIGIN/$key` without normalizing `$ORIGIN`. If `MODELS_ORIGIN` ends with one or more `/`, the workflow can request a different path than the downloader would (double-slash path segment), leading to false “MISSING” failures on S3-style mirrors.

## Issue Context
The downloader already trims all trailing slashes because S3 treats keys literally. Mirror-check should apply the same normalization so it validates the mirror the same way `alcatraz models download --origin` will.

## Fix Focus Areas
- .github/workflows/mirror-check.yml[34-61]

## Suggested change
- After the empty check for `$ORIGIN`, normalize it by removing **all** trailing slashes (e.g., `ORIGIN="$(printf '%s' "$ORIGIN" | sed 's:/*$::')"`), then continue using `"$ORIGIN/$key"`.
- Add a short comment explaining this mirrors the downloader’s origin normalization to avoid S3 double-slash key mismatches.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. S3 key existence doesn’t fail ✗ Dismissed 📎 Requirement gap ⛨ Security
Description
hack/mirror-model.sh silently skips uploads when an S3 key already exists, and the workflow
exposes a force flag that enables overwriting. This violates the required write-once behavior and
can allow republishing the same revision without an explicit failure signal.
Code

hack/mirror-model.sh[R113-116]

+	if [ "$force" != true ] && aws s3api head-object --bucket "$bucket_name" --key "$s3_key" >/dev/null 2>&1; then
+		echo "  skip   $s3_key (already present)"
+		skipped=$((skipped + 1))
+		continue
Relevance

●●● Strong

The PR explicitly promises write-once uploads, while this code silently skips collisions and exposes
overwriting.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 4 requires failing (not skipping/overwriting) when an object key already exists.
The script currently head-object checks and then continues, and the workflow exposes force to
allow overwriting.

Enforce write-once behavior: do not overwrite existing S3 keys
hack/mirror-model.sh[113-117]
.github/workflows/mirror-model.yml[21-24]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The mirroring process must be write-once: if a target S3 key already exists for a pinned revision, the run should fail rather than skipping or overwriting.

## Issue Context
Compliance requires immutability per revision/key. Current behavior skips existing keys (successfully) and additionally provides a `--force` overwrite path.

## Fix Focus Areas
- hack/mirror-model.sh[24-30]
- hack/mirror-model.sh[113-117]
- .github/workflows/mirror-model.yml[21-24]
- .github/workflows/mirror-model.yml[63-79]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Round-trip verification is optional ✓ Resolved 📎 Requirement gap ⛨ Security
Description
hack/mirror-model.sh exits successfully without performing any post-upload refetch/verification
when --origin is not provided. This allows publishing to complete without the required end-to-end
digest validation from the mirror.
Code

hack/mirror-model.sh[R138-143]

+if [ -z "$origin" ]; then
+	echo
+	echo "no --origin given, skipping the round-trip verify. Once the bucket is"
+	echo "served, check it with:"
+	echo "  alcatraz models download --model $model --origin <base url> --dest /tmp/roundtrip"
+	exit 0
Relevance

●●● Strong

The PR explicitly promises round-trip verification, but missing origin currently makes publication
succeed without that security check.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 5 requires a refetch from the mirror after upload and digest verification for every
file, and that the publish operation only succeeds if verification passes. The script explicitly
skips the round-trip verification when origin is empty and exits 0.

Perform round-trip verification by refetching mirrored files and matching pinned digests
hack/mirror-model.sh[138-148]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The publish path must always perform round-trip verification by refetching mirrored files from the configured origin and validating them against pinned digests.

## Issue Context
Compliance requires post-upload refetch + digest validation for every file. The script currently skips this check entirely when `--origin` is omitted and still exits 0.

## Fix Focus Areas
- hack/mirror-model.sh[138-148]
- .github/workflows/mirror-model.yml[57-79]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

4. Pins docs contradict output ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The models pins docs claim the JSON includes origin and licence, but the example omits those
fields, implying a different schema than the CLI emits. The CLI docs also claim pins output is
always JSON even though -list intentionally outputs plain lines.
Code

docs/cli.md[R150-153]

+Prints the pin table's entry as JSON — revision, origin, licence, and every
+file with its digest, size and the key the downloader requests:
+
+```json
Relevance

●●● Strong

The documentation directly contradicts emitted JSON and -list behavior; recent CLI documentation
corrections were accepted.

PR-#31

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The implementation’s JSON manifest includes origin and license, and -list is implemented as a
line-oriented mode; the new docs omit fields in the example and state output is always JSON, which
conflicts with the code.

cmd/alcatraz/models.go[114-200]
docs/cli.md[143-176]
cmd/alcatraz/doc.go[192-203]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The newly added `models pins` documentation contradicts the implemented output contract:
- The narrative says `origin` and `license` are present, but the JSON example omits them.
- `cmd/alcatraz/doc.go` claims pins output is always JSON, but `models pins -list` outputs one model id per line.

These inconsistencies can mislead users writing scripts around the command.

## Issue Context
Implementation emits `origin` and `license` fields and supports `-list` as a non-JSON output mode.

## Fix Focus Areas
- docs/cli.md[143-176]
- cmd/alcatraz/doc.go[192-203]
- cmd/alcatraz/models.go[114-141]

## Suggested change
- Update the docs/cli.md JSON example to either include `origin` and `license` fields or explicitly note the example is abbreviated.
- Update cmd/alcatraz/doc.go to say: default `models pins` output is JSON; `-list` prints newline-delimited model ids (non-JSON).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context
✅ Cross-repo context — repo relationships

Grey Divider

Tip of the day
💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread hack/mirror-model.sh
Comment thread hack/mirror-model.sh
Comment thread .github/workflows/mirror-check.yml
Comment thread docs/cli.md
The prose promised revision, origin, licence and every file; the example
showed revision and files. Someone writing a mirror tool against the
example would not know origin and license are there to read.
The script leaves --origin optional so a bucket can be filled before
anything serves it, which is the bootstrap case and only happens once. A
publish from CI has no such excuse: without the round-trip the job
reports success on bytes nobody has proved the downloader can consume.

A dry run still runs without one — it uploads nothing to verify.
@luanlorenzo
luanlorenzo merged commit e171e68 into main Aug 17, 2026
12 of 13 checks passed
@luanlorenzo
luanlorenzo deleted the luan/atr-218-mirror-pinned-models-to-an-s3-bucket branch August 17, 2026 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants