Changing azureUseDeprecatedCompletionsAPIForOldModels to default to
trueso that customers who upgrade don't have to change siteadmin
config
<img width="323" alt="image"
src="https://github.com/user-attachments/assets/a690b54b-aa43-43c3-9f7d-202f161866bc">
## Test plan
Tested locally
## Changelog
<br> Backport 3b16059588 from #64347
Co-authored-by: Ara <arafat.da.khan@gmail.com>
This makes some requested changes to the welcome banner.
Notably:
- We no longer show the dialog unless the user switches into the svelte
version from react
- We don't show the dialog on every switch
- The dialog is accessible in the React via the "Learn more" button in
the menu
- Copy is updated to be less verbose
- The request for feedback is only shown once when switching out of the
svelte webapp
Fixes srch-851
A consequence of https://github.com/sourcegraph/sourcegraph/pull/64272
is that redirecting to the cody marketing page on dotcom didn't work.
That's because `/cody` is not provided by the server as a known page.
A simple fix would be to mark the link as external, but we'd have to
keep in mind to do this in all places (present and future).
A more "central" fix is to add this page to a hardcoded list of known
pages that are not provided by the server.
## Test plan
Manual testing
Currently the link element doesn't extend to the full width of the
sidebar. You can click outside of the actual text to navigate to the
file, but it won't be preloaded.
Now the link occupies the full width.
## Test plan
Manual testing.
Closes srch-838
This commit extends the search progress popover and adds the search jobs
section.
In the React version the UI for the popover differs slightly when search
jobs are enabled vs not enabled.
I decided to ignore the differences and only impolemented the style used
for the 'search jobs enabled' version, which makes the popover a bit
more compact.
The logic for validating and creating a search job is encapsulated in a
class which makes it easy to conditionally show the search jobs UI.
Additional changes:
- Skipped items is now a list and uses `<details>` elements which is
semantically more correct. We loose the styling of the chevron but I
think that's OK.
- LoadingSpinner was updated to work inline.
- Logging was fixed (?) in the React version to send the correct
meta-data.
- Added integration tests for the search job popover behavior
## Test plan
Integration tests and manual testing.
Under high contention, the updating of execution logs query:
```
UPDATE
lsif_indexes
SET
execution_logs = execution_logs || $1::json
WHERE
id = $2
AND worker_hostname = $3
AND state = $4 RETURNING ARRAY_LENGTH(execution_logs, $5)
```
Was taking multiple seconds due to lock contention on the
lsif_indexes_state index.
Running `EXPLAIN ANALYZE` on Sourcegraph.com under lower contention uses
the primary key index on id, so we don't have an easy way to test the
high contention scenario. Try this alternate query form to see if that fixes the issue.
Closes SRCH-802
After a long and unsuccessful hunt of eventual consistency errors, I
noticed that we don't need the GraphQL fragments that are causing
problems.
I introduced a simplified request, which succeeds reliably. What
customers should see now is one error (see second picture below), which
is followed by a retry, and then success.
Previously we've seen this error, sometimes less often, sometimes more,
but customers saw it so often that it caused their batch changes to
fail.
<img width="1042" alt="Screenshot 2024-08-06 at 14 02 00"
src="https://github.com/user-attachments/assets/c5a099da-d474-43db-ac12-ac7c4f22d4d3">
Customers may still see an initial error, which I've seen to reliably
disappear on the first retry. It should not be a problem
<img width="1039" alt="Screenshot 2024-08-06 at 14 02 48"
src="https://github.com/user-attachments/assets/99f278d4-4020-48ea-b4ac-32cbdfce455d">
## Test plan
Manual testing
## Changelog
- fix(batches): improve GitHub Apps integration reliability by
simplifying the data requested from GitHub
Consolidates the two test helper/util modules, and moves one test file
to align with the file it's actually testing.
## Test plan
Tests continue to pass
Previously, the QueuedCount method was confusing because:
1. By default, it actually returned the count for both the 'queued' and
'errored' states (despite the name just have 'Queued').
2. There was an additional boolean flag for also returning entries in
the 'processing' state, but reduced clarity at call-sites.
So I've changed the method to take a bitset instead, mirroring the
just-added Exists API, and renamed the method to a more
generic 'CountByState'.
While this does make call-sites a bit more verbose, I think the
clarity win makes the change an overall positive one.
This change should not change any behaviour, but it prepares for syntactic/search-based pagination.
Both syntactic and search based usages now return a "NextCursor", which is `Some` if there are more results of their provenance to be had. For now they always return `None` which preserves the current behaviour. The logic in `root_resolver` implements a state machine that transitions the cursor through the provenances and makes sure to skip provenances if the cursor in the request starts a certain state.
## Test plan
Tests continue to pass, no behavioral changes
Closes DINF-89
Gazelle sometimes have trouble processing glob expressions, and this
isn't reported as a failure even though it ultimately results in the
`BUILD.bazel` not being correctly updated.
## Test plan
* Manual testing
In `client/web/BUILD.bazel`, add a new `src` to the `web_lib` ts_project
target that includes a glob pattern.
```
...
ts_project(
name = "web_lib",
srcs = glob(["!src/playwright/*.spec.ts"]) + [
"src/Index.tsx",
"src/LegacyLayout.tsx",
"src/LegacyRouteContext.tsx",
"src/LegacySourcegraphWebApp.tsx",
"src/PageError.tsx",
"src/SearchQueryStateObserver.tsx",
"src/SourcegraphWebApp.tsx",
...
```
When you run `go run ./dev/sg bazel configure`, the command should fail
with an error message instead of returning exit 0.
## Changelog
<!-- OPTIONAL; info at
https://www.notion.so/sourcegraph/Writing-a-changelog-entry-dd997f411d524caabf0d8d38a24a878c
-->
---------
Co-authored-by: Jean-Hadrien Chabran <jean-hadrien.chabran@sourcegraph.com>
I noticed that non-svelte repo subpages haven't been redirected to the
React app in local development (it works as expected in production).
That's because the proxy doesn't know about them since they are not part
of `knownRoutes`.
We could update the server code to include those routes as well but that
seems heavey handed just to make local development work.
I think in this case it's fine to have manual entries for the handful of
repo subpages that have not been migrated to Svelte yet.
## Test plan
Manual testing.
::sigh:: the problem here is super in the weeds, but ultimately this
fixes a problem introduced when using AWS Bedrock and Sourcegraph
instances using the older style "completions" config.
## The problem
AWS Bedrock has some LLM model names that contain a colon, e.g.
`anthropic.claude-3-opus-20240229-v1:0`. Cody clients connecting to
Sourcegraph instances using the older style "completions" config will
obtain the available LLM models by using GraphGL.
So the Cody client would see that the chat model is
`anthropic.claude-3-opus-20240229-v1:0`.
However, under the hood, the Sourcegraph instance will convert the site
config into the newer `modelconfig` format. And during that conversion,
we use a _different value_ for the **model ID** than what is in the site
config. (The **model name** is what is sent to the LLM API, and is
unmodified. The model ID is a stable, unique identifier but is sanitized
so that it adheres to naming rules.)
Because of this, we have a problem.
When the Cody client makes a request to the HTTP completions API with
the model name of `anthropic.claude-3-opus-20240229-v1:0` or
`anthropic/anthropic.claude-3-opus-20240229-v1:0` it fails. Because
there is no model with ID `...v1:0`. (We only have the sanitized
version, `...v1_0`.)
## The fix
There were a few ways we could fix this, but this goes with just having
the GraphQL component return the model ID instead of the model name. So
that when the Cody client passes that model ID to the completions API,
everything works as it should.
And, practically speaking, for 99.9% of cases, the model name and model
ID will be identical. We only strip out non-URL safe characters and
colons, which usually aren't used in model names.
## Potential bugs
With this fix however, there is a specific combination of { client,
server, and model name } where things could in theory break.
Specifically:
Client | Server | Modelname | Works |
--- | --- | --- | --- |
unaware-of-modelconfig | not-using-modelconfig | standard | 🟢 [1] |
aware-of-modelconfig | not-using-modelconfig | standard | 🟢 [1] |
unaware-of-modelconfig | using-modelconfig | standard | 🟢 [1] |
aware-of-modelconfig | using-modelconfig | standard | 🟢 [3] |
unaware-of-modelconfig | not-using-modelconfig | non-standard | 🔴 [2] |
aware-of-modelconfig | not-using-modelconfig | non-standard | 🔴 [2] |
unaware-of-modelconfig | using-modelconfig | non-standard | 🔴 [2] |
aware-of-modelconfig | using-modelconfig | non-standard | 🟢 [3] |
1. If the model name is something that doesn't require sanitization,
there is no problem. The model ID will be the same as the model name,
and things will work like they do today.
2. If the model name gets sanitized, then IFF the Cody client were to
make a decision based on that exact model name, it wouldn't work.
Because it would receive the sanitized name, and not the real one. As
long as the Cody client is only passing that model name onto the
Sourcegraph backend which will recognize the sanitized model name / ID,
all is well.
3. If the client and server are new, and using model config, then this
shouldn't be a problem because the client would use a different API to
fetch the Sourcegraph instance's supported models. And within the
client, natively refer to the model ID instead of the model name.
Fixes
[PRIME-464](https://linear.app/sourcegraph/issue/PRIME-464/aws-bedrock-x-completions-config-does-not-work-if-model-name-has-a).
## Test plan
Added some unit tests.
## Changelog
NA
This PR sets the defaults for "Sourcegraph supplied LLM models".
## When will these "defaults" be used?
These models will _only_ be used IFF the Sourcegraph instance is
_explicitly_ using the newer "modelConfiguration" site configuration
data. (And opts into using Sourcegraph-supplied LLM models.)
If the Sourcegraph instance is using the older "completions"
configuration blob, then _only_ the user-supplied models will be used.
(Or, based on the specific defaults defined in the code for the
completions provider.)
## What about Cody Free or Cody Pro?
😬 yeah, we're going to need to deal with that later. Currently
Sourcegraph.com is _not_ using the newer "modelConfiguration" site
configuration, and instead we have some hacks in the code to ignore the
internal modelconfig. See this "super-shady hack":
e5178a6bc0/cmd/frontend/internal/httpapi/completions/get_model.go (L425-L455)
So we are just erring on the side of having Cody Free / Cody Pro "do
whatever they do now", and this PR won't have any impact on that.
We _do_ want Sourcegraph.com to only return this data, but there are a
few things we need to get straightened out first. (e.g. Cody Gateway
being ware of mrefs, and having Cody Clients no longer using `dotcom.ts`
to hard-code Cody Pro LLM models.)
## What does this PR actually _do_?
1. It updates the code in `cmd/cody-gateway-config` so that it will
produce a new "supported-models.json" file.
2. I then ran the tool manually, the output of which was then written to
`internal/modelconfig/embedded/models.json`.
3. That's it.
For any Sourcegraph releases after this PR gets merged, the "Sourcegraph
supplied LLM models" will be the newer set defined in `models.json`.
(i.e. having these new defaults, and including
"fireworks::v1::starcoder".)
## Test plan
~~I tested things locally, and unfortunately it doesn't look like any
clients are filtering based on the model capabilities. So "StarCoder" is
showing up in the Cody Web UI, despite failing at runtime.~~
Update: This was a problem on my end. This isn't an issue.
## Changelog
NA?
This closes a pretty big gap in the query validation for Search Jobs. We
don't support repo search yet and searcher returned errors during
execution, complaining about missing patterns. With this PR we fail
during validation so users cannot even create these kinds of jobs. See
new test cases.
## Test plan
Updated unit tests
Running `COUNT(*)` on hot tables like lsif_indexes on Sourcegraph.com
shows up on profiles when the number of executors is bumped to
30+, and when the table has 100K+ jobs. So avoid `COUNT(*)`
where possible.
Reduce the exposed surface area with a smaller interface
minimalRepoStore, and make sure we're using fakes in the
tests which better document the intent, instead of using a
mock repo store. The fake makes sure that the methods
are mutually consistent, which is easier to do when working
with a smaller interface.
Fixes srch-765
When navigating to `/cody/dashboard` and the new web app is enabled, the
user gets a "Not found" error. This doesn't happen in the React app.
The page doesn't exist in the new web app, hence the server should fall
back to the old web app but that doesn't happen.
Why? Because the server doesn't know about `/cody/dashboard` either. It
only exists in the React app.
Instead the server interprets this path as a (non-existing) repository
page. When the new web app is enabled, repository pages are handled by
the new web app, but since neither the repo nor the page exist in the
new web app, the aforementioned error is thrown.
Configuring this route on the server makes it so that the React app is
served instead.
## Test plan
Manual testing. Going to https://sourcegraph.test:3443/cody/dashboard
loads the React app.
Follow up to #64207. In our old search semantics, quotes were
interpreted literally. So a query like `"sourcegraph"` would match only
strings like `fmt.Println("sourcegraph")`. Now, both single and double
quotes are used for escaping, and mean that the contents should be
searched exactly.
This PR makes sure to boost matches on quoted terms in result ranking.
This way, users familiar with the old syntax are more likely to find
what they're after.
## Test plan
Adapted unit tests. Re-tested all queries from #64207 manually, plus
these ones:
* `'sourcegraph'`
* `"sourcegraph"`
A long time ago, we dropped the requirement for frontend to have any
disk for caching. Our helm deployments use read-only rootFSes, so this
wouldn't even work.
This PR aims to make that clearer by removing some last remnants of
those times.
Test plan: Frontend starts locally and integration tests pass in CI.
In #64215, we removed support for smart search. We continued to allow
the `smart` search mode, but just execute with `precise` behavior.
However, we started to throw an error for `patterntype:lucky` (which is
the old way of specifying smart search). It turns out that `lucky` made
its way into some search links and settings, causing those searches to
fail.
This PR restores support for `lucky`, and remaps it to `standard`. This
preserves the semantics as much as possible (since smart search attempts
a 'standard' search as its first rule).
Closes SRCH-840
tl;dr: Everything is derived from `window.context.svelteKit.knownRoutes`
*buckle up*
### Context
After the new web app was enabled by default on dotcom I looked at
dotcom data in Sentry to see if there was anything out of the ordinary.
I noticed a high number of errors related to resolving repository
information. The most common non-existing repository that was reported
was `-/sign-out`.
Of course this shouldn't happen. `/-/sign-out` is the sign out URL and
shouldn't be interpreted as a repository.
Currently we prevent SvelteKit from interpreting specific paths as
repositories by using the `reporev` parameter validator:
```ts
// src/params/reporev.ts
const topLevelPaths = [
'insights',
'search-jobs',
'saved-searches',
// ... many more here
]
const topLevelPathRegex = new RegExp(`^(${topLevelPaths.join('|')})($|/)`)
// This ensures that we never consider paths containing /-/ and pointing
// to non-existing pages as repo name
export const match: ParamMatcher = param => {
// Note: param doesn't have a leading slash
return !topLevelPathRegex.test(param) && !param.includes('/-/')
}
```
`-/sign-out` was not in that list, including others which have been
added since this file was originally created.
Adding it would have been the simple solution but having to maintain
this list manually has always irked me. Now we have a better way...
### Production changes
#### Client side
It turns out we are already sending a list of known routes to the client
in `window.context.svelteKit.knownRoutes` to basically do the same
thing: test whether a given path is a page or a repository.
```ts
// src/lib/navigation.ts
let knownRoutesRegex: RegExp | undefined
function getKnownRoutesRegex(): RegExp {
if (!knownRoutesRegex) {
knownRoutesRegex = new RegExp(`(${window.context?.svelteKit?.knownRoutes?.join(')|(')})`)
}
return knownRoutesRegex
}
// ...
export function isRouteEnabled(pathname: string): boolean {
let foundRoute: SvelteKitRoute | undefined
// [...]
if (foundRoute) {
if (foundRoute.isRepoRoot) {
// Check known routes to see if there is a more specific route than the repo root.
// If yes then we should load the React app (if the more specific route was enabled
// it would have been found above).
return !getKnownRoutesRegex().test(pathname)
}
return true
}
return false
}
```
Why do we have the `reporev` validator and `isRouteEnabled`? They
basically do the same thing (check whether a path is a known page or a
repository) the first (`reporev`) is used by SvelteKit to determine
which route a path corresponds to (i.e. to navigate to the repository
page or whether the page doesn't exist) and the second one
(`isRouteEnabled`) is used after the route resolution but before route
loading. It's used to trigger a full page refresh to fetch the React
version if necessary.
Anyways, since we already have a list of known pages from the server,
the parameter validator should just use it too. Except it didn't work,
which made the following server side changes necessary.
#### Server
We register routes in multiple places. Right now `knownRoutes` is
populated from the router created in
`cmd/frontend/internal/app/ui/router.go`, but this does not include
`/-/sign-out`. This route (and others) are defined in
`cmd/frontend/internal/app/router/router.go` (I don't know what the
difference is). I extended the sveltekit route registration code so
that's possible to register routes from multiple routers.
I couldn't test it yet however because I currently can't get Sourcegraph
to run locally (Camden tested it; it works).
### Development mode changes
After the above changes, navigating to a React only page in development,
e.g. `/insights` would show a 'repo not found error' because
`window.context.svelteKit.knownRoutes` was empty in development. So
every non-existing page would be interpreted as missing repository.
Hardcoding a list of known pages *again* just for development seemed to
be a step backwards. So I spent quite a bit of time to find a way to
extract the JS context object from the HTML page returned by the origin
server and inject it into the HTML page generated by SvelteKit, similar
to how we do it for the React app.
Additionally the value of `window.context.svelteKit.knownRoutes` is now
used to setup the proxy to non-supported pages from the server, so we
don't have to maintain this regex anymore either:
```ts
// Proxy requests to specific endpoints to a real Sourcegraph
// instance.
'^(/sign-(in|out)|/.assets|/-|/.api|/.auth|/search/stream|/users|/notebooks|/insights|/batch-changes|/contexts)|/-/(raw|own|code-graph|batch-changes|settings)(/|$)': { ... }
```
This means that requesting non-svelte pages will also return the
corresponding React version in development.
## Test plan
Manual testing.
Relates to #64186
With this PR we only show `83 out of 120 tasks` if the search job is
currently processing. In all other states, we don't show this stat. This
is a consequence of the janitor job I recently added, because after
aggregation, this data is not available anymore. User's can still
inspect the logs and download results to get a detailed view of which
revisions were searched.
I also remove an unnecessary dependency of the download links on the job
state.
## Test plan:
I ran a search job locally and confirmed that the progress message is
only visible while the job is processing and that logs and downloads are
always available.
## Changelog
- Show detailed progress only while job is in status "processing"
- Remove dependency of download links on job state
Instead of re-generating and re-compiling the mocks 3 times,
consolidate them into a single storemocks package.
This should also provide better code navigation in the editor.
Previously, for metrics reporting, we were querying the number of
queued+errored records every 5 seconds. For large queues +
heavy contention, this scan itself takes a few seconds, despite using
an index. Reduce the frequency of this scan as we
don't need updates to the queue size every 5 seconds.
## Test plan
Check that frequency of `COUNT(*)` is reduced on Sourcegraph.com once
this patch makes its way to Sourcegraph.com
The current logging is outdated. This PR is not a complete rewrite. It
tries to keep semantics mostly as-is but it is more generous when it
comes to what we log. (For example we didn't log `(AND foo bar)` before)
IMO this is a good compromise we can ship quickly and then take some
time to figure out how to distinguish suggestions, searches, code-intel
and whether it is a good idea to mix search types and result types.
Updates:
- Prefer to log result types, fall back to pattern type. (This is
largely the same logic as before but we don't bail for complex queries)
- Don't guess repo and file results types if not explicitly specified.
We loose a bit of information here but it keeps the code much cleaner.
- Allow "AND" and "OR" operators.
The updated test case gives a good overview of how we would log things
in the future.
Kept as-is:
- I left "new" and "legacy" logging in place. I am not sure how we use
the target stores right now so I wanted to keep this as-is for now.
However we should see if we can retire the legacy logging.
- The previous and current logic translates `type:file` queries to
pattern types. I guess this makes sense from the perspective of how
things are implemented in the backend.
- "type:path" is logged as "file".
## Test plan:
Updated unit test
This PR fixes Cody autocomplete in some situations, fixing
[PRIME-426](https://linear.app/sourcegraph/issue/PRIME-426/autocomplete-completely-broken-in-main-with-and-sometimes-without-the).
Our story begins with this PR:
https://github.com/sourcegraph/sourcegraph/pull/63886. The PR updated
the `CodyLLMConfigurationResolver.Provider` GraphQL endpoint to no
longer return the "provider" that was put into the site configuration,
but to instead return the _display name_ of the provider, or if multiple
were configured the string "various".
```diff
- func (c *codyLLMConfigurationResolver) Provider() string {
- return string(c.config.Provider)
- }
+ func (c *codyLLMConfigurationResolver) Provider() string {
+ if len(r.modelconfig.Providers) != 1 {
+ return "various"
+ }
+ return r.modelconfig.Providers[0].DisplayName
+}
```
This change was wrong on several levels, and unfortunately stemmed from
the original author (me) not tracking down and understanding how the
GraphQL endpoint was actually used.
Once we discovered the problem, we quickly rectified this by changing
the behavior to the more correct version of returning the Provider ID of
the code completion model with
https://github.com/sourcegraph/sourcegraph/pull/64165:
```go
func (r *codyLLMConfigurationResolver) Provider() string {
return string(r.modelconfig.DefaultModels.CodeCompletion.ProviderID())
}
```
However, after some more testing we discovered yet another edge case. We
didn't realize that the provider "sourcegraph" (which is how you
configure Sourcegraph to use Cody Gateway, using the older site config
method) was required in some scenarios on the Cody client.
So the new logic, of returning the Provider ID was incorrect. (Because
we report the _model_ provider, e.g. "anthropic". Not the _API_
provider, e.g. "sourcegraph"/"Cody Gateway".)
With this change, we should just have the behavior we had initially:
returning whatever the admin had configured in the
"completions.provider" section of the site config. If only the newer
modelconfig settings were used, we return "sourcegraph" if using Cody
Gatway. And if not, just return the Provider ID. (Though in some
situations even that will lead to incorrect results, because ultimately
we need to update the client here.)
## Test plan
I wasn't able to test this super-well manually, and am relying on
Stephen and Taras' knowledge of how the client uses this today.
## Changelog
NA
