feat: S08.03 slice 6 cmdline -> set translation in target_driver (untag prival, message_fields, udp_mtu Full delivery)#307
Conversation
…nslation Land the cmdline -> `set` bridge in the BDD target driver and untag prival.feature on the FreeRTOS-on-QEMU runner. - target_driver: spawn no longer raises on FreeRTOS extra_args. New apply_extra_args helper walks --flag/value pairs after the prompt is up and writes `set NAME VALUE` lines to the UART. Unknown flags / odd-length args lists raise ValueError so a misconfigured scenario fails loudly rather than silently no-op'ing. - _run_with_prompt_protocol: invokes apply_extra_args after wait_for_prompt; Linux/Windows is a no-op there. - main.c::OnSet: drop the parsed > 23U / parsed > 7U upper-bound rejections on facility/severity. Mirrors the Linux example's atoi-and-cast so the library is the single authority on what's valid (out-of-range encodes as the internal-error PRIVAL 43). - prival.feature: drop the feature-level @freertoswip; all six scenarios pass against syslog-ng-freertos. Refs #306, #268. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dp_mtu Full delivery Extend the cmdline -> `set` translation table so msgid/message flags reach the FreeRTOS example, and grow the UART line buffer + g_msg storage so a single `set msg <body>` line can carry a full path-MTU-class message body. - target_driver: --msgid -> set msgid, --message -> set msg. - ExampleInteractive: MAX_LINE_LENGTH 256 -> SOLIDSYSLOG_MAX_MESSAGE_SIZE so fgets doesn't split a `set msg <372-byte UTF-8 body>` line across reads. The HandleSet name[] inherits the same size; future work may decouple the name buffer (always short) from the line buffer. - main.c: g_msg 256 -> SOLIDSYSLOG_MAX_MESSAGE_SIZE so the storage matches the library's bound and the BDD UTF-8 path-MTU test fits. INTERACTIVE_TASK_STACK_DEPTH *32 -> *40 to absorb the ~4 KB peak added by the larger line + name frames. - message_fields.feature: drop the feature-level @freertoswip; tag the Complete RFC 5424 scenario individually (still asserts system hostname / current timestamp / process PID). - udp_mtu.feature: drop the feature-level @freertoswip; tag the Oversize scenario individually (UDP path-MTU clipping depends on EMSGSIZE semantics that FreeRTOS-Plus-TCP doesn't currently surface). After this slice the FreeRTOS BDD sweep is 4 features / 10 scenarios passing, up from the 1-scenario walking-skeleton baseline. Refs #306, #268. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The longer SOLIDSYSLOG_MAX_MESSAGE_SIZE bracket pushes clang-format's alignment column for the g_* statics; the surrounding g_host / g_port / g_endpointVersion lines need the same extra padding to stay column-consistent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a post-prompt helper to translate BDD ChangesFreeRTOS BDD cmdline → set translation feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Bdd/features/steps/target_driver.py (1)
100-134: 💤 Low valueConsider adding
strict=Trueto the zip call for defensive programming.While the odd-length check at line 118 already prevents mismatched flag/value pairs, adding
strict=Trueto the zip at line 123 would make the code more robust against future refactoring and would be consistent with modern Python best practices (PEP 618).🔧 Proposed enhancement
- for flag, value in zip(extra_args[0::2], extra_args[1::2]): + for flag, value in zip(extra_args[0::2], extra_args[1::2], strict=True):Note: This requires Python 3.10+. If the BDD environment uses an earlier version, skip this change.
🤖 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 `@Bdd/features/steps/target_driver.py` around lines 100 - 134, The apply_extra_args function currently zips flags and values with zip(extra_args[0::2], extra_args[1::2]) which silently truncates if lengths ever mismatch; update that call to use zip(..., strict=True) so Python will raise a ValueError on any length mismatch and make the check defensive, keeping the existing odd-length pre-check; reference the symbols apply_extra_args and _FREERTOS_SET_TRANSLATION when locating the change and ensure your environment runs Python >=3.10 before applying strict=True.
🤖 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 `@DEVLOG.md`:
- Around line 59-63: The MD037 issue is caused by the plain `*32`/`*40` being
parsed as emphasis; update the DEVLOG entry mentioning
INTERACTIVE_TASK_STACK_DEPTH and HandleSet (and the `name[]` reference) to treat
those numeric values as literal text by wrapping them in code spans (e.g.,
`*32`, `*40`) or escaping the asterisks, so the markdown linter no longer
interprets them as emphasis while preserving the original wording about stack
size and headroom.
---
Nitpick comments:
In `@Bdd/features/steps/target_driver.py`:
- Around line 100-134: The apply_extra_args function currently zips flags and
values with zip(extra_args[0::2], extra_args[1::2]) which silently truncates if
lengths ever mismatch; update that call to use zip(..., strict=True) so Python
will raise a ValueError on any length mismatch and make the check defensive,
keeping the existing odd-length pre-check; reference the symbols
apply_extra_args and _FREERTOS_SET_TRANSLATION when locating the change and
ensure your environment runs Python >=3.10 before applying strict=True.
🪄 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: b6a2da9f-e4dc-4882-9140-db4197353036
📒 Files selected for processing (8)
Bdd/features/message_fields.featureBdd/features/prival.featureBdd/features/steps/syslog_steps.pyBdd/features/steps/target_driver.pyBdd/features/udp_mtu.featureDEVLOG.mdExample/Common/ExampleInteractive.cExample/FreeRtos/SingleTask/main.c
💤 Files with no reviewable changes (1)
- Bdd/features/prival.feature
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1105 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
correct escaping of literal * in emphasis section Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1105 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
1 participant
Closes #306. Refs #268.
Summary
apply_extra_argshelper in the BDD target driver: on FreeRTOS, translates--flag valuecmdline pairs intoset NAME VALUEUART writes after the prompt is up; Linux/Windows is a no-op (flags already in argv from spawn). Translation table covers exactly the four flags@udpfeatures pass torun_example(--facility,--severity,--msgid,--message); unknown flags raise loudly with a Python traceback.OnSetloosens facility/severity input validation (dropsparsed > 23U/parsed > 7Urejections) so out-of-range values reach the library and encode as the internal-error PRIVAL 43, mirroring Linux'satoi-and-cast.MAX_LINE_LENGTHandg_msggrow from 256 toSOLIDSYSLOG_MAX_MESSAGE_SIZE(2048) so a singleset msg <body>line can carry a full path-MTU-class UTF-8 body.INTERACTIVE_TASK_STACK_DEPTH*32 → *40 to absorb the larger line + name frames.@freertoswipfromprival.feature,message_fields.feature,udp_mtu.feature; keep scenario-level@freertoswiponmessage_fields::Complete RFC 5424(system identity / current time) andudp_mtu::Oversize(UDP-stack EMSGSIZE semantics).Test plan
cmake --build --preset freertos-cross --target SolidSyslogFreeRtosSingleTaskclean.behave --tags='not @wip and not @freertoswip and @udp' Bdd/features/againstsyslog-ng-freertos— 4 features pass, 10 scenarios pass, 36 skipped, 0 failed.clang-format --dry-run --Werroron every changed C file — clean.bdd-linux-syslog-ngregression — Linux suite must remain green (this PR's only Linux-visible touch isapply_extra_args, a no-op whentarget != "freertos").bdd-freertos-qemu— should report 10 passing scenarios.analyze-tidy,analyze-cppcheck,analyze-iwyu,analyze-format,build-linux-gcc/clang,sanitize-linux-gcc,coverage-linux-gcc— Linux-side static analysis, not runnable from the freertos-target devcontainer.build-windows-msvc,bdd-windows-otel— Windows-side regression.What stays tagged
@freertoswipsyslog.feature(single scenario)header_fields::Hostname,::Process IDmessage_fields::Complete RFC 5424timestamp.featurestructured_data.featuremetaSD wired in the FreeRTOS exampleorigin.featuretime_quality.featureudp_mtu::Oversizebuffered.feature🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests / Chores