nsmd — Review Context for CodeRabbit (and humans)

Committed at the repo root (or docs/) and ingested by CodeRabbit via knowledge_base.code_guidelines.filePatterns. This is the repo-level context CodeRabbit lacks when it reviews a single diff. Every item below corresponds to a false-positive class found in a 6-month review audit. Keep it current — CodeRabbit uses it as durable review guidance.

Architecture facts (so reviews stop mis-assuming)

  • Production nsmd event-loop code is intended to be single-threaded. It runs one sdbusplus/asio event loop with cooperative coroutines unless a change explicitly introduces another concurrent execution path such as worker threads, callbacks on a different execution context/strand, or shared state accessed from multiple OS threads. Therefore:
    • std::localtime, static local buffers, lazy if(p==nullptr) p=make_unique<>() init, and a if(flag){...} flag=true; check-then-set are NOT data races unless there is a co_await between the check and the set, or another concrete concurrent path is visible. Do not flag them as thread-unsafe without identifying that path.
  • mockupResponder/** is a test simulator, not production firmware. Production-grade error handling, leak/null-deref hardening, and behavioral test assertions do not apply. Advertising a command in supportedCommands before its handler exists is the intended “advertise-first” pattern (the default case returns NSM_ERR_UNSUPPORTED_COMMAND_CODE).
  • **/test/** includes intentional branch-coverage / sanitizer-cleanup tests. A test whose purpose is “this code path runs without throwing / without a sanitizer error” legitimately uses EXPECT_NO_THROW, an empty catch{}, or EXPECT_NE(rc,0) without asserting a specific value.
  • libnsm/** implements the NSM (MCTP System Management API) wire protocol. Enum numeric values and field widths are spec-mandated — changing them is usually spec-alignment, not a wire break. Many decoders perform structural validation (lengths/sizes), while semantic field validation may live in encoders or callers. Check the local encoder/decoder contract before demanding symmetric semantic validation. A trailing [1] / bitfield8_t[1] member means sizeof(struct) already counts one element, so resize(sizeof(struct)+n-1) is correct.

House conventions (intentional — not defects)

  • const on by-value parameters is intentional (prevents accidental mutation of the local copy).
  • instance_id = 0 at encode time is the universal convention; the real id is assigned by the requester/InstanceIdDb on send.
  • Local static inline helpers scoped to one translation unit are fine even if they look duplicated (no ODR issue); “local vs shared helper” is a free choice, not a defect.
  • D-Bus interfaces, EM (entity-manager) config keys, and bmcweb contracts live in other repos — if a value/name looks wrong, it may be defined there; ask rather than assert.

How to review nsmd well

  • Before flagging a missing guard / missing test / wrong error type: read the class header (access specifiers like private:, and C++ default member initializers = value;) and the function's callers — most “issues” are handled just outside the diff hunk.
  • Treat pure style/lint preferences as nitpicks, never “Potential issue / Major / Critical”.
  • Verify any “✅ Addressed” claim against the latest commit; don't assume.

Maintaining this context

  • If CodeRabbit raises a one-off false positive, reply on the CodeRabbit thread with the evidence and resolve it.
  • If the same false-positive pattern repeats, update this file through a normal MR.
  • If the correction is a strict review rule or path-specific instruction, update .coderabbit.yaml.
  • If the rule applies across BMC repositories, propose it in the parent dgx/bmc/coderabbit config.

Pointers

  • NSM protocol spec: MCTP System Management API (Type 0 Device Capability Discovery, Type 3 Platform Environmentals, Type 4 Diagnostics, Type 5 Device Configuration, + Base Spec). Ask a maintainer for the current PDFs if a wire/enum/field question arises.