feat(audit): add terminal audit outcomes - #53
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds structured JSONL audit logging for MCP operations and lifecycle events, shared secret redaction, serialized secure writes, configurable failure modes, centralized server integration, and comprehensive tests and documentation. ChangesAudit logging
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/upstream/upstream-process-manager.ts (1)
302-310: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEarly start failures skip lifecycle events
PROFILE_NOT_FOUNDand the missing-stdio-command guard run before thetry, so these reachableget()/restart()paths never publishstart-failure/restart-failure. If these errors should be part of the lifecycle record, move the checks into thetryor emit the failure event before throwing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/upstream/upstream-process-manager.ts` around lines 302 - 310, Ensure early validation failures in the private start() method participate in lifecycle tracking: move the profile existence and missing-stdio-command checks inside its try block, or emit the appropriate start-failure/restart-failure event before rethrowing. Preserve the existing MiftahError codes and messages while ensuring get() and restart() record these failures consistently.
🤖 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 `@src/audit/audit-logger.ts`:
- Around line 44-52: Reduce redundant filesystem preflights in
AuditLogger.ensureWritable by adding a short-lived, path-aware cache of
successful readiness checks; skip enqueueing prepareFile when the cached success
is still within the chosen TTL, and invalidate it whenever writeLine or
prepareFile fails so fail-closed behavior remains intact. Update the related
log/write flow around ensureWritable and log to reuse this state without
changing failure recording.
- Around line 29-42: Capture the event timestamp before calling enqueue in
AuditLogger.log, then close over that fixed ISO timestamp when constructing the
JSON payload inside the queued write callback. Preserve enqueue ordering and
failure handling while ensuring each audit event records the time log() was
invoked rather than the later write execution time.
- Around line 86-97: Update openAuditFile to explicitly enforce restrictive
permissions on the audit directory after mkdir, regardless of whether it was
newly created or already existed. Apply setRestrictiveMode to the directory path
with mode 0o700 before opening the audit file, while preserving existing error
handling and cleanup behavior.
In `@src/audit/audit-trail.ts`:
- Around line 115-139: Document the fail-closed behavior in `runAudited` and the
audit configuration guidance: when `finish()` fails after a successful
non-idempotent operation, the caller receives `AUDIT_WRITE_FAILED` even though
the side effect may have completed. Warn operators that retries can duplicate
execution, and recommend pairing fail-closed with `restartOnCrash` or downstream
retry handling for destructive/write tools.
In `@src/secrets/redact.ts`:
- Line 122: Update the retainedLength calculation in the relevant write method
to use Array.from(this.secretValues, secret => secret.length) instead of
spreading the Set before mapping, eliminating the intermediate array while
preserving the existing Math.max behavior.
- Around line 111-148: Preserve URI redaction across chunk boundaries in
createTextStream and safeTextBoundary: do not emit a split segment that can
separate a URI scheme from its credentials or query data. Retain enough
preceding text to keep URI tokens parseable, or maintain URI parsing state
across write calls, and ensure flush() redacts the reconstructed complete token
before returning output. Verify the stderr handling in the upstream process
manager remains safe for long newline-free lines.
In `@src/upstream/upstream-process-manager.ts`:
- Around line 884-902: Extract the duplicated notifyListeners and
publishLifecycle dispatch logic from UpstreamProcessManager and
MultiUpstreamProcessManager into a shared listener-notification utility.
Preserve structuredClone handling, listener iteration, error isolation, and the
existing MIFTAH_LISTENER_FAILED warning details, then update both managers to
use the shared implementation and retain their lifecycle publishing behavior.
In `@tests/audit-outcomes.test.ts`:
- Around line 47-583: Extract the repeated temporary-directory, configuration,
server/transport/client construction, connection, and cleanup logic into a
shared test helper such as createWiredServer. Have it accept configuration
overrides and return the connected client, wrapper, auditPath, and cleanup or
async-dispose capability; update the affected tests to use this helper while
preserving each test’s custom profiles, security, audit, and manager options.
---
Outside diff comments:
In `@src/upstream/upstream-process-manager.ts`:
- Around line 302-310: Ensure early validation failures in the private start()
method participate in lifecycle tracking: move the profile existence and
missing-stdio-command checks inside its try block, or emit the appropriate
start-failure/restart-failure event before rethrowing. Preserve the existing
MiftahError codes and messages while ensuring get() and restart() record these
failures consistently.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: e72bbbee-771e-4fa9-bed6-7c59db9fbe72
📒 Files selected for processing (28)
README.mddocs/architecture.mddocs/config.mddocs/security.mdexamples/github.miftah.jsonexamples/multi-upstream.miftah.jsonsrc/audit/audit-logger.tssrc/audit/audit-trail.tssrc/audit/audit-types.tssrc/cli/create-runtime.tssrc/config/presets.tssrc/config/schema.tssrc/config/types.tssrc/mcp/server/miftah-server.tssrc/mcp/server/operation-pipeline.tssrc/secrets/redact.tssrc/upstream/multi-upstream-process-manager.tssrc/upstream/upstream-process-manager.tssrc/utils/errors.tstests/audit-outcomes.test.tstests/audit.test.tstests/config-runtime-parity.test.tstests/config-schema-contract.test.tstests/fixtures/fake-upstream.mjstests/mcp-wrapper.test.tstests/operation-pipeline.test.tstests/secrets.test.tstests/upstream-manager.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Outside-diff lifecycle finding disposition: not changing early |
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 `@src/secrets/redact.ts`:
- Around line 142-152: Update createTextStream and its suppress helper so
redactedStreamLineMarker only suppresses the current overlong line, not
subsequent input. Reset suppressingOutput when the next line begins or after the
marker is emitted, while preserving the marker for the affected line and
ensuring later write()/flush() calls continue producing output.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 32563dc2-a64c-43a8-8b1b-c1f8dafad578
📒 Files selected for processing (7)
README.mddocs/config.mddocs/security.mdsrc/audit/audit-logger.tssrc/secrets/redact.tstests/audit.test.tstests/secrets.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Validation
npm test(243 passing)npm run lintnpm run typechecknpm run buildnpm run check:packgit diff --checkRefs #13
Summary by CodeRabbit
audit.failureMode(fail-closeddefault,fail-open) with explicit audit health behavior andAUDIT_WRITE_FAILEDsignaling.