Repository navigation
fix(core): govern forks as children of their source session - #775
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 43 minutes. View limit details
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72ceef6a66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const governanceParentID = session.parentID ?? session.fork?.sessionID | ||
| if (governanceParentID) { | ||
| const parent = yield* get(governanceParentID) |
There was a problem hiding this comment.
Preserve fork governance when the source session is deleted
After a user forks a session and deletes the source, the surviving fork can no longer switch models or agents, create children, or be forked again: these operations now require loading the deleted source. Session.remove in packages/core/src/session.ts recursively removes only sessions returned by list({ parentID: sessionID }), and SessionStore.list filters on parent_id, so top-level-projected forks survive deletion with a dangling fork.sessionID. The new governance walks then fail with NotFoundError or Parent session not found, even when no model restrictions were configured. Coordinate source deletion with the new governance relationship so surviving forks retain usable, enforceable governance rather than depending on a row that can be removed.
Useful? React with 👍 / 👎.
|
The following comment was made by an LLM, it may be inaccurate: |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 72 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
Fixes #505
Forking a child session laundered it out of ancestor governance: the projector inserts a fork with
parent_id: null(keeping onlyfork_session_idand a copy of the source's ownsubagent_policy), and every governance walk followedparentIDonly — so a fork escaped every policy above its source, its ownswitchAgent/switchModelwere unchecked (top-level gate), and its descendants were governed only by the copied policy. Specspecs/2026-10-25-subsession-model-governance.md§6.2 already decided the opposite: "Session.fork creates a child whose parent is the source session."Shape (governance-link, projection untouched)
The fork stays projected top-level (
parent_idstill null, listing/projection unchanged); governance now treats the fork source as the fork's governance parent:packages/core/src/session/model-governance.ts— theassertAllowedancestor walk followsparentID ?? fork.sessionID, so ancestor session policies above the source bind the fork, its switches (via the creation/fork checks that call it), and its descendants' creation.packages/core/src/session/session.ts—switchAgent/switchModelgate on the governance parent (parentID ?? fork.sessionID) instead ofparentIDalone, so a fork's own switches are checked against its source's chain; genuinely top-level sessions remain unchecked.packages/core/src/session/subagent-depth.ts— the depth walk follows the same link, somax_depthcaps above the source bind the fork's subtree (a fork hop counts as one level, as a child would).Tests
New regression test in
packages/core/test/session-governance.test.ts: a grandchild is forked under a grandparent policy oftest/haikuonly — the fork (projected top-level,parentIDundefined) cannotswitchModeltotest/opus, cannot launch children ontest/opus, and can launch children ontest/haiku. Existing fork governance tests (fork-of-policy-denied, permitted fork copies policy) and the session-governance suite run in CI.Verification is CI-only per fleet policy; the unit suites on this PR are the evidence.
Queue task 2725.
Need help on this PR? Tag
@codesmith-botwith what you need.