Skip to content

ci: add the ingestion image build+import gate (PR A0 of 3, registers #52, #53) - #71

Merged
tornidomaroc-web merged 2 commits into
mainfrom
chore/b1a0-ingestion-image-ci
Jul 26, 2026
Merged

ci: add the ingestion image build+import gate (PR A0 of 3, registers #52, #53)#71
tornidomaroc-web merged 2 commits into
mainfrom
chore/b1a0-ingestion-image-ci

Conversation

@tornidomaroc-web

@tornidomaroc-web tornidomaroc-web commented Jul 26, 2026

Copy link
Copy Markdown
Owner

PR A0 of the three-PR (b1) sequence. CI-ONLY. Zero files under src/, zero files under services/. No service code, no migration, no DB change, no dashboard change.

Adds one workflow, .github/workflows/ingestion-image.yml, plus this repo's required PROGRESS.md records.

This PR's first version shipped the exact defect it exists to end

Kept in the history (commit 64e4e80, fixed in ac14b46) rather than force-pushed away. See "The vacuous-green defect" below. If you read only one section, read that one.

What this PR proves

  • The ingestion image builds, using Railway's own build context — Root Directory /services/ingestion, empty Dockerfile Path, read from the dashboard on 2026-07-26 rather than assumed.
  • main.py imports inside that built image, with the image's own interpreter at its own WORKDIR, and the job asserts on the printed output that it did. services/ingestion/main.py:13 imports acreate_client/AsyncClient/AsyncClientOptions from supabase at module scope, and :50 calls tiktoken.get_encoding at module scope, so a bad resolve is a boot-time crash, not a runtime one.
  • The container boots and serves /health — strictly stronger than an import, because it catches ASGI/lifespan startup failures a bare import does not.

All four endpoints (/health, /embed, /convert, /ingest) share one uvicorn process, so a boot crash takes the Ask path down with the upload path. That is the failure this closes.

What this PR does NOT prove

It does not prove the Railway environment is correct. CI has no SUPABASE_URL or SUPABASE_ANON_KEY, so the CI container's /health correctly reports "supabase_configured": false. The job asserts only "ok":true.

That separation is load-bearing and must not be collapsed later: this job is the CI twin of the live boot probe. The environment probe has no CI twin and cannot have one — only a live GET /health against the deployed service can assert supabase_configured: true.

It also proves nothing about ingestion behaviour. It never runs an upload, never touches Supabase, never embeds anything.

The vacuous-green defect

The first version of this workflow (64e4e80) reported the ingestion-image job green in 38 seconds with its import step emitting zero bytes of stdout — no ROUTES:, no IMPORT OK, no IMPORT FAILED. Run 30218194135.

Cause, proven by direct experiment rather than inferred. python - reads its program from stdin, and docker run without -i does not attach stdin to the container. Python read EOF, executed an empty program, exited 0, and the step passed having asserted nothing:

=== docker run WITHOUT -i (what the workflow did) ===
exit=0  <-- the script never ran

=== docker run WITH -i ===
STDIN REACHED THE CONTAINER
exit=3  <-- the script ran

A workflow written to end vacuous green checks shipped a vacuous green check.

Severity stated accurately, not dramatically. The job was not proving nothing about the import — the boot probe transitively proves the module imports, since uvicorn cannot serve /health without importing main:app. What the dead step actually cost was the route assertions, the ROUTES: evidence line, and the network-vs-ImportError diagnostic banner.

Two fixes (ac14b46), and the second is the one that matters:

  1. Add -i, so the heredoc reaches the interpreter.
  2. Pipe through tee and make a trailing grep -q "IMPORT OK" the thing that actually fails the step. GitHub runs steps with bash -e and NOT -o pipefail (shell: /usr/bin/bash -e {0}), so tee would otherwise mask a non-zero docker run.

Both paths were tested locally under bash -e before pushing — with -i the step exits 0 and prints IMPORT OK; with -i removed the step now exits 1, so the regression cannot silently return. A comment at the step records why -i is load-bearing so a future reader cannot innocently remove it.

The generalised rule: a check step must assert on its own OUTPUT, not merely on its exit code. Register #52 now carries it.

Audit of the other four steps under the same lens, so the fix is not mistaken for a sweep:

Step Exposed?
Check out the repository No — self-verifying downstream; a bad checkout fails the build on a missing Dockerfile.
Build the ingestion image No — its exit code and its effect are the same fact. COPY requirements.txt / COPY main.py make a wrong context or missing file fail loudly. Nothing further to assert on.
Import the module inside the image YES — the defect. Fixed.
Boot the container and probe /health No — already asserts on payload (grep -q '"ok":true'), in an if body where bash -e applies.
Container logs (always) No — || true by design, claims nothing.

The import step was the only exposed one. No other fix was invented.

Runtime: measured, and my estimate was wrong by an order of magnitude

Measured: 38 seconds (build ~24 s). Projected before the run: 5-9 minutes. The projection is recorded rather than deleted so the next estimate carries a known error bar. It was wrong because every dependency in markitdown[all] resolves to a prebuilt manylinux wheel and GitHub's path to PyPI is very fast — nothing compiles from source.

Two riders on that number:

  • It was measured against main's requirements.txt, which does not yet contain supabase==2.29.0. PR A's run will be slower — though on this evidence nowhere near the original projection.
  • It was measured on the defective run, whose import step was a no-op. The corrected step adds a real container start, so the true figure is slightly higher again.

timeout-minutes: 20 now carries far more headroom than intended. Left as-is: a generous ceiling costs nothing, and a hung third-party pull still fails loudly instead of burning the 6-hour default.

V11 is satisfied only by a green run on main of the CORRECTED workflow. The 38 s figure came from a run whose central assertion did not execute; it pins the runtime and nothing else.

Deliberately NOT a required check

This is not added to branch protection, and that is intentional.

Three third-party network dependencies sit in this job's hot path — Docker Hub (python:3.11-slim), PyPI (markitdown[all] + the supabase tree), and Azure blob storage (tiktoken's cl100k_base, see #53). db-types now has one, digest-pinned.

Register #43 is precisely this class. It redded main twice on docker: toomanyrequests with no commit to this repo, and its residual (1) records that Docker Hub is also anonymous-rate-limited per IP with GitHub runners sharing NAT pools. #43's governing quotes apply unchanged:

"A gate that reds main on a coin flip is worse than no gate."

"a required check that reds on a third party's traffic trains people to re-run it."

Promotion rule, binding, recorded in register #52: required-check status is considered only after 20 consecutive green runs on main with zero re-runs. Any red caused by a third party's network resets the counter to zero, and weakens rather than strengthens the case. Never being promoted is an acceptable outcome. Mechanically it also cannot be required until a green run on main exists (typecheck.yml:11-12).

Register rows: why two new ones, and why #51 is skipped

Other design notes

  • The base image is not digest-pinned in the workflow, deliberately. services/ingestion/Dockerfile says FROM python:3.11-slim. Pinning in CI but not in the Dockerfile would make CI green on an image production never builds — a silent false-green, worse than the flakiness it avoids. If the base should be pinned, pin it in the Dockerfile so both paths move together.
  • No paths: filter, deliberately. A path-filtered job never reports a status on PRs it skips; if this is ever promoted to required, a rule demanding a context nothing publishes blocks every unrelated PR indefinitely.
  • Needs no secret. No registry credential, no Supabase key, not INGESTION_TOKEN. permissions: contents: read is the entire grant; the job cannot reach, read, or mutate production.
  • Costs nothing. This repository is public, and GitHub bills Actions minutes only for private repositories.

Known forward reference

The workflow's comment header cites docs/b1-verification-protocol.md, which does not exist yet — PR A creates it. Accurate for the sequence, but a dangling path for the A0-to-A window. Flagged rather than silently left.

Logged for later, deliberately out of scope here

actions/checkout's post-job cleanup emits fatal: No url found for submodule path 'profile-readme' in .gitmodules and a git failed with exit code 128 warning. Non-fatal, does not affect any result, and near-certainly pre-existing. Not investigated in this PR, including whether tsc and db-types show it too.

Checks

tsc and db-types will pass — vacuously, since this PR contains zero TypeScript and zero SQL. That is the entire point of register #52. The only meaningful signal here is ingestion-image, and it is meaningful only now that its import step actually executes.

Deliberately not in this PR

docs/b1-verification-protocol.md (PR A) · any change to services/ingestion/ · any change to src/ · any branch-protection change · any dashboard change · closing PR #70.

🤖 Generated with Claude Code

PR A0 of the three-PR (b1) sequence. CI-ONLY: zero files under src/,
zero files under services/.

Adds .github/workflows/ingestion-image.yml, which builds services/ingestion
through Railway's own build context (Root Directory /services/ingestion,
empty Dockerfile Path), then imports the module and boots the container
inside the built image.

Closes the gap that tsc and db-types inspect no Python at all, so a
service-only PR passes both vacuously while Railway's "Wait for CI" is OFF.

Opens register #52 (the vacuous-green gap + the 20-green-run promotion
rule) and register #53 (the tiktoken cold-start network dependency the
workflow surfaced, pre-existing on main, not a (b1) regression).

