Skip to content

feat(observability): add trusted trace correlation - #64779

Open
zepdevelopers wants to merge 1 commit into
NousResearch:mainfrom
zepdevelopers:main
Open

feat(observability): add trusted trace correlation#64779
zepdevelopers wants to merge 1 commit into
NousResearch:mainfrom
zepdevelopers:main

Conversation

@zepdevelopers

Copy link
Copy Markdown

Summary

  • add disabled-by-default trusted-edge observability correlation config
  • derive a strict environment-only K1 HMAC identifier on Langfuse root traces
  • cache configuration at plugin registration and omit correlation metadata on invalid or disabled configuration
  • document privacy and restart requirements

Verification

  • ................................................................ [100%]
    64 passed in 0.49s
  • 64 passed on current upstream main

The HMAC key remains environment-only and is never logged or serialized.

@alt-glitch alt-glitch added type/feature New feature or request comp/plugins Plugin system and bundled plugins telemetry Touches outbound telemetry, usage attribution, or analytics — needs opt-in gating before merge P3 Low — cosmetic, nice to have labels Jul 15, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the careful key parsing, disabled default, and documentation. Current main does not already provide this metadata (plugins/observability/langfuse/__init__.py:717-726), so the feature is not superseded.

Problems

  • plugins/observability/langfuse/__init__.py:727-730 sends a new third-party correlation identifier, but the change adds only a config gate. Current policy requires a config gate, setup prompt, and hermes tools toggle (AGENTS.md:118-121); current Langfuse setup only prompts for its existing credentials (hermes_cli/tools_config.py:555-579).
  • history_execution_writer_enabled is added at hermes_cli/config.py:3015 but is never read by _load_correlation_config() (plugins/observability/langfuse/__init__.py:162-173).
  • The registration test mocks the config loader (tests/plugins/test_langfuse_plugin.py:329-376), so it does not cover persisted-config/plugin-discovery propagation.

Suggested changes

  • Wire this through the required explicit consent surfaces before emitting metadata.
  • Remove the unused writer flag, or ship its concrete consumer and coverage together.
  • Add an isolated-HERMES_HOME integration test for enabled and disabled persisted configuration.

This is an automated hermes-sweeper review.

Comment thread hermes_cli/config.py
"correlation": {
"scheme": "hmac-sha256-v1:k1",
"emitter_metadata_enabled": False,
"history_execution_writer_enabled": False,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flag has no consumer: _load_correlation_config() reads only scheme and emitter_metadata_enabled, and the new test asserts this field is absent from metadata. Please remove it until a concrete writer uses it, or implement that writer and its behavior in this change.

"model": model,
"api_mode": api_mode,
}
correlation_id = _derive_correlation_id(task_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the new identifier becomes outbound Langfuse metadata. Current policy requires a generic consent flow with config gate, setup prompt, and hermes tools toggle (AGENTS.md:118-121); this PR adds the gate but does not update the Langfuse setup surface. Please add the required explicit opt-in flow before emitting it.

def register_hook(self, name, hook):
self.hooks[name] = hook

monkeypatch.setattr(config_mod, "load_config_readonly", fake_load_config)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an isolated-HERMES_HOME integration test using persisted config and normal plugin discovery. Mocking load_config_readonly verifies the helper, but not the configuration propagation and consent boundary.

@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Fourteen PRs address or reference this Langfuse observability complex, spanning the merged trace-I/O and wrong-prefix credential baseline, residual right-prefix template validation, credential diagnostics, system-prompt capture, privacy and endpoint handling, profile metadata, and trusted correlation. For target #64779, the diff adds disabled-by-default HMAC-derived trace correlation, but the contributor review identifies missing setup consent surfaces, an unused writer flag, and missing persisted-config propagation coverage.

Related pull requests

Duplicates

#22345 and #23831 supplied components consolidated into #26320, while #23188 is the narrower competing placeholder guard; #64309 and #77373 contain the same focused right-prefix template-key change, #60974 overlaps that validator work while adding runtime diagnostics, and #32175 is the Anthropic-only subset of #64292.

Suggested consolidation

Keep #64779 open with a salvage path that adds the required setup prompt and hermes tools consent toggle, resolves the unused writer flag, and verifies persisted-config propagation before emitting correlation metadata. Separately, close #64309 as a duplicate of #77373 despite #64309's keep_open review because their supplied production diffs and focused tests are the same; retain merged #26320 and closed #22345/#23188/#23831/#60974 as reference or superseded implementations, and keep the reviewed diagnostic, prompt-tracing, endpoint/state, privacy, profile, and credential-salvage paths independent.

Cross-PR triage: Reviewed 14 pull requests and 18 issues in this complex. Each diff was read against this issue; Assessment working set: 274 kB of PR diffs, 86 kB of issue/PR text, 37 kB of discussion (57 comments), 82 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data telemetry Touches outbound telemetry, usage attribution, or analytics — needs opt-in gating before merge type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants