Skip to content

infra(freshness-gate): CI gates for generated artifacts (copy-docs + openapi.json + types.ts)#433

Open
SoundMindsAI wants to merge 10 commits into
mainfrom
infra_generated_artifact_freshness_gate
Open

infra(freshness-gate): CI gates for generated artifacts (copy-docs + openapi.json + types.ts)#433
SoundMindsAI wants to merge 10 commits into
mainfrom
infra_generated_artifact_freshness_gate

Conversation

@SoundMindsAI
Copy link
Copy Markdown
Owner

Summary

Ships infra_generated_artifact_freshness_gate end-to-end (both phases together). CI now catches every "developer edits a source file but forgets to regenerate the committed artifact built from it" failure mode for the three generated surfaces in the repo:

  • docs/08_guides/*.mdui/public/docs/*.md (Phase 1)
  • backend FastAPI route table → ui/openapi.json (Phase 2)
  • ui/openapi.jsonui/src/lib/types.ts (Phase 2)

The single canonical fix command for all three: bash scripts/regen-generated-artifacts.sh.

Spec: feature_spec.md. Plan: implementation_plan.md. The standalone Phase-2 record at 02_mvp2/infra_openapi_types_freshness_gate/ is retired at finalization since both phases ship together.

Phase 1 — copy-docs freshness gate (Stories 1.1 + 1.2)

  • ui/scripts/copy-docs.mjs refactored to export DOCS, pruneStale, getDestDir, runCopyDocs + added an ESM entry-point guard so importing the module is a no-op. Adds an FR-9 prune step that deletes any *.md in ui/public/docs/ outside {README.md} ∪ {DOCS[].dest} — a renamed entry never leaves a stale public copy behind.
  • .github/workflows/copy-docs-freshness.yml (new) — runs on every PR with NO paths-ignore filter (FR-3 escape from pr.yml's docs/** filter, mirroring secrets-defense.yml's own-workflow precedent) so docs-only PRs still get the check.
  • scripts/ci/verify_copy_docs_fresh.sh + test_verify_copy_docs_fresh.sh — bash guard + self-test (clean / source-drift / untracked AC-9 cases against disposable mktemp git fixtures).

Phase 2 — OpenAPI export + types gate (Stories 2.1 / 2.2 / 2.3 / 2.4)

  • backend/app/openapi_export.py (new) — offline deterministic CLI exporter. Module docstring records the FR-4 import-graph spike resolution (path (a) — *_FILE dummy stand-ins). build_openapi() + serialize(sort_keys=True, compact, ensure_ascii=False, trailing newline). The unit test runs against a deliberately non-resolvable REDIS_URL host to prove no live clients are instantiated at import or app.openapi() time.
  • ui/openapi.json (new, 149KB, 52 paths) — canonical snapshot, byte-stable across runs.
  • ui/scripts/gen-types.mjs — refactored to use the lockfile-pinned node_modules/.bin/openapi-typescript (no npx fallback, fails loudly if pnpm install was skipped) + imports a buildBanner() from the new pure module ui/scripts/gen-types-banner.mjs (the banner names the COMMITTED snapshot path, not the live OPENAPI_URL, so a local-dev regen + a CI-snapshot regen produce byte-identical banners — FR-5 source-invariance). ESM entry-point guard added.
  • .github/workflows/pr.yml — new generated-artifacts-fresh job mirroring license-inventory's shape (uv + Python + pnpm + node). Runs the snapshot guard, the types guard, AND an AC-7 clean-tree determinism step that calls the real scripts/regen-generated-artifacts.sh end-to-end and asserts the working tree stays clean (catches a regenerator that is itself non-deterministic across runs).
  • scripts/regen-generated-artifacts.sh (new) — one-paste chained fix for all three artifacts. Respects REGEN_NO_STAGE=1 so the CI determinism step can inspect the working tree directly.
  • ui/.prettierignore (new) — generated files are NOT prettier-formatted. src/lib/types.ts + public/docs/*.md are listed: the generator is the source of truth, and reformatting them would make the gates flap between local-prettier-formatted bytes and CI-canonical openapi-typescript bytes.

Tangential inline fix (per CLAUDE.md tangential-discoveries rule)

  • ui/src/__tests__/components/studies/studies-table-ceiling-badge.test.tsx — fixture omitted trial_count, which the backend marks required (int = 0 at backend/app/api/v1/schemas.py:902, shipped with PR docs(research): complementary-architecture one-pager (three-layer handoff) #421). The freshness-gate regen of types.ts surfaced the pre-existing schema/test drift. One-line fix (trial_count: 0) with the rationale documented in a block comment.

Test coverage

48 new test cases total:

Layer File Cases
Backend unit backend/tests/unit/test_openapi_export.py 10
UI vitest ui/src/__tests__/scripts/copy-docs.prune.test.ts 11
UI vitest ui/src/__tests__/scripts/gen-types-banner.test.ts 6
Shell guard self-test scripts/ci/test_verify_copy_docs_fresh.sh 7
Shell guard self-test scripts/ci/test_verify_openapi_snapshot_fresh.sh 7
Shell guard self-test scripts/ci/test_verify_types_fresh.sh 7

Plan §3.2 (integration), §3.3 (contract), §3.5 (E2E), §3.6 (migration) are explicitly N/A for this CI-tooling feature.

Cross-model review (so far)

Test plan

  • make test-unittest_openapi_export.py 10/10 green
  • pnpm --dir ui test → 1025/1025 (140 files; baseline was 1019, +6 from buildBanner test)
  • bash scripts/ci/test_verify_copy_docs_fresh.sh → 7/7
  • bash scripts/ci/test_verify_openapi_snapshot_fresh.sh → 7/7
  • bash scripts/ci/test_verify_types_fresh.sh → 7/7
  • make lint → 0 errors
  • make typecheck → clean
  • pnpm --dir ui typecheck → clean
  • .venv/bin/ruff format --check backend/ → 565 files clean
  • AC-7 clean-tree determinism: REGEN_NO_STAGE=1 bash scripts/regen-generated-artifacts.sh against the committed tree leaves git status --porcelain empty
  • Operator-path verification: bash scripts/ci/verify_copy_docs_fresh.sh + verify_openapi_snapshot_fresh.sh + verify_types_fresh.sh all emit "OK: …is fresh."
  • CI green on the new generated-artifacts-fresh job + the new copy-docs-freshness workflow

🤖 Generated with Claude Code

SoundMindsAI and others added 9 commits June 2, 2026 22:56
… 1.1)

Story 1.1 of infra_generated_artifact_freshness_gate (FR-9 / AC-11):
make copy-docs.mjs delete any *.md not in {README.md} ∪ {DOCS[].dest}
so a renamed or removed DOCS entry no longer leaves a stale public copy.

- Refactor copy-docs.mjs to export DOCS, getDestDir, pruneStale,
  runCopyDocs + add an ESM entrypoint guard so importing the module
  no longer triggers generation (mirrors gen-types.mjs pattern).
- Add ui/src/__tests__/scripts/copy-docs.prune.test.ts (11 cases):
  exported-shape sanity, pruneStale direct behavior (delete .md,
  preserve non-.md, no-op on clean), runCopyDocs end-to-end against
  tmp dirs (clean run, prune-on-removed-entry, idempotency,
  rename-mid-flight, cwd-equivalence, entry-point-guard).
- Verified operator path: node ui/scripts/copy-docs.mjs on a clean
  tree leaves git status --porcelain -- ui/public/docs/ empty.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
…y 1.2)

Story 1.2 of infra_generated_artifact_freshness_gate (FR-1 + FR-3 +
FR-8 Phase-1 + FR-6 docs half). Catches the failure mode where a
contributor edits a source guide under docs/08_guides/ without
re-running copy-docs.mjs, leaving ui/public/docs/ stale.

- scripts/ci/verify_copy_docs_fresh.sh — regen via copy-docs.mjs,
  fail on git status --porcelain drift (--porcelain catches
  modified, untracked, AND deleted; bare git diff misses untracked,
  which is the FR-9 / AC-9 case). Prints the canonical fix command
  on failure. Honors COPY_DOCS_FRESH_REPO_ROOT override for the
  self-test's disposable git fixture.
- scripts/ci/test_verify_copy_docs_fresh.sh — three cases against
  fresh mktemp git fixtures: clean (exit 0), source-drift (exit 1
  with the canonical fix-command text), untracked AC-9 via
  `git rm --cached` (exit 1 with ?? marker).
- .github/workflows/copy-docs-freshness.yml — runs on every PR to
  main with NO paths/paths-ignore filter (FR-3 escape from pr.yml's
  docs/** filter so docs-only PRs still get the check). Mirrors
  secrets-defense.yml's own-workflow precedent. Action SHAs pinned
  per chore_scorecard_pin_deps_postcss (PR #430).
- docs/05_quality/testing.md — new "Generated-artifact freshness
  gates" subsection documenting the gate, why --porcelain (not
  --exit-code), and the canonical fix command.

Verification: 7/7 self-test cases green; guard against the live repo
emits "OK: ui/public/docs/ is fresh."; workflow YAML parses.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
…hase-gate finding #3)

GPT-5.5 phase-gate review flagged that the freshness-gates subsection
opened with "Three CI gates" while only documenting one — the Phase 2
snapshot + types gates land later. Soften the lede to "a family of CI
gates" + add an explicit Phase 1 / Phase 2 sentence so a reader at this
commit sees an accurate map of what ships when.

Findings #1 (prune set derivation) and #2 (cwd-robustness coverage) were
rejected with cited counter-evidence in the PR adjudication summary.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
Story 2.1 of infra_generated_artifact_freshness_gate (FR-4 / AC-4).
A CLI entrypoint that emits the canonical OpenAPI schema with no
running server, live Postgres, Redis, ES/OpenSearch/Solr, or OpenAI
client — the foundation for Story 2.2's `ui/openapi.json` snapshot
freshness gate.

- backend/app/openapi_export.py — argparse CLI with --out (atomic
  tmpfile + os.replace) or stdout. build_openapi() stubs the
  *_FILE-mounted Settings inputs via tempfile.mkdtemp + REDIS_URL
  bare env (non-secret, per Absolute Rule #2). serialize() applies
  the canonical form (sort_keys=True, compact separators,
  ensure_ascii=False, trailing newline) so output is byte-stable
  macOS↔Linux. All diagnostics → stderr; stdout is byte-pure JSON.

- Module docstring records the FR-4 import-graph spike (path (a)
  resolution): app.openapi() walks routes + Pydantic models and
  does NOT trigger FastAPI's lifespan — no asyncpg pool / Redis
  client / engine adapter is constructed at schema-build time. The
  companion unit test runs with a deliberately non-resolvable
  REDIS_URL host and asserts build_openapi() still succeeds,
  converting any future regression (a router opening a connection
  at import) into an immediate unit-test failure.

- backend/tests/unit/test_openapi_export.py — 10 cases: parsed-key
  assertions (NOT a leading-byte prefix, per plan task 2.1.4 note),
  byte-stability across repeated calls, canonical-form invariants,
  no-service-containers smoke, stdout-vs-stderr discipline, atomic
  write verification (no .tmp leak), overwrite path, idempotency,
  and the `python -m`-style invocation smoke.

Operator-path verification: `python -m backend.app.openapi_export`
emits 52 paths and parses cleanly. Lint + mypy --strict clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
Story 2.2 task 1 of infra_generated_artifact_freshness_gate (FR-7).
Generated by `python -m backend.app.openapi_export --out ui/openapi.json`
using Story 2.1's exporter. 52 paths, canonical form (sort_keys=True,
compact separators, ensure_ascii=False, trailing newline).

REUSE-lint coverage: ui/openapi.json is automatically covered by the
existing **/*.json glob at REUSE.toml:23, so no annotation needed
(Risk R-3 already mitigated).

