Repository navigation
Address review findings from #40: ceilings, task-token ownership, reload consistency - #44
Merged
Merged
Conversation
A lease stops being usable at the earlier of expiry and revocation, but pruneLeases counted retention from the later one. A year-long lease revoked early stayed in the file until a month after its original expiry. Addresses review feedback on #40.
Truncated was set only after a normal exit, so a run that overflowed the output limit and then timed out or was cancelled returned Truncated=false and its partial output read as complete. processResult now sets it for every exit path. Addresses review feedback on #40.
CheckInbound and CheckOutbound took a config snapshot for the peer, hop and cwd checks, then checkProfile read the engine again. A concurrent Reload could split one decision across two configs. checkProfile now takes the caller's snapshot. Addresses review feedback on #40.
repair_peers read and rewrote config.DefaultPath() while the reload that follows read g.ConfigPath. A daemon started with --config could repair one file and report another, untouched one as active. Diagnosis, rewrite and reload now all use g.configPath(), as do doctor, init_status, the generated config path and validate_config's default. The stale comment claiming the daemon has no reload path is replaced. Addresses review feedback on #40.
repair_peers reloads the policy engine the daemon shares with the mesh RPC server, but the server kept its own startup Config pointer. peerForIdentity and validateLeaseRequest went on using the startup peers and profiles while the repair reported active_immediately=true. The server now reads peers, profiles, runners and policy through liveConfig(), which follows Engine.Config(). Node and tailnet settings stay on the startup config, since ports, bind address and data dir are fixed for the life of the process. Lease issuance and peer smoke each take one snapshot for the whole request. Addresses review feedback on #40.
validateProfileCeilings and the request-time backstop compared only the risk rank, so an equal-risk mapping passed in either direction even when the two profiles grant disjoint powers (readonly-agent vs query-shell, repo-dev vs system-change). profiledefs.CeilingExcess now requires the ceiling's writes, shell, network, git_push and risk not to exceed the requested profile's, and every ceiling capability to be held by the requested profile, listed or implied by an axis it already has (edit_repo by writes, execute_* by shell=allow, and so on). That keeps the documented system-change -> repo-dev cap legal. Unrecognized writes/shell values fail closed. Load-time validation and checkProfile share the one helper via ProfileConfig.Authority. Outbound denied_profiles now also applies to the ceiling a request is remapped to. Addresses review feedback on #40.
Task-token reads and cancels were authorized by origin-session equality. Every host-token delegation that omits origin_session_id shares gateway-default, which task-token contexts inherit, so one task's runner could read or cancel children created by unrelated tasks in that session. peer_tasks gains owner_task_id (migration 6), set to the worker task a token was minted for when that token delegates. Status, result, transcript, artifact, events, cancel and list_tasks now require owner equality; rows with no owner (host-token delegations, pre-migration rows) stay host-only. The owner is set once and survives later upserts. Two related gaps closed in the same change: a follow-up now passes the same ownership check before resuming another task's runner session, and the owner is mixed into the deterministic task id so identical requests from two tasks in one session no longer collapse onto one task. Addresses review feedback on #40.
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a1ba73f28
ℹ️ 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".
…rungs The ceiling subset check rejected capping an agent profile such as repo-dev or system-change to query, because no agent profile lists the probe capabilities. A config using such a cap would then fail to load. Unrestricted shell already covers those read-only inspection commands, so treat them as implied by shell=allow. A shell=deny profile such as readonly-agent still cannot cap to them. Addresses review feedback on #40.
Addresses review feedback on #40.
Reading git_push from the live config at completion meant a reload that flipped a profile to git_push=true let an already-running task push work it was accepted without push authority for. runTask now receives the config snapshot the task was accepted under, and pushes only when that snapshot and the current one both allow it: a reload can revoke push, not grant it. Addresses review feedback on #44.
acquireRunner read the live limit after the reload change, and a different limit replaced the semaphore with an empty channel. Running tasks and waiters kept the old channel while new tasks acquired the new one, so more tasks could start than either limit allows. The limit now comes from the startup config and is fixed for the life of the process, as it was before. Addresses review feedback on #44.
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edbb68000a
ℹ️ 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".
The runner registry is built once from the startup config, so an agent runner keeps the write authority its profile had then. Reading edit_repo from the live config let a reload that dropped it skip the dirty check and isolated worktree while the runner could still write, so the agent would edit the original checkout directly. Git isolation now applies when either the startup or the live profile grants edit_repo. Addresses review feedback on #44.
Since 1917be6 the policy engine and mesh server read the reloaded config, but the runner registry is built once at startup. A reload that changed a profile left policy accepting tasks under the new definition while the runner executed them with the startup one, and repair_peers picked up any unrelated profile edits sitting on disk. Config.Reloadable now takes only [peers.*] and [policy] (minus profile_ceilings) from a reloaded file; profiles, runners, ceilings and node, tailnet, store, audit and gateway settings keep their startup values and are reported when they differ. Engine.Reload applies the same merge, so no caller can change profiles through it. repair_peers and profile_configure report requires_daemon_restart and restart_required_for when held-back sections changed; profile_configure no longer claims a new profile is active before restart. With profiles fixed for the life of the process, the mesh server reads profiles and runners from its startup config again. That replaces the per-task git-push snapshot (0485104) and the startup/live claim union for git isolation (151c11a), which were only needed while profiles could change underneath a running runner. Runner concurrency stays on the startup config (edbb680). Addresses review feedback on #44.
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
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
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.
Follow-up to #40. Addresses its seven open review findings, one commit per finding, each with a regression test.
peer_tasks.owner_task_id(migration 6) records the worker task whose token delegated each task. Status/result/transcript/artifact/events/cancel/list_tasks require owner equality instead of session equality, since every host-token delegation withoutorigin_session_idsharesgateway-default. Also: follow-ups now pass the same check, and the owner is mixed into the deterministic task id so identical requests from two tasks in one session don't collapse onto one task. Ownership is per task, not per follow-up chain (documented in bccfc31): a follow-up's token cannot reach children its predecessor delegated.profile.CeilingExcessrequires the ceiling not to exceed the requested profile on writes, shell, network, git_push or risk, and every ceiling capability to be held by the requested profile (listed, or implied by an axis it already has, e.g.edit_repobywrites>=repo_only). The documentedsystem-change -> repo-devcap stays legal;readonly-agent <-> query-shellandrepo-dev -> system-changeare rejected. c76fa65 lets ashell = "allow"profile cap down toquery/query-shell(its shell already covers those read-only commands);readonly-agent -> querystays rejected. Shared by load-time validation and the request-time check. Outbounddenied_profilesnow also applies to the ceiling.processResultsetsTruncatedfor every exit path.liveConfig(), which followsEngine.Config(), sopeerForIdentityand lease validation see a repair. Node/tailnet settings (ports, bind, data dir) stay on the startup config.g.configPath(), as do doctor, init_status, validate_config's default and the generated config path.checkProfiletakes the caller's snapshot instead of re-reading the engine.Docs updated:
docs/configuration.md(ceiling rules) anddocs/security.md(task-token scoping).Local:
make test(fmt, modules, supply-chain, workflow lint, dup, vuln, unit, build) andgo test -raceon the touched packages pass.Reload scope (a218b66): a reload (
repair_peers,profile_configure) now applies only[peers.*]and[policy](notprofile_ceilings). Profiles, runners, ceilings and node settings stay on the startup config until restart, because the runner registry is built at startup; held-back sections are reported asrestart_required_forwithrequires_daemon_restart=true.profile_configuretherefore no longer hot-loads a new profile. This supersedes the earlier per-task git-push snapshot (0485104) and the git-isolation claim union (151c11a); runner concurrency stays on the startup config (edbb680).