Skip to content

PR 12 — Reviewer documentation and final hardening - #34

Merged
batbrainy merged 3 commits into
mainfrom
issue-22-reviewer-documentation
Jul 31, 2026
Merged

PR 12 — Reviewer documentation and final hardening#34
batbrainy merged 3 commits into
mainfrom
issue-22-reviewer-documentation

Conversation

@batbrainy

Copy link
Copy Markdown
Owner

Linked issue

Closes #22IMPLEMENTATION_PLAN.md §13's final PR.

Problem

The code was done; the documentation was not. docs/DESIGN_BRIEF.md did not exist though
§14 calls it the reviewer's primary architecture document. README.md:13 still announced
"Background processing and recovery stage (PR 8)" — four PRs stale, and the first line a
reviewer reads. The data model was absent entirely (S2.7), there was not one sample JSON log
line anywhere (S4.9), processing guarantees had no home (B10), and a ## Planned contents
table still deferred three items to this PR. Two of §14's eight named ADR topics — "Why Kafka
was not selected" and the pinned API version — had zero coverage in the repository. §16
forbids stale documentation, and PR 12 is the last chance to close any of it.

Scope

New: docs/DESIGN_BRIEF.md (two pages, mermaid request-path and data-model figures);
docs/SUBMISSION_CHECKLIST.md (§16's four gate lists as checkboxes, the forbidden-claim
scan, the post-merge email template); ADR 0011 (pinned API version 2022-11-28) and ADR 0012
(Solid Queue over Kafka); docs/evidence/2026-07-31-clean-checkout-verification.md.

README restructured to a 20-heading spine with a table of contents. Mostly moves
nine existing blocks relocate into new containers (Rate limits and the request budget,
Inspecting the data, Troubleshooting and reset, Logs), which is what keeps this affordable.
Genuinely new: the data model, sample log lines, processing guarantees, known limitations,
the §15 eleven-step verification walk, the SSRF boundary with a worked example, a three-level
reset ladder, and a symptom table.

IMPLEMENTATION_PLAN.md gains ## Appendix E only. §1–§17 and Appendices A–D are
byte-identical — the freeze governs changing direction mid-build without stating the
tradeoff, not recording afterward what changed.

Important technical decisions

Sample log lines are captured from a real fixture run, not hand-authored.
ingestion.run_completed alone is splat-merged across ~20 to_log methods; hand-authoring
gets a key or a nesting level wrong eventually, and a wrong sample in the section whose
entire purpose is trustworthiness is exactly the stale documentation §16 forbids. The capture
command sits beside the samples so they are self-verifying.

Two duplications are deliberate: the allowance formula (§14 requires it in the README
and the brief) and §8's at-least-once equation (§14 requires the durability boundary in the
brief; B10 requires processing guarantees where a reviewer will look). Both are short fences
kept byte-identical to the plan.

The two ADRs ship here rather than with their code because neither records a decision made
during a PR — both were made before PR 1 and never written down.

A defect found by this PR's own clean-checkout verification

docker compose up --build failed from a cold image and started zero containers:

target worker: failed to solve: image "github-push-ingestor-app:latest": already exists

Compose Bake — on by default in Docker Desktop — makes every service with a build: its own
bake target, and six services declared build: . against the same image: tag, so the
targets raced to export it. It reproduces only when the image is absent — a reviewer's
first command and no other run — which is why a spec-guarded design property had been
silently broken for a reviewer the whole time.

Fixed: setup is the only service on the up path that builds; web and worker reference
the tag and already wait on it; the tools one-shots keep their own build (each invoked alone
by docker compose run, a single target) plus pull_policy: build.
spec/docker_compose_spec.rb now asserts that invariant instead of the one that broke.

Other non-documentation changes

  • CI's foundation job asserts docs/DESIGN_BRIEF.md exists.
  • spec/requests/health_spec.rb asserts the health endpoints spend no budget — the one §16
    gate that was structurally true but untested.
  • Deleted config/initializers/cors.rb, commented-out scaffolding for a gem the plan
    excludes ("no dead or speculative infrastructure").
  • README's test command gains --build: the code is baked into the image rather than
    mounted, so running the suite against a stale image tests what the image was built from.

Testing performed

1728 examples, 0 failures plus 10 examples, 0 failures (spec/stress), against a rebuilt
image, repeated with --seed 4242.

Clean-checkout verification from a fresh clone into an empty directory on a host with no
image, both halves:

  • Live: one unauthenticated poll persisted 96 push events from a page of 100, ledger
    bootstrapped from GitHub's own headers, both entity classes enriched. 3 requests of 60.
  • Fixture, empty volume: 4 / 3 / 3 / 3, complete 2 / permanent_failure 1 per class,
    replay absorbing 4 duplicates with occurrences 3 → 6 and no reactivation.
  • script/verify_recovery.sh --confirm: 19 checks, absolute count mode for the first
    time (the committed kill transcript had to run in delta).
  • Test isolation measured rather than asserted: both development databases unchanged across a
    full test run, poller stopped so the counts could not drift.

Docker verification

Covered above and transcribed in docs/evidence/2026-07-31-clean-checkout-verification.md.

Documentation updates

This PR is the documentation update.

Known limitations

  • The clean-checkout transcript is one host, one date, one Docker version. The Bake collision
    is specific to Compose Bake plus the containerd image store.
  • The live half is one poll from one shared-budget IP. It shows the API is reachable without a
    token; it shows nothing about sustained behaviour or authenticated requests.
  • The authoritative clean-checkout run is post-merge, against the default branch.

