docs: S23.22 hold the RFC compliance matrix to the RFC text - #749
Conversation
The citations were checked against the published RFCs rather than against the matrix. Five were wrong: RFC 5426 cited §3.2 for the default port (§3.3), §3.3 for unreliable delivery (§4.1) and §4 for the absence of security (§5); RFC 6587 cited §3.2 for session initiation (§3.3) and for port 601, which it standardises nowhere — §3.3 records that the protocol "has no standardized port assignment", and 601 is the IANA syslog-conn assignment from RFC 3195. Each RFC now names the copy it was checked against, linked to rfc-editor.org with its category, and every Section cell deep-links the section it cites. All 68 anchors were verified to exist in the target documents. RFC 5424 walks its section tree from §5, so a reader can see what was covered rather than trust it: 40 rows where there were 18, with examples and registry sections carried as N/A saying why. Walking it found three requirements the matrix never claimed and two the library does not enforce — §7.1.3 syncAccuracy against isSynced, and the §7.2.3/§7.2.4 length bounds. Both obligations are now stated on the headers that carry them, and #748 tracks enforcing them. Also adds RFC 9662 §6, which forbids TLS early data. The page said 9662 stated no requirement RFC 5425 did not already frame; it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe change expands RFC-focused interface comments and updates the RFC compliance matrix with linked references and requirements for RFC 5424, RFC 5425, RFC 9662, RFC 5426, and RFC 6587. ChangesRFC documentation alignment
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 6
🤖 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 `@Core/Interface/SolidSyslogOriginSd.h`:
- Around line 39-41: Complete the caller-responsibility sentence in the
documentation comment near SolidSyslogOriginSd by explicitly stating what
callers must do with the supplied value, while preserving the existing reference
to issue `#748` and the surrounding RFC guidance.
In `@docs/rfc-compliance.md`:
- Around line 54-60: Condense the RFC compliance matrix entries around
SolidSyslogTimeQuality, SolidSyslogOriginSd, and the TLS rows to retain only the
RFC section, requirement, status, and brief cross-references. Remove duplicated
field-level and implementation obligations, linking instead to the authoritative
interface header comments and docs/tls.md; keep configuration semantics in the
relevant field comments and avoid maintaining duplicate TLS obligation lists
outside docs/tls.md.
- Line 111: Update the RFC 5426 §3.2 compliance entry to mark support as
Partial, documenting that the lwIP adapter’s LwipRawDatagram_MaxPayload returns
1232 and does not reliably report OVERSIZE before transmission. Do not claim
full fragmentation avoidance unless family-specific IPv4/IPv6 safe limits and
reliable OVERSIZE handling are implemented.
- Line 31: Update the RFC 5424 compliance entries in docs/rfc-compliance.md:
revise §6.1 to state that receivers SHOULD accept messages up to 2048 octets,
MUST support at least 480 octets, and may accept larger messages; revise §6.2.3
to describe TIMESTAMP as RFC 3339-derived with an optional 1–6 digit fractional
part, while retaining the library’s fixed six-digit output in the Notes column.
- Around line 94-95: Update the RFC 9662 §4 and §6 entries in the compliance
table to narrow their claims: qualify TLS 1.3 support and preference by the
selected backend and its configuration, without treating the absence of a
maximum-version setting as proof of preference; state only that this library’s
adapters send application data through their normal write APIs and do not send
0-RTT data, removing claims about disabled session tickets or PSK.
- Line 46: Update the RFC compliance table to replace the §6.3.4 N/A status,
mark §§7.1.3, 7.2.1, 7.2.2, and 7.3.3 as Partial, and document the §7.3.1
limitation of SolidSyslogNullAtomicCounter returning 1 for every message.
Correct the §§7.2.3 and 7.2.4 notes to state that SolidSyslogOriginSd truncates
values to 48 and 32 decoded bytes rather than preserving over-long values, then
recalculate the Summary totals.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96b91d13-8071-4294-8053-0a6e42646ae7
📒 Files selected for processing (3)
Core/Interface/SolidSyslogOriginSd.hCore/Interface/SolidSyslogTimeQuality.hdocs/rfc-compliance.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/rfc-compliance.md (1)
127-129: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the TCP port capability from the RFC 6587 requirement count.
RFC 6587 §3.3 states that the protocol has no standardised port assignment. This row describes 601 as a library/RFC 3195 default, not an RFC 6587 requirement, but marks it
Supported. Remove the row or mark itN/Aas a project capability. Exclude no-section capability rows fromTotal requirements, or rename that column toTotal assessed rows, so the summary does not overstate RFC coverage. (rfc-editor.org)🤖 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 `@docs/rfc-compliance.md` around lines 127 - 129, Update the RFC 6587 compliance table’s “Default port 601” row so it is marked N/A or removed, since it is a project/RFC 3195 capability rather than an RFC 6587 requirement. Adjust the summary count or column label accordingly so this no-section capability row is excluded from RFC requirement coverage.
🤖 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.
Outside diff comments:
In `@docs/rfc-compliance.md`:
- Around line 127-129: Update the RFC 6587 compliance table’s “Default port 601”
row so it is marked N/A or removed, since it is a project/RFC 3195 capability
rather than an RFC 6587 requirement. Adjust the summary count or column label
accordingly so this no-section capability row is excluded from RFC requirement
coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d1f8983e-d29f-4bda-b60c-d9ca86c057f8
📒 Files selected for processing (1)
docs/rfc-compliance.md
…he RFC does The §6.1 cell read "max 2048 recommended". RFC 5424 §6.1 states no upper limit, requires receivers to accept 480 octets and recommends they accept 2048 — a bound on what a receiver accepts, not on what a sender emits, so the 2048 default is a sender-side choice rather than a mandated cap. The §6.2.3 cell read "ISO 8601 with microseconds". §6.2.3 derives the format from RFC 3339 and restricts it further, and the ABNF bounds TIME-SECFRAC at 1*6DIGIT where RFC 3339 allows any number of digits. The fraction is optional; writing all six is the library's choice. Both notes were already accurate. What was loose was the paraphrase of the requirement, which is the defect this document exists to remove. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/rfc-compliance.md (2)
127-127: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the RFC 6587 default-port row.
The row states that RFC 6587 standardises no port, then marks project default
601asSupported. A project default from RFC 3195 is not an RFC 6587 requirement. Remove this row or move it to a non-RFC capability section, then exclude it from the RFC 6587 totals.Also applies to: 135-138
🤖 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 `@docs/rfc-compliance.md` at line 127, Remove the “Default port 601” entry from the RFC 6587 compliance table and any corresponding RFC 6587 totals or summary counts. Keep the project’s SOLIDSYSLOG_TCP_DEFAULT_PORT definition documented only in a separate non-RFC capability section if needed, without presenting it as RFC 6587 support.
63-63: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not recommend timestamp sorting for strict ordering.
If concurrent raises invert adjacent
sequenceIdvalues, sorting later by timestamp cannot restore originator order. Timestamps can collide or reflect different clock states. State that strict monotonic ordering requires serialised raises or an explicit ordering contract.🤖 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 `@docs/rfc-compliance.md` at line 63, Update the RFC 5424 compliance entry’s concurrency trade-off text to remove the recommendation to sort by timestamp for strict ordering. State instead that strict monotonic sequenceId ordering requires serialized raises or an explicit ordering contract, while preserving the existing explanation of concurrent reordering.
🤖 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 `@docs/rfc-compliance.md`:
- Line 28: Correct the §5 note in the RFC compliance table by replacing the
malformed phrase with grammatical wording stating that the length prefix is
added by octet-counting framing and defined by the transport mapping; preserve
the rest of the note unchanged.
---
Outside diff comments:
In `@docs/rfc-compliance.md`:
- Line 127: Remove the “Default port 601” entry from the RFC 6587 compliance
table and any corresponding RFC 6587 totals or summary counts. Keep the
project’s SOLIDSYSLOG_TCP_DEFAULT_PORT definition documented only in a separate
non-RFC capability section if needed, without presenting it as RFC 6587 support.
- Line 63: Update the RFC 5424 compliance entry’s concurrency trade-off text to
remove the recommendation to sort by timestamp for strict ordering. State
instead that strict monotonic sequenceId ordering requires serialized raises or
an explicit ordering contract, while preserving the existing explanation of
concurrent reordering.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54eff481-c915-4eba-8940-5ff67b665f1d
📒 Files selected for processing (2)
Core/Interface/SolidSyslogOriginSd.hdocs/rfc-compliance.md
The §7.2.3 and §7.2.4 rows said software and swVersion were carried as supplied and that observing the RFC bounds was the caller's. The opposite is true: SolidSyslogOriginSd writes both through SolidSyslogSdValue_BoundedString with ORIGIN_SOFTWARE_MAX = 48 and ORIGIN_SWVERSION_MAX = 32, and Tests/SolidSyslogOriginSdTest.cpp covers the max and one-past-max cases. The bound counts decoded bytes, so multi-byte UTF-8 truncates earlier than the character count §7.2.3 states, never later. The claim came from a keyword grep that could not match the named constants rather than from reading the file, which is the failure the documentation rules in CLAUDE.md exist to prevent. Also: enterpriseId is bounded at 64, a library limit and not an RFC one; the ip PARAM carries no library bound; the §6.3.4 note no longer says the rule binds this library while the row reads N/A; the early-data row claims only what was verified, that no adapter calls an early-data API, rather than asserting session tickets and PSK are disabled; the RFC 5426 §3.2 row links #736 for the platforms that cannot report oversize; and the §5 note reads. #748 is corrected to the syncAccuracy pairing alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The §7.3.1 note described a CAS loop, which is one implementation of a contract the row itself calls an abstraction, and framed concurrent raise as a reorder trade-off to be mitigated by sorting on timestamp. Both misread the field. The id is taken when a message is raised, so it defines the order messages originated in; what varies is delivery order, from several threads or from any transport that reorders, and recovering origination order by sorting on sequenceId is what the field is for. Sorting on timestamp cannot do it: two messages formatted within one tick carry the same value. The summary counted rows carrying no RFC section — the TCP port, address rotation, partial writes — as RFC 6587 requirements. They are library capabilities assessed alongside the requirements, so the column now counts rows and says so. RFC 9662 §4 asks that TLS 1.3 be preferred where implemented. A floor with no ceiling shows the library never holds a handshake below it; whether it is available belongs to the backend the integrator links. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@Core/Interface/SolidSyslogOriginSd.h`:
- Around line 45-47: Update the EnterpriseId interface comment to explicitly
state that supplying an IANA-registered private enterprise number is the
caller’s obligation to observe, while preserving the existing NULL behavior,
truncation limit, and lack of form validation.
- Around line 38-43: Update the Software and swVersion interface comments to
distinguish the library’s decoded-byte truncation limits from RFC 5424’s 48- and
32-character limits, stating that multibyte UTF-8 may be truncated by the
library before reaching the RFC character maximums. Verify the wording against
the implementation and the published RFC before asserting the limits.
In `@docs/rfc-compliance.md`:
- Line 94: Revise the RFC 9662 §4 row in the compliance matrix so its TLS 1.3
“Supported” claim is qualified by the linked backend and its configuration. Do
not treat the absence of a maximum-version setting as evidence of TLS 1.3
support or preference; explicitly describe unsupported or unverified cases
instead.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 40afe7e8-93ec-46a1-828c-a26b5324e7c5
📒 Files selected for processing (2)
Core/Interface/SolidSyslogOriginSd.hdocs/rfc-compliance.md
The field comments said software and swVersion were "truncated to the 48/32 bytes RFC 5424 allows". The RFC allows 48 and 32 *characters*; the decoded-byte bounds are this library's, and for multi-byte UTF-8 they truncate earlier. The compliance matrix already said this correctly, so the header was the one out of step. enterpriseId now states the IANA registration as the caller obligation it is rather than implying it through "not checked". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
☀️ Quality Summary Created by Quality Monitor v4.15.0 (#82d77af). More details are shown in the GitHub Checks Result. |
1 participant
Purpose
Part of #708.
The RFC compliance matrix is the document an integrator or an auditor reaches for
first, and its citations had never been checked against the RFCs themselves. They
have now, section by section, against the text published at rfc-editor.org.
Change Description
Five citations were wrong. RFC 5426 cited §3.2 for the default port (it is
§3.3, Source and Target Ports), §3.3 for unreliable delivery (§4.1, Lost
Datagrams), and §4 for the absence of authentication, integrity and
confidentiality (§5, Security Considerations). RFC 6587 cited §3.2 for session
initiation (§3.3), and §3.2 for "default port 601" — which cannot be repaired,
only removed: §3.3 records that the protocol "has no standardized port
assignment", §4 that operators must select one per deployment, and the string 601
appears nowhere in the RFC. It is the IANA
syslog-connassignment from RFC 3195,which is what the row now says.
Each RFC names the copy it was checked against, linked to rfc-editor.org with
its category — Standards Track for 5424, 5425, 5426 and 9662, Historic for 6587,
each read from the RFC's own header. "RFC 6587 is Historic" therefore comes out of
a row note, where it was a second home for the same fact.
Every Section cell deep-links its section. All 68 links were verified to
resolve against the fetched RFC texts. Worth knowing for later: the
docs-linkslychee lane checks liveness, not fragments, so this verification is the thing to
repeat if sections are ever re-cited.
RFC 5424 walks its section tree from §5 — 40 rows where there were 18 — so
completeness is visible rather than assumed. Example and registry sections appear
as N/A rows stating that they carry no requirement, rather than being omitted
silently.
That walk found requirements the matrix had never claimed, including §5.1's
"implementations MUST support a TLS-based transport", §6.2's seven-bit ASCII
HEADER, and the per-parameter rules under §7.1 to §7.3. It also found two
constraints the API lets a caller break, both verified in code and now raised as
#748 for 0.2.0:
syncAccuracywhenisSyncedis 0.TimeQualitySd_Formatdecides on the sentinel alone and never consults
IsSynced.softwareat 48 characters andswVersionat 32.Neither is enforced anywhere in
SolidSyslogOriginSd.Both obligations are now stated on the headers that carry the fields, so the rows
read Supported on the same basis the §6.3.2 row already uses for the SD-NAME
exclusions: the library frames and escapes, and the contract states what the
caller must observe. The §7.1.3 note is explicit that this one is runtime state
rather than fixed configuration, which is why #748 matters more there.
RFC 9662 §6 is added — "implementations MUST NOT use early data". The page
previously asserted that RFC 9662 stated no requirement RFC 5425 did not already
frame; it does, and RFC 5425 predates TLS 1.3 entirely. No adapter references
early data, session tickets or PSK, so no pre-shared key can exist for 0-RTT to
use, and the row is Supported on that basis.
Test Evidence
Documentation and header comments only — no behaviour changes, so no unit tests
were added or modified.
previous matrix or the working notes. One claim in those notes — that RFC 6587
§3.2 was wrong for session initiation — turned out to be incorrect, since §3.2
does say the sender initiates; it was sharpened to §3.3 rather than corrected.
#section-anchors verified present in the target RFCs.syncAccuracybranch in
Core/Source/SolidSyslogTimeQualitySd.c, and the absence of anylength logic in
SolidSyslogOriginSd.{c,Private.h,Static.c}.mkdocs build --strictexits 0; markdownlint-cli2 v0.22.1 reports 0 errors.clang-format --dry-run --Werrorclean on both headers.misra_renumber.pyproposes no renumbering: the one suppression in
SolidSyslogTimeQuality.hannotates line 15 and the edit begins at line 25.
Areas Affected
docs/rfc-compliance.md, plus doc comments onCore/Interface/SolidSyslogTimeQuality.hand
Core/Interface/SolidSyslogOriginSd.h. No executable change.RFC 5425, 5426 and 6587 have not been tree-walked yet — 5426 is missing §3.4, §3.5,
§3.6 ("Syslog senders MUST NOT disable UDP checksums"), §4.3 and §4.4; 5425 is
missing §4.2.2, §4.3 and the §5.x authorization policies. That stays on #708 for a
follow-up PR.
Summary by CodeRabbit