Skip to content
Open
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
4 changes: 4 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,10 @@ data.json
.orgii/*
!.orgii/skills/
.orgii/skills/*
!.orgii/skills/architecture-audit/
!.orgii/skills/architecture-audit/**
!.orgii/skills/e2e-testing/
!.orgii/skills/e2e-testing/**
!.orgii/skills/org2-performance-guard/
!.orgii/skills/org2-performance-guard/**
!.orgii/skills/react-best-practices/
Expand Down
593 changes: 30 additions & 563 deletions .orgii/skills/architecture-audit/SKILL.md

Large diffs are not rendered by default.

58 changes: 58 additions & 0 deletions .orgii/skills/architecture-audit/references/acceptance-criteria.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
# Architecture Audit Acceptance Criteria

## Core Principle: Acceptance Criteria First

Before writing any plan, define the **completion checklist** — measurable criteria the codebase must satisfy when done. Every phase must map to at least one checklist item.

```
- [ ] Zero compiler warnings (cargo check / tsc --noEmit)
- [ ] Zero clippy warnings (cargo clippy --all-targets)
- [ ] Zero hardcoded domain strings (grep for known patterns)
- [ ] Zero duplicate type definitions across modules
- [ ] Zero layer violations (lower layers do not import upper layers)
- [ ] All files under size limit (per workspace rules)
- [ ] No backward-compat shims remaining (grep "compat", "legacy", "backward")
- [ ] Pre-user schema changes modify canonical DDL directly; no `ALTER TABLE`, legacy rebuilds, or migration tests unless explicitly requested
- [ ] No duplicate logic patterns (manual audit of init/setup/registration flows)
- [ ] No unused pub items (compiler warnings or manual grep)
- [ ] Term overloading table complete (Layer 4)
- [ ] Default branch analysis complete (Layer 5)
- [ ] Core modules free of variant-specific leakage (Layer 6)
- [ ] Wire payloads inspected for bloat/unwanted fields (Layer 8)
- [ ] All entry points perform identical init steps — comparison matrix complete (Layer 9)
- [ ] Multi-field resolvers use symmetric fallback chains — fallback matrix complete (Layer 10)
- [ ] Every found issue class has been swept globally, not just fixed at the reported site
- [ ] No types alive only in definition + re-export + test chains (Layer 2 call-chain trace)
- [ ] No cross-module naming collisions (same type name, different fields)
- [ ] No config structs spanning multiple unrelated domains (embedding + learnings + model selection in one struct)
- [ ] No background subsystems calling full session resolvers that enforce model-presence invariants
- [ ] No `expect()` on fallback paths that share the same failure mode as the primary path
- [ ] Session-layer decisions (LLM model, account) stay in session records, not agent config layer
- [ ] User-visible control actions have one dispatcher/source of truth, not UI-side duplicate send/cancel paths
- [ ] Runtime-completed assistant output is written to the authoritative EventStore, not only broadcast over transient UI channels
- [ ] Cancel APIs distinguish user Stop from programmatic Force Send so one path cannot poison the next turn
- [ ] Long-running orchestration surfaces reconcile finality from durable state, not from optimistic UI/session assumptions
- [ ] Run status, session status, task status, and member activity are asserted as separate dimensions
- [ ] No ownerless `in_progress`/claimed work can be persisted; if open work remains after all workers are terminal, the run is explicitly abandoned/failed/cancelled, not running
- [ ] Multi-agent task tools are role-aware: member self-claim is distinct from coordinator assignment, and recoverable misuse returns structured guidance rather than trajectory-visible execution errors
- [ ] Live orchestration context (task board, inbox, member activity) is marked volatile or revision-keyed; it is never hidden inside a stale stable prompt cache
- [ ] Rendered E2E for orchestration proves final outcome, durable invariants, prompt/context evidence, readable UI evidence, and absence of hidden tool-error trajectory leaks
- [ ] Rendered E2E does not use debug/helper endpoints as the side-effect path for the user-visible behavior under assertion; helpers may seed or inspect only
- [ ] Control/sentinel records (`redo:*`, batch envelopes, internal markers) are excluded from user-actionable UI registries and transcript input surfaces unless explicitly rendered as diagnostic metadata
- [ ] Team-mode / Agent Org member identity is sourced from runtime `member_id`/member name, not inferred from `agent_definition_id` or `agent_id` (one definition can back coordinator + multiple members)
- [ ] Drained inbox/mailbox messages are persisted as visible turn input before agent execution; LLM-only ephemeral attachments are not enough, and raw XML/internal payloads must not leak into the UI transcript
- [ ] Member turn completion maps to idle/available semantics, not terminal session completion; run finality must remain separate from per-turn member availability
- [ ] Task queue progress is event-driven: blocked assigned tasks are not notified early, dependency completion redispatches newly ready assigned tasks, and coordinator/cross-member tool calls cannot persist another member's work as `in_progress`
- [ ] Agent Org E2E asserts production inbox drain: unread member inbox rows must become visible turn input through the real member session path, and ready assigned open work must have either an active owner turn or unread wake row
- [ ] Adding a new E2E helper (`setTextarea`, custom drain endpoint, seeded snapshot helper, etc.) includes a sweep of all semantically matching call sites so old helpers do not keep driving the wrong DOM/runtime shape
- [ ] Turn finality has exactly one authoritative source (an FSM or equivalent monotonic state machine); `runtimeStatus` atoms, rendered events, heuristic timestamps, and streaming deltas are UI mirrors only and MUST NOT drive queue-flush decisions
- [ ] Every turn-ending signal (provider terminal, stream end, error, user Stop) carries a monotonically increasing generation counter; signals whose generation does not match the current turn are silently discarded
- [ ] The queue dispatcher reads a single gate (`turnPhase === "idle"`) — it does not read multiple atoms, boolean flags, or heuristic conditions to decide whether to send or queue
- [ ] User Stop and programmatic interrupt (Force Send cancel) travel separate code paths with explicit intent encoding; no shared cancel atom, flag, or default branch handles both simultaneously
- [ ] No separate "hold" atom or boolean flag shadows FSM state (e.g. "don't flush even if idle"); the FSM phase is the only source of truth for whether the queue may flush
- [ ] Provider events (stream end, tool call complete, error) are FSM *inputs*, not direct setters of `runtimeStatus`; the FSM transitions on them, the UI mirrors the FSM
- [ ] For every user-visible send/submit control, there is exactly one code path from button click to message dispatch; UI shortcut paths and background dispatcher paths that perform the same mutation are eliminated
- [ ] Atoms or flags that serve more than one concern (e.g. "signal user Stop" AND "gate draft restoration") are split; each concern has its own named atom with a single documented purpose
```

---
200 changes: 200 additions & 0 deletions .orgii/skills/architecture-audit/references/audit-layers.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,200 @@
# The 10-Layer Architecture Audit

## Contents

- Layers 1–3: compilation, dead code, and naming
- Layers 4–7: semantic overload, defaults, leakage, and developer confusion
- Layers 8–10: wire payloads, entry-point parity, and resolver symmetry

## The 10-Layer Audit

Every audit MUST cover all 10 layers. Previous failures came from only covering layers 1-3, then layers 1-7 (missing wire protocol and init parity), then layers 1-9 (missing resolver symmetry).

### Layer 1: Compilation Correctness

- Does it compile? (`cargo check`, `tsc --noEmit`)
- Zero warnings? (`cargo clippy --all-targets`)

### Layer 2: Dead Code & Structural Deduplication

- Duplicate functions/structs across modules?
- Parallel code paths doing the same work?
- Abstractions created but never wired into execution path?
- Types that only appear in definition + re-export chains + tests?

**Method: Call-Chain Tracing (not static grep)**

For each major entry point:

1. Identify entry point (e.g., "user sends message" -> Tauri command -> handler)
2. Trace forward: what functions does it call? What structs does it construct?
3. Mark every touched function/struct as "alive"
4. Everything NOT marked is a deletion candidate
5. For "alive" items: is the same work done in >1 place? -> duplication candidate

Static grep for `TODO`, `legacy`, `dead` only finds self-documented problems. It misses structs never instantiated, functions never called, and duplicate logic in parallel paths.

**CRITICAL: Reference counting is NOT a dead code audit.** A type with 15+ grep hits can still be dead if all hits are: (a) its own definition, (b) re-export chains (`types/mod.rs` → `session/mod.rs`), (c) internal conversion methods, and (d) tests that only exercise those conversions. Trace from **business entry points** (Tauri commands, API handlers, gateway dispatchers) forward — if no production code path constructs or consumes the type, it's dead. See anti-pattern #26.

### Layer 3: Naming Consistency

- Are renamed items updated everywhere?
- Old names still referenced in comments/strings?

### Layer 4: Semantic Overloading (CRITICAL — Often Missed)

**Search for the same word used with different meanings across the codebase.**

Method: Pick every domain term and search ALL usages. Build a table:

```
Term: "gateway"
Usage 1: ProviderSpec.is_gateway -> means API aggregator
Usage 2: AgentVariant::Gateway -> means message routing agent
Usage 3: GATEWAY_AGENT_TYPES -> means Azure cross-provider proxy
VERDICT: Rename usages 1 and 3 to avoid confusion
```

Common overloaded terms: gateway, session, channel, provider, context, runtime, config, state, manager, handler, bridge, proxy, client.

### Layer 5: Default Branch Analysis (CRITICAL — Often Missed)

**Find every `match` with `_ =>` or `else` catch-all and ask: "Is the default correct for ALL current and future variants?"**

Dangerous pattern:

```rust
match variant {
Sde => SdePromptBuilder,
_ => OsPromptBuilder, // Custom agents silently get OS identity!
}
```

Audit every:

- `match x { ..., _ => default }` — is the default truly universal?
- `if is_os { ... } else { ... }` — does the else work for Custom/Gateway/future variants?
- `unwrap_or(some_default)` — is the default always correct?

### Layer 6: Cross-Domain Concept Leakage (Often Missed)

**Check if domain-specific concepts leak into shared/core modules.**

Examples: `sde_config` field on shared `SessionRuntime`, hardcoded `AgentVariant::Os.agent_id()` in shared work item code, display labels "SDE Agent" hardcoded in shared aggregation code.

Method: For every file in `core/` or shared modules, grep for variant-specific terms. Each hit needs justification.

### Layer 7: "New Developer Confusion" Test (Often Missed)

Read the code as if you've never seen the codebase. For each function/struct:

1. Does the name accurately describe what it does?
2. Would a new developer understand this without tribal knowledge?
3. Are there misleading names that suggest a relationship that doesn't exist?

### Layer 8: Wire Protocol & Serialization Audit (CRITICAL — Added 2026-04)

**Check what the code ACTUALLY SENDS over the wire, not just what the source looks like.**

This layer was added after `schemars::openapi3()` silently injected `$schema`, `title`, `nullable`, and `default` fields into tool schemas. The Rust source looked perfectly reasonable — the problem was only visible in the serialized JSON output, and only triggered by a specific proxy resolving the `$schema` URL.

Method:

1. **Dump real payloads**: For every external API call (LLM, HTTP, WebSocket), add a temporary debug dump of the serialized body to a file. Inspect the actual bytes, not the source structs.
2. **Check schema generation libraries**: If using `schemars`, `serde_json::to_value`, or any schema generator, inspect the output for fields the target API does not expect (`$schema`, `title`, `nullable`, `default`, `examples`, `$ref`).
3. **Test against actual endpoints**: A payload that "should work" per the source code may fail at a proxy or gateway. Always verify with a real call, not just `cargo test`.
4. **Measure token impact**: For LLM APIs, check `prompt_tokens` in the response. If it's 10x higher than expected, the payload has hidden bloat.

Dangerous patterns:

```rust
// Looks fine in source, but openapi3() adds $schema URL, title, nullable
schemars::generate::SchemaSettings::openapi3()

// Fix: use draft07 with no meta_schema
schemars::generate::SchemaSettings::draft07()
.with(|s| { s.meta_schema = None; })
```

Checklist:

- Every `to_value()` / `to_string()` that crosses a network boundary: inspect the output
- Every schema generator: verify no unwanted fields in output
- Every proxy/gateway in the call chain: test with real payloads

### Layer 9: Init Parity Across Entry Points (Added 2026-04)

**Every entry point (production, test, E2E, API endpoint) must perform the SAME initialization steps.**

This layer was added after the E2E test endpoint (`/agent/test/sde`) skipped `AgentSession` registration, causing `init.rs` to miss definition-level disabled tools — but production code via Tauri commands did register it.

Method:

1. **List ALL entry points** that create or initialize a session:
- Tauri commands (production)
- HTTP API endpoints (gateway/test)
- Test helpers (`#[cfg(test)]`)
- CLI entry points
2. **For each entry point, list the initialization steps** it performs (in order)
3. **Build a comparison matrix**: rows = entry points, columns = init steps
4. **Every cell must be filled** — if an entry point skips a step, it needs explicit justification
5. **Missing steps are bugs**, not "simplifications for testing"

Dangerous pattern:

```rust
// Production path: registers definition, then inits session
state.register_session(agent_session).await;
ensure_session_initialized(&state, &session_id, &model).await;

// Test endpoint: skips registration, so init can't read definition
// This means disabled_tools from definition are never applied!
ensure_session_initialized(&state, &session_id, &model).await;
```

### Layer 10: Resolver Symmetry (Added 2026-04)

**When a single function resolves multiple fields using a priority chain (overrides → cache → DB → fallback), every field MUST follow the same chain unless there is an explicit, documented reason to diverge.**

This was found in `identity.rs` where `model` only checked overrides + runtime (2 layers), while `account_id` and `workspace_root` checked overrides + runtime + DB (3 layers). The DB always had a valid `model` (required at creation time), but the resolver skipped it — causing an error on app restart when the frontend lost its `lastModelSelectionAtom` and the in-memory runtime hadn't been initialised yet.

Method:

1. **Find every multi-field resolver** — functions that resolve N related fields from the same set of sources
2. **Build a fallback matrix**: rows = fields, columns = data sources. Mark which sources each field checks.
3. **Every cell should be filled** — if a field skips a source, ask "why doesn't field X check source Y?"
4. **Check the DB query trigger condition** — if the DB query is conditional (lazy), verify the condition accounts for ALL fields, not just a subset

Dangerous pattern:

```rust
// model checks 2 layers, account_id and workspace check 3 — asymmetric!
let model = overrides.model
.or_else(|| runtime.model.clone()); // stops here — no DB fallback
let model = model.ok_or("model is required")?; // errors on app restart

let account_id = overrides.account_id
.or_else(|| runtime.account_id.clone())
.or_else(|| db_record.account_id.clone()); // has DB fallback

// Fix: all fields follow the same chain
let model = overrides.model
.or_else(|| runtime.model.clone())
.or_else(|| db_record.model.clone()) // now symmetric
.ok_or("model is required")?;
```

Also watch for the DB query gate:

```rust
// BAD: gate only checks 2 of 3 fields — model miss won't trigger DB
let db_record = if account_id.is_none() || workspace.is_none() { query_db() }

// GOOD: gate checks all fields that may need DB fallback
let needs_db = model.is_none() || account_id.is_none() || workspace.is_none();
let db_record = if needs_db { query_db() }
```

Also audit for **dimension mismatch**: when a boolean flag (like `is_channel`) is used to branch behavior, check whether the flag's semantic dimension matches the actual requirement. Example: `is_channel_session` (dimension: "message source") was used to decide workspace path (dimension: "agent type"). OS Agent from the UI had no workspace — but `is_channel_session` was `false` for UI-launched sessions, so it hit the wrong branch.

---
Loading
Loading