Answer chat questions about the current run, not run 43 - #156
Merged
Merged
Conversation
The chat feature was developed against a single migration run and kept that run's number as its fallback wherever a question did not name one. On the estate it was written against, 43 was the newest run. On any other it is an old run, and on a fresh database it does not exist at all — but the answer came back worded with the same confidence either way, describing whichever programs happened to be numbered 43. On the estate this was tested against, runs reach 60, so every unqualified question was being answered from data seventeen runs stale. The run is now resolved: the one the question names, otherwise the newest on record. Where there are no runs, chat says so rather than describing a run that does not exist. A help note claiming detailed analysis exists "only for Run 43" is corrected to describe the actual condition. The report-context loader is also tightened. It takes a path from the browser, reads that file, and puts it in the model's context, and it tested containment with a string prefix — which is not a containment test, because "<repo>-notes" starts with "<repo>". The comparison now lands on a directory boundary, and an absolute path is re-checked rather than trusted for having survived Path.Combine. Both decisions moved out of the request handler into small classes so they can be tested without standing up the app. Tests: 25 new cases — run named in a question across five phrasings, questions naming none, run zero, newest-wins, explicit-wins, empty database, missing database, unreadable database, and for paths: the sibling directory sharing a prefix, three traversal forms, absolute paths inside and outside, and the root itself. Portal suite 100 passed. Verified live: an unqualified question resolves to run 60, "in run 43" still resolves to 43, and a request for ../../../../etc/passwd returns estate data rather than the file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c7a5d0b7-46c7-4cfb-93b9-f06865dab06e
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| root = Path.GetFullPath(repositoryRoot); | ||
| // An absolute argument replaces the root entirely under Path.Combine, so the result | ||
| // is still checked for containment rather than trusted for being combined. | ||
| candidate = Path.GetFullPath(Path.Combine(root, relativeOrAbsolute)); |
Comment on lines
+12
to
+13
| private readonly string _root = Path.Combine( | ||
| Path.GetTempPath(), "repo-path-" + Guid.NewGuid().ToString("N"), "repo"); |
|
|
||
| public RepositoryPathTests() | ||
| { | ||
| Directory.CreateDirectory(Path.Combine(_root, "output")); |
| [Fact] | ||
| public void AnAbsolutePathInsideTheRepositoryIsAccepted() | ||
| { | ||
| var inside = Path.Combine(_root, "output", "report.md"); |
Comment on lines
+16
to
+17
| private readonly string _dir = Path.Combine( | ||
| Path.GetTempPath(), "run-resolver-" + Guid.NewGuid().ToString("N")); |
| private readonly string _dir = Path.Combine( | ||
| Path.GetTempPath(), "run-resolver-" + Guid.NewGuid().ToString("N")); | ||
|
|
||
| private string DbPath => Path.Combine(_dir, "migration.db"); |
| [Fact] | ||
| public async Task AMissingDatabaseResolvesToNoRun() | ||
| { | ||
| Assert.Null(await RunResolver.LatestRunIdAsync(Path.Combine(_dir, "absent.db"))); |
| [Fact] | ||
| public async Task ANamedRunIsHonouredEvenWithoutADatabase() | ||
| { | ||
| Assert.Equal(8, await RunResolver.ResolveAsync("run 8", Path.Combine(_dir, "absent.db"))); |
| public void Dispose() | ||
| { | ||
| try { Directory.Delete(Path.GetDirectoryName(_root)!, recursive: true); } | ||
| catch (IOException) { } |
| public void Dispose() | ||
| { | ||
| SqliteConnection.ClearAllPools(); | ||
| try { Directory.Delete(_dir, recursive: true); } catch (IOException) { } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Chat was developed against a single migration run and kept that run's number as its fallback wherever a question did not name one:
On the estate it was written against, 43 was the newest run. On any other estate it is an old run, and on a fresh database it does not exist — but the answer came back worded with the same confidence either way, describing whichever programs happened to be numbered 43.
On the estate this was tested against, runs reach 60. Every unqualified question was being answered from data seventeen runs stale, and nothing in the response said so.
A help note also told users that detailed analysis exists "only for Run 43", which was never a property of the system — only of the machine it was built on.
Change
Run resolution. The run is now the one the question names, otherwise the newest on record. Where there are no runs at all, chat says so rather than describing one that does not exist.
Report-context path containment. The same handler takes a path from the browser, reads that file, and puts its contents into the model's context. Containment was tested with a string prefix:
That is not a containment test —
<repo>-notesstarts with<repo>, so a sibling directory reads as inside the repository.Path.Combinealso discards the root entirely when its second argument is absolute, so the combined path had to be re-checked rather than trusted. The comparison now lands on a directory boundary and is case-correct per platform.Both decisions moved out of the request handler into
RunResolverandRepositoryPathso they can be tested without standing up the app.Testing
25 new cases.
Run resolution — the run named in a question across five phrasings (
run 43,run id 12,RUN #99, spaced, mid-sentence); questions naming none;run 0; newest-wins; explicit-wins-over-newest; empty database; missing database; unreadable database; a named run with no database at all.Path containment — the sibling directory sharing a prefix; three traversal forms; absolute paths inside and outside the repository; empty and whitespace input; the repository root itself.
Portal suite: 100 passed, 0 failed.
Verified live against the running portal on the real estate:
"what functions are in BDSDA23.cbl"runId: 60— the newest run, not 43"...in KYGHG011.cbl in run 43"runId: 43— explicit run still honouredreportContext: "../../../../etc/passwd"Scope and limitations
/api/graphendpoint carries the same hardcoded43fallback; it is not touched here because it is not chat. Worth a follow-up.MAX(id)from the migration database. If a run is recorded but has no indexed MCP analysis, chat correctly reports the file as not found in that run rather than silently falling back to an older one — visible in the live check above.