Repository navigation
Don't abort the SDR repository walk on OEM or undecodable records - #112
Merged
Merged
Conversation
Fixes #76. populateSensorRecord threw IllegalArgumentException for any SDR record type it does not model, and the runners only advanced nextRecId after a successful decode, so a single unknown record killed both FRU and sensor collection. IPMI 2.0 reserves C0h-FFh for OEM records and vendors use values above C0h (19 of 85 records are D0h on a GIGABYTE ME62-GE0-00). - SensorRecord: map every record type >= C0h to OemRecord. - OemRecord: tolerate records shorter than the C0h layout. - AbstractIpmiRunner: advance nextRecId before decoding, log and skip a record that fails to decode, and bound the header copy in the chunked path by the record size. Verified live: chassis, 2 FRUs and 62 sensors now collected from the GIGABYTE BMC that previously failed in under 200 ms; Lenovo IMM unchanged. Co-Authored-By: Claude Fable 5.1 <[email protected]>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6f3487656
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Only the C0h OEM record defines a 3-byte manufacturer ID followed by OEM data. Record types C1h-FFh, which populateSensorRecord now also maps to OemRecord, have a vendor-defined layout: interpreting bytes 5-7 as a manufacturer ID fabricated an ID and dropped the first three payload bytes. For those types the manufacturer ID stays 0 and getOemData() returns the entire type-specific payload. Co-Authored-By: Claude Fable 5.1 <[email protected]>
…AUDE.md Same convention as jawk: CLAUDE.md only imports AGENTS.md. The instructions describe this repository as it is today (Java 8 target, license headers, no formatter yet, ungated quality reports and the workaround for SpotBugs on recent JDKs) plus the IPMI-specific lessons from the October 2026 review: never abort an SDR or FRU walk on an unknown record, keep BMC credentials out of the repository, and expect dropped UDP replies when testing against real hardware. Co-Authored-By: Claude Fable 5.1 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7aac625fb6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…rd tests Some BMCs answer a whole-record Get SDR with a success code but fewer bytes than the record length declared in the SDR header. Advancing to the next record before decoding would have turned such a record into a silent skip, so the runner now detects the short reply and uses the existing chunked read while the record ID is still known. The chunked path also tolerates a BMC returning fewer bytes than requested instead of throwing ArrayIndexOutOfBounds. New unit tests cover the OEM decoding path (vendor-defined types keep their whole payload, C0h parses the manufacturer ID, short records do not throw, reserved types are still rejected), the runner's skip behaviour for undecodable records, and the truncation check. AGENTS.md now states that the LGPL header requirement applies to the Java sources under src/main/java only, which is what the license plugin checks; tests, Markdown and resources carry no header. Co-Authored-By: Claude Fable 5.1 <[email protected]>
This was referenced Oct 7, 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.
Fixes #76.
What changed
SensorRecord.populateSensorRecord: every record type ≥0xC0now maps toOemRecordinstead of throwingIllegalArgumentException("Invalid record type"). IPMI 2.0 reservesC0h–FFhfor OEM records and vendors use values aboveC0h(19 of the 85 SDR records on a GIGABYTE ME62-GE0-00 BMC areD0h).OemRecord.populateTypeSpecficValues: tolerates records shorter than theC0hlayout (3-byte manufacturer ID + data) so vendor-specific types cannot triggerNegativeArraySize/ArrayIndexOutOfBounds.AbstractIpmiRunner:nextRecIdis advanced before decoding in both the whole-record and chunked paths, and a record that fails to decode is logged at WARN and skipped (decodeRecord) instead of aborting the walk. The header copy ingetSensorViaChunksis bounded by the record size.Why
A single SDR record the library does not model killed the entire repository walk:
IpmiClient.getFrus()andIpmiClient.getSensors()both failed in under 200 ms withExecutionException: Invalid record type: -48on the BMC above, so no FRU and no sensor was ever collected from it. The deprecated0x14record type has the same effect and is now skipped too.Verification
mvn test: 13 tests pass.OemRecordwith their raw bytes.0xC0OEM record): output unchanged.Also in this PR
CLAUDE.md(a single@AGENTS.mdimport, as in jawk) andAGENTS.mdwith the project instructions for AI agents: Java 8 target, license headers, current (ungated) quality reports and the SpotBugs workaround on recent JDKs, plus the IPMI-specific rules learned during the review (never abort an SDR/FRU walk on an unknown record, no BMC credentials in the repo, expect dropped UDP replies when testing against hardware).Reviewer notes
getSensorData/getSensorViaChunksalready useinstanceofchecks, so anullreturn for a skipped record is harmless inGetFrusRunnerandGetSensorsRunner.RateUnit7,ModifierUnitUsage3) now drop only the affected record thanks to the catch indecodeRecord; making them graceful is tracked in Reserved enum values throw and abort processing (RateUnit 7, ModifierUnitUsage 3, PowerRestorePolicy 3, record type 0x14) #87.🤖 Generated with Claude Code