Repository navigation
fix(chat): key chat history on a stable workspace identifier - #584
laileni-aws wants to merge 2 commits into
Conversation
Send the Eclipse workspace root as awsClientCapabilities.q.workspaceFilePath in the LSP initialize request. The language server uses it to name the chat history database; without it the server falls back to hashing the set of open project folders, which changes whenever a project is opened, closed, imported or deleted. On the next restart a different history file is loaded, so previously open chat tabs are not restored (or stale tabs from an old file are restored instead). The server migrates the existing folder-based history file to the new identifier the first time it sees workspaceFilePath, so existing chat history is preserved.
The approach looks right. I checked the server against language-servers @7c9f732. The key path you set ( Before merge
Should fix
Nits
Server follow-ups (not this PR)
Side benefit worth a changelog line: two Eclipse workspaces with the same projects no longer share one history file. |
Review feedback:
- Canonicalize the workspace root path. A symlinked -data directory, a
redundant path segment, or a different drive-letter case previously
produced a different identifier, which defeats the point of a stable
id. Falls back to the literal path if canonicalization fails. The
blank check runs first, because new File("").getCanonicalPath()
resolves to the process working directory.
- Add AmazonQLspServerBuilderTest, driving an initialize request through
wrapMessageConsumer and asserting aws.awsClientCapabilities.q.
workspaceFilePath is present with the workspace root and absent when
the root is unknown. That key is a plain string in a nested map, so a
typo would otherwise compile and pass CI silently.
- Log the caught exception instead of only its message.
- Rename getWorkspaceFilePath to getWorkspaceRootPath, which describes
what the method returns rather than the protocol field it feeds.
- Drop the redundant isNotBlank check in the builder; the util already
returns null rather than a blank string, so a null check is enough.
|
Thanks — I verified each of the factual points against Should fix — done
Nits — done
Before merge — partly open
The end-to-end restart check on Linux and Windows is still outstanding — I don't have an Eclipse runtime available, so I could not watch for On the draft point: the body's stale "Draft" line is gone. The PR itself is not marked draft, and I've left that as-is rather than flipping its state after you'd already reviewed it. Server follow-ups — agreed and left out of this PR. The unguarded Added the side benefit you mentioned to the PR body: two Eclipse workspaces with the same projects open no longer share one history file. This repo has no |
ashishrp-aws
left a comment
There was a problem hiding this comment.
🤖 AI-assisted review. Requesting changes per the blocking items in #584 (comment) — happy to re-review once they're addressed.
Problem
After an Eclipse restart (or plugin update), previously open Amazon Q chat tabs are intermittently not restored. Sometimes an old, already-closed tab is restored instead. Plugin logs are clean when this happens. Reported in #565 (split out of #559).
Root cause
The language server stores chat history in
~/.aws/amazonq/history/chat-history-<workspaceId>.json. It derivesworkspaceIdfromawsClientCapabilities.q.workspaceFilePathwhen the client provides one, and otherwise falls back to hashing the workspace folders sent in the LSPinitializerequest:|.no-workspace, i.e. one shared history file.The Eclipse plugin does not send
workspaceFilePath, so the fallback is used. LSP4E populatesworkspaceFoldersfrom the set of open projects, so the history file identity changes whenever a project is opened, closed, imported or deleted between sessions. On the next start the server opens a different history file: either a brand-new one (no tabs restored) or an older one whoseisOpenflags are stale (an old closed tab is restored). This matches the reporter's manual workaround of renaming the history file.Fix
Send the canonicalized Eclipse workspace root path (
ResourcesPlugin.getWorkspace().getRoot().getLocation()) asawsClientCapabilities.q.workspaceFilePath. This is stable for the lifetime of a workspace regardless of which projects are open, and is the same mechanism the other IDE plugins use.The path is canonicalized so a symlinked
-datadirectory, a redundant path segment, or a different drive-letter case all resolve to one identifier, with a fallback to the literal path if canonicalization fails.Migration behaviour, precisely
When the server first sees
workspaceFilePathit hashes it with SHA-256 (not MD5 — that distinguishes the new scheme from older plugins that MD5-hashed the same field) and, only if no file exists at the new name, performs a one-timerenameSyncof the folder-based file that matches the projects open at that first launch.Consequences worth knowing before merge:
workspaceFilePathshows empty history.no-workspace, so the sharedchat-history-no-workspace.jsonis moved into this workspace's file and is no longer available to other workspaces that start with no projects open.Testing
mvn -pl plugin -am -Dskip.npm -Dskip.installnodenpm verify→ BUILD SUCCESS, 536 tests, 0 failures, 0 errors, 0 Checkstyle violations.WorkspaceUtilsTest(6): canonical path returned; redundant path segments resolve to the same id; fallback to the literal path when canonicalization fails, with the exception logged;nulllocation; blank path short-circuits before touching the filesystem;ResourcesPluginunavailable.AmazonQLspServerBuilderTest(3, new): drives aninitializerequest throughwrapMessageConsumerand assertsaws.awsClientCapabilities.q.workspaceFilePathis present with the workspace root, absent when the root is unknown, and that the otherqcapabilities are unaffected.Not verified: the end-to-end restart behaviour in a running Eclipse instance on Linux and Windows — open two tabs on the old build, upgrade and restart to see
Migrating history file…with both tabs back, then add or close a project and restart to confirm theInitializing database atpath is unchanged. I do not have an Eclipse runtime available, so this still needs a manual pass before merge.Notes
workspaceFilePathis read only by the chat history database on the server side; no other server feature consumes it.renameSyncruns unguarded duringChatDatabaseconstruction, so a failure there would prevent chat from starting; copying instead of renaming, and skipping the sharedno-workspacefile, would both be safer.