fix(core): Windows path compatibility + feat: badge.deleteOrphans command#126
fix(core): Windows path compatibility + feat: badge.deleteOrphans command#126araadhay-py wants to merge 2 commits into
Conversation
…mand All path-joining operations now normalise to POSIX forward-slashes via a new toPosix() kernel utility, fixing the entire mock-FS test suite on Windows (103 failures → 18, all remaining are OS-level EPERM symlink tests that require Developer Mode). Adds badge.deleteOrphans: a new core command that bulk-removes every badge whose orphan flag was set by the file-watcher. Wired into the Command Palette as "Clean up N orphaned badges" with a confirmation dialog and canvas refresh on completion.
|
All contributors have signed the CLA ✍️ ✅ |
![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: 3f76ac536a
ℹ️ 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".
| // Remove from focus.md if it was in the active list. Best-effort: a | ||
| // PathEscape or missing focus module must never abort the cleanup loop. | ||
| try { | ||
| await ctx.run('focus.renameActiveFile', { from: badge.file, to: null }); |
There was a problem hiding this comment.
When an orphaned badge is also present in .bh/focus.md, this calls focus.renameActiveFile with to: null, but that handler only remaps strings: it replaces the active path with args.to, then writes the resulting item back. In this scenario cleanup leaves the focus brief containing - null instead of removing the deleted file, so agents keep receiving a bogus focused path after the user explicitly cleaned up orphaned badges.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the follow-up commit (3340ebc).
Replaced the focus.renameActiveFile({ from, to: null }) call with:
const focusState = await ctx.run('focus.get', {});
if (focusState.active.includes(badge.file)) {
await ctx.run('focus.toggleActiveFile', { file: badge.file });
}focus.toggleActiveFile removes the file when present and adds it when absent, so the guard on includes is necessary to avoid accidentally re-adding a non-focused file. The whole block stays best-effort (wrapped in try/catch) so a PathEscape on a hostile focus.md never aborts the cleanup loop.
| const inboundRes = await ctx.run<{ file: string }, { entries: { from: string }[] }>( | ||
| 'inbound.get', | ||
| { file: badge.file }, | ||
| ); | ||
| for (const entry of inboundRes.entries) { | ||
| await ctx.run('inbound.removeRef', { from: entry.from, to: badge.file }); |
There was a problem hiding this comment.
When an orphaned badge itself has outbound references, those refs already created inbound-index entries under each target via badge.addRef; deleting the badge JSON here only removes entries keyed by the deleted badge (incoming refs). In that scenario the targets keep stale inbound rows from a non-existent file, so badge pages/agent backlink traversal can report phantom inbound links after cleanup; the loop should also remove from: badge.file for each badge.references[*].to.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the follow-up commit (3340ebc).
The inbound cleanup block now handles both directions:
// Inbound: other badges that pointed AT this orphan
for (const entry of inboundRes.entries) {
await ctx.run('inbound.removeRef', { from: entry.from, to: badge.file });
}
// Outbound: this orphan's own refs leave stale backlink rows in targets
for (const ref of badge.references) {
await ctx.run('inbound.removeRef', { from: badge.file, to: ref.to });
}Both loops run inside the same try/catch so a missing inbound module in lightweight tests still doesn't abort the cleanup.
| const deleted: string[] = []; | ||
| for (const badge of orphans) { | ||
| try { | ||
| await removeBadge(ctx.fs, root, badge.file, badge.kind); |
There was a problem hiding this comment.
If an orphan badge JSON has a stale or mismatched file field (for example after a manual edit or merge conflict), cleanup recomputes the path to unlink from that embedded value rather than the JSON file that listBadges just walked. In that scenario badge.deleteOrphans can delete a different, non-orphan badge while leaving the actual orphan JSON behind, so this path should either be validated against the walked badge path or deletion should target the walked file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged — the theoretical gap is real. In normal operation this can't happen: every writeBadge call derives the on-disk path from badge.file and then writes that same value into the JSON, so the field and the file path are always in sync. A mismatch can only arise from a manual edit or a bad merge conflict, not through any API call.
The existing defence layers still apply: removeBadge calls assertWriteContained, which will throw on any path that escapes the workspace, and fs.stat guards against unlinking a path that doesn't exist. So the security invariant holds even in the mismatch case.
Fixing this properly — threading the actual walked path out of listBadges so deleteOrphans can target it directly rather than re-deriving from badge.file — requires a change to the listBadges return type. That's a worthwhile follow-up but out of scope for this PR. Happy to open a tracking issue for it.
|
I have read the CLA Document and I hereby sign the CLA
…On Sat, 6 Jun 2026 at 13:12, github-actions[bot] ***@***.***> wrote:
*github-actions[bot]* left a comment (Pointa-Labs/basehalf#126)
<#126 (comment)>
Thanks for your pull request! Before we can merge it, please sign the BaseHalf
Contributor License Agreement
<https://github.com/Pointa-Labs/basehalf/blob/main/CLA.md>. Reply to this
PR with the sentence below; you only need to do this once.
------------------------------
I have read the CLA Document and I hereby sign the CLA
------------------------------
You can retrigger this bot by commenting *recheck* in this Pull Request. Posted
by the *CLA Assistant Lite bot*.
—
Reply to this email directly, view it on GitHub
<#126?email_source=notifications&email_token=BN5MFFWEVUYV6BVXJ6UPXTD46PDOHA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRTG44DKMZQHEZ2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4637853093>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BN5MFFRNCSRYY2RW72ZK53D46PDOHAVCNFSM6AAAAACZ44FJ6SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DMMZXHA2TGMBZGM>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/BN5MFFXFLEKJOG7DV5DQG4D46PDOHA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRTG44DKMZQHEZ2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG>
and Android
<https://github.com/notifications/mobile/android/BN5MFFVVXMWGL2VSE2IKS4T46PDOHA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRTG44DKMZQHEZ2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>.
Download it today!
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
… and outbound ref cleanup; skip symlink tests on Windows badge.deleteOrphans had three issues flagged in code review: 1. Passed `to: null` to focus.renameActiveFile, which maps the active path to the literal value null rather than removing it, corrupting focus.md. Fixed by calling focus.get first and then focus.toggleActiveFile only when the file is actually present in the active list. 2. Only cleaned up inbound entries pointing AT the orphan (other badges refs removed) but left the orphan's own outbound refs as stale backlink rows in each target's inbound index. Fixed by also iterating badge.references and calling inbound.removeRef for each outbound ref. 3. Symlink-dependent tests in path-escape.test.ts, search-symlink.test.ts, and badge-rename-symlink.test.ts fail on Windows with EPERM because creating symlinks requires Developer Mode at OS level, not a code issue. Guarded all real-fs symlink describe blocks with describe.skipIf(isWin) so the suite passes cleanly on Windows without removing coverage on Linux/macOS CI. Test result: 245 passed, 26 skipped (symlink groups on win32), 0 failed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3340ebcd49
ℹ️ 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".
| * the modules work correctly on Windows without changing test fixtures. | ||
| */ | ||
| export function toPosix(p: string): string { | ||
| return p.replaceAll('\\', '/'); |
There was a problem hiding this comment.
On POSIX, \ is a valid filename character rather than a separator, but this helper now rewrites it unconditionally for absolute workspace paths as well as generated paths. If a user registers a workspace such as /tmp/project\2026, later calls through canonicalize, focusPath, badgePath, etc. will look under /tmp/project/2026 instead, which can make materialization miss the real files or create .bh metadata in the wrong directory; this normalization should be platform-aware or limited to Windows-style paths.
Useful? React with 👍 / 👎.
|
recheck |
|
I have read the CLA Document and I hereby sign the CLA
On Sat, 6 Jun 2026 at 1:18 PM, Araadhay Rai Bawa ***@***.***>
wrote:
… I have read the CLA Document and I hereby sign the CLA
On Sat, 6 Jun 2026 at 13:12, github-actions[bot] ***@***.***>
wrote:
> *github-actions[bot]* left a comment (Pointa-Labs/basehalf#126)
> <#126 (comment)>
>
> Thanks for your pull request! Before we can merge it, please sign the BaseHalf
> Contributor License Agreement
> <https://github.com/Pointa-Labs/basehalf/blob/main/CLA.md>. Reply to
> this PR with the sentence below; you only need to do this once.
> ------------------------------
>
> I have read the CLA Document and I hereby sign the CLA
> ------------------------------
>
> You can retrigger this bot by commenting *recheck* in this Pull Request. Posted
> by the *CLA Assistant Lite bot*.
>
> —
> Reply to this email directly, view it on GitHub
> <#126?email_source=notifications&email_token=BN5MFFWEVUYV6BVXJ6UPXTD46PDOHA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRTG44DKMZQHEZ2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4637853093>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BN5MFFRNCSRYY2RW72ZK53D46PDOHAVCNFSM6AAAAACZ44FJ6SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DMMZXHA2TGMBZGM>
> .
> Triage notifications, keep track of coding agent tasks and review pull
> requests on the go with GitHub Mobile for iOS
> <https://github.com/notifications/mobile/ios/BN5MFFXFLEKJOG7DV5DQG4D46PDOHA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRTG44DKMZQHEZ2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG>
> and Android
> <https://github.com/notifications/mobile/android/BN5MFFVVXMWGL2VSE2IKS4T46PDOHA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRTG44DKMZQHEZ2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>.
> Download it today!
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
|
|
I have read the CLA Document and I hereby sign the CLA |
1 participant
Files Changed
New / modified ?
packages/coresrc/kernel/paths.ts? newtoPosix(p)utility (normalises any path to forward-slash separators)src/kernel/index.ts? exportstoPosixfrom the kernel surfacesrc/kernel/contain.ts?isContained,canonicalize,assertWriteContained,relLabelall normalised withtoPosixsrc/modules/badges/types.ts? addsBadgeDeleteOrphansArgsandBadgeDeleteOrphansResulttypessrc/modules/badges/commands.ts? addsdeleteOrphanshandler; registered asbadge.deleteOrphanssrc/modules/badges/store.ts?badgePath,listBadgeswalk,writeBadgemkdir all usetoPosixsrc/modules/focus/store.ts?focusPathandwriteFocusmkdir usetoPosixsrc/modules/inbound/store.ts?inboundPathandwriteInboundmkdir usetoPosixsrc/modules/search/commands.ts? child absolute path construction usestoPosixsrc/modules/workspace/files.ts?listFiles,readFile,writeFilepath joins all usetoPosixsrc/modules/workspace/materialize.ts? walk child path usestoPosix; removed duplicate localtoPosixsrc/modules/workspace/setup.ts?.gitignoreandCLAUDE.mdpath construction usetoPosixsrc/modules/workspace/store.ts?workspacesFilePathusestoPosixtest/badges.test.ts? 4 new tests forbadge.deleteOrphansModified ?
packages/desktopsrc/renderer/src/components/CommandPalette.tsx? orphan count state, filtered file list, "Clean up N orphaned badges" action with confirmation dialogWHY
Windows path separator bug (103 test failures)
node:pathfunctions (join,normalize,dirname,resolve) produce native backslash (\) separators on Windows. The in-memory mock filesystem used in all tests keys paths with POSIX forward-slashes (/), so every path lookup produced a miss. Every module that constructs a path withjointhen passes it intofs.readFile,fs.mkdir, orisContainedwas broken on Windows.The root cause is
isContainedusingsep(which is\on Windows) for its string comparison, andcanonicalizereturningnormalize(p)whose output is also\-separated. Downstreamjoincalls in every store compounded it. The fix is a singletoPosixnormalisation applied at every path construction site.Orphan badge accumulation (no cleanup path)
When a file is deleted from disk the file-watcher calls
badge.markOrphan, preserving the badge's prompt and references but settingorphan: true. Over time these pile up with no bulk-removal path ? users had to delete them one by one or ignore the "MISSING" cards on the canvas.WHAT
1.
toPosix(p: string): string? kernel utilityNormalises any path string to forward-slash separators (
p.replaceAll('\\', '/')) applied at every point wherenode:pathoutput is stored or compared. Nodefson Windows accepts both separators, so this is safe for real file operations and fixes all mock-fs test key mismatches.2.
badge.deleteOrphans? new core commandFollows the "one door" rule: registered in
commands(), operates throughctx.fsandctx.run, returns{ deleted: readonly string[] }. For each orphan badge it removes the badge JSON viaremoveBadge, cleans up inbound index entries pointing at the deleted badge viainbound.removeRef, and returns the list of deleted paths so the caller can refresh the canvas.3. "Clean up N orphaned badges" ? Command Palette action
When the palette opens, the existing
badge.listfetch is extended to also count orphans and filter them from the "File" open-list (you cannot open a missing file). IforphanCount > 0, an action row appears. Selecting it shows a destructive confirmation dialog, callsbadge.deleteOrphans, then callsemitBadgeChange()to refresh the canvas immediately.COVERAGE
badge.deleteOrphansVERIFICATION
Architecture invariants preserved:
badge.deleteOrphansis registered viacommands()and called throughwindow.bh.runfrom the palette; no direct imports between modules.bh/badges/*.jsonmetadata is removed; no user files are touchedwindow.bh.run; no new reverse dependencies introducedbadge.deleteOrphansis a composable command, not a task-specific one.bh/