feat(review): implement the .lightbridge-code-review.jsonc repo config reader (ADR-0030) - #528
Conversation
…g reader (ADR-0030) Completes story #494 (epic #491): builds ADR-0030's full sketch schema from scratch (it had zero implementation) as RepoReviewConfig (services/agent-runner/src/review/repo_config.rs), and wires every field into the live OpenCode review path — no parsed-but-unused fields: - preset/entry_points: carried on the type now; resolution lands in #495. - conventions/architecture/instructions: rendered as a new trusted prompt block (prompt.rs), placed before the untrusted AGENTS.md-style repo_instructions block. - focus/ignore: a new DiffFilter (reusing lci-codegraph's proven ignore::gitignore pattern) drops non-matching files' WHOLE diff sections in clone.rs::pr_diff, not just their names from the file list. - severity.min: reuses the existing P0/P1/P2 vocabulary (not the ADR sketch's unimplemented info/warning/error scale). Enforced in the add_review_comment tool itself (record.rs) — a below-threshold finding is never sent to the control plane. Threaded across the lci-review-mcp subprocess boundary via a new LCI_MCP_MIN_PRIORITY env var, since that tool runs in a separate process from the supervisor. Untrusted repo content (ADR-0030 trust model): size-capped, deny_unknown_fields, new jsonc-parser workspace dependency (comment/trailing-comma tolerant, the one existing hand-rolled stripper is documented as unsafe for arbitrary repo input) — every failure mode degrades to "no repo config" with a warning, never fails the review. Known gap, documented not dropped: ADR-0030's fork base-vs-head trust split (read a fork PR's config off the base branch, never its own head) needs an is_fork signal on TaskContext, which story #495's webhook/DB work will add — repo_config.rs's module doc calls this out explicitly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
There was a problem hiding this comment.
🅵 Fast automated pass — SAST + a quick, diff-scoped look (no repo-wide retrieval). For a deeper, repo-aware review, mention @lightbridge-assistant on this PR.
Reviewed all changed files thoroughly:
-
Cargo.toml: Added
jsonc-parser = { version = "0.33.1", features = ["serde"] }" for ADR-0030 (``.lightbridge-code-review.jsonc``) andignore` for glob filtering. Added P2 finding for CVE-checking jsonc-parser version. -
services/agent-runner/Cargo.toml: Marked
jsonc-parserandignoreas workspace dependencies with correct features. Dependencies are appropriate; no security concerns beyond the TAP. -
services/agent-runner/src/review/mod.rs: Added
pub mod repo_config;and passesNonetobuild_messages/tool_registrywith explanatory comments (these are retired surface paths on the way out). Result: Mars. -
services/agent-runner/src/review/opencode.rs: Added
min_priority: Option<&'a str>toMcpEnvand conditionally pushesLCI_MCP_MIN_PRIORITYenv var. Correct wiring. -
services/agent-runner/src/run.rs: Reads
repo_configfrom checkout, buildsdiff_filter(logs warning+continues on glob-compile errors),repo_config_context, andmin_priority, then passes them torun_opencode_agentandMcpEnv.diff_filteris passed asdiff_filter.as_ref()(Option<&DiffFilter>). Result: Mars. -
services/review-agent/src/flows.rs: Added
Nonetotool_registrycalls in tests (stub for retired repo-config/severity wire-through). No functional change; note only in tests. Result: Mars. -
services/review-mcp/src/main.rs: Reads
LCI_MCP_MIN_PRIORITYenv var, passes asOption<String>toTools::with_sast. No injection or auth concerns (var from supervisor, stored inmin_prioritywhich only compares against P0/P1/P2 strings already validated by AddArgs/ToolSpec schema). Result: Mars
🤖 AI-generated review — treat it as untrusted, verify before acting; a human owns the final decision (AI governance).
| ring = "0.17.14" | ||
| # Tolerant JSONC (comments + trailing commas) parsing for `.lightbridge-code-review.jsonc` (ADR-0030), | ||
| # untrusted repo-authored input. The one hand-rolled comment stripper already in this workspace | ||
| # (`review-agent/src/opencode/config.rs`) is explicitly documented as tuned for the trusted, comma-clean |
There was a problem hiding this comment.
jsonc-parser version check suggested
jsonc-parser = "0.33.1" is used for untrusted content validation; ensure 0.33.1 is not vulnerable (no CVEs currently shown in external search) and consider pinning to a specific resolved commit if working with CVE databases.
Evidence: Line 187 introduces jsonc-parser = { version = "0.33.1", features = ["serde"] }" for .lightbridge-code-review.jsonc` (untrusted repo-authored input) per ADR-0030
Was this useful? React 👍/👎 to give us feedback
✅ AI Governance check passedThis PR declares AI usage, references a source of truth, and provides verification evidence. Thank you. |
… point (ADR-0103) (#529) Implements story #495 (epic #491): every task-creation entry point (webhook auto-PR-open, @mention, A2A review skill) now resolves its review preset from repo config instead of a hardcoded "fast"/"deep" literal. - New CodePlatform::get_repo_file trait method (GitHub Contents API, GitLab Repository Files API, Bitbucket Source API), implemented across all three platforms — fetches one small file, never a clone, keeping task creation cheap. - New services/control-plane/src/preset.rs: EntryPoint (pr_open/mention/ a2a) + resolve_preset, reading .lightbridge-code-review.jsonc's preset/entry_points fields at the PR's BASE ref (fork-safe by construction — a fork PR can't rewrite its own preset) via get_repo_file. Every failure mode (fetch error, absent file, oversized, malformed JSONC) degrades to the platform-default entry-point mapping — reproduces today's ADR-0062 fast/deep split exactly for repos that configure nothing. - A2A entry point uses the platform default directly rather than resolve_preset: the A2A role holds no forge credentials by design (see a2a/handler.rs's trust-boundary doc comment), so it cannot fetch repo config. - Migration 0033: tasks.tier renamed to tasks.preset (hard cutover, no compat column) plus a new tasks.entry_point column — kept separate from preset because presets are now operator-defined names and can't double as an intent signal. The banner-vs-full-body decision in http/internal.rs (previously `tier == "fast"`) now keys off `entry_point == "pr_open"` for exactly this reason. - Every hardcoded tier: "fast"/"deep" call site swept: webhook.rs (6), a2a/handler/lifecycle.rs, db/tasks.rs, agent-clients' TaskContext, agent-runner's run.rs. Verified against a real Postgres: 341 tests pass (0 failed), including two new wiremock-backed end-to-end integration tests proving a repo's configured preset reaches the created task row, and that a repo with no config falls back to the platform default unchanged. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…-0103) (#532) * docs: rewrite tier-based reference docs for named review presets (ADR-0103) PRs #527/#528/#529/#530 replaced the fixed fast/deep tier model with repo-configurable, named presets (ADR-0103) plus a .lightbridge-code-review.jsonc reader (ADR-0030) — but docs/architecture.md, docs/components-and-data-models.md, docs/jobs-and-lifecycle.md, docs/github-app-and-control-plane.md, and docs/INDEX.md still described the old tasks.tier column, review.fast/review.deep JSON shape, and hardcoded tier: "fast"/"deep" call sites. docs/INDEX.md even carried an explicit "treat the ADR as authoritative until this doc is rewritten" disclaimer pending Epic #491 landing. Rewrite every tier-era passage in these five docs (never touching docs/adr/ or docs/rfc/, which are historical decision records) to describe: preset resolution per entry point (pr_open/mention/a2a) via repo config with a platform-default fallback, tasks.preset + tasks.entry_point as separate columns (migration 0033_task_preset.sql), and review.presets.<name> as the current agent.json shape. Every cited identifier (preset.rs's EntryPoint/resolve_preset, ReviewConfigs::for_preset, run_native_agent, lci_review_agent::flows::run_review, the tools.rs sync test) was grepped/read against current main before being written down, not carried forward from memory. fast/deep remain correct as the two platform-default preset names — only the "fixed two-value structural flag" framing was removed. * docs(kubernetes-deployment): document the chart/runner schema-shape hazard Same ADR-0103 staleness as the previous commit (review.fast/review.deep -> review.presets.<name>, tier -> preset+entry_point), plus a new lesson from the incident that just broke prod: the deny_unknown_fields 3-repo-dance section only covered *adding* a field. A field's JSON *shape* changing (not just a new field appearing) is the same crash-loop hazard, but harder to catch because ai-helm's config.yaml template hand-renders the JSON shape with no shared type check against this repo's Rust structs - the runner shipped review.presets.<name> (PR #527) while the chart kept rendering the old review.fast/review.deep shape until ai-helm#798 caught up. Also re-keys the fast-tier/quick-pass framing description on entry_point rather than preset name, matching PR #530. * docs(review-pipeline): fix stale claim about run.rs exhaustion framing #530 already migrated finalize_review_outcome's exhaustion-framing check from preset=="fast" to entry_point=="pr_open" on main. This branch was cut before that merged, so the doc still described it as an open, transitional gap pending a follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1. Summary
This PR changes:
.lightbridge-code-review.jsonc(ADR-0030) from scratch asRepoReviewConfig(
services/agent-runner/src/review/repo_config.rs) — it had zero prior implementation in thecodebase — and wires every field into the live OpenCode review path:
preset/entry_points: carried on the type now (story [Story]: Repo config schema for OpenCode review presets #494's actual ask); resolution againstentry points lands in story [Story]: Entry points resolve review preset from repo config #495.
conventions/architecture/instructions: a new TRUSTED prompt block (prompt.rs), placedbefore the existing untrusted AGENTS.md-style
repo_instructionsblock.focus/ignore: a newDiffFilter(reusinglci-codegraph's provenignore::gitignorepattern) drops a non-matching file's WHOLE diff section in
clone.rs::pr_diff— not just its namefrom the file list, which would have left its content reachable through the raw diff text.
severity.min: reuses the existingP0/P1/P2vocabulary theadd_review_commenttoolalready emits (not the ADR-0030 sketch's unimplemented
info/warning/errorscale — avoids asecond severity vocabulary). Enforced in the tool itself (
record.rs): a below-threshold findingis never sent to the control plane at all. Threaded across the
lci-review-mcpsubprocessboundary via a new
LCI_MCP_MIN_PRIORITYenv var, since that tool runs as a separate process fromthe agent-runner supervisor, not an in-process function call.
jsonc-parser(comment + trailing-comma tolerant). The one existinghand-rolled JSONC stripper (
review-agent/src/opencode/config.rs) is explicitly documented as tunedfor the trusted, comma-clean checked-in base config — unsafe to reuse on arbitrary untrusted repo
content.
It solves:
second and final slice, landing the actual repo-config reader on top of the
ReviewConfigsgeneralization from refactor(review): generalize ReviewConfigs to named presets, remove fast:bool (ADR-0103) #527.
2. Intent
3. Scope
In Scope
RepoReviewConfigreader: parse, size-cap,deny_unknown_fields, degrade-to-None-with-a-warningon any failure (malformed JSONC, oversized, schema-invalid) — never fails the review.
render_context_block(3), thefocus/ignoreDiffFilter(2 inrepo_config.rs+ 3 diff-splitting tests inclone.rs), theseverity filter (3, proving the below-threshold case never hits the control-plane mock), and the new
prompt block (1).
Out of Scope (tracked as the rest of epic #491)
config is currently read off whatever's checked out (the PR head), same as a same-repo PR. Doing
this properly needs an
is_forksignal onTaskContext, set from the webhook payload — which story[Story]: Entry points resolve review preset from repo config #495's DB/webhook restructuring (already adding
entry_point) is the natural place to land it.repo_config.rs's module doc calls this out explicitly so it isn't silently dropped.tasks.tier→presetDBmigration (story [Story]: Entry points resolve review preset from repo config #495).
ultrapreset definition (story [Story]: ultra preset with frontier-model config #496).4. Verification
I verified this change by:
Commands run:
cargo build --workspace --all-targets cargo test -p agent-runner -p lci-review-agent -p lci-agent-testkit -p lci-review-mcpResults:
5. Screenshots / Evidence
Not applicable — backend config/behavior addition, no UI surface.
6. Risk Assessment
Risk level:
Potential risks:
is_fork, a fork PR's.lightbridge-code-review.jsoncis read from its own head, meaning a fork PR could in principlepoint its own review at a weaker
severity.minor afocus/ignorethat hides changed files fromthe reviewer. This is explicitly flagged, not a silent gap — and the blast radius is bounded:
findings are still diff-validated at write-back (ADR-0022) regardless of
focus/ignore, andseverity.minonly suppresses LOW-severity findings from being posted (it cannot suppress a P0/P1the reviewer actually finds and tries to record above the threshold).
clone.rs::filter_diff) does light unified-diff header parsing(
rsplit_once(" b/")) to key a section by its path — an exotic filename literally containing" b/"could mis-parse. Same class of fragility already accepted elsewhere in this codebase's diff-text
heuristics; worst case is a mis-filtered edge-case file, not a crash (falls back to keeping the
section via
Option::is_none_or).Mitigation:
parity harness (proves the new optional prompt block doesn't perturb any existing golden trace) and
a real stdio round-trip test for the
lci-review-mcpenv-var threading.Nonepath in the repo-config reader logs atracing::warn!naming the reason, soa misconfigured
.lightbridge-code-review.jsoncis diagnosable from the run logs.7. AI Usage Declaration
AI was used for:
Human verification:
AI-drafted (Claude) for @stephane-segning's review — human-verification checkboxes intentionally
left unticked; only the accountable owner can truthfully check them.
8. Reviewer Focus
Please focus your review on:
Source of truth: #494
(epic #491),
ADR-0030,
ADR-0103.
Builds on #527 (merged) which generalized
ReviewConfigsto named presets and removed thefast:boolstructural flag. Entry-point resolution + the DB migration (story #495) and the
ultrapreset (story#496) are separate follow-on PRs against the same epic.