feat(nuke): IDomainConventionsApi build + AGENTS.md#18
Conversation
Scaffold a Nuke 10 build host under build/ that overrides every target on
IDomainConventionsApi from the locally-built Nuke.OpenTelemetry.Conventions
0.1.0 package. Each target maps to either an existing npm script
(lint:public, lint, npm ci, npm pack) or the lockstep / determinism /
no-manual-edits policy this repo owns.
- VerifyKeysLockstep parses package-lock.json for the upstream keys
package, asserts an exact pin against OtelKeysVersion, and byte-diffs
generated/otel-keys.gen.tsp against the installed lib/otel-keys.gen.tsp.
Skips with a warning when the package isn't installed yet (lockstep
flip pending), per the contribution plan.
- VerifyEmitDeterministic runs every emitter twice into
Artifacts/emit-{a,b} and byte-compares. `lint` is excluded because
it's a TypeSpec library (no $onEmit), not an emitter.
- VerifyNoManualEditsToGenerated runs `git diff --exit-code generated/`
*and* `git status --porcelain generated/`, closing the Codex
untracked-file gap.
- PublishApiPackage mirrors publish.yml's dist-tag resolution
(NPM_DIST_TAG validated against ^[A-Za-z0-9._-]+$, default `latest`).
- Top-level `Check` target chains restore -> lockstep -> compile ->
no-manual-edits -> emit-all -> determinism -> pack.
Build host targets net10.0, references Nuke.Common 10.1.0 plus the
local Nuke.OpenTelemetry.Conventions 0.1.0 .nupkg via build/NuGet.config.
Adds the same NuGet.Packaging + System.Security.Cryptography.Xml
security overrides as the component package so NuGetAudit stays silent.
Pins the .NET SDK to 10.0.203 via build/global.json and nuke.global.json.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a GitHub Actions workflow that runs the end-to-end IDomainConventionsApi gate (`./build.sh Check`) on every PR and push to main. The workflow complements (does not replace) publish.yml: - publish.yml stays npm-direct so the trusted-publishing OIDC flow keeps working unchanged. - nuke.yml provides the local-build verification gate: restore -> lockstep -> compile -> no-manual-edits -> emit-all -> determinism -> pack, exercising the Nuke build host on a clean checkout. Uses actions/checkout@v6, actions/setup-node@v6 (Node 22), and actions/setup-dotnet@v4 (10.0.x), matching the toolchain pinned in build/global.json. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- AGENTS.md documents agent conventions for the repo: tarball boundary, lint:public regression gate, generated/ read-only, peer-dep policy, publishing flow, and the planned IDomainConventionsApi wiring. - build/NuGet.config: add the O-ANcppLua GitHub Packages NuGet feed (https://nuget.pkg.github.com/O-ANcppLua/index.json) as the primary source for Nuke.OpenTelemetry.Conventions, with the local sibling checkout path kept as offline-dev fallback. Credentials read from %GITHUB_ACTOR% + %GITHUB_PACKAGES_TOKEN%. - nuke.yml: grant packages:read, pass GITHUB_ACTOR + GITHUB_TOKEN to the job env so the NuGet.config credential section resolves on CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
NU1301 fires when a configured packageSource path doesn't exist, even
if other sources resolve the package fine. The local fallback only
made sense for offline dev next to an uncommitted Nuke.OpenTelemetry
.Conventions checkout; CI runners never had that path, so it broke
restore.
Local dev now opts in explicitly via
dotnet nuget add source ../Nuke.OpenTelemetry.Conventions/src/.../bin/Release \
--name local-nuke-otel-conventions --configfile build/NuGet.config
(documented in the file's comment header). The org GitHub Packages
feed remains the only committed source for the shared Nuke component.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
XML comments cannot contain double-hyphens. The embedded `dotnet nuget add source` example included `--name`/`--configfile` flags inside the comment, which made the file invalid XML and broke restore with "NuGet.Config is not valid XML." Rewrote the comment to describe the opt-in path in prose instead of pasting the literal command.
There was a problem hiding this comment.
Pull request overview
Risk summary: Medium — new build/CI wiring is substantial and includes at least one likely cross-platform build-break in the git pathspec usage.
This PR wires the repo into the new producer/consumer split by adding a Nuke 10 build host implementing IDomainConventionsApi, plus a CI workflow and agent-facing documentation.
Changes:
- Add a Nuke build host (
build/_build.csproj,build/Build.cs) implementingIDomainConventionsApitargets and aCheckaggregate. - Add CI workflow to run
./build.sh Checkand add Nuke bootstrap/config files (build.sh,build.cmd,.nuke/*,build/NuGet.config). - Add
AGENTS.mdguidance and update.gitignorefor Nuke/Artifacts outputs.
Blockers
VerifyNoManualEditsToGeneratedpasses an absolute path togit diff/git status, which commonly fails because git pathspecs are repo-relative (likely to break theCheckgate on clean environments).
Important issues
- SDK pinning is currently inconsistent/ineffective for local runs:
build.shruns from repo root, sobuild/global.jsonwon’t be applied, andnuke.global.jsonis not used bydotnet. AGENTS.mdstill describes Nuke/IDomainConventionsApias “planned” even though this PR lands it, which will mislead agents.
Minor / optional
- None noted.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| nuke.global.json | Attempts to pin .NET SDK for Nuke, but currently not applied by dotnet. |
| build/NuGet.config | Adds GitHub Packages NuGet source + local fallback for restoring Nuke shared component. |
| build/global.json | Pins .NET SDK under build/, but won’t affect build.sh as written. |
| build/Build.cs | Implements IDomainConventionsApi targets (restore/compile/emit/determinism/pack/publish) and adds Check. |
| build/_build.csproj | Adds net10.0 Nuke build host referencing Nuke.OpenTelemetry.Conventions. |
| build.sh | Bash bootstrap to run the build host. |
| build.cmd | Windows bootstrap to run the build host. |
| AGENTS.md | Adds agent guidance on surfaces, lint gates, tarball boundary, publishing, and Nuke usage. |
| .nuke/parameters.json | Default Nuke parameters for emitters/lockstep behavior. |
| .nuke/build.schema.json | Generated schema for Nuke parameters/targets. |
| .gitignore | Ignores Nuke build outputs, Artifacts emit output, and npm pack tarballs. |
| .github/workflows/nuke.yml | Adds CI job to run ./build.sh Check on PR/push. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Tracked-diff guard: any modified tracked file under generated/. | ||
| Git($"diff --exit-code -- \"{generated}\"", workingDirectory: self.DomainSpecRoot); | ||
|
|
||
| // Untracked-file guard: porcelain output for generated/ must be empty. | ||
| var porcelain = Git($"status --porcelain -- \"{generated}\"", workingDirectory: self.DomainSpecRoot, logOutput: false); |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| cd "$SCRIPT_DIR" | ||
|
|
||
| dotnet run --project ./build/_build.csproj -- "$@" |
| { | ||
| "sdk": { | ||
| "version": "10.0.203", | ||
| "rollForward": "latestFeature" | ||
| } | ||
| } |
| ## Nuke (planned) | ||
|
|
||
| The `Nuke.OpenTelemetry.Conventions` shared package will land an `IDomainConventionsApi` component that this repo's `Build.cs` will implement. Targets: `RestoreTypeSpecDeps`, `VerifyKeysLockstep` (assert `generated/otel-keys.gen.tsp` matches the locked `@ancplua/typespec-otel-semconv` version), `CompileDomainSpec`, `EmitAll`, `VerifyEmitDeterministic`, `BuildCSharpEmit`, `VerifyNoManualEditsToGenerated`, `PackApiPackage`, `PublishApiPackage`. Until that lands, the npm scripts above are the source of truth. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebfb19037c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| Target IDomainConventionsApi.EmitCSharp => _ => _ | ||
| .Description("Run the C# TypeSpec emitter into Artifacts/emit/csharp.") | ||
| .DependsOn(((IDomainConventionsApi)this).CompileDomainSpec) | ||
| .Executes(() => RunTypeSpecEmitter("@ancplua/typespec-emit-csharp", "csharp")); |
There was a problem hiding this comment.
EmitCSharp (and the sibling emitter targets) only depend on CompileDomainSpec, which runs lint:public and does not compile local emitters. On a clean checkout, those packages resolve to file: links and their lib/main.tsp files import ../dist/index.js, so running EmitCSharp/EmitDuckDb/EmitTsTypes before npm run build:emitters fails. This also makes EmitAll fragile because LintConventions (the only target that runs npm run lint, which builds emitters) is a sibling dependency and not an ordering dependency.
Useful? React with 👍 / 👎.
| Target IDomainConventionsApi.EmitCSharp => _ => _ | ||
| .Description("Run the C# TypeSpec emitter into Artifacts/emit/csharp.") | ||
| .DependsOn(((IDomainConventionsApi)this).CompileDomainSpec) | ||
| .Executes(() => RunTypeSpecEmitter("@ancplua/typespec-emit-csharp", "csharp")); |
There was a problem hiding this comment.
EmitCSharp/EmitDuckDb/EmitTsTypes depend only on CompileDomainSpec, which runs lint:public and does not compile local emitter JS. These emitters are file: packages whose lib/main.tsp imports ../dist/index.js, so invoking any Emit* target on a clean checkout can fail before artifacts are generated. This breaks common workflows such as running ./build.sh EmitCSharp directly and makes the target graph fragile.
Useful? React with 👍 / 👎.
| .DependsOn( | ||
| ((IDomainConventionsApi)this).RestoreTypeSpecDeps, | ||
| ((IDomainConventionsApi)this).VerifyKeysLockstep, | ||
| ((IDomainConventionsApi)this).CompileDomainSpec, | ||
| ((IDomainConventionsApi)this).VerifyNoManualEditsToGenerated, | ||
| ((IDomainConventionsApi)this).EmitAll, | ||
| ((IDomainConventionsApi)this).VerifyEmitDeterministic, | ||
| ((IDomainConventionsApi)this).PackApiPackage); |
There was a problem hiding this comment.
Check describes a linear gate (emit-all -> determinism -> pack) but wires all targets as sibling dependencies. In NUKE, DependsOn does not impose list order and execution between siblings is nondeterministic/parallel, so VerifyEmitDeterministic can run before EmitAll has completed its lint/build side effects, causing flaky failures on clean environments. Add explicit ordering edges (Before/After) or explicit prerequisites so the declared pipeline order is guaranteed.
Useful? React with 👍 / 👎.
| Target IDomainConventionsApi.EmitCSharp => _ => _ | ||
| .Description("Run the C# TypeSpec emitter into Artifacts/emit/csharp.") | ||
| .DependsOn(((IDomainConventionsApi)this).CompileDomainSpec) | ||
| .Executes(() => RunTypeSpecEmitter("@ancplua/typespec-emit-csharp", "csharp")); |
There was a problem hiding this comment.
EmitCSharp/EmitDuckDb/EmitTsTypes depend only on CompileDomainSpec, which runs lint:public and does not compile local emitter JS. These emitters are file: packages whose lib/main.tsp imports ../dist/index.js, so invoking any Emit* target on a clean checkout can fail before artifacts are generated. This breaks direct target runs like ./build.sh EmitCSharp and makes emitter execution fragile.
Useful? React with 👍 / 👎.
| .DependsOn( | ||
| ((IDomainConventionsApi)this).RestoreTypeSpecDeps, | ||
| ((IDomainConventionsApi)this).VerifyKeysLockstep, | ||
| ((IDomainConventionsApi)this).CompileDomainSpec, | ||
| ((IDomainConventionsApi)this).VerifyNoManualEditsToGenerated, | ||
| ((IDomainConventionsApi)this).EmitAll, | ||
| ((IDomainConventionsApi)this).VerifyEmitDeterministic, | ||
| ((IDomainConventionsApi)this).PackApiPackage); |
There was a problem hiding this comment.
Check is described as a linear gate (... -> emit-all -> determinism -> pack) but wires these as sibling DependsOn edges. In NUKE, dependency list position does not define execution order, so sibling targets can run in nondeterministic order/parallel. That allows VerifyEmitDeterministic to run before EmitAll completes its prerequisite side effects on clean environments, creating flaky or misleading failures.
Useful? React with 👍 / 👎.
| if (!File.Exists(installed)) | ||
| { | ||
| Log.Warning( | ||
| "VerifyKeysLockstep: '{Installed}' not found — skipping byte diff with '{Committed}'. " + | ||
| "Once the upstream package is installed this comparison will be enforced.", | ||
| installed, committed); | ||
| return; |
There was a problem hiding this comment.
When the upstream package is present in package-lock.json but node_modules/{pkg}/lib/otel-keys.gen.tsp is missing, VerifyKeysLockstep only logs a warning and returns success. In that scenario, the lockstep check is silently bypassed even though the package layout is invalid for this repo’s contract, so CI can pass without actually verifying generated keys consistency.
Useful? React with 👍 / 👎.
Summary
Wires this repo into the producer/consumer split: a Nuke 10 build host implementing
IDomainConventionsApifrom the freshly-publishedNuke.OpenTelemetry.Conventions@0.1.0shared component, plus anAGENTS.mdand the matching CI workflow.What's in this PR
Nuke build host (
build/)build/_build.csproj— net10.0, referencesNuke.Common 10.1.0+Nuke.OpenTelemetry.Conventions 0.1.0(resolved from the org GitHub Packages NuGet feed)build/Build.cs— overrides every target onIDomainConventionsApivia explicit-interface implementation; adds a top-levelCheckaggregate (RestoreTypeSpecDeps → VerifyKeysLockstep → CompileDomainSpec → VerifyNoManualEditsToGenerated → EmitAll → VerifyEmitDeterministic → PackApiPackage)build/NuGet.config— points athttps://nuget.pkg.github.com/O-ANcppLua/index.jsonas the primary source, with the local sibling checkout path kept as offline-dev fallbackbuild.sh/build.cmd/.nuke/— standard Nuke bootstrapCI
.github/workflows/nuke.yml— runs./build.sh Checkon PR/push to main. Grantspackages: read, passesGITHUB_ACTOR+GITHUB_TOKENso the GitHub Packages NuGet feed credentials resolve.Docs
AGENTS.md— documents tarball boundary (main.tspvsindex.tsp),lint:publicregression gate,generated/read-only policy, exports-map rules, peer-dep policy, publishing flow, plannedIDomainConventionsApiwiring, and "what not to do" list.Notable design choices
EmitOutputDirdefaults toArtifacts/emit/, not./emittersemitters/directory holds emitter source code — using it as the output dir would shadow source. The interface default (./emitters) is overridden via doc-commented<inheritdoc>.LintConventionsrunsnpm run lint, nottsp compile --emit ...@ancplua/typespec-otelconventions-lintis a TypeSpec library (decorators + diagnostics), not an emitter — it has no$onEmit, so the interface's literal command errors withinvalid-emitter.npm run lint(full-surface compile with--warn-as-error) is what actually exercises the linter.lintexcluded fromVerifyEmitDeterministicbyte-diffnuke.ymlis independent ofpublish.ymlpublish.ymlstays small + npm-direct so trusted-publishing OIDC keeps working unchanged.nuke.ymlis the local-build verification gate.Verification
All five gates pass locally on
feat/nuke-domain-conventions:dotnet restore build/_build.csproj✅dotnet build build/_build.csproj -c Release✅ (0 errors, 0 warnings)./build.sh --helplists all 13 targets ✅./build.sh Checkruns end-to-end ✅.VerifyKeysLockstepcorrectly skips with a warning (lockstep flip pending).VerifyEmitDeterministicconfirms byte-identical output across two emit runs.VerifyNoManualEditsToGenerated,CompileDomainSpec,EmitAll,PackApiPackageall green.git statusclean.Test plan
Nuke build verificationworkflow goes green on this PRCodeRabbitreview surfaces no blocker on the Nuke wiringPublish to GitHub Packagesworkflow remains green (unchanged by this PR)Related
Nuke.OpenTelemetry.Conventions@0.1.0shipped at https://github.com/O-ANcppLua/Nuke.OpenTelemetry.Conventions/releases/tag/v0.1.0@o-ancpplua/otel-conventions-api@0.1.0shipped at https://github.com/O-ANcppLua/ANcpLua.OtelConventions.Api/releases/tag/v0.1.0ANcpLua/typespec-otel-semconvwill unblockVerifyKeysLockstep(currently warns/skips).🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need.