Remove dead code and three tests that assert nothing
Finding 5 and the "remove" list of REVIEW-2026-07-30.md.
The tests were prose, not verification. TestContract_ChangesSinceAlwaysZero
documented a bug commit 74b19e0 had already fixed and could only t.Logf.
TestContract_TimeoutGoroutineNotCancelled was a bare t.Log, and is now
falsified by the preceding commit anyway.
TestContract_DestructiveCapabilityDisabledByDefault re-implemented
`risk != "destructive"` inside the test and asserted on its own local
variable — it would have passed if the handler were deleted. The real
derivation is now domain.EnabledForRisk and is tested against production code.
Also removed: the systemd provider (no systemctl in the distroless image and
nothing ever registered it), Server.ListenUnix, Registry.List,
DiscoveredTools, Capability.IsDestructive, the ExecutionDenied status,
EventCapabilityUnavailable, ResolveRequest/ResolveResult (superseded by
nexusclient), Engine.emitEvent, and eight unused Err values.
Three items on the review's list were kept, having turned out to be wrong:
ifString is still used by BuildCapabilities; EventExecutionDenied is asserted
on by a real test; and schema_migrations was NOT dropped. migrate() applies
migrations by slice index and writes user_version = index + 1, so removing an
element renumbers every later migration and any database past that point would
permanently skip one it had not yet applied. The live database is far behind
HEAD, so that is a data hazard rather than a cleanup. The CREATE TABLE is now
a no-op holding its slot, with a comment saying why the slot must stay.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uea55zaiWuEByEDC4UBSdd
This commit is contained in:
@@ -36,8 +36,5 @@ func (s *Server) Shutdown(ctx context.Context) error {
|
||||
if s.httpSrv != nil {
|
||||
return s.httpSrv.Shutdown(ctx)
|
||||
}
|
||||
if s.socket != "" {
|
||||
os.Remove(s.socket)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -78,15 +78,21 @@ const (
|
||||
ExecutionStarted ExecutionStatus = "started"
|
||||
ExecutionSucceeded ExecutionStatus = "succeeded"
|
||||
ExecutionFailed ExecutionStatus = "failed"
|
||||
ExecutionDenied ExecutionStatus = "denied"
|
||||
// ExecutionUnknown marks a wall-clock timeout: the executor may or may not
|
||||
// have completed the side effect. Never retried automatically.
|
||||
ExecutionUnknown ExecutionStatus = "unknown"
|
||||
)
|
||||
|
||||
type Execution struct {
|
||||
// Seq is the storage-assigned, monotonically increasing insertion
|
||||
// sequence. It is the pagination cursor for GET /api/v1/executions and
|
||||
// mirrors Event.Sequence on GET /api/v1/changes: pass the last Seq you saw
|
||||
// back as `since` and the server returns strictly greater rows. Only
|
||||
// populated by list reads; single-execution reads leave it zero.
|
||||
Seq int64 `json:"seq,omitempty"`
|
||||
ID string `json:"id"`
|
||||
CapabilityID string `json:"capability_id"`
|
||||
CapabilityVersion int64 `json:"capability_version,omitempty"`
|
||||
TargetEntityID string `json:"target_entity_id"`
|
||||
EntityVersion int64 `json:"entity_version,omitempty"`
|
||||
Arguments map[string]any `json:"arguments,omitempty"`
|
||||
@@ -108,7 +114,6 @@ type HexisEventType string
|
||||
|
||||
const (
|
||||
EventCapabilityRegistered HexisEventType = "hexis.capability.registered"
|
||||
EventCapabilityUnavailable HexisEventType = "hexis.capability.unavailable"
|
||||
EventExecutionStarted HexisEventType = "hexis.execution.started"
|
||||
EventExecutionSucceeded HexisEventType = "hexis.execution.succeeded"
|
||||
EventExecutionFailed HexisEventType = "hexis.execution.failed"
|
||||
|
||||
@@ -6,18 +6,10 @@ var (
|
||||
ErrCapabilityNotFound = errors.New("capability not found")
|
||||
ErrExecutionNotFound = errors.New("execution not found")
|
||||
ErrConflict = errors.New("version conflict")
|
||||
ErrAmbiguousTarget = errors.New("ambiguous target")
|
||||
ErrTargetNotFound = errors.New("target not found")
|
||||
ErrTargetTypeMismatch = errors.New("target type does not match capability")
|
||||
ErrCapabilityNotBound = errors.New("capability not registered for this entity")
|
||||
ErrEntityRetired = errors.New("entity is retired")
|
||||
ErrEntityMerged = errors.New("entity is merged")
|
||||
ErrValidation = errors.New("validation error")
|
||||
ErrInternal = errors.New("internal error")
|
||||
ErrIdempotencyReplay = errors.New("idempotent request already processed")
|
||||
|
||||
ErrCapabilityDisabled = errors.New("capability is disabled")
|
||||
ErrCapabilityVersionMismatch = errors.New("capability version mismatch")
|
||||
ErrConfirmationRequired = errors.New("confirmation required")
|
||||
ErrConfirmationNotFound = errors.New("confirmation not found")
|
||||
ErrConfirmationInvalid = errors.New("confirmation does not match request")
|
||||
|
||||
@@ -227,17 +227,6 @@ func TestContract_EventCorrelationFieldPopulated(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestContract_DestructiveCapabilityDisabledByDefault validates that
|
||||
// capabilities with risk="destructive" are created with enabled=false
|
||||
// per ECOSYSTEM-SPEC.md §4.3.
|
||||
func TestContract_DestructiveCapabilityDisabledByDefault(t *testing.T) {
|
||||
risk := "destructive"
|
||||
enabled := risk != "destructive" // line 117 in handler.go
|
||||
if enabled {
|
||||
t.Error("expected destructive capability to be disabled by default")
|
||||
}
|
||||
}
|
||||
|
||||
// TestContract_ExecutionStatusDeniedNotEmitted documents that the
|
||||
// execution.denied event type is defined but never emitted.
|
||||
func TestContract_ExecutionDeniedNotEmitted(t *testing.T) {
|
||||
@@ -261,39 +250,3 @@ func TestContract_ExecutionDeniedNotEmitted(t *testing.T) {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestContract_ChangesSinceReturnsAll validates the current behavior:
|
||||
// handleChanges always queries from sequence 0 regardless of the `since`
|
||||
// parameter (known bug).
|
||||
func TestContract_ChangesSinceAlwaysZero(t *testing.T) {
|
||||
eng, store := newContractTestEngine(t)
|
||||
cap := mustCreateCapability(t, store, nil)
|
||||
|
||||
eng.Execute(&domain.ExecuteRequest{
|
||||
CapabilityID: cap.ID,
|
||||
TargetEntityID: "ent_x",
|
||||
})
|
||||
|
||||
// EventsAfter with since=0 returns all events
|
||||
all, err := store.EventsAfter(0, 1000)
|
||||
if err != nil {
|
||||
t.Fatalf("events after 0: %v", err)
|
||||
}
|
||||
|
||||
// EventsAfter with since=5 should return fewer events
|
||||
after5, err := store.EventsAfter(5, 1000)
|
||||
if err != nil {
|
||||
t.Fatalf("events after 5: %v", err)
|
||||
}
|
||||
|
||||
if len(all) <= len(after5) {
|
||||
t.Logf("BUG CONFIRMED: EventsAfter(0) returned %d events, EventsAfter(5) returned %d events (should be fewer)",
|
||||
len(all), len(after5))
|
||||
}
|
||||
}
|
||||
|
||||
// TestContract_TimeoutGoroutineNotCancelled documents that when an execution
|
||||
// times out, the provider goroutine continues running in the background.
|
||||
func TestContract_TimeoutGoroutineNotCancelled(t *testing.T) {
|
||||
t.Log("CONFIRMED: runWithTimeout does not cancel the provider goroutine on timeout")
|
||||
}
|
||||
|
||||
@@ -386,27 +386,3 @@ func (e *Engine) emitEventCorrelated(evtType domain.HexisEventType, entityID, co
|
||||
Payload: payload,
|
||||
})
|
||||
}
|
||||
|
||||
type ResolveRequest struct {
|
||||
Query string `json:"query"`
|
||||
Types []string `json:"types,omitempty"`
|
||||
}
|
||||
|
||||
type ResolveResult struct {
|
||||
Status string `json:"status"`
|
||||
Candidates []map[string]any `json:"candidates,omitempty"`
|
||||
EntityID string `json:"entity_id,omitempty"`
|
||||
}
|
||||
|
||||
var _ json.Marshaler = (*ResolveResult)(nil)
|
||||
|
||||
func (r *ResolveResult) MarshalJSON() ([]byte, error) {
|
||||
m := map[string]any{"status": r.Status}
|
||||
if r.Candidates != nil {
|
||||
m["candidates"] = r.Candidates
|
||||
}
|
||||
if r.EntityID != "" {
|
||||
m["entity_id"] = r.EntityID
|
||||
}
|
||||
return json.Marshal(m)
|
||||
}
|
||||
|
||||
@@ -43,13 +43,3 @@ func (r *Registry) Get(name string) (Provider, error) {
|
||||
}
|
||||
return p, nil
|
||||
}
|
||||
|
||||
func (r *Registry) List() []string {
|
||||
r.mu.RLock()
|
||||
defer r.mu.RUnlock()
|
||||
var names []string
|
||||
for n := range r.providers {
|
||||
names = append(names, n)
|
||||
}
|
||||
return names
|
||||
}
|
||||
|
||||
@@ -1,68 +0,0 @@
|
||||
package provider
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"os/exec"
|
||||
"strings"
|
||||
|
||||
"github.com/kami/hexis/internal/domain"
|
||||
)
|
||||
|
||||
type SystemdProvider struct{}
|
||||
|
||||
func NewSystemdProvider() *SystemdProvider {
|
||||
return &SystemdProvider{}
|
||||
}
|
||||
|
||||
func (p *SystemdProvider) Name() string { return "systemd" }
|
||||
|
||||
func (p *SystemdProvider) Execute(capability *domain.Capability, req *domain.ExecuteRequest) (map[string]any, error) {
|
||||
unitName := req.TargetEntityID
|
||||
if unitName == "" {
|
||||
unitName = capability.TargetEntityID
|
||||
}
|
||||
|
||||
if !isValidUnitName(unitName) {
|
||||
return nil, fmt.Errorf("invalid unit name: %q", unitName)
|
||||
}
|
||||
|
||||
var cmd *exec.Cmd
|
||||
switch capability.Operation {
|
||||
case "restart":
|
||||
cmd = exec.Command("systemctl", "restart", unitName)
|
||||
case "start":
|
||||
cmd = exec.Command("systemctl", "start", unitName)
|
||||
case "stop":
|
||||
cmd = exec.Command("systemctl", "stop", unitName)
|
||||
case "status":
|
||||
cmd = exec.Command("systemctl", "status", unitName)
|
||||
case "reload":
|
||||
cmd = exec.Command("systemctl", "reload", unitName)
|
||||
case "enable":
|
||||
cmd = exec.Command("systemctl", "enable", unitName)
|
||||
case "disable":
|
||||
cmd = exec.Command("systemctl", "disable", unitName)
|
||||
default:
|
||||
return nil, fmt.Errorf("unsupported systemd operation: %s", capability.Operation)
|
||||
}
|
||||
|
||||
output, err := cmd.CombinedOutput()
|
||||
if err != nil {
|
||||
return map[string]any{
|
||||
"stdout": string(output),
|
||||
"exit_code": cmd.ProcessState.ExitCode(),
|
||||
}, fmt.Errorf("systemctl %s failed: %w\n%s", capability.Operation, err, string(output))
|
||||
}
|
||||
|
||||
return map[string]any{
|
||||
"stdout": string(output),
|
||||
"exit_code": 0,
|
||||
}, nil
|
||||
}
|
||||
|
||||
func isValidUnitName(name string) bool {
|
||||
if name == "" || strings.Contains(name, "..") || strings.Contains(name, "/") || strings.Contains(name, ";") || strings.Contains(name, "|") || strings.Contains(name, "$") || strings.Contains(name, "`") || strings.Contains(name, "'") || strings.Contains(name, `"`) || strings.Contains(name, "\\") {
|
||||
return false
|
||||
}
|
||||
return true
|
||||
}
|
||||
@@ -120,10 +120,11 @@ var migrations = []string{
|
||||
payload TEXT NOT NULL DEFAULT '{}'
|
||||
)`,
|
||||
`CREATE INDEX IF NOT EXISTS idx_hevents_sequence ON hexis_events(sequence)`,
|
||||
`CREATE TABLE IF NOT EXISTS schema_migrations (
|
||||
version INTEGER PRIMARY KEY,
|
||||
applied_at TEXT NOT NULL
|
||||
)`,
|
||||
// Retired: schema_migrations was never read — PRAGMA user_version is the
|
||||
// real mechanism. Migrations are applied by slice index, so this slot must
|
||||
// keep its position or every later migration would be renumbered and
|
||||
// skipped on already-migrated databases. Left as a no-op instead.
|
||||
`SELECT 1`,
|
||||
`ALTER TABLE capabilities ADD COLUMN requires_confirmation INTEGER NOT NULL DEFAULT 0`,
|
||||
`ALTER TABLE capabilities ADD COLUMN enabled INTEGER NOT NULL DEFAULT 1`,
|
||||
`ALTER TABLE capabilities ADD COLUMN timeout_seconds INTEGER NOT NULL DEFAULT 30`,
|
||||
|
||||
Reference in New Issue
Block a user