Merge pull request #282 from morluto/agent/make-invalid-states-unrepresentable

refactor: make invalid states unrepresentable

Author: morluto

docs/architecture.md

@@ -346,6 +346,12 @@ remain write operations. Catalog changes require realistic multi-call agent
 evaluations, including held-out queries, tool-call count, errors, latency, and
 context size; scripted schema checks alone do not establish good tool choice.
 
+Recovery-plan actions are sealed variants. The concrete input type derives the
+action discriminator, the output schema advertises the variants with `oneOf`,
+and decoding rejects unknown, mismatched, or multiple payloads. Application
+code therefore receives a typed action rather than validating a discriminator
+against a nullable argument bag.
+
 The canonical source-audit workflow is a machine-readable contract exposed by
 `workflow.get_source_audit_contract`:
 
@@ -430,6 +436,42 @@ rate capacity. Only replayable reads are retried. Backoff honors GitHub rate
 headers, is bounded, observes context cancellation, and redacts URL userinfo
 before retry metadata is persisted.
 
+Repository identities are parsed at input, provider, and persistence
+boundaries into a private, comparable `domain.RepoRef`. Interior code cannot
+construct an owner without a repository name, carry whitespace, or bypass the
+owner and repository grammar; it receives a parsed identity and uses explicit
+accessors. The zero value is reserved for optional scope and must be tested
+with `IsValid`. JSON decoding reparses the identity, and larger domain records
+hold it in named fields so its codec cannot be promoted over the enclosing
+record.
+
+Pull-request merge knowledge is likewise a parsed `domain.MergeStatus`, not
+independent `merged`, `merged_known`, and `merged_at` fields. Constructors make
+unknown, observed-unmerged, and observed-merged outcomes explicit. SQLite and
+GitHub adapter reads reject contradictions such as an unknown outcome marked
+merged or an unmerged outcome with a merge timestamp; interior code cannot
+create those combinations. The relational schema keeps scalar columns for
+querying, but rows are reparsed before they enter application models.
+
+Durable run and job lifecycles are read through private state values that bind
+statuses to their timestamps. Running work cannot be completed, queued jobs
+cannot already be started, terminal work requires a completion time, and only
+cancelled or cancellation-requested jobs carry a cancellation time. Terminal
+run transitions are conditional on the stored running state, while job
+transitions update the status and required timestamps atomically. Corrupt or
+unknown persisted combinations fail at the corpus boundary.
+
+JSON inputs that express alternatives remain wire-compatible discriminated
+objects, but they are parsed before any durable job is submitted. Thread sync
+becomes either repository discovery with repository-only filters or an exact
+thread set. Portfolio sync becomes either authored discovery or an explicit
+pull-request set. Actor identity becomes either a canonical login or a node ID,
+and coverage becomes either a repository target or an exact typed thread.
+Workers receive these private variants rather than the original field bags, so
+mode-specific fields cannot be silently ignored and identity strings are
+canonicalized before duplicate detection. The normalized wire form, not the
+caller's mutable slices or pointers, is what the durable job records.
+
 ## Acquisition and workspaces
 
 Acquisition and workspace packages invoke `git` directly with prompts, hooks,
@@ -453,6 +495,30 @@ host paths. The application resolves each ID and verifies that it belongs to
 the selected investigation before persisting executable state. The explicit
 CLI remains a local-user interface and may accept a directly supplied path.
 
