fix: P1/P7/P8 — version unification, FME config wiring, debug log redaction

[sunilgattupalle]

Summary

Addresses three Phase 1 quality review items (stacked on #107):

• P1 (Version unification): MCP server metadata now reads version from package.json via getVersion() instead of hardcoded "2.0.0". Single source of truth — CLI --version and MCP initialize response both reflect package.json.

• P7 (Dead config): resolveProductBaseUrl() now uses HARNESS_FME_BASE_URL from config (defaults to https://api.split.io) instead of a dead hardcoded const. Self-managed/staging environments can now override the FME base URL via env var.

• P8 (Debug log redaction): All 3 debug-level body log sites in harness-client.ts now run through redactSensitiveFields() by default. Redacts values for keys matching: token, secret, password, apiKey, credential, privateKey, clientSecret, authorization, bearer, webhook, passphrase, encrypted, sshKey, accessKey, secretKey. Set HARNESS_LOG_UNSAFE_BODIES=true to bypass for local debugging.

Also includes deferred spec 002 (TransportOverrides) for future reference — design is pre-approved but implementation deprioritized per review.

Test plan

• pnpm typecheck — clean
• pnpm test — 1072/1072 pass (includes 11 new redaction tests)
• Zero lint errors on changed files
• Verify --version CLI output matches package.json
• Verify MCP initialize response serverInfo.version matches package.json
• Verify debug logs with LOG_LEVEL=debug show [REDACTED] for sensitive fields
• Verify HARNESS_LOG_UNSAFE_BODIES=true bypasses redaction

Made with Cursor

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[cursor]

Stale comment

Important findings:

1. src/config.ts:121 — the new FME base URL override bypasses the repo's HTTPS config-validation convention. HARNESS_FME_BASE_URL is accepted as any URL, but only HARNESS_BASE_URL is gated by HARNESS_ALLOW_HTTP, so feature-flag requests can now send Authorization: Bearer ${HARNESS_API_KEY} over plain HTTP when misconfigured.
2. src/utils/redact.ts:37-54 — the new non-JSON redaction fallback still leaks whitespace-delimited secrets. Inputs such as authorization: Bearer <token> or quoted secrets containing spaces are only partially scrubbed (or missed entirely), and harness-client.ts now relies on this helper for every debug body log path.

  

_{Sent by Cursor Automation: Sunil On Demand Architecture Review}

[cursor]

Stale comment

• Important — src/utils/redact.ts:37-39, tests/utils/redact.test.ts:135-145: INLINE_SECRET_PATTERN is still narrower than SENSITIVE_KEY_PATTERN. On the parse-failure path, non-JSON bodies using keys such as sshKey / ssh_key, secretKey / secret_key, webhook, or encrypted are returned almost verbatim, so the new default redaction in HarnessClient can still leak those values into debug logs. The fallback test only covers token, so this leak path would pass the new suite unchanged.

• Important — tests/utils/redact.test.ts:1-147, src/client/harness-client.ts:242-244, src/client/harness-client.ts:270-272, src/client/harness-client.ts:319-320, src/config.ts:78: the added suite only proves the helper functions in isolation. It never exercises the three HarnessClient logging call sites or the HARNESS_LOG_UNSAFE_BODIES escape hatch that this PR introduced. That means the headline behavior change here — redact request / error / response bodies by default, bypass only when explicitly enabled — still has no material regression coverage at the integration point that can actually break.

• Important — src/index.ts:54-75, CLAUDE.md:504-508: the current server instructions block is still out of compliance with the anti-bloat rules. It contains per-resource PR/pipeline examples, explicit PR resource inventory, schema guidance, ordinal-selection heuristics, and pipeline-version-specific guidance, while CLAUDE.md says the global block should contain only universal patterns and move resource/feature-specific guidance into structured metadata (description, executeHint, inputExpansions, bodySchema, etc.). Even if this was inherited from the stacked base, the branch state still does not meet Sunil's instruction-budget standard.

  

_{Sent by Cursor Automation: Sunil On Demand Architecture Review}

[cursor]

Stale comment

Important findings:

1. src/config.ts:77, src/config.ts:120-121 — HARNESS_FME_BASE_URL is now configurable, but it does not share the HTTPS / HARNESS_ALLOW_HTTP validation that already protects HARNESS_BASE_URL. Because the FME path sends Authorization: Bearer ${HARNESS_API_KEY}, a misconfigured http://... override would put the API key on the wire in plaintext.
2. src/utils/redact.ts:37-54, tests/utils/redact.test.ts:135-145 — the new parse-failure redaction fallback is still narrower than the canonical sensitive-key matcher and it truncates values at whitespace. Malformed or non-JSON bodies can therefore still leak secrets in exactly the debug logging path this PR is enabling by default.

Open questions / assumptions:

• I could not independently run pnpm typecheck or pnpm test in this runner because node / pnpm are not available on PATH, so this review is source-based.
• I scoped findings to issues introduced by this PR and did not re-raise inherited branch-state debt outside the touched behavior.

Change summary:

• Server version is now sourced from package.json via getVersion().
• FME base URL became configurable.
• Debug request / response body logging now routes through redactJsonString() by default.

  

_{Sent by Cursor Automation: Sunil On Demand Architecture Review}

[cursor]

Stale comment

Important finding:

1. src/utils/redact.ts:1-3, src/utils/redact.ts:41-58 — the new non-JSON redaction path still misses some common secret-key spellings, so plaintext/YAML debug logs can leak values in cases the helper is supposed to protect. The canonical matcher accepts mixed underscore forms like client_secret, but it does not match kebab-case variants such as private-key, and the inline scrubber inherits the same gap. I verified this against the current regex: private-key=AAA is returned unchanged, while client-secret=BBB only redacts because the generic secret alternative happens to match that substring. Since HarnessClient now runs all request / response / error bodies through redactJsonString() by default, malformed or YAML payloads using private-key: (a realistic PEM field spelling) can still hit logs with sensitive material intact.

Open questions / assumptions:

• I scoped this review to the requested commit range and the changed files. I did not re-raise the inherited src/index.ts instructions-budget issue because the touched lines there only swap in getVersion() and the current oversized instructions block predates this PR.
• I could not run pnpm test or pnpm typecheck in this runner because node and pnpm are not available on PATH, so the verification notes below are based on source inspection plus small standalone regex probes with python3.

Change summary:

• The earlier FME transport-safety concern appears fixed: ConfigSchema now applies the same HTTPS / HARNESS_ALLOW_HTTP gate to HARNESS_FME_BASE_URL, and tests/config.test.ts covers reject / allow / https cases.
• The earlier non-JSON parse-failure redaction concern also appears materially fixed for the reported cases: the inline fallback now shares the same key list as the structured matcher and correctly redacts bearer tokens, quoted multi-word secrets, webhook, encrypted, secret_key, and ssh_key in the added tests.
• Remaining verification gap: the new HarnessClient logging integration (HARNESS_LOG_UNSAFE_BODIES off by default, bypass when true) still has no direct regression test at the call sites in src/client/harness-client.ts.

Verification notes:

• Reviewed the diff for specs/002-endpoint-spec-transport-overrides.md, src/client/harness-client.ts, src/config.ts, src/index.ts, src/utils/cli.ts, src/utils/redact.ts, tests/config.test.ts, and tests/utils/redact.test.ts against the architecture rules in CLAUDE.md / AGENTS.md.
• Confirmed from source that FME requests set Authorization: Bearer ${HARNESS_API_KEY} in src/registry/index.ts, which makes the new HTTPS guard in src/config.ts security-relevant and correctly aligned with the standards.
• Ran targeted regex probes with python3 to verify that the prior authorization: Bearer ... and whitespace/quoted-value cases are now scrubbed, and to confirm the residual private-key miss described above.

  

_{Sent by Cursor Automation: Sunil On Demand Architecture Review}

[cursor]

Stale comment

Findings

• Important: src/config.ts / src/client/harness-client.ts — HARNESS_FME_BASE_URL is now configurable, but HarnessClient still unconditionally adds Harness-Account and accountIdentifier to product: "fme" requests. That means the new override sends Harness-specific scoping data to whatever Split/FME endpoint the user configures, which contradicts the transport contract and leaves the FME wiring incomplete.
• Important: src/utils/redact.ts / src/client/harness-client.ts / tests/utils/redact.test.ts — the new redaction matcher only catches exact key names like webhook, so common payload fields such as webhookUrl still pass through unredacted. Because the client now funnels request/response/error body debug logs through this helper, debug mode can still leak secret-bearing webhook URLs, and the added tests do not cover that shape.
• Suggestion: src/index.ts — the server instructions string still violates the anti-bloat rules in CLAUDE.md / AGENTS.md: it is over the ~20-line target and still contains per-resource examples plus pipeline-version guidance that those docs say should live in endpoint/resource metadata instead of the global instructions block.

Open Questions / Assumptions

• I could not rerun pnpm test / pnpm typecheck in this automation environment because node / pnpm were not available on PATH, so this review is based on static inspection of the diff, current HEAD, and the checked-in tests.
• I assumed HARNESS_FME_BASE_URL is meant to support non-default/custom endpoints. If it is guaranteed to stay on Harness-controlled Split infrastructure only, the first finding is lower risk, but the transport mismatch still remains.
• If end-to-end debug redaction is meant to cover non-client logs too, src/tools/harness-execute.ts still appears to log merged input / params raw at debug level outside the new helper path.

Assessment

Version sourcing looks improved: src/index.ts now pulls the server version from package.json, which moves this PR toward the intended single source of truth, and the new HTTPS validation for HARNESS_FME_BASE_URL is the right guard. I don't think the PR fully meets Sunil's architecture/security standards yet, though, because the FME transport override and debug-log redaction are still incomplete, and the global instructions block remains out of policy.

  

_{Sent by Cursor Automation: Sunil On Demand Architecture Review}

[cursor]

Making HARNESS_FME_BASE_URL configurable is a good start, but the transport layer still treats product: "fme" requests like Harness requests: HarnessClient always adds Harness-Account and accountIdentifier unless headerBasedScoping is set. With this override in place, those internal scoping values now get sent to whatever endpoint the user points this at, which contradicts the RequestOptions contract and leaves the FME wiring incomplete.

[cursor]

SENSITIVE_KEY_PATTERN is anchored to exact key names, so this catches webhook but not common fields like webhookUrl. harness-client now routes request/response/error body logs through this helper, so payloads with a webhookUrl field will still be logged verbatim at debug level. The repo already accepts/emits webhookUrl fields in pipeline/template payloads, so this is a live leak path rather than a hypothetical regex gap.

[cursor]

Stale comment

Important findings:

1. src/config.ts, src/client/harness-client.ts, src/client/types.ts — P7 wires HARNESS_FME_BASE_URL into config, but product: "fme" requests still do not honor the transport contract. HarnessClient.request() / requestStream() compute isFme and then ignore it, still sending Harness-Account, while buildUrl() still injects accountIdentifier unless headerBasedScoping is set. That leaves the new FME override only half-wired: Split/FME traffic still carries Harness-specific scoping metadata even though the client type comment says FME should skip Harness-specific auth/headers/params.

2. src/utils/redact.ts — the new debug-log redaction still misses live webhookUrl fields. SENSITIVE_KEY_PATTERN is exact-match on webhook, so webhookUrl survives both the JSON walk and the parse-failure fallback. This repo's pipeline/template schemas already include webhookUrl, and a quick probe against the current regex leaves webhookUrl: https://example.invalid/... intact, so request/response debug logging can still leak secret callback endpoints.

Open questions / assumptions:

• I reviewed current HEAD 2ebd148c8cc88b1021fad61afa4c1f4e15a4eff7; the earlier HARNESS_FME_BASE_URL HTTPS-guard gap and the kebab-case parse-fallback redaction gap appear fixed.
• I could not run pnpm test or pnpm typecheck in this runner because node / pnpm are not available on PATH, so this pass is source-based plus small standalone regex probes.

Change summary:

• Version sourcing now correctly uses getVersion() / package.json.
• HARNESS_FME_BASE_URL is now configurable and guarded by HTTPS / HARNESS_ALLOW_HTTP.
• The redaction work materially improves the prior leak paths, but the remaining webhookUrl miss means the PR still does not fully meet Sunil's logging-safety standard.

  

_{Sent by Cursor Automation: Sunil On Demand Architecture Review}

[cursor]

Findings

1. Critical — still present on this PR branch: src/utils/redact.ts, src/client/harness-client.ts
   The new redaction path is not strong enough to satisfy the repo's NEVER expose secret values rule. redactJsonString() falls back to a single-line regex for any non-JSON body, so common YAML block-scalar shapes (for example privateKey: |- ...) still leak the actual secret material, and the key allowlist misses real secret fields from our shipped schemas such as integrationKey. Because HarnessClient now routes request/error/response debug body logging through this helper, secrets can still land in stderr logs whenever LOG_LEVEL=debug. The base branch already logged bodies unsafely; this PR improves the default, but the leak is still present on the PR branch.
2. Important — introduced by this PR: src/config.ts
HARNESS_FME_BASE_URL is now a supported runtime setting, but the shipped config surfaces are not updated. .env.example, manifest.json, and mcp-directory/manifest.json still do not expose the variable, so bundled/manifest consumers cannot actually configure the new FME base URL. That leaves the "FME config wiring" change incomplete and drifts from the repo's environment-configuration/documentation standard.

Open Questions / Assumptions

• I could not run pnpm test or pnpm typecheck in this runner because pnpm was not on PATH, so this review is based on static diff inspection and repo searches.
• I'm assuming the MCPB manifests are part of the supported shipping surface, since scripts/prepare-mcpb.js copies them into the bundle.

Overall Assessment

Not yet aligned with Sunil's architecture standards. The PR moves in the right direction on version unification and safer default logging, but it still falls short of the NEVER expose secret values requirement and does not fully wire/document the new runtime config across the supported distribution surfaces.

  

_{Sent by Cursor Automation: Sunil On Demand Architecture Review}

[cursor]

This fallback still leaks real secrets for the non-JSON/YAML path that HarnessClient now uses for debug body logging. It only rewrites single-line key: value pairs, so a common shape like:

privateKey: |-
  -----BEGIN PRIVATE KEY-----
  ...

still leaves the PEM body intact. The allowlist also misses concrete secret fields from our shipped schemas (for example integrationKey on PagerDuty notification channels). Since the project standard is "NEVER expose secret values", I think this needs a parser-based/non-logging fallback before we rely on it as the redaction layer.

[cursor]

HARNESS_FME_BASE_URL becomes a supported runtime setting here, but the PR doesn't propagate it to the public config surfaces (.env.example, manifest.json, mcp-directory/manifest.json). That makes the "FME config wiring" only partial: manifest/MCPB consumers still can't set the new base URL, and the documented env surface drifts from runtime behavior.

[cursor]

Important finding:

1. src/utils/redact.ts, src/client/harness-client.ts — the new non-JSON fallback redactor still leaks structured secret values. INLINE_SECRET_PATTERN stops at the first } / newline / ], so payloads like secret: {"value":"supersecret"} become secret: [REDACTED]{"value":"supersecret"}, and YAML block scalars such as privateKey: | ... keep the PEM body after the first line. Because HarnessClient now routes request, error, and response debug bodies through redactJsonString() by default, the branch still misses Sunil's NEVER expose secret values standard in the exact path P8 is intended to harden.