Deferred, out of issue #22's scope — noted, not fixed: bin/dev execs rails server
(host-only scaffolding; removing it means editing bin/setup too); config/ci.rb runs
bin/setup --skip-serverdb:prepare on development databases, though no workflow
invokes it; config/credentials.yml.enc is committed with no key and nothing reads it;
config/bundler-audit.yml's CVE-THAT-DOES-NOT-APPLY placeholder; and §11's optional
/api/actors/:id, /api/repositories/:id, /api/ingestion_runs endpoints, correctly unbuilt.

🤖 Generated with Claude Code

batbrainy and others added 3 commits July 31, 2026 11:06
Completes the documentation deliverables of IMPLEMENTATION_PLAN.md §13-§16
and closes the three §4 documentation bullets: S2.7 (data model), S4.9
(expected logs and verification steps), and B10 (processing guarantees).

New:
- docs/DESIGN_BRIEF.md — the reviewer's primary architecture document, two
  pages, with mermaid request-path and data-model figures.
- docs/SUBMISSION_CHECKLIST.md — §16's four gate lists as checkboxes, the
  forbidden-claim scan, and the post-merge email template.
- docs/adr/0011, 0012 — the two §14 ADR topics that no implementation PR
  owned: the pinned API version 2022-11-28, and Solid Queue over Kafka.

README restructured to a 20-heading spine with a table of contents. Mostly
moves: nine existing blocks relocate into new containers (Rate limits,
Inspecting the data, Troubleshooting and reset, Logs). Genuinely new are the
data model, sample log lines, processing guarantees, known limitations, the
§15 eleven-step verification walk, the SSRF boundary, the reset ladder, and a
symptom table. Deleted the stale PR-by-PR Status narrative and the "Planned
contents" placeholder.

Sample log lines are captured verbatim from a fixture-mode run against an
empty database rather than hand-authored — the payloads are splat-merged
across ~20 to_log methods, and a wrong sample in the section whose purpose is
trustworthiness is exactly the stale documentation §16 forbids.

IMPLEMENTATION_PLAN.md gains Appendix E only; §1-§17 and Appendices A-D are
byte-identical. It records the seven divergences the build produced, notably
that §15 step 8's own `docker kill` cannot exercise `restart: unless-stopped`.

Non-documentation changes, all §16-driven:
- CI foundation job now asserts docs/DESIGN_BRIEF.md exists.
- spec/requests/health_spec.rb asserts the endpoints spend no budget — the
  one §16 gate that was structurally true but untested.
- Deleted config/initializers/cors.rb, commented-out scaffolding for a gem
  the plan excludes ("no dead or speculative infrastructure").

Closes #22

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by PR 12's own clean-checkout verification, which is the deliverable
that exists to catch exactly this.

From a cold image, `docker compose up --build` — the assignment's first
command, §14's required-commands block, and the README's Quick start —
failed and started zero containers:

  target worker: failed to solve:
  image "docker.io/library/github-push-ingestor-app:latest": already exists

Compose Bake, on by default in Docker Desktop, makes every service with a
`build:` its own bake target. Six services declared `build: .` against the
same `image:` tag, so the targets raced to export it. It reproduces only when
the image is absent — which is a reviewer's first command and no other run,
which is why every previous run passed.

Fix: `setup` is the only service on the `up` path that builds; `web` and
`worker` reference the tag and already wait on
`setup: service_completed_successfully`, and Compose completes the build
phase before the create phase. The `tools` one-shots keep their own build —
each is invoked alone by `docker compose run`, a single target that cannot
collide — plus `pull_policy: build`, so Compose stops attempting a registry
pull of a tag that is local-only by construction.

spec/docker_compose_spec.rb now asserts the invariant that keeps this working
rather than the one that broke it: exactly one build among the services a
plain `up` starts, the buildless two waiting on it, and each one-shot able to
build for itself.

Also: the README's test command gains `--build`, because the code is baked
into the image rather than mounted — running the suite against a stale image
tests the code the image was built from, which is green and meaningless.

Suite: 1728 examples + 10 stress, 0 failures, against a rebuilt image.

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

Commits the dated first-party transcript for §13's clean-checkout deliverable:
docs/evidence/2026-07-31-clean-checkout-verification.md.

Run from a fresh clone into an empty directory, on a host with no image for
this project, against 6eab84c. Both halves:

- Live: one unauthenticated poll persisted 96 push events from a page of 100,
  4 non-push ignored, ledger bootstrapped from GitHub's own headers, and both
  entity classes enriched. 3 requests of 60 spent. This is the only thing that
  can prove §16's first functional gate.
- Fixture, from an empty volume: 4 / 3 / 3 / 3, complete 2 / permanent_failure
  1 per class, replay absorbing 4 duplicates with occurrences 3 -> 6 and no
  reactivation, and verify_recovery.sh's 19 checks in *absolute* count mode
  for the first time — the committed kill transcript had to run in delta.
- Suite 1728 + 10, 0 failures, repeated with --seed 4242. Test isolation
  measured rather than asserted: both development databases unchanged across
  a full test run, with the poller stopped so the counts could not drift.

The transcript documents the cold-build defect it found (fixed in 6eab84c)
rather than quietly presenting a passing run, because the failure is the
strongest argument for why the deliverable exists.

Also documents one hazard the run exposed: bringing a killed container back
means `docker compose up -d <service>`, which recreates it from the current
shell environment, where GITHUB_MODE defaults to live. A reviewer who starts
in fixture mode and runs the recovery script ends up polling real GitHub and
spending real quota with nothing saying so. README now says so, and gives the
one command that puts it back.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@batbrainy
batbrainy merged commit 3c938a2 into main Jul 31, 2026
2 checks passed
@batbrainy
batbrainy deleted the issue-22-reviewer-documentation branch July 31, 2026 17:35
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.

PR 12 — Reviewer documentation and final hardening

1 participant