Uh oh!
There was an error while loading. Please reload this page.
feat(desktop): open workspace Markdown references - #3819
Conversation
6f258a7 to
7c25d6fCompare1625567290
commented
Aug 26, 2026
Rebased onto current main 51eeaa6 and Astryx core 0.5.0; the exact head is 7c25d6f. All hosted checks are terminal and successful:
Exact-head local validation passed UI tests 236/236, Desktop typecheck, workspace IPC and guard boundaries 9/9, packaged-app verification 18/18, the workspace Markdown Electron E2E, ASF headers, Astryx surface inventory, and changed-file lint/format checks. There are no unresolved review threads. Ready for human review. |
7c25d6f to
39e0bc1Compare1625567290
commented
Aug 26, 2026
Exact head All hosted checks for this exact head are terminal and successful:
The hosted CI executed the full build, affected workspace tests, Desktop E2E, alignment audit, Storybook smoke, and installed CLI release-candidate validation. The final diff contains no documentation/Markdown or Java test sources, and there are no unresolved review threads. After these runs completed, |
39e0bc1 to
cb7847aCompare1625567290
commented
Aug 26, 2026
Current exact head The same assertion failed at the same line and with the same 0-vs-1 result in two independent concurrent runs:
This PR run otherwise passed 66 Desktop E2E cases with 1 conditional skip, and its dependency audit, Windows package, and Windows recovery checks are green. Contributor retry was attempted and GitHub returned |
cb7847a to
d69957cCompare1625567290
commented
Aug 26, 2026
Exact head All hosted workflows for this head are terminal and successful:
The new CI run passed the complete Desktop E2E collection, including the unchanged prompt-rail case that failed concurrently on multiple branches in the previous batch, plus alignment audit, Storybook smoke, and installed CLI release-candidate validation. The exact PR diff contains no documentation/Markdown or Java test sources, and there are no unresolved review threads. Target |
d69957c to
b852fb8Compare1625567290
commented
Aug 26, 2026
Rebased onto current The feature commit is patch-equivalent to the previous reviewed head according to The 26-file diff contains no documentation/Markdown or Java test sources. GitHub reports the PR mergeable with no unresolved review threads. |
1625567290
commented
Aug 26, 2026
Follow-up on exact head |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for extending workspace Markdown references through the existing Workbar and Markdown seams. I reviewed exact head b852fb87ba8959c73f4ca6317952767dba365be4.
The capability is useful, but the file-authorization boundary and raw-Markdown preprocessing need revision before it is safe to merge. I found one P1 and three P2 issues inline:
- a checked path string is reused after the filesystem authorization check, leaving a symlink-replacement window for out-of-workspace reads;
- the Markdown extension allowlist is checked before canonicalization and can be bypassed by an in-workspace
.mdalias; - the line-oriented preprocessor rewrites valid Markdown code/external/path forms incorrectly;
- the barrel re-export restores a measured eager-renderer chunk regression that current
maindeliberately removed.
The latest exact-head test, package, windows_recovery, and audit runs have successful results. The PR is currently conflicting with main; please preserve the current lazy Workbar boundary when rebasing and rerun checks on the resolved head.
Because this also changes a user-facing preview surface, human UI/UX acceptance remains required before approval.
Review analysis was assisted by Codex and independent @reviewer agents. Astro-Han verified the exact-head diff, path trust boundary, Markdown production behavior, current-main chunk composition, reachability, and severity before publication and owns this review.
中文对照
谢谢你沿用现有 Workbar 和 Markdown seam 增加 workspace Markdown references。我复核了精确 head b852fb87ba8959c73f4ca6317952767dba365be4。
当前有一个 P1 和三个 P2：文件授权后复用 path string 留下 symlink 替换窗口；扩展名只在 canonicalize 前检查，可由 workspace 内 .md alias 绕过；逐行 raw Markdown 预处理会错误改写多种合法 Markdown；barrel re-export 重新引入 current main 已明确删除、且有量化证据的 eager chunk 回归。
当前最新几项检查有成功结果，但 PR 与 main 冲突；重基时必须保留当前 lazy Workbar 边界，并在解决后的 exact head 重新跑 CI。该 PR 也改变了用户可见 preview surface，因此 Approve 前仍需人工 UI/UX 验收。
本次审查分析由 Codex 和独立的 @reviewer 子代理协助；Astro-Han 核验了精确 head、path trust boundary、Markdown 生产行为、current-main chunk composition、可达性和问题分级，并对本次 Review 负责。
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
f997445 to
2e11bfcCompare2e11bfc to
16e8e2aCompare1625567290
commented
Aug 30, 2026
Exact-head follow-up for
All four review findings remain resolved, with zero unresolved review threads. The 33-file PR diff contains no documentation/Markdown files or Java test sources. |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the substantial follow-up—the opened-handle preview read, canonical extension check, and restored Workbar lazy boundary close the previous findings well. I found three remaining production seams below. These are suggestions from an outside review, so please do push back where the Markdown or local-workspace threat model is intentionally narrower.
AI-assisted review disclosure: Codex ran independent workspace-security, Markdown, and production/test analysis lanes; Astro-Han is the contributor of record for this review.
| input.ipcMain.handle('workspace:openFile', async (_event, sessionId: unknown, relativePath: unknown) => { | ||
| const resolved = await resolveSessionWorkspaceFile(input, sessionId, relativePath); | ||
| if (!resolved.ok) return resolved; | ||
| const error = await input.openPath?.(resolved.path); |
There was a problem hiding this comment.
[P2] (category ②/③ — local replacement/trust-boundary path)
Thanks for binding preview reads to an opened inode. The “Open locally” path still authorizes a pathname and later hands that mutable pathname to the OS opener: after resolveSessionWorkspaceFile() verifies realpath/containment/extension/stat, another local process can replace guide.md with an outside-workspace symlink before openPath() resolves it. That leaves the PR’s Open locally boundary weaker than the fixed preview read. Could this action consume an identity-bound artifact (for example a controlled staged copy from the authorized handle), or be withheld until the platform exposes a non-re-resolving primitive, with a deterministic replacement-race regression? Please push back if local workspace replacement is explicitly outside this action’s supported threat model.
| return source | ||
| .split('\n') | ||
| .map((line) => { | ||
| const blockLine = markdownBlockContent(line.endsWith('\r') ? line.slice(0, -1) : line); |
There was a problem hiding this comment.
[P2] (category ① — normal Markdown content)
Thanks for adding the code/link exclusions. The remaining line-oriented preprocessor still changes valid Markdown structures that span lines or containers: a line - ``` inside an already-open top-level fence is treated as that fence’s close, multiline backtick code and multiline link destinations lose their state between lines, and math is linkified before prepareMarkdownMath() (for example \\( \\texttt{docs/guide.md} \\)). In each case literal content is rewritten into [...](...). Rather than adding more line cases, could workspace linking run on parsed text nodes (or share a real block/inline state machine), with regressions for nested-container fences, multiline code/link, and inline/display math? Please feel free to push back if Astryx intentionally excludes any of these CommonMark/math forms.
| if (href.length === 0 || href.length > HREF_MAX_LENGTH) return null; | ||
| let decoded: string; | ||
| try { | ||
| decoded = decodeURIComponent(href); |
There was a problem hiding this comment.
[P2] (category ① — normal percent-named workspace paths)
Thanks for keeping URI parsing fail-closed. The renderer already decodes an explicit href in parseWorkspaceFileHref() and sends the decoded filesystem path in WorkspaceFileDest.relativePath, but main calls decodeURIComponent() again. A valid docs/100%25-safe.md href becomes literal docs/100%-safe.md in the renderer and then fails the second decode; docs/100%2525-safe.md can instead open the wrong percent-named file. Structured workspace_file chips also start from a literal path and hit the same decoder. Could the wire contract choose one owner for URI decoding—decode explicit Markdown hrefs once, then have IPC/main validate the literal relative filesystem path—and cover %, %25, and percent-like directory names end to end? Please push back if the IPC contract is intended to carry encoded hrefs rather than paths.
Summary
Fixes#2664
Verification
The full local Desktop run reached 1504 passing tests. Eight existing MCP OAuth deadline tests were cancelled under local Node 22.22.1; the affected workspace tests and exact-head Electron E2E pass, and the OAuth source and test files are unchanged from main.
Checklist
Does this PR entail a change in behavior?