Choose preserved original sources in Companion - #2855
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7767a7239
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch { | ||
| if (request === generation) error.value = 'Originals could not be checked. Retry, or refresh all sources if this memory changed.' |
There was a problem hiding this comment.
When board access is revoked while the picker is open, the new endpoint returns 403, but this catch collapses it into the same retry/refresh message as stale-version and network failures. That leaves the user repeatedly retrying an operation they no longer have permission to perform; inspect the response status and show the required permission-specific state for 403 while retaining the generic retry message for transient failures.
AGENTS.md reference: frontend/AGENTS.md:L5-L6
Useful? React with 👍 / 👎.
|
Review disposition for comment3973520894: MEDIUM usability, non-blocking. The API enforces revoked board access with403 and the picker does not reveal a source; the defect is that its error text does not distinguish permission loss from transient/stale selection failures. Track permission-specific picker messaging and retry disabling in #2808's remaining frontend follow-through. No security authorization bypass or data-loss path was identified, so this does not trigger a parent fix/review cascade. |
…text-base-refresh
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 427e6b86c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| public async Task<Result<ChatAssetPage>> ListSourcesAsync(Guid actorId, Guid boardId, Guid memoryId, int revision, int offset, CancellationToken ct) | ||
| { | ||
| if (boardId == Guid.Empty || memoryId == Guid.Empty || revision < 1 || offset < 0 || offset > 1000) |
There was a problem hiding this comment.
When a memory accumulates at least 1,011 inline assets through corrections, the page at offset 1000 returns nextOffset: 1010, but this guard rejects that continuation request with 400. Capture.AppendSourceAsset deliberately leaves superseded correction history unbounded, so preserved originals after the first 1,010 assets become unreachable; either accept every emitted offset or stop emitting an unusable continuation.
Useful? React with 👍 / 👎.
| <button v-if="!loaded || nextOffset !== null" type="button" :disabled="disabled || loading" @click="load"> | ||
| {{ loading ? 'Checking originals…' : loaded ? 'Load more originals' : 'Choose original sources' }} |
There was a problem hiding this comment.
When a board has multiple memories, every child renders the same accessible button name (Choose original sources or Load more originals), and the plain containing div has no semantic association with the corresponding memory label. Screen-reader button navigation and voice control therefore cannot distinguish which memory each button operates on; include the memory title in the accessible name or group each picker under an associated label.
Useful? React with 👍 / 👎.
|
PR2855 comments3973786334 and3973786336 are MEDIUM, non-blocking follow-through: pagination stops accepting offsets beyond1000 despite emitting the next offset for longer correction histories; repeated source-picker buttons lack a memory-specific accessible name. Neither bypasses authorization or deletes evidence. Track accepting every emitted continuation within a bounded page size and adding memory-specific accessible labels in the #2808 source-picker follow-through. The existing permission-message finding3973520894 remains tracked separately. No parent review loop or broad source audit is reopened. |
1 participant
Companion can now use an explicitly chosen preserved answer or evidence asset without copying private text into the message. The picker shows bounded excerpts and labels superseded answers as historical. The saved turn retains the exact asset ID, memory version, content hash and supersession state across reloads and all four experiences.
Server reads enforce owner, active board access, native capture membership and expected version/hash. Current context reads avoid memory-history graphs; source choices page ten rows and the combined five-private-source/20,000-character context limits remain. Existing context JSON stays compatible. No migration, implicit retrieval, external fetch or automatic board Apply is introduced. This reduces source-copying and maintenance while preserving Review/Approve/Apply.
Validation:
Stacked on the older-memory preservation branch until its parent lands. Related to #2808; audio/transcription, richer observations/attention, proposal overlays and other retained follow-through remain open. Test/Mock transport evidence does not establish live model quality, physical-device acceptance or deployment.