Deliberately NOT added to branch protection. #51 is skipped and reserved
for (b1)'s own row, which PR A adds.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented Jul 26, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
knowflow Ready Ready Preview, Comment Jul 26, 2026 8:29pm

… output

The first run of this workflow (PR #71, run 30218194135) reported the
ingestion-image job GREEN in 38 seconds with its import step emitting zero
bytes of stdout: no ROUTES line, no IMPORT OK, no IMPORT FAILED.

Cause, proven by direct experiment: `python -` reads its program from stdin,
and `docker run` WITHOUT `-i` does not attach stdin to the container. Python
read EOF, executed an empty program, exited 0, and the step passed having
asserted nothing. A workflow written to end vacuous green checks shipped a
vacuous green check.

Severity stated accurately: the job was not proving nothing about the import.
The boot probe transitively proves the module imports, since uvicorn cannot
serve /health without importing main:app. What the dead step cost was the
route assertions, the ROUTES evidence line, and the network-vs-ImportError
diagnostic banner.

Two fixes, the second being the one that matters:

1. Add -i, so the heredoc reaches the interpreter.
2. Pipe through tee and make a trailing `grep -q "IMPORT OK"` the thing that
   actually fails the step. GitHub runs steps with `bash -e` and NOT
   `-o pipefail`, so tee would otherwise mask a non-zero docker run.

Both paths tested locally under bash -e before pushing: with -i the step
exits 0 and prints IMPORT OK; with -i removed it now exits 1, so the
regression cannot silently return.

Audited the other four steps under the same lens. The build step is not
exposed (its exit code and its effect are the same fact). The boot probe
already asserts on its payload, in an if-body where bash -e applies.
Checkout is self-verifying downstream. The logs step is `|| true` by design.
The import step was the only exposed one; no other fix was invented.

Recorded in the PR A0 section 7 block and in register #52, whose thesis this
is now the first instance of. Kept as a separate commit rather than an amend
so the defect stays in the PR's own history.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tornidomaroc-web
tornidomaroc-web merged commit eaad752 into main Jul 26, 2026
5 checks passed
@tornidomaroc-web
tornidomaroc-web deleted the chore/b1a0-ingestion-image-ci branch July 26, 2026 20:56
tornidomaroc-web added a commit that referenced this pull request Jul 28, 2026
The row this PR added at docs/PROGRESS.md:150 was a SINGLE-CELL row in a
four-column table: 2 pipes, where the header (| # | Item | Why deferred |
Where addressed |) and every other row from #1 to #53 have 5 or more. Its
Item, Why deferred and Where addressed cells were empty and its entire body
rendered inside the narrow # column.

Repaired here rather than in a later PR for one reason: this PR introduced the
defect and this PR is still open, so fixing it here means the malformed row
never reaches main at all.

Separate commit, no amend and no force-push, so the defect stays visible in
this PR's history, the same way the vacuous-green fix was handled on #71.

ONE FACTUAL CORRECTION rides along, disclosed rather than folded in silently.
The old status narrative opened "PR A of 3 landed 2026-07-26" and marked
"A = ... (landed)". PR A has not landed: gh pr view 72 reports state OPEN with
mergedAt null and no merge commit, and git branch --contains c27163a lists only
the two feature branches, not main. PROGRESS.md:9 already contradicted it,
correctly describing the same work as the active branch. A §4 row asserting a
merge that did not happen is the false-check class registers #39 and #23 exist
to police, so the status cell now reads "opened 2026-07-26; PR A NOT YET MERGED
(PR #72 open as of 2026-07-28)" and Where addressed records A as PR #72, OPEN,
not merged.

Content otherwise preserved and redistributed as the neighbouring rows do:
status in #, the substance in Item, the three-PR rationale in Why deferred, and
the sequence with its gates, closing rule and related registers in Where
addressed.

One line replaced by one line. CRLF preserved. §7 untouched: zero additions and
zero deletions inside it. No change to services/, none to src/, no migration,
no DB change, no branch-protection change, no Railway or Vercel dashboard
change; PR #73 and PR #70 untouched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tornidomaroc-web added a commit that referenced this pull request Aug 2, 2026
**This is the instrument that would have caught the 2026-07-23 → 2026-08-01 outage on day one instead of day nine.** PR #74 (register #54) made ingestion failures *legible* at all four layers. Nothing yet *looks*. This looks, every 15 minutes, with no credential.

## Why this is a peer of the section-10 table, not a component of it

`docs/b1-verification-protocol.md` §10 records each verification row against the Railway deployment that produced it, because — quoting the file — *"without it a green row cannot be tied to the artifact that produced it after any rollback."*

**Railway marks deployments REMOVED on this plan, for the entire deployment history** (confirmed 2026-08-02 from the dashboard, not inferred). So every §10 row is an observation against an artifact that Railway has already reaped or will reap. The deployment *ID* survives; the *image* does not. §10 can still be filled honestly, but it is a table of things that were true once, against artifacts that no longer exist.

A standing probe is the only thing that converts those into claims that are still true *now*. That is why this ships as a peer of B8 rather than after it.

## What it asserts

| Probe | Assertion | Maps to |
|---|---|---|
| `GET /health` | HTTP 200 | V1, TRIGGER 1 class |
| | `ok == true` | V1 |
| | `embed_dim == 1024` | new — `chunks.embedding` is `vector(1024)` |
| | `supabase_configured == true` | V2, explicitly labelled **not** a rollback trigger per §8.2 |
| `POST /convert`, unauthenticated | never `2xx` | **N9 / TRIGGER 4**, register #45's exact failure |
| | status matches what `main.py` declares | **N6**, **N8**, and register #39 drift |

## The derivation is the load-bearing idea

The expected `/convert` status is **derived from `services/ingestion/main.py` at the checked-out commit**, not hardcoded:

- `main.py` declares `@app.post("/convert")` → expect **401**
- it does not → expect **404**

Three consequences, none of which need a future edit to this file:

1. **N6 for free.** PR C removes the shim; the next tick asserts 404 automatically.
2. **N8 for free — and this is the one that matters.** §9 warns that a Watch Paths glob which never matches means *"PR C merges green and `/convert` stays live forever, with no symptom anywhere."* **This is the symptom.** Within one tick of PR C merging, a `/convert` still answering 401 while `main.py` no longer declares it turns that silent failure loud. That dependency is the reason this lands **before** Watch Paths is set, not after.
3. **A live-vs-`main` drift detector.** The expectation comes from the checked-out commit; the answer comes from the running service. Disagreement means live ≠ `main`, which is register #39's entire hazard. Reddening on that is the feature.

## Security posture

Every probe is unauthenticated. `INGESTION_SERVICE_URL` is a **hostname, not a credential** — a repository secret only so the production host is not published in a public repo and is masked in this public repo's job logs. The job cannot upload, convert, embed, or read a row. `permissions: contents: read`. If that property is ever lost, this file has become something else.

## Two things reviewers should check rather than take on trust

- **The multipart file part in probe 2 is load-bearing.** FastAPI validates the body *before* the bearer check, so an unauthenticated `POST /convert` with no file returns **422 `"Field required"`**, not 401. Verified by direct probe on 2026-08-02, not inferred. Without the file part this step would compare 422 against 401 and red on every run — and the fix someone reaches for under that pressure is to relax the assertion, which is how a monitor becomes decorative. Same failure shape as `ingestion-image.yml`'s missing `-i` (PR #71).
- **Three retries before red, on purpose.** A monitor that alarms on one cold start gets muted, and a muted monitor is worse than none because it reads as coverage.

## Stated limitations, not hidden ones

- **GitHub disables scheduled workflows in a public repo after 60 days without a commit.** A monitor that can stop silently is the exact bug class this repo is retiring, so it is written into the file. The durable fix is a dead-man's-switch alarming on the *absence* of a run. Until then, 60 days of silence is an accepted, written-down risk.
- **Scheduled runs are delayed under load.** Real detection window is tens of minutes, not 15 exactly. Still two orders of magnitude better than nine days.
- **This PR cannot self-test.** There is no `pull_request` trigger, by design — it probes production. It will be verified by `workflow_dispatch` immediately after merge, and that run is the evidence this file is trusted on.
- **No register number claimed.** Register bookkeeping belongs in B8's docs PR; inventing a number here would collide with it.

## Deliberately NOT in this PR

- **No Watch Paths change.** This monitor is a prerequisite for that decision, not a companion to it.
- **No `railway.json`.** That is B9, and it carries its own abort condition.
- **No base-image digest pin or hash-locked requirements.** Both are `services/` changes and both deserve their own review — see the note below.

## One thing this PR makes newly urgent

§8.1 states the rollback *"re-activates an already-built image with no rebuild."* **With every deployment marked REMOVED, that is no longer true.** Rollback is now a rebuild from source, against an unpinned `python:3.11-slim` and unlocked transitive dependencies — so it restores *source*, not *state*, and cannot reproduce the artifact that was running. §8.1's four triggers survive as detection conditions; its remedy paragraph does not. That correction belongs in B8's docs PR, batched — because a standalone docs PR would itself redeploy production ingestion, which is the root cause we just documented.

---

⚠️ **Merging this PR will rebuild and redeploy production ingestion**, despite touching only `.github/`. That is the root cause confirmed empirically today, and it is stated here rather than discovered again later.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant