Repository navigation
Overhaul policy/session/TUI + fix 15 review findings - #3
Merged
Merged
Conversation
A sequence of related changes to the policy engine, session handling, the operator skill, and the policy TUI. They were developed together and the per-file edits interleave across themes, so they land as one commit. Sessions - `run` now requires an explicit session (--session or AGENTSSH_SESSION); with neither it errors (exit 2, ErrNoSession). Removed the 30-min idle-window resume and the on-disk session pointer file, so logically distinct tasks can no longer merge by time. Added `agentssh session new`. Skills - Replaced the two task playbooks (restart-service, investigate-cpu) with a single `agentssh-usage` operating manual: trust boundary, one-session- per-task discipline, policy, bounded output, audit — best practices for using AgentSSH, not a server runbook. Policy engine - Rebuilt as a two-tier, priority-ordered, default-deny model. Evaluate: host tier (host:<name> + matched group overrides, pooled, priority desc, ties by file order) then global tier, else a hard-coded default-deny. Rules carry allow/deny + priority; action is required. Seed policy.yaml is a default-deny scaffold with no active rules. Rule groups + policy TUI - Added rule_groups: reusable authoring presets the engine never evaluates. Stamping copies a group's rules onto a host with a `group` provenance tag (snapshot, not a live link); removing a group filters host rules by that tag. Engine ignores rule_groups and Rule.Group. - Entry-level Policy tab: card grid (Global card + one per rule group) with drill-in rule CRUD. Host detail Policy pane is a borderless unified list (host + global in one list, SCOPE/PRIORITY/ACTION/COMMAND/GROUP columns, host tier first then priority desc, global rows read-only). Help bar - Slimmed every footer to a few essential keys; the ? overlay carries the full, grouped key list (FullHelp is now preserved, not flattened). Host rules ownership - Deleting a host cascades to remove its own host:<name> rules; group overrides and rule groups are left intact. - Renamed the "binding" terminology to "host rules": HostBinding -> HostRuleSet, HostBindingGroup -> HostRulesKey, LookupHostBinding -> LookupHostRules, SetHostBinding -> SetHostRules, RemoveHostBinding -> ClearHostRules, ErrHostUnbound -> ErrNoHostRules (HostOverride / host_overrides unchanged). Review fixes (from an adversarial pass) - removeSelectedHostGroup now removes the group from the selected row's own source override, not blindly from the host's binding (wrong-rule delete). - RemoveHostGroup rejects an empty group name (would have matched every ungrouped rule via Group == ""). go build ./... && go vet ./... && go test ./... all green; default-deny, host>global, snapshot provenance, and the host-delete cascade were exercised end-to-end. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses correctness bugs and cleanups found reviewing the policy/session/ TUI overhaul. All fixes carry regression tests; go build/test pass. Correctness: - audit: TruncateBroken now cuts by break index (new VerifyResult.BrokenIndex), not the stored seq value, so a tampered seq can no longer wipe valid records or report a false-success repair. - policy: reject removed v0.5.1 schema keys (defaults / policy / allow_rules) on load with a migration hint instead of silently dropping them and flipping allow-by-default to deny-all. - run: group/multi-target runs derive a unique session id per host, restoring one-session-per-host; stale comments corrected. - policy: empty action defaults to allow again (invalid actions still error), so one action-less rule no longer bricks all execution. - policy/store: RemoveHostRule/RemoveHostGroup work on orphaned host overrides whose host left inventory. - inventory rm: clear/validate/save policy before writing inventory removal, avoiding an orphaned override on policy failure. - policy: custom MarshalYAML preserves host_overrides file order, protecting equal-priority tie-breaks across a save round-trip. - tui: editing a named global rule preserves its Name so CLI lookups still work. - session/tui: deterministic secondary sort by id for same-second sessions and session groups. Cleanup: - idempotent group stamping; shared internal/fileutil.WriteFileAtomic for policy+inventory saves; saveValidatedPolicy helper; remove dead parseHostRuleInput; simplify withHost target reset. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Overview
This branch overhauls the policy model, sessions, skills, and policy TUI (commit
a1b92f3), then fixes 15 findings from a code review of that overhaul (commitb7de37c). All fixes carry regression tests;go build ./...andgo test ./...pass.Correctness fixes
TruncateBrokennow cuts by a newVerifyResult.BrokenIndex(slice position) instead of the stored seq value, so a tampered/displaced seq can no longer delete all valid records or report a false-success repair.defaults, top-level/per-overridepolicy/allow_rules) now fails fast with a migration hint rather than silently dropping them and flipping allow-by-default to deny-all.<base>@<host>), restoring one-session-per-host; stale comments corrected.actiondefaults toallowagain (invalid non-empty actions still error), so one malformed rule no longer bricks allrunexecution.RemoveHostRule/RemoveHostGroupwork on a host override whose host left inventory.MarshalYAMLpreserves file order, protecting equal-priority tie-breaks across a save round-trip.Name, so CLIpolicy rule update/rm <name>still resolves it.session.Summariesand TUIbuildGroupssort same-second rows deterministically by id.Cleanup
Idempotent group stamping · shared
internal/fileutil.WriteFileAtomic(policy + inventory saves) ·saveValidatedPolicyhelper replacing ~12 repeated validate+save pairs · removed deadparseHostRuleInput· simplifiedwithHosttarget reset.policy.yamlnow errors on load (with a migration hint) instead of silently degrading. This is intentional — under default-deny semantics, silent migration would lock users out.session_idformat to embed the host (<base>@<host>), which is visible in audit records andsession ls.Testing
go build ./...— cleango test ./...— all packages pass; new regression tests cover findings 1, 3, 4, 5, 6, 7, 8, 9, 10.🤖 Generated with Claude Code