refactor-openapi-gen #21

This PR refactors the OpenAPI codegen pipeline: instead of a committed stub crate at target/generated/server produced by scripts/codegen.sh, the generated server code now lives in a proper workspace crate core/ whose build.rs invokes openapi-gen-build (from a private forgejo registry) at build time, writing output into core/src/generated. CI and Nix packaging are reworked accordingly, and the generated source is gitignored.

The direction is sound — moving codegen into a workspace member with a build script removes the bootstrap/stub-crate hack and the pre-codegen step for plain cargo build. Lockfile, imports, docs, and CI wiring are all internally consistent, and the --locked CI runs (which install Java + the generator CLI) should exercise the whole path.

The main concern is that core/build.rs writes generated output into the source tree (core/src/generated). Under nix build the source is a read-only Nix store path, so the build script's writes will fail with a permission/read-only error — this breaks the documented nix build / just image workflows (which the new CI no longer covers). It also dirties the working tree on every build (mitigated only by gitignore) and can race between concurrent cargo invocations.

Secondary issues: the generator CLI jar is downloaded in CI without a checksum pin (supply-chain), the CARGO_* substitution is duplicated across two build scripts and (in core/build.rs) substitutes every CARGO_* env var rather than just the CARGO_PKG_* set, which means the spec fed to the generator resolves CARGO_PKG_NAME/CARGO_PKG_DESCRIPTION to the recurring-events-core crate identity instead of the app's (cosmetic today, but a latent correctness trap).

*🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: changes

# Review Summary This PR refactors the OpenAPI codegen pipeline: instead of a committed stub crate at `target/generated/server` produced by `scripts/codegen.sh`, the generated server code now lives in a proper workspace crate `core/` whose `build.rs` invokes `openapi-gen-build` (from a private forgejo registry) at build time, writing output into `core/src/generated`. CI and Nix packaging are reworked accordingly, and the generated source is gitignored. The direction is sound — moving codegen into a workspace member with a build script removes the bootstrap/stub-crate hack and the pre-codegen step for plain `cargo build`. Lockfile, imports, docs, and CI wiring are all internally consistent, and the `--locked` CI runs (which install Java + the generator CLI) should exercise the whole path. The main concern is that `core/build.rs` writes generated output into the **source tree** (`core/src/generated`). Under `nix build` the source is a read-only Nix store path, so the build script's writes will fail with a permission/read-only error — this breaks the documented `nix build` / `just image` workflows (which the new CI no longer covers). It also dirties the working tree on every build (mitigated only by gitignore) and can race between concurrent cargo invocations. Secondary issues: the generator CLI jar is downloaded in CI without a checksum pin (supply-chain), the CARGO_* substitution is duplicated across two build scripts and (in `core/build.rs`) substitutes every `CARGO_*` env var rather than just the `CARGO_PKG_*` set, which means the spec fed to the generator resolves `CARGO_PKG_NAME`/`CARGO_PKG_DESCRIPTION` to the `recurring-events-core` crate identity instead of the app's (cosmetic today, but a latent correctness trap). --- *🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: **changes**

clank-bot reviewed

2026-08-01 17:50:16 +00:00

clank-bot left a comment

Code review findings from opencode.

.woodpecker/verify.yaml

					
				@ -28,1 +16,4 @@

				    commands:

				      - version=7.24.0

				      - apt-get update -qq && apt-get install -y -qq default-jre-headless curl

				      - curl -fsSL --retry 3 https://repo1.maven.org/maven2/org/openapitools/openapi-generator-cli/$version/openapi-generator-cli-$version.jar -o /usr/local/bin/openapi-generator-cli.jar

[security] The openapi-generator-cli-7.24.0.jar is downloaded from Maven Central over HTTPS but its SHA-256 is never verified. Because this jar executes Java code with full build-script privileges, pin/verify the checksum (as is done for other pinned artifacts) to guard against tampering or a compromised mirror.

[security] The `openapi-generator-cli-7.24.0.jar` is downloaded from Maven Central over HTTPS but its SHA-256 is never verified. Because this jar executes Java code with full build-script privileges, pin/verify the checksum (as is done for other pinned artifacts) to guard against tampering or a compromised mirror.

core/build.rs

					
				@ -0,0 +5,4 @@

				    let workspace_root = manifest_dir.join("..");

				    let spec = workspace_root.join("openapi.yaml");

				    let rendered_spec = PathBuf::from(env::var("OUT_DIR").unwrap()).join("openapi.yaml");

				    let generated_dir = manifest_dir.join("src/generated");

[bug] Generated code is written into manifest_dir/src/generated, i.e. into the source tree. With nix build, lib.cleanSource ../. places the source in a read-only Nix store path, so fs::write in this build script will fail with a permission/read-only error. The old preBuild approach wrote into the writable target/generated. Since CI no longer exercises nix build / just image, this path is unverified and will likely break. Prefer writing generated output into OUT_DIR/CARGO_TARGET_DIR and include!-ing it (or make the output dir configurable via an env var, as the old GENERATED_DIR did). Writing into the source tree also dirties the checkout on every build and can race between concurrent cargo invocations.

[bug] Generated code is written into `manifest_dir/src/generated`, i.e. into the source tree. With `nix build`, `lib.cleanSource ../.` places the source in a read-only Nix store path, so `fs::write` in this build script will fail with a permission/read-only error. The old `preBuild` approach wrote into the writable `target/generated`. Since CI no longer exercises `nix build` / `just image`, this path is unverified and will likely break. Prefer writing generated output into `OUT_DIR`/`CARGO_TARGET_DIR` and `include!`-ing it (or make the output dir configurable via an env var, as the old `GENERATED_DIR` did). Writing into the source tree also dirties the checkout on every build and can race between concurrent cargo invocations.

core/build.rs

					
				@ -0,0 +8,4 @@

				    let generated_dir = manifest_dir.join("src/generated");

				    let mut content = fs::read_to_string(&spec).expect("openapi.yaml missing");

				    for (name, value) in env::vars() {

[style] The loop substitutes every CARGO_* env var into the spec, but the set exported to a build script is much broader than the CARGO_PKG_* placeholders the spec uses (it includes CARGO_MANIFEST_DIR, CARGO_HOME, CARGO_CFG_*, etc.). Restrict substitution to CARGO_PKG_ prefixed vars to avoid accidentally replacing future spec tokens with host-specific paths. The same logic is also duplicated verbatim in the root build.rs — consider extracting a shared helper (or a single rendering script) so the two copies cannot drift.

[style] The loop substitutes every `CARGO_*` env var into the spec, but the set exported to a build script is much broader than the `CARGO_PKG_*` placeholders the spec uses (it includes CARGO_MANIFEST_DIR, CARGO_HOME, CARGO_CFG_*, etc.). Restrict substitution to `CARGO_PKG_` prefixed vars to avoid accidentally replacing future spec tokens with host-specific paths. The same logic is also duplicated verbatim in the root `build.rs` — consider extracting a shared helper (or a single rendering script) so the two copies cannot drift.

martin marked this conversation as resolved

core/build.rs

					
				@ -0,0 +10,4 @@

				    let mut content = fs::read_to_string(&spec).expect("openapi.yaml missing");

				    for (name, value) in env::vars() {

				        if name.starts_with("CARGO_") {

				            content = content.replace(&format!("{{{{{name}}}}}"), &value);

[style] Because this build script runs in the recurring-events-core crate, CARGO_PKG_NAME and CARGO_PKG_DESCRIPTION resolve to recurring-events-core / 'Generated OpenAPI server code...'. The spec consumed by the generator therefore carries the core crate's identity rather than the app's (the old scripts/codegen.sh read values from the root [package]). The version is unaffected (shared workspace version), and the root-rendered spec served by Swagger UI is still correct, so this is cosmetic today — but it silently changes info.title/info.description in the generated code and is easy to trip on. Consider reading the root package identity explicitly (e.g. from the workspace Cargo.toml) instead of relying on the core crate's env vars.

[style] Because this build script runs in the `recurring-events-core` crate, `CARGO_PKG_NAME` and `CARGO_PKG_DESCRIPTION` resolve to `recurring-events-core` / 'Generated OpenAPI server code...'. The spec consumed by the generator therefore carries the core crate's identity rather than the app's (the old `scripts/codegen.sh` read values from the root `[package]`). The version is unaffected (shared workspace version), and the root-rendered spec served by Swagger UI is still correct, so this is cosmetic today — but it silently changes `info.title`/`info.description` in the generated code and is easy to trip on. Consider reading the root package identity explicitly (e.g. from the workspace `Cargo.toml`) instead of relying on the core crate's env vars.

martin marked this conversation as resolved

martin added 1 commit

2026-08-01 18:05:19 +00:00

Add missing dependency futures_util

ci/woodpecker/push/verify Pipeline was successful

Details

ci/woodpecker/pr/pr-review Pipeline was successful

Details

6a82e6e743

clank-bot commented

2026-08-01 18:10:10 +00:00

Bash appears unavailable; I'll use file tools instead.
{
"summary": "# Review Summary\n\nSolid refactor: codegen moves into a dedicated core crate driven by a build script, CI is consolidated into one step, and AGENTS.md/docs are updated to match. The generated-server-as-workspace-member pattern (core/src/generated, gitignored, produced by core/build.rs) is coherent and the router/import updates in src/ are consistent.\n\nMain concerns are around supply-chain hygiene and environment consistency rather than core logic:\n\n- CI downloads a JAR from Maven Central and executes it with only a test -s size check — no checksum verification.\n- Generator version drift: CI pins openapi-generator-cli 7.24.0, but the Nix devshell and nix build use whatever nixpkgs ships. Generated server code can differ between the CI-validated binary and the nix-built image.\n- New hard dependency on a private Forgejo cargo registry for the openapi-gen-build build-dependency; every cargo/nix build now requires reachability (and possibly auth) to code.martials.no.\n- Codegen writes into the crate's own source tree (core/src/generated) rather than OUT_DIR; every cargo command in the workspace now hard-requires Java + the CLI on PATH, and the old nix run nixpkgs#openapi-generator-cli fallback from scripts/codegen.sh is gone.\n- Minor: the just release flow's awk/sed now match [workspace.package] version only by coincidence (they ignore TOML sections), a broken sentence in AGENTS.md, and CI builds twice (cargo build + cargo test --all-targets).\n\nNone of these are blockers on their own, but the checksum pinning and single-source-of-truth for the generator version are worth fixing before merge.",
"verdict": "changes",
"comments": [
{
"file": ".woodpecker/verify.yaml",
"line": 19,
"severity": "security",
"message": "The openapi-generator-cli JAR is downloaded from Maven Central and later executed by core/build.rs, but only a test -s size check is performed — no checksum verification. Pin and verify a SHA-256 (e.g. curl ... | sha256sum -c) so a tampered or truncated artifact fails the build instead of running. Also consider --max-time on curl to avoid stalling CI on a hanging download."
},
{
"file": ".woodpecker/verify.yaml",
"line": 17,
"severity": "bug",
"message": "Generator version drift: CI pins openapi-generator-cli 7.24.0, but flake.nix:64 (devshell) and the nix build nativeBuildInputs (.nix/pkgs.nix:12) pull whatever version nixpkgs ships. The generated core/src/generated code is not committed, so CI-validated output and the nix-built image can differ and produce divergent API behavior. Pin a single generator version across CI, devshell, and the Nix build (e.g. via an overlay or a pinned derivation)."
},
{
"file": ".cargo/config.toml",
"line": 5,
"severity": "security",
"message": "The new openapi-gen-build build-dependency (core/Cargo.toml:46) resolves from a private Forgejo sparse registry. Every cargo/nix build (including CI and any future contributor) now requires network access — and possibly credentials — to code.martials.no; if that registry is unavailable or moves, all builds break and there is no crates.io fallback. Consider mirroring the crate to crates.io or vendoring it so builds stay reproducible and portable."
},
{
"file": "core/build.rs",
"line": 8,
"severity": "style",
"message": "Generating into the crate's own source tree (core/src/generated) instead of OUT_DIR is unusual: every cargo command in the workspace now hard-requires Java + openapi-generator-cli on PATH, and the previous graceful fallback (nix run nixpkgs#openapi-generator-cli) from scripts/codegen.sh is gone. It also means the core crate can never build standalone or be published. This is documented in AGENTS.md, but worth confirming the tradeoff is intentional — using include! from OUT_DIR would avoid the source-tree churn."
},
{
"file": "core/build.rs",
"line": 11,
"severity": "bug",
"message": "The placeholder substitution iterates env::vars() (nondeterministic order) over ALL CARGO_* vars using the core crate's values — e.g. {{CARGO_PKG_NAME}} would render as recurring-events-core, and {{CARGO_MANIFEST_DIR}} would point into core/. Only {{CARGO_PKG_VERSION}} is used today, so this is latent, but the logic is also duplicated verbatim with the root build.rs. Consider a shared helper that substitutes only the explicitly-supported CARGO_PKG_* placeholders from the workspace [package]/[workspace.package] values."
},
{
"file": ".woodpecker/verify.yaml",
"line": 23,
"severity": "performance",
"message": "cargo build --workspace --all-targets --locked followed by cargo test --workspace --all-targets --all-features --locked compiles the workspace twice. cargo test builds on its own, so the explicit build step is redundant. Also --all-features pulls in the conversion deps (frunk, itertools, syn 1/2/3) into the lockfile and build even though that feature isn't exercised; scope --all-features to the tests that actually need it if possible."
},
{
"file": "Cargo.toml",
"line": 6,
"severity": "style",
"message": "The version now lives under [workspace.package] (version = \"0.3.3\"). The Justfile release flow (_current_version/write_version, Justfile:65-70) still greps/seds for a top-level ^version = \"...\" line and only works because awk/sed ignore TOML sections — fragile for just release. Update those recipes to read/write the [workspace.package] version explicitly."
},
{
"file": "AGENTS.md",
"line": 93,
"severity": "style",
"message": "Broken sentence introduced by the doc rewrite: 'Any {{CARGO_PKG_<KEY>}} token in the spec is substituted with the' (line 92) is followed by 'The root build.rs renders...' (line 93). Drop the dangling 'the' and merge the two lines."
}
]
}

*🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: comment

Bash appears unavailable; I'll use file tools instead. { "summary": "# Review Summary\n\nSolid refactor: codegen moves into a dedicated `core` crate driven by a build script, CI is consolidated into one step, and `AGENTS.md`/docs are updated to match. The generated-server-as-workspace-member pattern (`core/src/generated`, gitignored, produced by `core/build.rs`) is coherent and the router/import updates in `src/` are consistent.\n\nMain concerns are around supply-chain hygiene and environment consistency rather than core logic:\n\n- **CI downloads a JAR from Maven Central and executes it with only a `test -s` size check** — no checksum verification.\n- **Generator version drift**: CI pins `openapi-generator-cli` 7.24.0, but the Nix devshell and `nix build` use whatever nixpkgs ships. Generated server code can differ between the CI-validated binary and the nix-built image.\n- **New hard dependency on a private Forgejo cargo registry** for the `openapi-gen-build` build-dependency; every `cargo`/`nix` build now requires reachability (and possibly auth) to `code.martials.no`.\n- **Codegen writes into the crate's own source tree** (`core/src/generated`) rather than `OUT_DIR`; every `cargo` command in the workspace now hard-requires Java + the CLI on PATH, and the old `nix run nixpkgs#openapi-generator-cli` fallback from `scripts/codegen.sh` is gone.\n- Minor: the `just release` flow's awk/sed now match `[workspace.package] version` only by coincidence (they ignore TOML sections), a broken sentence in AGENTS.md, and CI builds twice (`cargo build` + `cargo test --all-targets`).\n\nNone of these are blockers on their own, but the checksum pinning and single-source-of-truth for the generator version are worth fixing before merge.", "verdict": "changes", "comments": [ { "file": ".woodpecker/verify.yaml", "line": 19, "severity": "security", "message": "The openapi-generator-cli JAR is downloaded from Maven Central and later executed by `core/build.rs`, but only a `test -s` size check is performed — no checksum verification. Pin and verify a SHA-256 (e.g. `curl ... | sha256sum -c`) so a tampered or truncated artifact fails the build instead of running. Also consider `--max-time` on curl to avoid stalling CI on a hanging download." }, { "file": ".woodpecker/verify.yaml", "line": 17, "severity": "bug", "message": "Generator version drift: CI pins `openapi-generator-cli` 7.24.0, but `flake.nix:64` (devshell) and the `nix build` nativeBuildInputs (`.nix/pkgs.nix:12`) pull whatever version nixpkgs ships. The generated `core/src/generated` code is not committed, so CI-validated output and the nix-built image can differ and produce divergent API behavior. Pin a single generator version across CI, devshell, and the Nix build (e.g. via an overlay or a pinned derivation)." }, { "file": ".cargo/config.toml", "line": 5, "severity": "security", "message": "The new `openapi-gen-build` build-dependency (core/Cargo.toml:46) resolves from a private Forgejo sparse registry. Every cargo/nix build (including CI and any future contributor) now requires network access — and possibly credentials — to `code.martials.no`; if that registry is unavailable or moves, all builds break and there is no crates.io fallback. Consider mirroring the crate to crates.io or vendoring it so builds stay reproducible and portable." }, { "file": "core/build.rs", "line": 8, "severity": "style", "message": "Generating into the crate's own source tree (`core/src/generated`) instead of `OUT_DIR` is unusual: every cargo command in the workspace now hard-requires Java + `openapi-generator-cli` on PATH, and the previous graceful fallback (`nix run nixpkgs#openapi-generator-cli`) from `scripts/codegen.sh` is gone. It also means the `core` crate can never build standalone or be published. This is documented in AGENTS.md, but worth confirming the tradeoff is intentional — using `include!` from `OUT_DIR` would avoid the source-tree churn." }, { "file": "core/build.rs", "line": 11, "severity": "bug", "message": "The placeholder substitution iterates `env::vars()` (nondeterministic order) over ALL `CARGO_*` vars using the *core* crate's values — e.g. `{{CARGO_PKG_NAME}}` would render as `recurring-events-core`, and `{{CARGO_MANIFEST_DIR}}` would point into `core/`. Only `{{CARGO_PKG_VERSION}}` is used today, so this is latent, but the logic is also duplicated verbatim with the root `build.rs`. Consider a shared helper that substitutes only the explicitly-supported `CARGO_PKG_*` placeholders from the workspace `[package]`/`[workspace.package]` values." }, { "file": ".woodpecker/verify.yaml", "line": 23, "severity": "performance", "message": "`cargo build --workspace --all-targets --locked` followed by `cargo test --workspace --all-targets --all-features --locked` compiles the workspace twice. `cargo test` builds on its own, so the explicit build step is redundant. Also `--all-features` pulls in the `conversion` deps (frunk, itertools, syn 1/2/3) into the lockfile and build even though that feature isn't exercised; scope `--all-features` to the tests that actually need it if possible." }, { "file": "Cargo.toml", "line": 6, "severity": "style", "message": "The version now lives under `[workspace.package]` (`version = \"0.3.3\"`). The Justfile release flow (`_current_version`/`write_version`, Justfile:65-70) still greps/seds for a top-level `^version = \"...\"` line and only works because awk/sed ignore TOML sections — fragile for `just release`. Update those recipes to read/write the `[workspace.package]` version explicitly." }, { "file": "AGENTS.md", "line": 93, "severity": "style", "message": "Broken sentence introduced by the doc rewrite: 'Any `{{CARGO_PKG_<KEY>}}` token in the spec is substituted with the' (line 92) is followed by 'The root `build.rs` renders...' (line 93). Drop the dangling 'the' and merge the two lines." } ] } --- *🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: **comment**

martin added 1 commit

2026-08-01 18:11:54 +00:00

Remove duplicate build.rs file

ci/woodpecker/push/verify Pipeline was successful

Details

ci/woodpecker/pr/pr-review Pipeline was successful

Details

dc2afaeb2c

clank-bot commented

2026-08-01 18:14:24 +00:00

Review Summary

This PR refactors code generation so the generated OpenAPI server code moves out of a target/generated/server workspace member into a dedicated core crate (recurring-events-core) whose build.rs runs openapi-gen-build at build time. It also converts the root Cargo.toml into a workspace, replaces the shell scripts/codegen.sh flow, updates CI to install Java + the generator CLI inline, pins my-nix-lib to v0.1.0, and updates docs.

Overall this is a well-executed, cohesive change: AGENTS.md is updated to match reality, the {{CARGO_PKG_VERSION}} substitution is preserved through the core crate's OUT_DIR, generated sources are gitignored and excluded from treefmt, and the flake/CI wiring is documented. The migration itself is sound.

Issues worth addressing before merge:

just release / tag-current / bump-patch are broken by the workspace migration. The Justfile still reads Cargo.toml with awk '/^version = /' and writes with sed 's/^version = ".*"/...', but the root package now uses version.workspace = true and the real version lives indented under [workspace.package]. _current_version returns an empty string, so the release flow tags v and computes a garbage new_version. This is a concrete regression.
The core build script writes generated code into its own package source tree (core/src/generated). Concurrent cargo invocations (cargo build + cargo check/cargo clippy/rust-analyzer) will run the generator into the same directory and can race/interleave files, and cargo package explicitly disallows build scripts modifying the package dir. Writing to a per-target/OUT_DIR location (or guarding with a lock) would be more robust.
CI executes an unverified JAR. verify.yaml downloads openapi-generator-cli-7.24.0.jar via curl and runs it as root with only a non-empty (test -s) check. Pinning and verifying a SHA-256 checksum would remove a supply-chain vector.
Minor: the new build script only emits rerun-if-changed for openapi.yaml; the old one also watched Cargo.toml, which matters for the {{CARGO_PKG_VERSION}} placeholder. Worth adding for robustness.

No runtime/security problems found in the application code paths (src/server.rs, src/service/date_service.rs import updates are mechanical and correct).

*🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: changes

# Review Summary This PR refactors code generation so the generated OpenAPI server code moves out of a `target/generated/server` workspace member into a dedicated `core` crate (`recurring-events-core`) whose `build.rs` runs `openapi-gen-build` at build time. It also converts the root `Cargo.toml` into a workspace, replaces the shell `scripts/codegen.sh` flow, updates CI to install Java + the generator CLI inline, pins `my-nix-lib` to `v0.1.0`, and updates docs. Overall this is a well-executed, cohesive change: AGENTS.md is updated to match reality, the `{{CARGO_PKG_VERSION}}` substitution is preserved through the core crate's OUT_DIR, generated sources are gitignored and excluded from treefmt, and the flake/CI wiring is documented. The migration itself is sound. Issues worth addressing before merge: 1. **`just release` / `tag-current` / `bump-patch` are broken by the workspace migration.** The Justfile still reads `Cargo.toml` with `awk '/^version = /'` and writes with `sed 's/^version = ".*"/...'`, but the root package now uses `version.workspace = true` and the real version lives indented under `[workspace.package]`. `_current_version` returns an empty string, so the release flow tags `v` and computes a garbage `new_version`. This is a concrete regression. 2. **The core build script writes generated code into its own package source tree (`core/src/generated`).** Concurrent cargo invocations (`cargo build` + `cargo check`/`cargo clippy`/rust-analyzer) will run the generator into the same directory and can race/interleave files, and `cargo package` explicitly disallows build scripts modifying the package dir. Writing to a per-target/OUT_DIR location (or guarding with a lock) would be more robust. 3. **CI executes an unverified JAR.** `verify.yaml` downloads `openapi-generator-cli-7.24.0.jar` via curl and runs it as root with only a non-empty (`test -s`) check. Pinning and verifying a SHA-256 checksum would remove a supply-chain vector. 4. Minor: the new build script only emits `rerun-if-changed` for `openapi.yaml`; the old one also watched `Cargo.toml`, which matters for the `{{CARGO_PKG_VERSION}}` placeholder. Worth adding for robustness. No runtime/security problems found in the application code paths (`src/server.rs`, `src/service/date_service.rs` import updates are mechanical and correct). --- *🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: **changes**

clank-bot reviewed

2026-08-01 18:14:24 +00:00

clank-bot left a comment

Code review findings from opencode.

.woodpecker/verify.yaml

					
				@ -28,1 +16,4 @@

				    commands:

				      - version=7.24.0

				      - apt-get update -qq && apt-get install -y -qq default-jre-headless curl

				      - curl -fsSL --retry 3 https://repo1.maven.org/maven2/org/openapitools/openapi-generator-cli/$version/openapi-generator-cli-$version.jar -o /usr/local/bin/openapi-generator-cli.jar

[security] The CI pipeline downloads openapi-generator-cli-7.24.0.jar from Maven Central with curl and executes it as root, but only verifies the file is non-empty (test -s). A tampered or replaced artifact would execute arbitrary code with the CI job's credentials. Pin and verify a known SHA-256 checksum (e.g. via sha256sum -c or an --checksum capable downloader) before running it.

[security] The CI pipeline downloads `openapi-generator-cli-7.24.0.jar` from Maven Central with curl and executes it as root, but only verifies the file is non-empty (`test -s`). A tampered or replaced artifact would execute arbitrary code with the CI job's credentials. Pin and verify a known SHA-256 checksum (e.g. via `sha256sum -c` or an `--checksum` capable downloader) before running it.

Justfile

[bug] The version helpers are now broken by the workspace migration. Root Cargo.toml uses version.workspace = true (line 12) and the actual version lives indented under [workspace.package] (line 6). _current_version runs awk -F'"' '/^version = / {print $2}' which only matches a column-0 version = "..." line, so it returns an empty string; write_version's sed 's/^version = ".*"/...' has the same column-0 anchor. As a result just release, tag-current and bump-patch will tag v and compute a garbage new_version (just release is now a footgun on a fresh checkout of this branch). Update both helpers to read/write the [workspace.package] version entry.

[bug] The version helpers are now broken by the workspace migration. Root `Cargo.toml` uses `version.workspace = true` (line 12) and the actual version lives indented under `[workspace.package]` (line 6). `_current_version` runs `awk -F'"' '/^version = / {print $2}'` which only matches a column-0 `version = "..."` line, so it returns an empty string; `write_version`'s `sed 's/^version = ".*"/...'` has the same column-0 anchor. As a result `just release`, `tag-current` and `bump-patch` will tag `v` and compute a garbage `new_version` (`just release` is now a footgun on a fresh checkout of this branch). Update both helpers to read/write the `[workspace.package] version` entry.

core/build.rs

					
				@ -0,0 +5,4 @@

				    let workspace_root = manifest_dir.join("..");

				    let spec = workspace_root.join("openapi.yaml");

				    let rendered_spec = PathBuf::from(env::var("OUT_DIR").unwrap()).join("openapi.yaml");

				    let generated_dir = manifest_dir.join("src/generated");

[style] The build script writes generated output into its own package source directory (manifest_dir/src/generated). This is a Cargo anti-pattern: (1) concurrent cargo invocations (e.g. cargo build while cargo check/cargo clippy/rust-analyzer run) will invoke the generator into the same directory and can race/interleave files, producing nondeterministic output; (2) cargo package refuses to package crates whose build script modifies the package dir; (3) stale files from a changed spec are not cleaned, so an interrupted generator run can leave a partially-overwritten tree. Consider writing to a target-relative or per-build directory and including via a stable path, or at minimum serialize generation (file lock) and clear the directory before regenerating.

[style] The build script writes generated output into its own package source directory (`manifest_dir/src/generated`). This is a Cargo anti-pattern: (1) concurrent cargo invocations (e.g. `cargo build` while `cargo check`/`cargo clippy`/rust-analyzer run) will invoke the generator into the same directory and can race/interleave files, producing nondeterministic output; (2) `cargo package` refuses to package crates whose build script modifies the package dir; (3) stale files from a changed spec are not cleaned, so an interrupted generator run can leave a partially-overwritten tree. Consider writing to a target-relative or per-build directory and including via a stable path, or at minimum serialize generation (file lock) and clear the directory before regenerating.

core/build.rs

					
				@ -0,0 +23,4 @@

				        .expect("failed to generate OpenAPI server")

				        .emit_rerun_if_changed();

				    println!("cargo:rerun-if-changed={}", spec.display());

[style] The previous root build.rs emitted cargo:rerun-if-changed=Cargo.toml so a version bump would re-render the {{CARGO_PKG_VERSION}} placeholder. The new script only watches openapi.yaml and relies on a version bump incidentally invalidating the crate fingerprint. Since AGENTS.md documents that editing the workspace version is the only step needed to bump the API version, emit println!("cargo:rerun-if-changed={}", workspace_root.join("Cargo.toml").display()) to make regeneration robust and explicit.

[style] The previous root `build.rs` emitted `cargo:rerun-if-changed=Cargo.toml` so a version bump would re-render the `{{CARGO_PKG_VERSION}}` placeholder. The new script only watches `openapi.yaml` and relies on a version bump incidentally invalidating the crate fingerprint. Since AGENTS.md documents that editing the workspace version is the only step needed to bump the API version, emit `println!("cargo:rerun-if-changed={}", workspace_root.join("Cargo.toml").display())` to make regeneration robust and explicit.

martin marked this conversation as resolved

martin added 1 commit

2026-08-01 21:40:33 +00:00

fix: Regenerate code when Crate.toml changes

ci/woodpecker/push/verify Pipeline was successful

Details

ci/woodpecker/pr/pr-review Pipeline was successful

Details

4f1d257009

clank-bot commented

2026-08-01 21:43:42 +00:00

Review Summary

The PR moves OpenAPI code generation from a standalone scripts/codegen.sh + committed generated crate under target/ into a build-time step: a new core workspace member (recurring-events-core) whose build.rs invokes openapi-gen-build (fetched from a private Forgejo registry) and writes generated modules into core/src/generated. Application code is updated to import from recurring_events_core::generated::contract::*, the root build.rs is deleted, CI is consolidated into a single step that installs Java/CLI before cargo build/test/clippy, and the Nix package drops its preBuild codegen hook. AGENTS.md and treefmt are updated consistently.

Overall this is a clean, well-executed refactor: generated code is properly gitignored, excluded from treefmt, workspace versioning is shared via [workspace.package], and the Renovate circular-dependency bootstrap hack disappears because core/Cargo.toml is committed with explicit deps. The cargo:rerun-if-changed wiring for the spec and Cargo.toml is correct.

The main blocker is the release tooling in Justfile: the _current_version/write_version/tag-current/bump-patch recipes still parse version = "..." from the root Cargo.toml, but this PR changes it to version.workspace = true, so just release would create a tag literally named v and bump-patch would no-op/fail. Secondary points: the generated-code-into-source-tree approach is fragile for read-only/sandboxed builds, the CI generator jar is downloaded without a checksum pin, CARGO_PKG_* placeholder substitution now resolves against the core package (not the root), the whole build now depends on a private registry, and clippy -D warnings runs over generated code with only a small allow-list.

*🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: changes

# Review Summary The PR moves OpenAPI code generation from a standalone `scripts/codegen.sh` + committed generated crate under `target/` into a build-time step: a new `core` workspace member (`recurring-events-core`) whose `build.rs` invokes `openapi-gen-build` (fetched from a private Forgejo registry) and writes generated modules into `core/src/generated`. Application code is updated to import from `recurring_events_core::generated::contract::*`, the root `build.rs` is deleted, CI is consolidated into a single step that installs Java/CLI before `cargo build/test/clippy`, and the Nix package drops its `preBuild` codegen hook. AGENTS.md and treefmt are updated consistently. Overall this is a clean, well-executed refactor: generated code is properly gitignored, excluded from treefmt, workspace versioning is shared via `[workspace.package]`, and the Renovate circular-dependency bootstrap hack disappears because `core/Cargo.toml` is committed with explicit deps. The `cargo:rerun-if-changed` wiring for the spec and Cargo.toml is correct. The main blocker is the release tooling in `Justfile`: the `_current_version`/`write_version`/`tag-current`/`bump-patch` recipes still parse `version = "..."` from the root `Cargo.toml`, but this PR changes it to `version.workspace = true`, so `just release` would create a tag literally named `v` and `bump-patch` would no-op/fail. Secondary points: the generated-code-into-source-tree approach is fragile for read-only/sandboxed builds, the CI generator jar is downloaded without a checksum pin, `CARGO_PKG_*` placeholder substitution now resolves against the `core` package (not the root), the whole build now depends on a private registry, and clippy `-D warnings` runs over generated code with only a small allow-list. --- *🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: **changes**

clank-bot reviewed

2026-08-01 21:43:42 +00:00

clank-bot left a comment

Code review findings from opencode.

.cargo/config.toml

					
				@ -0,0 +2,4 @@

				git-fetch-with-cli = true

				[registries.forgejo]

				index = "sparse+https://code.martials.no/api/packages/Personal/cargo/"

[style] The build now hard-depends on openapi-gen-build from a private Forgejo registry: both CI (cargo build --locked) and nix build must reach code.martials.no to resolve dependencies (and the git-fetch-with-cli = true setting requires a git CLI in every build environment). If that server is unreachable, nothing builds. Worth confirming the Nix build sandbox has network access to the registry and documenting the availability expectation, e.g. in AGENTS.md.

[style] The build now hard-depends on `openapi-gen-build` from a private Forgejo registry: both CI (`cargo build --locked`) and `nix build` must reach `code.martials.no` to resolve dependencies (and the `git-fetch-with-cli = true` setting requires a git CLI in every build environment). If that server is unreachable, nothing builds. Worth confirming the Nix build sandbox has network access to the registry and documenting the availability expectation, e.g. in AGENTS.md.

.woodpecker/verify.yaml

					
				@ -28,1 +16,4 @@

				    commands:

				      - version=7.24.0

				      - apt-get update -qq && apt-get install -y -qq default-jre-headless curl

				      - curl -fsSL --retry 3 https://repo1.maven.org/maven2/org/openapitools/openapi-generator-cli/$version/openapi-generator-cli-$version.jar -o /usr/local/bin/openapi-generator-cli.jar

[security] The openapi-generator-cli jar is downloaded over HTTPS but its SHA-256 is not pinned; test -s only checks that the file is non-empty. This is a supply-chain risk on the build tooling. Consider pinning the expected checksum (and verifying with e.g. sha256sum -c) and ideally caching the jar.

[security] The openapi-generator-cli jar is downloaded over HTTPS but its SHA-256 is not pinned; `test -s` only checks that the file is non-empty. This is a supply-chain risk on the build tooling. Consider pinning the expected checksum (and verifying with e.g. `sha256sum -c`) and ideally caching the jar.

.woodpecker/verify.yaml

					
				@ -29,2 +23,4 @@

				      - cargo build --workspace --all-targets --locked

				      - cargo test --workspace --all-targets --all-features --locked

				      - rustup component add clippy

				      - cargo clippy --workspace --all-targets --locked -- -D warnings

[style] cargo clippy --workspace --all-targets -- -D warnings also lints the generated core/src/generated modules, which are only covered by a small crate-level allow-list (non_camel_case_types, unused_imports, unused_mut). If the generator output ever trips any clippy lint (e.g. too_many_arguments, needless_borrow), CI will break for reasons unrelated to the app code. Consider adding #![allow(clippy::all)] in core/src/lib.rs or scoping clippy to non-generated code.

[style] `cargo clippy --workspace --all-targets -- -D warnings` also lints the generated `core/src/generated` modules, which are only covered by a small crate-level allow-list (`non_camel_case_types`, `unused_imports`, `unused_mut`). If the generator output ever trips any clippy lint (e.g. `too_many_arguments`, `needless_borrow`), CI will break for reasons unrelated to the app code. Consider adding `#![allow(clippy::all)]` in `core/src/lib.rs` or scoping clippy to non-generated code.

Justfile

[bug] This PR changes the root package version to version.workspace = true (Cargo.toml line 12), but the release recipes still parse version = "...". _current_version (awk /^version = /) now matches nothing, so just tag-current builds tag="v${version}" = v and would create/push a tag literally named v; bump-patch then errors on the empty major.minor.patch split, and write_version's sed pattern no longer matches anything, so the commit no-ops. just release is broken by this PR. Update these recipes to read/write the version from the [workspace.package] table.

[bug] This PR changes the root package version to `version.workspace = true` (Cargo.toml line 12), but the release recipes still parse `version = "..."`. `_current_version` (awk `/^version = /`) now matches nothing, so `just tag-current` builds `tag="v${version}"` = `v` and would create/push a tag literally named `v`; `bump-patch` then errors on the empty `major.minor.patch` split, and `write_version`'s sed pattern no longer matches anything, so the commit no-ops. `just release` is broken by this PR. Update these recipes to read/write the version from the `[workspace.package]` table.

core/build.rs

					
				@ -0,0 +5,4 @@

				    let workspace_root = manifest_dir.join("..");

				    let spec = workspace_root.join("openapi.yaml");

				    let rendered_spec = PathBuf::from(env::var("OUT_DIR").unwrap()).join("openapi.yaml");

				    let generated_dir = manifest_dir.join("src/generated");

[style] The build script writes generated code into the source tree (core/src/generated). This requires the source directory to be writable at build time, which is fragile for read-only checkouts/sandboxed builds and leaves build artifacts in the working tree. It happens to work for the current nixpkgs/Crane unpack flow, but a future change (e.g. vendoring, read-only CI cache, SOURCE_DATE_EPOCH) would silently break cargo build. Consider emitting into OUT_DIR and wiring via include!, or at least documenting the writable-source requirement.

[style] The build script writes generated code into the source tree (`core/src/generated`). This requires the source directory to be writable at build time, which is fragile for read-only checkouts/sandboxed builds and leaves build artifacts in the working tree. It happens to work for the current nixpkgs/Crane unpack flow, but a future change (e.g. vendoring, read-only CI cache, `SOURCE_DATE_EPOCH`) would silently break `cargo build`. Consider emitting into `OUT_DIR` and wiring via `include!`, or at least documenting the writable-source requirement.

core/build.rs

					
				@ -0,0 +8,4 @@

				    let generated_dir = manifest_dir.join("src/generated");

				    let mut content = fs::read_to_string(&spec).expect("openapi.yaml missing");

				    for (name, value) in env::vars() {

[style] {{CARGO_PKG_*}} substitution now runs in the core crate's build script, so CARGO_PKG_NAME/CARGO_PKG_DESCRIPTION resolve to recurring-events-core rather than the root recurring-events package. Only {{CARGO_PKG_VERSION}} is currently used (workspace version matches, so no breakage), but this is a latent footgun for future spec edits — worth a comment or a pinned substitution of the root package values.

[style] `{{CARGO_PKG_*}}` substitution now runs in the `core` crate's build script, so `CARGO_PKG_NAME`/`CARGO_PKG_DESCRIPTION` resolve to `recurring-events-core` rather than the root `recurring-events` package. Only `{{CARGO_PKG_VERSION}}` is currently used (workspace version matches, so no breakage), but this is a latent footgun for future spec edits — worth a comment or a pinned substitution of the root package values.