Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions docs/coding-agent/lessons.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,28 @@ Purpose:

## Entries

## 2026-07-04 - Evidence Greps Must Match The Syntactic Class Being Ruled Out [tags: validation, grep-evidence]

Context:
- Plan: lint-suppression cleanup
- Task/Wave: Task_1 acceptance check
- Roles involved: Worker | Orchestrator

Symptom:
- Both the Orchestrator's inventory grep and the Worker's done-evidence grep for module-wide allows matched only single-lint forms (`allow(dead_code)`), missing two combined-form sites (`#![allow(dead_code, unused_imports)]`) — the task was reported done with its acceptance criterion unmet.

Root cause:
- The regex encoded the expected spelling of the attribute, not the syntactic class being ruled out.

Fix applied:
- Addendum cleaned the two missed files; completion evidence switched to `rg '^#!\[allow' src -n`, which matches every module-wide allow regardless of lint list.

Prevention:
- When acceptance is "no X remains", the evidence grep must match the broadest syntactic form of X (then narrower greps may summarize permitted exceptions). Encode the class, not the expected spelling.

Evidence:
- Task_1 report grep showed clean; Orchestrator's independent broad grep found embedded.rs/shared.rs; addendum report + corrected grep confirm only the two permitted files remain.

## 2026-07-04 - Verify Skip-Gating Predicate Branches Against What The Producer Actually Emits [tags: validation, review]

Context:
Expand Down
104 changes: 104 additions & 0 deletions docs/coding-agent/plans/completed/lint-suppression-cleanup-plan.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
# Plan: Remove or justify blanket lint suppressions

- status: done (Reviewer APPROVED 2026-07-04, zero findings)
- generated: 2026-07-04
- last_updated: 2026-07-04
- work_type: code

## Goal

- Eliminate module-wide `#![allow(unused_imports)]` and `#![allow(dead_code)]` suppressions so the compiler can see rot again; every surviving allow is item-level with a rationale comment (per repo lessons "Explain Temporary Suppressions" and "Prefer Root-Cause Fixes Over Symptom Patches").

## Definition of Done

- No module-wide (`#![allow(...)]`) lint suppressions remain in `src/` except where a documented structural reason survives review (expected: `test_support.rs`, possibly `reconciliation.rs`).
- Every remaining `#[allow(dead_code)]`/`#[allow(unused_imports)]` is item-level and carries a rationale + removal-condition comment.
- Genuinely dead code is deleted; unused imports removed.
- `cargo fmt --check`, `cargo check`, `cargo clippy --all-targets -- -D warnings`, `cargo test --no-run`, `cargo test` all pass.

## Scope / Non-goals

- Scope: the 26 inventoried suppression sites (2 unused_imports module-wide, 12 dead_code module-wide, 12 item/statement-level) and the code they hide.
- Non-goals: behavior changes; API additions; restructuring (the reorg is done); deleting roadmap-intended API surface.

## Context (workspace)

- Research: Task_9 review + post-completion architecture audit flagged these as rot-masking debt (debt items #8, #11-adjacent in the completed reorg plan's register). Inventory grep run 2026-07-04 (26 sites). Research dispatch waived: inventory is mechanical and prior audit context is fresh.
- Key judgment rule: ports/, models/, api DTO families contain deliberately pre-built surface for roadmap phases — for items unused today but API-shaped (pub(crate) types, trait methods, builders), NARROW the allow to the item with a rationale, do not delete. Delete only provably dead internals (private helpers with no callers, stale imports).
- Known-special sites: `src/test_support.rs` (shared fakes: different tests use different subsets — module-wide allow is structurally justified, keep with comment); `src/usecases/reconciliation.rs` (internal seam pending a governance surface per its own comments — narrow or keep with explicit rationale); `src/policy/graph_expansion.rs:20` (audit explicitly wants this narrowed).

## Open Questions (max 3)

- None.

## Assumptions

- A1: The compiler/clippy under `-D warnings` is the arbiter for what each removal exposes; the worker iterates per file.
- A2: Local full `cargo test` is currently viable (machine stall condition cleared 2026-07-04).

## Tasks

### Task_1: Remove/narrow all blanket suppressions and clean exposed rot
- type: impl
- owns:
- src/** (suppression attributes, unused imports, dead private code, rationale comments only — no logic changes)
- depends_on: []
- description: |
For each inventoried site: remove the module-wide allow; run cargo clippy --all-targets -- -D warnings;
triage each exposed warning: (a) delete unused imports; (b) delete provably dead PRIVATE code;
(c) for API-shaped or roadmap-intended items, add item-level #[allow(dead_code)] with a one-line
rationale + removal condition; (d) for test_support.rs (and reconciliation.rs if narrowing is
impractical) keep a module-wide allow with an explanatory comment. Report per-site outcomes
(removed / narrowed / kept-with-comment, and lines of dead code deleted).
- acceptance:
- Zero undocumented module-wide allows in src/
- All surviving allows are item-level (or documented module-level exceptions) with rationale comments
- No logic/behavior changes; test counts unchanged or reduced only by deleted dead test helpers
- validation:
- kind: command
required: true
owner: worker
detail: "cargo fmt --check && cargo check && cargo clippy --all-targets -- -D warnings && cargo test --no-run && cargo test"

### Task_2: Independent review
- type: review
- owns: []
- depends_on: [Task_1]
- description: |
Reviewer verifies: no deleted item was roadmap-intended API surface (spot-check deletions against
ports/models/api families); surviving allows have honest rationales; validation evidence complete.
- acceptance:
- Reviewer status APPROVED
- validation:
- kind: review
required: true
owner: reviewer
detail: "Diff review vs acceptance criteria + deletion-safety spot check"

## Task Waves (explicit parallel dispatch sets)

- Wave 1: [Task_1]
- Wave 2: [Task_2]

## Rollback / Safety

- Single-purpose branch refactor/lint-suppression-cleanup off merged main; pure-cleanup commit, trivially revertible.

## Progress Log (append-only)

- 2026-07-04: Plan drafted from suppression inventory (26 sites) + reorg audit debt register.
- 2026-07-04 Wave 1 completed: [Task_1 + addendum] (codex worker via agmsg)
- Summary: all module-wide suppressions removed except the two documented exceptions (test_support.rs shared fakes, reconciliation.rs dormant governance seam, both with rationale + removal conditions); API-shaped dormant seams narrowed to item-level allows with rationale/removal comments; unused imports cleaned; 19 lines of provably dead private helpers deleted (embedded.rs insert_quads/remove_quads, shared.rs graph_object_ref). Inventory gap: two combined-form allows (embedded.rs, shared.rs) were missed by both the plan inventory grep and the worker's done-evidence grep — caught by Orchestrator broad grep, fixed via addendum; lesson recorded.
- Validation evidence: fmt --check / check / clippy --all-targets -D warnings / test --no-run / FULL cargo test all pass twice (main pass + addendum): 338 lib + 25 integration passed, 0 failed, 3 ignored live smokes. Orchestrator independent broad grep + clippy pass.
- Notes: one attempted Qdrant constant deletion was self-reverted when all-target clippy showed test ownership — deletion-safety rule worked.
- 2026-07-04 Wave 2 completed: [Task_2]
- Reviewer verdict: APPROVED, zero findings. Independently confirmed: the three deleted helpers have zero callers on main (including tests) and no roadmap/doc references; only the two sanctioned module-wide allows remain (tests/ also clean); cfg_attr(not(test), allow(dead_code)) correctly applied to test-asserted items; diff contains no logic changes; fmt/clippy/test --no-run/test --lib pass independently.
- Non-blocking next-touch note: candidate_record.rs "test fixtures use selectively" allows could adopt the self-policing cfg_attr(not(test)) form.

## Decision Log (append-only; re-plans and major discoveries)

- 2026-07-04 Decision: Deletion-safety rule — API-shaped/roadmap-intended items get narrowed allows with rationale, never deletion; only provably dead private internals are removed.

## Notes

- Risks: deleting something a service-gated integration path uses (mitigated: full cargo test now runs locally); allow removal cascading into large diffs in ports/models (mitigated: narrowing is always available).
8 changes: 6 additions & 2 deletions src/adapters.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
#![allow(unused_imports)]

pub(crate) mod openai;
pub(crate) mod oxigraph;
pub(crate) mod qdrant;
pub(crate) mod stats;

pub(crate) use openai::OpenAIEmbeddingProvider;
// Composition modes may select either graph adapter; remove when both are referenced in every target.
#[allow(unused_imports)]
pub(crate) use oxigraph::{OxigraphGraphAuthorityStore, OxigraphHttpGraphAuthorityStore};
pub(crate) use qdrant::QdrantVectorCandidateStore;
// Stats adapters are selected by composition/tests in different target sets; remove when target use converges.
#[allow(unused_imports)]
pub(crate) use stats::{InMemoryRetrievalStatsStore, SqliteRetrievalStatsStore};

#[cfg(test)]
// Unit tests share this adapter through the barrel selectively; remove when tests import stats:: directly.
#[allow(unused_imports)]
pub(crate) use stats::noop_retrieval_stats_store;
46 changes: 14 additions & 32 deletions src/adapters/oxigraph/embedded.rs
Original file line number Diff line number Diff line change
@@ -1,37 +1,29 @@
#![allow(dead_code, unused_imports)]

use std::collections::{HashMap, HashSet};
use std::fs;
use std::path::Path;
use std::sync::{Mutex, MutexGuard};
use std::sync::Mutex;

use async_trait::async_trait;
use chrono::{DateTime, Utc};
use oxigraph::model::{GraphName, Literal, NamedNode, NamedOrBlankNode, Quad, Term};
use oxigraph::model::{GraphName, NamedNode, Quad};
#[cfg(test)]
use oxigraph::model::{Literal, NamedOrBlankNode, Term};
use oxigraph::store::Store;
use serde::{de::DeserializeOwned, Deserialize};

use crate::api::types::{
graph_uri, DerivedMemory, Entity, Episode, MemoryId, MemoryLink, MemoryObject, MemoryThread,
ObjectType, Observation, RelationType,
};
use crate::api::types::{graph_uri, DerivedMemory, MemoryLink, MemoryObject, ObjectType};
use crate::errors::CustomError;
use crate::policy::graph_expansion::{
bounded_expansion, derived_memories_by_provenance, derived_memories_by_thread,
};
use crate::policy::graph_expansion::{
bounded_incident_link_refs, graph_expansion_bounded_error, BoundedExpansionLinkRef,
};
use crate::ports::graph_authority::{
GraphAuthorityStore, GraphDerivedMemoryProvenanceQuery, GraphDerivedMemoryThreadQuery,
GraphExpansion, GraphExpansionBoundedFailure, GraphExpansionBoundedFailureReason,
GraphExpansionQuery, GraphObjectQuery, GraphObjectRef,
GraphExpansion, GraphExpansionQuery, GraphObjectQuery, GraphObjectRef,
};

use super::rdf_mapping::{rdf_triples_for_link, rdf_triples_for_object, RdfObject, RdfTriple};
#[cfg(test)]
use super::rdf_mapping::RdfObject;
use super::rdf_mapping::{rdf_triples_for_link, rdf_triples_for_object, RdfTriple};
use super::shared::*;
use super::sparql_selectors::{SparqlGraphSelectors, SparqlLinkRef};
use super::vocabulary as vocab;
use super::sparql_selectors::SparqlGraphSelectors;

pub(crate) struct OxigraphGraphAuthorityStore {
pub(crate) store: Store,
Expand Down Expand Up @@ -67,10 +59,14 @@ impl OxigraphGraphAuthorityStore {
})
}

// Embedded adapter tests assert graph mutation counts; remove when tests assert via graph queries only.
#[cfg_attr(not(test), allow(dead_code))]
pub(crate) fn triple_count(&self) -> Result<usize, CustomError> {
Ok(self.store.iter().count())
}

// Embedded adapter tests simulate graph corruption directly; remove when those tests use public writes only.
#[cfg_attr(not(test), allow(dead_code))]
pub(crate) fn replace_triples(
&self,
owner_graph_uri: String,
Expand Down Expand Up @@ -112,20 +108,6 @@ impl OxigraphGraphAuthorityStore {
Ok(())
}

fn insert_quads(&self, quads: &[Quad]) -> Result<(), CustomError> {
for quad in quads {
self.store.insert(quad).map_err(oxigraph_error)?;
}
Ok(())
}

fn remove_quads(&self, quads: &[Quad]) -> Result<(), CustomError> {
for quad in quads {
self.store.remove(quad).map_err(oxigraph_error)?;
}
Ok(())
}

fn quads_in_graph(&self, owner_graph_uri: &str) -> Result<Vec<Quad>, CustomError> {
let graph_name = GraphName::NamedNode(NamedNode::new(owner_graph_uri)?);
self.store
Expand Down
24 changes: 6 additions & 18 deletions src/adapters/oxigraph/http.rs
Original file line number Diff line number Diff line change
@@ -1,36 +1,24 @@
#![allow(unused_imports)]

use std::collections::{HashMap, HashSet};
use std::fs;
use std::path::Path;
use std::sync::{Mutex, MutexGuard};
use std::collections::HashSet;

use async_trait::async_trait;
use chrono::{DateTime, Utc};
use oxigraph::model::{GraphName, Literal, NamedNode, NamedOrBlankNode, Quad, Term};
use oxigraph::model::Quad;
use oxigraph::store::Store;
use serde::{de::DeserializeOwned, Deserialize};

use crate::api::types::{
graph_uri, DerivedMemory, Entity, Episode, MemoryId, MemoryLink, MemoryObject, MemoryThread,
ObjectType, Observation, RelationType,
graph_uri, DerivedMemory, MemoryId, MemoryLink, MemoryObject, ObjectType, RelationType,
};
use crate::errors::CustomError;
use crate::policy::graph_expansion::{
bounded_expansion, derived_memories_by_provenance, derived_memories_by_thread,
};
use crate::policy::graph_expansion::{
bounded_incident_link_refs, graph_expansion_bounded_error, BoundedExpansionLinkRef,
};
use crate::policy::graph_expansion::{bounded_incident_link_refs, BoundedExpansionLinkRef};
use crate::ports::graph_authority::{
GraphAuthorityStore, GraphDerivedMemoryProvenanceQuery, GraphDerivedMemoryThreadQuery,
GraphExpansion, GraphExpansionBoundedFailure, GraphExpansionBoundedFailureReason,
GraphExpansionQuery, GraphObjectQuery, GraphObjectRef,
GraphExpansion, GraphExpansionQuery, GraphObjectQuery, GraphObjectRef,
};

use super::rdf_mapping::{rdf_triples_for_link, rdf_triples_for_object, RdfObject, RdfTriple};
use super::rdf_mapping::{rdf_triples_for_link, rdf_triples_for_object};
use super::shared::*;
use super::sparql_selectors::{SparqlGraphSelectors, SparqlLinkRef};
use super::vocabulary as vocab;

pub(crate) struct OxigraphHttpGraphAuthorityStore {
Expand Down
2 changes: 0 additions & 2 deletions src/adapters/oxigraph/rdf_mapping.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
// RDF mapping surface. Public domain objects stay independent from
// Oxigraph types.
#![allow(dead_code)]

use chrono::{DateTime, SecondsFormat, Utc};
use serde::Serialize;

Expand Down
22 changes: 5 additions & 17 deletions src/adapters/oxigraph/shared.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
#![allow(dead_code, unused_imports)]

use std::collections::{HashMap, HashSet};
use std::fs;
use std::path::Path;
use std::sync::{Mutex, MutexGuard};

use async_trait::async_trait;
use chrono::{DateTime, Utc};
use oxigraph::model::{GraphName, Literal, NamedNode, NamedOrBlankNode, Quad, Term};
use oxigraph::store::Store;
Expand All @@ -16,19 +11,15 @@ use crate::api::types::{
ObjectType, Observation, RelationType,
};
use crate::errors::CustomError;
use crate::policy::graph_expansion::{
bounded_expansion, derived_memories_by_provenance, derived_memories_by_thread,
};
use crate::policy::graph_expansion::{
bounded_incident_link_refs, graph_expansion_bounded_error, BoundedExpansionLinkRef,
};
use crate::ports::graph_authority::{
GraphAuthorityStore, GraphDerivedMemoryProvenanceQuery, GraphDerivedMemoryThreadQuery,
GraphExpansion, GraphExpansionBoundedFailure, GraphExpansionBoundedFailureReason,
GraphExpansionQuery, GraphObjectQuery, GraphObjectRef,
GraphExpansionBoundedFailure, GraphExpansionBoundedFailureReason, GraphExpansionQuery,
GraphObjectQuery, GraphObjectRef,
};

use super::rdf_mapping::{rdf_triples_for_link, rdf_triples_for_object, RdfObject, RdfTriple};
use super::rdf_mapping::{RdfObject, RdfTriple};
use super::sparql_selectors::{SparqlGraphSelectors, SparqlLinkRef};
use super::vocabulary as vocab;
impl BoundedExpansionLinkRef for SparqlLinkRef {
Expand Down Expand Up @@ -412,6 +403,8 @@ pub(super) struct SparqlBinding {
#[serde(rename = "type")]
kind: String,
pub(super) value: String,
// SPARQL JSON includes datatype metadata for literals; remove if parsing no longer preserves literal metadata.
#[allow(dead_code)]
datatype: Option<String>,
}

Expand Down Expand Up @@ -965,11 +958,6 @@ pub(super) fn bounded_graph_visible_refs(
})
}

pub(super) fn graph_object_ref(object: &MemoryObject) -> GraphObjectRef {
let (object_id, object_type) = object_identity(object);
GraphObjectRef::new(object_id, object_type)
}

pub(super) fn quad_for_triple(
owner_graph_uri: &str,
triple: &RdfTriple,
Expand Down
10 changes: 8 additions & 2 deletions src/adapters/oxigraph/sparql_selectors.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
// Internal SPARQL selectors for the embedded Oxigraph authority. These helpers
// return backend-neutral IDs/refs; canonical object hydration reads RDF state.
#![allow(dead_code)]

use std::collections::HashSet;

use oxigraph::model::Term;
Expand Down Expand Up @@ -32,6 +30,8 @@ pub(crate) struct SparqlLinkRef {
}

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
// Lifecycle predicate metadata is reserved for governance diagnostics; remove if lifecycle SPARQL inspection is retired.
#[allow(dead_code)]
pub(crate) struct LifecycleCurrentnessPredicates {
pub(crate) retention_state: &'static str,
pub(crate) is_current: &'static str,
Expand Down Expand Up @@ -160,6 +160,8 @@ impl<'a> SparqlGraphSelectors<'a> {
)
}

// Entity-scoped derived-memory selectors are reserved for governance diagnostics; remove if that surface drops entity filters.
#[allow(dead_code)]
pub(crate) fn select_derived_memories_by_entity(
&self,
entity_ids: &[MemoryId],
Expand All @@ -181,6 +183,8 @@ impl<'a> SparqlGraphSelectors<'a> {
)
}

// Lifecycle predicate metadata is reserved for governance diagnostics; remove if lifecycle SPARQL inspection is retired.
#[allow(dead_code)]
pub(crate) const fn lifecycle_currentness_predicates() -> LifecycleCurrentnessPredicates {
LifecycleCurrentnessPredicates {
retention_state: vocab::RETENTION_STATE,
Expand All @@ -191,6 +195,8 @@ impl<'a> SparqlGraphSelectors<'a> {
}
}

// Supersession selectors are reserved for governance diagnostics; remove if lifecycle reconciliation stops using SPARQL.
#[allow(dead_code)]
pub(crate) fn select_superseded_derived_memory_ids(
&self,
) -> Result<Vec<MemoryId>, CustomError> {
Expand Down
Loading
Loading