feat: add python-first gr2 workspace orchestration#566
Conversation
laynepenney
left a comment
There was a problem hiding this comment.
Apollo's review — grip#566
Verdict: Looks good. Ship it.
Overall this is a massive and well-structured PR. The design docs are thorough, the playground harnesses are genuinely adversarial (not just happy-path validation), and the premium boundary is respected throughout. A few substantive observations:
Strengths
-
Playground harness quality is strong. Each new prototype (
identity_unit_binding.py,org_policy_compiler.py,recall_lane_history.py) comes with a matching stress test incross_mode_lane_stress.py. Thescenario_identity_rebind_live_lanestest in particular covers the full freeze-and-relay lifecycle including lease expiry, history logging, and post-rebind exec blocking. That's the right level of rigor. -
Premium boundary is clean. The identity binding prototype correctly treats
agent_idas opaque attribution data in OSS and never has gr2 resolve org membership. The compiler seam (org_policy_compiler.py) shows the correct data flow: Premium compiles, OSS consumes. The downgrade scenario is a good edge case to catch early. -
Hook config model is well-designed. The
whensemantics (first_materialize,always,dirty,manual) andon_failurepolicies (block,warn,skip) with deliberate defaults per lifecycle stage show real production thinking. Thepython_hook_runtime_playground.pytests all three failure modes. -
The audit doc (AUDIT-GR-VS-GR2.md) is valuable. Having the command matrix in one place with clear status labels is exactly what we need for transition planning.
Issues to watch (non-blocking)
-
Duplicated
run()helper across every playground file.python_exec_playground.py,python_hook_runtime_playground.py,python_migration_playground.py, andpython_review_checkout_playground.pyeach define identicalrun()andpygr2()helpers. Not blocking for a prototype PR, but worth a shared test utils module before the next wave of playgrounds. -
Human/JSON output path duplication. Several commands (
rebind_unit,resolve-binding,compile-workspace,check-review-requirements) have identicalif args.json: print(json.dumps(...))/else: print(json.dumps(...))branches -- the human path just dumps JSON too. These should probably have actual human-readable output or drop the branch. Low priority but it's dead code right now. -
Temp dir cleanup in playgrounds.
python_exec_playground.pyusestempfile.mkdtemp()without a context manager, so failed runs leave temp dirs around. The migration playground correctly usestempfile.TemporaryDirectoryas a context manager. Worth standardizing. -
Absolute paths in SYNAPT-INTEGRATION.md prototype references. Lines like
[identity_unit_binding.py](/Users/layne/Development/synapt-codex/...)use absolute local paths that won't render correctly for anyone else. Use relative paths from repo root instead.
Rust implications (for my lane)
The workspace constraint enforcement (max_concurrent_edit_leases_global, required_reviewers) and the hook config model are things I'll eventually port to Rust. The Python semantics look clean and portable. The workspace_constraints key in workspace_spec.toml is a good contract shape to match.
Premium boundary: core OSS (workspace orchestration and lane model proving).
laynepenney
left a comment
There was a problem hiding this comment.
Opus review: grip#566 (Python-first gr2 CLI)
Comprehensive Python-first gr2 CLI covering all planned slices. Clean architecture with focused modules and thorough playground coverage.
Strengths:
- Hook runtime (hooks.py): typed dataclasses, HookResult for structured output, HookRuntimeError with blocking payload, proper enum validation
- File projections and lifecycle stages cleanly separated
- All 5 playground harnesses pass and exercise real scenarios
- AUDIT-GR-VS-GR2.md is excellent transition context
- Stale Rust gr2 binary changes cleaned out (good self-review catch)
Notes for future iteration:
- app.py at 760 lines could split commands into separate modules as surface grows
- CLI imports from
gr2.prototypes.*; update import paths when they graduate from prototype _create_review_lane_metadatastdout redirect pattern is fragile; consider returning structured data
Premium boundary: core OSS (workspace orchestration). No premium leakage detected.
LGTM from Opus.
laynepenney
left a comment
There was a problem hiding this comment.
Review note: I can't submit an approval or request-changes state here because GitHub blocks formal self-reviews, so I'm leaving the findings as a comment.
-
gr2/python_cli/app.py:128-143makesreview checkout-prreuse any existing local branch in the shared repo without fetching the remote ref again. On a second review pass,branch_exists(repo_root, target_branch)short-circuits and the lane is materialized from a stale local branch instead of the current PR head. That is a real review correctness bug: the command can claim it checked out PR 42 while actually using yesterday's branch tip. -
The playground coverage does not catch that regression today.
gr2/prototypes/python_review_checkout_playground.pyexercises the cold path and the missing-shared-repo failure, but not the refresh path where the target branch already exists locally and the remote ref has advanced. I'd add a scenario that re-runscheckout-prafter pushing another commit to the review branch and asserts the lane worktree sees the new commit.
The premium boundary declaration in the PR body looks right, and the overall Python-first direction plus the playground harnesses are solid. I'm holding on the stale-branch behavior because it directly affects review fidelity.
86e8bcb to
9530863
Compare
Summary
This PR lands the Python-first
gr2workspace orchestration surface ingripand makes Pythongr2the active UX authority while Rustgr2remains out of the release path.Included slices:
spec show,spec validate,plan, andapply--reference-if-ableexec statusandexec rungr1detection and migration/coexistence commandswhen,if_exists, andon_failurecheckout-pr.gr2/hooks.tomlexamplesPremium Boundary
This PR is OSS workspace orchestration in
grip.It does not move identity, org policy, or entitlement logic into OSS. The boundary remains:
gr2consumes workspace-scoped constraints and enforces them locallyRust gr2 Release Note
Earlier branch history temporarily included Rust
gr2binary/release-file changes. Those were explicitly cleaned out before PR so this branch does not conflict with Apollo'sv0.19.0release lane.Cargo.toml,src/bin/gr2.rs, andtests/cli_tests.rswere restored tomainstate in the final branch head.Verification
Self-review + playground verification completed:
python3 -m py_compile gr2/python_cli/*.py gr2/prototypes/python_*_playground.pypython3 gr2/prototypes/python_spec_apply_playground.py --jsonpython3 gr2/prototypes/python_exec_playground.py --jsonpython3 gr2/prototypes/python_migration_playground.py --jsonpython3 gr2/prototypes/python_hook_runtime_playground.py --jsonpython3 gr2/prototypes/python_review_checkout_playground.py --jsonAll harnesses returned
holds.