Repository navigation
Add governance audit hook exemplar - #123
Conversation
Implement pm-7m8p by making pm-governance-audit the first-party onRead/onWrite hook exemplar. - declare hooks capability in governance-audit manifests and package metadata - register default-inert opt-in compact JSONL sidecar logging via PM_GOVERNANCE_AUDIT_HOOK_LOG - document the package-author hook pattern in SDK, extensions, and package docs - add package-specific regression coverage and close/release the pm item Verification: focused unit/package/SDK tests 62/62; pnpm build; isolated temp install smoke; package doctor; full coverage 2517 tests with 100% statements/branches/functions/lines; changelog check; audit/security/GitHub/Sentry/telemetry gates.
|
To use Codex here, create a Codex account and connect to github. |
|
Warning Review limit reached
More reviews will be available in 49 minutes and 28 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR implements hooks support for the ChangesGovernance Audit Hooks Implementation
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
Code Review
This pull request implements the pm-governance-audit onRead/onWrite hook exemplar, adding the hooks capability to the package manifest and registering hooks that log compact JSONL read/write records to a sidecar path specified by PM_GOVERNANCE_AUDIT_HOOK_LOG. It also includes comprehensive documentation updates and unit tests to verify the new logging behavior. The feedback highlights a critical robustness and performance issue where synchronous I/O operations (mkdirSync and appendFileSync) are executed on every hook invocation without error handling. It is recommended to wrap these operations in a try-catch block to prevent CLI crashes and to cache successfully created directories to avoid redundant disk hits.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch PR info and changed files
gh pr view 123 --repo unbraind/pm-cli --json title,body,files,additions,deletions,commits 2>/dev/null | head -200
echo "---FILES---"
gh pr diff 123 --repo unbraind/pm-cli 2>/dev/null | head -2000Length of output: 49618
🔴 Missing error handling in
|
There was a problem hiding this comment.
Code Review
This pull request implements the pm-governance-audit package as the first-party governance hook exemplar, registering opt-in onRead and onWrite hooks that log compact JSONL records to a sidecar file when PM_GOVERNANCE_AUDIT_HOOK_LOG is set. It also updates the extension manifests, package metadata, documentation, and adds robust unit tests. The feedback highlights a potential reliability issue where synchronous file system operations (mkdirSync and appendFileSync) are performed without error handling. Wrapping these operations in a try...catch block is recommended to prevent any logging failures from crashing the main CLI workflows.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pm-governance-audit/extensions/governance-audit/index.ts`:
- Around line 113-133: In appendHookAuditRecord, make filesystem ops fail-open:
wrap the mkdirSync and appendFileSync calls (the code using absoluteLogPath
derived from process.env[HOOK_LOG_ENV] and the writeContext construction) in a
try/catch so any errors (permissions/ENOSPC/invalid path) are caught and do not
propagate; on error, emit a non-throwing warning (e.g., console.warn or
processLogger.warn) that includes the absoluteLogPath and the error message,
then return silently so normal read/write operations continue unaffected.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 106ca321-02e0-4e3f-ab6d-aef5e9c4fb2d
📒 Files selected for processing (12)
.agents/pm/extensions/.managed-extensions.json.agents/pm/features/pm-7m8p.toon.agents/pm/history/pm-7m8p.jsonlCHANGELOG.mddocs/EXTENSIONS.mddocs/SDK.mdpackages/pm-governance-audit/README.mdpackages/pm-governance-audit/extensions/governance-audit/index.jspackages/pm-governance-audit/extensions/governance-audit/index.tspackages/pm-governance-audit/extensions/governance-audit/manifest.jsonpackages/pm-governance-audit/package.jsontests/unit/builtin-extension-entrypoints.spec.ts
Address PR #123 CodeRabbit and Gemini feedback. - wrap governance-audit sidecar filesystem writes in best-effort try/catch - cache created log directories to avoid redundant mkdir calls - add regression coverage for invalid sidecar paths - track and close pm-mzlu review follow-up with regenerated changelog Verification: focused package tests 62/62; pnpm build; temp governance-audit install smoke with invalid sidecar path fail-open; full coverage 2517 tests at 100%; changelog check; validate history drift ok.
|
@codex review Pushed review follow-up aabcb37: sidecar hook logging now fails open, caches created directories, and includes invalid-path regression coverage. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
To use Codex here, create a Codex account and connect to github. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch the follow-up commit diff
gh api repos/unbraind/pm-cli/commits/aabcb376 --jq '.files[] | "=== \(.filename) ===\n\(.patch)"' 2>/dev/nullLength of output: 18570
The pm-mzlu task chain (create → in_progress → close → release) is also complete with evidence and linked to parent pm-7m8p. Everything looks clean. [resolve] 🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Summary
Implements
pm-7m8pby makingpm-governance-auditthe first-party governance hook exemplar for package authors.hookscapability in governance-audit package metadata and manifestonReadandonWritehooksPM_GOVERNANCE_AUDIT_HOOK_LOGpm-7m8pand regenerates package-ownedCHANGELOG.mdVerification
node scripts/run-tests.mjs test -- tests/unit/builtin-extension-entrypoints.spec.ts tests/unit/package-manifest.spec.ts tests/unit/sdk-index.spec.ts-> 62 passedpnpm buildPM_PATH/PM_GLOBAL_PATHsmoke:pm init --type-preset agile,pm install governance-audit --project, create/update/get withPM_GOVERNANCE_AUDIT_HOOK_LOG, verified compact read/write sidecar and no body/snapshot fields,pm package doctor --project --detail deep --trace --jsonnode scripts/run-tests.mjs coverage-> 146 files, 2517 tests, 100% statements/branches/functions/linespnpm changelog:pm:checkpm validate --check-resolution --check-history-drift-> history drift ok; pre-existing resolution warnings remain on older closed items onlypnpm audit --audit-level moderate-> cleanpm package doctor --project --detail deep --trace --json-> okReview Agents
@codex review
/gemini review
@gemini-code-assist