Add C# bridge and generated SDK - #4074
Draft
rossirpaulo wants to merge 3 commits into
Draft
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (4)
baml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlHttpRequest.cs (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using case-insensitive header keys.
HTTP header keys are fundamentally case-insensitive according to HTTP standards (RFC 2616). Using
StringComparer.Ordinalforces exact-case lookups (e.g. checkingHeaders["Content-Type"]will throw aKeyNotFoundExceptionif it was mapped as"content-type").Consider using
StringComparer.OrdinalIgnoreCaseto align with the standard behavior of .NET'sHttpHeaders, assuming the underlying BAML runtime logic does not strictly demand exact-case string matching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlHttpRequest.cs` at line 21, Update the headers dictionary initialization in BamlHttpRequest to use StringComparer.OrdinalIgnoreCase instead of StringComparer.Ordinal, ensuring header lookups remain case-insensitive while preserving the existing copy behavior.baml_language/sdk_tests/crates/csharp/csharp_csv/customizable/Program.cs (1)
82-93: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert every configured reader option.
The “round trip exactly” check omits
SkipBlankRecords,Encoding,Bom,OnError, andMaxSkipped, allowing those fields to be dropped by the codec without failing this fixture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdk_tests/crates/csharp/csharp_csv/customizable/Program.cs` around lines 82 - 93, Extend the validation in the RoundTripCsvReaderOptions check to assert the configured SkipBlankRecords, Encoding, Bom, OnError, and MaxSkipped values alongside the existing reader options. Keep the exact-round-trip failure behavior and use the corresponding fields on returnedReaderOptions.baml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlClient.cs (1)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
ArgumentNullException.ThrowIfNull.For consistency with modern C# practices and your usage of
ArgumentException.ThrowIfNullOrWhiteSpaceelsewhere, consider usingArgumentNullException.ThrowIfNull(name)instead of the null-coalescing throw expression.♻️ Proposed refactor
- Name = name ?? throw new ArgumentNullException(nameof(name)); + ArgumentNullException.ThrowIfNull(name); + Name = name;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlClient.cs` at line 46, Update the BamlClient constructor’s name validation to call ArgumentNullException.ThrowIfNull(name) before assigning Name, replacing the null-coalescing throw expression while preserving the existing argument validation behavior.TASK/state-of-csharp-completeness.md (1)
61-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEscape the pipe inside the code span to prevent table parsing issues.
The unescaped pipe
|within the inline code block`int | RecursiveNumbers[]`is parsed as a column separator by some Markdown parsers (and triggersmarkdownlintMD056), causing the table to have an incorrect column count.♻️ Proposed refactor
-| Recursive type alias | yes | supported | generated nominal wrapper over recursive `BamlUnion` | `csharp_glob::dotnet` | P6 | Nested `int | RecursiveNumbers[]` passes native parity; erased outputs must match one structural arm, ambiguous shapes are rejected, and nullable recursive aliases retain a nominal wrapper. | +| Recursive type alias | yes | supported | generated nominal wrapper over recursive `BamlUnion` | `csharp_glob::dotnet` | P6 | Nested `int \| RecursiveNumbers[]` passes native parity; erased outputs must match one structural arm, ambiguous shapes are rejected, and nullable recursive aliases retain a nominal wrapper. |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TASK/state-of-csharp-completeness.md` at line 61, Escape the pipe character in the inline code example within the “Recursive type alias” table row so Markdown parsers treat it as code content rather than a column separator, while preserving the example’s meaning and the table’s column count.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build2-csharp-sdk.reusable.yaml:
- Line 91: Update the checksum generation command in the C# package artifact
workflow to write paths relative to the uploaded artifact root, removing the
artifacts/csharp-package/ prefix from SHA256SUMS. Apply the same path handling
to the corresponding checksum-generation commands around the additionally
referenced lines so sha256sum --check works after artifact download.
- Around line 91-97: Update the provenance-generation step around the jq --arg
source_sha usage to pass inputs.source_sha through the workflow step’s env
configuration, then reference the resulting shell environment variable in Bash
instead of embedding the GitHub template expression directly. Keep the existing
version, package size, and provenance output unchanged.
In `@baml_language/crates/baml_cli/src/generate.rs`:
- Around line 424-443: The rollback handling around the transaction error must
track failures from removing, renaming, and directory-creation operations
instead of discarding them. In the rollback branch, only remove the staging
directory when every restoration succeeds; otherwise preserve it, return an
error indicating rollback was incomplete, and retain the original error context.
Add a fault-injection test covering a restoration failure and verifying the
staging backups remain available.
In
`@baml_language/sdk_tests/crates/csharp/csharp_resources/customizable/Program.cs`:
- Around line 80-99: Bound the header-reading loop in
FetchUrlAsync/ServeResponse by creating or reusing the timeout cancellation
token established near the existing timeout setup and passing it to each
stream.ReadAsync call. Ensure a client that never sends the "\r\n\r\n" header
terminator causes the read to cancel when the timeout expires rather than
waiting indefinitely, while preserving the current disconnect exception for a
zero-byte read.
In
`@baml_language/sdk_tests/crates/csharp/primitive_calls/customizable/Program.cs`:
- Around line 402-429: Update AssertHardExit in
baml_language/sdk_tests/crates/csharp/primitive_calls/customizable/Program.cs:402-429
to wait with a bounded timeout, then kill and reap the exit-probe process if it
does not exit. In
baml_language/sdk_tests/crates/csharp/llm_functions/customizable/Program.cs:39-51
and :262-301, ensure WaitAsync timeouts or startup failures kill the relevant
process tree and that cleanup executes unconditionally.
In `@baml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlMedia.cs`:
- Around line 40-44: Update the static Create method to validate the
non-nullable value argument before calling NativeApi.CreateMedia, throwing the
established managed argument exception for null inputs and preserving the
existing native call for valid values.
In
`@baml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Bridge/BridgeVersion.cs`:
- Around line 7-12: Update the BridgeVersion.Current metadata lookup to use
SingleOrDefault before accessing Value, allowing missing BamlSdkVersion metadata
to produce the existing BamlBridgeException. Preserve the current behavior for
valid metadata and avoid changing unrelated assembly-version handling.
In
`@baml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Bridge/HostValueRegistry.cs`:
- Around line 220-225: Update the emergency completion in HostValueRegistry’s
nested catch around NativeApi.CompleteHostCall so it passes isError: true when
exception encoding fails. Preserve the empty payload fallback while ensuring the
failed host call remains on the error channel.
In `@baml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Bridge/NativeApi.cs`:
- Around line 398-431: Restrict DevelopmentCandidates to explicit development
opt-in paths and stop traversing Environment.CurrentDirectory or its ancestors
during normal package loading. Update the native library load flow around
DevelopmentCandidates so production cannot discover target/debug or
target/release binaries from attacker-controlled working-directory trees, while
preserving intentional development probing when the opt-in is enabled.
In
`@baml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Bridge/ProtoCodec.cs`:
- Around line 2033-2036: Update the class-valued union validation around the
BamlTy.TyOneofCase.ClassTy branch and its corresponding logic near the
additional referenced range so all codec-supported resource classes, including
BamlGlob, BamlGlobScanOptions, BamlCancelToken, and BamlTaskGroup, are
recognized. Reuse ProtoTypeCodec.Encode(targetType) as the shared descriptor
source or extend BuiltinClassName with these mappings, preserving existing
generated-class validation.
- Around line 1249-1258: Update DecodeValue and the related list/map/class
construction paths to use a shared decode context that tracks every handle
decoded during graph construction. If any child decoding or construction step
throws, dispose all handles owned by that context before propagating the
exception; transfer ownership only after successful construction so the existing
outer cleanup remains correct.
In
`@baml_language/sdks/csharp/bridge_csharp/tools/Baml.NuGet.Normalize/Program.cs`:
- Around line 18-20: Update the output handling around inputPath and outputPath
to reject existing output targets, including case-insensitive Windows aliases
and filesystem links that could refer to the input. Create the output package
exclusively so pre-existing files are never truncated, and track whether this
invocation successfully created it. In the failure cleanup path, delete only
that newly created output, never a pre-existing path or the input package.
In `@baml_language/sdks/csharp/sdkgen_csharp/src/lib.rs`:
- Around line 230-247: The csharp_string function must escape Unicode line
separators U+2028 and U+2029 so generated C# literals remain valid. Add explicit
handling for both characters using their Unicode escape sequences, and add
crate-local unit coverage verifying each is escaped.
In `@baml_language/sdks/csharp/sdkgen_csharp/src/models.rs`:
- Around line 103-110: Update the generated equality and hashing expressions in
the model-generation code around the `Equals` implementations and `GetHashCode`
output to use `EqualityComparer<T>.Default`, where T is the alias value type.
Ensure both comparisons and hash generation remain null-safe when `Value` is
nullable, while preserving the existing nullable `other` check and generated API
shape.
- Around line 325-333: Adjust the support decision in the method-generation
logic around is_static, class_supported, and supported so static methods are not
gated by class_supported or instance-property codec support. Preserve the
existing class_supported validation for instance methods, while allowing
otherwise supported static methods to be emitted normally instead of as
NotSupportedException.
In `@baml_language/sdks/csharp/sdkgen_csharp/src/routing.rs`:
- Around line 235-269: Update file_segment to escape reserved Windows device
names with a prefix that cannot occur in a valid C# identifier, avoiding
collisions with projected names such as _Con. Adjust
keeps_windows_device_names_out_of_generated_paths and add direct assertions for
transformed reserved and non-reserved segments, including that distinct inputs
produce distinct paths.
In `@TASK/codex/implementation-notes.md`:
- Around line 448-453: Update the question-9 coverage status in the
implementation notes to remove trimming/NativeAOT from the outstanding items,
while preserving the remaining supported-host matrix and version-skew work. Keep
the explicit v1 non-goal statements elsewhere unchanged.
---
Nitpick comments:
In `@baml_language/sdk_tests/crates/csharp/csharp_csv/customizable/Program.cs`:
- Around line 82-93: Extend the validation in the RoundTripCsvReaderOptions
check to assert the configured SkipBlankRecords, Encoding, Bom, OnError, and
MaxSkipped values alongside the existing reader options. Keep the
exact-round-trip failure behavior and use the corresponding fields on
returnedReaderOptions.
In `@baml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlClient.cs`:
- Line 46: Update the BamlClient constructor’s name validation to call
ArgumentNullException.ThrowIfNull(name) before assigning Name, replacing the
null-coalescing throw expression while preserving the existing argument
validation behavior.
In `@baml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlHttpRequest.cs`:
- Line 21: Update the headers dictionary initialization in BamlHttpRequest to
use StringComparer.OrdinalIgnoreCase instead of StringComparer.Ordinal, ensuring
header lookups remain case-insensitive while preserving the existing copy
behavior.
In `@TASK/state-of-csharp-completeness.md`:
- Line 61: Escape the pipe character in the inline code example within the
“Recursive type alias” table row so Markdown parsers treat it as code content
rather than a column separator, while preserving the example’s meaning and the
table’s column count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d9268423-aa95-4787-b128-dd8f486e8419
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (109)
.github/workflows/build2-csharp-sdk.reusable.yaml.github/workflows/cargo-tests.reusable.yaml.github/workflows/release-baml-language.ymlTASK/bridge-csharp.mdTASK/codex/bytecode-carrier-probe.mdTASK/codex/design-decisions.mdTASK/codex/implementation-notes.mdTASK/codex/native-package-probe.mdTASK/codex/protocol-package-probe.mdTASK/codex/union-layout-probe.mdTASK/design.mdTASK/state-of-csharp-completeness.mdTASK/state-of-python-completeness.mdbaml_language/.config/nextest.tomlbaml_language/Cargo.tomlbaml_language/crates/baml_cli/Cargo.tomlbaml_language/crates/baml_cli/src/generate.rsbaml_language/crates/baml_cli/tests/exit_code_e2e.rsbaml_language/crates/baml_codegen_types/src/generator_fields.rsbaml_language/crates/bex_engine/src/conversion.rsbaml_language/crates/bex_engine/tests/prompt_ast_roundtrip.rsbaml_language/crates/bridge_cffi/src/api.rsbaml_language/sdk_tests/README.mdbaml_language/sdk_tests/crates/csharp/Cargo.tomlbaml_language/sdk_tests/crates/csharp/build.rsbaml_language/sdk_tests/crates/csharp/csharp_cancel_token/customizable/Program.csbaml_language/sdk_tests/crates/csharp/csharp_csv/customizable/Program.csbaml_language/sdk_tests/crates/csharp/csharp_glob/customizable/Program.csbaml_language/sdk_tests/crates/csharp/csharp_llm_clients/customizable/Program.csbaml_language/sdk_tests/crates/csharp/csharp_resources/customizable/Program.csbaml_language/sdk_tests/crates/csharp/csharp_task_group/customizable/Program.csbaml_language/sdk_tests/crates/csharp/function_calls/customizable/Program.csbaml_language/sdk_tests/crates/csharp/llm_functions/customizable/Program.csbaml_language/sdk_tests/crates/csharp/primitive_calls/customizable/Program.csbaml_language/sdk_tests/crates/csharp/setup.ps1baml_language/sdk_tests/crates/csharp/setup.shbaml_language/sdk_tests/crates/csharp/src/lib.rsbaml_language/sdk_tests/fixtures/csharp_cancel_token/baml_src/main.bamlbaml_language/sdk_tests/fixtures/csharp_csv/baml_src/main.bamlbaml_language/sdk_tests/fixtures/csharp_glob/baml_src/main.bamlbaml_language/sdk_tests/fixtures/csharp_llm_clients/baml_src/main.bamlbaml_language/sdk_tests/fixtures/csharp_resources/baml_src/main.bamlbaml_language/sdk_tests/fixtures/csharp_task_group/baml_src/main.bamlbaml_language/sdk_tests/fixtures/function_calls/baml_src/ns_host_callable_tests/main.bamlbaml_language/sdk_tests/fixtures/primitive_calls/baml_src/main.bamlbaml_language/sdk_tests/harness_runner/src/lib.rsbaml_language/sdk_tests/harness_setup/Cargo.tomlbaml_language/sdk_tests/harness_setup/src/csharp.rsbaml_language/sdk_tests/harness_setup/src/lib.rsbaml_language/sdks/csharp/bridge_csharp/.gitignorebaml_language/sdks/csharp/bridge_csharp/Baml.Bridge.slnxbaml_language/sdks/csharp/bridge_csharp/Directory.Packages.propsbaml_language/sdks/csharp/bridge_csharp/README.mdbaml_language/sdks/csharp/bridge_csharp/buildTransitive/baml-bridge.targetsbaml_language/sdks/csharp/bridge_csharp/global.jsonbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Baml.Bridge.csprojbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlBridge.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlClient.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlCsv.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlHandle.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlHttpRequest.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlMedia.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlNullable.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlOptional.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlProgram.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlPromptAst.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlResources.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlStream.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlTypeAlias.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlUnion.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/BamlWireNameAttribute.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Bridge/BridgePlatform.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Bridge/BridgeVersion.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Bridge/CallDispatcher.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Bridge/GeneratedContracts.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Bridge/HostValueRegistry.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Bridge/IBamlNullableValue.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Bridge/IBamlStreamValue.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Bridge/IBamlUnionValue.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Bridge/NativeApi.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Bridge/NativeHandle.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Bridge/ProtoCodec.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Bridge/ProtoTypeCodec.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Exceptions.csbaml_language/sdks/csharp/bridge_csharp/src/Baml.Bridge/Properties/AssemblyInfo.csbaml_language/sdks/csharp/bridge_csharp/tests/Baml.Bridge.Tests/Baml.Bridge.Tests.csprojbaml_language/sdks/csharp/bridge_csharp/tests/Baml.Bridge.Tests/BamlBridgeTests.csbaml_language/sdks/csharp/bridge_csharp/tests/Baml.Bridge.Tests/BamlNullableTests.csbaml_language/sdks/csharp/bridge_csharp/tests/Baml.Bridge.Tests/BamlOptionalTests.csbaml_language/sdks/csharp/bridge_csharp/tests/Baml.Bridge.Tests/BamlUnionTests.csbaml_language/sdks/csharp/bridge_csharp/tests/Baml.Bridge.Tests/GlobalUsings.csbaml_language/sdks/csharp/bridge_csharp/tests/Baml.Bridge.Tests/ProtoCodecTests.csbaml_language/sdks/csharp/bridge_csharp/tools/Baml.NuGet.Normalize/Baml.NuGet.Normalize.csprojbaml_language/sdks/csharp/bridge_csharp/tools/Baml.NuGet.Normalize/Program.csbaml_language/sdks/csharp/bridge_csharp/tools/Baml.Union.Generate/Baml.Union.Generate.csprojbaml_language/sdks/csharp/bridge_csharp/tools/Baml.Union.Generate/Program.csbaml_language/sdks/csharp/bridge_csharp/tools/Baml.Union.LayoutProbe/Baml.Union.LayoutProbe.csprojbaml_language/sdks/csharp/bridge_csharp/tools/Baml.Union.LayoutProbe/Program.csbaml_language/sdks/csharp/bridge_csharp/tools/pack-all-native.shbaml_language/sdks/csharp/bridge_csharp/tools/pack-native.ps1baml_language/sdks/csharp/bridge_csharp/tools/pack-native.shbaml_language/sdks/csharp/sdkgen_csharp/Cargo.tomlbaml_language/sdks/csharp/sdkgen_csharp/src/leaf.rsbaml_language/sdks/csharp/sdkgen_csharp/src/lib.rsbaml_language/sdks/csharp/sdkgen_csharp/src/models.rsbaml_language/sdks/csharp/sdkgen_csharp/src/names.rsbaml_language/sdks/csharp/sdkgen_csharp/src/routing.rsbaml_language/sdks/csharp/sdkgen_csharp/src/translate_ty.rsscripts/baml-language-version
| exit 1 | ||
| fi | ||
|
|
||
| sha256sum "$package" artifacts/csharp-package/*.snupkg > artifacts/csharp-package/SHA256SUMS |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Generate artifact-relative checksum paths.
The manifest records artifacts/csharp-package/..., but that directory's contents become the uploaded artifact root. After download, sha256sum --check SHA256SUMS therefore cannot find the packages.
Proposed fix
- sha256sum "$package" artifacts/csharp-package/*.snupkg > artifacts/csharp-package/SHA256SUMS
+ (
+ cd artifacts/csharp-package
+ sha256sum -- *.nupkg *.snupkg > SHA256SUMS
+ )Also applies to: 99-104
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build2-csharp-sdk.reusable.yaml at line 91, Update the
checksum generation command in the C# package artifact workflow to write paths
relative to the uploaded artifact root, removing the artifacts/csharp-package/
prefix from SHA256SUMS. Apply the same path handling to the corresponding
checksum-generation commands around the additionally referenced lines so
sha256sum --check works after artifact download.
Comment on lines
+91
to
+97
| sha256sum "$package" artifacts/csharp-package/*.snupkg > artifacts/csharp-package/SHA256SUMS | ||
| jq -n \ | ||
| --arg source_sha "${{ inputs.source_sha }}" \ | ||
| --arg version "$(jq -r .canonical_version release-plan.json)" \ | ||
| --argjson package_size "$size" \ | ||
| '{source_sha: $source_sha, version: $version, package_size: $package_size, package_size_ceiling: 200000000}' \ | ||
| > artifacts/csharp-package/provenance.json |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pass source_sha through the environment instead of template expansion.
Line 93 embeds a reusable-workflow input directly into Bash source. A crafted input can alter the command before execution; expose it through env and reference the shell variable.
Proposed fix
- name: Pack and inspect baml-bridge
+ env:
+ SOURCE_SHA: ${{ inputs.source_sha }}
run: |
...
jq -n \
- --arg source_sha "${{ inputs.source_sha }}" \
+ --arg source_sha "$SOURCE_SHA" \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| sha256sum "$package" artifacts/csharp-package/*.snupkg > artifacts/csharp-package/SHA256SUMS | |
| jq -n \ | |
| --arg source_sha "${{ inputs.source_sha }}" \ | |
| --arg version "$(jq -r .canonical_version release-plan.json)" \ | |
| --argjson package_size "$size" \ | |
| '{source_sha: $source_sha, version: $version, package_size: $package_size, package_size_ceiling: 200000000}' \ | |
| > artifacts/csharp-package/provenance.json | |
| - name: Pack and inspect baml-bridge | |
| env: | |
| SOURCE_SHA: ${{ inputs.source_sha }} | |
| run: | | |
| sha256sum "$package" artifacts/csharp-package/*.snupkg > artifacts/csharp-package/SHA256SUMS | |
| jq -n \ | |
| --arg source_sha "$SOURCE_SHA" \ | |
| --arg version "$(jq -r .canonical_version release-plan.json)" \ | |
| --argjson package_size "$size" \ | |
| '{source_sha: $source_sha, version: $version, package_size: $package_size, package_size_ceiling: 200000000}' \ | |
| > artifacts/csharp-package/provenance.json |
🧰 Tools
🪛 zizmor (1.26.1)
[error] 93-93: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build2-csharp-sdk.reusable.yaml around lines 91 - 97,
Update the provenance-generation step around the jq --arg source_sha usage to
pass inputs.source_sha through the workflow step’s env configuration, then
reference the resulting shell environment variable in Bash instead of embedding
the GitHub template expression directly. Keep the existing version, package
size, and provenance output unchanged.
Source: Linters/SAST tools
Comment on lines
+424
to
+443
| if let Err(error) = transaction { | ||
| if manifest_installed { | ||
| let _ = std::fs::remove_file(&manifest_path); | ||
| } | ||
| for relative in installed.iter().rev() { | ||
| let _ = std::fs::remove_file(output_dir.join(Path::new(relative))); | ||
| } | ||
| if manifest_backed_up { | ||
| let _ = std::fs::rename(staged_backup.join(GENERATED_MANIFEST_FILE), &manifest_path); | ||
| } | ||
| for relative in backed_up.iter().rev() { | ||
| let backup = staged_backup.join(Path::new(relative)); | ||
| let destination = output_dir.join(Path::new(relative)); | ||
| if let Some(parent) = destination.parent() { | ||
| let _ = std::fs::create_dir_all(parent); | ||
| } | ||
| let _ = std::fs::rename(backup, destination); | ||
| } | ||
| let _ = std::fs::remove_dir_all(&staging); | ||
| return Err(error.context("Generated output was rolled back")); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve backups when rollback is incomplete.
Every restoration error is discarded, then Line 442 deletes the staging directory containing any backup that failed to restore. A filesystem failure can therefore leave output missing while reporting that it “was rolled back.”
Collect rollback failures and retain the staging state whenever restoration is incomplete; add a fault-injection test for this path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/crates/baml_cli/src/generate.rs` around lines 424 - 443, The
rollback handling around the transaction error must track failures from
removing, renaming, and directory-creation operations instead of discarding
them. In the rollback branch, only remove the staging directory when every
restoration succeeds; otherwise preserve it, return an error indicating rollback
was incomplete, and retain the original error context. Add a fault-injection
test covering a restoration failure and verifying the staging backups remain
available.
Comment on lines
+80
to
+99
| var suffix = new Queue<byte>(4); | ||
| var buffer = new byte[1]; | ||
| while (true) | ||
| { | ||
| if (await stream.ReadAsync(buffer) == 0) | ||
| { | ||
| throw new IOException("The loopback HTTP client disconnected before sending its headers."); | ||
| } | ||
|
|
||
| suffix.Enqueue(buffer[0]); | ||
| if (suffix.Count > 4) | ||
| { | ||
| suffix.Dequeue(); | ||
| } | ||
|
|
||
| if (suffix.SequenceEqual("\r\n\r\n"u8.ToArray())) | ||
| { | ||
| break; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Bound the HTTP header read loop.
A client that connects but never sends \r\n\r\n leaves both FetchUrlAsync and ServeResponse waiting indefinitely; the timeout on Line 69 is reached only after the fetch returns. Pass a timed cancellation token to ReadAsync.
Proposed timeout
static async Task ServeResponse(TcpListener listener)
{
using var client = await listener.AcceptTcpClientAsync().WaitAsync(TimeSpan.FromSeconds(10));
await using var stream = client.GetStream();
+ using var timeout = new CancellationTokenSource(TimeSpan.FromSeconds(10));
var suffix = new Queue<byte>(4);
var buffer = new byte[1];
while (true)
{
- if (await stream.ReadAsync(buffer) == 0)
+ if (await stream.ReadAsync(buffer, timeout.Token) == 0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var suffix = new Queue<byte>(4); | |
| var buffer = new byte[1]; | |
| while (true) | |
| { | |
| if (await stream.ReadAsync(buffer) == 0) | |
| { | |
| throw new IOException("The loopback HTTP client disconnected before sending its headers."); | |
| } | |
| suffix.Enqueue(buffer[0]); | |
| if (suffix.Count > 4) | |
| { | |
| suffix.Dequeue(); | |
| } | |
| if (suffix.SequenceEqual("\r\n\r\n"u8.ToArray())) | |
| { | |
| break; | |
| } | |
| } | |
| var suffix = new Queue<byte>(4); | |
| var buffer = new byte[1]; | |
| using var timeout = new CancellationTokenSource(TimeSpan.FromSeconds(10)); | |
| while (true) | |
| { | |
| if (await stream.ReadAsync(buffer, timeout.Token) == 0) | |
| { | |
| throw new IOException("The loopback HTTP client disconnected before sending its headers."); | |
| } | |
| suffix.Enqueue(buffer[0]); | |
| if (suffix.Count > 4) | |
| { | |
| suffix.Dequeue(); | |
| } | |
| if (suffix.SequenceEqual("\r\n\r\n"u8.ToArray())) | |
| { | |
| break; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@baml_language/sdk_tests/crates/csharp/csharp_resources/customizable/Program.cs`
around lines 80 - 99, Bound the header-reading loop in
FetchUrlAsync/ServeResponse by creating or reusing the timeout cancellation
token established near the existing timeout setup and passing it to each
stream.ReadAsync call. Ensure a client that never sends the "\r\n\r\n" header
terminator causes the read to cancel when the timeout expires rather than
waiting indefinitely, while preserving the current disconnect exception for a
zero-byte read.
Comment on lines
+402
to
+429
| static async Task AssertHardExit(int expectedCode) | ||
| { | ||
| var executable = Environment.ProcessPath | ||
| ?? throw new InvalidOperationException("The current executable path is unavailable."); | ||
| using var process = Process.Start(new ProcessStartInfo | ||
| { | ||
| FileName = executable, | ||
| ArgumentList = { "--exit-probe", expectedCode.ToString() }, | ||
| RedirectStandardOutput = true, | ||
| RedirectStandardError = true, | ||
| UseShellExecute = false, | ||
| }) ?? throw new InvalidOperationException("Failed to start the hard-exit child process."); | ||
|
|
||
| var stdout = process.StandardOutput.ReadToEndAsync(); | ||
| var stderr = process.StandardError.ReadToEndAsync(); | ||
| await process.WaitForExitAsync(); | ||
| if (process.ExitCode != expectedCode) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Hard-exit child returned {process.ExitCode}, expected {expectedCode}. " | ||
| + $"stdout={await stdout}; stderr={await stderr}"); | ||
| } | ||
|
|
||
| if ((await stderr).Contains("returned unexpectedly", StringComparison.Ordinal)) | ||
| { | ||
| throw new InvalidOperationException("baml.sys.exit returned instead of terminating the child."); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- primitive_calls/customizable/Program.cs (around AssertHardExit) ---'
sed -n '360,460p' baml_language/sdk_tests/crates/csharp/primitive_calls/customizable/Program.cs
echo
echo '--- llm_functions/customizable/Program.cs (around RunBuildRequestProcess) ---'
sed -n '1,90p' baml_language/sdk_tests/crates/csharp/llm_functions/customizable/Program.cs
echo
echo '--- llm_functions/customizable/Program.cs (around RunReplay) ---'
sed -n '240,340p' baml_language/sdk_tests/crates/csharp/llm_functions/customizable/Program.cs
echo
echo '--- search for process cleanup / timeout helpers in these test crates ---'
rg -n "WaitForExitAsync|KillTree|Kill\(|Timeout|WaitAsync\\(TimeSpan|Process.Start\\(|CreateSelfStartInfo|StartSelf|Dispose|shutdown" \
baml_language/sdk_tests/crates/csharp/primitive_calls/customizable/Program.cs \
baml_language/sdk_tests/crates/csharp/llm_functions/customizable/Program.csRepository: BoundaryML/baml
Length of output: 12360
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '340,410p' baml_language/sdk_tests/crates/csharp/llm_functions/customizable/Program.csRepository: BoundaryML/baml
Length of output: 1320
Terminate timed-out child processes.
baml_language/sdk_tests/crates/csharp/primitive_calls/customizable/Program.cs#L402-L429: add a timeout and kill/reap the exit-probe child if it never exits.baml_language/sdk_tests/crates/csharp/llm_functions/customizable/Program.cs#L39-L51and#L262-L301:WaitAsync(...)can throw while the child/server keeps running; kill the process tree on timeout or startup failure, and ensure cleanup runs unconditionally.
📍 Affects 2 files
baml_language/sdk_tests/crates/csharp/primitive_calls/customizable/Program.cs#L402-L429(this comment)baml_language/sdk_tests/crates/csharp/llm_functions/customizable/Program.cs#L39-L51baml_language/sdk_tests/crates/csharp/llm_functions/customizable/Program.cs#L262-L301
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@baml_language/sdk_tests/crates/csharp/primitive_calls/customizable/Program.cs`
around lines 402 - 429, Update AssertHardExit in
baml_language/sdk_tests/crates/csharp/primitive_calls/customizable/Program.cs:402-429
to wait with a bounded timeout, then kill and reap the exit-probe process if it
does not exit. In
baml_language/sdk_tests/crates/csharp/llm_functions/customizable/Program.cs:39-51
and :262-301, ensure WaitAsync timeouts or startup failures kill the relevant
process tree and that cleanup executes unconditionally.
Comment on lines
+230
to
+247
| pub(crate) fn csharp_string(value: &str) -> String { | ||
| let mut out = String::from("\""); | ||
| for ch in value.chars() { | ||
| match ch { | ||
| '\\' => out.push_str("\\\\"), | ||
| '"' => out.push_str("\\\""), | ||
| '\n' => out.push_str("\\n"), | ||
| '\r' => out.push_str("\\r"), | ||
| '\t' => out.push_str("\\t"), | ||
| ch if ch.is_control() => { | ||
| let _ = write!(out, "\\u{:04x}", ch as u32); | ||
| } | ||
| ch => out.push(ch), | ||
| } | ||
| } | ||
| out.push('"'); | ||
| out | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- lib.rs around csharp_string ---'
sed -n '200,290p' baml_language/sdks/csharp/sdkgen_csharp/src/lib.rs
echo
echo '--- search for csharp_string and related tests ---'
rg -n "csharp_string|2028|2029|unicode separator|line separator" baml_language/sdks/csharp/sdkgen_csharp/src -S
echo
echo '--- file list in crate ---'
git ls-files baml_language/sdks/csharp/sdkgen_csharp/src baml_language/sdks/csharp/sdkgen_csharp/testsRepository: BoundaryML/baml
Length of output: 6162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tests in lib.rs around the existing assertions ---'
sed -n '340,460p' baml_language/sdks/csharp/sdkgen_csharp/src/lib.rs
echo
echo '--- any unit tests in sibling modules using csharp_string ---'
rg -n "assert!\\(|csharp_string\\(" baml_language/sdks/csharp/sdkgen_csharp/src/{models.rs,leaf.rs,translate_ty.rs} -SRepository: BoundaryML/baml
Length of output: 8773
🌐 Web query:
C# string literal U+2028 U+2029 source newline line separator lexical grammar
💡 Result:
In the C# lexical grammar, the Unicode characters U+2028 (Line Separator) and U+2029 (Paragraph Separator) are explicitly classified as New_Line_Character components [1][2]. Because they are defined as New_Line_Character, they are treated as line terminators by the C# compiler [1][2]. This has specific implications for string literals: 1. Regular String Literals: The lexical grammar for regular string literals forbids the inclusion of raw new-line characters, including U+2028 and U+2029 [3][1]. To include these characters in a regular string literal, you must use their corresponding Unicode escape sequences (\u2028 and \u2029) [3]. 2. Raw String Literals: The content of a raw string literal also excludes these new-line characters [1]. The grammar for raw string literal content is defined as any character except the specified New_Line_Character set (which includes U+2028 and U+2029) [1]. In summary, U+2028 and U+2029 are treated as structural newline separators in C# source code [1][2]. They cannot appear literally within standard or raw string literals and must be escaped if you wish to represent them as data within a string [3][1].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/grammar
- 2: https://github.com/dotnet/csharplang/blob/479f29fc61086718aeadaa66d3fe62e605190e41/spec/lexical-structure.md
- 3: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/lexical-structure
Escape C# Unicode line separators. csharp_string also needs to escape U+2028/U+2029; C# treats both as line terminators, so generated .g.cs literals can break when wire values contain them. Please add a crate-local unit test for both characters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/sdks/csharp/sdkgen_csharp/src/lib.rs` around lines 230 - 247,
The csharp_string function must escape Unicode line separators U+2028 and U+2029
so generated C# literals remain valid. Add explicit handling for both characters
using their Unicode escape sequences, and add crate-local unit coverage
verifying each is escaped.
Source: Coding guidelines
Comment on lines
+103
to
+110
| out, | ||
| " public bool Equals({class_name}? other) => other is not null && Value.Equals(other.Value);\n" | ||
| ); | ||
| let _ = writeln!( | ||
| out, | ||
| " public override bool Equals(object? obj) => obj is {class_name} other && Equals(other);\n" | ||
| ); | ||
| out.push_str(" public override int GetHashCode() => Value.GetHashCode();\n\n"); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make recursive-alias equality null-safe.
A nullable alias value makes Value.Equals(...) or Value.GetHashCode() throw. Generate equality and hashing through EqualityComparer<T>.Default, with a null-safe hash.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/sdks/csharp/sdkgen_csharp/src/models.rs` around lines 103 -
110, Update the generated equality and hashing expressions in the
model-generation code around the `Equals` implementations and `GetHashCode`
output to use `EqualityComparer<T>.Default`, where T is the alias value type.
Ensure both comparisons and hash generation remain null-safe when `Value` is
nullable, while preserving the existing nullable `other` check and generated API
shape.
Comment on lines
+325
to
+333
| let is_static = context.receiver.is_static(); | ||
| let is_async = variant.is_async(); | ||
| let class_supported = context.class_supported; | ||
| let class_generic_params = context.class_generic_params; | ||
| let aliases = context.aliases; | ||
| let use_async_callback_types = variant.uses_async_callbacks(); | ||
| let mut occupied = BTreeSet::new(); | ||
| let mut parameters = Vec::new(); | ||
| let mut supported = class_supported; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not gate static methods on instance-property codec support.
class_supported depends on the class properties, but static methods never encode self. A supported static method on a class containing an unsupported property is currently emitted as NotSupportedException.
Proposed fix
- let mut supported = class_supported;
+ let mut supported = is_static || class_supported;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let is_static = context.receiver.is_static(); | |
| let is_async = variant.is_async(); | |
| let class_supported = context.class_supported; | |
| let class_generic_params = context.class_generic_params; | |
| let aliases = context.aliases; | |
| let use_async_callback_types = variant.uses_async_callbacks(); | |
| let mut occupied = BTreeSet::new(); | |
| let mut parameters = Vec::new(); | |
| let mut supported = class_supported; | |
| let is_static = context.receiver.is_static(); | |
| let is_async = variant.is_async(); | |
| let class_supported = context.class_supported; | |
| let class_generic_params = context.class_generic_params; | |
| let aliases = context.aliases; | |
| let use_async_callback_types = variant.uses_async_callbacks(); | |
| let mut occupied = BTreeSet::new(); | |
| let mut parameters = Vec::new(); | |
| let mut supported = is_static || class_supported; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/sdks/csharp/sdkgen_csharp/src/models.rs` around lines 325 -
333, Adjust the support decision in the method-generation logic around
is_static, class_supported, and supported so static methods are not gated by
class_supported or instance-property codec support. Preserve the existing
class_supported validation for instance methods, while allowing otherwise
supported static methods to be emitted normally instead of as
NotSupportedException.
Comment on lines
+235
to
+269
| fn file_segment(projected: &str) -> String { | ||
| let unescaped = projected.strip_prefix('@').unwrap_or(projected); | ||
| let upper = unescaped.to_ascii_uppercase(); | ||
| let reserved = matches!(upper.as_str(), "CON" | "PRN" | "AUX" | "NUL") | ||
| || upper.strip_prefix("COM").is_some_and(|suffix| { | ||
| matches!(suffix, "1" | "2" | "3" | "4" | "5" | "6" | "7" | "8" | "9") | ||
| }) | ||
| || upper.strip_prefix("LPT").is_some_and(|suffix| { | ||
| matches!(suffix, "1" | "2" | "3" | "4" | "5" | "6" | "7" | "8" | "9") | ||
| }); | ||
| if reserved { | ||
| format!("_{unescaped}") | ||
| } else { | ||
| unescaped.to_string() | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use baml_base::Name as BaseName; | ||
|
|
||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn keeps_windows_device_names_out_of_generated_paths() { | ||
| let name = Name::new( | ||
| BaseName::new("user"), | ||
| vec![BaseName::new("con"), BaseName::new("lpt1")], | ||
| BaseName::new("probe"), | ||
| ); | ||
|
|
||
| let leaf = route(&name); | ||
| assert_eq!(leaf.namespace, "BamlSdk.Con.Lpt1"); | ||
| assert_eq!(leaf.path, PathBuf::from("_Con/_Lpt1")); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make Windows device-name escaping collision-free.
Con is rewritten to _Con, which collides with the valid projected identifier _Con. Distinct leaves can therefore receive the same Functions.g.cs/Types.g.cs path, and the path-keyed output map in sdkgen_csharp/src/lib.rs will replace one generated file.
Use a prefix impossible in a C# identifier and test the transformed segments directly.
Proposed fix
if reserved {
- format!("_{unescaped}")
+ format!("~{unescaped}")
} else {
unescaped.to_string()
}- assert_eq!(leaf.path, PathBuf::from("_Con/_Lpt1"));
+ assert_eq!(leaf.path, PathBuf::from("~Con/~Lpt1"));
+ assert_ne!(
+ file_segment("Con").to_ascii_lowercase(),
+ file_segment("_Con").to_ascii_lowercase()
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn file_segment(projected: &str) -> String { | |
| let unescaped = projected.strip_prefix('@').unwrap_or(projected); | |
| let upper = unescaped.to_ascii_uppercase(); | |
| let reserved = matches!(upper.as_str(), "CON" | "PRN" | "AUX" | "NUL") | |
| || upper.strip_prefix("COM").is_some_and(|suffix| { | |
| matches!(suffix, "1" | "2" | "3" | "4" | "5" | "6" | "7" | "8" | "9") | |
| }) | |
| || upper.strip_prefix("LPT").is_some_and(|suffix| { | |
| matches!(suffix, "1" | "2" | "3" | "4" | "5" | "6" | "7" | "8" | "9") | |
| }); | |
| if reserved { | |
| format!("_{unescaped}") | |
| } else { | |
| unescaped.to_string() | |
| } | |
| } | |
| #[cfg(test)] | |
| mod tests { | |
| use baml_base::Name as BaseName; | |
| use super::*; | |
| #[test] | |
| fn keeps_windows_device_names_out_of_generated_paths() { | |
| let name = Name::new( | |
| BaseName::new("user"), | |
| vec![BaseName::new("con"), BaseName::new("lpt1")], | |
| BaseName::new("probe"), | |
| ); | |
| let leaf = route(&name); | |
| assert_eq!(leaf.namespace, "BamlSdk.Con.Lpt1"); | |
| assert_eq!(leaf.path, PathBuf::from("_Con/_Lpt1")); | |
| } | |
| fn file_segment(projected: &str) -> String { | |
| let unescaped = projected.strip_prefix('@').unwrap_or(projected); | |
| let upper = unescaped.to_ascii_uppercase(); | |
| let reserved = matches!(upper.as_str(), "CON" | "PRN" | "AUX" | "NUL") | |
| || upper.strip_prefix("COM").is_some_and(|suffix| { | |
| matches!(suffix, "1" | "2" | "3" | "4" | "5" | "6" | "7" | "8" | "9") | |
| }) | |
| || upper.strip_prefix("LPT").is_some_and(|suffix| { | |
| matches!(suffix, "1" | "2" | "3" | "4" | "5" | "6" | "7" | "8" | "9") | |
| }); | |
| if reserved { | |
| format!("~{unescaped}") | |
| } else { | |
| unescaped.to_string() | |
| } | |
| } | |
| #[cfg(test)] | |
| mod tests { | |
| use baml_base::Name as BaseName; | |
| use super::*; | |
| #[test] | |
| fn keeps_windows_device_names_out_of_generated_paths() { | |
| let name = Name::new( | |
| BaseName::new("user"), | |
| vec![BaseName::new("con"), BaseName::new("lpt1")], | |
| BaseName::new("probe"), | |
| ); | |
| let leaf = route(&name); | |
| assert_eq!(leaf.namespace, "BamlSdk.Con.Lpt1"); | |
| assert_eq!(leaf.path, PathBuf::from("~Con/~Lpt1")); | |
| assert_ne!( | |
| file_segment("Con").to_ascii_lowercase(), | |
| file_segment("_Con").to_ascii_lowercase() | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/sdks/csharp/sdkgen_csharp/src/routing.rs` around lines 235 -
269, Update file_segment to escape reserved Windows device names with a prefix
that cannot occur in a valid C# identifier, avoiding collisions with projected
names such as _Con. Adjust keeps_windows_device_names_out_of_generated_paths and
add direct assertions for transformed reserved and non-reserved segments,
including that distinct inputs produce distinct paths.
Comment on lines
+448
to
+453
| This is still not the full question-9 probe. The complete supported-host | ||
| matrix, trimming/NativeAOT, and version-skew coverage remain outstanding. | ||
| Deterministic generation, direct/imported-schema invalidation, managed | ||
| compilation, package contents/dependencies, generated-source compilation, | ||
| primitive runtime round trips, and a clean package consumer now pass on Linux | ||
| x64. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove trimming/NativeAOT from the outstanding question-9 coverage.
They are explicit v1 non-goals elsewhere in this document, not pending protobuf-host validation. Keeping them here makes the completion status contradictory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@TASK/codex/implementation-notes.md` around lines 448 - 453, Update the
question-9 coverage status in the implementation notes to remove
trimming/NativeAOT from the outstanding items, while preserving the remaining
supported-host matrix and version-skew work. Keep the explicit v1 non-goal
statements elsewhere unchanged.
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Binary size checks failed❌ 4 violations · ✅ 3 passed
Details & how to fixViolations:
Add/update baselines:
[artifacts.baml-cli]
file_bytes = 19727136
stripped_bytes = 19727184
gzip_bytes = 9417000
[artifacts.bridge_wasm]
file_bytes = 16160601
gzip_bytes = 4402874
[artifacts.baml-cli]
file_bytes = 21229568
stripped_bytes = 21229568
gzip_bytes = 9625194
[artifacts.baml-cli]
file_bytes = 25514064
stripped_bytes = 25514056
gzip_bytes = 10836002Generated by |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
net10.0baml-bridgehost runtime that loads the versioned C API table and supports typed calls, generics, classes, enums, unions, aliases, callbacks, streams, media, resources, cancellation, errors, panics, and hard exitssdkgen_csharpplusbaml generateintegration with deterministic naming/routing, bounded embedded bytecode, exact generator/runtime version checks, and transactional generated-file ownershipTASK/Why
BAML had no complete generated C# SDK or managed bridge across the stable C ABI. This establishes the first supported C# boundary and fixes the public API, ownership, packaging, and compatibility contracts before publication.
User impact
C# consumers generate source under
BamlSdk, reference the matching exactbaml-bridgeversion, and call idiomatic sync/async APIs without building the native runtime themselves. The initial runtime supports one distinct compiled BAML program per process. Trimming and NativeAOT are explicit v1 non-goals.Validation
cargo test --locked -p sdkgen_csharp: 18 passedsdkgen_csharpandbaml_cli.nupkg/.snupkgassemblies with exactly eight native RID pathsRemaining release gates
Production publication remains disabled until real native binaries are inspected and exercised on all eight RID runners and the NuGet organization/trusted-publisher setup is complete. Two listener-backed fixtures could not be rerun in the current sandbox; their earlier focused runs and managed coverage are recorded in the implementation notes.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation