feat: add governance foundation for agent loops#26
Merged
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds a "Governance Foundation" for Dorian v1.4: durable human-authored Goal records, a ChangesGovernance Foundation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Host as Claude Code Host
participant SubagentStop as dorian_loop_preflight.py
participant Gate as dorian gate
participant Packet as .dorian/local/last-decision.json
participant Veto as dorian_preflight_veto.py
Host->>SubagentStop: SubagentStop event
SubagentStop->>Gate: invoke dorian gate (stdin tool JSON)
Gate-->>SubagentStop: decision packet (continue/repair/escalate)
SubagentStop->>Packet: write stamped packet (timestamp, repo_root, base_ref, nonce)
Host->>Veto: PreToolUse event (tool JSON via stdin)
Veto->>Packet: read last-decision.json
Veto-->>Veto: check freshness, identity, sensitivity, mode
Veto-->>Host: exit 0 (allow) or exit 2 (block)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
src/dorian/templates/claude_code/dorian-governance/hooks/dorian_loop_preflight.py (2)
55-60: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNon-atomic write of the governance decision packet.
last-decision.jsonis written with a plainopen(..., "w")+json.dump, truncating-then-writing in place. If this hook is killed mid-write (or two SubagentStop invocations race), the concurrently-reading PreToolUse veto hook can observe a truncated/partial file. The project already has atomic-write helpers for exactly this class of file (sidecars.write_sidecar,_atomic_writeinclaude_code.py/init.py); this hook doesn't reuse either.♻️ Suggested atomic write
try: os.makedirs(".dorian/local", exist_ok=True) - with open(".dorian/local/last-decision.json", "w", encoding="utf-8") as fh: - json.dump(packet, fh) + tmp = ".dorian/local/last-decision.json.tmp" + with open(tmp, "w", encoding="utf-8") as fh: + json.dump(packet, fh) + os.replace(tmp, ".dorian/local/last-decision.json") except Exception: return 0🤖 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/dorian/templates/claude_code/dorian-governance/hooks/dorian_loop_preflight.py` around lines 55 - 60, The governance packet write in dorian_loop_preflight.py is non-atomic, so replace the plain open/json.dump path with the project’s atomic write mechanism used elsewhere for sidecars (for example sidecars.write_sidecar or the _atomic_write pattern from claude_code.py/init.py). Keep the existing os.makedirs setup and update the write in the hook that produces last-decision.json so readers never see a partial file, using the existing packet-building flow in this hook.
38-41: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win600s timeout is long for a per-turn synchronous hook.
Other subprocess calls in this codebase (e.g.
changed_pathsindorian_claim_warrants_stop.py) use a 10s timeout for a comparable git operation. A 10-minute timeout here means a hung/slowdorian gatecall (e.g. store lock contention) can stall every SubagentStop turn for up to 10 minutes before this hook fails open.♻️ Suggested reduction
- cmd, input="{}", capture_output=True, text=True, timeout=600, check=False + cmd, input="{}", capture_output=True, text=True, timeout=30, check=False🤖 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/dorian/templates/claude_code/dorian-governance/hooks/dorian_loop_preflight.py` around lines 38 - 41, The per-turn subprocess call in the preflight hook uses an excessively long timeout, which can block SubagentStop turns for too long. Reduce the timeout in the dorian_loop_preflight hook’s subprocess.run invocation to a much shorter value consistent with other git-related hooks (for example, the same 10s pattern used in changed_paths), and keep the existing fail-open behavior on timeout or nonzero exit.
🤖 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 `@docs/GOVERNANCE_DATA_MODEL.md`:
- Around line 15-23: The record-count wording is inconsistent because only the
Goal entry is actually a new durable record type, while the gate decision packet
is stdout-only and the last-decision packet is ephemeral. Update the summary
around the table to separate durable records from emitted/runtime-only packets,
and use the existing symbols “Goal,” “Gate decision packet,” “Last-decision
packet,” and “v1.4” to make the classification explicit and consistent.
In `@docs/VALIDATION_HONESTY.md`:
- Around line 57-67: Limit the determinism note in VALIDATION_HONESTY.md to
C4-backed pytest claims only; the current text overstates that C1/C3/C5 families
are deterministic by construction. Update the paragraph around the “C4 verdict
determinism” section so it explicitly scopes the stability discussion to C4 and,
if mentioning other families, qualify that shell-backed claims can still vary
with environment or time. Use the existing “C4 verdict determinism” heading and
“revalidate”/“TRUSTED”/“REVOKED” wording to keep the edit localized.
In `@README.md`:
- Around line 386-390: The README section for dorian governance install
overstates attended behavior; update the description to note the sensitive-path
exception, where certain escalate cases still block even when attended. Either
add that caveat directly in this paragraph or point readers to the
enforcement-modes matrix, and make sure the wording around the PreToolUse veto
and fail-open behavior matches the actual enforcement in the governance adapter.
In `@src/dorian/commands.py`:
- Around line 1211-1253: The plain-text path in _cmd_governance_install is
dropping warnings returned by claude_code.apply, so mirror result.warnings in
the non-JSON output as well. After calling claude_code.apply, keep the existing
JSON behavior, and in the human-readable branch print each warning from
result.warnings before the final summary so users see cases like the non-git
repository warning. Use the existing _cmd_governance_install, claude_code.apply,
and result.warnings symbols to place the change.
In `@src/dorian/goals.py`:
- Around line 62-69: `load()` currently reads the goal file without the same
containment guard that `save()` gets through `sidecars.write_sidecar`. Update
`load(repo, goal_id)` to validate the resolved path with
`sidecars.ensure_within` before calling `read_text`, using the same `repo` root
and path derived from `goal_path(goal_id)` so `../` escapes are rejected
consistently. Keep the schema-loading logic unchanged; only add the missing
path-safety check around the file read.
In `@src/dorian/templates/claude_code/dorian-governance/README.md`:
- Around line 10-11: The README wording around hooks/dorian_preflight_veto.py
overstates the behavior by implying every `escalate` decision always blocks the
tool. Update the description for the `PreToolUse` hook to reflect the actual
gate in `dorian_preflight_veto.py`: it only exits 2 in the strict block cases,
while attended modes can fail open for non-sensitive, path-scoped breaks. Keep
the wording aligned with the `PreToolUse` and `escalate` behavior so it
describes the conditional blocking accurately.
In `@tests/test_firewall_import_closure.py`:
- Around line 182-228: The import walker in external_imports/_absorb misses
ancestor package __init__.py files when a deep module name is discovered, so
forbidden imports in intermediate package initializers can be skipped. Update
the traversal to enqueue and scan every dotted parent package for each internal
import root returned by _scan (for example, walk from a module like
checkers.sub.leaf back through checkers.sub and checkers) before or alongside
the existing _module_path resolution. Keep using sites, stack, and seen so
ancestor __init__.py files are absorbed transitively and package-level imports
are not missed.
---
Nitpick comments:
In
`@src/dorian/templates/claude_code/dorian-governance/hooks/dorian_loop_preflight.py`:
- Around line 55-60: The governance packet write in dorian_loop_preflight.py is
non-atomic, so replace the plain open/json.dump path with the project’s atomic
write mechanism used elsewhere for sidecars (for example sidecars.write_sidecar
or the _atomic_write pattern from claude_code.py/init.py). Keep the existing
os.makedirs setup and update the write in the hook that produces
last-decision.json so readers never see a partial file, using the existing
packet-building flow in this hook.
- Around line 38-41: The per-turn subprocess call in the preflight hook uses an
excessively long timeout, which can block SubagentStop turns for too long.
Reduce the timeout in the dorian_loop_preflight hook’s subprocess.run invocation
to a much shorter value consistent with other git-related hooks (for example,
the same 10s pattern used in changed_paths), and keep the existing fail-open
behavior on timeout or nonzero exit.
🪄 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: 9f09c6a7-2cca-4ec1-85c2-a32d2ebe8b66
📒 Files selected for processing (24)
.gitignoreCHANGELOG.mdREADME.mddocs/DORIAN_PANE.mddocs/GOVERNANCE_DATA_MODEL.mddocs/SECURITY_BOUNDARY.mddocs/TESTING.mddocs/VALIDATION_HONESTY.mdsrc/dorian/cli.pysrc/dorian/commands.pysrc/dorian/goals.pysrc/dorian/governance.pysrc/dorian/sidecars.pysrc/dorian/templates/claude_code/dorian-governance/README.mdsrc/dorian/templates/claude_code/dorian-governance/hooks/dorian_loop_preflight.pysrc/dorian/templates/claude_code/dorian-governance/hooks/dorian_preflight_veto.pysrc/dorian/templates/claude_code/dorian-governance/reference/enforcement-modes.mdsrc/dorian/templates/claude_code/dorian-governance/settings.dorian-governance.example.jsontests/test_firewall_import_closure.pytests/test_gate.pytests/test_goals_coverage.pytests/test_governance_install.pytests/test_governance_packaging.pytests/test_sidecars.py
Comment on lines
+15
to
+23
| v1.4 adds three durable record types and one ephemeral runtime file: | ||
|
|
||
| | Record | Path | Written by | Tracked in git? | | ||
| |---|---|---|---| | ||
| | **Goal** | `.dorian/goals/<goal_id>.goal.json` | `dorian goal add` | yes (a repo can commit its goals) | | ||
| | **Warrant** (unchanged) | `<artifact>.warrant` | `dorian seal`/`verify` | yes | | ||
| | **Gate decision packet** | stdout of `dorian gate` (not persisted by core) | `dorian gate` | n/a — emitted, not stored | | ||
| | **Last-decision packet** | `.dorian/local/last-decision.json` | the SubagentStop **host hook** | **no** — ephemeral, gitignored | | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Tighten the record-count wording.
v1.4 does not add “three durable record types” here: the gate packet is emitted stdout, and the last-decision packet is explicitly ephemeral. Consider naming the new durable record separately from the emitted/runtime-only packets so the model stays consistent.
♻️ Proposed wording fix
-v1.4 adds three durable record types and one ephemeral runtime file:
+v1.4 adds one durable record type, one emitted gate packet, and one ephemeral runtime file:📝 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
| v1.4 adds three durable record types and one ephemeral runtime file: | |
| | Record | Path | Written by | Tracked in git? | | |
| |---|---|---|---| | |
| | **Goal** | `.dorian/goals/<goal_id>.goal.json` | `dorian goal add` | yes (a repo can commit its goals) | | |
| | **Warrant** (unchanged) | `<artifact>.warrant` | `dorian seal`/`verify` | yes | | |
| | **Gate decision packet** | stdout of `dorian gate` (not persisted by core) | `dorian gate` | n/a — emitted, not stored | | |
| | **Last-decision packet** | `.dorian/local/last-decision.json` | the SubagentStop **host hook** | **no** — ephemeral, gitignored | | |
| v1.4 adds one durable record type, one emitted gate packet, and one ephemeral runtime file: | |
| | Record | Path | Written by | Tracked in git? | | |
| |---|---|---|---| | |
| | **Goal** | `.dorian/goals/<goal_id>.goal.json` | `dorian goal add` | yes (a repo can commit its goals) | | |
| | **Warrant** (unchanged) | `<artifact>.warrant` | `dorian seal`/`verify` | yes | | |
| | **Gate decision packet** | stdout of `dorian gate` (not persisted by core) | `dorian gate` | n/a — emitted, not stored | | |
| | **Last-decision packet** | `.dorian/local/last-decision.json` | the SubagentStop **host hook** | **no** — ephemeral, gitignored | |
🤖 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 `@docs/GOVERNANCE_DATA_MODEL.md` around lines 15 - 23, The record-count wording
is inconsistent because only the Goal entry is actually a new durable record
type, while the gate decision packet is stdout-only and the last-decision packet
is ephemeral. Update the summary around the table to separate durable records
from emitted/runtime-only packets, and use the existing symbols “Goal,” “Gate
decision packet,” “Last-decision packet,” and “v1.4” to make the classification
explicit and consistent.
Comment on lines
+57
to
+67
| ## C4 verdict determinism is conditional on a deterministic test | ||
|
|
||
| A `C4` (`pytest:`) verdict is deterministic *given a deterministic test*. If the bound test is | ||
| **flaky** — passing on some runs, failing on others with the source unchanged — the warrant's | ||
| trust state can flip between `TRUSTED` and `REVOKED` across `revalidate` runs. That is a | ||
| **test-quality issue, not Dorian nondeterminism**: the C1/C3/C5-typed families are deterministic by | ||
| construction, and the same flaky test would flip a CI run too. But it means a `C4` claim's truth is | ||
| only as stable as the test backing it. An **N-run stability lever** (consensus across repeats) is a | ||
| **deferred (v1.5) idea**; until then, read a flaky `C4` flip as a signal about the test, not the | ||
| code, and prefer deterministic tests for load-bearing behavior claims. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Scope the determinism claim to C4-backed tests.
This overgeneralizes by treating C1/C3/C5 families as deterministic by construction. shell:-backed claims can still vary with environment or time, so please limit the note to C4 pytest claims or qualify the exception.
🤖 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 `@docs/VALIDATION_HONESTY.md` around lines 57 - 67, Limit the determinism note
in VALIDATION_HONESTY.md to C4-backed pytest claims only; the current text
overstates that C1/C3/C5 families are deterministic by construction. Update the
paragraph around the “C4 verdict determinism” section so it explicitly scopes
the stability discussion to C4 and, if mentioning other families, qualify that
shell-backed claims can still vary with environment or time. Use the existing
“C4 verdict determinism” heading and “revalidate”/“TRUSTED”/“REVOKED” wording to
keep the edit localized.
Comment on lines
+386
to
+390
| - **`dorian governance install`** — scaffold the **Claude Code** governance adapter: a | ||
| `SubagentStop` hook that runs `dorian gate`, plus a **fail-closed** `PreToolUse` veto (blocks a | ||
| mutating tool on a standing `escalate` under a strict policy — `unattended`, or | ||
| `DORIAN_EFFORT=godmode`; fails open when a human is attended). It is **steering plus a host-side | ||
| veto, not a sandbox**. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the sensitive-path exception.
This is not fully fail-open when attended: sensitive-path escalate cases still block. Please call that out here or link to the enforcement-modes matrix so this section does not overstate the behavior.
🤖 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` around lines 386 - 390, The README section for dorian governance
install overstates attended behavior; update the description to note the
sensitive-path exception, where certain escalate cases still block even when
attended. Either add that caveat directly in this paragraph or point readers to
the enforcement-modes matrix, and make sure the wording around the PreToolUse
veto and fail-open behavior matches the actual enforcement in the governance
adapter.
Comment on lines
+1211
to
+1253
| def _cmd_governance_install(args: argparse.Namespace) -> int: | ||
| """Scaffold the Claude Code governance adapter (two host hooks + settings example + docs) | ||
| into .claude/. Writes files only; never overwrites without --force; idempotent. Not a | ||
| sandbox. The hooks own the exit-2 veto and wall-clock; Dorian core stays pure.""" | ||
| target = Path(args.target).resolve() if args.target else _repo(args) | ||
| if _missing_repo(target, "governance install"): | ||
| return EXIT_USAGE | ||
| try: | ||
| plan = claude_code.build_plan( | ||
| target, groups=governance.DEFAULT_GROUPS, manifest=governance.GOVERNANCE_MANIFEST | ||
| ) | ||
| result = claude_code.apply(plan, force=args.force, dry_run=args.dry_run) | ||
| except (ValueError, OSError) as exc: | ||
| print(f"dorian governance install: {exc}", file=sys.stderr) | ||
| return EXIT_USAGE | ||
| if args.json: | ||
| print( | ||
| json.dumps( | ||
| { | ||
| "repo": str(plan.repo_root), | ||
| "is_git": plan.is_git, | ||
| "dry_run": args.dry_run, | ||
| "created": list(result.created), | ||
| "overwritten": list(result.overwritten), | ||
| "skipped": list(result.skipped), | ||
| "warnings": list(result.warnings), | ||
| }, | ||
| indent=2, | ||
| ) | ||
| ) | ||
| else: | ||
| verb = "would write" if args.dry_run else "wrote" | ||
| print( | ||
| f"dorian governance install: {verb} {len(result.created)} file(s)" | ||
| f" (skipped {len(result.skipped)}, overwrote {len(result.overwritten)})" | ||
| f" under {plan.repo_root}" | ||
| ) | ||
| for created in result.created: | ||
| print(f" + {created}") | ||
| for skipped in result.skipped: | ||
| print(f" = {skipped} (exists; --force to overwrite)") | ||
| print("next: merge settings.dorian-governance.example.json into .claude/settings.json") | ||
| return EXIT_OK |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Warnings from claude_code.apply are silently dropped in the human-readable output.
result.warnings (e.g. the "not a git repository" warning) is only included in the --json branch (Line 1236). The plain-text branch (Lines 1241-1252) never prints result.warnings, so a user running dorian governance install without --json against a non-git target gets no indication that anything is off, even though the underlying claude_code.apply computed a warning specifically for that case.
🩹 Proposed fix
for skipped in result.skipped:
print(f" = {skipped} (exists; --force to overwrite)")
+ for warning in result.warnings:
+ print(f" ! {warning}")
print("next: merge settings.dorian-governance.example.json into .claude/settings.json")📝 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
| def _cmd_governance_install(args: argparse.Namespace) -> int: | |
| """Scaffold the Claude Code governance adapter (two host hooks + settings example + docs) | |
| into .claude/. Writes files only; never overwrites without --force; idempotent. Not a | |
| sandbox. The hooks own the exit-2 veto and wall-clock; Dorian core stays pure.""" | |
| target = Path(args.target).resolve() if args.target else _repo(args) | |
| if _missing_repo(target, "governance install"): | |
| return EXIT_USAGE | |
| try: | |
| plan = claude_code.build_plan( | |
| target, groups=governance.DEFAULT_GROUPS, manifest=governance.GOVERNANCE_MANIFEST | |
| ) | |
| result = claude_code.apply(plan, force=args.force, dry_run=args.dry_run) | |
| except (ValueError, OSError) as exc: | |
| print(f"dorian governance install: {exc}", file=sys.stderr) | |
| return EXIT_USAGE | |
| if args.json: | |
| print( | |
| json.dumps( | |
| { | |
| "repo": str(plan.repo_root), | |
| "is_git": plan.is_git, | |
| "dry_run": args.dry_run, | |
| "created": list(result.created), | |
| "overwritten": list(result.overwritten), | |
| "skipped": list(result.skipped), | |
| "warnings": list(result.warnings), | |
| }, | |
| indent=2, | |
| ) | |
| ) | |
| else: | |
| verb = "would write" if args.dry_run else "wrote" | |
| print( | |
| f"dorian governance install: {verb} {len(result.created)} file(s)" | |
| f" (skipped {len(result.skipped)}, overwrote {len(result.overwritten)})" | |
| f" under {plan.repo_root}" | |
| ) | |
| for created in result.created: | |
| print(f" + {created}") | |
| for skipped in result.skipped: | |
| print(f" = {skipped} (exists; --force to overwrite)") | |
| print("next: merge settings.dorian-governance.example.json into .claude/settings.json") | |
| return EXIT_OK | |
| if args.json: | |
| print( | |
| json.dumps( | |
| { | |
| "repo": str(plan.repo_root), | |
| "is_git": plan.is_git, | |
| "dry_run": args.dry_run, | |
| "created": list(result.created), | |
| "overwritten": list(result.overwritten), | |
| "skipped": list(result.skipped), | |
| "warnings": list(result.warnings), | |
| }, | |
| indent=2, | |
| ) | |
| ) | |
| else: | |
| verb = "would write" if args.dry_run else "wrote" | |
| print( | |
| f"dorian governance install: {verb} {len(result.created)} file(s)" | |
| f" (skipped {len(result.skipped)}, overwrote {len(result.overwritten)})" | |
| f" under {plan.repo_root}" | |
| ) | |
| for created in result.created: | |
| print(f" + {created}") | |
| for skipped in result.skipped: | |
| print(f" = {skipped} (exists; --force to overwrite)") | |
| for warning in result.warnings: | |
| print(f" ! {warning}") | |
| print("next: merge settings.dorian-governance.example.json into .claude/settings.json") | |
| return EXIT_OK |
🧰 Tools
🪛 ast-grep (0.44.0)
[info] 1227-1238: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"repo": str(plan.repo_root),
"is_git": plan.is_git,
"dry_run": args.dry_run,
"created": list(result.created),
"overwritten": list(result.overwritten),
"skipped": list(result.skipped),
"warnings": list(result.warnings),
},
indent=2,
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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/dorian/commands.py` around lines 1211 - 1253, The plain-text path in
_cmd_governance_install is dropping warnings returned by claude_code.apply, so
mirror result.warnings in the non-JSON output as well. After calling
claude_code.apply, keep the existing JSON behavior, and in the human-readable
branch print each warning from result.warnings before the final summary so users
see cases like the non-git repository warning. Use the existing
_cmd_governance_install, claude_code.apply, and result.warnings symbols to place
the change.
Comment on lines
+62
to
+69
| def load(repo: Path, goal_id: str) -> Goal: | ||
| """Read back a goal record, validating its schema version. | ||
|
|
||
| Raises ``ValueError`` for a malformed record (not JSON / not an object / missing required | ||
| field) or an unsupported ``schema_version``. | ||
| """ | ||
| raw = (repo / goal_path(goal_id)).read_text(encoding="utf-8") | ||
| try: |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
load() skips the path-containment check that save() enforces.
save() routes through sidecars.write_sidecar, which calls ensure_within to block ../ escapes (see test_path_escape_refused_via_save). load() builds the read path directly from an unsanitized goal_id and reads it with no equivalent check — a crafted goal_id containing ../ sequences can make load() read a file outside the repo (as long as the resolved path happens to end in the expected filename), breaking the containment invariant sidecars.py documents elsewhere in this codebase.
🔒 Proposed fix: reuse `sidecars.ensure_within` in `load()`
def load(repo: Path, goal_id: str) -> Goal:
"""Read back a goal record, validating its schema version.
Raises ``ValueError`` for a malformed record (not JSON / not an object / missing required
field) or an unsupported ``schema_version``.
"""
- raw = (repo / goal_path(goal_id)).read_text(encoding="utf-8")
+ target = sidecars.ensure_within(repo, Path(goal_path(goal_id)))
+ raw = target.read_text(encoding="utf-8")📝 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
| def load(repo: Path, goal_id: str) -> Goal: | |
| """Read back a goal record, validating its schema version. | |
| Raises ``ValueError`` for a malformed record (not JSON / not an object / missing required | |
| field) or an unsupported ``schema_version``. | |
| """ | |
| raw = (repo / goal_path(goal_id)).read_text(encoding="utf-8") | |
| try: | |
| def load(repo: Path, goal_id: str) -> Goal: | |
| """Read back a goal record, validating its schema version. | |
| Raises ``ValueError`` for a malformed record (not JSON / not an object / missing required | |
| field) or an unsupported ``schema_version``. | |
| """ | |
| target = sidecars.ensure_within(repo, Path(goal_path(goal_id))) | |
| raw = target.read_text(encoding="utf-8") | |
| try: |
🤖 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/dorian/goals.py` around lines 62 - 69, `load()` currently reads the goal
file without the same containment guard that `save()` gets through
`sidecars.write_sidecar`. Update `load(repo, goal_id)` to validate the resolved
path with `sidecars.ensure_within` before calling `read_text`, using the same
`repo` root and path derived from `goal_path(goal_id)` so `../` escapes are
rejected consistently. Keep the schema-loading logic unchanged; only add the
missing path-safety check around the file read.
Comment on lines
+10
to
+11
| - **`hooks/dorian_preflight_veto.py`** (`PreToolUse`) — before a mutating tool runs, reads that | ||
| packet and **exits `2` to block** the tool when the standing decision is `escalate`. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Clarify: escalate doesn't always block the tool.
dorian_preflight_veto.py only blocks a non-strict escalate decision when the broken claim is sensitive or the tool call lacks a file_path; under assist/cautious with a non-sensitive, path-scoped break, it fails open despite escalate. As written, this line reads as an unconditional guarantee, which overstates the actual protection in attended modes.
📝 Proposed wording fix
- **`hooks/dorian_preflight_veto.py`** (`PreToolUse`) — before a mutating tool runs, reads that
- packet and **exits `2` to block** the tool when the standing decision is `escalate`.
+ packet and **exits `2` to block** the tool when the standing decision is `escalate` **and**
+ the run is in a strict mode, the broken claim is sensitive, or the tool call has no specific
+ `file_path`; otherwise (attended, non-sensitive, path-scoped) it fails open like a missing packet.📝 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
| - **`hooks/dorian_preflight_veto.py`** (`PreToolUse`) — before a mutating tool runs, reads that | |
| packet and **exits `2` to block** the tool when the standing decision is `escalate`. | |
| - **`hooks/dorian_preflight_veto.py`** (`PreToolUse`) — before a mutating tool runs, reads that | |
| packet and **exits `2` to block** the tool when the standing decision is `escalate` **and** | |
| the run is in a strict mode, the broken claim is sensitive, or the tool call has no specific | |
| `file_path`; otherwise (attended, non-sensitive, path-scoped) it fails open like a missing packet. |
🤖 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/dorian/templates/claude_code/dorian-governance/README.md` around lines 10
- 11, The README wording around hooks/dorian_preflight_veto.py overstates the
behavior by implying every `escalate` decision always blocks the tool. Update
the description for the `PreToolUse` hook to reflect the actual gate in
`dorian_preflight_veto.py`: it only exits 2 in the strict block cases, while
attended modes can fail open for non-sensitive, path-scoped breaks. Keep the
wording aligned with the `PreToolUse` and `escalate` behavior so it describes
the conditional blocking accurately.
Comment on lines
+182
to
+228
| def external_imports(seeds: list[str], src: pathlib.Path = SRC) -> dict[str, list[str]]: | ||
| """Map each external import root reachable from ``seeds`` to the sorted list of source | ||
| files (relative to ``src``) that import it. Follows dorian.* edges transitively.""" | ||
| if not src.is_dir(): | ||
| # Fail loudly instead of passing vacuously if the core source tree is missing or | ||
| # the repo layout changed — a silent empty result would defeat the guard. | ||
| raise FileNotFoundError(f"deterministic-core source tree not found: {src}") | ||
| seen: set[str] = set() | ||
| stack: list[str] = list(seeds) | ||
| sites: dict[str, set[str]] = {} | ||
|
|
||
| while stack: | ||
| name = stack.pop() | ||
| if name in seen: | ||
| continue | ||
| seen.add(name) | ||
| path = _module_path(name, src) | ||
| if path is None: | ||
| # Not a file/package (e.g. a symbol re-exported from a package __init__, | ||
| # or a bare package dir without __init__). Expand a dir if one exists. | ||
| d = src / name.replace(".", "/") | ||
| if d.is_dir(): | ||
| for f in sorted(d.rglob("*.py")): | ||
| _absorb(f, src, sites, stack, seen) | ||
| continue | ||
| targets = [path] | ||
| if path.name == "__init__.py": | ||
| # a package seed: scan every module in the package, not just __init__ | ||
| targets += [p for p in sorted(path.parent.rglob("*.py")) if p != path] | ||
| for t in targets: | ||
| _absorb(t, src, sites, stack, seen) | ||
|
|
||
| return {root: sorted(files) for root, files in sorted(sites.items())} | ||
|
|
||
|
|
||
| def _absorb( | ||
| f: pathlib.Path, | ||
| src: pathlib.Path, | ||
| sites: dict[str, set[str]], | ||
| stack: list[str], | ||
| seen: set[str], | ||
| ) -> None: | ||
| internal, external = _scan(f, src) | ||
| rel = f.relative_to(src).as_posix() | ||
| for root in external: | ||
| sites.setdefault(root, set()).add(rel) | ||
| stack.extend(m for m in internal if m not in seen) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Walker can miss forbidden imports hidden in an intermediate package's __init__.py.
external_imports/_absorb only enqueue the exact dotted name captured by _scan (e.g. "checkers.sub.leaf" for from dorian.checkers.sub.leaf import X or import dorian.checkers.sub.leaf). _module_path then resolves that straight to checkers/sub/leaf.py, so it's scanned as a plain file (path.name != "__init__.py"), and neither checkers/__init__.py nor checkers/sub/__init__.py get added to sites/stack. But importing a deep submodule always implicitly executes every ancestor package's __init__.py first, so a forbidden import placed in one of those __init__.py files would never be detected by this scanner unless that ancestor package also happens to be reached independently (e.g. it's a seed itself, or imported directly elsewhere).
Today this is masked for checkers because "checkers" is itself a VERDICT_SEEDS entry (so its whole subtree gets rglob-scanned via the package-seed branch), but the gap is real for any nested package reached only through a deep dotted import — the walker's docstring guarantee ("the seed list is a floor... modules these reach... are guarded too") does not actually hold for that case.
♻️ Proposed fix — also enqueue ancestor package names
def _absorb(
f: pathlib.Path,
src: pathlib.Path,
sites: dict[str, set[str]],
stack: list[str],
seen: set[str],
) -> None:
internal, external = _scan(f, src)
rel = f.relative_to(src).as_posix()
for root in external:
sites.setdefault(root, set()).add(rel)
- stack.extend(m for m in internal if m not in seen)
+ for m in internal:
+ if m not in seen:
+ stack.append(m)
+ # importing a dotted submodule implicitly executes every ancestor
+ # package's __init__.py — make sure those get scanned too.
+ parts = m.split(".")
+ for i in range(1, len(parts)):
+ ancestor = ".".join(parts[:i])
+ if ancestor not in seen:
+ stack.append(ancestor)📝 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
| def external_imports(seeds: list[str], src: pathlib.Path = SRC) -> dict[str, list[str]]: | |
| """Map each external import root reachable from ``seeds`` to the sorted list of source | |
| files (relative to ``src``) that import it. Follows dorian.* edges transitively.""" | |
| if not src.is_dir(): | |
| # Fail loudly instead of passing vacuously if the core source tree is missing or | |
| # the repo layout changed — a silent empty result would defeat the guard. | |
| raise FileNotFoundError(f"deterministic-core source tree not found: {src}") | |
| seen: set[str] = set() | |
| stack: list[str] = list(seeds) | |
| sites: dict[str, set[str]] = {} | |
| while stack: | |
| name = stack.pop() | |
| if name in seen: | |
| continue | |
| seen.add(name) | |
| path = _module_path(name, src) | |
| if path is None: | |
| # Not a file/package (e.g. a symbol re-exported from a package __init__, | |
| # or a bare package dir without __init__). Expand a dir if one exists. | |
| d = src / name.replace(".", "/") | |
| if d.is_dir(): | |
| for f in sorted(d.rglob("*.py")): | |
| _absorb(f, src, sites, stack, seen) | |
| continue | |
| targets = [path] | |
| if path.name == "__init__.py": | |
| # a package seed: scan every module in the package, not just __init__ | |
| targets += [p for p in sorted(path.parent.rglob("*.py")) if p != path] | |
| for t in targets: | |
| _absorb(t, src, sites, stack, seen) | |
| return {root: sorted(files) for root, files in sorted(sites.items())} | |
| def _absorb( | |
| f: pathlib.Path, | |
| src: pathlib.Path, | |
| sites: dict[str, set[str]], | |
| stack: list[str], | |
| seen: set[str], | |
| ) -> None: | |
| internal, external = _scan(f, src) | |
| rel = f.relative_to(src).as_posix() | |
| for root in external: | |
| sites.setdefault(root, set()).add(rel) | |
| stack.extend(m for m in internal if m not in seen) | |
| def external_imports(seeds: list[str], src: pathlib.Path = SRC) -> dict[str, list[str]]: | |
| """Map each external import root reachable from ``seeds`` to the sorted list of source | |
| files (relative to ``src``) that import it. Follows dorian.* edges transitively.""" | |
| if not src.is_dir(): | |
| # Fail loudly instead of passing vacuously if the core source tree is missing or | |
| # the repo layout changed — a silent empty result would defeat the guard. | |
| raise FileNotFoundError(f"deterministic-core source tree not found: {src}") | |
| seen: set[str] = set() | |
| stack: list[str] = list(seeds) | |
| sites: dict[str, set[str]] = {} | |
| while stack: | |
| name = stack.pop() | |
| if name in seen: | |
| continue | |
| seen.add(name) | |
| path = _module_path(name, src) | |
| if path is None: | |
| # Not a file/package (e.g. a symbol re-exported from a package __init__, | |
| # or a bare package dir without __init__). Expand a dir if one exists. | |
| d = src / name.replace(".", "/") | |
| if d.is_dir(): | |
| for f in sorted(d.rglob("*.py")): | |
| _absorb(f, src, sites, stack, seen) | |
| continue | |
| targets = [path] | |
| if path.name == "__init__.py": | |
| # a package seed: scan every module in the package, not just __init__ | |
| targets += [p for p in sorted(path.parent.rglob("*.py")) if p != path] | |
| for t in targets: | |
| _absorb(t, src, sites, stack, seen) | |
| return {root: sorted(files) for root, files in sorted(sites.items())} | |
| def _absorb( | |
| f: pathlib.Path, | |
| src: pathlib.Path, | |
| sites: dict[str, set[str]], | |
| stack: list[str], | |
| seen: set[str], | |
| ) -> None: | |
| internal, external = _scan(f, src) | |
| rel = f.relative_to(src).as_posix() | |
| for root in external: | |
| sites.setdefault(root, set()).add(rel) | |
| for m in internal: | |
| if m not in seen: | |
| stack.append(m) | |
| # importing a dotted submodule implicitly executes every ancestor | |
| # package's __init__.py — make sure those get scanned too. | |
| parts = m.split(".") | |
| for i in range(1, len(parts)): | |
| ancestor = ".".join(parts[:i]) | |
| if ancestor not in seen: | |
| stack.append(ancestor) |
🤖 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/test_firewall_import_closure.py` around lines 182 - 228, The import
walker in external_imports/_absorb misses ancestor package __init__.py files
when a deep module name is discovered, so forbidden imports in intermediate
package initializers can be skipped. Update the traversal to enqueue and scan
every dotted parent package for each internal import root returned by _scan (for
example, walk from a module like checkers.sub.leaf back through checkers.sub and
checkers) before or alongside the existing _module_path resolution. Keep using
sites, stack, and seen so ancestor __init__.py files are absorbed transitively
and package-level imports are not missed.
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
2 participants
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
The deterministic spine of the "give your goal and go to sleep" direction: a human-authored goal
record, a path-derived coverage check, a host-mappable preflight gate, and one concrete Claude Code
governance adapter — while keeping Dorian's verification path model-free and zero-dependency.
What changed
tests/test_firewall_import_closure.py),src/dorian/sidecars.py),dorian goal add/show/check(src/dorian/goals.py),goals.coverage_diff→ {covered, uncovered}),dorian gate(stdin tool JSON → continue/repair/escalate packet; exits 0/4, 2 only on malformed),dorian governance install(SubagentStop hook + fail-closedPreToolUse veto + settings + docs),
docs/DORIAN_PANE.md,docs/GOVERNANCE_DATA_MODEL.md, SECURITY/TESTING/VALIDATION notes)and
.dorian/local/gitignore.Deterministic boundary
No model in the verdict path; zero core runtime deps; exit-code contract, warrant schema, checker
grammar, and fold policy all unchanged. Purely additive.
Adapter boundary
dorian gate/core emit only 0/4. The exit-2 tool-block lives only in the Claude Code PreToolUse hosthook; fails closed under a strict policy (
unattended/DORIAN_EFFORT=godmode), open whenattended (except escalate on a sensitive path).
.dorian/local/last-decision.jsonis ephemeral/gitignored — never an authoritative ledger. Not a sandbox.
Deferred / cut
Provenance → v1.5 or later · effort presets → cut from core · pane/TUI → deferred (Claude Design owns
final design) · generic provider abstraction → deferred until a second concrete adapter exists.
Validation
Focused governance/core (68), loop/gate/governance/goals (174), and docs/release tests pass under
PYTHONPATH=src; ruff check + format clean; the built wheel ships the governance templates. Locally,4 pre-existing subprocess end-to-end tests (
test_init,test_examples_claude_code,test_readme_example) fail only under the known editable-install/worktreepython -m dorianinstability — CI (
uv sync --all-extrason 3.11/3.12/3.13) is authoritative. Version stays 1.3.0(release bump happens on a separate branch).
Review notes
Rebased onto main after PR #25 (Loop Guard) merged, so this diff is the clean governance stack only.
Non-blocking hardening follow-ups tracked for before the release bump: installed-wheel governance
scaffold test; veto tests for identity-mismatch / empty-path / attended-malformed-stdin / godmode /
freshness-boundary; end-to-end
dorian goal checkintegration test.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation