feat: project-scoped Jira credentials, check_integrations tool, and docs update - #76
Conversation
Implements feedback items #4 (Project-Scoped Jira Credentials) and #5 (Integration Health Check / Credential Safety). Project-scoped credentials (#4): - Add host and email to JiraProjectConfig for per-project overrides - Resolution chain: project config → user config → env vars - API token intentionally excluded from project config (stays in env vars or user config for security) - Backward-compatible: legacy (user-only) signature still works Integration health check (#5): - New check_integrations tool reports credential status without exposing secrets — shows configured (bool), source (project/user/env), and host value but never email addresses or API tokens - Prevents agents from reading ~/.claude/mcp.json to check config - Confluence status derived from Jira credentials (same Atlassian auth)
… credentials - personas.md: PO now includes use-case doc type; DM has Sprint 0 section - mcp-server.md: document diagnostic tools (run_doctor, check_project_health, get_started, check_integrations) - configuration.md: add jira.host/email project overrides, aem.currentPhase, credential resolution chain, Jira env vars - jira.md: project-scoped credentials, check_integrations usage example - getting-started.md: mention get_started and check_project_health tools
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 34 minutes and 38 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds per-project Jira credential overrides with deterministic resolution (project → user → env), refactors Jira client and tools to consume merged credential sources, introduces a read-only MCP tool Changes
Sequence Diagram(s)sequenceDiagram
participant Agent
participant MCP as MCP/doctor tool
participant Config as Project/User Config
participant JiraClient as Jira client / Env
Agent->>MCP: invoke check_integrations
MCP->>Config: load project + user config
MCP->>JiraClient: resolveJiraStatus(sources)
JiraClient-->>MCP: status (host/email/apiToken configured + source)
MCP-->>Agent: structured result (no secrets, provenance metadata)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/skills/builtin/jira/tools.ts (1)
764-781:⚠️ Potential issue | 🟠 Major
fetch_jira_statusesbypasses the new credential resolution chain.Unlike the surrounding tools (which now use
createJiraClient(jiraSources)), this block hand-rolls Basic auth usingjiraUserConfig?.email/apiTokenand env vars directly. That has two consequences:
- Project-scoped
.marvin/config.yamlfor a per-project email (the new feature this PR introduces),fetch_jira_statuseswill authenticate with the wrong (or missing) identity, while other tools resolve correctly.- Non-null assertions (
!) onundefinedvalues produce an"undefined:undefined"Basic header at runtime rather than a clean "not configured" error.createJiraClientalready returnsnullin that case (handled viajiraNotConfiguredError()), so the early null-check is misleading — by the time we reach line 769, only the user config's email/token are considered.Consider exposing the resolved credentials from
createJiraClient(or a sibling helper) so this tool can reuse them, or switch tojira.clientfor the search instead of a rawfetch.♻️ Sketch
- const jira = createJiraClient(jiraSources); + const jira = createJiraClient(jiraSources); if (!jira) return jiraNotConfiguredError(); - // Use v3 search/jql to get statuses - const host = jira.host; - const auth = `Basic ${Buffer.from( - `${(jiraUserConfig?.email ?? process.env.JIRA_EMAIL)!}:${(jiraUserConfig?.apiToken ?? process.env.JIRA_API_TOKEN)!}`, - ).toString("base64")}`; - - const params = new URLSearchParams({ ... }); - const resp = await fetch(`https://${host}/rest/api/3/search/jql?${params}`, { ... }); + const result = await jira.client.searchIssuesV3( + `project = ${resolvedProjectKey}`, + ["status"], + args.maxResults ?? 100, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/skills/builtin/jira/tools.ts` around lines 764 - 781, fetch_jira_statuses currently builds a hand-rolled Basic auth from jiraUserConfig/env vars which bypasses createJiraClient's credential resolution and can emit "undefined:undefined"; change it to reuse the resolved client/credentials from createJiraClient instead of constructing the Authorization header manually: call createJiraClient(jiraSources), return jiraNotConfiguredError() if null, then either use jira.client (the Jira API client returned by createJiraClient) to run the JQL search or have createJiraClient expose the resolved email/apiToken pair and use those values (no non-null assertions) to build auth; remove the inline Basic header logic in fetch_jira_statuses so project-scoped overrides are honored and missing credentials produce the existing not-configured error path.
🧹 Nitpick comments (4)
src/agent/tools/doctor.ts (1)
166-207:check_integrationscorrectly avoids exposing secrets.
resolveJiraStatusreturns only presence/source metadata (plus normalized host), so passingloadUserConfig().jirathrough it is safe. Confluence derivation from the same Atlassian credentials is reasonable.One small suggestion: wrap the body in a try/catch consistent with the sibling tools (
run_doctor,check_project_health,get_started) —loadUserConfig()can throwConfigErrorif the YAML is malformed, which would currently surface as an unhandled tool error.🛡️ Proposed fix
async () => { + try { const jiraSources = { project: options?.config?.jira ? { host: options.config.jira.host, email: options.config.jira.email } : undefined, user: loadUserConfig().jira, }; ... return { content: [{ type: "text" as const, text: JSON.stringify(result, null, 2) }], }; + } catch (err) { + return { + content: [ + { + type: "text" as const, + text: `check_integrations error: ${err instanceof Error ? err.message : String(err)}`, + }, + ], + isError: true, + }; + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/tools/doctor.ts` around lines 166 - 207, Wrap the async body of the "check_integrations" tool in a try/catch like the sibling tools (run_doctor, check_project_health, get_started) to catch loadUserConfig() errors (e.g., ConfigError) and return a controlled tool response instead of letting the error bubble; specifically, surround the calls to loadUserConfig() and resolveJiraStatus() with try, catch the ConfigError (and fallback to a generic catch), and in the catch return the same response shape used by other tools (content with a text message describing the config parse error) so secrets remain protected and malformed YAML is reported gracefully.test/skills/jira/client-project.test.ts (1)
5-9: Consider restoring env in place rather than reassigningprocess.env.
process.env = { ...originalEnv }replaces the specialprocess.envproxy with a plain object, which loses Node's automatic string coercion and case-insensitive behavior on Windows. It's harmless for these tests today, but if future tests rely on those semantics (or spawn child processes) they could behave unexpectedly. Prefer mutating in place:♻️ Suggested cleanup
afterEach(() => { - process.env = { ...originalEnv }; + for (const k of Object.keys(process.env)) { + if (!(k in originalEnv)) delete process.env[k]; + } + Object.assign(process.env, originalEnv); });Also applies to the duplicated block at Lines 81-85.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/skills/jira/client-project.test.ts` around lines 5 - 9, The test resets process.env by reassigning it to a plain object (process.env = { ...originalEnv }), which breaks Node's special env proxy; instead, in the afterEach handler (the afterEach block referencing originalEnv and process.env) restore keys in place by removing any keys not in originalEnv and setting all keys from originalEnv on process.env so the original proxy semantics are preserved; apply the same in the duplicated cleanup block around lines referenced (the second afterEach that also uses originalEnv/process.env).src/skills/builtin/jira/client.ts (2)
280-305: Minor: redundant host normalization and discriminator edge case.Two small observations on the refactor:
Line 304 passes the raw
hosttonew JiraClient(...), whose constructor (Line 123) re-applies the samereplace(/^https?:\/\//, "").replace(/\/+$/, ""). You already computenormalizedHoston Line 303 — pass that in to avoid the duplicated regex pass and keep the normalization source-of-truth single.
isConfigSources(Line 342-347) discriminates purely by the presence of"project"or"user"keys. That's fine today because legacyJiraConfighas neither, but the guard will misclassify any future legacy object that happens to gain auserfield. Consider narrowing it to reject shapes that also containapiToken/host/createJiraClient(user)andcreateJiraClientFromSources(sources)) for clearer intent.♻️ Optional fix for (1)
const normalizedHost = host.replace(/^https?:\/\//, "").replace(/\/+$/, ""); - return { client: new JiraClient({ host, email, apiToken }), host: normalizedHost }; + return { client: new JiraClient({ host: normalizedHost, email, apiToken }), host: normalizedHost };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/skills/builtin/jira/client.ts` around lines 280 - 305, createJiraClient currently computes normalizedHost but passes the raw host into new JiraClient causing duplicated normalization; update createJiraClient to pass normalizedHost into the JiraClient constructor (return { client: new JiraClient({ host: normalizedHost, email, apiToken }), host: normalizedHost }). Also tighten the isConfigSources type guard (or add explicit overloads like createJiraClient(user) and createJiraClientFromSources(sources)) so it does not treat an object with top-level credentials (apiToken/host/email) as a sources object — e.g., ensure the guard rejects objects that contain apiToken/host/email at the top level or use separate functions to disambiguate legacy vs new shapes.
307-340:resolveJiraStatuslooks correct and secret-safe.Source labelling precedence (
project→user→env), host-onlyvalueexposure, and the deliberate omission ofapiTokenvalues from the return shape all match what thecheck_integrationstool needs. Normalization of the reported host value is consistent withcreateJiraClient.One nit: the three
sourcefields are typed asstring— tightening to a literal union ("project" | "user" | "env") would give downstream consumers (e.g. the doctor tool) better type safety.♻️ Optional type tightening
-export function resolveJiraStatus(sources?: JiraConfigSources): { - host: { configured: boolean; value?: string; source?: string }; - email: { configured: boolean; source?: string }; - apiToken: { configured: boolean; source?: string }; -} { +type JiraCredentialSource = "project" | "user" | "env"; +export function resolveJiraStatus(sources?: JiraConfigSources): { + host: { configured: boolean; value?: string; source?: JiraCredentialSource }; + email: { configured: boolean; source?: JiraCredentialSource }; + apiToken: { configured: boolean; source?: Exclude<JiraCredentialSource, "project"> }; +} {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/skills/builtin/jira/client.ts` around lines 307 - 340, The return type of resolveJiraStatus should tighten the three source fields from plain string to the literal union "project" | "user" | "env" (or undefined where appropriate) so callers get stronger typing; update the function's return type signature (the host.source, email.source, apiToken.source fields) to use that union, adjust any local conditional expressions if needed to keep undefined as a possible value, and ensure the implementation still returns the same "project"/"user"/"env" strings from resolveJiraStatus.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/guides/jira.md`:
- Around line 48-60: Update the example payload for the check_integrations
output in docs/guides/jira.md to match the actual response shape produced by
src/agent/tools/doctor.ts by adding the missing emailSource and apiTokenSource
fields under the "jira" object; locate the sample block that shows the
check_integrations JSON and insert emailSource and apiTokenSource with
representative values (e.g., "user" or "env") so the docs reflect the real tool
output produced by check_integrations in doctor.ts.
---
Outside diff comments:
In `@src/skills/builtin/jira/tools.ts`:
- Around line 764-781: fetch_jira_statuses currently builds a hand-rolled Basic
auth from jiraUserConfig/env vars which bypasses createJiraClient's credential
resolution and can emit "undefined:undefined"; change it to reuse the resolved
client/credentials from createJiraClient instead of constructing the
Authorization header manually: call createJiraClient(jiraSources), return
jiraNotConfiguredError() if null, then either use jira.client (the Jira API
client returned by createJiraClient) to run the JQL search or have
createJiraClient expose the resolved email/apiToken pair and use those values
(no non-null assertions) to build auth; remove the inline Basic header logic in
fetch_jira_statuses so project-scoped overrides are honored and missing
credentials produce the existing not-configured error path.
---
Nitpick comments:
In `@src/agent/tools/doctor.ts`:
- Around line 166-207: Wrap the async body of the "check_integrations" tool in a
try/catch like the sibling tools (run_doctor, check_project_health, get_started)
to catch loadUserConfig() errors (e.g., ConfigError) and return a controlled
tool response instead of letting the error bubble; specifically, surround the
calls to loadUserConfig() and resolveJiraStatus() with try, catch the
ConfigError (and fallback to a generic catch), and in the catch return the same
response shape used by other tools (content with a text message describing the
config parse error) so secrets remain protected and malformed YAML is reported
gracefully.
In `@src/skills/builtin/jira/client.ts`:
- Around line 280-305: createJiraClient currently computes normalizedHost but
passes the raw host into new JiraClient causing duplicated normalization; update
createJiraClient to pass normalizedHost into the JiraClient constructor (return
{ client: new JiraClient({ host: normalizedHost, email, apiToken }), host:
normalizedHost }). Also tighten the isConfigSources type guard (or add explicit
overloads like createJiraClient(user) and createJiraClientFromSources(sources))
so it does not treat an object with top-level credentials (apiToken/host/email)
as a sources object — e.g., ensure the guard rejects objects that contain
apiToken/host/email at the top level or use separate functions to disambiguate
legacy vs new shapes.
- Around line 307-340: The return type of resolveJiraStatus should tighten the
three source fields from plain string to the literal union "project" | "user" |
"env" (or undefined where appropriate) so callers get stronger typing; update
the function's return type signature (the host.source, email.source,
apiToken.source fields) to use that union, adjust any local conditional
expressions if needed to keep undefined as a possible value, and ensure the
implementation still returns the same "project"/"user"/"env" strings from
resolveJiraStatus.
In `@test/skills/jira/client-project.test.ts`:
- Around line 5-9: The test resets process.env by reassigning it to a plain
object (process.env = { ...originalEnv }), which breaks Node's special env
proxy; instead, in the afterEach handler (the afterEach block referencing
originalEnv and process.env) restore keys in place by removing any keys not in
originalEnv and setting all keys from originalEnv on process.env so the original
proxy semantics are preserved; apply the same in the duplicated cleanup block
around lines referenced (the second afterEach that also uses
originalEnv/process.env).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f3f8852-d35e-4eae-8a71-b99122a43f8b
📒 Files selected for processing (11)
docs/guides/getting-started.mddocs/guides/jira.mddocs/guides/mcp-server.mddocs/guides/personas.mddocs/reference/configuration.mdsrc/agent/session.tssrc/agent/tools/doctor.tssrc/core/config.tssrc/skills/builtin/jira/client.tssrc/skills/builtin/jira/tools.tstest/skills/jira/client-project.test.ts
- fetch_jira_statuses: replace hand-rolled Basic auth with jira.client.searchIssuesV3 so project-scoped overrides are honored - check_integrations: wrap in try/catch for ConfigError safety - createJiraClient: pass normalizedHost to JiraClient constructor to avoid double normalization - isConfigSources: reject objects with top-level host/email/apiToken to prevent legacy objects being misidentified as sources - resolveJiraStatus: tighten source field to "project"|"user"|"env" literal union - Test: restore process.env via Reflect.deleteProperty instead of reassigning the proxy object - Docs: add missing emailSource/apiTokenSource to jira.md example
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/skills/builtin/jira/client.ts (1)
311-343: Consider extracting the inline return type.The return shape of
resolveJiraStatusis likely consumed bysrc/agent/tools/doctor.tsto build thecheck_integrationspayload. Naming it (e.g.,JiraIntegrationStatus) and exporting would let downstream modules reference the exact shape instead of duplicating it, and keeps the signature readable.♻️ Proposed refactor
+export interface JiraIntegrationStatus { + host: { configured: boolean; value?: string; source?: CredentialSource }; + email: { configured: boolean; source?: CredentialSource }; + apiToken: { configured: boolean; source?: CredentialSource }; +} + /** Check which Jira credentials are present without exposing values. */ -export function resolveJiraStatus(sources?: JiraConfigSources): { - host: { configured: boolean; value?: string; source?: CredentialSource }; - email: { configured: boolean; source?: CredentialSource }; - apiToken: { configured: boolean; source?: CredentialSource }; -} { +export function resolveJiraStatus(sources?: JiraConfigSources): JiraIntegrationStatus {Note:
CredentialSourceis declared at Line 345 below this function; moving it above (or exporting it) would also be cleaner if the new interface is introduced.As per coding guidelines: "Use
interfacefor object shapes,typefor unions/intersections/aliases".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/skills/builtin/jira/client.ts` around lines 311 - 343, The function resolveJiraStatus currently returns an inline object type; extract and export a named interface (e.g., JiraIntegrationStatus) that models the returned shape (host, email, apiToken with configured/value/source fields) and update resolveJiraStatus's signature to return JiraIntegrationStatus; ensure CredentialSource is available (move its declaration above or export it) so the interface can reference it, and export the new interface so downstream modules like src/agent/tools/doctor.ts can import and reuse the exact type.docs/guides/jira.md (1)
48-62: Add a language specifier to the fenced block.markdownlint flags MD040 on line 48. The block content is JSON, so label it as such for consistent rendering and linting.
📝 Proposed doc update
-``` +```json > check_integrations { "jira": {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/jira.md` around lines 48 - 62, The fenced code block starting with the line "> check_integrations" is missing a language specifier; update that markdown block to use "json" (i.e., change the opening fence from ``` to ```json) so the JSON payload (the object with "jira", "configured", "host", "emailConfigured", etc.) is properly recognized and satisfies markdownlint MD040; locate the block containing > check_integrations and the JSON object and add the json label to the opening ``` fence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/guides/jira.md`:
- Around line 48-62: The fenced code block starting with the line ">
check_integrations" is missing a language specifier; update that markdown block
to use "json" (i.e., change the opening fence from ``` to ```json) so the JSON
payload (the object with "jira", "configured", "host", "emailConfigured", etc.)
is properly recognized and satisfies markdownlint MD040; locate the block
containing > check_integrations and the JSON object and add the json label to
the opening ``` fence.
In `@src/skills/builtin/jira/client.ts`:
- Around line 311-343: The function resolveJiraStatus currently returns an
inline object type; extract and export a named interface (e.g.,
JiraIntegrationStatus) that models the returned shape (host, email, apiToken
with configured/value/source fields) and update resolveJiraStatus's signature to
return JiraIntegrationStatus; ensure CredentialSource is available (move its
declaration above or export it) so the interface can reference it, and export
the new interface so downstream modules like src/agent/tools/doctor.ts can
import and reuse the exact type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2e6e7a7-4795-4ccc-a978-f18999eda31a
📒 Files selected for processing (6)
docs/guides/jira.mdpackage.jsonsrc/agent/tools/doctor.tssrc/skills/builtin/jira/client.tssrc/skills/builtin/jira/tools.tstest/skills/jira/client-project.test.ts
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/agent/tools/doctor.ts
- src/skills/builtin/jira/tools.ts
…type - Add json language specifier to check_integrations code block (MD040) - Extract and export CredentialSource and JiraIntegrationStatus from resolveJiraStatus inline return type for downstream reuse
1 participant
Summary
Implements feedback items #4 (Project-Scoped Jira Credentials) and #5 (Integration Health Check / Credential Safety), plus documentation updates covering all changes from this session.
Project-scoped Jira credentials (#4)
Jira
hostandemailcan now be set per-project in.marvin/config.yaml, overriding user config and environment variables. Resolution chain: project config → user config → env vars.The API token is intentionally excluded from project config to prevent committing secrets — it stays in user config (
~/.config/marvin/config.yaml) orJIRA_API_TOKENenv var.Backward-compatible: the legacy
createJiraClient(userConfig)signature still works unchanged.Integration health check (#5)
New
check_integrationsMCP tool that reports which credentials are configured without exposing secret values. Returns:This prevents agents from reading
~/.claude/mcp.jsonor other config files to check Jira setup.Documentation updates
Updated 5 doc pages covering all changes from this session (PRs #72-#75):
personas.md— PO use-case type, DM Sprint 0 guidancemcp-server.md— diagnostic tools tableconfiguration.md— project Jira overrides, AEM phase, credential resolution, env varsjira.md— project-scoped credentials, check_integrations examplegetting-started.md— onboarding toolsTest plan
test/skills/jira/client-project.test.ts— 10 tests: project override priority, fallback chain, legacy compat, resolveJiraStatus safetyjira.hostin project config, verify it takes priority over user configcheck_integrationsand verify no secrets appear in output