feat: herdr session protection + version audit + dry-run mode (0.5.0) - #11
Merged
Merged
Conversation
Add 4 herdr rules (server stop, session stop/delete, workspace close) to protect herdr terminal workspace manager from accidental kills. Extend session_kill_targets to include herdr/bermuda daemon. Add pi_opa_net_version to all audit traces (decision metadata, filesystem JSONL, OTLP export) for version correlation. Add PIOPANET_DRY_RUN=1 safety marker — tests run with this flag to guarantee evaluation-only mode. Bump to 0.5.0. 443 tests pass (2 pi-session smoke tests remain pre-existing failures due to stale deployed v0.3.0).
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughThe PR adds four Herdr session-protection rules, package-version and dry-run metadata, audit propagation, release documentation, and E2E coverage. It also records the complete rule-unlock-keys design and implementation decisions. ChangesHerdr protection and release metadata
Rule-unlock-keys exploration
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
PR Summary by Qodofeat: herdr session protection, version audit trail, dry-run mode (0.5.0)
AI Description
Diagram
High-Level Assessment
Files changed (25)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-open-threads.yaml`:
- Line 128: Quote the resolution scalar containing the embedded “: ” so the YAML
parser treats the entire value as a string. Update the resolution field in the
open-thread record while preserving its existing text exactly.
In
`@flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn3-lock-decisions-ttl.md`:
- Around line 127-130: Update the piped-key example under the git stash pop
usage to include the --unlock-stdin flag, ensuring the CLI reads the unlock key
from stdin while preserving the existing command structure.
In `@flow/findings/2026-07-20-rule-unlock-keys/README.md`:
- Around line 8-9: Update the rule-unlock-keys design summary in the README to
state that 15 open threads are resolved, with OT2 remaining open; keep the
existing locked-decision count and other design details unchanged.
In `@schemas/decision-output.v1.json`:
- Around line 98-99: Update the description associated with the rule-family enum
to reflect that it contains 14 families, or remove the outdated numeric count
while preserving the existing grouping/filtering meaning.
In `@tests/e2e/e2e.test.ts`:
- Around line 125-160: Add the missing `killall bermuda` case alongside the
existing `pkill bermuda` entry in the GROUP H test cases, using the appropriate
killall rule identifier and preserving the `killall` family so both command
variants are covered.
- Around line 229-238: Update the dry-run metadata test around the
`pi_opa_net_version` assertions to compare the emitted version against the
authoritative `PI_OPA_NET_VERSION` or package.json version, rather than only
checking that it is a string. Preserve the existing definedness and dry_run
assertions.
🪄 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: 19b75e8c-ddac-4f18-8f9e-4f2a5131a8d4
📒 Files selected for processing (25)
.gitignoreCHANGELOG.mdflow/findings/2026-07-20-rule-unlock-keys/2026-07-20-locked-decisions.yamlflow/findings/2026-07-20-rule-unlock-keys/2026-07-20-open-threads.yamlflow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn1-safetynet-gap-assessment.mdflow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn2-explore-unlock-design-forks.mdflow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn2a-gotcha-unlock-design-batch-a.mdflow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn3-lock-decisions-ttl.mdflow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn4-yagni-finalize-seams.mdflow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn4a-gotcha-yagni-seams-batch-b.mdflow/findings/2026-07-20-rule-unlock-keys/README.mdflow/findings/2026-07-20-rule-unlock-keys/references.mdpackage.jsonpolicy/safety.regoschemas/decision-output.v1.jsonsrc/audit/OtlpAuditSink.tssrc/output/DecisionBuilder.tssrc/pi/audit.tssrc/rules/RuleRegistry.tssrc/rules/catalog.tssrc/version.tstests/e2e/e2e.test.tstests/e2e/pi-session-smoke.test.tstests/unit/audit/OtlpAuditSink.test.tstests/unit/rules/catalog-parity.test.ts
| topic: "IMPORTANT: TTL replayable-within-window (not single-use)" | ||
| raised_in: turn2a | ||
| status: resolved | ||
| resolution: Documentation task (not a design decision). Already decided in turn2a: "document clearly: TTL = time-bounded unlimited use; no single-use mode." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the YAML scalar valid.
Line 128 contains an unquoted : inside a plain scalar. YAML parsing fails, so consumers cannot load the open-thread records.
Proposed fix
- resolution: Documentation task (not a design decision). Already decided in turn2a: "document clearly: TTL = time-bounded unlimited use; no single-use mode."
+ resolution: >-
+ Documentation task (not a design decision). Already decided in turn2a:
+ "document clearly: TTL = time-bounded unlimited use; no single-use mode."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| resolution: Documentation task (not a design decision). Already decided in turn2a: "document clearly: TTL = time-bounded unlimited use; no single-use mode." | |
| resolution: >- | |
| Documentation task (not a design decision). Already decided in turn2a: | |
| "document clearly: TTL = time-bounded unlimited use; no single-use mode." |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 128-128: syntax error: mapping values are not allowed here
(syntax)
🤖 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 `@flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-open-threads.yaml` at
line 128, Quote the resolution scalar containing the embedded “: ” so the YAML
parser treats the entire value as a string. Update the resolution field in the
open-thread record while preserving its existing text exactly.
Source: Linters/SAST tools
Comment on lines
+127
to
+130
| ``` | ||
| pi-opa-net eval "git stash pop" | ||
| pi-opa-net eval "git stash pop" --unlock ll_a3f9c2b8e1d4 | ||
| echo "ttl.1753127056.7c2f..." | pi-opa-net eval "git stash pop" # --unlock-stdin |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pass the --unlock-stdin flag in the example.
Line 130 pipes a key but does not provide --unlock-stdin. The CLI will not treat stdin as an unlock key, so the documented command remains blocked.
Proposed fix
-echo "ttl.1753127056.7c2f..." | pi-opa-net eval "git stash pop" # --unlock-stdin
+echo "ttl.1753127056.7c2f..." | pi-opa-net eval "git stash pop" --unlock-stdin📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` | |
| pi-opa-net eval "git stash pop" | |
| pi-opa-net eval "git stash pop" --unlock ll_a3f9c2b8e1d4 | |
| echo "ttl.1753127056.7c2f..." | pi-opa-net eval "git stash pop" # --unlock-stdin |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 127-127: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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
`@flow/findings/2026-07-20-rule-unlock-keys/2026-07-20-turn3-lock-decisions-ttl.md`
around lines 127 - 130, Update the piped-key example under the git stash pop
usage to include the --unlock-stdin flag, ensuring the CLI reads the unlock key
from stdin while preserving the existing command structure.
Comment on lines
+8
to
+9
| ### rule-unlock-keys design explore (2026-07-20) | ||
| Explored hidden-bypass/unlock-key feature for pi-opa-net. 4 turns → 16 locked decisions (LD-L1..L6, LD-Y1, LD-Y2, LD-G1..G8) + 16 open threads (all resolved). YAGNI applied to salt + audit. Ship surface: src/unlock/* (6 files), src/audit/AuditSink.ts, src/cli/unlock-key.ts, schema additive, policy unchanged. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the open-thread status.
Line 9 says that all 16 threads are resolved. Line 21 states that OT2 is open. State that 15 threads are resolved and OT2 remains open.
🤖 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 `@flow/findings/2026-07-20-rule-unlock-keys/README.md` around lines 8 - 9,
Update the rule-unlock-keys design summary in the README to state that 15 open
threads are resolved, with OT2 remaining open; keep the existing locked-decision
count and other design details unchanged.
Comment on lines
+98
to
99
| "enum": ["git", "docker", "rm", "gcloud", "bq", "gh", "glab", "bd", "builtin", "custom", "tmux", "pkill", "killall", "herdr"], | ||
| "description": "Rule family for grouping/filtering. Matches the 6 families in the rego translation (turn6)." |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the family-count description.
The enum now contains 14 families, but the description still states that it matches six families. Update the count or remove the count from the description.
🤖 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 `@schemas/decision-output.v1.json` around lines 98 - 99, Update the description
associated with the rule-family enum to reflect that it contains 14 families, or
remove the outdated numeric count while preserving the existing
grouping/filtering meaning.
Comment on lines
+125
to
+160
| // GROUP H — herdr session protection. | ||
| { | ||
| command: 'herdr server stop', | ||
| ruleId: 'block-herdr-server-stop', | ||
| family: 'herdr', | ||
| }, | ||
| { | ||
| command: 'herdr session stop foo', | ||
| ruleId: 'block-herdr-session-stop', | ||
| family: 'herdr', | ||
| }, | ||
| { | ||
| command: 'herdr session delete bar', | ||
| ruleId: 'block-herdr-session-delete', | ||
| family: 'herdr', | ||
| }, | ||
| { | ||
| command: 'herdr workspace close baz', | ||
| ruleId: 'block-herdr-workspace-close', | ||
| family: 'herdr', | ||
| }, | ||
| { | ||
| command: 'pkill herdr', | ||
| ruleId: 'block-pkill-tmux-wezterm', | ||
| family: 'pkill', | ||
| }, | ||
| { | ||
| command: 'killall herdr', | ||
| ruleId: 'block-killall-tmux-wezterm', | ||
| family: 'killall', | ||
| }, | ||
| { | ||
| command: 'pkill bermuda', | ||
| ruleId: 'block-pkill-tmux-wezterm', | ||
| family: 'pkill', | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the missing killall bermuda deny case.
The new cases cover pkill herdr, killall herdr, and pkill bermuda, but not killall bermuda. Add both command variants for each new target. This prevents a policy change from covering only one command path.
Proposed test case
{
command: 'pkill bermuda',
ruleId: 'block-pkill-tmux-wezterm',
family: 'pkill',
},
+ {
+ command: 'killall bermuda',
+ ruleId: 'block-killall-tmux-wezterm',
+ family: 'killall',
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // GROUP H — herdr session protection. | |
| { | |
| command: 'herdr server stop', | |
| ruleId: 'block-herdr-server-stop', | |
| family: 'herdr', | |
| }, | |
| { | |
| command: 'herdr session stop foo', | |
| ruleId: 'block-herdr-session-stop', | |
| family: 'herdr', | |
| }, | |
| { | |
| command: 'herdr session delete bar', | |
| ruleId: 'block-herdr-session-delete', | |
| family: 'herdr', | |
| }, | |
| { | |
| command: 'herdr workspace close baz', | |
| ruleId: 'block-herdr-workspace-close', | |
| family: 'herdr', | |
| }, | |
| { | |
| command: 'pkill herdr', | |
| ruleId: 'block-pkill-tmux-wezterm', | |
| family: 'pkill', | |
| }, | |
| { | |
| command: 'killall herdr', | |
| ruleId: 'block-killall-tmux-wezterm', | |
| family: 'killall', | |
| }, | |
| { | |
| command: 'pkill bermuda', | |
| ruleId: 'block-pkill-tmux-wezterm', | |
| family: 'pkill', | |
| }, | |
| // GROUP H — herdr session protection. | |
| { | |
| command: 'herdr server stop', | |
| ruleId: 'block-herdr-server-stop', | |
| family: 'herdr', | |
| }, | |
| { | |
| command: 'herdr session stop foo', | |
| ruleId: 'block-herdr-session-stop', | |
| family: 'herdr', | |
| }, | |
| { | |
| command: 'herdr session delete bar', | |
| ruleId: 'block-herdr-session-delete', | |
| family: 'herdr', | |
| }, | |
| { | |
| command: 'herdr workspace close baz', | |
| ruleId: 'block-herdr-workspace-close', | |
| family: 'herdr', | |
| }, | |
| { | |
| command: 'pkill herdr', | |
| ruleId: 'block-pkill-tmux-wezterm', | |
| family: 'pkill', | |
| }, | |
| { | |
| command: 'killall herdr', | |
| ruleId: 'block-killall-tmux-wezterm', | |
| family: 'killall', | |
| }, | |
| { | |
| command: 'pkill bermuda', | |
| ruleId: 'block-pkill-tmux-wezterm', | |
| family: 'pkill', | |
| }, | |
| { | |
| command: 'killall bermuda', | |
| ruleId: 'block-killall-tmux-wezterm', | |
| family: 'killall', | |
| }, |
🤖 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/e2e.test.ts` around lines 125 - 160, Add the missing `killall
bermuda` case alongside the existing `pkill bermuda` entry in the GROUP H test
cases, using the appropriate killall rule identifier and preserving the
`killall` family so both command variants are covered.
Comment on lines
+229
to
+238
| it('dry-run mode: PIOPANET_DRY_RUN=1 adds dry_run flag to metadata', () => { | ||
| const r = runCli('git stash list', 'json'); | ||
| expect(r.exitCode).toBe(0); | ||
| const rec = r.record!; | ||
| const metadata = rec.metadata as Record<string, unknown>; | ||
| expect(metadata.dry_run).toBe(true); | ||
| expect(metadata.pi_opa_net_version).toBeDefined(); | ||
| expect(typeof metadata.pi_opa_net_version).toBe('string'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the exact package version.
The test only checks that metadata.pi_opa_net_version is a string. It will pass if the CLI emits a stale or hard-coded value. Compare the value with PI_OPA_NET_VERSION or the package.json version.
[details]
[summary]Proposed assertion[/summary]
- expect(metadata.pi_opa_net_version).toBeDefined();
- expect(typeof metadata.pi_opa_net_version).toBe('string');
+ expect(metadata.pi_opa_net_version).toBe(PI_OPA_NET_VERSION);[/details]
🤖 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/e2e.test.ts` around lines 229 - 238, Update the dry-run metadata
test around the `pi_opa_net_version` assertions to compare the emitted version
against the authoritative `PI_OPA_NET_VERSION` or package.json version, rather
than only checking that it is a string. Preserve the existing definedness and
dry_run assertions.
Code Review by Qodo
1. Blank version in audits
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
90
to
94
| }, | ||
| }, | ||
| { key: 'evaluated_at', value: stringValue(entry.evaluated_at) }, | ||
| { key: 'pi_opa_net_version', value: stringValue(entry.pi_opa_net_version) }, | ||
| ]; |
There was a problem hiding this comment.
1. Blank version in audits 🐞 Bug ◔ Observability
OtlpAuditSink always emits the pi_opa_net_version key, but the Hermes/Zcode audit entry writers don’t populate this field, so their OTLP audit records will contain an empty version string instead of the package version. This breaks the PR’s stated goal of having the exact version in every audit trace and makes cross-version correlation unreliable for those integrations.
Agent Prompt
### Issue description
`src/audit/OtlpAuditSink.ts` now always exports a `pi_opa_net_version` field in the OTLP kvlist body. However, not all audit-entry producers (notably `src/hermes/audit.ts` and `src/zcode/audit.ts`) include/populate that field, so the sink serializes it as an empty string.
### Issue Context
- The PR adds `pi_opa_net_version` to the OTLP sink output.
- `stringValue()` turns `undefined` into `""`, so missing producer fields become blank version values.
- `src/pi/audit.ts` was updated to include `pi_opa_net_version`, but Hermes/Zcode writers were not.
### Fix Focus Areas
- src/hermes/audit.ts[38-60]
- src/zcode/audit.ts[38-60]
- src/audit/OtlpAuditSink.ts[73-95]
### Suggested fix
1. Update `src/hermes/audit.ts` and `src/zcode/audit.ts` to include `pi_opa_net_version` in their `AuditEntry` interface and populate it when building the entry. Prefer using the already-produced decision metadata when available:
- `pi_opa_net_version: (input.decision.metadata as any)?.pi_opa_net_version ?? PI_OPA_NET_VERSION`
- (or just `PI_OPA_NET_VERSION` if you want a single source of truth)
2. (Optional hardening) In `OtlpAuditSink`, consider omitting the kv entirely when the version is missing, but only if you **also** ensure all producers populate it to satisfy the “all audit traces” requirement.
3. Add/extend unit tests around Hermes/Zcode audit writers or OTLP sink inputs to assert the exported kv contains a non-empty `pi_opa_net_version`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three user-requested features for pi-opa-net 0.5.0:
1. Herdr session protection rules
Blocks destructive herdr commands (terminal workspace manager for AI agents):
block-herdr-server-stop—herdr server stopblock-herdr-session-stop—herdr session stop <name>block-herdr-session-delete—herdr session delete <name>block-herdr-workspace-close—herdr workspace close <name>Also extends
session_kill_targetsto includeherdrandbermuda(herdr plugin daemon), sopkill herdr/killall bermudaare blocked via existing tmux/pkill/killall rules.2.
pi_opa_net_versionin ALL audit tracesEvery decision metadata + audit log entry now carries the exact package version:
DecisionBuildermetadata →metadata.pi_opa_net_versionsrc/pi/audit.ts) →pi_opa_net_versionfieldOtlpAuditSink) →pi_opa_net_versionin kvlistValue bodyNew
src/version.tsreads version from package.json once at module load.3.
PIOPANET_DRY_RUNsafety modeSetting
PIOPANET_DRY_RUN=1addsdry_run: trueto decision metadata. E2E tests now run with this flag enabled — guarantees CLI is in evaluation-only mode (no accidental command execution if bugs exist).Test results
bun run typecheck✅ cleanbun run lint✅ clean (87 files, no fixes)bun test→ 443 pass, 2 fail (pre-existing pi-session smoke failures due to stale deployed v0.3.0 — will resolve after deploy of 0.5.0)Pre-existing failures
2
Layer A1 — pi-session E2E smoketests fail because the deployed pi-opa-net (v0.3.0) doesn't block commands correctly in spawned pi sessions. These are not caused by this PR — they fail onorigin/maintoo. Deploying 0.5.0 will fix them. Also includes a fix: the smoke test nowgit inits the temp cwd so pi can actually start.Schema change
Additive —
pi_opa_net_version(string) anddry_run(boolean) added toDecisionMetadata.herdradded to family enum. No breaking changes.Checklist
Summary by cubic
Adds herdr session protection to block destructive commands, records the
pi-opa-netversion in all audit traces, and adds aPIOPANET_DRY_RUNsafety mode for evaluation-only runs. Released aspi-opa-net0.5.0 with no breaking changes.New Features
herdrrule family and blocksserver stop,session stop/delete, andworkspace close; extendssession_kill_targetsto includeherdrandbermuda.pi_opa_net_versionto decision metadata, filesystem JSONL, and OTLP export.PIOPANET_DRY_RUN=1marks decisions withdry_run: trueand keeps the CLI in evaluation-only mode.Migration
pi_opa_net_versionanddry_runadded toDecisionMetadata; audit consumers should accept these fields.PIOPANET_DRY_RUN=1to run in safe, non-executing mode.Written for commit b41abb6. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation