Skip to content

Conversation

@ssalbdivad
Copy link
Collaborator

No description provided.

Copy link

@pullfrog pullfrog bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid refactor. This replaces the binary sandbox flag with granular readonly, network, and bash controls, giving more flexibility. The precedence ordering (sandbox → payload agentConfig → repo settings → defaults) is clear. A few issues to address before merging.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow runpullfrog.com𝕏

updatedPermissions: [],
};

console.error("can i use this tool?", toolName);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug log left in. Remove this console.error before merging.


if (payload.sandbox) {
log.info("🔒 sandbox mode enabled: restricting to read-only operations");
const cliArgs = parseCliArgs(agentConfig.cliArgs);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parsed cliArgs are logged but never used. They need to be passed to the query() call or removed if not needed for the Claude SDK.

export function parseCliArgs(cliArgs: string): string[] {
if (!cliArgs.trim()) return [];
// split on spaces, respecting double quotes, single quotes, and backticks
return cliArgs.match(/(?:[^\s"'`]+|"[^"]*"|'[^']*'|`[^`]*`)+/g) || [];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This regex doesn't handle escaped quotes within quoted strings (e.g., --flag "value with \"nested\" quotes"). For CLI args this is likely fine, but worth noting in the docstring that escaping is not supported.

*/

import { type } from "arktype";
import { z } from "zod";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding zod as a dependency to a file that's imported by Next.js adds bundle weight. Since arktype is already used here, consider using arktype for this schema too, or move the zod schema to a separate file that's only imported server-side.

if (agent.name === "opencode") {
const opencodeKeys: Array<[string, string | undefined]> = [];
for (const [key, value] of Object.entries(process.env)) {
if (value && typeof value === "string" && key.includes("API_KEY")) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition key.includes("API_KEY") will match env vars like SOME_OTHER_API_KEY_BACKUP and could leak unintended keys. Consider using a stricter pattern or an explicit allowlist.

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.

2 participants