packages/web/src/features/search/searchApi.test.ts (1)

4-31: Unused mocks add unnecessary coupling to external modules

This file only uses dateUtils, but still mocks @/lib/serviceError, @/actions, and @/lib/auth. That adds extra surface area that can break these tests if those modules change, without providing value here.

You can either:

• Remove these mocks from this file, or
• Add actual searchApi integration tests that rely on them, so the mocks are justified.

packages/web/src/features/search/dateUtils.test.ts (1)

288-292: Clarify toGitDate behavior for unrecognized formats

The test name and comment say “convert unrecognized format to ISO”, but the assertion expects the original string to be preserved:

// For formats git doesn't natively understand, we convert to ISO
const result = toGitDate('some random string');
expect(result).toBe('some random string');

This is confusing for future readers. Either:

• Update the expectation to actually check for an ISO conversion, if that’s the intended behavior, or
• Rename the test and fix the comment to state that unknown formats are passed through unchanged to Git.

packages/web/src/app/api/(server)/repos/route.ts (1)

5-20: Temporal filters are correctly forwarded; consider normalizing empty query params

The new GET handler cleanly threads activeAfter / activeBefore through to getRepos and preserves the existing error handling pattern.

One minor robustness tweak: searchParams.get(...) ?? undefined will pass through an empty string when the URL contains ?activeAfter= or ?activeBefore=. If you prefer to treat “present but empty” the same as “unset”, you could normalize with:

const activeAfter = searchParams.get("activeAfter") || undefined;
const activeBefore = searchParams.get("activeBefore") || undefined;

This avoids feeding accidental empty strings into downstream date parsing.

packages/web/src/app/api/(server)/commits/route.ts (1)

1-34: Solid validation flow; tighten type usage / schema reuse

The POST handler’s flow (JSON → zod safeParseAsync → schemaValidationError / serviceErrorResponse → searchCommits) looks good and matches the rest of the API.

Two minor nits:

• SearchCommitsRequest is imported but never used. Either remove the import or use it to type the validated payload (e.g., const response = await searchCommits(parsed.data as SearchCommitsRequest);) so this route stays coupled to the shared request type.
• If you already have a shared Zod schema for searchCommits (e.g., alongside other search schemas), consider reusing it here instead of defining a new one inline to avoid future drift between layers.

packages/mcp/src/schemas.ts (1)

195-214: Consider constraining maxCount to avoid pathological requests

The new searchCommitsRequestSchema / commitSchema pair matches the documented shape and looks consistent.

maxCount is currently z.number().optional(), which allows negative values or extremely large limits. Given this is likely passed through to git log (and could be user‑controlled via MCP), you may want to harden it, e.g.:

maxCount: z
  .number()
  .int()
  .positive()
  .max(500)
  .optional(),

(or whatever upper bound matches your performance expectations).

packages/web/src/features/search/dateUtils.ts (1)

6-26: Clarify parseTemporalDate contract and clean up unused state

The JSDoc and implementation for parseTemporalDate don’t currently line up:

• Doc says it “Returns an ISO 8601 date string or null if invalid”, but the function:
  • Returns undefined when !dateStr.
  • Returns the original dateStr when parsing fails (so not necessarily ISO).
• Callers like toDbDate and validateDateRange implicitly treat “unparseable” as undefined/Invalid Date, while toGitDate is fine with git-native strings.

This makes the effective contract subtle and easy to misread from the JSDoc.

Consider tightening this up by either:

• Updating the JSDoc and return type to something like “ISO 8601 when parsed, otherwise the original string, or undefined when input is falsy”, or
• Returning undefined for unparseable inputs and letting toGitDate own the “fallback to original for git” behavior.

Also, relativePatterns is currently unused and can be removed or wired into the parsing logic to avoid dead code.

Also applies to: 35-41, 99-102, 144-156

packages/web/src/features/search/gitApi.test.ts (1)

11-19: Align error-handling tests with production sew behavior

In tests you mock sew as a simple passthrough:

vi.mock('@/actions', () => ({
    sew: (fn: () => any) => fn(),
}));

and later assert that searchCommits rejects for “other Error instances” and non-Error exceptions. In production, searchCommits is wrapped by the real sew from actions.ts, which catches thrown errors and returns a ServiceError instead of letting the promise reject.

That means the “throws” behavior in these tests doesn’t match what callers will see at runtime. To avoid surprises, consider either:

• Asserting on a ServiceError shape for these cases (matching real sew), or
• Removing the sew dependency from searchCommits and letting routes/actions wrap it, if you explicitly want searchCommits itself to throw.

Either way, having tests reflect the actual public contract of searchCommits will make future changes safer.

Also applies to: 287-347

packages/web/src/actions.ts (1)

463-541: Handle ServiceError results from searchCommits explicitly in activity filtering

In the activity-based branch of getRepos:

const activityChecks = await Promise.all(repos.map(async (repo) => {
    try {
        const commits = await searchCommits({ ... });

        if (Array.isArray(commits) && commits.length > 0) {
            return repo;
        }
    } catch (e) {
        // ...
    }
    return null;
}));

searchCommits is already wrapped in sew (in gitApi.ts), so for git/log failures it resolves to a ServiceError rather than throwing. As a result:

• The catch block here will rarely, if ever, run for git-related issues.
• Cases like “repo not on disk”, “invalid date range”, or git timeouts will show up as a ServiceError value, get treated as “no activity”, and be silently filtered out with no logging.

If you want list_repos with activeAfter/activeBefore to surface date-range validation failures instead of just returning an empty list, and to log unexpected git errors, consider branching explicitly on the union:

const result = await searchCommits({
    repoId: repo.id,
    since: activeAfter,
    until: activeBefore,
    maxCount: 1,
});

if (Array.isArray(result)) {
    return result.length > 0 ? repo : null;
}

// `result` is a ServiceError here
const message = result.message ?? '';
if (!message.includes('does not exist')) {
    console.error(
        `Error checking activity for repo ${repo.id} (${repo.name}):`,
        result,
    );
}
return null;

This keeps “repo not yet cloned” behaving as “no activity” while making invalid ranges and other unexpected failures visible.

Separately, this wiring introduces a circular dependency: actions.ts imports searchCommits from ./features/search/gitApi, and gitApi.ts imports sew from @/actions. It works today because sew is only used at call time, but it’d be cleaner long term to extract sew to a small shared module (or inject it) to avoid subtle initialization issues in future refactors.

packages/web/src/features/search/gitApi.ts (1)

1-22: searchCommits implementation looks solid; consider small cleanups

Overall, this module cleanly encapsulates git-based commit search:

• Validates the repo path and date range up front.
• Uses toGitDate to preserve git-native formats when possible.
• Configures simpleGit with a sensible timeout and concurrency.
• Provides clear, user-facing messages for common git failures (invalid repo, ambiguous args, timeout), with a generic fallback.

A couple of minor nits you might consider:

• You check existsSync(repoPath) both before calling createGitClientForPath and inside that helper. One of those checks could be dropped unless you specifically want the second check to catch races and produce a different message.
• In the catch block you return unexpectedError(...) for known cases but throw for unknown ones, relying on sew to convert those into generic ServiceErrors. That’s fine, but it means callers always see ServiceError (never a thrown error) in production, so other code (like getRepos) should treat the result as Commit[] | ServiceError rather than depending on exceptions.

These are non-blocking; the core behavior and error mapping look good.

Also applies to: 58-150

packages/mcp/src/index.ts (1)

233-347: list_repos client‑side filtering/pagination looks good; confirm listRepos returns the full filtered set.

The flow of: call listRepos({ activeAfter, activeBefore }), then apply name filtering, sort, compute totalCount, and paginate on the MCP side cleanly fixes the “take before filter” bug, and the new empty/out‑of‑range page messages (including the temporal‑filtering note) are clear.

This does rely on listRepos returning the complete set of repositories for the given temporal filters (i.e., not already paginated). If the underlying API still pages results, totalCount, totalPages, and the “Try pageNumber between 1 and X” guidance will all be based only on that first backend page.

If listRepos is still paginated underneath, consider either:

• Fetching all pages when temporal filters are used, or
• Threading backend pagination parameters and totals through to this tool instead of recomputing them locally.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

packages/mcp/README.md (1)

220-252: Verify that all documented relative date formats are actually supported

The Date Format Examples list "last month" in addition to "30 days ago", "1 week ago", "last week", "yesterday". Please confirm parseTemporalDate (and any schema refinements) really accept "last month"; if not, either add support or trim the docs to only list supported forms.

packages/shared/src/index.server.ts (1)