Subsequent commit on this branch adds the snapshot-freshness guard +
self-test + the generated-artifacts-fresh pr.yml job.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
…ry 2.2 b)

Story 2.2 task 2-4 of infra_generated_artifact_freshness_gate
(FR-7 + FR-6 + FR-8 Phase-2 half).

- scripts/ci/verify_openapi_snapshot_fresh.sh — regen via the offline
  exporter (Story 2.1), fail on `git status --porcelain` drift. Uses
  --porcelain (not --exit-code) so the untracked case (a first commit
  forgetting to git add the snapshot) is flagged. Supports an
  OPENAPI_SNAPSHOT_REGEN_SCRIPT path-override for the self-test
  fixture (script path, not shell command — avoids read -ra word-
  splitting and shell-quoting traps).

- scripts/ci/test_verify_openapi_snapshot_fresh.sh — three cases
  against fresh mktemp git fixtures: clean (same bytes → exit 0),
  source-drift (different bytes → exit 1 with canonical fix-command
  text), untracked AC-9 (`git rm --cached` → ?? marker → exit 1).
  The override means the fixture doesn't need uv + the project venv
  — the exporter has its own Story-2.1 unit test; this self-test
  verifies the guard's diff-detection logic only.

- .github/workflows/pr.yml — new `generated-artifacts-fresh` job
  mirroring license-inventory's structure (uv + Python + pnpm +
  node). Snapshot guard runs here; Story 2.3 appends the types-guard
  step to the same job. Not under paths-ignore — both backend and
  UI changes can invalidate the snapshot.

- docs/05_quality/testing.md — appends gate #2 row to the freshness-
  gates table per the cross-story testing.md ownership declared in
  implementation_plan.md §11; documents both fix commands.

Verification: 7/7 self-test cases green; live-repo guard re-runs the
exporter and emits "OK: ui/openapi.json is fresh."; `uv run python -m
backend.app.openapi_export` produces byte-identical output to the
committed snapshot (determinism confirmed).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
Story 2.3 of infra_generated_artifact_freshness_gate (FR-5 + FR-2 +
FR-6 types half).

- ui/scripts/gen-types-banner.mjs (new) — pure, side-effect-free
  module exporting buildBanner(). The banner names the COMMITTED
  snapshot path (ui/openapi.json), not the live OPENAPI_URL value,
  so local-dev + CI-snapshot regens produce byte-identical banners
  (FR-5 source-invariance). Drops the false "CI does NOT regenerate"
  stance and names the generated-artifacts-fresh CI gate instead.

- ui/scripts/gen-types.mjs — three changes:
  1. Pinned-binary invocation via node_modules/.bin/openapi-typescript
     (no npx fallback) — fails loudly if pnpm install was skipped.
  2. Imports buildBanner from the new pure module.
  3. ESM entry-point guard — importing the module is a no-op.

- ui/src/__tests__/scripts/gen-types-banner.test.ts (new) — 6 cases:
  byte-stability, invariance across OPENAPI_URL values, canonical
  Source-line, SPDX prefix preserved, freshness-gate stance.
  Automated AC-8.

