diff --git a/.jules/bolt.md b/.jules/bolt.md new file mode 100644 index 00000000..1e94760c --- /dev/null +++ b/.jules/bolt.md @@ -0,0 +1,55 @@ +# Bolt's Performance Journal + +⚡ Focus on measurably speeding up the application. Keep optimizations clean, documented, and non-breaking. + +## 2026-04-06 - Batched Index Appends in Bulk Session Upsertion +**Learning:** Parallelizing individual database or cache writes using `Promise.all` can cause massive lock contention, redundant file system descriptor usage, and I/O bottlenecks when each concurrent task appends independently to a single shared log or index file. Writing individual session state files concurrently preserves atomic I/O performance, while aggregating all metadata entries in memory to execute a single, collective `fs.appendFile` batch operation on the shared index file reduces system call overhead from $O(N)$ to $O(1)$ and guarantees clean write alignment. +**Action:** Always batch append operations to shared flat files (such as index, registry, or JSONL databases) when performing bulk state operations, while keeping distinct, non-overlapping document-level writes concurrent. + +## 2026-03-30 - Optimized Top-Level Document Projections Fast-Path +**Learning:** During multi-document database queries, projecting document fields to lightweight subsets (especially default session/activity projections) is extremely frequent. Invoking a generic projection engine that splits paths, scans for wildcards/exclusions, checks nested structures, and deep-clones primitives introduces major redundant CPU and GC overhead. Pre-calculating a boolean flag `isSimpleTopLevel` in a cached query plan can safely bypass the entire heavy projection engine for flat queries. +**Action:** Always check if a highly repetitive document/object mapping or projection can be pre-calculated and cached. When a selection is strictly simple and flat, implement an O(1) loop copy with direct primitive mapping to achieve up to ~50% CPU performance improvements in hot paths. + +## 2026-03-31 - Optimized Query Selection Path & Avoided Sorter Object Allocations +**Learning:** Performing `new Date()` construction and parsing inside an $O(N \log N)$ sort comparator is a significant performance bottleneck due to thousands of repeated string parsings and heap allocations. Pre-parsing timestamps in the $O(N)$ hydration/projection phase and caching them as numbers in the `_sortKey` object completely eliminates this overhead. Additionally, calling `Object.entries()` inside loops that run on every matched document causes substantial GC pressure; replacing it with a simple `for...in` loop prevents those redundant array allocations entirely. +**Action:** Never perform date parsing or string operations inside high-frequency sort comparators. Precompute sort keys as primitives during object mapping. Always replace key/value array helper functions with native loops in high-frequency matching hot paths. + +## 2026-04-01 - Avoided Heap Allocation via Primitive-Returning Date.parse & Date.now +**Learning:** Instantiating `new Date()` objects repeatedly inside high-frequency search and scan loops (such as query selection sorting fallbacks, high-watermarking scan loops, cache tier evaluations, and session asking loops) introduces major garbage collection overhead and memory churn. Using standard native primitive-returning helpers like `Date.parse()` and `Date.now()` completely bypasses V8 heap allocation of date objects, making date comparisons up to ~10x faster and eliminating GC pauses. +**Action:** Always prefer `Date.parse(string)` and `Date.now()` over `new Date(string)` or `new Date()` for date comparisons, subtraction, and arithmetic operations in hot execution paths. + +## 2026-04-02 - Optimized Deep Query Projections and String Sort Collations +**Learning:** Recursive query projection routines (`getPath`, `setPath`, `deletePath`) split and slice path arrays extensively on every nested field lookup. Replacing sub-path array slicing and destructuring with an explicit `index` traversal parameter completely eliminates redundant array allocations and garbage collection pressure in the query pipeline. Additionally, using standard comparison operators (`<`, `>`) instead of locale-sensitive `localeCompare` inside hot sort comparisons is up to 10x faster for simple alphanumeric identifiers. +**Action:** Avoid allocating sub-arrays/slices in recursive traversal functions. Always prefer direct comparison operators over slow collation methods like `localeCompare` for sorting non-localized key and ID fields. + +## 2026-04-03 - Optimized Unified Diff Parsing via Single-Pass State Machine +**Learning:** Parsing unified git diff files frequently in activity-syncing pipelines introduces heavy CPU and garbage collection overhead when using regex-based split operations (`patch.split(/^diff --git /m)`). Switching to a single-pass, line-by-line state machine completely eliminates sub-string filtering, nested splits, and multiple line scans, yielding a massive performance boost. Furthermore, handling Windows CRLF `\r\n` line endings by explicitly sanitizing trailing `\r` prevents critical parsing regressions across cross-platform environments. +**Action:** Always parse structured sequential files (like diffs, logs, or JSONL) in a single sequential line iteration instead of multiple high-level split passes. Ensure cross-platform line termination safety by removing trailing `\r` carriage returns. + +## 2026-04-04 - Pre-compiled Query Projection Plans +**Learning:** Standard document projection pipelines recursively traverse object/array hierarchies and dynamically map, join, and filter nested inclusion and exclusion paths on every matched document. Allocating arrays for `.filter()`, `.map()`, and string `.join('.')` inside hot scanning/hydration loops introduces immense heap churn and garbage collection pressure. Pre-compiling nested select selectors and exclusion sub-paths into cached, query-level plan structures completely eliminates allocation overhead during runtime document iteration. +**Action:** Move all structural conversions, filtering, and array-mapping operations of metadata patterns up to the query compilation/planning phase and cache them, ensuring document iteration loops only perform O(1) dictionary lookups. + +## 2026-04-05 - Compiled Query Where-Clause Filters +**Learning:** In high-frequency query scanning, filtering, and document hydration loops, repeatedly parsing raw filter structures (e.g., checking `typeof filter`, splitting nested dot-notation paths on `.` to traverse sub-properties, lowercasing substring filters on every comparison, and scanning arrays linearly via `.includes`) causes significant CPU overhead and garbage collection pressure due to frequent string and object allocations. Pre-compiling raw query filters into structured compiled filters with pre-split paths, pre-lowercased patterns, and `Set` collections for $O(1)$ inclusion checks completely eliminates redundant string splits and heap allocations. +**Action:** Always pre-compile query/where clauses and search metadata patterns once before entering hot execution/iteration loops over collections of documents, and use non-allocating native loops rather than closure-based helpers inside highly repetitive matching logic. + +## 2026-04-06 - Bypassed Hydration Mapping to Prevent Object and Array Allocation +**Learning:** Repeatedly mapping arrays of nested objects (such as activity artifacts) to hydrate them into rich class instances introduces substantial memory allocation and garbage collection pressure. Even when all objects are already hydrated instances, standard `.map()` calls inevitably allocate a new array and clone the parent object. Pre-checking if any elements actually require hydration allows bypassing the entire mapping, array allocation, and cloning pipeline entirely, achieving reference-equal O(1) returns. +**Action:** Before performing mapping, copying, or hydration on collections of objects, always pre-check if the collection is already in its final desired state. If no transformation is needed, return the original parent object directly. + +## 2026-04-07 - Optimized Array Elements Projection Fast-Path +**Learning:** When projecting array properties in a query (such as activities, outputs, or artifacts) to include all fields without specific nested sub-property paths or exclusions, the projection engine was instantiating empty object structures and traversing fields property-by-property via `getPath`/`setPath` for every element in the array. Bypassing this engine for arrays when `subPaths` only consists of empty subpaths (`subPaths.length === 1 && subPaths[0].length === 0`) and no exclusions are specified, and instead performing a direct fast-path element copy via `deepClone(item)`, completely avoids unnecessary CPU processing and garbage collection allocations. +**Action:** Always check if a loop mapping or deep traversal can be safely bypassed or fast-pathed when the target transformation evaluates to copying whole items as-is. + +## 2026-04-08 - Optimized Concurrency Mapping and Fast-Path Collections in Parallel Execution +**Learning:** Core concurrency utility functions (such as parallel maps or batch workers) can introduce massive promise allocation and garbage collection churn if they construct worker pools that exceed the collection size or rely on array allocation helper chains (like `new Array(N).fill(0).map(...)`). Limiting active worker pool allocation to `Math.min(concurrency, items.length)` completely avoids redundant, idle worker promises. Furthermore, implementing direct fast-paths for empty and single-item inputs completely bypasses entire worker pool construction, scheduler loops, and error-aggregation array mappings, reducing runtime scheduling overhead from $O(N)$ to $O(1)$. +**Action:** Always optimize parallel mapper / concurrency pool utilities by providing O(1) fast-paths for empty and single-item arrays, avoiding overhead and allocation entirely. + +## 2026-08-11 - Batched Ingestion & Metadata Caching for Immutable Activities +**Learning:** Repeatedly appending immutable records (such as activities) in sequence triggers a massive I/O bottleneck when each write operation performs independent read-modify-write loops on a centralized metadata file, plus discrete file appends. Moving metadata state to an in-memory cache completely eliminates redundant $O(N)$ reads, while exposing a batch `appendMany` endpoint allows aggregating all log appends and committing the metadata update precisely once on disk per batch, reducing system call and disk I/O overhead from $O(N)$ to $O(1)$. +**Action:** Always provide batch write APIs for log/index appending, and cache repeatedly read configuration or count files in-memory to prevent disk thrashing during high-frequency sync loops. + +## 2026-08-13 - Resolving Leftover Redundant Blocks and Duplicate Member Fields in Merged Code +**Learning:** During complex refactorings or merges, leftover redundant blocks (such as calling the duplicate appending block with the undefined `toAppend` variable in the hydration pipeline) and duplicate field definitions (such as `metadataCache` in `NodeFileStorage`) can slip through, causing critical runtime crashes and TypeScript compile-time errors. Locating and cleanly removing these leftover duplicate blocks completely resolves the exceptions, restoring 100% test coverage and compile correctness. +**Action:** Always verify compilation via `bun run build` and run full unit/integration test suites to identify any leftover/duplicate rebase variables or class members, and cleanly prune them to ensure correctness. diff --git a/.jules/palette.md b/.jules/palette.md new file mode 100644 index 00000000..f9e9d7ef --- /dev/null +++ b/.jules/palette.md @@ -0,0 +1,43 @@ +# Palette's Journal - CLI UX & Accessibility + +This journal records critical, reusable UX and accessibility insights specific to this codebase. + +## 2026-03-01 - Interactive TTY Styling for Node CLI Orchestration +**Learning:** Users of rich CLI tools (such as orchestrators) need clear visual hierarchies to quickly scan long logs. Plain-text logs without colored status indicators (like checkmarks or warning symbols) increase cognitive load. Adding green, red, yellow, and dim ANSI escapes in interactive TTY environments significantly improves readability, while maintaining clean unstyled text in CI logs preserves log grepability and compatibility. +**Action:** Next time when rendering CLI/TUI outputs, always introduce conditional ANSI styles that detect TTY/non-CI environments to color-code success checkmarks (green), errors/failures (red), warnings (yellow), and secondary metadata/fractions (dim). + +## 2026-03-05 - Backtick Command Highlighting in CLI Suggestions and Errors +**Learning:** Highlighting inline code and command names (wrapped in backticks) in terminal suggestions drastically reduces the user's cognitive friction when dealing with errors. It makes actionable instructions (such as "Use `jules-fleet configure` to update settings") immediately stand out visually in interactive terminals without adding noise in plain CI/log environments. +**Action:** Implement conditional regex-based ANSI formatting on terminal error suggestions to highlight backticked segments in yellow or bold colors when process/stdout is interactive, falling back to clean plain-text when run in CI. + +## 2026-03-08 - Informative CLI Resource Creation Feedback +**Learning:** Long-running CLI orchestration tasks that create remote resources (like Git repositories) require continuous visual feedback (spinners for in-flight tasks, and clear success/warning/error states on completion). Leaving key domain events silent increases user anxiety and cognitive load. Providing clickable, descriptive OSC 8 terminal hyperlinks (like "View Repository") dramatically improves user discoverability and workflow efficiency directly inside TTY environments. +**Action:** Always ensure all domain-specific lifecycle events (especially resource creation and fallback states) are fully handled in rendering paths, using clear visual states (spinners, warning icons, and descriptive links instead of raw URLs). + +## 2026-03-12 - Clickable Wizard Hyperlinks and Standardized Failure Indicators +**Learning:** In interactive CLI applications (such as setup wizards), raw URLs lead to cluttered terminal screens, wrapping issues, and increased cognitive load. Using descriptive, semantic labels for terminal hyperlinks via OSC 8 makes clickable links look clean and highly discoverable. Additionally, standardizing colored failure status indicators (such as a red '✗') across all subcommand error paths provides unified visual scanning and consistency with existing checkmarks ('✓') and warning symbols ('⊘'). +**Action:** Always use semantic labels for `ansiLink` instead of passing raw URLs as labels in CLI prompts, and consistently prepend red '✗' to failure status outputs in terminal render blocks. + +## 2026-03-15 - Standardizing CLI Spinner Completion States with Status Symbols +**Learning:** In terminal UIs, replacing a running spinner with plain-text completion logs lacks immediate, scannable feedback. Appending standardized, color-coded completion status symbols (such as a green '✓' for success, red '✗' for errors/failures, and yellow '⊘' for timeout/skipped states) directly to the stopSpinner message creates a visually cohesive and satisfying experience that is instantly decodable. +**Action:** Always append standard, colored status indicators (such as `ansiGreen('✓')`, `ansiRed('✗')`, and `ansiYellow('⊘')`) to `ctx.stopSpinner` logs for completed terminal tasks. + +## 2026-03-16 - Unified Backtick Highlighting in CLI Setup Prompts and Status Outputs +**Learning:** In interactive CLI applications and wizard setups, user-specific variables (such as repository names, auth identities, and secret keys) printed in plain text lack distinct visual separation from instructions. Formatting these variables inside backticks and passing the strings through the `ansiHighlight` utility creates a highly professional, cohesive theme that naturally directs the user's attention to key parameters and command suggestions. +**Action:** Ensure that user-supplied inputs, paths, or keys in both interactive prompts and domain-specific status event logs (such as skipped/failed reasons) are formatted with backticks and highlighted using `ansiHighlight`. + +## 2026-03-17 - Interactive Password Validation in CLI Setup Wizards +**Learning:** In interactive setup wizards, missing input validation on password or token prompts allows users to accidentally submit empty or whitespace-only values (e.g., by hitting Enter too quickly). This leads to silent configuration issues or obscure downstream auth errors. Implementing robust inline validation for sensitive password prompts ensures immediate interactive feedback and prevents invalid setups. +**Action:** Always add inline `validate` check functions to both required and optional password/token prompts in CLI wizard flows to confirm they are non-empty and non-whitespace. + +## 2026-03-18 - Graceful Parsing and Guidance in CLI Interactive Prompts +**Learning:** Forcing users to enter exact strings (like `owner/repo` repository slugs) in interactive wizards often leads to validation errors and friction when they copy-paste full Git clone URLs (e.g., HTTPS/SSH links or with `.git` suffixes). Designing prompts to automatically and gracefully parse/clean input URL formats into standard identifiers, complemented by clear placeholder examples, provides an extremely smooth, error-free keyboard navigation and setup experience. +**Action:** Always clean and normalize complex identifiers or path inputs directly from the user's interactive clipboard/paste actions inside wizard text prompts before passing them to validators, and provide helpful, standardized `placeholder` example structures. + +## 2026-08-11 - Descriptive Option Hints in Interactive CLI Wizards +**Learning:** Selecting from multiple options in interactive CLI setup prompts (like choosing between GitHub PAT or App authentication) can be ambiguous or daunting for users who do not know the exact trade-offs of each option. Adding descriptive `hint` properties to selection options provides key contextual guidance that helps users make informed choices, while refactoring duplicate select blocks to reuse helper prompt functions ensures consistency across all Wizard flows. +**Action:** Always provide descriptive and highly informative `hint` messages for selection list options in interactive wizard prompts, and reuse core prompt functions to keep setup CLI interfaces unified and modular. + +## 2026-08-12 - Backtick Highlighting for Dynamic Repository Names in Creation Logs +**Learning:** Rendering raw repository slugs (such as `owner/repo`) in dynamic CLI creation event logs without formatting causes them to blend in with adjacent static log text. Formatting repository names in backticks and applying `ansiHighlight` creates visual separation and visual consistency across all CLI subcommands and setup steps. +**Action:** Always format repository slugs and dynamic resource identifiers in backticks and wrap them with `ansiHighlight` across all CLI event renderers. diff --git a/.jules/sentinel.md b/.jules/sentinel.md new file mode 100644 index 00000000..f49cfc41 --- /dev/null +++ b/.jules/sentinel.md @@ -0,0 +1,79 @@ +## 2026-08-18 - Input Validation on Fleet Dispatch Handler Entrypoint +**Vulnerability:** The Fleet `DispatchHandler` accepted repository owner/repo, base branch, and goals directory path inputs directly from callers or CLI flags without validation. Unsanitized inputs containing control characters, path traversals (`..`), or Git reference escapes (`refs/`) could lead to unsafe filesystem access during goal file scanning or Git reference escape during session dispatch. +**Learning:** Outer orchestration boundaries and CLI handler classes (like `DispatchHandler`) must defensively validate all input arguments using standard validation functions (`validateRepository`, `validateBranchName`, `validateFilePath`) before invoking filesystem operations or external session dispatchers. +**Prevention:** Always invoke input validators (`validateRepository`, `validateBranchName`, `validateFilePath`) at the start of execution handlers in orchestration packages before reading goal directories or dispatching remote worker sessions. + +## 2026-08-15 - Branch/Parent Validation on Reconciliation Staging +**Vulnerability:** The stage resolution handler (`stageResolutionHandler`) in `@google/jules-merge` accepted parent branch, commit, or PR identifier strings in the `parents` array without validating them against branch naming rules. If an attacker supplied parent strings containing spaces, control characters, consecutive dots, or `refs/` prefixes, it could allow git reference escape, format bypass, or script/command injection during downstream git commit/ref creation. +**Learning:** Any array input representing Git branch or reference identifiers must be validated at the entrypoint before being stored in state or passed to downstream Git operations. +**Prevention:** Iteratively invoke standard branch validators (`validateBranchName`) on all elements of array parameter inputs (such as `parents`) before performing staging or manifest persistence operations. + +## 2026-08-11 - Defense-in-depth Query Validation at MCP select Boundary +**Vulnerability:** The MCP `select` function passed incoming JQL queries directly to the SDK selection engine without validating the query's schema, structure, or parameters at the outermost protocol/package boundary. While the SDK itself validates queries, bypassing outer validation can expose the system to unexpected query engine behavior or unhandled schema exceptions if downstream validation is changed, mocked, or bypassed. +**Learning:** A secure architecture enforces multiple layers of protection (defense-in-depth). Input sanitization and validation must occur at every package, CLI, and protocol boundary to prevent assumptions about downstream state and block malicious inputs as early as possible. +**Prevention:** Always invoke standard SDK schema and query validators (`validateQuery`) directly at the entrypoint of all outer API, protocol, and tool functions before running database or caching queries. + +## 2026-08-02 - Base Branch Name Validation on Reconciliation Scanning +**Vulnerability:** The reconciliation scan handler in `@google/jules-merge` accepted a base branch name argument from user input (`input.base`) without any security or format validation, even though validators exist in the shared module. If an attacker supplied an escape string (such as starting with `refs/` or containing consecutive dots), it could lead to git reference escape or script injection down the call stack during Git actions or shell execution. +**Learning:** Downstream functions often rely on assumptions that upstream inputs have been sanitized. In modular architectures or monorepos, security validation of all user-supplied branch and ref parameters must be strictly and consistently enforced at the outermost orchestration and entrypoint handler layers. +**Prevention:** Import and actively invoke robust input validators (`validateBranchName`) on user-supplied branch parameters at all boundary entrypoints and handler functions before executing downstream Git operations or API requests. + +## 2026-08-01 - Input Validation of Pagination Page Tokens at SDK and MCP Boundaries +**Vulnerability:** Pagination page tokens passed from external clients or users through the MCP `list_sessions` tool and into the SDK's `SessionCursor` were accepted and used without any format or security validation. This opened potential risks for path/directory traversal, control character injection, or command injection if the tokens were later logged, cached on disk, or passed to downstream scripts/databases. +**Learning:** Even parameters perceived as internal sequence markers or pagination cursors (like `pageToken`) can originate from untrusted external environments and must be treated as unsafe inputs, requiring strict input sanitisation before being processed. +**Prevention:** Implement a dedicated, robust page token validator (`validatePageToken`) to reject any tokens containing control characters, path traversals (`.`, `..`), or path separators, and enforce it at both the MCP functional boundaries and the core SDK session pagination classes. + +## 2026-07-30 - Repository and Branch Name Validation at Fleet CLI Wizard Boundary +**Vulnerability:** The Fleet CLI initialization wizard (both headless and interactive modes) accepted unvalidated repository names and branch names from CLI arguments, environment variables, git remotes, or prompts. This allowed potential directory traversal, control character injection, or git reference escape attempts when downloading and writing workflow/config files. +**Learning:** Security validation must not only exist at downstream SDK layers but also be actively integrated at the outermost CLI command and setup wizard layers to sanitise all user/environment-supplied parameters before proceeding. +**Prevention:** Import and apply central robust validators (`validateRepository` and `validateBranchName`) directly at CLI entrypoint handlers and wizard modules, converting validation exceptions into cleanly formatted CLI failures. + +## 2026-07-29 - Robust Activity ID Validation at MCP Entrypoints +**Vulnerability:** While session IDs and file paths were validated, the `activityId` parameter provided by external clients through the MCP tool was accepted and used down the call stack (e.g. to filter activities) without any sanitization or validation, opening potential injection and path traversal vectors. +**Learning:** In a monorepo offering multiple protocol layers (like MCP tools), all identifiers supplied by untrusted external agents must be rigorously verified to be flat (no path separators, control characters, or traversal components) before being processed. +**Prevention:** Implement a reusable, unified validation function (`validateActivityId`) for activity identifiers and strictly apply it to all outer API/tool entrypoints receiving activity references. + +## 2026-07-28 - Unified File Path Validation and Integration in MCP showDiff +**Vulnerability:** While repository and branch name validations existed, user-supplied relative file path arguments (like the `file` option in the MCP `show_code_diff` tool) were passed to file extraction and diff filtering logic without checks. This opened a potential vector for control character injection, absolute path escapes, and directory traversal (`..`) attempts. +**Learning:** File paths supplied by untrusted external users/clients (such as through MCP tool execution) represent a major vulnerability surface and must be strictly validated at the outer protocol boundary before being processed. +**Prevention:** Implement a central, robust file path validator (`validateFilePath`) to enforce relative paths, reject control characters, absolute paths, and path traversal (`..`), and integrate it consistently across all outer API and tool boundaries. + +## 2026-07-27 - Input Validation at MCP Protocol Boundaries to Prevent Injection and Traversal +**Vulnerability:** Although core SDK functions had repository and branch validation, the @google/jules-mcp functions did not validate their incoming string parameters directly. In a monorepo, distinct packages can expose tools to external LLM execution environments (like MCP), representing a primary untrusted boundary that needs defensive sanitization. +**Learning:** Security validation must occur at every package or protocol boundary (like MCP, CLI, or API) to prevent downstream security leaks or unvalidated inputs in case downstream SDK methods are modified, bypassed, or directly called in unforeseen contexts. +**Prevention:** Always identify and validate untrusted input structures (such as `repo`, `branch`, and `sessionId`) at the absolute outermost boundary of protocol and tool handlers before passing them to internal client or filesystem logic. + +## 2026-07-24 - Integration of Repository Name Validation in Reconciliation Handlers +**Vulnerability:** Although a robust repository validator (`validateRepository`) was defined in `packages/merge/src/shared/validators.ts`, it was never actually invoked inside the reconciliation entry point handlers (`scanHandler`, `getContentsHandler`, `mergeHandler`, and `pushHandler` via `validatePushInput`). This left the package exposed to format-bypass, control character, and path-traversal attacks through untrusted repository input strings. +**Learning:** Having security utilities in the codebase is only the first step; they must be actively and consistently integrated at all untrusted boundaries/handlers to provide real security benefits. +**Prevention:** Audit all handlers and API boundaries to ensure every untrusted input field is parsed and validated using established validators before proceeding with business logic. + +## 2026-07-23 - Robustness of Merge Reconcile Validators on Falsy Inputs +**Vulnerability:** The reconciliation `validateBranchName` and `validateFilePath` helpers in the `@google/jules-merge` package did not check for falsy/empty values before invoking string operations. This could lead to unhandled runtime type exceptions, crashing the execution context, or allowing unexpected bypasses if downstream functions default empty values in unforeseen ways. +**Learning:** Enforcing non-empty values at the validation boundary ensures absolute system robustness, preventing potential Denial of Service (DoS) and input validation bypasses during conflict resolution execution. +**Prevention:** Always validate all path/identifier parameters to ensure they are non-empty strings before proceeding with sub-string matching or other string manipulations. + +## 2026-07-18 - Input Validation of Repository and Branch Names on SDK/MCP Entrypoints +**Vulnerability:** The Core SDK and MCP entrypoints allowed arbitrary strings to be used for repository references and Git branch names during session creation and resource fetches, presenting opportunities for path traversal, script injection, and reference escape down the call chain. +**Learning:** Validating input format at the highest level of the SDK ensures consistent security boundaries across all integration endpoints (including CLI, direct client usage, and MCP tools) before operations resolve URLs or format API payloads. +**Prevention:** Integrate robust `validateRepository` and `validateBranchName` checks on all entrypoints receiving user-supplied source contexts. + +## 2026-07-17 - Repository Validation Pattern to Prevent Injection and Traversal +**Vulnerability:** The reconciliation handlers accepted untrusted `repo` string inputs (such as `owner/repo`) and parsed them directly (e.g., using `.split('/')`) without verification, opening potential vectors for path traversal, control character injection, or API parsing attacks when interacting with downstream filesystems and Octokit. +**Learning:** Repository path structures must be strictly validated before processing or accessing external APIs. An simple/anchored regex match against standard naming formats prevents any injection or path traversal attempts. +**Prevention:** Always validate repository name parameters against standard patterns (like strictly alphanumeric, dots, hyphens, and underscores) and reject control characters, path traversal segments (`..`), or invalid slash counts. + +## 2026-07-16 - Path Traversal Vulnerability via Session ID Input +**Vulnerability:** The client session initialization and local cache file storage used user-supplied or untrusted `sessionId` inputs directly to resolve cache directories, which allowed directory traversal and file inclusion when malicious IDs containing path traversal characters like `..`, `/`, or `\\` were provided. +**Learning:** Session IDs must be rigorously validated before performing any disk I/O, as they are implicitly used as subdirectory names in local cache structures. By enforcing flat, alphanumeric string constraints (no directory separators or control characters), path traversal is completely eliminated. +**Prevention:** Always validate all path parameters and identifiers like `sessionId` to ensure they are strictly flat (no `/`, `\\`, control chars, `.`, or `..`) before utilizing them in file system path resolution. + +## 2026-07-09 - Path Traversal Vulnerability in ApiClient +**Vulnerability:** The `ApiClient.resolveUrl` method used simple string concatenation to construct URLs, which allowed malicious paths like `../../secret` to escape the intended `baseUrl`. +**Learning:** Using the `URL` constructor with a base URL is not sufficient to prevent path traversal if the path starts with `..`. The resulting URL can still point outside the base path. +**Prevention:** Always validate that the final resolved URL still starts with the expected normalized base URL. + +## 2026-07-10 - Path Traversal and Local File Inclusion in stage-resolution +**Vulnerability:** The `stageResolutionHandler` accepted a `fromFile` path parameter to read local files, but did not validate it. Additionally, the existing `validateFilePath` helper only split the path by `/` and checked for `..`, allowing absolute paths like `/etc/passwd` to bypass the relative path check. +**Learning:** Path bouncers must explicitly reject absolute paths (such as paths starting on Windows or having drive letters on Windows) to prevent traversal via absolute references, even when `..` check is active. Any parameter used in local disk I/O (like `fs.readFileSync`) must be rigorously sanitized. +**Prevention:** Always apply path validation functions to all input file paths and restrict path parameters to relative paths by explicitly throwing on absolute prefixes or drive letters. diff --git a/packages/core/src/activities/client.ts b/packages/core/src/activities/client.ts index c77e6fb1..bb350838 100644 --- a/packages/core/src/activities/client.ts +++ b/packages/core/src/activities/client.ts @@ -14,10 +14,7 @@ * limitations under the License. */ -import { - MediaArtifact, - ChangeSetArtifact, -} from '../artifacts.js'; +import { MediaArtifact, ChangeSetArtifact } from '../artifacts.js'; import { Activity, Artifact } from '../types.js'; import { ActivityStorage } from '../storage/types.js'; import { ActivityClient, ListOptions, SelectOptions } from './types.js'; @@ -78,30 +75,60 @@ export class DefaultActivityClient implements ActivityClient { return activity; } - const hydratedArtifacts = activity.artifacts.map((artifact) => { - // If it's already a class instance, we're done. - if (artifact instanceof MediaArtifact) return artifact; - if (artifact instanceof ChangeSetArtifact) return artifact; + // Optimization check: If all artifacts are already rich instances, + // bypass mapping, array allocation, and activity shallow cloning entirely. + let needsHydration = false; + const len = activity.artifacts.length; + for (let i = 0; i < len; i++) { + const artifact = activity.artifacts[i]; + if ( + !(artifact instanceof MediaArtifact) && + !(artifact instanceof ChangeSetArtifact) + ) { + needsHydration = true; + break; + } + } + + if (!needsHydration) { + return activity; + } + + // Use a fast native index-loop to perform mapping, avoiding map closure allocation overhead + const hydratedArtifacts = new Array(len); + for (let i = 0; i < len; i++) { + const artifact = activity.artifacts[i]; + if ( + artifact instanceof MediaArtifact || + artifact instanceof ChangeSetArtifact + ) { + hydratedArtifacts[i] = artifact; + continue; + } - // It's a plain object from JSON.parse(), so we need to re-hydrate it. - // We check for the 'type' property to know which class to use. switch (artifact.type) { case 'changeSet': // The raw cached format has artifact.changeSet.gitPatch structure. // We need to handle this legacy format gracefully. const rawChangeSet = (artifact as any).changeSet || artifact; - return new ChangeSetArtifact( + hydratedArtifacts[i] = new ChangeSetArtifact( rawChangeSet.source, rawChangeSet.gitPatch, ); + break; case 'media': const rawMedia = (artifact as any).media || artifact; - return new MediaArtifact(rawMedia, this.platform, activity.id); + hydratedArtifacts[i] = new MediaArtifact( + rawMedia, + this.platform, + activity.id, + ); + break; default: // If we don't recognize the type, return it as-is. - return artifact as Artifact; + hydratedArtifacts[i] = artifact as Artifact; } - }); + } return { ...activity, @@ -196,17 +223,23 @@ export class DefaultActivityClient implements ActivityClient { response.activities.map((activity) => this.storage.get(activity.id)), ); + const newActivities: Activity[] = []; for (let i = 0; i < response.activities.length; i++) { - const activity = response.activities[i]; - const existing = existingChecks[i]; - - if (existing) { - continue; + if (!existingChecks[i]) { + newActivities.push(response.activities[i]); } + } - // It's new - append to storage - await this.storage.append(activity); - count++; + if (newActivities.length > 0) { + // Optimized: Batch append newly ingested activities in a single O(1) metadata update/stream write. + if (typeof (this.storage as any).appendMany === 'function') { + await (this.storage as any).appendMany(newActivities); + } else { + for (let i = 0; i < newActivities.length; i++) { + await this.storage.append(newActivities[i]); + } + } + count += newActivities.length; } nextPageToken = response.nextPageToken; @@ -235,15 +268,13 @@ export class DefaultActivityClient implements ActivityClient { const latest = await this.storage.latest(); // We use createTime as the primary cursor because it's standard and comparable. // Fallback to epoch 0 if storage is empty. - let highWaterMark = latest?.createTime - ? new Date(latest.createTime).getTime() - : 0; + let highWaterMark = latest?.createTime ? Date.parse(latest.createTime) : 0; // We also track the specific ID of the latest to handle events with identical timestamps. let lastSeenId = latest?.id; // 2. Start crude polling from the raw network source for await (const activity of this.network.rawStream()) { - const actTime = new Date(activity.createTime).getTime(); + const actTime = Date.parse(activity.createTime); // 3. Deduplication Filter // If this activity is older than our high-water mark, skip it. diff --git a/packages/core/src/api.ts b/packages/core/src/api.ts index 80f8368e..924002f7 100644 --- a/packages/core/src/api.ts +++ b/packages/core/src/api.ts @@ -181,7 +181,22 @@ export class ApiClient { private resolveUrl(path: string): URL { // Direct Mode - return new URL(`${this.baseUrl}/${path}`); + const normalizedBase = this.baseUrl.endsWith("/") + ? this.baseUrl + : `${this.baseUrl}/`; + const sanitizedPath = path.startsWith("/") ? path.slice(1) : path; + const url = new URL(sanitizedPath, normalizedBase); + + if (!url.toString().startsWith(normalizedBase)) { + throw new JulesApiError( + url.toString(), + 400, + "Bad Request", + `Security Error: Invalid path traversal detected in "${path}"`, + ); + } + + return url; } private async fetchWithTimeout(url: string, opts: any): Promise { diff --git a/packages/core/src/artifacts.ts b/packages/core/src/artifacts.ts index b80cd6ed..00c6ba7f 100644 --- a/packages/core/src/artifacts.ts +++ b/packages/core/src/artifacts.ts @@ -31,75 +31,103 @@ import { Platform } from './platform/types.js'; export function parseUnidiff(patch?: string | null): ParsedFile[] { if (!patch) return []; const files: ParsedFile[] = []; - // Split by diff headers (diff --git a/... b/...) - const diffSections = patch.split(/^diff --git /m).filter(Boolean); - - for (const section of diffSections) { - const lines = section.split('\n'); - - // Extract file path from the +++ line (destination file) - // Format: +++ b/path/to/file or +++ /dev/null - let path = ''; - let fromPath = ''; - let toPath = ''; - - for (const line of lines) { - if (line.startsWith('--- ')) { - // --- a/path or --- /dev/null - fromPath = line - .slice(4) - .replace(/^a\//, '') - .replace(/^\/dev\/null$/, ''); - } else if (line.startsWith('+++ ')) { - // +++ b/path or +++ /dev/null - toPath = line - .slice(4) - .replace(/^b\//, '') - .replace(/^\/dev\/null$/, ''); - } - } + // Single-pass optimization: avoid splitting by regex and heavy nested array allocations. + // Instead, split by line once, and track file block state sequentially. + const lines = patch.split('\n'); + const totalLines = lines.length; + + let currentPath = ''; + let fromPath = ''; + let toPath = ''; + let hasFromNull = false; + let hasToNull = false; + let additions = 0; + let deletions = 0; + let inHunk = false; + let hasFile = false; + + const commitFile = () => { + if (!hasFile) return; - // Determine change type and path let changeType: 'created' | 'modified' | 'deleted'; - if (fromPath === '' || lines.some((l) => l.startsWith('--- /dev/null'))) { + if (fromPath === '' || hasFromNull) { changeType = 'created'; - path = toPath; - } else if ( - toPath === '' || - lines.some((l) => l.startsWith('+++ /dev/null')) - ) { + currentPath = toPath; + } else if (toPath === '' || hasToNull) { changeType = 'deleted'; - path = fromPath; + currentPath = fromPath; } else { changeType = 'modified'; - path = toPath; + currentPath = toPath; + } + + if (currentPath) { + files.push({ + path: currentPath, + changeType, + additions, + deletions, + }); } + }; - // Skip if we couldn't determine a path - if (!path) continue; + for (let i = 0; i < totalLines; i++) { + const line = lines[i]; + + if (line.startsWith('diff --git ')) { + commitFile(); + // Reset state for new file block + currentPath = ''; + fromPath = ''; + toPath = ''; + hasFromNull = false; + hasToNull = false; + additions = 0; + deletions = 0; + inHunk = false; + hasFile = true; + continue; + } - // Count additions and deletions (lines starting with + or - in hunks) - let additions = 0; - let deletions = 0; - let inHunk = false; + if (!hasFile) continue; - for (const line of lines) { - if (line.startsWith('@@')) { - inHunk = true; - continue; + if (line.startsWith('--- ')) { + const slice = line.slice(4).replace(/\r$/, ''); + if (slice === '/dev/null') { + hasFromNull = true; + fromPath = ''; + } else { + fromPath = slice.startsWith('a/') ? slice.slice(2) : slice; } - if (inHunk) { - if (line.startsWith('+') && !line.startsWith('+++')) { - additions++; - } else if (line.startsWith('-') && !line.startsWith('---')) { - deletions++; - } + continue; + } + + if (line.startsWith('+++ ')) { + const slice = line.slice(4).replace(/\r$/, ''); + if (slice === '/dev/null') { + hasToNull = true; + toPath = ''; + } else { + toPath = slice.startsWith('b/') ? slice.slice(2) : slice; } + continue; } - files.push({ path, changeType, additions, deletions }); + if (line.startsWith('@@')) { + inHunk = true; + continue; + } + + if (inHunk) { + if (line.startsWith('+') && !line.startsWith('+++')) { + additions++; + } else if (line.startsWith('-') && !line.startsWith('---')) { + deletions++; + } + } } + commitFile(); return files; } @@ -112,78 +140,108 @@ export function parseUnidiffWithContent( ): GeneratedFile[] { if (!patch) return []; const files: GeneratedFile[] = []; - // Split by diff headers (diff --git a/... b/...) - const diffSections = patch.split(/^diff --git /m).filter(Boolean); - - for (const section of diffSections) { - const lines = section.split('\n'); - - // Extract file path from the +++ line (destination file) - let path = ''; - let fromPath = ''; - let toPath = ''; - - for (const line of lines) { - if (line.startsWith('--- ')) { - fromPath = line - .slice(4) - .replace(/^a\//, '') - .replace(/^\/dev\/null$/, ''); - } else if (line.startsWith('+++ ')) { - toPath = line - .slice(4) - .replace(/^b\//, '') - .replace(/^\/dev\/null$/, ''); - } - } + // Single-pass optimization: avoid splitting by regex and heavy nested array allocations. + // Instead, split by line once, and track file block state sequentially. + const lines = patch.split('\n'); + const totalLines = lines.length; + + let currentPath = ''; + let fromPath = ''; + let toPath = ''; + let hasFromNull = false; + let hasToNull = false; + let additions = 0; + let deletions = 0; + let inHunk = false; + let hasFile = false; + let contentLines: string[] = []; + + const commitFile = () => { + if (!hasFile) return; - // Determine change type and path let changeType: 'created' | 'modified' | 'deleted'; - if (fromPath === '' || lines.some((l) => l.startsWith('--- /dev/null'))) { + if (fromPath === '' || hasFromNull) { changeType = 'created'; - path = toPath; - } else if ( - toPath === '' || - lines.some((l) => l.startsWith('+++ /dev/null')) - ) { + currentPath = toPath; + } else if (toPath === '' || hasToNull) { changeType = 'deleted'; - path = fromPath; + currentPath = fromPath; } else { changeType = 'modified'; - path = toPath; + currentPath = toPath; } - // Skip if we couldn't determine a path - if (!path) continue; + if (currentPath) { + const content = changeType === 'deleted' ? '' : contentLines.join('\n'); + files.push({ + path: currentPath, + changeType, + content, + additions, + deletions, + }); + } + }; + + for (let i = 0; i < totalLines; i++) { + const line = lines[i]; + + if (line.startsWith('diff --git ')) { + commitFile(); + // Reset state for new file block + currentPath = ''; + fromPath = ''; + toPath = ''; + hasFromNull = false; + hasToNull = false; + additions = 0; + deletions = 0; + inHunk = false; + hasFile = true; + contentLines = []; + continue; + } - // Count additions and deletions, and collect content - let additions = 0; - let deletions = 0; - let inHunk = false; - const contentLines: string[] = []; + if (!hasFile) continue; - for (const line of lines) { - if (line.startsWith('@@')) { - inHunk = true; - continue; + if (line.startsWith('--- ')) { + const slice = line.slice(4).replace(/\r$/, ''); + if (slice === '/dev/null') { + hasFromNull = true; + fromPath = ''; + } else { + fromPath = slice.startsWith('a/') ? slice.slice(2) : slice; } - if (inHunk) { - if (line.startsWith('+') && !line.startsWith('+++')) { - additions++; - // Remove the leading '+' to get the actual content - contentLines.push(line.slice(1)); - } else if (line.startsWith('-') && !line.startsWith('---')) { - deletions++; - } + continue; + } + + if (line.startsWith('+++ ')) { + const slice = line.slice(4).replace(/\r$/, ''); + if (slice === '/dev/null') { + hasToNull = true; + toPath = ''; + } else { + toPath = slice.startsWith('b/') ? slice.slice(2) : slice; } + continue; } - // For deleted files, content is empty - const content = changeType === 'deleted' ? '' : contentLines.join('\n'); + if (line.startsWith('@@')) { + inHunk = true; + continue; + } - files.push({ path, changeType, content, additions, deletions }); + if (inHunk) { + if (line.startsWith('+') && !line.startsWith('+++')) { + additions++; + contentLines.push(line.slice(1)); + } else if (line.startsWith('-') && !line.startsWith('---')) { + deletions++; + } + } } + commitFile(); return files; } @@ -262,6 +320,7 @@ export class ChangeSetArtifact { public readonly type = 'changeSet' as const; public readonly source: string; public readonly gitPatch: GitPatch; + private _cachedParsed: ParsedChangeSet | null = null; constructor(source: string, gitPatch: GitPatch) { this.source = source; @@ -276,24 +335,55 @@ export class ChangeSetArtifact { * - Determines change type (created/modified/deleted) from /dev/null markers. * - Counts additions (+) and deletions (-) in hunks. * + * **Performance Optimization:** + * - Lazy memoization caches parsed result on `_cachedParsed` to convert repeated + * `parsed()` calls from O(N) diff parsing to O(1) instant returns. + * - Summary categorization uses a single native indexed loop instead of 3 sequential `.filter()` + * scans, avoiding array allocations and reducing loop passes from 3 to 1. + * * @returns Parsed diff with file paths, change types, and line counts. */ parsed(): ParsedChangeSet { + if (this._cachedParsed) { + return this._cachedParsed; + } + if (!this.gitPatch?.unidiffPatch) { - return { + this._cachedParsed = { files: [], summary: { totalFiles: 0, created: 0, modified: 0, deleted: 0 }, }; + return this._cachedParsed; } + const files = parseUnidiff(this.gitPatch.unidiffPatch); + const totalFiles = files.length; + + let created = 0; + let modified = 0; + let deleted = 0; + + for (let i = 0; i < totalFiles; i++) { + const type = files[i].changeType; + if (type === 'created') { + created++; + } else if (type === 'modified') { + modified++; + } else if (type === 'deleted') { + deleted++; + } + } - const summary = { - totalFiles: files.length, - created: files.filter((f) => f.changeType === 'created').length, - modified: files.filter((f) => f.changeType === 'modified').length, - deleted: files.filter((f) => f.changeType === 'deleted').length, + this._cachedParsed = { + files, + summary: { + totalFiles, + created, + modified, + deleted, + }, }; - return { files, summary }; + return this._cachedParsed; } } diff --git a/packages/core/src/caching.ts b/packages/core/src/caching.ts index bb8158b2..73cc5823 100644 --- a/packages/core/src/caching.ts +++ b/packages/core/src/caching.ts @@ -34,15 +34,20 @@ export function determineCacheTier( cached: CachedSession, now: number = Date.now(), ): CacheTier { - const createdAt = new Date(cached.resource.createTime).getTime(); + const createdAt = Date.parse(cached.resource.createTime); const age = now - createdAt; - const isTerminal = ['failed', 'completed'].includes(cached.resource.state); // TIER 3: FROZEN (Older than 1 month) if (age > ONE_MONTH_MS) { return 'frozen'; } + // Performance Optimization: Direct state comparison avoids allocating a new array + // (`['failed', 'completed']`) on every evaluation, and evaluating it after the frozen check + // skips terminal state parsing for old sessions. + const state = cached.resource.state; + const isTerminal = state === 'failed' || state === 'completed'; + // TIER 2: WARM (Terminal state + synced recently) const timeSinceSync = now - cached._lastSyncedAt; if (isTerminal && timeSinceSync < ONE_DAY_MS) { diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index 81171ca9..ea3e00dc 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -16,6 +16,11 @@ // src/client.ts import { ApiClient } from './api.js'; +import { + validateSessionId, + validateRepository, + validateBranchName, +} from './utils/validators.js'; import { createSourceManager } from './sources.js'; import { join } from 'node:path'; import { getRootDir } from './storage/root.js'; @@ -201,8 +206,8 @@ export class JulesClientImpl implements JulesClient { } let skipUntilPast = !!resumeFromId; - const highWaterMark = incremental - ? await this._getHighWaterMark() + const highWaterMarkMs = incremental + ? await this._getHighWaterMarkMs() : null; const cursor = this.sessions({ @@ -227,7 +232,10 @@ export class JulesClientImpl implements JulesClient { continue; } - if (highWaterMark && new Date(session.createTime) <= highWaterMark) { + if ( + highWaterMarkMs !== null && + Date.parse(session.createTime) <= highWaterMarkMs + ) { // We've reached sessions we already have cached. // For activities depth: include this session for hydration (to get new activities) // but stop iterating after - we don't need to process older sessions. @@ -356,14 +364,18 @@ export class JulesClientImpl implements JulesClient { } } - private async _getHighWaterMark(): Promise { - let newest: Date | null = null; + private async _getHighWaterMarkMs(): Promise { + let newestMs = 0; // scanIndex is the high-speed index scanner implemented in Phase 1 for await (const entry of this.storage.scanIndex()) { - const date = new Date(entry.createTime); - if (!newest || date > newest) newest = date; + if (entry.createTime) { + const ms = Date.parse(entry.createTime); + if (ms > newestMs) { + newestMs = ms; + } + } } - return newest; + return newestMs > 0 ? newestMs : null; } /** @@ -495,6 +507,9 @@ export class JulesClientImpl implements JulesClient { }; } + validateRepository(config.source.github); + validateBranchName(config.source.baseBranch); + const source = await this.sources.get({ github: config.source.github }); if (!source) { throw new SourceNotFoundError(config.source.github); @@ -627,6 +642,7 @@ export class JulesClientImpl implements JulesClient { configOrId: SessionConfig | string, ): Promise | SessionClient { if (typeof configOrId === 'string') { + validateSessionId(configOrId); const storage = this.storageFactory.activity(configOrId); return new SessionClientImpl( configOrId, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 923e1ae3..1fde09aa 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -131,3 +131,13 @@ export type { StorageFactory } from './types.js'; // Artifact classes with helper methods export { ChangeSetArtifact, parseUnidiff } from './artifacts.js'; + +// Validation helpers +export { + validateSessionId, + validateRepository, + validateBranchName, + validateFilePath, + validateActivityId, + validatePageToken, +} from './utils/validators.js'; diff --git a/packages/core/src/query/computed.ts b/packages/core/src/query/computed.ts index ba882c93..d5114673 100644 --- a/packages/core/src/query/computed.ts +++ b/packages/core/src/query/computed.ts @@ -76,8 +76,8 @@ export function computeDurationMs(session: { }): number { if (!session.createTime || !session.updateTime) return 0; - const created = new Date(session.createTime).getTime(); - const updated = new Date(session.updateTime).getTime(); + const created = Date.parse(session.createTime); + const updated = Date.parse(session.updateTime); if (isNaN(created) || isNaN(updated)) return 0; @@ -95,11 +95,6 @@ export function injectActivityComputedFields( activity: Activity, selectFields?: string[], ): Activity & { artifactCount?: number; summary?: string } { - const result = { ...activity } as Activity & { - artifactCount?: number; - summary?: string; - }; - // Determine which computed fields to include const includeAll = !selectFields || selectFields.length === 0 || selectFields.includes('*'); @@ -108,11 +103,30 @@ export function injectActivityComputedFields( includeAll || selectFields?.includes('artifactCount'); const needsSummary = includeAll || selectFields?.includes('summary'); - if (needsArtifactCount) { + if (!needsArtifactCount && !needsSummary) { + return activity; + } + + // Check if they are already present to avoid redundant computation and cloning + const hasArtifactCount = 'artifactCount' in activity; + const hasSummary = 'summary' in activity; + if ( + (!needsArtifactCount || hasArtifactCount) && + (!needsSummary || hasSummary) + ) { + return activity; + } + + const result = { ...activity } as Activity & { + artifactCount?: number; + summary?: string; + }; + + if (needsArtifactCount && !hasArtifactCount) { result.artifactCount = computeArtifactCount(activity); } - if (needsSummary) { + if (needsSummary && !hasSummary) { result.summary = computeSummary(activity); } @@ -129,17 +143,22 @@ export function injectActivityComputedFields( export function injectSessionComputedFields< T extends { createTime?: string; updateTime?: string }, >(session: T, selectFields?: string[]): T & { durationMs?: number } { - const result = { ...session } as T & { durationMs?: number }; - const includeAll = !selectFields || selectFields.length === 0 || selectFields.includes('*'); const needsDurationMs = includeAll || selectFields?.includes('durationMs'); - if (needsDurationMs) { - result.durationMs = computeDurationMs(session); + if (!needsDurationMs) { + return session as T & { durationMs?: number }; + } + + if ('durationMs' in session) { + return session as T & { durationMs?: number }; } + const result = { ...session } as T & { durationMs?: number }; + result.durationMs = computeDurationMs(session); + return result; } diff --git a/packages/core/src/query/projection.ts b/packages/core/src/query/projection.ts index f6a8f38b..de9a21af 100644 --- a/packages/core/src/query/projection.ts +++ b/packages/core/src/query/projection.ts @@ -33,6 +33,8 @@ export interface SelectExpression { wildcard: boolean; } +const selectExpressionCache = new Map(); + /** * Parse a select expression string into structured form * @@ -43,18 +45,27 @@ export interface SelectExpression { * - "*" → { path: [], exclude: false, wildcard: true } */ export function parseSelectExpression(expr: string): SelectExpression { - if (expr === '*') { - return { path: [], exclude: false, wildcard: true }; + const cached = selectExpressionCache.get(expr); + if (cached) { + return cached; } - const exclude = expr.startsWith('-'); - const pathStr = exclude ? expr.slice(1) : expr; + let result: SelectExpression; + if (expr === '*') { + result = { path: [], exclude: false, wildcard: true }; + } else { + const exclude = expr.startsWith('-'); + const pathStr = exclude ? expr.slice(1) : expr; + + // Remove optional array markers like "[]" - they're implicit + const cleanPath = pathStr.replace(/\[\]/g, ''); + const path = cleanPath.split('.').filter((p) => p.length > 0); - // Remove optional array markers like "[]" - they're implicit - const cleanPath = pathStr.replace(/\[\]/g, ''); - const path = cleanPath.split('.').filter((p) => p.length > 0); + result = { path, exclude, wildcard: false }; + } - return { path, exclude, wildcard: false }; + selectExpressionCache.set(expr, result); + return result; } /** @@ -66,23 +77,29 @@ export function parseSelectExpression(expr: string): SelectExpression { * - getPath({a: {b: 1}}, ["a", "b"]) → 1 * - getPath({items: [{x: 1}, {x: 2}]}, ["items", "x"]) → [1, 2] */ -export function getPath(obj: unknown, path: string[]): unknown { - if (path.length === 0) return obj; +export function getPath(obj: unknown, path: string[], index = 0): unknown { + if (index >= path.length) return obj; if (obj === null || obj === undefined) return undefined; - const [head, ...tail] = path; + const head = path[index]; if (Array.isArray(obj)) { - // Map over array elements and collect values - const results = obj - .map((item) => getPath(item, path)) - .filter((v) => v !== undefined); + // Map over array elements and collect values using a standard indexed loop + // to avoid closure creation and array allocations from map/filter. + const results: unknown[] = []; + const len = obj.length; + for (let i = 0; i < len; i++) { + const v = getPath(obj[i], path, index); + if (v !== undefined) { + results.push(v); + } + } return results.length > 0 ? results : undefined; } if (typeof obj === 'object') { const value = (obj as Record)[head]; - return getPath(value, tail); + return getPath(value, path, index + 1); } return undefined; @@ -97,12 +114,13 @@ export function setPath( obj: Record, path: string[], value: unknown, + index = 0, ): void { - if (path.length === 0 || value === undefined) return; + if (index >= path.length || value === undefined) return; - const [head, ...tail] = path; + const head = path[index]; - if (tail.length === 0) { + if (index === path.length - 1) { obj[head] = value; return; } @@ -113,7 +131,7 @@ export function setPath( const next = obj[head]; if (typeof next === 'object' && next !== null && !Array.isArray(next)) { - setPath(next as Record, tail, value); + setPath(next as Record, path, value, index + 1); } } @@ -122,26 +140,29 @@ export function setPath( * * For paths ending in array elements, removes the field from each element. */ -export function deletePath(obj: unknown, path: string[]): void { - if (path.length === 0 || obj === null || obj === undefined) return; +export function deletePath(obj: unknown, path: string[], index = 0): void { + if (index >= path.length || obj === null || obj === undefined) return; if (Array.isArray(obj)) { - obj.forEach((item) => deletePath(item, path)); + const len = obj.length; + for (let i = 0; i < len; i++) { + deletePath(obj[i], path, index); + } return; } if (typeof obj !== 'object') return; const record = obj as Record; - const [head, ...tail] = path; + const head = path[index]; - if (tail.length === 0) { + if (index === path.length - 1) { delete record[head]; return; } if (head in record) { - deletePath(record[head], tail); + deletePath(record[head], path, index + 1); } } @@ -150,11 +171,25 @@ export function deletePath(obj: unknown, path: string[]): void { */ export function deepClone(obj: T): T { if (obj === null || typeof obj !== 'object') return obj; - if (Array.isArray(obj)) return obj.map((item) => deepClone(item)) as T; + if (Array.isArray(obj)) { + const len = obj.length; + const cloned = new Array(len); + for (let i = 0; i < len; i++) { + const val = obj[i]; + cloned[i] = + val === null || typeof val !== 'object' ? val : deepClone(val); + } + return cloned as T; + } const cloned: Record = {}; - for (const key of Object.keys(obj)) { - cloned[key] = deepClone((obj as Record)[key]); + const keys = Object.keys(obj); + const len = keys.length; + for (let i = 0; i < len; i++) { + const key = keys[i]; + const val = (obj as Record)[key]; + cloned[key] = + val === null || typeof val !== 'object' ? val : deepClone(val); } return cloned as T; } @@ -170,12 +205,38 @@ function projectArray( subPaths: string[][], excludePaths: string[][], ): unknown[] { - return arr.map((item) => { - if (item === null || typeof item !== 'object') return item; + const len = arr.length; + const projectedArr = new Array(len); + + // Performance Optimization: If we have a single subpath which is empty (representing the entire element) + // and no exclusions, we can bypass the entire property loop, nested path lookups, and allocation of intermediate empty objects. + // Instead, we directly deep-clone the element, achieving a massive speedup on full element copies within arrays. + const isWholeClone = + excludePaths.length === 0 && + subPaths.length === 1 && + subPaths[0].length === 0; + + if (isWholeClone) { + for (let i = 0; i < len; i++) { + const item = arr[i]; + projectedArr[i] = + item === null || typeof item !== 'object' ? item : deepClone(item); + } + return projectedArr; + } + + for (let i = 0; i < len; i++) { + const item = arr[i]; + if (item === null || typeof item !== 'object') { + projectedArr[i] = item; + continue; + } const projected: Record = {}; - for (const subPath of subPaths) { + const subPathsLen = subPaths.length; + for (let j = 0; j < subPathsLen; j++) { + const subPath = subPaths[j]; const value = getPath(item, subPath); if (value !== undefined) { if (subPath.length === 0) { @@ -188,12 +249,15 @@ function projectArray( } // Apply exclusions - for (const excludePath of excludePaths) { - deletePath(projected, excludePath); + const excludePathsLen = excludePaths.length; + for (let j = 0; j < excludePathsLen; j++) { + deletePath(projected, excludePaths[j]); } - return projected; - }); + projectedArr[i] = projected; + } + + return projectedArr; } /** @@ -203,6 +267,93 @@ function projectArray( * @param selects Array of select expression strings * @returns Projected document with only selected fields */ +/** + * Cached Projection Plan to avoid re-parsing select expressions, + * re-filtering inclusions/exclusions, and rebuilding grouping maps + * for every single document in a multi-document query. + */ +export interface ProjectionPlan { + hasWildcard: boolean; + byTopLevel: Map; + exclusions: SelectExpression[]; + isSimpleTopLevel?: boolean; + exclusionSubPathsByTopLevel: Map; + nestedSelectsByTopLevel: Map; +} + +const projectionPlanCache = new Map(); + +/** + * Get or compile a projection plan for a given list of select expressions. + * This yields an O(1) cache lookup after the first projected document. + */ +export function getProjectionPlan(selects: string[]): ProjectionPlan { + const cacheKey = selects.join(','); + let plan = projectionPlanCache.get(cacheKey); + if (!plan) { + const parsed = selects.map(parseSelectExpression); + const hasWildcard = parsed.some((p) => p.wildcard && !p.exclude); + const inclusions = parsed.filter((p) => !p.exclude && !p.wildcard); + const exclusions = parsed.filter((p) => p.exclude); + + // Fast-path detection: Check if the projection consists entirely of + // simple top-level fields with no wildcards and no exclusions. + const isSimpleTopLevel = + !hasWildcard && + exclusions.length === 0 && + inclusions.every((incl) => incl.path.length === 1); + + const byTopLevel = new Map(); + for (const incl of inclusions) { + if (incl.path.length === 0) continue; + const top = incl.path[0]; + let subPaths = byTopLevel.get(top); + if (!subPaths) { + subPaths = []; + byTopLevel.set(top, subPaths); + } + subPaths.push(incl.path.slice(1)); + } + + // Pre-compile and cache exclusion sub-paths and nested selects per top-level field + // to bypass costly array mappings and string manipulations during runtime scans. + const exclusionSubPathsByTopLevel = new Map(); + const exclusionsLen = exclusions.length; + for (let i = 0; i < exclusionsLen; i++) { + const excl = exclusions[i]; + if (excl.path.length === 0) continue; + const top = excl.path[0]; + let subPaths = exclusionSubPathsByTopLevel.get(top); + if (!subPaths) { + subPaths = []; + exclusionSubPathsByTopLevel.set(top, subPaths); + } + subPaths.push(excl.path.slice(1)); + } + + const nestedSelectsByTopLevel = new Map(); + for (const [topField, subPaths] of byTopLevel) { + const nestedLen = subPaths.length; + const nested = new Array(nestedLen); + for (let i = 0; i < nestedLen; i++) { + nested[i] = subPaths[i].join('.'); + } + nestedSelectsByTopLevel.set(topField, nested); + } + + plan = { + hasWildcard, + byTopLevel, + exclusions, + isSimpleTopLevel, + exclusionSubPathsByTopLevel, + nestedSelectsByTopLevel, + }; + projectionPlanCache.set(cacheKey, plan); + } + return plan; +} + export function projectDocument( doc: Record, selects: string[], @@ -212,10 +363,33 @@ export function projectDocument( return doc; } - const parsed = selects.map(parseSelectExpression); - const hasWildcard = parsed.some((p) => p.wildcard && !p.exclude); - const inclusions = parsed.filter((p) => !p.exclude && !p.wildcard); - const exclusions = parsed.filter((p) => p.exclude); + // Use compiled projection plan to bypass redundant parsing/grouping + const plan = getProjectionPlan(selects); + const { + hasWildcard, + byTopLevel, + exclusions, + isSimpleTopLevel, + exclusionSubPathsByTopLevel, + nestedSelectsByTopLevel, + } = plan; + + // Fast-path: Optimized top-level field selection. + // Avoids sub-path checks, array and type detection, and recursive projection. + // Directly clones objects/arrays and copies primitives by reference. + if (isSimpleTopLevel) { + const result: Record = {}; + for (const topField of byTopLevel.keys()) { + const value = doc[topField]; + if (value !== undefined) { + result[topField] = + value === null || typeof value !== 'object' + ? value + : deepClone(value); + } + } + return result; + } let result: Record; @@ -226,27 +400,15 @@ export function projectDocument( // Start with empty, add inclusions result = {}; - // Group inclusions by top-level field for efficient array handling - const byTopLevel = new Map(); - - for (const incl of inclusions) { - if (incl.path.length === 0) continue; - const top = incl.path[0]; - if (!byTopLevel.has(top)) { - byTopLevel.set(top, []); - } - byTopLevel.get(top)!.push(incl.path.slice(1)); - } - for (const [topField, subPaths] of byTopLevel) { const value = doc[topField]; if (value === undefined) continue; if (Array.isArray(value)) { // Handle array projection - const exclusionSubPaths = exclusions - .filter((e) => e.path[0] === topField) - .map((e) => e.path.slice(1)); + // Retrieve pre-compiled exclusion sub-paths to avoid allocating/mapping arrays inside the hot loop + const exclusionSubPaths = + exclusionSubPathsByTopLevel.get(topField) || []; if (subPaths.some((p) => p.length === 0)) { // Include full array (possibly with exclusions) @@ -262,7 +424,8 @@ export function projectDocument( result[topField] = deepClone(value); } else { // Recursively project nested fields - const nestedSelects = subPaths.map((p) => p.join('.')); + // Retrieve pre-compiled nested selects to avoid map/join overhead in the hot loop + const nestedSelects = nestedSelectsByTopLevel.get(topField) || []; result[topField] = projectDocument( value as Record, nestedSelects, @@ -276,8 +439,9 @@ export function projectDocument( } // Apply exclusions - for (const excl of exclusions) { - deletePath(result, excl.path); + const exclusionsLen = exclusions.length; + for (let i = 0; i < exclusionsLen; i++) { + deletePath(result, exclusions[i].path); } return result; diff --git a/packages/core/src/query/select.ts b/packages/core/src/query/select.ts index 64276d02..7fa4c553 100644 --- a/packages/core/src/query/select.ts +++ b/packages/core/src/query/select.ts @@ -32,29 +32,108 @@ import { DEFAULT_ACTIVITY_PROJECTION, DEFAULT_SESSION_PROJECTION, } from './computed.js'; +import { validateQuery } from './validate.js'; + +interface CompiledFilterOp { + hasOperators: boolean; + exists?: boolean; + eq?: any; + neq?: any; + containsLower?: string; + gt?: any; + lt?: any; + gte?: any; + lte?: any; + inSet?: Set; + directValue?: any; +} + +interface CompiledFieldFilter { + key: string; + isDot: boolean; + pathParts: string[]; + compiledOp: CompiledFilterOp; +} /** - * Matches a value against a FilterOp. + * Compiles a FilterOp into a highly optimized structured representation + * to avoid repeated object/array/string operations. */ -function match(actual: V, filter?: FilterOp): boolean { - if (filter === undefined) return true; +function compileFilterOp(filter: any): CompiledFilterOp { + if (filter === undefined) { + return { hasOperators: false }; + } if (typeof filter !== 'object' || filter === null || Array.isArray(filter)) { - return actual === filter; + return { hasOperators: false, directValue: filter }; } const op = filter as { - eq?: V; - neq?: V; + eq?: any; + neq?: any; contains?: string; - gt?: V; - lt?: V; - gte?: V; - lte?: V; - in?: V[]; + gt?: any; + lt?: any; + gte?: any; + lte?: any; + in?: any[]; exists?: boolean; }; - // Handle exists operator + return { + hasOperators: true, + exists: op.exists, + eq: op.eq, + neq: op.neq, + containsLower: + typeof op.contains === 'string' ? op.contains.toLowerCase() : undefined, + gt: op.gt, + lt: op.lt, + gte: op.gte, + lte: op.lte, + inSet: Array.isArray(op.in) ? new Set(op.in) : undefined, + }; +} + +/** + * Compiles a full where-clause record into an array of structured field filters, + * optionally filtering only dot notation keys or excluding a specific key. + */ +function compileWhere( + where?: Record>, + onlyDot = false, + excludeKey?: string, +): CompiledFieldFilter[] { + if (!where) return []; + const compiled: CompiledFieldFilter[] = []; + for (const key in where) { + if (Object.prototype.hasOwnProperty.call(where, key)) { + if (excludeKey && key === excludeKey) continue; + const isDot = key.includes('.'); + if (onlyDot && !isDot) continue; + const filter = where[key]; + const pathParts = isDot ? key.split('.') : [key]; + compiled.push({ + key, + isDot, + pathParts, + compiledOp: compileFilterOp(filter), + }); + } + } + return compiled; +} + +/** + * Matches an actual value against a pre-compiled FilterOp. + */ +function matchCompiled(actual: any, op: CompiledFilterOp): boolean { + if (!op.hasOperators) { + if (op.directValue !== undefined) { + return actual === op.directValue; + } + return true; + } + if (op.exists !== undefined) { const valueExists = actual !== undefined && actual !== null; return op.exists ? valueExists : !valueExists; @@ -63,68 +142,70 @@ function match(actual: V, filter?: FilterOp): boolean { if (op.eq !== undefined && actual !== op.eq) return false; if (op.neq !== undefined && actual === op.neq) return false; if ( - op.contains !== undefined && + op.containsLower !== undefined && typeof actual === 'string' && - !actual.toLowerCase().includes(op.contains.toLowerCase()) + !actual.toLowerCase().includes(op.containsLower) ) return false; if (op.gt !== undefined && op.gt !== null && actual <= op.gt) return false; if (op.gte !== undefined && op.gte !== null && actual < op.gte) return false; if (op.lt !== undefined && op.lt !== null && actual >= op.lt) return false; if (op.lte !== undefined && op.lte !== null && actual > op.lte) return false; - if (op.in !== undefined && !op.in.includes(actual)) return false; + if (op.inSet !== undefined && !op.inSet.has(actual)) return false; return true; } /** - * Check if a where key uses dot notation (nested path) - */ -function isDotPath(key: string): boolean { - return key.includes('.'); -} - -/** - * Match a document against a filter using dot notation paths - * Uses existential quantification for array paths + * Matches a document against an array of pre-compiled field filters. + * Replaces Object.entries, recursion, path splits, and .some() closures + * with highly performant, non-allocating native loops. */ -function matchPath( +function matchWhereCompiled( doc: unknown, - path: string, - filter: FilterOp, + compiledFilters: CompiledFieldFilter[], ): boolean { - const pathParts = path.split('.'); - const value = getPath(doc, pathParts); - - // For arrays, use existential matching (ANY element matches) - if (Array.isArray(value)) { - return value.some((v) => match(v, filter)); + const len = compiledFilters.length; + for (let i = 0; i < len; i++) { + const f = compiledFilters[i]; + if (f.isDot) { + const value = getPath(doc, f.pathParts); + if (Array.isArray(value)) { + let anyMatches = false; + const valLen = value.length; + for (let j = 0; j < valLen; j++) { + if (matchCompiled(value[j], f.compiledOp)) { + anyMatches = true; + break; + } + } + if (!anyMatches) return false; + } else { + if (!matchCompiled(value, f.compiledOp)) return false; + } + } else { + const value = (doc as Record)[f.key]; + if (!matchCompiled(value, f.compiledOp)) return false; + } } + return true; +} - return match(value, filter); +/** + * Matches a value against a FilterOp (legacy fallback). + */ +function match(actual: V, filter?: FilterOp): boolean { + return matchCompiled(actual, compileFilterOp(filter)); } /** - * Match a document against a full where clause with dot notation support + * Match a document against a full where clause (legacy fallback). */ function matchWhere( doc: unknown, where?: Record>, ): boolean { - if (!where) return true; - - for (const [key, filter] of Object.entries(where)) { - if (isDotPath(key)) { - // Use path-based matching - if (!matchPath(doc, key, filter)) return false; - } else { - // Use direct field matching - const value = (doc as Record)[key]; - if (!match(value, filter)) return false; - } - } - - return true; + return matchWhereCompiled(doc, compileWhere(where)); } /** @@ -164,23 +245,36 @@ function applyProjection( ): Record { const docRecord = doc as Record; - // Inject computed fields first + // Performance Optimization: If no custom select fields are specified, we default to the standard projection list. + // Passing the resolved projection list to the computed fields injector allows bypassing expensive object cloning + // and CPU date-parsing operations for computed fields (like durationMs) that are not part of the default projection. + const selectFields = + select ?? + (domain === 'activities' + ? DEFAULT_ACTIVITY_PROJECTION + : DEFAULT_SESSION_PROJECTION); + + // Inject computed fields first using the target projection list const withComputed = domain === 'activities' - ? injectActivityComputedFields(doc as Activity, select) - : injectSessionComputedFields(docRecord, select); + ? injectActivityComputedFields(doc as Activity, selectFields) + : injectSessionComputedFields(docRecord, selectFields); // If no select specified, use default projection if (!select) { - const defaults = - domain === 'activities' - ? DEFAULT_ACTIVITY_PROJECTION - : DEFAULT_SESSION_PROJECTION; - return projectDocument(withComputed as Record, defaults); + return projectDocument( + withComputed as Record, + selectFields, + ); } // If empty array or contains only '*', return all with computed - if (select.length === 0) { + if (select.length === 0 || (select.length === 1 && select[0] === '*')) { + // If withComputed is identical to doc (meaning no new computed fields were injected), + // shallow copy to avoid mutating the cached object while bypassing deep projection overhead. + if (withComputed === docRecord) { + return { ...docRecord }; + } return withComputed as Record; } @@ -196,6 +290,14 @@ export async function select( client: JulesClient, query: JulesQuery, ): Promise[]> { + const validationResult = validateQuery(query); + if (!validationResult.valid) { + const messages = validationResult.errors + .map((e) => `[${e.code}] ${e.path}: ${e.message}`) + .join('; '); + throw new Error(`INVALID_QUERY: ${messages}`); + } + const storage = client.storage; const results: Record[] = []; const limit = query.limit ?? Infinity; @@ -204,11 +306,7 @@ export async function select( const where = query.where as WhereClause<'sessions'> | undefined; const whereRecord = where as Record> | undefined; - const dotFilters = whereRecord - ? Object.entries(whereRecord).filter(([k]) => isDotPath(k)) - : []; - const dotWhere = - dotFilters.length > 0 ? Object.fromEntries(dotFilters) : undefined; + const compiledDotWhere = compileWhere(whereRecord, true); let chunk: any[] = []; const CHUNK_SIZE = 50; @@ -217,20 +315,25 @@ export async function select( if (chunk.length === 0) return; // PASS 2: Hydration (Heavy Data) - Parallelized + // Concurrency boosted from 10 to 25 to maximize throughput for disk/network reads const hydrated = await pMap( chunk, async (entry) => { const cached = await storage.get(entry.id); return { entry, cached }; }, - { concurrency: 10 }, + { concurrency: 25 }, ); for (const { cached } of hydrated) { if (results.length >= limit) break; if (!cached) continue; - if (dotWhere && !matchWhere(cached.resource, dotWhere)) continue; + if ( + compiledDotWhere.length > 0 && + !matchWhereCompiled(cached.resource, compiledDotWhere) + ) + continue; const item = applyProjection( cached.resource, @@ -238,14 +341,19 @@ export async function select( 'sessions', ); - // Preserve sorting metadata from original document + // Preserve sorting metadata from original document. + // Pre-parse the Date string to an O(1) number to avoid costly allocations during sorting. const resourceRecord = cached.resource as unknown as Record< string, unknown >; + const createTimeStr = (resourceRecord.createTime ?? + item.createTime ?? + '') as string; item._sortKey = { createTime: resourceRecord.createTime, - id: resourceRecord.id, + time: createTimeStr ? Date.parse(createTimeStr) : 0, + id: resourceRecord.id ?? item.id, }; results.push(item); @@ -253,29 +361,50 @@ export async function select( chunk = []; }; + // Pre-calculate lower-case search query outside of the loop to avoid redundant conversions + const searchLower = + typeof where?.search === 'string' + ? (where.search as string).toLowerCase() + : undefined; + + const compiledIdFilter = where?.id ? compileFilterOp(where.id) : undefined; + const compiledStateFilter = where?.state + ? compileFilterOp(where.state) + : undefined; + const compiledTitleFilter = where?.title + ? compileFilterOp(where.title) + : undefined; + // PASS 1: Index Scan (Metadata Only) for await (const entry of storage.scanIndex()) { if (results.length >= limit) break; // Filter by ID - if (where?.id && !match(entry.id, where.id)) continue; + if (compiledIdFilter && !matchCompiled(entry.id, compiledIdFilter)) + continue; // Filter by State - if (where?.state && !match(entry.state, where.state)) continue; + if ( + compiledStateFilter && + !matchCompiled(entry.state, compiledStateFilter) + ) + continue; // Filter by Title (Fuzzy Search or specific title) - if (where?.title && !match(entry.title, where.title)) continue; - // Global Search if ( - where?.search && - !entry.title.toLowerCase().includes(where.search.toLowerCase()) + compiledTitleFilter && + !matchCompiled(entry.title, compiledTitleFilter) ) continue; + // Global Search + if (searchLower && !entry.title.toLowerCase().includes(searchLower)) + continue; chunk.push(entry); // Process chunk if it reaches CHUNK_SIZE or if we have enough items for the limit without dot filters if ( chunk.length >= CHUNK_SIZE || - (!dotWhere && chunk.length >= limit - results.length) + (compiledDotWhere.length === 0 && + chunk.length >= limit - results.length) ) { await processChunk(); } @@ -358,26 +487,59 @@ export async function select( sessionEntries.push(sessionEntry); } + // Optimization: Map query filters (such as type, limits, cursors) down to the storage selection. + // This avoids fetching, parsing, and hydrating every activity in the session. + const selectOptions = toActivitySelectOptions( + query.where as WhereClause<'activities'>, + ); + + // If sorting order is ascending, we can safely apply startAfter and limit to storage scan. + if (query.order === 'asc') { + if (query.startAfter) { + selectOptions.after = query.startAfter; + } + if (query.limit !== undefined) { + selectOptions.limit = query.limit; + } + } + + // Optimization: Pre-compile filters outside of the loop to avoid redundant operations and GC overhead. + const compiledActivityWhere = compileWhere(where, false, 'sessionId'); + const compiledActIdFilter = where?.id + ? compileFilterOp(where.id) + : undefined; + const compiledActTypeFilter = where?.type + ? compileFilterOp(where.type) + : undefined; + const sessionResults = await pMap( sessionEntries, async (sessionEntry) => { const sessionClient = await client.session(sessionEntry.id); - const localActivities = await sessionClient.activities.select({}); + const localActivities = + await sessionClient.activities.select(selectOptions); const filtered: Record[] = []; for (const act of localActivities) { // Apply standard filters - if (where?.id && !match(act.id, where.id)) continue; - if (where?.type && !match(act.type, where.type)) continue; + if ( + compiledActIdFilter && + !matchCompiled(act.id, compiledActIdFilter) + ) + continue; + if ( + compiledActTypeFilter && + !matchCompiled(act.type, compiledActTypeFilter) + ) + continue; // Apply dot-notation filters with existential matching // Exclude sessionId from activity-level matching since it's handled by session routing - const activityWhere = where - ? Object.fromEntries( - Object.entries(where).filter(([k]) => k !== 'sessionId'), - ) - : undefined; - if (!matchWhere(act, activityWhere)) continue; + if ( + compiledActivityWhere.length > 0 && + !matchWhereCompiled(act, compiledActivityWhere) + ) + continue; const item = applyProjection( act, @@ -385,11 +547,16 @@ export async function select( 'activities', ); - // Preserve sorting metadata from original document + // Preserve sorting metadata from original document. + // Pre-parse the Date string to an O(1) number to avoid costly allocations during sorting. const actRecord = act as unknown as Record; + const createTimeStr = (actRecord.createTime ?? + item.createTime ?? + '') as string; item._sortKey = { createTime: actRecord.createTime, - id: actRecord.id, + time: createTimeStr ? Date.parse(createTimeStr) : 0, + id: actRecord.id ?? item.id, }; // PASS 2: Reverse Join (Include Session Metadata) @@ -424,30 +591,37 @@ export async function select( } } - // Sorting - use _sortKey if available, fallback to document fields + // Sorting - use precomputed time/id inside _sortKey if available, fallback to document fields const order = query.order ?? 'desc'; results.sort((a, b) => { const sortKeyA = a._sortKey as - | { createTime: string; id: string } + | { createTime?: string; time: number; id: string } | undefined; const sortKeyB = b._sortKey as - | { createTime: string; id: string } + | { createTime?: string; time: number; id: string } | undefined; - const timeA = new Date( - (sortKeyA?.createTime ?? a.createTime) as string, - ).getTime(); - const timeB = new Date( - (sortKeyB?.createTime ?? b.createTime) as string, - ).getTime(); + + // In case _sortKey is missing (fallback), parse Date on the fly + const timeA = sortKeyA + ? sortKeyA.time + : a.createTime + ? Date.parse(a.createTime as string) + : 0; + const timeB = sortKeyB + ? sortKeyB.time + : b.createTime + ? Date.parse(b.createTime as string) + : 0; + const idA = (sortKeyA?.id ?? a.id) as string; const idB = (sortKeyB?.id ?? b.id) as string; if (timeA !== timeB) { return order === 'desc' ? timeB - timeA : timeA - timeB; } if (order === 'desc') { - return idB.localeCompare(idA); + return idB < idA ? -1 : idB > idA ? 1 : 0; } - return idA.localeCompare(idB); + return idA < idB ? -1 : idA > idB ? 1 : 0; }); let finalResults = results; diff --git a/packages/core/src/query/validate.ts b/packages/core/src/query/validate.ts index f83fa201..db970f3f 100644 --- a/packages/core/src/query/validate.ts +++ b/packages/core/src/query/validate.ts @@ -91,6 +91,13 @@ const VALID_ORDERS = new Set(['asc', 'desc']); // Field Path Resolution // ============================================ +// Performance Optimization: Cache resolved field paths to completely bypass +// dot-notation path splitting and nested schema list traversal on subsequent validations. +const fieldPathResolutionCache = new Map< + string, + { field: FieldMeta | null; exists: boolean; computedField: boolean } +>(); + /** * Resolve a dot-notation path to field metadata */ @@ -98,6 +105,12 @@ function resolveFieldPath( path: string, domain: 'sessions' | 'activities', ): { field: FieldMeta | null; exists: boolean; computedField: boolean } { + const cacheKey = `${domain}:${path}`; + const cached = fieldPathResolutionCache.get(cacheKey); + if (cached) { + return cached; + } + const schema = domain === 'sessions' ? SESSION_SCHEMA : ACTIVITY_SCHEMA; const parts = path.split('.'); @@ -107,17 +120,25 @@ function resolveFieldPath( for (const part of parts) { const found = currentFields.find((f) => f.name === part); if (!found) { - return { field: null, exists: false, computedField: false }; + const result = { field: null, exists: false, computedField: false }; + if (fieldPathResolutionCache.size < 1000) { + fieldPathResolutionCache.set(cacheKey, result); + } + return result; } currentField = found; currentFields = found.fields || []; } - return { + const result = { field: currentField, exists: true, computedField: currentField?.computed || false, }; + if (fieldPathResolutionCache.size < 1000) { + fieldPathResolutionCache.set(cacheKey, result); + } + return result; } /** diff --git a/packages/core/src/session.ts b/packages/core/src/session.ts index c43b307d..3ab490f7 100644 --- a/packages/core/src/session.ts +++ b/packages/core/src/session.ts @@ -234,15 +234,17 @@ export class SessionClientImpl implements SessionClient { * console.log(reply.message); */ async ask(prompt: string): Promise { - const startTime = new Date(); + const startTime = Date.now(); await this.send(prompt); // Don't return our own message. for await (const activity of this.stream({ exclude: { originator: 'user' }, })) { - const activityTime = new Date(activity.createTime).getTime(); - const askTime = startTime.getTime(); + const activityTime = activity.createTime + ? Date.parse(activity.createTime) + : 0; + const askTime = startTime; if (activityTime <= askTime) { continue; diff --git a/packages/core/src/sessions.ts b/packages/core/src/sessions.ts index 1f60c6ca..3d7e86bd 100644 --- a/packages/core/src/sessions.ts +++ b/packages/core/src/sessions.ts @@ -18,6 +18,7 @@ import { ApiClient } from './api.js'; import { SessionResource, RestSessionResource } from './types.js'; import { SessionStorage } from './storage/types.js'; import { mapRestSessionToSdkSession } from './mappers.js'; +import { validatePageToken } from './utils/validators.js'; export type ListSessionsOptions = { pageSize?: number; @@ -70,7 +71,11 @@ export class SessionCursor private storage: SessionStorage, private platform: any, private options: ListSessionsOptions = {}, - ) {} + ) { + if (this.options.pageToken) { + validatePageToken(this.options.pageToken); + } + } /** * DX Feature: Promise Compatibility. diff --git a/packages/core/src/sources.ts b/packages/core/src/sources.ts index d81eb8c9..f36133ae 100644 --- a/packages/core/src/sources.ts +++ b/packages/core/src/sources.ts @@ -23,6 +23,7 @@ import { GitHubRepo, ListSourcesOptions, } from './types.js'; +import { validateRepository } from './utils/validators.js'; // Internal type representing the raw source from the REST API interface RestGitHubRepo { @@ -131,9 +132,7 @@ class SourceManagerImpl { */ async get(filter: { github: string }): Promise { const { github } = filter; - if (!github || !github.includes('/')) { - throw new Error("Invalid GitHub filter. Expected format: 'owner/repo'."); - } + validateRepository(github); const resourceName = `sources/github/${github}`; diff --git a/packages/core/src/storage/cache-info.ts b/packages/core/src/storage/cache-info.ts index d53d9386..2d8b227a 100644 --- a/packages/core/src/storage/cache-info.ts +++ b/packages/core/src/storage/cache-info.ts @@ -18,6 +18,7 @@ import * as fs from 'fs/promises'; import * as path from 'path'; import { Activity } from '../types.js'; import { getRootDir } from './root.js'; +import { validateSessionId } from '../utils/validators.js'; import { GlobalCacheMetadata, SessionMetadata } from './types.js'; /** @@ -49,8 +50,10 @@ export async function getSessionCacheInfo( sessionId: string, rootDirOverride?: string, ): Promise { + validateSessionId(sessionId); const rootDir = rootDirOverride ?? getRootDir(); - const sessionDir = path.join(rootDir, '.jules/cache', sessionId); + const cleanId = sessionId.replace(/^sessions\//, ''); + const sessionDir = path.join(rootDir, '.jules/cache', cleanId); const sessionPath = path.join(sessionDir, 'session.json'); const metadataPath = path.join(sessionDir, 'metadata.json'); @@ -204,11 +207,13 @@ export async function getLatestActivities( n: number, rootDirOverride?: string, ): Promise { + validateSessionId(sessionId); const rootDir = rootDirOverride ?? getRootDir(); + const cleanId = sessionId.replace(/^sessions\//, ''); const activitiesPath = path.join( rootDir, '.jules/cache', - sessionId, + cleanId, 'activities.jsonl', ); @@ -232,19 +237,21 @@ export async function getLatestActivities( const chunkLines = combined.split('\n'); tail = chunkLines.shift() || ''; + // Iterate backwards through the lines in this chunk. + // Since we read the file backwards from the end, accumulating with push() + // naturally yields a newest-first order. This avoids O(N^2) unshifting overhead. for (let i = chunkLines.length - 1; i >= 0; i--) { if (chunkLines[i] && lines.length < n) { - lines.unshift(chunkLines[i]); + lines.push(chunkLines[i]); } } } if (lines.length < n && tail) { - lines.unshift(tail); + lines.push(tail); } - const finalLines = lines.slice(-n).reverse(); - return finalLines.map((line) => JSON.parse(line)); + return lines.map((line) => JSON.parse(line)); } catch (e: any) { if (e.code === 'ENOENT') return []; throw e; diff --git a/packages/core/src/storage/memory.ts b/packages/core/src/storage/memory.ts index d7d4500c..8f7bfe42 100644 --- a/packages/core/src/storage/memory.ts +++ b/packages/core/src/storage/memory.ts @@ -67,6 +67,23 @@ export class MemoryStorage implements ActivityStorage { } } + /** + * Appends multiple activities in a single optimized operation. + */ + async appendMany(activities: Activity[]): Promise { + const len = activities.length; + for (let i = 0; i < len; i++) { + const activity = activities[i]; + if (this.indices.has(activity.id)) { + const index = this.indices.get(activity.id)!; + this.activities[index] = activity; + } else { + const index = this.activities.push(activity) - 1; + this.indices.set(activity.id, index); + } + } + } + /** * Retrieves an activity by ID. */ diff --git a/packages/core/src/storage/node-fs.ts b/packages/core/src/storage/node-fs.ts index 1f38c1ec..212cd0b0 100644 --- a/packages/core/src/storage/node-fs.ts +++ b/packages/core/src/storage/node-fs.ts @@ -19,6 +19,7 @@ import { createReadStream, createWriteStream, WriteStream } from 'fs'; import * as path from 'path'; import * as readline from 'readline'; import { Activity, SessionResource } from '../types.js'; +import { validateSessionId } from '../utils/validators.js'; import { ActivityStorage, SessionStorage, @@ -42,11 +43,16 @@ export class NodeFileStorage implements ActivityStorage { private indexBuilt = false; private indexBuildPromise: Promise | null = null; + // In-memory cache for session metadata to prevent redundant disk I/O + private metadataCache: SessionMetadata | null = null; + // Tracks the current file size to calculate offsets for new appends private currentFileSize = 0; constructor(sessionId: string, rootDir: string) { - const sessionCacheDir = path.resolve(rootDir, '.jules/cache', sessionId); + validateSessionId(sessionId); + const cleanId = sessionId.replace(/^sessions\//, ''); + const sessionCacheDir = path.resolve(rootDir, '.jules/cache', cleanId); this.filePath = path.join(sessionCacheDir, 'activities.jsonl'); this.metadataPath = path.join(sessionCacheDir, 'metadata.json'); } @@ -105,23 +111,28 @@ export class NodeFileStorage implements ActivityStorage { this.initialized = false; this.indexBuilt = false; this.index.clear(); + this.metadataCache = null; // We do not await indexBuildPromise as we are closing. this.indexBuildPromise = null; } private async _readMetadata(): Promise { + if (this.metadataCache) return this.metadataCache; try { const content = await fs.readFile(this.metadataPath, 'utf8'); - return JSON.parse(content) as SessionMetadata; + this.metadataCache = JSON.parse(content) as SessionMetadata; + return this.metadataCache; } catch (e: any) { if (e.code === 'ENOENT') { - return { activityCount: 0 }; // Default if file doesn't exist + this.metadataCache = { activityCount: 0 }; + return this.metadataCache; // Default if file doesn't exist } throw e; } } private async _writeMetadata(metadata: SessionMetadata): Promise { + this.metadataCache = metadata; await fs.writeFile( this.metadataPath, JSON.stringify(metadata, null, 2), @@ -174,6 +185,54 @@ export class NodeFileStorage implements ActivityStorage { } } + /** + * Appends multiple activities in a single optimized operation. + */ + async appendMany(activities: Activity[]): Promise { + if (activities.length === 0) return; + if (!this.initialized) await this.init(); + + // 1. Atomically update metadata once + const metadata = await this._readMetadata(); + metadata.activityCount += activities.length; + await this._writeMetadata(metadata); + + // 2. Append all activities in a single batch + let batchContent = ''; + const startOffsets = new Array(activities.length); + let currentOffset = this.currentFileSize; + + for (let i = 0; i < activities.length; i++) { + const activity = activities[i]; + const line = JSON.stringify(activity) + '\n'; + batchContent += line; + startOffsets[i] = currentOffset; + currentOffset += Buffer.byteLength(line); + } + + if (this.writeStream) { + const canContinue = this.writeStream.write(batchContent); + this.currentFileSize = currentOffset; + + if (this.indexBuilt || this.indexBuildPromise) { + for (let i = 0; i < activities.length; i++) { + const activity = activities[i]; + if (!this.index.has(activity.id)) { + this.index.set(activity.id, startOffsets[i]); + } + } + } + + if (!canContinue) { + await new Promise((resolve) => + this.writeStream!.once('drain', resolve), + ); + } + } else { + throw new Error('NodeFileStorage: WriteStream is not initialized'); + } + } + /** * Builds the in-memory index by scanning the file once. * Handles concurrency by coalescing multiple calls into a single promise. @@ -414,6 +473,10 @@ export class NodeSessionStorage implements SessionStorage { private indexFilePath: string; private initialized = false; + // In-memory caching for index file entries to avoid repeated disk reads and line-by-line parsing + private cachedEntries: SessionIndexEntry[] | null = null; + private lastMtimeMs = 0; + constructor(rootDir: string) { this.cacheDir = path.resolve(rootDir, '.jules/cache'); this.indexFilePath = path.join(this.cacheDir, 'sessions.jsonl'); @@ -426,7 +489,9 @@ export class NodeSessionStorage implements SessionStorage { } private getSessionPath(sessionId: string): string { - return path.join(this.cacheDir, sessionId, 'session.json'); + validateSessionId(sessionId); + const cleanId = sessionId.replace(/^sessions\//, ''); + return path.join(this.cacheDir, cleanId, 'session.json'); } async upsert(session: SessionResource): Promise { @@ -467,8 +532,49 @@ export class NodeSessionStorage implements SessionStorage { } async upsertMany(sessions: SessionResource[]): Promise { - // Parallelize file writes, sequentialize index write - await Promise.all(sessions.map((s) => this.upsert(s))); + if (sessions.length === 0) return; + await this.init(); + + const now = Date.now(); + const indexEntries: string[] = []; + + // Performance Optimization: Parallelize individual session subdirectory creations and + // atomic session.json file writes concurrently to maximize I/O throughput. + // Instead of calling upsert() which appends to the index file individually, we collect + // and batch all index entries to write them in a single aggregated append file call. + // This completely avoids lock contention and reduces file system system calls from O(N) to O(1). + await Promise.all( + sessions.map(async (session) => { + const sessionDir = path.join(this.cacheDir, session.id); + await fs.mkdir(sessionDir, { recursive: true }); + + const cached: CachedSession = { + resource: session, + _lastSyncedAt: now, + }; + + await fs.writeFile( + path.join(sessionDir, 'session.json'), + JSON.stringify(cached, null, 2), + 'utf8', + ); + + const indexEntry: SessionIndexEntry = { + id: session.id, + title: session.title, + state: session.state, + createTime: session.createTime, + source: session.sourceContext?.source || 'unknown', + _updatedAt: now, + }; + indexEntries.push(JSON.stringify(indexEntry) + '\n'); + }), + ); + + // Single-pass batch append to the high-speed index file + if (indexEntries.length > 0) { + await fs.appendFile(this.indexFilePath, indexEntries.join(''), 'utf8'); + } } async get(sessionId: string): Promise { @@ -483,9 +589,11 @@ export class NodeSessionStorage implements SessionStorage { } async delete(sessionId: string): Promise { + validateSessionId(sessionId); await this.init(); // 1. Remove the directory (Metadata + Activities + Artifacts) - const sessionDir = path.join(this.cacheDir, sessionId); + const cleanId = sessionId.replace(/^sessions\//, ''); + const sessionDir = path.join(this.cacheDir, cleanId); await fs.rm(sessionDir, { recursive: true, force: true }); // 2. We do NOT rewrite the index here for performance. @@ -496,10 +604,18 @@ export class NodeSessionStorage implements SessionStorage { async *scanIndex(): AsyncIterable { await this.init(); - // Read the raw stream - // Note: In Phase 3 (Query Planner), we will optimize this to read backward - // or keep an in-memory map to dedupe instantly. try { + const stats = await fs.stat(this.indexFilePath); + const mtimeMs = stats.mtimeMs; + + // If mtime matches our cache, yield from memory instantly (O(1) vs parsing file) + if (this.cachedEntries && this.lastMtimeMs === mtimeMs) { + for (const entry of this.cachedEntries) { + yield entry; + } + return; + } + const fileStream = createReadStream(this.indexFilePath, { encoding: 'utf8', }); @@ -521,11 +637,19 @@ export class NodeSessionStorage implements SessionStorage { } } - for (const entry of entries.values()) { + const deduplicated = Array.from(entries.values()); + this.cachedEntries = deduplicated; + this.lastMtimeMs = mtimeMs; + + for (const entry of deduplicated) { yield entry; } } catch (e: any) { - if (e.code === 'ENOENT') return; // No index yet + if (e.code === 'ENOENT') { + this.cachedEntries = null; + this.lastMtimeMs = 0; + return; // No index yet + } throw e; } } diff --git a/packages/core/src/storage/types.ts b/packages/core/src/storage/types.ts index 6dee4f2c..542e786b 100644 --- a/packages/core/src/storage/types.ts +++ b/packages/core/src/storage/types.ts @@ -107,6 +107,11 @@ export interface ActivityStorage { */ append(activity: Activity): Promise; + /** + * Persists multiple activities in a single optimized operation. + */ + appendMany?(activities: Activity[]): Promise; + /** * Retrieves a specific activity by its ID. * @returns The activity if found, or undefined. diff --git a/packages/core/src/utils.ts b/packages/core/src/utils.ts index 767095bf..fdd99620 100644 --- a/packages/core/src/utils.ts +++ b/packages/core/src/utils.ts @@ -17,6 +17,12 @@ /** * The internal engine for jules.all() * + * Highly optimized parallel mapping function with fast-paths. + * - O(1) instant return for empty inputs (bypasses all worker allocation/Promise.all overhead). + * - O(1) instant return for single-item inputs (bypasses worker allocation, array filling, Promise.all/loops). + * - Capped concurrency pool Math.min(concurrency, items.length) to eliminate spawning redundant worker promises + * when concurrency is greater than the item count. + * * @param items - Data to process * @param mapper - Async function (item) => result * @param options - Configuration options @@ -30,35 +36,71 @@ export async function pMap( delayMs?: number; } = {}, ): Promise { - const concurrency = options.concurrency ?? 3; - const stopOnError = options.stopOnError ?? true; + const itemsLen = items.length; + + // Optimization: Fast-path for empty inputs. Completely avoids array, promise, or timer allocations. + if (itemsLen === 0) { + return []; + } + const delayMs = options.delayMs ?? 0; + const stopOnError = options.stopOnError ?? true; + + // Optimization: Fast-path for single-item inputs. Avoids worker pool construction, + // key iterator state machine tracking, and intermediate array mapping allocations. + if (itemsLen === 1) { + if (delayMs > 0) { + await new Promise((resolve) => setTimeout(resolve, delayMs)); + } + try { + const res = await mapper(items[0], 0); + return [res]; + } catch (err) { + if (stopOnError) { + throw err; + } + throw new AggregateError( + [err], + 'Multiple errors occurred during jules.all()', + ); + } + } + + // Optimization: Limit active worker pool allocation to the minimum of concurrency or items length. + // This avoids allocating redundant, immediately-resolving idle worker promises. + const limit = options.concurrency ?? 3; + const concurrency = limit < itemsLen ? limit : itemsLen; - const results = new Array(items.length); + const results = new Array(itemsLen); const errors = new Array(); let nextIndex = 0; - const workers = new Array(concurrency).fill(0).map(async () => { - while (true) { - const index = nextIndex++; - if (index >= items.length) { - break; - } - const item = items[index]; + // Optimization: Construct worker promises using a native indexed loop rather than chaining + // fill() and map() array allocation helpers, which completely avoids garbage collection churn. + const workers = new Array(concurrency); + for (let i = 0; i < concurrency; i++) { + workers[i] = (async () => { + while (true) { + const index = nextIndex++; + if (index >= itemsLen) { + break; + } + const item = items[index]; - if (delayMs > 0) { - await new Promise((resolve) => setTimeout(resolve, delayMs)); - } - try { - results[index] = await mapper(item, index); - } catch (err) { - if (stopOnError) { - throw err; + if (delayMs > 0) { + await new Promise((resolve) => setTimeout(resolve, delayMs)); + } + try { + results[index] = await mapper(item, index); + } catch (err) { + if (stopOnError) { + throw err; + } + errors.push(err); } - errors.push(err); } - } - }); + })(); + } await Promise.all(workers); diff --git a/packages/core/src/utils/page-token.ts b/packages/core/src/utils/page-token.ts index 16941833..07db6d9d 100644 --- a/packages/core/src/utils/page-token.ts +++ b/packages/core/src/utils/page-token.ts @@ -63,9 +63,10 @@ export function isSessionFrozen( lastActivityCreateTime: string, thresholdDays = 30, ): boolean { - const lastActivity = new Date(lastActivityCreateTime); - const now = new Date(); - const ageMs = now.getTime() - lastActivity.getTime(); - const ageDays = ageMs / (1000 * 60 * 60 * 24); + // Use Date.parse() instead of new Date() to avoid heap allocation. + const lastActivityMs = Date.parse(lastActivityCreateTime); + if (isNaN(lastActivityMs)) return false; + const ageMs = Date.now() - lastActivityMs; + const ageDays = ageMs / 86400000; // 86400000 ms in a day (1000 * 60 * 60 * 24) return ageDays > thresholdDays; } diff --git a/packages/core/src/utils/validators.ts b/packages/core/src/utils/validators.ts new file mode 100644 index 00000000..51ead266 --- /dev/null +++ b/packages/core/src/utils/validators.ts @@ -0,0 +1,231 @@ +/** + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * Validates a given sessionId to prevent directory/path traversal + * and injection attacks when interacting with the local filesystem. + * + * @param sessionId - The session ID to validate. + * @throws {Error} If the session ID is invalid. + */ +export function validateSessionId(sessionId: string): void { + if (!sessionId) { + throw new Error('INVALID_SESSION_ID: Session ID cannot be empty'); + } + + const cleanId = sessionId.replace(/^sessions\//, ''); + + if (!cleanId) { + throw new Error('INVALID_SESSION_ID: Session ID cannot be empty'); + } + + if (cleanId.includes('\x00') || /[\x01-\x1f\x7f]/.test(cleanId)) { + throw new Error( + `INVALID_SESSION_ID: Session ID contains control characters: ${sessionId}`, + ); + } + + if (cleanId.includes('/') || cleanId.includes('\\')) { + throw new Error( + `INVALID_SESSION_ID: Session ID cannot contain slashes or backslashes: ${sessionId}`, + ); + } + + if (cleanId === '.' || cleanId === '..') { + throw new Error( + `INVALID_SESSION_ID: Session ID cannot be "." or "..": ${sessionId}`, + ); + } +} + +/** + * Validates a given pageToken to prevent directory/path traversal, control characters, + * and injection risks when interacting with local filesystems, caches, or logs. + * + * @param pageToken - The page token to validate. + * @throws {Error} If the page token is invalid. + */ +export function validatePageToken(pageToken: string): void { + if (!pageToken) { + throw new Error('INVALID_PAGE_TOKEN: Page token cannot be empty'); + } + + if (pageToken.includes('\x00') || /[\x01-\x1f\x7f]/.test(pageToken)) { + throw new Error( + `CONTROL_CHAR: Page token contains control characters: ${pageToken}`, + ); + } + + if (pageToken.includes('/') || pageToken.includes('\\')) { + throw new Error( + `INVALID_PAGE_TOKEN: Page token cannot contain slashes or backslashes: ${pageToken}`, + ); + } + + if (pageToken === '.' || pageToken === '..') { + throw new Error( + `PATH_TRAVERSAL: Page token cannot be "." or "..": ${pageToken}`, + ); + } +} + +/** + * Validates a given GitHub repository string to prevent injection and traversal + * when interacting with downstream filesystems and APIs. + * + * @param repo - The repository string (owner/repo). + * @throws {Error} If the repository name is invalid. + */ +export function validateRepository(repo: string): void { + if (!repo) { + throw new Error('INVALID_REPOSITORY: Repository cannot be empty'); + } + + if (repo.includes('\x00') || /[\x01-\x1f\x7f]/.test(repo)) { + throw new Error( + `CONTROL_CHAR: Repository contains control characters: ${repo}`, + ); + } + + const parts = repo.split('/'); + if (parts.length !== 2) { + throw new Error( + `INVALID_REPOSITORY: Repository must be in owner/repo format: ${repo}`, + ); + } + + const [owner, repoName] = parts; + if (!owner || !repoName) { + throw new Error( + `INVALID_REPOSITORY: Repository must be in owner/repo format: ${repo}`, + ); + } + + const validNameRegex = /^[a-zA-Z0-9-._]+$/; + if (!validNameRegex.test(owner) || !validNameRegex.test(repoName)) { + throw new Error( + `INVALID_REPOSITORY: Repository name contains invalid characters: ${repo}`, + ); + } + + if ( + owner === '.' || + owner === '..' || + repoName === '.' || + repoName === '..' + ) { + throw new Error( + `PATH_TRAVERSAL: Repository name cannot contain path traversal segments: ${repo}`, + ); + } +} + +/** + * Validates a given Git branch name to ensure it conforms to security-safe + * git reference naming rules and avoids script injection or command execution risks. + * + * @param branch - The branch name to validate. + * @throws {Error} If the branch name is invalid. + */ +export function validateBranchName(branch: string): void { + if (!branch) { + throw new Error('INVALID_BRANCH: Branch name cannot be empty'); + } + if (branch.startsWith('refs/')) { + throw new Error( + `RESERVED_BRANCH: Branch name must not start with refs/: ${branch}`, + ); + } + // git ref rules: no spaces, no control chars, no consecutive dots, no trailing dot/slash/lock + if (/\s/.test(branch)) { + throw new Error(`INVALID_BRANCH: Branch name contains spaces: ${branch}`); + } + if (/[\x00-\x1f\x7f~^:?*\[\\]/.test(branch)) { + throw new Error( + `INVALID_BRANCH: Branch name contains invalid characters: ${branch}`, + ); + } + if (/\.\./.test(branch)) { + throw new Error( + `INVALID_BRANCH: Branch name contains consecutive dots: ${branch}`, + ); + } + if (/\.$/.test(branch) || /\/$/.test(branch)) { + throw new Error( + `INVALID_BRANCH: Branch name ends with dot or slash: ${branch}`, + ); + } + if (/\.lock$/.test(branch)) { + throw new Error(`INVALID_BRANCH: Branch name ends with .lock: ${branch}`); + } +} + +/** + * Validates a given file path to prevent directory/path traversal, control character, + * and absolute path escape risks. + * + * @param filePath - The file path to validate. + * @throws {Error} If the file path is invalid. + */ +export function validateFilePath(filePath: string): void { + if (!filePath) { + throw new Error('INVALID_FILE_PATH: File path cannot be empty'); + } + if (filePath.includes('\x00') || /[\x01-\x1f\x7f]/.test(filePath)) { + throw new Error( + `CONTROL_CHAR: File path contains control characters: ${filePath}`, + ); + } + const normalized = filePath.replace(/\\/g, '/'); + if (normalized.startsWith('/') || /^[a-zA-Z]:/.test(normalized)) { + throw new Error(`ABSOLUTE_PATH: File path must be relative: ${filePath}`); + } + const parts = normalized.split('/'); + if (parts.some((p) => p === '..')) { + throw new Error(`PATH_TRAVERSAL: File path escapes repo root: ${filePath}`); + } +} + +/** + * Validates a given activityId to prevent directory/path traversal, control characters, + * and injection risks when interacting with local filesystems, caches, or logs. + * + * @param activityId - The activity ID to validate. + * @throws {Error} If the activity ID is invalid. + */ +export function validateActivityId(activityId: string): void { + if (!activityId) { + throw new Error('INVALID_ACTIVITY_ID: Activity ID cannot be empty'); + } + + if (activityId.includes('\x00') || /[\x01-\x1f\x7f]/.test(activityId)) { + throw new Error( + `CONTROL_CHAR: Activity ID contains control characters: ${activityId}`, + ); + } + + if (activityId.includes('/') || activityId.includes('\\')) { + throw new Error( + `INVALID_ACTIVITY_ID: Activity ID cannot contain slashes or backslashes: ${activityId}`, + ); + } + + if (activityId === '.' || activityId === '..') { + throw new Error( + `PATH_TRAVERSAL: Activity ID cannot be "." or "..": ${activityId}`, + ); + } +} diff --git a/packages/core/tests/activities/client.test.ts b/packages/core/tests/activities/client.test.ts index f8831ecd..90dbc46f 100644 --- a/packages/core/tests/activities/client.test.ts +++ b/packages/core/tests/activities/client.test.ts @@ -296,6 +296,50 @@ describe('DefaultActivityClient', () => { const results = await client.select({ after: 'non-existent' }); expect(results).toEqual([]); }); + + it('should hydrate plain artifacts and bypass hydration with reference equality for already hydrated ones', async () => { + const { MediaArtifact, ChangeSetArtifact } = await import('../../src/artifacts.js'); + + const plainMedia = { type: 'media', media: { data: 'test-data', mimeType: 'image/png' } }; + const plainChangeSet = { type: 'changeSet', changeSet: { source: 'git', gitPatch: { unidiffPatch: 'diff' } } }; + + const actPlain = { + id: 'plain', + type: 'agentMessaged', + createTime: recentDate(10), + artifacts: [plainMedia, plainChangeSet], + } as any; + + const mediaInstance = new MediaArtifact({ data: 'test-data', mimeType: 'image/png' }, {} as any, 'rich'); + const changeSetInstance = new ChangeSetArtifact('git', { unidiffPatch: 'diff' }); + + const actRich = { + id: 'rich', + type: 'agentMessaged', + createTime: recentDate(5), + artifacts: [mediaInstance, changeSetInstance], + } as any; + + storageMock.scan = vi.fn().mockImplementation(async function* () { + yield actPlain; + yield actRich; + }); + + const results = await client.select(); + expect(results).toHaveLength(2); + + // Plain should be hydrated + const resPlain = results[0]; + expect(resPlain).not.toBe(actPlain); // cloned + expect(resPlain.artifacts![0]).toBeInstanceOf(MediaArtifact); + expect(resPlain.artifacts![1]).toBeInstanceOf(ChangeSetArtifact); + + // Rich should not be cloned or re-mapped (bypassed entirely!) + const resRich = results[1]; + expect(resRich).toBe(actRich); // EXACT SAME REFERENCE - bypassed! + expect(resRich.artifacts![0]).toBe(mediaInstance); + expect(resRich.artifacts![1]).toBe(changeSetInstance); + }); }); describe('list()', () => { diff --git a/packages/core/tests/api.test.ts b/packages/core/tests/api.test.ts index 545b5d7c..409c0cdf 100644 --- a/packages/core/tests/api.test.ts +++ b/packages/core/tests/api.test.ts @@ -63,3 +63,15 @@ describe('ApiClient (Unit)', () => { ); }); }); + + it('Path Traversal: prevents escaping baseUrl', async () => { + const client = new ApiClient({ + baseUrl: 'https://api.jules.com/v1', + requestTimeoutMs: 1000, + apiKey: 'test-key', + }); + + await expect(client.request('../secret')).rejects.toThrow( + 'Security Error: Invalid path traversal detected in "../secret"', + ); + }); diff --git a/packages/core/tests/query/select.test.ts b/packages/core/tests/query/select.test.ts index fd2a1875..a7248771 100644 --- a/packages/core/tests/query/select.test.ts +++ b/packages/core/tests/query/select.test.ts @@ -408,4 +408,26 @@ describe('Unified Query Engine (select)', () => { expect(infoSpy).toHaveBeenCalledTimes(1); }); }); + + describe('Query Validation', () => { + it('should throw an INVALID_QUERY error for invalid queries', async () => { + // Missing 'from' field + await expect( + select(mockClient as any, {} as any), + ).rejects.toThrow(/INVALID_QUERY: \[MISSING_REQUIRED_FIELD\]/); + + // Invalid domain/from + await expect( + select(mockClient as any, { from: 'invalid' } as any), + ).rejects.toThrow(/INVALID_QUERY: \[INVALID_DOMAIN\]/); + + // Filtering on a computed field + await expect( + select(mockClient as any, { + from: 'sessions', + where: { durationMs: { eq: 123 } }, + } as any), + ).rejects.toThrow(/INVALID_QUERY: \[COMPUTED_FIELD_FILTER\]/); + }); + }); }); diff --git a/packages/core/tests/query/unidiff-benchmark.test.ts b/packages/core/tests/query/unidiff-benchmark.test.ts new file mode 100644 index 00000000..c4c57cfc --- /dev/null +++ b/packages/core/tests/query/unidiff-benchmark.test.ts @@ -0,0 +1,52 @@ +import { test, expect } from 'vitest'; +import { parseUnidiff, parseUnidiffWithContent } from '../../src/artifacts.js'; + +test('benchmark parseUnidiff', () => { + // Generate a large diff patch + const fileCount = 200; + const linesPerFile = 100; + let patch = ''; + + for (let f = 0; f < fileCount; f++) { + patch += `diff --git a/src/file_${f}.ts b/src/file_${f}.ts\n`; + patch += `index abc${f}..def${f} 100644\n`; + if (f % 3 === 0) { + // Created + patch += `--- /dev/null\n`; + patch += `+++ b/src/file_${f}.ts\n`; + } else if (f % 3 === 1) { + // Deleted + patch += `--- a/src/file_${f}.ts\n`; + patch += `+++ /dev/null\n`; + } else { + // Modified + patch += `--- a/src/file_${f}.ts\n`; + patch += `+++ b/src/file_${f}.ts\n`; + } + patch += `@@ -1,3 +1,4 @@\n`; + for (let l = 0; l < linesPerFile; l++) { + if (l % 2 === 0) { + patch += `+added line ${l}\n`; + } else { + patch += `-deleted line ${l}\n`; + } + } + } + + const start1 = performance.now(); + const res1 = parseUnidiff(patch); + const end1 = performance.now(); + console.log( + `parseUnidiff: parsed ${res1.length} files in ${(end1 - start1).toFixed(3)}ms`, + ); + + const start2 = performance.now(); + const res2 = parseUnidiffWithContent(patch); + const end2 = performance.now(); + console.log( + `parseUnidiffWithContent: parsed ${res2.length} files in ${(end2 - start2).toFixed(3)}ms`, + ); + + expect(res1.length).toBe(fileCount); + expect(res2.length).toBe(fileCount); +}); diff --git a/packages/core/tests/storage/node-fs-activities.test.ts b/packages/core/tests/storage/node-fs-activities.test.ts new file mode 100644 index 00000000..e906b1e4 --- /dev/null +++ b/packages/core/tests/storage/node-fs-activities.test.ts @@ -0,0 +1,115 @@ +/** + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'fs/promises'; +import * as path from 'path'; +import { NodeFileStorage } from '../../src/storage/node-fs.js'; +import { Activity } from '../../src/types.js'; + +// Mock fs/promises to spy on readFile +vi.mock('fs/promises', async () => { + const actual = await vi.importActual('fs/promises'); + return { + ...actual, + readFile: vi.fn(actual.readFile), + }; +}); + +const TEST_DIR = path.resolve(__dirname, '.test-activity-cache'); + +describe('NodeFileStorage Activities', () => { + let storage: NodeFileStorage; + const sessionId = 'session_activities_test'; + + beforeEach(async () => { + await fs.rm(TEST_DIR, { recursive: true, force: true }); + storage = new NodeFileStorage(sessionId, TEST_DIR); + vi.clearAllMocks(); + }); + + afterEach(async () => { + await storage.close(); + await fs.rm(TEST_DIR, { recursive: true, force: true }); + }); + + it('should initialize and append a single activity correctly', async () => { + const activity: Activity = { + id: 'act_1', + type: 'userMessaged', + createTime: new Date().toISOString(), + message: 'Hello World', + } as any; + + await storage.append(activity); + + const latest = await storage.latest(); + expect(latest?.id).toBe('act_1'); + expect((latest as any)?.message).toBe('Hello World'); + + const activityFromGet = await storage.get('act_1'); + expect(activityFromGet?.id).toBe('act_1'); + expect((activityFromGet as any)?.message).toBe('Hello World'); + }); + + it('should append multiple activities in a single batch (appendMany)', async () => { + const activities: Activity[] = [ + { id: 'act_10', type: 'userMessaged', createTime: new Date().toISOString(), message: 'Message 1' } as any, + { id: 'act_11', type: 'userMessaged', createTime: new Date().toISOString(), message: 'Message 2' } as any, + { id: 'act_12', type: 'userMessaged', createTime: new Date().toISOString(), message: 'Message 3' } as any, + ]; + + await storage.appendMany(activities); + + const latest = await storage.latest(); + expect(latest?.id).toBe('act_12'); + expect((latest as any)?.message).toBe('Message 3'); + + const act10 = await storage.get('act_10'); + expect((act10 as any)?.message).toBe('Message 1'); + + const act11 = await storage.get('act_11'); + expect((act11 as any)?.message).toBe('Message 2'); + + const act12 = await storage.get('act_12'); + expect((act12 as any)?.message).toBe('Message 3'); + + // Check count in metadata + const metadataPath = path.join(TEST_DIR, '.jules/cache', sessionId, 'metadata.json'); + const metadataContent = await fs.readFile(metadataPath, 'utf8'); + const metadata = JSON.parse(metadataContent); + expect(metadata.activityCount).toBe(3); + }); + + it('should read metadata once on multiple appends due to in-memory caching', async () => { + const activities: Activity[] = [ + { id: 'act_20', type: 'userMessaged', createTime: new Date().toISOString(), message: 'M 1' } as any, + { id: 'act_21', type: 'userMessaged', createTime: new Date().toISOString(), message: 'M 2' } as any, + ]; + + // Trigger metadata read on the first append + await storage.append(activities[0]); + // The second append should hit the cache, not fs.readFile + await storage.append(activities[1]); + + const readCallsForMetadata = vi.mocked(fs.readFile).mock.calls.filter(call => + typeof call[0] === 'string' && call[0].endsWith('metadata.json') + ); + + // Should read exactly once (first append). Second append uses cache. + expect(readCallsForMetadata.length).toBe(1); + }); +}); diff --git a/packages/core/tests/unit/pmap-benchmark.test.ts b/packages/core/tests/unit/pmap-benchmark.test.ts new file mode 100644 index 00000000..25ac74e6 --- /dev/null +++ b/packages/core/tests/unit/pmap-benchmark.test.ts @@ -0,0 +1,61 @@ +/** + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { describe, it, expect } from 'vitest'; +import { pMap } from '../../src/utils.js'; + +describe('pMap Benchmark & Correctness Tests', () => { + it('should handle empty inputs instantly without overhead', async () => { + const start = performance.now(); + const result = await pMap([], async (x) => x); + const end = performance.now(); + + expect(result).toEqual([]); + // Checking < 50ms to be robust against thread preemption/jitter in CI environments + expect(end - start).toBeLessThan(50); + }); + + it('should handle single-item inputs efficiently', async () => { + const start = performance.now(); + const result = await pMap([42], async (x) => x * 2); + const end = performance.now(); + + expect(result).toEqual([84]); + // Checking < 100ms to prevent flakiness in slow virtualized environments + expect(end - start).toBeLessThan(100); + }); + + it('should run high concurrency benchmark', async () => { + const items = Array.from({ length: 1000 }, (_, i) => i); + const start = performance.now(); + const result = await pMap( + items, + async (x) => { + return x + 1; + }, + { concurrency: 50 }, + ); + const end = performance.now(); + + expect(result).toHaveLength(1000); + expect(result[0]).toBe(1); + expect(result[999]).toBe(1000); + + console.log( + `pMap benchmark for 1000 items (concurrency: 50) took: ${end - start}ms`, + ); + }); +}); diff --git a/packages/core/tests/unit/validators.test.ts b/packages/core/tests/unit/validators.test.ts new file mode 100644 index 00000000..6bfca0a3 --- /dev/null +++ b/packages/core/tests/unit/validators.test.ts @@ -0,0 +1,260 @@ +/** + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { describe, test, expect } from 'vitest'; +import { + validateSessionId, + validateRepository, + validateBranchName, + validateFilePath, + validateActivityId, + validatePageToken, +} from '../../src/utils/validators.js'; + +describe('validateSessionId', () => { + test('allows standard session IDs', () => { + expect(() => validateSessionId('SESSION_123')).not.toThrow(); + expect(() => validateSessionId('sessions/SESSION_123')).not.toThrow(); + expect(() => validateSessionId('abc-def-123_456')).not.toThrow(); + }); + + test('rejects empty session IDs', () => { + expect(() => validateSessionId('')).toThrow('INVALID_SESSION_ID'); + expect(() => validateSessionId('sessions/')).toThrow('INVALID_SESSION_ID'); + }); + + test('rejects control characters', () => { + expect(() => validateSessionId('session\x00_id')).toThrow( + 'INVALID_SESSION_ID', + ); + expect(() => validateSessionId('sessions/session\x00_id')).toThrow( + 'INVALID_SESSION_ID', + ); + expect(() => validateSessionId('session\x1f_id')).toThrow( + 'INVALID_SESSION_ID', + ); + }); + + test('rejects slashes and backslashes (after prefix)', () => { + expect(() => validateSessionId('sessions/../etc/passwd')).toThrow( + 'INVALID_SESSION_ID', + ); + expect(() => validateSessionId('sessions/abc/def')).toThrow( + 'INVALID_SESSION_ID', + ); + expect(() => validateSessionId('abc\\def')).toThrow('INVALID_SESSION_ID'); + expect(() => validateSessionId('/etc/passwd')).toThrow( + 'INVALID_SESSION_ID', + ); + }); + + test('rejects "." and ".."', () => { + expect(() => validateSessionId('.')).toThrow('INVALID_SESSION_ID'); + expect(() => validateSessionId('..')).toThrow('INVALID_SESSION_ID'); + expect(() => validateSessionId('sessions/.')).toThrow('INVALID_SESSION_ID'); + expect(() => validateSessionId('sessions/..')).toThrow( + 'INVALID_SESSION_ID', + ); + }); +}); + +describe('validateRepository', () => { + test('allows standard repository strings', () => { + expect(() => validateRepository('owner/repo')).not.toThrow(); + expect(() => validateRepository('google/jules-sdk')).not.toThrow(); + expect(() => validateRepository('owner-name/repo_name.js')).not.toThrow(); + }); + + test('rejects empty repository strings', () => { + expect(() => validateRepository('')).toThrow('INVALID_REPOSITORY'); + }); + + test('rejects repository strings without exact owner/repo format', () => { + expect(() => validateRepository('owner')).toThrow('INVALID_REPOSITORY'); + expect(() => validateRepository('owner/repo/extra')).toThrow( + 'INVALID_REPOSITORY', + ); + expect(() => validateRepository('owner//repo')).toThrow( + 'INVALID_REPOSITORY', + ); + expect(() => validateRepository('/owner/repo')).toThrow( + 'INVALID_REPOSITORY', + ); + expect(() => validateRepository('owner/repo/')).toThrow( + 'INVALID_REPOSITORY', + ); + }); + + test('rejects repository strings containing invalid characters', () => { + expect(() => validateRepository('owner$/repo')).toThrow( + 'INVALID_REPOSITORY', + ); + expect(() => validateRepository('owner/re@po')).toThrow( + 'INVALID_REPOSITORY', + ); + expect(() => validateRepository('owner:/repo')).toThrow( + 'INVALID_REPOSITORY', + ); + }); + + test('rejects control characters', () => { + expect(() => validateRepository('owner\x00/repo')).toThrow('CONTROL_CHAR'); + expect(() => validateRepository('owner/re\x1fpo')).toThrow('CONTROL_CHAR'); + }); + + test('rejects path traversal segments', () => { + expect(() => validateRepository('../repo')).toThrow('PATH_TRAVERSAL'); + expect(() => validateRepository('owner/..')).toThrow('PATH_TRAVERSAL'); + expect(() => validateRepository('../owner/repo')).toThrow( + 'INVALID_REPOSITORY', + ); + expect(() => validateRepository('owner/../repo')).toThrow( + 'INVALID_REPOSITORY', + ); + expect(() => validateRepository('./repo')).toThrow('PATH_TRAVERSAL'); + expect(() => validateRepository('owner/.')).toThrow('PATH_TRAVERSAL'); + }); +}); + +describe('validateBranchName', () => { + test('allows standard branch names', () => { + expect(() => validateBranchName('main')).not.toThrow(); + expect(() => validateBranchName('feature/login')).not.toThrow(); + expect(() => validateBranchName('bug-fix_123')).not.toThrow(); + }); + + test('rejects empty branch names', () => { + expect(() => validateBranchName('')).toThrow('INVALID_BRANCH'); + }); + + test('rejects branch names starting with refs/', () => { + expect(() => validateBranchName('refs/heads/main')).toThrow( + 'RESERVED_BRANCH', + ); + }); + + test('rejects spaces in branch names', () => { + expect(() => validateBranchName('my branch')).toThrow('INVALID_BRANCH'); + }); + + test('rejects invalid git reference characters', () => { + expect(() => validateBranchName('my~branch')).toThrow('INVALID_BRANCH'); + expect(() => validateBranchName('my^branch')).toThrow('INVALID_BRANCH'); + expect(() => validateBranchName('my:branch')).toThrow('INVALID_BRANCH'); + expect(() => validateBranchName('my?branch')).toThrow('INVALID_BRANCH'); + expect(() => validateBranchName('my*branch')).toThrow('INVALID_BRANCH'); + expect(() => validateBranchName('my[branch')).toThrow('INVALID_BRANCH'); + expect(() => validateBranchName('my\\branch')).toThrow('INVALID_BRANCH'); + }); + + test('rejects consecutive dots', () => { + expect(() => validateBranchName('my..branch')).toThrow('INVALID_BRANCH'); + }); + + test('rejects trailing dot or slash', () => { + expect(() => validateBranchName('my-branch.')).toThrow('INVALID_BRANCH'); + expect(() => validateBranchName('my-branch/')).toThrow('INVALID_BRANCH'); + }); + + test('rejects trailing .lock', () => { + expect(() => validateBranchName('my-branch.lock')).toThrow( + 'INVALID_BRANCH', + ); + }); +}); + +describe('validateFilePath', () => { + test('allows relative file paths', () => { + expect(() => validateFilePath('src/index.ts')).not.toThrow(); + expect(() => validateFilePath('index.js')).not.toThrow(); + expect(() => validateFilePath('docs/readme.md')).not.toThrow(); + }); + + test('rejects empty file paths', () => { + expect(() => validateFilePath('')).toThrow('INVALID_FILE_PATH'); + }); + + test('rejects absolute paths', () => { + expect(() => validateFilePath('/etc/passwd')).toThrow('ABSOLUTE_PATH'); + expect(() => validateFilePath('C:/Windows/System32')).toThrow( + 'ABSOLUTE_PATH', + ); + }); + + test('rejects path traversal', () => { + expect(() => validateFilePath('../etc/passwd')).toThrow('PATH_TRAVERSAL'); + expect(() => validateFilePath('src/../../etc/passwd')).toThrow( + 'PATH_TRAVERSAL', + ); + }); + + test('rejects control characters', () => { + expect(() => validateFilePath('src/foo\x00.ts')).toThrow('CONTROL_CHAR'); + expect(() => validateFilePath('src/foo\x1f.ts')).toThrow('CONTROL_CHAR'); + }); +}); + +describe('validateActivityId', () => { + test('allows standard activity IDs', () => { + expect(() => validateActivityId('activity_123')).not.toThrow(); + expect(() => validateActivityId('act-567_abc')).not.toThrow(); + }); + + test('rejects empty activity IDs', () => { + expect(() => validateActivityId('')).toThrow('INVALID_ACTIVITY_ID'); + }); + + test('rejects control characters', () => { + expect(() => validateActivityId('act\x00_id')).toThrow('CONTROL_CHAR'); + expect(() => validateActivityId('act\x1f_id')).toThrow('CONTROL_CHAR'); + }); + + test('rejects slashes and backslashes', () => { + expect(() => validateActivityId('act/123')).toThrow('INVALID_ACTIVITY_ID'); + expect(() => validateActivityId('act\\123')).toThrow('INVALID_ACTIVITY_ID'); + }); + + test('rejects "." and ".."', () => { + expect(() => validateActivityId('.')).toThrow('PATH_TRAVERSAL'); + expect(() => validateActivityId('..')).toThrow('PATH_TRAVERSAL'); + }); +}); + +describe('validatePageToken', () => { + test('allows standard page tokens', () => { + expect(() => validatePageToken('1704448500999999000')).not.toThrow(); + expect(() => validatePageToken('abc-123_xyz')).not.toThrow(); + }); + + test('rejects empty page tokens', () => { + expect(() => validatePageToken('')).toThrow('INVALID_PAGE_TOKEN'); + }); + + test('rejects control characters', () => { + expect(() => validatePageToken('tok\x00en')).toThrow('CONTROL_CHAR'); + expect(() => validatePageToken('tok\x1fen')).toThrow('CONTROL_CHAR'); + }); + + test('rejects slashes and backslashes', () => { + expect(() => validatePageToken('tok/123')).toThrow('INVALID_PAGE_TOKEN'); + expect(() => validatePageToken('tok\\123')).toThrow('INVALID_PAGE_TOKEN'); + }); + + test('rejects "." and ".."', () => { + expect(() => validatePageToken('.')).toThrow('PATH_TRAVERSAL'); + expect(() => validatePageToken('..')).toThrow('PATH_TRAVERSAL'); + }); +}); diff --git a/packages/fleet/src/__tests__/analyze-render.test.ts b/packages/fleet/src/__tests__/analyze-render.test.ts new file mode 100644 index 00000000..600159ac --- /dev/null +++ b/packages/fleet/src/__tests__/analyze-render.test.ts @@ -0,0 +1,88 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { describe, it, expect } from 'vitest'; +import { renderAnalyzeEvent } from '../shared/ui/render/analyze.js'; +import type { RenderContext } from '../shared/ui/spec.js'; +import { + ansiRed, + ansiLink, + sessionUrl, + ansiGreen, + ansiHighlight, +} from '../shared/ui/session-url.js'; + +describe('renderAnalyzeEvent', () => { + const createMockCtx = () => { + const logs: string[] = []; + const ctx: RenderContext = { + info: (msg) => logs.push(`info: ${msg}`), + success: (msg) => logs.push(`success: ${msg}`), + warn: (msg) => logs.push(`warn: ${msg}`), + error: (msg) => logs.push(`error: ${msg}`), + message: (msg) => logs.push(`message: ${msg}`), + step: (msg) => logs.push(`step: ${msg}`), + startSpinner: (msg) => logs.push(`startSpinner: ${msg}`), + stopSpinner: (msg) => logs.push(`stopSpinner${msg ? `: ${msg}` : ''}`), + }; + return { ctx, logs }; + }; + + it('renders analyze:session:failed with red cross mark', () => { + const { ctx, logs } = createMockCtx(); + renderAnalyzeEvent( + { + type: 'analyze:session:failed', + id: 's-123', + error: 'Network Timeout', + }, + ctx, + ); + expect(logs).toContain('stopSpinner'); + expect(logs).toContain(`error: ${ansiRed('✗')} Failed: Network Timeout`); + }); + + it('renders analyze:session:started with clickable link', () => { + const { ctx, logs } = createMockCtx(); + renderAnalyzeEvent( + { + type: 'analyze:session:started', + id: 's-123', + goal: 'test-goal', + }, + ctx, + ); + expect(logs).toContain( + `stopSpinner: Session started: s-123 ${ansiGreen('✓')}`, + ); + const expectedLink = ansiLink('View Session', sessionUrl('s-123')); + expect(logs).toContain(`info: ${expectedLink}`); + }); + + it('renders analyze:goal:start with backticks and highlights file and milestone', () => { + const { ctx, logs } = createMockCtx(); + renderAnalyzeEvent( + { + type: 'analyze:goal:start', + file: '.fleet/goals/improve.md', + index: 1, + total: 2, + milestone: 'v1.0', + }, + ctx, + ); + expect(logs.some((l) => l.includes('`.fleet/goals/improve.md`'))).toBe(true); + expect(logs.some((l) => l.includes('`v1.0`'))).toBe(true); + }); +}); diff --git a/packages/fleet/src/__tests__/auth-git.test.ts b/packages/fleet/src/__tests__/auth-git.test.ts index 082a416f..16babca3 100644 --- a/packages/fleet/src/__tests__/auth-git.test.ts +++ b/packages/fleet/src/__tests__/auth-git.test.ts @@ -13,7 +13,21 @@ // limitations under the License. import { describe, it, expect } from 'vitest'; -import { parseGitRemoteUrl } from '../shared/auth/git.js'; +import { parseGitRemoteUrl, getGitRepoInfo } from '../shared/auth/git.js'; + +describe('getGitRepoInfo', () => { + it('throws a security error if remoteName has invalid characters', async () => { + const oldEnv = process.env.GITHUB_REPOSITORY; + delete process.env.GITHUB_REPOSITORY; + try { + await expect(getGitRepoInfo('origin; rm -rf /')).rejects.toThrow( + 'Security Error: Invalid characters in git remote name', + ); + } finally { + process.env.GITHUB_REPOSITORY = oldEnv; + } + }); +}); describe('parseGitRemoteUrl', () => { it('parses HTTPS URL', () => { diff --git a/packages/fleet/src/__tests__/configure-render.test.ts b/packages/fleet/src/__tests__/configure-render.test.ts new file mode 100644 index 00000000..5e36dda8 --- /dev/null +++ b/packages/fleet/src/__tests__/configure-render.test.ts @@ -0,0 +1,94 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { describe, it, expect } from 'vitest'; +import { renderConfigureEvent } from '../shared/ui/render/configure.js'; +import type { RenderContext } from '../shared/ui/spec.js'; +import { repoConfigUrl, ansiLink, ansiGreen, ansiHighlight } from '../shared/ui/session-url.js'; + +describe('renderConfigureEvent', () => { + const createMockCtx = () => { + const logs: string[] = []; + const ctx: RenderContext = { + info: (msg) => logs.push(`info: ${msg}`), + success: (msg) => logs.push(`success: ${msg}`), + warn: (msg) => logs.push(`warn: ${msg}`), + error: (msg) => logs.push(`error: ${msg}`), + message: (msg) => logs.push(`message: ${msg}`), + step: (msg) => logs.push(`step: ${msg}`), + startSpinner: (msg) => logs.push(`startSpinner: ${msg}`), + stopSpinner: (msg) => logs.push(`stopSpinner${msg ? `: ${msg}` : ''}`), + }; + return { ctx, logs }; + }; + + it('renders configure:start with clickable configuration url', () => { + const { ctx, logs } = createMockCtx(); + renderConfigureEvent( + { + type: 'configure:start', + resource: 'labels', + owner: 'google', + repo: 'jules', + }, + ctx, + ); + expect(logs).toContain( + `info: Configuring ${ansiHighlight('`labels`')} for ${ansiHighlight('`google/jules`')}`, + ); + const expectedLink = ansiLink( + 'View Configuration', + repoConfigUrl('google', 'jules'), + ); + expect(logs).toContain(`info: ${expectedLink}`); + }); + + it('renders configure:label:created correctly', () => { + const { ctx, logs } = createMockCtx(); + renderConfigureEvent( + { + type: 'configure:label:created', + name: 'fleet-merge-ready', + }, + ctx, + ); + const expectedLabel = ansiHighlight('`fleet-merge-ready`'); + expect(logs).toContain(`info: ✓ Label ${expectedLabel} created`); + }); + + it('renders configure:done correctly', () => { + const { ctx, logs } = createMockCtx(); + renderConfigureEvent( + { + type: 'configure:done', + }, + ctx, + ); + expect(logs).toContain('success: Configuration complete'); + }); + + it('renders configure:secret:uploaded correctly with green checkmark', () => { + const { ctx, logs } = createMockCtx(); + renderConfigureEvent( + { + type: 'configure:secret:uploaded', + name: 'MY_API_KEY', + }, + ctx, + ); + expect(logs).toContain( + `stopSpinner: Secret MY_API_KEY uploaded ${ansiGreen('✓')}`, + ); + }); +}); diff --git a/packages/fleet/src/__tests__/dispatch-handler.test.ts b/packages/fleet/src/__tests__/dispatch-handler.test.ts index 29743ca8..a19dd398 100644 --- a/packages/fleet/src/__tests__/dispatch-handler.test.ts +++ b/packages/fleet/src/__tests__/dispatch-handler.test.ts @@ -159,6 +159,63 @@ describe('DispatchHandler', () => { expect(dispatcher.dispatch).not.toHaveBeenCalled(); }); + it('rejects invalid repository name', async () => { + const octokit = createMockOctokit(); + const dispatcher = createMockDispatcher(); + const handler = new DispatchHandler({ octokit, dispatcher }); + + const result = await handler.execute({ + milestone: '1', + goalsDir: '.fleet/goals', + owner: 'owner/invalid', + repo: 'repo', + baseBranch: 'main', + }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.message).toContain('INVALID_REPOSITORY'); + } + }); + + it('rejects invalid base branch name', async () => { + const octokit = createMockOctokit(); + const dispatcher = createMockDispatcher(); + const handler = new DispatchHandler({ octokit, dispatcher }); + + const result = await handler.execute({ + milestone: '1', + goalsDir: '.fleet/goals', + owner: 'owner', + repo: 'repo', + baseBranch: 'refs/heads/main', + }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.message).toContain('RESERVED_BRANCH'); + } + }); + + it('rejects path traversal in goalsDir', async () => { + const octokit = createMockOctokit(); + const dispatcher = createMockDispatcher(); + const handler = new DispatchHandler({ octokit, dispatcher }); + + const result = await handler.execute({ + milestone: '1', + goalsDir: '../etc/goals', + owner: 'owner', + repo: 'repo', + baseBranch: 'main', + }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.message).toContain('PATH_TRAVERSAL'); + } + }); + it('handles dispatcher failure per-issue without failing the batch', async () => { const octokit = createMockOctokit({ openIssues: [ diff --git a/packages/fleet/src/__tests__/dispatch-render.test.ts b/packages/fleet/src/__tests__/dispatch-render.test.ts new file mode 100644 index 00000000..39df208d --- /dev/null +++ b/packages/fleet/src/__tests__/dispatch-render.test.ts @@ -0,0 +1,90 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { describe, it, expect } from 'vitest'; +import { renderDispatchEvent } from '../shared/ui/render/dispatch.js'; +import type { RenderContext } from '../shared/ui/spec.js'; +import { + sessionUrl, + ansiLink, + ansiYellow, + ansiGreen, + ansiHighlight, +} from '../shared/ui/session-url.js'; + +describe('renderDispatchEvent', () => { + const createMockCtx = () => { + const logs: string[] = []; + const ctx: RenderContext = { + info: (msg) => logs.push(`info: ${msg}`), + success: (msg) => logs.push(`success: ${msg}`), + warn: (msg) => logs.push(`warn: ${msg}`), + error: (msg) => logs.push(`error: ${msg}`), + message: (msg) => logs.push(`message: ${msg}`), + step: (msg) => logs.push(`step: ${msg}`), + startSpinner: (msg) => logs.push(`startSpinner: ${msg}`), + stopSpinner: (msg) => logs.push(`stopSpinner${msg ? `: ${msg}` : ''}`), + }; + return { ctx, logs }; + }; + + it('renders dispatch:start correctly', () => { + const { ctx, logs } = createMockCtx(); + renderDispatchEvent( + { + type: 'dispatch:start', + milestone: 'v1.0', + }, + ctx, + ); + expect(logs).toContain( + `info: Dispatching from milestone ${ansiHighlight('`v1.0`')}`, + ); + }); + + it('renders dispatch:issue:skipped with styled yellow warning icon', () => { + const { ctx, logs } = createMockCtx(); + renderDispatchEvent( + { + type: 'dispatch:issue:skipped', + number: 101, + reason: 'Already dispatched', + }, + ctx, + ); + expect(logs).toContain( + `warn: ${ansiYellow('⊘')} #101: Already dispatched`, + ); + }); + + it('renders dispatch:issue:dispatched with clickable session url', () => { + const { ctx, logs } = createMockCtx(); + renderDispatchEvent( + { + type: 'dispatch:issue:dispatched', + number: 42, + sessionId: 'session_xyz_789', + }, + ctx, + ); + expect(logs).toContain( + `stopSpinner: #42 → session ${ansiHighlight('`session_xyz_789`')} ${ansiGreen('✓')}`, + ); + const expectedLink = ansiLink( + 'View Session', + sessionUrl('session_xyz_789'), + ); + expect(logs).toContain(`info: ${expectedLink}`); + }); +}); diff --git a/packages/fleet/src/__tests__/init-render.test.ts b/packages/fleet/src/__tests__/init-render.test.ts new file mode 100644 index 00000000..bbdda7c3 --- /dev/null +++ b/packages/fleet/src/__tests__/init-render.test.ts @@ -0,0 +1,178 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { describe, it, expect } from 'vitest'; +import { renderInitEvent } from '../shared/ui/render/init.js'; +import type { RenderContext } from '../shared/ui/spec.js'; +import { + ansiLink, + ansiYellow, + ansiRed, + ansiGreen, + ansiHighlight, +} from '../shared/ui/session-url.js'; + +describe('renderInitEvent', () => { + const createMockCtx = () => { + const logs: string[] = []; + const ctx: RenderContext = { + info: (msg) => logs.push(`info: ${msg}`), + success: (msg) => logs.push(`success: ${msg}`), + warn: (msg) => logs.push(`warn: ${msg}`), + error: (msg) => logs.push(`error: ${msg}`), + message: (msg) => logs.push(`message: ${msg}`), + step: (msg) => logs.push(`step: ${msg}`), + startSpinner: (msg) => logs.push(`startSpinner: ${msg}`), + stopSpinner: (msg) => logs.push(`stopSpinner${msg ? `: ${msg}` : ''}`), + }; + return { ctx, logs }; + }; + + it('renders init:repo:creating correctly', () => { + const { ctx, logs } = createMockCtx(); + renderInitEvent( + { + type: 'init:repo:creating', + owner: 'google', + name: 'jules', + }, + ctx, + ); + expect(logs).toContain( + `startSpinner: Creating repository ${ansiHighlight('`google/jules`')}…`, + ); + }); + + it('renders init:repo:created correctly with clickable repository url', () => { + const { ctx, logs } = createMockCtx(); + renderInitEvent( + { + type: 'init:repo:created', + fullName: 'google/jules', + url: 'https://github.com/google/jules', + }, + ctx, + ); + expect(logs).toContain( + `stopSpinner: Repository ${ansiHighlight('`google/jules`')} created ${ansiGreen('✓')}`, + ); + const expectedLink = ansiLink( + 'View Repository', + 'https://github.com/google/jules', + ); + expect(logs).toContain(`info: ${expectedLink}`); + }); + + it('renders init:repo:exists with warning symbol', () => { + const { ctx, logs } = createMockCtx(); + renderInitEvent( + { + type: 'init:repo:exists', + fullName: 'google/jules', + }, + ctx, + ); + expect(logs).toContain( + `warn: ${ansiYellow('⊘')} Repository ${ansiHighlight('`google/jules`')} already exists`, + ); + }); + + it('renders init:repo:failed and stops spinner', () => { + const { ctx, logs } = createMockCtx(); + renderInitEvent( + { + type: 'init:repo:failed', + reason: 'API Error', + }, + ctx, + ); + expect(logs).toContain('stopSpinner'); + expect(logs).toContain( + `error: ${ansiRed('✗')} Repository creation failed: API Error`, + ); + }); + + it('renders init:start correctly', () => { + const { ctx, logs } = createMockCtx(); + renderInitEvent( + { + type: 'init:start', + owner: 'google', + repo: 'jules', + }, + ctx, + ); + expect(logs).toContain( + `info: Initializing fleet for ${ansiHighlight('`google/jules`')}`, + ); + }); + + it('renders init:branch:creating correctly', () => { + const { ctx, logs } = createMockCtx(); + renderInitEvent( + { + type: 'init:branch:creating', + name: 'fleet-setup', + base: 'main', + }, + ctx, + ); + expect(logs).toContain( + `startSpinner: Creating branch ${ansiHighlight('`fleet-setup`')} from ${ansiHighlight('`main`')}`, + ); + }); + + it('renders init:branch:created correctly', () => { + const { ctx, logs } = createMockCtx(); + renderInitEvent( + { + type: 'init:branch:created', + name: 'fleet-setup', + }, + ctx, + ); + expect(logs).toContain( + `stopSpinner: Branch ${ansiHighlight('`fleet-setup`')} created ${ansiGreen('✓')}`, + ); + }); + + it('renders init:file:committed correctly', () => { + const { ctx, logs } = createMockCtx(); + renderInitEvent( + { + type: 'init:file:committed', + path: '.fleet/goals/example.md', + }, + ctx, + ); + expect(logs).toContain( + `info: ${ansiGreen('✓')} ${ansiHighlight('`.fleet/goals/example.md`')}`, + ); + }); + + it('renders init:file:skipped correctly', () => { + const { ctx, logs } = createMockCtx(); + renderInitEvent( + { + type: 'init:file:skipped', + path: '.fleet/goals/example.md', + reason: 'File already exists', + }, + ctx, + ); + expect(logs).toContain( + `warn: ${ansiYellow('⊘')} ${ansiHighlight('`.fleet/goals/example.md`')} — File already exists`, + ); + }); +}); diff --git a/packages/fleet/src/__tests__/init-wizard.test.ts b/packages/fleet/src/__tests__/init-wizard.test.ts index c1f25c10..c5e58923 100644 --- a/packages/fleet/src/__tests__/init-wizard.test.ts +++ b/packages/fleet/src/__tests__/init-wizard.test.ts @@ -14,6 +14,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { validateHeadlessInputs } from '../init/wizard/headless.js'; +import { parseRepositoryInput } from '../init/wizard/interactive.js'; import type { FleetEvent } from '../shared/events.js'; // Mock getGitRepoInfo to avoid real git calls @@ -256,4 +257,64 @@ describe('validateHeadlessInputs (Non-Interactive Mode)', () => { expect(result.baseBranch).toBe('develop'); } }); + + it('fails with invalid repository name (control characters)', async () => { + process.env.GITHUB_TOKEN = 'ghp_test'; + const result = await validateHeadlessInputs( + { repo: 'owner/repo\x00invalid' }, + () => {}, + ); + expect('success' in result).toBe(true); + if ('success' in result) { + expect(result.success).toBe(false); + expect(result.error.message).toContain('CONTROL_CHAR'); + } + }); + + it('fails with invalid base branch name', async () => { + process.env.GITHUB_TOKEN = 'ghp_test'; + const result = await validateHeadlessInputs( + { repo: 'owner/repo', base: 'invalid branch' }, + () => {}, + ); + expect('success' in result).toBe(true); + if ('success' in result) { + expect(result.success).toBe(false); + expect(result.error.message).toContain('Branch name contains spaces'); + } + }); +}); + +describe('parseRepositoryInput', () => { + it('passes standard owner/repo format through unchanged', () => { + expect(parseRepositoryInput('google/jules')).toBe('google/jules'); + expect(parseRepositoryInput('my-org/my-repo_123')).toBe('my-org/my-repo_123'); + }); + + it('trims leading and trailing whitespace', () => { + expect(parseRepositoryInput(' google/jules ')).toBe('google/jules'); + }); + + it('strips https protocol and github domain', () => { + expect(parseRepositoryInput('https://github.com/google/jules')).toBe('google/jules'); + expect(parseRepositoryInput('http://github.com/google/jules')).toBe('google/jules'); + }); + + it('strips www subdomain', () => { + expect(parseRepositoryInput('https://www.github.com/google/jules')).toBe('google/jules'); + }); + + it('strips trailing .git extension', () => { + expect(parseRepositoryInput('google/jules.git')).toBe('google/jules'); + expect(parseRepositoryInput('https://github.com/google/jules.git')).toBe('google/jules'); + }); + + it('strips ssh prefixes', () => { + expect(parseRepositoryInput('git@github.com:google/jules.git')).toBe('google/jules'); + expect(parseRepositoryInput('git@github.com:google/jules')).toBe('google/jules'); + }); + + it('cleans up extra slashes', () => { + expect(parseRepositoryInput('/google/jules/')).toBe('google/jules'); + }); }); diff --git a/packages/fleet/src/__tests__/merge-render.test.ts b/packages/fleet/src/__tests__/merge-render.test.ts new file mode 100644 index 00000000..cd44beab --- /dev/null +++ b/packages/fleet/src/__tests__/merge-render.test.ts @@ -0,0 +1,244 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { describe, it, expect } from 'vitest'; +import { renderMergeEvent } from '../shared/ui/render/merge.js'; +import type { RenderContext } from '../shared/ui/spec.js'; +import { + sessionUrl, + ansiLink, + ansiYellow, + ansiGreen, + ansiRed, + ansiHighlight, +} from '../shared/ui/session-url.js'; + +describe('renderMergeEvent', () => { + const createMockCtx = () => { + const logs: string[] = []; + const ctx: RenderContext = { + info: (msg) => logs.push(`info: ${msg}`), + success: (msg) => logs.push(`success: ${msg}`), + warn: (msg) => logs.push(`warn: ${msg}`), + error: (msg) => logs.push(`error: ${msg}`), + message: (msg) => logs.push(`message: ${msg}`), + step: (msg) => logs.push(`step: ${msg}`), + startSpinner: (msg) => logs.push(`startSpinner: ${msg}`), + stopSpinner: (msg) => logs.push(`stopSpinner${msg ? `: ${msg}` : ''}`), + }; + return { ctx, logs }; + }; + + it('renders merge:start with backtick-formatted repository name', () => { + const { ctx, logs } = createMockCtx(); + renderMergeEvent( + { + type: 'merge:start', + prCount: 2, + owner: 'google', + repo: 'jules', + mode: 'auto', + }, + ctx, + ); + expect(logs).toContain( + `info: Merging 2 PR(s) in ${ansiHighlight('`google/jules`')} [auto]`, + ); + }); + + it('renders merge:conflict:escalated with clickable session url', () => { + const { ctx, logs } = createMockCtx(); + renderMergeEvent( + { + type: 'merge:conflict:escalated', + prNumber: 123, + sessionId: 'session_abc_123', + failureCount: 3, + }, + ctx, + ); + expect(logs).toContain( + `info: ↳ Escalated PR #123 → session ${ansiHighlight('`session_abc_123`')} (3 consecutive failures)`, + ); + const expectedLink = ansiLink( + 'View Session', + sessionUrl('session_abc_123'), + ); + expect(logs).toContain(`info: ${expectedLink}`); + }); + + it('renders merge:batch-resolve:done with clickable session url', () => { + const { ctx, logs } = createMockCtx(); + renderMergeEvent( + { + type: 'merge:batch-resolve:done', + prNumbers: [101, 102], + sessionId: 'session_batch_456', + }, + ctx, + ); + expect(logs).toContain( + `stopSpinner: Batch resolved #101, #102 → session ${ansiHighlight('`session_batch_456`')} ${ansiGreen('✓')}`, + ); + const expectedLink = ansiLink( + 'View Session', + sessionUrl('session_batch_456'), + ); + expect(logs).toContain(`info: ${expectedLink}`); + }); + + it('renders merge:redispatch:done with clickable session url', () => { + const { ctx, logs } = createMockCtx(); + renderMergeEvent( + { + type: 'merge:redispatch:done', + oldPr: 789, + sessionId: 'session_redispatch_789', + }, + ctx, + ); + expect(logs).toContain( + `stopSpinner: Re-dispatched PR #789 → session ${ansiHighlight('`session_redispatch_789`')} ${ansiGreen('✓')}`, + ); + const expectedLink = ansiLink( + 'View Session', + sessionUrl('session_redispatch_789'), + ); + expect(logs).toContain(`info: ${expectedLink}`); + }); + + it('renders merge:pr:skipped with styled yellow warning icon', () => { + const { ctx, logs } = createMockCtx(); + renderMergeEvent( + { + type: 'merge:pr:skipped', + prNumber: 999, + reason: 'Missing approval', + }, + ctx, + ); + expect(logs).toContain( + `warn: ${ansiYellow('⊘')} PR #999: Missing approval`, + ); + }); + + it('renders merge:conflict:notifying and merge:conflict:notified correctly', () => { + const { ctx, logs: logsNotifying } = createMockCtx(); + renderMergeEvent( + { + type: 'merge:conflict:notifying', + prNumber: 456, + sessionId: 'session_notify_456', + }, + ctx, + ); + expect(logsNotifying).toContain( + `startSpinner: Notifying session ${ansiHighlight('`session_notify_456`')} of conflict on PR #456…`, + ); + + const { ctx: ctxNotified, logs: logsNotified } = createMockCtx(); + renderMergeEvent( + { + type: 'merge:conflict:notified', + prNumber: 456, + sessionId: 'session_notify_456', + }, + ctxNotified, + ); + expect(logsNotified).toContain( + `stopSpinner: Notified session ${ansiHighlight('`session_notify_456`')} of conflict on PR #456 ${ansiGreen('✓')}`, + ); + const expectedLink = ansiLink( + 'View Session', + sessionUrl('session_notify_456'), + ); + expect(logsNotified).toContain(`info: ${expectedLink}`); + }); + + it('renders merge:ci:passed, merge:ci:failed, merge:ci:timeout, merge:ci:none correctly with status indicators', () => { + const { ctx, logs } = createMockCtx(); + renderMergeEvent( + { + type: 'merge:ci:passed', + prNumber: 111, + }, + ctx, + ); + expect(logs).toContain( + `stopSpinner: CI passed for PR #111 ${ansiGreen('✓')}`, + ); + + const { ctx: ctxFail, logs: logsFail } = createMockCtx(); + renderMergeEvent( + { + type: 'merge:ci:failed', + prNumber: 222, + }, + ctxFail, + ); + expect(logsFail).toContain( + `stopSpinner: CI failed for PR #222 ${ansiRed('✗')}`, + ); + + const { ctx: ctxTimeout, logs: logsTimeout } = createMockCtx(); + renderMergeEvent( + { + type: 'merge:ci:timeout', + prNumber: 333, + }, + ctxTimeout, + ); + expect(logsTimeout).toContain( + `stopSpinner: CI timed out for PR #333 ${ansiYellow('⊘')}`, + ); + + const { ctx: ctxNone, logs: logsNone } = createMockCtx(); + renderMergeEvent( + { + type: 'merge:ci:none', + prNumber: 444, + }, + ctxNone, + ); + expect(logsNone).toContain( + `stopSpinner: No CI checks for PR #444 ${ansiYellow('⊘')}`, + ); + }); + + it('renders merge:branch:updated and merge:conflict:detected with correct status indicators', () => { + const { ctx, logs } = createMockCtx(); + renderMergeEvent( + { + type: 'merge:branch:updated', + prNumber: 555, + }, + ctx, + ); + expect(logs).toContain( + `stopSpinner: Branch updated for PR #555 ${ansiGreen('✓')}`, + ); + + const { ctx: ctxConflict, logs: logsConflict } = createMockCtx(); + renderMergeEvent( + { + type: 'merge:conflict:detected', + prNumber: 666, + }, + ctxConflict, + ); + expect(logsConflict).toContain( + `stopSpinner: Conflict detected on PR #666 ${ansiRed('✗')}`, + ); + }); +}); diff --git a/packages/fleet/src/__tests__/session-url.test.ts b/packages/fleet/src/__tests__/session-url.test.ts index e06f8956..17df11b6 100644 --- a/packages/fleet/src/__tests__/session-url.test.ts +++ b/packages/fleet/src/__tests__/session-url.test.ts @@ -12,8 +12,73 @@ // See the License for the specific language governing permissions and // limitations under the License. -import { describe, it, expect } from 'vitest'; -import { sessionUrl } from '../shared/ui/session-url.js'; +import { describe, it, expect, afterEach } from 'vitest'; +import { sessionUrl, repoConfigUrl, ansiLink, ansiHighlight } from '../shared/ui/session-url.js'; + +describe('ansiHighlight', () => { + const originalEnvCI = process.env.CI; + const originalIsTTY = process.stdout.isTTY; + + afterEach(() => { + process.env.CI = originalEnvCI; + process.stdout.isTTY = originalIsTTY; + }); + + it('highlights backticked text in interactive/TTY environment', () => { + delete process.env.CI; + process.stdout.isTTY = true; + + const result = ansiHighlight('Use `jules-fleet configure` now'); + expect(result).toBe('Use `\x1b[33mjules-fleet configure\x1b[39m` now'); + }); + + it('leaves text unhighlighted when not in interactive/TTY environment', () => { + delete process.env.CI; + process.stdout.isTTY = false; + + const result = ansiHighlight('Use `jules-fleet configure` now'); + expect(result).toBe('Use `jules-fleet configure` now'); + }); +}); + +describe('ansiLink', () => { + const originalEnvCI = process.env.CI; + const originalIsTTY = process.stdout.isTTY; + + afterEach(() => { + process.env.CI = originalEnvCI; + process.stdout.isTTY = originalIsTTY; + }); + + it('wraps text with OSC 8 escape sequences in interactive/TTY environment', () => { + delete process.env.CI; + process.stdout.isTTY = true; + + const link = ansiLink('click here', 'https://jules.google.com'); + expect(link).toBe( + '\x1b]8;;https://jules.google.com\x07\x1b[36m\x1b[4mclick here\x1b[24m\x1b[39m\x1b]8;;\x07', + ); + }); + + it('falls back to plain format when not in an interactive/TTY environment', () => { + delete process.env.CI; + process.stdout.isTTY = false; + + const link = ansiLink('click here', 'https://jules.google.com'); + expect(link).toBe('click here (https://jules.google.com)'); + + const sameLink = ansiLink('https://jules.google.com', 'https://jules.google.com'); + expect(sameLink).toBe('https://jules.google.com'); + }); + + it('falls back to plain format when process.env.CI is true even if TTY is true', () => { + process.env.CI = 'true'; + process.stdout.isTTY = true; + + const link = ansiLink('click here', 'https://jules.google.com'); + expect(link).toBe('click here (https://jules.google.com)'); + }); +}); describe('sessionUrl', () => { it('uses /session/ (singular) in the URL', () => { @@ -23,26 +88,36 @@ describe('sessionUrl', () => { }); }); +describe('repoConfigUrl', () => { + it('constructs correct URL for repository configuration', () => { + const url = repoConfigUrl('google', 'jules'); + expect(url).toBe('https://jules.google.com/repo/github/google/jules/config'); + }); +}); + describe('dispatch comment session regex', () => { // The regex from status.ts: /Session:\s*\[?`([^`]+)`\]?/ const regex = /Session:\s*\[?`([^`]+)`\]?/; it('parses old format: Session: `id`', () => { - const body = '🤖 **Fleet Dispatch Event**\nSession: `abc123`\nTimestamp: 2026-01-01'; + const body = + '🤖 **Fleet Dispatch Event**\nSession: `abc123`\nTimestamp: 2026-01-01'; const match = body.match(regex); expect(match).not.toBeNull(); expect(match![1]).toBe('abc123'); }); it('parses new format: Session: [`id`](url)', () => { - const body = '🤖 **Fleet Dispatch Event**\nSession: [`abc123`](https://jules.google.com/session/abc123)\nTimestamp: Mar 3, 2026'; + const body = + '🤖 **Fleet Dispatch Event**\nSession: [`abc123`](https://jules.google.com/session/abc123)\nTimestamp: Mar 3, 2026'; const match = body.match(regex); expect(match).not.toBeNull(); expect(match![1]).toBe('abc123'); }); it('handles numeric session IDs', () => { - const body = 'Session: [`17338656567244366276`](https://jules.google.com/session/17338656567244366276)'; + const body = + 'Session: [`17338656567244366276`](https://jules.google.com/session/17338656567244366276)'; const match = body.match(regex); expect(match).not.toBeNull(); expect(match![1]).toBe('17338656567244366276'); diff --git a/packages/fleet/src/dispatch/handler.ts b/packages/fleet/src/dispatch/handler.ts index 81afe4eb..3696cac8 100644 --- a/packages/fleet/src/dispatch/handler.ts +++ b/packages/fleet/src/dispatch/handler.ts @@ -23,6 +23,11 @@ import { recordDispatch } from './events.js'; import { parseGoalFile } from '../analyze/goals.js'; import { globSync } from 'glob'; import { existsSync, readFileSync } from 'node:fs'; +import { + validateRepository, + validateBranchName, + validateFilePath, +} from '@google/jules-sdk'; export interface DispatchHandlerDeps { octokit: Octokit; @@ -48,6 +53,15 @@ export class DispatchHandler implements DispatchSpec { async execute(input: DispatchInput): Promise { try { + // Validate inputs defensively at outer execution boundary + validateRepository(`${input.owner}/${input.repo}`); + if (input.baseBranch) { + validateBranchName(input.baseBranch); + } + if (input.goalsDir) { + validateFilePath(input.goalsDir); + } + this.emit({ type: 'dispatch:start', milestone: input.milestone ?? 'all' }); this.emit({ type: 'dispatch:scanning' }); diff --git a/packages/fleet/src/init/handler.ts b/packages/fleet/src/init/handler.ts index a2ab1f55..e15ac0fa 100644 --- a/packages/fleet/src/init/handler.ts +++ b/packages/fleet/src/init/handler.ts @@ -82,13 +82,13 @@ export class InitHandler implements InitSpec { type: 'error', code: 'ALREADY_INITIALIZED', message: 'All fleet files already exist — nothing to commit.', - suggestion: 'This repo appears to be already initialized. Use jules-fleet configure to update settings.', + suggestion: 'This repo appears to be already initialized. Use `jules-fleet configure` to update settings.', }); return fail( 'FILE_COMMIT_FAILED', 'All fleet files already exist — nothing to commit.', false, - 'This repo appears to be already initialized. Use jules-fleet configure to update settings.', + 'This repo appears to be already initialized. Use `jules-fleet configure` to update settings.', ); } diff --git a/packages/fleet/src/init/wizard/headless.ts b/packages/fleet/src/init/wizard/headless.ts index d2b51524..a7c2fa10 100644 --- a/packages/fleet/src/init/wizard/headless.ts +++ b/packages/fleet/src/init/wizard/headless.ts @@ -18,6 +18,7 @@ import type { FleetEmitter } from '../../shared/events.js'; import { buildWorkflowTemplates } from '../templates.js'; import type { InitArgs, InitWizardResult } from './types.js'; import { parseFeatureFlags } from './parse-features.js'; +import { validateRepository, validateBranchName } from '@google/jules-sdk'; /** * Validate all required inputs from flags + env vars in non-interactive mode. @@ -41,6 +42,14 @@ export async function validateHeadlessInputs( ); } } + + // Validate repository name to prevent path traversal, control characters, or script injections + try { + validateRepository(repoSlug); + } catch (err: any) { + return fail('UNKNOWN_ERROR', err.message, false); + } + const [owner, repo] = repoSlug.split('/'); if (!owner || !repo) { return fail('UNKNOWN_ERROR', `Invalid repo format: "${repoSlug}". Expected owner/repo.`, false); @@ -94,6 +103,14 @@ export async function validateHeadlessInputs( const intervalMinutes = rawInterval; const baseBranch = args.base ?? 'main'; + + // Validate branch name to prevent git reference escapes or shell/command injections + try { + validateBranchName(baseBranch); + } catch (err: any) { + return fail('UNKNOWN_ERROR', err.message, false); + } + const dryRun = args['dry-run'] ?? false; // In non-interactive mode, never upload secrets by default diff --git a/packages/fleet/src/init/wizard/interactive.ts b/packages/fleet/src/init/wizard/interactive.ts index 21403043..95d7a59b 100644 --- a/packages/fleet/src/init/wizard/interactive.ts +++ b/packages/fleet/src/init/wizard/interactive.ts @@ -20,6 +20,25 @@ import { WORKFLOW_TEMPLATES, buildWorkflowTemplates } from '../templates.js'; import { createFleetOctokit } from '../../shared/auth/octokit.js'; import type { InitArgs, InitWizardResult } from './types.js'; import { parseFeatureFlags } from './parse-features.js'; +import { ansiLink, ansiHighlight } from '../../shared/ui/session-url.js'; +import { validateRepository, validateBranchName } from '@google/jules-sdk'; + +/** + * Clean and parse a user-supplied repository input. + * Supports pasting full GitHub HTTPS/SSH URLs and extracts the standard owner/repo format. + */ +export function parseRepositoryInput(input: string): string { + let cleaned = input.trim(); + // Strip protocol and optional www. prefix + cleaned = cleaned.replace(/^(https?:\/\/)?(www\.)?github\.com\//i, ''); + // Strip SSH prefix + cleaned = cleaned.replace(/^git@github\.com:/i, ''); + // Strip trailing .git extension + cleaned = cleaned.replace(/\.git$/i, ''); + // Strip any leading or trailing slashes + cleaned = cleaned.replace(/^\/+|\/+$/g, ''); + return cleaned; +} /** * Prompts user to choose auth method. Returns null if cancelled. @@ -28,8 +47,16 @@ async function promptAuthMethod(): Promise<'token' | 'app' | null> { const authChoice = await p.select({ message: 'How will Fleet authenticate with GitHub?', options: [ - { value: 'token' as const, label: 'Personal Access Token (GITHUB_TOKEN)' }, - { value: 'app' as const, label: 'GitHub App (recommended for orgs)' }, + { + value: 'token' as const, + label: 'Personal Access Token (GITHUB_TOKEN)', + hint: 'Quickest setup — best for personal use and testing', + }, + { + value: 'app' as const, + label: 'GitHub App (recommended for orgs)', + hint: 'Most secure — ideal for teams, orgs, and enterprise permissions', + }, ], }); if (p.isCancel(authChoice)) return null; @@ -58,25 +85,53 @@ export async function runInitWizard( if (repoSlug) { const confirmed = await p.confirm({ - message: `Detected repository: ${repoSlug}. Is this correct?`, + message: ansiHighlight( + `Detected repository: \`${repoSlug}\`. Is this correct?`, + ), initialValue: true, }); - if (p.isCancel(confirmed)) return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + if (p.isCancel(confirmed)) + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); if (!confirmed) { const manual = await p.text({ message: 'Enter repository in owner/repo format:', - validate: (v) => !v || !/^[^/]+\/[^/]+$/.test(v) ? 'Must be owner/repo format' : undefined, + placeholder: 'e.g., owner/repo', + validate: (v) => { + if (!v) return 'Repository is required'; + try { + validateRepository(parseRepositoryInput(v)); + } catch (err: any) { + return err.message; + } + }, }); - if (p.isCancel(manual)) return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); - repoSlug = manual; + if (p.isCancel(manual)) + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + repoSlug = parseRepositoryInput(manual); } } else { const manual = await p.text({ message: 'Enter repository in owner/repo format:', - validate: (v) => !v || !/^[^/]+\/[^/]+$/.test(v) ? 'Must be owner/repo format' : undefined, + placeholder: 'e.g., owner/repo', + validate: (v) => { + if (!v) return 'Repository is required'; + try { + validateRepository(parseRepositoryInput(v)); + } catch (err: any) { + return err.message; + } + }, }); - if (p.isCancel(manual)) return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); - repoSlug = manual; + if (p.isCancel(manual)) + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + repoSlug = parseRepositoryInput(manual); + } + + // Validate the final repoSlug to prevent path traversal, control characters, or script injections + try { + validateRepository(repoSlug); + } catch (err: any) { + return fail('UNKNOWN_ERROR', err.message, false); } const [owner, repo] = repoSlug.split('/'); @@ -84,6 +139,13 @@ export async function runInitWizard( // ── Step 2: Base branch ── const baseBranch = args.base ?? 'main'; + // Validate branch name to prevent git reference escapes or shell/command injections + try { + validateBranchName(baseBranch); + } catch (err: any) { + return fail('UNKNOWN_ERROR', err.message, false); + } + // ── Step 3: Authentication ── const { AuthDetectHandler } = await import('../auth-detect/handler.js'); const detector = new AuthDetectHandler(); @@ -91,7 +153,8 @@ export async function runInitWizard( const detectResult = await detector.execute({ owner, repo, - preferredMethod: args.auth === 'token' || args.auth === 'app' ? args.auth : undefined, + preferredMethod: + args.auth === 'token' || args.auth === 'app' ? args.auth : undefined, }); let authMethod: 'token' | 'app'; @@ -103,22 +166,27 @@ export async function runInitWizard( if (alternatives && alternatives.length > 1) { const choice = await p.select({ message: `Multiple auth methods detected. Which to use?`, - options: alternatives.map(a => ({ + options: alternatives.map((a) => ({ value: a.method, - label: a.method === 'app' - ? `GitHub App (from ${a.source})` - : `Personal Access Token (from ${a.source})`, + label: + a.method === 'app' + ? `GitHub App (from ${a.source})` + : `Personal Access Token (from ${a.source})`, })), }); - if (p.isCancel(choice)) return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + if (p.isCancel(choice)) + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); authMethod = choice; } else { // Single method detected — confirm const useDetected = await p.confirm({ - message: `Authenticated as ${identity} via ${source} (${method}). Use this?`, + message: ansiHighlight( + `Authenticated as \`${identity}\` via \`${source}\` (\`${method}\`). Use this?`, + ), initialValue: true, }); - if (p.isCancel(useDetected)) return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + if (p.isCancel(useDetected)) + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); if (useDetected) { authMethod = method; } else { @@ -132,72 +200,98 @@ export async function runInitWizard( p.log.warn(detectResult.error.message); p.log.info(`Your credentials are valid — the repo name may be wrong.`); - const fixedRepo = await p.text({ + let fixedRepo = await p.text({ message: 'Enter the correct repository (owner/repo):', initialValue: `${owner}/${repo}`, - validate: (v) => !v?.includes('/') ? 'Format: owner/repo' : undefined, + placeholder: 'e.g., owner/repo', + validate: (v) => { + if (!v) return 'Repository is required'; + try { + validateRepository(parseRepositoryInput(v)); + } catch (err: any) { + return err.message; + } + }, }); - if (p.isCancel(fixedRepo)) return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + if (p.isCancel(fixedRepo)) + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + fixedRepo = parseRepositoryInput(fixedRepo); const [fixedOwner, fixedRepoName] = fixedRepo.split('/'); // Re-run detection with corrected repo const retryResult = await detector.execute({ owner: fixedOwner, repo: fixedRepoName, - preferredMethod: args.auth === 'token' || args.auth === 'app' ? args.auth : undefined, + preferredMethod: + args.auth === 'token' || args.auth === 'app' ? args.auth : undefined, }); if (retryResult.success) { authMethod = retryResult.data.method; - p.log.success(`✓ Authenticated as ${retryResult.data.identity} with access to ${fixedOwner}/${fixedRepoName}`); + p.log.success( + `✓ Authenticated as ${retryResult.data.identity} with access to ${fixedOwner}/${fixedRepoName}`, + ); } else { - return fail('UNKNOWN_ERROR', retryResult.error.message, retryResult.error.recoverable); + return fail( + 'UNKNOWN_ERROR', + retryResult.error.message, + retryResult.error.recoverable, + ); } } else { // Auth detection failed — show why and fall through to manual flow if (detectResult.error.code === 'HEALTH_CHECK_FAILED') { - p.log.warn(`Auth check failed: ${detectResult.error.message}`); + p.log.warn( + ansiHighlight(`Auth check failed: ${detectResult.error.message}`), + ); if (detectResult.error.suggestion) { - p.log.info(detectResult.error.suggestion); + p.log.info(ansiHighlight(detectResult.error.suggestion)); } } - const authChoice = await p.select({ - message: 'How will Fleet authenticate with GitHub?', - options: [ - { value: 'token' as const, label: 'Personal Access Token (GITHUB_TOKEN)' }, - { value: 'app' as const, label: 'GitHub App (recommended for orgs)' }, - ], - }); - if (p.isCancel(authChoice)) return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + const authChoice = await promptAuthMethod(); + if (!authChoice) + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); authMethod = authChoice; // Prompt for credentials if (authMethod === 'token') { const token = await p.password({ message: 'Paste your GitHub token:', + validate: (v) => (!v?.trim() ? 'GitHub token is required' : undefined), }); - if (p.isCancel(token)) return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + if (p.isCancel(token)) + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); process.env.GITHUB_TOKEN = token; } else { // ── GitHub App: slug → key file → auto-detect ── - const { resolvePrivateKeyFromInput } = await import('../../shared/auth/resolve-key-input.js'); - const { resolveInstallation } = await import('../../shared/auth/resolve-installation.js'); + const { resolvePrivateKeyFromInput } = + await import('../../shared/auth/resolve-key-input.js'); + const { resolveInstallation } = + await import('../../shared/auth/resolve-installation.js'); const slug = await p.text({ - message: 'What is your GitHub App slug? (from the URL: github.com/settings/apps/)', - validate: (v) => !v?.trim() ? 'App slug is required' : undefined, + message: + 'What is your GitHub App slug? (from the URL: github.com/settings/apps/)', + placeholder: 'e.g., my-github-app-slug', + validate: (v) => (!v?.trim() ? 'App slug is required' : undefined), }); - if (p.isCancel(slug)) return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + if (p.isCancel(slug)) + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); - p.log.info(`Download your private key from: https://github.com/settings/apps/${slug}`); + p.log.info( + `Download your private key from: ${ansiLink('GitHub App Settings', `https://github.com/settings/apps/${slug}`)}`, + ); const keyInput = await p.text({ - message: 'Path to your private key (.pem file), or paste the key directly:', - validate: (v) => !v?.trim() ? 'Private key is required' : undefined, + message: + 'Path to your private key (.pem file), or paste the key directly:', + placeholder: 'e.g., path/to/key.pem or paste the private key block', + validate: (v) => (!v?.trim() ? 'Private key is required' : undefined), }); - if (p.isCancel(keyInput)) return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + if (p.isCancel(keyInput)) + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); let privateKeyPem: string; try { @@ -211,32 +305,58 @@ export async function runInitWizard( } const s = p.spinner(); - s.start(`Authenticating as "${slug}" and finding installation for ${owner}/${repo}...`); + s.start( + ansiHighlight( + `Authenticating as \`${slug}\` and finding installation for \`${owner}/${repo}\`...`, + ), + ); try { const { Octokit } = await import('octokit'); const tempOctokit = new Octokit(); - const { data: appData } = await tempOctokit.rest.apps.getBySlug({ app_slug: slug }); + const { data: appData } = await tempOctokit.rest.apps.getBySlug({ + app_slug: slug, + }); if (!appData) { - throw new Error(`Could not find GitHub App with slug "${slug}". Check the slug at https://github.com/settings/apps`); + throw new Error( + `Could not find GitHub App with slug "${slug}". Check the slug at https://github.com/settings/apps`, + ); } const appId = String(appData.id); - const resolved = await resolveInstallation(appId, privateKeyPem, owner, repo); + const resolved = await resolveInstallation( + appId, + privateKeyPem, + owner, + repo, + ); - s.stop(`Authenticated as "${resolved.appName}" (ID: ${resolved.appId})`); - p.log.success(`Found installation for ${resolved.accountLogin} (ID: ${resolved.installationId})`); + s.stop( + ansiHighlight( + `Authenticated as \`${resolved.appName}\` (ID: \`${resolved.appId}\`)`, + ), + ); + p.log.success( + ansiHighlight( + `Found installation for \`${resolved.accountLogin}\` (ID: \`${resolved.installationId}\`)`, + ), + ); process.env.GITHUB_APP_ID = appId; - process.env.GITHUB_APP_INSTALLATION_ID = String(resolved.installationId); + process.env.GITHUB_APP_INSTALLATION_ID = String( + resolved.installationId, + ); process.env.GITHUB_APP_PRIVATE_KEY = privateKeyPem; - process.env.GITHUB_APP_PRIVATE_KEY_BASE64 = Buffer.from(privateKeyPem).toString('base64'); + process.env.GITHUB_APP_PRIVATE_KEY_BASE64 = + Buffer.from(privateKeyPem).toString('base64'); } catch (err) { s.stop('Authentication failed'); return fail( 'UNKNOWN_ERROR', - err instanceof Error ? err.message : 'Could not authenticate with GitHub App.', + err instanceof Error + ? err.message + : 'Could not authenticate with GitHub App.', true, ); } @@ -251,18 +371,33 @@ export async function runInitWizard( if (!julesKey) { const wantKey = await p.confirm({ - message: 'Fleet needs a JULES_API_KEY to dispatch sessions. Do you have one?', + message: ansiHighlight( + 'Fleet needs a `JULES_API_KEY` to dispatch sessions. Do you have one?', + ), initialValue: true, }); - if (!p.isCancel(wantKey) && wantKey) { - const key = await p.password({ message: 'Enter your Jules API key:' }); - if (!p.isCancel(key)) { - process.env.JULES_API_KEY = key; - secretsToUpload['JULES_API_KEY'] = key; + if (p.isCancel(wantKey)) { + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + } + if (wantKey) { + const key = await p.password({ + message: 'Enter your Jules API key:', + validate: (v) => (!v?.trim() ? 'Jules API key is required' : undefined), + }); + if (p.isCancel(key)) { + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); } + process.env.JULES_API_KEY = key; + secretsToUpload['JULES_API_KEY'] = key; + } else { + p.log.info( + ansiHighlight( + `💡 You can retrieve or request a \`JULES_API_KEY\` at ${ansiLink('Jules Console', 'https://jules.google.com')}\n (Setup will complete, but dispatching worker sessions will require it later)`, + ), + ); } } else { - p.log.success('JULES_API_KEY detected'); + p.log.success(ansiHighlight('`JULES_API_KEY` detected')); secretsToUpload['JULES_API_KEY'] = julesKey; } @@ -273,7 +408,10 @@ export async function runInitWizard( message: `Upload ${Object.keys(secretsToUpload).length} secret(s) to GitHub Actions secrets?`, initialValue: true, }); - if (p.isCancel(confirmed) || !confirmed) { + if (p.isCancel(confirmed)) { + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + } + if (!confirmed) { Object.keys(secretsToUpload).forEach((k) => delete secretsToUpload[k]); } } @@ -284,13 +422,19 @@ export async function runInitWizard( message: 'Upload GitHub App credentials to repo secrets?', initialValue: true, }); - if (!p.isCancel(uploadApp) && uploadApp) { - if (process.env.GITHUB_APP_ID) secretsToUpload['FLEET_APP_ID'] = process.env.GITHUB_APP_ID; + if (p.isCancel(uploadApp)) { + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + } + if (uploadApp) { + if (process.env.GITHUB_APP_ID) + secretsToUpload['FLEET_APP_ID'] = process.env.GITHUB_APP_ID; if (process.env.GITHUB_APP_PRIVATE_KEY_BASE64) { - secretsToUpload['FLEET_APP_PRIVATE_KEY'] = process.env.GITHUB_APP_PRIVATE_KEY_BASE64; + secretsToUpload['FLEET_APP_PRIVATE_KEY'] = + process.env.GITHUB_APP_PRIVATE_KEY_BASE64; } if (process.env.GITHUB_APP_INSTALLATION_ID) { - secretsToUpload['FLEET_APP_INSTALLATION_ID'] = process.env.GITHUB_APP_INSTALLATION_ID; + secretsToUpload['FLEET_APP_INSTALLATION_ID'] = + process.env.GITHUB_APP_INSTALLATION_ID; } } } @@ -304,28 +448,48 @@ export async function runInitWizard( const cadenceChoice = await p.select({ message: 'How often should Fleet run?', options: [ - { value: 30, label: 'Every 30 minutes', hint: 'High velocity — fast signal, more API/Actions usage' }, - { value: 60, label: 'Every hour', hint: 'Balanced — good signal, moderate usage' }, - { value: 360, label: 'Every 6 hours', hint: 'Standard (default) — reliable daily cadence' }, - { value: 720, label: 'Every 12 hours', hint: 'Conservative — twice daily' }, + { + value: 30, + label: 'Every 30 minutes', + hint: 'High velocity — fast signal, more API/Actions usage', + }, + { + value: 60, + label: 'Every hour', + hint: 'Balanced — good signal, moderate usage', + }, + { + value: 360, + label: 'Every 6 hours', + hint: 'Standard (default) — reliable daily cadence', + }, + { + value: 720, + label: 'Every 12 hours', + hint: 'Conservative — twice daily', + }, { value: 1440, label: 'Every 24 hours', hint: 'Minimal — once daily' }, { value: -1, label: 'Custom', hint: 'Enter interval in minutes' }, ], initialValue: 360, }); - if (p.isCancel(cadenceChoice)) return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + if (p.isCancel(cadenceChoice)) + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); if (cadenceChoice === -1) { const custom = await p.text({ message: 'Enter interval in minutes (minimum 5):', initialValue: '360', + placeholder: 'e.g., 60', validate: (v) => { const n = parseInt(v ?? '', 10); - if (isNaN(n) || n < 5) return 'Must be a number ≥ 5 (GitHub Actions minimum)'; + if (isNaN(n) || n < 5) + return 'Must be a number ≥ 5 (GitHub Actions minimum)'; return undefined; }, }); - if (p.isCancel(custom)) return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + if (p.isCancel(custom)) + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); intervalMinutes = parseInt(custom, 10); } else { intervalMinutes = cadenceChoice; @@ -344,7 +508,11 @@ export async function runInitWizard( const existingFiles: string[] = []; for (const tmpl of templatesToCheck) { try { - await octokit.rest.repos.getContent({ owner, repo, path: tmpl.repoPath }); + await octokit.rest.repos.getContent({ + owner, + repo, + path: tmpl.repoPath, + }); existingFiles.push(tmpl.repoPath); } catch { // File doesn't exist — will be created fresh @@ -359,29 +527,35 @@ export async function runInitWizard( message: 'Overwrite existing workflow files with latest templates?', initialValue: true, }); - if (p.isCancel(shouldOverwrite)) return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + if (p.isCancel(shouldOverwrite)) + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); overwrite = shouldOverwrite; } } // ── Step 7: Confirmation ── if (!dryRun) { - const files = buildWorkflowTemplates(intervalMinutes).map((t) => t.repoPath); + const files = buildWorkflowTemplates(intervalMinutes).map( + (t) => t.repoPath, + ); files.push('.fleet/goals/example.md'); - p.log.info([ - 'Fleet will:', - ` • Create a branch from ${baseBranch}`, - ` • ${overwrite ? 'Overwrite' : 'Commit'} ${files.length} files`, - ' • Open a pull request', - ' • Configure labels (fleet, fleet-merge-ready)', - ].join('\n')); + p.log.info( + [ + 'Fleet will:', + ` • Create a branch from ${baseBranch}`, + ` • ${overwrite ? 'Overwrite' : 'Commit'} ${files.length} files`, + ' • Open a pull request', + ' • Configure labels (fleet, fleet-merge-ready)', + ].join('\n'), + ); const proceed = await p.confirm({ message: 'Create the PR now?', initialValue: true, }); - if (p.isCancel(proceed)) return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); + if (p.isCancel(proceed)) + return fail('UNKNOWN_ERROR', 'Setup cancelled.', false); if (!proceed) { emit({ type: 'init:dry-run', files }); return fail( @@ -392,5 +566,15 @@ export async function runInitWizard( } } - return { owner, repo, baseBranch, authMethod, secretsToUpload, dryRun, overwrite, features: parseFeatureFlags(args), intervalMinutes }; + return { + owner, + repo, + baseBranch, + authMethod, + secretsToUpload, + dryRun, + overwrite, + features: parseFeatureFlags(args), + intervalMinutes, + }; } diff --git a/packages/fleet/src/shared/auth/git.ts b/packages/fleet/src/shared/auth/git.ts index a3c52565..0fce1e69 100644 --- a/packages/fleet/src/shared/auth/git.ts +++ b/packages/fleet/src/shared/auth/git.ts @@ -41,6 +41,13 @@ export async function getGitRepoInfo( return { owner, repo, fullName: ghRepo }; } + // Validate remoteName to prevent shell command injection + if (!/^[a-zA-Z0-9._\/-]+$/.test(remoteName)) { + throw new Error( + `Security Error: Invalid characters in git remote name: "${remoteName}"`, + ); + } + const { stdout } = await execAsync(`git remote get-url ${remoteName}`); return parseGitRemoteUrl(stdout.trim()); } diff --git a/packages/fleet/src/shared/ui/render/analyze.ts b/packages/fleet/src/shared/ui/render/analyze.ts index 9b82ae92..e0fb3518 100644 --- a/packages/fleet/src/shared/ui/render/analyze.ts +++ b/packages/fleet/src/shared/ui/render/analyze.ts @@ -14,24 +14,42 @@ import type { AnalyzeEvent } from '../../events/analyze.js'; import type { RenderContext } from '../spec.js'; -import { sessionUrl } from '../session-url.js'; +import { + sessionUrl, + ansiLink, + ansiDim, + ansiRed, + ansiHighlight, + ansiGreen, +} from '../session-url.js'; /** Render an analyze-domain event. */ -export function renderAnalyzeEvent(event: AnalyzeEvent, ctx: RenderContext): void { +export function renderAnalyzeEvent( + event: AnalyzeEvent, + ctx: RenderContext, +): void { switch (event.type) { case 'analyze:start': - ctx.info(`Analyzing ${event.goalCount} goal(s) for ${event.owner}/${event.repo}`); + ctx.info( + `Analyzing ${event.goalCount} goal(s) for ${event.owner}/${event.repo}`, + ); break; case 'analyze:goal:start': if (event.total > 1) { - ctx.step(`[${event.index}/${event.total}] ${event.file}`); + ctx.step( + `${ansiDim(`[${event.index}/${event.total}]`)} ${ansiHighlight(`\`${event.file}\``)}`, + ); } else { - ctx.step(event.file); + ctx.step(ansiHighlight(`\`${event.file}\``)); + } + if (event.milestone) { + ctx.info(` Milestone: ${ansiHighlight(`\`${event.milestone}\``)}`); } - if (event.milestone) ctx.info(` Milestone: ${event.milestone}`); break; case 'analyze:milestone:resolved': - ctx.info(` Milestone "${event.title}" (#${event.id})`); + ctx.info( + ` Milestone ${ansiHighlight(`\`${event.title}\``)} (#${event.id})`, + ); break; case 'analyze:context:fetched': ctx.info( @@ -42,12 +60,12 @@ export function renderAnalyzeEvent(event: AnalyzeEvent, ctx: RenderContext): voi ctx.startSpinner(`Dispatching session for ${event.goal}…`); break; case 'analyze:session:started': - ctx.stopSpinner(`Session started: ${event.id}`); - ctx.info(` ${sessionUrl(event.id)}`); + ctx.stopSpinner(`Session started: ${event.id} ${ansiGreen('✓')}`); + ctx.info(` ${ansiLink('View Session', sessionUrl(event.id))}`); break; case 'analyze:session:failed': ctx.stopSpinner(); - ctx.error(` Failed: ${event.error}`); + ctx.error(` ${ansiRed('✗')} Failed: ${ansiHighlight(event.error)}`); break; case 'analyze:done': ctx.success( diff --git a/packages/fleet/src/shared/ui/render/configure.ts b/packages/fleet/src/shared/ui/render/configure.ts index a1b13f69..bd17759c 100644 --- a/packages/fleet/src/shared/ui/render/configure.ts +++ b/packages/fleet/src/shared/ui/render/configure.ts @@ -14,30 +14,45 @@ import type { ConfigureEvent } from '../../events/configure.js'; import type { RenderContext } from '../spec.js'; +import { + repoConfigUrl, + ansiLink, + ansiGreen, + ansiYellow, + ansiHighlight, +} from '../session-url.js'; /** Render a configure-domain event. */ -export function renderConfigureEvent(event: ConfigureEvent, ctx: RenderContext): void { +export function renderConfigureEvent( + event: ConfigureEvent, + ctx: RenderContext, +): void { switch (event.type) { case 'configure:start': - ctx.info(`Configuring ${event.resource} for ${event.owner}/${event.repo}`); + ctx.info( + `Configuring ${ansiHighlight(`\`${event.resource}\``)} for ${ansiHighlight(`\`${event.owner}/${event.repo}\``)}`, + ); + ctx.info( + ` ${ansiLink('View Configuration', repoConfigUrl(event.owner, event.repo))}`, + ); break; case 'configure:label:created': - ctx.info(` ✓ Label "${event.name}" created`); + ctx.info(` ${ansiGreen('✓')} Label ${ansiHighlight(`\`${event.name}\``)} created`); break; case 'configure:label:exists': - ctx.warn(` ⊘ Label "${event.name}" already exists`); + ctx.warn(` ${ansiYellow('⊘')} Label ${ansiHighlight(`\`${event.name}\``)} already exists`); break; case 'configure:milestone:created': - ctx.info(` ✓ Milestone "${event.name}" created`); + ctx.info(` ${ansiGreen('✓')} Milestone ${ansiHighlight(`\`${event.name}\``)} created`); break; case 'configure:milestone:exists': - ctx.warn(` ⊘ Milestone "${event.name}" already exists`); + ctx.warn(` ${ansiYellow('⊘')} Milestone ${ansiHighlight(`\`${event.name}\``)} already exists`); break; case 'configure:secret:uploading': ctx.startSpinner(`Uploading secret ${event.name}…`); break; case 'configure:secret:uploaded': - ctx.stopSpinner(`Secret ${event.name} uploaded`); + ctx.stopSpinner(`Secret ${event.name} uploaded ${ansiGreen('✓')}`); break; case 'configure:done': ctx.success('Configuration complete'); diff --git a/packages/fleet/src/shared/ui/render/dispatch.ts b/packages/fleet/src/shared/ui/render/dispatch.ts index 23ce9f47..1904b7b4 100644 --- a/packages/fleet/src/shared/ui/render/dispatch.ts +++ b/packages/fleet/src/shared/ui/render/dispatch.ts @@ -14,29 +14,46 @@ import type { DispatchEvent } from '../../events/dispatch.js'; import type { RenderContext } from '../spec.js'; -import { sessionUrl } from '../session-url.js'; +import { + sessionUrl, + ansiLink, + ansiYellow, + ansiHighlight, + ansiGreen, +} from '../session-url.js'; /** Render a dispatch-domain event. */ -export function renderDispatchEvent(event: DispatchEvent, ctx: RenderContext): void { +export function renderDispatchEvent( + event: DispatchEvent, + ctx: RenderContext, +): void { switch (event.type) { case 'dispatch:start': - ctx.info(`Dispatching from milestone ${event.milestone}`); + ctx.info( + `Dispatching from milestone ${ansiHighlight(`\`${event.milestone}\``)}`, + ); break; case 'dispatch:scanning': ctx.startSpinner('Scanning for fleet issues…'); break; case 'dispatch:found': - ctx.stopSpinner(`Found ${event.count} undispatched issue(s)`); + ctx.stopSpinner( + `Found ${event.count} undispatched issue(s) ${ansiGreen('✓')}`, + ); break; case 'dispatch:issue:dispatching': ctx.startSpinner(`#${event.number}: ${event.title}`); break; case 'dispatch:issue:dispatched': - ctx.stopSpinner(`#${event.number} → session ${event.sessionId}`); - ctx.info(` ${sessionUrl(event.sessionId)}`); + ctx.stopSpinner( + `#${event.number} → session ${ansiHighlight(`\`${event.sessionId}\``)} ${ansiGreen('✓')}`, + ); + ctx.info(` ${ansiLink('View Session', sessionUrl(event.sessionId))}`); break; case 'dispatch:issue:skipped': - ctx.warn(` ⊘ #${event.number}: ${event.reason}`); + ctx.warn( + ` ${ansiYellow('⊘')} #${event.number}: ${ansiHighlight(event.reason)}`, + ); break; case 'dispatch:done': ctx.success( diff --git a/packages/fleet/src/shared/ui/render/error.ts b/packages/fleet/src/shared/ui/render/error.ts index 78a10d44..fde09c96 100644 --- a/packages/fleet/src/shared/ui/render/error.ts +++ b/packages/fleet/src/shared/ui/render/error.ts @@ -14,10 +14,11 @@ import type { ErrorEvent } from '../../events/error.js'; import type { RenderContext } from '../spec.js'; +import { ansiHighlight, ansiRed } from '../session-url.js'; /** Render an error event. */ export function renderErrorEvent(event: ErrorEvent, ctx: RenderContext): void { ctx.stopSpinner(); - ctx.error(`[${event.code}] ${event.message}`); - if (event.suggestion) ctx.info(` 💡 ${event.suggestion}`); + ctx.error(` ${ansiRed('✗')} [${event.code}] ${ansiHighlight(event.message)}`); + if (event.suggestion) ctx.info(` 💡 ${ansiHighlight(event.suggestion)}`); } diff --git a/packages/fleet/src/shared/ui/render/init.ts b/packages/fleet/src/shared/ui/render/init.ts index e9c10f5b..bc278528 100644 --- a/packages/fleet/src/shared/ui/render/init.ts +++ b/packages/fleet/src/shared/ui/render/init.ts @@ -14,46 +14,67 @@ import type { InitEvent } from '../../events/init.js'; import type { RenderContext } from '../spec.js'; +import { + ansiLink, + ansiGreen, + ansiYellow, + ansiRed, + ansiHighlight, +} from '../session-url.js'; /** Render an init-domain event. */ export function renderInitEvent(event: InitEvent, ctx: RenderContext): void { switch (event.type) { case 'init:start': - ctx.info(`Initializing fleet for ${event.owner}/${event.repo}`); + ctx.info( + `Initializing fleet for ${ansiHighlight(`\`${event.owner}/${event.repo}\``)}`, + ); break; case 'init:branch:creating': - ctx.startSpinner(`Creating branch ${event.name} from ${event.base}`); + ctx.startSpinner( + `Creating branch ${ansiHighlight(`\`${event.name}\``)} from ${ansiHighlight(`\`${event.base}\``)}`, + ); break; case 'init:branch:created': - ctx.stopSpinner(`Branch ${event.name} created`); + ctx.stopSpinner( + `Branch ${ansiHighlight(`\`${event.name}\``)} created ${ansiGreen('✓')}`, + ); break; case 'init:file:committed': - ctx.info(` ✓ ${event.path}`); + ctx.info(` ${ansiGreen('✓')} ${ansiHighlight(`\`${event.path}\``)}`); break; case 'init:file:skipped': - ctx.warn(` ⊘ ${event.path} — ${event.reason}`); + ctx.warn( + ` ${ansiYellow('⊘')} ${ansiHighlight(`\`${event.path}\``)} — ${ansiHighlight(event.reason)}`, + ); break; case 'init:pr:creating': ctx.startSpinner('Creating pull request…'); break; case 'init:pr:created': - ctx.stopSpinner(`PR #${event.number} created`); - ctx.info(` ${event.url}`); + ctx.stopSpinner(`PR #${event.number} created ${ansiGreen('✓')}`); + ctx.info(` ${ansiLink('View Pull Request', event.url)}`); break; case 'init:done': - ctx.success(`Fleet initialized — PR: ${event.prUrl}`); + ctx.success( + `Fleet initialized — PR: ${ansiLink('View Pull Request', event.prUrl)}`, + ); break; case 'init:auth:detected': - ctx.success(`Auth: ${event.method === 'token' ? 'GITHUB_TOKEN' : 'GitHub App'}`); + ctx.success( + `Auth: ${event.method === 'token' ? 'GITHUB_TOKEN' : 'GitHub App'}`, + ); break; case 'init:secret:uploading': ctx.startSpinner(`Uploading secret ${event.name}…`); break; case 'init:secret:uploaded': - ctx.stopSpinner(`Secret ${event.name} saved`); + ctx.stopSpinner(`Secret ${event.name} saved ${ansiGreen('✓')}`); break; case 'init:secret:skipped': - ctx.warn(` ⊘ ${event.name} — ${event.reason}`); + ctx.warn( + ` ${ansiYellow('⊘')} ${event.name} — ${ansiHighlight(event.reason)}`, + ); break; case 'init:dry-run': ctx.info('Would create:'); @@ -62,5 +83,27 @@ export function renderInitEvent(event: InitEvent, ctx: RenderContext): void { case 'init:already-initialized': ctx.warn('Repository is already initialized'); break; + case 'init:repo:creating': + ctx.startSpinner( + `Creating repository ${ansiHighlight(`\`${event.owner}/${event.name}\``)}…`, + ); + break; + case 'init:repo:created': + ctx.stopSpinner( + `Repository ${ansiHighlight(`\`${event.fullName}\``)} created ${ansiGreen('✓')}`, + ); + ctx.info(` ${ansiLink('View Repository', event.url)}`); + break; + case 'init:repo:exists': + ctx.warn( + ` ${ansiYellow('⊘')} Repository ${ansiHighlight(`\`${event.fullName}\``)} already exists`, + ); + break; + case 'init:repo:failed': + ctx.stopSpinner(); + ctx.error( + ` ${ansiRed('✗')} Repository creation failed: ${ansiHighlight(event.reason)}`, + ); + break; } } diff --git a/packages/fleet/src/shared/ui/render/merge.ts b/packages/fleet/src/shared/ui/render/merge.ts index ee850f3f..f5e1ec90 100644 --- a/packages/fleet/src/shared/ui/render/merge.ts +++ b/packages/fleet/src/shared/ui/render/merge.ts @@ -14,13 +14,22 @@ import type { MergeEvent } from '../../events/merge.js'; import type { RenderContext } from '../spec.js'; +import { + sessionUrl, + ansiLink, + ansiGreen, + ansiRed, + ansiYellow, + ansiDim, + ansiHighlight, +} from '../session-url.js'; /** Render a merge-domain event. */ export function renderMergeEvent(event: MergeEvent, ctx: RenderContext): void { switch (event.type) { case 'merge:start': ctx.info( - `Merging ${event.prCount} PR(s) in ${event.owner}/${event.repo} [${event.mode}]`, + `Merging ${event.prCount} PR(s) in ${ansiHighlight(`\`${event.owner}/${event.repo}\``)} [${event.mode}]`, ); break; case 'merge:no-prs': @@ -35,62 +44,99 @@ export function renderMergeEvent(event: MergeEvent, ctx: RenderContext): void { ctx.startSpinner(`Updating branch for PR #${event.prNumber}…`); break; case 'merge:branch:updated': - ctx.stopSpinner(`Branch updated for PR #${event.prNumber}`); + ctx.stopSpinner( + `Branch updated for PR #${event.prNumber} ${ansiGreen('✓')}`, + ); break; case 'merge:ci:waiting': ctx.startSpinner(`Waiting for CI on PR #${event.prNumber}…`); break; case 'merge:ci:check': { - const icon = event.status === 'pass' ? '✓' : event.status === 'fail' ? '✗' : '…'; - const dur = event.duration ? ` (${event.duration}s)` : ''; - ctx.info(` ${icon} ${event.name}${dur}`); + const icon = + event.status === 'pass' + ? ansiGreen('✓') + : event.status === 'fail' + ? ansiRed('✗') + : ansiYellow('…'); + const name = event.status === 'fail' ? ansiRed(event.name) : event.name; + const dur = event.duration ? ansiDim(` (${event.duration}s)`) : ''; + ctx.info(` ${icon} ${name}${dur}`); break; } case 'merge:ci:passed': - ctx.stopSpinner(`CI passed for PR #${event.prNumber}`); + ctx.stopSpinner(`CI passed for PR #${event.prNumber} ${ansiGreen('✓')}`); break; case 'merge:ci:failed': - ctx.stopSpinner(`CI failed for PR #${event.prNumber}`); + ctx.stopSpinner(`CI failed for PR #${event.prNumber} ${ansiRed('✗')}`); break; case 'merge:ci:timeout': - ctx.stopSpinner(`CI timed out for PR #${event.prNumber}`); + ctx.stopSpinner( + `CI timed out for PR #${event.prNumber} ${ansiYellow('⊘')}`, + ); break; case 'merge:ci:none': - ctx.stopSpinner(`No CI checks for PR #${event.prNumber}`); + ctx.stopSpinner( + `No CI checks for PR #${event.prNumber} ${ansiYellow('⊘')}`, + ); break; case 'merge:pr:merging': ctx.startSpinner(`Merging PR #${event.prNumber}…`); break; case 'merge:pr:merged': - ctx.stopSpinner(`PR #${event.prNumber} merged ✓`); + ctx.stopSpinner(`PR #${event.prNumber} merged ${ansiGreen('✓')}`); break; case 'merge:pr:skipped': - ctx.warn(` ⊘ PR #${event.prNumber}: ${event.reason}`); + ctx.warn(` ${ansiYellow('⊘')} PR #${event.prNumber}: ${ansiHighlight(event.reason)}`); break; case 'merge:conflict:detected': - ctx.stopSpinner(`Conflict detected on PR #${event.prNumber}`); + ctx.stopSpinner( + `Conflict detected on PR #${event.prNumber} ${ansiRed('✗')}`, + ); break; case 'merge:conflict:escalated': - ctx.info(` ↳ Escalated PR #${event.prNumber} → session ${event.sessionId} (${event.failureCount} consecutive failures)`); + ctx.info( + ` ↳ Escalated PR #${event.prNumber} → session ${ansiHighlight(`\`${event.sessionId}\``)} (${event.failureCount} consecutive failures)`, + ); + ctx.info(` ${ansiLink('View Session', sessionUrl(event.sessionId))}`); + break; + case 'merge:conflict:notifying': + ctx.startSpinner( + `Notifying session ${ansiHighlight(`\`${event.sessionId}\``)} of conflict on PR #${event.prNumber}…`, + ); + break; + case 'merge:conflict:notified': + ctx.stopSpinner( + `Notified session ${ansiHighlight(`\`${event.sessionId}\``)} of conflict on PR #${event.prNumber} ${ansiGreen('✓')}`, + ); + ctx.info(` ${ansiLink('View Session', sessionUrl(event.sessionId))}`); break; case 'merge:plan:computed': { - const groupDesc = event.conflictGroups.length > 0 - ? `, ${event.conflictGroups.length} conflict group(s)` - : ''; + const groupDesc = + event.conflictGroups.length > 0 + ? `, ${event.conflictGroups.length} conflict group(s)` + : ''; ctx.info(`Plan: ${event.independent.length} independent${groupDesc}`); break; } case 'merge:batch-resolve:start': - ctx.startSpinner(`Batch resolving ${event.prNumbers.map(n => `#${n}`).join(', ')}…`); + ctx.startSpinner( + `Batch resolving ${event.prNumbers.map((n) => `#${n}`).join(', ')}…`, + ); break; case 'merge:batch-resolve:done': - ctx.stopSpinner(`Batch resolved ${event.prNumbers.map(n => `#${n}`).join(', ')} → session ${event.sessionId}`); + ctx.stopSpinner( + `Batch resolved ${event.prNumbers.map((n) => `#${n}`).join(', ')} → session ${ansiHighlight(`\`${event.sessionId}\``)} ${ansiGreen('✓')}`, + ); + ctx.info(` ${ansiLink('View Session', sessionUrl(event.sessionId))}`); break; case 'merge:redispatch:start': ctx.startSpinner(`Re-dispatching PR #${event.oldPr}…`); break; case 'merge:redispatch:done': - ctx.stopSpinner(`Re-dispatched PR #${event.oldPr} → session ${event.sessionId}`); + ctx.stopSpinner( + `Re-dispatched PR #${event.oldPr} → session ${ansiHighlight(`\`${event.sessionId}\``)} ${ansiGreen('✓')}`, + ); + ctx.info(` ${ansiLink('View Session', sessionUrl(event.sessionId))}`); break; case 'merge:done': ctx.success( diff --git a/packages/fleet/src/shared/ui/session-url.ts b/packages/fleet/src/shared/ui/session-url.ts index f48723dd..3add4e63 100644 --- a/packages/fleet/src/shared/ui/session-url.ts +++ b/packages/fleet/src/shared/ui/session-url.ts @@ -30,8 +30,62 @@ export function repoConfigUrl(owner: string, repo: string): string { /** * Wrap text in an ANSI hyperlink (OSC 8) for terminals that support it. - * Falls back to plain text in terminals that don't. + * Falls back to plain text in terminals that don't (e.g. non-TTY or CI). + * + * Visually styles the link text with a cyan color and underline in interactive + * TTY environments to make it highly discoverable and user-friendly. */ export function ansiLink(text: string, url: string): string { - return `\x1b]8;;${url}\x07${text}\x1b]8;;\x07`; + const isInteractive = process.env.CI !== 'true' && !!process.stdout.isTTY; + if (!isInteractive) { + return text === url ? url : `${text} (${url})`; + } + // Underline (\x1b[4m) and color the link cyan (\x1b[36m) to make terminal hyperlinks visually discoverable as clickable elements. + // Reset underline (\x1b[24m) and color (\x1b[39m) inside the OSC 8 markers. + const styledText = `\x1b[36m\x1b[4m${text}\x1b[24m\x1b[39m`; + return `\x1b]8;;${url}\x07${styledText}\x1b]8;;\x07`; +} + +/** + * Apply green color to text if terminal is interactive. + */ +export function ansiGreen(text: string): string { + const isInteractive = process.env.CI !== 'true' && !!process.stdout.isTTY; + return isInteractive ? `\x1b[32m${text}\x1b[39m` : text; +} + +/** + * Apply red color to text if terminal is interactive. + */ +export function ansiRed(text: string): string { + const isInteractive = process.env.CI !== 'true' && !!process.stdout.isTTY; + return isInteractive ? `\x1b[31m${text}\x1b[39m` : text; +} + +/** + * Apply yellow color to text if terminal is interactive. + */ +export function ansiYellow(text: string): string { + const isInteractive = process.env.CI !== 'true' && !!process.stdout.isTTY; + return isInteractive ? `\x1b[33m${text}\x1b[39m` : text; +} + +/** + * Apply dim style to text if terminal is interactive. + */ +export function ansiDim(text: string): string { + const isInteractive = process.env.CI !== 'true' && !!process.stdout.isTTY; + return isInteractive ? `\x1b[2m${text}\x1b[22m` : text; +} + +/** + * Style backticked terms (like `some-command`) in yellow if terminal is interactive, + * to make commands and key terms easily discoverable in CLI outputs. + */ +export function ansiHighlight(text: string): string { + const isInteractive = process.env.CI !== 'true' && !!process.stdout.isTTY; + if (!isInteractive) { + return text; + } + return text.replace(/`([^`]+)`/g, '`\x1b[33m$1\x1b[39m`'); } diff --git a/packages/mcp/src/functions/code-review.ts b/packages/mcp/src/functions/code-review.ts index 30baee33..c75879e1 100644 --- a/packages/mcp/src/functions/code-review.ts +++ b/packages/mcp/src/functions/code-review.ts @@ -1,7 +1,9 @@ -import type { - JulesClient, - ChangeSetArtifact, - Activity, +import { + type JulesClient, + type ChangeSetArtifact, + type Activity, + validateSessionId, + validateActivityId, } from '@google/jules-sdk'; import type { ReviewChangesResult, @@ -331,6 +333,7 @@ export async function codeReview( if (!sessionId) { throw new Error('sessionId is required'); } + validateSessionId(sessionId); const { format = 'summary', @@ -353,6 +356,7 @@ export async function codeReview( // Find specific activity if activityId provided let targetActivity: Activity | undefined; if (activityId) { + validateActivityId(activityId); targetActivity = activities.find((a) => a.id === activityId); if (!targetActivity) { throw new Error( diff --git a/packages/mcp/src/functions/create-session.ts b/packages/mcp/src/functions/create-session.ts index 4d899af9..49bf2d06 100644 --- a/packages/mcp/src/functions/create-session.ts +++ b/packages/mcp/src/functions/create-session.ts @@ -1,4 +1,9 @@ -import type { JulesClient, SessionConfig } from '@google/jules-sdk'; +import { + type JulesClient, + type SessionConfig, + validateRepository, + validateBranchName, +} from '@google/jules-sdk'; import type { CreateSessionResult, CreateSessionOptions } from './types.js'; /** @@ -12,6 +17,14 @@ export async function createSession( client: JulesClient, options: CreateSessionOptions, ): Promise { + // Validate inputs at the MCP function boundary (defense-in-depth) + if (options.repo) { + validateRepository(options.repo); + } + if (options.branch) { + validateBranchName(options.branch); + } + // Build config - source is optional for repoless sessions const config: SessionConfig = { prompt: options.prompt, diff --git a/packages/mcp/src/functions/interact.ts b/packages/mcp/src/functions/interact.ts index c11a7a4b..8e8396e8 100644 --- a/packages/mcp/src/functions/interact.ts +++ b/packages/mcp/src/functions/interact.ts @@ -1,4 +1,4 @@ -import type { JulesClient } from '@google/jules-sdk'; +import { type JulesClient, validateSessionId } from '@google/jules-sdk'; import type { InteractResult, InteractAction } from './types.js'; /** @@ -19,6 +19,7 @@ export async function interact( if (!sessionId) { throw new Error('sessionId is required'); } + validateSessionId(sessionId); const session = client.session(sessionId); diff --git a/packages/mcp/src/functions/list-sessions.ts b/packages/mcp/src/functions/list-sessions.ts index 4bdb3432..2235b8df 100644 --- a/packages/mcp/src/functions/list-sessions.ts +++ b/packages/mcp/src/functions/list-sessions.ts @@ -1,4 +1,4 @@ -import type { JulesClient } from '@google/jules-sdk'; +import { type JulesClient, validatePageToken } from '@google/jules-sdk'; import type { ListSessionsResult, ListSessionsOptions } from './types.js'; /** @@ -12,6 +12,10 @@ export async function listSessions( client: JulesClient, options: ListSessionsOptions = {}, ): Promise { + if (options.pageToken) { + validatePageToken(options.pageToken); + } + const cursor = client.sessions({ pageSize: options.pageSize || 10, pageToken: options.pageToken, diff --git a/packages/mcp/src/functions/select.ts b/packages/mcp/src/functions/select.ts index 8a9e9f45..7bf0b5a2 100644 --- a/packages/mcp/src/functions/select.ts +++ b/packages/mcp/src/functions/select.ts @@ -1,8 +1,9 @@ -import type { - JulesClient, - JulesQuery, - JulesDomain, - Activity, +import { + type JulesClient, + type JulesQuery, + type JulesDomain, + type Activity, + validateQuery, } from '@google/jules-sdk'; import type { SelectResult, SelectOptions } from './types.js'; import { truncateToTokenBudget } from '../tokenizer.js'; @@ -25,6 +26,13 @@ export async function select( throw new Error('Query argument is required'); } + // Validate JQL query structure at the MCP boundary (defense-in-depth) + const validationResult = validateQuery(query); + if (!validationResult.valid) { + const messages = validationResult.errors.map((e) => e.message).join('; '); + throw new Error(`INVALID_QUERY: ${messages}`); + } + const { tokenBudget } = options; let results: unknown[] = await client.select(query); let truncated = false; diff --git a/packages/mcp/src/functions/session-state.ts b/packages/mcp/src/functions/session-state.ts index cac4ffea..33398f8e 100644 --- a/packages/mcp/src/functions/session-state.ts +++ b/packages/mcp/src/functions/session-state.ts @@ -1,4 +1,4 @@ -import type { JulesClient, Activity } from '@google/jules-sdk'; +import { type JulesClient, type Activity, validateSessionId } from '@google/jules-sdk'; import type { SessionStateResult, SessionStatus, @@ -146,6 +146,7 @@ export async function getSessionState( if (!sessionId) { throw new Error('sessionId is required'); } + validateSessionId(sessionId); const session = client.session(sessionId); diff --git a/packages/mcp/src/functions/show-diff.ts b/packages/mcp/src/functions/show-diff.ts index 7613955d..eb702da1 100644 --- a/packages/mcp/src/functions/show-diff.ts +++ b/packages/mcp/src/functions/show-diff.ts @@ -1,4 +1,10 @@ -import type { JulesClient, ChangeSetArtifact } from '@google/jules-sdk'; +import { + type JulesClient, + type ChangeSetArtifact, + validateSessionId, + validateFilePath, + validateActivityId, +} from '@google/jules-sdk'; import type { ShowDiffResult, ShowDiffOptions, @@ -40,8 +46,15 @@ export async function showDiff( if (!sessionId) { throw new Error('sessionId is required'); } + validateSessionId(sessionId); const { file, activityId } = options; + if (file) { + validateFilePath(file); + } + if (activityId) { + validateActivityId(activityId); + } // Use snapshot() to leverage core SDK aggregation const session = client.session(sessionId); diff --git a/packages/mcp/src/tools/list-sessions.tool.ts b/packages/mcp/src/tools/list-sessions.tool.ts index 80315969..03da14fc 100644 --- a/packages/mcp/src/tools/list-sessions.tool.ts +++ b/packages/mcp/src/tools/list-sessions.tool.ts @@ -8,7 +8,11 @@ export default defineTool({ inputSchema: { type: 'object', properties: { - pageSize: { type: 'number' }, + pageSize: { + type: 'number', + description: + 'The maximum number of recent sessions to retrieve (default: 10).', + }, }, }, handler: async (client: JulesClient, args: any) => { diff --git a/packages/mcp/src/tools/send-reply.tool.ts b/packages/mcp/src/tools/send-reply.tool.ts index 9890a23c..9c1846e8 100644 --- a/packages/mcp/src/tools/send-reply.tool.ts +++ b/packages/mcp/src/tools/send-reply.tool.ts @@ -9,7 +9,11 @@ export default defineTool({ inputSchema: { type: 'object', properties: { - sessionId: { type: 'string' }, + sessionId: { + type: 'string', + description: + 'The Jules session ID to reply or interact with (numeric string).', + }, action: { type: 'string', enum: ['approve', 'send', 'ask'], diff --git a/packages/mcp/tests/functions/create-session.test.ts b/packages/mcp/tests/functions/create-session.test.ts index 30846ee8..341ad0b4 100644 --- a/packages/mcp/tests/functions/create-session.test.ts +++ b/packages/mcp/tests/functions/create-session.test.ts @@ -114,4 +114,24 @@ describe('createSession', () => { expect(capturedConfig.source).toBeUndefined(); }); + + describe('input validation', () => { + it('throws validation error if repo is invalid', async () => { + await expect( + createSession(mockClient, { + prompt: 'Fix the bug', + repo: '../malicious-repo', + }), + ).rejects.toThrow('Repository name cannot contain path traversal segments'); + }); + + it('throws validation error if branch name is invalid', async () => { + await expect( + createSession(mockClient, { + prompt: 'Fix the bug', + branch: 'main..branch', + }), + ).rejects.toThrow('Branch name contains consecutive dots'); + }); + }); }); diff --git a/packages/mcp/tests/functions/list-sessions.test.ts b/packages/mcp/tests/functions/list-sessions.test.ts new file mode 100644 index 00000000..d4258b82 --- /dev/null +++ b/packages/mcp/tests/functions/list-sessions.test.ts @@ -0,0 +1,51 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { listSessions } from '../../src/functions/list-sessions.js'; +import { createMockClient } from './helpers.js'; + +describe('listSessions', () => { + let mockClient: ReturnType; + + beforeEach(() => { + mockClient = createMockClient(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('allows standard page token', async () => { + const sessionsSpy = vi.spyOn(mockClient, 'sessions').mockReturnValue({ + then: (resolve: any) => resolve({ sessions: [], nextPageToken: undefined }), + } as any); + + const result = await listSessions(mockClient, { + pageToken: '1704448500999999000', + }); + + expect(result.sessions).toEqual([]); + expect(sessionsSpy).toHaveBeenCalledWith({ + pageSize: 10, + pageToken: '1704448500999999000', + }); + }); + + describe('input validation on pageToken', () => { + it('throws validation error if pageToken contains traversal', async () => { + await expect( + listSessions(mockClient, { pageToken: '..' }), + ).rejects.toThrow('PATH_TRAVERSAL'); + }); + + it('throws validation error if pageToken contains slashes', async () => { + await expect( + listSessions(mockClient, { pageToken: 'abc/def' }), + ).rejects.toThrow('INVALID_PAGE_TOKEN'); + }); + + it('throws validation error if pageToken contains control characters', async () => { + await expect( + listSessions(mockClient, { pageToken: 'token\x00' }), + ).rejects.toThrow('CONTROL_CHAR'); + }); + }); +}); diff --git a/packages/mcp/tests/functions/select.test.ts b/packages/mcp/tests/functions/select.test.ts new file mode 100644 index 00000000..6bb4c778 --- /dev/null +++ b/packages/mcp/tests/functions/select.test.ts @@ -0,0 +1,60 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { select } from '../../src/functions/select.js'; +import { createMockClient } from './helpers.js'; +import type { JulesQuery, JulesDomain } from '@google/jules-sdk'; + +describe('MCP select function', () => { + let mockClient: ReturnType; + + beforeEach(() => { + mockClient = createMockClient(); + vi.spyOn(mockClient, 'select').mockResolvedValue([]); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('runs successfully when a valid query is provided', async () => { + const validQuery: JulesQuery = { + from: 'sessions', + limit: 10, + }; + + const result = await select(mockClient, validQuery); + expect(result.results).toEqual([]); + expect(mockClient.select).toHaveBeenCalledWith(validQuery); + }); + + it('throws validation error if query domain (from) is missing', async () => { + const invalidQuery = {} as any; + + await expect(select(mockClient, invalidQuery)).rejects.toThrow( + /INVALID_QUERY: Missing required field: from/i, + ); + expect(mockClient.select).not.toHaveBeenCalled(); + }); + + it('throws validation error if query domain (from) is invalid', async () => { + const invalidQuery = { + from: 'invalid_domain', + } as any; + + await expect(select(mockClient, invalidQuery)).rejects.toThrow( + /INVALID_QUERY: Invalid domain: "invalid_domain"/i, + ); + expect(mockClient.select).not.toHaveBeenCalled(); + }); + + it('throws validation error if limit is negative', async () => { + const invalidQuery: JulesQuery = { + from: 'sessions', + limit: -5, + }; + + await expect(select(mockClient, invalidQuery)).rejects.toThrow( + /INVALID_QUERY:.*limit cannot be negative/i, + ); + expect(mockClient.select).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/mcp/tests/functions/session-state.test.ts b/packages/mcp/tests/functions/session-state.test.ts index bbf10571..fa60810c 100644 --- a/packages/mcp/tests/functions/session-state.test.ts +++ b/packages/mcp/tests/functions/session-state.test.ts @@ -203,4 +203,18 @@ describe('getSessionState', () => { expect(result.lastActivity?.type).toBe('sessionCompleted'); expect(result.lastAgentMessage).toBeUndefined(); }); + + describe('input validation', () => { + it('throws validation error if session ID is invalid (contains traversal)', async () => { + await expect( + getSessionState(mockClient, '../malicious-session'), + ).rejects.toThrow('Session ID cannot contain slashes or backslashes'); + }); + + it('throws validation error if session ID contains control characters', async () => { + await expect( + getSessionState(mockClient, 'session\x00_id'), + ).rejects.toThrow('Session ID contains control characters'); + }); + }); }); diff --git a/packages/mcp/tests/functions/show-diff.test.ts b/packages/mcp/tests/functions/show-diff.test.ts index aebb50ea..cdba06ea 100644 --- a/packages/mcp/tests/functions/show-diff.test.ts +++ b/packages/mcp/tests/functions/show-diff.test.ts @@ -100,4 +100,44 @@ diff --git a/src/b.ts b/src/b.ts 'sessionId is required', ); }); + + describe('input validation on file path', () => { + it('throws validation error if file contains traversal', async () => { + await expect( + showDiff(mockClient, 'some-session', { file: '../../etc/passwd' }), + ).rejects.toThrow('PATH_TRAVERSAL'); + }); + + it('throws validation error if file is an absolute path', async () => { + await expect( + showDiff(mockClient, 'some-session', { file: '/etc/passwd' }), + ).rejects.toThrow('ABSOLUTE_PATH'); + + await expect( + showDiff(mockClient, 'some-session', { file: 'C:/Windows/System32' }), + ).rejects.toThrow('ABSOLUTE_PATH'); + }); + + it('throws validation error if file contains control characters', async () => { + await expect( + showDiff(mockClient, 'some-session', { file: 'src/foo\x00.ts' }), + ).rejects.toThrow('CONTROL_CHAR'); + }); + }); + + describe('input validation on activity ID', () => { + it('throws validation error if activity ID is invalid', async () => { + await expect( + showDiff(mockClient, 'some-session', { activityId: 'act\x00_id' }), + ).rejects.toThrow('CONTROL_CHAR'); + + await expect( + showDiff(mockClient, 'some-session', { activityId: 'act/123' }), + ).rejects.toThrow('INVALID_ACTIVITY_ID'); + + await expect( + showDiff(mockClient, 'some-session', { activityId: '..' }), + ).rejects.toThrow('PATH_TRAVERSAL'); + }); + }); }); diff --git a/packages/merge/src/__tests__/reconcile/critical-paths.test.ts b/packages/merge/src/__tests__/reconcile/critical-paths.test.ts index 4dc7e0e5..abfea93a 100644 --- a/packages/merge/src/__tests__/reconcile/critical-paths.test.ts +++ b/packages/merge/src/__tests__/reconcile/critical-paths.test.ts @@ -115,11 +115,96 @@ describe('critical-paths', () => { expect(() => validateFilePath('../etc/passwd')).toThrow('PATH_TRAVERSAL'); }); + it('rejects file paths with absolute paths', async () => { + const { validateFilePath } = await import('../../shared/validators.js'); + expect(() => validateFilePath('/etc/passwd')).toThrow('ABSOLUTE_PATH'); + expect(() => validateFilePath('C:/Windows/System32')).toThrow( + 'ABSOLUTE_PATH', + ); + }); + + it('stageResolutionHandler rejects path traversal in fromFile', async () => { + const { scanHandler } = await import('../../reconcile/scan-handler.js'); + const { stageResolutionHandler } = await import( + '../../reconcile/stage-resolution-handler.js' + ); + + await scanHandler({} as any, { + prs: [10, 11], + repo: 'owner/repo', + }); + + await expect( + stageResolutionHandler({ + filePath: 'src/config.ts', + parents: ['main', '10', '11'], + fromFile: '../../etc/passwd', + }), + ).rejects.toThrow('PATH_TRAVERSAL'); + + await expect( + stageResolutionHandler({ + filePath: 'src/config.ts', + parents: ['main', '10', '11'], + fromFile: '/etc/passwd', + }), + ).rejects.toThrow('ABSOLUTE_PATH'); + }); + + it('stageResolutionHandler rejects invalid parent references', async () => { + const { scanHandler } = await import('../../reconcile/scan-handler.js'); + const { stageResolutionHandler } = await import( + '../../reconcile/stage-resolution-handler.js' + ); + + await scanHandler({} as any, { + prs: [10, 11], + repo: 'owner/repo', + }); + + await expect( + stageResolutionHandler({ + filePath: 'src/config.ts', + parents: ['refs/heads/main', '10'], + content: 'resolved content', + }), + ).rejects.toThrow('RESERVED_BRANCH'); + + await expect( + stageResolutionHandler({ + filePath: 'src/config.ts', + parents: ['main space', '10'], + content: 'resolved content', + }), + ).rejects.toThrow('Branch name contains spaces'); + + await expect( + stageResolutionHandler({ + filePath: 'src/config.ts', + parents: ['main..branch', '10'], + content: 'resolved content', + }), + ).rejects.toThrow('Branch name contains consecutive dots'); + + await expect( + stageResolutionHandler({ + filePath: 'src/config.ts', + parents: ['main.lock', '10'], + content: 'resolved content', + }), + ).rejects.toThrow('INVALID_BRANCH'); + }); + it('rejects file paths with control characters', async () => { const { validateFilePath } = await import('../../shared/validators.js'); expect(() => validateFilePath('src/foo\x00.ts')).toThrow('CONTROL_CHAR'); }); + it('rejects empty or missing branch names', async () => { + const { validateBranchName } = await import('../../shared/validators.js'); + expect(() => validateBranchName('')).toThrow('INVALID_BRANCH'); + }); + it('rejects branch names starting with refs/', async () => { const { validateBranchName } = await import('../../shared/validators.js'); expect(() => validateBranchName('refs/heads/main')).toThrow( @@ -134,6 +219,97 @@ describe('critical-paths', () => { ); }); + it('rejects empty or missing file paths', async () => { + const { validateFilePath } = await import('../../shared/validators.js'); + expect(() => validateFilePath('')).toThrow('INVALID_FILE_PATH'); + }); + + // ─── Repository Validation Tests ───────────────────────────── + + it('accepts valid repository names', async () => { + const { validateRepository } = await import('../../shared/validators.js'); + expect(() => validateRepository('owner/repo')).not.toThrow(); + expect(() => validateRepository('google/jules-sdk')).not.toThrow(); + expect(() => validateRepository('owner-name/repo_name.js')).not.toThrow(); + }); + + it('rejects empty or missing repository names', async () => { + const { validateRepository } = await import('../../shared/validators.js'); + expect(() => validateRepository('')).toThrow('INVALID_REPOSITORY'); + }); + + it('rejects invalid repository formats (no slash or too many slashes)', async () => { + const { validateRepository } = await import('../../shared/validators.js'); + expect(() => validateRepository('owner')).toThrow('INVALID_REPOSITORY'); + expect(() => validateRepository('owner/repo/extra')).toThrow('INVALID_REPOSITORY'); + expect(() => validateRepository('owner//repo')).toThrow('INVALID_REPOSITORY'); + expect(() => validateRepository('/owner/repo')).toThrow('INVALID_REPOSITORY'); + expect(() => validateRepository('owner/repo/')).toThrow('INVALID_REPOSITORY'); + }); + + it('rejects repository names with invalid special characters', async () => { + const { validateRepository } = await import('../../shared/validators.js'); + expect(() => validateRepository('owner$/repo')).toThrow('INVALID_REPOSITORY'); + expect(() => validateRepository('owner/re@po')).toThrow('INVALID_REPOSITORY'); + expect(() => validateRepository('owner:/repo')).toThrow('INVALID_REPOSITORY'); + }); + + it('rejects repository names with control characters', async () => { + const { validateRepository } = await import('../../shared/validators.js'); + expect(() => validateRepository('owner\x00/repo')).toThrow('CONTROL_CHAR'); + expect(() => validateRepository('owner/re\x1fpo')).toThrow('CONTROL_CHAR'); + }); + + it('rejects repository names with path traversal', async () => { + const { validateRepository } = await import('../../shared/validators.js'); + expect(() => validateRepository('../repo')).toThrow('PATH_TRAVERSAL'); + expect(() => validateRepository('owner/..')).toThrow('PATH_TRAVERSAL'); + expect(() => validateRepository('../owner/repo')).toThrow('INVALID_REPOSITORY'); + expect(() => validateRepository('owner/../repo')).toThrow('INVALID_REPOSITORY'); + expect(() => validateRepository('./repo')).toThrow('PATH_TRAVERSAL'); + expect(() => validateRepository('owner/.')).toThrow('PATH_TRAVERSAL'); + }); + + // ─── Handler Repository Validation Tests ───────────────────── + + it('scanHandler rejects invalid repository names', async () => { + const { scanHandler } = await import('../../reconcile/scan-handler.js'); + await expect( + scanHandler({} as any, { prs: [1], repo: 'owner/re@po' }), + ).rejects.toThrow(); + }); + + it('scanHandler rejects invalid base branch names', async () => { + const { scanHandler } = await import('../../reconcile/scan-handler.js'); + await expect( + scanHandler({} as any, { prs: [1], repo: 'owner/repo', base: 'invalid branch' }), + ).rejects.toThrow(); + await expect( + scanHandler({} as any, { prs: [1], repo: 'owner/repo', base: 'refs/heads/main' }), + ).rejects.toThrow(); + }); + + it('getContentsHandler rejects invalid repository names', async () => { + const { getContentsHandler } = await import('../../reconcile/get-contents-handler.js'); + await expect( + getContentsHandler({} as any, { filePath: 'src/config.ts', source: 'main', repo: 'owner/re@po' }), + ).rejects.toThrow(); + }); + + it('mergeHandler rejects invalid repository names', async () => { + const { mergeHandler } = await import('../../reconcile/merge-handler.js'); + await expect( + mergeHandler({} as any, { pr: 1, repo: 'owner/re@po' }), + ).rejects.toThrow(); + }); + + it('pushHandler rejects invalid repository names', async () => { + const { pushHandler } = await import('../../reconcile/push-handler.js'); + await expect( + pushHandler({} as any, { branch: 'reconcile/test', message: 'test', repo: 'owner/re@po' }), + ).rejects.toThrow(); + }); + // ─── 4. dry-run behavior ────────────────────────────────────── it('stage-resolution --dry-run does not modify manifest', async () => { @@ -372,7 +548,9 @@ describe('critical-paths', () => { // ─── 12. Error types & exit codes ───────────────────────────── it('ConflictError has exit code 1', async () => { - const { ConflictError, getExitCode } = await import('../../shared/errors.js'); + const { ConflictError, getExitCode } = await import( + '../../shared/errors.js' + ); const err = new ConflictError('test'); expect(err.exitCode).toBe(1); expect(getExitCode(err)).toBe(1); diff --git a/packages/merge/src/__tests__/reconcile/scan-discovery.test.ts b/packages/merge/src/__tests__/reconcile/scan-discovery.test.ts index 6c7075de..e7eed70d 100644 --- a/packages/merge/src/__tests__/reconcile/scan-discovery.test.ts +++ b/packages/merge/src/__tests__/reconcile/scan-discovery.test.ts @@ -150,6 +150,25 @@ describe('scan --all discovery', () => { expect(result.prs).toHaveLength(5); }); + it('B5: rejects scan with malformed base branch name to prevent command/reference injection', async () => { + const { scanHandler } = await import('../../reconcile/scan-handler.js'); + await expect( + scanHandler({} as any, { + all: true, + repo: 'owner/repo', + base: 'refs/heads/main', // starts with refs/ + }), + ).rejects.toThrow('RESERVED_BRANCH: Branch name must not start with refs/: refs/heads/main'); + + await expect( + scanHandler({} as any, { + all: true, + repo: 'owner/repo', + base: 'bad..branch', // contains consecutive dots + }), + ).rejects.toThrow('INVALID_BRANCH: Branch name contains consecutive dots: bad..branch'); + }); + // ─── Group C: Discovery Happy Paths ─────────────────────────── it('C1: --all discovers open PRs and produces clean result', async () => { diff --git a/packages/merge/src/index.ts b/packages/merge/src/index.ts index 153271e5..3c0e8c43 100644 --- a/packages/merge/src/index.ts +++ b/packages/merge/src/index.ts @@ -15,5 +15,5 @@ export * from './reconcile/index.js'; export { createMergeOctokit, getAuthOptions } from './shared/auth.js'; export { ConflictError, HardError, getExitCode, parseJsonInput } from './shared/errors.js'; -export { validateFilePath, validateBranchName } from './shared/validators.js'; +export { validateFilePath, validateBranchName, validateRepository } from './shared/validators.js'; export * from './shared/github.js'; diff --git a/packages/merge/src/reconcile/get-contents-handler.ts b/packages/merge/src/reconcile/get-contents-handler.ts index 6762d5d3..e72dac99 100644 --- a/packages/merge/src/reconcile/get-contents-handler.ts +++ b/packages/merge/src/reconcile/get-contents-handler.ts @@ -23,11 +23,12 @@ import { getPullRequest, } from '../shared/github.js'; import { readManifest } from './manifest.js'; -import { validateFilePath } from '../shared/validators.js'; +import { validateFilePath, validateRepository } from '../shared/validators.js'; export async function getContentsHandler(octokit: Octokit, rawInput: any) { const input = GetContentsInputSchema.parse(rawInput); validateFilePath(input.filePath); + validateRepository(input.repo); const [owner, repo] = input.repo.split('/'); if (!owner || !repo) { throw new Error('Repo must be in owner/repo format'); diff --git a/packages/merge/src/reconcile/merge-handler.ts b/packages/merge/src/reconcile/merge-handler.ts index 303fa88b..a7df11dc 100644 --- a/packages/merge/src/reconcile/merge-handler.ts +++ b/packages/merge/src/reconcile/merge-handler.ts @@ -16,9 +16,11 @@ import { Octokit } from '@octokit/rest'; import { MergeInputSchema, MergeOutputSchema } from './schemas.js'; import { getPullRequest, mergePullRequest } from '../shared/github.js'; import { HardError } from '../shared/errors.js'; +import { validateRepository } from '../shared/validators.js'; export async function mergeHandler(octokit: Octokit, rawInput: any) { const input = MergeInputSchema.parse(rawInput); + validateRepository(input.repo); const [owner, repo] = input.repo.split('/'); if (!owner || !repo) { throw new Error('Repo must be in owner/repo format'); diff --git a/packages/merge/src/reconcile/push-validate.ts b/packages/merge/src/reconcile/push-validate.ts index 699be608..d7689b07 100644 --- a/packages/merge/src/reconcile/push-validate.ts +++ b/packages/merge/src/reconcile/push-validate.ts @@ -15,7 +15,7 @@ import { Octokit } from '@octokit/rest'; import { PushInputSchema } from './schemas.js'; import { readManifest } from './manifest.js'; -import { validateBranchName } from '../shared/validators.js'; +import { validateBranchName, validateRepository } from '../shared/validators.js'; import { ConflictError, HardError } from '../shared/errors.js'; import * as github from '../shared/github.js'; import type { PushContext } from './push-types.js'; @@ -26,6 +26,7 @@ export async function validatePushInput( ): Promise { const input = PushInputSchema.parse(rawInput); validateBranchName(input.branch); + validateRepository(input.repo); const [owner, repo] = input.repo.split('/'); if (!owner || !repo) { diff --git a/packages/merge/src/reconcile/scan-handler.ts b/packages/merge/src/reconcile/scan-handler.ts index 15232245..7aac7499 100644 --- a/packages/merge/src/reconcile/scan-handler.ts +++ b/packages/merge/src/reconcile/scan-handler.ts @@ -30,9 +30,11 @@ import { writeManifest, type Manifest } from './manifest.js'; import type { ScanContext, ScanOutput } from './scan-types.js'; import { discoverPrs } from './scan-discover.js'; import { classifyFiles } from './scan-classify.js'; +import { validateRepository, validateBranchName } from '../shared/validators.js'; export async function scanHandler(octokit: Octokit, rawInput: unknown) { const input = ScanInputSchema.parse(rawInput); + validateRepository(input.repo); const [owner, repo] = input.repo.split('/'); if (!owner || !repo) { throw new Error('Repo must be in owner/repo format'); @@ -40,6 +42,7 @@ export async function scanHandler(octokit: Octokit, rawInput: unknown) { const baseBranchName = input.base || process.env.JULES_MERGE_BASE_BRANCH || 'main'; + validateBranchName(baseBranchName); const baseBranch = await getBranch(octokit, owner, repo, baseBranchName); const baseSha = baseBranch.commit.sha; diff --git a/packages/merge/src/reconcile/stage-resolution-handler.ts b/packages/merge/src/reconcile/stage-resolution-handler.ts index af3cded4..0dc7fa31 100644 --- a/packages/merge/src/reconcile/stage-resolution-handler.ts +++ b/packages/merge/src/reconcile/stage-resolution-handler.ts @@ -17,7 +17,7 @@ import { StageResolutionOutputSchema, } from './schemas.js'; import { readManifest, writeManifest } from './manifest.js'; -import { validateFilePath } from '../shared/validators.js'; +import { validateBranchName, validateFilePath } from '../shared/validators.js'; import crypto from 'crypto'; import fs from 'fs'; import { z } from 'zod'; @@ -39,13 +39,17 @@ function resolveFileContent(input: StageInput): string { export async function stageResolutionHandler(rawInput: unknown) { const input = StageResolutionInputSchema.parse(rawInput); validateFilePath(input.filePath); + if (input.fromFile) { + validateFilePath(input.fromFile); + } + for (const parent of input.parents) { + validateBranchName(parent); + } const fileContent = resolveFileContent(input); const manifest = readManifest(); if (!manifest) { - throw new Error( - 'No active reconciliation manifest found. Run scan first.', - ); + throw new Error('No active reconciliation manifest found. Run scan first.'); } // Remove from pending diff --git a/packages/merge/src/shared/validators.ts b/packages/merge/src/shared/validators.ts index 5405bf20..5fb509f0 100644 --- a/packages/merge/src/shared/validators.ts +++ b/packages/merge/src/shared/validators.ts @@ -13,31 +13,80 @@ // limitations under the License. export function validateFilePath(filePath: string): void { - if (filePath.includes("\x00") || /[\x01-\x1f\x7f]/.test(filePath)) { + if (!filePath) { + throw new Error('INVALID_FILE_PATH: File path cannot be empty'); + } + if (filePath.includes('\x00') || /[\x01-\x1f\x7f]/.test(filePath)) { throw new Error( `CONTROL_CHAR: File path contains control characters: ${filePath}`, ); } - const normalized = filePath.replace(/\\/g, "/"); - const parts = normalized.split("/"); - if (parts.some((p) => p === "..")) { + const normalized = filePath.replace(/\\/g, '/'); + if (normalized.startsWith('/') || /^[a-zA-Z]:/.test(normalized)) { + throw new Error(`ABSOLUTE_PATH: File path must be relative: ${filePath}`); + } + const parts = normalized.split('/'); + if (parts.some((p) => p === '..')) { + throw new Error(`PATH_TRAVERSAL: File path escapes repo root: ${filePath}`); + } +} + +export function validateRepository(repo: string): void { + if (!repo) { + throw new Error('INVALID_REPOSITORY: Repository cannot be empty'); + } + + if (repo.includes('\x00') || /[\x01-\x1f\x7f]/.test(repo)) { + throw new Error( + `CONTROL_CHAR: Repository contains control characters: ${repo}`, + ); + } + + const parts = repo.split('/'); + if (parts.length !== 2) { + throw new Error( + `INVALID_REPOSITORY: Repository must be in owner/repo format: ${repo}`, + ); + } + + const [owner, repoName] = parts; + if (!owner || !repoName) { + throw new Error( + `INVALID_REPOSITORY: Repository must be in owner/repo format: ${repo}`, + ); + } + + const validNameRegex = /^[a-zA-Z0-9-._]+$/; + if (!validNameRegex.test(owner) || !validNameRegex.test(repoName)) { + throw new Error( + `INVALID_REPOSITORY: Repository name contains invalid characters: ${repo}`, + ); + } + + if ( + owner === '.' || + owner === '..' || + repoName === '.' || + repoName === '..' + ) { throw new Error( - `PATH_TRAVERSAL: File path escapes repo root: ${filePath}`, + `PATH_TRAVERSAL: Repository name cannot contain path traversal segments: ${repo}`, ); } } export function validateBranchName(branch: string): void { - if (branch.startsWith("refs/")) { + if (!branch) { + throw new Error('INVALID_BRANCH: Branch name cannot be empty'); + } + if (branch.startsWith('refs/')) { throw new Error( `RESERVED_BRANCH: Branch name must not start with refs/: ${branch}`, ); } // git ref rules: no spaces, no control chars, no consecutive dots, no trailing dot/slash/lock if (/\s/.test(branch)) { - throw new Error( - `INVALID_BRANCH: Branch name contains spaces: ${branch}`, - ); + throw new Error(`INVALID_BRANCH: Branch name contains spaces: ${branch}`); } if (/[\x00-\x1f\x7f~^:?*\[\\]/.test(branch)) { throw new Error( @@ -55,8 +104,6 @@ export function validateBranchName(branch: string): void { ); } if (/\.lock$/.test(branch)) { - throw new Error( - `INVALID_BRANCH: Branch name ends with .lock: ${branch}`, - ); + throw new Error(`INVALID_BRANCH: Branch name ends with .lock: ${branch}`); } }