Skip to content

fix(cli): ignore null profile envelope metadata#2274

Merged
zhoujh01 merged 1 commit into
mainfrom
fix/cli-ignore-null-profile-envelope
May 28, 2026
Merged

fix(cli): ignore null profile envelope metadata#2274
zhoujh01 merged 1 commit into
mainfrom
fix/cli-ignore-null-profile-envelope

Conversation

@ZaynJarvis
Copy link
Copy Markdown
Collaborator

@ZaynJarvis ZaynJarvis commented May 28, 2026

Summary

  • Treat profile: null in success envelopes as absent when the Rust CLI unwraps dynamic JSON results.
  • When real non-null profile metadata is preserved in compact JSON output, emit the server-style status/result/profile envelope instead of nesting profile inside result.
  • Preserve existing no-profile compact CLI JSON output (ok/result) and existing table/text profile rendering behavior.
  • Add regression tests for ls-style list responses, profiled object responses, and null profile handling.

Root Cause

PR #2125 (e9e6ce5, "Feature/http profile middleware") added request-scoped HTTP profiling and introduced profile on the standard server response model. The server currently exposes success responses in this envelope shape:

{"status":"ok","result":[...],"error":null,"telemetry":null,"profile":null}

When profiling is inactive, profile is serialized as null. The same PR also added CLI profile-preserving unwrap logic for serde_json::Value: if an envelope has any profile key, unwrap_success_envelope() wraps or merges the payload with profile. Because that check treated profile: null as meaningful, commands like ls rendered an extra nested result layer in compact JSON output:

{"ok":true,"result":{"result":[...],"profile":null}}

A real non-null profile should also remain envelope metadata rather than becoming business payload. With this PR:

  • inactive profile: null is treated as absent, so normal no-profile CLI output stays unchanged:
{"ok":true,"result":[...]}
  • real non-null profile metadata is emitted in a server-aligned envelope shape:
{"status":"ok","result":[...],"profile":[...]}

This keeps the existing CLI compact ok/result convention for ordinary no-profile responses, but avoids inventing a second nested CLI envelope for profiled server metadata. This PR intentionally does not change the server response shape. PR #2215 only moved profile-aware table rendering order for component status output; it was not the source of this regression.

Test Plan

  • cargo test -p ov_cli
  • git diff --check
  • cargo run -q -p ov_cli -- ls viking:// --output json
  • Raw server check: GET /api/v1/fs/ls?uri=viking:// returns status/result/error/telemetry/profile envelope with profile:null when inactive.

@github-actions
Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

2215 - Partially compliant

Compliant requirements:

(No changes directly related to this ticket)

Non-compliant requirements:

(No changes directly related to this ticket)

Requires further human verification:

(No changes directly related to this ticket)

2125 - Partially compliant

Compliant requirements:

(No changes directly related to this ticket)

Non-compliant requirements:

(No changes directly related to this ticket)

Requires further human verification:

(No changes directly related to this ticket)

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🏅 Score: 98
🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions
Copy link
Copy Markdown

PR Code Suggestions ✨

No code suggestions found for the PR.

@ZaynJarvis ZaynJarvis force-pushed the fix/cli-ignore-null-profile-envelope branch from 054bed2 to 2d60800 Compare May 28, 2026 04:15
@ZaynJarvis ZaynJarvis force-pushed the fix/cli-ignore-null-profile-envelope branch from 2d60800 to a14d929 Compare May 28, 2026 06:04
@zhoujh01 zhoujh01 merged commit a88007b into main May 28, 2026
11 checks passed
@zhoujh01 zhoujh01 deleted the fix/cli-ignore-null-profile-envelope branch May 28, 2026 11:03
@github-project-automation github-project-automation Bot moved this from Backlog to Done in OpenViking project May 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants