fix: npm install warning, onboarding audit telemetry, and dashboard error noise#560
Conversation
a90f3a6 to
d452f0d
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR removes npm lifecycle install scripts, adds CLI-driven install telemetry, instruments onboarding audits, and filters dashboard error telemetry to application-origin errors. It also updates related tests, documentation, changelog entries, and telemetry payload handling. ChangesCLI-driven install reporting
Audit telemetry
Dashboard error-origin filtering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant InstallCheck
participant Telemetry
CLI->>InstallCheck: run maybeReportInstall(version)
InstallCheck->>Telemetry: emit install/version events
InstallCheck-->>CLI: continue despite reporting failures
sequenceDiagram
participant Onboarding
participant AuditRunner
participant PostHog
Onboarding->>PostHog: emit cli_audit_started
Onboarding->>AuditRunner: run audit
AuditRunner-->>Onboarding: results or error
Onboarding->>PostHog: emit completed or failed event
sequenceDiagram
participant Window
participant Listeners
participant ErrorOrigin
participant Telemetry
Window->>Listeners: dispatch browser error
Listeners->>ErrorOrigin: classify origin
ErrorOrigin-->>Listeners: app or non-app result
Listeners->>Telemetry: report app-origin errors
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
|
Automated code review started - full review. Results will be posted here. |
|
⏳ Phase 1-3 complete: cloned, fetched, diff analyzed, tests pass.
|
Automated Code ReviewExecutive SummaryThis PR removes npm lifecycle scripts ( Change Architecturegraph TD
A["package.json (removed scripts)"] -->|"drops"| B["scripts/postinstall.mjs (deleted 242 lines)"]
A -->|"drops"| C["scripts/preuninstall.mjs (deleted 131 lines)"]
D["bin/failproofai.mjs (runCli entrypoint)"] -->|"calls"| E["lib/install-check.ts (NEW: 215 lines)"]
E -->|"uses"| F["scripts/install-telemetry.mjs (modified: version param)"]
E -->|"checks"| G["~/.failproofai/last-version"]
E -->|"reports"| H["PostHog (first_install/version_changed/package_installed)"]
style E fill:#90EE90
style F fill:#87CEEB
style B fill:#FFB3B3
style C fill:#FFB3B3
style D fill:#87CEEB
Legend: Green = New, Blue = Modified, Red = Removed Breaking ChangesNo breaking changes detected. The Behavioural changes (documented in PR description):
Issues FoundNo critical or warning-level issues found.
Logical / Bug AnalysisFailure containment (excellent):
Semver comparison (
Steady-state performance:
No race conditions, no resource leaks, no security issues. Evidence — Build and Test ResultsBuild: Tests: ALL 2090 passed across 121 files in 72.58s.
Issue LinkageNo issue linked. Consider creating one for better traceability — the PR description is thorough enough to serve as documentation, but an issue reference helps future archaeologists. Human Review Feedback
Suggestions
VerdictVERDICT: APPROVED Automated code review · 2026-07-17 09:00 UTC |
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. Code quality is excellent — thorough failure containment, comprehensive tests (18 new tests for install-check, 2 new for telemetry), clean build. No breaking changes. See summary comment for full analysis.
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. ✅
d452f0d to
522f490
Compare
chhhee10
changed the title
|
Automated code review started - full review. Results will be posted here. |
|
⏳ Hermes review in progress — Phase 1-3 complete
|
Automated Code Review - Hermes AgentExecutive SummaryThis PR delivers three tightly-scoped telemetry/packaging fixes: (1) removes npm lifecycle scripts to eliminate Change Architecturegraph TD
A["package.json<br/>-postinstall<br/>-preuninstall"] -->|"removed"| B["lib/install-check.ts<br/>(new: CLI-driven telemetry)"]
B -->|"called from top of runCli()"| C["bin/failproofai.mjs<br/>(after --hook fast path)"]
B -->|"version tracking"| D["scripts/install-telemetry.mjs<br/>(+opts.version, +opts.timeoutMs)"]
E["src/audit/cli.ts<br/>runPostSetupAudit()"] -->|"3 new telemetry events"| F["PostHog<br/>(source=onboarding)"]
G["lib/error-origin.ts<br/>(new: attribution)"] -->|"isAppError()"| H["global-error-listeners.tsx<br/>(filters extensions)"]
style B fill:#90EE90
style G fill:#90EE90
style A fill:#FFD700
style E fill:#87CEEB
Breaking ChangesNo breaking changes detected. All public APIs, config keys, and serialization formats are unchanged. Logical / Bug AnalysisCommit 1 —
Commit 2 —
Commit 3 —
Evidence — Build & Test ResultsBuild: Lint: Tests: Issue LinkageNo linked issues. All three fixes appear to be directly observed production issues rather than issue-tracked bugs. Consider creating issues for traceability. Human Review FeedbackNo human review comments on this PR. 1 previous bot comment (re: checkHooks inspecting only Claude settings) — resolved as the behavior is preserved and documented. Suggestions
VerdictVERDICT: APPROVED Hermes Agent code review · 2026-07-17 11:03 UTC |
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. All 2117 tests pass, 0 type errors, 0 new lint issues. Three well-scoped fixes: npm lifecycle scripts removed, onboarding audit telemetry restored, dashboard error noise filtered. Clean code with thorough test coverage.
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. ✅
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: APPROVED
Reviewed all 19 files (+1031 / -929 lines). All CI checks pass. 2117 tests pass locally. No merge conflicts, no leaked secrets, no debug statements in live code.
Commit 1: Stop shipping npm lifecycle scripts
What it does: Deletes scripts/postinstall.mjs (242 lines) and scripts/preuninstall.mjs (131 lines) + their tests (527 lines). Moves install telemetry (first_install, version_changed, package_installed) to lib/install-check.ts, called from runCli() deliberately outside the --hook fast path. trackInstallEvent gains an explicit opts.version parameter since npm only sets npm_package_version inside lifecycle scripts.
Correctness: Dead-code removal is correct — preuninstall was verified to never execute (npm honours no uninstall lifecycle scripts). The semver comparison is a faithful port from the old script. Same-version guard (previousVersion === version) correctly prevents re-reporting on every CLI invocation. Version file is written before awaiting telemetry so delivery failures don't cause re-retries.
Security: hostname() is hashed with createHmac(sha256) before transmission — same pattern as old code. Pre-existing PostHog API key is a phc_ public client-side key (not a secret). No new credential exposure.
Caveat (pre-existing, now visible): Removing the dead preuninstall — which never ran — makes visible that uninstalling failproofai strands __failproofai_hook__ entries in settings files. Needs failproofai policies --uninstall. Not a blocker, but worth a follow-up issue.
Commit 2: Report the onboarding auto-audit
What it does: runPostSetupAudit() now emits cli_audit_started (fire-and-forget), cli_audit_completed (awaited, before empty-history early return), and cli_audit_failed (awaited). All tagged source: "onboarding". Properties extracted into a shared auditCompletedProps() helper so the cli and onboarding paths cannot drift.
Correctness: cli_audit_started is fire-and-forget (the multi-second scan keeps the process alive). cli_audit_failed is awaited (returns straight into dashboard boot). cli_audit_completed fires before the empty-history early return — matching runAuditCli behavior. Shared helper prevents property drift.
Testing: 6 new tests cover: happy path, source distinction, audit failure, zero-history case, opt-out, and process-never-exits guarantee.
Commit 3: Report only our own dashboard errors
What it does: New lib/error-origin.ts with classifyErrorOrigin() and isAppError(). GlobalErrorListeners component now attributes errors before reporting: only errors traceable to the app origin pass through; extensions (matched by <vendor>-extension:// suffix) and unattributable errors are silently dropped.
Correctness: Extension check runs first (before app-origin check) — correct, since an error passing through an extension frame is the extension's problem even if an app frame appears lower in the stack. Regex matches the shared -extension:// suffix rather than a vendor denylist — future browsers covered. Filename fallback handles ErrorEvents without stacks. Unmount cleanup removes listeners.
Testing: 17 new tests (6 component + 11 unit) covering all major browsers, mixed stacks, our-origin, cross-origin Script error., empty stacks, filename-only, unmount.
Tradeoff (acknowledged): Non-Error rejections Promise.reject("boom") are no longer reported (no stack for attribution). Rare; deliberate.
Overall
| Category | Assessment |
|---|---|
| Correctness | Each fix addresses the stated problem precisely |
| Security | No new secrets; hostname hashed; public phc_ key unchanged |
| Code Quality | Clean separation (install-check vs hook path, error-origin as pure function, shared auditCompletedProps) |
| Testing | 2117 tests pass (123 files); new code has comprehensive coverage with real-world edge cases |
| Performance | Steady-state maybeReportInstall returns after one existsSync(); reporting timeout capped at 2s |
| Documentation | Detailed PR body, inline JSDoc, CHANGELOG entries |
Reviewed by Hermes Agent
npm 12 (released 2026-07-08, now the `latest` dist-tag) blocks dependency install scripts by default, so `npm install failproofai` printed: npm warn install-scripts 1 package had install scripts blocked ... npm warn install-scripts failproofai@... (postinstall: node scripts/postinstall.mjs) and skipped the script. npm 11.16+ prints the advisory `allow-scripts` variant and still runs it. `allowScripts` is purely consumer-side — a package declaring `allowScripts` or `trustedDependencies` for itself is ignored — so the only fix is to ship no install scripts. postinstall's install telemetry (first_install / version_changed / package_installed, same names and properties) now fires from the CLI's first non-hook invocation via lib/install-check.ts: at most once per version, and a no-op once the version is recorded. It is deliberately outside the --hook fast path in bin/failproofai.mjs, which runs on every tool call. trackInstallEvent now takes an explicit version. npm only sets npm_package_version inside a lifecycle script, so outside one it reported "unknown". The script's server.js check and shadowed-PATH diagnosis were redundant: scripts/launch.ts already does both at launch, with a better message. preuninstall is deleted as dead code — npm honours no uninstall lifecycle scripts (verified with a probe package: postinstall fired, preuninstall did not), so its hook cleanup has never run. Uninstalling therefore leaves __failproofai_hook__ entries behind; that gap now needs an explicit `failproofai policies --uninstall`. Behaviour changes: - package_installed measures install->activation, not raw installs. - Same-version reinstalls (direction: "reinstall") are no longer reported; the CLI cannot distinguish one from an ordinary invocation. - The install-time welcome message is dropped; the config wizard's first-run redirect already handles onboarding. This also recovers telemetry already dropped for bun, pnpm and Yarn >=4.14 users, which have blocked install scripts for some time. Verified against a local registry on npm 12: registry install is warning-free (exit 0), the CLI runs, first_install/package_installed fire on first run, nothing fires on the second, and version_changed fires on upgrade — all carrying the real version and the existing $lib/product stamps. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
runPostSetupAudit() — the audit that runs automatically at the end of first-run setup, and therefore the first audit any new user ever runs — emitted no telemetry at all, while an explicit `failproofai audit` reported cli_audit_started / cli_audit_completed / cli_audit_failed. First-run audits were invisible, so the audit funnel silently undercounted exactly the activation moment it exists to measure. Confirmed live: a fresh install's auto-audit wrote its dashboard cache and PostHog saw nothing. It now emits the same three events tagged `source: "onboarding"`, against the existing `source: "cli"`, so the two paths stay distinguishable. - cli_audit_completed fires before the empty-history return, matching runAuditCli, so a fresh user with no agent history is still counted. - cli_audit_failed is awaited: this function returns straight into the dashboard boot, which would race a fire-and-forget send. - The completed-event properties now come from one shared helper so the two entry points cannot drift apart. Onboarding stays best-effort — never throws, never exits. Verified by driving the real (unmocked) runPostSetupAudit against a local capture server: cli_audit_started and cli_audit_completed both land with source=onboarding and real counts (7,468 tool calls / 210 sessions), and the function returns without throwing or exiting. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GlobalErrorListeners registers page-global error/unhandledrejection handlers. Browser extensions inject content scripts into the same page and share the same window, so their failures reached our listeners and went to PostHog stamped $lib: failproofai-web, as though the dashboard had thrown them. Observed live: MetaMask's "Failed to connect to MetaMask" (error_name "i", its minified class) arriving as a failproofai unhandled_rejection on /policies. Extensions are user-installed and open-ended, so the noise was unbounded and depended on which extensions a user happens to run rather than on our code. Left alone it would drown the real signal these listeners exist to catch, and make the dashboard's error rate a function of users' browser setups. Both handlers now attribute an error before reporting it (new lib/error-origin.ts) and report only what traces back to our own origin: - Positive attribution, not a denylist of known extensions, so an unrecognised extension is filtered by default. - The match is on the shared `<vendor>-extension://` suffix, so a new browser's scheme is covered without a code change. - A stack passing through an extension is treated as the extension's, even when one of our frames appears further down. - Unattributable errors (cross-origin "Script error.", rejections of non-Error values) are dropped — there is nothing in them to debug. React render errors are unaffected: the error boundaries report client_error directly, and React only invokes those for errors thrown inside our own tree. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Automated code review started - full review. Results will be posted here. |
The install events are the first thing a fresh process sends, so they always
pay the DNS + TLS handshake. Measured against production PostHog they deliver
in ~1.1s from cold, with a 1.85s worst case observed.
A 2s budget left almost no headroom over that, for a saving nobody
experiences: the timeout is a ceiling, not a cost — delivery resolves as soon
as PostHog answers. A tight budget only ever converts a slow network into a
silently dropped report, which is exactly what this module exists to prevent.
5s matches hook-telemetry and the postinstall script this replaces, and
install-telemetry's own default.
Verified by driving trackInstallEvent against real PostHog from a cold process
at both budgets, plus a transparent proxy confirming production returns
HTTP 200 {"status":"Ok"} for every event (14/14).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
70cac3a to
5daad39
Compare
|
🔍 Hermes re-review in progress — Phase 0-2 complete, Phase 3 starting
|
|
✅ Resolved 1 previously-reported issue that has been addressed in this update (re: cli_audit_started fire-and-forget rationale now well-documented). |
🔍 Hermes Agent Code Review — PR #560 (re-review)📋 Executive SummaryThis PR delivers three tightly-scoped telemetry/packaging fixes (npm lifecycle scripts, onboarding audit telemetry, dashboard error filtering) plus a 4th post-review commit raising install-reporting timeout from 2s→5s. Code quality remains excellent: thorough JSDoc, comprehensive test coverage, clean failure containment. 1 new commit since last review: 📊 Change Architecturegraph TD
A["package.json<br/>−postinstall<br/>−preuninstall"] -->|"removed"| B["lib/install-check.ts<br/>(CLI-driven telemetry)"]
B -->|"v2: 5s timeout"| B2["5daad39: REPORT_TIMEOUT_MS 2000→5000"]
B -->|"called from runCli()"| C["bin/failproofai.mjs<br/>(after --hook fast path)"]
B -->|"version tracking"| D["scripts/install-telemetry.mjs<br/>(+opts.version, +opts.timeoutMs)"]
E["src/audit/cli.ts<br/>runPostSetupAudit()"] -->|"3 telemetry events"| F["PostHog<br/>(source=onboarding)"]
G["lib/error-origin.ts<br/>(new: attribution)"] -->|"isAppError()"| H["global-error-listeners.tsx<br/>(filters extensions)"]
style B2 fill:#FFD700,stroke:#333,stroke-width:2px
style B fill:#90EE90
style G fill:#90EE90
style E fill:#87CEEB
Legend: 🟢 New | 🔵 Modified | 🟡 Changed since last review 🔴 Breaking Changes✅ No breaking changes detected. All public APIs, config keys, and serialization formats unchanged.
|
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated re-review: Approved. ✅
- 1 new commit since last review (: 2s→5s timeout, with rationale)
- All 2200 tests pass across 125 files
- Clean lint on all changed files
- Previous review threads addressed and resolved
- No regressions detected
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. ✅
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
__tests__/components/global-error-listeners.test.tsx (1)
1-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign test file path with application directory structure.
Consider moving this test file to
__tests__/app/components/global-error-listeners.test.tsxto maintain structural consistency. Based on learnings, unit tests should be placed by feature area under__tests__/to mirror the corresponding source structure.🤖 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__/components/global-error-listeners.test.tsx` around lines 1 - 8, Move the global error listeners test from the top-level components test directory into the app-mirrored directory __tests__/app/components/global-error-listeners.test.tsx, preserving its contents and existing test behavior.Source: Learnings
app/components/global-error-listeners.tsx (1)
37-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify the
handleUnhandledRejectionlogic with an early return.Since
isAppErrorwill predictably returnfalseif thestackisundefined(becausefilenameis also absent), a rejection with a non-Errorreason is guaranteed to be ignored anyway.You can add an explicit early return to handle this upfront. This leverages TypeScript's type narrowing, eliminating the need to duplicate the
instanceofchecks and ternaries when constructing the telemetry payload.♻️ Proposed refactor
- // A rejection carries no filename, so the stack is the only evidence of - // where it came from. A non-Error reason has none and stays unattributed. - if ( - !isAppError({ - stack: reason instanceof Error ? reason.stack : undefined, - appOrigin: window.location.origin, - }) - ) { + // A rejection carries no filename, so the stack is the only evidence of + // where it came from. A non-Error reason has none and stays unattributed. + if (!(reason instanceof Error)) { + return; + } + + if ( + !isAppError({ + stack: reason.stack, + appOrigin: window.location.origin, + }) + ) { return; } captureClientEvent("unhandled_rejection", { - error_message: reason instanceof Error ? reason.message : String(reason), - error_name: reason instanceof Error ? reason.name : undefined, + error_message: reason.message, + error_name: reason.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 `@app/components/global-error-listeners.tsx` around lines 37 - 50, Update handleUnhandledRejection to return immediately when reason is not an Error, then pass the narrowed Error’s stack to isAppError and its message and name directly to captureClientEvent, preserving the existing unattributed-rejection filtering.
🤖 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 `@CHANGELOG.md`:
- Line 14: Update the npm 12 release date in the changelog entry to July 15,
2026, matching the registry information, or remove the exact date while
preserving the surrounding explanation about npm 12 and allowScripts.
In `@lib/install-check.ts`:
- Around line 138-143: Validate that matcher.hooks is an array before invoking
.some() in the hook inspection logic, while preserving the existing marker
detection and return behavior for valid arrays. Ensure malformed entries such as
hooks: {} are skipped without throwing so the surrounding install-check flow can
continue normally.
- Around line 58-92: Update the prerelease parsing in compareSemver to split
identifiers only on dots, preserving hyphens as part of a single identifier.
Ensure complete non-numeric identifiers such as beta-2 and beta-10 are compared
lexically, and add a regression case covering 1.0.0-beta-2 versus 1.0.0-beta-10.
In `@scripts/install-telemetry.mjs`:
- Line 117: Add a unit test covering the configurable timeout in the install
telemetry flow, passing timeoutMs: 1234 and asserting that AbortSignal.timeout
is called with 1234. Keep the existing test coverage for the version option
unchanged.
---
Nitpick comments:
In `@__tests__/components/global-error-listeners.test.tsx`:
- Around line 1-8: Move the global error listeners test from the top-level
components test directory into the app-mirrored directory
__tests__/app/components/global-error-listeners.test.tsx, preserving its
contents and existing test behavior.
In `@app/components/global-error-listeners.tsx`:
- Around line 37-50: Update handleUnhandledRejection to return immediately when
reason is not an Error, then pass the narrowed Error’s stack to isAppError and
its message and name directly to captureClientEvent, preserving the existing
unattributed-rejection filtering.
🪄 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
Run ID: 3cd80ed1-928a-4689-8e10-3705869979fb
📒 Files selected for processing (19)
CHANGELOG.md__tests__/audit/audit-cli-telemetry.test.ts__tests__/components/global-error-listeners.test.tsx__tests__/lib/error-origin.test.ts__tests__/lib/install-check.test.ts__tests__/scripts/install-telemetry.test.ts__tests__/scripts/postinstall.test.ts__tests__/scripts/preuninstall.test.tsapp/components/global-error-listeners.tsxbin/failproofai.mjsdocs/package-aliases.mdxlib/error-origin.tslib/install-check.tspackage.jsonscripts/install-diagnosis.mjsscripts/install-telemetry.mjsscripts/postinstall.mjsscripts/preuninstall.mjssrc/audit/cli.ts
💤 Files with no reviewable changes (5)
- tests/scripts/preuninstall.test.ts
- scripts/preuninstall.mjs
- scripts/postinstall.mjs
- tests/scripts/postinstall.test.ts
- package.json
|
Automated code review started - full review. Results will be posted here. |
Semver §9 separates prerelease identifiers with dots; a hyphen is a legal
character *inside* one. So `beta-2` is a single non-numeric identifier and
compares lexically against `beta-10` — making it GREATER ("2" > "1").
Splitting on /[.-]/ shredded it into ["beta", "2"] and compared numerically,
producing the opposite order:
1.0.0-beta-2 vs 1.0.0-beta-10 → spec: greater · was: less
Inherited verbatim from postinstall.mjs, so not a regression, and harmless for
our own dot-separated versions — but wrong, and this module is the place it
lives now.
Caught by CodeRabbit on #560; verified against the spec and pinned with a
regression test that fails on the old split.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
⏳ Hermes review in progress — Phase 3: Running test suite... |
|
🔍 Hermes re-review in progress — 1 new commit since last review ( |
|
✅ Resolved 1 previously-reported issue: compareSemver now splits prerelease identifiers on dots only (commit ced8e13), with a regression test for |
🔍 Hermes Agent Code Review — PR #560 (re-review)Executive SummaryThis is the 4th review of this PR. 1 new commit since the last full review: New Commit:
|
| Area | Verdict |
|---|---|
| npm lifecycle removal | ✅ Clean — postinstall.mjs (242 lines) and preuninstall.mjs (131 lines) deleted, telemetry moved to lib/install-check.ts called from runCli() |
| Onboarding audit telemetry | ✅ Clean — runPostSetupAudit() now emits cli_audit_started/completed/failed with source: "onboarding" |
| Dashboard error filtering | ✅ Clean — GlobalErrorListeners filters via isAppError() from lib/error-origin.ts |
| Semver fix (new) | ✅ Clean — correct per semver §9, regression test pinned |
Build & Test Evidence
Lint: eslint clean on all 6 changed source files (0 errors, 0 warnings)
Tests (changed files):
✓ __tests__/lib/install-check.test.ts (20 tests) 163ms
✓ __tests__/audit/audit-cli-telemetry.test.ts (9 tests) 164ms
✓ __tests__/scripts/install-telemetry.test.ts (4 tests) 71ms
✓ __tests__/lib/error-origin.test.ts (15 tests) 29ms
✓ __tests__/components/global-error-listeners.test.tsx (6 tests) 45ms
Full suite: 2201 tests, 124/125 files passing
- 1 pre-existing failure: integrations.test.ts (path resolution issue, unrelated to PR)
Unresolved from Previous Review
- 💡
lib/install-check.ts:145—if (!matcher?.hooks) continue;guards againstnull/undefinedbut not{hooks: {}}(a truthy object).Array.isArray()would add defense-in-depth against a malformed settings file. In practice failproofai always writes arrays, so this is non-blocking. The CodeRabbit bot flagged the same issue.
Issue Linkage
No linked issues. All fixes are directly-observed production issues. Suggest creating issues for traceability.
Verdict
VERDICT: APPROVED
The ced8e13 commit is a minimal, correct fix for a semver edge case — caught by tooling, verified against spec, and regression-tested. The rest of the PR remains approved as before. The one unresolved Array.isArray() suggestion is defense-in-depth only and does not block merge.
Hermes Agent code review · 2026-07-17 13:15 UTC
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
🔍 Hermes Agent Code Review — PR #560 (re-review)
Executive Summary
This is the 4th review of this PR. 1 new commit since the last full review: ced8e13 fixes a semver prerelease comparison bug where /[.-]/ incorrectly split hyphenated identifiers like beta-2 into two parts. Caught by CodeRabbit, verified against semver §9, and pinned with a regression test. The rest of the PR (npm lifecycle removal, onboarding audit telemetry, dashboard error filtering) is unchanged from the prior reviewed state. Code quality remains excellent. 1 unresolved suggestion from the previous review (defense-in-depth Array.isArray() on L145 — non-blocking).
New Commit: ced8e13
fix(install-check): split prerelease identifiers on dots only
- Before:
pa.pre.split(/[.-]/)— treated hyphens as separators, incorrectly splittingbeta-2into["beta", "2"]instead of treating it as one identifier per semver §9. - After:
pa.pre.split(".")— correctly preserves hyphens as part of a single identifier. - Impact:
1.0.0-beta-2vs1.0.0-beta-10now compares correctly (lexical: "2" > "1" → greater, was: numeric 2 < 10 → less). - Scope: Harmless for failproofai's own dot-separated versions (
0.0.14-beta.0), but correct per spec. Inherited verbatim from oldpostinstall.mjs, now fixed where it lives. - Test: Regression test added —
compareSemver("1.0.0-beta-2", "1.0.0-beta-10")expects> 0and fails on the old split. - JSDoc: Updated to document the semver §9 rule.
✅ Correct fix, clean commit, well-tested.
Overall State
| Area | Verdict |
|---|---|
| npm lifecycle removal | ✅ Clean — postinstall.mjs (242 lines) and preuninstall.mjs (131 lines) deleted, telemetry moved to lib/install-check.ts called from runCli() |
| Onboarding audit telemetry | ✅ Clean — runPostSetupAudit() now emits cli_audit_started/completed/failed with source: "onboarding" |
| Dashboard error filtering | ✅ Clean — GlobalErrorListeners filters via isAppError() from lib/error-origin.ts |
| Semver fix (new) | ✅ Clean — correct per semver §9, regression test pinned |
Build & Test Evidence
Lint: eslint clean on all 6 changed source files (0 errors, 0 warnings)
Tests (changed files):
✓ __tests__/lib/install-check.test.ts (20 tests) 163ms
✓ __tests__/audit/audit-cli-telemetry.test.ts (9 tests) 164ms
✓ __tests__/scripts/install-telemetry.test.ts (4 tests) 71ms
✓ __tests__/lib/error-origin.test.ts (15 tests) 29ms
✓ __tests__/components/global-error-listeners.test.tsx (6 tests) 45ms
Full suite: 2201 tests, 124/125 files passing
- 1 pre-existing failure: integrations.test.ts (path resolution issue, unrelated to PR)
Unresolved from Previous Review
- 💡
lib/install-check.ts:145—if (!matcher?.hooks) continue;guards againstnull/undefinedbut not{hooks: {}}(a truthy object).Array.isArray()would add defense-in-depth against a malformed settings file. In practice failproofai always writes arrays, so this is non-blocking. The CodeRabbit bot flagged the same issue.
Issue Linkage
No linked issues. All fixes are directly-observed production issues. Suggest creating issues for traceability.
Verdict
VERDICT: APPROVED
The ced8e13 commit is a minimal, correct fix for a semver edge case — caught by tooling, verified against spec, and regression-tested. The rest of the PR remains approved as before. The one unresolved Array.isArray() suggestion is defense-in-depth only and does not block merge.
Hermes Agent code review · 2026-07-17 13:15 UTC
Hermes Agent Code Review — PR #560Executive SummaryThis PR delivers three tightly-scoped telemetry/packaging fixes: (1) removes npm lifecycle scripts to eliminate Change Architecturegraph TD
A[package.json] -->|REMOVED: postinstall + preuninstall scripts| B[lib/install-check.ts NEW]
B -->|called from runCli| C[bin/failproofai.mjs]
B -->|trackInstallEvent with explicit version| D[scripts/install-telemetry.mjs]
E[src/audit/cli.ts] -->|NEW: telemetry for onboarding audit| F[PostHog]
G[app/components/global-error-listeners.tsx] -->|NEW: filter via| H[lib/error-origin.ts NEW]
H -->|positive attribution| I[captureClientEvent]
style B fill:#90EE90
style H fill:#90EE90
style A fill:#FFD700
style D fill:#87CEEB
style E fill:#87CEEB
style G fill:#87CEEB
Legend: Green = New | Blue = Modified | Gold = Breaking Change Risk Breaking ChangesNo breaking changes detected. The removed Issues Found
Logical / Bug Analysislib/install-check.ts — maybeReportInstall:
lib/error-origin.ts — classifyErrorOrigin:
src/audit/cli.ts — runPostSetupAudit:
app/components/global-error-listeners.tsx:
Build and Test ResultsTest ResultsType CheckLintIssue LinkageThis PR addresses installation warning behavior tracked internally. No standalone issue linked. Review Thread Status (from previous reviews)
Suggestions
VerdictVERDICT: APPROVED Hermes Agent code review · 2026-07-17 13:16 UTC |
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. All tests pass (2201/2201), clean tsc, no new lint errors. One non-blocking suggestion posted inline. [PASS]
|
✅ Resolved 1 previously-reported issue — |
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. ✅
|
Automated code review started - full review. Results will be posted here. |
|
🔍 Hermes re-review in progress — Phase 0-2 complete, Phase 3 starting
|
|
🔍 Hermes re-review in progress — Phase 0-2 complete, Phase 3 starting. HEAD: ced8e13. 19 files changed (1072+, 929-). Running full test suite... |
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. ✅
3 participants
Three related telemetry/packaging fixes, all surfaced while chasing the
npm installwarning. One PR for review, but three separate commits — reviewable one at a time.fix: stop shipping npm lifecycle scriptsnpm warnon every installfix(audit): report the onboarding auto-auditfix(dashboard): report only our own errorsGate on the combined branch: 2117 tests / 123 files, 0 lint errors, clean
tsc.1. Stop shipping npm lifecycle scripts
npm install failproofai@0.0.14-beta.0printed annpm warnnaming a.mjsfile — our ownscripts/postinstall.mjs. Not specific to beta.0: every published version declares it, including0.0.13(currentlatest).Two messages, both naming our
.mjs, verified live in Docker:allow-scripts … not yet coverednpm i -g npm@latestinstall-scripts … blockednpm 12 (2026-07-08) is now the
latestdist-tag. There is no publisher-side fix —allowScriptsis consumer-side, and a package declaringallowScripts/trustedDependenciesfor itself is ignored (verified with a probe package). Shipping no install scripts is the only fix.Functionality was never affected (
bin→ prebuiltdist/cli.mjs), but the install telemetry was silently lost. bun/pnpm/Yarn ≥4.14 already block by default, so it was already undercounting.Three of postinstall's five jobs needed no migration —
scripts/launch.tsalready does theserver.jscheck and shadow diagnosis (with a better message), andconfigure-wizard.tsalready owns first-run onboarding. So only telemetry moved, intolib/install-check.ts, called from the top ofrunCli()— deliberately outside the--hookfast path, which runs on every tool call.trackInstallEventnow takes an explicit version: npm only setsnpm_package_versioninside a lifecycle script, so outside one it reported"unknown".preuninstalldeleted as dead code (131 lines + 319 of tests). npm honours no uninstall lifecycle scripts — verified with a probe package:postinstallfired,preuninstallnever did. Its hook cleanup has therefore never run, so uninstalling strands__failproofai_hook__entries. Pre-existing bug, now visible — hit for real on a live machine, where~/.pi/settings.jsonwas left pointing at a deletedpi-extensionpath. Needs an explicitfailproofai policies --uninstall; worth a follow-up.Behaviour changes:
package_installednow measures install→activation, not raw installs (the PostHog series will step — though volume should rise, recovering bun/pnpm/Yarn users). Same-version reinstalls (direction: "reinstall") are no longer reported. The install-time welcome message is dropped.Verified by publishing the packed tarball to a local verdaccio registry and installing on npm 12: zero warnings, exit 0, CLI runs. Captured PostHog payloads:
first_install+package_installedon run 1, nothing on run 2,version_changedon upgrade — all with the real version and existing$lib/product. Also provedprepare: bun run buildis not gated on registry installs (beta.0 declares both, npm 12 lists onlypostinstall), so it stays —publish.ymlrelies on it to build.2. Report the onboarding auto-audit
runPostSetupAudit()— the audit that runs automatically at the end of first-run setup, i.e. the first audit any user ever runs — emitted nothing, while explicitfailproofai auditreported normally (4trackHookEventcalls vs 0). The funnel silently undercounted exactly the activation moment it exists to measure.Confirmed live: fresh install → wizard → auto-audit wrote its cache at 15:29:41 and PostHog saw nothing.
Now emits the same three events tagged
source: "onboarding"vs the existingsource: "cli".cli_audit_completedfires before the empty-history return (matchingrunAuditCli) so a user with no history is still counted;cli_audit_failedis awaited because the function returns straight into the dashboard boot, which would race a fire-and-forget send. Completed-event properties now come from one shared helper so the paths can't drift.Verified by driving the real, unmocked function against a local capture server:
cli_audit_started+cli_audit_completed,source=onboarding, real counts (7,468 tool calls / 210 sessions), no throw, no exit.3. Report only our own dashboard errors
GlobalErrorListeners(mounted inapp/layout.tsx) registers page-global handlers. Extensions inject content scripts into the same page and share the samewindow, so their failures went to PostHog stamped$lib: failproofai-webas though the dashboard threw them:The noise is unbounded and unrelated to our code — it makes our error rate a function of users' browser setups and would drown the real signal.
Both handlers now attribute before reporting (
lib/error-origin.ts) and report only what traces to our own origin. Positive attribution, not a denylist, so an unknown extension is filtered by default; matched on the shared<vendor>-extension://suffix so a new browser's scheme needs no code change. Unattributable errors (cross-origin"Script error.", non-Error rejections) are dropped — nothing in them to debug.React render errors are unaffected: the error boundaries report
client_errordirectly, and React only invokes those for errors in our own tree — which is why the global net can be filtered strictly without losing signal.Tradeoff: a genuine error rejecting a non-Error value (
Promise.reject("boom")) has no stack, so it's no longer reported. Rare; flagging it as a deliberate choice.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Chores