Open questions / assumptions:

• Reviewed current HEAD 17d47761543ed3e3cb5d1b9567a7c82992761ef6.
• node / pnpm are not available on this runner, so I could not rerun pnpm test / pnpm typecheck; verification here is source-based plus targeted python3 probes of the new regex behavior.
• I scoped findings to the PR's current diff. The oversized global instructions block in src/index.ts looks inherited from the base branch, so I did not re-raise it as a diff-scoped blocker here.

Change summary:

• P1 looks aligned now: MCP server version comes from package.json via getVersion().
• P7 also looks materially aligned now: HARNESS_FME_BASE_URL is wired through config, guarded by HTTPS / HARNESS_ALLOW_HTTP, and product: "fme" requests now skip Harness account header/query scoping.
• P8 is improved, but not complete yet because the fallback redactor still leaks object/block-style secrets.

  

_{Sent by Cursor Automation: Sunil On Demand Architecture Review}

[cursor]

This still leaks secrets on the parse-failure path. The value pattern stops at } / newline / ], so inputs like secret: {"value":"supersecret"} become secret: [REDACTED]{"value":"supersecret"}, and YAML block scalars such as privateKey: | -----BEGIN PRIVATE KEY----- ...
keep the PEM body after the first line. Since HarnessClient now sends request / error / response debug bodies through redactJsonString() by default, this branch can still expose secrets in the exact path P8 is supposed to harden. Can we make the fallback drop the entire field payload (or add a safer multiline parser) and cover these shapes in tests/utils/redact.test.ts?

[thisrohangupta]

Overall: Approve with minor follow-ups
The PR cleanly addresses the three stated review items (P1/P7/P8). Implementation is small, focused, well-tested (11 new redaction tests, 1085/1085 passing), and pnpm typecheck is clean. Worth merging once the documentation gap and a test coverage gap are addressed.

P1 — Version unification
Clean. Exporting getVersion() from src/utils/cli.ts and using it in the McpServer ctor makes package.json the single source of truth. Verified locally: getVersion() returns 0.9.6 matching package.json. CLI --version and serverInfo.version now stay in sync automatically on every release bump.

P7 — FME base URL wiring
resolveProductBaseUrl(config, "fme") now reads from config.HARNESS_FME_BASE_URL instead of the dead hardcoded const. The HTTPS guard mirrors the one for HARNESS_BASE_URL, and the new tests in tests/config.test.ts cover the three cases (reject HTTP by default, accept HTTP when HARNESS_ALLOW_HTTP=true, accept HTTPS).

The bonus FME transport fixes in commit 17d4776 are correct and necessary:

harness-client.ts
Lines 192-198
const isFme = options.product === "fme";
const accountId = this.resolveAccountId();
const headers: Record<string, string> = {
...(isFme ? {} : { "Harness-Account": accountId }),
...options.headers,
};
harness-client.ts
Lines 459-468
const params = new URLSearchParams();
if (!options.headerBasedScoping && options.product !== "fme") {
const accountId = this.resolveAccountId();
params.set("accountIdentifier", accountId);
// Log-service gateway expects accountID (capital ID) in query params
if (path.includes("/log-service/")) {
params.set("accountID", accountId);
}
}
Without these, every FME call would have leaked Harness-Account and accountIdentifier to Split's API, which would either be ignored or trigger validation noise.

Issue (must-fix nit): the FME transport changes have zero test coverage. tests/client/harness-client.test.ts covers baseUrl override but never asserts that product: "fme" omits Harness-Account or accountIdentifier. A regression to either branch would silently start leaking account scope to Split.io. Recommend adding two assertions in request — headers / request — URL describes:

it("omits Harness-Account header for product='fme'", async () => {
fetchSpy.mockResolvedValue(new Response("{}", { status: 200 }));
const client = new HarnessClient(makeConfig());
await client.request({ path: "/internal/api/v2/x", product: "fme" });
const headers = fetchSpy.mock.calls[0][1]?.headers as Record<string, string>;
expect(headers["Harness-Account"]).toBeUndefined();
});
it("omits accountIdentifier query param for product='fme'", async () => {
// assert URL has no accountIdentifier
});
Design observation (not blocking): there are now two overlapping flags — headerBasedScoping ("skip query, keep header") and product === "fme" ("skip both") — implicitly coupled in buildUrl. A future cleanup could collapse to scopingMode: "query" | "header" | "none", but that's clearly out of scope here.

P8 — Debug log redaction
src/utils/redact.ts is well-designed:

Depth cap (10) — verified that anything deeper collapses to [REDACTED] instead of leaking. Defense in depth.
Anchored key pattern (^...$) — deliberately strict to avoid over-redacting keys like tokenize / secretCount. Trade-off: custom key names like gitToken, accountSecret will slip through. The PR's choice is the safer of the two; worth a code comment explaining why.
redactJsonString parse-then-fallback — primary path parses JSON, fallback uses inline regex. Verified empirically that inline alternation precedence works correctly for compound keys (access_token, webhook_url, client-secret etc. all redact properly even though token/secret come first in the alternation).
No mutation — confirmed with the dedicated test.
The 223-line test file is solid and exercises both top-level keys, nested objects, arrays, mixed cases, kebab/snake/camel, and the depth limit.

One real gap: the call sites in harness-client.ts aren't tested for the HARNESS_LOG_UNSAFE_BODIES toggle behavior. The redaction utility is heavily tested in isolation; the integration ("when flag is true, raw body is logged; when false, redacted") relies on visual inspection. Not blocking, but a one-line assertion would close the loop.

Test 4 from my probe also surfaces a non-issue worth being aware of: stringified secrets inside JSON values (e.g. {"yamlBody": "token: secret123"}) are NOT scrubbed because the redactor short-circuits on strings. This is correct behavior for "Request body" logs (the keys are what we trust as a redaction signal), but if anyone ever logs a YAML-as-string body, secrets in those values will pass through. Probably worth a JSDoc note on redactSensitiveFields.

Documentation gap
Neither README.md nor .env.example document the two new env vars introduced in this PR:

HARNESS_FME_BASE_URL — useful for self-managed/staging FME backends
HARNESS_LOG_UNSAFE_BODIES — escape hatch for local debug
These should be added to .env.example (under "Security and transport controls" makes sense) and the env-var table in README.md so users can discover them.

Spec 002
361-line design doc for a deferred refactor lives in specs/002-endpoint-spec-transport-overrides.md. The analysis is thorough and the chosen TransportOverrides bag is the right call (I agree with the reasoning rejecting interface extension/middleware/declaration merging given that all consumption is in one executeSpec function).

Two suggestions:

Land it as a separate doc-only commit/PR rather than bundled here. PR 108's stated scope is P1/P7/P8; the spec is unrelated to those. Mixing 361 lines of design discussion with three small fixes makes the PR harder to skim.
The spec catalogues EndpointSpec.scopeParams as dead. I confirmed: it's declared in src/registry/types.ts:293 but never set or read; only ResourceDefinition.scopeParams (line 199) is used. Easy 1-line cleanup that could land independently.
Verification I ran
pnpm install --frozen-lockfile — clean
pnpm typecheck — clean
pnpm test — 1085/1085 pass (PR description says 1072/1072; the gap is from PR #107's test additions also present here)
pnpm build — clean
Manually exercised redactJsonString against ~15 inputs (kebab/snake/camel, deeply nested, alternation precedence, parse failures, truncation, empty strings, numeric values) — all behaved correctly
getVersion() returns 0.9.6 matching package.json
Recommendation
Approve once:

FME transport assertions added to tests/client/harness-client.test.ts (Harness-Account header omission, accountIdentifier query omission).
HARNESS_FME_BASE_URL and HARNESS_LOG_UNSAFE_BODIES documented in .env.example + README.md env table.
Optional follow-ups (separate PRs):

Move specs/002-endpoint-spec-transport-overrides.md to its own doc PR.
Add an integration test for the HARNESS_LOG_UNSAFE_BODIES toggle in harness-client.ts.
Clean up dead EndpointSpec.scopeParams field (1-line change).