feat: colorful oh-my-pi logo and optional omp notify hook - #389
feat: colorful oh-my-pi logo and optional omp notify hook#389Gyarados4157 wants to merge 4 commits into
Conversation
Add multi-color brand logos for oh-my-pi (omp) and pi so the dashboard no longer falls back to mono placeholders. Install a managed omp extension on init that fires notify.cjs --source=omp after turn_end/agent_end while keeping the existing passive session scanner as the source of truth.
github-actions
Bot
added
documentation
📝 WalkthroughWalkthroughThe changes add managed oh-my-pi notify-extension lifecycle handling, integrate it with setup, status, and uninstall commands, update dashboard provider branding, and document near-real-time synchronization. Changesoh-my-pi notify integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant InitCommand
participant OmpHook
participant NotifyExtension
participant NotifyScript
User->>InitCommand: run integration setup
InitCommand->>OmpHook: probe or upsert notify extension
OmpHook->>NotifyExtension: write managed extension
NotifyExtension->>NotifyScript: spawn lifecycle notification
NotifyScript->>User: synchronize token data
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
test/omp-hook.test.js (1)
88-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting
removeOmpHookleaves the unmanaged file alone.The non-clobber path is covered for
upsertOmpHookbut not forremoveOmpHook'sskippedReason === "unmanaged"branch, whichuninstall.jsline 202 depends on.🤖 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 `@test/omp-hook.test.js` around lines 88 - 113, Add a test covering removeOmpHook’s unmanaged-extension path: create an unmanaged extension file, invoke removeOmpHook with the same temporary environment setup, assert skippedReason is "unmanaged", and verify the file content remains unchanged.src/commands/init.js (1)
59-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
removeOmpHookimport.
removeOmpHookis only listed in the destructuredrequire, whileupsertOmpHookandprobeOmpHookStateare the only symbols used insrc/commands/init.js.🤖 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 `@src/commands/init.js` around lines 59 - 63, Remove removeOmpHook from the destructured require in init.js, leaving only the used upsertOmpHook and probeOmpHookState imports.
🤖 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 `@dashboard/src/lib/provider-display.js`:
- Line 4: Move the user-facing "oh-my-pi" label from the provider mapping in
provider-display.js into copy.csv, then replace the hardcoded value with the
existing content lookup mechanism. Preserve the provider mapping behavior while
ensuring the label is sourced through dashboard copy content.
In `@README.md`:
- Line 227: Update the oh-my-pi entries in README.md lines 227-227 and
README.zh-CN.md lines 182-182 to distinguish passive session reading from the
optional near-real-time notify extension; state that tokentracker init installs
the extension and lifecycle commands manage it, and remove oh-my-pi from any
claim that passive-reader integrations install nothing. Apply the equivalent
clarification in Chinese in README.zh-CN.md.
In `@src/commands/init.js`:
- Around line 652-667: Update the init integration flow around
applyIntegrationSetup to accept a dryRun parameter in its setup object, sourced
from the caller’s opts.dryRun value. Replace the undeclared opts reference in
the oh-my-pi detection branch with that passed dryRun flag, preserving the
existing dry-run probe behavior.
In `@src/lib/omp-hook.js`:
- Around line 11-38: The local resolveOmpHome and resolveOmpAgentDir
implementations in omp-hook.js duplicate divergent rollout behavior. Remove or
replace these helpers with the shared rollout resolvers, preserving
expandHomePath, null handling, and decidePiCodingAgentDirOwner semantics; update
resolveOmpExtensionsDir and its callers, including upsertOmpHook, to guard when
the shared OMP home/agent directory is null so install paths cannot be created
from invalid values.
- Around line 240-249: Update the extension removal flow around the unlink call
in the uninstall logic so it only returns removed: true after fs.unlink
succeeds. Capture unlink failures instead of swallowing them, and return an
appropriate unsuccessful result while preserving the existing unmanaged-file
skip behavior.
---
Nitpick comments:
In `@src/commands/init.js`:
- Around line 59-63: Remove removeOmpHook from the destructured require in
init.js, leaving only the used upsertOmpHook and probeOmpHookState imports.
In `@test/omp-hook.test.js`:
- Around line 88-113: Add a test covering removeOmpHook’s unmanaged-extension
path: create an unmanaged extension file, invoke removeOmpHook with the same
temporary environment setup, assert skippedReason is "unmanaged", and verify the
file content remains 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ced0860a-d7b2-43b7-aab2-903930b2b9e1
⛔ Files ignored due to path filters (2)
dashboard/public/brand-logos/omp.svgis excluded by!**/*.svgdashboard/public/brand-logos/pi.svgis excluded by!**/*.svg
📒 Files selected for processing (11)
README.mdREADME.zh-CN.mddashboard/src/lib/provider-display.jsdashboard/src/lib/provider-display.test.jsdashboard/src/ui/dashboard/components/ProviderIcon.jsxdashboard/src/ui/dashboard/components/ProviderIcon.test.jsxsrc/commands/init.jssrc/commands/status.jssrc/commands/uninstall.jssrc/lib/omp-hook.jstest/omp-hook.test.js
| const SPECIAL_PROVIDER_NAMES = { | ||
| anythingllm: "AnythingLLM", | ||
| claudescience: "Claude Science", | ||
| omp: "oh-my-pi", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move the new display label into the dashboard copy source.
"oh-my-pi" is user-facing text and is hardcoded in dashboard/src/lib/provider-display.js. Add it to dashboard/src/content/copy.csv and use the existing content lookup instead.
🤖 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 `@dashboard/src/lib/provider-display.js` at line 4, Move the user-facing
"oh-my-pi" label from the provider mapping in provider-display.js into copy.csv,
then replace the hardcoded value with the existing content lookup mechanism.
Preserve the provider mapping behavior while ensuring the label is sourced
through dashboard copy content.
Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lib/omp-hook.js`:
- Around line 207-218: Update the extension ownership check around
isManagedOmpExtension in the hook flow: remove the broad tokentracker text match
and treat empty or markerless content as unmanaged, preserving it instead of
overwriting it. In the fs.readFile catch, continue only for ENOENT; return the
established skipped or error result for all other read failures.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3fe201ae-f024-48a0-bc76-1001a4d0b80e
📒 Files selected for processing (1)
src/lib/omp-hook.js
xiufengsun
left a comment
xiufengsun
left a comment
There was a problem hiding this comment.
Thanks for adding the oh-my-pi branding and near-real-time notification integration. The direction is useful, but I reproduced several correctness and data-safety blockers at exact head fb0621a on top of latest main 03f924e:
-
OMP detection makes normal init crash after partial setup. applyIntegrationSetup does not receive an opts parameter, but the new OMP branch reads opts.dryRun in src/commands/init.js. With an OMP directory present, tokentracker init exits with ReferenceError: opts is not defined after earlier setup work has already run. Please thread the dry-run state into this function and add an init integration regression test.
-
User-authored extensions can be overwritten and deleted. In src/lib/omp-hook.js, both upsert and removal treat any same-named file containing the word tokentracker as managed, even when it does not contain MARKER. I reproduced a custom tokentracker-notify.ts being overwritten by upsertOmpHook and then deleted by removeOmpHook. Ownership must be marker-only: a file without the exact managed marker must never be overwritten or removed. Please add regressions for an unmanaged file that mentions tokentracker.
-
removeOmpHook swallows unlink failures and still returns removed: true. Please report removal only after unlink succeeds, and preserve/report the real error or an accurate skipped result otherwise.
-
The hook path resolver diverges from the existing passive scanner. It ignores the passed home plus existing PI_CONFIG_DIR and PI_CODING_AGENT_DIR resolution semantics, so status/init/uninstall can target a different OMP installation from the one whose sessions TokenTracker scans. Please reuse the canonical resolver and add path-parity tests, including custom HOME/config and Windows/WSL cases.
-
git diff --check currently fails at dashboard/src/ui/dashboard/components/ProviderIcon.test.jsx:59 because of a new blank line at EOF.
The existing tests and green CI do not cover the init integration path, marker ownership with tokentracker text, unlink failure, or resolver parity. Please address these before merge.
- Thread dryRun into applyIntegrationSetup so init no longer throws ReferenceError when ~/.omp is present. - Enforce marker-only ownership for upsert/remove so user-authored tokentracker-notify extensions are never overwritten or deleted. - Report unlink failures instead of claiming removed: true. - Reuse rollout resolveOmpHome/resolveOmpAgentDir for path parity with the passive scanner (HOME, PI_CONFIG_DIR, ~ expansion, Win/WSL). - Add init integration, marker, unlink, and path-parity regressions; fix trailing blank line in ProviderIcon.test.jsx.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dashboard/src/ui/dashboard/components/ProviderIcon.test.jsx (1)
52-57: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the Pi icon height too.
The test verifies
widthbut notheight, so a regression in the component’s height handling would go undetected.expect(icon).not.toBeNull(); expect(icon).toHaveAttribute("width", "18"); + expect(icon).toHaveAttribute("height", "18");🤖 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 `@dashboard/src/ui/dashboard/components/ProviderIcon.test.jsx` around lines 52 - 57, Update the “renders the multi-color Pi brand logo” test for ProviderIcon to also assert that the selected Pi image has a height attribute of “18”, alongside the existing width assertion.
🤖 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.
Outside diff comments:
In `@dashboard/src/ui/dashboard/components/ProviderIcon.test.jsx`:
- Around line 52-57: Update the “renders the multi-color Pi brand logo” test for
ProviderIcon to also assert that the selected Pi image has a height attribute of
“18”, alongside the existing width assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dc638d23-1138-4f3c-83a7-47f40b37c8f9
📒 Files selected for processing (4)
dashboard/src/ui/dashboard/components/ProviderIcon.test.jsxsrc/commands/init.jssrc/lib/omp-hook.jstest/omp-hook.test.js
|
Thanks for the careful review and for reproducing these at 1.
|
xiufengsun
left a comment
xiufengsun
left a comment
There was a problem hiding this comment.
Thanks for the update. I re-reviewed exact head 4e93f46bf0fd7b166b6527bf398d1d7cde84d214 on top of latest main (f9b23e10c22879f74c332a7899be202d5c7da3b9). The five blockers from the previous review are fixed, and the normal init/status/uninstall flow now works. I am still requesting changes for the remaining file-ownership issues:
-
src/lib/omp-hook.js:203-227still has a check-then-write race. IfreadFile()returnsENOENTand another process creates a markerlesstokentracker-notify.tsbeforewriteFile(), TokenTracker overwrites the user's file. A deterministic race repro producedwritten=true,userFilePreserved=false. -
src/lib/omp-hook.js:245-265has the corresponding check-then-unlink race. If the managed file is replaced with a user-authored file after the marker check but beforeunlink(), uninstall deletes the replacement. The deterministic repro producedremoved=true,userReplacementStillExists=false.
Please make creation/update/removal preserve ownership atomically (or validate stable file identity through the mutation) and add regression tests that replace/create the path between the check and mutation.
-
src/commands/uninstall.js:200-204collapsesunlink-failedandextension-read-failedinto “no change”, then removes the notify handler/runtime. Please surface the failure and residual extension path so users are not left with a silent orphan hook. -
The README currently says OMP gets an automatically installed notify extension, while the passive-reader paragraph still says nothing is installed into OMP. Please make the English and Chinese documentation accurately describe the write behavior; “optional” is also misleading unless users can opt out.
Validation on the integrated latest-main state: targeted OMP/provider tests passed (22/22), real isolated-HOME init → status → uninstall passed, git diff --check passed, and the complete local gate passed after building the ignored dashboard artifact (1992/1992 plus locale, guardrails, and production build). Please ping me after the ownership and uninstall-contract issues are addressed.
- Create the managed notify extension with exclusive open (wx) so a concurrent user file cannot be clobbered after an ENOENT check. - Update only through an opened managed inode with path-identity checks. - Remove by rename-to-staging then marker verify, restoring unmanaged replacements instead of unlinking them. - Surface unlink/read/identity failures and residual paths in uninstall. - Document that init writes the managed OMP extension (not optional / not a pure passive-only install) in EN and ZH README. - Add deterministic create/remove race regressions and uninstall format coverage.
|
Thanks for the re-review and the deterministic race repros — addressed in 1. Create race (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/lib/omp-hook.js (1)
264-333: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the exclusive-create block.
The
open(path, "wx")→writeFile→close→probeOmpHookStatesequence appears verbatim twice (Lines 265-278 and 301-309). A smallcreateExclusive()helper keeps the two branches from drifting.🤖 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 `@src/lib/omp-hook.js` around lines 264 - 333, Extract the duplicated exclusive-create sequence into a local createExclusive helper near the surrounding hook-update logic, encapsulating fs.open(extensionPath, "wx"), writing source, closing the handle, probing state, and returning the existing success result. Replace both create branches with this helper while preserving their current error handling and retry behavior.
🤖 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 `@src/commands/uninstall.js`:
- Around line 214-231: The uninstall formatter in src/commands/uninstall.js
lines 214-231 should build residual from ompHookRemove.stagedPath when
available, falling back to extensionPath. In src/lib/omp-hook.js lines 436-448,
update the staged-read-failure handling to detect restore rename failure and
return stagedPath when the staged file remains, matching the identity-changed
result behavior.
In `@src/lib/omp-hook.js`:
- Around line 230-231: Update the write path around handle.truncate and
handle.writeFile so the replacement source is written from position 0 after
truncation. Use an explicit position-0 write or reset the handle position,
preserving the existing UTF-8 source replacement behavior in
writeManagedThroughHandle.
- Around line 166-173: Update the write-after path involving pathIdentity and
writeManagedThroughHandle so non-ENOENT fs.stat failures are propagated as
upsertOmpHook’s structured { written: false, skippedReason, error } result
instead of escaping as exceptions. Preserve null for ENOENT and keep successful
identity checks unchanged.
---
Nitpick comments:
In `@src/lib/omp-hook.js`:
- Around line 264-333: Extract the duplicated exclusive-create sequence into a
local createExclusive helper near the surrounding hook-update logic,
encapsulating fs.open(extensionPath, "wx"), writing source, closing the handle,
probing state, and returning the existing success result. Replace both create
branches with this helper while preserving their current error handling and
retry behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cbce4a10-df3c-4685-b650-1193901b573c
📒 Files selected for processing (5)
README.mdREADME.zh-CN.mdsrc/commands/uninstall.jssrc/lib/omp-hook.jstest/omp-hook.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
| const reason = ompHookRemove?.skippedReason; | ||
| const residual = ompHookRemove?.extensionPath | ||
| ? ` (left in place: ${ompHookRemove.extensionPath})` | ||
| : ""; | ||
| if (reason === "unmanaged") { | ||
| return `- oh-my-pi notify extension: skipped (unmanaged file)${residual}`; | ||
| } | ||
| if (reason === "unlink-failed") { | ||
| const detail = ompHookRemove?.error ? `: ${ompHookRemove.error}` : ""; | ||
| return `- oh-my-pi notify extension: failed to remove${detail}${residual}`; | ||
| } | ||
| if (reason === "extension-read-failed") { | ||
| const detail = ompHookRemove?.error ? `: ${ompHookRemove.error}` : ""; | ||
| return `- oh-my-pi notify extension: failed to read${detail}${residual}`; | ||
| } | ||
| if (reason === "identity-changed") { | ||
| return `- oh-my-pi notify extension: skipped (file changed during uninstall)${residual}`; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Staged residual file is misreported during uninstall. When removeOmpHook stages the extension under a .tokentracker-removing-* temp name and cannot restore it, the leftover lives at the staging path — but the result object does not always carry stagedPath, and the formatter never reads it, so users are pointed at extensionPath instead.
src/commands/uninstall.js#L214-L231: buildresidualfromompHookRemove.stagedPath || ompHookRemove.extensionPathso the reported path matches the actual leftover.src/lib/omp-hook.js#L436-L448: on the staged-read-failure path, detect whether the restorerenamesucceeded and includestagedPathin the returned result when it did not, matching theidentity-changedbranch.
📍 Affects 2 files
src/commands/uninstall.js#L214-L231(this comment)src/lib/omp-hook.js#L436-L448
🤖 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 `@src/commands/uninstall.js` around lines 214 - 231, The uninstall formatter in
src/commands/uninstall.js lines 214-231 should build residual from
ompHookRemove.stagedPath when available, falling back to extensionPath. In
src/lib/omp-hook.js lines 436-448, update the staged-read-failure handling to
detect restore rename failure and return stagedPath when the staged file
remains, matching the identity-changed result behavior.
| async function pathIdentity(filePath) { | ||
| try { | ||
| return fileIdentity(await fs.stat(filePath)); | ||
| } catch (err) { | ||
| if (err && err.code === "ENOENT") return null; | ||
| throw err; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Node.js fs.stat return a usable ino value on Windows NTFS?
💡 Result:
Yes, Node.js fs.stat returns an ino (inode) value on Windows NTFS, but it is important to understand its nature and limitations. On Windows, the file system does not have "inodes" in the POSIX sense. To maintain a consistent API across platforms, Node.js maps the 64-bit Windows "File ID" to the ino property [1][2]. Key points regarding the usage of ino on Windows: 1. Data Integrity and Precision: Older versions of Node.js returned 0 or truncated values for the ino property because the 64-bit file identifier could not be represented accurately as a standard JavaScript number [3][1]. Modern Node.js versions (v10.5.0 and later) support the bigint: true option in fs.stat [1][4]. Using this option allows you to receive the ino as a BigInt, which correctly represents the full 64-bit file identifier without precision loss [5][6]. 2. Not a Unique Identifier: While it acts as a file identifier, the ino value on Windows should not necessarily be relied upon as a globally unique identifier for file tracking in the same way as POSIX inodes [1]. 3. Consistency: Because Windows does not use inodes, you may occasionally encounter situations where different paths or specific network drive configurations lead to unexpected or inconsistent ino values [7]. Recommendation: If you require reliable file identification on Windows, it is best practice to use the bigint option with fs.stat (or fs.promises.stat) to ensure you are capturing the full, accurate 64-bit identifier [1][6][4]: const stats = fs.statSync(path, { bigint: true }); console.log(stats.ino); // Returns a BigInt representing the 64-bit File ID
Citations:
- 1: https://github.com/gyselroth/node-windows-fsstat
- 2: https://www.npmjs.com/package/@gyselroth/windows-fsstat
- 3: fs.Stats.ino always returns 0 on Windows nodejs/node-v0.x-archive#2670
- 4: https://nodejs.org/api/fs.html
- 5: Node occasionally gives multiple files/folders the same inode nodejs/node#12115
- 6: https://nodejs.org/download/release/v14.17.4/docs/api/fs.html
- 7: https://stackoverflow.com/questions/73862020/fs-statsync-returning-inconsistent-data-on-case-sensitive-network-drive
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant file and call sites.
printf 'Files matching omp-hook.js:\n'
fd -a 'omp-hook\.js$' . || true
if [ -f src/lib/omp-hook.js ]; then
printf '\nsrc/lib/omp-hook.js line count:\n'
wc -l src/lib/omp-hook.js
printf '\nRelevant identity/helper sections:\n'
sed -n '1,240p' src/lib/omp-hook.js | cat -n
fi
printf '\nSearch for upsertOmpHook/writeManagedThroughHandle/removal callers:\n'
rg -n "upsertOmpHook|removeOmpHook|writeManagedThroughHandle|pathIdentity|sameFileIdentity|skippedReason|written" -S .Repository: xiufengsun/TokenTracker
Length of output: 36267
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'src/lib/omp-hook.js upsertOmpHook/removeOmpHook sections:\n'
sed -n '242,499p' src/lib/omp-hook.js | cat -n | sed 's/^/src\/lib\/omp-hook.js:/'
printf '\nsrc/commands/init.js upsertOmpHook caller section:\n'
sed -n '640,705p' src/commands/init.js | cat -n
printf '\nsrc/commands/uninstall.js OMP removal caller section:\n'
sed -n '100,225p' src/commands/uninstall.js | cat -nRepository: xiufengsun/TokenTracker
Length of output: 25413
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Runtime probe: Node fs.readdirSync stats ino behavior and value shape:\n'
node - <<'JS'
const fs = require('node:fs');
const os = require('node:os');
const path = require('node:path');
let dir = null;
try {
dir = fs.mkdtempSync(path.join(os.tmpdir(), 'node-ino-probe-'));
const a = path.join(dir, 'a.txt');
const b = path.join(dir, 'b.txt');
const content = 'probe';
fs.writeFileSync(a, content, 'utf8');
fs.writeFileSync(b, content, 'utf8');
const entries = fs.readdirSync(dir, { withFileTypes: true });
const stats = entries.map(d => {
const s = fs.statSync(path.join(dir, d.name));
return { name: d.name, dev: typeof s.dev, ino: String(s.ino), inoType: typeof s.ino, inoBits: s.ino.toString(2).length, valid: Number.isInteger(s.dev) && Number.isInteger(s.ino) };
});
for (const row of stats) console.log(JSON.stringify(row));
const same = fs.statSync(a).dev === fs.statSync(b).dev && fs.statSync(a).ino === fs.statSync(b).ino;
console.log(JSON.stringify({ sameDifferentFiles: same }));
} finally {
if (dir && fs.existsSync(dir)) {
fs.readdirSync(dir).forEach(name => fs.rmSync(path.join(dir, name)));
fs.rmSync(dir);
}
}
JSRepository: xiufengsun/TokenTracker
Length of output: 1362
Don’t let non-ENOENT identity checks bypass upsertOmpHook’s structured result contract.
writeManagedThroughHandle() is not wrapped by upsertOmpHook’s try/catch, so a non-ENOENT fs.stat() failure during the write-after path turns into a thrown exception rather than the function’s usual { written: false, skippedReason, error } response. Return/propagate the failure as a known result instead of crashing init/uninstall flow.
🛡️ Proposed fix: preserve structure when path identity cannot be read
async function pathIdentity(filePath) {
try {
return fileIdentity(await fs.stat(filePath));
} catch (err) {
if (err && err.code === "ENOENT") return null;
- throw err;
+ // Unknown identity: callers must treat this as "not verified" rather than
+ // crashing the whole init/uninstall flow.
+ return null;
}
}📝 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.
| async function pathIdentity(filePath) { | |
| try { | |
| return fileIdentity(await fs.stat(filePath)); | |
| } catch (err) { | |
| if (err && err.code === "ENOENT") return null; | |
| throw err; | |
| } | |
| } | |
| async function pathIdentity(filePath) { | |
| try { | |
| return fileIdentity(await fs.stat(filePath)); | |
| } catch (err) { | |
| if (err && err.code === "ENOENT") return null; | |
| // Unknown identity: callers must treat this as "not verified" rather than | |
| // crashing the whole init/uninstall flow. | |
| return null; | |
| } | |
| } |
🤖 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 `@src/lib/omp-hook.js` around lines 166 - 173, Update the write-after path
involving pathIdentity and writeManagedThroughHandle so non-ENOENT fs.stat
failures are propagated as upsertOmpHook’s structured { written: false,
skippedReason, error } result instead of escaping as exceptions. Preserve null
for ENOENT and keep successful identity checks unchanged.
| await handle.truncate(0); | ||
| await handle.writeFile(source, "utf8"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Node.js filehandle.writeFile current position after readFile truncate
💡 Result:
In Node.js, when using a FileHandle, calling filehandle.writeFile after performing other read or write operations (such as readFile or truncate) will write data starting from the current file position [1][2]. The FileHandle maintains an internal file position (or offset) [1][3]. If you have previously performed read or write operations, the file pointer is updated to the position immediately following the last operation [1]. If you call filehandle.writeFile without specifying a position, it writes from this current position until the end of the file [1][4]. It is important to note the following behaviors: - Truncate: The filehandle.truncate(len) method changes the file's size but does not inherently reset the file pointer position to the beginning of the file [1][5]. If you truncate a file and then immediately call filehandle.writeFile without a specified position, it will attempt to write from wherever the file pointer was left by previous operations [1]. - Controlling Position: To ensure you are writing to the beginning of a file or a specific location after truncation, you should explicitly provide the position argument (e.g., filehandle.writeFile(data, { position: 0 })) [4][6]. Alternatively, you can use filehandle.truncate(0) followed by a write operation at position 0 to effectively overwrite the file [4][6]. - Non-seekable files: If the file descriptor is not seekable, attempting to use positional operations may result in an error [3]. In summary, because filehandle.writeFile defaults to using the current file position, you cannot assume it will automatically reset to the beginning of the file after a truncation [1][4]. Always specify the position if you need to ensure the data is written at a specific location [4].
Citations:
- 1: https://nodejs.org/api/fs.html
- 2: https://nodejs.org/docs/latest-v24.x/api/fs.html
- 3: fs: behaviour of readFile and writeFile with file descriptors nodejs/node#23433
- 4: https://nodejs.org/api/fs.md
- 5: https://nodejs.org/docs/latest-v26.x/api/fs.html
- 6: https://github.com/nodejs/node/blob/master/doc/api/fs.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching omp-hook.js:"
fd -a 'omp-hook\.js$' . || true
echo
echo "Relevant symbols/outline:"
file="$(fd 'omp-hook\.js$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo "FILE=$file"
wc -l "$file"
ast-grep outline "$file" --view compact || true
echo
echo "Lines 200-245:"
sed -n '200,245p' "$file" | nl -ba -v200
echo
echo "Lines 320-360:"
sed -n '320,360p' "$file" | nl -ba -v320
echo
echo "Search upsert/probe/writeManagedThroughHandle:"
rg -n "function (writeManagedThroughHandle|upsertOmpHook|probeOmpHookState)|handle\.writeFile|handle\.readFile|handle\.truncate|handle\.write" "$file"
fiRepository: xiufengsun/TokenTracker
Length of output: 516
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="./src/lib/omp-hook.js"
echo "Lines 200-245:"
awk '{printf "%5d\t%s\n", NR, $0}' "$file" | sed -n '200,245p'
echo
echo "Lines 320-360:"
awk '{printf "%5d\t%s\n", NR, $0}' "$file" | sed -n '320,370p'
echo
echo "Relevant handle usages:"
rg -n "handle\.writeFile|handle\.readFile|handle\.truncate|handle\.read|handle\.write|fs\.open|appendFile|writeFile" "$file"
echo
echo "Context around writeManagedThroughHandle and upsertOmpHook:"
awk '{printf "%5d\t%s\n", NR, $0}' "$file" | sed -n '160,380p'Repository: xiufengsun/TokenTracker
Length of output: 12087
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node <<'JS'
const fs = require('fs/promises');
const os = require('os');
const path = require('path');
(async () => {
const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'node-filepos-'));
const file = path.join(dir, 'test.txt');
const source = 'abc';
// Simulate existing managed file bytes, then handle.readFile("utf8") leaving position at EOF.
await fs.writeFile(file, 'x'.repeat(3));
const handle = await fs.open(file, 'r+');
await handle.readFile('utf8');
const beforePos = await handle.read(Buffer.alloc(0), 0, 0, null);
console.log('beforePos.offset=', beforePos.bytesRead);
await handle.truncate(0);
await handle.writeFile(source, 'utf8');
const content = await handle.read(Buffer.alloc(1024), 0, 1024, 0);
const written = content.bytesRead;
const byteString = Buffer.from(content.buffer, 0, written).toString('hex');
console.log('writtenLength=', written);
console.log('contentHex=', byteString);
console.log('content=', Buffer.from(content.buffer, 0, written).toString());
await handle.close();
await fs.rm(dir, { recursive: true, force: true });
})().catch(err => { console.error(err.stack); process.exit(1); });
JSRepository: xiufengsun/TokenTracker
Length of output: 3293
Write managed files from the beginning after truncating.
In the update path, handle.readFile("utf8") leaves the handle position at EOF before writeManagedThroughHandle is called. handle.truncate(0) does not reset that position, and handle.writeFile() writes from the current position, so the managed extension becomes NUL-padded instead of a clean source replacement. Use a second handle opened for writing, or reset the position/filehandle.writeFile() with position: 0 before writing.
🐛 Proposed fix: write at position 0
await handle.truncate(0);
- await handle.writeFile(source, "utf8");
+ await handle.write(source, 0, "utf8");</ details>
📝 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.
| await handle.truncate(0); | |
| await handle.writeFile(source, "utf8"); | |
| await handle.truncate(0); | |
| await handle.write(source, 0, "utf8"); |
🤖 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 `@src/lib/omp-hook.js` around lines 230 - 231, Update the write path around
handle.truncate and handle.writeFile so the replacement source is written from
position 0 after truncation. Use an explicit position-0 write or reset the
handle position, preserving the existing UTF-8 source replacement behavior in
writeManagedThroughHandle.
3 participants
Summary
omp) and pi so dashboard/source chips no longer fall back to mono placeholders.init(~/.omp/agent/extensions/tokentracker-notify.ts) that firesnotify.cjs --source=ompafterturn_end/agent_end/session_shutdown, while keeping the existing passive session scanner as the source of truth.status, remove it onuninstall(managed files only), and document the dual path in README / README.zh-CN.Why
TokenTracker already parses
~/.omp/agent/sessions/**/*.jsonlcorrectly, but:This keeps passive scanning (zero-invasive, reliable) and adds an optional near-real-time path with debounce (~12s extension + notify's ~20s throttle).
Test plan
node --test test/omp-hook.test.js(4/4 pass)vitestProviderIcon + provider-display tests (13/13 pass)upsertOmpHookwrites managed extension under~/.omp/agent/extensions/node bin/tracker.js statusshowsnotify extension: yeswhen omp is presentnotify.signalupdates / queue gainssource: "omp"bucketsNotes
tokentracker-notify.tsis left untouched.Summary by CodeRabbit
statusnow detects and reports oh-my-pi notify-extension configuration (including path).uninstallnow removes the managed notify extension while preserving unmanaged/user-created files.