Repository navigation
Conversation
github-actions
Bot
added
the
needs:compliance
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
This stops the permission bypass, but it breaks the session: a session created with a .. directory never runs its prompts.
I ran the #55909 repro from source on the base and on this branch. On the base, the <base>/src/../proj session runs the tool without asking, which is the bug. With this change, that session is still created and stored as …/src/../proj and the prompt request is accepted. But the model is never called, the session has no messages (not even the user's), and nothing is logged. The bypass is gone only because nothing runs.
Only the instance key is normalized. The session row and its events keep the raw path, and a few places compare the two with ===:
bus.ts:734routes events to the instance, so this session's events are filtered out.session/context.ts:124interruptsselectwhen the directories differ.plugin/host.ts:92andsession/move.ts:136compare the same way.
Please normalize the directory where it comes in (when the Location.Ref / location.directory is decoded, or in session create, move and import), so the stored session, its events and the instance all use the same path. A regression test should go through session create and prompt with an a/../b directory, rather than only call canonical(). The current test passes even though the session is broken.
Public API: no schema change. Callers that send a .. directory to POST /api/session currently get a session that silently never runs.
Related: #53735
| export function canonical(ref: Location.Ref) { | ||
| return Location.Ref.make({ | ||
| directory: AbsolutePath.make(process.platform === "win32" ? path.normalize(ref.directory) : ref.directory), | ||
| directory: AbsolutePath.make(path.normalize(ref.directory)), |
There was a problem hiding this comment.
Normalizing only here makes the instance key differ from the session's stored location.directory, which stays raw (…/src/../proj). bus.ts:734 filters events with ref.directory === location.directory, and session/context.ts:124 interrupts on the same mismatch. In the #55909 repro, the .. session's prompt never reaches the model and no messages are recorded. Normalizing where the location comes in (when it is decoded, or in session create, move and import) would keep the session, its events and the instance on the same path.
There was a problem hiding this comment.
Good catch — moved the normalization to the entry boundary. Session.create now stores LocationServiceMap.canonical(location), so the session row, the location-service key, bus filtering and SessionContext.select all compare the same directory, and canonical itself now normalizes on every platform (it was win32-only). New test creates a session with /project/src/.. and asserts the stored directory is the canonical one; it fails on base and passes with the fix. The existing 'follows the directory's project identity' test pinned the aliased spelling on the stored location — updated to expect the canonical form, since that is the behavior the issue asks for.
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
There was a problem hiding this comment.
My earlier concern is resolved: Session.create now stores the normalized location, so the session row, the location-service key, bus routing and SessionContext.select all use the same path. I re-ran the #55909 repro from source. Both <base>/proj and <base>/src/../proj now run the prompt and stop at one pending shell permission, with no marker.txt (the base runs the tool without asking). The related packages/core tests pass (70 pass, 2 skip) and typecheck is clean. The updated project-identity test expectation is correct for this fix.
Public API: no change in shape. POST /api/session now returns and stores location.directory normalized (e.g. /a/src/../b → /a/b). This is backwards compatible.
Still worth fixing:
SessionTransfer.import(session/transfer.ts, around lines 74–109) still storesinput.locationas given. Now that the instance key is normalized on every platform, an imported session with a..directory gets the problem I described last time: its events are filtered out and its prompts never run. Normalizing it the same way ascreatewould fix this. Normalizing once in a shared place (where the location is decoded, or a helper both paths use) would cover create and import together.- Sessions saved before this change with a
..directory will hit the same mismatch after upgrading. This is probably rare, but worth knowing about.
Related: #53735
|
Added the import path: |
|
@Finn763 thanks for your contributions and on this specific case also :3 |
Issue for this PR
Closes #55909
Type of change
What does this PR do?
Config discovery walked
location.directoryas given, so<base>/src/../projloaded<base>/src/opencode.json— a directory that is not an ancestor of the real placement — and applied itspermissionrules to the session.Normalization now happens where the location comes in, not only at the resource-cache key:
Session.createstoresLocationServiceMap.canonical(location), so the session row, the location-service key,busevent filtering andSessionContext.selectall compare the same directory.LocationServiceMap.canonicalnormalizes on every platform — it was win32-only.The existing test
follows the directory's project identity established after creationpinned the aliased spelling (alias/../packages/app) on the stored location; it now expects the canonical form, which is the point of this fix.How did you verify your code works?
normalizes a location that arrives with dot segmentsfails on base (storedlocation.directorykeepssrc/..) and passes with the fix.bun test test/session-create.test.ts— 44 pass, 1 pre-existing Windows failure (runs a shell command and projects the started/ended shell message, fails identically on the pristine base).bun test test/location-service-map.test.ts test/location.test.ts test/location-layer.test.ts— 22 pass, 5 pre-existing skips.bun run typecheckclean.Screenshots / recordings
N/A.
Checklist
Added by the opencode bot (AI-generated).
Related: #53735