feat(settings): guided issue reporting with upload flow (v4, part 3) - #2755
feat(settings): guided issue reporting with upload flow (v4, part 3)#2755Emt-lin wants to merge 5 commits into
Conversation
8f3fe3d to
f2cc5ee
Compare
32fb3df to
cde11da
Compare
f2cc5ee to
6d61a3f
Compare
cde11da to
823f54d
Compare
|
@codex review |
![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: 823f54d11b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
6d61a3f to
8a80d01
Compare
823f54d to
edd2f01
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
8a80d01 to
359cbf7
Compare
edd2f01 to
fb0800e
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
359cbf7 to
4b042d7
Compare
fb0800e to
20da64e
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20da64e94e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
4b042d7 to
b1cfce1
Compare
20da64e to
a5f1f26
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5f1f26fdf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
b1cfce1 to
16cdb87
Compare
a5f1f26 to
c5e27b0
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
7558427 to
f606964
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f606964ef2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
08f156d to
f73aaa0
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f73aaa0218
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
a4cc9fb to
9fc8a01
Compare
9290cf4 to
cb3229f
Compare
- Replace drag-and-drop with upload flow (mock uploader) - Add 3-page ReportIssueFlow (details → review → done) - Build linked issue URL with non-truncatable prefix - Add ReportUploader contract + mockReportUploader - Expand issueReport/redactLog/logFileManager utilities - Wire uploader into AdvancedSettings - Update docs: drag → upload - Add REPORT_UPLOAD_FLOW.md design doc
The review step invites the user to edit the staging folder before rebuilding, so a file that has since been deleted is an ordinary edit rather than a failure. It comes back demoted in the returned outcomes and the rest of the report still packs, instead of throwing and stranding the user with nothing to send — the rebuild deletes the old zip first. Deletion is not the only way a read can fail, though. A file that is still there and merely cannot be read — locked by another program, or its permissions changed underneath us — must not be reported as "Removed from the report folder": that blames the user for an edit they never made, while the report goes out a source short on the strength of it. Only a missing file is a removal; anything else is a failure carrying the real error, which is the split `assembleReportBundle` already makes for the same class of trouble. Reuses the existing `isMissingFileError`, which recognises both a Node `ENOENT` code and the message-only shapes other adapters throw. `report.md` remains the one file whose loss stops the rebuild, because without it there is no issue to file.
…hment Deleting an attachment from the staging folder and rebuilding leaves `report.md` still listing it, so the public issue can claim an attachment the zip omits. The obvious fix — regenerate `report.md` from the final manifest — is worse than the problem it solves. The flow explicitly invites the user to edit the staging folder, and `report.md` is one of the files they may have edited, including to redact their own prose; regenerating it would discard that silently. What goes stale is a filename in a list, never file content, so a user who deleted a screenshot to keep it private still gets that. The note records the tradeoff and points at logancyang/obsidian-copilot-preview#279, which tracks the two ways out: detecting the discrepancy and letting the user choose, or dropping the attachment list from `report.md` altogether.
`ReportUploadResult.reportId` had no reader anywhere in the plugin: the type declared it and the mock produced it, and nothing else touched it. Its stated purpose was revocation and support correlation, neither of which exists yet. The cost of keeping it is not the field but the promise. The upload endpoint has not been built, so this type is the contract it will be written against, and a required field obliges it to return something no caller wants. Revocation, when it lands, can add both the id and the call that consumes it.
The failure matrix said the object was orphaned and left to server-side expiry. That was true of an earlier draft; the flow now opens the prefilled issue whether or not the dialog survived, because the upload cannot be aborted and withholding the link would leave an upload the user can neither see nor use. The matrix is where a reader goes to look up exactly this boundary, so it was the one place the code and its own description disagreed. Covers the two paths that were asserted nowhere: a Retry that really uploads a second time (proving the single-flight lock is released on failure), and a failure arriving after the dialog is gone, which must open nothing and write no state.
2732e1d to
931f37e
Compare
1 participant
Stacked on #2713. Base is
settings-v4-agents-basic; it retargets tov4-previewautomatically once #2713 lands. Merge #2713 first.Reworks the report flow first shipped in #2614 for
logancyang/obsidian-copilot-preview#155, which is already closed — this is the
follow-on refactor and hardening of the same feature, not a second attempt at
that issue.
Problem
Reporting a bug meant assembling the evidence by hand. #2614 gave the report
a modal, but the user still ended up with a folder: a screenshot, a frame log, a
chat log, each attached separately, and no indication of which ones actually
made it in. Sources that could not be read were dropped silently, so the issue
arrived missing the log the maintainer needed with nothing saying so.
Three concrete defects behind that:
chatLogRequest()exportedlogFileManagerwithout draining the promptrecorder first, so the attached chat log stopped one request short of the
failure being reported — the Settings row it replaced did flush
(
commands/index.ts:544still does).redactLog.tshad an unbounded local part, which backtracksover every suffix of a long run of address-legal characters. A pasted token
supplies exactly that: 27 s for a 256 KB run, versus 31 ms bounded.
<tmpdir>/obsidian-copilot/reports. On a machinewhose temp dir is shared between accounts, that path is someone else's to
create first, and a symlink planted inside it ahead of time is followed by the
write that lands there — reproduced locally: victim file overwritten and
chmod-ed.
Fix
A three-page dialog — describe → review → done — that produces exactly one
copilot-report-<id>.zipand reports a per-source outcome for everything in it(
skipped/failed/truncated), so the list reflects the zip rather thanechoing the user's checkboxes back at them.
Upload replaces the drag hand-off: the bundle is uploaded, the returned link is
written into the prefilled issue body, and the user only presses Submit.
Upload & open issue is a separate click from packing — nothing leaves the
machine until it is pressed. If the upload fails the zip is still on disk, with
Retry upload, Show in folder, or Open issue anyway to attach by hand.
Private per-report directory via
mkdtemprather than the fixed path, chosenover hardening the fixed one: it creates atomically, names unpredictably, and is
owner-only from the start, which settles squatting, prediction, and permissions
in one call instead of three guards.
The linked issue URL puts the share link in a non-truncatable prefix and shrinks
only the body beneath it, so an over-long note can never cost the reader the
link.
Scope
21 files changed, +5206 / −465 relative to #2713. About 2,300 of those additions
are tests.
Changes
utils/issueReport.ts— bundle assembly, per-source outcomes,budget/limit enforcement, redaction of everything reaching
report.mdor theissue URL, and
buildLinkedReportIssueUrl. Injectable Node runtime, so it isunit-testable without a filesystem.
agentMode/ui/ReportIssueFlow.tsx(new) — the three pages and onemutually-exclusive state machine; rebuild and upload each lock the other out
in both directions via a shared synchronous operation lock.
agentMode/ui/ReportIssueModal.tsx— capture, log collection, privatetemp dir, cleanup, upload orchestration.
waitForStableTargetpolls geometryon the target's own window rather than the main window's frame clock, which
never fires when Obsidian is backgrounded.
utils/reportUpload.ts(new) — theReportUploadercontract, keptseparate from both
issueReport.tsand the mock so swapping in a real adaptertouches one new file.
utils/redactLog.ts— RFC-bounded quantifiers on the email rule.settings/v2/components/AdvancedSettings.tsx— the report entry pointunder Debugging & support, injecting the uploader.
agentMode/ui/ReportIssueFlow.stories.tsx(new) — gallery stories for allsix load-bearing states (details, packing, review, uploading, upload-failed,
done), per the component-gallery workflow in
TESTING_GUIDE.md.Blocked on
The real upload endpoint. When it is built, note that
makeFormDataRequestunconditionally attaches an
Authorizationheader even withskipLicenseCheck(
brevilabsClient.ts:222,243), so the adapter will need anexcludeAuthHeaderpath. Retry idempotency and completion ambiguity ("failed but the server
actually succeeded") are contract properties the endpoint must provide; a mock
cannot demonstrate them.
Verification
npm run test— 396 suites, 5607 tests, all passing (3 suites / 115 testsmore than feat(settings): fold Agents into Basic (v4, part 3) #2713, which is exactly this PR's coverage).
npx tsc --noEmit,npm run lint,npm run format:check— clean.chmod-ed), then confirmed closed: the squatted directory is no longer used and
the actual path is unpredictable and
0700.redaction is not weakened (an over-length local part still has its
@anddomain replaced).
umask000, 022, and077; POSIX-only tests skip on win32.
Not verified: a real upload. The mock always succeeds, so the failure paths are
covered by injected rejecting uploaders in tests rather than by a live endpoint.
Deferred
session/debugSink.tswrites full prompt frames to a predictable<tmpdir>/obsidian-copilot/acp-frames/<hash>/with default permissions — thesame defect class as the report path, more sensitive and longer-lived, but it
predates this PR. Filed as logancyang/obsidian-copilot-preview#250.
Actual Effect