+Observation definitions cross command and MCP boundaries as untrusted specs.
+The application parses a complete base-and-candidate contract before it enters
+the evidence service. Parsed observations have a private representation: their
+source and artifact-path relationship is established once, default occurrence
+is normalized, and regular expressions are compiled once for execution.
+Persistence decodes through the same parser, so malformed stored contracts do
+not re-enter the trusted model. Execution therefore consumes parsed values and
+does not repeat structural validation or regular-expression compilation.
+
+Durable workflow JSON is parsed again on read. Concern, investigation,
+hypothesis, opportunity, validation, and evidence discriminators cannot enter
+application logic as unchecked strings; legacy empty states are canonicalized
+only where their historical meaning is unambiguous. Telemetry metrics decode as
+either an available value or an unavailable reason and reject payloads claiming
+both. External validation receipts atomically store their synthetic definition
+and run, while external evidence manifests atomically store the complete claim
+set. A failed import therefore leaves no orphan definition or partial manifest.
+
+Bulk local-metadata and collection inputs are fully parsed before writable
+corpus access. Collection references are stored in canonical repository,
+thread, or UUID form, and malformed later members cannot follow earlier writes.
+Thread projections similarly parse kind, lifecycle state, repository key,
+and number before a transaction begins and again when SQLite rows are read.
+
 ## Search and analysis
 
 Search uses the local SQLite corpus and FTS5 indexes; agents query bounded
@@ -468,6 +534,14 @@ Snapshots created before manifests were introduced report
 `indexed_coverage_unknown`; their zero skip counts are never presented as proof
 of complete coverage.
 
+Repository coverage uses collection membership to represent presence: a
+returned `domain.FacetCoverage` is necessarily present, while a missing facet is
+absent from the collection. Its private constructor binds the facet name,
+observation time, completeness, and non-negative count. Immutable code-index
+artifacts similarly use their digest-bound manifest as the sole in-memory
+authority; duplicated query columns are checked against that manifest while
+decoding and discarded rather than exposed as a second source of truth.
+
 Title, labels, body, and hydrated evidence are materialized into one search
 document per thread and ranked by one BM25 invocation. Ranks from the legacy
 thread and facet indexes are never compared; the facet index is used only to

internal/acquire/acquire.go

@@ -168,8 +168,8 @@ func NewManager(root string, runner runner) (*Manager, error) {
 // clean checkout at the resolved default branch. The returned Acquisition
 // records remote URL, default branch, commit SHA, and acquisition time.
 func (m *Manager) Acquire(ctx context.Context, owner, repo, remote string) (*Acquisition, error) {
-	ref := domain.RepoRef{Owner: owner, Repo: repo}
-	if err := ref.Validate(); err != nil {
+	_, err := domain.NewRepoRef(owner, repo)
+	if err != nil {
 		return nil, fmt.Errorf("%w: %w", ErrInvalidRepo, err)
 	}
 	if err := validateRemote(remote); err != nil {

internal/app/acquisition.go

@@ -17,13 +17,13 @@ import (
 // resolved remote URL/default branch/commit SHA/acquired time, and indexes the
 // clean checkout into the corpus. It does not execute repository code.
 func (s *Service) Acquire(ctx context.Context, repo contracts.RepoRef, remote string) (result *contracts.AcquisitionResult, returnErr error) {
-	ref := domain.RepoRef{Owner: repo.Owner, Repo: repo.Repo}
-	if err := ref.Validate(); err != nil {
+	ref, err := domain.NewRepoRef(repo.Owner, repo.Repo)
+	if err != nil {
 		return nil, err
 	}
 	remote = strings.TrimSpace(remote)
 	if remote == "" {
-		remote = fmt.Sprintf("https://github.com/%s/%s.git", ref.Owner, ref.Repo)
+		remote = fmt.Sprintf("https://github.com/%s/%s.git", ref.Owner(), ref.Repo())
 	}
 
 	cacheRoot, err := s.paths.AcquisitionCacheDir()
@@ -36,7 +36,7 @@ func (s *Service) Acquire(ctx context.Context, repo contracts.RepoRef, remote st
 		return nil, fmt.Errorf("create acquisition manager: %w", err)
 	}
 
-	acq, err := mgr.Acquire(ctx, ref.Owner, ref.Repo, remote)
+	acq, err := mgr.Acquire(ctx, ref.Owner(), ref.Repo(), remote)
 	if err != nil {
 		return nil, fmt.Errorf("acquire %s: %w", ref, err)
 	}

internal/app/acquisition_test.go

@@ -83,7 +83,7 @@ func TestAcquireSuccess(t *testing.T) {
 	if err != nil {
 		t.Fatalf("open corpus: %v", err)
 	}
-	snap, err := c.LatestCodeSnapshot(ctx, domain.RepoRef{Owner: "testowner", Repo: "testrepo"})
+	snap, err := c.LatestCodeSnapshot(ctx, domain.MustRepoRef("testowner", "testrepo"))
 	if err != nil {
 		t.Fatalf("latest snapshot: %v", err)
 	}
@@ -125,7 +125,7 @@ func TestAcquireRepeatFetch(t *testing.T) {
 	if err != nil {
 		t.Fatalf("open corpus: %v", err)
 	}
-	snap, err := c.LatestCodeSnapshot(ctx, domain.RepoRef{Owner: "owner", Repo: "repo"})
+	snap, err := c.LatestCodeSnapshot(ctx, domain.MustRepoRef("owner", "repo"))
 	if err != nil {
 		t.Fatalf("latest snapshot: %v", err)
 	}
@@ -163,7 +163,7 @@ func TestAcquireUnchangedCommitReusesCurrentSnapshot(t *testing.T) {
 		},
 	}
 	if _, _, err := svc.corpus.StoreCodeSnapshot(
-		ctx, domain.RepoRef{Owner: ref.Owner, Repo: ref.Repo}, replacement,
+		ctx, domain.MustRepoRef(ref.Owner, ref.Repo), replacement,
 	); err != nil {
 		t.Fatalf("replace snapshot: %v", err)
 	}
@@ -176,7 +176,7 @@ func TestAcquireUnchangedCommitReusesCurrentSnapshot(t *testing.T) {
 		t.Fatalf("second acquire = %+v", second)
 	}
 	matches, err := svc.corpus.SearchCode(
-		ctx, "sentinel", domain.RepoRef{Owner: ref.Owner, Repo: ref.Repo}, 10,
+		ctx, "sentinel", domain.MustRepoRef(ref.Owner, ref.Repo), 10,
 	)
 	if err != nil {
 		t.Fatalf("search preserved snapshot: %v", err)

internal/app/actor_selector.go

@@ -0,0 +1,87 @@
+package app
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"strings"
+
+	"github.com/morluto/gitcontribute/internal/corpus"
+	"github.com/morluto/gitcontribute/internal/mcpcontract"
+)
+
+// parsedActorSelector is the executable form of ActorSelector's JSON union.
+// Implementations contain exactly one identity, so downstream acquisition does
+// not need to keep re-checking the discriminator and mutually exclusive fields.
+type parsedActorSelector interface {
+	key() string
+	resolveLogin(context.Context, *corpus.Corpus) (string, error)
+}
+
+type actorLogin string
+
+func (login actorLogin) key() string { return strings.ToLower(string(login)) }
+func (login actorLogin) resolveLogin(context.Context, *corpus.Corpus) (string, error) {
+	return string(login), nil
+}
+
+type actorNodeID string
+
+func (nodeID actorNodeID) key() string { return string(nodeID) }
+func (nodeID actorNodeID) resolveLogin(ctx context.Context, c *corpus.Corpus) (string, error) {
+	actor, err := c.GetActor(ctx, string(nodeID))
+	if err != nil {
+		return "", err
+	}
+	if actor == nil || actor.Login == "" {
+		return "", fmt.Errorf("node ID %q is not stored; search or sync by login first", nodeID)
+	}
+	return actor.Login, nil
+}
+
+func parseActorSelectors(inputs []mcpcontract.ActorSelector) ([]parsedActorSelector, []mcpcontract.ActorSelector, error) {
+	selectors := make([]parsedActorSelector, len(inputs))
+	normalized := make([]mcpcontract.ActorSelector, len(inputs))
+	seen := make(map[string]struct{}, len(inputs))
+	for i, input := range inputs {
+		switch input.Type {
+		case "login":
+			login := strings.TrimSpace(input.Login)
+			if login == "" || input.NodeID != "" {
+				return nil, nil, errors.New("login selectors require login and forbid node_id")
+			}
+			selectors[i] = actorLogin(login)
+			normalized[i] = mcpcontract.ActorSelector{Type: "login", Login: login}
+		case "node_id":
+			nodeID := strings.TrimSpace(input.NodeID)
+			if nodeID == "" || input.Login != "" {
+				return nil, nil, errors.New("node_id selectors require node_id and forbid login")
+			}
+			selectors[i] = actorNodeID(nodeID)
+			normalized[i] = mcpcontract.ActorSelector{Type: "node_id", NodeID: nodeID}
+		default:
+			return nil, nil, errors.New("actor selector type must be login or node_id")
+		}
+		key := selectors[i].key()
+		if _, ok := seen[key]; ok {
+			return nil, nil, fmt.Errorf("duplicate actor selector %q", key)
+		}
+		seen[key] = struct{}{}
+	}
+	return selectors, normalized, nil
+}
+
+func storedActorForSelector(ctx context.Context, c *corpus.Corpus, selector parsedActorSelector) (*corpus.Actor, string, error) {
+	login, err := selector.resolveLogin(ctx, c)
+	if err != nil {
+		return nil, "", err
+	}
+	actor, err := c.GetActor(ctx, login)
+	if err != nil {
+		return nil, "", err
+	}
+	if actor == nil {
+		return nil, "", fmt.Errorf("actor %q has no stored identity; call github.sync_users first", login)
+	}
+	return actor, login, nil
+}

internal/app/app.go

@@ -219,7 +219,7 @@ func (s *Service) openCorpus(ctx context.Context) (*corpus.Corpus, error) {
 	if err != nil {
 		return nil, err
 	}
-	if inspection.Exists {
+	if inspection.Exists() {
 		switch inspection.State {
 		case corpus.SchemaMigrationRequired:
 			return nil, &corpus.MigrationRequiredError{Current: inspection.Current, Target: inspection.Target}
@@ -438,7 +438,7 @@ func (s *Service) Init(ctx context.Context) (*contracts.InitResult, error) {
 	if err != nil {
 		return nil, err
 	}
-	if inspection.Exists {
+	if inspection.Exists() {
 		switch inspection.State {
 		case corpus.SchemaMigrationRequired:
 			return nil, &corpus.MigrationRequiredError{Current: inspection.Current, Target: inspection.Target}
@@ -645,8 +645,8 @@ func corpusRepoFromGitHub(r github.Repository) corpus.Repository {
 
 // Dossier builds a deterministic, local-corpus-backed repository dossier.
 func (s *Service) Dossier(ctx context.Context, repo contracts.RepoRef) (*contracts.DossierResult, error) {
-	ref := domain.RepoRef{Owner: repo.Owner, Repo: repo.Repo}
-	if err := ref.Validate(); err != nil {
+	ref, err := domain.NewRepoRef(repo.Owner, repo.Repo)
+	if err != nil {
 		return nil, err
 	}
 	if _, err := s.openReadOnlyCorpus(ctx); err != nil {

internal/app/app_test.go

@@ -285,7 +285,7 @@ func TestContributionGuidanceDoesNotClaimUnfetchedSource(t *testing.T) {
 	if _, err := svc.RepositoryContextSync(ctx, contracts.RepoRef{Owner: "octocat", Repo: "test"}, 0); err != nil {
 		t.Fatal(err)
 	}
-	guidance, refs, err := (&corpusReader{s: svc}).ReadContributionGuidance(ctx, domain.RepoRef{Owner: "octocat", Repo: "test"})
+	guidance, refs, err := (&corpusReader{s: svc}).ReadContributionGuidance(ctx, domain.MustRepoRef("octocat", "test"))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -340,12 +340,12 @@ func TestMCPReaderLocalReads(t *testing.T) {
 
 	_, err = reader.Dossier(ctx, mcpcontract.RepoInput{Owner: "acme", Repo: "rocket"})
 	var dossierErr *mcpcontract.ToolError
-	if !errors.As(err, &dossierErr) || dossierErr.Code != "dossier_not_persisted" || dossierErr.Recovery == nil || len(dossierErr.Recovery.Then) != 1 || dossierErr.Recovery.Then[0].Type != "get_repositories" {
+	if !errors.As(err, &dossierErr) || dossierErr.Code != "dossier_not_persisted" || dossierErr.Recovery == nil || len(dossierErr.Recovery.Then) != 1 || dossierErr.Recovery.Then[0].Type() != "get_repositories" {
 		t.Fatalf("MCP dossier before build error = %+v", err)
 	}
 	_, err = reader.Dossier(ctx, mcpcontract.RepoInput{Owner: "acme", Repo: "missing"})
 	var repositoryErr *mcpcontract.ToolError
-	if !errors.As(err, &repositoryErr) || repositoryErr.Code != "repository_not_indexed" || repositoryErr.Recovery == nil || len(repositoryErr.Recovery.Then) != 1 || repositoryErr.Recovery.Then[0].Type != "sync_repository_context" {
+	if !errors.As(err, &repositoryErr) || repositoryErr.Code != "repository_not_indexed" || repositoryErr.Recovery == nil || len(repositoryErr.Recovery.Then) != 1 || repositoryErr.Recovery.Then[0].Type() != "sync_repository_context" {
 		t.Fatalf("MCP dossier for missing repository error = %+v", err)
 	}
 	if _, err := svc.BuildRepositoryDossier(ctx, contracts.RepoRef{Owner: "acme", Repo: "rocket"}); err != nil {
@@ -392,7 +392,7 @@ func TestSearchCodeUsesStoredSnapshotWithoutNetwork(t *testing.T) {
 	if _, err := svc.Init(ctx); err != nil {
 		t.Fatal(err)
 	}
-	_, _, err = svc.corpus.StoreCodeSnapshot(ctx, domain.RepoRef{Owner: "owner", Repo: "repo"}, codeindex.Snapshot{
+	_, _, err = svc.corpus.StoreCodeSnapshot(ctx, domain.MustRepoRef("owner", "repo"), codeindex.Snapshot{
 		RepoPath: "/repo", Commit: "abc", CreatedAt: time.Now(), TotalBytes: 20,
 		Documents: []codeindex.Document{{Path: "parser.go", Content: "func searchableParser() {}", Bytes: 25, LanguageHint: "go"}},
 	})

internal/app/clustering.go

@@ -18,7 +18,10 @@ import (
 // ListClusters reads the current stored duplicate-candidate projection. It does
 // not compute or write cluster state.
 func (s *Service) ListClusters(ctx context.Context, repo contracts.RepoRef, limit int) (*contracts.ClusterListResult, error) {
-	ref := domain.RepoRef{Owner: repo.Owner, Repo: repo.Repo}
+	ref, err := domain.NewRepoRef(repo.Owner, repo.Repo)
+	if err != nil {
+		return nil, err
+	}
 	if err := validateClusterList(ref, limit); err != nil {
 		return nil, err
 	}
@@ -36,8 +39,8 @@ func (s *Service) ListClusters(ctx context.Context, repo contracts.RepoRef, limi
 // RefreshClusters explicitly computes and persists the duplicate-candidate
 // projection for a repository.
 func (s *Service) RefreshClusters(ctx context.Context, repo contracts.RepoRef) (*contracts.ClusterRefreshResult, error) {
-	ref := domain.RepoRef{Owner: repo.Owner, Repo: repo.Repo}
-	if err := ref.Validate(); err != nil {
+	ref, err := domain.NewRepoRef(repo.Owner, repo.Repo)
+	if err != nil {
 		return nil, err
 	}
 	c, err := s.openCorpus(ctx)
@@ -97,8 +100,8 @@ func clusterRefreshToCLI(repo contracts.RepoRef, disposition string, identity cl
 }
 
 func validateClusterList(ref domain.RepoRef, limit int) error {
-	if err := ref.Validate(); err != nil {
-		return err
+	if !ref.IsValid() {
+		return errors.New("repository reference is not parsed")
 	}
 	if limit < 1 || limit > 1000 {
 		return errors.New("cluster limit must be between 1 and 1000")