feat: reconcile deployed 0.4.0 ghost into main — OTLP sink, MultiSink, rm-rf rule (+ preserve A4) - #9
Conversation
…ink, sinkFactory, rm-rf rule)
Strict TDD RED phase. 4 new test files, 16 failing tests, 0 implementation.
Test matrix:
- tests/unit/audit/MultiSink.test.ts (5 tests): fan-out, sequential ordering,
per-child error isolation, stderr logging with [pi-opa-net] prefix.
- tests/unit/audit/OtlpAuditSink.test.ts (6 tests): OTLP Logs JSON body shape,
severityText deny→ERROR/allow→INFO, resource.service.name, kvlistValue keys,
fetch headers/method, fetch-reject graceful degradation.
- tests/unit/audit/sinkFactory.test.ts (9 tests): env-driven factory routing
(OTel disabled → fs only; OTel+endpoint → MultiSink; OTel+no-endpoint → fs+warn),
parseHeaders edge cases, env param override precedence.
- tests/e2e/block-rm-rf-dangerous-target.test.ts (13 tests): DENY on /, ~, ., ..,
/*, $HOME, /home, -rf -rf /, -fr /; ALLOW on /tmp/specific-dir, ./subdir, rm -r /;
catalog-registration precondition ensures ALL tests fail until rule is added.
RED confirmation: bun test → 399 pass (baseline preserved), 16 fail (all new).
GREEN phase: implement src/audit/{MultiSink,OtlpAuditSink,sinkFactory}.ts,
adapt src/pi/tool-call.ts, insert catalog entry + rego block for rm-rf rule.
Reference: /tmp/scout-ghost-drift.md (port plan)
…, rm-rf rule (+ preserve A4)
0.4.0 was deployed as a ghost binary without a matching repo commit. This brings the source tree in sync with that deployed artifact while preserving the A4 runtime self-check layer unchanged.
Changes copied/adapted from /tmp/piopanet-040-ghost/:
- src/audit/MultiSink.ts (verbatim)
- src/audit/OtlpAuditSink.ts (verbatim)
- src/audit/sinkFactory.ts (verbatim + exported parseHeaders for RED test)
- policy/safety.rego: block-rm-rf-dangerous-target rule block (verbatim)
- src/rules/catalog.ts: block-rm-rf-dangerous-target entry
- src/pi/tool-call.ts: use createAuditSink({ cwd }) from new sink factory
- tests/unit/audit/sinkFactory.test.ts: biome auto-format (no semantic change)
RED commit: 02723a3
- CHANGELOG [0.4.1] entry: reconcile deployed 0.4.0 ghost (OTLP, MultiSink, sinkFactory, rm-rf) + preserve A4 - README: add OTLP audit sink env vars + rm-rf dangerous-target rule - SKILL.md: same env vars + rule - package.json: 0.3.3 → 0.4.1
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughChangesAudit sink routing
Dangerous rm policy
Release metadata and documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ToolCall
participant SinkFactory
participant MultiSink
participant FileAudit
participant OtlpAudit
participant Collector
ToolCall->>SinkFactory: createAuditSink(cwd)
SinkFactory->>MultiSink: configure filesystem and OTLP sinks
ToolCall->>MultiSink: write(audit entry)
MultiSink->>FileAudit: write(audit entry)
MultiSink->>OtlpAudit: write(audit entry)
OtlpAudit->>Collector: POST OTLP Logs JSON
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Kilo Code Review could not run — your account is out of credits. Add credits or switch to a free model to enable reviews on this change. |
![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 (1)
policy/safety.rego (1)
372-388: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated deny message literal across both branches.
The exact same message string is repeated in the args-based (line 378) and raw-based (line 387) deny rules. Extracting it to a shared variable would prevent future edits from silently diverging between the two branches.
♻️ Proposed dedup
+rm_dangerous_target_msg := "rm -rf on dangerous targets (/, ~, ., .., *, /*, $HOME, /home) is blocked. Use specific paths like /tmp/dir or ./subdir." + # Args-based deny: dangerous literal target present in args deny[msg] if { input.program == "rm" rm_has_recursive(input.args) rm_has_force(input.args) rm_has_dangerous_arg_target(input.args) - msg := "rm -rf on dangerous targets (/, ~, ., .., *, /*, $HOME, /home) is blocked. Use specific paths like /tmp/dir or ./subdir." + msg := rm_dangerous_target_msg } # Raw-based deny: dangerous glob/env token present in raw (disappeared from args) deny[msg] if { input.program == "rm" rm_has_recursive(input.args) rm_has_force(input.args) rm_raw_dangerous_token(input.raw) - msg := "rm -rf on dangerous targets (/, ~, ., .., *, /*, $HOME, /home) is blocked. Use specific paths like /tmp/dir or ./subdir." + msg := rm_dangerous_target_msg }🤖 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 `@policy/safety.rego` around lines 372 - 388, Define a shared variable for the rm safety denial message in policy/safety.rego, then have both deny rules using rm_has_dangerous_arg_target and rm_raw_dangerous_token assign msg from that variable instead of duplicating the literal. Preserve the existing message text and rule behavior.
🤖 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 `@README.md`:
- Line 223: Update the dangerous-target catalogs to include the implemented ~/*
pattern, preserving the existing wording and formatting: add ~/* in README.md
lines 223-223, skills/pi-opa-net/SKILL.md lines 74-74, and CHANGELOG.md lines
13-13.
In `@src/audit/OtlpAuditSink.ts`:
- Line 104: Update the timeUnixNano assignment in the OTLP audit export to
compute nanoseconds with BigInt before stringification, using BigInt(Date.now())
and a BigInt nanosecond multiplier to avoid unsafe Number arithmetic.
- Around line 54-70: Update OtlpAuditSink.write to bound the fetch duration with
an AbortController-based timeout, and validate the response after fetch so
non-2xx statuses throw and enter the existing error logging path. Preserve the
non-fatal behavior by continuing to catch timeout, network, and HTTP-status
failures without propagating them.
In `@tests/e2e/block-rm-rf-dangerous-target.test.ts`:
- Around line 32-34: Update the OPA_BIN fallback used by the e2e test to use the
PATH-resolved command name “opa” instead of the machine-specific absolute path,
while preserving the OPA_AVAILABLE and SKIP_REASON behavior.
---
Nitpick comments:
In `@policy/safety.rego`:
- Around line 372-388: Define a shared variable for the rm safety denial message
in policy/safety.rego, then have both deny rules using
rm_has_dangerous_arg_target and rm_raw_dangerous_token assign msg from that
variable instead of duplicating the literal. Preserve the existing message text
and rule behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ed7bd169-4f69-488c-a3d1-d47f3806d9b5
📒 Files selected for processing (14)
CHANGELOG.mdREADME.mdpackage.jsonpolicy/safety.regoskills/pi-opa-net/SKILL.mdsrc/audit/MultiSink.tssrc/audit/OtlpAuditSink.tssrc/audit/sinkFactory.tssrc/pi/tool-call.tssrc/rules/catalog.tstests/e2e/block-rm-rf-dangerous-target.test.tstests/unit/audit/MultiSink.test.tstests/unit/audit/OtlpAuditSink.test.tstests/unit/audit/sinkFactory.test.ts
|
|
||
| ### Rules | ||
|
|
||
| - `block-rm-rf-dangerous-target` — blocks `rm -rf` on `/`, `~`, `.`, `..`, `*`, `/*`, `$HOME`, `/home`. Safe carve-outs: `/tmp/<specific>`, `./<specific>`, named dirs. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the implemented ~/* dangerous target consistently.
The Rego policy blocks ~/*, but all three user-facing/release descriptions omit it. Add ~/* to each listed dangerous-target catalog so the documentation matches policy/safety.rego.
README.md#L223-L223: add~/*to the blocked target list.skills/pi-opa-net/SKILL.md#L74-L74: add~/*to the blocked target list.CHANGELOG.md#L13-L13: add~/*to the 0.4.1 release-note target list.
📍 Affects 3 files
README.md#L223-L223(this comment)skills/pi-opa-net/SKILL.md#L74-L74CHANGELOG.md#L13-L13
🤖 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 `@README.md` at line 223, Update the dangerous-target catalogs to include the
implemented ~/* pattern, preserving the existing wording and formatting: add ~/*
in README.md lines 223-223, skills/pi-opa-net/SKILL.md lines 74-74, and
CHANGELOG.md lines 13-13.
| async write(entry: unknown): Promise<void> { | ||
| const body = this.buildOtlpBody(entry as AuditEntry); | ||
| try { | ||
| await fetch(this.endpoint, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json', ...this.headers }, | ||
| body: JSON.stringify(body), | ||
| }); | ||
| } catch (err) { | ||
| // Audit export failure is non-fatal — log to stderr and continue. | ||
| console.error( | ||
| `[pi-opa-net] OTLP audit export failed, continuing without OTel: ${ | ||
| err instanceof Error ? err.message : String(err) | ||
| }`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File map ==\n'
git ls-files 'src/audit/OtlpAuditSink.ts' 'src/pi/tool-call.ts' 'src/audit/*' | sed 's#^`#-` #'
printf '\n== OtlpAuditSink outline ==\n'
ast-grep outline src/audit/OtlpAuditSink.ts --view expanded || true
printf '\n== Relevant slices ==\n'
sed -n '1,220p' src/audit/OtlpAuditSink.ts | cat -n
printf '\n--- tool-call slice ---\n'
sed -n '120,190p' src/pi/tool-call.ts | cat -nRepository: buihongduc132/opa-net
Length of output: 1919
🌐 Web query:
Bun fetch default timeout AbortSignal.timeout documentation
💡 Result:
In Bun, fetch() supports AbortSignal.timeout() as a standard way to manage request timeouts, but it also features a native timeout option in its RequestInit configuration [1][2][3]. Key details regarding timeout behavior in Bun: 1. Native Timeout Support: Beyond AbortSignal, Bun allows you to pass a timeout property directly in the fetch options object (e.g., fetch(url, { timeout: 5000 })) [3]. 2. Default Timeout Behavior: Historically, Bun implemented a default idle socket timeout to prevent requests from hanging indefinitely [2][4]. Recent updates have refined this, distinguishing between a whole-request deadline (timeout) and specific phases like connection or inactivity (connectTimeout, socketTimeout) [2]. 3. Disabling Timeouts: You can disable the default request timeout by setting { timeout: false } or { timeout: 0 } in the fetch options [2][4]. This is often used as an "escape hatch" for long-polling or Server-Sent Events (SSE) [2]. 4. Interaction with AbortSignal: AbortSignal.timeout(ms) is the standard Web API way to trigger an AbortError after a specific duration [1][5]. While you can use both native timeout and AbortSignal.timeout simultaneously, they serve as different mechanisms for managing request lifecycle [2]. For authoritative guidance, refer to the official Bun networking documentation, which explicitly recommends AbortSignal.timeout for standard use cases [1]. Note that Bun's internal implementation of these timeouts continues to evolve to better support diverse networking requirements [2].
Citations:
- 1: https://bun.com/docs/runtime/networking/fetch
- 2: fetch: add connectTimeout, socketTimeout, and a whole-request timeout oven-sh/bun#33338
- 3: Add
timeout: numberoption to fetch oven-sh/bun#19767 - 4: Set
fetchtimeout to 5 minutes oven-sh/bun#6217 - 5: https://bun.com/reference/globals/AbortSignal/timeout
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== OtlpAuditSink outline ==\n'
ast-grep outline src/audit/OtlpAuditSink.ts --view expanded || true
printf '\n== OtlpAuditSink lines 1-220 ==\n'
sed -n '1,220p' src/audit/OtlpAuditSink.ts | cat -n
printf '\n== tool-call lines 120-190 ==\n'
sed -n '120,190p' src/pi/tool-call.ts | cat -nRepository: buihongduc132/opa-net
Length of output: 1919
Add a bounded timeout and reject non-2xx OTLP responses in src/audit/OtlpAuditSink.ts:54-70. write() is awaited on the deny/block path, so a slow collector can delay the response, and HTTP 4xx/5xx replies are currently treated as success because only thrown fetch failures are caught.
🤖 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/audit/OtlpAuditSink.ts` around lines 54 - 70, Update OtlpAuditSink.write
to bound the fetch duration with an AbortController-based timeout, and validate
the response after fetch so non-2xx statuses throw and enter the existing error
logging path. Preserve the non-fatal behavior by continuing to catch timeout,
network, and HTTP-status failures without propagating them.
| { | ||
| logRecords: [ | ||
| { | ||
| timeUnixNano: String(Date.now() * 1_000_000), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' src/audit/OtlpAuditSink.ts
printf '\n---\n'
rg -n "timeUnixNano|Date\.now\(\) \* 1_000_000|BigInt\(Date\.now\(\)\)" srcRepository: buihongduc132/opa-net
Length of output: 1919
Use a BigInt nanosecond timestamp here
Date.now() * 1_000_000 exceeds Number.MAX_SAFE_INTEGER, so the exported OTLP timestamp can be rounded before stringification. Use a BigInt-based nanosecond value instead, e.g. String(BigInt(Date.now()) * 1_000_000n).
🤖 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/audit/OtlpAuditSink.ts` at line 104, Update the timeUnixNano assignment
in the OTLP audit export to compute nanoseconds with BigInt before
stringification, using BigInt(Date.now()) and a BigInt nanosecond multiplier to
avoid unsafe Number arithmetic.
| const OPA_BIN = process.env.OPA_BIN ?? '/home/bhd/.local/share/mise/installs/opa/1.18.2/opa'; | ||
| const OPA_AVAILABLE = existsSync(OPA_BIN); | ||
| const SKIP_REASON = !OPA_AVAILABLE ? 'OPA binary not found' : ''; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the same hardcoded-path convention already exists elsewhere,
# and how CI wires OPA_BIN for e2e tests.
rg -n 'OPA_BIN' tests/e2e/cli-contract.test.ts
rg -rn 'OPA_BIN' .github/workflows 2>/dev/null
fd -e yml -e yaml . .github/workflows --exec grep -l 'OPA_BIN' {} \;Repository: buihongduc132/opa-net
Length of output: 1948
Replace the machine-specific OPA fallback
OPA_BIN should not default to /home/bhd/.local/share/mise/installs/opa/1.18.2/opa; on any other machine, this makes the e2e suite skip unless OPA_BIN is set. Use a PATH-based fallback instead, e.g. process.env.OPA_BIN ?? 'opa'.
🤖 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/e2e/block-rm-rf-dangerous-target.test.ts` around lines 32 - 34, Update
the OPA_BIN fallback used by the e2e test to use the PATH-resolved command name
“opa” instead of the machine-specific absolute path, while preserving the
OPA_AVAILABLE and SKIP_REASON behavior.
1 participant
Reconcile deployed 0.4.0 ghost into repo main
Problem
The deployed production copy of pi-opa-net (
~/.pi/agent/npm/node_modules/pi-opa-net/) contained unpublished features (OTLP/HTTP audit sink, MultiSink, config-driven audit factory,rm -rfdangerous-target policy) that had never been committed to git. Worse, the ghost release silently dropped the A4 runtime self-check layer (markHookRegistered()+runtime-self-check.ts) that was added in PR #7.This meant:
What this PR does
Reconciles the ghost into repo main on branch
feat/reconcile-040-ghost:02723a3): added 16 failing tests for the 0.4.0 ghost features (MultiSink, OtlpAuditSink, sinkFactory, rm-rf rule).c110100): implemented the features by copying/adapting the ghost source, preserving A4 untouched.Changes
src/audit/MultiSink.ts— fan-out audit dispatch (filesystem + OTLP simultaneously), per-child error isolation.src/audit/OtlpAuditSink.ts— OTLP/HTTP logs sink; graceful degradation (network failure is non-fatal).src/audit/sinkFactory.ts— config-driven audit factory (PIOPANET_OTEL_ENABLED,PIOPANET_OTEL_ENDPOINT, etc.).src/pi/tool-call.ts— wire default sink throughcreateAuditSink({cwd}).src/rules/catalog.ts+policy/safety.rego— addblock-rm-rf-dangerous-targetrule (blocksrm -rfon/,~,.,..,*,/*,/home/bhd,/home; carve-outs preserved).README.md+skills/pi-opa-net/SKILL.md— document OTLP env vars + new rule.CHANGELOG.md— [0.4.1] entry.package.json— bump to 0.4.1.Verification
bun run check→ 432 pass, 0 fail (typecheck + lint:ci + test --coverage)./tmp/verifier2-premerge.md).markHookRegisteredpresent insrc/pi/index.tslines 3 + 30;runtime-self-check.tsunchanged.pi-session-smoke.test.ts(live pi E2E gate) passes standalone (3/3, twice).After merge
npm publishto push 0.4.1 to registry.pi install pi-opa-net@latestto redeploy prod with both 0.4.0 features AND A4.Summary by cubic
Reconciles the deployed 0.4.0 ghost into
pi-opa-netmain, adding OTLP/HTTP audit export,MultiSinkfan-out, and a safety rule for dangerousrm -rftargets while keeping the A4 runtime self-check intact. Ships as 0.4.1 with docs and tests; default behavior stays the same.New Features
MultiSinkfan-out with per-child error isolation; network failures are non-fatal.PIOPANET_OTEL_ENABLED,PIOPANET_OTEL_ENDPOINT,PIOPANET_OTEL_SERVICE_NAME,PIOPANET_OTEL_HEADERS); default remains filesystem-only, andtool-callnow uses it.block-rm-rf-dangerous-targetrule with safe carve-outs (/tmp/<specific>,./<specific>); docs updated.Migration
PIOPANET_OTEL_ENABLED=1andPIOPANET_OTEL_ENDPOINT=<http://.../v1/logs>(optional:PIOPANET_OTEL_SERVICE_NAME,PIOPANET_OTEL_HEADERS).Written for commit fa17a5b. Summary will update on new commits.
Summary by CodeRabbit
New Features
block-rm-rf-dangerous-targetsafety rule to block recursive forced deletion of dangerous paths while allowing safe paths.Bug Fixes
Documentation