feat: implement compute profile detection and default tool policy#26
Closed
anand-testcompare wants to merge 5 commits into
Closed
feat: implement compute profile detection and default tool policy#26anand-testcompare wants to merge 5 commits into
anand-testcompare wants to merge 5 commits into
Conversation
6 tasks
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds centralized profile-policy infra and compute-module detection, introduces CLI/env profile override parsing and profile-aware flows, defaults unset Changes
Sequence DiagramsequenceDiagram
rect rgba(220,220,255,0.5)
participant User
end
rect rgba(200,255,200,0.5)
participant CLI as CLI (index.ts)
end
participant RepoScan as RepoScan
participant Resolver as ProfileResolver
participant Policy as PolicyRegistry
participant Allowlist as AllowlistEngine
participant Patcher as ConfigPatcher
participant Output as Summary
User->>CLI: run setup/rescan (optional --profile / env)
CLI->>RepoScan: scanRepoForProfile(worktree)
RepoScan-->>CLI: detectedProfile + reasons
CLI->>Resolver: resolveProfileOverride(cliArgs, env, detectedProfile)
Resolver-->>CLI: ProfileResolution (selected, source, reasons)
CLI->>Policy: getProfilePolicy(selectedProfile)
Policy-->>CLI: ProfilePolicy (librarian/foundry rules)
CLI->>Allowlist: computeAllowedTools(discoveredTools, policy)
Allowlist-->>CLI: ComputedAllowlist (allows, deniedTools, policy)
CLI->>Patcher: create/patch config (apply mode defaults, preserve explicit)
Patcher-->>CLI: PatchResult
CLI->>Output: formatPatchSummary(PatchResult, ProfileResolution, ComputedAllowlist)
Output-->>User: Summary (selected profile, source, reasons, policy preview)
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/palantir-mcp/types.ts (1)
20-28: Consider simplifyingparseProfileIdusing thePROFILE_IDSconstant.The function repeats each profile ID value that's already defined in
PROFILE_IDS. This could be simplified to reduce duplication and ensure consistency.♻️ Optional simplification
export function parseProfileId(value: unknown): ProfileId | null { - if (value === 'compute_modules') return value; - if (value === 'compute_modules_ts') return value; - if (value === 'compute_modules_py') return value; - if (value === 'pipelines_transforms') return value; - if (value === 'osdk_functions_ts') return value; - if (value === 'all') return value; - if (value === 'unknown') return value; - return null; + if (typeof value === 'string' && PROFILE_IDS.includes(value as ProfileId)) { + return value as ProfileId; + } + return null; }src/index.ts (1)
68-83: Consider splitting long description strings.Lines 72 and 80 contain description strings that exceed the 100-character line width guideline. While Prettier typically doesn't auto-split string literals, consider using array join for readability:
♻️ Suggested refactor for line width
if (!cfg.command['setup-palantir-mcp']) { cfg.command['setup-palantir-mcp'] = { template: 'Set up palantir-mcp for this repo.', description: - 'Guided MCP setup for Foundry. Usage: /setup-palantir-mcp <foundry_api_url> [--profile <profile_id>]. Requires FOUNDRY_TOKEN for tool discovery.', + 'Guided MCP setup for Foundry. ' + + 'Usage: /setup-palantir-mcp <foundry_api_url> [--profile <profile_id>]. ' + + 'Requires FOUNDRY_TOKEN for tool discovery.', }; } if (!cfg.command['rescan-palantir-mcp-tools']) { cfg.command['rescan-palantir-mcp-tools'] = { template: 'Re-scan palantir-mcp tools and patch tool gating.', description: - 'Re-discovers the palantir-mcp tool list and adds missing palantir-mcp_* toggles (does not overwrite existing toggles). Usage: /rescan-palantir-mcp-tools [--profile <profile_id>]. Requires FOUNDRY_TOKEN.', + 'Re-discovers the palantir-mcp tool list and adds missing palantir-mcp_* toggles ' + + '(does not overwrite existing toggles). ' + + 'Usage: /rescan-palantir-mcp-tools [--profile <profile_id>]. Requires FOUNDRY_TOKEN.', }; }
| `Valid profile IDs: ${formatProfileChoices()}`, | ||
| '', | ||
| 'Example:', | ||
| ' /setup-palantir-mcp https://23dimethyl.usw-3.palantirfoundry.com --profile compute_modules_ts', |
Owner
Author
There was a problem hiding this comment.
replace with something else thts not mine, make the example funny.. easter egg
Owner
Author
There was a problem hiding this comment.
Updated in cae2bde with a non-personal easter-egg URL: https://totally-not-skynet.palantirfoundry.com.
Owner
Author
There was a problem hiding this comment.
Updated again in 764299a to a more unhinged pun: https://who-let-the-dag-out.palantirfoundry.com.
Owner
Author
There was a problem hiding this comment.
Updated per request in 587cddc: https://buschlightdid911.usw-3.palantirfoundry.com.
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@src/palantir-mcp/commands.ts`:
- Around line 121-196: Both parseSetupArgs and parseRescanArgs duplicate the
same --profile parsing logic; extract that logic into a small helper (e.g.,
parseProfileFlag) that accepts the token array and current index (or returns a
tuple of {value, consumed, error}) and reuse it from both functions; update
parseSetupArgs and parseRescanArgs to call tokenizeArgs, invoke the new
parseProfileFlag when encountering '--profile' or '--profile=', handle returned
errors/consumed count, and preserve existing positional-argument behaviors in
parseSetupArgs (single Foundry URL) and parseRescanArgs (no positional args).
- Around line 113-119: The tokenizeArgs function contains a redundant .map((t)
=> t.trim()) because splitting with /\s+/ already strips inter-token whitespace;
update the function tokenizeArgs to remove the unnecessary map step and simply
trim the input, split on /\s+/ and filter out empty tokens (i.e., keep the
.trim(), .split(/\s+/g) and .filter((t) => t.length > 0) but drop the .map
call).
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@src/palantir-mcp/commands.ts`: - Around line 121-196: Both parseSetupArgs and parseRescanArgs duplicate the same --profile parsing logic; extract that logic into a small helper (e.g., parseProfileFlag) that accepts the token array and current index (or returns a tuple of {value, consumed, error}) and reuse it from both functions; update parseSetupArgs and parseRescanArgs to call tokenizeArgs, invoke the new parseProfileFlag when encountering '--profile' or '--profile=', handle returned errors/consumed count, and preserve existing positional-argument behaviors in parseSetupArgs (single Foundry URL) and parseRescanArgs (no positional args). - Around line 113-119: The tokenizeArgs function contains a redundant .map((t) => t.trim()) because splitting with /\s+/ already strips inter-token whitespace; update the function tokenizeArgs to remove the unnecessary map step and simply trim the input, split on /\s+/ and filter out empty tokens (i.e., keep the .trim(), .split(/\s+/g) and .filter((t) => t.length > 0) but drop the .map call).src/palantir-mcp/commands.ts (2)
121-196: Consider extracting shared--profileparsing logic.Both
parseSetupArgsandparseRescanArgscontain identical logic for handling--profileand--profile=flags (lines 128-143 and 168-183). While the functions have different positional argument requirements, the profile parsing could be extracted into a shared helper to reduce duplication.This is a low-priority refactor given the functions are readable and the duplication is contained.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/palantir-mcp/commands.ts` around lines 121 - 196, Both parseSetupArgs and parseRescanArgs duplicate the same --profile parsing logic; extract that logic into a small helper (e.g., parseProfileFlag) that accepts the token array and current index (or returns a tuple of {value, consumed, error}) and reuse it from both functions; update parseSetupArgs and parseRescanArgs to call tokenizeArgs, invoke the new parseProfileFlag when encountering '--profile' or '--profile=', handle returned errors/consumed count, and preserve existing positional-argument behaviors in parseSetupArgs (single Foundry URL) and parseRescanArgs (no positional args).
113-119: Minor: Redundant.map((t) => t.trim())after whitespace split.Splitting on
/\s+/gwon't produce tokens with leading/trailing whitespace, making the.map((t) => t.trim())call redundant.♻️ Suggested simplification
function tokenizeArgs(rawArgs: string): string[] { return rawArgs .trim() .split(/\s+/g) - .map((t) => t.trim()) .filter((t) => t.length > 0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/palantir-mcp/commands.ts` around lines 113 - 119, The tokenizeArgs function contains a redundant .map((t) => t.trim()) because splitting with /\s+/ already strips inter-token whitespace; update the function tokenizeArgs to remove the unnecessary map step and simply trim the input, split on /\s+/ and filter out empty tokens (i.e., keep the .trim(), .split(/\s+/g) and .filter((t) => t.length > 0) but drop the .map call).
anand-testcompare
deleted the
22-compute-module-profile-detection-and-default-tool-policy
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
compute_modules,compute_modules_ts,compute_modules_py) and deterministic repo-scan scoring with reason tracessrc/palantir-mcp/profiles/and switch tool gating to policy-driven broad defaults with a minimal destructive deny list--profile, plus env fallback) with explicit output showing selected profile, detected profile, and policy summaryagent.foundry.modetoallwhen unset across config hook + setup/rescan/bootstrap flows while preserving explicit user mode valuesValidation
mise run formatmise run lintmise run typecheckmise run buildmise run testbun run scripts/dev/link-into-repo.ts --target /tmp/opencode-regression-1771222961 --source distbun run scripts/dev/run-opencode-command.ts --repo /tmp/opencode-regression-1771222961 --command setup-palantir-mcp --args "$FOUNDRY_URL --profile compute_modules_ts"bun run scripts/dev/run-opencode-command.ts --repo /tmp/opencode-regression-1771222961 --command rescan-palantir-mcp-tools --args "--profile compute_modules"Notes
mise run smokestill fails in this environment because the local docs snapshot has too few indexed pages (list_all_docs returned only 0 pages, expected 3000+).Closes #22
Summary by CodeRabbit
New Features
Bug Fixes
Tests