feat: S13.5 walking-skeleton scenario green on Windows via OTel oracle#154
Merged
Conversation
Introduces an OpenTelemetry Collector Contrib oracle as a Windows-native replacement for syslog-ng, makes the BDD step code oracle-agnostic, and promotes the walking-skeleton scenario out of @windows_wip. Oracle infrastructure: - Bdd/otel/config.yaml — RFC 5424 UDP receiver on 127.0.0.1:5514, file exporter writing JSON Lines. - Bdd/otel/Install-OtelCollector.ps1 — pinned otelcol-contrib v0.150.1, SHA-256 verified, idempotent. - Bdd/requirements.txt — pins behave==1.3.3 (matches the Linux container image so both runners parse identically). Step abstraction: - environment.py reads EXAMPLE_BINARY, RECEIVED_LOG, ORACLE_FORMAT from the env so the same step file drives both runners. - syslog_steps.py adds parse_otel_jsonl_line + parse_oracle_line dispatch. Walking-skeleton scope only — STRUCTURED_DATA re-rendering for OTel is added when the structured-data scenarios are promoted. - run_example refactored to use process.communicate() and an absolute binary path — portable across Linux and Windows (no select.select on a pipe fd, which Windows doesn't support; Windows CreateProcess can't resolve forward-slash relative paths). - run_threaded_example keeps the prompt-protocol path so the threaded example's service thread has time to drain the buffer between "send N" and "quit". POSIX-only — all threaded scenarios are tagged @Buffered and excluded from the Windows runner. ExampleAppName_Set now strips a trailing .exe/.EXE so the syslog app name omits the OS executable extension regardless of build platform. TDD-driven by the failing walking-skeleton scenario which expected "SolidSyslogExample" but was getting "SolidSyslogExample.exe". 4 new unit tests cover lower/upper case, with-path, and non-.exe-extension control cases. Verified locally: - Linux BDD via ci/docker-compose.bdd.yml: 14 features / 31 scenarios / 160 steps all green. - Windows walking-skeleton via behave + otelcol locally: 1 scenario / 7 steps green. - Linux gates: tidy, cppcheck, sanitize (530 tests), format, coverage (99.9%/100%) all green. Refs #129 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds OpenTelemetry Collector as an alternative "oracle" for Windows BDD tests, expands BDD environment/config to select syslog-ng or OTel JSONL parsing, updates runners and parsers to be oracle-agnostic, adds OTel installer/config files and .gitignore rule, and strips trailing Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Behave as BDD Test Runner
participant Example as Example Binary
participant OTel as OTel Collector
participant File as File Exporter
participant Parser as parse_oracle_line
rect rgba(100, 150, 255, 0.5)
Note over User,Parser: OTel JSONL Oracle (Windows)
User->>Behave: run behave --tags `@udp` ORACLE_FORMAT=otel-jsonl
Behave->>Example: launch context.example_binary
Example-->>OTel: send syslog UDP 5514
OTel->>OTel: RFC5424 parse → records
OTel->>File: write JSON Lines to Bdd/output/received.jsonl
Behave->>File: read latest jsonl line from context.received_log
Behave->>Parser: parse_oracle_line(latest_line, "otel-jsonl")
Parser->>Behave: flat field dict
Behave->>Behave: assert fields match expectations
end
sequenceDiagram
actor User
participant Behave as BDD Test Runner
participant Example as Example Binary
participant Syslog as syslog-ng
participant Parser as parse_oracle_line
rect rgba(150, 200, 100, 0.5)
Note over User,Parser: syslog-ng Oracle (POSIX)
User->>Behave: run behave --tags `@udp` ORACLE_FORMAT=syslog-ng
Behave->>Example: launch context.example_binary
Example-->>Syslog: send syslog UDP 514
Syslog->>Syslog: key=value parsing -> append to context.received_log
Behave->>Syslog: tail context.received_log
Behave->>Parser: parse_oracle_line(latest_line, "syslog-ng")
Parser->>Behave: flat field dict (via parse_syslog_ng_line)
Behave->>Behave: assert fields match expectations
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 99% successful (✔️ 539 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Bdd/features/steps/syslog_steps.py (1)
243-260:⚠️ Potential issue | 🟠 MajorAlways tear down the threaded helper on intermediate failures.
If
wait_for_prompt,send_command, orwait_for_messagesraises here, the function exits before sendingquit, leaving the example process alive for later scenarios. That kind of leak tends to cascade through the rest of the BDD run.🧹 Suggested fix
process = subprocess.Popen( cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, ) context.example_pid = process.pid - wait_for_prompt(process) - send_command(process, f"send {expected_messages}") - wait_for_messages(context, expected_messages) - - process.stdin.write("quit\n") - process.stdin.flush() - process.wait(timeout=10) - assert process.returncode == 0, ( - f"Threaded example failed with exit code {process.returncode}" - ) + try: + wait_for_prompt(process) + send_command(process, f"send {expected_messages}") + wait_for_messages(context, expected_messages) + + process.stdin.write("quit\n") + process.stdin.flush() + process.wait(timeout=10) + assert process.returncode == 0, ( + f"Threaded example failed with exit code {process.returncode}" + ) + finally: + if process.poll() is None: + process.kill() + process.wait(timeout=5)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Bdd/features/steps/syslog_steps.py` around lines 243 - 260, Ensure the spawned helper process is always torn down by wrapping the interaction sequence (wait_for_prompt, send_command, wait_for_messages) in a try/finally: use the existing process variable and context.example_pid to send the "quit\n" (via process.stdin.write + flush) and wait for process termination in the finally block, and if the process does not exit within timeout call process.kill() and then process.wait(); keep the final assert on process.returncode after cleanup so intermediate exceptions cannot leak the helper process.
🧹 Nitpick comments (1)
Example/Common/ExampleAppName.c (1)
54-57: Consider using the named constant for consistency.Line 56 uses the literal
4while the helper definesEXE_SUFFIX_LENGTH = 4. Using the constant would improve maintainability, though not critical since the value is unlikely to change.♻️ Optional: Use the constant
+enum +{ + EXE_SUFFIX_LENGTH = 4 +}; + void ExampleAppName_Set(const char* argv0) { ... if (EndsWithDotExe(appName, appNameLength)) { - appNameLength -= 4; + appNameLength -= EXE_SUFFIX_LENGTH; } }This would require hoisting the enum to file scope or repeating it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Example/Common/ExampleAppName.c` around lines 54 - 57, The code subtracts a magic number 4 from appNameLength after calling EndsWithDotExe; replace the literal with the named constant EXE_SUFFIX_LENGTH to improve maintainability: update the subtraction to use EXE_SUFFIX_LENGTH instead of 4 and ensure EXE_SUFFIX_LENGTH is visible here (either by hoisting the enum/constant to file scope or defining an equivalent local constant) so the change compiles and retains the same behavior for EndsWithDotExe and appNameLength adjustments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Bdd/features/steps/syslog_steps.py`:
- Around line 88-121: The parser never sets fields["MSG"], so update
parse_otel_jsonl_line to extract the log message from the OTel log record (use
the local variable log) before returning; check log.get("body") and handle both
plain string bodies and object bodies (e.g., dicts containing "stringValue" or
similar string keys), normalize to a Python string, and assign it to
fields["MSG"] (skip or omit if no body present) so the returned dict shape
includes MSG alongside PRIORITY/TIMESTAMP/HOSTNAME/APP_NAME/PROCID.
In `@Bdd/otel/Install-OtelCollector.ps1`:
- Around line 22-27: The current fast-path returns early if $exePath exists,
skipping the requested $Version/$Sha256 verification; change the logic in
Install-OtelCollector so that when Test-Path $exePath is true you compute and
validate the existing binary's integrity (e.g., use Get-FileHash -Algorithm
SHA256 on $exePath and compare to $Sha256, and/or inspect the binary/version
metadata against $Version) and only return if the hash/version matches; if it
doesn't match, remove or overwrite the existing otelcol-contrib.exe in $BinDir
and continue with the normal download/install flow to ensure the pinned
$Version/$Sha256 are enforced.
In `@docs/bdd.md`:
- Around line 167-174: The documentation uses bash background operator '&' after
'./Bdd/otel/bin/otelcol-contrib.exe --config=Bdd/otel/config.yaml' which is
invalid in PowerShell; update the docs to show both POSIX and PowerShell ways to
run the OTel collector in the background by keeping the existing bash example
with '&' and adding a PowerShell alternative (e.g., referencing Start-Process to
launch ./Bdd/otel/bin/otelcol-contrib.exe with the
"--config=Bdd/otel/config.yaml" argument) plus a short note that PowerShell
users should use that form or run the collector in a separate terminal.
- Around line 170-174: The docs show bash-style inline environment variables
(EXAMPLE_BINARY, RECEIVED_LOG, ORACLE_FORMAT) preceding the behave command which
won’t work in PowerShell; update the Windows instructions to provide the
PowerShell equivalent by either (a) setting the env vars beforehand using
PowerShell syntax ($env:EXAMPLE_BINARY = "...", $env:RECEIVED_LOG = "...",
$env:ORACLE_FORMAT = "...") and then running the behave command, or (b) wrap the
bash-style invocation in pwsh -c "..." so the same inline VAR=value behave
invocation runs in a bash-like shell; make the change near the block that shows
EXAMPLE_BINARY=build/... and the behave --tags=... Bdd/features/ invocation and
add a short note indicating which approach is for PowerShell.
---
Outside diff comments:
In `@Bdd/features/steps/syslog_steps.py`:
- Around line 243-260: Ensure the spawned helper process is always torn down by
wrapping the interaction sequence (wait_for_prompt, send_command,
wait_for_messages) in a try/finally: use the existing process variable and
context.example_pid to send the "quit\n" (via process.stdin.write + flush) and
wait for process termination in the finally block, and if the process does not
exit within timeout call process.kill() and then process.wait(); keep the final
assert on process.returncode after cleanup so intermediate exceptions cannot
leak the helper process.
---
Nitpick comments:
In `@Example/Common/ExampleAppName.c`:
- Around line 54-57: The code subtracts a magic number 4 from appNameLength
after calling EndsWithDotExe; replace the literal with the named constant
EXE_SUFFIX_LENGTH to improve maintainability: update the subtraction to use
EXE_SUFFIX_LENGTH instead of 4 and ensure EXE_SUFFIX_LENGTH is visible here
(either by hoisting the enum/constant to file scope or defining an equivalent
local constant) so the change compiles and retains the same behavior for
EndsWithDotExe and appNameLength adjustments.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 397fa805-f22f-4b58-9a7c-e6edb5732f30
📒 Files selected for processing (10)
.gitignoreBdd/features/environment.pyBdd/features/steps/syslog_steps.pyBdd/features/syslog.featureBdd/otel/Install-OtelCollector.ps1Bdd/otel/config.yamlBdd/requirements.txtExample/Common/ExampleAppName.cTests/Example/ExampleAppNameTest.cppdocs/bdd.md
Comment on lines
+88
to
+121
| def parse_otel_jsonl_line(line): | ||
| """Parse one JSON line from the OTel Collector file exporter into the | ||
| same flat dict shape as parse_syslog_ng_line. | ||
|
|
||
| Walking-skeleton scope only: PRIORITY/TIMESTAMP/HOSTNAME/APP_NAME/PROCID | ||
| plus MSG. STRUCTURED_DATA is not yet rendered back into syslog-ng's | ||
| `[name key="value"]` text form — to be added when promoting the | ||
| structured_data scenarios out of @windows_wip. | ||
| """ | ||
| record = json.loads(line) | ||
| log = record["resourceLogs"][0]["scopeLogs"][0]["logRecords"][0] | ||
| attrs = log.get("attributes", []) | ||
|
|
||
| fields = {} | ||
| priority = _otel_attribute(attrs, "priority") | ||
| if priority is not None: | ||
| fields["PRIORITY"] = priority | ||
| hostname = _otel_attribute(attrs, "hostname") | ||
| if hostname is not None: | ||
| fields["HOSTNAME"] = hostname | ||
| appname = _otel_attribute(attrs, "appname") | ||
| if appname is not None: | ||
| fields["APP_NAME"] = appname | ||
| proc_id = _otel_attribute(attrs, "proc_id") | ||
| if proc_id is not None: | ||
| fields["PROCID"] = proc_id | ||
|
|
||
| # timeUnixNano → ISO 8601 string compatible with datetime.fromisoformat | ||
| time_ns = log.get("timeUnixNano") | ||
| if time_ns is not None: | ||
| seconds = int(time_ns) / 1e9 | ||
| fields["TIMESTAMP"] = datetime.fromtimestamp(seconds, tz=timezone.utc).isoformat() | ||
|
|
||
| return fields |
Contributor
There was a problem hiding this comment.
Populate MSG before returning from the OTel parser.
This parser never assigns fields["MSG"], so any step that asserts the message body will fail under otel-jsonl even when the oracle received the record. The docstring also says MSG is part of the returned shape.
🧩 Suggested fix
time_ns = log.get("timeUnixNano")
if time_ns is not None:
seconds = int(time_ns) / 1e9
fields["TIMESTAMP"] = datetime.fromtimestamp(seconds, tz=timezone.utc).isoformat()
+
+ body = log.get("body", {})
+ if "stringValue" in body:
+ fields["MSG"] = body["stringValue"]
+ elif "intValue" in body:
+ fields["MSG"] = str(body["intValue"])
return fields🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Bdd/features/steps/syslog_steps.py` around lines 88 - 121, The parser never
sets fields["MSG"], so update parse_otel_jsonl_line to extract the log message
from the OTel log record (use the local variable log) before returning; check
log.get("body") and handle both plain string bodies and object bodies (e.g.,
dicts containing "stringValue" or similar string keys), normalize to a Python
string, and assign it to fields["MSG"] (skip or omit if no body present) so the
returned dict shape includes MSG alongside
PRIORITY/TIMESTAMP/HOSTNAME/APP_NAME/PROCID.
Owner
Author
There was a problem hiding this comment.
Partially accepted — fixed in 016310b by tightening the docstring to remove the false plus MSG claim. MSG extraction itself is deferred to the message_fields.feature promotion PR, because the suggested body.stringValue mapping isn't right: OTel's body carries the whole raw RFC 5424 line (priority+timestamp+host+app+procid+SD+MSG) for the message-less record we currently send, not just the MSG part. To pick the right extraction I need to drive it from a scenario that sends a real bodied message via --message, which is part of the next promotion slice.
Contributor
There was a problem hiding this comment.
@DavidCozens, thanks for the clarification — that's a good catch. The docstring correction in 016310b removes the misleading claim, and the deferral rationale is solid: mapping body.stringValue → MSG would produce the entire raw RFC 5424 line rather than just the message part, so the extraction really does need to be grounded in a scenario that sends a bodied message via --message to verify the correct field mapping. Makes sense to tie that to the message_fields.feature promotion slice.
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 154
File: Bdd/features/steps/syslog_steps.py:88-123
Timestamp: 2026-04-18T15:14:43.668Z
Learning: In `Bdd/features/steps/syslog_steps.py`, the OTel Collector file exporter's `body` field (accessed as `log.get("body")`) carries the **entire raw RFC 5424 line** (priority + timestamp + host + appname + procid + SD + MSG) for message-less records sent by the walking-skeleton example. It does NOT contain just the MSG part. Therefore `body["stringValue"]` must NOT be mapped directly to `fields["MSG"]`; correct MSG extraction requires a scenario that sends a real bodied message via `--message` to determine the right field mapping. This is deferred to the `message_fields.feature` promotion PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
- Install-OtelCollector.ps1: write a .version marker on install, fast-path only when both the exe and a matching marker are present. A stale or hand-replaced binary now triggers a re-download instead of silently bypassing the pinned $Version / $Sha256. - run_threaded_example: wrap the prompt-protocol interaction in try/finally so an intermediate exception in wait_for_prompt / send_command / wait_for_messages no longer leaks the spawned process into later scenarios. - ExampleAppName.c: hoist EXE_SUFFIX_LENGTH to file scope so the strip arithmetic uses the named constant instead of a magic 4. - parse_otel_jsonl_line: tighten the docstring — don't claim MSG is in the returned shape when it isn't. MSG extraction is deferred to the message_fields scenario promotion (OTel's body field carries the whole raw RFC 5424 line for message-less records, so MSG extraction needs verifying against a real bodied message). - docs/bdd.md: clarify that the local Windows recipe uses bash syntax and is intended for Git Bash; PowerShell users translate or invoke bash -c. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 99% successful (✔️ 539 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Bdd/otel/Install-OtelCollector.ps1 (1)
25-33:⚠️ Potential issue | 🟠 MajorValidate the cached executable before taking the fast-path.
The
.versionmarker narrows drift, but it still doesn't prove the existingotelcol-contrib.exematches the pinned$Sha256. A hand-replaced binary with the same marker will still be trusted here, so the script can silently run an unverified oracle.🔧 Suggested fix
$exePath = Join-Path $BinDir 'otelcol-contrib.exe' $versionMarker = Join-Path $BinDir 'otelcol-contrib.version' -# Fast-path only when both the exe and a matching version marker exist; -# otherwise re-download so a stale or hand-replaced binary can't silently -# bypass the pinned $Version / $Sha256. -if ((Test-Path $exePath) -and (Test-Path $versionMarker) -and - ((Get-Content $versionMarker -Raw).Trim() -eq $Version)) +# Fast-path only when the cached binary still matches both the requested +# version and the pinned SHA-256. +if ((Test-Path $exePath) -and (Test-Path $versionMarker) -and + ((Get-Content $versionMarker -Raw).Trim() -eq $Version) -and + ((Get-FileHash -Algorithm SHA256 $exePath).Hash -eq $Sha256)) { Write-Host "otelcol-contrib v$Version already installed at $exePath" return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Bdd/otel/Install-OtelCollector.ps1` around lines 25 - 33, The fast-path should additionally validate the cached otel binary's SHA256 before returning: compute the file's hash (e.g. via Get-FileHash -Algorithm SHA256) for $exePath and compare it to the pinned $Sha256, and only short-circuit when the file exists, the $versionMarker matches $Version, and the computed hash equals $Sha256; if the hash differs, fall through to re-download and replace the binary and marker. Ensure you reference the existing variables $exePath, $versionMarker, $Version and $Sha256 and update the conditional in the Install-OtelCollector flow to include this hash equality check.
🧹 Nitpick comments (1)
Bdd/features/steps/syslog_steps.py (1)
214-223: Include child output in the nonzero-exit assertion.Line 215 already collects the process streams; keeping them and appending stderr/stdout to the failure message would make broken BDD runs much easier to debug.
♻️ Suggested refactor
try: - process.communicate(input=f"send {expected_messages}\nquit\n", timeout=15) + stdout, stderr = process.communicate( + input=f"send {expected_messages}\nquit\n", + timeout=15, + ) except subprocess.TimeoutExpired: process.kill() process.wait() raise assert process.returncode == 0, ( - f"Example binary failed with exit code {process.returncode}" + f"Example binary failed with exit code {process.returncode}\n" + f"stdout:\n{stdout}\n" + f"stderr:\n{stderr}" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Bdd/features/steps/syslog_steps.py` around lines 214 - 223, The test currently calls process.communicate(...) but doesn't capture stdout/stderr and the non-zero-exit assertion lacks child output; change the code to capture the outputs (stdout, stderr = process.communicate(...)) and in the subprocess.TimeoutExpired handler call process.kill(); process.wait(); then call process.communicate() again to obtain any remaining stdout/stderr; finally include those captured stdout and stderr in the assertion message that checks process.returncode (referencing process.communicate, subprocess.TimeoutExpired, process.kill, process.wait, and process.returncode) so failures show child output for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Bdd/features/steps/syslog_steps.py`:
- Around line 126-130: The function parse_oracle_line currently defaults any
unknown oracle_format to parse_syslog_ng_line; change it to explicitly accept
known formats and raise an error for unknown values. Update parse_oracle_line to
call parse_otel_jsonl_line when oracle_format == "otel-jsonl", call
parse_syslog_ng_line when oracle_format == "syslog-ng", and otherwise raise a
ValueError (or RuntimeError) that includes the invalid oracle_format so
misconfiguration fails fast and produces a clear error message.
---
Duplicate comments:
In `@Bdd/otel/Install-OtelCollector.ps1`:
- Around line 25-33: The fast-path should additionally validate the cached otel
binary's SHA256 before returning: compute the file's hash (e.g. via Get-FileHash
-Algorithm SHA256) for $exePath and compare it to the pinned $Sha256, and only
short-circuit when the file exists, the $versionMarker matches $Version, and the
computed hash equals $Sha256; if the hash differs, fall through to re-download
and replace the binary and marker. Ensure you reference the existing variables
$exePath, $versionMarker, $Version and $Sha256 and update the conditional in the
Install-OtelCollector flow to include this hash equality check.
---
Nitpick comments:
In `@Bdd/features/steps/syslog_steps.py`:
- Around line 214-223: The test currently calls process.communicate(...) but
doesn't capture stdout/stderr and the non-zero-exit assertion lacks child
output; change the code to capture the outputs (stdout, stderr =
process.communicate(...)) and in the subprocess.TimeoutExpired handler call
process.kill(); process.wait(); then call process.communicate() again to obtain
any remaining stdout/stderr; finally include those captured stdout and stderr in
the assertion message that checks process.returncode (referencing
process.communicate, subprocess.TimeoutExpired, process.kill, process.wait, and
process.returncode) so failures show child output for debugging.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a33e8726-f124-484a-9639-d454bb3f3d99
📒 Files selected for processing (4)
Bdd/features/steps/syslog_steps.pyBdd/otel/Install-OtelCollector.ps1Example/Common/ExampleAppName.cdocs/bdd.md
🚧 Files skipped from review as they are similar to previous changes (2)
- Example/Common/ExampleAppName.c
- docs/bdd.md
Comment on lines
+126
to
+130
| def parse_oracle_line(line, oracle_format): | ||
| """Dispatch to the right parser based on the active oracle.""" | ||
| if oracle_format == "otel-jsonl": | ||
| return parse_otel_jsonl_line(line) | ||
| return parse_syslog_ng_line(line) |
Contributor
There was a problem hiding this comment.
Fail fast on unsupported ORACLE_FORMAT.
Right now any typoed value falls through to the syslog-ng parser, which makes misconfigured Windows runs fail much later and much less clearly. Rejecting unknown formats here will make setup errors obvious.
🔧 Suggested fix
def parse_oracle_line(line, oracle_format):
"""Dispatch to the right parser based on the active oracle."""
if oracle_format == "otel-jsonl":
return parse_otel_jsonl_line(line)
- return parse_syslog_ng_line(line)
+ if oracle_format == "syslog-ng":
+ return parse_syslog_ng_line(line)
+ raise ValueError(f"Unsupported ORACLE_FORMAT: {oracle_format}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Bdd/features/steps/syslog_steps.py` around lines 126 - 130, The function
parse_oracle_line currently defaults any unknown oracle_format to
parse_syslog_ng_line; change it to explicitly accept known formats and raise an
error for unknown values. Update parse_oracle_line to call parse_otel_jsonl_line
when oracle_format == "otel-jsonl", call parse_syslog_ng_line when oracle_format
== "syslog-ng", and otherwise raise a ValueError (or RuntimeError) that includes
the invalid oracle_format so misconfiguration fails fast and produces a clear
error message.
Owner
Author
|
@coderabbitai dispositions on this review (covers the outside-diff and nitpick items that have no inline anchor): Outside-diff: try/finally on threaded helper teardown — ✅ Accepted, fixed in 016310b. Wrapped the prompt-protocol interaction in Nitpick: use EXE_SUFFIX_LENGTH constant — ✅ Accepted, fixed in 016310b. Hoisted the enum from inside Review summary
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
All items are either fixed or deferred with clear rationale. Happy to re-review once the deferred (´• ω •`) 🐇✨ |
This was referenced Apr 18, 2026
This was referenced Apr 28, 2026
This was referenced May 9, 2026
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
127.0.0.1:5514, file exporter writing JSON Lines. Pinnedv0.150.1, SHA-256 verified, idempotently installed viaBdd/otel/Install-OtelCollector.ps1.syslog_steps.pydrives both runners.EXAMPLE_BINARY/RECEIVED_LOG/ORACLE_FORMATenv vars select binary path + oracle output file + parser. Newparse_otel_jsonl_lineproduces the same flat field dict asparse_syslog_ng_line, soThensteps don't care which oracle is in use.run_exampleportable — usesprocess.communicate()and an absolute binary path. No moreselect.selecton a pipe fd (Windows doesn't support it) and no more forward-slash relative paths (WindowsCreateProcesscan't resolve them). The threaded example keeps the prompt-protocol path via the newrun_threaded_examplehelper, since its service thread is async and needs synchronisation betweensend Nandquit.ExampleAppName_Setstrips trailing.exe/.EXEso the syslog app name omits the OS executable extension regardless of build platform. TDD-driven by the failing walking-skeleton scenario which expectedSolidSyslogExamplebut was gettingSolidSyslogExample.exe. Four new unit tests cover lower/upper case, with-path, and non-.exe-extension control cases.syslog.featurepromoted out of@windows_wip.docs/bdd.mddocuments the two-oracle architecture and the local Windows setup recipe.TDD trail
app name "SolidSyslogExample"gotSolidSyslogExample.exe.Tests/Example/ExampleAppNameTest.cpp::ExeExtensionIsStripped—expected <app> but was <app.exe>.EndsWithDotExehelper and a separateappNameLengthso the borrowedargv0slice can be terminated short of the extension..execontrol) — total 8 unit tests forExampleAppName.Local end-to-end
Local gates
docker compose -f ci/docker-compose.bdd.yml up)debug(530 unit tests)sanitize(530 unit tests, ASan+UBSan)tidycppcheckcoverageformat(clang-format --Werror)msvc-debugbuild ofSolidSyslogWindowsExampleDeferred to next PR
bdd-windowsCI job onwindows-2025— packaging the now-known-good local recipe. Requires a runner with Python + the install script. Kept separate so this PR's behavioural change (oracle abstraction + walking-skeleton green) lands independently of CI plumbing.@windows_wip(header_fields,prival,timestamp,message_fields,structured_data,origin,time_quality). Some will needSTRUCTURED_DATAre-rendering inparse_otel_jsonl_line; others may need additional.exe/ Windows-portability fixes — let the failing scenarios drive each one.Refs #129
Test plan
bddjob still passes — every scenario it ran before still runs (no regressions from the step refactor)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
.exeChores