Repository navigation
CPD: remove the duplicated sensor record and response code, gate cpd-check (#115) - #122
Merged
Conversation
CPD reported 9 duplicated blocks (490 lines at 100 tokens), almost all of them the Full, Compact and Event-Only sensor records copying each other. - AbstractSensorRecord holds the fields shared by the three sensor records (owner, entity, type, reading type, direction, ID string, capabilities and units, record sharing) and decodes them; the subclasses only give their byte offsets. Full keeps its linearization and thresholds. - IpmiCommandCoder.validateResponse() replaces the response checks copied in 16 commands. This fixes three copy-pasted error messages that named the wrong command (ReadFruData, SetSessionPrivilegeLevel, ChassisControl). - Sensor and GetSensorsRunner read the shared fields through the base class instead of branching on Full and Compact. - cpd-check runs at verify for duplications of 100 tokens or more; the site report still lists those of 50 tokens or more. Decoding tests for the three record types were written against the previous code first, so the refactoring is checked against the old behavior. Verified live on a Lenovo IMM and a GIGABYTE BMC: same sensors, states, thresholds and FRUs as main, apart from reading drift. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Running `mvn license:update-file-header` (as AGENTS.md asks for every new file) rewrote the copyright line of all 278 source files: - the parent POM sets canUpdateCopyright, so every "Copyright 2023 Verax Systems, MetricsHub" became "Copyright 2023 - <current year> MetricsHub", dropping the original author; - it also sets canUpdateDescription, and on Windows the plugin compares the description with the CRLF system line separator while the sources use LF, so every header looks outdated and is re-rendered (as "Copyright (C) ..." when the copyright cannot be updated). Both flags are now false: the plugin adds the header to new files and leaves existing ones alone unless the license text changes. Checked on the whole tree: 279 headers untouched, a new file gets a MetricsHub header. README and AGENTS.md now state the copyright convention (Verax Systems stays the copyright holder of the files derived from its code) and that the Verax library, published under the GPL v3, is used under a commercial non-GPL license granted to Sentry Software in 2021. Co-Authored-By: Claude Opus 5.5 <[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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d30696f8c2
ℹ️ 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".
The upgrade notes now describe the new superclass of the Full, Compact and Event-Only sensor records, the defaults returned by the getters a record type does not define, and IpmiCommandCoder.validateResponse(). Co-Authored-By: Claude Opus 5.5 <[email protected]>
bertysentry
deleted the
115-cpd-remove-the-421-duplicated-lines-shared-by-fullsensorrecord-compactsensorrecord-and-eventonlyrecord
branch
October 7, 2026 21:38
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
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.
Closes #115.
Duplicated code (first commit)
CPD reported 9 duplicated blocks (490 lines at 100 tokens), almost all of them the three sensor records copying each other.
AbstractSensorRecord(new) holds whatFullSensorRecord,CompactSensorRecordandEventOnlyRecordshare: sensor owner, entity, sensor type, event/reading type, direction, ID string, capabilities and units, record sharing. It decodes them; each subclass only gives its byte offsets, andFullSensorRecordkeeps its linearization and thresholds. The three classes go from 1,356 lines to 478, plus 502 in the base class (mostly accessor Javadoc, which the copies lacked).IpmiCommandCoder.validateResponse()(new, protected) replaces the response checks copied in 16 commands (DeactivatePayloadkeeps its own, it tolerates a specific completion code). This fixes three copy-pasted error messages that named the wrong command:ReadFruDatasaid "Get SDR Repository Info",SetSessionPrivilegeLevelsaid "Get SEL Entry",ChassisControlsaid "Get Chassis Status". The message now names the command class.SensorandGetSensorsRunnerread the shared fields through the base class instead of branching on Full and Compact.cpd-checkruns atverifyfor duplications of 100 tokens or more.Notes for review:
minimumTokens=50, but its list of 9 blocks matches the 100-token default. At 50 tokens, 35 smaller duplications remain (611 lines, mostly the session state machine and the SEL/SDR command coders), outside this issue's scope. The gate is at 100 and the site report still lists 50+.FullSensorRecordnow also exposes the record-sharing getters (share count 0, no instance modifier) andEventOnlyRecordthe unit getters (null); the Javadoc says so. All existing getters and setters keep their signatures.FullSensorRecordformula bugs (accuracy exponent, linearization order, tolerance) are untouched, they belong to FullSensorRecord decoding: thresholds gated on the wrong byte, incomplete linearization, precedence bug in accuracy exponent, tolerance #83.License headers (second commit)
mvn license:update-file-header, which AGENTS.md asks to run for every new file, rewrote the copyright line of all 278 source files. The parent POM setscanUpdateCopyright(every "Copyright 2023 Verax Systems, MetricsHub" became "Copyright 2023 - 2026 MetricsHub") andcanUpdateDescription(on Windows the CRLF system line separator makes every header look outdated, and it gets re-rendered as "Copyright (C) ..."). Both are nowfalse: the plugin adds headers to new files and leaves existing ones alone. The same parent settings probably affect the other projects built onoss-parent.README and AGENTS.md now state the copyright convention (Verax Systems stays the copyright holder of the files derived from its code) and that the Verax library, published under the GPL v3, is used under the commercial non-GPL license granted to Sentry Software in 2021.
Testing
mvn verify(JDK 17): 33 tests pass, 0 Checkstyle, PMD and CPD clean.mvn sitebuilds; SpotBugs reports nothing new in the changed files.SensorRecordTestcases decode a Full, a Compact and an Event-Only record field by field. They were written and passed against the previous code before the refactoring. NewIpmiCommandCoderTestcovers a successful response, an error completion code and a response to another command.mainand from this branch:D0h): identical output. Both builds occasionally timed out during the FRU read when the BMC drops a reply (300 s per-message timeout, A single lost UDP response stalls collection for 300 s; per-message timeout is not configurable from IpmiClientConfiguration #77); re-runs passed.update-file-headeron the whole tree leaves the 279 existing headers untouched and adds a MetricsHub header to a new file.🤖 Generated with Claude Code