Skip to content

fix: preserve base URL path segments in buildUrl() for Managed Dynatrace hosts - #284

Merged
kolatts merged 1 commit into
mainfrom
claude/issue-283-20260729-2050
Jul 29, 2026
Merged

fix: preserve base URL path segments in buildUrl() for Managed Dynatrace hosts#284
kolatts merged 1 commit into
mainfrom
claude/issue-283-20260729-2050

Conversation

@github-actions

@github-actions github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Problem

dynatrace commands fail with HTTP 404 when baseUrl includes a path component (e.g. https://dynatrace.imagile.dev/e/abc12345-0000-0000-0000-000000000000 for Dynatrace Managed multi-environment clusters). The /e/<environment-id> segment was silently dropped, so requests landed at https://dynatrace.imagile.dev/api/v2/entities instead of the correct https://dynatrace.imagile.dev/e/abc12345-0000-0000-0000-000000000000/api/v2/entities.

This is the same root cause as BUG-4 (Artifactory baseUrl-with-path, fixed in PR #94): buildUrl() calls new URL(path, base) where path starts with /. Per the WHATWG URL spec, an absolute-path reference resolves against the origin, discarding any path already present in base.

Fix

Strip a single leading / from path inside the shared buildUrl() function in src/lib/http.ts before constructing the URL. Because buildUrl() already ensures base ends with /, the resulting relative path resolves correctly against the full base (including any path component).

PR #94 worked around BUG-4 by removing the leading slash from Artifactory path constants, leaving the underlying flaw in buildUrl() itself. This fix closes that gap at the source so future services with context-root base URLs don't hit it again.

Changes

  • src/lib/http.ts — buildUrl(): strip leading / from path before new URL()
  • src/lib/http.test.ts — new test: Dynatrace Managed base URL preserves /e/<environment-id> path segment

Verification

npm run typecheck  ✓
npm run lint       ✓
npm test           ✓  397 tests pass

Closes #283

Generated with Claude Code

…ace and similar hosts

Strip the leading '/' from path before new URL(path, base) so that an absolute-path
reference does not resolve against the origin and silently discard path components
already present in base (e.g. /e/<environment-id> for Dynatrace Managed clusters).

Fixes #283 — recurrence of BUG-4 root cause (same WHATWG URL spec behavior that
dropped /artifactory from Artifactory base URLs in PR #94).

Co-authored-by: Sunny Kolattukudy <kolatts@users.noreply.github.com>
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Claude finished @github-actions[bot]'s task in 2m 26s —— View job


PR Review

  • Clean up previous Claude comments
  • Read changed files (src/lib/http.ts, src/lib/http.test.ts)
  • Review the fix for correctness and edge cases
  • Submit formal review

PASS — Clean fix, correct analysis, solid test.

Root cause

The diagnosis is accurate. new URL('/api/v2/entities', 'https://host/e/env-id/') resolves against the origin per the WHATWG URL spec, silently dropping /e/env-id — the same class of bug that hit Artifactory in PR #94. Fixing it once in buildUrl() is the right call.

Fix (src/lib/http.ts lines 46–50)

const normalizedPath = path.startsWith('/') ? path.slice(1) : path;
const url = new URL(normalizedPath, normalizedBase);

Correct. Because normalizedBase always ends with /, a relative path resolves against the full base including any path segments.

Backward-compat check: Services whose base URLs are bare origins (https://jira.example.com) are unaffected — stripping / from /rest/api/2/... against https://jira.example.com/ produces the same URL as before.

Artifactory: PR #94 removed leading slashes from Artifactory path constants, so they enter buildUrl() without / — the ternary is a no-op and the existing test at line 354 still passes.

ADO percent-encoding: The ADO double-encode test (/myorg/My%20Project/...) still passes because the base URL has no path component; the result is identical with or without stripping the leading /.

Test (src/lib/http.test.ts lines 685–703)

The new test covers the exact failure case — Dynatrace Managed base URL with /e/<env-id> path, path argument /api/v2/entities, expected merged URL. It follows the established vi.stubGlobal('fetch', ...) pattern used throughout the suite.

One minor observation: no test asserts the platform-URL variant (dynatracePlatform) with a path-component base, but given dynatraceRequest() is the shared implementation for both, the coverage is sufficient.

No issues found

@claude claude Bot 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.

PASS — Clean fix at the right layer. Root cause correctly identified (WHATWG URL spec absolute-path reference discards base path segments). Stripping a leading / from path in buildUrl() before constructing new URL() is the minimal correct fix. Backward-compatible for all existing services with bare-origin base URLs. New test directly exercises the Dynatrace Managed multi-environment URL pattern. 397 tests pass. Good to merge.

@kolatts
kolatts enabled auto-merge (squash) July 29, 2026 21:05
@kolatts
kolatts merged commit 87fef4b into main Jul 29, 2026
3 checks passed
@kolatts
kolatts deleted the claude/issue-283-20260729-2050 branch July 29, 2026 21:06
kolatts added a commit that referenced this pull request Jul 31, 2026
…i/api/ after buildUrl fix) (#287)

* fix: drop leading /api/ from CheckmarxClient paths to fix doubled segment after buildUrl() fix

CheckmarxClient used absolute paths like /api/projects while checkmarx.baseUrl
already ends with /api (e.g. https://tenant.cxone.cloud/api). After PR #284
correctly fixed buildUrl() to preserve base path segments, the combination
produced /api/api/projects. Fix paths to be relative (projects, scans,
results/summary) so they resolve correctly against the /api base — mirrors
the same fix applied to Artifactory in BUG-4.

Closes #285

Co-authored-by: Sunny Kolattukudy <kolatts@users.noreply.github.com>

* docs: update Checkmarx API base URL

---------

Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Co-authored-by: Sunny Kolattukudy <kolatts@users.noreply.github.com>
kolatts added a commit that referenced this pull request Aug 4, 2026
…i/api/ after buildUrl fix) (#287)

* fix: drop leading /api/ from CheckmarxClient paths to fix doubled segment after buildUrl() fix

CheckmarxClient used absolute paths like /api/projects while checkmarx.baseUrl
already ends with /api (e.g. https://tenant.cxone.cloud/api). After PR #284
correctly fixed buildUrl() to preserve base path segments, the combination
produced /api/api/projects. Fix paths to be relative (projects, scans,
results/summary) so they resolve correctly against the /api base — mirrors
the same fix applied to Artifactory in BUG-4.

Closes #285

Co-authored-by: Sunny Kolattukudy <kolatts@users.noreply.github.com>

* docs: update Checkmarx API base URL

---------

Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Co-authored-by: Sunny Kolattukudy <kolatts@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dynatrace commands fail when baseUrl includes a path component (recurrence of BUG-4's root cause)

1 participant