diff --git a/config/example.toml b/config/example.toml index 083b40b..7661c51 100644 --- a/config/example.toml +++ b/config/example.toml @@ -53,3 +53,14 @@ trigger_labels = ["coven:fix", "coven:review"] # Labels that trigger this famil # audit_instruction = "Focus on correctness and security." # [review.repos."OpenCoven/coven-code"] # pull_request = false # Per-repo override + +# ── Hosted memory governance (issue #6) ───────────────────────────────────── +# Off by default. Enabling memory is a hosted decision coordinated with the +# coven-code side of the contract (docs/memory-contract.md). Fork and external +# actors can never write durable memory regardless of these settings. +# [memory] +# enabled = false +# approval_required = true # learned facts stay pending until a maintainer approves +# retention_days = 365 +# [memory.repos."acme/billing"] +# enabled = true # per-repo opt-in diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index 62b510b..3825254 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -16,6 +16,59 @@ pub struct Config { /// Durable adapter state (issue #2). Absent section = default path. #[serde(default)] pub storage: StorageConfig, + /// Hosted memory governance policy (issue #6). Absent section = memory off. + #[serde(default)] + pub memory: MemoryConfig, +} + +/// Hosted memory governance policy (issue #6). Off by default; opting in is a +/// hosted decision coordinated with the coven-code side of the contract (see +/// `docs/memory-contract.md`). +#[derive(Debug, Clone, Default, Deserialize, Serialize)] +pub struct MemoryConfig { + /// Master opt-in. `false` (or section absent) → the adapter emits no memory + /// policy and the runtime does no memory work. + #[serde(default)] + pub enabled: bool, + /// Written memory stays `pending` until a maintainer approves it. + #[serde(default = "default_true")] + pub approval_required: bool, + /// Optional retention horizon for durable memory. + pub retention_days: Option, + /// Per-repo overrides keyed "owner/name". + #[serde(default)] + pub repos: std::collections::HashMap, +} + +/// Per-repo override of the global [`MemoryConfig`]; unset fields inherit. +#[derive(Debug, Clone, Default, Deserialize, Serialize)] +pub struct RepoMemoryOverride { + pub enabled: Option, + pub approval_required: Option, +} + +impl MemoryConfig { + fn overrides(&self, repo: &str) -> Option<&RepoMemoryOverride> { + self.repos.get(repo) + } + + /// Whether memory is opted in for `repo` ("owner/name"). + pub fn enabled_for(&self, repo: &str) -> bool { + self.overrides(repo) + .and_then(|o| o.enabled) + .unwrap_or(self.enabled) + } + + /// Whether writes for `repo` require maintainer approval. + pub fn approval_required_for(&self, repo: &str) -> bool { + self.overrides(repo) + .and_then(|o| o.approval_required) + .unwrap_or(self.approval_required) + } +} + +fn default_true() -> bool { + true } /// Durable store location. See `docs/durable-task-store.md`. @@ -340,6 +393,22 @@ impl Config { } } + // ── Memory governance (issue #6) ──────────────────────────────── + // Memory is off by default; when an operator enables it anywhere, + // warn if writes are not approval-gated — that is the posture that + // lets untrusted content shape future reviews. + let memory_on = self.memory.enabled || self.memory.repos.values().any(|o| o.enabled == Some(true)); + if memory_on { + let gated = self.memory.approval_required + && self.memory.repos.values().all(|o| o.approval_required != Some(false)); + if !gated { + out.push(Diagnostic::warning( + "memory.approval_required", + "memory is enabled with approval_required = false — learned facts write without maintainer review.", + )); + } + } + out } } @@ -423,6 +492,9 @@ fn next_step_for(field: &str, _message: &str) -> &'static str { "storage.path" => { "Point storage.path at a writable SQLite file location on a persistent volume." } + "memory.approval_required" => { + "Keep memory.approval_required = true so learned facts need maintainer review, or accept the risk deliberately." + } _ => "Update this config field, then rerun coven-github doctor.", } } @@ -522,6 +594,7 @@ mod tests { familiars, review: ReviewConfig::default(), storage: StorageConfig::default(), + memory: MemoryConfig::default(), } } @@ -588,6 +661,62 @@ mod tests { ); } + #[test] + fn memory_policy_defaults_off_and_resolves_repo_overrides() { + let mut memory = MemoryConfig::default(); + assert!(!memory.enabled_for("acme/any"), "memory is off by default"); + + memory.enabled = true; + assert!(memory.enabled_for("acme/any")); + // approval_required serde-defaults to true, but Default::default() is + // false; set it explicitly to model the deserialized default. + memory.approval_required = true; + assert!(memory.approval_required_for("acme/any")); + + memory.repos.insert( + "acme/quiet".to_string(), + RepoMemoryOverride { + enabled: Some(false), + approval_required: None, + }, + ); + assert!(!memory.enabled_for("acme/quiet"), "override disables the repo"); + assert!(memory.enabled_for("acme/loud")); + } + + #[test] + fn doctor_warns_when_memory_enabled_without_approval() { + let dir = tmpdir(); + let pem = write_pem(&dir); + let bin = write_bin(&dir); + let mut cfg = config_with( + GitHubAppConfig { + app_id: 123, + private_key_path: pem, + webhook_secret: "a-long-random-webhook-secret".into(), + api_base_url: None, + }, + WorkerConfig { + concurrency: 4, + coven_code_bin: bin, + workspace_root: dir.clone(), + timeout_secs: 600, + max_retries: 2, + }, + vec![good_familiar()], + ); + cfg.memory.enabled = true; + cfg.memory.approval_required = false; + + let warned = cfg + .check() + .iter() + .any(|d| d.field == "memory.approval_required"); + assert!(warned, "diags: {:?}", cfg.check()); + // It is a warning, not an error — the operator may accept the risk. + assert!(errors(&cfg.check()).is_empty()); + } + #[test] fn review_policy_defaults_to_disabled() { let policy = ReviewConfig::default(); diff --git a/crates/github/src/lib.rs b/crates/github/src/lib.rs index 6afe424..826cbc1 100644 --- a/crates/github/src/lib.rs +++ b/crates/github/src/lib.rs @@ -260,6 +260,58 @@ pub struct SessionResult { pub pr_body: String, pub review: ReviewResult, pub exit_reason: Option, + /// Memory activity the runtime reports for hosted governance (issue #6). + /// Absent/`None` when the run did no memory work. The adapter re-validates + /// every proposed write against the invocation's memory policy before + /// anything is persisted — see `docs/memory-contract.md`. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub memory_used: Option, +} + +/// Runtime-reported memory activity for one session (issue #6). +#[derive(Debug, Clone, Deserialize, Serialize, Default, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +pub struct MemoryUsed { + pub enabled: bool, + /// Entries loaded and used — the basis for citing what shaped a review. + #[serde(default)] + pub read: Vec, + /// Candidate writes the runtime proposes (subject to adapter validation + /// and, when the policy requires it, maintainer approval). + #[serde(default)] + pub proposed: Vec, + /// Candidates the runtime itself declined, with a reason. + #[serde(default)] + pub rejected: Vec, +} + +/// A memory entry the runtime read, by fully-qualified id and namespace scope. +#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +pub struct MemoryEntryRef { + pub id: String, + pub scope: String, +} + +/// A memory write the runtime proposes. +#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +pub struct ProposedMemory { + pub key: String, + pub summary: String, + pub scope: String, + /// `pending` | `applied` | `auto`. + pub approval: String, +} + +/// A memory write the runtime declined on its own. +#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +pub struct RejectedMemory { + pub summary: String, + pub scope: String, + /// `pii` | `secret` | `out_of_scope` | `low_confidence`. + pub reason: String, } #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] @@ -369,3 +421,44 @@ pub enum ExitReason { GitConflict, InfraError, } + +#[cfg(test)] +mod memory_result_tests { + use super::*; + + const BASE: &str = r#"{"contract_version":"2","status":"success","branch":null,"commits":[],"files_changed":[],"summary":"s","pr_body":"","review":{"mode":"none","evidence_status":"not_applicable","reviewed_files":[],"supporting_files":[],"findings":[],"tests_run":[],"no_findings_reason":null,"limitations":[]},"exit_reason":null"#; + + #[test] + fn result_without_memory_used_parses_as_none_and_omits_on_reserialize() { + let json = format!("{BASE}}}"); + let result: SessionResult = serde_json::from_str(&json).expect("parses"); + assert!(result.memory_used.is_none()); + // Backward compatible: a memory-free result never grows the field. + let out = serde_json::to_string(&result).expect("serializes"); + assert!(!out.contains("memory_used"), "unexpected field: {out}"); + } + + #[test] + fn result_with_memory_used_round_trips() { + let json = format!( + r#"{BASE},"memory_used":{{"enabled":true,"read":[{{"id":"repo/o/r/conventions/x","scope":"repo"}}],"proposed":[{{"key":"repo/o/r/conventions/y","summary":"y","scope":"repo","approval":"pending"}}],"rejected":[]}}}}"# + ); + let result: SessionResult = serde_json::from_str(&json).expect("parses"); + let mem = result.memory_used.as_ref().expect("memory_used present"); + assert!(mem.enabled); + assert_eq!(mem.read[0].id, "repo/o/r/conventions/x"); + assert_eq!(mem.proposed[0].approval, "pending"); + + let value = serde_json::to_value(&result).expect("serializes"); + assert_eq!(value["memory_used"]["proposed"][0]["scope"], "repo"); + } + + #[test] + fn unknown_memory_field_is_rejected() { + let json = format!( + r#"{BASE},"memory_used":{{"enabled":true,"bogus":1}}}}"# + ); + let err = serde_json::from_str::(&json).expect_err("deny_unknown_fields"); + assert!(err.to_string().contains("bogus"), "unexpected error: {err}"); + } +} diff --git a/crates/webhook/src/routes.rs b/crates/webhook/src/routes.rs index 126d63a..1aeacab 100644 --- a/crates/webhook/src/routes.rs +++ b/crates/webhook/src/routes.rs @@ -589,6 +589,7 @@ mod tests { }], review, storage: coven_github_config::StorageConfig::default(), + memory: coven_github_config::MemoryConfig::default(), }), store: Store::open_in_memory().expect("in-memory store"), notify: std::sync::Arc::new(tokio::sync::Notify::new()), diff --git a/crates/worker/src/brief.rs b/crates/worker/src/brief.rs index d06d473..b69e29f 100644 --- a/crates/worker/src/brief.rs +++ b/crates/worker/src/brief.rs @@ -42,6 +42,10 @@ pub struct SessionBrief { pub review_context: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub audit_instruction: Option, + /// Hosted memory governance policy (issue #6). Present only when the + /// installation has opted memory in for this repo; absent → memory off. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub memory_policy: Option, } #[derive(Debug, Serialize, Deserialize)] @@ -105,6 +109,7 @@ pub fn build( workspace: &Path, default_branch: &str, review: Option<&ReviewContext>, + memory_policy: Option, ) -> SessionBrief { let trigger = match &task.kind { TaskKind::FixIssue { .. } => "issue_assigned", @@ -205,6 +210,7 @@ pub fn build( }) }), audit_instruction: review.and_then(|r| r.audit_instruction.clone()), + memory_policy, } } @@ -258,10 +264,31 @@ mod tests { #[test] fn brief_uses_resolved_default_branch_not_hardcoded_main() { - let brief = build(&task(), &familiar(), Path::new("/tmp/ws"), "develop", None); + let brief = build(&task(), &familiar(), Path::new("/tmp/ws"), "develop", None, None); assert_eq!(brief.repo.default_branch, "develop"); } + #[test] + fn brief_omits_memory_policy_by_default_and_stamps_it_when_present() { + // No policy → the field is absent (memory off, deny-by-default). + let plain = build(&task(), &familiar(), Path::new("/tmp/ws"), "main", None, None); + assert!(plain.memory_policy.is_none()); + let json = serde_json::to_string(&plain).unwrap(); + assert!(!json.contains("memory_policy"), "unexpected field: {json}"); + + // A policy → stamped verbatim into the brief. + let policy = serde_json::json!({ "enabled": true, "repo": "acme/billing" }); + let stamped = build( + &task(), + &familiar(), + Path::new("/tmp/ws"), + "main", + None, + Some(policy.clone()), + ); + assert_eq!(stamped.memory_policy.as_ref(), Some(&policy)); + } + #[test] fn review_task_briefs_as_contract_v2_review_comment_with_context() { let review = ReviewContext { @@ -274,6 +301,7 @@ mod tests { Path::new("/tmp/ws"), "main", Some(&review), + None, ); // Contract v2 locks trigger/task enums — the review lane must ride on @@ -311,14 +339,14 @@ mod tests { #[test] fn non_review_tasks_carry_no_review_context() { - let brief = build(&task(), &familiar(), Path::new("/tmp/ws"), "main", None); + let brief = build(&task(), &familiar(), Path::new("/tmp/ws"), "main", None, None); assert!(brief.review_context.is_none()); assert!(brief.audit_instruction.is_none()); } #[test] fn brief_serialization_never_contains_token_or_auth_fields() { - let brief = build(&task(), &familiar(), Path::new("/tmp/ws"), "main", None); + let brief = build(&task(), &familiar(), Path::new("/tmp/ws"), "main", None, None); let value = serde_json::to_value(&brief).expect("brief should serialize"); let json = serde_json::to_string(&brief).expect("brief should serialize"); diff --git a/crates/worker/src/lib.rs b/crates/worker/src/lib.rs index 9882a51..68fef0b 100644 --- a/crates/worker/src/lib.rs +++ b/crates/worker/src/lib.rs @@ -14,6 +14,7 @@ use coven_github_config::{Config, FamiliarConfig}; use coven_github_store::{Store, Terminal, TerminalState}; pub mod brief; +pub mod memory; pub mod redact; pub mod status_comment; @@ -627,12 +628,34 @@ async fn run_and_publish( } _ => None, }; + // Compute the memory governance policy (issue #6). Deny-by-default: unless + // the installation opts memory in for this repo, this is None and no policy + // is stamped, so the runtime does no memory work. Trust: a commander here + // already passed the #13 write-access gate (below-write is declined + // earlier), so a command carries maintainer trust; auto-triggered work gets + // the safe collaborator default. Fork-PR hardening is a follow-up. + let repo_full = format!("{}/{}", task.repo_owner, task.repo_name); + let memory_policy = memory::compute_policy(memory::PolicyInputs { + enabled: config.memory.enabled_for(&repo_full), + installation_id: task.installation_id, + repo: &repo_full, + trust: if task.commander.is_some() { + memory::TrustScope::Maintainer + } else { + memory::TrustScope::Collaborator + }, + approval_required: config.memory.approval_required_for(&repo_full), + retention_days: config.memory.retention_days, + }); let brief = brief::build( task, familiar, workspace, &targets.default_branch, review.as_ref(), + memory_policy + .as_ref() + .map(|p| serde_json::to_value(p).expect("memory policy serializes")), ); let brief_path = workspace.join("session-brief.json"); let result_path = workspace.join("result.json"); @@ -703,6 +726,22 @@ async fn run_and_publish( // PR body, Check Run output). redact::sanitize_result(&mut result, &[orchestration, &agent_git]); + // Re-validate the runtime's reported memory activity against the policy we + // granted (issue #6). The runtime's self-report is not trusted on its own: + // any read or write outside scope — including a fork PR that tried to write + // durable memory — is refused here before it can be persisted. + if let (Some(policy), Some(used)) = (&memory_policy, &result.memory_used) { + let rejections = + memory::validate_memory_used(policy, used, |text| redact::redact(text, &[]) != text); + if !rejections.is_empty() { + warn!( + task_id = %task.id, + rejected = rejections.len(), + "refused out-of-policy memory activity — not persisting those entries" + ); + } + } + // Stale-ref gate (issue #8): review findings are only valid for the head // SHA that was actually reviewed. Re-fetch the PR before publishing; if // the head moved mid-session, surface the task as superseded rather than @@ -1606,6 +1645,7 @@ mod disposition_tests { pr_body: "body".to_string(), review: ReviewResult::none(), exit_reason: None, + memory_used: None, } } @@ -1682,6 +1722,7 @@ mod disposition_tests { familiars: vec![], review: coven_github_config::ReviewConfig::default(), storage: coven_github_config::StorageConfig::default(), + memory: coven_github_config::MemoryConfig::default(), } } @@ -1733,6 +1774,7 @@ mod disposition_tests { pr_body: "body".to_string(), review: ReviewResult::none(), exit_reason: None, + memory_used: None, }; let done = final_status_body(&config, "task-42", &result, Some(17)); assert!(done.starts_with("Status: done")); @@ -1793,6 +1835,7 @@ mod process_tests { }], review: coven_github_config::ReviewConfig::default(), storage: coven_github_config::StorageConfig::default(), + memory: coven_github_config::MemoryConfig::default(), } } @@ -2066,6 +2109,7 @@ exit 0 familiars: vec![familiar.clone()], review: coven_github_config::ReviewConfig::default(), storage: coven_github_config::StorageConfig::default(), + memory: coven_github_config::MemoryConfig::default(), }; let task = Task { id: "task-pub".to_string(), @@ -2291,6 +2335,7 @@ exit 0 }], review: coven_github_config::ReviewConfig::default(), storage: coven_github_config::StorageConfig::default(), + memory: coven_github_config::MemoryConfig::default(), }; let task = Task { id: "task-stale".to_string(), @@ -2424,6 +2469,7 @@ mod command_and_marker_tests { }], review: coven_github_config::ReviewConfig::default(), storage: coven_github_config::StorageConfig::default(), + memory: coven_github_config::MemoryConfig::default(), } } diff --git a/crates/worker/src/memory.rs b/crates/worker/src/memory.rs new file mode 100644 index 0000000..d3340d6 --- /dev/null +++ b/crates/worker/src/memory.rs @@ -0,0 +1,398 @@ +//! Hosted memory governance policy (issue #6, `docs/memory-contract.md`). +//! +//! The adapter is the policy authority: it computes a [`MemoryPolicy`] from +//! installation policy, repository, and actor trust, stamps it into the session +//! brief, and — crucially — **re-validates** what the runtime reports it did +//! against that same policy. A runtime bug or compromise therefore cannot +//! silently exceed the grant, because [`validate_memory_used`] rejects any +//! read or write the policy did not authorize before it is persisted. + +use serde::Serialize; + +use coven_github_api::MemoryUsed; + +/// Actor trust level, computed by the adapter — never inferred by the runtime. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] +#[serde(rename_all = "snake_case")] +pub enum TrustScope { + /// admin/maintain/write on the target repo (see the #13 permission gate). + Maintainer, + /// triage/read collaborator. + Collaborator, + /// Non-collaborator commenter. + External, + /// Pull request whose head is a fork — untrusted content. + ForkPr, + /// Branch Gardener / cron (#14): system-authored, no untrusted input. + Scheduled, +} + +/// A memory namespace. Never a raw path — keys are resolved under a namespace +/// and MUST be prefixed by the owning coordinates (see [`validate_memory_used`]). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] +#[serde(rename_all = "snake_case")] +pub enum MemoryScope { + /// This repository's memory. + Repo, + /// Cross-repo memory within one installation. + TenantShared, + /// Cross-tenant — never granted in hosted mode by default. + FamiliarGlobal, +} + +impl MemoryScope { + fn parse(s: &str) -> Option { + match s { + "repo" => Some(MemoryScope::Repo), + "tenant_shared" => Some(MemoryScope::TenantShared), + "familiar_global" => Some(MemoryScope::FamiliarGlobal), + _ => None, + } + } +} + +/// The computed policy the adapter stamps into the brief and validates against. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct MemoryPolicy { + pub enabled: bool, + pub installation_id: u64, + /// `owner/name` — the addressing authority for `repo`-scoped keys. + pub repo: String, + pub trust_scope: TrustScope, + pub read_scopes: Vec, + pub write_scopes: Vec, + pub approval_required: bool, + #[serde(skip_serializing_if = "Option::is_none")] + pub retention_days: Option, +} + +/// Inputs the adapter derives before computing a policy. +pub struct PolicyInputs<'a> { + /// Installation policy opt-in for this repo. `false` → deny by default. + pub enabled: bool, + pub installation_id: u64, + /// `owner/name`. + pub repo: &'a str, + pub trust: TrustScope, + pub approval_required: bool, + pub retention_days: Option, +} + +/// Computes the memory policy for one invocation, **deny-by-default**: returns +/// `None` when memory is not opted in, so an omitted brief block always means +/// "no memory", never "unrestricted". When enabled, read/write scopes are +/// granted by trust level per the contract's trust table. +pub fn compute_policy(input: PolicyInputs) -> Option { + if !input.enabled { + return None; + } + let (read_scopes, write_scopes) = grants(input.trust); + // A collaborator may propose but every write needs approval; fork/external + // never write. Force approval on whenever the trust level demands it, so a + // permissive config cannot loosen the trust rule. + let approval_required = input.approval_required || approval_forced(input.trust); + Some(MemoryPolicy { + enabled: true, + installation_id: input.installation_id, + repo: input.repo.to_string(), + trust_scope: input.trust, + read_scopes, + write_scopes, + approval_required, + retention_days: input.retention_days, + }) +} + +/// Default `(read, write)` namespace grants per trust level (contract's table). +/// The load-bearing rule: `ForkPr` and `External` get **no** write scope, so a +/// hostile fork PR can never poison durable memory. +fn grants(trust: TrustScope) -> (Vec, Vec) { + use MemoryScope::{Repo, TenantShared}; + match trust { + TrustScope::Maintainer => (vec![Repo, TenantShared], vec![Repo]), + TrustScope::Collaborator => (vec![Repo], vec![Repo]), + TrustScope::External => (vec![], vec![]), + TrustScope::ForkPr => (vec![Repo], vec![]), + TrustScope::Scheduled => (vec![Repo], vec![Repo]), + } +} + +/// Trust levels whose writes must always be approval-gated regardless of config. +fn approval_forced(trust: TrustScope) -> bool { + matches!(trust, TrustScope::Collaborator) +} + +/// Why the adapter refused a memory read or write. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct MemoryRejection { + pub op: MemoryOp, + /// The offending key or id. + pub target: String, + pub reason: String, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum MemoryOp { + Read, + Write, +} + +/// Re-validates the runtime's reported memory activity against `policy`. +/// Returns every read or write the adapter refuses; an empty vec means the +/// report is fully within policy. Callers MUST NOT persist a rejected write. +/// +/// `looks_secret` decides whether a string still carries a credential after the +/// adapter's scrubbing — wired to the `redact` module by the caller so a token +/// can never enter durable memory. +pub fn validate_memory_used( + policy: &MemoryPolicy, + used: &MemoryUsed, + looks_secret: impl Fn(&str) -> bool, +) -> Vec { + let mut rejections = Vec::new(); + if !used.enabled { + return rejections; + } + + for entry in &used.read { + if let Some(reason) = check_access(policy, &entry.scope, &entry.id, &policy.read_scopes) { + rejections.push(MemoryRejection { + op: MemoryOp::Read, + target: entry.id.clone(), + reason, + }); + } + } + + for write in &used.proposed { + if let Some(reason) = check_access(policy, &write.scope, &write.key, &policy.write_scopes) { + rejections.push(MemoryRejection { + op: MemoryOp::Write, + target: write.key.clone(), + reason, + }); + continue; + } + if looks_secret(&write.summary) || looks_secret(&write.key) { + rejections.push(MemoryRejection { + op: MemoryOp::Write, + target: write.key.clone(), + reason: "write still carries a credential after redaction".to_string(), + }); + continue; + } + if policy.approval_required && write.approval != "pending" { + rejections.push(MemoryRejection { + op: MemoryOp::Write, + target: write.key.clone(), + reason: format!( + "approval is required but write is '{}', not 'pending'", + write.approval + ), + }); + } + } + + rejections +} + +/// Checks one key/id against the granted scopes and its required prefix. +/// Returns `Some(reason)` on refusal, `None` when allowed. +fn check_access( + policy: &MemoryPolicy, + scope: &str, + target: &str, + granted: &[MemoryScope], +) -> Option { + let Some(scope) = MemoryScope::parse(scope) else { + return Some(format!("unknown memory scope '{scope}'")); + }; + if !granted.contains(&scope) { + return Some(format!("scope '{scope:?}' is not granted by policy")); + } + let prefix = required_prefix(policy, scope); + if !target.starts_with(&prefix) { + return Some(format!( + "key is not addressable under this invocation's scope (expected prefix '{prefix}')" + )); + } + None +} + +/// The mandatory key prefix that ties a memory entry to its owning coordinates, +/// so inspect/revoke-by-installation/repo is mechanically possible. +fn required_prefix(policy: &MemoryPolicy, scope: MemoryScope) -> String { + match scope { + MemoryScope::Repo => format!("repo/{}/", policy.repo), + MemoryScope::TenantShared => format!("tenant/{}/", policy.installation_id), + MemoryScope::FamiliarGlobal => "familiar/".to_string(), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use coven_github_api::{MemoryEntryRef, ProposedMemory}; + + fn inputs(trust: TrustScope, enabled: bool) -> PolicyInputs<'static> { + PolicyInputs { + enabled, + installation_id: 42, + repo: "acme/billing", + trust, + approval_required: false, + retention_days: Some(365), + } + } + + fn never_secret(_: &str) -> bool { + false + } + + fn proposed(scope: &str, key: &str, approval: &str) -> ProposedMemory { + ProposedMemory { + key: key.to_string(), + summary: "a fact".to_string(), + scope: scope.to_string(), + approval: approval.to_string(), + } + } + + fn used_with(proposed: Vec, read: Vec) -> MemoryUsed { + MemoryUsed { + enabled: true, + read, + proposed, + rejected: vec![], + } + } + + #[test] + fn memory_disabled_yields_no_policy() { + assert!(compute_policy(inputs(TrustScope::Maintainer, false)).is_none()); + } + + #[test] + fn fork_pr_gets_read_but_never_write() { + let policy = compute_policy(inputs(TrustScope::ForkPr, true)).unwrap(); + assert_eq!(policy.read_scopes, vec![MemoryScope::Repo]); + assert!( + policy.write_scopes.is_empty(), + "a fork PR must never have a write scope" + ); + + // A fork that proposes a durable write is refused. + let used = used_with( + vec![proposed("repo", "repo/acme/billing/conventions/x", "pending")], + vec![], + ); + let rejections = validate_memory_used(&policy, &used, never_secret); + assert_eq!(rejections.len(), 1); + assert_eq!(rejections[0].op, MemoryOp::Write); + } + + #[test] + fn external_actor_gets_nothing() { + let policy = compute_policy(inputs(TrustScope::External, true)).unwrap(); + assert!(policy.read_scopes.is_empty()); + assert!(policy.write_scopes.is_empty()); + } + + #[test] + fn maintainer_write_within_repo_scope_is_accepted() { + let policy = compute_policy(inputs(TrustScope::Maintainer, true)).unwrap(); + let used = used_with( + vec![proposed("repo", "repo/acme/billing/conventions/rounding", "pending")], + vec![MemoryEntryRef { + id: "repo/acme/billing/conventions/tables".to_string(), + scope: "repo".to_string(), + }], + ); + assert!(validate_memory_used(&policy, &used, never_secret).is_empty()); + } + + #[test] + fn cross_repo_key_is_rejected() { + let policy = compute_policy(inputs(TrustScope::Maintainer, true)).unwrap(); + // Correct scope, but the key points at a different repo. + let used = used_with( + vec![proposed("repo", "repo/acme/OTHER/conventions/x", "pending")], + vec![], + ); + let rejections = validate_memory_used(&policy, &used, never_secret); + assert_eq!(rejections.len(), 1); + assert!(rejections[0].reason.contains("addressable")); + } + + #[test] + fn write_to_ungranted_scope_is_rejected() { + let policy = compute_policy(inputs(TrustScope::Maintainer, true)).unwrap(); + // familiar_global is never granted. + let used = used_with( + vec![proposed("familiar_global", "familiar/cody/x", "pending")], + vec![], + ); + let rejections = validate_memory_used(&policy, &used, never_secret); + assert_eq!(rejections.len(), 1); + assert!(rejections[0].reason.contains("not granted")); + } + + #[test] + fn secret_bearing_write_is_rejected() { + let policy = compute_policy(inputs(TrustScope::Maintainer, true)).unwrap(); + let used = used_with( + vec![proposed("repo", "repo/acme/billing/notes/x", "pending")], + vec![], + ); + let rejections = validate_memory_used(&policy, &used, |_| true); + assert_eq!(rejections.len(), 1); + assert!(rejections[0].reason.contains("credential")); + } + + #[test] + fn collaborator_write_must_be_pending() { + let policy = compute_policy(inputs(TrustScope::Collaborator, true)).unwrap(); + assert!(policy.approval_required, "collaborator writes are always gated"); + + let auto = used_with( + vec![proposed("repo", "repo/acme/billing/notes/x", "auto")], + vec![], + ); + let rejections = validate_memory_used(&policy, &auto, never_secret); + assert_eq!(rejections.len(), 1); + assert!(rejections[0].reason.contains("approval is required")); + + let pending = used_with( + vec![proposed("repo", "repo/acme/billing/notes/x", "pending")], + vec![], + ); + assert!(validate_memory_used(&policy, &pending, never_secret).is_empty()); + } + + #[test] + fn cross_tenant_read_is_rejected() { + let policy = compute_policy(inputs(TrustScope::Maintainer, true)).unwrap(); + // tenant_shared is granted to maintainer, but the id is under another install. + let used = used_with( + vec![], + vec![MemoryEntryRef { + id: "tenant/999/shared/x".to_string(), + scope: "tenant_shared".to_string(), + }], + ); + let rejections = validate_memory_used(&policy, &used, never_secret); + assert_eq!(rejections.len(), 1); + assert_eq!(rejections[0].op, MemoryOp::Read); + } + + #[test] + fn disabled_memory_used_report_is_ignored() { + let policy = compute_policy(inputs(TrustScope::Maintainer, true)).unwrap(); + let used = MemoryUsed { + enabled: false, + proposed: vec![proposed("repo", "bogus", "auto")], + ..Default::default() + }; + assert!(validate_memory_used(&policy, &used, never_secret).is_empty()); + } +} diff --git a/crates/worker/src/redact.rs b/crates/worker/src/redact.rs index 205f6c9..ebbb411 100644 --- a/crates/worker/src/redact.rs +++ b/crates/worker/src/redact.rs @@ -230,6 +230,7 @@ mod tests { limitations: vec![poison("limitation")], }, exit_reason: None, + memory_used: None, }; sanitize_result(&mut result, &[tok]); diff --git a/docs/contracts/result.schema.json b/docs/contracts/result.schema.json index 4cae129..c5dd1de 100644 --- a/docs/contracts/result.schema.json +++ b/docs/contracts/result.schema.json @@ -142,6 +142,54 @@ "exit_reason": { "type": ["string", "null"], "enum": ["test_failure", "ambiguous_spec", "git_conflict", "infra_error", null] + }, + "memory_used": { + "type": ["object", "null"], + "description": "Runtime-reported memory activity for hosted governance (issue #6). Optional; omitted when the run did no memory work.", + "additionalProperties": false, + "required": ["enabled"], + "properties": { + "enabled": { "type": "boolean" }, + "read": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["id", "scope"], + "properties": { + "id": { "type": "string" }, + "scope": { "type": "string" } + } + } + }, + "proposed": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["key", "summary", "scope", "approval"], + "properties": { + "key": { "type": "string" }, + "summary": { "type": "string" }, + "scope": { "type": "string" }, + "approval": { "type": "string", "enum": ["pending", "applied", "auto"] } + } + } + }, + "rejected": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["summary", "scope", "reason"], + "properties": { + "summary": { "type": "string" }, + "scope": { "type": "string" }, + "reason": { "type": "string", "enum": ["pii", "secret", "out_of_scope", "low_confidence"] } + } + } + } + } } }, "allOf": [ diff --git a/docs/contracts/session-brief.schema.json b/docs/contracts/session-brief.schema.json index 60d7625..95846d7 100644 --- a/docs/contracts/session-brief.schema.json +++ b/docs/contracts/session-brief.schema.json @@ -111,6 +111,31 @@ } } } + }, + "memory_policy": { + "type": ["object", "null"], + "description": "Optional hosted memory governance policy (issue #6). Present only when the installation opted memory in for this repo; absent means memory is off. See docs/memory-contract.md.", + "additionalProperties": false, + "required": ["enabled", "installation_id", "repo", "trust_scope", "read_scopes", "write_scopes", "approval_required"], + "properties": { + "enabled": { "type": "boolean" }, + "installation_id": { "type": "integer" }, + "repo": { "type": "string" }, + "trust_scope": { + "type": "string", + "enum": ["maintainer", "collaborator", "external", "fork_pr", "scheduled"] + }, + "read_scopes": { + "type": "array", + "items": { "type": "string", "enum": ["repo", "tenant_shared", "familiar_global"] } + }, + "write_scopes": { + "type": "array", + "items": { "type": "string", "enum": ["repo", "tenant_shared", "familiar_global"] } + }, + "approval_required": { "type": "boolean" }, + "retention_days": { "type": ["integer", "null"] } + } } } }