Skip to content

feat(config): add separate base URLs for summary and embedding models#99

Closed
larroy wants to merge 1 commit into
openclaw:mainfrom
larroy:improv_base_urls
Closed

feat(config): add separate base URLs for summary and embedding models#99
larroy wants to merge 1 commit into
openclaw:mainfrom
larroy:improv_base_urls

Conversation

@larroy

@larroy larroy commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Introduce per-purpose OpenAI endpoint overrides so summaries and embeddings can target different endpoints instead of sharing one:

  • summary_base_url (--summary-base-url, GITCRAWL_SUMMARY_BASE_URL)
  • embed_base_url (--embed-base-url, GITCRAWL_EMBED_BASE_URL)

Each resolves to its per-purpose value first, then falls back to the shared GITCRAWL_OPENAI_BASE_URL / OPENAI_BASE_URL, so existing single-endpoint setups are unaffected. The embedding client now uses the resolved embed endpoint at both call sites, and configure/doctor output surfaces both fields.

Document the fallback chain and resolution order in docs/configuration.md and docs/reference.md, and add config/CLI tests covering the config value, env override, and shared fallback.

@clawsweeper

clawsweeper Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 14, 2026, 10:30 AM ET / 14:30 UTC.

Summary
Adds separate summary and embedding base-URL config fields, environment overrides, configure flags, diagnostics, embedding-client routing, documentation, and tests while retaining the shared endpoint fallback.

Reproducibility: not applicable. This PR proposes new endpoint-selection capability rather than fixing a defined current-main bug.

Review metrics: 1 noteworthy metric.

  • New config surface: 2 fields, 2 environment variables, 2 CLI flags. These six new entry points become user-facing configuration contracts that need an intentional long-term support decision.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦐 gold shrimp
Patch quality: 🐚 platinum hermit
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Provide redacted output or server logs showing a nonzero embedding request reaching embed_base_url.
  • Resolve the dirty merge state against current main and refresh validation.
  • Obtain maintainer direction on retaining or deferring summary_base_url.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: Terminal output proves config persistence and diagnostics, but the zero-selection embed run proves no outbound routing; add a redacted nonzero request transcript or endpoint log, update the PR body, and request @clawsweeper re-review if review does not rerun automatically.

Risk before merge

  • [P1] The terminal proof does not demonstrate that a nonzero embedding request actually used embed_base_url; a resolver or call-site wiring error could remain hidden.
  • [P1] summary_base_url would become a documented persisted contract even though current main has no summary command consuming it, potentially creating speculative compatibility obligations.
  • [P1] The branch is currently not cleanly mergeable, so conflict resolution against current main could change the reviewed implementation.

Maintainer options:

  1. Decide the mitigation before merge
    Ship an embedding-specific endpoint override with nonzero live routing proof, and add summary-specific endpoint configuration only when a summary command consumes it or maintainers explicitly sponsor the future contract.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] A maintainer must choose the permanent summary-config boundary; afterward the contributor must resolve conflicts and provide live endpoint-routing proof.

Maintainer decision needed

  • Question: Should gitcrawl establish both purpose-specific endpoint contracts now even though the summary endpoint has no current runtime consumer?
  • Rationale: Code review cannot determine whether maintainers want to support a speculative summary configuration contract or prefer configuration to follow implemented functionality.
  • Likely owner: Vincent Koc — Recent substantial ownership of the central CLI file makes them the strongest available person to decide the configuration boundary.
  • Options:
    • Narrow to embeddings (recommended): Land embed_base_url with live request proof and defer summary_base_url until summary functionality exists.
    • Sponsor both endpoints: Accept both fields now as an intentional forward-compatible contract and require endpoint-routing proof for embeddings before merge.
    • Defer the feature: Keep the shared endpoint as the only supported path until both model purposes have active runtime consumers.

Security
Cleared: The diff adds no dependency, workflow, permission, secret-handling, or new execution-source changes, and purpose-specific URLs remain within the existing user-controlled OpenAI endpoint trust boundary.

Review details

Best possible solution:

Ship an embedding-specific endpoint override with nonzero live routing proof, and add summary-specific endpoint configuration only when a summary command consumes it or maintainers explicitly sponsor the future contract.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this PR proposes new endpoint-selection capability rather than fixing a defined current-main bug.

Is this the best way to solve the issue?

Unclear: separate endpoint resolution is a maintainable embedding solution, but adding an unused summary endpoint contract before its consumer exists is not clearly the narrowest product path.

AGENTS.md: unclear because the file could not be read completely.

Codex review notes: model internal, reasoning high; reviewed against 51f279896340.

Label changes

Label justifications:

  • P3: This is a low-urgency additive configuration feature with no demonstrated regression in existing single-endpoint setups.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Terminal output proves config persistence and diagnostics, but the zero-selection embed run proves no outbound routing; add a redacted nonzero request transcript or endpoint log, update the PR body, and request @clawsweeper re-review if review does not rerun automatically.
Evidence reviewed

What I checked:

  • Current product boundary: Current-main documentation describes summary_model as reserved for future summary commands and exposes only the shared GITCRAWL_OPENAI_BASE_URL / OPENAI_BASE_URL endpoint override, so summary_base_url would establish a contract before a summary consumer exists. (docs/configuration.md:109, 51f279896340)
  • Additive config contract: The branch adds SummaryBaseURL and EmbedBaseURL to persisted OpenAI configuration and applies purpose-specific environment overrides at runtime. (internal/config/config.go:38, 6df7bec0660b)
  • Embedding routing implementation: The branch resolves the purpose-specific embedding endpoint with fallback to the existing shared endpoint and passes that result into both embedding client construction paths. (internal/cli/app.go, 6df7bec0660b)
  • Partial real behavior proof: The contributor's terminal transcript proves configure persistence and doctor resolution, but the embed run selected zero records and therefore made no observable request to the configured embedding endpoint. (6df7bec0660b)
  • Current merge state: GitHub reports the current head as dirty and not mergeable against the updated default branch, so the exact merged result still requires conflict resolution and refreshed review. (6df7bec0660b)
  • CLI area history: Vincent Koc recently made a substantial current-main change to internal/cli/app.go in commit e13976f, making them a strong routing candidate for the CLI/config direction. (internal/cli/app.go, e13976fbea7a)

Likely related people:

  • Vincent Koc: Recently made a substantial current-main change to internal/cli/app.go and is the strongest available history signal for the CLI/config surface affected here. (role: recent CLI area contributor; confidence: high; commits: e13976fbea7a; files: internal/cli/app.go)
  • Peter Steinberger: Broad repository and release history makes them a plausible secondary decision owner, though the available evidence is less specific to these OpenAI config helpers. (role: adjacent repository owner; confidence: low; files: docs/configuration.md, internal/config/config.go)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (2 earlier review cycles)
  • reviewed 2026-07-09T19:26:32.522Z sha 89f5189 :: needs real behavior proof before merge. :: [P3] Remove the release-owned changelog entries
  • reviewed 2026-07-10T07:09:45.483Z sha 6df7bec :: needs real behavior proof before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. labels Jul 9, 2026
@plarroy-nv

Copy link
Copy Markdown

Real behavior proof:

plarroy@ip-172-31-63-139 ~/gitcrawl (improv_colors)> gitcrawl configure \
                                                             --summary-base-url https://summary.example.test/v1 \
                                                             --embed-base-url https://embed.example.test/v1
configure
{
  "config_path": "/home/plarroy/.config/gitcrawl/config.toml",
  "embed_base_url": "https://embed.example.test/v1",
  "embed_model": "text-embedding-3-small",
  "embedding_basis": "title_original",
  "summary_base_url": "https://summary.example.test/v1",
  "summary_model": "gpt-5.4",
  "updated": true
}
plarroy@ip-172-31-63-139 ~/gitcrawl (improv_colors)> gitcrawl doctor
doctor
{
  "api_supported": false,
  "cluster_count": 4040,
  "config_exists": true,
  "config_path": "/home/plarroy/.config/gitcrawl/config.toml",
  "db_path": "/home/plarroy/.local/share/gitcrawl/gitcrawl.db",
  "db_schema": {
    "path": "/home/plarroy/.local/share/gitcrawl/gitcrawl.db",
    "exists": true,
    "current_version": 4,
    "supported_version": 4,
    "state": "current",
    "current": true,
    "pending_migration": false,
    "legacy": false,
    "newer": false,
    "pending_migrations": [],
    "pr_details": {
      "details_table": true,
      "files_table": true,
      "files_position_key": true,
      "duplicate_path_files_supported": true,
      "state": "supported"
    }
  },
  "embed_base_url": "https://embed.example.test/v1",
  "embed_model": "text-embedding-3-small",
  "embedding_basis": "title_original",
  "executable_path": "/home/plarroy/.local/bin/gitcrawl",
  "github_token_present": false,
  "github_token_source": "",
  "last_sync_at": "2026-07-09T16:19:26.2657342Z",
  "open_thread_count": 6252,
  "openai_key_present": false,
  "openai_key_source": "",
  "portable_refresh": {},
  "portable_store_status": {},
  "repair_action": "",
  "repository_count": 1,
  "runtime": {
    "build_modified": true,
    "build_revision": "89f518984ae53c5bb11bb4f41f1cc1a0ba8b66e9",
    "build_time": "2026-07-09T19:18:18Z",
    "executable_path": "/home/plarroy/.local/bin/gitcrawl",
    "go_version": "go1.26.4",
    "module_path": "github.com/openclaw/gitcrawl",
    "module_version": "v0.7.1-0.20260709191818-89f518984ae5+dirty",
    "version": "dev"
  },
  "runtime_db_health": {},
  "source_db_health": {
    "exists": true,
    "health": "ok",
    "manifest": "missing",
    "manifest_path": "/home/plarroy/.local/share/gitcrawl/gitcrawl.db.manifest.json",
    "path": "/home/plarroy/.local/share/gitcrawl/gitcrawl.db",
    "sha256": "b7257a1db4eca1a31e757ba672cf6f5d6e971b977c72e50c8bfdef4ce4452129",
    "size": 352727040
  },
  "source_db_schema": {
    "path": "/home/plarroy/.local/share/gitcrawl/gitcrawl.db",
    "exists": true,
    "current_version": 4,
    "supported_version": 4,
    "state": "current",
    "current": true,
    "pending_migration": false,
    "legacy": false,
    "newer": false,
    "pending_migrations": [],
    "pr_details": {
      "details_table": true,
      "files_table": true,
      "files_position_key": true,
      "duplicate_path_files_supported": true,
      "state": "supported"
    }
  },
  "summary_base_url": "https://summary.example.test/v1",
  "summary_model": "gpt-5.4",
  "thread_count": 6252,
  "version": "dev"
}
plarroy@ip-172-31-63-139 ~/gitcrawl (improv_colors)>
plarroy@ip-172-31-63-139 ~/gitcrawl (improv_colors)> gitcrawl embed openclaw/openclaw
embed
{
  "repository": "openclaw/openclaw",
  "model": "azure/openai/text-embedding-3-small",
  "basis": "title_original",
  "selected": 0,
  "embedded": 0,
  "skipped": 0,
  "status": "success",
  "run_id": 6
}

Introduce per-purpose OpenAI endpoint overrides so summaries and
embeddings can target different endpoints instead of sharing one:

- summary_base_url  (--summary-base-url, GITCRAWL_SUMMARY_BASE_URL)
- embed_base_url    (--embed-base-url,   GITCRAWL_EMBED_BASE_URL)

Each resolves to its per-purpose value first, then falls back to the
shared GITCRAWL_OPENAI_BASE_URL / OPENAI_BASE_URL, so existing
single-endpoint setups are unaffected. The embedding client now uses
the resolved embed endpoint at both call sites, and configure/doctor
output surfaces both fields.

Document the fallback chain and resolution order in
docs/configuration.md and docs/reference.md, and add config/CLI tests
covering the config value, env override, and shared fallback.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@larroy
larroy force-pushed the improv_base_urls branch from 89f5189 to 6df7bec Compare July 9, 2026 22:43
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 10, 2026
@clawsweeper

clawsweeper Bot commented Jul 17, 2026

Copy link
Copy Markdown

ClawSweeper status: review started.

I am starting a fresh review of this pull request: feat(config): add separate base URLs for summary and embedding models This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

@steipete

Copy link
Copy Markdown
Contributor

Thanks @larroy for the original implementation and configuration work. Peter asked us to narrow the permanent surface to the embedding-only endpoint and supply nonzero live routing proof. That repaired version landed in #117, with your contribution credited in the changelog and commit. Closing this pull request as superseded by the landed replacement.

@steipete steipete closed this Jul 17, 2026
@steipete steipete self-assigned this Jul 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants