Skip to content

Commit 3092c47

Browse files
docs(codereview): add Logging Hygiene rule against object dumps in info logs
Codify the policy uncovered while toning down agent-server / SDK info logs (PRs #3216, #3217). Reviewers should flag `logger.info(...)` calls that interpolate `model_dump(...)`, `.json()`, `to_dict()`, lists, dicts, or other unbounded values — those belong in `logger.debug(...)`. Adds: - A "Logging Hygiene" subsection under SDK Architecture Conventions with concrete bad/good examples drawn from real cases in this repo. - A "What to Check" bullet pointing at the new section. Co-authored-by: openhands <openhands@all-hands.dev>
1 parent 2e3fbc8 commit 3092c47

1 file changed

Lines changed: 7 additions & 0 deletions

File tree

.agents/skills/custom-codereview-guide.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ If the updated package was uploaded **within the last 7 days**, treat it as a re
130130
- **Server-Side Cleanup**: Endpoints that create persistent state (directories, files) must have rollback logic for partial failures — see the [Server Error Handling](#server-side-error-handling) section below
131131
- **Cross-File Data Flow**: When new code calls existing APIs (constructors, factory methods), trace 1–2 levels into those APIs to verify the caller uses them correctly. Bugs often hide at layer boundaries where the caller's assumptions don't match the callee's behavior
132132
- **Secret Serialization**: Fields that carry secrets must use `serialize_secret()` from `openhands.sdk.utils.pydantic_secrets`. For `dict[str, str]` secret fields, wrap each value in `SecretStr` and call `serialize_secret` per value. Do not hand-roll redaction logic (e.g. custom sentinels or inline `expose_secrets` checks) in field serializers
133+
- **Info-Log Payloads**: `logger.info(...)` must not dump objects, dicts, or variable-length lists — see [Logging Hygiene](#logging-hygiene)
133134

134135
## Event Type Deprecation - Critical Review Checkpoint
135136

@@ -221,6 +222,12 @@ When reviewing server endpoints that create conversations or persistent artifact
221222
2. Check that subsequent operations are wrapped in try/except with cleanup.
222223
3. For client-supplied IDs, verify there's a duplicate check before creating state (return 409 Conflict if taken).
223224

225+
### Logging Hygiene
226+
227+
`logger.info(...)` must not interpolate `model_dump(...)`, `.json()`, `to_dict()`, a list/dict of tool/skill/server names, or arbitrary user-supplied values. Log a count and/or id; move full payloads to `logger.debug(...)`.
228+
229+
When reviewing a new or changed `logger.info(...)` call: if any interpolated value is an object, a dict, or a list whose size scales with load (tools, skills, conversations, requests), flag it.
230+
224231
## What NOT to Comment On
225232

226233
Do not leave comments for:

0 commit comments

Comments
 (0)