feat(dist): the installer, the two channels, the plugin, and the socket (9.14, 10.1-10.6) - #16
Conversation
Plan 9.14 and 10.1 to 10.6 — the last of the plan. **9.14 is two halves and both are here.** The bundle, because an unbundled CLI pays Node's module resolution on every invocation and a `PreToolUse` hook fires it on *every page write* — so the cost is not paid once when a person types a command, it is paid every time an agent touches a page. And the socket: when the desktop application already has the project open, a query is a round trip on a named pipe rather than a process start. The socket carries read and validate and **never** write. That is not scope, it is the safety of the idea: a write over the socket would be a second writer into a project the application has open, with no snapshot of its own, and the operation log would record it as something the application did rather than something an agent did. Both paths call the same functions, so "both paths produce the same answer" is true by construction rather than by two implementations agreeing — and absence is not failure, because a CLI that needed the application running would be the opposite of what `adr:0013` says. Red observed first: 7 assertion failures against signature-only stubs. The installer embeds ffmpeg and `recorder.exe` and puts `ow` on PATH, because `adr:0013` makes `ow` in a directory the way a project is opened — an application the user has to add to PATH by hand is one whose main entrance is undocumented. Two artifacts ship from one tag and are checked to agree *before* anything is published. `adr:0014` named the cost it accepted — a skew "fails looking like corrupted state rather than a bad install" — so the check is a tested module rather than a shell snippet, and it names every artifact that disagrees rather than the first. The winget and Scoop manifests are generated from the tag and the hash that was just published, and both quote it. A package manager that downloads without verifying is the fetch-and-execute this product refuses to ship, which is the same argument `fetch-ffmpeg.mjs` already makes about its own download. Generated rather than hand-maintained because a manifest carrying last release's hash fails as "the download is corrupt" rather than "somebody forgot to update a file". The plugin ships the hooks and the scaffolding command and deliberately not the skills — `adr:0015` gives the convention one home and it is the project, so a copy here would be a second one and two copies of a convention drift. Not a `.mcp.json` either: its contents name *other* projects on that person's machine. `check-plugin.mjs` enforces both, because `claude plugin validate` needs the CLI installed and a check that silently passes when its tool is missing is not a check. The README carries what the plan asked it to and does not soften any of it: that the installer is unsigned and what SmartScreen will therefore say, that telling the other people in a call they are being recorded is the user's responsibility and in many places the law, and that committing a wiki puts `raw/` and `wiki/` in front of everyone with repository access — including `.state/`, which is where a redaction survives the redaction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D3VYWWTZtEE2NxPiksKsAK
The reviews found the packaging did not work and the socket trusted whoever
answered it. Both are fixed here, with what they cost written down.
**The packaged application could not start, and would not have recorded.**
The main bundle was CommonJS, so esbuild erased `import.meta.url` into an
empty object — three call sites, `fileURLToPath(undefined)` at module load,
and a *warning* rather than an error, so the packaging run stayed green.
It is ESM now, the sandboxed preload is `.cjs` because a sandboxed preload
cannot be an ES module, and both carry a `createRequire` shim: the ESM
output's `require` stub throws, and `yaml` calls `require("process")` on the
first page parsed. `resources.ts` states where ffmpeg and `recorder.exe` sit
in the packaged app instead of counting directories up from a source file,
which is three different depths flattened into one bundle. The preload took
`CHANNELS` from `ipc.ts` and dragged the whole main graph in behind it —
273 KB down to 5.4 KB through `channels.ts`.
**The published npm package could not be installed.** It declared two
`workspace:*` dependencies that are never published, so every `npx open-wiki`
would 404, and left three libraries external that npm installs for nobody.
The bundle carries the graph; the manifest declares nothing. Running the
built `ow.mjs` — which nothing had done — found two more: a duplicated
shebang on line 2, and the `require` stub above.
**The `ow` shim pointed at a file that is never packaged**, and `.mjs` has no
Windows file association to be invoked through anyway. It runs the CLI bundle
through `open-wiki.exe` with `ELECTRON_RUN_AS_NODE=1`, which is the only Node
an installer promising no external dependency can count on. It also passed
`--project "%CD%"`, overriding the one test that makes `ow` in a download
folder open the launcher rather than treating that folder as a wiki. The shim
now lives in its own `bin/` directory and only that goes on PATH, so nothing
else in the installation can be shadowed by a file dropped beside it. `EnVar`
is gone — a plugin that may not be in whichever NSIS electron-builder
downloads fails at `makensis` on a machine that is not this one.
**The socket trusted the endpoint name.** It is a hash of a directory path:
obscure, not secret, and off Windows it sat in world-writable `/tmp` where no
legitimate listener ever exists. A failed bind was swallowed, so a squatter
won silently and `ow read` printed its bytes as trusted wiki content into an
agent's context. Both directions are authenticated now — a random token in
the application's data directory, 0600 in a 0700 directory, and an HMAC over
the client's nonce coming back — both buffers are capped, connections are
bounded, the listen error is reported, and the project key is normalised so
two spellings of one directory reach one pipe. The read is confined to
`wiki/` rather than to the project, matching `mcp/src/tools.ts`.
**The release could publish half of itself.** npm ran before the GitHub
release: a failure after it left a permanent version on the registry with no
installer and no way to re-run the tag. Reversed, and the republish guard now
checks both registries. `id-token: write` and `registry-url` were both
missing, so provenance and authentication would each have failed; the tag is
no longer interpolated into a `run:` body in the job holding the npm token
and the signing certificate; a prerelease goes to the `next` dist-tag rather
than becoming what `npx open-wiki` resolves to; and the manifests 10.4
generates are attached to the release instead of being zipped and dropped.
**The plugin.** `continue-on-error` made `claude plugin validate --strict`
report success whatever it said. Its hooks dropped `Bash` — the matcher whose
whole purpose is shell writes — and ran `npx -y open-wiki` unpinned on every
page write, which is the cost the CLI bundle exists to remove and a skew 10.3
exists to prevent. `HOOK_MATCHERS` is now one declaration the plugin is
tested against, and `check-plugin.mjs` refuses an unpinned hook.
README: winget and Scoop were offered as if published. Scoop is, from the
manifest each release attaches; winget is not, and says so.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D3VYWWTZtEE2NxPiksKsAK
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe change adds authenticated CLI-to-desktop queries, packaged desktop and CLI builds, Open Wiki plugin manifests and validation, Windows launcher support, release manifest generation, npm publishing, documentation, and tests. ChangesOpen Wiki access and CLI
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as ow read
participant Socket as askRunningApp
participant App as serveQueries
participant Wiki as Project wiki files
CLI->>Socket: Send authenticated read request
Socket->>App: Connect using project socket
App->>Wiki: Read indexed page
Wiki-->>App: Return page content
App-->>Socket: Return signed response
Socket-->>CLI: Verify response and print page
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
plugins/open-wiki/README.md (1)
5-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language to the fenced code block.
The fenced block has no language tag. markdownlint-cli2 flags this as MD040. Add a language such as
textto the fence.📝 Proposed fix
-``` +```text /plugin marketplace add protonspy/open-wiki /plugin install open-wiki@protonspy</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@plugins/open-wiki/README.mdaround lines 5 - 8, Update the fenced code block
in the README to include a language tag, using text or another appropriate
plain-text language, while preserving the commands unchanged.</details> <!-- cr-comment:v1:1332452798af9287ef619484 --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>.github/workflows/ci.yml (1)</summary><blockquote> `150-156`: _🔒 Security & Privacy_ | _🔵 Trivial_ | _⚡ Quick win_ **Pin the Claude Code CLI version installed in CI.** `npm i -g `@anthropic-ai/claude-code`` installs whatever is `latest` on the registry. The package publishes new releases frequently, so this CI step is not reproducible: two runs on the same commit can install different CLI versions, and a compromised or broken release can affect CI unexpectedly. Pin the version installed here. <details> <summary>🔧 Proposed fix</summary> ```diff - if ! npm i -g `@anthropic-ai/claude-code` >/dev/null 2>&1; then + if ! npm i -g `@anthropic-ai/claude-code`@<pinned-version> >/dev/null 2>&1; thenConfirm the intended pinned version and whether it should be updated alongside a Renovate/Dependabot rule.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 150 - 156, Pin the package version in the global installation command within the “claude plugin validate --strict” workflow step instead of installing the registry’s latest release. Use the confirmed intended `@anthropic-ai/claude-code` version, and preserve the existing skip behavior and strict validation command; update dependency automation configuration only if the repository already uses it for this package.Source: Linters/SAST tools
eslint.config.js (1)
20-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
"**/build/**"makes the existing"apps/desktop/build/**"entry redundant.The new glob already covers
apps/desktop/build/. Remove the narrower entry so one rule governs generated bundles.♻️ Proposed cleanup
"**/release/**", "vendor/**", - "apps/desktop/build/**", ],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eslint.config.js` around lines 20 - 23, Remove the redundant "apps/desktop/build/**" ignore entry from the ESLint configuration, keeping the broader "**/build/**" pattern as the single rule governing generated bundles.packages/access/tests/socket.spec.ts (1)
137-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a large page with non-ASCII characters to the end-to-end test.
Every page in this spec is small ASCII, so each response arrives in one chunk. That hides the chunk-boundary decode defect raised on
packages/access/src/socket.tsLine 314. A page of a few hundred kilobytes containing multi-byte characters exercises the multi-chunk path.Note that such a page also exceeds
MAX_LINE_BYTES, soaskRunningAppreturnsnullby design. Size the test body underMAX_LINE_BYTESbut over one socket chunk, for example 32 KiB of a repeated multi-byte character, to assert that the wire answer still equals the standalone answer byte for byte.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/access/tests/socket.spec.ts` around lines 137 - 181, Add a large page containing repeated multi-byte characters to the end-to-end test around serveQueries, askRunningApp, and handleRequest, keeping the payload below MAX_LINE_BYTES but larger than one socket chunk (for example, about 32 KiB). Assert that the over-wire response equals the standalone response byte for byte, while preserving the existing server cleanup..github/workflows/release.yml (1)
166-179: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoffConsider replacing the third-party release action with the built-in
ghCLI.Static analysis notes that
softprops/action-gh-release@v2's functionality is already available on the runner through theghCLI, which is already used elsewhere in this job (the "Refuse to republish" step). Replacing the third-party action reduces the supply-chain surface for a job that already holds write permissions and secrets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 166 - 179, Replace the softprops/action-gh-release step with a shell step using the runner’s existing gh CLI, preserving tag gating, release-note generation, prerelease detection from github.ref_name, all four uploaded files, and unmatched-file failure behavior. Reuse the job’s existing release context and authentication setup from the “Refuse to republish” step.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 135-136: Update the actions/checkout@v4 step in the CI job to set
persist-credentials to false, ensuring the GitHub token is not retained in the
repository configuration while preserving the checkout behavior.
In @.github/workflows/release.yml:
- Around line 76-91: Restrict caching in the release job to trusted,
workflow-specific entries: update the setup-node pnpm cache and
Swatinem/rust-cache configuration so neither can restore cache data shared with
lower-trust workflows, using workflow/ref-scoped keys where supported or
disabling these caches and retaining dependency installation correctness.
In `@apps/desktop/src/main/index.ts`:
- Around line 75-79: Guard desktop startup in the main process before
initializing serveQueries so duplicate ow launches for the same project are
rejected via app.requestSingleInstanceLock() or an equivalent single-instance
mechanism. Ensure the duplicate process exits without invoking serveQueries,
preserving the first process’s shared query token and listener.
In `@packages/access/src/socket.ts`:
- Around line 314-321: Import StringDecoder from node:string_decoder and create
a separate decoder per connection. In packages/access/src/socket.ts lines
314-321, update the askRunningApp data handler to append decoder.write(chunk);
apply the same change in packages/access/src/socket.ts lines 213-218 within the
serveQueries connection handler, preserving each handler’s existing buffering
and termination behavior.
- Around line 254-261: Update the token cleanup around the server error and
close handlers in serveQueries to retain the exact token value written by that
server and remove the shared token file only when its current contents still
match that value. Preserve the existing cleanup and onError behavior while
preventing a failed second server from deleting the first server’s token.
In `@packages/cli/tests/main.spec.ts`:
- Around line 192-195: Update the test block containing “prints the page when no
application is listening” to isolate application-data resolution by setting
APPDATA and, on non-Windows platforms, HOME to a temporary fixture directory
before invoking main. Restore the environment after the test so
defaultAppDataDir and endpointDir cannot create directories in the developer’s
real profile.
In `@README.md`:
- Around line 42-44: Update the fenced code block containing “npx open-wiki
init” in README.md to specify the bash language identifier, preserving the
command and surrounding documentation unchanged.
In `@scripts/build-cli.mjs`:
- Around line 16-23: Update the esbuild configuration in the CLI build script to
use an output directory with code splitting enabled, so the dynamic-import graph
is emitted as lazy chunks instead of being inlined. Ensure the generated entry
filename remains aligned with the packages/cli package.json bin.ow target,
updating that target if necessary.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 150-156: Pin the package version in the global installation
command within the “claude plugin validate --strict” workflow step instead of
installing the registry’s latest release. Use the confirmed intended
`@anthropic-ai/claude-code` version, and preserve the existing skip behavior and
strict validation command; update dependency automation configuration only if
the repository already uses it for this package.
In @.github/workflows/release.yml:
- Around line 166-179: Replace the softprops/action-gh-release step with a shell
step using the runner’s existing gh CLI, preserving tag gating, release-note
generation, prerelease detection from github.ref_name, all four uploaded files,
and unmatched-file failure behavior. Reuse the job’s existing release context
and authentication setup from the “Refuse to republish” step.
In `@eslint.config.js`:
- Around line 20-23: Remove the redundant "apps/desktop/build/**" ignore entry
from the ESLint configuration, keeping the broader "**/build/**" pattern as the
single rule governing generated bundles.
In `@packages/access/tests/socket.spec.ts`:
- Around line 137-181: Add a large page containing repeated multi-byte
characters to the end-to-end test around serveQueries, askRunningApp, and
handleRequest, keeping the payload below MAX_LINE_BYTES but larger than one
socket chunk (for example, about 32 KiB). Assert that the over-wire response
equals the standalone response byte for byte, while preserving the existing
server cleanup.
In `@plugins/open-wiki/README.md`:
- Around line 5-8: Update the fenced code block in the README to include a
language tag, using text or another appropriate plain-text language, while
preserving the commands unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 88e08c05-adc2-4202-a226-7408edc09a24
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
.claude-plugin/marketplace.json.github/workflows/ci.yml.github/workflows/release.yml.gitignoreREADME.mdapps/desktop/build-resources/installer.nshapps/desktop/package.jsonapps/desktop/src/main/channels.tsapps/desktop/src/main/index.tsapps/desktop/src/main/ipc.tsapps/desktop/src/main/preload.tsapps/desktop/src/main/resources.tsapps/desktop/tests/resources.spec.tseslint.config.jspackage.jsonpackages/access/package.jsonpackages/access/src/config/app-dir.tspackages/access/src/config/secrets.tspackages/access/src/socket.tspackages/access/tests/socket.spec.tspackages/cli/package.jsonpackages/cli/src/cli.tspackages/cli/src/install.tspackages/cli/src/main.tspackages/cli/tests/install.spec.tspackages/cli/tests/main.spec.tspackages/cli/tests/release.spec.tspackages/cli/tsconfig.jsonpackages/cli/types/release-scripts.d.tsplans/open-wiki.mdplugins/open-wiki/.claude-plugin/plugin.jsonplugins/open-wiki/README.mdplugins/open-wiki/commands/ow-init.mdplugins/open-wiki/hooks/hooks.jsonscripts/build-cli.mjsscripts/build-main.mjsscripts/ci/check-plugin.mjsscripts/ci/package-manifests.mjsscripts/ci/release-version.mjs
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Set persist-credentials: false on this checkout.
This job later runs npm i -g @anthropic-ai/claude-code``, third-party code with a postinstall path. By default, actions/checkout leaves the `GITHUB_TOKEN` credential in `.git/config`. If the installed package or one of its dependencies is compromised, it can read and exfiltrate that token.
Add persist-credentials: false, since this job does not need to push or fetch with the token afterward.
🔒 Proposed fix
- uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| steps: | |
| - uses: actions/checkout@v4 | |
| steps: | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 136-136: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 135 - 136, Update the
actions/checkout@v4 step in the CI job to set persist-credentials to false,
ensuring the GitHub token is not retained in the repository configuration while
preserving the checkout behavior.
Source: Linters/SAST tools
| - uses: pnpm/action-setup@v4 | ||
|
|
||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 22 | ||
| cache: pnpm | ||
| # Without this, `setup-node` writes no .npmrc and NODE_AUTH_TOKEN is | ||
| # read by nothing — the publish below would be unauthenticated. | ||
| registry-url: https://registry.npmjs.org | ||
|
|
||
| - run: pnpm install --frozen-lockfile | ||
|
|
||
| # The installer carries them; `vendor/ffmpeg/` is gitignored and fetched | ||
| # with hash verification, and the recorder is the one Rust crate. | ||
| - uses: dtolnay/rust-toolchain@stable | ||
| - uses: Swatinem/rust-cache@v2 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Address the cache-poisoning risk flagged on the caching actions.
Static analysis flags both actions/setup-node's cache: pnpm and Swatinem/rust-cache@v2 for cache-poisoning risk. This job runs on a tag push with contents: write and id-token: write permissions and holds the npm token and the signing certificate. If a cache entry with a shared restore key were populated by a lower-trust workflow run (for example, a regular PR-triggered CI run), this privileged release job could restore it and execute code from a poisoned dependency cache.
Consider scoping the cache key to this workflow only (for example a key that includes the workflow name or github.ref), or disabling the built-in cache here and installing dependencies without relying on a cache shared with other workflows.
🧰 Tools
🪛 zizmor (1.28.0)
[error] 78-78: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): this step
(cache-poisoning)
[error] 91-91: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 76 - 91, Restrict caching in the
release job to trusted, workflow-specific entries: update the setup-node pnpm
cache and Swatinem/rust-cache configuration so neither can restore cache data
shared with lower-trust workflows, using workflow/ref-scoped keys where
supported or disabling these caches and retaining dependency installation
correctness.
Source: Linters/SAST tools
| // 9.14 — the CLI asks here rather than starting a process, when this | ||
| // window already has the project open. Read and validate only; the socket | ||
| // never carries a write. | ||
| const queries = projectRoot ? serveQueries(projectRoot) : null; | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for single-instance-lock or duplicate-project guards in the desktop main process.
rg -n "requestSingleInstanceLock|second-instance|projectRoot" apps/desktop/src/main/index.tsRepository: protonspy/open-wiki
Length of output: 433
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- desktop main process ---'
ast-grep outline apps/desktop/src/main/index.ts
sed -n '1,180p' apps/desktop/src/main/index.ts
printf '%s\n' '--- socket implementation and usages ---'
rg -n -A35 -B15 "function serveQueries|const serveQueries|serveQueries\\(" packages/access apps
printf '%s\n' '--- single-instance APIs across desktop sources ---'
rg -n -i "requestSingleInstanceLock|hasSingleInstanceLock|second-instance|single.?instance|makeSingleInstance" apps/desktop packagesRepository: protonspy/open-wiki
Length of output: 24875
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- socket implementation ---'
sed -n '1,130p' packages/access/src/socket.ts
sed -n '180,330p' packages/access/src/socket.ts
printf '%s\n' '--- project resolution and desktop launch wiring ---'
sed -n '1,220p' apps/desktop/src/main/project.ts
rg -n -i "open-wiki\\.exe|ow\\.exe|requestSingleInstanceLock|second-instance|spawn|execFile|protocol|argv" --glob '!**/node_modules/**' .
printf '%s\n' '--- desktop package and entrypoint metadata ---'
fd -i 'package.json' apps packages | xargs -r rg -n -A8 -B3 '"(main|bin|scripts)"'Repository: protonspy/open-wiki
Length of output: 33998
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- token-file helper and remaining socket lifecycle ---'
rg -n -A25 -B10 "function tokenFile|const tokenFile|export function tokenFile|server\\.on\\(\"error\"|server\\.on\\(\"close\"" packages/access/src/socket.ts
printf '%s\n' '--- CLI shim and installer launch commands ---'
fd -i -t f 'ow*' . apps packages scripts | sort
rg -n -A12 -B8 "resources/ow|ELECTRON_RUN_AS_NODE|open-wiki\\.exe|--project" apps packages scripts
printf '%s\n' '--- deterministic token-file race model ---'
python3 - <<'PY'
from pathlib import Path
from tempfile import TemporaryDirectory
with TemporaryDirectory() as d:
token = Path(d) / "project.token"
token.write_text("first-process-token")
assert token.read_text() == "first-process-token"
# serveQueries writes the new token before the second process calls listen.
token.write_text("second-process-token")
assert token.read_text() == "second-process-token"
# The second server's bind error invokes rmSync(file, { force: true }).
token.unlink(missing_ok=True)
assert not token.exists()
print("second bind failure removes the shared token path; first listener has no discoverable token")
PYRepository: protonspy/open-wiki
Length of output: 38392
Protect the shared query token from duplicate desktop processes. apps/desktop/src/main/index.ts has no app.requestSingleInstanceLock() or equivalent guard. Each ow launch can start another open-wiki.exe process for the same project. When the second serveQueries call fails to bind, its error handler removes the shared token file while the first server continues listening. Add single-instance handling or make bind failure remove only the token created by that server.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/index.ts` around lines 75 - 79, Guard desktop startup
in the main process before initializing serveQueries so duplicate ow launches
for the same project are rejected via app.requestSingleInstanceLock() or an
equivalent single-instance mechanism. Ensure the duplicate process exits without
invoking serveQueries, preserving the first process’s shared query token and
listener.
| const file = tokenFile(projectRoot, options); | ||
| server.on("error", (error: Error) => { | ||
| // Whatever the reason, this window is not the one answering. Take the | ||
| // token back so a client reads nothing rather than the wrong thing. | ||
| rmSync(file, { force: true }); | ||
| onError(error); | ||
| }); | ||
| server.on("close", () => rmSync(file, { force: true })); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A failed second server removes the first window's token.
The token file path depends only on projectRoot, so every window for the same project uses one file. Consider two windows on the same project. The second serveQueries overwrites the token file at Line 268, then listen fails with EADDRINUSE. The error handler removes the file. The first window keeps listening, but its token no longer exists on disk, so askRunningApp returns null for every later query. The socket optimisation stays off until the first window restarts.
Record the token this server wrote and remove the file only when the contents still match.
🐛 Proposed fix to scope the token removal
const file = tokenFile(projectRoot, options);
+ // Only ours. Another window's token at this path is not ours to remove:
+ // taking it back would silently disable a healthy window's socket.
+ const dropToken = (): void => {
+ try {
+ if (readFileSync(file, "utf8") !== token) return;
+ } catch {
+ return;
+ }
+ rmSync(file, { force: true });
+ };
server.on("error", (error: Error) => {
- // Whatever the reason, this window is not the one answering. Take the
- // token back so a client reads nothing rather than the wrong thing.
- rmSync(file, { force: true });
+ dropToken();
onError(error);
});
- server.on("close", () => rmSync(file, { force: true }));
+ server.on("close", () => dropToken());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/access/src/socket.ts` around lines 254 - 261, Update the token
cleanup around the server error and close handlers in serveQueries to retain the
exact token value written by that server and remove the shared token file only
when its current contents still match that value. Preserve the existing cleanup
and onError behavior while preventing a failed second server from deleting the
first server’s token.
| socket.on("data", (chunk: Buffer) => { | ||
| buffered += chunk.toString("utf8"); | ||
| // A peer that answers with megabytes and no newline is not the | ||
| // application, and holding it all first is how it wins anyway. | ||
| if (buffered.length > MAX_LINE_BYTES) { | ||
| done(null); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Incremental UTF-8 decoding without StringDecoder on both sides of the socket. Both data handlers call chunk.toString("utf8") on each chunk and append the result to a string. Socket chunk boundaries fall at arbitrary byte offsets, so a multi-byte UTF-8 character split across two chunks decodes to U+FFFD on both sides of the split. Import StringDecoder from node:string_decoder once and give each connection its own decoder instance.
packages/access/src/socket.ts#L314-L321: replacebuffered += chunk.toString("utf8")withbuffered += decoder.write(chunk)in theaskRunningAppdata handler. This site returns the corrupted body asok, becausemacOfcovers only the nonce, so the CLI prints mojibake as wiki content.packages/access/src/socket.ts#L213-L218: replacebuffered += chunk.toString("utf8")withbuffered += decoder.write(chunk)in theserveQueriesconnection handler. This site failsJSON.parseand destroys the connection when a slug carries non-ASCII characters.
📍 Affects 1 file
packages/access/src/socket.ts#L314-L321(this comment)packages/access/src/socket.ts#L213-L218
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/access/src/socket.ts` around lines 314 - 321, Import StringDecoder
from node:string_decoder and create a separate decoder per connection. In
packages/access/src/socket.ts lines 314-321, update the askRunningApp data
handler to append decoder.write(chunk); apply the same change in
packages/access/src/socket.ts lines 213-218 within the serveQueries connection
handler, preserving each handler’s existing buffering and termination behavior.
| it("prints the page when no application is listening", async () => { | ||
| expect(await main(["read", "fenix"], root)).toBe(0); | ||
| expect(stdout()).toContain("Fenix is a rebuild."); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This test touches the real application data directory.
main calls askRunningApp without SocketOptions, so the socket resolves defaultAppDataDir() and endpointDir calls mkdirSync on the real user profile. Running the suite creates %APPDATA%/open-wiki/sockets on the developer machine and in CI. The assertions still pass, because the missing token file makes askRunningApp return null.
Set APPDATA (and HOME off Windows) to a temporary directory for this block, so the test stays inside its own fixture.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/tests/main.spec.ts` around lines 192 - 195, Update the test
block containing “prints the page when no application is listening” to isolate
application-data resolution by setting APPDATA and, on non-Windows platforms,
HOME to a temporary fixture directory before invoking main. Restore the
environment after the test so defaultAppDataDir and endpointDir cannot create
directories in the developer’s real profile.
| ``` | ||
| npx open-wiki init | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the fenced code block.
markdownlint (MD040) flags this fence for missing a language identifier.
📝 Proposed fix
-```
+```bash
npx open-wiki init</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 42-42: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 42 - 44, Update the fenced code block containing “npx
open-wiki init” in README.md to specify the bash language identifier, preserving
the command and surrounding documentation unchanged.
Source: Linters/SAST tools
| * **Nothing stays external.** `open-wiki` declares no runtime dependencies: | ||
| * `@open-wiki/access` and `@open-wiki/mcp` are workspace packages that are | ||
| * never published, and declaring them would make every install 404. So the | ||
| * bundle has to carry the whole graph — including the source adapters' | ||
| * libraries, which are reached through a dynamic `import()` and would | ||
| * otherwise be an `ERR_MODULE_NOT_FOUND` the first time somebody drops a PDF | ||
| * into `raw/`. The dynamic import still buys what it was for: esbuild splits | ||
| * those out of the startup path, so a hook run does not parse them. |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Does esbuild code splitting require outdir instead of outfile, and does a non-split dynamic import get inlined into the single bundle output?
💡 Result:
Yes, esbuild requires the use of --outdir instead of --outfile when code splitting is enabled [1][2]. Attempting to use --outfile with code splitting will result in an error [1]. Because code splitting produces multiple output files (the entry point bundle plus additional shared chunks), a directory is necessary to store these files [2][3]. Regarding dynamic imports, the behavior depends on whether code splitting is enabled: 1. If code splitting is enabled (--splitting=true), dynamic imports are treated as additional entry points [4][3]. This means the dynamically imported module will be bundled into its own separate chunk rather than being inlined into the main bundle [5][3]. 2. If code splitting is disabled, dynamic imports are typically bundled into the single output file [5]. In this case, esbuild will wrap the dynamic module in a way that allows it to be loaded (often using Promise.resolve and a require-style wrapper) [5][6], keeping everything within the single bundle output [5]. In summary, enabling code splitting automatically changes how dynamic imports are handled, moving them from inlined code to separate, lazily-loaded chunks [5][4][7].
Citations:
- 1: Support both outfile and splitting at the same time evanw/esbuild#1666
- 2: https://www.codestudy.net/blog/esbuild-not-bundling-files-into-a-single-output/
- 3: [Feature] Code splitting on async import() statements. evanw/esbuild#16
- 4: https://github.com/evanw/esbuild/blob/d926cebcc3fffce6e12aa345ae741211353fe6e2/docs/architecture.md
- 5: [Question] Dynamic imports evanw/esbuild#4175
- 6: Provide an option to transform dynamic import to Promise.resolve(() => require(...)) evanw/esbuild#1084
- 7: https://makandracards.com/makandra/595482-code-splitting-esbuild-caveats-setup
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- build script ---'
sed -n '1,140p' scripts/build-cli.mjs
printf '%s\n' '--- CLI package metadata ---'
pkg=$(git ls-files | grep -E '(^|/)packages/cli/package\.json$' | head -n 1 || true)
if [ -n "$pkg" ]; then
cat -n "$pkg"
else
echo 'packages/cli/package.json not found'
fi
printf '%s\n' '--- dynamic imports and build references ---'
rg -n -C 3 'import\(|splitting|outfile|outdir|build-cli|bin|ow\.mjs|build/' \
scripts packages package.json 2>/dev/null || true
printf '%s\n' '--- relevant tracked files ---'
git ls-files | grep -E '(^|/)(build-cli\.mjs|main\.ts|package\.json)$' | head -80Repository: protonspy/open-wiki
Length of output: 37425
🌐 Web query:
site:esbuild.github.io/api code splitting outdir outfile dynamic import bundle splitting false
💡 Result:
Code splitting and bundling are fundamentally linked in modern build tools like esbuild. To perform code splitting—such as splitting shared code into separate chunks or handling dynamic imports—the bundler must first be enabled [1]. When bundling is disabled (bundle: false), the tool does not resolve or inline imported dependencies; instead, it treats files individually, which makes code splitting impossible because the tool cannot analyze or restructure the dependency graph to create shared chunks [1]. Regarding output configuration, tools like esbuild enforce specific rules when code splitting is active: 1. Outdir vs. Outfile: When code splitting is enabled (which happens automatically when you have multiple entry points or use dynamic imports), you must use the outdir option to specify an output directory [1]. The outfile option cannot be used because code splitting generates multiple files, whereas outfile is designed to produce a single, specific file [1]. 2. Bundling Dependency: Code splitting is a feature of the bundling process [1]. If you attempt to disable bundling (bundle: false), the engine will not perform the recursive dependency analysis required to identify shared modules or handle dynamic imports, effectively disabling the code splitting mechanism [1]. In summary, if you need code splitting or dynamic imports, you must enable bundling and use outdir instead of outfile to accommodate the multiple output files generated by the process [1].
Citations:
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- esbuild availability and declared version ---'
command -v esbuild || true
node -e 'try { console.log(require.resolve("esbuild")) } catch { console.log("esbuild module unavailable") }' 2>/dev/null || true
rg -n -C 2 '"esbuild"|esbuild@' package.json pnpm-lock.yaml package-lock.json yarn.lock 2>/dev/null || true
printf '%s\n' '--- read-only configuration verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
p = Path("scripts/build-cli.mjs").read_text()
outfile = re.search(r'\boutFile:\s*', p, re.I)
outdir = re.search(r'\boutdir:\s*', p, re.I)
splitting = re.search(r'\bsplitting\s*:\s*true\b', p)
dynamic = "await import(" in Path("packages/cli/src/main.ts").read_text()
print({
"outfile_configured": bool(outfile),
"outdir_configured": bool(outdir),
"splitting_true": bool(splitting),
"cli_dynamic_import": dynamic,
})
PYRepository: protonspy/open-wiki
Length of output: 2462
🏁 Script executed:
node - <<'JS'
const { build } = require("esbuild");
const plugin = {
name: "virtual",
setup(build) {
build.onResolve({ filter: /^entry$/ }, () => ({ path: "entry", namespace: "v" }));
build.onResolve({ filter: /^lazy$/ }, () => ({ path: "lazy", namespace: "v" }));
build.onLoad({ filter: /.*/, namespace: "v" }, ({ path }) => ({
contents: path === "entry"
? 'export async function run() { return import("lazy"); }'
: 'export const value = 42;',
loader: "js",
}));
},
};
(async () => {
const base = {
stdin: { contents: 'export async function run() { return import("lazy"); }', sourcefile: "entry.js", loader: "js" },
bundle: true,
format: "esm",
plugins: [plugin],
write: false,
logLevel: "silent",
};
const single = await build({ ...base, outfile: "ow.mjs" });
const singleText = single.outputFiles[0].text;
console.log("single_output_files:", single.outputFiles.map((f) => f.path));
console.log("single_contains_dynamic_import:", /\bimport\s*\(/.test(singleText));
console.log("single_contains_inlined_value:", singleText.includes("42"));
try {
await build({ ...base, splitting: true, outfile: "ow.mjs" });
console.log("outfile_with_splitting: unexpectedly accepted");
} catch (error) {
console.log("outfile_with_splitting:", error.errors?.[0]?.text ?? String(error));
}
const split = await build({ ...base, splitting: true, outdir: "build" });
console.log("split_output_files:", split.outputFiles.map((f) => f.path));
})();
JSRepository: protonspy/open-wiki
Length of output: 466
Enable esbuild code splitting for lazy imports. scripts/build-cli.mjs uses outfile without splitting: true, so esbuild emits one ow.mjs file and inlines the dynamic-import graph. The import defers execution, but it does not emit a separate startup chunk. If this cold-start goal is required, use outdir with splitting: true and preserve or update packages/cli/package.json's bin.ow to match the generated entry filename.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/build-cli.mjs` around lines 16 - 23, Update the esbuild configuration
in the CLI build script to use an output directory with code splitting enabled,
so the dynamic-import graph is emitted as lazy chunks instead of being inlined.
Ensure the generated entry filename remains aligned with the packages/cli
package.json bin.ow target, updating that target if necessary.
…nifests `claude plugin validate --strict` failed the moment CI could actually report it — which is the point of removing `continue-on-error`. A marketplace with no description is a warning, and `--strict` makes a warning an error. Two things follow it into `check-plugin.mjs` and CI: - The floor now refuses a marketplace with no description too. That script exists because "a check that silently passes when its tool is missing is not a check", and a floor that misses what the tool catches is the same failure one level down. - The marketplace manifest and the plugin manifest are different files, and validating the first never opens the second. CI runs both. Verified locally against the real CLI: both validate clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D3VYWWTZtEE2NxPiksKsAK
1 participant
The last group in
plans/open-wiki.md. Every box in the plan is now ticked.What changed
9.14 — cold start (TDD). The CLI is one bundled file, and when the desktop
application already has the project open,
ow readand the validate path ask itover a local socket instead of starting a process — which a
PreToolUsehookdoes on every page write. The socket carries read and validate and never
write: a write over it would be a second writer into a project the application
has open, with no snapshot of its own, logged as something the application did.
Both directions are authenticated (a token in the app's data directory, an HMAC
over the client's nonce coming back), because the endpoint name is a hash of a
directory path — obscure, not secret — and whatever holds it decides what gets
printed into an agent's context. Absence is not failure: nothing listening means
the CLI runs standalone.
Red was observed first — seven assertions failing against signature-only stubs.
10.1 — the installer. NSIS, per-user, ffmpeg and
recorder.exeembedded, andan
owshim on PATH. With no arguments it opens the application scoped to theshell's directory; with a subcommand it runs the CLI headless through
ELECTRON_RUN_AS_NODE=1, which is the only Node an installer promising noexternal dependency can count on. The shim gets its own
bin/directory and onlythat goes on PATH.
10.2 / 10.3 — releasing from a tag. One version across the installer and the
npm package, checked before anything is built and against both registries. The
reversible step (the GitHub release) runs before the permanent one (npm), a
prerelease publishes to
next, and provenance is on withid-token: write.10.4 — winget and Scoop manifests, generated from the tag and the hash of the
installer being released, attached to it. Scoop runs the NSIS setup rather than
pretending the download is portable.
10.5 — the README, with the recording notice, the responsibility to inform
participants, what SmartScreen on an unsigned build means, and what committing a
wiki puts in front of everyone with repository access.
10.6 — the Claude Code plugin. The hooks and
/ow-init, never the skills(
adr:0015— the convention has one home and it is the project) and never a.mcp.json.HOOK_MATCHERSis one declaration the plugin is tested against, sothe two hook files cannot drift.
How it was verified
pnpm test— 1059 tests across five packages.pnpm test:coverageclears the76% floor per package.
pnpm run typecheck,pnpm lint,prettier --checkandscc validateare all clean.The built artifacts were run, not just built: the bundled
ow.mjsscaffoldsa project, checks it, and reads a page back. That is what found the duplicated
shebang and the
requirestub, neither of which any test would have.What the reviews found, and what it cost
code-reviewandsecurity-reviewboth ran before this was pushed. The secondcommit closes what they found — the packaged application could not start, the
npm package could not be installed, the shim pointed at a file that is never
packaged, the socket trusted whoever answered it, and the release could publish
half of itself. Details are in that commit's message.
What is not verified here
No packaging run happens in CI, so
electron-builder --winand the NSISscript are exercised only by a tag push. The installer script now uses nothing
beyond the headers NSIS itself ships, which was the largest of the unverifiable
risks; the rest is a first release's job to confirm.
vars.FFMPEG_SHA256is on the release critical path and the workflow now failsearly and by name when it is unset.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
ow read <slug>to retrieve wiki pages from the command line.Documentation
Quality