Repository navigation
fix(wiki): the semantic harvest was implemented but never invoked - #229
Merged
Merged
Conversation
The wiki design specifies findings extracted "at Stop and PreCompact" by a cheap
model, out of band. hooks/lib/harvest.mjs implements that -- digest, model call,
validation -- and was imported by NOTHING. The only match for its own filename
across the repo is its header comment. Stop carried no token-optimizer hook at
all, and precompact-optimize.mjs makes no reference to harvesting.
The failure was silent rather than loud, which is why it survived: the graph kept
filling with its structural layer while the semantic layer never ran.
Measured on a real project after a full session of heavy work:
node kinds file 122, symbol 132, task 61, finding 0
edges derived_from 61, contains 132
metrics injections 0, harvestTokens 0
"insufficient data (0 treated, 0 holdout; need 20 and 5)"
Everything downstream -- inject, staleness, skeleton, restore, curate, and the
treated-vs-holdout metrics -- was wired and idle, waiting on findings that could
not arrive. The whole value proposition of the graph (retrieve verdicts rather
than re-derive them, and record dead ends that exist nowhere in the source tree)
was unreachable.
Three pieces were missing:
1. A writer. harvest.mjs validates findings but nothing stored them, and
curate.create() could not be reused: it stamps origin ORIGIN_HUMAN and a
`human-` key because it exists for a person asserting something. Routing
machine output through it would label a model's guess as a human assertion --
the exact confusion curate.mjs warns destroys trust calibration. So
harvest-write.mjs writes with ORIGIN_HARVESTED, copying create()'s anchor
discipline: index the file first, and refuse a finding whose anchors do not
resolve rather than store an un-invalidatable claim.
2. An invocation. stop-harvest.mjs runs at Stop and spawns a DETACHED worker,
because the extraction is a model call and doing it inline would make the
session that did the work pay for summarising itself -- the cost this
subsystem exists to avoid. Stop is never delayed.
3. A voice. harvestEnabled() returns false without an API key and the module
skips silently; nothing in doctor, audit or waste mentions harvest, so a user
sees a graph with no findings and cannot learn why. The Stop hook now says so
once per session, names the variable, and states that the structural layer
still works.
Verified end to end: writeHarvested stored 2 findings with origin=harvested and
types failure/decision, auto-created their file and symbol anchor nodes, and
REFUSED a third whose anchor did not exist. The Stop hook fires, reports the
disabled state once, and stays silent on the second call.
Note the remaining precondition: extraction needs TOKEN_OPTIMIZER_API_KEY or
ANTHROPIC_API_KEY. Without one this now reports that fact instead of hiding it.
99 suites / 1423 tests green; tsc clean.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThe PR adds graph persistence for harvested and agent findings, a ChangesHarvest and graph pipeline
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Agent
participant Server
participant wikiWrite
participant writeHarvested
participant Graph
Agent->>Server: wiki_write claim and anchors
Server->>wikiWrite: validated arguments
wikiWrite->>writeHarvested: agent finding with session provenance
writeHarvested->>Graph: resolve anchors and persist finding
Graph-->>writeHarvested: stored key and derived_from edges
wikiWrite-->>Server: structured result
Server-->>Agent: serialized tool result
sequenceDiagram
participant ClaudeCode
participant stopHarvest
participant harvestWorker
participant writeHarvested
ClaudeCode->>stopHarvest: session stop input
stopHarvest->>harvestWorker: detached transcript payload
harvestWorker->>writeHarvested: validated harvested findings
writeHarvested-->>harvestWorker: stored keys
stopHarvest-->>ClaudeCode: immediate successful return
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![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)
plugin/hooks/harvest-worker.mjs (1)
75-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecord the extraction output cost, not only the digest size.
The header states the cost of the extraction is recorded either way.
tokens: estimateTokens(digest)counts input only, sonetTokensstill understates the expense by the model response size. Include the extracted response length in the recorded cost.🤖 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 `@plugin/hooks/harvest-worker.mjs` around lines 75 - 81, Update the tokens value in the record call within the harvest flow to include both the digest input tokens and the extracted response output tokens, using the existing extraction output symbol rather than recording only estimateTokens(digest). Preserve the current metadata and record structure.hooks-core/harvest-write.mjs (1)
41-46: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winLoad the graph once and index each file at most once per run.
resolveAnchorcallsload(dir)for every anchor of every finding. Each call replays the whole append-only log, so cost grows with log size times anchor count.indexFilealso runs unconditionally, so each harvest appends duplicate file and symbol records for files that are already indexed.Hoist the graph load out of the loop and cache indexed paths.
♻️ Proposed refactor
-function resolveAnchor(dir, anchor) { +function resolveAnchor(dir, anchor, indexed) { const [rawPath, symbol] = String(anchor).split('#'); if (!rawPath) return null; const path = canonicalPath(rawPath); // Indexing creates the file node and its symbols with hashes and spans, which // is what makes the claim checkable later. - indexFile(dir, path); + if (!indexed.has(path)) { + indexFile(dir, path); + indexed.add(path); + } const ? nodeId('symbol', symbolKey(path, symbol)) : nodeId('file', path); return load(dir).nodes.has(target) ? target : null; }Also applies to: 59-64
🤖 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 `@hooks-core/harvest-write.mjs` around lines 41 - 46, Update the harvest flow and resolveAnchor so the graph is loaded once per run and reused for all anchor lookups, rather than calling load(dir) for each anchor. Track indexed file paths in a per-run cache and invoke indexFile only for paths not already cached or present in the graph, preventing duplicate file and symbol records while preserving existing target resolution.
🤖 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 `@hooks-core/harvest-write.mjs`:
- Around line 34-47: Update resolveAnchor in hooks-core/harvest-write.mjs to
reject canonicalized anchor paths outside the project directory before calling
indexFile; preserve valid in-project file and symbol resolution. Regenerate
integrations/codex/hooks/lib/harvest-write.mjs,
integrations/codex/plugin/hooks/lib/harvest-write.mjs,
integrations/gemini/hooks/lib/harvest-write.mjs,
integrations/opencode/hooks/lib/harvest-write.mjs,
integrations/qwen/hooks/lib/harvest-write.mjs, and
plugin/hooks/lib/harvest-write.mjs using npm run sync:hooks rather than editing
generated copies.
- Line 69: Update the finding key construction in the harvest write flow to
include a sufficiently random component alongside the timestamp and written
index. Preserve the existing key format components while ensuring concurrent
detached workers cannot generate the same key and cause putNode to overwrite a
finding.
In `@plugin/hooks/stop-harvest.mjs`:
- Around line 36-44: Update noticePath to use os.tmpdir() as the base temporary
directory instead of the environment-variable chain and "." fallback, and
sanitize sessionId before interpolation so separators, "..", and other path
traversal input cannot escape the token-optimizer directory while preserving the
existing length limit and unknown fallback.
- Around line 91-101: Add per-session debounce logic in the Stop hook before
spawning the detached worker, using a session-specific marker and minimum
elapsed interval to skip recent harvests. Ensure the marker is updated only when
a harvest is actually initiated, while preserving the existing worker arguments
and detached spawn behavior.
- Around line 79-86: The semantic-harvest-off path must not force immediate
shutdown before its stdout notice is delivered. Update the surrounding hook flow
to mark the JSON notice as emitted, handle the return value of
process.stdout.write by waiting for drain/completion when it returns false, and
use process.exitCode or natural process termination instead of process.exit(0).
---
Nitpick comments:
In `@hooks-core/harvest-write.mjs`:
- Around line 41-46: Update the harvest flow and resolveAnchor so the graph is
loaded once per run and reused for all anchor lookups, rather than calling
load(dir) for each anchor. Track indexed file paths in a per-run cache and
invoke indexFile only for paths not already cached or present in the graph,
preventing duplicate file and symbol records while preserving existing target
resolution.
In `@plugin/hooks/harvest-worker.mjs`:
- Around line 75-81: Update the tokens value in the record call within the
harvest flow to include both the digest input tokens and the extracted response
output tokens, using the existing extraction output symbol rather than recording
only estimateTokens(digest). Preserve the current metadata and record structure.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7dbcd688-2473-48a6-b992-7e2751d86d32
📒 Files selected for processing (10)
hooks-core/harvest-write.mjsintegrations/codex/hooks/lib/harvest-write.mjsintegrations/codex/plugin/hooks/lib/harvest-write.mjsintegrations/gemini/hooks/lib/harvest-write.mjsintegrations/opencode/hooks/lib/harvest-write.mjsintegrations/qwen/hooks/lib/harvest-write.mjsplugin/hooks/harvest-worker.mjsplugin/hooks/hooks.jsonplugin/hooks/lib/harvest-write.mjsplugin/hooks/stop-harvest.mjs
…d store
Three structural defects measured on one real project's graph, all of which
made the wiki cost more and reach less than its design says it should.
1. SYMBOL SNAPSHOTS WERE UNBOUNDED.
File snapshots have been capped at 256 KB since P2. Symbol spans were stored
unconditionally, on the stated reasoning that "a function is small". That holds
for hand-written code and fails badly on generated sources, where one class body
is megabytes.
before 132 symbol nodes holding 34.3 MB -- 95.8% of a 35 MB graph
largest single span 1.8 MB
file nodes, already capped, held 0.7 MB
The graph had become a second and worse copy of the repository. Symbol spans now
take the same cap the file path already had (32 KB, TOKEN_OPTIMIZER_SYMBOL_
SNAPSHOT_LIMIT). Same trade as the file cap: the hash is still stored, so
staleness detection is untouched and only the reconstructed diff degrades --
which `serve` already reports plainly. Verified: a 170 KB span is omitted while
its hash is kept, and a small sibling is still stored in full.
2. `imports` AND `calls` EDGES WERE NEVER WRITTEN.
Of the eight declared edge kinds, only derived_from, contains, supersedes (human
correction only) and related were produced anywhere. The measured graph contained
exactly two: derived_from 61, contains 132.
That leaves the graph as disconnected stars -- a file and the symbols inside it
-- with no way to cross a file boundary. Retrieval is traversal by design, with
no embedding index as the deliberate trade, so a finding one hop from the current
work was simply unreachable. "This symbol AND its callers, causally correct" is
the design's stated advantage over RAG and it could not happen.
Both are now written during structural indexing, which already parses the file:
imports file -> file, RELATIVE specifiers only. A bare specifier (react,
numpy) names a package, not a node, so it is dropped rather than
resolved to a lie. The target must exist on disk.
calls symbol -> symbol, within ONE file. Cross-file resolution needs a real
type resolver; a regex guess would bind `handle` here to `handle`
somewhere unrelated. symbols.mjs's doctrine is that a missed symbol is
a coarser answer and a wrong one is unacceptable, so the scope stops
where the match stops being unambiguous.
Verified on a fixture: `./sub/dep.mjs` linked, `react` skipped, alpha->beta
linked within the file, and the cross-file call to `helper` correctly not linked.
3. THE STORE DID NOT IGNORE ITSELF.
The design says this directory is gitignored by default, because findings are
unreviewed agent output and committing them puts unreviewed analysis into git
history. Nothing enforced it: on two real checkouts `.token-optimizer/` showed
as untracked, so one `git add -A` would have committed 35 MB of it.
It now writes a `.gitignore` containing `*` INSIDE the store rather than editing
the project's own -- writing to a file the user reviews is not this tool's
business.
Strictly inside, and that is not a detail. My first attempt wrote the marker to
the PARENT, which is correct for the default layout but not when
TOKEN_OPTIMIZER_WIKI_DIR points at a directory the user owns -- there the parent
is their project root and `*` ignores their entire repository. The skeleton
suite caught it immediately: `git add auth.ts` started refusing a file the test
had just written. Ignoring only what we create cannot reach outside the store.
98 suites / 1409 tests green; tsc clean.
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (24)
integrations/qwen/hooks/lib/staleness.mjs-202-237 (1)
202-237: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
linkCallscan attach an edge to the wrong same-named declaration.Same root cause as
hooks-core/staleness.mjs:byNamecannot distinguish disambiguated duplicate names from bare call-site text. See the consolidated comment.🤖 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 `@integrations/qwen/hooks/lib/staleness.mjs` around lines 202 - 237, Update linkCalls so call targets are resolved using each symbol’s disambiguated identity rather than the name-only byName map. Ensure bare call-site text cannot select the wrong declaration when duplicate names exist, while preserving edges for unambiguous same-file declarations.integrations/codex/hooks/lib/wiki.mjs-201-236 (1)
201-236: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
ignoreSelfhas no guard againstTOKEN_OPTIMIZER_WIKI_DIRpointing at a directory the user cares about.Same concern as
hooks-core/wiki.mjs: ifTOKEN_OPTIMIZER_WIKI_DIRresolves to the project root (or another tracked directory), this writes a.gitignorewith*directly into it, silently un-tracking everything. See the consolidated comment.🤖 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 `@integrations/codex/hooks/lib/wiki.mjs` around lines 201 - 236, Update ignoreSelf to guard against writing the wildcard .gitignore when TOKEN_OPTIMIZER_WIKI_DIR resolves to the project root or another user-owned/tracked directory. Reuse the established safety check from hooks-core/wiki.mjs if available, and preserve best-effort behavior by skipping the marker write when the directory is not a safe tool-owned store.integrations/gemini/hooks/lib/wiki.mjs-201-236 (1)
201-236: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
ignoreSelfhas no guard againstTOKEN_OPTIMIZER_WIKI_DIRpointing at a directory the user cares about.Same concern as
hooks-core/wiki.mjs. See the consolidated comment.🤖 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 `@integrations/gemini/hooks/lib/wiki.mjs` around lines 201 - 236, Update ignoreSelf to guard against TOKEN_OPTIMIZER_WIKI_DIR targeting a user-owned or otherwise unsafe directory before creating the internal .gitignore marker. Reuse the established safety validation from the corresponding hooks-core wiki implementation, and preserve the best-effort behavior for valid store directories.integrations/codex/plugin/hooks/lib/wiki.mjs-201-236 (1)
201-236: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
ignoreSelfhas no guard againstTOKEN_OPTIMIZER_WIKI_DIRpointing at a directory the user cares about.Same concern as
hooks-core/wiki.mjs: an unfortunateTOKEN_OPTIMIZER_WIKI_DIRvalue can cause this to gitignore an entire tracked directory. See the consolidated comment.🤖 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 `@integrations/codex/plugin/hooks/lib/wiki.mjs` around lines 201 - 236, Update ignoreSelf to validate that the resolved store directory is safe to modify before creating its .gitignore marker, reusing the existing protection approach from hooks-core/wiki.mjs. Skip writing the marker when TOKEN_OPTIMIZER_WIKI_DIR points to a user-owned or otherwise unsafe directory, while preserving best-effort behavior for valid store paths.integrations/opencode/hooks/lib/wiki.mjs-201-236 (1)
201-236: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
ignoreSelfhas no guard againstTOKEN_OPTIMIZER_WIKI_DIRpointing at a directory the user cares about.Same concern as
hooks-core/wiki.mjs. See the consolidated comment.🤖 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 `@integrations/opencode/hooks/lib/wiki.mjs` around lines 201 - 236, Update ignoreSelf to guard against TOKEN_OPTIMIZER_WIKI_DIR resolving to a user-owned directory before creating the internal .gitignore marker. Reuse the established safety validation or trusted-store check from the corresponding hooks-core wiki implementation, and skip marker creation when the directory is not a known token-optimizer store while preserving best-effort behavior for valid stores.integrations/qwen/hooks/lib/wiki.mjs-201-236 (1)
201-236: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
ignoreSelfhas no guard againstTOKEN_OPTIMIZER_WIKI_DIRpointing at a directory the user cares about.Same concern as
hooks-core/wiki.mjs. See the consolidated comment.🤖 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 `@integrations/qwen/hooks/lib/wiki.mjs` around lines 201 - 236, Update ignoreSelf to validate that the target directory is the managed token-optimizer store before creating its .gitignore marker, rejecting or skipping user-owned directories configured through TOKEN_OPTIMIZER_WIKI_DIR. Preserve best-effort behavior so an unsafe or invalid target does not fail the graph write, and keep the marker scoped strictly inside the verified store.integrations/gemini/hooks/lib/staleness.mjs-202-237 (1)
202-237: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
linkCallscan attach an edge to the wrong same-named declaration.Same root cause as
hooks-core/staleness.mjs:byNamecannot distinguish disambiguated duplicate names from bare call-site text. See the consolidated comment.🤖 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 `@integrations/gemini/hooks/lib/staleness.mjs` around lines 202 - 237, Update linkCalls so duplicate declarations with the same name are not resolved through the name-only byName map. Only create calls edges when the target name maps to exactly one unambiguous symbol, while preserving the existing same-file, self-call, keyword, and deduplication checks.integrations/codex/hooks/lib/staleness.mjs-202-237 (1)
202-237: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
linkCallscan attach an edge to the wrong same-named declaration.Same root cause as
hooks-core/staleness.mjs:byNameis keyed by post-disambiguation names, but call-site text never carries the~NsuffixextractSymbolsuses to distinguish duplicates. See the consolidated comment for the shared fix.🤖 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 `@integrations/codex/hooks/lib/staleness.mjs` around lines 202 - 237, Update linkCalls so call-site names resolve only to an unambiguous declaration, rather than using byName entries keyed by post-disambiguation names that may include ~N suffixes. Preserve same-file linking and skip calls when multiple declarations share the textual name, preventing edges from targeting an arbitrary duplicate.integrations/qwen/hooks/lib/staleness.mjs-146-168 (1)
146-168: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
linkImportscreates a bare file-node stub that can overwrite a fully-indexed node.Same concern as
hooks-core/staleness.mjs: the stubputNode(dir, { kind: 'file', key: target })at Line 166 omitshash/snapshotand can replace an already fully-indexed node fortarget. See the consolidated comment.🤖 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 `@integrations/qwen/hooks/lib/staleness.mjs` around lines 146 - 168, Update linkImports so resolving an import reuses the existing fully indexed file node for target instead of creating a metadata-free stub that can overwrite it. Preserve the current behavior of adding the imports edge and only create or merge a node when no existing node is available, retaining the target’s hash and snapshot metadata.integrations/codex/hooks/lib/staleness.mjs-146-168 (1)
146-168: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
linkImportscreates a bare file-node stub that can overwrite a fully-indexed node.Same concern as
hooks-core/staleness.mjs:putNode(dir, { kind: 'file', key: target })at Line 166 omitshash/snapshot. Iftargetalready has a fully-indexed file node, this write can replace it with one lackinghash, causingcheckAnchorto report every anchor ontargetas stale afterward. See the consolidated comment for the shared fix direction.🤖 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 `@integrations/codex/hooks/lib/staleness.mjs` around lines 146 - 168, Update linkImports so creating or retrieving the target file node cannot replace an existing fully indexed node with a bare { kind: 'file', key: target } stub. Reuse the existing node-preserving behavior or merge only missing node data when calling putNode, while keeping the imports edge creation and avoiding recursive indexing.integrations/gemini/hooks/lib/staleness.mjs-146-168 (1)
146-168: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
linkImportscreates a bare file-node stub that can overwrite a fully-indexed node.Same concern as
hooks-core/staleness.mjs: the stubputNode(dir, { kind: 'file', key: target })at Line 166 omitshash/snapshotand can replace an already fully-indexed node fortarget. See the consolidated comment.🤖 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 `@integrations/gemini/hooks/lib/staleness.mjs` around lines 146 - 168, Update linkImports so creating the target file node cannot overwrite an existing fully indexed node or discard its hash/snapshot metadata. Reuse the existing node when available, or make putNode preserve previously indexed fields while adding the stub; keep the imports edge and unresolved-target behavior unchanged.integrations/codex/hooks/lib/symbols.mjs-98-160 (1)
98-160: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThe relative-specifier filter rejects every Python and Rust match.
Same issue as
hooks-core/symbols.mjs: thepypatterns (Lines 152-153) capture dotted paths like.foo, and therustpattern (Line 156) capturesself::foo/super::foo. Neither format passes the/^\.\.?[\\/]/relative check at Line 132, soextractImportsnever returns a specifier for these two languages. See the consolidated comment for the shared fix direction.🤖 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 `@integrations/codex/hooks/lib/symbols.mjs` around lines 98 - 160, Update extractImports so Python dotted-relative specifiers and Rust self::/super:: paths pass the relative-import filter instead of being discarded. Adjust the language-specific handling in IMPORT_PATTERNS or normalize its captured values before the filter, while preserving rejection of non-relative package and namespace imports and keeping Java/C# behavior unchanged.hooks-core/wiki.mjs-199-234 (1)
199-234: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRefuse
ignoreSelfwhendiris a tracked repository root.
wikiDir()allowsTOKEN_OPTIMIZER_WIKI_DIRto override the default, andignoreSelf()writes.gitignoredirectly indir. If a user points this at their repository root or another directory they track,*silently makes git stop tracking files under that directory. Add a guard that refuses to write the marker whendircontains a.gitentry, or only write it into self-hosting nested stores.🤖 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 `@hooks-core/wiki.mjs` around lines 199 - 234, Update ignoreSelf to detect whether dir contains a .git entry before creating its .gitignore marker, and return without writing when it does. Preserve the existing best-effort behavior for valid nested stores, including the marker contents and error handling.hooks-core/staleness.mjs-200-235 (1)
200-235: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
linkCallscan attach an edge to the wrong same-named declaration.
extractSymbolsdisambiguates duplicate declaration names by renaming every occurrence after the first toname~N(seehooks-core/symbols.mjs, the "DISAMBIGUATE REPEATED NAMES" block).byNameat Line 214 is keyed by these post-disambiguation names, but a call site in the source text never writes the~Nsuffix — it only ever reads the plain name (for example,render().When a file declares two symbols with the same name, every call to that name resolves through
byName.has(name)to whichever declaration kept the plain name (the first one), regardless of which declaration the call site is actually inside. This produces exactly the "confidently wrong edge" the function's own doc comment says same-file scoping is meant to avoid.🐛 Proposed fix: skip names that are ambiguous within the file
function linkCalls(dir, path, source, symbols) { if (symbols.length < 2) return; const byName = new Map(symbols.map((s) => [s.name, s])); + // Names that were disambiguated by extractSymbols exist more than once in + // this file. A bare call-site name cannot say which declaration it means, + // so resolving it would produce a confidently wrong edge. + const ambiguous = new Set( + symbols.filter((s) => s.occurrence !== undefined).map((s) => s.name.replace(/~\d+$/, '')) + ); for (const caller of symbols) { const body = spanText(source, caller); const seen = new Set(); for (const match of body.matchAll(/([A-Za-z_$][\w$]*)\s*\(/g)) { const name = match[1]; - if (name === caller.name || seen.has(name) || !byName.has(name)) continue; + if (name === caller.name || seen.has(name) || !byName.has(name) || ambiguous.has(name)) continue; seen.add(name);🤖 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 `@hooks-core/staleness.mjs` around lines 200 - 235, Update linkCalls so duplicate declaration names are treated as ambiguous rather than resolved to the plain-name entry in byName. Build or reuse a set of names with multiple declarations, and skip matching call names in that set before putEdge; retain current behavior for uniquely declared symbols and existing self/seen/keyword filtering.hooks-core/staleness.mjs-144-166 (1)
144-166: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFix
linkImportsbefore it turns referenced files stale.
putNode(dir, { kind: 'file', key: target })appends a later file record fortargetwith nohashorsnapshot.load()uses the last record for each id, so anindexFile(dir, target, ...)record can be hidden by this import-only record. Anchors ontargetthen comparehash(source)to an undefinedanchor.hash, making stale findings report file changed even when the file did not change. Track visited targets inlinkImports, or defer import-node creation until the target has been indexed, and keeplinkImportsoffindexFile’s hot path.🤖 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 `@hooks-core/staleness.mjs` around lines 144 - 166, Update linkImports so creating an import edge cannot append an incomplete file node that supersedes an existing indexFile record for the same target. Track visited targets or defer import-node creation until the target is indexed, while keeping linkImports out of indexFile’s hot path and preserving valid import edges.hooks-core/symbols.mjs-96-158 (1)
96-158: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake import filtering language-aware and resolve Python/Rust specifiers.
extractImportskeeps a match only when/^\.\.?[\\/]/is true after the capture. Python and Rust imports are added toIMPORT_PATTERNS, but Python captures.foo/..foo.barand Rust capturesself::foo/super::bar::Baz, so no edge is created for those languages.If Python/Rust support is intended, use a package-aware relative filter and pass the language to
resolveImportso dotted notation and::namespaces are translated before path joining; otherwise keepIMPORT_PATTERNSabsent for those families so unresolved patterns do not accumulate dead graph probes.🤖 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 `@hooks-core/symbols.mjs` around lines 96 - 158, The import filter in extractImports must support the syntax captured by the Python and Rust entries in IMPORT_PATTERNS. Make the relative-import check language-aware, pass the language family into resolveImport, and translate Python dotted and Rust :: relative specifiers into path components before joining; otherwise remove those pattern entries so unsupported imports do not produce unresolved probes.integrations/opencode/hooks/lib/staleness.mjs-146-168 (1)
146-168: 🗄️ Data Integrity & Integration | 🟠 MajorSame target-node overwrite risk as
integrations/codex/plugin/hooks/lib/staleness.mjs.
putNode(dir, { kind: 'file', key: target })here writes a hash-less, snapshot-less placeholder for the import target on every reindex of the importer, which can overwrite an already-indexed target's realhash/snapshot(see the full analysis onintegrations/codex/plugin/hooks/lib/staleness.mjs, lines 146-168).🤖 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 `@integrations/opencode/hooks/lib/staleness.mjs` around lines 146 - 168, The import-linking logic in linkImports must not overwrite an already-indexed target node with a hash-less, snapshot-less placeholder. Update the target-node handling around putNode to reuse the existing indexed file node or otherwise preserve its hash and snapshot, while still creating a minimal node when the target has not been indexed.integrations/gemini/hooks/lib/symbols.mjs-98-160 (1)
98-160: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame Python/Rust import-filter gap as the other
symbols.mjscopies.The relative-path filter
/^\.\.?[\\/]/at line 132 rejects every specifierIMPORT_PATTERNS.pyandIMPORT_PATTERNS.rustcan produce (see the identical code and full analysis inintegrations/codex/plugin/hooks/lib/symbols.mjs, lines 98-160). Python and Rust import edges are never created here either.🤖 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 `@integrations/gemini/hooks/lib/symbols.mjs` around lines 98 - 160, Update extractImports and the Python/Rust entries in IMPORT_PATTERNS so their relative import specifiers pass the filter and produce edges. Preserve the existing relative-only behavior for JavaScript, while accepting Python dot imports and Rust self::/super:: paths without admitting package or namespace imports.integrations/opencode/hooks/lib/symbols.mjs-98-160 (1)
98-160: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame Python/Rust import-filter gap as the other
symbols.mjscopies.Same issue as
integrations/codex/plugin/hooks/lib/symbols.mjs, lines 98-160: the relative-path filter rejects every Python and Rust specifier these patterns can produce, so those languages never contributeimportsedges.🤖 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 `@integrations/opencode/hooks/lib/symbols.mjs` around lines 98 - 160, Update the relative-import filter in extractImports so it accepts the specifier formats produced by the py and rust IMPORT_PATTERNS, including Python dot-prefixed modules and Rust self:: or super:: paths, while continuing to exclude bare package or namespace imports. Preserve the existing JavaScript and Go behavior and deduplication.plugin/hooks/lib/symbols.mjs-98-160 (1)
98-160: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPython and Rust import extraction is dead code here too; fix the shared source, not this copy.
Same root cause as
integrations/codex/plugin/hooks/lib/symbols.mjs, lines 98-160: the relative-path filter/^\.\.?[\\/]/at line 132 rejects every specifierIMPORT_PATTERNS.pyandIMPORT_PATTERNS.rustcan produce (Python's dot-separated module paths and Rust'sself::/super::paths never start with.immediately followed by/or\). See that comment for the verification script.This file is a generated copy of the shared hook core. Apply the fix in
hooks-core/symbols.mjsand re-runscripts/sync-hook-core.mjsrather than editing this file directly.Based on learnings, treat hook integration copies such as this one as generated artifacts derived from the shared hook core; after fixing the core, run
scripts/sync-hook-core.mjsand verify the sync check passes.🤖 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 `@plugin/hooks/lib/symbols.mjs` around lines 98 - 160, Update the shared extractImports implementation and its language-specific relative-specifier handling in hooks-core/symbols.mjs so Python and Rust patterns are accepted without weakening package filtering; do not edit this generated copy directly. Run scripts/sync-hook-core.mjs to regenerate integrations/codex and other hook copies, then verify the synchronization check passes.Source: Learnings
plugin/hooks/lib/staleness.mjs-146-168 (1)
146-168: 🗄️ Data Integrity & Integration | 🟠 MajorSame target-node overwrite risk as the other
staleness.mjscopies; fix the shared source, not this copy.Same issue as
integrations/codex/plugin/hooks/lib/staleness.mjs, lines 146-168:putNode(dir, { kind: 'file', key: target })writes a hash-less, snapshot-less placeholder that can overwrite an already-indexed target's real data on reindex.This file is a generated copy of the shared hook core. Apply the fix in
hooks-core/staleness.mjsand re-runscripts/sync-hook-core.mjsrather than editing this file directly.Based on learnings, treat hook integration copies such as this one as generated artifacts derived from the shared hook core; after fixing the core, run
scripts/sync-hook-core.mjsand verify the sync check passes.🤖 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 `@plugin/hooks/lib/staleness.mjs` around lines 146 - 168, Fix the target-node overwrite risk in the shared hook-core implementation of linkImports, not this generated copy: reuse the existing indexed file node or otherwise preserve its hash and snapshot data when creating the import edge instead of writing a placeholder through putNode. Then run scripts/sync-hook-core.mjs to regenerate all integration copies and verify the synchronization check passes.Source: Learnings
integrations/qwen/hooks/lib/symbols.mjs-98-160 (1)
98-160: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame Python/Rust import-filter gap as the other
symbols.mjscopies.Same issue as
integrations/codex/plugin/hooks/lib/symbols.mjs, lines 98-160: the relative-path filter rejects every Python and Rust specifier these patterns can produce, so those languages never contributeimportsedges.🤖 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 `@integrations/qwen/hooks/lib/symbols.mjs` around lines 98 - 160, Update extractImports so its relative-import filter accepts the Python specifiers produced by IMPORT_PATTERNS (dot-prefixed module names) and the Rust self::/super:: specifiers, while continuing to exclude package and namespace imports. Keep the existing deduplication and raw-specifier return behavior unchanged.integrations/codex/plugin/hooks/lib/symbols.mjs-98-160 (1)
98-160: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake Python and Rust import patterns produce edges, or remove them.
extractImportskeeps Python and Rust specifiers only when they match/^\.\.?[\\/]/, so Python imports like.utils/..pkg.suband Rust imports likeself::foo::Bar/super::bazare always discarded. Also,resolveImporttreats specifiers as filesystem paths, so dotted Python modules or::Rust modules still need language-aware resolution before edges can be added.🤖 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 `@integrations/codex/plugin/hooks/lib/symbols.mjs` around lines 98 - 160, Update extractImports and resolveImport so Python relative module specifiers and Rust self/super module paths are retained and resolved using language-aware rules rather than filesystem-prefix checks. Convert Python dots and Rust :: segments into the appropriate relative path representation before resolving, and ensure the resulting edges target existing files. Alternatively remove the Python and Rust entries from IMPORT_PATTERNS if these imports cannot be resolved reliably.integrations/codex/plugin/hooks/lib/staleness.mjs-146-168 (1)
146-168: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the import target file node when creating the
importsedge.
linkImportscallsputNode(dir, { kind: 'file', key: target })withouthashorsnapshot.putNodeappends a new node record, andload()keeps the latest record withnodes.set(record.id, record). An imported file already indexed byindexFile()can be replaced with ahash: undefined/snapshot: undefinednode, so lazy stale checks on anchored findings return stale. Populatehashfromtarget’s content before linking, without recursing into its imports/symbols.🤖 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 `@integrations/codex/plugin/hooks/lib/staleness.mjs` around lines 146 - 168, Update linkImports to preserve metadata for an already indexed import target by obtaining target content and populating its hash before passing the file node to putNode. Keep this limited to the target file’s content and do not recurse into its imports or symbols; retain the existing unresolved-target behavior and edge creation flow.
🧹 Nitpick comments (2)
integrations/codex/plugin/hooks/lib/staleness.mjs (1)
202-237: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the call-scan length to match the documented snapshot cap.
linkCallsscans the full, un-truncatedbodywith a global regex for every symbol, regardless of the newsymbolSnapshotLimit()cap. The docstring abovesymbolSnapshotLimitcites a measured case of a 1.8 MB single span; that same span still gets a fullmatchAllscan here on every index, on the hot path this module's own comments elsewhere describe as latency-sensitive per tool call.Skip or truncate the call-scan for spans beyond the same size threshold, consistent with the snapshot bound already applied above.
⚡ Proposed bound
for (const caller of symbols) { const body = spanText(source, caller); + if (body.length > symbolSnapshotLimit()) continue; const seen = new Set();🤖 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 `@integrations/codex/plugin/hooks/lib/staleness.mjs` around lines 202 - 237, Bound the call-target scan in linkCalls to the same symbolSnapshotLimit() threshold used for snapshots. Before running body.matchAll, skip or truncate spans exceeding that limit, while preserving existing same-file, unambiguous edge behavior for bounded spans.plugin/hooks/lib/wiki.mjs (1)
237-260: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the ignore-marker check to avoid a redundant
existsSyncon every write.
appendcallsignoreSelf(dir)on line 252 before every single node and edge write.ignoreSelfperforms anexistsSyncsyscall each time, even after the marker file has already been confirmed present earlier in the same process. A singleindexFilecall writes one file node, N symbol nodes, Ncontainsedges, plus import and call edges — each of those now pays one extra synchronous filesystem stat.Cache the per-directory result in a module-level
Setso the check runs at most once per directory per process.⚡ Proposed cache
+const ignoredDirs = new Set(); + function ignoreSelf(dir) { + if (ignoredDirs.has(dir)) return; const marker = join(dir, '.gitignore'); try { - if (existsSync(marker)) return; + if (existsSync(marker)) { + ignoredDirs.add(dir); + return; + } appendFileSync( marker, '# Written by token-optimizer. Findings are unreviewed agent output;\n' + '# keeping them out of git history is the default. Delete this file to\n' + '# opt in to committing them.\n*\n' ); + ignoredDirs.add(dir); } catch { // Best effort. Failing to write the marker must not fail the graph write. } }This file is a generated copy of the shared hook core; apply this in
hooks-core/wiki.mjsand re-runscripts/sync-hook-core.mjsrather than editing this file directly.
Based on learnings, treat hook integration copies such as this one as generated artifacts derived from the shared hook core; after fixing the core, runscripts/sync-hook-core.mjsand verify the sync check passes.🤖 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 `@plugin/hooks/lib/wiki.mjs` around lines 237 - 260, Update the shared hook core’s ignoreSelf/append flow to maintain a module-level Set of directories whose ignore marker has already been confirmed, skipping repeated existsSync checks for cached directories while preserving current ignore behavior. Apply the change in hooks-core/wiki.mjs, then run scripts/sync-hook-core.mjs to regenerate this copy and verify the synchronization check passes.Source: Learnings
🤖 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.
Major comments:
In `@hooks-core/staleness.mjs`:
- Around line 200-235: Update linkCalls so duplicate declaration names are
treated as ambiguous rather than resolved to the plain-name entry in byName.
Build or reuse a set of names with multiple declarations, and skip matching call
names in that set before putEdge; retain current behavior for uniquely declared
symbols and existing self/seen/keyword filtering.
- Around line 144-166: Update linkImports so creating an import edge cannot
append an incomplete file node that supersedes an existing indexFile record for
the same target. Track visited targets or defer import-node creation until the
target is indexed, while keeping linkImports out of indexFile’s hot path and
preserving valid import edges.
In `@hooks-core/symbols.mjs`:
- Around line 96-158: The import filter in extractImports must support the
syntax captured by the Python and Rust entries in IMPORT_PATTERNS. Make the
relative-import check language-aware, pass the language family into
resolveImport, and translate Python dotted and Rust :: relative specifiers into
path components before joining; otherwise remove those pattern entries so
unsupported imports do not produce unresolved probes.
In `@hooks-core/wiki.mjs`:
- Around line 199-234: Update ignoreSelf to detect whether dir contains a .git
entry before creating its .gitignore marker, and return without writing when it
does. Preserve the existing best-effort behavior for valid nested stores,
including the marker contents and error handling.
In `@integrations/codex/hooks/lib/staleness.mjs`:
- Around line 202-237: Update linkCalls so call-site names resolve only to an
unambiguous declaration, rather than using byName entries keyed by
post-disambiguation names that may include ~N suffixes. Preserve same-file
linking and skip calls when multiple declarations share the textual name,
preventing edges from targeting an arbitrary duplicate.
- Around line 146-168: Update linkImports so creating or retrieving the target
file node cannot replace an existing fully indexed node with a bare { kind:
'file', key: target } stub. Reuse the existing node-preserving behavior or merge
only missing node data when calling putNode, while keeping the imports edge
creation and avoiding recursive indexing.
In `@integrations/codex/hooks/lib/symbols.mjs`:
- Around line 98-160: Update extractImports so Python dotted-relative specifiers
and Rust self::/super:: paths pass the relative-import filter instead of being
discarded. Adjust the language-specific handling in IMPORT_PATTERNS or normalize
its captured values before the filter, while preserving rejection of
non-relative package and namespace imports and keeping Java/C# behavior
unchanged.
In `@integrations/codex/hooks/lib/wiki.mjs`:
- Around line 201-236: Update ignoreSelf to guard against writing the wildcard
.gitignore when TOKEN_OPTIMIZER_WIKI_DIR resolves to the project root or another
user-owned/tracked directory. Reuse the established safety check from
hooks-core/wiki.mjs if available, and preserve best-effort behavior by skipping
the marker write when the directory is not a safe tool-owned store.
In `@integrations/codex/plugin/hooks/lib/staleness.mjs`:
- Around line 146-168: Update linkImports to preserve metadata for an already
indexed import target by obtaining target content and populating its hash before
passing the file node to putNode. Keep this limited to the target file’s content
and do not recurse into its imports or symbols; retain the existing
unresolved-target behavior and edge creation flow.
In `@integrations/codex/plugin/hooks/lib/symbols.mjs`:
- Around line 98-160: Update extractImports and resolveImport so Python relative
module specifiers and Rust self/super module paths are retained and resolved
using language-aware rules rather than filesystem-prefix checks. Convert Python
dots and Rust :: segments into the appropriate relative path representation
before resolving, and ensure the resulting edges target existing files.
Alternatively remove the Python and Rust entries from IMPORT_PATTERNS if these
imports cannot be resolved reliably.
In `@integrations/codex/plugin/hooks/lib/wiki.mjs`:
- Around line 201-236: Update ignoreSelf to validate that the resolved store
directory is safe to modify before creating its .gitignore marker, reusing the
existing protection approach from hooks-core/wiki.mjs. Skip writing the marker
when TOKEN_OPTIMIZER_WIKI_DIR points to a user-owned or otherwise unsafe
directory, while preserving best-effort behavior for valid store paths.
In `@integrations/gemini/hooks/lib/staleness.mjs`:
- Around line 202-237: Update linkCalls so duplicate declarations with the same
name are not resolved through the name-only byName map. Only create calls edges
when the target name maps to exactly one unambiguous symbol, while preserving
the existing same-file, self-call, keyword, and deduplication checks.
- Around line 146-168: Update linkImports so creating the target file node
cannot overwrite an existing fully indexed node or discard its hash/snapshot
metadata. Reuse the existing node when available, or make putNode preserve
previously indexed fields while adding the stub; keep the imports edge and
unresolved-target behavior unchanged.
In `@integrations/gemini/hooks/lib/symbols.mjs`:
- Around line 98-160: Update extractImports and the Python/Rust entries in
IMPORT_PATTERNS so their relative import specifiers pass the filter and produce
edges. Preserve the existing relative-only behavior for JavaScript, while
accepting Python dot imports and Rust self::/super:: paths without admitting
package or namespace imports.
In `@integrations/gemini/hooks/lib/wiki.mjs`:
- Around line 201-236: Update ignoreSelf to guard against
TOKEN_OPTIMIZER_WIKI_DIR targeting a user-owned or otherwise unsafe directory
before creating the internal .gitignore marker. Reuse the established safety
validation from the corresponding hooks-core wiki implementation, and preserve
the best-effort behavior for valid store directories.
In `@integrations/opencode/hooks/lib/staleness.mjs`:
- Around line 146-168: The import-linking logic in linkImports must not
overwrite an already-indexed target node with a hash-less, snapshot-less
placeholder. Update the target-node handling around putNode to reuse the
existing indexed file node or otherwise preserve its hash and snapshot, while
still creating a minimal node when the target has not been indexed.
In `@integrations/opencode/hooks/lib/symbols.mjs`:
- Around line 98-160: Update the relative-import filter in extractImports so it
accepts the specifier formats produced by the py and rust IMPORT_PATTERNS,
including Python dot-prefixed modules and Rust self:: or super:: paths, while
continuing to exclude bare package or namespace imports. Preserve the existing
JavaScript and Go behavior and deduplication.
In `@integrations/opencode/hooks/lib/wiki.mjs`:
- Around line 201-236: Update ignoreSelf to guard against
TOKEN_OPTIMIZER_WIKI_DIR resolving to a user-owned directory before creating the
internal .gitignore marker. Reuse the established safety validation or
trusted-store check from the corresponding hooks-core wiki implementation, and
skip marker creation when the directory is not a known token-optimizer store
while preserving best-effort behavior for valid stores.
In `@integrations/qwen/hooks/lib/staleness.mjs`:
- Around line 202-237: Update linkCalls so call targets are resolved using each
symbol’s disambiguated identity rather than the name-only byName map. Ensure
bare call-site text cannot select the wrong declaration when duplicate names
exist, while preserving edges for unambiguous same-file declarations.
- Around line 146-168: Update linkImports so resolving an import reuses the
existing fully indexed file node for target instead of creating a metadata-free
stub that can overwrite it. Preserve the current behavior of adding the imports
edge and only create or merge a node when no existing node is available,
retaining the target’s hash and snapshot metadata.
In `@integrations/qwen/hooks/lib/symbols.mjs`:
- Around line 98-160: Update extractImports so its relative-import filter
accepts the Python specifiers produced by IMPORT_PATTERNS (dot-prefixed module
names) and the Rust self::/super:: specifiers, while continuing to exclude
package and namespace imports. Keep the existing deduplication and raw-specifier
return behavior unchanged.
In `@integrations/qwen/hooks/lib/wiki.mjs`:
- Around line 201-236: Update ignoreSelf to validate that the target directory
is the managed token-optimizer store before creating its .gitignore marker,
rejecting or skipping user-owned directories configured through
TOKEN_OPTIMIZER_WIKI_DIR. Preserve best-effort behavior so an unsafe or invalid
target does not fail the graph write, and keep the marker scoped strictly inside
the verified store.
In `@plugin/hooks/lib/staleness.mjs`:
- Around line 146-168: Fix the target-node overwrite risk in the shared
hook-core implementation of linkImports, not this generated copy: reuse the
existing indexed file node or otherwise preserve its hash and snapshot data when
creating the import edge instead of writing a placeholder through putNode. Then
run scripts/sync-hook-core.mjs to regenerate all integration copies and verify
the synchronization check passes.
In `@plugin/hooks/lib/symbols.mjs`:
- Around line 98-160: Update the shared extractImports implementation and its
language-specific relative-specifier handling in hooks-core/symbols.mjs so
Python and Rust patterns are accepted without weakening package filtering; do
not edit this generated copy directly. Run scripts/sync-hook-core.mjs to
regenerate integrations/codex and other hook copies, then verify the
synchronization check passes.
---
Nitpick comments:
In `@integrations/codex/plugin/hooks/lib/staleness.mjs`:
- Around line 202-237: Bound the call-target scan in linkCalls to the same
symbolSnapshotLimit() threshold used for snapshots. Before running
body.matchAll, skip or truncate spans exceeding that limit, while preserving
existing same-file, unambiguous edge behavior for bounded spans.
In `@plugin/hooks/lib/wiki.mjs`:
- Around line 237-260: Update the shared hook core’s ignoreSelf/append flow to
maintain a module-level Set of directories whose ignore marker has already been
confirmed, skipping repeated existsSync checks for cached directories while
preserving current ignore behavior. Apply the change in hooks-core/wiki.mjs,
then run scripts/sync-hook-core.mjs to regenerate this copy and verify the
synchronization check passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b518681-df1e-46ff-bb70-1ff52608f0b7
📒 Files selected for processing (21)
hooks-core/staleness.mjshooks-core/symbols.mjshooks-core/wiki.mjsintegrations/codex/hooks/lib/staleness.mjsintegrations/codex/hooks/lib/symbols.mjsintegrations/codex/hooks/lib/wiki.mjsintegrations/codex/plugin/hooks/lib/staleness.mjsintegrations/codex/plugin/hooks/lib/symbols.mjsintegrations/codex/plugin/hooks/lib/wiki.mjsintegrations/gemini/hooks/lib/staleness.mjsintegrations/gemini/hooks/lib/symbols.mjsintegrations/gemini/hooks/lib/wiki.mjsintegrations/opencode/hooks/lib/staleness.mjsintegrations/opencode/hooks/lib/symbols.mjsintegrations/opencode/hooks/lib/wiki.mjsintegrations/qwen/hooks/lib/staleness.mjsintegrations/qwen/hooks/lib/symbols.mjsintegrations/qwen/hooks/lib/wiki.mjsplugin/hooks/lib/staleness.mjsplugin/hooks/lib/symbols.mjsplugin/hooks/lib/wiki.mjs
Contributor
Commit Message Format IssueYour commit messages don't follow the Conventional Commits specification. Required Format:Valid Types:
Examples:Breaking Changes:Add Please amend your commit messages to follow this format. Learn more: Conventional Commits |
Contributor
Performance Benchmark Results |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
integrations/codex/plugin/hooks/lib/harvest.mjs (1)
222-241: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSame local-endpoint API-key leak as
hooks-core/harvest.mjs.Line 241 attaches
x-api-keywheneverkeyis truthy, regardless oflocal. An ambient key still reaches a configured local endpoint. Fix the shared source inhooks-core/harvest.mjsand re-sync this copy.🤖 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 `@integrations/codex/plugin/hooks/lib/harvest.mjs` around lines 222 - 241, Update the shared request-header logic in hooks-core/harvest.mjs so x-api-key is included only for non-local endpoints, then re-sync integrations/codex/plugin/hooks/lib/harvest.mjs from that source. Preserve local requests without authentication headers while retaining the existing remote key behavior.integrations/opencode/hooks/lib/harvest.mjs (1)
222-241: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSame local-endpoint API-key leak as
hooks-core/harvest.mjs.Line 241 attaches
x-api-keywheneverkeyis truthy, regardless oflocal. An ambient key still reaches a configured local endpoint. Fix the shared source inhooks-core/harvest.mjsand re-sync this copy.🤖 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 `@integrations/opencode/hooks/lib/harvest.mjs` around lines 222 - 241, Update the header construction in the harvest request flow so x-api-key is attached only for remote endpoints, never when local is true, even if apiKey() returns a value. Apply the shared fix in hooks-core/harvest.mjs and then re-sync the corresponding logic in this copy, preserving existing local and remote behavior otherwise.integrations/qwen/hooks/lib/harvest.mjs (1)
222-241: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSame local-endpoint API-key leak as
hooks-core/harvest.mjs.Line 241 attaches
x-api-keywheneverkeyis truthy, regardless oflocal. An ambient key still reaches a configured local endpoint. Fix the shared source inhooks-core/harvest.mjsand re-sync this copy.🤖 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 `@integrations/qwen/hooks/lib/harvest.mjs` around lines 222 - 241, Update the request header construction in hooks-core/harvest.mjs so x-api-key is included only for remote endpoints, requiring both a truthy key and !local. Re-sync integrations/qwen/hooks/lib/harvest.mjs from that shared source, preserving local requests without authentication.integrations/gemini/hooks/lib/harvest.mjs (1)
222-241: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSame local-endpoint API-key leak as
hooks-core/harvest.mjs.Line 241 attaches
x-api-keywheneverkeyis truthy, regardless oflocal. An ambient key still reaches a configured local endpoint. Fix the shared source inhooks-core/harvest.mjsand re-sync this copy.🤖 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 `@integrations/gemini/hooks/lib/harvest.mjs` around lines 222 - 241, Update the shared header construction in hooks-core/harvest.mjs so x-api-key is included only for non-local endpoints, even when an ambient key exists; preserve unauthenticated local requests. Then re-sync integrations/gemini/hooks/lib/harvest.mjs from the corrected shared implementation.plugin/hooks/lib/harvest.mjs (1)
222-241: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSame local-endpoint API-key leak as
hooks-core/harvest.mjs, and this is the copy the Stop hook actually runs.
tests/unit/harvest-gating.test.tsandplugin/hooks/stop-harvest.mjsboth consume this exact file at runtime. Line 241 attachesx-api-keywheneverkeyis truthy, regardless oflocal, so a real credential can reach a configured local endpoint. Fix the shared core and re-sync this file; do not patch this copy directly.Based on learnings, generated hook copies should be fixed at the shared source and propagated via
scripts/sync-hook-core.mjs.🤖 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 `@plugin/hooks/lib/harvest.mjs` around lines 222 - 241, Update the shared harvest implementation in hooks-core/harvest.mjs so the request headers never include x-api-key when localEndpoint() is configured, even if apiKey() returns a value; retain the key for remote endpoints. Then regenerate this plugin/hooks/lib/harvest.mjs copy using scripts/sync-hook-core.mjs rather than editing the generated file directly.Source: Learnings
hooks-core/harvest.mjs (1)
220-241: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winLocal requests can still leak the API key in the
x-api-keyheader.
key = apiKey()is computed independently oflocal. When a local endpoint is configured (mode'local') and an ambientANTHROPIC_API_KEYorTOKEN_OPTIMIZER_API_KEYhappens to be set — the exact scenario the code's own comment at lines 76-79 calls out as common — the header spread at line 239 still attachesx-api-keyto the request. This contradicts the design stated in the file's own doc comment ("nothing leaves the machine ... it needs no key") and the PR's stated behavior that local requests omit authentication headers. A real credential is sent to whatever the user configured as "local," including.localmDNS names that another device on the same network can spoof.No test in
tests/unit/harvest-gating.test.tscurrently asserts header contents, so this gap was not caught.🔒 Proposed fix to omit the key header for local requests
headers: { 'content-type': 'application/json', - ...(key ? { 'x-api-key': key } : {}), + ...(!local && key ? { 'x-api-key': key } : {}), 'anthropic-version': '2023-06-01', },🤖 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 `@hooks-core/harvest.mjs` around lines 220 - 241, Update the request header construction in the harvest flow so local endpoints never include the API key: derive the header credential from the existing local check, and only attach x-api-key for non-local requests with a key. Preserve the current gating behavior for local and remote requests while ensuring credentials are omitted from all local fetch calls.integrations/codex/hooks/lib/harvest.mjs (1)
222-241: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSame local-endpoint API-key leak as
hooks-core/harvest.mjs.Line 241's
...(key ? { 'x-api-key': key } : {})does not checklocal, so an ambient key is still attached to requests aimed at a configured local endpoint. This mirrors the same bug inhooks-core/harvest.mjs; the fix belongs in the shared source before it is synced here.🤖 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 `@integrations/codex/hooks/lib/harvest.mjs` around lines 222 - 241, Update the request header construction in the harvest flow to include x-api-key only for non-local endpoints, using the existing local and key values. Preserve unauthenticated local requests and the current authenticated remote behavior, and apply the fix in the shared source that generates this synced hook rather than only editing the generated file.
🧹 Nitpick comments (2)
src/validation/tool-schemas.ts (1)
395-401: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSchema is looser than the documented
wiki_writeinput shape.
WikiWriteSchemavalidates onlyclaimandanchors, with no.min(1)constraint, and.passthrough()letstype,confidence,sessionId, andprojectRootthrough unchecked even thoughWIKI_WRITE_TOOL_DEFINITION.inputSchemadocuments them with specific types and an enum fortype. A caller sending a wrong-typedconfidenceor an out-of-enumtypegets no validation error; it silently falls back to a default insidewikiWrite()instead.Add the documented fields to the schema so the runtime validator matches the advertised contract.
♻️ Proposed refactor
export const WikiWriteSchema = z .object({ - claim: z.string(), - anchors: z.array(z.string()), + claim: z.string().min(8), + anchors: z.array(z.string()).min(1), + type: z.enum(['finding', 'decision', 'failure', 'command', 'map']).optional(), + confidence: z.number().min(0).max(1).optional(), + sessionId: z.string().optional(), + projectRoot: z.string().optional(), }) .passthrough();🤖 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/validation/tool-schemas.ts` around lines 395 - 401, Update WikiWriteSchema to require non-empty claim and anchors, define the documented type, confidence, sessionId, and projectRoot fields with their advertised types and type enum, and remove passthrough behavior so unexpected fields are rejected. Keep the schema aligned with WIKI_WRITE_TOOL_DEFINITION.inputSchema and ensure invalid values fail validation before reaching wikiWrite().tests/unit/harvest-gating.test.ts (1)
114-117: 🩺 Stability & Availability | 🔵 TrivialAdd a regression test for the local-endpoint header leak.
This suite verifies
extract()is a no-op when disabled, but no test mocksfetchto confirm that a local endpoint with an ambientANTHROPIC_API_KEYomits thex-api-keyheader. Add a case that setsTOKEN_OPTIMIZER_HARVEST_ENDPOINTto a local URL, setsANTHROPIC_API_KEY, mocksglobal.fetch, and asserts the request headers do not containx-api-key. This would have caught the header leak flagged inplugin/hooks/lib/harvest.mjs.🤖 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 `@tests/unit/harvest-gating.test.ts` around lines 114 - 117, Add a regression test alongside the existing harvest tests that configures a local TOKEN_OPTIMIZER_HARVEST_ENDPOINT and an ambient ANTHROPIC_API_KEY, mocks global.fetch, invokes extract(), and verifies the request headers omit x-api-key. Keep the assertion focused on the local-endpoint header behavior and restore the modified environment and fetch mock afterward.
🤖 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 `@hooks-core/curate.mjs`:
- Around line 33-53: Update the provenance and ranking paths for the new agent
origin: in exportMarkdown, add an ORIGIN_AGENT branch that emits the documented
agent tag, and in the weighted sorting logic, apply AGENT_WEIGHT for agent
findings instead of the fallback weight of 1. Keep the existing human and
harvested behavior unchanged.
In `@hooks-core/harvest.mjs`:
- Around line 43-61: Update the IPv6 loopback comparison in localEndpoint() to
match URL.hostname’s bracketed form, using '[::1]' instead of '::1', while
preserving the other local-host checks and fallback behavior.
In `@integrations/codex/hooks/lib/curate.mjs`:
- Around line 35-44: Update exportMarkdown in
integrations/codex/hooks/lib/curate.mjs (lines 35-44),
integrations/gemini/hooks/lib/curate.mjs (lines 35-44), and
integrations/qwen/hooks/lib/curate.mjs (lines 35-44) to add the agent tag when
finding.origin equals ORIGIN_AGENT, while preserving the existing human-tag
behavior.
In `@integrations/codex/hooks/lib/harvest-write.mjs`:
- Around line 81-88: Update key generation in writeHarvested, where the key
combines prefix, Date.now(), and written.length, to prevent collisions between
concurrent writers. Use a collision-resistant per-finding identifier or the
graph’s atomic sequence mechanism so distinct findings always receive distinct
keys and putNode cannot overwrite another writer’s node.
- Around line 82-91: Update the finding creation flow around putNode and the
subsequent putEdge loop so the finding node and all required derived_from edges
are persisted atomically. Use the existing transaction or batch-append mechanism
if available, ensuring partial writes cannot leave an active finding without its
resolved anchors.
In `@integrations/codex/hooks/lib/harvest.mjs`:
- Around line 45-63: Update the IPv6 loopback comparison in localEndpoint() to
match URL.hostname’s bracketed “[::1]” value, then apply the same change to
every repeated or generated harvest.mjs hook copy so endpoints using
http://[::1]:PORT are recognized as local.
In `@integrations/codex/plugin/hooks/lib/harvest.mjs`:
- Around line 45-63: Update localEndpoint() to recognize bracketed IPv6
hostnames returned by URL.hostname, including [::1], while preserving the
existing local-host checks and remote endpoint behavior. Apply the corresponding
change to the synchronized hooks-core/harvest.mjs copy, then run npm run
sync:hooks.
In `@integrations/gemini/hooks/lib/harvest.mjs`:
- Around line 45-63: Update localEndpoint to compare URL.hostname against
'[::1]' rather than '::1' so IPv6 localhost endpoints are recognized. Apply this
fix in the shared hooks-core harvest source, then regenerate all generated hook
copies.
In `@integrations/qwen/hooks/lib/harvest.mjs`:
- Around line 45-63: Update localEndpoint() in the shared hooks-core source to
recognize URL.hostname’s bracketed IPv6 loopback value ([::1]) alongside the
existing local host checks, then propagate the corrected implementation to the
generated harvest.mjs output so IPv6 loopback endpoints return the configured
endpoint.
In `@plugin/hooks/lib/harvest.mjs`:
- Around line 45-63: Update the IPv6 loopback comparison in
hooks-core/harvest.mjs so URL hostnames formatted as [::1] are recognized by
localEndpoint() as local; then run npm run sync:hooks to regenerate
plugin/hooks/lib/harvest.mjs and other generated copies, without editing
generated files directly.
In `@src/tools/intelligence/wiki-write.ts`:
- Around line 113-136: Update the unresolvedAnchors computation in the
surrounding wiki write flow to run unconditionally, removing the keys.length
guard so every anchor that does not resolve to a graph node is reported.
Preserve the existing node ID resolution and success/error behavior based on
keys.length.
---
Outside diff comments:
In `@hooks-core/harvest.mjs`:
- Around line 220-241: Update the request header construction in the harvest
flow so local endpoints never include the API key: derive the header credential
from the existing local check, and only attach x-api-key for non-local requests
with a key. Preserve the current gating behavior for local and remote requests
while ensuring credentials are omitted from all local fetch calls.
In `@integrations/codex/hooks/lib/harvest.mjs`:
- Around line 222-241: Update the request header construction in the harvest
flow to include x-api-key only for non-local endpoints, using the existing local
and key values. Preserve unauthenticated local requests and the current
authenticated remote behavior, and apply the fix in the shared source that
generates this synced hook rather than only editing the generated file.
In `@integrations/codex/plugin/hooks/lib/harvest.mjs`:
- Around line 222-241: Update the shared request-header logic in
hooks-core/harvest.mjs so x-api-key is included only for non-local endpoints,
then re-sync integrations/codex/plugin/hooks/lib/harvest.mjs from that source.
Preserve local requests without authentication headers while retaining the
existing remote key behavior.
In `@integrations/gemini/hooks/lib/harvest.mjs`:
- Around line 222-241: Update the shared header construction in
hooks-core/harvest.mjs so x-api-key is included only for non-local endpoints,
even when an ambient key exists; preserve unauthenticated local requests. Then
re-sync integrations/gemini/hooks/lib/harvest.mjs from the corrected shared
implementation.
In `@integrations/opencode/hooks/lib/harvest.mjs`:
- Around line 222-241: Update the header construction in the harvest request
flow so x-api-key is attached only for remote endpoints, never when local is
true, even if apiKey() returns a value. Apply the shared fix in
hooks-core/harvest.mjs and then re-sync the corresponding logic in this copy,
preserving existing local and remote behavior otherwise.
In `@integrations/qwen/hooks/lib/harvest.mjs`:
- Around line 222-241: Update the request header construction in
hooks-core/harvest.mjs so x-api-key is included only for remote endpoints,
requiring both a truthy key and !local. Re-sync
integrations/qwen/hooks/lib/harvest.mjs from that shared source, preserving
local requests without authentication.
In `@plugin/hooks/lib/harvest.mjs`:
- Around line 222-241: Update the shared harvest implementation in
hooks-core/harvest.mjs so the request headers never include x-api-key when
localEndpoint() is configured, even if apiKey() returns a value; retain the key
for remote endpoints. Then regenerate this plugin/hooks/lib/harvest.mjs copy
using scripts/sync-hook-core.mjs rather than editing the generated file
directly.
---
Nitpick comments:
In `@src/validation/tool-schemas.ts`:
- Around line 395-401: Update WikiWriteSchema to require non-empty claim and
anchors, define the documented type, confidence, sessionId, and projectRoot
fields with their advertised types and type enum, and remove passthrough
behavior so unexpected fields are rejected. Keep the schema aligned with
WIKI_WRITE_TOOL_DEFINITION.inputSchema and ensure invalid values fail validation
before reaching wikiWrite().
In `@tests/unit/harvest-gating.test.ts`:
- Around line 114-117: Add a regression test alongside the existing harvest
tests that configures a local TOKEN_OPTIMIZER_HARVEST_ENDPOINT and an ambient
ANTHROPIC_API_KEY, mocks global.fetch, invokes extract(), and verifies the
request headers omit x-api-key. Keep the assertion focused on the local-endpoint
header behavior and restore the modified environment and fetch mock afterward.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9583f60a-a222-4724-a5cf-89e906553055
📒 Files selected for processing (27)
hooks-core/curate.mjshooks-core/harvest-write.mjshooks-core/harvest.mjsintegrations/codex/hooks/lib/curate.mjsintegrations/codex/hooks/lib/harvest-write.mjsintegrations/codex/hooks/lib/harvest.mjsintegrations/codex/plugin/hooks/lib/curate.mjsintegrations/codex/plugin/hooks/lib/harvest-write.mjsintegrations/codex/plugin/hooks/lib/harvest.mjsintegrations/gemini/hooks/lib/curate.mjsintegrations/gemini/hooks/lib/harvest-write.mjsintegrations/gemini/hooks/lib/harvest.mjsintegrations/opencode/hooks/lib/curate.mjsintegrations/opencode/hooks/lib/harvest-write.mjsintegrations/opencode/hooks/lib/harvest.mjsintegrations/qwen/hooks/lib/curate.mjsintegrations/qwen/hooks/lib/harvest-write.mjsintegrations/qwen/hooks/lib/harvest.mjsintegrations/windsurf/mcp_config.jsonplugin/hooks/lib/curate.mjsplugin/hooks/lib/harvest-write.mjsplugin/hooks/lib/harvest.mjsplugin/hooks/stop-harvest.mjssrc/server/index.tssrc/tools/intelligence/wiki-write.tssrc/validation/tool-schemas.tstests/unit/harvest-gating.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- integrations/qwen/hooks/lib/harvest-write.mjs
- plugin/hooks/stop-harvest.mjs
ooples
added a commit
that referenced
this pull request
Aug 3, 2026
…ifecycle Seven findings from the review on #229, all valid on inspection. ANCHOR CONFINEMENT. resolveAnchor called indexFile on any path a finding named, and indexFile READS the file and stores a snapshot of it in the graph -- so an anchor is a read primitive. The findings come from a model reading a transcript, so those paths are untrusted: a claim naming ../../.ssh/id_rsa would have copied it into .token-optimizer and served it back on the next touch. Anchors are now confined to the project root, compared with a trailing separator so /repo-secrets is not read as inside /repo. wiki_write passes the project it resolved. KEY COLLISIONS. `harvested-<ms>-<index>` is not unique across CONCURRENT detached workers: two Stop hooks finishing in the same millisecond produce the same key, and putNode keeps the last write for an id -- so one session's finding silently replaced another's. Four random bytes added. NOTICE PATH. The session id arrives in the hook payload and was interpolated straight into a path, so separators or `..` placed the marker anywhere on disk. Now sanitised to an allowlist, under os.tmpdir() rather than a TEMP/TMPDIR chain that fell back to `.` and scattered markers through whatever repository the session was in. STDOUT TRUNCATION. The off-notice was written and then process.exit(0) ran in the finally, which discards anything still buffered -- so on a filled pipe the one message explaining why no findings exist was the most likely thing to be dropped. The write is awaited (callback, or drain when buffered) and the process now sets exitCode and terminates naturally. DEBOUNCE. Stop fires at the end of every assistant turn, so a talkative session spawned a model call per turn, each re-reading an overlapping transcript. Now rate-limited per session, with the marker touched only when a harvest actually starts so skipped turns do not push the next one further away. IPv6 LOOPBACK. URL.hostname returns IPv6 literals BRACKETED, so the bare '::1' comparison never matched and http://[::1]:11434 was treated as remote -- the loopback spelling a user is most likely to copy from a server that printed it. PROVENANCE, AND A PRE-EXISTING GAP IT EXPOSED. ORIGIN_AGENT was added without an export tag or any ranking effect. Wiring it revealed that HUMAN_WEIGHT has been declared since curate.mjs was written and consumed by nothing: a person's correction ranked exactly level with a machine guess of equal confidence, which is the one thing the origin field exists to prevent. The single ranking function now applies human 1.5 / agent 1.2 / harvested 1. UNRESOLVED ANCHORS were computed only when NOTHING was written, so a finding with three anchors of which one is a typo was stored against the other two, reported as a success, and said nothing about the anchor that will never invalidate it. Now reported unconditionally. Verified against the built server: an out-of-project anchor is refused; a partial write reports written=1 with the unresolved anchor named; http://[::1]:11434 resolves as local while api.anthropic.com stays opt-in. 99 suites / 1417 tests.
Contributor
Commit Message Format IssueYour commit messages don't follow the Conventional Commits specification. Required Format:Valid Types:
Examples:Breaking Changes:Add Please amend your commit messages to follow this format. Learn more: Conventional Commits |
Contributor
Performance Benchmark Results |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
integrations/codex/plugin/hooks/lib/wiki.mjs (1)
201-236: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse lock-confined initialization for the store-local
.gitignore.
append(dir)callsignoreSelf(dir)beforewithLock(dir, ...). A normalexistsSync()/write can let concurrent appends create or append to.gitignoreafter the check, so the marker creation must use an exclusive write or happen under the same lock.🤖 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 `@integrations/codex/plugin/hooks/lib/wiki.mjs` around lines 201 - 236, Move the ignoreSelf(dir) invocation from before withLock(dir, ...) into the lock-protected append flow, or otherwise make marker creation atomic with exclusive file creation. Update ignoreSelf so concurrent appends cannot both pass existsSync(marker) and write the store-local .gitignore, while preserving its best-effort behavior and avoiding failure of the graph write.plugin/hooks/stop-harvest.mjs (1)
162-179: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle detached worker startup errors.
If
spawn()fails asynchronously, thechildprocess emits anerrorevent and exits immediately. Add anerrorlistener beforeunref()and clear the debounce marker for this failure, or record it only after worker startup succeeds.🤖 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 `@plugin/hooks/stop-harvest.mjs` around lines 162 - 179, The detached worker started in the stop hook does not handle asynchronous spawn failures. Add an error listener to the child returned by spawn before calling unref(), and clear the debounce marker when startup emits an error so the failed attempt does not suppress the next harvest.
🤖 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 `@integrations/codex/hooks/lib/harvest-write.mjs`:
- Around line 47-59: Resolve relative anchors against projectRoot before the
containment check in resolveAnchor by passing projectRoot || undefined to
canonicalPath. Apply this change in
integrations/codex/hooks/lib/harvest-write.mjs (lines 47-59), then run npm run
sync:hooks to update the generated copies in
integrations/codex/plugin/hooks/lib/harvest-write.mjs (lines 47-59),
integrations/gemini/hooks/lib/harvest-write.mjs (lines 47-59),
integrations/opencode/hooks/lib/harvest-write.mjs (lines 47-59),
integrations/qwen/hooks/lib/harvest-write.mjs (lines 47-59), and
plugin/hooks/lib/harvest-write.mjs (lines 47-59).
In `@plugin/hooks/stop-harvest.mjs`:
- Around line 93-97: Update the HARVEST_INTERVAL_MS configuration validation to
require the parsed TOKEN_OPTIMIZER_HARVEST_INTERVAL_MS value to be finite and
greater than zero before accepting it. Retain the existing 10-minute fallback
for zero, negative, NaN, and other invalid non-finite values.
- Around line 67-78: Update noticePath() and the debounce marker path handling
to use per-user private state storage instead of the shared os.tmpdir()
location. Ensure both notice and debounce writes reject existing marker paths
unless they are owned by the current user and are regular non-symlinked files,
preventing writeFileSync() from following attacker-controlled links.
---
Outside diff comments:
In `@integrations/codex/plugin/hooks/lib/wiki.mjs`:
- Around line 201-236: Move the ignoreSelf(dir) invocation from before
withLock(dir, ...) into the lock-protected append flow, or otherwise make marker
creation atomic with exclusive file creation. Update ignoreSelf so concurrent
appends cannot both pass existsSync(marker) and write the store-local
.gitignore, while preserving its best-effort behavior and avoiding failure of
the graph write.
In `@plugin/hooks/stop-harvest.mjs`:
- Around line 162-179: The detached worker started in the stop hook does not
handle asynchronous spawn failures. Add an error listener to the child returned
by spawn before calling unref(), and clear the debounce marker when startup
emits an error so the failed attempt does not suppress the next harvest.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 66507b72-2550-4c43-aa03-23b0f499d70c
📒 Files selected for processing (30)
hooks-core/curate.mjshooks-core/harvest-write.mjshooks-core/harvest.mjshooks-core/wiki.mjsintegrations/codex/hooks/lib/curate.mjsintegrations/codex/hooks/lib/harvest-write.mjsintegrations/codex/hooks/lib/harvest.mjsintegrations/codex/hooks/lib/wiki.mjsintegrations/codex/plugin/hooks/lib/curate.mjsintegrations/codex/plugin/hooks/lib/harvest-write.mjsintegrations/codex/plugin/hooks/lib/harvest.mjsintegrations/codex/plugin/hooks/lib/wiki.mjsintegrations/gemini/hooks/lib/curate.mjsintegrations/gemini/hooks/lib/harvest-write.mjsintegrations/gemini/hooks/lib/harvest.mjsintegrations/gemini/hooks/lib/wiki.mjsintegrations/opencode/hooks/lib/curate.mjsintegrations/opencode/hooks/lib/harvest-write.mjsintegrations/opencode/hooks/lib/harvest.mjsintegrations/opencode/hooks/lib/wiki.mjsintegrations/qwen/hooks/lib/curate.mjsintegrations/qwen/hooks/lib/harvest-write.mjsintegrations/qwen/hooks/lib/harvest.mjsintegrations/qwen/hooks/lib/wiki.mjsplugin/hooks/lib/curate.mjsplugin/hooks/lib/harvest-write.mjsplugin/hooks/lib/harvest.mjsplugin/hooks/lib/wiki.mjsplugin/hooks/stop-harvest.mjssrc/tools/intelligence/wiki-write.ts
🚧 Files skipped from review as they are similar to previous changes (21)
- integrations/gemini/hooks/lib/curate.mjs
- hooks-core/curate.mjs
- plugin/hooks/lib/wiki.mjs
- integrations/qwen/hooks/lib/wiki.mjs
- integrations/opencode/hooks/lib/curate.mjs
- integrations/opencode/hooks/lib/wiki.mjs
- src/tools/intelligence/wiki-write.ts
- integrations/opencode/hooks/lib/harvest.mjs
- integrations/qwen/hooks/lib/harvest.mjs
- integrations/codex/plugin/hooks/lib/curate.mjs
- integrations/qwen/hooks/lib/curate.mjs
- plugin/hooks/lib/curate.mjs
- integrations/codex/hooks/lib/curate.mjs
- integrations/codex/plugin/hooks/lib/harvest.mjs
- integrations/gemini/hooks/lib/wiki.mjs
- hooks-core/harvest.mjs
- integrations/codex/hooks/lib/wiki.mjs
- plugin/hooks/lib/harvest.mjs
- hooks-core/harvest-write.mjs
- integrations/codex/hooks/lib/harvest.mjs
- integrations/gemini/hooks/lib/harvest.mjs
Contributor
Commit Message Format IssueYour commit messages don't follow the Conventional Commits specification. Required Format:Valid Types:
Examples:Breaking Changes:Add Please amend your commit messages to follow this format. Learn more: Conventional Commits |
Contributor
Performance Benchmark Results |
Contributor
🔒 Security Audit Results
Run |
…i key
Two changes to who may write findings and what that is allowed to cost.
1. wiki_write -- the agent records a conclusion while it still holds the context.
The design named this tool ("Model-invoked `wiki_write` exists for the agent to
record something deliberately") and it was never built, so the only route into
the graph was a post-hoc extraction that was itself never wired. A real project
after a full session: 122 file nodes, 132 symbol nodes, 61 task nodes, and zero
findings.
The design's objection to agent writes was that "anything opt-in does not
happen". True of a suggestion -- but this project answered exactly that problem
elsewhere by ENFORCING tool routing rather than advising it, and a write that
costs nothing and sends nothing is the cheapest possible path into the graph.
The two are complements: this captures what the working session concluded, and
the harvest, when enabled, catches what it forgot.
Anchors are required and load-bearing: an unanchored finding can never be
invalidated, so it would be served as current forever. A finding whose anchors
resolve to nothing is refused and the unresolved paths are named back.
Provenance gets a third origin rather than being folded into an existing one.
`harvested` means a cheap model inferred a claim from a transcript afterwards;
`agent` means the session that did the reasoning recorded its own conclusion.
Those deserve different trust, and collapsing them destroys the calibration the
origin field exists to provide. Weight sits between harvested and human.
2. An ambient credential is not consent.
before: harvestEnabled = Boolean(apiKey()) && MODE !== 'off'
ANTHROPIC_API_KEY is already set in most development environments, so that rule
would have started a billed third-party call -- carrying a digest of the user's
prompts, commands and conclusions off the machine -- the moment this shipped,
without anyone choosing it.
The rule now separates the two things that actually differ, cost and disclosure,
instead of treating "a key exists" as permission:
local endpoint -> free and private, so on by default, no key required
anything else -> deliberate TOKEN_OPTIMIZER_HARVEST opt-in AND a key
`harvestMode()` returns why rather than just whether, because "off" needs a
reason a user can act on: off:mode, off:not-opted-in, off:no-key, local, remote.
The Stop hook reports that reason once per session, names the variable that
changes it, and says what still works meanwhile -- previously it failed silently,
which is why a graph with no findings gave no clue why.
extract() sends no auth header when the endpoint is local, so the free path is
actually reachable rather than blocked on a key it does not need.
Verified end to end through the built server: wiki_write stores with
origin=agent, resolves the project from the anchor's repository rather than the
session cwd, accepts path#symbol anchors, refuses a missing claim via the
published-schema guard, and the finding comes back on the next read of that file
through forTouch. Gating matrix covered by 8 tests, including that a bare
ANTHROPIC_API_KEY does not enable billing and that "localhost.attacker.example"
is not treated as local.
99 suites / 1417 tests green; tsc clean.
…ifecycle Seven findings from the review on #229, all valid on inspection. ANCHOR CONFINEMENT. resolveAnchor called indexFile on any path a finding named, and indexFile READS the file and stores a snapshot of it in the graph -- so an anchor is a read primitive. The findings come from a model reading a transcript, so those paths are untrusted: a claim naming ../../.ssh/id_rsa would have copied it into .token-optimizer and served it back on the next touch. Anchors are now confined to the project root, compared with a trailing separator so /repo-secrets is not read as inside /repo. wiki_write passes the project it resolved. KEY COLLISIONS. `harvested-<ms>-<index>` is not unique across CONCURRENT detached workers: two Stop hooks finishing in the same millisecond produce the same key, and putNode keeps the last write for an id -- so one session's finding silently replaced another's. Four random bytes added. NOTICE PATH. The session id arrives in the hook payload and was interpolated straight into a path, so separators or `..` placed the marker anywhere on disk. Now sanitised to an allowlist, under os.tmpdir() rather than a TEMP/TMPDIR chain that fell back to `.` and scattered markers through whatever repository the session was in. STDOUT TRUNCATION. The off-notice was written and then process.exit(0) ran in the finally, which discards anything still buffered -- so on a filled pipe the one message explaining why no findings exist was the most likely thing to be dropped. The write is awaited (callback, or drain when buffered) and the process now sets exitCode and terminates naturally. DEBOUNCE. Stop fires at the end of every assistant turn, so a talkative session spawned a model call per turn, each re-reading an overlapping transcript. Now rate-limited per session, with the marker touched only when a harvest actually starts so skipped turns do not push the next one further away. IPv6 LOOPBACK. URL.hostname returns IPv6 literals BRACKETED, so the bare '::1' comparison never matched and http://[::1]:11434 was treated as remote -- the loopback spelling a user is most likely to copy from a server that printed it. PROVENANCE, AND A PRE-EXISTING GAP IT EXPOSED. ORIGIN_AGENT was added without an export tag or any ranking effect. Wiring it revealed that HUMAN_WEIGHT has been declared since curate.mjs was written and consumed by nothing: a person's correction ranked exactly level with a machine guess of equal confidence, which is the one thing the origin field exists to prevent. The single ranking function now applies human 1.5 / agent 1.2 / harvested 1. UNRESOLVED ANCHORS were computed only when NOTHING was written, so a finding with three anchors of which one is a typo was stored against the other two, reported as a success, and said nothing about the anchor that will never invalidate it. Now reported unconditionally. Verified against the built server: an out-of-project anchor is refused; a partial write reports written=1 with the unresolved anchor named; http://[::1]:11434 resolves as local while api.anthropic.com stays opt-in. 99 suites / 1417 tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ooples
force-pushed
the
fix/semantic-harvest-never-invoked
branch
from
August 4, 2026 00:15
37e77cb to
3b15214
Compare
Contributor
🔒 Security Audit Results
Run |
Contributor
Performance Benchmark Results |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/harvest-gating.test.ts (1)
65-71: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd coverage for the
TOKEN_OPTIMIZER_API_KEYpath.The suite clears and restores
TOKEN_OPTIMIZER_API_KEY, but every enabled remote case sets onlyANTHROPIC_API_KEY. Add a case withTOKEN_OPTIMIZER_API_KEYset andANTHROPIC_API_KEYunset. AssertharvestMode()isremoteandharvestEnabled()istrue. This protects both credential paths described by the PR objective.Suggested test
+ it('runs with TOKEN_OPTIMIZER_API_KEY when ANTHROPIC_API_KEY is absent', async () => { + delete process.env.ANTHROPIC_API_KEY; + process.env.TOKEN_OPTIMIZER_API_KEY = 'tok-xxx'; + process.env.TOKEN_OPTIMIZER_HARVEST = '1'; + const { harvestMode, harvestEnabled } = await harvest(); + expect(harvestMode()).toBe('remote'); + expect(harvestEnabled()).toBe(true); + });🤖 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 `@tests/unit/harvest-gating.test.ts` around lines 65 - 71, Add a test in the harvest gating suite covering the alternate credential path: set TOKEN_OPTIMIZER_API_KEY and TOKEN_OPTIMIZER_HARVEST while leaving ANTHROPIC_API_KEY unset, then assert harvestMode() returns remote and harvestEnabled() returns true. Keep the existing ANTHROPIC_API_KEY coverage unchanged.
🤖 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 `@tests/unit/harvest-gating.test.ts`:
- Around line 114-117: Update the disabled-extraction test around harvest() and
extract() to mock the transport dependency used by extract, invoke extract while
harvesting is disabled, and assert that the transport was called zero times in
addition to preserving the empty-array assertion.
---
Nitpick comments:
In `@tests/unit/harvest-gating.test.ts`:
- Around line 65-71: Add a test in the harvest gating suite covering the
alternate credential path: set TOKEN_OPTIMIZER_API_KEY and
TOKEN_OPTIMIZER_HARVEST while leaving ANTHROPIC_API_KEY unset, then assert
harvestMode() returns remote and harvestEnabled() returns true. Keep the
existing ANTHROPIC_API_KEY coverage 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ba1c35a3-525f-40de-a51b-ebad3e5342a0
📒 Files selected for processing (34)
hooks-core/curate.mjshooks-core/harvest-write.mjshooks-core/harvest.mjshooks-core/wiki.mjsintegrations/codex/hooks/lib/curate.mjsintegrations/codex/hooks/lib/harvest-write.mjsintegrations/codex/hooks/lib/harvest.mjsintegrations/codex/hooks/lib/wiki.mjsintegrations/codex/plugin/hooks/lib/curate.mjsintegrations/codex/plugin/hooks/lib/harvest-write.mjsintegrations/codex/plugin/hooks/lib/harvest.mjsintegrations/codex/plugin/hooks/lib/wiki.mjsintegrations/gemini/hooks/lib/curate.mjsintegrations/gemini/hooks/lib/harvest-write.mjsintegrations/gemini/hooks/lib/harvest.mjsintegrations/gemini/hooks/lib/wiki.mjsintegrations/opencode/hooks/lib/curate.mjsintegrations/opencode/hooks/lib/harvest-write.mjsintegrations/opencode/hooks/lib/harvest.mjsintegrations/opencode/hooks/lib/wiki.mjsintegrations/qwen/hooks/lib/curate.mjsintegrations/qwen/hooks/lib/harvest-write.mjsintegrations/qwen/hooks/lib/harvest.mjsintegrations/qwen/hooks/lib/wiki.mjsintegrations/windsurf/mcp_config.jsonplugin/hooks/lib/curate.mjsplugin/hooks/lib/harvest-write.mjsplugin/hooks/lib/harvest.mjsplugin/hooks/lib/wiki.mjsplugin/hooks/stop-harvest.mjssrc/server/index.tssrc/tools/intelligence/wiki-write.tssrc/validation/tool-schemas.tstests/unit/harvest-gating.test.ts
🚧 Files skipped from review as they are similar to previous changes (33)
- src/validation/tool-schemas.ts
- src/server/index.ts
- integrations/windsurf/mcp_config.json
- integrations/qwen/hooks/lib/wiki.mjs
- hooks-core/harvest-write.mjs
- integrations/codex/hooks/lib/curate.mjs
- plugin/hooks/lib/curate.mjs
- integrations/gemini/hooks/lib/wiki.mjs
- integrations/opencode/hooks/lib/curate.mjs
- integrations/opencode/hooks/lib/harvest.mjs
- hooks-core/curate.mjs
- integrations/gemini/hooks/lib/harvest-write.mjs
- integrations/codex/plugin/hooks/lib/curate.mjs
- integrations/codex/hooks/lib/harvest-write.mjs
- hooks-core/wiki.mjs
- plugin/hooks/lib/wiki.mjs
- src/tools/intelligence/wiki-write.ts
- integrations/qwen/hooks/lib/harvest.mjs
- plugin/hooks/stop-harvest.mjs
- integrations/codex/plugin/hooks/lib/wiki.mjs
- integrations/codex/hooks/lib/wiki.mjs
- hooks-core/harvest.mjs
- integrations/codex/hooks/lib/harvest.mjs
- integrations/opencode/hooks/lib/wiki.mjs
- integrations/codex/plugin/hooks/lib/harvest-write.mjs
- integrations/opencode/hooks/lib/harvest-write.mjs
- integrations/gemini/hooks/lib/harvest.mjs
- integrations/qwen/hooks/lib/harvest-write.mjs
- integrations/codex/plugin/hooks/lib/harvest.mjs
- plugin/hooks/lib/harvest-write.mjs
- integrations/qwen/hooks/lib/curate.mjs
- integrations/gemini/hooks/lib/curate.mjs
- plugin/hooks/lib/harvest.mjs
Three review findings from #229, all in the path that persists a harvested or agent-written claim. ONE APPEND FOR A FINDING AND ITS ANCHORS. writeHarvested wrote the finding node, then looped putEdge for each resolved anchor. Every append is a separate syscall and the harvest runs in a DETACHED worker that a session end or a sleeping machine can kill between any two of them, so the surviving record could be a finding node with no derived_from edge. That is exactly the record this module refuses to create on purpose -- it drops a finding whose anchors all failed to resolve, because an unanchored claim can never be invalidated by staleness and is therefore served as current forever. putNodeWithEdges writes the group in a single appendFileSync inside a single lock, and orders the NODE LAST. The ordering is the real guarantee: one write can still tear, so what matters is that the surviving prefix is safe. A tear leaves edges whose `from` names no node, and every consumer already dereferences that through a nodes.get(...) guard (findingsFor, audit, sweep), so a dangling edge is inert. The reverse order has no such property. A torn final line is discarded by load(), which skips unparseable records. Tested by truncating the log at every byte offset in turn and asserting that no cut point yields a finding without an anchor. MARKERS MOVE OUT OF SHARED TEMP. stop-harvest kept its once-per-session notice flag and its debounce timestamp in os.tmpdir()/token-optimizer. That is per-user on Windows but a shared, world-writable /tmp on POSIX, and the names derive from the session id, so another local account can predict the path and pre-create it as a symlink -- writeFileSync follows symlinks, so the write lands wherever the link points with this user's privileges. The state is a flag and a timestamp, so the fix is to stop using a shared directory rather than to guard one: XDG_STATE_HOME (LOCALAPPDATA on Windows), created 0o700. Writes additionally go through O_NOFOLLOW with an lstat ownership check, which makes the refusal atomic rather than a check-then-use window. THE DISABLED-HARVEST TEST NOW ASSERTS ON THE TRANSPORT. It checked only that extract() resolves to [], which passes whether extract declined to run or fired a request that failed. Those are opposite outcomes -- one spends nothing and discloses nothing, the other bills a call and ships a digest of the user's prompts off the machine -- and that difference is the entire point of the module. It now stubs fetch and asserts zero calls, paired with a positive test asserting exactly one call once a local endpoint enables it, so zero is meaningful rather than vacuous. 101 suites, 1426 tests, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
🔒 Security Audit Results
Run |
Contributor
Performance Benchmark Results |
Contributor
|
This issue has been resolved in version 5.3.6. Release notes: https://github.com/ooples/token-optimizer-mcp/releases/tag/v5.3.6 |
This was referenced Aug 4, 2026
Merged
7 tasks done
hamsterish
pushed a commit
to hamsterish/token-optimizer-mcp
that referenced
this pull request
Oct 2, 2026
…ples#229) * fix(wiki): the semantic harvest was implemented but never invoked The wiki design specifies findings extracted "at Stop and PreCompact" by a cheap model, out of band. hooks/lib/harvest.mjs implements that -- digest, model call, validation -- and was imported by NOTHING. The only match for its own filename across the repo is its header comment. Stop carried no token-optimizer hook at all, and precompact-optimize.mjs makes no reference to harvesting. The failure was silent rather than loud, which is why it survived: the graph kept filling with its structural layer while the semantic layer never ran. Measured on a real project after a full session of heavy work: node kinds file 122, symbol 132, task 61, finding 0 edges derived_from 61, contains 132 metrics injections 0, harvestTokens 0 "insufficient data (0 treated, 0 holdout; need 20 and 5)" Everything downstream -- inject, staleness, skeleton, restore, curate, and the treated-vs-holdout metrics -- was wired and idle, waiting on findings that could not arrive. The whole value proposition of the graph (retrieve verdicts rather than re-derive them, and record dead ends that exist nowhere in the source tree) was unreachable. Three pieces were missing: 1. A writer. harvest.mjs validates findings but nothing stored them, and curate.create() could not be reused: it stamps origin ORIGIN_HUMAN and a `human-` key because it exists for a person asserting something. Routing machine output through it would label a model's guess as a human assertion -- the exact confusion curate.mjs warns destroys trust calibration. So harvest-write.mjs writes with ORIGIN_HARVESTED, copying create()'s anchor discipline: index the file first, and refuse a finding whose anchors do not resolve rather than store an un-invalidatable claim. 2. An invocation. stop-harvest.mjs runs at Stop and spawns a DETACHED worker, because the extraction is a model call and doing it inline would make the session that did the work pay for summarising itself -- the cost this subsystem exists to avoid. Stop is never delayed. 3. A voice. harvestEnabled() returns false without an API key and the module skips silently; nothing in doctor, audit or waste mentions harvest, so a user sees a graph with no findings and cannot learn why. The Stop hook now says so once per session, names the variable, and states that the structural layer still works. Verified end to end: writeHarvested stored 2 findings with origin=harvested and types failure/decision, auto-created their file and symbol anchor nodes, and REFUSED a third whose anchor did not exist. The Stop hook fires, reports the disabled state once, and stays silent on the second call. Note the remaining precondition: extraction needs TOKEN_OPTIMIZER_API_KEY or ANTHROPIC_API_KEY. Without one this now reports that fact instead of hiding it. 99 suites / 1423 tests green; tsc clean. * fix(wiki): unbounded symbol snapshots, missing edges, and an unignored store Three structural defects measured on one real project's graph, all of which made the wiki cost more and reach less than its design says it should. 1. SYMBOL SNAPSHOTS WERE UNBOUNDED. File snapshots have been capped at 256 KB since P2. Symbol spans were stored unconditionally, on the stated reasoning that "a function is small". That holds for hand-written code and fails badly on generated sources, where one class body is megabytes. before 132 symbol nodes holding 34.3 MB -- 95.8% of a 35 MB graph largest single span 1.8 MB file nodes, already capped, held 0.7 MB The graph had become a second and worse copy of the repository. Symbol spans now take the same cap the file path already had (32 KB, TOKEN_OPTIMIZER_SYMBOL_ SNAPSHOT_LIMIT). Same trade as the file cap: the hash is still stored, so staleness detection is untouched and only the reconstructed diff degrades -- which `serve` already reports plainly. Verified: a 170 KB span is omitted while its hash is kept, and a small sibling is still stored in full. 2. `imports` AND `calls` EDGES WERE NEVER WRITTEN. Of the eight declared edge kinds, only derived_from, contains, supersedes (human correction only) and related were produced anywhere. The measured graph contained exactly two: derived_from 61, contains 132. That leaves the graph as disconnected stars -- a file and the symbols inside it -- with no way to cross a file boundary. Retrieval is traversal by design, with no embedding index as the deliberate trade, so a finding one hop from the current work was simply unreachable. "This symbol AND its callers, causally correct" is the design's stated advantage over RAG and it could not happen. Both are now written during structural indexing, which already parses the file: imports file -> file, RELATIVE specifiers only. A bare specifier (react, numpy) names a package, not a node, so it is dropped rather than resolved to a lie. The target must exist on disk. calls symbol -> symbol, within ONE file. Cross-file resolution needs a real type resolver; a regex guess would bind `handle` here to `handle` somewhere unrelated. symbols.mjs's doctrine is that a missed symbol is a coarser answer and a wrong one is unacceptable, so the scope stops where the match stops being unambiguous. Verified on a fixture: `./sub/dep.mjs` linked, `react` skipped, alpha->beta linked within the file, and the cross-file call to `helper` correctly not linked. 3. THE STORE DID NOT IGNORE ITSELF. The design says this directory is gitignored by default, because findings are unreviewed agent output and committing them puts unreviewed analysis into git history. Nothing enforced it: on two real checkouts `.token-optimizer/` showed as untracked, so one `git add -A` would have committed 35 MB of it. It now writes a `.gitignore` containing `*` INSIDE the store rather than editing the project's own -- writing to a file the user reviews is not this tool's business. Strictly inside, and that is not a detail. My first attempt wrote the marker to the PARENT, which is correct for the default layout but not when TOKEN_OPTIMIZER_WIKI_DIR points at a directory the user owns -- there the parent is their project root and `*` ignores their entire repository. The skeleton suite caught it immediately: `git add auth.ts` started refusing a file the test had just written. Ignoring only what we create cannot reach outside the store. 98 suites / 1409 tests green; tsc clean. * feat(wiki): agent-written findings, and stop billing on an ambient api key Two changes to who may write findings and what that is allowed to cost. 1. wiki_write -- the agent records a conclusion while it still holds the context. The design named this tool ("Model-invoked `wiki_write` exists for the agent to record something deliberately") and it was never built, so the only route into the graph was a post-hoc extraction that was itself never wired. A real project after a full session: 122 file nodes, 132 symbol nodes, 61 task nodes, and zero findings. The design's objection to agent writes was that "anything opt-in does not happen". True of a suggestion -- but this project answered exactly that problem elsewhere by ENFORCING tool routing rather than advising it, and a write that costs nothing and sends nothing is the cheapest possible path into the graph. The two are complements: this captures what the working session concluded, and the harvest, when enabled, catches what it forgot. Anchors are required and load-bearing: an unanchored finding can never be invalidated, so it would be served as current forever. A finding whose anchors resolve to nothing is refused and the unresolved paths are named back. Provenance gets a third origin rather than being folded into an existing one. `harvested` means a cheap model inferred a claim from a transcript afterwards; `agent` means the session that did the reasoning recorded its own conclusion. Those deserve different trust, and collapsing them destroys the calibration the origin field exists to provide. Weight sits between harvested and human. 2. An ambient credential is not consent. before: harvestEnabled = Boolean(apiKey()) && MODE !== 'off' ANTHROPIC_API_KEY is already set in most development environments, so that rule would have started a billed third-party call -- carrying a digest of the user's prompts, commands and conclusions off the machine -- the moment this shipped, without anyone choosing it. The rule now separates the two things that actually differ, cost and disclosure, instead of treating "a key exists" as permission: local endpoint -> free and private, so on by default, no key required anything else -> deliberate TOKEN_OPTIMIZER_HARVEST opt-in AND a key `harvestMode()` returns why rather than just whether, because "off" needs a reason a user can act on: off:mode, off:not-opted-in, off:no-key, local, remote. The Stop hook reports that reason once per session, names the variable that changes it, and says what still works meanwhile -- previously it failed silently, which is why a graph with no findings gave no clue why. extract() sends no auth header when the endpoint is local, so the free path is actually reachable rather than blocked on a key it does not need. Verified end to end through the built server: wiki_write stores with origin=agent, resolves the project from the anchor's repository rather than the session cwd, accepts path#symbol anchors, refuses a missing claim via the published-schema guard, and the finding comes back on the next read of that file through forTouch. Gating matrix covered by 8 tests, including that a bare ANTHROPIC_API_KEY does not enable billing and that "localhost.attacker.example" is not treated as local. 99 suites / 1417 tests green; tsc clean. * fix(wiki): review fixes -- anchor confinement, key collisions, hook lifecycle Seven findings from the review on ooples#229, all valid on inspection. ANCHOR CONFINEMENT. resolveAnchor called indexFile on any path a finding named, and indexFile READS the file and stores a snapshot of it in the graph -- so an anchor is a read primitive. The findings come from a model reading a transcript, so those paths are untrusted: a claim naming ../../.ssh/id_rsa would have copied it into .token-optimizer and served it back on the next touch. Anchors are now confined to the project root, compared with a trailing separator so /repo-secrets is not read as inside /repo. wiki_write passes the project it resolved. KEY COLLISIONS. `harvested-<ms>-<index>` is not unique across CONCURRENT detached workers: two Stop hooks finishing in the same millisecond produce the same key, and putNode keeps the last write for an id -- so one session's finding silently replaced another's. Four random bytes added. NOTICE PATH. The session id arrives in the hook payload and was interpolated straight into a path, so separators or `..` placed the marker anywhere on disk. Now sanitised to an allowlist, under os.tmpdir() rather than a TEMP/TMPDIR chain that fell back to `.` and scattered markers through whatever repository the session was in. STDOUT TRUNCATION. The off-notice was written and then process.exit(0) ran in the finally, which discards anything still buffered -- so on a filled pipe the one message explaining why no findings exist was the most likely thing to be dropped. The write is awaited (callback, or drain when buffered) and the process now sets exitCode and terminates naturally. DEBOUNCE. Stop fires at the end of every assistant turn, so a talkative session spawned a model call per turn, each re-reading an overlapping transcript. Now rate-limited per session, with the marker touched only when a harvest actually starts so skipped turns do not push the next one further away. IPv6 LOOPBACK. URL.hostname returns IPv6 literals BRACKETED, so the bare '::1' comparison never matched and http://[::1]:11434 was treated as remote -- the loopback spelling a user is most likely to copy from a server that printed it. PROVENANCE, AND A PRE-EXISTING GAP IT EXPOSED. ORIGIN_AGENT was added without an export tag or any ranking effect. Wiring it revealed that HUMAN_WEIGHT has been declared since curate.mjs was written and consumed by nothing: a person's correction ranked exactly level with a machine guess of equal confidence, which is the one thing the origin field exists to prevent. The single ranking function now applies human 1.5 / agent 1.2 / harvested 1. UNRESOLVED ANCHORS were computed only when NOTHING was written, so a finding with three anchors of which one is a typo was stored against the other two, reported as a success, and said nothing about the anchor that will never invalidate it. Now reported unconditionally. Verified against the built server: an out-of-project anchor is refused; a partial write reports written=1 with the unresolved anchor named; http://[::1]:11434 resolves as local while api.anthropic.com stays opt-in. 99 suites / 1417 tests. * style: apply prettier to wiki_write and its registration Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(wiki): write a finding and its anchors as one append Three review findings from ooples#229, all in the path that persists a harvested or agent-written claim. ONE APPEND FOR A FINDING AND ITS ANCHORS. writeHarvested wrote the finding node, then looped putEdge for each resolved anchor. Every append is a separate syscall and the harvest runs in a DETACHED worker that a session end or a sleeping machine can kill between any two of them, so the surviving record could be a finding node with no derived_from edge. That is exactly the record this module refuses to create on purpose -- it drops a finding whose anchors all failed to resolve, because an unanchored claim can never be invalidated by staleness and is therefore served as current forever. putNodeWithEdges writes the group in a single appendFileSync inside a single lock, and orders the NODE LAST. The ordering is the real guarantee: one write can still tear, so what matters is that the surviving prefix is safe. A tear leaves edges whose `from` names no node, and every consumer already dereferences that through a nodes.get(...) guard (findingsFor, audit, sweep), so a dangling edge is inert. The reverse order has no such property. A torn final line is discarded by load(), which skips unparseable records. Tested by truncating the log at every byte offset in turn and asserting that no cut point yields a finding without an anchor. MARKERS MOVE OUT OF SHARED TEMP. stop-harvest kept its once-per-session notice flag and its debounce timestamp in os.tmpdir()/token-optimizer. That is per-user on Windows but a shared, world-writable /tmp on POSIX, and the names derive from the session id, so another local account can predict the path and pre-create it as a symlink -- writeFileSync follows symlinks, so the write lands wherever the link points with this user's privileges. The state is a flag and a timestamp, so the fix is to stop using a shared directory rather than to guard one: XDG_STATE_HOME (LOCALAPPDATA on Windows), created 0o700. Writes additionally go through O_NOFOLLOW with an lstat ownership check, which makes the refusal atomic rather than a check-then-use window. THE DISABLED-HARVEST TEST NOW ASSERTS ON THE TRANSPORT. It checked only that extract() resolves to [], which passes whether extract declined to run or fired a request that failed. Those are opposite outcomes -- one spends nothing and discloses nothing, the other bills a call and ships a digest of the user's prompts off the machine -- and that difference is the entire point of the module. It now stubs fetch and asserts zero calls, paired with a positive test asserting exactly one call once a local endpoint enables it, so zero is meaningful rather than vacuous. 101 suites, 1426 tests, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix(wiki): the semantic harvest was implemented but never invoked
The wiki design specifies findings extracted "at Stop and PreCompact" by a cheap
model, out of band. hooks/lib/harvest.mjs implements that -- digest, model call,
validation -- and was imported by NOTHING. The only match for its own filename
across the repo is its header comment. Stop carried no token-optimizer hook at
all, and precompact-optimize.mjs makes no reference to harvesting.
The failure was silent rather than loud, which is why it survived: the graph kept
filling with its structural layer while the semantic layer never ran.
Measured on a real project after a full session of heavy work:
node kinds file 122, symbol 132, task 61, finding 0
edges derived_from 61, contains 132
metrics injections 0, harvestTokens 0
"insufficient data (0 treated, 0 holdout; need 20 and 5)"
Everything downstream -- inject, staleness, skeleton, restore, curate, and the
treated-vs-holdout metrics -- was wired and idle, waiting on findings that could
not arrive. The whole value proposition of the graph (retrieve verdicts rather
than re-derive them, and record dead ends that exist nowhere in the source tree)
was unreachable.
Three pieces were missing:
A writer. harvest.mjs validates findings but nothing stored them, and
curate.create() could not be reused: it stamps origin ORIGIN_HUMAN and a
human-key because it exists for a person asserting something. Routingmachine output through it would label a model's guess as a human assertion --
the exact confusion curate.mjs warns destroys trust calibration. So
harvest-write.mjs writes with ORIGIN_HARVESTED, copying create()'s anchor
discipline: index the file first, and refuse a finding whose anchors do not
resolve rather than store an un-invalidatable claim.
An invocation. stop-harvest.mjs runs at Stop and spawns a DETACHED worker,
because the extraction is a model call and doing it inline would make the
session that did the work pay for summarising itself -- the cost this
subsystem exists to avoid. Stop is never delayed.
A voice. harvestEnabled() returns false without an API key and the module
skips silently; nothing in doctor, audit or waste mentions harvest, so a user
sees a graph with no findings and cannot learn why. The Stop hook now says so
once per session, names the variable, and states that the structural layer
still works.
Verified end to end: writeHarvested stored 2 findings with origin=harvested and
types failure/decision, auto-created their file and symbol anchor nodes, and
REFUSED a third whose anchor did not exist. The Stop hook fires, reports the
disabled state once, and stays silent on the second call.
Note the remaining precondition: extraction needs TOKEN_OPTIMIZER_API_KEY or
ANTHROPIC_API_KEY. Without one this now reports that fact instead of hiding it.
99 suites / 1423 tests green; tsc clean.
🤖 Generated with Claude Code