feat: installation TUI + onboarding, PostHog telemetry flush, audit failure-reason & Copilot 1.0.71 fix#516
Conversation
|
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 (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughAdds the interactive ChangesConfiguration wizard and TUI
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as failproofai
participant Wizard as runConfigureWizard
participant Hooks as installHooks
participant Audit as runPostSetupAudit
User->>CLI: run config or bare command
CLI->>Wizard: start interactive setup
Wizard->>Hooks: install selected policies and CLIs
Hooks-->>Wizard: return applied result
Wizard->>Audit: run optional post-setup audit
Audit-->>CLI: finish onboarding
Possibly related PRs
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 timed out. The project may have too many dependencies for the sandbox. 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 |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… onboarding, auto-audit
Adds a guided 4-step setup wizard (`failproofai config`, aliases configure/setup):
Where (global/project) → Assistants (12 CLIs, windowed multi-select with
"Everything available") → Policies (combinable presets + "Everything") →
Review & apply. Selections replace the enabled set at the chosen scope via a
new {replace, quiet} options object on installHooks.
The wizard wears the befailproof.ai identity: the real logomark rendered as
half-block art, pink-forward palette (teal step spine), shared via tui.ts
(paint/glyphs/truncate single-sourced) with the restyled policies
--install/--uninstall menus, policy picker, and dashboard launch splash.
First-run onboarding: bare `failproofai` now flows config wizard → audit
("failproofai audit now running · ctrl+c to stop", pre-warms the dashboard
cache; FAILPROOFAI_NO_AUTO_AUDIT=1 opts out) → dashboard. Explicit `config`
applies and exits without auditing. Replaces and removes first-run-nudge.
Also: responsive README 12-logo grid (6-column table), responsive dashboard
header (900/620px breakpoints), HOME isolation in the e2e CLI runner, and a
16-finding cleanup pass (shared prompt engine, no console.log monkey-patch,
fire-and-forget telemetry off the first paint, dead code trimmed).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two PostHog telemetry fixes, on top of the installation-TUI work: 1. Flush hook telemetry before process exit. The hook binary is short-lived — bin/failproofai.mjs calls process.exit() the moment handleHookEvent returns — so events fired with un-awaited `void trackHookEvent(...)` (custom_hooks_loaded, convention_policies_loaded, *_error) were killed mid-flight and never reached PostHog; they only survived when a deny/instruct decision happened to add a trailing await. hook-telemetry.ts now tracks every in-flight POST and exposes flushHookTelemetry(), which handleHookEvent awaits before returning (and bin's hook error path drains on throw). Delivery is now reliable regardless of decision. No coverage change — allow decisions still send nothing by design. 2. Include the failure reason in audit failure telemetry. cli_audit_failed and audit_run_failed carried only error_type (the error class name); they now also send error_message — the actual failure text, home-dir-stripped to ~ and length-capped via a shared sanitizeErrorMessage() helper (lib/telemetry-sanitize.ts) so no local paths leak. Verified via a local capture sink from the packaged tarball: the allow path now delivers its events, all 12 CLIs emit hook_policy_triggered on a deny, and the audit module loads with the new sanitizer. Full unit suite 2052 green; typecheck + Next build clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
chhhee10
force-pushed
the
telemetry-audit-pkg
branch
from
f39de7f to
20b3702
Compare
|
Automated code review started - full review. Results will be posted here. |
|
⏳ Phase 2 — Deep Analysis underway. I've gathered all existing comments (no review threads yet, 1 prior bot comment + 1 CodeRabbit skip notice), the full 31-file diff, and all test changes. Now running build & test verification. |
|
✅ Build & test complete. Results:
Docker smoke test was skipped (failproofai policies blocked execution — ironic). Now posting detailed review... |
Automated Code ReviewExecutive SummaryThis PR delivers three well-scoped improvements: an interactive Change Architecturegraph TD
A[bin/failproofai.mjs] -->|"new: config subcommand"| B[configure-wizard.ts]
A -->|"mod: flush on error"| C[hook-telemetry.ts]
C -->|"new: flushHookTelemetry()"| D[handler.ts]
B -->|"uses"| E[tui.ts]
B -->|"uses"| F[policy-presets.ts]
B -->|"replace/quiet"| G[manager.ts]
H[telemetry-sanitize.ts] -->|"used by"| I[audit/cli.ts]
H -->|"used by"| J[app/api/audit/run/route.ts]
E -->|"shares palette"| K[install-prompt.ts]
B -->|"on apply"| L[runPostSetupAudit()]
style B fill:#90EE90
style C fill:#87CEEB
style E fill:#90EE90
style F fill:#90EE90
style H fill:#90EE90
style G fill:#87CEEB
style K fill:#FFD700
Breaking ChangesNo breaking changes detected. installHooks has a new optional options parameter (default {}). first-run-nudge.ts removed but replaced with better behavior. trackHookEvent now awaits internally but was already async. promptPolicySelection keybind changed (Enter = submit, Space = toggle) to match standard UX. Issues Found
Logical / Bug AnalysisTelemetry flush design is correct: trackHookEvent adds to pending set, flushHookTelemetry loops on while(pending.size > 0), handler calls flush before returning, hook error path also flushes. Replace vs additive in installHooks: correctly toggles at manager.ts:207. All existing callers get replace=false (default). First-run onboarding: hasSeenLauncher checks marker file, anyHooksInstalledGlobally checks real state, marker written only on apply (cancel keeps redirecting), post-setup audit only on first-run onboarding. Home isolation in e2e: cli-runner.ts creates temp HOME per test module. NO_COLOR and non-TTY: colorsEnabled checks isTTY && !NO_COLOR, truecolorEnabled checks COLORTERM. Evidence: Build & Test Results
Suggestions
Human Review Feedback
VerdictVERDICT: APPROVED All gates green. Code is well-structured, well-tested, solves real problems. No breaking changes, no security issues, no logical errors. Suggestions are minor polish. Automated code review - 2026-07-16 12:15:00 UTC |
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. All gates green (tsc, lint, 2054 unit, 307 e2e, build). No breaking changes, no security issues, no logical errors. A few minor suggestions in the summary comment.
|
Tip For best results, initiate chat on the files or code changes.
None of these block merge — they're all polish. Let me know if you'd like me to open follow-up issues for any of them, or if you want a plan to implement the fixes directly in this PR. (ノ◕‿◕)ノ |
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. ✅
…quest normalization
Re-verified the Copilot CLI hook contract live against 1.0.71 (recorder-hook
captures; previous verification was 1.0.41). Two real gaps:
- File tools deliver Copilot's own input keys (Read {path}, Write {path,
file_text}, Edit {path, old_str, new_str}), so path/content builtins like
block-env-files never fired — a live dotenv-file read was observed passing.
New COPILOT_TOOL_INPUT_MAP (keyed by canonical tool name) maps them to
file_path/content/old_string/new_string.
- The permissionRequest event alone pipes a camelCase payload (toolName in
lowercase, toolInput, sessionId); handler.ts now normalizes it so
PermissionRequest-matched policies fire instead of seeing a null tool name.
Not broken (verified live, contrary to initial diagnosis): Bash policies,
PostToolUse sanitizers, Stop gates, and the Claude-shaped deny response all
work on 1.0.71 — the bypass was purely the input-key gap. CLAUDE.md's
Copilot section now carries the 1.0.71 contract, incl. the headless
project-scope .github/hooks discovery caveat.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Update — Copilot 1.0.71 enforcement fix + telemetry reliability verified end-to-endPushed New in this push: Copilot CLI 1.0.71 file-policy bypass (
|
chhhee10
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
bin/failproofai.mjs (1)
759-761: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueProvide optional diagnostic logging on failure.
As identified in the PR objectives, it is recommended to optionally log the error if the onboarding/audit import fails. This helps developers debug failures without breaking the fall-through behavior for users.
💡 Proposed refactor to add optional logging
- } catch { + } catch (err) { + if (process.env.FAILPROOFAI_LOG_LEVEL === "debug") { + console.error("First-run onboarding/audit failed:", err); + } // First-run onboarding is non-critical; fall through to the dashboard. }🤖 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 `@bin/failproofai.mjs` around lines 759 - 761, Update the onboarding/audit import catch block to capture the thrown error and optionally emit diagnostic logging, while preserving the existing non-critical fall-through to the dashboard. Use the surrounding CLI or debug-logging mechanism if available, and keep logging disabled by default for normal users.lib/telemetry-sanitize.ts (1)
35-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse path-boundary-aware home directory sanitization.
As identified in the PR objectives, using a simple substring replacement can inadvertently mangle paths that share a common prefix (e.g., replacing
/home/userinside/home/user2/appresulting in~2/app). Consider using a boundary-aware regular expression to ensure only exact directory matches are sanitized.♻️ Proposed refactor for boundary-aware replacement
- if (home && home.length > 1) { - msg = msg.split(home).join("~"); - } + if (home && home.length > 1) { + const escapedHome = home.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const boundaryRegex = new RegExp(escapedHome + '(?=$|[\\\\/\\s\'":;.,])', 'g'); + msg = msg.replace(boundaryRegex, "~"); + }🤖 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 `@lib/telemetry-sanitize.ts` around lines 35 - 37, Update the home-directory replacement in the telemetry sanitization logic to be path-boundary-aware, so only the exact home path followed by a separator or path end is replaced. Preserve the existing "~" substitution while preventing shared-prefix paths such as a similarly named sibling directory from being altered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/e2e/helpers/cli-runner.ts`:
- Around line 10-22: Add an afterAll teardown in the CLI test helper to
recursively remove the directory created by mkdtempSync for ISOLATED_HOME.
Import and use the existing test framework cleanup hook, ensuring teardown runs
after the module’s tests complete while preserving the current isolated HOME
setup.
In `@__tests__/hooks/configure-wizard.test.ts`:
- Around line 243-254: Extend the first-run configuration tests around
maybeFirstRunConfigure to verify that a completed apply invokes
runPostSetupAudit(), while a canceled flow does not invoke it. Reuse the
existing apply and cancellation setup patterns, and preserve the existing
launcher and hook assertions; do not add environment-variable handling because
FAILPROOFAI_NO_AUTO_AUDIT is already covered by src/audit/cli.ts.
In `@__tests__/hooks/policy-presets.test.ts`:
- Around line 33-35: Update the expected Git policy names in the test “git
preset is exactly the Git category” to filter out beta policies, matching
resolvePreset("git") behavior. Keep the category filter and set comparison
intact so future beta Git policies do not affect the assertion.
In `@src/hooks/configure-wizard.ts`:
- Around line 320-343: Update the configure wizard’s installHooks call to enable
CLI replacement cleanup alongside { replace: true }, so hooks are removed from
previously configured integrations omitted from clis while preserving
installation for the selected assistants.
- Around line 105-114: Update the replacement wizard flow after scope selection
to load the scoped configuration and preselect matching POLICY_PRESETS choices,
including the Everything option when applicable. Detect policy sets that cannot
be represented by those presets and route them through the documented Custom
flow so the exact existing set is preserved before allowing replace-mode
installation. Keep buildPresetChoices and resolveEverything aligned with the
available selectable policies.
- Around line 51-55: Update homeify to replace the home prefix only when the
path is exactly the home directory or has a path separator immediately after it;
preserve paths such as /home/alice-work/repo unchanged while retaining tilde
replacement for the home directory and its descendants.
In `@src/hooks/handler.ts`:
- Around line 377-383: Move the handler processing and result return into a try
block, and invoke flushHookTelemetry() from a finally block so telemetry is
flushed on both success and exceptions, including custom-hook loading and policy
evaluation failures. Preserve the existing return of result.exitCode and add a
unit test covering an exception path that asserts flushHookTelemetry() is
called.
In `@src/hooks/tui.ts`:
- Around line 473-480: Update selectOne to return null immediately when
opts.choices is empty, before opening or configuring the TTY selector, while
preserving the existing first-choice fallback for non-empty non-TTY selections.
Add a regression test in __tests__/hooks/tui.test.ts covering empty choices on a
TTY.
---
Nitpick comments:
In `@bin/failproofai.mjs`:
- Around line 759-761: Update the onboarding/audit import catch block to capture
the thrown error and optionally emit diagnostic logging, while preserving the
existing non-critical fall-through to the dashboard. Use the surrounding CLI or
debug-logging mechanism if available, and keep logging disabled by default for
normal users.
In `@lib/telemetry-sanitize.ts`:
- Around line 35-37: Update the home-directory replacement in the telemetry
sanitization logic to be path-boundary-aware, so only the exact home path
followed by a separator or path end is replaced. Preserve the existing "~"
substitution while preventing shared-prefix paths such as a similarly named
sibling directory from being altered.
🪄 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: 960f7482-6a77-4fd7-91f4-66a79f9fc4e8
📒 Files selected for processing (34)
CHANGELOG.mdCLAUDE.mdREADME.md__tests__/api/audit-run-route.test.ts__tests__/audit/audit-cli-telemetry.test.ts__tests__/e2e/helpers/cli-runner.ts__tests__/hooks/configure-wizard.test.ts__tests__/hooks/copilot-canonicalize.test.ts__tests__/hooks/first-run-nudge.test.ts__tests__/hooks/handler.test.ts__tests__/hooks/hook-telemetry.test.ts__tests__/hooks/policy-presets.test.ts__tests__/hooks/tui.test.ts__tests__/lib/telemetry-sanitize.test.ts__tests__/scripts/postinstall.test.tsapp/api/audit/run/route.tsapp/globals.cssbin/failproofai.mjscomponents/navbar.tsxcomponents/reach-developers.tsxlib/telemetry-sanitize.tsscripts/launch.tsscripts/postinstall.mjssrc/audit/cli.tssrc/hooks/configure-wizard.tssrc/hooks/first-run-nudge.tssrc/hooks/handler.tssrc/hooks/hook-telemetry.tssrc/hooks/install-prompt.tssrc/hooks/manager.tssrc/hooks/policy-presets.tssrc/hooks/tool-name-canonicalize.tssrc/hooks/tui.tssrc/hooks/types.ts
💤 Files with no reviewable changes (2)
- tests/hooks/first-run-nudge.test.ts
- src/hooks/first-run-nudge.ts
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅ (0 issues, 2 suggestions)
PR: #516 — feat: installation TUI + onboarding, PostHog telemetry flush, audit failure-reason & Copilot 1.0.71 fix
Author: @chhhee10
Files changed: 31 (+2,197 -717)
🔴 Critical
None found.
⚠️ Warnings
None found.
💡 Suggestions
-
src/hooks/hook-telemetry.ts:76-82— TheflushHookTelemetry()busy-loop (while (pending.size > 0) { await Promise.allSettled([...pending]); }) works correctly in practice because nosendEventspawns another, but a small guard (max 5 iterations, or a singleawait Promise.allSettled([...pending])pass) would be more defensive and eliminate even the theoretical unbounded-loop risk. Not a real bug — just a future-hardening nit. -
bin/failproofai.mjs:109-112— The error-pathflushHookTelemetry()in the catch block is wrapped in its own try/catch with an empty catch body. If the flush itself fails (e.g., the module fails to import), the error is silently swallowed. Adding aconsole.errordebug log (behind--debugor similar) would help future debugging without affecting the happy-path UX. Practically harmless since the outer catch already printed the dispatch error.
✅ Looks Good
-
Telemetry flush fix is well-designed — The
pendingSet +flushHookTelemetry()pattern cleanly solves the short-lived-binary problem without changing any call-site contract.trackHookEventstill returns a Promise (backward-compatible), thependingSet ensuresvoidcallers are also covered. The handler'sawait flushHookTelemetry()before return + the error path's separate drain covers both normal and exceptional exits. -
Telemetry sanitization is privacy-aware —
sanitizeErrorMessage()collapses home paths to~, length-caps at 300 chars, and never throws. All the right properties for telemetry safety. -
Copilot 1.0.71 fix is surgical and verified — The
COPILOT_TOOL_INPUT_MAP+permissionRequestcamelCase normalization inhandler.tsare minimal, well-scoped, and backed by 9 tests using verbatim captures from a live 1.0.71 recorder hook. The.env-read bypass being caught is exactly the kind of regression this kind of contract drift causes. -
Test coverage is thorough — 262 lines for
configure-wizard.test.ts(full wizard orchestration, all cancel paths, first-run logic, HOME isolation), 68 lines forcopilot-canonicalize.test.ts, 61 forpolicy-presets.test.ts, 95 fortui.test.ts, 46 fortelemetry-sanitize.test.ts, plus flush cases inhook-telemetry.test.tsand handler assertions. New tests use isolated HOME dirs so they never touch real config. -
No security concerns — Static scan found zero hardcoded secrets, zero dangerous calls (eval/exec/os.system), zero SQL injection vectors in the diff. The telemetry message sanitizer explicitly prevents raw paths from reaching PostHog.
-
Clean architecture — The wizard is pure builders + mocked I/O, first-run redirect is a clean state machine (marker file gate, global-hooks shortcut, TTY guard), the
installHooksreplace+quietoptions are additive to the existing API. Removedfirst-run-nudge.ts(146 lines) in favor of the richerconfigure-wizard.ts(352 lines) + shared TUI module. -
docs.jsonis the largest diff (3,184 reordered lines) — this is just a JSON index reshuffle, no behavioral change. -
PR description is excellent — Full gate results, suggested test plan, CHANGELOG entries, and a pinned comment with end-to-end telemetry + Copilot verification writeup. Sets a high bar for PR documentation.
Reviewed by Hermes Agent
|
Automated code review started - full review. Results will be posted here. |
… wizard/tui edge cases - handler.ts: wrap handleHookEvent body in try/finally so flushHookTelemetry() runs even when custom-hook loading or policy eval throws before the happy path (defense in depth alongside bin's catch-path flush). + finally-path test. - configure-wizard.ts homeify(): require a path boundary so a home of /home/alice no longer collapses /home/alice-work to ~-work. - tui.ts selectOne(): return null for empty choices before the TTY branch too, matching the non-TTY behavior (previously dereferenced choices[0] on a TTY). - policy-presets.test.ts: exclude beta policies from the expected Git preset to match resolvePreset()'s !p.beta filter (future beta Git policy would falsely fail). - configure-wizard.test.ts: assert runPostSetupAudit() runs on a completed apply and not on cancel. - cli-runner.ts (e2e): afterAll teardown removes the mkdtempSync ISOLATED_HOME. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
⏳ [Re-review progress] Phase 0 complete: analyzed 8 review threads (6 resolved, 2 unresolved), read 7 prior comments (1 human: @chhhee10 update). Phase 1-2 done: examined all 5 commits, the new Copilot input-map code, handler try/finally restructuring, and CodeRabbit-address commit. Now running build & test verification... |
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Code Review Summary (Re-review)
Verdict: Approved ✅ (0 critical, 0 warnings, 2 suggestions)
PR: #516 — feat: installation TUI + onboarding, PostHog telemetry flush, audit failure-reason & Copilot 1.0.71 fix
Author: @chhhee10
Re-review of: commit 09798e1 — "fix: address CodeRabbit review on #516 — telemetry flush robustness + wizard/tui edge cases"
Delta since first review: 7 files, +302/-264
🔴 Critical
None found.
⚠️ Warnings
None found.
💡 Suggestions
-
src/hooks/hook-telemetry.ts:76-82(pre-existing) — TheflushHookTelemetry()busy-loop could benefit from a max-iteration guard. Not a real bug, just future-hardening. The newtry/finallywrap in handler.ts makes this even less likely to matter in practice. -
src/hooks/configure-wizard.ts:54— Thehomeify()path-boundary fix usingpath.sepis correct on Linux/macOS but won't catch the Windows case wherehomedir()returnsC:\Users\aliceand a path likeC:\Users\alice-workwould also fail the boundary check — thoughpath.sepbeing\on Windows makes this correct. Just worth a cross-platform sanity check.
✅ Looks Good — Changes in this commit
-
handler.ts—try/finallyflush robustness 🔥 The critical fix. Previously flush was anawaiton the happy path only — a throw during stdin read, payload parse, custom hook load, or policy eval would skip it entirely and the caller'sprocess.exit()would drop in-flight telemetry. Now the ENTIRE body is wrapped intry { ... } finally { await flushHookTelemetry(); }, so even an early throw drains pending POSTs before exit. Confirmed by the new test"flushes pending telemetry even when policy evaluation throws (finally path)". -
handler.test.ts— New test for the finally-path flush: mocksevaluatePoliciesto reject, assertsflushHookTelemetrywas called despite the throw. Good coverage of the exact failure mode. -
configure-wizard.ts—homeify()path-boundary fix — Switched fromp.startsWith(home)top.startsWith(home + sep), preventing false positives like/home/alice-workcollapsing to~-workforhome=/home/alice. Also handles the exact-match case (p === home). Clean edge-case fix. -
tui.ts— Empty-choices guard — Moved the!choices.lengthguard before the TTY branch to prevent the Enter handler from dereferencingchoices[0].valueon a TTY with zero choices. Matches the non-TTY null behavior. Clean defensive fix. -
policy-presets.test.ts— Beta filter on git preset — Added!p.betato the git preset test assertion, ensuring the test only expects non-beta builtins (matchingresolveEverythingsemantics). Good test hygiene. -
cli-runner.ts— e2e HOME cleanup — AddedafterAllblock to remove the isolated HOME temp dir after the suite finishes. Prevents accumulation of config artifacts across repeated e2e runs. Good test hygiene. -
configure-wizard.test.ts— Two new assertions: verifiesrunPostSetupAuditis NOT called on cancel, and IS called on completed apply. Ensures the auto-audit handoff is correct.
Re-review summary: The try/finally wrapping in handler.ts directly addresses the most important concern from the first round (telemetry flush could be skipped on early throw). The homeify() and tui fixes are clean defensive improvements. All changes are small, surgical, and well-tested. APPROVED.
Reviewed by Hermes Agent (re-review)
|
✅ Build & test complete (re-review). Results:
Now posting detailed review... |
🔍 Re-Review — Copilot 1.0.71 Fix + CodeRabbit Address📋 Executive SummaryThis re-review covers commit 📊 Change Architecture — New Commits Onlygraph TD
A["handler.ts — try/finally flush"] -->|"guarantees flush on throw"| B["hook-telemetry.ts"]
C["tool-name-canonicalize.ts — Copilot input map"] -->|"path→file_path, file_text→content, old_str→old_string"| D["types.ts — COPILOT_TOOL_INPUT_MAP"]
C -->|"permissionRequest camelCase normalization"| A
E["tui.ts — empty-choices guard"] -->|"returns null before deref"| F["configure-wizard.ts"]
F -->|"homeify with sep boundary"| G["review output"]
H["cli-runner.ts — afterAll rmSync"] -->|"no accumulated tmp artifacts"| I["e2e test isolation"]
style A fill:#FFD700
style C fill:#90EE90
style D fill:#87CEEB
style E fill:#FFD700
style H fill:#FFD700
Legend: 🟢 New | 🔵 Modified | 🟡 Bugfix 🔴 Breaking Changes✅ No breaking changes detected in the new commits.
|
| # | Suggestion | Status |
|---|---|---|
| 1 | Path-anchored regex in telemetry-sanitize | Still open (suggestion, not blocker) |
| 2 | Structured logger for installHooks | Still open (suggestion, not blocker) |
| 3 | process.on("exit") raw-mode restore | Still open (suggestion, not blocker) |
| 4 | Dev-mode logging for dynamic import failure | Still open (suggestion, not blocker) |
🏆 Verdict
VERDICT: APPROVED
All gates green. The Copilot 1.0.71 fix is a real, verified security fix with strong test coverage from verbatim captures. The CodeRabbit-address commit cleanly applies the critical try/finally flush, the homeify boundary fix, the empty-choices guard, and test improvements. 2 CodeRabbit threads intentionally left unresolved (heavy-lift design suggestions, not bugs). No new issues introduced.
Automated re-review · 2026-07-16 14:32 UTC
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Approved — re-review of commits e116310 (Copilot 1.0.71 fix) and 09798e1 (CodeRabbit address).
All gates green (2064 unit + 307 e2e). The Copilot input-key canonicalization and permissionRequest normalization are verified with real 1.0.71 fixtures. The try/finally telemetry flush closes the allow-path event drop. No breaking changes, no regressions.
2 unresolved CodeRabbit threads remain on configure-wizard.ts (wizard replace semantics for existing policies and unticked CLIs) — these are design suggestions, not bugs.
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review: Approved. ✅
3 participants
Overview
One combined PR for two threads of work that landed together on this branch:
32c68df)308ce1d)e116310) — added after live end-to-end verificationFull gate is green locally (typecheck, lint, 2063 unit, build, e2e). See the pinned comment for the end-to-end telemetry + Copilot verification writeup.
1. Installation TUI, config wizard & onboarding
New interactive setup that replaces flag-juggling, plus a branded first-run flow.
failproofai config(aliasesconfigure,setup) — a guided 4-step wizard:① Where (global vs this project) → ② Assistants (multi-select of detected + install-ahead CLIs, sourced from
INTEGRATION_TYPESso all 12 CLIs list) → ③ Policies (themed presets — Secrets & data / Git safety / Ship discipline / Cloud & infra — plus Everything or a searchable custom picker) → ④ Review (shows the exact files it will change, then applies). One flow writes both the hook registration and the enabled-policy config at the chosen scope. Selections replace the enabled set at that scope (new opt-inreplaceflag oninstallHooks; existing additive callers unchanged).resolvePolicySource→resolvePresetSelection.failproofainow flows setup → audit → dashboard on first invocation; every later run goes straight to the dashboard. The~/.failproofai/.launcher-configuredmarker is written only on a finished apply. Replaces/removes the oldfirst-run-nudge.runPostSetupAudit()) — runs the same scanfailproofai auditdoes, pre-warms~/.failproofai/audit-dashboard.json, so the dashboard renders instantly with "N patterns slipping through · M already blocked". Best-effort, opt-out viaFAILPROOFAI_NO_AUTO_AUDIT=1.failproofai) — half-block logomark +failproof aiwordmark (pink "il") + tagline + teal-labelled version/links column. Shared viarenderBrandLogo/renderLaunchBannerin the newtui.tsso the wizard intro and launch banner stay in lockstep (24-bit color where advertised, monochrome shape otherwise, plain text off a TTY).src/hooks/{configure-wizard,policy-presets,tui}.ts(+ testsconfigure-wizard.test.ts,policy-presets.test.ts,tui.test.ts). Removedsrc/hooks/first-run-nudge.ts(+ its test).Cosmetic fixes
components/navbar.tsx,app/globals.css) — on narrow windows the single-row header crushed its children (tab clipped mid-word, version wrapped mid-token, "Reach Us" broke to two lines). Now the version+section cluster never wraps mid-token and sheds below 900px; below 620px the actions row wraps under the nav right-aligned. Verified headless at 1500/760/480px.reach-developers.tsx,scripts/launch.ts,scripts/postinstall.mjspolish.__tests__/e2e/helpers/cli-runner.ts) —runClinow pointsHOME/USERPROFILEat a throwaway temp dir so config-mutating e2e commands no longer touch the developer's real~/.failproofai/.2. PostHog telemetry reliability fix
Problem: the hook binary is short-lived —
bin/failproofai.mjscallsprocess.exit()the momenthandleHookEventreturns — so events fired with un-awaitedvoid trackHookEvent(...)(custom_hooks_loaded,convention_policies_loaded, the*_errorevents) were killed mid-flight and never reached PostHog. They only survived when adeny/instructdecision happened to add a trailingawait. On the common allow path they were dropped entirely — verified: an allowed tool call sent zero events to a local capture sink.Fix:
src/hooks/hook-telemetry.tsnow tracks every in-flight POST in a pending set and exposesflushHookTelemetry().handleHookEventawaits it before returning, andbin's hook error path drains it on throw. Delivery is now reliable regardless of decision.hook_policy_triggered(correctclitag) on a deny.3. Audit failure-reason telemetry
cli_audit_failed(CLI,src/audit/cli.ts) andaudit_run_failed(dashboard,app/api/audit/run/route.ts) previously carried onlyerror_type(the error's class name). They now also senderror_message— the actual failure text, home-directory-stripped to~and length-capped via a sharedsanitizeErrorMessage()helper (lib/telemetry-sanitize.ts) so no local paths leak.cli_audit_started/completedandaudit_run_started/completedalready fired.4. Copilot CLI 1.0.71 file-policy bypass fix (
e116310)Copilot's hook contract drifted since our
1.0.41verification and silently broke file-policy enforcement. Re-verified live against Copilot CLI 1.0.71 with a recorder hook capturing real stdin payloads.{path}, Write{path, file_text}, Edit{path, old_str, new_str}— so path/content builtins (block-.env-files,block-secrets-write) read the wrong keys and silently no-op'd. A live.envread was observed passing. Fix: newCOPILOT_TOOL_INPUT_MAP(keyed by canonical tool name) →file_path/content/old_string/new_string.permissionRequestalone is camelCase (toolNamelowercase,toolInput,sessionId) → newcli === "copilot"normalization branch inhandler.tsso PermissionRequest policies fire.CLAUDE.mdCopilot section rewritten with the verified 1.0.71 contract (incl. a headless-copilot -pproject-scope.github/hooks/discovery caveat).__tests__/hooks/copilot-canonicalize.test.ts+ a handler permissionRequest case). Verified end-to-end: the captured.env-read replay is now denied and 1.0.71 honors it (code:"denied", tool never runs).Testing done
tsc --noEmitclean;lint0 errors (5 pre-existing warnings).telemetry-sanitize.test.ts,flushHookTelemetrycases, handler flush assertion, updated the two audit-telemetry failure assertions, TUI/wizard/preset tests).Suggested review test plan
failproofai config→ walk all 4 steps; try Everything rows on both assistants and policies; confirm the Review step lists the exact files and applies them.~/.failproofai, run barefailproofai→ expect setup → auto-audit → dashboard; second run goes straight to the dashboard.FAILPROOFAI_POSTHOG_HOSTat a local capture server, run an allowed tool call through--hook PreToolUsein a cwd with a custom/convention policy → confirm the load/error events now arrive (previously dropped).cli_audit_failed/audit_run_failednow carry a sanitizederror_message(home path →~, capped at 300 chars).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
failproofai configsetup wizard (scope/assistant selection, policy presets including “Everything”, and an apply preview), plus branded launch/banner rendering.Bug Fixes
error_messagedetails for failed audits/CLI runs.Documentation