feat: structured JSON logging via VOUCH_LOG_FORMAT=json#114
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds configurable structured JSON logging via VOUCH_LOG_FORMAT=json: a new logging_config module (JsonFormatter, selection, configure_logging) is idempotently wired into CLI, JSONL, and MCP server startups, documented in CHANGELOG, and covered by tests validating output and handler behavior. ChangesJSON Logging Configuration and Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 1
🧹 Nitpick comments (1)
tests/test_logging.py (1)
66-69: ⚡ Quick winAdd a regression test for falsy
eventoverride values.Current coverage validates only truthy override values; a falsy-case test will guard the override semantics.
Proposed test addition
def test_json_formatter_event_override_wins_over_message(): line = _emit({"event": "kb.approve"}) payload = json.loads(line) assert payload["event"] == "kb.approve" +def test_json_formatter_event_override_allows_empty_string(): + line = _emit({"event": ""}) + payload = json.loads(line) + assert payload["event"] == ""🤖 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 `@tests/test_logging.py` around lines 66 - 69, Add a regression test that verifies falsy "event" override values win over the message fallback: create a test (e.g., test_json_formatter_event_override_allows_falsy_values) that calls _emit({"event": ""}) and asserts payload["event"] == "" (and optionally repeat for other falsy values like 0 and False), ensuring the json formatter uses the provided event value even when it's falsy rather than falling back to the message.
🤖 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 `@src/vouch/logging_config.py`:
- Line 59: The current event override uses a falsy-coalescing pattern that
treats explicit falsy values as absent; change the "event" assignment in the
logging_config's formatter/dict creation so it checks key presence instead of
truthiness (i.e., if "event" is a key in record.__dict__ use
record.__dict__["event"] even if falsy, otherwise use record.getMessage());
update the code path where the dict entry "event": ... is built (the line using
record.__dict__.get("event") or record.getMessage()) to perform an explicit
key-existence test and return the message only when the key is not present.
---
Nitpick comments:
In `@tests/test_logging.py`:
- Around line 66-69: Add a regression test that verifies falsy "event" override
values win over the message fallback: create a test (e.g.,
test_json_formatter_event_override_allows_falsy_values) that calls
_emit({"event": ""}) and asserts payload["event"] == "" (and optionally repeat
for other falsy values like 0 and False), ensuring the json formatter uses the
provided event value even when it's falsy rather than falling back to the
message.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 02ece305-70b2-4a8b-8b18-569b9e6cafd0
📒 Files selected for processing (6)
CHANGELOG.mdsrc/vouch/cli.pysrc/vouch/jsonl_server.pysrc/vouch/logging_config.pysrc/vouch/server.pytests/test_logging.py
Khaostica
force-pushed
the
fix/97-vouch-log-format-json
branch
from
ceffeba to
92b597b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_logging.py (1)
95-132: ⚡ Quick winAdd explicit no-op tests for
VOUCH_LOG_FORMAT=""and"text"Nice coverage overall, but the contract includes both empty-string and explicit
textas no-op modes; adding those two cases would lock in the acceptance criteria and prevent regressions in format selection behavior.🤖 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 `@tests/test_logging.py` around lines 95 - 132, Add two explicit no-op tests for the empty-string and explicit "text" VOUCH_LOG_FORMAT values: call configure_logging() after monkeypatch.setenv("VOUCH_LOG_FORMAT", "") and after monkeypatch.setenv("VOUCH_LOG_FORMAT", "text"), assert the returned selected value is "text", that the logger retrieved with logging.getLogger(VOUCH_LOGGER_NAME) has no handlers with attribute _vouch_managed, and that logger.propagate is True; use the same conventions as the existing tests (referencing configure_logging, VOUCH_LOG_FORMAT, VOUCH_LOGGER_NAME, and JsonFormatter) so these behaviors are locked into the test suite.
🤖 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.
Nitpick comments:
In `@tests/test_logging.py`:
- Around line 95-132: Add two explicit no-op tests for the empty-string and
explicit "text" VOUCH_LOG_FORMAT values: call configure_logging() after
monkeypatch.setenv("VOUCH_LOG_FORMAT", "") and after
monkeypatch.setenv("VOUCH_LOG_FORMAT", "text"), assert the returned selected
value is "text", that the logger retrieved with
logging.getLogger(VOUCH_LOGGER_NAME) has no handlers with attribute
_vouch_managed, and that logger.propagate is True; use the same conventions as
the existing tests (referencing configure_logging, VOUCH_LOG_FORMAT,
VOUCH_LOGGER_NAME, and JsonFormatter) so these behaviors are locked into the
test suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 809a8284-bc71-4577-8bfa-48ff1b39a4d3
📒 Files selected for processing (6)
CHANGELOG.mdsrc/vouch/cli.pysrc/vouch/jsonl_server.pysrc/vouch/logging_config.pysrc/vouch/server.pytests/test_logging.py
✅ Files skipped from review due to trivial changes (1)
- src/vouch/jsonl_server.py
Khaostica
force-pushed
the
fix/97-vouch-log-format-json
branch
from
92b597b to
ddc51a4
Compare
|
@Khaostica , what's the meaning of this??!! |
|
Close it up immediately 😂 |
|
@Tet-9 looks like we were working on this in parallel — you opened #113 about an hour before I pushed this. Happy to defer to whoever the maintainer picks, but leaving #114 open since it emits event/actor/object_ids per the acceptance criteria on #97 and scopes the handler to the vouch logger namespace (so library logs aren't reformatted), which #113 doesn't currently do. |
|
@plind-junior Could you approve the workflow run? |
Lollllll |
Although you're right 👍, we should leave it up to who the maintainer picks; survival of the fittest. |
|
PR should target test branch |
Khaostica
force-pushed
the
fix/97-vouch-log-format-json
branch
from
ddc51a4 to
ef6485b
Compare
…chdev#97) Adds a `vouch.logging_config` module with a `JsonFormatter` and an idempotent `configure_logging()` that the CLI, MCP server, and JSONL server entry points call at startup. When `VOUCH_LOG_FORMAT=json`, the `vouch` logger emits one JSON object per line with `level`, `logger`, `event`, and any structured extras (`actor`, `object_ids`, ...) passed via the stdlib `extra=` argument. Any other value (including unset) is a no-op - vouch's loggers keep stdlib default behaviour, so callers who don't opt in see no change. Scoped to the `vouch` logger namespace with propagate=False in JSON mode, so this never reformats logs emitted by libraries vouch happens to depend on.
Khaostica
force-pushed
the
fix/97-vouch-log-format-json
branch
from
ef6485b to
7c8bd79
Compare
|
@plind-junior Could you approve the workflow? |
|
Really nice — the env-var contract was sitting in our docs unimplemented, and you closed it cleanly. Two small things I want to call out as genuinely well done: writing to stderr (so the stdio MCP / JSONL server's stdout stream stays uncontaminated) and One tiny non-blocker — the Small process note: this is pointed at Thanks for taking it on! 🙏 |
|
Thanks for the thorough review! 🙏 On the process note: yes, targeting On the handler: good call — I've adopted the subclass. There's now a small |
Replace the _vouch_managed attribute (and its type: ignore[attr-defined]) with a _VouchManagedHandler(StreamHandler) subclass. configure_logging() identifies its handler by isinstance for install/reuse/remove, which reads cleaner and drops the attribute hack. Tests updated to match.
…LEVEL/FILE, and LoggingConfig return type Combines the best of both PR vouchdev#113 and vouchdev#114 into a superior implementation: From vouchdev#114 (Khaostica): - Scoped vouch logger namespace instead of root logger - _STDLIB_RECORD_FIELDS explicit whitelist for extra= merging - event key with caller override via extra={event: ...} - sort_keys=True, default=str in json.dumps - propagate=False in json mode to prevent double-emit - _selected_format() helper Added on top: - time field in JSON output (ISO 8601) - VOUCH_LOG_LEVEL env var: sets vouch logger level (default WARNING) - VOUCH_LOG_FILE env var: appends logs to file alongside stderr - Sensitive field redaction: keys containing secret/token/password/key are replaced with ***REDACTED*** so credentials never reach log sinks - LoggingConfig named tuple return type for testability and introspection - force=True parameter for clean reconfiguration in tests - CONTRIBUTING.md: documents all env vars, logging guidelines, and managed-handler pattern - 11 tests covering all new behaviour Fixes vouchdev#97
…LEVEL/FILE, and LoggingConfig return type Combines the best of both PR vouchdev#113 and vouchdev#114 into a superior implementation: From vouchdev#114 (Khaostica): - Scoped vouch logger namespace instead of root logger - _STDLIB_RECORD_FIELDS explicit whitelist for extra= merging - event key with caller override via extra={event: ...} - sort_keys=True, default=str in json.dumps - propagate=False in json mode to prevent double-emit - _selected_format() helper Added on top: - time field in JSON output (ISO 8601) - VOUCH_LOG_LEVEL env var: sets vouch logger level (default WARNING) - VOUCH_LOG_FILE env var: appends logs to file alongside stderr - Sensitive field redaction: keys containing secret/token/password/key are replaced with ***REDACTED*** so credentials never reach log sinks - LoggingConfig named tuple return type for testability and introspection - force=True parameter for clean reconfiguration in tests - CONTRIBUTING.md: documents all env vars, logging guidelines, and managed-handler pattern - 11 tests covering all new behaviour Fixes vouchdev#97
3 participants
What changed
Implements
VOUCH_LOG_FORMAT=json- already documented inROADMAP.mdandadapters/generic-mcp/README.md, but never wired up. New modulesrc/vouch/logging_config.pyexposes aJsonFormatterand an idempotentconfigure_logging(); the CLI (vouch.cli:cli), MCP server (vouch.server:run_stdio), and JSONL server (vouch.jsonl_server:run_jsonl) entry points each call it once at startup.When
VOUCH_LOG_FORMAT=json, vouch attaches a JSON handler to thevouchlogger namespace withpropagate=False. Each log record is emitted as one JSON object per line withlevel,logger,event, and any structured extras the caller attached via the stdlibextra=argument (notablyactorandobject_ids, to mirror the audit-log shape).Any other value of
VOUCH_LOG_FORMAT- including unset, empty, ortext- is a no-op: vouch's loggers keep stdlib default behaviour, so callers who don't opt in see no change in routing or output.Closes #97
Why
Running vouch as a service makes its logs hard to parse without a machine-readable format.
VOUCH_LOG_FORMAT=jsonwas already advertised in ROADMAP and the generic-MCP adapter docs as a supported knob, so operators reasonably expected it to work - implementing it closes that documentation/behaviour gap.What might break
Nothing for existing users. With
VOUCH_LOG_FORMATunset (the default),configure_logging()does not touch thevouchlogger's handlers, level, or propagation - vouch's loggers continue to behave exactly as they did before. Operators who explicitly opt intojsonwill see output land on stderr as one JSON object per line; that is the documented contract.No
.vouch/file shape changes. Nokb.*method changes. No on-disk layout, bundle format, or audit-log event shape change.VEP
Not needed. This does not change the object model,
kb.*method surface, on-disk layout, bundle format, or audit-log event shape. It implements an environment-variable knob that is already documented in ROADMAP andadapters/generic-mcp/README.md.Tests
make checkpasses locally (lint + mypy + pytest)CHANGELOG.mdupdated under## [Unreleased]Detail:
make checkverified in a fresh Linux env (WSL Ubuntu, Python 3.14.4,pip install -e '.[dev]'):ruff check src testsclean,mypy srcclean ("Success: no issues found in 30 source files"),pytest95 passed / 6 skipped / 0 failed.tests/test_logging.py(9 tests) — coversJsonFormatterfield emission,extra=merging,eventoverride, exception capture, env-var-driven configuration, idempotency, the text/JSON round-trip, and end-to-end one-object-per-line output via a real stdlib logger.VOUCH_LOG_FORMAT=jsonthevouchlogger emits{"actor": "...", "event": "...", "level": "...", "logger": "...", "object_ids": [...]}to stderr; without it, plain text logging is unchanged.Summary by CodeRabbit
New Features
Tests