From 84946f2e382b390fd1dc9fb1e598bbb099155bf6 Mon Sep 17 00:00:00 2001 From: factory-nizar Date: Wed, 3 Jun 2026 10:37:24 -0700 Subject: [PATCH 1/2] feat(gitlab): Phase 1 GitLab support via CI/CD Component (#93) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(gitlab): add hello-world CI/CD template to validate include:remote Adds a minimal gitlab/templates/review.yml job that prints a hello-world message. Used to verify that a GitLab CI pipeline can pull the template over HTTPS from raw.githubusercontent.com before we layer in the Docker image and real review entrypoints. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * feat(gitlab): scaffold src/gitlab adapter (context, token, api, types) Pure-addition first slice of GitLab support. Mirrors the shape of src/github/ so subsequent entrypoints can swap adapter by PLATFORM env. - src/gitlab/context.ts parses CI_* env vars into a normalized ParsedGitlabContext with MR + commit + user + inputs - src/gitlab/token.ts reads GITLAB_TOKEN with OVERRIDE/CI_JOB_TOKEN fallbacks; throws a clear error when missing - src/gitlab/api/client.ts is a thin fetch wrapper around GitLab v4 exposing getMr/getMrChanges/listNotes/createNote/updateNote/ createDiscussionOnDiff/updateMrDescription - src/gitlab/types.ts defines GitlabMr, GitlabNote, GitlabDiscussion, GitlabPosition - 16 new unit tests; full existing suite (381 -> 397) still passes No GitHub code paths touched. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * feat(gitlab): add MCP server exposing MR operations to droid exec src/mcp/gitlab-mr-server.ts mirrors src/mcp/github-pr-server.ts in shape so the review prompt can stay platform-agnostic at the tool layer. Tools registered: - get_mr / get_mr_changes / list_mr_notes - create_mr_note / update_mr_note (sticky tracking comment lifecycle) - update_mr_description (for @droid fill) - submit_review: posts an optional summary note plus N inline discussions anchored to diff positions; per-comment errors are collected so one bad position doesn't abort the batch. Position objects are built from the MR's diff_refs (base/head/start SHA) and switch new_line vs old_line based on RIGHT vs LEFT side, matching the GitHub PR review tool's contract. 5 new unit tests cover summary, inline anchoring, LEFT-side mapping, and partial-failure batching. Full suite: 402/402. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * feat(gitlab): add prepare + update-comment-link entrypoints and sticky-note logic Implements the sticky tracking note lifecycle for GitLab MR pipelines: prepare creates (or reuses) the running-state note before droid exec runs; update-comment-link rewrites it with success/failure + pipeline links after droid exec completes. - src/gitlab/operations/tracking-note.ts builds the note body with a hidden marker so retries find and update the same note instead of creating duplicates. Renders pipeline/job links, security badge (when automatic_security_review is on), and an error-details
block on failure. - src/entrypoints/gitlab-prepare.ts parses CI env, gates on merge_request_event + automatic_review, writes a JSON state file (DROID_STATE_FILE) for downstream steps. - src/entrypoints/gitlab-update-comment-link.ts reads the state file and PUTs the final body. Skips cleanly when no review ran. 7 new tracking-note tests; full suite 402 -> 409. Also tightened two gitlab-mr-server tests to satisfy noUncheckedIndexedAccess. Cross-SCM include:remote was validated end-to-end against gitlab.com pipeline #2568334623 (passed) before this commit. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * feat(gitlab): real CI/CD Component template (runtime-clone, no Docker) Replaces the hello-world include-test template with a real droid-review job. No Docker image dependency for v1; instead the job: 1. Uses the public oven/bun:1.2.11 image (Bun preinstalled) 2. apt-get installs git/curl/ca-certificates on top 3. Shallow-clones Factory-AI/droid-action at $DROID_ACTION_REF 4. bun install --frozen-lockfile 5. Runs src/entrypoints/gitlab-prepare.ts (creates sticky tracking note) 6. droid exec placeholder (real prompt + MCP wiring next commit) 7. after_script always runs src/entrypoints/gitlab-update-comment-link.ts so failures still rewrite the note to the failure state Template uses GitLab's spec.inputs so it works equally well via `include: component:` (post-Catalog publish) and `include: remote:` (today). Inputs: automatic_review, review_depth, review_model, reasoning_effort, droid_action_repo, droid_action_ref, stage. Required CI variables on the consumer side: FACTORY_API_KEY, GITLAB_TOKEN. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * feat(gitlab): wire up real droid exec with MCP server + inline review The template now installs the Droid CLI, registers the gitlab-mr MCP server with the runtime CI env, and runs `droid exec` against a review prompt that gitlab-prepare writes to /tmp/droid-prompts/droid-prompt.txt. - src/gitlab/review-prompt.ts: minimal v1 review prompt builder embedding the MR diff and instructions to call submit_review with a single batched set of inline comments (max 8). Returns LGTM body when no issues found. - src/entrypoints/gitlab-prepare.ts: after creating the sticky note, fetches MR + diff via the GitLab API and writes the prompt file; exposes promptPath on the state JSON. - gitlab/templates/review.yml: * Installs Droid CLI via curl https://app.factory.ai/cli | sh * Skips droid exec if prepare set shouldRunReview=false * Registers MCP server: droid mcp add gitlab_mr --env ... * Runs: droid exec -f $DROID_PROMPT_FILE --output-format stream-json --skip-permissions-unsafe (plus optional --model/--reasoning-effort) * after_script reads /tmp/droid-error.txt for failure details - 3 new prompt-builder tests; suite at 412 passing. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * fix(gitlab): wire review_depth presets via resolveReviewConfig Previously, review_depth was a declared spec.input that was parsed and threaded through gitlab-prepare but never consumed by the prompt builder or the template -- so customers passing review_depth: shallow silently still got Droid's default model. This wires it end-to-end: - gitlab-prepare now calls resolveReviewConfig({reviewModel, reasoningEffort, reviewDepth}) from src/utils/review-depth.ts (which already had the preset table: shallow -> kimi-k2-0711, deep -> gpt-5.2 + high reasoning), reusing the same logic the GitHub flow uses. - Explicit review_model / reasoning_effort still beat the depth preset (resolveReviewConfig handles priority). - Resolved values are written to /tmp/droid-prompts/resolved-env.sh as shell exports (RESOLVED_MODEL / RESOLVED_REASONING_EFFORT), plus echoed into the state.json for visibility. - Template sources the shim before constructing droid exec args and uses the RESOLVED_* values for --model / --reasoning-effort. Adds 6 unit tests covering: default deep preset, shallow preset, explicit model override, explicit effort override, both overrides simultaneously, unknown depth fallback. Suite at 418 passing. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * feat(gitlab): two-pass review (candidates + validator) mirroring GitHub Replaces the v1 single-pass bespoke prompt with the same two-pass flow the GitHub Action uses: Pass 1 generates review_candidates.json without posting; Pass 2 validates each candidate, writes review_validated.json, and submits approved findings in a single batched call. The /review skill (loaded by Droid at runtime) drives both passes; the prompts here are the runtime harness that selects which pass + which tools. Why two droid exec calls rather than one: 1. Tool gating — Pass 1 has NO access to gitlab_mr___submit_review so the model can't shortcut by posting raw candidates. --enabled-tools is fixed at process start, so a single exec can't switch mid-run. 2. Fresh-eyes context — Pass 2 re-reads the diff without memory of why each candidate was generated, dropping ~30-60% false positives. 3. Hard output checkpoint on disk between passes (resumable, debuggable). 4. Clean failure semantics — half-baked Pass 1 won't pollute Pass 2. New files: * src/gitlab/data/review-artifacts.ts — fetch + write mr.diff, existing_comments.json, mr_description.txt (mirror of GitHub's precomputed PR artifacts). * src/gitlab/prompts/types.ts — shared prompt-context shape. * src/gitlab/prompts/candidates.ts — Pass 1 prompt, ported from GitHub's review-candidates-prompt.ts with MR/project terminology. * src/gitlab/prompts/validator.ts — Pass 2 prompt, ported from GitHub's review-validator-prompt.ts; uses gitlab_mr___submit_review and the new gitlab_mr___update_tracking_note tool. * src/entrypoints/gitlab-prepare-validator.ts — overwrites the prompt file with Pass 2 between the two droid exec calls. * test/gitlab/prompts.test.ts + test/gitlab/review-artifacts.test.ts — 14 new tests covering the two prompts and artifact computation. MCP additions: * src/mcp/gitlab-mr-server.ts — new update_tracking_note tool that reads DROID_MR_IID + DROID_TRACKING_NOTE_ID from env, symmetric with GitHub's github_comment___update_droid_comment. Template changes (gitlab/templates/review.yml): * Sources resolved-env.sh once up front (now includes DROID_MR_IID + DROID_TRACKING_NOTE_ID). * Registers the MCP server with those env vars exposed. * Pass 1 droid exec: --enabled-tools excludes submit_review. * Runs gitlab-prepare-validator between the two execs. * Pass 2 droid exec: --enabled-tools includes gitlab_mr___submit_review and gitlab_mr___update_tracking_note. * Artifacts archive bumped to include review_candidates.json, review_validated.json, droid-prompt.txt for debugging. Removed: * src/gitlab/review-prompt.ts (v1 single-pass bespoke prompt). * test/gitlab/review-prompt.test.ts. Refactoring src/gitlab/prompts/ + src/create-prompt/ into a shared src/core/review/ tree is queued as a follow-up PR. All 432 tests pass; tsc + prettier clean. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * feat(gitlab): wire `settings` input pass-through (parity with GitHub) Adds the same `settings` input the GitHub action exposes: * spec.input on the CI/CD Component (JSON string OR path to JSON file). * Plumbed via DROID_SETTINGS env var to gitlab-prepare. * gitlab-prepare reuses base-action/src/setup-droid-settings.ts (zero GitHub deps, fully portable) to write ~/.factory/droid/settings.json before either droid exec call. Always sets enableAllProjectMcpServers=true to match the GitHub flow. * context.inputs.settings surfaces the raw value. * 2 new context tests; 434 tests pass. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * feat(gitlab): GH parity batch — custom droids, debug artifacts, telemetry, draft skip, stderr capture - 1.6b Copy bundled .factory/droids from droid-action checkout into runner ~/.factory/droids before droid exec, mirroring the GitHub action's Setup Custom Droids step. - 1.6c Stage debug artifacts (/tmp/droid-prompts/*, droid-error.txt) into .droid-debug/ inside CI_PROJECT_DIR in after_script so GitLab can upload them on failure (GitLab artifacts cannot reach outside CI_PROJECT_DIR). - 1.6d Capture droid exec stream-json output to per-pass JSONL log files via tee + pipefail; parse session_id / numTurns / durationMs (+ legacy cost_usd) from the last completion event of each pass; render a summary line (44 turns • 11m 40s • $0.42) and a collapsible session-IDs block in the sticky tracking note. - C7 Skip Draft: / WIP: MRs at the rules: level (GitLab-native equivalent of GitHub's draft == false workflow if:). - C9 On droid exec failure, tail 60 lines of the per-pass log into /tmp/droid-error.txt so the existing error-details block in the sticky note surfaces real failure context instead of a generic message. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * feat(gitlab): expose include_suggestions input (A5) Adds the include_suggestions spec input to the GitLab review template. Threading was already in place: gitlab-prepare reads the INCLUDE_SUGGESTIONS env var, stores it in the on-disk state, gitlab-prepare-validator restores it, and both candidate + validator prompts already consume includeSuggestions to gate suggestion-block guidance. This commit just exposes the toggle as a customer-facing input. Mirrors the GitHub action's include_suggestions input. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * feat(gitlab): cache bun install cache across pipelines (C8) Adds a cache: block keyed on droid_action_ref so bun's package cache survives across MR pipelines. BUN_INSTALL_CACHE_DIR is pinned to CI_PROJECT_DIR/.bun-cache because GitLab's cache mechanism can only persist paths inside the project directory. policy: pull-push lets every job benefit from prior downloads and contribute newly-fetched packages back to the cache. Mirrors the actions/cache pattern in the GitHub action. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * feat(gitlab): rate-limit + exponential backoff in GitlabClient (C1) Wraps every GitlabClient.request call with retry-on-transient-failure logic, mirroring the implicit retry behaviour the GitHub action gets from Octokit. Retries are limited to a configurable maxRetries (default 5) for: 408, 429, 500, 502, 503, 504, and raw fetch network errors. On 429 the client honours the Retry-After header when present (seconds or HTTP-date), and otherwise uses exponential backoff with jitter clamped to maxDelayMs. Non-retryable 4xx errors (401, 403, 404, etc.) surface immediately as GitlabApiError. The retry knobs are exposed via a third constructor argument so tests can use sub-millisecond delays without needing fake timers. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * feat(gitlab): paginate listNotes + add listDiscussions (C2) Adds a private paginate() helper that follows GitLab's standard pagination either via the X-Next-Page header (newer GitLab) or Link rel="next" (older GitLab + GitLab.com responses) with per_page=100 to minimise round-trips. listNotes now uses it and a new listDiscussions method is exposed for use cases that need to inspect existing diff discussions. Mirrors Octokit's implicit paginate-everything behaviour that the GitHub action gets for free. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * Revert "feat(gitlab): paginate listNotes + add listDiscussions (C2)" This reverts commit 7c721d1f2c7d174f688f9795981ef3d0d19f3266. * Revert "feat(gitlab): rate-limit + exponential backoff in GitlabClient (C1)" This reverts commit d70dbd70197c8810c6e55204d3ac0f6dc20686cb. * Revert "feat(gitlab): cache bun install cache across pipelines (C8)" This reverts commit 0d6c73b0feae6598590844a3f8739df4aced1f58. * revert(gitlab): drop draft-skip rules + stderr tail-60 capture Removes two GitLab-only behaviours that don't exist in the GitHub action: - Draft/WIP `rules: when: never` block. GitHub leaves draft handling to the user's workflow `if: ... draft == false` condition and doesn't bake it into the action. Customers who want to skip drafts can add the same condition to their own .gitlab-ci.yml. - Stderr tail-60 capture into droid-error.txt. GitHub uses core.setFailed(error.message) which surfaces in the Actions run summary, not in the PR comment body — the comment just shows an actionFailed flag. Revert to the plain "droid exec pass N failed" message for parity. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * feat(gitlab): expose automatic_security_review input (A1, per-MR scope) Wires the automatic_security_review toggle through the GitLab CI/CD Component. When set to "true", the security-reviewer subagent already bundled in .factory/droids/security-reviewer.md is spawned in parallel with the code-review subagents during Pass 1 (the conditional block in src/gitlab/prompts/candidates.ts already handles this via the securityReviewEnabled flag). Security findings are prefixed with [security] and flow through the same Pass-2 validator path as code-review findings, ultimately posted via a single batched submit_review call. No plugin install is required: the security-review skill that the subagent invokes as its first action is built into the Droid CLI binary. Mirrors the GitHub action's automatic_security_review input for the per-MR flow only; the scheduled full-repo scan (which additionally uses threat-model-generation / commit-security-scan / vulnerability-validation skills from the security-engineer plugin) is a separate feature. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * feat(gitlab): expose security_block_on_critical/high inputs (A10) Matches GitHub action's input surface for security_block_on_critical (default "true") and security_block_on_high (default "false"). Like the GitHub action, the inputs are parsed into context but not currently consumed by production code — they exist for surface-level parity and forward compatibility when blocking logic lands. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * docs(gitlab): add gitlab-setup.md and README section for GitLab CI/CD Component Documents the manual installation flow for the new GitLab CI/CD Component (`gitlab/templates/review.yml`): prerequisites, full inputs table, what the pipeline produces, how the two-pass review works, what is intentionally not yet supported (comment triggers, fill mode, scheduled scans), and troubleshooting steps. README now has a short GitLab section that points to the full guide. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * docs(gitlab): add drop-in .gitlab-ci.yml samples under gitlab/examples/ Adds two consumer-facing sample files plus a README so users have a canonical reference for what a Component-consuming .gitlab-ci.yml looks like: * gitlab/examples/.gitlab-ci.minimal.yml — shortest possible, every default accepted, only the two required CI/CD variables wired. * gitlab/examples/.gitlab-ci.example.yml — annotated, every input spelled out with safe-default guidance, plus an optional @droid fill block (commented out) showing how to add fill. * gitlab/examples/README.md — table mapping each file to its use case and the variable-setup story. Also updates docs/gitlab-setup.md: * Points readers at gitlab/examples/ instead of just inlining the snippet, so customers can clone the project, copy the file, done. * Drops the stale 'service account provisioning' reference now that the install-code-review skill no longer provisions SAs. * Tightens the GITLAB_TOKEN row to make the poster-identity story explicit: the token owner IS the poster, no API impersonation. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * docs(gitlab): hide internal inputs, trim setup.md, default ref to main Customer-facing surface cleanup before GA: * gitlab/templates/review.yml: change droid_action_ref default from "dev" to "main" so customers who don't set it track stable. Description annotated 'Internal: most users leave at the default'. * docs/gitlab-setup.md: drop droid_action_ref, droid_action_repo, stage from the Inputs table — they're for self-hosted GitLab mirrors / advanced overrides, not standard customer config. * docs/gitlab-setup.md: drop everything from 'How it works' onward (How it works, What's not yet supported, Troubleshooting, Self-hosted GitLab). Those belong in deeper docs / runbooks; the setup page should be exactly setup. * docs/gitlab-setup.md: drop the 'pin to a release tag (e.g. v1)' advice — droid-action doesn't tag yet, and the @main URL pin is the canonical pattern. * gitlab/examples/.gitlab-ci.example.yml: drop droid_action_ref from the inputs block + drop the same line from the optional fill snippet. Update the comment to just explain the @main pin. * gitlab/examples/README.md: drop the 'custom stage' reference now that stage isn't documented as a user-facing knob. The hidden inputs still exist in the template with sensible defaults, so air-gapped customers mirroring droid-action can still override droid_action_repo or pin droid_action_ref to a private SHA — they just aren't part of the documented surface. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * docs(readme): drop GitLab non-support note from quick-start Customer-facing surface should describe what is supported, not what isn't. Caveats belong in deeper docs / runbooks if anywhere, not in the README quick-start. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * docs(readme): tighten GitLab section wording Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * refactor(gitlab): rename template to droid-review.yml; ship as two-file pattern Component template renamed from gitlab/templates/review.yml to gitlab/templates/droid-review.yml. Customer drops a self-contained factory/droid-review.yml in their project and adds a single `include: - local:` line to their .gitlab-ci.yml, mirroring the GitHub action's `.github/workflows/droid-review.yml` model. Examples restructured to reflect the two-file layout. Docs + README snippets updated accordingly. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * refactor(gitlab): move component to top-level templates/ for Catalog compatibility GitLab CI/CD Catalog requires components to live at top-level templates/. Move gitlab/templates/droid-review.yml to templates/droid-review.yml so the repo is ready for Catalog publish (gitlab.com/factory-ai/droid-action) without future renames. Add templates/README.md explaining the directory ownership (mandated by Catalog, parallel to action.yml + .github/workflows/ for GitHub) and a headline comment in droid-review.yml. Update all existing include URLs (README, docs, examples) to the new path. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * feat(gitlab): point at gitlab.com/factory-components/droid-action mirror Switch the customer-facing include form from a raw GitHub URL to GitLab's native include: project: form pointing at the gitlab.com pull-mirror of this repo. Customers no longer need github.com egress to fetch the template — gitlab.com is sufficient. Also flip the runtime droid_action_repo default from github.com/Factory-AI/droid-action.git to gitlab.com/factory-components/droid-action.git so the at-job clone of the runtime source also goes through gitlab.com. Customers behind a firewall that only permits gitlab.com + app.factory.ai can now run the full flow. Updated: - README.md GitLab quick-start snippet (include: project: form) - docs/gitlab-setup.md Step 2 snippet - gitlab/examples/factory/droid-review.yml - templates/README.md — documents include: project: (default), future include: component: form once Catalog-tagged, and a raw GitHub URL fallback for projects that can't reach gitlab.com - templates/droid-review.yml headline comment + droid_action_repo default Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * fix(gitlab): address bot review nits on #93 - templates/droid-review.yml: remove top-level `stages:` block so the Component doesn't leak its stage list into the consuming project. Job-level `stage: $[[ inputs.stage ]]` is unchanged; consumers add the stage to their own `stages:` list (or omit it for the default). - templates/droid-review.yml: tighten droid_action_ref docstring to "tag/branch" since the clone uses `git clone --branch` (SHAs would silently fail). - src/gitlab/token.ts: drop CI_JOB_TOKEN fallback. It was always set in CI so the MissingGitlabTokenError never fired, producing confusing 401s on first API call instead of a clear "set GITLAB_TOKEN" message. CI_JOB_TOKEN also lacks scopes for notes/discussions. - src/mcp/gitlab-mr-server.ts: require a line anchor on ReviewComment (line for RIGHT, old_line for LEFT) via .refine(); GitLab rejects unanchored diff discussions. - src/mcp/gitlab-mr-server.ts: remove `.max(30)` cap on comments so large MRs don't fail the entire submit_review call. Tests: 445/445 pass; tsc clean. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --------- Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- README.md | 33 ++ docs/gitlab-setup.md | 106 ++++++ gitlab/examples/.gitlab-ci.yml | 9 + gitlab/examples/README.md | 21 + gitlab/examples/factory/droid-review.yml | 31 ++ src/entrypoints/gitlab-prepare-validator.ts | 97 +++++ src/entrypoints/gitlab-prepare.ts | 322 ++++++++++++++++ src/entrypoints/gitlab-update-comment-link.ts | 128 +++++++ src/gitlab/api/client.ts | 156 ++++++++ src/gitlab/api/config.ts | 7 + src/gitlab/context.ts | 148 ++++++++ src/gitlab/data/exec-telemetry.ts | 149 ++++++++ src/gitlab/data/review-artifacts.ts | 94 +++++ src/gitlab/operations/tracking-note.ts | 118 ++++++ src/gitlab/prompts/candidates.ts | 148 ++++++++ src/gitlab/prompts/types.ts | 29 ++ src/gitlab/prompts/validator.ts | 132 +++++++ src/gitlab/token.ts | 20 + src/gitlab/types.ts | 81 ++++ src/mcp/gitlab-mr-server.ts | 359 ++++++++++++++++++ templates/README.md | 40 ++ templates/droid-review.yml | 213 +++++++++++ test/gitlab/api-client.test.ts | 109 ++++++ test/gitlab/context.test.ts | 188 +++++++++ test/gitlab/exec-telemetry.test.ts | 116 ++++++ test/gitlab/prompts.test.ts | 129 +++++++ test/gitlab/review-artifacts.test.ts | 153 ++++++++ test/gitlab/review-config-resolution.test.ts | 52 +++ test/gitlab/token.test.ts | 50 +++ test/gitlab/tracking-note.test.ts | 112 ++++++ test/mcp/gitlab-mr-server.test.ts | 156 ++++++++ 31 files changed, 3506 insertions(+) create mode 100644 docs/gitlab-setup.md create mode 100644 gitlab/examples/.gitlab-ci.yml create mode 100644 gitlab/examples/README.md create mode 100644 gitlab/examples/factory/droid-review.yml create mode 100644 src/entrypoints/gitlab-prepare-validator.ts create mode 100644 src/entrypoints/gitlab-prepare.ts create mode 100644 src/entrypoints/gitlab-update-comment-link.ts create mode 100644 src/gitlab/api/client.ts create mode 100644 src/gitlab/api/config.ts create mode 100644 src/gitlab/context.ts create mode 100644 src/gitlab/data/exec-telemetry.ts create mode 100644 src/gitlab/data/review-artifacts.ts create mode 100644 src/gitlab/operations/tracking-note.ts create mode 100644 src/gitlab/prompts/candidates.ts create mode 100644 src/gitlab/prompts/types.ts create mode 100644 src/gitlab/prompts/validator.ts create mode 100644 src/gitlab/token.ts create mode 100644 src/gitlab/types.ts create mode 100644 src/mcp/gitlab-mr-server.ts create mode 100644 templates/README.md create mode 100644 templates/droid-review.yml create mode 100644 test/gitlab/api-client.test.ts create mode 100644 test/gitlab/context.test.ts create mode 100644 test/gitlab/exec-telemetry.test.ts create mode 100644 test/gitlab/prompts.test.ts create mode 100644 test/gitlab/review-artifacts.test.ts create mode 100644 test/gitlab/review-config-resolution.test.ts create mode 100644 test/gitlab/token.test.ts create mode 100644 test/gitlab/tracking-note.test.ts create mode 100644 test/mcp/gitlab-mr-server.test.ts diff --git a/README.md b/README.md index eadf5e9..c47e1ca 100644 --- a/README.md +++ b/README.md @@ -38,6 +38,39 @@ The guided flow will: For GitHub-only setups you can also run `/install-github-app`. See the [Automated Code Review guide](https://docs.factory.ai/guides/droid-exec/code-review) and the [GitHub App installation guide](https://docs.factory.ai/cli/features/install-github-app) for full details. +### GitLab + +GitLab support ships as a **GitLab CI/CD Component** that delivers automated code review — inline MR comments on every merge request, with optional security review. + +Two files in your project: + +`factory/droid-review.yml`: + +```yaml +include: + - project: "factory-components/droid-action" + ref: main + file: "/templates/droid-review.yml" + inputs: + automatic_review: "true" + automatic_security_review: "false" + review_depth: "deep" + +droid-review: + variables: + FACTORY_API_KEY: $FACTORY_API_KEY + GITLAB_TOKEN: $GITLAB_TOKEN +``` + +`.gitlab-ci.yml` (one include line, append to existing if present): + +```yaml +include: + - local: "factory/droid-review.yml" +``` + +Full setup, available inputs, and troubleshooting live in [`docs/gitlab-setup.md`](docs/gitlab-setup.md). + ### Manual Setup If you prefer to wire things up by hand: diff --git a/docs/gitlab-setup.md b/docs/gitlab-setup.md new file mode 100644 index 0000000..2a5c835 --- /dev/null +++ b/docs/gitlab-setup.md @@ -0,0 +1,106 @@ +# GitLab Setup + +This action ships a **GitLab CI/CD Component** that delivers the same +automated code-review experience as the GitHub action on GitLab merge +requests (MRs). The component runs on every `merge_request_event` pipeline, +posts inline comments on the diff, maintains a sticky tracking note, and +optionally runs a security-focused subagent in parallel. + +## Quick start with `/install-code-review` + +The fastest path is the guided installer built into the Droid CLI: + +```bash +droid +> /install-code-review +``` + +It detects GitLab, asks which account should be the poster of review +comments (you supply its PAT as `GITLAB_TOKEN`), asks the configuration +questions below, drops `factory/droid-review.yml` in your project, wires +it into `.gitlab-ci.yml`, and opens an MR / direct-commits to the target +project(s). + +## Manual installation + +### 1. Prerequisites + +| Requirement | How to get it | +| ------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| GitLab Maintainer role on the project | Repo admin grants you Maintainer (40) | +| `FACTORY_API_KEY` CI/CD variable | Generate at ; add as **masked**, **unprotected** variable at the project, subgroup, or top-level group level | +| `GITLAB_TOKEN` CI/CD variable | A personal access token with the `api` scope, owned by whichever account should post review comments. The token owner is the poster — there is no API impersonation. Add as **masked**, **unprotected**. | + +### 2. Add the CI/CD Component + +Drop-in samples live in [`gitlab/examples/`](../gitlab/examples/). The +layout is two files: + +- [`factory/droid-review.yml`](../gitlab/examples/factory/droid-review.yml) — self-contained config (include + inputs + variables). Drop verbatim. +- [`.gitlab-ci.yml`](../gitlab/examples/.gitlab-ci.yml) — project-root entry point. If you already have one, append the include line below to its `include:` block. + +**`factory/droid-review.yml`** (drop into your project): + +```yaml +include: + - project: "factory-components/droid-action" + ref: main + file: "/templates/droid-review.yml" + inputs: + automatic_review: "true" + automatic_security_review: "false" + review_depth: "deep" + include_suggestions: "true" + security_block_on_critical: "true" + security_block_on_high: "false" + +droid-review: + variables: + FACTORY_API_KEY: $FACTORY_API_KEY + GITLAB_TOKEN: $GITLAB_TOKEN +``` + +**`.gitlab-ci.yml`** (project root, just needs the one include line): + +```yaml +include: + - local: "factory/droid-review.yml" +``` + +> The remote `include:` URL is pinned to `@main`, which tracks the +> latest stable cut of droid-action. + +### 3. Push an MR + +Open or push to an MR. The next `merge_request_event` pipeline will run +the `droid-review` job. Expect ~5-10 minutes for a typical change. + +## Inputs + +| Input | Default | Description | +| ---------------------------- | --------- | -------------------------------------------------------------------------------------------------------------------------------------------- | +| `automatic_review` | `"true"` | Run code review automatically on every MR pipeline. | +| `automatic_security_review` | `"false"` | Run a parallel security-focused subagent on every MR pipeline. Findings are prefixed `[security]` and posted alongside code-review comments. | +| `review_depth` | `"deep"` | `"deep"` (thorough) or `"shallow"` (fast). | +| `review_model` | `""` | Override the model. Empty = use depth preset. | +| `reasoning_effort` | `""` | Override reasoning effort. Empty = use depth preset. | +| `include_suggestions` | `"true"` | Include code suggestion blocks in review comments when the fix is high-confidence. | +| `security_block_on_critical` | `"true"` | Block merge on CRITICAL security findings. (Mirrors GitHub action; surface-level parity.) | +| `security_block_on_high` | `"false"` | Block merge on HIGH security findings. (Mirrors GitHub action; surface-level parity.) | +| `settings` | `""` | Droid Exec settings as a JSON string or a path to a JSON file. Merged into `~/.factory/droid/settings.json` before each `droid exec` call. | + +## What you get + +Each MR pipeline produces: + +- **Inline review comments** anchored to the relevant diff lines, posted in a + single batched `submit_review` call. Findings are prefixed with priority + tags (`P0`, `P1`, `P2`, `P3`) and `[security]` for security findings. +- **A sticky tracking note** on the MR with pipeline + job links, telemetry + (`N turns • Xm Ys`), session IDs, and a security badge when + `automatic_security_review` is enabled. +- **Debug artifacts** at `.droid-debug/` (prompts, candidate JSON, raw + stream-json logs) retained for 1 week. +- **A custom droid library** copied from + `$DROID_ACTION_DIR/.factory/droids` into `~/.factory/droids` on the + runner, so subagents like `security-reviewer` are reachable. diff --git a/gitlab/examples/.gitlab-ci.yml b/gitlab/examples/.gitlab-ci.yml new file mode 100644 index 0000000..36cb454 --- /dev/null +++ b/gitlab/examples/.gitlab-ci.yml @@ -0,0 +1,9 @@ +# Example project-root .gitlab-ci.yml. +# +# If the project doesn't have a .gitlab-ci.yml yet, create one with at +# minimum the include line below. If it already has one, just append the +# `- local: "factory/droid-review.yml"` entry to its existing `include:` +# block (or add a new `include:` block if there isn't one). + +include: + - local: "factory/droid-review.yml" diff --git a/gitlab/examples/README.md b/gitlab/examples/README.md new file mode 100644 index 0000000..05fcc72 --- /dev/null +++ b/gitlab/examples/README.md @@ -0,0 +1,21 @@ +# GitLab examples + +Two-file layout for consuming the droid-action GitLab CI/CD Component: + +``` +your-project/ +├── .gitlab-ci.yml # gains one `include:` line +└── factory/ + └── droid-review.yml # self-contained droid-review config +``` + +| File | Where it lives in your project | Purpose | +| ----------------------------- | -------------------------------------------------- | --------------------------------------------------------------------------------------------- | +| `factory/droid-review.yml` | drop verbatim | Self-contained config: includes the remote Component, sets inputs, wires CI/CD variables. | +| `.gitlab-ci.yml` | append one `include:` line if the file exists | Project-root entry point. Just needs to include `factory/droid-review.yml`. | + +The two required CI/CD variables (`FACTORY_API_KEY`, `GITLAB_TOKEN`) are set +in the GitLab UI under **Project → Settings → CI/CD → Variables** (or at +the group level for org-wide rollout), masked and unprotected. + +For the full input reference see the docs at [`docs/gitlab-setup.md`](../../docs/gitlab-setup.md). diff --git a/gitlab/examples/factory/droid-review.yml b/gitlab/examples/factory/droid-review.yml new file mode 100644 index 0000000..bb64e1f --- /dev/null +++ b/gitlab/examples/factory/droid-review.yml @@ -0,0 +1,31 @@ +# factory/droid-review.yml +# +# Drop this file at `factory/droid-review.yml` in your project, then +# add a single `include: - local: "factory/droid-review.yml"` line to +# your `.gitlab-ci.yml` (see ../.gitlab-ci.yml for the entry-point +# example). +# +# Prerequisites (one-time, set in Project / Group → Settings → CI/CD): +# +# * FACTORY_API_KEY — masked variable. Get one at +# https://app.factory.ai/settings/api-keys +# * GITLAB_TOKEN — masked variable. A personal access token with +# `api` scope, owned by whichever GitLab account should post review +# comments. The token owner IS the poster. + +include: + - project: "factory-components/droid-action" + ref: main + file: "/templates/droid-review.yml" + inputs: + automatic_review: "true" # run on every MR event; "false" disables + review_depth: "deep" # "deep" (thorough) or "shallow" (fast) + include_suggestions: "true" # post code-suggestion blocks for high-confidence fixes + automatic_security_review: "false" # enable to flag vulns alongside the regular review + security_block_on_critical: "true" # block merge on CRITICAL findings (when security review is on) + security_block_on_high: "false" # block merge on HIGH findings + +droid-review: + variables: + FACTORY_API_KEY: $FACTORY_API_KEY + GITLAB_TOKEN: $GITLAB_TOKEN diff --git a/src/entrypoints/gitlab-prepare-validator.ts b/src/entrypoints/gitlab-prepare-validator.ts new file mode 100644 index 0000000..d71fb7e --- /dev/null +++ b/src/entrypoints/gitlab-prepare-validator.ts @@ -0,0 +1,97 @@ +#!/usr/bin/env bun + +/** + * Prepare step for Pass 2 (validator) of the GitLab two-pass review flow. + * + * Runs between the two `droid exec` invocations in the CI template: + * + * 1. Reads the state file produced by `gitlab-prepare.ts` (Pass 1). + * Bails out cleanly if Pass 1 decided not to review. + * 2. Reconstructs the GitLab review prompt context from state. + * 3. Generates the Pass-2 validator prompt. + * 4. Overwrites the shared prompt file (the same file Pass 1 used) + * so the next `droid exec -f ` consumes Pass 2. + * + * No GitLab API calls are made here — all the data we need is already + * on disk from Pass 1's artifact precomputation. + */ + +import * as fs from "fs/promises"; +import * as path from "path"; +import { generateGitlabReviewValidatorPrompt } from "../gitlab/prompts/validator"; +import type { GitlabReviewPromptContext } from "../gitlab/prompts/types"; +import type { PrepareState } from "./gitlab-prepare"; + +function stateFilePath(): string { + return ( + process.env.DROID_STATE_FILE || + path.join(process.env.CI_PROJECT_DIR || "/tmp", ".droid-state.json") + ); +} + +async function readState(): Promise { + const filePath = stateFilePath(); + const raw = await fs.readFile(filePath, "utf8"); + return JSON.parse(raw) as PrepareState; +} + +function ensure(value: T | null | undefined, name: string): T { + if (value === null || value === undefined) { + throw new Error( + `gitlab-prepare-validator: missing state.${name}; was gitlab-prepare run successfully?`, + ); + } + return value; +} + +async function run(): Promise { + const state = await readState(); + + if (!state.shouldRunReview) { + console.log( + `Pass 1 was skipped (reason: ${state.reason ?? "unknown"}); skipping validator prepare.`, + ); + return; + } + + const promptPath = ensure(state.promptPath, "promptPath"); + const mrIid = ensure(state.mrIid, "mrIid"); + const candidatesPath = ensure(state.candidatesPath, "candidatesPath"); + const validatedPath = ensure(state.validatedPath, "validatedPath"); + const diffPath = ensure(state.diffPath, "diffPath"); + const commentsPath = ensure(state.commentsPath, "commentsPath"); + const descriptionPath = ensure(state.descriptionPath, "descriptionPath"); + const headSha = ensure(state.headSha, "headSha"); + + const promptCtx: GitlabReviewPromptContext = { + projectPath: state.projectPath, + mrIid, + mrTitle: state.mrTitle ?? "", + sourceBranch: state.sourceBranch ?? "", + targetBranch: state.targetBranch ?? "", + headSha, + diffPath, + commentsPath, + descriptionPath, + candidatesPath, + validatedPath, + includeSuggestions: state.includeSuggestions, + securityReviewEnabled: state.securityReviewEnabled, + }; + + const prompt = generateGitlabReviewValidatorPrompt(promptCtx); + await fs.mkdir(path.dirname(promptPath), { recursive: true }); + await fs.writeFile(promptPath, prompt); + console.log( + `Wrote Pass-2 validator prompt (${prompt.length} bytes) to ${promptPath} (overwrote Pass 1)`, + ); +} + +if (import.meta.main) { + run().catch((error) => { + console.error("gitlab-prepare-validator failed:", error); + process.exit(1); + }); +} + +export { run }; diff --git a/src/entrypoints/gitlab-prepare.ts b/src/entrypoints/gitlab-prepare.ts new file mode 100644 index 0000000..5fec141 --- /dev/null +++ b/src/entrypoints/gitlab-prepare.ts @@ -0,0 +1,322 @@ +#!/usr/bin/env bun + +/** + * Prepare step for the GitLab CI/CD Component (Pass 1 of the two-pass + * review flow). + * + * Responsibilities: + * 1. Parse GitLab CI env into a normalized context. + * 2. Decide whether this pipeline should run a review (automaticReview + * flag + merge_request_event source). + * 3. Ensure a sticky tracking note exists on the MR; reuse the existing + * one if a prior pipeline already created it. + * 4. Fetch + persist the three review artifacts (mr.diff, + * existing_comments.json, mr_description.txt). + * 5. Resolve review depth presets into concrete model+effort. + * 6. Generate the Pass-1 (candidate-generation) prompt and write it + * to the shared prompt file. + * 7. Write a JSON state file consumed by `gitlab-prepare-validator` + * (Pass 2) and `gitlab-update-comment-link` (post-step). + * + * Pass 2 is generated later by `gitlab-prepare-validator.ts`, which + * reads this state file and overwrites the prompt file with the + * Pass-2 content before the second `droid exec` invocation. + */ + +import * as fs from "fs/promises"; +import * as path from "path"; +import { parseGitlabContext, isMergeRequestContext } from "../gitlab/context"; +import { setupGitlabToken, MissingGitlabTokenError } from "../gitlab/token"; +import { GitlabClient } from "../gitlab/api/client"; +import { + buildTrackingNoteBody, + findExistingTrackingNote, +} from "../gitlab/operations/tracking-note"; +import { computeReviewArtifacts } from "../gitlab/data/review-artifacts"; +import { generateGitlabReviewCandidatesPrompt } from "../gitlab/prompts/candidates"; +import type { GitlabReviewPromptContext } from "../gitlab/prompts/types"; +import { resolveReviewConfig } from "../utils/review-depth"; +import { setupDroidSettings } from "../../base-action/src/setup-droid-settings"; + +export type PrepareState = { + shouldRunReview: boolean; + projectId: string; + projectPath: string; + mrIid: number | null; + trackingNoteId: number | null; + diffBaseSha: string | null; + sourceBranchSha: string | null; + pipelineUrl: string | null; + jobUrl: string | null; + promptPath: string | null; + resolvedModel: string | null; + resolvedReasoningEffort: string | null; + + diffPath: string | null; + commentsPath: string | null; + descriptionPath: string | null; + candidatesPath: string | null; + validatedPath: string | null; + + mrTitle: string | null; + sourceBranch: string | null; + targetBranch: string | null; + headSha: string | null; + includeSuggestions: boolean; + securityReviewEnabled: boolean; + + reason?: string; +}; + +function promptFilePath(): string { + return process.env.DROID_PROMPT_FILE || "/tmp/droid-prompts/droid-prompt.txt"; +} + +function promptsDir(): string { + return path.dirname(promptFilePath()); +} + +function candidatesFilePath(): string { + return ( + process.env.REVIEW_CANDIDATES_PATH || + path.join(promptsDir(), "review_candidates.json") + ); +} + +function validatedFilePath(): string { + return ( + process.env.REVIEW_VALIDATED_PATH || + path.join(promptsDir(), "review_validated.json") + ); +} + +function resolvedEnvShimPath(): string { + return ( + process.env.DROID_RESOLVED_ENV_FILE || "/tmp/droid-prompts/resolved-env.sh" + ); +} + +function shellEscape(value: string): string { + return `'${value.replace(/'/g, `'\\''`)}'`; +} + +async function writeResolvedEnvShim( + model: string | null, + reasoningEffort: string | null, + extras: Record, +): Promise { + const filePath = resolvedEnvShimPath(); + await fs.mkdir(path.dirname(filePath), { recursive: true }); + const lines = [ + "# Generated by gitlab-prepare; source this to expose resolved review presets.", + `export RESOLVED_MODEL=${shellEscape(model ?? "")}`, + `export RESOLVED_REASONING_EFFORT=${shellEscape(reasoningEffort ?? "")}`, + ]; + for (const [k, v] of Object.entries(extras)) { + lines.push(`export ${k}=${shellEscape(v ?? "")}`); + } + await fs.writeFile(filePath, lines.join("\n") + "\n"); + console.log(`Wrote resolved env shim to ${filePath}`); +} + +function stateFilePath(): string { + return ( + process.env.DROID_STATE_FILE || + path.join(process.env.CI_PROJECT_DIR || "/tmp", ".droid-state.json") + ); +} + +async function writeState(state: PrepareState): Promise { + const filePath = stateFilePath(); + await fs.mkdir(path.dirname(filePath), { recursive: true }); + await fs.writeFile(filePath, JSON.stringify(state, null, 2)); + console.log(`Wrote droid state to ${filePath}`); +} + +async function run(): Promise { + const context = parseGitlabContext(); + + const baseState: PrepareState = { + shouldRunReview: false, + projectId: context.project.id, + projectPath: context.project.pathWithNamespace, + mrIid: context.mr?.iid ?? null, + trackingNoteId: null, + diffBaseSha: context.mr?.diffBaseSha ?? null, + sourceBranchSha: context.mr?.sourceBranchSha ?? null, + pipelineUrl: context.pipelineUrl, + jobUrl: context.jobUrl, + promptPath: null, + resolvedModel: null, + resolvedReasoningEffort: null, + diffPath: null, + commentsPath: null, + descriptionPath: null, + candidatesPath: null, + validatedPath: null, + mrTitle: context.mr?.title ?? null, + sourceBranch: null, + targetBranch: null, + headSha: null, + includeSuggestions: true, + securityReviewEnabled: false, + }; + + if (!isMergeRequestContext(context)) { + console.log( + "Not a merge_request_event pipeline; skipping droid review prepare.", + ); + await writeState({ + ...baseState, + reason: "not-merge-request-event", + }); + return; + } + + if (!context.inputs.automaticReview) { + console.log("automatic_review is disabled; skipping droid review prepare."); + await writeState({ + ...baseState, + reason: "automatic-review-disabled", + }); + return; + } + + let token: string; + try { + token = setupGitlabToken(); + } catch (err) { + if (err instanceof MissingGitlabTokenError) { + console.error(err.message); + } + throw err; + } + + // Write ~/.factory/droid/settings.json. Accepts either a raw JSON string + // or a path to a JSON file (matching the GitHub action's `settings` + // input). Always sets enableAllProjectMcpServers=true so project-scope + // MCP servers (if any) are trusted without prompting. + try { + await setupDroidSettings(context.inputs.settings || undefined); + } catch (err) { + console.warn( + "Failed to setup droid settings; continuing with defaults:", + err, + ); + } + + const client = new GitlabClient(token, context.apiUrl); + const mrIid = context.mr.iid; + const projectId = context.project.id; + + const existingNotes = await client.listNotes(projectId, mrIid); + const existing = findExistingTrackingNote(existingNotes); + + const includeSuggestions = + (process.env.INCLUDE_SUGGESTIONS ?? "true").toLowerCase() !== "false"; + const securityReviewEnabled = context.inputs.automaticSecurityReview; + + const noteBody = buildTrackingNoteBody({ + state: "running", + pipelineUrl: context.pipelineUrl, + jobUrl: context.jobUrl, + triggerUsername: context.user.login, + securityReviewRan: securityReviewEnabled, + }); + + let trackingNoteId: number; + if (existing) { + await client.updateNote(projectId, mrIid, existing.id, noteBody); + trackingNoteId = existing.id; + console.log(`Reused existing tracking note ${trackingNoteId}`); + } else { + const created = await client.createNote(projectId, mrIid, noteBody); + trackingNoteId = created.id; + console.log(`Created tracking note ${trackingNoteId}`); + } + + console.log("Computing review artifacts (diff, notes, description)..."); + const artifacts = await computeReviewArtifacts({ + client, + projectId, + mrIid, + outDir: promptsDir(), + }); + console.log( + `Artifacts written:\n ${artifacts.diffPath}\n ${artifacts.commentsPath}\n ${artifacts.descriptionPath}`, + ); + + const resolved = resolveReviewConfig({ + reviewModel: context.inputs.reviewModel, + reasoningEffort: context.inputs.reasoningEffort, + reviewDepth: context.inputs.reviewDepth, + }); + console.log( + `Resolved review config: depth=${context.inputs.reviewDepth} ` + + `explicitModel=${context.inputs.reviewModel || "(empty)"} ` + + `explicitEffort=${context.inputs.reasoningEffort || "(empty)"} ` + + `=> model=${resolved.model} effort=${resolved.reasoningEffort ?? "(none)"}`, + ); + + await writeResolvedEnvShim(resolved.model, resolved.reasoningEffort ?? null, { + DROID_MR_IID: String(mrIid), + DROID_TRACKING_NOTE_ID: String(trackingNoteId), + }); + + const candidatesPath = candidatesFilePath(); + const validatedPath = validatedFilePath(); + + const promptCtx: GitlabReviewPromptContext = { + projectPath: context.project.pathWithNamespace, + mrIid, + mrTitle: artifacts.mr.title ?? context.mr.title ?? "", + sourceBranch: artifacts.mr.source_branch ?? "", + targetBranch: artifacts.mr.target_branch ?? "", + headSha: + artifacts.mr.diff_refs?.head_sha ?? context.mr.sourceBranchSha ?? "", + diffPath: artifacts.diffPath, + commentsPath: artifacts.commentsPath, + descriptionPath: artifacts.descriptionPath, + candidatesPath, + validatedPath, + includeSuggestions, + securityReviewEnabled, + }; + + const prompt = generateGitlabReviewCandidatesPrompt(promptCtx); + const promptPath = promptFilePath(); + await fs.mkdir(path.dirname(promptPath), { recursive: true }); + await fs.writeFile(promptPath, prompt); + console.log( + `Wrote Pass-1 candidates prompt (${prompt.length} bytes) to ${promptPath}`, + ); + + await writeState({ + ...baseState, + shouldRunReview: true, + trackingNoteId, + promptPath, + resolvedModel: resolved.model, + resolvedReasoningEffort: resolved.reasoningEffort ?? null, + diffPath: artifacts.diffPath, + commentsPath: artifacts.commentsPath, + descriptionPath: artifacts.descriptionPath, + candidatesPath, + validatedPath, + mrTitle: promptCtx.mrTitle, + sourceBranch: promptCtx.sourceBranch, + targetBranch: promptCtx.targetBranch, + headSha: promptCtx.headSha, + includeSuggestions, + securityReviewEnabled, + }); +} + +if (import.meta.main) { + run().catch((error) => { + console.error("gitlab-prepare failed:", error); + process.exit(1); + }); +} + +export { run }; diff --git a/src/entrypoints/gitlab-update-comment-link.ts b/src/entrypoints/gitlab-update-comment-link.ts new file mode 100644 index 0000000..a190552 --- /dev/null +++ b/src/entrypoints/gitlab-update-comment-link.ts @@ -0,0 +1,128 @@ +#!/usr/bin/env bun + +/** + * Post-step for the GitLab CI/CD Component: edit the sticky tracking note + * to reflect the final outcome (success/failure) and link to the pipeline. + * + * Inputs (env): + * GITLAB_TOKEN - access token (api scope) + * DROID_STATE_FILE - JSON state written by gitlab-prepare + * DROID_SUCCESS - "true" | "false" set by the CI job + * DROID_ERROR_DETAILS - optional error blob to embed on failure + * AUTOMATIC_SECURITY_REVIEW - "true" to render the security badge + * TRIGGER_USERNAME - optional, e.g. GITLAB_USER_LOGIN + * CI_PIPELINE_URL / CI_JOB_URL - used to keep links fresh + */ + +import * as fs from "fs/promises"; +import * as path from "path"; +import { setupGitlabToken } from "../gitlab/token"; +import { GitlabClient } from "../gitlab/api/client"; +import { buildTrackingNoteBody } from "../gitlab/operations/tracking-note"; +import { collectExecTelemetry } from "../gitlab/data/exec-telemetry"; + +type PrepareState = { + shouldRunReview: boolean; + projectId: string; + projectPath: string; + mrIid: number | null; + trackingNoteId: number | null; + pipelineUrl: string | null; + jobUrl: string | null; + reason?: string; +}; + +function stateFilePath(): string { + return ( + process.env.DROID_STATE_FILE || + path.join(process.env.CI_PROJECT_DIR || "/tmp", ".droid-state.json") + ); +} + +async function readState(): Promise { + const filePath = stateFilePath(); + try { + const raw = await fs.readFile(filePath, "utf8"); + return JSON.parse(raw) as PrepareState; + } catch (err) { + console.warn(`Could not read droid state file at ${filePath}:`, err); + return null; + } +} + +async function run(): Promise { + const state = await readState(); + if (!state) { + console.log("No droid state available; nothing to update."); + return; + } + + if (!state.shouldRunReview || !state.mrIid || !state.trackingNoteId) { + console.log( + `Skipping note update (shouldRunReview=${state.shouldRunReview}, ` + + `mrIid=${state.mrIid}, trackingNoteId=${state.trackingNoteId}).`, + ); + return; + } + + const token = setupGitlabToken(); + const apiUrl = + process.env.CI_API_V4_URL || + process.env.GITLAB_API_URL || + "https://gitlab.com/api/v4"; + + const client = new GitlabClient(token, apiUrl); + + const droidSuccess = process.env.DROID_SUCCESS !== "false"; + const errorDetails = process.env.DROID_ERROR_DETAILS || null; + const securityReviewRan = process.env.AUTOMATIC_SECURITY_REVIEW === "true"; + const triggerUsername = + process.env.TRIGGER_USERNAME || process.env.GITLAB_USER_LOGIN || null; + + const pipelineUrl = process.env.CI_PIPELINE_URL || state.pipelineUrl; + const jobUrl = process.env.CI_JOB_URL || state.jobUrl; + + const telemetry = await collectExecTelemetry({ + pass1LogPath: + process.env.DROID_PASS1_LOG || "/tmp/droid-prompts/pass1-output.jsonl", + pass2LogPath: + process.env.DROID_PASS2_LOG || "/tmp/droid-prompts/pass2-output.jsonl", + }); + + const body = buildTrackingNoteBody({ + state: droidSuccess ? "success" : "failure", + pipelineUrl, + jobUrl, + triggerUsername, + errorDetails, + securityReviewRan, + telemetry: { + totalNumTurns: telemetry.totalNumTurns, + totalDurationMs: telemetry.totalDurationMs, + totalCostUsd: telemetry.totalCostUsd, + pass1SessionId: telemetry.pass1?.sessionId ?? null, + pass2SessionId: telemetry.pass2?.sessionId ?? null, + }, + }); + + await client.updateNote( + state.projectId, + state.mrIid, + state.trackingNoteId, + body, + ); + + console.log( + `Updated tracking note ${state.trackingNoteId} on MR !${state.mrIid} ` + + `(state=${droidSuccess ? "success" : "failure"}).`, + ); +} + +if (import.meta.main) { + run().catch((error) => { + console.error("gitlab-update-comment-link failed:", error); + process.exit(1); + }); +} + +export { run }; diff --git a/src/gitlab/api/client.ts b/src/gitlab/api/client.ts new file mode 100644 index 0000000..0747d4e --- /dev/null +++ b/src/gitlab/api/client.ts @@ -0,0 +1,156 @@ +import { GITLAB_API_URL } from "./config"; +import type { + GitlabMr, + GitlabMrChanges, + GitlabNote, + GitlabDiscussion, + GitlabPosition, +} from "../types"; + +export class GitlabApiError extends Error { + status: number; + body: unknown; + + constructor(status: number, message: string, body: unknown) { + super(`GitLab API ${status}: ${message}`); + this.name = "GitlabApiError"; + this.status = status; + this.body = body; + } +} + +type RequestInitWithJson = Omit & { json?: unknown }; + +export class GitlabClient { + private readonly baseUrl: string; + private readonly token: string; + + constructor(token: string, baseUrl: string = GITLAB_API_URL) { + this.token = token; + this.baseUrl = baseUrl.replace(/\/+$/, ""); + } + + private async request( + path: string, + init: RequestInitWithJson = {}, + ): Promise { + const { json, headers: rawHeaders, ...rest } = init; + const headers: Record = { + "PRIVATE-TOKEN": this.token, + Accept: "application/json", + ...(rawHeaders as Record | undefined), + }; + + let body: string | undefined; + if (json !== undefined) { + headers["Content-Type"] = "application/json"; + body = JSON.stringify(json); + } + + const url = path.startsWith("http") ? path : `${this.baseUrl}${path}`; + const response = await fetch(url, { + ...rest, + headers, + body, + }); + + if (!response.ok) { + let parsed: unknown = null; + const text = await response.text(); + try { + parsed = text ? JSON.parse(text) : null; + } catch { + parsed = text; + } + throw new GitlabApiError( + response.status, + response.statusText || "Request failed", + parsed, + ); + } + + if (response.status === 204) { + return undefined as unknown as T; + } + + return (await response.json()) as T; + } + + private projectPath(projectId: string | number): string { + return `/projects/${encodeURIComponent(String(projectId))}`; + } + + getMr(projectId: string | number, mrIid: number): Promise { + return this.request( + `${this.projectPath(projectId)}/merge_requests/${mrIid}`, + ); + } + + getMrChanges( + projectId: string | number, + mrIid: number, + ): Promise { + return this.request( + `${this.projectPath(projectId)}/merge_requests/${mrIid}/changes`, + ); + } + + listNotes(projectId: string | number, mrIid: number): Promise { + return this.request( + `${this.projectPath(projectId)}/merge_requests/${mrIid}/notes?per_page=100`, + ); + } + + createNote( + projectId: string | number, + mrIid: number, + body: string, + ): Promise { + return this.request( + `${this.projectPath(projectId)}/merge_requests/${mrIid}/notes`, + { method: "POST", json: { body } }, + ); + } + + updateNote( + projectId: string | number, + mrIid: number, + noteId: number, + body: string, + ): Promise { + return this.request( + `${this.projectPath(projectId)}/merge_requests/${mrIid}/notes/${noteId}`, + { method: "PUT", json: { body } }, + ); + } + + createDiscussionOnDiff( + projectId: string | number, + mrIid: number, + body: string, + position: GitlabPosition, + ): Promise { + return this.request( + `${this.projectPath(projectId)}/merge_requests/${mrIid}/discussions`, + { method: "POST", json: { body, position } }, + ); + } + + updateMrDescription( + projectId: string | number, + mrIid: number, + description: string, + ): Promise { + return this.request( + `${this.projectPath(projectId)}/merge_requests/${mrIid}`, + { method: "PUT", json: { description } }, + ); + } +} + +export function createGitlabClient( + token: string, + baseUrl?: string, +): GitlabClient { + return new GitlabClient(token, baseUrl); +} diff --git a/src/gitlab/api/config.ts b/src/gitlab/api/config.ts new file mode 100644 index 0000000..24c18a4 --- /dev/null +++ b/src/gitlab/api/config.ts @@ -0,0 +1,7 @@ +export const GITLAB_API_URL = ( + process.env.CI_API_V4_URL || "https://gitlab.com/api/v4" +).replace(/\/+$/, ""); + +export const GITLAB_SERVER_URL = ( + process.env.CI_SERVER_URL || "https://gitlab.com" +).replace(/\/+$/, ""); diff --git a/src/gitlab/context.ts b/src/gitlab/context.ts new file mode 100644 index 0000000..1a1576f --- /dev/null +++ b/src/gitlab/context.ts @@ -0,0 +1,148 @@ +export type ParsedGitlabContext = { + serverUrl: string; + apiUrl: string; + pipelineId: string | null; + pipelineUrl: string | null; + jobId: string | null; + jobUrl: string | null; + pipelineSource: string | null; + project: { + id: string; + path: string; + pathWithNamespace: string; + webUrl: string; + }; + mr: { + iid: number; + sourceBranchSha: string | null; + targetBranchSha: string | null; + diffBaseSha: string | null; + title: string | null; + } | null; + commit: { + sha: string; + branch: string | null; + tag: string | null; + }; + user: { + login: string | null; + name: string | null; + email: string | null; + }; + inputs: { + automaticReview: boolean; + automaticSecurityReview: boolean; + triggerPhrase: string; + reviewDepth: string; + reviewModel: string; + reasoningEffort: string; + fillModel: string; + securityModel: string; + securitySeverityThreshold: string; + securityBlockOnCritical: boolean; + securityBlockOnHigh: boolean; + securityNotifyTeam: string; + securityScanSchedule: boolean; + securityScanDays: number; + settings: string; + }; +}; + +function required(name: string): string { + const value = process.env[name]; + if (!value) { + throw new Error( + `Missing required GitLab CI environment variable: ${name}. Are you running inside a GitLab CI job?`, + ); + } + return value; +} + +function optional(name: string): string | null { + const value = process.env[name]; + return value && value.length > 0 ? value : null; +} + +export function parseGitlabContext(): ParsedGitlabContext { + const serverUrl = (process.env.CI_SERVER_URL || "https://gitlab.com").replace( + /\/+$/, + "", + ); + const apiUrl = (process.env.CI_API_V4_URL || `${serverUrl}/api/v4`).replace( + /\/+$/, + "", + ); + + const projectId = required("CI_PROJECT_ID"); + const projectPath = required("CI_PROJECT_PATH"); + const projectName = + process.env.CI_PROJECT_NAME || projectPath.split("/").pop()!; + const projectUrl = + process.env.CI_PROJECT_URL || `${serverUrl}/${projectPath}`; + + const mrIid = optional("CI_MERGE_REQUEST_IID"); + const mr = mrIid + ? { + iid: parseInt(mrIid, 10), + sourceBranchSha: optional("CI_MERGE_REQUEST_SOURCE_BRANCH_SHA"), + targetBranchSha: optional("CI_MERGE_REQUEST_TARGET_BRANCH_SHA"), + diffBaseSha: optional("CI_MERGE_REQUEST_DIFF_BASE_SHA"), + title: optional("CI_MERGE_REQUEST_TITLE"), + } + : null; + + return { + serverUrl, + apiUrl, + pipelineId: optional("CI_PIPELINE_ID"), + pipelineUrl: optional("CI_PIPELINE_URL"), + jobId: optional("CI_JOB_ID"), + jobUrl: optional("CI_JOB_URL"), + pipelineSource: optional("CI_PIPELINE_SOURCE"), + project: { + id: projectId, + path: projectName, + pathWithNamespace: projectPath, + webUrl: projectUrl, + }, + mr, + commit: { + sha: required("CI_COMMIT_SHA"), + branch: optional("CI_COMMIT_BRANCH"), + tag: optional("CI_COMMIT_TAG"), + }, + user: { + login: optional("GITLAB_USER_LOGIN"), + name: optional("GITLAB_USER_NAME"), + email: optional("GITLAB_USER_EMAIL"), + }, + inputs: { + automaticReview: process.env.AUTOMATIC_REVIEW === "true", + automaticSecurityReview: process.env.AUTOMATIC_SECURITY_REVIEW === "true", + triggerPhrase: process.env.TRIGGER_PHRASE ?? "@droid", + reviewDepth: process.env.REVIEW_DEPTH ?? "deep", + reviewModel: process.env.REVIEW_MODEL ?? "", + reasoningEffort: process.env.REASONING_EFFORT ?? "", + fillModel: process.env.FILL_MODEL ?? "", + securityModel: process.env.SECURITY_MODEL ?? "", + securitySeverityThreshold: + process.env.SECURITY_SEVERITY_THRESHOLD ?? "medium", + securityBlockOnCritical: + process.env.SECURITY_BLOCK_ON_CRITICAL !== "false", + securityBlockOnHigh: process.env.SECURITY_BLOCK_ON_HIGH === "true", + securityNotifyTeam: process.env.SECURITY_NOTIFY_TEAM ?? "", + securityScanSchedule: process.env.SECURITY_SCAN_SCHEDULE === "true", + securityScanDays: Math.max( + 1, + parseInt(process.env.SECURITY_SCAN_DAYS ?? "7", 10) || 7, + ), + settings: process.env.DROID_SETTINGS ?? "", + }, + }; +} + +export function isMergeRequestContext( + ctx: ParsedGitlabContext, +): ctx is ParsedGitlabContext & { mr: NonNullable } { + return ctx.mr !== null; +} diff --git a/src/gitlab/data/exec-telemetry.ts b/src/gitlab/data/exec-telemetry.ts new file mode 100644 index 0000000..76bf30a --- /dev/null +++ b/src/gitlab/data/exec-telemetry.ts @@ -0,0 +1,149 @@ +/** + * Parse `droid exec --output-format stream-json` output captured to a + * JSONL log file by the GitLab CI template, and extract a small summary + * of execution telemetry (session IDs, turn counts, durations, token + * usage). The result is rendered into the sticky tracking note so a + * reviewer can sanity-check what Droid did without opening the full + * job log. + * + * Mirrors the surface area GitHub's `update-comment-link.ts` extracts + * from its array-format result file, parameterized for our stream-json + * one-event-per-line format. + */ + +import * as fs from "fs/promises"; + +export type PassTelemetry = { + sessionId: string | null; + numTurns: number | null; + durationMs: number | null; + inputTokens: number | null; + outputTokens: number | null; + cacheReadInputTokens: number | null; + costUsd: number | null; +}; + +export type ExecTelemetry = { + pass1: PassTelemetry | null; + pass2: PassTelemetry | null; + totalCostUsd: number | null; + totalDurationMs: number | null; + totalNumTurns: number | null; +}; + +const EMPTY_PASS: PassTelemetry = { + sessionId: null, + numTurns: null, + durationMs: null, + inputTokens: null, + outputTokens: null, + cacheReadInputTokens: null, + costUsd: null, +}; + +function readNumber(value: unknown): number | null { + if (typeof value === "number" && Number.isFinite(value)) { + return value; + } + return null; +} + +function readString(value: unknown): string | null { + if (typeof value === "string" && value.length > 0) { + return value; + } + return null; +} + +export function extractPassTelemetry(lines: string[]): PassTelemetry | null { + let result: PassTelemetry | null = null; + + for (let i = lines.length - 1; i >= 0; i--) { + const line = lines[i]?.trim(); + if (!line || !line.startsWith("{")) continue; + let event: Record; + try { + event = JSON.parse(line) as Record; + } catch { + continue; + } + if (event.type !== "completion" && event.type !== "result") continue; + + const usage = (event.usage as Record | undefined) ?? {}; + result = { + sessionId: readString(event.session_id) ?? readString(event.sessionId), + numTurns: readNumber(event.numTurns) ?? readNumber(event.num_turns), + durationMs: readNumber(event.durationMs) ?? readNumber(event.duration_ms), + inputTokens: + readNumber(usage.input_tokens) ?? readNumber(usage.inputTokens), + outputTokens: + readNumber(usage.output_tokens) ?? readNumber(usage.outputTokens), + cacheReadInputTokens: + readNumber(usage.cache_read_input_tokens) ?? + readNumber(usage.cacheReadInputTokens), + costUsd: readNumber(event.cost_usd) ?? readNumber(event.costUsd), + }; + break; + } + + return result; +} + +export async function parseTelemetryFile( + filePath: string, +): Promise { + try { + const raw = await fs.readFile(filePath, "utf8"); + const lines = raw.split(/\r?\n/); + return extractPassTelemetry(lines); + } catch { + return null; + } +} + +export async function collectExecTelemetry(opts: { + pass1LogPath?: string | null; + pass2LogPath?: string | null; +}): Promise { + const pass1 = opts.pass1LogPath + ? await parseTelemetryFile(opts.pass1LogPath) + : null; + const pass2 = opts.pass2LogPath + ? await parseTelemetryFile(opts.pass2LogPath) + : null; + + const sumNullable = (a: number | null, b: number | null): number | null => { + if (a === null && b === null) return null; + return (a ?? 0) + (b ?? 0); + }; + + return { + pass1, + pass2, + totalCostUsd: sumNullable(pass1?.costUsd ?? null, pass2?.costUsd ?? null), + totalDurationMs: sumNullable( + pass1?.durationMs ?? null, + pass2?.durationMs ?? null, + ), + totalNumTurns: sumNullable( + pass1?.numTurns ?? null, + pass2?.numTurns ?? null, + ), + }; +} + +export function isEmptyPass(p: PassTelemetry | null | undefined): boolean { + if (!p) return true; + return ( + p.sessionId === null && + p.numTurns === null && + p.durationMs === null && + p.inputTokens === null && + p.outputTokens === null && + p.costUsd === null + ); +} + +export function emptyPass(): PassTelemetry { + return { ...EMPTY_PASS }; +} diff --git a/src/gitlab/data/review-artifacts.ts b/src/gitlab/data/review-artifacts.ts new file mode 100644 index 0000000..be45775 --- /dev/null +++ b/src/gitlab/data/review-artifacts.ts @@ -0,0 +1,94 @@ +/** + * Compute and persist the three artifact files that the GitLab review + * prompts read from disk: + * + * - mr.diff : concatenated unified diff for every changed file + * - existing_comments.json : array of notes already on the MR (system+human) + * - mr_description.txt : MR title + description + * + * These mirror the artifacts the GitHub Action precomputes + * (`pr.diff`, `existing_comments.json`, `pr_description.txt`) so the GitLab + * Pass-1 / Pass-2 prompts can refer to them by stable paths and the model + * doesn't have to round-trip through MCP tools to fetch them. + */ + +import * as fs from "fs/promises"; +import * as path from "path"; +import type { GitlabClient } from "../api/client"; +import type { GitlabMr, GitlabMrChanges, GitlabNote } from "../types"; + +export type GitlabReviewArtifactPaths = { + diffPath: string; + commentsPath: string; + descriptionPath: string; +}; + +export type GitlabReviewArtifacts = GitlabReviewArtifactPaths & { + mr: GitlabMr; + changes: GitlabMrChanges; + notes: GitlabNote[]; +}; + +export type ComputeReviewArtifactsOptions = { + client: GitlabClient; + projectId: string | number; + mrIid: number; + outDir: string; +}; + +export function buildDiffContent(changes: GitlabMrChanges): string { + const files = changes.changes ?? []; + if (files.length === 0) { + return ""; + } + return files + .map((c) => { + const oldPath = c.old_path || c.new_path; + const newPath = c.new_path || c.old_path; + const header = `diff --git a/${oldPath} b/${newPath}`; + return `${header}\n${c.diff}`; + }) + .join("\n"); +} + +export function buildDescriptionContent(mr: GitlabMr): string { + const title = mr.title ?? ""; + const description = mr.description ?? ""; + return `Title: ${title}\n\n${description}\n`; +} + +export async function computeReviewArtifacts( + opts: ComputeReviewArtifactsOptions, +): Promise { + const { client, projectId, mrIid, outDir } = opts; + + await fs.mkdir(outDir, { recursive: true }); + + const [mr, changes, notes] = await Promise.all([ + client.getMr(projectId, mrIid), + client.getMrChanges(projectId, mrIid), + client.listNotes(projectId, mrIid), + ]); + + const diffPath = path.join(outDir, "mr.diff"); + const commentsPath = path.join(outDir, "existing_comments.json"); + const descriptionPath = path.join(outDir, "mr_description.txt"); + + const diffContent = buildDiffContent(changes); + const descriptionContent = buildDescriptionContent(mr); + + await Promise.all([ + fs.writeFile(diffPath, diffContent), + fs.writeFile(commentsPath, JSON.stringify(notes, null, 2)), + fs.writeFile(descriptionPath, descriptionContent), + ]); + + return { + diffPath, + commentsPath, + descriptionPath, + mr, + changes, + notes, + }; +} diff --git a/src/gitlab/operations/tracking-note.ts b/src/gitlab/operations/tracking-note.ts new file mode 100644 index 0000000..286da9d --- /dev/null +++ b/src/gitlab/operations/tracking-note.ts @@ -0,0 +1,118 @@ +/** + * Sticky tracking note helpers for GitLab MR pipelines. + * + * The tracking note carries a hidden HTML marker so we can find and + * update the same note across retries instead of creating duplicates. + */ + +export const DROID_TRACKING_MARKER = ""; +export const DROID_SECURITY_BADGE_MARKER = ""; + +export type TrackingNoteState = "running" | "success" | "failure"; + +export type TrackingNoteTelemetry = { + totalNumTurns?: number | null; + totalDurationMs?: number | null; + totalCostUsd?: number | null; + pass1SessionId?: string | null; + pass2SessionId?: string | null; +}; + +export interface TrackingNoteOptions { + state: TrackingNoteState; + pipelineUrl?: string | null; + jobUrl?: string | null; + triggerUsername?: string | null; + errorDetails?: string | null; + securityReviewRan?: boolean; + telemetry?: TrackingNoteTelemetry | null; +} + +function formatDurationMs(ms: number): string { + if (ms < 1000) return `${ms}ms`; + const seconds = ms / 1000; + if (seconds < 60) return `${seconds.toFixed(1)}s`; + const minutes = Math.floor(seconds / 60); + const remSec = Math.round(seconds - minutes * 60); + return `${minutes}m ${remSec}s`; +} + +function formatCostUsd(usd: number): string { + if (usd >= 1) return `$${usd.toFixed(2)}`; + return `$${usd.toFixed(4)}`; +} + +const SECURITY_BADGE = + "![security](https://img.shields.io/badge/security%20review-enabled-blue?style=flat-square&logo=shield) "; + +const STATE_HEADER: Record = { + running: + "**Droid is reviewing this merge request...** :hourglass_flowing_sand:", + success: "**Droid finished reviewing this merge request** :white_check_mark:", + failure: "**Droid encountered an error reviewing this MR** :x:", +}; + +export function buildTrackingNoteBody(options: TrackingNoteOptions): string { + const lines: string[] = []; + + if (options.securityReviewRan) { + lines.push(`${DROID_SECURITY_BADGE_MARKER}${SECURITY_BADGE}`); + } + + lines.push(DROID_TRACKING_MARKER); + lines.push(""); + lines.push(STATE_HEADER[options.state]); + lines.push(""); + + if (options.triggerUsername) { + lines.push(`Triggered by @${options.triggerUsername}.`); + } + + if (options.pipelineUrl) { + lines.push(`Pipeline: ${options.pipelineUrl}`); + } + if (options.jobUrl) { + lines.push(`Job log: ${options.jobUrl}`); + } + + if (options.state === "failure" && options.errorDetails) { + lines.push(""); + lines.push("
Error details"); + lines.push(""); + lines.push("```"); + lines.push(options.errorDetails.trim()); + lines.push("```"); + lines.push("
"); + } + + if (options.telemetry) { + const t = options.telemetry; + const bits: string[] = []; + if (typeof t.totalNumTurns === "number") + bits.push(`${t.totalNumTurns} turns`); + if (typeof t.totalDurationMs === "number") + bits.push(formatDurationMs(t.totalDurationMs)); + if (typeof t.totalCostUsd === "number" && t.totalCostUsd > 0) + bits.push(formatCostUsd(t.totalCostUsd)); + if (bits.length > 0) { + lines.push(""); + lines.push(`${bits.join(" • ")}`); + } + if (t.pass1SessionId || t.pass2SessionId) { + lines.push(""); + lines.push("
Droid session IDs"); + lines.push(""); + if (t.pass1SessionId) lines.push(`- Pass 1: \`${t.pass1SessionId}\``); + if (t.pass2SessionId) lines.push(`- Pass 2: \`${t.pass2SessionId}\``); + lines.push("
"); + } + } + + return lines.join("\n").trim() + "\n"; +} + +export function findExistingTrackingNote< + T extends { id: number; body: string }, +>(notes: T[]): T | undefined { + return notes.find((n) => n.body && n.body.includes(DROID_TRACKING_MARKER)); +} diff --git a/src/gitlab/prompts/candidates.ts b/src/gitlab/prompts/candidates.ts new file mode 100644 index 0000000..aa5966a --- /dev/null +++ b/src/gitlab/prompts/candidates.ts @@ -0,0 +1,148 @@ +/** + * GitLab Pass-1 (candidate generation) prompt. + * + * Direct port of the GitHub Action's + * `src/create-prompt/templates/review-candidates-prompt.ts` with PR→MR + * terminology and GitLab-specific entity numbering. The /review skill + * itself is platform-agnostic and contains the two-pass methodology; this + * file is the runtime harness that tells Droid which pass it's in, where + * the input artifacts live on disk, and where to write the candidate JSON. + * + * STRICT contract: this prompt MUST NOT instruct the model to call the + * GitLab posting tool (`gitlab_mr___submit_review`). Posting only happens + * in Pass 2. Tool gating is also enforced at the `droid exec` level by + * excluding `gitlab_mr___submit_review` from `--enabled-tools` during + * Pass 1. + */ + +import type { GitlabReviewPromptContext } from "./types"; + +export function generateGitlabReviewCandidatesPrompt( + ctx: GitlabReviewPromptContext, +): string { + const { + projectPath, + mrIid, + sourceBranch, + targetBranch, + headSha, + diffPath, + commentsPath, + descriptionPath, + candidatesPath, + includeSuggestions, + securityReviewEnabled, + } = ctx; + + const bodyFieldDescription = includeSuggestions + ? " - `body`: Comment text starting with priority tag [P0|P1|P2], then title, then 1 paragraph explanation.\n" + + " Follow the suggestion block rules from the review skill when including suggestions." + : " - `body`: Comment text starting with priority tag [P0|P1|P2], then title, then 1 paragraph explanation"; + + const sideFieldDescription = includeSuggestions + ? ' - `side`: "RIGHT" for new/modified code (default). Use "LEFT" only for removed code **without** suggestions.\n' + + " If you include a suggestion block, choose a RIGHT-side anchor and keep it unchanged so the validator can reuse it." + : ' - `side`: "RIGHT" for new/modified code (default), "LEFT" only for removed code'; + + const skillInstruction = includeSuggestions + ? "Invoke the 'review' skill to load the review methodology, then execute its **Pass 1: Candidate Generation** procedure — including suggestion block rules." + : "Invoke the 'review' skill to load the review methodology, then execute its **Pass 1: Candidate Generation** procedure. Do NOT include code suggestion blocks."; + + const securitySubagentInstruction = securityReviewEnabled + ? ` + +## Security Review (run concurrently) + +In addition to the code review, you MUST also spawn a \`security-reviewer\` subagent via the Task tool. +This subagent runs **concurrently** with the code review subagents during Step 2. + +Spawn it with: +- \`subagent_type\`: "security-reviewer" +- \`description\`: "Security review" +- \`prompt\`: Include the full MR context (project, MR IID, head SHA, target branch) and the paths to precomputed data files (diff, description, existing comments). The security-reviewer will invoke the security-review skill and return a JSON array of security findings. + +**IMPORTANT**: Spawn the security-reviewer in the SAME response as the code review subagents so they all run in parallel. + +After all subagents complete (both code review and security-reviewer), merge the security findings into the \`comments\` array alongside code review findings. Security findings use the same schema but are prefixed with \`[security]\` in their body (e.g., \`[P1] [security] Title\`). +` + : ""; + + return `You are a senior staff software engineer and expert code reviewer. + +Your task: Review MR !${mrIid} in ${projectPath} and generate a JSON file with **high-confidence, actionable** review comments that pinpoint genuine issues. + +${skillInstruction}${securitySubagentInstruction} + + +Project: ${projectPath} +MR IID: ${mrIid} +MR Source Branch: ${sourceBranch} +MR Head SHA: ${headSha} +MR Target Branch: ${targetBranch} + +Precomputed data files: +- MR Description: \`${descriptionPath}\` +- Full MR Diff: \`${diffPath}\` +- Existing Comments: \`${commentsPath}\` + + + +Write output to \`${candidatesPath}\` using this exact schema: + +\`\`\`json +{ + "version": 1, + "meta": { + "project": "group/project", + "mrIid": 123, + "headSha": "", + "targetBranch": "main", + "generatedAt": "" + }, + "comments": [ + { + "path": "src/index.ts", + "body": "[P1] Title\\n\\n1 paragraph.", + "line": 42, + "startLine": null, + "side": "RIGHT", + "commit_id": "" + } + ], + "reviewSummary": { + "body": "1-3 sentence overall assessment" + } +} +\`\`\` + + +- **version**: Always \`1\` + +- **meta**: Metadata object + - \`project\`: "${projectPath}" + - \`mrIid\`: ${mrIid} + - \`headSha\`: "${headSha}" + - \`targetBranch\`: "${targetBranch}" + - \`generatedAt\`: ISO 8601 timestamp (e.g., "2024-01-15T10:30:00Z") + +- **comments**: Array of comment objects + - \`path\`: Relative file path (use the new_path from the diff, e.g., "src/index.ts") +${bodyFieldDescription} + - \`line\`: Target line number in the new file (single-line) or end line number (multi-line). Must be ≥ 0. + - \`startLine\`: \`null\` for single-line comments, or start line number for multi-line comments +${sideFieldDescription} + - \`commit_id\`: "${headSha}" + +- **reviewSummary**: + - \`body\`: 1-3 sentence overall assessment + + + + +**DO NOT** post to GitLab. +**DO NOT** invoke any MR mutation tools (\`gitlab_mr___submit_review\`, \`gitlab_mr___create_mr_note\`, \`gitlab_mr___update_mr_note\`, \`gitlab_mr___update_mr_description\`, \`gitlab_mr___update_tracking_note\`, etc.). +**DO NOT** modify any files other than writing to \`${candidatesPath}\`. +Output ONLY the JSON file—no additional commentary. + +`; +} diff --git a/src/gitlab/prompts/types.ts b/src/gitlab/prompts/types.ts new file mode 100644 index 0000000..9105ed2 --- /dev/null +++ b/src/gitlab/prompts/types.ts @@ -0,0 +1,29 @@ +/** + * Shared shape passed to both the Pass-1 candidate-generation prompt and + * the Pass-2 validator prompt. Mirrors the subset of GitHub's + * `PreparedContext` that the review templates actually consume, but with + * GitLab terminology (MR instead of PR, project path instead of repo + * full_name) and without coupling to the GitHub action's webhook payload + * types. + * + * Kept platform-specific on purpose for v1; a follow-up will extract a + * platform-neutral context into `src/core/review/`. + */ + +export type GitlabReviewPromptContext = { + projectPath: string; + mrIid: number; + mrTitle: string; + sourceBranch: string; + targetBranch: string; + headSha: string; + + diffPath: string; + commentsPath: string; + descriptionPath: string; + candidatesPath: string; + validatedPath: string; + + includeSuggestions: boolean; + securityReviewEnabled: boolean; +}; diff --git a/src/gitlab/prompts/validator.ts b/src/gitlab/prompts/validator.ts new file mode 100644 index 0000000..9606e96 --- /dev/null +++ b/src/gitlab/prompts/validator.ts @@ -0,0 +1,132 @@ +/** + * GitLab Pass-2 (validator) prompt. + * + * Direct port of the GitHub Action's + * `src/create-prompt/templates/review-validator-prompt.ts` with PR→MR + * terminology and the GitLab MCP tool names. The validator reads the + * candidates JSON produced by Pass 1 from disk, validates each one, + * writes a refined JSON, and posts the approved findings as a single + * batched call to `gitlab_mr___submit_review`. + * + * Pass 2 is the only place where the posting tool is exposed (via + * `--enabled-tools` on the second `droid exec` invocation). + */ + +import type { GitlabReviewPromptContext } from "./types"; + +export function generateGitlabReviewValidatorPrompt( + ctx: GitlabReviewPromptContext, +): string { + const { + projectPath, + mrIid, + sourceBranch, + targetBranch, + headSha, + diffPath, + commentsPath, + descriptionPath, + candidatesPath, + validatedPath, + includeSuggestions, + } = ctx; + + const skillInstruction = includeSuggestions + ? "Invoke the 'review' skill to load the review methodology, then execute its **Pass 2: Validation** procedure — including suggestion block rules." + : "Invoke the 'review' skill to load the review methodology, then execute its **Pass 2: Validation** procedure. Do NOT include code suggestion blocks."; + + return `You are validating candidate review comments for MR !${mrIid} in ${projectPath}. + +IMPORTANT: This is Phase 2 (validator) of a two-pass review pipeline. + +${skillInstruction} + +### Context + +* Project: ${projectPath} +* MR IID: ${mrIid} +* MR Source Branch: ${sourceBranch} +* MR Head SHA: ${headSha} +* MR Target Branch: ${targetBranch} + +### Inputs + +Read these files before validating: +* MR Description: \`${descriptionPath}\` +* Candidates: \`${candidatesPath}\` +* Full MR Diff: \`${diffPath}\` +* Existing Comments: \`${commentsPath}\` + +If the diff is large, read in chunks (offset/limit). **Do not proceed until you have read the ENTIRE diff.** + +### Critical Requirements + +1. You MUST read and validate **every** candidate before posting anything. +2. Preserve ordering: keep results in the same order as candidates. +3. **Posting rule (STRICT):** Only post comments where \`status === "approved"\`. Never post rejected items. + +### Output: Write \`${validatedPath}\` + +\`\`\`json +{ + "version": 1, + "meta": { + "project": "${projectPath}", + "mrIid": ${mrIid}, + "headSha": "${headSha}", + "targetBranch": "${targetBranch}", + "validatedAt": "" + }, + "results": [ + { + "status": "approved", + "comment": { + "path": "src/index.ts", + "body": "[P1] Title\\n\\n1 paragraph.", + "line": 42, + "startLine": null, + "side": "RIGHT", + "commit_id": "${headSha}" + } + }, + { + "status": "rejected", + "candidate": { + "path": "src/other.ts", + "body": "[P2] ...", + "line": 10, + "startLine": null, + "side": "RIGHT", + "commit_id": "${headSha}" + }, + "reason": "Not a real bug because ..." + } + ], + "reviewSummary": { + "status": "approved", + "body": "1-3 sentence overall assessment" + } +} +\`\`\` + +Notes: +* Use \`commit_id\` = \`${headSha}\`. +* \`results\` MUST have exactly one entry per candidate, in the same order. + +Tooling note: +* If the tools list includes \`ApplyPatch\` (common for OpenAI models like GPT-5.2), use \`ApplyPatch\` to create/update the file at the exact path. +* Otherwise, use \`Create\` (or \`Edit\` if overwriting) to write the file. + +### Post approved items + +After writing \`${validatedPath}\`, post comments ONLY for \`status === "approved"\`: + +* Collect all approved comments and submit them as a **single batched review** via \`gitlab_mr___submit_review\`, passing them in the \`comments\` array parameter along with \`mr_iid: ${mrIid}\`. +* Do **NOT** post comments individually — batch them all into one \`submit_review\` call. +* Do **NOT** include a \`body\` parameter in \`submit_review\` (we use a separate tracking note for the summary). +* Use \`gitlab_mr___update_tracking_note\` to update the sticky tracking note with the review summary. +* If any approved comments contain \`[security]\` in their body, prepend a security badge to the tracking note: \`![Security Review](https://img.shields.io/badge/security%20review-ran-blue)\`. This indicates that security analysis was performed as part of the review. +* Do **NOT** post the summary as a separate top-level note. +* Do not approve the MR or request changes (GitLab approval rules are handled out-of-band). +`; +} diff --git a/src/gitlab/token.ts b/src/gitlab/token.ts new file mode 100644 index 0000000..0d73ed6 --- /dev/null +++ b/src/gitlab/token.ts @@ -0,0 +1,20 @@ +#!/usr/bin/env bun + +export class MissingGitlabTokenError extends Error { + constructor() { + super( + "Missing GITLAB_TOKEN. Set a GitLab Project or Group access token with `api` scope as a masked CI/CD variable named GITLAB_TOKEN.", + ); + this.name = "MissingGitlabTokenError"; + } +} + +export function setupGitlabToken(): string { + const token = process.env.GITLAB_TOKEN || process.env.OVERRIDE_GITLAB_TOKEN; + + if (!token) { + throw new MissingGitlabTokenError(); + } + + return token; +} diff --git a/src/gitlab/types.ts b/src/gitlab/types.ts new file mode 100644 index 0000000..15a6449 --- /dev/null +++ b/src/gitlab/types.ts @@ -0,0 +1,81 @@ +export type GitlabUser = { + id: number; + username: string; + name?: string; +}; + +export type GitlabMr = { + id: number; + iid: number; + project_id: number; + title: string; + description: string | null; + state: string; + author: GitlabUser; + source_branch: string; + target_branch: string; + source_project_id: number; + target_project_id: number; + draft: boolean; + work_in_progress: boolean; + diff_refs: { + base_sha: string; + head_sha: string; + start_sha: string; + }; + web_url: string; + created_at: string; + updated_at: string; +}; + +export type GitlabMrDiff = { + old_path: string; + new_path: string; + a_mode: string; + b_mode: string; + diff: string; + new_file: boolean; + renamed_file: boolean; + deleted_file: boolean; +}; + +export type GitlabMrChanges = { + changes: GitlabMrDiff[]; + diff_refs: GitlabMr["diff_refs"]; +}; + +export type GitlabNote = { + id: number; + type: string | null; + body: string; + author: GitlabUser; + created_at: string; + updated_at: string; + system: boolean; + noteable_id: number; + noteable_iid: number; + noteable_type: string; + resolvable: boolean; + resolved?: boolean; +}; + +export type GitlabDiscussion = { + id: string; + individual_note: boolean; + notes: GitlabNote[]; +}; + +export type GitlabPosition = { + base_sha: string; + start_sha: string; + head_sha: string; + position_type: "text"; + new_path: string; + new_line?: number; + old_path?: string; + old_line?: number; + line_range?: { + start: { line_code: string; type: "new" | "old" }; + end: { line_code: string; type: "new" | "old" }; + }; +}; diff --git a/src/mcp/gitlab-mr-server.ts b/src/mcp/gitlab-mr-server.ts new file mode 100644 index 0000000..292eaf3 --- /dev/null +++ b/src/mcp/gitlab-mr-server.ts @@ -0,0 +1,359 @@ +#!/usr/bin/env node + +import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js"; +import { z } from "zod"; +import { GitlabClient } from "../gitlab/api/client"; +import type { GitlabPosition } from "../gitlab/types"; + +export interface GitlabServerDependencies { + projectId: string; + client: GitlabClient; +} + +function textResult(text: string) { + return { content: [{ type: "text" as const, text }] }; +} + +function errorResult(error: unknown) { + const message = error instanceof Error ? error.message : String(error); + return { + content: [{ type: "text" as const, text: `Error: ${message}` }], + error: message, + isError: true, + }; +} + +const ReviewCommentSchema = z + .object({ + path: z + .string() + .describe( + "Path of the file to comment on (use the new_path from the diff)", + ), + body: z.string().min(1).describe("Comment text (supports markdown)"), + line: z + .number() + .int() + .optional() + .describe( + "Line number in the new file. Required for side=RIGHT (the default).", + ), + side: z + .enum(["LEFT", "RIGHT"]) + .optional() + .default("RIGHT") + .describe( + "Side of the diff: RIGHT for new/modified code, LEFT for removed code", + ), + old_path: z + .string() + .optional() + .describe("Path in the old file (defaults to path if unset)"), + old_line: z + .number() + .int() + .optional() + .describe( + "Line number in the old file. Required for side=LEFT comments.", + ), + }) + .refine( + (c) => { + const side = c.side ?? "RIGHT"; + if (side === "LEFT") return typeof c.old_line === "number"; + return typeof c.line === "number"; + }, + { + message: + "Inline diff discussions require a line anchor: provide `line` for side=RIGHT comments, or `old_line` for side=LEFT comments.", + }, + ); + +type ReviewComment = z.infer; + +function buildPosition( + comment: ReviewComment, + diffRefs: { base_sha: string; head_sha: string; start_sha: string }, +): GitlabPosition { + const newPath = comment.path; + const oldPath = comment.old_path ?? comment.path; + + const position: GitlabPosition = { + base_sha: diffRefs.base_sha, + start_sha: diffRefs.start_sha, + head_sha: diffRefs.head_sha, + position_type: "text", + new_path: newPath, + old_path: oldPath, + }; + + if (comment.side === "LEFT") { + if (typeof comment.old_line === "number") { + position.old_line = comment.old_line; + } else if (typeof comment.line === "number") { + position.old_line = comment.line; + } + } else { + if (typeof comment.line === "number") { + position.new_line = comment.line; + } + if (typeof comment.old_line === "number") { + position.old_line = comment.old_line; + } + } + + return position; +} + +export function createGitlabMrServer({ + projectId, + client, +}: GitlabServerDependencies) { + const server = new McpServer({ + name: "GitLab MR Server", + version: "0.0.1", + }); + + server.tool( + "get_mr", + "Fetch merge request metadata including diff_refs (base/head/start SHAs)", + { + mr_iid: z.number().int().describe("Merge request IID to fetch"), + }, + async ({ mr_iid }) => { + try { + const mr = await client.getMr(projectId, mr_iid); + return textResult(JSON.stringify(mr)); + } catch (error) { + return errorResult(error); + } + }, + ); + + server.tool( + "get_mr_changes", + "Fetch the file-level diff for a merge request", + { + mr_iid: z.number().int().describe("Merge request IID"), + }, + async ({ mr_iid }) => { + try { + const changes = await client.getMrChanges(projectId, mr_iid); + return textResult(JSON.stringify(changes)); + } catch (error) { + return errorResult(error); + } + }, + ); + + server.tool( + "list_mr_notes", + "List notes (comments) on a merge request", + { + mr_iid: z.number().int().describe("Merge request IID"), + }, + async ({ mr_iid }) => { + try { + const notes = await client.listNotes(projectId, mr_iid); + return textResult(JSON.stringify(notes)); + } catch (error) { + return errorResult(error); + } + }, + ); + + server.tool( + "create_mr_note", + "Post a top-level note (summary comment) on a merge request", + { + mr_iid: z.number().int().describe("Merge request IID"), + body: z.string().min(1).describe("Note body in markdown"), + }, + async ({ mr_iid, body }) => { + try { + const note = await client.createNote(projectId, mr_iid, body); + return textResult(`Created note ${note.id} on MR !${mr_iid}`); + } catch (error) { + return errorResult(error); + } + }, + ); + + server.tool( + "update_mr_note", + "Edit the body of an existing note (used for the sticky tracking comment)", + { + mr_iid: z.number().int().describe("Merge request IID"), + note_id: z.number().int().describe("Note ID to update"), + body: z.string().min(1).describe("New note body in markdown"), + }, + async ({ mr_iid, note_id, body }) => { + try { + await client.updateNote(projectId, mr_iid, note_id, body); + return textResult(`Updated note ${note_id} on MR !${mr_iid}`); + } catch (error) { + return errorResult(error); + } + }, + ); + + server.tool( + "update_tracking_note", + "Update the sticky tracking note that gitlab-prepare created at the " + + "start of the run. Reads the MR IID and note ID from env so the model " + + "doesn't have to thread them through the prompt. Mirrors the GitHub " + + "`update_droid_comment` tool.", + { + body: z + .string() + .min(1) + .describe("New tracking note body in markdown (replaces existing)"), + }, + async ({ body }) => { + try { + const mrIidEnv = + process.env.DROID_MR_IID || process.env.CI_MERGE_REQUEST_IID; + const noteIdEnv = process.env.DROID_TRACKING_NOTE_ID; + const mrIid = mrIidEnv ? Number(mrIidEnv) : NaN; + const noteId = noteIdEnv ? Number(noteIdEnv) : NaN; + if (!Number.isFinite(mrIid) || !Number.isFinite(noteId)) { + throw new Error( + "update_tracking_note requires DROID_MR_IID and " + + "DROID_TRACKING_NOTE_ID environment variables", + ); + } + await client.updateNote(projectId, mrIid, noteId, body); + return textResult(`Updated tracking note ${noteId} on MR !${mrIid}`); + } catch (error) { + return errorResult(error); + } + }, + ); + + server.tool( + "update_mr_description", + "Replace the description/body of a merge request", + { + mr_iid: z.number().int().describe("Merge request IID"), + description: z.string().describe("New description in markdown"), + }, + async ({ mr_iid, description }) => { + try { + await client.updateMrDescription(projectId, mr_iid, description); + return textResult(`Updated description for MR !${mr_iid}`); + } catch (error) { + return errorResult(error); + } + }, + ); + + server.tool( + "submit_review", + "Post an MR review: optional summary note plus zero or more inline " + + "discussions anchored to diff positions. Inline comments require the " + + "MR's current diff_refs (base/head/start SHAs).", + { + mr_iid: z.number().int().describe("Merge request IID to review"), + body: z + .string() + .optional() + .describe("Optional summary note body in markdown"), + comments: z + .array(ReviewCommentSchema) + .optional() + .describe("Inline review comments"), + }, + async ({ mr_iid, body, comments }) => { + try { + const summary = { + summaryNoteId: null as number | null, + discussionsCreated: 0, + discussionErrors: [] as Array<{ index: number; error: string }>, + }; + + if (body && body.trim().length > 0) { + const note = await client.createNote(projectId, mr_iid, body); + summary.summaryNoteId = note.id; + } + + if (comments && comments.length > 0) { + const mr = await client.getMr(projectId, mr_iid); + const diffRefs = mr.diff_refs; + if (!diffRefs) { + throw new Error( + "Merge request is missing diff_refs; cannot anchor inline comments", + ); + } + + for (let i = 0; i < comments.length; i++) { + const c = comments[i]!; + const position = buildPosition(c, diffRefs); + try { + await client.createDiscussionOnDiff( + projectId, + mr_iid, + c.body, + position, + ); + summary.discussionsCreated += 1; + } catch (error) { + const message = + error instanceof Error ? error.message : String(error); + summary.discussionErrors.push({ index: i, error: message }); + } + } + } + + return textResult(JSON.stringify(summary)); + } catch (error) { + return errorResult(error); + } + }, + ); + + return server; +} + +async function runServer() { + const projectId = process.env.CI_PROJECT_ID || process.env.GITLAB_PROJECT_ID; + const token = + process.env.GITLAB_TOKEN || + process.env.OVERRIDE_GITLAB_TOKEN || + process.env.CI_JOB_TOKEN; + const apiUrl = + process.env.CI_API_V4_URL || + process.env.GITLAB_API_URL || + "https://gitlab.com/api/v4"; + + if (!projectId) { + console.error( + "Error: CI_PROJECT_ID (or GITLAB_PROJECT_ID) environment variable is required", + ); + process.exit(1); + } + + if (!token) { + console.error( + "Error: GITLAB_TOKEN (or CI_JOB_TOKEN / OVERRIDE_GITLAB_TOKEN) environment variable is required", + ); + process.exit(1); + } + + const client = new GitlabClient(token, apiUrl); + const server = createGitlabMrServer({ projectId, client }); + + const transport = new StdioServerTransport(); + await server.connect(transport); + + process.on("exit", () => { + server.close(); + }); +} + +if (import.meta.main) { + runServer().catch((error) => { + console.error(error); + process.exit(1); + }); +} diff --git a/templates/README.md b/templates/README.md new file mode 100644 index 0000000..790ebf1 --- /dev/null +++ b/templates/README.md @@ -0,0 +1,40 @@ +# GitLab CI/CD Components + +This directory is **GitLab-specific**. The path is mandated by the +[GitLab CI/CD Catalog](https://docs.gitlab.com/ee/ci/components/) — every +Component project must define its components under top-level `templates/`. + +GitHub action code lives at `action.yml` (root) and `.github/workflows/`. + +## Components in this directory + +| File | Component | Usage | +| ------------------ | -------------- | ---------------------------------------------------------------- | +| `droid-review.yml` | `droid-review` | Automated MR code review (two-pass, optional security subagent). | + +## Consuming a component + +This repo is mirrored to [`gitlab.com/factory-components/droid-action`](https://gitlab.com/factory-components/droid-action). Customers include the template from the mirror: + +```yaml +include: + - project: "factory-components/droid-action" + ref: main + file: "/templates/droid-review.yml" +``` + +Once the project is marked as a [CI/CD Catalog](https://docs.gitlab.com/ee/ci/components/) project and the first tagged release is published, the shorter `component:` form also becomes available: + +```yaml +include: + - component: gitlab.com/factory-components/droid-action/droid-review@v1 +``` + +Fallback for projects that can't reach gitlab.com (use the raw GitHub URL): + +```yaml +include: + - remote: "https://raw.githubusercontent.com/Factory-AI/droid-action/main/templates/droid-review.yml" +``` + +See [`../docs/gitlab-setup.md`](../docs/gitlab-setup.md) for the full setup guide and [`../gitlab/examples/`](../gitlab/examples/) for drop-in samples. diff --git a/templates/droid-review.yml b/templates/droid-review.yml new file mode 100644 index 0000000..26f7b98 --- /dev/null +++ b/templates/droid-review.yml @@ -0,0 +1,213 @@ +# GitLab CI/CD Component: droid-review +# +# Automated Droid code review on merge requests. Two-pass pipeline +# (candidate generation + validator), inline MR comments, sticky tracking +# note with telemetry, optional parallel security-reviewer subagent. +# +# Consumed via: +# include: +# - project: "factory-components/droid-action" +# ref: main +# file: "/templates/droid-review.yml" +# +# Or, once Catalog-published: +# include: +# - component: gitlab.com/factory-components/droid-action/droid-review@ +# +# Catalog requires this file to live at `templates/.yml`. +# See ./README.md for the full directory contract. + +spec: + inputs: + automatic_review: + description: "Run code review automatically on every MR event." + default: "true" + review_depth: + description: "Review depth preset: `shallow` (fast) or `deep` (thorough)." + default: "deep" + review_model: + description: "Override the model used for code review. Empty = use depth preset." + default: "" + reasoning_effort: + description: "Override reasoning effort. Empty = use depth preset." + default: "" + include_suggestions: + description: | + Include code suggestion blocks in review comments when the fix is + high-confidence. Set to "false" to disable suggestion blocks + entirely. Mirrors the GitHub action's `include_suggestions` input. + default: "true" + automatic_security_review: + description: | + Automatically run a security-focused review subagent in parallel + with the regular code review on every MR pipeline. Findings are + prefixed with `[security]` and posted alongside code-review + comments. Mirrors the GitHub action's `automatic_security_review` + input. The security-review skill is built into the Droid CLI; no + plugin install is required. + default: "false" + security_block_on_critical: + description: | + When the security reviewer reports CRITICAL findings, block merge + of the MR. Mirrors the GitHub action's `security_block_on_critical` + input. Defaults to `"true"`. + default: "true" + security_block_on_high: + description: | + When the security reviewer reports HIGH findings, block merge of + the MR. Mirrors the GitHub action's `security_block_on_high` input. + Defaults to `"false"`. + default: "false" + settings: + description: | + Droid Exec settings as a JSON string or a path to a JSON file. + Merged into ~/.factory/droid/settings.json on the runner before + `droid exec` runs. Mirrors the GitHub action's `settings` input. + default: "" + droid_action_repo: + description: | + Git repo (clone URL) of droid-action. Defaults to the gitlab.com + mirror so GitLab runners with restricted egress only need + gitlab.com + app.factory.ai access. Override if you mirror it + privately. + default: "https://gitlab.com/factory-components/droid-action.git" + droid_action_ref: + description: | + Git tag or branch of droid-action to clone at runtime (SHAs are + not supported because the clone uses `--branch`). The same ref + SHOULD match the `include: remote:` URL above. + Internal: most users leave this at the default. + default: "main" + stage: + description: | + GitLab CI stage to assign the droid-review job to. The consuming + project's `.gitlab-ci.yml` must include this stage in its + `stages:` list (or omit the list to use the default ordering). + default: "test" +--- +droid-review: + stage: $[[ inputs.stage ]] + image: oven/bun:1.2.11 + rules: + - if: '$CI_PIPELINE_SOURCE == "merge_request_event"' + variables: + DROID_STATE_FILE: "$CI_PROJECT_DIR/.droid-state.json" + DROID_PROMPT_FILE: "/tmp/droid-prompts/droid-prompt.txt" + DROID_ACTION_DIR: "/tmp/droid-action" + DROID_ACTION_REPO: "$[[ inputs.droid_action_repo ]]" + DROID_ACTION_REF: "$[[ inputs.droid_action_ref ]]" + AUTOMATIC_REVIEW: "$[[ inputs.automatic_review ]]" + REVIEW_DEPTH: "$[[ inputs.review_depth ]]" + REVIEW_MODEL: "$[[ inputs.review_model ]]" + REASONING_EFFORT: "$[[ inputs.reasoning_effort ]]" + INCLUDE_SUGGESTIONS: "$[[ inputs.include_suggestions ]]" + AUTOMATIC_SECURITY_REVIEW: "$[[ inputs.automatic_security_review ]]" + SECURITY_BLOCK_ON_CRITICAL: "$[[ inputs.security_block_on_critical ]]" + SECURITY_BLOCK_ON_HIGH: "$[[ inputs.security_block_on_high ]]" + DROID_SETTINGS: "$[[ inputs.settings ]]" + DROID_SUCCESS: "true" + before_script: + - apt-get update -qq + - apt-get install -y -qq --no-install-recommends git ca-certificates curl + - git clone --depth 1 --branch "$DROID_ACTION_REF" "$DROID_ACTION_REPO" "$DROID_ACTION_DIR" + - cd "$DROID_ACTION_DIR" && bun install --frozen-lockfile + - cd "$CI_PROJECT_DIR" + - curl --retry 5 --retry-delay 2 --retry-all-errors -fsSL https://app.factory.ai/cli | sh + - export PATH="$HOME/.local/bin:$PATH" + - droid --version + # Copy bundled custom droids (e.g. security-reviewer) into the runner's + # ~/.factory/droids so the Skill / Task subagents can find them. Mirrors + # the GitHub action's "Setup Custom Droids" step. + - mkdir -p ~/.factory/droids + - | + if [ -d "$DROID_ACTION_DIR/.factory/droids" ]; then + cp -r "$DROID_ACTION_DIR/.factory/droids/." ~/.factory/droids/ + echo "Copied custom droids from $DROID_ACTION_DIR/.factory/droids to ~/.factory/droids" + fi + script: + - export PATH="$HOME/.local/bin:$PATH" + - | + if ! bun run "$DROID_ACTION_DIR/src/entrypoints/gitlab-prepare.ts"; then + export DROID_SUCCESS=false + echo "gitlab-prepare exited non-zero" > /tmp/droid-error.txt + exit 1 + fi + - | + # Skip droid exec if prepare decided not to review. + if ! grep -q '"shouldRunReview": true' "$DROID_STATE_FILE"; then + echo "shouldRunReview=false in state; skipping droid exec." + exit 0 + fi + - | + # Source the resolved env shim from gitlab-prepare to expose + # RESOLVED_MODEL / RESOLVED_REASONING_EFFORT / DROID_MR_IID / + # DROID_TRACKING_NOTE_ID for both droid exec calls and the MCP server. + if [ -f /tmp/droid-prompts/resolved-env.sh ]; then + # shellcheck disable=SC1091 + . /tmp/droid-prompts/resolved-env.sh + fi + - | + droid mcp remove gitlab_mr 2>/dev/null || true + droid mcp add gitlab_mr "bun run $DROID_ACTION_DIR/src/mcp/gitlab-mr-server.ts" \ + --env CI_PROJECT_ID="$CI_PROJECT_ID" \ + --env CI_API_V4_URL="$CI_API_V4_URL" \ + --env GITLAB_TOKEN="$GITLAB_TOKEN" \ + --env DROID_MR_IID="$DROID_MR_IID" \ + --env DROID_TRACKING_NOTE_ID="$DROID_TRACKING_NOTE_ID" + - | + # ---- Pass 1: candidate generation ---- + # Note: gitlab_mr___submit_review and the MR-mutation tools are + # NOT included here. Pass 1 must only WRITE the candidates JSON. + set -o pipefail + PASS1_TOOLS="Read,Grep,Glob,LS,Execute,Edit,Create,ApplyPatch,Task,Skill,FetchUrl" + PASS1_LOG="/tmp/droid-prompts/pass1-output.jsonl" + DROID_ARGS="exec -f $DROID_PROMPT_FILE --enabled-tools $PASS1_TOOLS --output-format stream-json --skip-permissions-unsafe --tag code-review" + if [ -n "$RESOLVED_MODEL" ]; then DROID_ARGS="$DROID_ARGS --model $RESOLVED_MODEL"; fi + if [ -n "$RESOLVED_REASONING_EFFORT" ]; then DROID_ARGS="$DROID_ARGS --reasoning-effort $RESOLVED_REASONING_EFFORT"; fi + echo "Running Pass 1 (candidates): droid $DROID_ARGS" + if ! droid $DROID_ARGS 2>&1 | tee "$PASS1_LOG"; then + export DROID_SUCCESS=false + echo "droid exec pass 1 (candidates) failed" > /tmp/droid-error.txt + exit 1 + fi + - | + # ---- Prepare Pass 2 prompt ---- + if ! bun run "$DROID_ACTION_DIR/src/entrypoints/gitlab-prepare-validator.ts"; then + export DROID_SUCCESS=false + echo "gitlab-prepare-validator failed" > /tmp/droid-error.txt + exit 1 + fi + - | + # ---- Pass 2: validator (posts approved comments) ---- + # gitlab_mr___submit_review and gitlab_mr___update_tracking_note are + # the only MR-mutation tools we expose. The model batches all approved + # findings into a single submit_review call. + set -o pipefail + PASS2_TOOLS="Read,Grep,Glob,LS,Execute,Edit,Create,ApplyPatch,Skill,gitlab_mr___submit_review,gitlab_mr___update_tracking_note" + PASS2_LOG="/tmp/droid-prompts/pass2-output.jsonl" + DROID_ARGS="exec -f $DROID_PROMPT_FILE --enabled-tools $PASS2_TOOLS --output-format stream-json --skip-permissions-unsafe --tag code-review" + if [ -n "$RESOLVED_MODEL" ]; then DROID_ARGS="$DROID_ARGS --model $RESOLVED_MODEL"; fi + if [ -n "$RESOLVED_REASONING_EFFORT" ]; then DROID_ARGS="$DROID_ARGS --reasoning-effort $RESOLVED_REASONING_EFFORT"; fi + echo "Running Pass 2 (validator): droid $DROID_ARGS" + if ! droid $DROID_ARGS 2>&1 | tee "$PASS2_LOG"; then + export DROID_SUCCESS=false + echo "droid exec pass 2 (validator) failed" > /tmp/droid-error.txt + exit 1 + fi + after_script: + - export PATH="$HOME/.local/bin:$PATH" + - export DROID_ERROR_DETAILS="$(cat /tmp/droid-error.txt 2>/dev/null || true)" + - export DROID_PASS1_LOG="/tmp/droid-prompts/pass1-output.jsonl" + - export DROID_PASS2_LOG="/tmp/droid-prompts/pass2-output.jsonl" + # Stage debug artifacts that live outside CI_PROJECT_DIR so GitLab can + # capture them. Failure here must not break the after_script chain. + - mkdir -p "$CI_PROJECT_DIR/.droid-debug/prompts" || true + - cp -r /tmp/droid-prompts/. "$CI_PROJECT_DIR/.droid-debug/prompts/" 2>/dev/null || true + - cp /tmp/droid-error.txt "$CI_PROJECT_DIR/.droid-debug/" 2>/dev/null || true + - bun run "$DROID_ACTION_DIR/src/entrypoints/gitlab-update-comment-link.ts" || true + artifacts: + when: always + paths: + - .droid-state.json + - .droid-debug/ + expire_in: 1 week diff --git a/test/gitlab/api-client.test.ts b/test/gitlab/api-client.test.ts new file mode 100644 index 0000000..927084f --- /dev/null +++ b/test/gitlab/api-client.test.ts @@ -0,0 +1,109 @@ +import { describe, expect, it, beforeEach, afterEach, mock } from "bun:test"; +import { GitlabClient, GitlabApiError } from "../../src/gitlab/api/client"; + +const ORIGINAL_FETCH = globalThis.fetch; + +function mockFetch(impl: (url: string, init: RequestInit) => Response) { + const fn = mock(async (url: string, init: RequestInit) => impl(url, init)); + // @ts-expect-error overriding global fetch in tests + globalThis.fetch = fn; + return fn; +} + +function jsonResponse(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { "Content-Type": "application/json" }, + }); +} + +describe("GitlabClient", () => { + beforeEach(() => { + // each test installs its own fetch mock + }); + + afterEach(() => { + globalThis.fetch = ORIGINAL_FETCH; + }); + + it("sends PRIVATE-TOKEN header on GET", async () => { + const fetchMock = mockFetch((url, init) => { + expect(url).toBe( + "https://gitlab.com/api/v4/projects/42/merge_requests/7", + ); + expect(init.method ?? "GET").toBe("GET"); + const headers = init.headers as Record; + expect(headers["PRIVATE-TOKEN"]).toBe("glpat-test"); + return jsonResponse({ iid: 7 }); + }); + + const client = new GitlabClient("glpat-test"); + const mr = await client.getMr(42, 7); + expect(mr).toEqual({ iid: 7 } as never); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("sends JSON body on POST createNote", async () => { + mockFetch((url, init) => { + expect(url).toBe( + "https://gitlab.com/api/v4/projects/42/merge_requests/7/notes", + ); + expect(init.method).toBe("POST"); + expect(init.body).toBe(JSON.stringify({ body: "hello" })); + const headers = init.headers as Record; + expect(headers["Content-Type"]).toBe("application/json"); + return jsonResponse({ id: 1, body: "hello" }); + }); + + const client = new GitlabClient("glpat-test"); + const note = await client.createNote(42, 7, "hello"); + expect((note as { id: number }).id).toBe(1); + }); + + it("URL-encodes string project ids (path-with-namespace)", async () => { + mockFetch((url) => { + expect(url).toBe( + "https://gitlab.com/api/v4/projects/group%2Fsub%2Frepo/merge_requests/7", + ); + return jsonResponse({ iid: 7 }); + }); + const client = new GitlabClient("glpat-test"); + await client.getMr("group/sub/repo", 7); + }); + + it("respects a custom baseUrl (self-hosted)", async () => { + mockFetch((url) => { + expect(url).toBe( + "https://gitlab.example.com/api/v4/projects/1/merge_requests/2", + ); + return jsonResponse({ iid: 2 }); + }); + const client = new GitlabClient( + "glpat-test", + "https://gitlab.example.com/api/v4", + ); + await client.getMr(1, 2); + }); + + it("throws GitlabApiError with parsed body on non-2xx", async () => { + mockFetch( + () => + new Response(JSON.stringify({ message: "404 Not Found" }), { + status: 404, + statusText: "Not Found", + headers: { "Content-Type": "application/json" }, + }), + ); + + const client = new GitlabClient("glpat-test"); + try { + await client.getMr(42, 999); + throw new Error("expected throw"); + } catch (err) { + expect(err).toBeInstanceOf(GitlabApiError); + const e = err as GitlabApiError; + expect(e.status).toBe(404); + expect((e.body as { message: string }).message).toBe("404 Not Found"); + } + }); +}); diff --git a/test/gitlab/context.test.ts b/test/gitlab/context.test.ts new file mode 100644 index 0000000..12fbb49 --- /dev/null +++ b/test/gitlab/context.test.ts @@ -0,0 +1,188 @@ +import { describe, expect, it, beforeEach, afterEach } from "bun:test"; +import { + parseGitlabContext, + isMergeRequestContext, +} from "../../src/gitlab/context"; + +const REQUIRED_CI_ENV = [ + "CI_SERVER_URL", + "CI_API_V4_URL", + "CI_PROJECT_ID", + "CI_PROJECT_PATH", + "CI_PROJECT_NAME", + "CI_PROJECT_URL", + "CI_COMMIT_SHA", + "CI_COMMIT_BRANCH", + "CI_COMMIT_TAG", + "CI_PIPELINE_ID", + "CI_PIPELINE_URL", + "CI_PIPELINE_SOURCE", + "CI_JOB_ID", + "CI_JOB_URL", + "CI_MERGE_REQUEST_IID", + "CI_MERGE_REQUEST_SOURCE_BRANCH_SHA", + "CI_MERGE_REQUEST_TARGET_BRANCH_SHA", + "CI_MERGE_REQUEST_DIFF_BASE_SHA", + "CI_MERGE_REQUEST_TITLE", + "GITLAB_USER_LOGIN", + "GITLAB_USER_NAME", + "GITLAB_USER_EMAIL", + "AUTOMATIC_REVIEW", + "AUTOMATIC_SECURITY_REVIEW", + "TRIGGER_PHRASE", + "REVIEW_DEPTH", + "REVIEW_MODEL", + "REASONING_EFFORT", + "FILL_MODEL", + "SECURITY_MODEL", + "SECURITY_SEVERITY_THRESHOLD", + "SECURITY_BLOCK_ON_CRITICAL", + "SECURITY_BLOCK_ON_HIGH", + "SECURITY_NOTIFY_TEAM", + "SECURITY_SCAN_SCHEDULE", + "SECURITY_SCAN_DAYS", + "DROID_SETTINGS", +]; + +function clearEnv() { + for (const key of REQUIRED_CI_ENV) { + delete process.env[key]; + } +} + +describe("parseGitlabContext", () => { + const originalEnv: Record = {}; + + beforeEach(() => { + for (const key of REQUIRED_CI_ENV) { + originalEnv[key] = process.env[key]; + } + clearEnv(); + }); + + afterEach(() => { + clearEnv(); + for (const key of Object.keys(originalEnv)) { + if (originalEnv[key] !== undefined) { + process.env[key] = originalEnv[key]; + } + } + }); + + it("parses a typical MR pipeline context", () => { + process.env.CI_SERVER_URL = "https://gitlab.com"; + process.env.CI_API_V4_URL = "https://gitlab.com/api/v4"; + process.env.CI_PROJECT_ID = "12345"; + process.env.CI_PROJECT_PATH = "group/sub/repo-name"; + process.env.CI_PROJECT_NAME = "repo-name"; + process.env.CI_PROJECT_URL = "https://gitlab.com/group/sub/repo-name"; + process.env.CI_COMMIT_SHA = "abc123"; + process.env.CI_COMMIT_BRANCH = "feature/x"; + process.env.CI_PIPELINE_SOURCE = "merge_request_event"; + process.env.CI_PIPELINE_ID = "999"; + process.env.CI_PIPELINE_URL = + "https://gitlab.com/group/sub/repo-name/-/pipelines/999"; + process.env.CI_MERGE_REQUEST_IID = "42"; + process.env.CI_MERGE_REQUEST_SOURCE_BRANCH_SHA = "src-sha"; + process.env.CI_MERGE_REQUEST_TARGET_BRANCH_SHA = "tgt-sha"; + process.env.CI_MERGE_REQUEST_DIFF_BASE_SHA = "base-sha"; + process.env.CI_MERGE_REQUEST_TITLE = "Add feature x"; + process.env.AUTOMATIC_REVIEW = "true"; + + const ctx = parseGitlabContext(); + + expect(ctx.serverUrl).toBe("https://gitlab.com"); + expect(ctx.apiUrl).toBe("https://gitlab.com/api/v4"); + expect(ctx.project.id).toBe("12345"); + expect(ctx.project.pathWithNamespace).toBe("group/sub/repo-name"); + expect(ctx.commit.sha).toBe("abc123"); + expect(ctx.pipelineSource).toBe("merge_request_event"); + expect(isMergeRequestContext(ctx)).toBe(true); + expect(ctx.mr?.iid).toBe(42); + expect(ctx.mr?.diffBaseSha).toBe("base-sha"); + expect(ctx.inputs.automaticReview).toBe(true); + expect(ctx.inputs.reviewDepth).toBe("deep"); + expect(ctx.inputs.securityScanDays).toBe(7); + }); + + it("parses a push-event context with no MR", () => { + process.env.CI_PROJECT_ID = "1"; + process.env.CI_PROJECT_PATH = "g/r"; + process.env.CI_COMMIT_SHA = "deadbeef"; + process.env.CI_PIPELINE_SOURCE = "push"; + + const ctx = parseGitlabContext(); + + expect(ctx.mr).toBeNull(); + expect(isMergeRequestContext(ctx)).toBe(false); + }); + + it("trims trailing slashes from server and api urls", () => { + process.env.CI_SERVER_URL = "https://gitlab.example.com///"; + process.env.CI_API_V4_URL = "https://gitlab.example.com/api/v4///"; + process.env.CI_PROJECT_ID = "1"; + process.env.CI_PROJECT_PATH = "g/r"; + process.env.CI_COMMIT_SHA = "x"; + + const ctx = parseGitlabContext(); + + expect(ctx.serverUrl).toBe("https://gitlab.example.com"); + expect(ctx.apiUrl).toBe("https://gitlab.example.com/api/v4"); + }); + + it("falls back to derived api url when CI_API_V4_URL is unset", () => { + process.env.CI_SERVER_URL = "https://self-hosted.example.com"; + process.env.CI_PROJECT_ID = "1"; + process.env.CI_PROJECT_PATH = "g/r"; + process.env.CI_COMMIT_SHA = "x"; + + const ctx = parseGitlabContext(); + + expect(ctx.apiUrl).toBe("https://self-hosted.example.com/api/v4"); + }); + + it("throws when CI_PROJECT_ID is missing", () => { + process.env.CI_PROJECT_PATH = "g/r"; + process.env.CI_COMMIT_SHA = "x"; + expect(() => parseGitlabContext()).toThrow(/CI_PROJECT_ID/); + }); + + it("clamps negative securityScanDays to at least 1", () => { + process.env.CI_PROJECT_ID = "1"; + process.env.CI_PROJECT_PATH = "g/r"; + process.env.CI_COMMIT_SHA = "x"; + process.env.SECURITY_SCAN_DAYS = "-3"; + + const ctx = parseGitlabContext(); + expect(ctx.inputs.securityScanDays).toBe(1); + }); + + it("falls back to 7 days when securityScanDays is invalid", () => { + process.env.CI_PROJECT_ID = "1"; + process.env.CI_PROJECT_PATH = "g/r"; + process.env.CI_COMMIT_SHA = "x"; + process.env.SECURITY_SCAN_DAYS = "not-a-number"; + + const ctx = parseGitlabContext(); + expect(ctx.inputs.securityScanDays).toBe(7); + }); + + it("surfaces DROID_SETTINGS as inputs.settings", () => { + process.env.CI_PROJECT_ID = "1"; + process.env.CI_PROJECT_PATH = "g/r"; + process.env.CI_COMMIT_SHA = "x"; + process.env.DROID_SETTINGS = '{"reasoning_effort":"medium"}'; + + const ctx = parseGitlabContext(); + expect(ctx.inputs.settings).toBe('{"reasoning_effort":"medium"}'); + }); + + it("defaults inputs.settings to empty string when unset", () => { + process.env.CI_PROJECT_ID = "1"; + process.env.CI_PROJECT_PATH = "g/r"; + process.env.CI_COMMIT_SHA = "x"; + + const ctx = parseGitlabContext(); + expect(ctx.inputs.settings).toBe(""); + }); +}); diff --git a/test/gitlab/exec-telemetry.test.ts b/test/gitlab/exec-telemetry.test.ts new file mode 100644 index 0000000..a9ed21b --- /dev/null +++ b/test/gitlab/exec-telemetry.test.ts @@ -0,0 +1,116 @@ +import { describe, it, expect } from "bun:test"; +import { + collectExecTelemetry, + extractPassTelemetry, + parseTelemetryFile, +} from "../../src/gitlab/data/exec-telemetry"; +import * as fs from "fs/promises"; +import * as os from "os"; +import * as path from "path"; + +describe("extractPassTelemetry", () => { + it("returns null when there are no JSON lines", () => { + expect(extractPassTelemetry([])).toBeNull(); + expect(extractPassTelemetry(["not json", "still not"])).toBeNull(); + }); + + it("picks the last completion event and extracts usage fields", () => { + const lines = [ + '{"type":"system","subtype":"init","session_id":"abc"}', + '{"type":"message","role":"user"}', + '{"type":"completion","finalText":"hi","numTurns":12,"durationMs":42000,"session_id":"final-session","usage":{"input_tokens":1000,"output_tokens":250,"cache_read_input_tokens":50000}}', + ]; + const out = extractPassTelemetry(lines); + expect(out).not.toBeNull(); + expect(out!.sessionId).toBe("final-session"); + expect(out!.numTurns).toBe(12); + expect(out!.durationMs).toBe(42000); + expect(out!.inputTokens).toBe(1000); + expect(out!.outputTokens).toBe(250); + expect(out!.cacheReadInputTokens).toBe(50000); + expect(out!.costUsd).toBeNull(); + }); + + it("also recognizes legacy result events with cost_usd", () => { + const lines = [ + '{"type":"result","cost_usd":0.42,"duration_ms":15000,"num_turns":8,"session_id":"sess"}', + ]; + const out = extractPassTelemetry(lines); + expect(out).not.toBeNull(); + expect(out!.costUsd).toBe(0.42); + expect(out!.durationMs).toBe(15000); + expect(out!.numTurns).toBe(8); + }); + + it("ignores malformed JSON lines and keeps scanning", () => { + const lines = [ + "not json at all", + "{broken", + '{"type":"completion","numTurns":3,"session_id":"x"}', + ]; + const out = extractPassTelemetry(lines); + expect(out!.numTurns).toBe(3); + expect(out!.sessionId).toBe("x"); + }); +}); + +describe("parseTelemetryFile", () => { + it("returns null when file doesn't exist", async () => { + const out = await parseTelemetryFile("/does/not/exist.jsonl"); + expect(out).toBeNull(); + }); + + it("parses a real-looking pass log", async () => { + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), "droid-tele-")); + const file = path.join(tmp, "pass.jsonl"); + await fs.writeFile( + file, + [ + '{"type":"system","subtype":"init","session_id":"s1"}', + '{"type":"completion","numTurns":15,"durationMs":311617,"session_id":"s1","usage":{"input_tokens":35300,"output_tokens":10341}}', + ].join("\n"), + ); + const out = await parseTelemetryFile(file); + expect(out!.sessionId).toBe("s1"); + expect(out!.numTurns).toBe(15); + expect(out!.durationMs).toBe(311617); + await fs.rm(tmp, { recursive: true, force: true }); + }); +}); + +describe("collectExecTelemetry", () => { + it("returns null passes and null totals when both files missing", async () => { + const out = await collectExecTelemetry({ + pass1LogPath: "/missing/pass1.jsonl", + pass2LogPath: "/missing/pass2.jsonl", + }); + expect(out.pass1).toBeNull(); + expect(out.pass2).toBeNull(); + expect(out.totalCostUsd).toBeNull(); + expect(out.totalDurationMs).toBeNull(); + expect(out.totalNumTurns).toBeNull(); + }); + + it("sums numTurns and durations across both passes", async () => { + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), "droid-tele-")); + const p1 = path.join(tmp, "p1.jsonl"); + const p2 = path.join(tmp, "p2.jsonl"); + await fs.writeFile( + p1, + '{"type":"completion","numTurns":15,"durationMs":300000,"session_id":"s1"}', + ); + await fs.writeFile( + p2, + '{"type":"completion","numTurns":29,"durationMs":400000,"session_id":"s2"}', + ); + const out = await collectExecTelemetry({ + pass1LogPath: p1, + pass2LogPath: p2, + }); + expect(out.pass1!.sessionId).toBe("s1"); + expect(out.pass2!.sessionId).toBe("s2"); + expect(out.totalNumTurns).toBe(44); + expect(out.totalDurationMs).toBe(700000); + await fs.rm(tmp, { recursive: true, force: true }); + }); +}); diff --git a/test/gitlab/prompts.test.ts b/test/gitlab/prompts.test.ts new file mode 100644 index 0000000..83b0c0e --- /dev/null +++ b/test/gitlab/prompts.test.ts @@ -0,0 +1,129 @@ +import { describe, it, expect } from "bun:test"; +import { generateGitlabReviewCandidatesPrompt } from "../../src/gitlab/prompts/candidates"; +import { generateGitlabReviewValidatorPrompt } from "../../src/gitlab/prompts/validator"; +import type { GitlabReviewPromptContext } from "../../src/gitlab/prompts/types"; + +function baseCtx( + overrides: Partial = {}, +): GitlabReviewPromptContext { + return { + projectPath: "top-group/sub/project", + mrIid: 42, + mrTitle: "Add feature X", + sourceBranch: "feat/x", + targetBranch: "main", + headSha: "deadbeef1234567890abcdef", + diffPath: "/tmp/droid-prompts/mr.diff", + commentsPath: "/tmp/droid-prompts/existing_comments.json", + descriptionPath: "/tmp/droid-prompts/mr_description.txt", + candidatesPath: "/tmp/droid-prompts/review_candidates.json", + validatedPath: "/tmp/droid-prompts/review_validated.json", + includeSuggestions: true, + securityReviewEnabled: false, + ...overrides, + }; +} + +describe("generateGitlabReviewCandidatesPrompt", () => { + it("uses MR/project terminology, not PR/repo", () => { + const prompt = generateGitlabReviewCandidatesPrompt(baseCtx()); + expect(prompt).toContain("MR !42"); + expect(prompt).toContain("top-group/sub/project"); + expect(prompt).toContain("MR IID: 42"); + expect(prompt).toContain("MR Source Branch: feat/x"); + expect(prompt).toContain("MR Target Branch: main"); + expect(prompt).toContain("MR Head SHA: deadbeef1234567890abcdef"); + expect(prompt).not.toMatch(/\bPR #\d+\b/); + expect(prompt).not.toContain("PR Head Ref"); + }); + + it("references the three artifact files and candidates output", () => { + const prompt = generateGitlabReviewCandidatesPrompt(baseCtx()); + expect(prompt).toContain("/tmp/droid-prompts/mr.diff"); + expect(prompt).toContain("/tmp/droid-prompts/existing_comments.json"); + expect(prompt).toContain("/tmp/droid-prompts/mr_description.txt"); + expect(prompt).toContain("/tmp/droid-prompts/review_candidates.json"); + }); + + it("instructs the model to invoke the review skill Pass 1 procedure", () => { + const prompt = generateGitlabReviewCandidatesPrompt(baseCtx()); + expect(prompt).toContain("Invoke the 'review' skill"); + expect(prompt).toContain("Pass 1: Candidate Generation"); + }); + + it("explicitly forbids GitLab posting tools", () => { + const prompt = generateGitlabReviewCandidatesPrompt(baseCtx()); + expect(prompt).toContain("post to GitLab"); + expect(prompt).toMatch(/DO NOT\*?\*?\s+post to GitLab/); + expect(prompt).toContain("gitlab_mr___submit_review"); + expect(prompt).toContain("gitlab_mr___update_tracking_note"); + }); + + it("includes the security-reviewer subagent block only when enabled", () => { + const off = generateGitlabReviewCandidatesPrompt( + baseCtx({ securityReviewEnabled: false }), + ); + expect(off).not.toContain("security-reviewer"); + + const on = generateGitlabReviewCandidatesPrompt( + baseCtx({ securityReviewEnabled: true }), + ); + expect(on).toContain("security-reviewer"); + expect(on).toContain("Task tool"); + expect(on).toContain("[security]"); + }); + + it("drops suggestion-block guidance when includeSuggestions is false", () => { + const off = generateGitlabReviewCandidatesPrompt( + baseCtx({ includeSuggestions: false }), + ); + expect(off).toContain("Do NOT include code suggestion blocks"); + expect(off).not.toContain("suggestion block rules"); + }); +}); + +describe("generateGitlabReviewValidatorPrompt", () => { + it("identifies itself as Phase 2 / validator", () => { + const prompt = generateGitlabReviewValidatorPrompt(baseCtx()); + expect(prompt).toContain("Phase 2 (validator)"); + expect(prompt).toContain("Pass 2: Validation"); + }); + + it("uses GitLab MR terminology, not GitHub PR terminology", () => { + const prompt = generateGitlabReviewValidatorPrompt(baseCtx()); + expect(prompt).toContain("MR !42"); + expect(prompt).not.toMatch(/\bPR #\d+\b/); + expect(prompt).not.toContain("github_pr___submit_review"); + }); + + it("instructs the model to batch approved findings via gitlab_mr___submit_review", () => { + const prompt = generateGitlabReviewValidatorPrompt(baseCtx()); + expect(prompt).toContain("gitlab_mr___submit_review"); + expect(prompt).toContain("single batched review"); + expect(prompt).toContain("mr_iid: 42"); + expect(prompt).toContain("gitlab_mr___update_tracking_note"); + }); + + it("enforces the approved/rejected ordering contract", () => { + const prompt = generateGitlabReviewValidatorPrompt(baseCtx()); + expect(prompt).toContain('status === "approved"'); + expect(prompt).toContain("Preserve ordering"); + expect(prompt).toContain("exactly one entry per candidate"); + }); + + it("points at the right artifact paths from state", () => { + const ctx = baseCtx({ + diffPath: "/custom/mr.diff", + commentsPath: "/custom/comments.json", + descriptionPath: "/custom/desc.txt", + candidatesPath: "/custom/candidates.json", + validatedPath: "/custom/validated.json", + }); + const prompt = generateGitlabReviewValidatorPrompt(ctx); + expect(prompt).toContain("/custom/mr.diff"); + expect(prompt).toContain("/custom/comments.json"); + expect(prompt).toContain("/custom/desc.txt"); + expect(prompt).toContain("/custom/candidates.json"); + expect(prompt).toContain("/custom/validated.json"); + }); +}); diff --git a/test/gitlab/review-artifacts.test.ts b/test/gitlab/review-artifacts.test.ts new file mode 100644 index 0000000..047bcc4 --- /dev/null +++ b/test/gitlab/review-artifacts.test.ts @@ -0,0 +1,153 @@ +import { describe, it, expect, beforeEach, afterEach } from "bun:test"; +import * as fs from "fs/promises"; +import * as os from "os"; +import * as path from "path"; +import { + buildDescriptionContent, + buildDiffContent, + computeReviewArtifacts, +} from "../../src/gitlab/data/review-artifacts"; +import type { GitlabClient } from "../../src/gitlab/api/client"; + +function fakeClient(opts: { + mr?: object; + changes?: object; + notes?: object; +}): GitlabClient { + return { + getMr: async () => opts.mr ?? {}, + getMrChanges: async () => opts.changes ?? { changes: [] }, + listNotes: async () => opts.notes ?? [], + } as unknown as GitlabClient; +} + +describe("buildDiffContent", () => { + it("returns empty string when there are no changes", () => { + expect(buildDiffContent({ changes: [] } as never)).toBe(""); + }); + + it("emits a `diff --git` header per file using old/new paths", () => { + const out = buildDiffContent({ + changes: [ + { + old_path: "a/foo.ts", + new_path: "a/foo.ts", + diff: "@@ -1 +1 @@\n-old\n+new\n", + a_mode: "", + b_mode: "", + new_file: false, + renamed_file: false, + deleted_file: false, + }, + { + old_path: "b/old.ts", + new_path: "b/renamed.ts", + diff: "@@ -1 +1 @@\n line\n", + a_mode: "", + b_mode: "", + new_file: false, + renamed_file: true, + deleted_file: false, + }, + ], + } as never); + expect(out).toContain("diff --git a/a/foo.ts b/a/foo.ts"); + expect(out).toContain("diff --git a/b/old.ts b/b/renamed.ts"); + expect(out).toContain("-old"); + expect(out).toContain("+new"); + }); + + it("falls back to new_path when old_path is missing (new file)", () => { + const out = buildDiffContent({ + changes: [ + { + old_path: "", + new_path: "src/new.ts", + diff: "+content\n", + a_mode: "", + b_mode: "", + new_file: true, + renamed_file: false, + deleted_file: false, + }, + ], + } as never); + expect(out).toContain("diff --git a/src/new.ts b/src/new.ts"); + }); +}); + +describe("buildDescriptionContent", () => { + it("includes the title and description", () => { + const out = buildDescriptionContent({ + title: "Add feature X", + description: "Closes #1.", + } as never); + expect(out).toContain("Title: Add feature X"); + expect(out).toContain("Closes #1."); + }); + + it("tolerates a null description", () => { + const out = buildDescriptionContent({ + title: "T", + description: null, + } as never); + expect(out).toContain("Title: T"); + }); +}); + +describe("computeReviewArtifacts", () => { + let tmp: string; + beforeEach(async () => { + tmp = await fs.mkdtemp(path.join(os.tmpdir(), "droid-artifacts-")); + }); + afterEach(async () => { + await fs.rm(tmp, { recursive: true, force: true }); + }); + + it("writes mr.diff, existing_comments.json, mr_description.txt", async () => { + const client = fakeClient({ + mr: { + title: "Hello", + description: "world", + diff_refs: { base_sha: "b", head_sha: "h", start_sha: "s" }, + }, + changes: { + changes: [ + { + old_path: "x.ts", + new_path: "x.ts", + diff: "@@ -1 +1 @@\n+x\n", + a_mode: "", + b_mode: "", + new_file: false, + renamed_file: false, + deleted_file: false, + }, + ], + }, + notes: [{ id: 1, body: "hi", system: false }], + }); + + const result = await computeReviewArtifacts({ + client, + projectId: "42", + mrIid: 7, + outDir: tmp, + }); + + expect(result.diffPath).toBe(path.join(tmp, "mr.diff")); + expect(result.commentsPath).toBe(path.join(tmp, "existing_comments.json")); + expect(result.descriptionPath).toBe(path.join(tmp, "mr_description.txt")); + + const diff = await fs.readFile(result.diffPath, "utf8"); + expect(diff).toContain("diff --git a/x.ts b/x.ts"); + + const comments = JSON.parse(await fs.readFile(result.commentsPath, "utf8")); + expect(comments).toHaveLength(1); + expect(comments[0].body).toBe("hi"); + + const desc = await fs.readFile(result.descriptionPath, "utf8"); + expect(desc).toContain("Title: Hello"); + expect(desc).toContain("world"); + }); +}); diff --git a/test/gitlab/review-config-resolution.test.ts b/test/gitlab/review-config-resolution.test.ts new file mode 100644 index 0000000..9541d6b --- /dev/null +++ b/test/gitlab/review-config-resolution.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, it } from "bun:test"; +import { resolveReviewConfig } from "../../src/utils/review-depth"; + +describe("resolveReviewConfig (used by gitlab-prepare)", () => { + it("uses deep preset by default", () => { + expect(resolveReviewConfig()).toEqual({ + model: "gpt-5.2", + reasoningEffort: "high", + }); + }); + + it("returns shallow preset for review_depth=shallow", () => { + expect(resolveReviewConfig({ reviewDepth: "shallow" })).toEqual({ + model: "kimi-k2.6", + reasoningEffort: undefined, + }); + }); + + it("explicit reviewModel beats depth preset", () => { + const out = resolveReviewConfig({ + reviewDepth: "shallow", + reviewModel: "claude-sonnet-4-5-20250929", + }); + expect(out.model).toBe("claude-sonnet-4-5-20250929"); + }); + + it("explicit reasoningEffort beats depth preset", () => { + const out = resolveReviewConfig({ + reviewDepth: "deep", + reasoningEffort: "medium", + }); + expect(out.reasoningEffort).toBe("medium"); + expect(out.model).toBe("gpt-5.2"); + }); + + it("both explicit overrides win simultaneously", () => { + const out = resolveReviewConfig({ + reviewDepth: "shallow", + reviewModel: "claude-opus-4-8", + reasoningEffort: "low", + }); + expect(out).toEqual({ + model: "claude-opus-4-8", + reasoningEffort: "low", + }); + }); + + it("unknown reviewDepth falls back to shallow preset", () => { + const out = resolveReviewConfig({ reviewDepth: "neutron-star" }); + expect(out.model).toBe("kimi-k2.6"); + }); +}); diff --git a/test/gitlab/token.test.ts b/test/gitlab/token.test.ts new file mode 100644 index 0000000..734fdb3 --- /dev/null +++ b/test/gitlab/token.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it, beforeEach, afterEach } from "bun:test"; +import { + setupGitlabToken, + MissingGitlabTokenError, +} from "../../src/gitlab/token"; + +const KEYS = ["GITLAB_TOKEN", "OVERRIDE_GITLAB_TOKEN", "CI_JOB_TOKEN"]; + +describe("setupGitlabToken", () => { + const original: Record = {}; + + beforeEach(() => { + for (const k of KEYS) { + original[k] = process.env[k]; + delete process.env[k]; + } + }); + + afterEach(() => { + for (const k of KEYS) { + if (original[k] !== undefined) { + process.env[k] = original[k]; + } else { + delete process.env[k]; + } + } + }); + + it("prefers GITLAB_TOKEN", () => { + process.env.GITLAB_TOKEN = "glpat-primary"; + process.env.OVERRIDE_GITLAB_TOKEN = "glpat-override"; + process.env.CI_JOB_TOKEN = "ci-job-token"; + expect(setupGitlabToken()).toBe("glpat-primary"); + }); + + it("falls back to OVERRIDE_GITLAB_TOKEN", () => { + process.env.OVERRIDE_GITLAB_TOKEN = "glpat-override"; + process.env.CI_JOB_TOKEN = "ci-job-token"; + expect(setupGitlabToken()).toBe("glpat-override"); + }); + + it("does NOT fall back to CI_JOB_TOKEN (its scopes are insufficient for notes/discussions)", () => { + process.env.CI_JOB_TOKEN = "ci-job-token"; + expect(() => setupGitlabToken()).toThrow(MissingGitlabTokenError); + }); + + it("throws MissingGitlabTokenError when nothing is set", () => { + expect(() => setupGitlabToken()).toThrow(MissingGitlabTokenError); + }); +}); diff --git a/test/gitlab/tracking-note.test.ts b/test/gitlab/tracking-note.test.ts new file mode 100644 index 0000000..409ad18 --- /dev/null +++ b/test/gitlab/tracking-note.test.ts @@ -0,0 +1,112 @@ +import { describe, expect, it } from "bun:test"; +import { + DROID_TRACKING_MARKER, + buildTrackingNoteBody, + findExistingTrackingNote, +} from "../../src/gitlab/operations/tracking-note"; + +describe("buildTrackingNoteBody", () => { + it("includes the tracking marker in every state", () => { + for (const state of ["running", "success", "failure"] as const) { + const body = buildTrackingNoteBody({ state }); + expect(body).toContain(DROID_TRACKING_MARKER); + } + }); + + it("renders pipeline + job links when provided", () => { + const body = buildTrackingNoteBody({ + state: "running", + pipelineUrl: "https://gitlab.com/p/-/pipelines/1", + jobUrl: "https://gitlab.com/p/-/jobs/2", + }); + expect(body).toContain("Pipeline: https://gitlab.com/p/-/pipelines/1"); + expect(body).toContain("Job log: https://gitlab.com/p/-/jobs/2"); + }); + + it("renders security badge when securityReviewRan is true", () => { + const body = buildTrackingNoteBody({ + state: "running", + securityReviewRan: true, + }); + expect(body).toContain("security%20review-enabled"); + }); + + it("omits security badge when securityReviewRan is false", () => { + const body = buildTrackingNoteBody({ + state: "running", + securityReviewRan: false, + }); + expect(body).not.toContain("security%20review-enabled"); + }); + + it("embeds error details only on failure state", () => { + const failure = buildTrackingNoteBody({ + state: "failure", + errorDetails: "boom", + }); + expect(failure).toContain("
"); + expect(failure).toContain("boom"); + + const success = buildTrackingNoteBody({ + state: "success", + errorDetails: "boom", + }); + expect(success).not.toContain("
"); + }); + + it("renders telemetry summary line when totals are provided", () => { + const body = buildTrackingNoteBody({ + state: "success", + telemetry: { + totalNumTurns: 44, + totalDurationMs: 700000, + totalCostUsd: 0.42, + }, + }); + expect(body).toContain("44 turns"); + expect(body).toContain("11m 40s"); + expect(body).toContain("$0.42"); + }); + + it("includes session IDs in a collapsible block when provided", () => { + const body = buildTrackingNoteBody({ + state: "success", + telemetry: { + pass1SessionId: "sess-1", + pass2SessionId: "sess-2", + }, + }); + expect(body).toContain("Droid session IDs"); + expect(body).toContain("`sess-1`"); + expect(body).toContain("`sess-2`"); + }); + + it("omits telemetry block entirely when telemetry is missing or empty", () => { + const none = buildTrackingNoteBody({ state: "success" }); + expect(none).not.toContain("turns"); + expect(none).not.toContain("Droid session IDs"); + + const empty = buildTrackingNoteBody({ + state: "success", + telemetry: {}, + }); + expect(empty).not.toContain("turns"); + expect(empty).not.toContain("Droid session IDs"); + }); +}); + +describe("findExistingTrackingNote", () => { + it("finds the note containing the droid marker", () => { + const notes = [ + { id: 1, body: "regular comment" }, + { id: 2, body: `${DROID_TRACKING_MARKER}\nDroid is reviewing...` }, + { id: 3, body: "another comment" }, + ]; + expect(findExistingTrackingNote(notes)?.id).toBe(2); + }); + + it("returns undefined when no tracking note exists", () => { + const notes = [{ id: 1, body: "regular comment" }]; + expect(findExistingTrackingNote(notes)).toBeUndefined(); + }); +}); diff --git a/test/mcp/gitlab-mr-server.test.ts b/test/mcp/gitlab-mr-server.test.ts new file mode 100644 index 0000000..879decd --- /dev/null +++ b/test/mcp/gitlab-mr-server.test.ts @@ -0,0 +1,156 @@ +import { describe, expect, it, mock } from "bun:test"; +import { createGitlabMrServer } from "../../src/mcp/gitlab-mr-server"; +import { GitlabClient } from "../../src/gitlab/api/client"; + +function makeFakeClient(overrides: Partial = {}): GitlabClient { + const fake = { + getMr: mock(async () => ({ + iid: 7, + diff_refs: { + base_sha: "base-sha", + head_sha: "head-sha", + start_sha: "start-sha", + }, + })), + getMrChanges: mock(async () => ({ changes: [], diff_refs: {} })), + listNotes: mock(async () => []), + createNote: mock(async (_p: unknown, _m: unknown, body: string) => ({ + id: 101, + body, + })), + updateNote: mock(async () => ({ id: 101 })), + createDiscussionOnDiff: mock(async () => ({ + id: "disc-1", + individual_note: false, + notes: [], + })), + updateMrDescription: mock(async () => ({ iid: 7 })), + ...overrides, + } as unknown as GitlabClient; + return fake; +} + +function listTools(server: ReturnType) { + // McpServer keeps tools internally; we call _registeredTools via a small probe. + // Instead, exercise the public API by invoking a known tool name via internal handler. + return server; +} + +describe("createGitlabMrServer", () => { + it("registers without throwing and exposes a server instance", () => { + const client = makeFakeClient(); + const server = createGitlabMrServer({ projectId: "42", client }); + expect(server).toBeTruthy(); + listTools(server); + }); + + it("submit_review posts a summary note when body provided", async () => { + const client = makeFakeClient(); + const server = createGitlabMrServer({ projectId: "42", client }); + + // @ts-expect-error - reach into internal tool registry to invoke directly + const tool = server._registeredTools?.submit_review; + expect(tool).toBeTruthy(); + + const result = await tool.callback({ + mr_iid: 7, + body: "Hello reviewers", + comments: [], + }); + + expect(result.isError).toBeUndefined(); + expect( + (client.createNote as ReturnType).mock.calls.length, + ).toBe(1); + const payload = JSON.parse(result.content[0].text); + expect(payload.summaryNoteId).toBe(101); + expect(payload.discussionsCreated).toBe(0); + }); + + it("submit_review anchors inline comments using diff_refs", async () => { + const client = makeFakeClient(); + const server = createGitlabMrServer({ projectId: "42", client }); + // @ts-expect-error - reach into internal tool registry + const tool = server._registeredTools?.submit_review; + + const result = await tool.callback({ + mr_iid: 7, + comments: [ + { path: "src/x.ts", body: "Issue here", line: 12, side: "RIGHT" }, + ], + }); + + expect(result.isError).toBeUndefined(); + const createDisc = client.createDiscussionOnDiff as ReturnType; + expect(createDisc.mock.calls.length).toBe(1); + + const call = createDisc.mock.calls[0]!; + const position = call[3]; + expect(position).toMatchObject({ + base_sha: "base-sha", + head_sha: "head-sha", + start_sha: "start-sha", + position_type: "text", + new_path: "src/x.ts", + old_path: "src/x.ts", + new_line: 12, + }); + }); + + it("submit_review uses old_line on LEFT side", async () => { + const client = makeFakeClient(); + const server = createGitlabMrServer({ projectId: "42", client }); + // @ts-expect-error + const tool = server._registeredTools?.submit_review; + + await tool.callback({ + mr_iid: 7, + comments: [ + { + path: "src/x.ts", + body: "Removed code issue", + line: 5, + side: "LEFT", + }, + ], + }); + + const createDisc = client.createDiscussionOnDiff as ReturnType; + const call = createDisc.mock.calls[0]!; + const position = call[3]; + expect(position.old_line).toBe(5); + expect(position.new_line).toBeUndefined(); + }); + + it("submit_review collects per-comment errors without aborting", async () => { + const failingDisc = mock(async (_p, _m, body: string) => { + if (body.includes("bad")) { + throw new Error("position out of range"); + } + return { id: "disc", individual_note: false, notes: [] }; + }); + const client = makeFakeClient({ + createDiscussionOnDiff: failingDisc, + } as Partial); + const server = createGitlabMrServer({ projectId: "42", client }); + // @ts-expect-error + const tool = server._registeredTools?.submit_review; + + const result = await tool.callback({ + mr_iid: 7, + comments: [ + { path: "a.ts", body: "good", line: 1 }, + { path: "a.ts", body: "bad", line: 999 }, + { path: "a.ts", body: "good again", line: 2 }, + ], + }); + + const payload = JSON.parse(result.content[0].text); + expect(payload.discussionsCreated).toBe(2); + expect(payload.discussionErrors).toHaveLength(1); + expect(payload.discussionErrors[0].index).toBe(1); + expect(payload.discussionErrors[0].error).toContain( + "position out of range", + ); + }); +}); From 150e8c231191631016a9563bf5d8388e36382de9 Mon Sep 17 00:00:00 2001 From: factory-nizar Date: Wed, 3 Jun 2026 17:04:18 -0700 Subject: [PATCH 2/2] refactor(review): hoist GH+GL shared review code into src/core/review/ (#96) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(review): extract shared candidates+validator prompts to src/core/review/ Both Pass 1 (candidate generation) and Pass 2 (validator) prompts were ~95% identical between GitHub and GitLab — same skill invocation, same output schema, same critical constraints. Differences were limited to labels (PR/MR, Repo/Project), JSON meta keys (prNumber/mrIid, baseRef/targetBranch), MCP tool names, and a handful of platform-specific instruction lines. Extract the shared structure into platform-agnostic builders under src/core/review/prompts/ that take a ReviewTerminology shape, and convert the four existing prompt files into thin adapters: src/core/review/prompts/ types.ts ReviewTerminology + ReviewPromptContext candidates.ts generateCandidatesPrompt(ctx) validator.ts generateValidatorPrompt(ctx) src/create-prompt/terminology.ts GITHUB_TERMINOLOGY constant src/gitlab/prompts/terminology.ts GITLAB_TERMINOLOGY + factory Both platform-specific wrappers now just map their context shape onto ReviewPromptContext and delegate. GitLab's submit_review needs the MR IID re-asserted in the tool call, so we expose a gitlabTerminologyFor(mrIid) helper that bakes it into the submit_review hint. Net LOC is roughly even on this commit alone (+61), but ~280 LOC of literal duplication is gone, and any future shared prompt (security review/report when GitLab adds it) gets the shared scaffolding for free. All 29 prompt-specific tests still pass on both platforms; full suite 445/445; tsc clean. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * refactor(review): share ReviewArtifactPaths type + GitLab uses central disk-write helper The two platforms fetch review artifacts with fundamentally different mechanics (GitHub shells out to git/gh CLI with shallow-clone unshallow and a 50MB-buffered fallback; GitLab uses parallel REST calls), so the fetchers stay platform-specific. But the on-disk shape is identical, so: src/core/review/artifacts/types.ts ReviewArtifactPaths + ReviewArtifactContents shapes src/core/review/artifacts/write.ts writeReviewArtifacts(outDir, contents, names) — mkdir + parallel writeFile×3 GitLab's computeReviewArtifacts now delegates the disk-write through the shared helper. GitHub keeps its 3-helper public API (each does its own write so tests pin those signatures); it just re-exports the shared type via the existing ReviewArtifacts alias. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * refactor(review): hoist sticky-comment state/telemetry types + formatters to core The two platforms render the sticky review-tracking comment with very different mechanics today — GitHub builds it from inside the agent via the github-comment-server MCP tool, while GitLab writes a rich state- machine body from CI — so we can't share a single renderer. What we *can* share is the contract: src/core/review/tracking/types.ts ReviewTrackingState + ReviewTrackingTelemetry + ReviewTrackingFields src/core/review/tracking/format.ts formatDurationMs(ms) + formatCostUsd(usd) — keeps the "1m 23s • $0.0042" rendering identical across platforms GitLab's tracking-note.ts now imports those and re-exports the GitLab aliases (TrackingNoteState, TrackingNoteTelemetry, TrackingNoteOptions) so existing call sites stay untouched. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * refactor(review): extract @droid command parser to platform-agnostic core The actual string-matching for `@droid fill`, `@droid review`, `@droid security`, `@droid security --full`, and bare `@droid` is identical between platforms. Move parseDroidCommand + DroidCommand + ParsedCommand into: src/core/review/triggers/parse-command.ts GitHub's command-parser keeps extractCommandFromContext (which scans GitHub-payload-shaped events) and re-exports the shared types/parser so existing imports from `../utils/command-parser` stay valid. When the GitLab trigger path is ported (currently a pending task on the GitLab side), it can reuse parseDroidCommand directly instead of duplicating the regex set. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * fix(review): formatDurationMs rollover + single source of truth for GL security badge Two pre-existing bugs surfaced by the bot review on PR #96 — both ship on dev today, this just brings the fixes along with the refactor. 1. formatDurationMs returned "1m 60s" for ms values whose remainder seconds rounded up to 60 (e.g. ms=119600 → seconds=119.6 → minutes=1, remSec=round(59.6)=60). Round to whole seconds first, then split into minutes+seconds so the carry happens cleanly. Add regression coverage in test/core/review/tracking/format.test.ts covering the rollover boundary (119600/179600/239600 ms) plus the normal sub-second / sub-minute / multi-minute cases. 2. The GitLab security-badge URL in the prompt instruction (`security%20review-ran-blue`) didn't match the badge actually rendered by buildTrackingNoteBody (`security%20review-enabled-blue`, plus shields.io style params + logo). If the validator pass followed the prompt literally and the CI-prepended badge also fired, the MR could end up with two distinct security badges. Export the SECURITY_BADGE constant from tracking-note.ts and reference it from GITLAB_TERMINOLOGY.securityBadgeInstruction so the renderer is the single source of truth. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * fix(review): address local code-review findings on the refactor Three small issues caught by a thorough re-review of the refactor diff: 1. `securityBadgeInstruction` wrapped the badge markdown in single backticks (```![security](…)```), so an LLM literally following the prompt would paste the badge inside an inline-code span and produce `![security](...)` rendered as code instead of an image. Rephrased to emit the raw markdown with an explicit "no surrounding backticks or code fences" caveat. 2. The security-subagent prompt block interpolated `${t.baseRefLabel.toLowerCase()}` which produced awkward bare phrases ("pr base ref" / "mr target branch") instead of the original prompts' "base ref" / "target branch". Added a dedicated `baseRefShortLabel` field on ReviewTerminology and use it in the narrative prose; the noun-prefixed `baseRefLabel` is still used in the structured block where it matches today's wording. 3. format.ts docstring claimed the helpers are "shared between the GitHub MCP comment server and the GitLab tracking-note builder" but the GH side doesn't consume them yet. Softened the wording to reflect actual usage today plus the unification intent. Tests: 451/451 still pass; tsc clean. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --------- Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- src/core/review/artifacts/types.ts | 25 +++ src/core/review/artifacts/write.ts | 41 +++++ src/core/review/prompts/candidates.ts | 146 ++++++++++++++++ src/core/review/prompts/types.ts | 93 ++++++++++ src/core/review/prompts/validator.ts | 138 +++++++++++++++ src/core/review/tracking/format.ts | 25 +++ src/core/review/tracking/types.ts | 31 ++++ src/core/review/triggers/parse-command.ts | 74 ++++++++ .../templates/review-candidates-prompt.ts | 165 ++++-------------- .../templates/review-validator-prompt.ts | 155 ++++------------ src/create-prompt/terminology.ts | 33 ++++ src/create-prompt/types.ts | 7 +- src/github/utils/command-parser.ts | 96 ++-------- src/gitlab/data/review-artifacts.ts | 50 +++--- src/gitlab/operations/tracking-note.ts | 55 +++--- src/gitlab/prompts/candidates.ts | 161 +++-------------- src/gitlab/prompts/terminology.ts | 49 ++++++ src/gitlab/prompts/validator.ts | 145 +++------------ test/core/review/tracking/format.test.ts | 42 +++++ 19 files changed, 867 insertions(+), 664 deletions(-) create mode 100644 src/core/review/artifacts/types.ts create mode 100644 src/core/review/artifacts/write.ts create mode 100644 src/core/review/prompts/candidates.ts create mode 100644 src/core/review/prompts/types.ts create mode 100644 src/core/review/prompts/validator.ts create mode 100644 src/core/review/tracking/format.ts create mode 100644 src/core/review/tracking/types.ts create mode 100644 src/core/review/triggers/parse-command.ts create mode 100644 src/create-prompt/terminology.ts create mode 100644 src/gitlab/prompts/terminology.ts create mode 100644 test/core/review/tracking/format.test.ts diff --git a/src/core/review/artifacts/types.ts b/src/core/review/artifacts/types.ts new file mode 100644 index 0000000..62843d0 --- /dev/null +++ b/src/core/review/artifacts/types.ts @@ -0,0 +1,25 @@ +/** + * Shared shape for the three on-disk review artifacts. Both GitHub and + * GitLab pipelines pre-compute the same trio (diff, existing comments, + * description) and write them under `${tempDir}/droid-prompts/`, then + * pass the resulting paths into the Pass 1 / Pass 2 prompts. + * + * The fetch mechanics differ substantially per platform (git+gh CLI vs + * REST API), so we don't try to share the fetchers — only the path + * shape and the disk-write helper. + */ +export type ReviewArtifactPaths = { + diffPath: string; + commentsPath: string; + descriptionPath: string; +}; + +/** + * Raw content for each of the three artifacts, before they're written + * to disk by `writeReviewArtifacts`. + */ +export type ReviewArtifactContents = { + diff: string; + comments: unknown; // JSON-serializable + description: string; +}; diff --git a/src/core/review/artifacts/write.ts b/src/core/review/artifacts/write.ts new file mode 100644 index 0000000..4befa76 --- /dev/null +++ b/src/core/review/artifacts/write.ts @@ -0,0 +1,41 @@ +import * as fs from "fs/promises"; +import * as path from "path"; +import type { ReviewArtifactContents, ReviewArtifactPaths } from "./types"; + +/** + * Naming convention for review-artifact files on disk. Each platform + * gets its own basename for the diff and description (pr.diff / + * mr.diff, pr_description.txt / mr_description.txt) but the existing + * comments file is platform-neutral. + */ +export type ReviewArtifactNames = { + diff: string; + comments: string; + description: string; +}; + +/** + * Write the three review-artifact files into `outDir` in parallel, + * creating `outDir` if it doesn't yet exist. Returns the resolved + * on-disk paths so the caller can hand them straight to the prompt + * builder. + */ +export async function writeReviewArtifacts( + outDir: string, + contents: ReviewArtifactContents, + names: ReviewArtifactNames, +): Promise { + await fs.mkdir(outDir, { recursive: true }); + + const diffPath = path.join(outDir, names.diff); + const commentsPath = path.join(outDir, names.comments); + const descriptionPath = path.join(outDir, names.description); + + await Promise.all([ + fs.writeFile(diffPath, contents.diff), + fs.writeFile(commentsPath, JSON.stringify(contents.comments, null, 2)), + fs.writeFile(descriptionPath, contents.description), + ]); + + return { diffPath, commentsPath, descriptionPath }; +} diff --git a/src/core/review/prompts/candidates.ts b/src/core/review/prompts/candidates.ts new file mode 100644 index 0000000..d5fb278 --- /dev/null +++ b/src/core/review/prompts/candidates.ts @@ -0,0 +1,146 @@ +/** + * Platform-agnostic Pass 1 (candidate generation) prompt. + * + * Both `src/create-prompt/templates/review-candidates-prompt.ts` (GitHub) + * and `src/gitlab/prompts/candidates.ts` (GitLab) delegate to this builder + * via thin adapters that supply a `ReviewTerminology` object plus the + * runtime context (entity number, SHAs, artifact paths). The /review skill + * itself is platform-agnostic; this builder produces the runtime harness + * (where to read inputs, where to write the candidates JSON, what NOT to + * call) around it. + * + * STRICT contract: this prompt MUST NOT instruct the model to call any + * posting tool. Posting is Pass 2's responsibility, and tool gating is + * additionally enforced at the `droid exec` level via `--enabled-tools`. + */ + +import type { ReviewPromptContext } from "./types"; + +export function generateCandidatesPrompt(ctx: ReviewPromptContext): string { + const { + terminology: t, + entityNumber, + repoOrProject, + headRef, + headSha, + baseRef, + diffPath, + commentsPath, + descriptionPath, + candidatesPath, + includeSuggestions, + securityReviewEnabled, + } = ctx; + + const bodyFieldDescription = includeSuggestions + ? " - `body`: Comment text starting with priority tag [P0|P1|P2], then title, then 1 paragraph explanation.\n" + + " Follow the suggestion block rules from the review skill when including suggestions." + : " - `body`: Comment text starting with priority tag [P0|P1|P2], then title, then 1 paragraph explanation"; + + const sideFieldDescription = includeSuggestions + ? ' - `side`: "RIGHT" for new/modified code (default). Use "LEFT" only for removed code **without** suggestions.\n' + + " If you include a suggestion block, choose a RIGHT-side anchor and keep it unchanged so the validator can reuse it." + : ' - `side`: "RIGHT" for new/modified code (default), "LEFT" only for removed code'; + + const skillInstruction = includeSuggestions + ? "Invoke the 'review' skill to load the review methodology, then execute its **Pass 1: Candidate Generation** procedure — including suggestion block rules." + : "Invoke the 'review' skill to load the review methodology, then execute its **Pass 1: Candidate Generation** procedure. Do NOT include code suggestion blocks."; + + const securitySubagentInstruction = securityReviewEnabled + ? ` + +## Security Review (run concurrently) + +In addition to the code review, you MUST also spawn a \`security-reviewer\` subagent via the Task tool. +This subagent runs **concurrently** with the code review subagents during Step 2. + +Spawn it with: +- \`subagent_type\`: "security-reviewer" +- \`description\`: "Security review" +- \`prompt\`: Include the full ${t.entityNoun} context (${t.repoLabel.toLowerCase()}, ${t.entityNoun} ${t.metaEntityNumberKey === "prNumber" ? "number" : "IID"}, head SHA, ${t.baseRefShortLabel}) and the paths to precomputed data files (diff, description, existing comments). The security-reviewer will invoke the security-review skill and return a JSON array of security findings. + +**IMPORTANT**: Spawn the security-reviewer in the SAME response as the code review subagents so they all run in parallel. + +After all subagents complete (both code review and security-reviewer), merge the security findings into the \`comments\` array alongside code review findings. Security findings use the same schema but are prefixed with \`[security]\` in their body (e.g., \`[P1] [security] Title\`). +` + : ""; + + return `You are a senior staff software engineer and expert code reviewer. + +Your task: Review ${t.entityNoun} ${t.entityNumberSigil}${entityNumber} in ${repoOrProject} and generate a JSON file with **high-confidence, actionable** review comments that pinpoint genuine issues. + +${skillInstruction}${securitySubagentInstruction} + + +${t.repoLabel}: ${repoOrProject} +${t.entityNumberLabel}: ${entityNumber} +${t.headRefLabel}: ${headRef} +${t.headShaLabel}: ${headSha} +${t.baseRefLabel}: ${baseRef} + +Precomputed data files: +- ${t.descriptionLabel}: \`${descriptionPath}\` +- ${t.diffLabel}: \`${diffPath}\` +- Existing Comments: \`${commentsPath}\` + + + +Write output to \`${candidatesPath}\` using this exact schema: + +\`\`\`json +{ + "version": 1, + "meta": { + "${t.metaRepoKey}": "${t.repoExample}", + "${t.metaEntityNumberKey}": 123, + "headSha": "", + "${t.metaBaseRefKey}": "main", + "generatedAt": "" + }, + "comments": [ + { + "path": "src/index.ts", + "body": "[P1] Title\\n\\n1 paragraph.", + "line": 42, + "startLine": null, + "side": "RIGHT", + "commit_id": "" + } + ], + "reviewSummary": { + "body": "1-3 sentence overall assessment" + } +} +\`\`\` + + +- **version**: Always \`1\` + +- **meta**: Metadata object + - \`${t.metaRepoKey}\`: "${repoOrProject}" + - \`${t.metaEntityNumberKey}\`: ${entityNumber} + - \`headSha\`: "${headSha}" + - \`${t.metaBaseRefKey}\`: "${baseRef}" + - \`generatedAt\`: ISO 8601 timestamp (e.g., "2024-01-15T10:30:00Z") + +- **comments**: Array of comment objects + - \`path\`: ${t.pathFieldDescription} +${bodyFieldDescription} + - \`line\`: ${t.lineFieldDescription} + - \`startLine\`: \`null\` for single-line comments, or start line number for multi-line comments +${sideFieldDescription} + - \`commit_id\`: "${headSha}" + +- **reviewSummary**: + - \`body\`: 1-3 sentence overall assessment + + + + +**DO NOT** post to ${t.platformName}. +**DO NOT** invoke any ${t.entityNoun} mutation tools ${t.mutationToolForbiddance}. +**DO NOT** modify any files other than writing to \`${candidatesPath}\`. +Output ONLY the JSON file—no additional commentary. + +`; +} diff --git a/src/core/review/prompts/types.ts b/src/core/review/prompts/types.ts new file mode 100644 index 0000000..58313c7 --- /dev/null +++ b/src/core/review/prompts/types.ts @@ -0,0 +1,93 @@ +/** + * Shared types for the platform-agnostic review prompts. + * + * The candidate-generation (Pass 1) and validator (Pass 2) prompts are + * structurally identical between GitHub and GitLab. The only things that + * vary are labels, identifier names, MCP tool names, and a small number + * of platform-specific instruction lines. Those variations are captured + * here so the shared templates can stay platform-agnostic. + */ + +export interface ReviewTerminology { + /** "PR" or "MR" */ + entityNoun: string; + /** "#" for GitHub PRs, "!" for GitLab MRs */ + entityNumberSigil: string; + /** "GitHub" or "GitLab" */ + platformName: string; + /** Label for the repo/project value (e.g. "Repo" or "Project") */ + repoLabel: string; + /** Label for the entity number row in (e.g. "PR Number" or "MR IID") */ + entityNumberLabel: string; + /** Label for the source branch row (e.g. "PR Head Ref" or "MR Source Branch") */ + headRefLabel: string; + /** Label for the head SHA row (e.g. "PR Head SHA" or "MR Head SHA") */ + headShaLabel: string; + /** Label for the target branch row (e.g. "PR Base Ref" or "MR Target Branch") */ + baseRefLabel: string; + /** Short form of the target-branch concept, used in narrative prose (e.g. "base ref" or "target branch") */ + baseRefShortLabel: string; + /** Label for the description artifact (e.g. "PR Description" or "MR Description") */ + descriptionLabel: string; + /** Label for the diff artifact (e.g. "Full PR Diff" or "Full MR Diff") */ + diffLabel: string; + /** JSON meta key for the repo identifier (e.g. "repo" or "project") */ + metaRepoKey: string; + /** JSON meta key for the entity number (e.g. "prNumber" or "mrIid") */ + metaEntityNumberKey: string; + /** JSON meta key for the base ref (e.g. "baseRef" or "targetBranch") */ + metaBaseRefKey: string; + /** Example identifier in schema doc (e.g. "owner/repo" or "group/project") */ + repoExample: string; + /** Free-form description for the `path` field (small wording variation) */ + pathFieldDescription: string; + /** Free-form description for the `line` field (small wording variation) */ + lineFieldDescription: string; + /** Free-form blurb listing mutation tools the candidates pass MUST NOT use */ + mutationToolForbiddance: string; + /** MCP tool name for posting a batched review (Pass 2) */ + submitReviewToolName: string; + /** Extra args appended to the submit_review instruction (e.g. " along with `mr_iid: 5`") */ + submitReviewExtraArg: string; + /** Trailing clause on the "Do NOT include a body parameter" line */ + submitReviewBodyExclusionTrailer: string; + /** MCP tool name for updating the sticky tracking comment/note */ + updateTrackingToolName: string; + /** Human-readable name for the sticky comment (e.g. "tracking comment" or "sticky tracking note") */ + trackingCommentName: string; + /** "comment" (GH) or "top-level note" (GL), used in "post the summary as a separate X" */ + summaryEntityName: string; + /** Trailing clause on the summary-forbiddance line (e.g. " or as the body of `submit_review`" on GH; "" on GL) */ + summaryPostingExtraExclusion: string; + /** Approval/changes line at the end of validator instructions */ + approvalChangesNote: string; + /** Optional security-badge instruction line — GL only */ + securityBadgeInstruction?: string; +} + +export interface ReviewPromptContext { + terminology: ReviewTerminology; + /** PR number / MR IID */ + entityNumber: number | string; + /** Full repo/project identifier */ + repoOrProject: string; + /** Source branch name */ + headRef: string; + /** Head SHA */ + headSha: string; + /** Target branch name */ + baseRef: string; + /** On-disk path to the diff artifact */ + diffPath: string; + /** On-disk path to existing-comments JSON */ + commentsPath: string; + /** On-disk path to description text */ + descriptionPath: string; + /** On-disk path where Pass 1 writes the candidates JSON */ + candidatesPath: string; + /** On-disk path where Pass 2 writes the validated JSON (validator only) */ + validatedPath?: string; + includeSuggestions: boolean; + /** Spawn security-reviewer subagent during Pass 1 (candidates only) */ + securityReviewEnabled: boolean; +} diff --git a/src/core/review/prompts/validator.ts b/src/core/review/prompts/validator.ts new file mode 100644 index 0000000..50c2c40 --- /dev/null +++ b/src/core/review/prompts/validator.ts @@ -0,0 +1,138 @@ +/** + * Platform-agnostic Pass 2 (validator) prompt. + * + * The validator reads the candidates JSON produced by Pass 1, validates + * each one, writes a refined JSON to disk, and posts the approved + * findings as a single batched call to the platform's submit-review tool. + * Both `src/create-prompt/templates/review-validator-prompt.ts` (GitHub) + * and `src/gitlab/prompts/validator.ts` (GitLab) delegate to this builder + * via thin adapters that supply a `ReviewTerminology` shape. + * + * Pass 2 is the only place where the posting tool is exposed (via + * `--enabled-tools` on the second `droid exec` invocation). + */ + +import type { ReviewPromptContext } from "./types"; + +export function generateValidatorPrompt(ctx: ReviewPromptContext): string { + const { + terminology: t, + entityNumber, + repoOrProject, + headRef, + headSha, + baseRef, + diffPath, + commentsPath, + descriptionPath, + candidatesPath, + validatedPath, + includeSuggestions, + } = ctx; + + if (!validatedPath) { + throw new Error("validator prompt requires validatedPath in context"); + } + + const skillInstruction = includeSuggestions + ? "Invoke the 'review' skill to load the review methodology, then execute its **Pass 2: Validation** procedure — including suggestion block rules." + : "Invoke the 'review' skill to load the review methodology, then execute its **Pass 2: Validation** procedure. Do NOT include code suggestion blocks."; + + const securityBadgeLine = t.securityBadgeInstruction + ? `\n* ${t.securityBadgeInstruction}` + : ""; + + return `You are validating candidate review comments for ${t.entityNoun} ${t.entityNumberSigil}${entityNumber} in ${repoOrProject}. + +IMPORTANT: This is Phase 2 (validator) of a two-pass review pipeline. + +${skillInstruction} + +### Context + +* ${t.repoLabel}: ${repoOrProject} +* ${t.entityNumberLabel}: ${entityNumber} +* ${t.headRefLabel}: ${headRef} +* ${t.headShaLabel}: ${headSha} +* ${t.baseRefLabel}: ${baseRef} + +### Inputs + +Read these files before validating: +* ${t.descriptionLabel}: \`${descriptionPath}\` +* Candidates: \`${candidatesPath}\` +* ${t.diffLabel}: \`${diffPath}\` +* Existing Comments: \`${commentsPath}\` + +If the diff is large, read in chunks (offset/limit). **Do not proceed until you have read the ENTIRE diff.** + +### Critical Requirements + +1. You MUST read and validate **every** candidate before posting anything. +2. Preserve ordering: keep results in the same order as candidates. +3. **Posting rule (STRICT):** Only post comments where \`status === "approved"\`. Never post rejected items. + +### Output: Write \`${validatedPath}\` + +\`\`\`json +{ + "version": 1, + "meta": { + "${t.metaRepoKey}": "${repoOrProject}", + "${t.metaEntityNumberKey}": ${entityNumber}, + "headSha": "${headSha}", + "${t.metaBaseRefKey}": "${baseRef}", + "validatedAt": "" + }, + "results": [ + { + "status": "approved", + "comment": { + "path": "src/index.ts", + "body": "[P1] Title\\n\\n1 paragraph.", + "line": 42, + "startLine": null, + "side": "RIGHT", + "commit_id": "${headSha}" + } + }, + { + "status": "rejected", + "candidate": { + "path": "src/other.ts", + "body": "[P2] ...", + "line": 10, + "startLine": null, + "side": "RIGHT", + "commit_id": "${headSha}" + }, + "reason": "Not a real bug because ..." + } + ], + "reviewSummary": { + "status": "approved", + "body": "1-3 sentence overall assessment" + } +} +\`\`\` + +Notes: +* Use \`commit_id\` = \`${headSha}\`. +* \`results\` MUST have exactly one entry per candidate, in the same order. + +Tooling note: +* If the tools list includes \`ApplyPatch\` (common for OpenAI models like GPT-5.2), use \`ApplyPatch\` to create/update the file at the exact path. +* Otherwise, use \`Create\` (or \`Edit\` if overwriting) to write the file. + +### Post approved items + +After writing \`${validatedPath}\`, post comments ONLY for \`status === "approved"\`: + +* Collect all approved comments and submit them as a **single batched review** via \`${t.submitReviewToolName}\`, passing them in the \`comments\` array parameter${t.submitReviewExtraArg}. +* Do **NOT** post comments individually — batch them all into one \`submit_review\` call. +* Do **NOT** include a \`body\` parameter in \`submit_review\`${t.submitReviewBodyExclusionTrailer}. +* Use \`${t.updateTrackingToolName}\` to update the ${t.trackingCommentName} with the review summary.${securityBadgeLine} +* Do **NOT** post the summary as a separate ${t.summaryEntityName}${t.summaryPostingExtraExclusion}. +* ${t.approvalChangesNote} +`; +} diff --git a/src/core/review/tracking/format.ts b/src/core/review/tracking/format.ts new file mode 100644 index 0000000..b008cb1 --- /dev/null +++ b/src/core/review/tracking/format.ts @@ -0,0 +1,25 @@ +/** + * Small formatting helpers for sticky review-tracking + * comments/notes. Currently consumed by the GitLab tracking-note + * builder (`src/gitlab/operations/tracking-note.ts`); kept platform- + * agnostic in core so the GitHub MCP comment server can adopt the same + * "1m 23s • $0.0042" rendering when its telemetry path is unified. + */ + +export function formatDurationMs(ms: number): string { + if (ms < 1000) return `${ms}ms`; + const seconds = ms / 1000; + if (seconds < 60) return `${seconds.toFixed(1)}s`; + // Round to whole seconds first, then split into minutes+seconds, so + // that a remainder rounding up to 60 carries cleanly into the next + // minute (e.g. 119600ms → "2m 0s", not "1m 60s"). + const totalSeconds = Math.round(seconds); + const minutes = Math.floor(totalSeconds / 60); + const remSec = totalSeconds - minutes * 60; + return `${minutes}m ${remSec}s`; +} + +export function formatCostUsd(usd: number): string { + if (usd >= 1) return `$${usd.toFixed(2)}`; + return `$${usd.toFixed(4)}`; +} diff --git a/src/core/review/tracking/types.ts b/src/core/review/tracking/types.ts new file mode 100644 index 0000000..5a24b64 --- /dev/null +++ b/src/core/review/tracking/types.ts @@ -0,0 +1,31 @@ +/** + * Shared shapes for sticky review-tracking comments/notes. + * + * Both GitHub and GitLab keep a single "sticky" comment per PR/MR that + * carries the live status of the Droid review pipeline (running → + * success/failure) plus telemetry. The body-building mechanics differ + * between platforms today — GitHub updates it from inside the agent via + * the github-comment-server MCP tool, while GitLab builds the whole body + * in TypeScript and writes it from CI — so we only share the contract + * (state machine + telemetry shape) here, not the renderer itself. + */ + +export type ReviewTrackingState = "running" | "success" | "failure"; + +export type ReviewTrackingTelemetry = { + totalNumTurns?: number | null; + totalDurationMs?: number | null; + totalCostUsd?: number | null; + pass1SessionId?: string | null; + pass2SessionId?: string | null; +}; + +export interface ReviewTrackingFields { + state: ReviewTrackingState; + pipelineUrl?: string | null; + jobUrl?: string | null; + triggerUsername?: string | null; + errorDetails?: string | null; + securityReviewRan?: boolean; + telemetry?: ReviewTrackingTelemetry | null; +} diff --git a/src/core/review/triggers/parse-command.ts b/src/core/review/triggers/parse-command.ts new file mode 100644 index 0000000..725f4a5 --- /dev/null +++ b/src/core/review/triggers/parse-command.ts @@ -0,0 +1,74 @@ +/** + * Platform-agnostic Droid command parser. + * + * Both GitHub and GitLab look for the same `@droid ` mentions + * in PR/MR bodies and comments. This parser only operates on a raw + * string; the per-platform context extraction (which fields of which + * webhook payload to scan) stays in the platform-specific module + * because the payload shapes diverge. + */ + +export type DroidCommand = + | "fill" + | "review" + | "security" + | "security-full" + | "default"; + +export interface ParsedCommand { + command: DroidCommand; + raw: string; + location: "body" | "comment"; + timestamp?: string | null; +} + +/** + * Parses text to detect specific @droid commands. + * + * Returns `null` when no `@droid` mention is present, the generic + * `default` command for a bare `@droid`, or the specific subcommand + * when matched. `location` defaults to "body" and is overridden by + * the caller when the text came from a comment. + */ +export function parseDroidCommand(text: string): ParsedCommand | null { + if (!text) { + return null; + } + + const fillMatch = text.match(/@droid\s+fill/i); + if (fillMatch) { + return { command: "fill", raw: fillMatch[0], location: "body" }; + } + + // Note: `@droid review security` will match as just `@droid review`. + const reviewMatch = text.match(/@droid\s+review/i); + if (reviewMatch) { + return { command: "review", raw: reviewMatch[0], location: "body" }; + } + + const securityFullMatch = text.match(/@droid\s+security\s+--full/i); + if (securityFullMatch) { + return { + command: "security-full", + raw: securityFullMatch[0], + location: "body", + }; + } + + // Check after security-full to avoid false matches. + const securityMatch = text.match(/@droid\s+security(?:\s|$|[^-\w])/i); + if (securityMatch) { + return { + command: "security", + raw: securityMatch[0].trim(), + location: "body", + }; + } + + const droidMatch = text.match(/@droid/i); + if (droidMatch) { + return { command: "default", raw: droidMatch[0], location: "body" }; + } + + return null; +} diff --git a/src/create-prompt/templates/review-candidates-prompt.ts b/src/create-prompt/templates/review-candidates-prompt.ts index a596041..310d551 100644 --- a/src/create-prompt/templates/review-candidates-prompt.ts +++ b/src/create-prompt/templates/review-candidates-prompt.ts @@ -1,3 +1,14 @@ +/** + * GitHub Pass-1 (candidate generation) prompt — thin adapter. + * + * Delegates to the platform-agnostic builder in + * `src/core/review/prompts/candidates.ts`, mapping the GitHub + * `PreparedContext` shape onto the shared `ReviewPromptContext` and + * supplying `GITHUB_TERMINOLOGY` (PR labels, github_* MCP tool names). + */ + +import { generateCandidatesPrompt } from "../../core/review/prompts/candidates"; +import { GITHUB_TERMINOLOGY } from "../terminology"; import type { PreparedContext } from "../types"; export function generateReviewCandidatesPrompt( @@ -9,137 +20,25 @@ export function generateReviewCandidatesPrompt( ? String(context.githubContext.entityNumber) : "unknown"; - const repoFullName = context.repository; - const prHeadRef = context.prBranchData?.headRefName ?? "unknown"; - const prHeadSha = context.prBranchData?.headRefOid ?? "unknown"; - const prBaseRef = context.eventData.baseBranch ?? "unknown"; - - const diffPath = - context.reviewArtifacts?.diffPath ?? "$RUNNER_TEMP/droid-prompts/pr.diff"; - const commentsPath = - context.reviewArtifacts?.commentsPath ?? - "$RUNNER_TEMP/droid-prompts/existing_comments.json"; - const descriptionPath = - context.reviewArtifacts?.descriptionPath ?? - "$RUNNER_TEMP/droid-prompts/pr_description.txt"; - - const reviewCandidatesPath = - process.env.REVIEW_CANDIDATES_PATH ?? - "$RUNNER_TEMP/droid-prompts/review_candidates.json"; - - const includeSuggestions = context.includeSuggestions !== false; - - const bodyFieldDescription = includeSuggestions - ? " - `body`: Comment text starting with priority tag [P0|P1|P2], then title, then 1 paragraph explanation.\n" + - " Follow the suggestion block rules from the review skill when including suggestions." - : " - `body`: Comment text starting with priority tag [P0|P1|P2], then title, then 1 paragraph explanation"; - - const sideFieldDescription = includeSuggestions - ? ' - `side`: "RIGHT" for new/modified code (default). Use "LEFT" only for removed code **without** suggestions.\n' + - " If you include a suggestion block, choose a RIGHT-side anchor and keep it unchanged so the validator can reuse it." - : ' - `side`: "RIGHT" for new/modified code (default), "LEFT" only for removed code'; - - const skillInstruction = includeSuggestions - ? "Invoke the 'review' skill to load the review methodology, then execute its **Pass 1: Candidate Generation** procedure — including suggestion block rules." - : "Invoke the 'review' skill to load the review methodology, then execute its **Pass 1: Candidate Generation** procedure. Do NOT include code suggestion blocks."; - - const securityReviewEnabled = process.env.SECURITY_REVIEW_ENABLED === "true"; - - const securitySubagentInstruction = securityReviewEnabled - ? ` - -## Security Review (run concurrently) - -In addition to the code review, you MUST also spawn a \`security-reviewer\` subagent via the Task tool. -This subagent runs **concurrently** with the code review subagents during Step 2. - -Spawn it with: -- \`subagent_type\`: "security-reviewer" -- \`description\`: "Security review" -- \`prompt\`: Include the full PR context (repo, PR number, head SHA, base ref) and the paths to precomputed data files (diff, description, existing comments). The security-reviewer will invoke the security-review skill and return a JSON array of security findings. - -**IMPORTANT**: Spawn the security-reviewer in the SAME response as the code review subagents so they all run in parallel. - -After all subagents complete (both code review and security-reviewer), merge the security findings into the \`comments\` array alongside code review findings. Security findings use the same schema but are prefixed with \`[security]\` in their body (e.g., \`[P1] [security] Title\`). -` - : ""; - - return `You are a senior staff software engineer and expert code reviewer. - -Your task: Review PR #${prNumber} in ${repoFullName} and generate a JSON file with **high-confidence, actionable** review comments that pinpoint genuine issues. - -${skillInstruction}${securitySubagentInstruction} - - -Repo: ${repoFullName} -PR Number: ${prNumber} -PR Head Ref: ${prHeadRef} -PR Head SHA: ${prHeadSha} -PR Base Ref: ${prBaseRef} - -Precomputed data files: -- PR Description: \`${descriptionPath}\` -- Full PR Diff: \`${diffPath}\` -- Existing Comments: \`${commentsPath}\` - - - -Write output to \`${reviewCandidatesPath}\` using this exact schema: - -\`\`\`json -{ - "version": 1, - "meta": { - "repo": "owner/repo", - "prNumber": 123, - "headSha": "", - "baseRef": "main", - "generatedAt": "" - }, - "comments": [ - { - "path": "src/index.ts", - "body": "[P1] Title\\n\\n1 paragraph.", - "line": 42, - "startLine": null, - "side": "RIGHT", - "commit_id": "" - } - ], - "reviewSummary": { - "body": "1-3 sentence overall assessment" - } -} -\`\`\` - - -- **version**: Always \`1\` - -- **meta**: Metadata object - - \`repo\`: "${repoFullName}" - - \`prNumber\`: ${prNumber} - - \`headSha\`: "${prHeadSha}" - - \`baseRef\`: "${prBaseRef}" - - \`generatedAt\`: ISO 8601 timestamp (e.g., "2024-01-15T10:30:00Z") - -- **comments**: Array of comment objects - - \`path\`: Relative file path (e.g., "src/index.ts") -${bodyFieldDescription} - - \`line\`: Target line number (single-line) or end line number (multi-line). Must be ≥ 0. - - \`startLine\`: \`null\` for single-line comments, or start line number for multi-line comments -${sideFieldDescription} - - \`commit_id\`: "${prHeadSha}" - -- **reviewSummary**: - - \`body\`: 1-3 sentence overall assessment - - - - -**DO NOT** post to GitHub. -**DO NOT** invoke any PR mutation tools (inline comments, submit review, delete/minimize/reply/resolve, etc.). -**DO NOT** modify any files other than writing to \`${reviewCandidatesPath}\`. -Output ONLY the JSON file—no additional commentary. - -`; + return generateCandidatesPrompt({ + terminology: GITHUB_TERMINOLOGY, + entityNumber: prNumber, + repoOrProject: context.repository, + headRef: context.prBranchData?.headRefName ?? "unknown", + headSha: context.prBranchData?.headRefOid ?? "unknown", + baseRef: context.eventData.baseBranch ?? "unknown", + diffPath: + context.reviewArtifacts?.diffPath ?? "$RUNNER_TEMP/droid-prompts/pr.diff", + commentsPath: + context.reviewArtifacts?.commentsPath ?? + "$RUNNER_TEMP/droid-prompts/existing_comments.json", + descriptionPath: + context.reviewArtifacts?.descriptionPath ?? + "$RUNNER_TEMP/droid-prompts/pr_description.txt", + candidatesPath: + process.env.REVIEW_CANDIDATES_PATH ?? + "$RUNNER_TEMP/droid-prompts/review_candidates.json", + includeSuggestions: context.includeSuggestions !== false, + securityReviewEnabled: process.env.SECURITY_REVIEW_ENABLED === "true", + }); } diff --git a/src/create-prompt/templates/review-validator-prompt.ts b/src/create-prompt/templates/review-validator-prompt.ts index 866ceb0..ded234f 100644 --- a/src/create-prompt/templates/review-validator-prompt.ts +++ b/src/create-prompt/templates/review-validator-prompt.ts @@ -1,3 +1,14 @@ +/** + * GitHub Pass-2 (validator) prompt — thin adapter. + * + * Delegates to the platform-agnostic builder in + * `src/core/review/prompts/validator.ts`, mapping the GitHub + * `PreparedContext` shape onto the shared `ReviewPromptContext` and + * supplying `GITHUB_TERMINOLOGY`. + */ + +import { generateValidatorPrompt } from "../../core/review/prompts/validator"; +import { GITHUB_TERMINOLOGY } from "../terminology"; import type { PreparedContext } from "../types"; export function generateReviewValidatorPrompt( @@ -9,124 +20,28 @@ export function generateReviewValidatorPrompt( ? String(context.githubContext.entityNumber) : "unknown"; - const repoFullName = context.repository; - const prHeadRef = context.prBranchData?.headRefName ?? "unknown"; - const prHeadSha = context.prBranchData?.headRefOid ?? "unknown"; - const prBaseRef = context.eventData.baseBranch ?? "unknown"; - - const diffPath = - context.reviewArtifacts?.diffPath ?? "$RUNNER_TEMP/droid-prompts/pr.diff"; - const commentsPath = - context.reviewArtifacts?.commentsPath ?? - "$RUNNER_TEMP/droid-prompts/existing_comments.json"; - const descriptionPath = - context.reviewArtifacts?.descriptionPath ?? - "$RUNNER_TEMP/droid-prompts/pr_description.txt"; - - const reviewCandidatesPath = - process.env.REVIEW_CANDIDATES_PATH ?? - "$RUNNER_TEMP/droid-prompts/review_candidates.json"; - const reviewValidatedPath = - process.env.REVIEW_VALIDATED_PATH ?? - "$RUNNER_TEMP/droid-prompts/review_validated.json"; - - const includeSuggestions = context.includeSuggestions !== false; - - const skillInstruction = includeSuggestions - ? "Invoke the 'review' skill to load the review methodology, then execute its **Pass 2: Validation** procedure — including suggestion block rules." - : "Invoke the 'review' skill to load the review methodology, then execute its **Pass 2: Validation** procedure. Do NOT include code suggestion blocks."; - - return `You are validating candidate review comments for PR #${prNumber} in ${repoFullName}. - -IMPORTANT: This is Phase 2 (validator) of a two-pass review pipeline. - -${skillInstruction} - -### Context - -* Repo: ${repoFullName} -* PR Number: ${prNumber} -* PR Head Ref: ${prHeadRef} -* PR Head SHA: ${prHeadSha} -* PR Base Ref: ${prBaseRef} - -### Inputs - -Read these files before validating: -* PR Description: \`${descriptionPath}\` -* Candidates: \`${reviewCandidatesPath}\` -* Full PR Diff: \`${diffPath}\` -* Existing Comments: \`${commentsPath}\` - -If the diff is large, read in chunks (offset/limit). **Do not proceed until you have read the ENTIRE diff.** - -### Critical Requirements - -1. You MUST read and validate **every** candidate before posting anything. -2. Preserve ordering: keep results in the same order as candidates. -3. **Posting rule (STRICT):** Only post comments where \`status === "approved"\`. Never post rejected items. - -### Output: Write \`${reviewValidatedPath}\` - -\`\`\`json -{ - "version": 1, - "meta": { - "repo": "${repoFullName}", - "prNumber": ${prNumber}, - "headSha": "${prHeadSha}", - "baseRef": "${prBaseRef}", - "validatedAt": "" - }, - "results": [ - { - "status": "approved", - "comment": { - "path": "src/index.ts", - "body": "[P1] Title\\n\\n1 paragraph.", - "line": 42, - "startLine": null, - "side": "RIGHT", - "commit_id": "${prHeadSha}" - } - }, - { - "status": "rejected", - "candidate": { - "path": "src/other.ts", - "body": "[P2] ...", - "line": 10, - "startLine": null, - "side": "RIGHT", - "commit_id": "${prHeadSha}" - }, - "reason": "Not a real bug because ..." - } - ], - "reviewSummary": { - "status": "approved", - "body": "1-3 sentence overall assessment" - } -} -\`\`\` - -Notes: -* Use \`commit_id\` = \`${prHeadSha}\`. -* \`results\` MUST have exactly one entry per candidate, in the same order. - -Tooling note: -* If the tools list includes \`ApplyPatch\` (common for OpenAI models like GPT-5.2), use \`ApplyPatch\` to create/update the file at the exact path. -* Otherwise, use \`Create\` (or \`Edit\` if overwriting) to write the file. - -### Post approved items - -After writing \`${reviewValidatedPath}\`, post comments ONLY for \`status === "approved"\`: - -* Collect all approved comments and submit them as a **single batched review** via \`github_pr___submit_review\`, passing them in the \`comments\` array parameter. -* Do **NOT** post comments individually — batch them all into one \`submit_review\` call. -* Do **NOT** include a \`body\` parameter in \`submit_review\`. -* Use \`github_comment___update_droid_comment\` to update the tracking comment with the review summary. -* Do **NOT** post the summary as a separate comment or as the body of \`submit_review\`. -* Do not approve or request changes. -`; + return generateValidatorPrompt({ + terminology: GITHUB_TERMINOLOGY, + entityNumber: prNumber, + repoOrProject: context.repository, + headRef: context.prBranchData?.headRefName ?? "unknown", + headSha: context.prBranchData?.headRefOid ?? "unknown", + baseRef: context.eventData.baseBranch ?? "unknown", + diffPath: + context.reviewArtifacts?.diffPath ?? "$RUNNER_TEMP/droid-prompts/pr.diff", + commentsPath: + context.reviewArtifacts?.commentsPath ?? + "$RUNNER_TEMP/droid-prompts/existing_comments.json", + descriptionPath: + context.reviewArtifacts?.descriptionPath ?? + "$RUNNER_TEMP/droid-prompts/pr_description.txt", + candidatesPath: + process.env.REVIEW_CANDIDATES_PATH ?? + "$RUNNER_TEMP/droid-prompts/review_candidates.json", + validatedPath: + process.env.REVIEW_VALIDATED_PATH ?? + "$RUNNER_TEMP/droid-prompts/review_validated.json", + includeSuggestions: context.includeSuggestions !== false, + securityReviewEnabled: process.env.SECURITY_REVIEW_ENABLED === "true", + }); } diff --git a/src/create-prompt/terminology.ts b/src/create-prompt/terminology.ts new file mode 100644 index 0000000..d3b2ef2 --- /dev/null +++ b/src/create-prompt/terminology.ts @@ -0,0 +1,33 @@ +import type { ReviewTerminology } from "../core/review/prompts/types"; + +export const GITHUB_TERMINOLOGY: ReviewTerminology = { + entityNoun: "PR", + entityNumberSigil: "#", + platformName: "GitHub", + repoLabel: "Repo", + entityNumberLabel: "PR Number", + headRefLabel: "PR Head Ref", + headShaLabel: "PR Head SHA", + baseRefLabel: "PR Base Ref", + baseRefShortLabel: "base ref", + descriptionLabel: "PR Description", + diffLabel: "Full PR Diff", + metaRepoKey: "repo", + metaEntityNumberKey: "prNumber", + metaBaseRefKey: "baseRef", + repoExample: "owner/repo", + pathFieldDescription: 'Relative file path (e.g., "src/index.ts")', + lineFieldDescription: + "Target line number (single-line) or end line number (multi-line). Must be ≥ 0.", + mutationToolForbiddance: + "(inline comments, submit review, delete/minimize/reply/resolve, etc.)", + submitReviewToolName: "github_pr___submit_review", + submitReviewExtraArg: "", + submitReviewBodyExclusionTrailer: "", + updateTrackingToolName: "github_comment___update_droid_comment", + trackingCommentName: "tracking comment", + summaryEntityName: "comment", + summaryPostingExtraExclusion: " or as the body of `submit_review`", + approvalChangesNote: "Do not approve or request changes.", + // No security-badge instruction on GitHub today; left undefined intentionally. +}; diff --git a/src/create-prompt/types.ts b/src/create-prompt/types.ts index f9945e0..8ec8238 100644 --- a/src/create-prompt/types.ts +++ b/src/create-prompt/types.ts @@ -1,4 +1,5 @@ import type { GitHubContext } from "../github/context"; +import type { ReviewArtifactPaths } from "../core/review/artifacts/types"; export type CommonFields = { repository: string; @@ -103,11 +104,7 @@ export type EventData = | PullRequestEvent | PullRequestTargetEvent; -export type ReviewArtifacts = { - diffPath: string; - commentsPath: string; - descriptionPath: string; -}; +export type ReviewArtifacts = ReviewArtifactPaths; export type PreparedContext = CommonFields & { eventData: EventData; diff --git a/src/github/utils/command-parser.ts b/src/github/utils/command-parser.ts index 5a9ca27..9903038 100644 --- a/src/github/utils/command-parser.ts +++ b/src/github/utils/command-parser.ts @@ -1,87 +1,19 @@ /** - * Command parser for detecting specific @droid commands in GitHub comments and PR bodies + * Command parser for detecting specific @droid commands in GitHub + * comments and PR bodies. The pure string-parsing portion is platform- + * agnostic and lives in `src/core/review/triggers/parse-command.ts`; + * this file owns the GitHub-payload-aware context extraction. */ import type { GitHubContext } from "../context"; +import { + parseDroidCommand, + type DroidCommand, + type ParsedCommand, +} from "../../core/review/triggers/parse-command"; -export type DroidCommand = - | "fill" - | "review" - | "security" - | "security-full" - | "default"; - -export interface ParsedCommand { - command: DroidCommand; - raw: string; - location: "body" | "comment"; - timestamp?: string | null; -} - -/** - * Parses text to detect specific @droid commands - * @param text The text to parse (comment body or PR description) - * @returns ParsedCommand if a command is found, null otherwise - */ -export function parseDroidCommand(text: string): ParsedCommand | null { - if (!text) { - return null; - } - - // Check for @droid fill command (case insensitive) - const fillMatch = text.match(/@droid\s+fill/i); - if (fillMatch) { - return { - command: "fill", - raw: fillMatch[0], - location: "body", // Will be set by caller - }; - } - - // Check for @droid review command (case insensitive) - // Note: @droid review security will match as just @droid review - const reviewMatch = text.match(/@droid\s+review/i); - if (reviewMatch) { - return { - command: "review", - raw: reviewMatch[0], - location: "body", // Will be set by caller - }; - } - - // Check for @droid security --full command (case insensitive) - const securityFullMatch = text.match(/@droid\s+security\s+--full/i); - if (securityFullMatch) { - return { - command: "security-full", - raw: securityFullMatch[0], - location: "body", // Will be set by caller - }; - } - - // Check for @droid security command (case insensitive) - // Must check after security-full to avoid false matches - const securityMatch = text.match(/@droid\s+security(?:\s|$|[^-\w])/i); - if (securityMatch) { - return { - command: "security", - raw: securityMatch[0].trim(), - location: "body", // Will be set by caller - }; - } - - // Check for generic @droid mention (default behavior) - const droidMatch = text.match(/@droid/i); - if (droidMatch) { - return { - command: "default", - raw: droidMatch[0], - location: "body", // Will be set by caller - }; - } - - return null; -} +export type { DroidCommand, ParsedCommand }; +export { parseDroidCommand }; /** * Extracts a droid command from the GitHub context @@ -91,12 +23,10 @@ export function parseDroidCommand(text: string): ParsedCommand | null { export function extractCommandFromContext( context: GitHubContext, ): ParsedCommand | null { - // Handle missing payload if (!context.payload) { return null; } - // Check PR body for commands (pull_request events) if ( context.eventName === "pull_request" && "pull_request" in context.payload @@ -110,7 +40,6 @@ export function extractCommandFromContext( } } - // Check issue body for commands (issues events) if (context.eventName === "issues" && "issue" in context.payload) { const body = context.payload.issue.body; if (body) { @@ -121,7 +50,6 @@ export function extractCommandFromContext( } } - // Check comment body for commands (issue_comment events) if (context.eventName === "issue_comment" && "comment" in context.payload) { const comment = context.payload.comment; if (comment.body) { @@ -136,7 +64,6 @@ export function extractCommandFromContext( } } - // Check review comment body (pull_request_review_comment events) if ( context.eventName === "pull_request_review_comment" && "comment" in context.payload @@ -154,7 +81,6 @@ export function extractCommandFromContext( } } - // Check review body (pull_request_review events) if ( context.eventName === "pull_request_review" && "review" in context.payload diff --git a/src/gitlab/data/review-artifacts.ts b/src/gitlab/data/review-artifacts.ts index be45775..6dcd706 100644 --- a/src/gitlab/data/review-artifacts.ts +++ b/src/gitlab/data/review-artifacts.ts @@ -9,19 +9,17 @@ * These mirror the artifacts the GitHub Action precomputes * (`pr.diff`, `existing_comments.json`, `pr_description.txt`) so the GitLab * Pass-1 / Pass-2 prompts can refer to them by stable paths and the model - * doesn't have to round-trip through MCP tools to fetch them. + * doesn't have to round-trip through MCP tools to fetch them. The disk + * write itself uses the shared `writeReviewArtifacts` helper in + * `src/core/review/artifacts/write.ts`. */ -import * as fs from "fs/promises"; -import * as path from "path"; +import { writeReviewArtifacts } from "../../core/review/artifacts/write"; +import type { ReviewArtifactPaths } from "../../core/review/artifacts/types"; import type { GitlabClient } from "../api/client"; import type { GitlabMr, GitlabMrChanges, GitlabNote } from "../types"; -export type GitlabReviewArtifactPaths = { - diffPath: string; - commentsPath: string; - descriptionPath: string; -}; +export type GitlabReviewArtifactPaths = ReviewArtifactPaths; export type GitlabReviewArtifacts = GitlabReviewArtifactPaths & { mr: GitlabMr; @@ -62,33 +60,25 @@ export async function computeReviewArtifacts( ): Promise { const { client, projectId, mrIid, outDir } = opts; - await fs.mkdir(outDir, { recursive: true }); - const [mr, changes, notes] = await Promise.all([ client.getMr(projectId, mrIid), client.getMrChanges(projectId, mrIid), client.listNotes(projectId, mrIid), ]); - const diffPath = path.join(outDir, "mr.diff"); - const commentsPath = path.join(outDir, "existing_comments.json"); - const descriptionPath = path.join(outDir, "mr_description.txt"); - - const diffContent = buildDiffContent(changes); - const descriptionContent = buildDescriptionContent(mr); - - await Promise.all([ - fs.writeFile(diffPath, diffContent), - fs.writeFile(commentsPath, JSON.stringify(notes, null, 2)), - fs.writeFile(descriptionPath, descriptionContent), - ]); + const paths = await writeReviewArtifacts( + outDir, + { + diff: buildDiffContent(changes), + comments: notes, + description: buildDescriptionContent(mr), + }, + { + diff: "mr.diff", + comments: "existing_comments.json", + description: "mr_description.txt", + }, + ); - return { - diffPath, - commentsPath, - descriptionPath, - mr, - changes, - notes, - }; + return { ...paths, mr, changes, notes }; } diff --git a/src/gitlab/operations/tracking-note.ts b/src/gitlab/operations/tracking-note.ts index 286da9d..f33ccae 100644 --- a/src/gitlab/operations/tracking-note.ts +++ b/src/gitlab/operations/tracking-note.ts @@ -3,46 +3,33 @@ * * The tracking note carries a hidden HTML marker so we can find and * update the same note across retries instead of creating duplicates. + * + * The state machine (running/success/failure), telemetry shape, and + * formatting helpers are platform-agnostic and live in + * `src/core/review/tracking/`. This file owns the GitLab-specific body + * rendering, including the markdown layout, the security badge, the + * error-details accordion, and the hidden marker conventions. */ +import { + formatCostUsd, + formatDurationMs, +} from "../../core/review/tracking/format"; +import type { + ReviewTrackingFields, + ReviewTrackingState, +} from "../../core/review/tracking/types"; + export const DROID_TRACKING_MARKER = ""; export const DROID_SECURITY_BADGE_MARKER = ""; -export type TrackingNoteState = "running" | "success" | "failure"; - -export type TrackingNoteTelemetry = { - totalNumTurns?: number | null; - totalDurationMs?: number | null; - totalCostUsd?: number | null; - pass1SessionId?: string | null; - pass2SessionId?: string | null; -}; - -export interface TrackingNoteOptions { - state: TrackingNoteState; - pipelineUrl?: string | null; - jobUrl?: string | null; - triggerUsername?: string | null; - errorDetails?: string | null; - securityReviewRan?: boolean; - telemetry?: TrackingNoteTelemetry | null; -} - -function formatDurationMs(ms: number): string { - if (ms < 1000) return `${ms}ms`; - const seconds = ms / 1000; - if (seconds < 60) return `${seconds.toFixed(1)}s`; - const minutes = Math.floor(seconds / 60); - const remSec = Math.round(seconds - minutes * 60); - return `${minutes}m ${remSec}s`; -} - -function formatCostUsd(usd: number): string { - if (usd >= 1) return `$${usd.toFixed(2)}`; - return `$${usd.toFixed(4)}`; -} +export type TrackingNoteState = ReviewTrackingState; +export type TrackingNoteTelemetry = NonNullable< + ReviewTrackingFields["telemetry"] +>; +export type TrackingNoteOptions = ReviewTrackingFields; -const SECURITY_BADGE = +export const SECURITY_BADGE = "![security](https://img.shields.io/badge/security%20review-enabled-blue?style=flat-square&logo=shield) "; const STATE_HEADER: Record = { diff --git a/src/gitlab/prompts/candidates.ts b/src/gitlab/prompts/candidates.ts index aa5966a..413c393 100644 --- a/src/gitlab/prompts/candidates.ts +++ b/src/gitlab/prompts/candidates.ts @@ -1,148 +1,35 @@ /** - * GitLab Pass-1 (candidate generation) prompt. + * GitLab Pass-1 (candidate generation) prompt — thin adapter. * - * Direct port of the GitHub Action's - * `src/create-prompt/templates/review-candidates-prompt.ts` with PR→MR - * terminology and GitLab-specific entity numbering. The /review skill - * itself is platform-agnostic and contains the two-pass methodology; this - * file is the runtime harness that tells Droid which pass it's in, where - * the input artifacts live on disk, and where to write the candidate JSON. + * Delegates to the platform-agnostic builder in + * `src/core/review/prompts/candidates.ts` after mapping the GitLab + * context shape onto the shared `ReviewPromptContext` and supplying + * `GITLAB_TERMINOLOGY` (PR→MR labels, GitLab MCP tool names, etc.). * - * STRICT contract: this prompt MUST NOT instruct the model to call the - * GitLab posting tool (`gitlab_mr___submit_review`). Posting only happens - * in Pass 2. Tool gating is also enforced at the `droid exec` level by - * excluding `gitlab_mr___submit_review` from `--enabled-tools` during - * Pass 1. + * The GitLab posting tool is exposed only in Pass 2; this Pass 1 prompt + * MUST NOT instruct the model to call it. The shared builder enforces + * that, plus tool gating at the `droid exec` level via `--enabled-tools`. */ +import { generateCandidatesPrompt } from "../../core/review/prompts/candidates"; +import { GITLAB_TERMINOLOGY } from "./terminology"; import type { GitlabReviewPromptContext } from "./types"; export function generateGitlabReviewCandidatesPrompt( ctx: GitlabReviewPromptContext, ): string { - const { - projectPath, - mrIid, - sourceBranch, - targetBranch, - headSha, - diffPath, - commentsPath, - descriptionPath, - candidatesPath, - includeSuggestions, - securityReviewEnabled, - } = ctx; - - const bodyFieldDescription = includeSuggestions - ? " - `body`: Comment text starting with priority tag [P0|P1|P2], then title, then 1 paragraph explanation.\n" + - " Follow the suggestion block rules from the review skill when including suggestions." - : " - `body`: Comment text starting with priority tag [P0|P1|P2], then title, then 1 paragraph explanation"; - - const sideFieldDescription = includeSuggestions - ? ' - `side`: "RIGHT" for new/modified code (default). Use "LEFT" only for removed code **without** suggestions.\n' + - " If you include a suggestion block, choose a RIGHT-side anchor and keep it unchanged so the validator can reuse it." - : ' - `side`: "RIGHT" for new/modified code (default), "LEFT" only for removed code'; - - const skillInstruction = includeSuggestions - ? "Invoke the 'review' skill to load the review methodology, then execute its **Pass 1: Candidate Generation** procedure — including suggestion block rules." - : "Invoke the 'review' skill to load the review methodology, then execute its **Pass 1: Candidate Generation** procedure. Do NOT include code suggestion blocks."; - - const securitySubagentInstruction = securityReviewEnabled - ? ` - -## Security Review (run concurrently) - -In addition to the code review, you MUST also spawn a \`security-reviewer\` subagent via the Task tool. -This subagent runs **concurrently** with the code review subagents during Step 2. - -Spawn it with: -- \`subagent_type\`: "security-reviewer" -- \`description\`: "Security review" -- \`prompt\`: Include the full MR context (project, MR IID, head SHA, target branch) and the paths to precomputed data files (diff, description, existing comments). The security-reviewer will invoke the security-review skill and return a JSON array of security findings. - -**IMPORTANT**: Spawn the security-reviewer in the SAME response as the code review subagents so they all run in parallel. - -After all subagents complete (both code review and security-reviewer), merge the security findings into the \`comments\` array alongside code review findings. Security findings use the same schema but are prefixed with \`[security]\` in their body (e.g., \`[P1] [security] Title\`). -` - : ""; - - return `You are a senior staff software engineer and expert code reviewer. - -Your task: Review MR !${mrIid} in ${projectPath} and generate a JSON file with **high-confidence, actionable** review comments that pinpoint genuine issues. - -${skillInstruction}${securitySubagentInstruction} - - -Project: ${projectPath} -MR IID: ${mrIid} -MR Source Branch: ${sourceBranch} -MR Head SHA: ${headSha} -MR Target Branch: ${targetBranch} - -Precomputed data files: -- MR Description: \`${descriptionPath}\` -- Full MR Diff: \`${diffPath}\` -- Existing Comments: \`${commentsPath}\` - - - -Write output to \`${candidatesPath}\` using this exact schema: - -\`\`\`json -{ - "version": 1, - "meta": { - "project": "group/project", - "mrIid": 123, - "headSha": "", - "targetBranch": "main", - "generatedAt": "" - }, - "comments": [ - { - "path": "src/index.ts", - "body": "[P1] Title\\n\\n1 paragraph.", - "line": 42, - "startLine": null, - "side": "RIGHT", - "commit_id": "" - } - ], - "reviewSummary": { - "body": "1-3 sentence overall assessment" - } -} -\`\`\` - - -- **version**: Always \`1\` - -- **meta**: Metadata object - - \`project\`: "${projectPath}" - - \`mrIid\`: ${mrIid} - - \`headSha\`: "${headSha}" - - \`targetBranch\`: "${targetBranch}" - - \`generatedAt\`: ISO 8601 timestamp (e.g., "2024-01-15T10:30:00Z") - -- **comments**: Array of comment objects - - \`path\`: Relative file path (use the new_path from the diff, e.g., "src/index.ts") -${bodyFieldDescription} - - \`line\`: Target line number in the new file (single-line) or end line number (multi-line). Must be ≥ 0. - - \`startLine\`: \`null\` for single-line comments, or start line number for multi-line comments -${sideFieldDescription} - - \`commit_id\`: "${headSha}" - -- **reviewSummary**: - - \`body\`: 1-3 sentence overall assessment - - - - -**DO NOT** post to GitLab. -**DO NOT** invoke any MR mutation tools (\`gitlab_mr___submit_review\`, \`gitlab_mr___create_mr_note\`, \`gitlab_mr___update_mr_note\`, \`gitlab_mr___update_mr_description\`, \`gitlab_mr___update_tracking_note\`, etc.). -**DO NOT** modify any files other than writing to \`${candidatesPath}\`. -Output ONLY the JSON file—no additional commentary. - -`; + return generateCandidatesPrompt({ + terminology: GITLAB_TERMINOLOGY, + entityNumber: ctx.mrIid, + repoOrProject: ctx.projectPath, + headRef: ctx.sourceBranch, + headSha: ctx.headSha, + baseRef: ctx.targetBranch, + diffPath: ctx.diffPath, + commentsPath: ctx.commentsPath, + descriptionPath: ctx.descriptionPath, + candidatesPath: ctx.candidatesPath, + includeSuggestions: ctx.includeSuggestions, + securityReviewEnabled: ctx.securityReviewEnabled, + }); } diff --git a/src/gitlab/prompts/terminology.ts b/src/gitlab/prompts/terminology.ts new file mode 100644 index 0000000..c2616b3 --- /dev/null +++ b/src/gitlab/prompts/terminology.ts @@ -0,0 +1,49 @@ +import type { ReviewTerminology } from "../../core/review/prompts/types"; +import { SECURITY_BADGE } from "../operations/tracking-note"; + +export const GITLAB_TERMINOLOGY: ReviewTerminology = { + entityNoun: "MR", + entityNumberSigil: "!", + platformName: "GitLab", + repoLabel: "Project", + entityNumberLabel: "MR IID", + headRefLabel: "MR Source Branch", + headShaLabel: "MR Head SHA", + baseRefLabel: "MR Target Branch", + baseRefShortLabel: "target branch", + descriptionLabel: "MR Description", + diffLabel: "Full MR Diff", + metaRepoKey: "project", + metaEntityNumberKey: "mrIid", + metaBaseRefKey: "targetBranch", + repoExample: "group/project", + pathFieldDescription: + 'Relative file path (use the new_path from the diff, e.g., "src/index.ts")', + lineFieldDescription: + "Target line number in the new file (single-line) or end line number (multi-line). Must be ≥ 0.", + mutationToolForbiddance: + "(`gitlab_mr___submit_review`, `gitlab_mr___create_mr_note`, `gitlab_mr___update_mr_note`, `gitlab_mr___update_mr_description`, `gitlab_mr___update_tracking_note`, etc.)", + submitReviewToolName: "gitlab_mr___submit_review", + submitReviewExtraArg: "", // populated dynamically below via factory + submitReviewBodyExclusionTrailer: + " (we use a separate tracking note for the summary)", + updateTrackingToolName: "gitlab_mr___update_tracking_note", + trackingCommentName: "sticky tracking note", + summaryEntityName: "top-level note", + summaryPostingExtraExclusion: "", + approvalChangesNote: + "Do not approve the MR or request changes (GitLab approval rules are handled out-of-band).", + securityBadgeInstruction: `If any approved comments contain \`[security]\` in their body, ensure a security badge is prepended to the tracking note body using exactly this markdown image (no surrounding backticks or code fences): ${SECURITY_BADGE.trim()} — this indicates that security analysis was performed as part of the review.`, +}; + +/** + * Build the GitLab terminology with the runtime MR IID baked into the + * "passing them in the comments array parameter along with mr_iid: N" arg. + * GitLab's submit_review tool needs the IID re-asserted in the call. + */ +export function gitlabTerminologyFor(mrIid: number): ReviewTerminology { + return { + ...GITLAB_TERMINOLOGY, + submitReviewExtraArg: ` along with \`mr_iid: ${mrIid}\``, + }; +} diff --git a/src/gitlab/prompts/validator.ts b/src/gitlab/prompts/validator.ts index 9606e96..602a9a6 100644 --- a/src/gitlab/prompts/validator.ts +++ b/src/gitlab/prompts/validator.ts @@ -1,132 +1,37 @@ /** - * GitLab Pass-2 (validator) prompt. + * GitLab Pass-2 (validator) prompt — thin adapter. * - * Direct port of the GitHub Action's - * `src/create-prompt/templates/review-validator-prompt.ts` with PR→MR - * terminology and the GitLab MCP tool names. The validator reads the - * candidates JSON produced by Pass 1 from disk, validates each one, - * writes a refined JSON, and posts the approved findings as a single - * batched call to `gitlab_mr___submit_review`. + * Delegates to the platform-agnostic builder in + * `src/core/review/prompts/validator.ts` after mapping the GitLab + * context shape onto the shared `ReviewPromptContext` and supplying + * `gitlabTerminologyFor(mrIid)` (which bakes the MR IID into the + * submit_review call hint, since GitLab's tool requires `mr_iid` to be + * re-asserted in the invocation). * - * Pass 2 is the only place where the posting tool is exposed (via + * Pass 2 is the only place the GitLab posting tool is exposed (via * `--enabled-tools` on the second `droid exec` invocation). */ +import { generateValidatorPrompt } from "../../core/review/prompts/validator"; +import { gitlabTerminologyFor } from "./terminology"; import type { GitlabReviewPromptContext } from "./types"; export function generateGitlabReviewValidatorPrompt( ctx: GitlabReviewPromptContext, ): string { - const { - projectPath, - mrIid, - sourceBranch, - targetBranch, - headSha, - diffPath, - commentsPath, - descriptionPath, - candidatesPath, - validatedPath, - includeSuggestions, - } = ctx; - - const skillInstruction = includeSuggestions - ? "Invoke the 'review' skill to load the review methodology, then execute its **Pass 2: Validation** procedure — including suggestion block rules." - : "Invoke the 'review' skill to load the review methodology, then execute its **Pass 2: Validation** procedure. Do NOT include code suggestion blocks."; - - return `You are validating candidate review comments for MR !${mrIid} in ${projectPath}. - -IMPORTANT: This is Phase 2 (validator) of a two-pass review pipeline. - -${skillInstruction} - -### Context - -* Project: ${projectPath} -* MR IID: ${mrIid} -* MR Source Branch: ${sourceBranch} -* MR Head SHA: ${headSha} -* MR Target Branch: ${targetBranch} - -### Inputs - -Read these files before validating: -* MR Description: \`${descriptionPath}\` -* Candidates: \`${candidatesPath}\` -* Full MR Diff: \`${diffPath}\` -* Existing Comments: \`${commentsPath}\` - -If the diff is large, read in chunks (offset/limit). **Do not proceed until you have read the ENTIRE diff.** - -### Critical Requirements - -1. You MUST read and validate **every** candidate before posting anything. -2. Preserve ordering: keep results in the same order as candidates. -3. **Posting rule (STRICT):** Only post comments where \`status === "approved"\`. Never post rejected items. - -### Output: Write \`${validatedPath}\` - -\`\`\`json -{ - "version": 1, - "meta": { - "project": "${projectPath}", - "mrIid": ${mrIid}, - "headSha": "${headSha}", - "targetBranch": "${targetBranch}", - "validatedAt": "" - }, - "results": [ - { - "status": "approved", - "comment": { - "path": "src/index.ts", - "body": "[P1] Title\\n\\n1 paragraph.", - "line": 42, - "startLine": null, - "side": "RIGHT", - "commit_id": "${headSha}" - } - }, - { - "status": "rejected", - "candidate": { - "path": "src/other.ts", - "body": "[P2] ...", - "line": 10, - "startLine": null, - "side": "RIGHT", - "commit_id": "${headSha}" - }, - "reason": "Not a real bug because ..." - } - ], - "reviewSummary": { - "status": "approved", - "body": "1-3 sentence overall assessment" - } -} -\`\`\` - -Notes: -* Use \`commit_id\` = \`${headSha}\`. -* \`results\` MUST have exactly one entry per candidate, in the same order. - -Tooling note: -* If the tools list includes \`ApplyPatch\` (common for OpenAI models like GPT-5.2), use \`ApplyPatch\` to create/update the file at the exact path. -* Otherwise, use \`Create\` (or \`Edit\` if overwriting) to write the file. - -### Post approved items - -After writing \`${validatedPath}\`, post comments ONLY for \`status === "approved"\`: - -* Collect all approved comments and submit them as a **single batched review** via \`gitlab_mr___submit_review\`, passing them in the \`comments\` array parameter along with \`mr_iid: ${mrIid}\`. -* Do **NOT** post comments individually — batch them all into one \`submit_review\` call. -* Do **NOT** include a \`body\` parameter in \`submit_review\` (we use a separate tracking note for the summary). -* Use \`gitlab_mr___update_tracking_note\` to update the sticky tracking note with the review summary. -* If any approved comments contain \`[security]\` in their body, prepend a security badge to the tracking note: \`![Security Review](https://img.shields.io/badge/security%20review-ran-blue)\`. This indicates that security analysis was performed as part of the review. -* Do **NOT** post the summary as a separate top-level note. -* Do not approve the MR or request changes (GitLab approval rules are handled out-of-band). -`; + return generateValidatorPrompt({ + terminology: gitlabTerminologyFor(ctx.mrIid), + entityNumber: ctx.mrIid, + repoOrProject: ctx.projectPath, + headRef: ctx.sourceBranch, + headSha: ctx.headSha, + baseRef: ctx.targetBranch, + diffPath: ctx.diffPath, + commentsPath: ctx.commentsPath, + descriptionPath: ctx.descriptionPath, + candidatesPath: ctx.candidatesPath, + validatedPath: ctx.validatedPath, + includeSuggestions: ctx.includeSuggestions, + securityReviewEnabled: ctx.securityReviewEnabled, + }); } diff --git a/test/core/review/tracking/format.test.ts b/test/core/review/tracking/format.test.ts new file mode 100644 index 0000000..faf9aea --- /dev/null +++ b/test/core/review/tracking/format.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, it } from "bun:test"; +import { + formatCostUsd, + formatDurationMs, +} from "../../../../src/core/review/tracking/format"; + +describe("formatDurationMs", () => { + it("renders sub-second durations in ms", () => { + expect(formatDurationMs(0)).toBe("0ms"); + expect(formatDurationMs(750)).toBe("750ms"); + }); + + it("renders sub-minute durations to one decimal in seconds", () => { + expect(formatDurationMs(1500)).toBe("1.5s"); + expect(formatDurationMs(59999)).toBe("60.0s"); + }); + + it("renders multi-minute durations as `Xm Ys`", () => { + expect(formatDurationMs(60000)).toBe("1m 0s"); + expect(formatDurationMs(90000)).toBe("1m 30s"); + expect(formatDurationMs(125000)).toBe("2m 5s"); + }); + + it("carries the remainder cleanly when seconds round up to 60", () => { + // Pre-fix bug: 119600ms produced "1m 60s" because remSec rounded to 60. + expect(formatDurationMs(119600)).toBe("2m 0s"); + expect(formatDurationMs(179600)).toBe("3m 0s"); + expect(formatDurationMs(239600)).toBe("4m 0s"); + }); +}); + +describe("formatCostUsd", () => { + it("uses 4 decimals under $1", () => { + expect(formatCostUsd(0.0042)).toBe("$0.0042"); + expect(formatCostUsd(0.5)).toBe("$0.5000"); + }); + + it("uses 2 decimals at $1 or above", () => { + expect(formatCostUsd(1)).toBe("$1.00"); + expect(formatCostUsd(12.345)).toBe("$12.35"); + }); +});