- scripts/ci/verify_types_fresh.sh + test_verify_types_fresh.sh —
  guard regenerates via canonical pnpm types:gen invocation; fails
  on git status --porcelain drift; prints chained fix command
  (Story 2.4). Self-test uses TYPES_FRESH_REGEN_SCRIPT path-override
  pattern from Story 2.2. 7/7 self-test cases green.

- .github/workflows/pr.yml — appends self-test + types-guard steps
  to the existing generated-artifacts-fresh job (cross-story edit
  declared in implementation_plan.md §11).

- docs/05_quality/testing.md — appends row #3 to the freshness-gates
  table + chained fix command.

- ui/src/lib/types.ts — regenerated via the refactored gen-types.mjs
  + new buildBanner. PR §16 rollout requirement: introducing PR
  freshens all artifacts. Prettier-formatted post-regen.

Tangential inline fix (per CLAUDE.md tangential-discoveries rule —
<60 min, same subsystem, no design fork):
- studies-table-ceiling-badge.test.tsx fixture omitted trial_count,
  which the backend marks required (int = 0 at backend/app/api/v1/
  schemas.py:902, shipped with PR #421). Pre-existing test passed
  only against the stale types.ts; the freshness-gate regen surfaced
  the drift. Added trial_count: 0 with a citing comment.

Verification: 17/17 scripts vitests green; 7/7 types-guard self-test
green; pnpm typecheck clean; reuse-lint compliant (REUSE-IgnoreStart/
End wrappers added around an SPDX-shaped regex literal in
gen-types-banner.test.ts that reuse-lint was mis-parsing as a real
declaration).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
…ory 2.4)

Story 2.4 of infra_generated_artifact_freshness_gate (FR-8 chained
+ FR-6 determinism + AC-7).

- scripts/regen-generated-artifacts.sh (new) — one-paste chained
  regen for all three CI-freshness-gated artifacts:
    1. ui/openapi.json    (uv run python -m backend.app.openapi_export)
    2. ui/src/lib/types.ts (pnpm types:gen, reading the snapshot at 1)
    3. ui/public/docs/    (node ui/scripts/copy-docs.mjs)
  Step ordering matters — types.ts derives from the snapshot, so the
  snapshot must regenerate first. After regen, all three are
  `git add`ed. REGEN_NO_STAGE=1 skips the staging step (used by CI's
  AC-7 determinism assertion so it inspects the working tree directly).

- ui/.prettierignore (new) — generated files are NOT prettier-formatted.
  `ui/src/lib/types.ts` (openapi-typescript output) and
  `ui/public/docs/*.md` (copy-docs.mjs output) are listed; the
  generator is the source of truth. Without this, prettier would
  reformat the openapi-typescript output and the freshness gate
  would flap between local-prettier-formatted and CI-canonical bytes.

- ui/src/lib/types.ts — regenerated via the canonical wrapper, NOT
  prettier-formatted. This is what every future regen produces and
  what the gate now expects. Two consecutive `bash scripts/regen-
  generated-artifacts.sh` invocations against this commit's tree
  produce byte-identical types.ts — FR-6 verified.

- scripts/ci/verify_*.sh — all three guards now point their fix-
  command output at the canonical chained wrapper as the primary,
  with the per-gate one-liner shown as a fallback. Self-tests still
  green (7+7+7 = 21 cases) because the existing per-gate substrings
  remain in the output.

- .github/workflows/pr.yml — appends an AC-7 clean-tree determinism
  step to the generated-artifacts-fresh job. After both per-gate
  guards have run, the step does a fresh canonical regen + asserts
  the working tree is clean. Catches a regenerator that is itself
  non-deterministic across runs, distinct from drift against the
  committed snapshot.

- docs/05_quality/testing.md — promotes the chained wrapper as the
  single canonical fix command, demotes per-gate fixes to a
  fallback section, names the AC-7 determinism assertion, documents
  the `.prettierignore` rationale.

- CLAUDE.md — adds a "Generated artifacts" subsection under Key
  Conventions naming the chained regen + the prettier-ignore rule.

Verification: 21/21 self-test cases green (7 per guard); canonical
regen output is byte-identical across consecutive runs (FR-6); a
fresh regen against the committed tree leaves git status clean
(AC-7); pr.yml parses cleanly.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
Adds the merge one-liner to "Last 5 merges" (drops the now-6th entry
to state_history.md's pointer); flips the "Current branch / execution
context" section to the new feature branch + 8 commits; updates the
"In flight" + "Plan-stage" sections.

state.md size: 24,725 bytes (60KB cap).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements the infra_generated_artifact_freshness_gate feature, introducing CI freshness gates to ensure committed generated artifacts (OpenAPI snapshot, TypeScript types, and public documentation) do not drift from their sources. It adds an offline OpenAPI schema exporter, verification scripts with self-tests, a unified regeneration script, and refactors existing documentation-copying and type-generation scripts to be deterministic and robust. Feedback on the changes suggests cleaning up temporary directories and files in the OpenAPI exporter to prevent resource leaks, and enabling shell: true on Windows platforms in gen-types.mjs to ensure cross-platform compatibility when executing the pinned binary.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +90 to +91
tmp_dir = Path(tempfile.mkdtemp(prefix="relyloop-openapi-export-"))
os.environ.setdefault("RELYLOOP_OPENAPI_EXPORT_TMP", str(tmp_dir))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The temporary directory created by tempfile.mkdtemp() is not cleaned up, which can lead to a resource leak over time during repeated executions or test runs. Registering an atexit handler ensures that the directory and its contents are automatically removed when the process exits.

    tmp_dir = Path(tempfile.mkdtemp(prefix="relyloop-openapi-export-"))
    import atexit
    import shutil
    atexit.register(lambda: shutil.rmtree(tmp_dir, ignore_errors=True))
    os.environ.setdefault("RELYLOOP_OPENAPI_EXPORT_TMP", str(tmp_dir))

Comment thread backend/app/openapi_export.py Outdated
Comment on lines +167 to +179
with tempfile.NamedTemporaryFile(
mode="w",
encoding="utf-8",
dir=str(path.parent),
prefix=f".{path.name}.",
suffix=".tmp",
delete=False,
) as tmp:
tmp.write(content)
tmp.flush()
os.fsync(tmp.fileno())
tmp_path = Path(tmp.name)
os.replace(tmp_path, path)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If an exception occurs during the file write or sync operations inside the with block, the temporary file created with delete=False will be leaked on disk. Wrapping the block in a try...finally statement ensures that the temporary file is cleaned up if the atomic replacement fails or is interrupted.

    tmp_path = None
    try:
        with tempfile.NamedTemporaryFile(
            mode="w",
            encoding="utf-8",
            dir=str(path.parent),
            prefix=f".{path.name}.",
            suffix=".tmp",
            delete=False,
        ) as tmp:
            tmp.write(content)
            tmp.flush()
            os.fsync(tmp.fileno())
            tmp_path = Path(tmp.name)
        os.replace(tmp_path, path)
        tmp_path = None
    finally:
        if tmp_path is not None:
            try:
                tmp_path.unlink(missing_ok=True)
            except OSError:
                pass

Comment thread ui/scripts/gen-types.mjs
Comment on lines +99 to +102
execFileSync(bin, [SOURCE_URL, '-o', OUTPUT], {
stdio: 'inherit',
cwd: UI_ROOT,
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

On Windows, executing .cmd or .bat files directly via execFileSync without shell: true will fail with ENOENT or EACCES because Windows cannot natively execute batch files without cmd.exe. Setting shell: process.platform === 'win32' ensures compatibility for Windows developers while preserving shell-free execution on POSIX platforms.

Suggested change
execFileSync(bin, [SOURCE_URL, '-o', OUTPUT], {
stdio: 'inherit',
cwd: UI_ROOT,
});
execFileSync(bin, [SOURCE_URL, '-o', OUTPUT], {
stdio: 'inherit',
cwd: UI_ROOT,
shell: process.platform === 'win32',
});

PR #433 Gemini Code Assist review surfaced three medium-severity
resource-hygiene findings, all accepted:

1. backend/app/openapi_export.py:91 — register atexit cleanup for the
   dummy *_FILE tmpdir created by _ensure_dummy_settings_env(). Each
   invocation leaked ~100 bytes; not a real disk concern but sloppy.
   atexit.register(shutil.rmtree, ..., ignore_errors=True) is the
   stdlib pattern.

2. backend/app/openapi_export.py:_write_atomic — wrap the
   NamedTemporaryFile(delete=False) + os.replace flow in try/finally.
   If write/flush/fsync OR the rename raised (disk full, permission
   denied), the orphan `.<file>.<rand>.tmp` would persist next to the
   destination. tmp_path = None after a successful replace tells the
   finally block "the rename took ownership; don't try to delete the
   now-renamed file". The finally's unlink is best-effort
   (missing_ok=True + caught OSError) so it never masks the original
   exception.

3. ui/scripts/gen-types.mjs:execFileSync — add `shell:
   process.platform === 'win32'` so Node can invoke the
   openapi-typescript.cmd shim on Windows (cmd.exe is required to
   interpret batch files; per the Node child_process docs:
   https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files).
   POSIX stays shell-free.

Each fix carries an inline citation back to the Gemini finding so a
future archeologist can trace the rationale.

Verification: 10/10 unit tests still passing; live snapshot + types
guards still emit OK on a clean tree; rtk mypy --strict + ruff clean on
the modified Python; rtk prettier clean on gen-types.mjs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
@SoundMindsAI
Copy link
Copy Markdown
Owner Author

Review adjudication (Gemini Code Assist + GPT-5.5 phase gates)

Commit landing accepted fixes: b0579fb7

Gemini Code Assist (3 findings — all accepted)

# Sev Location Verdict Notes
1 Medium backend/app/openapi_export.py:91 Accepted tempfile.mkdtemp() leaked the dummy-*_FILE tmpdir at every CLI invocation (~100 bytes). Registered atexit cleanup via shutil.rmtree(..., ignore_errors=True). Inline citation in b0579fb7.
2 Medium backend/app/openapi_export.py:_write_atomic Accepted A raise between NamedTemporaryFile(delete=False) write/flush/fsync OR an os.replace failure would leak the .tmp next to the destination. Wrapped in try / finally with tmp_path = None after a successful replace (tells the finally block "the rename took ownership; don't try to delete the now-renamed file") and a best-effort unlink(missing_ok=True) cleanup that never masks the original exception.
3 Medium ui/scripts/gen-types.mjs:execFileSync Accepted Real Windows bug — Node's execFileSync can't invoke .cmd shims without shell: true (cmd.exe is required to interpret batch files; see child_process docs). Gated shell: true to process.platform === 'win32' so POSIX stays shell-free.

GPT-5.5 phase-gate Epic 1 (3 findings)

# Sev Verdict Notes
1 Medium Rejected "prune set diverges from planned interface" — ui/scripts/copy-docs.mjs:137 literally reads const expected = new Set(['README.md', ...docs.map((d) => d.dest)]);, exactly per plan's Key Interfaces. copied is a return-value-only array, never feeds the prune set.
2 Medium Rejected "cwd-robustness DoD not fully covered" — proven transitively via copy-docs.prune.test.ts:55-65 (getDestDir() cwd-invariance) + copy-docs.prune.test.ts:163-189 (runCopyDocs two-cwd test asserting identical output). Running the unparametrized script in vitest would mutate ui/public/docs/ on every test run (bad hygiene).
3 Low Accepted Fixed in f49531f5 — softened "Three CI gates" lede in testing.md to "A family of CI gates" + added explicit Phase 1 / Phase 2 sentence.

GPT-5.5 phase-gate Epic 2 (5 findings — all rejected)

# Sev Verdict Notes
1 High Rejected (false positive) "ui/openapi.json not in diff" — IS committed at 1a0151e8 (Story 2.2 a). False positive from the slim diff I sent to GPT-5.5 (I excluded the 149KB bot-generated artifact for review clarity).
2 High Rejected (false positive) "types.ts not in diff" — IS committed at 42b0b3b0 (Story 2.3) + re-regenerated at 3da3896e (Story 2.4) to the canonical non-prettier form. Same slim-diff false-positive cause. AC-7 clean-tree check confirms committed bytes match canonical regen output.
3 Medium Rejected "types-guard self-test uses an override instead of the real pnpm types:gen" — the TYPES_FRESH_REGEN_SCRIPT override is plan-authorized in Story 2.3 Task 4 ("the guard supports a test-only generator override … fallback"). End-to-end coverage of the real chain exists via the AC-7 clean-tree determinism step in pr.yml (Story 2.4) which runs the real scripts/regen-generated-artifacts.sh on every PR.
4 Medium Rejected "snapshot-guard self-test uses an override instead of the real exporter" — same plan authorization (Story 2.2 plan: "fallback: the guard supports a test-only generator override"). End-to-end exporter coverage exists via (a) test_openapi_export.py no-service-containers smoke (10 tests); (b) AC-7 runs the real uv run python -m backend.app.openapi_export on every PR.
5 Low Rejected "studies-table-ceiling-badge.test.tsx is unrelated to Epic 2 — split into a separate PR" — per CLAUDE.md "Tangential discoveries — fix inline by default": pre-existing schema/test drift surfaced by this PR's regen of types.ts is the textbook inline-fix case. One-line fix, same subsystem, zero design fork, rationale documented in both the commit body and a code comment. Splitting would violate "One branch per session — don't stack PRs."

Outcomes

Final cross-model review will follow once CI is green on b0579fb7. Ready for human review + merge after that.

@SoundMindsAI
Copy link
Copy Markdown
Owner Author

Final cross-model review

GPT-5.5 final review against the complete PR diff (vs main) passed clean — 0 findings. The rejection log fed from the Epic 1 + Epic 2 phase-gate adjudications worked as designed; no previously-rejected findings were re-raised and no new ones surfaced.

Waiting on CI green on b0579fb7; then ready for human review + merge.

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