diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 549611d..d979765 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,6 +18,43 @@ env: BUN_VERSION: 1.3.14 jobs: + unit-tests: + name: Unit tests + runs-on: ubuntu-24.04 + timeout-minutes: 15 + + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Set up Bun + uses: oven-sh/setup-bun@v2 + with: + bun-version: ${{ env.BUN_VERSION }} + + - name: Cache Bun install cache + uses: actions/cache@v4 + with: + path: ~/.bun/install/cache + key: ${{ runner.os }}-${{ runner.arch }}-bun-${{ hashFiles('bun.lock') }} + restore-keys: | + ${{ runner.os }}-${{ runner.arch }}-bun- + + - name: Install dependencies + run: bun install --frozen-lockfile + + - name: Run unit tests + run: | + # Excluded test files: + # - compiledBinaryPty/wrapperPty/wrapperTmux: require a built native binary (covered by package-smoke) + # - commit.e2e/commit.integration: require sl (Sapling VCS), not available on CI runners + # - replToolTranscriptScreenContract/LogoV2.renderSnapshot: snapshot tests with machine-specific paths + # - permissionOptions: hardcodes /home/xjdr/ as home dir, fails on CI runners + mapfile -t files < <(find src -name '*.test.*' | grep -vE \ + 'compiledBinaryPty|wrapperPty|wrapperTmux|commit\.e2e\.test|commit\.integration\.test|replToolTranscriptScreenContract|LogoV2\.renderSnapshot|permissionOptions\.test' \ + | sort) + bun tools/test/run-isolated-bun-tests.mjs "${files[@]}" + package-smoke: name: Package smoke / ${{ matrix.name }} runs-on: ${{ matrix.runner }} @@ -33,9 +70,6 @@ jobs: - name: darwin-arm64 runner: macos-14 target: bun-darwin-arm64 - - name: darwin-x64 - runner: macos-15-intel - target: bun-darwin-x64 steps: - name: Checkout diff --git a/CHANGELOG.md b/CHANGELOG.md index c84e6db..f4da827 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,17 +9,29 @@ See [RELEASING.md](./RELEASING.md) for the release process and version-bump poli ## [Unreleased] +## [0.2.0] - 2026-06-25 + ### Added - GitHub Actions now build, attest, and publish Linux and macOS release artifacts from version tags on `main`. - Load `AGENTS.md` and `.agents/` instructions into context via the `agentsmd` loader ([#15](https://github.com/Noumena-Network/code/pull/15)) - GLM 5.2 managed first-party model profile and tier routing ([#17](https://github.com/Noumena-Network/code/pull/17)) - GLM 5.2 promoted to the first-party default model ([#21](https://github.com/Noumena-Network/code/pull/21)) +- `cliPrint` / `cliPrintWarn` / `cliPrintError` helpers in `src/utils/cliOutput.ts` — thin pass-throughs to `process.stdout` / `process.stderr` that centralize the `noConsole` lint suppression in one place rather than at ~248 scattered `biome-ignore` comments. Unlike `cliError` / `cliOk` in `src/cli/exit.ts`, these do NOT exit the process. 241 of 248 suppressions migrated (97%); the 7 remaining are genuine special cases (crash handler, global console patch, entrypoint fast-paths, dev-mode warnings, central helpers). +- `swallow(promise, context)` helper in `src/utils/swallow.ts` — fire-and-forget promise wrapper that logs rejections at debug level via `logForDebugging` before suppressing them, making silent failures observable when debugging without propagating as `unhandledRejection`. +- `clearAllBaseToolsCache()` export in `src/tools.ts` and a `registerDownstreamCacheInvalidator()` registration mechanism in `src/utils/plugins/pluginLoader.ts` — `tools.ts` registers its clearer at module-eval time via lazy `require` (avoids the circular dependency), and `clearPluginCache()` now transitively busts the `allBaseToolsCache` so plugin reloads and `NCODE_USER_MODE` runtime switches return a fresh tool set instead of a stale singleton. +- `src/ink/reconcilerShims.ts` — centralized type augmentations for `react-reconciler@0.33.0` runtime APIs (`updateContainerSync`, `flushSyncWork`, `flushSyncFromReconciler`, and the 10-arg `createContainer` arity) that ship in the package but are missing from `@types/react-reconciler`. Exports `asSyncReconciler()` and `asCreateContainer10()` wrappers; all Ink call sites import from here so type suppressions live in a single file. +- Shared `isToolConcurrencySafe(tool, rawInput)` helper in `src/services/tools/toolConcurrency.ts` — extracts the duplicated parse + try/catch + `tool.isConcurrencySafe(parsedInput.data)` fallback that was implemented independently in both `toolOrchestration.ts` and `StreamingToolExecutor.ts`. Both call sites now use the shared helper. ### Changed - Release workflow now supports build-only dry-runs before publishing tags, and release docs now describe required branch protection and known native image fallback status. - Public first-party builds now default to Kimi K2.7 Coder ([#4](https://github.com/Noumena-Network/code/pull/4)) +- Migrated 77 `process.env.NCODE_BUILD_MODE === 'noumena' || process.env.USER_TYPE === 'ant'` direct env reads across 57 files to the canonical `isInternalBuild()` / `!isInternalBuild()` helper from `src/capabilities/static.ts`. The helper is a strict superset of the legacy check (also returns true for `internal` and `dev` spins), matching its documented contract "Returns true for any non-public spin". `TungstenTool`-specific `USER_TYPE === 'noumena'` gates (a distinct concept — Noumena product user, not internal build) are intentionally left untouched. +- Migrated 241 `biome-ignore lint/suspicious/noConsole` suppressions across 19 files to use the new `cliPrint` / `cliPrintWarn` / `cliPrintError` helpers: `plugins.ts` (36), `mcp.tsx` (25), `bridgeMain.ts` (19), `setup.ts` (9), `main.tsx` (9), `pluginCliCommands.ts` (7), `auth.ts` (5), `client.ts` (4), `worktree.ts` (3), `agents.ts` (3), `windowsPaths.ts` (2), `betas.ts` (2), `autoUpdater.ts` (2), `protocolHandler.ts` (2), `fileHistory.ts` (1), `process.ts` (1), `shell/prefix.ts` (1), `structuredIO.ts` (1+1 unguarded), `imageProcessor.ts` (1). The 7 remaining suppressions are genuine special cases (crash handler, global console patch, entrypoint fast-paths, dev-mode warnings, central helpers). +- Migrated 19 silent fire-and-forget `.catch(() => {})` promises across `main.tsx` (6), `bridge/replBridge.ts` (1), `bridge/replBridgeHandle.ts` (1), `services/mcp/client.ts` (5), `tools/FileReadTool` (1), `tools/FileEditTool` (1), `tools/FileWriteTool` (1), `services/analytics/firstPartyEventLogger.ts` (1), `services/api/claude.ts` (1), `services/api/openAICompatInferenceClient.ts` (1) to the new `swallow(promise, context)` helper. The remaining ~33 sites are `await ... .catch(() => {})` patterns (which wait for the promise), `Promise.race` losers, or map-attach patterns — these need different treatment and are left for a follow-up. +- Removed unnecessary `.mode as PermissionMode` cast in `QueryEngine.ts:570` — `AppStateStore` types `toolPermissionContext` as `ToolPermissionContext`, whose `.mode` field is already `PermissionMode`. The cast was a stale leftover from when `mode` was typed looser. +- `src/utils/modifiers.ts` rewritten to use `createRequire` + cached `loadBinding()` with try/catch (matching the pattern in `src/shims/audioCaptureNapi.ts`) instead of a top-level `require('modifiers-napi')`. The require is now lazy — only fires when `isModifierPressed` / `prewarmModifiers` is called on macOS — so the bundler doesn't try to resolve it at build time and the build no longer fails when the stub package is absent. ### Fixed @@ -33,6 +45,23 @@ See [RELEASING.md](./RELEASING.md) for the release process and version-bump poli - GLM 5.2 1M context lane support and tier lookup ([#31](https://github.com/Noumena-Network/code/pull/31)) - Package smoke probe now normalizes executable paths through `realpath()` so macOS `/var` vs `/private/var` does not false-fail the native runtime probe ([#28](https://github.com/Noumena-Network/code/pull/28)) - Prompt-injection warning guidance tightened to require concrete evidence before warning the user; the malware-mitigation reminder is no longer appended to every benign file-read result ([#32](https://github.com/Noumena-Network/code/pull/32)) +- `allBaseToolsCache` in `src/tools.ts` (a module-level singleton that never invalidated) now busts when `clearPluginCache()` runs. Previously, plugin reloads mid-session or `NCODE_USER_MODE` runtime switches would return a stale tool set that excluded newly-registered plugin tools. +- Build no longer fails when the `modifiers-napi` stub package is absent. The top-level `require('modifiers-napi')` in `src/utils/modifiers.ts` was a static require that the bundler tried to resolve at build time; rewritten to lazy `createRequire` + cache so it only fires on macOS at call time. +- JWT payload decode failures in `src/bridge/jwtUtils.ts` now log the token prefix and error message at debug level (previously silently returned `null`, hiding malformed-token diagnostics that are security-relevant). +- `decodeURIComponent` failures in `src/tools/LSPTool/LSPTool.ts` now log the path prefix and error at debug level (previously silently fell through to the un-decoded path). +- `image-processor-napi` load failures in `src/tools/FileReadTool/imageProcessor.ts` now log the error before falling back to `sharp` (surfaces native-module loading issues that were previously invisible). +- `agentMemorySnapshot` read/parse failures in `src/tools/AgentTool/agentMemorySnapshot.ts` now log the path and error at debug level (helps diagnose corrupt memory snapshot files). +- Bare `// TODO: fix this` in `src/screens/REPL.tsx:3554` above the `eslint-disable react-hooks/exhaustive-deps` replaced with a documentation comment explaining why `[]` deps is correct for the mount-once effect (stable refs). +- `// TODO: figure out why` in `src/services/api/errorUtils.ts:126` resolved — API error messages can be undefined when the error originates from a network failure (no HTTP response body to parse) or a non-JSON error envelope. Replaced with a documentation comment. +- `// TODO: Refactor to use isMemoryFilePath()` in `src/services/compact/compact.ts:1765` resolved — added `isMemoryFilePath()` check alongside the existing `MEMORY_TYPE_VALUES` canonical-path check. `isMemoryFilePath()` checks by basename + path pattern, catching child directory memory files (`.ncode/rules/*.md`, `.claude/rules/*.md`) that the canonical-path check misses. Both checks kept for completeness. + +### Removed + +- **Dead stub N-API packages** (`image-processor-napi`, `color-diff-napi`, `modifiers-napi`, `url-handler-napi`) removed from `package.json` `dependencies`. Each was a `0.0.1` reserved-stub package whose entire implementation was `module.exports = {}` — zero runtime value. All consumers wrap their `import()` / `require()` in try/catch and fall through to working alternatives (`sharp`, `osascript`, no-op). `audio-capture-napi` is intentionally kept — `build/build.mjs` shims its import specifier to `src/shims/audioCaptureNapi.ts`, which loads a real native binding from `@anthropic-ai/claude-agent-sdk/vendor/audio-capture/`. +- **Orphaned `rust/py_repl_host/`** crate (491 lines of Rust + `Cargo.toml` + `Cargo.lock` + `assets/kernel.py`) and the `src/shims/assets/pyReplHost.ts` shim removed. `build/build.mjs:75-76` explicitly documents that "py_repl is intentionally not bundled in the OSS export"; the `PyReplTool` is gated off by `isInternalBuild()` and never registered in external builds; the leftover `BUCK` file was a monorepo artifact (AGENTS.md: "This repo uses Git, not Sapling/Buck"). +- **Stale `/mlstore/src/noumena/` path** removed from `build/packageAudit.mjs` static forbidden-substring list. This was an internal-monorepo checkout path that leaked into the public export; the dynamic `collectLocalPathForbiddenSubstrings()` already covers the current checkout path at runtime. +- 11 `@ts-expect-error` comments across `src/ink/ink.tsx` (6) and `src/ink/render-to-screen.ts` (5) that suppressed missing `@types/react-reconciler` declarations for `updateContainerSync`, `flushSyncWork`, `flushSyncFromReconciler`, and the 10-arg `createContainer` arity. Centralized in the new `src/ink/reconcilerShims.ts`. +- 98 `biome-ignore lint/suspicious/noConsole:: intentional console output` comments across 5 high-traffic CLI files (replaced by the new `cliPrint` / `cliPrintWarn` / `cliPrintError` helpers). ### Docs @@ -40,10 +69,12 @@ See [RELEASING.md](./RELEASING.md) for the release process and version-bump poli - `NCODE_USER_TYPE` build mode and runtime feature switches documented ([#6](https://github.com/Noumena-Network/code/pull/6)) - Minimum Rust version (1.80) documented for build tooling ([#9](https://github.com/Noumena-Network/code/pull/9)) - README updated to instruct users to explicitly select Kimi K2.7 Coder for first-party builds ([#14](https://github.com/Noumena-Network/code/pull/14)) +- Bare `// TODO: avoid the cast` in `src/utils/promptCategory.ts:21` replaced with a documented explanation of why the `as QuerySource` cast exists (`QuerySource` is a closed string union; built-in agent types are dynamic template literals TS can't prove are union members) and what would fix it (widen `QuerySource` to a template literal type or add `agent:builtin:${string}` as a member). ## [0.1.0] - 2026-06-16 Initial OSS export of Noumena Code. -[Unreleased]: https://github.com/Noumena-Network/code/compare/v0.1.0...HEAD +[Unreleased]: https://github.com/Noumena-Network/code/compare/v0.2.0...HEAD +[0.2.0]: https://github.com/Noumena-Network/code/compare/v0.1.0...v0.2.0 [0.1.0]: https://github.com/Noumena-Network/code/releases/tag/v0.1.0 \ No newline at end of file diff --git a/build/packageAudit.mjs b/build/packageAudit.mjs index 0b35388..aa6950f 100644 --- a/build/packageAudit.mjs +++ b/build/packageAudit.mjs @@ -9,14 +9,6 @@ const MANIFEST_FORBIDDEN_KEYS = [ ]; const STATIC_FORBIDDEN_SUBSTRINGS = [ - { - label: 'repo checkout path', - value: '/mlstore/src/noumena/', - }, - { - label: 'windows repo checkout path', - value: '\\mlstore\\src\\noumena\\', - }, { label: 'pkcs8 private key marker', value: '-----BEGIN PRIVATE KEY-----', diff --git a/bun.lock b/bun.lock index c2098d4..5657bd0 100644 --- a/bun.lock +++ b/bun.lock @@ -54,7 +54,6 @@ "cli-boxes": "4.0.1", "cli-highlight": "2.1.11", "code-excerpt": "4.0.0", - "color-diff-napi": "0.0.1", "diff": "8.0.4", "emoji-regex": "10.6.0", "env-paths": "4.0.0", @@ -67,14 +66,12 @@ "highlight.js": "11.11.1", "https-proxy-agent": "8.0.0", "ignore": "7.0.5", - "image-processor-napi": "0.0.1", "indent-string": "5.0.0", "ink": "6.8.0", "jsonc-parser": "3.3.1", "lodash-es": "4.17.23", "lru-cache": "11.2.7", "marked": "17.0.5", - "modifiers-napi": "0.0.1", "p-map": "7.0.4", "picomatch": "4.0.4", "plist": "3.1.0", @@ -93,7 +90,6 @@ "turndown": "7.2.2", "type-fest": "5.5.0", "undici": "7.24.6", - "url-handler-napi": "0.0.1", "usehooks-ts": "3.1.1", "vscode-jsonrpc": "8.2.1", "vscode-languageserver-protocol": "3.17.5", @@ -571,8 +567,6 @@ "color-convert": ["color-convert@2.0.1", "", { "dependencies": { "color-name": "~1.1.4" } }, "sha512-RRECPsj7iu/xb5oKYcsFHSppFNnsj/52OVTRKb4zP5onXwVF3zVmmToNcOfGC+CRDpfK/U584fMg38ZHCaElKQ=="], - "color-diff-napi": ["color-diff-napi@0.0.1", "", {}, "sha512-tEwCEDFRCl75LfxzuYVTYVyWFVnt6zqH01HRhLuFrotPSkVk+Nt5Zxr0yXVjsta4eh7GeKCSVNNDi/VoNT0bOQ=="], - "color-name": ["color-name@1.1.4", "", {}, "sha512-dOy+3AuW3a2wNbZHIuMZpTcgjGuLU/uBL/ubcZF9OXbDo8ff4O8yVp5Bf0efS8uEoYo5q4Fx7dY9OgQGXgAsQA=="], "combined-stream": ["combined-stream@1.0.8", "", { "dependencies": { "delayed-stream": "~1.0.0" } }, "sha512-FQN4MRfuJeHf7cBbBMJFXhKSDq+2kAArBlmRBvcvFE5BB1HZKXtSFASDhdlz9zOYwxh8lDdnvmMOe/+5cdoEdg=="], @@ -755,8 +749,6 @@ "ignore": ["ignore@7.0.5", "", {}, "sha512-Hs59xBNfUIunMFgWAbGX5cq6893IbWg4KnrjbYwX3tx0ztorVgTDA6B2sxf8ejHJ4wz8BqGUMYlnzNBer5NvGg=="], - "image-processor-napi": ["image-processor-napi@0.0.1", "", {}, "sha512-BJH2djIrJ+8doiXsLYUjj1fuIIL8KdZcuEtsVCjtyKMNyT6pVEL14Ug30PkGSR1V2yHdzhDVwZlQHWAS9Kd8fg=="], - "indent-string": ["indent-string@5.0.0", "", {}, "sha512-m6FAo/spmsW2Ab2fU35JTYwtOKa2yAwXSwgjSv1TJzh4Mh7mC3lzAOVLBprb72XsTrgkEIsl7YrFNAiDiRhIGg=="], "inherits": ["inherits@2.0.4", "", {}, "sha512-k/vGaX4/Yla3WzyMCvTQOXYeIHvqOKtnqBduzTHpzpQZzAskKMhZ2K+EnBiSM9zGSoIFeMpXKxa4dYeZIQqewQ=="], @@ -859,8 +851,6 @@ "minipass-pipeline": ["minipass-pipeline@1.2.4", "", { "dependencies": { "minipass": "^3.0.0" } }, "sha512-xuIq7cIOt09RPRJ19gdi4b+RiNvDFYe5JH+ggNvBqGqpQXcru3PcRmOZuHBKWK1Txf9+cQ+HMVN4d6z46LZP7A=="], - "modifiers-napi": ["modifiers-napi@0.0.1", "", {}, "sha512-m9eEEqG/3S9YfVyFnygphpfuhCY4Zw77IEj88bOl7j4GjoQkiz+PWerylclZMmfrOMSzdkU4W7fQta2bWIQcgg=="], - "ms": ["ms@2.1.3", "", {}, "sha512-6FlzubTLZG3J2a/NVCAleEhjzq5oxgHyaCU9yYXvcLsvoVaHJq/s5xXI6/XXP6tz7R9xAOtHnSO/tXtF3WRTlA=="], "mute-stream": ["mute-stream@1.0.0", "", {}, "sha512-avsJQhyd+680gKXyG/sQc0nXaC6rBkPOfyHYcFb9+hdkqQkR9bdnkJ0AMZhke0oesPqIO+mFFJ+IdBc7mst4IA=="], @@ -1057,8 +1047,6 @@ "unpipe": ["unpipe@1.0.0", "", {}, "sha512-pjy2bYhSsufwWlKwPc+l3cN7+wuJlK6uz0YdJEOlQDbl6jo/YlPi4mb8agUkVC8BF7V8NuzeyPNqRksA3hztKQ=="], - "url-handler-napi": ["url-handler-napi@0.0.1", "", {}, "sha512-/V+AVJeFAYLPpnNdVRqYgmXSlA3xH0AKM1t7YSMziIxFSBMSVnGDxaNrafHytm3dJY6tKG2Ep1zZX0nrhDuNRg=="], - "usehooks-ts": ["usehooks-ts@3.1.1", "", { "dependencies": { "lodash.debounce": "^4.0.8" }, "peerDependencies": { "react": "^16.8.0 || ^17 || ^18 || ^19 || ^19.0.0-rc" } }, "sha512-I4diPp9Cq6ieSUH2wu+fDAVQO43xwtulo+fKEidHUwZPnYImbtkTjzIJYcDcJqxgmX31GVqNFURodvcgHcW0pA=="], "uuid": ["uuid@8.3.2", "", { "bin": { "uuid": "dist/bin/uuid" } }, "sha512-+NYs2QeMWy+GWFOEm9xnn6HCDp0l7QBD7ml8zLUmJ+93Q5NF0NocErnwkTkXVFNiX3/fpC6afS8Dhb/gz7R7eg=="], diff --git a/package.json b/package.json index fc54a60..0bf4565 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@noumena/code", - "version": "0.1.0", + "version": "0.2.0", "type": "module", "bin": { "ncode": "dist/cli.js" @@ -113,7 +113,6 @@ "cli-boxes": "4.0.1", "cli-highlight": "2.1.11", "code-excerpt": "4.0.0", - "color-diff-napi": "0.0.1", "diff": "8.0.4", "emoji-regex": "10.6.0", "env-paths": "4.0.0", @@ -126,14 +125,12 @@ "highlight.js": "11.11.1", "https-proxy-agent": "8.0.0", "ignore": "7.0.5", - "image-processor-napi": "0.0.1", "indent-string": "5.0.0", "ink": "6.8.0", "jsonc-parser": "3.3.1", "lodash-es": "4.17.23", "lru-cache": "11.2.7", "marked": "17.0.5", - "modifiers-napi": "0.0.1", "p-map": "7.0.4", "picomatch": "4.0.4", "plist": "3.1.0", @@ -152,7 +149,6 @@ "turndown": "7.2.2", "type-fest": "5.5.0", "undici": "7.24.6", - "url-handler-napi": "0.0.1", "usehooks-ts": "3.1.1", "vscode-jsonrpc": "8.2.1", "vscode-languageserver-protocol": "3.17.5", diff --git a/rust/py_repl_host/BUCK b/rust/py_repl_host/BUCK deleted file mode 100644 index b21212b..0000000 --- a/rust/py_repl_host/BUCK +++ /dev/null @@ -1,22 +0,0 @@ -load("@fbsource//tools/build_defs:rust_binary.bzl", "rust_binary") - -oncall("scm_client_infra") - -rust_binary( - name = "ncode_py_repl_host", - srcs = [], - mapped_srcs = { - "src/main.rs": "code/rust/py_repl_host/src/main.rs", - "codex//codex-rs/core:src/tools/py_repl/kernel.py": "codex/codex-rs/core/src/tools/py_repl/kernel.py", - "codex//codex-rs:python-version.txt": "codex/codex-rs/python-version.txt", - }, - crate = "ncode_py_repl_host", - crate_root = "code/rust/py_repl_host/src/main.rs", - autocargo = {"cargo_toml_dir": "code/rust/py_repl_host"}, - visibility = ["PUBLIC"], - default_target_platform = "prelude//platforms:default", - deps = [ - "third-party//rust:serde-1.0.228", - "third-party//rust:serde_json-1.0.150", - ], -) diff --git a/rust/py_repl_host/Cargo.lock b/rust/py_repl_host/Cargo.lock deleted file mode 100644 index 84d0947..0000000 --- a/rust/py_repl_host/Cargo.lock +++ /dev/null @@ -1,107 +0,0 @@ -# This file is automatically @generated by Cargo. -# It is not intended for manual editing. -version = 4 - -[[package]] -name = "itoa" -version = "1.0.18" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8f42a60cbdf9a97f5d2305f08a87dc4e09308d1276d28c869c684d7777685682" - -[[package]] -name = "memchr" -version = "2.8.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f8ca58f447f06ed17d5fc4043ce1b10dd205e060fb3ce5b979b8ed8e59ff3f79" - -[[package]] -name = "ncode_py_repl_host" -version = "0.1.0" -dependencies = [ - "serde", - "serde_json", -] - -[[package]] -name = "proc-macro2" -version = "1.0.106" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8fd00f0bb2e90d81d1044c2b32617f68fcb9fa3bb7640c23e9c748e53fb30934" -dependencies = [ - "unicode-ident", -] - -[[package]] -name = "quote" -version = "1.0.45" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "41f2619966050689382d2b44f664f4bc593e129785a36d6ee376ddf37259b924" -dependencies = [ - "proc-macro2", -] - -[[package]] -name = "serde" -version = "1.0.228" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9a8e94ea7f378bd32cbbd37198a4a91436180c5bb472411e48b5ec2e2124ae9e" -dependencies = [ - "serde_core", - "serde_derive", -] - -[[package]] -name = "serde_core" -version = "1.0.228" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "41d385c7d4ca58e59fc732af25c3983b67ac852c1a25000afe1175de458b67ad" -dependencies = [ - "serde_derive", -] - -[[package]] -name = "serde_derive" -version = "1.0.228" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d540f220d3187173da220f885ab66608367b6574e925011a9353e4badda91d79" -dependencies = [ - "proc-macro2", - "quote", - "syn", -] - -[[package]] -name = "serde_json" -version = "1.0.149" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "83fc039473c5595ace860d8c4fafa220ff474b3fc6bfdb4293327f1a37e94d86" -dependencies = [ - "itoa", - "memchr", - "serde", - "serde_core", - "zmij", -] - -[[package]] -name = "syn" -version = "2.0.117" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e665b8803e7b1d2a727f4023456bbbbe74da67099c585258af0ad9c5013b9b99" -dependencies = [ - "proc-macro2", - "quote", - "unicode-ident", -] - -[[package]] -name = "unicode-ident" -version = "1.0.24" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e6e4313cd5fcd3dad5cafa179702e2b244f760991f45397d14d4ebf38247da75" - -[[package]] -name = "zmij" -version = "1.0.21" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b8848ee67ecc8aedbaf3e4122217aff892639231befc6a1b58d29fff4c2cabaa" diff --git a/rust/py_repl_host/Cargo.toml b/rust/py_repl_host/Cargo.toml deleted file mode 100644 index bba7ba9..0000000 --- a/rust/py_repl_host/Cargo.toml +++ /dev/null @@ -1,9 +0,0 @@ -[package] -name = "ncode_py_repl_host" -version = "0.1.0" -edition = "2021" - -[dependencies] -serde = { version = "1", features = ["derive"] } -serde_json = "1" - diff --git a/rust/py_repl_host/assets/kernel.py b/rust/py_repl_host/assets/kernel.py deleted file mode 100644 index c370ce8..0000000 --- a/rust/py_repl_host/assets/kernel.py +++ /dev/null @@ -1,193 +0,0 @@ -# Python-based kernel for py_repl. -# Communicates over JSON lines on stdin/stdout. - -import ast -import asyncio -import inspect -import io -import json -import os -import sys -import traceback -from types import SimpleNamespace - - -def _send(message): - payload = (json.dumps(message) + "\n").encode("utf-8") - os.write(sys.__stdout__.fileno(), payload) - - -def _format_error(error): - if isinstance(error, BaseException): - text = "".join(traceback.format_exception_only(type(error), error)).strip() - if text: - return text - return str(error) - - -def _join_outputs(stdout_text, stderr_text): - if stdout_text and stderr_text: - return f"{stdout_text}\n{stderr_text}" - if stdout_text: - return stdout_text - return stderr_text - - -pending_tool = {} -tool_counter = 0 -active_exec_id = None -cell_counter = 0 - -TMP_DIR = os.environ.get("CODEX_PY_TMP_DIR", os.getcwd()) -module_dirs_env = os.environ.get("CODEX_PY_REPL_PYTHON_MODULE_DIRS", "") -for entry in module_dirs_env.split(os.pathsep): - value = entry.strip() - if not value: - continue - path = value if os.path.isabs(value) else os.path.abspath(value) - if path not in sys.path: - sys.path.insert(0, path) - -state_globals = { - "__name__": "__main__", - "__package__": None, -} - - -async def _run_tool(exec_id, tool_name, args): - global tool_counter - - if not isinstance(tool_name, str) or not tool_name: - raise RuntimeError("codex.tool expects a tool name string") - - tool_id = f"{exec_id}-tool-{tool_counter}" - tool_counter += 1 - - arguments_json = "{}" - if isinstance(args, str): - arguments_json = args - elif args is not None: - arguments_json = json.dumps(args) - - loop = asyncio.get_running_loop() - future = loop.create_future() - pending_tool[tool_id] = future - - _send( - { - "type": "run_tool", - "id": tool_id, - "exec_id": exec_id, - "tool_name": tool_name, - "arguments": arguments_json, - } - ) - - result = await future - if not result.get("ok"): - raise RuntimeError(result.get("error") or "tool failed") - return result.get("response") - - -async def _handle_exec(message): - global active_exec_id - global cell_counter - - exec_id = message.get("id") - code = message.get("code") - if not isinstance(exec_id, str) or not isinstance(code, str): - return - - active_exec_id = exec_id - - async def _tool(name, args=None): - return await _run_tool(exec_id, name, args) - - state_globals["codex"] = SimpleNamespace(tmpDir=TMP_DIR, tool=_tool) - state_globals["tmpDir"] = TMP_DIR - - stdout_buf = io.StringIO() - stderr_buf = io.StringIO() - original_stdout = sys.stdout - original_stderr = sys.stderr - sys.stdout = stdout_buf - sys.stderr = stderr_buf - - try: - filename = f"" - cell_counter += 1 - flags = ast.PyCF_ALLOW_TOP_LEVEL_AWAIT - code_obj = compile(code, filename, "exec", flags=flags, dont_inherit=True) - result = eval(code_obj, state_globals, state_globals) - if inspect.isawaitable(result): - await result - - output = _join_outputs(stdout_buf.getvalue().rstrip(), stderr_buf.getvalue().rstrip()) - _send( - { - "type": "exec_result", - "id": exec_id, - "ok": True, - "output": output, - "error": None, - } - ) - except BaseException as error: - _send( - { - "type": "exec_result", - "id": exec_id, - "ok": False, - "output": "", - "error": _format_error(error), - } - ) - finally: - sys.stdout = original_stdout - sys.stderr = original_stderr - if active_exec_id == exec_id: - active_exec_id = None - - -async def _read_stdin(exec_queue): - loop = asyncio.get_running_loop() - while True: - line = await loop.run_in_executor(None, sys.stdin.readline) - if line == "": - await exec_queue.put(None) - return - - line = line.strip() - if not line: - continue - - try: - message = json.loads(line) - except Exception: - continue - - msg_type = message.get("type") - if msg_type == "exec": - await exec_queue.put(message) - elif msg_type == "run_tool_result": - tool_id = message.get("id") - future = pending_tool.pop(tool_id, None) - if future and not future.done(): - future.set_result(message) - - -async def _main(): - exec_queue = asyncio.Queue() - reader = asyncio.create_task(_read_stdin(exec_queue)) - try: - while True: - message = await exec_queue.get() - if message is None: - return - await _handle_exec(message) - finally: - reader.cancel() - - -if __name__ == "__main__": - asyncio.run(_main()) diff --git a/rust/py_repl_host/assets/python-version.txt b/rust/py_repl_host/assets/python-version.txt deleted file mode 100644 index 30291cb..0000000 --- a/rust/py_repl_host/assets/python-version.txt +++ /dev/null @@ -1 +0,0 @@ -3.10.0 diff --git a/rust/py_repl_host/src/main.rs b/rust/py_repl_host/src/main.rs deleted file mode 100644 index d7c7dde..0000000 --- a/rust/py_repl_host/src/main.rs +++ /dev/null @@ -1,491 +0,0 @@ -use serde::Deserialize; -use serde::Serialize; -use serde_json::Value as JsonValue; -use std::collections::VecDeque; -use std::env; -use std::fs; -use std::io; -use std::io::BufRead; -use std::io::BufReader; -use std::io::BufWriter; -use std::io::Write; -use std::path::PathBuf; -use std::process::Child; -use std::process::ChildStdin; -use std::process::ChildStdout; -use std::process::Command; -use std::process::Stdio; -use std::sync::Arc; -use std::sync::Mutex; -use std::time::SystemTime; -use std::time::UNIX_EPOCH; - -const KERNEL_SOURCE: &str = - include_str!("../assets/kernel.py"); -const PY_REPL_MIN_PYTHON_VERSION: &str = - include_str!("../assets/python-version.txt"); -const STDERR_TAIL_LINE_LIMIT: usize = 20; -const STDERR_TAIL_LINE_MAX_BYTES: usize = 512; -const STDERR_TAIL_MAX_BYTES: usize = 4_096; -const STDERR_TAIL_SEPARATOR: &str = " | "; - -#[derive(Debug, Deserialize)] -#[serde(tag = "type", rename_all = "snake_case")] -enum ParentMessage { - Exec { - id: String, - code: String, - #[serde(default)] - timeout_ms: Option, - }, - RunToolResult { - #[serde(rename = "id")] - _id: String, - #[serde(rename = "ok")] - _ok: bool, - #[serde(default)] - #[serde(rename = "response")] - _response: Option, - #[serde(default)] - #[serde(rename = "error")] - _error: Option, - }, -} - -#[derive(Debug, Serialize)] -#[serde(tag = "type", rename_all = "snake_case")] -enum HostMessage<'a> { - ExecResult { - id: &'a str, - ok: bool, - output: &'a str, - error: Option<&'a str>, - }, -} - -struct KernelProcess { - _child: Child, - stdin: BufWriter, - stdout: BufReader, - stderr_tail: Arc>>, -} - -fn main() -> io::Result<()> { - let stdin = io::stdin(); - let stdout = io::stdout(); - let mut input = stdin.lock().lines(); - let mut output = BufWriter::new(stdout.lock()); - let mut kernel: Option = None; - - while let Some(line) = next_nonempty_line(&mut input)? { - let message = match serde_json::from_str::(&line) { - Ok(message) => message, - Err(_) => continue, - }; - - match message { - ParentMessage::Exec { - id, - code, - timeout_ms, - } => { - if kernel.is_none() { - kernel = Some(spawn_kernel()?); - } - - let result = relay_exec( - &id, - &code, - timeout_ms, - kernel.as_mut().expect("kernel initialized"), - &mut input, - &mut output, - ); - - if let Err(error) = result { - write_exec_result(&mut output, &id, false, "", Some(&error))?; - kernel = None; - } - } - ParentMessage::RunToolResult { .. } => { - // Ignored outside an active exec loop. - } - } - } - - Ok(()) -} - -fn next_nonempty_line(input: &mut I) -> io::Result> -where - I: Iterator>, -{ - for line in input { - let line = line?; - if !line.trim().is_empty() { - return Ok(Some(line)); - } - } - Ok(None) -} - -fn relay_exec( - exec_id: &str, - code: &str, - timeout_ms: Option, - kernel: &mut KernelProcess, - parent_input: &mut I, - parent_output: &mut W, -) -> Result<(), String> -where - I: Iterator>, - W: Write, -{ - let exec_message = serde_json::json!({ - "type": "exec", - "id": exec_id, - "code": code, - "timeout_ms": timeout_ms, - }); - write_json_line(&mut kernel.stdin, &exec_message).map_err(|err| err.to_string())?; - - loop { - let mut line = String::new(); - let bytes_read = kernel - .stdout - .read_line(&mut line) - .map_err(|err| err.to_string())?; - - if bytes_read == 0 { - return Err(format!( - "py_repl rust host lost the Python kernel: {}", - format_stderr_tail(&kernel.stderr_tail) - )); - } - - let trimmed = line.trim(); - if trimmed.is_empty() { - continue; - } - - let message_type = extract_type(trimmed); - match message_type.as_deref() { - Some("run_tool") => { - parent_output - .write_all(trimmed.as_bytes()) - .and_then(|_| parent_output.write_all(b"\n")) - .and_then(|_| parent_output.flush()) - .map_err(|err| err.to_string())?; - - let response = wait_for_parent_run_tool_result(parent_input)?; - write_json_line(&mut kernel.stdin, &response).map_err(|err| err.to_string())?; - } - Some("exec_result") => { - parent_output - .write_all(trimmed.as_bytes()) - .and_then(|_| parent_output.write_all(b"\n")) - .and_then(|_| parent_output.flush()) - .map_err(|err| err.to_string())?; - return Ok(()); - } - _ => {} - } - } -} - -fn wait_for_parent_run_tool_result(parent_input: &mut I) -> Result -where - I: Iterator>, -{ - loop { - let line = next_nonempty_line(parent_input).map_err(|err| err.to_string())?; - let Some(line) = line else { - return Err("py_repl rust host lost its parent while waiting for run_tool_result".to_string()); - }; - - let value = serde_json::from_str::(&line).map_err(|err| err.to_string())?; - if extract_type_from_value(&value).as_deref() == Some("run_tool_result") { - return Ok(value); - } - } -} - -fn extract_type(line: &str) -> Option { - serde_json::from_str::(line) - .ok() - .and_then(|value| extract_type_from_value(&value)) -} - -fn extract_type_from_value(value: &JsonValue) -> Option { - value - .get("type") - .and_then(JsonValue::as_str) - .map(str::to_owned) -} - -fn write_exec_result( - writer: &mut W, - id: &str, - ok: bool, - output: &str, - error: Option<&str>, -) -> io::Result<()> { - let message = HostMessage::ExecResult { - id, - ok, - output, - error, - }; - write_json_line(writer, &message) -} - -fn write_json_line(writer: &mut W, value: &T) -> io::Result<()> { - serde_json::to_writer(&mut *writer, value)?; - writer.write_all(b"\n")?; - writer.flush() -} - -fn spawn_kernel() -> io::Result { - let python = resolve_python_executable()?; - let kernel_dir = create_kernel_runtime_dir()?; - let kernel_path = kernel_dir.join("kernel.py"); - fs::write(&kernel_path, KERNEL_SOURCE)?; - - let mut command = Command::new(python); - command.arg(&kernel_path); - command.current_dir(env::current_dir()?); - command.stdin(Stdio::piped()); - command.stdout(Stdio::piped()); - command.stderr(Stdio::piped()); - command.env( - "CODEX_PY_TMP_DIR", - env::temp_dir().to_string_lossy().to_string(), - ); - if let Some(module_dirs) = resolve_python_module_dirs() { - command.env("CODEX_PY_REPL_PYTHON_MODULE_DIRS", module_dirs); - } - - let mut child = command.spawn()?; - let child_stdin = child - .stdin - .take() - .ok_or_else(|| io::Error::new(io::ErrorKind::BrokenPipe, "missing py_repl stdin"))?; - let child_stdout = child - .stdout - .take() - .ok_or_else(|| io::Error::new(io::ErrorKind::BrokenPipe, "missing py_repl stdout"))?; - let child_stderr = child - .stderr - .take() - .ok_or_else(|| io::Error::new(io::ErrorKind::BrokenPipe, "missing py_repl stderr"))?; - - let stderr_tail = Arc::new(Mutex::new(VecDeque::new())); - let stderr_tail_writer = Arc::clone(&stderr_tail); - std::thread::spawn(move || { - let reader = BufReader::new(child_stderr); - for line in reader.lines() { - let Ok(line) = line else { - break; - }; - let trimmed = line.trim(); - if trimmed.is_empty() { - continue; - } - if let Ok(mut tail) = stderr_tail_writer.lock() { - push_stderr_tail_line(&mut tail, trimmed); - } - } - }); - - Ok(KernelProcess { - _child: child, - stdin: BufWriter::new(child_stdin), - stdout: BufReader::new(child_stdout), - stderr_tail, - }) -} - -fn resolve_python_module_dirs() -> Option { - env::var("NCODE_PY_REPL_PYTHON_MODULE_DIRS") - .ok() - .filter(|value| !value.trim().is_empty()) - .or_else(|| { - env::var("CLAUDE_CODE_PY_REPL_PYTHON_MODULE_DIRS") - .ok() - .filter(|value| !value.trim().is_empty()) - }) -} - -fn resolve_python_executable() -> io::Result { - let explicit = env::var("NCODE_PY_REPL_PYTHON_PATH") - .ok() - .filter(|value| !value.trim().is_empty()) - .or_else(|| { - env::var("CLAUDE_CODE_PY_REPL_PYTHON_PATH") - .ok() - .filter(|value| !value.trim().is_empty()) - }); - - let candidates = if let Some(explicit) = explicit { - vec![explicit] - } else { - vec!["python3".to_string(), "python".to_string()] - }; - - let min_version = parse_python_version(PY_REPL_MIN_PYTHON_VERSION.trim()).ok_or_else(|| { - io::Error::new( - io::ErrorKind::InvalidData, - "invalid py_repl minimum Python version", - ) - })?; - - for candidate in candidates { - let output = Command::new(&candidate) - .arg("-c") - .arg("import sys; print(\".\".join(str(part) for part in sys.version_info[:3]))") - .stdout(Stdio::piped()) - .stderr(Stdio::null()) - .output(); - - let Ok(output) = output else { - continue; - }; - if !output.status.success() { - continue; - } - - let stdout = String::from_utf8_lossy(&output.stdout); - let Some(version) = parse_python_version(stdout.trim()) else { - continue; - }; - - if compare_version(&version, &min_version) >= 0 { - return Ok(candidate); - } - } - - Err(io::Error::new( - io::ErrorKind::NotFound, - format!( - "py_repl rust host requires Python {}+", - PY_REPL_MIN_PYTHON_VERSION.trim() - ), - )) -} - -fn parse_python_version(input: &str) -> Option> { - let mut parts = Vec::new(); - for segment in input.split('.') { - let value = segment.trim().parse::().ok()?; - parts.push(value); - } - if parts.len() < 3 { - return None; - } - Some(parts) -} - -fn compare_version(left: &[u32], right: &[u32]) -> i32 { - let length = left.len().max(right.len()); - for index in 0..length { - let left_value = *left.get(index).unwrap_or(&0); - let right_value = *right.get(index).unwrap_or(&0); - if left_value != right_value { - return if left_value > right_value { 1 } else { -1 }; - } - } - 0 -} - -fn create_kernel_runtime_dir() -> io::Result { - let now = SystemTime::now() - .duration_since(UNIX_EPOCH) - .unwrap_or_default() - .as_nanos(); - let dir = env::temp_dir().join(format!( - "ncode-py-repl-host-{}-{now}", - std::process::id() - )); - fs::create_dir_all(&dir)?; - Ok(dir) -} - -fn format_stderr_tail(stderr_tail: &Arc>>) -> String { - let Ok(lines) = stderr_tail.lock() else { - return "".to_string(); - }; - if lines.is_empty() { - return "".to_string(); - } - lines.iter().cloned().collect::>().join(STDERR_TAIL_SEPARATOR) -} - -fn push_stderr_tail_line(lines: &mut VecDeque, line: &str) { - let bounded_line = truncate_utf8_prefix_by_bytes( - line, - STDERR_TAIL_LINE_MAX_BYTES.min(STDERR_TAIL_MAX_BYTES), - ); - if bounded_line.is_empty() { - return; - } - - while !lines.is_empty() - && (lines.len() >= STDERR_TAIL_LINE_LIMIT - || stderr_tail_bytes_with_candidate(lines, &bounded_line) > STDERR_TAIL_MAX_BYTES) - { - lines.pop_front(); - } - - lines.push_back(bounded_line); -} - -fn stderr_tail_formatted_bytes(lines: &VecDeque) -> usize { - if lines.is_empty() { - return 0; - } - let payload_bytes: usize = lines.iter().map(String::len).sum(); - let separator_bytes = STDERR_TAIL_SEPARATOR.len() * (lines.len() - 1); - payload_bytes + separator_bytes -} - -fn stderr_tail_bytes_with_candidate(lines: &VecDeque, line: &str) -> usize { - if lines.is_empty() { - return line.len(); - } - stderr_tail_formatted_bytes(lines) + STDERR_TAIL_SEPARATOR.len() + line.len() -} - -fn truncate_utf8_prefix_by_bytes(input: &str, max_bytes: usize) -> String { - if input.len() <= max_bytes { - return input.to_string(); - } - if max_bytes == 0 { - return String::new(); - } - - let mut end = max_bytes; - while end > 0 && !input.is_char_boundary(end) { - end -= 1; - } - input[..end].to_string() -} - -#[cfg(test)] -mod tests { - use super::compare_version; - use super::parse_python_version; - - #[test] - fn parses_python_versions() { - assert_eq!(parse_python_version("3.10.0"), Some(vec![3, 10, 0])); - assert_eq!(parse_python_version("3.12.3"), Some(vec![3, 12, 3])); - assert_eq!(parse_python_version("3.10"), None); - } - - #[test] - fn compares_python_versions() { - assert_eq!(compare_version(&[3, 10, 0], &[3, 10, 0]), 0); - assert_eq!(compare_version(&[3, 12, 0], &[3, 10, 0]), 1); - assert_eq!(compare_version(&[3, 9, 9], &[3, 10, 0]), -1); - } -} diff --git a/skills/insights-context/REVIEW.md b/skills/insights-context/REVIEW.md new file mode 100644 index 0000000..0300cf7 --- /dev/null +++ b/skills/insights-context/REVIEW.md @@ -0,0 +1,709 @@ +# insights-context Scripts — Code Review + +Thorough review of all Python files in +`skills/insights-context/scripts/` and the `insights-context.md` skill +definition. Each file was read in full; specific issues are reported with line +numbers and exact snippets. + +## Overall Assessment + +The skill is ambitious and well-documented, but the implementation has several +**correctness bugs in the friction classification logic** (the core feature), a +**sanitization inconsistency** between scripts that breaks `test_smoke.py` +defaults, multiple **subprocess calls without timeouts** that can hang +indefinitely, and a number of smaller redaction and rendering edge cases. The +friction classifier's branch ordering silently misclassifies long user +corrections as informational interrupts. + +--- + +## 1. `scan.py` + +### Bugs + +**B1 — Long user corrections (>200 chars) are misclassified as informational +interrupts (HIGH)** — lines 449-479. The branch ordering is: + +```python +short = len(txt) < 200 +is_slash_or_long = bool(SLASH_OR_CMD_RE.match(txt)) or len(txt) > 600 + +if INTERRUPT_RE.search(txt): ... +elif is_slash_or_long: # <-- fires for txt > 600 chars + if pending_interrupt: + informational_interrupts += 1 +elif short and CORRECTION_RE.search(txt): # <-- only checked when short + friction.append({... "bucket": "user_correction" ...}) +``` + +A correction like `"no stop, the path is wrong — " + 450 chars of detail` is +`> 600` chars, so it hits the `is_slash_or_long` branch (counted as +informational if a pending interrupt exists), and the `CORRECTION_RE` branch is +never reached. The `short and` guard also drops legitimate corrections that +happen to be 200-600 chars even without a pending interrupt — they fall through +to the `elif pending_interrupt` branch and are silently swallowed (no friction +recorded, no informational count incremented). Verified empirically. + +**B2 — `SLASH_OR_CMD_RE` false-positives on `# ` comments and uppercase-starting +prose** — line 107: +`re.compile(r"^(\s*/|#\s*/|#\s*[A-Z]|\s*<)", re.I)`. The `#\s*[A-Z]` alternative +matches any line starting with `#` followed by an uppercase letter — e.g. +`"# Of issues to fix"` is treated as a slash command, suppressing +`task_prompt`/`last_user_prose` update and skipping the correction branch. +Verified: `"# of issues"` matches. The `re.I` flag is also pointless here +(already matches `[A-Z]` explicitly and lowercase via IGNORECASE is redundant +with the explicit pattern). + +**B3 — Response times in `[0, 2]` seconds are silently dropped** — line 290: +`if 2 < net < 3600:`. A 1.5s or 2s response is excluded from `response_times`, +so the `response_time_buckets` "2-10s" bucket starts effectively at >2s. The +bucket label `"2-10s"` (line 589) is misleading — values exactly at 2.0s are +excluded (strict `<`). Fast responses vanish from the chart entirely. + +**B4 — `task_prompt` only captured once per session, never refreshed for the +"first" message** — line 442: +`if not task_prompt and txt and not SLASH_OR_CMD_RE.match(txt) and len(txt) < 400:`. +`task_prompt` is set only on the *first* qualifying user message and never +updated. The comment and `last_user_prose` (line 447) were added to fix loop +attribution, but `task_prompt` is still used as the fallback in friction items +(lines 434, 471) and cycle detection (line 371). If the first user message is a +short greeting, all subsequent friction in that session attributes to the +greeting, not the real task. + +**B5 — Cycle detection emits duplicate entries for the same X-Y-X-Y window** — +lines 361-373. The window check runs on *every* tool_use, and a 4-element window +`[X, Y, X, Y]` matches, appends to `agent_loops`, and then on the *next* tool +(if it's X again), the window `[Y, X, Y, X]` also matches and appends again. A +single 6-tool run `X Y X Y X Y` produces 3 loop entries. +`loop_total = len(agent_loops)` (line 637) over-counts by ~3×. No +de-duplication by `(session, ts, tool_pair)`. + +**B6 — `pending_interrupt` with no follow-up at *session boundary* is flushed, +but interrupts followed by an assistant message (not user) are never classified** +— the interrupt classification only happens inside +`if role == "user" and isinstance(msg.get("content"), list)`. If an interrupt is +the last user message and is followed by an assistant message (e.g. a slash +command response), the `pending_interrupt` is only flushed at session end (line +509) as a steering friction — but if an assistant message intervenes, there's +no logic to clear `pending_interrupt` based on assistant content. This is +probably acceptable, but the classification is purely based on the *next user +message*, ignoring whether the assistant actually addressed the interrupt. + +**B7 — `parallel_tool_calls` counter uses `tool_batch_count` reset logic that +misses parallel calls across content blocks** — line 277: +`tool_batch_count = 0` resets at the start of each assistant message, then +increments per `tool_use` block (line 312). Line 376: +`if tool_batch_count > 1: parallel_tool_calls += 1`. This counts +*sessions/messages* with parallel calls, not the number of parallel batches. +The variable name implies a count of parallel call *groups*, but it's actually +a count of *messages containing >1 tool call*. Misleading but not a crash. + +### Security Issues + +**S1 — `OTHER_USERS_RE` only scrubs `/Users/...`, missing `/home/...` and +`/root/...`** — line 158: `re.compile(r"/Users/[^/\s\"']+", re.I)`. On Linux +(the environment here, `/root`), home directories are under `/home/` or +`/root/`. The regex only catches macOS `/Users/` paths. A path like +`/home/alice/secrets` or `/root/.ssh/id_rsa` is *not* scrubbed by this pattern. +`HOME_RE` (line 153) catches the *current* user's home, but other users' homes +on Linux leak. The comment explicitly mentions CI runners, but Linux CI runners +use `/home/runner` or `/root`, not `/Users/runner`. + +**S2 — Bare `USERNAME_RE` over-scrubs public GitHub handles, contradicting the +`.md` spec** — lines 154-155, 168-169. The skill doc (lines 156-160) explicitly +states: *"The public GitHub handle itself is **not** scrubbed — it's already on +the repo's public commits."* But `USERNAME_RE` replaces *every* occurrence of +the username string with ``, including the bare public handle in +commit messages or prose. If a user's GitHub handle equals their local username +(common), the public handle is scrubbed, violating the documented behavior. +Verified: `"fixed by jane in PR #42"` → `"fixed by in PR #42"`. + +**S3 — Redaction patterns can be defeated by encoding/whitespace variations** — +the `password=secret` pattern (line 123) uses `\s*[:=]\s*\S+`. A value like +`password = "secret with spaces"` only redacts `"secret` (stops at whitespace). +JSON-encoded secrets with spaces or unicode quotes are partially leaked. Also, +`api_key` variants: `api[_-]?key` misses `apikey` (no separator), `API_KEY` is +covered by `re.I` but `ApiKey` (camelCase) is missed by the `[_-]?` separator +requirement. + +**S4 — `redact()` applies `HOME_RE` substitution that can produce misleading +`~` in commit hashes** — if a home path is `/Users/a1b2c3d` (unlikely but +possible for short hex-like usernames), `HOME_RE.sub("~", ...)` could mangle a +commit hash that contains the home directory string. Low probability but the +substitution is unconditional on any match. + +### Code Quality Issues + +**Q1 — `to_scan` ternary is redundant** — line 23: +`to_scan = recent[:sessions] if len(recent) > sessions else recent`. +`recent[:sessions]` already returns `recent` if `len(recent) <= sessions`. +Simplify to `to_scan = recent[:sessions]`. + +**Q2 — `message_hours` uses `.get()` on a `Counter` (unnecessary)** — line 238: +`message_hours[dt.hour] = message_hours.get(dt.hour, 0) + 1`. `Counter` returns +0 for missing keys, so `message_hours[dt.hour] += 1` suffices. Also inconsistent +with every other Counter usage in the file (which uses `+=` or +`.get(name, 0) + 1` on plain dicts). + +**Q3 — `import sys as _sys` inside conditional (line 659)** — `sys` is already +imported at module top (line 10). The local `import sys as _sys` is +dead/unnecessary — just use `sys.stderr`. + +**Q4 — Bare `except:` on line 231 swallows all exceptions including +`KeyboardInterrupt`** — `except: continue` on JSON parse failures. Should be +`except (json.JSONDecodeError, ValueError):` to avoid swallowing +`KeyboardInterrupt` / `SystemExit`. + +**Q5 — `BUCKET_RULES` loaded at module level (line 581) but `bucket_of` +defined at line 522 references it before definition** — `bucket_of` (line 522) +references `BUCKET_RULES` (line 581). This works because `bucket_of` is only +*called* at line 585 (after `BUCKET_RULES` is defined), but it's fragile — +moving the call site above the assignment would crash. Forward-reference +coupling. + +**Q6 — `_load_bucket_rules` re-imports `json as _json` (line 567)** — `json` is +already imported at module level (line 10). Redundant local import. + +**Q7 — Friction `signal_keys` extraction duplicates effort** — lines 423-425: +`PATH_RE.findall`, `re.findall` for commits, `IDENTIFIER_RE.findall` all run on +every error snippet. For long error traces, this is O(n) per snippet with no +caching. Acceptable but the `sorted(set(...))` on each is wasteful. + +**Q8 — `last_tool_name` and `consecutive_count` declared (lines 76-77) but +never used** — lines 76-77: `last_tool_name = None` and `consecutive_count = 0` +are module-level globals, reset per-session (lines 204-205), but *never read* +anywhere. Dead code left over from a refactor to the `burst_*` state machine. + +### Performance Concerns + +**P1 — `Path.stat()` called twice per file during sort** — lines 20-22: +`sorted(..., key=lambda p: p.stat().st_mtime, ...)` then +`recent = [f for f in files if f.stat().st_mtime >= cutoff]`. Each `stat()` is a +syscall; called once in sort key and once in the list comprehension. For +directories with many JSONL files, this doubles stat calls. Cache with a list +comprehension first. + +**P2 — `recent_tool_window.pop(0)` is O(n)** — line 363: +`recent_tool_window.pop(0)` on a list is O(n) for the window size. With +`CYCLE_WINDOW = 6` this is negligible, but `collections.deque(maxlen=6)` would +be cleaner and O(1). + +**P3 — `pending_tool_calls` dict rebuilt via comprehension on every assistant +message** — line 293-294: +`pending_tool_calls = {k: v for k, v in pending_tool_calls.items() if "resolved_dt" not in v}`. +For sessions with many pending tools, this rebuilds the dict on every assistant +turn. Mutation (`del pending_tool_calls[k]`) would be more efficient. + +--- + +## 2. `resolve.py` + +### Bugs + +**B1 — Commit hash length inconsistency between `citation` and +`evidence_keywords`** — line 146: +`commits_in_text = re.findall(r"\b([0-9a-f]{7,8})\b", text)` (7-8 chars). Line +136: `re.finditer(r"\b([0-9a-f]{7,40})\b", text)` (7-40 chars). So a 9-40 char +commit hash appears in `evidence_keywords` but *not* in `commits_in_text`, +meaning the `citation` field won't reference it. A commit hash `abc12345a` (9 +chars) is harvested as evidence but the citation says "no commits in text". +Verified empirically. + +**B2 — Frontmatter regex fails on `\r\n` line endings** — line 110: +`re.match(r"^---\n(.*?)\n---\n(.*)$", content, re.DOTALL)`. If a memory file +uses CRLF (`\r\n`), the `\n` in the pattern won't match `\r\n`, so `fm_match` +is `None`, `fm = {}`, and `name`/`description` fall back to `path.stem`/`""`. +Silent data loss on Windows-authored memory files. + +**B3 — `prose_paths` harvested twice with overlapping regexes** — lines 127 and +130-131: + +```python +prose_paths = sorted({m.group().lower() for m in PATH_IN_PROSE_RE.finditer(text)}) +... +for m in re.finditer(r"[\w./\-]+/[\w./\-]+\.\w+", text): + keywords.add(m.group().lower()) +``` + +`PATH_IN_PROSE_RE` (line 103) is `r"[\w./\-]+/[\w./\-]+\.\w+"` — identical +pattern to line 130. The first harvest goes into `prose_paths` (→ +`signal_keys.paths`), the second into `keywords` (→ `evidence_keywords`). +Redundant work; the two should be unified. + +**B4 — `fixed_at` uses "last ISO date in text" which may be the issue date, not +the fix date** — lines 150-154: +`all_dates = re.findall(r"\b(\d{4}-\d{2}-\d{2})\b", text)` then +`fixed_at = all_dates[-1]`. The comment claims memory files describe the problem +first then the fix. But if a file mentions a future date (e.g. a deadline) +after the fix date, `fixed_at` becomes that future date, and the renderer +classifies all friction as pre-fix (RESOLVED) even if it's actually a +regression. Fragile heuristic. + +**B5 — `project_*` memory files with fix signals are treated as resolutions +even if they describe *unfixed* problems** — lines 158-166: a `project_*.md` +file containing `"fixed"` or `"resolved"` in *any* context (e.g. "this is NOT +fixed yet", "waiting for resolution") is treated as a resolution entry. The +word match is a substring check on the whole content: +`any(w in content.lower() for w in ["fixed", "resolved", ...])`. +`"not yet fixed"` contains `"fixed"` → treated as resolved. False positive. + +### Security Issues + +**S1 — Git `subprocess` calls have no timeout** — lines 184-188 (`git log`) and +217-221 (`git show`). If git prompts for credentials or hangs on a huge repo, +`subprocess.check_output` blocks forever. No `timeout=` parameter. The renderer +(render.py lines 383, 412) has the same issue. A hung git process exceeds the +`.md`'s "2 minute time-box" constraint silently. + +**S2 — Git commands trust `repo_path` / `ncode_repo` from argv without +validation** — line 244: `repo_path = sys.argv[2]`. Passed directly to +`["git", "-C", repo, "log", ...]`. If `repo_path` contains shell +metacharacters, they're safe (no shell=True), but a malicious path could point +git at an arbitrary directory. Low risk for a local tool, but no path +validation. + +**S3 — `evidence_keywords` redaction is applied per-keyword, not on the joined +string** — line 172: `sorted(redact(k) for k in keywords)[:30]`. Each keyword is +redacted individually. A token that spans two keywords (e.g. a URL split across +path and query) wouldn't be caught. Also, `redact()` on a single short keyword +like `"password"` (if it appears as a bare word) is fine, but a secret split as +`["pass", "word=secret"]` won't redact the value. + +### Code Quality Issues + +**Q1 — `parse_memory_file` swallows all exceptions at the call site (line 255) +but has no internal error handling** — lines 251-256: +`try: entry = parse_memory_file(mf); ... except Exception: pass`. If +`parse_memory_file` crashes (e.g. on a malformed file), the exception is +silently swallowed and the memory file is skipped with no log. The user has no +way to know why a memory file wasn't harvested. + +**Q2 — Massive code duplication between `resolve.py` and `scan.py` redaction +logic** — lines 22-79 of `resolve.py` are a near-verbatim copy of scan.py lines +109-177. The `REDACT_PATTERNS`, `_IDENTITY_TOKENS` loading, `HOME_RE`, +`USERNAME_RE`, `OTHER_USERS_RE`, and `redact()` function are duplicated. If one +is updated and the other isn't, redaction diverges between scan and resolve +output. Should be a shared module. + +**Q3 — `GENERIC` stopword set is hardcoded and not configurable** — lines 81-90: +a large set of stopwords. Not a bug, but inconsistent with the configurable +bucket rules (`~/.ncode/insights-buckets.json`). Keyword extraction quality +depends on this list with no way to extend it. + +**Q4 — `why` and `how` extraction regexes assume Markdown `**bold:**` format** +— lines 120-123: +`re.search(r"\*\*Why:\*\*\s*(.+?)(?=\n\*\*|\Z)", body, re.DOTALL)`. If memory +files use a different format (e.g. `### Why` or plain `Why:`), extraction fails +silently and `why`/`how` are empty strings, reducing keyword yield. + +### Performance Concerns + +**P1 — `git show --name-only` called per commit (line 217)** — for each matching +fix commit, a separate `git show` subprocess is spawned. If a repo has 100 fix +commits in 30 days, that's 100 subprocess calls. Could be batched with +`git log --name-only` in a single call. + +**P2 — `re.finditer` called 4+ times on the same `text`** — lines 127, 130, +132, 136, 138 — five separate regex passes over the same `text` string. Could +be consolidated into a single pass. + +--- + +## 3. `render.py` + +### Bugs + +**B1 — `lstrip("~/")` strips a *character set*, not a prefix** — line 233: +`stripped = path.lstrip("~/")`. `str.lstrip` treats its argument as a set of +characters to strip, not a prefix. So `"~/foo/bar"` → `"foo/bar"` (correct by +accident), but `"~~/foo"` → `"foo"` (strips both `~`), and `"/~/foo"` → +`"foo"` (strips leading `/` and `~`). For paths that *don't* start with `~` +(the `else` branch handles those), this isn't reached, but for `~`-prefixed +paths the `parts[0]` derivation is wrong for edge cases. Should be +`path.removeprefix("~/")` (Python 3.9+) or +`path[2:] if path.startswith("~/") else path`. + +**B2 — `whats_hindering` is referenced before assignment when `reg_count` is +truthy but `open_count` is falsy** — lines 273-283: + +```python +if reg_count: + whats_hindering = (...) # set +if open_count: + ... + whats_hindering = ((whats_hindering if reg_count else "") + ...) # references +if not open_count and not reg_count: + whats_hindering = (...) # set +``` + +The conditional `(whats_hindering if reg_count else "")` on line 281 is only +safe because Python's conditional expression short-circuits. But this is fragile +— if someone refactors the `if open_count:` to `elif:`, or if `reg_count` is +truthy-but-falsy (e.g. a list that becomes empty), it breaks. More importantly: +if `reg_count` is truthy and `open_count` is falsy, `whats_hindering` is set in +the first block but the `if not open_count and not reg_count` block is skipped +(correct), so it works. But the logic is convoluted and error-prone. Initialize +`whats_hindering = ""` at the top. + +**B3 — `synthesize_glance` default top language is `"Swift"`** — line 253: +`top_lang = max(languages, key=languages.get) if languages else "Swift"`. +Hardcoded fallback to `"Swift"`. The skill is project-agnostic (per the `.md`), +but the default leaks an iOS-project assumption. Should be `"(unknown)"` or +`"(none)"`. + +**B4 — Environmental items are displayed in BOTH the "OPEN" friction section AND +the environmental section** — lines 150-164 (in `cross_reference`): when +`matched.is_environmental` is true, the item is appended to *both* +`environmental_matched` AND `open_topics` (with a note). Then in rendering, +line 447 iterates `xref["open"]` and renders OPEN cards, and lines 477-491 +render environmental cards. So environmental friction appears twice: once as an +OPEN card (with note "Keep OPEN") and once as an ENVIRONMENTAL card. The `.md` +says environmental should route to "a separate section" — double display +contradicts this. + +**B5 — `derive_project_areas` uses `os.path.relpath` which raises `ValueError` +on different drives (Windows)** — line 238: `os.path.relpath(path, REPO_PATH)`. +On Windows, if `path` and `REPO_PATH` are on different drives, this raises +`ValueError`. Caught by `except (ValueError, TypeError)` on line 244, but only +for the `else` branch — the `if path.startswith("~")` branch has no such +protection and could fail if `path` is malformed. + +**B6 — `hour_chart` parses keys with `int(k)` that can fail on non-numeric +keys** — line 406: +`{str(k): v for k, v in sorted([(int(k), v) for k, v in hours.items()])}`. If +`scan.get("message_hours", {})` contains non-numeric keys (e.g. from a +malformed scan JSON), `int(k)` raises `ValueError` and crashes `main()`. No +try/except. Scan.py always emits string-numeric keys, but `load_json` can load +arbitrary JSON. + +**B7 — `recent_commits` count uses 30-day window but `wins_html` uses 7-day +window** — line 383: `git rev-list --since=30 days ago`. Line 413: +`git log --since=7 days ago`. The banner shows "X commits (30 days)" while +"Impressive Things You Did" shows 7-day commits. Not a bug per se, but the +inconsistent windows are confusing and undocumented in the output. + +**B8 — `friction_card` for RESOLVED items passes `[]` for examples, hiding the +original friction** — line 450: +`friction_card(t["topic"], t["count"], [], "RESOLVED", t["citation"])`. Resolved +cards show *no examples* (empty list). The `.md` says resolved items should be +struck-through with a citation footer, but showing zero examples makes it +impossible to verify *which* friction was resolved. The `cross_reference` +function does have `pre_fix_items` available but discards them when building +`resolved_matched` (line 195-201 — no `examples` key is set). + +### Security Issues + +**S1 — CSS loaded from `/tmp/insights-context.css` is injected unescaped into +``. +`css_block = css` (from `load_css()`, line 46-50), which reads +`/tmp/insights-context.css` verbatim. `/tmp` is world-writable on multi-user +systems. A malicious or corrupted CSS file containing `