Skip to content

feat(control-plane): resolve review preset from repo config per entry point (ADR-0103) - #529

Merged
stephane-segning merged 1 commit into
claude/stories-494-495-496-b0cd38from
claude/story-495-entry-point-presets
Jul 28, 2026
Merged

feat(control-plane): resolve review preset from repo config per entry point (ADR-0103)#529
stephane-segning merged 1 commit into
claude/stories-494-495-496-b0cd38from
claude/story-495-entry-point-presets

Conversation

@stephane-segning

Copy link
Copy Markdown
Contributor

1. Summary

This PR changes:

  • Every task-creation entry point (webhook auto-PR-open, @mention, A2A review skill) now resolves
    its review preset from repo config (.lightbridge-code-review.jsonc, ADR-0030) instead of a
    hardcoded "fast"/"deep" literal.
  • New CodePlatform::get_repo_file trait method — a single small file fetch, never a clone —
    implemented across GitHub (Contents API), GitLab (Repository Files API), and Bitbucket (Source
    API).
  • New services/control-plane/src/preset.rs: EntryPoint (pr_open/mention/a2a) +
    resolve_preset, reading the repo config's preset/entry_points fields at the PR's base ref
    (fork-safe by construction — a fork PR can't rewrite its own preset by editing the file on its own
    branch). Every failure mode (fetch error, absent file, oversized, malformed JSONC) degrades to the
    platform-default entry-point mapping, reproducing today's ADR-0062 fast/deep split exactly for a
    repo that configures nothing.
  • Migration 0033_task_preset.sql: tasks.tier renamed to tasks.preset (hard cutover, no compat
    column) plus a new tasks.entry_point column.
  • Every hardcoded tier: "fast"/"deep" call site swept: webhook.rs (6 sites across GitHub/GitLab/
    Bitbucket), a2a/handler/lifecycle.rs, db/tasks.rs, agent-clients' TaskContext,
    agent-runner's run.rs.

It solves:


2. Intent

The preset config schema (story #494) is useless until every task-creation call site reads it
instead of hardcoding "fast"/"deep". This PR closes that gap: the control plane fetches the
repo's config at webhook time (a single cheap file read via the platform's API — control-plane never
clones repos, that's the agent-runner Job's job after the task already exists), resolves a preset
per entry point, and persists both the resolved preset and which entry point created the task —
because a preset name is now operator-defined and can no longer double as an intent signal for
presentation decisions like "was this the automatic on-open pass" (the old context.tier == "fast"
banner check, now context.entry_point == "pr_open").


3. Scope

In Scope

  • CodePlatform::get_repo_file + 3 platform implementations.
  • preset::resolve_preset/resolve_preset_or_default + EntryPoint.
  • DB migration + full call-site sweep (webhook.rs, a2a lifecycle, db/tasks.rs, agent-clients,
    agent-runner).
  • The internal.rs banner-vs-full-body fix (keyed off entry_point, not preset name).
  • Two new wiremock-backed end-to-end integration tests (custom preset resolved; no-config fallback)
    plus unit tests for the resolver's pure logic.

Out of Scope

  • A2A entry point does not resolve preset from repo config — it uses the platform default
    (deep) directly. The A2A role holds no forge credentials by design (its own module doc: "this role
    NEVER launches a Job or touches a forge... holds no forge credentials") — giving it a CodePlatform
    client to fetch config would hand it the ability to call every other platform method too (post
    reviews, mint tokens), which is exactly the trust boundary that doc comment exists to prevent. This
    is a deliberate, documented scope boundary, not an oversight.
  • The ultra preset's own model/budget definition (story [Story]: ultra preset with frontier-model config #496).

4. Verification

I verified this change by:

  • Running automated tests (against a real Postgres, not just compiled)
  • Checking logs (test output below)

Commands run:

cargo build --workspace --all-targets
export DATABASE_URL="postgres://<local test postgres>"
cargo test -p control-plane --bin control-plane
cargo test -p lci-agent-clients -p agent-runner

Results:

$ cargo build --workspace --all-targets
Finished `dev` profile [unoptimized + debuginfo] target(s) — 0 errors

$ cargo test -p control-plane --bin control-plane
test result: ok. 341 passed; 0 failed; 2 ignored (Neo4j-dependent, unrelated)

  Notably, among the 341:
  - preset::tests::* (6) — resolve_from_config pure-logic coverage (flat preset, per-entry-point
    override precedence, no-config fallback, unrelated-field tolerance)
  - http::webhook::tests::mr_open_resolves_a_custom_preset_from_repo_config — wiremock-backed,
    end-to-end: MR-open webhook → GitLab Repository Files API → JSONC parse → task row's `preset`
    column
  - http::webhook::tests::mr_open_falls_back_to_the_platform_default_preset_when_no_repo_config_exists
    — same path, 404 from the mocked API → falls back to `fast`
  - db::tests::get_task_context_joins_repo_identity — preset/entry_point round-trip through the new
    migration's columns

$ cargo test -p lci-agent-clients -p agent-runner
test result: ok. 69 passed; 0 failed  (agent-runner)
test result: ok. (agent-clients — all green)

5. Screenshots / Evidence

Not applicable — backend config/behavior addition, no UI surface.


6. Risk Assessment

Risk level:

  • Low
  • Medium
  • High

Potential risks:

  • Breaking DB migration: tasks.tiertasks.preset is a rename (no compat column, per this
    repo's hard-cutover convention) — every read/write call site had to be updated in the same change,
    which is exactly the kind of sweep a missed site fails silently on. Mitigated by: the full existing
    control-plane test suite (341 tests, unchanged assertions except field renames) staying green against
    a real database, which would have caught a missed/mismatched column reference.
  • New platform API surface: get_repo_file is new, untested-in-production code on the webhook hot
    path for every PR-open/@mention event. A slow or failing GitHub/GitLab/Bitbucket API call adds
    latency to task creation. Mitigated by: every error path (fetch failure, timeout, 404, oversized,
    malformed) degrades to the platform-default preset rather than blocking or failing task creation —
    proven by the mr_open_falls_back_to_the_platform_default_preset_when_no_repo_config_exists test.
  • A2A scope boundary: documented above (Scope) — flagging again here since it's a real, deliberate
    limitation a reviewer should confirm they agree with, not something to silently accept.

Mitigation:

  • Full workspace build + full control-plane suite (341 tests) verified against a real Postgres
    instance, not just cargo check.
  • Every degrade-to-default path in preset.rs logs a tracing::warn! naming the reason, so a
    misbehaving platform API call is diagnosable from run logs.

7. AI Usage Declaration

AI was used for:

  • Understanding existing code
  • Generating code
  • Refactoring
  • Generating tests
  • Drafting documentation
  • Reviewing the diff
  • Not used

Human verification:

  • I understand every meaningful change in this PR
  • I checked generated code manually
  • I checked generated tests manually
  • I removed unsupported AI assumptions
  • I accept responsibility for this PR

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:

  • Correctness
  • Architecture
  • Security
  • Performance
  • Tests
  • Maintainability
  • Product intent
  • Edge cases

Source of truth: #495
(epic #491),
ADR-0103.

This PR is stacked on #528 (story
#494's repo-config reader, not yet merged) — its base branch is claude/stories-494-495-496-b0cd38,
so this diff will shrink to just story #495's changes once #528 merges and this PR's base auto-updates
to main. The ultra preset (story #496) is a separate follow-on PR against the same epic.

… point (ADR-0103)

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>
@changeset-bot

changeset-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 97213d1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@stephane-segning
stephane-segning merged commit 21663f0 into claude/stories-494-495-496-b0cd38 Jul 28, 2026
5 of 6 checks passed
@stephane-segning
stephane-segning deleted the claude/story-495-entry-point-presets branch July 28, 2026 15:36
stephane-segning added a commit that referenced this pull request Jul 28, 2026
…g reader (ADR-0030) (#528)

* feat(review): implement the .lightbridge-code-review.jsonc repo config 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>

* feat(control-plane): resolve review preset from repo config per entry 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>

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

✅ AI Governance check passed

This PR declares AI usage, references a source of truth, and provides verification evidence. Thank you.

stephane-segning added a commit that referenced this pull request Jul 28, 2026
…name (#530)

finalize_review_outcome() still branched on preset == "fast" to choose
the fast-pass banner vs. deep-tier truncation note. Story #495/PR #529
fixed the equivalent check in control-plane/src/http/internal.rs
(tier == "fast" -> entry_point == "pr_open") but missed this call site
in run.rs, leaving it keyed on the operator-defined preset name
(ADR-0103) rather than the task's actual entry point. A repo whose
pr_open entry point resolves to a custom-named preset would silently
get the wrong (deep-style) framing.

Threads entry_point (already on TaskContext per PR #529) through to
finalize_review_outcome and switches the branch accordingly.

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
stephane-segning added a commit that referenced this pull request Jul 28, 2026
…#531)

* feat(control-plane): resolve review preset from repo config per entry point (ADR-0103)

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>

* feat(review): add ultra as a third platform-default preset (ADR-0103)

Registers `ultra` alongside `fast`/`deep` in PLATFORM_DEFAULT_PRESETS so `preset:
"ultra"` always resolves for any repo, with no built-in model/budget of its own —
operators back it with a frontier model via ai-helm-values, same mechanism
fast/deep already use. Also refreshes docs/review-pipeline.md, which had drifted
significantly behind the preset/entry_point architecture landed in #527-#529.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
stephane-segning added a commit that referenced this pull request Jul 28, 2026
…-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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant