Make Edgebase an automatic work-contract runtime - #3
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces several new features to Edgebase, including copy-focused installer prompts, active Work Contract state persistence, strict and warning pre-edit enforcement modes, additional Git freshness hooks, status reporting, session-end Patch Passports, Codex skill surfaces, and a team mode configuration. The review feedback highlights several high-quality improvement opportunities: enhancing configuration portability by avoiding hardcoded absolute paths and interpreter executables in generated team-mode scripts, ensuring backward compatibility with Python versions older than 3.11 by using timezone.utc instead of UTC, eliminating duplicate helper functions in runtime.py by importing them from goal.py, and gracefully handling potential setup errors during Codex skill installations with try-except blocks.
| def write_required_hook(repo_root: Path) -> Path: | ||
| path = repo_root / ".claude" / "hooks" / "edgebase-required.sh" | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| command = shlex.join([sys.executable, "-m", "edgebase", "status", "--root", str(repo_root), "--json"]) | ||
| path.write_text( | ||
| "#!/bin/sh\n" | ||
| "# Edgebase required-mode hook. Generated by `edgebase team init required`.\n" | ||
| f"if ! {command} >/dev/null 2>&1; then\n" | ||
| " echo '{\"permissionDecision\":\"deny\",\"permissionDecisionReason\":\"Edgebase is required for AI-assisted edits in this repo. Run edgebase install-prompt --agent all.\"}'\n" | ||
| " exit 0\n" | ||
| "fi\n" | ||
| "echo '{}'\n", | ||
| encoding="utf-8", | ||
| ) | ||
| path.chmod(0o755) | ||
| return path |
There was a problem hiding this comment.
Hardcoding sys.executable and the absolute repo_root path in the generated shell script .claude/hooks/edgebase-required.sh makes the hook non-portable. When committed to a shared repository, it will fail for other team members who have different Python installation paths or clone the repository to a different directory. Using python3 and omitting the absolute path is much more portable.
| def write_required_hook(repo_root: Path) -> Path: | |
| path = repo_root / ".claude" / "hooks" / "edgebase-required.sh" | |
| path.parent.mkdir(parents=True, exist_ok=True) | |
| command = shlex.join([sys.executable, "-m", "edgebase", "status", "--root", str(repo_root), "--json"]) | |
| path.write_text( | |
| "#!/bin/sh\n" | |
| "# Edgebase required-mode hook. Generated by `edgebase team init required`.\n" | |
| f"if ! {command} >/dev/null 2>&1; then\n" | |
| " echo '{\"permissionDecision\":\"deny\",\"permissionDecisionReason\":\"Edgebase is required for AI-assisted edits in this repo. Run edgebase install-prompt --agent all.\"}'\n" | |
| " exit 0\n" | |
| "fi\n" | |
| "echo '{}'\n", | |
| encoding="utf-8", | |
| ) | |
| path.chmod(0o755) | |
| return path | |
| def write_required_hook(repo_root: Path) -> Path: | |
| path = repo_root / ".claude" / "hooks" / "edgebase-required.sh" | |
| path.parent.mkdir(parents=True, exist_ok=True) | |
| path.write_text( | |
| "#!/bin/sh\n" | |
| "# Edgebase required-mode hook. Generated by `edgebase team init required`.\n" | |
| "if ! python3 -m edgebase status --json >/dev/null 2>&1; then\n" | |
| " echo '{\"permissionDecision\":\"deny\",\"permissionDecisionReason\":\"Edgebase is required for AI-assisted edits in this repo. Run edgebase install-prompt --agent all.\"}'\n" | |
| " exit 0\n" | |
| "fi\n" | |
| "echo '{}'\n", | |
| encoding="utf-8", | |
| ) | |
| path.chmod(0o755) | |
| return path |
| entry = { | ||
| "matcher": "Write|Edit|MultiEdit", | ||
| "hooks": [{"type": "command", "command": shlex.quote(str(hook_path)), "timeout": 10}], | ||
| } |
There was a problem hiding this comment.
Hardcoding the absolute hook_path in .claude/settings.json makes the configuration non-portable. Since this file is committed to the repository for team mode, it will break for other team members. Using a relative path is fully portable and robust.
relative_path = hook_path.relative_to(repo_root)
entry = {
"matcher": "Write|Edit|MultiEdit",
"hooks": [{"type": "command", "command": "./" + shlex.quote(relative_path.as_posix()), "timeout": 10}],
}| import json | ||
| import os | ||
| import time | ||
| from datetime import UTC, datetime |
There was a problem hiding this comment.
Importing UTC directly from datetime was introduced in Python 3.11. If the project is run on Python 3.10 or older (which is still very common in many production and development environments), this will raise an ImportError. Using timezone.utc is fully backward-compatible with all Python 3 versions.
| from datetime import UTC, datetime | |
| from datetime import datetime, timezone |
| def iso_timestamp(ts: float) -> str: | ||
| return datetime.fromtimestamp(ts, tz=UTC).isoformat(timespec="seconds").replace("+00:00", "Z") |
There was a problem hiding this comment.
Use timezone.utc instead of UTC for backward compatibility with Python versions older than 3.11.
| def iso_timestamp(ts: float) -> str: | |
| return datetime.fromtimestamp(ts, tz=UTC).isoformat(timespec="seconds").replace("+00:00", "Z") | |
| def iso_timestamp(ts: float) -> str: | |
| return datetime.fromtimestamp(ts, tz=timezone.utc).isoformat(timespec="seconds").replace("+00:00", "Z") |
| def safe_timestamp() -> str: | ||
| return datetime.fromtimestamp(time.time(), tz=UTC).strftime("%Y%m%dT%H%M%SZ") |
There was a problem hiding this comment.
Use timezone.utc instead of UTC for backward compatibility with Python versions older than 3.11.
| def safe_timestamp() -> str: | |
| return datetime.fromtimestamp(time.time(), tz=UTC).strftime("%Y%m%dT%H%M%SZ") | |
| def safe_timestamp() -> str: | |
| return datetime.fromtimestamp(time.time(), tz=timezone.utc).strftime("%Y%m%dT%H%M%SZ") |
| from .context import stale_files | ||
| from .git import changed_files as git_changed_files | ||
| from .git import current_commit, find_repo_root | ||
| from .goal import GoalCapsule, WorkContract, build_goal_capsule, build_patch_passport |
There was a problem hiding this comment.
| def unrecorded_checks(required: list[str], tests: list[str]) -> list[str]: | ||
| recorded = {test_command(test) for test in tests} | ||
| return [check for check in required if check not in recorded] | ||
|
|
||
|
|
||
| def test_command(test: str) -> str: | ||
| if ":" not in test: | ||
| return test.strip() | ||
| command, _status = test.rsplit(":", 1) | ||
| return command.strip() |
There was a problem hiding this comment.
The functions unrecorded_checks and test_command are duplicated exactly from src/edgebase/goal.py. Redundant code duplication reduces maintainability. Since src/edgebase/runtime.py already imports from .goal, we can import these functions directly instead of redefining them.
| def unrecorded_checks(required: list[str], tests: list[str]) -> list[str]: | |
| recorded = {test_command(test) for test in tests} | |
| return [check for check in required if check not in recorded] | |
| def test_command(test: str) -> str: | |
| if ":" not in test: | |
| return test.strip() | |
| command, _status = test.rsplit(":", 1) | |
| return command.strip() | |
| # unrecorded_checks and test_command are imported from .goal |
| skill_path = install_codex_skill(repo_root / ".agents" / "skills" / "edgebase" / "SKILL.md", repo_root, command, goal=False) | ||
| results.append(SetupResult(skill_path, "updated", "Codex project skill /edgebase")) | ||
| goal_skill_path = install_codex_skill(repo_root / ".agents" / "skills" / "goal" / "SKILL.md", repo_root, command, goal=True) | ||
| results.append(SetupResult(goal_skill_path, "updated", "Codex project skill /goal")) |
There was a problem hiding this comment.
Unlike setup_claude, which wraps skill installation in a try-except block to gracefully handle and report errors (e.g., if a skill file already exists without the Edgebase marker), setup_codex does not catch RuntimeError raised by install_codex_skill. This can cause the entire setup process to crash with a traceback instead of skipping the step gracefully.
| skill_path = install_codex_skill(repo_root / ".agents" / "skills" / "edgebase" / "SKILL.md", repo_root, command, goal=False) | |
| results.append(SetupResult(skill_path, "updated", "Codex project skill /edgebase")) | |
| goal_skill_path = install_codex_skill(repo_root / ".agents" / "skills" / "goal" / "SKILL.md", repo_root, command, goal=True) | |
| results.append(SetupResult(goal_skill_path, "updated", "Codex project skill /goal")) | |
| try: | |
| skill_path = install_codex_skill(repo_root / ".agents" / "skills" / "edgebase" / "SKILL.md", repo_root, command, goal=False) | |
| except RuntimeError as exc: | |
| results.append( | |
| SetupResult(repo_root / ".agents" / "skills" / "edgebase" / "SKILL.md", "skipped", str(exc)) | |
| ) | |
| else: | |
| results.append(SetupResult(skill_path, "updated", "Codex project skill /edgebase")) | |
| try: | |
| goal_skill_path = install_codex_skill(repo_root / ".agents" / "skills" / "goal" / "SKILL.md", repo_root, command, goal=True) | |
| except RuntimeError as exc: | |
| results.append( | |
| SetupResult(repo_root / ".agents" / "skills" / "goal" / "SKILL.md", "skipped", str(exc)) | |
| ) | |
| else: | |
| results.append(SetupResult(goal_skill_path, "updated", "Codex project skill /goal")) |
| skill_path = install_codex_skill(Path.home() / ".codex" / "skills" / "edgebase" / "SKILL.md", repo_root, command, goal=False) | ||
| results.append(SetupResult(skill_path, "updated", "Codex global skill /edgebase")) | ||
| goal_skill_path = install_codex_skill(Path.home() / ".codex" / "skills" / "goal" / "SKILL.md", repo_root, command, goal=True) | ||
| results.append(SetupResult(goal_skill_path, "updated", "Codex global skill /goal")) |
There was a problem hiding this comment.
Wrap global Codex skill installations in try-except blocks to handle potential RuntimeError gracefully, matching the robust behavior implemented for Claude.
| skill_path = install_codex_skill(Path.home() / ".codex" / "skills" / "edgebase" / "SKILL.md", repo_root, command, goal=False) | |
| results.append(SetupResult(skill_path, "updated", "Codex global skill /edgebase")) | |
| goal_skill_path = install_codex_skill(Path.home() / ".codex" / "skills" / "goal" / "SKILL.md", repo_root, command, goal=True) | |
| results.append(SetupResult(goal_skill_path, "updated", "Codex global skill /goal")) | |
| try: | |
| skill_path = install_codex_skill(Path.home() / ".codex" / "skills" / "edgebase" / "SKILL.md", repo_root, command, goal=False) | |
| except RuntimeError as exc: | |
| results.append( | |
| SetupResult(Path.home() / ".codex" / "skills" / "edgebase" / "SKILL.md", "skipped", str(exc)) | |
| ) | |
| else: | |
| results.append(SetupResult(skill_path, "updated", "Codex global skill /edgebase")) | |
| try: | |
| goal_skill_path = install_codex_skill(Path.home() / ".codex" / "skills" / "goal" / "SKILL.md", repo_root, command, goal=True) | |
| except RuntimeError as exc: | |
| results.append( | |
| SetupResult(Path.home() / ".codex" / "skills" / "goal" / "SKILL.md", "skipped", str(exc)) | |
| ) | |
| else: | |
| results.append(SetupResult(goal_skill_path, "updated", "Codex global skill /goal")) |
5d0d002 to
d8b80eb
Compare
Add host-aware install prompts, active Work Contract state, runtime status and finish gates, warn/strict pre-edit enforcement, broader freshness hooks, Codex skill surfaces, team guidance, and docs so agents can follow the Goal Capsule loop automatically after setup. Constraint: Preserve dirty /root/edgebase work by implementing in the clean feature worktree based on origin/main. Rejected: Rebase the older PR commit directly | main already contained overlapping preflight, radius, hook, and graph work that needed integration instead of replay. Confidence: high Scope-risk: broad Directive: Keep setup host-capability driven and keep default enforcement warn-only unless users opt into --strict. Tested: python3 -m unittest -v; python3 -m compileall -q src tests; git diff --check; edgebase install-prompt --agent all; edgebase status --json; edgebase finish "demo" --test "python3 -m unittest -v: pass" Not-tested: Live Claude Code/Codex hook execution inside their native hosts; verified with simulated hook payloads and generated config tests. Co-authored-by: OmX <omx@oh-my-codex.dev>
d8b80eb to
be9aa8c
Compare
Summary
Tests
Not tested