Repository navigation
Conversation
6f99596 to
69332ce
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if any(character in argument for argument in command for character in '"\r\n\0'): | ||
| raise ClaudeDesktopError("Claude Desktop OTEL helper arguments contain invalid characters.") | ||
| # Quote every argument for cmd.exe, including metacharacters without spaces. | ||
| quoted = " ".join('"' + argument.replace("%", "%%") + '"' for argument in command) |
There was a problem hiding this comment.
what happens if an argument ends with \ (e.g. a profile or path like C:\dir\)? "C:\dir\" makes the child's argv parser treat \" as a literal quote and merge it with the next arg. can we double trailing backslashes, or reject them?
Suggestion:
| quoted = " ".join('"' + argument.replace("%", "%%") + '"' for argument in command) | |
| def _quote(argument: str) -> str: | |
| stripped = argument.rstrip("\\") | |
| trailing = len(argument) - len(stripped) | |
| return '"' + (stripped + "\\" * (2 * trailing)).replace("%", "%%") + '"' | |
| quoted = " ".join(_quote(argument) for argument in command) |
| contents = after.encode("utf-8") | ||
| elif after is not None: | ||
| contents = json.dumps(after, indent=2).encode("utf-8") | ||
| if before == contents and (not isinstance(after, str) or modes[path] & stat.S_IXUSR): |
There was a problem hiding this comment.
this condition is hard to read. can we pull the exec-bit check into a named variable like needs_exec_bit, so it's clear why an unchanged script still gets rewritten?
| otel_endpoint=( | ||
| f"{workspace.rstrip('/')}/ai-gateway/otel" if state.get("claude_otel_tracing") else None | ||
| ), |
There was a problem hiding this comment.
can we derive this from build_otel_traces_endpoint/build_otel_headers_argv in databricks.py? right now the endpoint is rstripped but the helper's --host isn't, unlike Claude Code's helper.
| @pytest.mark.parametrize("source", ["discovery", "static"]) | ||
| @pytest.mark.parametrize("tracing", [None, False, True]) | ||
| def test_configure_otel_follows_claude_tracing_setting(desktop, source, tracing): |
There was a problem hiding this comment.
is the source param necessary? the model source doesn't affect OTEL, so this only doubles the cases.
| @pytest.mark.parametrize("failed_file", ["helper", "profile", "metadata", "state"]) | ||
| @pytest.mark.parametrize("operation", ["enable", "switch", "disable", "revert"]) |
There was a problem hiding this comment.
are all 16 cases necessary? test_claude_desktop.py already covers profile/metadata/state rollback with the same private _write_or_remove patch. can we keep only the helper cases?
Polly AI ReviewBlocking issues
Security vulnerabilitiesNo confirmed security vulnerability found. The generated files do not persist bearer tokens. POSIX arguments are shell-quoted; Windows arguments reject unsafe characters and disable delayed expansion. Non-blocking notes
The authentication-state issue is inherited from ApproachThe approach is sound: reuse the existing authentication command and Desktop transaction instead of adding a service or separate credential store. Including wrapper permissions in rollback is appropriate. No materially simpler replacement is apparent. SummaryDiff size (computed): 12 files; +512 / -10 lines (522 changed text lines); 0 binary files.
Shares use additions + deletions, including generated text and lockfiles. Binary files have no line count. Tests include fixtures/helpers under test paths and colocated test/spec files; categories are path-based, not coverage measurements. ChangesThe implementation connects managed Claude tracing to Desktop’s native OTLP settings, generates a refreshing helper, and includes that helper in configuration and cleanup transactions. Documentation records ownership, supported Desktop versions, and native-validation limits. No dependency, lockfile, generated-asset, proxy, or service changes are included. The size is justified by cross-platform generation and stateful rollback requirements. No substantial removable scaffolding was identified. The two test-guide coverage paragraphs could share a canonical description, but that is a minor documentation reduction, not grounds to split the feature. TestsTest-by-test assessmentAssessment is based on static inspection of the frozen diff and relevant existing coverage. Tests were not executed; the PR’s reported pass counts were not independently verified.
No existing test functions were removed or weakened. Existing profile ownership, JSON rollback, basic header-output, missing-workspace, and token-provider tests supply useful lower-layer coverage. Missing coverage is specific: Windows non-ASCII command decoding, cleanup after native-library removal, and successful executable-permission repair. These need process or filesystem component tests, not additional live E2E setup. Native Desktop schema acceptance, open-session credential refresh, and trace ingestion remain unverified. After fixes, run ScopeAll changes support the stated native Desktop tracing outcome. The stdout redirect supports JSON-only helper output, and permission restoration supports transactional rollback. No clearly unrelated changes were found. Automated review by Polly · workflow run |
1 participant
What did you change, and why?
Configure Claude Desktop's native OTLP trace export when managed Claude tracing is enabled. Use a refreshing headers helper so open Code/Cowork sessions can obtain current credentials instead of retaining their starting inference token.
Follow-up to the now-merged #1174. Rebased onto
main; the diff contains only OTEL support. Ticket: AIGTWY-5019.Requirements
.sh/.cmdwrapper pinned to the configuring Python installation, workspace, and CLI profile. Reuse cached tokens and silent refresh; keep stdout JSON-only.How do you know it works?
2,228 targeted tests passed across Desktop configuration/auth/locks, Claude and Codex agents, gateway discovery/proxy, CLI, managed configuration, integration contracts, and startup imports. Ruff lint/format, type checks, and diff checks passed.
The atomic writer accepts only bytes or deletion, with explicit file permissions; JSON and helper scripts are serialized by callers. Regression coverage includes unchanged JSON formatting, helper executable-permission repair, and failed replacement restoring original bytes and permissions.
Component coverage includes tracing on/off/absent for static and discovered model lists on simulated macOS, Windows, and Linux; native helper execution with rotated bearer overrides; literal argument quoting and child failures; JSON-only stdout; repeat configure; workspace switch; disable/re-enable; rollback of bytes and permissions at every write boundary; and revert preserving unrelated telemetry.
Native Desktop export across token expiry and trace ingestion have not been tested. Documentation includes requirements and manual validation steps.
Limits
otlpTracesEnabled(added in 1.22209.0) andotlpHeadersHelper(added in 1.30096.1), plus workspace trace ingestion enabled for the user./v1/traces, not/v1/logsor/v1/metrics.Schema reference: https://claude.com/docs/third-party/claude-desktop/configuration#otlp