29-32: Server cache directory constants re-export looks good

Re-exporting REPOS_CACHE_DIR and INDEX_CACHE_DIR here keeps server-only constants centralized. Just ensure these exports are only imported from server-side code paths to avoid accidental bundling into client bundles.

CHANGELOG.md (1)

12-14: Temporal filtering changelog entry is accurate; consider optionally mentioning commit search

The new “temporal filtering” bullet correctly captures branch/revision and index date filtering plus relative date support. If you want full traceability from the root changelog, you might also briefly call out the new commit-time search surface (searchCommits), but that’s optional since MCP-level changes are already documented separately.

packages/shared/src/constants.server.ts (1)

1-7: Env-aware cache directory constants are reasonable

Using env.DATA_CACHE_DIR with /tmp/sourcebot/... fallbacks is a pragmatic approach and the guard avoids load-time failures when the env var is unset. If you ever want to DRY this up, you could factor out a small helper for the base cache dir, but it’s not necessary.

packages/web/src/features/search/searchApi.ts (3)

27-62: Search/streamSearch integration of permission, temporal, and branch filters looks coherent

Using getAccessibleRepoNamesForUser plus getTemporallyFilteredRepos, then intersecting via combineRepoFilters, gives a clear repoSearchScope that respects both permissions and index-time constraints. Applying applyBranchFilter on the QueryIR when gitRevision is present is a sensible way to express branch selection in a Zoekt-friendly IR.

From a behavior standpoint, this matches the documented semantics:

• temporal filters operate at the repo (indexedAt) level, not commit time, and
• branch/revision filtering is orthogonal via gitRevision.

No correctness issues spotted here.

---

105-194: Temporal repo filter helpers are correct; small refinements are optional

getTemporallyFilteredRepos and combineRepoFilters do the right thing:

• They only apply temporal filtering when since or until are provided.
• They use toDbDate to normalize dates and gracefully skip bounds that resolve to undefined.
• They intersect permission-based and temporal repo sets when both are present, returning undefined when no filters apply (preserving prior unfiltered behavior).

Two optional refinements you might consider (not blockers):

• If schema/dateUtils don’t already enforce it, ensure since/until are validated (including since <= until) before reaching this helper, so we never silently treat an invalid date string as “no temporal filtering”.
• If you care about minimizing DB round trips, a future refactor could merge permission and temporal conditions into a single findMany call instead of two separate queries.

Overall, the helper logic is sound.

---

196-214: Branch filter wrapper aligns with a composable QueryIR design

applyBranchFilter wrapping the existing QueryIR inside an and node with a branch constraint is a clean, composable approach. It should play nicely even if the incoming IR already contains other filters.

If there are any special cases for gitRevision values (e.g., a wildcard such as "*" meaning “all branches”), double-check that callers either normalize those before reaching this function or handle them separately, since this helper currently always applies an exact branch match.

packages/web/src/features/search/gitApi.test.ts (1)

98-172: Consider centralizing and restoring dateUtils spies for test robustness

You call vi.spyOn(dateUtils, 'validateDateRange') / toGitDate in multiple tests and suites while only using vi.clearAllMocks() in the top-level beforeEach. To avoid any subtle interactions or “already spied” issues as Vitest/Jest evolve, it’d be a bit more robust to either:

• Create the spies once in a beforeAll/outer beforeEach and just adjust mockReturnValue/mockImplementation per test, or
• Add vi.restoreAllMocks() in an afterEach so each spyOn starts from the real implementation.

Not urgent, but it can make future test changes safer.

Also applies to: 239-271, 317-383

packages/mcp/src/schemas.ts (1)

200-219: Avoid future drift by sharing commit search shapes between web and MCP

searchCommitsRequestSchema, commitSchema, and searchCommitsResponseSchema match the web types and backend Commit interface today, but they’re now defined in three places. When convenient, consider moving these schemas/types into a small shared package (or exporting from a single canonical module) and importing them here and in web/gitApi.ts to reduce the risk of subtle mismatches over time.

packages/web/src/features/search/gitApi.ts (1)

61-78: Unify commit search types with shared schemas to reduce duplication

SearchCommitsRequest and Commit here duplicate the shapes already expressed in web/src/features/search/types.ts and mcp/src/schemas.ts. To keep things consistent and automatically pick up schema changes (like maxCount bounds), consider:

• Importing SearchCommitsRequest and Commit from the shared Zod-based types, or
• Exporting a single canonical TS type for these and reusing it in all three locations.

This is mainly a maintainability tweak; behavior is fine as-is.

Also applies to: 200-219

packages/mcp/src/index.ts (2)

202-202: Align schema default with documentation.

The description states "default: 50" but the schema doesn't enforce this default with .default(50). If maxCount is omitted, it will be undefined, requiring the backend to handle the default.

Consider adding .default(50) to the schema for consistency:

-        maxCount: z.number().describe(`Maximum number of commits to return (default: 50).`).optional(),
+        maxCount: z.number().int().positive().default(50).describe(`Maximum number of commits to return (default: 50).`),

This ensures the schema validates as an integer, is positive, and matches the documented default behavior.

---

298-303: Consider including indexedAt in output when temporal filtering is applied.

When users filter repositories by activeAfter or activeBefore, they might want to see when each repository was last indexed to understand the filtering results. Currently, the output only includes id and url.

Consider conditionally including the lastIndexed or indexedAt field when temporal parameters are provided:

const content: TextContent[] = paginated.map(repo => {
    let text = `id: ${repo.repoName}\nurl: ${repo.webUrl}`;
    if ((activeAfter || activeBefore) && repo.indexedAt) {
        text += `\nlast_indexed: ${repo.indexedAt}`;
    }
    return {
        type: "text",
        text,
    }
});

This would help users verify that the temporal filtering is working as expected.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

packages/mcp/README.md (1)

169-205: Temporal docs for search_code / list_repos are clear; consider clarifying semantics

The distinction that these filters operate on index time (not commit time) and accept ISO or relative formats is well documented. If you want to avoid ambiguity, consider briefly stating whether bounds are inclusive and which timezone the server uses to interpret relative phrases (e.g., “30 days ago” evaluated in server-local time vs UTC).

packages/web/src/features/search/dateUtils.ts (3)

21-75: parseTemporalDate covers key formats; minor robustness improvements possible

The ISO-first parsing and explicit handling of “yesterday” / N <unit>s ago look good and match the documented relative formats. To tighten things up:

• Use an explicit radix in parseInt(matchRelative[1], 10) to avoid historical parseInt footguns.
• Consider documenting that “last week/month/year” and N <unit>s ago are evaluated relative to the current timestamp (not normalized to start-of-week/month/year), so behavior is clear to callers.

---

110-155: Date range validation and DB conversion behave reasonably; consider surfacing invalid inputs

validateDateRange and toDbDate correctly use parseTemporalDate and gracefully handle unparsable values by returning null/undefined. One tradeoff is that a user-supplied but invalid since/until silently degrades to “no bound” instead of producing a clear error. If you want stricter UX for index-time filters (e.g., search_code, list_repos), consider having validateDateRange return an error string when either side is unparsable, and wiring that into the HTTP/MCP response for those call sites (similar to search_commits).

---

164-185: toGitDate preserves git-native formats; keep patterns in sync with parser

The whitelist in gitNativeFormats correctly preserves relative and ISO forms that Git already understands and falls back to parseTemporalDate otherwise. To avoid future drift, consider centralizing these regexes (or a shared enum of supported relative tokens) with parseTemporalDate so adding a new relative form only requires updating one place.

packages/web/src/features/search/gitApi.test.ts (2)

5-49: Mock setup cleanly isolates searchCommits; watch for future module-surface changes

The targeted mocks for simple-git, fs, @sourcebot/shared, @/lib/serviceError, @/actions, @/withAuthV2, and @/lib/utils give you good control over dependencies and error paths. If @sourcebot/shared’s surface grows and gitApi starts importing additional exports from it, this full-module mock will need updating—at that point, consider using vi.importActual and overriding only REPOS_CACHE_DIR to reduce maintenance.

---

71-96: Repository-path assertions are OS-specific; consider using path.join in expectations

The tests asserting existsSync is called with '/mock/cache/dir/456' (and similar) assume POSIX-style separators, but gitApi uses path.join, which will produce \-separated paths on Windows. To keep tests OS-agnostic, you could either:

• Import path and assert against path.join('/mock/cache/dir', '456'), or
• Use expect.stringContaining('/mock/cache/dir') + repo ID, if exact separators aren’t important.

Also applies to: 456-472

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro